Introduce Compatible Version plugin#64481
Conversation
Co-authored-by: Jay Modi <jaymode@users.noreply.github.com>
…m:pgomulka/elasticsearch into compat/introduce_per_endpoint_media_types
…register_compatible_handlers
…compatible_plugin
|
@elasticmachine run elasticsearch-ci/2 |
pgomulka
changed the title
pgomulka
added
:Core/Infra/REST API
|
Pinging @elastic/es-core-infra (:Core/Infra/REST API) |
jakelandis
left a comment
jakelandis
left a comment
There was a problem hiding this comment.
LGTM - some nit picks and one possible extraneous if check. (please take any of the nit picks as suggestions not a requirement to change )
| byte contentTypeVersion = cVersion == null ? Version.CURRENT.major : Integer.valueOf(cVersion).byteValue(); | ||
|
|
||
| // accept version must be current or prior | ||
| if (acceptVersion > Version.CURRENT.major || acceptVersion < Version.CURRENT.major - 1) { |
There was a problem hiding this comment.
nit: can we use Version.CURRENT.minimumRestCompatibilityVersion() instead of Version.CURRENT.major -1 ? (to help encapsulate the logic)
| // accept version must be current or prior | ||
| if (acceptVersion > Version.CURRENT.major || acceptVersion < Version.CURRENT.major - 1) { | ||
| throw new ElasticsearchStatusException( | ||
| "Compatible version must be equal or less then the current version. Accept={}} Content-Type={}}", |
There was a problem hiding this comment.
nit: rather then saying "must be equal or less then the current version" , can it say "must be either version {} or {}, but found {}. Accept={}" , Version.CURRENT.major, Version.CURRENT.minimumRestCompatibilityVersion() , acceptVersion, acceptHeader (no need to put content-type here)
| if (hasContent) { | ||
|
|
||
| // content-type version must be current or prior | ||
| if (contentTypeVersion > Version.CURRENT.major || contentTypeVersion < Version.CURRENT.major - 1) { |
There was a problem hiding this comment.
same nitpicks as above... use the minumRestCompatibltyVersion and tweak the error message to be more precise (and no need for the accept header here)
| // both headers should be versioned or none | ||
| if ((cVersion == null && aVersion != null) || (aVersion == null && cVersion != null)) { | ||
| throw new ElasticsearchStatusException( | ||
| "Versioning is required on both Content-Type and Accept headers. Accept={} Content-Type={}", |
There was a problem hiding this comment.
nit: A compatible version is required on both Content-Type and Accept headers if either one has requested a compatible version. Accept={}, Content-Type={}
| // if both accept and content-type are sent, the version must match | ||
| if (contentTypeVersion != acceptVersion) { | ||
| throw new ElasticsearchStatusException( | ||
| "Content-Type and Accept version requests have to match. Accept={} Content-Type={}", |
There was a problem hiding this comment.
nit: A compatible version is required on both Content-Type and Accept headers if either one has requested a compatible version and the compatible versions must match. Accept={}, Content-Type={}
| ); | ||
| } | ||
| // both headers should be versioned or none | ||
| if ((cVersion == null && aVersion != null) || (aVersion == null && cVersion != null)) { |
There was a problem hiding this comment.
Can this ever evaluate true ? wouldn't the above if catch any this case ?
There was a problem hiding this comment.
the above if if (contentTypeVersion != acceptVersion) { would not catch the scenario when one of the headers is set to compatible-with=8 (current in general) and the other is not set (defaults to current)
expectThrows(
ElasticsearchStatusException.class,
() -> requestWith(acceptHeader(CURRENT_VERSION), contentTypeHeader(null), bodyPresent())
);
expectThrows(
ElasticsearchStatusException.class,
() -> requestWith(acceptHeader(null), contentTypeHeader(CURRENT_VERSION), bodyPresent())
);
|
@elasticmachine run elasticsearch-ci/1 |
| plugins.add(TestRestCompatibility1.class); | ||
| plugins.add(TestRestCompatibility2.class); | ||
|
|
||
| expectThrows(IllegalStateException.class, () -> new MockNode(settings.build(), plugins)); |
There was a problem hiding this comment.
can you check the message? We've had tests like this before and didn't check the message and there was some other cause of the exception rather than the true cause.
|
@elasticmachine run elasticsearch-ci/2 - random test failing |
|
@elasticmachine update branch |
4 participants
A RestCompatibilityPlugin and its xpack implementation allow to calculate a version requested on REST request. It uses accept, content-type headers to return a version.
It also performs a validation of allowed combinations of versions and values provided on accept/content-type headers
relates #51816