fix(allow ITable for dynamodb stream patterns) #214

hnishar · 2021-06-08T18:10:14Z

Issue #, if available: 119

Description of changes:

Allow users to pass dynamodb.ITable for in construct props existingTableObj for dynamodb stream patterns

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

aws-solutions-constructs-team · 2021-06-08T18:16:48Z

AWS CodeBuild CI Report

CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
Commit ID: 7451a9a
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

biffgaut · 2021-06-08T18:28:56Z

source/patterns/@aws-solutions-constructs/aws-dynamodb-stream-lambda/lib/index.ts

@@ -69,7 +69,7 @@ export interface DynamoDBStreamToLambdaProps {

 export class DynamoDBStreamToLambda extends Construct {
  public readonly lambdaFunction: lambda.Function;
-  public readonly dynamoTable: dynamodb.Table;
+  public readonly dynamoTable?: dynamodb.Table;


I'd expose both this as an optional property as well as the ITable, which will always exist. Then we need to document well when the Table object would be available.

Are you suggesting something like this:

public readonly dynamoTable?: dynamodb.Table; public readonly dynamoTableInterface?: dynamodb.ITable;

where dynamoTable will be available if the user user provides dynamoTableProps or existingTableObj of type Table and dynamoTableInterface will be available if the user provides existingTableObj of type ITable ?

I was thinking:

public readonly dynamoTable?: dynamodb.Table; public readonly dynamoTableInterface: dynamodb.ITable;

(making as much info as possible available). But I hadn't thought about names, that's unfortunate.

As I recall, if the client supplies existingTableObject the code currently returns ITable regardless of what was passed to the ITable parameter. This is actually pretty sticky - why don't you set up a meeting tomorrow morning for you, Ryan and me to toss around ideas. The cleanest solution would be to go ITable for everything about this construct - the only drawback is that if someone needs the table somewhere else they can't use it. The workaround would be to create the table somewhere else (either independently or as part of another construct).

Yep, cannot go with ITable for everything then the user won't be able to make mutating calls on dynamoTable property e.g. dynamoTable.addGlobalSecondaryIndex(), also it will break the existing customer's code..

biffgaut · 2021-06-08T18:35:03Z

source/patterns/@aws-solutions-constructs/aws-dynamodb-stream-lambda/lib/index.ts

      dynamoTableProps: props.dynamoTableProps,
      existingTableObj: props.existingTableObj
    });

