From c3889873f66c946f8848291d887d20d6f125a02b Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Fri, 11 Dec 2015 12:25:54 +0100 Subject: [PATCH 01/10] 484167 - GOAWAY frames aren't handling disconnects appropriately on Client. Fixed by overriding onClose() to listen for GOAWAY frames, and acting appropriately. --- .../http/HttpClientTransportOverHTTP2.java | 67 ++++++++++++------- .../client/http/HttpConnectionOverHTTP2.java | 2 +- .../client/HttpClientIdleTimeoutTest.java | 26 +++++++ 3 files changed, 69 insertions(+), 26 deletions(-) diff --git a/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpClientTransportOverHTTP2.java b/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpClientTransportOverHTTP2.java index ea16c18feaf..6e2adcb1f17 100644 --- a/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpClientTransportOverHTTP2.java +++ b/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpClientTransportOverHTTP2.java @@ -32,6 +32,7 @@ import org.eclipse.jetty.http.HttpScheme; import org.eclipse.jetty.http2.api.Session; import org.eclipse.jetty.http2.client.HTTP2Client; import org.eclipse.jetty.http2.client.HTTP2ClientConnectionFactory; +import org.eclipse.jetty.http2.frames.GoAwayFrame; import org.eclipse.jetty.io.ClientConnectionFactory; import org.eclipse.jetty.io.EndPoint; import org.eclipse.jetty.io.ssl.SslClientConnectionFactory; @@ -107,37 +108,15 @@ public class HttpClientTransportOverHTTP2 extends ContainerLifeCycle implements final HttpDestination destination = (HttpDestination)context.get(HTTP_DESTINATION_CONTEXT_KEY); @SuppressWarnings("unchecked") - final Promise connection = (Promise)context.get(HTTP_CONNECTION_PROMISE_CONTEXT_KEY); + final Promise connectionPromise = (Promise)context.get(HTTP_CONNECTION_PROMISE_CONTEXT_KEY); - Session.Listener listener = new Session.Listener.Adapter() - { - @Override - public void onFailure(Session session, Throwable failure) - { - destination.abort(failure); - } - }; - - final Promise promise = new Promise() - { - @Override - public void succeeded(Session session) - { - connection.succeeded(newHttpConnection(destination, session)); - } - - @Override - public void failed(Throwable failure) - { - connection.failed(failure); - } - }; + SessionListenerPromise listenerPromise = new SessionListenerPromise(destination, connectionPromise); SslContextFactory sslContextFactory = null; if (HttpScheme.HTTPS.is(destination.getScheme())) sslContextFactory = httpClient.getSslContextFactory(); - client.connect(sslContextFactory, address, listener, promise, context); + client.connect(sslContextFactory, address, listenerPromise, listenerPromise, context); } @Override @@ -154,4 +133,42 @@ public class HttpClientTransportOverHTTP2 extends ContainerLifeCycle implements { return new HttpConnectionOverHTTP2(destination, session); } + + private class SessionListenerPromise extends Session.Listener.Adapter implements Promise + { + private final HttpDestination destination; + private final Promise promise; + private HttpConnectionOverHTTP2 connection; + + public SessionListenerPromise(HttpDestination destination, Promise promise) + { + this.destination = destination; + this.promise = promise; + } + + @Override + public void succeeded(Session session) + { + connection = newHttpConnection(destination, session); + promise.succeeded(connection); + } + + @Override + public void failed(Throwable failure) + { + promise.failed(failure); + } + + @Override + public void onClose(Session session, GoAwayFrame frame) + { + connection.close(); + } + + @Override + public void onFailure(Session session, Throwable failure) + { + connection.abort(failure); + } + } } diff --git a/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpConnectionOverHTTP2.java b/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpConnectionOverHTTP2.java index 0ad973f28f1..98bff3286a1 100644 --- a/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpConnectionOverHTTP2.java +++ b/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpConnectionOverHTTP2.java @@ -69,7 +69,7 @@ public class HttpConnectionOverHTTP2 extends HttpConnection abort(new AsynchronousCloseException()); } - private void abort(Throwable failure) + protected void abort(Throwable failure) { for (HttpChannel channel : channels) { diff --git a/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/HttpClientIdleTimeoutTest.java b/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/HttpClientIdleTimeoutTest.java index e168a0f3ef4..c38bceb2960 100644 --- a/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/HttpClientIdleTimeoutTest.java +++ b/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/HttpClientIdleTimeoutTest.java @@ -27,6 +27,8 @@ import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; +import org.eclipse.jetty.client.api.ContentResponse; +import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.server.Request; import org.eclipse.jetty.server.handler.AbstractHandler; import org.junit.Assert; @@ -93,4 +95,28 @@ public class HttpClientIdleTimeoutTest extends AbstractTest Assert.assertTrue(latch.await(2 * idleTimeout, TimeUnit.MILLISECONDS)); } + + @Test + public void testServerIdleTimeout() throws Exception + { + start(new AbstractHandler() + { + @Override + public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + { + baseRequest.setHandled(true); + } + }); + connector.setIdleTimeout(idleTimeout); + + ContentResponse response1 = client.newRequest(newURI()).send(); + Assert.assertEquals(HttpStatus.OK_200, response1.getStatus()); + + // Let the server idle timeout. + Thread.sleep(2 * idleTimeout); + + // Make sure we can make another request successfully. + ContentResponse response2 = client.newRequest(newURI()).send(); + Assert.assertEquals(HttpStatus.OK_200, response2.getStatus()); + } } From 77e0df1193c38e2fef6adeba855ff68d2ec0b2e3 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Fri, 11 Dec 2015 12:34:48 +0100 Subject: [PATCH 02/10] 484167 - GOAWAY frames aren't handling disconnects appropriately on Client. Improved handling of the failure case. --- .../http2/client/http/HttpClientTransportOverHTTP2.java | 2 +- .../jetty/http2/client/http/HttpConnectionOverHTTP2.java | 9 +++++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpClientTransportOverHTTP2.java b/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpClientTransportOverHTTP2.java index 6e2adcb1f17..c273aa4bb9e 100644 --- a/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpClientTransportOverHTTP2.java +++ b/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpClientTransportOverHTTP2.java @@ -168,7 +168,7 @@ public class HttpClientTransportOverHTTP2 extends ContainerLifeCycle implements @Override public void onFailure(Session session, Throwable failure) { - connection.abort(failure); + connection.close(failure); } } } diff --git a/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpConnectionOverHTTP2.java b/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpConnectionOverHTTP2.java index 98bff3286a1..f166728f772 100644 --- a/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpConnectionOverHTTP2.java +++ b/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpConnectionOverHTTP2.java @@ -61,15 +61,20 @@ public class HttpConnectionOverHTTP2 extends HttpConnection @Override public void close() + { + close(new AsynchronousCloseException()); + } + + protected void close(Throwable failure) { // First close then abort, to be sure that the connection cannot be reused // from an onFailure() handler or by blocking code waiting for completion. getHttpDestination().close(this); session.close(ErrorCode.NO_ERROR.code, null, Callback.NOOP); - abort(new AsynchronousCloseException()); + abort(failure); } - protected void abort(Throwable failure) + private void abort(Throwable failure) { for (HttpChannel channel : channels) { From e674d3ec5e90c2993563526aeb39714fc0bcedf2 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Fri, 11 Dec 2015 17:58:31 +0100 Subject: [PATCH 03/10] 483878 - Parallel requests stuck via the http client transport over HTTP/2. --- .../org/eclipse/jetty/client/MultiplexHttpDestination.java | 6 ++++++ .../jetty/http2/client/http/HttpConnectionOverHTTP2.java | 1 + 2 files changed, 7 insertions(+) diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/MultiplexHttpDestination.java b/jetty-client/src/main/java/org/eclipse/jetty/client/MultiplexHttpDestination.java index a50131f1ed2..76f57fd6cba 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/MultiplexHttpDestination.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/MultiplexHttpDestination.java @@ -119,6 +119,12 @@ public abstract class MultiplexHttpDestination extends Htt return true; } + @Override + public void release(Connection connection) + { + send(); + } + @Override public void close() { diff --git a/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpConnectionOverHTTP2.java b/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpConnectionOverHTTP2.java index f166728f772..09ae404448b 100644 --- a/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpConnectionOverHTTP2.java +++ b/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpConnectionOverHTTP2.java @@ -57,6 +57,7 @@ public class HttpConnectionOverHTTP2 extends HttpConnection protected void release(HttpChannel channel) { channels.remove(channel); + getHttpDestination().release(this); } @Override From 8d28be57868d7197286955689d37396b82edafa0 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Fri, 11 Dec 2015 18:00:48 +0100 Subject: [PATCH 04/10] 484210 - HttpClient over HTTP/2 should honor maxConcurrentStreams. Fixed by sending queued requests in a loop up to maxConcurrentStreams. Also updating the maxConcurrentStreams value when received from the server. --- .../client/MultiplexHttpDestination.java | 75 +++++--- .../http2-http-client-transport/pom.xml | 17 ++ .../http/HttpClientTransportOverHTTP2.java | 17 +- .../jetty/http2/client/http/AbstractTest.java | 70 ++++++++ .../client/http/MaxConcurrentStreamsTest.java | 165 ++++++++++++++++++ 5 files changed, 316 insertions(+), 28 deletions(-) create mode 100644 jetty-http2/http2-http-client-transport/src/test/java/org/eclipse/jetty/http2/client/http/AbstractTest.java create mode 100644 jetty-http2/http2-http-client-transport/src/test/java/org/eclipse/jetty/http2/client/http/MaxConcurrentStreamsTest.java diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/MultiplexHttpDestination.java b/jetty-client/src/main/java/org/eclipse/jetty/client/MultiplexHttpDestination.java index 76f57fd6cba..d221b7ebbfb 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/MultiplexHttpDestination.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/MultiplexHttpDestination.java @@ -18,6 +18,7 @@ package org.eclipse.jetty.client; +import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; import org.eclipse.jetty.client.api.Connection; @@ -27,6 +28,8 @@ import org.eclipse.jetty.util.Promise; public abstract class MultiplexHttpDestination extends HttpDestination implements Promise { private final AtomicReference connect = new AtomicReference<>(ConnectState.DISCONNECTED); + private final AtomicInteger requestsPerConnection = new AtomicInteger(); + private int maxRequestsPerConnection = 1024; private C connection; protected MultiplexHttpDestination(HttpClient client, Origin origin) @@ -34,6 +37,16 @@ public abstract class MultiplexHttpDestination extends Htt super(client, origin); } + public int getMaxRequestsPerConnection() + { + return maxRequestsPerConnection; + } + + public void setMaxRequestsPerConnection(int maxRequestsPerConnection) + { + this.maxRequestsPerConnection = maxRequestsPerConnection; + } + @Override public void send() { @@ -56,8 +69,7 @@ public abstract class MultiplexHttpDestination extends Htt } case CONNECTED: { - if (process(connection)) - break; + process(connection); return; } default: @@ -92,36 +104,51 @@ public abstract class MultiplexHttpDestination extends Htt abort(x); } - protected boolean process(final C connection) + protected void process(final C connection) { - HttpClient client = getHttpClient(); - final HttpExchange exchange = getHttpExchanges().poll(); - if (LOG.isDebugEnabled()) - LOG.debug("Processing {} on {}", exchange, connection); - if (exchange == null) - return false; + while (true) + { + int max = getMaxRequestsPerConnection(); + int count = requestsPerConnection.get(); + int next = count + 1; + if (next > max) + return; - final Request request = exchange.getRequest(); - Throwable cause = request.getAbortCause(); - if (cause != null) - { - if (LOG.isDebugEnabled()) - LOG.debug("Aborted before processing {}: {}", exchange, cause); - // It may happen that the request is aborted before the exchange - // is created. Aborting the exchange a second time will result in - // a no-operation, so we just abort here to cover that edge case. - exchange.abort(cause); + if (requestsPerConnection.compareAndSet(count, next)) + { + HttpExchange exchange = getHttpExchanges().poll(); + if (LOG.isDebugEnabled()) + LOG.debug("Processing {}/{} {} on {}", next, max, exchange, connection); + if (exchange == null) + { + requestsPerConnection.decrementAndGet(); + return; + } + + final Request request = exchange.getRequest(); + Throwable cause = request.getAbortCause(); + if (cause != null) + { + if (LOG.isDebugEnabled()) + LOG.debug("Aborted before processing {}: {}", exchange, cause); + // It may happen that the request is aborted before the exchange + // is created. Aborting the exchange a second time will result in + // a no-operation, so we just abort here to cover that edge case. + exchange.abort(cause); + requestsPerConnection.decrementAndGet(); + } + else + { + send(connection, exchange); + } + } } - else - { - send(connection, exchange); - } - return true; } @Override public void release(Connection connection) { + requestsPerConnection.decrementAndGet(); send(); } diff --git a/jetty-http2/http2-http-client-transport/pom.xml b/jetty-http2/http2-http-client-transport/pom.xml index 728b0fc3284..90517afd3d2 100644 --- a/jetty-http2/http2-http-client-transport/pom.xml +++ b/jetty-http2/http2-http-client-transport/pom.xml @@ -61,6 +61,23 @@ ${project.version} + + org.eclipse.jetty.toolchain + jetty-test-helper + test + + + org.eclipse.jetty + jetty-server + ${project.version} + test + + + org.eclipse.jetty.http2 + http2-server + ${project.version} + test + junit junit diff --git a/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpClientTransportOverHTTP2.java b/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpClientTransportOverHTTP2.java index c273aa4bb9e..6dd6c230518 100644 --- a/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpClientTransportOverHTTP2.java +++ b/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpClientTransportOverHTTP2.java @@ -33,6 +33,7 @@ import org.eclipse.jetty.http2.api.Session; import org.eclipse.jetty.http2.client.HTTP2Client; import org.eclipse.jetty.http2.client.HTTP2ClientConnectionFactory; import org.eclipse.jetty.http2.frames.GoAwayFrame; +import org.eclipse.jetty.http2.frames.SettingsFrame; import org.eclipse.jetty.io.ClientConnectionFactory; import org.eclipse.jetty.io.EndPoint; import org.eclipse.jetty.io.ssl.SslClientConnectionFactory; @@ -106,9 +107,9 @@ public class HttpClientTransportOverHTTP2 extends ContainerLifeCycle implements { client.setConnectTimeout(httpClient.getConnectTimeout()); - final HttpDestination destination = (HttpDestination)context.get(HTTP_DESTINATION_CONTEXT_KEY); + HttpDestinationOverHTTP2 destination = (HttpDestinationOverHTTP2)context.get(HTTP_DESTINATION_CONTEXT_KEY); @SuppressWarnings("unchecked") - final Promise connectionPromise = (Promise)context.get(HTTP_CONNECTION_PROMISE_CONTEXT_KEY); + Promise connectionPromise = (Promise)context.get(HTTP_CONNECTION_PROMISE_CONTEXT_KEY); SessionListenerPromise listenerPromise = new SessionListenerPromise(destination, connectionPromise); @@ -136,11 +137,11 @@ public class HttpClientTransportOverHTTP2 extends ContainerLifeCycle implements private class SessionListenerPromise extends Session.Listener.Adapter implements Promise { - private final HttpDestination destination; + private final HttpDestinationOverHTTP2 destination; private final Promise promise; private HttpConnectionOverHTTP2 connection; - public SessionListenerPromise(HttpDestination destination, Promise promise) + public SessionListenerPromise(HttpDestinationOverHTTP2 destination, Promise promise) { this.destination = destination; this.promise = promise; @@ -159,6 +160,14 @@ public class HttpClientTransportOverHTTP2 extends ContainerLifeCycle implements promise.failed(failure); } + @Override + public void onSettings(Session session, SettingsFrame frame) + { + Map settings = frame.getSettings(); + if (settings.containsKey(SettingsFrame.MAX_CONCURRENT_STREAMS)) + destination.setMaxRequestsPerConnection(settings.get(SettingsFrame.MAX_CONCURRENT_STREAMS)); + } + @Override public void onClose(Session session, GoAwayFrame frame) { diff --git a/jetty-http2/http2-http-client-transport/src/test/java/org/eclipse/jetty/http2/client/http/AbstractTest.java b/jetty-http2/http2-http-client-transport/src/test/java/org/eclipse/jetty/http2/client/http/AbstractTest.java new file mode 100644 index 00000000000..66f99929635 --- /dev/null +++ b/jetty-http2/http2-http-client-transport/src/test/java/org/eclipse/jetty/http2/client/http/AbstractTest.java @@ -0,0 +1,70 @@ +// +// ======================================================================== +// Copyright (c) 1995-2015 Mort Bay Consulting Pty. Ltd. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.http2.client.http; + +import org.eclipse.jetty.client.HttpClient; +import org.eclipse.jetty.http2.client.HTTP2Client; +import org.eclipse.jetty.http2.server.HTTP2ServerConnectionFactory; +import org.eclipse.jetty.server.Handler; +import org.eclipse.jetty.server.HttpConfiguration; +import org.eclipse.jetty.server.Server; +import org.eclipse.jetty.server.ServerConnector; +import org.eclipse.jetty.toolchain.test.TestTracker; +import org.eclipse.jetty.util.thread.QueuedThreadPool; +import org.junit.After; +import org.junit.Rule; + +public class AbstractTest +{ + @Rule + public TestTracker tracker = new TestTracker(); + protected Server server; + protected ServerConnector connector; + protected HttpClient client; + + protected void start(int maxConcurrentStreams, Handler handler) throws Exception + { + QueuedThreadPool serverExecutor = new QueuedThreadPool(); + serverExecutor.setName("server"); + server = new Server(serverExecutor); + + HTTP2ServerConnectionFactory http2 = new HTTP2ServerConnectionFactory(new HttpConfiguration()); + http2.setMaxConcurrentStreams(maxConcurrentStreams); + connector = new ServerConnector(server, 1, 1, http2); + server.addConnector(connector); + + server.setHandler(handler); + server.start(); + + client = new HttpClient(new HttpClientTransportOverHTTP2(new HTTP2Client()), null); + QueuedThreadPool clientExecutor = new QueuedThreadPool(); + clientExecutor.setName("client"); + client.setExecutor(clientExecutor); + client.start(); + } + + @After + public void dispose() throws Exception + { + if (client != null) + client.stop(); + if (server != null) + server.stop(); + } +} diff --git a/jetty-http2/http2-http-client-transport/src/test/java/org/eclipse/jetty/http2/client/http/MaxConcurrentStreamsTest.java b/jetty-http2/http2-http-client-transport/src/test/java/org/eclipse/jetty/http2/client/http/MaxConcurrentStreamsTest.java new file mode 100644 index 00000000000..0df843e320d --- /dev/null +++ b/jetty-http2/http2-http-client-transport/src/test/java/org/eclipse/jetty/http2/client/http/MaxConcurrentStreamsTest.java @@ -0,0 +1,165 @@ +// +// ======================================================================== +// Copyright (c) 1995-2015 Mort Bay Consulting Pty. Ltd. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.http2.client.http; + +import java.io.IOException; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; + +import javax.servlet.ServletException; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +import org.eclipse.jetty.client.api.ContentResponse; +import org.eclipse.jetty.http.HttpStatus; +import org.eclipse.jetty.server.Request; +import org.eclipse.jetty.server.handler.AbstractHandler; +import org.junit.Assert; +import org.junit.Test; + +public class MaxConcurrentStreamsTest extends AbstractTest +{ + @Test + public void testOneConcurrentStream() throws Exception + { + long sleep = 1000; + start(1, new AbstractHandler() + { + @Override + public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + { + baseRequest.setHandled(true); + // Sleep a bit to allow the second request to be queued. + sleep(sleep); + } + }); + + // Prime the connection so that the maxConcurrentStream setting arrives to the client. + client.newRequest("localhost", connector.getLocalPort()).path("/prime").send(); + + CountDownLatch latch = new CountDownLatch(2); + + // First request is sent immediately. + client.newRequest("localhost", connector.getLocalPort()) + .path("/first") + .send(result -> + { + if (result.isSucceeded()) + latch.countDown(); + }); + + // Second request is queued. + client.newRequest("localhost", connector.getLocalPort()) + .path("/second") + .send(result -> + { + if (result.isSucceeded()) + latch.countDown(); + }); + + // When the first request returns, the second must be sent. + Assert.assertTrue(latch.await(5 * sleep, TimeUnit.MILLISECONDS)); + } + + @Test + public void testTwoConcurrentStreamsThirdWaits() throws Exception + { + int maxStreams = 2; + long sleep = 1000; + start(maxStreams, new AbstractHandler() + { + @Override + public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + { + baseRequest.setHandled(true); + sleep(sleep); + } + }); + + // Prime the connection so that the maxConcurrentStream setting arrives to the client. + client.newRequest("localhost", connector.getLocalPort()).path("/prime").send(); + + // Send requests up to the max allowed. + for (int i = 0; i < maxStreams; ++i) + { + client.newRequest("localhost", connector.getLocalPort()) + .path("/" + i) + .send(null); + } + + // Send the request in excess. + CountDownLatch latch = new CountDownLatch(1); + String path = "/excess"; + client.newRequest("localhost", connector.getLocalPort()) + .path(path) + .send(result -> + { + if (result.getResponse().getStatus() == HttpStatus.OK_200) + latch.countDown(); + }); + + // The last exchange should remain in the queue. + HttpDestinationOverHTTP2 destination = (HttpDestinationOverHTTP2)client.getDestination("http", "localhost", connector.getLocalPort()); + Assert.assertEquals(1, destination.getHttpExchanges().size()); + Assert.assertEquals(path, destination.getHttpExchanges().peek().getRequest().getPath()); + + Assert.assertTrue(latch.await(5 * sleep, TimeUnit.MILLISECONDS)); + } + + @Test + public void testAbortedWhileQueued() throws Exception + { + long sleep = 1000; + start(1, new AbstractHandler() + { + @Override + public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + { + baseRequest.setHandled(true); + sleep(sleep); + } + }); + + // Prime the connection so that the maxConcurrentStream setting arrives to the client. + client.newRequest("localhost", connector.getLocalPort()).path("/prime").send(); + + // Send a request that is aborted while queued. + client.newRequest("localhost", connector.getLocalPort()) + .path("/aborted") + .onRequestQueued(request -> request.abort(new Exception())) + .send(null); + + // Must be able to send another request. + ContentResponse response = client.newRequest("localhost", connector.getLocalPort()).path("/check").send(); + + Assert.assertEquals(HttpStatus.OK_200, response.getStatus()); + } + + private void sleep(long time) + { + try + { + Thread.sleep(time); + } + catch (InterruptedException x) + { + throw new RuntimeException(x); + } + } +} From bf9f39dc17fb36c97eada2a67bd208ae75016e93 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Mon, 14 Dec 2015 15:00:19 +0100 Subject: [PATCH 05/10] Improved exception reporting. --- .../java/org/eclipse/jetty/client/MultiplexHttpDestination.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/MultiplexHttpDestination.java b/jetty-client/src/main/java/org/eclipse/jetty/client/MultiplexHttpDestination.java index d221b7ebbfb..fbf8bd9cb23 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/MultiplexHttpDestination.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/MultiplexHttpDestination.java @@ -93,7 +93,7 @@ public abstract class MultiplexHttpDestination extends Htt else { connection.close(); - failed(new IllegalStateException()); + failed(new IllegalStateException("Invalid connection state " + connect)); } } From dddba5b0043c7f821602b22f5ddab38a6e813309 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Mon, 14 Dec 2015 11:17:31 -0700 Subject: [PATCH 06/10] 484349 - Promote WebSocket PathMappings / PathSpec to Jetty Http + Moving PathMappings from jetty-websocket to jetty-http + Renaming WebSocketPathSpec to UriTemplatePathSpec + Improving implementation with knowledge gained from PathMap and PathMapTest cases. --- .../jetty/http/pathmap/MappedResource.java | 101 ++++++ .../jetty/http/pathmap/PathMappings.java | 145 ++++++++ .../eclipse/jetty/http/pathmap/PathSpec.java | 167 +++++++++ .../jetty/http/pathmap/PathSpecGroup.java | 101 ++++++ .../jetty/http/pathmap/RegexPathSpec.java | 176 +++++++++ .../jetty/http/pathmap/ServletPathSpec.java | 261 ++++++++++++++ .../http/pathmap/UriTemplatePathSpec.java | 341 ++++++++++++++++++ .../jetty/http/pathmap/PathMappingsTest.java | 320 ++++++++++++++++ .../jetty/http/pathmap/PathSpecAssert.java | 37 ++ .../jetty/http/pathmap/RegexPathSpecTest.java | 135 +++++++ .../pathmap/ServletPathSpecMatchListTest.java | 101 ++++++ .../pathmap/ServletPathSpecOrderTest.java | 103 ++++++ .../http/pathmap/ServletPathSpecTest.java | 188 ++++++++++ .../UriTemplatePathSpecBadSpecsTest.java | 87 +++++ .../http/pathmap/UriTemplatePathSpecTest.java | 284 +++++++++++++++ 15 files changed, 2547 insertions(+) create mode 100644 jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/MappedResource.java create mode 100644 jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/PathMappings.java create mode 100644 jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/PathSpec.java create mode 100644 jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/PathSpecGroup.java create mode 100644 jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/RegexPathSpec.java create mode 100644 jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/ServletPathSpec.java create mode 100644 jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/UriTemplatePathSpec.java create mode 100644 jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/PathMappingsTest.java create mode 100644 jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/PathSpecAssert.java create mode 100644 jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/RegexPathSpecTest.java create mode 100644 jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/ServletPathSpecMatchListTest.java create mode 100644 jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/ServletPathSpecOrderTest.java create mode 100644 jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/ServletPathSpecTest.java create mode 100644 jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/UriTemplatePathSpecBadSpecsTest.java create mode 100644 jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/UriTemplatePathSpecTest.java diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/MappedResource.java b/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/MappedResource.java new file mode 100644 index 00000000000..b44aff11d56 --- /dev/null +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/MappedResource.java @@ -0,0 +1,101 @@ +// +// ======================================================================== +// Copyright (c) 1995-2015 Mort Bay Consulting Pty. Ltd. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.http.pathmap; + +import org.eclipse.jetty.util.annotation.ManagedAttribute; +import org.eclipse.jetty.util.annotation.ManagedObject; + +@ManagedObject("Mapped Resource") +public class MappedResource implements Comparable> +{ + private final PathSpec pathSpec; + private final E resource; + + public MappedResource(PathSpec pathSpec, E resource) + { + this.pathSpec = pathSpec; + this.resource = resource; + } + + /** + * Comparison is based solely on the pathSpec + */ + @Override + public int compareTo(MappedResource other) + { + return this.pathSpec.compareTo(other.pathSpec); + } + + @Override + public boolean equals(Object obj) + { + if (this == obj) + { + return true; + } + if (obj == null) + { + return false; + } + if (getClass() != obj.getClass()) + { + return false; + } + MappedResource other = (MappedResource)obj; + if (pathSpec == null) + { + if (other.pathSpec != null) + { + return false; + } + } + else if (!pathSpec.equals(other.pathSpec)) + { + return false; + } + return true; + } + + @ManagedAttribute(value = "path spec", readonly = true) + public PathSpec getPathSpec() + { + return pathSpec; + } + + @ManagedAttribute(value = "resource", readonly = true) + public E getResource() + { + return resource; + } + + @Override + public int hashCode() + { + final int prime = 31; + int result = 1; + result = (prime * result) + ((pathSpec == null) ? 0 : pathSpec.hashCode()); + return result; + } + + @Override + public String toString() + { + return String.format("MappedResource[pathSpec=%s,resource=%s]",pathSpec,resource); + } +} \ No newline at end of file diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/PathMappings.java b/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/PathMappings.java new file mode 100644 index 00000000000..e0d975ca7df --- /dev/null +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/PathMappings.java @@ -0,0 +1,145 @@ +// +// ======================================================================== +// Copyright (c) 1995-2015 Mort Bay Consulting Pty. Ltd. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.http.pathmap; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.Iterator; +import java.util.List; + +import org.eclipse.jetty.util.annotation.ManagedAttribute; +import org.eclipse.jetty.util.annotation.ManagedObject; +import org.eclipse.jetty.util.component.ContainerLifeCycle; +import org.eclipse.jetty.util.component.Dumpable; +import org.eclipse.jetty.util.log.Log; +import org.eclipse.jetty.util.log.Logger; + +/** + * Path Mappings of PathSpec to Resource. + *

+ * Sorted into search order upon entry into the Set + * + * @param the type of mapping endpoint + */ +@ManagedObject("Path Mappings") +public class PathMappings implements Iterable>, Dumpable +{ + private static final Logger LOG = Log.getLogger(PathMappings.class); + private List> mappings = new ArrayList>(); + private MappedResource defaultResource = null; + + @Override + public String dump() + { + return ContainerLifeCycle.dump(this); + } + + @Override + public void dump(Appendable out, String indent) throws IOException + { + ContainerLifeCycle.dump(out,indent,mappings); + } + + @ManagedAttribute(value = "mappings", readonly = true) + public List> getMappings() + { + return mappings; + } + + public void reset() + { + mappings.clear(); + } + + /** + * Return a list of MappedResource matches for the specified path. + * + * @param path the path to return matches on + * @return the list of mapped resource the path matches on + */ + public List> getMatches(String path) + { + boolean matchRoot = "/".equals(path); + + List> ret = new ArrayList<>(); + int len = mappings.size(); + for (int i = 0; i < len; i++) + { + MappedResource mr = mappings.get(i); + + switch (mr.getPathSpec().group) + { + case ROOT: + if (matchRoot) + ret.add(mr); + break; + case DEFAULT: + if (matchRoot || mr.getPathSpec().matches(path)) + ret.add(mr); + break; + default: + if (mr.getPathSpec().matches(path)) + ret.add(mr); + break; + } + } + return ret; + } + + public MappedResource getMatch(String path) + { + int len = mappings.size(); + for (int i = 0; i < len; i++) + { + MappedResource mr = mappings.get(i); + if (mr.getPathSpec().matches(path)) + { + return mr; + } + } + return defaultResource; + } + + @Override + public Iterator> iterator() + { + return mappings.iterator(); + } + + public void put(PathSpec pathSpec, E resource) + { + MappedResource entry = new MappedResource<>(pathSpec,resource); + if (pathSpec.group == PathSpecGroup.DEFAULT) + { + defaultResource = entry; + } + // TODO: warning on replacement of existing mapping? + mappings.add(entry); + if (LOG.isDebugEnabled()) + LOG.debug("Added {} to {}",entry,this); + Collections.sort(mappings); + } + + @Override + public String toString() + { + return String.format("%s[size=%d]",this.getClass().getSimpleName(),mappings.size()); + } +} diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/PathSpec.java b/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/PathSpec.java new file mode 100644 index 00000000000..122211a5586 --- /dev/null +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/PathSpec.java @@ -0,0 +1,167 @@ +// +// ======================================================================== +// Copyright (c) 1995-2015 Mort Bay Consulting Pty. Ltd. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.http.pathmap; + +/** + * The base PathSpec, what all other path specs are based on + */ +public abstract class PathSpec implements Comparable +{ + protected String pathSpec; + protected PathSpecGroup group; + protected int pathDepth; + protected int specLength; + + @Override + public int compareTo(PathSpec other) + { + // Grouping (increasing) + int diff = this.group.ordinal() - other.group.ordinal(); + if (diff != 0) + { + return diff; + } + + // Spec Length (decreasing) + diff = other.specLength - this.specLength; + if (diff != 0) + { + return diff; + } + + // Path Spec Name (alphabetical) + return this.pathSpec.compareTo(other.pathSpec); + } + + @Override + public boolean equals(Object obj) + { + if (this == obj) + { + return true; + } + if (obj == null) + { + return false; + } + if (getClass() != obj.getClass()) + { + return false; + } + PathSpec other = (PathSpec)obj; + if (pathSpec == null) + { + if (other.pathSpec != null) + { + return false; + } + } + else if (!pathSpec.equals(other.pathSpec)) + { + return false; + } + return true; + } + + public PathSpecGroup getGroup() + { + return group; + } + + /** + * Get the number of path elements that this path spec declares. + *

+ * This is used to determine longest match logic. + * + * @return the depth of the path segments that this spec declares + */ + public int getPathDepth() + { + return pathDepth; + } + + /** + * Return the portion of the path that is after the path spec. + * + * @param path + * the path to match against + * @return the path info portion of the string + */ + public abstract String getPathInfo(String path); + + /** + * Return the portion of the path that matches a path spec. + * + * @param path + * the path to match against + * @return the match, or null if no match at all + */ + public abstract String getPathMatch(String path); + + /** + * The as-provided path spec. + * + * @return the as-provided path spec + */ + public String getPathSpec() + { + return pathSpec; + } + + /** + * Get the relative path. + * + * @param base + * the base the path is relative to + * @param path + * the additional path + * @return the base plus path with pathSpec portion removed + */ + public abstract String getRelativePath(String base, String path); + + @Override + public int hashCode() + { + final int prime = 31; + int result = 1; + result = (prime * result) + ((pathSpec == null)?0:pathSpec.hashCode()); + return result; + } + + /** + * Test to see if the provided path matches this path spec + * + * @param path + * the path to test + * @return true if the path matches this path spec, false otherwise + */ + public abstract boolean matches(String path); + + @Override + public String toString() + { + StringBuilder str = new StringBuilder(); + str.append(this.getClass().getSimpleName()).append("[\""); + str.append(pathSpec); + str.append("\",pathDepth=").append(pathDepth); + str.append(",group=").append(group); + str.append("]"); + return str.toString(); + } +} diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/PathSpecGroup.java b/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/PathSpecGroup.java new file mode 100644 index 00000000000..3a191a94e8b --- /dev/null +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/PathSpecGroup.java @@ -0,0 +1,101 @@ +// +// ======================================================================== +// Copyright (c) 1995-2015 Mort Bay Consulting Pty. Ltd. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.http.pathmap; + +/** + * Types of path spec groups. + *

+ * This is used to facilitate proper pathspec search order. + *

+ * Search Order: + *

    + *
  1. {@link PathSpecGroup#ordinal()} [increasing]
    2. + *
  2. {@link PathSpec#specLength} [decreasing]
    3. + *
  3. {@link PathSpec#pathSpec} [natural sort order]
    4. + *
+ */ +public enum PathSpecGroup +{ + // NOTE: Order of enums determines order of Groups. + + /** + * For exactly defined path specs, no glob. + */ + EXACT, + /** + * For path specs that have a hardcoded prefix and suffix with wildcard glob in the middle. + * + * 
+     *   "^/downloads/[^/]*.zip$"  - regex spec
+     *   "/a/{var}/c"              - uri-template spec
+     *
+ * + * Note: there is no known servlet spec variant of this kind of path spec + */ + MIDDLE_GLOB, + /** + * For path specs that have a hardcoded prefix and a trailing wildcard glob. + *

+ * + * 

+     *   "/downloads/*"          - servlet spec
+     *   "/api/*"                - servlet spec
+     *   "^/rest/.*$"            - regex spec
+     *   "/bookings/{guest-id}"  - uri-template spec
+     *   "/rewards/{vip-level}"  - uri-template spec
+     *
+ */ + PREFIX_GLOB, + /** + * For path specs that have a wildcard glob with a hardcoded suffix + * + * 
+     *   "*.do"        - servlet spec
+     *   "*.css"       - servlet spec
+     *   "^.*\.zip$"   - regex spec
+     *
+ * + * Note: there is no known uri-template spec variant of this kind of path spec + */ + SUFFIX_GLOB, + /** + * The root spec for accessing the Root behavior. + * + * 
+     *   ""           - servlet spec       (Root Servlet)
+     *   null         - servlet spec       (Root Servlet)
+     *
+ * + * Note: there is no known uri-template spec variant of this kind of path spec + */ + ROOT, + /** + * The default spec for accessing the Default path behavior. + * + * 
+     *   "/"           - servlet spec      (Default Servlet)
+     *   "/"           - uri-template spec (Root Context)
+     *   "^/$"         - regex spec        (Root Context)
+     *
+ * + * Per Servlet Spec, pathInfo is always null for these specs. + * If nothing above matches, then default will match. + */ + DEFAULT, +} diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/RegexPathSpec.java b/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/RegexPathSpec.java new file mode 100644 index 00000000000..4f9a0a23abe --- /dev/null +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/RegexPathSpec.java @@ -0,0 +1,176 @@ +// +// ======================================================================== +// Copyright (c) 1995-2015 Mort Bay Consulting Pty. Ltd. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.http.pathmap; + +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +public class RegexPathSpec extends PathSpec +{ + protected Pattern pattern; + + protected RegexPathSpec() + { + super(); + } + + public RegexPathSpec(String regex) + { + super.pathSpec = regex; + boolean inGrouping = false; + this.pathDepth = 0; + this.specLength = pathSpec.length(); + // build up a simple signature we can use to identify the grouping + StringBuilder signature = new StringBuilder(); + for (char c : pathSpec.toCharArray()) + { + switch (c) + { + case '[': + inGrouping = true; + break; + case ']': + inGrouping = false; + signature.append('g'); // glob + break; + case '*': + signature.append('g'); // glob + break; + case '/': + if (!inGrouping) + { + this.pathDepth++; + } + break; + default: + if (!inGrouping) + { + if (Character.isLetterOrDigit(c)) + { + signature.append('l'); // literal (exact) + } + } + break; + } + } + this.pattern = Pattern.compile(pathSpec); + + // Figure out the grouping based on the signature + String sig = signature.toString(); + + if (Pattern.matches("^l*$",sig)) + { + this.group = PathSpecGroup.EXACT; + } + else if (Pattern.matches("^l*g+",sig)) + { + this.group = PathSpecGroup.PREFIX_GLOB; + } + else if (Pattern.matches("^g+l+$",sig)) + { + this.group = PathSpecGroup.SUFFIX_GLOB; + } + else + { + this.group = PathSpecGroup.MIDDLE_GLOB; + } + } + + public Matcher getMatcher(String path) + { + return this.pattern.matcher(path); + } + + @Override + public String getPathInfo(String path) + { + // Path Info only valid for PREFIX_GLOB types + if (group == PathSpecGroup.PREFIX_GLOB) + { + Matcher matcher = getMatcher(path); + if (matcher.matches()) + { + if (matcher.groupCount() >= 1) + { + String pathInfo = matcher.group(1); + if ("".equals(pathInfo)) + { + return "/"; + } + else + { + return pathInfo; + } + } + } + } + return null; + } + + @Override + public String getPathMatch(String path) + { + Matcher matcher = getMatcher(path); + if (matcher.matches()) + { + if (matcher.groupCount() >= 1) + { + int idx = matcher.start(1); + if (idx > 0) + { + if (path.charAt(idx - 1) == '/') + { + idx--; + } + return path.substring(0,idx); + } + } + return path; + } + return null; + } + + public Pattern getPattern() + { + return this.pattern; + } + + @Override + public String getRelativePath(String base, String path) + { + // TODO Auto-generated method stub + return null; + } + + @Override + public boolean matches(final String path) + { + int idx = path.indexOf('?'); + if (idx >= 0) + { + // match only non-query part + return getMatcher(path.substring(0,idx)).matches(); + } + else + { + // match entire path + return getMatcher(path).matches(); + } + } +} diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/ServletPathSpec.java b/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/ServletPathSpec.java new file mode 100644 index 00000000000..55399748e29 --- /dev/null +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/ServletPathSpec.java @@ -0,0 +1,261 @@ +// +// ======================================================================== +// Copyright (c) 1995-2015 Mort Bay Consulting Pty. Ltd. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.http.pathmap; + +import org.eclipse.jetty.util.URIUtil; + +public class ServletPathSpec extends PathSpec +{ + public ServletPathSpec(String servletPathSpec) + { + super(); + assertValidServletPathSpec(servletPathSpec); + + // The Root Path Spec + if ((servletPathSpec == null) || (servletPathSpec.length() == 0)) + { + super.pathSpec = ""; + super.pathDepth = -1; // force this to be at the end of the sort order + this.specLength = 1; + this.group = PathSpecGroup.ROOT; + return; + } + + // The Default Path Spec + if("/".equals(servletPathSpec)) + { + super.pathSpec = "/"; + super.pathDepth = -1; // force this to be at the end of the sort order + this.specLength = 1; + this.group = PathSpecGroup.DEFAULT; + return; + } + + this.specLength = servletPathSpec.length(); + super.pathDepth = 0; + char lastChar = servletPathSpec.charAt(specLength - 1); + // prefix based + if ((servletPathSpec.charAt(0) == '/') && (specLength > 1) && (lastChar == '*')) + { + this.group = PathSpecGroup.PREFIX_GLOB; + } + // suffix based + else if (servletPathSpec.charAt(0) == '*') + { + this.group = PathSpecGroup.SUFFIX_GLOB; + } + else + { + this.group = PathSpecGroup.EXACT; + } + + for (int i = 0; i < specLength; i++) + { + int cp = servletPathSpec.codePointAt(i); + if (cp < 128) + { + char c = (char)cp; + switch (c) + { + case '/': + super.pathDepth++; + break; + } + } + } + + super.pathSpec = servletPathSpec; + } + + private void assertValidServletPathSpec(String servletPathSpec) + { + if ((servletPathSpec == null) || servletPathSpec.equals("")) + { + return; // empty path spec + } + + int len = servletPathSpec.length(); + // path spec must either start with '/' or '*.' + if (servletPathSpec.charAt(0) == '/') + { + // Prefix Based + if (len == 1) + { + return; // simple '/' path spec + } + int idx = servletPathSpec.indexOf('*'); + if (idx < 0) + { + return; // no hit on glob '*' + } + // only allowed to have '*' at the end of the path spec + if (idx != (len - 1)) + { + throw new IllegalArgumentException("Servlet Spec 12.2 violation: glob '*' can only exist at end of prefix based matches: bad spec \""+ servletPathSpec +"\""); + } + } + else if (servletPathSpec.startsWith("*.")) + { + // Suffix Based + int idx = servletPathSpec.indexOf('/'); + // cannot have path separator + if (idx >= 0) + { + throw new IllegalArgumentException("Servlet Spec 12.2 violation: suffix based path spec cannot have path separators: bad spec \""+ servletPathSpec +"\""); + } + + idx = servletPathSpec.indexOf('*',2); + // only allowed to have 1 glob '*', at the start of the path spec + if (idx >= 1) + { + throw new IllegalArgumentException("Servlet Spec 12.2 violation: suffix based path spec cannot have multiple glob '*': bad spec \""+ servletPathSpec +"\""); + } + } + else + { + throw new IllegalArgumentException("Servlet Spec 12.2 violation: path spec must start with \"/\" or \"*.\": bad spec \""+ servletPathSpec +"\""); + } + } + + @Override + public String getPathInfo(String path) + { + // Path Info only valid for PREFIX_GLOB types + if (group == PathSpecGroup.PREFIX_GLOB) + { + if (path.length() == (specLength - 2)) + { + return null; + } + return path.substring(specLength - 2); + } + + return null; + } + + @Override + public String getPathMatch(String path) + { + switch (group) + { + case EXACT: + if (pathSpec.equals(path)) + { + return path; + } + else + { + return null; + } + case PREFIX_GLOB: + if (isWildcardMatch(path)) + { + return path.substring(0,specLength - 2); + } + else + { + return null; + } + case SUFFIX_GLOB: + if (path.regionMatches(path.length() - (specLength - 1),pathSpec,1,specLength - 1)) + { + return path; + } + else + { + return null; + } + case DEFAULT: + return path; + default: + return null; + } + } + + @Override + public String getRelativePath(String base, String path) + { + String info = getPathInfo(path); + if (info == null) + { + info = path; + } + + if (info.startsWith("./")) + { + info = info.substring(2); + } + if (base.endsWith(URIUtil.SLASH)) + { + if (info.startsWith(URIUtil.SLASH)) + { + path = base + info.substring(1); + } + else + { + path = base + info; + } + } + else if (info.startsWith(URIUtil.SLASH)) + { + path = base + info; + } + else + { + path = base + URIUtil.SLASH + info; + } + return path; + } + + private boolean isWildcardMatch(String path) + { + // For a spec of "/foo/*" match "/foo" , "/foo/..." but not "/foobar" + int cpl = specLength - 2; + if ((group == PathSpecGroup.PREFIX_GLOB) && (path.regionMatches(0,pathSpec,0,cpl))) + { + if ((path.length() == cpl) || ('/' == path.charAt(cpl))) + { + return true; + } + } + return false; + } + + @Override + public boolean matches(String path) + { + switch (group) + { + case EXACT: + return pathSpec.equals(path); + case PREFIX_GLOB: + return (!"/".equals(path) && isWildcardMatch(path)); + case SUFFIX_GLOB: + return path.regionMatches((path.length() - specLength) + 1,pathSpec,1,specLength - 1); + case ROOT: + // Only "/" matches + return ("/".equals(path)); + case DEFAULT: + // If we reached this point, then everything matches + return true; + default: + return false; + } + } +} diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/UriTemplatePathSpec.java b/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/UriTemplatePathSpec.java new file mode 100644 index 00000000000..fe25230b771 --- /dev/null +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/UriTemplatePathSpec.java @@ -0,0 +1,341 @@ +// +// ======================================================================== +// Copyright (c) 1995-2015 Mort Bay Consulting Pty. Ltd. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.http.pathmap; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +import org.eclipse.jetty.util.TypeUtil; +import org.eclipse.jetty.util.log.Log; +import org.eclipse.jetty.util.log.Logger; + +/** + * PathSpec for URI Template based declarations + * + * @see URI Templates (Level 1) + */ +public class UriTemplatePathSpec extends RegexPathSpec +{ + private static final Logger LOG = Log.getLogger(UriTemplatePathSpec.class); + + private static final Pattern VARIABLE_PATTERN = Pattern.compile("\\{(.*)\\}"); + /** Reserved Symbols in URI Template variable */ + private static final String VARIABLE_RESERVED = ":/?#[]@" + // gen-delims + "!$&'()*+,;="; // sub-delims + /** Allowed Symbols in a URI Template variable */ + private static final String VARIABLE_SYMBOLS="-._"; + private static final Set FORBIDDEN_SEGMENTS; + + static + { + FORBIDDEN_SEGMENTS = new HashSet<>(); + FORBIDDEN_SEGMENTS.add("/./"); + FORBIDDEN_SEGMENTS.add("/../"); + FORBIDDEN_SEGMENTS.add("//"); + } + + private String variables[]; + + public UriTemplatePathSpec(String rawSpec) + { + super(); + Objects.requireNonNull(rawSpec,"Path Param Spec cannot be null"); + + if ("".equals(rawSpec) || "/".equals(rawSpec)) + { + super.pathSpec = "/"; + super.pattern = Pattern.compile("^/$"); + super.pathDepth = 1; + this.specLength = 1; + this.variables = new String[0]; + this.group = PathSpecGroup.EXACT; + return; + } + + if (rawSpec.charAt(0) != '/') + { + // path specs must start with '/' + StringBuilder err = new StringBuilder(); + err.append("Syntax Error: path spec \""); + err.append(rawSpec); + err.append("\" must start with '/'"); + throw new IllegalArgumentException(err.toString()); + } + + for (String forbidden : FORBIDDEN_SEGMENTS) + { + if (rawSpec.contains(forbidden)) + { + StringBuilder err = new StringBuilder(); + err.append("Syntax Error: segment "); + err.append(forbidden); + err.append(" is forbidden in path spec: "); + err.append(rawSpec); + throw new IllegalArgumentException(err.toString()); + } + } + + this.pathSpec = rawSpec; + + StringBuilder regex = new StringBuilder(); + regex.append('^'); + + List varNames = new ArrayList<>(); + // split up into path segments (ignoring the first slash that will always be empty) + String segments[] = rawSpec.substring(1).split("/"); + char segmentSignature[] = new char[segments.length]; + this.pathDepth = segments.length; + for (int i = 0; i < segments.length; i++) + { + String segment = segments[i]; + Matcher mat = VARIABLE_PATTERN.matcher(segment); + + if (mat.matches()) + { + // entire path segment is a variable. + String variable = mat.group(1); + if (varNames.contains(variable)) + { + // duplicate variable names + StringBuilder err = new StringBuilder(); + err.append("Syntax Error: variable "); + err.append(variable); + err.append(" is duplicated in path spec: "); + err.append(rawSpec); + throw new IllegalArgumentException(err.toString()); + } + + assertIsValidVariableLiteral(variable); + + segmentSignature[i] = 'v'; // variable + // valid variable name + varNames.add(variable); + // build regex + regex.append("/([^/]+)"); + } + else if (mat.find(0)) + { + // variable exists as partial segment + StringBuilder err = new StringBuilder(); + err.append("Syntax Error: variable "); + err.append(mat.group()); + err.append(" must exist as entire path segment: "); + err.append(rawSpec); + throw new IllegalArgumentException(err.toString()); + } + else if ((segment.indexOf('{') >= 0) || (segment.indexOf('}') >= 0)) + { + // variable is split with a path separator + StringBuilder err = new StringBuilder(); + err.append("Syntax Error: invalid path segment /"); + err.append(segment); + err.append("/ variable declaration incomplete: "); + err.append(rawSpec); + throw new IllegalArgumentException(err.toString()); + } + else if (segment.indexOf('*') >= 0) + { + // glob segment + StringBuilder err = new StringBuilder(); + err.append("Syntax Error: path segment /"); + err.append(segment); + err.append("/ contains a wildcard symbol (not supported by this uri-template implementation): "); + err.append(rawSpec); + throw new IllegalArgumentException(err.toString()); + } + else + { + // valid path segment + segmentSignature[i] = 'e'; // exact + // build regex + regex.append('/'); + // escape regex special characters + for (char c : segment.toCharArray()) + { + if ((c == '.') || (c == '[') || (c == ']') || (c == '\\')) + { + regex.append('\\'); + } + regex.append(c); + } + } + } + + // Handle trailing slash (which is not picked up during split) + if(rawSpec.charAt(rawSpec.length()-1) == '/') + { + regex.append('/'); + } + + regex.append('$'); + + this.pattern = Pattern.compile(regex.toString()); + + int varcount = varNames.size(); + this.variables = varNames.toArray(new String[varcount]); + + // Convert signature to group + String sig = String.valueOf(segmentSignature); + + if (Pattern.matches("^e*$",sig)) + { + this.group = PathSpecGroup.EXACT; + } + else if (Pattern.matches("^e*v+",sig)) + { + this.group = PathSpecGroup.PREFIX_GLOB; + } + else if (Pattern.matches("^v+e+",sig)) + { + this.group = PathSpecGroup.SUFFIX_GLOB; + } + else + { + this.group = PathSpecGroup.MIDDLE_GLOB; + } + } + + /** + * Validate variable literal name, per RFC6570, Section 2.1 Literals + * @param variable + * @param pathParamSpec + */ + private void assertIsValidVariableLiteral(String variable) + { + int len = variable.length(); + + int i = 0; + int codepoint; + boolean valid = (len > 0); // must not be zero length + + while (valid && i < len) + { + codepoint = variable.codePointAt(i); + i += Character.charCount(codepoint); + + // basic letters, digits, or symbols + if (isValidBasicLiteralCodepoint(codepoint)) + { + continue; + } + + // The ucschar and iprivate pieces + if (Character.isSupplementaryCodePoint(codepoint)) + { + continue; + } + + // pct-encoded + if (codepoint == '%') + { + if (i + 2 > len) + { + // invalid percent encoding, missing extra 2 chars + valid = false; + continue; + } + codepoint = TypeUtil.convertHexDigit(variable.codePointAt(i++)) << 4; + codepoint |= TypeUtil.convertHexDigit(variable.codePointAt(i++)); + + // validate basic literal + if (isValidBasicLiteralCodepoint(codepoint)) + { + continue; + } + } + + valid = false; + } + + if (!valid) + { + // invalid variable name + StringBuilder err = new StringBuilder(); + err.append("Syntax Error: variable {"); + err.append(variable); + err.append("} an invalid variable name: "); + err.append(pathSpec); + throw new IllegalArgumentException(err.toString()); + } + } + + private boolean isValidBasicLiteralCodepoint(int codepoint) + { + // basic letters or digits + if((codepoint >= 'a' && codepoint <= 'z') || + (codepoint >= 'A' && codepoint <= 'Z') || + (codepoint >= '0' && codepoint <= '9')) + { + return true; + } + + // basic allowed symbols + if(VARIABLE_SYMBOLS.indexOf(codepoint) >= 0) + { + return true; // valid simple value + } + + // basic reserved symbols + if(VARIABLE_RESERVED.indexOf(codepoint) >= 0) + { + LOG.warn("Detected URI Template reserved symbol [{}] in path spec \"{}\"",(char)codepoint,pathSpec); + return false; // valid simple value + } + + return false; + } + + public Map getPathParams(String path) + { + Matcher matcher = getMatcher(path); + if (matcher.matches()) + { + if (group == PathSpecGroup.EXACT) + { + return Collections.emptyMap(); + } + Map ret = new HashMap<>(); + int groupCount = matcher.groupCount(); + for (int i = 1; i <= groupCount; i++) + { + ret.put(this.variables[i - 1],matcher.group(i)); + } + return ret; + } + return null; + } + + public int getVariableCount() + { + return variables.length; + } + + public String[] getVariables() + { + return this.variables; + } +} diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/PathMappingsTest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/PathMappingsTest.java new file mode 100644 index 00000000000..06cd14bcce6 --- /dev/null +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/PathMappingsTest.java @@ -0,0 +1,320 @@ +// +// ======================================================================== +// Copyright (c) 1995-2015 Mort Bay Consulting Pty. Ltd. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.http.pathmap; + +import static org.hamcrest.Matchers.notNullValue; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +import org.junit.Assert; +import org.junit.Test; + +public class PathMappingsTest +{ + private void assertMatch(PathMappings pathmap, String path, String expectedValue) + { + String msg = String.format(".getMatch(\"%s\")",path); + MappedResource match = pathmap.getMatch(path); + Assert.assertThat(msg,match,notNullValue()); + String actualMatch = match.getResource(); + Assert.assertEquals(msg,expectedValue,actualMatch); + } + + public void dumpMappings(PathMappings p) + { + for (MappedResource res : p) + { + System.out.printf(" %s%n",res); + } + } + + /** + * Test the match order rules with a mixed Servlet and regex path specs + *

+ *

    + *
  • Exact match
    • + *
  • Longest prefix match
    • + *
  • Longest suffix match
    • + *
+ */ + @Test + public void testMixedMatchOrder() + { + PathMappings p = new PathMappings<>(); + + p.put(new ServletPathSpec("/"),"default"); + p.put(new ServletPathSpec("/animal/bird/*"),"birds"); + p.put(new ServletPathSpec("/animal/fish/*"),"fishes"); + p.put(new ServletPathSpec("/animal/*"),"animals"); + p.put(new RegexPathSpec("^/animal/.*/chat$"),"animalChat"); + p.put(new RegexPathSpec("^/animal/.*/cam$"),"animalCam"); + p.put(new RegexPathSpec("^/entrance/cam$"),"entranceCam"); + + // dumpMappings(p); + + assertMatch(p,"/animal/bird/eagle","birds"); + assertMatch(p,"/animal/fish/bass/sea","fishes"); + assertMatch(p,"/animal/peccary/javalina/evolution","animals"); + assertMatch(p,"/","default"); + assertMatch(p,"/animal/bird/eagle/chat","animalChat"); + assertMatch(p,"/animal/bird/penguin/chat","animalChat"); + assertMatch(p,"/animal/fish/trout/cam","animalCam"); + assertMatch(p,"/entrance/cam","entranceCam"); + } + + /** + * Test the match order rules imposed by the Servlet API. + *

+ *

    + *
  • Exact match
    • + *
  • Longest prefix match
    • + *
  • Longest suffix match
    • + *
  • default
    • + *
+ */ + @Test + public void testServletMatchOrder() + { + PathMappings p = new PathMappings<>(); + + p.put(new ServletPathSpec("/abs/path"),"abspath"); // 1 + p.put(new ServletPathSpec("/abs/path/longer"),"longpath"); // 2 + p.put(new ServletPathSpec("/animal/bird/*"),"birds"); // 3 + p.put(new ServletPathSpec("/animal/fish/*"),"fishes"); // 4 + p.put(new ServletPathSpec("/animal/*"),"animals"); // 5 + p.put(new ServletPathSpec("*.tar.gz"),"tarball"); // 6 + p.put(new ServletPathSpec("*.gz"),"gzipped"); // 7 + p.put(new ServletPathSpec("/"),"default"); // 8 + // 9 was the old Jetty ":" spec delimited case (no longer valid) + p.put(new ServletPathSpec(""),"root"); // 10 + p.put(new ServletPathSpec("/\u20ACuro/*"),"money"); // 11 + + // dumpMappings(p); + + // From old PathMapTest + assertMatch(p,"/abs/path","abspath"); + assertMatch(p,"/abs/path/xxx","default"); + assertMatch(p,"/abs/pith","default"); + assertMatch(p,"/abs/path/longer","longpath"); + assertMatch(p,"/abs/path/","default"); + assertMatch(p,"/abs/path/foo","default"); + assertMatch(p,"/animal/bird/eagle/bald","birds"); + assertMatch(p,"/animal/fish/shark/hammerhead","fishes"); + assertMatch(p,"/animal/insect/ladybug","animals"); + assertMatch(p,"/animal","animals"); + assertMatch(p,"/animal/","animals"); + assertMatch(p,"/animal/other","animals"); + assertMatch(p,"/animal/*","animals"); + assertMatch(p,"/downloads/distribution.tar.gz","tarball"); + assertMatch(p,"/downloads/script.gz","gzipped"); + assertMatch(p,"/animal/arhive.gz","animals"); + assertMatch(p,"/Other/path","default"); + assertMatch(p,"/\u20ACuro/path","money"); + assertMatch(p,"/","root"); + + // Extra tests + assertMatch(p,"/downloads/readme.txt","default"); + assertMatch(p,"/downloads/logs.tgz","default"); + assertMatch(p,"/main.css","default"); + } + + /** + * Test the match order rules with a mixed Servlet and URI Template path specs + *

+ *

    + *
  • Exact match
    • + *
  • Longest prefix match
    • + *
  • Longest suffix match
    • + *
+ */ + @Test + public void testMixedMatchUriOrder() + { + PathMappings p = new PathMappings<>(); + + p.put(new ServletPathSpec("/"),"default"); + p.put(new ServletPathSpec("/animal/bird/*"),"birds"); + p.put(new ServletPathSpec("/animal/fish/*"),"fishes"); + p.put(new ServletPathSpec("/animal/*"),"animals"); + p.put(new UriTemplatePathSpec("/animal/{type}/{name}/chat"),"animalChat"); + p.put(new UriTemplatePathSpec("/animal/{type}/{name}/cam"),"animalCam"); + p.put(new UriTemplatePathSpec("/entrance/cam"),"entranceCam"); + + // dumpMappings(p); + + assertMatch(p,"/animal/bird/eagle","birds"); + assertMatch(p,"/animal/fish/bass/sea","fishes"); + assertMatch(p,"/animal/peccary/javalina/evolution","animals"); + assertMatch(p,"/","default"); + assertMatch(p,"/animal/bird/eagle/chat","animalChat"); + assertMatch(p,"/animal/bird/penguin/chat","animalChat"); + assertMatch(p,"/animal/fish/trout/cam","animalCam"); + assertMatch(p,"/entrance/cam","entranceCam"); + } + + /** + * Test the match order rules for URI Template based specs + *

+ *

    + *
  • Exact match
    • + *
  • Longest prefix match
    • + *
  • Longest suffix match
    • + *
+ */ + @Test + public void testUriTemplateMatchOrder() + { + PathMappings p = new PathMappings<>(); + + p.put(new UriTemplatePathSpec("/a/{var}/c"),"endpointA"); + p.put(new UriTemplatePathSpec("/a/b/c"),"endpointB"); + p.put(new UriTemplatePathSpec("/a/{var1}/{var2}"),"endpointC"); + p.put(new UriTemplatePathSpec("/{var1}/d"),"endpointD"); + p.put(new UriTemplatePathSpec("/b/{var2}"),"endpointE"); + + // dumpMappings(p); + + assertMatch(p,"/a/b/c","endpointB"); + assertMatch(p,"/a/d/c","endpointA"); + assertMatch(p,"/a/x/y","endpointC"); + + assertMatch(p,"/b/d","endpointE"); + } + + @Test + public void testPathMap() throws Exception + { + PathMappings p = new PathMappings<>(); + + p.put(new ServletPathSpec("/abs/path"), "1"); + p.put(new ServletPathSpec("/abs/path/longer"), "2"); + p.put(new ServletPathSpec("/animal/bird/*"), "3"); + p.put(new ServletPathSpec("/animal/fish/*"), "4"); + p.put(new ServletPathSpec("/animal/*"), "5"); + p.put(new ServletPathSpec("*.tar.gz"), "6"); + p.put(new ServletPathSpec("*.gz"), "7"); + p.put(new ServletPathSpec("/"), "8"); + // p.put(new ServletPathSpec("/XXX:/YYY"), "9"); // special syntax from Jetty 3.1.x + p.put(new ServletPathSpec(""), "10"); + p.put(new ServletPathSpec("/\u20ACuro/*"), "11"); + + assertEquals("pathMatch exact", "/Foo/bar", new ServletPathSpec("/Foo/bar").getPathMatch("/Foo/bar")); + assertEquals("pathMatch prefix", "/Foo", new ServletPathSpec("/Foo/*").getPathMatch("/Foo/bar")); + assertEquals("pathMatch prefix", "/Foo", new ServletPathSpec("/Foo/*").getPathMatch("/Foo/")); + assertEquals("pathMatch prefix", "/Foo", new ServletPathSpec("/Foo/*").getPathMatch("/Foo")); + assertEquals("pathMatch suffix", "/Foo/bar.ext", new ServletPathSpec("*.ext").getPathMatch("/Foo/bar.ext")); + assertEquals("pathMatch default", "/Foo/bar.ext", new ServletPathSpec("/").getPathMatch("/Foo/bar.ext")); + + assertEquals("pathInfo exact", null, new ServletPathSpec("/Foo/bar").getPathInfo("/Foo/bar")); + assertEquals("pathInfo prefix", "/bar", new ServletPathSpec("/Foo/*").getPathInfo("/Foo/bar")); + assertEquals("pathInfo prefix", "/*", new ServletPathSpec("/Foo/*").getPathInfo("/Foo/*")); + assertEquals("pathInfo prefix", "/", new ServletPathSpec("/Foo/*").getPathInfo("/Foo/")); + assertEquals("pathInfo prefix", null, new ServletPathSpec("/Foo/*").getPathInfo("/Foo")); + assertEquals("pathInfo suffix", null, new ServletPathSpec("*.ext").getPathInfo("/Foo/bar.ext")); + assertEquals("pathInfo default", null, new ServletPathSpec("/").getPathInfo("/Foo/bar.ext")); + + p.put(new ServletPathSpec("/*"), "0"); + + // assertEquals("Get absolute path", "1", p.get("/abs/path")); + assertEquals("Match absolute path", "/abs/path", p.getMatch("/abs/path").getPathSpec().pathSpec); + assertEquals("Match absolute path", "1", p.getMatch("/abs/path").getResource()); + assertEquals("Mismatch absolute path", "0", p.getMatch("/abs/path/xxx").getResource()); + assertEquals("Mismatch absolute path", "0", p.getMatch("/abs/pith").getResource()); + assertEquals("Match longer absolute path", "2", p.getMatch("/abs/path/longer").getResource()); + assertEquals("Not exact absolute path", "0", p.getMatch("/abs/path/").getResource()); + assertEquals("Not exact absolute path", "0", p.getMatch("/abs/path/xxx").getResource()); + + assertEquals("Match longest prefix", "3", p.getMatch("/animal/bird/eagle/bald").getResource()); + assertEquals("Match longest prefix", "4", p.getMatch("/animal/fish/shark/grey").getResource()); + assertEquals("Match longest prefix", "5", p.getMatch("/animal/insect/bug").getResource()); + assertEquals("mismatch exact prefix", "5", p.getMatch("/animal").getResource()); + assertEquals("mismatch exact prefix", "5", p.getMatch("/animal/").getResource()); + + assertEquals("Match longest suffix", "0", p.getMatch("/suffix/path.tar.gz").getResource()); + assertEquals("Match longest suffix", "0", p.getMatch("/suffix/path.gz").getResource()); + assertEquals("prefix rather than suffix", "5", p.getMatch("/animal/path.gz").getResource()); + + assertEquals("default", "0", p.getMatch("/Other/path").getResource()); + + assertEquals("pathMatch /*", "", new ServletPathSpec("/*").getPathMatch("/xxx/zzz")); + assertEquals("pathInfo /*", "/xxx/zzz", new ServletPathSpec("/*").getPathInfo("/xxx/zzz")); + + assertTrue("match /", new ServletPathSpec("/").matches("/anything")); + assertTrue("match /*", new ServletPathSpec("/*").matches("/anything")); + assertTrue("match /foo", new ServletPathSpec("/foo").matches("/foo")); + assertTrue("!match /foo", !new ServletPathSpec("/foo").matches("/bar")); + assertTrue("match /foo/*", new ServletPathSpec("/foo/*").matches("/foo")); + assertTrue("match /foo/*", new ServletPathSpec("/foo/*").matches("/foo/")); + assertTrue("match /foo/*", new ServletPathSpec("/foo/*").matches("/foo/anything")); + assertTrue("!match /foo/*", !new ServletPathSpec("/foo/*").matches("/bar")); + assertTrue("!match /foo/*", !new ServletPathSpec("/foo/*").matches("/bar/")); + assertTrue("!match /foo/*", !new ServletPathSpec("/foo/*").matches("/bar/anything")); + assertTrue("match *.foo", new ServletPathSpec("*.foo").matches("anything.foo")); + assertTrue("!match *.foo", !new ServletPathSpec("*.foo").matches("anything.bar")); + + assertEquals("match / with ''", "10", p.getMatch("/").getResource()); + + assertTrue("match \"\"", new ServletPathSpec("").matches("/")); + } + + /** + * See JIRA issue: JETTY-88. + * @throws Exception failed test + */ + @Test + public void testPathMappingsOnlyMatchOnDirectoryNames() throws Exception + { + ServletPathSpec spec = new ServletPathSpec("/xyz/*"); + + PathSpecAssert.assertMatch(spec, "/xyz"); + PathSpecAssert.assertMatch(spec, "/xyz/"); + PathSpecAssert.assertMatch(spec, "/xyz/123"); + PathSpecAssert.assertMatch(spec, "/xyz/123/"); + PathSpecAssert.assertMatch(spec, "/xyz/123.txt"); + PathSpecAssert.assertNotMatch(spec, "/xyz123"); + PathSpecAssert.assertNotMatch(spec, "/xyz123;jessionid=99"); + PathSpecAssert.assertNotMatch(spec, "/xyz123/"); + PathSpecAssert.assertNotMatch(spec, "/xyz123/456"); + PathSpecAssert.assertNotMatch(spec, "/xyz.123"); + PathSpecAssert.assertNotMatch(spec, "/xyz;123"); // as if the ; was encoded and part of the path + PathSpecAssert.assertNotMatch(spec, "/xyz?123"); // as if the ? was encoded and part of the path + } + + @Test + public void testPrecidenceVsOrdering() throws Exception + { + PathMappings p = new PathMappings<>(); + p.put(new ServletPathSpec("/dump/gzip/*"),"prefix"); + p.put(new ServletPathSpec("*.txt"),"suffix"); + + assertEquals(null,p.getMatch("/foo/bar")); + assertEquals("prefix",p.getMatch("/dump/gzip/something").getResource()); + assertEquals("suffix",p.getMatch("/foo/something.txt").getResource()); + assertEquals("prefix",p.getMatch("/dump/gzip/something.txt").getResource()); + + p = new PathMappings<>(); + p.put(new ServletPathSpec("*.txt"),"suffix"); + p.put(new ServletPathSpec("/dump/gzip/*"),"prefix"); + + assertEquals(null,p.getMatch("/foo/bar")); + assertEquals("prefix",p.getMatch("/dump/gzip/something").getResource()); + assertEquals("suffix",p.getMatch("/foo/something.txt").getResource()); + assertEquals("prefix",p.getMatch("/dump/gzip/something.txt").getResource()); + } +} diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/PathSpecAssert.java b/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/PathSpecAssert.java new file mode 100644 index 00000000000..57546e74bbf --- /dev/null +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/PathSpecAssert.java @@ -0,0 +1,37 @@ +// +// ======================================================================== +// Copyright (c) 1995-2015 Mort Bay Consulting Pty. Ltd. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.http.pathmap; + +import static org.hamcrest.Matchers.is; +import static org.junit.Assert.assertThat; + +public class PathSpecAssert +{ + public static void assertMatch(PathSpec spec, String path) + { + boolean match = spec.matches(path); + assertThat(spec.getClass().getSimpleName() + " '" + spec + "' should match path '" + path + "'", match, is(true)); + } + + public static void assertNotMatch(PathSpec spec, String path) + { + boolean match = spec.matches(path); + assertThat(spec.getClass().getSimpleName() + " '" + spec + "' should not match path '" + path + "'", match, is(false)); + } +} diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/RegexPathSpecTest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/RegexPathSpecTest.java new file mode 100644 index 00000000000..a44c4dca504 --- /dev/null +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/RegexPathSpecTest.java @@ -0,0 +1,135 @@ +// +// ======================================================================== +// Copyright (c) 1995-2015 Mort Bay Consulting Pty. Ltd. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.http.pathmap; + +import static org.hamcrest.Matchers.is; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThat; + +import org.junit.Test; + +public class RegexPathSpecTest +{ + public static void assertMatches(PathSpec spec, String path) + { + String msg = String.format("Spec(\"%s\").matches(\"%s\")",spec.getPathSpec(),path); + assertThat(msg,spec.matches(path),is(true)); + } + + public static void assertNotMatches(PathSpec spec, String path) + { + String msg = String.format("!Spec(\"%s\").matches(\"%s\")",spec.getPathSpec(),path); + assertThat(msg,spec.matches(path),is(false)); + } + + @Test + public void testExactSpec() + { + RegexPathSpec spec = new RegexPathSpec("^/a$"); + assertEquals("Spec.pathSpec","^/a$",spec.getPathSpec()); + assertEquals("Spec.pattern","^/a$",spec.getPattern().pattern()); + assertEquals("Spec.pathDepth",1,spec.getPathDepth()); + assertEquals("Spec.group",PathSpecGroup.EXACT,spec.group); + + assertMatches(spec,"/a"); + + assertNotMatches(spec,"/aa"); + assertNotMatches(spec,"/a/"); + } + + @Test + public void testMiddleSpec() + { + RegexPathSpec spec = new RegexPathSpec("^/rest/([^/]*)/list$"); + assertEquals("Spec.pathSpec","^/rest/([^/]*)/list$",spec.getPathSpec()); + assertEquals("Spec.pattern","^/rest/([^/]*)/list$",spec.getPattern().pattern()); + assertEquals("Spec.pathDepth",3,spec.getPathDepth()); + assertEquals("Spec.group",PathSpecGroup.MIDDLE_GLOB,spec.group); + + assertMatches(spec,"/rest/api/list"); + assertMatches(spec,"/rest/1.0/list"); + assertMatches(spec,"/rest/2.0/list"); + assertMatches(spec,"/rest/accounts/list"); + + assertNotMatches(spec,"/a"); + assertNotMatches(spec,"/aa"); + assertNotMatches(spec,"/aa/bb"); + assertNotMatches(spec,"/rest/admin/delete"); + assertNotMatches(spec,"/rest/list"); + } + + @Test + public void testMiddleSpecNoGrouping() + { + RegexPathSpec spec = new RegexPathSpec("^/rest/[^/]+/list$"); + assertEquals("Spec.pathSpec","^/rest/[^/]+/list$",spec.getPathSpec()); + assertEquals("Spec.pattern","^/rest/[^/]+/list$",spec.getPattern().pattern()); + assertEquals("Spec.pathDepth",3,spec.getPathDepth()); + assertEquals("Spec.group",PathSpecGroup.MIDDLE_GLOB,spec.group); + + assertMatches(spec,"/rest/api/list"); + assertMatches(spec,"/rest/1.0/list"); + assertMatches(spec,"/rest/2.0/list"); + assertMatches(spec,"/rest/accounts/list"); + + assertNotMatches(spec,"/a"); + assertNotMatches(spec,"/aa"); + assertNotMatches(spec,"/aa/bb"); + assertNotMatches(spec,"/rest/admin/delete"); + assertNotMatches(spec,"/rest/list"); + } + + @Test + public void testPrefixSpec() + { + RegexPathSpec spec = new RegexPathSpec("^/a/(.*)$"); + assertEquals("Spec.pathSpec","^/a/(.*)$",spec.getPathSpec()); + assertEquals("Spec.pattern","^/a/(.*)$",spec.getPattern().pattern()); + assertEquals("Spec.pathDepth",2,spec.getPathDepth()); + assertEquals("Spec.group",PathSpecGroup.PREFIX_GLOB,spec.group); + + assertMatches(spec,"/a/"); + assertMatches(spec,"/a/b"); + assertMatches(spec,"/a/b/c/d/e"); + + assertNotMatches(spec,"/a"); + assertNotMatches(spec,"/aa"); + assertNotMatches(spec,"/aa/bb"); + } + + @Test + public void testSuffixSpec() + { + RegexPathSpec spec = new RegexPathSpec("^(.*).do$"); + assertEquals("Spec.pathSpec","^(.*).do$",spec.getPathSpec()); + assertEquals("Spec.pattern","^(.*).do$",spec.getPattern().pattern()); + assertEquals("Spec.pathDepth",0,spec.getPathDepth()); + assertEquals("Spec.group",PathSpecGroup.SUFFIX_GLOB,spec.group); + + assertMatches(spec,"/a.do"); + assertMatches(spec,"/a/b/c.do"); + assertMatches(spec,"/abcde.do"); + assertMatches(spec,"/abc/efg.do"); + + assertNotMatches(spec,"/a"); + assertNotMatches(spec,"/aa"); + assertNotMatches(spec,"/aa/bb"); + assertNotMatches(spec,"/aa/bb.do/more"); + } +} diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/ServletPathSpecMatchListTest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/ServletPathSpecMatchListTest.java new file mode 100644 index 00000000000..b1295a7d9c9 --- /dev/null +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/ServletPathSpecMatchListTest.java @@ -0,0 +1,101 @@ +// +// ======================================================================== +// Copyright (c) 1995-2015 Mort Bay Consulting Pty. Ltd. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.http.pathmap; + +import static org.hamcrest.Matchers.is; +import static org.junit.Assert.assertThat; + +import java.util.Arrays; +import java.util.List; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameter; +import org.junit.runners.Parameterized.Parameters; + +/** + * Tests of {@link PathMappings#getMatches(String)} + */ +@RunWith(Parameterized.class) +public class ServletPathSpecMatchListTest +{ + @Parameters(name="{0} = {1}") + public static List testCases() + { + String data[][] = new String[][] { + // From old PathMapTest + { "All matches", "/animal/bird/path.tar.gz", "[/animal/bird/*=birds, /animal/*=animals, *.tar.gz=tarball, *.gz=gzipped, /=default]"}, + { "Dir matches", "/animal/fish/", "[/animal/fish/*=fishes, /animal/*=animals, /=default]"}, + { "Dir matches", "/animal/fish", "[/animal/fish/*=fishes, /animal/*=animals, /=default]"}, + { "Root matches", "/", "[=root, /=default]"}, + { "Dir matches", "", "[/=default]"} + }; + + return Arrays.asList(data); + } + + private static PathMappings mappings; + + static { + mappings = new PathMappings<>(); + + // From old PathMapTest + mappings.put(new ServletPathSpec("/abs/path"),"abspath"); // 1 + mappings.put(new ServletPathSpec("/abs/path/longer"),"longpath"); // 2 + mappings.put(new ServletPathSpec("/animal/bird/*"),"birds"); // 3 + mappings.put(new ServletPathSpec("/animal/fish/*"),"fishes"); // 4 + mappings.put(new ServletPathSpec("/animal/*"),"animals"); // 5 + mappings.put(new ServletPathSpec("*.tar.gz"),"tarball"); // 6 + mappings.put(new ServletPathSpec("*.gz"),"gzipped"); // 7 + mappings.put(new ServletPathSpec("/"),"default"); // 8 + // 9 was the old Jetty ":" spec delimited case (no longer valid) + mappings.put(new ServletPathSpec(""),"root"); // 10 + mappings.put(new ServletPathSpec("/\u20ACuro/*"),"money"); // 11 + } + + @Parameter(0) + public String message; + + @Parameter(1) + public String inputPath; + + @Parameter(2) + public String expectedListing; + + @Test + public void testGetMatches() + { + List> matches = mappings.getMatches(inputPath); + + StringBuilder actual = new StringBuilder(); + actual.append('['); + boolean delim = false; + for (MappedResource res : matches) + { + if (delim) + actual.append(", "); + actual.append(res.getPathSpec().pathSpec).append('=').append(res.getResource()); + delim = true; + } + actual.append(']'); + + assertThat(message + " on [" + inputPath + "]",actual.toString(),is(expectedListing)); + } +} diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/ServletPathSpecOrderTest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/ServletPathSpecOrderTest.java new file mode 100644 index 00000000000..ea5dddb3f8a --- /dev/null +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/ServletPathSpecOrderTest.java @@ -0,0 +1,103 @@ +// +// ======================================================================== +// Copyright (c) 1995-2015 Mort Bay Consulting Pty. Ltd. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.http.pathmap; + +import static org.hamcrest.Matchers.is; +import static org.junit.Assert.assertThat; + +import java.util.Arrays; +import java.util.List; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameter; +import org.junit.runners.Parameterized.Parameters; + +/** + * Tests of {@link PathMappings#getMatch(String)}, with a focus on correct mapping selection order + */ +@RunWith(Parameterized.class) +public class ServletPathSpecOrderTest +{ + @Parameters(name="{0} = {1}") + public static List testCases() + { + String data[][] = new String[][] { + // From old PathMapTest + {"/abs/path","abspath"}, + {"/abs/path/xxx","default"}, + {"/abs/pith","default"}, + {"/abs/path/longer","longpath"}, + {"/abs/path/","default"}, + {"/abs/path/foo","default"}, + {"/animal/bird/eagle/bald","birds"}, + {"/animal/fish/shark/hammerhead","fishes"}, + {"/animal/insect/ladybug","animals"}, + {"/animal","animals"}, + {"/animal/","animals"}, + {"/animal/other","animals"}, + {"/animal/*","animals"}, + {"/downloads/distribution.tar.gz","tarball"}, + {"/downloads/script.gz","gzipped"}, + {"/animal/arhive.gz","animals"}, + {"/Other/path","default"}, + {"/\u20ACuro/path","money"}, + {"/","root"}, + + // Extra tests + {"/downloads/readme.txt","default"}, + {"/downloads/logs.tgz","default"}, + {"/main.css","default"} + }; + + return Arrays.asList(data); + } + + private static PathMappings mappings; + + static { + mappings = new PathMappings<>(); + + // From old PathMapTest + mappings.put(new ServletPathSpec("/abs/path"),"abspath"); // 1 + mappings.put(new ServletPathSpec("/abs/path/longer"),"longpath"); // 2 + mappings.put(new ServletPathSpec("/animal/bird/*"),"birds"); // 3 + mappings.put(new ServletPathSpec("/animal/fish/*"),"fishes"); // 4 + mappings.put(new ServletPathSpec("/animal/*"),"animals"); // 5 + mappings.put(new ServletPathSpec("*.tar.gz"),"tarball"); // 6 + mappings.put(new ServletPathSpec("*.gz"),"gzipped"); // 7 + mappings.put(new ServletPathSpec("/"),"default"); // 8 + // 9 was the old Jetty ":" spec delimited case (no longer valid) + mappings.put(new ServletPathSpec(""),"root"); // 10 + mappings.put(new ServletPathSpec("/\u20ACuro/*"),"money"); // 11 + } + + @Parameter(0) + public String inputPath; + + @Parameter(1) + public String expectedResource; + + @Test + public void testMatch() + { + assertThat("Match on ["+ inputPath+ "]", mappings.getMatch(inputPath).getResource(), is(expectedResource)); + } +} diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/ServletPathSpecTest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/ServletPathSpecTest.java new file mode 100644 index 00000000000..7252a9c1b48 --- /dev/null +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/ServletPathSpecTest.java @@ -0,0 +1,188 @@ +// +// ======================================================================== +// Copyright (c) 1995-2015 Mort Bay Consulting Pty. Ltd. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.http.pathmap; + +import static org.hamcrest.Matchers.is; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.fail; + +import org.junit.Test; + +public class ServletPathSpecTest +{ + private void assertBadServletPathSpec(String pathSpec) + { + try + { + new ServletPathSpec(pathSpec); + fail("Expected IllegalArgumentException for a bad servlet pathspec on: " + pathSpec); + } + catch (IllegalArgumentException e) + { + // expected path + System.out.println(e); + } + } + + private void assertMatches(ServletPathSpec spec, String path) + { + String msg = String.format("Spec(\"%s\").matches(\"%s\")",spec.getPathSpec(),path); + assertThat(msg,spec.matches(path),is(true)); + } + + private void assertNotMatches(ServletPathSpec spec, String path) + { + String msg = String.format("!Spec(\"%s\").matches(\"%s\")",spec.getPathSpec(),path); + assertThat(msg,spec.matches(path),is(false)); + } + + @Test + public void testBadServletPathSpecA() + { + assertBadServletPathSpec("foo"); + } + + @Test + public void testBadServletPathSpecB() + { + assertBadServletPathSpec("/foo/*.do"); + } + + @Test + public void testBadServletPathSpecC() + { + assertBadServletPathSpec("foo/*.do"); + } + + @Test + public void testBadServletPathSpecD() + { + assertBadServletPathSpec("foo/*.*do"); + } + + @Test + public void testBadServletPathSpecE() + { + assertBadServletPathSpec("*do"); + } + + @Test + public void testDefaultPathSpec() + { + ServletPathSpec spec = new ServletPathSpec("/"); + assertEquals("Spec.pathSpec","/",spec.getPathSpec()); + assertEquals("Spec.pathDepth",-1,spec.getPathDepth()); + } + + @Test + public void testExactPathSpec() + { + ServletPathSpec spec = new ServletPathSpec("/abs/path"); + assertEquals("Spec.pathSpec","/abs/path",spec.getPathSpec()); + assertEquals("Spec.pathDepth",2,spec.getPathDepth()); + + assertMatches(spec,"/abs/path"); + + assertNotMatches(spec,"/abs/path/"); + assertNotMatches(spec,"/abs/path/more"); + assertNotMatches(spec,"/foo"); + assertNotMatches(spec,"/foo/abs/path"); + assertNotMatches(spec,"/foo/abs/path/"); + } + + @Test + public void testGetPathInfo() + { + assertEquals("pathInfo exact",null,new ServletPathSpec("/Foo/bar").getPathInfo("/Foo/bar")); + assertEquals("pathInfo prefix","/bar",new ServletPathSpec("/Foo/*").getPathInfo("/Foo/bar")); + assertEquals("pathInfo prefix","/*",new ServletPathSpec("/Foo/*").getPathInfo("/Foo/*")); + assertEquals("pathInfo prefix","/",new ServletPathSpec("/Foo/*").getPathInfo("/Foo/")); + assertEquals("pathInfo prefix",null,new ServletPathSpec("/Foo/*").getPathInfo("/Foo")); + assertEquals("pathInfo suffix",null,new ServletPathSpec("*.ext").getPathInfo("/Foo/bar.ext")); + assertEquals("pathInfo default",null,new ServletPathSpec("/").getPathInfo("/Foo/bar.ext")); + + assertEquals("pathInfo default","/xxx/zzz",new ServletPathSpec("/*").getPathInfo("/xxx/zzz")); + } + + @Test + public void testNullPathSpec() + { + ServletPathSpec spec = new ServletPathSpec(null); + assertEquals("Spec.pathSpec","",spec.getPathSpec()); + assertEquals("Spec.pathDepth",-1,spec.getPathDepth()); + } + + @Test + public void testRootPathSpec() + { + ServletPathSpec spec = new ServletPathSpec(""); + assertEquals("Spec.pathSpec","",spec.getPathSpec()); + assertEquals("Spec.pathDepth",-1,spec.getPathDepth()); + } + + @Test + public void testPathMatch() + { + assertEquals("pathMatch exact","/Foo/bar",new ServletPathSpec("/Foo/bar").getPathMatch("/Foo/bar")); + assertEquals("pathMatch prefix","/Foo",new ServletPathSpec("/Foo/*").getPathMatch("/Foo/bar")); + assertEquals("pathMatch prefix","/Foo",new ServletPathSpec("/Foo/*").getPathMatch("/Foo/")); + assertEquals("pathMatch prefix","/Foo",new ServletPathSpec("/Foo/*").getPathMatch("/Foo")); + assertEquals("pathMatch suffix","/Foo/bar.ext",new ServletPathSpec("*.ext").getPathMatch("/Foo/bar.ext")); + assertEquals("pathMatch default","/Foo/bar.ext",new ServletPathSpec("/").getPathMatch("/Foo/bar.ext")); + + assertEquals("pathMatch default","",new ServletPathSpec("/*").getPathMatch("/xxx/zzz")); + } + + @Test + public void testPrefixPathSpec() + { + ServletPathSpec spec = new ServletPathSpec("/downloads/*"); + assertEquals("Spec.pathSpec","/downloads/*",spec.getPathSpec()); + assertEquals("Spec.pathDepth",2,spec.getPathDepth()); + + assertMatches(spec,"/downloads/logo.jpg"); + assertMatches(spec,"/downloads/distribution.tar.gz"); + assertMatches(spec,"/downloads/distribution.tgz"); + assertMatches(spec,"/downloads/distribution.zip"); + + assertMatches(spec,"/downloads"); + + assertEquals("Spec.pathInfo","/",spec.getPathInfo("/downloads/")); + assertEquals("Spec.pathInfo","/distribution.zip",spec.getPathInfo("/downloads/distribution.zip")); + assertEquals("Spec.pathInfo","/dist/9.0/distribution.tar.gz",spec.getPathInfo("/downloads/dist/9.0/distribution.tar.gz")); + } + + @Test + public void testSuffixPathSpec() + { + ServletPathSpec spec = new ServletPathSpec("*.gz"); + assertEquals("Spec.pathSpec","*.gz",spec.getPathSpec()); + assertEquals("Spec.pathDepth",0,spec.getPathDepth()); + + assertMatches(spec,"/downloads/distribution.tar.gz"); + assertMatches(spec,"/downloads/jetty.log.gz"); + + assertNotMatches(spec,"/downloads/distribution.zip"); + assertNotMatches(spec,"/downloads/distribution.tgz"); + assertNotMatches(spec,"/abs/path"); + + assertEquals("Spec.pathInfo",null,spec.getPathInfo("/downloads/distribution.tar.gz")); + } +} diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/UriTemplatePathSpecBadSpecsTest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/UriTemplatePathSpecBadSpecsTest.java new file mode 100644 index 00000000000..4136ba2f299 --- /dev/null +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/UriTemplatePathSpecBadSpecsTest.java @@ -0,0 +1,87 @@ +// +// ======================================================================== +// Copyright (c) 1995-2015 Mort Bay Consulting Pty. Ltd. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.http.pathmap; + +import static org.junit.Assert.fail; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameters; + +/** + * Tests for bad path specs on ServerEndpoint Path Param / URI Template + */ +@RunWith(Parameterized.class) +public class UriTemplatePathSpecBadSpecsTest +{ + private static void bad(List data, String str) + { + data.add(new String[] + { str }); + } + + @Parameters + public static Collection data() + { + List data = new ArrayList<>(); + bad(data,"/a/b{var}"); // bad syntax - variable does not encompass whole path segment + bad(data,"a/{var}"); // bad syntax - no start slash + bad(data,"/a/{var/b}"); // path segment separator in variable name + bad(data,"/{var}/*"); // bad syntax - no globs allowed + bad(data,"/{var}.do"); // bad syntax - variable does not encompass whole path segment + bad(data,"/a/{var*}"); // use of glob character not allowed in variable name + bad(data,"/a/{}"); // bad syntax - no variable name + // MIGHT BE ALLOWED bad(data,"/a/{---}"); // no alpha in variable name + bad(data,"{var}"); // bad syntax - no start slash + bad(data,"/a/{my special variable}"); // bad syntax - space in variable name + bad(data,"/a/{var}/{var}"); // variable name duplicate + // MIGHT BE ALLOWED bad(data,"/a/{var}/{Var}/{vAR}"); // variable name duplicated (diff case) + bad(data,"/a/../../../{var}"); // path navigation not allowed + bad(data,"/a/./{var}"); // path navigation not allowed + bad(data,"/a//{var}"); // bad syntax - double path slash (no path segment) + return data; + } + + private String pathSpec; + + public UriTemplatePathSpecBadSpecsTest(String pathSpec) + { + this.pathSpec = pathSpec; + } + + @Test + public void testBadPathSpec() + { + try + { + new UriTemplatePathSpec(this.pathSpec); + fail("Expected IllegalArgumentException for a bad PathParam pathspec on: " + pathSpec); + } + catch (IllegalArgumentException e) + { + // expected path + System.out.println(e.getMessage()); + } + } +} diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/UriTemplatePathSpecTest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/UriTemplatePathSpecTest.java new file mode 100644 index 00000000000..7908344e09c --- /dev/null +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/UriTemplatePathSpecTest.java @@ -0,0 +1,284 @@ +// +// ======================================================================== +// Copyright (c) 1995-2015 Mort Bay Consulting Pty. Ltd. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.http.pathmap; + +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.notNullValue; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThat; + +import java.util.Map; + +import org.junit.Test; + +/** + * Tests for URI Template Path Specs + */ +public class UriTemplatePathSpecTest +{ + private void assertDetectedVars(UriTemplatePathSpec spec, String... expectedVars) + { + String prefix = String.format("Spec(\"%s\")",spec.getPathSpec()); + assertEquals(prefix + ".variableCount",expectedVars.length,spec.getVariableCount()); + assertEquals(prefix + ".variable.length",expectedVars.length,spec.getVariables().length); + for (int i = 0; i < expectedVars.length; i++) + { + assertEquals(String.format("%s.variable[%d]",prefix,i),expectedVars[i],spec.getVariables()[i]); + } + } + + private void assertMatches(PathSpec spec, String path) + { + String msg = String.format("Spec(\"%s\").matches(\"%s\")",spec.getPathSpec(),path); + assertThat(msg,spec.matches(path),is(true)); + } + + private void assertNotMatches(PathSpec spec, String path) + { + String msg = String.format("!Spec(\"%s\").matches(\"%s\")",spec.getPathSpec(),path); + assertThat(msg,spec.matches(path),is(false)); + } + + @Test + public void testDefaultPathSpec() + { + UriTemplatePathSpec spec = new UriTemplatePathSpec("/"); + assertEquals("Spec.pathSpec","/",spec.getPathSpec()); + assertEquals("Spec.pattern","^/$",spec.getPattern().pattern()); + assertEquals("Spec.pathDepth",1,spec.getPathDepth()); + assertEquals("Spec.group",PathSpecGroup.EXACT,spec.getGroup()); + + assertEquals("Spec.variableCount",0,spec.getVariableCount()); + assertEquals("Spec.variable.length",0,spec.getVariables().length); + } + + @Test + public void testExactOnePathSpec() + { + UriTemplatePathSpec spec = new UriTemplatePathSpec("/a"); + assertEquals("Spec.pathSpec","/a",spec.getPathSpec()); + assertEquals("Spec.pattern","^/a$",spec.getPattern().pattern()); + assertEquals("Spec.pathDepth",1,spec.getPathDepth()); + assertEquals("Spec.group",PathSpecGroup.EXACT,spec.getGroup()); + + assertMatches(spec,"/a"); + assertMatches(spec,"/a?type=other"); + assertNotMatches(spec,"/a/b"); + assertNotMatches(spec,"/a/"); + + assertEquals("Spec.variableCount",0,spec.getVariableCount()); + assertEquals("Spec.variable.length",0,spec.getVariables().length); + } + + @Test + public void testExactPathSpec_TestWebapp() + { + UriTemplatePathSpec spec = new UriTemplatePathSpec("/deep.thought/"); + assertEquals("Spec.pathSpec","/deep.thought/",spec.getPathSpec()); + assertEquals("Spec.pattern","^/deep\\.thought/$",spec.getPattern().pattern()); + assertEquals("Spec.pathDepth",1,spec.getPathDepth()); + assertEquals("Spec.group",PathSpecGroup.EXACT,spec.getGroup()); + + assertMatches(spec,"/deep.thought/"); + assertNotMatches(spec,"/deep.thought"); + + assertEquals("Spec.variableCount",0,spec.getVariableCount()); + assertEquals("Spec.variable.length",0,spec.getVariables().length); + } + + @Test + public void testExactTwoPathSpec() + { + UriTemplatePathSpec spec = new UriTemplatePathSpec("/a/b"); + assertEquals("Spec.pathSpec","/a/b",spec.getPathSpec()); + assertEquals("Spec.pattern","^/a/b$",spec.getPattern().pattern()); + assertEquals("Spec.pathDepth",2,spec.getPathDepth()); + assertEquals("Spec.group",PathSpecGroup.EXACT,spec.getGroup()); + + assertEquals("Spec.variableCount",0,spec.getVariableCount()); + assertEquals("Spec.variable.length",0,spec.getVariables().length); + + assertMatches(spec,"/a/b"); + + assertNotMatches(spec,"/a/b/"); + assertNotMatches(spec,"/a/"); + assertNotMatches(spec,"/a/bb"); + } + + @Test + public void testMiddleVarPathSpec() + { + UriTemplatePathSpec spec = new UriTemplatePathSpec("/a/{var}/c"); + assertEquals("Spec.pathSpec","/a/{var}/c",spec.getPathSpec()); + assertEquals("Spec.pattern","^/a/([^/]+)/c$",spec.getPattern().pattern()); + assertEquals("Spec.pathDepth",3,spec.getPathDepth()); + assertEquals("Spec.group",PathSpecGroup.MIDDLE_GLOB,spec.getGroup()); + + assertDetectedVars(spec,"var"); + + assertMatches(spec,"/a/b/c"); + assertMatches(spec,"/a/zz/c"); + assertMatches(spec,"/a/hello+world/c"); + assertNotMatches(spec,"/a/bc"); + assertNotMatches(spec,"/a/b/"); + assertNotMatches(spec,"/a/b"); + + Map mapped = spec.getPathParams("/a/b/c"); + assertThat("Spec.pathParams",mapped,notNullValue()); + assertThat("Spec.pathParams.size",mapped.size(),is(1)); + assertEquals("Spec.pathParams[var]","b",mapped.get("var")); + } + + @Test + public void testOneVarPathSpec() + { + UriTemplatePathSpec spec = new UriTemplatePathSpec("/a/{foo}"); + assertEquals("Spec.pathSpec","/a/{foo}",spec.getPathSpec()); + assertEquals("Spec.pattern","^/a/([^/]+)$",spec.getPattern().pattern()); + assertEquals("Spec.pathDepth",2,spec.getPathDepth()); + assertEquals("Spec.group",PathSpecGroup.PREFIX_GLOB,spec.getGroup()); + + assertDetectedVars(spec,"foo"); + + assertMatches(spec,"/a/b"); + assertNotMatches(spec,"/a/"); + assertNotMatches(spec,"/a"); + + Map mapped = spec.getPathParams("/a/b"); + assertThat("Spec.pathParams",mapped,notNullValue()); + assertThat("Spec.pathParams.size",mapped.size(),is(1)); + assertEquals("Spec.pathParams[foo]","b",mapped.get("foo")); + } + + @Test + public void testOneVarSuffixPathSpec() + { + UriTemplatePathSpec spec = new UriTemplatePathSpec("/{var}/b/c"); + assertEquals("Spec.pathSpec","/{var}/b/c",spec.getPathSpec()); + assertEquals("Spec.pattern","^/([^/]+)/b/c$",spec.getPattern().pattern()); + assertEquals("Spec.pathDepth",3,spec.getPathDepth()); + assertEquals("Spec.group",PathSpecGroup.SUFFIX_GLOB,spec.getGroup()); + + assertDetectedVars(spec,"var"); + + assertMatches(spec,"/a/b/c"); + assertMatches(spec,"/az/b/c"); + assertMatches(spec,"/hello+world/b/c"); + assertNotMatches(spec,"/a/bc"); + assertNotMatches(spec,"/a/b/"); + assertNotMatches(spec,"/a/b"); + + Map mapped = spec.getPathParams("/a/b/c"); + assertThat("Spec.pathParams",mapped,notNullValue()); + assertThat("Spec.pathParams.size",mapped.size(),is(1)); + assertEquals("Spec.pathParams[var]","a",mapped.get("var")); + } + + @Test + public void testTwoVarComplexInnerPathSpec() + { + UriTemplatePathSpec spec = new UriTemplatePathSpec("/a/{var1}/c/{var2}/e"); + assertEquals("Spec.pathSpec","/a/{var1}/c/{var2}/e",spec.getPathSpec()); + assertEquals("Spec.pattern","^/a/([^/]+)/c/([^/]+)/e$",spec.getPattern().pattern()); + assertEquals("Spec.pathDepth",5,spec.getPathDepth()); + assertEquals("Spec.group",PathSpecGroup.MIDDLE_GLOB,spec.getGroup()); + + assertDetectedVars(spec,"var1","var2"); + + assertMatches(spec,"/a/b/c/d/e"); + assertNotMatches(spec,"/a/bc/d/e"); + assertNotMatches(spec,"/a/b/d/e"); + assertNotMatches(spec,"/a/b//d/e"); + + Map mapped = spec.getPathParams("/a/b/c/d/e"); + assertThat("Spec.pathParams",mapped,notNullValue()); + assertThat("Spec.pathParams.size",mapped.size(),is(2)); + assertEquals("Spec.pathParams[var1]","b",mapped.get("var1")); + assertEquals("Spec.pathParams[var2]","d",mapped.get("var2")); + } + + @Test + public void testTwoVarComplexOuterPathSpec() + { + UriTemplatePathSpec spec = new UriTemplatePathSpec("/{var1}/b/{var2}/{var3}"); + assertEquals("Spec.pathSpec","/{var1}/b/{var2}/{var3}",spec.getPathSpec()); + assertEquals("Spec.pattern","^/([^/]+)/b/([^/]+)/([^/]+)$",spec.getPattern().pattern()); + assertEquals("Spec.pathDepth",4,spec.getPathDepth()); + assertEquals("Spec.group",PathSpecGroup.MIDDLE_GLOB,spec.getGroup()); + + assertDetectedVars(spec,"var1","var2","var3"); + + assertMatches(spec,"/a/b/c/d"); + assertNotMatches(spec,"/a/bc/d/e"); + assertNotMatches(spec,"/a/c/d/e"); + assertNotMatches(spec,"/a//d/e"); + + Map mapped = spec.getPathParams("/a/b/c/d"); + assertThat("Spec.pathParams",mapped,notNullValue()); + assertThat("Spec.pathParams.size",mapped.size(),is(3)); + assertEquals("Spec.pathParams[var1]","a",mapped.get("var1")); + assertEquals("Spec.pathParams[var2]","c",mapped.get("var2")); + assertEquals("Spec.pathParams[var3]","d",mapped.get("var3")); + } + + @Test + public void testTwoVarPrefixPathSpec() + { + UriTemplatePathSpec spec = new UriTemplatePathSpec("/a/{var1}/{var2}"); + assertEquals("Spec.pathSpec","/a/{var1}/{var2}",spec.getPathSpec()); + assertEquals("Spec.pattern","^/a/([^/]+)/([^/]+)$",spec.getPattern().pattern()); + assertEquals("Spec.pathDepth",3,spec.getPathDepth()); + assertEquals("Spec.group",PathSpecGroup.PREFIX_GLOB,spec.getGroup()); + + assertDetectedVars(spec,"var1","var2"); + + assertMatches(spec,"/a/b/c"); + assertNotMatches(spec,"/a/bc"); + assertNotMatches(spec,"/a/b/"); + assertNotMatches(spec,"/a/b"); + + Map mapped = spec.getPathParams("/a/b/c"); + assertThat("Spec.pathParams",mapped,notNullValue()); + assertThat("Spec.pathParams.size",mapped.size(),is(2)); + assertEquals("Spec.pathParams[var1]","b",mapped.get("var1")); + assertEquals("Spec.pathParams[var2]","c",mapped.get("var2")); + } + + @Test + public void testVarOnlyPathSpec() + { + UriTemplatePathSpec spec = new UriTemplatePathSpec("/{var1}"); + assertEquals("Spec.pathSpec","/{var1}",spec.getPathSpec()); + assertEquals("Spec.pattern","^/([^/]+)$",spec.getPattern().pattern()); + assertEquals("Spec.pathDepth",1,spec.getPathDepth()); + assertEquals("Spec.group",PathSpecGroup.PREFIX_GLOB,spec.getGroup()); + + assertDetectedVars(spec,"var1"); + + assertMatches(spec,"/a"); + assertNotMatches(spec,"/"); + assertNotMatches(spec,"/a/b"); + assertNotMatches(spec,"/a/b/c"); + + Map mapped = spec.getPathParams("/a"); + assertThat("Spec.pathParams",mapped,notNullValue()); + assertThat("Spec.pathParams.size",mapped.size(),is(1)); + assertEquals("Spec.pathParams[var1]","a",mapped.get("var1")); + } +} From 1df5a05ee1b50cda1572baf11af34307bac42a39 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Mon, 14 Dec 2015 11:22:24 -0700 Subject: [PATCH 07/10] 482042 - New API, Allow customization of ServletHandler path mapping + Swapping out PathMap for PathMappings in ServletHandler --- .../jetty/http/pathmap/PathMappings.java | 20 ++++++- .../eclipse/jetty/http/pathmap/PathSpec.java | 2 +- .../jetty/http/pathmap/ServletPathSpec.java | 2 +- .../jetty/http/pathmap/PathMappingsTest.java | 57 +++---------------- .../jetty/http/pathmap/RegexPathSpecTest.java | 14 ++--- .../http/pathmap/ServletPathSpecTest.java | 16 +++--- .../http/pathmap/UriTemplatePathSpecTest.java | 28 ++++----- .../eclipse/jetty/servlet/DefaultServlet.java | 7 ++- .../org/eclipse/jetty/servlet/Invoker.java | 8 ++- .../eclipse/jetty/servlet/ServletHandler.java | 24 ++++---- 10 files changed, 80 insertions(+), 98 deletions(-) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/PathMappings.java b/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/PathMappings.java index e0d975ca7df..348a4b4284d 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/PathMappings.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/PathMappings.java @@ -44,6 +44,7 @@ public class PathMappings implements Iterable>, Dumpable private static final Logger LOG = Log.getLogger(PathMappings.class); private List> mappings = new ArrayList>(); private MappedResource defaultResource = null; + private MappedResource rootResource = null; @Override public String dump() @@ -105,6 +106,11 @@ public class PathMappings implements Iterable>, Dumpable public MappedResource getMatch(String path) { + if (path.equals("/") && rootResource != null) + { + return rootResource; + } + int len = mappings.size(); for (int i = 0; i < len; i++) { @@ -123,14 +129,22 @@ public class PathMappings implements Iterable>, Dumpable return mappings.iterator(); } + @SuppressWarnings("incomplete-switch") public void put(PathSpec pathSpec, E resource) { MappedResource entry = new MappedResource<>(pathSpec,resource); - if (pathSpec.group == PathSpecGroup.DEFAULT) + switch (pathSpec.group) { - defaultResource = entry; + case DEFAULT: + defaultResource = entry; + break; + case ROOT: + rootResource = entry; + break; } - // TODO: warning on replacement of existing mapping? + + // TODO: add warning when replacing an existing pathspec? + mappings.add(entry); if (LOG.isDebugEnabled()) LOG.debug("Added {} to {}",entry,this); diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/PathSpec.java b/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/PathSpec.java index 122211a5586..b9878472bbb 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/PathSpec.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/PathSpec.java @@ -119,7 +119,7 @@ public abstract class PathSpec implements Comparable * * @return the as-provided path spec */ - public String getPathSpec() + public String getDeclaration() { return pathSpec; } diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/ServletPathSpec.java b/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/ServletPathSpec.java index 55399748e29..44f8a5fae28 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/ServletPathSpec.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/ServletPathSpec.java @@ -245,7 +245,7 @@ public class ServletPathSpec extends PathSpec case EXACT: return pathSpec.equals(path); case PREFIX_GLOB: - return (!"/".equals(path) && isWildcardMatch(path)); + return isWildcardMatch(path); case SUFFIX_GLOB: return path.regionMatches((path.length() - specLength) + 1,pathSpec,1,specLength - 1); case ROOT: diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/PathMappingsTest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/PathMappingsTest.java index 06cd14bcce6..36df42e553d 100644 --- a/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/PathMappingsTest.java +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/PathMappingsTest.java @@ -77,61 +77,22 @@ public class PathMappingsTest assertMatch(p,"/animal/fish/trout/cam","animalCam"); assertMatch(p,"/entrance/cam","entranceCam"); } - + /** - * Test the match order rules imposed by the Servlet API. - *

- *

    - *
  • Exact match
    • - *
  • Longest prefix match
    • - *
  • Longest suffix match
    • - *
  • default
    • - *
+ * Test the match order rules imposed by the Servlet API (default vs any) */ @Test - public void testServletMatchOrder() + public void testServletMatchDefault() { PathMappings p = new PathMappings<>(); - p.put(new ServletPathSpec("/abs/path"),"abspath"); // 1 - p.put(new ServletPathSpec("/abs/path/longer"),"longpath"); // 2 - p.put(new ServletPathSpec("/animal/bird/*"),"birds"); // 3 - p.put(new ServletPathSpec("/animal/fish/*"),"fishes"); // 4 - p.put(new ServletPathSpec("/animal/*"),"animals"); // 5 - p.put(new ServletPathSpec("*.tar.gz"),"tarball"); // 6 - p.put(new ServletPathSpec("*.gz"),"gzipped"); // 7 - p.put(new ServletPathSpec("/"),"default"); // 8 - // 9 was the old Jetty ":" spec delimited case (no longer valid) - p.put(new ServletPathSpec(""),"root"); // 10 - p.put(new ServletPathSpec("/\u20ACuro/*"),"money"); // 11 + p.put(new ServletPathSpec("/"),"default"); + p.put(new ServletPathSpec("/*"),"any"); - // dumpMappings(p); - - // From old PathMapTest - assertMatch(p,"/abs/path","abspath"); - assertMatch(p,"/abs/path/xxx","default"); - assertMatch(p,"/abs/pith","default"); - assertMatch(p,"/abs/path/longer","longpath"); - assertMatch(p,"/abs/path/","default"); - assertMatch(p,"/abs/path/foo","default"); - assertMatch(p,"/animal/bird/eagle/bald","birds"); - assertMatch(p,"/animal/fish/shark/hammerhead","fishes"); - assertMatch(p,"/animal/insect/ladybug","animals"); - assertMatch(p,"/animal","animals"); - assertMatch(p,"/animal/","animals"); - assertMatch(p,"/animal/other","animals"); - assertMatch(p,"/animal/*","animals"); - assertMatch(p,"/downloads/distribution.tar.gz","tarball"); - assertMatch(p,"/downloads/script.gz","gzipped"); - assertMatch(p,"/animal/arhive.gz","animals"); - assertMatch(p,"/Other/path","default"); - assertMatch(p,"/\u20ACuro/path","money"); - assertMatch(p,"/","root"); - - // Extra tests - assertMatch(p,"/downloads/readme.txt","default"); - assertMatch(p,"/downloads/logs.tgz","default"); - assertMatch(p,"/main.css","default"); + assertMatch(p,"/abs/path","any"); + assertMatch(p,"/abs/path/xxx","any"); + assertMatch(p,"/animal/bird/eagle/bald","any"); + assertMatch(p,"/","any"); } /** diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/RegexPathSpecTest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/RegexPathSpecTest.java index a44c4dca504..f78cd750409 100644 --- a/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/RegexPathSpecTest.java +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/RegexPathSpecTest.java @@ -28,13 +28,13 @@ public class RegexPathSpecTest { public static void assertMatches(PathSpec spec, String path) { - String msg = String.format("Spec(\"%s\").matches(\"%s\")",spec.getPathSpec(),path); + String msg = String.format("Spec(\"%s\").matches(\"%s\")",spec.getDeclaration(),path); assertThat(msg,spec.matches(path),is(true)); } public static void assertNotMatches(PathSpec spec, String path) { - String msg = String.format("!Spec(\"%s\").matches(\"%s\")",spec.getPathSpec(),path); + String msg = String.format("!Spec(\"%s\").matches(\"%s\")",spec.getDeclaration(),path); assertThat(msg,spec.matches(path),is(false)); } @@ -42,7 +42,7 @@ public class RegexPathSpecTest public void testExactSpec() { RegexPathSpec spec = new RegexPathSpec("^/a$"); - assertEquals("Spec.pathSpec","^/a$",spec.getPathSpec()); + assertEquals("Spec.pathSpec","^/a$",spec.getDeclaration()); assertEquals("Spec.pattern","^/a$",spec.getPattern().pattern()); assertEquals("Spec.pathDepth",1,spec.getPathDepth()); assertEquals("Spec.group",PathSpecGroup.EXACT,spec.group); @@ -57,7 +57,7 @@ public class RegexPathSpecTest public void testMiddleSpec() { RegexPathSpec spec = new RegexPathSpec("^/rest/([^/]*)/list$"); - assertEquals("Spec.pathSpec","^/rest/([^/]*)/list$",spec.getPathSpec()); + assertEquals("Spec.pathSpec","^/rest/([^/]*)/list$",spec.getDeclaration()); assertEquals("Spec.pattern","^/rest/([^/]*)/list$",spec.getPattern().pattern()); assertEquals("Spec.pathDepth",3,spec.getPathDepth()); assertEquals("Spec.group",PathSpecGroup.MIDDLE_GLOB,spec.group); @@ -78,7 +78,7 @@ public class RegexPathSpecTest public void testMiddleSpecNoGrouping() { RegexPathSpec spec = new RegexPathSpec("^/rest/[^/]+/list$"); - assertEquals("Spec.pathSpec","^/rest/[^/]+/list$",spec.getPathSpec()); + assertEquals("Spec.pathSpec","^/rest/[^/]+/list$",spec.getDeclaration()); assertEquals("Spec.pattern","^/rest/[^/]+/list$",spec.getPattern().pattern()); assertEquals("Spec.pathDepth",3,spec.getPathDepth()); assertEquals("Spec.group",PathSpecGroup.MIDDLE_GLOB,spec.group); @@ -99,7 +99,7 @@ public class RegexPathSpecTest public void testPrefixSpec() { RegexPathSpec spec = new RegexPathSpec("^/a/(.*)$"); - assertEquals("Spec.pathSpec","^/a/(.*)$",spec.getPathSpec()); + assertEquals("Spec.pathSpec","^/a/(.*)$",spec.getDeclaration()); assertEquals("Spec.pattern","^/a/(.*)$",spec.getPattern().pattern()); assertEquals("Spec.pathDepth",2,spec.getPathDepth()); assertEquals("Spec.group",PathSpecGroup.PREFIX_GLOB,spec.group); @@ -117,7 +117,7 @@ public class RegexPathSpecTest public void testSuffixSpec() { RegexPathSpec spec = new RegexPathSpec("^(.*).do$"); - assertEquals("Spec.pathSpec","^(.*).do$",spec.getPathSpec()); + assertEquals("Spec.pathSpec","^(.*).do$",spec.getDeclaration()); assertEquals("Spec.pattern","^(.*).do$",spec.getPattern().pattern()); assertEquals("Spec.pathDepth",0,spec.getPathDepth()); assertEquals("Spec.group",PathSpecGroup.SUFFIX_GLOB,spec.group); diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/ServletPathSpecTest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/ServletPathSpecTest.java index 7252a9c1b48..fb8e65b25a5 100644 --- a/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/ServletPathSpecTest.java +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/ServletPathSpecTest.java @@ -43,13 +43,13 @@ public class ServletPathSpecTest private void assertMatches(ServletPathSpec spec, String path) { - String msg = String.format("Spec(\"%s\").matches(\"%s\")",spec.getPathSpec(),path); + String msg = String.format("Spec(\"%s\").matches(\"%s\")",spec.getDeclaration(),path); assertThat(msg,spec.matches(path),is(true)); } private void assertNotMatches(ServletPathSpec spec, String path) { - String msg = String.format("!Spec(\"%s\").matches(\"%s\")",spec.getPathSpec(),path); + String msg = String.format("!Spec(\"%s\").matches(\"%s\")",spec.getDeclaration(),path); assertThat(msg,spec.matches(path),is(false)); } @@ -87,7 +87,7 @@ public class ServletPathSpecTest public void testDefaultPathSpec() { ServletPathSpec spec = new ServletPathSpec("/"); - assertEquals("Spec.pathSpec","/",spec.getPathSpec()); + assertEquals("Spec.pathSpec","/",spec.getDeclaration()); assertEquals("Spec.pathDepth",-1,spec.getPathDepth()); } @@ -95,7 +95,7 @@ public class ServletPathSpecTest public void testExactPathSpec() { ServletPathSpec spec = new ServletPathSpec("/abs/path"); - assertEquals("Spec.pathSpec","/abs/path",spec.getPathSpec()); + assertEquals("Spec.pathSpec","/abs/path",spec.getDeclaration()); assertEquals("Spec.pathDepth",2,spec.getPathDepth()); assertMatches(spec,"/abs/path"); @@ -125,7 +125,7 @@ public class ServletPathSpecTest public void testNullPathSpec() { ServletPathSpec spec = new ServletPathSpec(null); - assertEquals("Spec.pathSpec","",spec.getPathSpec()); + assertEquals("Spec.pathSpec","",spec.getDeclaration()); assertEquals("Spec.pathDepth",-1,spec.getPathDepth()); } @@ -133,7 +133,7 @@ public class ServletPathSpecTest public void testRootPathSpec() { ServletPathSpec spec = new ServletPathSpec(""); - assertEquals("Spec.pathSpec","",spec.getPathSpec()); + assertEquals("Spec.pathSpec","",spec.getDeclaration()); assertEquals("Spec.pathDepth",-1,spec.getPathDepth()); } @@ -154,7 +154,7 @@ public class ServletPathSpecTest public void testPrefixPathSpec() { ServletPathSpec spec = new ServletPathSpec("/downloads/*"); - assertEquals("Spec.pathSpec","/downloads/*",spec.getPathSpec()); + assertEquals("Spec.pathSpec","/downloads/*",spec.getDeclaration()); assertEquals("Spec.pathDepth",2,spec.getPathDepth()); assertMatches(spec,"/downloads/logo.jpg"); @@ -173,7 +173,7 @@ public class ServletPathSpecTest public void testSuffixPathSpec() { ServletPathSpec spec = new ServletPathSpec("*.gz"); - assertEquals("Spec.pathSpec","*.gz",spec.getPathSpec()); + assertEquals("Spec.pathSpec","*.gz",spec.getDeclaration()); assertEquals("Spec.pathDepth",0,spec.getPathDepth()); assertMatches(spec,"/downloads/distribution.tar.gz"); diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/UriTemplatePathSpecTest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/UriTemplatePathSpecTest.java index 7908344e09c..50dcab923ce 100644 --- a/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/UriTemplatePathSpecTest.java +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/UriTemplatePathSpecTest.java @@ -34,7 +34,7 @@ public class UriTemplatePathSpecTest { private void assertDetectedVars(UriTemplatePathSpec spec, String... expectedVars) { - String prefix = String.format("Spec(\"%s\")",spec.getPathSpec()); + String prefix = String.format("Spec(\"%s\")",spec.getDeclaration()); assertEquals(prefix + ".variableCount",expectedVars.length,spec.getVariableCount()); assertEquals(prefix + ".variable.length",expectedVars.length,spec.getVariables().length); for (int i = 0; i < expectedVars.length; i++) @@ -45,13 +45,13 @@ public class UriTemplatePathSpecTest private void assertMatches(PathSpec spec, String path) { - String msg = String.format("Spec(\"%s\").matches(\"%s\")",spec.getPathSpec(),path); + String msg = String.format("Spec(\"%s\").matches(\"%s\")",spec.getDeclaration(),path); assertThat(msg,spec.matches(path),is(true)); } private void assertNotMatches(PathSpec spec, String path) { - String msg = String.format("!Spec(\"%s\").matches(\"%s\")",spec.getPathSpec(),path); + String msg = String.format("!Spec(\"%s\").matches(\"%s\")",spec.getDeclaration(),path); assertThat(msg,spec.matches(path),is(false)); } @@ -59,7 +59,7 @@ public class UriTemplatePathSpecTest public void testDefaultPathSpec() { UriTemplatePathSpec spec = new UriTemplatePathSpec("/"); - assertEquals("Spec.pathSpec","/",spec.getPathSpec()); + assertEquals("Spec.pathSpec","/",spec.getDeclaration()); assertEquals("Spec.pattern","^/$",spec.getPattern().pattern()); assertEquals("Spec.pathDepth",1,spec.getPathDepth()); assertEquals("Spec.group",PathSpecGroup.EXACT,spec.getGroup()); @@ -72,7 +72,7 @@ public class UriTemplatePathSpecTest public void testExactOnePathSpec() { UriTemplatePathSpec spec = new UriTemplatePathSpec("/a"); - assertEquals("Spec.pathSpec","/a",spec.getPathSpec()); + assertEquals("Spec.pathSpec","/a",spec.getDeclaration()); assertEquals("Spec.pattern","^/a$",spec.getPattern().pattern()); assertEquals("Spec.pathDepth",1,spec.getPathDepth()); assertEquals("Spec.group",PathSpecGroup.EXACT,spec.getGroup()); @@ -90,7 +90,7 @@ public class UriTemplatePathSpecTest public void testExactPathSpec_TestWebapp() { UriTemplatePathSpec spec = new UriTemplatePathSpec("/deep.thought/"); - assertEquals("Spec.pathSpec","/deep.thought/",spec.getPathSpec()); + assertEquals("Spec.pathSpec","/deep.thought/",spec.getDeclaration()); assertEquals("Spec.pattern","^/deep\\.thought/$",spec.getPattern().pattern()); assertEquals("Spec.pathDepth",1,spec.getPathDepth()); assertEquals("Spec.group",PathSpecGroup.EXACT,spec.getGroup()); @@ -106,7 +106,7 @@ public class UriTemplatePathSpecTest public void testExactTwoPathSpec() { UriTemplatePathSpec spec = new UriTemplatePathSpec("/a/b"); - assertEquals("Spec.pathSpec","/a/b",spec.getPathSpec()); + assertEquals("Spec.pathSpec","/a/b",spec.getDeclaration()); assertEquals("Spec.pattern","^/a/b$",spec.getPattern().pattern()); assertEquals("Spec.pathDepth",2,spec.getPathDepth()); assertEquals("Spec.group",PathSpecGroup.EXACT,spec.getGroup()); @@ -125,7 +125,7 @@ public class UriTemplatePathSpecTest public void testMiddleVarPathSpec() { UriTemplatePathSpec spec = new UriTemplatePathSpec("/a/{var}/c"); - assertEquals("Spec.pathSpec","/a/{var}/c",spec.getPathSpec()); + assertEquals("Spec.pathSpec","/a/{var}/c",spec.getDeclaration()); assertEquals("Spec.pattern","^/a/([^/]+)/c$",spec.getPattern().pattern()); assertEquals("Spec.pathDepth",3,spec.getPathDepth()); assertEquals("Spec.group",PathSpecGroup.MIDDLE_GLOB,spec.getGroup()); @@ -149,7 +149,7 @@ public class UriTemplatePathSpecTest public void testOneVarPathSpec() { UriTemplatePathSpec spec = new UriTemplatePathSpec("/a/{foo}"); - assertEquals("Spec.pathSpec","/a/{foo}",spec.getPathSpec()); + assertEquals("Spec.pathSpec","/a/{foo}",spec.getDeclaration()); assertEquals("Spec.pattern","^/a/([^/]+)$",spec.getPattern().pattern()); assertEquals("Spec.pathDepth",2,spec.getPathDepth()); assertEquals("Spec.group",PathSpecGroup.PREFIX_GLOB,spec.getGroup()); @@ -170,7 +170,7 @@ public class UriTemplatePathSpecTest public void testOneVarSuffixPathSpec() { UriTemplatePathSpec spec = new UriTemplatePathSpec("/{var}/b/c"); - assertEquals("Spec.pathSpec","/{var}/b/c",spec.getPathSpec()); + assertEquals("Spec.pathSpec","/{var}/b/c",spec.getDeclaration()); assertEquals("Spec.pattern","^/([^/]+)/b/c$",spec.getPattern().pattern()); assertEquals("Spec.pathDepth",3,spec.getPathDepth()); assertEquals("Spec.group",PathSpecGroup.SUFFIX_GLOB,spec.getGroup()); @@ -194,7 +194,7 @@ public class UriTemplatePathSpecTest public void testTwoVarComplexInnerPathSpec() { UriTemplatePathSpec spec = new UriTemplatePathSpec("/a/{var1}/c/{var2}/e"); - assertEquals("Spec.pathSpec","/a/{var1}/c/{var2}/e",spec.getPathSpec()); + assertEquals("Spec.pathSpec","/a/{var1}/c/{var2}/e",spec.getDeclaration()); assertEquals("Spec.pattern","^/a/([^/]+)/c/([^/]+)/e$",spec.getPattern().pattern()); assertEquals("Spec.pathDepth",5,spec.getPathDepth()); assertEquals("Spec.group",PathSpecGroup.MIDDLE_GLOB,spec.getGroup()); @@ -217,7 +217,7 @@ public class UriTemplatePathSpecTest public void testTwoVarComplexOuterPathSpec() { UriTemplatePathSpec spec = new UriTemplatePathSpec("/{var1}/b/{var2}/{var3}"); - assertEquals("Spec.pathSpec","/{var1}/b/{var2}/{var3}",spec.getPathSpec()); + assertEquals("Spec.pathSpec","/{var1}/b/{var2}/{var3}",spec.getDeclaration()); assertEquals("Spec.pattern","^/([^/]+)/b/([^/]+)/([^/]+)$",spec.getPattern().pattern()); assertEquals("Spec.pathDepth",4,spec.getPathDepth()); assertEquals("Spec.group",PathSpecGroup.MIDDLE_GLOB,spec.getGroup()); @@ -241,7 +241,7 @@ public class UriTemplatePathSpecTest public void testTwoVarPrefixPathSpec() { UriTemplatePathSpec spec = new UriTemplatePathSpec("/a/{var1}/{var2}"); - assertEquals("Spec.pathSpec","/a/{var1}/{var2}",spec.getPathSpec()); + assertEquals("Spec.pathSpec","/a/{var1}/{var2}",spec.getDeclaration()); assertEquals("Spec.pattern","^/a/([^/]+)/([^/]+)$",spec.getPattern().pattern()); assertEquals("Spec.pathDepth",3,spec.getPathDepth()); assertEquals("Spec.group",PathSpecGroup.PREFIX_GLOB,spec.getGroup()); @@ -264,7 +264,7 @@ public class UriTemplatePathSpecTest public void testVarOnlyPathSpec() { UriTemplatePathSpec spec = new UriTemplatePathSpec("/{var1}"); - assertEquals("Spec.pathSpec","/{var1}",spec.getPathSpec()); + assertEquals("Spec.pathSpec","/{var1}",spec.getDeclaration()); assertEquals("Spec.pattern","^/([^/]+)$",spec.getPattern().pattern()); assertEquals("Spec.pathDepth",1,spec.getPathDepth()); assertEquals("Spec.group",PathSpecGroup.PREFIX_GLOB,spec.getGroup()); diff --git a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/DefaultServlet.java b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/DefaultServlet.java index be9e81423fc..90b641c8fac 100644 --- a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/DefaultServlet.java +++ b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/DefaultServlet.java @@ -51,6 +51,7 @@ import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.HttpMethod; import org.eclipse.jetty.http.MimeTypes; import org.eclipse.jetty.http.PathMap.MappedEntry; +import org.eclipse.jetty.http.pathmap.MappedResource; import org.eclipse.jetty.http.PreEncodedHttpField; import org.eclipse.jetty.http.ResourceHttpContent; import org.eclipse.jetty.io.WriterOutputStream; @@ -672,9 +673,9 @@ public class DefaultServlet extends HttpServlet implements ResourceFactory if ((_welcomeServlets || _welcomeExactServlets) && welcome_servlet==null) { - MappedEntry entry=_servletHandler.getHolderEntry(welcome_in_context); - if (entry!=null && entry.getValue()!=_defaultHolder && - (_welcomeServlets || (_welcomeExactServlets && entry.getKey().equals(welcome_in_context)))) + MappedResource entry=_servletHandler.getHolderEntry(welcome_in_context); + if (entry!=null && entry.getResource()!=_defaultHolder && + (_welcomeServlets || (_welcomeExactServlets && entry.getPathSpec().getDeclaration().equals(welcome_in_context)))) welcome_servlet=welcome_in_context; } diff --git a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/Invoker.java b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/Invoker.java index ec87f24f86c..ea1430d5aa9 100644 --- a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/Invoker.java +++ b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/Invoker.java @@ -32,6 +32,8 @@ import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletRequestWrapper; import javax.servlet.http.HttpServletResponse; +import org.eclipse.jetty.http.PathMap.MappedEntry; +import org.eclipse.jetty.http.pathmap.MappedResource; import org.eclipse.jetty.server.Dispatcher; import org.eclipse.jetty.server.Handler; import org.eclipse.jetty.server.Request; @@ -71,7 +73,7 @@ public class Invoker extends HttpServlet private ContextHandler _contextHandler; private ServletHandler _servletHandler; - private Map.Entry _invokerEntry; + private MappedResource _invokerEntry; private Map _parameters; private boolean _nonContextServlets; private boolean _verbose; @@ -170,12 +172,12 @@ public class Invoker extends HttpServlet // Check for existing mapping (avoid threaded race). String path=URIUtil.addPaths(servlet_path,servlet); - Map.Entry entry = _servletHandler.getHolderEntry(path); + MappedResource entry = _servletHandler.getHolderEntry(path); if (entry!=null && !entry.equals(_invokerEntry)) { // Use the holder - holder=(ServletHolder)entry.getValue(); + holder=(ServletHolder)entry.getResource(); } else { diff --git a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java index 521468dd61b..ebafc965b28 100644 --- a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java +++ b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java @@ -49,7 +49,10 @@ import javax.servlet.http.HttpServlet; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; -import org.eclipse.jetty.http.PathMap; +import org.eclipse.jetty.http.pathmap.MappedResource; +import org.eclipse.jetty.http.pathmap.PathMappings; +import org.eclipse.jetty.http.pathmap.PathSpec; +import org.eclipse.jetty.http.pathmap.ServletPathSpec; import org.eclipse.jetty.security.IdentityService; import org.eclipse.jetty.security.SecurityHandler; import org.eclipse.jetty.server.Request; @@ -111,7 +114,8 @@ public class ServletHandler extends ScopedHandler private MultiMap _filterNameMappings; private final Map _servletNameMap=new HashMap<>(); - private PathMap _servletPathMap; + // private PathMap _servletPathMap; + private PathMappings _servletPathMap; private ListenerHolder[] _listeners=new ListenerHolder[0]; @@ -358,7 +362,7 @@ public class ServletHandler extends ScopedHandler * @param pathInContext Path within _context. * @return PathMap Entries pathspec to ServletHolder */ - public PathMap.MappedEntry getHolderEntry(String pathInContext) + public MappedResource getHolderEntry(String pathInContext) { if (_servletPathMap==null) return null; @@ -436,14 +440,14 @@ public class ServletHandler extends ScopedHandler if (target.startsWith("/")) { // Look for the servlet by path - PathMap.MappedEntry entry=getHolderEntry(target); + MappedResource entry=getHolderEntry(target); if (entry!=null) { - servlet_holder=entry.getValue(); + PathSpec pathSpec = entry.getPathSpec(); + servlet_holder=entry.getResource(); - String servlet_path_spec= entry.getKey(); - String servlet_path=entry.getMapped()!=null?entry.getMapped():PathMap.pathMatch(servlet_path_spec,target); - String path_info=PathMap.pathInfo(servlet_path_spec,target); + String servlet_path=pathSpec.getPathMatch(target); + String path_info=pathSpec.getPathInfo(target); if (DispatcherType.INCLUDE.equals(type)) { @@ -1307,7 +1311,7 @@ public class ServletHandler extends ScopedHandler } else { - PathMap pm = new PathMap<>(); + PathMappings pm = new PathMappings<>(); Map servletPathMappings = new HashMap<>(); //create a map of paths to set of ServletMappings that define that mapping @@ -1370,7 +1374,7 @@ public class ServletHandler extends ScopedHandler if (LOG.isDebugEnabled()) LOG.debug("Chose path={} mapped to servlet={} from default={}", pathSpec, finalMapping.getServletName(), finalMapping.isDefault()); servletPathMappings.put(pathSpec, finalMapping); - pm.put(pathSpec,_servletNameMap.get(finalMapping.getServletName())); + pm.put(new ServletPathSpec(pathSpec),_servletNameMap.get(finalMapping.getServletName())); } _servletPathMap=pm; From b6df9508c6991dd672851cc405f3250d733bc03e Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Mon, 14 Dec 2015 14:58:12 -0700 Subject: [PATCH 08/10] 484350 - Allow GzipHandler path include/exclude to use regex + Overhauled IncludeExclude to use java 8 predicate + Introduced PathSpecSet to standardize path IncludeExclude + GzipHandler now uses PathSpecSet for paths --- .../java/org/eclipse/jetty/http/PathMap.java | 11 +- .../jetty/http/pathmap/PathSpecSet.java | 216 ++++++++++++++++++ .../server/handler/gzip/GzipHandler.java | 51 ++++- .../server/handler/gzip/GzipHandlerTest.java | 42 ++++ .../eclipse/jetty/util/IncludeExclude.java | 72 ++++-- .../java/org/eclipse/jetty/util/RegexSet.java | 10 +- .../jetty/util/IncludeExcludeTest.java | 19 +- 7 files changed, 378 insertions(+), 43 deletions(-) create mode 100644 jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/PathSpecSet.java create mode 100644 jetty-server/src/test/java/org/eclipse/jetty/server/handler/gzip/GzipHandlerTest.java diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/PathMap.java b/jetty-http/src/main/java/org/eclipse/jetty/http/PathMap.java index bb30c3b4d15..448be2e26db 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/PathMap.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/PathMap.java @@ -27,6 +27,7 @@ import java.util.List; import java.util.Map; import java.util.StringTokenizer; import java.util.function.BiFunction; +import java.util.function.Predicate; import org.eclipse.jetty.util.ArrayTernaryTrie; import org.eclipse.jetty.util.RegexSet; @@ -592,9 +593,8 @@ public class PathMap extends HashMap } } - public static class PathSet extends AbstractSet + public static class PathSet extends AbstractSet implements Predicate { - public static final BiFunction MATCHER=(s,e)->{return s.containsMatch(e);}; private final PathMap _map = new PathMap<>(); @Override @@ -627,12 +627,15 @@ public class PathMap extends HashMap return _map.containsKey(o); } + @Override + public boolean test(String s) + { + return _map.containsMatch(s); + } public boolean containsMatch(String s) { return _map.containsMatch(s); } - - } } diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/PathSpecSet.java b/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/PathSpecSet.java new file mode 100644 index 00000000000..0fd66f01d0b --- /dev/null +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/PathSpecSet.java @@ -0,0 +1,216 @@ +// +// ======================================================================== +// Copyright (c) 1995-2015 Mort Bay Consulting Pty. Ltd. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.http.pathmap; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.Iterator; +import java.util.List; +import java.util.Set; +import java.util.TreeSet; +import java.util.function.Predicate; + +/** + * A Set of PathSpec strings. + *

+ * Used by {@link org.eclipse.jetty.util.IncludeExclude} logic + */ +public class PathSpecSet implements Set, Predicate +{ + private final Set specs = new TreeSet<>(); + + @Override + public boolean test(String s) + { + for (PathSpec spec : specs) + { + if (spec.matches(s)) + { + return true; + } + } + return false; + } + + @Override + public boolean isEmpty() + { + return specs.isEmpty(); + } + + @Override + public Iterator iterator() + { + return new Iterator() + { + private Iterator iter = specs.iterator(); + + @Override + public boolean hasNext() + { + return iter.hasNext(); + } + + @Override + public String next() + { + PathSpec spec = iter.next(); + if (spec == null) + { + return null; + } + return spec.getDeclaration(); + } + }; + } + + @Override + public int size() + { + return specs.size(); + } + + @Override + public boolean contains(Object o) + { + if (o instanceof PathSpec) + { + return specs.contains(o); + } + if (o instanceof String) + { + return specs.contains(toPathSpec((String)o)); + } + return false; + } + + private PathSpec asPathSpec(Object o) + { + if (o == null) + { + return null; + } + if (o instanceof PathSpec) + { + return (PathSpec)o; + } + if (o instanceof String) + { + return toPathSpec((String)o); + } + return toPathSpec(o.toString()); + } + + private PathSpec toPathSpec(String rawSpec) + { + if ((rawSpec == null) || (rawSpec.length() < 1)) + { + throw new RuntimeException("Path Spec String must start with '^', '/', or '*.': got [" + rawSpec + "]"); + } + if (rawSpec.charAt(0) == '^') + { + return new RegexPathSpec(rawSpec); + } + else + { + return new ServletPathSpec(rawSpec); + } + } + + @Override + public Object[] toArray() + { + return toArray(new String[specs.size()]); + } + + @Override + public T[] toArray(T[] a) + { + int i = 0; + for (PathSpec spec : specs) + { + a[i++] = (T)spec.getDeclaration(); + } + return a; + } + + @Override + public boolean add(String e) + { + return specs.add(toPathSpec(e)); + } + + @Override + public boolean remove(Object o) + { + return specs.remove(asPathSpec(o)); + } + + @Override + public boolean containsAll(Collection coll) + { + for (Object o : coll) + { + if (!specs.contains(asPathSpec(o))) + return false; + } + return true; + } + + @Override + public boolean addAll(Collection coll) + { + boolean ret = false; + + for (String s : coll) + { + ret |= add(s); + } + + return ret; + } + + @Override + public boolean retainAll(Collection coll) + { + List collSpecs = new ArrayList<>(); + for (Object o : coll) + { + collSpecs.add(asPathSpec(o)); + } + return specs.retainAll(collSpecs); + } + + @Override + public boolean removeAll(Collection coll) + { + List collSpecs = new ArrayList<>(); + for (Object o : coll) + { + collSpecs.add(asPathSpec(o)); + } + return specs.removeAll(collSpecs); + } + + @Override + public void clear() + { + specs.clear(); + } +} diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHandler.java index 8a14d13ab5f..da6182604ed 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHandler.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHandler.java @@ -36,7 +36,7 @@ import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.HttpMethod; import org.eclipse.jetty.http.HttpVersion; import org.eclipse.jetty.http.MimeTypes; -import org.eclipse.jetty.http.PathMap; +import org.eclipse.jetty.http.pathmap.PathSpecSet; import org.eclipse.jetty.server.HttpOutput; import org.eclipse.jetty.server.Request; import org.eclipse.jetty.server.handler.HandlerWrapper; @@ -72,9 +72,9 @@ public class GzipHandler extends HandlerWrapper implements GzipFactory // non-static, as other GzipHandler instances may have different configurations private final ThreadLocal _deflater = new ThreadLocal(); - private final IncludeExclude _agentPatterns=new IncludeExclude<>(RegexSet.class,RegexSet.MATCHER); + private final IncludeExclude _agentPatterns=new IncludeExclude<>(RegexSet.class); private final IncludeExclude _methods = new IncludeExclude<>(); - private final IncludeExclude _paths = new IncludeExclude<>(PathMap.PathSet.class,PathMap.PathSet.MATCHER); + private final IncludeExclude _paths = new IncludeExclude(PathSpecSet.class); private final IncludeExclude _mimeTypes = new IncludeExclude<>(); private HttpField _vary; @@ -144,9 +144,27 @@ public class GzipHandler extends HandlerWrapper implements GzipFactory /* ------------------------------------------------------------ */ /** + * Add path to excluded paths list. + *

+ * There are 2 syntaxes supported, Servlet url-pattern based, and + * Regex based. This means that the initial characters on the path spec + * line are very strict, and determine the behavior of the path matching. + *

    + *
  • If the spec starts with '^' the spec is assumed to be + * a regex based path spec and will match with normal Java regex rules.
    • + *
  • If the spec starts with '/' then spec is assumed to be + * a Servlet url-pattern rules path spec for either an exact match + * or prefix based match.
    • + *
  • If the spec starts with '*.' then spec is assumed to be + * a Servlet url-pattern rules path spec for a suffix based match.
    • + *
  • All other syntaxes are unsupported
    • + *
+ *

+ * Note: inclusion takes precedence over exclude. + * * @param pathspecs Path specs (as per servlet spec) to exclude. If a * ServletContext is available, the paths are relative to the context path, - * otherwise they are absolute. + * otherwise they are absolute.
* For backward compatibility the pathspecs may be comma separated strings, but this * will not be supported in future versions. */ @@ -191,12 +209,27 @@ public class GzipHandler extends HandlerWrapper implements GzipFactory /* ------------------------------------------------------------ */ /** - * Add path specs to include. Inclusion takes precedence over exclusion. + * Add path specs to include. + *

+ * There are 2 syntaxes supported, Servlet url-pattern based, and + * Regex based. This means that the initial characters on the path spec + * line are very strict, and determine the behavior of the path matching. + *

    + *
  • If the spec starts with '^' the spec is assumed to be + * a regex based path spec and will match with normal Java regex rules.
    • + *
  • If the spec starts with '/' then spec is assumed to be + * a Servlet url-pattern rules path spec for either an exact match + * or prefix based match.
    • + *
  • If the spec starts with '*.' then spec is assumed to be + * a Servlet url-pattern rules path spec for a suffix based match.
    • + *
  • All other syntaxes are unsupported
    • + *
+ *

+ * Note: inclusion takes precedence over exclude. + * * @param pathspecs Path specs (as per servlet spec) to include. If a * ServletContext is available, the paths are relative to the context path, * otherwise they are absolute - * For backward compatibility the pathspecs may be comma separated strings, but this - * will not be supported in future versions. */ public void addIncludedPaths(String... pathspecs) { @@ -334,9 +367,9 @@ public class GzipHandler extends HandlerWrapper implements GzipFactory /* ------------------------------------------------------------ */ /** - * Get the minimum reponse size. + * Get the minimum response size. * - * @return minimum reponse size + * @return minimum response size */ public int getMinGzipSize() { diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/handler/gzip/GzipHandlerTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/handler/gzip/GzipHandlerTest.java new file mode 100644 index 00000000000..0b3ac9252c2 --- /dev/null +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/handler/gzip/GzipHandlerTest.java @@ -0,0 +1,42 @@ +// +// ======================================================================== +// Copyright (c) 1995-2015 Mort Bay Consulting Pty. Ltd. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.server.handler.gzip; + +import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.is; +import static org.junit.Assert.assertThat; + +import java.util.Arrays; + +import org.junit.Test; + +public class GzipHandlerTest +{ + @Test + public void testAddGetPaths() + { + GzipHandler gzip = new GzipHandler(); + gzip.addIncludedPaths("/foo"); + gzip.addIncludedPaths("^/bar.*$"); + + String[] includedPaths = gzip.getIncludedPaths(); + assertThat("Included Paths.size", includedPaths.length, is(2)); + assertThat("Included Paths", Arrays.asList(includedPaths), contains("/foo","^/bar.*$")); + } +} diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/IncludeExclude.java b/jetty-util/src/main/java/org/eclipse/jetty/util/IncludeExclude.java index fd20040cab8..03cf0553a15 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/IncludeExclude.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/IncludeExclude.java @@ -20,7 +20,7 @@ package org.eclipse.jetty.util; import java.util.HashSet; import java.util.Set; -import java.util.function.BiFunction; +import java.util.function.Predicate; /** Utility class to maintain a set of inclusions and exclusions. @@ -36,35 +36,77 @@ public class IncludeExclude { private final Set _includes; private final Set _excludes; - private final BiFunction,ITEM, Boolean> _matcher; - + private final Predicate _includePredicate; + private final Predicate _excludePredicate; + + private static class SetContainsPredicate implements Predicate + { + private final Set set; + + public SetContainsPredicate(Set set) + { + this.set = set; + } + + @Override + public boolean test(ITEM item) + { + return set.contains(item); + } + } + /** * Default constructor over {@link HashSet} */ public IncludeExclude() { - this(HashSet.class,null); + this(HashSet.class); } /** * Construct an IncludeExclude * @param setClass The type of {@link Set} to using internally - * @param matcher A function to test if a passed ITEM is matched by the passed SET, or null to use {@link Set#contains(Object)} + * @param predicate A predicate function to test if a passed ITEM is matched by the passed SET} */ - public > IncludeExclude(Class setClass, BiFunction matcher) + public > IncludeExclude(Class setClass) { try { _includes = setClass.newInstance(); _excludes = setClass.newInstance(); - _matcher = (BiFunction,ITEM, Boolean>)matcher; + if(_includes instanceof Predicate) { + _includePredicate = (Predicate)_includes; + } else { + _includePredicate = new SetContainsPredicate<>(_includes); + } + if(_excludes instanceof Predicate) { + _excludePredicate = (Predicate)_excludes; + } else { + _excludePredicate = new SetContainsPredicate<>(_excludes); + } } catch (InstantiationException | IllegalAccessException e) { throw new RuntimeException(e); } } - + + /** + * Construct an IncludeExclude + * + * @param includeSet the Set of items that represent the included space + * @param includePredicate the Predicate for included item testing (null for simple {@link Set#contains(Object)} test) + * @param excludeSet the Set of items that represent the excluded space + * @param excludePredicate the Predicate for excluded item testing (null for simple {@link Set#contains(Object)} test) + */ + public > IncludeExclude(Set includeSet, Predicate includePredicate, Set excludeSet, Predicate excludePredicate) + { + _includes = includeSet; + _includePredicate = includePredicate; + _excludes = excludeSet; + _excludePredicate = excludePredicate; + } + public void include(ITEM element) { _includes.add(element); @@ -89,17 +131,11 @@ public class IncludeExclude public boolean matches(ITEM e) { - if (_matcher==null) - { - if (_includes.size()>0 && !_includes.contains(e)) - return false; - return !_excludes.contains(e); - } - if (_includes.size()>0 && !_matcher.apply(_includes,e)) + if (!_includes.isEmpty() && !_includePredicate.test(e)) return false; - return !_matcher.apply(_excludes,e); + return !_excludePredicate.test(e); } - + public int size() { return _includes.size()+_excludes.size(); @@ -124,6 +160,6 @@ public class IncludeExclude @Override public String toString() { - return String.format("%s@%x{i=%s,e=%s,m=%s}",this.getClass().getSimpleName(),hashCode(),_includes,_excludes,_matcher); + return String.format("%s@%x{i=%s,ip=%s,e=%s,ep=%s}",this.getClass().getSimpleName(),hashCode(),_includes,_includePredicate,_excludes,_excludePredicate); } } diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/RegexSet.java b/jetty-util/src/main/java/org/eclipse/jetty/util/RegexSet.java index ff72b039a43..c8db063fd77 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/RegexSet.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/RegexSet.java @@ -24,6 +24,7 @@ import java.util.HashSet; import java.util.Iterator; import java.util.Set; import java.util.function.BiFunction; +import java.util.function.Predicate; import java.util.regex.Pattern; @@ -32,9 +33,8 @@ import java.util.regex.Pattern; *

* Provides the efficient {@link #matches(String)} method to check for a match against all the combined Regex's */ -public class RegexSet extends AbstractSet +public class RegexSet extends AbstractSet implements Predicate { - public static final BiFunction MATCHER=(rs,p)->{return rs.matches(p);}; private final Set _patterns=new HashSet(); private final Set _unmodifiable=Collections.unmodifiableSet(_patterns); private Pattern _pattern; @@ -98,6 +98,12 @@ public class RegexSet extends AbstractSet builder.append(")$"); _pattern = Pattern.compile(builder.toString()); } + + @Override + public boolean test(String s) + { + return _pattern!=null && _pattern.matcher(s).matches(); + } public boolean matches(String s) { diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/IncludeExcludeTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/IncludeExcludeTest.java index 8da30fdb38a..4b7dde734ec 100644 --- a/jetty-util/src/test/java/org/eclipse/jetty/util/IncludeExcludeTest.java +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/IncludeExcludeTest.java @@ -20,7 +20,9 @@ package org.eclipse.jetty.util; import org.junit.Test; +import static org.hamcrest.Matchers.is; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThat; public class IncludeExcludeTest { @@ -29,8 +31,8 @@ public class IncludeExcludeTest { IncludeExclude ie = new IncludeExclude<>(); - assertEquals(0,ie.size()); - assertEquals(true,ie.matches("foo")); + assertThat("Empty IncludeExclude", ie.size(), is(0)); + assertThat("Matches 'foo'",ie.matches("foo"),is(true)); } @Test @@ -40,7 +42,7 @@ public class IncludeExcludeTest ie.include("foo"); ie.include("bar"); - assertEquals(2,ie.size()); + assertThat("IncludeExclude.size", ie.size(), is(2)); assertEquals(false,ie.matches("")); assertEquals(true,ie.matches("foo")); assertEquals(true,ie.matches("bar")); @@ -86,7 +88,7 @@ public class IncludeExcludeTest @Test public void testEmptyRegex() { - IncludeExclude ie = new IncludeExclude<>(RegexSet.class,RegexSet.MATCHER); + IncludeExclude ie = new IncludeExclude<>(RegexSet.class); assertEquals(0,ie.size()); assertEquals(true,ie.matches("foo")); @@ -95,7 +97,7 @@ public class IncludeExcludeTest @Test public void testIncludeRegex() { - IncludeExclude ie = new IncludeExclude<>(RegexSet.class,RegexSet.MATCHER); + IncludeExclude ie = new IncludeExclude<>(RegexSet.class); ie.include("f.."); ie.include("b((ar)|(oo))"); @@ -112,7 +114,7 @@ public class IncludeExcludeTest @Test public void testExcludeRegex() { - IncludeExclude ie = new IncludeExclude<>(RegexSet.class,RegexSet.MATCHER); + IncludeExclude ie = new IncludeExclude<>(RegexSet.class); ie.exclude("f.."); ie.exclude("b((ar)|(oo))"); @@ -130,7 +132,7 @@ public class IncludeExcludeTest @Test public void testIncludeExcludeRegex() { - IncludeExclude ie = new IncludeExclude<>(RegexSet.class,RegexSet.MATCHER); + IncludeExclude ie = new IncludeExclude<>(RegexSet.class); ie.include(".*[aeiou].*"); ie.include("[AEIOU].*"); ie.exclude("f.."); @@ -146,8 +148,5 @@ public class IncludeExcludeTest assertEquals(true,ie.matches("foobar")); assertEquals(true,ie.matches("Ant")); - } - - } From d4d9ceea86e7585c0346bc1e7efad6e2ed45d386 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Tue, 15 Dec 2015 14:11:56 +0100 Subject: [PATCH 09/10] Improved toString(). --- .../java/org/eclipse/jetty/http2/HTTP2Session.java | 12 ++++++++++-- .../http2/client/http/HttpConnectionOverHTTP2.java | 9 +++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java index a3efe4057ce..64465ab4dfa 100644 --- a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java +++ b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java @@ -989,8 +989,16 @@ public abstract class HTTP2Session implements ISession, Parser.Listener @Override public String toString() { - return String.format("%s@%x{queueSize=%d,sendWindow=%s,recvWindow=%s,streams=%d,%s}", getClass().getSimpleName(), - hashCode(), flusher.getQueueSize(), sendWindow, recvWindow, streams.size(), closed); + return String.format("%s@%x{l:%s <-> r:%s,queueSize=%d,sendWindow=%s,recvWindow=%s,streams=%d,%s}", + getClass().getSimpleName(), + hashCode(), + getEndPoint().getLocalAddress(), + getEndPoint().getRemoteAddress(), + flusher.getQueueSize(), + sendWindow, + recvWindow, + streams.size(), + closed); } private class ControlEntry extends HTTP2Flusher.Entry diff --git a/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpConnectionOverHTTP2.java b/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpConnectionOverHTTP2.java index 09ae404448b..d6624a54e8e 100644 --- a/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpConnectionOverHTTP2.java +++ b/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpConnectionOverHTTP2.java @@ -85,4 +85,13 @@ public class HttpConnectionOverHTTP2 extends HttpConnection } channels.clear(); } + + @Override + public String toString() + { + return String.format("%s@%h[%s]", + getClass().getSimpleName(), + this, + session); + } } From 717fc7819d8a41cb21acfe2d5f6c82d105aac30a Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Tue, 15 Dec 2015 15:35:04 +0100 Subject: [PATCH 10/10] 484262 - Race condition between GOAWAY disconnect and ability to make new request. Fixed by making sure that when a peer received a GOAWAY frame, it does not also notify the onFailure() callback. --- .../eclipse/jetty/http2/client/HTTP2Test.java | 43 +++++++++++++++++++ .../org/eclipse/jetty/http2/HTTP2Flusher.java | 16 +++---- 2 files changed, 50 insertions(+), 9 deletions(-) diff --git a/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/HTTP2Test.java b/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/HTTP2Test.java index 1f68283fcf4..1bf274298d1 100644 --- a/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/HTTP2Test.java +++ b/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/HTTP2Test.java @@ -38,6 +38,7 @@ import org.eclipse.jetty.http.HttpScheme; import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.HttpVersion; import org.eclipse.jetty.http.MetaData; +import org.eclipse.jetty.http2.ErrorCode; import org.eclipse.jetty.http2.api.Session; import org.eclipse.jetty.http2.api.Stream; import org.eclipse.jetty.http2.api.server.ServerSessionListener; @@ -416,4 +417,46 @@ public class HTTP2Test extends AbstractTest Assert.assertTrue(exchangeLatch2.await(5, TimeUnit.SECONDS)); Assert.assertEquals(0, session.getStreams().size()); } + + @Test + public void testCleanGoAwayDoesNotTriggerFailureNotification() throws Exception + { + start(new ServerSessionListener.Adapter() + { + @Override + public Stream.Listener onNewStream(Stream stream, HeadersFrame frame) + { + MetaData.Response metaData = new MetaData.Response(HttpVersion.HTTP_2, HttpStatus.OK_200, new HttpFields()); + HeadersFrame response = new HeadersFrame(stream.getId(), metaData, null, true); + stream.headers(response, Callback.NOOP); + // Close cleanly. + stream.getSession().close(ErrorCode.NO_ERROR.code, null, Callback.NOOP); + return null; + } + }); + + CountDownLatch closeLatch = new CountDownLatch(1); + CountDownLatch failureLatch = new CountDownLatch(1); + Session session = newClient(new Session.Listener.Adapter() + { + @Override + public void onClose(Session session, GoAwayFrame frame) + { + closeLatch.countDown(); + } + + @Override + public void onFailure(Session session, Throwable failure) + { + failureLatch.countDown(); + } + }); + MetaData.Request metaData = newRequest("GET", new HttpFields()); + HeadersFrame request = new HeadersFrame(metaData, null, true); + session.newStream(request, new Promise.Adapter<>(), new Stream.Listener.Adapter()); + + // Make sure onClose() is called. + Assert.assertTrue(closeLatch.await(5, TimeUnit.SECONDS)); + Assert.assertFalse(failureLatch.await(1, TimeUnit.SECONDS)); + } } diff --git a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Flusher.java b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Flusher.java index dd0bc539ce6..9dbb4068508 100644 --- a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Flusher.java +++ b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Flusher.java @@ -230,7 +230,7 @@ public class HTTP2Flusher extends IteratingCallback if (actives.isEmpty()) { if (isClosed()) - abort(new ClosedChannelException()); + fail(new ClosedChannelException(), true); if (LOG.isDebugEnabled()) LOG.debug("Flushed {}", session); @@ -289,9 +289,6 @@ public class HTTP2Flusher extends IteratingCallback @Override protected void onCompleteFailure(Throwable x) { - if (LOG.isDebugEnabled()) - LOG.debug("Failed", x); - lease.recycle(); // Transfer active items to avoid reentrancy. @@ -306,10 +303,10 @@ public class HTTP2Flusher extends IteratingCallback entry.failed(x); } - abort(x); + fail(x, isClosed()); } - private void abort(Throwable x) + private void fail(Throwable x, boolean closed) { Queue queued; synchronized (this) @@ -319,12 +316,13 @@ public class HTTP2Flusher extends IteratingCallback } if (LOG.isDebugEnabled()) - LOG.debug("Aborting, queued={}", queued.size()); + LOG.debug("{}, queued={}", closed ? "Closing" : "Failing", queued.size()); for (Entry entry : queued) - closed(entry, x); + entry.failed(x); - session.abort(x); + if (!closed) + session.abort(x); } private void closed(Entry entry, Throwable failure)