fix(api-graphql): incorrect list sk arg type #13249
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
iartemiev
commented
Apr 15, 2024
| @@ -563,7 +563,7 @@ describe('generateGraphQLDocument()', () => { | |||
| modelOperation | |||
| ); | |||
|
|
|||
| expect(document.includes(expectedArgs)).toBe(true); | |||
| expect(document).toEqual(expect.stringContaining(expectedArgs)) | |||
There was a problem hiding this comment.
Same effect, but this outputs a helpful diff if the string doesn't match, whereas previously it would only tell us got false expected true.
iartemiev
commented
Apr 15, 2024
| @@ -4092,12 +4092,12 @@ exports[`generateClient basic model operations can list() with sortDirection (AS | |||
| "abortController": AbortController {}, | |||
| "options": { | |||
| "body": { | |||
| "query": "query ($filter: ModelThingWithCustomPkFilterInput, $sortDirection: ModelSortDirection, $cpk_cluster_key: String, $cpk_sort_key: String, $limit: Int, $nextToken: String) { | |||
| "query": "query ($cpk_cluster_key: String, $cpk_sort_key: ModelStringKeyConditionInput, $sortDirection: ModelSortDirection, $filter: ModelThingWithCustomPkFilterInput, $limit: Int, $nextToken: String) { | |||
There was a problem hiding this comment.
Model${fieldType}KeyConditionInput is the expected GraphQL type for SKs in list operations
HuiSF
reviewed
Apr 15, 2024
Comment on lines
+731
to
+746
| const generateSkArgs = (op: 'get' | 'list') => { | ||
| return sortKeyFieldNames.reduce( | ||
| (acc: Record<string, any>, fieldName: string) => { | ||
| const fieldType = fields[fieldName].type; | ||
|
|
||
| if (op === 'get') { | ||
| acc[fieldName] = `${fieldType}!`; | ||
| } else if (op === 'list') { | ||
| acc[fieldName] = `Model${fieldType}KeyConditionInput`; | ||
| } | ||
|
|
||
| return acc; | ||
| }, | ||
| {}, | ||
| ); | ||
| }; |
There was a problem hiding this comment.
nit: can use Object.fromEntries to save a few bytes :D (still over the limit a bit though...)
Suggested change
| const generateSkArgs = (op: 'get' | 'list') => { | |
| return sortKeyFieldNames.reduce( | |
| (acc: Record<string, any>, fieldName: string) => { | |
| const fieldType = fields[fieldName].type; | |
| if (op === 'get') { | |
| acc[fieldName] = `${fieldType}!`; | |
| } else if (op === 'list') { | |
| acc[fieldName] = `Model${fieldType}KeyConditionInput`; | |
| } | |
| return acc; | |
| }, | |
| {}, | |
| ); | |
| }; | |
| const generateSkArgs = (op: 'get' | 'list') => | |
| Object.fromEntries( | |
| sortKeyFieldNames.map(fieldName => { | |
| const fieldType = fields[fieldName].type; | |
| return [ | |
| fieldName, | |
| op === 'get' ? `${fieldType}!` : `Model${fieldType}KeyConditionInput`, | |
| ]; | |
| }), | |
| ); |
There was a problem hiding this comment.
I think the suggested change makes this harder to read/grok. I'd rather keep it very simple since we've had lots of bugs in this code already.
HuiSF
approved these changes
Apr 15, 2024
HuiSF
changed the title
jimblanc
approved these changes
Apr 15, 2024
elorzafe
approved these changes
Apr 15, 2024
david-mcafee
approved these changes
Apr 15, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes
We were generating list query arguments for sort keys using the underlying field's primitive type as the argument type.
List queries on models with composite primary keys expect a condition input argument type for the sort key(s).
See this diff for current vs expected behavior: https://github.com/aws-amplify/amplify-js/compare/data-fix-sk-args?expand=1#diff-fa24101f776e07b54f8416746bbed348b96dd7e855c0eed2e5609bcb501a9b46L4095-R4095
Description of how you validated changes
Checklist
yarn testpassesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.