Introduce per REST endpoint media types #64406

pgomulka · 2020-10-30T11:50:10Z

Per REST endpoint media types declaration allows to make parsing/validation more strict.

If a media type was declared only in one endpoint (for instance CSV in SQL endpoint) it should not be allowed to parse that media type when using other endpoints.
However, the Compatible API need to be able to understand all media types supported by Elasticsearch in order to parse a compatible-with=version parameter.
This implies that endpoints need to declare which media type they support and how to parse them (if introducing new media types - like SQL).

How to parse:
MediaType interface still serves as an abstraction on top of XContentType and TextFormat. It also has a declaration of mappings String-MediaType with parameters. Parameters declares the names of parameters and regex to validate its values.
This instructs how to perform the parsing. For instance - XContentType.JSON has the mapping of application/vnd.elasticsearch+json -> JSON and allows parameters compatible-with=\d and charset=utf-8

MediaTypeParser was simplified into ParsedMediaType class with static factory method for parsing.

How to declare:
RestHandler interface is extended with a validAcceptMediaTypes which returns a MediaTypeRegistry - a class that encapsulates mappings of string (type/subtype) to MediaType, allowed parameters and formatPathParameter values.
We only need to allow of declaration of valid media types for Accept header. Content-Type valid media types are fixed to XContentType instances - json, yaml, smile, cbor.

relates #51816

…ionContext:L122

elasticmachine · 2020-10-30T11:50:12Z

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

jakelandis

changes are looking good. most of the comments are mostly around naming or minor things.

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

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

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

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

server/src/main/java/org/elasticsearch/rest/RestRequest.java

test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestResponse.java

jakelandis

looking good a couple more comments.

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

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

jakelandis · 2020-11-03T20:26:23Z

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

+                    if (paramsAsString.isEmpty()) {
+                        continue;
+                    }
+                    // intentionally allowing to have spaces around `=` (RFC disallows this)


The only mention I saw in the spec was " No whitespace is allowed between the header field-name and colon. "

The field value is not covered by this mandate:

header-field = field-name ":" OWS field-value OWS

This does not explicitly state that the field-value can not have whitespace. For example, "application/<space>json" isn't forbidden by the HTTP spec (spaces are allowed in the field-value). The correctness of spaces in mimetypes or parameters would be defined by https://tools.ietf.org/html/rfc6838 (media type spec). While it is pretty clear that "application/<space>json" != "application/json", I think we should still allow it since that is a nightmare to troubleshoot. Also the spec is not clear on if ;<space>param<space>=<space>value is acceptable. I think it actually is.

I would suggest to remove the "RFC disallows this".

by RFC I meant https://tools.ietf.org/html/rfc7231#section-3.1.1.1 where it refers to the rfc6838 (the one you quoted) and makes it more strict

Internet media types ought to be registered with IANA according to the procedures defined in [BCP13]. Note: Unlike some similar constructs in other header fields, media type parameters do not allow whitespace (even "bad" whitespace) around the "=" character.

[BCP13] Freed, N., Klensin, J., and T. Hansen, "Media Type
Specifications and Registration Procedures", BCP 13,
RFC 6838, January 2013.

I missed that, sorry. You were right, probably shouldn't be lenient here, but only with the whitespace around the =. The code here is fine, but a follow up (or on this PR) to remove adhere to that would probably be best. sorry for steering you in the wrong direction on this.

I will add validation around =

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

jakelandis

Thanks for iterations. LGTM

jaymode

I left some formatting nits and a request to expand on code level comments in one spot. LGTM

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

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

server/src/main/java/org/elasticsearch/rest/RestHandler.java

server/src/main/java/org/elasticsearch/rest/RestRequest.java

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

jaymode · 2020-11-04T22:15:47Z

server/src/main/java/org/elasticsearch/rest/RestRequest.java

@@ -104,6 +106,23 @@ private RestRequest(NamedXContentRegistry xContentRegistry, Map<String, String>
        this.requestId = requestId;
    }

+    private static @Nullable ParsedMediaType parseHeaderWithMediaType(Map<String, List<String>> headers, String headerName) {
+        //TODO: make all usages of headers case-insensitive


this is a not well documented fact, but by default Netty uses a case insensitive hashing strategy for retrieval of headers. Checkout the DefaultHttpHeaders class if you're interested. Due to our use of Netty for HTTP parsing we also get this benefit in the nio transport. Also, see the Netty4HttpRequest/NioHttpRequest.HttpHeadersMap class.

Interesting, I did not know about this.
I wrongly relied on tests only using map from java collections.
Maybe we might want to refactor usages of java.util.Map to an interface that documents/enforces case insensitivity?
I will leave this todo for now so that we can reconsider this in the future

jaymode · 2020-11-04T22:17:47Z

test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestResponse.java

@@ -71,6 +72,14 @@ public ClientYamlTestResponse(Response response) throws IOException {
        }
    }

