Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(api-graphql): incorrect list sk arg type #13249

Merged
merged 3 commits into from
Apr 15, 2024
Merged

fix(api-graphql): incorrect list sk arg type #13249

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
63e098f
fix: incorrect sk arg type
iartemiev Apr 14, 2024
954e112
bump bundle size
iartemiev Apr 14, 2024
2361247
pr feedback
iartemiev Apr 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/api-graphql/__tests__/internals/APIClient.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,7 +563,7 @@ describe('generateGraphQLDocument()', () => {
modelOperation
);

expect(document.includes(expectedArgs)).toBe(true);
expect(document).toEqual(expect.stringContaining(expectedArgs))
Copy link
Contributor Author

@iartemiev iartemiev Apr 15, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
);
});
Expand Down
12 changes: 6 additions & 6 deletions packages/api-graphql/__tests__/internals/__snapshots__/generateClient.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, $sortDirection: ModelSortDirection, $cpk_sort_key: ModelStringKeyConditionInput, $filter: ModelThingWithCustomPkFilterInput, $limit: Int, $nextToken: String) {
listThingWithCustomPks(
filter: $filter
sortDirection: $sortDirection
cpk_cluster_key: $cpk_cluster_key
sortDirection: $sortDirection
cpk_sort_key: $cpk_sort_key
filter: $filter
limit: $limit
nextToken: $nextToken
) {
Expand Down Expand Up @@ -4139,12 +4139,12 @@ exports[`generateClient basic model operations can list() with sortDirection (DE
"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, $sortDirection: ModelSortDirection, $cpk_sort_key: ModelStringKeyConditionInput, $filter: ModelThingWithCustomPkFilterInput, $limit: Int, $nextToken: String) {
listThingWithCustomPks(
filter: $filter
sortDirection: $sortDirection
cpk_cluster_key: $cpk_cluster_key
sortDirection: $sortDirection
cpk_sort_key: $cpk_sort_key
filter: $filter
limit: $limit
nextToken: $nextToken
) {
Expand Down
64 changes: 41 additions & 23 deletions packages/api-graphql/src/internals/APIClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -721,6 +721,45 @@ export function generateGraphQLDocument(
selectionSet as ListArgs['selectionSet'],
);

// default PK args for get and list operations
// modified below for CPK
const getPkArgs = {
[primaryKeyFieldName]: `${fields[primaryKeyFieldName].type}!`,
};
const listPkArgs = {};

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;
},
{},
);
};
Comment on lines +731 to +746
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`,
];
}),
);

Copy link
Contributor Author

@iartemiev iartemiev Apr 15, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if (isCustomPrimaryKey) {
Object.assign(getPkArgs, generateSkArgs('get'));

Object.assign(
listPkArgs,
{
// PK is only included in list query field args in the generated GQL
// when explicitly specifying PK with .identifier(['fieldName']) or @primaryKey in the schema definition
[primaryKeyFieldName]: `${fields[primaryKeyFieldName].type}`, // PK is always a nullable arg for list (no `!` after the type)
sortDirection: 'ModelSortDirection',
},
generateSkArgs('list'),
);
}

switch (modelOperation) {
case 'CREATE':
case 'UPDATE':
Expand All @@ -736,36 +775,15 @@ export function generateGraphQLDocument(
// TODO(Eslint): this this case clause correct without the break statement?
// eslint-disable-next-line no-fallthrough
case 'READ':
graphQLArguments ??
(graphQLArguments = isCustomPrimaryKey
? [primaryKeyFieldName, ...sortKeyFieldNames].reduce(
(acc: Record<string, any>, fieldName) => {
acc[fieldName] = `${fields[fieldName].type}!`;

return acc;
},
{},
)
: {
[primaryKeyFieldName]: `${fields[primaryKeyFieldName].type}!`,
});
graphQLArguments ?? (graphQLArguments = getPkArgs);
graphQLSelectionSet ?? (graphQLSelectionSet = selectionSetFields);
// TODO(Eslint): this this case clause correct without the break statement?
// eslint-disable-next-line no-fallthrough
case 'LIST':
graphQLArguments ??
(graphQLArguments = {
...listPkArgs,
filter: `Model${name}FilterInput`,
...(sortKeyFieldNames.length > 0
? [primaryKeyFieldName, ...sortKeyFieldNames].reduce(
(acc: Record<string, any>, fieldName) => {
acc[fieldName] = `${fields[fieldName].type}`;

return acc;
},
{ sortDirection: 'ModelSortDirection' },
)
: []),
limit: 'Int',
nextToken: 'String',
});
Expand Down
2 changes: 1 addition & 1 deletion packages/aws-amplify/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@
"name": "[API] generateClient (AppSync)",
"path": "./dist/esm/api/index.mjs",
"import": "{ generateClient }",
"limit": "38.5 kB"
"limit": "39.0 kB"
},
{
"name": "[API] REST API handlers",
Expand Down