Inject defaults into the planConnection query in the GTFS GraphQL schema

[optionsome]

Summary

Inject defaults (either from code or configuration) into the planConnection query in the GTFS GraphQL schema that is outputted through introspection.

This pr also allows to reset searchWindow as null in the planConnection query (in case it has been configured on server).

Issue

No issue

Unit tests

Added tests.

Documentation

In schema

Changelog

From title

Bumping the serialization version id

Not needed

[optionsome]

We need to decide if using a directive makes sense or should we just use the input type and field names as keys for the translations. Also, have to maybe figure out how to not construct the schema on each request.

[leonardehrenfried]

Can you resolve the conflicts?

[codecov]

Codecov Report

Attention: Patch coverage is 90.86294% with 18 lines in your changes missing coverage. Please review.

Project coverage is 69.82%. Comparing base (72019c0) to head (76db9b5).
Report is 53 commits behind head on dev-2.x.

Files with missing lines	Patch %	Lines
...ner/standalone/configure/ConstructApplication.java	0.00%	6 Missing ⚠️
...a/org/opentripplanner/apis/gtfs/SchemaFactory.java	95.87%	4 Missing ⚠️
...ntripplanner/apis/gtfs/configure/SchemaModule.java	0.00%	4 Missing ⚠️
...pentripplanner/apis/gtfs/DefaultValueInjector.java	97.56%	1 Missing and 1 partial ⚠️
...entripplanner/apis/gtfs/GraphQLRequestContext.java	0.00%	1 Missing ⚠️
...standalone/server/DefaultServerRequestContext.java	50.00%	1 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #6339      +/-   ##
=============================================
+ Coverage      69.79%   69.82%   +0.03%     
- Complexity     17943    17953      +10     
=============================================
  Files           2046     2049       +3     
  Lines          76685    76776      +91     
  Branches        7829     7833       +4     
=============================================
+ Hits           53520    53608      +88     
- Misses         20423    20425       +2     
- Partials        2742     2743       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[optionsome]

We need to decide if using a directive makes sense or should we just use the input type and field names as keys for the translations. Also, have to maybe figure out how to not construct the schema on each request.

We decided to not use directives (however directive wiring is used to inject the defaults). Used dependency injection to avoid constructing the schema on each request.

[leonardehrenfried]

If we say that we need the service, I'm fine with it. I don't think it is necessary though.

[t2gran]

Consider: A map and a bit of refactoring would make this code smaller and more readable. I switch might be faster but I doubt it. I would make builder for the map, so it would look like this:

 b = <new builder>;
 b
    // Add a requiered int
    .intReq("planConnection_first", defaultRouteRequest.numItineraries())
    // Add an optional string, since the argument is not a string the method dos the null check and calls `toString()`.
    .strOpt("planConnection_searchWindow", defaultRouteRequest.searchWindow())
    // Explicit more generic alternative
    .strOpt("planConnection_searchWindow", defaultRouteRequest.searchWindow(), Object::toString)

The comments are just to explain my code, I would strip those away and add JavaDoc on the metods instead.

[optionsome]

I did something like that in 76db9b5 . I left out the Object::toString and just called toString on the object, but I can change that if you want. I'm not sure if this is more readable for me since how the method calls get split on multiple lines sometimes, but it's not much less readable either.

[t2gran]

I left out the Object::toString and just called toString on the object
That is perfectly fine, I only did it to make it possible to provide a custom toString().

I would probably try to get ridd of the method chain(preferences.bike().walking().speed()). A simple way to do it is to group the calls and define varables and make a small change to the builder. There is two way to create an "inner" builder, you chose the one you like best. One restrict the scope on(type, lambda) - is safer, the other on(type) : Builder is make the code more compact and more readable.

Like this

    var defaultRequest = ...;
