Forbid direct usage of ContentType.create() methods #26457
Conversation
| */ | ||
| @SuppressForbidden(reason = "Only allowed place to convert a XContentType to a ContentType") | ||
| static ContentType createContentType(final XContentType xContentType) { | ||
| return ContentType.create(xContentType.mediaTypeWithoutParameters(), (Charset) null); |
There was a problem hiding this comment.
Any reason why you didn't use UTF8 here? Glossing over #22769 I thought we might make this a requirement at some point in the future, but maybe I got that wrong.
There was a problem hiding this comment.
I don't want to force a charset because Elasticsearch exposes many ways to set the source of, let's say, an IndexRequest. It can be a Map converted by Jackson as UTF-8 bytes, but it can also be raw bytes too... And Elasticsearch will rely on Jackson auto-detect encoding to parse this correctly.
I think there are many things to dig here and in the meanwhile setting a null charset allows to keep the current behavior.
| @@ -139,8 +141,8 @@ static Request bulk(BulkRequest bulkRequest) throws IOException { | |||
| bulkContentType = XContentType.JSON; | |||
| } | |||
|
|
|||
| byte separator = bulkContentType.xContent().streamSeparator(); | |||
| ContentType requestContentType = ContentType.create(bulkContentType.mediaType()); | |||
There was a problem hiding this comment.
As a side note, I wonder how useful XContentType.mediaType() still is, except for JSON it redirects to mediaTypeWithoutParameters(). Maybe worth investigating if we can get rid of it in another issue.
There was a problem hiding this comment.
I agree - I'm currently looking at this.
There was a problem hiding this comment.
Would mix it with this PR though.
It's easy to create a wrong Content-Type header when converting a XContentType to a Apache HTTP ContentType instance. This commit the direct usages of ContentType.create() methods in favor of a Request.createContentType(XContentType) method that does the right thing. Closes #26438
It's easy to create a wrong Content-Type header when converting a XContentType to a Apache HTTP ContentType instance. This commit the direct usages of ContentType.create() methods in favor of a Request.createContentType(XContentType) method that does the right thing. Closes #26438
It's easy to create a wrong Content-Type header when converting a XContentType to a Apache HTTP ContentType instance. This commit the direct usages of ContentType.create() methods in favor of a Request.createContentType(XContentType) method that does the right thing. Closes #26438
|
Thanks @cbuescher ! |
It's easy to create a wrong
Content-Typeheader when converting aXContentTypeto a Apache HTTPContentTypeinstance.We sometimes did it in the high level client, where we use
ContentType.create(XContentType.JSON.mediaType())instead of
ContentType.create(XContentType.JSON.mediaTypeWithoutParameters())But the
create(String)method expects a valid MIME type to be passed, and not a value that includes something else like a charset parameter. With XContentType.JSON, the first one builds aContent-Type: application/json; charset=UTF-8header and the second aContent-Type: application/json. There are both accepted by Elasticsearch because charsets are silently ignored (see #22769). But a new check was added in Apache HTTP Core version 4.4.6 (see #26438 (comment)) and that will just throw IAE. Some users already ran into this issue.So this pull request forbids the direct usages of
ContentType.create()methods in favor of aRequest.createContentType(XContentType)method that does the right thing.