-
Notifications
You must be signed in to change notification settings - Fork 25.2k
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
Changes from 19 commits
96ebb5c
2e922bf
e7eda97
5dac4b9
a213e6c
f1ad677
8ff3463
9b8a26a
b0a12bd
b94be5c
f78e021
9eb1c68
a421d56
3364141
8abcc0a
6efdb30
90c37e2
484e4ac
6184a9d
1ee4487
abe1d28
d8fc2e5
d998555
ec8df43
8602f65
f3d25e0
7024913
446c114
cafee2f
f52dbc1
0b07539
07a5dbc
8135794
2087c6a
7e7ed63
81a62fe
d65a4a2
adb1a1e
f819313
6e28d24
a77716e
dcf588c
da0edd7
42b40ec
8912c60
6c67db3
5a720c5
5f3158c
e8b28d6
83afde4
660f086
1f04044
24a44a8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
compile project(':modules:lang-mustache') | ||
compile project(':modules:reindex') | ||
} | ||
|
||
integTest.enabled = false | ||
|
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.HashSet; | ||
import java.util.Set; | ||
import java.util.function.Function; | ||
|
||
public class TypeConsumer implements Function<String, Boolean> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this class has a couple problems...
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 The I also agree that side effect is probably unexpected. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jakelandis I think this is the main issue on that PR.
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. 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,47 @@ | ||
/* | ||
* 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.rest.RestRequest; | ||
|
||
import java.io.IOException; | ||
import java.util.List; | ||
|
||
import static org.elasticsearch.rest.RestRequest.Method.POST; | ||
|
||
public class RestDeleteByQueryActionV7 extends RestDeleteByQueryAction { | ||
@Override | ||
public List<Route> routes() { | ||
return List.of(new Route(POST, "/{index}/{type}/_delete_by_query")); | ||
} | ||
|
||
@Override | ||
public String compatibleWithVersion() { | ||
return String.valueOf(Version.V_7_0_0.major); | ||
} | ||
|
||
@Override | ||
public RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException { | ||
request.param("type"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you plan another round of PRs to issue deprecation and compatibility warnings ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be good to include this in this PR too, so we could consider these API complete. Will revisit. |
||
return super.prepareRequest(request, client); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
/* | ||
* 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.rest.RestRequest; | ||
|
||
import java.io.IOException; | ||
import java.util.List; | ||
|
||
import static org.elasticsearch.rest.RestRequest.Method.POST; | ||
|
||
public class RestUpdateByQueryActionV7 extends RestUpdateByQueryAction { | ||
@Override | ||
public List<Route> routes() { | ||
return List.of(new Route(POST, "/{index}/{type}/_update_by_query")); | ||
} | ||
|
||
@Override | ||
public String compatibleWithVersion() { | ||
return String.valueOf(Version.V_7_0_0.major); | ||
} | ||
|
||
@Override | ||
public RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException { | ||
request.param("type"); | ||
return super.prepareRequest(request, client); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,109 @@ | ||
/* | ||
* 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.rest.action.admin.indices; | ||
|
||
import org.elasticsearch.Version; | ||
import org.elasticsearch.action.admin.indices.create.CreateIndexRequest; | ||
import org.elasticsearch.action.support.ActiveShardCount; | ||
import org.elasticsearch.client.node.NodeClient; | ||
import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; | ||
import org.elasticsearch.common.xcontent.XContentHelper; | ||
import org.elasticsearch.index.mapper.MapperService; | ||
import org.elasticsearch.rest.RestRequest; | ||
import org.elasticsearch.rest.action.RestToXContentListener; | ||
|
||
import java.io.IOException; | ||
import java.util.Collections; | ||
import java.util.HashMap; | ||
import java.util.Map; | ||
|
||
import static java.util.Collections.singletonMap; | ||
|
||
public class RestCreateIndexActionV7 extends RestCreateIndexAction { | ||
|
||
/** | ||
* Parameter that controls whether certain REST apis should include type names in their requests or responses. | ||
* Note: Support for this parameter will be removed after the transition period to typeless APIs. | ||
*/ | ||
public static final String INCLUDE_TYPE_NAME_PARAMETER = "include_type_name"; | ||
public static final boolean DEFAULT_INCLUDE_TYPE_NAME_POLICY = false; | ||
|
||
@Override | ||
public String compatibleWithVersion() { | ||
return String.valueOf(Version.V_7_0_0.major); | ||
} | ||
|
||
@Override | ||
public RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException { | ||
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 not replaced with _doc | ||
sourceAsMap = prepareMappingsV7(sourceAsMap, request); | ||
|
||
createIndexRequest.source(sourceAsMap, LoggingDeprecationHandler.INSTANCE); | ||
} | ||
|
||
createIndexRequest.timeout(request.paramAsTime("timeout", createIndexRequest.timeout())); | ||
createIndexRequest.masterNodeTimeout(request.paramAsTime("master_timeout", createIndexRequest.masterNodeTimeout())); | ||
createIndexRequest.waitForActiveShards(ActiveShardCount.parseString(request.param("wait_for_active_shards"))); | ||
return channel -> client.admin().indices().create(createIndexRequest, new RestToXContentListener<>(channel)); | ||
} | ||
|
||
static Map<String, Object> prepareMappingsV7(Map<String, Object> source, RestRequest request) { | ||
final boolean includeTypeName = request.paramAsBoolean(INCLUDE_TYPE_NAME_PARAMETER, DEFAULT_INCLUDE_TYPE_NAME_POLICY); | ||
|
||
@SuppressWarnings("unchecked") | ||
Map<String, Object> mappings = (Map<String, Object>) source.get("mappings"); | ||
|
||
if (includeTypeName && mappings.size() == 1) { | ||
// no matter what the type was, replace it with _doc | ||
Map<String, Object> newSource = new HashMap<>(); | ||
|
||
String typeName = mappings.keySet().iterator().next(); | ||
@SuppressWarnings("unchecked") | ||
Map<String, Object> typedMappings = (Map<String, Object>) mappings.get(typeName); | ||
|
||
newSource.put("mappings", Collections.singletonMap(MapperService.SINGLE_MAPPING_NAME, typedMappings)); | ||
return newSource; | ||
} else { | ||
return prepareMappings(source); | ||
} | ||
} | ||
|
||
static Map<String, Object> prepareMappings(Map<String, Object> source) { | ||
if (source.containsKey("mappings") == false || (source.get("mappings") instanceof Map) == false) { | ||
return source; | ||
} | ||
|
||
Map<String, Object> newSource = new HashMap<>(source); | ||
|
||
@SuppressWarnings("unchecked") | ||
Map<String, Object> mappings = (Map<String, Object>) source.get("mappings"); | ||
if (MapperService.isMappingSourceTyped(MapperService.SINGLE_MAPPING_NAME, mappings)) { | ||
throw new IllegalArgumentException("The mapping definition cannot be nested under a type"); | ||
} | ||
|
||
newSource.put("mappings", singletonMap(MapperService.SINGLE_MAPPING_NAME, mappings)); | ||
return newSource; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be re-named
keyAllowed
? (or the like)(it is does not use the Consumer interface, and I believe it more general purpose then the just for types)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea, I wasn't sure
typeConsumer
was right as it was just too specific for type removal eample.Will rename and update javadoc