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

ConfigMapping doesn't handler inner classes #17545

Closed
pjgg opened this issue May 31, 2021 · 6 comments
Closed

ConfigMapping doesn't handler inner classes #17545

pjgg opened this issue May 31, 2021 · 6 comments
Labels
area/config kind/bug Something isn't working

Comments

@pjgg
Copy link
Contributor

pjgg commented May 31, 2021

Describe the bug

Quarkus Version: Upstream (2.x)
Deprecated class org.eclipse.microprofile.config.inject.ConfigProperties supports inner classes
For example:

import org.eclipse.microprofile.config.inject.ConfigProperties;

@ConfigProperties(prefix = "antagonist")
public class AntagonistConfiguration {
    public String message;
    public String name;
    public AntagonistWifeConfig wife;

    public static class AntagonistWifeConfig {
        public String name;
        public String message;
    }
}

application.properties

antagonist.name=Sheldon Plankton
antagonist.message=Hi, I am Sheldon Plankton
antagonist.wife.name=Karen
antagonist.wife.message=Hi, I am Karen

But the new io.smallrye.config.ConfigMapping class (that should be used instead of ConfigProperties) doesn't support inner classes:

Exception:

Caused by: java.lang.IllegalArgumentException: SRCFG00013: No Converter registered for class io.quarkus.qe.config.AntagonistConfiguration$AntagonistWifeConfig

This transition should be smooth for the users, or be supported.

Reproducer:
Project: https://github.com/pjgg/beefy-scenarios/tree/feat/remove-deprecated-code
Command: mvn clean verify -pl 022-quarkus-properties-config-all

@rsvoboda
Copy link
Member

CC @kenfinnigan @radcortez

@radcortez
Copy link
Member

Hi @pjgg,

The @ConfigMapping only supports interfaces. Any inner class should also be converted to an interface.

@rsvoboda
Copy link
Member

rsvoboda commented Jun 1, 2021

What was the reason to not support inner classes with @ConfigMapping?
I think there should be good migration path from ConfigProperties to ConfigMapping.

What suggested transition path for users who use @ConfigProperties with inner classes?

@radcortez
Copy link
Member

What was the reason to not support inner classes with @ConfigMapping?

From the experience we had, supporting classes adds way more complexity for little gain (fields visibility, reflection, types defaults values). Please check the additional discussion here: smallrye/smallrye-config#310

I think there should be good migration path from ConfigProperties to ConfigMapping.

We can add one to the documentation.

What suggested transition path for users who use @ConfigProperties with inner classes?

Just convert the classes to use interfaces instead. They can still be used as inner to the main mapping interface. Check this example: https://quarkus.io/version/main/guides/config-mappings#nested-groups

@pjgg
Copy link
Contributor Author

pjgg commented Jun 1, 2021

Thank you @radcortez is very well explained on the above documents!

The transition path for end-users is clear, I mean the annotation ConfigProperties is still valid. Is deprecated but is still working so, they are aware of the changes and there is a migration guide.

@radcortez
Copy link
Member

Are we ok to close this issue then?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/config kind/bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants