-
Notifications
You must be signed in to change notification settings - Fork 434
Bubble up scalar error #434
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #434 +/- ##
==========================================
- Coverage 85.73% 85.73% -0.01%
==========================================
Files 110 110
Lines 15922 15929 +7
==========================================
+ Hits 13651 13657 +6
- Misses 2271 2272 +1
Continue to review full report at Codecov.
|
Awesome, thank you so much for the PR! Can you add a test exercising this so we don't regress in the future? |
@dreamcodez any help here? |
I am not sure what this 'In disguise' business is. really all you need is this:
and then send a string instead of an integer for the value as a quoted literal. |
I wanted to isolate it better but due to private restrictions i would have to dig deeper and go into the test suite than I have time for right now, here is where I left off:
|
@andy128k made a bounty on related issue 427 : https://www.bountysource.com/issues/80699749-literal-scalar-parsing-should-not-panic |
Please see the issue. We need a proper reproduction first. |
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.
Thanks so much for sticking with this! One small thing and I believe this is good to merge.
juniper/src/parser/tests/document.rs
Outdated
let schema = SchemaType::new::<QueryWithoutFloat, EmptyMutation<()>>(&(), &()); | ||
let parse_result = parse_document_source(r##"{ getInt(value: 123.0) }"##, &schema); | ||
|
||
assert!(parse_result.is_err()); |
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.
We should check that it is an ExpectedScalarError
rather than just check for any error to prevent shadowing a possible underlying bug.
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.
Done
Thanks so much for the PR 🍻 . Sorry it took so long to merge! Any bounty should be worked out between @dreamcodez and @andy128k. |
I pushed a point release with this fix in it 🎉 |
Closes #427