Polish

Issue: SPR-15206

spring-web/src/main/java/org/springframework/web/filter/reactive/HiddenHttpMethodFilter.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2017 the original author or authors.
+ * Copyright 2002-2017 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.
@@ -13,26 +13,28 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
+
 package org.springframework.web.filter.reactive;
 
+import java.util.Locale;
+
+import reactor.core.publisher.Mono;
+
 import org.springframework.http.HttpMethod;
 import org.springframework.http.server.reactive.ServerHttpRequest;
 import org.springframework.util.Assert;
+import org.springframework.util.StringUtils;
 import org.springframework.web.server.ServerWebExchange;
 import org.springframework.web.server.WebFilter;
 import org.springframework.web.server.WebFilterChain;
-import reactor.core.publisher.Mono;
-
-import java.util.Locale;
-import java.util.Optional;
 
 /**
  * Reactive {@link WebFilter} that converts posted method parameters into HTTP methods,
  * retrievable via {@link ServerHttpRequest#getMethod()}. Since browsers currently only
- * support GET and POST, a common technique - used by the Prototype library, for instance -
- * is to use a normal POST with an additional hidden form field ({@code _method})
- * to pass the "real" HTTP method along. This filter reads that parameter and changes
- * the {@link ServerHttpRequest#getMethod()} return value using {@link ServerWebExchange#mutate()}.
+ * support GET and POST, a common technique is to use a normal POST with an additional
+ * hidden form field ({@code _method}) to pass the "real" HTTP method along.
+ * This filter reads that parameter and changes the {@link ServerHttpRequest#getMethod()}
+ * return value using {@link ServerWebExchange#mutate()}.
  *
  * <p>The name of the request parameter defaults to {@code _method}, but can be
  * adapted via the {@link #setMethodParam(String) methodParam} property.
@@ -61,37 +63,41 @@ public class HiddenHttpMethodFilter implements WebFilter {
 	 *
 	 * @param exchange the current server exchange
 	 * @param chain provides a way to delegate to the next filter
-	 * @return
+	 * @return {@code Mono<Void>} to indicate when request processing is complete
 	 */
 	@Override
 	public Mono<Void> filter(ServerWebExchange exchange, WebFilterChain chain) {
 
 		if (exchange.getRequest().getMethod() == HttpMethod.POST) {
 			return exchange.getFormData()
-				.map(map -> Optional.ofNullable(map.getFirst(methodParam)))
-				.map(method -> convertedRequest(exchange, method))
-				.then(convertedExchange -> chain.filter(convertedExchange));
-		} else {
+					.map(formData -> {
+						String method = formData.getFirst(methodParam);
+						if (StringUtils.hasLength(method)) {
+							return convertedRequest(exchange, method);
+						}
+						else {
+							return exchange;
+						}
+					})
+					.then(convertedExchange -> chain.filter(convertedExchange));
+		}
+		else {
 			return chain.filter(exchange);
 		}
 	}
 
 	/**
-	 * Mutate exchange into a new HTTP method.
+	 * Mutate exchange into a new HTTP request method.
 	 *
-	 * @param exchange - original request
-	 * @param method - request HTTP method based on form data
+	 * @param exchange original {@link ServerWebExchange}
+	 * @param method request HTTP method based on form data
 	 * @return a mutated {@link ServerWebExchange}
 	 */
-	private ServerWebExchange convertedRequest(ServerWebExchange exchange, Optional<String> method) {
-
-		String upperMethod = method
-			.map(String::toString)
-			.orElse(HttpMethod.POST.toString())
-			.toUpperCase(Locale.ENGLISH);
-
+	private ServerWebExchange convertedRequest(ServerWebExchange exchange, String method) {
+		HttpMethod resolved = HttpMethod.resolve(method.toUpperCase(Locale.ENGLISH));
+		Assert.notNull(resolved, () -> "HttpMethod '" + method + "' is not supported");
 		return exchange.mutate()
-			.request(builder -> builder.method(HttpMethod.resolve(upperMethod)))
-			.build();
+				.request(builder -> builder.method(resolved))
+				.build();
 	}
 }

spring-web/src/test/java/org/springframework/web/filter/reactive/HiddenHttpMethodFilterTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2017 the original author or authors.
+ * Copyright 2002-2017 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.
@@ -13,9 +13,16 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
+
 package org.springframework.web.filter.reactive;
 
+import java.util.Optional;
+
+import org.hamcrest.Matchers;
 import org.junit.Test;
+import reactor.core.publisher.Mono;
+import reactor.test.StepVerifier;
+
 import org.springframework.http.HttpHeaders;
 import org.springframework.http.HttpMethod;
 import org.springframework.http.MediaType;
@@ -24,14 +31,13 @@ import org.springframework.mock.http.server.reactive.test.MockServerHttpResponse
 import org.springframework.web.server.ServerWebExchange;
 import org.springframework.web.server.WebFilterChain;
 import org.springframework.web.server.adapter.DefaultServerWebExchange;
-import reactor.core.publisher.Mono;
-import reactor.test.StepVerifier;
-
-import java.util.Optional;
 
-import static org.junit.Assert.*;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertThat;
 
 /**
+ * Tests for {@link HiddenHttpMethodFilter}
+ *
  * @author Greg Turnquist
  */
 public class HiddenHttpMethodFilterTests {
@@ -48,8 +54,22 @@ public class HiddenHttpMethodFilterTests {
 		};
 
 		StepVerifier.create(filter.filter(mockExchange, filterChain))
-			.expectComplete()
-			.verify();
+				.expectComplete()
+				.verify();
+	}
+
+	@Test
+	public void filterWithInvalidParameter() {
+		ServerWebExchange mockExchange = createExchange(Optional.of("INVALID"));
+
+		WebFilterChain filterChain = exchange -> Mono.empty();
+
+		StepVerifier.create(filter.filter(mockExchange, filterChain))
+				.consumeErrorWith(error -> {
+					assertThat(error, Matchers.instanceOf(IllegalArgumentException.class));
+					assertEquals(error.getMessage(), "HttpMethod 'INVALID' is not supported");
+				})
+				.verify();
 	}
 
 	@Test
@@ -62,8 +82,22 @@ public class HiddenHttpMethodFilterTests {
 		};
 
 		StepVerifier.create(filter.filter(mockExchange, filterChain))
-			.expectComplete()
-			.verify();
+				.expectComplete()
+				.verify();
+	}
+
+	@Test
+	public void filterWithEmptyStringParameter() {
+		ServerWebExchange mockExchange = createExchange(Optional.of(""));
+
+		WebFilterChain filterChain = exchange -> {
+			assertEquals("Invalid method", HttpMethod.POST, exchange.getRequest().getMethod());
+			return Mono.empty();
+		};
+
+		StepVerifier.create(filter.filter(mockExchange, filterChain))
+				.expectComplete()
+				.verify();
 	}
 
 	@Test
@@ -78,15 +112,15 @@ public class HiddenHttpMethodFilterTests {
 		filter.setMethodParam("_foo");
 
 		StepVerifier.create(filter.filter(mockExchange, filterChain))
-			.expectComplete()
-			.verify();
+				.expectComplete()
+				.verify();
 	}
 
 	@Test
 	public void filterWithoutPost() {
 		ServerWebExchange mockExchange = createExchange(Optional.of("DELETE")).mutate()
-			.request(builder -> builder.method(HttpMethod.PUT))
-			.build();
+				.request(builder -> builder.method(HttpMethod.PUT))
+				.build();
 
 		WebFilterChain filterChain = exchange -> {
 			assertEquals("Invalid method", HttpMethod.PUT, exchange.getRequest().getMethod());
@@ -94,8 +128,8 @@ public class HiddenHttpMethodFilterTests {
 		};
 
 		StepVerifier.create(filter.filter(mockExchange, filterChain))
-			.expectComplete()
-			.verify();
+				.expectComplete()
+				.verify();
 	}
 
 	private ServerWebExchange createExchange(Optional<String> optionalMethod) {
@@ -104,12 +138,12 @@ public class HiddenHttpMethodFilterTests {
 
 	private ServerWebExchange createExchange(String methodName, Optional<String> optionalBody) {
 		MockServerHttpRequest.BodyBuilder builder = MockServerHttpRequest
-			.post("/hotels")
-			.header(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_FORM_URLENCODED_VALUE);
+				.post("/hotels")
+				.header(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_FORM_URLENCODED_VALUE);
 
 		MockServerHttpRequest request = optionalBody
-			.map(method -> builder.body(methodName + "=" + method))
-			.orElse(builder.build());
+				.map(method -> builder.body(methodName + "=" + method))
+				.orElse(builder.build());
 
 		MockServerHttpResponse response = new MockServerHttpResponse();