Browse Source

Enhance error message for copy-to (#72820)

We currently don't support `copy_to` for fields that take the form of objects
(e.g. `date_range` or certain kinds of `geo_point` variants). The current
problem with objects is that when DocumentParser parses anything other than
single values, it potentially advances the underlying parser past the value that
we would need to stay on for parsing the value again. While we might want to
support this in the future, for now this PR enhances the otherwise confusing
MapperParsingException with something more helpful and adds a short note in the
documentation about this restriction.

Closes #49344
Christoph Büscher 4 years ago
parent
commit
f34c9a8a40

+ 3 - 1
docs/reference/mapping/params/copy-to.asciidoc

@@ -67,4 +67,6 @@ Some important points:
 * You cannot copy recursively via intermediary fields such as a `copy_to` on 
 `field_1` to `field_2` and `copy_to` on `field_2` to `field_3` expecting 
 indexing into `field_1` will eventuate in `field_3`, instead use copy_to 
-directly to multiple fields from the originating field. 
+directly to multiple fields from the originating field. 
+
+NOTE: `copy-to` is _not_ supported for field types where values take the form of objects, e.g. `date_range`

+ 27 - 21
server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java

@@ -535,7 +535,16 @@ public final class DocumentParser {
         } else if (mapper instanceof FieldMapper) {
             FieldMapper fieldMapper = (FieldMapper) mapper;
             fieldMapper.parse(context);
-            parseCopyFields(context, fieldMapper.copyTo().copyToFields());
+            List<String> copyToFields = fieldMapper.copyTo().copyToFields();
+            if (context.isWithinCopyTo() == false && copyToFields.isEmpty() == false) {
+                XContentParser.Token currentToken = context.parser().currentToken();
+                if (currentToken.isValue() == false) {
+                    // sanity check, we currently support copy-to only for value-type field, not objects
+                    throw new MapperParsingException("Cannot copy field [" + mapper.name() + "] to fields " + copyToFields +
+                        ". Copy-to currently only works for value-type fields, not objects.");
+                }
+                parseCopyFields(context, copyToFields);
+            }
         } else if (mapper instanceof FieldAliasMapper) {
             throw new IllegalArgumentException("Cannot write to a field alias [" + mapper.name() + "].");
         } else {
@@ -547,7 +556,6 @@ public final class DocumentParser {
     private static void parseObject(final ParseContext context, ObjectMapper mapper, String currentFieldName,
                                     String[] paths) throws IOException {
         assert currentFieldName != null;
-
         Mapper objectMapper = getMapper(context, mapper, currentFieldName, paths);
         if (objectMapper != null) {
             context.path().add(currentFieldName);
@@ -696,27 +704,25 @@ public final class DocumentParser {
 
     /** Creates instances of the fields that the current field should be copied to */
     private static void parseCopyFields(ParseContext context, List<String> copyToFields) throws IOException {
-        if (context.isWithinCopyTo() == false && copyToFields.isEmpty() == false) {
-            context = context.createCopyToContext();
-            for (String field : copyToFields) {
-                // In case of a hierarchy of nested documents, we need to figure out
-                // which document the field should go to
-                ParseContext.Document targetDoc = null;
-                for (ParseContext.Document doc = context.doc(); doc != null; doc = doc.getParent()) {
-                    if (field.startsWith(doc.getPrefix())) {
-                        targetDoc = doc;
-                        break;
-                    }
-                }
-                assert targetDoc != null;
-                final ParseContext copyToContext;
-                if (targetDoc == context.doc()) {
-                    copyToContext = context;
-                } else {
-                    copyToContext = context.switchDoc(targetDoc);
+        context = context.createCopyToContext();
+        for (String field : copyToFields) {
+            // In case of a hierarchy of nested documents, we need to figure out
+            // which document the field should go to
+            ParseContext.Document targetDoc = null;
+            for (ParseContext.Document doc = context.doc(); doc != null; doc = doc.getParent()) {
+                if (field.startsWith(doc.getPrefix())) {
+                    targetDoc = doc;
+                    break;
                 }
-                parseCopy(field, copyToContext);
             }
+            assert targetDoc != null;
+            final ParseContext copyToContext;
+            if (targetDoc == context.doc()) {
+                copyToContext = context;
+            } else {
+                copyToContext = context.switchDoc(targetDoc);
+            }
+            parseCopy(field, copyToContext);
         }
     }
 

+ 90 - 0
server/src/test/java/org/elasticsearch/index/mapper/CopyToMapperTests.java

@@ -13,6 +13,7 @@ import org.elasticsearch.common.bytes.BytesReference;
 import org.elasticsearch.common.xcontent.ToXContent;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentParser;
+import org.elasticsearch.common.xcontent.XContentType;
 import org.elasticsearch.common.xcontent.json.JsonXContent;
 import org.elasticsearch.index.mapper.ParseContext.Document;
 import org.hamcrest.Matchers;
@@ -624,4 +625,93 @@ public class CopyToMapperTests extends MapperServiceTestCase {
         assertThat(e.getMessage(),
             Matchers.containsString("[copy_to] may not be used to copy from a multi-field: [field.bar]"));
     }
+
+    public void testCopyToDateRangeFailure() throws Exception {
+        DocumentMapper docMapper = createDocumentMapper(topMapping(b -> {
+                b.startObject("properties");
+                {
+                    b.startObject("date_copy")
+                        .field("type", "date_range")
+                    .endObject()
+                    .startObject("date")
+                        .field("type", "date_range")
+                        .array("copy_to", "date_copy")
+                    .endObject();
+                }
+                b.endObject();
+              }));
+
+        BytesReference json = BytesReference.bytes(jsonBuilder().startObject()
+                .startObject("date")
+                    .field("gte", "2019-11-10T01:00:00.000Z")
+                    .field("lt", "2019-11-11T01:00:00.000Z")
+                .endObject()
+                .endObject());
+
+        MapperParsingException ex = expectThrows(MapperParsingException.class, () -> docMapper.parse(new SourceToParse("test", "1", json,
+            XContentType.JSON)).rootDoc());
+        assertEquals(
+            "Cannot copy field [date] to fields [date_copy]. Copy-to currently only works for value-type fields, not objects.",
+            ex.getMessage()
+        );
+    }
+
+    public void testCopyToGeoPoint() throws Exception {
+        DocumentMapper docMapper = createDocumentMapper(topMapping(b -> {
+                b.startObject("properties");
+                {
+                    b.startObject("geopoint_copy")
+                        .field("type", "geo_point")
+                    .endObject()
+                    .startObject("geopoint")
+                        .field("type", "geo_point")
+                        .array("copy_to", "geopoint_copy")
+                    .endObject();
+                }
+                b.endObject();
+              }));
+        // copy-to works for value-type representations
+        {
+            for (String value : List.of("41.12,-71.34", "drm3btev3e86", "POINT (-71.34 41.12)")) {
+                BytesReference json = BytesReference.bytes(jsonBuilder().startObject().field("geopoint", value).endObject());
+
+                ParseContext.Document doc = docMapper.parse(new SourceToParse("test", "1", json, XContentType.JSON)).rootDoc();
+
+                IndexableField[] fields = doc.getFields("geopoint");
+                assertThat(fields.length, equalTo(2));
+
+                fields = doc.getFields("geopoint_copy");
+                assertThat(fields.length, equalTo(2));
+            }
+        }
+        // check failure for object/array type representations
+        {
+            BytesReference json = BytesReference.bytes(
+                jsonBuilder().startObject().startObject("geopoint").field("lat", 41.12).field("lon", -71.34).endObject().endObject()
+            );
+
+            MapperParsingException ex = expectThrows(
+                MapperParsingException.class,
+                () -> docMapper.parse(new SourceToParse("test", "1", json, XContentType.JSON)).rootDoc()
+            );
+            assertEquals(
+                "Cannot copy field [geopoint] to fields [geopoint_copy]. Copy-to currently only works for value-type fields, not objects.",
+                ex.getMessage()
+            );
+        }
+        {
+            BytesReference json = BytesReference.bytes(
+                jsonBuilder().startObject().array("geopoint", new double[] { -71.34, 41.12 }).endObject()
+            );
+
+            MapperParsingException ex = expectThrows(
+                MapperParsingException.class,
+                () -> docMapper.parse(new SourceToParse("test", "1", json, XContentType.JSON)).rootDoc()
+            );
+            assertEquals(
+                "Cannot copy field [geopoint] to fields [geopoint_copy]. Copy-to currently only works for value-type fields, not objects.",
+                ex.getMessage()
+            );
+        }
+    }
 }