Switch Hibernate ORM/Reactive/Envers extensions to @ConfigMapping

[yrodiere]

Fixes #38321 (I checked manually)

Three reasons for this change:

1. As far as I can tell, the old @ConfigItem stuff is more or less deprecated. At least it's not supposed to evolve anymore.
2. Avoid bugs like Configuration ignored when using config maps and the prefix conflicts with a "static" config group #38321 in the future. Believe me, Configuration ignored when using config maps and the prefix conflicts with a "static" config group #38321 is just one among many. I have not-so-fond memories of bugs related to names containing dots, in particular; those are supposed to work much better with @ConfigMapping.
3. Get nicer documentation; thanks to what @radcortez did a few months ago, we no longer get duplicate entries in the config property list for the default persistence unit vs. named persistence units.

[quarkus-bot]

/cc @gsmet (hibernate-orm)

[yrodiere]

Yes it's very weird to have defaults on Optional properties, but as you can see that was already the case before. I didn't want to go into that rabbit hole for this PR, since my changes are unrelated.

[yrodiere]

Hey @zakkak , looks like native image size drifted off again... is that a new problem, or one that should be solved after a rebase? I'm really not sure why this PR would trigger this, so I assume it's also affecting main...

org.opentest4j.AssertionFailedError: Expected analysis_results.methods.reflection to be within range [4495 +- 3%] but was 4707 ==> expected: <true> but was: <false>

[gsmet]

Except if @ConfigMapping creates some additional reflection entries. But let's hear from @zakkak before pinging Roberto if needed.

[zakkak]

Except if @ConfigMapping creates some additional reflection entries. But let's hear from @zakkak before pinging Roberto if needed.

AFAIU this is exactly what's happening in

quarkus/extensions/spring-boot-properties/deployment/src/main/java/io/quarkus/spring/boot/properties/deployment/ClassConfigurationPropertiesUtil.java

Line 308 in 44672eb

reflectiveMethods.produce(new ReflectiveMethodBuildItem(method));

[yrodiere]

Still, +200 methods with reflection enabled? We don't have that many enums in that config. Unless it also enables reflection on every single method of every single configuration interface?

Let's see if @radcortez (hey!) notices something wrong in this PR or in the reflection-enabling code, and if not I'll just raise the baseline.

[zakkak]

Still, +200 methods with reflection enabled?

FWIW The actual difference (between this PR and main) is:

• jpa-postgresql 4707-4591= 116
• jpa-postgresql-withxml 4893-4776 = 117

The rest of the difference is carried over from previous changes that resulted in differences < 3%.

It still seems quite a lot.

[yrodiere]

Except if @ConfigMapping creates some additional reflection entries. But let's hear from @zakkak before pinging Roberto if needed.

AFAIU this is exactly what's happening in

quarkus/extensions/spring-boot-properties/deployment/src/main/java/io/quarkus/spring/boot/properties/deployment/ClassConfigurationPropertiesUtil.java

Line 308 in 44672eb

reflectiveMethods.produce(new ReflectiveMethodBuildItem(method));

Just noticed this code was taken from a "quarkus-spring" extension, which isn't relevant here.

I'll need to find the full list of methods enabled for reflection, before and after this PR. Then we can start asking ourselves if the new ones really are necessary.

[radcortez]

If I remember correctly, we enable reflection on every mapping method because of Bean Validation. Most extensions don't use it, so maybe we can be smarter and only register the BVal annotations methods.

Anyway, let me confirm if this is the case and adjust.

[radcortez]

Hum... BTW, we are looking into the wrong code. That code is from the old @ConfigProperties. The @ConfigMapping registration is in:

quarkus/core/deployment/src/main/java/io/quarkus/deployment/configuration/ConfigMappingUtils.java

