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

Are getters supported? #1230

Closed
jakub-bochenski opened this issue Sep 30, 2024 · 27 comments · Fixed by #1239
Closed

Are getters supported? #1230

jakub-bochenski opened this issue Sep 30, 2024 · 27 comments · Fixed by #1239
Labels
enhancement New feature or request

Comments

@jakub-bochenski
Copy link

jakub-bochenski commented Sep 30, 2024

I think they are:

If the member name is represented as a getter, the member name is taken from its property name equivalent, and then converted to its kebab-case format.
https://smallrye.io/smallrye-config/Main/config/mappings/

Yet, I can't get them to work, e.g.

java.util.NoSuchElementException: SRCFG00014: The config property graph.get-client-id is required but it could not be found in any config source

I'm using 3.9.1

@radcortez
Copy link
Member

Can you send me a copy of your mapping?

@jakub-bochenski
Copy link
Author

It's in Kotlin, but those should map to regular Java getters.

@ConfigMapping(prefix = "graph")
interface GraphOption {
    @get:WithDefault("https://graph.microsoft.com")
    val baseUrl: String
    val tenant: String
    val clientId: String
    val clientSecret: String
}

@jakub-bochenski
Copy link
Author

It decompiles to

@ConfigMapping(
   prefix = "graph"
)
@Metadata(
[..]
)
public interface GraphOption {
   @WithDefault("https://graph.microsoft.com")
   @NotNull
   String getBaseUrl();

   @NotNull
   String getTenant();

   @NotNull
   String getClientId();

   @NotNull
   String getClientSecret();
}

@radcortez
Copy link
Member

I've just added a test with your example, and it works:
f0a32ca

@jakub-bochenski
Copy link
Author

the thing is "the member name is taken from its property name equivalent" to me means that for a method named "getTenant" then it would look for a config key "tenant"

@jakub-bochenski
Copy link
Author

So I'd expect this to work:

            .withDefaultValue("graph.tenant", "tenant")
            .withDefaultValue("graph.client-id", "id")
            .withDefaultValue("graph.client-secret", "secret")

@radcortez
Copy link
Member

the thing is "the member name is taken from its property name equivalent" to me means that for a method named "getTenant" then it would look for a config key "tenant"

Sorry, the documentation is not clear. It is always supposed to be the method name because Java interfaces don't define member fields for getters.

@jakub-bochenski
Copy link
Author

It's very misleading. Why have a separate bullet for getters if there is no special handling for them?

I think adding special handling for getters would be nice to allow use of kotlin properties instead of methods

@radcortez
Copy link
Member

We discussed this in the very first implementation and decided not to do it:
#333 (comment)

Adding it now may break existent configuration and would require a compatibility mode. @dmlloyd, what do you think about this?

@dmlloyd
Copy link
Contributor

dmlloyd commented Oct 1, 2024

Maybe we could add it under an annotation? Something that is heritable from the interface to its methods, so you only have to do it once? e.g. @WithBeanStyleGetters or something (make it clear that most people shouldn't do it, perhaps?)

@radcortez
Copy link
Member

Or in @ConfigMapping? We already have the NamingStrategy there.

@jakub-bochenski
Copy link
Author

Personally I'd find it most useful to be able to configure it globally via SmallRyeConfig

@dmlloyd
Copy link
Contributor

dmlloyd commented Oct 1, 2024

I think @ConfigMapping would make sense, sort of, though I think our naming strategy has more to do with the output (how we combine words) than the input (how we understand words), so it'd still have to be a separate property in that case.

From there it might be possible to relay it into a global setting on the API, if someone really wanted to do so (I don't recall whether we have such a flag for setting the default naming strategy though).

@radcortez
Copy link
Member

I think @ConfigMapping would make sense, sort of, though I think our naming strategy has more to do with the output (how we combine words) than the input (how we understand words), so it'd still have to be a separate property in that case.

Yes, as separate property.

From there it might be possible to relay it into a global setting on the API, if someone really wanted to do so (I don't recall whether we have such a flag for setting the default naming strategy though).

This has been requested in #1091, and I ended up discarding it. It is not trivial to implement because it would require some adjustments to our generator to make the implementation less static.

@jakub-bochenski
Copy link
Author

hey, that's nice but I'd really like to configure this globally instead of having to write (and potentially forget) this on every config class.

Should I open another issue for that?

@dmlloyd
Copy link
Contributor

dmlloyd commented Oct 15, 2024

Is there some way we can detect that an interface is a Kotlin interface at run time? Maybe we could automatically detect this and avoid having to have a global configuration switch, which brings with it other problems.

@jakub-bochenski
Copy link
Author

That would cover my current use case

I guess the @metadata annotation is one point of detection?

OTOH the global switch could be useful in other contexts, e.g. somebody is migrating from Boot's @ConfigurationProperties

@jakub-bochenski
Copy link
Author

PS. Also I'd be wary about adding to much magical autoconfiguration. Personally I'd rather configure it as a global explicit setting. IMO that's a good price for better clarity.

@jakub-bochenski
Copy link
Author

jakub-bochenski commented Oct 15, 2024

@dmlloyd
Copy link
Contributor

dmlloyd commented Oct 15, 2024

So we'd probably want to do something like:

boolean isKotlin = Stream.of(interface_.getDeclaredAnnotations()).anyMatch(i -> i.getClass().getName().equals("kotlin.Metadata"));

The only possible issue is that this would return false (I think?) if the interface was compiled with Kotlin but Kotlin is not present at run time (which is an unlikely scenario since I assume nothing would work then).

@jakub-bochenski
Copy link
Author

I'm not sure ATM if the option implemented in #1239 is exclusive or are plain names also matched?
Might be nice to make it clear in the docs.

If it's exclusive the enabling it automatically would break existing kotlin usage

@dmlloyd
Copy link
Contributor

dmlloyd commented Oct 15, 2024

Is Kotlin using a mixture of bean-style names and fluent-style names for getters?

@radcortez
Copy link
Member

radcortez commented Oct 15, 2024

hey, that's nice but I'd really like to configure this globally instead of having to write (and potentially forget) this on every config class.

I understand the inconvenience. Right now, this is the best we can do. Adding a global switch requires a considerable amount of effort. The mapping implementation generation only takes into account the interface metadata. Adding a global switch would require adding config into the mix, plus generating dynamic pieces in the implementation to account for the flag change. Also, in the case of Quarkus, where the implementations are generated during build time, we would either have to fix that configuration or generate the implementation with config awareness so they can adjust to the flag. All doable, sure, but again, requiring a considerable amount of effort.

which brings with it other problems.

Even if we add the flag, another mapping in the classpath from a dependency may break with the global behavior, making it useless.

Might want to check this out re detecting Kotlin https://github.com/spring-projects/spring-framework/blob/a143b57d4b397f5778428be40cc7a5083c42ee89/spring-core/src/main/java/org/springframework/util/KotlinUtils.java#L50

The problem with Kotlin support is that you require deep integration with the Kotlin API, to detect certain constructs. That makes sense, as these constructs are not native to Java. We definitely don't want to introduce Kotlin as a core dependency. From our Quarkus experience, fixing a Kotlin version and having all Kotlin-aware libraries behave correctly is tricky. Ideally, we would need to introduce extension points in a few places to allow having a separate module with the Kotlin integration and have a clear separation. I've discussed this as well here #844 and #1098. As you can imagine, this would also require considerable work. I'm happy to do some of the work, but until now, no one else has volunteered to help and we have been working in other stuff.

@jakub-bochenski
Copy link
Author

@dmlloyd I mean this is what I would write in kotlin now. If plain names are not supported together with getters autodetecting kotlin would break it

@ConfigMapping(prefix = "graph")
interface GraphOption {
    @WithDefault("https://graph.microsoft.com")
    fun baseUrl(): String

    fun tenant(): String

    fun clientId(): String

    fun clientSecret(): String
}

@jakub-bochenski
Copy link
Author

The problem with Kotlin support is that you require deep integration with the Kotlin API,

@radcortez I wasn't advocating for that. In fact I wrote I'd advise against it. But since @dmlloyd wanted to do it I gave some links to how spring does that

@dmlloyd
Copy link
Contributor

dmlloyd commented Oct 17, 2024

@dmlloyd I mean this is what I would write in kotlin now. If plain names are not supported together with getters autodetecting kotlin would break it

@ConfigMapping(prefix = "graph")
interface GraphOption {
    @WithDefault("https://graph.microsoft.com")
    fun baseUrl(): String

    fun tenant(): String

    fun clientId(): String

    fun clientSecret(): String
}

I'm not a kotlin person so I don't know what you mean. This is why I asked my question: "Is Kotlin using a mixture of bean-style names and fluent-style names for getters?"

Normally we just implement the interface directly. Users don't generally implement it. Is that what you're trying to do - implement the interface with some kind of automatic kotlin bean object?

@jakub-bochenski
Copy link
Author

jakub-bochenski commented Oct 17, 2024

No, when you write that in kotlin you get a "plain" java interface as expected by smallrye-config

The kotlin code above decompiles to this java code:

@ConfigMapping(
   prefix = "graph"
)
@Metadata(
[..]
)
public interface GraphOption {
   @WithDefault("https://graph.microsoft.com")
   @NotNull
   String baseUrl();

   @NotNull
   String tenant();

   @NotNull
   String clientId();

   @NotNull
   String clientSecret();
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
3 participants