scottwrobinson / camo

A class-based ES6 ODM for Mongo-like databases.
556 stars 80 forks source link

findOneAndUpdate using nedb client returns not updated document with updated nested property #55

Closed royaltm closed 8 years ago

royaltm commented 8 years ago

complete example in gist

updating with nested property:

SomeDoc.findOneAndUpdate({_id: someid}, {"foo.bar": "some new value"})

resolves to a document before the update, however the document in nedb storage gets updated.

in case of mongo the mongo's internal findOneAndUpdate implementation is responsible for correct behavior in this instance however there are no equivalents for atomic find and update in nedb, so you are doing a hackish thing here, while instead you probably should just re-read the document from nedb:

                        db.findOne({_id: data._id}, function(error, doc) {
                            if (error) return reject(error);
                            resolve(doc);
                        });

nedb is already keeping the document in memory so this another call to db.findOne would most probably return consistent result with the last update.

scottwrobinson commented 8 years ago

NeDB does occasionally do file reads/writes (when using a file as the backend), so I wanted to avoid as many find/save/update calls as possible, but in hindsight this was a pretty bad idea that wasn't adequately backed up with tests...

Thanks for bringing this up, and for providing full example code :smiley:

royaltm commented 8 years ago

IMHO just after the update, nedb has 99.999% chance that the referred document will still be in memory (AFAIK it keeps everything in mem once it's there), so the question is how does nedb schedule client calls. There's a race possibility between update and re-read but only if the promise on the nedb side isn't fullfilled immediately. We won't know without RTFS of nedb.

royaltm commented 8 years ago

Maybe the proper way would be to ask for an atomic update function in nedb. I wonder why it's not already there.

scottwrobinson commented 8 years ago

Okay, just a few notes...

Looks like in NeDB v1.8+ db.update() now returns the updated doc data in the callback. This solves our issue, but the problem with v1.4+ is that there was a breaking change in the storage system. Camo currently uses v1.1.2.

So for the next patch version I'm about to release, I'll have to use a temporary fix (likely the one you posted above). In the next minor release I'll update the required NeDB version in package.json and provide a real fix to this issue.

Thanks!