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

Allow to disable ConfigProperties #17063

Merged
merged 1 commit into from May 10, 2021

Conversation

essobedo
Copy link
Contributor

@essobedo essobedo commented May 7, 2021

fixes #17061

Motivation

The annotation @ConfigProperties and @ConfigPrefix cannot be disabled once used which can be a problem when it maps an optional part of the configuration.

Modifications

  • Adds a new BuildItem called BuildExclusionsBuildItem to encapsulate all the classes, methods and fields that have been excluded thanks to build time conditions.
  • Relies on BuildExclusionsBuildItem#isExcluded to potentially exclude what has been annotated with @ConfigProperties and @ConfigPrefix
  • Adds tests to validate that we can exclude a @ConfigProperties if applied on a class and a @ConfigPrefix if applied on a field or parameter.
  • Adds some documentation about the feature

@quarkus-bot quarkus-bot bot added area/arc Issue related to ARC (dependency injection) area/documentation labels May 7, 2021
@quarkus-bot
Copy link

quarkus-bot bot commented May 7, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 63bbb3b

Status Name Step Test failures Logs Raw logs
Initial JDK 11 Build Build ⚠️ Check → Logs Raw logs
🚫 Calculate Test Jobs
🚫 Devtools Tests - JDK ${{ matrix.java.name }}
🚫 Devtools Tests - JDK 11 Windows
🚫 Gradle Tests - JDK 11 ${{ matrix.os.family }}
🚫 JVM Tests - JDK ${{ matrix.java.name }}
🚫 JVM Tests - JDK 11 Windows
🚫 Maven Tests - JDK ${{ matrix.java.name }}
🚫 Maven Tests - JDK 11 Windows
🚫 MicroProfile TCKs Tests
🚫 Native Tests - ${{ matrix.category }}

@essobedo
Copy link
Contributor Author

essobedo commented May 7, 2021

@geoand Hi, WDYT of this PR and related feature?

@geoand
Copy link
Contributor

geoand commented May 8, 2021

Thanks for taking this up.

I am not against it, but I would like to see how you intend to use this in a real application. Do you have an example?

@essobedo
Copy link
Contributor Author

essobedo commented May 9, 2021

I am not against it, but I would like to see how you intend to use this in a real application. Do you have an example?

@geoand I described briefly one potential use case in the doc part here.

Another example, let's say that I have applications A and B, the code is mostly common, only a slight part of the code is specific to A and B.
For this use case, I have 2 options:

  1. I could have 3 projects (common, specific-a and specific-b)
  2. I could have only one project that I build twice with different build profiles (A and B) leveraging build conditions.

Since the difference between A and B is very small, I finally chose option # 2 and because of this choice, I face a situation where I have a specific part of the configuration of application A that is mapped thanks to ConfigProperties which prevents application B to start because it is missing. The idea of this PR is to avoid this problem by using build conditions on ConfigProperties.

@geoand
Copy link
Contributor

geoand commented May 9, 2021

I am not against it, but I would like to see how you intend to use this in a real application. Do you have an example?

@geoand I described briefly one potential use case in the doc part here.

I saw that, but the example is not exactly concrete. In the abstract it looks OK, but it doesn't help me to justify the extra complexity.

Another example, let's say that I have applications A and B, the code is mostly common, only a slight part of the code is specific to A and B.
For this use case, I have 2 options:

  1. I could have 3 projects (common, specific-a and specific-b)
  2. I could have only one project that I build twice with different build profiles (A and B) leveraging build conditions.

Since the difference between A and B is very small, I finally chose option # 2 and because of this choice, I face a situation where I have a specific part of the configuration of application A that is mapped thanks to ConfigProperties which prevents application B to start because it is missing. The idea of this PR is to avoid this problem by using build conditions on ConfigProperties.

Again, can you show some specific example?

@essobedo
Copy link
Contributor Author

essobedo commented May 9, 2021

Let's say that application A needs a configuration for a service that only A needs.

The configuration of this specific configuration is mapped as next:

@ConfigProperties(prefix = "service")
public class ServiceConfiguration {
    public String url;
    public String user;
    public String password;
}

The related service:

@IfBuildProfile("a")
public class Service {

    @Inject
    ServiceConfiguration config;
...

Since Service is scoped to the build profile a, I expect Quarkus to raise an exception if a configuration key is missing (java.util.NoSuchElementException: SRCFG02004: Required property service.user not found) only when I start the application A but with the current code I get the same error when I start the application B because the processor managing the annotation @ConfigProperties is not aware of build conditions.

With this PR proposal, I can add a build condition to the class ServiceConfiguration to avoid this problem as next:

@IfBuildProfile("a")
@ConfigProperties(prefix = "service")
public class ServiceConfiguration {
    public String url;
    public String user;
    public String password;
}

@geoand
Copy link
Contributor

geoand commented May 10, 2021

Understood, thanks

@geoand geoand merged commit 489167f into quarkusio:main May 10, 2021
@quarkus-bot quarkus-bot bot added this to the 2.0 - main milestone May 10, 2021
@essobedo essobedo deleted the exclude-confg-properties branch May 10, 2021 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/arc Issue related to ARC (dependency injection) area/documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow to disable ConfigProperties
2 participants