Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 53 commits into from
Jul 14, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
53 commits
Select commit Hold shift + click to select a range
96ebb5c
init work. 221 vs 231 failing
pgomulka Mar 25, 2020
2e922bf
allow registering multiple rest actions under the same path
pgomulka Mar 27, 2020
e7eda97
revert out dirs
pgomulka Mar 27, 2020
5dac4b9
Merge branch 'compat_rest_api' into compat/create_index_include_type
pgomulka Mar 27, 2020
a213e6c
extend restcreateindexaction
pgomulka Mar 27, 2020
f1ad677
code style and discovery nodes in indexaction
pgomulka Mar 27, 2020
8ff3463
fix double registration
pgomulka Mar 30, 2020
9b8a26a
add search action v7
pgomulka Mar 30, 2020
b0a12bd
search & multisearch
pgomulka Mar 31, 2020
b94be5c
types with consumer
pgomulka Apr 1, 2020
f78e021
cleanup type funciton
pgomulka Apr 1, 2020
9eb1c68
packages fixed
pgomulka Apr 1, 2020
a421d56
update by query, delete by query, multi term and term
pgomulka Apr 1, 2020
3364141
package rename
pgomulka Apr 1, 2020
8abcc0a
spotless
pgomulka Apr 1, 2020
6efdb30
checkstyle
pgomulka Apr 1, 2020
90c37e2
fake request build iwth compat
pgomulka Apr 1, 2020
484e4ac
fixing some tests
pgomulka Apr 1, 2020
6184a9d
term vector body not finished
pgomulka Apr 2, 2020
1ee4487
additional testing and using version
pgomulka Apr 2, 2020
abe1d28
unused method
pgomulka Apr 2, 2020
d8fc2e5
method handlers - returning null when no handler under a method was r…
pgomulka Apr 2, 2020
d998555
comments
pgomulka Apr 3, 2020
ec8df43
remove unused constant
pgomulka Apr 3, 2020
8602f65
Merge branch 'compat_rest_api' into compat/create_index_include_type
pgomulka Apr 20, 2020
f3d25e0
fix javadoc
pgomulka Apr 20, 2020
7024913
compile
pgomulka Apr 20, 2020
446c114
Merge branch 'compat/create_index_include_type' into compat/search
pgomulka Apr 20, 2020
cafee2f
spotless
pgomulka Apr 20, 2020
f52dbc1
spotless
pgomulka Apr 20, 2020
0b07539
Merge branch 'compat_rest_api' into compat/create_index_include_type
pgomulka Apr 21, 2020
07a5dbc
Merge branch 'compat/create_index_include_type' into compat/search
pgomulka Apr 21, 2020
8135794
v7 name
pgomulka Apr 22, 2020
2087c6a
Merge branch 'compat/create_index_include_type' into compat/search
pgomulka Apr 22, 2020
7e7ed63
versions and names
pgomulka Apr 22, 2020
81a62fe
spotless
pgomulka Apr 22, 2020
d65a4a2
import fix
pgomulka Apr 22, 2020
adb1a1e
fix tests
pgomulka Apr 22, 2020
f819313
fix test
pgomulka Apr 22, 2020
6e28d24
javadoc
pgomulka Apr 22, 2020
a77716e
Merge branch 'compat_rest_api' into compat/create_index_include_type
pgomulka Apr 23, 2020
dcf588c
Merge branch 'compat/create_index_include_type' into compat/search
pgomulka Apr 23, 2020
da0edd7
Merge branch 'compat_rest_api' into compat/search
pgomulka Apr 28, 2020
42b40ec
fix merge problem
pgomulka Apr 29, 2020
8912c60
Merge branch 'compat_rest_api' into compat/search
pgomulka Apr 29, 2020
6c67db3
Merge branch 'compat_rest_api' into compat/search
pgomulka Apr 29, 2020
5a720c5
Merge branch 'compat_rest_api' into compat/search
pgomulka Jun 17, 2020
5f3158c
compile fix for deprecate method rename
pgomulka Jun 17, 2020
e8b28d6
Merge branch 'compat_rest_api' into compat/search
pgomulka Jul 7, 2020
83afde4
fix after merge master
pgomulka Jul 7, 2020
660f086
spotless
pgomulka Jul 7, 2020
1f04044
Merge branch 'compat_rest_api' into compat/search
pgomulka Jul 8, 2020
24a44a8
Merge branch 'compat_rest_api' into compat/search
pgomulka Jul 8, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -1223,7 +1223,7 @@ public void testMultiSearch() throws IOException {
};
MultiSearchRequest.readMultiLineFormat(new BytesArray(EntityUtils.toByteArray(request.getEntity())),
REQUEST_BODY_CONTENT_TYPE.xContent(), consumer, null, multiSearchRequest.indicesOptions(), null, null, null,
xContentRegistry(), true);
xContentRegistry(), true, key -> false);
assertEquals(requests, multiSearchRequest.requests());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.function.Function;

import static java.util.Arrays.asList;
import static org.elasticsearch.rest.RestRequest.Method.GET;
Expand All @@ -49,7 +50,7 @@ public class RestMultiSearchTemplateAction extends BaseRestHandler {
}


private final boolean allowExplicitIndex;
protected final boolean allowExplicitIndex;

public RestMultiSearchTemplateAction(Settings settings) {
this.allowExplicitIndex = MULTI_ALLOW_EXPLICIT_INDEX.get(settings);
Expand Down Expand Up @@ -79,6 +80,19 @@ public RestChannelConsumer prepareRequest(RestRequest request, NodeClient client
* Parses a {@link RestRequest} body and returns a {@link MultiSearchTemplateRequest}
*/
public static MultiSearchTemplateRequest parseRequest(RestRequest restRequest, boolean allowExplicitIndex) throws IOException {
return parseRequest(restRequest,allowExplicitIndex, k->false);
}

/**
* Parses a {@link RestRequest} body and returns a {@link MultiSearchTemplateRequest}
* @param typeConsumer - 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.
*/
public static MultiSearchTemplateRequest parseRequest(RestRequest restRequest,
boolean allowExplicitIndex,
Function<String,Boolean> typeConsumer) throws IOException {
MultiSearchTemplateRequest multiRequest = new MultiSearchTemplateRequest();
if (restRequest.hasParam("max_concurrent_searches")) {
multiRequest.maxConcurrentSearchRequests(restRequest.paramAsInt("max_concurrent_searches", 0));
Expand All @@ -94,7 +108,7 @@ public static MultiSearchTemplateRequest parseRequest(RestRequest restRequest, b
throw new IllegalArgumentException("Malformed search template");
}
RestSearchAction.checkRestTotalHits(restRequest, searchRequest);
});
}, typeConsumer);
return multiRequest;
}

Expand Down
7 changes: 6 additions & 1 deletion modules/rest-compatibility/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,12 @@

esplugin {
description 'Adds a compatiblity layer for the prior major versions REST API'
classname 'org.elasticsearch.rest.compat.RestCompatPlugin'
classname 'org.elasticsearch.compat.RestCompatPlugin'
}

dependencies {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these two dependencies are to allow making compatible versions of Actions from these modules

implementation project(':modules:lang-mustache')
implementation project(':modules:reindex')
}

integTest.enabled = false
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
Expand All @@ -17,7 +17,7 @@
* under the License.
*/

package org.elasticsearch.rest.compat;
package org.elasticsearch.compat;

import org.elasticsearch.Version;
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver;
Expand All @@ -26,13 +26,21 @@
import org.elasticsearch.common.settings.IndexScopedSettings;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.settings.SettingsFilter;
import org.elasticsearch.index.reindex.RestDeleteByQueryActionV7;
import org.elasticsearch.index.reindex.RestUpdateByQueryActionV7;
import org.elasticsearch.plugins.ActionPlugin;
import org.elasticsearch.plugins.Plugin;
import org.elasticsearch.rest.RestController;
import org.elasticsearch.rest.RestHandler;
import org.elasticsearch.rest.compat.version7.RestCreateIndexActionV7;
import org.elasticsearch.rest.compat.version7.RestGetActionV7;
import org.elasticsearch.rest.compat.version7.RestIndexActionV7;
import org.elasticsearch.rest.action.admin.indices.RestCreateIndexActionV7;
import org.elasticsearch.rest.action.document.RestGetActionV7;
import org.elasticsearch.rest.action.document.RestIndexActionV7;
import org.elasticsearch.rest.action.document.RestMultiTermVectorsActionV7;
import org.elasticsearch.rest.action.document.RestTermVectorsActionV7;
import org.elasticsearch.rest.action.search.RestMultiSearchActionV7;
import org.elasticsearch.rest.action.search.RestSearchActionV7;
import org.elasticsearch.script.mustache.RestMultiSearchTemplateActionV7;
import org.elasticsearch.script.mustache.RestSearchTemplateActionV7;

import java.util.Collections;
import java.util.List;
Expand All @@ -52,11 +60,19 @@ public List<RestHandler> getRestHandlers(
) {
if (Version.CURRENT.major == 8) {
return List.of(
new RestDeleteByQueryActionV7(),
new RestUpdateByQueryActionV7(),
new RestCreateIndexActionV7(),
new RestGetActionV7(),
new RestIndexActionV7.CompatibleRestIndexAction(),
new RestIndexActionV7.CompatibleCreateHandler(),
new RestIndexActionV7.CompatibleAutoIdHandler(nodesInCluster),
new RestCreateIndexActionV7()
new RestTermVectorsActionV7(),
new RestMultiTermVectorsActionV7(),
new RestSearchActionV7(),
new RestMultiSearchActionV7(settings),
new RestSearchTemplateActionV7(),
new RestMultiSearchTemplateActionV7(settings)
);
}
return Collections.emptyList();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.elasticsearch.compat;

import org.elasticsearch.common.Strings;
import org.elasticsearch.rest.RestRequest;

import java.util.Set;
import java.util.function.Function;

public class TypeConsumer implements Function<String, Boolean> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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?


private final RestRequest request;
private final Set<String> fieldNames;
private boolean foundTypeInBody = false;

public TypeConsumer(RestRequest request, String... fieldNames) {
this.request = request;
this.fieldNames = Set.of(fieldNames);
}

@Override
public Boolean apply(String fieldName) {
if (fieldNames.contains(fieldName)) {
foundTypeInBody = true;
return true;
}
return false;
}

public boolean hasTypes() {
// TODO can params be types too? or _types?
String[] types = Strings.splitStringByCommaToArray(request.param("type"));
return types.length > 0 || foundTypeInBody;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.elasticsearch.index.reindex;

import org.elasticsearch.Version;
import org.elasticsearch.client.node.NodeClient;
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.rest.RestRequest;
import org.elasticsearch.rest.action.search.RestSearchActionV7;

import java.io.IOException;
import java.util.List;

import static org.elasticsearch.rest.RestRequest.Method.POST;

public class RestDeleteByQueryActionV7 extends RestDeleteByQueryAction {
private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(RestDeleteByQueryActionV7.class);

@Override
public List<Route> routes() {
return List.of(new Route(POST, "/{index}/{type}/_delete_by_query"));
}

@Override
public String getName() {
return super.getName() + "_v7";
}

@Override
public Version compatibleWithVersion() {
return Version.V_7_0_0;
}

@Override
public RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException {
if (request.hasParam("type")) {
deprecationLogger.deprecate("search_with_types", RestSearchActionV7.TYPES_DEPRECATION_MESSAGE);
request.param("type");
}
return super.prepareRequest(request, client);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.elasticsearch.index.reindex;

import org.elasticsearch.Version;
import org.elasticsearch.client.node.NodeClient;
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.rest.RestRequest;
import org.elasticsearch.rest.action.search.RestSearchActionV7;

import java.io.IOException;
import java.util.List;

import static org.elasticsearch.rest.RestRequest.Method.POST;

public class RestUpdateByQueryActionV7 extends RestUpdateByQueryAction {

private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(RestUpdateByQueryActionV7.class);

@Override
public List<Route> routes() {
return List.of(new Route(POST, "/{index}/{type}/_update_by_query"));
}

@Override
public String getName() {
return super.getName() + "_v7";
}

@Override
public Version compatibleWithVersion() {
return Version.V_7_0_0;
}

@Override
public RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException {
if (request.hasParam("type")) {
deprecationLogger.deprecate("search_with_types", RestSearchActionV7.TYPES_DEPRECATION_MESSAGE);
request.param("type");
}
return super.prepareRequest(request, client);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
Expand All @@ -17,7 +17,7 @@
* under the License.
*/

package org.elasticsearch.rest.compat.version7;
package org.elasticsearch.rest.action.admin.indices;

import org.elasticsearch.Version;
import org.elasticsearch.action.admin.indices.create.CreateIndexRequest;
Expand All @@ -28,7 +28,6 @@
import org.elasticsearch.index.mapper.MapperService;
import org.elasticsearch.rest.RestRequest;
import org.elasticsearch.rest.action.RestToXContentListener;
import org.elasticsearch.rest.action.admin.indices.RestCreateIndexAction;

import java.io.IOException;
import java.util.Collections;
Expand All @@ -46,7 +45,7 @@ public class RestCreateIndexActionV7 extends RestCreateIndexAction {

@Override
public String getName() {
return "create_index_action_v7";
return super.getName() + "_v7";
}

@Override
Expand All @@ -56,19 +55,19 @@ public Version compatibleWithVersion() {

@Override
public RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {
CreateIndexRequest createIndexRequest = prepareRequest(request);
CreateIndexRequest createIndexRequest = prepareV7Request(request);
return channel -> client.admin().indices().create(createIndexRequest, new RestToXContentListener<>(channel));
}

// default scope for testing
CreateIndexRequest prepareRequest(RestRequest request) {
CreateIndexRequest prepareV7Request(RestRequest request) {
CreateIndexRequest createIndexRequest = new CreateIndexRequest(request.param("index"));

if (request.hasContent()) {
Map<String, Object> sourceAsMap = XContentHelper.convertToMap(request.requiredContent(), false, request.getXContentType()).v2();

request.param(INCLUDE_TYPE_NAME_PARAMETER);// just consume, it is always replaced with _doc
sourceAsMap = prepareMappings(sourceAsMap, request);
sourceAsMap = prepareMappingsV7(sourceAsMap, request);

createIndexRequest.source(sourceAsMap, LoggingDeprecationHandler.INSTANCE);
}
Expand All @@ -79,7 +78,7 @@ CreateIndexRequest prepareRequest(RestRequest request) {
return createIndexRequest;
}

static Map<String, Object> prepareMappings(Map<String, Object> source, RestRequest request) {
static Map<String, Object> prepareMappingsV7(Map<String, Object> source, RestRequest request) {
final boolean includeTypeName = request.paramAsBoolean(INCLUDE_TYPE_NAME_PARAMETER, false);

@SuppressWarnings("unchecked")
Expand Down
Loading