Skip to content

Conversation

addaleax
Copy link
Collaborator

  • Instead of fully replacing top-level properties only when editing
    documents, set only the individual nested values that have been
    edited.
    • This does not apply to array item removals, since there is no
      way to perform this (at least in MongoDB 4.0, but doing so
      on newer versions is also not straightforward).
    • Use $getField and $setField to modify fields whose names
      contain dots or start with a dollar sign.
      This is kind of a breaking change in that, previously, we
      would have queried for the field names containing dots,
      the server would have interpreted the field name as referring
      to nested documents, almost never found a document matching that
      (unless the doc happened to have a shape like
      { x: { y: 2 }, 'x.y': 2 } with matching values), and we then
      prompted the user to force-update due to the assumption that
      no document having been found means that it has been updated
      in the background.
      After this change, we just inform users that doing this
      properly requires MongoDB 5.0 or above. Users can still perform
      edits through the JSON view, at least for now (potentially only
      until COMPASS-5753 is done).
  • Split the query/update document generation parts into two steps,
    and move these out of the Document class to ObjectGenerator
    (which is already a collection of static methods for recursively
    traversing a document/element tree).
    • In the first step, gather all the elements that were updated,
      their original paths and values and their new paths and values.
    • In the second step, build either a query out of those original
      paths and values, or build the update document out of the new
      paths and values.
  • Drive-by bugfixes:
    • When renaming elements, we previously did not check that the
      new name was not already added in the background. We check
      this as well now.
    • Handle the case in which elements are renamed circularly,
      e.g. key2 → key3, key1 → key2 (or similar add/remove situations).
  • Use string arrays instead of objects that are only being used
    to extract their keys in the hadron-document API for hopefully
    a bit more API clarity.
    • Clarify in the docs that dots inside field names refer to
      nested documents. This is the way that sharding behaves,
      which is why we use them in the first place, and is not
      a practical API to break. (And if it does, there is an
      upcoming server sharding project will hopefully already have
      made inclusion of the shard keys unnecessary.)

This also addresses COMPASS-5528 (which is about adjusting the
update document sent to the server, whereas COMPASS-5805 is about
adjusting the query document).

Description

Checklist

  • New tests and/or benchmarks are included
  • Documentation is changed or added

Motivation and Context

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Open Questions

Dependents

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

…OMPASS-5805

- Instead of fully replacing top-level properties only when editing
  documents, set only the individual nested values that have been
  edited.
  - This does not apply to array item removals, since there is no
    way to perform this (at least in MongoDB 4.0, but doing so
    on newer versions is also not straightforward).
  - Use `$getField` and `$setField` to modify fields whose names
    contain dots or start with a dollar sign.
    This is kind of a breaking change in that, previously, we
    would have queried for the field names containing dots,
    the server would have interpreted the field name as referring
    to nested documents, almost never found a document matching that
    (unless the doc happened to have a shape like
    `{ x: { y: 2 }, 'x.y': 2 }` with matching values), and we then
    prompted the user to force-update due to the assumption that
    no document having been found means that it has been updated
    in the background.
    After this change, we just inform users that doing this
    properly requires MongoDB 5.0 or above. Users can still perform
    edits through the JSON view, at least for now (potentially only
    until COMPASS-5753 is done).
- Split the query/update document generation parts into two steps,
  and move these out of the `Document` class to `ObjectGenerator`
  (which is already a collection of static methods for recursively
  traversing a document/element tree).
  - In the first step, gather all the elements that were updated,
    their original paths and values and their new paths and values.
  - In the second step, build either a query out of those original
    paths and values, or build the update document out of the new
    paths and values.
- Drive-by bugfixes:
  - When renaming elements, we previously did not check that the
    new name was not already added in the background. We check
    this as well now.
  - Handle the case in which elements are renamed circularly,
    e.g. key2 → key3, key1 → key2 (or similar add/remove situations).
- Use string arrays instead of objects that are only being used
  to extract their keys in the hadron-document API for hopefully
  a bit more API clarity.
  - Clarify in the docs that dots inside field names refer to
    nested documents. This is the way that sharding behaves,
    which is why we use them in the first place, and is not
    a practical API to break. (And if it does, there is an
    upcoming server sharding project will hopefully already have
    made inclusion of the shard keys unnecessary.)

This also addresses COMPASS-5528 (which is about adjusting the
update document sent to the server, whereas COMPASS-5805 is about
adjusting the query document).
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.

❤️

Copy link
Collaborator

@gribnoysup gribnoysup left a comment

Choose a reason for hiding this comment

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

LGTM! Learned a ton about how v5 handles dots and dollars fields updates 😊

): {
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)

@addaleax addaleax merged commit c865b66 into main Jul 13, 2022
@addaleax addaleax deleted the 5805-dev branch July 13, 2022 09:29
addaleax added a commit that referenced this pull request Aug 25, 2022
…6011

Follow-up to c865b66 (#3239), which introduced this bug:
Handle updates to arrays in the case in which the updated array
(or another updated path) contains dots or starts with a dollar sign,
using alternatives to `$getField` and `$setField` which can work on
arrays (`$arrayElemAt` for the former, `$concatArrays` + `$slice` for
the latter).
addaleax added a commit that referenced this pull request Aug 25, 2022
…6011

Follow-up to c865b66 (#3239), which introduced this bug:
Handle updates to arrays in the case in which the updated array
(or another updated path) contains dots or starts with a dollar sign,
using alternatives to `$getField` and `$setField` which can work on
arrays (`$arrayElemAt` for the former, `$concatArrays` + `$slice` for
the latter).
addaleax added a commit that referenced this pull request Aug 25, 2022
…6011 (#3388)

Follow-up to c865b66 (#3239), which introduced this bug:
Handle updates to arrays in the case in which the updated array
(or another updated path) contains dots or starts with a dollar sign,
using alternatives to `$getField` and `$setField` which can work on
arrays (`$arrayElemAt` for the former, `$concatArrays` + `$slice` for
the latter).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants