From cc2a9e01cb3ec3e5c6940bf9095532c9064adb25 Mon Sep 17 00:00:00 2001 From: Igor Motov Date: Mon, 20 Aug 2018 08:12:02 -0700 Subject: [PATCH] Make Geo Context Mapping Parsing More Strict (#32862) Currently, if geo context is represented by something other than geo_point or an object with lat and lon fields, the parsing of it as a geo context can result in ignoring the context altogether, returning confusing errors such as number_format_exception or trying to parse the number specifying as long-encoded hash code. It would also fail if the geo_point was stored. This commit makes the deprecates creation of geo contexts without correct path fields. And it fixes the issue when geo_point is stored. This is a 6.x version of #32821. --- .../migration/migrate_6_0/search.asciidoc | 8 ++- .../index/mapper/MapperService.java | 3 + .../completion/context/ContextMapping.java | 29 +++++++++ .../completion/context/ContextMappings.java | 8 ++- .../completion/context/GeoContextMapping.java | 29 ++++++++- .../ContextCompletionSuggestSearchIT.java | 20 +++++- .../completion/GeoContextMappingTests.java | 61 +++++++++++++++++++ 7 files changed, 149 insertions(+), 9 deletions(-) diff --git a/docs/reference/migration/migrate_6_0/search.asciidoc b/docs/reference/migration/migrate_6_0/search.asciidoc index 3416c940e225e..c16e981a92d20 100644 --- a/docs/reference/migration/migrate_6_0/search.asciidoc +++ b/docs/reference/migration/migrate_6_0/search.asciidoc @@ -17,7 +17,7 @@ * The `mlt` query (a synonym for the `more_like_this` query) has been removed. * The deprecated `like_text`, `ids` and `docs` parameters (all synonyms for `like`) of the `more_like_this` query have -been removed. Also the deprecated `min_word_len` (a synonym for `min_word_length`) and `max_word_len` +been removed. Also the deprecated `min_word_len` (a synonym for `min_word_length`) and `max_word_len` (a synonym for `max_word_length`) have been removed. * The `fuzzy_match` and `match_fuzzy` query (synonyma for the `match` query) have been removed @@ -215,6 +215,11 @@ The ability to query and index context enabled suggestions without contexts has been deprecated. Context enabled suggestion queries without contexts have to visit every suggestion, which degrades the search performance considerably. +For geo context the value of the `path` parameter is now validated against the mapping, +and if `path` points to a non `geo_point` field or the field doesn't exist a deprecation +warning will be issued. In 7.0 it will be required for the `path` to point to a correct +`geo_point` field. + ==== Limiting the max number of expansion of span_multi queries `span_multi` queries will hit too many clauses failure if the number of terms that match the @@ -223,4 +228,3 @@ can set the <> of the multi term q rewrite. Or, if you use `span_multi` on `prefix` query only, you can activate the <> field option of the `text` field instead. This will rewrite any prefix query on the field to a a single term query that matches the indexed prefix. - diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index e0b754e7fa307..b0a0493ab6f63 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -53,6 +53,7 @@ import org.elasticsearch.indices.InvalidTypeNameException; import org.elasticsearch.indices.TypeMissingException; import org.elasticsearch.indices.mapper.MapperRegistry; +import org.elasticsearch.search.suggest.completion.context.ContextMapping; import java.io.Closeable; import java.io.IOException; @@ -454,6 +455,8 @@ private synchronized Map internalMerge(@Nullable Documen MapperMergeValidator.validateFieldReferences(indexSettings, fieldMappers, fieldAliasMappers, fullPathObjectMappers, fieldTypes); + ContextMapping.validateContextPaths(indexSettings.getIndexVersionCreated(), fieldMappers, fieldTypes::get); + if (reason == MergeReason.MAPPING_UPDATE) { // this check will only be performed on the master node when there is // a call to the update mapping API. For all other cases like diff --git a/server/src/main/java/org/elasticsearch/search/suggest/completion/context/ContextMapping.java b/server/src/main/java/org/elasticsearch/search/suggest/completion/context/ContextMapping.java index 7eab4e072f146..af7300bc79fbe 100644 --- a/server/src/main/java/org/elasticsearch/search/suggest/completion/context/ContextMapping.java +++ b/server/src/main/java/org/elasticsearch/search/suggest/completion/context/ContextMapping.java @@ -20,6 +20,7 @@ package org.elasticsearch.search.suggest.completion.context; import org.elasticsearch.ElasticsearchParseException; +import org.elasticsearch.Version; import org.elasticsearch.common.Strings; import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.common.xcontent.ToXContentFragment; @@ -28,6 +29,8 @@ import org.elasticsearch.common.xcontent.XContentParser.Token; import org.elasticsearch.common.xcontent.json.JsonXContent; import org.elasticsearch.index.mapper.CompletionFieldMapper; +import org.elasticsearch.index.mapper.FieldMapper; +import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.mapper.ParseContext; import java.io.IOException; @@ -35,6 +38,7 @@ import java.util.List; import java.util.Objects; import java.util.Set; +import java.util.function.Function; /** * A {@link ContextMapping} defines criteria that can be used to @@ -131,6 +135,31 @@ public final List parseQueryContext(XContentParser parser) */ protected abstract XContentBuilder toInnerXContent(XContentBuilder builder, Params params) throws IOException; + /** + * Checks if the current context is consistent with the rest of the fields. For example, the GeoContext + * should check that the field that it points to has the correct type. + */ + protected void validateReferences(Version indexVersionCreated, Function fieldResolver) { + // No validation is required by default + } + + /** + * Verifies that all field paths specified in contexts point to the fields with correct mappings + */ + public static void validateContextPaths(Version indexVersionCreated, List fieldMappers, + Function fieldResolver) { + for (FieldMapper fieldMapper : fieldMappers) { + if (CompletionFieldMapper.CONTENT_TYPE.equals(fieldMapper.typeName())) { + CompletionFieldMapper.CompletionFieldType fieldType = ((CompletionFieldMapper) fieldMapper).fieldType(); + if (fieldType.hasContextMappings()) { + for (ContextMapping context : fieldType.getContextMappings()) { + context.validateReferences(indexVersionCreated, fieldResolver); + } + } + } + } + } + @Override public final XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.field(FIELD_NAME, name); diff --git a/server/src/main/java/org/elasticsearch/search/suggest/completion/context/ContextMappings.java b/server/src/main/java/org/elasticsearch/search/suggest/completion/context/ContextMappings.java index 4d6b53296f157..3f10dfab9262f 100644 --- a/server/src/main/java/org/elasticsearch/search/suggest/completion/context/ContextMappings.java +++ b/server/src/main/java/org/elasticsearch/search/suggest/completion/context/ContextMappings.java @@ -39,6 +39,7 @@ import java.util.Collections; import java.util.HashMap; import java.util.HashSet; +import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Objects; @@ -52,7 +53,7 @@ * and creates context queries for defined {@link ContextMapping}s * for a {@link CompletionFieldMapper} */ -public class ContextMappings implements ToXContent { +public class ContextMappings implements ToXContent, Iterable { private static final DeprecationLogger DEPRECATION_LOGGER = new DeprecationLogger(Loggers.getLogger(ContextMappings.class)); @@ -102,6 +103,11 @@ public void addField(ParseContext.Document document, String name, String input, document.add(new TypedContextField(name, input, weight, contexts, document)); } + @Override + public Iterator iterator() { + return contextMappings.iterator(); + } + /** * Field prepends context values with a suggestion * Context values are associated with a type, denoted by diff --git a/server/src/main/java/org/elasticsearch/search/suggest/completion/context/GeoContextMapping.java b/server/src/main/java/org/elasticsearch/search/suggest/completion/context/GeoContextMapping.java index 48aaf705099da..1a10e3238ec96 100644 --- a/server/src/main/java/org/elasticsearch/search/suggest/completion/context/GeoContextMapping.java +++ b/server/src/main/java/org/elasticsearch/search/suggest/completion/context/GeoContextMapping.java @@ -19,12 +19,17 @@ package org.elasticsearch.search.suggest.completion.context; +import org.apache.logging.log4j.LogManager; +import org.apache.lucene.document.LatLonDocValuesField; +import org.apache.lucene.document.LatLonPoint; import org.apache.lucene.document.StringField; import org.apache.lucene.index.DocValuesType; import org.apache.lucene.index.IndexableField; import org.elasticsearch.ElasticsearchParseException; +import org.elasticsearch.Version; import org.elasticsearch.common.geo.GeoPoint; import org.elasticsearch.common.geo.GeoUtils; +import org.elasticsearch.common.logging.DeprecationLogger; import org.elasticsearch.common.unit.DistanceUnit; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; @@ -42,6 +47,7 @@ import java.util.Map; import java.util.Objects; import java.util.Set; +import java.util.function.Function; import java.util.stream.Collectors; import static org.elasticsearch.common.geo.GeoHashUtils.addNeighbors; @@ -69,6 +75,8 @@ public class GeoContextMapping extends ContextMapping { static final String CONTEXT_PRECISION = "precision"; static final String CONTEXT_NEIGHBOURS = "neighbours"; + private static final DeprecationLogger DEPRECATION_LOGGER = new DeprecationLogger(LogManager.getLogger(GeoContextMapping.class)); + private final int precision; private final String fieldName; @@ -205,11 +213,11 @@ public Set parseContext(Document document) { for (IndexableField field : fields) { if (field instanceof StringField) { spare.resetFromString(field.stringValue()); - } else { - // todo return this to .stringValue() once LatLonPoint implements it + geohashes.add(spare.geohash()); + } else if (field instanceof LatLonPoint || field instanceof LatLonDocValuesField) { spare.resetFromIndexableField(field); + geohashes.add(spare.geohash()); } - geohashes.add(spare.geohash()); } } } @@ -279,6 +287,21 @@ public List toInternalQueryContexts(List return internalQueryContextList; } + @Override + protected void validateReferences(Version indexVersionCreated, Function fieldResolver) { + if (fieldName != null) { + MappedFieldType mappedFieldType = fieldResolver.apply(fieldName); + if (mappedFieldType == null) { + DEPRECATION_LOGGER.deprecatedAndMaybeLog("geo_context_mapping", + "field [{}] referenced in context [{}] is not defined in the mapping", fieldName, name); + } else if (GeoPointFieldMapper.CONTENT_TYPE.equals(mappedFieldType.typeName()) == false) { + DEPRECATION_LOGGER.deprecatedAndMaybeLog("geo_context_mapping", + "field [{}] referenced in context [{}] must be mapped to geo_point, found [{}]", + fieldName, name, mappedFieldType.typeName()); + } + } + } + @Override public boolean equals(Object o) { if (this == o) return true; diff --git a/server/src/test/java/org/elasticsearch/search/suggest/ContextCompletionSuggestSearchIT.java b/server/src/test/java/org/elasticsearch/search/suggest/ContextCompletionSuggestSearchIT.java index 89f1b2ae28691..fcb4de5e04a8b 100644 --- a/server/src/test/java/org/elasticsearch/search/suggest/ContextCompletionSuggestSearchIT.java +++ b/server/src/test/java/org/elasticsearch/search/suggest/ContextCompletionSuggestSearchIT.java @@ -581,15 +581,24 @@ public void testGeoNeighbours() throws Exception { } public void testGeoField() throws Exception { -// Version version = VersionUtils.randomVersionBetween(random(), Version.V_2_0_0, Version.V_5_0_0_alpha5); -// Settings settings = Settings.builder().put(IndexMetaData.SETTING_VERSION_CREATED, version).build(); XContentBuilder mapping = jsonBuilder(); mapping.startObject(); mapping.startObject(TYPE); mapping.startObject("properties"); + mapping.startObject("location"); + mapping.startObject("properties"); mapping.startObject("pin"); mapping.field("type", "geo_point"); + // Enable store and disable indexing sometimes + if (randomBoolean()) { + mapping.field("store", "true"); + } + if (randomBoolean()) { + mapping.field("index", "false"); + } + mapping.endObject(); // pin mapping.endObject(); + mapping.endObject(); // location mapping.startObject(FIELD); mapping.field("type", "completion"); mapping.field("analyzer", "simple"); @@ -598,7 +607,7 @@ public void testGeoField() throws Exception { mapping.startObject(); mapping.field("name", "st"); mapping.field("type", "geo"); - mapping.field("path", "pin"); + mapping.field("path", "location.pin"); mapping.field("precision", 5); mapping.endObject(); mapping.endArray(); @@ -612,7 +621,9 @@ public void testGeoField() throws Exception { XContentBuilder source1 = jsonBuilder() .startObject() + .startObject("location") .latlon("pin", 52.529172, 13.407333) + .endObject() .startObject(FIELD) .array("input", "Hotel Amsterdam in Berlin") .endObject() @@ -621,7 +632,9 @@ public void testGeoField() throws Exception { XContentBuilder source2 = jsonBuilder() .startObject() + .startObject("location") .latlon("pin", 52.363389, 4.888695) + .endObject() .startObject(FIELD) .array("input", "Hotel Berlin in Amsterdam") .endObject() @@ -693,6 +706,7 @@ public void assertSuggestions(String suggestionName, SuggestionBuilder suggestBu private void createIndexAndMapping(CompletionMappingBuilder completionMappingBuilder) throws IOException { createIndexAndMappingAndSettings(Settings.EMPTY, completionMappingBuilder); } + private void createIndexAndMappingAndSettings(Settings settings, CompletionMappingBuilder completionMappingBuilder) throws IOException { XContentBuilder mapping = jsonBuilder().startObject() .startObject(TYPE).startObject("properties") diff --git a/server/src/test/java/org/elasticsearch/search/suggest/completion/GeoContextMappingTests.java b/server/src/test/java/org/elasticsearch/search/suggest/completion/GeoContextMappingTests.java index 260d919fd42ca..fa6dc59241dd0 100644 --- a/server/src/test/java/org/elasticsearch/search/suggest/completion/GeoContextMappingTests.java +++ b/server/src/test/java/org/elasticsearch/search/suggest/completion/GeoContextMappingTests.java @@ -203,6 +203,67 @@ public void testIndexingWithMultipleContexts() throws Exception { assertContextSuggestFields(fields, 3); } + public void testMalformedGeoField() throws Exception { + XContentBuilder mapping = jsonBuilder(); + mapping.startObject(); + mapping.startObject("type1"); + mapping.startObject("properties"); + String type = randomFrom("text", "keyword", "long", null); + if (type != null) { + mapping.startObject("pin"); + mapping.field("type", type); + mapping.endObject(); + } + mapping.startObject("suggestion"); + mapping.field("type", "completion"); + mapping.field("analyzer", "simple"); + + mapping.startArray("contexts"); + mapping.startObject(); + mapping.field("name", "st"); + mapping.field("type", "geo"); + mapping.field("path", "pin"); + mapping.field("precision", 5); + mapping.endObject(); + mapping.endArray(); + + mapping.endObject(); + + mapping.endObject(); + mapping.endObject(); + mapping.endObject(); + + MapperService mapperService = createIndex("test", Settings.EMPTY, "type1", mapping).mapperService(); + XContentBuilder builder = jsonBuilder().startObject(); + if (type != null) { + switch (type) { + case "keyword": + case "text": + builder.field("pin", "52.529172, 13.407333"); + break; + case "long": + builder.field("pin", 1234); + break; + case "object": + builder.latlon("pin", 52.529172, 13.407333); + break; + } + } else { + builder.field("pin", "52.529172, 13.407333"); + } + + builder.startObject("suggestion") + .array("input", "Hotel Amsterdam in Berlin") + .endObject() + .endObject(); + + ParsedDocument parsedDocument = mapperService.documentMapper("type1").parse( + SourceToParse.source("test", "type1", "1", BytesReference.bytes(builder), XContentType.JSON)); + IndexableField[] fields = parsedDocument.rootDoc().getFields("suggestion"); + // Make sure that in 6.x all these cases are still parsed + assertContextSuggestFields(fields, 1); + } + public void testParsingQueryContextBasic() throws Exception { XContentBuilder builder = jsonBuilder().value("ezs42e44yx96"); XContentParser parser = createParser(JsonXContent.jsonXContent, BytesReference.bytes(builder));