Make array value parsing flag more robust. (#63371)

When constructing a value fetcher, the 'parsesArrayValue' flag must match `FieldMapper#parsesArrayValue`. However there is nothing in code or tests to help enforce this. This PR reworks the value fetcher constructors so that `parsesArrayValue` is 'false' by default. Just as for `FieldMapper#parsesArrayValue`, field types must explicitly set it to true and ensure the behavior is covered by tests. Follow-up to #62974.
elastic · Oct 7, 2020 · f17ca18 · f17ca18
1 parent 2b838d1
commit f17ca18
Show file tree

Hide file tree

Showing 28 changed files with 140 additions and 54 deletions.
diff --git a/...es/mapper-extras/src/main/java/org/elasticsearch/index/mapper/RankFeatureFieldMapper.java b/...es/mapper-extras/src/main/java/org/elasticsearch/index/mapper/RankFeatureFieldMapper.java
@@ -118,7 +118,7 @@ public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searc
             if (format != null) {
                 throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
             }
-            return new SourceValueFetcher(name(), mapperService, false) {
+            return new SourceValueFetcher(name(), mapperService) {
                 @Override
                 protected Float parseSourceValue(Object value) {
                     return objectToFloat(value);

diff --git a/...s/mapper-extras/src/main/java/org/elasticsearch/index/mapper/RankFeaturesFieldMapper.java b/...s/mapper-extras/src/main/java/org/elasticsearch/index/mapper/RankFeaturesFieldMapper.java
@@ -93,7 +93,7 @@ public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searc
             if (format != null) {
                 throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
             }
-            return new SourceValueFetcher(name(), mapperService, false) {
+            return new SourceValueFetcher(name(), mapperService) {
                 @Override
                 protected Object parseSourceValue(Object value) {
                     return value;

diff --git a/...es/mapper-extras/src/main/java/org/elasticsearch/index/mapper/ScaledFloatFieldMapper.java b/...es/mapper-extras/src/main/java/org/elasticsearch/index/mapper/ScaledFloatFieldMapper.java
@@ -225,7 +225,7 @@ public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searc
             if (format != null) {
                 throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
             }
-            return new SourceValueFetcher(name(), mapperService, false) {
+            return new SourceValueFetcher(name(), mapperService) {
                 @Override
                 protected Double parseSourceValue(Object value) {
                     double doubleValue;

diff --git a/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java b/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java
@@ -227,7 +227,7 @@ public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searc
             if (format != null) {
                 throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
             }
-            return new SourceValueFetcher(name(), mapperService, false) {
+            return new SourceValueFetcher(name(), mapperService) {
                 @Override
                 protected Object parseSourceValue(Object value) {
                     return value;

diff --git a/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java b/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java
@@ -229,7 +229,7 @@ public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searc
             if (format != null) {
                 throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
             }
-            return new SourceValueFetcher(name(), mapperService, false) {
+            return new SourceValueFetcher(name(), mapperService) {
                 @Override
                 protected Object parseSourceValue(Object value) {
                     return value;

diff --git a/...ysis-icu/src/main/java/org/elasticsearch/index/mapper/ICUCollationKeywordFieldMapper.java b/...ysis-icu/src/main/java/org/elasticsearch/index/mapper/ICUCollationKeywordFieldMapper.java
@@ -104,7 +104,7 @@ public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searc
                 throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
             }
 
-            return new SourceValueFetcher(name(), mapperService, false, nullValue) {
+            return new SourceValueFetcher(name(), mapperService, nullValue) {
                 @Override
                 protected String parseSourceValue(Object value) {
                     String keywordValue = value.toString();

diff --git a/...pper-murmur3/src/main/java/org/elasticsearch/index/mapper/murmur3/Murmur3FieldMapper.java b/...pper-murmur3/src/main/java/org/elasticsearch/index/mapper/murmur3/Murmur3FieldMapper.java
@@ -111,7 +111,7 @@ public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searc
             if (format != null) {
                 throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
             }
-            return new SourceValueFetcher(name(), mapperService, false) {
+            return new SourceValueFetcher(name(), mapperService) {
                 @Override
                 protected String parseSourceValue(Object value) {
                     return value.toString();

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/AbstractGeometryFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/AbstractGeometryFieldMapper.java
@@ -259,13 +259,21 @@ public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searc
             String geoFormat = format != null ? format : GeoJsonGeometryFormat.NAME;
 
             Function<Object, Object> valueParser = value -> geometryParser.parseAndFormatObject(value, geoFormat);
-
-            return new SourceValueFetcher(name(), mapperService, parsesArrayValue) {
-                @Override
-                protected Object parseSourceValue(Object value) {
-                    return valueParser.apply(value);
-                }
-            };
+            if (parsesArrayValue) {
+                return new ArraySourceValueFetcher(name(), mapperService) {
+                    @Override
+                    protected Object parseSourceValue(Object value) {
+                        return valueParser.apply(value);
+                    }
+                };
+            } else {
+                return new SourceValueFetcher(name(), mapperService) {
+                    @Override
+                    protected Object parseSourceValue(Object value) {
+                        return valueParser.apply(value);
+                    }
+                };
+            }
         }
     }
 

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ArraySourceValueFetcher.java b/server/src/main/java/org/elasticsearch/index/mapper/ArraySourceValueFetcher.java
@@ -0,0 +1,74 @@
+/*
+ * 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.mapper;
+
+import org.elasticsearch.common.Nullable;
+import org.elasticsearch.search.lookup.SourceLookup;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Set;
+
+/**
+ * An implementation of {@link ValueFetcher} that knows how to extract values
+ * from the document source.
+ *
+ * This class differs from {@link SourceValueFetcher} in that it directly handles
+ * array values in parsing. Field types should use this class if their corresponding
+ * mapper returns true for {@link FieldMapper#parsesArrayValue()}.
+ */
+public abstract class ArraySourceValueFetcher implements ValueFetcher {
+    private final Set<String> sourcePaths;
+    private final @Nullable Object nullValue;
+
+    public ArraySourceValueFetcher(String fieldName, MapperService mapperService) {
+        this(fieldName, mapperService, null);
+    }
+
+    /**
+     * @param fieldName The name of the field.
+     * @param mapperService A mapper service.
+     * @param nullValue A optional substitute value if the _source value is 'null'.
+     */
+    public ArraySourceValueFetcher(String fieldName, MapperService mapperService, Object nullValue) {
+        this.sourcePaths = mapperService.sourcePath(fieldName);
+        this.nullValue = nullValue;
+    }
+
+    @Override
+    public List<Object> fetchValues(SourceLookup lookup) {
+        List<Object> values = new ArrayList<>();
+        for (String path : sourcePaths) {
+            Object sourceValue = lookup.extractValue(path, nullValue);
+            if (sourceValue == null) {
+                return org.elasticsearch.common.collect.List.of();
+            }
+            values.addAll((List<?>) parseSourceValue(sourceValue));
+        }
+        return values;
+    }
+
+    /**
+     * Given a value that has been extracted from a document's source, parse it into a standard
+     * format. This parsing logic should closely mirror the value parsing in
+     * {@link FieldMapper#parseCreateField} or {@link FieldMapper#parse}.
+     */
+    protected abstract Object parseSourceValue(Object value);
+}
diff --git a/server/src/main/java/org/elasticsearch/index/mapper/BinaryFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/BinaryFieldMapper.java
@@ -103,7 +103,7 @@ public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searc
             if (format != null) {
                 throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
             }
-            return new SourceValueFetcher(name(), mapperService, false) {
+            return new SourceValueFetcher(name(), mapperService) {
                 @Override
                 protected Object parseSourceValue(Object value) {
                     return value;

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/BooleanFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/BooleanFieldMapper.java
@@ -135,7 +135,7 @@ public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searc
                 throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
             }
 
-            return new SourceValueFetcher(name(), mapperService, false, nullValue) {
+            return new SourceValueFetcher(name(), mapperService, nullValue) {
                 @Override
                 protected Boolean parseSourceValue(Object value) {
                     if (value instanceof Boolean) {

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/CompletionFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/CompletionFieldMapper.java
@@ -307,7 +307,7 @@ public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searc
                 throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
             }
 
-            return new SourceValueFetcher(name(), mapperService, true) {
+            return new ArraySourceValueFetcher(name(), mapperService) {
                 @Override
                 protected List<?> parseSourceValue(Object value) {
                     if (value instanceof List) {

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java
@@ -330,7 +330,7 @@ public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searc
                 ? DateFormatter.forPattern(format).withLocale(defaultFormatter.locale())
                 : defaultFormatter;
 
-            return new SourceValueFetcher(name(), mapperService, false, nullValue) {
+            return new SourceValueFetcher(name(), mapperService, nullValue) {
                 @Override
                 public String parseSourceValue(Object value) {
                     String date = value.toString();

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/IpFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/IpFieldMapper.java
@@ -158,7 +158,7 @@ public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searc
             if (format != null) {
                 throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
             }
-            return new SourceValueFetcher(name(), mapperService, false, nullValue) {
+            return new SourceValueFetcher(name(), mapperService, nullValue) {
                 @Override
                 protected Object parseSourceValue(Object value) {
                     InetAddress address;

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/KeywordFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/KeywordFieldMapper.java
@@ -243,7 +243,7 @@ public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searc
                 throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
             }
 
-            return new SourceValueFetcher(name(), mapperService, false, nullValue) {
+            return new SourceValueFetcher(name(), mapperService, nullValue) {
                 @Override
                 protected String parseSourceValue(Object value) {
                     String keywordValue = value.toString();

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java
@@ -980,7 +980,7 @@ public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searc
                 throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
             }
 
-            return new SourceValueFetcher(name(), mapperService, false, nullValue) {
+            return new SourceValueFetcher(name(), mapperService, nullValue) {
                 @Override
                 protected Object parseSourceValue(Object value) {
                     if (value.equals("")) {

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java
@@ -205,7 +205,7 @@ public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searc
                 ? DateFormatter.forPattern(format).withLocale(defaultFormatter.locale())
                 : defaultFormatter;
 
-            return new SourceValueFetcher(name(), mapperService, false) {
+            return new SourceValueFetcher(name(), mapperService) {
 
                 @Override
                 @SuppressWarnings("unchecked")

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/SourceValueFetcher.java b/server/src/main/java/org/elasticsearch/index/mapper/SourceValueFetcher.java
@@ -32,25 +32,25 @@
  * An implementation of {@link ValueFetcher} that knows how to extract values
  * from the document source. Most standard field mappers will use this class
  * to implement value fetching.
+ *
+ * Field types that handle arrays directly should instead use {@link ArraySourceValueFetcher}.
  */
 public abstract class SourceValueFetcher implements ValueFetcher {
     private final Set<String> sourcePaths;
     private final @Nullable Object nullValue;
-    private final boolean parsesArrayValue;
 
-    public SourceValueFetcher(String fieldName, MapperService mapperService, boolean parsesArrayValue) {
-        this(fieldName, mapperService, parsesArrayValue, null);
+    public SourceValueFetcher(String fieldName, MapperService mapperService) {
+        this(fieldName, mapperService, null);
     }
 
     /**
      * @param fieldName The name of the field.
-     * @param parsesArrayValue Whether the fetcher handles array values during document parsing.
+     * @param mapperService A mapper service.
      * @param nullValue A optional substitute value if the _source value is 'null'.
      */
-    public SourceValueFetcher(String fieldName, MapperService mapperService, boolean parsesArrayValue, Object nullValue) {
+    public SourceValueFetcher(String fieldName, MapperService mapperService, Object nullValue) {
         this.sourcePaths = mapperService.sourcePath(fieldName);
         this.nullValue = nullValue;
-        this.parsesArrayValue = parsesArrayValue;
     }
 
     @Override
@@ -62,22 +62,18 @@ public List<Object> fetchValues(SourceLookup lookup) {
                 return org.elasticsearch.common.collect.List.of();
             }
 
-            if (parsesArrayValue) {
-                values.addAll((List<?>) parseSourceValue(sourceValue));
-            } else {
-                // We allow source values to contain multiple levels of arrays, such as `"field": [[1, 2]]`.
-                // So we need to unwrap these arrays before passing them on to be parsed.
-                Queue<Object> queue = new ArrayDeque<>();
-                queue.add(sourceValue);
-                while (queue.isEmpty() == false) {
-                    Object value = queue.poll();
-                    if (value instanceof List) {
-                        queue.addAll((List<?>) value);
-                    } else {
-                        Object parsedValue = parseSourceValue(value);
-                        if (parsedValue != null) {
-                            values.add(parsedValue);
-                        }
+            // We allow source values to contain multiple levels of arrays, such as `"field": [[1, 2]]`.
+            // So we need to unwrap these arrays before passing them on to be parsed.
+            Queue<Object> queue = new ArrayDeque<>();
+            queue.add(sourceValue);
+            while (queue.isEmpty() == false) {
+                Object value = queue.poll();
+                if (value instanceof List) {
+                    queue.addAll((List<?>) value);
+                } else {
+                    Object parsedValue = parseSourceValue(value);
+                    if (parsedValue != null) {
+                        values.add(parsedValue);
                     }
                 }
             }

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/TextFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/TextFieldMapper.java
@@ -645,7 +645,7 @@ public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searc
             if (format != null) {
                 throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
             }
-            return new SourceValueFetcher(name(), mapperService, false) {
+            return new SourceValueFetcher(name(), mapperService) {
                 @Override
                 protected Object parseSourceValue(Object value) {
                     return value.toString();

diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ExternalMapper.java b/server/src/test/java/org/elasticsearch/index/mapper/ExternalMapper.java
@@ -107,7 +107,7 @@ public String typeName() {
 
         @Override
         public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searchLookup, String format) {
-            return new SourceValueFetcher(name(), mapperService, false) {
+            return new SourceValueFetcher(name(), mapperService) {
                 @Override
                 protected Object parseSourceValue(Object value) {
                     return value;

diff --git a/server/src/test/java/org/elasticsearch/index/mapper/FakeStringFieldMapper.java b/server/src/test/java/org/elasticsearch/index/mapper/FakeStringFieldMapper.java
@@ -95,7 +95,7 @@ public String typeName() {
 
         @Override
         public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searchLookup, String format) {
-            return new SourceValueFetcher(name(), mapperService, false) {
+            return new SourceValueFetcher(name(), mapperService) {
                 @Override
                 protected String parseSourceValue(Object value) {
                     return value.toString();

diff --git a/...nalytics/src/main/java/org/elasticsearch/xpack/analytics/mapper/HistogramFieldMapper.java b/...nalytics/src/main/java/org/elasticsearch/xpack/analytics/mapper/HistogramFieldMapper.java
@@ -141,7 +141,7 @@ public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searc
             if (format != null) {
                 throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
             }
-            return new SourceValueFetcher(name(), mapperService, false) {
+            return new SourceValueFetcher(name(), mapperService) {
                 @Override
                 protected Object parseSourceValue(Object value) {
                     return value;

diff --git a/...attened/src/main/java/org/elasticsearch/xpack/flattened/mapper/FlatObjectFieldMapper.java b/...attened/src/main/java/org/elasticsearch/xpack/flattened/mapper/FlatObjectFieldMapper.java
@@ -479,7 +479,7 @@ public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searc
             if (format != null) {
                 throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
             }
-            return new SourceValueFetcher(name(), mapperService, false, nullValue) {
+            return new SourceValueFetcher(name(), mapperService, nullValue) {
                 @Override
                 protected Object parseSourceValue(Object value) {
                     return value;

diff --git a/...gned-long/src/main/java/org/elasticsearch/xpack/unsignedlong/UnsignedLongFieldMapper.java b/...gned-long/src/main/java/org/elasticsearch/xpack/unsignedlong/UnsignedLongFieldMapper.java
@@ -235,7 +235,7 @@ public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searc
                 throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
             }
 
-            return new SourceValueFetcher(name(), mapperService, false, nullValueFormatted) {
+            return new SourceValueFetcher(name(), mapperService, nullValueFormatted) {
                 @Override
                 protected Object parseSourceValue(Object value) {
                     if (value.equals("")) {

diff --git a/...-version/src/main/java/org/elasticsearch/xpack/versionfield/VersionStringFieldMapper.java b/...-version/src/main/java/org/elasticsearch/xpack/versionfield/VersionStringFieldMapper.java
@@ -138,7 +138,7 @@ public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searc
                 throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
             }
 
-            return new SourceValueFetcher(name(), mapperService, false, null) {
+            return new SourceValueFetcher(name(), mapperService, null) {
                 @Override
                 protected String parseSourceValue(Object value) {
                     return value.toString();