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

pgomulka · 2020-04-01T11:04:09Z

This adds a compatible APIs for typed endpoints removed in #41640

202 failing tests
These 2 tests are explicitly fixed by this PR
CompatRestIT. test {yaml=mtermvectors/21_deprecated_with_types/Deprecated camel case and _ parameters should fail in Term Vectors query}
CompatRestIT. test {yaml=mtermvectors/30_mix_typeless_typeful/mtermvectors without types on an index that has types}

I think more yml tests should be added to 7.x to cover these endpoints

17th june
1306tests | 197failures | 16ignored | 10m56.91sduration

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

elasticmachine · 2020-04-01T11:04:11Z

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

…egistered

jakelandis

This is really starting to take shape! I have a few comments and stopped with the repetitive comments (you can assume the same comment applies through out)

jakelandis · 2020-04-02T18:36:21Z

...ility/src/main/java/org/elasticsearch/rest/action/document/RestMultiTermVectorsActionV7.java

+        return List.of(new Route(GET, "/_mtermvectors"),
+            new Route(POST, "/_mtermvectors"),
+            new Route(GET, "/{index}/_mtermvectors"),
+            new Route(POST, "/{index}/_mtermvectors"),


should the the above non-typed endpoints be here ? Shouldn't the this error trying to register the route ?

we allow registering same method and path but under different version. (if someone tried to register same method and path and version twice - it would end with error)
We need this because multi term vectors (and most multi-* api) can have a type in a body. So a typed request can be send via non-typed endpoint.

jakelandis · 2020-04-02T18:42:29Z

...-mustache/src/main/java/org/elasticsearch/script/mustache/RestMultiSearchTemplateAction.java

+
+    /**
+     * Parses a {@link RestRequest} body and returns a {@link MultiSearchTemplateRequest}
+     * @param typeConsumer - is a function used when parsing a request body. if it contains a types field it will consume it,


can this be re-named keyAllowed ? (or the like)

A function used to validate if a provided xContent key is allowed. This is useful for xContent compatibility to determine if a key is allowed to be present in version agnostic manner. The provided function should return false if the key is not allowed.

(it is does not use the Consumer interface, and I believe it more general purpose then the just for types)

good idea, I wasn't sure typeConsumer was right as it was just too specific for type removal eample.
Will rename and update javadoc

jakelandis · 2020-04-02T19:11:51Z

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

+import java.util.Set;
+import java.util.function.Function;
+
+public class TypeConsumer implements Function<String, Boolean> {


I think this class has a couple problems...
it is named Consumer, but actually a Function with a side effect
it requires an ordering between methods. (e.g. if you call hasTypes can yield different results then calling apply then hasTypes)

hasTypes() method feels utility like, and apply is a pure function (which shouldn't have side effects even local to it's own class).

Can we separate this into two classes , a utility class and pure function class. If you need to hold to the state returned by the function, whoever calls that function can take that responsibility.

Also, in tangent with my other comment on renaming the variable, I think the function class be be generalized as well KeyAllowed ?

Agree - the name is not right. The consumer was an initial idea but it ended up a function and I didn't change it accordingly. The KeyAllowed also feels right to me.

I am not sure I understand how to refactor this. My idea was that the typeConsumer object being created at the V7 layer and passed down to v8 code can bubble up the state (information if type was found) back to v7 layer. Allowing the code to take action (most of the time just deprecation warning) to be present in v7 layer.

The apply() method was meant to serve two functions. Retrieve and store the information for v7 layer if the type was found. And to tell v8 code that the key was allowed and exception when parsing is not necessary.

I also agree that side effect is probably unexpected. The TypeConsumer is visible as just a Function in v8 layer, so noone looking just at code which uses it would expect side effects.
If we have two classes, that would mean we would need to pass down two objects down to v8 layer?

@jakelandis I think this is the main issue on that PR.
We can split this in two functions

KeyAllowed<String,Boolean> - a function returning true if a string is of allowed value (_type, types etc) false otherwise. This will then be pass down into a Request parsing logic - i.e. https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/action/termvectors/TermVectorsRequest.java#L588

second class - not sure about this one.. we need to go down from v7 rest layer to the same place where a request is parsed (v8 code).
I understand that it is not obvious that a function would has a state. But from the parsing point of view it wouldn't matter. All it care in TermVectorsRequest.java#L588 is if the field is allowed. The state if the field occurred (type for that example) is only relevant in v7 code where a class can be used directly (not by the Function interface)

If we were to have a utility class it would have live within some common to all modules utility module. That would not be ideal I guess.
We could refactor hasTypes to be a utility that is consuming the function that was passed down. But I am not sure this would benefit much (except from solving the ordering)

Anyways I think I might be missing something in your idea of splitting this class into two. Can you please give more details?

jakelandis · 2020-04-02T19:15:21Z

...t-compatibility/src/main/java/org/elasticsearch/index/reindex/RestDeleteByQueryActionV7.java

+
+    @Override
+    public RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException {
+        request.param("type");


Do you plan another round of PRs to issue deprecation and compatibility warnings ?

I think it would be good to include this in this PR too, so we could consider these API complete. Will revisit.

jakelandis · 2020-04-02T19:16:22Z

...patibility/src/main/java/org/elasticsearch/rest/action/document/RestTermVectorsActionV7.java

+            new Route(GET, "/{index}/_termvectors"),
+            new Route(POST, "/{index}/_termvectors"),
+            new Route(GET, "/{index}/_termvectors/{id}"),
+            new Route(POST, "/{index}/_termvectors/{id}"),


are the non-typed endpoints needed ?

this one also can expect a type in a body for all these paths (even non-typed) - hence it is using TypeConsumer (to be renamed).

jakelandis · 2020-04-02T19:17:49Z

...patibility/src/main/java/org/elasticsearch/rest/action/document/RestTermVectorsActionV7.java

+        }
+        readURIParameters(termVectorsRequest, request);
+
+        if (typeConsumer.hasTypes()) {


If you move hasTypes a static method in a utility class, maybe we can push this logic (consume the type and log) there ?

jakelandis · 2020-04-02T19:18:09Z

...ompatibility/src/main/java/org/elasticsearch/rest/action/search/RestMultiSearchActionV7.java

+            new Route(GET, "/_msearch"),
+            new Route(POST, "/_msearch"),
+            new Route(GET, "/{index}/_msearch"),
+            new Route(POST, "/{index}/_msearch"),


are the non-typed end points needed ?

as per previous comments - type is present in a body

jakelandis · 2020-04-02T19:18:27Z

...est-compatibility/src/main/java/org/elasticsearch/rest/action/search/RestSearchActionV7.java

+            new Route(GET, "/_search"),
+            new Route(POST, "/_search"),
+            new Route(GET, "/{index}/_search"),
+            new Route(POST, "/{index}/_search"),


non-typed endpoints ?

type can be present as a parameter in v7 (as well as in a path for typed endpoints)

although I am now not sure about INCLUDE_TYPE_NAME_PARAMETER
will revisit

jakelandis · 2020-04-02T19:19:40Z

...ibility/src/main/java/org/elasticsearch/script/mustache/RestMultiSearchTemplateActionV7.java

+            new Route(GET, "/_msearch/template"),
+            new Route(POST, "/_msearch/template"),
+            new Route(GET, "/{index}/_msearch/template"),
+            new Route(POST, "/{index}/_msearch/template"),


non-typed endpoints ?

again - type can be present in a body.

pgomulka added 13 commits March 25, 2020 16:38

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

extend restcreateindexaction

a213e6c

code style and discovery nodes in indexaction

f1ad677

fix double registration

8ff3463

add search action v7

9b8a26a

search & multisearch

b0a12bd

types with consumer

b94be5c

cleanup type funciton

f78e021

packages fixed

9eb1c68

update by query, delete by query, multi term and term

a421d56

pgomulka added WIP :Core/Infra/REST API REST infrastructure and utilities labels Apr 1, 2020

pgomulka self-assigned this Apr 1, 2020

package rename

3364141

pgomulka mentioned this pull request Apr 1, 2020

Types removal - compatible changes #54160

Closed

66 tasks

pgomulka added 2 commits April 1, 2020 13:54

spotless

8abcc0a

checkstyle

6efdb30

pgomulka requested a review from jakelandis April 1, 2020 12:19

pgomulka added 6 commits April 1, 2020 15:37

fake request build iwth compat

90c37e2

fixing some tests

484e4ac

term vector body not finished

6184a9d

additional testing and using version

1ee4487

unused method

abe1d28

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

d8fc2e5

…egistered

jakelandis requested changes Apr 2, 2020

View reviewed changes

comments

d998555

pgomulka added 17 commits April 20, 2020 15:56

spotless

f52dbc1

Merge branch 'compat_rest_api' into compat/create_index_include_type

0b07539

Merge branch 'compat/create_index_include_type' into compat/search

07a5dbc

v7 name

8135794

Merge branch 'compat/create_index_include_type' into compat/search

2087c6a

versions and names

7e7ed63

spotless

81a62fe

import fix

d65a4a2

fix tests

adb1a1e

fix test

f819313

javadoc

6e28d24

Merge branch 'compat_rest_api' into compat/create_index_include_type

a77716e

Merge branch 'compat/create_index_include_type' into compat/search

dcf588c

Merge branch 'compat_rest_api' into compat/search

da0edd7

fix merge problem

42b40ec

Merge branch 'compat_rest_api' into compat/search

8912c60

Merge branch 'compat_rest_api' into compat/search

6c67db3

pgomulka changed the title ~~WIP Compatible logic for Removes typed endpoint from search and related APIs~~ Compatible logic for Removes typed endpoint from search and related APIs Apr 29, 2020

pgomulka marked this pull request as ready for review April 29, 2020 14:14

pgomulka removed the WIP label Apr 30, 2020

rjernst added the Team:Core/Infra Meta label for core/infra team label May 4, 2020

pgomulka added 7 commits June 17, 2020 11:33

Merge branch 'compat_rest_api' into compat/search

5a720c5

compile fix for deprecate method rename

5f3158c

Merge branch 'compat_rest_api' into compat/search

e8b28d6

fix after merge master

83afde4

spotless

660f086

Merge branch 'compat_rest_api' into compat/search

1f04044

Merge branch 'compat_rest_api' into compat/search

24a44a8

pgomulka merged commit 2df6157 into elastic:compat_rest_api Jul 14, 2020

ChrisHegarty unassigned pgomulka Oct 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

pgomulka commented Apr 1, 2020 •

edited

Loading

elasticmachine commented Apr 1, 2020

jakelandis left a comment

jakelandis Apr 2, 2020

pgomulka Apr 3, 2020 •

edited

Loading

jakelandis Apr 2, 2020

pgomulka Apr 3, 2020

jakelandis Apr 2, 2020

pgomulka Apr 3, 2020

pgomulka May 15, 2020

jakelandis Apr 2, 2020

pgomulka Apr 3, 2020

jakelandis Apr 2, 2020

pgomulka Apr 3, 2020 •

edited

Loading

jakelandis Apr 2, 2020

jakelandis Apr 2, 2020

pgomulka Apr 3, 2020

jakelandis Apr 2, 2020

pgomulka Apr 3, 2020

pgomulka Apr 3, 2020

jakelandis Apr 2, 2020

pgomulka Apr 3, 2020

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

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

Conversation

pgomulka commented Apr 1, 2020 • edited Loading

elasticmachine commented Apr 1, 2020

jakelandis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgomulka Apr 3, 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

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 Apr 3, 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

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 Apr 1, 2020 •

edited

Loading

pgomulka Apr 3, 2020 •

edited

Loading

pgomulka Apr 3, 2020 •

edited

Loading