refactor(glue): change physical name to optional (aws#23221)

Currently, we must set the physical name in the Glue L2 Constructs. This is not abide by the [CDK Best Practices](https://docs.aws.amazon.com/cdk/v2/guide/best-practices.html#best-practices-apps-names). So, this mean also customer can not abide by the best practices too. example ```ts const myDatabase = new glue.Database(this, "MyDatabase", { databaseName: "my-database", // need }) new glue.Table(this, "MyTable", { tableName: "my-tabel", // need database: myDatabase, columns: [...] }) ``` This PR fix physicalName in props to optonal and when not privided physicalName then generate physicalName by CDK. Then customers can abide by the best practices and no more coding while worrying about physical name conflicts. example after fixed ```ts const myDatabase = new glue.Database(this, "MyDatabase") new glue.Table(this, "MyTable", { database: myDatabase, columns: [...] }) ``` Also fixd the documents. ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Construct Runtime Dependencies: * [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
brennanho · Jan 20, 2023 · a2e2893 · a2e2893
1 parent 0791928
commit a2e2893
Show file tree

Hide file tree

Showing 12 changed files with 165 additions and 191 deletions.
diff --git a/packages/@aws-cdk/aws-glue/README.md b/packages/@aws-cdk/aws-glue/README.md
@@ -136,7 +136,6 @@ A `SecurityConfiguration` is a set of security properties that can be used by AW
 
 ```ts
 new glue.SecurityConfiguration(this, 'MySecurityConfiguration', {
-  securityConfigurationName: 'name',
   cloudWatchEncryption: {
     mode: glue.CloudWatchEncryptionMode.KMS,
   },
@@ -154,7 +153,6 @@ By default, a shared KMS key is created for use with the encryption configuratio
 ```ts
 declare const key: kms.Key;
 new glue.SecurityConfiguration(this, 'MySecurityConfiguration', {
-  securityConfigurationName: 'name',
   cloudWatchEncryption: {
     mode: glue.CloudWatchEncryptionMode.KMS,
     kmsKey: key,
@@ -169,9 +167,7 @@ See [documentation](https://docs.aws.amazon.com/glue/latest/dg/encryption-securi
 A `Database` is a logical grouping of `Tables` in the Glue Catalog.
 
 ```ts
-new glue.Database(this, 'MyDatabase', {
-  databaseName: 'my_database',
-});
+new glue.Database(this, 'MyDatabase');
 ```
 
 ## Table
@@ -182,7 +178,6 @@ A Glue table describes a table of data in S3: its structure (column names and ty
 declare const myDatabase: glue.Database;
 new glue.Table(this, 'MyTable', {
   database: myDatabase,
-  tableName: 'my_table',
   columns: [{
     name: 'col1',
     type: glue.Schema.STRING,
@@ -205,7 +200,6 @@ new glue.Table(this, 'MyTable', {
   s3Prefix: 'my-table/',
   // ...
   database: myDatabase,
-  tableName: 'my_table',
   columns: [{
     name: 'col1',
     type: glue.Schema.STRING,
@@ -224,7 +218,6 @@ To improve query performance, a table can specify `partitionKeys` on which data
 declare const myDatabase: glue.Database;
 new glue.Table(this, 'MyTable', {
   database: myDatabase,
-  tableName: 'my_table',
   columns: [{
     name: 'col1',
     type: glue.Schema.STRING,
@@ -256,7 +249,6 @@ property:
 declare const myDatabase: glue.Database;
 new glue.Table(this, 'MyTable', {
   database: myDatabase,
-  tableName: 'my_table',
   columns: [{
     name: 'col1',
     type: glue.Schema.STRING,
@@ -294,7 +286,6 @@ If you have a table with a large number of partitions that grows over time, cons
 declare const myDatabase: glue.Database;
 new glue.Table(this, 'MyTable', {
     database: myDatabase,
-    tableName: 'my_table',
     columns: [{
         name: 'col1',
         type: glue.Schema.STRING,
@@ -324,7 +315,6 @@ new glue.Table(this, 'MyTable', {
   encryption: glue.TableEncryption.S3_MANAGED,
   // ...
   database: myDatabase,
-  tableName: 'my_table',
   columns: [{
     name: 'col1',
     type: glue.Schema.STRING,
@@ -342,7 +332,6 @@ new glue.Table(this, 'MyTable', {
   encryption: glue.TableEncryption.KMS,
   // ...
   database: myDatabase,
-  tableName: 'my_table',
   columns: [{
     name: 'col1',
     type: glue.Schema.STRING,
@@ -356,7 +345,6 @@ new glue.Table(this, 'MyTable', {
   encryptionKey: new kms.Key(this, 'MyKey'),
   // ...
   database: myDatabase,
-  tableName: 'my_table',
   columns: [{
     name: 'col1',
     type: glue.Schema.STRING,
@@ -373,7 +361,6 @@ new glue.Table(this, 'MyTable', {
   encryption: glue.TableEncryption.KMS_MANAGED,
   // ...
   database: myDatabase,
-  tableName: 'my_table',
   columns: [{
     name: 'col1',
     type: glue.Schema.STRING,
@@ -391,7 +378,6 @@ new glue.Table(this, 'MyTable', {
   encryption: glue.TableEncryption.CLIENT_SIDE_KMS, 
   // ...
   database: myDatabase,
-  tableName: 'my_table',
   columns: [{
     name: 'col1',
     type: glue.Schema.STRING,
@@ -405,7 +391,6 @@ new glue.Table(this, 'MyTable', {
   encryptionKey: new kms.Key(this, 'MyKey'),
   // ...
   database: myDatabase,
-  tableName: 'my_table',
   columns: [{
     name: 'col1',
     type: glue.Schema.STRING,
@@ -447,7 +432,6 @@ new glue.Table(this, 'MyTable', {
   }],
   // ...
   database: myDatabase,
-  tableName: 'my_table',
   dataFormat: glue.DataFormat.JSON,
 });  
 ```

diff --git a/packages/@aws-cdk/aws-glue/lib/database.ts b/packages/@aws-cdk/aws-glue/lib/database.ts
@@ -1,4 +1,4 @@
-import { ArnFormat, IResource, Resource, Stack } from '@aws-cdk/core';
+import { ArnFormat, IResource, Lazy, Names, Resource, Stack } from '@aws-cdk/core';
 import { Construct } from 'constructs';
 import { CfnDatabase } from './glue.generated';
 
@@ -31,8 +31,10 @@ export interface IDatabase extends IResource {
 export interface DatabaseProps {
   /**
    * The name of the database.
+   *
+   * @default - generated by CDK.
    */
-  readonly databaseName: string;
+  readonly databaseName?: string;
 
   /**
    * The location of the database (for example, an HDFS path).
@@ -86,13 +88,16 @@ export class Database extends Resource implements IDatabase {
    */
   public readonly locationUri?: string;
 
-  constructor(scope: Construct, id: string, props: DatabaseProps) {
+  constructor(scope: Construct, id: string, props: DatabaseProps = {}) {
     super(scope, id, {
-      physicalName: props.databaseName,
+      physicalName: props.databaseName ??
+        Lazy.string({
+          produce: () => Names.uniqueResourceName(this, {}).toLowerCase(),
+        }),
     });
 
     let databaseInput: CfnDatabase.DatabaseInputProperty = {
-      name: props.databaseName,
+      name: this.physicalName,
     };
 
     if (props.locationUri !== undefined) {

diff --git a/packages/@aws-cdk/aws-glue/lib/security-configuration.ts b/packages/@aws-cdk/aws-glue/lib/security-configuration.ts
@@ -1,5 +1,6 @@
 import * as kms from '@aws-cdk/aws-kms';
 import * as cdk from '@aws-cdk/core';
+import { Lazy, Names } from '@aws-cdk/core';
 import * as constructs from 'constructs';
 import { CfnSecurityConfiguration } from './glue.generated';
 
@@ -114,8 +115,10 @@ export interface JobBookmarksEncryption {
 export interface SecurityConfigurationProps {
   /**
    * The name of the security configuration.
+   *
+   * @default - generated by CDK.
    */
-  readonly securityConfigurationName: string;
+  readonly securityConfigurationName?: string;
 
   /**
    * The encryption configuration for Amazon CloudWatch Logs.
@@ -184,9 +187,12 @@ export class SecurityConfiguration extends cdk.Resource implements ISecurityConf
    */
   public readonly s3EncryptionKey?: kms.IKey;
 
-  constructor(scope: constructs.Construct, id: string, props: SecurityConfigurationProps) {
+  constructor(scope: constructs.Construct, id: string, props: SecurityConfigurationProps = {}) {
     super(scope, id, {
-      physicalName: props.securityConfigurationName,
+      physicalName: props.securityConfigurationName ??
+        Lazy.string({
+          produce: () => Names.uniqueResourceName(this, {}),
+        }),
     });
 
     if (!props.s3Encryption && !props.cloudWatchEncryption && !props.jobBookmarksEncryption) {
@@ -230,7 +236,7 @@ export class SecurityConfiguration extends cdk.Resource implements ISecurityConf
     }
 
     const resource = new CfnSecurityConfiguration(this, 'Resource', {
-      name: props.securityConfigurationName,
+      name: this.physicalName,
       encryptionConfiguration: {
         cloudWatchEncryption,
         jobBookmarksEncryption,

diff --git a/packages/@aws-cdk/aws-glue/lib/table.ts b/packages/@aws-cdk/aws-glue/lib/table.ts
@@ -1,7 +1,7 @@
 import * as iam from '@aws-cdk/aws-iam';
 import * as kms from '@aws-cdk/aws-kms';
 import * as s3 from '@aws-cdk/aws-s3';
-import { ArnFormat, Fn, IResource, Names, Resource, Stack } from '@aws-cdk/core';
+import { ArnFormat, Fn, IResource, Lazy, Names, Resource, Stack } from '@aws-cdk/core';
 import * as cr from '@aws-cdk/custom-resources';
 import { AwsCustomResource } from '@aws-cdk/custom-resources';
 import { Construct } from 'constructs';
@@ -83,8 +83,10 @@ export interface TableAttributes {
 export interface TableProps {
   /**
    * Name of the table.
+   *
+   * @default - generated by CDK.
    */
-  readonly tableName: string;
+  readonly tableName?: string;
 
   /**
    * Description of the table.
@@ -282,7 +284,10 @@ export class Table extends Resource implements ITable {
 
   constructor(scope: Construct, id: string, props: TableProps) {
     super(scope, id, {
-      physicalName: props.tableName,
+      physicalName: props.tableName ??
+        Lazy.string({
+          produce: () => Names.uniqueResourceName(this, {}).toLowerCase(),
+        }),
     });
 
     this.database = props.database;
@@ -306,7 +311,7 @@ export class Table extends Resource implements ITable {
 
       tableInput: {
         name: this.physicalName,
-        description: props.description || `${props.tableName} generated by CDK`,
+        description: props.description || `${this.physicalName} generated by CDK`,
 
         partitionKeys: renderColumns(props.partitionKeys),
 

diff --git a/packages/@aws-cdk/aws-glue/test/database.test.ts b/packages/@aws-cdk/aws-glue/test/database.test.ts
@@ -14,9 +14,7 @@ beforeEach( () => {
 });
 
 test('default database does not create a bucket', () => {
-  new glue.Database(stack, 'Database', {
-    databaseName: 'test_database',
-  });
+  new glue.Database(stack, 'Database');
 
   Template.fromStack(stack).templateMatches({
     Resources: {
@@ -27,7 +25,7 @@ test('default database does not create a bucket', () => {
             Ref: 'AWS::AccountId',
           },
           DatabaseInput: {
-            Name: 'test_database',
+            Name: 'database',
           },
         },
       },
@@ -38,7 +36,6 @@ test('default database does not create a bucket', () => {
 
 test('explicit locationURI', () => {
   new glue.Database(stack, 'Database', {
-    databaseName: 'test_database',
     locationUri: 's3://my-uri/',
   });
 
@@ -52,7 +49,7 @@ test('explicit locationURI', () => {
           },
           DatabaseInput: {
             LocationUri: 's3://my-uri/',
-            Name: 'test_database',
+            Name: 'database',
           },
         },
       },
@@ -78,7 +75,6 @@ test('fromDatabase', () => {
 test('locationUri length must be >= 1', () => {
   expect(() =>
     new glue.Database(stack, 'Database', {
-      databaseName: 'test_database',
       locationUri: '',
     }),
   ).toThrow();
@@ -87,8 +83,18 @@ test('locationUri length must be >= 1', () => {
 test('locationUri length must be <= 1024', () => {
   expect(() =>
     new glue.Database(stack, 'Database', {
-      databaseName: 'test_database',
       locationUri: 'a'.repeat(1025),
     }),
   ).toThrow();
 });
+
+test('can specify a physical name', () => {
+  new glue.Database(stack, 'Database', {
+    databaseName: 'my_database',
+  });
+  Template.fromStack(stack).hasResourceProperties('AWS::Glue::Database', {
+    DatabaseInput: {
+      Name: 'my_database',
+    },
+  });
+});
diff --git a/packages/@aws-cdk/aws-glue/test/integ.connection.js.snapshot/aws-glue-connection.assets.json b/packages/@aws-cdk/aws-glue/test/integ.connection.js.snapshot/aws-glue-connection.assets.json
@@ -1,5 +1,5 @@
 {
-  "version": "20.0.0",
+  "version": "22.0.0",
   "files": {
     "f076d8bcdf0c659b5885c5698672513188f149a0fe66e8f214a4ff4eaf97fda9": {
       "source": {

diff --git a/packages/@aws-cdk/aws-glue/test/integ.connection.js.snapshot/cdk.out b/packages/@aws-cdk/aws-glue/test/integ.connection.js.snapshot/cdk.out
@@ -1 +1 @@
-{"version":"20.0.0"}
+{"version":"22.0.0"}
diff --git a/packages/@aws-cdk/aws-glue/test/integ.connection.js.snapshot/integ.json b/packages/@aws-cdk/aws-glue/test/integ.connection.js.snapshot/integ.json
@@ -1,5 +1,5 @@
 {
-  "version": "20.0.0",
+  "version": "22.0.0",
   "testCases": {
     "integ.connection": {
       "stacks": [

diff --git a/packages/@aws-cdk/aws-glue/test/integ.connection.js.snapshot/manifest.json b/packages/@aws-cdk/aws-glue/test/integ.connection.js.snapshot/manifest.json
@@ -1,12 +1,6 @@
 {
-  "version": "20.0.0",
+  "version": "22.0.0",
   "artifacts": {
-    "Tree": {
-      "type": "cdk:tree",
-      "properties": {
-        "file": "tree.json"
-      }
-    },
     "aws-glue-connection.assets": {
       "type": "cdk:asset-manifest",
       "properties": {
@@ -203,6 +197,12 @@
         ]
       },
       "displayName": "aws-glue-connection"
+    },
+    "Tree": {
+      "type": "cdk:tree",
+      "properties": {
+        "file": "tree.json"
+      }
     }
   }
 }