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 94.11765% with 13 lines in your changes missing coverage. Please review.

Project coverage is 69.87%. Comparing base (fe5c570) to head (8aa1ce0).

Files with missing lines	Patch %	Lines
...a/org/opentripplanner/apis/gtfs/SchemaFactory.java	96.00%	4 Missing ⚠️
...ntripplanner/apis/gtfs/configure/SchemaModule.java	0.00%	4 Missing ⚠️
...pentripplanner/apis/gtfs/DefaultValueInjector.java	98.14%	1 Missing and 1 partial ⚠️
...entripplanner/apis/gtfs/GraphQLRequestContext.java	0.00%	1 Missing ⚠️
...ner/standalone/configure/ConstructApplication.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.84%   69.87%   +0.03%     
- Complexity     18124    18146      +22     
=============================================
  Files           2069     2072       +3     
  Lines          77268    77387     +119     
  Branches        7855     7858       +3     
=============================================
+ Hits           53965    54073     +108     
- Misses         20546    20556      +10     
- Partials        2757     2758       +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;
    }
  }

[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.

[optionsome]

I was under the impression that arguments/input fields which accept lists couldn't have default values in GraphQL spec but it seems like it is possible at least on some level (transmodel API uses it). I might have skipped over some parameters because they were lists. I'll check how well the defaults are supported for lists and check if there are some additional input fields/parameters that I could inject defaults for.