Skip to content

Move "ignore unknown parameters" in the common section between GET and POST#384

Open
martinbonnin wants to merge 1 commit intomainfrom
unknown-parameters
Open

Move "ignore unknown parameters" in the common section between GET and POST#384
martinbonnin wants to merge 1 commit intomainfrom
unknown-parameters

Conversation

@martinbonnin
Copy link
Copy Markdown
Contributor

This is not specific to JSON

Copy link
Copy Markdown
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. I think this is going to be more problematic for GraphQL GET queries where ?api_token=, ?csrf_token=, and similar fields are more likely to occur. These will typically need to be handled at the middleware level (before GraphQL), so it would normally not be suitable to encode them as JSON inside extensions for example. I think this one needs more thought/discussion. People should be using headers for these, but it's not always that easy.

@benjie
Copy link
Copy Markdown
Member

benjie commented Apr 23, 2026

Another type of query parameter is those involved in routing, e.g. https://example.com/graphql?tenant=27&query=... - again, these should typically be in the path, but it's not uncommon for people to add things like this to query strings, and they may need to be honored by a layer that's not GraphQL aware so putting JSON encoded in extensions is not ideal. Also things like ?version= to route to a particular schema version (should be processed at the routing level) or ?explain=true (which would be suitable to move into extensions).

@Shane32
Copy link
Copy Markdown
Contributor

Shane32 commented Apr 23, 2026

Personally I think this paragraph has no force. If someone needs a custom field for their own needs they will add it however they choose to do so, since it will need to be done on both the server and client ends. I have an implementation of persisted documents, for instance, but it could be auth related or something else. Can’t use regular tooling since it’s nonstandard anyway. So I don’t really see the purpose of using MUST here. The only consequence that can occur is that tooling might not support custom fields due to the word “MUST” which just makes it harder on the developer that needs a custom field. Perhaps this forces them to use an alternate implementation method like headers but honestly a compliant request won’t work without the additional header so what’s the difference? You can also argue that since it’s not going to work anyway, use of MUST is appropriate since the request won’t process and therefore is non-standard by definition.

@Shane32
Copy link
Copy Markdown
Contributor

Shane32 commented Apr 23, 2026

The benefit to not-specifying “MUST” is simply that tooling might better support custom requirements by a developer. Whether it’s inspection, server or client libraries.

As an example, I know GraphQL js totally rejects an introspection response listing a nonstandard reserved field. Completely breaks GraphiQL if you add one (e.g. __dummy), and makes it unusable, even though there’s no reason it couldn’t just ignore it. (This would also break official extensions to GraphQL which add a reserved field.) Very cumbersome if you want a custom introspection extension in your server.

Rather not repeat that decision here.

@martinbonnin
Copy link
Copy Markdown
Contributor Author

I think this is going to be more problematic for GraphQL GET queries where ?api_token=, ?csrf_token=, and similar fields are more likely to occur.

Indeed. Sounds selfish of GraphQL to claim the whole query string. But there's no silver bullet, right? Either we let it go (and risk nameclashes like for directive names) or we claim a prefix (i.e. __) but it doesn't look good.

I'm fine letting this one go. The probability of clash is probably low (onError is the only example that comes to mind). I'll keep this one open a bit to see if there's more feedback. If not, I'll close it.

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.

3 participants