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 all 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
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,9 @@ and this project adheres to
### Added

- Support `consistentRead` option on `get` API

## 1.5.0 - 2021-10-27

### Added

- Support optimistic locking for `put`, `update` and `delete` APIs
37 changes: 37 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,43 @@ const { total } = await myDocumentDao.decr(
);
```

**Optimistic Locking with Version Numbers**
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


For callers who wish to enable an optimistic locking strategy there are two
available toggles:

1. Provide the attribute you wish to be used to store the version number. This
will enable optimistic locking on the following operations: `put`, `update`,
and `delete`.

Writes for documents that do not have a version number attribute will
initialize the version number to 1. All subsequent writes will need to
provide the current version number. If an out-of-date version number is
supplied, an error will be thrown.

Example of Dao constructed with optimistic locking enabled.

```
const dao = new DynamoDbDao<DataModel, KeySchema>({
tableName,
documentClient,
optimisticLockingAttribute: 'version',
});
```

2. If you wish to ignore optimistic locking for a save operation, specify
`ignoreOptimisticLocking: true` in the options on your `put`, `update`, or
`delete`.

NOTE: Optimistic locking is NOT supported for `batchWrite` or `batchPut`
operations. Consuming those APIs for data models that do have optimistic locking
enabled may clobber your version data and could produce undesirable effects for
other callers.

This was modeled after the
[Java Dynamo client](https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/DynamoDBMapper.OptimisticLocking.html)
implementation.

## Developing

The test setup requires that [docker-compose]() be installed. To run the tests,
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@jupiterone/dynamodb-dao",
"version": "1.4.0",
"version": "1.5.0",
"description": "DynamoDB Data Access Object (DAO) helper library",
"main": "index.js",
"types": "index.d.ts",
Expand Down
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
149 changes: 134 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 {
export 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,8 @@ export default class DynamoDbDao<DataModel, KeySchema> {
key,
data,
...updateOptions,
optimisticLockVersionAttribute:
this.optimisticLockingAttribute?.toString(),
});
const { Attributes: attributes } = await this.documentClient
.update(params)
Expand Down
Loading