Lines 87 to 119 in 1d65999

	private static void processConfigClass(
	ConfigClassWithPrefix configClassWithPrefix,
	Kind configClassKind,
	boolean isApplicationClass,
	CombinedIndexBuildItem combinedIndex,
	BuildProducer<GeneratedClassBuildItem> generatedClasses,
	BuildProducer<ReflectiveClassBuildItem> reflectiveClasses,
	BuildProducer<ConfigClassBuildItem> configClasses) {
	Class<?> configClass = configClassWithPrefix.getKlass();
	String prefix = configClassWithPrefix.getPrefix();
	List<ConfigMappingMetadata> configMappingsMetadata = ConfigMappingLoader.getConfigMappingsMetadata(configClass);
	Set<String> generatedClassesNames = new HashSet<>();
	configMappingsMetadata.forEach(mappingMetadata -> {
	generatedClassesNames.add(mappingMetadata.getClassName());
	// This is the generated implementation of the mapping by SmallRye Config.
	generatedClasses.produce(new GeneratedClassBuildItem(isApplicationClass, mappingMetadata.getClassName(),
	mappingMetadata.getClassBytes()));
	// Register the interface and implementation methods for reflection. This is required for Bean Validation.
	reflectiveClasses.produce(ReflectiveClassBuildItem.builder(mappingMetadata.getInterfaceType()).methods().build());
	reflectiveClasses.produce(ReflectiveClassBuildItem.builder(mappingMetadata.getClassName()).methods().build());
	// Register also the interface hierarchy
	for (Class<?> parent : getHierarchy(mappingMetadata.getInterfaceType())) {
	reflectiveClasses.produce(ReflectiveClassBuildItem.builder(parent).methods().build());
	}
	processProperties(mappingMetadata.getInterfaceType(), reflectiveClasses);
	});
	configClasses.produce(new ConfigClassBuildItem(configClass, collectTypes(combinedIndex, configClass),
	generatedClassesNames, prefix, configClassKind));
	}

It even has a comment about BVal, which confirms my initial thoughts.

[gsmet]

FWIW, there is a build item with all the Bean Validation annotations we detected. So we could probably check if a method has a Bean Validation annotation.

[yrodiere]

Hum... BTW, we are looking into the wrong code

Right; see my previous comment.

It even has a comment about BVal, which confirms my initial thoughts.

I think the BV extension already registers the interfaces for reflection. And I'd expect it to register the implementations as well, it's completely its responsibility.
So I think this is some sort of workaround... Maybe mapping implementation classes are not in the Jandex index and BV "forgets" to register them? Something like that? In that case perhaps a better solution would be to make BV aware of these implementation classes... somehow.

[gsmet]

For me the ConfigMapping optimization problem is orthogonal to this PR. Given the size of it and the possibility of conflicts, I would prefer we raise the threshold for now (in a separate commit) and that we create an issue with a reference to this commit, that we would revert once the problem is fixed.

If by chance, we significantly improve things, we could even lower the thresholds.

[radcortez]

I think the BV extension already registers the interfaces for reflection. And I'd expect it to register the implementations as well, it's completely its responsibility. So I think this is some sort of workaround... Maybe mapping implementation classes are not in the Jandex index and BV "forgets" to register them? Something like that? In that case perhaps a better solution would be to make BV aware of these implementation classes... somehow.

Mappings are in Jandex, and there is a build item that carries the mapping information. We even use that in BVal, to start its own validator (to avoid circular dependencies).

In the initial implementations, I've probably added the full registration preemptively, because I knew it was going to be needed and didn't think about optimizations :)

[yrodiere]

For me the ConfigMapping optimization problem is orthogonal to this PR. Given the size of it and the possibility of conflicts, I would prefer we raise the threshold for now (in a separate commit) and that we create an issue with a reference to this commit, that we would revert once the problem is fixed.

Right. I added a commit to update the thresholds.

Let's merge if the build passes now.

[radcortez]

I've looked, and here I've changed to register the methods: https://github.com/quarkusio/quarkus/pull/12501/files#diff-26708186baae4b2358b51f3bc3c5731740eff57a0a63fb3a150d15183e817f55R214

Looking into the commit, it seems I added it to allow returning the mapping directly in a REST resource with Jackson. That is probably not required anymore. Let me remove that and see what fails and adjust.

[zakkak]

I'll need to find the full list of methods enabled for reflection, before and after this PR. Then we can start asking ourselves if the new ones really are necessary.

FWIW to do this you can extract the reflect-config.json from the generated jar files that are passed to native-image and compare them (between main and your branch), e.g.:

unzip -j target/code-with-quarkus-1.0.0-SNAPSHOT-native-image-source-jar/code-with-quarkus-1.0.0-SNAPSHOT-runner.jar META-INF/native-image/reflect-config.json

[gsmet]

If you're adding reflection solely for Bean Validation requirements, then you can drop it.
The HV extension should take care of it and if not, we need to fix the bug as it would be one.

[quarkus-bot]

Status for workflow Quarkus CI

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

✅ 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.

---

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

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

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

• Assertion condition defined as a io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase 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 defined as a io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase 
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)