Skip to content

feat(hadron-document)!: handle nested fields and dots & dollars well COMPASS-5805 #3239

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 2 commits into from
Jul 13, 2022
Merged

feat(hadron-document)!: handle nested fields and dots & dollars well COMPASS-5805 #3239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
029c55f
feat(hadron-document)!: handle nested fields and dots & dollars well …
addaleax Jun 10, 2022
3046933
Merge remote-tracking branch 'origin/main' into 5805-dev
addaleax Jul 12, 2022
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
22 changes: 16 additions & 6 deletions packages/compass-crud/src/stores/crud-store.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ import {

import configureGridStore from './grid-store';

const { log, mongoLogId, track } = createLoggerAndTelemetry('COMPASS-CRUD-UI');
const { debug, log, mongoLogId, track } =
createLoggerAndTelemetry('COMPASS-CRUD-UI');

function pickQueryProps({
filter,
Expand Down Expand Up @@ -455,8 +456,9 @@ const configureStore = (options = {}) => {
// required for updated documents in sharded collections.
const { query, updateDoc } =
doc.generateUpdateUnlessChangedInBackgroundQuery(
this.state.shardKeys
Object.keys(this.state.shardKeys)
);
debug('Performing findOneAndUpdate', { query, updateDoc });

if (Object.keys(updateDoc).length === 0) {
doc.emit('update-error', EMPTY_UPDATE_ERROR.message);
Expand All @@ -476,6 +478,13 @@ const configureStore = (options = {}) => {
);

if (error) {
if (
error.codeName === 'InvalidPipelineOperator' &&
error.message.match(/\$[gs]etField/)
) {
const nbsp = '\u00a0';
Copy link
Collaborator

Choose a reason for hiding this comment

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

❤️

error.message += ` (Updating fields whose names contain dots or start with $ require MongoDB${nbsp}5.0 or above.)`;
}
doc.emit('update-error', error.message);
} else if (d) {
doc.emit('update-success', d);
Expand Down Expand Up @@ -505,10 +514,11 @@ const configureStore = (options = {}) => {
try {
doc.emit('update-start');
const object = doc.generateObject();
const query = doc.getOriginalKeysAndValuesForSpecifiedKeys({
_id: 1,
...(this.state.shardKeys || {}),
});
const query = doc.getQueryForOriginalKeysAndValuesForSpecifiedKeys([
'_id',
...Object.keys(this.state.shardKeys || {}),
]);
debug('Performing findOneAndReplace', { query, object });

if (!(await this._verifyUpdateAllowed(this.state.ns, doc))) {
// _verifyUpdateAllowed emitted update-error
Expand Down
147 changes: 25 additions & 122 deletions packages/hadron-document/src/document.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import EventEmitter from 'eventemitter3';
import { EJSON, UUID } from 'bson';
import type { ObjectGeneratorOptions } from './object-generator';
import ObjectGenerator from './object-generator';
import type { BSONObject, BSONValue } from './utils';
import type { BSONArray, BSONObject, BSONValue } from './utils';
import { objectToIdiomaticEJSON } from './utils';
import type { HadronEJSONOptions } from './utils';

Expand Down Expand Up @@ -109,40 +109,40 @@ export class Document extends EventEmitter {
* where the update only succeeds when the changed document's elements have
* not been changed in the background.
*
* `query` and `updateDoc` may use $getField and $setField if field names
* contain either `.` or start with `$`. These operators are only available
* on MongoDB 5.0+. (Note that field names starting with `$` are also only
* allowed in MongoDB 5.0+.)
*
* @param {Object} alwaysIncludeKeys - An object whose keys are used as keys
* that are always included in the generated query.
* that are always included in the generated query. Dots inside key names
* are interpreted as referring to nested properties.
*
* @returns {Object} An object containing the `query` and `updateDoc` to be
* used in an update operation.
*/
generateUpdateUnlessChangedInBackgroundQuery(
alwaysIncludeKeys: BSONObject | null = null
alwaysIncludeKeys: string[] = []
): {
query: BSONObject;
updateDoc: { $set?: BSONObject; $unset?: BSONObject };
updateDoc: { $set?: BSONObject; $unset?: BSONObject } | BSONArray;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Totally can see that this PR is marked as a breaking change with a !, but still wanted to highlight that cloud seems to have very hard expectations for this to return set / unset object here. Just wondering if we checked with them whether they would be able to handle this new format with their backed at all, maybe we should create a downstream ticket for them to look into this 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oof, thanks for bringing this up! Pinged them on Slack (as you might have seen)

} {
// Build a query that will find the document to update only if it has the
// values of elements that were changed with their original value.
// This query won't find the document if an updated element's value isn't
// the same value as it was when it was originally loaded.
const originalFieldsThatWillBeUpdated =
this.getOriginalKeysAndValuesForFieldsThatWereUpdated(alwaysIncludeKeys);
ObjectGenerator.getQueryForOriginalKeysAndValuesForSpecifiedFields(
this,
alwaysIncludeKeys,
true
);
const query = {
_id: this.getId(),
...originalFieldsThatWillBeUpdated,
};

// Build the update document to be used in an update operation with `$set`
// and `$unset` reflecting the changes that have occured in the document.
const setUpdateObject = this.getSetUpdateForDocumentChanges();
const unsetUpdateObject = this.getUnsetUpdateForDocumentChanges();
const updateDoc: { $set?: BSONObject; $unset?: BSONObject } = {};
if (setUpdateObject && Object.keys(setUpdateObject).length > 0) {
updateDoc.$set = setUpdateObject;
}
if (unsetUpdateObject && Object.keys(unsetUpdateObject).length > 0) {
updateDoc.$unset = unsetUpdateObject;
}
const updateDoc = ObjectGenerator.generateUpdateDoc(this);

return {
query,
Expand Down Expand Up @@ -197,98 +197,24 @@ export class Document extends EventEmitter {
return element ? element.generateObject() : null;
}

/**
* Generate the query javascript object reflecting the elements that
* were updated in this document. The values of this object are the original
* values, this can be used when querying for an update to see if the original
* document was changed in the background while it was being updated elsewhere.
*
* @param {Object} alwaysIncludeKeys - An object whose keys are used as keys
* that are always included in the generated query.
*
* @returns {Object} The javascript object.
*/
getOriginalKeysAndValuesForFieldsThatWereUpdated(
alwaysIncludeKeys: BSONObject | null = null
): BSONObject {
const object: BSONObject = {};

if (this.elements) {
for (const element of this.elements) {
if (
(element.isModified() && !element.isAdded()) ||
(alwaysIncludeKeys && element.key in alwaysIncludeKeys)
) {
// Using `.key` instead of `.currentKey` to ensure we look at
// the original field's value.
object[element.key] = element.generateOriginalObject();
}
if (element.isAdded() && element.currentKey !== '') {
// When a new field is added, check if that field
// was already added in the background.
object[element.currentKey] = { $exists: false };
}
}
}

return object;
}

/**
* Generate the query javascript object reflecting the elements that
* are specified by the keys listed in `keys`. The values of this object are
* the original values, this can be used when querying for an update based
* on multiple criteria.
*
* @param {Object} keys - An object whose keys are used as keys
* that are included in the generated query.
* @param keys - An array whose entries are used as keys
* that are included in the generated query. Dots inside key names
* are interpreted as referring to nested properties.
*
* @returns {Object} The javascript object.
*/
getOriginalKeysAndValuesForSpecifiedKeys(keys: BSONObject): BSONObject {
const object: BSONObject = {};

if (this.elements) {
for (const element of this.elements) {
if (element.key in keys) {
// Using `.key` instead of `.currentKey` to ensure we look at
// the original field's value.
object[element.key] = element.generateOriginalObject();
}
}
}

return object;
}

/**
* Generate an $set javascript object, that can be used in update operations to
* set the changes which have occured in the document since it was loaded.
*
* @returns {Object} The javascript update object.
**/
getSetUpdateForDocumentChanges(): BSONObject {
const object: BSONObject = {};

if (this.elements) {
for (const element of this.elements) {
if (
!element.isRemoved() &&
element.currentKey !== '' &&
element.isModified()
) {
// Include the full modified element.
// We don't individually set nested fields because we can't guarantee a
// path to the element using '.' dot notation will update
// the correct field, because field names can contain dots as of 3.6.
// When a nested field has been altered (changed/added/removed) it is
// set at the top level field. This means we overwrite possible
// background changes that occur within sub documents.
object[element.currentKey] = element.generateObject();
}
}
}
return object;
getQueryForOriginalKeysAndValuesForSpecifiedKeys(keys: string[]): BSONObject {
return ObjectGenerator.getQueryForOriginalKeysAndValuesForSpecifiedFields(
this,
keys,
false
);
}

/**
Expand All @@ -309,29 +235,6 @@ export class Document extends EventEmitter {
return String(element.value);
}

/**
* Generate an $unset javascript object, that can be used in update
* operations, with the removals from the document.
*
* @returns {Object} The javascript update object.
**/
getUnsetUpdateForDocumentChanges(): BSONObject {
const object: BSONObject = {};

if (this.elements) {
for (const element of this.elements) {
if (!element.isAdded() && element.isRemoved() && element.key !== '') {
object[element.key] = true;
}
if (!element.isAdded() && element.isRenamed() && element.key !== '') {
// Remove the original field when an element is renamed.
object[element.key] = true;
}
}
}
return object;
}

/**
* Insert a placeholder element at the end of the document.
*
Expand Down
32 changes: 29 additions & 3 deletions packages/hadron-document/src/element.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -452,9 +452,9 @@ export class Element extends EventEmitter {
}

/**
* Determine if the element is renamed.
* Determine if the element was explicitly renamed by the user.
*
* @returns If the element was renamed.
* @returns If the element was explicitly renamed by the user.
*/
isRenamed(): boolean {
if (
Expand All @@ -468,7 +468,17 @@ export class Element extends EventEmitter {
}

/**
* Can changes to the elemnt be reverted?
* Determine if the element was renamed, potentially as part
* of moving array elements.
*
* @returns If the element was renamed, explicitly or implicitly.
*/
hasChangedKey(): boolean {
return this.key !== this.currentKey;
}

/**
* Can changes to the element be reverted?
*
* @returns If the element can be reverted.
*/
Expand Down Expand Up @@ -631,6 +641,22 @@ export class Element extends EventEmitter {
return this.removed;
}

/**
* Are any immediate children of this element flagged for removal?
*
* @returns If any immediate children of this element are flagged for removal.
*/
hasAnyRemovedChild(): boolean {
if (this.elements) {
for (const element of this.elements) {
if (element.isRemoved()) {
return true;
}
}
}
return false;
}

/**
* Elements themselves are not the root.
*
Expand Down
Loading