Support multitenancy with multiple persistence units #11772

gsmet · 2020-09-01T10:24:28Z

I have to add more tests to fully test it but given the timeframe, it would be nice if @geoand and @machi1990 could have a look to what I did.

/cc @michael-schnell
Michael, I have some questions for you, could you please ping me when you're available? Thanks!

gsmet · 2020-09-01T10:29:36Z

...ent/src/main/java/io/quarkus/hibernate/orm/deployment/HibernateOrmConfigPersistenceUnit.java

+     * if not set.
+     */
+    @ConfigItem
+    public Optional<String> multitenantSchemaDatasource;


@michael-schnell @Sanne I'm not entirely sure this is useful now that we can define a datasource for a PU. Or in the case of the schema do you want to use another datasource when the tenant is defined?

AFAICT there is actually zero difference between SCHEMA and DATABASE in Hibernate multitenancy, so I don't think you need two things.

I don't know about Hibernate ORM but it makes a difference here as things are handled in the Quarkus extension. I'm unclear though if we really want this.

I agree this needs more discussions and thoughts.

There might not be a difference within Hibernatate between SCHEMA and DATABASE, but there is one if you need to generate to database schema with Flyway.

gsmet · 2020-09-01T10:30:18Z

...rc/main/java/io/quarkus/hibernate/orm/runtime/tenant/DataSourceTenantConnectionResolver.java

-            AgroalDataSource dataSource = Arc.container().instance(AgroalDataSource.class).get();
+
+        if (multiTenancySchemaDataSourceName == null) {
+            AgroalDataSource dataSource = getDataSource(dataSourceName);
            return createFrom(dataSource.getConfiguration());


@michael-schnell @Sanne I'm not sure I understand the need to somehow copy the existing datasource?

geoand · 2020-09-01T10:37:03Z

...-orm/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/HibernateOrmProcessor.java

+                configurator.addQualifier().annotation(DotNames.NAMED)
+                        .addValue("value", persistenceUnitDescriptor.getPersistenceUnitName()).done();
+                configurator.addQualifier().annotation(PersistenceUnit.class)
+                        .addValue("value", persistenceUnitDescriptor.getPersistenceUnitName()).done();


Shouldn't this be "name"?

No, this is our own annotation: https://github.com/quarkusio/quarkus/blob/master/extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/PersistenceUnit.java#L39

Ah OK, I thought it was the javax.persistence annotation

I decided against it as they are quite confusing.

And also I needed it to be a qualifier.

Makes sense

geoand · 2020-09-01T11:07:29Z

I can't comment on the Hibernate specific stuff, but the Quarkus specific stuff looks great :)

gavinking · 2020-09-01T12:02:29Z

Alright, @Sanne, after looking over this stuff, and because coincidentally I happened to be working on multitenancy in HR yesterday, so I was forced to take a close look at how that worked, I gotta say this looks kinda wrong to me.

If I understand correctly (I may not!) You've added a second layer of Quarkus-specific tenant resolvers and connection providers on top of the resolvers Hibernate already has I think because you want these to be CDI components, rather than being configured by properties. But the code is really nonsimple and essentially introduces new Quarkus-specific APIs that mirror APIs already in Hibernate. (Again, assuming I understand correctly.)

I think this is all just the wrong way to go about it.

First of all, I think Hibernate's multitenancy stuff is almost useless in this context. It doesn't actually really do anything except for allowing you to write code to assign connections to tenant ids. It's essentially reasonable in Hibernate where we don't assume any sort of control over the container or non-container environment, but I don't see how it's reasonable here.
The Hibernate APIs aren't actually contributing any value here, since the user winds up having to write Quarkus-specific code anyway.

(Note tangentially that the whole MultiTenancyStrategy enum is misleading: there's only one strategy that's actually supported: writing code to assign a connection to a tenant id.)

There are significant problems with doing multitenancy at the level of the Hibernate extension, including that any non-Hibernate code doesn't automatically run in the context of the current tenant.

What I think should happen here is that multitenancy should be an aspect of the Quarkus datasource, and it should bypass Hibernate multitenancy entirely.

If we do want to support Hibernate's builtin multitenancy APIs, we should do that in a way that is compatible with existing code that users have, and just let them configure it by properties as they usually would in Hibernate.

gsmet · 2020-09-01T12:17:31Z

FWIW, I have absolutely no opinion about how things were done as I was not involved in the initial multitenancy patch.

I just made the existing code work with multiple persistence units.

If we want to rewrite this, it's something for 1.9 and is orthogonal to this PR.

gavinking · 2020-09-01T12:20:37Z

Yes @gsmet I know that, I just don't have anywhere else to write this feedback.

geoand

Let's just make sure the latest commit (a test commit) isn't added.

Please dismiss once the commit is removed

Commit removed!

gsmet · 2020-09-02T08:05:52Z

I'm merging this one as I want it tested as part of CR1.

I suggest we start a more general discussion about multitenancy in the mailing list.

michael-schnell · 2020-09-06T05:53:27Z

FWIW, I have absolutely no opinion about how things were done as I was not involved in the initial multitenancy patch.

I just made the existing code work with multiple persistence units.

If we want to rewrite this, it's something for 1.9 and is orthogonal to this PR.

Sorry, was off for some days.

@gsmet You should take a closer look at the history of #8545 where you were explicitly asked to do a review..,

michael-schnell · 2020-09-06T06:15:04Z

I think because you want these to be CDI components, rather than being configured by properties.

Yes, that was the idea.

But the code is really nonsimple and essentially introduces new Quarkus-specific APIs that mirror APIs already in Hibernate. (Again, assuming I understand correctly.)

Why is it not simple? It is exactly doing what is described in the Hibernate manuals:
https://docs.jboss.org/hibernate/orm/5.4/userguide/html_single/Hibernate_User_Guide.html#multitenacy-separate-database
https://docs.jboss.org/hibernate/orm/5.4/userguide/html_single/Hibernate_User_Guide.html#multitenacy-separate-schema

I think this is all just the wrong way to go about it

Unfortunately the Hibernate Multitenancy feature isn't finalized since years. As stated in the manual "The JPA expert group is in the process of defining multitenancy support for an upcoming version of the specification".

First of all, I think Hibernate's multitenancy stuff is almost useless in this context. It doesn't actually really do anything except for allowing you to write code to assign connections to tenant ids.

What is wong about it?

The Hibernate APIs aren't actually contributing any value here, since the user winds up having to write Quarkus-specific code anyway.

Might be right. Any suggestions to do it better are highly welcome.

(Note tangentially that the whole MultiTenancyStrategy enum is misleading: there's only one strategy that's actually supported: writing code to assign a connection to a tenant id.)

That is not correct. The information is also used to generate the schema with Flyway, which is different for database and schema based multitenany (see Quickstart example)

There are significant problems with doing multitenancy at the level of the Hibernate extension, including that any non-Hibernate code doesn't automatically run in the context of the current tenant.

This would not be any way better if you let the user "configure it by properties as they usually would in Hibernate".

What I think should happen here is that multitenancy should be an aspect of the Quarkus datasource, and it should bypass Hibernate multitenancy entirely.

Please go ahead and make a suggestion on how to do it.

If we do want to support Hibernate's builtin multitenancy APIs, we should do that in a way that is compatible with existing code that users have, and just let them configure it by properties as they usually would in Hibernate.

I think the current solution is very easy for end users. Simply add the resolver and configure your data sources. But I'm open to any suggestions on how to do it better.

gavinking · 2020-09-06T06:33:13Z

Sorry, I should have mentioned that I opened #11861 to discuss this idea.

boring-cyborg bot added the area/hibernate-orm Hibernate ORM label Sep 1, 2020

gsmet commented Sep 1, 2020

View reviewed changes

geoand reviewed Sep 1, 2020

View reviewed changes

geoand previously requested changes Sep 1, 2020

View reviewed changes

gsmet added 2 commits September 1, 2020 16:03

Add elements missing in Hibernate ORM config isAnyPropertySet() methods

0af95bb

Support multitenancy with multiple persistence units

aabda57

gsmet force-pushed the multi-pus-multitenancy branch from 280aefa to aabda57 Compare September 1, 2020 14:04

gsmet merged commit 2466510 into quarkusio:master Sep 2, 2020

gsmet added this to the 1.8.0.CR1 milestone Sep 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support multitenancy with multiple persistence units #11772

Support multitenancy with multiple persistence units #11772

gsmet commented Sep 1, 2020

gsmet Sep 1, 2020

gavinking Sep 1, 2020

gsmet Sep 1, 2020

michael-schnell Sep 6, 2020

gsmet Sep 1, 2020

geoand Sep 1, 2020 •

edited

gsmet Sep 1, 2020

geoand Sep 1, 2020

gsmet Sep 1, 2020

gsmet Sep 1, 2020

geoand Sep 1, 2020

geoand commented Sep 1, 2020

gavinking commented Sep 1, 2020 •

edited

gsmet commented Sep 1, 2020

gavinking commented Sep 1, 2020

geoand left a comment

gsmet commented Sep 2, 2020

michael-schnell commented Sep 6, 2020

michael-schnell commented Sep 6, 2020

gavinking commented Sep 6, 2020

Support multitenancy with multiple persistence units #11772

Support multitenancy with multiple persistence units #11772

Conversation

gsmet commented Sep 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoand Sep 1, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoand commented Sep 1, 2020

gavinking commented Sep 1, 2020 • edited

gsmet commented Sep 1, 2020

gavinking commented Sep 1, 2020

geoand left a comment

Choose a reason for hiding this comment

gsmet commented Sep 2, 2020

michael-schnell commented Sep 6, 2020

michael-schnell commented Sep 6, 2020

gavinking commented Sep 6, 2020

geoand Sep 1, 2020 •

edited

gavinking commented Sep 1, 2020 •

edited