Merge field mappers when updating mappings with [subobjects:false] (#120370) (#120520)

* Merge field mappers when updating mappings with [subobjects:false]

* Update docs/changelog/120370.yaml

* fix and add tests

* node feature guarding

(cherry picked from commit 0a5d2e7)

# Conflicts:
#	server/src/main/java/org/elasticsearch/index/mapper/MapperFeatures.java

Author: kkrik-es

docs/changelog/120370.yaml

@@ -0,0 +1,6 @@
+pr: 120370
+summary: "Merge field mappers when updating mappings with [subobjects:false]"
+area: Mapping
+type: bug
+issues:
+ - 120216

rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.put_mapping/10_basic.yml

@@ -211,3 +211,92 @@
         body:
           _source:
             mode: synthetic
+
+---
+"modify field type with subobjects:false":
+  - requires:
+      cluster_features: [ "mapper.subobjects_false_mapping_update_fix" ]
+      reason: requires fix for mapping updates when [subobjects:false] is set
+  - do:
+      indices.create:
+        index: test_index
+        body:
+          mappings:
+            subobjects: false
+            properties:
+              user.id:
+                type: long
+              user.name:
+                type: text
+
+  - do:
+      catch: bad_request
+      indices.put_mapping:
+        index: test_index
+        body:
+          properties:
+            user.id:
+              type: keyword
+
+  - match: { error.type: "illegal_argument_exception" }
+  - match: { error.reason: "mapper [user.id] cannot be changed from type [long] to [keyword]" }
+
+  - do:
+      indices.put_mapping:
+        index: test_index
+        body:
+          properties:
+            user.name:
+              type: text
+              fields:
+                raw:
+                  type: keyword
+
+  - is_true: acknowledged
+
+
+---
+"modify nested field type with subobjects:false":
+  - requires:
+      cluster_features: [ "mapper.subobjects_false_mapping_update_fix" ]
+      reason: requires fix for mapping updates when [subobjects:false] is set
+  - do:
+      indices.create:
+        index: test_index
+        body:
+          mappings:
+            properties:
+              path:
+                properties:
+                  to:
+                    subobjects: false
+                    properties:
+                      user.id:
+                        type: long
+                      user.name:
+                        type: text
+
+  - do:
+      catch: bad_request
+      indices.put_mapping:
+        index: test_index
+        body:
+          properties:
+            path.to.user.id:
+              type: keyword
+
+  - match: { error.type: "illegal_argument_exception" }
+  - match: { error.reason: "mapper [path.to.user.id] cannot be changed from type [long] to [keyword]" }
+
+  - do:
+      indices.put_mapping:
+        index: test_index
+        body:
+          properties:
+            path.to.user.name:
+              type: text
+              fields:
+                raw:
+                  type: keyword
+
+  - is_true: acknowledged

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

@@ -71,7 +71,8 @@ public Set<NodeFeature> getTestFeatures() {
             CONSTANT_KEYWORD_SYNTHETIC_SOURCE_WRITE_FIX,
             META_FETCH_FIELDS_ERROR_CODE_CHANGED,
             SPARSE_VECTOR_STORE_SUPPORT,
-            SourceFieldMapper.SYNTHETIC_RECOVERY_SOURCE
+            SourceFieldMapper.SYNTHETIC_RECOVERY_SOURCE,
+            ObjectMapper.SUBOBJECTS_FALSE_MAPPING_UPDATE_FIX
         );
     }
 }

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

