CORE-1836 Optimistic locking with version number

[benrj]

Inspired by: https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/DynamoDBMapper.OptimisticLocking.html

This adds optimistic locking to this library. In short, it optionally requires (after the first put/update) an item being mutated to include a version number that matches the current version number in the database. If it does not, it assumes it is trying to mutate a stale item and throws an error.

We won't support this for batch write operations, as the Dynamo APIs won't allow for use of conditional expressions in those operations. Question: Should we throw if a caller uses a batch write operation for a DataModel that has optimistic locking enabled? Or require them to specify the ignoreOptimisticLocking flag? Or just live and let live?

[austin-rausch]

This is awesome, I was probably going to be needing something like this soon so thanks for doing this!

Only other thing I have to mention is really just to feed discussion around it.

The update call doesn't increment the version attribute like the put call does, the linked doc mentions updates incrementing the number for the transactionWrite's update, but doesn't explicitly say otherwise.

I wonder if this should be either default behavior or opt-in? E.G. incrementVersionNumberOnUpdate?: boolean

Curious what everyone else thinks.

Nevermind, I see the increment is in the update params. This is awesome, thanks!

[austin-rausch]

Looks like your build is failing with a silly type error here since optimisticLockingAttribute?: keyof NumberPropertiesInType<T> is technically not assignable directly to undefined, it's just optional. You could either union that or do this fancy / semi hard to read conditional object spread syntax:

...(this.optimisticLockingAttribute
      ? {
          optimisticLockingAttribute:
            this.optimisticLockingAttribute.toString(),
        }
      : {}),

[benrj]

Thanks! As best I can tell, TS is happy with this line, but was displeased with line 103 in TestContext.ts. I did try your suggestion there but got the same complaint.

I ended up casting it like so:

const dao = new DynamoDbDao<DataModel, KeySchema>({
  tableName,
  documentClient,
  optimisticLockingAttribute: useOptimisticLocking ? 'version' : undefined,
} as DynamoDbDaoInput<DataModel>);

That said, I would like to know why it complains that Type 'string' is not assignable to type 'undefined' without the cast.

[austin-rausch]

I see why this is necessary, but I wonder if its possible to get a bit fancier with the NumberPropertiesInType and the constraint for the DataModel generic.

Maybe make the DataModel type explicitly extend a string record Record<string, any> or if that index type doesn't work because of the generic of Record {[key: string]: any}

Then you could get fancier with the NumberPropertiesInType and add a second ternary to make sure that the key is a string type.

E.G.

type NumberProp<T extends Record<string, any>> = Pick<
  T,
   {
     [K in keyof T]: K extends string ?
       T[k] extends number ? K
       : never
      : never;
    }[keyof T]
>;

That being said, all of that would become very hard to read & understand so this solution is probably better, we still have the guarantees on the constructor args that its a number attribute, just a slight risk with these casts that it could be used incorrectly in internal methods, but your tests are extremely thorough so thats unlikely.

[benrj]

That's a nifty idea.. although I think I might need to consider that a potential breaking change since it limits the DataModel generic.

Apart from that, though, when I tried that, I ran into this => microsoft/TypeScript#31661. 🤮

[tywalch]

Could the attribute name/value properties be post-fixed with a unique value (like _lock)? It would prevent accidentally overwriting a user's value if there ever was an overlap given how freeform things can be with the DAO

[benrj]

I thought about that, but since I'm letting the package caller tell me which attribute they want to store the version in (and enforcing it as numeric property of DataModel), I figure I'd let the callers deal with the potential consequences of choosing a version field.

I.e. if there is an overlap - the caller explicitly made that decision.

[ctdio]

Would be helpful to add a snippet that shows how to enable optimistic locking for the dao here.

[benrj]

Good idea - I've added that (along with a note in the changelog).

[ctdio]

Nice job with this! This is gonna be super helpful