fix #999 Add HttpInfos#fullPath for not removing leading/trailing '/'…

… symbol
reactor · Mar 10, 2020 · e01e911 · e01e911
1 parent a43b639
commit e01e911
Show file tree

Hide file tree

Showing 8 changed files with 156 additions and 58 deletions.
diff --git a/src/main/java/reactor/netty/http/HttpInfos.java b/src/main/java/reactor/netty/http/HttpInfos.java
@@ -65,7 +65,28 @@ public interface HttpInfos {
 	 *
 	 * @return the decoded path portion from the {@link #uri()} without the leading and trailing '/' if present
 	 */
-	String path();
+	default String path() {
+		String path = fullPath();
+		if (!path.isEmpty()) {
+			if (path.charAt(0) == '/') {
+				path = path.substring(1);
+				if (path.isEmpty()) {
+					return path;
+				}
+			}
+			if (path.charAt(path.length() - 1) == '/') {
+				return path.substring(0, path.length() - 1);
+			}
+		}
+		return path;
+	}
+
+	/**
+	 * Returns the decoded path portion from the {@link #uri()}
+	 *
+	 * @return the decoded path portion from the {@link #uri()}
+	 */
+	String fullPath();
 
 	/**
 	 * Returns the resolved target address

diff --git a/src/main/java/reactor/netty/http/HttpOperations.java b/src/main/java/reactor/netty/http/HttpOperations.java
@@ -335,10 +335,10 @@ protected final boolean markSentHeaderAndBody(Object... objectsToRelease) {
 	}
 
 	/**
-	 * Returns the decoded path portion from the provided {@code uri} without the leading and trailing '/' if present
+	 * Returns the decoded path portion from the provided {@code uri}
 	 *
 	 * @param uri an HTTP URL that may contain a path with query/fragment
-	 * @return the decoded path portion from the provided {@code uri} without the leading and trailing '/' if present
+	 * @return the decoded path portion from the provided {@code uri}
 	 */
 	public static String resolvePath(String uri) {
 		if (uri.isEmpty()) {
@@ -353,20 +353,8 @@ else if (!SCHEME_PATTERN.matcher(tempUri).matches()) {
 			tempUri = "http://" + tempUri;
 		}
 
-		String path = URI.create(tempUri)
-		                 .getPath();
-		if (!path.isEmpty()) {
-			if (path.charAt(0) == '/') {
-				path = path.substring(1);
-				if (path.isEmpty()) {
-					return path;
-				}
-			}
-			if (path.charAt(path.length() - 1) == '/') {
-				return path.substring(0, path.length() - 1);
-			}
-		}
-		return path;
+		return URI.create(tempUri)
+		          .getPath();
 	}
 
 	/**

diff --git a/src/main/java/reactor/netty/http/client/HttpClientDoOnError.java b/src/main/java/reactor/netty/http/client/HttpClientDoOnError.java
@@ -167,7 +167,7 @@ public String uri() {
 		}
 
 		@Override
-		public String path() {
+		public String fullPath() {
 			return path;
 		}
 

diff --git a/src/main/java/reactor/netty/http/client/HttpClientMetricsHandler.java b/src/main/java/reactor/netty/http/client/HttpClientMetricsHandler.java
@@ -63,7 +63,7 @@ public void write(ChannelHandlerContext ctx, Object msg, ChannelPromise promise)
 			ChannelOperations<?,?> channelOps = ChannelOperations.get(ctx.channel());
 			if (channelOps instanceof HttpClientOperations) {
 				HttpClientOperations ops = (HttpClientOperations) channelOps;
-				path = "/" + ops.path;
+				path = ops.path;
 				method = ops.method().name();
 			}
 

diff --git a/src/main/java/reactor/netty/http/client/HttpClientOperations.java b/src/main/java/reactor/netty/http/client/HttpClientOperations.java
@@ -438,7 +438,7 @@ public final String uri() {
 	}
 
 	@Override
-	public final String path() {
+	public final String fullPath() {
 		return this.path;
 	}
 

diff --git a/src/main/java/reactor/netty/http/server/HttpServerMetricsHandler.java b/src/main/java/reactor/netty/http/server/HttpServerMetricsHandler.java
@@ -74,7 +74,7 @@ else if (msg instanceof ByteBuf) {
 				ChannelOperations<?,?> channelOps = ChannelOperations.get(ctx.channel());
 				if (channelOps instanceof HttpServerOperations) {
 					HttpServerOperations ops = (HttpServerOperations) channelOps;
-					String path = "/" + ops.path;
+					String path = ops.path;
 					String method = ops.method().name();
 					String status = ops.status().codeAsText().toString();
 					recorder.recordDataSentTime(
@@ -121,7 +121,7 @@ else if (msg instanceof ByteBuf) {
 			ChannelOperations<?,?> channelOps = ChannelOperations.get(ctx.channel());
 			if (channelOps instanceof HttpServerOperations) {
 				HttpServerOperations ops = (HttpServerOperations) channelOps;
-				String path = "/" + ops.path;
+				String path = ops.path;
 				String method = ops.method().name();
 				recorder.recordDataReceivedTime(path, method, Duration.ofNanos(System.nanoTime() - dataReceivedTime));
 
@@ -141,7 +141,7 @@ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) {
 		if (channelOps instanceof HttpServerOperations) {
 			HttpServerOperations ops = (HttpServerOperations) channelOps;
 			// Always take the remote address from the operations in order to consider proxy information
-			recorder.incrementErrorsCount(ops.remoteAddress(), "/" + ops.path);
+			recorder.incrementErrorsCount(ops.remoteAddress(), ops.path);
 		}
 
 		ctx.fireExceptionCaught(cause);

diff --git a/src/main/java/reactor/netty/http/server/HttpServerOperations.java b/src/main/java/reactor/netty/http/server/HttpServerOperations.java
@@ -412,7 +412,7 @@ public String uri() {
 	}
 
 	@Override
-	public String path() {
+	public String fullPath() {
 		if (path != null) {
 			return path;
 		}

diff --git a/src/test/java/reactor/netty/http/HttpOperationsTest.java b/src/test/java/reactor/netty/http/HttpOperationsTest.java
@@ -24,15 +24,21 @@
 import io.netty.handler.codec.http.DefaultHttpResponse;
 import io.netty.handler.codec.http.DefaultLastHttpContent;
 import io.netty.handler.codec.http.HttpContent;
+import io.netty.handler.codec.http.HttpMethod;
 import io.netty.handler.codec.http.HttpResponse;
 import io.netty.handler.codec.http.HttpResponseStatus;
 import io.netty.handler.codec.http.HttpVersion;
 import io.netty.handler.codec.http.LastHttpContent;
+import io.netty.handler.codec.http.cookie.Cookie;
 import io.netty.handler.codec.json.JsonObjectDecoder;
 import io.netty.util.CharsetUtil;
 import org.junit.Test;
+import reactor.core.publisher.Flux;
 import reactor.netty.Connection;
 
+import java.util.Map;
+import java.util.Set;
+
 import static org.hamcrest.CoreMatchers.instanceOf;
 import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.CoreMatchers.nullValue;
@@ -95,44 +101,127 @@ public void httpAndJsonDecoders() {
 	}
 
 	@Test
-	public void testIssue948() {
+	public void testPath() {
+		TestHttpInfos infos = new TestHttpInfos();
+		Flux<String> expectations = Flux.just("", "", "", "/", "a", "a", "", "a", "", "a", "a", "a b");
+
+		doTestPath(infos, expectations,
+				Flux.just("http://localhost:8080",
+						"http://localhost:8080/",
+						"http://localhost:8080//",
+						"http://localhost:8080///",
+						"http://localhost:8080/a",
+						"http://localhost:8080/a/",
+						"http://localhost:8080/?b",
+						"http://localhost:8080/a?b",
+						"http://localhost:8080/#b",
+						"http://localhost:8080/a#b",
+						"http://localhost:8080/a?b#c",
+						"http://localhost:8080/a%20b"));
+
+		doTestPath(infos, expectations,
+				Flux.just("localhost:8080",
+						"localhost:8080/",
+						"localhost:8080//",
+						"localhost:8080///",
+						"localhost:8080/a",
+						"localhost:8080/a/",
+						"localhost:8080/?b",
+						"localhost:8080/a?b",
+						"localhost:8080/#b",
+						"localhost:8080/a#b",
+						"localhost:8080/a?b#c",
+						"localhost:8080/a%20b"));
+
+		doTestPath(infos, expectations, Flux.just("", "/", "//", "///", "/a", "/a/", "/?b", "/a?b", "/#b", "/a#b", "/a?b#c", "/a%20b"));
+	}
+	private void doTestPath(TestHttpInfos infos, Flux<String> expectations, Flux<String> uris) {
+		uris.zipWith(expectations)
+		            .doOnNext(tuple -> {
+		                infos.uri = tuple.getT1();
+		                assertEquals(tuple.getT2(), infos.path());
+		            })
+		            .blockLast();
+	}
+
+	@Test
+	public void testFullPath() {
 		assertEquals("", HttpOperations.resolvePath("http://localhost:8080"));
-		assertEquals("", HttpOperations.resolvePath("http://localhost:8080/"));
-		assertEquals("", HttpOperations.resolvePath("http://localhost:8080//"));
-		assertEquals("/", HttpOperations.resolvePath("http://localhost:8080///"));
-		assertEquals("a", HttpOperations.resolvePath("http://localhost:8080/a"));
-		assertEquals("a", HttpOperations.resolvePath("http://localhost:8080/a/"));
-		assertEquals("", HttpOperations.resolvePath("http://localhost:8080/?b"));
-		assertEquals("a", HttpOperations.resolvePath("http://localhost:8080/a?b"));
-		assertEquals("", HttpOperations.resolvePath("http://localhost:8080/#b"));
-		assertEquals("a", HttpOperations.resolvePath("http://localhost:8080/a#b"));
-		assertEquals("a", HttpOperations.resolvePath("http://localhost:8080/a?b#c"));
-		assertEquals("a b", HttpOperations.resolvePath("http://localhost:8080/a%20b"));
+		assertEquals("/", HttpOperations.resolvePath("http://localhost:8080/"));
+		assertEquals("//", HttpOperations.resolvePath("http://localhost:8080//"));
+		assertEquals("///", HttpOperations.resolvePath("http://localhost:8080///"));
+		assertEquals("/a", HttpOperations.resolvePath("http://localhost:8080/a"));
+		assertEquals("/a/", HttpOperations.resolvePath("http://localhost:8080/a/"));
+		assertEquals("/", HttpOperations.resolvePath("http://localhost:8080/?b"));
+		assertEquals("/a", HttpOperations.resolvePath("http://localhost:8080/a?b"));
+		assertEquals("/", HttpOperations.resolvePath("http://localhost:8080/#b"));
+		assertEquals("/a", HttpOperations.resolvePath("http://localhost:8080/a#b"));
+		assertEquals("/a", HttpOperations.resolvePath("http://localhost:8080/a?b#c"));
+		assertEquals("/a b", HttpOperations.resolvePath("http://localhost:8080/a%20b"));
 
 		assertEquals("", HttpOperations.resolvePath("localhost:8080"));
-		assertEquals("", HttpOperations.resolvePath("localhost:8080/"));
-		assertEquals("", HttpOperations.resolvePath("localhost:8080//"));
-		assertEquals("/", HttpOperations.resolvePath("localhost:8080///"));
-		assertEquals("a", HttpOperations.resolvePath("localhost:8080/a"));
-		assertEquals("a", HttpOperations.resolvePath("localhost:8080/a/"));
-		assertEquals("", HttpOperations.resolvePath("localhost:8080/?b"));
-		assertEquals("a", HttpOperations.resolvePath("localhost:8080/a?b"));
-		assertEquals("", HttpOperations.resolvePath("localhost:8080/#b"));
-		assertEquals("a", HttpOperations.resolvePath("localhost:8080/a#b"));
-		assertEquals("a", HttpOperations.resolvePath("localhost:8080/a?b#c"));
-		assertEquals("a b", HttpOperations.resolvePath("localhost:8080/a%20b"));
+		assertEquals("/", HttpOperations.resolvePath("localhost:8080/"));
+		assertEquals("//", HttpOperations.resolvePath("localhost:8080//"));
+		assertEquals("///", HttpOperations.resolvePath("localhost:8080///"));
+		assertEquals("/a", HttpOperations.resolvePath("localhost:8080/a"));
+		assertEquals("/a/", HttpOperations.resolvePath("localhost:8080/a/"));
+		assertEquals("/", HttpOperations.resolvePath("localhost:8080/?b"));
+		assertEquals("/a", HttpOperations.resolvePath("localhost:8080/a?b"));
+		assertEquals("/", HttpOperations.resolvePath("localhost:8080/#b"));
+		assertEquals("/a", HttpOperations.resolvePath("localhost:8080/a#b"));
+		assertEquals("/a", HttpOperations.resolvePath("localhost:8080/a?b#c"));
+		assertEquals("/a b", HttpOperations.resolvePath("localhost:8080/a%20b"));
 
 		assertEquals("", HttpOperations.resolvePath(""));
-		assertEquals("", HttpOperations.resolvePath("/"));
-		assertEquals("", HttpOperations.resolvePath("//"));
-		assertEquals("/", HttpOperations.resolvePath("///"));
-		assertEquals("a", HttpOperations.resolvePath("/a"));
-		assertEquals("a", HttpOperations.resolvePath("/a/"));
-		assertEquals("", HttpOperations.resolvePath("/?b"));
-		assertEquals("a", HttpOperations.resolvePath("/a?b"));
-		assertEquals("", HttpOperations.resolvePath("/#b"));
-		assertEquals("a", HttpOperations.resolvePath("/a#b"));
-		assertEquals("a", HttpOperations.resolvePath("/a?b#c"));
-		assertEquals("a b", HttpOperations.resolvePath("/a%20b"));
+		assertEquals("/", HttpOperations.resolvePath("/"));
+		assertEquals("//", HttpOperations.resolvePath("//"));
+		assertEquals("///", HttpOperations.resolvePath("///"));
+		assertEquals("/a", HttpOperations.resolvePath("/a"));
+		assertEquals("/a/", HttpOperations.resolvePath("/a/"));
+		assertEquals("/", HttpOperations.resolvePath("/?b"));
+		assertEquals("/a", HttpOperations.resolvePath("/a?b"));
+		assertEquals("/", HttpOperations.resolvePath("/#b"));
+		assertEquals("/a", HttpOperations.resolvePath("/a#b"));
+		assertEquals("/a", HttpOperations.resolvePath("/a?b#c"));
+		assertEquals("/a b", HttpOperations.resolvePath("/a%20b"));
+	}
+
+	static final class TestHttpInfos implements HttpInfos {
+		String uri;
+
+		@Override
+		public Map<CharSequence, Set<Cookie>> cookies() {
+			return null;
+		}
+
+		@Override
+		public boolean isKeepAlive() {
+			return false;
+		}
+
+		@Override
+		public boolean isWebsocket() {
+			return false;
+		}
+
+		@Override
+		public HttpMethod method() {
+			return null;
+		}
+
+		@Override
+		public String fullPath() {
+			return HttpOperations.resolvePath(uri);
+		}
+
+		@Override
+		public String uri() {
+			return null;
+		}
+
+		@Override
+		public HttpVersion version() {
+			return null;
+		}
 	}
 }