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

Introduce @RegisterForProxy to register interfaces of dynamic proxies for native image #35433

Closed
Eng-Fouad opened this issue Aug 21, 2023 · 8 comments · Fixed by #40913
Closed
Labels
Milestone

Comments

@Eng-Fouad
Copy link
Contributor

Eng-Fouad commented Aug 21, 2023

Description

Currently, the only option to register interfaces of dynamic proxies is to specify them in a configuration file (proxy-config.json):

[
 { "interfaces": [ "java.lang.AutoCloseable", "java.util.Comparator" ] },
 { "interfaces": [ "java.util.Comparator" ] },
 { "interfaces": [ "java.util.List" ] }
]
... -Dquarkus.native.additional-build-args=-H:DynamicProxyConfigurationFiles=proxy-config.json

See: Configure Dynamic Proxies Manually

Implementation ideas

Introduce new annotation @RegisterForProxy that is similar to @RegisterForReflection:

/**
 * Annotation that can be used to force an interface (including its super interfaces) to be registered for dynamic proxy generation in native image mode.
 */
@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.TYPE)
@Repeatable(RegisterForProxy.List.class)
public @interface RegisterForProxy {

    /**
     * Alternative interfaces that should actually be registered for dynamic proxy generation instead of the current interface.
     *
     * This allows for interfaces in 3rd party libraries to be registered without modification or writing an
     * extension. If this is set then the interface it is placed on is not registered for dynamic proxy generation, so this
     * should generally just be placed on an empty interface that is not otherwise used.
     */
    Class<?>[] targets() default {};
}

Usage:

@RegisterForProxy
public interface Foo {}

@RegisterForProxy
public interface Foo2 extends Bar2 {}

@RegisterForProxy(targets = {Foo1.class, Bar1.class})
@RegisterForProxy(targets = {Foo3.class, Bar3.class})
public interface ThirdPartyLibProxies {}
@geoand
Copy link
Contributor

geoand commented Aug 21, 2023

I think it makes sense. @zakkak WDYT?

@zakkak
Copy link
Contributor

zakkak commented Sep 6, 2023

I agree. Not sure if the includeSuperinterfaces and extraInterfaces are really necessary (especially the latter can be easily replaced by @RegisterForProxy(targets = ...)).

@gsmet
Copy link
Member

gsmet commented Sep 6, 2023

Not that we need to be extremely cautious about this one as ordering is important. We got bitten by that in the past.

gastaldi added a commit that referenced this issue May 31, 2024
Add @RegisterForProxy annotation
@quarkus-bot quarkus-bot bot added this to the 3.12 - main milestone May 31, 2024
@maxandersen
Copy link
Member

This was closed yesterday but I didn't spot response/change in code to accommodate for the ordering concern @gsmet mentions above is it no longer relevant?

@maxandersen maxandersen reopened this Jun 1, 2024
@Eng-Fouad
Copy link
Contributor Author

Eng-Fouad commented Jun 1, 2024

This was closed yesterday but I didn't spot response/change in code to accommodate for the ordering concern @gsmet mentions above is it no longer relevant?

I think classInfo.interfaceNames() returns the correct order of interfaces, am I right?

if (targetsValue == null) {
var classInfo = annotationInstance.target().asClass();
types.add(classInfo.name().toString());
classInfo.interfaceNames().forEach(dotName -> types.add(dotName.toString()));
} else {

Also, I added a note about interfaces order in docs:

[WARNING]
====
Note that the order of the specified proxy interfaces is significant. For more information, see link:https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/reflect/Proxy.html[Proxy javadoc].
====

@gastaldi
Copy link
Contributor

gastaldi commented Jun 1, 2024

For the record, I considered the note about interfaces order in docs would be enough for the concern @gsmet mentions, hence why I merged the PR

@Eng-Fouad
Copy link
Contributor Author

cc @gsmet @geoand

@gsmet gsmet removed this from the 3.12.0.CR1 milestone Jun 12, 2024
@maxandersen
Copy link
Member

assuming @gsmet is ok as this was merged so closing again.

@gsmet gsmet added this to the 3.12.0.CR1 milestone Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants