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

fix(connection): disable createConnection buffering if autoCreate option is changed #13007

Merged
merged 6 commits into from Feb 21, 2023

Conversation

lpizzinidev
Copy link
Contributor

fix #12940

Summary
Avoid usage of cached createCollection callback if autoCreate option value has changed.

@lpizzinidev
Copy link
Contributor Author

@vkarpov15
Do you think that this is the correct approach?
If so I'll setup a MongoDB 4.0.x env to check why tests are failing on that specific version only.
Otherwise I'll search for another way to solve the issue.

lib/model.js Outdated
const autoCreate = utils.getOption('autoCreate',
this.schema.options, this.db.config, this.db.base.options);

if (this.$init != null && this.$autoCreate === autoCreate) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with this approach is that you may have multiple $init's going at the same time. Model.init(); Model.schema.options.autoCreate = false; Model.init();. The purpose of $init is to avoid that.

I think a better approach would be to move the utils.getOption() call for autoCreate into the _createCollection() function itself. And put _createCollection() behind buffering like how _wrapConnHelper() works.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback 💪 I'll work on this

lib/model.js Outdated
});
};
};
const _createCollection = _wrapCollHelper(function _createCollection(cb) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency, let's also delay checking autoIndex until after buffering is done. Apply similar changes to _ensureIndexes(). No need to do _wrapCollHelper on _ensureIndexes() though, since _createCollection() will already wait for buffering.

lib/model.js Outdated
};
};
const _createCollection = _wrapCollHelper(function _createCollection(cb) {
const autoCreate = utils.getOption('autoCreate',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, let's not add a separate _wrapCollHelper() function, since that function won't be reused anywhere. Just add the if ((conn.readyState === STATES.connecting || conn.readyState === STATES.disconnected) && conn._shouldBufferCommands()) check directly into _createCollection().

Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 👍

@vkarpov15 vkarpov15 merged commit 3ddc2e2 into Automattic:master Feb 21, 2023
@vkarpov15 vkarpov15 added this to the 6.9.3 milestone Feb 21, 2023
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 this pull request may close these issues.

Connection autoCreate option behaves differently migrating 5 to 6
2 participants