feat: allow distinguishing between null and absent values in gql_code_builder

[knaeckeKami]

this is an attempt to solve issues like gql-dart/ferry#473 gql-dart/ferry#474 and gql-dart/ferry#219 . also related: google/built_value.dart#1123

It requires wrapping all nullable inputs and variables in a "Value" class so we make it explicit wether a field is explicitly set to null or just absent.

Then we generate a custom serializer for each input/var class that distinguish between a Value field that is null (will be absent in the json), is set to Value(null) (the key will be present in the json but the value will be null) or a present, non-null value like Value('Hello World') which will be serialized as String.

Most of the new logic here atm: https://github.com/gql-dart/gql/pull/381/files#diff-b581f94cad07799afdef5d265482e91644e305579526f4276d9d5388faf4c40d

[twklessor]

@knaeckeKami Thanks for working on this issue with literal null; what it's called in the GrapQL spec if I'm not mistaken (graphql/graphql-spec#83). How does one go about using these changes together with Ferry?

[knaeckeKami]

Hi!

ferry_generator internally uses gql_code_builder, so if you add dependency_overrides to use gql_code_builder from this commit, ferry_generator should pick it up.

It's possible that there are small changes required in ferry_generator, similar to the ones in gql_build (I don't remember the current state of this PR exactly)

note that I consider this as an experiment, which is untested and possibly incomplete

[twklessor]

@knaeckeKami Thanks! Unfortunately that seems to not work so I think you're right. Changes in ferry_generator are required. Also the Value wrapper is provided by the gql_exec so I think that's also needed in the dependency_overrides. I'll try and figure it out. Thanks for the hint!

I think it would make sense to have a branch for the ferry-specific changes required by this MR.

[knaeckeKami]

pushed changes to feat/null-experiment, it builds with the test schema ferry-test-graphql2

[twklessor]

@knaeckeKami Thanks so much! I must thank you and others for taking over as maintainers on this project! It really is very much appreciated! I'll try this out and report back!

[twklessor]

@knaeckeKami Sorry to bother you again! I can't seem to find the Value wrapper class (its not exported or included) and I can't reference gql_exec directly from the gql package together with the ferry branch you created. It results in a version solving failed;
Because lessor_api_client depends on ferry from git which depends on gql_exec >=0.4.0 <2.0.0, gql_exec from hosted is required.

[knaeckeKami]

I think you need to add dependency_overrides like this:

dependency_overrides:
  gql_exec:
    git:
      url: https://github.com/gql-dart/gql.git
      ref: feat/triStateNull
      path: links/gql_exec
  gql_code_builder:
    git:
      url: https://github.com/gql-dart/gql.git
      ref: feat/triStateNull
      path: codegen/gql_code_builder
  ferry_generator:
      url: https://github.com/gql-dart/ferry.git
      ref: feat/null-experiment
      path: packages/ferry_generator

[twklessor]

I have been looking at these warnings for quite some time and I simply cannot find the problem;

[WARNING] Configuring `ferry_generator:graphql_builder` in target `lessor_api_client:lessor_api_client` but this is not a known Builder
[WARNING] Configuring `ferry_generator:serializer_builder` in target `lessor_api_client:lessor_api_client` but this is not a known Builder

Nothing is generated. Which makes sense since it can't find the builders. I haven't changed anything else than the changes in the pubspec.yaml:

name: lessor_api_client
description: A client implementation for the Lessor API
version: 0.0.1

environment:
  sdk: '>=2.12.0 <3.0.0'

dependencies:
  json_annotation: ^4.6.0
  jwt_decoder: ^2.0.0
  ferry:
    git:
      url: https://github.com/gql-dart/ferry.git
      path: packages/ferry
      ref: feat/null-experiment
  gql_http_link: ^0.4.0-nullsafety.1
  mocktail: ^0.2.0

dependency_overrides:
  gql_exec:
    git:
      url: https://github.com/gql-dart/gql.git
      ref: feat/triStateNull
      path: links/gql_exec
  gql_code_builder:
    git:
      url: https://github.com/gql-dart/gql.git
      ref: feat/triStateNull
      path: codegen/gql_code_builder
  ferry_generator:
    git:
      url: https://github.com/gql-dart/ferry.git
      ref: feat/null-experiment
      path: packages/ferry_generator

dev_dependencies:
  build_runner: ^2.0.2
  json_serializable: ^6.2.0
  test: ^1.15.7
  intl: ^0.17.0

Here's my build.yaml;

targets:
  $default:
    builders:
      ferry_generator|graphql_builder:
        enabled: true
        options:
          schema: lessor_api_client|lib/graphql/schema.graphql
          type_overrides:
            LocalDate:
              name: DateTime
            LocalDateTime:
              name: DateTime
            BigDecimal:
              name: double
      ferry_generator|serializer_builder:
        enabled: true
        options:
          schema: lessor_api_client|lib/graphql/schema.graphql
          custom_serializers:
            - import: 'package:lessor_api_client/graphql/serializers/local_date_time_serializer.dart'
              name: DateTimeSerializer

[knaeckeKami]

you need to depend on ferry_generator in your dev_dependencies to make the builder visible. you can check out the https://github.com/gql-dart/ferry/blob/feat/null-experiment/packages/ferry_test_graphql2/pubspec.yaml for an example

[twklessor]

This feature works as intended. I know we have missing/failed tests. One thing that strikes me is that Value is always required regardless if inputs and variables are nullable or not. It seems the generated setters in the VarsBuilder's are always nullable although they aren't in my schema. So you end up with being required to wrap all inputs and variables with Value. That's not really desired in my opinion.

[knaeckeKami]

Can you clarify? Fields should only be wrapped in Value when they are nullable.

E.g. an input

# The input object sent when someone is creating a new review
input ReviewInput {
  # 0-5 stars
  stars: Int!
  # Comment about the movie, optional
  commentary: String
  # Favorite color, optional
  favorite_color: ColorInput
  # Date viewed
  seenOn: [Date]
}

generates

abstract class GReviewInput
    implements Built<GReviewInput, GReviewInputBuilder> {
  GReviewInput._();

  factory GReviewInput([Function(GReviewInputBuilder b) updates]) =
      _$GReviewInput;

  int get stars; // non- nullable, so not wrapped
   //rest of the fields are nullable, so they are wrapped
  _i1.Value<String>? get commentary;
  _i1.Value<GColorInput>? get favorite_color;
  _i1.Value<BuiltList<DateTime?>>? get seenOn;
  Map<String, dynamic> toJson() => (_i2.serializers.serializeWith(
        GReviewInput.serializer,
        this,
      ) as Map<String, dynamic>);
  static GReviewInput? fromJson(Map<String, dynamic> json) =>
      _i2.serializers.deserializeWith(
        GReviewInput.serializer,
        json,
      );
  @BuiltValueSerializer(custom: true, serializeNulls: true)
  static Serializer<GReviewInput> get serializer => GReviewInputSerializer();
}

I agree that this API is not pretty, but I did not come up with a better way yet.

[twklessor]

Scratch that! You're right! Agree that the API could be better. I'm wondering if it's better to create a custom serializer instead of using this draft API for now but it's very unfortunate as we will have to have many custom serializers.

Another consequence of this Value wrapper is that it changes the way you write requests. Check this example. The vars.pagination is of the type _i2.Value<_i1.GPaginationInput>? pagination which means you cannot write it the old way.
Old way;

GMileageRegistrationNumbersReq((b) => b
  ..vars.pagination.offset = 0
  ..vars.pagination.limit = 1)

Because of the wrapper you now have to write something like if I'm not mistaken;

GMileageRegistrationNumbersReq((b) => b
        ..vars.pagination = Value((GPaginationInputBuilder()
              ..limit = 5
              ..offset = 0)
            .build()))

Another way of thinking about this is the suggestion you made yourself. Having the possiblity to set in the build configuration that you want to always serialize null values. The @BuiltValueSerializer(serializeNulls: true) available with built_value you mentioned here; gql-dart/ferry#473 (comment)

In my case that would actually work as we almost always specifically set all fields (from a previous query with the fields) when we do a mutations etc..

Again I'm sorry to take your time but I think this is an important feature that needs to work with Ferry. I am contemplating if I could contribute myself; but I'm not well-versed on the inner workings of Ferry currently.

[knaeckeKami]

Another consequence of this Value wrapper is that it changes the way you write requests. Check this example. The vars.pagination is of the type _i2.Value<_i1.GPaginationInput>? pagination which means you cannot write it the old way.

Yes, this can potentially be fixed by making Value itself an instance of Built, so the nested syntax works again.

Another way of thinking about this is the suggestion you made yourself. Having the possiblity to set in the build configuration that you want to always serialize null values. The @BuiltValueSerializer(serializeNulls: true) available with built_value you mentioned here; gql-dart/ferry#473 (comment)

I'm hesitant to add this, because this would basically break any "update" mutations.

consider this schema:

input editUserInput {
   email: String
   firstName: String
   lastName: String
}

type Mutation {
   updateUser(input: editUserInput): User

mutation UpdateUser($input: editUserInput){
   updateUser(input: $input){ 
     id 
 9 
} 

when we set serializeNulls: true and call this mutation via

final req = GUpdateUserReq((b) => b..vars.email = "[email protected]");

it would set the firstname and lastname of the user to null.

this would likely by highly unexpected by users.

Again I'm sorry to take your time but I think this is an important feature that needs to work with Ferry. I am contemplating if I could contribute myself; but I'm not well-versed on the inner workings of Ferry currently.

Yes, I agree, and I would like to get to finish this feature, but it still needs some work (most importantly, to make this configurable so it's an opt-in feature in order to not break existing users, but also potentially making the API nicer if possible). I will probably not have time to get this done in the next few weeks.

[twklessor]

Yes that is certainly true! I think it still has its merits as an opt-in configuration with an appropiate disclaimer of sorts.

Today I had a look around the gql_code_builder and the BuilderConfig in ferry_generator. Succeeded in implementing this configuration option with the use of some of the code in this branch. I'll see how it works out in practice.

Thanks for the discussion and help!

[twklessor]

@knaeckeKami I have pushed changes to ferry_generator and gql_code_builder that support the new serialize_nulls build config option I mentioned. I have used your implementation as an reference and I'm honestly sure it could be better. Tests are still missing and I need help with this area. I haven't merged the latest upstream changes into these branches yet.

Here are the pull requests;
gql-dart/ferry#528
#403

As I said this serialize_nulls build config option makes a lot of sense in our team and I think others could benefit from it as an opt-in feature.

So where do we go from here? Do you agree on having this opt-in feature? What are your thoughts?

[knaeckeKami]

I finally got around to finishing this feature and adding tests.
I will merge and publish it.

@twklessor sorry for the late response. I think that a serialize_nulls option is not a good feature to have because it is very easy to use in the wrong way, especially if a properly typed alternative with makes the presence/absence of values explicit is available.

And, to be honest, I don't want to maintain an additional mode of code generation which increases to complexity of adding new features and adds more surface for bugs.

[nsanchezpc]

Hi @knaeckeKami, could you please provide some examples of how to use the tristate option? Our backend needs to distinguish between null and absent, and I'm having difficulty achieving that. I have the latest version of the ferry package and have added tristate_optionals: true to my build.yaml.
Thanks in advance!