-
Notifications
You must be signed in to change notification settings - Fork 2
fix(router): allow null value for nullable scalar types while validating variables & coerce list variables #483
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
Summary of ChangesHello @ardatan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where the GraphQL variable validation process incorrectly rejected Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
@gemini-code-assist review and summary |
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.
Pull Request Overview
This PR fixes a bug in variable validation where null values were incorrectly rejected for nullable scalar types. The validation function now properly accepts null values for scalar types like String, Int, Float, Boolean, and ID when they are not wrapped in a NonNull type.
Key changes:
- Added early null checks for Named and List type nodes to allow null values
- Added comprehensive test coverage for null validation on scalar types
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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.
Code Review
This pull request correctly adjusts variable validation to allow null
values for nullable scalar and list types, aligning with GraphQL's nullability rules. The changes in validate_runtime_value
are logical and are accompanied by a new test case. My review includes a suggestion to rename the test for clarity and another to add a test case for nullable list types to ensure full coverage of the changes. The PR description seems to have a minor mix-up between 'nullable' and 'non-nullable', but the code implementation is correct.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.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.
Code Review
This pull request correctly allows null
values for nullable scalar and list types during variable validation, which fixes a bug where they were incorrectly rejected. My review includes a suggestion to refactor the implementation to reduce code duplication, a correction for a misleading test name, and a recommendation to add a missing test case for nullable list types to improve test coverage.
✅
|
🐋 This PR was built and pushed to the following Docker images: Image Names: Platforms: Image Tags: Docker metadata{
"buildx.build.ref": "builder-1ba615dd-3fe6-40e7-82b6-c0273bb5cab1/builder-1ba615dd-3fe6-40e7-82b6-c0273bb5cab10/x8ejl8duq73b4zedx6vzyxpjx",
"containerimage.descriptor": {
"mediaType": "application/vnd.oci.image.index.v1+json",
"digest": "sha256:9ce2ce1e0c00d2a27a44e29da0a9508cf1c5acca5e8cfca06ba0e7a26fac03a5",
"size": 1609
},
"containerimage.digest": "sha256:9ce2ce1e0c00d2a27a44e29da0a9508cf1c5acca5e8cfca06ba0e7a26fac03a5",
"image.name": "ghcr.io/graphql-hive/router:pr-483,ghcr.io/graphql-hive/router:sha-4d3751b"
} |
Ref ROUTER-155
Ref ROUTER-156
When
validate_runtime_value
takesnull
for a non-nullable scalar type, it was throwing like;Failed to collect GraphQL variables: Expected a string for type 'String', got Null
When the list input value receives non-list value, it should be treated as a one-item list.
In this case if you have an operation like below;
And then variables like
{ "bar": "BAR" }
it should be treated as
["BAR"]