-
Notifications
You must be signed in to change notification settings - Fork 51
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
fix: GraphQL null argument parsing #3013
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3013 +/- ##
===========================================
- Coverage 79.37% 79.33% -0.04%
===========================================
Files 333 333
Lines 25766 25822 +56
===========================================
+ Hits 20451 20485 +34
- Misses 3855 3866 +11
- Partials 1460 1471 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 14 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
c4b2456
to
62afbad
Compare
62afbad
to
3564655
Compare
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.
Looks good to me Keenan, thanks for sorting this out so quickly. Approving now, but have a tiny todo and a question for you before merge please :)
@@ -110,7 +110,7 @@ func filterObjectToMap(mapping *core.DocumentMapping, obj map[connor.FilterKey]a | |||
for k, v := range obj { | |||
switch keyType := k.(type) { | |||
case *PropertyIndex: | |||
subObj := v.(map[connor.FilterKey]any) | |||
subObj, _ := v.(map[connor.FilterKey]any) |
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.
todo: This looks like it was accidentally left in after debugging - can it be reverted?
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 line was causing a panic when the property value is nil. Is there a different way I should be handling the nil value here?
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 I see. I would prefer a if v != nil
wrapper then - it is a much stronger declaration of intent.
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
}, | ||
testUtils.Request{ | ||
Request: `mutation { | ||
create_Users(encrypt: null, input: {name: "Bob"}) { |
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.
question: input
is not nillable? (I don't see any tests for that)
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.
nice catch! added a test
@@ -0,0 +1,110 @@ | |||
// Copyright 2024 Democratized Data Foundation |
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.
priase: Thanks for all theses tests Keenan, looks good :)
Relevant issue(s)
Resolves #3012
Description
This PR fixes an issue where explicitly null GQL inputs would cause a panic.
Tasks
How has this been tested?
Added tests
Specify the platform(s) on which this was tested: