Skip to content

Commit

Permalink
Emit deprecation warnings only for new index or template (elastic#117529
Browse files Browse the repository at this point in the history 
)

Currently, we emit a deprecation warning in the parser of the source 
field when source mode is used in mappings. However, this behavior 
causes warnings to be emitted for every mapping update. In tests with
assertions enabled, warnings are also triggered for every change to
index metadata. As a result, deprecation warnings are inadvertently
emitted for index or update requests.

This change relocates the deprecation check to the mapper, limiting it 
to cases where a new index is created or a template is created/updated.

Relates to elastic#117524
  • Loading branch information
@dnhatn
dnhatn authored Nov 27, 2024
1 parent 66108eb commit 5c928a4
Show file tree
Hide file tree
Showing 5 changed files with 11 additions and 32 deletions.
9 changes: 9 additions & 0 deletions server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
package org.elasticsearch.index.mapper;

import org.elasticsearch.common.compress.CompressedXContent;
import org.elasticsearch.common.logging.DeprecationCategory;
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.common.xcontent.XContentHelper;
import org.elasticsearch.core.Nullable;
import org.elasticsearch.index.mapper.MapperService.MergeReason;
Expand All @@ -31,6 +33,7 @@ public final class MappingParser {
private final Supplier<Map<Class<? extends MetadataFieldMapper>, MetadataFieldMapper>> metadataMappersSupplier;
private final Map<String, MetadataFieldMapper.TypeParser> metadataMapperParsers;
private final Function<String, String> documentTypeResolver;
private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(MappingParser.class);

MappingParser(
Supplier<MappingParserContext> mappingParserContextSupplier,
Expand Down Expand Up @@ -144,6 +147,12 @@ Mapping parse(@Nullable String type, MergeReason reason, Map<String, Object> map
}
@SuppressWarnings("unchecked")
Map<String, Object> fieldNodeMap = (Map<String, Object>) fieldNode;
if (reason == MergeReason.INDEX_TEMPLATE
&& SourceFieldMapper.NAME.equals(fieldName)
&& fieldNodeMap.containsKey("mode")
&& SourceFieldMapper.onOrAfterDeprecateModeVersion(mappingParserContext.indexVersionCreated())) {
deprecationLogger.critical(DeprecationCategory.MAPPINGS, "mapping_source_mode", SourceFieldMapper.DEPRECATION_WARNING);
}
MetadataFieldMapper metadataFieldMapper = typeParser.parse(fieldName, fieldNodeMap, mappingParserContext).build();
metadataMappers.put(metadataFieldMapper.getClass(), metadataFieldMapper);
assert fieldNodeMap.isEmpty();
Expand Down
14 changes: 1 addition & 13 deletions server/src/main/java/org/elasticsearch/index/mapper/SourceFieldMapper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import org.elasticsearch.common.Explicit;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.logging.DeprecationCategory;
import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.util.CollectionUtils;
Expand All @@ -40,7 +39,6 @@
import java.util.Collections;
import java.util.List;
import java.util.Locale;
import java.util.Map;

public class SourceFieldMapper extends MetadataFieldMapper {
public static final NodeFeature SYNTHETIC_SOURCE_FALLBACK = new NodeFeature("mapper.source.synthetic_source_fallback");
Expand Down Expand Up @@ -310,17 +308,7 @@ private static SourceFieldMapper resolveStaticInstance(final Mode sourceMode) {
c.indexVersionCreated().onOrAfter(IndexVersions.SOURCE_MAPPER_LOSSY_PARAMS_CHECK),
onOrAfterDeprecateModeVersion(c.indexVersionCreated()) == false
)
) {
@Override
public MetadataFieldMapper.Builder parse(String name, Map<String, Object> node, MappingParserContext parserContext)
throws MapperParsingException {
assert name.equals(SourceFieldMapper.NAME) : name;
if (onOrAfterDeprecateModeVersion(parserContext.indexVersionCreated()) && node.containsKey("mode")) {
deprecationLogger.critical(DeprecationCategory.MAPPINGS, "mapping_source_mode", SourceFieldMapper.DEPRECATION_WARNING);
}
return super.parse(name, node, parserContext);
}
};
);

static final class SourceFieldType extends MappedFieldType {
private final boolean enabled;
Expand Down
1 change: 0 additions & 1 deletion server/src/test/java/org/elasticsearch/index/mapper/DocumentParserContextTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,5 @@ public void testCreateDynamicMapperBuilderContext() throws IOException {
assertEquals(ObjectMapper.Defaults.DYNAMIC, resultFromParserContext.getDynamic());
assertEquals(MapperService.MergeReason.MAPPING_UPDATE, resultFromParserContext.getMergeReason());
assertFalse(resultFromParserContext.isInNestedContext());
assertWarnings(SourceFieldMapper.DEPRECATION_WARNING);
}
}
17 changes: 1 addition & 16 deletions server/src/test/java/org/elasticsearch/index/mapper/SourceFieldMapperTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ protected void registerParameters(ParameterChecker checker) throws IOException {
topMapping(b -> b.startObject(SourceFieldMapper.NAME).field("mode", "synthetic").endObject()),
dm -> {
assertTrue(dm.metadataMapper(SourceFieldMapper.class).isSynthetic());
assertWarnings(SourceFieldMapper.DEPRECATION_WARNING);
}
);
checker.registerConflictCheck("includes", b -> b.array("includes", "foo*"));
Expand All @@ -74,7 +73,7 @@ protected void registerParameters(ParameterChecker checker) throws IOException {
"mode",
topMapping(b -> b.startObject(SourceFieldMapper.NAME).field("mode", "synthetic").endObject()),
topMapping(b -> b.startObject(SourceFieldMapper.NAME).field("mode", "stored").endObject()),
dm -> assertWarnings(SourceFieldMapper.DEPRECATION_WARNING)
d -> {}
);
}

