From 5e4239099c44ff785746ed7ee86b326789910c4d Mon Sep 17 00:00:00 2001 From: pgomulka Date: Wed, 2 Sep 2020 14:03:20 +0200 Subject: [PATCH 1/8] 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 2/8] 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 3/8] 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 4/8] 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 5/8] 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 6/8] 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 7/8] 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 8/8] 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 __