Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow parsing Content-Type and Accept headers with version #61427

Merged
merged 43 commits into from
Oct 5, 2020
Merged

Allow parsing Content-Type and Accept headers with version #61427

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
7514d87
Allow parsing Content-Type and Accept headers with version
pgomulka Aug 21, 2020
921bc6f
code review follow up
pgomulka Sep 1, 2020
ee280ba
unused imports
pgomulka Sep 1, 2020
433de74
revert unrelated test
pgomulka Sep 2, 2020
51467e0
remove */* and method visibility
pgomulka Sep 2, 2020
ccaadbd
typos
pgomulka Sep 3, 2020
de13d4b
Merge branch 'master' into compat/content-type-header-with-version
pgomulka Sep 17, 2020
d9ee420
exted parsing with parameters
pgomulka Sep 17, 2020
41150a6
import
pgomulka Sep 17, 2020
2fc8f86
fix test
pgomulka Sep 17, 2020
734031a
scope parser
pgomulka Sep 18, 2020
c04af65
x-ndjson to be versioned
pgomulka Sep 18, 2020
474eb29
Merge branch 'master' into compat/content-type-header-with-version
pgomulka Sep 18, 2020
5c30813
Apply suggestions from code review
pgomulka Sep 18, 2020
7b760ba
precompile pattern
pgomulka Sep 18, 2020
5853561
import
pgomulka Sep 21, 2020
b6ebd63
removal of unused method and pattern in method
pgomulka Sep 21, 2020
ff6c94f
throwing exception when failing to parse
pgomulka Sep 22, 2020
b969f2b
checkstyle
pgomulka Sep 22, 2020
0684e58
test fixes
pgomulka Sep 23, 2020
b91d472
*/* header
pgomulka Sep 23, 2020
9edc49f
catch
pgomulka Sep 23, 2020
c5d8166
hacks around text/plain
pgomulka Sep 23, 2020
ad673b6
sout remove
pgomulka Sep 23, 2020
1ae6a60
precommit
pgomulka Sep 23, 2020
1ae63c6
fix nullpointer
pgomulka Sep 24, 2020
ee1e5a0
fix yml test
pgomulka Sep 24, 2020
9f4b6c3
fix yml test
pgomulka Sep 24, 2020
bd4dee1
throwing exception when failing to parse
pgomulka Sep 22, 2020
ce42f1b
Revert "throwing exception when failing to parse"
pgomulka Sep 24, 2020
0f8ae7b
Merge branch 'revert_strict_header_parsing' into compat/content-type-…
pgomulka Sep 24, 2020
c4f02ad
import
pgomulka Sep 28, 2020
176302e
fix storing response time in sql
pgomulka Sep 28, 2020
eab471c
return null on errors
pgomulka Sep 28, 2020
e128fd7
sqlmedia type parsing test - do not throw exceptions
pgomulka Sep 28, 2020
66435dd
remove todoes
pgomulka Sep 28, 2020
08f1395
rename argument
pgomulka Sep 28, 2020
3f3f0d5
review follow up
pgomulka Sep 29, 2020
427c391
remove pattern from signature
pgomulka Sep 30, 2020
e031605
fix regex
pgomulka Sep 30, 2020
4134d92
Apply suggestions from code review
pgomulka Oct 2, 2020
a976f5b
Apply suggestions from code review
pgomulka Oct 5, 2020
3e0eec0
relax delimiter validation
pgomulka Oct 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 61 additions & 24 deletions libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,33 +22,18 @@
import java.util.HashMap;
import java.util.Locale;
import java.util.Map;
import java.util.regex.Pattern;

public class MediaTypeParser<T extends MediaType> {
private final Map<String, T> formatToMediaType;
private final Map<String, T> typeWithSubtypeToMediaType;
private final Map<String, Map<String, Pattern>> parametersMap;

public MediaTypeParser(T[] acceptedMediaTypes) {
this(acceptedMediaTypes, Map.of());
}

public MediaTypeParser(T[] acceptedMediaTypes, Map<String, T> additionalMediaTypes) {
final int size = acceptedMediaTypes.length + additionalMediaTypes.size();
Map<String, T> formatMap = new HashMap<>(size);
Map<String, T> typeMap = new HashMap<>(size);
for (T mediaType : acceptedMediaTypes) {
typeMap.put(mediaType.typeWithSubtype(), mediaType);
formatMap.put(mediaType.format(), mediaType);
}
for (Map.Entry<String, T> 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<String, T> formatToMediaType, Map<String, T> typeWithSubtypeToMediaType,
Map<String, Map<String, Pattern>> parametersMap) {
this.formatToMediaType = Map.copyOf(formatToMediaType);
this.typeWithSubtypeToMediaType = Map.copyOf(typeWithSubtypeToMediaType);
this.parametersMap = Map.copyOf(parametersMap);
}

public T fromMediaType(String mediaType) {
Expand All @@ -65,6 +50,7 @@ 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
*/
Expand All @@ -75,9 +61,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<String, String> parameters = new HashMap<>();
for (int i = 1; i < split.length; i++) {
Expand All @@ -86,7 +74,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;
pgomulka marked this conversation as resolved.
Show resolved Hide resolved
}
parameters.put(parameterName, parameterValue);
}
return new ParsedMediaType(xContentType, parameters);
}
Expand All @@ -96,6 +89,17 @@ public ParsedMediaType parseMediaType(String headerValue) {
return null;
}

private boolean isValidParameter(String typeWithSubtype, String parameterName, String parameterValue) {
if (parametersMap.containsKey(typeWithSubtype)) {
Map<String, Pattern> parameters = parametersMap.get(typeWithSubtype);
if (parameters.containsKey(parameterName)) {
Pattern regex = parameters.get(parameterName);
return regex.matcher(parameterValue).matches();
}
}
return false;
}

private boolean hasSpaces(String s) {
return s.trim().equals(s) == false;
}
Expand All @@ -120,4 +124,37 @@ public Map<String, String> getParameters() {
return parameters;
}
}

