Correctly identify parent of copy_to destination field for synthetic source purposes

Author: lkts

rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.create/20_synthetic_source.yml

@@ -129,7 +129,7 @@ public int get() {
      * that copy_to field in introduced using a dynamic template
      * in this document and therefore is not present in mapping yet.
      */
-    private final Set<String> copyToFields;
+    public final Set<String> copyToFields;
 
     // Indicates if the source for this context has been marked to be recorded. Applies to synthetic source only.
     private boolean recordedSource;
@@ -453,20 +453,6 @@ public boolean isFieldAppliedFromTemplate(String name) {
 
     public void markFieldAsCopyTo(String fieldName) {
         copyToFields.add(fieldName);
-        if (mappingLookup.isSourceSynthetic() && indexSettings().getSkipIgnoredSourceWrite() == false) {
-            /*
-            Mark this field as containing copied data meaning it should not be present
-            in synthetic _source (to be consistent with stored _source).
-            Ignored source values take precedence over standard synthetic source implementation
-            so by adding this nothing entry we "disable" field in synthetic source.
-            Otherwise, it would be constructed f.e. from doc_values which leads to duplicate values
-            in copied field after reindexing.
-
-            Note that this applies to fields that are copied from fields using ignored source themselves
-            and therefore we don't check for canAddIgnoredField().
-            */
-            ignoredFieldValues.add(IgnoredSourceFieldMapper.NameValue.fromContext(this, fieldName, XContentDataHelper.nothing()));
-        }
     }
 
     public boolean isCopyToDestinationField(String name) {

server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java

@@ -27,7 +27,6 @@
 import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
 import java.util.Collections;
-import java.util.Comparator;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -113,6 +112,10 @@ NameValue cloneWithValue(BytesRef value) {
             assert value() == null;
             return new NameValue(name, parentOffset, value, doc);
         }
+
+        boolean hasValue() {
+            return XContentDataHelper.isDataPresent(value);
+        }
     }
 
     static final class IgnoredValuesFieldMapperType extends StringFieldType {
@@ -147,10 +150,37 @@ protected String contentType() {
     @Override
     public void postParse(DocumentParserContext context) {
         // Ignored values are only expected in synthetic mode.
-        assert context.getIgnoredFieldValues().isEmpty() || context.mappingLookup().isSourceSynthetic();
+        if (context.mappingLookup().isSourceSynthetic() == false) {
+            assert context.getIgnoredFieldValues().isEmpty();
+            return;
+        }
+
         List<NameValue> ignoredFieldValues = new ArrayList<>(context.getIgnoredFieldValues());
-        // ensure consistent ordering when retrieving synthetic source
-        Collections.sort(ignoredFieldValues, Comparator.comparing(NameValue::name));
+        if (indexSettings.getSkipIgnoredSourceWrite() == false) {
+            /*
+            Mark this field as containing copied data meaning it should not be present
+            in synthetic _source (to be consistent with stored _source).
+            Ignored source values take precedence over standard synthetic source implementation
+            so by adding this nothing entry we "disable" field in synthetic source.
+            Otherwise, it would be constructed f.e. from doc_values which leads to duplicate values
+            in copied field after reindexing.
+            */
+            for (String copyToField : context.copyToFields) {
+                ObjectMapper parent = context.parent().findParentMapper(copyToField);
+                if (parent == null) {
+                    // There are scenarios when this can happen:
+                    // 1. all values of the field that is the source of copy_to are null
+                    // 2. copy_to points at a field inside disabled object
+                    // 3. copy_to points at dynamic field which is not yet applied to mapping, we will process it properly on re-parse.
+                    continue;
+                }
+                int offset = parent.isRoot() ? 0 : parent.fullPath().length() + 1;
+                ignoredFieldValues.add(
+                    new IgnoredSourceFieldMapper.NameValue(copyToField, offset, XContentDataHelper.nothing(), context.doc())
+                );
+            }
+        }
+
         for (NameValue nameValue : ignoredFieldValues) {
             nameValue.doc().add(new StoredField(NAME, encode(nameValue)));
         }

server/src/main/java/org/elasticsearch/index/mapper/IgnoredSourceFieldMapper.java

@@ -40,7 +40,8 @@ public Set<NodeFeature> getFeatures() {
             Mapper.SYNTHETIC_SOURCE_KEEP_FEATURE,
             SourceFieldMapper.SYNTHETIC_SOURCE_WITH_COPY_TO_AND_DOC_VALUES_FALSE_SUPPORT,
             SourceFieldMapper.SYNTHETIC_SOURCE_COPY_TO_FIX,
-            FlattenedFieldMapper.IGNORE_ABOVE_SUPPORT
+            FlattenedFieldMapper.IGNORE_ABOVE_SUPPORT,
+            SourceFieldMapper.SYNTHETIC_SOURCE_COPY_TO_INSIDE_OBJECTS_FIX
         );
     }
 }

server/src/main/java/org/elasticsearch/index/mapper/MapperFeatures.java

@@ -808,6 +808,37 @@ protected void doXContent(XContentBuilder builder, Params params) throws IOExcep
 
     }
 
+    ObjectMapper findParentMapper(String leafFieldPath) {
+        ObjectMapper current = this;
+        String pathInCurrent = leafFieldPath;
+
+        while (current != null) {
+            if (current.mappers.containsKey(pathInCurrent)) {
+                return current;
+            }
+
+            // Go one level down if possible
+            var pathComponents = pathInCurrent.split("\\.");
+            var prefixParts = new ArrayList<String>();
+
+            var parent = current;
+            current = null;
+            for (int i = 0; i < pathComponents.length - 1; i++) {
+                prefixParts.add(pathComponents[i]);
+                var childMapperName = String.join(".", prefixParts);
+                var childMapper = parent.mappers.get(childMapperName);
+
+                if (childMapper instanceof ObjectMapper objectMapper) {
+                    current = objectMapper;
+                    pathInCurrent = pathInCurrent.substring(childMapperName.length() + 1);
+                    break;
+                }
+            }
+        }
+
+        return null;
+    }
+
     protected SourceLoader.SyntheticFieldLoader syntheticFieldLoader(Stream<Mapper> mappers, boolean isFragment) {
         var fields = mappers.sorted(Comparator.comparing(Mapper::fullPath))
             .map(Mapper::syntheticFieldLoader)
@@ -828,10 +859,16 @@ public SourceLoader.SyntheticFieldLoader syntheticFieldLoader() {
     private class SyntheticSourceFieldLoader implements SourceLoader.SyntheticFieldLoader {
         private final List<SourceLoader.SyntheticFieldLoader> fields;
         private final boolean isFragment;
+
         private boolean storedFieldLoadersHaveValues;
         private boolean docValuesLoadersHaveValues;
         private boolean ignoredValuesPresent;
         private List<IgnoredSourceFieldMapper.NameValue> ignoredValues;
+        // If this loader has anything to write.
+        // In special cases this can be false even if doc values loaders or stored field loaders
+        // have values.
+        private boolean writersHaveValues;
+        private TreeMap<String, FieldWriter> currentWriters;
 
         private SyntheticSourceFieldLoader(List<SourceLoader.SyntheticFieldLoader> fields, boolean isFragment) {
             this.fields = fields;
@@ -882,22 +919,69 @@ public boolean advanceToDoc(int docId) throws IOException {
             }
         }
 
+        @Override
+        public void prepare() {
+            if ((storedFieldLoadersHaveValues || docValuesLoadersHaveValues || ignoredValuesPresent) == false) {
+                writersHaveValues = false;
+                return;
+            }
+
+            for (var loader : fields) {
+                // Currently this logic is only relevant for object loaders.
+                if (loader instanceof ObjectMapper.SyntheticSourceFieldLoader objectSyntheticFieldLoader) {
+                    objectSyntheticFieldLoader.prepare();
+                }
+            }
+
+            currentWriters = new TreeMap<>();
+
+            if (ignoredValues != null && ignoredValues.isEmpty() == false) {
+                for (IgnoredSourceFieldMapper.NameValue value : ignoredValues) {
+                    if (value.hasValue()) {
+                        writersHaveValues |= true;
+                    }
+
+                    var existing = currentWriters.get(value.name());
+                    if (existing == null) {
+                        currentWriters.put(value.name(), new FieldWriter.IgnoredSource(value));
+                    } else if (existing instanceof FieldWriter.IgnoredSource isw) {
+                        isw.mergeWith(value);
+                    }
+                }
+            }
+
+            for (SourceLoader.SyntheticFieldLoader field : fields) {
+                if (field.hasValue()) {
+                    if (currentWriters.containsKey(field.fieldName()) == false) {
+                        writersHaveValues |= true;
+                        currentWriters.put(field.fieldName(), new FieldWriter.FieldLoader(field));
+                    } else {
+                        // Skip if the field source is stored separately, to avoid double-printing.
+                        // Make sure to reset the state of loader so that values stored inside will not
+                        // be used after this document is finished.
+                        field.reset();
+                    }
+                }
+            }
+        }
+
         @Override
         public boolean hasValue() {
-            return storedFieldLoadersHaveValues || docValuesLoadersHaveValues || ignoredValuesPresent;
+            return writersHaveValues;
         }
 
         @Override
         public void write(XContentBuilder b) throws IOException {
             if (hasValue() == false) {
                 return;
             }
+
             if (isRoot() && isEnabled() == false) {
                 // If the root object mapper is disabled, it is expected to contain
                 // the source encapsulated within a single ignored source value.
                 assert ignoredValues.size() == 1 : ignoredValues.size();
                 XContentDataHelper.decodeAndWrite(b, ignoredValues.get(0).value());
-                ignoredValues = null;
+                softReset();
                 return;
             }
 
@@ -907,41 +991,10 @@ public void write(XContentBuilder b) throws IOException {
                 b.startObject(leafName());
             }
 
-            if (ignoredValues != null && ignoredValues.isEmpty() == false) {
-                // Use an ordered map between field names and writer functions, to order writing by field name.
-                Map<String, FieldWriter> orderedFields = new TreeMap<>();
-                for (IgnoredSourceFieldMapper.NameValue value : ignoredValues) {
-                    var existing = orderedFields.get(value.name());
-                    if (existing == null) {
-                        orderedFields.put(value.name(), new FieldWriter.IgnoredSource(value));
-                    } else if (existing instanceof FieldWriter.IgnoredSource isw) {
-                        isw.mergeWith(value);
-                    }
-                }
-                for (SourceLoader.SyntheticFieldLoader field : fields) {
-                    if (field.hasValue()) {
-                        if (orderedFields.containsKey(field.fieldName()) == false) {
-                            orderedFields.put(field.fieldName(), new FieldWriter.FieldLoader(field));
-                        } else {
-                            // Skip if the field source is stored separately, to avoid double-printing.
-                            // Make sure to reset the state of loader so that values stored inside will not
-                            // be used after this document is finished.
-                            field.reset();
-                        }
-                    }
-                }
-
-                for (var writer : orderedFields.values()) {
-                    writer.writeTo(b);
-                }
-                ignoredValues = null;
-            } else {
-                for (SourceLoader.SyntheticFieldLoader field : fields) {
-                    if (field.hasValue()) {
-                        field.write(b);
-                    }
-                }
+            for (var writer : currentWriters.values()) {
+                writer.writeTo(b);
             }
+
             b.endObject();
             softReset();
         }
@@ -957,6 +1010,8 @@ private void softReset() {
             storedFieldLoadersHaveValues = false;
             docValuesLoadersHaveValues = false;
             ignoredValuesPresent = false;
+            ignoredValues = null;
+            writersHaveValues = false;
         }
 
         @Override

server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java

@@ -48,6 +48,9 @@ public class SourceFieldMapper extends MetadataFieldMapper {
         "mapper.source.synthetic_source_with_copy_to_and_doc_values_false"
     );
     public static final NodeFeature SYNTHETIC_SOURCE_COPY_TO_FIX = new NodeFeature("mapper.source.synthetic_source_copy_to_fix");
+    public static final NodeFeature SYNTHETIC_SOURCE_COPY_TO_INSIDE_OBJECTS_FIX = new NodeFeature(
+        "mapper.source.synthetic_source_copy_to_inside_objects_fix"
+    );
 
     public static final String NAME = "_source";
     public static final String RECOVERY_SOURCE_NAME = "_recovery_source";

server/src/main/java/org/elasticsearch/index/mapper/SourceFieldMapper.java

@@ -209,6 +209,9 @@ public void write(LeafStoredFieldLoader storedFieldLoader, int docId, XContentBu
                 if (docValuesLoader != null) {
                     docValuesLoader.advanceToDoc(docId);
                 }
+
+                loader.prepare();
+
                 // TODO accept a requested xcontent type
                 if (loader.hasValue()) {
                     loader.write(b);
@@ -299,6 +302,15 @@ public String fieldName() {
          */
         DocValuesLoader docValuesLoader(LeafReader leafReader, int[] docIdsInLeaf) throws IOException;
 
+        /**
+         Perform any preprocessing needed before producing synthetic source.
+         The expectation is for this method to be called before {@link SyntheticFieldLoader#hasValue()}
+         and {@link SyntheticFieldLoader#write(XContentBuilder)} are used.
+         */
+        default void prepare() {
+            // Noop
+        }
+
         /**
          * Has this field loaded any values for this document?
          */

server/src/main/java/org/elasticsearch/index/mapper/SourceLoader.java

@@ -1518,6 +1518,37 @@ public void testStoredNestedSubObjectWithNameOverlappingParentName() throws IOEx
             {"path":{"at":{"foo":"A"}}}""", syntheticSource);
     }
 
+    public void testCopyToLogicInsideObject() throws IOException {
+        DocumentMapper documentMapper = createMapperService(syntheticSourceMapping(b -> {
+            b.startObject("path");
+            b.startObject("properties");
+            {
+                b.startObject("at").field("type", "keyword").field("copy_to", "copy_top.copy").endObject();
+            }
+            b.endObject();
+            b.endObject();
+            b.startObject("copy_top");
+            b.startObject("properties");
+            {
+                b.startObject("copy").field("type", "keyword").endObject();
+            }
+            b.endObject();
+            b.endObject();
+        })).documentMapper();
+
+        CheckedConsumer<XContentBuilder, IOException> document = b -> {
+            b.startObject("path");
+            b.field("at", "A");
+            b.endObject();
+        };
+
+        var doc = documentMapper.parse(source(document));
+        assertNotNull(doc.docs().get(0).getField("copy_top.copy"));
+
+        var syntheticSource = syntheticSource(documentMapper, document);
+        assertEquals("{\"path\":{\"at\":\"A\"}}", syntheticSource);
+    }
+
     protected void validateRoundTripReader(String syntheticSource, DirectoryReader reader, DirectoryReader roundTripReader)
         throws IOException {
         // We exclude ignored source field since in some cases it contains an exact copy of a part of document source.

server/src/test/java/org/elasticsearch/index/mapper/IgnoredSourceFieldMapperTests.java

@@ -811,4 +811,36 @@ public void testFlattenExplicitSubobjectsTrue() {
             exception.getMessage()
         );
     }
+
+    public void testFindParentMapper() {
+        MapperBuilderContext rootContext = MapperBuilderContext.root(false, false);
+
+        var rootBuilder = new RootObjectMapper.Builder("_doc", Optional.empty());
+        rootBuilder.add(new KeywordFieldMapper.Builder("keyword", IndexVersion.current()));
+
+        var child = new ObjectMapper.Builder("child", Optional.empty());
+        child.add(new KeywordFieldMapper.Builder("keyword2", IndexVersion.current()));
+        child.add(new KeywordFieldMapper.Builder("keyword.with.dot", IndexVersion.current()));
+        rootBuilder.add(child);
+
+        var childWithDot = new ObjectMapper.Builder("childwith.dot", Optional.empty());
+        childWithDot.add(new KeywordFieldMapper.Builder("keyword3", IndexVersion.current()));
+        childWithDot.add(new KeywordFieldMapper.Builder("keyword4.with.dot", IndexVersion.current()));
+        rootBuilder.add(childWithDot);
+
+        RootObjectMapper root = rootBuilder.build(rootContext);
+
+        assertEquals("_doc", root.findParentMapper("keyword").fullPath());
+        assertNull(root.findParentMapper("aa"));
+
+        assertEquals("child", root.findParentMapper("child.keyword2").fullPath());
+        assertEquals("child", root.findParentMapper("child.keyword.with.dot").fullPath());
+        assertNull(root.findParentMapper("child.long"));
+        assertNull(root.findParentMapper("child.long.hello"));
+
+        assertEquals("childwith.dot", root.findParentMapper("childwith.dot.keyword3").fullPath());
+        assertEquals("childwith.dot", root.findParentMapper("childwith.dot.keyword4.with.dot").fullPath());
+        assertNull(root.findParentMapper("childwith.dot.long"));
+        assertNull(root.findParentMapper("childwith.dot.long.hello"));
+    }
 }