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.

Sign up for GitHub

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

Jump to bottom

Improve MessageBodyHandler selection #9347

Merged
merged 7 commits into from
May 30, 2023
Merged

Improve MessageBodyHandler selection #9347

merged 7 commits into from
May 30, 2023

Conversation

andriy-dmytruk
Copy link
Contributor

  • Use the message body writer and reader order in selection
  • For writers, only select those that can consume a particular type

yawkat
yawkat reviewed May 30, 2023
View reviewed changes
http/src/main/java/io/micronaut/http/body/DefaultMessageBodyHandlerRegistry.java
newMediaTypeQualifier(type, mediaTypes, Produces.class)
// Do not put the type here since we are looking for writers that can process the type
// but beanLocator will provide types that can be injected into the searched type
Argument.of(MessageBodyWriter.class),
Copy link
Member

Choose a reason for hiding this comment

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

i dont think this change is correct

Copy link
Member

Choose a reason for hiding this comment

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

are you moving this logic to reduce down below? what is the actual change here? I dont understand what youre fixing and why it's fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The change is the difference between reader and writer:

  • A MessageBodyReader<T> would have T as a return type therefore for a type P we would be looking for any MessageBodyReader<T> such that T extends P.
  • A MessageBodyWriter<T> would have T as a method parameter therefore for a type P we would be looking for any MessageBodyWriter<T> such that P extends T.

The main difference is how the isAssignableFrom method is called.

Copy link
Member

Choose a reason for hiding this comment

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

so basically, the change is to make writer matching covariant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, and prevent the selection of a body writer for contravariant types, which would throw an error when calling the method.

@andriy-dmytruk
Copy link
Contributor Author

Looks good. Thanks, Graeme!

@graemerocher graemerocher merged commit b42eaef into 4.0.x May 30, 2023
5 checks passed
@graemerocher graemerocher deleted the writer-changes branch May 30, 2023 15:00
@sonarcloud
Copy link

sonarcloud bot commented May 30, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

87.1% 87.1% Coverage
0.0% 0.0% Duplication

@dstepanov dstepanov added the type: improvement A minor improvement to an existing feature label May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yawkat yawkat yawkat left review comments

@graemerocher graemerocher graemerocher approved these changes

Assignees
No one assigned
Labels
type: improvement A minor improvement to an existing feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@andriy-dmytruk @graemerocher @yawkat @dstepanov