Skip to content

Optimize header removal in ForwardedHeaderFilter #27466

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

Conversation

GreenRecycleBin
Copy link
Contributor

The current implementation suggests that the request's headers are not
expected to change. Hence, it's not necessary to copy them.
Furthermore, it might be costly to do so if there are many headers.
Instead, cache only the request's header names for method getHeaderNames.

Methods getHeader and getHeaders delegate to the respective methods of
request if the header name is not in FORWARDED_HEADER_NAMES. Otherwise,
they return null or an empty Enumeration respectively.

@pivotal-cla
Copy link

@GreenRecycleBin Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-cla
Copy link

@GreenRecycleBin Thank you for signing the Contributor License Agreement!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Sep 25, 2021
@GreenRecycleBin GreenRecycleBin force-pushed the ForwardedHeaderRemovingRequest-should-not-cache-headers branch from c3ed30d to bb80534 Compare September 25, 2021 13:13
The current implementation suggests that the request's headers are not
expected to change. Hence, it's not necessary to copy them.
Furthermore, it might be costly to do so if there are many headers.
Instead, cache only the request's header names for method getHeaderNames.

Methods getHeader and getHeaders delegate to the respective methods of
request if the header name is not in FORWARDED_HEADER_NAMES. Otherwise,
they return null or an empty Enumeration respectively.
@GreenRecycleBin GreenRecycleBin force-pushed the ForwardedHeaderRemovingRequest-should-not-cache-headers branch from bb80534 to bca3927 Compare September 25, 2021 13:13
@GreenRecycleBin
Copy link
Contributor Author

GreenRecycleBin commented Sep 25, 2021

It took a few tries to get ./gradlew :spring-web:checkstyleMain passing. Is there an IntelliJ IDEA XML file for the project's style? I've looked through the documentation but didn't find any.

@rstoyanchev rstoyanchev added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Nov 10, 2021
@bclozel bclozel self-assigned this Nov 25, 2021
@bclozel bclozel removed the status: waiting-for-triage An issue we've not yet triaged or decided on label Nov 25, 2021
@bclozel bclozel added this to the 6.0 M1 milestone Nov 25, 2021
@bclozel bclozel added the type: enhancement A general enhancement label Nov 25, 2021
bclozel pushed a commit that referenced this pull request Nov 25, 2021
The current implementation suggests that the request's headers are not
expected to change. Hence, it's not necessary to copy them.
Furthermore, it might be costly to do so if there are many headers.
Instead, cache only the request's header names for method getHeaderNames.

Methods getHeader and getHeaders delegate to the respective methods of
request if the header name is not in FORWARDED_HEADER_NAMES. Otherwise,
they return null or an empty Enumeration respectively.

See gh-27466
@bclozel bclozel closed this in e66095b Nov 25, 2021
@bclozel bclozel changed the title ForwardedHeaderRemovingRequest should not cache headers Optimize header removal in ForwardedHeaderFilter Nov 25, 2021
@GreenRecycleBin GreenRecycleBin deleted the ForwardedHeaderRemovingRequest-should-not-cache-headers branch November 29, 2021 06:54
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.

5 participants