-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Register implicit converters for reflection #32162
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
67 changes: 67 additions & 0 deletions
67
...tests/smallrye-config/src/main/java/io/quarkus/it/smallrye/config/ImplicitConverters.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| package io.quarkus.it.smallrye.config; | ||
|
|
||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.Optional; | ||
|
|
||
| import io.smallrye.config.ConfigMapping; | ||
| import io.smallrye.config.WithDefault; | ||
|
|
||
| @ConfigMapping(prefix = "implicit.converters") | ||
| public interface ImplicitConverters { | ||
| @WithDefault("value") | ||
| Optional<ImplicitOptional> optional(); | ||
|
|
||
| @WithDefault("value") | ||
| List<ImplicitElement> list(); | ||
|
|
||
| Map<String, ImplicitValue> map(); | ||
|
|
||
| class ImplicitOptional { | ||
| private final String value; | ||
|
|
||
| public ImplicitOptional(final String value) { | ||
| this.value = value; | ||
| } | ||
|
|
||
| public String getValue() { | ||
| return value; | ||
| } | ||
|
|
||
| public static ImplicitOptional of(String value) { | ||
| return new ImplicitOptional("converted"); | ||
| } | ||
| } | ||
|
|
||
| class ImplicitElement { | ||
| private final String value; | ||
|
|
||
| public ImplicitElement(final String value) { | ||
| this.value = value; | ||
| } | ||
|
|
||
| public String getValue() { | ||
| return value; | ||
| } | ||
|
|
||
| public static ImplicitElement of(String value) { | ||
| return new ImplicitElement("converted"); | ||
| } | ||
| } | ||
|
|
||
| class ImplicitValue { | ||
| private final String value; | ||
|
|
||
| public ImplicitValue(final String value) { | ||
| this.value = value; | ||
| } | ||
|
|
||
| public String getValue() { | ||
| return value; | ||
| } | ||
|
|
||
| public static ImplicitValue of(String value) { | ||
| return new ImplicitValue("converted"); | ||
| } | ||
| } | ||
| } |
30 changes: 30 additions & 0 deletions
30
...allrye-config/src/main/java/io/quarkus/it/smallrye/config/ImplicitConvertersResource.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| package io.quarkus.it.smallrye.config; | ||
|
|
||
| import jakarta.inject.Inject; | ||
| import jakarta.ws.rs.GET; | ||
| import jakarta.ws.rs.Path; | ||
| import jakarta.ws.rs.core.Response; | ||
|
|
||
| @Path("/implicit/converters") | ||
| public class ImplicitConvertersResource { | ||
| @Inject | ||
| ImplicitConverters implicitConverters; | ||
|
|
||
| @GET | ||
| @Path("/optional") | ||
| public Response optional() { | ||
| return Response.ok(implicitConverters.optional().get().getValue()).build(); | ||
| } | ||
|
|
||
| @GET | ||
| @Path("/list") | ||
| public Response list() { | ||
| return Response.ok(implicitConverters.list().get(0).getValue()).build(); | ||
| } | ||
|
|
||
| @GET | ||
| @Path("/map") | ||
| public Response map() { | ||
| return Response.ok(implicitConverters.map().get("key").getValue()).build(); | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
8 changes: 8 additions & 0 deletions
8
...sts/smallrye-config/src/test/java/io/quarkus/it/smallrye/config/ImplicitConvertersIT.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| package io.quarkus.it.smallrye.config; | ||
|
|
||
| import io.quarkus.test.junit.QuarkusIntegrationTest; | ||
|
|
||
| @QuarkusIntegrationTest | ||
| public class ImplicitConvertersIT extends ImplicitConvertersTest { | ||
|
|
||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@radcortez how come you don't need
methodshere? What's the purpose of registering the converter if we can't call theconvertmethod? What am I missing?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Config system creates by reflection single instances of each
Converterand callsconvertas required directly into the instance.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so that's why we need to register the classes, in order to get them to work with reflection and be able to instantiate them (through the default constructors that are registered by default when creating a
ReflectiveClassBuildItem).Now that's the part I don't get how it works. Since we are registering the class without invoking
methods()we are not asking GraalVM to registerconvertfor reflection as well. So I would expect the call to fail :/There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we don't call
convertby reflection, we lookup theConvertertype:https://github.com/smallrye/smallrye-config/blob/c3fcadb5408db6e4b6a356c0bbc88f28af269106/implementation/src/main/java/io/smallrye/config/ConfigMappingContext.java#L103-L124
And call
convertdirectly. As an example:https://github.com/smallrye/smallrye-config/blob/c3fcadb5408db6e4b6a356c0bbc88f28af269106/implementation/src/main/java/io/smallrye/config/ConfigMappingContext.java#L460-L479
Are you having an issue with this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting.
No, I am just trying to understand what's going on.
My curiosity comes from the fact that GraalVM doesn't know if a converter let's say
MyConverteris reachable, thus treating it as unreachable (otherwise we would have an open world). Now when we registerMyConverterfor reflection we make it reachable, but not its methods (since we don't register the methods as well) which, as a result, I would expect to be left out of the native executable, but apparently they don't.Thanks for the clarification @radcortez