-
Notifications
You must be signed in to change notification settings - Fork 89
feat: import existing table to amplify managed table #2634
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
Conversation
...ges/amplify-graphql-model-transformer/src/__tests__/amplify-dynamodb-table-generator.test.ts
Outdated
Show resolved
Hide resolved
| expect(() => validateImportedTableMap(definition)).toThrow('Table mapping is missing for imported Amplify DynamoDB table strategy.'); | ||
| }); | ||
| }); | ||
| }); |
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.
We should also have multi-strategy tests. One case should ensure that a model that uses a different strategy throws an error if it is included in the importedAmplifyDynamoDBTableMap. Ideally, the error would note that the strategy is invalid, rather than a generic "missing" or "invalid" error. E.g., from your generator test below:
dataSourceStrategies: {
Comment: DDB_DEFAULT_DATASOURCE_STRATEGY,
Post: DDB_AMPLIFY_MANAGED_DATASOURCE_STRATEGY,
Author: IMPORTED_DDB_AMPLIFY_MANAGED_DATASOURCE_STRATEGY,
},
importedAmplifyDynamoDBTableMap: {
Author: 'Author-myApiId-myEnv',
},If you add Post to the map, we'd hope to see an error like "Table mapping includes models that do not use an imported strategy (Post)." or similar
| }; | ||
|
|
||
| /** | ||
| * Type predicate that returns true if `obj` is a AmplifyDynamoDbModelDataSourceStrategy |
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.
| * Type predicate that returns true if `obj` is a AmplifyDynamoDbModelDataSourceStrategy | |
| * Type predicate that returns true if `obj` is an ImportedAmplifyDynamoDbModelDataSourceStrategy |
packages/amplify-graphql-model-transformer/src/graphql-model-transformer.ts
Outdated
Show resolved
Hide resolved
| const tableLogicalName = ModelResourceIDs.ModelTableResourceID(modelName); | ||
| const tableName = context.resourceHelper.generateTableName(modelName); | ||
| const importedTableMap = context.importedAmplifyDynamoDBTableMap ?? {}; | ||
| const isTableImported = importedTableMap[modelName] !== undefined; |
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.
Piling onto my above comments -- here you check for the existence of the importedTableMap, but don't validate the strategy. If we have conditions that can result in inconsistency b/t strategy & mapping, this is a source of potential future bugs.
...sources/amplify-dynamodb-table/amplify-table-manager-lambda/amplify-table-manager-handler.ts
Outdated
Show resolved
Hide resolved
| let result; | ||
| switch (event.RequestType) { | ||
| case 'Create': | ||
| if (tableDef.isImported) { |
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.
- rather than inlining this big
ifblock, can we refactor to a standalone function to make the "Create" case a bit easier to parse? - I don't see anything to suggest otherwise, but please confirm that this won't cause any issues with the Tagging support for Amplify-managed tables that we're introducing in fix: add tags to dynamodb tables #2694
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.
- Will do.
- I don't see any issues here, but I'll add a test to cover this case before merging to main.
| switch (event.RequestType) { | ||
| case 'Create': | ||
| if (tableDef.isImported) { | ||
| // TODO: Add import validation |
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.
What validation needs to be added?
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.
The validation is to ensure the DDB properties align (Example: KeySchema, SSESpecification, etc.). WIP PR: #2696
| }, | ||
| }, | ||
| }; | ||
| // @ts-expect-error |
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 assume the @ts-expect-error is to suppress the compile warning about the missing tableName attribute?
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.
Yes, updated to clarify
| typeof (strategy as any)['provisionStrategy'] === 'string' && | ||
| (strategy as any)['provisionStrategy'] === 'IMPORTED_AMPLIFY_TABLE' && | ||
| typeof (strategy as any)['tableName'] === 'string' && | ||
| (strategy as any)['tableName'] !== '' |
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.
Should we turn this "empty imported table name" scenario handling into a validation check and fail instead of treating this as a greenfield scenario?
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 would actually fail with No resource generator assigned for Post with dbType DYNAMODB. The error message could be improved though. I can tackle this in the next PR.
| // TODO: Add import validation | ||
| console.log('Initiating table import process'); | ||
| console.log('Fetching current table state'); | ||
| console.log(`Table name: ${tableDef.tableName}`); |
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.
minor nit for next PR: (since you already have a TODO up there):
Can combine above two lines to console.log('Fetching current state of table ${tableDef.tableName}'); to make it more readable in the logs.
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'll update this in the next PR.
Description of changes
Add a feature to import existing DynamoDB tables to an Amplify managed table. Import validation will come with a later PR.
CDK / CloudFormation Parameters Changed
IMPORTED_AMPLIFY_TABLE.importedAmplifyDynamoDBTableMap.importedAmplifyDynamoDBTableMapwill define a mapping of model name to imported table name. All models defined with theIMPORTED_AMPLIFY_TABLEshould have a matching key in theimportedAmplifyDynamoDBTableMapprop.Issue #, if available
N/A
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.