Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Strict config lookup for the REST Client #43345

Merged
merged 2 commits into from
Oct 22, 2024

Conversation

radcortez
Copy link
Member

@radcortez radcortez commented Sep 17, 2024

Follow up #42932

SmallRye Config now has the ability to add a @WithKeys annotation to a Map mapping. This allows us to provide the list of keys a Map must load. In the case of the REST Client, these keys are known during build time, so we can set them to be loaded during runtime with the following benefits:

  • Allows to retrieve configuration from sources that do not list their property names
  • Heavily reduce the number of lookups due to how many combinations the REST Client allows

This PR is a breaking change . Currently, to populate a REST Client configuration, we query the following configuration 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]".*
  • [FQN]/mp-rest/*
  • [Config key from @RegisterRestClient]/mp-rest/*

Users can mix all of these in their configuration. The final configuration is a merge of all names.

This PR will selectively add which fallbacks are required. For instance, if @RegisterRestClient#configKey is absent, we don't include fallbacks for that style name. We don't include an unquoted fallback if the configKey is multi-segments. This should help reduce the number of lookups, but there are still too many combinations for little value.

We may want to consider removing the [Simple Class Name] and "[Simple Class Name]" lookups. Or merge it with configKey, as if configKey is absent use [Simple Class Name]; if not, use configKey. In the documentation, I couldn't find any references to [Simple Class Name] to configure the REST Client: https://quarkus.io/guides/rest-client#create-the-configuration

Breaking changes:

  • Only REST Clients discovered by Quarkus are loaded by the @ConfigMapping
  • The REST Clients config Map always uses the FQN of the REST Client. Before, it would use all the keys found, even if related with the same REST Client duplicating (or more) entries.
  • Removed legacy configuration names quarkus.rest.client.max-redirects and quarkus.rest.client.multipart-post-encoder-mode

@geoand geoand removed area/resteasy-classic area/dependencies Pull requests that update a dependency file area/rest labels Sep 18, 2024
@radcortez radcortez force-pushed the rest-client-with-keys branch from 41c340b to 425c91d Compare September 20, 2024 11:24
@quarkus-bot quarkus-bot bot added area/dependencies Pull requests that update a dependency file area/rest area/resteasy-classic labels Sep 20, 2024
@radcortez radcortez force-pushed the rest-client-with-keys branch 2 times, most recently from 18216d0 to 030a2b6 Compare September 20, 2024 15:25
@radcortez radcortez force-pushed the rest-client-with-keys branch from 030a2b6 to 66fab28 Compare October 16, 2024 21:59
@quarkus-bot quarkus-bot bot added area/arc Issue related to ARC (dependency injection) area/config area/core area/testing labels Oct 16, 2024
@radcortez radcortez marked this pull request as ready for review October 16, 2024 22:25
Copy link

github-actions bot commented Oct 16, 2024

🎊 PR Preview de27583 has been successfully built and deployed to https://quarkus-pr-main-43345-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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@radcortez radcortez requested a review from gsmet October 17, 2024 13:34
@radcortez radcortez force-pushed the rest-client-with-keys branch from d4a650a to 5c62c82 Compare October 21, 2024 17:01
Copy link

quarkus-bot bot commented Oct 21, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 5c62c82.

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

Copy link

quarkus-bot bot commented Oct 21, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 5c62c82.

✅ 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

📦 devtools/cli

io.quarkus.cli.plugin.PluginCatalogServiceTest.shouldSyncWhenProjectFileIsNewerThanCatalog - History

  • expected: <true> but was: <false> - org.opentest4j.AssertionFailedError
org.opentest4j.AssertionFailedError: 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.AssertTrue.assertTrue(AssertTrue.java:31)
	at org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:183)
	at io.quarkus.cli.plugin.PluginCatalogServiceTest.shouldSyncWhenProjectFileIsNewerThanCatalog(PluginCatalogServiceTest.java:109)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants