Compatible logic for include_type_param and RestCreateIndexAction by pgomulka · Pull Request #54197 · elastic/elasticsearch

pgomulka · 2020-03-25T15:40:37Z

Refactoring of the compatible infrastructure to allow registering multiple RestAction under the same path. It only extends the current mechanism which allowed registering RestAction under the same path but with different method. Now it also uses a version together with a method to find a matching RestAction

This PR also provides a V7 compatible RestCreateIndexAction that needs include_Type_param and a different logic for parsing mapping from its body.

fixed get/index tests
CompatRestIT. test {yaml=get/21_stored_fields_with_types/Stored fields}
CompatRestIT. test {yaml=get/71_source_filtering_with_types/Source
filtering}

CompatRestIT. test {yaml=index/70_mix_typeless_typeful/Index call that
introduces new field mappings}
CompatRestIT. test {yaml=index/70_mix_typeless_typeful/Index with
typeless API on an index that has types}

however the last one from get is still failing
CompatRestIT. test {yaml=get/100_mix_typeless_typeful/GET with typeless
API on an index that has

relates #54160

fixed get/index tests CompatRestIT. test {yaml=get/21_stored_fields_with_types/Stored fields} CompatRestIT. test {yaml=get/71_source_filtering_with_types/Source filtering} CompatRestIT. test {yaml=index/70_mix_typeless_typeful/Index call that introduces new field mappings} CompatRestIT. test {yaml=index/70_mix_typeless_typeful/Index with typeless API on an index that has types} however the last one from get is still failing CompatRestIT. test {yaml=get/100_mix_typeless_typeful/GET with typeless API on an index that has

cla-checker-service · 2020-03-27T14:16:20Z

💚 CLA has been signed

elasticmachine · 2020-03-27T14:26:48Z

Pinging @elastic/es-core-infra (:Core/Infra/Core)

pgomulka · 2020-03-30T07:06:07Z

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

                      return;
                  }
                } else {
-                    if(handler.compatibilityRequired() == false //regular (not removed) handlers are always dispatched


no longer need that logic. each handler is registered with a compatibleWith version.
The compatible API registers with Version.7
the current version register with Version.Current
this version is being used in handlers.getHandler to get the right handler

jakelandis

looking good, I like the approach here. some nitpicks, a request for a bit more testing, and a request to pass around a real Version object.

...patibility/src/main/java/org/elasticsearch/rest/compat/version7/RestCreateIndexActionV7.java

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

...patibility/src/main/java/org/elasticsearch/rest/compat/version7/RestCreateIndexActionV7.java

server/src/main/java/org/elasticsearch/action/ActionModule.java

jakelandis · 2020-04-02T01:02:26Z

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

        for (RestRequest.Method method : methods) {
-            methodHandlers.put(method, handler);
+            methodHandlers.putIfAbsent(method, new HashMap<>());
+            methodHandlers.get(method).put(version, handler);


nit: this can be simplified to methodHandlers.computeIfAbsent(method, k -> new HashMap<>()).put(version, handler); (mostly a cosmetic change though...)

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

…egistered

pgomulka · 2020-04-20T15:19:58Z

ok to test

pgomulka · 2020-04-22T14:18:20Z

@elasticmachine run elasticsearch-ci/bwc

jakelandis

LGTM, thanks for the revisions.

pgomulka · 2020-04-23T09:19:04Z

@elasticmachine run elasticsearch-ci/bwc

pgomulka · 2020-04-23T15:04:34Z

@elasticmachine run elasticsearch-ci/bwc

jaymode

Thank you for bearing with me on the slow turnaround for a review. I like this. I left some nits and suggestions. One change in MethodHandlers I think we do need to make though.

...patibility/src/main/java/org/elasticsearch/rest/compat/version7/RestCreateIndexActionV7.java

jaymode · 2020-04-25T01:58:29Z

...st-compatibility/src/main/java/org/elasticsearch/rest/compat/version7/RestIndexActionV7.java

        @Override
        public List<Route> routes() {
-            assert Version.CURRENT.major == 8 : "REST API compatilbity for version 7 is only supported on version 8";
+            assert Version.CURRENT.major == 8 : "REST API compatibility for version 7 is only supported on version 8";


This is a different way of doing things, but I'd much rather keep the assertion out of the individual rest handlers and add logic to the RestController for throwing an exception if a handler is registered with an incompatible version. So when the version is bumped to 9 that we would throw for any handler registered with a compatible version of 7. What do you think?

good idea, this will reduce duplication too

...ility/src/test/java/org/elasticsearch/rest/compat/version7/RestCreateIndexActionV7Tests.java

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

jaymode · 2020-04-25T02:47:07Z

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

+     *
+     * Handlers can be registered under the same path and method, but will require to have different versions (CURRENT or CURRENT-1)
+     *
+     * //todo What if a handler was added in V8 but was not present in V7?


IMO if you request V7 compatibility mode but the API doesn't exist in V7 then this should be a bad request (HTTP 400)

jaymode · 2020-04-25T02:51:32Z

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

+        if(versionToHandlers == null){
+            return null;
+        }
+        if (versionToHandlers.containsKey(version)) {


Is it ever valid for an entry in the map to have a null handler? I would think not so this could be simplified to:

final RestHandler handler = versionToHandlers.get(version); return handler != null || version.equals(Version.CURRENT) ? handler : versionToHandlers.get(Version.CURRENT);

agree - I don't expect null values in versionToHandlers map. Will apply the suggestion

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

jaymode · 2020-04-25T02:55:22Z

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

    private boolean contentConsumed = false;
-
    private final long requestId;
+    private final Version compatibleApiVersion;


This is a nit but I prefer to keep final variables grouped together and non-final member variables grouped together. This means that these two variables should be moved under httpChannel and followed by an empty line, which would separate them from httpRequest and contentConsumed

agree, I like that arrangement too

Co-Authored-By: Jay Modi <jaymode@users.noreply.github.com>

jaymode

LGTM!

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

Co-Authored-By: Jay Modi <jaymode@users.noreply.github.com>

…a/elasticsearch into compat/create_index_include_type

pgomulka added the WIP label Mar 25, 2020

pgomulka self-assigned this Mar 25, 2020

pgomulka added 3 commits March 27, 2020 14:57

allow registering multiple rest actions under the same path

2e922bf

revert out dirs

e7eda97

Merge branch 'compat_rest_api' into compat/create_index_include_type

5dac4b9

pgomulka added the :Core/Infra/Core Core issues without another label label Mar 27, 2020

pgomulka added :Core/Infra/REST API REST infrastructure and utilities and removed :Core/Infra/Core Core issues without another label WIP labels Mar 27, 2020

pgomulka changed the title ~~WIP compatible logic for include_type_param~~ Compatible logic for include_type_param and RestCreateIndexAction Mar 27, 2020

pgomulka marked this pull request as ready for review March 27, 2020 14:28

pgomulka added 2 commits March 27, 2020 15:29

extend restcreateindexaction

a213e6c

code style and discovery nodes in indexaction

f1ad677

pgomulka commented Mar 30, 2020

View reviewed changes

pgomulka mentioned this pull request Mar 30, 2020

Types removal - compatible changes #54160

Closed

66 tasks

fix double registration

8ff3463

pgomulka requested review from jakelandis and jaymode March 30, 2020 08:33

pgomulka mentioned this pull request Apr 1, 2020

Compatible logic for Removes typed endpoint from search and related APIs #54572

Merged

jakelandis requested changes Apr 2, 2020

View reviewed changes

pgomulka added 7 commits April 2, 2020 14:07

additional testing and using version

1ee4487

unused method

abe1d28

method handlers - returning null when no handler under a method was r…

d8fc2e5

…egistered

remove unused constant

ec8df43

Merge branch 'compat_rest_api' into compat/create_index_include_type

8602f65

fix javadoc

f3d25e0

compile

7024913

spotless

f52dbc1

pgomulka added 3 commits April 21, 2020 13:38

Merge branch 'compat_rest_api' into compat/create_index_include_type

0b07539

v7 name

8135794

fix test

f819313

javadoc

6e28d24

jakelandis approved these changes Apr 23, 2020

View reviewed changes

Merge branch 'compat_rest_api' into compat/create_index_include_type

a77716e

jaymode requested changes Apr 25, 2020

View reviewed changes

pgomulka and others added 4 commits April 27, 2020 10:10

Apply suggestions from code review

094a462

Co-Authored-By: Jay Modi <jaymode@users.noreply.github.com>

merge

f704d0c

compile fix

18651ae

assertion on version and test

0e1c917

pgomulka requested a review from jaymode April 27, 2020 13:06

jaymode approved these changes Apr 27, 2020

View reviewed changes

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

pgomulka and others added 4 commits April 28, 2020 09:04

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

93bebbf

Co-Authored-By: Jay Modi <jaymode@users.noreply.github.com>

Merge branch 'compat_rest_api' into compat/create_index_include_type

281ec7f

fix method not found test

4ee93ae

Merge branch 'compat/create_index_include_type' of github.com:pgomulk…

594b02f

…a/elasticsearch into compat/create_index_include_type

pgomulka merged commit 3a8303e into elastic:compat_rest_api Apr 28, 2020

ChrisHegarty unassigned pgomulka Oct 29, 2024

Conversation

pgomulka commented Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cla-checker-service bot commented Mar 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Mar 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakelandis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pgomulka commented Apr 20, 2020

Uh oh!

pgomulka commented Apr 22, 2020

Uh oh!

jakelandis left a comment

Choose a reason for hiding this comment

Uh oh!

pgomulka commented Apr 23, 2020

Uh oh!

pgomulka commented Apr 23, 2020

Uh oh!

jaymode left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaymode left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pgomulka commented Mar 25, 2020 •

edited

Loading

cla-checker-service bot commented Mar 27, 2020 •

edited

Loading