Support multiple Hibernate ORM persistence units via Quarkus configuration #11322

gsmet · 2020-08-11T10:07:15Z

/cc @Sanne @machi1990 @michael-schnell

This is the preliminary infrastructure work for the support of multiple persistence units. I would like to merge this PR early (well, as soon as we agree the code is correct) and then do additional follow-ups for tweaking how we do the entity <-> persistence unit mapping and if we allow an entity to belong to several persistence units.

Fixes #2835

gsmet · 2020-08-18T10:23:10Z

@machi1990 I had to tweak the config a bit due to a SmallRye Config issue and I end up having another issue with the documentation:

[INFO] asciidoctor: WARN: ed/config/quarkus-hibernate-orm.adoc: line 147: id assigned to anchor already in use: quarkus-hibernate-orm_quarkus.hibernate-orm.dialect
[INFO] asciidoctor: WARN: ed/config/quarkus-hibernate-orm.adoc: line 455: id assigned to anchor already in use: quarkus-hibernate-orm_quarkus.hibernate-orm.-persistence-unit-name-.dialect
[INFO] Converted /data/home/gsmet/git/quarkus/docs/src/main/asciidoc/hibernate-orm.adoc
[ERROR] asciidoctor: WARN: ed/config/quarkus-hibernate-orm.adoc: line 147: id assigned to anchor already in use: quarkus-hibernate-orm_quarkus.hibernate-orm.dialect
[ERROR] asciidoctor: WARN: ed/config/quarkus-hibernate-orm.adoc: line 455: id assigned to anchor already in use: quarkus-hibernate-orm_quarkus.hibernate-orm.-persistence-unit-name-.dialect

Could you have a look?

gsmet · 2020-08-18T10:26:29Z

I added a commit that somehow makes it work (removing the @ConfigDocSection). You need to drop this commit to be able to reproduce.

machi1990 · 2020-08-18T10:35:47Z

I added a commit that somehow makes it work (removing the @ConfigDocSection). You need to drop this commit to be able to reproduce.

Okay, I'll check it out later today. Thanks for the update.

machi1990 · 2020-08-18T11:44:55Z

I added a commit that somehow makes it work (removing the @ConfigDocSection). You need to drop this commit to be able to reproduce.

Yeap. This was the issue as the anchor used for the dialect config section and the contained dialect config element was the same since the later inherited the parent name.

An easy fix will be to make the config section anchor unique enough by using the title for example. A more appropriate fix will be to construct the anchor separate from the configuration name e.g simply traversing the object and hyphenating the field name will be enough.

…rged properly

…section key

It has a special meaning and I don't want it to conflict with the user defined ones.

No code changes, just moving methods around.

In passing, we now make JTA the only case handled. I will throw a proper error in a follow-up commit when not enabled.

I decided to support multiple PUs there. We will see how it goes in Panache.

Sanne confirmed that not having JTA around is not a valid configuration. We used to have some partial support for it but it was buggy at best. The support was removed in a previous commit, this commit only adds a proper exception for it.

We don't have a prefix in the other extensions so let's try to be consistent and see how it goes.

In passing, reorganize the dialect configuration to work around an issue with SmallRye Config.

Sanne · 2020-08-20T16:19:39Z

@gsmet I checked in ORM and can confirm that hibernate.dialect.storage_engine is only applied to MySQL (and MariaDB as a consequence)

gsmet · 2020-08-20T16:21:41Z

OK, I will change the logic once this PR is in, then!

gavinking · 2020-08-20T16:49:57Z

@gsmet I checked in ORM and can confirm that hibernate.dialect.storage_engine is only applied to MySQL (and MariaDB as a consequence)

Isn't this kinda legacy? Does anyone really need to generate MyISAM schemas these days?

IMO this is pretty much deprecated functionality.

Sanne · 2020-08-20T17:32:37Z

Ironically this configuration flag was only added recently, "bolted on" as environment variable since dialects in 5 don't support configuration. There's still people preferring MyISAM, and there's other pluggable engines as well.

…

On Thu, 20 Aug 2020, 17:50 Gavin King, ***@***.***> wrote: @gsmet <https://github.com/gsmet> I checked in ORM and can confirm that hibernate.dialect.storage_engine is only applied to MySQL (and MariaDB as a consequence) Isn't this kinda legacy? Does anyone really need to generate MyISAM schemas these days? IMO this is pretty much deprecated functionality. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11322 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAKMTMHJJSP5OX5E4XW4QTSBVH4JANCNFSM4P23WQ7A> .

Sanne · 2020-08-20T18:58:26Z

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

@@ -666,6 +697,14 @@ private static void producePersistenceUnitDescriptorFromConfig(
        // we found one
        ParsedPersistenceXmlDescriptor descriptor = new ParsedPersistenceXmlDescriptor(null); //todo URL
        descriptor.setName(persistenceUnitName);
+
+        descriptor.setExcludeUnlistedClasses(true);


Just curious, did you figure that this was necessary?

It's not necessary as you forced it in LightPersistence... but... I'm not sure we will keep it that way so wanted to be extra sure it wouldn't break if we change it later.

Ok, thanks I was just puzzled.

Sanne · 2020-08-20T21:12:02Z

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

@@ -126,7 +126,7 @@

    public static final String HIBERNATE_ORM_CONFIG_PREFIX = "quarkus.hibernate-orm.";
    public static final String NO_SQL_LOAD_SCRIPT_FILE = "no-file";
-    public static final String DEFAULT_PERSISTENCE_UNIT_NAME = "default";
+    public static final String DEFAULT_PERSISTENCE_UNIT_NAME = "<default>";


What do you mean by "has special meaning", is it not possible for a user to speficy this name explicitly?

The idea was to avoid a conflict between the default one and a potential named PU called "default". Thus why I added the special characters. Also, it's consistent with the Agroal extension.

Sanne · 2020-08-20T21:15:10Z

...-orm/deployment/src/test/resources/application-multiple-persistence-units-invalid.properties

+quarkus.hibernate-orm.persistence-units."users".dialect=org.hibernate.dialect.H2Dialect
+quarkus.hibernate-orm.persistence-units."users".log.sql=true
+quarkus.hibernate-orm.persistence-units."users".database.generation=drop-and-create
+quarkus.hibernate-orm.persistence-units."users".datasource=users


Could you add a comment somewhere to remind why this is an invalid configuration? I guess because it's missing the package names filter packages?

Sanne · 2020-08-20T21:29:00Z

extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/JPAConfig.java

-        return jtaEnabled.get();
+    public EntityManagerFactory getEntityManagerFactory(String unitName) {
+        LazyPersistenceUnit lazyPersistenceUnit = null;
+        if (unitName == null) {


is this condition check useful? I was assuming that unitName would be always defined, at most <default>.

If I'm wrong, we might want to improve the following (nested) if so that the default can still be used/injected even when there's more than one PU defined.

It's always set except when you're explicitly calling null. This was the case somewhere in the code base so I decided to make it null safe just to be on the safe side.

Ok, it feels a bit smelly but we can clean it up later.

I would call it safe not smelly. I'm not sure I understand your problem.

Everything is working fine with the default injection, we have tests for that. The null test is there just if someone calls it programmatically. When dealing with injection, you pass the default unit name (which is not null).

For me passing null is not really valid, I only support it because someone used it (and I changed that call to not use it) and I thought maybe someone else could have.

Sanne

I'll approve as my minor comments and questions are certainly not blocking, but please have a look at them.

boring-cyborg bot added area/hibernate-orm Hibernate ORM area/core labels Aug 11, 2020

gsmet force-pushed the multi-persistence-units branch from 8143d0b to 0dc2c5a Compare August 13, 2020 16:27

gastaldi added the triage/needs-rebase This PR needs to be rebased first because it has merge conflicts label Aug 14, 2020

gsmet changed the title ~~Multi persistence units infrastructure (1/n)~~ Multiple persistence units infrastructure Aug 17, 2020

gsmet force-pushed the multi-persistence-units branch 2 times, most recently from 4d251f4 to 974f27f Compare August 18, 2020 10:20

gsmet marked this pull request as ready for review August 18, 2020 10:26

gsmet removed the triage/needs-rebase This PR needs to be rebased first because it has merge conflicts label Aug 18, 2020

gsmet added this to the 1.8.0 - master milestone Aug 18, 2020

gsmet mentioned this pull request Aug 18, 2020

Support multiple persistence units #2835

Closed

machi1990 and others added 15 commits August 18, 2020 18:03

fix: make sure that config section within the same config root are me…

d1dabdd

…rged properly

fix: generate unique anchors for ConfigSection fom section title and …

f9894aa

…section key

Enable scanning in JPA tests as we enforce it now

74514b0

Prepare configuration for multiple persistence units support

4a03d47

Support defining the datasource used by a persistence unit

bc9096d

Add config for multiple persistence units

6773605

Rename the default persistence unit to "<default>"

3154795

It has a special meaning and I don't want it to conflict with the user defined ones.

Make archives containing entity packages part of the Jandex index

0810c07

Reorganize HibernateOrmProcessor a bit

4f3362f

No code changes, just moving methods around.

Introduce the CDI infrastructure for multi-persistence units

1fbaefb

In passing, we now make JTA the only case handled. I will throw a proper error in a follow-up commit when not enabled.

Make JPAConfig less brittle and also publish the entity -> PUs mapping

51c87b8

I decided to support multiple PUs there. We will see how it goes in Panache.

Move PersistenceUnitUtil to the runtime module

40a3711

Throw an exception if no JTA

6faaa73

Sanne confirmed that not having JTA around is not a valid configuration. We used to have some partial support for it but it was buggy at best. The support was removed in a previous commit, this commit only adds a proper exception for it.

Simplify multiple persistence units configuration

f3f7181

We don't have a prefix in the other extensions so let's try to be consistent and see how it goes.

Throw an exception if storage engine configuration is not consistent

e79df93

In passing, reorganize the dialect configuration to work around an issue with SmallRye Config.

gsmet force-pushed the multi-persistence-units branch from e729b01 to e79df93 Compare August 18, 2020 16:26

Sanne reviewed Aug 20, 2020

View reviewed changes

Sanne approved these changes Aug 20, 2020

View reviewed changes

gsmet merged commit 9cd6e1b into quarkusio:master Aug 21, 2020

machi1990 mentioned this pull request Aug 21, 2020

Support of multiple persistence units for Hibernate Panache #8695

Merged

gsmet changed the title ~~Multiple persistence units infrastructure~~ Support multiple Hibernate ORM persistence units via Quarkus configuration Sep 2, 2020

gsmet added the release/noteworthy-feature label Sep 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support multiple Hibernate ORM persistence units via Quarkus configuration #11322

Support multiple Hibernate ORM persistence units via Quarkus configuration #11322

gsmet commented Aug 11, 2020 •

edited

gsmet commented Aug 18, 2020

gsmet commented Aug 18, 2020

machi1990 commented Aug 18, 2020 •

edited

machi1990 commented Aug 18, 2020

Sanne commented Aug 20, 2020

gsmet commented Aug 20, 2020

gavinking commented Aug 20, 2020

Sanne commented Aug 20, 2020 via email

Sanne Aug 20, 2020

gsmet Aug 21, 2020

Sanne Aug 21, 2020

Sanne Aug 20, 2020

gsmet Aug 20, 2020

Sanne Aug 20, 2020 •

edited

gsmet Aug 20, 2020

Sanne Aug 20, 2020

gsmet Aug 20, 2020

Sanne Aug 21, 2020

gsmet Aug 21, 2020

Sanne left a comment

Support multiple Hibernate ORM persistence units via Quarkus configuration #11322

Support multiple Hibernate ORM persistence units via Quarkus configuration #11322

Conversation

gsmet commented Aug 11, 2020 • edited

gsmet commented Aug 18, 2020

gsmet commented Aug 18, 2020

machi1990 commented Aug 18, 2020 • edited

machi1990 commented Aug 18, 2020

Sanne commented Aug 20, 2020

gsmet commented Aug 20, 2020

gavinking commented Aug 20, 2020

Sanne commented Aug 20, 2020 via email

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

Sanne Aug 20, 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

Sanne left a comment

Choose a reason for hiding this comment

gsmet commented Aug 11, 2020 •

edited

machi1990 commented Aug 18, 2020 •

edited

Sanne Aug 20, 2020 •

edited