public static class Builder<T extends MediaType> {
private final Map<String, T> formatToMediaType = new HashMap<>();
private final Map<String, T> typeWithSubtypeToMediaType = new HashMap<>();
private final Map<String, Map<String, Pattern>> parametersMap = new HashMap<>();

public Builder<T> withMediaTypeAndParams(String alternativeMediaType, T mediaType, Map<String, String> paramNameAndValueRegex) {
pgomulka marked this conversation as resolved.
Show resolved Hide resolved
typeWithSubtypeToMediaType.put(alternativeMediaType.toLowerCase(Locale.ROOT), mediaType);
formatToMediaType.put(mediaType.format(), mediaType);

Map<String, Pattern> parametersForMediaType = new HashMap<>(paramNameAndValueRegex.size());
for (Map.Entry<String, String> params : paramNameAndValueRegex.entrySet()) {
String parameterName = params.getKey().toLowerCase(Locale.ROOT);
String parameterRegex = params.getValue();
Pattern pattern = Pattern.compile(parameterRegex, Pattern.CASE_INSENSITIVE);
parametersForMediaType.put(parameterName, pattern);
}
parametersMap.put(alternativeMediaType, parametersForMediaType);

return this;
}

public Builder<T> copyFromMediaTypeParser(MediaTypeParser<? extends T> mediaTypeParser) {
formatToMediaType.putAll(mediaTypeParser.formatToMediaType);
typeWithSubtypeToMediaType.putAll(mediaTypeParser.typeWithSubtypeToMediaType);
parametersMap.putAll(mediaTypeParser.parametersMap);
return this;
}

public MediaTypeParser<T> build() {
return new MediaTypeParser<>(formatToMediaType, typeWithSubtypeToMediaType, parametersMap);
}
}
}
36 changes: 32 additions & 4 deletions libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand Down Expand Up @@ -113,9 +114,26 @@ public XContent xContent() {
}
};

public static final MediaTypeParser<XContentType> mediaTypeParser = new MediaTypeParser<>(XContentType.values(),
Map.of("application/*", JSON, "application/x-ndjson", JSON));

