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

Remove type from ResolverMapKey #36719

Merged
merged 2 commits into from
Oct 28, 2023

Conversation

manofthepeace
Copy link
Contributor

This is an attempt to fix #36067. The reproducer can still reproduce the issue by using different "types". Since the object mapper is per restclient, it should apply to any types used within it. The types should I believe not be checked.
With this attempt, the reproducer goes from 7 invocations to 2. I would appreciate help on finding out why it is still called twice.

reproducer: https://github.com/manofthepeace/quarkus-restclient-annotations-issue

Note: the type parameter is passed in a method chain that could potentially be cleaned up is this is the right approach, which I am not sure.

cc @geoand

many thanks,
Alex

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.

Makes sense!

@manofthepeace
Copy link
Contributor Author

manofthepeace commented Oct 27, 2023

@geoand I have found the cause of the why the@ClientObjectMapper was called twice. It is because the contextResolverMap existed in both ClientJacksonMessageBodyReader and ClientJacksonMessageBodyWriter. I think they can both use the same map as for both, the object mapper will be the same, as the annotation in the rest client does not specify if it is for reading or writing, in contrast to the server counterpart where ObjectReader and ObjectWriter can be specified separately.

I have added a second commit to get your opinion. I can adjust / squash if you find it is ok.

Before publishing the pr i'll try soaking it in my main app and see if it does what it should there. Seems ok after testing in my app.

Thank you.

@manofthepeace manofthepeace marked this pull request as ready for review October 27, 2023 20:32
@geoand
Copy link
Contributor

geoand commented Oct 28, 2023

Makes sense, thanks!

@quarkus-bot
Copy link

quarkus-bot bot commented Oct 28, 2023

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@geoand geoand merged commit 5afc0ec into quarkusio:main Oct 28, 2023
23 of 36 checks passed
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Oct 28, 2023
@quarkus-bot quarkus-bot bot added this to the 3.6 - main milestone Oct 28, 2023
@quarkus-bot
Copy link

quarkus-bot bot commented Oct 28, 2023

Milestone is already set for some of the items:

We haven't automatically updated the milestones for these items.

This message is automatically generated by a bot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/rest kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@ClientObjectMapper should run only once
3 participants