Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CORE-1836 Optimistic locking with version number #24

Merged
merged 11 commits into from
Oct 29, 2021
Merged

CORE-1836 Optimistic locking with version number #24

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
4899f07
Add version increment to `update`
benrj Oct 19, 2021
53b0020
Implement optimistic lock via version number
benrj Oct 19, 2021
bd11283
Add delete support for optimistic locking
benrj Oct 19, 2021
225ba12
Add support for optimistic locking with `put`
benrj Oct 20, 2021
0061c09
Refactor conditional lock expression
benrj Oct 20, 2021
9ac66f8
Cast DataModel as map
benrj Oct 20, 2021
c20ecd9
Remove unnecessary destructure properties
benrj Oct 20, 2021
1f513df
Type tweaks
benrj Oct 26, 2021
d40b46c
Bumping package version
benrj Oct 26, 2021
d14fbc4
Add readme
benrj Oct 26, 2021
332e251
Add further docs
benrj Oct 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 76 additions & 0 deletions src/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -559,6 +559,82 @@ test('#generateUpdateParams should generate both update and remove params for do
}
});

test('#generateUpdateParams should increment the version number', () => {
{
const options = {
tableName: 'blah2',
key: {
HashKey: 'abc',
},
data: {
a: 123,
b: 'abc',
c: undefined,
lockVersion: 1,
},
optimisticLockVersionAttribute: 'lockVersion',
};

expect(generateUpdateParams(options)).toEqual({
TableName: options.tableName,
Key: options.key,
ReturnValues: 'ALL_NEW',
ConditionExpression: '#lockVersion = :lockVersion',
UpdateExpression:
'add #lockVersion :lockVersionInc set #a0 = :a0, #a1 = :a1 remove #a2',
ExpressionAttributeNames: {
'#a0': 'a',
'#a1': 'b',
'#a2': 'c',
'#lockVersion': 'lockVersion',
},
ExpressionAttributeValues: {
':a0': options.data.a,
':a1': options.data.b,
':lockVersionInc': 1,
':lockVersion': 1,
},
});
}
});

test('#generateUpdateParams should increment the version number even when not supplied', () => {
{
const options = {
tableName: 'blah3',
key: {
HashKey: 'abc',
},
data: {
a: 123,
b: 'abc',
c: undefined,
},
optimisticLockVersionAttribute: 'lockVersion',
};

expect(generateUpdateParams(options)).toEqual({
TableName: options.tableName,
Key: options.key,
ReturnValues: 'ALL_NEW',
UpdateExpression:
'add #lockVersion :lockVersionInc set #a0 = :a0, #a1 = :a1 remove #a2',
ConditionExpression: 'attribute_not_exists(lockVersion)',
ExpressionAttributeNames: {
'#a0': 'a',
'#a1': 'b',
'#a2': 'c',
'#lockVersion': 'lockVersion',
},
ExpressionAttributeValues: {
':a0': options.data.a,
':a1': options.data.b,
':lockVersionInc': 1,
},
});
}
});

test(`#queryUntilLimitReached should call #query if "filterExpression" not provided`, async () => {
const keyConditionExpression = 'id = :id';
const attributeValues = { id: uuid() };
Expand Down
151 changes: 136 additions & 15 deletions src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,9 +120,57 @@ export interface ConditionalOptions {
attributeValues?: AttributeValues;
}

export type PutOptions = ConditionalOptions;
export type UpdateOptions = ConditionalOptions;
export type DeleteOptions = ConditionalOptions;
export interface SaveBehavior {
optimisticLockVersionAttribute?: string;
optimisticLockVersionIncrement?: number;
}

export interface MutateBehavior {
ignoreOptimisticLocking?: boolean;
}

export type PutOptions = ConditionalOptions & MutateBehavior;
export type UpdateOptions = ConditionalOptions & MutateBehavior;
export type DeleteOptions = ConditionalOptions & MutateBehavior;

export interface BuildOptimisticLockOptionsInput extends ConditionalOptions {
versionAttribute: string;
versionAttributeValue: any;
}

export function buildOptimisticLockOptions(
options: BuildOptimisticLockOptionsInput
): ConditionalOptions {
const { versionAttribute, versionAttributeValue } = options;
let { conditionExpression, attributeNames, attributeValues } = options;

const lockExpression = versionAttributeValue
? `#${versionAttribute} = :${versionAttribute}`
: `attribute_not_exists(${versionAttribute})`;

conditionExpression = conditionExpression
? `(${conditionExpression}) AND ${lockExpression}`
: lockExpression;

if (versionAttributeValue) {
attributeNames = {
...attributeNames,
[`#${versionAttribute}`]: versionAttribute,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

};
attributeValues = {
...attributeValues,
[`:${versionAttribute}`]: versionAttributeValue,
};
}

return {
conditionExpression,
attributeNames,
attributeValues,
};
}

type DataModelAsMap = { [key: string]: any };

export interface GenerateUpdateParamsInput extends UpdateOptions {
tableName: string;
Expand All @@ -131,9 +179,10 @@ export interface GenerateUpdateParamsInput extends UpdateOptions {
}

export function generateUpdateParams(
options: GenerateUpdateParamsInput
options: GenerateUpdateParamsInput & SaveBehavior
): DocumentClient.UpdateItemInput {
const setExpressions: string[] = [];
const addExpressions: string[] = [];
const removeExpressions: string[] = [];
const expressionAttributeNameMap: AttributeNames = {};
const expressionAttributeValueMap: AttributeValues = {};
Expand All @@ -142,15 +191,41 @@ export function generateUpdateParams(
tableName,
key,
data,
conditionExpression,
attributeNames,
attributeValues,
optimisticLockVersionAttribute: versionAttribute,
optimisticLockVersionIncrement: versionInc,
ignoreOptimisticLocking: ignoreLocking = false,
} = options;

let conditionExpression = options.conditionExpression;

if (versionAttribute) {
addExpressions.push(`#${versionAttribute} :${versionAttribute}Inc`);
expressionAttributeNameMap[`#${versionAttribute}`] = versionAttribute;
expressionAttributeValueMap[`:${versionAttribute}Inc`] = versionInc ?? 1;

if (!ignoreLocking) {
({ conditionExpression } = buildOptimisticLockOptions({
versionAttribute,
versionAttributeValue: (data as DataModelAsMap)[versionAttribute],
conditionExpression,
}));
expressionAttributeValueMap[`:${versionAttribute}`] = (
data as DataModelAsMap
)[versionAttribute];
}
}

const keys = Object.keys(options.data).sort();

for (let i = 0; i < keys.length; i++) {
const name = keys[i];
if (name === versionAttribute) {
// versionAttribute is a special case and should always be handled
// explicitly as above with the supplied value ignored
continue;
}

const valueName = `:a${i}`;
const attributeName = `#a${i}`;
Expand Down Expand Up @@ -178,11 +253,13 @@ export function generateUpdateParams(
? 'remove ' + removeExpressions.join(', ')
: undefined;

const addString =
addExpressions.length > 0 ? 'add ' + addExpressions.join(', ') : undefined;
return {
TableName: tableName,
Key: key,
ConditionExpression: conditionExpression,
UpdateExpression: [setString, removeString]
UpdateExpression: [addString, setString, removeString]
.filter((val) => val !== undefined)
.join(' '),
ExpressionAttributeNames: {
Expand All @@ -197,9 +274,10 @@ export function generateUpdateParams(
};
}

interface DynamoDbDaoInput {
interface DynamoDbDaoInput<T> {
tableName: string;
documentClient: DocumentClient;
optimisticLockingAttribute?: keyof NumberPropertiesInType<T>;
}

function invalidCursorError(cursor: string): Error {
Expand Down Expand Up @@ -261,10 +339,12 @@ export type NumberPropertiesInType<T> = Pick<
export default class DynamoDbDao<DataModel, KeySchema> {
public readonly tableName: string;
public readonly documentClient: DocumentClient;
public readonly optimisticLockingAttribute?: keyof NumberPropertiesInType<DataModel>;

constructor(options: DynamoDbDaoInput) {
constructor(options: DynamoDbDaoInput<DataModel>) {
this.tableName = options.tableName;
this.documentClient = options.documentClient;
this.optimisticLockingAttribute = options.optimisticLockingAttribute;
}

/**
Expand Down Expand Up @@ -292,16 +372,30 @@ export default class DynamoDbDao<DataModel, KeySchema> {
*/
async delete(
key: KeySchema,
options: DeleteOptions = {}
options: DeleteOptions = {},
data: Partial<DataModel> = {}
): Promise<DataModel | undefined> {
let { attributeNames, attributeValues, conditionExpression } = options;

if (this.optimisticLockingAttribute && !options.ignoreOptimisticLocking) {
const versionAttribute = this.optimisticLockingAttribute.toString();
({ attributeNames, attributeValues, conditionExpression } =
buildOptimisticLockOptions({
versionAttribute,
versionAttributeValue: (data as DataModelAsMap)[versionAttribute],
conditionExpression: conditionExpression,
attributeNames,
attributeValues,
}));
}
const { Attributes: attributes } = await this.documentClient
.delete({
TableName: this.tableName,
Key: key,
ReturnValues: 'ALL_OLD',
ConditionExpression: options.conditionExpression,
ExpressionAttributeNames: options.attributeNames,
ExpressionAttributeValues: options.attributeValues,
ConditionExpression: conditionExpression,
ExpressionAttributeNames: attributeNames,
ExpressionAttributeValues: attributeValues,
})
.promise();

Expand All @@ -312,13 +406,36 @@ export default class DynamoDbDao<DataModel, KeySchema> {
* Creates/Updates an item in the table
*/
async put(data: DataModel, options: PutOptions = {}): Promise<DataModel> {
let { conditionExpression, attributeNames, attributeValues } = options;
if (this.optimisticLockingAttribute) {
// Must cast data to avoid tripping the linter, otherwise, it'll complain
// about expression of type 'string' can't be used to index type 'unknown'
const dataAsMap = data as DataModelAsMap;
Copy link
Contributor

@austin-rausch austin-rausch Oct 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 🤮

const versionAttribute = this.optimisticLockingAttribute.toString();

if (!options.ignoreOptimisticLocking) {
({ conditionExpression, attributeNames, attributeValues } =
buildOptimisticLockOptions({
versionAttribute,
versionAttributeValue: dataAsMap[versionAttribute],
conditionExpression,
attributeNames,
attributeValues,
}));
}

dataAsMap[versionAttribute] = dataAsMap[versionAttribute]
? dataAsMap[versionAttribute] + 1
: 1;
}

await this.documentClient
.put({
TableName: this.tableName,
Item: data,
ConditionExpression: options.conditionExpression,
ExpressionAttributeNames: options.attributeNames,
ExpressionAttributeValues: options.attributeValues,
ConditionExpression: conditionExpression,
ExpressionAttributeNames: attributeNames,
ExpressionAttributeValues: attributeValues,
})
.promise();
return data;
Expand All @@ -337,6 +454,9 @@ export default class DynamoDbDao<DataModel, KeySchema> {
key,
data,
...updateOptions,
optimisticLockVersionAttribute: this.optimisticLockingAttribute
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(),
        }
      : {}),

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

? this.optimisticLockingAttribute.toString()
: undefined,
});
const { Attributes: attributes } = await this.documentClient
.update(params)
Expand Down Expand Up @@ -623,6 +743,7 @@ export default class DynamoDbDao<DataModel, KeySchema> {
.batchWrite({
RequestItems: {
[this.tableName]: operations.map((operation) => {
// TODO: optionally add the opt lock here
if (isBatchPutOperation(operation)) {
return {
PutRequest: {
Expand Down
Loading