Skip to content

Make it clear that other keys are reserved #278

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 25, 2024
Merged

Conversation

benjie
Copy link
Member

@benjie benjie commented Oct 27, 2023

Fixes #271

The extensions object exists so that vendors may add their own properties to requests/responses/errors/etc. All top-level keys are reserved by the spec. This PR makes this explicit for JSON-encoded requests.

@enisdenjo Will this break any of the existing implementations, other than Relay's persisted queries network layer recommendation?

@enisdenjo
Copy link
Member

I don't think this will break anything. Looks good to me!

It is kinda conventionalized that all metadata lands on the extensions field.

@enisdenjo enisdenjo merged commit 1e56098 into main Jan 25, 2024
@benjie benjie deleted the reserved-properties branch January 25, 2024 23:53
@enisdenjo
Copy link
Member

Follow-up question: should I add a test to graphql-http reflecting this? It's now illegal to have extraneous keys in the request body.

@benjie
Copy link
Member Author

benjie commented Jan 26, 2024

Servers don't need to validate this, and in fact they should not since a client from a future spec version may send additional keys, which legacy servers should just ignore.

@enisdenjo
Copy link
Member

Hmm, is the "must" then confusing?

[...] if implementors need to add additional information to a request they MUST do so via other means,

If we add more fields in future spec versions, we can just add the fields/relax the compliance tests?

@Shane32
Copy link
Contributor

Shane32 commented Jan 26, 2024

Servers don't need to validate this, and in fact they should not since a client from a future spec version may send additional keys, which legacy servers should just ignore.

I think we should add this to the spec. The main graphql spec did not state this explicitly for reserved names (those starting with __), and now any current GraphQL UI applications built on Apollo's graphql library crash if they see additional reserved names in the introspection result. (I consider this a bug but 🤷‍♂️ ). They should be built to ignore additional features added to the spec, and the spec should state this.

We could add something like this:

Servers receiving a request with additional properties MUST ignore the additional properties when processing the request.

@benjie
Copy link
Member Author

benjie commented Jan 29, 2024

and now any current GraphQL UI applications built on Apollo's graphql library crash if they see additional reserved names in the introspection result. (I consider this a bug but 🤷‍♂️ )

That must be a bug. All potential field names that start with __ are reserved for introspection usage, it must be anticipated that we can use additional such fields in future.

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.

Make it clear that extra keys in the request/response payloads are not allowed
5 participants