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

Rest Client Reactive - fixed @RegisterProvider support #16757

Conversation

michalszynkiewicz
Copy link
Member

@michalszynkiewicz michalszynkiewicz commented Apr 23, 2021

related to #16702

Before the change, providers registered with the @RegisterProvider annotation were registered in the generated stub, on the WebTarget. While this is enough for most non-MP-specific cases, it is not for microprofile ones, like ResponseExceptionMapper.

This change introduces a new bean AnnotationRegisteredProviders that stores information about the provider classes coming from annotations. This bean is used when a rest client stub is built with RestClientBuilderImpl.
The info about the annotations is gathered at build-time. The bean has an abstract superclass to minimize the amount of generated bytecode.

@michalszynkiewicz michalszynkiewicz force-pushed the rest-client-reactive-fixes-for-providers branch from 383dac9 to ce75636 Compare April 23, 2021 13:35
@geoand
Copy link
Contributor

geoand commented Apr 24, 2021

👍🏼

I'll check it out on Monday

* @param generatedBeans build producer for generated beans
*/
@BuildStep
void registerProvidersFromAnnotations(CombinedIndexBuildItem indexBuildItem,
Copy link
Contributor

Choose a reason for hiding this comment

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

I am fairly certain that the generated class can be replaced by a synthetic bean which would make maintenance a lot easier

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll see if I can do that

Copy link
Member Author

@michalszynkiewicz michalszynkiewicz Apr 26, 2021

Choose a reason for hiding this comment

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

I created a version with synthetic bean: #16815
It has its own drawbacks: more code, usage of Class.forName and registering classes for reflection. I have a slight preference on this version but I don't mind if we merge the other.

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer #16815, but it's up to you as this is your baby :)

Copy link
Member Author

Choose a reason for hiding this comment

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

okay, let's go with this one then :)

If it's okay now, can you approve?

Copy link
Contributor

@geoand geoand Apr 26, 2021

Choose a reason for hiding this comment

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

As you wish :)

Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

I like the approach, I just added a couple minor and a more serious question about the implementation

Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@geoand geoand merged commit 1bd986e into quarkusio:main Apr 27, 2021
@quarkus-bot quarkus-bot bot added this to the 2.0 - main milestone Apr 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants