Fix #13425 Hibernate ORM and Hibernate Reactive cannot be used in the same application #44473

lucamolteni · 2024-11-13T14:34:45Z

I've added a test to verify the compatibility of both the extensions together.

This is not a big refactor as both extensions are still coupled together, but this lets users use both implementation in their Quarkus application.

The idea is to have a MultiplePersistenceProviderResolver https://github.com/quarkusio/quarkus/compare/main...lucamolteni:13425-ter?expand=1#diff-c1c6c402fc142de3b1c4d726cb080f2b856cf560b693a1686f52ad3a35a972f0R10

And filter the correct one at runtime

https://github.com/quarkusio/quarkus/compare/main...lucamolteni:13425-ter?expand=1#diff-db8c5c1b299098a43c5d8e4b3f3267e4bb72c6e7841536d9bbb487dcff8c54ffR88

There's also a new ReactivePersistenceUnit annotation https://github.com/quarkusio/quarkus/compare/main...lucamolteni:13425-ter?expand=1#diff-8c10ee8583155144d5c9f796e33b785471701a346c768b2f707fe3a036f0590dR24 in order to disambiguate the correct bean to be injected.

I've also changed the default name for the reactive persistence unit as there are effectively two together and they can't share the same name.

Connection strings are in two different fields so they don't collide
https://github.com/quarkusio/quarkus/compare/main...lucamolteni:13425-ter?expand=1#diff-6dd4fdea2c39574c084d9b03d106b6dd15b4877db728858cebf88641216bcd72R13

Feedbacks are welcome

quarkus-bot · 2024-11-13T14:34:49Z

Thanks for your pull request!

Your pull request does not follow our editorial rules. Could you have a look?

title should not contain an issue number (use Fix #1234 in the description instead)

_{This message is automatically generated by a bot.}

quarkus-bot · 2024-11-13T14:35:08Z

/cc @gsmet (hibernate-orm), @yrodiere (hibernate-orm)

yrodiere · 2024-11-13T14:55:47Z

Thanks!

I'll have a closer look tomorrow, but a few early comments:

There's also a new ReactivePersistenceUnit annotation https://github.com/quarkusio/quarkus/compare/main...lucamolteni:13425-ter?expand=1#diff-8c10ee8583155144d5c9f796e33b785471701a346c768b2f707fe3a036f0590dR24 in order to disambiguate the correct bean to be injected.

Can this be more "hidden"? E.g. move the annotation to a clearly internal package, or even just don't make it a bean for Hibernate Reactive.

The reason I'm asking is that users really shouldn't be using the EntityManagerFactory for Hibernate Reactive, and thus @ReactivePersistenceUnit really has no use except internally, which could be confusing.

I've also changed the default name for the reactive persistence unit as there are effectively two together and they can't share the same name.

A few things:

This is just for the EntityManagerFactory bean, right? So the problem would go away if the EntityManagerFactory for Hibernate Reactive was not a CDI bean?
How will this work once we support named persistence units in the Hibernate Reactive extension?
Wasn't that persistence unit already using a different name? I've seen <default-reactive> somewhere before...

lucamolteni · 2024-11-13T15:01:11Z

I'll have a closer look tomorrow, but a few early comments:

There's also a new ReactivePersistenceUnit annotation https://github.com/quarkusio/quarkus/compare/main...lucamolteni:13425-ter?expand=1#diff-8c10ee8583155144d5c9f796e33b785471701a346c768b2f707fe3a036f0590dR24 in order to disambiguate the correct bean to be injected.

Can this be more "hidden"? E.g. move the annotation to a clearly internal package, or even just don't make it a bean for Hibernate Reactive.

The reason I'm asking is that users really shouldn't be using the EntityManagerFactory for Hibernate Reactive, and thus @ReactivePersistenceUnit really has no use except internally, which could be confusing.

I agree with your point, I'll work on it and see if I can hide it somehow. But yes it's just a matter of CDI injection (answering also 1 below)

I've also changed the default name for the reactive persistence unit as there are effectively two together and they can't share the same name.

A few things:

This is just for the EntityManagerFactory bean, right? So the problem would go away if the EntityManagerFactory for Hibernate Reactive was not a CDI bean?

How will this work once we support named persistence units in the Hibernate Reactive extension?

I don't see how this should prevent the multiple units to be implemented, it's something that has to be done from scratch anyway

Wasn't that persistence unit already using a different name? I've seen <default-reactive> somewhere before...

I saw that as well, but I also saw the original persistence unit default string hardcoded and that was a problem, so I thought that keeping it separated would be better.

FroMage · 2024-11-13T15:24:00Z

Impressive. I've no idea how to support this in Panache, though 😱

yrodiere

Well I must say I didn't expect this to be doable with so few changes... impressive indeed. Though I guess we need to see where test failures are coming from :)

I also added a few comments below.

yrodiere · 2024-11-21T10:50:12Z

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

+        } else if (capabilities.isPresent(Capability.HIBERNATE_REACTIVE)) {
+            // We have reactive, but we have also defined a blocking datasource
+            return ImpliedBlockingPersistenceUnitTypeBuildItem.generateImpliedPersistenceUnit();


This is exactly the same code as the else block... probably not useful?

Regardless, I'm wondering if this whole code is still useful. During earlier discussions, the idea was that a given persistence unit would have a blocking variant if the associated datasource has its JDBC variant enabled, and a non-blocking variant if the associated datasource has its reactive variant enabled.

Applying this logic to the default persistence unit probably gives us the same behavior as this code... or an even more correct one, as it wouldn't depend on unrelated datasources?

...ns/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/ReactivePersistenceUnit.java

...nt/src/main/java/io/quarkus/hibernate/orm/deployment/PersistenceUnitDescriptorBuildItem.java

...yment/src/main/java/io/quarkus/hibernate/reactive/deployment/HibernateReactiveProcessor.java

integration-tests/hibernate-reactive-orm-compatibility/pom.xml

...src/main/java/io/quarkus/it/hibernate/reactive/postgresql/HibernateReactiveTestEndpoint.java

...gration-tests/hibernate-reactive-orm-compatibility/src/main/resources/application.properties

github-actions · 2024-11-21T15:08:24Z

🎊 PR Preview 4c8e860 has been successfully built and deployed to https://quarkus-pr-main-44473-preview.surge.sh/version/main/guides/

Images of blog posts older than 3 months are not available.
Newsletters older than 3 months are not available.

extensions/hibernate-reactive/deployment/pom.xml

...ons/hibernate-reactive/deployment/src/test/java/io/quarkus/hibernate/reactive/NoJtaTest.java

...st/java/io/quarkus/hibernate/reactive/sql_load_script/FileNotFoundSqlLoadScriptTestCase.java

lucamolteni · 2024-11-22T16:36:10Z

@yrodiere I removed the annotation, I had to ignore a ClassNameTest because the constant is based on a class that is not present in the -orm extension. Of course this test needs to be reenabled, but in the meanwhile take a look at this way to inject the Mutiny.SessionFactory bean instead

yrodiere

Nice cleanup; I think removing ReactiveSessionFactoryProducer definitely is a good move.

I see this is still WIP (failing tests?) though, so I'll let you work now :]

...bernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/deployment/ClassNamesTest.java

...nt/src/main/java/io/quarkus/hibernate/reactive/deployment/HibernateReactiveCdiProcessor.java

yrodiere · 2024-11-27T08:53:03Z

...nt/src/main/java/io/quarkus/hibernate/reactive/deployment/HibernateReactiveCdiProcessor.java

+
+        for (PersistenceUnitDescriptorBuildItem persistenceUnitDescriptor : persistenceUnitDescriptors) {
+            String persistenceUnitName = persistenceUnitDescriptor.getPersistenceUnitName();
+            String persistenceUnitConfigName = persistenceUnitDescriptor.getConfigurationName();


With your changes, the concept of "configuration name" might not be relevant at all anymore... If so we'll need to remember to clean this up and remove getConfigurationName() and related code from the codebase.

integration-tests/hibernate-reactive-orm-compatibility/pom.xml

…cation quarkusio#13425 quarkusio#13425

…ave a JDBC data source we don't generate the blocking pU

…metadata

…ionFactory bean in the extension itself

…U as well

…ngPersistenceUnitType in HibernateReactiveCdiProcessor

…active PU as well" This reverts commit 9109611.

* Added reactive configuration key to DataSourceBuildTimeConfig (not sure if it's a good idea) * Removed quarkus-jdbc-h2-deployment as the reactive tests are based on postgres * Postgres quarkus-jdbc-postgresql-deployment which triggers agroal is included with forcedDependencies to not include in every reactive test * Tests are included for default data source / PU, named Datasource and named PU

…ata source supports reactive to avoid failing while using reactive and blocking together

Fix ConfigActiveFalseAndEntityTest.mutinySessionFactory

…suite)

quarkus-bot bot added area/hibernate-orm Hibernate ORM area/hibernate-reactive Hibernate Reactive labels Nov 13, 2024

lucamolteni changed the title ~~Hibernate ORM and Hibernate Reactive cannot be used in the same application #13425~~ Fix #13425 iHibernate ORM and Hibernate Reactive cannot be used in the same application Nov 13, 2024

lucamolteni changed the title ~~Fix #13425 iHibernate ORM and Hibernate Reactive cannot be used in the same application~~ Fix #13425 Hibernate ORM and Hibernate Reactive cannot be used in the same application Nov 13, 2024