From ab8310b5f37df0e1e86595acec05aff402c8f574 Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Thu, 18 Oct 2018 11:18:29 +0200 Subject: [PATCH] Fix HeadersAdapters implementations This commit harmonizes the `HeadersAdapter` implementations across all supported servers with regards to the `get(Object key)` contract; some server implementations are not sticking to a `Map`-like contract and return empty `List` instead of `null` when a header is not present. This also fixes the `size()` implementations to reflect the number of header keys, as some implementations consider multiple values for the same header as different entries. Issue: SPR-17396 --- .../server/reactive/JettyHeadersAdapter.java | 2 +- .../server/reactive/NettyHeadersAdapter.java | 4 +- .../server/reactive/TomcatHeadersAdapter.java | 2 +- .../server/reactive/HeadersAdaptersTests.java | 99 +++++++++++++++++++ 4 files changed, 103 insertions(+), 4 deletions(-) create mode 100644 spring-web/src/test/java/org/springframework/http/server/reactive/HeadersAdaptersTests.java diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/JettyHeadersAdapter.java b/spring-web/src/main/java/org/springframework/http/server/reactive/JettyHeadersAdapter.java index c1c0a1f5d7..ca9f7b3f58 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/JettyHeadersAdapter.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/JettyHeadersAdapter.java @@ -114,7 +114,7 @@ class JettyHeadersAdapter implements MultiValueMap { @Nullable @Override public List get(Object key) { - if (key instanceof String) { + if (containsKey(key)) { return this.headers.getValuesList((String) key); } return null; diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/NettyHeadersAdapter.java b/spring-web/src/main/java/org/springframework/http/server/reactive/NettyHeadersAdapter.java index ffd5680eab..e43a3db659 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/NettyHeadersAdapter.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/NettyHeadersAdapter.java @@ -91,7 +91,7 @@ class NettyHeadersAdapter implements MultiValueMap { @Override public int size() { - return this.headers.size(); + return this.headers.names().size(); } @Override @@ -114,7 +114,7 @@ class NettyHeadersAdapter implements MultiValueMap { @Override @Nullable public List get(Object key) { - if (key instanceof String) { + if (containsKey(key)) { return this.headers.getAll((String) key); } return null; diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/TomcatHeadersAdapter.java b/spring-web/src/main/java/org/springframework/http/server/reactive/TomcatHeadersAdapter.java index 9b0e462dda..1cde7d5928 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/TomcatHeadersAdapter.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/TomcatHeadersAdapter.java @@ -128,7 +128,7 @@ class TomcatHeadersAdapter implements MultiValueMap { @Override @Nullable public List get(Object key) { - if (key instanceof String) { + if (containsKey(key)) { return Collections.list(this.headers.values((String) key)); } return null; diff --git a/spring-web/src/test/java/org/springframework/http/server/reactive/HeadersAdaptersTests.java b/spring-web/src/test/java/org/springframework/http/server/reactive/HeadersAdaptersTests.java new file mode 100644 index 0000000000..d7b3055a95 --- /dev/null +++ b/spring-web/src/test/java/org/springframework/http/server/reactive/HeadersAdaptersTests.java @@ -0,0 +1,99 @@ +/* + * Copyright 2002-2018 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.http.server.reactive; + +import java.util.Locale; + +import io.netty.handler.codec.http.DefaultHttpHeaders; +import io.undertow.util.HeaderMap; +import org.apache.tomcat.util.http.MimeHeaders; +import org.eclipse.jetty.http.HttpFields; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; + +import org.springframework.util.CollectionUtils; +import org.springframework.util.LinkedCaseInsensitiveMap; +import org.springframework.util.MultiValueMap; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; + +/** + * Unit tests for {@code HeadersAdapters} {@code MultiValueMap} implementations. + * + * @author Brian Clozel + */ +@RunWith(Parameterized.class) +public class HeadersAdaptersTests { + + @Parameterized.Parameter(0) + public MultiValueMap headers; + + @Parameterized.Parameters(name = "headers [{0}]") + public static Object[][] arguments() { + return new Object[][] { + {CollectionUtils.toMultiValueMap( + new LinkedCaseInsensitiveMap<>(8, Locale.ENGLISH))}, + {new NettyHeadersAdapter(new DefaultHttpHeaders())}, + {new TomcatHeadersAdapter(new MimeHeaders())}, + {new UndertowHeadersAdapter(new HeaderMap())}, + {new JettyHeadersAdapter(new HttpFields())} + }; + } + + @Test + public void getWithUnknownHeaderShouldReturnNull() { + assertNull(this.headers.get("Unknown")); + } + + @Test + public void getFirstWithUnknownHeaderShouldReturnNull() { + assertNull(this.headers.getFirst("Unknown")); + } + + @Test + public void sizeWithMultipleValuesForHeaderShouldCountHeaders() { + this.headers.add("TestHeader", "first"); + this.headers.add("TestHeader", "second"); + assertEquals(1, this.headers.size()); + } + + @Test + public void keySetShouldNotDuplicateHeaderNames() { + this.headers.add("TestHeader", "first"); + this.headers.add("OtherHeader", "test"); + this.headers.add("TestHeader", "second"); + assertEquals(2, this.headers.keySet().size()); + } + + @Test + public void containsKeyShouldBeCaseInsensitive() { + this.headers.add("TestHeader", "first"); + assertTrue(this.headers.containsKey("testheader")); + } + + @Test + public void addShouldKeepOrdering() { + this.headers.add("TestHeader", "first"); + this.headers.add("TestHeader", "second"); + assertEquals("first", this.headers.getFirst("TestHeader")); + assertEquals("first", this.headers.get("TestHeader").get(0)); + } + +}