Skip to content

Assert matching response shapes in fields_will_merge#4347

Merged
rmosolgo merged 7 commits intomasterfrom
fields-will-merge-response-type-check
Apr 11, 2025
Merged

Assert matching response shapes in fields_will_merge#4347
rmosolgo merged 7 commits intomasterfrom
fields-will-merge-response-type-check

Conversation

@rmosolgo
Copy link
Copy Markdown
Owner

@rmosolgo rmosolgo commented Feb 16, 2023

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:

  • Does this make other tests fail?
    • One now-removed test was added here: Interpreter type at path fix #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?
    • Added a compat layer for deprecation and future removal
  • Performance implications?
    • I ran bench:validate and didn't see any regression. The recursion here would only occur for list/non-null types, but in the average case, the overhead is a few simple attr_reader-type method calls.

Comment thread lib/graphql/static_validation/rules/fields_will_merge.rb
Comment thread lib/graphql/static_validation/rules/fields_will_merge.rb Outdated
@gmac
Copy link
Copy Markdown
Contributor

gmac commented Feb 16, 2023

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.

@rmosolgo
Copy link
Copy Markdown
Owner Author

opt-in setting

Done in 75c164c with a handler method for tracking occurrences of this problem.

@rmosolgo rmosolgo merged commit b8e2bff into master Apr 11, 2025
15 checks passed
@rmosolgo rmosolgo deleted the fields-will-merge-response-type-check branch April 11, 2025 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants