graphql-ruby icon indicating copy to clipboard operation
graphql-ruby copied to clipboard

Assert matching response shapes in fields_will_merge

Open rmosolgo opened this issue 2 years ago • 1 comments

Oops, it seems like one part of FieldsWillMerge was missing. Adding it now might break some queries that used to work, so I may have to ensure there's a way to opt out of this change.

From the spec:

SameResponseShape(fieldA, fieldB)

  • Let typeA be the return type of fieldA.
  • Let typeB be the return type of fieldB.
  • If typeA or typeB is Non-Null:
    • If typeA or typeB is nullable, return false.
    • Let typeA be the nullable type of typeA.
    • Let typeB be the nullable type of typeB.
  • If typeA or typeB is List:
    • If typeA or typeB is not List, return false.
    • Let typeA be the item type of typeA.
    • Let typeB be the item type of typeB.
    • Repeat from step 3.
  • If typeA or typeB is Scalar or Enum:
    • If typeA and typeB are the same type return true, otherwise return false.

https://spec.graphql.org/draft/#SameResponseShape()

In GraphQL-JS:

https://github.com/graphql/graphql-js/blob/f201681bf806a2c46c4ee8b2533287421327a302/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts#L691-L718

https://github.com/graphql/graphql-js/blob/a24a9f35b876bdd0d5050eca34d3020bd0db9a29/src/validation/tests/OverlappingFieldsCanBeMergedRule-test.ts#L738-L767

TODO:

  • [x] Does this make other tests fail?
    • One now-removed test was added here: https://github.com/rmosolgo/graphql-ruby/pull/2004 I think it was a test for code that doesn't exist anymore but ... will it cause a bug to make that fail?
  • [ ] Is this too big of a change to make at this point?
  • [ ] What about people who depend on this check not being here?
  • [ ] Performance implications?

rmosolgo avatar Feb 16 '23 14:02 rmosolgo

Adding it now might break some queries that used to work, so I may have to ensure there's a way to opt out of this change.

Unfortunately, yes... this is a breaking change that will start rejecting queries that used to be accepted. Probably the smoothest path to reaching spec would be to make this an opt-in setting on GraphQL::Schema that is off by default for now and activates in the next major release.

gmac avatar Feb 16 '23 21:02 gmac