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

Hibernate ORM - Multitenancy support #5681

Closed
Sanne opened this issue Nov 22, 2019 · 18 comments · Fixed by #8545
Closed

Hibernate ORM - Multitenancy support #5681

Sanne opened this issue Nov 22, 2019 · 18 comments · Fixed by #8545

Comments

@Sanne
Copy link
Member

Sanne commented Nov 22, 2019

Description

Hibernate ORM is able to support multi-tenancy in various ways.

Some of these imply "plugging" custom strategy implementations during the bootstrap of the ORM, and/or setting the tenant ID when the Session is opened.

Both aspects could see some level of automation and "just work" OOB without needing to implement such SPIs - most importantly, since Quarkus needs to control the boostrap proces we would need to ensure such details are available during augmentation.

Analysis

TBD

First user request is here: https://quarkusio.zulipchat.com/#narrow/stream/187030-users/topic/Quarkus.20ORM.20multitenancy

Tasks

  • [ ]
@Sanne Sanne added area/hibernate-orm Hibernate ORM kind/epic Large issue with links to sub-issues labels Nov 22, 2019
@FroMage
Copy link
Member

FroMage commented Nov 22, 2019

Can you add a pointer to the docs showing how it works currently in ORM?

@trsantospt
Copy link

trsantospt commented Nov 22, 2019

@stefanorg
Copy link

+1

@michael-schnell
Copy link
Contributor

@michael-schnell
Copy link
Contributor

I started working on this issue.

@michael-schnell
Copy link
Contributor

michael-schnell commented Apr 4, 2020

I added the first classes for tenant support:
https://github.com/michael-schnell/quarkus/commits/master

In general the design is inspired by the existing ODIC multitenant code:

  • TenantResolver = Allows user to create the tenant ID from the request (mandatory). Same interface as for ODIC, so it could be combined to allow same resolution for ODIC and Hibernate ORM.
  • TenantConnectionResolver = Allows user to create the connection not based on properties (optional)
  • DataSourceTenantConnectionResolver = Default implementation of the TenantConnectionResolver that uses data sources from the application.properties (if the user only provided a TenantResolver).
  • multiTenant = Config property that defines the mode (NONE, DATABASE, SCHEMA). DISCRIMINATOR will not be supported, because not finished in Hibernate itself.

As I'm new to the Quarkus build & runtime extension mechanism, I would need some help to get the configuation done right. Some questions here:

@emmanuelbernard
Copy link
Member

I suspect these settings should be build time only. i.e. the user decides of the multiTenant strategy is fine to be fixed at build time. So that data would be read at build time in HibernateOrmProcessor and passed along as resolved configuration. Maybe it would generate some build time static service factory from the Hibernate ORM Service registry (@Sanne WDYT). That way, this configuration would be 100% build time processed.

What does validateExistingCurrentSessions supposed to do? i.e. why would I want to swap it?

The Tenant Id resolution / resolver, I wonder if that should be a global information (i.e. shared by several Quarkus extensions). We would put it in its own shared extension to be consumed by anyone interested.

@michael-schnell
Copy link
Contributor

michael-schnell commented Apr 7, 2020

That way, this configuration would be 100% build time processed

Yes exactly. I tried to follow the existing Hibernate configuration process, but if I test the code there is a message like "multiTenant property ignored" at runtime. Sounds like I missed something.

What does validateExistingCurrentSessions supposed to do? i.e. why would I want to swap it?

This is a value from Hibernate (CurrentTenantIdentifierResolver) that enabled re-validation of the tenant.

Tenant Id resolution / resolver, I wonder if that should be a global information

Could be done that way, but as it is an interface, it does not really matter because you can simply implement multiple ones like from ODIC & Hibernate ORM.

@emmanuelbernard
Copy link
Member

That way, this configuration would be 100% build time processed

Yes exactly. I tried to follow the existing Hibernate configuration process, but if I test the code there is a message like "multiTenant property ignored" at runtime. Sounds like I missed something.

My point is that you should not even need to read it at runtime. Read it at build time and set the right properties for Hibernate when it starts in the STATIC_INIT phase or runtime phase. The fact that you read it at runtime means you are doing work too late.

