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

Expose headers from native client request after request is committed #27768

Conversation

sokomishalov
Copy link
Contributor

Fixes issues like that, when public API does not expose headers that have been set by connector implementation.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Dec 6, 2021
@sokomishalov sokomishalov force-pushed the feature/fix-get-headers-implementation branch 2 times, most recently from 03e76b0 to 104a18f Compare December 6, 2021 10:05
Copy link
Contributor

@rstoyanchev rstoyanchev left a comment

Choose a reason for hiding this comment

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

The change might not be implemented in exactly this way. For example we don't use protected fields.

Rather than providing a solution, please update the description of the pull request in order to describe the actual problem, and not just reference some external issue which requires additional context.

@rstoyanchev rstoyanchev added in: web Issues in web modules (web, webmvc, webflux, websocket) status: waiting-for-feedback We need additional information before we can continue labels Dec 6, 2021
@sokomishalov
Copy link
Contributor Author

@rstoyanchev Fair enough
The problem is that ClientHttpRequest::getHeaders contain only headers that have been applied via WebClient API and don't contain headers applied by an underlying connector, (i.e. "user-agent").
Regarding implementation with restriction on protected fields - should this logic be duplicated to the ReactorClientHttpRequest?

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Dec 7, 2021
Copy link
Contributor

@rstoyanchev rstoyanchev left a comment

Choose a reason for hiding this comment

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

Thanks for elaborating.

We have a NettyHeadersAdapter that's used to expose the underlying headers on the response. We can enable the same on the request side. So, rather than exposing all the protected fields, I would suggest a protected initReadOnlyHeaders method that can be overridden in sub-classes. For Reactor Netty, it would be:

@Override
protected HttpHeaders initReadOnlyHeaders() {
	return HttpHeaders.readOnlyHttpHeaders(new NettyHeadersAdapter(request.requestHeaders()));
}

Let me know if you plan to update the PR, or otherwise I'll take it from here.

@rstoyanchev rstoyanchev added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on status: feedback-provided Feedback has been provided labels Jan 7, 2022
@rstoyanchev rstoyanchev added this to the 6.0.0-M2 milestone Jan 7, 2022
@rstoyanchev rstoyanchev changed the title Fix ReactorClientHttpRequest::getHeaders implementation to provide actually committed headers to the public API ReactorClientHttpRequest#getHttpHeaders to expose all headers from native request after request is committed Jan 7, 2022
@sokomishalov sokomishalov force-pushed the feature/fix-get-headers-implementation branch from 104a18f to bdc5207 Compare January 10, 2022 08:52
@sokomishalov
Copy link
Contributor Author

@rstoyanchev totally makes sense, I've reworked it according to your recommendations (netty, apache http 5, jetty).
p.s. I've noticed you marked these changes to the 6.x milestone - is there any chance this enhancement to be backported on 5.x ?

@sokomishalov sokomishalov force-pushed the feature/fix-get-headers-implementation branch from bdc5207 to 7f648e3 Compare January 10, 2022 11:09
@rstoyanchev rstoyanchev self-assigned this Jan 11, 2022
@rstoyanchev rstoyanchev modified the milestones: 6.0.0-M2, 5.3.15 Jan 11, 2022
@rstoyanchev rstoyanchev changed the title ReactorClientHttpRequest#getHttpHeaders to expose all headers from native request after request is committed Expose headers from native client request after request is committed Jan 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants