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.

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-key-transformer): prevent non-scalar key fields #5319

Merged
merged 2 commits into from Dec 11, 2020

Conversation

RossWilliams
Copy link

Issue #, if available:
#5300

Description of changes:
Logic for 'isScalar' function treated lists as scalar types. Lists are not scalar, and are not valid primary key types. This allowed indexes to be created with invalid key types, resulting in runtime errors.

This modifies the isScalar function to return false for list types, and updates other areas as needed with a new 'isScalarOrScalarList' function. Added explicit list checks where needed, type narrowing, and changes to satisfy compiler.

Includes new test which fails without this fix.
Fixes adjacent tests failing because of missing model transforms, and not for the stated reason.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

resolves aws-amplify#5300

Logic for 'isScalar' treated lists as scalar types. This allowed indexes to be created with invalid key types.

This modifies the isScalar function to return false for list types, and updates other areas as needed with a new 'isScalarOrScalarList' function

Includes new test which fails without this fix.
Fixes adjacent tests failing because of missing model transforms, and not for the stated reason.
@codecov
Copy link

codecov bot commented Oct 18, 2020

Codecov Report

Merging #5319 into master will increase coverage by 0.01%.
The diff coverage is 68.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5319      +/-   ##
==========================================
+ Coverage   57.76%   57.77%   +0.01%     
==========================================
  Files         406      406              
  Lines       18630    18640      +10     
  Branches     3721     3725       +4     
==========================================
+ Hits        10761    10769       +8     
- Misses       7187     7189       +2     
  Partials      682      682              
Impacted Files Coverage Δ
...es/graphql-dynamodb-transformer/src/definitions.ts 83.42% <0.00%> (ø)
...es/graphql-connection-transformer/src/resources.ts 77.50% <50.00%> (-0.71%) ⬇️
...tion-transformer/src/ModelConnectionTransformer.ts 89.30% <80.00%> (-0.22%) ⬇️
...ages/graphql-key-transformer/src/KeyTransformer.ts 90.90% <85.71%> (+0.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a337c8...45e68ed. Read the comment docs.

@edwardfoyle edwardfoyle added the pending-review Pending review from core-team label Oct 27, 2020
Copy link
Contributor

@SwaySway SwaySway left a comment

Choose a reason for hiding this comment

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

LGTM - Thanks for your contribution!

@github-actions
Copy link

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels for those types of questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pending-review Pending review from core-team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants