lucamolteni commented 1 week ago

Fixes #13425

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[bot] commented 1 week ago

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[bot] commented 1 week ago

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

yrodiere commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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

quarkus-bot[bot] commented 1 week ago

:waning_crescent_moon: This workflow status is outdated as a new workflow run has been triggered.

Status for workflow `Quarkus CI`

This is the status report for running Quarkus CI on commit e6702807a6efb54ae8b23817a7a921e3f10cbd24.

[!WARNING] Unable to include the stracktraces as the report was too long. See annotations below for the details.

Failing Jobs

Status	Name	Step	Failures	Logs	Raw logs	Build scan
✖	JVM Tests - JDK 17	`Build`	Failures	Logs	Raw logs	:mag:
✖	JVM Tests - JDK 21	`Build`	Failures	Logs	Raw logs	:mag:
✖	JVM Tests - JDK 17 Windows	`Build`	Failures	Logs	Raw logs	:mag:
✖	Gradle Tests - JDK 17	`Build`	Failures	Logs	Raw logs	:mag:
✖	Gradle Tests - JDK 17 Windows	`Build`	Failures	Logs	Raw logs	:construction:
✖	Quickstarts Compilation - JDK 17	`Compile Quickstarts`	Failures	Logs	Raw logs	:construction:
✖	Native Tests - Cache	`Build`	Failures	Logs	Raw logs	:mag:
✖	Native Tests - Data5	`Build`	Failures	Logs	Raw logs	:mag:
✖	Native Tests - Data7	`Build`	Failures	Logs	Raw logs	:mag:
✖	Native Tests - Security3	`Build`	Failures	Logs	Raw logs	:mag:

Full information is available in the Build summary check run. You can consult the Develocity build scans.

Failures

:gear: JVM Tests - JDK 17 #

- Failing: extensions/hibernate-reactive/deployment integration-tests/hibernate-search-orm-opensearch integration-tests/jpa-mysql and 1 more
! Skipped: extensions/panache/hibernate-reactive-panache-common/deployment extensions/panache/hibernate-reactive-panache-kotlin/deployment extensions/panache/hibernate-reactive-panache/deployment and 22 more

:package: extensions/hibernate-reactive/deployment

✖ io.quarkus.hibernate.reactive.MultiLineImportTest. - History - More details - Source on GitHub

✖ io.quarkus.hibernate.reactive.NoJtaTest. - History - More details - Source on GitHub

✖ io.quarkus.hibernate.reactive.SchemaUpdateTest. - History - More details - Source on GitHub

✖ io.quarkus.hibernate.reactive.SchemaValidateTest. line 40 - History - More details - Source on GitHub

✖ io.quarkus.hibernate.reactive.config.ConfigActiveFalseAndEntityTest. - History - More details - Source on GitHub

✖ io.quarkus.hibernate.reactive.config.datasource.EntitiesInDefaultPUWithExplicitDatasourceConfigActiveFalseTest. - History - More details - Source on GitHub

✖ io.quarkus.hibernate.reactive.config.datasource.EntitiesInDefaultPUWithExplicitDatasourceConfigUrlMissingTest. - History - More details - Source on GitHub

✖ io.quarkus.hibernate.reactive.config.datasource.EntitiesInDefaultPUWithExplicitDatasourceMissingTest. - History - More details - Source on GitHub

✖ io.quarkus.hibernate.reactive.config.datasource.EntitiesInDefaultPUWithImplicitDatasourceConfigActiveFalseTest. - History - More details - Source on GitHub

✖ io.quarkus.hibernate.reactive.config.unsupportedproperties.UnsupportedPropertiesTest. line 61 - History - More details - Source on GitHub

✖ io.quarkus.hibernate.reactive.context.ContextValidationTest.testListAllFruits - History - More details - Source on GitHub

✖ io.quarkus.hibernate.reactive.dev.HibernateReactiveDevModeTest.testListAllFruits - History - More details - Source on GitHub

✖ io.quarkus.hibernate.reactive.mapping.id.optimizer.optimizer.IdOptimizerDefaultDefaultTest. - History - More details - Source on GitHub

✖ io.quarkus.hibernate.reactive.mapping.id.optimizer.optimizer.IdOptimizerDefaultNoneTest. - History - More details - Source on GitHub

✖ io.quarkus.hibernate.reactive.mapping.id.optimizer.optimizer.IdOptimizerDefaultPooledLoTest. - History - More details - Source on GitHub

