honeymoose
/
OpenSearch
mirror of https://github.com/honeymoose/OpenSearch.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Emit deprecation warning when TermsLookup contains a type (#53731) 

TermsLookup in master no longer accepts a type parameter. We should emit
a deprecate warning in 7.x when a terms lookup requests includes type to prepare
users for its removal.

Relates to #41059
This commit is contained in:
Alan Woodward 2020-03-20 15:11:31 +00:00 committed by GitHub
parent d486bdefdd
commit a3f21f24ea
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 94 additions and 75 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/51_filter_with_types.yml
View File

@ -35,8 +35,11 @@ setup:
---
"Filter aggs with terms lookup and ensure it's cached":
# Because the filter agg rewrites the terms lookup in the rewrite phase the request can be cached
- skip:
features: allowed_warnings
- do:
allowed_warnings:
- "Deprecated field [type] used, this field is unused and will be removed entirely"
search:
rest_total_hits_as_int: true
size: 0

10
rest-api-spec/src/main/resources/rest-api-spec/test/search/150_rewrite_on_coordinator.yml
View File

@ -1,4 +1,6 @@
"Ensure that we fetch the document only once":
- skip:
features: allowed_warnings
- do:
indices.create:
index: search_index
@ -31,11 +33,13 @@
indices.refresh: {}
- do:
allowed_warnings:
- "Deprecated field [type] used, this field is unused and will be removed entirely"
catch: /no such index/
search:
rest_total_hits_as_int: true
index: "search_index"
body: { "size" : 0, "query" : { "terms" : { "user" : { "index": "lookup_index", "type": "_doc", "id": "1", "path": "followers"} } } }
body: { "size" : 0, "query" : { "terms" : { "user" : { "index": "lookup_index", "type" : "_doc", "id": "1", "path": "followers"} } } }
- do:
indices.create:
index: lookup_index
@ -55,10 +59,12 @@
indices.refresh: {}
- do:
allowed_warnings:
- "Deprecated field [type] used, this field is unused and will be removed entirely"
search:
rest_total_hits_as_int: true
index: "search_index"
body: { "size" : 0, "query" : { "terms" : { "user" : { "index": "lookup_index", "type": "_doc", "id": "1", "path": "followers"} } } }
body: { "size" : 0, "query" : { "terms" : { "user" : { "index": "lookup_index", "type" : "_doc", "id": "1", "path": "followers"} } } }
- match: { _shards.total: 5 }
- match: { _shards.successful: 5 }

3
rest-api-spec/src/main/resources/rest-api-spec/test/search/171_terms_query_with_types.yml
View File

@ -3,6 +3,7 @@
- skip:
version: " - 6.99.99"
reason: index.max_terms_count setting has been added in 7.0.0
features: allowed_warnings
- do:
indices.create:
include_type_name: true
@ -46,6 +47,8 @@
body: {"query" : {"terms" : {"user" : ["u1", "u2", "u3"]}}}
- do:
allowed_warnings:
- "Deprecated field [type] used, this field is unused and will be removed entirely"
search:
rest_total_hits_as_int: true
index: test_index

17
server/src/main/java/org/elasticsearch/index/query/TermsQueryBuilder.java
View File

@ -19,7 +19,6 @@
package org.elasticsearch.index.query;
import org.apache.logging.log4j.LogManager;
import org.apache.lucene.search.MatchAllDocsQuery;
import org.apache.lucene.search.MatchNoDocsQuery;
import org.apache.lucene.search.Query;
@ -35,13 +34,12 @@ import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.io.stream.BytesStreamOutput;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.support.XContentMapValues;
import org.elasticsearch.index.IndexSettings;
import org.elasticsearch.index.mapper.MappedFieldType;
import org.elasticsearch.index.mapper.ConstantFieldType;
import org.elasticsearch.index.mapper.MappedFieldType;
import org.elasticsearch.indices.TermsLookup;
import java.io.IOException;
@ -64,11 +62,6 @@ import java.util.stream.IntStream;
public class TermsQueryBuilder extends AbstractQueryBuilder<TermsQueryBuilder> {
public static final String NAME = "terms";
private static final DeprecationLogger deprecationLogger = new DeprecationLogger(
LogManager.getLogger(TermsQueryBuilder.class));
static final String TYPES_DEPRECATION_MESSAGE = "[types removal] Types are deprecated " +
"in [terms] lookup queries.";
private final String fieldName;
private final List<?> values;
private final TermsLookup termsLookup;
@ -402,15 +395,9 @@ public class TermsQueryBuilder extends AbstractQueryBuilder<TermsQueryBuilder> {
"followed by array of terms or a document lookup specification");
}
TermsQueryBuilder builder = new TermsQueryBuilder(fieldName, values, termsLookup)
return new TermsQueryBuilder(fieldName, values, termsLookup)
.boost(boost)
.queryName(queryName);
if (builder.isTypeless() == false) {
deprecationLogger.deprecatedAndMaybeLog("terms_lookup_with_types", TYPES_DEPRECATION_MESSAGE);
}
return builder;
}
static List<Object> parseValues(XContentParser parser) throws IOException {

65
server/src/main/java/org/elasticsearch/indices/TermsLookup.java
View File

@ -21,10 +21,11 @@ package org.elasticsearch.indices;
import org.elasticsearch.Version;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.ParsingException;
import org.elasticsearch.common.ParseField;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.io.stream.Writeable;
import org.elasticsearch.common.xcontent.ConstructingObjectParser;
import org.elasticsearch.common.xcontent.ToXContentFragment;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser;
@ -33,10 +34,14 @@ import org.elasticsearch.index.query.TermsQueryBuilder;
import java.io.IOException;
import java.util.Objects;
import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg;
import static org.elasticsearch.common.xcontent.ConstructingObjectParser.optionalConstructorArg;
/**
* Encapsulates the parameters needed to fetch terms.
*/
public class TermsLookup implements Writeable, ToXContentFragment {
private final String index;
private @Nullable String type;
private final String id;
@ -142,48 +147,24 @@ public class TermsLookup implements Writeable, ToXContentFragment {
return this;
}
private static final ConstructingObjectParser<TermsLookup, Void> PARSER = new ConstructingObjectParser<>("terms_lookup",
args -> {
String index = (String) args[0];
String type = (String) args[1];
String id = (String) args[2];
String path = (String) args[3];
return new TermsLookup(index, type, id, path);
});
static {
PARSER.declareString(constructorArg(), new ParseField("index"));
PARSER.declareString(optionalConstructorArg(), new ParseField("type").withAllDeprecated());
PARSER.declareString(constructorArg(), new ParseField("id"));
PARSER.declareString(constructorArg(), new ParseField("path"));
PARSER.declareString(TermsLookup::routing, new ParseField("routing"));
}
public static TermsLookup parseTermsLookup(XContentParser parser) throws IOException {
String index = null;
String type = null;
String id = null;
String path = null;
String routing = null;
XContentParser.Token token;
String currentFieldName = "";
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
if (token == XContentParser.Token.FIELD_NAME) {
currentFieldName = parser.currentName();
} else if (token.isValue()) {
switch (currentFieldName) {
case "index":
index = parser.text();
break;
case "type":
type = parser.text();
break;
case "id":
id = parser.text();
break;
case "routing":
routing = parser.textOrNull();
break;
case "path":
path = parser.text();
break;
default:
throw new ParsingException(parser.getTokenLocation(), "[" + TermsQueryBuilder.NAME +
"] query does not support [" + currentFieldName + "] within lookup element");
}
} else {
throw new ParsingException(parser.getTokenLocation(), "[" + TermsQueryBuilder.NAME + "] unknown token ["
+ token + "] after [" + currentFieldName + "]");
}
}
if (type == null) {
return new TermsLookup(index, id, path).routing(routing);
} else {
return new TermsLookup(index, type, id, path).routing(routing);
}
return PARSER.parse(parser, null);
}
@Override

41
server/src/test/java/org/elasticsearch/index/query/TermsQueryBuilderTests.java
View File

@ -57,6 +57,7 @@ import static org.hamcrest.Matchers.instanceOf;
public class TermsQueryBuilderTests extends AbstractQueryTestCase<TermsQueryBuilder> {
private List<Object> randomTerms;
private String termsPath;
private boolean maybeIncludeType = true;
@Before
public void randomTerms() {
@ -100,10 +101,10 @@ public class TermsQueryBuilderTests extends AbstractQueryTestCase<TermsQueryBuil
private TermsLookup randomTermsLookup() {
// Randomly choose between a typeless terms lookup and one with an explicit type to make sure we are
TermsLookup lookup = maybeIncludeType && randomBoolean()
? new TermsLookup(randomAlphaOfLength(10), randomAlphaOfLength(10), randomAlphaOfLength(10), termsPath)
: new TermsLookup(randomAlphaOfLength(10), randomAlphaOfLength(10), termsPath);
// testing both cases.
TermsLookup lookup = randomBoolean()
? new TermsLookup(randomAlphaOfLength(10), randomAlphaOfLength(10), termsPath)
: new TermsLookup(randomAlphaOfLength(10), randomAlphaOfLength(10), randomAlphaOfLength(10), termsPath);
lookup.routing(randomBoolean() ? randomAlphaOfLength(10) : null);
return lookup;
}
@ -149,7 +150,13 @@ public class TermsQueryBuilderTests extends AbstractQueryTestCase<TermsQueryBuil
}
}
public void testEmtpyFieldName() {
@Override
public void testUnknownField() throws IOException {
maybeIncludeType = false; // deprecation warnings will fail the parent test, so we disable types
super.testUnknownField();
}
public void testEmptyFieldName() {
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> new TermsQueryBuilder(null, "term"));
assertEquals("field name cannot be null.", e.getMessage());
e = expectThrows(IllegalArgumentException.class, () -> new TermsQueryBuilder("", "term"));
@ -326,32 +333,36 @@ public class TermsQueryBuilderTests extends AbstractQueryTestCase<TermsQueryBuil
builder.doToQuery(createShardContext());
assertWarnings(QueryShardContext.TYPES_DEPRECATION_MESSAGE);
}
public void testRewriteIndexQueryToMatchNone() throws IOException {
TermsQueryBuilder query = new TermsQueryBuilder("_index", "does_not_exist", "also_does_not_exist");
QueryShardContext queryShardContext = createShardContext();
QueryBuilder rewritten = query.rewrite(queryShardContext);
assertThat(rewritten, instanceOf(MatchNoneQueryBuilder.class));
}
}
public void testRewriteIndexQueryToNotMatchNone() throws IOException {
// At least one name is good
TermsQueryBuilder query = new TermsQueryBuilder("_index", "does_not_exist", getIndex().getName());
QueryShardContext queryShardContext = createShardContext();
QueryBuilder rewritten = query.rewrite(queryShardContext);
assertThat(rewritten, instanceOf(MatchAllQueryBuilder.class));
}
}
@Override
protected QueryBuilder parseQuery(XContentParser parser) throws IOException {
QueryBuilder query = super.parseQuery(parser);
assertThat(query, CoreMatchers.instanceOf(TermsQueryBuilder.class));
try {
QueryBuilder query = super.parseQuery(parser);
assertThat(query, CoreMatchers.instanceOf(TermsQueryBuilder.class));
TermsQueryBuilder termsQuery = (TermsQueryBuilder) query;
if (termsQuery.isTypeless() == false) {
assertWarnings("Deprecated field [type] used, this field is unused and will be removed entirely");
}
return query;
} finally {
TermsQueryBuilder termsQuery = (TermsQueryBuilder) query;
if (termsQuery.isTypeless() == false) {
assertWarnings(TermsQueryBuilder.TYPES_DEPRECATION_MESSAGE);
}
return query;
}
}

28
server/src/test/java/org/elasticsearch/indices/TermsLookupTests.java
View File

@ -22,6 +22,8 @@ package org.elasticsearch.indices;
import org.elasticsearch.Version;
import org.elasticsearch.common.io.stream.BytesStreamOutput;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.json.JsonXContent;
import org.elasticsearch.test.ESTestCase;
import java.io.IOException;
@ -105,6 +107,32 @@ public class TermsLookupTests extends ESTestCase {
}
}
public void testXContentParsingWithType() throws IOException {
XContentParser parser = createParser(JsonXContent.jsonXContent,
"{ \"index\" : \"index\", \"id\" : \"id\", \"type\" : \"type\", \"path\" : \"path\", \"routing\" : \"routing\" }");
TermsLookup tl = TermsLookup.parseTermsLookup(parser);
assertEquals("index", tl.index());
assertEquals("type", tl.type());
assertEquals("id", tl.id());
assertEquals("path", tl.path());
assertEquals("routing", tl.routing());
assertWarnings("Deprecated field [type] used, this field is unused and will be removed entirely");
}
public void testXContentParsing() throws IOException {
XContentParser parser = createParser(JsonXContent.jsonXContent,
"{ \"index\" : \"index\", \"id\" : \"id\", \"path\" : \"path\", \"routing\" : \"routing\" }");
TermsLookup tl = TermsLookup.parseTermsLookup(parser);
assertEquals("index", tl.index());
assertNull(tl.type());
assertEquals("id", tl.id());
assertEquals("path", tl.path());
assertEquals("routing", tl.routing());
}
public static TermsLookup randomTermsLookup() {
return new TermsLookup(
randomAlphaOfLength(10),