@@ -49,6 +49,7 @@ public class ObjectMapper extends Mapper {
     private static final Logger logger = LogManager.getLogger(ObjectMapper.class);
     private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(ObjectMapper.class);
     public static final FeatureFlag SUB_OBJECTS_AUTO_FEATURE_FLAG = new FeatureFlag("sub_objects_auto");
+    static final NodeFeature SUBOBJECTS_FALSE_MAPPING_UPDATE_FIX = new NodeFeature("mapper.subobjects_false_mapping_update_fix");
 
     public static final String CONTENT_TYPE = "object";
     static final String STORE_ARRAY_SOURCE_PARAM = "store_array_source";
@@ -663,11 +664,21 @@ private static Map<String, Mapper> buildMergedMappers(
                     if (subobjects.isPresent()
                         && subobjects.get() == Subobjects.DISABLED
                         && mergeWithMapper instanceof ObjectMapper objectMapper) {
-                        // An existing mapping that has set `subobjects: false` is merged with a mapping with sub-objects
-                        objectMapper.asFlattenedFieldMappers(objectMergeContext.getMapperBuilderContext())
-                            .stream()
-                            .filter(m -> objectMergeContext.decrementFieldBudgetIfPossible(m.getTotalFieldsCount()))
-                            .forEach(m -> putMergedMapper(mergedMappers, m));
+                        // An existing mapping that has set `subobjects: false` is merged with a mapping with sub-objects.
+                        List<FieldMapper> flattenedMappers = objectMapper.asFlattenedFieldMappers(
+                            objectMergeContext.getMapperBuilderContext()
+                        );
+                        for (FieldMapper flattenedMapper : flattenedMappers) {
+                            if (objectMergeContext.decrementFieldBudgetIfPossible(flattenedMapper.getTotalFieldsCount())) {
+                                var conflict = mergedMappers.get(flattenedMapper.leafName());
+                                if (objectMergeContext.getMapperBuilderContext().getMergeReason() == MergeReason.INDEX_TEMPLATE
+                                    || conflict == null) {
+                                    putMergedMapper(mergedMappers, flattenedMapper);
+                                } else {
+                                    putMergedMapper(mergedMappers, conflict.merge(flattenedMapper, objectMergeContext));
+                                }
+                            }
+                        }
                     } else if (objectMergeContext.decrementFieldBudgetIfPossible(mergeWithMapper.getTotalFieldsCount())) {
                         putMergedMapper(mergedMappers, mergeWithMapper);
                     } else if (mergeWithMapper instanceof ObjectMapper om) {

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

@@ -329,6 +329,40 @@ public void testDisallowFieldReplacementForIndexTemplates() throws IOException {
         assertThat(e.getMessage(), containsString("can't merge a non object mapping [object.field1] with an object mapping"));
     }
 
+    public void testFieldReplacementSubobjectsFalse() throws IOException {
+        MapperService mapperService = createMapperService(mapping(b -> {
+            b.startObject("obj").field("type", "object").field("subobjects", false).startObject("properties");
+            {
+                b.startObject("my.field").field("type", "keyword").endObject();
+            }
+            b.endObject().endObject();
+        }));
+        DocumentMapper mapper = mapperService.documentMapper();
+        assertNull(mapper.mapping().getRoot().dynamic());
+        Mapping mergeWith = mapperService.parseMapping(
+            "_doc",
+            MergeReason.INDEX_TEMPLATE,
+            new CompressedXContent(BytesReference.bytes(topMapping(b -> {
+                b.startObject("properties").startObject("obj").field("type", "object").field("subobjects", false).startObject("properties");
+                {
+                    b.startObject("my.field").field("type", "long").endObject();
+                }
+                b.endObject().endObject().endObject();
+            })))
+        );
+
+        // Fails on mapping update.
+        IllegalArgumentException exception = expectThrows(
+            IllegalArgumentException.class,
+            () -> mapper.mapping().merge(mergeWith, MergeReason.MAPPING_UPDATE, Long.MAX_VALUE)
+        );
+        assertEquals("mapper [obj.my.field] cannot be changed from type [keyword] to [long]", exception.getMessage());
+
+        // Passes on template merging.
+        Mapping merged = mapper.mapping().merge(mergeWith, MergeReason.INDEX_TEMPLATE, Long.MAX_VALUE);
+        assertThat(((ObjectMapper) merged.getRoot().getMapper("obj")).getMapper("my.field"), instanceOf(NumberFieldMapper.class));
+    }
+
     public void testUnknownLegacyFields() throws Exception {
         MapperService service = createMapperService(IndexVersion.fromId(5000099), Settings.EMPTY, () -> false, mapping(b -> {
             b.startObject("name");