fix(redshift): tables were dropped on table name change

packages/@aws-cdk/aws-redshift/lib/private/database-query-provider/privileges.ts

@@ -65,7 +65,6 @@ async function updatePrivileges(
   if (oldTablePrivileges !== tablePrivileges) {
     await revokePrivileges(username, oldTablePrivileges, clusterProps);
     await grantPrivileges(username, tablePrivileges, clusterProps);
-    return { replace: false };
   }
 
   return { replace: false };

packages/@aws-cdk/aws-redshift/lib/private/database-query-provider/table.ts

@@ -13,20 +13,26 @@ export async function handler(props: TableAndClusterProps, event: AWSLambda.Clou
 
   if (event.RequestType === 'Create') {
     const tableName = await createTable(tableNamePrefix, tableNameSuffix, tableColumns, tableAndClusterProps);
-    return { PhysicalResourceId: tableName };
+    return {
+      PhysicalResourceId: tableName,
+      Data: { TableName: tableName },
+    };
   } else if (event.RequestType === 'Delete') {
-    await dropTable(event.PhysicalResourceId, tableAndClusterProps);
+    await dropTable(event.ResourceProperties.Data.TableName, tableAndClusterProps);
     return;
   } else if (event.RequestType === 'Update') {
     const tableName = await updateTable(
-      event.PhysicalResourceId,
+      event.OldResourceProperties.Data.TableName ?? event.PhysicalResourceId,
       tableNamePrefix,
       tableNameSuffix,
       tableColumns,
       tableAndClusterProps,
       event.OldResourceProperties as TableAndClusterProps,
     );
-    return { PhysicalResourceId: tableName };
+    return {
+      PhysicalResourceId: event.PhysicalResourceId,
+      Data: { TableName: tableName },
+    };
   } else {
     /* eslint-disable-next-line dot-notation */
     throw new Error(`Unrecognized event type: ${event['RequestType']}`);
@@ -87,11 +93,6 @@ async function updateTable(
     return createTable(tableNamePrefix, tableNameSuffix, tableColumns, tableAndClusterProps);
   }
 
-  const oldTableNamePrefix = oldResourceProperties.tableName.prefix;
-  if (tableNamePrefix !== oldTableNamePrefix) {
-    return createTable(tableNamePrefix, tableNameSuffix, tableColumns, tableAndClusterProps);
-  }
-
   const oldTableColumns = oldResourceProperties.tableColumns;
   const columnDeletions = oldTableColumns.filter(oldColumn => (
     tableColumns.every(column => oldColumn.name !== column.name)
@@ -156,6 +157,12 @@ async function updateTable(
 
   await Promise.all(alterationStatements.map(statement => executeStatement(statement, tableAndClusterProps)));
 
+  const oldTableNamePrefix = oldResourceProperties.tableName.prefix;
+  if (tableNamePrefix !== oldTableNamePrefix) {
+    await executeStatement(`ALTER TABLE ${tableName} RENAME TO ${tableNamePrefix + tableNameSuffix}`, tableAndClusterProps);
+    return tableNamePrefix + tableNameSuffix;
+  }
+
   return tableName;
 }
 

packages/@aws-cdk/aws-redshift/package.json

@@ -88,7 +88,8 @@
     "@aws-cdk/integ-tests": "0.0.0",
     "@types/jest": "^27.5.2",
     "aws-sdk": "^2.1317.0",
-    "jest": "^27.5.1"
+    "jest": "^27.5.1",
+    "jsii": "v4.9-next"
   },
   "dependencies": {
     "@aws-cdk/aws-ec2": "0.0.0",

packages/@aws-cdk/aws-redshift/test/database-query-provider/table.test.ts

@@ -10,14 +10,16 @@ import { Column, TableDistStyle, TableSortStyle } from '../../lib';
 import { handler as manageTable } from '../../lib/private/database-query-provider/table';
 import { TableAndClusterProps } from '../../lib/private/database-query-provider/types';
 
-type ResourcePropertiesType = TableAndClusterProps & { ServiceToken: string };
+type ResourcePropertiesType = TableAndClusterProps & { ServiceToken: string, Data: { TableName: string } };
 
 const tableNamePrefix = 'tableNamePrefix';
 const tableColumns = [{ name: 'col1', dataType: 'varchar(1)' }];
 const clusterName = 'clusterName';
 const adminUserArn = 'adminUserArn';
 const databaseName = 'databaseName';
 const physicalResourceId = 'PhysicalResourceId';
+const requestId = 'requestId';
+const requestIdTruncated = 'requestI';
 const resourceProperties: ResourcePropertiesType = {
   tableName: {
     prefix: tableNamePrefix,
@@ -28,10 +30,11 @@ const resourceProperties: ResourcePropertiesType = {
   clusterName,
   adminUserArn,
   databaseName,
+  Data: {
+    TableName: `${tableNamePrefix}${requestIdTruncated}`,
+  },
   ServiceToken: '',
 };
-const requestId = 'requestId';
-const requestIdTruncated = 'requestI';
 const genericEvent: AWSLambda.CloudFormationCustomResourceEventCommon = {
   ResourceProperties: resourceProperties,
   ServiceToken: '',
@@ -56,6 +59,9 @@ describe('create', () => {
     const event = baseEvent;
 
     await expect(manageTable(resourceProperties, event)).resolves.toEqual({
+      Data: {
+        TableName: `${tableNamePrefix}${requestIdTruncated}`,
+      },
       PhysicalResourceId: `${tableNamePrefix}${requestIdTruncated}`,
     });
     expect(mockExecuteStatement).toHaveBeenCalledWith(expect.objectContaining({
@@ -74,6 +80,9 @@ describe('create', () => {
     };
 
     await expect(manageTable(newResourceProperties, event)).resolves.toEqual({
+      Data: {
+        TableName: tableNamePrefix,
+      },
       PhysicalResourceId: tableNamePrefix,
     });
     expect(mockExecuteStatement).toHaveBeenCalledWith(expect.objectContaining({
@@ -163,7 +172,7 @@ describe('delete', () => {
     await manageTable(resourceProperties, event);
 
     expect(mockExecuteStatement).toHaveBeenCalledWith(expect.objectContaining({
-      Sql: `DROP TABLE ${physicalResourceId}`,
+      Sql: `DROP TABLE ${event.ResourceProperties.Data.TableName}`,
     }));
   });
 });
@@ -200,6 +209,9 @@ describe('update', () => {
     };
 
     await expect(manageTable(newResourceProperties, event)).resolves.toMatchObject({
+      Data: {
+        TableName: `${tableNamePrefix}${requestIdTruncated}`,
+      },
       PhysicalResourceId: physicalResourceId,
     });
     expect(mockExecuteStatement).not.toHaveBeenCalled();
@@ -221,22 +233,58 @@ describe('update', () => {
     }));
   });
 
-  test('replaces if table name changes', async () => {
-    const newTableNamePrefix = 'newTableNamePrefix';
-    const newResourceProperties = {
-      ...resourceProperties,
-      tableName: {
-        ...resourceProperties.tableName,
-        prefix: newTableNamePrefix,
-      },
-    };
+  describe('table name', () => {
+    test('does not replace if table name changes', async () => {
+      const newResourceProperties = {
+        ...resourceProperties,
+        tableName: {
+          ...resourceProperties.tableName,
+          prefix: 'newTableName',
+          generateSuffix: 'false',
+        },
+      };
 
-    await expect(manageTable(newResourceProperties, event)).resolves.not.toMatchObject({
-      PhysicalResourceId: physicalResourceId,
+      await expect(manageTable(newResourceProperties, event)).resolves.toMatchObject({
+        PhysicalResourceId: physicalResourceId,
+      });
+      expect(mockExecuteStatement).toHaveBeenCalledWith(expect.objectContaining({
+        Sql: `ALTER TABLE ${event.OldResourceProperties.Data.TableName} RENAME TO newTableName`,
+      }));
+    });
+
+    test('does not replace if table name added', async () => {
+      const newResourceProperties = {
+        ...resourceProperties,
+        tableName: {
+          prefix: 'newTable',
+          generateSuffix: 'false',
+        },
+      };
+
+      await expect(manageTable(newResourceProperties, event)).resolves.toMatchObject({
+        PhysicalResourceId: physicalResourceId,
+      });
+      expect(mockExecuteStatement).toHaveBeenCalledWith(expect.objectContaining({
+        Sql: `ALTER TABLE ${event.OldResourceProperties.Data.TableName} RENAME TO newTable`,
+      }));
+    });
+
+    test('does not replace if table name removed', async () => {
+      const newResourceProperties = {
+        ...resourceProperties,
+        tableName: {
+          prefix: 'Table',
+          generateSuffix: 'true',
+        },
+      };
+
+      await expect(manageTable(newResourceProperties, event)).resolves.toMatchObject({
+        PhysicalResourceId: physicalResourceId,
+      });
+      expect(mockExecuteStatement).toHaveBeenCalledWith(expect.objectContaining({
+        Sql: `ALTER TABLE ${event.OldResourceProperties.Data.TableName} RENAME TO Table${requestIdTruncated}`,
+      }));
     });
-    expect(mockExecuteStatement).toHaveBeenCalledWith(expect.objectContaining({
-      Sql: expect.stringMatching(new RegExp(`CREATE TABLE ${newTableNamePrefix}${requestIdTruncated}`)),
-    }));
   });
 
   test('does not replace if table columns removed', async () => {
@@ -249,7 +297,7 @@ describe('update', () => {
       PhysicalResourceId: physicalResourceId,
     });
     expect(mockExecuteStatement).toHaveBeenCalledWith(expect.objectContaining({
-      Sql: `ALTER TABLE ${physicalResourceId} DROP COLUMN col1`,
+      Sql: expect.stringMatching(new RegExp(`ALTER TABLE ${newResourceProperties.tableName.prefix}.+ DROP COLUMN col1`)),
     }));
   });
 
@@ -266,7 +314,7 @@ describe('update', () => {
       PhysicalResourceId: physicalResourceId,
     });
     expect(mockExecuteStatement).toHaveBeenCalledWith(expect.objectContaining({
-      Sql: `ALTER TABLE ${physicalResourceId} ADD ${newTableColumnName} ${newTableColumnDataType}`,
+      Sql: `ALTER TABLE ${event.OldResourceProperties.Data.TableName} ADD ${newTableColumnName} ${newTableColumnDataType}`,
     }));
   });
 
@@ -323,7 +371,7 @@ describe('update', () => {
         PhysicalResourceId: physicalResourceId,
       });
       expect(mockExecuteStatement).toHaveBeenCalledWith(expect.objectContaining({
-        Sql: `ALTER TABLE ${physicalResourceId} ALTER DISTSTYLE ${newDistStyle}`,
+        Sql: `ALTER TABLE ${newEvent.OldResourceProperties.Data.TableName} ALTER DISTSTYLE ${newDistStyle}`,
       }));
     });
 
@@ -385,7 +433,7 @@ describe('update', () => {
         PhysicalResourceId: physicalResourceId,
       });
       expect(mockExecuteStatement).toHaveBeenCalledWith(expect.objectContaining({
-        Sql: `ALTER TABLE ${physicalResourceId} ALTER DISTKEY ${newDistKey}`,
+        Sql: `ALTER TABLE ${newEvent.OldResourceProperties.Data.TableName} ALTER DISTKEY ${newDistKey}`,
       }));
     });
   });
@@ -465,7 +513,7 @@ describe('update', () => {
         PhysicalResourceId: physicalResourceId,
       });
       expect(mockExecuteStatement).toHaveBeenCalledWith(expect.objectContaining({
-        Sql: `ALTER TABLE ${physicalResourceId} ALTER COMPOUND SORTKEY(col2)`,
+        Sql: `ALTER TABLE ${newEvent.OldResourceProperties.Data.TableName} ALTER COMPOUND SORTKEY(col2)`,
       }));
     });
 
@@ -488,7 +536,7 @@ describe('update', () => {
         PhysicalResourceId: physicalResourceId,
       });
       expect(mockExecuteStatement).toHaveBeenCalledWith(expect.objectContaining({
-        Sql: `ALTER TABLE ${physicalResourceId} ALTER COMPOUND SORTKEY(col1)`,
+        Sql: `ALTER TABLE ${newEvent.OldResourceProperties.Data.TableName} ALTER COMPOUND SORTKEY(col1)`,
       }));
     });
 
@@ -511,7 +559,7 @@ describe('update', () => {
         PhysicalResourceId: physicalResourceId,
       });
       expect(mockExecuteStatement).toHaveBeenCalledWith(expect.objectContaining({
-        Sql: `ALTER TABLE ${physicalResourceId} ALTER SORTKEY AUTO`,
+        Sql: `ALTER TABLE ${newEvent.OldResourceProperties.Data.TableName} ALTER SORTKEY AUTO`,
       }));
     });
   });
@@ -528,7 +576,7 @@ describe('update', () => {
         PhysicalResourceId: physicalResourceId,
       });
       expect(mockExecuteStatement).toHaveBeenCalledWith(expect.objectContaining({
-        Sql: `COMMENT ON TABLE ${physicalResourceId} IS '${newComment}'`,
+        Sql: `COMMENT ON TABLE ${event.OldResourceProperties.Data.TableName} IS '${newComment}'`,
       }));
     });
 
@@ -548,7 +596,7 @@ describe('update', () => {
         PhysicalResourceId: physicalResourceId,
       });
       expect(mockExecuteStatement).toHaveBeenCalledWith(expect.objectContaining({
-        Sql: `COMMENT ON TABLE ${physicalResourceId} IS NULL`,
+        Sql: `COMMENT ON TABLE ${event.OldResourceProperties.Data.TableName} IS NULL`,
       }));
     });
   });

packages/@aws-cdk/aws-redshift/test/integ.database.ts

@@ -42,6 +42,7 @@ const databaseOptions = {
 const user = new redshift.User(stack, 'User', databaseOptions);
 const table = new redshift.Table(stack, 'Table', {
   ...databaseOptions,
+  tableName: 'IntegTable',
   tableColumns: [
     { name: 'col1', dataType: 'varchar(4)', distKey: true },
     { name: 'col2', dataType: 'float', sortKey: true },