Removing a key from HttpHeaders' key set leaves it in an inconsistent state

[wilkinsona]

Affects: 5.1.10 snapshots

The problem is illustrated by the following test:

package example;

import java.util.Arrays;

import org.junit.Test;

import org.springframework.http.HttpHeaders;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.entry;

public class HttpHeadersTests {

	@Test
	public void removeFromKeySet() {
		HttpHeaders httpHeaders = new HttpHeaders();
		httpHeaders.set("Alpha", "apple");
		httpHeaders.set("Bravo", "banana");
		httpHeaders.keySet().remove("Alpha");
		assertThat(httpHeaders).containsOnly(entry("Bravo", Arrays.asList("banana")));
		assertThat(httpHeaders).doesNotContainKey("Alpha");
	}

}

The first assertion passes but the second fails:

java.lang.AssertionError: 
Expecting:
  <{"Bravo"=["banana"]}>
not to contain key:
  <"Alpha">
	at example.HttpHeadersTests.removeFromKeySet(HttpHeadersTests.java:21)

[sbrannen]

Interestingly, the provided test case passes on master.

We'll look into it!

[sbrannen]

I have confirmed that the following test fails when added to org.springframework.http.HttpHeadersTests in 5.1.x.

@Test
public void removeFromKeySet() {
	headers.add("Alpha", "apple");
	headers.add("Bravo", "banana");
	assertEquals(2, headers.size());

	headers.keySet().remove("Alpha");
	assertEquals(1, headers.size());
	assertEquals(Collections.singletonMap("Bravo", Arrays.asList("banana")).entrySet(), headers.entrySet());

	// Fails!
	assertFalse("Alpha should have been removed", headers.containsKey("Alpha"));
}

[sbrannen]

It turns out that the underlying cause is a bug in the anonymous inner LinkedHashMap subclass in org.springframework.util.LinkedCaseInsensitiveMap<V>. Specifically, that anonymous subclass must override keySet() with a custom implementation. I've got a prototype working locally and will push changes once I've tested more thoroughly.

[sbrannen]

My local fix works, but it turns out that @philwebb already addressed this issue plus additional related issues in #22821.

I am therefore closing this as a duplicate of #22821. Rationale provided by @jhoeller in #22821 (comment).

[wilkinsona]

I think it's worth noting that this is a regression. The supplied test works in 5.1.0 but fails with 5.1.1 and later.

[sbrannen]

I think it's worth noting that this is a regression. The supplied test works in 5.1.0 but fails with 5.1.1 and later.

That's a very valid point. I'll raise that with the team.

[sbrannen]

Update: #22821 will backported to 5.1.10 in #23644.