feat: add projection support for @index directive #3359
Conversation
svidgen
left a comment
svidgen
left a comment
There was a problem hiding this comment.
I'm not sure offhand what is or isn't required for the AppSync/resolver side of this. But, I suspect it's more than this. I would actually kind of expect to see some new generated GraphQL types.
Sufficiently thorough e2e tests would give me some confidence this is working as expected either way though.
| field: definition, | ||
| directive, | ||
| } as IndexDirectiveConfiguration, | ||
| } as Required<IndexDirectiveConfiguration>, |
There was a problem hiding this comment.
Hi! Quick question - just trying to understand why we're setting this as Required<> when IndexDirectiveConfiguration properties are defined as optional. Maybe some comments could be added to explain this decision in the file
There was a problem hiding this comment.
This is due to the introduction of the possibly undefined projection property in the IndexDirectiveConfiguration type (found here). The getArguments function signature is getArguments = <T>(defaultValue: Required<T>,...), so the first argument has to be cast as Required.
| const partitionKeyType = attrDefs.find((attr) => attr.attributeName === partitionKeyName)?.attributeType ?? 'S'; | ||
| const sortKeyType = sortKeyName ? attrDefs.find((attr) => attr.attributeName === sortKeyName)?.attributeType ?? 'S' : undefined; | ||
|
|
||
| const projectionType = projection?.type || 'ALL'; |
There was a problem hiding this comment.
Just wanted to check if this is intended.
js allows any false values with the || operator. That is, if we had an undefined value, then it would default to ALL (as we want). However, empty or invalid strings, or null values would also end up defaulting to ALL from my understanding. Do we want to add checks for correct types?
There was a problem hiding this comment.
I am trying to keep this as loose as possible while defaulting to ALL in most error cases.
There was a problem hiding this comment.
I think we should be having stricter checks. In the case an unintended value is sent, we want to catch that ideally instead of defaulting to ALL (and not letting the user know). @svidgen what do you think?
- Add comprehensive E2E tests for KEYS_ONLY, INCLUDE, and ALL projection types - Tests validate GSI creation and query functionality for each projection type - Addresses reviewer feedback requesting E2E test coverage
ahmedhamouda78
force-pushed
the
feat/support-gsi-projection-types
branch
from
8ac302f to
24d86a1
Compare
3 participants
Description of changes
This PR adds projection support for the @index directive in Amplify GraphQL API. The changes enable developers to specify which fields should be projected in DynamoDB Global Secondary Indexes (GSI) when using the @index directive, providing better control over query performance and cost optimization.
Key changes:
CDK / CloudFormation Parameters Changed
ProjectionTypeandNonKeyAttributesparameters for DynamoDB Global Secondary Indexes. Prior to this PR, theProjectionTypeparameter was always set toALL. This PR allows developers to set theProjectionTypethemselves and defaults toALLif not set, so it doesn't break backwards compatibility.Issue #, if available
aws-amplify/amplify-data#604
Description of how you validated changes
amplify-graphql-index-transformer.test.tsChecklist
yarn testpassesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.