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.

Author: imotov

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 <<query-dsl-multi-term-rewrite, rewrite method>> of the multi term q
 rewrite. Or, if you use `span_multi` on `prefix` query only, you can activate the
 <<index-prefix-config,`index_prefixes`>> 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.
-

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<String, DocumentMapper> 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

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,13 +29,16 @@
 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;
 import java.util.ArrayList;
 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<InternalQueryContext> 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<String, MappedFieldType> 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<FieldMapper> fieldMappers,
+                                            Function<String, MappedFieldType> 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);

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<ContextMapping> {
 
     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<ContextMapping> iterator() {
+        return contextMappings.iterator();
+    }
+
     /**
      * Field prepends context values with a suggestion
      * Context values are associated with a type, denoted by

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<GeoQueryContext> {
     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<CharSequence> 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<InternalQueryContext> toInternalQueryContexts(List<GeoQueryContext>
         return internalQueryContextList;
     }
 
+    @Override
+    protected void validateReferences(Version indexVersionCreated, Function<String, MappedFieldType> 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;

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")

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