Skip to content

Improve containsValue Method for CharSequence Comparison #32448

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

alidandach
Copy link
Contributor

Problem

The current implementation of the containsValue method checks if a provided value is an instance of String and then compares this value to the entries within headers using String.equals(). This approach limits the comparison strictly to String instances and does not account for other CharSequence implementations like StringBuilder or StringBuffer, which might also represent valid sequences of characters to be compared.

Solution

This MR introduces an improvement to the containsValue method, enabling it to handle comparisons with any objects implementing the CharSequence interface, not just String. This is achieved by converting both the value and the entry value to String before comparison, ensuring that the comparison logic remains effective while becoming more flexible.

Changes Made

  • Check if value is an instance of CharSequence instead of strictly String.
  • Convert value to String (if it's a CharSequence) before the comparison.
  • Update the comparison logic to ensure it correctly handles CharSequence implementations.

The method `containsValue` in `Netty5HeadersAdapter` has been refactored. It now converts the input value and the entry value into the string prior to comparison. This approach optimizes accuracy, preventing false positives when a CharSequence object is passed in that isn't a string.
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Mar 15, 2024
@bclozel bclozel self-assigned this Mar 15, 2024
@bclozel
Copy link
Member

bclozel commented Mar 15, 2024

I think this change would break the implementation here. While netty headers can indeed contain CharSequence keys and values, the adapter here is only exposing a MultiValueMap<String, String>.

This change would make the following behavior possible, which I think would be inconsistent:

HttpHeaders nettyHeaders = HttpHeaders.newHeaders();
nettyHeaders.add(new StringBuilder().append("test"), new StringBuilder().append("spring"));
Netty5HeadersAdapter adapter = new Netty5HeadersAdapter(nettyHeaders);

// we got a List<String> here
assertThat(adapter.getFirst("test")).isInstanceOf(String.class).isEqualTo("spring");

// this is true, you can get a String "spring"
assertThat(adapter.containsValue("spring")).isTrue();
// this should be false, you will never get a `StringBuilder` value from the map
assertThat(adapter.containsValue(new StringBuilder().append("spring"))).isFalse();

@bclozel bclozel closed this Mar 15, 2024
@bclozel bclozel added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Mar 15, 2024
@alidandach alidandach deleted the fix/charsequence-comparison-issue branch March 15, 2024 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants