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): throw an error if calling mongoose.connect() multiple times while connected #7905

Merged
merged 2 commits into from
Jun 20, 2019

Conversation

Fonger
Copy link
Contributor

@Fonger Fonger commented Jun 17, 2019

Summary

This may prevent developers from the misuse of mongoose. Resolves #7814.

@Fonger Fonger force-pushed the feat/multiple-connect-error branch from 81ebbc5 to b30d16f Compare June 17, 2019 07:57
@Fonger Fonger force-pushed the feat/multiple-connect-error branch from b30d16f to 8ca7be4 Compare June 17, 2019 08:05
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 added this to the 5.6.1 milestone Jun 20, 2019
@vkarpov15 vkarpov15 merged commit 2a2c71a into Automattic:master Jun 20, 2019
@sarora2073
Copy link

This fix to prevent multiple connections ignores valid scenarios where we may want multiple separate connections to the DB e.g. multiple background processing connections that may intentionally have the own separate connections setup e.g. so they can be moved around servers. I hope this "fix" will be modified e.g. a parameter like "allowMultipleConnections", if it doesn't already exist.

@dankingtech
Copy link

I think that overall, this is a very good thing, and it can help isolate some unexpected problems. However, I think I would agree that having a option to create multiple connections might be nice. For instance, while running asynchronous unit tests, where I'm now getting the error.

I currently use mocha and have one unit test file per model, and each is opening its own connection and closing it when the tests are done, causing them to potentially overlap. Is it bad form to run simultaneous tests like this, or could we have an option to allow such uses to override this check?

@Fonger
Copy link
Contributor Author

Fonger commented Jun 27, 2019

@sarora2073 @dankadmin

I think in that case, you should call mongoose.createConnection instead.

mongoose.connect is used for a global mongoose default connection singleton and is not designed to call connect() multiple times when connected in any cases.

More info on multiple connections: https://mongoosejs.com/docs/connections.html#multiple_connections

@sarora2073
Copy link

@Fonger - thanks! Didn’t know that was an option.

@giovanni-orciuolo
Copy link

Seriously, you did this in a MINOR patch!?

@Fonger
Copy link
Contributor Author

Fonger commented Jul 16, 2019

Sorry for the trouble and breaking changes.

This change has been reverted ca24377 and probably will be released as 5.6.5 shortly.

However, this changes may be applied in mongoose 6 again because calling mongoose.connect multiple times is still a misuse and it may lead to potential bugs (even if it worked).

If your code rely on mongoose.connect multiple times, it's highly recommended to update your code asap.

@sarora2073
Copy link

FWIW - i almost freaked out at first when i thought i wasn't going to be able to use mongoose for multiple connections due to this change BUT as per above discussion really this change is just enforcing the intended usage of connect vs createConnection as detailed in docs and here: https://stackoverflow.com/questions/40818016/connect-vs-createconnection

To that end, it shouldn't be consider a "breaking change" so much as an attempt to enforce expected usage, even though as a practical matter it may require you to update your code to use the correct connection method, if you have multiple connections intentionally.

@dankingtech
Copy link

It is true that this is not removing any previously intended functionality, and it is only enforcing the use of the software as it has been intended, and I even found a mention of wanting to enforce this behavior from back in 2012.

On the other hand, most, in not all, of the tutorials and documentation that I can find on the web uses the single connection method. That's no surprise, though, considering that even the official documentation almost exclusively seems to use mongoose.connect. I understand that it's the most simple thing to use, but I believe that many professional projects are going to need multiple connections, if for nothing more than asynchronous unit testing.

It's not a trivial change in code to switch, considering that you have to modify the way your models work to ensure that you are using the correct connection, and there is a lot of code re-factoring. So, I would recommend that if you are going make this change, please also update the official documentation to use mongoose.createConnection for the majority of the examples, including the way that models are used. Then, maybe future tutorials on the web will begin to update as well to make it easier on new users.

@vkarpov15
Copy link
Collaborator

There's no problem with using mongoose.connect(), that's sufficient for most apps. The problem is only with calling mongoose.connect() multiple times, which is likely to cause nasty surprises - what happens if you call mongoose.connect() in the middle of a populate()?

I've had a couple people reach out saying they used multiple mongoose.connect() calls as an alternative to useDb() because they weren't aware of useDb(). Is there another reason to call mongoose.connect() multiple times?

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.

Throw an error if calling mongoose.connect() multiple times while connected
5 participants