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

Split new HttpHeaders(...) constructor into static factory methods HttpHeaders.backedBy(...) and HttpHeaders.copyOf(...) #34340

Closed
craftmaster2190 opened this issue Jan 30, 2025 · 1 comment · May be fixed by #34341
Labels
status: superseded An issue that has been superseded by another

Comments

@craftmaster2190
Copy link

** Enhancements requests **
Before explaining how you would like things to work,
please describe a concrete use case for this feature and how you have tried to solve this so far.

Enhancement Request

Replace new HttpHeaders(...) constructor with static factory method HttpHeaders.backedBy(...) and add also a method called HttpHeaders.copyOf(...).

Use cases

There are already use cases for a backedBy method that behaves the way the constructor does now.

Use cases for a copyOf method that creates a copy and is not backed by the previous map.

  • Copying HttpHeaders to be redacted for logging.
  • Copying HttpHeaders to mutated for security-related headers.
  • Copying a default HttpHeaders object and adding customized headers. (Yes, I am aware that there are defaultHeader(...) methods on WebClient, RestTemplate, etc.)
  • Copying a HttpHeaders during proxy type operations.

Background

Unless one is a seasoned Spring maintainer; the behavior of the HttpHeaders(...) constructor seems very surprising.

Take the following example code.

static HttpHeaders redactProtectedHeaders(HttpHeaders headers) {  
  HttpHeaders httpHeadersCopy = new HttpHeaders(headers);  
  Optional.ofNullable(httpHeadersCopy.get("Authorization"))  
      .ifPresent(authorizationHeaderValues -> 
            httpHeadersCopy.replace("Authorization", redact(authorizationHeaderValues)));  
  return httpHeadersCopy;  
}

The above code is incorrect in that the second instance of HttpHeaders is not a true deep copy.
Changes to the copy are also changed in the first instance.
If (and that is a big if) the developer reads the documentation and HttpHeadersTest unit test;
they might be led to write:

HttpHeaders httpHeadersCopy = new HttpHeaders(HttpHeaders.readOnlyHttpHeaders(headers));

However, this is not intuitive and also does not protect the original headers object from being mutated since ReadOnlyHttpHeaders does not make the underlying MultiValueMap read-only.
So then the developer writes this code since there is not a copy/clone method.

HttpHeaders httpHeadersCopy = new HttpHeaders();  
headers.forEach((key, values) -> httpHeadersCopy.put(key, new ArrayList<>(values)));

I've read the following related issues and I recognize there are historical reasons for why the constructor is the way it is. #28336 #33789 #32116
This comment stuck out:

Unlike collection-related constructors (like HashMap or ArrayList), this is not about copying entries to a new collection, but creating a new HttpHeaders instance backed by an existing map.

This comment, while explaining the desired behavior, acknowledges the root of the surprise in that there is an implied norm given by collection-related constructors. For developers who have trained on collections, the conflicting behavior of HttpHeaders is disorienting.

I'd ask that the new HttpHeaders(...) constructors be deprecated, made private, and eventually fully replaced with static factory methods HttpHeaders.backedBy(...) and HttpHeaders.copyOf(...) to remove any ambiguity, keep the desired behavior for adapting existing structures, and also provide the functionality of creating deep clones.

craftmaster2190 added a commit to craftmaster2190/spring-framework that referenced this issue Jan 30, 2025
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jan 30, 2025
craftmaster2190 added a commit to craftmaster2190/spring-framework that referenced this issue Jan 30, 2025
craftmaster2190 added a commit to craftmaster2190/spring-framework that referenced this issue Jan 30, 2025
@bclozel
Copy link
Member

bclozel commented Jan 30, 2025

Let's discuss this change on the PR directly.
Closing in favour of #34341

@bclozel bclozel closed this as completed Jan 30, 2025
@bclozel bclozel added status: superseded An issue that has been superseded by another and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jan 30, 2025
craftmaster2190 added a commit to craftmaster2190/spring-framework that referenced this issue Jan 30, 2025
… on the constructor.

Fixes spring-projects#34340

chore: Update constructor references with backedBy method.

Signed-off-by: Bryce J. Fisher <[email protected]>
craftmaster2190 added a commit to craftmaster2190/spring-framework that referenced this issue Jan 31, 2025
… on the constructor.

chore: Update constructor references with backedBy method.
chore: Remove `@Deprecated` annotations.

Fixes spring-projects#34340

Signed-off-by: Bryce J. Fisher <[email protected]>
craftmaster2190 added a commit to craftmaster2190/spring-framework that referenced this issue Jan 31, 2025
… on the constructor.

chore: Update constructor references with backedBy method.
chore: Remove `@Deprecated` annotations.

Fixes spring-projects#34340

Signed-off-by: Bryce J. Fisher <[email protected]>
craftmaster2190 added a commit to craftmaster2190/spring-framework that referenced this issue Jan 31, 2025
… on the constructor.

chore: Update constructor references with backedBy method.
chore: Remove `@Deprecated` annotations.

Fixes spring-projects#34340

Signed-off-by: Bryce J. Fisher <[email protected]>
craftmaster2190 added a commit to craftmaster2190/spring-framework that referenced this issue Jan 31, 2025
… on the constructor.

chore: Update constructor references with backedBy method.
chore: Remove `@Deprecated` annotations.

Fixes spring-projects#34340

Signed-off-by: Bryce J. Fisher <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: superseded An issue that has been superseded by another
Projects
None yet
3 participants