Switch Vert.x, datasource, reactive datasource and Agroal to @ConfigMapping #33228

gsmet · 2023-05-09T14:54:32Z

...s/agroal/runtime/src/main/java/io/quarkus/agroal/runtime/DataSourcesJdbcBuildTimeConfig.java

...nsions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/VertxHttpRecorder.java

radcortez · 2023-05-23T11:05:09Z

Please, have a look at smallrye/smallrye-config#939. It would be great if we could try this properly before merging and releasing it in SR Config, so we can make any adjustments.

gsmet · 2023-05-26T12:35:12Z

@radcortez I have been struggling with this for a while and I think I will need your help.
I had to convert the Redis extension because it is using some config classes from Vert.x.

Unfortunately, I have two issues converting this extension:

NetConfig has an Optional<ProxyConfig> proxyOptions() configuration property. It used to be an empty Optional when no proxy configuration was set, it is now defined (with the default values set). This causes problems as it enables the proxy configuration. I think this new behavior will cause some problems for a lot of extensions so I suppose we should try to go back to the existing behavior. Or we will have to find another way to cover that use case.
I wonder if @WithConverter works similarly to @ConvertWith when it comes to List because I also have this error:

[ERROR]   NonDefaultFileForDefaultClientImportPreloadingTest » Runtime java.lang.RuntimeException: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
	[error]: Build step io.quarkus.redis.deployment.client.RedisClientProcessor#init threw an exception: java.lang.ClassCastException: class java.lang.String cannot be cast to class java.util.List (java.lang.String and java.util.List are in module java.base of loader 'bootstrap')
	at io.quarkus.redis.deployment.client.RedisClientProcessor.getRedisLoadScript(RedisClientProcessor.java:300)

I added some comments in the code, I will point them to you in subsequent GitHub comments.

I will start to test your Map default branch now and report back.

gsmet · 2023-05-26T12:35:51Z

...client/deployment/src/main/java/io/quarkus/redis/deployment/client/RedisClientProcessor.java

+        // TODO @radcortez, this apparently returns a String now instead of a List<String>
+        var scripts = config.loadScript();


This is the @WithConverter issue that leads to a class cast exception.

You need to map it like this:
Optional<List<@WithConverter(TrimmedStringConverter.class)String>> loadScript();

@WithConverter now applies in the exact nested element type.

That's what I did and the class cast error is gone:

@ConfigDocDefault("import.redis in DEV, TEST ; no-file otherwise") Optional<List<@WithConverter(TrimmedStringConverter.class) String>> loadScript();

But it looks like the converter is somehow not applied:

Caused by: io.quarkus.runtime.configuration.ConfigurationException: Unable to find file referenced in 'quarkus.redis.redis-load-script=import/my-import.redis, sample.redis'. Remove property or add file to your path. at io.quarkus.redis.deployment.client.RedisClientProcessor.preloadRedisData(RedisClientProcessor.java:263) at io.quarkus.redis.deployment.client.RedisClientProcessor.init(RedisClientProcessor.java:185) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.base/java.lang.reflect.Method.invoke(Method.java:566) at io.quarkus.deployment.ExtensionLoader$3.execute(ExtensionLoader.java:909) at io.quarkus.builder.BuildContext.run(BuildContext.java:282) at org.jboss.threads.ContextHandler$1.runWith(ContextHandler.java:18) at org.jboss.threads.EnhancedQueueExecutor$Task.run(EnhancedQueueExecutor.java:2513) at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1538) at java.base/java.lang.Thread.run(Thread.java:829) at org.jboss.threads.JBossThread.run(JBossThread.java:501)

It needs to be defined as:
Optional<@WithConverter(TrimmedStringConverter.class) List<String>> loadScript();

Sorry for misleading in the previous commented. Just pushed a fix.

Mkay. TBH, that's odd, I would have expected the converter to be placed on its target rather than the container.

Yes, it is a little misleading. I'll need to improve that.

smallrye/smallrye-config#948

gsmet · 2023-05-26T12:36:12Z

...is-client/runtime/src/main/java/io/quarkus/redis/runtime/client/VertxRedisClientFactory.java

+
+        tcp.localAddress().ifPresent(net::setLocalAddress);
+        tcp.nonProxyHosts().ifPresent(net::setNonProxyHosts);
+        // TODO @radcortez proxyOptions() used to be empty when nothing was set and is not empty anymore


This is where the problem with Optional manifests itself.

The issue here is that it can work both ways. You may want an instance of the configuration with the defaults and use it, or completely disregard it. This was not introduced with the defaults for Maps work.

Looking into the mapping and business logic, what we are saying is:

"Use these defaults only if certain properties are defined"

This creates conditional elements where only certain things are set. This was indeed supported when the default was only set with @WithDefault, but caused issues when we wanted to override only the default (since in that case it would trigger the element creation)

We simplified this, by creating elements for defaults in every case, and it is up to the user to apply the business logic on how they want to handle it. You are going to find cases where you want always to find the defaults, and there are going to be cases where you only want to use them if other properties are set (like the Proxy case here).

I believe a more correct mapping to represent such case is something like:

@ConfigMapping(prefix = "proxy") interface ProxyOptionalDefaults { ProxyConfig proxy(); interface ProxyConfig { @WithParentName Optional<ProxyHost> host(); @WithParentName ProxySettings settings(); } interface ProxyHost { String host(); } interface ProxySettings { @WithDefault("3128") int port(); } }

radcortez · 2023-05-26T13:48:58Z

@radcortez I have been struggling with this for a while and I think I will need your help.

I'll have a look.

gsmet · 2023-05-26T14:17:06Z

@radcortez BTW, maybe I was unclear but the rest is working. The two remaining issues are the ones I mentioned about the Redis extension.

radcortez · 2023-05-26T15:01:28Z

@radcortez BTW, maybe I was unclear but the rest is working. The two remaining issues are the ones I mentioned about the Redis extension.

No, I've got it :)

I've already replied to both.

geoand · 2023-07-28T13:23:16Z

🏆

radcortez · 2023-07-28T13:30:50Z

And ready \o/. Thanks for your help on this @radcortez !

You did all the work. I've just fixed my own bugs :)

gsmet · 2023-07-29T14:54:46Z

@radcortez unfortunately, I lost your commit when I rebased and I remember it was just a line but couldn't figure it out. Could you again push your fix on top of the rebased branch? Sorry and thanks!

radcortez · 2023-07-31T08:34:12Z

@radcortez unfortunately, I lost your commit when I rebased and I remember it was just a line but couldn't figure it out. Could you again push your fix on top of the rebased branch? Sorry and thanks!

Sure. Done!

radcortez · 2023-07-31T08:38:57Z

Hum, now sure why but my commit is only showing in your branch and the PR did not update automatically.

gsmet · 2023-07-31T08:55:50Z

That's very odd. Let me try to get the commit, rebase and force push.

gsmet · 2023-07-31T08:57:36Z

It worked, let's see what CI has to say.

yrodiere

LGTM overall, though I'd be lying if I said this was a pleasant read :x

Also, I added a few minor comments below.

core/runtime/src/main/java/io/quarkus/runtime/annotations/ConfigDocIgnore.java

...nsions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/VertxHttpRecorder.java

yrodiere · 2023-08-01T11:16:08Z

.../vertx/runtime/src/main/java/io/quarkus/vertx/core/runtime/config/EventBusConfiguration.java

-    @ConfigItem
-    public int reconnectAttempts;
+    @WithDefault("0")
+    int reconnectAttempts();


Is there a reason for this new @WithDefault("0")? Isn't 0 the default for primitive integers anyway?

Just asking in case this is a workaround for a weird behavior I should be aware of...

See my answer above. For booleans, SmallRye Config was complaining so I made them explicit.

yrodiere · 2023-08-01T11:17:12Z

.../vertx/runtime/src/main/java/io/quarkus/vertx/core/runtime/config/EventBusConfiguration.java

-    @ConfigItem
-    public boolean tcpKeepAlive;
+    @WithDefault("false")
+    boolean tcpKeepAlive();


Same here, I'd expect false to be the default for primitive booleans... ?

yrodiere · 2023-08-01T11:17:28Z

.../vertx/runtime/src/main/java/io/quarkus/vertx/core/runtime/config/EventBusConfiguration.java

-    @ConfigItem
-    public boolean trustAll;
+    @WithDefault("false")
+    boolean trustAll();


Same here, I'd expect false to be the default for primitive booleans... ?

...tasource/runtime/src/main/java/io/quarkus/datasource/runtime/DataSourcesBuildTimeConfig.java

yrodiere · 2023-08-01T12:43:17Z

...ons/agroal/runtime/src/main/java/io/quarkus/agroal/runtime/DataSourcesJdbcRuntimeConfig.java

-    @ConfigItem
-    public DataSourceJdbcRuntimeConfig jdbc;
+    DataSourceJdbcRuntimeConfig jdbc();

    /**
     * Additional named datasources.
     */
    @ConfigDocSection
    @ConfigDocMapKey("datasource-name")
-    @ConfigItem(name = ConfigItem.PARENT)
-    public Map<String, DataSourceJdbcOuterNamedRuntimeConfig> namedDataSources;
+    @WithParentName
+    @WithDefaults
+    Map<String, DataSourceJdbcOuterNamedRuntimeConfig> namedDataSources();


Any reason not to merge these into a single datasources() method?

If so, can you please add a comment here so I don't break your stuff next time I end up in this part of the code? :)

I think it doesn't fly because you have:

quarkus.datasource.jdbc.... quarkus.datasource."name".jdbc....

I didn't really try but I thought it would be hard to make it work.

Ok I suppose we'll send a PR about that later

...src/main/java/io/quarkus/datasource/deployment/spi/DevServicesDatasourceResultBuildItem.java

...nt-spi/src/main/java/io/quarkus/datasource/deployment/spi/DevServicesDatasourceProvider.java

yrodiere · 2023-08-01T12:53:55Z

.../deployment/src/test/java/io/quarkus/redis/deployment/client/RedisConfigClientNamesTest.java

+        DevServicesConfig devServicesConfig = new DevServicesConfig() {
+
+            @Override
+            public boolean enabled() {
+                return enabled;
+            }
+
+            @Override
+            public Optional<String> imageName() {
+                return Optional.empty();
+            }
+
+            @Override
+            public OptionalInt port() {
+                return OptionalInt.empty();
+            }
+
+            @Override
+            public boolean shared() {
+                return false;
+            }
+
+            @Override
+            public String serviceName() {
+                return null;
+            }
+
+            @Override
+            public Map<String, String> containerEnv() {
+                return Map.of();
+            }
+        };
+
+        DevServiceConfiguration devServiceConfiguration = new DevServiceConfiguration() {
+
+            @Override
+            public DevServicesConfig devservices() {
+                return devServicesConfig;
+            }
+        };
+
+        return devServiceConfiguration;


Same here, I'd suggest mockito.

This unifies the config reference doc and make sure we have both runtime and build time config in the same file.

quarkus-bot · 2023-08-01T23:59:29Z

Failing Jobs - Building `e244bbe`

Status	Name	Step	Failures	Logs	Raw logs
✔️	JVM Tests - JDK 11
✔️	JVM Tests - JDK 17
⌛	JVM Tests - JDK 20	`Build`	⚠️ Check →	Logs	Raw logs

- Class CrashingXADataSource used methods[1], which were added in 3.3.0 [1] quarkusio/quarkus#33228

quarkus-bot bot added area/agroal area/core area/persistence OBSOLETE, DO NOT USE area/reactive-sql-clients area/vertx labels May 9, 2023

gsmet commented May 9, 2023

View reviewed changes

...s/agroal/runtime/src/main/java/io/quarkus/agroal/runtime/DataSourcesJdbcBuildTimeConfig.java Outdated Show resolved Hide resolved

...nsions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/VertxHttpRecorder.java Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

gsmet force-pushed the config-mapping-vertx branch from cfbae00 to 1fc9e5c Compare May 17, 2023 17:02

quarkus-bot bot added area/config area/hibernate-reactive Hibernate Reactive labels May 17, 2023

This comment has been minimized.

Sign in to view

gsmet force-pushed the config-mapping-vertx branch from 1fc9e5c to edda14a Compare May 26, 2023 07:48

quarkus-bot bot added area/redis area/documentation labels May 26, 2023

gsmet force-pushed the config-mapping-vertx branch from 5d8c153 to 8796e3a Compare May 26, 2023 12:30

gsmet commented May 26, 2023

View reviewed changes

gsmet marked this pull request as draft May 26, 2023 14:16

gsmet force-pushed the config-mapping-vertx branch from 476cc16 to 8796e3a Compare May 26, 2023 14:29

gsmet mentioned this pull request May 26, 2023

Config mapping vertx default map #33640

Closed

gsmet force-pushed the config-mapping-vertx branch 3 times, most recently from 71350ad to 718a5aa Compare June 16, 2023 13:42

gsmet mentioned this pull request Jul 5, 2023

Remove long deprecated options from CertificateConfig #34540

Merged

gsmet force-pushed the config-mapping-vertx branch from 718a5aa to 3549078 Compare July 5, 2023 09:51

This comment has been minimized.

Sign in to view

gsmet force-pushed the config-mapping-vertx branch from b6ed878 to a6a94d4 Compare July 31, 2023 08:57

This comment has been minimized.

Sign in to view

gsmet mentioned this pull request Aug 1, 2023

Add maxLifetime to reactive datasource configurations #34985

Merged

yrodiere reviewed Aug 1, 2023

View reviewed changes

gsmet and others added 7 commits August 1, 2023 16:33

Switch to @ConfigMapping for Vert.x extension

164c4ba

Convert datasource and reactive datasource config to @ConfigMapping

1e5f3f7

Switch Redis extension to @ConfigMapping

9613259

Reorganize Redis deployment package

ca15bf7

This unifies the config reference doc and make sure we have both runtime and build time config in the same file.

Use new SmallRye Config map default feature

265bd2e

Create empty config with default and discovered sources

acb9191

Improve compatibility for DevServicesDatasourceProvider

e244bbe

gsmet force-pushed the config-mapping-vertx branch from 64a3552 to e244bbe Compare August 1, 2023 14:34

yrodiere approved these changes Aug 1, 2023

View reviewed changes

radcortez linked an issue Aug 2, 2023 that may be closed by this pull request

Optimize config generation #32209

Closed

gsmet merged commit 9aa28e3 into quarkusio:main Aug 2, 2023
49 of 50 checks passed

quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Aug 2, 2023

quarkus-bot bot added this to the 3.3 - main milestone Aug 2, 2023

quarkus-bot bot added the kind/enhancement New feature or request label Aug 2, 2023

fedinskiy added a commit to fedinskiy/quarkus-test-suite that referenced this pull request Aug 28, 2023

Adapt backported code to Quarkus 3.2

1008d5c

- Class CrashingXADataSource used methods[1], which were added in 3.3.0 [1] quarkusio/quarkus#33228

yrodiere mentioned this pull request Sep 27, 2024

Use SmallRye Config @WithDefaults/@WithUnnamedKey for reactive datasource config #43564

Merged

		// TODO @radcortez, this apparently returns a String now instead of a List<String>
		var scripts = config.loadScript();

Switch Vert.x, datasource, reactive datasource and Agroal to @ConfigMapping #33228

Switch Vert.x, datasource, reactive datasource and Agroal to @ConfigMapping #33228

Conversation

gsmet commented May 9, 2023 • edited Loading

This comment has been minimized.

This comment has been minimized.

radcortez commented May 23, 2023

gsmet commented May 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsmet Jun 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radcortez commented May 26, 2023

gsmet commented May 26, 2023

radcortez commented May 26, 2023

geoand commented Jul 28, 2023

radcortez commented Jul 28, 2023

This comment has been minimized.

gsmet commented Jul 29, 2023

radcortez commented Jul 31, 2023

radcortez commented Jul 31, 2023

gsmet commented Jul 31, 2023

gsmet commented Jul 31, 2023

This comment has been minimized.

yrodiere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quarkus-bot bot commented Aug 1, 2023

Failing Jobs - Building e244bbe

gsmet commented May 9, 2023 •

edited

Loading

gsmet Jun 15, 2023 •

edited

Loading

Failing Jobs - Building `e244bbe`