What does validateExistingCurrentSessions supposed to do? i.e. why would I want to swap it?

This is a value from Hibernate (CurrentTenantIdentifierResolver) that enabled re-validation of the tenant.

Force it to true to be safe but, I don't think we use CurrentSessionContext in Quarkus so that's a moot flag.

Tenant Id resolution / resolver, I wonder if that should be a global information

Could be done that way, but as it is an interface, it does not really matter because you can simply implement multiple ones like from ODIC & Hibernate ORM.

It depends who would be the person implementing these interfaces. If that's the user than needs to sync, that's not great. But we can wait for another multi tenant framework before digging further,

@emmanuelbernard
Copy link
Member

That way, this configuration would be 100% build time processed

Yes exactly. I tried to follow the existing Hibernate configuration process, but if I test the code there is a message like "multiTenant property ignored" at runtime. Sounds like I missed something.

My point is that you should not even need to read it at runtime. Read it at build time and set the right properties for Hibernate when it starts in the STATIC_INIT phase or runtime phase. The fact that you read it at runtime means you are doing work too late.

I suspect that doing work in HibernateOrmProcessor#build would be the right level. @Sanne thoughts?

@michael-schnell
Copy link
Contributor

michael-schnell commented Apr 10, 2020

OK, finally got it working (See commits). To be honest, the documentation in "quarkus-hibernate-orm" and "quarkus-hibernate-orm-deployment" is really a mess... Almost no classes and no methods have any JavaDoc. I created another task with request to clean this up: #8525

Only thing that remains now, is the DataSourceTenantConnectionResolver that is not found at runtime. If I copy the same class into my demo app, it works fine.

Any idea why the class isn't found up at runtime?

@michael-schnell
Copy link
Contributor

michael-schnell commented Apr 11, 2020

  • Both SCHEMA and DATABASE approach works now (Code is in my forked repo)
  • A Quickstart that can be used to test the multitenancy with SCHEMA or DATABASE approach is available
  • Native mode works fine
  • Section added to the guide on how to use it

I will now create a pull request for the feature.

@michael-schnell
Copy link
Contributor

michael-schnell commented Apr 12, 2020

Can someone do the review?

#8545

@emmanuelbernard
Copy link
Member

I prefer if @Sanne or @gsmet review. It will be more accurate. Be aware though that this is a difficult time, a lot of the reviewers have confined small children so the todo list is not going down as fast as usual.

For the fact the the interface / class is not found, I am not sure why it is eliminated by GraalVM (and not when you copy it). But you can add a io.quarkus.deployment.builditem.nativeimage.ReflectiveClassBuildItem with the class to make sure it is picked up. Only enlist it if multi tenancy is really necessary (based on configuration)

@Sanne
Copy link
Member Author

Sanne commented Apr 14, 2020

hi @michael-schnell , many thanks for the PR! I'll try to review it today, maybe tomorrow. Apologies for the delay.

@michael-schnell
Copy link
Contributor

I prefer if @Sanne or @gsmet review. It will be more accurate. Be aware though that this is a difficult time, a lot of the reviewers have confined small children so the todo list is not going down as fast as usual.

For the fact the the interface / class is not found, I am not sure why it is eliminated by GraalVM (and not when you copy it). But you can add a io.quarkus.deployment.builditem.nativeimage.ReflectiveClassBuildItem with the class to make sure it is picked up. Only enlist it if multi tenancy is really necessary (based on configuration)

The problem is already fixed. I found the place to add the class and it works fine now.

@michael-schnell
Copy link
Contributor

hi @michael-schnell , many thanks for the PR! I'll try to review it today, maybe tomorrow. Apologies for the delay.

OK, cool. I created another pull request for the feature's quickstart: quarkusio/quarkus-quickstarts#525

@michael-schnell
Copy link
Contributor

@Sanne How about the review? If there are changes needed, it would be nice if I could do it over the weekend as I have to do it in my spare time

@Sanne Sanne added kind/new-feature release/noteworthy-feature and removed kind/epic Large issue with links to sub-issues labels May 17, 2020
@Sanne Sanne added this to the 1.5.0 milestone May 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants