Field Selection Merging demonstration of problem #19

potatosalad · 2024-04-19T15:50:30Z

See GraphQL Spec: 5.3.2 Field Selection Merging.

The attached test case currently fails:

- Expected  -  4
+ Received  + 10

  {
    hero: {
-    id?: STRING<ID>
+    id: STRING<ID>
-    properties?: {
+    id: {
+
+    }
+    properties: {
      x: VARINT{Int}
-     y?: STRING<String>
+     x: {
+
+     }
+     y: STRING<String>
-     z?: STRING<String>
+     z: STRING<String>
      }
    }?
  }

Here is the JSON dump of the wire type:

{
  "type": "RECORD",
  "fields": [
    {
      "name": "hero",
      "of": {
        "type": "NULLABLE",
        "of": {
          "type": "RECORD",
          "fields": [
            {
              "name": "id",
              "of": {
                "type": "BLOCK",
                "of": {
                  "type": "STRING"
                },
                "key": "ID",
                "dedupe": true
              },
              "omittable": false
            },
            {
              "name": "id",
              "of": {
                "type": "RECORD",
                "fields": []
              },
              "omittable": false
            },
            {
              "name": "properties",
              "of": {
                "type": "RECORD",
                "fields": [
                  {
                    "name": "x",
                    "of": {
                      "type": "BLOCK",
                      "of": {
                        "type": "VARINT"
                      },
                      "key": "Int",
                      "dedupe": false
                    },
                    "omittable": false
                  },
                  {
                    "name": "x",
                    "of": {
                      "type": "RECORD",
                      "fields": []
                    },
                    "omittable": false
                  },
                  {
                    "name": "y",
                    "of": {
                      "type": "BLOCK",
                      "of": {
                        "type": "STRING"
                      },
                      "key": "String",
                      "dedupe": true
                    },
                    "omittable": false
                  },
                  {
                    "name": "z",
                    "of": {
                      "type": "BLOCK",
                      "of": {
                        "type": "STRING"
                      },
                      "key": "String",
                      "dedupe": true
                    },
                    "omittable": false
                  }
                ]
              },
              "omittable": false
            }
          ]
        }
      },
      "omittable": false
    }
  ]
}

I think the expected wire type should look like this:

{
  hero: {
    id?: STRING<ID>
    properties?: {
      x: VARINT{Int}
      y?: STRING<String>
      z?: STRING<String>
    }
  }?
}?

msolomon · 2024-04-23T03:58:42Z

It turns out this is a bit tricky.

In the test case, I think the type we want is:

{
 hero: {
   id: STRING<ID>  # not omittable because it's selected directly from the interface which defines it
   properties?: {
    x?: VARINT{Int} # omittable because it should be "dangerous" but not "breaking" to add a new implementation of Character
    y?: STRING<String>
    z?: STRING<String>
   }
  }?
}

I put together a branch to take a stab at achieving this, but it has some issues--it serializes a few things differently, but also worse. In particular, I think a query like this:

    query {
      hero {
        ... on Droid {
          id
          properties {
            x
            y
          }
        }
      }
    }

should result in

{
 hero: {
   id?: STRING<ID> # omittable because it might not be Droid
   properties?: { # omittable because it might not be Droid
    x: VARINT{Int} # not omittable, because only Droid was selected. would be omittable again if Human were selected
    y: STRING<String>
   }
  }?
}

but it doesn't.

I think this all stems from one issue: the rules of omittability are implicit for JSON (and therefore not carefully specified), but not so for Argo. The algorithms in the GraphQL spec also aren't easy to alter to collect this information. Tight rules around omittability are helpful for the client experience, so probably some client code generators have run into this issue.

I'll think more about it, but I'm also open to any ideas you might have!

potatosalad · 2024-04-23T14:46:37Z

In the test case, I think the type we want is:

{
 hero: {
   id: STRING<ID>  # not omittable because it's selected directly from the interface which defines it
   properties?: {
    x?: VARINT{Int} # omittable because it should be "dangerous" but not "breaking" to add a new implementation of Character
    y?: STRING<String>
    z?: STRING<String>
   }
  }?
}

For hero.id:
- I think you might be right that id? is incorrect.
- See the logic below for what the Erlang code is currently doing. There may be a flaw in the logic.
For hero.properties.x:
- I think x? should still be x since the omittable behavior is actually covered by the parent properties?.
- When adding a new implementation of Character, then the parent properties? would never be called anyway because the object/interface type that contains properties? would never match on the TypeCondition.

The logic I followed in the Erlang implementation was to add two new functions used by CollectFieldWireTypes():

When creating the RecordWireType inside CollectFieldWireTypes(), if there is an existing FieldAlias found that is not identical to the candidate FieldWireType, then (1) MergeFieldWireType() is called (see this code for an example).

(1) `MergeFieldWireType()`

If both Name and Of are equal in both FieldWireTypeA and FieldWireTypeB, then create a new FieldWireTypeC with the same Name, Of, and where Omittable is true if either FieldWireTypeA or FieldWireTypeB; false otherwise.
If both Name are equal and Of is a RecordWireType in both FieldWireTypeA and FieldWireTypeB, call (2) MergeRecordWireType(), passing RecordWireTypeA and RecordWireTypeB. The resulting RecordWireTypeC can then be used to create a new FieldWireTypeC with the same Name, Of set to RecordWireTypeC, and where Omittable is true if either FieldWireTypeA or FieldWireTypeB; false otherwise.
Otherwise, throw a BadShape error.

(2) `MergeRecordWireType()`

If RecordWireTypeA and RecordWireTypeB are identical, return either one as RecordWireTypeC.
Otherwise, call (2.i) MergeRecordWireTypeFields, passing FieldsAIterator, FieldsB, and and empty FieldsC. The resulting FieldsC can be used to create and return RecordWireTypeC.

(2.i) `MergeRecordWireTypeFields()`

For each FieldWireTypeA in FieldsAIterator:
- If the Name of FieldWireTypeA is found in FieldsB as FieldWireTypeB:
  - If FieldWireTypeA is identical to FieldWireTypeB, add FieldWireTypeA to FieldsC.
  - Otherwise, call (1) MergeFieldWireType() passing FieldWireTypeA and FieldWireTypeB. The resulting FieldWireTypeC should be added to FieldsC.
- Otherwise, create a new FieldWireTypeC using the same Name and Of from FieldWireTypeA, but Omittable MUST be set to true. Add FieldWireTypeC to FieldsC.
When iteration of FieldsAIterator has completed, call (2.ii) MergeRecordWireTypeFields() passing FieldsBIterator and FieldsC.
Return FieldsC.

(2.ii) `MergeRecordWireTypeFields()`

For each FieldWireTypeB in FieldsBIterator:
- If the Name of FieldsWireTypeB is found in FieldsC as FieldWireTypeC:
  - Assert that the Name and Of are identical for both FieldsWireTypeB and FieldWireTypeC (note: the Omittable value may not match as it was already determined in a previous step).
- Otherwise, create a new FieldWireTypeC using the same Name and Of from FieldWireTypeB, but Omittable MUST be set to true. Add FieldWireTypeC to FieldsC.
Return FieldsC.

The TL;DR being that if a field exists only in A or B, then it is always marked as Omittable. Otherwise, it will merge both using OR on the Omittable field.

I put together a branch to take a stab at achieving this, but it has some issues--it serializes a few things differently, but also worse. In particular, I think a query like this:

    query {
      hero {
        ... on Droid {
          id
          properties {
            x
            y
          }
        }
      }
    }

should result in

{
 hero: {
   id?: STRING<ID> # omittable because it might not be Droid
   properties?: { # omittable because it might not be Droid
    x: VARINT{Int} # not omittable, because only Droid was selected. would be omittable again if Human were selected
    y: STRING<String>
   }
  }?
}

but it doesn't.

The current Erlang implementation of Argo seems to derive the correct wire type in this case, if you run rebar3 shell and then paste in the contents of this gist it will print out the following:

{
  data: {
    hero: {
      id?: STRING<ID>
      properties?: {
        x: VARINT{Int}
        y: STRING<String>
      }
    }?
  }?
  errors?: ERROR[]
  extensions?: EXTENSIONS
}

You can also run argo:display(WT, #{strict => true}) and it will expand the ERROR wire type to, hopefully, match the output from argo-js.

potatosalad · 2024-04-23T15:07:27Z

One additional thing to consider is this point from section 2.b.ii. part of FieldsInSetCanMerge() in the GraphQL spec:

fieldA and fieldB must have identical sets of arguments.

The current Argo Typer doesn't account for arguments at all, but perhaps the "canonical name" should be kept as part of the Wire Type information in order to account for this as described in this PR.

The "canonical name" here would collapse selected fields like field(arg: "foo") into something like "field(arg:\"foo\")" so that when comparing in the Argo Typer, we could easily determine whether two fields could be merged or not.

Perhaps something like this?

type Field = {
  name: string
  of: Wire.Type
  omittable: boolean
  source: string?
}

msolomon · 2024-04-23T19:11:55Z

For hero.id:
...

See the logic below for what the Erlang code is currently doing. There may be a flaw in the logic.

I think the missing piece is to check for an "exact" selection--anywhere a fragment's type condition exactly matches the field's type. When it's exact, don't mark it omittable (unless a directive demands it).
See: https://github.com/msolomon/argo/blob/overlapping-fields/argo-js/src/typer.ts#L146-L154

For hero.properties.x:

I think x? should still be x since the omittable behavior is actually covered by the parent properties?.

Yes, I think you're right.

The logic I followed in the Erlang implementation...

Thank you, all of this was very helpful! I updated this branch, and it behaves as expected: https://github.com/msolomon/argo/blob/overlapping-fields
It is still not added back into the spec.

One additional thing to consider is this point from section 2.b.ii. part of FieldsInSetCanMerge() in the GraphQL spec:
> fieldA and fieldB must have identical sets of arguments.

Fortunately, I think here (similar to elsewhere, including the reference implementation of lots of the field merging logic) we can rely on GraphQL validating this for us before we ever get to Argo. The query will fail validation if these will later not be mergable.

potatosalad · 2024-04-23T21:27:43Z

For hero.id:
...

See the logic below for what the Erlang code is currently doing. There may be a flaw in the logic.

I think the missing piece is to check for an "exact" selection--anywhere a fragment's type condition exactly matches the field's type. When it's exact, don't mark it omittable (unless a directive demands it).
See: https://github.com/msolomon/argo/blob/overlapping-fields/argo-js/src/typer.ts#L146-L154

This was my first thought, too. I tried this with the query in this PR by adding a new value for Omittable called never which would always win in the merging step.

However, I was running into the problem where fields underneath properties were being erroneously marked as never for Omittable since the object types for these nested selection sets were exact matches.

Does your branch handle this correctly? If so, I'll revisit what I did to see if I can find where my mistake is.

msolomon · 2024-04-24T00:22:58Z

Does your branch handle this correctly? If so, I'll revisit what I did to see if I can find where my mistake is.

I think it works correctly, at least, this test passes: https://github.com/msolomon/argo/blob/overlapping-fields/argo-js/test/wire.test.ts#L190-L191

It's possible further nesting would be a problem, I didn't write a test for it.

Field Selection Merging issue demonstration

3e1cf8a

potatosalad mentioned this pull request Apr 19, 2024

Possible restrictions needed related to NULLABLE(DESC) #18

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Field Selection Merging demonstration of problem #19

Field Selection Merging demonstration of problem #19

potatosalad commented Apr 19, 2024 •

edited

msolomon commented Apr 23, 2024

potatosalad commented Apr 23, 2024

potatosalad commented Apr 23, 2024

msolomon commented Apr 23, 2024

potatosalad commented Apr 23, 2024

msolomon commented Apr 24, 2024

Field Selection Merging demonstration of problem #19

Are you sure you want to change the base?

Field Selection Merging demonstration of problem #19

Conversation

potatosalad commented Apr 19, 2024 • edited

msolomon commented Apr 23, 2024

potatosalad commented Apr 23, 2024

(1) MergeFieldWireType()

(2) MergeRecordWireType()

(2.i) MergeRecordWireTypeFields()

(2.ii) MergeRecordWireTypeFields()

potatosalad commented Apr 23, 2024

msolomon commented Apr 23, 2024

potatosalad commented Apr 23, 2024

msolomon commented Apr 24, 2024

potatosalad commented Apr 19, 2024 •

edited

(1) `MergeFieldWireType()`

(2) `MergeRecordWireType()`

(2.i) `MergeRecordWireTypeFields()`

(2.ii) `MergeRecordWireTypeFields()`