Hibernate Reactive: don't make transaction-related beans as unremovable and fail on unconfigured datasource

[mkouba]

No description provided.

[quarkus-bot]

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

[mkouba]

CC @FroMage

[yrodiere]

Thanks, but the diff would have been much easier to read if you didn't move the method around xD I don't know why we even have a dedicated processor class for CDI stuff anyway, we don't do that for anything else.

So for others wondering, the key is the addition of capabilities.isMissing(Capability.HIBERNATE_REACTIVE)). I suppose you noticed a problem somewhere and discussed that on Zulip... ?

The other change about failing the build makes sense too since we don't support using Hibernate ORM + reactive in the same app (yet), but I wonder how you even got to that point 🤔

Regardless... +1 from me but I think there's a mistake, see below.

[mkouba]

Thanks, but the diff would have been much easier to read if you didn't move the method around xD I don't know why we even have a dedicated processor class for CDI stuff anyway, we don't do that for anything else.

Well, I moved the build step because it seems to be the only one related to CDI but placed in the HibernateOrmProcessor. Anyway, I think that it makes sense to have a dedicated build step "container" to group the steps related to a specific technology/topic. It's definitely better than navigating a huge class...

So for others wondering, the key is the addition of capabilities.isMissing(Capability.HIBERNATE_REACTIVE)). I suppose you noticed a problem somewhere and discussed that on Zulip... ?

Not a problem. I just noticed in the Dev UI of the HR panache quickstart that we register those beans for no reason...

The other change about failing the build makes sense too since we don't support using Hibernate ORM + reactive in the same app (yet), but I wonder how you even got to that point 🤔

Stef had a project with no PU configured... https://quarkusio.zulipchat.com/#narrow/stream/187038-dev/topic/hibernate.20reactive/near/423364478

Regardless... +1 from me but I think there's a mistake, see below.

[yrodiere]

The other change about failing the build makes sense too since we don't support using Hibernate ORM + reactive in the same app (yet), but I wonder how you even got to that point 🤔

Stef had a project with no PU configured... https://quarkusio.zulipchat.com/#narrow/stream/187038-dev/topic/hibernate.20reactive/near/423364478

Wait, you mean with no entity? That's actually allowed, and we have a test to check it remains that way: io.quarkus.hibernate.reactive.NoEntitiesTest.

Is it wise? Not if you ask me.

Do we have a general expectations for extensions to not break an application if they are added to the build but are pointless? Yes we do...

I'd mention @gsmet but I think I can probably sum up his answer with "it's been decided that way for good reasons, and if we want to change it we need a team discussion".

So... I think I'd revert that part of your changes for now.

[mkouba]

The other change about failing the build makes sense too since we don't support using Hibernate ORM + reactive in the same app (yet), but I wonder how you even got to that point 🤔

Stef had a project with no PU configured... https://quarkusio.zulipchat.com/#narrow/stream/187038-dev/topic/hibernate.20reactive/near/423364478

Wait, you mean with no entity? That's actually allowed, and we have a test to check it remains that way: io.quarkus.hibernate.reactive.NoEntitiesTest.

@FroMage's use case was a bit different - he had an entity, but forgot to add datasource or something like that... basically empty application.properties .

Is it wise? Not if you ask me.

Do we have a general expectations for extensions to not break an application if they are added to the build but are pointless? Yes we do...

I'd mention @gsmet but I think I can probably sum up his answer with "it's been decided that way for good reasons, and if we want to change it we need a team discussion".

So... I think I'd revert that part of your changes for now.

Hm, I do understand but I'm not so sure it's the same type of problem. Maybe we should only fail the build if some entities are present?

[yrodiere]

Hm, I do understand but I'm not so sure it's the same type of problem. Maybe we should only fail the build if some entities are present?

You mean if there are entities, but no datasource? Yes we do fail in that case with the Hibernate ORM extension, so we should for Reactive too. I think we'll want to make this code in the HR extension closer to the Hibernate ORM extension:

quarkus/extensions/hibernate-reactive/deployment/src/main/java/io/quarkus/hibernate/reactive/deployment/HibernateReactiveProcessor.java

Lines 162 to 196 in 8abb590

	// we only support the default pool for now
	DataSourceBuildTimeConfig defaultDataSourceBuildTimeConfig = dataSourcesBuildTimeConfig.dataSources()
	.get(DataSourceUtil.DEFAULT_DATASOURCE_NAME);
	Optional<String> explicitDialect = hibernateOrmConfig.defaultPersistenceUnit().dialect().dialect();
	Optional<String> explicitDbMinVersion = defaultDataSourceBuildTimeConfig.dbVersion();
	Optional<String> dbKindOptional = DefaultDataSourceDbKindBuildItem.resolve(
	defaultDataSourceBuildTimeConfig.dbKind(),
	defaultDataSourceDbKindBuildItems,
	defaultDataSourceBuildTimeConfig.devservices().enabled()
	.orElse(!dataSourcesBuildTimeConfig.hasNamedDataSources()),
	curateOutcomeBuildItem);
	if (dbKindOptional.isPresent()) {
	HibernateOrmConfigPersistenceUnit persistenceUnitConfig = hibernateOrmConfig.defaultPersistenceUnit();
	ParsedPersistenceXmlDescriptor reactivePU = generateReactivePersistenceUnit(
	hibernateOrmConfig, index, persistenceUnitConfig, jpaModel,
	dbKindOptional, explicitDialect, explicitDbMinVersion, applicationArchivesBuildItem,
	launchMode.getLaunchMode(),
	systemProperties, nativeImageResources, hotDeploymentWatchedFiles, dbKindDialectBuildItems);
	//Some constant arguments to the following method:
	// - this is Reactive
	// - we don't support starting Hibernate Reactive from a persistence.xml
	// - we don't support Hibernate Envers with Hibernate Reactive
	persistenceUnitDescriptors.produce(new PersistenceUnitDescriptorBuildItem(reactivePU,
	DEFAULT_PERSISTENCE_UNIT_NAME,
	new RecordedConfig(Optional.of(DataSourceUtil.DEFAULT_DATASOURCE_NAME),
	dbKindOptional, Optional.empty(),
	io.quarkus.hibernate.orm.runtime.migration.MultiTenancyStrategy.NONE,
	hibernateOrmConfig.database().ormCompatibilityVersion(),
	persistenceUnitConfig.unsupportedProperties()),
	null,
	jpaModel.getXmlMappings(reactivePU.getName()),
	true, false, capabilities));

Here's the (better) reference in the Hibernate ORM extension:

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

Lines 1128 to 1204 in 3b35908

	private static void collectDialectConfig(String persistenceUnitName,
	HibernateOrmConfigPersistenceUnit persistenceUnitConfig,
	List<DatabaseKindDialectBuildItem> dbKindMetadataBuildItems, Optional<JdbcDataSourceBuildItem> jdbcDataSource,
	MultiTenancyStrategy multiTenancyStrategy,
	BuildProducer<SystemPropertyBuildItem> systemProperties,
	BiConsumer<String, String> puPropertiesCollector, Set<String> storageEngineCollector) {
	Optional<String> explicitDialect = persistenceUnitConfig.dialect().dialect();
	Optional<String> dbKind = jdbcDataSource.map(JdbcDataSourceBuildItem::getDbKind);
	Optional<String> explicitDbMinVersion = jdbcDataSource.flatMap(JdbcDataSourceBuildItem::getDbVersion);
	if (multiTenancyStrategy != MultiTenancyStrategy.DATABASE && jdbcDataSource.isEmpty()) {
	throw new ConfigurationException(String.format(Locale.ROOT,
	"Datasource must be defined for persistence unit '%s'."
	+ " Refer to https://quarkus.io/guides/datasource for guidance.",
	persistenceUnitName),
	new HashSet<>(Arrays.asList("quarkus.datasource.db-kind", "quarkus.datasource.username",
	"quarkus.datasource.password", "quarkus.datasource.jdbc.url")));
	}
	Optional<String> dialect = explicitDialect;
	Optional<String> dbProductVersion = explicitDbMinVersion;
	if (dbKind.isPresent() || explicitDialect.isPresent()) {
	for (DatabaseKindDialectBuildItem item : dbKindMetadataBuildItems) {
	if (dbKind.isPresent() && DatabaseKind.is(dbKind.get(), item.getDbKind())
	// Set the default version based on the dialect when we don't have a datasource
	// (i.e. for database multi-tenancy)
	|| explicitDialect.isPresent() && explicitDialect.get().equals(item.getDialect())) {
	if (explicitDialect.isEmpty()) {
	dialect = Optional.of(item.getDialect());
	}
	if (explicitDbMinVersion.isEmpty()) {
	dbProductVersion = item.getDefaultDatabaseProductVersion();
	}
	break;
	}
	}
	if (dialect.isEmpty()) {
	throw new ConfigurationException(
	"The Hibernate ORM extension could not guess the dialect from the database kind '" + dbKind.get()
	+ "'. Add an explicit '"
	+ HibernateOrmRuntimeConfig.puPropertyKey(persistenceUnitName, "dialect")
	+ "' property.");
	}
	}
	if (dialect.isPresent()) {
	puPropertiesCollector.accept(AvailableSettings.DIALECT, dialect.get());
	} else {
	// We only get here with the database multi-tenancy strategy; see the initial check, up top.
	assert multiTenancyStrategy == MultiTenancyStrategy.DATABASE;
	throw new ConfigurationException(String.format(Locale.ROOT,
	"The Hibernate ORM extension could not infer the dialect for persistence unit '%s'."
	+ " When using database multi-tenancy, you must either configure a datasource for that persistence unit"
	+ " (refer to https://quarkus.io/guides/datasource for guidance),"
	+ " or set the dialect explicitly through property '"
	+ HibernateOrmRuntimeConfig.puPropertyKey(persistenceUnitName, "dialect") + "'.",
	persistenceUnitName));
	}
	if (persistenceUnitConfig.dialect().storageEngine().isPresent()) {
	// Only actually set the storage engines if MySQL or MariaDB
	if (isMySQLOrMariaDB(dialect.get())) {
	// The storage engine has to be set as a system property.
	// We record it so that we can later run checks (because we can only set a single value)
	storageEngineCollector.add(persistenceUnitConfig.dialect().storageEngine().get());
	systemProperties.produce(new SystemPropertyBuildItem(AvailableSettings.STORAGE_ENGINE,
	persistenceUnitConfig.dialect().storageEngine().get()));
	} else {
	LOG.warnf("The storage engine set through configuration property '%1$s' is being ignored"
	+ " because the database is neither MySQL nor MariaDB.",
	HibernateOrmRuntimeConfig.puPropertyKey(persistenceUnitName, "dialect.storage-engine"));
	}
	}
	if (dbProductVersion.isPresent()) {
	puPropertiesCollector.accept(AvailableSettings.JAKARTA_HBM2DDL_DB_VERSION, dbProductVersion.get());
	}
	}

Not sure it'll be easy though; it might require some hacking. With the Hibernate ORM extension we have access to the full list of datasources configured in Agroal, but I'm not sure the Hibernate Reactive extension has access to similar information for reactive clients, which are a bit more heterogeneous.

[mkouba]

@yrodiere This test is weird: io.quarkus.hibernate.reactive.config.datasource.EntitiesInDefaultPUWithExplicitUnconfiguredDatasourceTest. There's an entity and the configuration is invalid and we still expect no failure.

[yrodiere]

@mkouba Yes that's just a non-regression test, added to check the behavior doesn't change when I worked on an unrelated feature.
I'm totally with you on the fact the behavior is garbage, though. We can definitely decide to change it to match the behavior expected for Hibernate ORM: https://github.com/quarkusio/quarkus/blob/989ce28f7c6cf9c345ad0cc9d329337d85a2ca01/extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/config/datasource/EntitiesInDefaultPUWithExplicitUnconfiguredDatasourceTest.java

[mkouba]

@yrodiere the io.quarkus.hibernate.reactive.config.datasource.EntitiesInDefaultPUWithImplicitUnconfiguredDatasourceTest still fails but TBH I have no idea if this test makes any sense...

[FroMage]

Well, I've added questions, but perhaps that's just because I don't understand how DEV services affect configuration and I'm misguided. I do share @yrodiere 's remarks, though, so if he says we're good, I'll follow :)

[mkouba]

@yrodiere the io.quarkus.hibernate.reactive.config.datasource.EntitiesInDefaultPUWithImplicitUnconfiguredDatasourceTest still fails but TBH I have no idea if this test makes any sense...

I've aligned the EntitiesInDefaultPUWithImplicitUnconfiguredDatasourceTest with the ORM version, i.e. expect failure if dev services are disabled.

[mkouba]

Hm, according to the integration-tests/hibernate-reactive-mysql-agroal-flyway it's legal to include both HR and JTA/agroal in a quarkus app. So I've modified the HibernateOrmCdiProcessor to only register beans for org.hibernate.Session/org.hibernate.StatelessSession if HR is absent.

[quarkus-bot]

Status for workflow Quarkus CI

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

Failing Jobs

Status	Name	Step	Failures	Logs	Raw logs	Build scan
✖	Native Tests - Data5	Build	Failures	Logs	Raw logs	🚧

Full information is available in the Build summary check run.

Failures

⚙️ Native Tests - Data5 #

- Failing: integration-tests/jpa-postgresql-withxml 

📦 integration-tests/jpa-postgresql-withxml

✖ io.quarkus.it.jpa.postgresql.ImageMetricsITCase.verifyImageMetrics line 15 - History - More details - Source on GitHub

org.opentest4j.AssertionFailedError: Expected analysis_results.fields.reflection to be within range [163 +- 3%] but was 168 ==> expected: <true> but was: <false>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63)
	at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36)
	at org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:214)
	at io.quarkus.test.junit.nativeimage.NativeBuildOutputExtension.assertValueWithinRange(NativeBuildOutputExtension.java:90)
	at io.quarkus.test.junit.nativeimage.NativeBuildOutputExtension.lambda$verifyImageMetrics$0(NativeBuildOutputExtension.java:66)

