From e468fdf350c54bcfd63ee9a1f221232478c111bc Mon Sep 17 00:00:00 2001 From: pgomulka Date: Fri, 30 Oct 2020 10:28:47 +0100 Subject: [PATCH 01/21] Introduce per endpoint media types --- .../common/xcontent/MediaType.java | 27 ++- .../common/xcontent/MediaTypeParser.java | 160 ------------------ .../common/xcontent/MediaTypeRegistry.java | 73 ++++++++ .../common/xcontent/ParsedMediaType.java | 121 +++++++++++++ .../common/xcontent/XContentType.java | 94 +++++----- .../common/xcontent/MediaTypeParserTests.java | 77 --------- .../common/xcontent/ParsedMediaTypeTests.java | 121 +++++++++++++ .../rest/Netty4BadRequestIT.java | 2 +- .../rest/AbstractRestChannel.java | 2 +- .../org/elasticsearch/rest/RestHandler.java | 7 + .../org/elasticsearch/rest/RestRequest.java | 42 ++++- .../rest/action/cat/RestTable.java | 9 +- .../common/xcontent/XContentTypeTests.java | 29 ++-- .../rest/BytesRestResponseTests.java | 2 +- .../elasticsearch/rest/RestRequestTests.java | 4 +- .../rest/yaml/ClientYamlTestResponse.java | 12 +- .../security/rest/SecurityRestFilter.java | 7 + .../xpack/sql/plugin/RestSqlQueryAction.java | 8 +- .../xpack/sql/plugin/SqlMediaTypeParser.java | 34 +--- .../xpack/sql/plugin/TextFormat.java | 40 +++-- .../watcher/common/http/HttpResponse.java | 7 +- .../input/http/ExecutableHttpInput.java | 2 +- .../watcher/support/WatcherTemplateTests.java | 2 +- 23 files changed, 506 insertions(+), 376 deletions(-) delete mode 100644 libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeParser.java create mode 100644 libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeRegistry.java create mode 100644 libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ParsedMediaType.java delete mode 100644 libs/x-content/src/test/java/org/elasticsearch/common/xcontent/MediaTypeParserTests.java create mode 100644 libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ParsedMediaTypeTests.java diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaType.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaType.java index 2d61120288706..3136e8d94c4f5 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaType.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaType.java @@ -19,34 +19,31 @@ package org.elasticsearch.common.xcontent; +import org.elasticsearch.common.collect.Tuple; + +import java.util.Map; +import java.util.Set; + /** * Abstracts a Media Type and a format parameter. * Media types are used as values on Content-Type and Accept headers * format is an URL parameter, specifies response media type. */ public interface MediaType { - /** - * Returns a type part of a MediaType - * i.e. application for application/json - */ - String type(); + String COMPATIBLE_WITH_PARAMETER_NAME = "compatible-with"; + String VERSION_PATTERN = "\\d+"; - /** - * Returns a subtype part of a MediaType. - * i.e. json for application/json - */ - String subtype(); /** * Returns a corresponding format for a MediaType. i.e. json for application/json media type * Can differ from the MediaType's subtype i.e plain/text has a subtype of text but format is txt */ - String format(); + String formatPathParameter(); /** - * returns a string representation of a media type. + * returns a set of Tuples where a key is a sting - MediaType's type with subtype i.e application/json + * and a value is a map of parameters to be validated. + * Map's key is a parameter name, value is a parameter regex which is used for validation */ - default String typeWithSubtype(){ - return type() + "/" + subtype(); - } + Set>> mediaTypeMappings(); } diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeParser.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeParser.java deleted file mode 100644 index 62a3f3fd915d0..0000000000000 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeParser.java +++ /dev/null @@ -1,160 +0,0 @@ -/* - * 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.common.xcontent; - -import java.util.HashMap; -import java.util.Locale; -import java.util.Map; -import java.util.regex.Pattern; - -public class MediaTypeParser { - private final Map formatToMediaType; - private final Map typeWithSubtypeToMediaType; - private final Map> parametersMap; - - public MediaTypeParser(Map formatToMediaType, Map typeWithSubtypeToMediaType, - Map> parametersMap) { - this.formatToMediaType = Map.copyOf(formatToMediaType); - this.typeWithSubtypeToMediaType = Map.copyOf(typeWithSubtypeToMediaType); - this.parametersMap = Map.copyOf(parametersMap); - } - - public T fromMediaType(String mediaType) { - ParsedMediaType parsedMediaType = parseMediaType(mediaType); - return parsedMediaType != null ? parsedMediaType.getMediaType() : null; - } - - public T fromFormat(String format) { - if (format == null) { - return null; - } - return formatToMediaType.get(format.toLowerCase(Locale.ROOT)); - } - - /** - * parsing media type that follows https://tools.ietf.org/html/rfc7231#section-3.1.1.1 - * - * @param headerValue a header value from Accept or Content-Type - * @return a parsed media-type - */ - public ParsedMediaType parseMediaType(String headerValue) { - if (headerValue != null) { - String[] split = headerValue.toLowerCase(Locale.ROOT).split(";"); - - String[] typeSubtype = split[0].trim().toLowerCase(Locale.ROOT) - .split("/"); - if (typeSubtype.length == 2) { - - String type = typeSubtype[0]; - String subtype = typeSubtype[1]; - String typeWithSubtype = type + "/" + subtype; - T xContentType = typeWithSubtypeToMediaType.get(typeWithSubtype); - if (xContentType != null) { - Map parameters = new HashMap<>(); - for (int i = 1; i < split.length; i++) { - //spaces are allowed between parameters, but not between '=' sign - String[] keyValueParam = split[i].trim().split("="); - if (keyValueParam.length != 2 || hasSpaces(keyValueParam[0]) || hasSpaces(keyValueParam[1])) { - return null; - } - String parameterName = keyValueParam[0].toLowerCase(Locale.ROOT); - String parameterValue = keyValueParam[1].toLowerCase(Locale.ROOT); - if (isValidParameter(typeWithSubtype, parameterName, parameterValue) == false) { - return null; - } - parameters.put(parameterName, parameterValue); - } - return new ParsedMediaType(xContentType, parameters); - } - } - - } - return null; - } - - private boolean isValidParameter(String typeWithSubtype, String parameterName, String parameterValue) { - if (parametersMap.containsKey(typeWithSubtype)) { - Map parameters = parametersMap.get(typeWithSubtype); - if (parameters.containsKey(parameterName)) { - Pattern regex = parameters.get(parameterName); - return regex.matcher(parameterValue).matches(); - } - } - return false; - } - - private boolean hasSpaces(String s) { - return s.trim().equals(s) == false; - } - - /** - * A media type object that contains all the information provided on a Content-Type or Accept header - */ - public class ParsedMediaType { - private final Map parameters; - private final T mediaType; - - public ParsedMediaType(T mediaType, Map parameters) { - this.parameters = parameters; - this.mediaType = mediaType; - } - - public T getMediaType() { - return mediaType; - } - - public Map getParameters() { - return parameters; - } - } - - public static class Builder { - private final Map formatToMediaType = new HashMap<>(); - private final Map typeWithSubtypeToMediaType = new HashMap<>(); - private final Map> parametersMap = new HashMap<>(); - - public Builder withMediaTypeAndParams(String alternativeMediaType, T mediaType, Map paramNameAndValueRegex) { - typeWithSubtypeToMediaType.put(alternativeMediaType.toLowerCase(Locale.ROOT), mediaType); - formatToMediaType.put(mediaType.format(), mediaType); - - Map parametersForMediaType = new HashMap<>(paramNameAndValueRegex.size()); - for (Map.Entry params : paramNameAndValueRegex.entrySet()) { - String parameterName = params.getKey().toLowerCase(Locale.ROOT); - String parameterRegex = params.getValue(); - Pattern pattern = Pattern.compile(parameterRegex, Pattern.CASE_INSENSITIVE); - parametersForMediaType.put(parameterName, pattern); - } - parametersMap.put(alternativeMediaType, parametersForMediaType); - - return this; - } - - public Builder copyFromMediaTypeParser(MediaTypeParser mediaTypeParser) { - formatToMediaType.putAll(mediaTypeParser.formatToMediaType); - typeWithSubtypeToMediaType.putAll(mediaTypeParser.typeWithSubtypeToMediaType); - parametersMap.putAll(mediaTypeParser.parametersMap); - return this; - } - - public MediaTypeParser build() { - return new MediaTypeParser<>(formatToMediaType, typeWithSubtypeToMediaType, parametersMap); - } - } -} diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeRegistry.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeRegistry.java new file mode 100644 index 0000000000000..f9c3c7d8a511f --- /dev/null +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeRegistry.java @@ -0,0 +1,73 @@ +/* + * 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.common.xcontent; + +import org.elasticsearch.common.collect.Tuple; + +import java.util.HashMap; +import java.util.Locale; +import java.util.Map; +import java.util.Set; +import java.util.regex.Pattern; + +public class MediaTypeRegistry { + + private Map formatToMediaType = new HashMap<>(); + private Map typeWithSubtypeToMediaType = new HashMap<>(); + private Map> parametersMap = new HashMap<>(); + + public T formatToMediaType(String format) { + if(format == null) { + return null; + } + return formatToMediaType.get(format.toLowerCase(Locale.ROOT)); + } + + public T typeWithSubtypeToMediaType(String typeWithSubtype) { + return typeWithSubtypeToMediaType.get(typeWithSubtype.toLowerCase(Locale.ROOT)); + } + + public Map parametersFor(String typeWithSubtype) { + return parametersMap.get(typeWithSubtype); + } + + public MediaTypeRegistry register(T[] mediaTypes ) { + for (T mediaType : mediaTypes) { + Set>> tuples = mediaType.mediaTypeMappings(); + for (Tuple> tuple : tuples) { + formatToMediaType.put(mediaType.formatPathParameter(),mediaType); + typeWithSubtypeToMediaType.put(tuple.v1(), mediaType); + parametersMap.put(tuple.v1(), convertPatterns(tuple.v2())); + } + } + return this; + } + + private Map convertPatterns(Map paramNameAndValueRegex){ + Map parametersForMediaType = new HashMap<>(paramNameAndValueRegex.size()); + for (Map.Entry params : paramNameAndValueRegex.entrySet()) { + String parameterName = params.getKey().toLowerCase(Locale.ROOT); + String parameterRegex = params.getValue(); + Pattern pattern = Pattern.compile(parameterRegex, Pattern.CASE_INSENSITIVE); + parametersForMediaType.put(parameterName, pattern); + } + return parametersForMediaType; + } +} diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ParsedMediaType.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ParsedMediaType.java new file mode 100644 index 0000000000000..852b72fb503ff --- /dev/null +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ParsedMediaType.java @@ -0,0 +1,121 @@ +/* + * 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.common.xcontent; + +import java.util.Collections; +import java.util.HashMap; +import java.util.Locale; +import java.util.Map; +import java.util.regex.Pattern; + +public class ParsedMediaType { + //sun.net.www.protocol.http.HttpURLConnection sets a default Accept header if it was not provided on a request + public static final String DEFAULT_ACCEPT_STRING = "text/html, image/gif, image/jpeg, *; q=.2, */*; q=.2"; + + private final String type; + private final String subType; + private final Map parameters; + // tchar pattern as defined by RFC7230 section 3.2.6 + private static final Pattern TCHAR_PATTERN = Pattern.compile("[a-zA-z0-9!#$%&'*+\\-.\\^_`|~]+"); + + private ParsedMediaType(String type, String subType, Map parameters) { + this.type = type; + this.subType = subType; + this.parameters = Collections.unmodifiableMap(parameters); + } + + /** + * The parsed mime type without the associated parameters. Will always return lowercase. + */ + public String mimeTypeWithoutParams() { + return type + "/" + subType; + } + + public Map getParameters() { + return parameters; + } + + /** + * Parses a header value into it's parts. + * + * @return a {@link ParsedMediaType} if the header could be parsed. TODO: don't return null + * @throws IllegalArgumentException if the header is malformed + */ + public static ParsedMediaType parseMediaType(String headerValue) { + if (DEFAULT_ACCEPT_STRING.equals(headerValue) || "*/*".equals(headerValue)) { + return null; + } + if (headerValue != null) { + final String[] elements = headerValue.toLowerCase(Locale.ROOT).split("[\\s\\t]*;"); + + final String[] splitMediaType = elements[0].split("/"); + if ((splitMediaType.length == 2 && TCHAR_PATTERN.matcher(splitMediaType[0].trim()).matches() + && TCHAR_PATTERN.matcher(splitMediaType[1].trim()).matches()) == false) { + throw new IllegalArgumentException("invalid media type [" + headerValue + "]"); + } + if (elements.length == 1) { + return new ParsedMediaType(splitMediaType[0].trim(), splitMediaType[1].trim(), Collections.emptyMap()); + } else { + Map parameters = new HashMap<>(); + for (int i = 1; i < elements.length; i++) { + String paramsAsString = elements[i].trim(); + if (paramsAsString.isEmpty()) { + continue; + } + String[] keyValueParam = elements[i].trim().split("="); + if (keyValueParam.length == 2) { + String parameterName = keyValueParam[0].toLowerCase(Locale.ROOT).trim(); + String parameterValue = keyValueParam[1].toLowerCase(Locale.ROOT).trim(); + parameters.put(parameterName, parameterValue); + } else { + throw new IllegalArgumentException("invalid parameters for header [" + headerValue + "]"); + } + } + return new ParsedMediaType(splitMediaType[0].trim().toLowerCase(Locale.ROOT), + splitMediaType[1].trim().toLowerCase(Locale.ROOT), parameters); + } + } + return null; + } + + public T toMediaType(MediaTypeRegistry mediaTypeRegistry) { + T type = mediaTypeRegistry.typeWithSubtypeToMediaType(mimeTypeWithoutParams()); + if (type != null) { + + Map registeredParams = mediaTypeRegistry.parametersFor(mimeTypeWithoutParams()); + for (Map.Entry givenParamEntry : parameters.entrySet()) { + if (isValidParameter(givenParamEntry.getKey(), givenParamEntry.getValue(), registeredParams) == false) { + return null; + } + } + return type; + } + return null; + } + + private boolean isValidParameter(String paramName, String value, Map registeredParams) { + if(registeredParams.containsKey(paramName)){ + Pattern regex = registeredParams.get(paramName); + return regex.matcher(value).matches(); + } + // undefined parameters are not allowed + return false; + } +} 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 076a20bad006a..8148d7c323a43 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 @@ -19,6 +19,7 @@ package org.elasticsearch.common.xcontent; +import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.xcontent.cbor.CborXContent; import org.elasticsearch.common.xcontent.json.JsonXContent; import org.elasticsearch.common.xcontent.smile.SmileXContent; @@ -26,6 +27,7 @@ import java.util.Collections; import java.util.Map; +import java.util.Set; /** * The content type of {@link org.elasticsearch.common.xcontent.XContent}. @@ -47,7 +49,7 @@ public String mediaType() { } @Override - public String subtype() { + public String formatPathParameter() { return "json"; } @@ -55,6 +57,18 @@ public String subtype() { public XContent xContent() { return JsonXContent.jsonXContent; } + + @Override + public Set>> mediaTypeMappings() { + return Set.of( + Tuple.tuple("application/json", Map.of("charset", "UTF-8")), + Tuple.tuple("application/x-ndjson", Map.of("charset", "UTF-8")), + Tuple.tuple("application/*", Collections.emptyMap()), + Tuple.tuple("application/vnd.elasticsearch+json", + Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", "UTF-8")), + Tuple.tuple("application/vnd.elasticsearch+x-ndjson", + Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", "UTF-8"))); + } }, /** * The jackson based smile binary format. Fast and compact binary format. @@ -66,7 +80,7 @@ public String mediaTypeWithoutParameters() { } @Override - public String subtype() { + public String formatPathParameter() { return "smile"; } @@ -74,6 +88,14 @@ public String subtype() { public XContent xContent() { return SmileXContent.smileXContent; } + + @Override + public Set>> mediaTypeMappings() { + return Set.of( + Tuple.tuple("application/smile", Collections.emptyMap()), + Tuple.tuple("application/vnd.elasticsearch+smile", + Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN))); + } }, /** * A YAML based content type. @@ -85,7 +107,7 @@ public String mediaTypeWithoutParameters() { } @Override - public String subtype() { + public String formatPathParameter() { return "yaml"; } @@ -93,6 +115,14 @@ public String subtype() { public XContent xContent() { return YamlXContent.yamlXContent; } + + @Override + public Set>> mediaTypeMappings() { + return Set.of( + Tuple.tuple("application/yaml", Map.of("charset", "UTF-8")), + Tuple.tuple("application/vnd.elasticsearch+yaml", + Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", "UTF-8"))); + } }, /** * A CBOR based content type. @@ -104,7 +134,7 @@ public String mediaTypeWithoutParameters() { } @Override - public String subtype() { + public String formatPathParameter() { return "cbor"; } @@ -112,37 +142,27 @@ public String subtype() { public XContent xContent() { return CborXContent.cborXContent; } + + @Override + public Set>> mediaTypeMappings() { + return Set.of( + Tuple.tuple("application/cbor", Collections.emptyMap()), + Tuple.tuple("application/vnd.elasticsearch+cbor", + Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN))); + } }; - private static final String COMPATIBLE_WITH_PARAMETER_NAME = "compatible-with"; - private static final String VERSION_PATTERN = "\\d+"; - public static final MediaTypeParser mediaTypeParser = new MediaTypeParser.Builder() - .withMediaTypeAndParams("application/smile", SMILE, Collections.emptyMap()) - .withMediaTypeAndParams("application/cbor", CBOR, Collections.emptyMap()) - .withMediaTypeAndParams("application/json", JSON, Map.of("charset", "UTF-8")) - .withMediaTypeAndParams("application/yaml", YAML, Map.of("charset", "UTF-8")) - .withMediaTypeAndParams("application/*", JSON, Map.of("charset", "UTF-8")) - .withMediaTypeAndParams("application/x-ndjson", JSON, Map.of("charset", "UTF-8")) - .withMediaTypeAndParams("application/vnd.elasticsearch+json", JSON, - Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", "UTF-8")) - .withMediaTypeAndParams("application/vnd.elasticsearch+smile", SMILE, - Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", "UTF-8")) - .withMediaTypeAndParams("application/vnd.elasticsearch+yaml", YAML, - Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", "UTF-8")) - .withMediaTypeAndParams("application/vnd.elasticsearch+cbor", CBOR, - Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", "UTF-8")) - .withMediaTypeAndParams("application/vnd.elasticsearch+x-ndjson", JSON, - Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", "UTF-8")) - .build(); + public static final MediaTypeRegistry MEDIA_TYPE_REGISTRY = new MediaTypeRegistry() + .register(XContentType.values()); /** - * Accepts a format string, which is most of the time is equivalent to {@link XContentType#subtype()} + * Accepts a format string, which is most of the time is equivalent to MediaType's subtype i.e. application/json * and attempts to match the value to an {@link XContentType}. * The comparisons are done in lower case format. * This method will return {@code null} if no match is found */ - public static XContentType fromFormat(String mediaType) { - return mediaTypeParser.fromFormat(mediaType); + public static XContentType fromFormat(String format) { + return MEDIA_TYPE_REGISTRY.formatToMediaType(format); } /** @@ -151,8 +171,13 @@ public static XContentType fromFormat(String mediaType) { * This method is suitable for parsing of the {@code Content-Type} and {@code Accept} HTTP headers. * This method will return {@code null} if no match is found */ - public static XContentType fromMediaType(String mediaTypeHeaderValue) { - return mediaTypeParser.fromMediaType(mediaTypeHeaderValue); + public static XContentType fromMediaType(String mediaTypeHeaderValue) throws IllegalArgumentException { + ParsedMediaType parsedMediaType = ParsedMediaType.parseMediaType(mediaTypeHeaderValue); + if (parsedMediaType != null) { + return parsedMediaType + .toMediaType(MEDIA_TYPE_REGISTRY); + } + return null; } private int index; @@ -162,7 +187,7 @@ public static XContentType fromMediaType(String mediaTypeHeaderValue) { } public static Byte parseVersion(String mediaType) { - MediaTypeParser.ParsedMediaType parsedMediaType = mediaTypeParser.parseMediaType(mediaType); + ParsedMediaType parsedMediaType = ParsedMediaType.parseMediaType(mediaType); if (parsedMediaType != null) { String version = parsedMediaType .getParameters() @@ -186,13 +211,4 @@ public String mediaType() { public abstract String mediaTypeWithoutParameters(); - @Override - public String type() { - return "application"; - } - - @Override - public String format() { - return subtype(); - } } diff --git a/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/MediaTypeParserTests.java b/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/MediaTypeParserTests.java deleted file mode 100644 index 08ca08a3d2240..0000000000000 --- a/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/MediaTypeParserTests.java +++ /dev/null @@ -1,77 +0,0 @@ -/* - * 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.common.xcontent; - -import org.elasticsearch.test.ESTestCase; - -import java.util.Collections; -import java.util.Map; - -import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.is; -import static org.hamcrest.Matchers.nullValue; - -public class MediaTypeParserTests extends ESTestCase { - - MediaTypeParser mediaTypeParser = new MediaTypeParser.Builder() - .withMediaTypeAndParams("application/vnd.elasticsearch+json", - XContentType.JSON, Map.of("compatible-with", "\\d+", - "charset", "UTF-8")) - .build(); - - public void testJsonWithParameters() throws Exception { - String mediaType = "application/vnd.elasticsearch+json"; - assertThat(mediaTypeParser.parseMediaType(mediaType).getParameters(), - equalTo(Collections.emptyMap())); - assertThat(mediaTypeParser.parseMediaType(mediaType + ";").getParameters(), - equalTo(Collections.emptyMap())); - assertThat(mediaTypeParser.parseMediaType(mediaType + "; charset=UTF-8").getParameters(), - equalTo(Map.of("charset", "utf-8"))); - assertThat(mediaTypeParser.parseMediaType(mediaType + "; compatible-with=123;charset=UTF-8").getParameters(), - equalTo(Map.of("charset", "utf-8", "compatible-with", "123"))); - } - - public void testWhiteSpaceInTypeSubtype() { - String mediaType = " application/vnd.elasticsearch+json "; - assertThat(mediaTypeParser.parseMediaType(mediaType).getMediaType(), - equalTo(XContentType.JSON)); - - assertThat(mediaTypeParser.parseMediaType(mediaType + "; compatible-with=123; charset=UTF-8").getParameters(), - equalTo(Map.of("charset", "utf-8", "compatible-with", "123"))); - assertThat(mediaTypeParser.parseMediaType(mediaType + "; compatible-with=123;\n charset=UTF-8").getParameters(), - equalTo(Map.of("charset", "utf-8", "compatible-with", "123"))); - - mediaType = " application / json "; - assertThat(mediaTypeParser.parseMediaType(mediaType), - is(nullValue())); - } - - public void testInvalidParameters() { - String mediaType = "application/vnd.elasticsearch+json"; - assertThat(mediaTypeParser.parseMediaType(mediaType + "; charset=unknown") , - is(nullValue())); - assertThat(mediaTypeParser.parseMediaType(mediaType + "; keyvalueNoEqualsSign"), - is(nullValue())); - assertThat(mediaTypeParser.parseMediaType(mediaType + "; key = value"), - is(nullValue())); - assertThat(mediaTypeParser.parseMediaType(mediaType + "; key=") , - is(nullValue())); - } -} diff --git a/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ParsedMediaTypeTests.java b/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ParsedMediaTypeTests.java new file mode 100644 index 0000000000000..3995dd391be1b --- /dev/null +++ b/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ParsedMediaTypeTests.java @@ -0,0 +1,121 @@ +/* + * 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.common.xcontent; + +import org.elasticsearch.test.ESTestCase; + +import java.util.Collections; +import java.util.Map; + +import static org.hamcrest.Matchers.equalTo; + +public class ParsedMediaTypeTests extends ESTestCase { + + MediaTypeRegistry mediaTypeRegistry = new MediaTypeRegistry() + .register(XContentType.values()); + + public void testJsonWithParameters() throws Exception { + String mediaType = "application/vnd.elasticsearch+json"; + assertThat(ParsedMediaType.parseMediaType(mediaType).getParameters(), + equalTo(Collections.emptyMap())); + assertThat(ParsedMediaType.parseMediaType(mediaType + ";").getParameters(), + equalTo(Collections.emptyMap())); + assertThat(ParsedMediaType.parseMediaType(mediaType + "; charset=UTF-8").getParameters(), + equalTo(Map.of("charset", "utf-8"))); + assertThat(ParsedMediaType.parseMediaType(mediaType + "; compatible-with=123;charset=UTF-8").getParameters(), + equalTo(Map.of("charset", "utf-8", "compatible-with", "123"))); + } + + public void testWhiteSpaceInTypeSubtype() { + String mediaType = " application/vnd.elasticsearch+json "; + assertThat(ParsedMediaType.parseMediaType(mediaType).toMediaType(mediaTypeRegistry), + equalTo(XContentType.JSON)); + + assertThat(ParsedMediaType.parseMediaType(mediaType + "; compatible-with=123; charset=UTF-8").getParameters(), + equalTo(Map.of("charset", "utf-8", "compatible-with", "123"))); + assertThat(ParsedMediaType.parseMediaType(mediaType + "; compatible-with=123;\n charset=UTF-8").getParameters(), + equalTo(Map.of("charset", "utf-8", "compatible-with", "123"))); + + + } + +// public void testInvalidParameters() { +// String mediaType = "application/vnd.elasticsearch+json"; +// assertThat(ParsedMediaType.parseMediaType(mediaType + "; charset=unknown") +// .toMediaType(mediaTypeRegistry), +// is(nullValue())); +// assertThat(ParsedMediaType.parseMediaType(mediaType + "; keyvalueNoEqualsSign") +// .toMediaType(mediaTypeRegistry), +// is(nullValue())); +// assertThat(ParsedMediaType.parseMediaType(mediaType + "; key = value") +// .toMediaType(mediaTypeRegistry), +// is(nullValue())); +// assertThat(ParsedMediaType.parseMediaType(mediaType + "; key=") +// .toMediaType(mediaTypeRegistry), +// is(nullValue())); +// } + + public void testXContentTypes() { + for (XContentType xContentType : XContentType.values()) { + ParsedMediaType parsedMediaType = ParsedMediaType.parseMediaType(xContentType.mediaTypeWithoutParameters()); + assertEquals(xContentType.mediaTypeWithoutParameters(), parsedMediaType.mimeTypeWithoutParams()); + } + } + + public void testWithParameters() { + String mediaType = "application/foo"; + assertEquals(Collections.emptyMap(), ParsedMediaType.parseMediaType(mediaType).getParameters()); + assertEquals(Collections.emptyMap(), ParsedMediaType.parseMediaType(mediaType + ";").getParameters()); + assertEquals(Map.of("charset", "utf-8"), ParsedMediaType.parseMediaType(mediaType + "; charset=UTF-8").getParameters()); + assertEquals(Map.of("charset", "utf-8", "compatible-with", "123"), + ParsedMediaType.parseMediaType(mediaType + "; compatible-with=123;charset=UTF-8").getParameters()); + } + + public void testWhiteSpaces() { + //be lenient with white space since it can be really hard to troubleshoot + String mediaType = " application/foo "; + ParsedMediaType parsedMediaType = ParsedMediaType.parseMediaType(mediaType + " ; compatible-with = 123 ; charset=UTF-8"); + assertEquals("application/foo", parsedMediaType.mimeTypeWithoutParams()); + assertEquals((Map.of("charset", "utf-8", "compatible-with", "123")), parsedMediaType.getParameters()); + } + + public void testEmptyParams() { + String mediaType = "application/foo"; + ParsedMediaType parsedMediaType = ParsedMediaType.parseMediaType(mediaType + randomFrom("", " ", ";", ";;", ";;;")); + assertEquals("application/foo", parsedMediaType.mimeTypeWithoutParams()); + assertEquals(Collections.emptyMap(), parsedMediaType.getParameters()); + } + + public void testMalformedParameters() { + String mediaType = "application/foo"; + IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, + () -> ParsedMediaType.parseMediaType(mediaType + "; charsetunknown")); + assertThat(exception.getMessage(), equalTo("invalid parameters for header [application/foo; charsetunknown]")); + + exception = expectThrows(IllegalArgumentException.class, + () -> ParsedMediaType.parseMediaType(mediaType + "; char=set=unknown")); + assertThat(exception.getMessage(), equalTo("invalid parameters for header [application/foo; char=set=unknown]")); + } + +// public void testMultipleValues() { +// String mediaType = "text/html, image/gif, image/jpeg, *; q=.2, */*; q=.2"; +// ParsedMediaType.parseMediaType(mediaType); +// } +} diff --git a/modules/transport-netty4/src/javaRestTest/java/org/elasticsearch/rest/Netty4BadRequestIT.java b/modules/transport-netty4/src/javaRestTest/java/org/elasticsearch/rest/Netty4BadRequestIT.java index cfda71f10096e..70cf295cb54a0 100644 --- a/modules/transport-netty4/src/javaRestTest/java/org/elasticsearch/rest/Netty4BadRequestIT.java +++ b/modules/transport-netty4/src/javaRestTest/java/org/elasticsearch/rest/Netty4BadRequestIT.java @@ -99,6 +99,6 @@ public void testInvalidHeaderValue() throws IOException { final ObjectPath objectPath = ObjectPath.createFromResponse(response); final Map map = objectPath.evaluate("error"); assertThat(map.get("type"), equalTo("content_type_header_exception")); - assertThat(map.get("reason"), equalTo("java.lang.IllegalArgumentException: invalid Content-Type header []")); + assertThat(map.get("reason"), equalTo("java.lang.IllegalArgumentException: Header [Content-Type] cannot be empty.")); } } diff --git a/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java b/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java index 6f5aa618ae4d0..f08bf5b58e8b4 100644 --- a/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java +++ b/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java @@ -104,7 +104,7 @@ public XContentBuilder newBuilder(@Nullable XContentType requestContentType, @Nu if (Strings.hasText(format)) { responseContentType = XContentType.fromFormat(format); } - if (responseContentType == null) { + if (responseContentType == null && Strings.hasText(acceptHeader)) { responseContentType = XContentType.fromMediaType(acceptHeader); } } diff --git a/server/src/main/java/org/elasticsearch/rest/RestHandler.java b/server/src/main/java/org/elasticsearch/rest/RestHandler.java index 054c618876314..21d4a6f278f3c 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestHandler.java +++ b/server/src/main/java/org/elasticsearch/rest/RestHandler.java @@ -20,7 +20,10 @@ package org.elasticsearch.rest; import org.elasticsearch.client.node.NodeClient; +import org.elasticsearch.common.xcontent.MediaType; +import org.elasticsearch.common.xcontent.MediaTypeRegistry; import org.elasticsearch.common.xcontent.XContent; +import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.rest.RestRequest.Method; import java.util.Collections; @@ -99,6 +102,10 @@ default boolean allowSystemIndexAccessByDefault() { return false; } + default MediaTypeRegistry validAcceptMediaTypes(){ + return XContentType.MEDIA_TYPE_REGISTRY; + } + class Route { private final String path; diff --git a/server/src/main/java/org/elasticsearch/rest/RestRequest.java b/server/src/main/java/org/elasticsearch/rest/RestRequest.java index 512bf72e9c0d3..26dd9e7776a6d 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestRequest.java +++ b/server/src/main/java/org/elasticsearch/rest/RestRequest.java @@ -32,6 +32,7 @@ import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; import org.elasticsearch.common.xcontent.NamedXContentRegistry; +import org.elasticsearch.common.xcontent.ParsedMediaType; import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentType; @@ -68,7 +69,8 @@ public class RestRequest implements ToXContent.Params { private final Set consumedParams = new HashSet<>(); private final SetOnce xContentType = new SetOnce<>(); private final HttpChannel httpChannel; - + private final ParsedMediaType parsedAccept; + private final ParsedMediaType parsedContentType; private HttpRequest httpRequest; private boolean contentConsumed = false; @@ -86,15 +88,15 @@ protected RestRequest(NamedXContentRegistry xContentRegistry, Map params, String path, Map> headers, HttpRequest httpRequest, HttpChannel httpChannel, long requestId) { - final XContentType xContentType; - try { - xContentType = parseContentType(headers.get("Content-Type")); - } catch (final IllegalArgumentException e) { + try{ + this.parsedAccept = parseHeaderWithMediaType(httpRequest.getHeaders(), "Accept"); + this.parsedContentType = parseHeaderWithMediaType(httpRequest.getHeaders(), "Content-Type"); + if (parsedContentType != null) { + this.xContentType.set(parsedContentType.toMediaType(XContentType.MEDIA_TYPE_REGISTRY)); + } + }catch (IllegalArgumentException e){ throw new ContentTypeHeaderException(e); } - if (xContentType != null) { - this.xContentType.set(xContentType); - } this.xContentRegistry = xContentRegistry; this.httpRequest = httpRequest; this.httpChannel = httpChannel; @@ -104,6 +106,22 @@ private RestRequest(NamedXContentRegistry xContentRegistry, Map this.requestId = requestId; } + private static @Nullable ParsedMediaType parseHeaderWithMediaType(Map> headers, String headerName) { + //TOOD: shouldn't this be case insensitive ? + List header = headers.get(headerName); + if (header == null || header.isEmpty()) { + return null; + } else if (header.size() > 1) { + throw new IllegalArgumentException("only one value for the header should be provided"); + } + String rawContentType = header.get(0); + if (Strings.hasText(rawContentType)) { + return ParsedMediaType.parseMediaType(rawContentType); + } else { + throw new IllegalArgumentException("Header [" + headerName + "] cannot be empty."); + } + } + protected RestRequest(RestRequest restRequest) { this(restRequest.getXContentRegistry(), restRequest.params(), restRequest.path(), restRequest.getHeaders(), restRequest.getHttpRequest(), restRequest.getHttpChannel(), restRequest.getRequestId()); @@ -497,6 +515,14 @@ public final Tuple contentOrSourceParam() { return new Tuple<>(xContentType, bytes); } + public ParsedMediaType getParsedAccept() { + return parsedAccept; + } + + public ParsedMediaType getParsedContentType() { + return parsedContentType; + } + /** * Parses the given content type string for the media type. This method currently ignores parameters. */ diff --git a/server/src/main/java/org/elasticsearch/rest/action/cat/RestTable.java b/server/src/main/java/org/elasticsearch/rest/action/cat/RestTable.java index bbb746baafe92..0587e633867e6 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/cat/RestTable.java +++ b/server/src/main/java/org/elasticsearch/rest/action/cat/RestTable.java @@ -51,18 +51,21 @@ public class RestTable { public static RestResponse buildResponse(Table table, RestChannel channel) throws Exception { RestRequest request = channel.request(); - XContentType xContentType = getXContentType(request); + XContentType xContentType = getResponseContentType(request); if (xContentType != null) { return buildXContentBuilder(table, channel); } return buildTextPlainResponse(table, channel); } - private static XContentType getXContentType(RestRequest request) { + private static XContentType getResponseContentType(RestRequest request) { if (request.hasParam("format")) { return XContentType.fromFormat(request.param("format")); } - return XContentType.fromMediaType(request.header("Accept")); + if(request.getParsedAccept() != null){ + return request.getParsedAccept().toMediaType(XContentType.MEDIA_TYPE_REGISTRY); + } + return null; } public static RestResponse buildXContentBuilder(Table table, RestChannel channel) throws Exception { 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 7b18eaa2bad38..741f380a8cc0f 100644 --- a/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java +++ b/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java @@ -91,9 +91,9 @@ public void testFromWildcardUppercase() throws Exception { public void testFromRubbish() throws Exception { assertThat(XContentType.fromMediaType(null), nullValue()); - assertThat(XContentType.fromMediaType(""), nullValue()); + expectThrows(IllegalArgumentException.class, ()->XContentType.fromMediaType("")); + expectThrows(IllegalArgumentException.class, ()->XContentType.fromMediaType("gobbly;goop")); assertThat(XContentType.fromMediaType("text/plain"), nullValue()); - assertThat(XContentType.fromMediaType("gobbly;goop"), nullValue()); } public void testVersionedMediaType() { @@ -116,6 +116,8 @@ public void testVersionedMediaType() { equalTo(XContentType.JSON)); assertThat(XContentType.fromMediaType("APPLICATION/JSON"), equalTo(XContentType.JSON)); + + } public void testVersionParsing() { @@ -137,23 +139,20 @@ public void testVersionParsing() { assertThat(XContentType.parseVersion("APPLICATION/JSON"), nullValue()); - assertThat(XContentType.parseVersion("application/json;compatible-with=" + version + ".0"), + //validation is done when parsing a MediaType + assertThat(XContentType.fromMediaType("application/vnd.elasticsearch+json;compatible-with=" + version + ".0"), is(nullValue())); + assertThat(XContentType.fromMediaType("application/vnd.elasticsearch+json;compatible-with=" + version + "_sth"), + nullValue()); } - public void testUnrecognizedParameter() { - assertThat(XContentType.parseVersion("application/json; sth=123"), - is(nullValue())); } - - public void testMediaTypeWithoutESSubtype() { + public void testUnrecognizedParameters() { String version = String.valueOf(randomNonNegativeByte()); - assertThat(XContentType.fromMediaType("application/json;compatible-with=" + version), nullValue()); - } - public void testAnchoring() { - String version = String.valueOf(randomNonNegativeByte()); - assertThat(XContentType.fromMediaType("sth_application/json;compatible-with=" + version + ".0"), nullValue()); - assertThat(XContentType.fromMediaType("sth_application/json;compatible-with=" + version + "_sth"), nullValue()); - assertThat(XContentType.fromMediaType("application/json;compatible-with=" + version + "_sth"), nullValue()); + assertThat(XContentType.fromMediaType("application/json;compatible-with=" + version), + is(nullValue())); + + assertThat(XContentType.parseVersion("application/json; sth=123"), + is(nullValue())); } } diff --git a/server/src/test/java/org/elasticsearch/rest/BytesRestResponseTests.java b/server/src/test/java/org/elasticsearch/rest/BytesRestResponseTests.java index 8eff3b23dc5cb..824ec611ea767 100644 --- a/server/src/test/java/org/elasticsearch/rest/BytesRestResponseTests.java +++ b/server/src/test/java/org/elasticsearch/rest/BytesRestResponseTests.java @@ -299,7 +299,7 @@ public void testErrorToAndFromXContent() throws IOException { final XContentType xContentType = randomFrom(XContentType.values()); - Map params = Collections.singletonMap("format", xContentType.format()); + Map params = Collections.singletonMap("format", xContentType.formatPathParameter()); RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withParams(params).build(); RestChannel channel = detailed ? new DetailedExceptionRestChannel(request) : new SimpleExceptionRestChannel(request); diff --git a/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java b/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java index 487bbed5a5999..017e304b14acc 100644 --- a/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java +++ b/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java @@ -207,7 +207,7 @@ public void testMalformedContentTypeHeader() { }); assertNotNull(e.getCause()); assertThat(e.getCause(), instanceOf(IllegalArgumentException.class)); - assertThat(e.getMessage(), equalTo("java.lang.IllegalArgumentException: invalid Content-Type header [" + type + "]")); + assertThat(e.getMessage(), equalTo("java.lang.IllegalArgumentException: invalid media type [" + type + "]")); } public void testNoContentTypeHeader() { @@ -222,7 +222,7 @@ public void testMultipleContentTypeHeaders() { () -> 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")); + assertThat(e.getMessage(), equalTo("java.lang.IllegalArgumentException: only one value for the header should be provided")); } public void testRequiredContent() { diff --git a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestResponse.java b/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestResponse.java index 9f5d0dfe9dcd7..0d1bf3208936b 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestResponse.java +++ b/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestResponse.java @@ -53,8 +53,8 @@ public ClientYamlTestResponse(Response response) throws IOException { this.response = response; if (response.getEntity() != null) { String contentType = response.getHeader("Content-Type"); - this.bodyContentType = XContentType.fromMediaType(contentType); - try { + //Do not know about sql media types. relies on null + this.bodyContentType = getContentTypeIgnoreExceptions(contentType); try { byte[] bytes = EntityUtils.toByteArray(response.getEntity()); //skip parsing if we got text back (e.g. if we called _cat apis) if (bodyContentType != null) { @@ -71,6 +71,14 @@ public ClientYamlTestResponse(Response response) throws IOException { } } + private XContentType getContentTypeIgnoreExceptions(String contentType) { + try { + return XContentType.fromMediaType(contentType); + } catch (IllegalArgumentException e) { + return null; + } + } + public int getStatusCode() { return response.getStatusLine().getStatusCode(); } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/SecurityRestFilter.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/SecurityRestFilter.java index 69c444aa9bf4d..516c1e800e8b7 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/SecurityRestFilter.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/SecurityRestFilter.java @@ -13,6 +13,8 @@ import org.elasticsearch.action.ActionListener; import org.elasticsearch.client.node.NodeClient; import org.elasticsearch.common.util.concurrent.ThreadContext; +import org.elasticsearch.common.xcontent.MediaType; +import org.elasticsearch.common.xcontent.MediaTypeRegistry; import org.elasticsearch.http.HttpChannel; import org.elasticsearch.license.XPackLicenseState; import org.elasticsearch.rest.BytesRestResponse; @@ -142,4 +144,9 @@ private RestRequest maybeWrapRestRequest(RestRequest restRequest) throws IOExcep } return restRequest; } + + @Override + public MediaTypeRegistry validAcceptMediaTypes() { + return restHandler.validAcceptMediaTypes(); + } } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java index 763e460170cf0..155bb7de05bd3 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java @@ -8,6 +8,7 @@ import org.elasticsearch.client.node.NodeClient; import org.elasticsearch.common.xcontent.MediaType; +import org.elasticsearch.common.xcontent.MediaTypeRegistry; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentType; @@ -44,6 +45,10 @@ public List routes() { new Route(POST, Protocol.SQL_QUERY_REST_ENDPOINT)); } + public MediaTypeRegistry validAcceptMediaTypes(){ + return SqlMediaTypeParser.MEDIA_TYPE_REGISTRY; + } + @Override protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException { @@ -84,9 +89,6 @@ public RestResponse buildResponse(SqlQueryResponse response) throws Exception { }); } - - - @Override protected Set responseParams() { return responseMediaType == TextFormat.CSV ? Collections.singleton(URL_PARAM_DELIMITER) : Collections.emptySet(); diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java index 189dc137b654c..536ac918aa459 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java @@ -7,27 +7,19 @@ package org.elasticsearch.xpack.sql.plugin; import org.elasticsearch.common.xcontent.MediaType; -import org.elasticsearch.common.xcontent.MediaTypeParser; +import org.elasticsearch.common.xcontent.MediaTypeRegistry; import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.rest.RestRequest; import org.elasticsearch.xpack.sql.action.SqlQueryRequest; import org.elasticsearch.xpack.sql.proto.Mode; -import java.util.Map; import static org.elasticsearch.xpack.sql.proto.Protocol.URL_PARAM_FORMAT; public class SqlMediaTypeParser { - private static final MediaTypeParser parser = new MediaTypeParser.Builder<>() - .copyFromMediaTypeParser(XContentType.mediaTypeParser) - .withMediaTypeAndParams(TextFormat.PLAIN_TEXT.typeWithSubtype(), TextFormat.PLAIN_TEXT, - Map.of("header", "present|absent", "charset", "utf-8")) - .withMediaTypeAndParams(TextFormat.CSV.typeWithSubtype(), TextFormat.CSV, - Map.of("header", "present|absent", "charset", "utf-8", - "delimiter", ".+"))// more detailed parsing is in TextFormat.CSV#delimiter - .withMediaTypeAndParams(TextFormat.TSV.typeWithSubtype(), TextFormat.TSV, - Map.of("header", "present|absent", "charset", "utf-8")) - .build(); + public static final MediaTypeRegistry MEDIA_TYPE_REGISTRY = new MediaTypeRegistry<>() + .register(XContentType.values()) + .register(TextFormat.values()); /* * Since we support {@link TextFormat} and @@ -42,25 +34,18 @@ public class SqlMediaTypeParser { * isn't then we use the {@code Content-Type} header which is required. */ public MediaType getMediaType(RestRequest request, SqlQueryRequest sqlRequest) { - if (Mode.isDedicatedClient(sqlRequest.requestInfo().mode()) && (sqlRequest.binaryCommunication() == null || sqlRequest.binaryCommunication())) { // enforce CBOR response for drivers and CLI (unless instructed differently through the config param) return XContentType.CBOR; } else if (request.hasParam(URL_PARAM_FORMAT)) { - return validateColumnarRequest(sqlRequest.columnar(), parser.fromFormat(request.param(URL_PARAM_FORMAT))); - } - if (request.getHeaders().containsKey("Accept")) { - String accept = request.header("Accept"); - // */* means "I don't care" which we should treat like not specifying the header - if ("*/*".equals(accept) == false) { - return validateColumnarRequest(sqlRequest.columnar(), parser.fromMediaType(accept)); - } + return validateColumnarRequest(sqlRequest.columnar(), MEDIA_TYPE_REGISTRY.formatToMediaType(request.param(URL_PARAM_FORMAT))); } - String contentType = request.header("Content-Type"); - assert contentType != null : "The Content-Type header is required"; - return validateColumnarRequest(sqlRequest.columnar(), parser.fromMediaType(contentType)); + if(request.getParsedAccept() != null) { + return request.getParsedAccept().toMediaType(MEDIA_TYPE_REGISTRY); + } + return request.getXContentType(); } private static MediaType validateColumnarRequest(boolean requestIsColumnar, MediaType fromMediaType) { @@ -70,5 +55,4 @@ private static MediaType validateColumnarRequest(boolean requestIsColumnar, Medi } return fromMediaType; } - } 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 d397d2316959c..11082122a66c9 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 @@ -24,7 +24,9 @@ import java.time.ZonedDateTime; import java.util.List; import java.util.Locale; +import java.util.Map; import java.util.Objects; +import java.util.Set; import java.util.function.Function; import static org.elasticsearch.xpack.sql.action.BasicFormatter.FormatOption.TEXT; @@ -83,7 +85,7 @@ String format(RestRequest request, SqlQueryResponse response) { } @Override - public String format() { + public String formatPathParameter() { return FORMAT_TEXT; } @@ -103,11 +105,12 @@ protected String eol() { } @Override - public String subtype() { - return "plain"; + public Set>> mediaTypeMappings() { + return Set.of( + Tuple.tuple(CONTENT_TYPE_TXT, + Map.of("header", "present|absent", "charset", "utf-8"))); } - }, /** @@ -132,7 +135,7 @@ protected String eol() { } @Override - public String format() { + public String formatPathParameter() { return FORMAT_CSV; } @@ -224,11 +227,15 @@ boolean hasHeader(RestRequest request) { } @Override - public String subtype() { - return "csv"; + public Set>> mediaTypeMappings() { + return Set.of( + Tuple.tuple(CONTENT_TYPE_CSV, + Map.of("header", "present|absent", "charset", "utf-8", + "delimiter", ".+")));// more detailed parsing is in TextFormat.CSV#delimiter } }, + TSV() { @Override protected Character delimiter() { @@ -242,7 +249,7 @@ protected String eol() { } @Override - public String format() { + public String formatPathParameter() { return FORMAT_TSV; } @@ -278,8 +285,10 @@ String maybeEscape(String value, Character __) { } @Override - public String subtype() { - return "tab-separated-values"; + public Set>> mediaTypeMappings() { + return Set.of( + Tuple.tuple(CONTENT_TYPE_TSV, + Map.of("header", "present|absent", "charset", "utf-8"))); } }; @@ -358,15 +367,4 @@ protected Character delimiter(RestRequest request) { String maybeEscape(String value, Character delimiter) { return value; } - - - @Override - public String type() { - return "text"; - } - - @Override - public String typeWithSubtype() { - return contentType(); - } } diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/http/HttpResponse.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/http/HttpResponse.java index 6fc19feb8ffd4..0b98c884d99b9 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/http/HttpResponse.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/http/HttpResponse.java @@ -105,7 +105,12 @@ public XContentType xContentType() { if (values == null || values.length == 0) { return null; } - return XContentType.fromMediaType(values[0]); + try { + return XContentType.fromMediaType(values[0]); + }catch (IllegalArgumentException e){ + //HttpInputTests - content-type being unrecognized_content_type + return null; + } } @Override diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/input/http/ExecutableHttpInput.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/input/http/ExecutableHttpInput.java index d484ce1e93272..8cce85cbba11a 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/input/http/ExecutableHttpInput.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/input/http/ExecutableHttpInput.java @@ -99,7 +99,7 @@ HttpInput.Result doExecute(WatchExecutionContext ctx, HttpRequest request) throw } } catch (Exception e) { throw new ElasticsearchParseException("could not parse response body [{}] it does not appear to be [{}]", type(), ctx.id(), - response.body().utf8ToString(), contentType.format()); + response.body().utf8ToString(), contentType.formatPathParameter()); } } else { payloadMap.put("_value", response.body().utf8ToString()); diff --git a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/support/WatcherTemplateTests.java b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/support/WatcherTemplateTests.java index 1b27e8151ecb2..04789dc97ea1b 100644 --- a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/support/WatcherTemplateTests.java +++ b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/support/WatcherTemplateTests.java @@ -160,7 +160,7 @@ static String prepareTemplate(String template, @Nullable XContentType contentTyp return template; } return new StringBuilder("__") - .append(contentType.format().toLowerCase(Locale.ROOT)) + .append(contentType.formatPathParameter().toLowerCase(Locale.ROOT)) .append("__::") .append(template) .toString(); From 66224e74f5ede806080c282914870ebac7a8c508 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Fri, 30 Oct 2020 11:14:20 +0100 Subject: [PATCH 02/21] allow smile and cbor to parse charset param. See ClientYamlTestExecutionContext:L122 --- .../org/elasticsearch/common/xcontent/XContentType.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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 8148d7c323a43..b77b5705fa69b 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 @@ -92,9 +92,9 @@ public XContent xContent() { @Override public Set>> mediaTypeMappings() { return Set.of( - Tuple.tuple("application/smile", Collections.emptyMap()), + Tuple.tuple("application/smile", Map.of("charset", "UTF-8")), Tuple.tuple("application/vnd.elasticsearch+smile", - Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN))); + Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", "UTF-8"))); } }, /** @@ -146,9 +146,9 @@ public XContent xContent() { @Override public Set>> mediaTypeMappings() { return Set.of( - Tuple.tuple("application/cbor", Collections.emptyMap()), + Tuple.tuple("application/cbor", Map.of("charset", "UTF-8")), Tuple.tuple("application/vnd.elasticsearch+cbor", - Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN))); + Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN,"charset", "UTF-8"))); } }; From 29b27e805f8e31f7b2e21cd54ae9a2342d9398aa Mon Sep 17 00:00:00 2001 From: pgomulka Date: Fri, 30 Oct 2020 13:37:26 +0100 Subject: [PATCH 03/21] javadocs --- .../common/xcontent/MediaTypeRegistry.java | 5 +++ .../common/xcontent/ParsedMediaType.java | 13 ++++++ .../common/xcontent/ParsedMediaTypeTests.java | 41 ++++++++++--------- 3 files changed, 40 insertions(+), 19 deletions(-) diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeRegistry.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeRegistry.java index f9c3c7d8a511f..80fd6f9970780 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeRegistry.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeRegistry.java @@ -27,6 +27,11 @@ import java.util.Set; import java.util.regex.Pattern; +/** + * A registry of mappings between String typeWithSubtype to a MediaType instances. For instance application/json -> XContentType.JSON + * Defines parameters that are allowed for media types and a regex to validate them. + * Specifies format path parameter values that are used to specify requested response Content-Type. + */ public class MediaTypeRegistry { private Map formatToMediaType = new HashMap<>(); diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ParsedMediaType.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ParsedMediaType.java index 852b72fb503ff..3eef3c1ac6b75 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ParsedMediaType.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ParsedMediaType.java @@ -25,6 +25,13 @@ import java.util.Map; import java.util.regex.Pattern; +/** + * A raw result of parsing media types from Accept or Content-Type headers. + * It follow parsing and validates as per rules defined in https://tools.ietf.org/html/rfc7231#section-3.1.1.1 + * Can be resolved to MediaType + * @see MediaType + * @see MediaTypeRegistry + */ public class ParsedMediaType { //sun.net.www.protocol.http.HttpURLConnection sets a default Accept header if it was not provided on a request public static final String DEFAULT_ACCEPT_STRING = "text/html, image/gif, image/jpeg, *; q=.2, */*; q=.2"; @@ -95,6 +102,12 @@ public static ParsedMediaType parseMediaType(String headerValue) { return null; } + /** + * Resolves this instance to a MediaType instance defined in given MediaTypeRegistry. + * Performs validation against parameters. + * @param mediaTypeRegistry a registry where a mapping between a raw media type to an instance MediaType is defined + * @return a MediaType instance + */ public T toMediaType(MediaTypeRegistry mediaTypeRegistry) { T type = mediaTypeRegistry.typeWithSubtypeToMediaType(mimeTypeWithoutParams()); if (type != null) { diff --git a/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ParsedMediaTypeTests.java b/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ParsedMediaTypeTests.java index 3995dd391be1b..2f63e4e22743e 100644 --- a/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ParsedMediaTypeTests.java +++ b/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ParsedMediaTypeTests.java @@ -25,6 +25,8 @@ import java.util.Map; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.nullValue; public class ParsedMediaTypeTests extends ESTestCase { @@ -56,21 +58,16 @@ public void testWhiteSpaceInTypeSubtype() { } -// public void testInvalidParameters() { -// String mediaType = "application/vnd.elasticsearch+json"; -// assertThat(ParsedMediaType.parseMediaType(mediaType + "; charset=unknown") -// .toMediaType(mediaTypeRegistry), -// is(nullValue())); -// assertThat(ParsedMediaType.parseMediaType(mediaType + "; keyvalueNoEqualsSign") -// .toMediaType(mediaTypeRegistry), -// is(nullValue())); -// assertThat(ParsedMediaType.parseMediaType(mediaType + "; key = value") -// .toMediaType(mediaTypeRegistry), -// is(nullValue())); -// assertThat(ParsedMediaType.parseMediaType(mediaType + "; key=") -// .toMediaType(mediaTypeRegistry), -// is(nullValue())); -// } + public void testInvalidParameters() { + String mediaType = "application/vnd.elasticsearch+json"; + expectThrows(IllegalArgumentException.class, () -> ParsedMediaType.parseMediaType(mediaType + "; keyvalueNoEqualsSign") + .toMediaType(mediaTypeRegistry)); + // allowing spaces between = + // expectThrows(IllegalArgumentException.class, () -> ParsedMediaType.parseMediaType(mediaType + "; key = value") + // .toMediaType(mediaTypeRegistry)); + expectThrows(IllegalArgumentException.class, () -> ParsedMediaType.parseMediaType(mediaType + "; key=") + .toMediaType(mediaTypeRegistry)); + } public void testXContentTypes() { for (XContentType xContentType : XContentType.values()) { @@ -114,8 +111,14 @@ public void testMalformedParameters() { assertThat(exception.getMessage(), equalTo("invalid parameters for header [application/foo; char=set=unknown]")); } -// public void testMultipleValues() { -// String mediaType = "text/html, image/gif, image/jpeg, *; q=.2, */*; q=.2"; -// ParsedMediaType.parseMediaType(mediaType); -// } + public void testDefaultAcceptHeader() { + // This media type is defined in sun.net.www.protocol.http.HttpURLConnection as a default Accept header + // and used when a header was not set on a request + // It should be treated as if a user did not specify a header value + String mediaType = "text/html, image/gif, image/jpeg, *; q=.2, */*; q=.2"; + assertThat(ParsedMediaType.parseMediaType(mediaType), is(nullValue())); + + // When using curl */* is used a default Accept header when not specified by a user + assertThat(ParsedMediaType.parseMediaType("*/*"), is(nullValue())); + } } From 46903625a056c3f615d0c593370e3c442b1a99b5 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Fri, 30 Oct 2020 13:42:38 +0100 Subject: [PATCH 04/21] fix javadoc --- .../org/elasticsearch/common/xcontent/MediaTypeRegistry.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeRegistry.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeRegistry.java index 80fd6f9970780..2b7a070e06c04 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeRegistry.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeRegistry.java @@ -28,7 +28,7 @@ import java.util.regex.Pattern; /** - * A registry of mappings between String typeWithSubtype to a MediaType instances. For instance application/json -> XContentType.JSON + * A registry of mappings between String typeWithSubtype to a MediaType instances. For instance application/json to XContentType.JSON * Defines parameters that are allowed for media types and a regex to validate them. * Specifies format path parameter values that are used to specify requested response Content-Type. */ From e7866a982d18f3beecc26a217b600f0fc96ba110 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Fri, 30 Oct 2020 16:59:56 +0100 Subject: [PATCH 05/21] Allow registration of compatible version-1 handlers --- .../elasticsearch/action/ActionModule.java | 6 ++-- .../java/org/elasticsearch/node/Node.java | 14 +++++++- .../elasticsearch/rest/CompatibleVersion.java | 35 +++++++++++++++++++ .../elasticsearch/rest/MethodHandlers.java | 27 ++++++++------ .../elasticsearch/rest/RestController.java | 17 ++++++--- .../org/elasticsearch/rest/RestHandler.java | 11 ++++++ .../action/ActionModuleTests.java | 7 ++-- .../rest/RestControllerTests.java | 23 +++++++----- .../rest/RestHttpResponseHeadersTests.java | 2 +- .../indices/RestValidateQueryActionTests.java | 3 +- .../test/rest/RestActionTestCase.java | 3 +- 11 files changed, 117 insertions(+), 31 deletions(-) create mode 100644 server/src/main/java/org/elasticsearch/rest/CompatibleVersion.java diff --git a/server/src/main/java/org/elasticsearch/action/ActionModule.java b/server/src/main/java/org/elasticsearch/action/ActionModule.java index b96189ef69764..68ebe9e631eac 100644 --- a/server/src/main/java/org/elasticsearch/action/ActionModule.java +++ b/server/src/main/java/org/elasticsearch/action/ActionModule.java @@ -259,6 +259,7 @@ import org.elasticsearch.persistent.UpdatePersistentTaskStatusAction; import org.elasticsearch.plugins.ActionPlugin; import org.elasticsearch.plugins.ActionPlugin.ActionHandler; +import org.elasticsearch.rest.CompatibleVersion; import org.elasticsearch.rest.RestController; import org.elasticsearch.rest.RestHandler; import org.elasticsearch.rest.RestHeaderDefinition; @@ -427,7 +428,8 @@ public class ActionModule extends AbstractModule { public ActionModule(Settings settings, IndexNameExpressionResolver indexNameExpressionResolver, IndexScopedSettings indexScopedSettings, ClusterSettings clusterSettings, SettingsFilter settingsFilter, ThreadPool threadPool, List actionPlugins, NodeClient nodeClient, - CircuitBreakerService circuitBreakerService, UsageService usageService, SystemIndices systemIndices) { + CircuitBreakerService circuitBreakerService, UsageService usageService, SystemIndices systemIndices, + CompatibleVersion compatibleVersion) { this.settings = settings; this.indexNameExpressionResolver = indexNameExpressionResolver; this.indexScopedSettings = indexScopedSettings; @@ -459,7 +461,7 @@ public ActionModule(Settings settings, IndexNameExpressionResolver indexNameExpr indicesAliasesRequestRequestValidators = new RequestValidators<>( actionPlugins.stream().flatMap(p -> p.indicesAliasesRequestValidators().stream()).collect(Collectors.toList())); - restController = new RestController(headers, restWrapper, nodeClient, circuitBreakerService, usageService); + restController = new RestController(headers, restWrapper, nodeClient, circuitBreakerService, usageService, compatibleVersion); } diff --git a/server/src/main/java/org/elasticsearch/node/Node.java b/server/src/main/java/org/elasticsearch/node/Node.java index c734c23bf657e..d2b5769891bd2 100644 --- a/server/src/main/java/org/elasticsearch/node/Node.java +++ b/server/src/main/java/org/elasticsearch/node/Node.java @@ -149,6 +149,7 @@ import org.elasticsearch.plugins.SystemIndexPlugin; import org.elasticsearch.repositories.RepositoriesModule; import org.elasticsearch.repositories.RepositoriesService; +import org.elasticsearch.rest.CompatibleVersion; import org.elasticsearch.rest.RestController; import org.elasticsearch.script.ScriptContext; import org.elasticsearch.script.ScriptEngine; @@ -539,9 +540,11 @@ protected Node(final Environment initialEnvironment, repositoriesServiceReference::get).stream()) .collect(Collectors.toList()); + ActionModule actionModule = new ActionModule(settings, clusterModule.getIndexNameExpressionResolver(), settingsModule.getIndexScopedSettings(), settingsModule.getClusterSettings(), settingsModule.getSettingsFilter(), - threadPool, pluginsService.filterPlugins(ActionPlugin.class), client, circuitBreakerService, usageService, systemIndices); + threadPool, pluginsService.filterPlugins(ActionPlugin.class), client, circuitBreakerService, usageService, systemIndices, + getRestCompatibleFunction()); modules.add(actionModule); final RestController restController = actionModule.getRestController(); @@ -716,6 +719,15 @@ protected Node(final Environment initialEnvironment, } } + /** + * @return A function that can be used to determine the requested REST compatible version + * package scope for testing + */ + CompatibleVersion getRestCompatibleFunction() { + // TODO PG Until compatible version plugin is implemented, return current version. + return CompatibleVersion.CURRENT_VERSION; + } + protected TransportService newTransportService(Settings settings, Transport transport, ThreadPool threadPool, TransportInterceptor interceptor, Function localNodeFactory, diff --git a/server/src/main/java/org/elasticsearch/rest/CompatibleVersion.java b/server/src/main/java/org/elasticsearch/rest/CompatibleVersion.java new file mode 100644 index 0000000000000..feedb7aaac373 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/rest/CompatibleVersion.java @@ -0,0 +1,35 @@ +/* + * 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.Nullable; +import org.elasticsearch.common.xcontent.ParsedMediaType; + +/** + * An interface used to specify a function that returns a compatible API version + * Intended to be used in a code base instead of a plugin. + */ +@FunctionalInterface +public interface CompatibleVersion { + Version get(@Nullable ParsedMediaType acceptHeader, @Nullable ParsedMediaType contentTypeHeader, boolean hasContent); + + CompatibleVersion CURRENT_VERSION = (acceptHeader, contentTypeHeader, hasContent) -> Version.CURRENT; +} diff --git a/server/src/main/java/org/elasticsearch/rest/MethodHandlers.java b/server/src/main/java/org/elasticsearch/rest/MethodHandlers.java index 0d6233e62f925..22494adefe1a9 100644 --- a/server/src/main/java/org/elasticsearch/rest/MethodHandlers.java +++ b/server/src/main/java/org/elasticsearch/rest/MethodHandlers.java @@ -19,7 +19,7 @@ package org.elasticsearch.rest; -import org.elasticsearch.common.Nullable; +import org.elasticsearch.Version; import java.util.HashMap; import java.util.Map; @@ -31,13 +31,14 @@ final class MethodHandlers { private final String path; - private final Map methodHandlers; + private final Map> methodHandlers; - MethodHandlers(String path, RestHandler handler, RestRequest.Method... methods) { + MethodHandlers(String path, RestHandler handler, Version version, RestRequest.Method... methods) { this.path = path; this.methodHandlers = new HashMap<>(methods.length); for (RestRequest.Method method : methods) { - methodHandlers.put(method, handler); + methodHandlers.computeIfAbsent(method, k -> new HashMap<>()) + .put(version, handler); } } @@ -45,9 +46,10 @@ final class MethodHandlers { * Add a handler for an additional array of methods. Note that {@code MethodHandlers} * does not allow replacing the handler for an already existing method. */ - MethodHandlers addMethods(RestHandler handler, RestRequest.Method... methods) { + MethodHandlers addMethods(RestHandler handler, Version version, RestRequest.Method... methods) { for (RestRequest.Method method : methods) { - RestHandler existing = methodHandlers.putIfAbsent(method, handler); + RestHandler existing = methodHandlers.computeIfAbsent(method, k -> new HashMap<>()) + .putIfAbsent(version, handler); if (existing != null) { throw new IllegalArgumentException("Cannot replace existing handler for [" + path + "] for method: " + method); } @@ -56,11 +58,16 @@ MethodHandlers addMethods(RestHandler handler, RestRequest.Method... methods) { } /** - * Returns the handler for the given method or {@code null} if none exists. + * Returns the handler for the given method and version or {@code null} if none exists. */ - @Nullable - RestHandler getHandler(RestRequest.Method method) { - return methodHandlers.get(method); + RestHandler getHandler(RestRequest.Method method, Version version) { + Map versionToHandlers = methodHandlers.get(method); + if (versionToHandlers == null) { + return null; //method not found + } + final RestHandler handler = versionToHandlers.get(version); + return handler != null || version.equals(Version.CURRENT) ? handler : versionToHandlers.get(Version.CURRENT); + } /** diff --git a/server/src/main/java/org/elasticsearch/rest/RestController.java b/server/src/main/java/org/elasticsearch/rest/RestController.java index e1ca179460794..1572beb840442 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestController.java +++ b/server/src/main/java/org/elasticsearch/rest/RestController.java @@ -23,6 +23,7 @@ import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.message.ParameterizedMessage; import org.elasticsearch.ElasticsearchException; +import org.elasticsearch.Version; import org.elasticsearch.client.node.NodeClient; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.Strings; @@ -90,11 +91,14 @@ public class RestController implements HttpServerTransport.Dispatcher { /** Rest headers that are copied to internal requests made during a rest request. */ private final Set headersToCopy; private final UsageService usageService; + private CompatibleVersion compatibleVersion; public RestController(Set headersToCopy, UnaryOperator handlerWrapper, - NodeClient client, CircuitBreakerService circuitBreakerService, UsageService usageService) { + NodeClient client, CircuitBreakerService circuitBreakerService, UsageService usageService, + CompatibleVersion compatibleVersion) { this.headersToCopy = headersToCopy; this.usageService = usageService; + this.compatibleVersion = compatibleVersion; if (handlerWrapper == null) { handlerWrapper = h -> h; // passthrough if no wrapper set } @@ -168,8 +172,10 @@ protected void registerHandler(RestRequest.Method method, String path, RestHandl } private void registerHandlerNoWrap(RestRequest.Method method, String path, RestHandler maybeWrappedHandler) { - handlers.insertOrUpdate(path, new MethodHandlers(path, maybeWrappedHandler, method), - (mHandlers, newMHandler) -> mHandlers.addMethods(maybeWrappedHandler, method)); + final Version version = maybeWrappedHandler.compatibleWithVersion(); + + handlers.insertOrUpdate(path, new MethodHandlers(path, maybeWrappedHandler, version, method), + (mHandlers, newMHandler) -> mHandlers.addMethods(maybeWrappedHandler, version, method)); } /** @@ -318,6 +324,9 @@ private void tryAllHandlers(final RestRequest request, final RestChannel channel final String rawPath = request.rawPath(); final String uri = request.uri(); final RestRequest.Method requestMethod; + + Version compatibleVersion = this.compatibleVersion. + get(request.getParsedAccept(), request.getParsedContentType(), request.hasContent()); try { // Resolves the HTTP method and fails if the method is invalid requestMethod = request.method(); @@ -329,7 +338,7 @@ private void tryAllHandlers(final RestRequest request, final RestChannel channel if (handlers == null) { handler = null; } else { - handler = handlers.getHandler(requestMethod); + handler = handlers.getHandler(requestMethod, compatibleVersion); } if (handler == null) { if (handleNoHandlerFound(rawPath, requestMethod, uri, channel)) { diff --git a/server/src/main/java/org/elasticsearch/rest/RestHandler.java b/server/src/main/java/org/elasticsearch/rest/RestHandler.java index 21d4a6f278f3c..c19d91255fb7b 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestHandler.java +++ b/server/src/main/java/org/elasticsearch/rest/RestHandler.java @@ -19,6 +19,7 @@ package org.elasticsearch.rest; +import org.elasticsearch.Version; import org.elasticsearch.client.node.NodeClient; import org.elasticsearch.common.xcontent.MediaType; import org.elasticsearch.common.xcontent.MediaTypeRegistry; @@ -106,6 +107,16 @@ default MediaTypeRegistry validAcceptMediaTypes(){ return XContentType.MEDIA_TYPE_REGISTRY; } + /** + * Returns a version a handler is compatible with. + * This version is then used to math a handler with a request that specified a version. + * If no version is specified, handler is assumed to be compatible with Version.CURRENT + * @return a version + */ + default Version compatibleWithVersion() { + return Version.CURRENT; + } + class Route { private final String path; diff --git a/server/src/test/java/org/elasticsearch/action/ActionModuleTests.java b/server/src/test/java/org/elasticsearch/action/ActionModuleTests.java index 15d2aff8d8787..023745cbe8d46 100644 --- a/server/src/test/java/org/elasticsearch/action/ActionModuleTests.java +++ b/server/src/test/java/org/elasticsearch/action/ActionModuleTests.java @@ -34,6 +34,7 @@ import org.elasticsearch.common.util.concurrent.ThreadContext; import org.elasticsearch.plugins.ActionPlugin; import org.elasticsearch.plugins.ActionPlugin.ActionHandler; +import org.elasticsearch.rest.CompatibleVersion; import org.elasticsearch.rest.RestChannel; import org.elasticsearch.rest.RestController; import org.elasticsearch.rest.RestHandler; @@ -111,7 +112,7 @@ public void testSetupRestHandlerContainsKnownBuiltin() { ActionModule actionModule = new ActionModule(settings.getSettings(), new IndexNameExpressionResolver(new ThreadContext(Settings.EMPTY)), settings.getIndexScopedSettings(), settings.getClusterSettings(), settings.getSettingsFilter(), null, emptyList(), null, - null, usageService, null); + null, usageService, null, CompatibleVersion.CURRENT_VERSION); actionModule.initRestHandlers(null); // At this point the easiest way to confirm that a handler is loaded is to try to register another one on top of it and to fail Exception e = expectThrows(IllegalArgumentException.class, () -> @@ -151,7 +152,7 @@ public String getName() { ActionModule actionModule = new ActionModule(settings.getSettings(), new IndexNameExpressionResolver(threadPool.getThreadContext()), settings.getIndexScopedSettings(), settings.getClusterSettings(), settings.getSettingsFilter(), threadPool, singletonList(dupsMainAction), - null, null, usageService, null); + null, null, usageService, null, CompatibleVersion.CURRENT_VERSION); Exception e = expectThrows(IllegalArgumentException.class, () -> actionModule.initRestHandlers(null)); assertThat(e.getMessage(), startsWith("Cannot replace existing handler for [/] for method: GET")); } finally { @@ -186,7 +187,7 @@ public List getRestHandlers(Settings settings, RestController restC ActionModule actionModule = new ActionModule(settings.getSettings(), new IndexNameExpressionResolver(threadPool.getThreadContext()), settings.getIndexScopedSettings(), settings.getClusterSettings(), settings.getSettingsFilter(), threadPool, singletonList(registersFakeHandler), - null, null, usageService, null); + null, null, usageService, null, CompatibleVersion.CURRENT_VERSION); actionModule.initRestHandlers(null); // At this point the easiest way to confirm that a handler is loaded is to try to register another one on top of it and to fail Exception e = expectThrows(IllegalArgumentException.class, () -> diff --git a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java index 0b756acb5ef86..c6e5b5fe28659 100644 --- a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java +++ b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java @@ -19,6 +19,7 @@ package org.elasticsearch.rest; +import org.elasticsearch.Version; import org.elasticsearch.client.node.NodeClient; import org.elasticsearch.common.breaker.CircuitBreaker; import org.elasticsearch.common.bytes.BytesArray; @@ -97,7 +98,8 @@ public void setup() { HttpServerTransport httpServerTransport = new TestHttpServerTransport(); client = new NoOpNodeClient(this.getTestName()); - restController = new RestController(Collections.emptySet(), null, client, circuitBreakerService, usageService); + restController = new RestController(Collections.emptySet(), null, client, circuitBreakerService, usageService, + CompatibleVersion.CURRENT_VERSION); restController.registerHandler(RestRequest.Method.GET, "/", (request, channel, client) -> channel.sendResponse( new BytesRestResponse(RestStatus.OK, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY))); @@ -117,7 +119,8 @@ public void testApplyRelevantHeaders() throws Exception { final ThreadContext threadContext = client.threadPool().getThreadContext(); Set headers = new HashSet<>(Arrays.asList(new RestHeaderDefinition("header.1", true), new RestHeaderDefinition("header.2", true))); - final RestController restController = new RestController(headers, null, null, circuitBreakerService, usageService); + final RestController restController = new RestController(headers, null, null, circuitBreakerService, usageService, + CompatibleVersion.CURRENT_VERSION); Map> restHeaders = new HashMap<>(); restHeaders.put("header.1", Collections.singletonList("true")); restHeaders.put("header.2", Collections.singletonList("true")); @@ -137,7 +140,7 @@ public MethodHandlers next() { assertEquals("true", threadContext.getHeader("header.1")); assertEquals("true", threadContext.getHeader("header.2")); assertNull(threadContext.getHeader("header.3")); - }, RestRequest.Method.GET); + }, Version.CURRENT, RestRequest.Method.GET); } }); AssertingChannel channel = new AssertingChannel(fakeRequest, false, RestStatus.BAD_REQUEST); @@ -153,7 +156,8 @@ public void testRequestWithDisallowedMultiValuedHeader() { final ThreadContext threadContext = client.threadPool().getThreadContext(); Set headers = new HashSet<>(Arrays.asList(new RestHeaderDefinition("header.1", true), new RestHeaderDefinition("header.2", false))); - final RestController restController = new RestController(headers, null, null, circuitBreakerService, usageService); + final RestController restController = new RestController(headers, null, null, circuitBreakerService, usageService, + CompatibleVersion.CURRENT_VERSION); Map> restHeaders = new HashMap<>(); restHeaders.put("header.1", Collections.singletonList("boo")); restHeaders.put("header.2", List.of("foo", "bar")); @@ -167,7 +171,8 @@ public void testRequestWithDisallowedMultiValuedHeaderButSameValues() { final ThreadContext threadContext = client.threadPool().getThreadContext(); Set headers = new HashSet<>(Arrays.asList(new RestHeaderDefinition("header.1", true), new RestHeaderDefinition("header.2", false))); - final RestController restController = new RestController(headers, null, client, circuitBreakerService, usageService); + final RestController restController = new RestController(headers, null, client, circuitBreakerService, usageService, + CompatibleVersion.CURRENT_VERSION); Map> restHeaders = new HashMap<>(); restHeaders.put("header.1", Collections.singletonList("boo")); restHeaders.put("header.2", List.of("foo", "foo")); @@ -221,7 +226,8 @@ public void testRegisterWithDeprecatedHandler() { } public void testRegisterSecondMethodWithDifferentNamedWildcard() { - final RestController restController = new RestController(null, null, null, circuitBreakerService, usageService); + final RestController restController = new RestController(null, null, null, circuitBreakerService, usageService, + CompatibleVersion.CURRENT_VERSION); RestRequest.Method firstMethod = randomFrom(RestRequest.Method.values()); RestRequest.Method secondMethod = @@ -248,7 +254,7 @@ public void testRestHandlerWrapper() throws Exception { h -> { assertSame(handler, h); return (RestRequest request, RestChannel channel, NodeClient client) -> wrapperCalled.set(true); - }, client, circuitBreakerService, usageService); + }, client, circuitBreakerService, usageService, CompatibleVersion.CURRENT_VERSION); restController.registerHandler(RestRequest.Method.GET, "/wrapped", handler); RestRequest request = testRestRequest("/wrapped", "{}", XContentType.JSON); AssertingChannel channel = new AssertingChannel(request, true, RestStatus.BAD_REQUEST); @@ -311,7 +317,8 @@ public void testDispatchRequiresContentTypeForRequestsWithContent() { String content = randomAlphaOfLength((int) Math.round(BREAKER_LIMIT.getBytes() / inFlightRequestsBreaker.getOverhead())); RestRequest request = testRestRequest("/", content, null); AssertingChannel channel = new AssertingChannel(request, true, RestStatus.NOT_ACCEPTABLE); - restController = new RestController(Collections.emptySet(), null, null, circuitBreakerService, usageService); + restController = new RestController(Collections.emptySet(), null, null, circuitBreakerService, usageService, + CompatibleVersion.CURRENT_VERSION); restController.registerHandler(RestRequest.Method.GET, "/", (r, c, client) -> c.sendResponse( new BytesRestResponse(RestStatus.OK, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY))); diff --git a/server/src/test/java/org/elasticsearch/rest/RestHttpResponseHeadersTests.java b/server/src/test/java/org/elasticsearch/rest/RestHttpResponseHeadersTests.java index ce54b896ef36e..90eb2288d104e 100644 --- a/server/src/test/java/org/elasticsearch/rest/RestHttpResponseHeadersTests.java +++ b/server/src/test/java/org/elasticsearch/rest/RestHttpResponseHeadersTests.java @@ -90,7 +90,7 @@ public void testUnsupportedMethodResponseHttpHeader() throws Exception { final Settings settings = Settings.EMPTY; UsageService usageService = new UsageService(); RestController restController = new RestController(Collections.emptySet(), - null, null, circuitBreakerService, usageService); + null, null, circuitBreakerService, usageService, CompatibleVersion.CURRENT_VERSION); // A basic RestHandler handles requests to the endpoint RestHandler restHandler = new RestHandler() { diff --git a/server/src/test/java/org/elasticsearch/rest/action/admin/indices/RestValidateQueryActionTests.java b/server/src/test/java/org/elasticsearch/rest/action/admin/indices/RestValidateQueryActionTests.java index 6711beb52bdca..98a21263151ec 100644 --- a/server/src/test/java/org/elasticsearch/rest/action/admin/indices/RestValidateQueryActionTests.java +++ b/server/src/test/java/org/elasticsearch/rest/action/admin/indices/RestValidateQueryActionTests.java @@ -30,6 +30,7 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.indices.breaker.NoneCircuitBreakerService; +import org.elasticsearch.rest.CompatibleVersion; import org.elasticsearch.rest.RestController; import org.elasticsearch.rest.RestRequest; import org.elasticsearch.search.AbstractSearchTestCase; @@ -60,7 +61,7 @@ public class RestValidateQueryActionTests extends AbstractSearchTestCase { private static UsageService usageService = new UsageService(); private static RestController controller = new RestController(emptySet(), null, client, - new NoneCircuitBreakerService(), usageService); + new NoneCircuitBreakerService(), usageService, CompatibleVersion.CURRENT_VERSION); private static RestValidateQueryAction action = new RestValidateQueryAction(); /** diff --git a/test/framework/src/main/java/org/elasticsearch/test/rest/RestActionTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/rest/RestActionTestCase.java index 0577ad0c23441..5467f71eb11ba 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/rest/RestActionTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/rest/RestActionTestCase.java @@ -26,6 +26,7 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.util.concurrent.ThreadContext; import org.elasticsearch.indices.breaker.NoneCircuitBreakerService; +import org.elasticsearch.rest.CompatibleVersion; import org.elasticsearch.rest.RestController; import org.elasticsearch.rest.RestRequest; import org.elasticsearch.tasks.Task; @@ -54,7 +55,7 @@ public void setUpController() { controller = new RestController(Collections.emptySet(), null, verifyingClient, new NoneCircuitBreakerService(), - new UsageService()); + new UsageService(), CompatibleVersion.CURRENT_VERSION); } @After From 0330d97d2c3722f09530e2ab0546d738981536ef Mon Sep 17 00:00:00 2001 From: pgomulka Date: Mon, 2 Nov 2020 13:27:48 +0100 Subject: [PATCH 06/21] add tests --- .../main/java/org/elasticsearch/Version.java | 9 +++ .../elasticsearch/rest/RestController.java | 2 + .../rest/RestControllerTests.java | 74 ++++++++++++++++++- 3 files changed, 82 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/Version.java b/server/src/main/java/org/elasticsearch/Version.java index 7b869198c7567..268aa32558dee 100644 --- a/server/src/main/java/org/elasticsearch/Version.java +++ b/server/src/main/java/org/elasticsearch/Version.java @@ -259,6 +259,7 @@ private static Version fromStringSlow(String version) { public final byte build; public final org.apache.lucene.util.Version luceneVersion; private final String toString; + public final int previousMajorId; Version(int id, org.apache.lucene.util.Version luceneVersion) { this.id = id; @@ -268,6 +269,14 @@ private static Version fromStringSlow(String version) { this.build = (byte) (id % 100); this.luceneVersion = Objects.requireNonNull(luceneVersion); this.toString = major + "." + minor + "." + revision; + this.previousMajorId = major > 0 ? (major - 1) * 1000000 + 99 : major; + } + public Version previousMajor() { + return Version.fromId(previousMajorId); + } + + public static Version minimumRestCompatibilityVersion(){ + return Version.CURRENT.previousMajor(); } public boolean after(Version version) { diff --git a/server/src/main/java/org/elasticsearch/rest/RestController.java b/server/src/main/java/org/elasticsearch/rest/RestController.java index 1572beb840442..7d3c49a8a0769 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestController.java +++ b/server/src/main/java/org/elasticsearch/rest/RestController.java @@ -173,6 +173,8 @@ protected void registerHandler(RestRequest.Method method, String path, RestHandl private void registerHandlerNoWrap(RestRequest.Method method, String path, RestHandler maybeWrappedHandler) { final Version version = maybeWrappedHandler.compatibleWithVersion(); + assert Version.minimumRestCompatibilityVersion() == version || Version.CURRENT == version + : "REST API compatibility is only supported for version " + Version.minimumRestCompatibilityVersion().major; handlers.insertOrUpdate(path, new MethodHandlers(path, maybeWrappedHandler, version, method), (mHandlers, newMHandler) -> mHandlers.addMethods(maybeWrappedHandler, version, method)); diff --git a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java index c6e5b5fe28659..550fd12c8a384 100644 --- a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java +++ b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java @@ -48,6 +48,7 @@ import org.elasticsearch.usage.UsageService; import org.junit.After; import org.junit.Before; +import org.mockito.Mockito; import java.io.IOException; import java.util.Arrays; @@ -61,6 +62,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; @@ -193,7 +195,7 @@ public void testRegisterAsDeprecatedHandler() { RestRequest.Method method = randomFrom(RestRequest.Method.values()); String path = "/_" + randomAlphaOfLengthBetween(1, 6); - RestHandler handler = mock(RestHandler.class); + RestHandler handler = v8mockHandler(); String deprecationMessage = randomAlphaOfLengthBetween(1, 10); // don't want to test everything -- just that it actually wraps the handler @@ -209,7 +211,7 @@ public void testRegisterWithDeprecatedHandler() { final RestRequest.Method method = randomFrom(RestRequest.Method.values()); final String path = "/_" + randomAlphaOfLengthBetween(1, 6); - final RestHandler handler = mock(RestHandler.class); + final RestHandler handler = v8mockHandler(); final RestRequest.Method deprecatedMethod = randomFrom(RestRequest.Method.values()); final String deprecatedPath = "/_" + randomAlphaOfLengthBetween(1, 6); @@ -235,7 +237,8 @@ public void testRegisterSecondMethodWithDifferentNamedWildcard() { final String path = "/_" + randomAlphaOfLengthBetween(1, 6); - RestHandler handler = mock(RestHandler.class); + RestHandler handler = v8mockHandler(); + restController.registerHandler(firstMethod, path + "/{wildcard1}", handler); IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, @@ -244,6 +247,12 @@ public void testRegisterSecondMethodWithDifferentNamedWildcard() { assertThat(exception.getMessage(), equalTo("Trying to use conflicting wildcard names for same path: wildcard1 and wildcard2")); } + private RestHandler v8mockHandler() { + RestHandler mock = mock(RestHandler.class); + Mockito.when(mock.compatibleWithVersion()).thenReturn(Version.CURRENT); + return mock; + } + public void testRestHandlerWrapper() throws Exception { AtomicBoolean handlerCalled = new AtomicBoolean(false); AtomicBoolean wrapperCalled = new AtomicBoolean(false); @@ -627,6 +636,65 @@ public Exception getInboundException() { assertThat(channel.getRestResponse().getHeaders().get("Allow"), hasItem(equalTo(RestRequest.Method.GET.toString()))); } + public void testDispatchCompatibleHandler() { + + RestController restController = new RestController(Collections.emptySet(), null, client, circuitBreakerService, usageService, + (a,c,h)->Version.minimumRestCompatibilityVersion());//always return compatible version + + final byte version = Version.minimumRestCompatibilityVersion().major; + + final String mimeType = randomCompatibleMimeType(version); + String content = randomAlphaOfLength((int) Math.round(BREAKER_LIMIT.getBytes() / inFlightRequestsBreaker.getOverhead())); + final List mimeTypeList = Collections.singletonList(mimeType); + FakeRestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY) + .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() { + @Override + public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception { + XContentBuilder xContentBuilder = channel.newBuilder(); +// assertThat(xContentBuilder.getCompatibleMajorVersion(), equalTo(version)); + channel.sendResponse(new BytesRestResponse(RestStatus.OK, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY)); + } + + @Override + public Version compatibleWithVersion() { + return Version.V_7_0_0; + } + }); + + assertFalse(channel.getSendResponseCalled()); + restController.dispatchRequest(fakeRestRequest, channel, new ThreadContext(Settings.EMPTY)); + assertTrue(channel.getSendResponseCalled()); + } + + public void testRegisterIncompatibleVersionHandler() { + //using restController which uses a compatible version function returning always Version.CURRENT + final byte version = (byte) (Version.CURRENT.major - 2); + + expectThrows(AssertionError.class, + () -> restController.registerHandler(RestRequest.Method.GET, "/foo", new RestHandler() { + @Override + public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception { + } + + @Override + public Version compatibleWithVersion() { + return Version.fromString(version + ".0.0"); + } + })); + } + + private String randomCompatibleMimeType(byte version) { + String subtype = randomFrom(Stream.of(XContentType.values()) + .map(XContentType::mediaTypeWithoutParameters) + .toArray(String[]::new)) + .split("/")[1]; + return randomFrom("application/vnd.elasticsearch+" + subtype + ";compatible-with=" + version); + } private static final class TestHttpServerTransport extends AbstractLifecycleComponent implements HttpServerTransport { From bc5de8b5c53664ab70e3e0c45342ea7b3af7398b Mon Sep 17 00:00:00 2001 From: pgomulka Date: Mon, 2 Nov 2020 16:19:00 +0100 Subject: [PATCH 07/21] pass version to xcontentbuilder --- .../common/xcontent/XContentBuilder.java | 11 ++++++++++ .../elasticsearch/rest/RestController.java | 22 ++++++++++++++----- .../rest/RestControllerTests.java | 2 +- 3 files changed, 28 insertions(+), 7 deletions(-) diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java index bf5984fdfde57..6caa5af826899 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java @@ -48,6 +48,8 @@ */ public final class XContentBuilder implements Closeable, Flushable { + private byte compatibleMajorVersion; + /** * Create a new {@link XContentBuilder} using the given {@link XContent} content. *

@@ -1004,6 +1006,15 @@ public XContentBuilder copyCurrentStructure(XContentParser parser) throws IOExce return this; } + public XContentBuilder withCompatibleMajorVersion(byte compatibleMajorVersion){ + this.compatibleMajorVersion = compatibleMajorVersion; + return this; + } + + public byte getCompatibleMajorVersion() { + return compatibleMajorVersion; + } + @Override public void flush() throws IOException { generator.flush(); diff --git a/server/src/main/java/org/elasticsearch/rest/RestController.java b/server/src/main/java/org/elasticsearch/rest/RestController.java index 7d3c49a8a0769..000701eab02c1 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestController.java +++ b/server/src/main/java/org/elasticsearch/rest/RestController.java @@ -250,7 +250,10 @@ private void dispatchRequest(RestRequest request, RestChannel channel, RestHandl inFlightRequestsBreaker(circuitBreakerService).addWithoutBreaking(contentLength); } // iff we could reserve bytes for the request we need to send the response also over this channel - responseChannel = new ResourceHandlingHttpChannel(channel, circuitBreakerService, contentLength); + // Using a version from a handler because if a version from headers was PREVIOUS, but no handler was found for that version, + // we would return a handler for CURRENT. Therefore no compatible logic in serialisation (toXContent) should be applied + responseChannel = new ResourceHandlingHttpChannel(channel, circuitBreakerService, contentLength, + handler.compatibleWithVersion()); // TODO: Count requests double in the circuit breaker if they need copying? if (handler.allowsUnsafeBuffers() == false) { request.ensureSafeBuffers(); @@ -465,33 +468,40 @@ private static final class ResourceHandlingHttpChannel implements RestChannel { private final RestChannel delegate; private final CircuitBreakerService circuitBreakerService; private final int contentLength; + private final Version compatibleVersion; private final AtomicBoolean closed = new AtomicBoolean(); - ResourceHandlingHttpChannel(RestChannel delegate, CircuitBreakerService circuitBreakerService, int contentLength) { + ResourceHandlingHttpChannel(RestChannel delegate, CircuitBreakerService circuitBreakerService, int contentLength, + Version compatibleVersion) { this.delegate = delegate; this.circuitBreakerService = circuitBreakerService; this.contentLength = contentLength; + this.compatibleVersion = compatibleVersion; } @Override public XContentBuilder newBuilder() throws IOException { - return delegate.newBuilder(); + return delegate.newBuilder() + .withCompatibleMajorVersion(compatibleVersion.major); } @Override public XContentBuilder newErrorBuilder() throws IOException { - return delegate.newErrorBuilder(); + return delegate.newErrorBuilder() + .withCompatibleMajorVersion(compatibleVersion.major); } @Override public XContentBuilder newBuilder(@Nullable XContentType xContentType, boolean useFiltering) throws IOException { - return delegate.newBuilder(xContentType, useFiltering); + return delegate.newBuilder(xContentType, useFiltering) + .withCompatibleMajorVersion(compatibleVersion.major); } @Override public XContentBuilder newBuilder(XContentType xContentType, XContentType responseContentType, boolean useFiltering) throws IOException { - return delegate.newBuilder(xContentType, responseContentType, useFiltering); + return delegate.newBuilder(xContentType, responseContentType, useFiltering) + .withCompatibleMajorVersion(compatibleVersion.major); } @Override diff --git a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java index 550fd12c8a384..8f882dabf3ae3 100644 --- a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java +++ b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java @@ -656,7 +656,7 @@ public void testDispatchCompatibleHandler() { @Override public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception { XContentBuilder xContentBuilder = channel.newBuilder(); -// assertThat(xContentBuilder.getCompatibleMajorVersion(), equalTo(version)); + assertThat(xContentBuilder.getCompatibleMajorVersion(), equalTo(version)); channel.sendResponse(new BytesRestResponse(RestStatus.OK, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY)); } From 2bda74f773d6b682deaa0c256a3568cbc0c1956c Mon Sep 17 00:00:00 2001 From: pgomulka Date: Tue, 3 Nov 2020 13:15:42 +0100 Subject: [PATCH 08/21] code review follow up --- .../common/xcontent/MediaType.java | 23 +++++++--- .../common/xcontent/MediaTypeRegistry.java | 32 +++++++------ .../common/xcontent/ParsedMediaType.java | 17 ++++--- .../common/xcontent/XContentType.java | 45 +++++++++---------- .../common/xcontent/ParsedMediaTypeTests.java | 12 ++--- .../http/AbstractHttpServerTransport.java | 2 +- .../org/elasticsearch/rest/RestRequest.java | 15 ++++--- .../rest/BytesRestResponseTests.java | 2 +- .../elasticsearch/rest/RestRequestTests.java | 11 ++--- .../rest/yaml/ClientYamlTestResponse.java | 3 +- .../xpack/sql/plugin/SqlMediaTypeParser.java | 3 +- .../xpack/sql/plugin/TextFormat.java | 36 ++++++++++----- .../input/http/ExecutableHttpInput.java | 2 +- .../watcher/support/WatcherTemplateTests.java | 2 +- 14 files changed, 116 insertions(+), 89 deletions(-) diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaType.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaType.java index 3136e8d94c4f5..60e10010e3ed7 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaType.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaType.java @@ -33,17 +33,26 @@ public interface MediaType { String COMPATIBLE_WITH_PARAMETER_NAME = "compatible-with"; String VERSION_PATTERN = "\\d+"; + /** + * Returns a corresponding format path parameter for a MediaType. + * i.e. ?format=txt for plain/text media type + */ + String queryParameter(); /** - * Returns a corresponding format for a MediaType. i.e. json for application/json media type - * Can differ from the MediaType's subtype i.e plain/text has a subtype of text but format is txt + * returns a set of MediaTypeValues - allowed media type values on Accept or Content-Type headers + * Also defines parameters for validation. */ - String formatPathParameter(); + Set mediaTypeValues(); /** - * returns a set of Tuples where a key is a sting - MediaType's type with subtype i.e application/json - * and a value is a map of parameters to be validated. - * Map's key is a parameter name, value is a parameter regex which is used for validation + * A class to represent supported mediaType values i.e. application/json and parameters to be validated. + * Parameters for validation is a map where a key is a parameter name, value is a parameter regex which is used for validation. + * Regex will be applied with case insensitivity. */ - Set>> mediaTypeMappings(); + class MediaTypeValue extends Tuple> { + public MediaTypeValue(String mediaTypeValue, Map parametersForValidation) { + super(mediaTypeValue, parametersForValidation); + } + } } diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeRegistry.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeRegistry.java index 2b7a070e06c04..0984a4cdbc161 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeRegistry.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeRegistry.java @@ -19,8 +19,6 @@ package org.elasticsearch.common.xcontent; -import org.elasticsearch.common.collect.Tuple; - import java.util.HashMap; import java.util.Locale; import java.util.Map; @@ -28,21 +26,29 @@ import java.util.regex.Pattern; /** - * A registry of mappings between String typeWithSubtype to a MediaType instances. For instance application/json to XContentType.JSON - * Defines parameters that are allowed for media types and a regex to validate them. - * Specifies format path parameter values that are used to specify requested response Content-Type. + * A registry for quick media type lookup. + * It allows to find media type by specified header value - typeWithSubtype aka mediaType without parameters. + * I.e. application/json will return XContentType.JSON + * Also allows to find media type by a specified path parameter format. + * I.e. txt used in path _sql?format=txt will return TextFormat.PLAIN_TEXT + * + * Note: there might be multiple typeWithSubtype mapping to the same MediaType, + * but there is only one format path param mapping to the MediaType. + * + * The registry also specifies parameters values for validation. + * Parameter value is a String regex which will be parsed in a case-insensitive manner. */ public class MediaTypeRegistry { - private Map formatToMediaType = new HashMap<>(); + private Map queryParamToMediaType = new HashMap<>(); private Map typeWithSubtypeToMediaType = new HashMap<>(); private Map> parametersMap = new HashMap<>(); - public T formatToMediaType(String format) { + public T queryParamToMediaType(String format) { if(format == null) { return null; } - return formatToMediaType.get(format.toLowerCase(Locale.ROOT)); + return queryParamToMediaType.get(format.toLowerCase(Locale.ROOT)); } public T typeWithSubtypeToMediaType(String typeWithSubtype) { @@ -55,11 +61,11 @@ public Map parametersFor(String typeWithSubtype) { public MediaTypeRegistry register(T[] mediaTypes ) { for (T mediaType : mediaTypes) { - Set>> tuples = mediaType.mediaTypeMappings(); - for (Tuple> tuple : tuples) { - formatToMediaType.put(mediaType.formatPathParameter(),mediaType); - typeWithSubtypeToMediaType.put(tuple.v1(), mediaType); - parametersMap.put(tuple.v1(), convertPatterns(tuple.v2())); + Set tuples = mediaType.mediaTypeValues(); + for (MediaType.MediaTypeValue mediaTypeValue : tuples) { + queryParamToMediaType.put(mediaType.queryParameter(),mediaType); + typeWithSubtypeToMediaType.put(mediaTypeValue.v1(), mediaType); + parametersMap.put(mediaTypeValue.v1(), convertPatterns(mediaTypeValue.v2())); } } return this; diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ParsedMediaType.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ParsedMediaType.java index 3eef3c1ac6b75..70ff25340322a 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ParsedMediaType.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ParsedMediaType.java @@ -33,7 +33,9 @@ * @see MediaTypeRegistry */ public class ParsedMediaType { - //sun.net.www.protocol.http.HttpURLConnection sets a default Accept header if it was not provided on a request + // TODO this should be removed once strict parsing is implemented https://github.com/elastic/elasticsearch/issues/63080 + // sun.net.www.protocol.http.HttpURLConnection sets a default Accept header if it was not provided on a request. + // For this value Parsing returns null. public static final String DEFAULT_ACCEPT_STRING = "text/html, image/gif, image/jpeg, *; q=.2, */*; q=.2"; private final String type; @@ -51,7 +53,7 @@ private ParsedMediaType(String type, String subType, Map paramet /** * The parsed mime type without the associated parameters. Will always return lowercase. */ - public String mimeTypeWithoutParams() { + public String mediaTypeWithoutParameters() { return type + "/" + subType; } @@ -86,6 +88,7 @@ public static ParsedMediaType parseMediaType(String headerValue) { if (paramsAsString.isEmpty()) { continue; } + // intentionally allowing to have spaces around `=` (RFC disallows this) String[] keyValueParam = elements[i].trim().split("="); if (keyValueParam.length == 2) { String parameterName = keyValueParam[0].toLowerCase(Locale.ROOT).trim(); @@ -106,13 +109,13 @@ public static ParsedMediaType parseMediaType(String headerValue) { * Resolves this instance to a MediaType instance defined in given MediaTypeRegistry. * Performs validation against parameters. * @param mediaTypeRegistry a registry where a mapping between a raw media type to an instance MediaType is defined - * @return a MediaType instance + * @return a MediaType instance or null if no media type could be found or if a known parameter do not passes validation */ public T toMediaType(MediaTypeRegistry mediaTypeRegistry) { - T type = mediaTypeRegistry.typeWithSubtypeToMediaType(mimeTypeWithoutParams()); + T type = mediaTypeRegistry.typeWithSubtypeToMediaType(mediaTypeWithoutParameters()); if (type != null) { - Map registeredParams = mediaTypeRegistry.parametersFor(mimeTypeWithoutParams()); + Map registeredParams = mediaTypeRegistry.parametersFor(mediaTypeWithoutParameters()); for (Map.Entry givenParamEntry : parameters.entrySet()) { if (isValidParameter(givenParamEntry.getKey(), givenParamEntry.getValue(), registeredParams) == false) { return null; @@ -128,7 +131,7 @@ private boolean isValidParameter(String paramName, String value, Map>> mediaTypeMappings() { + public Set mediaTypeValues() { return Set.of( - Tuple.tuple("application/json", Map.of("charset", "UTF-8")), - Tuple.tuple("application/x-ndjson", Map.of("charset", "UTF-8")), - Tuple.tuple("application/*", Collections.emptyMap()), - Tuple.tuple("application/vnd.elasticsearch+json", + new MediaType.MediaTypeValue("application/json", Map.of("charset", "UTF-8")), + new MediaType.MediaTypeValue("application/x-ndjson", Map.of("charset", "UTF-8")), + new MediaType.MediaTypeValue("application/*", Collections.emptyMap()), + new MediaType.MediaTypeValue(VENDOR_APPLICATION_PREFIX +"json", Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", "UTF-8")), - Tuple.tuple("application/vnd.elasticsearch+x-ndjson", + new MediaType.MediaTypeValue(VENDOR_APPLICATION_PREFIX +"x-ndjson", Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", "UTF-8"))); } }, @@ -80,7 +79,7 @@ public String mediaTypeWithoutParameters() { } @Override - public String formatPathParameter() { + public String queryParameter() { return "smile"; } @@ -90,10 +89,10 @@ public XContent xContent() { } @Override - public Set>> mediaTypeMappings() { + public Set mediaTypeValues() { return Set.of( - Tuple.tuple("application/smile", Map.of("charset", "UTF-8")), - Tuple.tuple("application/vnd.elasticsearch+smile", + new MediaType.MediaTypeValue("application/smile", Map.of("charset", "UTF-8")), + new MediaType.MediaTypeValue(VENDOR_APPLICATION_PREFIX +"smile", Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", "UTF-8"))); } }, @@ -107,7 +106,7 @@ public String mediaTypeWithoutParameters() { } @Override - public String formatPathParameter() { + public String queryParameter() { return "yaml"; } @@ -117,10 +116,10 @@ public XContent xContent() { } @Override - public Set>> mediaTypeMappings() { + public Set mediaTypeValues() { return Set.of( - Tuple.tuple("application/yaml", Map.of("charset", "UTF-8")), - Tuple.tuple("application/vnd.elasticsearch+yaml", + new MediaType.MediaTypeValue("application/yaml", Map.of("charset", "UTF-8")), + new MediaType.MediaTypeValue(VENDOR_APPLICATION_PREFIX +"yaml", Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", "UTF-8"))); } }, @@ -134,7 +133,7 @@ public String mediaTypeWithoutParameters() { } @Override - public String formatPathParameter() { + public String queryParameter() { return "cbor"; } @@ -144,16 +143,17 @@ public XContent xContent() { } @Override - public Set>> mediaTypeMappings() { + public Set mediaTypeValues() { return Set.of( - Tuple.tuple("application/cbor", Map.of("charset", "UTF-8")), - Tuple.tuple("application/vnd.elasticsearch+cbor", + new MediaType.MediaTypeValue("application/cbor", Map.of("charset", "UTF-8")), + new MediaType.MediaTypeValue(VENDOR_APPLICATION_PREFIX +"cbor", Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN,"charset", "UTF-8"))); } }; public static final MediaTypeRegistry MEDIA_TYPE_REGISTRY = new MediaTypeRegistry() .register(XContentType.values()); + public static final String VENDOR_APPLICATION_PREFIX = "application/vnd.elasticsearch+"; /** * Accepts a format string, which is most of the time is equivalent to MediaType's subtype i.e. application/json @@ -162,7 +162,7 @@ public Set>> mediaTypeMappings() { * This method will return {@code null} if no match is found */ public static XContentType fromFormat(String format) { - return MEDIA_TYPE_REGISTRY.formatToMediaType(format); + return MEDIA_TYPE_REGISTRY.queryParamToMediaType(format); } /** @@ -205,10 +205,7 @@ public String mediaType() { return mediaTypeWithoutParameters(); } - public abstract XContent xContent(); public abstract String mediaTypeWithoutParameters(); - - } diff --git a/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ParsedMediaTypeTests.java b/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ParsedMediaTypeTests.java index 2f63e4e22743e..15b01014830ba 100644 --- a/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ParsedMediaTypeTests.java +++ b/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ParsedMediaTypeTests.java @@ -54,17 +54,13 @@ public void testWhiteSpaceInTypeSubtype() { equalTo(Map.of("charset", "utf-8", "compatible-with", "123"))); assertThat(ParsedMediaType.parseMediaType(mediaType + "; compatible-with=123;\n charset=UTF-8").getParameters(), equalTo(Map.of("charset", "utf-8", "compatible-with", "123"))); - - } public void testInvalidParameters() { String mediaType = "application/vnd.elasticsearch+json"; expectThrows(IllegalArgumentException.class, () -> ParsedMediaType.parseMediaType(mediaType + "; keyvalueNoEqualsSign") .toMediaType(mediaTypeRegistry)); - // allowing spaces between = - // expectThrows(IllegalArgumentException.class, () -> ParsedMediaType.parseMediaType(mediaType + "; key = value") - // .toMediaType(mediaTypeRegistry)); + expectThrows(IllegalArgumentException.class, () -> ParsedMediaType.parseMediaType(mediaType + "; key=") .toMediaType(mediaTypeRegistry)); } @@ -72,7 +68,7 @@ public void testInvalidParameters() { public void testXContentTypes() { for (XContentType xContentType : XContentType.values()) { ParsedMediaType parsedMediaType = ParsedMediaType.parseMediaType(xContentType.mediaTypeWithoutParameters()); - assertEquals(xContentType.mediaTypeWithoutParameters(), parsedMediaType.mimeTypeWithoutParams()); + assertEquals(xContentType.mediaTypeWithoutParameters(), parsedMediaType.mediaTypeWithoutParameters()); } } @@ -89,14 +85,14 @@ public void testWhiteSpaces() { //be lenient with white space since it can be really hard to troubleshoot String mediaType = " application/foo "; ParsedMediaType parsedMediaType = ParsedMediaType.parseMediaType(mediaType + " ; compatible-with = 123 ; charset=UTF-8"); - assertEquals("application/foo", parsedMediaType.mimeTypeWithoutParams()); + assertEquals("application/foo", parsedMediaType.mediaTypeWithoutParameters()); assertEquals((Map.of("charset", "utf-8", "compatible-with", "123")), parsedMediaType.getParameters()); } public void testEmptyParams() { String mediaType = "application/foo"; ParsedMediaType parsedMediaType = ParsedMediaType.parseMediaType(mediaType + randomFrom("", " ", ";", ";;", ";;;")); - assertEquals("application/foo", parsedMediaType.mimeTypeWithoutParams()); + assertEquals("application/foo", parsedMediaType.mediaTypeWithoutParameters()); assertEquals(Collections.emptyMap(), parsedMediaType.getParameters()); } diff --git a/server/src/main/java/org/elasticsearch/http/AbstractHttpServerTransport.java b/server/src/main/java/org/elasticsearch/http/AbstractHttpServerTransport.java index af8095d6dece1..c033ac9bfc62c 100644 --- a/server/src/main/java/org/elasticsearch/http/AbstractHttpServerTransport.java +++ b/server/src/main/java/org/elasticsearch/http/AbstractHttpServerTransport.java @@ -345,7 +345,7 @@ private void handleIncomingRequest(final HttpRequest httpRequest, final HttpChan RestRequest innerRestRequest; try { innerRestRequest = RestRequest.request(xContentRegistry, httpRequest, httpChannel); - } catch (final RestRequest.ContentTypeHeaderException e) { + } catch (final RestRequest.MediaTypeHeaderException e) { badRequestCause = ExceptionsHelper.useOrSuppress(badRequestCause, e); innerRestRequest = requestWithoutContentTypeHeader(httpRequest, httpChannel, badRequestCause); } catch (final RestRequest.BadParameterException e) { diff --git a/server/src/main/java/org/elasticsearch/rest/RestRequest.java b/server/src/main/java/org/elasticsearch/rest/RestRequest.java index 26dd9e7776a6d..400ad6ab11910 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestRequest.java +++ b/server/src/main/java/org/elasticsearch/rest/RestRequest.java @@ -95,7 +95,7 @@ private RestRequest(NamedXContentRegistry xContentRegistry, Map this.xContentType.set(parsedContentType.toMediaType(XContentType.MEDIA_TYPE_REGISTRY)); } }catch (IllegalArgumentException e){ - throw new ContentTypeHeaderException(e); + throw new MediaTypeHeaderException(e); } this.xContentRegistry = xContentRegistry; this.httpRequest = httpRequest; @@ -107,12 +107,13 @@ private RestRequest(NamedXContentRegistry xContentRegistry, Map } private static @Nullable ParsedMediaType parseHeaderWithMediaType(Map> headers, String headerName) { - //TOOD: shouldn't this be case insensitive ? + //TODO: make all usages of headers case-insensitive List header = headers.get(headerName); if (header == null || header.isEmpty()) { return null; } else if (header.size() > 1) { - throw new IllegalArgumentException("only one value for the header should be provided"); + throw new IllegalArgumentException("Incorrect header ["+headerName+"]. " + + "Only one value should be provided"); } String rawContentType = header.get(0); if (Strings.hasText(rawContentType)) { @@ -144,7 +145,7 @@ void ensureSafeBuffers() { * @param httpRequest the http request * @param httpChannel the http channel * @throws BadParameterException if the parameters can not be decoded - * @throws ContentTypeHeaderException if the Content-Type header can not be parsed + * @throws MediaTypeHeaderException if the Content-Type or Accept header can not be parsed */ public static RestRequest request(NamedXContentRegistry xContentRegistry, HttpRequest httpRequest, HttpChannel httpChannel) { Map params = params(httpRequest.uri()); @@ -182,7 +183,7 @@ private static String path(final String uri) { * @param xContentRegistry the content registry * @param httpRequest the http request * @param httpChannel the http channel - * @throws ContentTypeHeaderException if the Content-Type header can not be parsed + * @throws MediaTypeHeaderException if the Content-Type or Accept header can not be parsed */ public static RestRequest requestWithoutParameters(NamedXContentRegistry xContentRegistry, HttpRequest httpRequest, HttpChannel httpChannel) { @@ -548,9 +549,9 @@ public static XContentType parseContentType(List header) { throw new IllegalArgumentException("empty Content-Type header"); } - public static class ContentTypeHeaderException extends RuntimeException { + public static class MediaTypeHeaderException extends RuntimeException { - ContentTypeHeaderException(final IllegalArgumentException cause) { + MediaTypeHeaderException(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 824ec611ea767..e5ed811643d5a 100644 --- a/server/src/test/java/org/elasticsearch/rest/BytesRestResponseTests.java +++ b/server/src/test/java/org/elasticsearch/rest/BytesRestResponseTests.java @@ -299,7 +299,7 @@ public void testErrorToAndFromXContent() throws IOException { final XContentType xContentType = randomFrom(XContentType.values()); - Map params = Collections.singletonMap("format", xContentType.formatPathParameter()); + Map params = Collections.singletonMap("format", xContentType.queryParameter()); RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withParams(params).build(); RestChannel channel = detailed ? new DetailedExceptionRestChannel(request) : new SimpleExceptionRestChannel(request); diff --git a/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java b/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java index 017e304b14acc..20fa04b72573b 100644 --- a/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java +++ b/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java @@ -199,8 +199,8 @@ public void testPlainTextSupport() { public void testMalformedContentTypeHeader() { final String type = randomFrom("text", "text/:ain; charset=utf-8", "text/plain\";charset=utf-8", ":", "/", "t:/plain"); - final RestRequest.ContentTypeHeaderException e = expectThrows( - RestRequest.ContentTypeHeaderException.class, + final RestRequest.MediaTypeHeaderException e = expectThrows( + RestRequest.MediaTypeHeaderException.class, () -> { final Map> headers = Collections.singletonMap("Content-Type", Collections.singletonList(type)); contentRestRequest("", Collections.emptyMap(), headers); @@ -217,12 +217,13 @@ public void testNoContentTypeHeader() { public void testMultipleContentTypeHeaders() { List headers = new ArrayList<>(randomUnique(() -> randomAlphaOfLengthBetween(1, 16), randomIntBetween(2, 10))); - final RestRequest.ContentTypeHeaderException e = expectThrows( - RestRequest.ContentTypeHeaderException.class, + final RestRequest.MediaTypeHeaderException e = expectThrows( + RestRequest.MediaTypeHeaderException.class, () -> 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 value for the header should be provided")); + assertThat(e.getMessage(), equalTo("java.lang.IllegalArgumentException: Incorrect header [Content-Type]." + + " Only one value should be provided")); } public void testRequiredContent() { diff --git a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestResponse.java b/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestResponse.java index 0d1bf3208936b..819f33a1b3220 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestResponse.java +++ b/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestResponse.java @@ -54,7 +54,8 @@ public ClientYamlTestResponse(Response response) throws IOException { if (response.getEntity() != null) { String contentType = response.getHeader("Content-Type"); //Do not know about sql media types. relies on null - this.bodyContentType = getContentTypeIgnoreExceptions(contentType); try { + this.bodyContentType = getContentTypeIgnoreExceptions(contentType); + try { byte[] bytes = EntityUtils.toByteArray(response.getEntity()); //skip parsing if we got text back (e.g. if we called _cat apis) if (bodyContentType != null) { diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java index 536ac918aa459..d4a5fddac14ce 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java @@ -39,7 +39,8 @@ public MediaType getMediaType(RestRequest request, SqlQueryRequest sqlRequest) { // enforce CBOR response for drivers and CLI (unless instructed differently through the config param) return XContentType.CBOR; } else if (request.hasParam(URL_PARAM_FORMAT)) { - return validateColumnarRequest(sqlRequest.columnar(), MEDIA_TYPE_REGISTRY.formatToMediaType(request.param(URL_PARAM_FORMAT))); + return validateColumnarRequest(sqlRequest.columnar(), + MEDIA_TYPE_REGISTRY.queryParamToMediaType(request.param(URL_PARAM_FORMAT))); } if(request.getParsedAccept() != null) { 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 11082122a66c9..83923e1bc597d 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 @@ -85,7 +85,7 @@ String format(RestRequest request, SqlQueryResponse response) { } @Override - public String formatPathParameter() { + public String queryParameter() { return FORMAT_TEXT; } @@ -105,10 +105,12 @@ protected String eol() { } @Override - public Set>> mediaTypeMappings() { + public Set mediaTypeValues() { return Set.of( - Tuple.tuple(CONTENT_TYPE_TXT, - Map.of("header", "present|absent", "charset", "utf-8"))); + new MediaType.MediaTypeValue(CONTENT_TYPE_TXT, + Map.of("header", "present|absent", "charset", "utf-8")), + new MediaType.MediaTypeValue(VENDOR_CONTENT_TYPE_TXT, + Map.of("header", "present|absent", "charset", "utf-8", COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN))); } }, @@ -135,7 +137,7 @@ protected String eol() { } @Override - public String formatPathParameter() { + public String queryParameter() { return FORMAT_CSV; } @@ -227,11 +229,15 @@ boolean hasHeader(RestRequest request) { } @Override - public Set>> mediaTypeMappings() { + public Set mediaTypeValues() { return Set.of( - Tuple.tuple(CONTENT_TYPE_CSV, + new MediaType.MediaTypeValue(CONTENT_TYPE_CSV, Map.of("header", "present|absent", "charset", "utf-8", - "delimiter", ".+")));// more detailed parsing is in TextFormat.CSV#delimiter + "delimiter", ".+")),// more detailed parsing is in TextFormat.CSV#delimiter + new MediaType.MediaTypeValue(VENDOR_CONTENT_TYPE_CSV, + Map.of("header", "present|absent", "charset", "utf-8", + "delimiter", ".+", + COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN))); } }, @@ -249,7 +255,7 @@ protected String eol() { } @Override - public String formatPathParameter() { + public String queryParameter() { return FORMAT_TSV; } @@ -285,10 +291,13 @@ String maybeEscape(String value, Character __) { } @Override - public Set>> mediaTypeMappings() { + public Set mediaTypeValues() { return Set.of( - Tuple.tuple(CONTENT_TYPE_TSV, - Map.of("header", "present|absent", "charset", "utf-8"))); + new MediaType.MediaTypeValue(CONTENT_TYPE_TSV, + Map.of("header", "present|absent", "charset", "utf-8")), + new MediaType.MediaTypeValue(VENDOR_CONTENT_TYPE_TSV, + Map.of("header", "present|absent", "charset", "utf-8", + COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN))); } }; @@ -296,8 +305,11 @@ public Set>> mediaTypeMappings() { private static final String FORMAT_CSV = "csv"; private static final String FORMAT_TSV = "tsv"; private static final String CONTENT_TYPE_TXT = "text/plain"; + private static final String VENDOR_CONTENT_TYPE_TXT = "text/vnd.elasticsearch+plain"; private static final String CONTENT_TYPE_CSV = "text/csv"; + private static final String VENDOR_CONTENT_TYPE_CSV = "text/vnd.elasticsearch+csv"; private static final String CONTENT_TYPE_TSV = "text/tab-separated-values"; + private static final String VENDOR_CONTENT_TYPE_TSV = "text/vnd.elasticsearch+tab-separated-values"; private static final String URL_PARAM_HEADER = "header"; private static final String PARAM_HEADER_ABSENT = "absent"; private static final String PARAM_HEADER_PRESENT = "present"; diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/input/http/ExecutableHttpInput.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/input/http/ExecutableHttpInput.java index 8cce85cbba11a..39d5b2a1cd7d3 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/input/http/ExecutableHttpInput.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/input/http/ExecutableHttpInput.java @@ -99,7 +99,7 @@ HttpInput.Result doExecute(WatchExecutionContext ctx, HttpRequest request) throw } } catch (Exception e) { throw new ElasticsearchParseException("could not parse response body [{}] it does not appear to be [{}]", type(), ctx.id(), - response.body().utf8ToString(), contentType.formatPathParameter()); + response.body().utf8ToString(), contentType.queryParameter()); } } else { payloadMap.put("_value", response.body().utf8ToString()); diff --git a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/support/WatcherTemplateTests.java b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/support/WatcherTemplateTests.java index 04789dc97ea1b..bf0f3d4d82dfb 100644 --- a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/support/WatcherTemplateTests.java +++ b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/support/WatcherTemplateTests.java @@ -160,7 +160,7 @@ static String prepareTemplate(String template, @Nullable XContentType contentTyp return template; } return new StringBuilder("__") - .append(contentType.formatPathParameter().toLowerCase(Locale.ROOT)) + .append(contentType.queryParameter().toLowerCase(Locale.ROOT)) .append("__::") .append(template) .toString(); From 7779083b4e5ed3301bd75bfc340bff51bae2042c Mon Sep 17 00:00:00 2001 From: pgomulka Date: Tue, 3 Nov 2020 13:52:14 +0100 Subject: [PATCH 09/21] minor tweaks --- .../org/elasticsearch/common/xcontent/MediaType.java | 2 +- .../common/xcontent/MediaTypeRegistry.java | 4 ++-- .../common/xcontent/ParsedMediaType.java | 4 +++- .../common/xcontent/XContentTypeTests.java | 11 +++++------ 4 files changed, 11 insertions(+), 10 deletions(-) diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaType.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaType.java index 60e10010e3ed7..173f7a9c60bad 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaType.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaType.java @@ -40,7 +40,7 @@ public interface MediaType { String queryParameter(); /** - * returns a set of MediaTypeValues - allowed media type values on Accept or Content-Type headers + * Returns a set of MediaTypeValues - allowed media type values on Accept or Content-Type headers * Also defines parameters for validation. */ Set mediaTypeValues(); diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeRegistry.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeRegistry.java index 0984a4cdbc161..2fd71653f472a 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeRegistry.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeRegistry.java @@ -27,9 +27,9 @@ /** * A registry for quick media type lookup. - * It allows to find media type by specified header value - typeWithSubtype aka mediaType without parameters. + * It allows to find media type by a header value - typeWithSubtype aka mediaType without parameters. * I.e. application/json will return XContentType.JSON - * Also allows to find media type by a specified path parameter format. + * Also allows to find media type by a path parameter format. * I.e. txt used in path _sql?format=txt will return TextFormat.PLAIN_TEXT * * Note: there might be multiple typeWithSubtype mapping to the same MediaType, diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ParsedMediaType.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ParsedMediaType.java index 70ff25340322a..e63f0064d583a 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ParsedMediaType.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ParsedMediaType.java @@ -63,8 +63,10 @@ public Map getParameters() { /** * Parses a header value into it's parts. + * Note: parsing can return null, but it will throw exceptions once https://github.com/elastic/elasticsearch/issues/63080 is done + * Do not rely on nulls * - * @return a {@link ParsedMediaType} if the header could be parsed. TODO: don't return null + * @return a {@link ParsedMediaType} if the header could be parsed. * @throws IllegalArgumentException if the header is malformed */ public static ParsedMediaType parseMediaType(String headerValue) { 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 741f380a8cc0f..72bdf549b8d40 100644 --- a/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java +++ b/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java @@ -116,8 +116,6 @@ public void testVersionedMediaType() { equalTo(XContentType.JSON)); assertThat(XContentType.fromMediaType("APPLICATION/JSON"), equalTo(XContentType.JSON)); - - } public void testVersionParsing() { @@ -147,12 +145,13 @@ public void testVersionParsing() { } public void testUnrecognizedParameters() { + //unrecognised parameters are ignored String version = String.valueOf(randomNonNegativeByte()); assertThat(XContentType.fromMediaType("application/json;compatible-with=" + version), - is(nullValue())); - - assertThat(XContentType.parseVersion("application/json; sth=123"), - is(nullValue())); + is(XContentType.JSON)); + // TODO do not allow parsing unrecognized parameter value https://github.com/elastic/elasticsearch/issues/63080 + // assertThat(XContentType.parseVersion("application/json;compatible-with=123"), + // is(nullValue())); } } From 94b4a0a85ece16e597818e815d958675e5879125 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Tue, 3 Nov 2020 14:41:27 +0100 Subject: [PATCH 10/21] fix test after exception msg rename --- .../java/org/elasticsearch/rest/Netty4BadRequestIT.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/transport-netty4/src/javaRestTest/java/org/elasticsearch/rest/Netty4BadRequestIT.java b/modules/transport-netty4/src/javaRestTest/java/org/elasticsearch/rest/Netty4BadRequestIT.java index 70cf295cb54a0..6afee02d9ebb8 100644 --- a/modules/transport-netty4/src/javaRestTest/java/org/elasticsearch/rest/Netty4BadRequestIT.java +++ b/modules/transport-netty4/src/javaRestTest/java/org/elasticsearch/rest/Netty4BadRequestIT.java @@ -98,7 +98,7 @@ 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("content_type_header_exception")); + assertThat(map.get("type"), equalTo("media_type_header_exception")); assertThat(map.get("reason"), equalTo("java.lang.IllegalArgumentException: Header [Content-Type] cannot be empty.")); } } From 43855911ed3ec0ad1d245cc398e7d67f59acb975 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Tue, 3 Nov 2020 16:30:35 +0100 Subject: [PATCH 11/21] rename to header value --- .../common/xcontent/MediaType.java | 19 +++++++----- .../common/xcontent/MediaTypeRegistry.java | 8 ++--- .../common/xcontent/XContentType.java | 31 +++++++++---------- .../xpack/sql/plugin/TextFormat.java | 18 +++++------ 4 files changed, 40 insertions(+), 36 deletions(-) diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaType.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaType.java index 173f7a9c60bad..2221f1bc97614 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaType.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaType.java @@ -21,11 +21,12 @@ import org.elasticsearch.common.collect.Tuple; +import java.util.Collections; import java.util.Map; import java.util.Set; /** - * Abstracts a Media Type and a format parameter. + * Abstracts a Media Type and a query parameter format. * Media types are used as values on Content-Type and Accept headers * format is an URL parameter, specifies response media type. */ @@ -40,19 +41,23 @@ public interface MediaType { String queryParameter(); /** - * Returns a set of MediaTypeValues - allowed media type values on Accept or Content-Type headers - * Also defines parameters for validation. + * Returns a set of HeaderValues - allowed media type values on Accept or Content-Type headers + * Also defines media type parameters for validation. */ - Set mediaTypeValues(); + Set headerValues(); /** * A class to represent supported mediaType values i.e. application/json and parameters to be validated. * Parameters for validation is a map where a key is a parameter name, value is a parameter regex which is used for validation. * Regex will be applied with case insensitivity. */ - class MediaTypeValue extends Tuple> { - public MediaTypeValue(String mediaTypeValue, Map parametersForValidation) { - super(mediaTypeValue, parametersForValidation); + class HeaderValue extends Tuple> { + public HeaderValue(String headerValue, Map parametersForValidation) { + super(headerValue, parametersForValidation); + } + + public HeaderValue(String headerValue) { + this(headerValue, Collections.emptyMap()); } } } diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeRegistry.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeRegistry.java index 2fd71653f472a..981dd515f72fc 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeRegistry.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeRegistry.java @@ -61,11 +61,11 @@ public Map parametersFor(String typeWithSubtype) { public MediaTypeRegistry register(T[] mediaTypes ) { for (T mediaType : mediaTypes) { - Set tuples = mediaType.mediaTypeValues(); - for (MediaType.MediaTypeValue mediaTypeValue : tuples) { + Set tuples = mediaType.headerValues(); + for (MediaType.HeaderValue headerValue : tuples) { queryParamToMediaType.put(mediaType.queryParameter(),mediaType); - typeWithSubtypeToMediaType.put(mediaTypeValue.v1(), mediaType); - parametersMap.put(mediaTypeValue.v1(), convertPatterns(mediaTypeValue.v2())); + typeWithSubtypeToMediaType.put(headerValue.v1(), mediaType); + parametersMap.put(headerValue.v1(), convertPatterns(headerValue.v2())); } } return this; 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 6b9c6654e867f..20848b4c0bb22 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 @@ -24,7 +24,6 @@ import org.elasticsearch.common.xcontent.smile.SmileXContent; import org.elasticsearch.common.xcontent.yaml.YamlXContent; -import java.util.Collections; import java.util.Map; import java.util.Set; @@ -58,14 +57,14 @@ public XContent xContent() { } @Override - public Set mediaTypeValues() { + public Set headerValues() { return Set.of( - new MediaType.MediaTypeValue("application/json", Map.of("charset", "UTF-8")), - new MediaType.MediaTypeValue("application/x-ndjson", Map.of("charset", "UTF-8")), - new MediaType.MediaTypeValue("application/*", Collections.emptyMap()), - new MediaType.MediaTypeValue(VENDOR_APPLICATION_PREFIX +"json", + new HeaderValue("application/json", Map.of("charset", "UTF-8")), + new HeaderValue("application/x-ndjson", Map.of("charset", "UTF-8")), + new HeaderValue("application/*"), + new HeaderValue(VENDOR_APPLICATION_PREFIX +"json", Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", "UTF-8")), - new MediaType.MediaTypeValue(VENDOR_APPLICATION_PREFIX +"x-ndjson", + new HeaderValue(VENDOR_APPLICATION_PREFIX +"x-ndjson", Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", "UTF-8"))); } }, @@ -89,10 +88,10 @@ public XContent xContent() { } @Override - public Set mediaTypeValues() { + public Set headerValues() { return Set.of( - new MediaType.MediaTypeValue("application/smile", Map.of("charset", "UTF-8")), - new MediaType.MediaTypeValue(VENDOR_APPLICATION_PREFIX +"smile", + new HeaderValue("application/smile", Map.of("charset", "UTF-8")), + new HeaderValue(VENDOR_APPLICATION_PREFIX +"smile", Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", "UTF-8"))); } }, @@ -116,10 +115,10 @@ public XContent xContent() { } @Override - public Set mediaTypeValues() { + public Set headerValues() { return Set.of( - new MediaType.MediaTypeValue("application/yaml", Map.of("charset", "UTF-8")), - new MediaType.MediaTypeValue(VENDOR_APPLICATION_PREFIX +"yaml", + new HeaderValue("application/yaml", Map.of("charset", "UTF-8")), + new HeaderValue(VENDOR_APPLICATION_PREFIX +"yaml", Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", "UTF-8"))); } }, @@ -143,10 +142,10 @@ public XContent xContent() { } @Override - public Set mediaTypeValues() { + public Set headerValues() { return Set.of( - new MediaType.MediaTypeValue("application/cbor", Map.of("charset", "UTF-8")), - new MediaType.MediaTypeValue(VENDOR_APPLICATION_PREFIX +"cbor", + new HeaderValue("application/cbor", Map.of("charset", "UTF-8")), + new HeaderValue(VENDOR_APPLICATION_PREFIX +"cbor", Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN,"charset", "UTF-8"))); } }; 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 83923e1bc597d..f702df5fdfe96 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 @@ -105,11 +105,11 @@ protected String eol() { } @Override - public Set mediaTypeValues() { + public Set headerValues() { return Set.of( - new MediaType.MediaTypeValue(CONTENT_TYPE_TXT, + new HeaderValue(CONTENT_TYPE_TXT, Map.of("header", "present|absent", "charset", "utf-8")), - new MediaType.MediaTypeValue(VENDOR_CONTENT_TYPE_TXT, + new HeaderValue(VENDOR_CONTENT_TYPE_TXT, Map.of("header", "present|absent", "charset", "utf-8", COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN))); } @@ -229,12 +229,12 @@ boolean hasHeader(RestRequest request) { } @Override - public Set mediaTypeValues() { + public Set headerValues() { return Set.of( - new MediaType.MediaTypeValue(CONTENT_TYPE_CSV, + new HeaderValue(CONTENT_TYPE_CSV, Map.of("header", "present|absent", "charset", "utf-8", "delimiter", ".+")),// more detailed parsing is in TextFormat.CSV#delimiter - new MediaType.MediaTypeValue(VENDOR_CONTENT_TYPE_CSV, + new HeaderValue(VENDOR_CONTENT_TYPE_CSV, Map.of("header", "present|absent", "charset", "utf-8", "delimiter", ".+", COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN))); @@ -291,11 +291,11 @@ String maybeEscape(String value, Character __) { } @Override - public Set mediaTypeValues() { + public Set headerValues() { return Set.of( - new MediaType.MediaTypeValue(CONTENT_TYPE_TSV, + new HeaderValue(CONTENT_TYPE_TSV, Map.of("header", "present|absent", "charset", "utf-8")), - new MediaType.MediaTypeValue(VENDOR_CONTENT_TYPE_TSV, + new HeaderValue(VENDOR_CONTENT_TYPE_TSV, Map.of("header", "present|absent", "charset", "utf-8", COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN))); } From dc61731ba05c6d2338ff46cf3f3bf665b334c525 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Wed, 4 Nov 2020 10:24:25 +0100 Subject: [PATCH 12/21] remove charset validation --- .../common/xcontent/MediaTypeRegistry.java | 10 ++++++---- .../common/xcontent/ParsedMediaType.java | 3 ++- .../common/xcontent/XContentType.java | 20 +++++++++---------- .../xpack/sql/plugin/TextFormat.java | 19 +++++++----------- 4 files changed, 25 insertions(+), 27 deletions(-) diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeRegistry.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeRegistry.java index 981dd515f72fc..c1b185fbd1154 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeRegistry.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeRegistry.java @@ -32,11 +32,13 @@ * Also allows to find media type by a path parameter format. * I.e. txt used in path _sql?format=txt will return TextFormat.PLAIN_TEXT * - * Note: there might be multiple typeWithSubtype mapping to the same MediaType, - * but there is only one format path param mapping to the MediaType. + * Multiple header representations may map to a single {@link MediaType} for example, "application/json" + * and "application/vnd.elasticsearch+json" both represent a JSON MediaType. + * A MediaType can have only one query parameter representation. + * For example "json" (case insensitive) maps back to a JSON media type. * - * The registry also specifies parameters values for validation. - * Parameter value is a String regex which will be parsed in a case-insensitive manner. + * Additionally, a http header may optionally have parameters. For example "application/json; charset=utf-8". + * This class also allows to define a regular expression for valid values of charset. */ public class MediaTypeRegistry { diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ParsedMediaType.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ParsedMediaType.java index e63f0064d583a..fbae201451441 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ParsedMediaType.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ParsedMediaType.java @@ -90,7 +90,8 @@ public static ParsedMediaType parseMediaType(String headerValue) { if (paramsAsString.isEmpty()) { continue; } - // intentionally allowing to have spaces around `=` (RFC disallows this) + // intentionally allowing to have spaces around `=` + // https://tools.ietf.org/html/rfc7231#section-3.1.1.1 disallows this String[] keyValueParam = elements[i].trim().split("="); if (keyValueParam.length == 2) { String parameterName = keyValueParam[0].toLowerCase(Locale.ROOT).trim(); 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 20848b4c0bb22..8ab6115a22c23 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 @@ -59,13 +59,13 @@ public XContent xContent() { @Override public Set headerValues() { return Set.of( - new HeaderValue("application/json", Map.of("charset", "UTF-8")), - new HeaderValue("application/x-ndjson", Map.of("charset", "UTF-8")), + new HeaderValue("application/json"), + new HeaderValue("application/x-ndjson"), new HeaderValue("application/*"), new HeaderValue(VENDOR_APPLICATION_PREFIX +"json", - Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", "UTF-8")), + Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN)), new HeaderValue(VENDOR_APPLICATION_PREFIX +"x-ndjson", - Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", "UTF-8"))); + Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN))); } }, /** @@ -90,9 +90,9 @@ public XContent xContent() { @Override public Set headerValues() { return Set.of( - new HeaderValue("application/smile", Map.of("charset", "UTF-8")), + new HeaderValue("application/smile"), new HeaderValue(VENDOR_APPLICATION_PREFIX +"smile", - Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", "UTF-8"))); + Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN))); } }, /** @@ -117,9 +117,9 @@ public XContent xContent() { @Override public Set headerValues() { return Set.of( - new HeaderValue("application/yaml", Map.of("charset", "UTF-8")), + new HeaderValue("application/yaml"), new HeaderValue(VENDOR_APPLICATION_PREFIX +"yaml", - Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", "UTF-8"))); + Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN))); } }, /** @@ -144,9 +144,9 @@ public XContent xContent() { @Override public Set headerValues() { return Set.of( - new HeaderValue("application/cbor", Map.of("charset", "UTF-8")), + new HeaderValue("application/cbor"), new HeaderValue(VENDOR_APPLICATION_PREFIX +"cbor", - Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN,"charset", "UTF-8"))); + Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN))); } }; 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 f702df5fdfe96..6b771dcfd7ca3 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 @@ -108,9 +108,9 @@ protected String eol() { public Set headerValues() { return Set.of( new HeaderValue(CONTENT_TYPE_TXT, - Map.of("header", "present|absent", "charset", "utf-8")), + Map.of("header", "present|absent")), new HeaderValue(VENDOR_CONTENT_TYPE_TXT, - Map.of("header", "present|absent", "charset", "utf-8", COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN))); + Map.of("header", "present|absent", COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN))); } }, @@ -232,12 +232,9 @@ boolean hasHeader(RestRequest request) { public Set headerValues() { return Set.of( new HeaderValue(CONTENT_TYPE_CSV, - Map.of("header", "present|absent", "charset", "utf-8", - "delimiter", ".+")),// more detailed parsing is in TextFormat.CSV#delimiter + Map.of("header", "present|absent","delimiter", ".+")),// more detailed parsing is in TextFormat.CSV#delimiter new HeaderValue(VENDOR_CONTENT_TYPE_CSV, - Map.of("header", "present|absent", "charset", "utf-8", - "delimiter", ".+", - COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN))); + Map.of("header", "present|absent","delimiter", ".+", COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN))); } }, @@ -291,13 +288,11 @@ String maybeEscape(String value, Character __) { } @Override - public Set headerValues() { + public Set headerValues() { return Set.of( - new HeaderValue(CONTENT_TYPE_TSV, - Map.of("header", "present|absent", "charset", "utf-8")), + new HeaderValue(CONTENT_TYPE_TSV, Map.of("header", "present|absent")), new HeaderValue(VENDOR_CONTENT_TYPE_TSV, - Map.of("header", "present|absent", "charset", "utf-8", - COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN))); + Map.of("header", "present|absent", COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN))); } }; From 4a3138dd46c7826e902f0375e2a61d5aef036c70 Mon Sep 17 00:00:00 2001 From: Przemyslaw Gomulka Date: Thu, 5 Nov 2020 08:33:33 +0100 Subject: [PATCH 13/21] Apply suggestions from code review Co-authored-by: Jay Modi --- .../common/xcontent/MediaTypeRegistry.java | 6 +++--- .../elasticsearch/common/xcontent/ParsedMediaType.java | 2 +- .../elasticsearch/common/xcontent/XContentType.java | 10 +++++----- .../main/java/org/elasticsearch/rest/RestHandler.java | 2 +- .../main/java/org/elasticsearch/rest/RestRequest.java | 6 +++--- .../org/elasticsearch/rest/action/cat/RestTable.java | 2 +- .../xpack/sql/plugin/RestSqlQueryAction.java | 2 +- .../xpack/sql/plugin/SqlMediaTypeParser.java | 2 +- .../xpack/watcher/common/http/HttpResponse.java | 2 +- 9 files changed, 17 insertions(+), 17 deletions(-) diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeRegistry.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeRegistry.java index c1b185fbd1154..8324adfa25384 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeRegistry.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeRegistry.java @@ -47,7 +47,7 @@ public class MediaTypeRegistry { private Map> parametersMap = new HashMap<>(); public T queryParamToMediaType(String format) { - if(format == null) { + if (format == null) { return null; } return queryParamToMediaType.get(format.toLowerCase(Locale.ROOT)); @@ -65,7 +65,7 @@ public MediaTypeRegistry register(T[] mediaTypes ) { for (T mediaType : mediaTypes) { Set tuples = mediaType.headerValues(); for (MediaType.HeaderValue headerValue : tuples) { - queryParamToMediaType.put(mediaType.queryParameter(),mediaType); + queryParamToMediaType.put(mediaType.queryParameter(), mediaType); typeWithSubtypeToMediaType.put(headerValue.v1(), mediaType); parametersMap.put(headerValue.v1(), convertPatterns(headerValue.v2())); } @@ -73,7 +73,7 @@ public MediaTypeRegistry register(T[] mediaTypes ) { return this; } - private Map convertPatterns(Map paramNameAndValueRegex){ + private Map convertPatterns(Map paramNameAndValueRegex) { Map parametersForMediaType = new HashMap<>(paramNameAndValueRegex.size()); for (Map.Entry params : paramNameAndValueRegex.entrySet()) { String parameterName = params.getKey().toLowerCase(Locale.ROOT); diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ParsedMediaType.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ParsedMediaType.java index fbae201451441..83676cbfa1902 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ParsedMediaType.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ParsedMediaType.java @@ -130,7 +130,7 @@ public T toMediaType(MediaTypeRegistry mediaTypeRegist } private boolean isValidParameter(String paramName, String value, Map registeredParams) { - if(registeredParams.containsKey(paramName)){ + if (registeredParams.containsKey(paramName)) { Pattern regex = registeredParams.get(paramName); return regex.matcher(value).matches(); } 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 8ab6115a22c23..c1cd11aef5f19 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 @@ -62,9 +62,9 @@ public Set headerValues() { new HeaderValue("application/json"), new HeaderValue("application/x-ndjson"), new HeaderValue("application/*"), - new HeaderValue(VENDOR_APPLICATION_PREFIX +"json", + new HeaderValue(VENDOR_APPLICATION_PREFIX + "json", Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN)), - new HeaderValue(VENDOR_APPLICATION_PREFIX +"x-ndjson", + new HeaderValue(VENDOR_APPLICATION_PREFIX + "x-ndjson", Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN))); } }, @@ -91,7 +91,7 @@ public XContent xContent() { public Set headerValues() { return Set.of( new HeaderValue("application/smile"), - new HeaderValue(VENDOR_APPLICATION_PREFIX +"smile", + new HeaderValue(VENDOR_APPLICATION_PREFIX + "smile", Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN))); } }, @@ -118,7 +118,7 @@ public XContent xContent() { public Set headerValues() { return Set.of( new HeaderValue("application/yaml"), - new HeaderValue(VENDOR_APPLICATION_PREFIX +"yaml", + new HeaderValue(VENDOR_APPLICATION_PREFIX + "yaml", Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN))); } }, @@ -145,7 +145,7 @@ public XContent xContent() { public Set headerValues() { return Set.of( new HeaderValue("application/cbor"), - new HeaderValue(VENDOR_APPLICATION_PREFIX +"cbor", + new HeaderValue(VENDOR_APPLICATION_PREFIX + "cbor", Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN))); } }; diff --git a/server/src/main/java/org/elasticsearch/rest/RestHandler.java b/server/src/main/java/org/elasticsearch/rest/RestHandler.java index 21d4a6f278f3c..aa9393d7aa834 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestHandler.java +++ b/server/src/main/java/org/elasticsearch/rest/RestHandler.java @@ -102,7 +102,7 @@ default boolean allowSystemIndexAccessByDefault() { return false; } - default MediaTypeRegistry validAcceptMediaTypes(){ + default MediaTypeRegistry validAcceptMediaTypes() { return XContentType.MEDIA_TYPE_REGISTRY; } diff --git a/server/src/main/java/org/elasticsearch/rest/RestRequest.java b/server/src/main/java/org/elasticsearch/rest/RestRequest.java index 400ad6ab11910..49a8eabef5f46 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestRequest.java +++ b/server/src/main/java/org/elasticsearch/rest/RestRequest.java @@ -88,13 +88,13 @@ protected RestRequest(NamedXContentRegistry xContentRegistry, Map params, String path, Map> headers, HttpRequest httpRequest, HttpChannel httpChannel, long requestId) { - try{ + try { this.parsedAccept = parseHeaderWithMediaType(httpRequest.getHeaders(), "Accept"); this.parsedContentType = parseHeaderWithMediaType(httpRequest.getHeaders(), "Content-Type"); if (parsedContentType != null) { this.xContentType.set(parsedContentType.toMediaType(XContentType.MEDIA_TYPE_REGISTRY)); } - }catch (IllegalArgumentException e){ + } catch (IllegalArgumentException e) { throw new MediaTypeHeaderException(e); } this.xContentRegistry = xContentRegistry; @@ -112,7 +112,7 @@ private RestRequest(NamedXContentRegistry xContentRegistry, Map if (header == null || header.isEmpty()) { return null; } else if (header.size() > 1) { - throw new IllegalArgumentException("Incorrect header ["+headerName+"]. " + + throw new IllegalArgumentException("Incorrect header [" + headerName + "]. " + "Only one value should be provided"); } String rawContentType = header.get(0); diff --git a/server/src/main/java/org/elasticsearch/rest/action/cat/RestTable.java b/server/src/main/java/org/elasticsearch/rest/action/cat/RestTable.java index 0587e633867e6..645d3afdcd18b 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/cat/RestTable.java +++ b/server/src/main/java/org/elasticsearch/rest/action/cat/RestTable.java @@ -62,7 +62,7 @@ private static XContentType getResponseContentType(RestRequest request) { if (request.hasParam("format")) { return XContentType.fromFormat(request.param("format")); } - if(request.getParsedAccept() != null){ + if (request.getParsedAccept() != null) { return request.getParsedAccept().toMediaType(XContentType.MEDIA_TYPE_REGISTRY); } return null; diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java index 155bb7de05bd3..7243b8b8ad49e 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java @@ -45,7 +45,7 @@ public List routes() { new Route(POST, Protocol.SQL_QUERY_REST_ENDPOINT)); } - public MediaTypeRegistry validAcceptMediaTypes(){ + public MediaTypeRegistry validAcceptMediaTypes() { return SqlMediaTypeParser.MEDIA_TYPE_REGISTRY; } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java index d4a5fddac14ce..560ad710dfd94 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java @@ -43,7 +43,7 @@ public MediaType getMediaType(RestRequest request, SqlQueryRequest sqlRequest) { MEDIA_TYPE_REGISTRY.queryParamToMediaType(request.param(URL_PARAM_FORMAT))); } - if(request.getParsedAccept() != null) { + if (request.getParsedAccept() != null) { return request.getParsedAccept().toMediaType(MEDIA_TYPE_REGISTRY); } return request.getXContentType(); diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/http/HttpResponse.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/http/HttpResponse.java index 0b98c884d99b9..e89e049ffe482 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/http/HttpResponse.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/http/HttpResponse.java @@ -107,7 +107,7 @@ public XContentType xContentType() { } try { return XContentType.fromMediaType(values[0]); - }catch (IllegalArgumentException e){ + } catch (IllegalArgumentException e) { //HttpInputTests - content-type being unrecognized_content_type return null; } From 0b565e566cfb0555b25d02e082105bb482e7e6fd Mon Sep 17 00:00:00 2001 From: pgomulka Date: Thu, 5 Nov 2020 09:02:03 +0100 Subject: [PATCH 14/21] javadoc --- .../test/rest/yaml/ClientYamlTestResponse.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestResponse.java b/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestResponse.java index 819f33a1b3220..5e000d38c0531 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestResponse.java +++ b/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestResponse.java @@ -53,7 +53,7 @@ public ClientYamlTestResponse(Response response) throws IOException { this.response = response; if (response.getEntity() != null) { String contentType = response.getHeader("Content-Type"); - //Do not know about sql media types. relies on null + this.bodyContentType = getContentTypeIgnoreExceptions(contentType); try { byte[] bytes = EntityUtils.toByteArray(response.getEntity()); @@ -72,6 +72,12 @@ public ClientYamlTestResponse(Response response) throws IOException { } } + /** + * A content type returned on a response can be a media type defined outside XContentType (for instance plain/text, plain/csv etc). + * This means that the response cannot be parsed.DefaultHttpHeaders + * Also in testing there is no access to media types defined outside of XContentType. + * Therefore a null has to be returned if a response content-type has a mediatype not defined in XContentType. + */ private XContentType getContentTypeIgnoreExceptions(String contentType) { try { return XContentType.fromMediaType(contentType); From afba70d3cd862b7b3a83536381bf32e5cea575e6 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Thu, 5 Nov 2020 10:57:15 +0100 Subject: [PATCH 15/21] javadoc and cleanup --- server/src/main/java/org/elasticsearch/Version.java | 3 ++- .../src/main/java/org/elasticsearch/rest/MethodHandlers.java | 5 +++-- .../src/main/java/org/elasticsearch/rest/RestController.java | 3 ++- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/Version.java b/server/src/main/java/org/elasticsearch/Version.java index 268aa32558dee..6d213871c6033 100644 --- a/server/src/main/java/org/elasticsearch/Version.java +++ b/server/src/main/java/org/elasticsearch/Version.java @@ -271,11 +271,12 @@ private static Version fromStringSlow(String version) { this.toString = major + "." + minor + "." + revision; this.previousMajorId = major > 0 ? (major - 1) * 1000000 + 99 : major; } + public Version previousMajor() { return Version.fromId(previousMajorId); } - public static Version minimumRestCompatibilityVersion(){ + public static Version minimumRestCompatibilityVersion() { return Version.CURRENT.previousMajor(); } diff --git a/server/src/main/java/org/elasticsearch/rest/MethodHandlers.java b/server/src/main/java/org/elasticsearch/rest/MethodHandlers.java index 22494adefe1a9..b2664fc82ee83 100644 --- a/server/src/main/java/org/elasticsearch/rest/MethodHandlers.java +++ b/server/src/main/java/org/elasticsearch/rest/MethodHandlers.java @@ -58,7 +58,9 @@ MethodHandlers addMethods(RestHandler handler, Version version, RestRequest.Meth } /** - * Returns the handler for the given method and version or {@code null} if none exists. + * Returns the handler for the given method and version. + * If a handler for given version do not exist, a handler for Version.CURRENT will be returned. + * or {@code null} if none exists. */ RestHandler getHandler(RestRequest.Method method, Version version) { Map versionToHandlers = methodHandlers.get(method); @@ -67,7 +69,6 @@ RestHandler getHandler(RestRequest.Method method, Version version) { } final RestHandler handler = versionToHandlers.get(version); return handler != null || version.equals(Version.CURRENT) ? handler : versionToHandlers.get(Version.CURRENT); - } /** diff --git a/server/src/main/java/org/elasticsearch/rest/RestController.java b/server/src/main/java/org/elasticsearch/rest/RestController.java index 000701eab02c1..5f877c18af116 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestController.java +++ b/server/src/main/java/org/elasticsearch/rest/RestController.java @@ -250,8 +250,9 @@ private void dispatchRequest(RestRequest request, RestChannel channel, RestHandl inFlightRequestsBreaker(circuitBreakerService).addWithoutBreaking(contentLength); } // iff we could reserve bytes for the request we need to send the response also over this channel - // Using a version from a handler because if a version from headers was PREVIOUS, but no handler was found for that version, + // Using a version from a handler because if no handler was found for requested version, // we would return a handler for CURRENT. Therefore no compatible logic in serialisation (toXContent) should be applied + // see MethodHandlers#getHandler responseChannel = new ResourceHandlingHttpChannel(channel, circuitBreakerService, contentLength, handler.compatibleWithVersion()); // TODO: Count requests double in the circuit breaker if they need copying? From 836fe0f1b842929aa6c6e46694aa4e8ca8dead95 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Fri, 6 Nov 2020 09:09:28 +0100 Subject: [PATCH 16/21] tests and javadoc --- .../elasticsearch/rest/CompatibleVersion.java | 4 +- .../elasticsearch/rest/MethodHandlers.java | 10 +-- .../elasticsearch/rest/RestController.java | 4 +- .../rest/MethodHandlersTests.java | 89 +++++++++++++++++++ .../rest/RestControllerTests.java | 2 +- 5 files changed, 99 insertions(+), 10 deletions(-) create mode 100644 server/src/test/java/org/elasticsearch/rest/MethodHandlersTests.java diff --git a/server/src/main/java/org/elasticsearch/rest/CompatibleVersion.java b/server/src/main/java/org/elasticsearch/rest/CompatibleVersion.java index feedb7aaac373..1e3181bdbedf8 100644 --- a/server/src/main/java/org/elasticsearch/rest/CompatibleVersion.java +++ b/server/src/main/java/org/elasticsearch/rest/CompatibleVersion.java @@ -24,8 +24,8 @@ import org.elasticsearch.common.xcontent.ParsedMediaType; /** - * An interface used to specify a function that returns a compatible API version - * Intended to be used in a code base instead of a plugin. + * An interface used to specify a function that returns a compatible API version. + * This function abstracts how the version calculation is provided (for instance from plugin). */ @FunctionalInterface public interface CompatibleVersion { diff --git a/server/src/main/java/org/elasticsearch/rest/MethodHandlers.java b/server/src/main/java/org/elasticsearch/rest/MethodHandlers.java index b2664fc82ee83..b3dcf812b2323 100644 --- a/server/src/main/java/org/elasticsearch/rest/MethodHandlers.java +++ b/server/src/main/java/org/elasticsearch/rest/MethodHandlers.java @@ -26,19 +26,19 @@ import java.util.Set; /** - * Encapsulate multiple handlers for the same path, allowing different handlers for different HTTP verbs. + * Encapsulate multiple handlers for the same path, allowing different handlers for different HTTP verbs and versions. */ final class MethodHandlers { private final String path; private final Map> methodHandlers; - MethodHandlers(String path, RestHandler handler, Version version, RestRequest.Method... methods) { + MethodHandlers(String path, RestHandler handler, RestRequest.Method... methods) { this.path = path; this.methodHandlers = new HashMap<>(methods.length); for (RestRequest.Method method : methods) { methodHandlers.computeIfAbsent(method, k -> new HashMap<>()) - .put(version, handler); + .put(handler.compatibleWithVersion(), handler); } } @@ -46,10 +46,10 @@ final class MethodHandlers { * Add a handler for an additional array of methods. Note that {@code MethodHandlers} * does not allow replacing the handler for an already existing method. */ - MethodHandlers addMethods(RestHandler handler, Version version, RestRequest.Method... methods) { + MethodHandlers addMethods(RestHandler handler, RestRequest.Method... methods) { for (RestRequest.Method method : methods) { RestHandler existing = methodHandlers.computeIfAbsent(method, k -> new HashMap<>()) - .putIfAbsent(version, handler); + .putIfAbsent(handler.compatibleWithVersion(), handler); if (existing != null) { throw new IllegalArgumentException("Cannot replace existing handler for [" + path + "] for method: " + method); } diff --git a/server/src/main/java/org/elasticsearch/rest/RestController.java b/server/src/main/java/org/elasticsearch/rest/RestController.java index 5f877c18af116..b1e69d7f8217f 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestController.java +++ b/server/src/main/java/org/elasticsearch/rest/RestController.java @@ -176,8 +176,8 @@ private void registerHandlerNoWrap(RestRequest.Method method, String path, RestH assert Version.minimumRestCompatibilityVersion() == version || Version.CURRENT == version : "REST API compatibility is only supported for version " + Version.minimumRestCompatibilityVersion().major; - handlers.insertOrUpdate(path, new MethodHandlers(path, maybeWrappedHandler, version, method), - (mHandlers, newMHandler) -> mHandlers.addMethods(maybeWrappedHandler, version, method)); + handlers.insertOrUpdate(path, new MethodHandlers(path, maybeWrappedHandler, method), + (mHandlers, newMHandler) -> mHandlers.addMethods(maybeWrappedHandler, method)); } /** diff --git a/server/src/test/java/org/elasticsearch/rest/MethodHandlersTests.java b/server/src/test/java/org/elasticsearch/rest/MethodHandlersTests.java new file mode 100644 index 0000000000000..e9afe46ee2982 --- /dev/null +++ b/server/src/test/java/org/elasticsearch/rest/MethodHandlersTests.java @@ -0,0 +1,89 @@ +/* + * 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.client.node.NodeClient; +import org.elasticsearch.test.ESTestCase; + +import static org.hamcrest.Matchers.sameInstance; + +public class MethodHandlersTests extends ESTestCase { + + public void testLookupForDifferentMethodsSameVersion() { + RestHandler putHandler = new CurrentVersionHandler(); + RestHandler postHandler = new CurrentVersionHandler(); + MethodHandlers methodHandlers = new MethodHandlers("path", putHandler, RestRequest.Method.PUT); + methodHandlers.addMethods(postHandler, RestRequest.Method.POST); + + RestHandler handler = methodHandlers.getHandler(RestRequest.Method.PUT, Version.CURRENT); + assertThat(handler, sameInstance(putHandler)); + } + + public void testLookupForHandlerUnderMultipleMethods() { + RestHandler handler = new CurrentVersionHandler(); + MethodHandlers methodHandlers = new MethodHandlers("path", handler, RestRequest.Method.PUT, RestRequest.Method.POST); + + RestHandler handlerFound = methodHandlers.getHandler(RestRequest.Method.PUT, Version.CURRENT); + assertThat(handlerFound, sameInstance(handler)); + + handlerFound = methodHandlers.getHandler(RestRequest.Method.POST, Version.CURRENT); + assertThat(handlerFound, sameInstance(handler)); + } + + public void testLookupForHandlersUnderDifferentVersions() { + RestHandler currentVersionHandler = new CurrentVersionHandler(); + RestHandler previousVersionHandler = new PreviousVersionHandler(); + MethodHandlers methodHandlers = new MethodHandlers("path", currentVersionHandler, RestRequest.Method.PUT); + methodHandlers.addMethods(previousVersionHandler, RestRequest.Method.PUT); + + RestHandler handler = methodHandlers.getHandler(RestRequest.Method.PUT, Version.CURRENT); + assertThat(handler, sameInstance(currentVersionHandler)); + + handler = methodHandlers.getHandler(RestRequest.Method.PUT, Version.CURRENT.previousMajor()); + assertThat(handler, sameInstance(previousVersionHandler)); + } + + public void testExceptionOnOverride() { + RestHandler currentVersionHandler = new CurrentVersionHandler(); + + MethodHandlers methodHandlers = new MethodHandlers("path", currentVersionHandler, RestRequest.Method.PUT); + expectThrows(IllegalArgumentException.class, () -> methodHandlers.addMethods(currentVersionHandler, RestRequest.Method.PUT)); + } + + static class CurrentVersionHandler implements RestHandler { + + @Override + public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception { + + } + } + + static class PreviousVersionHandler implements RestHandler { + @Override + public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception { + } + + @Override + public Version compatibleWithVersion() { + return Version.CURRENT.previousMajor(); + } + } +} diff --git a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java index 8f882dabf3ae3..5c56362b6d89b 100644 --- a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java +++ b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java @@ -142,7 +142,7 @@ public MethodHandlers next() { assertEquals("true", threadContext.getHeader("header.1")); assertEquals("true", threadContext.getHeader("header.2")); assertNull(threadContext.getHeader("header.3")); - }, Version.CURRENT, RestRequest.Method.GET); + }, RestRequest.Method.GET); } }); AssertingChannel channel = new AssertingChannel(fakeRequest, false, RestStatus.BAD_REQUEST); From 8f2ea927874ce0e737322b6cfc9f348e1f31fa5b Mon Sep 17 00:00:00 2001 From: pgomulka Date: Mon, 9 Nov 2020 13:05:40 +0100 Subject: [PATCH 17/21] javadoc --- .../common/xcontent/XContentBuilder.java | 9 ++++++- .../main/java/org/elasticsearch/Version.java | 24 ++++++++++++------- .../java/org/elasticsearch/node/Node.java | 1 - .../elasticsearch/rest/MethodHandlers.java | 4 +++- .../elasticsearch/rest/RestController.java | 4 ++-- .../rest/MethodHandlersTests.java | 14 +++++++++++ .../rest/RestControllerTests.java | 4 ++-- 7 files changed, 45 insertions(+), 15 deletions(-) diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java index 6caa5af826899..8889b601d17ec 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java @@ -1006,11 +1006,18 @@ public XContentBuilder copyCurrentStructure(XContentParser parser) throws IOExce return this; } - public XContentBuilder withCompatibleMajorVersion(byte compatibleMajorVersion){ + /** + * Sets a version used for serialising a response compatible with a previous version. + */ + public XContentBuilder withCompatibleMajorVersion(byte compatibleMajorVersion) { + assert compatibleMajorVersion == 0 : "Compatible version has already been set"; this.compatibleMajorVersion = compatibleMajorVersion; return this; } + /** + * Returns a version used for serialising a response compatible with a previous version. + */ public byte getCompatibleMajorVersion() { return compatibleMajorVersion; } diff --git a/server/src/main/java/org/elasticsearch/Version.java b/server/src/main/java/org/elasticsearch/Version.java index 6d213871c6033..adccc5a5cb364 100644 --- a/server/src/main/java/org/elasticsearch/Version.java +++ b/server/src/main/java/org/elasticsearch/Version.java @@ -272,14 +272,6 @@ private static Version fromStringSlow(String version) { this.previousMajorId = major > 0 ? (major - 1) * 1000000 + 99 : major; } - public Version previousMajor() { - return Version.fromId(previousMajorId); - } - - public static Version minimumRestCompatibilityVersion() { - return Version.CURRENT.previousMajor(); - } - public boolean after(Version version) { return version.id < id; } @@ -401,6 +393,22 @@ public boolean isCompatible(Version version) { return compatible; } + /** + * Returns the minimum version that can be used for compatible REST API + */ + public Version minimumRestCompatibilityVersion() { + return Version.CURRENT.previousMajor(); + } + + /** + * Returns a first major version previous to the version stored in this object. + * I.e 8.1.0 will return 7.0.0 + */ + public Version previousMajor() { + return Version.fromId(previousMajorId); + } + + @SuppressForbidden(reason = "System.out.*") public static void main(String[] args) { final String versionOutput = String.format( diff --git a/server/src/main/java/org/elasticsearch/node/Node.java b/server/src/main/java/org/elasticsearch/node/Node.java index d2b5769891bd2..3da9923a0eafe 100644 --- a/server/src/main/java/org/elasticsearch/node/Node.java +++ b/server/src/main/java/org/elasticsearch/node/Node.java @@ -540,7 +540,6 @@ protected Node(final Environment initialEnvironment, repositoriesServiceReference::get).stream()) .collect(Collectors.toList()); - ActionModule actionModule = new ActionModule(settings, clusterModule.getIndexNameExpressionResolver(), settingsModule.getIndexScopedSettings(), settingsModule.getClusterSettings(), settingsModule.getSettingsFilter(), threadPool, pluginsService.filterPlugins(ActionPlugin.class), client, circuitBreakerService, usageService, systemIndices, diff --git a/server/src/main/java/org/elasticsearch/rest/MethodHandlers.java b/server/src/main/java/org/elasticsearch/rest/MethodHandlers.java index b3dcf812b2323..761e1f5ae403a 100644 --- a/server/src/main/java/org/elasticsearch/rest/MethodHandlers.java +++ b/server/src/main/java/org/elasticsearch/rest/MethodHandlers.java @@ -59,6 +59,7 @@ MethodHandlers addMethods(RestHandler handler, RestRequest.Method... methods) { /** * Returns the handler for the given method and version. + * * If a handler for given version do not exist, a handler for Version.CURRENT will be returned. * or {@code null} if none exists. */ @@ -68,7 +69,8 @@ RestHandler getHandler(RestRequest.Method method, Version version) { return null; //method not found } final RestHandler handler = versionToHandlers.get(version); - return handler != null || version.equals(Version.CURRENT) ? handler : versionToHandlers.get(Version.CURRENT); + return handler == null ? versionToHandlers.get(Version.CURRENT) : handler; + } /** diff --git a/server/src/main/java/org/elasticsearch/rest/RestController.java b/server/src/main/java/org/elasticsearch/rest/RestController.java index b1e69d7f8217f..65631893cb621 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestController.java +++ b/server/src/main/java/org/elasticsearch/rest/RestController.java @@ -173,8 +173,8 @@ protected void registerHandler(RestRequest.Method method, String path, RestHandl private void registerHandlerNoWrap(RestRequest.Method method, String path, RestHandler maybeWrappedHandler) { final Version version = maybeWrappedHandler.compatibleWithVersion(); - assert Version.minimumRestCompatibilityVersion() == version || Version.CURRENT == version - : "REST API compatibility is only supported for version " + Version.minimumRestCompatibilityVersion().major; + assert Version.CURRENT.minimumRestCompatibilityVersion() == version || Version.CURRENT == version + : "REST API compatibility is only supported for version " + Version.CURRENT.minimumRestCompatibilityVersion().major; handlers.insertOrUpdate(path, new MethodHandlers(path, maybeWrappedHandler, method), (mHandlers, newMHandler) -> mHandlers.addMethods(maybeWrappedHandler, method)); diff --git a/server/src/test/java/org/elasticsearch/rest/MethodHandlersTests.java b/server/src/test/java/org/elasticsearch/rest/MethodHandlersTests.java index e9afe46ee2982..0d92b9e1b4f84 100644 --- a/server/src/test/java/org/elasticsearch/rest/MethodHandlersTests.java +++ b/server/src/test/java/org/elasticsearch/rest/MethodHandlersTests.java @@ -68,6 +68,20 @@ public void testExceptionOnOverride() { expectThrows(IllegalArgumentException.class, () -> methodHandlers.addMethods(currentVersionHandler, RestRequest.Method.PUT)); } + public void testMissingCurrentHandler(){ + RestHandler previousVersionHandler = new PreviousVersionHandler(); + MethodHandlers methodHandlers = new MethodHandlers("path", previousVersionHandler, RestRequest.Method.PUT, RestRequest.Method.POST); + RestHandler handler = methodHandlers.getHandler(RestRequest.Method.PUT, Version.CURRENT); + assertNull(handler); + } + + public void testMissingPriorHandlerReturnsCurrentHandler(){ + RestHandler currentVersionHandler = new CurrentVersionHandler(); + MethodHandlers methodHandlers = new MethodHandlers("path", currentVersionHandler, RestRequest.Method.PUT, RestRequest.Method.POST); + RestHandler handler = methodHandlers.getHandler(RestRequest.Method.PUT, Version.CURRENT.previousMajor()); + assertThat(handler, sameInstance(currentVersionHandler)); + } + static class CurrentVersionHandler implements RestHandler { @Override diff --git a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java index 5c56362b6d89b..df95f65d92d27 100644 --- a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java +++ b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java @@ -639,9 +639,9 @@ public Exception getInboundException() { public void testDispatchCompatibleHandler() { RestController restController = new RestController(Collections.emptySet(), null, client, circuitBreakerService, usageService, - (a,c,h)->Version.minimumRestCompatibilityVersion());//always return compatible version + (a,c,h)->Version.CURRENT.minimumRestCompatibilityVersion());//always return compatible version - final byte version = Version.minimumRestCompatibilityVersion().major; + final byte version = Version.CURRENT.minimumRestCompatibilityVersion().major; final String mimeType = randomCompatibleMimeType(version); String content = randomAlphaOfLength((int) Math.round(BREAKER_LIMIT.getBytes() / inFlightRequestsBreaker.getOverhead())); From d70a0712bc4578e4351e6deb29a6adbb60fd69b6 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Mon, 9 Nov 2020 13:13:22 +0100 Subject: [PATCH 18/21] do not set compatible version twice --- .../java/org/elasticsearch/common/xcontent/XContentBuilder.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java index 8889b601d17ec..f598f81679df5 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java @@ -1010,7 +1010,7 @@ public XContentBuilder copyCurrentStructure(XContentParser parser) throws IOExce * Sets a version used for serialising a response compatible with a previous version. */ public XContentBuilder withCompatibleMajorVersion(byte compatibleMajorVersion) { - assert compatibleMajorVersion == 0 : "Compatible version has already been set"; + assert compatibleMajorVersion != 0 : "Compatible version has already been set"; this.compatibleMajorVersion = compatibleMajorVersion; return this; } From deff73987b4db898044e43ffae89a02a2ac66b2e Mon Sep 17 00:00:00 2001 From: pgomulka Date: Mon, 9 Nov 2020 16:06:50 +0100 Subject: [PATCH 19/21] javadocs --- .../src/main/java/org/elasticsearch/rest/MethodHandlers.java | 2 ++ .../src/main/java/org/elasticsearch/rest/RestController.java | 3 --- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/rest/MethodHandlers.java b/server/src/main/java/org/elasticsearch/rest/MethodHandlers.java index 761e1f5ae403a..4990504baf7fe 100644 --- a/server/src/main/java/org/elasticsearch/rest/MethodHandlers.java +++ b/server/src/main/java/org/elasticsearch/rest/MethodHandlers.java @@ -61,6 +61,8 @@ MethodHandlers addMethods(RestHandler handler, RestRequest.Method... methods) { * Returns the handler for the given method and version. * * If a handler for given version do not exist, a handler for Version.CURRENT will be returned. + * The reasoning behind is that in a minor a new API could be added passively, therefore new APIs are compatible + * (as opposed to non-compatible/breaking) * or {@code null} if none exists. */ RestHandler getHandler(RestRequest.Method method, Version version) { diff --git a/server/src/main/java/org/elasticsearch/rest/RestController.java b/server/src/main/java/org/elasticsearch/rest/RestController.java index 65631893cb621..9dcec1d9e61ef 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestController.java +++ b/server/src/main/java/org/elasticsearch/rest/RestController.java @@ -250,9 +250,6 @@ private void dispatchRequest(RestRequest request, RestChannel channel, RestHandl inFlightRequestsBreaker(circuitBreakerService).addWithoutBreaking(contentLength); } // iff we could reserve bytes for the request we need to send the response also over this channel - // Using a version from a handler because if no handler was found for requested version, - // we would return a handler for CURRENT. Therefore no compatible logic in serialisation (toXContent) should be applied - // see MethodHandlers#getHandler responseChannel = new ResourceHandlingHttpChannel(channel, circuitBreakerService, contentLength, handler.compatibleWithVersion()); // TODO: Count requests double in the circuit breaker if they need copying? From c39d0bb6e0b1428736b05c0cde01be02598f2252 Mon Sep 17 00:00:00 2001 From: Przemyslaw Gomulka Date: Tue, 10 Nov 2020 17:44:09 +0100 Subject: [PATCH 20/21] Apply suggestions from code review Co-authored-by: Jay Modi --- .../org/elasticsearch/common/xcontent/XContentBuilder.java | 5 ++++- server/src/main/java/org/elasticsearch/Version.java | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java index f598f81679df5..e5aebb26fff4d 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java @@ -1010,7 +1010,10 @@ public XContentBuilder copyCurrentStructure(XContentParser parser) throws IOExce * Sets a version used for serialising a response compatible with a previous version. */ public XContentBuilder withCompatibleMajorVersion(byte compatibleMajorVersion) { - assert compatibleMajorVersion != 0 : "Compatible version has already been set"; + assert this.compatibleMajorVersion == 0 : "Compatible version has already been set"; + if (compatibleMajorVersion == 0) { + throw new IllegalArgumentException("Compatible major version must not be equal to 0"); + } this.compatibleMajorVersion = compatibleMajorVersion; return this; } diff --git a/server/src/main/java/org/elasticsearch/Version.java b/server/src/main/java/org/elasticsearch/Version.java index adccc5a5cb364..5d880fd05f9fa 100644 --- a/server/src/main/java/org/elasticsearch/Version.java +++ b/server/src/main/java/org/elasticsearch/Version.java @@ -259,7 +259,7 @@ private static Version fromStringSlow(String version) { public final byte build; public final org.apache.lucene.util.Version luceneVersion; private final String toString; - public final int previousMajorId; + private final int previousMajorId; Version(int id, org.apache.lucene.util.Version luceneVersion) { this.id = id; From 023c8d992d6853b2fa4a3eda248654cb82cee701 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Tue, 10 Nov 2020 17:47:12 +0100 Subject: [PATCH 21/21] xcontentbuilder has the version requested by a user --- .../elasticsearch/rest/RestController.java | 8 ++-- .../rest/RestControllerTests.java | 43 ++++++++++++++++++- 2 files changed, 46 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/rest/RestController.java b/server/src/main/java/org/elasticsearch/rest/RestController.java index 9dcec1d9e61ef..10386d19a2030 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestController.java +++ b/server/src/main/java/org/elasticsearch/rest/RestController.java @@ -228,7 +228,8 @@ public void dispatchBadRequest(final RestChannel channel, final ThreadContext th } } - private void dispatchRequest(RestRequest request, RestChannel channel, RestHandler handler) throws Exception { + private void dispatchRequest(RestRequest request, RestChannel channel, RestHandler handler, Version compatibleVersion) + throws Exception { final int contentLength = request.contentLength(); if (contentLength > 0) { final XContentType xContentType = request.getXContentType(); @@ -250,8 +251,7 @@ private void dispatchRequest(RestRequest request, RestChannel channel, RestHandl inFlightRequestsBreaker(circuitBreakerService).addWithoutBreaking(contentLength); } // iff we could reserve bytes for the request we need to send the response also over this channel - responseChannel = new ResourceHandlingHttpChannel(channel, circuitBreakerService, contentLength, - handler.compatibleWithVersion()); + responseChannel = new ResourceHandlingHttpChannel(channel, circuitBreakerService, contentLength, compatibleVersion); // TODO: Count requests double in the circuit breaker if they need copying? if (handler.allowsUnsafeBuffers() == false) { request.ensureSafeBuffers(); @@ -348,7 +348,7 @@ private void tryAllHandlers(final RestRequest request, final RestChannel channel return; } } else { - dispatchRequest(request, channel, handler); + dispatchRequest(request, channel, handler, compatibleVersion); return; } } diff --git a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java index df95f65d92d27..91f229e7b60aa 100644 --- a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java +++ b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java @@ -652,6 +652,7 @@ public void testDispatchCompatibleHandler() { .withHeaders(Map.of("Content-Type", mimeTypeList, "Accept", mimeTypeList)) .build(); AssertingChannel channel = new AssertingChannel(fakeRestRequest, true, RestStatus.OK); + // dispatch to a compatible handler restController.registerHandler(RestRequest.Method.GET, "/foo", new RestHandler() { @Override public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception { @@ -662,7 +663,47 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c @Override public Version compatibleWithVersion() { - return Version.V_7_0_0; + return Version.CURRENT.minimumRestCompatibilityVersion(); + } + }); + + assertFalse(channel.getSendResponseCalled()); + restController.dispatchRequest(fakeRestRequest, channel, new ThreadContext(Settings.EMPTY)); + assertTrue(channel.getSendResponseCalled()); + } + + public void testDispatchCompatibleRequestToNewlyAddedHandler() { + + RestController restController = new RestController(Collections.emptySet(), null, client, circuitBreakerService, usageService, + (a,c,h)->Version.CURRENT.minimumRestCompatibilityVersion());//always return compatible version + + final byte version = Version.CURRENT.minimumRestCompatibilityVersion().major; + + final String mimeType = randomCompatibleMimeType(version); + String content = randomAlphaOfLength((int) Math.round(BREAKER_LIMIT.getBytes() / inFlightRequestsBreaker.getOverhead())); + final List mimeTypeList = Collections.singletonList(mimeType); + FakeRestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY) + .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); + + // dispatch to a CURRENT newly added handler + restController.registerHandler(RestRequest.Method.GET, "/foo", new RestHandler() { + @Override + public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception { + XContentBuilder xContentBuilder = channel.newBuilder(); + // even though the handler is CURRENT, the xContentBuilder has the version requested by a client. + // This allows to implement the compatible logic within the serialisation without introducing V7 (compatible) handler + // when only response shape has changed + assertThat(xContentBuilder.getCompatibleMajorVersion(), equalTo(version)); + channel.sendResponse(new BytesRestResponse(RestStatus.OK, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY)); + } + + @Override + public Version compatibleWithVersion() { + return Version.CURRENT; } });