✖ io.quarkus.hibernate.reactive.mapping.id.optimizer.optimizer.IdOptimizerDefaultPooledTest. - History - More details - Source on GitHub

✖ io.quarkus.hibernate.reactive.mapping.timezone.TimezoneDefaultStorageAutoTest. - History - More details - Source on GitHub

✖ io.quarkus.hibernate.reactive.mapping.timezone.TimezoneDefaultStorageColumnTest. - History - More details - Source on GitHub

✖ io.quarkus.hibernate.reactive.mapping.timezone.TimezoneDefaultStorageDefaultTest. - History - More details - Source on GitHub

✖ io.quarkus.hibernate.reactive.mapping.timezone.TimezoneDefaultStorageNormalizeTest. - History - More details - Source on GitHub

✖ io.quarkus.hibernate.reactive.mapping.timezone.TimezoneDefaultStorageNormalizeUtcTest. - History - More details - Source on GitHub

✖ io.quarkus.hibernate.reactive.publicfields.PublicFieldAccessFieldTypesTest. - History - More details - Source on GitHub

✖ io.quarkus.hibernate.reactive.publicfields.PublicFieldAccessInheritanceTest. - History - More details - Source on GitHub

✖ io.quarkus.hibernate.reactive.singlepersistenceunit.SinglePersistenceUnitCdiMutinySessionTest. - History - More details - Source on GitHub

✖ io.quarkus.hibernate.reactive.singlepersistenceunit.SinglePersistenceUnitPackageAnnotationTest. - History - More details - Source on GitHub

✖ io.quarkus.hibernate.reactive.singlepersistenceunit.SinglePersistenceUnitPackageConfigurationTest. line 39 - History - More details - Source on GitHub

✖ io.quarkus.hibernate.reactive.sql_load_script.FileNotFoundSqlLoadScriptTestCase. line 24 - History - More details - Source on GitHub

:package: integration-tests/hibernate-search-orm-opensearch

✖ Failed to execute goal io.fabric8:docker-maven-plugin:0.45.1:start (docker-start) on project quarkus-integration-test-hibernate-search-orm-opensearch: I/O Error

:package: integration-tests/jpa-mysql

✖ Failed to execute goal io.fabric8:docker-maven-plugin:0.45.1:build (docker-start) on project quarkus-integration-test-jpa-mysql: Unable to pull 'docker.io/mysql:8.4' from registry 'docker.io' : {"message":"Head \"https://registry-1.docker.io/v2/library/mysql/manifests/8.4\": Get \"https://auth.docker.io/token?account=githubactions&scope=repository%3Alibrary%2Fmysql%3Apull&service=registry.docker.io\": context deadline exceeded (Client.Timeout exceeded while awaiting headers)"} (Internal Server Error: 500)

:package: integration-tests/jpa-oracle

✖ Failed to execute goal io.fabric8:docker-maven-plugin:0.45.1:start (docker-start) on project quarkus-integration-test-jpa-oracle: I/O Error

:gear: JVM Tests - JDK 21 #

- Failing: extensions/hibernate-reactive/deployment 
! Skipped: extensions/panache/hibernate-reactive-panache-common/deployment extensions/panache/hibernate-reactive-panache-kotlin/deployment extensions/panache/hibernate-reactive-panache/deployment and 22 more

:package: extensions/hibernate-reactive/deployment

✖ io.quarkus.hibernate.reactive.MultiLineImportTest. - History - More details - Source on GitHub

✖ io.quarkus.hibernate.reactive.NoJtaTest. - History - More details - Source on GitHub

✖ io.quarkus.hibernate.reactive.SchemaUpdateTest. - History - More details - Source on GitHub

✖ io.quarkus.hibernate.reactive.SchemaValidateTest. line 40 - History - More details - Source on GitHub

✖ io.quarkus.hibernate.reactive.config.ConfigActiveFalseAndEntityTest. - History - More details - Source on GitHub

✖ io.quarkus.hibernate.reactive.config.datasource.EntitiesInDefaultPUWithExplicitDatasourceConfigUrlMissingTest. - History - More details - Source on GitHub

✖ io.quarkus.hibernate.reactive.config.datasource.EntitiesInDefaultPUWithImplicitDatasourceConfigActiveFalseTest. - History - More details - Source on GitHub

✖ io.quarkus.hibernate.reactive.config.datasource.EntitiesInDefaultPUWithImplicitDatasourceConfigUrlMissingTest. - History - More details - Source on GitHub