[REST Compatible API] Typed endpoints for Index and Get APIs #69131

pgomulka · 2021-02-17T15:16:18Z

The types removal effort has removed the type from Index API in #47671 and from Get API in #46587

This commit allows to use 'typed' endpoints for the both Index and Get APIs

relates compatible types-removal meta issue #54160

romseygeek

LGTM - a couple of nits but otherwise good to go.

romseygeek · 2021-03-17T15:02:42Z

server/src/main/java/org/elasticsearch/rest/action/document/RestIndexAction.java

    }

    @Override
    public String getName() {
        return "document_index_action";
    }

-    public static final class CreateHandler extends RestIndexAction {
+    public static class CreateHandler extends RestIndexAction {


I think these can stay as final now?

yes, these should be final. will fix

romseygeek · 2021-03-17T15:02:50Z

server/src/main/java/org/elasticsearch/rest/action/document/RestIndexAction.java

@@ -70,7 +86,7 @@ void validateOpType(String opType) {
        }
    }

-    public static final class AutoIdHandler extends RestIndexAction {
+    public static class AutoIdHandler extends RestIndexAction {


romseygeek · 2021-03-17T15:04:13Z

test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/DoSection.java

@@ -388,6 +388,7 @@ void checkWarningHeaders(final List<String> warningHeaders, final Version master
            final boolean matches = matcher.matches();
            if (matches) {
                final String message = HeaderWarning.extractWarningValueFromWarningHeader(header, true);
+


nit: can remove this

romseygeek · 2021-03-17T15:06:30Z

server/src/test/java/org/elasticsearch/action/DocWriteResponseTests.java

+            // DocWriteResponse is abstract so we have to sneak a subclass in here to test it.
+        };
+        try (XContentBuilder builder = JsonXContent.contentBuilder()) {
+            builder.withCompatibleVersion(RestApiVersion.V_7);


I'm a bit uneasy that we can just change the compatible version on the XContentBuilder at any time, as that means that a consumer could change it accidentally; I think I'd prefer that it's a parameter passed to contentBuilder(). But that's not really part of this issue, so we can look at it separately.

I agree, these should be fixed, but also feel that this should be in a separate PR. The change might require touching many files and would be unrelated to the index/get api.
I will follow up in a separate PR

also take a look at https://github.com/elastic/elasticsearch/pull/64423/files#r518884033
we try to prevent accidental change of the version with the assert inside withCompatibleVersion

+1 to making it immutable or explicitly 'set once', that would clearly indicate the intent that it should never be updated.

I refactored the XContentBuilder to have this field final and set in a constructor #70878

pgomulka · 2021-03-18T12:12:37Z

@elasticmachine update branch

jakelandis · 2021-03-18T14:14:02Z

server/src/test/java/org/elasticsearch/rest/action/document/RestGetActionTests.java

+
+import static org.hamcrest.Matchers.instanceOf;
+
+public class RestGetActionTests extends RestActionTestCase {


hmm... this will fail when master is on version 9. Can we add an assumeTrue(RestApiVersion.current() == 8) ..or something like that, also with a note to delete in v9+

Will RestApiVersion.V_7 exist in master at all? Often dead code like this is removed on upgrade because the constants that it uses for guards are deleted, in which case this test class will fail to compile.

@romseygeek that is the plan. Once the RestApiVersion.V_7 is deleted, we will delete the tests using it.
However we don't want to create a blocker during the upgrade process and we expect for a brief period of time RestApiVersion will have V_7 (obsolete) V_8 compatible and V_9 current.

jakelandis · 2021-03-18T14:14:53Z

server/src/test/java/org/elasticsearch/rest/action/document/RestIndexActionTests.java

@@ -84,4 +91,34 @@ private void checkAutoIdOpType(Version minClusterVersion, DocWriteRequest.OpType
        dispatchRequest(autoIdRequest);
        assertThat(executeCalled.get(), equalTo(true));
    }
+
+    public void testTypeInPath() {


same comment here about failing on version 9

this one is actually tricky.
This test is testing both compatible and current version behaviour. The RestIndexAction will be registered using its all routes, including the compatible routes. After the version bump, when the V_7 is obsolete the test will fail in setUpAction because of the assert in the RestController:L179

assert RestApiVersion.minimumSupported() == version || RestApiVersion.current() == version : "REST API compatibility is only supported for version " + RestApiVersion.minimumSupported().major;

I guess I will move the compat tests into separate file and use the assert there?
although hmm this won't help..

the only thing that comes to mind is to have a strategy that we only bump the minimumSupported/current versions when all of the V_7 code is removed..
But that is also against whet we already assumed and had assertions in place
Version:L124

assert RestApiVersion.current().major == CURRENT.major : "RestApiVersion must be upgraded " + "to reflect major from Version.CURRENT [" + CURRENT.major + "]" + " but is still set to [" + RestApiVersion.current().major + "]";

all in all this feels bigger then just a small fix and deserves a separate follow up PR - there is plenty of time before 8 actually being released

I'm sure I completely follow, but i am fine with follow ups later on to help enusre the bump to v9 is easier.

However, I think adding in an assumeTrue per Test that shouldn't run in v9 is pretty easy to implement now..but if I am wrong +1 for followup.

as per discussion on the sync, I gathered the conclusions on the issue #70617

jakelandis

LGTM , however we should protect the tests from running (and failing) on v9. Either in this PR or a followup.

jtibshirani · 2021-03-18T18:13:56Z

server/src/main/java/org/elasticsearch/index/get/GetResult.java

@@ -40,6 +42,9 @@

 public class GetResult implements Writeable, Iterable<DocumentField>, ToXContentObject {

+    private static final String TYPE_FIELD_NAME = "_type";
+    private static final Text SINGLE_MAPPING_TYPE = new Text(MapperService.SINGLE_MAPPING_NAME);


Small comment, I think we can use MapperService.SINGLE_MAPPING_NAME directly and avoid this extra constant.

jtibshirani · 2021-03-18T18:25:42Z

server/src/test/java/org/elasticsearch/rest/action/document/RestGetActionTests.java

+
+public class RestGetActionTests extends RestActionTestCase {
+    final List<String> contentTypeHeader =
+        Collections.singletonList("application/vnd.elasticsearch+json;compatible-with="+ RestApiVersion.V_7.major);


Small comment, is there a way to avoid hardcoding this header in every test?

good point, there is randomCompatibleMediaType in ESTestCase`

jtibshirani · 2021-03-18T18:31:09Z

server/src/test/java/org/elasticsearch/action/DocWriteResponseTests.java

+            // DocWriteResponse is abstract so we have to sneak a subclass in here to test it.
+        };
+        try (XContentBuilder builder = JsonXContent.contentBuilder()) {
+            builder.withCompatibleVersion(RestApiVersion.V_7);


+1 to making it immutable or explicitly 'set once', that would clearly indicate the intent that it should never be updated.

jtibshirani

The approach looks good to me too! I like the targeted checks, they keep the structure simple and don't hurt readability too much. Also the test coverage looks good.

…into compat/index_get

pgomulka · 2021-03-22T08:58:29Z

@elasticmachine update branch

joegallo

🚢

jakelandis and others added 30 commits February 11, 2021 16:39

refactor

e13468b

Merge branch 'master' into refactor_inject_headers

0fbd2f9

start of support for allowed_warnings

3cb7200

start of injectWarning

526cdd3

start remove warning

19141dd

review changes

bc7b2aa

Merge branch 'refactor_inject_headers' into warnings_transform

a6c2433

clean up match tests

4cea425

more refactoring and tests passing

757200e

Merge remote-tracking branch 'upstream/master' into warnings_transform

2178081

more refactor of tests

4b0629e

add missing file

3884c3d

spotless

106f226

Merge branch 'another_refactor' into warnings_transform

511a3db

checkstyle

d9ba6c0

Merge branch 'another_refactor' into warnings_transform

b47d98b

review changes

857b2c6

Merge branch 'another_refactor' into warnings_transform

775565e

more tests

98ee1f7

finish up testing

cdd8ad5

Merge remote-tracking branch 'upstream/master' into warnings_transform

cd2d042

spotless

b51b72f

Merge remote-tracking branch 'upstream/master' into warnings_transform

230d0ae

fix merge

5de4a30

add @internal for gradle tracked objects that look like getters

b60bf0c

index and get and testing

7390a91

import

c83fdc1

move blacklist

ec3cd22

Merge remote-tracking branch 'upstream/master' into compat/index_get

90ab29a

Merge remote-tracking branch 'upstream/master' into compat/index_get

67c499a

pgomulka added 3 commits March 16, 2021 11:59

surgical appproach

af12ce3

Merge remote-tracking branch 'upstream/master' into compat/index_get

595f59c

remove empty space

a699329

pgomulka requested review from romseygeek and jtibshirani March 16, 2021 12:45

pgomulka added 2 commits March 17, 2021 07:17

Merge remote-tracking branch 'upstream/master' into compat/index_get

e0ba336

enable get tests and use allowed warnings

8dcea4c

pgomulka mentioned this pull request Mar 17, 2021

Types removal - compatible changes #54160

Closed

66 tasks

romseygeek approved these changes Mar 17, 2021

View reviewed changes

make classes final and remove empty line

ee88fab

Merge branch 'master' into compat/index_get

2cb33c2

jakelandis reviewed Mar 18, 2021

View reviewed changes

jakelandis approved these changes Mar 18, 2021

View reviewed changes

jtibshirani reviewed Mar 18, 2021

View reviewed changes

jtibshirani approved these changes Mar 18, 2021

View reviewed changes

pgomulka added 3 commits March 22, 2021 09:29

use Mapping service type fields

5899451

Merge branch 'compat/index_get' of github.com:pgomulka/elasticsearch …

3680b39

…into compat/index_get

empty line

7993dd1

pgomulka mentioned this pull request Mar 22, 2021

Ensure old rest api compatible code is not used after version bump #70617

Open

Merge branch 'master' into compat/index_get

8e1aaf9

joegallo approved these changes Mar 22, 2021

View reviewed changes

pgomulka merged commit e942873 into elastic:master Mar 23, 2021

jakelandis added v8.0.0-alpha1 >enhancement 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

[REST Compatible API] Typed endpoints for Index and Get APIs #69131

[REST Compatible API] Typed endpoints for Index and Get APIs #69131

pgomulka commented Feb 17, 2021 •

edited

Loading

romseygeek left a comment

romseygeek Mar 17, 2021

pgomulka Mar 18, 2021

romseygeek Mar 17, 2021

romseygeek Mar 17, 2021

romseygeek Mar 17, 2021

pgomulka Mar 18, 2021

pgomulka Mar 18, 2021

jtibshirani Mar 18, 2021

pgomulka Mar 29, 2021

pgomulka commented Mar 18, 2021

jakelandis Mar 18, 2021 •

edited

Loading

romseygeek Mar 18, 2021

pgomulka Mar 18, 2021

jakelandis Mar 18, 2021

pgomulka Mar 18, 2021 •

edited

Loading

pgomulka Mar 18, 2021

pgomulka Mar 18, 2021

jakelandis Mar 18, 2021

pgomulka Mar 23, 2021

jakelandis left a comment

jtibshirani Mar 18, 2021

jtibshirani Mar 18, 2021

pgomulka Mar 22, 2021

jtibshirani Mar 18, 2021

jtibshirani left a comment

pgomulka commented Mar 22, 2021

joegallo left a comment


		import static org.hamcrest.Matchers.instanceOf;

		public class RestGetActionTests extends RestActionTestCase {

[REST Compatible API] Typed endpoints for Index and Get APIs #69131

[REST Compatible API] Typed endpoints for Index and Get APIs #69131

Conversation

pgomulka commented Feb 17, 2021 • edited Loading

romseygeek 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

pgomulka commented Mar 18, 2021

jakelandis Mar 18, 2021 • 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 Mar 18, 2021 • 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

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

jtibshirani left a comment

Choose a reason for hiding this comment

pgomulka commented Mar 22, 2021

joegallo left a comment

Choose a reason for hiding this comment

pgomulka commented Feb 17, 2021 •

edited

Loading

jakelandis Mar 18, 2021 •

edited

Loading

pgomulka Mar 18, 2021 •

edited

Loading