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.

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

Check that different update operators do not target same field (conflict) #232

Closed
tatu-at-datastax opened this issue Mar 8, 2023 · 4 comments · Fixed by #267
Closed
Assignees

Comments

@tatu-at-datastax
Copy link
Contributor

tatu-at-datastax commented Mar 8, 2023

Although I have not seen this mentioned in general (but will see if I can find), update operators can NOT target same field in same operation. There is an explicit mention of "$set" and "$unset" not allowing this but it appears general limitation; for example trying:

db.arrays.findOneAndUpdate({ _id : 1 }, { "$max" : {"x":false }, "$min" : {"x":true} })

gives error

Updating the path 'x' would create a conflict at 'x'

possibly because order in which operators are executed does not appear to be strongly defined (unlike per-field ordering that is).

We should similarly cross-check paths across different operators (within operators JSON does not allow duplicates) and fail update upon found conflict.

@vkarpov15
Copy link
Collaborator

An additional caveat is that an update operator cannot target subpaths of a path that another update operator is targetting. For example, the following throws a "Updating the path 'coordinates.lat' would create a conflict at 'coordinates'" error.

> db.test.insertOne({ coordinates: { lat: 37, lng: -132 } })
> db.test.updateOne({}, { $set: { coordinates: { lat: 37.14, lng: -132.33 } }, $mul: { 'coordinates.lat': 1.1 } })

@tatu-at-datastax
Copy link
Contributor Author

tatu-at-datastax commented Mar 13, 2023

@vkarpov15 yes, I was worried this comes up. Straight up comparison is easier than prefixes. :)

But... well, I'll see whether it makes sense to first cover simple(r) case & then do follow-up, or if I just want to tackle the full case right on. Implementation-wise latter can be done, I think, by sorting the path & comparing adjacent paths to see if first one is a parent-prefix of the second one (suffix starting with .). So not necessarily much more complicated, but a bit.

@vkarpov15
Copy link
Collaborator

What we often end up doing in Mongoose is creating a Set of all the updated paths and their parents. So something like:

const updatedPaths = new Set();
for (const key of Object.keys(update)) {
  if (key.indexOf('.') === -1) {
    updatedPaths.add(key);
    continue;
  }
  const pieces = key.split('.');
  let cur = pieces[0];
  updatedPaths.add(cur);
  for (let i = 1; i < pieces.length; ++i) {
    cur += '.' + pieces[i];
    updatedPaths.add(cur);
  }
}

Add a check for duplicates and you're done. Should work in this case, unless you think it is too slow.

@tatu-at-datastax
Copy link
Contributor Author

Thanks @vkarpov15! Something along these lines would work, yes. Will tackle this after doing some refactoring to unify path handling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants