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 1 commit
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
38 changes: 35 additions & 3 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 @@ -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}.
Expand Down Expand Up @@ -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+))?",
Copy link
Contributor

@sethmlarson sethmlarson Aug 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Regex needs a few more escapes:

  • Escape the . within vnd.elasticsearch+
  • Escape the / between the type and subtype

(application|text)\\/(vnd\\.elasticsearch\\+)?([^;]+)(\\s*;\\s*compatible-with=(\\d+))?

Copy link
Member

@dakrone dakrone Aug 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Something I am curious about is the (\\d+))? at the end. If possible, it would be nice to define this as part of our "spec". This would be something we could be strict (eschew leniency!) about, such as enforcing it to be in a specific format (and throwing an exception when someone specifies a pattern that is not correct. We can also put the format into the documentation.

If you agree with the sentiment, I would be curious what we expect the final number to be, for example, which of these do we intend to support (now and in the future):

  • compatible-with=7
  • compatible-with=7.11
  • compatible-with=7.x
  • compatible-with=7x
  • compatible-with=7.11.2

Then I think we should make this strict so users find out early if they've send an illegal value

Copy link
Contributor

Choose a reason for hiding this comment

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

@sethmlarson Why does the forward slash needs escaping? It's not special in Java?

I wondering if we should anchor the pattern with ^ at the start and $ at the end, to be on the safe side?

In ([^;]+)(\\s*;, the [^;]+ will also consume whitespace, so I believe you can remove the \\s*.

Can we switch any of these capture groups to non-capturing if we don't actually need the contents, just the grouping? i.e (?:<pattern>)

Copy link
Contributor

Choose a reason for hiding this comment

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

@pugnascotia You're right the / doesn't need an escape in Java

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sethmlarson fixed the \\.
@dakrone Only major versions are supported, hence only digits are allowed - representing the major part of the version #51816 (no dots or any other character, preventing parsing minor version)
Good point on validation - how much should be done, how precise the exception should be.. @jakelandis or @jaymode any views on this?
@pugnascotia good point on consuming a space. The subtype (json, yaml, etc) are not allowing spaces. Added the anchoring and skipped capturing when possible. I think all of the groups require capturing though (except maybe for the optional ; charset=UTF-8)

Copy link
Member

Choose a reason for hiding this comment

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

(vnd.elasticsearch\+)?

Am I reading this correctly that the vnd.elasticsearch part is completely optional? I thought the intent was to only add compatible-with when vnd.elasticsearch is present?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right. This is possibly a bit oversimplified.
Do you think we should aim to be more strict on the regex side or should we validate the values after the parsing with more relaxed regex?

The more strict could look like

Pattern.compile(
        "(application|text)/((vnd\\.elasticsearch\\+(json|smile|yaml|cbor)(\\s*;\\s*compatible-with=(\\d+)))" +
            "|(json|smile|yaml|cbor))");

there is a simplistic validation in parseVerion method, but it still allows to add compatible-with even when subtype vnd.elasticsearch is not present. The version won't be parsed though

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;
}
Expand All @@ -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("*/*")) {
jaymode marked this conversation as resolved.
Show resolved Hide resolved
return JSON;
}

Expand All @@ -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)) {
Expand All @@ -157,6 +167,28 @@ public static XContentType fromMediaType(String mediaType) {
return null;
}

//public scope needed for text formats hack
jaymode marked this conversation as resolved.
Show resolved Hide resolved
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) + ";") ||
Expand Down
53 changes: 53 additions & 0 deletions server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,4 +84,57 @@ public void testFromRubbish() throws Exception {
assertThat(XContentType.fromMediaTypeOrFormat("text/plain"), nullValue());
assertThat(XContentType.fromMediaTypeOrFormat("gobbly;goop"), nullValue());
}

public void testMediaType() {
pgomulka marked this conversation as resolved.
Show resolved Hide resolved
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());
}
}