Spring-framework: LinkedCaseInsensitiveMap does not track removals via keySet, entrySet, or values

[codeexistent]

I'm learning spring boot (using Spring-boot 2.1.3) while playing with REST API calls i found something interesting with HttpHeaders object

Suppose if i have an HttpHeaders object with some values

      HttpHeaders headers = new HttpHeaders();
      headers.add("Connection", "keep-alive");

But now if i try to delete and add them again it throws NullPointerException

      headers.entrySet().removeIf(entry -> entry.getKey().equals("Connection"));
      headers.add("Connection", "keep-alive");

Error:

       Caused by: java.lang.NullPointerException: null
    at org.springframework.util.CollectionUtils$MultiValueMapAdapter.add(CollectionUtils.java:460) ~[spring-core-5.1.5.RELEASE.jar:5.1.5.RELEASE]
    at org.springframework.http.HttpHeaders.add(HttpHeaders.java:1559) ~[spring-web-5.1.5.RELEASE.jar:5.1.5.RELEASE]
    at com.demo.DemoMain.run(DemoMain.java:27) ~[main/:na]
    at org.springframework.boot.SpringApplication.callRunner(SpringApplication.java:813) ~[spring-boot-2.1.3.RELEASE.jar:2.1.3.RELEASE]
    ... 5 common frames omitted

[philwebb]

Thanks. This looks like a bug in LinkedCaseInsensitiveMap:

@Override
@Nullable
public V computeIfAbsent(String key, Function<? super String, ? extends V> mappingFunction) {
    String oldKey = this.caseInsensitiveKeys.putIfAbsent(convertKey(key), key);
    if (oldKey != null) {
        return this.targetMap.get(oldKey);
    }
    return this.targetMap.computeIfAbsent(key, mappingFunction);
}

The oldKey variable is not null but the target map doesn't contain the value. I guess we either need to track removals done via entrySet or that method should change.

[rhamedy]

@philwebb should this be a bug to be fixed, I offer to work on it 👍

[codeexistent]

@philwebb @rhamedy How this works? any one can contribute to this? i thought only spring people are allowed to fix these and outsiders are just to report bugs, correct me please if i'm wrong and if i'm allowed to work on it i feel it's a great opportunity for me

[philwebb]

@Deadpool1111 @rhamedy Thanks for the offer of help. Anyone is free to contribute to Spring as long as they are happy to sign a CLA. Submissions are usually handled via pull-requests which are reviewed by a member of the team.

For this specific issue, I think we should wait until someone from the Framework team has reviewed it. It looks like the fix might be quite involved as we'll probably need to return specialized types from getValues() getKeys() and getKeySet() that can track removals and update the caseInsensitiveKeys.

[sbrannen]

FYI: I introduced failing tests that reproduce this bug in e187a42bfca79b75548cdf6dd5c46298ccfa8134.

[nkjackzhang]

FYI: I introduced failing tests that reproduce this bug in e187a42.

headers.get() returns List<String>, so headers.getFirst() should be used.

[philwebb]

@jhoeller I have a fix here that you can cherry-pick to the appropriate branch if you're happy with it.

[jhoeller]

@philwebb This looks good to me; please simply merge it to master right away! Since the changes are quite involved (and the use cases rather limited), I'd rather keep this as 5.2 only.

As for the example above, why not simply call headers.remove("Connection") which works reliably on existing versions and is way more efficient than entrySet() iteration?

[wilkinsona]

Can we please consider backporting this, or at least the parts of it that are necessary to fix #23633, to 5.1? As things stand Spring REST Docs' support for removing headers from a request or response before they're documented does not work with Framework 5.1.x.

[wilkinsona]

Nevermind. I've worked around it in a way that works with 5.0, 5.1, and (hopefully) 5.2.