Allow registering compatible handlers #64423

pgomulka · 2020-10-30T16:01:13Z

Adding an infrastructure to allow for registration of Compatible Handlers.
Compatible handlers are RestHandlers used for handling rest request from old version clients ( CURRENT-1 version). They might be registered under an endpoint that was removed or changed in CURRENT version (different path, method or an endpoint completely removed).
But they also can be registered under the same endpoint (same path, method as the RestHandler in CURRENT)
RestHandler's endpoint is at the moment 2dimensional - a method and a path.

This PR adds a 3rd dimension - a version.

Registration:
RestHandler declares a new compatibleWithVersion method, which will be overridden by Compatible Handlers and returning a Version.CURRENT -1. By default the method returns Version.CURRENT
compatibleWithVersion is used when iterating over handlers within RestController#registerHandler. The returned value is used to set a version on MethodHandlers

Lookup:
An interface CompatibleVersion is introduced in order to abstract a logic to calculate a compatible version requested by a user.
It is not implemented in this PR. A simplified, always returning Version.CURRENT implementation is used.
Within RestController, a version is calculated with the use of CompatibleVersion, then the lookup for MethodHandlers is performed (the logic is the same)
Once it is find, an additional lookup for a RestHandler for requested version is made.

The requested version has to be also passed down to XContentBuilder in order to allow for per version serialisation logic

relates #51816

…ionContext:L122

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

…m:pgomulka/elasticsearch into compat/introduce_per_endpoint_media_types

…register_compatible_handlers

pgomulka · 2020-11-05T09:47:19Z

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

+            return null; //method not found
+        }
+        final RestHandler handler = versionToHandlers.get(version);
+        return handler != null || version.equals(Version.CURRENT) ? handler : versionToHandlers.get(Version.CURRENT);


I really can't remember the use cases where we needed this. (returning a v8 handler when a handler was not present in v7)
@jaymode made a comment about this when we were still working on a feature branch https://github.com/elastic/elasticsearch/pull/54197/files#r414959451

I think the reason for this is @Mpdreamz's comment #60516 (comment) where clients plan to send compatible with to all APIs. If this is the case, can we please document this in code/method so we do not lose the information

I had to think about this waaay to long :) and wrote a unit test to help me understand

/** * We want clients to be able to request compatibility with a prior version for all APIs. If a there does not exist a handler registered * for the prior version, we want to return the current version handler. This effectively means that the current endpoint is compatible * with the prior version, which should be true since if it were not compatible there would be a registered compatible endpoint. */ public void testMissingPriorHandlerReturnsCurrentHandler(){ RestHandler currentVersionHandler = new CurrentVersionHandler(); MethodHandlers methodHandlers = new MethodHandlers("path", currentVersionHandler, RestRequest.Method.PUT, RestRequest.Method.POST); RestHandler handler = methodHandlers.getHandler(RestRequest.Method.PUT, Version.CURRENT.previousMajor()); assertThat(handler, sameInstance(currentVersionHandler)); }

Can you add a similar comment inline here, and add that test?

The return can be simplified to:

return handler == null ? versionToHandlers.get(Version.CURRENT) : handler;

with a test:

public void testMissingCurrentHandler(){ RestHandler previousVersionHandler = new PreviousVersionHandler(); MethodHandlers methodHandlers = new MethodHandlers("path", previousVersionHandler, RestRequest.Method.PUT, RestRequest.Method.POST); RestHandler handler = methodHandlers.getHandler(RestRequest.Method.PUT, Version.CURRENT); assertNull(handler); }

to be fair, I am not super convinced about returning the CURRENT.
if an endpoint did not exist in v7 server version, client would be getting 404 when trying to access it with v7 client.
When he upgrades server to next version and he still uses v7 client, I guess he should be still getting 404?

@jakelandis I am not sure I understand this:

This effectively means that the current endpoint is compatible
* with the prior version, which should be true since if it were not compatible there would be a registered compatible endpoint.

from high level point of view, there is no knowledge if there is a compatible endpoint or not. If the handler's code did not change, user has no idea that a response came from v7 code, or if the response is compatible shape from v8 (in ideal 100% compatible handler).

THe same for when an endpoint do not exist. Returning 404 from v8 when for not existing in v7 endpoint is 100% compatible in shape and behaviour

@Mpdreamz any views on this?

I think we should return new APIs with compatible requested. Below is a table with my thought for how behaviour should look.

The rationale is that in a minor you can add a new API passively, therefor new APIs are compatible (as opposed to non-compatible/breaking). Further, one motivation to add a new API is to replace an old one. With a client sending compatibility all the time, they should be able to fix the compatibility warnings and migrate to the new API. This allows them to do just that.

scenario v7 v8 v7 compat / v8 server result

new in v8 404 200 200 (v8)

no change/compat change in v8 (no compat handler) 200 200 200 (v8)

removed in v8 (with compat handler) 200 404 200 (v7)

non passive change (with compat handler) 200 200 200 (v7)

The rationale is that in a minor you can add a new API passively, therefor new APIs are compatible (as opposed to non-compatible/breaking)

great explanation. thank you. I think this was the initial reason we went for that behaviour on a feature branch. I will add a comment about this at least in a test

Since clients generate methods for known API's the only way to talk to an API that exists in 8.0 but not (yet) in 7.x is to call transport.Request(url)

I think the new in v8 case needs some zooming in on:

new in v8

accept/content-type v7 compat / v8 server result

application/json application/json

application/vnd.elasticsearch+json;compatible-with=8 application/vnd.elasticsearch+json;compatible-with=8

application/vnd.elasticsearch+json;compatible-with=7 ?

The first two seem straight forward the last one not so much It could return a

404, since thats what it did in 7

200 with application/vnd.elasticsearch+json;compatible-with=7.

200 with application/vnd.elasticsearch+json;compatible-with=8.

Returning with compatible-with=8 is not what the user requested so seems like an unexpected return.

Returning with compatible-with=7 since its a new API it can be argued anything is compatible with 7. But this will become a pain to track when e.g a new API gets introduced in 8.0 as experimental and GA's in 8.4. You potentially get back two different 7 responses for 8.0 and 8.4.

That leaves returning a 404.

cc @elastic/es-clients we have not discussed if the the transport used in the client should default to application/vnd.elasticsearch+json;compatible-with=7 or application/json. I feel that if the transport defaults to application/json but the API methods calling the transport default to the vendor mime type the client user has the least suprises.

I think with the current approach where we version compatible API per major we won't be able to give a 100% experience of previous version server. And that wasn't a goal - after all we want to achieve the compatible API, not the versioned one.
I feel all the comments from this "thread" are valid and I will summarise them on a separate issue.
The current implementation is not returning the right response header yet, so the discussion on new in v8 could continue on the new issue.
Lets use this issue to facilitate the discussion #64852

jakelandis

took a quick a pass and things are looking good. I will take a deeper look tomorrow.

jakelandis · 2020-11-05T22:30:21Z

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

+
+/**
+ * An interface used to specify a function that returns a compatible API version
+ * Intended to be used in a code base instead of a plugin.


not sure i follow the second sentence (Intended to be ...)

right.. I rephrased this, but maybe it is still to vague. let me know
My intention was to not describe the actual plugin implementation, but just to mention that this abstract the way we actually provide the logic.
It happens that we use a plugin for this, but could if we want to use any class within a server to do this as well (we did in the previous attempts)

jakelandis · 2020-11-05T22:44:42Z

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


-    MethodHandlers(String path, RestHandler handler, RestRequest.Method... methods) {
+    MethodHandlers(String path, RestHandler handler, Version version,  RestRequest.Method... methods) {


Can you a few unit tests for this class ?

I also realised that changing the method signature was not necessary.

jaymode · 2020-11-06T17:00:00Z

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

+            return null; //method not found
+        }
+        final RestHandler handler = versionToHandlers.get(version);
+        return handler != null || version.equals(Version.CURRENT) ? handler : versionToHandlers.get(Version.CURRENT);


I think the reason for this is @Mpdreamz's comment #60516 (comment) where clients plan to send compatible with to all APIs. If this is the case, can we please document this in code/method so we do not lose the information

jaymode · 2020-11-06T17:01:30Z

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

@@ -1004,6 +1006,15 @@ public XContentBuilder copyCurrentStructure(XContentParser parser) throws IOExce
        return this;
    }

+    public XContentBuilder withCompatibleMajorVersion(byte compatibleMajorVersion){


Suggested change

public XContentBuilder withCompatibleMajorVersion(byte compatibleMajorVersion){

public XContentBuilder withCompatibleMajorVersion(byte compatibleMajorVersion) {

jaymode · 2020-11-06T17:02:40Z

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

+        return this;
+    }
+
+    public byte getCompatibleMajorVersion() {


can you add javadocs to this method and the one above?

jaymode · 2020-11-06T17:03:37Z

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

@@ -48,6 +48,8 @@
 */
 public final class XContentBuilder implements Closeable, Flushable {

+    private byte compatibleMajorVersion;


hmm, I wonder if we should make this a SetOnce or add validation that if it is not some sentinel value that it cannot be changed again? I'm not sure that I like it being mutable with a builder pattern and allowing it to be set multiple times

with a regular builder pattern I would be ok with the field being mutable, as it is normally used within some narrow code scope.
With XContentBuilder we often pass it around so makes sense to protect against some accidental changes.
I feel like we should assert about this in testing only though.
I added assert

jaymode · 2020-11-06T17:06:42Z

server/src/main/java/org/elasticsearch/Version.java

+        return Version.fromId(previousMajorId);
+    }
+
+    public static Version minimumRestCompatibilityVersion() {


hmm, should we make this non-static like minimumCompatibilityVersion and minimumIndexCompatibilityVersion? Also, please add javadocs :)

jaymode · 2020-11-06T17:06:52Z

server/src/main/java/org/elasticsearch/Version.java

+        this.previousMajorId = major > 0 ? (major - 1) * 1000000 + 99 : major;
+    }
+
+    public Version previousMajor() {


javadocs please

jaymode · 2020-11-06T17:07:08Z

server/src/main/java/org/elasticsearch/node/Node.java

@@ -539,9 +540,11 @@ protected Node(final Environment initialEnvironment,
                                                 repositoriesServiceReference::get).stream())
                .collect(Collectors.toList());

+


Suggested change

jakelandis · 2020-11-09T14:17:02Z

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

@@ -242,7 +250,11 @@ private void dispatchRequest(RestRequest request, RestChannel channel, RestHandl
                inFlightRequestsBreaker(circuitBreakerService).addWithoutBreaking(contentLength);
            }
            // iff we could reserve bytes for the request we need to send the response also over this channel
-            responseChannel = new ResourceHandlingHttpChannel(channel, circuitBreakerService, contentLength);
+            // Using a version from a handler because if no handler was found for requested version,


I am not sure i understand the purpose of this comment here.

my intention was to indicate that even if you requested an applicaiton/vnd.elasticsearch+json;compatible-with=7 but the handler did not exist in 7, the actual resthandler will be used from v8.
but that indeed makes no difference, as there would be no compatible serialisation code.
Removed the comment.

jakelandis

LGTM

…ompatible_handlers

pgomulka · 2020-11-10T09:35:07Z

@elasticmachine run elasticsearch-ci/2

…ompatible_handlers

jaymode

I left some minor comments to address but otherwise LGTM

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

server/src/main/java/org/elasticsearch/Version.java

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

…ulka/elasticsearch into compat/register_compatible_handlers

pgomulka added 6 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

javadocs

29b27e8

fix javadoc

4690362

Allow registration of compatible version-1 handlers

e7866a9

pgomulka added the WIP label Oct 30, 2020

pgomulka self-assigned this Oct 30, 2020

pgomulka and others added 16 commits November 2, 2020 13:27

add tests

0330d97

Merge branch 'master' into compat/register_compatible_handlers

78d4660

pass version to xcontentbuilder

bc5de8b

Merge branch 'master' into compat/register_compatible_handlers

f958cdd

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

remove charset validation

dc61731

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

Merge branch 'compat/introduce_per_endpoint_media_types' into compat/…

950ba7b

…register_compatible_handlers

Merge branch 'master' into compat/register_compatible_handlers

310b735

pgomulka commented Nov 5, 2020

View reviewed changes

javadoc and cleanup

afba70d

pgomulka changed the title ~~WIP Allow registering compatible handlers~~ Allow registering compatible handlers Nov 5, 2020

pgomulka marked this pull request as ready for review November 5, 2020 09:58

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

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

pgomulka removed Team:Core/Infra Meta label for core/infra team WIP labels Nov 5, 2020

jakelandis requested review from jakelandis and jaymode November 5, 2020 22:26

jakelandis reviewed Nov 5, 2020

View reviewed changes

pgomulka added 2 commits November 6, 2020 07:59

Merge branch 'master' into compat/register_compatible_handlers

385f5a9

tests and javadoc

836fe0f

jaymode reviewed Nov 6, 2020

View reviewed changes

pgomulka added 3 commits November 9, 2020 13:05

javadoc

8f2ea92

do not set compatible version twice

d70a071

Merge branch 'master' into compat/register_compatible_handlers

87d762f

jakelandis reviewed Nov 9, 2020

View reviewed changes

jakelandis approved these changes Nov 9, 2020

View reviewed changes

pgomulka added 2 commits November 9, 2020 16:06

javadocs

deff739

Merge remote-tracking branch 'upstream/master' into compat/register_c…

4ecbf22

…ompatible_handlers

pgomulka mentioned this pull request Nov 10, 2020

Support endpoints added in new version with compatible api #64852

Open

pgomulka requested a review from jaymode November 10, 2020 13:51

Merge remote-tracking branch 'upstream/master' into compat/register_c…

a38ed04

…ompatible_handlers

jaymode approved these changes Nov 10, 2020

View reviewed changes

libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java Outdated Show resolved Hide resolved

server/src/main/java/org/elasticsearch/Version.java Outdated Show resolved Hide resolved

pgomulka and others added 3 commits November 10, 2020 17:44

Apply suggestions from code review

c39d0bb

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

Merge branch 'compat/register_compatible_handlers' of github.com:pgom…

09704f3

…ulka/elasticsearch into compat/register_compatible_handlers

xcontentbuilder has the version requested by a user

023c8d9

pgomulka merged commit 618d8bc into elastic:master Nov 16, 2020

pgomulka mentioned this pull request Nov 16, 2020

Compatible API header parsing plugin #60516

Closed

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

ChrisHegarty unassigned pgomulka Oct 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow registering compatible handlers #64423

Allow registering compatible handlers #64423

pgomulka commented Oct 30, 2020 •

edited

Loading

pgomulka Nov 5, 2020

jaymode Nov 6, 2020

jakelandis Nov 6, 2020

jakelandis Nov 6, 2020

pgomulka Nov 9, 2020 •

edited

Loading

jakelandis Nov 9, 2020 •

edited

Loading

pgomulka Nov 9, 2020

Mpdreamz Nov 9, 2020 •

edited

Loading

pgomulka Nov 10, 2020 •

edited

Loading

jakelandis left a comment

jakelandis Nov 5, 2020

pgomulka Nov 6, 2020

jakelandis Nov 5, 2020

pgomulka Nov 6, 2020

jaymode Nov 6, 2020

jaymode Nov 6, 2020

jaymode Nov 6, 2020

jaymode Nov 6, 2020

pgomulka Nov 9, 2020

jaymode Nov 6, 2020

jaymode Nov 6, 2020

jaymode Nov 6, 2020

jakelandis Nov 9, 2020

pgomulka Nov 9, 2020

jakelandis left a comment

pgomulka commented Nov 10, 2020

jaymode left a comment

scenario	v7	v8	v7 compat / v8 server result
new in v8	404	200	200 (v8)
no change/compat change in v8 (no compat handler)	200	200	200 (v8)
removed in v8 (with compat handler)	200	404	200 (v7)
non passive change (with compat handler)	200	200	200 (v7)

accept/content-type	v7 compat / v8 server result
application/json	application/json
application/vnd.elasticsearch+json;compatible-with=8	application/vnd.elasticsearch+json;compatible-with=8
application/vnd.elasticsearch+json;compatible-with=7	?


		MethodHandlers(String path, RestHandler handler, RestRequest.Method... methods) {
		MethodHandlers(String path, RestHandler handler, Version version, RestRequest.Method... methods) {

	public XContentBuilder withCompatibleMajorVersion(byte compatibleMajorVersion){
	public XContentBuilder withCompatibleMajorVersion(byte compatibleMajorVersion) {

		@@ -539,9 +540,11 @@ protected Node(final Environment initialEnvironment,
		repositoriesServiceReference::get).stream())
		.collect(Collectors.toList());

Allow registering compatible handlers #64423

Allow registering compatible handlers #64423

Conversation

pgomulka commented Oct 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgomulka Nov 9, 2020 • edited Loading

Choose a reason for hiding this comment

jakelandis Nov 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mpdreamz Nov 9, 2020 • edited Loading

Choose a reason for hiding this comment

new in v8

pgomulka Nov 10, 2020 • edited Loading

Choose a reason for hiding this comment

jakelandis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakelandis left a comment

Choose a reason for hiding this comment

pgomulka commented Nov 10, 2020

jaymode left a comment

Choose a reason for hiding this comment

pgomulka commented Oct 30, 2020 •

edited

Loading

pgomulka Nov 9, 2020 •

edited

Loading

jakelandis Nov 9, 2020 •

edited

Loading

Mpdreamz Nov 9, 2020 •

edited

Loading

pgomulka Nov 10, 2020 •

edited

Loading