BREAKING: Support DynamoDB LSI

[jungseoklee]

Changes since v1:

• Introduce separate properties for GSI and LSI
• Throw an error unless a partition key of a table is specified first
• Rebase on top of the latest tip

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

Summary

This patch supports DynamoDB LSI, which is part of #655.

Test

OK: 53 assertions (1906ms)

=============================== Coverage summary ===============================
Statements   : 98.48% ( 130/132 )
Branches     : 97.75% ( 87/89 )
Functions    : 100% ( 26/26 )
Lines        : 98.45% ( 127/129 )
================================================================================
Verifying integ.dynamodb.js against integ.dynamodb.expected.json... OK.

Notes

• I updated integ.dynamodb.expected.json through npm run integ command.

[rix0rrr]

Why make it even possible to have this mistake? Make two separate props types, inherit one from the other

[jungseoklee]

My intention was to unify props for GSI and LSI. As you pointed out, having separate props definitely helps users not to make mistake and simplifies validation logic. Regarding inheritance, GSI and LSI are different in terms of characteristics, especially key schema and read consistency, so I propose the following.

export interface SecondaryIndexProps{
  indexName: string;
  projectionType?: ProjectionType;
  nonKeyAttributes?: string[];
};

export interface GlobalSecondaryIndexProps extends SecondaryIndexProps {
  partitionKey: Attribute;
  sortKey?: Attibute;
  readCapacity?: number;
  writeCapacity?: number;
}

// ideally, but..
export interface LocalSecondaryIndexProps extends SecondaryIndexProps {
  sortKey: Attribute;
}

// we need partition key here as well. please check the below comment.
export interface LocalSecondaryIndexProps extends SecondaryIndexProps {
  partitionKey: Attribute;
  sortKey: Attribute;
}

How do you think about it?

[rix0rrr]

Yes, perfect!

[rix0rrr]

Why even allow specifying a partition key then?

[jungseoklee]

If we don't specify a partition key, then we could not infer the second scenario in the below.

// OK
table.addPartitionKey();
table.addLocalSecondaryIndex();

// we cannot infer a partition key of table when addLocalSecondaryIndex() is called.
table.addLocalSecondaryIndex();
table.addPartitionKey();

All validation complexities come from the fact that we allow lazy initialization of a base table's key schema via addPartitionKey. An underlying principle of the PR is that all public methods should be independent each other.

[rix0rrr]

I see. That's an admirable goal!

Still, I think we'd also be perfectly fine with requiring that people call addPartitionKey() before calling addLocalSecondaryIndex() (so we can just check for it and throw an error), thereby simplifying the implementation a lot.

If you insist on either call order, you can also use cdk.Token() (which lazily evaluates values) to return the eventual partition key during synthesis. You'll have to change the implementation somewhat, but it should end up looking like:

this.localSecondaryIndexes.push({
  keySchema: [
    new cdk.Token(() => this.partitionKeySchema),  // <-- notice this
    rangeKeySchema
  ],
  indexName: ...
  projection: ...
});

What the cdk.Token does is delay the retrieval of this.partitionKeySchema until after the complete setup has happened, so that it has a chance to have been set later on.

[jungseoklee]

Still, I think we'd also be perfectly fine with requiring that people call addPartitionKey() before calling addLocalSecondaryIndex() (so we can just check for it and throw an error), thereby simplifying the implementation a lot.

I will go this way. I agree with the points you mentioned. In addition, not adding a partition key to LocalSecondaryIndexProps would be more aligned with the LSI nature, the same partition key as the one of a base table.

[rix0rrr]

Double validation?

[jungseoklee]

Whereas this validation is for the following,

table.addPartitionKey();
table.addLocalSecondaryIndex();

the above check where you left the second comment is for the below case:

table.addLocalSecondaryIndex();
table.addPartitionKey();

[jungseoklee]

This is a breaking change because SecondaryIndexProps has been updated.

[rix0rrr]

You're technically correct, but I'm going to say it's not breaking since customers shouldn't be using SecondaryIndexProps directly anyway.