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

Fix invokeAsync with @PathParam annotations #1848

Closed
wants to merge 1 commit into from
Closed

Fix invokeAsync with @PathParam annotations #1848

wants to merge 1 commit into from

Conversation

fabienrousseau
Copy link

No description provided.

@asoldano
Copy link
Member

@fabienrousseau thanks for your contribution. Can you please provide a bit more information on the issue, how you're reproducing it, etc? A new jira issue (https://issues.jboss.org/browse/RESTEASY) would actually be great.

@fabienrousseau
Copy link
Author

Sorry, I already opened an issue but forgot to link it back to the PR: https://issues.jboss.org/browse/RESTEASY-2139

ClientContext context = new ClientContext(request, response, entityExtractorFactory);
return extractor.extractEntity(context);
}

protected ClientInvocation createRequest(Object[] args)
protected ClientInvocationBuilder createRequest(Object[] args)
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to be careful about backward compatibility in 3.6 PR. This changes protected method signature from public class from public WF module.

@asoldano + @ronsigal WDYT about this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point, @marekkopecky. It's probably not a problem, but it's good to be safe. @fabienrousseau, can you just move the line

builder.setInvocation(request);

to the end of createRequest(), where it would become

builder.setInvocation(clientInvocation);

?

@marekkopecky
Copy link
Contributor

Master PR would be useful as well ...

I check 3.6 branch, proxy + MP REST client, Single + CompletionStage, RESTEasy works as expected in my checks, IllegalArgumentException is no more thrown

@asoldano
Copy link
Member

Thanks @marekkopecky ; yes, if possible it would be really better not changing that method signature. The class is in a package named "internal", still we should try avoiding changing it this way.
Besides that, the PR here needs a test.
@fabienrousseau if you still expect us to merge this PR, can you please work on the two mentioned things? Otherwise we'll likely include the changes in another revisited PR. Thanks

@ronsigal ronsigal added the 3.6 label Feb 15, 2019
@asoldano
Copy link
Member

asoldano commented Mar 7, 2019

Replaced by #1900 , thanks anyway for the contribution!

@asoldano asoldano closed this Mar 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants