Ensure field types consistency on supporting text queries (#63487)

Some supported field types don't support term queries, and throw exception in their termQuery method. That exception is either an IllegalArgumentException or a QueryShardException. There is logic in MatchQuery that skips the field or not depending on the exception that is thrown.

Also, such field types should hold a TextSearchInfo.NONE while that is not always the case.

With this commit we make the following changes:

- streamline using TextSearchInfo.NONE in all field types that don't support text queries
- standardize the exception being thrown when a field type does not support term queries to be IllegalArgumentException. Note that this is not a breaking change as both exceptions previously returned translated to 400 status code.
- Adapt the MatchQuery logic to skip fields that don't support term queries. There is no need to call termQuery passing an empty string and catch exceptions potentially thrown. We can rather check the TextSearchInfo which tells already whether the field supports text queries or not.
- add a test method to MapperTestCase that verifies the consistency of a field type by verifying that it is not searchable whenever it uses TextSearchInfo.NONE, while it is otherwise. This is what triggered all of the above changes.

Author: javanna

modules/mapper-extras/src/test/java/org/elasticsearch/index/mapper/RankFeatureFieldMapperTests.java

@@ -39,8 +39,8 @@
 public class RankFeatureFieldMapperTests extends MapperTestCase {
 
     @Override
-    protected void writeFieldValue(XContentBuilder builder) throws IOException {
-        builder.value(10);
+    protected Object getSampleValueForDocument() {
+        return 10;
     }
 
     @Override
@@ -57,6 +57,12 @@ protected void assertExistsQuery(MappedFieldType fieldType, Query query, ParseCo
         assertNotNull(fields.getField("_feature"));
     }
 
+    @Override
+    protected void assertSearchable(MappedFieldType fieldType) {
+        //always searchable even if it uses TextSearchInfo.NONE
+        assertTrue(fieldType.isSearchable());
+    }
+
     @Override
     protected Collection<? extends Plugin> getPlugins() {
         return List.of(new MapperExtrasPlugin());

modules/mapper-extras/src/test/java/org/elasticsearch/index/mapper/RankFeaturesFieldMapperTests.java

@@ -30,12 +30,13 @@
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.List;
+import java.util.Map;
 
 public class RankFeaturesFieldMapperTests extends MapperTestCase {
 
     @Override
-    protected void writeFieldValue(XContentBuilder builder) throws IOException {
-        builder.startObject().field("foo", 10).field("bar", 20).endObject();
+    protected Object getSampleValueForDocument() {
+        return Map.of("ten", 10, "twenty", 20);
     }
 
     @Override
@@ -73,10 +74,17 @@ public void testDefaults() throws Exception {
         IndexableField[] fields = doc1.rootDoc().getFields("field");
         assertEquals(2, fields.length);
         assertThat(fields[0], Matchers.instanceOf(FeatureField.class));
-        FeatureField featureField1 = (FeatureField) fields[0];
-        assertThat(featureField1.stringValue(), Matchers.equalTo("foo"));
-        FeatureField featureField2 = (FeatureField) fields[1];
-        assertThat(featureField2.stringValue(), Matchers.equalTo("bar"));
+        FeatureField featureField1 = null;
+        FeatureField featureField2 = null;
+        for (IndexableField field : fields) {
+            if (field.stringValue().equals("ten")) {
+                featureField1 = (FeatureField)field;
+            } else if (field.stringValue().equals("twenty")) {
+                featureField2 = (FeatureField)field;
+            } else {
+                throw new UnsupportedOperationException();
+            }
+        }
 
         int freq1 = RankFeatureFieldMapperTests.getFrequency(featureField1.tokenStream(null, null));
         int freq2 = RankFeatureFieldMapperTests.getFrequency(featureField2.tokenStream(null, null));

modules/mapper-extras/src/test/java/org/elasticsearch/index/mapper/ScaledFloatFieldMapperTests.java

@@ -44,8 +44,8 @@ protected Collection<? extends Plugin> getPlugins() {
     }
 
     @Override
-    protected void writeFieldValue(XContentBuilder builder) throws IOException {
-        builder.value(123);
+    protected Object getSampleValueForDocument() {
+        return 123;
     }
 
     @Override

modules/mapper-extras/src/test/java/org/elasticsearch/index/mapper/SearchAsYouTypeFieldMapperTests.java

@@ -122,8 +122,9 @@ protected void registerParameters(ParameterChecker checker) throws IOException {
 
     }
 
-    protected void writeFieldValue(XContentBuilder builder) throws IOException {
-        builder.value("new york city");
+    @Override
+    protected Object getSampleValueForDocument() {
+        return "new york city";
     }
 
     @Override

modules/mapper-extras/src/test/java/org/elasticsearch/index/mapper/TokenCountFieldMapperTests.java

@@ -57,8 +57,13 @@ protected void minimalMapping(XContentBuilder b) throws IOException {
     }
 
     @Override
-    protected void writeFieldValue(XContentBuilder builder) throws IOException {
-        builder.value("some words");
+    protected Object getSampleValueForDocument() {
+        return "some words";
+    }
+
+    @Override
+    protected Object getSampleValueForQuery() {
+        return 1;
     }
 
     @Override

plugins/analysis-icu/src/test/java/org/elasticsearch/index/mapper/ICUCollationKeywordFieldMapperTests.java

@@ -69,8 +69,8 @@ protected void minimalMapping(XContentBuilder b) throws IOException {
     }
 
     @Override
-    protected void writeFieldValue(XContentBuilder builder) throws IOException {
-        builder.value(1234);
+    protected Object getSampleValueForDocument() {
+        return 1234;
     }
 
     public void testDefaults() throws Exception {

plugins/mapper-annotated-text/src/internalClusterTest/java/org/elasticsearch/index/mapper/annotatedtext/AnnotatedTextFieldMapperTests.java

@@ -80,8 +80,8 @@ protected void minimalMapping(XContentBuilder b) throws IOException {
     }
 
     @Override
-    protected void writeFieldValue(XContentBuilder builder) throws IOException {
-        builder.value("some text");
+    protected Object getSampleValueForDocument() {
+        return "some text";
     }
 
     @Override

plugins/mapper-murmur3/src/main/java/org/elasticsearch/index/mapper/murmur3/Murmur3FieldMapper.java

@@ -38,7 +38,6 @@
 import org.elasticsearch.index.mapper.TextSearchInfo;
 import org.elasticsearch.index.mapper.ValueFetcher;
 import org.elasticsearch.index.query.QueryShardContext;
-import org.elasticsearch.index.query.QueryShardException;
 import org.elasticsearch.search.lookup.SearchLookup;
 
 import java.io.IOException;
@@ -91,7 +90,7 @@ public Murmur3FieldMapper build(BuilderContext context) {
     // this only exists so a check can be done to match the field type to using murmur3 hashing...
     public static class Murmur3FieldType extends MappedFieldType {
         private Murmur3FieldType(String name, boolean isStored, Map<String, String> meta) {
-            super(name, false, isStored, true, TextSearchInfo.SIMPLE_MATCH_ONLY, meta);
+            super(name, false, isStored, true, TextSearchInfo.NONE, meta);
         }
 
         @Override
@@ -112,7 +111,7 @@ public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searc
 
         @Override
         public Query termQuery(Object value, QueryShardContext context) {
-            throw new QueryShardException(context, "Murmur3 fields are not searchable: [" + name() + "]");
+            throw new IllegalArgumentException("Murmur3 fields are not searchable: [" + name() + "]");
         }
     }
 

plugins/mapper-murmur3/src/test/java/org/elasticsearch/index/mapper/murmur3/Murmur3FieldMapperTests.java

@@ -37,8 +37,8 @@
 public class Murmur3FieldMapperTests extends MapperTestCase {
 
     @Override
-    protected void writeFieldValue(XContentBuilder builder) throws IOException {
-        builder.value("value");
+    protected Object getSampleValueForDocument() {
+        return "value";
     }
 
     @Override

server/src/main/java/org/elasticsearch/index/mapper/AbstractGeometryFieldMapper.java

@@ -33,7 +33,6 @@
 import org.elasticsearch.common.xcontent.support.MapXContentParser;
 import org.elasticsearch.common.xcontent.support.XContentMapValues;
 import org.elasticsearch.index.query.QueryShardContext;
-import org.elasticsearch.index.query.QueryShardException;
 import org.elasticsearch.search.lookup.SearchLookup;
 
 import java.io.IOException;
@@ -224,7 +223,7 @@ public abstract static class AbstractGeometryFieldType<Parsed, Processed> extend
 
         protected AbstractGeometryFieldType(String name, boolean indexed, boolean stored, boolean hasDocValues,
                                             boolean parsesArrayValue, Map<String, String> meta) {
-            super(name, indexed, stored, hasDocValues, TextSearchInfo.SIMPLE_MATCH_ONLY, meta);
+            super(name, indexed, stored, hasDocValues, TextSearchInfo.NONE, meta);
             this.parsesArrayValue = parsesArrayValue;
         }
 
@@ -245,14 +244,13 @@ protected Parser<Parsed> geometryParser() {
         }
 
         @Override
-        public Query termQuery(Object value, QueryShardContext context) {
-            throw new QueryShardException(context,
-                "Geometry fields do not support exact searching, use dedicated geometry queries instead: ["
+        public final Query termQuery(Object value, QueryShardContext context) {
+            throw new IllegalArgumentException("Geometry fields do not support exact searching, use dedicated geometry queries instead: ["
                     + name() + "]");
         }
 
         @Override
-        public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searchLookup, String format) {
+        public final ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searchLookup, String format) {
             String geoFormat = format != null ? format : GeoJsonGeometryFormat.NAME;
 
             Function<Object, Object> valueParser = value -> geometryParser.parseAndFormatObject(value, geoFormat);