+    private XContentType getContentTypeIgnoreExceptions(String contentType) {


I'd love if we can move the comment from the usage to a method level documentation that expands on why we use null; I get it now but at some point it won't be obvious anymore

true, will add more comment on per method javadoc

Co-authored-by: Jay Modi <jaymode@users.noreply.github.com>

…m:pgomulka/elasticsearch into compat/introduce_per_endpoint_media_types

Per https://tools.ietf.org/html/rfc7231#section-3.1.1.1 MediaType can have spaces around parameters pair, but do not allow spaces within parameter. That means that parameter pair forbids spaces around `=` follow up after elastic#64406 (comment) relates elastic#51816

Per https://tools.ietf.org/html/rfc7231#section-3.1.1.1 MediaType can have spaces around parameters pair, but do not allow spaces within parameter. That means that parameter pair forbids spaces around = follow up after #64406 (comment) relates #51816

This adds a (branch-specific) media type parser as a near-drop-in replacement for the (master-only) per-REST-endpoint media types parser (elastic#64406).

This adds a (branch-specific) media type parser as a near-drop-in replacement for the (master-only) per-REST-endpoint media types parser (#64406).

pgomulka added 3 commits October 30, 2020 10:28

Introduce per endpoint media types

e468fdf

Merge branch 'master' into compat/parsed_media_type

5cfa11e

allow smile and cbor to parse charset param. See ClientYamlTestExecut…

66224e7

…ionContext:L122

pgomulka added :Core/Infra/REST API REST infrastructure and utilities v8.0.0 labels Oct 30, 2020

pgomulka self-assigned this Oct 30, 2020

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

pgomulka added 2 commits October 30, 2020 13:37

javadocs

29b27e8

fix javadoc

4690362

pgomulka requested review from jakelandis and jaymode October 30, 2020 14:23

jakelandis reviewed Nov 2, 2020

View reviewed changes

pgomulka added 5 commits November 3, 2020 13:15

code review follow up

2bda74f

minor tweaks

7779083

Merge branch 'master' into compat/introduce_per_endpoint_media_types

dd26408

fix test after exception msg rename

94b4a0a

rename to header value

4385591

jakelandis reviewed Nov 3, 2020

View reviewed changes

remove charset validation

dc61731

jakelandis approved these changes Nov 4, 2020

View reviewed changes

jaymode approved these changes Nov 4, 2020

View reviewed changes

pgomulka and others added 4 commits November 5, 2020 08:33

Apply suggestions from code review

4a3138d

Co-authored-by: Jay Modi <jaymode@users.noreply.github.com>

javadoc

0b565e5

Merge branch 'master' into compat/introduce_per_endpoint_media_types

b35ad2c

Merge branch 'compat/introduce_per_endpoint_media_types' of github.co…

b908bfe

…m:pgomulka/elasticsearch into compat/introduce_per_endpoint_media_types

pgomulka merged commit c219e17 into elastic:master Nov 5, 2020

pgomulka mentioned this pull request Nov 5, 2020

Do not allow spaces within MediaType's parameters #64650

Merged

pgomulka mentioned this pull request Nov 16, 2020

Compatible API header parsing plugin #60516

Closed

bpintea mentioned this pull request Jun 15, 2021

SQL: Add a media type parser for SQL requests #74116

Merged

bpintea added a commit that referenced this pull request Jun 21, 2021

SQL: Add a media type parser for SQL requests (#74116)

a9bbebe

This adds a (branch-specific) media type parser as a near-drop-in replacement for the (master-only) per-REST-endpoint media types parser (#64406).

jakelandis added v8.0.0-alpha1 >refactoring and removed v8.0.0 labels Jul 26, 2021

pgomulka mentioned this pull request Jan 24, 2022

Allow for customised content-type validation #80906

Merged

ldematte mentioned this pull request Oct 25, 2024

Use charset from Content-Type header #22769

Open

ChrisHegarty unassigned pgomulka Oct 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce per REST endpoint media types #64406

Introduce per REST endpoint media types #64406

pgomulka commented Oct 30, 2020 •

edited

Loading

elasticmachine commented Oct 30, 2020

jakelandis left a comment

jakelandis left a comment

jakelandis Nov 3, 2020

pgomulka Nov 4, 2020 •

edited

Loading

jakelandis Nov 4, 2020

pgomulka Nov 4, 2020

jakelandis left a comment

jaymode left a comment

jaymode Nov 4, 2020

pgomulka Nov 5, 2020

jaymode Nov 4, 2020

pgomulka Nov 5, 2020

Introduce per REST endpoint media types #64406

Introduce per REST endpoint media types #64406

Conversation

pgomulka commented Oct 30, 2020 • edited Loading

elasticmachine commented Oct 30, 2020

jakelandis left a comment

Choose a reason for hiding this comment

jakelandis left a comment

Choose a reason for hiding this comment

jakelandis Nov 3, 2020

Choose a reason for hiding this comment

pgomulka Nov 4, 2020 • edited Loading

Choose a reason for hiding this comment

jakelandis Nov 4, 2020

Choose a reason for hiding this comment

pgomulka Nov 4, 2020

Choose a reason for hiding this comment

jakelandis left a comment

Choose a reason for hiding this comment

jaymode left a comment

Choose a reason for hiding this comment

jaymode Nov 4, 2020

Choose a reason for hiding this comment

pgomulka Nov 5, 2020

Choose a reason for hiding this comment

jaymode Nov 4, 2020

Choose a reason for hiding this comment

pgomulka Nov 5, 2020

Choose a reason for hiding this comment

pgomulka commented Oct 30, 2020 •

edited

Loading

pgomulka Nov 4, 2020 •

edited

Loading