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

Support for Kotlin value types #1098

Open
FWest98 opened this issue Jan 29, 2024 · 3 comments
Open

Support for Kotlin value types #1098

FWest98 opened this issue Jan 29, 2024 · 3 comments
Labels
enhancement New feature or request

Comments

@FWest98
Copy link

FWest98 commented Jan 29, 2024

Currently, this library does not support Kotlin (inline) value types/classes for a config mapping setup.

For an example, let's consider the following definition:

@JvmInline
value class Size (private val value: Long)
class SizeConverter : Converter<Size> { ... }

@ConfigMapping
interface Settings {
    fun size(): Size
}

In this setup, explicitly requesting an instance of Size using getValue works fine, as expected since the target type is explicitly passed. However, for the configmap setup, Smallrye will see the size field for its backing type, Long. Then, conversion fails since the converter is defined for Size and not Long. Note that annotating the field with WithConverter does not work - Smallrye will throw an error that the types do not match.

I am afraid this is not a very easy fix: it seems like special, Kotlin-specific, reflection APIs are needed to find the "actual" value class type. For example, Jackson does not yet support this in mainline and has a separate experimental library to implement it: https://github.com/ProjectMapK/jackson-module-kogera

There do seem to be a few workarounds at the moment:

  • One could force Kotlin to make the field "boxed" by making the value class implement an interface and then setting the configmap field type to be the interface. However, that triggers another issue: Converter for interface type is ignored #931
  • One could write a custom converter for the backing field type (Long), but then this converter cannot be auto-discovered to prevent conflicts with normal uses of that backing type. Consequently, each configmap field needs to be annotated by hand.
  • Not using a value class - which is not always desirable given the overhead of boxing/unboxing a primitive type.
@radcortez
Copy link
Member

Yes, unfortunately, to be able to support some of the Kotlin native features, we are required to use specific Kotlin APIs when generating the model.

I guess the cleanest way is to duplicate the current generation model and enhance the pieces that Kotlin could benefit from. Having a single model with checks could work as well, but probably harder to maintain.

Relates to #844

Is this something you can help with?

@FWest98
Copy link
Author

FWest98 commented Jan 29, 2024

I feel like a clean approach would be to open up some of the classes in the current codebase, so we can write a separate smallrye-config-kotlin library that provides custom logic for those special usecases. Think of ConfigMappingInterface aspects. However, I do not have full insight in what needs to happen there to support it - the entire config mapping seems like a lot of magic to me and I am not entirely sure how it connects to the rest of the converter infrastructure (it seems to not be trivial considering #931)

Using custom interceptors, I have been able to at least support properties in config maps, in addition to normal functions. But with better reflection support and closer integration with the rest of the library, this can probably be done much neater and safer. My current approach just tries to strip get- prefixes from computed property names (since Kotlin properties in interface produce getter methods).

@radcortez
Copy link
Member

Think of ConfigMappingInterface aspects. However, I do not have full insight in what needs to happen there to support it - the entire config mapping seems like a lot of magic to me and I am not entirely sure how it connects to the rest of the converter infrastructure (it seems to not be trivial considering #931)

The ConfigMappingInterface only provides introspection information about the mapping class. From that information, a plain class is generated in https://github.com/smallrye/smallrye-config/blob/main/implementation/src/main/java/io/smallrye/config/ConfigMappingGenerator.java#L65 and then loaded in the classloader, to use as the implementation class and provide the actual values for each declared method. Once the class is generated, ConfigMappingInterface is no longer required.

The generated code is just a call to the programmatic API to create and retrieve each method value. In https://github.com/smallrye/smallrye-config/blob/main/implementation/src/test/java/io/smallrye/config/ObjectCreatorTest.java#L18, you can find handwritten implementations using the same APIs that are used when generating the class so you can have an idea.

Probably, we would require a Kotlin module to provide its view of ConfigMappingInterface and ConfigMappingGenerator. The introspection information for Kotlin will be richer than its Java counterpart, and we probably want to make different adjustments on how the code is generated, so I'm not sure how much of the original code we can reuse.

I can do the work to open up these mechanisms and allow them to be replaced by an overriding module. It would be great to get help with the Kotlin pieces since I'm not very proficient with Kotlin.

@radcortez radcortez added the enhancement New feature or request label Feb 1, 2024
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
Development

No branches or pull requests

2 participants