diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentType.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentType.java index 25c390c9a1142..42129e4f5e4bb 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentType.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentType.java @@ -117,7 +117,7 @@ public XContent xContent() { }; private static final Pattern COMPATIBLE_API_HEADER_PATTERN = Pattern.compile( - "application/(vnd.elasticsearch\\+)?([^;]+)(\\s*;\\s*compatible-with=(\\d+))?", + "(application|text)/(vnd.elasticsearch\\+)?([^;]+)(\\s*;\\s*compatible-with=(\\d+))?", Pattern.CASE_INSENSITIVE); /** @@ -126,7 +126,9 @@ public XContent xContent() { * also supports a wildcard accept for {@code application/*}. This method can be used to parse the {@code Accept} HTTP header or a * format query string parameter. This method will return {@code null} if no match is found */ - public static XContentType fromMediaTypeOrFormat(String mediaType) { + public static XContentType fromMediaTypeOrFormat(String mediaTypeHeaderValue) { + String mediaType = parseMediaType(mediaTypeHeaderValue); + if (mediaType == null) { return null; } @@ -136,7 +138,7 @@ public static XContentType fromMediaTypeOrFormat(String mediaType) { } } final String lowercaseMediaType = mediaType.toLowerCase(Locale.ROOT); - if (lowercaseMediaType.startsWith("application/*")) { + if (lowercaseMediaType.startsWith("application/*") || lowercaseMediaType.equals("*/*")) { return JSON; } @@ -165,11 +167,12 @@ public static XContentType fromMediaType(String mediaTypeHeaderValue) { return null; } + //public scope needed for text formats hack public static String parseMediaType(String mediaType) { if (mediaType != null) { Matcher matcher = COMPATIBLE_API_HEADER_PATTERN.matcher(mediaType); if (matcher.find()) { - return "application/" + matcher.group(2).toLowerCase(Locale.ROOT); + return (matcher.group(1) + "/" + matcher.group(3)).toLowerCase(Locale.ROOT); } } @@ -179,9 +182,9 @@ public static String parseMediaType(String mediaType) { public static String parseVersion(String mediaType){ if(mediaType != null){ Matcher matcher = COMPATIBLE_API_HEADER_PATTERN.matcher(mediaType); - if (matcher.find() && "vnd.elasticsearch+".equalsIgnoreCase(matcher.group(1))) { + if (matcher.find() && "vnd.elasticsearch+".equalsIgnoreCase(matcher.group(2))) { - return matcher.group(4); + return matcher.group(5); } } return null; diff --git a/modules/rest-compatibility/src/main/java/org/elasticsearch/rest/compat/version7/RestGetActionV7.java b/modules/rest-compatibility/src/main/java/org/elasticsearch/rest/compat/version7/RestGetActionV7.java index 554ea947b8f2a..d8d01f42698f7 100644 --- a/modules/rest-compatibility/src/main/java/org/elasticsearch/rest/compat/version7/RestGetActionV7.java +++ b/modules/rest-compatibility/src/main/java/org/elasticsearch/rest/compat/version7/RestGetActionV7.java @@ -37,12 +37,12 @@ public class RestGetActionV7 extends RestGetAction { private static final DeprecationLogger deprecationLogger = new DeprecationLogger(LogManager.getLogger(RestGetAction.class)); static final String TYPES_DEPRECATION_MESSAGE = "[types removal] Specifying types in " + "document get requests is deprecated, use the /{index}/_doc/{id} endpoint instead."; + @Override public String getName() { return "document_get_action_v7"; } - @Override public List routes() { assert Version.CURRENT.major == 8 : "REST API compatibility for version 7 is only supported on version 8"; diff --git a/modules/rest-compatibility/src/main/java/org/elasticsearch/rest/compat/version7/RestIndexActionV7.java b/modules/rest-compatibility/src/main/java/org/elasticsearch/rest/compat/version7/RestIndexActionV7.java index 33e52ca5e4cc6..51d4a56baa8fa 100644 --- a/modules/rest-compatibility/src/main/java/org/elasticsearch/rest/compat/version7/RestIndexActionV7.java +++ b/modules/rest-compatibility/src/main/java/org/elasticsearch/rest/compat/version7/RestIndexActionV7.java @@ -74,6 +74,7 @@ public boolean compatibilityRequired() { } public static class CompatibleCreateHandler extends RestIndexAction.CreateHandler { + @Override public String getName() { return "document_create_action_v7"; @@ -100,15 +101,16 @@ public boolean compatibilityRequired() { } public static final class CompatibleAutoIdHandler extends RestIndexAction.AutoIdHandler { - @Override - public String getName() { - return "document_create_action_auto_id_v7"; - } public CompatibleAutoIdHandler(Supplier nodesInCluster) { super(nodesInCluster); } + @Override + public String getName() { + return "document_create_action_auto_id_v7"; + } + @Override public List routes() { return singletonList(new Route(POST, "/{index}/{type}")); 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 cfda71f10096e..ecf7e86e748ef 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 @@ -101,4 +101,27 @@ public void testInvalidHeaderValue() throws IOException { assertThat(map.get("type"), equalTo("content_type_header_exception")); assertThat(map.get("reason"), equalTo("java.lang.IllegalArgumentException: invalid Content-Type header []")); } + + public void testInvalidHeaderCombinations() throws IOException { + final Request request = new Request("GET", "/_cluster/settings"); + final RequestOptions.Builder options = request.getOptions().toBuilder(); + options.addHeader("Content-Type", "application/vnd.elasticsearch+json;compatible-with=7"); + options.addHeader("Accept", "application/vnd.elasticsearch+json;compatible-with=8"); + request.setOptions(options); + request.setJsonEntity("{\"transient\":{\"search.*\":null}}"); + + final ResponseException e = expectThrows(ResponseException.class, () -> client().performRequest(request)); + 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("compatible_api_headers_combination_exception")); + assertThat(map.get("reason"), equalTo("Content-Type and Accept headers have to match when content is present. " + + "Accept=application/vnd.elasticsearch+json;compatible-with=8 " + + "Content-Type=application/vnd.elasticsearch+json;compatible-with=7 " + + "hasContent=true " + + "path=/_cluster/settings " + + "params={} " + + "method=GET")); + } } diff --git a/qa/rest-compat-tests/src/main/java/org/elasticsearch/rest/compat/AbstractCompatRestTest.java b/qa/rest-compat-tests/src/main/java/org/elasticsearch/rest/compat/AbstractCompatRestTest.java index 9cf2a780471fc..36bfd8b56c45f 100644 --- a/qa/rest-compat-tests/src/main/java/org/elasticsearch/rest/compat/AbstractCompatRestTest.java +++ b/qa/rest-compat-tests/src/main/java/org/elasticsearch/rest/compat/AbstractCompatRestTest.java @@ -33,6 +33,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.function.Consumer; import java.util.stream.StreamSupport; /** @@ -43,7 +44,6 @@ protected AbstractCompatRestTest(@Name("yaml") ClientYamlTestCandidate testCandi super(testCandidate); } - private static final Logger staticLogger = LogManager.getLogger(AbstractCompatRestTest.class); public static final String COMPAT_TESTS_PATH = "/rest-api-spec/test-compat"; @@ -66,38 +66,48 @@ public static Iterable createParameters() throws Exception { }); finalTestCandidates.add(testCandidates.toArray()); } - localCandidates.keySet().forEach(lc -> finalTestCandidates.add(new Object[]{lc})); + localCandidates.keySet().forEach(lc -> finalTestCandidates.add(new Object[] { lc })); return finalTestCandidates; } - private static void mutateTestCandidate(ClientYamlTestCandidate testCandidate) { - testCandidate.getTestSection().getExecutableSections().stream().filter(s -> s instanceof DoSection).forEach(ds -> { + testCandidate.getSetupSection().getExecutableSections().stream().filter(s -> s instanceof DoSection).forEach(updateDoSection()); + testCandidate.getTestSection().getExecutableSections().stream().filter(s -> s instanceof DoSection).forEach(updateDoSection()); + } + + private static Consumer updateDoSection() { + return ds -> { DoSection doSection = (DoSection) ds; - //TODO: be more selective here + // TODO: be more selective here doSection.setIgnoreWarnings(true); String compatibleHeader = createCompatibleHeader(); - //TODO decide which one to use - Accept or Content-Type - doSection.getApiCallSection() - .addHeaders(Map.of( - CompatibleConstants.COMPATIBLE_HEADER, compatibleHeader, - "Content-Type", compatibleHeader - )); - }); + // for cat apis accept headers would break tests which expect txt response + if (doSection.getApiCallSection().getApi().startsWith("cat") == false) { + doSection.getApiCallSection() + .addHeaders( + Map.of( + CompatibleConstants.COMPATIBLE_ACCEPT_HEADER, + compatibleHeader, + CompatibleConstants.COMPATIBLE_CONTENT_TYPE_HEADER, + compatibleHeader + ) + ); + } + + }; } private static String createCompatibleHeader() { return "application/vnd.elasticsearch+json;compatible-with=" + CompatibleConstants.COMPATIBLE_VERSION; } - private static Map getLocalCompatibilityTests() throws Exception { - Iterable candidates = - ESClientYamlSuiteTestCase.createParameters(ExecutableSection.XCONTENT_REGISTRY, COMPAT_TESTS_PATH); + Iterable candidates = ESClientYamlSuiteTestCase.createParameters(ExecutableSection.XCONTENT_REGISTRY, COMPAT_TESTS_PATH); Map localCompatibilityTests = new HashMap<>(); StreamSupport.stream(candidates.spliterator(), false) - .flatMap(Arrays::stream).forEach(o -> localCompatibilityTests.put((ClientYamlTestCandidate) o, (ClientYamlTestCandidate) o)); + .flatMap(Arrays::stream) + .forEach(o -> localCompatibilityTests.put((ClientYamlTestCandidate) o, (ClientYamlTestCandidate) o)); return localCompatibilityTests; } } diff --git a/server/src/main/java/org/elasticsearch/Version.java b/server/src/main/java/org/elasticsearch/Version.java index 168c823e3f2ac..99f4b57935c2b 100644 --- a/server/src/main/java/org/elasticsearch/Version.java +++ b/server/src/main/java/org/elasticsearch/Version.java @@ -342,6 +342,10 @@ public boolean isCompatible(Version version) { return compatible; } + public static Version minimumRestCompatibilityVersion(){ + return Version.V_7_0_0; + } + @SuppressForbidden(reason = "System.out.*") public static void main(String[] args) { final String versionOutput = String.format( diff --git a/server/src/main/java/org/elasticsearch/http/AbstractHttpServerTransport.java b/server/src/main/java/org/elasticsearch/http/AbstractHttpServerTransport.java index 8d02eac5b8b13..f30acc2e3e8b7 100644 --- a/server/src/main/java/org/elasticsearch/http/AbstractHttpServerTransport.java +++ b/server/src/main/java/org/elasticsearch/http/AbstractHttpServerTransport.java @@ -352,6 +352,9 @@ private void handleIncomingRequest(final HttpRequest httpRequest, final HttpChan } catch (final RestRequest.BadParameterException e) { badRequestCause = ExceptionsHelper.useOrSuppress(badRequestCause, e); innerRestRequest = RestRequest.requestWithoutParameters(xContentRegistry, httpRequest, httpChannel); + } catch (final RestRequest.CompatibleApiHeadersCombinationException e){ + badRequestCause = ExceptionsHelper.useOrSuppress(badRequestCause, e); + innerRestRequest = RestRequest.requestNoValidation(xContentRegistry, httpRequest, httpChannel); } restRequest = innerRestRequest; } @@ -384,12 +387,11 @@ private void handleIncomingRequest(final HttpRequest httpRequest, final HttpChan } private RestRequest requestWithoutContentTypeHeader(HttpRequest httpRequest, HttpChannel httpChannel, Exception badRequestCause) { - HttpRequest httpRequestWithoutContentType = httpRequest.removeHeader("Content-Type"); try { - return RestRequest.request(xContentRegistry, httpRequestWithoutContentType, httpChannel); + return RestRequest.requestWithoutContentType(xContentRegistry, httpRequest, httpChannel); } catch (final RestRequest.BadParameterException e) { badRequestCause.addSuppressed(e); - return RestRequest.requestWithoutParameters(xContentRegistry, httpRequestWithoutContentType, httpChannel); + return RestRequest.requestNoValidation(xContentRegistry, httpRequest, httpChannel); } } } diff --git a/server/src/main/java/org/elasticsearch/rest/CompatibleConstants.java b/server/src/main/java/org/elasticsearch/rest/CompatibleConstants.java index 78b62dac49ba7..2f3f13b893fe3 100644 --- a/server/src/main/java/org/elasticsearch/rest/CompatibleConstants.java +++ b/server/src/main/java/org/elasticsearch/rest/CompatibleConstants.java @@ -26,8 +26,9 @@ public class CompatibleConstants { /** * TODO revisit when https://github.com/elastic/elasticsearch/issues/52370 is resolved */ - public static final String COMPATIBLE_HEADER = "Accept"; + public static final String COMPATIBLE_ACCEPT_HEADER = "Accept"; + public static final String COMPATIBLE_CONTENT_TYPE_HEADER = "Content-Type"; public static final String COMPATIBLE_PARAMS_KEY = "Compatible-With"; - public static final String COMPATIBLE_VERSION = "" + (Version.CURRENT.major - 1); + public static final String COMPATIBLE_VERSION = String.valueOf(Version.minimumRestCompatibilityVersion().major); } diff --git a/server/src/main/java/org/elasticsearch/rest/RestRequest.java b/server/src/main/java/org/elasticsearch/rest/RestRequest.java index 63ebe4bf6e185..cd52770fd6683 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestRequest.java +++ b/server/src/main/java/org/elasticsearch/rest/RestRequest.java @@ -21,6 +21,7 @@ import org.apache.lucene.util.SetOnce; import org.elasticsearch.ElasticsearchParseException; +import org.elasticsearch.Version; import org.elasticsearch.common.Booleans; import org.elasticsearch.common.CheckedConsumer; import org.elasticsearch.common.Nullable; @@ -45,6 +46,7 @@ import java.util.HashMap; import java.util.HashSet; import java.util.List; +import java.util.Locale; import java.util.Map; import java.util.Set; import java.util.concurrent.atomic.AtomicLong; @@ -86,6 +88,12 @@ protected RestRequest(NamedXContentRegistry xContentRegistry, Map params, String path, Map> headers, HttpRequest httpRequest, HttpChannel httpChannel, long requestId) { + this(xContentRegistry, params, path, headers, httpRequest, httpChannel, requestId, true); + + } + private RestRequest(NamedXContentRegistry xContentRegistry, Map params, String path, + Map> headers, HttpRequest httpRequest, HttpChannel httpChannel, + long requestId, boolean headersValidation) { final XContentType xContentType; try { xContentType = parseContentType(headers.get("Content-Type")); @@ -102,7 +110,7 @@ private RestRequest(NamedXContentRegistry xContentRegistry, Map this.rawPath = path; this.headers = Collections.unmodifiableMap(headers); this.requestId = requestId; - addCompatibleParameter(); + addCompatibleParameter(headersValidation); } protected RestRequest(RestRequest restRequest) { @@ -132,29 +140,61 @@ void ensureSafeBuffers() { public static RestRequest request(NamedXContentRegistry xContentRegistry, HttpRequest httpRequest, HttpChannel httpChannel) { Map params = params(httpRequest.uri()); String path = path(httpRequest.uri()); - RestRequest restRequest = new RestRequest(xContentRegistry, params, path, httpRequest.getHeaders(), httpRequest, httpChannel, + return new RestRequest(xContentRegistry, params, path, httpRequest.getHeaders(), httpRequest, httpChannel, requestIdGenerator.incrementAndGet()); - return restRequest; } - private void addCompatibleParameter() { - if (isRequestCompatible()) { - String compatibleVersion = XContentType.parseVersion(header(CompatibleConstants.COMPATIBLE_HEADER)); - params().put(CompatibleConstants.COMPATIBLE_PARAMS_KEY, compatibleVersion); - //use it so it won't fail request validation with unused parameter - param(CompatibleConstants.COMPATIBLE_PARAMS_KEY); + private void addCompatibleParameter(boolean headersValidation) { + if(headersValidation){ + if(isRequestingCompatibility()) { + params().put(CompatibleConstants.COMPATIBLE_PARAMS_KEY, + String.valueOf(Version.minimumRestCompatibilityVersion().major)); + //use it so it won't fail request validation with unused parameter + param(CompatibleConstants.COMPATIBLE_PARAMS_KEY); + } } } + private boolean isRequestingCompatibility() { + String acceptHeader = header(CompatibleConstants.COMPATIBLE_ACCEPT_HEADER); + String aVersion = XContentType.parseVersion(acceptHeader); + byte acceptVersion = aVersion == null ? Version.CURRENT.major : Integer.valueOf(aVersion).byteValue(); + String contentTypeHeader = header(CompatibleConstants.COMPATIBLE_CONTENT_TYPE_HEADER); + String cVersion = XContentType.parseVersion(contentTypeHeader); + byte contentTypeVersion = cVersion == null ? Version.CURRENT.major : Integer.valueOf(cVersion).byteValue(); + + if(Version.CURRENT.major < acceptVersion || Version.CURRENT.major - acceptVersion > 1 ){ + throw new CompatibleApiHeadersCombinationException( + String.format(Locale.ROOT, "Unsupported version provided. " + + "Accept=%s Content-Type=%s hasContent=%b path=%s params=%s method=%s", acceptHeader, + contentTypeHeader, hasContent(), path(), params.toString(), method().toString())); + } + if (hasContent()) { + if(Version.CURRENT.major < contentTypeVersion || Version.CURRENT.major - contentTypeVersion > 1 ){ + throw new CompatibleApiHeadersCombinationException( + String.format(Locale.ROOT, "Unsupported version provided. " + + "Accept=%s Content-Type=%s hasContent=%b path=%s params=%s method=%s", acceptHeader, + contentTypeHeader, hasContent(), path(), params.toString(), method().toString())); + } - private boolean isRequestCompatible() { - return isHeaderCompatible(header(CompatibleConstants.COMPATIBLE_HEADER)); - } + if (contentTypeVersion != acceptVersion) { + throw new CompatibleApiHeadersCombinationException( + String.format(Locale.ROOT, "Content-Type and Accept headers have to match when content is present. " + + "Accept=%s Content-Type=%s hasContent=%b path=%s params=%s method=%s", acceptHeader, + contentTypeHeader, hasContent(), path(), params.toString(), method().toString())); + } + // both headers should be versioned or none + if ((cVersion == null && aVersion!=null) || (aVersion ==null && cVersion!=null) ){ + throw new CompatibleApiHeadersCombinationException( + String.format(Locale.ROOT, "Versioning is required on both Content-Type and Accept headers. " + + "Accept=%s Content-Type=%s hasContent=%b path=%s params=%s method=%s", acceptHeader, + contentTypeHeader, hasContent(), path(), params.toString(), method().toString())); + } - private boolean isHeaderCompatible(String headerValue) { - String version = XContentType.parseVersion(headerValue); - return CompatibleConstants.COMPATIBLE_VERSION.equals(version); - } + return contentTypeVersion < Version.CURRENT.major; + } + return acceptVersion < Version.CURRENT.major; + } private static Map params(final String uri) { final Map params = new HashMap<>(); @@ -191,9 +231,38 @@ public static RestRequest requestWithoutParameters(NamedXContentRegistry xConten HttpChannel httpChannel) { Map params = Collections.emptyMap(); return new RestRequest(xContentRegistry, params, httpRequest.uri(), httpRequest.getHeaders(), httpRequest, httpChannel, - requestIdGenerator.incrementAndGet()); + requestIdGenerator.incrementAndGet(), false); } + public static RestRequest requestWithoutContentType(NamedXContentRegistry xContentRegistry, HttpRequest httpRequest, + HttpChannel httpChannel) { + HttpRequest httpRequestWithoutContentType = httpRequest.removeHeader("Content-Type"); + Map params = params(httpRequest.uri()); + String path = path(httpRequest.uri()); + return new RestRequest(xContentRegistry, params, path, httpRequestWithoutContentType.getHeaders(), + httpRequestWithoutContentType, httpChannel, + requestIdGenerator.incrementAndGet(), false); + } + + /** + * creates a Rest request when it is not able to pass a validation but a response is needed to be returned. + * @param xContentRegistry the content registry + * @param httpRequest the http request + * @param httpChannel the http channel + * @return a RestRequest without headers and parameters + */ + public static RestRequest requestNoValidation(NamedXContentRegistry xContentRegistry, + HttpRequest httpRequest, + HttpChannel httpChannel) { + Map params = Collections.emptyMap(); + HttpRequest httpRequestWithoutContentType = httpRequest.removeHeader("Content-Type"); + + return new RestRequest(xContentRegistry, params, httpRequestWithoutContentType.uri(), + httpRequestWithoutContentType.getHeaders(), httpRequestWithoutContentType, httpChannel, + requestIdGenerator.incrementAndGet(), false); + } + + public enum Method { GET, POST, PUT, DELETE, OPTIONS, HEAD, PATCH, TRACE, CONNECT } @@ -559,4 +628,12 @@ public static class BadParameterException extends RuntimeException { } + public static class CompatibleApiHeadersCombinationException extends RuntimeException { + + CompatibleApiHeadersCombinationException(String cause) { + super(cause); + } + + } + } diff --git a/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java b/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java index 856e544a23055..3ceb93ef6abec 100644 --- a/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java +++ b/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java @@ -121,4 +121,21 @@ public void testVersionParsing() { assertThat(XContentType.parseVersion("APPLICATION/JSON"), nullValue()); } + + public void testVersionParsingOnText() { + String version = String.valueOf(Math.abs(randomByte())); + assertThat(XContentType.parseVersion("text/vnd.elasticsearch+csv;compatible-with=" + version), + equalTo(version)); + assertThat(XContentType.parseVersion("text/vnd.elasticsearch+text;compatible-with=" + version), + equalTo(version)); + assertThat(XContentType.parseVersion("text/vnd.elasticsearch+tab-separated-values;compatible-with=" + version), + equalTo(version)); + assertThat(XContentType.parseVersion("text/csv"), + nullValue()); + + assertThat(XContentType.parseVersion("TEXT/VND.ELASTICSEARCH+CSV;COMPATIBLE-WITH=" + version), + equalTo(version)); + assertThat(XContentType.parseVersion("TEXT/csv"), + nullValue()); + } } diff --git a/server/src/test/java/org/elasticsearch/rest/CompatibleHeaderCombinationTests.java b/server/src/test/java/org/elasticsearch/rest/CompatibleHeaderCombinationTests.java new file mode 100644 index 0000000000000..e38b7f9efab38 --- /dev/null +++ b/server/src/test/java/org/elasticsearch/rest/CompatibleHeaderCombinationTests.java @@ -0,0 +1,242 @@ +/* + * 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.rest; + +import org.elasticsearch.Version; +import org.elasticsearch.common.bytes.BytesArray; +import org.elasticsearch.common.xcontent.NamedXContentRegistry; +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.test.hamcrest.ElasticsearchMatchers; +import org.elasticsearch.test.rest.FakeRestRequest; +import org.hamcrest.Matcher; +import org.hamcrest.Matchers; + +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.not; +import static org.hamcrest.Matchers.nullValue; + +public class CompatibleHeaderCombinationTests extends ESTestCase { + int CURRENT_VERSION = Version.CURRENT.major; + int PREVIOUS_VERSION = Version.CURRENT.major - 1; + int OBSOLETE_VERSION = Version.CURRENT.major - 2; + + public void testAcceptAndContentTypeCombinations() { + assertThat(requestWith(acceptHeader(PREVIOUS_VERSION), contentTypeHeader(PREVIOUS_VERSION), bodyPresent()), + Matchers.allOf(requestCreated(), isCompatible())); + + assertThat(requestWith(acceptHeader(PREVIOUS_VERSION), contentTypeHeader(PREVIOUS_VERSION), bodyNotPresent()), + Matchers.allOf(requestCreated(), isCompatible())); + + expectThrows(RestRequest.CompatibleApiHeadersCombinationException.class, () -> + requestWith(acceptHeader(PREVIOUS_VERSION), contentTypeHeader(CURRENT_VERSION), bodyPresent())); + + // no body - content-type is ignored + assertThat(requestWith(acceptHeader(PREVIOUS_VERSION), contentTypeHeader(CURRENT_VERSION), bodyNotPresent()), + Matchers.allOf(requestCreated(), isCompatible())); + // no body - content-type is ignored + assertThat(requestWith(acceptHeader(CURRENT_VERSION), contentTypeHeader(PREVIOUS_VERSION), bodyNotPresent()), + Matchers.allOf(requestCreated(), not(isCompatible()))); + + expectThrows(RestRequest.CompatibleApiHeadersCombinationException.class, () -> + requestWith(acceptHeader(CURRENT_VERSION), contentTypeHeader(PREVIOUS_VERSION), bodyPresent())); + + assertThat(requestWith(acceptHeader(CURRENT_VERSION), contentTypeHeader(CURRENT_VERSION), bodyPresent()), + Matchers.allOf(requestCreated(), not(isCompatible()))); + + assertThat(requestWith(acceptHeader(CURRENT_VERSION), contentTypeHeader(CURRENT_VERSION), bodyNotPresent()), + Matchers.allOf(requestCreated(), not(isCompatible()))); + + //tests when body present and one of the headers missing - versioning is required on both when body is present + expectThrows(RestRequest.CompatibleApiHeadersCombinationException.class, () -> + requestWith(acceptHeader(PREVIOUS_VERSION), contentTypeHeader(null), bodyPresent())); + + expectThrows(RestRequest.CompatibleApiHeadersCombinationException.class, () -> + requestWith(acceptHeader(CURRENT_VERSION), contentTypeHeader(null), bodyPresent())); + + expectThrows(RestRequest.CompatibleApiHeadersCombinationException.class, () -> + requestWith(acceptHeader(null), contentTypeHeader(CURRENT_VERSION), bodyPresent())); + + expectThrows(RestRequest.CompatibleApiHeadersCombinationException.class, () -> + requestWith(acceptHeader(null), contentTypeHeader(PREVIOUS_VERSION), bodyPresent())); + + //tests when body NOT present and one of the headers missing + assertThat(requestWith(acceptHeader(PREVIOUS_VERSION), contentTypeHeader(null), bodyNotPresent()), + Matchers.allOf(requestCreated(), isCompatible())); + + assertThat(requestWith(acceptHeader(CURRENT_VERSION), contentTypeHeader(null), bodyNotPresent()), + Matchers.allOf(requestCreated(), not(isCompatible()))); + + //body not present - accept header is missing - it will default to Current version. Version on content type is ignored + assertThat(requestWith(acceptHeader(null), contentTypeHeader(PREVIOUS_VERSION), bodyNotPresent()), + Matchers.allOf(requestCreated(), not(isCompatible()))); + + assertThat(requestWith(acceptHeader(null), contentTypeHeader(CURRENT_VERSION), bodyNotPresent()), + Matchers.allOf(requestCreated(), not(isCompatible()))); + + assertThat(requestWith(acceptHeader(null), contentTypeHeader(null), bodyNotPresent()), + Matchers.allOf(requestCreated(), not(isCompatible()))); + + //Accept header = application/json means current version. If body is provided then accept and content-Type should be the same + assertThat(requestWith(acceptHeader("application/json"), contentTypeHeader(null), bodyNotPresent()), + Matchers.allOf(requestCreated(), not(isCompatible()))); + + assertThat(requestWith(acceptHeader("application/json"), contentTypeHeader("application/json"), bodyPresent()), + Matchers.allOf(requestCreated(), not(isCompatible()))); + + assertThat(requestWith(acceptHeader(null), contentTypeHeader("application/json"), bodyPresent()), + Matchers.allOf(requestCreated(), not(isCompatible()))); + } + + public void testObsoleteVersion() { + expectThrows(RestRequest.CompatibleApiHeadersCombinationException.class, () -> + requestWith(acceptHeader(OBSOLETE_VERSION), contentTypeHeader(OBSOLETE_VERSION), bodyPresent())); + + expectThrows(RestRequest.CompatibleApiHeadersCombinationException.class, () -> + requestWith(acceptHeader(OBSOLETE_VERSION), contentTypeHeader(null), bodyNotPresent())); + } + + + public void testMediaTypeCombinations() { + //body not present - ignore content-type + assertThat(requestWith(acceptHeader(null), contentTypeHeader(PREVIOUS_VERSION), bodyNotPresent()), + Matchers.allOf(requestCreated(), not(isCompatible()))); + + assertThat(requestWith(acceptHeader(null), contentTypeHeader("application/json"), bodyNotPresent()), + Matchers.allOf(requestCreated(), not(isCompatible()))); + + assertThat(requestWith(acceptHeader("*/*"), contentTypeHeader("application/json"), bodyNotPresent()), + Matchers.allOf(requestCreated(), not(isCompatible()))); + + //this is for instance used by SQL + assertThat(requestWith(acceptHeader("application/json"), contentTypeHeader("application/cbor"), bodyPresent()), + Matchers.allOf(requestCreated(), not(isCompatible()))); + + assertThat(requestWith(acceptHeader("application/vnd.elasticsearch+json;compatible-with=7"), + contentTypeHeader("application/vnd.elasticsearch+cbor;compatible-with=7"), bodyPresent()), + Matchers.allOf(requestCreated(), isCompatible())); + + //different versions on different media types + expectThrows(RestRequest.CompatibleApiHeadersCombinationException.class, () -> + requestWith(acceptHeader("application/vnd.elasticsearch+json;compatible-with=7"), + contentTypeHeader("application/vnd.elasticsearch+cbor;compatible-with=8"), bodyPresent())); + } + + public void testTextMediaTypes() { + assertThat(requestWith(acceptHeader("text/tab-separated-values"), contentTypeHeader("application/json"), bodyNotPresent()), + Matchers.allOf(requestCreated(), not(isCompatible()))); + + assertThat(requestWith(acceptHeader("text/plain"), contentTypeHeader("application/json"), bodyNotPresent()), + Matchers.allOf(requestCreated(), not(isCompatible()))); + + assertThat(requestWith(acceptHeader("text/csv"), contentTypeHeader("application/json"), bodyNotPresent()), + Matchers.allOf(requestCreated(), not(isCompatible()))); + + //versioned + assertThat(requestWith(acceptHeader("text/vnd.elasticsearch+tab-separated-values;compatible-with=7"), + contentTypeHeader(7), bodyNotPresent()), + Matchers.allOf(requestCreated(), isCompatible())); + + assertThat(requestWith(acceptHeader("text/vnd.elasticsearch+plain;compatible-with=7"), + contentTypeHeader(7), bodyNotPresent()), + Matchers.allOf(requestCreated(), isCompatible())); + + assertThat(requestWith(acceptHeader("text/vnd.elasticsearch+csv;compatible-with=7"), + contentTypeHeader(7), bodyNotPresent()), + Matchers.allOf(requestCreated(), isCompatible())); + } + + private Matcher requestCreated() { + return Matchers.not(nullValue(RestRequest.class)); + } + + private Matcher isCompatible() { + return requestHasVersion(Version.CURRENT.major - 1); + } + + private Matcher requestHasVersion(int version) { + return ElasticsearchMatchers.HasPropertyLambdaMatcher.hasProperty(build -> + build.param(CompatibleConstants.COMPATIBLE_PARAMS_KEY) //TODO to be refactored into getVersion + , equalTo(String.valueOf(version))); + } + + private String bodyNotPresent() { + return null; + } + + private String bodyPresent() { + return "some body"; + } + + private List contentTypeHeader(int version) { + return mediaType(version); + } + + private List acceptHeader(int version) { + return mediaType(version); + } + + private List acceptHeader(String value) { + return headerValue(value); + } + + private List contentTypeHeader(String value) { + return headerValue(value); + } + + private List headerValue(String value) { + if (value != null) { + return List.of(value); + } + return null; + } + + private List mediaType(Integer version) { + if (version != null) { + return List.of("application/vnd.elasticsearch+json;compatible-with=" + version); + } + return null; + } + + private RestRequest requestWith(List accept, List contentType, String body) { + FakeRestRequest.Builder builder = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY); + + builder.withHeaders(createHeaders(accept, contentType)); + if (body != null) { + // xContentType header is set explicitly in headers + builder.withContent(new BytesArray(body), null); + } + return builder.build(); + } + + private Map> createHeaders(List accept, List contentType) { + Map> headers = new HashMap<>(); + if (accept != null) { + headers.put("Accept", accept); + } + if (contentType != null) { + headers.put("Content-Type", contentType); + } + return headers; + } +} diff --git a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java index 22df41f20df83..4811dbc466623 100644 --- a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java +++ b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java @@ -58,6 +58,7 @@ import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; import java.util.stream.Collectors; +import java.util.stream.Stream; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; @@ -608,12 +609,13 @@ public HttpRequest releaseAndCopy() { public void testDispatchCompatibleHandler() { final byte version = (byte) (Version.CURRENT.major - 1); - final String mimeType = randomFrom("application/vnd.elasticsearch+json;compatible-with="+version); + final String mimeType = randomCompatibleMimeType(version); String content = randomAlphaOfLength((int) Math.round(BREAKER_LIMIT.getBytes() / inFlightRequestsBreaker.getOverhead())); - final List contentTypeHeader = Collections.singletonList(mimeType); + final List mimeTypeList = Collections.singletonList(mimeType); FakeRestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY) - .withContent(new BytesArray(content), RestRequest.parseContentType(contentTypeHeader)).withPath("/foo") - .withHeaders(Map.of("Content-Type", contentTypeHeader, "Accept", contentTypeHeader)) + .withContent(new BytesArray(content), RestRequest.parseContentType(mimeTypeList)) + .withPath("/foo") + .withHeaders(Map.of("Content-Type", mimeTypeList, "Accept", mimeTypeList)) .build(); AssertingChannel channel = new AssertingChannel(fakeRestRequest, true, RestStatus.OK); restController.registerHandler(RestRequest.Method.GET, "/foo", new RestHandler() { @@ -624,11 +626,6 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c channel.sendResponse(new BytesRestResponse(RestStatus.OK, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY)); } - @Override - public boolean supportsContentStream() { - return true; - } - @Override public boolean compatibilityRequired() { return true; @@ -640,6 +637,13 @@ public boolean compatibilityRequired() { assertTrue(channel.getSendResponseCalled()); } + private String randomCompatibleMimeType(byte version) { + String subtype = randomFrom(Stream.of(XContentType.values()) + .map(XContentType::shortName) + .toArray(String[]::new)); + return randomFrom("application/vnd.elasticsearch+" + subtype + ";compatible-with=" + version); + } + private static final class TestHttpServerTransport extends AbstractLifecycleComponent implements HttpServerTransport { 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 2f2f5fb76bfe7..093b5ce28651b 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 @@ -219,5 +219,18 @@ public FakeRestRequest build() { FakeHttpRequest fakeHttpRequest = new FakeHttpRequest(method, path, content, headers); return new FakeRestRequest(xContentRegistry, fakeHttpRequest, params, new FakeHttpChannel(address)); } + + @Override + public String toString() { + return "Builder{" + + "xContentRegistry=" + xContentRegistry + + ", headers=" + headers + + ", params=" + params + + ", content=" + content + + ", path='" + path + '\'' + + ", method=" + method + + ", address=" + address + + '}'; + } } } diff --git a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestClient.java b/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestClient.java index 64b61773461f6..0040116ff4fce 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestClient.java +++ b/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestClient.java @@ -23,6 +23,7 @@ import org.apache.http.HttpHost; import org.apache.http.client.methods.HttpGet; import org.apache.http.entity.ContentType; +import org.apache.http.protocol.HTTP; import org.apache.http.util.EntityUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -179,6 +180,11 @@ public ClientYamlTestResponse callApi(String apiName, Map params for (Map.Entry param : queryStringParams.entrySet()) { request.addParameter(param.getKey(), param.getValue()); } + // that means content type was set based on Content-Type from entity. + // Setting content-type on both entity and headers will result in exception in server + if (entity != null && entity.getContentType() != null) { + headers.remove(HTTP.CONTENT_TYPE); + } request.setEntity(entity); setOptions(request, headers); diff --git a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestExecutionContext.java b/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestExecutionContext.java index b1337172a5679..de36c25f2998b 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestExecutionContext.java +++ b/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestExecutionContext.java @@ -23,6 +23,7 @@ import org.apache.http.HttpEntity; import org.apache.http.entity.ByteArrayEntity; import org.apache.http.entity.ContentType; +import org.apache.http.protocol.HTTP; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.lucene.util.BytesRef; @@ -118,8 +119,10 @@ private HttpEntity createEntity(List> bodies, Map bytesRefList = new ArrayList<>(bodies.size()); @@ -137,7 +140,16 @@ private HttpEntity createEntity(List> bodies, Map headers, ByteArrayEntity byteArrayEntity) { + if (byteArrayEntity.getContentType() != null && headers.get(HTTP.CONTENT_TYPE) != null) { + byteArrayEntity.setContentType(headers.get(HTTP.CONTENT_TYPE)); } } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/TextFormat.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/TextFormat.java index dd837e65315aa..bde664f4ad4af 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/TextFormat.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/TextFormat.java @@ -258,6 +258,7 @@ boolean hasHeader(RestRequest request) { } static TextFormat fromMediaTypeOrFormat(String accept) { + //TODO we should include version parsing here too. This and XContentType should be unified somehow.. for (TextFormat text : values()) { String contentType = text.contentType(); if (contentType.equalsIgnoreCase(accept)