Handle ignored fields directly in SourceValueFetcher

modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/TokenCountFieldMapper.java

@@ -83,7 +83,7 @@ static class TokenCountFieldType extends NumberFieldMapper.NumberFieldType {
         @Override
         public ValueFetcher valueFetcher(SearchExecutionContext context, String format) {
             if (hasDocValues() == false) {
-                return (lookup, ignoredFields) -> List.of();
+                return lookup -> List.of();
             }
             return new DocValueFetcher(docValueFormat(format, null), context.getForField(this));
         }

rest-api-spec/src/main/resources/rest-api-spec/test/search/330_fetch_fields.yml

@@ -335,7 +335,7 @@ Test unmapped field:
               - f1
               - { "field" : "f4", "include_unmapped" : true }
   -  match:
-        hits.hits.0.fields.f1: 
+        hits.hits.0.fields.f1:
         - some text
   -  match:
         hits.hits.0.fields.f4:
@@ -347,7 +347,7 @@ Test unmapped field:
               fields:
               - { "field" : "f*", "include_unmapped" : true }
   -  match:
-        hits.hits.0.fields.f1: 
+        hits.hits.0.fields.f1:
         - some text
   -  match:
         hits.hits.0.fields.f2\.a:
@@ -381,7 +381,7 @@ Test unmapped fields inside disabled objects:
            id: 1
            refresh: true
            body:
-              f1: 
+              f1:
                  - some text
                  - a: b
                  -
@@ -809,9 +809,44 @@ Test nested field with sibling field resolving to DocValueFetcher:
         hits.hits.0.fields.owner:
           - "Anna Ott"
   - match:
-        hits.hits.0.fields.owner\.length: 
+        hits.hits.0.fields.owner\.length:
         - 2
   - match:
         hits.hits.0.fields.products.0: { "manufacturer" : ["Supersoft"]}
   - match:
         hits.hits.0.fields.products.1: { "manufacturer" : ["HyperSmart"]}
+
+---
+"Test ignores malformed values while returning valid ones":
+  - skip:
+      version: ' - 8.0.0'
+      reason:  'Added in 8.0 - change on backport'
+  - do:
+      indices.create:
+        index: test
+        body:
+          mappings:
+            properties:
+              number:
+                type: long
+                ignore_malformed: true
+  - do:
+      index:
+        index: test
+        id: 1
+        refresh: true
+        body:
+          number: [ 1, 2, "3", "four", 5, 6 ]
+
+  - do:
+      search:
+        index: test
+        body:
+          fields: [ "*" ]
+
+  - length: { hits.hits.0.fields.number : 5 }
+  - match: { hits.hits.0.fields.number.0 : 1 }
+  - match: { hits.hits.0.fields.number.1 : 2 }
+  - match: { hits.hits.0.fields.number.2 : 3 }
+  - match: { hits.hits.0.fields.number.3 : 5 }
+  - match: { hits.hits.0.fields.number.4 : 6 }

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

@@ -27,7 +27,6 @@
 public abstract class ArraySourceValueFetcher implements ValueFetcher {
     private final Set<String> sourcePaths;
     private final @Nullable Object nullValue;
-    private final String fieldName;
 
     public ArraySourceValueFetcher(String fieldName, SearchExecutionContext context) {
         this(fieldName, context, null);
@@ -41,21 +40,23 @@ public ArraySourceValueFetcher(String fieldName, SearchExecutionContext context)
     public ArraySourceValueFetcher(String fieldName, SearchExecutionContext context, Object nullValue) {
         this.sourcePaths = context.sourcePath(fieldName);
         this.nullValue = nullValue;
-        this.fieldName = fieldName;
     }
 
     @Override
-    public List<Object> fetchValues(SourceLookup lookup, Set<String> ignoredFields) {
+    public List<Object> fetchValues(SourceLookup lookup) {
         List<Object> values = new ArrayList<>();
-        if (ignoredFields.contains(fieldName)) {
-            return values;
-        }
         for (String path : sourcePaths) {
             Object sourceValue = lookup.extractValue(path, nullValue);
             if (sourceValue == null) {
                 return List.of();
             }
-            values.addAll((List<?>) parseSourceValue(sourceValue));
+            try {
+                values.addAll((List<?>) parseSourceValue(sourceValue));
+            } catch (Exception e) {
+                // if parsing fails here then it would have failed at index time
+                // as well, meaning that we must be ignoring malformed values.
+                // So ignore it here too.
+            }
         }
         return values;
     }

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

@@ -17,7 +17,6 @@
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.List;
-import java.util.Set;
 
 import static java.util.Collections.emptyList;
 
@@ -40,7 +39,7 @@ public void setNextReader(LeafReaderContext context) {
     }
 
     @Override
-    public List<Object> fetchValues(SourceLookup lookup, Set<String> ignoredFields) throws IOException {
+    public List<Object> fetchValues(SourceLookup lookup) throws IOException {
         if (false == formattedDocValues.advanceExact(lookup.docId())) {
             return emptyList();
         }

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

@@ -19,7 +19,6 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.Set;
 
 public class NestedValueFetcher implements ValueFetcher {
 
@@ -39,7 +38,7 @@ public NestedValueFetcher(String nestedField, FieldFetcher nestedFieldFetcher) {
     }
 
     @Override
-    public List<Object> fetchValues(SourceLookup lookup, Set<String> ignoreFields) throws IOException {
+    public List<Object> fetchValues(SourceLookup lookup) throws IOException {
         List<Object> nestedEntriesToReturn = new ArrayList<>();
         Map<String, Object> filteredSource = new HashMap<>();
         Map<String, Object> stub = createSourceMapStub(filteredSource);
@@ -53,7 +52,7 @@ public List<Object> fetchValues(SourceLookup lookup, Set<String> ignoreFields) t
             SourceLookup nestedSourceLookup = new SourceLookup();
             nestedSourceLookup.setSource(filteredSource);
 
-            Map<String, DocumentField> fetchResult = nestedFieldFetcher.fetch(nestedSourceLookup, ignoreFields);
+            Map<String, DocumentField> fetchResult = nestedFieldFetcher.fetch(nestedSourceLookup);
 
             Map<String, Object> nestedEntry = new HashMap<>();
             for (DocumentField field : fetchResult.values()) {

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

@@ -28,29 +28,23 @@
 public abstract class SourceValueFetcher implements ValueFetcher {
     private final Set<String> sourcePaths;
     private final @Nullable Object nullValue;
-    private final String fieldName;
 
     public SourceValueFetcher(String fieldName, SearchExecutionContext context) {
         this(fieldName, context, null);
     }
 
     /**
-     * @param fieldName The name of the field.
      * @param context The query shard context
      * @param nullValue A optional substitute value if the _source value is 'null'.
      */
     public SourceValueFetcher(String fieldName, SearchExecutionContext context, Object nullValue) {
         this.sourcePaths = context.sourcePath(fieldName);
         this.nullValue = nullValue;
-        this.fieldName = fieldName;
     }
 
     @Override
-    public List<Object> fetchValues(SourceLookup lookup, Set<String> ignoredFields) {
+    public List<Object> fetchValues(SourceLookup lookup) {
         List<Object> values = new ArrayList<>();
-        if (ignoredFields.contains(fieldName)) {
-            return values;
-        }
         for (String path : sourcePaths) {
             Object sourceValue = lookup.extractValue(path, nullValue);
             if (sourceValue == null) {
@@ -66,9 +60,16 @@ public List<Object> fetchValues(SourceLookup lookup, Set<String> ignoredFields)
                 if (value instanceof List) {
                     queue.addAll((List<?>) value);
                 } else {
-                    Object parsedValue = parseSourceValue(value);
-                    if (parsedValue != null) {
-                        values.add(parsedValue);
+                    try {
+                        Object parsedValue = parseSourceValue(value);
+                        if (parsedValue != null) {
+                            values.add(parsedValue);
+                        }
+                    } catch (Exception e) {
+                        // if we get a parsing exception here, that means that the
+                        // value in _source would have also caused a parsing
+                        // exception at index time and the value ignored.
+                        // so ignore it here as well
                     }
                 }
             }

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

@@ -9,13 +9,11 @@
 package org.elasticsearch.index.mapper;
 
 import org.apache.lucene.index.LeafReaderContext;
-import org.elasticsearch.common.Nullable;
 import org.elasticsearch.search.fetch.subphase.FetchFieldsPhase;
 import org.elasticsearch.search.lookup.SourceLookup;
 
 import java.io.IOException;
 import java.util.List;
-import java.util.Set;
 
 /**
  * A helper class for fetching field values during the {@link FetchFieldsPhase}. Each {@link MappedFieldType}
@@ -33,10 +31,9 @@ public interface ValueFetcher {
     * should not be relied on.
     *
     * @param lookup a lookup structure over the document's source.
-    * @param ignoredFields the fields in _ignored that have been ignored for this document because they were malformed
     * @return a list a standardized field values.
     */
-    List<Object> fetchValues(SourceLookup lookup, @Nullable Set<String> ignoredFields) throws IOException;
+    List<Object> fetchValues(SourceLookup lookup) throws IOException;
 
     /**
      * Update the leaf reader used to fetch values.

server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchDocValuesPhase.java

@@ -18,7 +18,6 @@
 
 import java.io.IOException;
 import java.util.ArrayList;
-import java.util.Collections;
 import java.util.List;
 
 /**
@@ -71,7 +70,7 @@ public void process(HitContext hit) throws IOException {
                         // docValues fields will still be document fields, and put under "fields" section of a hit.
                         hit.hit().setDocumentField(f.field, hitField);
                     }
-                    hitField.getValues().addAll(f.fetcher.fetchValues(hit.sourceLookup(), Collections.emptySet()));
+                    hitField.getValues().addAll(f.fetcher.fetchValues(hit.sourceLookup()));
                 }
             }
         };

server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchFieldsPhase.java

@@ -10,17 +10,14 @@
 
 import org.apache.lucene.index.LeafReaderContext;
 import org.elasticsearch.common.document.DocumentField;
-import org.elasticsearch.index.mapper.IgnoredFieldMapper;
 import org.elasticsearch.search.SearchHit;
 import org.elasticsearch.search.fetch.FetchContext;
 import org.elasticsearch.search.fetch.FetchSubPhase;
 import org.elasticsearch.search.fetch.FetchSubPhaseProcessor;
 import org.elasticsearch.search.lookup.SourceLookup;
 
 import java.io.IOException;
-import java.util.HashSet;
 import java.util.Map;
-import java.util.Set;
 
 /**
  * A fetch sub-phase for high-level field retrieval. Given a list of fields, it
@@ -53,25 +50,11 @@ public void process(HitContext hitContext) throws IOException {
                 SearchHit hit = hitContext.hit();
                 SourceLookup sourceLookup = hitContext.sourceLookup();
 
-                Set<String> ignoredFields = getIgnoredFields(hit);
-                Map<String, DocumentField> documentFields = fieldFetcher.fetch(sourceLookup, ignoredFields);
+                Map<String, DocumentField> documentFields = fieldFetcher.fetch(sourceLookup);
                 for (Map.Entry<String, DocumentField> entry : documentFields.entrySet()) {
                     hit.setDocumentField(entry.getKey(), entry.getValue());
                 }
             }
         };
     }
-
-    private Set<String> getIgnoredFields(SearchHit hit) {
-        DocumentField field = hit.field(IgnoredFieldMapper.NAME);
-        if (field == null) {
-            return Set.of();
-        }
-
-        Set<String> ignoredFields = new HashSet<>();
-        for (Object value : field.getValues()) {
-            ignoredFields.add((String) value);
-        }
-        return ignoredFields;
-    }
 }

server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldFetcher.java

@@ -134,14 +134,13 @@ private FieldFetcher(
         this.unmappedFieldsFetchAutomaton = unmappedFieldsFetchAutomaton;
     }
 
-    public Map<String, DocumentField> fetch(SourceLookup sourceLookup, Set<String> ignoredFields) throws IOException {
-        assert ignoredFields != null;
+    public Map<String, DocumentField> fetch(SourceLookup sourceLookup) throws IOException {
         Map<String, DocumentField> documentFields = new HashMap<>();
         for (FieldContext context : fieldContexts.values()) {
             String field = context.fieldName;
 
             ValueFetcher valueFetcher = context.valueFetcher;
-            List<Object> parsedValues = valueFetcher.fetchValues(sourceLookup, ignoredFields);
+            List<Object> parsedValues = valueFetcher.fetchValues(sourceLookup);
             if (parsedValues.isEmpty() == false) {
                 documentFields.put(field, new DocumentField(field, parsedValues));
             }

server/src/main/java/org/elasticsearch/search/fetch/subphase/highlight/HighlightUtils.java

@@ -48,7 +48,7 @@ public static List<Object> loadFieldValues(MappedFieldType fieldType,
         }
         ValueFetcher fetcher = fieldType.valueFetcher(searchContext, null);
         fetcher.setNextReader(hitContext.readerContext());
-        return fetcher.fetchValues(hitContext.sourceLookup(), Collections.emptySet());
+        return fetcher.fetchValues(hitContext.sourceLookup());
     }
 
     public static class Encoders {

server/src/test/java/org/elasticsearch/index/mapper/GeoPointFieldTypeTests.java

@@ -41,5 +41,9 @@ public void testFetchSourceValue() throws IOException {
         sourceValue = "POINT (42.0 27.1)";
         assertEquals(List.of(jsonPoint), fetchSourceValue(mapper, sourceValue, null));
         assertEquals(List.of(wktPoint), fetchSourceValue(mapper, sourceValue, "wkt"));
+
+        // Test a malformed value
+        sourceValue = "malformed";
+        assertEquals(List.of(), fetchSourceValue(mapper, sourceValue, null));
     }
 }

server/src/test/java/org/elasticsearch/index/mapper/NumberFieldTypeTests.java

@@ -639,6 +639,7 @@ public void testFetchSourceValue() throws IOException {
             .fieldType();
         assertEquals(List.of(3), fetchSourceValue(mapper, 3.14));
         assertEquals(List.of(42), fetchSourceValue(mapper, "42.9"));
+        assertEquals(List.of(3, 42), fetchSourceValues(mapper, 3.14, "foo", "42.9"));
 
         MappedFieldType nullValueMapper = new NumberFieldMapper.Builder("field", NumberType.FLOAT, false, true)
             .nullValue(2.71f)