aldettinger commented on PR #9414: URL: https://github.com/apache/camel-quarkus/pull/9414#issuecomment-6528024317
Reviewed this one — **no correctness bugs found**. Notes below in case they're useful. I checked the mechanics against the actual Quarkus 3.33.4 bytecode rather than assuming: - `quarkus.hibernate-orm."pu".datasource=<default>` resolves correctly: `HibernateDataSourceUtil.findDataSourceWithNameDefault` matches by string equality on `JdbcDataSourceBuildItem.getName()`, and Agroal names the default datasource `<default>`. - No default persistence unit gets created by the now-always-present default datasource. `HibernateOrmProcessor.handleHibernateORMWithNoPersistenceXml` computes `createDefaultPU = (defaultDataSourcePresent && namedPUs.isEmpty()) || defaultPU.isAnyPropertySet()`, and named PUs are non-empty in every profile — so `JpaSingleNamedResourceWithNoDefaultTest` / `JpaMultipleNamedResourcesWithNoDefaultTest` still see no `@Default` EMF and their 200/500 expectations hold. The "entities defined but datasource cannot be found" guard doesn't trip either, since all entity packages are assigned to named PUs. - `none` is a valid `schema-management.strategy` (`HibernateGenerationStrategy.NONE`) and the property is runtime config, so the `multiB` override applies. - Quarkus imposes no restriction on two PUs sharing one datasource. - `JpaProcessor.registerPersistenceUnitCamelBeanQualifierResolver` keys off `persistenceUnitDescriptors.size()`, not datasource names, so it's unaffected. There's no surefire `forkCount` or JUnit parallel config in this module, so the now-shared database isn't accessed concurrently. Two minor notes, neither blocking: 1. The equivalent commit on `main` (`eecf8fdf81d`, which this is a byte-identical backport of) has never actually run in CI: `integration-tests/jpa/pom.xml` on `main` sets `<skipTests>true</skipTests>` and `<quarkus.build.skip>true</quarkus.build.skip>` pending #9357. So 3.33.x is the first branch where this config is exercised — worth confirming a green run on the jpa module before merging, ideally also with `-Dcq.sqlJdbcKind=postgresql` since that's the container path this targets. 2. After this change no test in the repo binds a persistence unit to a non-default *named* datasource, so `quarkus.hibernate-orm."pu".datasource=<named-ds>` resolution is no longer covered anywhere (`0745f6390e5` had deliberately set those names up). The lost coverage is of Quarkus rather than Camel Quarkus code — the extension keys off PU names only — so the trade against one container instead of five looks reasonable. Just noting it's a trade. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
