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

Add @JsonView support to quarkus-resteasy-reactive-jackson #13561

Merged
merged 2 commits into from Nov 30, 2020

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Nov 30, 2020

Fixes: #13566

Set<String> classesNeedingReflectionOnMethods = new HashSet<>();
for (ClassInfo resourceClass : resourceClasses) {
if (resourceClass.annotations().containsKey(JSON_VIEW)) {
classesNeedingReflectionOnMethods.add(resourceClass.name().toString());
Copy link
Member

Choose a reason for hiding this comment

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

Sorry to be that guy but shouldn't we do this in the quarkus-jackson extension given it's a generic Jackson annotation?

It could be used in other extensions then.

And I would do it for all @JsonView annotations in the classpath.

Copy link
Contributor Author

@geoand geoand Nov 30, 2020

Choose a reason for hiding this comment

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

No, because this reflection is only needed in order to make resteasy-reactive work. Normally RESTEasy reactive does not have to perform reflection on the resource method, but in this case (and in other that we will surely find later on) it does.
That means that this reflection is resteasy-reactive specific, not Jackson specific.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe I'm missing something but if we do need reflection for the class referenced in JsonView, I think it's a general requirement.

I'm not sure though if you added reflection for that?

Copy link
Contributor Author

@geoand geoand Nov 30, 2020

Choose a reason for hiding this comment

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

We are not doing reflection for the class referenced in the annotation, but for the class that contains the resource method in question.
Perhaps the variables names are not clear enough?

Copy link
Member

Choose a reason for hiding this comment

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

They are clear but I'm still puzzled on how it can work without having the class in @JsonView registered for reflection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do you think the value needs to be registered? Jackson almost certainly doesn't need to do any reflection on this class. It probably just has to call isAssignable on the class

@geoand geoand added this to the 1.11 - master milestone Nov 30, 2020
@geoand geoand merged commit dd8c39d into quarkusio:master Nov 30, 2020
@geoand geoand deleted the json-view branch November 30, 2020 19:48
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.

Add Jackson's @JsonView support to RESTEasy Reactive
3 participants