This is an automated email from the ASF dual-hosted git repository. mblow pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/asterixdb.git
commit 4bb97916ced8503cf2fc574dbb504c91d1bfc633 Author: Michael Blow <[email protected]> AuthorDate: Thu Feb 20 21:03:21 2020 -0500 [NO ISSUE][HYR][HTTP] Include 'Permanent' response header on unknown servlet 404 - also, fix processing order of specific over wildcard for non-primary servlet paths Change-Id: I9b838abc3b8af327b3597bf75039550147f6ee61 Reviewed-on: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/5143 Contrib: Michael Blow <[email protected]> Reviewed-by: Michael Blow <[email protected]> Reviewed-by: Till Westmann <[email protected]> Integration-Tests: Jenkins <[email protected]> Tested-by: Jenkins <[email protected]> --- .../org/apache/hyracks/http/server/HttpServer.java | 64 +++------------- .../hyracks/http/server/HttpServerHandler.java | 13 +++- .../hyracks/http/server/ServletRegistry.java | 87 ++++++++++++++++++++++ .../apache/hyracks/http/server/utils/HttpUtil.java | 50 +++++++------ .../apache/hyracks/http/server/PathMatchTest.java | 18 ++--- 5 files changed, 145 insertions(+), 87 deletions(-) diff --git a/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/HttpServer.java b/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/HttpServer.java index 2a7b47e..4cae09f 100644 --- a/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/HttpServer.java +++ b/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/HttpServer.java @@ -19,9 +19,7 @@ package org.apache.hyracks.http.server; import java.net.InetSocketAddress; -import java.util.ArrayList; -import java.util.Collections; -import java.util.List; +import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; import java.util.concurrent.LinkedBlockingQueue; @@ -71,7 +69,7 @@ public class HttpServer { private final AtomicInteger threadId = new AtomicInteger(); private final ConcurrentMap<String, Object> ctx; private final LinkedBlockingQueue<Runnable> workQueue; - private final List<IServlet> servlets; + private final ServletRegistry servlets; private final EventLoopGroup bossGroup; private final EventLoopGroup workerGroup; private final InetSocketAddress address; @@ -100,7 +98,7 @@ public class HttpServer { this.closedHandler = closeHandler; this.config = config; ctx = new ConcurrentHashMap<>(); - servlets = new ArrayList<>(); + servlets = new ServletRegistry(); workQueue = new LinkedBlockingQueue<>(config.getRequestQueueSize()); int numExecutorThreads = config.getThreadCount(); executor = new ThreadPoolExecutor(numExecutorThreads, numExecutorThreads, 0L, TimeUnit.MILLISECONDS, workQueue, @@ -219,21 +217,14 @@ public class HttpServer { } public void addServlet(IServlet let) { - servlets.add(let); + servlets.register(let); + } + + public Set<IServlet> getServlets() { + return servlets.getServlets(); } protected void doStart() throws InterruptedException { - /* - * This is a hacky way to ensure that IServlets with more specific paths are checked first. - * For example: - * "/path/to/resource/" - * is checked before - * "/path/to/" - * which in turn is checked before - * "/path/" - * Note that it doesn't work for the case where multiple paths map to a single IServlet - */ - Collections.sort(servlets, (l1, l2) -> l2.getPaths()[0].length() - l1.getPaths()[0].length()); channel = bind(); } @@ -341,44 +332,7 @@ public class HttpServer { } public IServlet getServlet(FullHttpRequest request) { - String uri = request.uri(); - int i = uri.indexOf('?'); - if (i >= 0) { - uri = uri.substring(0, i); - } - for (IServlet servlet : servlets) { - for (String path : servlet.getPaths()) { - if (match(path, uri)) { - return servlet; - } - } - } - return null; - } - - static boolean match(String pathSpec, String path) { - char c = pathSpec.charAt(0); - if (c == '/') { - if (pathSpec.equals(path) || (pathSpec.length() == 1 && path.isEmpty())) { - return true; - } - if (isPathWildcardMatch(pathSpec, path)) { - return true; - } - } else if (c == '*') { - return path.regionMatches(path.length() - pathSpec.length() + 1, pathSpec, 1, pathSpec.length() - 1); - } - return false; - } - - static boolean isPathWildcardMatch(String pathSpec, String path) { - final int length = pathSpec.length(); - if (length < 2) { - return false; - } - final int cpl = length - 2; - final boolean b = pathSpec.endsWith("/*") && path.regionMatches(0, pathSpec, 0, cpl); - return b && (path.length() == cpl || '/' == path.charAt(cpl)); + return servlets.getServlet(request.uri()); } protected HttpServerHandler<? extends HttpServer> createHttpHandler(int chunkSize) { diff --git a/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/HttpServerHandler.java b/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/HttpServerHandler.java index 4882572..0f4ce8a 100644 --- a/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/HttpServerHandler.java +++ b/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/HttpServerHandler.java @@ -23,6 +23,7 @@ import static org.apache.hyracks.http.server.utils.HttpUtil.X_FORWARDED_PROTO; import java.io.IOException; import java.util.concurrent.Future; import java.util.concurrent.RejectedExecutionException; +import java.util.function.Consumer; import org.apache.hyracks.http.api.IChannelClosedHandler; import org.apache.hyracks.http.api.IServlet; @@ -43,6 +44,7 @@ import io.netty.handler.codec.http.FullHttpRequest; import io.netty.handler.codec.http.HttpHeaderNames; import io.netty.handler.codec.http.HttpHeaderValues; import io.netty.handler.codec.http.HttpRequest; +import io.netty.handler.codec.http.HttpResponse; import io.netty.handler.codec.http.HttpResponseStatus; import io.netty.handler.codec.http.HttpScheme; @@ -119,11 +121,19 @@ public class HttpServerHandler<T extends HttpServer> extends SimpleChannelInboun } protected void respond(ChannelHandlerContext ctx, HttpRequest request, HttpResponseStatus status) { + respond(ctx, request, status, null); + } + + protected void respond(ChannelHandlerContext ctx, HttpRequest request, HttpResponseStatus status, + Consumer<HttpResponse> beforeWrite) { final DefaultHttpResponse response = new DefaultFullHttpResponse(request.protocolVersion(), status); response.headers().setInt(HttpHeaderNames.CONTENT_LENGTH, 0); HttpUtil.setConnectionHeader(request, response); final ChannelPromise responseCompletionPromise = ctx.newPromise(); responseCompletionPromise.addListener(this); + if (beforeWrite != null) { + beforeWrite.accept(response); + } final ChannelFuture clientChannel = ctx.writeAndFlush(response, responseCompletionPromise); if (!io.netty.handler.codec.http.HttpUtil.isKeepAlive(request)) { clientChannel.addListener(ChannelFutureListener.CLOSE); @@ -160,7 +170,8 @@ public class HttpServerHandler<T extends HttpServer> extends SimpleChannelInboun if (LOGGER.isDebugEnabled()) { LOGGER.debug("No servlet for " + request.uri()); } - respond(ctx, request, HttpResponseStatus.NOT_FOUND); + respond(ctx, request, HttpResponseStatus.NOT_FOUND, + response -> response.headers().set(HttpUtil.PERMANENT, "true")); } @Override diff --git a/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/ServletRegistry.java b/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/ServletRegistry.java new file mode 100644 index 0000000..083c0ff --- /dev/null +++ b/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/ServletRegistry.java @@ -0,0 +1,87 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.hyracks.http.server; + +import java.util.Collections; +import java.util.Comparator; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; +import java.util.SortedMap; +import java.util.TreeMap; + +import org.apache.hyracks.http.api.IServlet; +import org.apache.hyracks.http.server.utils.HttpUtil; +import org.apache.hyracks.util.annotations.NotThreadSafe; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; + +@NotThreadSafe // thread safe for concurrent reads, but concurrent registrations are not supported +public class ServletRegistry { + private static final Logger LOGGER = LogManager.getLogger(); + + private final SortedMap<String, IServlet> servletMap = new TreeMap<>(Comparator.reverseOrder()); + private final Set<IServlet> servlets = new HashSet<>(); + + public void register(IServlet let) { + servlets.add(let); + for (String path : let.getPaths()) { + LOGGER.debug("registering servlet {}[{}] with path {}", let, let.getClass().getName(), path); + IServlet prev = servletMap.put(path, let); + if (prev != null) { + throw new IllegalStateException("duplicate servlet mapping! (path = " + path + ", orig = " + prev + "[" + + prev.getClass().getName() + "], dup = " + let + "[" + let.getClass().getName() + "])"); + } + } + } + + public Set<IServlet> getServlets() { + return Collections.unmodifiableSet(servlets); + } + + public IServlet getServlet(String uri) { + String baseUri = HttpUtil.trimQuery(uri); + return servletMap.entrySet().stream().filter(entry -> match(entry.getKey(), baseUri)).map(Map.Entry::getValue) + .findFirst().orElse(null); + } + + static boolean match(String pathSpec, String path) { + char c = pathSpec.charAt(0); + if (c == '/') { + if (pathSpec.equals(path) || (pathSpec.length() == 1 && path.isEmpty())) { + return true; + } + return isPathWildcardMatch(pathSpec, path); + } else if (c == '*') { + return path.regionMatches(path.length() - pathSpec.length() + 1, pathSpec, 1, pathSpec.length() - 1); + } + return false; + } + + static boolean isPathWildcardMatch(String pathSpec, String path) { + final int length = pathSpec.length(); + if (length < 2) { + return false; + } + final int cpl = length - 2; + final boolean b = pathSpec.endsWith("/*") && path.regionMatches(0, pathSpec, 0, cpl); + return b && (path.length() == cpl || '/' == path.charAt(cpl)); + } + +} diff --git a/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/utils/HttpUtil.java b/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/utils/HttpUtil.java index 591cf8c..8451898 100644 --- a/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/utils/HttpUtil.java +++ b/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/utils/HttpUtil.java @@ -53,32 +53,11 @@ public class HttpUtil { private static final Charset DEFAULT_RESPONSE_CHARSET = StandardCharsets.UTF_8; public static final AsciiString X_FORWARDED_PROTO = AsciiString.cached("x-forwarded-proto"); + public static final AsciiString PERMANENT = AsciiString.cached("permanent"); private HttpUtil() { } - public static class Encoding { - public static final String UTF8 = "utf-8"; - - private Encoding() { - } - } - - public static class ContentType { - public static final String APPLICATION_ADM = "application/x-adm"; - public static final String APPLICATION_JSON = "application/json"; - public static final String JSON = "json"; - public static final String APPLICATION_X_WWW_FORM_URLENCODED = "application/x-www-form-urlencoded"; - public static final String CSV = "csv"; - public static final String TEXT_CSV = "text/csv"; - public static final String IMG_PNG = "image/png"; - public static final String TEXT_HTML = "text/html"; - public static final String TEXT_PLAIN = "text/plain"; - - private ContentType() { - } - } - public static String getParameter(Map<String, List<String>> parameters, CharSequence name) { List<String> parameter = parameters.get(String.valueOf(name)); return parameter == null ? null : String.join(",", parameter); @@ -200,6 +179,33 @@ public class HttpUtil { return preferredCharset.orElse(defaultCharset); } + public static String trimQuery(String uri) { + int i = uri.indexOf('?'); + return i < 0 ? uri : uri.substring(0, i); + } + + public static class Encoding { + public static final String UTF8 = "utf-8"; + + private Encoding() { + } + } + + public static class ContentType { + public static final String APPLICATION_ADM = "application/x-adm"; + public static final String APPLICATION_JSON = "application/json"; + public static final String JSON = "json"; + public static final String APPLICATION_X_WWW_FORM_URLENCODED = "application/x-www-form-urlencoded"; + public static final String CSV = "csv"; + public static final String TEXT_CSV = "text/csv"; + public static final String IMG_PNG = "image/png"; + public static final String TEXT_HTML = "text/html"; + public static final String TEXT_PLAIN = "text/plain"; + + private ContentType() { + } + } + private static class WeightedHeaderValue implements Comparable<WeightedHeaderValue> { final String value; diff --git a/hyracks-fullstack/hyracks/hyracks-http/src/test/java/org/apache/hyracks/http/server/PathMatchTest.java b/hyracks-fullstack/hyracks/hyracks-http/src/test/java/org/apache/hyracks/http/server/PathMatchTest.java index 89260c5..9f9e174 100644 --- a/hyracks-fullstack/hyracks/hyracks-http/src/test/java/org/apache/hyracks/http/server/PathMatchTest.java +++ b/hyracks-fullstack/hyracks/hyracks-http/src/test/java/org/apache/hyracks/http/server/PathMatchTest.java @@ -24,18 +24,18 @@ import org.junit.Test; public class PathMatchTest { @Test public void test() { - Assert.assertTrue(HttpServer.match("/", "")); + Assert.assertTrue(ServletRegistry.match("/", "")); - Assert.assertTrue(HttpServer.match("/", "/")); - Assert.assertFalse(HttpServer.match("/", "/a")); + Assert.assertTrue(ServletRegistry.match("/", "/")); + Assert.assertFalse(ServletRegistry.match("/", "/a")); - Assert.assertFalse(HttpServer.match("/a", "/")); - Assert.assertTrue(HttpServer.match("/a", "/a")); + Assert.assertFalse(ServletRegistry.match("/a", "/")); + Assert.assertTrue(ServletRegistry.match("/a", "/a")); - Assert.assertTrue(HttpServer.match("/*", "/")); - Assert.assertTrue(HttpServer.match("/*", "/a")); + Assert.assertTrue(ServletRegistry.match("/*", "/")); + Assert.assertTrue(ServletRegistry.match("/*", "/a")); - Assert.assertFalse(HttpServer.match("/a/*", "/")); - Assert.assertTrue(HttpServer.match("/a/*", "/a")); + Assert.assertFalse(ServletRegistry.match("/a/*", "/")); + Assert.assertTrue(ServletRegistry.match("/a/*", "/a")); } }