Expand Down Expand Up @@ -211,22 +210,19 @@ public void testSyntheticDisabledNotSupported() {
)
);
assertThat(e.getMessage(), containsString("Cannot set both [mode] and [enabled] parameters"));
assertWarnings(SourceFieldMapper.DEPRECATION_WARNING);
}

public void testSyntheticUpdates() throws Exception {
MapperService mapperService = createMapperService("""
{ "_doc" : { "_source" : { "mode" : "synthetic" } } }
""");
assertWarnings(SourceFieldMapper.DEPRECATION_WARNING);
SourceFieldMapper mapper = mapperService.documentMapper().sourceMapper();
assertTrue(mapper.enabled());
assertTrue(mapper.isSynthetic());

merge(mapperService, """
{ "_doc" : { "_source" : { "mode" : "synthetic" } } }
""");
assertWarnings(SourceFieldMapper.DEPRECATION_WARNING);
mapper = mapperService.documentMapper().sourceMapper();
assertTrue(mapper.enabled());
assertTrue(mapper.isSynthetic());
Expand All @@ -239,12 +235,10 @@ public void testSyntheticUpdates() throws Exception {
"""));

assertThat(e.getMessage(), containsString("Cannot update parameter [mode] from [synthetic] to [stored]"));
assertWarnings(SourceFieldMapper.DEPRECATION_WARNING);

merge(mapperService, """
{ "_doc" : { "_source" : { "mode" : "disabled" } } }
""");
assertWarnings(SourceFieldMapper.DEPRECATION_WARNING);

mapper = mapperService.documentMapper().sourceMapper();
assertFalse(mapper.enabled());
Expand Down Expand Up @@ -281,7 +275,6 @@ public void testSupportsNonDefaultParameterValues() throws IOException {
topMapping(b -> b.startObject("_source").field("mode", randomBoolean() ? "synthetic" : "stored").endObject())
).documentMapper().sourceMapper();
assertThat(sourceFieldMapper, notNullValue());
assertWarnings(SourceFieldMapper.DEPRECATION_WARNING);
}
Exception e = expectThrows(
MapperParsingException.class,
Expand Down Expand Up @@ -313,8 +306,6 @@ public void testSupportsNonDefaultParameterValues() throws IOException {
.documentMapper()
.sourceMapper()
);
assertWarnings(SourceFieldMapper.DEPRECATION_WARNING);

assertThat(e.getMessage(), containsString("Parameter [mode=disabled] is not allowed in source"));

e = expectThrows(
Expand Down Expand Up @@ -423,7 +414,6 @@ public void testRecoverySourceWithSyntheticSource() throws IOException {
ParsedDocument doc = docMapper.parse(source(b -> { b.field("field1", "value1"); }));
assertNotNull(doc.rootDoc().getField("_recovery_source"));
assertThat(doc.rootDoc().getField("_recovery_source").binaryValue(), equalTo(new BytesRef("{\"field1\":\"value1\"}")));
assertWarnings(SourceFieldMapper.DEPRECATION_WARNING);
}
{
Settings settings = Settings.builder().put(INDICES_RECOVERY_SOURCE_ENABLED_SETTING.getKey(), false).build();
Expand All @@ -434,7 +424,6 @@ public void testRecoverySourceWithSyntheticSource() throws IOException {
DocumentMapper docMapper = mapperService.documentMapper();
ParsedDocument doc = docMapper.parse(source(b -> b.field("field1", "value1")));
assertNull(doc.rootDoc().getField("_recovery_source"));
assertWarnings(SourceFieldMapper.DEPRECATION_WARNING);
}
}

Expand Down Expand Up @@ -629,7 +618,6 @@ public void testRecoverySourceWithLogsCustom() throws IOException {
ParsedDocument doc = docMapper.parse(source(b -> { b.field("@timestamp", "2012-02-13"); }));
assertNotNull(doc.rootDoc().getField("_recovery_source"));
assertThat(doc.rootDoc().getField("_recovery_source").binaryValue(), equalTo(new BytesRef("{\"@timestamp\":\"2012-02-13\"}")));
assertWarnings(SourceFieldMapper.DEPRECATION_WARNING);
}
{
Settings settings = Settings.builder()
Expand All @@ -640,7 +628,6 @@ public void testRecoverySourceWithLogsCustom() throws IOException {
DocumentMapper docMapper = mapperService.documentMapper();
ParsedDocument doc = docMapper.parse(source(b -> b.field("@timestamp", "2012-02-13")));
assertNull(doc.rootDoc().getField("_recovery_source"));
assertWarnings(SourceFieldMapper.DEPRECATION_WARNING);
}
}

Expand Down Expand Up @@ -709,7 +696,6 @@ public void testRecoverySourceWithTimeSeriesCustom() throws IOException {
doc.rootDoc().getField("_recovery_source").binaryValue(),
equalTo(new BytesRef("{\"@timestamp\":\"2012-02-13\",\"field\":\"value1\"}"))
);
assertWarnings(SourceFieldMapper.DEPRECATION_WARNING);
}
{
Settings settings = Settings.builder()
Expand All @@ -723,7 +709,6 @@ public void testRecoverySourceWithTimeSeriesCustom() throws IOException {
source("123", b -> b.field("@timestamp", "2012-02-13").field("field", randomAlphaOfLength(5)), null)
);
assertNull(doc.rootDoc().getField("_recovery_source"));
assertWarnings(SourceFieldMapper.DEPRECATION_WARNING);
}
}
}
2 changes: 0 additions & 2 deletions server/src/test/java/org/elasticsearch/index/shard/ShardGetServiceTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import org.elasticsearch.index.engine.VersionConflictEngineException;
import org.elasticsearch.index.get.GetResult;
import org.elasticsearch.index.mapper.RoutingFieldMapper;
import org.elasticsearch.index.mapper.SourceFieldMapper;
import org.elasticsearch.search.fetch.subphase.FetchSourceContext;
import org.elasticsearch.xcontent.XContentType;

Expand Down Expand Up @@ -115,7 +114,6 @@ public void testGetFromTranslogWithSyntheticSource() throws IOException {
"mode": "synthetic"
""";
runGetFromTranslogWithOptions(docToIndex, sourceOptions, expectedFetchedSource, "\"long\"", 7L, true);
assertWarnings(SourceFieldMapper.DEPRECATION_WARNING);
}

public void testGetFromTranslogWithDenseVector() throws IOException {
Expand Down

0 comments on commit 5c928a4

Please sign in to comment.