From 7514d87cbc1269be095ebcf77e586fc20e67849e Mon Sep 17 00:00:00 2001 From: pgomulka Date: Fri, 21 Aug 2020 17:31:35 +0200 Subject: [PATCH 01/40] Allow parsing Content-Type and Accept headers with version Content-Type and Accept headers expect a verioned form of media types like application/vnd.elasticsearch+json;compatible-with=7 when previously it was simple application/json or similar - it is still supported --- .../common/xcontent/XContentType.java | 38 +++++++++++-- .../common/xcontent/XContentTypeTests.java | 53 +++++++++++++++++++ 2 files changed, 88 insertions(+), 3 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 606284f046244..42129e4f5e4bb 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentType.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentType.java @@ -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}. @@ -114,13 +116,19 @@ public XContent xContent() { } }; + private static final Pattern COMPATIBLE_API_HEADER_PATTERN = Pattern.compile( + "(application|text)/(vnd.elasticsearch\\+)?([^;]+)(\\s*;\\s*compatible-with=(\\d+))?", + Pattern.CASE_INSENSITIVE); + /** * 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 */ - public static XContentType fromMediaTypeOrFormat(String mediaType) { + public static XContentType fromMediaTypeOrFormat(String mediaTypeHeaderValue) { + String mediaType = parseMediaType(mediaTypeHeaderValue); + if (mediaType == null) { return null; } @@ -130,7 +138,7 @@ public static XContentType fromMediaTypeOrFormat(String mediaType) { } } final String lowercaseMediaType = mediaType.toLowerCase(Locale.ROOT); - if (lowercaseMediaType.startsWith("application/*")) { + if (lowercaseMediaType.startsWith("application/*") || lowercaseMediaType.equals("*/*")) { return JSON; } @@ -142,7 +150,9 @@ public static XContentType fromMediaTypeOrFormat(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) { + public static XContentType fromMediaType(String mediaTypeHeaderValue) { + String mediaType = parseMediaType(mediaTypeHeaderValue); + final String lowercaseMediaType = Objects.requireNonNull(mediaType, "mediaType cannot be null").toLowerCase(Locale.ROOT); for (XContentType type : values()) { if (type.mediaTypeWithoutParameters().equals(lowercaseMediaType)) { @@ -157,6 +167,28 @@ public static XContentType fromMediaType(String mediaType) { return null; } + //public scope needed for text formats hack + public static String parseMediaType(String mediaType) { + if (mediaType != null) { + Matcher matcher = COMPATIBLE_API_HEADER_PATTERN.matcher(mediaType); + if (matcher.find()) { + return (matcher.group(1) + "/" + matcher.group(3)).toLowerCase(Locale.ROOT); + } + } + + return mediaType; + } + + public static String parseVersion(String mediaType){ + if(mediaType != null){ + Matcher matcher = COMPATIBLE_API_HEADER_PATTERN.matcher(mediaType); + if (matcher.find() && "vnd.elasticsearch+".equalsIgnoreCase(matcher.group(2))) { + + return matcher.group(5); + } + } + 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) + ";") || 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..7dd063e7e58d3 100644 --- a/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java +++ b/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java @@ -84,4 +84,57 @@ public void testFromRubbish() throws Exception { assertThat(XContentType.fromMediaTypeOrFormat("text/plain"), nullValue()); assertThat(XContentType.fromMediaTypeOrFormat("gobbly;goop"), nullValue()); } + + public void testMediaType() { + byte version = randomByte(); + assertThat(XContentType.parseMediaType("application/vnd.elasticsearch+json;compatible-with=" + version), + equalTo("application/json")); + assertThat(XContentType.parseMediaType("application/vnd.elasticsearch+cbor;compatible-with=" + version), + equalTo("application/cbor")); + assertThat(XContentType.parseMediaType("application/vnd.elasticsearch+smile;compatible-with=" + version), + equalTo("application/smile")); + assertThat(XContentType.parseMediaType("application/json"), + equalTo("application/json")); + + + assertThat(XContentType.parseMediaType("APPLICATION/VND.ELASTICSEARCH+JSON;COMPATIBLE-WITH=" + version), + equalTo("application/json")); + assertThat(XContentType.parseMediaType("APPLICATION/JSON"), + equalTo("application/json")); + } + + public void testVersionParsing() { + String version = String.valueOf(Math.abs(randomByte())); + assertThat(XContentType.parseVersion("application/vnd.elasticsearch+json;compatible-with=" + version), + equalTo(version)); + assertThat(XContentType.parseVersion("application/vnd.elasticsearch+cbor;compatible-with=" + version), + equalTo(version)); + assertThat(XContentType.parseVersion("application/vnd.elasticsearch+smile;compatible-with=" + version), + equalTo(version)); + assertThat(XContentType.parseVersion("application/json"), + nullValue()); + + + assertThat(XContentType.parseVersion("APPLICATION/VND.ELASTICSEARCH+JSON;COMPATIBLE-WITH=" + version), + equalTo(version)); + assertThat(XContentType.parseVersion("APPLICATION/JSON"), + nullValue()); + } + + public void testVersionParsingOnText() { + String version = String.valueOf(Math.abs(randomByte())); + assertThat(XContentType.parseVersion("text/vnd.elasticsearch+csv;compatible-with=" + version), + equalTo(version)); + assertThat(XContentType.parseVersion("text/vnd.elasticsearch+text;compatible-with=" + version), + equalTo(version)); + assertThat(XContentType.parseVersion("text/vnd.elasticsearch+tab-separated-values;compatible-with=" + version), + equalTo(version)); + assertThat(XContentType.parseVersion("text/csv"), + nullValue()); + + assertThat(XContentType.parseVersion("TEXT/VND.ELASTICSEARCH+CSV;COMPATIBLE-WITH=" + version), + equalTo(version)); + assertThat(XContentType.parseVersion("TEXT/csv"), + nullValue()); + } } From 921bc6f6047c2c8412a8cd44c2fa3b40c398cc23 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Tue, 1 Sep 2020 09:43:11 +0200 Subject: [PATCH 02/40] code review follow up --- .../common/xcontent/XContentType.java | 32 +++++++++++++++---- .../common/time/JavaDateMathParserTests.java | 28 ++++++++++++++++ .../common/xcontent/XContentTypeTests.java | 23 +++++++++++-- 3 files changed, 74 insertions(+), 9 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 42129e4f5e4bb..54686486db168 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,10 +24,12 @@ import org.elasticsearch.common.xcontent.smile.SmileXContent; import org.elasticsearch.common.xcontent.yaml.YamlXContent; +import java.util.Arrays; import java.util.Locale; import java.util.Objects; import java.util.regex.Matcher; import java.util.regex.Pattern; +import java.util.stream.Collectors; /** * The content type of {@link org.elasticsearch.common.xcontent.XContent}. @@ -116,10 +118,26 @@ public XContent xContent() { } }; + /** + * 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 COMPATIBLE_API_HEADER_PATTERN = Pattern.compile( - "(application|text)/(vnd.elasticsearch\\+)?([^;]+)(\\s*;\\s*compatible-with=(\\d+))?", + //type + "^(application|text)/" + + // type with custom subtype and a version: vnd.elasticsearch+json;compatible-with=7 + "((vnd\\.elasticsearch\\+([^;\\s]+)(\\s*;\\s*compatible-with=(\\d+)))" + + "|([^;\\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); + + /*Pattern.compile( + "^(application|text)/(vnd\\.elasticsearch\\+)?([^;]+)(\\s*;\\s*compatible-with=(\\d+))?$", + Pattern.CASE_INSENSITIVE); +*/ /** * 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 @@ -172,19 +190,21 @@ public static String parseMediaType(String mediaType) { if (mediaType != null) { Matcher matcher = COMPATIBLE_API_HEADER_PATTERN.matcher(mediaType); if (matcher.find()) { - return (matcher.group(1) + "/" + matcher.group(3)).toLowerCase(Locale.ROOT); + if(matcher.group(2).toLowerCase(Locale.ROOT).startsWith("vnd.elasticsearch")){ + return (matcher.group(1) + "/" + matcher.group(4)).toLowerCase(Locale.ROOT); + } + return (matcher.group(1) + "/" + matcher.group(7)).toLowerCase(Locale.ROOT); } } - return mediaType; + return null; } public static String parseVersion(String mediaType){ if(mediaType != null){ Matcher matcher = COMPATIBLE_API_HEADER_PATTERN.matcher(mediaType); - if (matcher.find() && "vnd.elasticsearch+".equalsIgnoreCase(matcher.group(2))) { - - return matcher.group(5); + if (matcher.find() && matcher.group(2).toLowerCase(Locale.ROOT).startsWith("vnd.elasticsearch")) { + return matcher.group(6); } } return null; diff --git a/server/src/test/java/org/elasticsearch/common/time/JavaDateMathParserTests.java b/server/src/test/java/org/elasticsearch/common/time/JavaDateMathParserTests.java index 96480ee60e7f6..3d640864bd72a 100644 --- a/server/src/test/java/org/elasticsearch/common/time/JavaDateMathParserTests.java +++ b/server/src/test/java/org/elasticsearch/common/time/JavaDateMathParserTests.java @@ -37,6 +37,34 @@ public class JavaDateMathParserTests extends ESTestCase { private final DateFormatter formatter = DateFormatter.forPattern("date_optional_time||epoch_millis"); private final DateMathParser parser = formatter.toDateMathParser(); + public void testInvalidAMPM() { + { + DateFormatter formatter = DateFormatter.forPattern("MM/dd/yyyy hh:mm a"); + DateMathParser parser = formatter.toDateMathParser(); + String date = "04/30/2020 05:48 PM"; + parser.parse(date, () -> 0, true, ZoneId.systemDefault()); + } + + } + public void testInvalidAMPM2() { + + DateFormatter formatter = DateFormatter.forPattern("MM/dd/yyyy N"); + DateMathParser parser = formatter.toDateMathParser(); + String date = "04/30/2020 1"; + parser.parse(date, () -> 0, true, ZoneId.systemDefault()); + + + } + + public void testInvalidAMPM3() { +//xxxx-'W'ww-e'T'HH:mm:ss.SSSZZ + DateFormatter formatter = DateFormatter.forPattern("weekyear_week"); + DateMathParser parser = formatter.toDateMathParser(); + String date = "2020-W08"; + parser.parse(date, () -> 0, true, ZoneId.systemDefault()); + + + } public void testOverridingLocaleOrZoneAndCompositeRoundUpParser() { //the pattern has to be composite and the match should not be on the first one DateFormatter formatter = DateFormatter.forPattern("date||epoch_millis").withLocale(randomLocale(random())); 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 7dd063e7e58d3..bde87d2c43b0b 100644 --- a/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java +++ b/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java @@ -22,8 +22,7 @@ import java.util.Locale; -import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.nullValue; +import static org.hamcrest.Matchers.*; public class XContentTypeTests extends ESTestCase { public void testFromJson() throws Exception { @@ -86,13 +85,15 @@ public void testFromRubbish() throws Exception { } public void testMediaType() { - byte version = randomByte(); + String version = String.valueOf(Math.abs(randomByte())); assertThat(XContentType.parseMediaType("application/vnd.elasticsearch+json;compatible-with=" + version), equalTo("application/json")); assertThat(XContentType.parseMediaType("application/vnd.elasticsearch+cbor;compatible-with=" + version), equalTo("application/cbor")); assertThat(XContentType.parseMediaType("application/vnd.elasticsearch+smile;compatible-with=" + version), equalTo("application/smile")); + assertThat(XContentType.parseMediaType("application/vnd.elasticsearch+yaml;compatible-with=" + version), + equalTo("application/yaml")); assertThat(XContentType.parseMediaType("application/json"), equalTo("application/json")); @@ -119,6 +120,22 @@ public void testVersionParsing() { equalTo(version)); assertThat(XContentType.parseVersion("APPLICATION/JSON"), nullValue()); + + assertThat(XContentType.parseVersion("application/json;compatible-with=" + version+".0"), + is(nullValue())); + } + + public void testMediaTypeWithoutESSubtype() { + String version = String.valueOf(Math.abs(randomByte())); + assertThat(XContentType.fromMediaTypeOrFormat("application/json;compatible-with=" + version), nullValue()); + } + + public void testAnchoring(){ + String version = String.valueOf(Math.abs(randomByte())); + assertThat(XContentType.fromMediaTypeOrFormat("sth_application/json;compatible-with=" + version+".0"), nullValue()); + assertThat(XContentType.fromMediaTypeOrFormat("sth_application/json;compatible-with=" + version+"_sth"), nullValue()); + //incorrect parameter not validated at the moment, just ignored + assertThat(XContentType.fromMediaTypeOrFormat("application/json;compatible-with=" + version+"_sth"), nullValue()); } public void testVersionParsingOnText() { From ee280bab965e94a29a388710804b1379ee7b9801 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Tue, 1 Sep 2020 10:58:47 +0200 Subject: [PATCH 03/40] unused imports --- .../org/elasticsearch/common/xcontent/XContentType.java | 6 ++---- .../elasticsearch/common/xcontent/XContentTypeTests.java | 4 +++- 2 files changed, 5 insertions(+), 5 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 54686486db168..dd1d10175720c 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,12 +24,10 @@ import org.elasticsearch.common.xcontent.smile.SmileXContent; import org.elasticsearch.common.xcontent.yaml.YamlXContent; -import java.util.Arrays; import java.util.Locale; import java.util.Objects; import java.util.regex.Matcher; import java.util.regex.Pattern; -import java.util.stream.Collectors; /** * The content type of {@link org.elasticsearch.common.xcontent.XContent}. @@ -125,9 +123,9 @@ public XContent xContent() { * 2. Media type without a version - for users not using compatible API i.e. application/json */ private static final Pattern COMPATIBLE_API_HEADER_PATTERN = Pattern.compile( - //type + //type "^(application|text)/" + - // type with custom subtype and a version: vnd.elasticsearch+json;compatible-with=7 + // custom subtype and a version: vnd.elasticsearch+json;compatible-with=7 "((vnd\\.elasticsearch\\+([^;\\s]+)(\\s*;\\s*compatible-with=(\\d+)))" + "|([^;\\s]+))" + //subtype: json,yaml,etc some of these are defined in x-pack so can't be enumerated "(?:\\s*;\\s*(charset=UTF-8)?)?$", 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 bde87d2c43b0b..a177e7f48c057 100644 --- a/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java +++ b/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java @@ -22,7 +22,9 @@ import java.util.Locale; -import static org.hamcrest.Matchers.*; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.nullValue; public class XContentTypeTests extends ESTestCase { public void testFromJson() throws Exception { From 433de741d264768174cd95a9475d77cc0d41f247 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Wed, 2 Sep 2020 11:31:48 +0200 Subject: [PATCH 04/40] revert unrelated test --- .../common/time/JavaDateMathParserTests.java | 28 ------------------- 1 file changed, 28 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/common/time/JavaDateMathParserTests.java b/server/src/test/java/org/elasticsearch/common/time/JavaDateMathParserTests.java index 3d640864bd72a..96480ee60e7f6 100644 --- a/server/src/test/java/org/elasticsearch/common/time/JavaDateMathParserTests.java +++ b/server/src/test/java/org/elasticsearch/common/time/JavaDateMathParserTests.java @@ -37,34 +37,6 @@ public class JavaDateMathParserTests extends ESTestCase { private final DateFormatter formatter = DateFormatter.forPattern("date_optional_time||epoch_millis"); private final DateMathParser parser = formatter.toDateMathParser(); - public void testInvalidAMPM() { - { - DateFormatter formatter = DateFormatter.forPattern("MM/dd/yyyy hh:mm a"); - DateMathParser parser = formatter.toDateMathParser(); - String date = "04/30/2020 05:48 PM"; - parser.parse(date, () -> 0, true, ZoneId.systemDefault()); - } - - } - public void testInvalidAMPM2() { - - DateFormatter formatter = DateFormatter.forPattern("MM/dd/yyyy N"); - DateMathParser parser = formatter.toDateMathParser(); - String date = "04/30/2020 1"; - parser.parse(date, () -> 0, true, ZoneId.systemDefault()); - - - } - - public void testInvalidAMPM3() { -//xxxx-'W'ww-e'T'HH:mm:ss.SSSZZ - DateFormatter formatter = DateFormatter.forPattern("weekyear_week"); - DateMathParser parser = formatter.toDateMathParser(); - String date = "2020-W08"; - parser.parse(date, () -> 0, true, ZoneId.systemDefault()); - - - } public void testOverridingLocaleOrZoneAndCompositeRoundUpParser() { //the pattern has to be composite and the match should not be on the first one DateFormatter formatter = DateFormatter.forPattern("date||epoch_millis").withLocale(randomLocale(random())); From 51467e0ea1d8af04da4c4f5eac3449a3a40da9e6 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Wed, 2 Sep 2020 17:26:06 +0200 Subject: [PATCH 05/40] remove */* and method visibility --- .../org/elasticsearch/common/xcontent/XContentType.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 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 dd1d10175720c..37c72ac4fc5f2 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 @@ -183,8 +183,8 @@ public static XContentType fromMediaType(String mediaTypeHeaderValue) { return null; } - //public scope needed for text formats hack - public static String parseMediaType(String mediaType) { + // package private for testing + static String parseMediaType(String mediaType) { if (mediaType != null) { Matcher matcher = COMPATIBLE_API_HEADER_PATTERN.matcher(mediaType); if (matcher.find()) { @@ -198,7 +198,8 @@ public static String parseMediaType(String mediaType) { return null; } - public static String parseVersion(String mediaType){ + // package private for testing + static String parseVersion(String mediaType){ if(mediaType != null){ Matcher matcher = COMPATIBLE_API_HEADER_PATTERN.matcher(mediaType); if (matcher.find() && matcher.group(2).toLowerCase(Locale.ROOT).startsWith("vnd.elasticsearch")) { From ccaadbd7867f9a5cf9211f6a6f3a22456b8e9abd Mon Sep 17 00:00:00 2001 From: pgomulka Date: Thu, 3 Sep 2020 15:52:05 +0200 Subject: [PATCH 06/40] typos --- .../java/org/elasticsearch/common/xcontent/XContentType.java | 2 +- .../src/main/java/org/elasticsearch/ElasticsearchException.java | 2 +- 2 files changed, 2 insertions(+), 2 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 37c72ac4fc5f2..5836cd11f3de9 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,7 +118,7 @@ public XContent xContent() { /** * 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 + * 1. Media type with a version - requires a custom vnd.elasticsearch 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 */ diff --git a/server/src/main/java/org/elasticsearch/ElasticsearchException.java b/server/src/main/java/org/elasticsearch/ElasticsearchException.java index 26793eaf2dcdd..cd3330a1e18b3 100644 --- a/server/src/main/java/org/elasticsearch/ElasticsearchException.java +++ b/server/src/main/java/org/elasticsearch/ElasticsearchException.java @@ -631,7 +631,7 @@ public static ElasticsearchException[] guessRootCauses(Throwable t) { * parsing exception because that is generally the most interesting * exception to return to the user. If that exception is caused by * an ElasticsearchException we'd like to keep unwrapping because - * ElasticserachExceptions tend to contain useful information for + * ElasticsearchExceptions tend to contain useful information for * the user. */ Throwable cause = ex.getCause(); From d9ee420f73ebe68a40842963a2df89c3eb03f98d Mon Sep 17 00:00:00 2001 From: pgomulka Date: Thu, 17 Sep 2020 19:04:37 +0200 Subject: [PATCH 07/40] exted parsing with parameters --- .../common/xcontent/MediaTypeParser.java | 86 ++++++++++++++----- .../common/xcontent/XContentType.java | 47 +++++----- .../common/xcontent/XContentTypeTests.java | 65 ++++++-------- .../xpack/sql/plugin/TextFormat.java | 7 +- .../xpack/sql/plugin/TextFormatTests.java | 19 +++- 5 files changed, 135 insertions(+), 89 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 ce310a5b3f8e4..2d6410dc68824 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,33 +22,19 @@ import java.util.HashMap; import java.util.Locale; import java.util.Map; +import java.util.stream.Collectors; public class MediaTypeParser { private final Map formatToMediaType; private final Map typeWithSubtypeToMediaType; + private final Map> parametersMap; - public MediaTypeParser(T[] acceptedMediaTypes) { - this(acceptedMediaTypes, Map.of()); - } - - public MediaTypeParser(T[] acceptedMediaTypes, Map additionalMediaTypes) { - 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(); - - typeMap.put(typeWithSubtype.toLowerCase(Locale.ROOT), mediaType); - formatMap.put(mediaType.format(), mediaType); - } - this.formatToMediaType = Map.copyOf(formatMap); - this.typeWithSubtypeToMediaType = Map.copyOf(typeMap); + public MediaTypeParser(Map formatToMediaType, Map typeWithSubtypeToMediaType, + Map> parametersMap) { + this.formatToMediaType = Map.copyOf(formatToMediaType); + this.typeWithSubtypeToMediaType = Map.copyOf(typeWithSubtypeToMediaType); + this.parametersMap = Map.copyOf(parametersMap); } public T fromMediaType(String mediaType) { @@ -65,8 +51,10 @@ public T fromFormat(String format) { /** * 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 + * //todo pg should this all be maybe based on a regex? */ public ParsedMediaType parseMediaType(String headerValue) { if (headerValue != null) { @@ -75,9 +63,11 @@ public ParsedMediaType parseMediaType(String headerValue) { String[] typeSubtype = split[0].trim().toLowerCase(Locale.ROOT) .split("/"); if (typeSubtype.length == 2) { + String type = typeSubtype[0]; String subtype = typeSubtype[1]; - T xContentType = typeWithSubtypeToMediaType.get(type + "/" + subtype); + String typeWithSubtype = type + "/" + subtype; + T xContentType = typeWithSubtypeToMediaType.get(typeWithSubtype); if (xContentType != null) { Map parameters = new HashMap<>(); for (int i = 1; i < split.length; i++) { @@ -86,7 +76,12 @@ public ParsedMediaType parseMediaType(String headerValue) { if (keyValueParam.length != 2 || hasSpaces(keyValueParam[0]) || hasSpaces(keyValueParam[1])) { return null; } - parameters.put(keyValueParam[0].toLowerCase(Locale.ROOT), keyValueParam[1].toLowerCase(Locale.ROOT)); + String parameterName = keyValueParam[0].toLowerCase(Locale.ROOT); + String parameterValue = keyValueParam[1].toLowerCase(Locale.ROOT); + if(isValidParameter(typeWithSubtype,parameterName,parameterValue) == false) { + return null; + } + parameters.put(parameterName, parameterValue); } return new ParsedMediaType(xContentType, parameters); } @@ -96,6 +91,17 @@ public ParsedMediaType parseMediaType(String headerValue) { return null; } + private boolean isValidParameter(String typeWithSubtype, String parameterName, String parameterValue) { + if(parametersMap.containsKey(typeWithSubtype)){ + Map parameters = parametersMap.get(typeWithSubtype); + if(parameters.containsKey(parameterName)){ + String regex = parameters.get(parameterName); + return parameterValue.matches(regex);//todo pg should we precompile regex? + } + } + return false; + } + private boolean hasSpaces(String s) { return s.trim().equals(s) == false; } @@ -120,4 +126,38 @@ public Map getParameters() { return parameters; } } + + public static class Builder { + private final Map formatMap = new HashMap<>(); + private final Map typeMap = new HashMap<>(); + private final Map> parametersMap = new HashMap<>(); + + public Builder withMediaTypesNoParams(T[] acceptedMediaTypes) { + for (T mediaType : acceptedMediaTypes) { + typeMap.put(mediaType.typeWithSubtype(), mediaType); + formatMap.put(mediaType.format(), mediaType); + } + return this; + } + + public Builder withMediaTypeNoParams(String alternativeMediaType, T mediaType) { + typeMap.put(alternativeMediaType.toLowerCase(Locale.ROOT), mediaType); + formatMap.put(mediaType.format(), mediaType); + return this; + } + + public Builder withMediaTypeAndParams(String alternativeMediaType, T mediaType, Map paramNameAndValueRegex) { + typeMap.put(alternativeMediaType.toLowerCase(Locale.ROOT), mediaType); + formatMap.put(mediaType.format(), mediaType); + //paramNameAndValueRegex all entries to lowercase + parametersMap.put(alternativeMediaType, paramNameAndValueRegex.entrySet().stream() + .collect(Collectors.toMap(entry -> entry.getKey().toLowerCase(Locale.ROOT), + entry-> entry.getValue().toLowerCase(Locale.ROOT)))); + return this; + } + + public MediaTypeParser build() { + return new MediaTypeParser<>(formatMap, typeMap, parametersMap); + } + } } 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 a72889d56c379..2af91bbdf2ccf 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,10 +24,6 @@ import org.elasticsearch.common.xcontent.smile.SmileXContent; 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; import java.util.Map; /** @@ -116,29 +112,24 @@ 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.elasticsearch 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 + * //public scope for testing //todo pg consider a getter? */ -//private static final Pattern COMPATIBLE_API_HEADER_PATTERN = Pattern.compile( -// //type -// "^(application|text)/" + -// // custom subtype and a version: vnd.elasticsearch+json;compatible-with=7 -// "((vnd\\.elasticsearch\\+([^;\\s]+)(\\s*;\\s*compatible-with=(\\d+)))" + -// "|([^;\\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); -// */ - public static final MediaTypeParser mediaTypeParser = new MediaTypeParser<>(XContentType.values(), - Map.of("application/*", JSON, "application/x-ndjson", JSON, - "application/vnd.elasticsearch+json", JSON, - "application/vnd.elasticsearch+smile", SMILE, - "application/vnd.elasticsearch+yaml", YAML, - "application/vnd.elasticsearch+cbor", CBOR)); - + public static final MediaTypeParser mediaTypeParser = new MediaTypeParser.Builder() + .withMediaTypesNoParams(XContentType.values())//todo pg maybe we could explicitly add cbor and smile? + .withMediaTypeAndParams("application/json", JSON, Map.of("charset", "UTF-8")) + .withMediaTypeAndParams("application/yaml", YAML, Map.of("charset", "UTF-8")) + .withMediaTypeAndParams("application/*", JSON, Map.of("charset", "UTF-8")) + .withMediaTypeAndParams("application/x-ndjson", JSON, Map.of("charset", "UTF-8")) + .withMediaTypeAndParams("application/vnd.elasticsearch+json", JSON, Map.of("compatible-with", "\\d+","charset", "UTF-8")) + .withMediaTypeAndParams("application/vnd.elasticsearch+smile", SMILE, Map.of("compatible-with", "\\d+","charset", "UTF-8")) + .withMediaTypeAndParams("application/vnd.elasticsearch+yaml", YAML, Map.of("compatible-with", "\\d+","charset", "UTF-8")) + .withMediaTypeAndParams("application/vnd.elasticsearch+cbor", CBOR, Map.of("compatible-with", "\\d+","charset", "UTF-8")) + .build(); /** * Accepts a format string, which is most of the time is equivalent to {@link XContentType#subtype()} @@ -160,13 +151,23 @@ public static XContentType fromMediaType(String mediaTypeHeaderValue) { return mediaTypeParser.fromMediaType(mediaTypeHeaderValue); } - private int index; XContentType(int index) { this.index = index; } + public static Byte parseVersion(String mediaType) { + MediaTypeParser.ParsedMediaType parsedMediaType = mediaTypeParser.parseMediaType(mediaType); + if(parsedMediaType != null) { + String version = parsedMediaType + .getParameters() + .get("compatible-with"); + return version != null ? Byte.parseByte(version) : null; + } + return null; + } + public int index() { return index; } 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 6cc885c7ad11c..ecfafe7ebccf4 100644 --- a/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java +++ b/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java @@ -97,26 +97,26 @@ public void testFromRubbish() throws Exception { public void testMediaType() { String version = String.valueOf(Math.abs(randomByte())); - assertThat(XContentType.parseMediaType("application/vnd.elasticsearch+json;compatible-with=" + version), - equalTo("application/json")); - assertThat(XContentType.parseMediaType("application/vnd.elasticsearch+cbor;compatible-with=" + version), - equalTo("application/cbor")); - assertThat(XContentType.parseMediaType("application/vnd.elasticsearch+smile;compatible-with=" + version), - equalTo("application/smile")); - assertThat(XContentType.parseMediaType("application/vnd.elasticsearch+yaml;compatible-with=" + version), - equalTo("application/yaml")); - assertThat(XContentType.parseMediaType("application/json"), - equalTo("application/json")); - - - assertThat(XContentType.parseMediaType("APPLICATION/VND.ELASTICSEARCH+JSON;COMPATIBLE-WITH=" + version), - equalTo("application/json")); - assertThat(XContentType.parseMediaType("APPLICATION/JSON"), - equalTo("application/json")); + assertThat(XContentType.fromMediaType("application/vnd.elasticsearch+json;compatible-with=" + version), + equalTo(XContentType.JSON)); + assertThat(XContentType.fromMediaType("application/vnd.elasticsearch+cbor;compatible-with=" + version), + equalTo(XContentType.CBOR)); + assertThat(XContentType.fromMediaType("application/vnd.elasticsearch+smile;compatible-with=" + version), + equalTo(XContentType.SMILE)); + assertThat(XContentType.fromMediaType("application/vnd.elasticsearch+yaml;compatible-with=" + version), + equalTo(XContentType.YAML)); + assertThat(XContentType.fromMediaType("application/json"), + equalTo(XContentType.JSON)); + + + assertThat(XContentType.fromMediaType("APPLICATION/VND.ELASTICSEARCH+JSON;COMPATIBLE-WITH=" + version), + equalTo(XContentType.JSON)); + assertThat(XContentType.fromMediaType("APPLICATION/JSON"), + equalTo(XContentType.JSON)); } public void testVersionParsing() { - String version = String.valueOf(Math.abs(randomByte())); + byte version = (byte)Math.abs(randomByte()); assertThat(XContentType.parseVersion("application/vnd.elasticsearch+json;compatible-with=" + version), equalTo(version)); assertThat(XContentType.parseVersion("application/vnd.elasticsearch+cbor;compatible-with=" + version), @@ -136,33 +136,20 @@ public void testVersionParsing() { is(nullValue())); } - public void testMediaTypeWithoutESSubtype() { - String version = String.valueOf(Math.abs(randomByte())); - assertThat(XContentType.fromMediaTypeOrFormat("application/json;compatible-with=" + version), nullValue()); + public void testUnrecognizedParameter() { + assertThat(XContentType.parseVersion("application/json; sth=123"), + is(nullValue())); } - public void testAnchoring(){ + public void testMediaTypeWithoutESSubtype() { String version = String.valueOf(Math.abs(randomByte())); - assertThat(XContentType.fromMediaTypeOrFormat("sth_application/json;compatible-with=" + version+".0"), nullValue()); - assertThat(XContentType.fromMediaTypeOrFormat("sth_application/json;compatible-with=" + version+"_sth"), nullValue()); - //incorrect parameter not validated at the moment, just ignored - assertThat(XContentType.fromMediaTypeOrFormat("application/json;compatible-with=" + version+"_sth"), nullValue()); + assertThat(XContentType.fromMediaType("application/json;compatible-with=" + version), nullValue()); } - public void testVersionParsingOnText() { + public void testAnchoring(){ String version = String.valueOf(Math.abs(randomByte())); - assertThat(XContentType.parseVersion("text/vnd.elasticsearch+csv;compatible-with=" + version), - equalTo(version)); - assertThat(XContentType.parseVersion("text/vnd.elasticsearch+text;compatible-with=" + version), - equalTo(version)); - assertThat(XContentType.parseVersion("text/vnd.elasticsearch+tab-separated-values;compatible-with=" + version), - equalTo(version)); - assertThat(XContentType.parseVersion("text/csv"), - nullValue()); - - assertThat(XContentType.parseVersion("TEXT/VND.ELASTICSEARCH+CSV;COMPATIBLE-WITH=" + version), - equalTo(version)); - assertThat(XContentType.parseVersion("TEXT/csv"), - nullValue()); + assertThat(XContentType.fromMediaType("sth_application/json;compatible-with=" + version+".0"), nullValue()); + assertThat(XContentType.fromMediaType("sth_application/json;compatible-with=" + version+"_sth"), nullValue()); + assertThat(XContentType.fromMediaType("application/json;compatible-with=" + version+"_sth"), nullValue()); } } 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 1f50c5fa7ea28..09da5b6952387 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 @@ -25,6 +25,7 @@ import java.time.ZonedDateTime; import java.util.List; import java.util.Locale; +import java.util.Map; import java.util.Objects; import java.util.function.Function; @@ -294,7 +295,11 @@ public String subtype() { private static final String PARAM_HEADER_ABSENT = "absent"; private static final String PARAM_HEADER_PRESENT = "present"; - private static final MediaTypeParser parser = new MediaTypeParser<>(TextFormat.values()); + private static final MediaTypeParser parser = new MediaTypeParser.Builder() + .withMediaTypeAndParams(PLAIN_TEXT.typeWithSubtype(), PLAIN_TEXT, Map.of("header", "present|absent", "charset", "utf-8")) + .withMediaTypeAndParams(CSV.typeWithSubtype(), CSV, Map.of("header", "present|absent", "charset", "utf-8")) + .withMediaTypeAndParams(TSV.typeWithSubtype(), TSV, Map.of("header", "present|absent", "charset", "utf-8")) + .build(); String format(RestRequest request, SqlQueryResponse response) { StringBuilder sb = new StringBuilder(); diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/TextFormatTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/TextFormatTests.java index a53fd8f94ad59..7bd8eaa4ba45d 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/TextFormatTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/TextFormatTests.java @@ -24,15 +24,14 @@ import static java.util.Collections.emptyList; import static java.util.Collections.singletonList; import static java.util.Collections.singletonMap; -import static org.elasticsearch.xpack.sql.plugin.TextFormat.CSV; -import static org.elasticsearch.xpack.sql.plugin.TextFormat.TSV; +import static org.elasticsearch.xpack.sql.plugin.TextFormat.*; import static org.hamcrest.CoreMatchers.is; public class TextFormatTests extends ESTestCase { public void testPlainTextDetection() { TextFormat text = TextFormat.fromMediaTypeOrFormat("text/plain"); - assertThat(text, is(TextFormat.PLAIN_TEXT)); + assertThat(text, is(PLAIN_TEXT)); } public void testCsvDetection() { @@ -45,6 +44,20 @@ public void testTsvDetection() { assertThat(text, is(TSV)); } + public void testParametersParsing() { + assertThat(TextFormat.fromMediaTypeOrFormat("text/plain; charset=utf-8"), is(PLAIN_TEXT)); + assertThat(TextFormat.fromMediaTypeOrFormat("text/plain; header=present"), is(PLAIN_TEXT)); + assertThat(TextFormat.fromMediaTypeOrFormat("text/plain; charset=utf-8; header=present"), is(PLAIN_TEXT)); + + assertThat(TextFormat.fromMediaTypeOrFormat("text/csv; charset=utf-8"), is(CSV)); + assertThat(TextFormat.fromMediaTypeOrFormat("text/csv; header=present"), is(CSV)); + assertThat(TextFormat.fromMediaTypeOrFormat("text/csv; charset=utf-8; header=present"), is(CSV)); + + assertThat(TextFormat.fromMediaTypeOrFormat("text/tab-separated-values; charset=utf-8"), is(TSV)); + assertThat(TextFormat.fromMediaTypeOrFormat("text/tab-separated-values; header=present"), is(TSV)); + assertThat(TextFormat.fromMediaTypeOrFormat("text/tab-separated-values; charset=utf-8; header=present"), is(TSV)); + } + public void testInvalidFormat() { Exception e = expectThrows(IllegalArgumentException.class, () -> TextFormat.fromMediaTypeOrFormat("text/garbage")); assertEquals("invalid format [text/garbage]", e.getMessage()); From 41150a6a7efce3a34ebcfa603274a7130c863788 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Thu, 17 Sep 2020 19:06:21 +0200 Subject: [PATCH 08/40] import --- .../org/elasticsearch/xpack/sql/plugin/TextFormatTests.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/TextFormatTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/TextFormatTests.java index 7bd8eaa4ba45d..a9cf6c57f34aa 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/TextFormatTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/TextFormatTests.java @@ -24,7 +24,9 @@ import static java.util.Collections.emptyList; import static java.util.Collections.singletonList; import static java.util.Collections.singletonMap; -import static org.elasticsearch.xpack.sql.plugin.TextFormat.*; +import static org.elasticsearch.xpack.sql.plugin.TextFormat.CSV; +import static org.elasticsearch.xpack.sql.plugin.TextFormat.PLAIN_TEXT; +import static org.elasticsearch.xpack.sql.plugin.TextFormat.TSV; import static org.hamcrest.CoreMatchers.is; public class TextFormatTests extends ESTestCase { From 2fc8f86027e3576cd677f644902b75265dc6ad26 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Thu, 17 Sep 2020 19:22:35 +0200 Subject: [PATCH 09/40] fix test --- .../common/xcontent/MediaTypeParserTests.java | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) 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 6514c7ec73306..076046b2caf2b 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 @@ -32,26 +32,26 @@ public class MediaTypeParserTests extends ESTestCase { MediaTypeParser mediaTypeParser = XContentType.mediaTypeParser; public void testJsonWithParameters() throws Exception { - String mediaType = "application/json"; + String mediaType = "application/vnd.elasticsearch+json"; assertThat(mediaTypeParser.parseMediaType(mediaType).getParameters(), equalTo(Collections.emptyMap())); assertThat(mediaTypeParser.parseMediaType(mediaType + ";").getParameters(), equalTo(Collections.emptyMap())); assertThat(mediaTypeParser.parseMediaType(mediaType + "; charset=UTF-8").getParameters(), equalTo(Map.of("charset", "utf-8"))); - assertThat(mediaTypeParser.parseMediaType(mediaType + "; custom=123;charset=UTF-8").getParameters(), - equalTo(Map.of("charset", "utf-8", "custom", "123"))); + assertThat(mediaTypeParser.parseMediaType(mediaType + "; compatible-with=123;charset=UTF-8").getParameters(), + equalTo(Map.of("charset", "utf-8", "compatible-with", "123"))); } public void testWhiteSpaceInTypeSubtype() { - String mediaType = " application/json "; + String mediaType = " application/vnd.elasticsearch+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"))); + assertThat(mediaTypeParser.parseMediaType(mediaType + "; compatible-with=123; charset=UTF-8").getParameters(), + equalTo(Map.of("charset", "utf-8", "compatible-with", "123"))); + assertThat(mediaTypeParser.parseMediaType(mediaType + "; compatible-with=123;\n charset=UTF-8").getParameters(), + equalTo(Map.of("charset", "utf-8", "compatible-with", "123"))); mediaType = " application / json "; assertThat(mediaTypeParser.parseMediaType(mediaType), @@ -59,7 +59,7 @@ public void testWhiteSpaceInTypeSubtype() { } public void testInvalidParameters() { - String mediaType = "application/json"; + String mediaType = "application/vnd.elasticsearch+json"; assertThat(mediaTypeParser.parseMediaType(mediaType + "; keyvalueNoEqualsSign"), is(nullValue())); From 734031a55d9f784f66c5e4a4fcfb9fb90e6e13cf Mon Sep 17 00:00:00 2001 From: pgomulka Date: Fri, 18 Sep 2020 12:21:48 +0200 Subject: [PATCH 10/40] scope parser --- .../elasticsearch/common/xcontent/MediaTypeParser.java | 8 -------- .../org/elasticsearch/common/xcontent/XContentType.java | 7 ++++--- .../common/xcontent/MediaTypeParserTests.java | 7 ++++++- 3 files changed, 10 insertions(+), 12 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 2d6410dc68824..1d6c264f31c9c 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 @@ -132,14 +132,6 @@ public static class Builder { private final Map typeMap = new HashMap<>(); private final Map> parametersMap = new HashMap<>(); - public Builder withMediaTypesNoParams(T[] acceptedMediaTypes) { - for (T mediaType : acceptedMediaTypes) { - typeMap.put(mediaType.typeWithSubtype(), mediaType); - formatMap.put(mediaType.format(), mediaType); - } - return this; - } - public Builder withMediaTypeNoParams(String alternativeMediaType, T mediaType) { typeMap.put(alternativeMediaType.toLowerCase(Locale.ROOT), mediaType); formatMap.put(mediaType.format(), 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 2af91bbdf2ccf..1ea3a5e9416d0 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,7 @@ import org.elasticsearch.common.xcontent.smile.SmileXContent; import org.elasticsearch.common.xcontent.yaml.YamlXContent; +import java.util.Collections; import java.util.Map; /** @@ -117,10 +118,10 @@ public XContent xContent() { * 1. Media type with a version - requires a custom vnd.elasticsearch 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 - * //public scope for testing //todo pg consider a getter? */ - public static final MediaTypeParser mediaTypeParser = new MediaTypeParser.Builder() - .withMediaTypesNoParams(XContentType.values())//todo pg maybe we could explicitly add cbor and smile? + private static final MediaTypeParser mediaTypeParser = new MediaTypeParser.Builder() + .withMediaTypeAndParams("application/smile", SMILE, Collections.emptyMap()) + .withMediaTypeAndParams("application/cbor", CBOR, Collections.emptyMap()) .withMediaTypeAndParams("application/json", JSON, Map.of("charset", "UTF-8")) .withMediaTypeAndParams("application/yaml", YAML, Map.of("charset", "UTF-8")) .withMediaTypeAndParams("application/*", JSON, Map.of("charset", "UTF-8")) 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 076046b2caf2b..26105082530b5 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 @@ -29,7 +29,12 @@ import static org.hamcrest.Matchers.nullValue; public class MediaTypeParserTests extends ESTestCase { - MediaTypeParser mediaTypeParser = XContentType.mediaTypeParser; + + MediaTypeParser mediaTypeParser = new MediaTypeParser.Builder() + .withMediaTypeAndParams("application/vnd.elasticsearch+json", + XContentType.JSON,Map.of("compatible-with","\\d+", + "charset","UTF-8")) + .build(); public void testJsonWithParameters() throws Exception { String mediaType = "application/vnd.elasticsearch+json"; From c04af650551c1522c8300830be2c3c754dad1759 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Fri, 18 Sep 2020 12:30:24 +0200 Subject: [PATCH 11/40] x-ndjson to be versioned --- .../org/elasticsearch/common/xcontent/XContentType.java | 1 + .../elasticsearch/common/xcontent/XContentTypeTests.java | 6 +++++- 2 files changed, 6 insertions(+), 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 1ea3a5e9416d0..d73c970f24a7e 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 @@ -130,6 +130,7 @@ public XContent xContent() { .withMediaTypeAndParams("application/vnd.elasticsearch+smile", SMILE, Map.of("compatible-with", "\\d+","charset", "UTF-8")) .withMediaTypeAndParams("application/vnd.elasticsearch+yaml", YAML, Map.of("compatible-with", "\\d+","charset", "UTF-8")) .withMediaTypeAndParams("application/vnd.elasticsearch+cbor", CBOR, Map.of("compatible-with", "\\d+","charset", "UTF-8")) + .withMediaTypeAndParams("application/vnd.elasticsearch+x-ndjson", JSON, Map.of("compatible-with", "\\d+","charset", "UTF-8")) .build(); /** 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 ecfafe7ebccf4..db7ec87922eb8 100644 --- a/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java +++ b/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java @@ -95,7 +95,7 @@ public void testFromRubbish() throws Exception { assertThat(XContentType.fromMediaType("gobbly;goop"), nullValue()); } - public void testMediaType() { + public void testVersionedMediaType() { String version = String.valueOf(Math.abs(randomByte())); assertThat(XContentType.fromMediaType("application/vnd.elasticsearch+json;compatible-with=" + version), equalTo(XContentType.JSON)); @@ -107,6 +107,8 @@ public void testMediaType() { equalTo(XContentType.YAML)); assertThat(XContentType.fromMediaType("application/json"), equalTo(XContentType.JSON)); + assertThat(XContentType.fromMediaType("application/vnd.elasticsearch+x-ndjson;compatible-with=" + version), + equalTo(XContentType.JSON)); assertThat(XContentType.fromMediaType("APPLICATION/VND.ELASTICSEARCH+JSON;COMPATIBLE-WITH=" + version), @@ -123,6 +125,8 @@ public void testVersionParsing() { equalTo(version)); assertThat(XContentType.parseVersion("application/vnd.elasticsearch+smile;compatible-with=" + version), equalTo(version)); + assertThat(XContentType.parseVersion("application/vnd.elasticsearch+x-ndjson;compatible-with=" + version), + equalTo(version)); assertThat(XContentType.parseVersion("application/json"), nullValue()); From 5c308136a2c10c74bca289b54b7f899afda6c9fb Mon Sep 17 00:00:00 2001 From: Przemyslaw Gomulka Date: Fri, 18 Sep 2020 16:51:05 +0200 Subject: [PATCH 12/40] Apply suggestions from code review Co-authored-by: Jay Modi --- .../common/xcontent/MediaTypeParser.java | 6 +++--- .../elasticsearch/common/xcontent/XContentType.java | 2 +- .../common/xcontent/MediaTypeParserTests.java | 4 ++-- .../common/xcontent/XContentTypeTests.java | 12 ++++++------ 4 files changed, 12 insertions(+), 12 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 1d6c264f31c9c..ba87858b1e814 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 @@ -78,7 +78,7 @@ public ParsedMediaType parseMediaType(String headerValue) { } String parameterName = keyValueParam[0].toLowerCase(Locale.ROOT); String parameterValue = keyValueParam[1].toLowerCase(Locale.ROOT); - if(isValidParameter(typeWithSubtype,parameterName,parameterValue) == false) { + if (isValidParameter(typeWithSubtype, parameterName, parameterValue) == false) { return null; } parameters.put(parameterName, parameterValue); @@ -92,9 +92,9 @@ public ParsedMediaType parseMediaType(String headerValue) { } private boolean isValidParameter(String typeWithSubtype, String parameterName, String parameterValue) { - if(parametersMap.containsKey(typeWithSubtype)){ + if (parametersMap.containsKey(typeWithSubtype)) { Map parameters = parametersMap.get(typeWithSubtype); - if(parameters.containsKey(parameterName)){ + if (parameters.containsKey(parameterName)) { String regex = parameters.get(parameterName); return parameterValue.matches(regex);//todo pg should we precompile regex? } 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 d73c970f24a7e..31280cfe51534 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 @@ -161,7 +161,7 @@ public static XContentType fromMediaType(String mediaTypeHeaderValue) { public static Byte parseVersion(String mediaType) { MediaTypeParser.ParsedMediaType parsedMediaType = mediaTypeParser.parseMediaType(mediaType); - if(parsedMediaType != null) { + if (parsedMediaType != null) { String version = parsedMediaType .getParameters() .get("compatible-with"); 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 26105082530b5..76cbb03a4ca53 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 @@ -32,8 +32,8 @@ public class MediaTypeParserTests extends ESTestCase { MediaTypeParser mediaTypeParser = new MediaTypeParser.Builder() .withMediaTypeAndParams("application/vnd.elasticsearch+json", - XContentType.JSON,Map.of("compatible-with","\\d+", - "charset","UTF-8")) + XContentType.JSON, Map.of("compatible-with", "\\d+", + "charset", "UTF-8")) .build(); public void testJsonWithParameters() throws Exception { diff --git a/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java b/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java index db7ec87922eb8..686bc9fa65405 100644 --- a/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java +++ b/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java @@ -118,7 +118,7 @@ public void testVersionedMediaType() { } public void testVersionParsing() { - byte version = (byte)Math.abs(randomByte()); + byte version = (byte) Math.abs(randomByte()); assertThat(XContentType.parseVersion("application/vnd.elasticsearch+json;compatible-with=" + version), equalTo(version)); assertThat(XContentType.parseVersion("application/vnd.elasticsearch+cbor;compatible-with=" + version), @@ -136,7 +136,7 @@ public void testVersionParsing() { assertThat(XContentType.parseVersion("APPLICATION/JSON"), nullValue()); - assertThat(XContentType.parseVersion("application/json;compatible-with=" + version+".0"), + assertThat(XContentType.parseVersion("application/json;compatible-with=" + version + ".0"), is(nullValue())); } @@ -150,10 +150,10 @@ public void testMediaTypeWithoutESSubtype() { assertThat(XContentType.fromMediaType("application/json;compatible-with=" + version), nullValue()); } - public void testAnchoring(){ + public void testAnchoring() { String version = String.valueOf(Math.abs(randomByte())); - assertThat(XContentType.fromMediaType("sth_application/json;compatible-with=" + version+".0"), nullValue()); - assertThat(XContentType.fromMediaType("sth_application/json;compatible-with=" + version+"_sth"), nullValue()); - assertThat(XContentType.fromMediaType("application/json;compatible-with=" + version+"_sth"), nullValue()); + assertThat(XContentType.fromMediaType("sth_application/json;compatible-with=" + version + ".0"), nullValue()); + assertThat(XContentType.fromMediaType("sth_application/json;compatible-with=" + version + "_sth"), nullValue()); + assertThat(XContentType.fromMediaType("application/json;compatible-with=" + version + "_sth"), nullValue()); } } From 7b760ba92ea2c032febabc5ced960802dd9385b1 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Fri, 18 Sep 2020 17:10:06 +0200 Subject: [PATCH 13/40] precompile pattern --- .../common/xcontent/MediaTypeParser.java | 28 +++++++++++-------- 1 file changed, 17 insertions(+), 11 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 ba87858b1e814..75e91a7743d42 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,16 +22,16 @@ import java.util.HashMap; import java.util.Locale; import java.util.Map; +import java.util.regex.Pattern; import java.util.stream.Collectors; public class MediaTypeParser { private final Map formatToMediaType; private final Map typeWithSubtypeToMediaType; - private final Map> parametersMap; - + private final Map> parametersMap; public MediaTypeParser(Map formatToMediaType, Map typeWithSubtypeToMediaType, - Map> parametersMap) { + Map> parametersMap) { this.formatToMediaType = Map.copyOf(formatToMediaType); this.typeWithSubtypeToMediaType = Map.copyOf(typeWithSubtypeToMediaType); this.parametersMap = Map.copyOf(parametersMap); @@ -93,10 +93,10 @@ public ParsedMediaType parseMediaType(String headerValue) { private boolean isValidParameter(String typeWithSubtype, String parameterName, String parameterValue) { if (parametersMap.containsKey(typeWithSubtype)) { - Map parameters = parametersMap.get(typeWithSubtype); + Map parameters = parametersMap.get(typeWithSubtype); if (parameters.containsKey(parameterName)) { - String regex = parameters.get(parameterName); - return parameterValue.matches(regex);//todo pg should we precompile regex? + Pattern regex = parameters.get(parameterName); + return regex.matcher(parameterValue).matches(); } } return false; @@ -130,7 +130,7 @@ public Map getParameters() { public static class Builder { private final Map formatMap = new HashMap<>(); private final Map typeMap = new HashMap<>(); - private final Map> parametersMap = new HashMap<>(); + private final Map> parametersMap = new HashMap<>(); public Builder withMediaTypeNoParams(String alternativeMediaType, T mediaType) { typeMap.put(alternativeMediaType.toLowerCase(Locale.ROOT), mediaType); @@ -141,10 +141,16 @@ public Builder withMediaTypeNoParams(String alternativeMediaType, T mediaType public Builder withMediaTypeAndParams(String alternativeMediaType, T mediaType, Map paramNameAndValueRegex) { typeMap.put(alternativeMediaType.toLowerCase(Locale.ROOT), mediaType); formatMap.put(mediaType.format(), mediaType); - //paramNameAndValueRegex all entries to lowercase - parametersMap.put(alternativeMediaType, paramNameAndValueRegex.entrySet().stream() - .collect(Collectors.toMap(entry -> entry.getKey().toLowerCase(Locale.ROOT), - entry-> entry.getValue().toLowerCase(Locale.ROOT)))); + + Map parametersForMediaType = new HashMap<>(paramNameAndValueRegex.size()); + for (Map.Entry params : paramNameAndValueRegex.entrySet()) { + String parameterName = params.getKey().toLowerCase(Locale.ROOT); + String parameterRegex = params.getValue().toLowerCase(Locale.ROOT); + Pattern pattern = Pattern.compile(parameterRegex, Pattern.CASE_INSENSITIVE); + parametersForMediaType.put(parameterName, pattern); + } + parametersMap.put(alternativeMediaType, parametersForMediaType); + return this; } From 5853561bb30b1d61aaf3db6b74bd1aa7f43e6ea8 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Mon, 21 Sep 2020 10:45:46 +0200 Subject: [PATCH 14/40] import --- .../java/org/elasticsearch/common/xcontent/MediaTypeParser.java | 1 - 1 file changed, 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 75e91a7743d42..ef498349d132b 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 @@ -23,7 +23,6 @@ import java.util.Locale; import java.util.Map; import java.util.regex.Pattern; -import java.util.stream.Collectors; public class MediaTypeParser { private final Map formatToMediaType; From b6ebd6399abcec1faad8a994ae202c82151139f7 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Mon, 21 Sep 2020 19:07:24 +0200 Subject: [PATCH 15/40] removal of unused method and pattern in method --- .../common/xcontent/MediaTypeParser.java | 14 ++++------- .../common/xcontent/XContentType.java | 24 ++++++++++++------- .../common/xcontent/MediaTypeParserTests.java | 5 ++-- .../xpack/sql/plugin/TextFormat.java | 10 +++++--- 4 files changed, 29 insertions(+), 24 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 ef498349d132b..1b737c8549691 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 @@ -131,21 +131,15 @@ public static class Builder { private final Map typeMap = new HashMap<>(); private final Map> parametersMap = new HashMap<>(); - public Builder withMediaTypeNoParams(String alternativeMediaType, T mediaType) { - typeMap.put(alternativeMediaType.toLowerCase(Locale.ROOT), mediaType); - formatMap.put(mediaType.format(), mediaType); - return this; - } - - public Builder withMediaTypeAndParams(String alternativeMediaType, T mediaType, Map paramNameAndValueRegex) { + public Builder withMediaTypeAndParams(String alternativeMediaType, T mediaType, Map paramNameAndValueRegex) { typeMap.put(alternativeMediaType.toLowerCase(Locale.ROOT), mediaType); formatMap.put(mediaType.format(), mediaType); Map parametersForMediaType = new HashMap<>(paramNameAndValueRegex.size()); - for (Map.Entry params : paramNameAndValueRegex.entrySet()) { + for (Map.Entry params : paramNameAndValueRegex.entrySet()) { String parameterName = params.getKey().toLowerCase(Locale.ROOT); - String parameterRegex = params.getValue().toLowerCase(Locale.ROOT); - Pattern pattern = Pattern.compile(parameterRegex, Pattern.CASE_INSENSITIVE); + Pattern parameterRegex = params.getValue(); + Pattern pattern = Pattern.compile(parameterRegex.pattern(), Pattern.CASE_INSENSITIVE); parametersForMediaType.put(parameterName, pattern); } parametersMap.put(alternativeMediaType, parametersForMediaType); 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 31280cfe51534..431d30c891223 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,7 @@ import java.util.Collections; import java.util.Map; +import java.util.regex.Pattern; /** * The content type of {@link org.elasticsearch.common.xcontent.XContent}. @@ -122,15 +123,20 @@ public XContent xContent() { private static final MediaTypeParser mediaTypeParser = new MediaTypeParser.Builder() .withMediaTypeAndParams("application/smile", SMILE, Collections.emptyMap()) .withMediaTypeAndParams("application/cbor", CBOR, Collections.emptyMap()) - .withMediaTypeAndParams("application/json", JSON, Map.of("charset", "UTF-8")) - .withMediaTypeAndParams("application/yaml", YAML, Map.of("charset", "UTF-8")) - .withMediaTypeAndParams("application/*", JSON, Map.of("charset", "UTF-8")) - .withMediaTypeAndParams("application/x-ndjson", JSON, Map.of("charset", "UTF-8")) - .withMediaTypeAndParams("application/vnd.elasticsearch+json", JSON, Map.of("compatible-with", "\\d+","charset", "UTF-8")) - .withMediaTypeAndParams("application/vnd.elasticsearch+smile", SMILE, Map.of("compatible-with", "\\d+","charset", "UTF-8")) - .withMediaTypeAndParams("application/vnd.elasticsearch+yaml", YAML, Map.of("compatible-with", "\\d+","charset", "UTF-8")) - .withMediaTypeAndParams("application/vnd.elasticsearch+cbor", CBOR, Map.of("compatible-with", "\\d+","charset", "UTF-8")) - .withMediaTypeAndParams("application/vnd.elasticsearch+x-ndjson", JSON, Map.of("compatible-with", "\\d+","charset", "UTF-8")) + .withMediaTypeAndParams("application/json", JSON, Map.of("charset", Pattern.compile("UTF-8"))) + .withMediaTypeAndParams("application/yaml", YAML, Map.of("charset", Pattern.compile("UTF-8"))) + .withMediaTypeAndParams("application/*", JSON, Map.of("charset", Pattern.compile("UTF-8"))) + .withMediaTypeAndParams("application/x-ndjson", JSON, Map.of("charset", Pattern.compile("UTF-8"))) + .withMediaTypeAndParams("application/vnd.elasticsearch+json", JSON, + Map.of("compatible-with", Pattern.compile("\\d+"),"charset", Pattern.compile("UTF-8"))) + .withMediaTypeAndParams("application/vnd.elasticsearch+smile", SMILE, + Map.of("compatible-with", Pattern.compile("\\d+"),"charset", Pattern.compile("UTF-8"))) + .withMediaTypeAndParams("application/vnd.elasticsearch+yaml", YAML, + Map.of("compatible-with", Pattern.compile("\\d+"),"charset", Pattern.compile("UTF-8"))) + .withMediaTypeAndParams("application/vnd.elasticsearch+cbor", CBOR, + Map.of("compatible-with", Pattern.compile("\\d+"),"charset", Pattern.compile("UTF-8"))) + .withMediaTypeAndParams("application/vnd.elasticsearch+x-ndjson", JSON, + Map.of("compatible-with", Pattern.compile("\\d+"),"charset", Pattern.compile("UTF-8"))) .build(); /** 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 76cbb03a4ca53..8303c8e4b31f0 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 @@ -23,6 +23,7 @@ import java.util.Collections; import java.util.Map; +import java.util.regex.Pattern; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; @@ -32,8 +33,8 @@ public class MediaTypeParserTests extends ESTestCase { MediaTypeParser mediaTypeParser = new MediaTypeParser.Builder() .withMediaTypeAndParams("application/vnd.elasticsearch+json", - XContentType.JSON, Map.of("compatible-with", "\\d+", - "charset", "UTF-8")) + XContentType.JSON, Map.of("compatible-with", Pattern.compile("\\d+"), + "charset", Pattern.compile("UTF-8"))) .build(); public void testJsonWithParameters() throws Exception { 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 09da5b6952387..15ef8fa83ecac 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 @@ -28,6 +28,7 @@ import java.util.Map; import java.util.Objects; import java.util.function.Function; +import java.util.regex.Pattern; import static org.elasticsearch.xpack.sql.action.BasicFormatter.FormatOption.TEXT; import static org.elasticsearch.xpack.sql.proto.Protocol.URL_PARAM_DELIMITER; @@ -296,9 +297,12 @@ public String subtype() { private static final String PARAM_HEADER_PRESENT = "present"; private static final MediaTypeParser parser = new MediaTypeParser.Builder() - .withMediaTypeAndParams(PLAIN_TEXT.typeWithSubtype(), PLAIN_TEXT, Map.of("header", "present|absent", "charset", "utf-8")) - .withMediaTypeAndParams(CSV.typeWithSubtype(), CSV, Map.of("header", "present|absent", "charset", "utf-8")) - .withMediaTypeAndParams(TSV.typeWithSubtype(), TSV, Map.of("header", "present|absent", "charset", "utf-8")) + .withMediaTypeAndParams(PLAIN_TEXT.typeWithSubtype(), PLAIN_TEXT, + Map.of("header", Pattern.compile("present|absent"), "charset", Pattern.compile("utf-8"))) + .withMediaTypeAndParams(CSV.typeWithSubtype(), CSV, + Map.of("header", Pattern.compile("present|absent"), "charset", Pattern.compile("utf-8"))) + .withMediaTypeAndParams(TSV.typeWithSubtype(), TSV, + Map.of("header", Pattern.compile("present|absent"), "charset", Pattern.compile("utf-8"))) .build(); String format(RestRequest request, SqlQueryResponse response) { From ff6c94f3e6d483b53415b7a2162c4a9f651fad7c Mon Sep 17 00:00:00 2001 From: pgomulka Date: Tue, 22 Sep 2020 15:49:35 +0200 Subject: [PATCH 16/40] throwing exception when failing to parse --- .../common/xcontent/MediaTypeParser.java | 24 +++-- .../common/xcontent/XContentType.java | 9 +- .../rest/AbstractRestChannel.java | 6 +- .../common/xcontent/XContentTypeTests.java | 26 +++--- .../xpack/sql/plugin/RestSqlQueryAction.java | 74 ++++----------- .../xpack/sql/plugin/SqlMediaTypeParser.java | 75 ++++++++++++++++ .../xpack/sql/plugin/TextFormat.java | 14 +-- .../sql/plugin/SqlMediaTypeParserTests.java | 90 +++++++++++++++++++ .../xpack/sql/plugin/TextFormatTests.java | 36 -------- 9 files changed, 223 insertions(+), 131 deletions(-) create mode 100644 x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java create mode 100644 x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParserTests.java 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 1b737c8549691..e8de57386c57e 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 @@ -73,12 +73,13 @@ public ParsedMediaType parseMediaType(String headerValue) { //spaces are allowed between parameters, but not between '=' sign String[] keyValueParam = split[i].trim().split("="); if (keyValueParam.length != 2 || hasSpaces(keyValueParam[0]) || hasSpaces(keyValueParam[1])) { - return null; + throw new IllegalArgumentException("Spaces are not allowed between parameters. " + headerValue); } String parameterName = keyValueParam[0].toLowerCase(Locale.ROOT); String parameterValue = keyValueParam[1].toLowerCase(Locale.ROOT); if (isValidParameter(typeWithSubtype, parameterName, parameterValue) == false) { - return null; + throw new IllegalArgumentException("unrecognized parameter " + + parameterName + " " + parameterValue + " headerValue=" + headerValue); } parameters.put(parameterName, parameterValue); } @@ -87,7 +88,7 @@ public ParsedMediaType parseMediaType(String headerValue) { } } - return null; + throw new IllegalArgumentException("unrecognized media type: " + headerValue); } private boolean isValidParameter(String typeWithSubtype, String parameterName, String parameterValue) { @@ -127,13 +128,13 @@ public Map getParameters() { } public static class Builder { - private final Map formatMap = new HashMap<>(); - private final Map typeMap = new HashMap<>(); + private final Map formatToMediaType = new HashMap<>(); + private final Map typeWithSubtypeToMediaType = new HashMap<>(); private final Map> parametersMap = new HashMap<>(); public Builder withMediaTypeAndParams(String alternativeMediaType, T mediaType, Map paramNameAndValueRegex) { - typeMap.put(alternativeMediaType.toLowerCase(Locale.ROOT), mediaType); - formatMap.put(mediaType.format(), mediaType); + typeWithSubtypeToMediaType.put(alternativeMediaType.toLowerCase(Locale.ROOT), mediaType); + formatToMediaType.put(mediaType.format(), mediaType); Map parametersForMediaType = new HashMap<>(paramNameAndValueRegex.size()); for (Map.Entry params : paramNameAndValueRegex.entrySet()) { @@ -147,8 +148,15 @@ public Builder withMediaTypeAndParams(String alternativeMediaType, T mediaTyp return this; } + public Builder copyFromMediaTypeParser(MediaTypeParser mediaTypeParser) { + formatToMediaType.putAll(mediaTypeParser.formatToMediaType); + typeWithSubtypeToMediaType.putAll(mediaTypeParser.typeWithSubtypeToMediaType); + parametersMap.putAll(mediaTypeParser.parametersMap); + return this; + } + public MediaTypeParser build() { - return new MediaTypeParser<>(formatMap, typeMap, parametersMap); + return new MediaTypeParser<>(formatToMediaType, typeWithSubtypeToMediaType, parametersMap); } } } 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 431d30c891223..0767eca4d1698 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 @@ -114,13 +114,8 @@ 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.elasticsearch 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 MediaTypeParser mediaTypeParser = new MediaTypeParser.Builder() + + public static final MediaTypeParser mediaTypeParser = new MediaTypeParser.Builder() .withMediaTypeAndParams("application/smile", SMILE, Collections.emptyMap()) .withMediaTypeAndParams("application/cbor", CBOR, Collections.emptyMap()) .withMediaTypeAndParams("application/json", JSON, Map.of("charset", Pattern.compile("UTF-8"))) diff --git a/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java b/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java index c5bf1efa2cdb1..a1274e0a2546b 100644 --- a/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java +++ b/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java @@ -98,9 +98,9 @@ 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) { + if (Strings.hasText(format)) { + responseContentType = XContentType.fromFormat(format); + } else if (Strings.hasText(acceptHeader)) { responseContentType = XContentType.fromMediaType(acceptHeader); } } 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 686bc9fa65405..9e372fe1b300f 100644 --- a/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java +++ b/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java @@ -89,10 +89,10 @@ public void testFromWildcardUppercase() throws Exception { } public void testFromRubbish() throws Exception { - assertThat(XContentType.fromMediaType(null), nullValue()); - assertThat(XContentType.fromMediaType(""), nullValue()); - assertThat(XContentType.fromMediaType("text/plain"), nullValue()); - assertThat(XContentType.fromMediaType("gobbly;goop"), nullValue()); + expectThrows(IllegalArgumentException.class, () -> XContentType.fromMediaType(null)); + expectThrows(IllegalArgumentException.class, () -> XContentType.fromMediaType("")); + expectThrows(IllegalArgumentException.class, () -> XContentType.fromMediaType("text/plain")); + expectThrows(IllegalArgumentException.class, () -> XContentType.fromMediaType("gobbly;goop")); } public void testVersionedMediaType() { @@ -136,24 +136,26 @@ public void testVersionParsing() { assertThat(XContentType.parseVersion("APPLICATION/JSON"), nullValue()); - assertThat(XContentType.parseVersion("application/json;compatible-with=" + version + ".0"), - is(nullValue())); + expectThrows(IllegalArgumentException.class, + () -> XContentType.parseVersion("application/json;compatible-with=" + version + ".0")); } public void testUnrecognizedParameter() { - assertThat(XContentType.parseVersion("application/json; sth=123"), - is(nullValue())); + expectThrows(IllegalArgumentException.class, () -> XContentType.parseVersion("application/json; sth=123")); } public void testMediaTypeWithoutESSubtype() { String version = String.valueOf(Math.abs(randomByte())); - assertThat(XContentType.fromMediaType("application/json;compatible-with=" + version), nullValue()); + expectThrows(IllegalArgumentException.class, () -> XContentType.fromMediaType("application/json;compatible-with=" + version)); } public void testAnchoring() { String version = String.valueOf(Math.abs(randomByte())); - assertThat(XContentType.fromMediaType("sth_application/json;compatible-with=" + version + ".0"), nullValue()); - assertThat(XContentType.fromMediaType("sth_application/json;compatible-with=" + version + "_sth"), nullValue()); - assertThat(XContentType.fromMediaType("application/json;compatible-with=" + version + "_sth"), nullValue()); + expectThrows(IllegalArgumentException.class, + () -> XContentType.fromMediaType("application/json;compatible-with=" + version + ".0")); + expectThrows(IllegalArgumentException.class, + () -> XContentType.fromMediaType("application/json;compatible-with=" + version + "_sth")); + expectThrows(IllegalArgumentException.class, + () -> XContentType.fromMediaType("application/json;compatible-with=" + version + "_sth")); } } 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 4ade958b60e85..aca2b74702cb8 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 @@ -7,6 +7,8 @@ package org.elasticsearch.xpack.sql.plugin; import org.elasticsearch.client.node.NodeClient; +import org.elasticsearch.common.xcontent.MediaType; +import org.elasticsearch.common.xcontent.MediaTypeParser; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentType; @@ -26,7 +28,9 @@ import java.nio.charset.StandardCharsets; import java.util.Collections; import java.util.List; +import java.util.Map; import java.util.Set; +import java.util.regex.Pattern; import static org.elasticsearch.rest.RestRequest.Method.GET; import static org.elasticsearch.rest.RestRequest.Method.POST; @@ -35,7 +39,8 @@ public class RestSqlQueryAction extends BaseRestHandler { - TextFormat textFormat; + private final SqlMediaTypeParser sqlMediaTypeParser = new SqlMediaTypeParser(); + MediaType textFormat; @Override public List routes() { @@ -52,53 +57,7 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli sqlRequest = SqlQueryRequest.fromXContent(parser); } - /* - * Since we support {@link TextFormat} and - * {@link XContent} outputs we can't use {@link RestToXContentListener} - * like everything else. We want to stick as closely as possible to - * Elasticsearch's defaults though, while still layering in ways to - * control the output more easily. - * - * First we find the string that the user used to specify the response - * format. If there is a {@code format} parameter we use that. If there - * 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; - - if (Mode.isDedicatedClient(sqlRequest.requestInfo().mode()) - && (sqlRequest.binaryCommunication() == null || sqlRequest.binaryCommunication())) { - // enforce CBOR response for drivers and CLI (unless instructed differently through the config param) - accept = XContentType.CBOR.name(); - } else { - accept = request.param(URL_PARAM_FORMAT); - } - if (accept == null) { - accept = request.header("Accept"); - if ("*/*".equals(accept)) { - // */* means "I don't care" which we should treat like not specifying the header - accept = null; - } - } - if (accept == null) { - accept = request.header("Content-Type"); - } - assert accept != null : "The Content-Type header is required"; - - /* - * Second, we pick the actual content type to use by first parsing the - * string from the previous step as an {@linkplain XContent} value. If - * that doesn't parse we parse it as a {@linkplain TextFormat} value. If - * that doesn't parse it'll throw an {@link IllegalArgumentException} - * which we turn into a 400 error. - */ - XContentType xContentType = getXContentType(accept); - textFormat = xContentType == null ? TextFormat.fromMediaTypeOrFormat(accept) : null; - - if (xContentType == null && sqlRequest.columnar()) { - throw new IllegalArgumentException("Invalid use of [columnar] argument: cannot be used in combination with " - + "txt, csv or tsv formats"); - } + MediaType responseMediaType = sqlMediaTypeParser.getMediaType(request, sqlRequest); long startNanos = System.nanoTime(); return channel -> client.execute(SqlQueryAction.INSTANCE, sqlRequest, new RestResponseListener(channel) { @@ -107,16 +66,18 @@ public RestResponse buildResponse(SqlQueryResponse response) throws Exception { RestResponse restResponse; // XContent branch - if (xContentType != null) { - XContentBuilder builder = channel.newBuilder(request.getXContentType(), xContentType, true); + if (responseMediaType != null && responseMediaType instanceof XContentType) { + XContentType type = (XContentType)responseMediaType; + XContentBuilder builder = channel.newBuilder(request.getXContentType(), type, true); response.toXContent(builder, request); restResponse = new BytesRestResponse(RestStatus.OK, builder); } // TextFormat else { - final String data = textFormat.format(request, response); + TextFormat type = (TextFormat)textFormat; + final String data = type.format(request, response); - restResponse = new BytesRestResponse(RestStatus.OK, textFormat.contentType(request), + restResponse = new BytesRestResponse(RestStatus.OK, type.contentType(request), data.getBytes(StandardCharsets.UTF_8)); if (response.hasCursor()) { @@ -130,13 +91,8 @@ public RestResponse buildResponse(SqlQueryResponse response) throws Exception { }); } - private XContentType getXContentType(String accept) { - if (accept == null) { - return XContentType.JSON; - } - XContentType xContentType = XContentType.fromFormat(accept); - return xContentType != null ? xContentType : XContentType.fromMediaType(accept); - } + + @Override protected Set responseParams() { diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java new file mode 100644 index 0000000000000..41cbdf1d9918d --- /dev/null +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java @@ -0,0 +1,75 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +package org.elasticsearch.xpack.sql.plugin; + +import org.elasticsearch.common.xcontent.MediaType; +import org.elasticsearch.common.xcontent.MediaTypeParser; +import org.elasticsearch.common.xcontent.XContentType; +import org.elasticsearch.rest.RestRequest; +import org.elasticsearch.xpack.sql.action.SqlQueryRequest; +import org.elasticsearch.xpack.sql.proto.Mode; + +import java.util.Map; +import java.util.regex.Pattern; + +import static org.elasticsearch.xpack.sql.proto.Protocol.URL_PARAM_FORMAT; + +public class SqlMediaTypeParser { + private static final MediaTypeParser parser = new MediaTypeParser.Builder<>() + .copyFromMediaTypeParser(XContentType.mediaTypeParser) + .withMediaTypeAndParams(TextFormat.PLAIN_TEXT.typeWithSubtype(), TextFormat.PLAIN_TEXT, + Map.of("header", Pattern.compile("present|absent"), "charset", Pattern.compile("utf-8"))) + .withMediaTypeAndParams(TextFormat.CSV.typeWithSubtype(), TextFormat.CSV, + Map.of("header", Pattern.compile("present|absent"), "charset", Pattern.compile("utf-8"))) + .withMediaTypeAndParams(TextFormat.TSV.typeWithSubtype(), TextFormat.TSV, + Map.of("header", Pattern.compile("present|absent"), "charset", Pattern.compile("utf-8"))) + .build(); + + /* + * Since we support {@link TextFormat} and + * {@link XContent} outputs we can't use {@link RestToXContentListener} + * like everything else. We want to stick as closely as possible to + * Elasticsearch's defaults though, while still layering in ways to + * control the output more easily. + * + * First we find the string that the user used to specify the response + * format. If there is a {@code format} parameter we use that. If there + * 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. + */ + public MediaType getMediaType(RestRequest request, SqlQueryRequest sqlRequest) { + + if (Mode.isDedicatedClient(sqlRequest.requestInfo().mode()) + && (sqlRequest.binaryCommunication() == null || sqlRequest.binaryCommunication())) { + // enforce CBOR response for drivers and CLI (unless instructed differently through the config param) + return XContentType.CBOR; + } else if (request.hasParam(URL_PARAM_FORMAT)){ + return validateColumnarRequest(sqlRequest.columnar(), parser.fromFormat( request.param(URL_PARAM_FORMAT))); + } + if (request.getHeaders().containsKey("Accept")) { + String accept = request.header("Accept"); + // */* means "I don't care" which we should treat like not specifying the header + if ("*/*".equals(accept) == false) { + return validateColumnarRequest(sqlRequest.columnar(), parser.fromMediaType(accept)); + } + } + + String contentType = request.header("Content-Type"); + assert contentType != null : "The Content-Type header is required"; + return validateColumnarRequest(sqlRequest.columnar(), parser.fromMediaType(contentType)); + } + + private MediaType validateColumnarRequest(boolean requestIsColumnar, MediaType fromMediaType) { + if(requestIsColumnar && fromMediaType instanceof TextFormat){ + throw new IllegalArgumentException("Invalid use of [columnar] argument: cannot be used in combination with " + + "txt, csv or tsv formats"); + } + return fromMediaType; + + } + +} diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/TextFormat.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/TextFormat.java index 15ef8fa83ecac..560f716826843 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 @@ -9,6 +9,7 @@ import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.xcontent.MediaType; import org.elasticsearch.common.xcontent.MediaTypeParser; +import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.rest.RestRequest; import org.elasticsearch.xpack.ql.util.StringUtils; import org.elasticsearch.xpack.sql.SqlIllegalArgumentException; @@ -296,12 +297,13 @@ public String subtype() { private static final String PARAM_HEADER_ABSENT = "absent"; private static final String PARAM_HEADER_PRESENT = "present"; - private static final MediaTypeParser parser = new MediaTypeParser.Builder() - .withMediaTypeAndParams(PLAIN_TEXT.typeWithSubtype(), PLAIN_TEXT, + private static final MediaTypeParser parser = new MediaTypeParser.Builder<>() + .copyFromMediaTypeParser(XContentType.mediaTypeParser) + .withMediaTypeAndParams(TextFormat.PLAIN_TEXT.typeWithSubtype(), TextFormat.PLAIN_TEXT, Map.of("header", Pattern.compile("present|absent"), "charset", Pattern.compile("utf-8"))) - .withMediaTypeAndParams(CSV.typeWithSubtype(), CSV, + .withMediaTypeAndParams(TextFormat.CSV.typeWithSubtype(), TextFormat.CSV, Map.of("header", Pattern.compile("present|absent"), "charset", Pattern.compile("utf-8"))) - .withMediaTypeAndParams(TSV.typeWithSubtype(), TSV, + .withMediaTypeAndParams(TextFormat.TSV.typeWithSubtype(), TextFormat.TSV, Map.of("header", Pattern.compile("present|absent"), "charset", Pattern.compile("utf-8"))) .build(); @@ -325,8 +327,8 @@ boolean hasHeader(RestRequest request) { return true; } - static TextFormat fromMediaTypeOrFormat(String accept) { - TextFormat textFormat = parser.fromFormat(accept); + static MediaType fromMediaTypeOrFormat(String accept) { + MediaType textFormat = parser.fromFormat(accept); if (textFormat != null) { return textFormat; } diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParserTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParserTests.java new file mode 100644 index 0000000000000..ad62e63531c0b --- /dev/null +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParserTests.java @@ -0,0 +1,90 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +package org.elasticsearch.xpack.sql.plugin; + +import org.elasticsearch.common.unit.TimeValue; +import org.elasticsearch.common.xcontent.MediaType; +import org.elasticsearch.common.xcontent.NamedXContentRegistry; +import org.elasticsearch.rest.RestRequest; +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.test.rest.FakeRestRequest; +import org.elasticsearch.xpack.sql.action.SqlQueryRequest; +import org.elasticsearch.xpack.sql.proto.Mode; +import org.elasticsearch.xpack.sql.proto.RequestInfo; + +import java.util.Collections; +import java.util.Map; + +import static org.elasticsearch.xpack.sql.plugin.TextFormat.CSV; +import static org.elasticsearch.xpack.sql.plugin.TextFormat.PLAIN_TEXT; +import static org.elasticsearch.xpack.sql.plugin.TextFormat.TSV; +import static org.elasticsearch.xpack.sql.proto.RequestInfo.CLIENT_IDS; +import static org.hamcrest.CoreMatchers.is; + +public class SqlMediaTypeParserTests extends ESTestCase { + SqlMediaTypeParser parser = new SqlMediaTypeParser(); + + public void testPlainTextDetection() { + MediaType text = parser.getMediaType(reqWithAccept("text/plain"), createTestInstance(false, Mode.PLAIN, false)); + assertThat(text, is(PLAIN_TEXT)); + } + + public void testCsvDetection() { + MediaType text = parser.getMediaType(reqWithAccept("text/csv"), createTestInstance(false, Mode.PLAIN, false)); + assertThat(text, is(CSV)); + } + + public void testTsvDetection() { + MediaType text = parser.getMediaType(reqWithAccept("text/tab-separated-values"), createTestInstance(false, Mode.PLAIN, false)); + assertThat(text, is(TSV)); + } + + public void testParametersParsing() { + assertThat(parser.getMediaType(reqWithAccept("text/plain; charset=utf-8"), + createTestInstance(false, Mode.PLAIN, false)), is(PLAIN_TEXT)); + assertThat(parser.getMediaType(reqWithAccept("text/plain; header=present"), + createTestInstance(false, Mode.PLAIN, false)), is(PLAIN_TEXT)); + assertThat(parser.getMediaType(reqWithAccept("text/plain; charset=utf-8; header=present"), + createTestInstance(false, Mode.PLAIN, false)), is(PLAIN_TEXT)); + + assertThat(parser.getMediaType(reqWithAccept("text/csv; charset=utf-8"), + createTestInstance(false, Mode.PLAIN, false)), is(CSV)); + assertThat(parser.getMediaType(reqWithAccept("text/csv; header=present"), + createTestInstance(false, Mode.PLAIN, false)), is(CSV)); + assertThat(parser.getMediaType(reqWithAccept("text/csv; charset=utf-8; header=present"), + createTestInstance(false, Mode.PLAIN, false)), is(CSV)); + + assertThat(parser.getMediaType(reqWithAccept("text/tab-separated-values; charset=utf-8"), + createTestInstance(false, Mode.PLAIN, false)), is(TSV)); + assertThat(parser.getMediaType(reqWithAccept("text/tab-separated-values; header=present"), + createTestInstance(false, Mode.PLAIN, false)), is(TSV)); + assertThat(parser.getMediaType(reqWithAccept("text/tab-separated-values; charset=utf-8; header=present"), + createTestInstance(false, Mode.PLAIN, false)), is(TSV)); + } + + public void testInvalidFormat() { + Exception e = expectThrows(IllegalArgumentException.class, + () -> parser.getMediaType(reqWithAccept("text/garbage"), createTestInstance(false, Mode.PLAIN, false))); + assertEquals("unrecognized media type: text/garbage", e.getMessage()); + } + + private static RestRequest reqWithAccept(String acceptHeader) { + + return new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY) + .withHeaders(Map.of("Content-Type", Collections.singletonList("application/json"), + "Accept", Collections.singletonList(acceptHeader))) + .build(); + } + + protected SqlQueryRequest createTestInstance(boolean binaryCommunication, Mode mode, boolean columnar) { + return new SqlQueryRequest(randomAlphaOfLength(10), Collections.emptyList(), null, + randomZone(), between(1, Integer.MAX_VALUE), TimeValue.parseTimeValue(randomTimeValue(), null, "test"), + TimeValue.parseTimeValue(randomTimeValue(), null, "test"), columnar, randomAlphaOfLength(10), + new RequestInfo(mode, randomFrom(randomFrom(CLIENT_IDS), randomAlphaOfLengthBetween(10, 20))), + randomBoolean(), randomBoolean()).binaryCommunication(binaryCommunication); + } +} diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/TextFormatTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/TextFormatTests.java index a9cf6c57f34aa..09d916af5cbc2 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/TextFormatTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/TextFormatTests.java @@ -25,46 +25,10 @@ import static java.util.Collections.singletonList; import static java.util.Collections.singletonMap; import static org.elasticsearch.xpack.sql.plugin.TextFormat.CSV; -import static org.elasticsearch.xpack.sql.plugin.TextFormat.PLAIN_TEXT; import static org.elasticsearch.xpack.sql.plugin.TextFormat.TSV; -import static org.hamcrest.CoreMatchers.is; public class TextFormatTests extends ESTestCase { - public void testPlainTextDetection() { - TextFormat text = TextFormat.fromMediaTypeOrFormat("text/plain"); - assertThat(text, is(PLAIN_TEXT)); - } - - public void testCsvDetection() { - TextFormat text = TextFormat.fromMediaTypeOrFormat("text/csv"); - assertThat(text, is(CSV)); - } - - public void testTsvDetection() { - TextFormat text = TextFormat.fromMediaTypeOrFormat("text/tab-separated-values"); - assertThat(text, is(TSV)); - } - - public void testParametersParsing() { - assertThat(TextFormat.fromMediaTypeOrFormat("text/plain; charset=utf-8"), is(PLAIN_TEXT)); - assertThat(TextFormat.fromMediaTypeOrFormat("text/plain; header=present"), is(PLAIN_TEXT)); - assertThat(TextFormat.fromMediaTypeOrFormat("text/plain; charset=utf-8; header=present"), is(PLAIN_TEXT)); - - assertThat(TextFormat.fromMediaTypeOrFormat("text/csv; charset=utf-8"), is(CSV)); - assertThat(TextFormat.fromMediaTypeOrFormat("text/csv; header=present"), is(CSV)); - assertThat(TextFormat.fromMediaTypeOrFormat("text/csv; charset=utf-8; header=present"), is(CSV)); - - assertThat(TextFormat.fromMediaTypeOrFormat("text/tab-separated-values; charset=utf-8"), is(TSV)); - assertThat(TextFormat.fromMediaTypeOrFormat("text/tab-separated-values; header=present"), is(TSV)); - assertThat(TextFormat.fromMediaTypeOrFormat("text/tab-separated-values; charset=utf-8; header=present"), is(TSV)); - } - - public void testInvalidFormat() { - Exception e = expectThrows(IllegalArgumentException.class, () -> TextFormat.fromMediaTypeOrFormat("text/garbage")); - assertEquals("invalid format [text/garbage]", e.getMessage()); - } - public void testCsvContentType() { assertEquals("text/csv; charset=utf-8; header=present", CSV.contentType(req())); } From b969f2bd860b74caff6bbf11b9eb19e3f4651ae9 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Tue, 22 Sep 2020 17:03:06 +0200 Subject: [PATCH 17/40] checkstyle --- .../elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java | 5 ----- 1 file changed, 5 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 aca2b74702cb8..ea46a3db8356c 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java @@ -8,7 +8,6 @@ import org.elasticsearch.client.node.NodeClient; import org.elasticsearch.common.xcontent.MediaType; -import org.elasticsearch.common.xcontent.MediaTypeParser; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentType; @@ -21,21 +20,17 @@ import org.elasticsearch.xpack.sql.action.SqlQueryAction; import org.elasticsearch.xpack.sql.action.SqlQueryRequest; import org.elasticsearch.xpack.sql.action.SqlQueryResponse; -import org.elasticsearch.xpack.sql.proto.Mode; import org.elasticsearch.xpack.sql.proto.Protocol; import java.io.IOException; import java.nio.charset.StandardCharsets; import java.util.Collections; import java.util.List; -import java.util.Map; import java.util.Set; -import java.util.regex.Pattern; import static org.elasticsearch.rest.RestRequest.Method.GET; import static org.elasticsearch.rest.RestRequest.Method.POST; import static org.elasticsearch.xpack.sql.proto.Protocol.URL_PARAM_DELIMITER; -import static org.elasticsearch.xpack.sql.proto.Protocol.URL_PARAM_FORMAT; public class RestSqlQueryAction extends BaseRestHandler { From 0684e584eb812fe76e2eb8427ac0b323b43ff83d Mon Sep 17 00:00:00 2001 From: pgomulka Date: Wed, 23 Sep 2020 11:34:18 +0200 Subject: [PATCH 18/40] test fixes --- .../elasticsearch/client/RestHighLevelClient.java | 6 +++--- .../client/RestHighLevelClientTests.java | 4 ++-- .../common/xcontent/XContentType.java | 2 +- .../common/xcontent/MediaTypeParserTests.java | 15 ++++++--------- .../elasticsearch/rest/action/cat/RestTable.java | 9 ++++++--- .../xpack/watcher/input/http/HttpInputTests.java | 2 ++ 6 files changed, 20 insertions(+), 18 deletions(-) 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 8d1a8e22a4b1c..3cf285bf23a90 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 @@ -1900,9 +1900,9 @@ protected final Resp parseEntity(final HttpEntity entity, throw new IllegalStateException("Elasticsearch didn't return the [Content-Type] header, unable to parse response body"); } XContentType xContentType = XContentType.fromMediaType(entity.getContentType().getValue()); - if (xContentType == null) { - throw new IllegalStateException("Unsupported Content-Type: " + entity.getContentType().getValue()); - } +// if (xContentType == null) { +// throw new IllegalStateException("Unsupported Content-Type: " + entity.getContentType().getValue()); +// } try (XContentParser parser = xContentType.xContent().createParser(registry, DEPRECATION_HANDLER, entity.getContent())) { return entityParser.apply(parser); } diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/RestHighLevelClientTests.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/RestHighLevelClientTests.java index 8df8f7299965e..65a84a0d87dc4 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/RestHighLevelClientTests.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/RestHighLevelClientTests.java @@ -302,8 +302,8 @@ public void testParseEntity() throws IOException { } { NStringEntity entity = new NStringEntity("", ContentType.APPLICATION_SVG_XML); - IllegalStateException ise = expectThrows(IllegalStateException.class, () -> restHighLevelClient.parseEntity(entity, null)); - assertEquals("Unsupported Content-Type: " + entity.getContentType().getValue(), ise.getMessage()); + IllegalArgumentException ise = expectThrows(IllegalArgumentException.class, () -> restHighLevelClient.parseEntity(entity, null)); + assertEquals("unrecognized media type: " + entity.getContentType().getValue(), ise.getMessage()); } { CheckedFunction entityParser = 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 0767eca4d1698..48462a40a20e4 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 @@ -148,7 +148,7 @@ 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 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 + * This method will throw IllegalArgumentException if no match is found */ public static XContentType fromMediaType(String 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 8303c8e4b31f0..7f3bf8571cf51 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 @@ -59,19 +59,16 @@ public void testWhiteSpaceInTypeSubtype() { assertThat(mediaTypeParser.parseMediaType(mediaType + "; compatible-with=123;\n charset=UTF-8").getParameters(), equalTo(Map.of("charset", "utf-8", "compatible-with", "123"))); - mediaType = " application / json "; - assertThat(mediaTypeParser.parseMediaType(mediaType), - is(nullValue())); + String mediaTypeWithWhitespace = " application / json "; + expectThrows(IllegalArgumentException.class , () -> mediaTypeParser.parseMediaType(mediaTypeWithWhitespace)); } public void testInvalidParameters() { String mediaType = "application/vnd.elasticsearch+json"; - assertThat(mediaTypeParser.parseMediaType(mediaType + "; keyvalueNoEqualsSign"), - is(nullValue())); + expectThrows(IllegalArgumentException.class , () -> mediaTypeParser.parseMediaType(mediaType + "; keyvalueNoEqualsSign")); - assertThat(mediaTypeParser.parseMediaType(mediaType + "; key = value"), - is(nullValue())); - assertThat(mediaTypeParser.parseMediaType(mediaType + "; key=") , - is(nullValue())); + expectThrows(IllegalArgumentException.class , () -> mediaTypeParser.parseMediaType(mediaType + "; key = value")); + + expectThrows(IllegalArgumentException.class , () -> mediaTypeParser.parseMediaType(mediaType + "; key=")); } } 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 fa4bd586a4723..b7d2760eeebb4 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/cat/RestTable.java +++ b/server/src/main/java/org/elasticsearch/rest/action/cat/RestTable.java @@ -51,18 +51,21 @@ public class RestTable { public static RestResponse buildResponse(Table table, RestChannel channel) throws Exception { RestRequest request = channel.request(); - XContentType xContentType = getxContentType(request); + XContentType xContentType = getXContentType(request); if (xContentType != null) { return buildXContentBuilder(table, channel); } return buildTextPlainResponse(table, channel); } - private static XContentType getxContentType(RestRequest request) { + private static XContentType getXContentType(RestRequest request) { if (request.hasParam("format")) { return XContentType.fromFormat(request.param("format")); } - return XContentType.fromMediaType(request.header("Accept")); + if (request.header("Accept") != null) { + return XContentType.fromMediaType(request.header("Accept")); + } + return null; } public static RestResponse buildXContentBuilder(Table table, RestChannel channel) throws Exception { diff --git a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/input/http/HttpInputTests.java b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/input/http/HttpInputTests.java index 42788b2ea7ad1..d3587dd7f36c4 100644 --- a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/input/http/HttpInputTests.java +++ b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/input/http/HttpInputTests.java @@ -114,11 +114,13 @@ public void testExecute() throws Exception { } ExecutableHttpInput input = new ExecutableHttpInput(httpInput, httpClient, templateEngine); + //todo pg I am not sure this is actually what will happen now. When a ES will not return a response with unrecognized_content_type when(httpClient.execute(any(HttpRequest.class))).thenReturn(response); when(templateEngine.render(eq(new TextTemplate("_body")), any(Map.class))).thenReturn("_body"); WatchExecutionContext ctx = WatcherTestUtils.createWatchExecutionContext(); HttpInput.Result result = input.execute(ctx, new Payload.Simple()); + //TODO PG this test will fail now when it has `unrecognized_content_type` as we throw an exception. Is this ok? or should we catch a parse exception and somehow return httpinput.result? assertThat(result.type(), equalTo(HttpInput.TYPE)); assertThat(result.payload().data(), hasEntry("key", "value")); } From b91d4721754e7d0111ba1a3731bb0170b52fe8cb Mon Sep 17 00:00:00 2001 From: pgomulka Date: Wed, 23 Sep 2020 14:16:03 +0200 Subject: [PATCH 19/40] */* header --- .../main/java/org/elasticsearch/rest/AbstractRestChannel.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java b/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java index a1274e0a2546b..506de9635383d 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) { if (Strings.hasText(format)) { responseContentType = XContentType.fromFormat(format); - } else if (Strings.hasText(acceptHeader)) { + } else if (Strings.hasText(acceptHeader) && acceptHeader.equals("*/*") == false) { //TODO PG previously we would just null for */*. where is that set?? responseContentType = XContentType.fromMediaType(acceptHeader); } } From 9edc49f64ad91799628068bd80abc8706ebc86c4 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Wed, 23 Sep 2020 14:43:58 +0200 Subject: [PATCH 20/40] catch --- .../org/elasticsearch/rest/AbstractRestChannel.java | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java b/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java index 506de9635383d..a57bdb76b3309 100644 --- a/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java +++ b/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java @@ -18,6 +18,8 @@ */ package org.elasticsearch.rest; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.Streams; @@ -36,6 +38,7 @@ public abstract class AbstractRestChannel implements RestChannel { + private static final Logger logger = LogManager.getLogger(AbstractRestChannel.class); private static final Predicate INCLUDE_FILTER = f -> f.charAt(0) != '-'; private static final Predicate EXCLUDE_FILTER = INCLUDE_FILTER.negate(); @@ -101,7 +104,12 @@ public XContentBuilder newBuilder(@Nullable XContentType requestContentType, @Nu if (Strings.hasText(format)) { responseContentType = XContentType.fromFormat(format); } else if (Strings.hasText(acceptHeader) && acceptHeader.equals("*/*") == false) { //TODO PG previously we would just null for */*. where is that set?? - responseContentType = XContentType.fromMediaType(acceptHeader); + try { + responseContentType = XContentType.fromMediaType(acceptHeader); + }catch (IllegalArgumentException e){ + //todo pg this is in a way controlling the flow by exceptions. to be discussed + logger.warn("Unrecognized accept header",e); + } } } // try to determine the response content type from the media type or the format query string parameter, with the format parameter From c5d816693e18003a9b78246cb3ed2e3efc0be3be Mon Sep 17 00:00:00 2001 From: pgomulka Date: Wed, 23 Sep 2020 19:48:43 +0200 Subject: [PATCH 21/40] hacks around text/plain --- .../org/elasticsearch/gradle/http/WaitForHttpResource.java | 2 ++ .../src/main/java/org/elasticsearch/client/RestClient.java | 3 +++ .../main/java/org/elasticsearch/rest/AbstractRestChannel.java | 3 ++- .../main/java/org/elasticsearch/rest/BytesRestResponse.java | 1 + .../elasticsearch/test/rest/yaml/ClientYamlTestResponse.java | 3 ++- 5 files changed, 10 insertions(+), 2 deletions(-) diff --git a/buildSrc/src/main/java/org/elasticsearch/gradle/http/WaitForHttpResource.java b/buildSrc/src/main/java/org/elasticsearch/gradle/http/WaitForHttpResource.java index db805b26cdca5..c48149df2eae2 100644 --- a/buildSrc/src/main/java/org/elasticsearch/gradle/http/WaitForHttpResource.java +++ b/buildSrc/src/main/java/org/elasticsearch/gradle/http/WaitForHttpResource.java @@ -145,6 +145,8 @@ HttpURLConnection buildConnection(SSLContext ssl) throws IOException { configureSslContext(connection, ssl); configureBasicAuth(connection); connection.setRequestMethod("GET"); + connection.setRequestProperty("Accept", "text/plain; charset=UTF-8"); + // retConnection.setRequestProperty(Constants.HeaderConstants.ACCEPT, Constants.HeaderConstants.XML_TYPE); return connection; } diff --git a/client/rest/src/main/java/org/elasticsearch/client/RestClient.java b/client/rest/src/main/java/org/elasticsearch/client/RestClient.java index ec9f98d1a7115..15006f8fa018c 100644 --- a/client/rest/src/main/java/org/elasticsearch/client/RestClient.java +++ b/client/rest/src/main/java/org/elasticsearch/client/RestClient.java @@ -762,6 +762,9 @@ private void setRequestConfig(HttpRequestBase httpRequest, RequestConfig request RequestContext createContextForNextAttempt(Node node, AuthCache authCache) { this.httpRequest.reset(); +// httpRequest.addHeader("Accept","application/json"); + System.out.println(Arrays.toString(httpRequest.getAllHeaders())); + return new RequestContext(this, node, authCache); } } diff --git a/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java b/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java index a57bdb76b3309..ed5ac389a2c5d 100644 --- a/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java +++ b/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java @@ -71,6 +71,7 @@ protected AbstractRestChannel(RestRequest request, boolean detailedErrorsEnabled @Override public XContentBuilder newBuilder() throws IOException { + System.out.println(request.path()+" "+request.getHeaders().keySet().toString() +" "+request.getHeaders().values().toString()); return newBuilder(request.getXContentType(), true); } @@ -103,7 +104,7 @@ public XContentBuilder newBuilder(@Nullable XContentType requestContentType, @Nu if (responseContentType == null) { if (Strings.hasText(format)) { responseContentType = XContentType.fromFormat(format); - } else if (Strings.hasText(acceptHeader) && acceptHeader.equals("*/*") == false) { //TODO PG previously we would just null for */*. where is that set?? + } else if (Strings.hasText(acceptHeader) && acceptHeader.equals("*/*") == false && acceptHeader.startsWith("text/plain")==false) { //TODO PG previously we would just null for */*. where is that set?? try { responseContentType = XContentType.fromMediaType(acceptHeader); }catch (IllegalArgumentException e){ diff --git a/server/src/main/java/org/elasticsearch/rest/BytesRestResponse.java b/server/src/main/java/org/elasticsearch/rest/BytesRestResponse.java index 3babe43474266..91f3c1e5d3b6e 100644 --- a/server/src/main/java/org/elasticsearch/rest/BytesRestResponse.java +++ b/server/src/main/java/org/elasticsearch/rest/BytesRestResponse.java @@ -83,6 +83,7 @@ public BytesRestResponse(RestStatus status, String contentType, byte[] content) /** * Creates a binary response. */ + //todo pg this should probably be acceptType? (2nd param) public BytesRestResponse(RestStatus status, String contentType, BytesReference content) { this.status = status; this.content = content; diff --git a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestResponse.java b/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestResponse.java index 9f5d0dfe9dcd7..a50deb0cbb409 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,8 @@ public ClientYamlTestResponse(Response response) throws IOException { this.response = response; if (response.getEntity() != null) { String contentType = response.getHeader("Content-Type"); - this.bodyContentType = XContentType.fromMediaType(contentType); + //todo pg it feels like there is a lot of 'ifs' around text being returned. shouldn't we treat text as the same json and others? + this.bodyContentType = contentType.startsWith("text/plain")? null : XContentType.fromMediaType(contentType); try { byte[] bytes = EntityUtils.toByteArray(response.getEntity()); //skip parsing if we got text back (e.g. if we called _cat apis) From ad673b64ddd1a2bbe01823084515af5706d46c4e Mon Sep 17 00:00:00 2001 From: pgomulka Date: Wed, 23 Sep 2020 19:53:13 +0200 Subject: [PATCH 22/40] sout remove --- .../src/main/java/org/elasticsearch/client/RestClient.java | 3 --- .../main/java/org/elasticsearch/rest/AbstractRestChannel.java | 1 - 2 files changed, 4 deletions(-) diff --git a/client/rest/src/main/java/org/elasticsearch/client/RestClient.java b/client/rest/src/main/java/org/elasticsearch/client/RestClient.java index 15006f8fa018c..ec9f98d1a7115 100644 --- a/client/rest/src/main/java/org/elasticsearch/client/RestClient.java +++ b/client/rest/src/main/java/org/elasticsearch/client/RestClient.java @@ -762,9 +762,6 @@ private void setRequestConfig(HttpRequestBase httpRequest, RequestConfig request RequestContext createContextForNextAttempt(Node node, AuthCache authCache) { this.httpRequest.reset(); -// httpRequest.addHeader("Accept","application/json"); - System.out.println(Arrays.toString(httpRequest.getAllHeaders())); - return new RequestContext(this, node, authCache); } } diff --git a/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java b/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java index ed5ac389a2c5d..347194e535d42 100644 --- a/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java +++ b/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java @@ -71,7 +71,6 @@ protected AbstractRestChannel(RestRequest request, boolean detailedErrorsEnabled @Override public XContentBuilder newBuilder() throws IOException { - System.out.println(request.path()+" "+request.getHeaders().keySet().toString() +" "+request.getHeaders().values().toString()); return newBuilder(request.getXContentType(), true); } From 1ae6a609a9b6ed0826ac5bb1847f431e5e56190e Mon Sep 17 00:00:00 2001 From: pgomulka Date: Wed, 23 Sep 2020 20:12:03 +0200 Subject: [PATCH 23/40] precommit --- .../org/elasticsearch/gradle/http/WaitForHttpResource.java | 1 - .../org/elasticsearch/client/RestHighLevelClientTests.java | 3 ++- .../elasticsearch/common/xcontent/MediaTypeParserTests.java | 2 -- .../main/java/org/elasticsearch/rest/AbstractRestChannel.java | 3 ++- .../org/elasticsearch/common/xcontent/XContentTypeTests.java | 1 - .../elasticsearch/xpack/watcher/input/http/HttpInputTests.java | 3 ++- 6 files changed, 6 insertions(+), 7 deletions(-) diff --git a/buildSrc/src/main/java/org/elasticsearch/gradle/http/WaitForHttpResource.java b/buildSrc/src/main/java/org/elasticsearch/gradle/http/WaitForHttpResource.java index c48149df2eae2..9aef2f6c98c41 100644 --- a/buildSrc/src/main/java/org/elasticsearch/gradle/http/WaitForHttpResource.java +++ b/buildSrc/src/main/java/org/elasticsearch/gradle/http/WaitForHttpResource.java @@ -146,7 +146,6 @@ HttpURLConnection buildConnection(SSLContext ssl) throws IOException { configureBasicAuth(connection); connection.setRequestMethod("GET"); connection.setRequestProperty("Accept", "text/plain; charset=UTF-8"); - // retConnection.setRequestProperty(Constants.HeaderConstants.ACCEPT, Constants.HeaderConstants.XML_TYPE); return connection; } diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/RestHighLevelClientTests.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/RestHighLevelClientTests.java index 65a84a0d87dc4..0f302b3e0592f 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/RestHighLevelClientTests.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/RestHighLevelClientTests.java @@ -302,7 +302,8 @@ public void testParseEntity() throws IOException { } { NStringEntity entity = new NStringEntity("", ContentType.APPLICATION_SVG_XML); - IllegalArgumentException ise = expectThrows(IllegalArgumentException.class, () -> restHighLevelClient.parseEntity(entity, null)); + IllegalArgumentException ise = expectThrows(IllegalArgumentException.class, + () -> restHighLevelClient.parseEntity(entity, null)); assertEquals("unrecognized media type: " + entity.getContentType().getValue(), ise.getMessage()); } { 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 7f3bf8571cf51..0173f2bc9122c 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,8 +26,6 @@ import java.util.regex.Pattern; import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.is; -import static org.hamcrest.Matchers.nullValue; public class MediaTypeParserTests extends ESTestCase { diff --git a/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java b/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java index 347194e535d42..4082e6edb028e 100644 --- a/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java +++ b/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java @@ -103,7 +103,8 @@ public XContentBuilder newBuilder(@Nullable XContentType requestContentType, @Nu if (responseContentType == null) { if (Strings.hasText(format)) { responseContentType = XContentType.fromFormat(format); - } else if (Strings.hasText(acceptHeader) && acceptHeader.equals("*/*") == false && acceptHeader.startsWith("text/plain")==false) { //TODO PG previously we would just null for */*. where is that set?? + } else if (Strings.hasText(acceptHeader) && acceptHeader.equals("*/*") == false + && acceptHeader.startsWith("text/plain")==false) { //TODO PG previously we would just null for */*. where is that set?? try { responseContentType = XContentType.fromMediaType(acceptHeader); }catch (IllegalArgumentException e){ 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 9e372fe1b300f..9e5c162ee2bda 100644 --- a/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java +++ b/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java @@ -23,7 +23,6 @@ import java.util.Locale; import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.nullValue; public class XContentTypeTests extends ESTestCase { diff --git a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/input/http/HttpInputTests.java b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/input/http/HttpInputTests.java index d3587dd7f36c4..59eea96c4c7f1 100644 --- a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/input/http/HttpInputTests.java +++ b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/input/http/HttpInputTests.java @@ -120,7 +120,8 @@ public void testExecute() throws Exception { WatchExecutionContext ctx = WatcherTestUtils.createWatchExecutionContext(); HttpInput.Result result = input.execute(ctx, new Payload.Simple()); - //TODO PG this test will fail now when it has `unrecognized_content_type` as we throw an exception. Is this ok? or should we catch a parse exception and somehow return httpinput.result? + //TODO PG this test will fail now when it has `unrecognized_content_type` as we throw an exception. + // Is this ok? or should we catch a parse exception and somehow return httpinput.result? assertThat(result.type(), equalTo(HttpInput.TYPE)); assertThat(result.payload().data(), hasEntry("key", "value")); } From 1ae63c69122b0deec888afa0f236cfb579472eeb Mon Sep 17 00:00:00 2001 From: pgomulka Date: Thu, 24 Sep 2020 08:28:56 +0200 Subject: [PATCH 24/40] fix nullpointer --- .../xpack/sql/plugin/RestSqlQueryAction.java | 10 ++++----- .../xpack/sql/plugin/TextFormat.java | 22 ------------------- 2 files changed, 4 insertions(+), 28 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 ea46a3db8356c..133d03d1b5bb2 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 @@ -35,7 +35,7 @@ public class RestSqlQueryAction extends BaseRestHandler { private final SqlMediaTypeParser sqlMediaTypeParser = new SqlMediaTypeParser(); - MediaType textFormat; + MediaType responseMediaType; @Override public List routes() { @@ -66,10 +66,8 @@ public RestResponse buildResponse(SqlQueryResponse response) throws Exception { XContentBuilder builder = channel.newBuilder(request.getXContentType(), type, true); response.toXContent(builder, request); restResponse = new BytesRestResponse(RestStatus.OK, builder); - } - // TextFormat - else { - TextFormat type = (TextFormat)textFormat; + } else {// TextFormat + TextFormat type = (TextFormat)responseMediaType; final String data = type.format(request, response); restResponse = new BytesRestResponse(RestStatus.OK, type.contentType(request), @@ -91,7 +89,7 @@ public RestResponse buildResponse(SqlQueryResponse response) throws Exception { @Override protected Set responseParams() { - return textFormat == TextFormat.CSV ? Collections.singleton(URL_PARAM_DELIMITER) : Collections.emptySet(); + return responseMediaType == TextFormat.CSV ? Collections.singleton(URL_PARAM_DELIMITER) : Collections.emptySet(); } @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 560f716826843..3201c5fd7b188 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 @@ -297,16 +297,6 @@ public String subtype() { private static final String PARAM_HEADER_ABSENT = "absent"; private static final String PARAM_HEADER_PRESENT = "present"; - private static final MediaTypeParser parser = new MediaTypeParser.Builder<>() - .copyFromMediaTypeParser(XContentType.mediaTypeParser) - .withMediaTypeAndParams(TextFormat.PLAIN_TEXT.typeWithSubtype(), TextFormat.PLAIN_TEXT, - Map.of("header", Pattern.compile("present|absent"), "charset", Pattern.compile("utf-8"))) - .withMediaTypeAndParams(TextFormat.CSV.typeWithSubtype(), TextFormat.CSV, - Map.of("header", Pattern.compile("present|absent"), "charset", Pattern.compile("utf-8"))) - .withMediaTypeAndParams(TextFormat.TSV.typeWithSubtype(), TextFormat.TSV, - Map.of("header", Pattern.compile("present|absent"), "charset", Pattern.compile("utf-8"))) - .build(); - String format(RestRequest request, SqlQueryResponse response) { StringBuilder sb = new StringBuilder(); @@ -327,18 +317,6 @@ boolean hasHeader(RestRequest request) { return true; } - static MediaType fromMediaTypeOrFormat(String accept) { - MediaType textFormat = parser.fromFormat(accept); - if (textFormat != null) { - return textFormat; - } - textFormat = parser.fromMediaType(accept); - if (textFormat != null) { - return textFormat; - } - throw new IllegalArgumentException("invalid format [" + accept + "]"); - } - /** * Formal IANA mime type. */ From ee1e5a0459d7228c53369928516d0e9acb1e2e2f Mon Sep 17 00:00:00 2001 From: pgomulka Date: Thu, 24 Sep 2020 09:23:53 +0200 Subject: [PATCH 25/40] fix yml test --- .../test/rest/yaml/ClientYamlTestResponse.java | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) 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 a50deb0cbb409..0b8bf9367b8fa 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 @@ -45,7 +45,7 @@ public class ClientYamlTestResponse { private final Response response; private final byte[] body; - private final XContentType bodyContentType; + private XContentType bodyContentType; private ObjectPath parsedResponse; private String bodyAsString; @@ -53,8 +53,13 @@ public ClientYamlTestResponse(Response response) throws IOException { this.response = response; if (response.getEntity() != null) { String contentType = response.getHeader("Content-Type"); - //todo pg it feels like there is a lot of 'ifs' around text being returned. shouldn't we treat text as the same json and others? - this.bodyContentType = contentType.startsWith("text/plain")? null : XContentType.fromMediaType(contentType); + // todo pg it feels like there is a lot of 'ifs' around text, csv etc being returned. shouldn't we treat text as the same json and others? + // we are testing sql formats with this client, but don't have access to sql parsing code + try{ + this.bodyContentType = XContentType.fromMediaType(contentType); + }catch (IllegalArgumentException e){ + this.bodyContentType = null; + } try { byte[] bytes = EntityUtils.toByteArray(response.getEntity()); //skip parsing if we got text back (e.g. if we called _cat apis) From 9f4b6c38a5869c22b8354e59bf0f7a7aeb1e69f2 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Thu, 24 Sep 2020 09:51:37 +0200 Subject: [PATCH 26/40] fix yml test --- .../java/org/elasticsearch/rest/AbstractRestChannel.java | 5 +++-- .../java/org/elasticsearch/rest/action/cat/RestTable.java | 2 +- .../java/org/elasticsearch/rest/RestControllerTests.java | 1 + .../elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java | 2 ++ 4 files changed, 7 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 4082e6edb028e..eaf0fd301d8d2 100644 --- a/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java +++ b/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java @@ -104,12 +104,13 @@ public XContentBuilder newBuilder(@Nullable XContentType requestContentType, @Nu if (Strings.hasText(format)) { responseContentType = XContentType.fromFormat(format); } else if (Strings.hasText(acceptHeader) && acceptHeader.equals("*/*") == false - && acceptHeader.startsWith("text/plain")==false) { //TODO PG previously we would just null for */*. where is that set?? + && acceptHeader.startsWith("text/plain")==false) { //TODO PG there are a lot of usages of text/plain as a response type. + // I feel it would fit into xcontenttype.. try { responseContentType = XContentType.fromMediaType(acceptHeader); }catch (IllegalArgumentException e){ //todo pg this is in a way controlling the flow by exceptions. to be discussed - logger.warn("Unrecognized accept header",e); + logger.debug("Unrecognized accept header",e); } } } 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 b7d2760eeebb4..c68b59050966a 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/cat/RestTable.java +++ b/server/src/main/java/org/elasticsearch/rest/action/cat/RestTable.java @@ -62,7 +62,7 @@ private static XContentType getXContentType(RestRequest request) { if (request.hasParam("format")) { return XContentType.fromFormat(request.param("format")); } - if (request.header("Accept") != null) { + if (request.header("Accept") != null && request.header("Accept").startsWith("text/plain") == false) { return XContentType.fromMediaType(request.header("Accept")); } return null; diff --git a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java index 8c8e83e5bfe42..df859084eea3a 100644 --- a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java +++ b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java @@ -321,6 +321,7 @@ public void testDispatchDoesNotRequireContentTypeForRequestsWithoutContent() { } public void testDispatchFailsWithPlainText() { + //todo pg now the exception is thrown earlier when request is created in AbstractHttpServerTransport. possibly move these tests there? String content = randomAlphaOfLength((int) Math.round(BREAKER_LIMIT.getBytes() / inFlightRequestsBreaker.getOverhead())); FakeRestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY) .withContent(new BytesArray(content), null).withPath("/foo") diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java index 41cbdf1d9918d..e784d0cff49b9 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java @@ -58,6 +58,8 @@ public MediaType getMediaType(RestRequest request, SqlQueryRequest sqlRequest) { } } + //todo pg we should probably remove this possibility. it should only be accept or format. + // also if you specify sql's textformat in content type it will fail with exception now String contentType = request.header("Content-Type"); assert contentType != null : "The Content-Type header is required"; return validateColumnarRequest(sqlRequest.columnar(), parser.fromMediaType(contentType)); From bd4dee1126974d0cf66d63eaf62564b8c3d9f032 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Tue, 22 Sep 2020 15:49:35 +0200 Subject: [PATCH 27/40] throwing exception when failing to parse checkstyle test fixes */* header catch hacks around text/plain sout remove precommit fix nullpointer fix yml test fix yml test --- .../gradle/http/WaitForHttpResource.java | 1 + .../client/RestHighLevelClient.java | 6 +- .../client/RestHighLevelClientTests.java | 5 +- .../common/xcontent/MediaTypeParser.java | 24 +++-- .../common/xcontent/XContentType.java | 11 +-- .../common/xcontent/MediaTypeParserTests.java | 17 ++-- .../rest/AbstractRestChannel.java | 18 +++- .../elasticsearch/rest/BytesRestResponse.java | 1 + .../rest/action/cat/RestTable.java | 9 +- .../common/xcontent/XContentTypeTests.java | 27 +++--- .../rest/RestControllerTests.java | 1 + .../rest/yaml/ClientYamlTestResponse.java | 10 ++- .../xpack/sql/plugin/RestSqlQueryAction.java | 79 +++------------- .../xpack/sql/plugin/SqlMediaTypeParser.java | 77 ++++++++++++++++ .../xpack/sql/plugin/TextFormat.java | 22 +---- .../sql/plugin/SqlMediaTypeParserTests.java | 90 +++++++++++++++++++ .../xpack/sql/plugin/TextFormatTests.java | 36 -------- .../watcher/input/http/HttpInputTests.java | 3 + 18 files changed, 261 insertions(+), 176 deletions(-) create mode 100644 x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java create mode 100644 x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParserTests.java diff --git a/buildSrc/src/main/java/org/elasticsearch/gradle/http/WaitForHttpResource.java b/buildSrc/src/main/java/org/elasticsearch/gradle/http/WaitForHttpResource.java index db805b26cdca5..9aef2f6c98c41 100644 --- a/buildSrc/src/main/java/org/elasticsearch/gradle/http/WaitForHttpResource.java +++ b/buildSrc/src/main/java/org/elasticsearch/gradle/http/WaitForHttpResource.java @@ -145,6 +145,7 @@ HttpURLConnection buildConnection(SSLContext ssl) throws IOException { configureSslContext(connection, ssl); configureBasicAuth(connection); connection.setRequestMethod("GET"); + connection.setRequestProperty("Accept", "text/plain; charset=UTF-8"); return connection; } 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 8d1a8e22a4b1c..3cf285bf23a90 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 @@ -1900,9 +1900,9 @@ protected final Resp parseEntity(final HttpEntity entity, throw new IllegalStateException("Elasticsearch didn't return the [Content-Type] header, unable to parse response body"); } XContentType xContentType = XContentType.fromMediaType(entity.getContentType().getValue()); - if (xContentType == null) { - throw new IllegalStateException("Unsupported Content-Type: " + entity.getContentType().getValue()); - } +// if (xContentType == null) { +// throw new IllegalStateException("Unsupported Content-Type: " + entity.getContentType().getValue()); +// } try (XContentParser parser = xContentType.xContent().createParser(registry, DEPRECATION_HANDLER, entity.getContent())) { return entityParser.apply(parser); } diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/RestHighLevelClientTests.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/RestHighLevelClientTests.java index 8df8f7299965e..0f302b3e0592f 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/RestHighLevelClientTests.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/RestHighLevelClientTests.java @@ -302,8 +302,9 @@ public void testParseEntity() throws IOException { } { NStringEntity entity = new NStringEntity("", ContentType.APPLICATION_SVG_XML); - IllegalStateException ise = expectThrows(IllegalStateException.class, () -> restHighLevelClient.parseEntity(entity, null)); - assertEquals("Unsupported Content-Type: " + entity.getContentType().getValue(), ise.getMessage()); + IllegalArgumentException ise = expectThrows(IllegalArgumentException.class, + () -> restHighLevelClient.parseEntity(entity, null)); + assertEquals("unrecognized media type: " + entity.getContentType().getValue(), ise.getMessage()); } { CheckedFunction entityParser = parser -> { 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 1b737c8549691..e8de57386c57e 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 @@ -73,12 +73,13 @@ public ParsedMediaType parseMediaType(String headerValue) { //spaces are allowed between parameters, but not between '=' sign String[] keyValueParam = split[i].trim().split("="); if (keyValueParam.length != 2 || hasSpaces(keyValueParam[0]) || hasSpaces(keyValueParam[1])) { - return null; + throw new IllegalArgumentException("Spaces are not allowed between parameters. " + headerValue); } String parameterName = keyValueParam[0].toLowerCase(Locale.ROOT); String parameterValue = keyValueParam[1].toLowerCase(Locale.ROOT); if (isValidParameter(typeWithSubtype, parameterName, parameterValue) == false) { - return null; + throw new IllegalArgumentException("unrecognized parameter " + + parameterName + " " + parameterValue + " headerValue=" + headerValue); } parameters.put(parameterName, parameterValue); } @@ -87,7 +88,7 @@ public ParsedMediaType parseMediaType(String headerValue) { } } - return null; + throw new IllegalArgumentException("unrecognized media type: " + headerValue); } private boolean isValidParameter(String typeWithSubtype, String parameterName, String parameterValue) { @@ -127,13 +128,13 @@ public Map getParameters() { } public static class Builder { - private final Map formatMap = new HashMap<>(); - private final Map typeMap = new HashMap<>(); + private final Map formatToMediaType = new HashMap<>(); + private final Map typeWithSubtypeToMediaType = new HashMap<>(); private final Map> parametersMap = new HashMap<>(); public Builder withMediaTypeAndParams(String alternativeMediaType, T mediaType, Map paramNameAndValueRegex) { - typeMap.put(alternativeMediaType.toLowerCase(Locale.ROOT), mediaType); - formatMap.put(mediaType.format(), mediaType); + typeWithSubtypeToMediaType.put(alternativeMediaType.toLowerCase(Locale.ROOT), mediaType); + formatToMediaType.put(mediaType.format(), mediaType); Map parametersForMediaType = new HashMap<>(paramNameAndValueRegex.size()); for (Map.Entry params : paramNameAndValueRegex.entrySet()) { @@ -147,8 +148,15 @@ public Builder withMediaTypeAndParams(String alternativeMediaType, T mediaTyp return this; } + public Builder copyFromMediaTypeParser(MediaTypeParser mediaTypeParser) { + formatToMediaType.putAll(mediaTypeParser.formatToMediaType); + typeWithSubtypeToMediaType.putAll(mediaTypeParser.typeWithSubtypeToMediaType); + parametersMap.putAll(mediaTypeParser.parametersMap); + return this; + } + public MediaTypeParser build() { - return new MediaTypeParser<>(formatMap, typeMap, parametersMap); + return new MediaTypeParser<>(formatToMediaType, typeWithSubtypeToMediaType, parametersMap); } } } 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 431d30c891223..48462a40a20e4 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 @@ -114,13 +114,8 @@ 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.elasticsearch 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 MediaTypeParser mediaTypeParser = new MediaTypeParser.Builder() + + public static final MediaTypeParser mediaTypeParser = new MediaTypeParser.Builder() .withMediaTypeAndParams("application/smile", SMILE, Collections.emptyMap()) .withMediaTypeAndParams("application/cbor", CBOR, Collections.emptyMap()) .withMediaTypeAndParams("application/json", JSON, Map.of("charset", Pattern.compile("UTF-8"))) @@ -153,7 +148,7 @@ 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 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 + * This method will throw IllegalArgumentException if no match is found */ public static XContentType fromMediaType(String 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 8303c8e4b31f0..0173f2bc9122c 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,8 +26,6 @@ import java.util.regex.Pattern; import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.is; -import static org.hamcrest.Matchers.nullValue; public class MediaTypeParserTests extends ESTestCase { @@ -59,19 +57,16 @@ public void testWhiteSpaceInTypeSubtype() { assertThat(mediaTypeParser.parseMediaType(mediaType + "; compatible-with=123;\n charset=UTF-8").getParameters(), equalTo(Map.of("charset", "utf-8", "compatible-with", "123"))); - mediaType = " application / json "; - assertThat(mediaTypeParser.parseMediaType(mediaType), - is(nullValue())); + String mediaTypeWithWhitespace = " application / json "; + expectThrows(IllegalArgumentException.class , () -> mediaTypeParser.parseMediaType(mediaTypeWithWhitespace)); } public void testInvalidParameters() { String mediaType = "application/vnd.elasticsearch+json"; - assertThat(mediaTypeParser.parseMediaType(mediaType + "; keyvalueNoEqualsSign"), - is(nullValue())); + expectThrows(IllegalArgumentException.class , () -> mediaTypeParser.parseMediaType(mediaType + "; keyvalueNoEqualsSign")); - assertThat(mediaTypeParser.parseMediaType(mediaType + "; key = value"), - is(nullValue())); - assertThat(mediaTypeParser.parseMediaType(mediaType + "; key=") , - is(nullValue())); + expectThrows(IllegalArgumentException.class , () -> mediaTypeParser.parseMediaType(mediaType + "; key = value")); + + expectThrows(IllegalArgumentException.class , () -> mediaTypeParser.parseMediaType(mediaType + "; key=")); } } diff --git a/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java b/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java index c5bf1efa2cdb1..eaf0fd301d8d2 100644 --- a/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java +++ b/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java @@ -18,6 +18,8 @@ */ package org.elasticsearch.rest; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.Streams; @@ -36,6 +38,7 @@ public abstract class AbstractRestChannel implements RestChannel { + private static final Logger logger = LogManager.getLogger(AbstractRestChannel.class); private static final Predicate INCLUDE_FILTER = f -> f.charAt(0) != '-'; private static final Predicate EXCLUDE_FILTER = INCLUDE_FILTER.negate(); @@ -98,10 +101,17 @@ 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); + if (Strings.hasText(format)) { + responseContentType = XContentType.fromFormat(format); + } else if (Strings.hasText(acceptHeader) && acceptHeader.equals("*/*") == false + && acceptHeader.startsWith("text/plain")==false) { //TODO PG there are a lot of usages of text/plain as a response type. + // I feel it would fit into xcontenttype.. + try { + responseContentType = XContentType.fromMediaType(acceptHeader); + }catch (IllegalArgumentException e){ + //todo pg this is in a way controlling the flow by exceptions. to be discussed + logger.debug("Unrecognized accept header",e); + } } } // try to determine the response content type from the media type or the format query string parameter, with the format parameter diff --git a/server/src/main/java/org/elasticsearch/rest/BytesRestResponse.java b/server/src/main/java/org/elasticsearch/rest/BytesRestResponse.java index 3babe43474266..91f3c1e5d3b6e 100644 --- a/server/src/main/java/org/elasticsearch/rest/BytesRestResponse.java +++ b/server/src/main/java/org/elasticsearch/rest/BytesRestResponse.java @@ -83,6 +83,7 @@ public BytesRestResponse(RestStatus status, String contentType, byte[] content) /** * Creates a binary response. */ + //todo pg this should probably be acceptType? (2nd param) public BytesRestResponse(RestStatus status, String contentType, BytesReference content) { this.status = status; this.content = content; 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 fa4bd586a4723..c68b59050966a 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/cat/RestTable.java +++ b/server/src/main/java/org/elasticsearch/rest/action/cat/RestTable.java @@ -51,18 +51,21 @@ public class RestTable { public static RestResponse buildResponse(Table table, RestChannel channel) throws Exception { RestRequest request = channel.request(); - XContentType xContentType = getxContentType(request); + XContentType xContentType = getXContentType(request); if (xContentType != null) { return buildXContentBuilder(table, channel); } return buildTextPlainResponse(table, channel); } - private static XContentType getxContentType(RestRequest request) { + private static XContentType getXContentType(RestRequest request) { if (request.hasParam("format")) { return XContentType.fromFormat(request.param("format")); } - return XContentType.fromMediaType(request.header("Accept")); + if (request.header("Accept") != null && request.header("Accept").startsWith("text/plain") == false) { + return XContentType.fromMediaType(request.header("Accept")); + } + return null; } public static RestResponse buildXContentBuilder(Table table, RestChannel channel) throws Exception { diff --git a/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java b/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java index 686bc9fa65405..9e5c162ee2bda 100644 --- a/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java +++ b/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java @@ -23,7 +23,6 @@ import java.util.Locale; import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.nullValue; public class XContentTypeTests extends ESTestCase { @@ -89,10 +88,10 @@ public void testFromWildcardUppercase() throws Exception { } public void testFromRubbish() throws Exception { - assertThat(XContentType.fromMediaType(null), nullValue()); - assertThat(XContentType.fromMediaType(""), nullValue()); - assertThat(XContentType.fromMediaType("text/plain"), nullValue()); - assertThat(XContentType.fromMediaType("gobbly;goop"), nullValue()); + expectThrows(IllegalArgumentException.class, () -> XContentType.fromMediaType(null)); + expectThrows(IllegalArgumentException.class, () -> XContentType.fromMediaType("")); + expectThrows(IllegalArgumentException.class, () -> XContentType.fromMediaType("text/plain")); + expectThrows(IllegalArgumentException.class, () -> XContentType.fromMediaType("gobbly;goop")); } public void testVersionedMediaType() { @@ -136,24 +135,26 @@ public void testVersionParsing() { assertThat(XContentType.parseVersion("APPLICATION/JSON"), nullValue()); - assertThat(XContentType.parseVersion("application/json;compatible-with=" + version + ".0"), - is(nullValue())); + expectThrows(IllegalArgumentException.class, + () -> XContentType.parseVersion("application/json;compatible-with=" + version + ".0")); } public void testUnrecognizedParameter() { - assertThat(XContentType.parseVersion("application/json; sth=123"), - is(nullValue())); + expectThrows(IllegalArgumentException.class, () -> XContentType.parseVersion("application/json; sth=123")); } public void testMediaTypeWithoutESSubtype() { String version = String.valueOf(Math.abs(randomByte())); - assertThat(XContentType.fromMediaType("application/json;compatible-with=" + version), nullValue()); + expectThrows(IllegalArgumentException.class, () -> XContentType.fromMediaType("application/json;compatible-with=" + version)); } public void testAnchoring() { String version = String.valueOf(Math.abs(randomByte())); - assertThat(XContentType.fromMediaType("sth_application/json;compatible-with=" + version + ".0"), nullValue()); - assertThat(XContentType.fromMediaType("sth_application/json;compatible-with=" + version + "_sth"), nullValue()); - assertThat(XContentType.fromMediaType("application/json;compatible-with=" + version + "_sth"), nullValue()); + expectThrows(IllegalArgumentException.class, + () -> XContentType.fromMediaType("application/json;compatible-with=" + version + ".0")); + expectThrows(IllegalArgumentException.class, + () -> XContentType.fromMediaType("application/json;compatible-with=" + version + "_sth")); + expectThrows(IllegalArgumentException.class, + () -> XContentType.fromMediaType("application/json;compatible-with=" + version + "_sth")); } } diff --git a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java index 8c8e83e5bfe42..df859084eea3a 100644 --- a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java +++ b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java @@ -321,6 +321,7 @@ public void testDispatchDoesNotRequireContentTypeForRequestsWithoutContent() { } public void testDispatchFailsWithPlainText() { + //todo pg now the exception is thrown earlier when request is created in AbstractHttpServerTransport. possibly move these tests there? String content = randomAlphaOfLength((int) Math.round(BREAKER_LIMIT.getBytes() / inFlightRequestsBreaker.getOverhead())); FakeRestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY) .withContent(new BytesArray(content), null).withPath("/foo") diff --git a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestResponse.java b/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestResponse.java index 9f5d0dfe9dcd7..0b8bf9367b8fa 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 @@ -45,7 +45,7 @@ public class ClientYamlTestResponse { private final Response response; private final byte[] body; - private final XContentType bodyContentType; + private XContentType bodyContentType; private ObjectPath parsedResponse; private String bodyAsString; @@ -53,7 +53,13 @@ public ClientYamlTestResponse(Response response) throws IOException { this.response = response; if (response.getEntity() != null) { String contentType = response.getHeader("Content-Type"); - this.bodyContentType = XContentType.fromMediaType(contentType); + // todo pg it feels like there is a lot of 'ifs' around text, csv etc being returned. shouldn't we treat text as the same json and others? + // we are testing sql formats with this client, but don't have access to sql parsing code + try{ + this.bodyContentType = XContentType.fromMediaType(contentType); + }catch (IllegalArgumentException e){ + this.bodyContentType = null; + } try { byte[] bytes = EntityUtils.toByteArray(response.getEntity()); //skip parsing if we got text back (e.g. if we called _cat apis) 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 4ade958b60e85..133d03d1b5bb2 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 @@ -7,6 +7,7 @@ package org.elasticsearch.xpack.sql.plugin; import org.elasticsearch.client.node.NodeClient; +import org.elasticsearch.common.xcontent.MediaType; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentType; @@ -19,7 +20,6 @@ import org.elasticsearch.xpack.sql.action.SqlQueryAction; import org.elasticsearch.xpack.sql.action.SqlQueryRequest; import org.elasticsearch.xpack.sql.action.SqlQueryResponse; -import org.elasticsearch.xpack.sql.proto.Mode; import org.elasticsearch.xpack.sql.proto.Protocol; import java.io.IOException; @@ -31,11 +31,11 @@ import static org.elasticsearch.rest.RestRequest.Method.GET; import static org.elasticsearch.rest.RestRequest.Method.POST; import static org.elasticsearch.xpack.sql.proto.Protocol.URL_PARAM_DELIMITER; -import static org.elasticsearch.xpack.sql.proto.Protocol.URL_PARAM_FORMAT; public class RestSqlQueryAction extends BaseRestHandler { - TextFormat textFormat; + private final SqlMediaTypeParser sqlMediaTypeParser = new SqlMediaTypeParser(); + MediaType responseMediaType; @Override public List routes() { @@ -52,53 +52,7 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli sqlRequest = SqlQueryRequest.fromXContent(parser); } - /* - * Since we support {@link TextFormat} and - * {@link XContent} outputs we can't use {@link RestToXContentListener} - * like everything else. We want to stick as closely as possible to - * Elasticsearch's defaults though, while still layering in ways to - * control the output more easily. - * - * First we find the string that the user used to specify the response - * format. If there is a {@code format} parameter we use that. If there - * 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; - - if (Mode.isDedicatedClient(sqlRequest.requestInfo().mode()) - && (sqlRequest.binaryCommunication() == null || sqlRequest.binaryCommunication())) { - // enforce CBOR response for drivers and CLI (unless instructed differently through the config param) - accept = XContentType.CBOR.name(); - } else { - accept = request.param(URL_PARAM_FORMAT); - } - if (accept == null) { - accept = request.header("Accept"); - if ("*/*".equals(accept)) { - // */* means "I don't care" which we should treat like not specifying the header - accept = null; - } - } - if (accept == null) { - accept = request.header("Content-Type"); - } - assert accept != null : "The Content-Type header is required"; - - /* - * Second, we pick the actual content type to use by first parsing the - * string from the previous step as an {@linkplain XContent} value. If - * that doesn't parse we parse it as a {@linkplain TextFormat} value. If - * that doesn't parse it'll throw an {@link IllegalArgumentException} - * which we turn into a 400 error. - */ - XContentType xContentType = getXContentType(accept); - textFormat = xContentType == null ? TextFormat.fromMediaTypeOrFormat(accept) : null; - - if (xContentType == null && sqlRequest.columnar()) { - throw new IllegalArgumentException("Invalid use of [columnar] argument: cannot be used in combination with " - + "txt, csv or tsv formats"); - } + MediaType responseMediaType = sqlMediaTypeParser.getMediaType(request, sqlRequest); long startNanos = System.nanoTime(); return channel -> client.execute(SqlQueryAction.INSTANCE, sqlRequest, new RestResponseListener(channel) { @@ -107,16 +61,16 @@ public RestResponse buildResponse(SqlQueryResponse response) throws Exception { RestResponse restResponse; // XContent branch - if (xContentType != null) { - XContentBuilder builder = channel.newBuilder(request.getXContentType(), xContentType, true); + if (responseMediaType != null && responseMediaType instanceof XContentType) { + XContentType type = (XContentType)responseMediaType; + XContentBuilder builder = channel.newBuilder(request.getXContentType(), type, true); response.toXContent(builder, request); restResponse = new BytesRestResponse(RestStatus.OK, builder); - } - // TextFormat - else { - final String data = textFormat.format(request, response); + } else {// TextFormat + TextFormat type = (TextFormat)responseMediaType; + final String data = type.format(request, response); - restResponse = new BytesRestResponse(RestStatus.OK, textFormat.contentType(request), + restResponse = new BytesRestResponse(RestStatus.OK, type.contentType(request), data.getBytes(StandardCharsets.UTF_8)); if (response.hasCursor()) { @@ -130,17 +84,12 @@ public RestResponse buildResponse(SqlQueryResponse response) throws Exception { }); } - private XContentType getXContentType(String accept) { - if (accept == null) { - return XContentType.JSON; - } - XContentType xContentType = XContentType.fromFormat(accept); - return xContentType != null ? xContentType : XContentType.fromMediaType(accept); - } + + @Override protected Set responseParams() { - return textFormat == TextFormat.CSV ? Collections.singleton(URL_PARAM_DELIMITER) : Collections.emptySet(); + return responseMediaType == TextFormat.CSV ? Collections.singleton(URL_PARAM_DELIMITER) : Collections.emptySet(); } @Override diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java new file mode 100644 index 0000000000000..e784d0cff49b9 --- /dev/null +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java @@ -0,0 +1,77 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +package org.elasticsearch.xpack.sql.plugin; + +import org.elasticsearch.common.xcontent.MediaType; +import org.elasticsearch.common.xcontent.MediaTypeParser; +import org.elasticsearch.common.xcontent.XContentType; +import org.elasticsearch.rest.RestRequest; +import org.elasticsearch.xpack.sql.action.SqlQueryRequest; +import org.elasticsearch.xpack.sql.proto.Mode; + +import java.util.Map; +import java.util.regex.Pattern; + +import static org.elasticsearch.xpack.sql.proto.Protocol.URL_PARAM_FORMAT; + +public class SqlMediaTypeParser { + private static final MediaTypeParser parser = new MediaTypeParser.Builder<>() + .copyFromMediaTypeParser(XContentType.mediaTypeParser) + .withMediaTypeAndParams(TextFormat.PLAIN_TEXT.typeWithSubtype(), TextFormat.PLAIN_TEXT, + Map.of("header", Pattern.compile("present|absent"), "charset", Pattern.compile("utf-8"))) + .withMediaTypeAndParams(TextFormat.CSV.typeWithSubtype(), TextFormat.CSV, + Map.of("header", Pattern.compile("present|absent"), "charset", Pattern.compile("utf-8"))) + .withMediaTypeAndParams(TextFormat.TSV.typeWithSubtype(), TextFormat.TSV, + Map.of("header", Pattern.compile("present|absent"), "charset", Pattern.compile("utf-8"))) + .build(); + + /* + * Since we support {@link TextFormat} and + * {@link XContent} outputs we can't use {@link RestToXContentListener} + * like everything else. We want to stick as closely as possible to + * Elasticsearch's defaults though, while still layering in ways to + * control the output more easily. + * + * First we find the string that the user used to specify the response + * format. If there is a {@code format} parameter we use that. If there + * 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. + */ + public MediaType getMediaType(RestRequest request, SqlQueryRequest sqlRequest) { + + if (Mode.isDedicatedClient(sqlRequest.requestInfo().mode()) + && (sqlRequest.binaryCommunication() == null || sqlRequest.binaryCommunication())) { + // enforce CBOR response for drivers and CLI (unless instructed differently through the config param) + return XContentType.CBOR; + } else if (request.hasParam(URL_PARAM_FORMAT)){ + return validateColumnarRequest(sqlRequest.columnar(), parser.fromFormat( request.param(URL_PARAM_FORMAT))); + } + if (request.getHeaders().containsKey("Accept")) { + String accept = request.header("Accept"); + // */* means "I don't care" which we should treat like not specifying the header + if ("*/*".equals(accept) == false) { + return validateColumnarRequest(sqlRequest.columnar(), parser.fromMediaType(accept)); + } + } + + //todo pg we should probably remove this possibility. it should only be accept or format. + // also if you specify sql's textformat in content type it will fail with exception now + String contentType = request.header("Content-Type"); + assert contentType != null : "The Content-Type header is required"; + return validateColumnarRequest(sqlRequest.columnar(), parser.fromMediaType(contentType)); + } + + private MediaType validateColumnarRequest(boolean requestIsColumnar, MediaType fromMediaType) { + if(requestIsColumnar && fromMediaType instanceof TextFormat){ + throw new IllegalArgumentException("Invalid use of [columnar] argument: cannot be used in combination with " + + "txt, csv or tsv formats"); + } + return fromMediaType; + + } + +} diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/TextFormat.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/TextFormat.java index 15ef8fa83ecac..3201c5fd7b188 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 @@ -9,6 +9,7 @@ import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.xcontent.MediaType; import org.elasticsearch.common.xcontent.MediaTypeParser; +import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.rest.RestRequest; import org.elasticsearch.xpack.ql.util.StringUtils; import org.elasticsearch.xpack.sql.SqlIllegalArgumentException; @@ -296,15 +297,6 @@ public String subtype() { private static final String PARAM_HEADER_ABSENT = "absent"; private static final String PARAM_HEADER_PRESENT = "present"; - private static final MediaTypeParser parser = new MediaTypeParser.Builder() - .withMediaTypeAndParams(PLAIN_TEXT.typeWithSubtype(), PLAIN_TEXT, - Map.of("header", Pattern.compile("present|absent"), "charset", Pattern.compile("utf-8"))) - .withMediaTypeAndParams(CSV.typeWithSubtype(), CSV, - Map.of("header", Pattern.compile("present|absent"), "charset", Pattern.compile("utf-8"))) - .withMediaTypeAndParams(TSV.typeWithSubtype(), TSV, - Map.of("header", Pattern.compile("present|absent"), "charset", Pattern.compile("utf-8"))) - .build(); - String format(RestRequest request, SqlQueryResponse response) { StringBuilder sb = new StringBuilder(); @@ -325,18 +317,6 @@ boolean hasHeader(RestRequest request) { return true; } - static TextFormat fromMediaTypeOrFormat(String accept) { - 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 + "]"); - } - /** * Formal IANA mime type. */ diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParserTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParserTests.java new file mode 100644 index 0000000000000..ad62e63531c0b --- /dev/null +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParserTests.java @@ -0,0 +1,90 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +package org.elasticsearch.xpack.sql.plugin; + +import org.elasticsearch.common.unit.TimeValue; +import org.elasticsearch.common.xcontent.MediaType; +import org.elasticsearch.common.xcontent.NamedXContentRegistry; +import org.elasticsearch.rest.RestRequest; +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.test.rest.FakeRestRequest; +import org.elasticsearch.xpack.sql.action.SqlQueryRequest; +import org.elasticsearch.xpack.sql.proto.Mode; +import org.elasticsearch.xpack.sql.proto.RequestInfo; + +import java.util.Collections; +import java.util.Map; + +import static org.elasticsearch.xpack.sql.plugin.TextFormat.CSV; +import static org.elasticsearch.xpack.sql.plugin.TextFormat.PLAIN_TEXT; +import static org.elasticsearch.xpack.sql.plugin.TextFormat.TSV; +import static org.elasticsearch.xpack.sql.proto.RequestInfo.CLIENT_IDS; +import static org.hamcrest.CoreMatchers.is; + +public class SqlMediaTypeParserTests extends ESTestCase { + SqlMediaTypeParser parser = new SqlMediaTypeParser(); + + public void testPlainTextDetection() { + MediaType text = parser.getMediaType(reqWithAccept("text/plain"), createTestInstance(false, Mode.PLAIN, false)); + assertThat(text, is(PLAIN_TEXT)); + } + + public void testCsvDetection() { + MediaType text = parser.getMediaType(reqWithAccept("text/csv"), createTestInstance(false, Mode.PLAIN, false)); + assertThat(text, is(CSV)); + } + + public void testTsvDetection() { + MediaType text = parser.getMediaType(reqWithAccept("text/tab-separated-values"), createTestInstance(false, Mode.PLAIN, false)); + assertThat(text, is(TSV)); + } + + public void testParametersParsing() { + assertThat(parser.getMediaType(reqWithAccept("text/plain; charset=utf-8"), + createTestInstance(false, Mode.PLAIN, false)), is(PLAIN_TEXT)); + assertThat(parser.getMediaType(reqWithAccept("text/plain; header=present"), + createTestInstance(false, Mode.PLAIN, false)), is(PLAIN_TEXT)); + assertThat(parser.getMediaType(reqWithAccept("text/plain; charset=utf-8; header=present"), + createTestInstance(false, Mode.PLAIN, false)), is(PLAIN_TEXT)); + + assertThat(parser.getMediaType(reqWithAccept("text/csv; charset=utf-8"), + createTestInstance(false, Mode.PLAIN, false)), is(CSV)); + assertThat(parser.getMediaType(reqWithAccept("text/csv; header=present"), + createTestInstance(false, Mode.PLAIN, false)), is(CSV)); + assertThat(parser.getMediaType(reqWithAccept("text/csv; charset=utf-8; header=present"), + createTestInstance(false, Mode.PLAIN, false)), is(CSV)); + + assertThat(parser.getMediaType(reqWithAccept("text/tab-separated-values; charset=utf-8"), + createTestInstance(false, Mode.PLAIN, false)), is(TSV)); + assertThat(parser.getMediaType(reqWithAccept("text/tab-separated-values; header=present"), + createTestInstance(false, Mode.PLAIN, false)), is(TSV)); + assertThat(parser.getMediaType(reqWithAccept("text/tab-separated-values; charset=utf-8; header=present"), + createTestInstance(false, Mode.PLAIN, false)), is(TSV)); + } + + public void testInvalidFormat() { + Exception e = expectThrows(IllegalArgumentException.class, + () -> parser.getMediaType(reqWithAccept("text/garbage"), createTestInstance(false, Mode.PLAIN, false))); + assertEquals("unrecognized media type: text/garbage", e.getMessage()); + } + + private static RestRequest reqWithAccept(String acceptHeader) { + + return new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY) + .withHeaders(Map.of("Content-Type", Collections.singletonList("application/json"), + "Accept", Collections.singletonList(acceptHeader))) + .build(); + } + + protected SqlQueryRequest createTestInstance(boolean binaryCommunication, Mode mode, boolean columnar) { + return new SqlQueryRequest(randomAlphaOfLength(10), Collections.emptyList(), null, + randomZone(), between(1, Integer.MAX_VALUE), TimeValue.parseTimeValue(randomTimeValue(), null, "test"), + TimeValue.parseTimeValue(randomTimeValue(), null, "test"), columnar, randomAlphaOfLength(10), + new RequestInfo(mode, randomFrom(randomFrom(CLIENT_IDS), randomAlphaOfLengthBetween(10, 20))), + randomBoolean(), randomBoolean()).binaryCommunication(binaryCommunication); + } +} diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/TextFormatTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/TextFormatTests.java index a9cf6c57f34aa..09d916af5cbc2 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/TextFormatTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/TextFormatTests.java @@ -25,46 +25,10 @@ import static java.util.Collections.singletonList; import static java.util.Collections.singletonMap; import static org.elasticsearch.xpack.sql.plugin.TextFormat.CSV; -import static org.elasticsearch.xpack.sql.plugin.TextFormat.PLAIN_TEXT; import static org.elasticsearch.xpack.sql.plugin.TextFormat.TSV; -import static org.hamcrest.CoreMatchers.is; public class TextFormatTests extends ESTestCase { - public void testPlainTextDetection() { - TextFormat text = TextFormat.fromMediaTypeOrFormat("text/plain"); - assertThat(text, is(PLAIN_TEXT)); - } - - public void testCsvDetection() { - TextFormat text = TextFormat.fromMediaTypeOrFormat("text/csv"); - assertThat(text, is(CSV)); - } - - public void testTsvDetection() { - TextFormat text = TextFormat.fromMediaTypeOrFormat("text/tab-separated-values"); - assertThat(text, is(TSV)); - } - - public void testParametersParsing() { - assertThat(TextFormat.fromMediaTypeOrFormat("text/plain; charset=utf-8"), is(PLAIN_TEXT)); - assertThat(TextFormat.fromMediaTypeOrFormat("text/plain; header=present"), is(PLAIN_TEXT)); - assertThat(TextFormat.fromMediaTypeOrFormat("text/plain; charset=utf-8; header=present"), is(PLAIN_TEXT)); - - assertThat(TextFormat.fromMediaTypeOrFormat("text/csv; charset=utf-8"), is(CSV)); - assertThat(TextFormat.fromMediaTypeOrFormat("text/csv; header=present"), is(CSV)); - assertThat(TextFormat.fromMediaTypeOrFormat("text/csv; charset=utf-8; header=present"), is(CSV)); - - assertThat(TextFormat.fromMediaTypeOrFormat("text/tab-separated-values; charset=utf-8"), is(TSV)); - assertThat(TextFormat.fromMediaTypeOrFormat("text/tab-separated-values; header=present"), is(TSV)); - assertThat(TextFormat.fromMediaTypeOrFormat("text/tab-separated-values; charset=utf-8; header=present"), is(TSV)); - } - - public void testInvalidFormat() { - Exception e = expectThrows(IllegalArgumentException.class, () -> TextFormat.fromMediaTypeOrFormat("text/garbage")); - assertEquals("invalid format [text/garbage]", e.getMessage()); - } - public void testCsvContentType() { assertEquals("text/csv; charset=utf-8; header=present", CSV.contentType(req())); } diff --git a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/input/http/HttpInputTests.java b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/input/http/HttpInputTests.java index 42788b2ea7ad1..59eea96c4c7f1 100644 --- a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/input/http/HttpInputTests.java +++ b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/input/http/HttpInputTests.java @@ -114,11 +114,14 @@ public void testExecute() throws Exception { } ExecutableHttpInput input = new ExecutableHttpInput(httpInput, httpClient, templateEngine); + //todo pg I am not sure this is actually what will happen now. When a ES will not return a response with unrecognized_content_type when(httpClient.execute(any(HttpRequest.class))).thenReturn(response); when(templateEngine.render(eq(new TextTemplate("_body")), any(Map.class))).thenReturn("_body"); WatchExecutionContext ctx = WatcherTestUtils.createWatchExecutionContext(); HttpInput.Result result = input.execute(ctx, new Payload.Simple()); + //TODO PG this test will fail now when it has `unrecognized_content_type` as we throw an exception. + // Is this ok? or should we catch a parse exception and somehow return httpinput.result? assertThat(result.type(), equalTo(HttpInput.TYPE)); assertThat(result.payload().data(), hasEntry("key", "value")); } From ce42f1bd933ef7823b11258d1b3c5f5efa88fcbc Mon Sep 17 00:00:00 2001 From: pgomulka Date: Thu, 24 Sep 2020 12:25:20 +0200 Subject: [PATCH 28/40] Revert "throwing exception when failing to parse" This reverts commit bd4dee1126974d0cf66d63eaf62564b8c3d9f032. --- .../gradle/http/WaitForHttpResource.java | 1 - .../client/RestHighLevelClient.java | 6 +- .../client/RestHighLevelClientTests.java | 5 +- .../common/xcontent/MediaTypeParser.java | 7 +- .../common/xcontent/XContentType.java | 11 ++- .../common/xcontent/MediaTypeParserTests.java | 17 ++-- .../rest/AbstractRestChannel.java | 18 +--- .../elasticsearch/rest/BytesRestResponse.java | 1 - .../rest/action/cat/RestTable.java | 9 +- .../common/xcontent/XContentTypeTests.java | 27 +++--- .../rest/RestControllerTests.java | 1 - .../rest/yaml/ClientYamlTestResponse.java | 10 +-- .../xpack/sql/plugin/RestSqlQueryAction.java | 79 +++++++++++++--- .../xpack/sql/plugin/SqlMediaTypeParser.java | 77 ---------------- .../xpack/sql/plugin/TextFormat.java | 22 ++++- .../sql/plugin/SqlMediaTypeParserTests.java | 90 ------------------- .../xpack/sql/plugin/TextFormatTests.java | 36 ++++++++ .../watcher/input/http/HttpInputTests.java | 3 - 18 files changed, 171 insertions(+), 249 deletions(-) delete mode 100644 x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java delete mode 100644 x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParserTests.java diff --git a/buildSrc/src/main/java/org/elasticsearch/gradle/http/WaitForHttpResource.java b/buildSrc/src/main/java/org/elasticsearch/gradle/http/WaitForHttpResource.java index 9aef2f6c98c41..db805b26cdca5 100644 --- a/buildSrc/src/main/java/org/elasticsearch/gradle/http/WaitForHttpResource.java +++ b/buildSrc/src/main/java/org/elasticsearch/gradle/http/WaitForHttpResource.java @@ -145,7 +145,6 @@ HttpURLConnection buildConnection(SSLContext ssl) throws IOException { configureSslContext(connection, ssl); configureBasicAuth(connection); connection.setRequestMethod("GET"); - connection.setRequestProperty("Accept", "text/plain; charset=UTF-8"); return connection; } 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 3cf285bf23a90..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 @@ -1900,9 +1900,9 @@ protected final Resp parseEntity(final HttpEntity entity, throw new IllegalStateException("Elasticsearch didn't return the [Content-Type] header, unable to parse response body"); } XContentType xContentType = XContentType.fromMediaType(entity.getContentType().getValue()); -// if (xContentType == null) { -// throw new IllegalStateException("Unsupported Content-Type: " + entity.getContentType().getValue()); -// } + if (xContentType == null) { + throw new IllegalStateException("Unsupported Content-Type: " + entity.getContentType().getValue()); + } try (XContentParser parser = xContentType.xContent().createParser(registry, DEPRECATION_HANDLER, entity.getContent())) { return entityParser.apply(parser); } diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/RestHighLevelClientTests.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/RestHighLevelClientTests.java index 0f302b3e0592f..8df8f7299965e 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/RestHighLevelClientTests.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/RestHighLevelClientTests.java @@ -302,9 +302,8 @@ public void testParseEntity() throws IOException { } { NStringEntity entity = new NStringEntity("", ContentType.APPLICATION_SVG_XML); - IllegalArgumentException ise = expectThrows(IllegalArgumentException.class, - () -> restHighLevelClient.parseEntity(entity, null)); - assertEquals("unrecognized media type: " + entity.getContentType().getValue(), ise.getMessage()); + IllegalStateException ise = expectThrows(IllegalStateException.class, () -> restHighLevelClient.parseEntity(entity, null)); + assertEquals("Unsupported Content-Type: " + entity.getContentType().getValue(), ise.getMessage()); } { CheckedFunction entityParser = parser -> { 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 e8de57386c57e..47cda1f4e3604 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 @@ -73,13 +73,12 @@ public ParsedMediaType parseMediaType(String headerValue) { //spaces are allowed between parameters, but not between '=' sign String[] keyValueParam = split[i].trim().split("="); if (keyValueParam.length != 2 || hasSpaces(keyValueParam[0]) || hasSpaces(keyValueParam[1])) { - throw new IllegalArgumentException("Spaces are not allowed between parameters. " + headerValue); + return null; } String parameterName = keyValueParam[0].toLowerCase(Locale.ROOT); String parameterValue = keyValueParam[1].toLowerCase(Locale.ROOT); if (isValidParameter(typeWithSubtype, parameterName, parameterValue) == false) { - throw new IllegalArgumentException("unrecognized parameter " - + parameterName + " " + parameterValue + " headerValue=" + headerValue); + return null; } parameters.put(parameterName, parameterValue); } @@ -88,7 +87,7 @@ public ParsedMediaType parseMediaType(String headerValue) { } } - throw new IllegalArgumentException("unrecognized media type: " + headerValue); + return null; } private boolean isValidParameter(String typeWithSubtype, String parameterName, String parameterValue) { 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 48462a40a20e4..431d30c891223 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 @@ -114,8 +114,13 @@ public XContent xContent() { return CborXContent.cborXContent; } }; - - public static final MediaTypeParser mediaTypeParser = new MediaTypeParser.Builder() + /** + * A regexp to allow parsing media types. It covers two use cases. + * 1. Media type with a version - requires a custom vnd.elasticsearch 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 MediaTypeParser mediaTypeParser = new MediaTypeParser.Builder() .withMediaTypeAndParams("application/smile", SMILE, Collections.emptyMap()) .withMediaTypeAndParams("application/cbor", CBOR, Collections.emptyMap()) .withMediaTypeAndParams("application/json", JSON, Map.of("charset", Pattern.compile("UTF-8"))) @@ -148,7 +153,7 @@ 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 can optionally has parameters. * This method is suitable for parsing of the {@code Content-Type} and {@code Accept} HTTP headers. - * This method will throw IllegalArgumentException if no match is found + * This method will return {@code null} if no match is found */ public static XContentType fromMediaType(String 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 0173f2bc9122c..8303c8e4b31f0 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,6 +26,8 @@ import java.util.regex.Pattern; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.nullValue; public class MediaTypeParserTests extends ESTestCase { @@ -57,16 +59,19 @@ public void testWhiteSpaceInTypeSubtype() { assertThat(mediaTypeParser.parseMediaType(mediaType + "; compatible-with=123;\n charset=UTF-8").getParameters(), equalTo(Map.of("charset", "utf-8", "compatible-with", "123"))); - String mediaTypeWithWhitespace = " application / json "; - expectThrows(IllegalArgumentException.class , () -> mediaTypeParser.parseMediaType(mediaTypeWithWhitespace)); + mediaType = " application / json "; + assertThat(mediaTypeParser.parseMediaType(mediaType), + is(nullValue())); } public void testInvalidParameters() { String mediaType = "application/vnd.elasticsearch+json"; - expectThrows(IllegalArgumentException.class , () -> mediaTypeParser.parseMediaType(mediaType + "; keyvalueNoEqualsSign")); + assertThat(mediaTypeParser.parseMediaType(mediaType + "; keyvalueNoEqualsSign"), + is(nullValue())); - expectThrows(IllegalArgumentException.class , () -> mediaTypeParser.parseMediaType(mediaType + "; key = value")); - - expectThrows(IllegalArgumentException.class , () -> mediaTypeParser.parseMediaType(mediaType + "; key=")); + assertThat(mediaTypeParser.parseMediaType(mediaType + "; key = value"), + is(nullValue())); + assertThat(mediaTypeParser.parseMediaType(mediaType + "; key=") , + is(nullValue())); } } diff --git a/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java b/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java index eaf0fd301d8d2..c5bf1efa2cdb1 100644 --- a/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java +++ b/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java @@ -18,8 +18,6 @@ */ package org.elasticsearch.rest; -import org.apache.logging.log4j.LogManager; -import org.apache.logging.log4j.Logger; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.Streams; @@ -38,7 +36,6 @@ public abstract class AbstractRestChannel implements RestChannel { - private static final Logger logger = LogManager.getLogger(AbstractRestChannel.class); private static final Predicate INCLUDE_FILTER = f -> f.charAt(0) != '-'; private static final Predicate EXCLUDE_FILTER = INCLUDE_FILTER.negate(); @@ -101,17 +98,10 @@ public XContentBuilder newBuilder(@Nullable XContentType requestContentType, boo public XContentBuilder newBuilder(@Nullable XContentType requestContentType, @Nullable XContentType responseContentType, boolean useFiltering) throws IOException { if (responseContentType == null) { - if (Strings.hasText(format)) { - responseContentType = XContentType.fromFormat(format); - } else if (Strings.hasText(acceptHeader) && acceptHeader.equals("*/*") == false - && acceptHeader.startsWith("text/plain")==false) { //TODO PG there are a lot of usages of text/plain as a response type. - // I feel it would fit into xcontenttype.. - try { - responseContentType = XContentType.fromMediaType(acceptHeader); - }catch (IllegalArgumentException e){ - //todo pg this is in a way controlling the flow by exceptions. to be discussed - logger.debug("Unrecognized accept header",e); - } + //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); } } // try to determine the response content type from the media type or the format query string parameter, with the format parameter diff --git a/server/src/main/java/org/elasticsearch/rest/BytesRestResponse.java b/server/src/main/java/org/elasticsearch/rest/BytesRestResponse.java index 91f3c1e5d3b6e..3babe43474266 100644 --- a/server/src/main/java/org/elasticsearch/rest/BytesRestResponse.java +++ b/server/src/main/java/org/elasticsearch/rest/BytesRestResponse.java @@ -83,7 +83,6 @@ public BytesRestResponse(RestStatus status, String contentType, byte[] content) /** * Creates a binary response. */ - //todo pg this should probably be acceptType? (2nd param) public BytesRestResponse(RestStatus status, String contentType, BytesReference content) { this.status = status; this.content = content; 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 c68b59050966a..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 @@ -51,21 +51,18 @@ public class RestTable { public static RestResponse buildResponse(Table table, RestChannel channel) throws Exception { RestRequest request = channel.request(); - XContentType xContentType = getXContentType(request); + XContentType xContentType = getxContentType(request); if (xContentType != null) { return buildXContentBuilder(table, channel); } return buildTextPlainResponse(table, channel); } - private static XContentType getXContentType(RestRequest request) { + private static XContentType getxContentType(RestRequest request) { if (request.hasParam("format")) { return XContentType.fromFormat(request.param("format")); } - if (request.header("Accept") != null && request.header("Accept").startsWith("text/plain") == false) { - return XContentType.fromMediaType(request.header("Accept")); - } - return null; + return XContentType.fromMediaType(request.header("Accept")); } public static RestResponse buildXContentBuilder(Table table, RestChannel channel) throws Exception { diff --git a/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java b/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java index 9e5c162ee2bda..686bc9fa65405 100644 --- a/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java +++ b/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java @@ -23,6 +23,7 @@ import java.util.Locale; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.nullValue; public class XContentTypeTests extends ESTestCase { @@ -88,10 +89,10 @@ public void testFromWildcardUppercase() throws Exception { } public void testFromRubbish() throws Exception { - expectThrows(IllegalArgumentException.class, () -> XContentType.fromMediaType(null)); - expectThrows(IllegalArgumentException.class, () -> XContentType.fromMediaType("")); - expectThrows(IllegalArgumentException.class, () -> XContentType.fromMediaType("text/plain")); - expectThrows(IllegalArgumentException.class, () -> XContentType.fromMediaType("gobbly;goop")); + assertThat(XContentType.fromMediaType(null), nullValue()); + assertThat(XContentType.fromMediaType(""), nullValue()); + assertThat(XContentType.fromMediaType("text/plain"), nullValue()); + assertThat(XContentType.fromMediaType("gobbly;goop"), nullValue()); } public void testVersionedMediaType() { @@ -135,26 +136,24 @@ public void testVersionParsing() { assertThat(XContentType.parseVersion("APPLICATION/JSON"), nullValue()); - expectThrows(IllegalArgumentException.class, - () -> XContentType.parseVersion("application/json;compatible-with=" + version + ".0")); + assertThat(XContentType.parseVersion("application/json;compatible-with=" + version + ".0"), + is(nullValue())); } public void testUnrecognizedParameter() { - expectThrows(IllegalArgumentException.class, () -> XContentType.parseVersion("application/json; sth=123")); + assertThat(XContentType.parseVersion("application/json; sth=123"), + is(nullValue())); } public void testMediaTypeWithoutESSubtype() { String version = String.valueOf(Math.abs(randomByte())); - expectThrows(IllegalArgumentException.class, () -> XContentType.fromMediaType("application/json;compatible-with=" + version)); + assertThat(XContentType.fromMediaType("application/json;compatible-with=" + version), nullValue()); } public void testAnchoring() { String version = String.valueOf(Math.abs(randomByte())); - expectThrows(IllegalArgumentException.class, - () -> XContentType.fromMediaType("application/json;compatible-with=" + version + ".0")); - expectThrows(IllegalArgumentException.class, - () -> XContentType.fromMediaType("application/json;compatible-with=" + version + "_sth")); - expectThrows(IllegalArgumentException.class, - () -> XContentType.fromMediaType("application/json;compatible-with=" + version + "_sth")); + assertThat(XContentType.fromMediaType("sth_application/json;compatible-with=" + version + ".0"), nullValue()); + assertThat(XContentType.fromMediaType("sth_application/json;compatible-with=" + version + "_sth"), nullValue()); + assertThat(XContentType.fromMediaType("application/json;compatible-with=" + version + "_sth"), nullValue()); } } diff --git a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java index df859084eea3a..8c8e83e5bfe42 100644 --- a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java +++ b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java @@ -321,7 +321,6 @@ public void testDispatchDoesNotRequireContentTypeForRequestsWithoutContent() { } public void testDispatchFailsWithPlainText() { - //todo pg now the exception is thrown earlier when request is created in AbstractHttpServerTransport. possibly move these tests there? String content = randomAlphaOfLength((int) Math.round(BREAKER_LIMIT.getBytes() / inFlightRequestsBreaker.getOverhead())); FakeRestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY) .withContent(new BytesArray(content), null).withPath("/foo") 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 0b8bf9367b8fa..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 @@ -45,7 +45,7 @@ public class ClientYamlTestResponse { private final Response response; private final byte[] body; - private XContentType bodyContentType; + private final XContentType bodyContentType; private ObjectPath parsedResponse; private String bodyAsString; @@ -53,13 +53,7 @@ public ClientYamlTestResponse(Response response) throws IOException { this.response = response; if (response.getEntity() != null) { String contentType = response.getHeader("Content-Type"); - // todo pg it feels like there is a lot of 'ifs' around text, csv etc being returned. shouldn't we treat text as the same json and others? - // we are testing sql formats with this client, but don't have access to sql parsing code - try{ - this.bodyContentType = XContentType.fromMediaType(contentType); - }catch (IllegalArgumentException e){ - this.bodyContentType = null; - } + 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/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 133d03d1b5bb2..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 @@ -7,7 +7,6 @@ package org.elasticsearch.xpack.sql.plugin; import org.elasticsearch.client.node.NodeClient; -import org.elasticsearch.common.xcontent.MediaType; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentType; @@ -20,6 +19,7 @@ import org.elasticsearch.xpack.sql.action.SqlQueryAction; import org.elasticsearch.xpack.sql.action.SqlQueryRequest; import org.elasticsearch.xpack.sql.action.SqlQueryResponse; +import org.elasticsearch.xpack.sql.proto.Mode; import org.elasticsearch.xpack.sql.proto.Protocol; import java.io.IOException; @@ -31,11 +31,11 @@ import static org.elasticsearch.rest.RestRequest.Method.GET; import static org.elasticsearch.rest.RestRequest.Method.POST; import static org.elasticsearch.xpack.sql.proto.Protocol.URL_PARAM_DELIMITER; +import static org.elasticsearch.xpack.sql.proto.Protocol.URL_PARAM_FORMAT; public class RestSqlQueryAction extends BaseRestHandler { - private final SqlMediaTypeParser sqlMediaTypeParser = new SqlMediaTypeParser(); - MediaType responseMediaType; + TextFormat textFormat; @Override public List routes() { @@ -52,7 +52,53 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli sqlRequest = SqlQueryRequest.fromXContent(parser); } - MediaType responseMediaType = sqlMediaTypeParser.getMediaType(request, sqlRequest); + /* + * Since we support {@link TextFormat} and + * {@link XContent} outputs we can't use {@link RestToXContentListener} + * like everything else. We want to stick as closely as possible to + * Elasticsearch's defaults though, while still layering in ways to + * control the output more easily. + * + * First we find the string that the user used to specify the response + * format. If there is a {@code format} parameter we use that. If there + * 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; + + if (Mode.isDedicatedClient(sqlRequest.requestInfo().mode()) + && (sqlRequest.binaryCommunication() == null || sqlRequest.binaryCommunication())) { + // enforce CBOR response for drivers and CLI (unless instructed differently through the config param) + accept = XContentType.CBOR.name(); + } else { + accept = request.param(URL_PARAM_FORMAT); + } + if (accept == null) { + accept = request.header("Accept"); + if ("*/*".equals(accept)) { + // */* means "I don't care" which we should treat like not specifying the header + accept = null; + } + } + if (accept == null) { + accept = request.header("Content-Type"); + } + assert accept != null : "The Content-Type header is required"; + + /* + * Second, we pick the actual content type to use by first parsing the + * string from the previous step as an {@linkplain XContent} value. If + * that doesn't parse we parse it as a {@linkplain TextFormat} value. If + * that doesn't parse it'll throw an {@link IllegalArgumentException} + * which we turn into a 400 error. + */ + XContentType xContentType = getXContentType(accept); + textFormat = xContentType == null ? TextFormat.fromMediaTypeOrFormat(accept) : null; + + if (xContentType == null && sqlRequest.columnar()) { + throw new IllegalArgumentException("Invalid use of [columnar] argument: cannot be used in combination with " + + "txt, csv or tsv formats"); + } long startNanos = System.nanoTime(); return channel -> client.execute(SqlQueryAction.INSTANCE, sqlRequest, new RestResponseListener(channel) { @@ -61,16 +107,16 @@ public RestResponse buildResponse(SqlQueryResponse response) throws Exception { RestResponse restResponse; // XContent branch - if (responseMediaType != null && responseMediaType instanceof XContentType) { - XContentType type = (XContentType)responseMediaType; - XContentBuilder builder = channel.newBuilder(request.getXContentType(), type, true); + if (xContentType != null) { + XContentBuilder builder = channel.newBuilder(request.getXContentType(), xContentType, true); response.toXContent(builder, request); restResponse = new BytesRestResponse(RestStatus.OK, builder); - } else {// TextFormat - TextFormat type = (TextFormat)responseMediaType; - final String data = type.format(request, response); + } + // TextFormat + else { + final String data = textFormat.format(request, response); - restResponse = new BytesRestResponse(RestStatus.OK, type.contentType(request), + restResponse = new BytesRestResponse(RestStatus.OK, textFormat.contentType(request), data.getBytes(StandardCharsets.UTF_8)); if (response.hasCursor()) { @@ -84,12 +130,17 @@ public RestResponse buildResponse(SqlQueryResponse response) throws Exception { }); } - - + private XContentType getXContentType(String accept) { + if (accept == null) { + return XContentType.JSON; + } + XContentType xContentType = XContentType.fromFormat(accept); + return xContentType != null ? xContentType : XContentType.fromMediaType(accept); + } @Override protected Set responseParams() { - return responseMediaType == TextFormat.CSV ? Collections.singleton(URL_PARAM_DELIMITER) : Collections.emptySet(); + return textFormat == TextFormat.CSV ? Collections.singleton(URL_PARAM_DELIMITER) : Collections.emptySet(); } @Override diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java deleted file mode 100644 index e784d0cff49b9..0000000000000 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java +++ /dev/null @@ -1,77 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - */ - -package org.elasticsearch.xpack.sql.plugin; - -import org.elasticsearch.common.xcontent.MediaType; -import org.elasticsearch.common.xcontent.MediaTypeParser; -import org.elasticsearch.common.xcontent.XContentType; -import org.elasticsearch.rest.RestRequest; -import org.elasticsearch.xpack.sql.action.SqlQueryRequest; -import org.elasticsearch.xpack.sql.proto.Mode; - -import java.util.Map; -import java.util.regex.Pattern; - -import static org.elasticsearch.xpack.sql.proto.Protocol.URL_PARAM_FORMAT; - -public class SqlMediaTypeParser { - private static final MediaTypeParser parser = new MediaTypeParser.Builder<>() - .copyFromMediaTypeParser(XContentType.mediaTypeParser) - .withMediaTypeAndParams(TextFormat.PLAIN_TEXT.typeWithSubtype(), TextFormat.PLAIN_TEXT, - Map.of("header", Pattern.compile("present|absent"), "charset", Pattern.compile("utf-8"))) - .withMediaTypeAndParams(TextFormat.CSV.typeWithSubtype(), TextFormat.CSV, - Map.of("header", Pattern.compile("present|absent"), "charset", Pattern.compile("utf-8"))) - .withMediaTypeAndParams(TextFormat.TSV.typeWithSubtype(), TextFormat.TSV, - Map.of("header", Pattern.compile("present|absent"), "charset", Pattern.compile("utf-8"))) - .build(); - - /* - * Since we support {@link TextFormat} and - * {@link XContent} outputs we can't use {@link RestToXContentListener} - * like everything else. We want to stick as closely as possible to - * Elasticsearch's defaults though, while still layering in ways to - * control the output more easily. - * - * First we find the string that the user used to specify the response - * format. If there is a {@code format} parameter we use that. If there - * 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. - */ - public MediaType getMediaType(RestRequest request, SqlQueryRequest sqlRequest) { - - if (Mode.isDedicatedClient(sqlRequest.requestInfo().mode()) - && (sqlRequest.binaryCommunication() == null || sqlRequest.binaryCommunication())) { - // enforce CBOR response for drivers and CLI (unless instructed differently through the config param) - return XContentType.CBOR; - } else if (request.hasParam(URL_PARAM_FORMAT)){ - return validateColumnarRequest(sqlRequest.columnar(), parser.fromFormat( request.param(URL_PARAM_FORMAT))); - } - if (request.getHeaders().containsKey("Accept")) { - String accept = request.header("Accept"); - // */* means "I don't care" which we should treat like not specifying the header - if ("*/*".equals(accept) == false) { - return validateColumnarRequest(sqlRequest.columnar(), parser.fromMediaType(accept)); - } - } - - //todo pg we should probably remove this possibility. it should only be accept or format. - // also if you specify sql's textformat in content type it will fail with exception now - String contentType = request.header("Content-Type"); - assert contentType != null : "The Content-Type header is required"; - return validateColumnarRequest(sqlRequest.columnar(), parser.fromMediaType(contentType)); - } - - private MediaType validateColumnarRequest(boolean requestIsColumnar, MediaType fromMediaType) { - if(requestIsColumnar && fromMediaType instanceof TextFormat){ - throw new IllegalArgumentException("Invalid use of [columnar] argument: cannot be used in combination with " - + "txt, csv or tsv formats"); - } - return fromMediaType; - - } - -} diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/TextFormat.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/TextFormat.java index 3201c5fd7b188..15ef8fa83ecac 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 @@ -9,7 +9,6 @@ import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.xcontent.MediaType; import org.elasticsearch.common.xcontent.MediaTypeParser; -import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.rest.RestRequest; import org.elasticsearch.xpack.ql.util.StringUtils; import org.elasticsearch.xpack.sql.SqlIllegalArgumentException; @@ -297,6 +296,15 @@ public String subtype() { private static final String PARAM_HEADER_ABSENT = "absent"; private static final String PARAM_HEADER_PRESENT = "present"; + private static final MediaTypeParser parser = new MediaTypeParser.Builder() + .withMediaTypeAndParams(PLAIN_TEXT.typeWithSubtype(), PLAIN_TEXT, + Map.of("header", Pattern.compile("present|absent"), "charset", Pattern.compile("utf-8"))) + .withMediaTypeAndParams(CSV.typeWithSubtype(), CSV, + Map.of("header", Pattern.compile("present|absent"), "charset", Pattern.compile("utf-8"))) + .withMediaTypeAndParams(TSV.typeWithSubtype(), TSV, + Map.of("header", Pattern.compile("present|absent"), "charset", Pattern.compile("utf-8"))) + .build(); + String format(RestRequest request, SqlQueryResponse response) { StringBuilder sb = new StringBuilder(); @@ -317,6 +325,18 @@ boolean hasHeader(RestRequest request) { return true; } + static TextFormat fromMediaTypeOrFormat(String accept) { + 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 + "]"); + } + /** * Formal IANA mime type. */ diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParserTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParserTests.java deleted file mode 100644 index ad62e63531c0b..0000000000000 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParserTests.java +++ /dev/null @@ -1,90 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - */ - -package org.elasticsearch.xpack.sql.plugin; - -import org.elasticsearch.common.unit.TimeValue; -import org.elasticsearch.common.xcontent.MediaType; -import org.elasticsearch.common.xcontent.NamedXContentRegistry; -import org.elasticsearch.rest.RestRequest; -import org.elasticsearch.test.ESTestCase; -import org.elasticsearch.test.rest.FakeRestRequest; -import org.elasticsearch.xpack.sql.action.SqlQueryRequest; -import org.elasticsearch.xpack.sql.proto.Mode; -import org.elasticsearch.xpack.sql.proto.RequestInfo; - -import java.util.Collections; -import java.util.Map; - -import static org.elasticsearch.xpack.sql.plugin.TextFormat.CSV; -import static org.elasticsearch.xpack.sql.plugin.TextFormat.PLAIN_TEXT; -import static org.elasticsearch.xpack.sql.plugin.TextFormat.TSV; -import static org.elasticsearch.xpack.sql.proto.RequestInfo.CLIENT_IDS; -import static org.hamcrest.CoreMatchers.is; - -public class SqlMediaTypeParserTests extends ESTestCase { - SqlMediaTypeParser parser = new SqlMediaTypeParser(); - - public void testPlainTextDetection() { - MediaType text = parser.getMediaType(reqWithAccept("text/plain"), createTestInstance(false, Mode.PLAIN, false)); - assertThat(text, is(PLAIN_TEXT)); - } - - public void testCsvDetection() { - MediaType text = parser.getMediaType(reqWithAccept("text/csv"), createTestInstance(false, Mode.PLAIN, false)); - assertThat(text, is(CSV)); - } - - public void testTsvDetection() { - MediaType text = parser.getMediaType(reqWithAccept("text/tab-separated-values"), createTestInstance(false, Mode.PLAIN, false)); - assertThat(text, is(TSV)); - } - - public void testParametersParsing() { - assertThat(parser.getMediaType(reqWithAccept("text/plain; charset=utf-8"), - createTestInstance(false, Mode.PLAIN, false)), is(PLAIN_TEXT)); - assertThat(parser.getMediaType(reqWithAccept("text/plain; header=present"), - createTestInstance(false, Mode.PLAIN, false)), is(PLAIN_TEXT)); - assertThat(parser.getMediaType(reqWithAccept("text/plain; charset=utf-8; header=present"), - createTestInstance(false, Mode.PLAIN, false)), is(PLAIN_TEXT)); - - assertThat(parser.getMediaType(reqWithAccept("text/csv; charset=utf-8"), - createTestInstance(false, Mode.PLAIN, false)), is(CSV)); - assertThat(parser.getMediaType(reqWithAccept("text/csv; header=present"), - createTestInstance(false, Mode.PLAIN, false)), is(CSV)); - assertThat(parser.getMediaType(reqWithAccept("text/csv; charset=utf-8; header=present"), - createTestInstance(false, Mode.PLAIN, false)), is(CSV)); - - assertThat(parser.getMediaType(reqWithAccept("text/tab-separated-values; charset=utf-8"), - createTestInstance(false, Mode.PLAIN, false)), is(TSV)); - assertThat(parser.getMediaType(reqWithAccept("text/tab-separated-values; header=present"), - createTestInstance(false, Mode.PLAIN, false)), is(TSV)); - assertThat(parser.getMediaType(reqWithAccept("text/tab-separated-values; charset=utf-8; header=present"), - createTestInstance(false, Mode.PLAIN, false)), is(TSV)); - } - - public void testInvalidFormat() { - Exception e = expectThrows(IllegalArgumentException.class, - () -> parser.getMediaType(reqWithAccept("text/garbage"), createTestInstance(false, Mode.PLAIN, false))); - assertEquals("unrecognized media type: text/garbage", e.getMessage()); - } - - private static RestRequest reqWithAccept(String acceptHeader) { - - return new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY) - .withHeaders(Map.of("Content-Type", Collections.singletonList("application/json"), - "Accept", Collections.singletonList(acceptHeader))) - .build(); - } - - protected SqlQueryRequest createTestInstance(boolean binaryCommunication, Mode mode, boolean columnar) { - return new SqlQueryRequest(randomAlphaOfLength(10), Collections.emptyList(), null, - randomZone(), between(1, Integer.MAX_VALUE), TimeValue.parseTimeValue(randomTimeValue(), null, "test"), - TimeValue.parseTimeValue(randomTimeValue(), null, "test"), columnar, randomAlphaOfLength(10), - new RequestInfo(mode, randomFrom(randomFrom(CLIENT_IDS), randomAlphaOfLengthBetween(10, 20))), - randomBoolean(), randomBoolean()).binaryCommunication(binaryCommunication); - } -} diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/TextFormatTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/TextFormatTests.java index 09d916af5cbc2..a9cf6c57f34aa 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/TextFormatTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/TextFormatTests.java @@ -25,10 +25,46 @@ import static java.util.Collections.singletonList; import static java.util.Collections.singletonMap; import static org.elasticsearch.xpack.sql.plugin.TextFormat.CSV; +import static org.elasticsearch.xpack.sql.plugin.TextFormat.PLAIN_TEXT; import static org.elasticsearch.xpack.sql.plugin.TextFormat.TSV; +import static org.hamcrest.CoreMatchers.is; public class TextFormatTests extends ESTestCase { + public void testPlainTextDetection() { + TextFormat text = TextFormat.fromMediaTypeOrFormat("text/plain"); + assertThat(text, is(PLAIN_TEXT)); + } + + public void testCsvDetection() { + TextFormat text = TextFormat.fromMediaTypeOrFormat("text/csv"); + assertThat(text, is(CSV)); + } + + public void testTsvDetection() { + TextFormat text = TextFormat.fromMediaTypeOrFormat("text/tab-separated-values"); + assertThat(text, is(TSV)); + } + + public void testParametersParsing() { + assertThat(TextFormat.fromMediaTypeOrFormat("text/plain; charset=utf-8"), is(PLAIN_TEXT)); + assertThat(TextFormat.fromMediaTypeOrFormat("text/plain; header=present"), is(PLAIN_TEXT)); + assertThat(TextFormat.fromMediaTypeOrFormat("text/plain; charset=utf-8; header=present"), is(PLAIN_TEXT)); + + assertThat(TextFormat.fromMediaTypeOrFormat("text/csv; charset=utf-8"), is(CSV)); + assertThat(TextFormat.fromMediaTypeOrFormat("text/csv; header=present"), is(CSV)); + assertThat(TextFormat.fromMediaTypeOrFormat("text/csv; charset=utf-8; header=present"), is(CSV)); + + assertThat(TextFormat.fromMediaTypeOrFormat("text/tab-separated-values; charset=utf-8"), is(TSV)); + assertThat(TextFormat.fromMediaTypeOrFormat("text/tab-separated-values; header=present"), is(TSV)); + assertThat(TextFormat.fromMediaTypeOrFormat("text/tab-separated-values; charset=utf-8; header=present"), is(TSV)); + } + + public void testInvalidFormat() { + Exception e = expectThrows(IllegalArgumentException.class, () -> TextFormat.fromMediaTypeOrFormat("text/garbage")); + assertEquals("invalid format [text/garbage]", e.getMessage()); + } + public void testCsvContentType() { assertEquals("text/csv; charset=utf-8; header=present", CSV.contentType(req())); } diff --git a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/input/http/HttpInputTests.java b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/input/http/HttpInputTests.java index 59eea96c4c7f1..42788b2ea7ad1 100644 --- a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/input/http/HttpInputTests.java +++ b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/input/http/HttpInputTests.java @@ -114,14 +114,11 @@ public void testExecute() throws Exception { } ExecutableHttpInput input = new ExecutableHttpInput(httpInput, httpClient, templateEngine); - //todo pg I am not sure this is actually what will happen now. When a ES will not return a response with unrecognized_content_type when(httpClient.execute(any(HttpRequest.class))).thenReturn(response); when(templateEngine.render(eq(new TextTemplate("_body")), any(Map.class))).thenReturn("_body"); WatchExecutionContext ctx = WatcherTestUtils.createWatchExecutionContext(); HttpInput.Result result = input.execute(ctx, new Payload.Simple()); - //TODO PG this test will fail now when it has `unrecognized_content_type` as we throw an exception. - // Is this ok? or should we catch a parse exception and somehow return httpinput.result? assertThat(result.type(), equalTo(HttpInput.TYPE)); assertThat(result.payload().data(), hasEntry("key", "value")); } From c4f02ade240cd69c6966d0082800c8cc5ba69854 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Mon, 28 Sep 2020 09:27:51 +0200 Subject: [PATCH 29/40] import --- .../elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java | 3 ++- .../java/org/elasticsearch/xpack/sql/plugin/TextFormat.java | 4 ---- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java index e784d0cff49b9..5b313d0a010ee 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java @@ -24,7 +24,8 @@ public class SqlMediaTypeParser { .withMediaTypeAndParams(TextFormat.PLAIN_TEXT.typeWithSubtype(), TextFormat.PLAIN_TEXT, Map.of("header", Pattern.compile("present|absent"), "charset", Pattern.compile("utf-8"))) .withMediaTypeAndParams(TextFormat.CSV.typeWithSubtype(), TextFormat.CSV, - Map.of("header", Pattern.compile("present|absent"), "charset", Pattern.compile("utf-8"))) + Map.of("header", Pattern.compile("present|absent"), "charset", Pattern.compile("utf-8"), + "delimiter", Pattern.compile("[^\"\n\r\t]+"))) .withMediaTypeAndParams(TextFormat.TSV.typeWithSubtype(), TextFormat.TSV, Map.of("header", Pattern.compile("present|absent"), "charset", Pattern.compile("utf-8"))) .build(); 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 3201c5fd7b188..d397d2316959c 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,8 +8,6 @@ import org.elasticsearch.common.Strings; import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.xcontent.MediaType; -import org.elasticsearch.common.xcontent.MediaTypeParser; -import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.rest.RestRequest; import org.elasticsearch.xpack.ql.util.StringUtils; import org.elasticsearch.xpack.sql.SqlIllegalArgumentException; @@ -26,10 +24,8 @@ import java.time.ZonedDateTime; import java.util.List; import java.util.Locale; -import java.util.Map; import java.util.Objects; import java.util.function.Function; -import java.util.regex.Pattern; import static org.elasticsearch.xpack.sql.action.BasicFormatter.FormatOption.TEXT; import static org.elasticsearch.xpack.sql.proto.Protocol.URL_PARAM_DELIMITER; From 176302efd7687f267a211337d6c5632bc44d3085 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Mon, 28 Sep 2020 09:57:09 +0200 Subject: [PATCH 30/40] fix storing response time in sql --- .../test/java/org/elasticsearch/rest/RestControllerTests.java | 3 ++- .../elasticsearch/test/rest/yaml/ClientYamlTestResponse.java | 3 ++- .../org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java index df859084eea3a..4d8359999f9d5 100644 --- a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java +++ b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java @@ -321,7 +321,8 @@ public void testDispatchDoesNotRequireContentTypeForRequestsWithoutContent() { } public void testDispatchFailsWithPlainText() { - //todo pg now the exception is thrown earlier when request is created in AbstractHttpServerTransport. possibly move these tests there? + //todo pg now the exception is thrown earlier when request is created in AbstractHttpServerTransport. + // possibly move these tests there? String content = randomAlphaOfLength((int) Math.round(BREAKER_LIMIT.getBytes() / inFlightRequestsBreaker.getOverhead())); FakeRestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY) .withContent(new BytesArray(content), null).withPath("/foo") 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 0b8bf9367b8fa..459e88538481a 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,8 @@ public ClientYamlTestResponse(Response response) throws IOException { this.response = response; if (response.getEntity() != null) { String contentType = response.getHeader("Content-Type"); - // todo pg it feels like there is a lot of 'ifs' around text, csv etc being returned. shouldn't we treat text as the same json and others? + // todo pg it feels like there is a lot of 'ifs' around text, csv etc being returned. + // shouldn't we treat text as the same json and others? // we are testing sql formats with this client, but don't have access to sql parsing code try{ this.bodyContentType = XContentType.fromMediaType(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 133d03d1b5bb2..7ad9ac8c8d537 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 @@ -52,7 +52,7 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli sqlRequest = SqlQueryRequest.fromXContent(parser); } - MediaType responseMediaType = sqlMediaTypeParser.getMediaType(request, sqlRequest); + responseMediaType = sqlMediaTypeParser.getMediaType(request, sqlRequest); long startNanos = System.nanoTime(); return channel -> client.execute(SqlQueryAction.INSTANCE, sqlRequest, new RestResponseListener(channel) { From eab471c5ebd45f8ab008bdf1f3272cd091bae2b7 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Mon, 28 Sep 2020 11:34:40 +0200 Subject: [PATCH 31/40] return null on errors --- .../gradle/http/WaitForHttpResource.java | 1 - .../client/RestHighLevelClient.java | 6 ++-- .../client/RestHighLevelClientTests.java | 5 ++-- .../common/xcontent/MediaTypeParser.java | 9 +++--- .../common/xcontent/XContentType.java | 2 +- .../common/xcontent/MediaTypeParserTests.java | 18 +++++++----- .../rest/AbstractRestChannel.java | 12 ++------ .../elasticsearch/rest/BytesRestResponse.java | 1 - .../rest/action/cat/RestTable.java | 5 +--- .../common/xcontent/XContentTypeTests.java | 28 +++++++++---------- .../rest/RestControllerTests.java | 2 -- .../rest/yaml/ClientYamlTestResponse.java | 11 ++------ 12 files changed, 40 insertions(+), 60 deletions(-) diff --git a/buildSrc/src/main/java/org/elasticsearch/gradle/http/WaitForHttpResource.java b/buildSrc/src/main/java/org/elasticsearch/gradle/http/WaitForHttpResource.java index 9aef2f6c98c41..db805b26cdca5 100644 --- a/buildSrc/src/main/java/org/elasticsearch/gradle/http/WaitForHttpResource.java +++ b/buildSrc/src/main/java/org/elasticsearch/gradle/http/WaitForHttpResource.java @@ -145,7 +145,6 @@ HttpURLConnection buildConnection(SSLContext ssl) throws IOException { configureSslContext(connection, ssl); configureBasicAuth(connection); connection.setRequestMethod("GET"); - connection.setRequestProperty("Accept", "text/plain; charset=UTF-8"); return connection; } 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 3cf285bf23a90..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 @@ -1900,9 +1900,9 @@ protected final Resp parseEntity(final HttpEntity entity, throw new IllegalStateException("Elasticsearch didn't return the [Content-Type] header, unable to parse response body"); } XContentType xContentType = XContentType.fromMediaType(entity.getContentType().getValue()); -// if (xContentType == null) { -// throw new IllegalStateException("Unsupported Content-Type: " + entity.getContentType().getValue()); -// } + if (xContentType == null) { + throw new IllegalStateException("Unsupported Content-Type: " + entity.getContentType().getValue()); + } try (XContentParser parser = xContentType.xContent().createParser(registry, DEPRECATION_HANDLER, entity.getContent())) { return entityParser.apply(parser); } diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/RestHighLevelClientTests.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/RestHighLevelClientTests.java index 0f302b3e0592f..8df8f7299965e 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/RestHighLevelClientTests.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/RestHighLevelClientTests.java @@ -302,9 +302,8 @@ public void testParseEntity() throws IOException { } { NStringEntity entity = new NStringEntity("", ContentType.APPLICATION_SVG_XML); - IllegalArgumentException ise = expectThrows(IllegalArgumentException.class, - () -> restHighLevelClient.parseEntity(entity, null)); - assertEquals("unrecognized media type: " + entity.getContentType().getValue(), ise.getMessage()); + IllegalStateException ise = expectThrows(IllegalStateException.class, () -> restHighLevelClient.parseEntity(entity, null)); + assertEquals("Unsupported Content-Type: " + entity.getContentType().getValue(), ise.getMessage()); } { CheckedFunction entityParser = parser -> { 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 e8de57386c57e..387a971f3dc44 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 @@ -53,7 +53,7 @@ public T fromFormat(String format) { * * @param headerValue a header value from Accept or Content-Type * @return a parsed media-type - * //todo pg should this all be maybe based on a regex? + * //todo pg write a benchmark and consider using a regex */ public ParsedMediaType parseMediaType(String headerValue) { if (headerValue != null) { @@ -73,13 +73,12 @@ public ParsedMediaType parseMediaType(String headerValue) { //spaces are allowed between parameters, but not between '=' sign String[] keyValueParam = split[i].trim().split("="); if (keyValueParam.length != 2 || hasSpaces(keyValueParam[0]) || hasSpaces(keyValueParam[1])) { - throw new IllegalArgumentException("Spaces are not allowed between parameters. " + headerValue); + return null; } String parameterName = keyValueParam[0].toLowerCase(Locale.ROOT); String parameterValue = keyValueParam[1].toLowerCase(Locale.ROOT); if (isValidParameter(typeWithSubtype, parameterName, parameterValue) == false) { - throw new IllegalArgumentException("unrecognized parameter " - + parameterName + " " + parameterValue + " headerValue=" + headerValue); + return null; } parameters.put(parameterName, parameterValue); } @@ -88,7 +87,7 @@ public ParsedMediaType parseMediaType(String headerValue) { } } - throw new IllegalArgumentException("unrecognized media type: " + headerValue); + return null; } private boolean isValidParameter(String typeWithSubtype, String parameterName, String parameterValue) { 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 48462a40a20e4..0767eca4d1698 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 @@ -148,7 +148,7 @@ 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 can optionally has parameters. * This method is suitable for parsing of the {@code Content-Type} and {@code Accept} HTTP headers. - * This method will throw IllegalArgumentException if no match is found + * This method will return {@code null} if no match is found */ public static XContentType fromMediaType(String 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 0173f2bc9122c..65ae16d152e49 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,6 +26,8 @@ import java.util.regex.Pattern; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.nullValue; public class MediaTypeParserTests extends ESTestCase { @@ -57,16 +59,18 @@ public void testWhiteSpaceInTypeSubtype() { assertThat(mediaTypeParser.parseMediaType(mediaType + "; compatible-with=123;\n charset=UTF-8").getParameters(), equalTo(Map.of("charset", "utf-8", "compatible-with", "123"))); - String mediaTypeWithWhitespace = " application / json "; - expectThrows(IllegalArgumentException.class , () -> mediaTypeParser.parseMediaType(mediaTypeWithWhitespace)); + mediaType = " application / json "; + assertThat(mediaTypeParser.parseMediaType(mediaType), + is(nullValue())); } public void testInvalidParameters() { String mediaType = "application/vnd.elasticsearch+json"; - expectThrows(IllegalArgumentException.class , () -> mediaTypeParser.parseMediaType(mediaType + "; keyvalueNoEqualsSign")); - - expectThrows(IllegalArgumentException.class , () -> mediaTypeParser.parseMediaType(mediaType + "; key = value")); - - expectThrows(IllegalArgumentException.class , () -> mediaTypeParser.parseMediaType(mediaType + "; key=")); + assertThat(mediaTypeParser.parseMediaType(mediaType + "; keyvalueNoEqualsSign"), + is(nullValue())); + assertThat(mediaTypeParser.parseMediaType(mediaType + "; key = value"), + is(nullValue())); + assertThat(mediaTypeParser.parseMediaType(mediaType + "; key=") , + is(nullValue())); } } diff --git a/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java b/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java index eaf0fd301d8d2..6f5aa618ae4d0 100644 --- a/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java +++ b/server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java @@ -103,15 +103,9 @@ public XContentBuilder newBuilder(@Nullable XContentType requestContentType, @Nu if (responseContentType == null) { if (Strings.hasText(format)) { responseContentType = XContentType.fromFormat(format); - } else if (Strings.hasText(acceptHeader) && acceptHeader.equals("*/*") == false - && acceptHeader.startsWith("text/plain")==false) { //TODO PG there are a lot of usages of text/plain as a response type. - // I feel it would fit into xcontenttype.. - try { - responseContentType = XContentType.fromMediaType(acceptHeader); - }catch (IllegalArgumentException e){ - //todo pg this is in a way controlling the flow by exceptions. to be discussed - logger.debug("Unrecognized accept header",e); - } + } + 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 diff --git a/server/src/main/java/org/elasticsearch/rest/BytesRestResponse.java b/server/src/main/java/org/elasticsearch/rest/BytesRestResponse.java index 91f3c1e5d3b6e..3babe43474266 100644 --- a/server/src/main/java/org/elasticsearch/rest/BytesRestResponse.java +++ b/server/src/main/java/org/elasticsearch/rest/BytesRestResponse.java @@ -83,7 +83,6 @@ public BytesRestResponse(RestStatus status, String contentType, byte[] content) /** * Creates a binary response. */ - //todo pg this should probably be acceptType? (2nd param) public BytesRestResponse(RestStatus status, String contentType, BytesReference content) { this.status = status; this.content = content; 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 c68b59050966a..bbb746baafe92 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/cat/RestTable.java +++ b/server/src/main/java/org/elasticsearch/rest/action/cat/RestTable.java @@ -62,10 +62,7 @@ private static XContentType getXContentType(RestRequest request) { if (request.hasParam("format")) { return XContentType.fromFormat(request.param("format")); } - if (request.header("Accept") != null && request.header("Accept").startsWith("text/plain") == false) { - return XContentType.fromMediaType(request.header("Accept")); - } - return null; + return XContentType.fromMediaType(request.header("Accept")); } public static RestResponse buildXContentBuilder(Table table, RestChannel channel) throws Exception { diff --git a/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java b/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java index 9e5c162ee2bda..3f66ef917e53f 100644 --- a/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java +++ b/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java @@ -23,6 +23,7 @@ import java.util.Locale; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.nullValue; public class XContentTypeTests extends ESTestCase { @@ -88,10 +89,10 @@ public void testFromWildcardUppercase() throws Exception { } public void testFromRubbish() throws Exception { - expectThrows(IllegalArgumentException.class, () -> XContentType.fromMediaType(null)); - expectThrows(IllegalArgumentException.class, () -> XContentType.fromMediaType("")); - expectThrows(IllegalArgumentException.class, () -> XContentType.fromMediaType("text/plain")); - expectThrows(IllegalArgumentException.class, () -> XContentType.fromMediaType("gobbly;goop")); + assertThat(XContentType.fromMediaType(null), nullValue()); + assertThat(XContentType.fromMediaType(""), nullValue()); + assertThat(XContentType.fromMediaType("text/plain"), nullValue()); + assertThat(XContentType.fromMediaType("gobbly;goop"), nullValue()); } public void testVersionedMediaType() { @@ -135,26 +136,23 @@ public void testVersionParsing() { assertThat(XContentType.parseVersion("APPLICATION/JSON"), nullValue()); - expectThrows(IllegalArgumentException.class, - () -> XContentType.parseVersion("application/json;compatible-with=" + version + ".0")); + assertThat(XContentType.parseVersion("application/json;compatible-with=" + version + ".0"), + is(nullValue())); } public void testUnrecognizedParameter() { - expectThrows(IllegalArgumentException.class, () -> XContentType.parseVersion("application/json; sth=123")); - } + assertThat(XContentType.parseVersion("application/json; sth=123"), + is(nullValue())); } public void testMediaTypeWithoutESSubtype() { String version = String.valueOf(Math.abs(randomByte())); - expectThrows(IllegalArgumentException.class, () -> XContentType.fromMediaType("application/json;compatible-with=" + version)); + assertThat(XContentType.fromMediaType("application/json;compatible-with=" + version), nullValue()); } public void testAnchoring() { String version = String.valueOf(Math.abs(randomByte())); - expectThrows(IllegalArgumentException.class, - () -> XContentType.fromMediaType("application/json;compatible-with=" + version + ".0")); - expectThrows(IllegalArgumentException.class, - () -> XContentType.fromMediaType("application/json;compatible-with=" + version + "_sth")); - expectThrows(IllegalArgumentException.class, - () -> XContentType.fromMediaType("application/json;compatible-with=" + version + "_sth")); + assertThat(XContentType.fromMediaType("sth_application/json;compatible-with=" + version + ".0"), nullValue()); + assertThat(XContentType.fromMediaType("sth_application/json;compatible-with=" + version + "_sth"), nullValue()); + assertThat(XContentType.fromMediaType("application/json;compatible-with=" + version + "_sth"), nullValue()); } } diff --git a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java index 4d8359999f9d5..8c8e83e5bfe42 100644 --- a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java +++ b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java @@ -321,8 +321,6 @@ public void testDispatchDoesNotRequireContentTypeForRequestsWithoutContent() { } public void testDispatchFailsWithPlainText() { - //todo pg now the exception is thrown earlier when request is created in AbstractHttpServerTransport. - // possibly move these tests there? String content = randomAlphaOfLength((int) Math.round(BREAKER_LIMIT.getBytes() / inFlightRequestsBreaker.getOverhead())); FakeRestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY) .withContent(new BytesArray(content), null).withPath("/foo") 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 459e88538481a..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 @@ -45,7 +45,7 @@ public class ClientYamlTestResponse { private final Response response; private final byte[] body; - private XContentType bodyContentType; + private final XContentType bodyContentType; private ObjectPath parsedResponse; private String bodyAsString; @@ -53,14 +53,7 @@ public ClientYamlTestResponse(Response response) throws IOException { this.response = response; if (response.getEntity() != null) { String contentType = response.getHeader("Content-Type"); - // todo pg it feels like there is a lot of 'ifs' around text, csv etc being returned. - // shouldn't we treat text as the same json and others? - // we are testing sql formats with this client, but don't have access to sql parsing code - try{ - this.bodyContentType = XContentType.fromMediaType(contentType); - }catch (IllegalArgumentException e){ - this.bodyContentType = null; - } + 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) From e128fd78e363cb027ccf7a41018cb9e965fcc801 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Mon, 28 Sep 2020 12:16:28 +0200 Subject: [PATCH 32/40] sqlmedia type parsing test - do not throw exceptions --- .../xpack/sql/plugin/SqlMediaTypeParserTests.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParserTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParserTests.java index ad62e63531c0b..d2671c1748baf 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParserTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParserTests.java @@ -24,6 +24,7 @@ import static org.elasticsearch.xpack.sql.plugin.TextFormat.TSV; import static org.elasticsearch.xpack.sql.proto.RequestInfo.CLIENT_IDS; import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.CoreMatchers.nullValue; public class SqlMediaTypeParserTests extends ESTestCase { SqlMediaTypeParser parser = new SqlMediaTypeParser(); @@ -67,9 +68,8 @@ public void testParametersParsing() { } public void testInvalidFormat() { - Exception e = expectThrows(IllegalArgumentException.class, - () -> parser.getMediaType(reqWithAccept("text/garbage"), createTestInstance(false, Mode.PLAIN, false))); - assertEquals("unrecognized media type: text/garbage", e.getMessage()); + MediaType mediaType = parser.getMediaType(reqWithAccept("text/garbage"), createTestInstance(false, Mode.PLAIN, false)); + assertThat(mediaType, is(nullValue())); } private static RestRequest reqWithAccept(String acceptHeader) { From 66435dd9c9bf07f1457344b9e6772d65ac4a4afc Mon Sep 17 00:00:00 2001 From: pgomulka Date: Mon, 28 Sep 2020 14:45:26 +0200 Subject: [PATCH 33/40] remove todoes --- .../common/xcontent/XContentType.java | 14 ++++++++------ .../xpack/sql/plugin/SqlMediaTypeParser.java | 2 -- .../xpack/watcher/input/http/HttpInputTests.java | 3 --- 3 files changed, 8 insertions(+), 11 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 0767eca4d1698..f7c1625a26837 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 @@ -115,6 +115,8 @@ public XContent xContent() { } }; + private static final Pattern VERSION_PATTERN = Pattern.compile("\\d+"); + private static final String COMPATIBLE_WITH_PARAMETER_NAME = "compatible-with"; public static final MediaTypeParser mediaTypeParser = new MediaTypeParser.Builder() .withMediaTypeAndParams("application/smile", SMILE, Collections.emptyMap()) .withMediaTypeAndParams("application/cbor", CBOR, Collections.emptyMap()) @@ -123,15 +125,15 @@ public XContent xContent() { .withMediaTypeAndParams("application/*", JSON, Map.of("charset", Pattern.compile("UTF-8"))) .withMediaTypeAndParams("application/x-ndjson", JSON, Map.of("charset", Pattern.compile("UTF-8"))) .withMediaTypeAndParams("application/vnd.elasticsearch+json", JSON, - Map.of("compatible-with", Pattern.compile("\\d+"),"charset", Pattern.compile("UTF-8"))) + Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN,"charset", Pattern.compile("UTF-8"))) .withMediaTypeAndParams("application/vnd.elasticsearch+smile", SMILE, - Map.of("compatible-with", Pattern.compile("\\d+"),"charset", Pattern.compile("UTF-8"))) + Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN,"charset", Pattern.compile("UTF-8"))) .withMediaTypeAndParams("application/vnd.elasticsearch+yaml", YAML, - Map.of("compatible-with", Pattern.compile("\\d+"),"charset", Pattern.compile("UTF-8"))) + Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN,"charset", Pattern.compile("UTF-8"))) .withMediaTypeAndParams("application/vnd.elasticsearch+cbor", CBOR, - Map.of("compatible-with", Pattern.compile("\\d+"),"charset", Pattern.compile("UTF-8"))) + Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN,"charset", Pattern.compile("UTF-8"))) .withMediaTypeAndParams("application/vnd.elasticsearch+x-ndjson", JSON, - Map.of("compatible-with", Pattern.compile("\\d+"),"charset", Pattern.compile("UTF-8"))) + Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN,"charset", Pattern.compile("UTF-8"))) .build(); /** @@ -165,7 +167,7 @@ public static Byte parseVersion(String mediaType) { if (parsedMediaType != null) { String version = parsedMediaType .getParameters() - .get("compatible-with"); + .get(COMPATIBLE_WITH_PARAMETER_NAME); return version != null ? Byte.parseByte(version) : null; } return null; diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java index 5b313d0a010ee..b0ab4c5f642e3 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java @@ -59,8 +59,6 @@ public MediaType getMediaType(RestRequest request, SqlQueryRequest sqlRequest) { } } - //todo pg we should probably remove this possibility. it should only be accept or format. - // also if you specify sql's textformat in content type it will fail with exception now String contentType = request.header("Content-Type"); assert contentType != null : "The Content-Type header is required"; return validateColumnarRequest(sqlRequest.columnar(), parser.fromMediaType(contentType)); diff --git a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/input/http/HttpInputTests.java b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/input/http/HttpInputTests.java index 59eea96c4c7f1..42788b2ea7ad1 100644 --- a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/input/http/HttpInputTests.java +++ b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/input/http/HttpInputTests.java @@ -114,14 +114,11 @@ public void testExecute() throws Exception { } ExecutableHttpInput input = new ExecutableHttpInput(httpInput, httpClient, templateEngine); - //todo pg I am not sure this is actually what will happen now. When a ES will not return a response with unrecognized_content_type when(httpClient.execute(any(HttpRequest.class))).thenReturn(response); when(templateEngine.render(eq(new TextTemplate("_body")), any(Map.class))).thenReturn("_body"); WatchExecutionContext ctx = WatcherTestUtils.createWatchExecutionContext(); HttpInput.Result result = input.execute(ctx, new Payload.Simple()); - //TODO PG this test will fail now when it has `unrecognized_content_type` as we throw an exception. - // Is this ok? or should we catch a parse exception and somehow return httpinput.result? assertThat(result.type(), equalTo(HttpInput.TYPE)); assertThat(result.payload().data(), hasEntry("key", "value")); } From 08f1395ce1f1014d06cefaff9f2722945f761e75 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Mon, 28 Sep 2020 18:30:07 +0200 Subject: [PATCH 34/40] rename argument --- .../elasticsearch/rest/BytesRestResponse.java | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/rest/BytesRestResponse.java b/server/src/main/java/org/elasticsearch/rest/BytesRestResponse.java index 3babe43474266..859eddc9a613a 100644 --- a/server/src/main/java/org/elasticsearch/rest/BytesRestResponse.java +++ b/server/src/main/java/org/elasticsearch/rest/BytesRestResponse.java @@ -50,7 +50,7 @@ public class BytesRestResponse extends RestResponse { private final RestStatus status; private final BytesReference content; - private final String contentType; + private final String responseMediaType; /** * Creates a new response based on {@link XContentBuilder}. @@ -69,24 +69,24 @@ public BytesRestResponse(RestStatus status, String content) { /** * Creates a new plain text response. */ - public BytesRestResponse(RestStatus status, String contentType, String content) { - this(status, contentType, new BytesArray(content)); + public BytesRestResponse(RestStatus status, String responseMediaType, String content) { + this(status, responseMediaType, new BytesArray(content)); } /** * Creates a binary response. */ - public BytesRestResponse(RestStatus status, String contentType, byte[] content) { - this(status, contentType, new BytesArray(content)); + public BytesRestResponse(RestStatus status, String responseMediaType, byte[] content) { + this(status, responseMediaType, new BytesArray(content)); } /** * Creates a binary response. */ - public BytesRestResponse(RestStatus status, String contentType, BytesReference content) { + public BytesRestResponse(RestStatus status, String responseMediaType, BytesReference content) { this.status = status; this.content = content; - this.contentType = contentType; + this.responseMediaType = responseMediaType; } public BytesRestResponse(RestChannel channel, Exception e) throws IOException { @@ -109,7 +109,7 @@ public BytesRestResponse(RestChannel channel, RestStatus status, Exception e) th try (XContentBuilder builder = channel.newErrorBuilder()) { build(builder, params, status, channel.detailedErrorsEnabled(), e); this.content = BytesReference.bytes(builder); - this.contentType = builder.contentType().mediaType(); + this.responseMediaType = builder.contentType().mediaType(); } if (e instanceof ElasticsearchException) { copyHeaders(((ElasticsearchException) e)); @@ -118,7 +118,7 @@ public BytesRestResponse(RestChannel channel, RestStatus status, Exception e) th @Override public String contentType() { - return this.contentType; + return this.responseMediaType; } @Override From 3f3f0d55628bc05dd679ac6dd0f333446cd6093c Mon Sep 17 00:00:00 2001 From: pgomulka Date: Tue, 29 Sep 2020 20:04:50 +0200 Subject: [PATCH 35/40] review follow up --- .../common/xcontent/MediaTypeParser.java | 1 - .../elasticsearch/common/xcontent/XContentType.java | 12 ++++++------ .../common/xcontent/MediaTypeParserTests.java | 4 +++- .../common/xcontent/XContentTypeTests.java | 1 + 4 files changed, 10 insertions(+), 8 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 387a971f3dc44..e5825cae50cac 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 @@ -53,7 +53,6 @@ public T fromFormat(String format) { * * @param headerValue a header value from Accept or Content-Type * @return a parsed media-type - * //todo pg write a benchmark and consider using a regex */ public ParsedMediaType parseMediaType(String headerValue) { if (headerValue != null) { 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 f7c1625a26837..c15cca4d1ab3f 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentType.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentType.java @@ -117,7 +117,7 @@ public XContent xContent() { private static final Pattern VERSION_PATTERN = Pattern.compile("\\d+"); private static final String COMPATIBLE_WITH_PARAMETER_NAME = "compatible-with"; - public static final MediaTypeParser mediaTypeParser = new MediaTypeParser.Builder() + public static final MediaTypeParser mediaTypeParser = new MediaTypeParser.Builder() .withMediaTypeAndParams("application/smile", SMILE, Collections.emptyMap()) .withMediaTypeAndParams("application/cbor", CBOR, Collections.emptyMap()) .withMediaTypeAndParams("application/json", JSON, Map.of("charset", Pattern.compile("UTF-8"))) @@ -125,15 +125,15 @@ public XContent xContent() { .withMediaTypeAndParams("application/*", JSON, Map.of("charset", Pattern.compile("UTF-8"))) .withMediaTypeAndParams("application/x-ndjson", JSON, Map.of("charset", Pattern.compile("UTF-8"))) .withMediaTypeAndParams("application/vnd.elasticsearch+json", JSON, - Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN,"charset", Pattern.compile("UTF-8"))) + Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", Pattern.compile("UTF-8"))) .withMediaTypeAndParams("application/vnd.elasticsearch+smile", SMILE, - Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN,"charset", Pattern.compile("UTF-8"))) + Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", Pattern.compile("UTF-8"))) .withMediaTypeAndParams("application/vnd.elasticsearch+yaml", YAML, - Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN,"charset", Pattern.compile("UTF-8"))) + Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", Pattern.compile("UTF-8"))) .withMediaTypeAndParams("application/vnd.elasticsearch+cbor", CBOR, - Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN,"charset", Pattern.compile("UTF-8"))) + Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", Pattern.compile("UTF-8"))) .withMediaTypeAndParams("application/vnd.elasticsearch+x-ndjson", JSON, - Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN,"charset", Pattern.compile("UTF-8"))) + Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", Pattern.compile("UTF-8"))) .build(); /** 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 65ae16d152e49..e9eaf251cdd6f 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 @@ -34,7 +34,7 @@ public class MediaTypeParserTests extends ESTestCase { MediaTypeParser mediaTypeParser = new MediaTypeParser.Builder() .withMediaTypeAndParams("application/vnd.elasticsearch+json", XContentType.JSON, Map.of("compatible-with", Pattern.compile("\\d+"), - "charset", Pattern.compile("UTF-8"))) + "charset", Pattern.compile("UTF-8", Pattern.CASE_INSENSITIVE))) .build(); public void testJsonWithParameters() throws Exception { @@ -66,6 +66,8 @@ public void testWhiteSpaceInTypeSubtype() { public void testInvalidParameters() { String mediaType = "application/vnd.elasticsearch+json"; + assertThat(mediaTypeParser.parseMediaType(mediaType + "; charset=unknown") , + is(nullValue())); assertThat(mediaTypeParser.parseMediaType(mediaType + "; keyvalueNoEqualsSign"), is(nullValue())); assertThat(mediaTypeParser.parseMediaType(mediaType + "; key = value"), 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 3f66ef917e53f..fa937f84d615c 100644 --- a/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java +++ b/server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java @@ -34,6 +34,7 @@ public void testFromJson() throws Exception { assertThat(XContentType.fromMediaType(mediaType), equalTo(expectedXContentType)); assertThat(XContentType.fromMediaType(mediaType + ";"), equalTo(expectedXContentType)); assertThat(XContentType.fromMediaType(mediaType + "; charset=UTF-8"), equalTo(expectedXContentType)); + assertThat(XContentType.fromMediaType(mediaType + "; charset=utf-8"), equalTo(expectedXContentType)); } public void testFromNdJson() throws Exception { From 427c3913e197afecb821f36f29adbf8a298e2055 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Wed, 30 Sep 2020 09:07:59 +0200 Subject: [PATCH 36/40] remove pattern from signature --- .../common/xcontent/MediaTypeParser.java | 8 +++---- .../common/xcontent/XContentType.java | 21 +++++++++---------- .../common/xcontent/MediaTypeParserTests.java | 5 ++--- .../xpack/sql/plugin/SqlMediaTypeParser.java | 9 ++++---- 4 files changed, 20 insertions(+), 23 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 e5825cae50cac..62a3f3fd915d0 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 @@ -130,15 +130,15 @@ public static class Builder { private final Map typeWithSubtypeToMediaType = new HashMap<>(); private final Map> parametersMap = new HashMap<>(); - public Builder withMediaTypeAndParams(String alternativeMediaType, T mediaType, Map paramNameAndValueRegex) { + public Builder withMediaTypeAndParams(String alternativeMediaType, T mediaType, Map paramNameAndValueRegex) { typeWithSubtypeToMediaType.put(alternativeMediaType.toLowerCase(Locale.ROOT), mediaType); formatToMediaType.put(mediaType.format(), mediaType); Map parametersForMediaType = new HashMap<>(paramNameAndValueRegex.size()); - for (Map.Entry params : paramNameAndValueRegex.entrySet()) { + for (Map.Entry params : paramNameAndValueRegex.entrySet()) { String parameterName = params.getKey().toLowerCase(Locale.ROOT); - Pattern parameterRegex = params.getValue(); - Pattern pattern = Pattern.compile(parameterRegex.pattern(), Pattern.CASE_INSENSITIVE); + String parameterRegex = params.getValue(); + Pattern pattern = Pattern.compile(parameterRegex, Pattern.CASE_INSENSITIVE); parametersForMediaType.put(parameterName, pattern); } parametersMap.put(alternativeMediaType, parametersForMediaType); 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 c15cca4d1ab3f..fcd02a23173ca 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,7 +26,6 @@ import java.util.Collections; import java.util.Map; -import java.util.regex.Pattern; /** * The content type of {@link org.elasticsearch.common.xcontent.XContent}. @@ -115,25 +114,25 @@ public XContent xContent() { } }; - private static final Pattern VERSION_PATTERN = Pattern.compile("\\d+"); private static final String COMPATIBLE_WITH_PARAMETER_NAME = "compatible-with"; + private static final String VERSION_PATTERN = "\\d"; public static final MediaTypeParser mediaTypeParser = new MediaTypeParser.Builder() .withMediaTypeAndParams("application/smile", SMILE, Collections.emptyMap()) .withMediaTypeAndParams("application/cbor", CBOR, Collections.emptyMap()) - .withMediaTypeAndParams("application/json", JSON, Map.of("charset", Pattern.compile("UTF-8"))) - .withMediaTypeAndParams("application/yaml", YAML, Map.of("charset", Pattern.compile("UTF-8"))) - .withMediaTypeAndParams("application/*", JSON, Map.of("charset", Pattern.compile("UTF-8"))) - .withMediaTypeAndParams("application/x-ndjson", JSON, Map.of("charset", Pattern.compile("UTF-8"))) + .withMediaTypeAndParams("application/json", JSON, Map.of("charset", "UTF-8")) + .withMediaTypeAndParams("application/yaml", YAML, Map.of("charset", "UTF-8")) + .withMediaTypeAndParams("application/*", JSON, Map.of("charset", "UTF-8")) + .withMediaTypeAndParams("application/x-ndjson", JSON, Map.of("charset", "UTF-8")) .withMediaTypeAndParams("application/vnd.elasticsearch+json", JSON, - Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", Pattern.compile("UTF-8"))) + Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", "UTF-8")) .withMediaTypeAndParams("application/vnd.elasticsearch+smile", SMILE, - Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", Pattern.compile("UTF-8"))) + Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", "UTF-8")) .withMediaTypeAndParams("application/vnd.elasticsearch+yaml", YAML, - Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", Pattern.compile("UTF-8"))) + Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", "UTF-8")) .withMediaTypeAndParams("application/vnd.elasticsearch+cbor", CBOR, - Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", Pattern.compile("UTF-8"))) + Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", "UTF-8")) .withMediaTypeAndParams("application/vnd.elasticsearch+x-ndjson", JSON, - Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", Pattern.compile("UTF-8"))) + Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", "UTF-8")) .build(); /** 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 e9eaf251cdd6f..08ca08a3d2240 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 @@ -23,7 +23,6 @@ import java.util.Collections; import java.util.Map; -import java.util.regex.Pattern; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; @@ -33,8 +32,8 @@ public class MediaTypeParserTests extends ESTestCase { MediaTypeParser mediaTypeParser = new MediaTypeParser.Builder() .withMediaTypeAndParams("application/vnd.elasticsearch+json", - XContentType.JSON, Map.of("compatible-with", Pattern.compile("\\d+"), - "charset", Pattern.compile("UTF-8", Pattern.CASE_INSENSITIVE))) + XContentType.JSON, Map.of("compatible-with", "\\d+", + "charset", "UTF-8")) .build(); public void testJsonWithParameters() throws Exception { diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java index b0ab4c5f642e3..f3b1bd228e9c5 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java @@ -14,7 +14,6 @@ import org.elasticsearch.xpack.sql.proto.Mode; import java.util.Map; -import java.util.regex.Pattern; import static org.elasticsearch.xpack.sql.proto.Protocol.URL_PARAM_FORMAT; @@ -22,12 +21,12 @@ public class SqlMediaTypeParser { private static final MediaTypeParser parser = new MediaTypeParser.Builder<>() .copyFromMediaTypeParser(XContentType.mediaTypeParser) .withMediaTypeAndParams(TextFormat.PLAIN_TEXT.typeWithSubtype(), TextFormat.PLAIN_TEXT, - Map.of("header", Pattern.compile("present|absent"), "charset", Pattern.compile("utf-8"))) + Map.of("header", "present|absent", "charset", "utf-8")) .withMediaTypeAndParams(TextFormat.CSV.typeWithSubtype(), TextFormat.CSV, - Map.of("header", Pattern.compile("present|absent"), "charset", Pattern.compile("utf-8"), - "delimiter", Pattern.compile("[^\"\n\r\t]+"))) + Map.of("header", "present|absent", "charset", "utf-8", + "delimiter", "[^\"\n\r\t]+")) .withMediaTypeAndParams(TextFormat.TSV.typeWithSubtype(), TextFormat.TSV, - Map.of("header", Pattern.compile("present|absent"), "charset", Pattern.compile("utf-8"))) + Map.of("header", "present|absent", "charset", "utf-8")) .build(); /* From e0316054c4a9559ddc5b115b9b5b72999c701649 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Wed, 30 Sep 2020 10:01:26 +0200 Subject: [PATCH 37/40] fix regex --- .../java/org/elasticsearch/common/xcontent/XContentType.java | 2 +- 1 file changed, 1 insertion(+), 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 fcd02a23173ca..076a20bad006a 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 @@ -115,7 +115,7 @@ public XContent xContent() { }; private static final String COMPATIBLE_WITH_PARAMETER_NAME = "compatible-with"; - private static final String VERSION_PATTERN = "\\d"; + private static final String VERSION_PATTERN = "\\d+"; public static final MediaTypeParser mediaTypeParser = new MediaTypeParser.Builder() .withMediaTypeAndParams("application/smile", SMILE, Collections.emptyMap()) .withMediaTypeAndParams("application/cbor", CBOR, Collections.emptyMap()) From 4134d92ef9ea93c205746bc2f5031ee40c1d579e Mon Sep 17 00:00:00 2001 From: Przemyslaw Gomulka Date: Fri, 2 Oct 2020 08:24:26 +0200 Subject: [PATCH 38/40] Apply suggestions from code review Co-authored-by: Jay Modi --- .../elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java | 4 ++-- 1 file changed, 2 insertions(+), 2 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 7ad9ac8c8d537..763e460170cf0 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 @@ -62,11 +62,11 @@ public RestResponse buildResponse(SqlQueryResponse response) throws Exception { // XContent branch if (responseMediaType != null && responseMediaType instanceof XContentType) { - XContentType type = (XContentType)responseMediaType; + XContentType type = (XContentType) responseMediaType; XContentBuilder builder = channel.newBuilder(request.getXContentType(), type, true); response.toXContent(builder, request); restResponse = new BytesRestResponse(RestStatus.OK, builder); - } else {// TextFormat + } else { // TextFormat TextFormat type = (TextFormat)responseMediaType; final String data = type.format(request, response); From a976f5b53d024edf45b5e583130f53f65184ffd9 Mon Sep 17 00:00:00 2001 From: Przemyslaw Gomulka Date: Mon, 5 Oct 2020 11:11:33 +0200 Subject: [PATCH 39/40] Apply suggestions from code review Co-authored-by: Bogdan Pintea --- .../elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java index f3b1bd228e9c5..75c896a68318d 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java @@ -47,8 +47,8 @@ public MediaType getMediaType(RestRequest request, SqlQueryRequest sqlRequest) { && (sqlRequest.binaryCommunication() == null || sqlRequest.binaryCommunication())) { // enforce CBOR response for drivers and CLI (unless instructed differently through the config param) return XContentType.CBOR; - } else if (request.hasParam(URL_PARAM_FORMAT)){ - return validateColumnarRequest(sqlRequest.columnar(), parser.fromFormat( request.param(URL_PARAM_FORMAT))); + } else if (request.hasParam(URL_PARAM_FORMAT)) { + return validateColumnarRequest(sqlRequest.columnar(), parser.fromFormat(request.param(URL_PARAM_FORMAT))); } if (request.getHeaders().containsKey("Accept")) { String accept = request.header("Accept"); From 3e0eec01383a9cbb7ec2f5e858149a93848ae8d6 Mon Sep 17 00:00:00 2001 From: pgomulka Date: Mon, 5 Oct 2020 14:09:59 +0200 Subject: [PATCH 40/40] relax delimiter validation --- .../elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java | 5 ++--- .../xpack/sql/plugin/SqlMediaTypeParserTests.java | 5 ++++- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java index 75c896a68318d..189dc137b654c 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java @@ -24,7 +24,7 @@ public class SqlMediaTypeParser { Map.of("header", "present|absent", "charset", "utf-8")) .withMediaTypeAndParams(TextFormat.CSV.typeWithSubtype(), TextFormat.CSV, Map.of("header", "present|absent", "charset", "utf-8", - "delimiter", "[^\"\n\r\t]+")) + "delimiter", ".+"))// more detailed parsing is in TextFormat.CSV#delimiter .withMediaTypeAndParams(TextFormat.TSV.typeWithSubtype(), TextFormat.TSV, Map.of("header", "present|absent", "charset", "utf-8")) .build(); @@ -63,13 +63,12 @@ public MediaType getMediaType(RestRequest request, SqlQueryRequest sqlRequest) { return validateColumnarRequest(sqlRequest.columnar(), parser.fromMediaType(contentType)); } - private MediaType validateColumnarRequest(boolean requestIsColumnar, MediaType fromMediaType) { + private static MediaType validateColumnarRequest(boolean requestIsColumnar, MediaType fromMediaType) { if(requestIsColumnar && fromMediaType instanceof TextFormat){ throw new IllegalArgumentException("Invalid use of [columnar] argument: cannot be used in combination with " + "txt, csv or tsv formats"); } return fromMediaType; - } } diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParserTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParserTests.java index d2671c1748baf..0459b777b15f9 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParserTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParserTests.java @@ -37,6 +37,9 @@ public void testPlainTextDetection() { public void testCsvDetection() { MediaType text = parser.getMediaType(reqWithAccept("text/csv"), createTestInstance(false, Mode.PLAIN, false)); assertThat(text, is(CSV)); + + text = parser.getMediaType(reqWithAccept("text/csv; delimiter=x"), createTestInstance(false, Mode.PLAIN, false)); + assertThat(text, is(CSV)); } public void testTsvDetection() { @@ -44,7 +47,7 @@ public void testTsvDetection() { assertThat(text, is(TSV)); } - public void testParametersParsing() { + public void testMediaTypeDetectionWithParameters() { assertThat(parser.getMediaType(reqWithAccept("text/plain; charset=utf-8"), createTestInstance(false, Mode.PLAIN, false)), is(PLAIN_TEXT)); assertThat(parser.getMediaType(reqWithAccept("text/plain; header=present"),