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.
This commit is contained in:
Julie Tibshirani 2020-10-06 17:49:25 -07:00 committed by GitHub
parent 2b838d1ea6
commit f17ca18dfa
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
28 changed files with 140 additions and 54 deletions

View File

@ -118,7 +118,7 @@ public class RankFeatureFieldMapper extends ParametrizedFieldMapper {
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);

View File

@ -93,7 +93,7 @@ public class RankFeaturesFieldMapper extends ParametrizedFieldMapper {
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;

View File

@ -225,7 +225,7 @@ public class ScaledFloatFieldMapper extends ParametrizedFieldMapper {
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;

View File

@ -227,7 +227,7 @@ public final class ParentJoinFieldMapper extends FieldMapper {
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;

View File

@ -229,7 +229,7 @@ public class PercolatorFieldMapper extends ParametrizedFieldMapper {
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;

View File

@ -104,7 +104,7 @@ public class ICUCollationKeywordFieldMapper extends FieldMapper {
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();

View File

@ -111,7 +111,7 @@ public class Murmur3FieldMapper extends ParametrizedFieldMapper {
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();

View File

@ -259,13 +259,21 @@ public abstract class AbstractGeometryFieldMapper<Parsed, Processed> extends Fie
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);
}
};
}
}
}

View File

@ -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);
}

View File

@ -103,7 +103,7 @@ public class BinaryFieldMapper extends ParametrizedFieldMapper {
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;

View File

@ -135,7 +135,7 @@ public class BooleanFieldMapper extends ParametrizedFieldMapper {
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) {

View File

@ -307,7 +307,7 @@ public class CompletionFieldMapper extends ParametrizedFieldMapper {
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) {

View File

@ -330,7 +330,7 @@ public final class DateFieldMapper extends ParametrizedFieldMapper {
? 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();

View File

@ -158,7 +158,7 @@ public class IpFieldMapper extends ParametrizedFieldMapper {
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;

View File

@ -243,7 +243,7 @@ public final class KeywordFieldMapper extends ParametrizedFieldMapper {
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();

View File

@ -980,7 +980,7 @@ public class NumberFieldMapper extends ParametrizedFieldMapper {
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("")) {

View File

@ -205,7 +205,7 @@ public class RangeFieldMapper extends ParametrizedFieldMapper {
? DateFormatter.forPattern(format).withLocale(defaultFormatter.locale())
: defaultFormatter;
return new SourceValueFetcher(name(), mapperService, false) {
return new SourceValueFetcher(name(), mapperService) {
@Override
@SuppressWarnings("unchecked")

View File

@ -32,25 +32,25 @@ import java.util.Set;
* 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 abstract class SourceValueFetcher implements ValueFetcher {
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);
}
}
}

View File

@ -645,7 +645,7 @@ public class TextFieldMapper extends FieldMapper {
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();

View File

@ -107,7 +107,7 @@ public class ExternalMapper extends ParametrizedFieldMapper {
@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;

View File

@ -95,7 +95,7 @@ public class FakeStringFieldMapper extends FieldMapper {
@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();

View File

@ -141,7 +141,7 @@ public class HistogramFieldMapper extends ParametrizedFieldMapper {
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;

View File

@ -479,7 +479,7 @@ public final class FlatObjectFieldMapper extends DynamicKeyFieldMapper {
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;

View File

@ -235,7 +235,7 @@ public class UnsignedLongFieldMapper extends ParametrizedFieldMapper {
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("")) {

View File

@ -138,7 +138,7 @@ public class VersionStringFieldMapper extends ParametrizedFieldMapper {
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();

View File

@ -15,13 +15,13 @@ import org.elasticsearch.Version;
import org.elasticsearch.common.xcontent.XContentParser.Token;
import org.elasticsearch.common.xcontent.support.XContentMapValues;
import org.elasticsearch.index.fielddata.IndexFieldData;
import org.elasticsearch.index.mapper.ArraySourceValueFetcher;
import org.elasticsearch.index.mapper.FieldMapper;
import org.elasticsearch.index.mapper.MappedFieldType;
import org.elasticsearch.index.mapper.MapperParsingException;
import org.elasticsearch.index.mapper.MapperService;
import org.elasticsearch.index.mapper.ParametrizedFieldMapper;
import org.elasticsearch.index.mapper.ParseContext;
import org.elasticsearch.index.mapper.SourceValueFetcher;
import org.elasticsearch.index.mapper.TextSearchInfo;
import org.elasticsearch.index.mapper.ValueFetcher;
import org.elasticsearch.index.query.QueryShardContext;
@ -116,7 +116,7 @@ public class DenseVectorFieldMapper extends ParametrizedFieldMapper {
if (format != null) {
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 Object parseSourceValue(Object value) {
return value;

View File

@ -8,7 +8,9 @@ package org.elasticsearch.xpack.vectors.mapper;
import org.elasticsearch.index.mapper.FieldTypeTestCase;
import java.io.IOException;
import java.util.Collections;
import java.util.List;
public class DenseVectorFieldTypeTests extends FieldTypeTestCase {
@ -33,4 +35,10 @@ public class DenseVectorFieldTypeTests extends FieldTypeTestCase {
DenseVectorFieldMapper.DenseVectorFieldType ft = new DenseVectorFieldMapper.DenseVectorFieldType("f", 1, Collections.emptyMap());
expectThrows(UnsupportedOperationException.class, () -> ft.docValueFormat(null, null));
}
public void testFetchSourceValue() throws IOException {
DenseVectorFieldMapper.DenseVectorFieldType ft = new DenseVectorFieldMapper.DenseVectorFieldType("f", 5, Collections.emptyMap());
List<Double> vector = org.elasticsearch.common.collect.List.of(0.0, 1.0, 2.0, 3.0, 4.0);
assertEquals(vector, fetchSourceValue(ft, vector));
}
}

View File

@ -982,7 +982,7 @@ public class WildcardFieldMapper extends FieldMapper {
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();