Skip to content
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

Specifying extensions as null in POST request causes INVALID_GRAPHQL_REQUEST error #3388

Closed
lorf opened this issue Jul 7, 2023 · 0 comments · Fixed by #4911
Closed

Specifying extensions as null in POST request causes INVALID_GRAPHQL_REQUEST error #3388

lorf opened this issue Jul 7, 2023 · 0 comments · Fixed by #4911
Assignees

Comments

@lorf
Copy link

lorf commented Jul 7, 2023

Describe the bug

When making an HTTP POST request with extensions field set to null Apollo Router returns INVALID_GRAPHQL_REQUEST error:

$ curl -H 'content-type: application/json' -d '{"query":"{__typename}","extensions":null}' http://127.0.0.1:4000/
{"errors":[{"message":"Invalid GraphQL request","extensions":{"details":"failed to deserialize the request body into JSON: invalid type: null, expected a map at line 1 column 41","code":"INVALID_GRAPHQL_REQUEST"}}]}

However "GraphQL over HTTP" spec draft says that the extensions field is an optional request parameter (https://graphql.github.io/graphql-over-http/draft/#sec-Request-Parameters ). It also says (https://graphql.github.io/graphql-over-http/draft/#note-22957 ):

Note: Specifying null for optional request parameters is equivalent to not specifying them at all.

To Reproduce
When specifying extensions field in POST request as null, the following error is produced:

$ curl -H 'content-type: application/json' -d '{"query":"{__typename}","extensions":null}' http://127.0.0.1:4000/
{"errors":[{"message":"Invalid GraphQL request","extensions":{"details":"failed to deserialize the request body into JSON: invalid type: null, expected a map at line 1 column 41","code":"INVALID_GRAPHQL_REQUEST"}}]}

However, omitting extensions field works as expected:

$ curl -H 'content-type: application/json' -d '{"query":"{__typename}"}' http://127.0.0.1:4000/
{"data":{"__typename":"Query"}}

Expected behavior
Apollo Router should process POST requests with extensions field set to null as if there was no extensions field specified.

Output
Please, see To reproduce section.

Desktop (please complete the following information):

  • OS: Ubuntu
  • Version: 18.04

Additional context
Setting null_is_default = true here seems to fix the issue - requests with extensions=null start working as expected, but this was not thoroughly tested.

$ git diff
diff --git a/apollo-router/src/request.rs b/apollo-router/src/request.rs
index cbd3c3d7..dfe91bc7 100644
--- a/apollo-router/src/request.rs
+++ b/apollo-router/src/request.rs
@@ -281,7 +281,7 @@ impl<'data, 'de> DeserializeSeed<'de> for RequestFromBytesSeed<'data> {
                             }
                             let seed = serde_json_bytes::value::BytesSeed::new(self.0);
                             let value = map.next_value_seed(seed)?;
-                            let null_is_default = false;
+                            let null_is_default = true;
                             extensions = Some(as_object(value, null_is_default)?);
                         }
                         Field::Other => {
@SimonSapin SimonSapin self-assigned this Apr 4, 2024
SimonSapin added a commit that referenced this issue Apr 4, 2024
Fixes #3388

In GraphQL requests, `extensions` is an optional map.
Passing an explicit `null` was incorrectly considered a parse error.
Now it is equivalent to omiting that field entirely, or to passing an empty map.
SimonSapin added a commit that referenced this issue Apr 4, 2024
Fixes #3388

In GraphQL requests, `extensions` is an optional map.
Passing an explicit `null` was incorrectly considered a parse error.
Now it is equivalent to omiting that field entirely, or to passing an empty map.
SimonSapin added a commit that referenced this issue Apr 4, 2024
Fixes #3388

In GraphQL requests, `extensions` is an optional map. Passing an
explicit `null` was incorrectly considered a parse error. Now it is
equivalent to omiting that field entirely, or to passing an empty map.

<!-- start metadata -->
---

**Checklist**

Complete the checklist (and note appropriate exceptions) before the PR
is marked ready-for-review.

- [ ] Changes are compatible[^1]
- [ ] Documentation[^2] completed
- [ ] Performance impact assessed and acceptable
- Tests added and passing[^3]
    - [ ] Unit Tests
    - [ ] Integration Tests
    - [ ] Manual Tests

**Exceptions**

*Note any exceptions here*

**Notes**

[^1]: It may be appropriate to bring upcoming changes to the attention
of other (impacted) groups. Please endeavour to do this before seeking
PR approval. The mechanism for doing this will vary considerably, so use
your judgement as to how and when to do this.
[^2]: Configuration is an important part of many changes. Where
applicable please try to document configuration examples.
[^3]: Tick whichever testing boxes are applicable. If you are adding
Manual Tests, please document the manual testing (extensively) in the
Exceptions.

---------

Co-authored-by: Bryn Cooke <BrynCooke@gmail.com>
Co-authored-by: bryn <bryn@apollographql.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants