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: resolve issue with ordering of middleware being applied #189

Merged
merged 17 commits into from
Jan 23, 2025

Conversation

tsangste
Copy link
Contributor

@tsangste tsangste commented Sep 18, 2024

When setting up the nestjs modules with MikroORM there is a chance it can throw Using global EntityManager instance methods for context specific actions is disallowed. when interacting with the EM within another middleware.

This fix applies to both single and multi database set-ups.

…`forRoot` registering to resolve issue with ordering of middleware being applied
@B4nan
Copy link
Member

B4nan commented Sep 20, 2024

i cant say i like the naming, the word multiple feels weird here, also the whole point of that module is to deal with the middleware, why should we rename it in the first place?

@tsangste
Copy link
Contributor Author

i cant say i like the naming, the word multiple feels weird here, also the whole point of that module is to deal with the middleware, why should we rename it in the first place?

The module doesn't just handle the middleware it also handles registering multiple MikroOrm to allow use of the MikroOrms token for DI, which is why I thought it would be better to rename it to multiple.

@B4nan
Copy link
Member

B4nan commented Sep 21, 2024

the user still needs to do the MikroOrmModule.forRoot calls explicitly, right? sounds like an implementation detail to me, from user point of view, it doesn't do anything else than the middleware i would say.

strictly speaking, the renaming of various symbols and types is a breaking change, i would rather keep the old naming everywhere

@tsangste
Copy link
Contributor Author

tsangste commented Sep 21, 2024

the user still needs to do the MikroOrmModule.forRoot calls explicitly, right? sounds like an implementation detail to me, from user point of view, it doesn't do anything else than the middleware i would say.

strictly speaking, the renaming of various symbols and types is a breaking change, i would rather keep the old naming everywhere

Yes the user would be required to MikroOrmModule.forRoot but that would always be the case for each database. But going along with the export changes to expose the MikroOrms (#88) and that this specific middleware is only used for multiple database it makes sense for the module to be named more scope to used for mutiple database. Also I thought there would might be a case where someone could want to use the MikroOrms token to get all databases but might not require the middleware (was planning to do after as it did make sense as part of this PR to disable the middleware) and use the DI scopes.

Yeah I understand adding the new MULTPLE_MIKRO_ORM_MODULE_OPTIONS symbol and type is a breaking change but I wanted those options to be closer to the module due to the renaming and it being scoped to multiple databases, but as this should be used internally I didn't think this would be an issue. Either way I am happy to continue using the old symbol and alias the old type for removal in the future.

@tsangste
Copy link
Contributor Author

tsangste commented Nov 9, 2024

@B4nan, I have added additional tests and a test to show the issue arising; I have also found that this issue also occurs with the original middleware registration within mikro-orm-core.module and I think the fix is to move the middleware registration up to mikro-orm.module but I think I would do this in another PR

Copy link
Member

@B4nan B4nan left a comment

Choose a reason for hiding this comment

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

few notes to begin with, the main takeaway: lets rename all the symbols from Multiple* to just Multi*, feels like better english to me

also be careful with all the renames, there are still breaking changes in this PR

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/mikro-orm.module.ts Outdated Show resolved Hide resolved
src/mikro-orm.module.ts Outdated Show resolved Hide resolved
src/typings.ts Outdated Show resolved Hide resolved
tests/mikro-orm.middleware.test.ts Show resolved Hide resolved
@tsangste tsangste changed the title fix: resolve issue with ordering of middleware being applied for multiple database fix: resolve issue with ordering of middleware being applied Nov 18, 2024
@tsangste tsangste requested a review from B4nan November 18, 2024 11:22
@tsangste
Copy link
Contributor Author

@B4nan updated based off your review

@B4nan
Copy link
Member

B4nan commented Jan 22, 2025

looks like your approach is not working with nest 11

@tsangste
Copy link
Contributor Author

looks like your approach is not working with nest 11

i will give it ago with nestjs 11

@B4nan
Copy link
Member

B4nan commented Jan 22, 2025

maybe the problem you were trying to solve is no longer valid with v11? that's the first thing I would try to verify

@tsangste
Copy link
Contributor Author

tsangste commented Jan 22, 2025

maybe the problem you were trying to solve is no longer valid with v11? that's the first thing I would try to verify

Yeah I notice some module changes and will go through it, if its fixed I will just leave the tests in to make sure its working to what I was expecting

@tsangste
Copy link
Contributor Author

maybe the problem you were trying to solve is no longer valid with v11? that's the first thing I would try to verify

Yeah I notice some module changes and will go through it, if its fixed I will just leave the tests in to make sure its working to what I was expecting

I've done some quick investigation on this, so with nestjs 11 they have changed the way how module distances are calculated with global modules and with the latest change it makes the global module middleware get executed last. I will raise an issue on the nestjs repo to see what they say.

@tsangste
Copy link
Contributor Author

tsangste commented Jan 23, 2025

maybe the problem you were trying to solve is no longer valid with v11? that's the first thing I would try to verify

The issue has been resolved https://github.com/nestjs/nest/releases/tag/v11.0.5.

With the fix Global Modules should execute first now and think we should keep the code fix so it works for nestjs 10.

@tsangste
Copy link
Contributor Author

@B4nan I have updated the peerDependency for nestjs 11 to go from ^11.0.5 so anyone installing shouldn't be using anything lower

@B4nan
Copy link
Member

B4nan commented Jan 23, 2025

why? because of something that was present for years now, in 10.x versions that are still supported? i would keep the peer deps loose, its up to the user to stay up to date. we can enforce this in v7 where we disallow older nest versions, but now it feels a bit weird, especially since it doesn't affect everyone (as mentioned above, the real-world example app still works just fine without any changes, even with lower versions).

@B4nan
Copy link
Member

B4nan commented Jan 23, 2025

do i get it right that things without your PR worked fine, and with the changes you did here it broke with v11 before 11.0.5?

@tsangste
Copy link
Contributor Author

do i get it right that things without your PR worked fine, and with the changes you did here it broke with v11 before 11.0.5?

My changes were mainly to resolve the issue in 10.x where the middleware registered in a global module it could be executed later and the test I made here we were able to catch an issue in nestjs 11 where it made the issue worse due to a bug where it made the global module middleware execute last hence why we should force to go from ^11.0.5 cause even without my fix it would cause the request context be executed last but happy to keep it loose at ^11.0.0 if you want it to be up to the users responsibility.

@tsangste
Copy link
Contributor Author

If we get this fix in then for nestjs 10.x we can remove this line https://github.com/mikro-orm/nestjs-realworld-example-app/blame/a61ea041db0d0cf24f32f49b262c5bfecd6a726d/src/app.module.ts#L32 in your example project

@B4nan
Copy link
Member

B4nan commented Jan 23, 2025

If we get this fix in then for nestjs 10.x we can remove this line mikro-orm/nestjs-realworld-example-app@a61ea04/src/app.module.ts#L32 (blame) in your example project

Oh, now that's interesting, didn't realize it's about this. Agreed that it makes sense to merge it in.

Let's keep it I guess, since the bump is not released out, I guess its fine to enforce first usable version.

@B4nan B4nan merged commit 98171c4 into mikro-orm:master Jan 23, 2025
2 checks passed
@tsangste tsangste deleted the multiple-database-middleware-order branch January 23, 2025 21:27
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.

2 participants