-
Notifications
You must be signed in to change notification settings - Fork 122
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
Correct Database per service + mongoose V8 #370
base: master
Are you sure you want to change the base?
Correct Database per service + mongoose V8 #370
Conversation
A note about virtual tests . Actually, they are using global models ( from global mongoose connection ) . So, they doesn't work, because the default connection is never connected, and so, we can't use the models . |
Virtuals seems to come from this PR : #354 . But, the logic seems not working with the "one database per service" pattern, because mongoose can't load the other object ... I need to investigate how to handle this, and what is the problem resolved by the other PR |
} else if (this.conn && this.conn.close) { | ||
return this.conn.close(); | ||
} else { | ||
return mongoose.connection.close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this row is not relevant if we don't "touch" the default connection, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes . ( on the first check, I think opening a connection with populate the default one ... but it's not the case, so this will do nothing ) .
I'll note to remove this ( and maybe add a warning, just in case )
@icebob I have a choice to do . By default, did we create a new connection, or use the global one ?
Cons :
I think, don't using the global connection is better, because everyone need to adapt following the breaking change . ( about the rest of the code, it doesn't change so much ) Also, did you remember why of this : https://github.com/moleculerjs/moleculer-db/blame/master/packages/moleculer-db-adapter-mongoose/src/index.js#L326-L330 |
Since a Mongoose 7 upgrade already causes a breaking change, we can adapt the new connection way, but we should write a migration help for mongoose adapter users on how they should migrate their codes to work with the new version.
Hmm, strange, I have no idea why I implemented it this complicated way. :) You can rewrite this method. |
Hey! Mongoose 8.0.0-rc0 was released two days ago. |
I'm doing some tests with V8 . And, first, I think I will need to do something different to handle user using global connection . not sure if I add an option "useGlobalConnection", or a method that will be called with the current connection, I think I prefer the second one to respect "database per service" . But option 1 will maybe be easier for users . Also, it seems that mongoose V8 remove/rename some functions ( findByIdAndRemove disapear ) |
This PR will correct the "database per service" pattern . So, running multiples services on the same node, can use different databases connections (one per service) .
Because this can include some breaking changes, I choose to upgrade mongoose to V7 (and so drop old code supporting older versions)
I will pass this PR in ready when I think it will be good . For the moment, I will just to tests . Don't hesitate to do some discussions