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

feat(mongoose): add setDriver() function to allow overwriting driver in a more consistent way #11900

Merged
merged 8 commits into from
Jun 16, 2022

Conversation

vkarpov15
Copy link
Collaborator

Summary

Right now overwriting the Mongoose driver is a bit messy, because 1) Connection and Collection aren't changed when you do mongoose.driver.set(), and 2) driver is always global, so no way to have 2 Mongoose instances with the same driver in the same process.

Examples

You can now do mongoose.setDriver() without any extra work to use a different driver.

@vkarpov15 vkarpov15 self-assigned this Jun 5, 2022
lib/index.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@AbdelrahmanHafez AbdelrahmanHafez left a comment

Choose a reason for hiding this comment

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

Can you think of a way we can mimic having different drivers and setting them to verify that this works?

Would deep cloning the driver, modify some arbitrary property, then setting it, and asserting that getting the driver, we'll have the arbitrary property be enough testing?

Otherwise, LGTM.

Copy link
Collaborator

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

I dont know how to test this. Maybe creating a mock with a Proxy around the actual mongodb driver, and then checking if the Proxy is set after using setDriver. And when we are successful we set back the original driver.

lib/connection.js Outdated Show resolved Hide resolved

const openConnection = _mongoose.connections && _mongoose.connections.find(conn => conn.readyState !== STATES.disconnected);
if (openConnection) {
const msg = 'Cannot modify Mongoose driver if a connection is already open. ' +
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick: why do we need a one time variable? We could directly pass it to MongooseError

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Better readability IMO. Easier to break up the long message if there's a temporary variable rather than putting it as a param.

lib/index.js Outdated Show resolved Hide resolved
lib/index.js Outdated Show resolved Hide resolved
vkarpov15 and others added 4 commits June 16, 2022 13:04
Co-authored-by: Uzlopak <aras.abbasi@googlemail.com>
Co-authored-by: Uzlopak <aras.abbasi@googlemail.com>
@vkarpov15 vkarpov15 changed the base branch from master to 6.4 June 16, 2022 17:34
Copy link
Collaborator

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

LGTM

@vkarpov15 vkarpov15 merged commit 6db2e4a into 6.4 Jun 16, 2022
@vkarpov15 vkarpov15 deleted the vkarpov15/driver-fixes branch June 16, 2022 19:26
@vkarpov15 vkarpov15 added this to the 6.4 milestone Jun 16, 2022
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.

None yet

3 participants