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

Copy default methods of a RestClient Reactive interface to the generated class #21692

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Nov 25, 2021

RestClient Reactive generates a ...$$CDIWrapper class for each
RestClient interface, and creates an implementation of each
RestClient method. This leaves non-RestClient methods (default
methods) on the interface, which means that if they are annotated
with an interceptor binding, the interceptor is not invoked.
This is because interceptor bindings are not inherited from
superinterfaces, only from superclasses (and while ArC has some
support for intercepting default methods from superinterfaces,
it doesn't extends that far).

This PR doesn't attempt to support intercepting default methods
in general, because that's a gray area. Instead, it fixes how
RestClient Reactive generates the class for a RestClient interface
(because RestClient interfaces are special in that they may define
class-based beans if annotated @RegisterRestClient). In addition
to RestClient methods, default methods from the RestClient
interface are also copied to the generated class. (The "copy"
delegates to the default method inherited from the superinterface,
which had to be added to Gizmo, hence the Gizmo update.) That itself
is enough for interceptors to start working.

Fixes #21674

@quarkus-bot
Copy link

quarkus-bot bot commented Nov 25, 2021

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

  • title should not end up with ellipsis (make sure the title is complete)

This message is automatically generated by a bot.

@Ladicek
Copy link
Contributor Author

Ladicek commented Nov 25, 2021

Draft because we need quarkusio/gizmo#93 merged and released first.

@quarkus-bot quarkus-bot bot added area/dependencies Pull requests that update a dependency file area/rest labels Nov 25, 2021
@Ladicek Ladicek changed the title Copy default methods of a RestClient Reactive interface to the genera… Copy default methods of a RestClient Reactive interface to the generated class Nov 25, 2021
…ted class

RestClient Reactive generates a `...$$CDIWrapper` class for each
RestClient interface, and creates an implementation of each
RestClient method. This leaves non-RestClient methods (`default`
methods) on the interface, which means that if they are annotated
with an interceptor binding, the interceptor is not invoked.
This is because interceptor bindings are not inherited from
superinterfaces, only from superclasses (and while ArC has some
support for intercepting `default` methods from superinterfaces,
it doesn't extends that far).

This PR doesn't attempt to support intercepting `default` methods
in general, because that's a gray area. Instead, it fixes how
RestClient Reactive generates the class for a RestClient interface
(because RestClient interfaces are special in that they may define
class-based beans if annotated `@RegisterRestClient`). In addition
to RestClient methods, `default` methods from the RestClient
interface are also copied to the generated class. (The "copy"
delegates to the `default` method inherited from the superinterface,
which had to be added to Gizmo, hence the Gizmo update.) That itself
is enough for interceptors to start working.
@Ladicek Ladicek force-pushed the restclient-reactive-copy-default-methods branch from a0ab2e6 to 6b19e71 Compare November 30, 2021 14:34
@Ladicek Ladicek marked this pull request as ready for review November 30, 2021 14:35
@Ladicek
Copy link
Contributor Author

Ladicek commented Nov 30, 2021

Rebased and marked ready for review, as Quarkus was updated to use Gizmo 1.0.10.Final.

@geoand
Copy link
Contributor

geoand commented Nov 30, 2021

Do we want to backport this to 2.5?

@Ladicek
Copy link
Contributor Author

Ladicek commented Nov 30, 2021

Good point, I think we should. Added a label.

@Ladicek
Copy link
Contributor Author

Ladicek commented Nov 30, 2021

CC @edeandrea

Copy link
Member

@michalszynkiewicz michalszynkiewicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's cool! Thanks @Ladicek !

@michalszynkiewicz michalszynkiewicz merged commit 6276695 into quarkusio:main Nov 30, 2021
@quarkus-bot quarkus-bot bot added this to the 2.6 - main milestone Nov 30, 2021
@gsmet gsmet modified the milestones: 2.6 - main, 2.5.1.Final Nov 30, 2021
@Ladicek Ladicek deleted the restclient-reactive-copy-default-methods branch December 1, 2021 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dependencies Pull requests that update a dependency file area/rest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

interceptor bound to a non-RestClient method on a RestClient interface is not called with RestClient Reactive
4 participants