Media-type parser #61987

pgomulka · 2020-09-04T13:17:00Z

Splitting method XContentType.fromMediaTypeOrFormat into two separate methods. This will help to validate media type provided in Accept or Content-Type headers.
Extract parsing logic from XContentType (fromMediaType and fromFormat methods) to a separate MediaTypeParser class. This will help reuse the same parsing logic for XContentType and TextFormat (used in sql)

Media-Types type/subtype; parameters parsing is in defined https://tools.ietf.org/html/rfc7231#section-3.1.1.1

based on #61845

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

elasticmachine · 2020-09-04T13:17:02Z

Pinging @elastic/es-core-infra (:Core/Infra/REST API)

pgomulka · 2020-09-07T13:45:37Z

@elasticmachine run elasticsearch-ci/2

pgomulka · 2020-09-07T13:49:12Z

@elasticmachine run elasticsearch-ci/packaging-sample-windows

bpintea

The SQL related changes look good, but I wonder if we can't simplify the change a bit.

bpintea · 2020-09-08T14:24:02Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java

+        XContentType acceptHeader = null;
+        XContentType contentTypeHeader = null;


Wondering if this media-type origin split is strictly necessary: the previous code tried to fetch a token from somewhere - in order of preference: enforced format (CBOR), format URL attribute, Accept header, Content-Type header and repeatedly checking accept against null - then validate that token against a media type or as a format (#fromMediaTypeOrFormat()).

The new code checks if it's a format (in getXContentType() @ L143), or then if it's a media type, by one of these XContentType instances (@ L147). But the source preferences is already stored in the way the accept member is set (by those subsequent null checks), so could we not simply serially invoke #fromMediaType(accept) and #fromFormat(accept) and return the first non-null value to simplify the code? Similar to what we do in TextFormat#fromMediaTypeOrFormat().

good point, I have refactored this as per your suggestions

bpintea · 2020-09-08T14:26:33Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/TextFormat.java

@@ -32,7 +34,7 @@
 /**
 * Templating class for displaying SQL responses in text formats.
 */
-enum TextFormat {
+enum TextFormat implements MediaType {


Is the extension necessary?

in order to use a MediaType parser for both TextFormat and XContentType i had to introduce an interface that is implemented by both

Right. Thanks.

bpintea · 2020-09-08T14:29:06Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java

@@ -92,7 +97,8 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli
         * that doesn't parse it'll throw an {@link IllegalArgumentException}
         * which we turn into a 400 error.
         */
-        XContentType xContentType = accept == null ? XContentType.JSON : XContentType.fromMediaTypeOrFormat(accept);
+        //TODO PG this all logic needs a review from SQL team


remove reminder.

bpintea · 2020-09-08T14:35:19Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/TextFormat.java

@@ -197,6 +204,7 @@ String maybeEscape(String value, Character delimiter) {
        boolean hasHeader(RestRequest request) {
            String header = request.param(URL_PARAM_HEADER);
            if (header == null) {
+                //TODO PG in most places we only assume one accept header


Good to know. Having multiple header fields with same name is legal though, I guess we should leave it as is.

…rch into header_version_split

jakelandis · 2020-09-09T15:05:34Z

...in/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/text/TextTemplateEngine.java

@@ -82,7 +82,8 @@ private XContentType detectContentType(String content) {
            //There must be a __<content_type__:: prefix so the minimum length before detecting '__::' is 3
            int endOfContentName = content.indexOf("__::", 3);
            if (endOfContentName != -1) {
-                return XContentType.fromMediaTypeOrFormat(content.substring(2, endOfContentName));
+                //TODO PG what do we expect here?


You can remove this todo, answered in another comment.

jaymode · 2020-09-10T16:43:31Z

libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaType.java

+
+package org.elasticsearch.common.xcontent;
+
+public interface MediaType {


can you add javadocs to the class and methods?

jaymode · 2020-09-10T16:45:46Z

libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeParser.java

+    }
+
+    /**
+     * parsing media type that follows https://tools.ietf.org/html/rfc2616#section-3.7


shouldn't we base this on https://tools.ietf.org/html/rfc7231#section-3.1.1.1

you are right. the rfc2616 is obsolete.
rfc7231 is the updated one

jaymode · 2020-09-10T16:52:04Z

libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeParser.java

+                    Map<String, String> parameters = new HashMap<>();
+                    for (int i = 1; i < split.length; i++) {
+                        String[] keyValueParam = split[i].trim().split("=");
+                        // should we validate that there are no spaces between key = value?


per the RFC, it is not allowed so I guess we should

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java

bpintea

The SQL bits LGTM.

…ugin/RestSqlQueryAction.java Co-authored-by: Bogdan Pintea <[email protected]>

…rch into header_version_split

pgomulka · 2020-09-15T06:45:24Z

@elasticmachine run elasticsearch-ci/bwc

pgomulka · 2020-09-15T06:45:33Z

@elasticmachine run elasticsearch-ci/default-distro

jakelandis · 2020-09-15T13:18:47Z

libs/x-content/src/test/java/org/elasticsearch/common/xcontent/MediaTypeParserTests.java

+            is(nullValue()));
+
+        assertThat(mediaTypeParser.parseMediaType(mediaType + "; key = value"),
+            is(nullValue()));


can you add a test for assertThat(mediaTypeParser.parseMediaType(mediaType + "; key=") I think it will error with index out of bounds.

this would mean that the result of String[] keyValueParam = split[i].trim().split("="); has length = 1
it is checked in a next line.

if (keyValueParam.length != 2 || hasSpaces(keyValueParam[0]) || hasSpaces(keyValueParam[1])) {

added a testcase

jakelandis · 2020-09-15T13:21:59Z

libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeParser.java

+
+    /**
+     * A media type object that contains all the information provided on a Content-Type or Accept header
+     * // TODO PG to be extended with getCompatibleAPIVersion and more


can you leave these specific TODO's out. IMO TODO's left in the code base should be rare and general enough such that any future dev can understand and implement it.

agree. removed the todo

jakelandis · 2020-09-15T13:23:42Z

libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaType.java

+
+    /**
+     * Returns a corresponding format for a MediaType. i.e. json for application/json media type
+     * Can differ from the MediaType's subtype i.e plain/text but format is txt


jakelandis

LGTM, thanks for iterations. (a couple very minor new comments, with this PR or in the future)

pgomulka · 2020-09-16T16:29:48Z

@elasticmachine run elasticsearch-ci/1
failed test is being fixed in #62411

jaymode

LGTM

libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaType.java

libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeParser.java

Co-authored-by: Jay Modi <[email protected]>

This reverts commit 86ba732

pgomulka added 11 commits September 2, 2020 14:03

Split responsibility for format parsing

5e42390

parse * and ndjson

01f001d

make format not accepting applicaiton/

50a88a0

post data request should parse applicaiton/json style

fb03ffd

unused import

09f281e

fix sql parsing

8bc024f

split format and accept header

070508c

fix and todos

3c7ab16

Merge branch 'master' into xcontent_format_parsing

59a7f42

media type parser

968b1c9

media type parser

46f8f33

pgomulka added WIP :Core/Infra/REST API REST infrastructure and utilities labels Sep 4, 2020

pgomulka self-assigned this Sep 4, 2020

elasticmachine added the Team:Core/Infra Meta label for core/infra team label Sep 4, 2020

pgomulka added 5 commits September 4, 2020 16:33

precommit

cbbe093

rename and null check

222caee

Merge branch 'master' into header_version_split

6bdec13

Merge branch 'master' into header_version_split

ee97564

fix text format parsing

7f52e11

Merge branch 'master' into header_version_split

63db70c

pgomulka requested review from bpintea and jaymode September 8, 2020 11:48

pgomulka changed the title ~~WIP Media-type parser~~ Media-type parser Sep 8, 2020

pgomulka removed the WIP label Sep 8, 2020

pgomulka requested a review from jakelandis September 8, 2020 13:53

bpintea reviewed Sep 8, 2020

View reviewed changes

Merge branch 'header_version_split' of github.com:pgomulka/elasticsea…

90e798d

…rch into header_version_split

jakelandis reviewed Sep 9, 2020

View reviewed changes

jaymode reviewed Sep 10, 2020

View reviewed changes

bpintea reviewed Sep 10, 2020

View reviewed changes

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java Outdated Show resolved Hide resolved

bpintea approved these changes Sep 10, 2020

View reviewed changes

pgomulka and others added 8 commits September 14, 2020 11:22

javadoc and validation

fa49be4

Update x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/pl…

31d92ac

…ugin/RestSqlQueryAction.java Co-authored-by: Bogdan Pintea <[email protected]>

Merge branch 'header_version_split' of github.com:pgomulka/elasticsea…

a925fbe

…rch into header_version_split

javadoc fix

23c4e41

remove shortName

b1e3fb1

javadoc fix

c17a895

fix compile error

7d6bd08

fix test compile

3c93954

Merge branch 'master' into header_version_split

77068a8

jakelandis reviewed Sep 15, 2020

View reviewed changes

jakelandis approved these changes Sep 15, 2020

View reviewed changes

Merge branch 'master' into header_version_split

8fb0cd4

remove todo and a testcase

63fb6c7

jaymode approved these changes Sep 16, 2020

View reviewed changes

libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaType.java Outdated Show resolved Hide resolved

libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeParser.java Outdated Show resolved Hide resolved

Apply suggestions from code review

4598a0a

Co-authored-by: Jay Modi <[email protected]>

pgomulka merged commit 86ba732 into elastic:master Sep 17, 2020

This was referenced Sep 17, 2020

Split responsibility for format parsing #61845

Closed

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

Merged

jakelandis added a commit to jakelandis/elasticsearch that referenced this pull request Oct 19, 2020

Revert "Media-type parser (elastic#61987)"

6d4820a

This reverts commit 86ba732

ChrisHegarty unassigned pgomulka Oct 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Media-type parser #61987

Media-type parser #61987

pgomulka commented Sep 4, 2020 •

edited

Loading

elasticmachine commented Sep 4, 2020

pgomulka commented Sep 7, 2020

pgomulka commented Sep 7, 2020

bpintea left a comment

bpintea Sep 8, 2020

pgomulka Sep 9, 2020

bpintea Sep 8, 2020

pgomulka Sep 9, 2020

bpintea Sep 10, 2020

bpintea Sep 8, 2020

bpintea Sep 8, 2020

jakelandis Sep 9, 2020

jaymode Sep 10, 2020

jaymode Sep 10, 2020

pgomulka Sep 14, 2020

jaymode Sep 10, 2020

bpintea left a comment

pgomulka commented Sep 15, 2020

pgomulka commented Sep 15, 2020

jakelandis Sep 15, 2020

pgomulka Sep 16, 2020

jakelandis Sep 15, 2020

pgomulka Sep 16, 2020

jakelandis Sep 15, 2020

jakelandis left a comment

pgomulka commented Sep 16, 2020

jaymode left a comment

		XContentType acceptHeader = null;
		XContentType contentTypeHeader = null;


		package org.elasticsearch.common.xcontent;

		public interface MediaType {

Media-type parser #61987

Media-type parser #61987

Conversation

pgomulka commented Sep 4, 2020 • edited Loading

elasticmachine commented Sep 4, 2020

pgomulka commented Sep 7, 2020

pgomulka commented Sep 7, 2020

bpintea left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpintea left a comment

Choose a reason for hiding this comment

pgomulka commented Sep 15, 2020

pgomulka commented Sep 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakelandis left a comment

Choose a reason for hiding this comment

pgomulka commented Sep 16, 2020

jaymode left a comment

Choose a reason for hiding this comment

pgomulka commented Sep 4, 2020 •

edited

Loading