-
Notifications
You must be signed in to change notification settings - Fork 15
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
Counter increments even if document validation fails #10
Comments
Autoincrement should not call validation, cause it may break somebody logic and produce unnecessary overhead and unobvious behavior. You may disable auto increment by: doc.__maiRanOnce = true; But it is not clear and internal property. So let to expose this via adding a new public method for the document (like it did for another stuff): schema.method('disableAutoIncrement', function() {
const doc: MongooseDocument = this;
doc.__maiRanOnce = true;
}); PR are welcome. And please don't forget to add tests. |
Good point on not calling validation from autoincrement. I however fail to see how I would disable the auto increment selectively for documents that fail validation, without calling the validation function. Since the schema itself runs validate and then save, maybe we should just do the autoincrement on the post hook for validate? This would be a much cleaner fix. We could just replace the pre hook on 'validate' with a post hook: Thoughts? |
I dont know, to tell the truth It would be expected behaviour for me. |
Interested in this. Same here. Thanks for solving the issue. |
On trying to save a document that fails the Mongoose validation specified in the schema, the counter for the corresponding collection still gets incremented but the document does not get saved (as expected). This results in the auto incremented ids not being continuous.
Could we add a flag that checks if the validation fails for a document and not increment the counter in that case? I had a look at the pre hook on 'validate' and this could be a potential fix:
Replace:
if ((doc.isNew && !alreadyGetId) || settings.migrate) {
with:
if ((doc.isNew && !alreadyGetId && doc.validateSync === undefined) || settings.migrate) {
I'm not sure if this is the best way to achieve this, but this could be a start.
The text was updated successfully, but these errors were encountered: