Propagate MapperBuilderContext across merge calls (#86946)

With #86166 we have added support for leaf fields that have dots in their names, without needing to expand their path to intermediate objects. That means that for instance a host.name keyword field mapper can be held directly by the root object mapper if the root has subobjects set to false. This opens up for situations where a field name containing dots can't necessarily be split into a leaf name and a parent path made of intermediate object mappers. In the field mapper merge code, we build the full path of the merged field making that now incorrect assumption, which causes the result of merged leaf fields to have incorrect full path. This is a bug although it got unnoticed so far as the full path of a field mapper is not so widely used compared to its leaf name (returned by the simpleName method). One place where the full path is used is when sorting the child mappers in ObjectMapper#serializeMappers which was causing MapperService#assertRefreshIsNotNeeded to trip as the result of the merge has fields in a different order compared to the incoming mappings. With this fix, we add a MapperBuilderContext argument to the different merge methods, propagate the MapperBuilderContext in all the merge calls and create a child context when needed, meaning whenever merging object mappers, so that their children mappers are created with the proper full path.
elastic · May 20, 2022 · 095d1fc · 095d1fc
1 parent 954d288
commit 095d1fc
Show file tree

Hide file tree

Showing 14 changed files with 199 additions and 65 deletions.
diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java
@@ -364,6 +364,14 @@ public DocumentDimensions getDimensions() {
 
     public abstract ContentPath path();
 
+    public final MapperBuilderContext createMapperBuilderContext() {
+        String p = path().pathAsText("");
+        if (p.endsWith(".")) {
+            p = p.substring(0, p.length() - 1);
+        }
+        return new MapperBuilderContext(p);
+    }
+
     public abstract XContentParser parser();
 
     public abstract LuceneDocument rootDoc();

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DynamicFieldsBuilder.java b/server/src/main/java/org/elasticsearch/index/mapper/DynamicFieldsBuilder.java
@@ -159,15 +159,17 @@ void createDynamicFieldFromValue(final DocumentParserContext context, XContentPa
      */
     static Mapper createDynamicObjectMapper(DocumentParserContext context, String name) {
         Mapper mapper = createObjectMapperFromTemplate(context, name);
-        return mapper != null ? mapper : new ObjectMapper.Builder(name).enabled(true).build(MapperBuilderContext.forPath(context.path()));
+        return mapper != null
+            ? mapper
+            : new ObjectMapper.Builder(name).enabled(ObjectMapper.Defaults.ENABLED).build(context.createMapperBuilderContext());
     }
 
     /**
      * Returns a dynamically created object mapper, based exclusively on a matching dynamic template, null otherwise.
      */
     static Mapper createObjectMapperFromTemplate(DocumentParserContext context, String name) {
         Mapper.Builder templateBuilder = findTemplateBuilderForObject(context, name);
-        return templateBuilder == null ? null : templateBuilder.build(MapperBuilderContext.forPath(context.path()));
+        return templateBuilder == null ? null : templateBuilder.build(context.createMapperBuilderContext());
     }
 
     /**
@@ -302,7 +304,7 @@ private static final class Concrete implements Strategy {
         }
 
         void createDynamicField(Mapper.Builder builder, DocumentParserContext context) throws IOException {
-            Mapper mapper = builder.build(MapperBuilderContext.forPath(context.path()));
+            Mapper mapper = builder.build(context.createMapperBuilderContext());
             context.addDynamicMapper(mapper);
             parseField.accept(context, mapper);
         }

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/FieldAliasMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/FieldAliasMapper.java
@@ -55,7 +55,7 @@ public String path() {
     }
 
     @Override
-    public Mapper merge(Mapper mergeWith) {
+    public Mapper merge(Mapper mergeWith, MapperBuilderContext mapperBuilderContext) {
         if ((mergeWith instanceof FieldAliasMapper) == false) {
             throw new IllegalArgumentException(
                 "Cannot merge a field alias mapping [" + name() + "] with a mapping that is not for a field alias."

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java
@@ -321,7 +321,7 @@ private static void checkNestedScopeCompatibility(String source, String target)
     public abstract Builder getMergeBuilder();
 
     @Override
-    public final FieldMapper merge(Mapper mergeWith) {
+    public final FieldMapper merge(Mapper mergeWith, MapperBuilderContext mapperBuilderContext) {
         if (mergeWith == this) {
             return this;
         }
@@ -343,9 +343,9 @@ public final FieldMapper merge(Mapper mergeWith) {
             return (FieldMapper) mergeWith;
         }
         Conflicts conflicts = new Conflicts(name());
-        builder.merge((FieldMapper) mergeWith, conflicts);
+        builder.merge((FieldMapper) mergeWith, conflicts, mapperBuilderContext);
         conflicts.check();
-        return builder.build(MapperBuilderContext.forPath(Builder.parentPath(name())));
+        return builder.build(mapperBuilderContext);
     }
 
     protected void checkIncomingMergeType(FieldMapper mergeWith) {
@@ -408,7 +408,7 @@ public Builder update(FieldMapper toMerge, MapperBuilderContext context) {
                     add(toMerge);
                 } else {
                     FieldMapper existing = mapperBuilders.get(toMerge.simpleName()).apply(context);
-                    add(existing.merge(toMerge));
+                    add(existing.merge(toMerge, context));
                 }
                 return this;
             }
@@ -1138,12 +1138,13 @@ public Builder init(FieldMapper initializer) {
             return this;
         }
 
-        protected void merge(FieldMapper in, Conflicts conflicts) {
+        protected void merge(FieldMapper in, Conflicts conflicts, MapperBuilderContext mapperBuilderContext) {
             for (Parameter<?> param : getParameters()) {
                 param.merge(in, conflicts);
             }
+            MapperBuilderContext childContext = mapperBuilderContext.createChildContext(in.simpleName());
             for (FieldMapper newSubField : in.multiFields.mappers) {
-                multiFieldsBuilder.update(newSubField, MapperBuilderContext.forPath(parentPath(newSubField.name())));
+                multiFieldsBuilder.update(newSubField, childContext);
             }
             this.copyTo.reset(in.copyTo);
             validate();

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/Mapper.java b/server/src/main/java/org/elasticsearch/index/mapper/Mapper.java
@@ -25,6 +25,7 @@ protected Builder(String name) {
             this.name = internFieldName(name);
         }
 
+        // TODO rename this to leafName?
         public String name() {
             return this.name;
         }
@@ -53,11 +54,13 @@ public Mapper(String simpleName) {
 
     /** Returns the simple name, which identifies this mapper against other mappers at the same level in the mappers hierarchy
      * TODO: make this protected once Mapper and FieldMapper are merged together */
+    // TODO rename this to leafName?
     public final String simpleName() {
         return simpleName;
     }
 
     /** Returns the canonical name which uniquely identifies the mapper against other mappers in a type. */
+    // TODO rename this to fullPath???
     public abstract String name();
 
     /**
@@ -67,7 +70,7 @@ public final String simpleName() {
 
     /** Return the merge of {@code mergeWith} into this.
      *  Both {@code this} and {@code mergeWith} will be left unmodified. */
-    public abstract Mapper merge(Mapper mergeWith);
+    public abstract Mapper merge(Mapper mergeWith, MapperBuilderContext mapperBuilderContext);
 
     /**
      * Validate any cross-field references made by this mapper

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperBuilderContext.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperBuilderContext.java
@@ -10,29 +10,26 @@
 
 import org.elasticsearch.common.Strings;
 
+import java.util.Objects;
+
 /**
  * Holds context for building Mapper objects from their Builders
  */
-public class MapperBuilderContext {
+public final class MapperBuilderContext {
 
     /**
      * The root context, to be used when building a tree of mappers
      */
-    public static final MapperBuilderContext ROOT = new MapperBuilderContext(null);
-
-    // TODO remove this
-    public static MapperBuilderContext forPath(ContentPath path) {
-        String p = path.pathAsText("");
-        if (p.endsWith(".")) {
-            p = p.substring(0, p.length() - 1);
-        }
-        return new MapperBuilderContext(p);
-    }
+    public static final MapperBuilderContext ROOT = new MapperBuilderContext();
 
     private final String path;
 
-    private MapperBuilderContext(String path) {
-        this.path = path;
+    private MapperBuilderContext() {
+        this.path = null;
+    }
+
+    MapperBuilderContext(String path) {
+        this.path = Objects.requireNonNull(path);
     }
 
     /**
@@ -47,7 +44,7 @@ public MapperBuilderContext createChildContext(String name) {
     /**
      * Builds the full name of the field, taking into account parent objects
      */
-    public final String buildFullName(String name) {
+    public String buildFullName(String name) {
         if (Strings.isEmpty(path)) {
             return name;
         }

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java b/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java
@@ -127,7 +127,7 @@ Mapping mappingUpdate(RootObjectMapper rootObjectMapper) {
      * @return the resulting merged mapping.
      */
     Mapping merge(Mapping mergeWith, MergeReason reason) {
-        RootObjectMapper mergedRoot = root.merge(mergeWith.root, reason);
+        RootObjectMapper mergedRoot = root.merge(mergeWith.root, reason, MapperBuilderContext.ROOT);
 
         // When merging metadata fields as part of applying an index template, new field definitions
         // completely overwrite existing ones instead of being merged. This behavior matches how we
@@ -139,7 +139,7 @@ Mapping merge(Mapping mergeWith, MergeReason reason) {
             if (mergeInto == null || reason == MergeReason.INDEX_TEMPLATE) {
                 merged = metaMergeWith;
             } else {
-                merged = (MetadataFieldMapper) mergeInto.merge(metaMergeWith);
+                merged = (MetadataFieldMapper) mergeInto.merge(metaMergeWith, MapperBuilderContext.ROOT);
             }
             mergedMetadataMappers.put(merged.getClass(), merged);
         }

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java
@@ -170,7 +170,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
     }
 
     @Override
-    public ObjectMapper merge(Mapper mergeWith, MapperService.MergeReason reason) {
+    public ObjectMapper merge(Mapper mergeWith, MapperService.MergeReason reason, MapperBuilderContext mapperBuilderContext) {
         if ((mergeWith instanceof NestedObjectMapper) == false) {
             throw new IllegalArgumentException("can't merge a non nested mapping [" + mergeWith.name() + "] with a nested mapping");
         }
@@ -191,7 +191,7 @@ public ObjectMapper merge(Mapper mergeWith, MapperService.MergeReason reason) {
                 throw new MapperException("the [include_in_root] parameter can't be updated on a nested object mapping");
             }
         }
-        toMerge.doMerge(mergeWithObject, reason);
+        toMerge.doMerge(mergeWithObject, reason, mapperBuilderContext);
         return toMerge;
     }
 

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java
@@ -163,7 +163,7 @@ protected final Map<String, Mapper> buildMappers(boolean root, MapperBuilderCont
                 }
                 Mapper existing = mappers.get(mapper.simpleName());
                 if (existing != null) {
-                    mapper = existing.merge(mapper);
+                    mapper = existing.merge(mapper, mapperBuilderContext);
                 }
                 mappers.put(mapper.simpleName(), mapper);
             }
@@ -414,8 +414,8 @@ public final boolean subobjects() {
     }
 
     @Override
-    public ObjectMapper merge(Mapper mergeWith) {
-        return merge(mergeWith, MergeReason.MAPPING_UPDATE);
+    public ObjectMapper merge(Mapper mergeWith, MapperBuilderContext mapperBuilderContext) {
+        return merge(mergeWith, MergeReason.MAPPING_UPDATE, mapperBuilderContext);
     }
 
     @Override
@@ -425,7 +425,7 @@ public void validate(MappingLookup mappers) {
         }
     }
 
-    public ObjectMapper merge(Mapper mergeWith, MergeReason reason) {
+    public ObjectMapper merge(Mapper mergeWith, MergeReason reason, MapperBuilderContext mapperBuilderContext) {
         if ((mergeWith instanceof ObjectMapper) == false) {
             throw new IllegalArgumentException("can't merge a non object mapping [" + mergeWith.name() + "] with an object mapping");
         }
@@ -435,11 +435,11 @@ public ObjectMapper merge(Mapper mergeWith, MergeReason reason) {
         }
         ObjectMapper mergeWithObject = (ObjectMapper) mergeWith;
         ObjectMapper merged = clone();
-        merged.doMerge(mergeWithObject, reason);
+        merged.doMerge(mergeWithObject, reason, mapperBuilderContext);
         return merged;
     }
 
-    protected void doMerge(final ObjectMapper mergeWith, MergeReason reason) {
+    protected void doMerge(final ObjectMapper mergeWith, MergeReason reason, MapperBuilderContext mapperBuilderContext) {
 
         if (mergeWith.dynamic != null) {
             this.dynamic = mergeWith.dynamic;
@@ -469,7 +469,8 @@ protected void doMerge(final ObjectMapper mergeWith, MergeReason reason) {
             if (mergeIntoMapper == null) {
                 merged = mergeWithMapper;
             } else if (mergeIntoMapper instanceof ObjectMapper objectMapper) {
-                merged = objectMapper.merge(mergeWithMapper, reason);
+                MapperBuilderContext childContext = mapperBuilderContext.createChildContext(objectMapper.simpleName());
+                merged = objectMapper.merge(mergeWithMapper, reason, childContext);
             } else {
                 assert mergeIntoMapper instanceof FieldMapper || mergeIntoMapper instanceof FieldAliasMapper;
                 if (mergeWithMapper instanceof ObjectMapper) {
@@ -483,7 +484,7 @@ protected void doMerge(final ObjectMapper mergeWith, MergeReason reason) {
                 if (reason == MergeReason.INDEX_TEMPLATE) {
                     merged = mergeWithMapper;
                 } else {
-                    merged = mergeIntoMapper.merge(mergeWithMapper);
+                    merged = mergeIntoMapper.merge(mergeWithMapper, mapperBuilderContext);
                 }
             }
             if (mergedMappers == null) {

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/PlaceHolderFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/PlaceHolderFieldMapper.java
@@ -65,10 +65,10 @@ public FieldMapper.Builder init(FieldMapper initializer) {
         }
 
         @Override
-        protected void merge(FieldMapper in, Conflicts conflicts) {
+        protected void merge(FieldMapper in, Conflicts conflicts, MapperBuilderContext mapperBuilderContext) {
             assert in instanceof PlaceHolderFieldMapper;
             unknownParams.putAll(((PlaceHolderFieldMapper) in).unknownParams);
-            super.merge(in, conflicts);
+            super.merge(in, conflicts, mapperBuilderContext);
         }
 
         @Override

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java
@@ -322,13 +322,13 @@ RuntimeField getRuntimeField(String name) {
     }
 
     @Override
-    public RootObjectMapper merge(Mapper mergeWith, MergeReason reason) {
-        return (RootObjectMapper) super.merge(mergeWith, reason);
+    public RootObjectMapper merge(Mapper mergeWith, MergeReason reason, MapperBuilderContext mapperBuilderContext) {
+        return (RootObjectMapper) super.merge(mergeWith, reason, mapperBuilderContext);
     }
 
     @Override
-    protected void doMerge(ObjectMapper mergeWith, MergeReason reason) {
-        super.doMerge(mergeWith, reason);
+    protected void doMerge(ObjectMapper mergeWith, MergeReason reason, MapperBuilderContext mapperBuilderContext) {
+        super.doMerge(mergeWith, reason, mapperBuilderContext);
         RootObjectMapper mergeWithObject = (RootObjectMapper) mergeWith;
         if (mergeWithObject.numericDetection.explicit()) {
             this.numericDetection = mergeWithObject.numericDetection;

diff --git a/server/src/test/java/org/elasticsearch/index/mapper/FieldAliasMapperValidationTests.java b/server/src/test/java/org/elasticsearch/index/mapper/FieldAliasMapperValidationTests.java
@@ -158,9 +158,7 @@ public void testFieldAliasWithDifferentNestedScopes() {
     }
 
     private static FieldMapper createFieldMapper(String parent, String name) {
-        return new BooleanFieldMapper.Builder(name, ScriptCompiler.NONE, Version.CURRENT).build(
-            MapperBuilderContext.forPath(new ContentPath(parent))
-        );
+        return new BooleanFieldMapper.Builder(name, ScriptCompiler.NONE, Version.CURRENT).build(new MapperBuilderContext(parent));
     }
 
     private static ObjectMapper createObjectMapper(String name) {