-
Notifications
You must be signed in to change notification settings - Fork 46
Fix: Accept all valid Float value inputs on coerce_variable_value
#1002
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
base: main
Are you sure you want to change the base?
Conversation
Coerce JSON ints to finite f64 in compliance with GraphQL spec
@rb090: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for your contribution! There are few small things to fix, which I've noted in the review comments. Once those are in, I can merge this in.
@@ -0,0 +1,50 @@ | |||
|
|||
#[test] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test won't run as is. It needs to be added to apollo-compiler/tests/validation/main.rs
. Once added, you will notice rust compiler won't compile this file, and you will need to declare Schema
, ExecutableDocument
and coerce_variable_values
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve moved input_coercion.rs
to apollo-compiler/tests/validation
directory and added its declaration to apollo-compiler/tests/validation/mod.rs
with c5f4bac. I believe this is what you were referring to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sorry, my bad, I typed out the path wrong. Where you had it was good - input coercion is not a validation rule. It needed to be added to apolo-compiler/tests/main.rs
as mod input_coercion
(and I wrote apolo-compiler/tests/validation/main.rs
, leading to confusion).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let kilometers_value = 3000; | ||
|
||
// Provide an integer for a Float field | ||
let input_variables = serde_json_bytes::json!({ "car": { "kilometers": kilometers_value } }); | ||
let map = match input_variables { | ||
serde_json_bytes::Value::Object(m) => m, | ||
_ => unreachable!(), | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could I ask you to add another test to this file that shows how coercion would fail?
Since we have changed the coercion code to now be value.as_f64().is_some_and(f64::is_finite)
, there is a chance for a None
to be returned from as_f64
. You might have to have kilometers_value
be defined as a string, otherwise the compiler won't let this compile. So potentially two variations of the test like this:
// Infinity; should fail coercion
let kilometers_value = "1e1000";
and another one:
// i128 MAX; should also fail coercion, since it's > i64 MAX
let kilometers_value = "170141183460469231731687303715884105727";
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch, thank you! It seems that the coercion changes I made in resolvers/input_coercion.rs
didn’t fully cover the case involving values beyond i128::MAX
. The extended unit tests helped me spotting this edge case.
I’ve fixed the issue in 23d67b8 and also refactored tests/input_coercion.rs
to reduce code duplication and improve readability.
Add it to mod.rs
Add module declaration to tests/main.rs
Extend unit tests to cover failing Float coercion cases
This PR fixes the behaviour of
Float
input values in theapollo-compiler
crate inapollo_compiler::request::coerce_variable_values
to align with the GraphQL specification.The GraphQL specifications defines the input coercion like this:
The issue this PR solves is, that
coerce_variable_values
incorrectly rejects JSON numbers without fractional parts (e.g. 42) when the corresponding input variable type wasFloat
.The changes include accepting any JSON number (int or float) that can be converted to an
f64
forFloat
inputs as well as a small unit testtest_graphql_float_variable_coercion
showing the behaviour.Big thanks to my colleague David, who identified where the changes needed to be applied and also reviewed the code updates. Much appreciated! 🙌
Once PR#1001 fix(smith): apply new clippy rules from Rust 1.90 is merged, the CI on this PR will fully succeed. It fails yet on→ ✅cargo clippy --all-targets --all-features -- -D warnings
because of the code incrates/apollo-smith/src/schema.rs
fixed in PR#1001.