From 5e4239099c44ff785746ed7ee86b326789910c4d Mon Sep 17 00:00:00 2001 From: pgomulka Date: Wed, 2 Sep 2020 14:03:20 +0200 Subject: [PATCH 01/24] Split responsibility for format parsing --- .../elasticsearch/client/LicenseClient.java | 2 +- .../client/RestHighLevelClient.java | 2 +- .../client/ml/PostDataRequest.java | 2 +- .../client/RequestConvertersTests.java | 2 +- .../common/xcontent/XContentType.java | 4 +- .../rest/AbstractRestChannel.java | 2 +- .../rest/action/cat/RestTable.java | 9 +++- .../common/xcontent/XContentTypeTests.java | 42 +++++++++---------- .../test/rest/ESRestTestCase.java | 8 ++-- .../rest/yaml/ClientYamlTestResponse.java | 2 +- .../test/rest/yaml/ObjectPath.java | 2 +- .../xpack/TimeSeriesRestDriver.java | 2 +- ...stractSearchableSnapshotsRestTestCase.java | 2 +- .../xpack/sql/client/HttpClient.java | 2 +- .../xpack/sql/plugin/RestSqlQueryAction.java | 8 +++- .../watcher/common/http/HttpResponse.java | 2 +- .../watcher/common/text/TextTemplate.java | 2 +- .../common/text/TextTemplateEngine.java | 2 +- .../xpack/restart/FullClusterRestartIT.java | 2 +- .../upgrades/TransformSurvivesUpgradeIT.java | 2 +- 20 files changed, 57 insertions(+), 44 deletions(-) diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/LicenseClient.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/LicenseClient.java index e0db84e6b2bbd..04bfee87d2b50 100644 --- a/client/rest-high-level/src/main/java/org/elasticsearch/client/LicenseClient.java +++ b/client/rest-high-level/src/main/java/org/elasticsearch/client/LicenseClient.java @@ -217,7 +217,7 @@ static String convertResponseToJson(Response response) throws IOException { if (entity.getContentType() == null) { throw new IllegalStateException("Elasticsearch didn't return the [Content-Type] header, unable to parse response body"); } - XContentType xContentType = XContentType.fromMediaTypeOrFormat(entity.getContentType().getValue()); + XContentType xContentType = XContentType.fromMediaType(entity.getContentType().getValue()); if (xContentType == null) { throw new IllegalStateException("Unsupported Content-Type: " + entity.getContentType().getValue()); } diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java index 4d1e9dc96f580..8d1a8e22a4b1c 100644 --- a/client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java +++ b/client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java @@ -1899,7 +1899,7 @@ protected final Resp parseEntity(final HttpEntity entity, if (entity.getContentType() == null) { throw new IllegalStateException("Elasticsearch didn't return the [Content-Type] header, unable to parse response body"); } - XContentType xContentType = XContentType.fromMediaTypeOrFormat(entity.getContentType().getValue()); + XContentType xContentType = XContentType.fromMediaType(entity.getContentType().getValue()); if (xContentType == null) { throw new IllegalStateException("Unsupported Content-Type: " + entity.getContentType().getValue()); } diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/ml/PostDataRequest.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/ml/PostDataRequest.java index e81464bc4282c..7dcf904ca91d4 100644 --- a/client/rest-high-level/src/main/java/org/elasticsearch/client/ml/PostDataRequest.java +++ b/client/rest-high-level/src/main/java/org/elasticsearch/client/ml/PostDataRequest.java @@ -47,7 +47,7 @@ public class PostDataRequest implements Validatable, ToXContentObject { public static final ConstructingObjectParser PARSER = new ConstructingObjectParser<>("post_data_request", - (a) -> new PostDataRequest((String)a[0], XContentType.fromMediaTypeOrFormat((String)a[1]), new byte[0])); + (a) -> new PostDataRequest((String)a[0], XContentType.fromFormat((String)a[1]), new byte[0])); static { PARSER.declareString(ConstructingObjectParser.constructorArg(), Job.ID); diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/RequestConvertersTests.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/RequestConvertersTests.java index 58c6b998f56df..c0b6b93c4544f 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/RequestConvertersTests.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/RequestConvertersTests.java @@ -767,7 +767,7 @@ public void testUpdate() throws IOException { UpdateRequest parsedUpdateRequest = new UpdateRequest(); - XContentType entityContentType = XContentType.fromMediaTypeOrFormat(entity.getContentType().getValue()); + XContentType entityContentType = XContentType.fromMediaType(entity.getContentType().getValue()); try (XContentParser parser = createParser(entityContentType.xContent(), entity.getContent())) { parsedUpdateRequest.fromXContent(parser); } 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 606284f046244..ee1945fcf1027 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 @@ -120,7 +120,9 @@ public XContent xContent() { * also supports a wildcard accept for {@code application/*}. This method can be used to parse the {@code Accept} HTTP header or a * format query string parameter. This method will return {@code null} if no match is found */ - public static XContentType fromMediaTypeOrFormat(String mediaType) { + public static XContentType fromFormat(String mediaType) { +// String mediaType = parseMediaType(mediaTypeHeaderValue); + if (mediaType == null) { return null; } diff --git a/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java b/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java index 467f1d969e8be..e2841c70ea7ca 100644 --- a/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java +++ b/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java @@ -96,7 +96,7 @@ public XContentBuilder newBuilder(@Nullable XContentType requestContentType, boo public XContentBuilder newBuilder(@Nullable XContentType requestContentType, @Nullable XContentType responseContentType, boolean useFiltering) throws IOException { if (responseContentType == null) { - responseContentType = XContentType.fromMediaTypeOrFormat(format); + responseContentType = XContentType.fromFormat(format); } // try to determine the response content type from the media type or the format query string parameter, with the format parameter // taking precedence over the Accept header 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 00e56f3773cca..bb3476d7ca3ef 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,13 +51,20 @@ public class RestTable { public static RestResponse buildResponse(Table table, RestChannel channel) throws Exception { RestRequest request = channel.request(); - XContentType xContentType = XContentType.fromMediaTypeOrFormat(request.param("format", request.header("Accept"))); + XContentType xContentType = getxContentType(request); if (xContentType != null) { return buildXContentBuilder(table, channel); } return buildTextPlainResponse(table, channel); } + private static XContentType getxContentType(RestRequest request) { + if(request.hasParam("format")) { + return XContentType.fromFormat(request.param("format")); + } + return XContentType.fromMediaType(request.header("Accept")); + } + public static RestResponse buildXContentBuilder(Table table, RestChannel channel) throws Exception { RestRequest request = channel.request(); XContentBuilder builder = channel.newBuilder(); 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 47a470e2cea84..012b06ba6f3db 100644 --- a/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java +++ b/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java @@ -29,59 +29,59 @@ public class XContentTypeTests extends ESTestCase { public void testFromJson() throws Exception { String mediaType = "application/json"; XContentType expectedXContentType = XContentType.JSON; - assertThat(XContentType.fromMediaTypeOrFormat(mediaType), equalTo(expectedXContentType)); - assertThat(XContentType.fromMediaTypeOrFormat(mediaType + ";"), equalTo(expectedXContentType)); - assertThat(XContentType.fromMediaTypeOrFormat(mediaType + "; charset=UTF-8"), equalTo(expectedXContentType)); + assertThat(XContentType.fromMediaType(mediaType), equalTo(expectedXContentType)); + assertThat(XContentType.fromMediaType(mediaType + ";"), equalTo(expectedXContentType)); + assertThat(XContentType.fromMediaType(mediaType + "; charset=UTF-8"), equalTo(expectedXContentType)); } public void testFromJsonUppercase() throws Exception { String mediaType = "application/json".toUpperCase(Locale.ROOT); XContentType expectedXContentType = XContentType.JSON; - assertThat(XContentType.fromMediaTypeOrFormat(mediaType), equalTo(expectedXContentType)); - assertThat(XContentType.fromMediaTypeOrFormat(mediaType + ";"), equalTo(expectedXContentType)); - assertThat(XContentType.fromMediaTypeOrFormat(mediaType + "; charset=UTF-8"), equalTo(expectedXContentType)); + assertThat(XContentType.fromMediaType(mediaType), equalTo(expectedXContentType)); + assertThat(XContentType.fromMediaType(mediaType + ";"), equalTo(expectedXContentType)); + assertThat(XContentType.fromMediaType(mediaType + "; charset=UTF-8"), equalTo(expectedXContentType)); } public void testFromYaml() throws Exception { String mediaType = "application/yaml"; XContentType expectedXContentType = XContentType.YAML; - assertThat(XContentType.fromMediaTypeOrFormat(mediaType), equalTo(expectedXContentType)); - assertThat(XContentType.fromMediaTypeOrFormat(mediaType + ";"), equalTo(expectedXContentType)); - assertThat(XContentType.fromMediaTypeOrFormat(mediaType + "; charset=UTF-8"), equalTo(expectedXContentType)); + assertThat(XContentType.fromMediaType(mediaType), equalTo(expectedXContentType)); + assertThat(XContentType.fromMediaType(mediaType + ";"), equalTo(expectedXContentType)); + assertThat(XContentType.fromMediaType(mediaType + "; charset=UTF-8"), equalTo(expectedXContentType)); } public void testFromSmile() throws Exception { String mediaType = "application/smile"; XContentType expectedXContentType = XContentType.SMILE; - assertThat(XContentType.fromMediaTypeOrFormat(mediaType), equalTo(expectedXContentType)); - assertThat(XContentType.fromMediaTypeOrFormat(mediaType + ";"), equalTo(expectedXContentType)); + assertThat(XContentType.fromMediaType(mediaType), equalTo(expectedXContentType)); + assertThat(XContentType.fromMediaType(mediaType + ";"), equalTo(expectedXContentType)); } public void testFromCbor() throws Exception { String mediaType = "application/cbor"; XContentType expectedXContentType = XContentType.CBOR; - assertThat(XContentType.fromMediaTypeOrFormat(mediaType), equalTo(expectedXContentType)); - assertThat(XContentType.fromMediaTypeOrFormat(mediaType + ";"), equalTo(expectedXContentType)); + assertThat(XContentType.fromMediaType(mediaType), equalTo(expectedXContentType)); + assertThat(XContentType.fromMediaType(mediaType + ";"), equalTo(expectedXContentType)); } public void testFromWildcard() throws Exception { String mediaType = "application/*"; XContentType expectedXContentType = XContentType.JSON; - assertThat(XContentType.fromMediaTypeOrFormat(mediaType), equalTo(expectedXContentType)); - assertThat(XContentType.fromMediaTypeOrFormat(mediaType + ";"), equalTo(expectedXContentType)); + assertThat(XContentType.fromMediaType(mediaType), equalTo(expectedXContentType)); + assertThat(XContentType.fromMediaType(mediaType + ";"), equalTo(expectedXContentType)); } public void testFromWildcardUppercase() throws Exception { String mediaType = "APPLICATION/*"; XContentType expectedXContentType = XContentType.JSON; - assertThat(XContentType.fromMediaTypeOrFormat(mediaType), equalTo(expectedXContentType)); - assertThat(XContentType.fromMediaTypeOrFormat(mediaType + ";"), equalTo(expectedXContentType)); + assertThat(XContentType.fromMediaType(mediaType), equalTo(expectedXContentType)); + assertThat(XContentType.fromMediaType(mediaType + ";"), equalTo(expectedXContentType)); } public void testFromRubbish() throws Exception { - assertThat(XContentType.fromMediaTypeOrFormat(null), nullValue()); - assertThat(XContentType.fromMediaTypeOrFormat(""), nullValue()); - assertThat(XContentType.fromMediaTypeOrFormat("text/plain"), nullValue()); - assertThat(XContentType.fromMediaTypeOrFormat("gobbly;goop"), nullValue()); + assertThat(XContentType.fromMediaType(null), nullValue()); + assertThat(XContentType.fromMediaType(""), nullValue()); + assertThat(XContentType.fromMediaType("text/plain"), nullValue()); + assertThat(XContentType.fromMediaType("gobbly;goop"), nullValue()); } } diff --git a/test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java index bea1327427511..962faaa5ff0dd 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java @@ -118,7 +118,7 @@ public abstract class ESRestTestCase extends ESTestCase { * Convert the entity from a {@link Response} into a map of maps. */ public static Map entityAsMap(Response response) throws IOException { - XContentType xContentType = XContentType.fromMediaTypeOrFormat(response.getEntity().getContentType().getValue()); + XContentType xContentType = XContentType.fromMediaType(response.getEntity().getContentType().getValue()); // EMPTY and THROW are fine here because `.map` doesn't use named x content or deprecation try (XContentParser parser = xContentType.xContent().createParser( NamedXContentRegistry.EMPTY, DeprecationHandler.THROW_UNSUPPORTED_OPERATION, @@ -131,7 +131,7 @@ public static Map entityAsMap(Response response) throws IOExcept * Convert the entity from a {@link Response} into a list of maps. */ public static List entityAsList(Response response) throws IOException { - XContentType xContentType = XContentType.fromMediaTypeOrFormat(response.getEntity().getContentType().getValue()); + XContentType xContentType = XContentType.fromMediaType(response.getEntity().getContentType().getValue()); // EMPTY and THROW are fine here because `.map` doesn't use named x content or deprecation try (XContentParser parser = xContentType.xContent().createParser( NamedXContentRegistry.EMPTY, DeprecationHandler.THROW_UNSUPPORTED_OPERATION, @@ -1183,7 +1183,7 @@ protected static Map getAlias(final String index, final String a protected static Map getAsMap(final String endpoint) throws IOException { Response response = client().performRequest(new Request("GET", endpoint)); - XContentType entityContentType = XContentType.fromMediaTypeOrFormat(response.getEntity().getContentType().getValue()); + XContentType entityContentType = XContentType.fromMediaType(response.getEntity().getContentType().getValue()); Map responseEntity = XContentHelper.convertToMap(entityContentType.xContent(), response.getEntity().getContent(), false); assertNotNull(responseEntity); @@ -1411,7 +1411,7 @@ protected static void waitForActiveLicense(final RestClient restClient) throws E assertOK(response); try (InputStream is = response.getEntity().getContent()) { - XContentType xContentType = XContentType.fromMediaTypeOrFormat(response.getEntity().getContentType().getValue()); + XContentType xContentType = XContentType.fromMediaType(response.getEntity().getContentType().getValue()); final Map map = XContentHelper.convertToMap(xContentType.xContent(), is, true); assertThat(map, notNullValue()); assertThat("License must exist", map.containsKey("license"), equalTo(true)); 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 3383d3bb21d04..9f5d0dfe9dcd7 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"); - this.bodyContentType = XContentType.fromMediaTypeOrFormat(contentType); + this.bodyContentType = XContentType.fromMediaType(contentType); try { byte[] bytes = EntityUtils.toByteArray(response.getEntity()); //skip parsing if we got text back (e.g. if we called _cat apis) diff --git a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ObjectPath.java b/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ObjectPath.java index 36d1ff04a5596..a9215caed48bc 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ObjectPath.java +++ b/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ObjectPath.java @@ -44,7 +44,7 @@ public class ObjectPath { public static ObjectPath createFromResponse(Response response) throws IOException { byte[] bytes = EntityUtils.toByteArray(response.getEntity()); String contentType = response.getHeader("Content-Type"); - XContentType xContentType = XContentType.fromMediaTypeOrFormat(contentType); + XContentType xContentType = XContentType.fromMediaType(contentType); return ObjectPath.createFromXContent(xContentType.xContent(), new BytesArray(bytes)); } diff --git a/x-pack/plugin/ilm/qa/multi-node/src/test/java/org/elasticsearch/xpack/TimeSeriesRestDriver.java b/x-pack/plugin/ilm/qa/multi-node/src/test/java/org/elasticsearch/xpack/TimeSeriesRestDriver.java index fe7d2d941baa8..ffccc7d0b07a2 100644 --- a/x-pack/plugin/ilm/qa/multi-node/src/test/java/org/elasticsearch/xpack/TimeSeriesRestDriver.java +++ b/x-pack/plugin/ilm/qa/multi-node/src/test/java/org/elasticsearch/xpack/TimeSeriesRestDriver.java @@ -234,7 +234,7 @@ public static void createIndexWithSettings(RestClient client, String index, Stri @SuppressWarnings("unchecked") public static Integer getNumberOfSegments(RestClient client, String index) throws IOException { Response response = client.performRequest(new Request("GET", index + "/_segments")); - XContentType entityContentType = XContentType.fromMediaTypeOrFormat(response.getEntity().getContentType().getValue()); + XContentType entityContentType = XContentType.fromMediaType(response.getEntity().getContentType().getValue()); Map responseEntity = XContentHelper.convertToMap(entityContentType.xContent(), response.getEntity().getContent(), false); responseEntity = (Map) responseEntity.get("indices"); diff --git a/x-pack/plugin/searchable-snapshots/src/test/java/org/elasticsearch/xpack/searchablesnapshots/AbstractSearchableSnapshotsRestTestCase.java b/x-pack/plugin/searchable-snapshots/src/test/java/org/elasticsearch/xpack/searchablesnapshots/AbstractSearchableSnapshotsRestTestCase.java index 00ac39c0773fe..7ab6d94e53207 100644 --- a/x-pack/plugin/searchable-snapshots/src/test/java/org/elasticsearch/xpack/searchablesnapshots/AbstractSearchableSnapshotsRestTestCase.java +++ b/x-pack/plugin/searchable-snapshots/src/test/java/org/elasticsearch/xpack/searchablesnapshots/AbstractSearchableSnapshotsRestTestCase.java @@ -407,7 +407,7 @@ protected static Map indexSettings(String index) throws IOExcept } protected static Map responseAsMap(Response response) throws IOException { - final XContentType xContentType = XContentType.fromMediaTypeOrFormat(response.getEntity().getContentType().getValue()); + final XContentType xContentType = XContentType.fromMediaType(response.getEntity().getContentType().getValue()); assertThat("Unknown XContentType", xContentType, notNullValue()); BytesReference bytesReference = Streams.readFully(response.getEntity().getContent()); diff --git a/x-pack/plugin/sql/sql-client/src/main/java/org/elasticsearch/xpack/sql/client/HttpClient.java b/x-pack/plugin/sql/sql-client/src/main/java/org/elasticsearch/xpack/sql/client/HttpClient.java index 2b2eee541472f..e3d02a4dea78a 100644 --- a/x-pack/plugin/sql/sql-client/src/main/java/org/elasticsearch/xpack/sql/client/HttpClient.java +++ b/x-pack/plugin/sql/sql-client/src/main/java/org/elasticsearch/xpack/sql/client/HttpClient.java @@ -158,7 +158,7 @@ private byte[] toXContent(Request xContent) { private Tuple readFrom(InputStream inputStream, Function headers) { String contentType = headers.apply("Content-Type"); - XContentType xContentType = XContentType.fromMediaTypeOrFormat(contentType); + XContentType xContentType = XContentType.fromMediaType(contentType); if (xContentType == null) { throw new IllegalStateException("Unsupported Content-Type: " + contentType); } 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 6bef224c11153..45d9111835d58 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 @@ -78,10 +78,14 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli if ("*/*".equals(accept)) { // */* means "I don't care" which we should treat like not specifying the header accept = null; + } else { + XContentType xContentType = XContentType.fromMediaType(accept); + accept = xContentType.shortName(); } } if (accept == null) { - accept = request.header("Content-Type"); + XContentType xContentType = XContentType.fromMediaType(request.header("Content-Type")); + accept = xContentType.shortName(); } assert accept != null : "The Content-Type header is required"; @@ -92,7 +96,7 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli * that doesn't parse it'll throw an {@link IllegalArgumentException} * which we turn into a 400 error. */ - XContentType xContentType = accept == null ? XContentType.JSON : XContentType.fromMediaTypeOrFormat(accept); + XContentType xContentType = accept == null ? XContentType.JSON : XContentType.fromFormat(accept); textFormat = xContentType == null ? TextFormat.fromMediaTypeOrFormat(accept) : null; if (xContentType == null && sqlRequest.columnar()) { 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 5fc43f192b16a..6fc19feb8ffd4 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,7 @@ public XContentType xContentType() { if (values == null || values.length == 0) { return null; } - return XContentType.fromMediaTypeOrFormat(values[0]); + return XContentType.fromMediaType(values[0]); } @Override diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/text/TextTemplate.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/text/TextTemplate.java index a261e6d01af1c..49acf831cad3d 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/text/TextTemplate.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/text/TextTemplate.java @@ -89,7 +89,7 @@ public XContentType getContentType() { return null; } - return XContentType.fromMediaTypeOrFormat(mediaType); + return XContentType.fromFormat(mediaType); } public ScriptType getType() { diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/text/TextTemplateEngine.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/text/TextTemplateEngine.java index fed102f1d5c1f..08d90bfc72926 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/text/TextTemplateEngine.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/text/TextTemplateEngine.java @@ -82,7 +82,7 @@ private XContentType detectContentType(String content) { //There must be a __ Date: Wed, 2 Sep 2020 14:24:16 +0200 Subject: [PATCH 02/24] parse * and ndjson --- .../common/xcontent/XContentType.java | 44 ++++++++++++++++--- 1 file changed, 38 insertions(+), 6 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 ee1945fcf1027..31dd40e1c32d3 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 @@ -26,6 +26,8 @@ import java.util.Locale; import java.util.Objects; +import java.util.regex.Matcher; +import java.util.regex.Pattern; /** * The content type of {@link org.elasticsearch.common.xcontent.XContent}. @@ -113,6 +115,18 @@ public XContent xContent() { return CborXContent.cborXContent; } }; + /** + * A regexp to allow parsing media types. It covers two use cases. + * 1. Media type with a version - requires a custom vnd.elasticserach subtype and a compatible-with parameter + * i.e. application/vnd.elasticsearch+json;compatible-with + * 2. Media type without a version - for users not using compatible API i.e. application/json + */ + private static final Pattern MEDIA_TYPE_PATTERN = Pattern.compile( + //type + "^(application|text)/" + + "([^;\\s]+)" + //subtype: json,yaml,etc some of these are defined in x-pack so can't be enumerated + "(?:\\s*;\\s*(charset=UTF-8)?)?$", + Pattern.CASE_INSENSITIVE); /** * Accepts either a format string, which is equivalent to {@link XContentType#shortName()} or a media type that optionally has @@ -144,16 +158,34 @@ public static XContentType fromFormat(String mediaType) { * The provided media type should not include any parameters. This method is suitable for parsing part of the {@code Content-Type} * HTTP header. This method will return {@code null} if no match is found */ - public static XContentType fromMediaType(String mediaType) { - final String lowercaseMediaType = Objects.requireNonNull(mediaType, "mediaType cannot be null").toLowerCase(Locale.ROOT); + public static XContentType fromMediaType(String mediaTypeHeaderValue) { + if (mediaTypeHeaderValue == null) { + return null; + } + // we also support newline delimited JSON: http://specs.okfnlabs.org/ndjson/ + if (mediaTypeHeaderValue.toLowerCase(Locale.ROOT).equals("application/x-ndjson")) { + return XContentType.JSON; + } + if (mediaTypeHeaderValue.toLowerCase(Locale.ROOT).startsWith("application/*")) { + return JSON; + } + + String mediaType = parseMediaType(mediaTypeHeaderValue); for (XContentType type : values()) { - if (type.mediaTypeWithoutParameters().equals(lowercaseMediaType)) { + if (type.mediaTypeWithoutParameters().equals(mediaType)) { return type; } } - // we also support newline delimited JSON: http://specs.okfnlabs.org/ndjson/ - if (lowercaseMediaType.toLowerCase(Locale.ROOT).equals("application/x-ndjson")) { - return XContentType.JSON; + + return null; + } + + public static String parseMediaType(String mediaType) { + if (mediaType != null) { + Matcher matcher = MEDIA_TYPE_PATTERN.matcher(mediaType); + if (matcher.find()) { + return (matcher.group(1) + "/" + matcher.group(2)).toLowerCase(Locale.ROOT); + } } return null; From 50a88a0d190a882ed3cb389d17a97349de6e540a Mon Sep 17 00:00:00 2001 From: pgomulka Date: Wed, 2 Sep 2020 14:26:40 +0200 Subject: [PATCH 03/24] make format not accepting applicaiton/ --- .../org/elasticsearch/common/xcontent/XContentType.java | 7 +------ 1 file changed, 1 insertion(+), 6 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 31dd40e1c32d3..19a7a934c7df2 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 @@ -135,20 +135,15 @@ public XContent xContent() { * format query string parameter. This method will return {@code null} if no match is found */ public static XContentType fromFormat(String mediaType) { -// String mediaType = parseMediaType(mediaTypeHeaderValue); if (mediaType == null) { return null; } for (XContentType type : values()) { - if (isSameMediaTypeOrFormatAs(mediaType, type)) { + if (type.shortName().equalsIgnoreCase(mediaType)) { return type; } } - final String lowercaseMediaType = mediaType.toLowerCase(Locale.ROOT); - if (lowercaseMediaType.startsWith("application/*")) { - return JSON; - } return null; } From fb03ffd4551e01e2b86b7ec1a8bdff4610eaccfd Mon Sep 17 00:00:00 2001 From: pgomulka Date: Wed, 2 Sep 2020 14:35:26 +0200 Subject: [PATCH 04/24] post data request should parse applicaiton/json style --- .../main/java/org/elasticsearch/client/ml/PostDataRequest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/ml/PostDataRequest.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/ml/PostDataRequest.java index 7dcf904ca91d4..0d3c32ba12477 100644 --- a/client/rest-high-level/src/main/java/org/elasticsearch/client/ml/PostDataRequest.java +++ b/client/rest-high-level/src/main/java/org/elasticsearch/client/ml/PostDataRequest.java @@ -47,7 +47,7 @@ public class PostDataRequest implements Validatable, ToXContentObject { public static final ConstructingObjectParser PARSER = new ConstructingObjectParser<>("post_data_request", - (a) -> new PostDataRequest((String)a[0], XContentType.fromFormat((String)a[1]), new byte[0])); + (a) -> new PostDataRequest((String)a[0], XContentType.fromMediaType((String)a[1]), new byte[0])); static { PARSER.declareString(ConstructingObjectParser.constructorArg(), Job.ID); From 09f281ecf956a7dc76c72daedc18dbb097709d02 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Wed, 2 Sep 2020 16:34:45 +0200 Subject: [PATCH 05/24] unused import --- .../java/org/elasticsearch/common/xcontent/XContentType.java | 1 - 1 file changed, 1 deletion(-) 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 19a7a934c7df2..840a6a0f42f48 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 @@ -25,7 +25,6 @@ import org.elasticsearch.common.xcontent.yaml.YamlXContent; import java.util.Locale; -import java.util.Objects; import java.util.regex.Matcher; import java.util.regex.Pattern; From 8bc024f5af943b2e38dce377dae21c8db34115aa Mon Sep 17 00:00:00 2001 From: pgomulka Date: Wed, 2 Sep 2020 17:11:32 +0200 Subject: [PATCH 06/24] fix sql parsing --- .../common/xcontent/XContentType.java | 2 +- .../xpack/sql/plugin/RestSqlQueryAction.java | 22 ++++++++++++++----- 2 files changed, 18 insertions(+), 6 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 840a6a0f42f48..b1c10f88317ed 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 @@ -124,7 +124,7 @@ public XContent xContent() { //type "^(application|text)/" + "([^;\\s]+)" + //subtype: json,yaml,etc some of these are defined in x-pack so can't be enumerated - "(?:\\s*;\\s*(charset=UTF-8)?)?$", + "(?:\\s*;.*)?$", Pattern.CASE_INSENSITIVE); /** 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 45d9111835d58..c5c9f351914d5 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 @@ -65,6 +65,8 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli * isn't then we use the {@code Content-Type} header which is required. */ String accept = null; + XContentType acceptHeader = null; + XContentType contentTypeHeader = null; if (Mode.isDedicatedClient(sqlRequest.requestInfo().mode()) && (sqlRequest.binaryCommunication() == null || sqlRequest.binaryCommunication())) { @@ -79,13 +81,12 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli // */* means "I don't care" which we should treat like not specifying the header accept = null; } else { - XContentType xContentType = XContentType.fromMediaType(accept); - accept = xContentType.shortName(); + acceptHeader = XContentType.fromMediaType(accept); } } if (accept == null) { - XContentType xContentType = XContentType.fromMediaType(request.header("Content-Type")); - accept = xContentType.shortName(); + accept = request.header("Content-Type"); + contentTypeHeader = XContentType.fromMediaType(accept); } assert accept != null : "The Content-Type header is required"; @@ -96,7 +97,7 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli * that doesn't parse it'll throw an {@link IllegalArgumentException} * which we turn into a 400 error. */ - XContentType xContentType = accept == null ? XContentType.JSON : XContentType.fromFormat(accept); + XContentType xContentType = getXContentType(accept, acceptHeader, contentTypeHeader); textFormat = xContentType == null ? TextFormat.fromMediaTypeOrFormat(accept) : null; if (xContentType == null && sqlRequest.columnar()) { @@ -134,6 +135,17 @@ public RestResponse buildResponse(SqlQueryResponse response) throws Exception { }); } + private XContentType getXContentType(String accept, XContentType acceptHeader, XContentType contentTypeHeader) { + if (accept == null) { + return XContentType.JSON; + } + XContentType xContentType = XContentType.fromFormat(accept); + if (xContentType != null) { + return xContentType; + } + return acceptHeader != null ? acceptHeader : contentTypeHeader; + } + @Override protected Set responseParams() { return textFormat == TextFormat.CSV ? Collections.singleton(URL_PARAM_DELIMITER) : Collections.emptySet(); From 070508c2a2adc3e1197377f6747de18fff3c4b88 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Thu, 3 Sep 2020 15:32:00 +0200 Subject: [PATCH 07/24] split format and accept header --- .../java/org/elasticsearch/rest/AbstractRestChannel.java | 7 ++++++- .../xpack/watcher/common/text/TextTemplate.java | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java b/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java index e2841c70ea7ca..96d6107f3ea76 100644 --- a/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java +++ b/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java @@ -45,6 +45,7 @@ public abstract class AbstractRestChannel implements RestChannel { private final String filterPath; private final boolean pretty; private final boolean human; + private final String acceptHeader; private BytesStreamOutput bytesOut; @@ -58,7 +59,8 @@ public abstract class AbstractRestChannel implements RestChannel { protected AbstractRestChannel(RestRequest request, boolean detailedErrorsEnabled) { this.request = request; this.detailedErrorsEnabled = detailedErrorsEnabled; - this.format = request.param("format", request.header("Accept")); + this.format = request.param("format"); + this.acceptHeader = request.header("Accept"); this.filterPath = request.param("filter_path", null); this.pretty = request.paramAsBoolean("pretty", false); this.human = request.paramAsBoolean("human", false); @@ -97,6 +99,9 @@ public XContentBuilder newBuilder(@Nullable XContentType requestContentType, @Nu boolean useFiltering) throws IOException { if (responseContentType == null) { responseContentType = XContentType.fromFormat(format); + if(responseContentType == null) { + responseContentType = XContentType.fromMediaType(acceptHeader); + } } // try to determine the response content type from the media type or the format query string parameter, with the format parameter // taking precedence over the Accept header diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/text/TextTemplate.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/text/TextTemplate.java index 49acf831cad3d..b082ab57f4410 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/text/TextTemplate.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/text/TextTemplate.java @@ -89,7 +89,7 @@ public XContentType getContentType() { return null; } - return XContentType.fromFormat(mediaType); + return XContentType.fromMediaType(mediaType); } public ScriptType getType() { From 3c7ab16fc839d7ade510e8025ad39f106d12752f Mon Sep 17 00:00:00 2001 From: pgomulka Date: Thu, 3 Sep 2020 17:29:51 +0200 Subject: [PATCH 08/24] fix and todos --- .../main/java/org/elasticsearch/rest/AbstractRestChannel.java | 1 + .../java/org/elasticsearch/rest/BytesRestResponseTests.java | 2 +- .../org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java | 1 + .../xpack/watcher/common/text/TextTemplateEngine.java | 1 + .../org/elasticsearch/xpack/restart/FullClusterRestartIT.java | 2 +- .../org/elasticsearch/upgrades/TransformSurvivesUpgradeIT.java | 2 +- 6 files changed, 6 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java b/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java index 96d6107f3ea76..a9f28ae286982 100644 --- a/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java +++ b/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java @@ -98,6 +98,7 @@ public XContentBuilder newBuilder(@Nullable XContentType requestContentType, boo public XContentBuilder newBuilder(@Nullable XContentType requestContentType, @Nullable XContentType responseContentType, boolean useFiltering) throws IOException { if (responseContentType == null) { + //TODO PG shoudld format vs acceptHeader be always the same, do we allow overriding? responseContentType = XContentType.fromFormat(format); if(responseContentType == null) { responseContentType = XContentType.fromMediaType(acceptHeader); diff --git a/server/src/test/java/org/elasticsearch/rest/BytesRestResponseTests.java b/server/src/test/java/org/elasticsearch/rest/BytesRestResponseTests.java index a1fd55e55d718..945c29684f712 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.mediaType()); + Map params = Collections.singletonMap("format", xContentType.shortName()); RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withParams(params).build(); RestChannel channel = detailed ? new DetailedExceptionRestChannel(request) : new SimpleExceptionRestChannel(request); 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 c5c9f351914d5..39f371b66c437 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 @@ -97,6 +97,7 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli * that doesn't parse it'll throw an {@link IllegalArgumentException} * which we turn into a 400 error. */ + //TODO PG this all logic needs a review from SQL team XContentType xContentType = getXContentType(accept, acceptHeader, contentTypeHeader); textFormat = xContentType == null ? TextFormat.fromMediaTypeOrFormat(accept) : null; diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/text/TextTemplateEngine.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/text/TextTemplateEngine.java index 08d90bfc72926..c6332bb196086 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/text/TextTemplateEngine.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/text/TextTemplateEngine.java @@ -82,6 +82,7 @@ private XContentType detectContentType(String content) { //There must be a __ Date: Fri, 4 Sep 2020 15:04:22 +0200 Subject: [PATCH 09/24] media type parser --- .../common/xcontent/MediaType.java | 32 +++++ .../common/xcontent/MediaTypeParser.java | 114 ++++++++++++++++++ .../common/xcontent/XContentType.java | 87 ++++--------- .../common/xcontent/MediaTypeParserTests.java | 43 +++++++ .../common/xcontent/XContentTypeTests.java | 9 ++ .../xpack/sql/plugin/TextFormat.java | 38 ++++-- 6 files changed, 251 insertions(+), 72 deletions(-) create mode 100644 libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaType.java create mode 100644 libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeParser.java create mode 100644 libs/x-content/src/test/java/org/elasticsearch/common/xcontent/MediaTypeParserTests.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 new file mode 100644 index 0000000000000..ef89a6e97a161 --- /dev/null +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaType.java @@ -0,0 +1,32 @@ +/* + * 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.Map; + +public interface MediaType { + String type(); + String subtype(); + String format(); + + default String typeSubtype(){ + return type()+"/"+subtype(); + } +} 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 new file mode 100644 index 0000000000000..9c7d90f73ef74 --- /dev/null +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeParser.java @@ -0,0 +1,114 @@ +/* + * 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.Objects; + +public class XContentTypeParser { + private Map formatToXContentType = new HashMap<>(); + private Map typeSubtypeToMediaType = new HashMap<>(); + + public XContentTypeParser(T[] acceptedXContentTypes) { + for (T xContentType : acceptedXContentTypes) { + typeSubtypeToMediaType.put(xContentType.typeSubtype(), xContentType); + formatToXContentType.put(xContentType.format(), xContentType); + } + } + + public T fromMediaType(String contentTypeHeader) { + ParsedMediaType parsedMediaType = parseMediaType(contentTypeHeader); + return parsedMediaType != null ? parsedMediaType.getMediaType() : null; + } + + public T fromFormat(String mediaType) { + return formatToXContentType.get(mediaType.toLowerCase(Locale.ROOT)); + } + + public XContentTypeParser withAdditionalMediaType(String typeSubtype, T xContentType) { + typeSubtypeToMediaType.put(typeSubtype.toLowerCase(Locale.ROOT), xContentType); + formatToXContentType.put(xContentType.format(), xContentType); + return this; + } + + public ParsedMediaType parseMediaType(String mediaType) { + if (mediaType != null) { + String headerValue = mediaType.toLowerCase(Locale.ROOT); + // split string on semicolon + // validate media type is accepted (IIRC whitespace is ok so trim it) //TODO PG whitespace only ok in params + // rest of strings are params. validate per RFC 7230 and use ones that we care about + // or use a regex and we can change if necessary + String[] split = headerValue.split(";"); + + String[] typeSubtype = split[0].toLowerCase(Locale.ROOT) + .split("/"); + if (typeSubtype.length == 2) { + String type = typeSubtype[0]; + String subtype = typeSubtype[1]; + T xContentType = typeSubtypeToMediaType.get(type + "/" + subtype); + if (xContentType != null) { + Map parameters = new HashMap<>(); + for (int i = 1; i < split.length; i++) { + String[] keyValueParam = split[i].trim().split("="); + parameters.put(keyValueParam[0].toLowerCase(Locale.ROOT), keyValueParam[1].toLowerCase(Locale.ROOT)); + } + return new ParsedMediaType(type, subtype, parameters, xContentType); + } + } + + } + return null; + } + + public class ParsedMediaType { + private final String type; + private final String subtype; + private final Map parameters; + private final T xContentType; + + public ParsedMediaType(String type, String subtype, Map parameters, T xContentType) { + this.type = type; + this.subtype = subtype; + this.parameters = parameters; + this.xContentType = xContentType; + } + + public T getMediaType() { + return xContentType; + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + ParsedMediaType parsedMediaType = (ParsedMediaType) o; + return Objects.equals(type, parsedMediaType.type) && + Objects.equals(subtype, parsedMediaType.subtype) && + Objects.equals(parameters, parsedMediaType.parameters) && + xContentType == parsedMediaType.xContentType; + } + + public Map getParameters() { + return parameters; + } + } +} 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 b1c10f88317ed..b1428f53f6b3d 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,14 +24,10 @@ import org.elasticsearch.common.xcontent.smile.SmileXContent; import org.elasticsearch.common.xcontent.yaml.YamlXContent; -import java.util.Locale; -import java.util.regex.Matcher; -import java.util.regex.Pattern; - /** * The content type of {@link org.elasticsearch.common.xcontent.XContent}. */ -public enum XContentType { +public enum XContentType implements MediaType { /** * A JSON based content type. @@ -114,18 +110,12 @@ public XContent xContent() { return CborXContent.cborXContent; } }; - /** - * A regexp to allow parsing media types. It covers two use cases. - * 1. Media type with a version - requires a custom vnd.elasticserach subtype and a compatible-with parameter - * i.e. application/vnd.elasticsearch+json;compatible-with - * 2. Media type without a version - for users not using compatible API i.e. application/json - */ - private static final Pattern MEDIA_TYPE_PATTERN = Pattern.compile( - //type - "^(application|text)/" + - "([^;\\s]+)" + //subtype: json,yaml,etc some of these are defined in x-pack so can't be enumerated - "(?:\\s*;.*)?$", - Pattern.CASE_INSENSITIVE); + + public static XContentTypeParser xContentTypeParser = new XContentTypeParser(XContentType.values()) + .withAdditionalMediaType("application/*", JSON) + .withAdditionalMediaType("application/x-ndjson", JSON); + + /** * Accepts either a format string, which is equivalent to {@link XContentType#shortName()} or a media type that optionally has @@ -134,17 +124,7 @@ public XContent xContent() { * format query string parameter. This method will return {@code null} if no match is found */ public static XContentType fromFormat(String mediaType) { - - if (mediaType == null) { - return null; - } - for (XContentType type : values()) { - if (type.shortName().equalsIgnoreCase(mediaType)) { - return type; - } - } - - return null; + return (XContentType) xContentTypeParser.fromFormat(mediaType); } /** @@ -153,43 +133,9 @@ public static XContentType fromFormat(String mediaType) { * HTTP header. This method will return {@code null} if no match is found */ public static XContentType fromMediaType(String mediaTypeHeaderValue) { - if (mediaTypeHeaderValue == null) { - return null; - } - // we also support newline delimited JSON: http://specs.okfnlabs.org/ndjson/ - if (mediaTypeHeaderValue.toLowerCase(Locale.ROOT).equals("application/x-ndjson")) { - return XContentType.JSON; - } - if (mediaTypeHeaderValue.toLowerCase(Locale.ROOT).startsWith("application/*")) { - return JSON; - } - - String mediaType = parseMediaType(mediaTypeHeaderValue); - for (XContentType type : values()) { - if (type.mediaTypeWithoutParameters().equals(mediaType)) { - return type; - } - } - - return null; + return (XContentType) xContentTypeParser.fromMediaType(mediaTypeHeaderValue); } - public static String parseMediaType(String mediaType) { - if (mediaType != null) { - Matcher matcher = MEDIA_TYPE_PATTERN.matcher(mediaType); - if (matcher.find()) { - return (matcher.group(1) + "/" + matcher.group(2)).toLowerCase(Locale.ROOT); - } - } - - return null; - } - - private static boolean isSameMediaTypeOrFormatAs(String stringType, XContentType type) { - return type.mediaTypeWithoutParameters().equalsIgnoreCase(stringType) || - stringType.toLowerCase(Locale.ROOT).startsWith(type.mediaTypeWithoutParameters().toLowerCase(Locale.ROOT) + ";") || - type.shortName().equalsIgnoreCase(stringType); - } private int index; @@ -211,4 +157,19 @@ public String mediaType() { public abstract String mediaTypeWithoutParameters(); + + @Override + public String type() { + return "application"; + } + + @Override + public String subtype() { + return shortName(); + } + + @Override + public String format() { + return shortName(); + } } 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 new file mode 100644 index 0000000000000..7d9f8011bf346 --- /dev/null +++ b/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/MediaTypeParserTests.java @@ -0,0 +1,43 @@ +/* + * 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 XContentTypeParserTests extends ESTestCase { + XContentTypeParser xContentTypeParser = XContentType.xContentTypeParser; + + public void testJsonWithParameters() throws Exception { + String mediaType = "application/json"; + assertThat(xContentTypeParser.parseMediaType(mediaType).getParameters(), + equalTo(Collections.emptyMap())); + assertThat(xContentTypeParser.parseMediaType(mediaType + ";").getParameters(), + equalTo(Collections.emptyMap())); + assertThat(xContentTypeParser.parseMediaType(mediaType + "; charset=UTF-8").getParameters(), + equalTo(Map.of("charset", "utf-8"))); + assertThat(xContentTypeParser.parseMediaType(mediaType + "; custom=123;charset=UTF-8").getParameters(), + equalTo(Map.of("charset", "utf-8", "custom", "123"))); + } +} 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 012b06ba6f3db..d6f9fb5e9ad5e 100644 --- a/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java +++ b/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java @@ -26,6 +26,7 @@ import static org.hamcrest.Matchers.nullValue; public class XContentTypeTests extends ESTestCase { + public void testFromJson() throws Exception { String mediaType = "application/json"; XContentType expectedXContentType = XContentType.JSON; @@ -34,6 +35,14 @@ public void testFromJson() throws Exception { assertThat(XContentType.fromMediaType(mediaType + "; charset=UTF-8"), equalTo(expectedXContentType)); } + public void testFromNdJson() throws Exception { + String mediaType = "application/x-ndjson"; + XContentType expectedXContentType = XContentType.JSON; + assertThat(XContentType.fromMediaType(mediaType), equalTo(expectedXContentType)); + assertThat(XContentType.fromMediaType(mediaType + ";"), equalTo(expectedXContentType)); + assertThat(XContentType.fromMediaType(mediaType + "; charset=UTF-8"), equalTo(expectedXContentType)); + } + public void testFromJsonUppercase() throws Exception { String mediaType = "application/json".toUpperCase(Locale.ROOT); XContentType expectedXContentType = XContentType.JSON; 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 3037a47bcd072..bcddfacdcb4ac 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 @@ -7,6 +7,8 @@ import org.elasticsearch.common.Strings; import org.elasticsearch.common.collect.Tuple; +import org.elasticsearch.common.xcontent.MediaType; +import org.elasticsearch.common.xcontent.XContentTypeParser; import org.elasticsearch.rest.RestRequest; import org.elasticsearch.xpack.ql.util.StringUtils; import org.elasticsearch.xpack.sql.SqlIllegalArgumentException; @@ -32,7 +34,7 @@ /** * Templating class for displaying SQL responses in text formats. */ -enum TextFormat { +enum TextFormat implements MediaType { /** * Default text writer. @@ -197,6 +199,7 @@ String maybeEscape(String value, Character delimiter) { boolean hasHeader(RestRequest request) { String header = request.param(URL_PARAM_HEADER); if (header == null) { + //TODO PG in most places we only assume one accept header List values = request.getAllHeaderValues("Accept"); if (values != null) { // header values are separated by `;` so try breaking it down @@ -275,6 +278,8 @@ String maybeEscape(String value, Character __) { private static final String PARAM_HEADER_ABSENT = "absent"; private static final String PARAM_HEADER_PRESENT = "present"; + private static final XContentTypeParser parser = new XContentTypeParser<>(TextFormat.values()); + String format(RestRequest request, SqlQueryResponse response) { StringBuilder sb = new StringBuilder(); @@ -296,15 +301,14 @@ boolean hasHeader(RestRequest request) { } static TextFormat fromMediaTypeOrFormat(String accept) { - for (TextFormat text : values()) { - String contentType = text.contentType(); - if (contentType.equalsIgnoreCase(accept) - || accept.toLowerCase(Locale.ROOT).startsWith(contentType + ";") - || text.shortName().equalsIgnoreCase(accept)) { - return text; - } + TextFormat textFormat = parser.fromFormat(accept); + if (textFormat != null) { + return textFormat; + } + textFormat = parser.fromMediaType(accept); + if (textFormat != null) { + return textFormat; } - throw new IllegalArgumentException("invalid format [" + accept + "]"); } @@ -360,4 +364,20 @@ protected Character delimiter(RestRequest request) { String maybeEscape(String value, Character delimiter) { return value; } + + + @Override + public String type() { + return "text"; + } + + @Override + public String subtype() { + return shortName(); + } + + @Override + public String format() { + return shortName(); + } } From 46f8f33af567e170cf235f12f246b689dbf687d9 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Fri, 4 Sep 2020 15:12:39 +0200 Subject: [PATCH 10/24] media type parser --- .../common/xcontent/MediaTypeParser.java | 6 +++--- .../elasticsearch/common/xcontent/XContentType.java | 6 +++--- .../common/xcontent/MediaTypeParserTests.java | 12 ++++++------ .../elasticsearch/xpack/sql/plugin/TextFormat.java | 4 ++-- 4 files changed, 14 insertions(+), 14 deletions(-) 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 index 9c7d90f73ef74..729208d6e8bf1 100644 --- 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 @@ -24,11 +24,11 @@ import java.util.Map; import java.util.Objects; -public class XContentTypeParser { +public class MediaTypeParser { private Map formatToXContentType = new HashMap<>(); private Map typeSubtypeToMediaType = new HashMap<>(); - public XContentTypeParser(T[] acceptedXContentTypes) { + public MediaTypeParser(T[] acceptedXContentTypes) { for (T xContentType : acceptedXContentTypes) { typeSubtypeToMediaType.put(xContentType.typeSubtype(), xContentType); formatToXContentType.put(xContentType.format(), xContentType); @@ -44,7 +44,7 @@ public T fromFormat(String mediaType) { return formatToXContentType.get(mediaType.toLowerCase(Locale.ROOT)); } - public XContentTypeParser withAdditionalMediaType(String typeSubtype, T xContentType) { + public MediaTypeParser withAdditionalMediaType(String typeSubtype, T xContentType) { typeSubtypeToMediaType.put(typeSubtype.toLowerCase(Locale.ROOT), xContentType); formatToXContentType.put(xContentType.format(), xContentType); 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 b1428f53f6b3d..a8aa97f1a14d0 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 @@ -111,7 +111,7 @@ public XContent xContent() { } }; - public static XContentTypeParser xContentTypeParser = new XContentTypeParser(XContentType.values()) + public static MediaTypeParser mediaTypeParser = new MediaTypeParser(XContentType.values()) .withAdditionalMediaType("application/*", JSON) .withAdditionalMediaType("application/x-ndjson", JSON); @@ -124,7 +124,7 @@ public XContent xContent() { * format query string parameter. This method will return {@code null} if no match is found */ public static XContentType fromFormat(String mediaType) { - return (XContentType) xContentTypeParser.fromFormat(mediaType); + return (XContentType) mediaTypeParser.fromFormat(mediaType); } /** @@ -133,7 +133,7 @@ public static XContentType fromFormat(String mediaType) { * HTTP header. This method will return {@code null} if no match is found */ public static XContentType fromMediaType(String mediaTypeHeaderValue) { - return (XContentType) xContentTypeParser.fromMediaType(mediaTypeHeaderValue); + return (XContentType) mediaTypeParser.fromMediaType(mediaTypeHeaderValue); } 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 index 7d9f8011bf346..5e4044628b6b3 100644 --- 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 @@ -26,18 +26,18 @@ import static org.hamcrest.Matchers.equalTo; -public class XContentTypeParserTests extends ESTestCase { - XContentTypeParser xContentTypeParser = XContentType.xContentTypeParser; +public class MediaTypeParserTests extends ESTestCase { + MediaTypeParser mediaTypeParser = XContentType.mediaTypeParser; public void testJsonWithParameters() throws Exception { String mediaType = "application/json"; - assertThat(xContentTypeParser.parseMediaType(mediaType).getParameters(), + assertThat(mediaTypeParser.parseMediaType(mediaType).getParameters(), equalTo(Collections.emptyMap())); - assertThat(xContentTypeParser.parseMediaType(mediaType + ";").getParameters(), + assertThat(mediaTypeParser.parseMediaType(mediaType + ";").getParameters(), equalTo(Collections.emptyMap())); - assertThat(xContentTypeParser.parseMediaType(mediaType + "; charset=UTF-8").getParameters(), + assertThat(mediaTypeParser.parseMediaType(mediaType + "; charset=UTF-8").getParameters(), equalTo(Map.of("charset", "utf-8"))); - assertThat(xContentTypeParser.parseMediaType(mediaType + "; custom=123;charset=UTF-8").getParameters(), + assertThat(mediaTypeParser.parseMediaType(mediaType + "; custom=123;charset=UTF-8").getParameters(), equalTo(Map.of("charset", "utf-8", "custom", "123"))); } } 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 bcddfacdcb4ac..3f29d4c43bba7 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 @@ -8,7 +8,7 @@ import org.elasticsearch.common.Strings; import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.xcontent.MediaType; -import org.elasticsearch.common.xcontent.XContentTypeParser; +import org.elasticsearch.common.xcontent.MediaTypeParser; import org.elasticsearch.rest.RestRequest; import org.elasticsearch.xpack.ql.util.StringUtils; import org.elasticsearch.xpack.sql.SqlIllegalArgumentException; @@ -278,7 +278,7 @@ String maybeEscape(String value, Character __) { private static final String PARAM_HEADER_ABSENT = "absent"; private static final String PARAM_HEADER_PRESENT = "present"; - private static final XContentTypeParser parser = new XContentTypeParser<>(TextFormat.values()); + private static final MediaTypeParser parser = new MediaTypeParser<>(TextFormat.values()); String format(RestRequest request, SqlQueryResponse response) { StringBuilder sb = new StringBuilder(); From cbbe0933024511f2e5fd9686df8f95df4d86be08 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Fri, 4 Sep 2020 16:33:00 +0200 Subject: [PATCH 11/24] precommit --- .../elasticsearch/common/xcontent/MediaType.java | 2 -- .../common/xcontent/MediaTypeParser.java | 14 +------------- .../common/xcontent/XContentType.java | 6 +++--- .../common/xcontent/MediaTypeParserTests.java | 2 +- 4 files changed, 5 insertions(+), 19 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 ef89a6e97a161..635375fb84368 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,8 +19,6 @@ package org.elasticsearch.common.xcontent; -import java.util.Map; - public interface MediaType { String type(); String subtype(); 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 index 729208d6e8bf1..696dc8b0b1548 100644 --- 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 @@ -22,7 +22,6 @@ import java.util.HashMap; import java.util.Locale; import java.util.Map; -import java.util.Objects; public class MediaTypeParser { private Map formatToXContentType = new HashMap<>(); @@ -44,7 +43,7 @@ public T fromFormat(String mediaType) { return formatToXContentType.get(mediaType.toLowerCase(Locale.ROOT)); } - public MediaTypeParser withAdditionalMediaType(String typeSubtype, T xContentType) { + public MediaTypeParser withAdditionalMediaType(String typeSubtype, T xContentType) { typeSubtypeToMediaType.put(typeSubtype.toLowerCase(Locale.ROOT), xContentType); formatToXContentType.put(xContentType.format(), xContentType); return this; @@ -96,17 +95,6 @@ public T getMediaType() { return xContentType; } - @Override - public boolean equals(Object o) { - if (this == o) return true; - if (o == null || getClass() != o.getClass()) return false; - ParsedMediaType parsedMediaType = (ParsedMediaType) o; - return Objects.equals(type, parsedMediaType.type) && - Objects.equals(subtype, parsedMediaType.subtype) && - Objects.equals(parameters, parsedMediaType.parameters) && - xContentType == parsedMediaType.xContentType; - } - public Map getParameters() { return parameters; } 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 a8aa97f1a14d0..3e004f88d806e 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 @@ -111,7 +111,7 @@ public XContent xContent() { } }; - public static MediaTypeParser mediaTypeParser = new MediaTypeParser(XContentType.values()) + public static MediaTypeParser mediaTypeParser = new MediaTypeParser<>(XContentType.values()) .withAdditionalMediaType("application/*", JSON) .withAdditionalMediaType("application/x-ndjson", JSON); @@ -124,7 +124,7 @@ public XContent xContent() { * format query string parameter. This method will return {@code null} if no match is found */ public static XContentType fromFormat(String mediaType) { - return (XContentType) mediaTypeParser.fromFormat(mediaType); + return mediaTypeParser.fromFormat(mediaType); } /** @@ -133,7 +133,7 @@ public static XContentType fromFormat(String mediaType) { * HTTP header. This method will return {@code null} if no match is found */ public static XContentType fromMediaType(String mediaTypeHeaderValue) { - return (XContentType) mediaTypeParser.fromMediaType(mediaTypeHeaderValue); + return mediaTypeParser.fromMediaType(mediaTypeHeaderValue); } 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 index 5e4044628b6b3..427f6d1b46358 100644 --- 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 @@ -27,7 +27,7 @@ import static org.hamcrest.Matchers.equalTo; public class MediaTypeParserTests extends ESTestCase { - MediaTypeParser mediaTypeParser = XContentType.mediaTypeParser; + MediaTypeParser mediaTypeParser = XContentType.mediaTypeParser; public void testJsonWithParameters() throws Exception { String mediaType = "application/json"; From 222caeea550d1d0340e717546cc280b64c5d1c58 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Mon, 7 Sep 2020 10:21:21 +0200 Subject: [PATCH 12/24] rename and null check --- .../common/xcontent/MediaTypeParser.java | 35 +++++++++++-------- 1 file changed, 21 insertions(+), 14 deletions(-) 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 index 696dc8b0b1548..33602bee43bcc 100644 --- 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 @@ -24,28 +24,31 @@ import java.util.Map; public class MediaTypeParser { - private Map formatToXContentType = new HashMap<>(); + private Map formatToMediaType = new HashMap<>(); private Map typeSubtypeToMediaType = new HashMap<>(); - public MediaTypeParser(T[] acceptedXContentTypes) { - for (T xContentType : acceptedXContentTypes) { - typeSubtypeToMediaType.put(xContentType.typeSubtype(), xContentType); - formatToXContentType.put(xContentType.format(), xContentType); + public MediaTypeParser(T[] acceptedMediaTypes) { + for (T mediaType : acceptedMediaTypes) { + typeSubtypeToMediaType.put(mediaType.typeSubtype(), mediaType); + formatToMediaType.put(mediaType.format(), mediaType); } } - public T fromMediaType(String contentTypeHeader) { - ParsedMediaType parsedMediaType = parseMediaType(contentTypeHeader); + public T fromMediaType(String mediaType) { + ParsedMediaType parsedMediaType = parseMediaType(mediaType); return parsedMediaType != null ? parsedMediaType.getMediaType() : null; } - public T fromFormat(String mediaType) { - return formatToXContentType.get(mediaType.toLowerCase(Locale.ROOT)); + public T fromFormat(String format) { + if(format == null) { + return null; + } + return formatToMediaType.get(format.toLowerCase(Locale.ROOT)); } public MediaTypeParser withAdditionalMediaType(String typeSubtype, T xContentType) { typeSubtypeToMediaType.put(typeSubtype.toLowerCase(Locale.ROOT), xContentType); - formatToXContentType.put(xContentType.format(), xContentType); + formatToMediaType.put(xContentType.format(), xContentType); return this; } @@ -78,21 +81,25 @@ public ParsedMediaType parseMediaType(String mediaType) { return null; } + /** + * A media type object that contains all the information provided on a Content-Type or Accept header + * // TODO PG to be extended with getCompatibleAPIVersion and more + */ public class ParsedMediaType { private final String type; private final String subtype; private final Map parameters; - private final T xContentType; + private final T mediaType; - public ParsedMediaType(String type, String subtype, Map parameters, T xContentType) { + public ParsedMediaType(String type, String subtype, Map parameters, T mediaType) { this.type = type; this.subtype = subtype; this.parameters = parameters; - this.xContentType = xContentType; + this.mediaType = mediaType; } public T getMediaType() { - return xContentType; + return mediaType; } public Map getParameters() { From 7f52e11aae3df3fe785f30d5c4668cae5b663058 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Mon, 7 Sep 2020 12:52:30 +0200 Subject: [PATCH 13/24] fix text format parsing --- .../xpack/sql/plugin/TextFormat.java | 21 ++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) 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 3f29d4c43bba7..e65577affb834 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 @@ -102,6 +102,11 @@ protected Character delimiter() { protected String eol() { throw new UnsupportedOperationException(); } + + @Override + public String subtype() { + return "plain"; + } }, /** @@ -217,6 +222,11 @@ boolean hasHeader(RestRequest request) { return !header.toLowerCase(Locale.ROOT).equals(PARAM_HEADER_ABSENT); } } + + @Override + public String subtype() { + return "csv"; + } }, TSV() { @@ -266,6 +276,11 @@ String maybeEscape(String value, Character __) { return sb.toString(); } + + @Override + public String subtype() { + return "tab-separated-values"; + } }; private static final String FORMAT_TEXT = "txt"; @@ -372,12 +387,12 @@ public String type() { } @Override - public String subtype() { + public String format() { return shortName(); } @Override - public String format() { - return shortName(); + public String typeSubtype() { + return contentType(); } } From 57ddb407557e4cf5b97a8a030bf97da7d836c728 Mon Sep 17 00:00:00 2001 From: Przemyslaw Gomulka Date: Wed, 9 Sep 2020 10:11:57 +0200 Subject: [PATCH 14/24] Apply suggestions from code review Co-authored-by: Jay Modi --- .../java/org/elasticsearch/common/xcontent/MediaType.java | 2 +- .../org/elasticsearch/common/xcontent/MediaTypeParser.java | 6 +++--- .../org/elasticsearch/common/xcontent/XContentType.java | 4 +--- .../java/org/elasticsearch/rest/AbstractRestChannel.java | 2 +- .../java/org/elasticsearch/rest/action/cat/RestTable.java | 2 +- 5 files changed, 7 insertions(+), 9 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 635375fb84368..25abc15f124af 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 @@ -25,6 +25,6 @@ public interface MediaType { String format(); default String typeSubtype(){ - return type()+"/"+subtype(); + return type() + "/" + subtype(); } } 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 index 33602bee43bcc..b74d5dbb4c393 100644 --- 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 @@ -24,8 +24,8 @@ import java.util.Map; public class MediaTypeParser { - private Map formatToMediaType = new HashMap<>(); - private Map typeSubtypeToMediaType = new HashMap<>(); + private final Map formatToMediaType = new HashMap<>(); + private final Map typeSubtypeToMediaType = new HashMap<>(); public MediaTypeParser(T[] acceptedMediaTypes) { for (T mediaType : acceptedMediaTypes) { @@ -40,7 +40,7 @@ public T fromMediaType(String mediaType) { } public T fromFormat(String format) { - if(format == null) { + if (format == null) { return null; } return formatToMediaType.get(format.toLowerCase(Locale.ROOT)); 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 3e004f88d806e..ab27dcf703ebc 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 @@ -111,12 +111,10 @@ public XContent xContent() { } }; - public static MediaTypeParser mediaTypeParser = new MediaTypeParser<>(XContentType.values()) + public static final MediaTypeParser mediaTypeParser = new MediaTypeParser<>(XContentType.values()) .withAdditionalMediaType("application/*", JSON) .withAdditionalMediaType("application/x-ndjson", JSON); - - /** * Accepts either a format string, which is equivalent to {@link XContentType#shortName()} or a media type that optionally has * parameters and attempts to match the value to an {@link XContentType}. The comparisons are done in lower case format and this method diff --git a/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java b/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java index a9f28ae286982..c5bf1efa2cdb1 100644 --- a/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java +++ b/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java @@ -100,7 +100,7 @@ public XContentBuilder newBuilder(@Nullable XContentType requestContentType, @Nu if (responseContentType == null) { //TODO PG shoudld format vs acceptHeader be always the same, do we allow overriding? responseContentType = XContentType.fromFormat(format); - if(responseContentType == null) { + if (responseContentType == null) { responseContentType = XContentType.fromMediaType(acceptHeader); } } 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 bb3476d7ca3ef..fa4bd586a4723 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 @@ -59,7 +59,7 @@ public static RestResponse buildResponse(Table table, RestChannel channel) throw } private static XContentType getxContentType(RestRequest request) { - if(request.hasParam("format")) { + if (request.hasParam("format")) { return XContentType.fromFormat(request.param("format")); } return XContentType.fromMediaType(request.header("Accept")); From b5f1eff4a93591c8f23fd657b7ec0c0971bb7e43 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Wed, 9 Sep 2020 16:59:09 +0200 Subject: [PATCH 15/24] code review follow up --- .../common/xcontent/MediaType.java | 2 +- .../common/xcontent/MediaTypeParser.java | 57 ++++++++++--------- .../common/xcontent/XContentType.java | 8 +-- .../common/xcontent/MediaTypeParserTests.java | 23 ++++++++ .../xpack/sql/plugin/RestSqlQueryAction.java | 18 +++--- .../xpack/sql/plugin/TextFormat.java | 3 +- 6 files changed, 68 insertions(+), 43 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 635375fb84368..85915771010dc 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 @@ -24,7 +24,7 @@ public interface MediaType { String subtype(); String format(); - default String typeSubtype(){ + default String typeWithSubtype(){ return type()+"/"+subtype(); } } 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 index 33602bee43bcc..39752556d7fcd 100644 --- 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 @@ -25,11 +25,22 @@ public class MediaTypeParser { private Map formatToMediaType = new HashMap<>(); - private Map typeSubtypeToMediaType = new HashMap<>(); + private Map typeWithSubtypeToMediaType = new HashMap<>(); public MediaTypeParser(T[] acceptedMediaTypes) { for (T mediaType : acceptedMediaTypes) { - typeSubtypeToMediaType.put(mediaType.typeSubtype(), mediaType); + typeWithSubtypeToMediaType.put(mediaType.typeWithSubtype(), mediaType); + formatToMediaType.put(mediaType.format(), mediaType); + } + } + + public MediaTypeParser(T[] acceptedMediaTypes, Map additionalMediaTypes) { + this(acceptedMediaTypes); + for (Map.Entry entry : additionalMediaTypes.entrySet()) { + String typeWithSubtype = entry.getKey(); + T mediaType = entry.getValue(); + + typeWithSubtypeToMediaType.put(typeWithSubtype.toLowerCase(Locale.ROOT), mediaType); formatToMediaType.put(mediaType.format(), mediaType); } } @@ -40,40 +51,38 @@ public T fromMediaType(String mediaType) { } public T fromFormat(String format) { - if(format == null) { + if (format == null) { return null; } return formatToMediaType.get(format.toLowerCase(Locale.ROOT)); } - public MediaTypeParser withAdditionalMediaType(String typeSubtype, T xContentType) { - typeSubtypeToMediaType.put(typeSubtype.toLowerCase(Locale.ROOT), xContentType); - formatToMediaType.put(xContentType.format(), xContentType); - return this; - } - - public ParsedMediaType parseMediaType(String mediaType) { - if (mediaType != null) { - String headerValue = mediaType.toLowerCase(Locale.ROOT); - // split string on semicolon - // validate media type is accepted (IIRC whitespace is ok so trim it) //TODO PG whitespace only ok in params - // rest of strings are params. validate per RFC 7230 and use ones that we care about - // or use a regex and we can change if necessary - String[] split = headerValue.split(";"); + /** + * parsing media type that follows https://tools.ietf.org/html/rfc2616#section-3.7 + * @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].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]; - T xContentType = typeSubtypeToMediaType.get(type + "/" + subtype); + T xContentType = typeWithSubtypeToMediaType.get(type + "/" + subtype); if (xContentType != null) { Map parameters = new HashMap<>(); for (int i = 1; i < split.length; i++) { String[] keyValueParam = split[i].trim().split("="); + // should we validate that there are no spaces between key = value? + if (keyValueParam.length != 2) { + return null; + } parameters.put(keyValueParam[0].toLowerCase(Locale.ROOT), keyValueParam[1].toLowerCase(Locale.ROOT)); } - return new ParsedMediaType(type, subtype, parameters, xContentType); + return new ParsedMediaType(xContentType, parameters); } } @@ -86,14 +95,10 @@ public ParsedMediaType parseMediaType(String mediaType) { * // TODO PG to be extended with getCompatibleAPIVersion and more */ public class ParsedMediaType { - private final String type; - private final String subtype; private final Map parameters; private final T mediaType; - public ParsedMediaType(String type, String subtype, Map parameters, T mediaType) { - this.type = type; - this.subtype = subtype; + public ParsedMediaType(T mediaType, Map parameters) { this.parameters = parameters; this.mediaType = mediaType; } 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 3e004f88d806e..e87be65af7e51 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,6 +24,8 @@ import org.elasticsearch.common.xcontent.smile.SmileXContent; import org.elasticsearch.common.xcontent.yaml.YamlXContent; +import java.util.Map; + /** * The content type of {@link org.elasticsearch.common.xcontent.XContent}. */ @@ -111,10 +113,8 @@ public XContent xContent() { } }; - public static MediaTypeParser mediaTypeParser = new MediaTypeParser<>(XContentType.values()) - .withAdditionalMediaType("application/*", JSON) - .withAdditionalMediaType("application/x-ndjson", JSON); - + public static MediaTypeParser mediaTypeParser = new MediaTypeParser<>(XContentType.values(), + Map.of("application/*", JSON, "application/x-ndjson", JSON)); /** 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 index 427f6d1b46358..348cdf768937c 100644 --- 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 @@ -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 MediaTypeParserTests extends ESTestCase { MediaTypeParser mediaTypeParser = XContentType.mediaTypeParser; @@ -40,4 +42,25 @@ public void testJsonWithParameters() throws Exception { assertThat(mediaTypeParser.parseMediaType(mediaType + "; custom=123;charset=UTF-8").getParameters(), equalTo(Map.of("charset", "utf-8", "custom", "123"))); } + + public void testWhiteSpaceInTypeSubtype() { + String mediaType = " application/json "; + assertThat(mediaTypeParser.parseMediaType(mediaType).getMediaType(), + equalTo(XContentType.JSON)); + + assertThat(mediaTypeParser.parseMediaType(mediaType + "; custom=123; charset=UTF-8").getParameters(), + equalTo(Map.of("charset", "utf-8", "custom", "123"))); + assertThat(mediaTypeParser.parseMediaType(mediaType + "; custom=123;\n charset=UTF-8").getParameters(), + equalTo(Map.of("charset", "utf-8", "custom", "123"))); + + mediaType = " application / json "; + assertThat(mediaTypeParser.parseMediaType(mediaType), + is(nullValue())); + } + + public void testInvalidParameters() throws Exception { + String mediaType = "application/json"; + assertThat(mediaTypeParser.parseMediaType(mediaType + "; keyvalueNoEqualsSign"), + is(nullValue())); + } } 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 39f371b66c437..6fe4b31ba1182 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 @@ -64,9 +64,7 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli * isn't but there is a {@code Accept} header then we use that. If there * isn't then we use the {@code Content-Type} header which is required. */ - String accept = null; - XContentType acceptHeader = null; - XContentType contentTypeHeader = null; + String accept; if (Mode.isDedicatedClient(sqlRequest.requestInfo().mode()) && (sqlRequest.binaryCommunication() == null || sqlRequest.binaryCommunication())) { @@ -80,13 +78,10 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli if ("*/*".equals(accept)) { // */* means "I don't care" which we should treat like not specifying the header accept = null; - } else { - acceptHeader = XContentType.fromMediaType(accept); } } if (accept == null) { accept = request.header("Content-Type"); - contentTypeHeader = XContentType.fromMediaType(accept); } assert accept != null : "The Content-Type header is required"; @@ -97,8 +92,7 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli * that doesn't parse it'll throw an {@link IllegalArgumentException} * which we turn into a 400 error. */ - //TODO PG this all logic needs a review from SQL team - XContentType xContentType = getXContentType(accept, acceptHeader, contentTypeHeader); + XContentType xContentType = getXContentType(accept); textFormat = xContentType == null ? TextFormat.fromMediaTypeOrFormat(accept) : null; if (xContentType == null && sqlRequest.columnar()) { @@ -136,7 +130,7 @@ public RestResponse buildResponse(SqlQueryResponse response) throws Exception { }); } - private XContentType getXContentType(String accept, XContentType acceptHeader, XContentType contentTypeHeader) { + private XContentType getXContentType(String accept) { if (accept == null) { return XContentType.JSON; } @@ -144,7 +138,11 @@ private XContentType getXContentType(String accept, XContentType acceptHeader, X if (xContentType != null) { return xContentType; } - return acceptHeader != null ? acceptHeader : contentTypeHeader; + xContentType = XContentType.fromMediaType(accept); + if (xContentType != null) { + return xContentType; + } + return xContentType; } @Override 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 e65577affb834..6bdcaa44e9595 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 @@ -204,7 +204,6 @@ String maybeEscape(String value, Character delimiter) { boolean hasHeader(RestRequest request) { String header = request.param(URL_PARAM_HEADER); if (header == null) { - //TODO PG in most places we only assume one accept header List values = request.getAllHeaderValues("Accept"); if (values != null) { // header values are separated by `;` so try breaking it down @@ -392,7 +391,7 @@ public String format() { } @Override - public String typeSubtype() { + public String typeWithSubtype() { return contentType(); } } From fa49be43ac51915037094f1e6dc16ab6bd78c0b5 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Mon, 14 Sep 2020 11:22:20 +0200 Subject: [PATCH 16/24] javadoc and validation --- .../common/xcontent/MediaType.java | 19 +++++++++++++++++++ .../common/xcontent/MediaTypeParser.java | 10 +++++++--- .../common/xcontent/MediaTypeParserTests.java | 5 ++++- .../common/text/TextTemplateEngine.java | 1 - 4 files changed, 30 insertions(+), 5 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 e58219a4a649b..fdbf9d0871862 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,11 +19,30 @@ package org.elasticsearch.common.xcontent; +/** + * 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, which when specified overrides Accept and Content-Type headers. + */ public interface MediaType { + /** + * Returns a type part of a MediaType + * i.e. application for application/json + */ String type(); + /** + * 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 + */ String format(); + /** + * returns a string representation of a media type. + */ default String typeWithSubtype(){ return type() + "/" + subtype(); } 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 index 4904ba6a3d530..7e99ae3e8e399 100644 --- 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 @@ -58,7 +58,7 @@ public T fromFormat(String format) { } /** - * parsing media type that follows https://tools.ietf.org/html/rfc2616#section-3.7 + * 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 */ @@ -75,9 +75,9 @@ public ParsedMediaType parseMediaType(String headerValue) { 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("="); - // should we validate that there are no spaces between key = value? - if (keyValueParam.length != 2) { + if (keyValueParam.length != 2 || hasSpaces(keyValueParam[0]) || hasSpaces(keyValueParam[1])) { return null; } parameters.put(keyValueParam[0].toLowerCase(Locale.ROOT), keyValueParam[1].toLowerCase(Locale.ROOT)); @@ -90,6 +90,10 @@ public ParsedMediaType parseMediaType(String headerValue) { return null; } + 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 * // TODO PG to be extended with getCompatibleAPIVersion and more 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 index 348cdf768937c..b2b1831ef7556 100644 --- 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 @@ -58,9 +58,12 @@ public void testWhiteSpaceInTypeSubtype() { is(nullValue())); } - public void testInvalidParameters() throws Exception { + public void testInvalidParameters() { String mediaType = "application/json"; assertThat(mediaTypeParser.parseMediaType(mediaType + "; keyvalueNoEqualsSign"), is(nullValue())); + + assertThat(mediaTypeParser.parseMediaType(mediaType + "; key = value"), + is(nullValue())); } } diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/text/TextTemplateEngine.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/text/TextTemplateEngine.java index 8e92532d6bc99..f6c84ef2ad995 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/text/TextTemplateEngine.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/text/TextTemplateEngine.java @@ -82,7 +82,6 @@ private XContentType detectContentType(String content) { //There must be a __ Date: Mon, 14 Sep 2020 11:22:25 +0200 Subject: [PATCH 17/24] Update x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java Co-authored-by: Bogdan Pintea --- .../xpack/sql/plugin/RestSqlQueryAction.java | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) 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 6fe4b31ba1182..4ade958b60e85 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 @@ -135,14 +135,7 @@ private XContentType getXContentType(String accept) { return XContentType.JSON; } XContentType xContentType = XContentType.fromFormat(accept); - if (xContentType != null) { - return xContentType; - } - xContentType = XContentType.fromMediaType(accept); - if (xContentType != null) { - return xContentType; - } - return xContentType; + return xContentType != null ? xContentType : XContentType.fromMediaType(accept); } @Override From 23c4e41ceca7ea2c90cc0f1708461e2dd81a71ac Mon Sep 17 00:00:00 2001 From: pgomulka Date: Mon, 14 Sep 2020 13:31:37 +0200 Subject: [PATCH 18/24] javadoc fix --- .../java/org/elasticsearch/common/xcontent/MediaType.java | 4 ++-- 1 file changed, 2 insertions(+), 2 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 fdbf9d0871862..12d3b02c9bac1 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 @@ -20,9 +20,9 @@ package org.elasticsearch.common.xcontent; /** - * Abstracts a Media Type and a format parameter. + * 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, which when specified overrides Accept and Content-Type headers. + * format is an URL parameter, specifies response media type. */ public interface MediaType { /** From b1e3fb1defcffa4f2ba75a7d4de20517e9550432 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Mon, 14 Sep 2020 13:45:10 +0200 Subject: [PATCH 19/24] remove shortName --- .../common/xcontent/MediaType.java | 3 +++ .../common/xcontent/XContentType.java | 16 +++++---------- .../xpack/sql/plugin/TextFormat.java | 20 +++++-------------- 3 files changed, 13 insertions(+), 26 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 12d3b02c9bac1..9e3affc5bceae 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 @@ -30,13 +30,16 @@ public interface MediaType { * i.e. application for application/json */ String type(); + /** * 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 but format is txt */ String format(); 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 542a0e419d8be..18a41955e4780 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 @@ -46,7 +46,7 @@ public String mediaType() { } @Override - public String shortName() { + public String subtype() { return "json"; } @@ -65,7 +65,7 @@ public String mediaTypeWithoutParameters() { } @Override - public String shortName() { + public String subtype() { return "smile"; } @@ -84,7 +84,7 @@ public String mediaTypeWithoutParameters() { } @Override - public String shortName() { + public String subtype() { return "yaml"; } @@ -103,7 +103,7 @@ public String mediaTypeWithoutParameters() { } @Override - public String shortName() { + public String subtype() { return "cbor"; } @@ -151,7 +151,6 @@ public String mediaType() { return mediaTypeWithoutParameters(); } - public abstract String shortName(); public abstract XContent xContent(); @@ -163,13 +162,8 @@ public String type() { return "application"; } - @Override - public String subtype() { - return shortName(); - } - @Override public String format() { - return shortName(); + return subtype(); } } 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 6bdcaa44e9595..1f50c5fa7ea28 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 @@ -84,7 +84,7 @@ String format(RestRequest request, SqlQueryResponse response) { } @Override - String shortName() { + public String format() { return FORMAT_TEXT; } @@ -107,6 +107,8 @@ protected String eol() { public String subtype() { return "plain"; } + + }, /** @@ -131,7 +133,7 @@ protected String eol() { } @Override - String shortName() { + public String format() { return FORMAT_CSV; } @@ -241,7 +243,7 @@ protected String eol() { } @Override - String shortName() { + public String format() { return FORMAT_TSV; } @@ -326,13 +328,6 @@ static TextFormat fromMediaTypeOrFormat(String accept) { throw new IllegalArgumentException("invalid format [" + accept + "]"); } - /** - * Short name typically used by format parameter. - * Can differ from the IANA mime type. - */ - abstract String shortName(); - - /** * Formal IANA mime type. */ @@ -385,11 +380,6 @@ public String type() { return "text"; } - @Override - public String format() { - return shortName(); - } - @Override public String typeWithSubtype() { return contentType(); From c17a8958fbeb2ce9339129f11d2236acdbac9d5a Mon Sep 17 00:00:00 2001 From: pgomulka Date: Mon, 14 Sep 2020 14:33:28 +0200 Subject: [PATCH 20/24] javadoc fix --- .../elasticsearch/common/xcontent/XContentType.java | 13 +++++++------ 1 file changed, 7 insertions(+), 6 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 18a41955e4780..5801a3cd76def 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 @@ -118,10 +118,10 @@ public XContent xContent() { /** - * Accepts either a format string, which is equivalent to {@link XContentType#shortName()} or a media type that optionally has - * parameters and attempts to match the value to an {@link XContentType}. The comparisons are done in lower case format and this method - * also supports a wildcard accept for {@code application/*}. This method can be used to parse the {@code Accept} HTTP header or a - * format query string parameter. This method will return {@code null} if no match is found + * Accepts a format string, which is most of the time is equivalent to {@link XContentType#subtype()} + * 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); @@ -129,8 +129,9 @@ public static XContentType fromFormat(String mediaType) { /** * Attempts to match the given media type with the known {@link XContentType} values. This match is done in a case-insensitive manner. - * The provided media type should not include any parameters. This method is suitable for parsing part of the {@code Content-Type} - * HTTP header. This method will return {@code null} if no match is found + * The provided media type can optionally has parameters. + * 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); From 7d6bd08e0a9f18576c30ac8b355794eb22605160 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Mon, 14 Sep 2020 14:40:41 +0200 Subject: [PATCH 21/24] fix compile error --- .../xpack/watcher/input/http/ExecutableHttpInput.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 79d3918f7a205..d484ce1e93272 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.shortName()); + response.body().utf8ToString(), contentType.format()); } } else { payloadMap.put("_value", response.body().utf8ToString()); From 3c93954f1c40b90ecdadb5e4d50f675cf9dab0f2 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Mon, 14 Sep 2020 14:56:58 +0200 Subject: [PATCH 22/24] fix test compile --- .../java/org/elasticsearch/rest/BytesRestResponseTests.java | 2 +- .../xpack/watcher/support/WatcherTemplateTests.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/rest/BytesRestResponseTests.java b/server/src/test/java/org/elasticsearch/rest/BytesRestResponseTests.java index 945c29684f712..8eff3b23dc5cb 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.shortName()); + Map params = Collections.singletonMap("format", xContentType.format()); RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withParams(params).build(); RestChannel channel = detailed ? new DetailedExceptionRestChannel(request) : new SimpleExceptionRestChannel(request); 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 8c45bc15b4c9e..1b27e8151ecb2 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.shortName().toLowerCase(Locale.ROOT)) + .append(contentType.format().toLowerCase(Locale.ROOT)) .append("__::") .append(template) .toString(); From 63fb6c7766f9ce39c00891a85a9594a1d84e49a1 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Wed, 16 Sep 2020 18:37:13 +0200 Subject: [PATCH 23/24] remove todo and a testcase --- .../java/org/elasticsearch/common/xcontent/MediaTypeParser.java | 1 - .../org/elasticsearch/common/xcontent/MediaTypeParserTests.java | 2 ++ 2 files changed, 2 insertions(+), 1 deletion(-) 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 index 7e99ae3e8e399..3a5273408ce45 100644 --- 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 @@ -96,7 +96,6 @@ private boolean hasSpaces(String s) { /** * A media type object that contains all the information provided on a Content-Type or Accept header - * // TODO PG to be extended with getCompatibleAPIVersion and more */ public class ParsedMediaType { private final Map parameters; 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 index b2b1831ef7556..6514c7ec73306 100644 --- 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 @@ -65,5 +65,7 @@ public void testInvalidParameters() { assertThat(mediaTypeParser.parseMediaType(mediaType + "; key = value"), is(nullValue())); + assertThat(mediaTypeParser.parseMediaType(mediaType + "; key=") , + is(nullValue())); } } From 4598a0a9a1dbc301673e217849be11c7778cc539 Mon Sep 17 00:00:00 2001 From: Przemyslaw Gomulka Date: Thu, 17 Sep 2020 14:58:23 +0200 Subject: [PATCH 24/24] Apply suggestions from code review Co-authored-by: Jay Modi --- .../common/xcontent/MediaType.java | 2 +- .../common/xcontent/MediaTypeParser.java | 24 ++++++++++++------- 2 files changed, 16 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 9e3affc5bceae..2d61120288706 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 @@ -39,7 +39,7 @@ public interface MediaType { /** * 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 but format is txt + * Can differ from the MediaType's subtype i.e plain/text has a subtype of text but format is txt */ String format(); 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 index 3a5273408ce45..ce310a5b3f8e4 100644 --- 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 @@ -24,25 +24,31 @@ import java.util.Map; public class MediaTypeParser { - private final Map formatToMediaType = new HashMap<>(); - private final Map typeWithSubtypeToMediaType = new HashMap<>(); + private final Map formatToMediaType; + private final Map typeWithSubtypeToMediaType; public MediaTypeParser(T[] acceptedMediaTypes) { - for (T mediaType : acceptedMediaTypes) { - typeWithSubtypeToMediaType.put(mediaType.typeWithSubtype(), mediaType); - formatToMediaType.put(mediaType.format(), mediaType); - } + this(acceptedMediaTypes, Map.of()); } public MediaTypeParser(T[] acceptedMediaTypes, Map additionalMediaTypes) { - this(acceptedMediaTypes); + final int size = acceptedMediaTypes.length + additionalMediaTypes.size(); + Map formatMap = new HashMap<>(size); + Map typeMap = new HashMap<>(size); + for (T mediaType : acceptedMediaTypes) { + typeMap.put(mediaType.typeWithSubtype(), mediaType); + formatMap.put(mediaType.format(), mediaType); + } for (Map.Entry entry : additionalMediaTypes.entrySet()) { String typeWithSubtype = entry.getKey(); T mediaType = entry.getValue(); - typeWithSubtypeToMediaType.put(typeWithSubtype.toLowerCase(Locale.ROOT), mediaType); - formatToMediaType.put(mediaType.format(), mediaType); + typeMap.put(typeWithSubtype.toLowerCase(Locale.ROOT), mediaType); + formatMap.put(mediaType.format(), mediaType); } + + this.formatToMediaType = Map.copyOf(formatMap); + this.typeWithSubtypeToMediaType = Map.copyOf(typeMap); } public T fromMediaType(String mediaType) {