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

LinkedCaseInsensitiveMap does not properly support Java 8's merge() and compute() methods [SPR-15026] #19593

Closed
spring-issuemaster opened this issue Dec 16, 2016 · 4 comments

Comments

@spring-issuemaster
Copy link
Collaborator

commented Dec 16, 2016

John Mark opened SPR-15026 and commented

LinkedCaseInsensitiveMap does not properly implement many of the new Map methods that have been added in Java 1.8 such as merge() and the compute* methods.

Using those methods on a LinkedCaseInsensitiveMap results in an entry that can never be retrieved.


Affects: 4.3.4

Issue Links:

  • #18553 LinkedCaseInsensitiveMap doesn't implement getOrDefault properly
  • #19078 LinkedCaseInsensitiveMap doesn't override HashMap.clone()
  • #19653 HttpHeaders.keySet() is no longer case-insensitive

Referenced from: commits e9a87de, 505480c, 8147c11, 50e5a65

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 16, 2016

Juergen Hoeller commented

We're unfortunately constrained in 4.3.x there since we cannot have hard dependencies on the java.util.function interfaces - as used in those merge and compute signatures - in our core codebase. However, we can certainly revisit this for 5.0 where we have a Java 8 baseline now.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 23, 2016

Juergen Hoeller commented

Looks like there is a straightforward way out: rearchitecting LinkedCaseInsensitiveMap to implement Map directly, delegating to an internal LinkedHashMap for specific operations only and therefore not inheriting HashMap's overrides of those new Java 8 default methods. Instead, we can transparently reuse the original default method implementations from the Map interface which seem to work for our purposes. This is even compatible with Java 6, 7 and 8 (and forward with 9) since we do not have to implement any of the new signatures in LinkedCaseInsensitiveMap itself, so we can technically backport this to 4.3.6.

From a positive perspective, this is also aligned with our existing LinkedMultiValueMap implementation style now. At the same time, it is a significant change in the class hierarchy which we usually wouldn't do in a maintenance release. Since I don't expect anybody to cast a LinkedCaseInsensitiveMap to LinkedHashMap or HashMap, this shouldn't be an issue in practice... and we can even keep removeEldestEntry overrides in custom subclasses working. All in all, it is more important that all of those new Map operations work properly; I therefore see this change as acceptable for the 4.3.x line as well.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 3, 2017

Andy Wilkinson commented

This has a rather unpleasant side-effect on HttpHeaders. Consider the following setup:

HttpHeaders headers = new HttpHeaders();
headers.add("X-Foo", "bar");

With 5.0 M3 and earlier, this is true:

headers.keySet().contains("x-foo")

For the latest 5.0 snapshots, it's false as LinkedCaseInsensitiveMap.containsKey(Object) is no longer used. This breaks Spring REST Docs with Spring Framework 5.0 snapshots as I currently rely on the case-insensitive behaviour of the key set.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 3, 2017

Andy Wilkinson commented

On Stephane's advice, I've opened #19653

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.