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

VALIDATION_INVALID_TYPE_VARIABLE error for ID input values overflowing i32 #3873

Closed
SimonSapin opened this issue Sep 21, 2023 · 0 comments · Fixed by #3896
Closed

VALIDATION_INVALID_TYPE_VARIABLE error for ID input values overflowing i32 #3873

SimonSapin opened this issue Sep 21, 2023 · 0 comments · Fixed by #3896
Assignees

Comments

@SimonSapin
Copy link
Contributor

Describe the bug
For the ID scalar type, the GraphQL specification defines coercion of input values as:
https://spec.graphql.org/October2021/#sec-ID.Input-Coercion

When expected as an input type, any string (such as "4") or integer (such as 4 or -4) input value should be coerced to ID as appropriate for the ID formats a given GraphQL service expects. Any other input value, including float input values (such as 4.0), must raise a request error indicating an incorrect type.

Apollo Router 1.30.0 implements this as "validate like an input value of type String or Int". The latter is limited to i32:
https://spec.graphql.org/October2021/#sec-Int.Input-Coercion

When expected as an input type, only integer input values are accepted. All other input values, including strings with numeric content, must raise a request error indicating an incorrect type. If the integer input value represents a value less than -2^31 or greater than or equal to 2^31, a request error should be raised.

However no such range is specified for ID. In theory the Router could be made to support integer IDs of arbitrary size. In practice it’s probably unwise to go beyond 53 bits, in case a client or subgraph represents integer like JavaScript does, as floating point with a 53-bit mantissa. Still, it’s easy enough to make the Router support up to i64 here.

To Reproduce
Steps to reproduce the behavior:

  1. Create a /tmp/router.yaml file that contains {}
  2. Run cargo run -- -c /tmp/router.yaml -s examples/graphql/supergraph.graphql
  3. In another terminal, run:
curl -s --request POST \
  --header 'content-type: application/json' \
  --url 'http://127.0.0.1:4000' \
  --data '{"variables": {"var": 2147483648}, "query":"query Q($var: ID!) { topProducts { name reviewsForAuthor(authorID: $var) { id } } }"}'

Expected behavior
The response contains data and no error.

Output
{"errors":[{"message":"invalid type for variable: 'var'","extensions":{"name":"var","code":"VALIDATION_INVALID_TYPE_VARIABLE"}}]}

Additional context
Manually tested fix:

--- a/apollo-router/src/spec/field_type.rs
+++ b/apollo-router/src/spec/field_type.rs
@@ -110,7 +110,7 @@ fn validate_input_value(
         // In practice it seems Int works too
         (hir::Type::Named { name, .. }, Value::String(_)) if name == "ID" => Ok(()),
         (hir::Type::Named { name, .. }, maybe_int) if name == "ID" => {
-            if maybe_int == &Value::Null || maybe_int.is_valid_int_input() {
+            if maybe_int == &Value::Null || maybe_int.as_i64().is_some() {
                 Ok(())
             } else {
                 Err(InvalidValue)

Still needs an automatic test, ideally one checking that 9007199254740993 (2^53 + 1) is passed through to the subgraph without being rounded.

SimonSapin added a commit that referenced this issue Sep 25, 2023
Fixes #3873

Input values for variables of type `ID` were previously validated as
"either like a GraphQL `Int` or like a GraphQL `String`". GraphQL `Int`
is specified as a signed 32-bit integer, such that values that overflow
fail validation. Applying this range restriction to `ID` values was
incorrect. Instead, validation for `ID` now accepts any JSON integer or
JSON string value, so that IDs larger than 32 bits can be used.

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

**Checklist**

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

- [x] Changes are compatible[^1]
- [x] Documentation[^2] completed
- [x] Performance impact assessed and acceptable
- Tests added and passing[^3]
    - [ ] Unit Tests
    - [x] 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.
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.

1 participant