From a03517fa353a37ac28e13be9d1869bdaa87cb652 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Fri, 2 Aug 2013 17:40:08 -0400 Subject: [PATCH] Polish ServletServerHttpRequest query param handling The method returning query parameters now returns only query string parameters as opposed to any Servlet request parameter. This commit also adds a ReadOnlyMultiValueMap. --- .../util/ReadOnlyMultiValueMap.java | 150 ++++++++++++++++++ .../org/springframework/http/MediaType.java | 4 +- .../http/server/ServerHttpResponse.java | 4 +- .../http/server/ServletServerHttpRequest.java | 21 ++- .../server/ServletServerHttpRequestTests.java | 19 ++- .../web/socket/sockjs/SockJsService.java | 10 ++ .../sockjs/support/AbstractSockJsService.java | 5 +- .../support/AbstractSockJsServiceTests.java | 19 ++- .../handler/DefaultSockJsServiceTests.java | 1 + .../HttpSendingTransportHandlerTests.java | 9 +- 10 files changed, 221 insertions(+), 21 deletions(-) create mode 100644 spring-core/src/main/java/org/springframework/util/ReadOnlyMultiValueMap.java diff --git a/spring-core/src/main/java/org/springframework/util/ReadOnlyMultiValueMap.java b/spring-core/src/main/java/org/springframework/util/ReadOnlyMultiValueMap.java new file mode 100644 index 000000000000..b6806eaef2ba --- /dev/null +++ b/spring-core/src/main/java/org/springframework/util/ReadOnlyMultiValueMap.java @@ -0,0 +1,150 @@ +/* + * Copyright 2002-2013 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.util; + +import java.util.Collection; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Set; + + +/** + * Wraps a {@link MultiValueMap} to make it immutable. + * + * @author Rossen Stoyanchev + * @since 4.0 + */ +public class ReadOnlyMultiValueMap implements MultiValueMap { + + private final MultiValueMap targetMap; + + + /** + * Create a new ReadOnlyMultiValueMap that wraps the given target map. + */ + public ReadOnlyMultiValueMap(MultiValueMap targetMap) { + this.targetMap = targetMap; + } + + // MultiValueMap implementation + + @Override + public void add(K key, V value) { + throw new UnsupportedOperationException("This map is immutable."); + } + + @Override + public V getFirst(K key) { + return this.targetMap.getFirst(key); + } + + @Override + public void set(K key, V value) { + throw new UnsupportedOperationException("This map is immutable."); + } + + @Override + public void setAll(Map values) { + throw new UnsupportedOperationException("This map is immutable."); + } + + @Override + public Map toSingleValueMap() { + return Collections.unmodifiableMap(this.targetMap.toSingleValueMap()); + } + + + // Map implementation + + @Override + public int size() { + return this.targetMap.size(); + } + + @Override + public boolean isEmpty() { + return this.targetMap.isEmpty(); + } + + @Override + public boolean containsKey(Object key) { + return this.targetMap.containsKey(key); + } + + @Override + public boolean containsValue(Object value) { + return this.targetMap.containsValue(value); + } + + @Override + public List get(Object key) { + return this.targetMap.get(key); + } + + @Override + public List put(K key, List value) { + throw new UnsupportedOperationException("This map is immutable."); + } + + @Override + public List remove(Object key) { + throw new UnsupportedOperationException("This map is immutable."); + } + + @Override + public void putAll(Map> m) { + this.targetMap.putAll(m); + } + + @Override + public void clear() { + throw new UnsupportedOperationException("This map is immutable."); + } + + @Override + public Set keySet() { + return Collections.unmodifiableSet(this.targetMap.keySet()); + } + + @Override + public Collection> values() { + return Collections.unmodifiableCollection(this.targetMap.values()); + } + + @Override + public Set>> entrySet() { + return Collections.unmodifiableSet(this.targetMap.entrySet()); + } + + + @Override + public boolean equals(Object obj) { + return this.targetMap.equals(obj); + } + + @Override + public int hashCode() { + return this.targetMap.hashCode(); + } + + @Override + public String toString() { + return this.targetMap.toString(); + } + +} \ No newline at end of file diff --git a/spring-web/src/main/java/org/springframework/http/MediaType.java b/spring-web/src/main/java/org/springframework/http/MediaType.java index fee2336466e5..222db739de70 100644 --- a/spring-web/src/main/java/org/springframework/http/MediaType.java +++ b/spring-web/src/main/java/org/springframework/http/MediaType.java @@ -686,7 +686,9 @@ public static MediaType valueOf(String value) { * @throws InvalidMediaTypeException if the string cannot be parsed */ public static MediaType parseMediaType(String mediaType) { - Assert.hasLength(mediaType, "'mediaType' must not be empty"); + if (!StringUtils.hasLength(mediaType)) { + throw new InvalidMediaTypeException(mediaType, "'mediaType' must not be empty"); + } String[] parts = StringUtils.tokenizeToStringArray(mediaType, ";"); String fullType = parts[0].trim(); diff --git a/spring-web/src/main/java/org/springframework/http/server/ServerHttpResponse.java b/spring-web/src/main/java/org/springframework/http/server/ServerHttpResponse.java index ea8cae6a4d34..7cfe6011f5d4 100644 --- a/spring-web/src/main/java/org/springframework/http/server/ServerHttpResponse.java +++ b/spring-web/src/main/java/org/springframework/http/server/ServerHttpResponse.java @@ -37,7 +37,9 @@ public interface ServerHttpResponse extends HttpOutputMessage, Closeable { void setStatusCode(HttpStatus status); /** - * TODO + * Ensure the headers and the content of the response are written out. After the first + * flush, headers can no longer be changed, only further content writing and flushing + * is possible. */ void flush() throws IOException; diff --git a/spring-web/src/main/java/org/springframework/http/server/ServletServerHttpRequest.java b/spring-web/src/main/java/org/springframework/http/server/ServletServerHttpRequest.java index b6cda3d97c92..c9a30e099d39 100644 --- a/spring-web/src/main/java/org/springframework/http/server/ServletServerHttpRequest.java +++ b/spring-web/src/main/java/org/springframework/http/server/ServletServerHttpRequest.java @@ -34,6 +34,8 @@ import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import javax.servlet.http.HttpServletRequest; @@ -44,6 +46,7 @@ import org.springframework.util.Assert; import org.springframework.util.LinkedMultiValueMap; import org.springframework.util.MultiValueMap; +import org.springframework.util.ReadOnlyMultiValueMap; /** * {@link ServerHttpRequest} implementation that is based on a {@link HttpServletRequest}. @@ -59,6 +62,8 @@ public class ServletServerHttpRequest implements ServerHttpRequest { private static final String METHOD_POST = "POST"; + private static final Pattern QUERY_PARAM_PATTERN = Pattern.compile("([^&=]+)(=?)([^&]+)?"); + private final HttpServletRequest servletRequest; private HttpHeaders headers; @@ -167,13 +172,19 @@ public Map getCookies() { @Override public MultiValueMap getQueryParams() { if (this.queryParams == null) { - // TODO: extract from query string - this.queryParams = new LinkedMultiValueMap(this.servletRequest.getParameterMap().size()); - for (String name : this.servletRequest.getParameterMap().keySet()) { - for (String value : this.servletRequest.getParameterValues(name)) { - this.queryParams.add(name, value); + MultiValueMap result = new LinkedMultiValueMap(); + String queryString = this.servletRequest.getQueryString(); + if (queryString != null) { + Matcher m = QUERY_PARAM_PATTERN.matcher(queryString); + while (m.find()) { + String name = m.group(1); + String[] values = this.servletRequest.getParameterValues(name); + if (values != null) { + result.put(name, Arrays.asList(values)); + } } } + this.queryParams = new ReadOnlyMultiValueMap(result); } return this.queryParams; } diff --git a/spring-web/src/test/java/org/springframework/http/server/ServletServerHttpRequestTests.java b/spring-web/src/test/java/org/springframework/http/server/ServletServerHttpRequestTests.java index 9b0555ae57c7..882c3c5886a5 100644 --- a/spring-web/src/test/java/org/springframework/http/server/ServletServerHttpRequestTests.java +++ b/spring-web/src/test/java/org/springframework/http/server/ServletServerHttpRequestTests.java @@ -18,11 +18,11 @@ import java.net.URI; import java.nio.charset.Charset; +import java.util.Arrays; import java.util.List; import org.junit.Before; import org.junit.Test; - import org.springframework.http.HttpHeaders; import org.springframework.http.HttpMethod; import org.springframework.http.MediaType; @@ -106,4 +106,21 @@ public void getFormBody() throws Exception { assertArrayEquals("Invalid content returned", content, result); } + @Test + public void getQueryParams() throws Exception { + mockRequest.setQueryString("foo=bar"); + mockRequest.addParameter("foo", "bar"); + mockRequest.addParameter("a", "b"); + assertEquals(Arrays.asList("bar"), request.getQueryParams().get("foo")); + assertNull(request.getQueryParams().get("a")); + } + + @Test + public void getQueryParamsTwoValues() throws Exception { + mockRequest.setQueryString("baz=qux&baz=42"); + mockRequest.addParameter("baz", "qux"); + mockRequest.addParameter("baz", "42"); + assertEquals(Arrays.asList("qux", "42"), request.getQueryParams().get("baz")); + } + } \ No newline at end of file diff --git a/spring-websocket/src/main/java/org/springframework/web/socket/sockjs/SockJsService.java b/spring-websocket/src/main/java/org/springframework/web/socket/sockjs/SockJsService.java index 365c6d13ebd6..0430606c602d 100644 --- a/spring-websocket/src/main/java/org/springframework/web/socket/sockjs/SockJsService.java +++ b/spring-websocket/src/main/java/org/springframework/web/socket/sockjs/SockJsService.java @@ -33,6 +33,16 @@ public interface SockJsService { + /** + * Process a SockJS request. + * + * @param request the current request + * @param response the current response + * @param handler the handler to process messages with + * + * @throws IOException raised if writing the to response of the current request fails + * @throws SockJsProcessingException + */ void handleRequest(ServerHttpRequest request, ServerHttpResponse response, WebSocketHandler handler) throws IOException, SockJsProcessingException; diff --git a/spring-websocket/src/main/java/org/springframework/web/socket/sockjs/support/AbstractSockJsService.java b/spring-websocket/src/main/java/org/springframework/web/socket/sockjs/support/AbstractSockJsService.java index dbd351924cfe..b80755601249 100644 --- a/spring-websocket/src/main/java/org/springframework/web/socket/sockjs/support/AbstractSockJsService.java +++ b/spring-websocket/src/main/java/org/springframework/web/socket/sockjs/support/AbstractSockJsService.java @@ -34,6 +34,7 @@ import org.apache.commons.logging.LogFactory; import org.springframework.http.HttpMethod; import org.springframework.http.HttpStatus; +import org.springframework.http.InvalidMediaTypeException; import org.springframework.http.MediaType; import org.springframework.http.server.ServerHttpRequest; import org.springframework.http.server.ServerHttpResponse; @@ -283,8 +284,8 @@ public final void handleRequest(ServerHttpRequest request, ServerHttpResponse re try { request.getHeaders(); } - catch (IllegalArgumentException ex) { - // Ignore invalid Content-Type (TODO) + catch (InvalidMediaTypeException ex) { + logger.warn("Invalid media type ignored: " + ex.getMediaType()); } try { diff --git a/spring-websocket/src/test/java/org/springframework/web/socket/sockjs/support/AbstractSockJsServiceTests.java b/spring-websocket/src/test/java/org/springframework/web/socket/sockjs/support/AbstractSockJsServiceTests.java index 29658cd8d64a..2503f5fafa68 100644 --- a/spring-websocket/src/test/java/org/springframework/web/socket/sockjs/support/AbstractSockJsServiceTests.java +++ b/spring-websocket/src/test/java/org/springframework/web/socket/sockjs/support/AbstractSockJsServiceTests.java @@ -164,6 +164,7 @@ public void handleInfoOptions() throws Exception { this.servletRequest.addHeader("Access-Control-Request-Headers", "Last-Modified"); handleRequest("OPTIONS", "/a/info", HttpStatus.NO_CONTENT); + this.response.flush(); assertEquals("*", this.servletResponse.getHeader("Access-Control-Allow-Origin")); assertEquals("true", this.servletResponse.getHeader("Access-Control-Allow-Credentials")); @@ -214,6 +215,15 @@ private void handleRequest(String httpMethod, String uri, HttpStatus httpStatus) assertEquals(httpStatus.value(), this.servletResponse.getStatus()); } + @Test + public void handleEmptyContentType() throws Exception { + + servletRequest.setContentType(""); + handleRequest("GET", "/a/info", HttpStatus.OK); + + assertEquals("Invalid/empty content should have been ignored", 200, this.servletResponse.getStatus()); + } + private static class TestSockJsService extends AbstractSockJsService { @@ -228,16 +238,15 @@ public TestSockJsService(TaskScheduler scheduler) { } @Override - protected void handleRawWebSocketRequest(ServerHttpRequest request, - ServerHttpResponse response, WebSocketHandler handler) throws IOException { + protected void handleRawWebSocketRequest(ServerHttpRequest req, ServerHttpResponse res, + WebSocketHandler handler) throws IOException { this.handler = handler; } @Override - protected void handleTransportRequest(ServerHttpRequest request, ServerHttpResponse response, - WebSocketHandler handler, String sessionId, String transport) - throws IOException, SockJsProcessingException { + protected void handleTransportRequest(ServerHttpRequest req, ServerHttpResponse res, WebSocketHandler handler, + String sessionId, String transport) throws IOException, SockJsProcessingException { this.sessionId = sessionId; this.transport = transport; diff --git a/spring-websocket/src/test/java/org/springframework/web/socket/sockjs/transport/handler/DefaultSockJsServiceTests.java b/spring-websocket/src/test/java/org/springframework/web/socket/sockjs/transport/handler/DefaultSockJsServiceTests.java index 0d64d79e6f20..0115f51266b8 100644 --- a/spring-websocket/src/test/java/org/springframework/web/socket/sockjs/transport/handler/DefaultSockJsServiceTests.java +++ b/spring-websocket/src/test/java/org/springframework/web/socket/sockjs/transport/handler/DefaultSockJsServiceTests.java @@ -130,6 +130,7 @@ public void dummySessionCookieEnabled() throws Exception { setRequest("POST", sessionUrlPrefix + "xhr"); this.service.setDummySessionCookieEnabled(true); this.service.handleRequest(this.request, this.response, this.wsHandler); + this.response.flush(); assertEquals(200, this.servletResponse.getStatus()); assertEquals("JSESSIONID=dummy;path=/", this.servletResponse.getHeader("Set-Cookie")); diff --git a/spring-websocket/src/test/java/org/springframework/web/socket/sockjs/transport/handler/HttpSendingTransportHandlerTests.java b/spring-websocket/src/test/java/org/springframework/web/socket/sockjs/transport/handler/HttpSendingTransportHandlerTests.java index e42177e30e53..b58937b540a5 100644 --- a/spring-websocket/src/test/java/org/springframework/web/socket/sockjs/transport/handler/HttpSendingTransportHandlerTests.java +++ b/spring-websocket/src/test/java/org/springframework/web/socket/sockjs/transport/handler/HttpSendingTransportHandlerTests.java @@ -25,12 +25,6 @@ import org.springframework.web.socket.WebSocketHandler; import org.springframework.web.socket.sockjs.support.frame.SockJsFrame; import org.springframework.web.socket.sockjs.support.frame.SockJsFrame.FrameFormat; -import org.springframework.web.socket.sockjs.transport.handler.AbstractHttpSendingTransportHandler; -import org.springframework.web.socket.sockjs.transport.handler.EventSourceTransportHandler; -import org.springframework.web.socket.sockjs.transport.handler.HtmlFileTransportHandler; -import org.springframework.web.socket.sockjs.transport.handler.JsonpPollingTransportHandler; -import org.springframework.web.socket.sockjs.transport.handler.XhrPollingTransportHandler; -import org.springframework.web.socket.sockjs.transport.handler.XhrStreamingTransportHandler; import org.springframework.web.socket.sockjs.transport.session.AbstractSockJsSession; import org.springframework.web.socket.sockjs.transport.session.PollingSockJsSession; import org.springframework.web.socket.sockjs.transport.session.StreamingSockJsSession; @@ -106,6 +100,7 @@ public void jsonpTransport() throws Exception { assertEquals("\"callback\" parameter required", this.servletResponse.getContentAsString()); resetRequestAndResponse(); + this.servletRequest.setQueryString("c=callback"); this.servletRequest.addParameter("c", "callback"); transportHandler.handleRequest(this.request, this.response, this.webSocketHandler, session); @@ -141,6 +136,7 @@ public void htmlFileTransport() throws Exception { assertEquals("\"callback\" parameter required", this.servletResponse.getContentAsString()); resetRequestAndResponse(); + this.servletRequest.setQueryString("c=callback"); this.servletRequest.addParameter("c", "callback"); transportHandler.handleRequest(this.request, this.response, this.webSocketHandler, session); @@ -166,6 +162,7 @@ public void eventSourceTransport() throws Exception { @Test public void frameFormats() throws Exception { + this.servletRequest.setQueryString("c=callback"); this.servletRequest.addParameter("c", "callback"); SockJsFrame frame = SockJsFrame.openFrame();