From 41263556ac491322608faf2578cf1b7834935b34 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 21 Jul 2020 19:07:38 -0400 Subject: [PATCH 1/2] Standardize script field's rejection error This standardizes the exception type thrown when attempting to build a query on a script field that it doesn't support. It also replaces the field type `[script]` in the error message with whatever the runtime type is. --- .../mapper/AbstractScriptMappedFieldType.java | 95 +++++++++++++++++++ ...tNonTextScriptMappedFieldTypeTestCase.java | 20 ++-- ...AbstractScriptMappedFieldTypeTestCase.java | 30 ++++++ .../ScriptDoubleMappedFieldTypeTests.java | 5 + .../ScriptKeywordMappedFieldTypeTests.java | 10 ++ .../ScriptLongMappedFieldTypeTests.java | 5 + 6 files changed, 157 insertions(+), 8 deletions(-) diff --git a/x-pack/plugin/runtime-fields/src/main/java/org/elasticsearch/xpack/runtimefields/mapper/AbstractScriptMappedFieldType.java b/x-pack/plugin/runtime-fields/src/main/java/org/elasticsearch/xpack/runtimefields/mapper/AbstractScriptMappedFieldType.java index e569ee0b8f653..710d6bc1e536e 100644 --- a/x-pack/plugin/runtime-fields/src/main/java/org/elasticsearch/xpack/runtimefields/mapper/AbstractScriptMappedFieldType.java +++ b/x-pack/plugin/runtime-fields/src/main/java/org/elasticsearch/xpack/runtimefields/mapper/AbstractScriptMappedFieldType.java @@ -6,12 +6,23 @@ package org.elasticsearch.xpack.runtimefields.mapper; +import org.apache.lucene.analysis.TokenStream; +import org.apache.lucene.search.MultiTermQuery; +import org.apache.lucene.search.Query; +import org.apache.lucene.search.spans.SpanMultiTermQueryWrapper; +import org.apache.lucene.search.spans.SpanQuery; import org.elasticsearch.ElasticsearchException; +import org.elasticsearch.common.geo.ShapeRelation; +import org.elasticsearch.common.time.DateMathParser; +import org.elasticsearch.common.unit.Fuzziness; import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.mapper.TextSearchInfo; import org.elasticsearch.index.query.QueryShardContext; import org.elasticsearch.script.Script; +import java.io.IOException; +import java.time.ZoneId; +import java.util.List; import java.util.Map; import static org.elasticsearch.search.SearchService.ALLOW_EXPENSIVE_QUERIES; @@ -49,6 +60,90 @@ public final boolean isAggregatable() { return true; } + public abstract Query termsQuery(List values, QueryShardContext context); + + public abstract Query rangeQuery( + Object lowerTerm, + Object upperTerm, + boolean includeLower, + boolean includeUpper, + ShapeRelation relation, + ZoneId timeZone, + DateMathParser parser, + QueryShardContext context + ); + + public Query fuzzyQuery( + Object value, + Fuzziness fuzziness, + int prefixLength, + int maxExpansions, + boolean transpositions, + QueryShardContext context + ) { + throw new IllegalArgumentException( + "Can only use fuzzy queries on keyword and text fields - not on [" + name() + "] which is of type [" + runtimeType() + "]" + ); + } + + public Query prefixQuery(String value, MultiTermQuery.RewriteMethod method, QueryShardContext context) { + throw new IllegalArgumentException( + "Can only use prefix queries on keyword, text and wildcard fields - not on [" + + name() + + "] which is of type [" + + runtimeType() + + "]" + ); + } + + public Query wildcardQuery(String value, MultiTermQuery.RewriteMethod method, QueryShardContext context) { + throw new IllegalArgumentException( + "Can only use wildcard queries on keyword, text and wildcard fields - not on [" + + name() + + "] which is of type [" + + runtimeType() + + "]" + ); + } + + public Query regexpQuery( + String value, + int flags, + int maxDeterminizedStates, + MultiTermQuery.RewriteMethod method, + QueryShardContext context + ) { + throw new IllegalArgumentException( + "Can only use regexp queries on keyword and text fields - not on [" + name() + "] which is of type [" + runtimeType() + "]" + ); + } + + public abstract Query existsQuery(QueryShardContext context); + + public Query phraseQuery(TokenStream stream, int slop, boolean enablePositionIncrements) throws IOException { + throw new IllegalArgumentException( + "Can only use phrase queries on text fields - not on [" + name() + "] which is of type [" + runtimeType() + "]" + ); + } + + public Query multiPhraseQuery(TokenStream stream, int slop, boolean enablePositionIncrements) throws IOException { + throw new IllegalArgumentException( + "Can only use phrase queries on text fields - not on [" + name() + "] which is of type [" + runtimeType() + "]" + ); + } + + public Query phrasePrefixQuery(TokenStream stream, int slop, int maxExpansions) throws IOException { + throw new IllegalArgumentException( + "Can only use phrase prefix queries on text fields - not on [" + name() + "] which is of type [" + runtimeType() + "]" + ); + } + + public SpanQuery spanPrefixQuery(String value, SpanMultiTermQueryWrapper.SpanRewriteMethod method, QueryShardContext context) { + throw new IllegalArgumentException( + "Can only use span prefix queries on text fields - not on [" + name() + "] which is of type [" + runtimeType() + "]" + ); + } + protected final void checkAllowExpensiveQueries(QueryShardContext context) { if (context.allowExpensiveQueries() == false) { throw new ElasticsearchException( diff --git a/x-pack/plugin/runtime-fields/src/test/java/org/elasticsearch/xpack/runtimefields/mapper/AbstractNonTextScriptMappedFieldTypeTestCase.java b/x-pack/plugin/runtime-fields/src/test/java/org/elasticsearch/xpack/runtimefields/mapper/AbstractNonTextScriptMappedFieldTypeTestCase.java index 4df5c6636fffc..53808d67e8237 100644 --- a/x-pack/plugin/runtime-fields/src/test/java/org/elasticsearch/xpack/runtimefields/mapper/AbstractNonTextScriptMappedFieldTypeTestCase.java +++ b/x-pack/plugin/runtime-fields/src/test/java/org/elasticsearch/xpack/runtimefields/mapper/AbstractNonTextScriptMappedFieldTypeTestCase.java @@ -14,8 +14,6 @@ import static org.hamcrest.Matchers.equalTo; abstract class AbstractNonTextScriptMappedFieldTypeTestCase extends AbstractScriptMappedFieldTypeTestCase { - protected abstract AbstractScriptMappedFieldType simpleMappedFieldType() throws IOException; - public void testFuzzyQueryIsError() throws IOException { assertQueryOnlyOnTextAndKeyword( "fuzzy", @@ -39,20 +37,26 @@ public void testWildcardQueryIsError() throws IOException { } private void assertQueryOnlyOnTextAndKeyword(String queryName, ThrowingRunnable buildQuery) { - // TODO use runtime type in the error message and a consistent exception type - Exception e = expectThrows(Exception.class, buildQuery); + Exception e = expectThrows(IllegalArgumentException.class, buildQuery); assertThat( e.getMessage(), - equalTo("Can only use " + queryName + " queries on keyword and text fields - not on [test] which is of type [script]") + equalTo( + "Can only use " + queryName + " queries on keyword and text fields - not on [test] which is of type [" + runtimeType() + "]" + ) ); } private void assertQueryOnlyOnTextKeywordAndWildcard(String queryName, ThrowingRunnable buildQuery) { - // TODO use runtime type in the error message and a consistent exception type - Exception e = expectThrows(Exception.class, buildQuery); + Exception e = expectThrows(IllegalArgumentException.class, buildQuery); assertThat( e.getMessage(), - equalTo("Can only use " + queryName + " queries on keyword, text and wildcard fields - not on [test] which is of type [script]") + equalTo( + "Can only use " + + queryName + + " queries on keyword, text and wildcard fields - not on [test] which is of type [" + + runtimeType() + + "]" + ) ); } } diff --git a/x-pack/plugin/runtime-fields/src/test/java/org/elasticsearch/xpack/runtimefields/mapper/AbstractScriptMappedFieldTypeTestCase.java b/x-pack/plugin/runtime-fields/src/test/java/org/elasticsearch/xpack/runtimefields/mapper/AbstractScriptMappedFieldTypeTestCase.java index c9f9b6a1f313d..95809f07126d7 100644 --- a/x-pack/plugin/runtime-fields/src/test/java/org/elasticsearch/xpack/runtimefields/mapper/AbstractScriptMappedFieldTypeTestCase.java +++ b/x-pack/plugin/runtime-fields/src/test/java/org/elasticsearch/xpack/runtimefields/mapper/AbstractScriptMappedFieldTypeTestCase.java @@ -15,12 +15,17 @@ import java.io.IOException; +import static org.hamcrest.Matchers.equalTo; import static org.mockito.Matchers.any; import static org.mockito.Matchers.anyString; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; abstract class AbstractScriptMappedFieldTypeTestCase extends ESTestCase { + protected abstract AbstractScriptMappedFieldType simpleMappedFieldType() throws IOException; + + protected abstract String runtimeType(); + public abstract void testDocValues() throws IOException; public abstract void testSort() throws IOException; @@ -71,4 +76,29 @@ protected static QueryShardContext mockContext(boolean allowExpensiveQueries, Ab when(context.lookup()).thenReturn(lookup); return context; } + + public void testPhraseQueryIsError() throws IOException { + assertQueryOnlyOnText("phrase", () -> simpleMappedFieldType().phraseQuery(null, 1, false)); + } + + public void testPhrasePrefixQueryIsError() throws IOException { + assertQueryOnlyOnText("phrase prefix", () -> simpleMappedFieldType().phrasePrefixQuery(null, 1, 1)); + } + + public void testMultiPhraseQueryIsError() throws IOException { + assertQueryOnlyOnText("phrase", () -> simpleMappedFieldType().multiPhraseQuery(null, 1, false)); + } + + public void testSpanPrefixQueryIsError() throws IOException { + assertQueryOnlyOnText("span prefix", () -> simpleMappedFieldType().spanPrefixQuery(null, null, null)); + } + + private void assertQueryOnlyOnText(String queryName, ThrowingRunnable buildQuery) { + Exception e = expectThrows(IllegalArgumentException.class, buildQuery); + assertThat( + e.getMessage(), + equalTo("Can only use " + queryName + " queries on text fields - not on [test] which is of type [" + runtimeType() + "]") + ); + } + } diff --git a/x-pack/plugin/runtime-fields/src/test/java/org/elasticsearch/xpack/runtimefields/mapper/ScriptDoubleMappedFieldTypeTests.java b/x-pack/plugin/runtime-fields/src/test/java/org/elasticsearch/xpack/runtimefields/mapper/ScriptDoubleMappedFieldTypeTests.java index 8eddac4e2e7e2..ef395fe87baa1 100644 --- a/x-pack/plugin/runtime-fields/src/test/java/org/elasticsearch/xpack/runtimefields/mapper/ScriptDoubleMappedFieldTypeTests.java +++ b/x-pack/plugin/runtime-fields/src/test/java/org/elasticsearch/xpack/runtimefields/mapper/ScriptDoubleMappedFieldTypeTests.java @@ -252,6 +252,11 @@ protected ScriptDoubleMappedFieldType simpleMappedFieldType() throws IOException return build("value(source.foo)"); } + @Override + protected String runtimeType() { + return "double"; + } + private static ScriptDoubleMappedFieldType build(String code) throws IOException { return build(new Script(code)); } diff --git a/x-pack/plugin/runtime-fields/src/test/java/org/elasticsearch/xpack/runtimefields/mapper/ScriptKeywordMappedFieldTypeTests.java b/x-pack/plugin/runtime-fields/src/test/java/org/elasticsearch/xpack/runtimefields/mapper/ScriptKeywordMappedFieldTypeTests.java index 833e3a811a39c..de85aec7acdc2 100644 --- a/x-pack/plugin/runtime-fields/src/test/java/org/elasticsearch/xpack/runtimefields/mapper/ScriptKeywordMappedFieldTypeTests.java +++ b/x-pack/plugin/runtime-fields/src/test/java/org/elasticsearch/xpack/runtimefields/mapper/ScriptKeywordMappedFieldTypeTests.java @@ -331,6 +331,16 @@ public void testMatchQuery() throws IOException { } } + @Override + protected AbstractScriptMappedFieldType simpleMappedFieldType() throws IOException { + return build("value(source.foo)"); + } + + @Override + protected String runtimeType() { + return "keyword"; + } + private static ScriptKeywordMappedFieldType build(String code) throws IOException { return build(new Script(code)); } diff --git a/x-pack/plugin/runtime-fields/src/test/java/org/elasticsearch/xpack/runtimefields/mapper/ScriptLongMappedFieldTypeTests.java b/x-pack/plugin/runtime-fields/src/test/java/org/elasticsearch/xpack/runtimefields/mapper/ScriptLongMappedFieldTypeTests.java index d2fda8ed776e2..b377c7e8346a5 100644 --- a/x-pack/plugin/runtime-fields/src/test/java/org/elasticsearch/xpack/runtimefields/mapper/ScriptLongMappedFieldTypeTests.java +++ b/x-pack/plugin/runtime-fields/src/test/java/org/elasticsearch/xpack/runtimefields/mapper/ScriptLongMappedFieldTypeTests.java @@ -245,6 +245,11 @@ protected ScriptLongMappedFieldType simpleMappedFieldType() throws IOException { return build("value(source.foo)"); } + @Override + protected String runtimeType() { + return "long"; + } + private static ScriptLongMappedFieldType build(String code) throws IOException { return build(new Script(code)); } From 628163c7575d9d42b07477e5c9eef3ca3b2790c0 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Wed, 22 Jul 2020 14:12:59 -0400 Subject: [PATCH 2/2] Update error messages --- .../mapper/AbstractScriptMappedFieldType.java | 45 ++++++------------- ...tNonTextScriptMappedFieldTypeTestCase.java | 8 +++- ...AbstractScriptMappedFieldTypeTestCase.java | 8 +++- 3 files changed, 26 insertions(+), 35 deletions(-) diff --git a/x-pack/plugin/runtime-fields/src/main/java/org/elasticsearch/xpack/runtimefields/mapper/AbstractScriptMappedFieldType.java b/x-pack/plugin/runtime-fields/src/main/java/org/elasticsearch/xpack/runtimefields/mapper/AbstractScriptMappedFieldType.java index 710d6bc1e536e..f616f5799c4d3 100644 --- a/x-pack/plugin/runtime-fields/src/main/java/org/elasticsearch/xpack/runtimefields/mapper/AbstractScriptMappedFieldType.java +++ b/x-pack/plugin/runtime-fields/src/main/java/org/elasticsearch/xpack/runtimefields/mapper/AbstractScriptMappedFieldType.java @@ -81,29 +81,15 @@ public Query fuzzyQuery( boolean transpositions, QueryShardContext context ) { - throw new IllegalArgumentException( - "Can only use fuzzy queries on keyword and text fields - not on [" + name() + "] which is of type [" + runtimeType() + "]" - ); + throw new IllegalArgumentException(unsupported("fuzzy", "keyword and text")); } public Query prefixQuery(String value, MultiTermQuery.RewriteMethod method, QueryShardContext context) { - throw new IllegalArgumentException( - "Can only use prefix queries on keyword, text and wildcard fields - not on [" - + name() - + "] which is of type [" - + runtimeType() - + "]" - ); + throw new IllegalArgumentException(unsupported("prefix", "keyword, text and wildcard")); } public Query wildcardQuery(String value, MultiTermQuery.RewriteMethod method, QueryShardContext context) { - throw new IllegalArgumentException( - "Can only use wildcard queries on keyword, text and wildcard fields - not on [" - + name() - + "] which is of type [" - + runtimeType() - + "]" - ); + throw new IllegalArgumentException(unsupported("wildcard", "keyword, text and wildcard")); } public Query regexpQuery( @@ -113,35 +99,30 @@ public Query regexpQuery( MultiTermQuery.RewriteMethod method, QueryShardContext context ) { - throw new IllegalArgumentException( - "Can only use regexp queries on keyword and text fields - not on [" + name() + "] which is of type [" + runtimeType() + "]" - ); + throw new IllegalArgumentException(unsupported("regexp", "keyword and text")); } public abstract Query existsQuery(QueryShardContext context); public Query phraseQuery(TokenStream stream, int slop, boolean enablePositionIncrements) throws IOException { - throw new IllegalArgumentException( - "Can only use phrase queries on text fields - not on [" + name() + "] which is of type [" + runtimeType() + "]" - ); + throw new IllegalArgumentException(unsupported("phrase", "text")); } public Query multiPhraseQuery(TokenStream stream, int slop, boolean enablePositionIncrements) throws IOException { - throw new IllegalArgumentException( - "Can only use phrase queries on text fields - not on [" + name() + "] which is of type [" + runtimeType() + "]" - ); + throw new IllegalArgumentException(unsupported("phrase", "text")); } public Query phrasePrefixQuery(TokenStream stream, int slop, int maxExpansions) throws IOException { - throw new IllegalArgumentException( - "Can only use phrase prefix queries on text fields - not on [" + name() + "] which is of type [" + runtimeType() + "]" - ); + throw new IllegalArgumentException(unsupported("phrase prefix", "text")); } public SpanQuery spanPrefixQuery(String value, SpanMultiTermQueryWrapper.SpanRewriteMethod method, QueryShardContext context) { - throw new IllegalArgumentException( - "Can only use span prefix queries on text fields - not on [" + name() + "] which is of type [" + runtimeType() + "]" - ); + throw new IllegalArgumentException(unsupported("span prefix", "text")); + } + + private String unsupported(String query, String supported) { + String thisField = "[" + name() + "] which is of type [script] with runtime_type [" + runtimeType() + "]"; + return "Can only use " + query + " queries on " + supported + " fields - not on " + thisField; } protected final void checkAllowExpensiveQueries(QueryShardContext context) { diff --git a/x-pack/plugin/runtime-fields/src/test/java/org/elasticsearch/xpack/runtimefields/mapper/AbstractNonTextScriptMappedFieldTypeTestCase.java b/x-pack/plugin/runtime-fields/src/test/java/org/elasticsearch/xpack/runtimefields/mapper/AbstractNonTextScriptMappedFieldTypeTestCase.java index 53808d67e8237..602d84310b761 100644 --- a/x-pack/plugin/runtime-fields/src/test/java/org/elasticsearch/xpack/runtimefields/mapper/AbstractNonTextScriptMappedFieldTypeTestCase.java +++ b/x-pack/plugin/runtime-fields/src/test/java/org/elasticsearch/xpack/runtimefields/mapper/AbstractNonTextScriptMappedFieldTypeTestCase.java @@ -41,7 +41,11 @@ private void assertQueryOnlyOnTextAndKeyword(String queryName, ThrowingRunnable assertThat( e.getMessage(), equalTo( - "Can only use " + queryName + " queries on keyword and text fields - not on [test] which is of type [" + runtimeType() + "]" + "Can only use " + + queryName + + " queries on keyword and text fields - not on [test] which is of type [script] with runtime_type [" + + runtimeType() + + "]" ) ); } @@ -53,7 +57,7 @@ private void assertQueryOnlyOnTextKeywordAndWildcard(String queryName, ThrowingR equalTo( "Can only use " + queryName - + " queries on keyword, text and wildcard fields - not on [test] which is of type [" + + " queries on keyword, text and wildcard fields - not on [test] which is of type [script] with runtime_type [" + runtimeType() + "]" ) diff --git a/x-pack/plugin/runtime-fields/src/test/java/org/elasticsearch/xpack/runtimefields/mapper/AbstractScriptMappedFieldTypeTestCase.java b/x-pack/plugin/runtime-fields/src/test/java/org/elasticsearch/xpack/runtimefields/mapper/AbstractScriptMappedFieldTypeTestCase.java index 95809f07126d7..6d45cd81d4a88 100644 --- a/x-pack/plugin/runtime-fields/src/test/java/org/elasticsearch/xpack/runtimefields/mapper/AbstractScriptMappedFieldTypeTestCase.java +++ b/x-pack/plugin/runtime-fields/src/test/java/org/elasticsearch/xpack/runtimefields/mapper/AbstractScriptMappedFieldTypeTestCase.java @@ -97,7 +97,13 @@ private void assertQueryOnlyOnText(String queryName, ThrowingRunnable buildQuery Exception e = expectThrows(IllegalArgumentException.class, buildQuery); assertThat( e.getMessage(), - equalTo("Can only use " + queryName + " queries on text fields - not on [test] which is of type [" + runtimeType() + "]") + equalTo( + "Can only use " + + queryName + + " queries on text fields - not on [test] which is of type [script] with runtime_type [" + + runtimeType() + + "]" + ) ); }