fix(hadron-document): fix updating arrays with dots in names COMPASS-6011 #3388
Conversation
…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).
|
|
||
| /** Used to represent missing values, i.e. non-existent fields. */ | ||
| const DoesNotExist = Symbol('DidNotExist'); | ||
| const DoesNotExist = Symbol('DoesNotExist'); |
There was a problem hiding this comment.
Purely cosmestic, I think I just forgot to change the name while working on #3239.
| path: [{ key: String(element.key), isArrayIndex }], | ||
| value: DoesNotExist, | ||
| }); | ||
| } |
There was a problem hiding this comment.
(This just mirrors the change above.)
| path: [{ key: String(element.currentKey), isArrayIndex }], | ||
| value: DoesNotExist, | ||
| }); | ||
| } |
There was a problem hiding this comment.
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.
| field: { $literal: key }, | ||
| input, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
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.
| if (equalityMatches.length === 1) { | ||
| query.$expr = equalityMatches[0]; | ||
| } else if (equalityMatches.length > 1) { | ||
| query.$expr = { $and: equalityMatches }; |
There was a problem hiding this comment.
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
| expect(doc.generateUpdateUnlessChangedInBackgroundQuery()).to.deep.equal({ | ||
| query: { _id: null, 'a.3': { $exists: false } }, | ||
| updateDoc: { $set: { 'a.3': new Int32(4) } }, | ||
| context('with a plain array name', function () { |
There was a problem hiding this comment.
These tests remain unchanged, only split into two context()s, one for plain names and one for dots-and-dollar names
| }, | ||
| }, | ||
| ], | ||
| }); |
There was a problem hiding this comment.
I don’t necessarily mind having these fairly verbose/repetitive test object assertions, but if we feel that this is too noisy, I’m happy to take suggestions on how to improve that. I guess ideally some kind of snapshot testing where it’s easier to do bulk updates to expected results would be a good pick, but I don’t want to introduce entirely new paradigms here.
2 participants
Diff probably best viewed with whitespace changes hidden.
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
$getFieldand$setFieldwhich can work onarrays (
$arrayElemAtfor the former,$concatArrays+$sliceforthe latter).
Description
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes