Fix: Accept all valid Float value inputs on coerce_variable_value
#1002
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/ |
| @@ -0,0 +1,50 @@ | |||
|
|
|||
| #[test] | |||
There was a problem hiding this comment.
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.
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.
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).
| 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.
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.
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
Floatinput values in theapollo-compilercrate inapollo_compiler::request::coerce_variable_valuesto align with the GraphQL specification.The GraphQL specifications defines the input coercion like this:
The issue this PR solves is, that
coerce_variable_valuesincorrectly 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
f64forFloatinputs as well as a small unit testtest_graphql_float_variable_coercionshowing 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 warningsbecause of the code incrates/apollo-smith/src/schema.rsfixed in PR#1001.