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

Propagate reason phrase of aborted request in JAX-RS Client #22994

Merged
merged 1 commit into from
Jan 21, 2022
Merged

Propagate reason phrase of aborted request in JAX-RS Client #22994

merged 1 commit into from
Jan 21, 2022

Conversation

fwippe
Copy link
Contributor

@fwippe fwippe commented Jan 18, 2022

This PR complements #22859 by providing the reason phrase of aborted requests in addition to their status code.

@geoand I hope you don't mind that I renamed method setExistingEntity to propagateAbortedWithEntityToResponse (I can undo that change otherwise, no problem). Please note, that the previous test in ClientRequestFilterAbortWithTestCase didn't check the code part where RestClientRequestContext.isCheckSuccessfulFamily == true. I added such a test so that we now cover both cases.

@geoand geoand marked this pull request as ready for review January 19, 2022 05:51
@geoand
Copy link
Contributor

geoand commented Jan 19, 2022

Thanks!

@quarkus-bot
Copy link

quarkus-bot bot commented Jan 19, 2022 •
edited

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 16788ff

Status Name Step Failures Logs Raw logs
✖ Initial JDK 11 Build Verify extension dependencies ⚠️ Check → Logs Raw logs

@geoand
Copy link
Contributor

geoand commented Jan 19, 2022

Can you please rebase the PR onto main?

Thanks

@geoand
Copy link
Contributor

geoand commented Jan 19, 2022

This will need another rebase when #22997 is in.

@quarkus-bot
Copy link

quarkus-bot bot commented Jan 19, 2022 •
edited

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building dfc7945

Status Name Step Failures Logs Raw logs
✖ Initial JDK 11 Build Verify extension dependencies ⚠️ Check → Logs Raw logs

@fwippe fwippe force-pushed the fwippe/jaxrs_client_abort_reason_phrase branch from dfc7945 to cee965e Compare January 19, 2022 07:45
@quarkus-bot quarkus-bot bot added the area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins label Jan 19, 2022
@fwippe fwippe force-pushed the fwippe/jaxrs_client_abort_reason_phrase branch from 240eded to a095e36 Compare January 19, 2022 08:00
@fwippe
Copy link
Contributor Author

fwippe commented Jan 19, 2022

OK, I rebased again after #22997 was in

@geoand
Copy link
Contributor

geoand commented Jan 19, 2022

Great, thanks!

@geoand geoand added triage/waiting-for-ci Ready to merge when CI successfully finishes and removed area/documentation area/core area/mongodb area/kubernetes area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins labels Jan 19, 2022
@gsmet
Copy link
Member

gsmet commented Jan 19, 2022

@FroMage @sberyozkin @michalszynkiewicz I know we had concerns at some point about security and propagating exception information. Could you have a look at this one and make sure it's not an issue? Better safe than sorry.

michalszynkiewicz
michalszynkiewicz reviewed Jan 19, 2022
View reviewed changes
...ain/java/org/jboss/resteasy/reactive/client/handlers/ClientSetResponseEntityRestHandler.java
if (value == null) {
responseContext.setEntityStream(null);
return;
private void propagateAbortedWithEntityToResponse(RestClientRequestContext restClientRequestContext) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

do I read it correctly that from this line there's no real change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right, it's just a minor refactoring

@michalszynkiewicz
Copy link
Member

Security-wise, I think this is safe

@FroMage
Copy link
Member

FroMage commented Jan 19, 2022

Security-wise, I think this is safe

Agreed.

@gsmet
Copy link
Member

gsmet commented Jan 19, 2022

@michalszynkiewicz @FroMage thanks for checking!

@gsmet
Copy link
Member

gsmet commented Jan 20, 2022

@michalszynkiewicz I let you merge it if you're happy with @fwippe 's answer.

@sberyozkin
Copy link
Member

+1, it is only part of the HTTP response status info

@gsmet gsmet merged commit aeb6bf6 into quarkusio:main Jan 21, 2022
@quarkus-bot quarkus-bot bot added this to the 2.8 - main milestone Jan 21, 2022
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jan 21, 2022
@gsmet
Copy link
Member

gsmet commented Jan 21, 2022

Thanks!

@gsmet gsmet modified the milestones: 2.8 - main, 2.7.0.Final Jan 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michalszynkiewicz michalszynkiewicz michalszynkiewicz left review comments

@geoand geoand geoand approved these changes

@mamadaliev mamadaliev mamadaliev approved these changes

Assignees
No one assigned
Labels
area/resteasy-reactive
Projects
None yet
Milestone
2.7.0.Final
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@fwippe @geoand @gsmet @michalszynkiewicz @FroMage @sberyozkin @mamadaliev