LinkedCaseInsensitiveMap#getOrDefault should return default value instead of null

[poying9464]

This method directly calls the LinkedHashMap # get of the parent class,
but after obtaining the value, it does not determine whether it is a null value

[pivotal-cla]

@poying9464 Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[pivotal-cla]

@poying9464 Thank you for signing the Contributor License Agreement!

[ngocnhan-tran1996]

@poying9464

Method getOrDefault was described

Returns:
the value to which the specified key is mapped, or defaultValue if this map contains no mapping for the key

More detail Interface Map

LinkedCaseInsensitiveMap<V> implements Map<String, V>, so the value return null belongs one of case

1. this map does not contains the input key
2. this input key has null value

I don't think this is a bug but I also wait for confirming from Spring team

[sdeleuze]

The method getOrDefault(Object key, V defaultValue) is annotated with @Nullable, so the behavior currently implemented is likely the right one, so I will decline this PR.

[sbrannen]

I took a look at this as well, and I think there's actually a bug in the implementation.

Specifically, I believe LinkedCaseInsensitiveMap.getOrDefault(Object, V) should delegate to LinkedHashMap.getOrDefault(Object, V) as follows.

	public V getOrDefault(Object key, V defaultValue) {
		if (key instanceof String string) {
			String caseInsensitiveKey = this.caseInsensitiveKeys.get(convertKey(string));
			if (caseInsensitiveKey != null) {
				return this.targetMap.getOrDefault(caseInsensitiveKey, defaultValue);
			}
		}
		return defaultValue;
	}

@sdeleuze, what do you think?

[sbrannen]

I took a look at this as well, and I think there's actually a bug in the implementation.

I took a closer look and realized that the if (caseInsensitiveKey != null) effectively already performs the "contains key" logic in this.targetMap.getOrDefault(...).

Thus, there is no need to invoke this.targetMap.getOrDefault(...) or perform any special handling as proposed in this PR.

@poying9464, please note that we have tests in org.springframework.util.LinkedCaseInsensitiveMapTests which verify the expected behavior.

In light of the above, I am re-closing this issue as @sdeleuze had originally.

[poying9464]

The method getOrDefault(Object key, V defaultValue) is annotated with @Nullable, so the behavior currently implemented is likely the right one, so I will decline this PR.

Although this method has the @Nullable annotation, I think that when the return result is null, the given defaultValue should be returned. It should be implemented according to the original design intention of Map#getOrDefault

[sbrannen]

Hi @poying9464,

It should be implemented according to the original design intention of Map#getOrDefault

As I mentioned above, it is already implemented according to the contract of Map#getOrDefault.

See the following tests for details.

spring-framework/spring-core/src/test/java/org/springframework/util/LinkedCaseInsensitiveMapTests.java

Lines 70 to 92 in 1ea4eb1

	@Test
	void getOrDefault() {
	assertThat(map.put("key", "value1")).isNull();
	assertThat(map.put("KEY", "value2")).isEqualTo("value1");
	assertThat(map.put("Key", "value3")).isEqualTo("value2");
	assertThat(map.getOrDefault("key", "N")).isEqualTo("value3");
	assertThat(map.getOrDefault("KEY", "N")).isEqualTo("value3");
	assertThat(map.getOrDefault("Key", "N")).isEqualTo("value3");
	assertThat(map.getOrDefault("keeeey", "N")).isEqualTo("N");
	assertThat(map.getOrDefault(new Object(), "N")).isEqualTo("N");
	}
	@Test
	void getOrDefaultWithNullValue() {
	assertThat(map.put("key", null)).isNull();
	assertThat(map.put("KEY", null)).isNull();
	assertThat(map.put("Key", null)).isNull();
	assertThat(map.getOrDefault("key", "N")).isNull();
	assertThat(map.getOrDefault("KEY", "N")).isNull();
	assertThat(map.getOrDefault("Key", "N")).isNull();
	assertThat(map.getOrDefault("keeeey", "N")).isEqualTo("N");
	assertThat(map.getOrDefault(new Object(), "N")).isEqualTo("N");
	}