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(core): allow setting default schema on EntityManager #4717

Merged
merged 13 commits into from Sep 25, 2023

Conversation

EmmEm
Copy link
Contributor

@EmmEm EmmEm commented Sep 21, 2023

Hi,

I want to start with thanking you all for this great ORM that is so actively maintained and offer so many great features. One thing that really catches my eye is the per request (shell)EntityManager that I think will make this ORM a winner when it comes to multi tenancy. Almost all of the other big ORM libraries are based on a global concept.

My aim with this PR is to make a shoot at implementing schema based multi tenancy support with help of the per request EntityManager. With this PR it's possible to set a schema when forking the EntityManager. That schema will be applied as a default schema if no other schema is present. It makes use of the schema options introduced in: #2296

If this seems like an approach to pursue there is some more work to do:

  • Add a hook in the other packages like Nestjs to allow for setting schema / request
  • Documentation
  • More extensive tests

Some questions

  • Should the number of this.context() calls be reduced?
  • When .fork() an existing fork with schema, should schema be inherited and should it be possible to clear schema?

@B4nan B4nan changed the title feat(schema multi tenancy): schema based multi tenancy support feat(core): schema based multi tenancy support Sep 21, 2023
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.

i like the idea, but the implementation can simplified

packages/core/src/EntityManager.ts Outdated Show resolved Hide resolved
packages/core/src/EntityManager.ts Outdated Show resolved Hide resolved
packages/core/src/EntityManager.ts Outdated Show resolved Hide resolved
fix: shallow copy options in count to avoid racing condition in findAndCount
packages/core/src/EntityManager.ts Outdated Show resolved Hide resolved
packages/core/src/EntityManager.ts Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 22, 2023

Codecov Report

Patch coverage is 100.00% of modified lines.

❗ Current head 960fcbc differs from pull request most recent head 828f117. Consider uploading reports for the commit 828f117 to get more accurate results

Files Changed Coverage
packages/core/src/EntityManager.ts 100.00%
packages/core/src/unit-of-work/UnitOfWork.ts 100.00%
packages/core/src/utils/RequestContext.ts 100.00%

📢 Thoughts on this report? Let us know!.

@B4nan
Copy link
Member

B4nan commented Sep 22, 2023

So I guess the last missing thing is to document this somewhere? We should also come up with a better commit message as this is a bit too generic for what this PR is actually doing.

@EmmEm
Copy link
Contributor Author

EmmEm commented Sep 22, 2023

I've tested some more with request context and found an issue. I think we need to change this._schema for this.schema otherwise it wont work when using request context. this._schema will refer to the global EntityManager instance which will be wrong.

@B4nan
Copy link
Member

B4nan commented Sep 22, 2023

I don't follow, change it where?

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.

Unless you plan to make more changes, we can merge.

@B4nan B4nan changed the title feat(core): schema based multi tenancy support feat(core): allow setting default schema on EntityManager fork Sep 22, 2023
@B4nan B4nan changed the title feat(core): allow setting default schema on EntityManager fork feat(core): allow setting default schema on EntityManager Sep 22, 2023
@B4nan B4nan merged commit f7c1ef2 into mikro-orm:master Sep 25, 2023
1 check passed
@B4nan
Copy link
Member

B4nan commented Sep 25, 2023

Thanks!

B4nan added a commit that referenced this pull request Oct 2, 2023
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