fix(redshift-alpha): extract tableName from custom resource functions #32452
Conversation
github-actions
bot
added
bug
There was a problem hiding this comment.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #32452 +/- ##
=======================================
Coverage 78.66% 78.66%
=======================================
Files 107 107
Lines 7161 7161
Branches 1320 1320
=======================================
Hits 5633 5633
Misses 1343 1343
Partials 185 185
Flags with carried forward coverage won't be shown. Click here to find out more.
|
5d
changed the title
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
5d
force-pushed
the
5d/fix-31817
branch
4 times, most recently
from
a2eb3e1 to
5d9c19d
Compare
aws-cdk-automation
added
the
pr/needs-maintainer-review
| (privileges, { table, actions }) => ({ | ||
| ...privileges, | ||
| [table.node.id]: { | ||
| actions: [ | ||
| ...(privileges[table.node.id]?.actions ?? []), | ||
| ...actions, | ||
| ], | ||
| tableName: table.tableName, | ||
| actions: [], | ||
| }; | ||
| } | ||
| actions = actions.concat(privileges[tableId].actions); | ||
| if (actions.includes(TableAction.ALL)) { | ||
| actions = [TableAction.ALL]; | ||
| } | ||
| if (actions.includes(TableAction.UPDATE) || actions.includes(TableAction.DELETE)) { | ||
| actions.push(TableAction.SELECT); | ||
| } | ||
| privileges[tableId] = { | ||
| tableName: table.tableName, | ||
| actions: Array.from(new Set(actions)), | ||
| }; | ||
| return privileges; | ||
| }, {} as { [key: string]: { tableName: string; actions: TableAction[] } }); | ||
| const serializedPrivileges: SerializedTablePrivilege[] = Object.entries(reducedPrivileges).map(([tableId, config]) => ({ | ||
| }, | ||
| }), | ||
| {} as Record<string, { tableName: string; actions: TableAction[] }>, | ||
| ); |
There was a problem hiding this comment.
Can we make this code more readable? It's already inside reduce and the use of spread operator with arrow function make this code hard to read. IMO, this can be simple and straightforward like
const groupedPrivileges = this.privileges.reduce(
(acc, { table, actions }) => {
const currentActions = acc[table.node.id]?.actions ?? [];
const updatedActions = [...currentActions, ...actions];
acc[table.node.id] = {
actions: updatedActions,
tableName: table.tableName,
};
return acc;
},
{} as Record<string, { tableName: string; actions: TableAction[] }>
);
There was a problem hiding this comment.
ok, I was using a single expression to do it here. I will update it with a function.
packages/@aws-cdk/aws-redshift-alpha/lib/private/database-query-provider/table.ts
Show resolved
Hide resolved
|
|
||
| /** | ||
| * We need this normalization logic because some of the `TableName` values | ||
| * are physical IDs generated in the `./util.ts` module. |
There was a problem hiding this comment.
I don't see a util.ts file in this module so I'm not sure how the TableName was generated. Can you share the link to the function?
There was a problem hiding this comment.
I will add the module link in the comment.
aws-cdk-automation
removed
the
pr/needs-maintainer-review
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
Closes
Reason for this change
This regression was introduced in PR#24308, which added support for executing the
ALT TABLESQL statement when the table name is changed in the CDK construct. The root cause is in the modification of the physical ID of the custom resource Lambda function. Previously, this Lambda function was returning thetableNameafter table is created. However, the physical ID was updated to a combined ID in the formatclusterId:databaseName:tableNamePrefix:stackIdSuffix.This change breaks the logic that depends on the return value being used as the
tableName. For example, in the reported issue, the integration test relies on this value to grant permissions to specific users. The new combined ID format causes SQL statement errors, as described in the issue.Description of changes
The fix involves adding logic to detect whether the resource's physical ID is in the new format. If it is, the logic reconstructs the
tableNamefrom the combined ID. This ensures that Lambda functions referencing thetableNamereceive the correct value.Description of how you validated changes
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license