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

Correct Database per service + mongoose V8 #370

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

thib3113
Copy link
Contributor

@thib3113 thib3113 commented Sep 22, 2023

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

@thib3113
Copy link
Contributor Author

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 .

@thib3113
Copy link
Contributor Author

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();
Copy link
Member

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?

Copy link
Contributor Author

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 )

@thib3113
Copy link
Contributor Author

thib3113 commented Sep 27, 2023

@icebob I have a choice to do .

By default, did we create a new connection, or use the global one ?
Pros :

  • people using the global connection will not be impacted by a breaking change on this

Cons :

  • this will be "first come first served", so sometimes the global connection will not be the same connection (and will become harder to debug)

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
( and why not reusing the objectIDToString function ? )

@icebob
Copy link
Member

icebob commented Oct 1, 2023

By default, did we create a new connection, or use the global one ?
I think, don't using the global connection is better, because everyone need to adapt following the breaking change.

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.

( 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 ( and why not reusing the objectIDToString function ? )

Hmm, strange, I have no idea why I implemented it this complicated way. :) You can rewrite this method.

@Freezystem
Copy link
Contributor

Hey! Mongoose 8.0.0-rc0 was released two days ago.
The stable version will be out any time soon.
As the PR is not acheived yet, you may consider skipping Mongoose 7.x.x in favor of 8.x.x
Keep up the good work !

@thib3113 thib3113 changed the title Correct Database per service + mongoose V7 Correct Database per service + mongoose V8 Jan 2, 2024
@thib3113
Copy link
Contributor Author

thib3113 commented Jan 2, 2024

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 )

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.

3 participants