-
Notifications
You must be signed in to change notification settings - Fork 44
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
refactor: GraphQL order input #3044
refactor: GraphQL order input #3044
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3044 +/- ##
===========================================
+ Coverage 79.40% 79.50% +0.10%
===========================================
Files 342 343 +1
Lines 26514 26511 -3
===========================================
+ Hits 21052 21075 +23
+ Misses 3942 3927 -15
+ Partials 1520 1509 -11
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 16 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
// prepend the current field name, to the parsed condition from the slice | ||
// Eg. order: {author: {name: ASC, birthday: DESC}} | ||
// This results in an array of [name, birthday] converted to | ||
// [author.name, author.birthday]. | ||
// etc. |
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.
thought: The order object in this comment seems invalid as it's an unordered map intending for name and birthday to be in an explicit order.
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 think it would have to be order: [{author: {name: ASC}}, {author: {birthday: DESC}}]
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! fixed a few others as well
if len(arg) == 0 { | ||
return nil, nil | ||
} | ||
if len(arg) != 1 { |
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: Please add a test for the case where we have more than one argument
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
case 1: | ||
return request.DESC, nil | ||
|
||
default: |
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: please add a test that satisfies this.
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 case is technically unreachable because the GQL parsing will fail. I've added a test to demonstrate this. I felt it was better to keep the default case instead of having it undefined, but I can remove it if you have a different preference.
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.
LGTM. Just a couple test todos and a comment to fix before merge.
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.
LGTM :)
Relevant issue(s)
Resolves #3043
Description
This PR fixes an issue where GQL variables could not be used with ordering input args. The order input type on collections and aggregates has been refactored to a list so that ordering is preserved in all environments.
Tasks
How has this been tested?
Added integration tests
Specify the platform(s) on which this was tested: