Improve compatibility of the REST Client configuration

[radcortez]

The move of REST Client configuration to @ConfigMapping in #42106 is causing some issues that this PR should fix:

• The old config was only queried when the REST Client was being created, so it was possible to use expressions like ${quarkus.http.port} in the URL/ URI. For all extensions, the config is loaded when the application starts, so any further changes to the configuration are not visible. This is the case when we generate a random port and update ${quarkus.http.port}. We are now loading the URL / URI on REST Client creation.

• The force lookup to allow loading configuration from "invisible" sources

  quarkus/extensions/resteasy-classic/rest-client-config/runtime/src/main/java/io/quarkus/restclient/config/AbstractRestClientConfigBuilder.java

  Lines 19 to 27 in e69225d

  	* Usually, named configuration is mapped using a <code>Map</code> because the names are dynamic and unknown to
  	* Quarkus. In the case of the REST Client, configuration names are fixed and known at build time, but not to the point
  	* where the names can be mapped statically, so they still need to be mapped in a <code>Map</code>.
  	* <p>
  	* To populate a <code>Map</code>, because the names are dynamic, the Config system has to rely on the list of
  	* property names provided by each source. This also applies to the REST Client, but since the names are known to
  	* Quarkus, the REST Client configuration could be loaded even for sources that don't provide a list of property
  	* names. To achieve such behaviour, we provide a dummy configuration under each REST Client name to force
  	* the Config system to look up the remaining configuration in the same tree.

  ) was not generating a quoted key when the @RegisterRestClient#configKey had multiple segments, so it was causing a few issues.

• Also, the old implementation allowed the mixing of quoted and unquoted keys in the configuration names. This was never supported in the current Config system. I've added it here, using some interceptors and I'll properly implement this in SmallRyeConfig directly so other extensions behave the same way.

• Fixes quarkus-rest-client-jackson - Force property #42857

• Fixes Test: quarkus.http.port is not updated with random port activated through quarkus.http.test-port=0 #42944

• Fixes https://quarkusio.zulipchat.com/#narrow/stream/187030-users/topic/Rest-client.20url.20is.20now.20build.20time.20property.3F

[github-actions]

🎊 PR Preview b702e31 has been successfully built and deployed to https://quarkus-pr-main-42932-preview.surge.sh/version/main/guides/

• Images of blog posts older than 3 months are not available.
• Newsletters older than 3 months are not available.

[geoand]

The CI failures seem related

…

On Sun, Sep 1, 2024, 03:46 quarkus-bot[bot] ***@***.***> wrote: Status for workflow Quarkus CI This is the status report for running Quarkus CI on commit e69225d <e69225d> . Failing Jobs Status Name Step Failures Logs Raw logs Build scan ✖ JVM Tests - JDK 17 Build Failures <#m_8440690412275905414_user-content-test-failures-job-29519280841> Logs <https://github.com/quarkusio/quarkus/actions/runs/10649192250/job/29519280841> Raw logs <https://github.com/quarkusio/quarkus/commit/e69225ded3de3013345457828305d2eaaf66bfc5/checks/29519280841/logs> 🚧 ✖ JVM Tests - JDK 21 Build Failures <#m_8440690412275905414_user-content-test-failures-job-29519281220> Logs <https://github.com/quarkusio/quarkus/actions/runs/10649192250/job/29519281220> Raw logs <https://github.com/quarkusio/quarkus/commit/e69225ded3de3013345457828305d2eaaf66bfc5/checks/29519281220/logs> 🚧 ✖ JVM Tests - JDK 17 Windows Build Failures <#m_8440690412275905414_user-content-test-failures-job-29519281346> Logs <https://github.com/quarkusio/quarkus/actions/runs/10649192250/job/29519281346> Raw logs <https://github.com/quarkusio/quarkus/commit/e69225ded3de3013345457828305d2eaaf66bfc5/checks/29519281346/logs> 🚧 Full information is available in the Build summary check run <https://github.com/quarkusio/quarkus/runs/29519854942>. Failures ⚙️ JVM Tests - JDK 17 # <#m_8440690412275905414_user-content-test-failures-job-29519280841> - Failing: extensions/resteasy-classic/resteasy-client/runtime ! Skipped: extensions/amazon-lambda-http/http-event-server extensions/amazon-lambda-rest/rest-event-server extensions/amazon-lambda/event-server and 98 more 📦 extensions/resteasy-classic/resteasy-client/runtime ✖ io.quarkus.restclient.runtime.RestClientBaseTest.testClientSpecificConfigs line 109 - History <https://ge.quarkus.io/scans/tests?tests.container=io.quarkus.restclient.runtime.RestClientBaseTest&tests.test=testClientSpecificConfigs> - More details <https://github.com/quarkusio/quarkus/runs/29519854942#user-content-test-failure-io.quarkus.restclient.runtime.restclientbasetest-1> - Source on GitHub <https://github.com/quarkusio/quarkus/blob/e69225ded3de3013345457828305d2eaaf66bfc5/extensions/resteasy-classic/resteasy-client/runtime/src/test/java/io/quarkus/restclient/runtime/RestClientBaseTest.java#L109> Argument(s) are different! Wanted: restClientBuilder.baseUrl( http://localhost ); -> at io.quarkus.restclient.runtime.RestClientBaseTest.testClientSpecificConfigs(RestClientBaseTest.java:109) Actual invocations have different arguments: restClientBuilder.baseUrl( http://localhost:8080 ------------------------------ ⚙️ JVM Tests - JDK 21 # <#m_8440690412275905414_user-content-test-failures-job-29519281220> - Failing: extensions/resteasy-classic/resteasy-client/runtime ! Skipped: extensions/amazon-lambda-http/http-event-server extensions/amazon-lambda-rest/rest-event-server extensions/amazon-lambda/event-server and 98 more 📦 extensions/resteasy-classic/resteasy-client/runtime ✖ io.quarkus.restclient.runtime.RestClientBaseTest.testClientSpecificConfigs line 109 - History <https://ge.quarkus.io/scans/tests?tests.container=io.quarkus.restclient.runtime.RestClientBaseTest&tests.test=testClientSpecificConfigs> - More details <https://github.com/quarkusio/quarkus/runs/29519854942#user-content-test-failure-io.quarkus.restclient.runtime.restclientbasetest-1> - Source on GitHub <https://github.com/quarkusio/quarkus/blob/e69225ded3de3013345457828305d2eaaf66bfc5/extensions/resteasy-classic/resteasy-client/runtime/src/test/java/io/quarkus/restclient/runtime/RestClientBaseTest.java#L109> Argument(s) are different! Wanted: restClientBuilder.baseUrl( http://localhost ); -> at io.quarkus.restclient.runtime.RestClientBaseTest.testClientSpecificConfigs(RestClientBaseTest.java:109) Actual invocations have different arguments: restClientBuilder.baseUrl( http://localhost:8080 ------------------------------ ⚙️ JVM Tests - JDK 17 Windows # <#m_8440690412275905414_user-content-test-failures-job-29519281346> - Failing: extensions/resteasy-classic/resteasy-client/runtime ! Skipped: extensions/amazon-lambda-http/http-event-server extensions/amazon-lambda-rest/rest-event-server extensions/amazon-lambda/event-server and 98 more 📦 extensions/resteasy-classic/resteasy-client/runtime ✖ io.quarkus.restclient.runtime.RestClientBaseTest.testClientSpecificConfigs line 109 - History <https://ge.quarkus.io/scans/tests?tests.container=io.quarkus.restclient.runtime.RestClientBaseTest&tests.test=testClientSpecificConfigs> - More details <https://github.com/quarkusio/quarkus/runs/29519854942#user-content-test-failure-io.quarkus.restclient.runtime.restclientbasetest-1> - Source on GitHub <https://github.com/quarkusio/quarkus/blob/e69225ded3de3013345457828305d2eaaf66bfc5/extensions/resteasy-classic/resteasy-client/runtime/src/test/java/io/quarkus/restclient/runtime/RestClientBaseTest.java#L109> Argument(s) are different! Wanted: restClientBuilder.baseUrl( http://localhost ); -> at io.quarkus.restclient.runtime.RestClientBaseTest.testClientSpecificConfigs(RestClientBaseTest.java:109) Actual invocations have different arguments: restClientBuilder.baseUrl( http://localhost:8080 ------------------------------ Flaky tests - Develocity <https://ge.quarkus.io/scans/tests> ⚙️ JVM Tests - JDK 17 📦 extensions/resteasy-reactive/rest-client/deployment ✖ io.quarkus.rest.client.reactive.stork.StorkResponseTimeLoadBalancerTest.shouldUseFasterService - History <https://ge.quarkus.io/scans/tests?tests.container=io.quarkus.rest.client.reactive.stork.StorkResponseTimeLoadBalancerTest&tests.test=shouldUseFasterService> - expected: "hello, Alice" but was: "hello, I'm a slow server" - org.opentest4j.AssertionFailedError org.opentest4j.AssertionFailedError: expected: "hello, Alice" but was: "hello, I'm a slow server" at io.quarkus.rest.client.reactive.stork.StorkResponseTimeLoadBalancerTest.shouldUseFasterService(StorkResponseTimeLoadBalancerTest.java:58) at java.base/java.lang.reflect.Method.invoke(Method.java:569) at io.quarkus.test.QuarkusUnitTest.runExtensionMethod(QuarkusUnitTest.java:499) at io.quarkus.test.QuarkusUnitTest.interceptTestMethod(QuarkusUnitTest.java:413) — Reply to this email directly, view it on GitHub <#42932 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABBMDPZFWKTVURMNWUTAQ4DZUJPVDAVCNFSM6AAAAABNOI7TZGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMRTGA4TENZSHE> . You are receiving this because your review was requested.Message ID: ***@***.***>

[radcortez]

The CI failures seem related

Just a wrong assertion: http://localhost vs http://localhost:8080.

[gsmet]

Thanks for the PR! I added some comments.

I must admit that I'm a bit concerned that this config requires so many hacks. But I can't say I understand fully all the requirements so it's just a vague impression.

[gsmet]

Why are we doing that and enforcing this value here? I don't understand why we would have to do that.

[gsmet]

OK, so I have done my homework about this thing and it seems reasonable.

I wonder though if we should come up with a way to filter it from the Dev UI? Maybe by filtering config that shouldn't appear in the doc? If it's not in the doc, I would thing that we wouldn't want it to be visible.

Or we would need another annotation.

[gsmet]

Note that I'm not requesting a change for this patch. Just thinking out loud for the future.

[radcortez]

I've tried to add that explanation here:

quarkus/extensions/resteasy-classic/rest-client-config/runtime/src/main/java/io/quarkus/restclient/config/AbstractRestClientConfigBuilder.java

Lines 19 to 27 in 1da0004

	* Usually, named configuration is mapped using a <code>Map</code> because the names are dynamic and unknown to
	* Quarkus. In the case of the REST Client, configuration names are fixed and known at build time, but not to the point
	* where the names can be mapped statically, so they still need to be mapped in a <code>Map</code>.
	* <p>
	* To populate a <code>Map</code>, because the names are dynamic, the Config system has to rely on the list of
	* property names provided by each source. This also applies to the REST Client, but since the names are known to
	* Quarkus, the REST Client configuration could be loaded even for sources that don't provide a list of property
	* names. To achieve such behaviour, we provide a dummy configuration under each REST Client name to force
	* the Config system to look up the remaining configuration in the same tree.

Let me expand that further, and then I can update the javadoc too:

In config, when we populate a Map, we must rely on the list of property names because the key is dynamic. As an example, if we want to retrieve the list of datasources, we have to look for properties quarkus.datasource."datasource-name", and then we do the rest.

There is a catch: of course, this only works when we get at least one property of the tree with the expected name. The problem is that some sources (like Vault) cannot list the properties. In the example case, if all the datasource properties are only set in Vault, we cannot load that configuration because we don't know it exists. What we recommend is to set some base properties and then override them.

In the case of the REST Client, it's a bit of a mix. At the mapping level, we use a Map because, obviously, we can't represent an unknown number of clients in the mapping structure, but we do know all the possible keys in advance. So, I'm adding a dummy key to force the lookup. Once the Config knows the dynamic part of a key, it will then query all the other pieces directly so that the Vault config will work.

I plan to support this out of the box without having to rely on workarounds.

[gsmet]

Yeah, I missed it because of the context of the diff but the Javadoc you already have is perfectly clear. That's what I was trying to say in my previous message by "doing my homework" :)

[radcortez]

I wonder though if we should come up with a way to filter it from the Dev UI? Maybe by filtering config that shouldn't appear in the doc? If it's not in the doc, I would thing that we wouldn't want it to be visible.

Yes, it is annoying, and I didn't think of that when I added it in the first place. We could filter it with the doc annotation, or maybe @Deprecated?

Anyway, I'll add something in the Config system for such cases (we have the same case in OTel), where we can't statically map the configuration, but the keys are well-known. I'll add something that will always query specific keys without relying on the list provided by the sources, so these types of hacks will no longer be required.

[gsmet]

Maybe it's a crazy idea but should we affect the random port when reading the config instead? Maybe by using a converter?

[radcortez]

Unfortunately, no. The problem here is how we read and set quarkus.http.port.

One of the first things we do when we start Quarkus is to load the configurations and cache the values in the mappings / roots (this is the same for old roots and new mappings). This is fine when we explicitly set the quarkus.http.port since the value gets expanded to the explicitly set port.

When we set a random port of 0, we delegate the random port assignment to Vert.x, but we also need the configuration to be loaded to start Vert.x. So we have a chicken-egg problem here because the configuration starts before Vert.x, any expansion done for ${quarkus.http.port} is 0 (the random port). This will get cached in the config objects.

Once Vert.x sets the port, this is mutating the SystemProperties:

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

Lines 1312 to 1332 in f399671

	if (https) {
	actualHttpsPort = actualPort;
	validateHttpPorts(actualHttpPort, actualHttpsPort);
	} else {
	actualHttpPort = actualPort;
	validateHttpPorts(actualHttpPort, actualHttpsPort);
	}
	if (actualPort != options.getPort()) {
	// Override quarkus.http(s)?.(test-)?port
	String schema;
	if (https) {
	clearHttpsProperty = true;
	schema = "https";
	} else {
	clearHttpProperty = true;
	actualHttpPort = actualPort;
	schema = "http";
	}
	portSystemProperties = new PortSystemProperties();
	portSystemProperties.set(schema, actualPort, launchMode);
	}

Which is a different problem that is causing other issues like:
#42844
#42106 (comment)
#27996

When we get the actual port, all the configuration objects are cached, and we need to query the configuration again to get the updated value. The problem is getting the discovered property name by the REST Client (see explanation in #42904 (comment)). Adding a ConfigValue here at the mapping class allows us to keep a reference to the full property name of the `URL' so we can query it without having to reconstruct all the REST Client name variations.

This worked previously because it was specifically implemented in REST Client with:
#21530

[gsmet]

But is Vert.x doing things that we can't do ourselves when the config is read? The contract of this is that you get a random port, we don't really care if it's Vert.x assigning it? Especially since you might want a random port for something else than Vert.x.

Now maybe Vert.x is doing something very clever that we can't replicate easily at the config level?

[gsmet]

BTW, if it wasn't obvious, I'm not advocating that we change things now. But I'm wondering if it's worth discussing it for the future.

[gsmet]

Also obviously, we would need to keep a list of the ports randomly affected around to make sure we don't affect the same port twice.

[radcortez]

I fully agree. We need to remove the random port generation from Vert.x and do it before the config is set up. This would eliminate many of our current hacks and issues.

Of course, I didn't want to make that change here, so I had to rely on another hack :)

[radcortez]

Thanks for the PR! I added some comments.

I must admit that I'm a bit concerned that this config requires so many hacks. But I can't say I understand fully all the requirements so it's just a vague impression.

Yes, I get it :(

The reason why we have to come up with these many hacks was how the REST Client configuration was implemented:
#21530

Namely, ignoring all the rules set in place by config roots. We probably shouldn't have allowed that, but at the time, we didn't have mappings fully developed and we didn't encounter issues like #41313, #42092 and others we tried to resolve with #42106.

Moving forward, I'll see what I can add to work out of the box in the Config system directly to be able to remove some of these hacks.

[geoand]

I'm fully in favor of removing the hacks at some point in the near future.

[radcortez]

I'm fully in favor of removing the hacks at some point in the near future.

That is the intention, especially because it will unlock other features, fix bugs, and remove other hacks. During this work, I took a lot of notes, and I'll start filling in issues with the work we have to do.

[geoand]

Have you by any chance done any profiling for this change?

Asking because lately we've seen stuff creep up where we didn't expect it :)

[quarkus-bot]

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 39d5ba1.

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

✅ 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 17 Windows

📦 integration-tests/jfr-reactive

✖ io.quarkus.jfr.it.JfrTest.blockingTest - History

• 1 expectation failed. JSON path start.resourceClass doesn't match. Expected: is "io.quarkus.jfr.it.AppResource" Actual: null - java.lang.AssertionError

java.lang.AssertionError: 
1 expectation failed.
JSON path start.resourceClass doesn't match.
Expected: is "io.quarkus.jfr.it.AppResource"
  Actual: null

	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)

[radcortez]

No. My concern was making it compatible with the old implementation, but both have serious issues.

Currently, to populate a REST Client Config, we may have to query all of the following names:

• quarkus.rest-client."[FQN]".*
• quarkus.rest-client.[Simple Class Name].*
• quarkus.rest-client."[Simple Class Name]".*
• quarkus.rest-client.[Config key from @RegisterRestClient].*
• quarkus.rest-client."[Config key from @RegisterRestClient]".*
• quarkus.rest-client-reactive."[FQN]".*
• quarkus.rest-client-reactive.[Simple Class Name].*
• quarkus.rest-client-reactive."[Simple Class Name]".*
• quarkus.rest-client-reactive.[Config key from @RegisterRestClient].*
• quarkus.rest-client-reactive."[Config key from @RegisterRestClient]".*
• [FQN]/mp-rest/*
• [Config Key from @RegisterRestClient]/mp-rest/*

This is crazy! Granted, that quarkus.rest-client-reactive will go away. But still, there are so many combinations that the loading time will depend on your available sources (for instance, Vault is a slow source) and which configuration style you use. For example, quarkus.rest-client."[FQN]".* is the first one to be queried (the previous list may not be ordered by query priority).

We probably need to break a few things to improve this... for instance if I set RegisterRestClient#configKey is there any point in looking for quarkus.rest-client."[FQN]".* or quarkus.rest-client."[Simple Class Name]".*? Also, once you are established on a particular style, is there any point in mixing all the style? Ugly but possible:

quarkus.rest-client."foo.bar.Client".url=
quarkus.rest-client."Client".uri=
quarkus.rest-client.config-key.providers=
quarkus.rest-client."config-key".connect-timeout=
config-key/mp-rest/read-timeout=

I hope no one is mixing things like this :)

I think we should move forward with some things (breaking), like:

• If you set RegisterRestClient#configKey, that is what we will look for, nothing else.
• If RegisterRestClient#configKey is not set, we only look for FQN and Simple name.

I still have to think about quotes... currently, the default implementation in SmallRyeConfig for Map keys is to use a quoted key if available and then use the quoted key to retrieve the rest of the tree. If not, it will use the unquoted key. If you mix quoted and unquoted, you only get the quoted values. I could implement a way to support the mixed style, but it will always require multiple lookups.

[gsmet]

I think we should move forward with some things (breaking), like:
If you set RegisterRestClient#configKey, that is what we will look for, nothing else.
If RegisterRestClient#configKey is not set, we only look for FQN and Simple name.

+1 from me. But we need to be careful as it would be a breaking change. But that's totally something we could merge for 3.16.

[geoand]

Agreed, go ahead and break/remove so of the old stuff for 3.16

[gsmet]

Also, I think we can drop most of the fallbacks we introduced recently (including quarkus.rest-client-reactive.*).

My rule of thumb is that we need the deprecation to be in at least one LTS and not be too new.
This will be the case now.

[geoand]

My rule of thumb is that we need the deprecation to be in at least one LTS and not be too new.

Makes sense.

think we can drop most of the fallbacks we introduced recently

Kill them :)

[radcortez]

Sharing some numbers (without much analysis yet - single REST Client app with application.properties):

	JVM RSS Startup	Native RSS Startup	JVM RSS First Req	Native RSS First Req	JVM Start Time	Native Start Time
3.13.2	103808	26645	133593	30640	0.689	0.030
3.14.1 Diff to 3.13.2	12.54%	3.87%	2.93%	0.87%	1.82%	12.28%
#42932 Diff to 3.13.2	5.99%	4.89%	1.30%	1.99%	1.60%	19.79%
#42932 Diff to 3.14.1	-7.48%	1.06%	-1.68%	1.13%	-0.23%	8.56%

This does not measure isolated code changes (it was just to get some quick numbers), so we cannot be sure of the actual degradation.

When comparing startup performance, we expect some degradation between 3.13.2 and 3.14.1 for the REST Client configuration. With #42106, mappings are loaded at startup time while in 3.13.2 the REST Client Config was loaded when we were creating the REST Client instance (on first request). I have some flame graphs that show config startup, 2.7 M samples vs 3.2 M but REST Client instance 3.6 M vs 2.9M.

With this PR, we can also expect to get worse startup numbers since we are doing even more work. Memory-wise, after the initial request, things are very similar (and I still need to try this PR directly on top of #42106 for a fair comparison.

I'm not too fond of these startup numbers, but I'm also unsure if we can improve them without breaking things. I can see two options:

• We revert Move REST Client configuration to use @ConfigMapping #42106 not to pay the price of the startup increase and we can reapply it again later with all the improvements (with breaking changes). This fixed other issues, but maybe we could live with those a little longer.
• We add another hack: offload the REST Client Config mapping creation on the instance creation (as it was already being done before Move REST Client configuration to use @ConfigMapping #42106). Temporarily until we add the improvements to make it work as we want.

In the meantime, I'll see if I can improve it in any way.

[geoand]

Stuck between a rock and a hard place :)

[geoand]

I guess going with an extra hack for the backport and then making the proper fix for 3.16 is an okay compromise

[gsmet]

I think we can live with this and merge this.

I think it's better than reverting everything now.

[geoand]

I very much look forward to having this cleaned up for 3.16 though :)

[radcortez]

I very much look forward to having this cleaned up for 3.16 though :)

Yes, this is going to be a top priority.