Skip to content

fix(hadron-document): fix updating arrays with dots in names COMPASS-6011 #3388

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

Merged
merged 1 commit into from
Aug 25, 2022
Merged

fix(hadron-document): fix updating arrays with dots in names COMPASS-6011 #3388

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
166 changes: 120 additions & 46 deletions packages/hadron-document/src/object-generator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,25 @@ function maybeDecorateWithDecryptedKeys(
}

/** Used to represent missing values, i.e. non-existent fields. */
const DoesNotExist = Symbol('DidNotExist');
const DoesNotExist = Symbol('DoesNotExist');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Purely cosmestic, I think I just forgot to change the name while working on #3239.


/**
* Describe a single property of a document. For us, not only the
* field name, but also whether it is an array index or a regular
* document property is relevant.
*/
type SubfieldDescription = {
key: string;
isArrayIndex: boolean;
};

/**
* Describe a field in a document, with its path and current value.
* For example, in the document `{ a: { b: 42 } }`, the nested property
* `b` of `a` would be described by `{ path: ['a', 'b'], value: 42 }`.
*/
type FieldDescription = {
path: string[];
path: SubfieldDescription[];
value: BSONValue | typeof DoesNotExist;
};

Expand Down Expand Up @@ -212,6 +222,7 @@ export class ObjectGenerator {
keyInclusionOptions.includableEncryptedKeys ?? [];

for (const element of target.elements ?? []) {
const isArrayIndex = target.currentType === 'Array';
// Do not include encrypted fields in the `originalFields` list
// unless we know that it is okay to include them (i.e. because
// we know that we can perform equality queries on those fields).
Expand Down Expand Up @@ -274,13 +285,16 @@ export class ObjectGenerator {
);
for (const { path, value } of nestedResult.originalFields) {
originalFields.push({
path: [String(element.key), ...path],
path: [{ key: String(element.key), isArrayIndex }, ...path],
value,
});
}
for (const { path, value } of nestedResult.newFields) {
newFields.push({
path: [String(element.currentKey), ...path],
path: [
{ key: String(element.currentKey), isArrayIndex },
...path,
],
value,
});
}
Expand All @@ -289,7 +303,7 @@ export class ObjectGenerator {
// the original field's value.
if (canIncludeOriginalValue) {
originalFields.push({
path: [String(element.key)],
path: [{ key: String(element.key), isArrayIndex }],
value: element.generateOriginalObject(),
});
}
Expand All @@ -300,7 +314,7 @@ export class ObjectGenerator {
!element.isRemoved()
) {
newFields.push({
path: [String(element.currentKey)],
path: [{ key: String(element.currentKey), isArrayIndex }],
value: element.generateObject(),
});
}
Expand All @@ -313,14 +327,29 @@ export class ObjectGenerator {
(element.isAdded() || element.hasChangedKey()) &&
element.currentKey !== ''
) {
// When a new field is added, check if that field
// was already added in the background.
originalFields.push({
path: [String(element.currentKey)],
value: DoesNotExist,
});
// When a new field is added, check if the original value of that
// field (which is typically that it was missing) was changed in
// the background. If there *was* another field in its place,
// that means that it was removed, and is added to `originalValue`
// at another point.
let wasRenamedToKeyOfPreviouslyExistingElement = false;
for (const otherElement of target.elements ?? []) {
if (
otherElement !== element &&
otherElement.key === element.currentKey
) {
wasRenamedToKeyOfPreviouslyExistingElement = true;
break;
}
}
if (!wasRenamedToKeyOfPreviouslyExistingElement) {
originalFields.push({
path: [{ key: String(element.currentKey), isArrayIndex }],
value: DoesNotExist,
});
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is kind of an independent issue, but didn’t appear in tests so far because when building the query in the non-dots-and-dollars mode, duplicate entries in originalFields would not cause trouble. For example, if property a with value 42 was removed and then later another element renamed to a, originalFields would contain two entries for a, one claiming that it does not exist, one claiming that it had the value 42; the query would still (accidentally correctly) end up being { a: 42 }, since the property in the query doc was simply overwritten.

In dots-and-dollars mode, however, the different values in originalFields are turned into multiple $eq checks which are then $and-ed together, so obviously there would be a conflict between an $eq asserting that the field did not exist and an $eq asserting that the field had its previous value.

newFields.push({
path: [String(element.currentKey)],
path: [{ key: String(element.currentKey), isArrayIndex }],
value: element.generateObject(),
});
}
Expand All @@ -334,11 +363,27 @@ export class ObjectGenerator {
// Remove the original field when an element is removed or renamed.
if (canIncludeOriginalValue) {
originalFields.push({
path: [String(element.key)],
path: [{ key: String(element.key), isArrayIndex }],
value: element.generateOriginalObject(),
});
}
newFields.push({ path: [String(element.key)], value: DoesNotExist });

let wasRemovedAndLaterReplacedByNewElement = false;
for (const otherElement of target.elements ?? []) {
if (
otherElement !== element &&
otherElement.currentKey === element.key
) {
wasRemovedAndLaterReplacedByNewElement = true;
break;
}
}
if (!wasRemovedAndLaterReplacedByNewElement) {
newFields.push({
path: [{ key: String(element.key), isArrayIndex }],
value: DoesNotExist,
});
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(This just mirrors the change above.)

}
}

Expand Down Expand Up @@ -367,34 +412,62 @@ export class ObjectGenerator {

// Return a $getField expression that evaluates to the current value
// of the document at `path`.
private static createGetFieldExpr(path: string[]): BSONObject {
private static createGetFieldExpr(path: SubfieldDescription[]): BSONObject {
return path.reduce(
(input, key) => ({
$getField: {
field: { $literal: key },
input,
},
}),
(input, { key, isArrayIndex }) =>
isArrayIndex
? {
$arrayElemAt: [input, +key],
}
: {
$getField: {
field: { $literal: key },
input,
},
},
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In retrospect, it feels a bit dumb to have assumed (or probably just not thought about) that $getField and $setField don’t work on arrays the way they do on objects.

'$$ROOT' as any
);
}

// Return a $setField expression that writes the specified value
// to the document at `path`.
private static createSetFieldExpr(
path: string[],
path: SubfieldDescription[],
value: BSONValue | typeof DoesNotExist
): BSONValue {
return path.reduceRight(
(value, key, idx, array) => ({
$setField: {
field: { $literal: key },
input: ObjectGenerator.createGetFieldExpr(array.slice(0, idx)),
value,
return path.reduceRight((value, { key, isArrayIndex }, idx, array) => {
const input = ObjectGenerator.createGetFieldExpr(array.slice(0, idx));
if (!isArrayIndex) {
// 'Simple' case: Change a property of a document
return {
$setField: {
field: { $literal: key },
input,
value,
},
};
}

// Array case: concatenate the prefix of the array before the changed
// index, an array containing the new value at the changed index,
// and the suffix afterwards; use $let to avoid specifying the full
// input value expression multiple times.
return {
$let: {
vars: { input },
in: {
$concatArrays: [
// The third argument to $slice must not be 0
...(+key > 0 ? [{ $slice: ['$$input', 0, +key] }] : []),
[value],
// The third argument is required; 2^31-1 is the maximum
// accepted value, and well beyond what BSON can represent.
{ $slice: ['$$input', +key + 1, 2 ** 31 - 1] },
],
},
},
}),
(value === DoesNotExist ? '$$REMOVE' : { $literal: value }) as any
);
};
}, (value === DoesNotExist ? '$$REMOVE' : { $literal: value }) as any);
}

/**
Expand Down Expand Up @@ -436,28 +509,29 @@ export class ObjectGenerator {
const query: any = {};
if (
originalFields.some(({ path }) =>
path.some((key) => key.includes('.') || key.startsWith('$'))
path.some(({ key }) => key.includes('.') || key.startsWith('$'))
)
) {
// Some of the keys in this query are only accesible via $getField,
// which was introduced in MongoDB 5.0.
const equalityMatches: any[] = [];
for (const { path, value } of originalFields) {
const getFieldExpr = ObjectGenerator.createGetFieldExpr(path);
const matchExpr =
equalityMatches.push(
value !== DoesNotExist
? {
$eq: [getFieldExpr, { $literal: value }],
}
: {
$eq: [{ $type: getFieldExpr }, 'missing'],
};
query.$expr ??= { $and: [] };
query.$expr.$and.push(matchExpr);
? { $eq: [getFieldExpr, { $literal: value }] }
: { $eq: [{ $type: getFieldExpr }, 'missing'] }
);
}
if (equalityMatches.length === 1) {
query.$expr = equalityMatches[0];
} else if (equalityMatches.length > 1) {
query.$expr = { $and: equalityMatches };
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This part of the change is also a little independent, and only makes it so that $and: [singleEntry] is reduced to singleEntry, with the main purpose being making the test files a little more readable

}
} else {
for (const { path, value } of originalFields) {
const matchValue = value === DoesNotExist ? { $exists: false } : value;
query[path.join('.')] = matchValue;
query[path.map(({ key }) => key).join('.')] = matchValue;
}
}
return query;
Expand All @@ -483,7 +557,7 @@ export class ObjectGenerator {

if (
newFields.some(({ path }) =>
path.some((key) => key.includes('.') || key.startsWith('$'))
path.some(({ key }) => key.includes('.') || key.startsWith('$'))
)
) {
// Some of the keys in this query are only writable via $setField/$unsetField,
Expand All @@ -498,10 +572,10 @@ export class ObjectGenerator {
for (const { path, value } of newFields) {
if (value === DoesNotExist) {
updateDoc.$unset ??= {};
updateDoc.$unset[path.join('.')] = true;
updateDoc.$unset[path.map(({ key }) => key).join('.')] = true;
} else {
updateDoc.$set ??= {};
updateDoc.$set[path.join('.')] = value;
updateDoc.$set[path.map(({ key }) => key).join('.')] = value;
}
}
return updateDoc;
Expand Down
Loading