---

Flaky tests - Develocity

⚙️ JVM Tests - JDK 21

📦 extensions/smallrye-reactive-messaging-kafka/deployment

✖ io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase.sseStream - History

• Assertion condition Expecting size of: [] to be greater than or equal to 2 but was 0 within 10 seconds. - org.awaitility.core.ConditionTimeoutException

org.awaitility.core.ConditionTimeoutException: 
Assertion condition 
Expecting size of:
  []
to be greater than or equal to 2 but was 0 within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)

---

⚙️ Native Tests - Misc4

📦 integration-tests/gradle

✖ io.quarkus.gradle.nativeimage.BasicJavaNativeBuildIT.shouldBuildNativeImageWithCustomName - History

• Gradle build failed with exit code 137 - java.lang.AssertionError

java.lang.AssertionError: Gradle build failed with exit code 137
	at io.quarkus.gradle.QuarkusGradleWrapperTestBase.runGradleWrapper(QuarkusGradleWrapperTestBase.java:140)
	at io.quarkus.gradle.QuarkusGradleWrapperTestBase.runGradleWrapper(QuarkusGradleWrapperTestBase.java:57)
	at io.quarkus.gradle.QuarkusGradleWrapperTestBase.runGradleWrapper(QuarkusGradleWrapperTestBase.java:52)
	at io.quarkus.gradle.nativeimage.QuarkusNativeGradleITBase.runGradleWrapper(QuarkusNativeGradleITBase.java:36)
	at io.quarkus.gradle.nativeimage.BasicJavaNativeBuildIT.shouldBuildNativeImageWithCustomName(BasicJavaNativeBuildIT.java:51)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

Will address in #39049

[yrodiere]

Merging, thanks. Will refine the solution in #39049.

[mkouba]

Merging, thanks. Will refine the solution in #39049.

Thanks @yrodiere!