Introduce validation mode instead of a boolean flag for BV integration in ORM

[marko-bekhta]

fixes #46034

I'm not sure about the property name itself ... I went with plural quarkus.hibernate-orm.validation.modes since we can accept multiple values there... but maybe it should be singular instead quarkus.hibernate-orm.validation.mode?

As for the migration guide, I'd add something along the lines:

Bean Validation integration changes its default behaviour and now considers validation constraints while generating DDL. 
To preserve the previous behaviour, use `quarkus.hibernate-orm.validation.mode=CALLBACK`. 

`quarkus.hibernate-orm.validation.enabled` is deprecated. To disable the Bean Validation integration, set `quarkus.hibernate-orm.validation.mode=NONE` instead.

as to:

Also we'd need to see what's different in Hibernate Reactive...

.... we do not set the property in the reactive processor so it is defaulted to null which in turn behaves as AUTO so both CALLBACK and DDL work:

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

Lines 251 to 428 in 1bdd69f

	private static QuarkusPersistenceUnitDescriptor generateReactivePersistenceUnit(
	HibernateOrmConfig hibernateOrmConfig, CombinedIndexBuildItem index,
	HibernateOrmConfigPersistenceUnit persistenceUnitConfig,
	JpaModelBuildItem jpaModel,
	Optional<String> dbKindOptional,
	Optional<String> explicitDialect,
	Optional<String> explicitDbMinVersion,
	ApplicationArchivesBuildItem applicationArchivesBuildItem,
	LaunchMode launchMode,
	BuildProducer<SystemPropertyBuildItem> systemProperties,
	BuildProducer<NativeImageResourceBuildItem> nativeImageResources,
	BuildProducer<HotDeploymentWatchedFileBuildItem> hotDeploymentWatchedFiles,
	List<DatabaseKindDialectBuildItem> dbKindDialectBuildItems) {
	//we have no persistence.xml so we will create a default one
	String persistenceUnitConfigName = DEFAULT_PERSISTENCE_UNIT_NAME;
	Map<String, Set<String>> modelClassesAndPackagesPerPersistencesUnits = HibernateOrmProcessor
	.getModelClassesAndPackagesPerPersistenceUnits(hibernateOrmConfig, jpaModel, index.getIndex(), true);
	Set<String> nonDefaultPUWithModelClassesOrPackages = modelClassesAndPackagesPerPersistencesUnits.entrySet().stream()
	.filter(e -> !DEFAULT_PERSISTENCE_UNIT_NAME.equals(e.getKey()) && !e.getValue().isEmpty())
	.map(Map.Entry::getKey)
	.collect(Collectors.toSet());
	if (!nonDefaultPUWithModelClassesOrPackages.isEmpty()) {
	// Not supported yet; see https://github.com/quarkusio/quarkus/issues/21110
	LOG.warnf("Entities are affected to non-default Hibernate Reactive persistence units %s."
	+ " Since Hibernate Reactive only works with the default persistence unit, those entities will be ignored.",
	nonDefaultPUWithModelClassesOrPackages);
	}
	Set<String> modelClassesAndPackages = modelClassesAndPackagesPerPersistencesUnits
	.getOrDefault(DEFAULT_PERSISTENCE_UNIT_NAME, Collections.emptySet());
	if (modelClassesAndPackages.isEmpty()) {
	LOG.warnf("Could not find any entities affected to the Hibernate Reactive persistence unit.");
	}
	QuarkusPersistenceUnitDescriptor desc = new QuarkusPersistenceUnitDescriptor(
	HibernateReactive.DEFAULT_REACTIVE_PERSISTENCE_UNIT_NAME, persistenceUnitConfigName,
	PersistenceUnitTransactionType.RESOURCE_LOCAL,
	new ArrayList<>(modelClassesAndPackages),
	new Properties());
	setDialectAndStorageEngine(dbKindOptional, explicitDialect, explicitDbMinVersion, dbKindDialectBuildItems,
	persistenceUnitConfig.dialect().storageEngine(), systemProperties, desc);
	// Physical Naming Strategy
	persistenceUnitConfig.physicalNamingStrategy().ifPresent(
	namingStrategy -> desc.getProperties()
	.setProperty(AvailableSettings.PHYSICAL_NAMING_STRATEGY, namingStrategy));
	// Implicit Naming Strategy
	persistenceUnitConfig.implicitNamingStrategy().ifPresent(
	namingStrategy -> desc.getProperties()
	.setProperty(AvailableSettings.IMPLICIT_NAMING_STRATEGY, namingStrategy));
	// Mapping
	if (persistenceUnitConfig.mapping().timezone().timeZoneDefaultStorage().isPresent()) {
	desc.getProperties().setProperty(AvailableSettings.TIMEZONE_DEFAULT_STORAGE,
	persistenceUnitConfig.mapping().timezone().timeZoneDefaultStorage().get().name());
	}
	desc.getProperties().setProperty(AvailableSettings.PREFERRED_POOLED_OPTIMIZER,
	persistenceUnitConfig.mapping().id().optimizer().idOptimizerDefault()
	.orElse(HibernateOrmConfigPersistenceUnit.IdOptimizerType.POOLED_LO).configName);
	//charset
	desc.getProperties().setProperty(AvailableSettings.HBM2DDL_CHARSET_NAME,
	persistenceUnitConfig.database().charset().name());
	// Quoting strategy
	if (persistenceUnitConfig.quoteIdentifiers().strategy() == IdentifierQuotingStrategy.ALL
	|| persistenceUnitConfig.quoteIdentifiers()
	.strategy() == IdentifierQuotingStrategy.ALL_EXCEPT_COLUMN_DEFINITIONS
	|| persistenceUnitConfig.database().globallyQuotedIdentifiers()) {
	desc.getProperties().setProperty(AvailableSettings.GLOBALLY_QUOTED_IDENTIFIERS, "true");
	}
	if (persistenceUnitConfig.quoteIdentifiers().strategy() == IdentifierQuotingStrategy.ALL_EXCEPT_COLUMN_DEFINITIONS) {
	desc.getProperties().setProperty(AvailableSettings.GLOBALLY_QUOTED_IDENTIFIERS_SKIP_COLUMN_DEFINITIONS, "true");
	} else if (persistenceUnitConfig.quoteIdentifiers().strategy() == IdentifierQuotingStrategy.ONLY_KEYWORDS) {
	desc.getProperties().setProperty(AvailableSettings.KEYWORD_AUTO_QUOTING_ENABLED, "true");
	}
	// Query
	// TODO ideally we should align on ORM and use 16 as a default, but that would break applications
	// because of https://github.com/hibernate/hibernate-reactive/issues/742
	int batchSize = firstPresent(persistenceUnitConfig.fetch().batchSize(), persistenceUnitConfig.batchFetchSize())
	.orElse(-1);
	if (batchSize > 0) {
	desc.getProperties().setProperty(AvailableSettings.DEFAULT_BATCH_FETCH_SIZE,
	Integer.toString(batchSize));
	desc.getProperties().setProperty(AvailableSettings.BATCH_FETCH_STYLE, BatchFetchStyle.PADDED.toString());
	}
	if (persistenceUnitConfig.fetch().maxDepth().isPresent()) {
	setMaxFetchDepth(desc, persistenceUnitConfig.fetch().maxDepth());
	} else if (persistenceUnitConfig.maxFetchDepth().isPresent()) {
	setMaxFetchDepth(desc, persistenceUnitConfig.maxFetchDepth());
	}
	desc.getProperties().setProperty(AvailableSettings.QUERY_PLAN_CACHE_MAX_SIZE, Integer.toString(
	persistenceUnitConfig.query().queryPlanCacheMaxSize()));
	desc.getProperties().setProperty(AvailableSettings.DEFAULT_NULL_ORDERING,
	persistenceUnitConfig.query().defaultNullOrdering().name().toLowerCase());
	desc.getProperties().setProperty(AvailableSettings.IN_CLAUSE_PARAMETER_PADDING,
	String.valueOf(persistenceUnitConfig.query().inClauseParameterPadding()));
	// JDBC
	persistenceUnitConfig.jdbc().timezone().ifPresent(
	timezone -> desc.getProperties().setProperty(AvailableSettings.JDBC_TIME_ZONE, timezone));
	persistenceUnitConfig.jdbc().statementFetchSize().ifPresent(
	fetchSize -> desc.getProperties().setProperty(AvailableSettings.STATEMENT_FETCH_SIZE,
	String.valueOf(fetchSize)));
	persistenceUnitConfig.jdbc().statementBatchSize().ifPresent(
	statementBatchSize -> desc.getProperties().setProperty(AvailableSettings.STATEMENT_BATCH_SIZE,
	String.valueOf(statementBatchSize)));
	// Statistics
	if (hibernateOrmConfig.metrics().enabled()
	|| (hibernateOrmConfig.statistics().isPresent() && hibernateOrmConfig.statistics().get())) {
	desc.getProperties().setProperty(AvailableSettings.GENERATE_STATISTICS, "true");
	}
	// sql-load-script
	List<String> importFiles = getSqlLoadScript(persistenceUnitConfig.sqlLoadScript(), launchMode);
	if (!importFiles.isEmpty()) {
	for (String importFile : importFiles) {
	Path loadScriptPath = applicationArchivesBuildItem.getRootArchive().getChildPath(importFile);
	if (loadScriptPath != null && !Files.isDirectory(loadScriptPath)) {
	// enlist resource if present
	nativeImageResources.produce(new NativeImageResourceBuildItem(importFile));
	hotDeploymentWatchedFiles.produce(new HotDeploymentWatchedFileBuildItem(importFile));
	} else if (persistenceUnitConfig.sqlLoadScript().isPresent()) {
	//raise exception if explicit file is not present (i.e. not the default)
	String propertyName = HibernateOrmRuntimeConfig.puPropertyKey(persistenceUnitConfigName, "sql-load-script");
	throw new ConfigurationException(
	"Unable to find file referenced in '"
	+ propertyName + "="
	+ String.join(",", persistenceUnitConfig.sqlLoadScript().get())
	+ "'. Remove property or add file to your path.",
	Collections.singleton(propertyName));
	}
	}
	if (persistenceUnitConfig.sqlLoadScript().isPresent()) {
	desc.getProperties().setProperty(AvailableSettings.HBM2DDL_IMPORT_FILES, String.join(",", importFiles));
	}
	} else {
	//Disable implicit loading of the default import script (import.sql)
	desc.getProperties().setProperty(AvailableSettings.HBM2DDL_IMPORT_FILES, "");
	}
	// Caching
	if (persistenceUnitConfig.secondLevelCachingEnabled()) {
	Properties p = desc.getProperties();
	//Only set these if the user isn't making an explicit choice:
	p.putIfAbsent(USE_DIRECT_REFERENCE_CACHE_ENTRIES, Boolean.TRUE);
	p.putIfAbsent(USE_SECOND_LEVEL_CACHE, Boolean.TRUE);
	p.putIfAbsent(USE_QUERY_CACHE, Boolean.TRUE);
	p.putIfAbsent(JAKARTA_SHARED_CACHE_MODE, SharedCacheMode.ENABLE_SELECTIVE);
	Map<String, String> cacheConfigEntries = HibernateConfigUtil.getCacheConfigEntries(persistenceUnitConfig);
	for (Entry<String, String> entry : cacheConfigEntries.entrySet()) {
	desc.getProperties().setProperty(entry.getKey(), entry.getValue());
	}
	} else {
	//Unless the global switch is explicitly set to off, in which case we disable all caching:
	Properties p = desc.getProperties();
	p.put(USE_DIRECT_REFERENCE_CACHE_ENTRIES, Boolean.FALSE);
	p.put(USE_SECOND_LEVEL_CACHE, Boolean.FALSE);
	p.put(USE_QUERY_CACHE, Boolean.FALSE);
	p.put(JAKARTA_SHARED_CACHE_MODE, SharedCacheMode.NONE);
	}
	return desc;
	}

[quarkus-bot]

/cc @gsmet (hibernate-orm)

[github-actions]

😭 Deploy PR Preview failed.

[gastaldi]

Perhaps get rid of this option and assume CALLBACK,DDL as the default value? It feels weird that you can select AUTO and other options.

[gsmet]

I think AUTO makes perfect sense as we want things to be integrated automatically when HV is added. It's also in line with what people are used to in ORM.

A question though, @marko-bekhta , do we really get to NONE if HV is not around? I would have expected DDL?

[marko-bekhta]

yeah... to apply the DDL ORM relies on the BV constraint metadata:
https://github.com/hibernate/hibernate-orm/blob/207a3637836d8405c140be8abcb0052284056eab/hibernate-core/src/main/java/org/hibernate/boot/beanvalidation/TypeSafeActivator.java#L237

so if BV is not around, it won't work. If the user explicitly requests DDL/CALLBACK and BV is not there, things throw exceptions 😃. I think that comes from the spec here:

If a Bean Validation provider is present in the environment, the persistence provider must perform the automatic validation of entities. If no Bean Validation provider is present in the environment, no lifecycle event validation takes place. This is the default behavior.

(https://jakarta.ee/specifications/persistence/3.2/apidocs/jakarta.persistence/jakarta/persistence/validationmode#AUTO)

and Hibernate ORM just adds on top of that the DDL mode 🙈

[gsmet]

Ok! Let’s just adjust the naming then and we should be fine.

[gsmet]

I would go with mode. It reads better and I don't think people will ever use two options at the same time, given auto does the job.

And even if you use two values, it's still the mode you want to use.

[gsmet]

Using auto as that's what will appear as possible values in the doc.

Suggested change

	@WithDefault("AUTO")
	@WithDefault("auto")

[gsmet]

I think AUTO makes perfect sense as we want things to be integrated automatically when HV is added. It's also in line with what people are used to in ORM.

A question though, @marko-bekhta , do we really get to NONE if HV is not around? I would have expected DDL?

[marko-bekhta]

btw.. do we want this to be included in the reactive processor? or reactive does not play nice with validation? (that was that other question that @yrodiere had: why does DDL work for reactive..)

[gsmet]

I have no idea, I haven't been involved in the Hibernate Reactive extension.

Either it's an oversight or it was not working. @DavideD would know better but, if we do implement it for HR also, I think it should be done in another PR anyway.

[gsmet]

I merged this one as that's something I want in 3.19. I let you discuss the situation in HR and see if it needs to be addressed in a follow-up PR.

[yrodiere]

Created #46327 for the Hibernate Reactive part.

[quarkus-bot]

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 86ee3e3.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

[quarkus-bot]

Status for workflow Quarkus CI

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

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

---

Flaky tests - Develocity

⚙️ JVM Tests - JDK 21

📦 extensions/panache/hibernate-reactive-rest-data-panache/deployment

✖ io.quarkus.hibernate.reactive.rest.data.panache.deployment.entity.PanacheEntityResourcePutMethodTest.shouldUpdateSimpleObject - History

• 1 expectation failed. JSON path name doesn't match. Expected: is "first-test" Actual: first - java.lang.AssertionError

java.lang.AssertionError: 
1 expectation failed.
JSON path name doesn't match.
Expected: is "first-test"
  Actual: first

	at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:502)
	at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:486)

[gsmet]

Ah ah, you're awesome, I was going to ask for a migration guide entry but I see it was already in the description :).