private static final String COMPATIBLE_WITH_PARAMETER_NAME = "compatible-with";
private static final String VERSION_PATTERN = "\\d+";
public static final MediaTypeParser<XContentType> mediaTypeParser = new MediaTypeParser.Builder<XContentType>()
.withMediaTypeAndParams("application/smile", SMILE, Collections.emptyMap())
.withMediaTypeAndParams("application/cbor", CBOR, Collections.emptyMap())
.withMediaTypeAndParams("application/json", JSON, Map.of("charset", "UTF-8"))
.withMediaTypeAndParams("application/yaml", YAML, Map.of("charset", "UTF-8"))
.withMediaTypeAndParams("application/*", JSON, Map.of("charset", "UTF-8"))
.withMediaTypeAndParams("application/x-ndjson", JSON, Map.of("charset", "UTF-8"))
.withMediaTypeAndParams("application/vnd.elasticsearch+json", JSON,
Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", "UTF-8"))
.withMediaTypeAndParams("application/vnd.elasticsearch+smile", SMILE,
Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", "UTF-8"))
.withMediaTypeAndParams("application/vnd.elasticsearch+yaml", YAML,
Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", "UTF-8"))
.withMediaTypeAndParams("application/vnd.elasticsearch+cbor", CBOR,
Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", "UTF-8"))
.withMediaTypeAndParams("application/vnd.elasticsearch+x-ndjson", JSON,
Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", "UTF-8"))
.build();
Copy link
Contributor Author

@pgomulka pgomulka Sep 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the discussion:
defining versioned media types up front, means that we won't throw an exception if someone specifies it with oss licence
if he uses oss on v8 server for api that was removed and provides application/vnd.elasticsearch+json;compatible-with=7 he will get a 404

is there an easy way to make XContentType plugin aware? or licence aware?
or are we ok with allowing to use versioned media types with oss?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a 404 (or maybe 400) is the right thing to do here since the infrastructure for version support is in OSS and we do not have a compatible handler for V7.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but this also would affect plugins like SQL. In oss we don't know about MediaTypes which are defined over there
if we throw an exception for a unknown type I am worried we might end up with a logic based on exceptions

try XContentType.parseMediaType
try XContentType.parseFormat
try TextFormat.parseMediaType
try TextFormat.parseFormat

@bpintea any thoughts on this? I guess SQL had to face this in the past

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way to register a custom parser ? or just add the SQL values to the core parser and specificy which rule set to use when parsing ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is an interesting idea.. We could have something like a SqlMediaTypeParser and it would either return TextFormat or XContentType. The parsing logic would remain the same in the MediaTypeParser. The idea of SqlMediaTypeParser woudl be just to abstract on the setup of the parser.
The same could be done for CompatibleApiMediaTypeParser and we could keep it in xpack plugin.
this does not solve the problem of trying to parse a format, then if failed to parse a media type. I think we should try to just check for presence of the format parameter and if present to used. Otherwise trying to use Accept Header.
I think also that Content-Type should not be used for response formatting in SQL plugin. It looks like it is allowed in code, but I don't think it would work (it would fail when parsing that header in server)


/**
* Accepts a format string, which is most of the time is equivalent to {@link XContentType#subtype()}
Expand All @@ -137,13 +155,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<XContentType>.ParsedMediaType parsedMediaType = mediaTypeParser.parseMediaType(mediaType);
if (parsedMediaType != null) {
String version = parsedMediaType
.getParameters()
.get(COMPATIBLE_WITH_PARAMETER_NAME);
return version != null ? Byte.parseByte(version) : null;
}
return null;
}

public int index() {
return index;
}
Expand Down
28 changes: 17 additions & 11 deletions libs/x-content/src/test/java/org/elasticsearch/common/xcontent/MediaTypeParserTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,40 +29,46 @@
import static org.hamcrest.Matchers.nullValue;

public class MediaTypeParserTests extends ESTestCase {
MediaTypeParser<XContentType> mediaTypeParser = XContentType.mediaTypeParser;

MediaTypeParser<XContentType> mediaTypeParser = new MediaTypeParser.Builder<XContentType>()
.withMediaTypeAndParams("application/vnd.elasticsearch+json",
XContentType.JSON, Map.of("compatible-with", "\\d+",
"charset", "UTF-8"))
.build();

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),
is(nullValue()));
}

public void testInvalidParameters() {
String mediaType = "application/json";
String mediaType = "application/vnd.elasticsearch+json";
assertThat(mediaTypeParser.parseMediaType(mediaType + "; charset=unknown") ,
is(nullValue()));
assertThat(mediaTypeParser.parseMediaType(mediaType + "; keyvalueNoEqualsSign"),
is(nullValue()));

assertThat(mediaTypeParser.parseMediaType(mediaType + "; key = value"),
is(nullValue()));
assertThat(mediaTypeParser.parseMediaType(mediaType + "; key=") ,
Expand Down
2 changes: 1 addition & 1 deletion server/src/main/java/org/elasticsearch/ElasticsearchException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
8 changes: 6 additions & 2 deletions server/src/main/java/org/elasticsearch/rest/AbstractRestChannel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -36,6 +38,7 @@

public abstract class AbstractRestChannel implements RestChannel {

private static final Logger logger = LogManager.getLogger(AbstractRestChannel.class);
private static final Predicate<String> INCLUDE_FILTER = f -> f.charAt(0) != '-';
private static final Predicate<String> EXCLUDE_FILTER = INCLUDE_FILTER.negate();

Expand Down Expand Up @@ -98,8 +101,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 (Strings.hasText(format)) {
responseContentType = XContentType.fromFormat(format);
}
if (responseContentType == null) {
responseContentType = XContentType.fromMediaType(acceptHeader);
}
Expand Down
18 changes: 9 additions & 9 deletions server/src/main/java/org/elasticsearch/rest/BytesRestResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}.
Expand All @@ -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 {
Expand All @@ -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));
Expand All @@ -118,7 +118,7 @@ public BytesRestResponse(RestChannel channel, RestStatus status, Exception e) th

@Override
public String contentType() {
return this.contentType;
return this.responseMediaType;
}

@Override
Expand Down
4 changes: 2 additions & 2 deletions server/src/main/java/org/elasticsearch/rest/action/cat/RestTable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,14 +51,14 @@ 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"));
}
Expand Down
Loading