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.

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

Jump to bottom

Failing update operations with encrypted subdocument array on Mongoose 6 #105

Open
yelworc opened this issue Nov 23, 2021 · 6 comments · May be fixed by #119
Open

Failing update operations with encrypted subdocument array on Mongoose 6 #105

yelworc opened this issue Nov 23, 2021 · 6 comments · May be fixed by #119
Labels
bug help wanted

Comments

@yelworc
Copy link
Contributor

yelworc commented Nov 23, 2021 •
edited
Loading

I'm getting errors like this one when trying to update encrypted documents after upgrading to Mongoose 6:

MongoServerError: Updating the path 'subdocs' would create a conflict at 'subdocs'

The schema/model in question has encryption enabled for an entire field that contains an array of subdocuments, i.e.
schema.plugin(encrypt, { /* keys… */, encryptedFields: ['subdocs'] });
This may not have been a good idea, but here we are 😄 (it's been in production like that for a long time).

I created a unit test that reproduces the problem. With Mongoose 5, the update operation results in this query:

Mongoose: parents.updateOne(
  { _id: ObjectId("6197884a403d8c659a0c1437") },
  {
    '$set': { _ac: 'BinData(0, "…")', _ct: 'BinData(0, "…")', _id: ObjectId("6197884a403d8c659a0c1437") },
    '$unset': { children: 1 }
  },
  { session: null }
)

whereas with Mongoose 6, it's:

Mongoose: parents.updateOne(
  { _id: new ObjectId("6197883f877393d38902437d"), __v: 0 },
  {
    '$unset': { 'children.0.text': 1, children: 1 },
    '$set': { _id: new ObjectId("6197883f877393d38902437d"), _ct: 'BinData(0, "…")', _ac: 'BinData(0, "…")' }},
  { session: null }
)

Most likely, the children.0.text in the $unset part of the query is the issue. That's about as far as I got – not sure where to go from here…
@spencersteers
Copy link

So I was able to get around this by calling unmarkModified on each modified path and then calling save:

doc.children[0].text = 'New unencrypted text';
doc.directModifiedPaths().forEach((path) => doc.unmarkModified(path));
doc.save();

I believe this is because mongoose v6 now uses proxies for arrays and uses them to track modifications.

@yelworc
Copy link
Contributor Author

yelworc commented Nov 25, 2021

@spencersteers cool, thanks! That workaround seems to help in my case, too.
Ideally, mongoose-encryption would take care of this internally… but I'm not sure whether that's even possible.

@joegoldbeck
Copy link
Owner

joegoldbeck commented Jan 10, 2022 •
edited
Loading

Thanks @yelworc for reporting this and for the excellent unit test! I agree that this should be taken care of within the plugin.

The unmarkModified trick seems a little risky to me in that there may be other side-effects – presumably mongoose (or other mongoose plugins?) use the modified path markers to do something..., right? At minimum (for any casual readers - you may have already done this), I'd suggest being specific about which fields to unmarkModified rather than simple doing it for all fields.

As to fixing it in the plugin, I do agree that should be done. I might not get to it for a bit since I'll first need to get up to date on whatever Mongoose is doing with its subdocuments these days and am not using it day-to-day anymore.

Definitely welcome any PRs if folks with a bit more familiarity want to take a stab. If you branch from @yelworc's excellent unit test then the CI pipeline will take care of testing across both Mongoose version 5 & 6, and also crediting appropriately.

Ideally there can be a simple fix, that just updates the handling of subdocs to match whatever Mongoose now expects, but maintains backward compatibility with Mongoose v5.

It does seem like unmarking modified could help, but again I'm not sure the repercussions, and importantly don't want to break scenarios in which a user might be using this alongside another mongoose plugin, or their own middleware, which certainly could take advantage of that. Maybe if we did it as the very last step before encrypting that would be safe...?

Welcome any thoughts on this as well!

@offaxis
Copy link

offaxis commented Feb 11, 2022

Any progress for this thread ? MongoDb Atlas will update to MongoDB 5 (witch is only compatible with Mongoose v6 => https://mongoosejs.com/docs/compatibility.html) on shared cluster next week...

yelworc pushed a commit to yelworc/mongoose-encryption that referenced this issue Aug 12, 2022
Fix errors with encrypted subdoc arrays on Mongoose v6
6400629 
As of Mongoose v6, encrypted subdocument arrays lead to the following
error when updating an existing document:

> MongoServerError: Updating the path '…' would create a conflict at '…'

This change circumvents the problem by marking the corresponding schema
paths as unmodified (which should be safe, since those encrypted fields
are not meant to be stored anyway).

Fixes joegoldbeck#105
@yelworc yelworc linked a pull request Aug 12, 2022 that will close this issue
Open
@yelworc
Copy link
Contributor Author

yelworc commented Aug 12, 2022

Tried my luck at a fix (see #119). Thanks for your feedback, @joegoldbeck – I share your concerns around unmarkModified, but since these fields should not be present in the persisted document to begin with, and we're setting them to undefined in the encrypt method anyway, I think we should be good… let me know what you think :)

I think this is the PR that introduced the new array change tracking in Mongoose; as far as I can see, there is no option to disable it (which might have been another solution to this problem).

@yelworc
Copy link
Contributor Author

yelworc commented Apr 2, 2024 •
edited
Loading

FWIW, I finally upgraded our production system to Mongoose 6 some 4 weeks ago, using the workaround/fix in #119 – no ugly surprises so far 😄

EDIT: Moved to Mongoose 7 for a short bit, and now finally on Mongoose 8. Everything still seems to be fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug help wanted
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix errors with encrypted subdoc arrays on Mongoose v6 yelworc/mongoose-encryption
4 participants
@yelworc @spencersteers @joegoldbeck @offaxis