Compatible version of typed Index And Get API #53228

pgomulka · 2020-03-06T15:21:31Z

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

1226 tests, 227 failures, 16ignored 23 / 2 failled in index/* failing 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}

elasticmachine · 2020-03-06T15:21:50Z

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

jakelandis · 2020-03-09T22:08:47Z

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

@@ -86,4 +100,30 @@ protected RestStatus getStatus(final GetResponse response) {
        });
    }

+    public static class CompatibleRestGetAction extends RestGetAction {


Until we defined a better approach, can you start adding in something like this

assert Version.CURRENT.major == 8 : "REST API compatilbity for version 7 is only supported on version 8";```

jakelandis · 2020-03-09T22:14:38Z

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

+import java.util.function.Consumer;
+import java.util.function.UnaryOperator;
+
+public class CompatibleHandlers {


Can you add a TODO in here to ensure we get some buy in on #52370 before we merge the header format.

pgomulka · 2020-03-10T15:08:48Z

ok to test

…ype-index-get

Introduce module for compat code

…failing

jakelandis · 2020-03-11T14:26:08Z

server/src/test/java/org/elasticsearch/rest/RestControllerTests.java

+        FakeRestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY)
+            .withContent(new BytesArray(content), RestRequest.parseContentType(contentTypeHeader)).withPath("/foo")
+            .withHeaders(Map.of("Content-Type", contentTypeHeader, "Accept", contentTypeHeader))
+            .build();


Also, what happens if you set the Content-Type to application/json, but Accept to application/vnd.elasticsearch+json;compatible-with=7 ?

I wonder if we need more testing for these edge cases ?(either unit or integration)

good question. we can be more specific for response generation with Accept and request parsing with Content-Type.
But on the other hand I think it will just complicate matters too much.
I would prefer to assume (and assert in code) that these are the same

Same as above (I added a "parking lot" section on #51816 so we don't forget.)

code review follow up

jtibshirani

I added a couple comments from the types deprecation side. One high-level comment: we should ensure there's test coverage for all the deprecation messages we're adding back. In 7.x we had dedicated unit tests in the various actions -- here's an example for RestIndexAction.

jtibshirani · 2020-03-11T19:38:55Z

...t-compatibility/src/main/java/org/elasticsearch/rest/compat/version7/CompatibleHandlers.java

+
+public class CompatibleHandlers {
+
+    public static Consumer<RestRequest> consumeParameterType(DeprecationLogger deprecationLogger) {


I think renaming this to consumeTypeParameter would make its purpose more clear. Also, could we just have a method void consumeParameterType(DeprecationLogger deprecationLogger, RestRequest request) instead of using a Consumer? Let me know if I'm missing something around why consumers are necessary, to me they make the logic less straightforward.

agree with both - method rename and making this a method.
Originally the reason for this being a consumer was because I was trying to fit in into the previous style of registering handlers. Extending the RestController.registerHandler with additional handlerWrappers. This is no longer needed after the recent refactoring made by Jay

jtibshirani · 2020-03-11T21:38:51Z

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

+    private static final DeprecationLogger deprecationLogger = new DeprecationLogger(LogManager.getLogger(RestGetAction.class));
+    private static final String TYPES_DEPRECATION_MESSAGE = "[types removal] Specifying types in "
+        + "document get requests is deprecated, use the /{index}/_doc/{id} endpoint instead.";
+    private static final Consumer<RestRequest> DEPRECATION_WARNING = r -> deprecationLogger.deprecatedAndMaybeLog(


Could we avoid introducing a Consumer here and just call deprecationLogger.deprecatedAndMaybeLog(...) directly inside prepareRequest?

sure, that similarly to previous use of consumer was influenced by previous style of registering handlers.
a direct call is better here.

jtibshirani · 2020-03-11T21:46:53Z

...t-compatibility/src/main/java/org/elasticsearch/rest/compat/version7/CompatibleHandlers.java

+        String TYPES_DEPRECATION_MESSAGE = "[types removal] Using type as a path parameter is deprecated.";
+
+        return r -> {
+            deprecationLogger.deprecatedAndMaybeLog("create_index_with_types", TYPES_DEPRECATION_MESSAGE);


If I'm understanding correctly, it seems like we'll log two deprecation messages per request, both the endpoint-specific one like "Specifying types in document get requests is deprecated..." and this more general one "Using type as a path parameter is deprecated". I think we could just stick with the endpoint-specific one to avoid warning twice about the same issue.

As a side note, this general deprecation message is also logged under the key "create_index_with_types", I think we'd want a more precise name.

agree - let's stick with just per endpoint warning.
WIth that I feel like we can even get rid of CompatibleHandlers.consumeParameterType - it would only consume a type param - and the whole utility class.
I was worried that it would end up a bag of all the helpers for compatibility. It might generate a little bit of duplication, but there is no need for it now.

Do you mean more precise name for a deprecation key? Like to be the same for Get and Index and other handlers?

jtibshirani · 2020-03-11T21:51:29Z

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

@@ -276,6 +280,9 @@ public void writeTo(StreamOutput out) throws IOException {
    @Override
    public final XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
        builder.startObject();
+        if (builder.getCompatibleMajorVersion() == Version.V_7_0_0.major) {
+            builder.field(TYPE_FIELD_NAME, SINGLE_MAPPING_TYPE);


I think you can just do the following, I've never had to wrap a string inside a Text object:

builder.field(TYPE_FIELD_NAME, MapperService.SINGLE_MAPPING_NAME);

jtibshirani · 2020-03-11T21:52:55Z

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

@@ -107,4 +107,6 @@ public void testToXContentDoesntIncludeForcedRefreshUnlessForced() throws IOExce
            }
        }
    }
+


Small comment, would be good to revert this.

rjernst

This is not an exhaustive review, but I have a few minor comments.

rjernst · 2020-03-13T18:12:15Z

modules/rest-compatibility/src/main/java/org/elasticsearch/rest/compat/RestCompatPlugin.java

+    ) {
+        if (Version.CURRENT.major == 8) {
+            return List.of(
+                new RestGetActionV7(),


Wouldn't we never have more than one major versions worth of compat in a single version? If that is the case, why do we need to version the class names?

There is a narrow window where 2 versions can be present.
Let's say we released version 8 (or a feature freeze) and we are now already developing ES9 and there will be features compatible with v8.
We won't be able to remove the v7 compat features instantly so for some time there will be both.

I am +1 on using the version in the class for this reason, and for the obviousness they provide. While (arguably) ugly, these classes are special in that they are not intended to be maintained, rather they are intended to be deleted after a some point in time. Having the version in the name makes is more obvious and helps to prevent accidental modifications (like from a simple class search) and makes it really obvious if these things exist for too long.

rjernst · 2020-03-13T18:12:58Z

modules/rest-compatibility/build.gradle

+  classname 'org.elasticsearch.rest.compat.RestCompatPlugin'
+}
+
+integTest.enabled = false


I see tests in this PR; why are they disabled?

they should be enabled. fixed

actually there are no integTests, but there are unit tests so integTests.enabled = false tests.enabled=true

rjernst · 2020-03-13T18:14:54Z

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

@@ -276,6 +278,9 @@ public void writeTo(StreamOutput out) throws IOException {
    @Override
    public final XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
        builder.startObject();
+        if (builder.getCompatibleMajorVersion() == Version.V_7_0_0.major) {


If we are just looking at major version number, using the Version constant just to extract "7" seems overkill and less readable than just using the literal 7.

I think this will help the removal of compatible code. We have not decided on what mechanism to use in order to help finding this code.
The plan at the moment is to put as much code v7-compatible into module/rest-compatibility
Using Version.V_7_0_0.major would make sure that if the version is removed after major upgrade it will also highlight the places where v7-compatible code has to be removed

Also, the removal of the of the Version.V_7_0_0 constant in the code (for v9) will cause this to fail compilation. Which is likely the correct the behavior. It does indeed make major version bumps harder (one more thing that breaks), but for now I think this is the best we can do. It feels like there is something that can be done to ease that upgrade pain, but that is outside the scope of this PR.

rjernst · 2020-03-13T18:18:14Z

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

@@ -340,6 +349,11 @@ private void tryAllHandlers(final RestRequest request, final RestChannel channel
        handleBadRequest(uri, requestMethod, channel);
    }

+    public static boolean isV7Compatible(ToXContent.Params params) {


Why is this V7 specific? Aren't the compatible params version agnostic?

good point, fixed

This reverts commit ae1799f, reversing changes made to f00f438.

This reverts commit a0fce89.

jakelandis · 2020-03-19T22:09:23Z

@pgomulka - looking good. I have question about error handling and the combinations of Accept and Content-Type headers.

Indexing with types and compatibility mode requires both Accept and Content-Type headers. This is how it is implemented and I believe is the correct behavior. We don't want to open the door (at least not now) for mixing normal requests with compatible responses (or vice-versa).

For example this works just fine:

curl -v -H "Accept: application/vnd.elasticsearch+json;compatible-with=7" -H "Content-Type: application/vnd.elasticsearch+json;compatible-with=7" -X PUT localhost:9200/foo/blah/1 -d '{ "b" : true }'

However, I think we could be more helpful for when it appears compatility mode is request. For example the following (correctly) doesn't work :

curl -v  -H "Content-Type: application/vnd.elasticsearch+json;compatible-with=7" -X PUT localhost:9200/foo/blah/1 -d '{ "b" : true }'

curl -v -H "Accept: application/json" -H "Content-Type: application/vnd.elasticsearch+json;compatible-with=7" -X PUT localhost:9200/foo/blah/1 -d '{ "b" : true }'

curl -v -H "Accept: application/vnd.elasticsearch+json;compatible-with=7"  -X PUT localhost:9200/foo/blah/1 -d '{ "b" : true }'

I think the above cases are reasonable attempts to trigger compatibility mode, but for (arguably pedantic) reasons don't. I think these should error in a meaningful way, maybe a 406 with a description stating that you need to set both the Accept and Content-Type header to trigger compatibility when a there is request body ?

I think the logic would be be if no body required on the request then only Accept is needed (we can ignore Content-Type), if a body is needed, then both Accept and Content-Type must be sent and equal to each other's compatible request, if a body is needed but only 1 of the Accept or Content-Type header is sent (or both are sent and they mismatch), then error with a 406 and meaningful error message.

To put it another way if the Content-Type is needed, then so is the Accept header, and they need to match w.r.t compatibility, but the Accept header by itself is OK assuming there is no body sent in the request.

Also we may want to surface a warning in the logs around poorly requested no-body requests. For example (the Content-Type is being ignored (I don't think the invalid version matters))

 curl  -v -H "Accept:: application/vnd.elasticsearch+json;compatible-with=7" -H "Content-Type: application/vnd.elasticsearch+json;compatible-with=99"  -X GET localhost:9200/foo/mytype/1

Also, for a valid request with correct header combinations, but invalid version, I believe that too should be an 406 error. For example:

 curl  -v -H "Accept:: application/vnd.elasticsearch+json;compatible-with=99" -X GET localhost:9200/foo/mytype/1

Thoughts ?

Also, I am fine if this happens in a follow up PR.

pgomulka · 2020-03-23T10:22:58Z

@jakelandis totally agree with all points raised. I wonder if we should not do this in a separate PR. Will raise it shortly.

pgomulka · 2020-03-23T14:02:52Z

ok to test

jakelandis

LGTM. Let's get this in the feature branch as the foundation for other compatibility changes.

jtibshirani

I didn't do an exhaustive review so I won't give an explicit approval, but all my earlier comments have been addressed. Thanks!

Adding validation of Accept And Content-Type headers. The idea is to verify combinations of presence and values of both headers depending if a request has a content, headers are versioned (type/vnd.elasticsearch+subtype;compatible-with) . It also changes the way media type is parsed (previously always assuming application as a type i.e application/json) It should expect different types like text - used in SQL Not adding a compatible headers for _cat api. These in order to return a default text do not expect Accept header (Content-type is not needed as content is not present) See #53228 (comment) for more context

pgomulka added 3 commits March 5, 2020 12:44

rest index action - 283 nofix

a4f80ca

minor tests

c9c0e55

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

pgomulka self-assigned this Mar 6, 2020

pgomulka changed the base branch from master to compat_rest_api March 6, 2020 15:21

pgomulka added the WIP label Mar 6, 2020

pgomulka added 3 commits March 6, 2020 19:35

compile fixees

258542e

Merge branch 'compat_rest_api' into compat/type-index-get

548fbd9

disable testing conventions

0dbea8a

pgomulka changed the base branch from compat_rest_api to master March 9, 2020 16:34

pgomulka changed the base branch from master to compat_rest_api March 9, 2020 16:39

jakelandis reviewed Mar 9, 2020

View reviewed changes

assertions and todo for header fix

4cf6bc1

pgomulka and others added 4 commits March 10, 2020 20:18

more tests and cleanup

3ac22b1

Merge remote-tracking branch 'upstream/compat_rest_api' into compat/t…

38721fe

…ype-index-get

introduce a module to house the REST code

f2db19f

fix preCommit

b83b4ce

jakelandis mentioned this pull request Mar 11, 2020

Introduce module for compat code pgomulka/elasticsearch#18

Merged

pgomulka added 2 commits March 11, 2020 09:33

Merge pull request #18 from jakelandis/introduce_module

cf1d340

Introduce module for compat code

move restindex compatible handlers to rest-compatibility module. 228 …

5c4a02d

…failing

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

pgomulka requested review from jtibshirani, romseygeek and rjernst March 11, 2020 10:18

jakelandis reviewed Mar 11, 2020

View reviewed changes

moving test classes and compat related code to separate v7 module

0ef7e18

code review follow up

jakelandis mentioned this pull request Mar 11, 2020

Rest API version compatibility #51816

Closed

jtibshirani reviewed Mar 11, 2020

View reviewed changes

pgomulka added 3 commits March 13, 2020 11:27

test class rename and return 400 when compatible header not present

835ce56

clean up deprecation warnings and remove use of consumers

f440231

v7 compatible actions warnings tests

84f1dde

rjernst reviewed Mar 13, 2020

View reviewed changes

pgomulka added 7 commits March 16, 2020 10:59

rename tests and enable them

d106d1b

rename isV7Compatible method

cf61bbd

checkstyle

f00f438

Merge branch 'master' into compat/type-index-get

ae1799f

Revert "Merge branch 'master' into compat/type-index-get"

a0fce89

This reverts commit ae1799f, reversing changes made to f00f438.

Merge branch 'compat_rest_api' into compat/type-index-get

7e55744

Revert "Revert "Merge branch 'master' into compat/type-index-get""

00fe62d

This reverts commit a0fce89.

pgomulka mentioned this pull request Mar 17, 2020

Remove TypeFieldMapper #53611

Closed

pgomulka added 4 commits March 17, 2020 08:57

use locale with toLowerCase

a6f0b9a

spotless

4eff534

imports and disable integ tests as there are none

2d4161c

fix tests

6830f97

pgomulka requested a review from jtibshirani March 17, 2020 16:20

jakelandis approved these changes Mar 23, 2020

View reviewed changes

jtibshirani reviewed Mar 24, 2020

View reviewed changes

pgomulka mentioned this pull request Mar 25, 2020

Types removal - compatible changes #54160

Closed

66 tasks

pgomulka merged commit 878ed12 into elastic:compat_rest_api Mar 25, 2020

pgomulka mentioned this pull request Mar 27, 2020

Validate Accept and Content-Type header for compatible API #54103

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compatible version of typed Index And Get API #53228

Compatible version of typed Index And Get API #53228

pgomulka commented Mar 6, 2020 •

edited

Loading

elasticmachine commented Mar 6, 2020

jakelandis Mar 9, 2020

jakelandis Mar 9, 2020

pgomulka commented Mar 10, 2020

jakelandis Mar 11, 2020

pgomulka Mar 11, 2020

jakelandis Mar 11, 2020

jtibshirani left a comment

jtibshirani Mar 11, 2020 •

edited

Loading

pgomulka Mar 13, 2020

jtibshirani Mar 11, 2020

pgomulka Mar 13, 2020

jtibshirani Mar 11, 2020 •

edited

Loading

pgomulka Mar 13, 2020

jtibshirani Mar 11, 2020

pgomulka Mar 13, 2020

jtibshirani Mar 11, 2020

rjernst left a comment

rjernst Mar 13, 2020

pgomulka Mar 16, 2020

jakelandis Mar 16, 2020

rjernst Mar 13, 2020

pgomulka Mar 16, 2020

pgomulka Mar 17, 2020

rjernst Mar 13, 2020

pgomulka Mar 16, 2020

jakelandis Mar 16, 2020

rjernst Mar 13, 2020

pgomulka Mar 16, 2020

jakelandis commented Mar 19, 2020 •

edited

Loading

pgomulka commented Mar 23, 2020

pgomulka commented Mar 23, 2020

jakelandis left a comment

jtibshirani left a comment


		public class CompatibleHandlers {

		public static Consumer<RestRequest> consumeParameterType(DeprecationLogger deprecationLogger) {

Compatible version of typed Index And Get API #53228

Compatible version of typed Index And Get API #53228

Conversation

pgomulka commented Mar 6, 2020 • edited Loading

elasticmachine commented Mar 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgomulka commented Mar 10, 2020

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

jtibshirani Mar 11, 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

jtibshirani Mar 11, 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

Choose a reason for hiding this comment

rjernst 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

jakelandis commented Mar 19, 2020 • edited Loading

pgomulka commented Mar 23, 2020

pgomulka commented Mar 23, 2020

jakelandis left a comment

Choose a reason for hiding this comment

jtibshirani left a comment

Choose a reason for hiding this comment

pgomulka commented Mar 6, 2020 •

edited

Loading

jtibshirani Mar 11, 2020 •

edited

Loading

jtibshirani Mar 11, 2020 •

edited

Loading

jakelandis commented Mar 19, 2020 •

edited

Loading