Header parsing for Rest versioning #52370
Comments
|
Pinging @elastic/es-core-infra (:Core/Infra/REST API) |
pgomulka
changed the title
|
Do you refer to the Also, wondering about considerations about (or thoughts against) using query strings (or URL params) to signal the desired compatibility, besides using already registered media types in the |
|
@bpintea right Accept - fixed the first comment. Thank you I think the path param - or any path modification - will be problematic as some users will hardcode permalinks to the api within their code. |
Wouldn't the code that adds a custom media type in the header be in "proximity" to the code that handles the URL? I'm not in the clear why one would be significantly more difficult than the other, but I admit I don't have a great overview in this regard. However, not having to mangle the URL and have the header "separation" does feel cleaner. Furthermore I see that RFC6838 does mandate the proposed format so it should be standards-compliant to a certain degree even without the actual IANA registration. (And one can find further endorsement in literature on the topic.) Which are arguments in favour of the original proposal. |
Providing a REST infrastructure to handle compatible APIs. Rest APIs that were removed in 8 will be available only if an HTTP request had a compatible header. At the moment is is expecting an Accept header to contain application/vnd.elasticsearch+json;compatible-with=7. This might be changed though when #52370 is resolved. The REST changes contain enriching RestRequest with a parameter Compatible-With. The value is taken from Accept header. See RestRequest creation. Used when deciding if a request to a compatible handler can be dispatched. See RestController. //TODO this at the moment can be simplified to only use a value from a header. However in rest layer we often have only access to Params, so headers won't be accessible when implementing some compatible code enriching XContentBuilder with a compatible version value. See AbstractRestChannel. Used when shape of a response is changed. see DocWriteResponse and GetResult MediaType parsing change - see XContentType. because we expect a different media type now application/vnd.elasticsearch+json;compatible-with=7 (when previously application/json or similar) testing changes - compat tests will have a compatible header set. see AbstractCompatRestTest.java Compatible rest handlers - based on RestIndexAction and RestGetAction. See modules/rest-compatibility. These extend already existing handlers and register them in a RestCompatPlugin under a typed paths. They will only be accessible when a compatible header is present (use of compatibilityRequired method from RestHandler interface) Without this PR 283 tests from 7.x are failing. - rest api spec tests only with this PR 228 tests are passing - with almost all tests from index/* package passed (missing 2 tests that require change for include_type_param, to be added in a new PR) relates #51816
|
We discussed in a meeting with @Mpdreamz and I'm also in favor of having a separate option as My proposal is to use a different naming, for instance |
|
@pgomulka @jakelandis ping regarding this issue and the comment from @ezimuel. I know that most of the work recently has been using compatible with and IMO I think we should close this as code has landed to do this parsing. Do you see a reason to keep this issue open? |
jaymode
added
feedback_needed
and removed
needs:triage
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
when accessing a compatible API a user has to specify a version its client is compatible with
Some people prefer to use a custom header for that purpose but it is also a common practice on the internet to use Accept and/or Content-Type for that purpose.
The argument against custom header is that it might often be filtered by firewalls or proxies.
This however might also happen with non IANA registered custom values of Accept/Content-Type.
There are several 'patterns' that people follow for values of Accept/Content-Type. Most popular are in a form:
application/vnd.elasticsearch.v7+json- however if we ever decide to register our subtype then we would have to do it for every new version. Also does not indicate well enought that the API is only compatible.application/vnd.elasticsearch+json; compatibleWith= 7- this seems to be the most preferable to me, it indicates the intention that the API is not version 7 but only compatible. Also registration would be easier. The subtype will not change - vnd.elasticsearch and we will just define that compatibleWith expects a major version number.relates #51816
The text was updated successfully, but these errors were encountered: