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

Suggestion: please add this as PR to mongoose repo #5

Closed
lonix1 opened this issue Dec 31, 2018 · 5 comments
Closed

Suggestion: please add this as PR to mongoose repo #5

lonix1 opened this issue Dec 31, 2018 · 5 comments
Assignees

Comments

@lonix1
Copy link

lonix1 commented Dec 31, 2018

Every discussion I encounter always points back to this issue, and your solution is the only one I can find, which is weird given how important optimistic concurrency is. Especially in a non-trivial systems. I've yet to work on one that doesn't use a mix of last-write-wins, optimistic concurrency, pessimistic/locking, etc.

So it's frustrating that mongoose doesn't have this out of the box.

The discussion in that issue and others show that the authors would be willing to add OC functionality if someone would help with it. It seems you've already solved this problem.

Please please add it as a PR to the mongoose repo, so it becomes official, and supported? I'll upvote and sound off about why it's needed to encourage it's acceptance!

@eoin-obrien
Copy link
Owner

Thanks for the suggestion! I can certainly look into getting this together into a PR for mongoose.

I see that one of the issues to which you linked discusses timestamp-based OCC; I'm pretty sure that a few minor tweaks to the plugin would let it accomplish this as well as the version-based OCC it uses at present.

I'm busy with work for the next few days, but I should be able to take a look into it after that.

@lonix1
Copy link
Author

lonix1 commented Jan 1, 2019

@eoin-obrien Happy new year. No rush, just a suggestion for the backburner!

BTW, a mongoose author documents how to accomplish OC in a blog article:

  • With timestamps (updatedAt), which is in line with how it's done in various DBMSs - and Azure and (I think) AWS work that way too, so it's more natural for many folks to think in that pattern
  • With versioning (__v), as you've done

Your way works as is, so it's good enough! However in my opinion, the timestamp approach is more robust as it doesn't clobber / interfere with the library's use of __v, and more importantly, you get the benefit of linking timestamps to the domain, for example if you're using event sourcing or auditing or logging.

Thanks for considering this! It would be great to have this baked into mongoose itself.

@eoin-obrien
Copy link
Owner

eoin-obrien commented Aug 12, 2019

@lonix1 I managed to get some free time to take a look at this plugin again after finishing my exams!

I've drafted up some changes, updated dependencies and so on, and I've taken a look at adding in a timestamp-based approach to OCC as well. It uses updatedAt (or whatever custom field name is set in timestamps.updatedAt when the schema is created) to manage concurrency in much the same way as it does with version-based OCC.

The only difficulty with the implementation was that Mongoose sets a new value for updatedAt using a built-in pre-save hook that executes before any pre-save hooks added by plugins, so the plugins receive the new value for updatedAt rather than the old one. I managed to work around this by accessing the schema's hooks object directly and merging the OCC hook in just before the built-in timestamp update hook; that way, we're guaranteed that we'll get the correct value for updatedAt, and we avoid clobbering any other plugins or affecting the timestamp update.

For backwards-compatibility, the plugin defaults to using version-based OCC, but using updatedAt instead is straightforward:

const schema = new mongoose.Schema({/* fields */}, {timestamps: true});
schema.plugin(updateIfCurrentPlugin, {strategy: 'timestamp'});

Hopefully, this will make the plugin more natural for folks to use! I'd love some feedback on the PR and/or the changes in general if you've got time; I'm holding off on publishing the new version on npm until I've had a chance to test it out robustly.

@eoin-obrien eoin-obrien self-assigned this Aug 12, 2019
@lonix1
Copy link
Author

lonix1 commented Aug 12, 2019

Thanks for this, though sadly we're moved on from that project, so I'm not using mongodb any longer...

As I recall this was a major source of frustration, so I'm sure this will be very well received by mongo users. The fact that there is not out-of-the-box concurrency control in mongoose was crazy, so I still think this should be merged into it so it's got first-class support!

@eoin-obrien
Copy link
Owner

Thanks!

I've joined the discussion in Mongoose issue #5424 and offered to help with adding out-of-the-box OCC support to Mongoose.

I'm going to close this issue for now - any further developments and discussions on this topic will likely be in the Mongoose repo. Thanks for all your help!

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

No branches or pull requests

2 participants