From 6eae2a198f08e65909588877c991276a826a0ede Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Mon, 26 Mar 2018 09:26:38 -0400 Subject: [PATCH 01/18] Fix handling of bad requests Today we have a few problems with how we handle bad requests: - handling requests with bad encoding - handling requests with invalid value for filter_path/pretty/human - handling requests with a garbage Content-Type header There are two problems: - in every case, we give an empty response to the client - in most cases, we leak the byte buffer backing the request! These problems are caused by a broader problem: poor handling preparing the request for handling, or the channel to write to when the response is ready. This commit addresses these issues by taking a unified approach to all of them that ensures that: - we respond to the client with the exception that blew us up - we do not leak the byte buffer backing the request --- .../http/netty4/Netty4HttpChannel.java | 68 ++++++++++------ .../http/netty4/Netty4HttpRequestHandler.java | 78 +++++++++++++++++-- .../http/netty4/Netty4BadRequestTests.java | 78 +++++++++++++++++++ .../rest/Netty4BadRequestIT.java | 36 +++++++++ .../TransportGetFieldMappingsIndexAction.java | 5 ++ .../common/xcontent/ToXContent.java | 22 ++++++ .../rest/AbstractRestChannel.java | 16 +++- .../elasticsearch/rest/BytesRestResponse.java | 54 ++++++++++--- .../org/elasticsearch/rest/RestRequest.java | 7 +- 9 files changed, 319 insertions(+), 45 deletions(-) create mode 100644 modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4BadRequestTests.java diff --git a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpChannel.java b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpChannel.java index 12db47908d1f3..8a9f7b1f1296b 100644 --- a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpChannel.java +++ b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpChannel.java @@ -51,7 +51,9 @@ import java.util.EnumMap; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; +import java.util.function.Supplier; final class Netty4HttpChannel extends AbstractRestChannel { @@ -90,20 +92,32 @@ protected BytesStreamOutput newBytesOutput() { @Override public void sendResponse(RestResponse response) { + sendResponse(channel, transport, nettyRequest, pipelinedRequest, response, threadContext, this::bytesOutputOrNull); + } + + static void sendResponse( + final Channel channel, + final Netty4HttpServerTransport transport, + final FullHttpRequest request, + final HttpPipelinedRequest pipelinedRequest, + final RestResponse response, + final ThreadContext threadContext, + final Supplier bytesStreamOutput) { + Objects.requireNonNull(bytesStreamOutput); // if the response object was created upstream, then use it; // otherwise, create a new one ByteBuf buffer = Netty4Utils.toByteBuf(response.content()); final FullHttpResponse resp; - if (HttpMethod.HEAD.equals(nettyRequest.method())) { - resp = newResponse(Unpooled.EMPTY_BUFFER); + if (HttpMethod.HEAD.equals(request.method())) { + resp = newResponse(request, Unpooled.EMPTY_BUFFER); } else { - resp = newResponse(buffer); + resp = newResponse(request, buffer); } resp.setStatus(getStatus(response.status())); - Netty4CorsHandler.setCorsResponseHeaders(nettyRequest, resp, transport.getCorsConfig()); + Netty4CorsHandler.setCorsResponseHeaders(request, resp, transport.getCorsConfig()); - String opaque = nettyRequest.headers().get("X-Opaque-Id"); + String opaque = request.headers().get("X-Opaque-Id"); if (opaque != null) { setHeaderField(resp, "X-Opaque-Id", opaque); } @@ -114,14 +128,14 @@ public void sendResponse(RestResponse response) { BytesReference content = response.content(); boolean releaseContent = content instanceof Releasable; - boolean releaseBytesStreamOutput = bytesOutputOrNull() instanceof ReleasableBytesStreamOutput; + boolean releaseBytesStreamOutput = bytesStreamOutput.get() instanceof ReleasableBytesStreamOutput; try { // If our response doesn't specify a content-type header, set one setHeaderField(resp, HttpHeaderNames.CONTENT_TYPE.toString(), response.contentType(), false); // If our response has no content-length, calculate and set one setHeaderField(resp, HttpHeaderNames.CONTENT_LENGTH.toString(), String.valueOf(buffer.readableBytes()), false); - addCookies(resp); + addCookies(transport, request, resp); final ChannelPromise promise = channel.newPromise(); @@ -130,10 +144,14 @@ public void sendResponse(RestResponse response) { } if (releaseBytesStreamOutput) { - promise.addListener(f -> bytesOutputOrNull().close()); + promise.addListener(f -> { + final BytesStreamOutput output = bytesStreamOutput.get(); + assert output != null; + output.close(); + }); } - if (isCloseConnection()) { + if (isCloseConnection(request)) { promise.addListener(ChannelFutureListener.CLOSE); } @@ -151,7 +169,9 @@ public void sendResponse(RestResponse response) { ((Releasable) content).close(); } if (releaseBytesStreamOutput) { - bytesOutputOrNull().close(); + final BytesStreamOutput output = bytesStreamOutput.get(); + assert output != null; + output.close(); } if (pipelinedRequest != null) { pipelinedRequest.release(); @@ -159,19 +179,19 @@ public void sendResponse(RestResponse response) { } } - private void setHeaderField(HttpResponse resp, String headerField, String value) { + private static void setHeaderField(HttpResponse resp, String headerField, String value) { setHeaderField(resp, headerField, value, true); } - private void setHeaderField(HttpResponse resp, String headerField, String value, boolean override) { + private static void setHeaderField(HttpResponse resp, String headerField, String value, boolean override) { if (override || !resp.headers().contains(headerField)) { resp.headers().add(headerField, value); } } - private void addCookies(HttpResponse resp) { + private static void addCookies(Netty4HttpServerTransport transport, FullHttpRequest request, FullHttpResponse resp) { if (transport.resetCookies) { - String cookieString = nettyRequest.headers().get(HttpHeaderNames.COOKIE); + String cookieString = request.headers().get(HttpHeaderNames.COOKIE); if (cookieString != null) { Set cookies = ServerCookieDecoder.STRICT.decode(cookieString); if (!cookies.isEmpty()) { @@ -182,7 +202,7 @@ private void addCookies(HttpResponse resp) { } } - private void addCustomHeaders(HttpResponse response, Map> customHeaders) { + private static void addCustomHeaders(HttpResponse response, Map> customHeaders) { if (customHeaders != null) { for (Map.Entry> headerEntry : customHeaders.entrySet()) { for (String headerValue : headerEntry.getValue()) { @@ -193,21 +213,21 @@ private void addCustomHeaders(HttpResponse response, Map> c } // Determine if the request protocol version is HTTP 1.0 - private boolean isHttp10() { - return nettyRequest.protocolVersion().equals(HttpVersion.HTTP_1_0); + private static boolean isHttp10(final FullHttpRequest request) { + return request.protocolVersion().equals(HttpVersion.HTTP_1_0); } // Determine if the request connection should be closed on completion. - private boolean isCloseConnection() { - final boolean http10 = isHttp10(); - return HttpHeaderValues.CLOSE.contentEqualsIgnoreCase(nettyRequest.headers().get(HttpHeaderNames.CONNECTION)) || - (http10 && !HttpHeaderValues.KEEP_ALIVE.contentEqualsIgnoreCase(nettyRequest.headers().get(HttpHeaderNames.CONNECTION))); + private static boolean isCloseConnection(final FullHttpRequest request) { + final boolean http10 = isHttp10(request); + return HttpHeaderValues.CLOSE.contentEqualsIgnoreCase(request.headers().get(HttpHeaderNames.CONNECTION)) || + (http10 && !HttpHeaderValues.KEEP_ALIVE.contentEqualsIgnoreCase(request.headers().get(HttpHeaderNames.CONNECTION))); } // Create a new {@link HttpResponse} to transmit the response for the netty request. - private FullHttpResponse newResponse(ByteBuf buffer) { - final boolean http10 = isHttp10(); - final boolean close = isCloseConnection(); + private static FullHttpResponse newResponse(final FullHttpRequest request, ByteBuf buffer) { + final boolean http10 = isHttp10(request); + final boolean close = isCloseConnection(request); // Build the response object. final HttpResponseStatus status = HttpResponseStatus.OK; // default to initialize final FullHttpResponse response; diff --git a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequestHandler.java b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequestHandler.java index 6da0f5433bae6..d8c86e9947049 100644 --- a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequestHandler.java +++ b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequestHandler.java @@ -25,10 +25,24 @@ import io.netty.channel.SimpleChannelInboundHandler; import io.netty.handler.codec.http.DefaultFullHttpRequest; import io.netty.handler.codec.http.FullHttpRequest; +import org.elasticsearch.common.CheckedSupplier; +import org.elasticsearch.common.io.stream.BytesStreamOutput; +import org.elasticsearch.common.io.stream.ReleasableBytesStreamOutput; import org.elasticsearch.common.util.concurrent.ThreadContext; +import org.elasticsearch.common.xcontent.ToXContent; +import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.http.netty4.pipelining.HttpPipelinedRequest; +import org.elasticsearch.rest.AbstractRestChannel; +import org.elasticsearch.rest.BytesRestResponse; +import org.elasticsearch.rest.RestRequest; import org.elasticsearch.transport.netty4.Netty4Utils; +import java.io.IOException; +import java.util.List; +import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Supplier; + @ChannelHandler.Sharable class Netty4HttpRequestHandler extends SimpleChannelInboundHandler { @@ -67,14 +81,24 @@ protected void channelRead0(ChannelHandlerContext ctx, Object msg) throws Except final Netty4HttpRequest httpRequest; try { httpRequest = new Netty4HttpRequest(serverTransport.xContentRegistry, copy, ctx.channel()); - } catch (Exception ex) { - if (pipelinedRequest != null) { - pipelinedRequest.release(); - } - throw ex; + } catch (final Exception e) { + // we use suppliers here because if acquiring these values blows up, we want error handling in handleException to takeover + final Supplier> contentTypes = () -> request.headers().getAll("Content-Type"); + final Supplier accept = () -> request.headers().get("Accept"); + handleException(e, ctx, copy, pipelinedRequest, request.uri(), ToXContent.EMPTY_PARAMS, contentTypes, accept); + return; + } + + final Netty4HttpChannel channel; + try { + channel = new Netty4HttpChannel(serverTransport, httpRequest, pipelinedRequest, detailedErrorsEnabled, threadContext); + } catch (final Exception e) { + // we use suppliers here because if acquiring these values blows up, we want error handling in handleException to takeover + final Supplier> contentTypes = () -> httpRequest.getAllHeaderValues("Content-Type"); + final Supplier accept = () -> httpRequest.header("Accept"); + handleException(e, ctx, copy, pipelinedRequest, httpRequest.rawPath(), httpRequest, contentTypes, accept); + return; } - final Netty4HttpChannel channel = - new Netty4HttpChannel(serverTransport, httpRequest, pipelinedRequest, detailedErrorsEnabled, threadContext); if (request.decoderResult().isSuccess()) { serverTransport.dispatchRequest(httpRequest, channel); @@ -84,6 +108,46 @@ protected void channelRead0(ChannelHandlerContext ctx, Object msg) throws Except } } + private void handleException( + final Exception e, + final ChannelHandlerContext ctx, + final FullHttpRequest copy, + final HttpPipelinedRequest pipelinedRequest, + final String rawPath, + final ToXContent.Params params, + final Supplier> contentType, + final Supplier accept) throws IOException { + // we failed to construct a channel so we use direct access to underlying channel to write a response to the client + boolean success = false; + try { + final AtomicReference bytesStreamOutput = new AtomicReference<>(); + final Supplier bytesOutput = () -> { + if (bytesStreamOutput.get() == null) { + bytesStreamOutput.set(new ReleasableBytesStreamOutput(serverTransport.bigArrays)); + } + return bytesStreamOutput.get(); + }; + final XContentType requestContentType; + XContentType parsedRequestContentType; + try { + parsedRequestContentType = RestRequest.parseContentType(contentType.get()); + } catch (final IllegalArgumentException inner) { + e.addSuppressed(inner); + parsedRequestContentType = null; + } + requestContentType = parsedRequestContentType; + final CheckedSupplier supplier = + () -> AbstractRestChannel.newBuilder(requestContentType, false, null, accept.get(), false, false, bytesOutput); + final BytesRestResponse response = new BytesRestResponse(params, rawPath, supplier, detailedErrorsEnabled, e); + Netty4HttpChannel.sendResponse(ctx.channel(), serverTransport, copy, pipelinedRequest, response, threadContext, bytesOutput); + success = true; + } finally { + if (success == false && pipelinedRequest != null) { + pipelinedRequest.release(); + } + } + } + @Override public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws Exception { Netty4Utils.maybeDie(cause); diff --git a/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4BadRequestTests.java b/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4BadRequestTests.java new file mode 100644 index 0000000000000..08ae318afbbea --- /dev/null +++ b/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4BadRequestTests.java @@ -0,0 +1,78 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch 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.elasticsearch.http.netty4; + +import io.netty.handler.codec.http.FullHttpResponse; +import org.elasticsearch.common.network.NetworkService; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.transport.TransportAddress; +import org.elasticsearch.common.util.MockBigArrays; +import org.elasticsearch.common.util.MockPageCacheRecycler; +import org.elasticsearch.http.HttpServerTransport; +import org.elasticsearch.http.NullDispatcher; +import org.elasticsearch.indices.breaker.NoneCircuitBreakerService; +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.threadpool.TestThreadPool; +import org.elasticsearch.threadpool.ThreadPool; +import org.junit.After; +import org.junit.Before; + +import java.util.Collection; +import java.util.Collections; + +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.hasSize; + +public class Netty4BadRequestTests extends ESTestCase { + + private NetworkService networkService; + private MockBigArrays bigArrays; + private ThreadPool threadPool; + + @Before + public void setup() throws Exception { + networkService = new NetworkService(Collections.emptyList()); + bigArrays = new MockBigArrays(new MockPageCacheRecycler(Settings.EMPTY), new NoneCircuitBreakerService()); + threadPool = new TestThreadPool("test"); + } + + @After + public void shutdown() throws Exception { + terminate(threadPool); + } + + public void testBadParameterEncoding() throws Exception { + final HttpServerTransport.Dispatcher dispatcher = new NullDispatcher(); + try (HttpServerTransport httpServerTransport = + new Netty4HttpServerTransport(Settings.EMPTY, networkService, bigArrays, threadPool, xContentRegistry(), dispatcher)) { + httpServerTransport.start(); + final TransportAddress transportAddress = randomFrom(httpServerTransport.boundAddress().boundAddresses()); + + try (Netty4HttpClient nettyHttpClient = new Netty4HttpClient()) { + final Collection responses = + nettyHttpClient.get(transportAddress.address(), "/_cluster/settings?pretty=%"); + final Collection responseBodies = Netty4HttpClient.returnHttpResponseBodies(responses); + assertThat(responseBodies, hasSize(1)); + assertThat(responseBodies.iterator().next(), containsString("unterminated escape sequence at end of string: %")); + } + } + } + +} diff --git a/modules/transport-netty4/src/test/java/org/elasticsearch/rest/Netty4BadRequestIT.java b/modules/transport-netty4/src/test/java/org/elasticsearch/rest/Netty4BadRequestIT.java index ae2449d2820d1..50b7cd4f4a54a 100644 --- a/modules/transport-netty4/src/test/java/org/elasticsearch/rest/Netty4BadRequestIT.java +++ b/modules/transport-netty4/src/test/java/org/elasticsearch/rest/Netty4BadRequestIT.java @@ -19,6 +19,7 @@ package org.elasticsearch.rest; +import org.apache.http.message.BasicHeader; import org.elasticsearch.client.Response; import org.elasticsearch.client.ResponseException; import org.elasticsearch.common.settings.Setting; @@ -74,4 +75,39 @@ public void testBadRequest() throws IOException { assertThat(e, hasToString(containsString("too_long_frame_exception"))); assertThat(e, hasToString(matches("An HTTP line is larger than \\d+ bytes"))); } + + public void testBadParameterEncoding() throws IOException { + final Response response = client().performRequest("GET", "/_cluster/settings?pretty%", Collections.emptyMap()); + // final Response response = client().performRequest("GET", "/_cluster/settings", Collections.singletonMap("pretty%", "")); + assertThat(response.getStatusLine().getStatusCode(), equalTo(400)); + final ObjectPath objectPath = ObjectPath.createFromResponse(response); + final Map map = objectPath.evaluate("error"); + assertThat(map.get("type"), equalTo("illegal_argument_exception")); + assertThat(map.get("reason"), equalTo("unterminated escape sequence at end of string: pretty%")); + } + + public void testInvalidParameterValue() throws IOException { + final ResponseException e = expectThrows( + ResponseException.class, + () -> client().performRequest("GET", "/_cluster/settings", Collections.singletonMap("pretty", "neither-true-nor-false"))); + final Response response = e.getResponse(); + assertThat(response.getStatusLine().getStatusCode(), equalTo(400)); + final ObjectPath objectPath = ObjectPath.createFromResponse(response); + final Map map = objectPath.evaluate("error"); + assertThat(map.get("type"), equalTo("illegal_argument_exception")); + assertThat(map.get("reason"), equalTo("Failed to parse value [neither-true-nor-false] as only [true] or [false] are allowed.")); + } + + public void testInvalidHeaderValue() throws IOException { + final BasicHeader header = new BasicHeader("Content-Type", "\t"); + final ResponseException e = + expectThrows(ResponseException.class, () -> client().performRequest("GET", "/_cluster/settings", header)); + final Response response = e.getResponse(); + assertThat(response.getStatusLine().getStatusCode(), equalTo(400)); + final ObjectPath objectPath = ObjectPath.createFromResponse(response); + final Map map = objectPath.evaluate("error"); + assertThat(map.get("type"), equalTo("illegal_argument_exception")); + assertThat(map.get("reason"), equalTo("invalid Content-Type header []")); + } + } diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/get/TransportGetFieldMappingsIndexAction.java b/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/get/TransportGetFieldMappingsIndexAction.java index e9551e6e69d01..c424ded1a0cb4 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/get/TransportGetFieldMappingsIndexAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/get/TransportGetFieldMappingsIndexAction.java @@ -166,6 +166,11 @@ public Boolean paramAsBoolean(String key, Boolean defaultValue) { } return defaultValue; } + + @Override + public Map toMap() { + return Collections.singletonMap("include_defaults", "true"); + } }; private static Map findFieldMappingsByType(Predicate fieldPredicate, diff --git a/server/src/main/java/org/elasticsearch/common/xcontent/ToXContent.java b/server/src/main/java/org/elasticsearch/common/xcontent/ToXContent.java index f74bdec17a9f6..4324ec647ad38 100644 --- a/server/src/main/java/org/elasticsearch/common/xcontent/ToXContent.java +++ b/server/src/main/java/org/elasticsearch/common/xcontent/ToXContent.java @@ -20,6 +20,9 @@ package org.elasticsearch.common.xcontent; import java.io.IOException; +import java.util.Collections; +import java.util.HashMap; +import java.util.Locale; import java.util.Map; /** @@ -37,6 +40,8 @@ interface Params { boolean paramAsBoolean(String key, boolean defaultValue); Boolean paramAsBoolean(String key, Boolean defaultValue); + + Map toMap(); } Params EMPTY_PARAMS = new Params() { @@ -60,6 +65,10 @@ public Boolean paramAsBoolean(String key, Boolean defaultValue) { return defaultValue; } + @Override + public Map toMap() { + return Collections.emptyMap(); + } }; class MapParams implements Params { @@ -93,6 +102,11 @@ public boolean paramAsBoolean(String key, boolean defaultValue) { public Boolean paramAsBoolean(String key, Boolean defaultValue) { return Booleans.parseBoolean(param(key), defaultValue); } + + @Override + public Map toMap() { + return Collections.unmodifiableMap(params); + } } class DelegatingMapParams extends MapParams { @@ -123,6 +137,14 @@ public boolean paramAsBoolean(String key, boolean defaultValue) { public Boolean paramAsBoolean(String key, Boolean defaultValue) { return super.paramAsBoolean(key, delegate.paramAsBoolean(key, defaultValue)); } + + @Override + public Map toMap() { + final Map map = new HashMap<>(); + map.putAll(super.toMap()); + map.putAll(delegate.toMap()); + return map; + } } XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException; diff --git a/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java b/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java index 6c84c1bb963fe..ce9614f0d80fe 100644 --- a/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java +++ b/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java @@ -31,6 +31,7 @@ import java.util.Collections; import java.util.Set; import java.util.function.Predicate; +import java.util.function.Supplier; import static java.util.stream.Collectors.toSet; @@ -77,6 +78,17 @@ public XContentBuilder newErrorBuilder() throws IOException { */ @Override public XContentBuilder newBuilder(@Nullable XContentType requestContentType, boolean useFiltering) throws IOException { + return newBuilder(requestContentType, useFiltering, filterPath, format, pretty, human, this::bytesOutput); + } + + public static XContentBuilder newBuilder( + final @Nullable XContentType requestContentType, + final boolean useFiltering, + final String filterPath, + final String format, + final boolean pretty, + final boolean human, + final Supplier bytesOutput) throws IOException { // try to determine the response content type from the media type or the format query string parameter, with the format parameter // taking precedence over the Accept header XContentType responseContentType = XContentType.fromMediaTypeOrFormat(format); @@ -99,9 +111,9 @@ public XContentBuilder newBuilder(@Nullable XContentType requestContentType, boo excludes = filters.stream().filter(EXCLUDE_FILTER).map(f -> f.substring(1)).collect(toSet()); } - OutputStream unclosableOutputStream = Streams.flushOnCloseStream(bytesOutput()); + OutputStream unclosableOutputStream = Streams.flushOnCloseStream(bytesOutput.get()); XContentBuilder builder = - new XContentBuilder(XContentFactory.xContent(responseContentType), unclosableOutputStream, includes, excludes); + new XContentBuilder(XContentFactory.xContent(responseContentType), unclosableOutputStream, includes, excludes); if (pretty) { builder.prettyPrint().lfAtEnd(); } diff --git a/server/src/main/java/org/elasticsearch/rest/BytesRestResponse.java b/server/src/main/java/org/elasticsearch/rest/BytesRestResponse.java index f8575b4a0127e..2bde6c692a1ce 100644 --- a/server/src/main/java/org/elasticsearch/rest/BytesRestResponse.java +++ b/server/src/main/java/org/elasticsearch/rest/BytesRestResponse.java @@ -25,6 +25,7 @@ import org.elasticsearch.ElasticsearchException; import org.elasticsearch.ElasticsearchStatusException; import org.elasticsearch.ExceptionsHelper; +import org.elasticsearch.common.CheckedSupplier; import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.logging.ESLoggerFactory; @@ -92,13 +93,32 @@ public BytesRestResponse(RestChannel channel, Exception e) throws IOException { } public BytesRestResponse(RestChannel channel, RestStatus status, Exception e) throws IOException { + this(channel.request(), channel.request().rawPath(), channel::newErrorBuilder, channel.detailedErrorsEnabled(), status, e); + } + + public BytesRestResponse( + final ToXContent.Params params, + final String rawPath, + final CheckedSupplier supplier, + final boolean detailedErrorsEnabled, + final Exception e) throws IOException { + this(params, rawPath, supplier, detailedErrorsEnabled, ExceptionsHelper.status(e), e); + } + + private BytesRestResponse( + final ToXContent.Params params, + final String rawPath, + final CheckedSupplier supplier, + final boolean detailedErrorsEnabled, + final RestStatus status, + final Exception e) throws IOException { this.status = status; - try (XContentBuilder builder = build(channel, status, e)) { + try (XContentBuilder builder = build(params, rawPath, supplier, detailedErrorsEnabled, status, e)) { this.content = BytesReference.bytes(builder); this.contentType = builder.contentType().mediaType(); } if (e instanceof ElasticsearchException) { - copyHeaders(((ElasticsearchException) e)); + copyHeaders((ElasticsearchException) e); } } @@ -119,13 +139,22 @@ public RestStatus status() { private static final Logger SUPPRESSED_ERROR_LOGGER = ESLoggerFactory.getLogger("rest.suppressed"); - private static XContentBuilder build(RestChannel channel, RestStatus status, Exception e) throws IOException { - ToXContent.Params params = channel.request(); + private static XContentBuilder build( + final ToXContent.Params params, + final String rawPath, + final CheckedSupplier supplier, + final boolean detailedErrorsEnabled, + final RestStatus status, + final Exception e) throws IOException { + final ToXContent.Params actualParams; if (params.paramAsBoolean("error_trace", !REST_EXCEPTION_SKIP_STACK_TRACE_DEFAULT)) { - params = new ToXContent.DelegatingMapParams(singletonMap(REST_EXCEPTION_SKIP_STACK_TRACE, "false"), params); - } else if (e != null) { - Supplier messageSupplier = () -> new ParameterizedMessage("path: {}, params: {}", - channel.request().rawPath(), channel.request().params()); + actualParams = new ToXContent.DelegatingMapParams(singletonMap(REST_EXCEPTION_SKIP_STACK_TRACE, "false"), params); + } else { + actualParams = params; + } + + if (e != null) { + Supplier messageSupplier = () -> new ParameterizedMessage("path: {}, params: {}", rawPath, actualParams.toMap()); if (status.getStatus() < 500) { SUPPRESSED_ERROR_LOGGER.debug(messageSupplier, e); @@ -134,9 +163,12 @@ private static XContentBuilder build(RestChannel channel, RestStatus status, Exc } } - XContentBuilder builder = channel.newErrorBuilder().startObject(); - ElasticsearchException.generateFailureXContent(builder, params, e, channel.detailedErrorsEnabled()); - builder.field(STATUS, status.getStatus()); + final XContentBuilder builder = supplier.get(); + builder.startObject(); + { + ElasticsearchException.generateFailureXContent(builder, actualParams, e, detailedErrorsEnabled); + builder.field(STATUS, status.getStatus()); + } builder.endObject(); return builder; } diff --git a/server/src/main/java/org/elasticsearch/rest/RestRequest.java b/server/src/main/java/org/elasticsearch/rest/RestRequest.java index e5b3cfa67e5a9..d2608686fbd5b 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestRequest.java +++ b/server/src/main/java/org/elasticsearch/rest/RestRequest.java @@ -306,6 +306,11 @@ public Boolean paramAsBoolean(String key, Boolean defaultValue) { return Booleans.parseBoolean(param(key), defaultValue); } + @Override + public Map toMap() { + return Collections.unmodifiableMap(params); + } + public TimeValue paramAsTime(String key, TimeValue defaultValue) { return parseTimeValue(param(key), defaultValue, key); } @@ -423,7 +428,7 @@ public final Tuple contentOrSourceParam() { * Parses the given content type string for the media type. This method currently ignores parameters. */ // TODO stop ignoring parameters such as charset... - private static XContentType parseContentType(List header) { + public static XContentType parseContentType(List header) { if (header == null || header.isEmpty()) { return null; } else if (header.size() > 1) { From 838847e7ae9b8c16415db01f7bc8489ed22d8cb4 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Mon, 26 Mar 2018 11:51:46 -0400 Subject: [PATCH 02/18] Wrap accept too --- .../http/netty4/Netty4HttpRequestHandler.java | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequestHandler.java b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequestHandler.java index d8c86e9947049..b46df9996599b 100644 --- a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequestHandler.java +++ b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequestHandler.java @@ -127,17 +127,24 @@ private void handleException( } return bytesStreamOutput.get(); }; - final XContentType requestContentType; XContentType parsedRequestContentType; try { parsedRequestContentType = RestRequest.parseContentType(contentType.get()); - } catch (final IllegalArgumentException inner) { + } catch (final Exception inner) { e.addSuppressed(inner); parsedRequestContentType = null; } - requestContentType = parsedRequestContentType; + final XContentType requestContentType = parsedRequestContentType; + String parsedAcceptHeader; + try { + parsedAcceptHeader = accept.get(); + } catch (final Exception inner) { + e.addSuppressed(inner); + parsedAcceptHeader = null; + } + final String acceptHeader = parsedAcceptHeader; final CheckedSupplier supplier = - () -> AbstractRestChannel.newBuilder(requestContentType, false, null, accept.get(), false, false, bytesOutput); + () -> AbstractRestChannel.newBuilder(requestContentType, false, null, acceptHeader, false, false, bytesOutput); final BytesRestResponse response = new BytesRestResponse(params, rawPath, supplier, detailedErrorsEnabled, e); Netty4HttpChannel.sendResponse(ctx.channel(), serverTransport, copy, pipelinedRequest, response, threadContext, bytesOutput); success = true; From c5414f9899fdcdb8ce6d0146aa90198260cab1ad Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Mon, 26 Mar 2018 11:53:30 -0400 Subject: [PATCH 03/18] Remove leftover test --- .../org/elasticsearch/rest/Netty4BadRequestIT.java | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/modules/transport-netty4/src/test/java/org/elasticsearch/rest/Netty4BadRequestIT.java b/modules/transport-netty4/src/test/java/org/elasticsearch/rest/Netty4BadRequestIT.java index 50b7cd4f4a54a..d01087ad3fd57 100644 --- a/modules/transport-netty4/src/test/java/org/elasticsearch/rest/Netty4BadRequestIT.java +++ b/modules/transport-netty4/src/test/java/org/elasticsearch/rest/Netty4BadRequestIT.java @@ -76,16 +76,6 @@ public void testBadRequest() throws IOException { assertThat(e, hasToString(matches("An HTTP line is larger than \\d+ bytes"))); } - public void testBadParameterEncoding() throws IOException { - final Response response = client().performRequest("GET", "/_cluster/settings?pretty%", Collections.emptyMap()); - // final Response response = client().performRequest("GET", "/_cluster/settings", Collections.singletonMap("pretty%", "")); - assertThat(response.getStatusLine().getStatusCode(), equalTo(400)); - final ObjectPath objectPath = ObjectPath.createFromResponse(response); - final Map map = objectPath.evaluate("error"); - assertThat(map.get("type"), equalTo("illegal_argument_exception")); - assertThat(map.get("reason"), equalTo("unterminated escape sequence at end of string: pretty%")); - } - public void testInvalidParameterValue() throws IOException { final ResponseException e = expectThrows( ResponseException.class, From ec9931bb0e193021e34a84ea31efe050785bdcbd Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Mon, 26 Mar 2018 12:29:21 -0400 Subject: [PATCH 04/18] Stricter testing --- .../elasticsearch/http/netty4/Netty4BadRequestTests.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4BadRequestTests.java b/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4BadRequestTests.java index 08ae318afbbea..ff00cca247c98 100644 --- a/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4BadRequestTests.java +++ b/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4BadRequestTests.java @@ -38,6 +38,7 @@ import java.util.Collections; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasSize; public class Netty4BadRequestTests extends ESTestCase { @@ -68,9 +69,12 @@ public void testBadParameterEncoding() throws Exception { try (Netty4HttpClient nettyHttpClient = new Netty4HttpClient()) { final Collection responses = nettyHttpClient.get(transportAddress.address(), "/_cluster/settings?pretty=%"); + assertThat(responses, hasSize(1)); + assertThat(responses.iterator().next().status().code(), equalTo(400)); final Collection responseBodies = Netty4HttpClient.returnHttpResponseBodies(responses); assertThat(responseBodies, hasSize(1)); - assertThat(responseBodies.iterator().next(), containsString("unterminated escape sequence at end of string: %")); + assertThat(responseBodies.iterator().next(), containsString("\"type\":\"illegal_argument_exception\"")); + assertThat(responseBodies.iterator().next(), containsString("\"reason\":\"unterminated escape sequence at end of string: %\"")); } } } From aaf0259f902c7ebd29a087593c7cd86cdb62b6b6 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Mon, 26 Mar 2018 12:31:36 -0400 Subject: [PATCH 05/18] Fix comment, takeover is not a verb --- .../org/elasticsearch/http/netty4/Netty4HttpRequestHandler.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequestHandler.java b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequestHandler.java index b46df9996599b..90e03a96efa33 100644 --- a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequestHandler.java +++ b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequestHandler.java @@ -82,7 +82,7 @@ protected void channelRead0(ChannelHandlerContext ctx, Object msg) throws Except try { httpRequest = new Netty4HttpRequest(serverTransport.xContentRegistry, copy, ctx.channel()); } catch (final Exception e) { - // we use suppliers here because if acquiring these values blows up, we want error handling in handleException to takeover + // we use suppliers here because if acquiring these values blows up, we want error handling in handleException to take over final Supplier> contentTypes = () -> request.headers().getAll("Content-Type"); final Supplier accept = () -> request.headers().get("Accept"); handleException(e, ctx, copy, pipelinedRequest, request.uri(), ToXContent.EMPTY_PARAMS, contentTypes, accept); From 37db37735f3ac35e43dbeccc61c4dbdb1ec280d7 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Mon, 26 Mar 2018 14:07:37 -0400 Subject: [PATCH 06/18] Fix another comment :( --- .../org/elasticsearch/http/netty4/Netty4HttpRequestHandler.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequestHandler.java b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequestHandler.java index 90e03a96efa33..2da28ecccc4a6 100644 --- a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequestHandler.java +++ b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequestHandler.java @@ -93,7 +93,7 @@ protected void channelRead0(ChannelHandlerContext ctx, Object msg) throws Except try { channel = new Netty4HttpChannel(serverTransport, httpRequest, pipelinedRequest, detailedErrorsEnabled, threadContext); } catch (final Exception e) { - // we use suppliers here because if acquiring these values blows up, we want error handling in handleException to takeover + // we use suppliers here because if acquiring these values blows up, we want error handling in handleException to take over final Supplier> contentTypes = () -> httpRequest.getAllHeaderValues("Content-Type"); final Supplier accept = () -> httpRequest.header("Accept"); handleException(e, ctx, copy, pipelinedRequest, httpRequest.rawPath(), httpRequest, contentTypes, accept); From 974736b9af99f515feef3c480cf64d8d113c92b1 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Mon, 26 Mar 2018 14:08:14 -0400 Subject: [PATCH 07/18] Fix checkstyle --- .../org/elasticsearch/http/netty4/Netty4BadRequestTests.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4BadRequestTests.java b/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4BadRequestTests.java index ff00cca247c98..e847849763ff5 100644 --- a/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4BadRequestTests.java +++ b/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4BadRequestTests.java @@ -74,7 +74,9 @@ public void testBadParameterEncoding() throws Exception { final Collection responseBodies = Netty4HttpClient.returnHttpResponseBodies(responses); assertThat(responseBodies, hasSize(1)); assertThat(responseBodies.iterator().next(), containsString("\"type\":\"illegal_argument_exception\"")); - assertThat(responseBodies.iterator().next(), containsString("\"reason\":\"unterminated escape sequence at end of string: %\"")); + assertThat( + responseBodies.iterator().next(), + containsString("\"reason\":\"unterminated escape sequence at end of string: %\"")); } } } From 696c00b11004d98e227de1613441fb976b1a8a76 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Mon, 26 Mar 2018 22:30:19 -0400 Subject: [PATCH 08/18] Simpler solution --- .../http/netty4/Netty4HttpChannel.java | 68 ++++----- .../http/netty4/Netty4HttpRequest.java | 58 +++++++- .../http/netty4/Netty4HttpRequestHandler.java | 134 +++++++++--------- .../http/netty4/Netty4BadRequestTests.java | 26 +++- .../http/netty4/Netty4HttpChannelTests.java | 12 +- .../TransportGetFieldMappingsIndexAction.java | 5 - .../common/xcontent/ToXContent.java | 22 --- .../rest/AbstractRestChannel.java | 23 ++- .../elasticsearch/rest/BytesRestResponse.java | 54 ++----- .../org/elasticsearch/rest/RestRequest.java | 47 ++---- .../test/rest/FakeRestRequest.java | 2 +- 11 files changed, 209 insertions(+), 242 deletions(-) diff --git a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpChannel.java b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpChannel.java index 8a9f7b1f1296b..12db47908d1f3 100644 --- a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpChannel.java +++ b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpChannel.java @@ -51,9 +51,7 @@ import java.util.EnumMap; import java.util.List; import java.util.Map; -import java.util.Objects; import java.util.Set; -import java.util.function.Supplier; final class Netty4HttpChannel extends AbstractRestChannel { @@ -92,32 +90,20 @@ protected BytesStreamOutput newBytesOutput() { @Override public void sendResponse(RestResponse response) { - sendResponse(channel, transport, nettyRequest, pipelinedRequest, response, threadContext, this::bytesOutputOrNull); - } - - static void sendResponse( - final Channel channel, - final Netty4HttpServerTransport transport, - final FullHttpRequest request, - final HttpPipelinedRequest pipelinedRequest, - final RestResponse response, - final ThreadContext threadContext, - final Supplier bytesStreamOutput) { - Objects.requireNonNull(bytesStreamOutput); // if the response object was created upstream, then use it; // otherwise, create a new one ByteBuf buffer = Netty4Utils.toByteBuf(response.content()); final FullHttpResponse resp; - if (HttpMethod.HEAD.equals(request.method())) { - resp = newResponse(request, Unpooled.EMPTY_BUFFER); + if (HttpMethod.HEAD.equals(nettyRequest.method())) { + resp = newResponse(Unpooled.EMPTY_BUFFER); } else { - resp = newResponse(request, buffer); + resp = newResponse(buffer); } resp.setStatus(getStatus(response.status())); - Netty4CorsHandler.setCorsResponseHeaders(request, resp, transport.getCorsConfig()); + Netty4CorsHandler.setCorsResponseHeaders(nettyRequest, resp, transport.getCorsConfig()); - String opaque = request.headers().get("X-Opaque-Id"); + String opaque = nettyRequest.headers().get("X-Opaque-Id"); if (opaque != null) { setHeaderField(resp, "X-Opaque-Id", opaque); } @@ -128,14 +114,14 @@ static void sendResponse( BytesReference content = response.content(); boolean releaseContent = content instanceof Releasable; - boolean releaseBytesStreamOutput = bytesStreamOutput.get() instanceof ReleasableBytesStreamOutput; + boolean releaseBytesStreamOutput = bytesOutputOrNull() instanceof ReleasableBytesStreamOutput; try { // If our response doesn't specify a content-type header, set one setHeaderField(resp, HttpHeaderNames.CONTENT_TYPE.toString(), response.contentType(), false); // If our response has no content-length, calculate and set one setHeaderField(resp, HttpHeaderNames.CONTENT_LENGTH.toString(), String.valueOf(buffer.readableBytes()), false); - addCookies(transport, request, resp); + addCookies(resp); final ChannelPromise promise = channel.newPromise(); @@ -144,14 +130,10 @@ static void sendResponse( } if (releaseBytesStreamOutput) { - promise.addListener(f -> { - final BytesStreamOutput output = bytesStreamOutput.get(); - assert output != null; - output.close(); - }); + promise.addListener(f -> bytesOutputOrNull().close()); } - if (isCloseConnection(request)) { + if (isCloseConnection()) { promise.addListener(ChannelFutureListener.CLOSE); } @@ -169,9 +151,7 @@ static void sendResponse( ((Releasable) content).close(); } if (releaseBytesStreamOutput) { - final BytesStreamOutput output = bytesStreamOutput.get(); - assert output != null; - output.close(); + bytesOutputOrNull().close(); } if (pipelinedRequest != null) { pipelinedRequest.release(); @@ -179,19 +159,19 @@ static void sendResponse( } } - private static void setHeaderField(HttpResponse resp, String headerField, String value) { + private void setHeaderField(HttpResponse resp, String headerField, String value) { setHeaderField(resp, headerField, value, true); } - private static void setHeaderField(HttpResponse resp, String headerField, String value, boolean override) { + private void setHeaderField(HttpResponse resp, String headerField, String value, boolean override) { if (override || !resp.headers().contains(headerField)) { resp.headers().add(headerField, value); } } - private static void addCookies(Netty4HttpServerTransport transport, FullHttpRequest request, FullHttpResponse resp) { + private void addCookies(HttpResponse resp) { if (transport.resetCookies) { - String cookieString = request.headers().get(HttpHeaderNames.COOKIE); + String cookieString = nettyRequest.headers().get(HttpHeaderNames.COOKIE); if (cookieString != null) { Set cookies = ServerCookieDecoder.STRICT.decode(cookieString); if (!cookies.isEmpty()) { @@ -202,7 +182,7 @@ private static void addCookies(Netty4HttpServerTransport transport, FullHttpRequ } } - private static void addCustomHeaders(HttpResponse response, Map> customHeaders) { + private void addCustomHeaders(HttpResponse response, Map> customHeaders) { if (customHeaders != null) { for (Map.Entry> headerEntry : customHeaders.entrySet()) { for (String headerValue : headerEntry.getValue()) { @@ -213,21 +193,21 @@ private static void addCustomHeaders(HttpResponse response, Map params(FullHttpRequest request) { + final String uri = request.uri(); + final Map params = new HashMap<>(); + int index = uri.indexOf('?'); + if (index >= 0) { + RestUtils.decodeQueryString(uri, index + 1, params); + } + return params; + } + + private static String uri(FullHttpRequest request) { + final String uri = request.uri(); + final int index = uri.indexOf('?'); + if (index >= 0) { + return uri.substring(0, index); + } else { + return uri; + } + } + + /** + * Construct a new request. In contrast to + * {@link Netty4HttpRequest#Netty4HttpRequest(XContentType, NamedXContentRegistry, Map, String, FullHttpRequest, Channel)}, the URI is + * not decoded so this method constructor should not throw any exceptions. + * + * @param xContentType the content type + * @param xContentRegistry the content registry + * @param params the parameters for the request + * @param uri the path for the request + * @param request the underlying request + * @param channel the channel for the request + */ + Netty4HttpRequest( + final XContentType xContentType, + final NamedXContentRegistry xContentRegistry, + final Map params, + final String uri, + final FullHttpRequest request, + final Channel channel) { + super(xContentType, xContentRegistry, params, uri, new HttpHeadersMap(request.headers())); this.request = request; this.channel = channel; if (request.content().isReadable()) { diff --git a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequestHandler.java b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequestHandler.java index 2da28ecccc4a6..84fe61997faf2 100644 --- a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequestHandler.java +++ b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequestHandler.java @@ -20,16 +20,20 @@ package org.elasticsearch.http.netty4; import io.netty.buffer.Unpooled; +import io.netty.channel.Channel; import io.netty.channel.ChannelHandler; import io.netty.channel.ChannelHandlerContext; import io.netty.channel.SimpleChannelInboundHandler; import io.netty.handler.codec.http.DefaultFullHttpRequest; import io.netty.handler.codec.http.FullHttpRequest; +import org.elasticsearch.ExceptionsHelper; import org.elasticsearch.common.CheckedSupplier; import org.elasticsearch.common.io.stream.BytesStreamOutput; import org.elasticsearch.common.io.stream.ReleasableBytesStreamOutput; import org.elasticsearch.common.util.concurrent.ThreadContext; +import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.common.xcontent.ToXContent; +import org.elasticsearch.common.xcontent.XContent; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.http.netty4.pipelining.HttpPipelinedRequest; @@ -39,7 +43,10 @@ import org.elasticsearch.transport.netty4.Netty4Utils; import java.io.IOException; +import java.util.Collections; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Supplier; @@ -78,80 +85,73 @@ protected void channelRead0(ChannelHandlerContext ctx, Object msg) throws Except Unpooled.copiedBuffer(request.content()), request.headers(), request.trailingHeaders()); - final Netty4HttpRequest httpRequest; - try { - httpRequest = new Netty4HttpRequest(serverTransport.xContentRegistry, copy, ctx.channel()); - } catch (final Exception e) { - // we use suppliers here because if acquiring these values blows up, we want error handling in handleException to take over - final Supplier> contentTypes = () -> request.headers().getAll("Content-Type"); - final Supplier accept = () -> request.headers().get("Accept"); - handleException(e, ctx, copy, pipelinedRequest, request.uri(), ToXContent.EMPTY_PARAMS, contentTypes, accept); - return; - } - - final Netty4HttpChannel channel; - try { - channel = new Netty4HttpChannel(serverTransport, httpRequest, pipelinedRequest, detailedErrorsEnabled, threadContext); - } catch (final Exception e) { - // we use suppliers here because if acquiring these values blows up, we want error handling in handleException to take over - final Supplier> contentTypes = () -> httpRequest.getAllHeaderValues("Content-Type"); - final Supplier accept = () -> httpRequest.header("Accept"); - handleException(e, ctx, copy, pipelinedRequest, httpRequest.rawPath(), httpRequest, contentTypes, accept); - return; - } - if (request.decoderResult().isSuccess()) { - serverTransport.dispatchRequest(httpRequest, channel); - } else { - assert request.decoderResult().isFailure(); - serverTransport.dispatchBadRequest(httpRequest, channel, request.decoderResult().cause()); + Exception badRequestCause = null; + final XContentType xContentType; + { + XContentType innerXContentType; + try { + innerXContentType = RestRequest.parseContentType(request.headers().getAll("Content-Type")); + } catch (final IllegalArgumentException e) { + innerXContentType = null; + badRequestCause = e; + } + xContentType = innerXContentType; } - } - private void handleException( - final Exception e, - final ChannelHandlerContext ctx, - final FullHttpRequest copy, - final HttpPipelinedRequest pipelinedRequest, - final String rawPath, - final ToXContent.Params params, - final Supplier> contentType, - final Supplier accept) throws IOException { - // we failed to construct a channel so we use direct access to underlying channel to write a response to the client - boolean success = false; - try { - final AtomicReference bytesStreamOutput = new AtomicReference<>(); - final Supplier bytesOutput = () -> { - if (bytesStreamOutput.get() == null) { - bytesStreamOutput.set(new ReleasableBytesStreamOutput(serverTransport.bigArrays)); - } - return bytesStreamOutput.get(); - }; - XContentType parsedRequestContentType; + final Netty4HttpRequest httpRequest; + { + Netty4HttpRequest innerHttpRequest; try { - parsedRequestContentType = RestRequest.parseContentType(contentType.get()); - } catch (final Exception inner) { - e.addSuppressed(inner); - parsedRequestContentType = null; + innerHttpRequest = new Netty4HttpRequest(xContentType, serverTransport.xContentRegistry, copy, ctx.channel()); + } catch (final Exception e) { + if (badRequestCause == null) { + badRequestCause = e; + } else { + badRequestCause.addSuppressed(e); + } + innerHttpRequest = + new Netty4HttpRequest( + xContentType, + serverTransport.xContentRegistry, + Collections.emptyMap(), + copy.uri(), + copy, + ctx.channel()); } - final XContentType requestContentType = parsedRequestContentType; - String parsedAcceptHeader; + httpRequest = innerHttpRequest; + } + + final Netty4HttpChannel channel; + { + Netty4HttpChannel innerChannel; try { - parsedAcceptHeader = accept.get(); - } catch (final Exception inner) { - e.addSuppressed(inner); - parsedAcceptHeader = null; - } - final String acceptHeader = parsedAcceptHeader; - final CheckedSupplier supplier = - () -> AbstractRestChannel.newBuilder(requestContentType, false, null, acceptHeader, false, false, bytesOutput); - final BytesRestResponse response = new BytesRestResponse(params, rawPath, supplier, detailedErrorsEnabled, e); - Netty4HttpChannel.sendResponse(ctx.channel(), serverTransport, copy, pipelinedRequest, response, threadContext, bytesOutput); - success = true; - } finally { - if (success == false && pipelinedRequest != null) { - pipelinedRequest.release(); + innerChannel = new Netty4HttpChannel(serverTransport, httpRequest, pipelinedRequest, detailedErrorsEnabled, threadContext); + } catch (final Exception e) { + if (badRequestCause == null) { + badRequestCause = e; + } else { + badRequestCause.addSuppressed(e); + } + final Netty4HttpRequest innerRequest = + new Netty4HttpRequest( + xContentType, + serverTransport.xContentRegistry, + Collections.emptyMap(), + copy.uri(), + copy, + ctx.channel()); + innerChannel = new Netty4HttpChannel(serverTransport, innerRequest, pipelinedRequest, detailedErrorsEnabled, threadContext); } + channel = innerChannel; + } + + if (request.decoderResult().isFailure()) { + serverTransport.dispatchBadRequest(httpRequest, channel, request.decoderResult().cause()); + } else if (badRequestCause != null) { + serverTransport.dispatchBadRequest(httpRequest, channel, badRequestCause); + } else { + serverTransport.dispatchRequest(httpRequest, channel); } } diff --git a/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4BadRequestTests.java b/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4BadRequestTests.java index e847849763ff5..088a25228b4d8 100644 --- a/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4BadRequestTests.java +++ b/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4BadRequestTests.java @@ -20,20 +20,28 @@ package org.elasticsearch.http.netty4; import io.netty.handler.codec.http.FullHttpResponse; +import org.elasticsearch.ElasticsearchException; import org.elasticsearch.common.network.NetworkService; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.transport.TransportAddress; import org.elasticsearch.common.util.MockBigArrays; import org.elasticsearch.common.util.MockPageCacheRecycler; +import org.elasticsearch.common.util.concurrent.ThreadContext; import org.elasticsearch.http.HttpServerTransport; import org.elasticsearch.http.NullDispatcher; import org.elasticsearch.indices.breaker.NoneCircuitBreakerService; +import org.elasticsearch.rest.BytesRestResponse; +import org.elasticsearch.rest.RestChannel; +import org.elasticsearch.rest.RestRequest; +import org.elasticsearch.rest.RestStatus; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.threadpool.TestThreadPool; import org.elasticsearch.threadpool.ThreadPool; import org.junit.After; import org.junit.Before; +import java.io.IOException; +import java.io.UncheckedIOException; import java.util.Collection; import java.util.Collections; @@ -60,7 +68,23 @@ public void shutdown() throws Exception { } public void testBadParameterEncoding() throws Exception { - final HttpServerTransport.Dispatcher dispatcher = new NullDispatcher(); + final HttpServerTransport.Dispatcher dispatcher = new HttpServerTransport.Dispatcher() { + @Override + public void dispatchRequest(RestRequest request, RestChannel channel, ThreadContext threadContext) { + fail(); + } + + @Override + public void dispatchBadRequest(RestRequest request, RestChannel channel, ThreadContext threadContext, Throwable cause) { + try { + final Exception e = cause instanceof Exception ? (Exception) cause : new ElasticsearchException(cause); + channel.sendResponse(new BytesRestResponse(channel, RestStatus.BAD_REQUEST, e)); + } catch (final IOException e) { + throw new UncheckedIOException(e); + } + } + }; + try (HttpServerTransport httpServerTransport = new Netty4HttpServerTransport(Settings.EMPTY, networkService, bigArrays, threadPool, xContentRegistry(), dispatcher)) { httpServerTransport.start(); diff --git a/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpChannelTests.java b/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpChannelTests.java index e9de4ef50a5a4..c96143fabe4b1 100644 --- a/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpChannelTests.java +++ b/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpChannelTests.java @@ -55,6 +55,7 @@ import org.elasticsearch.common.util.MockPageCacheRecycler; import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.common.xcontent.json.JsonXContent; import org.elasticsearch.http.HttpTransportSettings; import org.elasticsearch.http.NullDispatcher; @@ -211,7 +212,7 @@ public void testHeadersSet() { final FullHttpRequest httpRequest = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "/"); httpRequest.headers().add(HttpHeaderNames.ORIGIN, "remote"); final WriteCapturingChannel writeCapturingChannel = new WriteCapturingChannel(); - Netty4HttpRequest request = new Netty4HttpRequest(xContentRegistry(), httpRequest, writeCapturingChannel); + Netty4HttpRequest request = new Netty4HttpRequest(XContentType.JSON, xContentRegistry(), httpRequest, writeCapturingChannel); // send a response Netty4HttpChannel channel = @@ -240,7 +241,7 @@ public void testReleaseOnSendToClosedChannel() { new Netty4HttpServerTransport(settings, networkService, bigArrays, threadPool, registry, new NullDispatcher())) { final FullHttpRequest httpRequest = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "/"); final EmbeddedChannel embeddedChannel = new EmbeddedChannel(); - final Netty4HttpRequest request = new Netty4HttpRequest(registry, httpRequest, embeddedChannel); + final Netty4HttpRequest request = new Netty4HttpRequest(XContentType.JSON, registry, httpRequest, embeddedChannel); final HttpPipelinedRequest pipelinedRequest = randomBoolean() ? new HttpPipelinedRequest(request.request(), 1) : null; final Netty4HttpChannel channel = new Netty4HttpChannel(httpServerTransport, request, pipelinedRequest, randomBoolean(), threadPool.getThreadContext()); @@ -259,7 +260,7 @@ public void testReleaseOnSendToChannelAfterException() throws IOException { new Netty4HttpServerTransport(settings, networkService, bigArrays, threadPool, registry, new NullDispatcher())) { final FullHttpRequest httpRequest = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "/"); final EmbeddedChannel embeddedChannel = new EmbeddedChannel(); - final Netty4HttpRequest request = new Netty4HttpRequest(registry, httpRequest, embeddedChannel); + final Netty4HttpRequest request = new Netty4HttpRequest(XContentType.JSON, registry, httpRequest, embeddedChannel); final HttpPipelinedRequest pipelinedRequest = randomBoolean() ? new HttpPipelinedRequest(request.request(), 1) : null; final Netty4HttpChannel channel = new Netty4HttpChannel(httpServerTransport, request, pipelinedRequest, randomBoolean(), threadPool.getThreadContext()); @@ -302,7 +303,7 @@ public void testConnectionClose() throws Exception { } } final EmbeddedChannel embeddedChannel = new EmbeddedChannel(); - final Netty4HttpRequest request = new Netty4HttpRequest(xContentRegistry(), httpRequest, embeddedChannel); + final Netty4HttpRequest request = new Netty4HttpRequest(XContentType.JSON, xContentRegistry(), httpRequest, embeddedChannel); // send a response, the channel close status should match assertTrue(embeddedChannel.isOpen()); @@ -330,7 +331,8 @@ private FullHttpResponse executeRequest(final Settings settings, final String or } httpRequest.headers().add(HttpHeaderNames.HOST, host); final WriteCapturingChannel writeCapturingChannel = new WriteCapturingChannel(); - final Netty4HttpRequest request = new Netty4HttpRequest(xContentRegistry(), httpRequest, writeCapturingChannel); + final Netty4HttpRequest request = + new Netty4HttpRequest(XContentType.JSON, xContentRegistry(), httpRequest, writeCapturingChannel); Netty4HttpChannel channel = new Netty4HttpChannel(httpServerTransport, request, null, randomBoolean(), threadPool.getThreadContext()); diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/get/TransportGetFieldMappingsIndexAction.java b/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/get/TransportGetFieldMappingsIndexAction.java index c424ded1a0cb4..e9551e6e69d01 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/get/TransportGetFieldMappingsIndexAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/get/TransportGetFieldMappingsIndexAction.java @@ -166,11 +166,6 @@ public Boolean paramAsBoolean(String key, Boolean defaultValue) { } return defaultValue; } - - @Override - public Map toMap() { - return Collections.singletonMap("include_defaults", "true"); - } }; private static Map findFieldMappingsByType(Predicate fieldPredicate, diff --git a/server/src/main/java/org/elasticsearch/common/xcontent/ToXContent.java b/server/src/main/java/org/elasticsearch/common/xcontent/ToXContent.java index 4324ec647ad38..f74bdec17a9f6 100644 --- a/server/src/main/java/org/elasticsearch/common/xcontent/ToXContent.java +++ b/server/src/main/java/org/elasticsearch/common/xcontent/ToXContent.java @@ -20,9 +20,6 @@ package org.elasticsearch.common.xcontent; import java.io.IOException; -import java.util.Collections; -import java.util.HashMap; -import java.util.Locale; import java.util.Map; /** @@ -40,8 +37,6 @@ interface Params { boolean paramAsBoolean(String key, boolean defaultValue); Boolean paramAsBoolean(String key, Boolean defaultValue); - - Map toMap(); } Params EMPTY_PARAMS = new Params() { @@ -65,10 +60,6 @@ public Boolean paramAsBoolean(String key, Boolean defaultValue) { return defaultValue; } - @Override - public Map toMap() { - return Collections.emptyMap(); - } }; class MapParams implements Params { @@ -102,11 +93,6 @@ public boolean paramAsBoolean(String key, boolean defaultValue) { public Boolean paramAsBoolean(String key, Boolean defaultValue) { return Booleans.parseBoolean(param(key), defaultValue); } - - @Override - public Map toMap() { - return Collections.unmodifiableMap(params); - } } class DelegatingMapParams extends MapParams { @@ -137,14 +123,6 @@ public boolean paramAsBoolean(String key, boolean defaultValue) { public Boolean paramAsBoolean(String key, Boolean defaultValue) { return super.paramAsBoolean(key, delegate.paramAsBoolean(key, defaultValue)); } - - @Override - public Map toMap() { - final Map map = new HashMap<>(); - map.putAll(super.toMap()); - map.putAll(delegate.toMap()); - return map; - } } XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException; diff --git a/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java b/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java index ce9614f0d80fe..d376b65ef2d88 100644 --- a/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java +++ b/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java @@ -31,7 +31,6 @@ import java.util.Collections; import java.util.Set; import java.util.function.Predicate; -import java.util.function.Supplier; import static java.util.stream.Collectors.toSet; @@ -49,6 +48,13 @@ public abstract class AbstractRestChannel implements RestChannel { private BytesStreamOutput bytesOut; + /** + * Construct a channel for handling the request. + * + * @param request the request + * @param detailedErrorsEnabled if detailed errors should be reported to the channel + * @throws IllegalArgumentException if parsing the pretty or human parameters fails + */ protected AbstractRestChannel(RestRequest request, boolean detailedErrorsEnabled) { this.request = request; this.detailedErrorsEnabled = detailedErrorsEnabled; @@ -78,17 +84,6 @@ public XContentBuilder newErrorBuilder() throws IOException { */ @Override public XContentBuilder newBuilder(@Nullable XContentType requestContentType, boolean useFiltering) throws IOException { - return newBuilder(requestContentType, useFiltering, filterPath, format, pretty, human, this::bytesOutput); - } - - public static XContentBuilder newBuilder( - final @Nullable XContentType requestContentType, - final boolean useFiltering, - final String filterPath, - final String format, - final boolean pretty, - final boolean human, - final Supplier bytesOutput) throws IOException { // try to determine the response content type from the media type or the format query string parameter, with the format parameter // taking precedence over the Accept header XContentType responseContentType = XContentType.fromMediaTypeOrFormat(format); @@ -111,9 +106,9 @@ public static XContentBuilder newBuilder( excludes = filters.stream().filter(EXCLUDE_FILTER).map(f -> f.substring(1)).collect(toSet()); } - OutputStream unclosableOutputStream = Streams.flushOnCloseStream(bytesOutput.get()); + OutputStream unclosableOutputStream = Streams.flushOnCloseStream(bytesOutput()); XContentBuilder builder = - new XContentBuilder(XContentFactory.xContent(responseContentType), unclosableOutputStream, includes, excludes); + new XContentBuilder(XContentFactory.xContent(responseContentType), unclosableOutputStream, includes, excludes); if (pretty) { builder.prettyPrint().lfAtEnd(); } diff --git a/server/src/main/java/org/elasticsearch/rest/BytesRestResponse.java b/server/src/main/java/org/elasticsearch/rest/BytesRestResponse.java index 2bde6c692a1ce..f8575b4a0127e 100644 --- a/server/src/main/java/org/elasticsearch/rest/BytesRestResponse.java +++ b/server/src/main/java/org/elasticsearch/rest/BytesRestResponse.java @@ -25,7 +25,6 @@ import org.elasticsearch.ElasticsearchException; import org.elasticsearch.ElasticsearchStatusException; import org.elasticsearch.ExceptionsHelper; -import org.elasticsearch.common.CheckedSupplier; import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.logging.ESLoggerFactory; @@ -93,32 +92,13 @@ public BytesRestResponse(RestChannel channel, Exception e) throws IOException { } public BytesRestResponse(RestChannel channel, RestStatus status, Exception e) throws IOException { - this(channel.request(), channel.request().rawPath(), channel::newErrorBuilder, channel.detailedErrorsEnabled(), status, e); - } - - public BytesRestResponse( - final ToXContent.Params params, - final String rawPath, - final CheckedSupplier supplier, - final boolean detailedErrorsEnabled, - final Exception e) throws IOException { - this(params, rawPath, supplier, detailedErrorsEnabled, ExceptionsHelper.status(e), e); - } - - private BytesRestResponse( - final ToXContent.Params params, - final String rawPath, - final CheckedSupplier supplier, - final boolean detailedErrorsEnabled, - final RestStatus status, - final Exception e) throws IOException { this.status = status; - try (XContentBuilder builder = build(params, rawPath, supplier, detailedErrorsEnabled, status, e)) { + try (XContentBuilder builder = build(channel, status, e)) { this.content = BytesReference.bytes(builder); this.contentType = builder.contentType().mediaType(); } if (e instanceof ElasticsearchException) { - copyHeaders((ElasticsearchException) e); + copyHeaders(((ElasticsearchException) e)); } } @@ -139,22 +119,13 @@ public RestStatus status() { private static final Logger SUPPRESSED_ERROR_LOGGER = ESLoggerFactory.getLogger("rest.suppressed"); - private static XContentBuilder build( - final ToXContent.Params params, - final String rawPath, - final CheckedSupplier supplier, - final boolean detailedErrorsEnabled, - final RestStatus status, - final Exception e) throws IOException { - final ToXContent.Params actualParams; + private static XContentBuilder build(RestChannel channel, RestStatus status, Exception e) throws IOException { + ToXContent.Params params = channel.request(); if (params.paramAsBoolean("error_trace", !REST_EXCEPTION_SKIP_STACK_TRACE_DEFAULT)) { - actualParams = new ToXContent.DelegatingMapParams(singletonMap(REST_EXCEPTION_SKIP_STACK_TRACE, "false"), params); - } else { - actualParams = params; - } - - if (e != null) { - Supplier messageSupplier = () -> new ParameterizedMessage("path: {}, params: {}", rawPath, actualParams.toMap()); + params = new ToXContent.DelegatingMapParams(singletonMap(REST_EXCEPTION_SKIP_STACK_TRACE, "false"), params); + } else if (e != null) { + Supplier messageSupplier = () -> new ParameterizedMessage("path: {}, params: {}", + channel.request().rawPath(), channel.request().params()); if (status.getStatus() < 500) { SUPPRESSED_ERROR_LOGGER.debug(messageSupplier, e); @@ -163,12 +134,9 @@ private static XContentBuilder build( } } - final XContentBuilder builder = supplier.get(); - builder.startObject(); - { - ElasticsearchException.generateFailureXContent(builder, actualParams, e, detailedErrorsEnabled); - builder.field(STATUS, status.getStatus()); - } + XContentBuilder builder = channel.newErrorBuilder().startObject(); + ElasticsearchException.generateFailureXContent(builder, params, e, channel.detailedErrorsEnabled()); + builder.field(STATUS, status.getStatus()); builder.endObject(); return builder; } diff --git a/server/src/main/java/org/elasticsearch/rest/RestRequest.java b/server/src/main/java/org/elasticsearch/rest/RestRequest.java index d2608686fbd5b..ff1cd456eb008 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestRequest.java +++ b/server/src/main/java/org/elasticsearch/rest/RestRequest.java @@ -33,6 +33,7 @@ import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.common.xcontent.ToXContent; +import org.elasticsearch.common.xcontent.XContent; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentType; @@ -40,7 +41,6 @@ import java.io.InputStream; import java.net.SocketAddress; import java.util.Collections; -import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; @@ -66,47 +66,23 @@ public abstract class RestRequest implements ToXContent.Params { /** * Creates a new RestRequest * @param xContentRegistry the xContentRegistry to use when parsing XContent - * @param uri the URI of the request that potentially contains request parameters + * @param params the parameters of the request + * @param path the path of the request. This should not contain request parameters * @param headers a map of the headers. This map should implement a Case-Insensitive hashing for keys as HTTP header names are case * insensitive */ - public RestRequest(NamedXContentRegistry xContentRegistry, String uri, Map> headers) { - this.xContentRegistry = xContentRegistry; - final Map params = new HashMap<>(); - int pathEndPos = uri.indexOf('?'); - if (pathEndPos < 0) { - this.rawPath = uri; - } else { - this.rawPath = uri.substring(0, pathEndPos); - RestUtils.decodeQueryString(uri, pathEndPos + 1, params); - } - this.params = params; - this.headers = Collections.unmodifiableMap(headers); - final List contentType = getAllHeaderValues("Content-Type"); - final XContentType xContentType = parseContentType(contentType); + public RestRequest( + final XContentType xContentType, + final NamedXContentRegistry xContentRegistry, + final Map params, + final String path, Map> headers) { if (xContentType != null) { this.xContentType.set(xContentType); } - } - - /** - * Creates a new RestRequest - * @param xContentRegistry the xContentRegistry to use when parsing XContent - * @param params the parameters of the request - * @param path the path of the request. This should not contain request parameters - * @param headers a map of the headers. This map should implement a Case-Insensitive hashing for keys as HTTP header names are case - * insensitive - */ - public RestRequest(NamedXContentRegistry xContentRegistry, Map params, String path, Map> headers) { this.xContentRegistry = xContentRegistry; this.params = params; this.rawPath = path; this.headers = Collections.unmodifiableMap(headers); - final List contentType = getAllHeaderValues("Content-Type"); - final XContentType xContentType = parseContentType(contentType); - if (xContentType != null) { - this.xContentType.set(xContentType); - } } public enum Method { @@ -306,11 +282,6 @@ public Boolean paramAsBoolean(String key, Boolean defaultValue) { return Booleans.parseBoolean(param(key), defaultValue); } - @Override - public Map toMap() { - return Collections.unmodifiableMap(params); - } - public TimeValue paramAsTime(String key, TimeValue defaultValue) { return parseTimeValue(param(key), defaultValue, key); } @@ -405,7 +376,7 @@ public final void withContentOrSourceParamParserOrNull(CheckedConsumer contentOrSourceParam() { + public final Tuple contentOrSourceParam() throws IllegalArgumentException { if (hasContentOrSourceParam() == false) { throw new ElasticsearchParseException("request body or source parameter is required"); } else if (hasContent()) { diff --git a/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestRequest.java b/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestRequest.java index 83caf0293e0ab..ae04a142cc607 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestRequest.java +++ b/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestRequest.java @@ -42,7 +42,7 @@ public FakeRestRequest() { private FakeRestRequest(NamedXContentRegistry xContentRegistry, Map> headers, Map params, BytesReference content, Method method, String path, SocketAddress remoteAddress) { - super(xContentRegistry, params, path, headers); + super(XContentType.JSON, xContentRegistry, params, path, headers); this.content = content; this.method = method; this.remoteAddress = remoteAddress; From e0827f52e17254c6d324647fd6280304a0ddb8c4 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Mon, 26 Mar 2018 22:32:15 -0400 Subject: [PATCH 09/18] Cleanup --- .../http/netty4/Netty4HttpRequest.java | 1 - .../http/netty4/Netty4HttpRequestHandler.java | 17 ----------------- .../http/netty4/Netty4BadRequestTests.java | 1 - .../org/elasticsearch/rest/RestRequest.java | 3 +-- 4 files changed, 1 insertion(+), 21 deletions(-) diff --git a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequest.java b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequest.java index 790b498ce97d2..e46122ae63c4d 100644 --- a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequest.java +++ b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequest.java @@ -23,7 +23,6 @@ import io.netty.handler.codec.http.FullHttpRequest; import io.netty.handler.codec.http.HttpHeaders; import io.netty.handler.codec.http.HttpMethod; - import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.xcontent.NamedXContentRegistry; diff --git a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequestHandler.java b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequestHandler.java index 84fe61997faf2..3cd42219e5b6a 100644 --- a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequestHandler.java +++ b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequestHandler.java @@ -20,35 +20,18 @@ package org.elasticsearch.http.netty4; import io.netty.buffer.Unpooled; -import io.netty.channel.Channel; import io.netty.channel.ChannelHandler; import io.netty.channel.ChannelHandlerContext; import io.netty.channel.SimpleChannelInboundHandler; import io.netty.handler.codec.http.DefaultFullHttpRequest; import io.netty.handler.codec.http.FullHttpRequest; -import org.elasticsearch.ExceptionsHelper; -import org.elasticsearch.common.CheckedSupplier; -import org.elasticsearch.common.io.stream.BytesStreamOutput; -import org.elasticsearch.common.io.stream.ReleasableBytesStreamOutput; import org.elasticsearch.common.util.concurrent.ThreadContext; -import org.elasticsearch.common.xcontent.NamedXContentRegistry; -import org.elasticsearch.common.xcontent.ToXContent; -import org.elasticsearch.common.xcontent.XContent; -import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.http.netty4.pipelining.HttpPipelinedRequest; -import org.elasticsearch.rest.AbstractRestChannel; -import org.elasticsearch.rest.BytesRestResponse; import org.elasticsearch.rest.RestRequest; import org.elasticsearch.transport.netty4.Netty4Utils; -import java.io.IOException; import java.util.Collections; -import java.util.HashMap; -import java.util.List; -import java.util.Map; -import java.util.concurrent.atomic.AtomicReference; -import java.util.function.Supplier; @ChannelHandler.Sharable class Netty4HttpRequestHandler extends SimpleChannelInboundHandler { diff --git a/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4BadRequestTests.java b/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4BadRequestTests.java index 088a25228b4d8..8c79d5b5a3cef 100644 --- a/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4BadRequestTests.java +++ b/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4BadRequestTests.java @@ -28,7 +28,6 @@ import org.elasticsearch.common.util.MockPageCacheRecycler; import org.elasticsearch.common.util.concurrent.ThreadContext; import org.elasticsearch.http.HttpServerTransport; -import org.elasticsearch.http.NullDispatcher; import org.elasticsearch.indices.breaker.NoneCircuitBreakerService; import org.elasticsearch.rest.BytesRestResponse; import org.elasticsearch.rest.RestChannel; diff --git a/server/src/main/java/org/elasticsearch/rest/RestRequest.java b/server/src/main/java/org/elasticsearch/rest/RestRequest.java index ff1cd456eb008..c2dee742f1f7d 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestRequest.java +++ b/server/src/main/java/org/elasticsearch/rest/RestRequest.java @@ -33,7 +33,6 @@ import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.common.xcontent.ToXContent; -import org.elasticsearch.common.xcontent.XContent; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentType; @@ -376,7 +375,7 @@ public final void withContentOrSourceParamParserOrNull(CheckedConsumer contentOrSourceParam() throws IllegalArgumentException { + public final Tuple contentOrSourceParam() { if (hasContentOrSourceParam() == false) { throw new ElasticsearchParseException("request body or source parameter is required"); } else if (hasContent()) { From fdafd5cd39f86b4f0b9877b9c8d8cc0c8d1d0a97 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Mon, 26 Mar 2018 22:35:32 -0400 Subject: [PATCH 10/18] Correct constructor --- .../elasticsearch/test/rest/FakeRestRequest.java | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestRequest.java b/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestRequest.java index ae04a142cc607..529b5e6e307d2 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestRequest.java +++ b/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestRequest.java @@ -21,6 +21,7 @@ import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.xcontent.NamedXContentRegistry; +import org.elasticsearch.common.xcontent.XContent; import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.rest.RestRequest; @@ -37,12 +38,12 @@ public class FakeRestRequest extends RestRequest { private final SocketAddress remoteAddress; public FakeRestRequest() { - this(NamedXContentRegistry.EMPTY, new HashMap<>(), new HashMap<>(), null, Method.GET, "/", null); + this(null, NamedXContentRegistry.EMPTY, new HashMap<>(), new HashMap<>(), null, Method.GET, "/", null); } - private FakeRestRequest(NamedXContentRegistry xContentRegistry, Map> headers, Map params, - BytesReference content, Method method, String path, SocketAddress remoteAddress) { - super(XContentType.JSON, xContentRegistry, params, path, headers); + private FakeRestRequest(XContentType xContentType, NamedXContentRegistry xContentRegistry, Map> headers, + Map params, BytesReference content, Method method, String path, SocketAddress remoteAddress) { + super(xContentType, xContentRegistry, params, path, headers); this.content = content; this.method = method; this.remoteAddress = remoteAddress; @@ -82,6 +83,8 @@ public static class Builder { private BytesReference content; + private XContentType xContentType; + private String path = "/"; private Method method = Method.GET; @@ -105,6 +108,7 @@ public Builder withParams(Map params) { public Builder withContent(BytesReference content, XContentType xContentType) { this.content = content; if (xContentType != null) { + xContentType = xContentType; headers.put("Content-Type", Collections.singletonList(xContentType.mediaType())); } return this; @@ -126,7 +130,7 @@ public Builder withRemoteAddress(SocketAddress address) { } public FakeRestRequest build() { - return new FakeRestRequest(xContentRegistry, headers, params, content, method, path, address); + return new FakeRestRequest(xContentType, xContentRegistry, headers, params, content, method, path, address); } } From a312593796d6a5510cd73da683bb3600f9a09a44 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Mon, 26 Mar 2018 22:46:57 -0400 Subject: [PATCH 11/18] Fix compilation --- .../rest/BytesRestResponseTests.java | 43 ++++++++++--------- 1 file changed, 22 insertions(+), 21 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/rest/BytesRestResponseTests.java b/server/src/test/java/org/elasticsearch/rest/BytesRestResponseTests.java index 96106125f19ef..e62875312ce40 100644 --- a/server/src/test/java/org/elasticsearch/rest/BytesRestResponseTests.java +++ b/server/src/test/java/org/elasticsearch/rest/BytesRestResponseTests.java @@ -165,27 +165,28 @@ public void testConvert() throws IOException { public void testResponseWhenPathContainsEncodingError() throws IOException { final String path = "%a"; - final RestRequest request = new RestRequest(NamedXContentRegistry.EMPTY, Collections.emptyMap(), path, Collections.emptyMap()) { - @Override - public Method method() { - return null; - } - - @Override - public String uri() { - return null; - } - - @Override - public boolean hasContent() { - return false; - } - - @Override - public BytesReference content() { - return null; - } - }; + final RestRequest request = + new RestRequest(null, NamedXContentRegistry.EMPTY, Collections.emptyMap(), path, Collections.emptyMap()) { + @Override + public Method method() { + return null; + } + + @Override + public String uri() { + return null; + } + + @Override + public boolean hasContent() { + return false; + } + + @Override + public BytesReference content() { + return null; + } + }; final IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> RestUtils.decodeComponent(request.rawPath())); final RestChannel channel = new DetailedExceptionRestChannel(request); // if we try to decode the path, this will throw an IllegalArgumentException again From 7e77c53d4fea4bea78d73679a526b3662d4f4593 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Mon, 26 Mar 2018 23:13:05 -0400 Subject: [PATCH 12/18] Adjust compilation --- .../test/java/org/elasticsearch/rest/RestControllerTests.java | 2 +- .../src/test/java/org/elasticsearch/rest/RestRequestTests.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java index cb2d51f6a675e..e987e94fd0310 100644 --- a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java +++ b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java @@ -551,7 +551,7 @@ private static final class TestRestRequest extends RestRequest { private final BytesReference content; private TestRestRequest(String path, String content, XContentType xContentType) { - super(NamedXContentRegistry.EMPTY, Collections.emptyMap(), path, xContentType == null ? + super(xContentType, NamedXContentRegistry.EMPTY, Collections.emptyMap(), path, xContentType == null ? Collections.emptyMap() : Collections.singletonMap("Content-Type", Collections.singletonList(xContentType.mediaType()))); this.content = new BytesArray(content); } diff --git a/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java b/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java index d1c7d03e1b174..17d89dc8f9571 100644 --- a/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java +++ b/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java @@ -173,7 +173,7 @@ private static final class ContentRestRequest extends RestRequest { } ContentRestRequest(String content, Map params, Map> headers) { - super(NamedXContentRegistry.EMPTY, params, "not used by this test", headers); + super(XContentType.JSON, NamedXContentRegistry.EMPTY, params, "not used by this test", headers); this.content = new BytesArray(content); } From 653dc429ea13c63524acad96ae8211b362b97b8d Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 27 Mar 2018 06:29:09 -0400 Subject: [PATCH 13/18] Fix test --- .../test/java/org/elasticsearch/rest/RestRequestTests.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java b/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java index 17d89dc8f9571..69d680a68a748 100644 --- a/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java +++ b/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java @@ -173,7 +173,12 @@ private static final class ContentRestRequest extends RestRequest { } ContentRestRequest(String content, Map params, Map> headers) { - super(XContentType.JSON, NamedXContentRegistry.EMPTY, params, "not used by this test", headers); + super( + RestRequest.parseContentType(headers.get("Content-Type")), + NamedXContentRegistry.EMPTY, + params, + "not used by this test", + headers); this.content = new BytesArray(content); } From 992dd024025d542f575af0ccd742548bd6c8107d Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 27 Mar 2018 06:56:30 -0400 Subject: [PATCH 14/18] Fix more tests --- .../java/org/elasticsearch/rest/RestControllerTests.java | 5 +++-- .../java/org/elasticsearch/test/rest/FakeRestRequest.java | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java index e987e94fd0310..86f0ae13296e0 100644 --- a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java +++ b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java @@ -367,9 +367,10 @@ public boolean supportsContentStream() { public void testDispatchWithContentStream() { final String mimeType = randomFrom("application/json", "application/smile"); String content = randomAlphaOfLengthBetween(1, BREAKER_LIMIT.bytesAsInt()); + final List contentTypeHeader = Collections.singletonList(mimeType); FakeRestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY) - .withContent(new BytesArray(content), null).withPath("/foo") - .withHeaders(Collections.singletonMap("Content-Type", Collections.singletonList(mimeType))).build(); + .withContent(new BytesArray(content), RestRequest.parseContentType(contentTypeHeader)).withPath("/foo") + .withHeaders(Collections.singletonMap("Content-Type", contentTypeHeader)).build(); AssertingChannel channel = new AssertingChannel(fakeRestRequest, true, RestStatus.OK); restController.registerHandler(RestRequest.Method.GET, "/foo", new RestHandler() { @Override diff --git a/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestRequest.java b/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestRequest.java index 529b5e6e307d2..e8f8bbe1725e4 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestRequest.java +++ b/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestRequest.java @@ -108,7 +108,7 @@ public Builder withParams(Map params) { public Builder withContent(BytesReference content, XContentType xContentType) { this.content = content; if (xContentType != null) { - xContentType = xContentType; + this.xContentType = xContentType; headers.put("Content-Type", Collections.singletonList(xContentType.mediaType())); } return this; From 271dcd6f3b5372165055587827cea951bbdced05 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 27 Mar 2018 09:36:44 -0400 Subject: [PATCH 15/18] Iteration --- .../http/netty4/Netty4HttpRequest.java | 41 ++--- .../http/netty4/Netty4HttpRequestHandler.java | 152 ++++++++++-------- .../http/netty4/Netty4BadRequestTests.java | 5 +- .../http/netty4/Netty4HttpChannelTests.java | 11 +- .../rest/Netty4BadRequestIT.java | 6 +- .../org/elasticsearch/rest/RestRequest.java | 53 +++++- .../rest/BytesRestResponseTests.java | 2 +- .../rest/RestControllerTests.java | 2 +- .../elasticsearch/rest/RestRequestTests.java | 30 ++-- .../test/rest/FakeRestRequest.java | 12 +- 10 files changed, 182 insertions(+), 132 deletions(-) diff --git a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequest.java b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequest.java index e46122ae63c4d..08b2f780da952 100644 --- a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequest.java +++ b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequest.java @@ -26,16 +26,13 @@ import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.xcontent.NamedXContentRegistry; -import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.rest.RestRequest; -import org.elasticsearch.rest.RestUtils; import org.elasticsearch.transport.netty4.Netty4Utils; import java.net.SocketAddress; import java.util.AbstractMap; import java.util.Collection; import java.util.Collections; -import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Set; @@ -50,42 +47,27 @@ public class Netty4HttpRequest extends RestRequest { /** * Construct a new request. * - * @param xContentType the content type * @param xContentRegistry the content registry * @param request the underlying request * @param channel the channel for the request - * @throws IllegalArgumentException if the parameters can not be decoded + * @throws BadParameterException if the parameters can not be decoded */ - Netty4HttpRequest(XContentType xContentType, NamedXContentRegistry xContentRegistry, FullHttpRequest request, Channel channel) { - this(xContentType, xContentRegistry, params(request), uri(request), request, channel); - } - - private static Map params(FullHttpRequest request) { - final String uri = request.uri(); - final Map params = new HashMap<>(); - int index = uri.indexOf('?'); - if (index >= 0) { - RestUtils.decodeQueryString(uri, index + 1, params); - } - return params; - } - - private static String uri(FullHttpRequest request) { - final String uri = request.uri(); - final int index = uri.indexOf('?'); - if (index >= 0) { - return uri.substring(0, index); + Netty4HttpRequest(NamedXContentRegistry xContentRegistry, FullHttpRequest request, Channel channel) { + super(xContentRegistry, request.uri(), new HttpHeadersMap(request.headers())); + this.request = request; + this.channel = channel; + if (request.content().isReadable()) { + this.content = Netty4Utils.toBytesReference(request.content()); } else { - return uri; + this.content = BytesArray.EMPTY; } } /** * Construct a new request. In contrast to - * {@link Netty4HttpRequest#Netty4HttpRequest(XContentType, NamedXContentRegistry, Map, String, FullHttpRequest, Channel)}, the URI is - * not decoded so this method constructor should not throw any exceptions. + * {@link Netty4HttpRequest#Netty4HttpRequest(NamedXContentRegistry, Map, String, FullHttpRequest, Channel)}, the URI is not decoded so + * this method constructor should not throw any exceptions. * - * @param xContentType the content type * @param xContentRegistry the content registry * @param params the parameters for the request * @param uri the path for the request @@ -93,13 +75,12 @@ private static String uri(FullHttpRequest request) { * @param channel the channel for the request */ Netty4HttpRequest( - final XContentType xContentType, final NamedXContentRegistry xContentRegistry, final Map params, final String uri, final FullHttpRequest request, final Channel channel) { - super(xContentType, xContentRegistry, params, uri, new HttpHeadersMap(request.headers())); + super(xContentRegistry, params, uri, new HttpHeadersMap(request.headers())); this.request = request; this.channel = channel; if (request.content().isReadable()) { diff --git a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequestHandler.java b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequestHandler.java index 3cd42219e5b6a..3566653b12526 100644 --- a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequestHandler.java +++ b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequestHandler.java @@ -20,13 +20,15 @@ package org.elasticsearch.http.netty4; import io.netty.buffer.Unpooled; +import io.netty.channel.Channel; import io.netty.channel.ChannelHandler; import io.netty.channel.ChannelHandlerContext; import io.netty.channel.SimpleChannelInboundHandler; import io.netty.handler.codec.http.DefaultFullHttpRequest; +import io.netty.handler.codec.http.DefaultHttpHeaders; import io.netty.handler.codec.http.FullHttpRequest; +import io.netty.handler.codec.http.HttpHeaders; import org.elasticsearch.common.util.concurrent.ThreadContext; -import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.http.netty4.pipelining.HttpPipelinedRequest; import org.elasticsearch.rest.RestRequest; import org.elasticsearch.transport.netty4.Netty4Utils; @@ -60,84 +62,100 @@ protected void channelRead0(ChannelHandlerContext ctx, Object msg) throws Except request = (FullHttpRequest) msg; } - final FullHttpRequest copy = - new DefaultFullHttpRequest( - request.protocolVersion(), - request.method(), - request.uri(), - Unpooled.copiedBuffer(request.content()), - request.headers(), - request.trailingHeaders()); - - Exception badRequestCause = null; - final XContentType xContentType; - { - XContentType innerXContentType; - try { - innerXContentType = RestRequest.parseContentType(request.headers().getAll("Content-Type")); - } catch (final IllegalArgumentException e) { - innerXContentType = null; - badRequestCause = e; - } - xContentType = innerXContentType; - } + boolean success = false; + try { - final Netty4HttpRequest httpRequest; - { - Netty4HttpRequest innerHttpRequest; - try { - innerHttpRequest = new Netty4HttpRequest(xContentType, serverTransport.xContentRegistry, copy, ctx.channel()); - } catch (final Exception e) { - if (badRequestCause == null) { + final FullHttpRequest copy = + new DefaultFullHttpRequest( + request.protocolVersion(), + request.method(), + request.uri(), + Unpooled.copiedBuffer(request.content()), + request.headers(), + request.trailingHeaders()); + + Exception badRequestCause = null; + + final Netty4HttpRequest httpRequest; + { + Netty4HttpRequest innerHttpRequest; + try { + innerHttpRequest = new Netty4HttpRequest(serverTransport.xContentRegistry, copy, ctx.channel()); + } catch (final RestRequest.ContentTypeHeaderException e) { + badRequestCause = e; + innerHttpRequest = requestWithoutContentTypeHeader(copy, ctx.channel(), badRequestCause); + } catch (final RestRequest.BadParameterException e) { badRequestCause = e; - } else { - badRequestCause.addSuppressed(e); + innerHttpRequest = requestWithoutParameters(copy, ctx.channel()); } - innerHttpRequest = - new Netty4HttpRequest( - xContentType, - serverTransport.xContentRegistry, - Collections.emptyMap(), - copy.uri(), - copy, - ctx.channel()); + httpRequest = innerHttpRequest; } - httpRequest = innerHttpRequest; - } - final Netty4HttpChannel channel; - { - Netty4HttpChannel innerChannel; - try { - innerChannel = new Netty4HttpChannel(serverTransport, httpRequest, pipelinedRequest, detailedErrorsEnabled, threadContext); - } catch (final Exception e) { - if (badRequestCause == null) { - badRequestCause = e; - } else { - badRequestCause.addSuppressed(e); + final Netty4HttpChannel channel; + { + Netty4HttpChannel innerChannel; + try { + innerChannel = + new Netty4HttpChannel(serverTransport, httpRequest, pipelinedRequest, detailedErrorsEnabled, threadContext); + } catch (final IllegalArgumentException e) { + if (badRequestCause == null) { + badRequestCause = e; + } else { + badRequestCause.addSuppressed(e); + } + final Netty4HttpRequest innerRequest = + new Netty4HttpRequest( + serverTransport.xContentRegistry, + Collections.emptyMap(), + copy.uri(), + copy, + ctx.channel()); + innerChannel = + new Netty4HttpChannel(serverTransport, innerRequest, pipelinedRequest, detailedErrorsEnabled, threadContext); } - final Netty4HttpRequest innerRequest = - new Netty4HttpRequest( - xContentType, - serverTransport.xContentRegistry, - Collections.emptyMap(), - copy.uri(), - copy, - ctx.channel()); - innerChannel = new Netty4HttpChannel(serverTransport, innerRequest, pipelinedRequest, detailedErrorsEnabled, threadContext); + channel = innerChannel; + } + + if (request.decoderResult().isFailure()) { + serverTransport.dispatchBadRequest(httpRequest, channel, request.decoderResult().cause()); + } else if (badRequestCause != null) { + serverTransport.dispatchBadRequest(httpRequest, channel, badRequestCause); + } else { + serverTransport.dispatchRequest(httpRequest, channel); + } + success = true; + } finally { + if (success == false && pipelinedRequest != null) { + pipelinedRequest.release(); } - channel = innerChannel; } + } - if (request.decoderResult().isFailure()) { - serverTransport.dispatchBadRequest(httpRequest, channel, request.decoderResult().cause()); - } else if (badRequestCause != null) { - serverTransport.dispatchBadRequest(httpRequest, channel, badRequestCause); - } else { - serverTransport.dispatchRequest(httpRequest, channel); + private Netty4HttpRequest requestWithoutContentTypeHeader( + final FullHttpRequest request, final Channel channel, final Exception badRequestCause) { + final HttpHeaders headersWithoutContentTypeHeader = new DefaultHttpHeaders(); + headersWithoutContentTypeHeader.add(request.headers()); + headersWithoutContentTypeHeader.remove("Content-Type"); + final FullHttpRequest requestWithoutContentTypeHeader = + new DefaultFullHttpRequest( + request.protocolVersion(), + request.method(), + request.uri(), + request.content(), + headersWithoutContentTypeHeader, + request.trailingHeaders()); // Content-Type can not be a trailing header + try { + return new Netty4HttpRequest(serverTransport.xContentRegistry, requestWithoutContentTypeHeader, channel); + } catch (final RestRequest.BadParameterException e) { + badRequestCause.addSuppressed(e); + return requestWithoutParameters(requestWithoutContentTypeHeader, channel); } } + private Netty4HttpRequest requestWithoutParameters(final FullHttpRequest request, final Channel channel) { + return new Netty4HttpRequest(serverTransport.xContentRegistry, Collections.emptyMap(), request.uri(), request, channel); + } + @Override public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws Exception { Netty4Utils.maybeDie(cause); diff --git a/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4BadRequestTests.java b/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4BadRequestTests.java index 8c79d5b5a3cef..094f339059876 100644 --- a/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4BadRequestTests.java +++ b/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4BadRequestTests.java @@ -96,10 +96,11 @@ public void dispatchBadRequest(RestRequest request, RestChannel channel, ThreadC assertThat(responses.iterator().next().status().code(), equalTo(400)); final Collection responseBodies = Netty4HttpClient.returnHttpResponseBodies(responses); assertThat(responseBodies, hasSize(1)); - assertThat(responseBodies.iterator().next(), containsString("\"type\":\"illegal_argument_exception\"")); + assertThat(responseBodies.iterator().next(), containsString("\"type\":\"bad_parameter_exception\"")); assertThat( responseBodies.iterator().next(), - containsString("\"reason\":\"unterminated escape sequence at end of string: %\"")); + containsString( + "\"reason\":\"java.lang.IllegalArgumentException: unterminated escape sequence at end of string: %\"")); } } } diff --git a/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpChannelTests.java b/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpChannelTests.java index c96143fabe4b1..918e98fd2e7c0 100644 --- a/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpChannelTests.java +++ b/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpChannelTests.java @@ -55,7 +55,6 @@ import org.elasticsearch.common.util.MockPageCacheRecycler; import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.common.xcontent.XContentBuilder; -import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.common.xcontent.json.JsonXContent; import org.elasticsearch.http.HttpTransportSettings; import org.elasticsearch.http.NullDispatcher; @@ -212,7 +211,7 @@ public void testHeadersSet() { final FullHttpRequest httpRequest = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "/"); httpRequest.headers().add(HttpHeaderNames.ORIGIN, "remote"); final WriteCapturingChannel writeCapturingChannel = new WriteCapturingChannel(); - Netty4HttpRequest request = new Netty4HttpRequest(XContentType.JSON, xContentRegistry(), httpRequest, writeCapturingChannel); + Netty4HttpRequest request = new Netty4HttpRequest(xContentRegistry(), httpRequest, writeCapturingChannel); // send a response Netty4HttpChannel channel = @@ -241,7 +240,7 @@ public void testReleaseOnSendToClosedChannel() { new Netty4HttpServerTransport(settings, networkService, bigArrays, threadPool, registry, new NullDispatcher())) { final FullHttpRequest httpRequest = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "/"); final EmbeddedChannel embeddedChannel = new EmbeddedChannel(); - final Netty4HttpRequest request = new Netty4HttpRequest(XContentType.JSON, registry, httpRequest, embeddedChannel); + final Netty4HttpRequest request = new Netty4HttpRequest(registry, httpRequest, embeddedChannel); final HttpPipelinedRequest pipelinedRequest = randomBoolean() ? new HttpPipelinedRequest(request.request(), 1) : null; final Netty4HttpChannel channel = new Netty4HttpChannel(httpServerTransport, request, pipelinedRequest, randomBoolean(), threadPool.getThreadContext()); @@ -260,7 +259,7 @@ public void testReleaseOnSendToChannelAfterException() throws IOException { new Netty4HttpServerTransport(settings, networkService, bigArrays, threadPool, registry, new NullDispatcher())) { final FullHttpRequest httpRequest = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "/"); final EmbeddedChannel embeddedChannel = new EmbeddedChannel(); - final Netty4HttpRequest request = new Netty4HttpRequest(XContentType.JSON, registry, httpRequest, embeddedChannel); + final Netty4HttpRequest request = new Netty4HttpRequest(registry, httpRequest, embeddedChannel); final HttpPipelinedRequest pipelinedRequest = randomBoolean() ? new HttpPipelinedRequest(request.request(), 1) : null; final Netty4HttpChannel channel = new Netty4HttpChannel(httpServerTransport, request, pipelinedRequest, randomBoolean(), threadPool.getThreadContext()); @@ -303,7 +302,7 @@ public void testConnectionClose() throws Exception { } } final EmbeddedChannel embeddedChannel = new EmbeddedChannel(); - final Netty4HttpRequest request = new Netty4HttpRequest(XContentType.JSON, xContentRegistry(), httpRequest, embeddedChannel); + final Netty4HttpRequest request = new Netty4HttpRequest(xContentRegistry(), httpRequest, embeddedChannel); // send a response, the channel close status should match assertTrue(embeddedChannel.isOpen()); @@ -332,7 +331,7 @@ private FullHttpResponse executeRequest(final Settings settings, final String or httpRequest.headers().add(HttpHeaderNames.HOST, host); final WriteCapturingChannel writeCapturingChannel = new WriteCapturingChannel(); final Netty4HttpRequest request = - new Netty4HttpRequest(XContentType.JSON, xContentRegistry(), httpRequest, writeCapturingChannel); + new Netty4HttpRequest(xContentRegistry(), httpRequest, writeCapturingChannel); Netty4HttpChannel channel = new Netty4HttpChannel(httpServerTransport, request, null, randomBoolean(), threadPool.getThreadContext()); diff --git a/modules/transport-netty4/src/test/java/org/elasticsearch/rest/Netty4BadRequestIT.java b/modules/transport-netty4/src/test/java/org/elasticsearch/rest/Netty4BadRequestIT.java index d01087ad3fd57..d171999f4349b 100644 --- a/modules/transport-netty4/src/test/java/org/elasticsearch/rest/Netty4BadRequestIT.java +++ b/modules/transport-netty4/src/test/java/org/elasticsearch/rest/Netty4BadRequestIT.java @@ -84,7 +84,7 @@ public void testInvalidParameterValue() throws IOException { assertThat(response.getStatusLine().getStatusCode(), equalTo(400)); final ObjectPath objectPath = ObjectPath.createFromResponse(response); final Map map = objectPath.evaluate("error"); - assertThat(map.get("type"), equalTo("illegal_argument_exception")); + assertThat(map.get("type"), equalTo("invalid_argument_exception")); assertThat(map.get("reason"), equalTo("Failed to parse value [neither-true-nor-false] as only [true] or [false] are allowed.")); } @@ -96,8 +96,8 @@ public void testInvalidHeaderValue() throws IOException { assertThat(response.getStatusLine().getStatusCode(), equalTo(400)); final ObjectPath objectPath = ObjectPath.createFromResponse(response); final Map map = objectPath.evaluate("error"); - assertThat(map.get("type"), equalTo("illegal_argument_exception")); - assertThat(map.get("reason"), equalTo("invalid Content-Type header []")); + assertThat(map.get("type"), equalTo("content_type_header_exception")); + assertThat(map.get("reason"), equalTo("java.lang.IllegalArgumentException: invalid Content-Type header []")); } } diff --git a/server/src/main/java/org/elasticsearch/rest/RestRequest.java b/server/src/main/java/org/elasticsearch/rest/RestRequest.java index c2dee742f1f7d..8a1cb187ebb29 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestRequest.java +++ b/server/src/main/java/org/elasticsearch/rest/RestRequest.java @@ -40,6 +40,7 @@ import java.io.InputStream; import java.net.SocketAddress; import java.util.Collections; +import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; @@ -62,6 +63,32 @@ public abstract class RestRequest implements ToXContent.Params { private final Set consumedParams = new HashSet<>(); private final SetOnce xContentType = new SetOnce<>(); + public RestRequest(final NamedXContentRegistry xContentRegistry, final String uri, final Map> headers) { + this(xContentRegistry, params(uri), path(uri), headers); + } + + private static Map params(final String uri) { + final Map params = new HashMap<>(); + int index = uri.indexOf('?'); + if (index >= 0) { + try { + RestUtils.decodeQueryString(uri, index + 1, params); + } catch (final IllegalArgumentException e) { + throw new BadParameterException(e); + } + } + return params; + } + + private static String path(final String uri) { + final int index = uri.indexOf('?'); + if (index >= 0) { + return uri.substring(0, index); + } else { + return uri; + } + } + /** * Creates a new RestRequest * @param xContentRegistry the xContentRegistry to use when parsing XContent @@ -71,10 +98,16 @@ public abstract class RestRequest implements ToXContent.Params { * insensitive */ public RestRequest( - final XContentType xContentType, final NamedXContentRegistry xContentRegistry, final Map params, - final String path, Map> headers) { + final String path, + final Map> headers) { + final XContentType xContentType; + try { + xContentType = parseContentType(headers.get("Content-Type")); + } catch (final IllegalArgumentException e) { + throw new ContentTypeHeaderException(e); + } if (xContentType != null) { this.xContentType.set(xContentType); } @@ -419,4 +452,20 @@ public static XContentType parseContentType(List header) { throw new IllegalArgumentException("empty Content-Type header"); } + public static class ContentTypeHeaderException extends RuntimeException { + + ContentTypeHeaderException(final IllegalArgumentException cause) { + super(cause); + } + + } + + public static class BadParameterException extends RuntimeException { + + BadParameterException(final IllegalArgumentException cause) { + super(cause); + } + + } + } diff --git a/server/src/test/java/org/elasticsearch/rest/BytesRestResponseTests.java b/server/src/test/java/org/elasticsearch/rest/BytesRestResponseTests.java index e62875312ce40..a0e6f7020302d 100644 --- a/server/src/test/java/org/elasticsearch/rest/BytesRestResponseTests.java +++ b/server/src/test/java/org/elasticsearch/rest/BytesRestResponseTests.java @@ -166,7 +166,7 @@ public void testConvert() throws IOException { public void testResponseWhenPathContainsEncodingError() throws IOException { final String path = "%a"; final RestRequest request = - new RestRequest(null, NamedXContentRegistry.EMPTY, Collections.emptyMap(), path, Collections.emptyMap()) { + new RestRequest(NamedXContentRegistry.EMPTY, Collections.emptyMap(), path, Collections.emptyMap()) { @Override public Method method() { return null; diff --git a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java index 86f0ae13296e0..f36638a43909f 100644 --- a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java +++ b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java @@ -552,7 +552,7 @@ private static final class TestRestRequest extends RestRequest { private final BytesReference content; private TestRestRequest(String path, String content, XContentType xContentType) { - super(xContentType, NamedXContentRegistry.EMPTY, Collections.emptyMap(), path, xContentType == null ? + super(NamedXContentRegistry.EMPTY, Collections.emptyMap(), path, xContentType == null ? Collections.emptyMap() : Collections.singletonMap("Content-Type", Collections.singletonList(xContentType.mediaType()))); this.content = new BytesArray(content); } diff --git a/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java b/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java index 69d680a68a748..1b4bbff7322de 100644 --- a/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java +++ b/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java @@ -38,6 +38,8 @@ import static java.util.Collections.emptyMap; import static java.util.Collections.singletonMap; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.instanceOf; public class RestRequestTests extends ESTestCase { public void testContentParser() throws IOException { @@ -130,9 +132,15 @@ public void testPlainTextSupport() { public void testMalformedContentTypeHeader() { final String type = randomFrom("text", "text/:ain; charset=utf-8", "text/plain\";charset=utf-8", ":", "/", "t:/plain"); - IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> new ContentRestRequest("", Collections.emptyMap(), - Collections.singletonMap("Content-Type", Collections.singletonList(type)))); - assertEquals("invalid Content-Type header [" + type + "]", e.getMessage()); + final RestRequest.ContentTypeHeaderException e = expectThrows( + RestRequest.ContentTypeHeaderException.class, + () -> { + final Map> headers = Collections.singletonMap("Content-Type", Collections.singletonList(type)); + new ContentRestRequest("", Collections.emptyMap(), headers); + }); + assertNotNull(e.getCause()); + assertThat(e.getCause(), instanceOf(IllegalArgumentException.class)); + assertThat(e.getMessage(), equalTo("java.lang.IllegalArgumentException: invalid Content-Type header [" + type + "]")); } public void testNoContentTypeHeader() { @@ -142,9 +150,12 @@ public void testNoContentTypeHeader() { public void testMultipleContentTypeHeaders() { List headers = new ArrayList<>(randomUnique(() -> randomAlphaOfLengthBetween(1, 16), randomIntBetween(2, 10))); - IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> new ContentRestRequest("", Collections.emptyMap(), - Collections.singletonMap("Content-Type", headers))); - assertEquals("only one Content-Type header should be provided", e.getMessage()); + final RestRequest.ContentTypeHeaderException e = expectThrows( + RestRequest.ContentTypeHeaderException.class, + () -> new ContentRestRequest("", Collections.emptyMap(), Collections.singletonMap("Content-Type", headers))); + assertNotNull(e.getCause()); + assertThat(e.getCause(), instanceOf((IllegalArgumentException.class))); + assertThat(e.getMessage(), equalTo("java.lang.IllegalArgumentException: only one Content-Type header should be provided")); } public void testRequiredContent() { @@ -173,12 +184,7 @@ private static final class ContentRestRequest extends RestRequest { } ContentRestRequest(String content, Map params, Map> headers) { - super( - RestRequest.parseContentType(headers.get("Content-Type")), - NamedXContentRegistry.EMPTY, - params, - "not used by this test", - headers); + super(NamedXContentRegistry.EMPTY, params, "not used by this test", headers); this.content = new BytesArray(content); } diff --git a/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestRequest.java b/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestRequest.java index e8f8bbe1725e4..d0403736400cd 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestRequest.java +++ b/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestRequest.java @@ -21,7 +21,6 @@ import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.xcontent.NamedXContentRegistry; -import org.elasticsearch.common.xcontent.XContent; import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.rest.RestRequest; @@ -38,12 +37,12 @@ public class FakeRestRequest extends RestRequest { private final SocketAddress remoteAddress; public FakeRestRequest() { - this(null, NamedXContentRegistry.EMPTY, new HashMap<>(), new HashMap<>(), null, Method.GET, "/", null); + this(NamedXContentRegistry.EMPTY, new HashMap<>(), new HashMap<>(), null, Method.GET, "/", null); } - private FakeRestRequest(XContentType xContentType, NamedXContentRegistry xContentRegistry, Map> headers, + private FakeRestRequest(NamedXContentRegistry xContentRegistry, Map> headers, Map params, BytesReference content, Method method, String path, SocketAddress remoteAddress) { - super(xContentType, xContentRegistry, params, path, headers); + super(xContentRegistry, params, path, headers); this.content = content; this.method = method; this.remoteAddress = remoteAddress; @@ -83,8 +82,6 @@ public static class Builder { private BytesReference content; - private XContentType xContentType; - private String path = "/"; private Method method = Method.GET; @@ -108,7 +105,6 @@ public Builder withParams(Map params) { public Builder withContent(BytesReference content, XContentType xContentType) { this.content = content; if (xContentType != null) { - this.xContentType = xContentType; headers.put("Content-Type", Collections.singletonList(xContentType.mediaType())); } return this; @@ -130,7 +126,7 @@ public Builder withRemoteAddress(SocketAddress address) { } public FakeRestRequest build() { - return new FakeRestRequest(xContentType, xContentRegistry, headers, params, content, method, path, address); + return new FakeRestRequest(xContentRegistry, headers, params, content, method, path, address); } } From 080ee4c532270cdc036f7893693512e3584e28df Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 27 Mar 2018 10:48:49 -0400 Subject: [PATCH 16/18] Javadocs --- .../http/netty4/Netty4HttpRequest.java | 6 +++-- .../org/elasticsearch/rest/RestRequest.java | 24 ++++++++++++++----- 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequest.java b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequest.java index 08b2f780da952..5194c762b7e43 100644 --- a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequest.java +++ b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequest.java @@ -50,7 +50,8 @@ public class Netty4HttpRequest extends RestRequest { * @param xContentRegistry the content registry * @param request the underlying request * @param channel the channel for the request - * @throws BadParameterException if the parameters can not be decoded + * @throws BadParameterException if the parameters can not be decoded + * @throws ContentTypeHeaderException if the Content-Type header can not be parsed */ Netty4HttpRequest(NamedXContentRegistry xContentRegistry, FullHttpRequest request, Channel channel) { super(xContentRegistry, request.uri(), new HttpHeadersMap(request.headers())); @@ -66,13 +67,14 @@ public class Netty4HttpRequest extends RestRequest { /** * Construct a new request. In contrast to * {@link Netty4HttpRequest#Netty4HttpRequest(NamedXContentRegistry, Map, String, FullHttpRequest, Channel)}, the URI is not decoded so - * this method constructor should not throw any exceptions. + * this constructor will not throw a {@link BadParameterException}. * * @param xContentRegistry the content registry * @param params the parameters for the request * @param uri the path for the request * @param request the underlying request * @param channel the channel for the request + * @throws ContentTypeHeaderException if the Content-Type header can not be parsed */ Netty4HttpRequest( final NamedXContentRegistry xContentRegistry, diff --git a/server/src/main/java/org/elasticsearch/rest/RestRequest.java b/server/src/main/java/org/elasticsearch/rest/RestRequest.java index 8a1cb187ebb29..bd46a20f31231 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestRequest.java +++ b/server/src/main/java/org/elasticsearch/rest/RestRequest.java @@ -63,6 +63,15 @@ public abstract class RestRequest implements ToXContent.Params { private final Set consumedParams = new HashSet<>(); private final SetOnce xContentType = new SetOnce<>(); + /** + * Creates a new REST request. + * + * @param xContentRegistry the content registry + * @param uri the raw URI that will be parsed into the path and the parameters + * @param headers a map of the header; this map should implement a case-insensitive lookup + * @throws BadParameterException if the parameters can not be decoded + * @throws ContentTypeHeaderException if the Content-Type header can not be parsed + */ public RestRequest(final NamedXContentRegistry xContentRegistry, final String uri, final Map> headers) { this(xContentRegistry, params(uri), path(uri), headers); } @@ -90,12 +99,15 @@ private static String path(final String uri) { } /** - * Creates a new RestRequest - * @param xContentRegistry the xContentRegistry to use when parsing XContent - * @param params the parameters of the request - * @param path the path of the request. This should not contain request parameters - * @param headers a map of the headers. This map should implement a Case-Insensitive hashing for keys as HTTP header names are case - * insensitive + * Creates a new REST request. In contrast to + * {@link RestRequest#RestRequest(NamedXContentRegistry, Map, String, Map)}, the path is not decoded so this constructor will not throw + * a {@link BadParameterException}. + * + * @param xContentRegistry the content registry + * @param params the request parameters + * @param path the raw path (which is not parsed) + * @param headers a map of the header; this map should implement a case-insensitive lookup + * @throws ContentTypeHeaderException if the Content-Type header can not be parsed */ public RestRequest( final NamedXContentRegistry xContentRegistry, From cab60fb80cfcb0735be54da1c11f0058f2b7ac06 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 27 Mar 2018 12:10:15 -0400 Subject: [PATCH 17/18] Fix expected exception --- .../test/java/org/elasticsearch/rest/Netty4BadRequestIT.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/transport-netty4/src/test/java/org/elasticsearch/rest/Netty4BadRequestIT.java b/modules/transport-netty4/src/test/java/org/elasticsearch/rest/Netty4BadRequestIT.java index d171999f4349b..028770ed22469 100644 --- a/modules/transport-netty4/src/test/java/org/elasticsearch/rest/Netty4BadRequestIT.java +++ b/modules/transport-netty4/src/test/java/org/elasticsearch/rest/Netty4BadRequestIT.java @@ -84,7 +84,7 @@ public void testInvalidParameterValue() throws IOException { assertThat(response.getStatusLine().getStatusCode(), equalTo(400)); final ObjectPath objectPath = ObjectPath.createFromResponse(response); final Map map = objectPath.evaluate("error"); - assertThat(map.get("type"), equalTo("invalid_argument_exception")); + assertThat(map.get("type"), equalTo("illegal_argument_exception")); assertThat(map.get("reason"), equalTo("Failed to parse value [neither-true-nor-false] as only [true] or [false] are allowed.")); } From 2f491c5ec2313082ad31c139579426d2b293be2b Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 27 Mar 2018 12:35:12 -0400 Subject: [PATCH 18/18] Add some code comments --- .../http/netty4/Netty4HttpRequestHandler.java | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequestHandler.java b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequestHandler.java index 3566653b12526..1fd18b2a016d7 100644 --- a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequestHandler.java +++ b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequestHandler.java @@ -76,6 +76,13 @@ protected void channelRead0(ChannelHandlerContext ctx, Object msg) throws Except Exception badRequestCause = null; + /* + * We want to create a REST request from the incoming request from Netty. However, creating this request could fail if there + * are incorrectly encoded parameters, or the Content-Type header is invalid. If one of these specific failures occurs, we + * attempt to create a REST request again without the input that caused the exception (e.g., we remove the Content-Type header, + * or skip decoding the parameters). Once we have a request in hand, we then dispatch the request as a bad request with the + * underlying exception that caused us to treat the request as bad. + */ final Netty4HttpRequest httpRequest; { Netty4HttpRequest innerHttpRequest; @@ -91,6 +98,12 @@ protected void channelRead0(ChannelHandlerContext ctx, Object msg) throws Except httpRequest = innerHttpRequest; } + /* + * We now want to create a channel used to send the response on. However, creating this channel can fail if there are invalid + * parameter values for any of the filter_path, human, or pretty parameters. We detect these specific failures via an + * IllegalArgumentException from the channel constructor and then attempt to create a new channel that bypasses parsing of these + * parameter values. + */ final Netty4HttpChannel channel; { Netty4HttpChannel innerChannel; @@ -106,7 +119,7 @@ protected void channelRead0(ChannelHandlerContext ctx, Object msg) throws Except final Netty4HttpRequest innerRequest = new Netty4HttpRequest( serverTransport.xContentRegistry, - Collections.emptyMap(), + Collections.emptyMap(), // we are going to dispatch the request as a bad request, drop all parameters copy.uri(), copy, ctx.channel()); @@ -125,6 +138,7 @@ protected void channelRead0(ChannelHandlerContext ctx, Object msg) throws Except } success = true; } finally { + // the request is otherwise released in case of dispatch if (success == false && pipelinedRequest != null) { pipelinedRequest.release(); } @@ -142,7 +156,7 @@ private Netty4HttpRequest requestWithoutContentTypeHeader( request.method(), request.uri(), request.content(), - headersWithoutContentTypeHeader, + headersWithoutContentTypeHeader, // remove the Content-Type header so as to not parse it again request.trailingHeaders()); // Content-Type can not be a trailing header try { return new Netty4HttpRequest(serverTransport.xContentRegistry, requestWithoutContentTypeHeader, channel); @@ -153,6 +167,7 @@ private Netty4HttpRequest requestWithoutContentTypeHeader( } private Netty4HttpRequest requestWithoutParameters(final FullHttpRequest request, final Channel channel) { + // remove all parameters as at least one is incorrectly encoded return new Netty4HttpRequest(serverTransport.xContentRegistry, Collections.emptyMap(), request.uri(), request, channel); }