RootObjectMapper to clone the runtime fields map (elastic#65378)

ObjectMappers clone themselves as part of their merge method, and also as part of dynamic mapping updates. As part of its clone, RootObjectMapper should clone its map of runtime fields, otherwise changes to the cloned root around runtime fields are reflected in the original root, which is not desirable.
javanna · Nov 24, 2020 · 6144d9e · 6144d9e
1 parent 679bc3c
commit 6144d9e
Show file tree

Hide file tree

Showing 2 changed files with 52 additions and 2 deletions.
diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java
@@ -240,6 +240,13 @@ private boolean processField(RootObjectMapper.Builder builder, String fieldName,
         this.numericDetection = numericDetection;
     }
 
+    @Override
+    protected ObjectMapper clone() {
+        ObjectMapper clone = super.clone();
+        ((RootObjectMapper) clone).runtimeFieldTypes = new HashMap<>(this.runtimeFieldTypes);
+        return clone;
+    }
+
     @Override
     public ObjectMapper mappingUpdate(Mapper mapper) {
         RootObjectMapper update = (RootObjectMapper) super.mappingUpdate(mapper);
@@ -250,7 +257,8 @@ public ObjectMapper mappingUpdate(Mapper mapper) {
         update.dynamicDateTimeFormatters = new Explicit<>(Defaults.DYNAMIC_DATE_TIME_FORMATTERS, false);
         update.dateDetection = new Explicit<>(Defaults.DATE_DETECTION, false);
         update.numericDetection = new Explicit<>(Defaults.NUMERIC_DETECTION, false);
-        update.runtimeFieldTypes = new HashMap<>();
+        //also no need to carry the already defined runtime fields, only new ones need to be added
+        update.runtimeFieldTypes.clear();
         return update;
     }
 
@@ -355,7 +363,7 @@ protected void doMerge(ObjectMapper mergeWith, MergeReason reason) {
                 this.dynamicTemplates = mergeWithObject.dynamicTemplates;
             }
         }
-
+        assert this.runtimeFieldTypes != mergeWithObject.runtimeFieldTypes;
         this.runtimeFieldTypes.putAll(mergeWithObject.runtimeFieldTypes);
     }
 

diff --git a/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java
@@ -489,6 +489,48 @@ public void testRuntimeSection() throws IOException {
         assertEquals(mapping, mapperService.documentMapper().mappingSource().toString());
     }
 
+    public void testRuntimeSectionRejectedUpdate() throws IOException {
+        MapperService mapperService;
+        {
+            XContentBuilder builder = XContentFactory.jsonBuilder().startObject().startObject("_doc");
+            builder.startObject("runtime");
+            builder.startObject("field").field("type", "test").endObject();
+            builder.endObject();
+            builder.startObject("properties");
+            builder.startObject("concrete").field("type", "keyword").endObject();
+            builder.endObject();
+            builder.endObject().endObject();
+            mapperService = createMapperService(builder);
+            assertEquals(Strings.toString(builder), mapperService.documentMapper().mappingSource().toString());
+            MappedFieldType concrete = mapperService.fieldType("concrete");
+            assertThat(concrete, instanceOf(KeywordFieldMapper.KeywordFieldType.class));
+            MappedFieldType field = mapperService.fieldType("field");
+            assertThat(field, instanceOf(RuntimeField.class));
+        }
+        {
+            XContentBuilder builder = XContentFactory.jsonBuilder().startObject().startObject("_doc");
+            builder.startObject("runtime");
+            builder.startObject("another_field").field("type", "test").endObject();
+            builder.endObject();
+            builder.startObject("properties");
+            //try changing the type of the existing concrete field, so that merge fails
+            builder.startObject("concrete").field("type", "text").endObject();
+            builder.endObject();
+            builder.endObject().endObject();
+
+            expectThrows(IllegalArgumentException.class, () -> merge(mapperService, builder));
+
+            //make sure that the whole rejected update, including changes to runtime fields, has not been applied
+            MappedFieldType concrete = mapperService.fieldType("concrete");
+            assertThat(concrete, instanceOf(KeywordFieldMapper.KeywordFieldType.class));
+            MappedFieldType field = mapperService.fieldType("field");
+            assertThat(field, instanceOf(RuntimeField.class));
+            assertNull(mapperService.fieldType("another_field"));
+            assertEquals("{\"_doc\":{\"runtime\":{\"field\":{\"type\":\"test\"}},\"properties\":{\"concrete\":{\"type\":\"keyword\"}}}}",
+                Strings.toString(mapperService.documentMapper().mapping().root));
+        }
+    }
+
     public void testRuntimeSectionMerge() throws IOException {
         MapperService mapperService;
         {