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

Spring Config: Using @ConfigurationProperties to inject lists does not work #19365

Closed
Sgitario opened this issue Aug 12, 2021 · 15 comments · Fixed by #19961
Closed

Spring Config: Using @ConfigurationProperties to inject lists does not work #19365

Sgitario opened this issue Aug 12, 2021 · 15 comments · Fixed by #19961
Labels
area/config area/spring Issues relating to the Spring integration kind/enhancement New feature or request
Milestone

Comments

@Sgitario
Copy link
Contributor

Describe the bug

When using:

@ConfigurationProperties("lists")
public class ListWiringProperties {

    // Cover injection of lists of strings;
    public List<String> strings;

    // Cover injection of classes
    public List<Person> persons;

    public static class Person {
        public String name;
        public int age;

        @Override
        public String toString() {
            return String.format("person[%s:%s]", name, age);
        }
    }
}

And we inject it into one controller / or resource:

@RestController
@RequestMapping("/collections")
public class CollectionsController {
    @Autowired
    ListWiringProperties listProperties;
 
   // ...
}

And the following properties:

lists.strings[0]=Value 1

lists.persons[0].name=Sarah
lists.persons[0].age=19
lists.persons[1].name=Terminator
lists.persons[1].age=999

It fails to start with the following exception:

java.lang.UnsupportedOperationException: Mapping of YAML Configuration has been removed from io.quarkus.arc.config.ConfigProperties. Please use io.smallrye.config.ConfigMapping instead, which is a safer alternative.

Which is odd as I'm not using neither YAML nor @ConfigProperties (but @ConfigurationProperties from the Spring extension).

Expected behavior

I see that what we're doing internally is to map the @ConfigurationProperties annotation to @ConfigProperties.
But I think we should map it to use the @ConfigMapping annotation and support the wiring of complex annotations as Spring does.

Note that this is also not working for maps.

Actual behavior

No response

How to Reproduce?

No response

Output of uname -a or ver

No response

Output of java -version

No response

GraalVM version (if different from Java)

No response

Quarkus version or git rev

No response

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

No response

@Sgitario Sgitario added the kind/bug Something isn't working label Aug 12, 2021
@quarkus-bot
Copy link

quarkus-bot bot commented Aug 12, 2021

/cc @geoand

@quarkus-bot quarkus-bot bot added the area/spring Issues relating to the Spring integration label Aug 12, 2021
@Sgitario Sgitario changed the title Spring Config: Using @ConfigurationProperties to inject collections does not work Spring Config: Using @ConfigurationProperties to inject lists does not work Aug 12, 2021
@radcortez
Copy link
Member

java.lang.UnsupportedOperationException: Mapping of YAML Configuration has been removed from io.quarkus.arc.config.ConfigProperties. Please use io.smallrye.config.ConfigMapping instead, which is a safer alternative.

Which is odd as I'm not using neither YAML nor @ConfigProperties (but @ConfigurationProperties from the Spring extension).

Most likely you have the config-yaml extension in the classpath. This raises the exception, even if you don't have an application.yaml file.

Expected behavior

I see that what we're doing internally is to map the @ConfigurationProperties annotation to @ConfigProperties.
But I think we should map it to use the @ConfigMapping annotation and support the wiring of complex annotations as Spring does.

The problem is that @ConfigMapping was never intended to work for classes, and we hope to remove any configuration-based approach using classes in the future. May I ask why not use @ConfigMapping directly?

@Sgitario
Copy link
Contributor Author

java.lang.UnsupportedOperationException: Mapping of YAML Configuration has been removed from io.quarkus.arc.config.ConfigProperties. Please use io.smallrye.config.ConfigMapping instead, which is a safer alternative.

Which is odd as I'm not using neither YAML nor @ConfigProperties (but @ConfigurationProperties from the Spring extension).

Most likely you have the config-yaml extension in the classpath. This raises the exception, even if you don't have an application.yaml file.

Expected behavior

I see that what we're doing internally is to map the @ConfigurationProperties annotation to @ConfigProperties.
But I think we should map it to use the @ConfigMapping annotation and support the wiring of complex annotations as Spring does.

The problem is that @ConfigMapping was never intended to work for classes, and we hope to remove any configuration-based approach using classes in the future. May I ask why not use @ConfigMapping directly?

@radcortez the only dependencies I have in my pom.xml are:

  • quarkus-spring-boot-properties
  • quarkus-spring-web

These two dependencies bring the next dependencies:

[jcarvaja@localhost spring-properties]$ mvn dependency:tree | grep quarkus
[INFO] -----------------< io.quarkus.ts.qe:spring-properties >-----------------
[INFO] io.quarkus.ts.qe:spring-properties:jar:1.0.0-SNAPSHOT
[INFO] +- io.quarkus:quarkus-spring-boot-properties:jar:999-SNAPSHOT:compile
[INFO] |  +- io.quarkus:quarkus-spring-boot-properties-api:jar:2.1.SP1:compile
[INFO] |  +- io.quarkus:quarkus-spring-core-api:jar:5.2.SP4:compile
[INFO] |  \- io.quarkus:quarkus-arc:jar:999-SNAPSHOT:compile
[INFO] |     +- io.quarkus.arc:arc:jar:999-SNAPSHOT:compile
[INFO] +- io.quarkus:quarkus-spring-web:jar:999-SNAPSHOT:compile
[INFO] |  +- io.quarkus:quarkus-resteasy-jackson:jar:999-SNAPSHOT:compile
[INFO] |  |  +- io.quarkus:quarkus-resteasy:jar:999-SNAPSHOT:compile
[INFO] |  |  |  +- io.quarkus:quarkus-vertx-http:jar:999-SNAPSHOT:compile
[INFO] |  |  |  |  +- io.quarkus:quarkus-security-runtime-spi:jar:999-SNAPSHOT:compile
[INFO] |  |  |  |  +- io.quarkus:quarkus-mutiny:jar:999-SNAPSHOT:compile
[INFO] |  |  |  |  |  +- io.quarkus:quarkus-smallrye-context-propagation:jar:999-SNAPSHOT:compile
[INFO] |  |  |  |  +- io.quarkus:quarkus-vertx-http-dev-console-runtime-spi:jar:999-SNAPSHOT:compile
[INFO] |  |  |  |  +- io.quarkus.security:quarkus-security:jar:1.1.4.Final:compile
[INFO] |  |  |  |  +- io.quarkus:quarkus-vertx-core:jar:999-SNAPSHOT:compile
[INFO] |  |  |  |  |  +- io.quarkus:quarkus-netty:jar:999-SNAPSHOT:compile
[INFO] |  |  |  \- io.quarkus:quarkus-resteasy-server-common:jar:999-SNAPSHOT:compile
[INFO] |  |  |     \- io.quarkus:quarkus-resteasy-common:jar:999-SNAPSHOT:compile
[INFO] |  |  +- io.quarkus:quarkus-jackson:jar:999-SNAPSHOT:compile
[INFO] |  +- io.quarkus:quarkus-spring-di:jar:999-SNAPSHOT:compile
[INFO] |  |  \- io.quarkus:quarkus-spring-beans-api:jar:5.2.SP4:compile
[INFO] |  +- io.quarkus:quarkus-spring-web-api:jar:5.2.SP4:compile
[INFO] |  +- io.quarkus:quarkus-spring-webmvc-api:jar:5.2.SP4:compile
[INFO] |  \- io.quarkus:quarkus-spring-context-api:jar:5.2.SP4:compile
[INFO] +- io.quarkus.qe:quarkus-test-containers:jar:0.0.6:test
[INFO] |  +- io.quarkus.qe:quarkus-test-core:jar:0.0.6:test
[INFO] +- io.quarkus.qe:quarkus-test-openshift:jar:0.0.6:test
[INFO] |  +- io.quarkus.qe:quarkus-test-images:jar:0.0.6:test
[INFO] +- io.quarkus:quarkus-junit5:jar:999-SNAPSHOT:test
[INFO] |  +- io.quarkus:quarkus-bootstrap-core:jar:999-SNAPSHOT:test
[INFO] |  |  +- io.quarkus:quarkus-bootstrap-app-model:jar:999-SNAPSHOT:test
[INFO] |  |  +- io.quarkus:quarkus-bootstrap-maven-resolver:jar:999-SNAPSHOT:test
[INFO] |  |  +- io.quarkus:quarkus-bootstrap-gradle-resolver:jar:999-SNAPSHOT:test
[INFO] |  +- io.quarkus:quarkus-test-common:jar:999-SNAPSHOT:test
[INFO] |  |  +- io.quarkus:quarkus-core-deployment:jar:999-SNAPSHOT:test
[INFO] |  |  |  +- io.quarkus.gizmo:gizmo:jar:1.0.9.Final:test
[INFO] |  |  |  +- io.quarkus:quarkus-class-change-agent:jar:999-SNAPSHOT:test
[INFO] |  |  |  +- io.quarkus:quarkus-devtools-utilities:jar:999-SNAPSHOT:test
[INFO] |  |  |  +- io.quarkus:quarkus-builder:jar:999-SNAPSHOT:test
[INFO] |  |  +- io.quarkus:quarkus-jsonp-deployment:jar:999-SNAPSHOT:test
[INFO] |  |  |  \- io.quarkus:quarkus-jsonp:jar:999-SNAPSHOT:test
[INFO] |  +- io.quarkus:quarkus-core:jar:999-SNAPSHOT:compile
[INFO] |  |  +- io.quarkus:quarkus-ide-launcher:jar:999-SNAPSHOT:compile
[INFO] |  |  +- io.quarkus:quarkus-development-mode-spi:jar:999-SNAPSHOT:compile
[INFO] |  |  \- io.quarkus:quarkus-bootstrap-runner:jar:999-SNAPSHOT:compile

Note that I'm not using either @ConfigProperties or @ConfigMapping, but @ConfigurationProperties (from the Spring extension).

@radcortez
Copy link
Member

The exception you see is thrown by the config-yaml extension here:

throw new UnsupportedOperationException(
"Mapping of YAML Configuration has been removed from io.quarkus.arc.config.ConfigProperties. Please use io.smallrye.config.ConfigMapping instead, which is a safer alternative.");
. So, unless I'm missing something it should be present in the classpath.

Note that I'm not using either @ConfigProperties or @ConfigMapping, but @ConfigurationProperties (from the Spring extension).

I understood, I was just asking if there was any strong reason to prefer to use @ConfigurationProperties instead of @ConfigMapping.

@Sgitario
Copy link
Contributor Author

Sgitario commented Aug 13, 2021

The exception you see is thrown by the config-yaml extension here:

throw new UnsupportedOperationException(
"Mapping of YAML Configuration has been removed from io.quarkus.arc.config.ConfigProperties. Please use io.smallrye.config.ConfigMapping instead, which is a safer alternative.");

. So, unless I'm missing something it should be present in the classpath.

Note that I'm not using either @ConfigProperties or @ConfigMapping, but @ConfigurationProperties (from the Spring extension).

I understood, I was just asking if there was any strong reason to prefer to use @ConfigurationProperties instead of @ConfigMapping.

Using @ConfigurationProperties is what Spring users would do to start using Quarkus. Indeed, this is what the quarkus-spring-boot-properties extension is meant to.

I'm aware that this extension is just a limited wrapper of what Spring indeed does, but I think reporting the current limitations can help to:
(1) refactor what we could improve within the Spring extension - for ex: use @ConfigMapping instead of @ConfigProperties internally
(2) document what we can't achieve and let users how to approach these limitations using the Quarkus capabilities (for example: if there is a strong technical impediment to refactor @ConfigurationProperties to map lists, then we should document it and explain Spring users that want to try Quarkus out, to use the equivalent approach.

@radcortez
Copy link
Member

(1) refactor what we could improve within the Spring extension - for ex: use @ConfigMapping instead of @ConfigProperties internally

Unfortunately, that is not possible, because as you know, @ConfigMapping is intended for interfaces only, while Spring @ConfigurationProperties applies to classes. We could bridge between the two, and use the Spring annotation instead, but you still require an interface, and I don't think that is what you expect.

(2) document what we can't achieve and let users how to approach these limitations using the Quarkus capabilities (for example: if there is a strong technical impediment to refactor @ConfigurationProperties to map lists, then we should document it and explain Spring users that want to try Quarkus out, to use the equivalent approach.

At this point, we probably need to consider deprecating Quarkus support for Spring @ConfigurationProperties, since we also deprecated Quarkus @ConfigProperties. Some of the big issues with classes are that they require heavy use of reflection to modify the fields (if they are not public) or to call setters on them. It may not even be possible to even call them due to JPMS restrictions and how the class is exported. Fields may have been already initialized (what should we do if a collection already has elements added by the constructors? are these considered defaults? should we override them?), the class may have mutable state (config instances are global and should be immutable, so having a way to change them from anywhere is dangerous). There are some more details but in the end, we thought it was too much work for little gain, creates too many edge cases, compared to a clean approach using an interface.

Would you consider moving to @ConfigMapping and interfaces and dropping the Spring Extension? :)

@geoand
Copy link
Contributor

geoand commented Aug 13, 2021

I don't think we can remove the extension as it is part of the product

@radcortez
Copy link
Member

Yes, I was not suggesting to remove it now, it would still need to go through a deprecation cycle.

@Sgitario
Copy link
Contributor Author

Would you consider moving to @ConfigMapping and interfaces and dropping the Spring Extension? :)

:D I would never use the Spring extension to play with Quarkus, so I would have used the @ConfigMapping approach from the beginning.

But my duty was to check the Spring Properties Quarkus extension and see what users of Quarkus can expect of it. So, adding a note in the documentation or improve the exception handling to prevent this work for me.

@radcortez
Copy link
Member

Great. Thanks. I'll improve the documentation.

@fedinskiy
Copy link
Contributor

@radcortez is there a full list of functionality, which is not supported by Quarkus+spring boot properties? For example, the guide, you changed[1] only mentions Maps and not Lists.

[1] https://quarkus.io/guides/spring-boot-properties

@radcortez
Copy link
Member

Lists should work. Have you encountered an issue in that area?

Still, my recommendation is to use https://quarkus.io/guides/config-mappings :)

@fedinskiy
Copy link
Contributor

@radcortez "Using @ConfigurationProperties to inject lists does not work" is a name of the issue :) Reproduced with the same code.

I totally agree with recommendation, but my task is to check disabled tests, including the ones, made by my former colleague Sgitario, so I need to be sure, that this functionality either works or properly mentioned as non-working.

@radcortez
Copy link
Member

Yes, it does not work with the indexed property version, as in foo.bar[i]. This is specific to @ConfigMapping.

@fedinskiy
Copy link
Contributor

Is this behavior described somewhere? The only mention I was able to find is "Spring Boot @ConfigurationProperties has a few limitations. For instance, Map injection is not supported. Consider using Mapping configuration to objects"[1], which doesn't mention, that there also may be problems with, for example indexed property version in Lists.

[1] https://quarkus.io/guides/spring-boot-properties

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/config area/spring Issues relating to the Spring integration kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants