Deprecate silently ignoring type, fields, copy_to and boost in metadata field definition (#90989)

The parsing code for metadata fields historically inherits from the existing parsing code for fields in the mappings, where type is the one parameter that every field type accepts. As a consequence, the metadata field definition has been silently ignoring the type parameter, despite the type of metadata fields can never be changed. Furthermore, with the recent move to parsing through builders (#59847), we've been accepting other unsupported parameters like fields, copy_to and boost.

This commit refactors the parsing method for metadata field builders to not reuse the base logic but a rather simplified version. In the case of type, fields, copy_to and boost we issue a deprecation warning starting from indices created on 8.6.

Closes #35389

docs/changelog/90989.yaml

@@ -0,0 +1,16 @@
+pr: 90989
+summary: Deprecate silently ignoring type, fields, copy_to and boost in metadata field definition
+area: Mapping
+type: deprecation
+issues:
+ - 35389
+deprecation:
+  title: Deprecate silently ignoring type, fields, copy_to and boost in metadata field definition
+  area: Mapping
+  details: Unsupported parameters like type, fields, copy_to and boost are silently ignored when
+    provided as part of the configuration of a metadata field in the index mappings. They will
+    cause a deprecation warning when used in the mappings for indices that are created from 8.6
+    onwards.
+  impact: To resolve the deprecation warning, remove the mention of type, fields, copy_to or boost
+    from any metadata field definition as part of index mappings. They take no effect so removing
+    them won't have any impact besides resolving the deprecation warning.

modules/data-streams/src/test/java/org/elasticsearch/datastreams/mapper/DataStreamTimestampFieldMapperTests.java

@@ -37,6 +37,11 @@ public class DataStreamTimestampFieldMapperTests extends MetadataMapperTestCase
         return DataStreamTimestampFieldMapper.NAME;
     }
 
+    @Override
+    protected boolean isConfigurable() {
+        return true;
+    }
+
     @Override
     protected void registerParameters(ParameterChecker checker) throws IOException {
         checker.registerConflictCheck(

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

@@ -62,7 +62,7 @@ public abstract class FieldMapper extends Mapper implements Cloneable {
     );
     public static final Setting<Boolean> COERCE_SETTING = Setting.boolSetting("index.mapping.coerce", false, Property.IndexScope);
 
-    private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(FieldMapper.class);
+    protected static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(FieldMapper.class);
     @SuppressWarnings("rawtypes")
     static final Parameter<?>[] EMPTY_PARAMETERS = new Parameter[0];
 
@@ -1162,7 +1162,7 @@ public abstract class FieldMapper extends Mapper implements Cloneable {
             validate();
         }
 
-        private void validate() {
+        protected final void validate() {
             for (Parameter<?> param : getParameters()) {
                 param.validate();
             }
@@ -1326,14 +1326,6 @@ public abstract class FieldMapper extends Mapper implements Cloneable {
             // ignore
         }
 
-        protected static ContentPath parentPath(String name) {
-            int endPos = name.lastIndexOf(".");
-            if (endPos == -1) {
-                return new ContentPath(0);
-            }
-            return new ContentPath(name.substring(0, endPos));
-        }
-
         // These parameters were previously *always* parsed by TypeParsers#parseField(), even if they
         // made no sense; if we've got here, that means that they're not declared on a current mapper,
         // and so we emit a deprecation warning rather than failing a previously working mapping.

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

@@ -122,9 +122,7 @@ public final class MappingParser {
                 MetadataFieldMapper metadataFieldMapper = typeParser.parse(fieldName, fieldNodeMap, parserContext)
                     .build(MapperBuilderContext.forMetadata());
                 metadataMappers.put(metadataFieldMapper.getClass(), metadataFieldMapper);
-                fieldNodeMap.remove("type");
-                checkNoRemainingFields(fieldName, fieldNodeMap);
-
+                assert fieldNodeMap.isEmpty();
                 if (metadataFieldMapper instanceof SourceFieldMapper sfm) {
                     isSourceSynthetic = sfm.isSynthetic();
                 }

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

@@ -8,12 +8,17 @@
 
 package org.elasticsearch.index.mapper;
 
+import org.elasticsearch.Version;
 import org.elasticsearch.common.Explicit;
+import org.elasticsearch.common.logging.DeprecationCategory;
+import org.elasticsearch.common.util.Maps;
 import org.elasticsearch.common.xcontent.support.XContentMapValues;
 import org.elasticsearch.xcontent.XContentBuilder;
 
 import java.io.IOException;
+import java.util.Iterator;
 import java.util.Map;
+import java.util.Set;
 import java.util.function.Function;
 
 /**
@@ -99,7 +104,7 @@ public abstract class MetadataFieldMapper extends FieldMapper {
         @Override
         public Builder parse(String name, Map<String, Object> node, MappingParserContext parserContext) throws MapperParsingException {
             Builder builder = builderFunction.apply(parserContext);
-            builder.parse(name, parserContext, node);
+            builder.parseMetadataField(name, parserContext, node);
             return builder;
         }
 
@@ -129,6 +134,42 @@ public abstract class MetadataFieldMapper extends FieldMapper {
             return build();
         }
 
+        private static final Set<String> UNSUPPORTED_PARAMETERS_8_6_0 = Set.of("type", "fields", "copy_to", "boost");
+
+        public final void parseMetadataField(String name, MappingParserContext parserContext, Map<String, Object> fieldNode) {
+            final Parameter<?>[] params = getParameters();
+            Map<String, Parameter<?>> paramsMap = Maps.newHashMapWithExpectedSize(params.length);
+            for (Parameter<?> param : params) {
+                paramsMap.put(param.name, param);
+            }
+            for (Iterator<Map.Entry<String, Object>> iterator = fieldNode.entrySet().iterator(); iterator.hasNext();) {
+                Map.Entry<String, Object> entry = iterator.next();
+                final String propName = entry.getKey();
+                final Object propNode = entry.getValue();
+                Parameter<?> parameter = paramsMap.get(propName);
+                if (parameter == null) {
+                    if (UNSUPPORTED_PARAMETERS_8_6_0.contains(propName)) {
+                        if (parserContext.indexVersionCreated().onOrAfter(Version.V_8_6_0)) {
+                            // silently ignore type, and a few other parameters: sadly we've been doing this for a long time
+                            deprecationLogger.warn(
+                                DeprecationCategory.API,
+                                propName,
+                                "Parameter [{}] has no effect on metadata field [{}] and will be removed in future",
+                                propName,
+                                name
+                            );
+                        }
+                        iterator.remove();
+                        continue;
+                    }
+                    throw new MapperParsingException("unknown parameter [" + propName + "] on metadata field [" + name + "]");
+                }
+                parameter.parse(name, parserContext, propNode);
+                iterator.remove();
+            }
+            validate();
+        }
+
         public abstract MetadataFieldMapper build();
     }
 

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

@@ -21,11 +21,24 @@ import java.util.stream.IntStream;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
 
-public class DocCountFieldMapperTests extends MapperServiceTestCase {
+public class DocCountFieldMapperTests extends MetadataMapperTestCase {
 
     private static final String CONTENT_TYPE = DocCountFieldMapper.CONTENT_TYPE;
     private static final String DOC_COUNT_FIELD = DocCountFieldMapper.NAME;
 
+    @Override
+    protected String fieldName() {
+        return DocCountFieldMapper.NAME;
+    }
+
+    @Override
+    protected boolean isConfigurable() {
+        return false;
+    }
+
+    @Override
+    protected void registerParameters(ParameterChecker checker) throws IOException {}
+
     public void testParseValue() throws Exception {
         DocumentMapper mapper = createDocumentMapper(mapping(b -> {}));
         ParsedDocument doc = mapper.parse(source(b -> b.field("foo", 500).field(CONTENT_TYPE, 100)));

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

@@ -16,19 +16,33 @@ import org.elasticsearch.test.VersionUtils;
 import org.elasticsearch.xcontent.XContentFactory;
 import org.elasticsearch.xcontent.XContentType;
 
+import java.io.IOException;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.Set;
 import java.util.SortedSet;
 import java.util.TreeSet;
 
-public class FieldNamesFieldMapperTests extends MapperServiceTestCase {
+public class FieldNamesFieldMapperTests extends MetadataMapperTestCase {
+
+    @Override
+    protected String fieldName() {
+        return FieldNamesFieldMapper.NAME;
+    }
+
+    @Override
+    protected boolean isConfigurable() {
+        return true;
+    }
+
+    @Override
+    protected void registerParameters(ParameterChecker checker) throws IOException {}
 
     private static SortedSet<String> set(String... values) {
         return new TreeSet<>(Arrays.asList(values));
     }
 
-    void assertFieldNames(Set<String> expected, ParsedDocument doc) {
+    private static void assertFieldNames(Set<String> expected, ParsedDocument doc) {
         String[] got = TermVectorsService.getValues(doc.rootDoc().getFields("_field_names"));
         assertEquals(expected, set(got));
     }

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

@@ -24,7 +24,20 @@ import static org.hamcrest.Matchers.containsString;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
-public class IgnoredFieldMapperTests extends MapperServiceTestCase {
+public class IgnoredFieldMapperTests extends MetadataMapperTestCase {
+
+    @Override
+    protected String fieldName() {
+        return IgnoredFieldMapper.NAME;
+    }
+
+    @Override
+    protected boolean isConfigurable() {
+        return false;
+    }
+
+    @Override
+    protected void registerParameters(ParameterChecker checker) throws IOException {}
 
     public void testIncludeInObjectNotAllowed() throws Exception {
         DocumentMapper docMapper = createDocumentMapper(mapping(b -> {}));

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

@@ -22,7 +22,20 @@ import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.nullValue;
 
-public class IndexFieldMapperTests extends MapperServiceTestCase {
+public class IndexFieldMapperTests extends MetadataMapperTestCase {
+
+    @Override
+    protected String fieldName() {
+        return IndexFieldMapper.NAME;
+    }
+
+    @Override
+    protected boolean isConfigurable() {
+        return false;
+    }
+
+    @Override
+    protected void registerParameters(ParameterChecker checker) throws IOException {}
 
     public void testDefaultDisabledIndexMapper() throws Exception {
         DocumentMapper docMapper = createDocumentMapper(mapping(b -> {}));

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

@@ -9,6 +9,7 @@
 package org.elasticsearch.index.mapper;
 
 import org.apache.lucene.index.IndexableField;
+import org.elasticsearch.Version;
 import org.elasticsearch.common.bytes.BytesArray;
 import org.elasticsearch.xcontent.XContentType;
 
@@ -16,7 +17,25 @@ import java.io.IOException;
 import java.util.Arrays;
 import java.util.Collections;
 
-public class NestedPathFieldMapperTests extends MapperServiceTestCase {
+public class NestedPathFieldMapperTests extends MetadataMapperTestCase {
+
+    @Override
+    protected String fieldName() {
+        return NestedPathFieldMapper.NAME;
+    }
+
+    @Override
+    protected boolean isSupportedOn(Version version) {
+        return version.onOrAfter(Version.V_8_0_0);
+    }
+
+    @Override
+    protected boolean isConfigurable() {
+        return false;
+    }
+
+    @Override
+    protected void registerParameters(ParameterChecker checker) throws IOException {}
 
     public void testDefaults() throws IOException {
         DocumentMapper mapper = createDocumentMapper(mapping(b -> {}));

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

@@ -34,6 +34,11 @@ public class RoutingFieldMapperTests extends MetadataMapperTestCase {
         return RoutingFieldMapper.NAME;
     }
 
+    @Override
+    protected boolean isConfigurable() {
+        return true;
+    }
+
     @Override
     protected void registerParameters(ParameterChecker checker) throws IOException {
         checker.registerConflictCheck("required", b -> b.field("required", true));

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

@@ -30,6 +30,11 @@ public class SourceFieldMapperTests extends MetadataMapperTestCase {
         return SourceFieldMapper.NAME;
     }
 
+    @Override
+    protected boolean isConfigurable() {
+        return true;
+    }
+
     @Override
     protected void registerParameters(ParameterChecker checker) throws IOException {
         checker.registerConflictCheck(

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

@@ -36,6 +36,11 @@ public class TimeSeriesIdFieldMapperTests extends MetadataMapperTestCase {
         return TimeSeriesIdFieldMapper.NAME;
     }
 
+    @Override
+    protected boolean isConfigurable() {
+        return false;
+    }
+
     @Override
     protected void registerParameters(ParameterChecker checker) throws IOException {
         // There aren't any parameters

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

@@ -584,6 +584,11 @@ public class TsidExtractingIdFieldMapperTests extends MetadataMapperTestCase {
         return IdFieldMapper.NAME;
     }
 
+    @Override
+    protected boolean isConfigurable() {
+        return false;
+    }
+
     @Override
     protected void registerParameters(ParameterChecker checker) throws IOException {}
 

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

@@ -23,7 +23,20 @@ import java.util.List;
 
 import static org.hamcrest.Matchers.containsString;
 
-public class VersionFieldMapperTests extends MapperServiceTestCase {
+public class VersionFieldMapperTests extends MetadataMapperTestCase {
+
+    @Override
+    protected String fieldName() {
+        return VersionFieldMapper.NAME;
+    }
+
+    @Override
+    protected boolean isConfigurable() {
+        return false;
+    }
+
+    @Override
+    protected void registerParameters(ParameterChecker checker) throws IOException {}
 
     public void testIncludeInObjectNotAllowed() throws Exception {
         DocumentMapper docMapper = createDocumentMapper(mapping(b -> {}));

test/framework/src/main/java/org/elasticsearch/cluster/metadata/DataStreamTestHelper.java

@@ -501,7 +501,6 @@ public final class DataStreamTestHelper {
 
     public static MetadataFieldMapper getDataStreamTimestampFieldMapper() {
         Map<String, Object> fieldsMapping = new HashMap<>();
-        fieldsMapping.put("type", DataStreamTimestampFieldMapper.NAME);
         fieldsMapping.put("enabled", true);
         MappingParserContext mockedParserContext = mock(MappingParserContext.class);
         return DataStreamTimestampFieldMapper.PARSER.parse("field", fieldsMapping, mockedParserContext).build();

test/framework/src/main/java/org/elasticsearch/index/mapper/MetadataMapperTestCase.java

@@ -8,7 +8,10 @@
 
 package org.elasticsearch.index.mapper;
 
+import org.elasticsearch.Version;
+import org.elasticsearch.common.compress.CompressedXContent;
 import org.elasticsearch.core.CheckedConsumer;
+import org.elasticsearch.test.VersionUtils;
 import org.elasticsearch.xcontent.XContentBuilder;
 
 import java.io.IOException;
@@ -25,6 +28,12 @@ public abstract class MetadataMapperTestCase extends MapperServiceTestCase {
 
     protected abstract String fieldName();
 
+    protected abstract boolean isConfigurable();
+
+    protected boolean isSupportedOn(Version version) {
+        return true;
+    }
+
     protected abstract void registerParameters(ParameterChecker checker) throws IOException;
 
     private record ConflictCheck(XContentBuilder init, XContentBuilder update) {}
@@ -67,6 +76,7 @@ public abstract class MetadataMapperTestCase extends MapperServiceTestCase {
     }
 
     public final void testUpdates() throws IOException {
+        assumeTrue("Metadata field " + fieldName() + " isn't configurable", isConfigurable());
         ParameterChecker checker = new ParameterChecker();
         registerParameters(checker);
         for (String param : checker.conflictChecks.keySet()) {
@@ -92,4 +102,89 @@ public abstract class MetadataMapperTestCase extends MapperServiceTestCase {
             updateCheck.check.accept(mapperService.documentMapper());
         }
     }
+
+    public final void testUnsupportedParametersAreRejected() throws IOException {
+        assumeTrue("Metadata field " + fieldName() + " isn't configurable", isConfigurable());
+        Version version = VersionUtils.randomIndexCompatibleVersion(random());
+        assumeTrue("Metadata field " + fieldName() + " is not supported on version " + version, isSupportedOn(version));
+        MapperService mapperService = createMapperService(version, mapping(xContentBuilder -> {}));
+        String mappingAsString = "{\n"
+            + "    \"_doc\" : {\n"
+            + "      \""
+            + fieldName()
+            + "\" : {\n"
+            + "        \"anything\" : \"anything\"\n"
+            + "      }\n"
+            + "    }\n"
+            + "}";
+        MapperParsingException exception = expectThrows(
+            MapperParsingException.class,
+            () -> mapperService.parseMapping("_doc", new CompressedXContent(mappingAsString))
+        );
+        assertEquals(
+            "Failed to parse mapping: unknown parameter [anything] on metadata field [" + fieldName() + "]",
+            exception.getMessage()
+        );
+    }
+
+    public final void testFixedMetaFieldsAreNotConfigurable() throws IOException {
+        assumeFalse("Metadata field " + fieldName() + " is configurable", isConfigurable());
+        Version version = VersionUtils.randomIndexCompatibleVersion(random());
+        assumeTrue("Metadata field " + fieldName() + " is not supported on version " + version, isSupportedOn(version));
+        MapperService mapperService = createMapperService(version, mapping(xContentBuilder -> {}));
+        String mappingAsString = "{\n" + "    \"_doc\" : {\n" + "      \"" + fieldName() + "\" : {\n" + "      }\n" + "    }\n" + "}";
+        MapperParsingException exception = expectThrows(
+            MapperParsingException.class,
+            () -> mapperService.parseMapping("_doc", new CompressedXContent(mappingAsString))
+        );
+        assertEquals("Failed to parse mapping: " + fieldName() + " is not configurable", exception.getMessage());
+    }
+
+    public void testTypeAndFriendsAreAcceptedBefore_8_6_0() throws IOException {
+        assumeTrue("Metadata field " + fieldName() + " isn't configurable", isConfigurable());
+        Version previousVersion = VersionUtils.getPreviousVersion(Version.V_8_6_0);
+        Version version = VersionUtils.randomVersionBetween(random(), previousVersion.minimumIndexCompatibilityVersion(), previousVersion);
+        assumeTrue("Metadata field " + fieldName() + " is not supported on version " + version, isSupportedOn(version));
+        MapperService mapperService = createMapperService(version, mapping(b -> {}));
+        // these parameters were previously silently ignored, they will still be ignored in existing indices
+        String[] unsupportedParameters = new String[] { "fields", "copy_to", "boost", "type" };
+        for (String param : unsupportedParameters) {
+            String mappingAsString = "{\n"
+                + "    \"_doc\" : {\n"
+                + "      \""
+                + fieldName()
+                + "\" : {\n"
+                + "        \""
+                + param
+                + "\" : \"any\"\n"
+                + "      }\n"
+                + "    }\n"
+                + "}";
+            assertNotNull(mapperService.parseMapping("_doc", new CompressedXContent(mappingAsString)));
+        }
+    }
+
+    public void testTypeAndFriendsAreDeprecatedFrom_8_6_0() throws IOException {
+        assumeTrue("Metadata field " + fieldName() + " isn't configurable", isConfigurable());
+        Version version = VersionUtils.randomVersionBetween(random(), Version.V_8_6_0, Version.CURRENT);
+        assumeTrue("Metadata field " + fieldName() + " is not supported on version " + version, isSupportedOn(version));
+        MapperService mapperService = createMapperService(version, mapping(b -> {}));
+        // these parameters were previously silently ignored, they are now deprecated in new indices
+        String[] unsupportedParameters = new String[] { "fields", "copy_to", "boost", "type" };
+        for (String param : unsupportedParameters) {
+            String mappingAsString = "{\n"
+                + "    \"_doc\" : {\n"
+                + "      \""
+                + fieldName()
+                + "\" : {\n"
+                + "        \""
+                + param
+                + "\" : \"any\"\n"
+                + "      }\n"
+                + "    }\n"
+                + "}";
+            assertNotNull(mapperService.parseMapping("_doc", new CompressedXContent(mappingAsString)));
+            assertWarnings("Parameter [" + param + "] has no effect on metadata field [" + fieldName() + "] and will be removed in future");
+        }
+    }
 }