+    if (table instanceof dynamodb.Table) {


The definition of buildDynamoDBTableWithStream is:

export function buildDynamoDBTableWithStream(scope: cdk.Construct, props: BuildDynamoDBTableWithStreamProps): dynamodb.ITable {

Typescript sees this and will never trigger this condition (pretty sure instanceof isn't fully functional at runtime as the code actually runs as JavaScript with no type information). Either way, the function will never return a Table.

First thought suggestion, define the function as:

export function buildDynamoDBTableWithStream(scope: cdk.Construct, props: BuildDynamoDBTableWithStreamProps): [ dynamodb.ITable, dynamodbTable ] {

Then return everything you can from the from the function (either ITable, undefined or ITable, Table). Assign both to properties of the construct (and stressing again to explain the difference in the docs). I'm not a fan of this operator to return multiple values, but it seems to work here. There may be a better way.

Good catch!

aws-solutions-constructs-team · 2021-06-09T18:14:20Z

AWS CodeBuild CI Report

CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
Commit ID: 172e127
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-solutions-constructs-team · 2021-06-09T18:31:08Z

AWS CodeBuild CI Report

CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
Commit ID: 8ff9b21
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

biffgaut · 2021-06-09T18:26:55Z

...patterns/@aws-solutions-constructs/aws-dynamodb-stream-lambda-elasticsearch-kibana/README.md

@@ -73,7 +73,8 @@ _Parameters_

 | **Name**     | **Type**        | **Description** |
 |:-------------|:----------------|-----------------|
-|dynamoTable|[`dynamodb.Table`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-dynamodb.Table.html)|Returns an instance of dynamodb.Table created by the construct|
+|dynamoTableInterface|[`dynamodb.ITable`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-dynamodb.ITable.html)|Returns an instance of dynamodb.ITable created by the construct|
+|dynamoTable?|[`dynamodb.Table`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-dynamodb.Table.html)|Returns an instance of dynamodb.Table created by the construct|


We should add:

Returns an instance of dynamodb.Table created by the construct. IMPORTANT: If existingTableInterface was provided in the props object, this property will be undefined.

biffgaut · 2021-06-09T18:27:38Z

source/patterns/@aws-solutions-constructs/aws-dynamodb-stream-lambda/README.md

 |dynamoEventSourceProps?|[`aws-lambda-event-sources.DynamoEventSourceProps`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-lambda-event-sources.DynamoEventSourceProps.html)|Optional user provided props to override the default props for DynamoDB Event Source|

 ## Pattern Properties

 | **Name**     | **Type**        | **Description** |
 |:-------------|:----------------|-----------------|
-|dynamoTable|[`dynamodb.Table`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-dynamodb.Table.html)|Returns an instance of dynamodb.Table created by the construct|
+|dynamoTableInterface|[`dynamodb.ITable`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-dynamodb.ITable.html)|Returns an instance of dynamodb.ITable created by the construct|
+|dynamoTable?|[`dynamodb.Table`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-dynamodb.Table.html)|Returns an instance of dynamodb.Table created by the construct|


Same comment

biffgaut · 2021-06-09T18:33:55Z

source/patterns/@aws-solutions-constructs/aws-dynamodb-stream-lambda/lib/index.ts

@@ -90,11 +91,12 @@ export class DynamoDBStreamToLambda extends Construct {

    const table: dynamodb.ITable = defaults.buildDynamoDBTableWithStream(this, {


Purposefully pointing to an object as the wrong type seems a little cheesy (and fraught with peril). I like the returning 2 values approach better. It also allows cleaner code in this routine, as this line could be"

[dynamoTableInterface, dynamoTable] = defaults.buildDynamoDBTableWithStream(...

Lines 97-99 are then no longer needed

aws-solutions-constructs-team · 2021-06-09T19:35:01Z

AWS CodeBuild CI Report

CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
Commit ID: 774b75c
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-solutions-constructs-team · 2021-06-09T19:42:11Z

AWS CodeBuild CI Report

CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
Commit ID: aa9da31
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

fix(allow ITable for dynamodb stream patterns)

7451a9a

hnishar requested a review from biffgaut as a code owner June 8, 2021 18:10

biffgaut requested changes Jun 8, 2021

View reviewed changes

hnishar linked an issue Jun 8, 2021 that may be closed by this pull request

aws_dynamodb_stream_lambda raises exception when using existing Table #119

Closed

fix(allow ITable for dynamodb stream patterns)

172e127

fixing the cfn nag issues

8ff9b21

biffgaut requested changes Jun 9, 2021

View reviewed changes

PR feedback

774b75c

biffgaut approved these changes Jun 9, 2021

View reviewed changes

Merge branch 'main' into ddb-stream-itable-fix

aa9da31

hnishar merged commit 9f1d65d into main Jun 9, 2021

hnishar deleted the ddb-stream-itable-fix branch June 9, 2021 19:46

hnishar self-assigned this Jul 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(allow ITable for dynamodb stream patterns) #214

fix(allow ITable for dynamodb stream patterns) #214

hnishar commented Jun 8, 2021

aws-solutions-constructs-team commented Jun 8, 2021

biffgaut Jun 8, 2021

hnishar Jun 8, 2021

biffgaut Jun 8, 2021

hnishar Jun 8, 2021

biffgaut Jun 8, 2021

biffgaut Jun 8, 2021

hnishar Jun 8, 2021

aws-solutions-constructs-team commented Jun 9, 2021

aws-solutions-constructs-team commented Jun 9, 2021

biffgaut Jun 9, 2021

biffgaut Jun 9, 2021

biffgaut Jun 9, 2021

aws-solutions-constructs-team commented Jun 9, 2021

aws-solutions-constructs-team commented Jun 9, 2021

		@@ -90,11 +91,12 @@ export class DynamoDBStreamToLambda extends Construct {

		const table: dynamodb.ITable = defaults.buildDynamoDBTableWithStream(this, {

fix(allow ITable for dynamodb stream patterns) #214

fix(allow ITable for dynamodb stream patterns) #214

Conversation

hnishar commented Jun 8, 2021

aws-solutions-constructs-team commented Jun 8, 2021

AWS CodeBuild CI Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-solutions-constructs-team commented Jun 9, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Jun 9, 2021

AWS CodeBuild CI Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-solutions-constructs-team commented Jun 9, 2021

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Jun 9, 2021

AWS CodeBuild CI Report