var builder = new DefaultMappingBuilder()
      .intReq("planConnection.first", defaultRequest.numItineraries())
      .stringOpt("planConnection.searchWindow", defaultRequest.searchWindow());

    // Map Bike preferences
    {
      var bike = defaultRequest.preferences().bike();

      // EXAMPLE WITH LAMBDA
      builder
        .on(
          "BicycleParkingPreferencesInput",
          b ->
            b.intReq(
              "unpreferredCost",
              bike.parking().unpreferredVehicleParkingTagCost().toSeconds()
            )
        )
        .on(
          "BicyclePreferencesInput",
          b ->
            b
              .intReq("boardCost", bike.boardCost())
              .floatReq("reluctance", bike.reluctance())
              .floatReq("speed", bike.speed())
        );
      {
        var walking = bike.walking();
        // EXAMPLE RETURNING THE SCOPED BUILDER
        builder.on("BicycleWalkPreferencesCostInput")
          .intReq("mountDismountCost", walking.mountDismountCost().toSeconds())
          .floatReq("reluctance", walking.reluctance());

        builder.on("BicycleWalkPreferencesInput")
          .stringReq("mountDismountTime", walking.mountDismountTime())
          .floatReq("speed", walking.speed());
      }
      {
        var r = bike.rental();
        builder.on("DestinationBicyclePolicyInput")
          .boolReq("allowKeeping", r.allowArrivingInRentedVehicleAtDestination())
          .intReq("keepingCost", r.arrivingInRentalVehicleAtDestinationCost().toSeconds());
      }
    }
  }

  private static class DefaultMappingBuilder {

    private final Map<String, Value> defaultValueForKey;
    private final String typePrefix;

    private DefaultMappingBuilder(Map<String, Value> defaultValueForKey, String typePrefix) {
      this.defaultValueForKey = defaultValueForKey;
      this.typePrefix = typePrefix;
    }

    public DefaultMappingBuilder() {
      this(new HashMap<String, Value>(), "");
    }


    public DefaultMappingBuilder intReq(String key, int value) {
      return put(key, IntValue.of(value));
    }

    public DefaultMappingBuilder floatReq(String key, double value) {
      return put(key, FloatValue.of(value));
    }

    public DefaultMappingBuilder stringReq(String key, Object value) {
      return put(key, StringValue.of(value.toString()));
    }

    public DefaultMappingBuilder stringOpt(String key, @Nullable Object value) {
      return value == null ? null : put(key, StringValue.of(value.toString()));
    }

    public DefaultMappingBuilder boolReq(String key, boolean value) {
      return put(key, BooleanValue.of(value));
    }
    // Create a new builder and call lambda with it
    public DefaultMappingBuilder on(String type, Consumer<DefaultMappingBuilder> body) {
      body.accept(on(type));
      return this;
    }
    // Create new Builder and return it
    public DefaultMappingBuilder on(String type) {
      return new DefaultMappingBuilder(this.defaultValueForKey, type + ".");
    }

    private DefaultMappingBuilder put(String key, Value value) {
      defaultValueForKey.put(typePrefix + key, value);
      return this;
    }

    public Map<String, Value> build() {
      return defaultValueForKey;
    }
  }

[t2gran]

Is the method name correct? To me it looks like this only validate the schema. The schema used else where is created by Dagger? I am not particular found of this - this defeats part of the point of DI. Instead I think there should be unit-tests to test schema creation. Not sure what can go wrong when you start the server and create the schema based on custom config - that is not testable. Also, the schema should be created eagerly (not lazy) by Dagger and the server should go down almost tha same way as you enforce here - or did I miss something?

[optionsome]

I'm pretty sure that I tested that this does initialize the schema and it is not recreated ever again. That's why I named it like this.

Since the schema creation depends on configuration now, I didn't want to end up in a situation where the server starts up but can't serve GraphQL traffic due to some NPE for example. Ofc, it's possible to avoid those types of issues in other ways as well (thorough unit testing, null checks and/or wrapping the default value creation in a try catch).

[leonardehrenfried]

Here is a stackoverflow for eager initialization of dagger: https://stackoverflow.com/questions/31101039/dagger-2-how-to-create-provide-a-eagersingleton

[optionsome]

Here is a stackoverflow for eager initialization of dagger: https://stackoverflow.com/questions/31101039/dagger-2-how-to-create-provide-a-eagersingleton

I couldn't figure out a nice way to do it without too much boilerplate. If we want, I can try to use one of the examples from leonard's link but as said, I feel like they make the already complicated setup more complicated.

[t2gran]

About eager init...

Our strategy is to fail fast and fix the root cause - in general we should NOT add a "second line of defence". The reason is that it create a lot of noice. @optionsome have you proven that it is possible for this to fail, without making a programming error first? If not, I lean toward getting rid of it.

If you want some safty, adding a unt test using the default RoutingRequest should cover most cases. If something is allowed to be null/unset then it is likely to be in the default.

By the way, if you move the schema construction from the ConstructApplicationFactory to LoadApplicationFactory the you will it will be "eagerly loaded" loaded before the web-server start. The downside is that if it fails it takes down the server even for someone who is not using the GTFS API.

[optionsome]

@optionsome have you proven that it is possible for this to fail, without making a programming error first? If not, I lean toward getting rid of it.

For it to fail, it needs a programming error somewhere, but error can be hidden until someone changes their configuration to have a rare value.

If you want some safety, adding a unit test using the default RoutingRequest should cover most cases. If something is allowed to be null/unset then it is likely to be in the default.

This has been already done and I've added unit tests for a few values of different "categories" of mapping directly for the mapping class.

By the way, if you move the schema construction from the ConstructApplicationFactory to LoadApplicationFactory the you will it will be "eagerly loaded" loaded before the web-server start. The downside is that if it fails it takes down the server even for someone who is not using the GTFS API.

Ok, I didn't think of this possibility. The schema is not constructed if the GTFS API is not enabled but there might be some case where the API is enabled but not used.