From b503bd3a56ca2f69941449d83deeb03938a5d694 Mon Sep 17 00:00:00 2001 From: Adrien Grand Date: Thu, 28 Jun 2018 13:48:29 +0200 Subject: [PATCH] Change the way that fields are selected for inclusion in `all` queries. Currently this is a hardcoded whitelist, which is problematic due to the fact that plugins can't add new types to this list, we already have the problem with the `scaled_float` type today. This change proposes to rely on exception types of the `termQuery` factory method instead to know whether a field should be searched. Another option would be to add an API to MappedFieldType to check whether a field must be considered for `all` queries, but I wanted to avoid adding a new API. --- .../index/mapper/MappedFieldType.java | 9 ++- .../index/search/QueryParserHelper.java | 71 +++++-------------- 2 files changed, 24 insertions(+), 56 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java b/server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java index 5f3f4a4de49d6..4a3fa852e7f7d 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java @@ -35,6 +35,7 @@ import org.apache.lucene.search.TermInSetQuery; import org.apache.lucene.search.TermQuery; import org.apache.lucene.util.BytesRef; +import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.geo.ShapeRelation; import org.elasticsearch.common.joda.DateMathParser; @@ -314,7 +315,13 @@ public boolean isAggregatable() { /** Generates a query that will only match documents that contain the given value. * The default implementation returns a {@link TermQuery} over the value bytes, * boosted by {@link #boost()}. - * @throws IllegalArgumentException if {@code value} cannot be converted to the expected data type */ + * @throws IllegalArgumentException if {@code value} cannot be converted to the expected data type or if the field is not searchable + * due to the way it is configured (eg. not indexed) + * @throws ElasticsearchParseException if {@code value} cannot be converted to the expected data type + * @throws UnsupportedOperationException if the field is not searchable regardless of options + * @throws QueryShardException if the field is not searchable regardless of options + */ + // TODO: Standardize exception types public abstract Query termQuery(Object value, @Nullable QueryShardContext context); /** Build a constant-scoring query that matches all values. The default implementation uses a diff --git a/server/src/main/java/org/elasticsearch/index/search/QueryParserHelper.java b/server/src/main/java/org/elasticsearch/index/search/QueryParserHelper.java index b3751afbc9c5d..c26e5a218a839 100644 --- a/server/src/main/java/org/elasticsearch/index/search/QueryParserHelper.java +++ b/server/src/main/java/org/elasticsearch/index/search/QueryParserHelper.java @@ -20,43 +20,19 @@ package org.elasticsearch.index.search; import org.elasticsearch.common.regex.Regex; -import org.elasticsearch.index.mapper.DateFieldMapper; -import org.elasticsearch.index.mapper.DocumentMapper; -import org.elasticsearch.index.mapper.FieldMapper; -import org.elasticsearch.index.mapper.IpFieldMapper; -import org.elasticsearch.index.mapper.KeywordFieldMapper; -import org.elasticsearch.index.mapper.MapperService; -import org.elasticsearch.index.mapper.MetadataFieldMapper; -import org.elasticsearch.index.mapper.NumberFieldMapper; -import org.elasticsearch.index.mapper.TextFieldMapper; +import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.query.QueryShardContext; +import org.elasticsearch.index.query.QueryShardException; import java.util.Collection; import java.util.HashMap; -import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.Set; /** * Helpers to extract and expand field names and boosts */ public final class QueryParserHelper { - // Mapping types the "all-ish" query can be executed against - // TODO: Fix the API so that we don't need a hardcoded list of types - private static final Set ALLOWED_QUERY_MAPPER_TYPES; - - static { - ALLOWED_QUERY_MAPPER_TYPES = new HashSet<>(); - ALLOWED_QUERY_MAPPER_TYPES.add(DateFieldMapper.CONTENT_TYPE); - ALLOWED_QUERY_MAPPER_TYPES.add(IpFieldMapper.CONTENT_TYPE); - ALLOWED_QUERY_MAPPER_TYPES.add(KeywordFieldMapper.CONTENT_TYPE); - for (NumberFieldMapper.NumberType nt : NumberFieldMapper.NumberType.values()) { - ALLOWED_QUERY_MAPPER_TYPES.add(nt.typeName()); - } - ALLOWED_QUERY_MAPPER_TYPES.add("scaled_float"); - ALLOWED_QUERY_MAPPER_TYPES.add(TextFieldMapper.CONTENT_TYPE); - } private QueryParserHelper() {} @@ -83,22 +59,6 @@ public static Map parseFieldsAndWeights(List fields) { return fieldsAndWeights; } - /** - * Get a {@link FieldMapper} associated with a field name or null. - * @param mapperService The mapper service where to find the mapping. - * @param field The field name to search. - */ - public static FieldMapper getFieldMapper(MapperService mapperService, String field) { - DocumentMapper mapper = mapperService.documentMapper(); - if (mapper != null) { - FieldMapper fieldMapper = mapper.mappers().getMapper(field); - if (fieldMapper != null) { - return fieldMapper; - } - } - return null; - } - public static Map resolveMappingFields(QueryShardContext context, Map fieldsAndWeights) { return resolveMappingFields(context, fieldsAndWeights, null); @@ -136,8 +96,7 @@ public static Map resolveMappingFields(QueryShardContext context, * @param fieldOrPattern The field name or the pattern to resolve * @param weight The weight for the field * @param acceptAllTypes Whether all field type should be added when a pattern is expanded. - * If false, only {@link #ALLOWED_QUERY_MAPPER_TYPES} are accepted and other field types - * are discarded from the query. + * If false, only searchable field types are added. * @param acceptMetadataField Whether metadata fields should be added when a pattern is expanded. */ public static Map resolveMappingField(QueryShardContext context, String fieldOrPattern, float weight, @@ -152,8 +111,7 @@ public static Map resolveMappingField(QueryShardContext context, * @param fieldOrPattern The field name or the pattern to resolve * @param weight The weight for the field * @param acceptAllTypes Whether all field type should be added when a pattern is expanded. - * If false, only {@link #ALLOWED_QUERY_MAPPER_TYPES} are accepted and other field types - * are discarded from the query. + * If false, only searchable field types are added. * @param acceptMetadataField Whether metadata fields should be added when a pattern is expanded. * @param fieldSuffix The suffix name to add to the expanded field names if a mapping exists for that name. * The original name of the field is kept if adding the suffix to the field name does not point to a valid field @@ -167,22 +125,25 @@ public static Map resolveMappingField(QueryShardContext context, if (fieldSuffix != null && context.fieldMapper(fieldName + fieldSuffix) != null) { fieldName = fieldName + fieldSuffix; } - FieldMapper mapper = getFieldMapper(context.getMapperService(), fieldName); - if (mapper == null) { + MappedFieldType ft = context.fieldMapper(fieldName); + if (ft == null) { // Unmapped fields are not ignored fields.put(fieldOrPattern, weight); continue; } - if (acceptMetadataField == false && mapper instanceof MetadataFieldMapper) { + if (acceptMetadataField == false && ft.name().startsWith("_")) { // Ignore metadata fields continue; } - // Ignore fields that are not in the allowed mapper types. Some - // types do not support term queries, and thus we cannot generate - // a special query for them. - String mappingType = mapper.fieldType().typeName(); - if (acceptAllTypes == false && ALLOWED_QUERY_MAPPER_TYPES.contains(mappingType) == false) { - continue; + if (acceptAllTypes == false) { + try { + ft.termQuery("", context); + } catch (QueryShardException|UnsupportedOperationException e) { + // field type is never searchable with term queries (eg. geo point): ignore + continue; + } catch (RuntimeException e) { + // other exceptions are parsing errors or not indexed fields: keep + } } fields.put(fieldName, weight); }