Enforce Content-Type requirement on the rest layer and remove deprecated methods #23146
Conversation
…ted methods This commit enforces the requirement of Content-Type for the REST layer and removes the deprecated methods in transport requests and their usages. While doing this, it turns out that there are many places where *Entity classes are used from the apache http client libraries and many of these usages did not specify the content type. The methods that do not specify a content type explicitly have been added to forbidden apis to prevent more of these from entering our code base. Relates elastic#19388
jaymode
added
:Core/Infra/Transport API
|
Please add breaking changes docs |
Thanks for the reminder. Breaking changes docs were added as part of #22691 and I cleaned them up some more. |
| @defaultMessage Explicitly specify the ContentType of HTTP entities | ||
| org.apache.http.entity.StringEntity#<init>(java.lang.String) | ||
| org.apache.http.entity.StringEntity#<init>(java.lang.String,java.lang.String) | ||
| org.apache.http.entity.StringEntity#<init>(java.lang.String,java.nio.charset.Charset) |
There was a problem hiding this comment.
should we also add ByteArrayEntity here? what is the difference between the http-signatures file below and this one?
There was a problem hiding this comment.
++ I should add all of those here. The difference is the http-signatures file is used only for the rest projects as they do not use the es-test-signatures.
I could move the http signatures to the buildSrc and then have the default forbidden apis for tests include that file as well. This way we only need to update the file in one place.
There was a problem hiding this comment.
I think I would do that, unless there are downsides that I am not seeing
| @@ -431,7 +432,7 @@ private HttpUriRequest performRandomRequest(String method) throws Exception { | |||
| HttpEntity entity = null; | |||
| boolean hasBody = request instanceof HttpEntityEnclosingRequest && getRandom().nextBoolean(); | |||
| if (hasBody) { | |||
| entity = new StringEntity(randomAsciiOfLengthBetween(10, 100)); | |||
| entity = new StringEntity(randomAsciiOfLengthBetween(10, 100), ContentType.APPLICATION_JSON); | |||
| " upgrade easier"); | ||
| } | ||
| return true; | ||
| }, Property.NodeScope, Property.Deprecated); |
There was a problem hiding this comment.
do we have a test for this? just making sure that a 5.x cluster having the setting set to false can upgrade to 6.0.
There was a problem hiding this comment.
I can add this as a setting for a node in the rolling-upgrade tests?
| deprecated[5.3.0, Provide the proper Content-Type header] | ||
| The type of the content sent in a request body must be specified using | ||
| the `Content-Type` header. The value of this header must map to one of | ||
| the supported formats (JSON, YAML, CBOR, SMILE, and NDJSON). |
There was a problem hiding this comment.
we do throw error when one sets CBOR or YAML using _bulk, or NDJSON using e.g. search ?
|
|
||
| The default value is `false`. | ||
| Additionally, when using the `source` query string parameter the | ||
| content type should be specified using the `source_content_type` query |
| @@ -19,6 +19,9 @@ In previous versions of Elasticsearch, having a proper Content-Type for the data | |||
| Elasticsearch 6.0.0 enforces that all requests with a body must have a supported Content-Type and this type will | |||
| be used when parsing the data. | |||
|
|
|||
| When using the `source` query string parameter, the `source_content_type` parameter must also be specified with | |||
| the media type of the source. | |||
|
|
|||
There was a problem hiding this comment.
shall we add a bullet point on the removal of all those setters from the Java api? If we don't want to list them all, shall we just explain the change which type of methods it affects and how to fix the compile error?
|
Thanks @javanna ! |
This commit enforces the requirement of Content-Type for the REST layer and removes the deprecated methods in transport requests and their usages.
While doing this, it turns out that there are many places where *Entity classes are used from the apache http client libraries and many of these usages did not specify the content type. The methods that do not specify a content type explicitly have been added to forbidden apis to prevent more of these from entering our code base.
Relates #19388