From c810a4a12e803677009c218048604ab49bb9863d Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Mon, 13 Jul 2020 11:07:52 +0100 Subject: [PATCH] Continue to accept unused 'universal' params in <8.0 indexes (#59381) We have a number of parameters which are universally parsed by almost all mappers, whether or not they make sense. Migrating the binary and boolean mappers to the new style of declaring their parameters explicitly has meant that these universal parameters stopped being accepted, which would break existing mappings. This commit adds some extra logic to ParametrizedFieldMapper that checks for the existence of these universal parameters, and issues a warning on 7x indexes if it finds them. Indexes created in 8.0 and beyond will throw an error. Fixes #59359 --- .../index/mapper/ParametrizedFieldMapper.java | 25 +++++++++++++++++++ .../index/mapper/ParametrizedMapperTests.java | 25 +++++++++++++++---- 2 files changed, 45 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ParametrizedFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/ParametrizedFieldMapper.java index f0fe1906d05..3437db86eec 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ParametrizedFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ParametrizedFieldMapper.java @@ -19,18 +19,24 @@ package org.elasticsearch.index.mapper; +import org.apache.logging.log4j.LogManager; import org.apache.lucene.document.FieldType; +import org.elasticsearch.Version; +import org.elasticsearch.common.logging.DeprecationLogger; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.support.XContentMapValues; import java.io.IOException; import java.util.ArrayList; +import java.util.Arrays; import java.util.HashMap; +import java.util.HashSet; import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Set; import java.util.function.BiFunction; import java.util.function.Function; @@ -46,6 +52,9 @@ import java.util.function.Function; */ public abstract class ParametrizedFieldMapper extends FieldMapper { + private static final DeprecationLogger deprecationLogger + = new DeprecationLogger(LogManager.getLogger(ParametrizedFieldMapper.class)); + /** * Creates a new ParametrizedFieldMapper */ @@ -330,6 +339,12 @@ public abstract class ParametrizedFieldMapper extends FieldMapper { } Parameter parameter = paramsMap.get(propName); if (parameter == null) { + if (isDeprecatedParameter(propName, parserContext.indexVersionCreated())) { + deprecationLogger.deprecatedAndMaybeLog(propName, + "Parameter [{}] has no effect on type [{}] and will be removed in future", propName, type); + iterator.remove(); + continue; + } throw new MapperParsingException("unknown parameter [" + propName + "] on mapper [" + name + "] of type [" + type + "]"); } @@ -341,5 +356,15 @@ public abstract class ParametrizedFieldMapper extends FieldMapper { iterator.remove(); } } + + // 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. + private static final Set DEPRECATED_PARAMS + = new HashSet<>(Arrays.asList("store", "meta", "index", "doc_values", "boost", "index_options", "similarity")); + + private static boolean isDeprecatedParameter(String propName, Version indexCreatedVersion) { + return DEPRECATED_PARAMS.contains(propName); + } } } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java index 119485d3dbb..777c41d60e1 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java @@ -67,6 +67,7 @@ public class ParametrizedMapperTests extends ESSingleNodeTestCase { = Parameter.boolParam("fixed2", false, m -> toType(m).fixed2, false); final Parameter variable = Parameter.stringParam("variable", true, m -> toType(m).variable, "default").acceptsNull(); + final Parameter index = Parameter.boolParam("index", false, m -> toType(m).index, true); protected Builder(String name) { super(name); @@ -74,7 +75,7 @@ public class ParametrizedMapperTests extends ESSingleNodeTestCase { @Override protected List> getParameters() { - return Arrays.asList(fixed, fixed2, variable); + return Arrays.asList(fixed, fixed2, variable, index); } @Override @@ -99,6 +100,7 @@ public class ParametrizedMapperTests extends ESSingleNodeTestCase { private final boolean fixed; private final boolean fixed2; private final String variable; + private final boolean index; protected TestMapper(String simpleName, String fullName, MultiFields multiFields, CopyTo copyTo, ParametrizedMapperTests.Builder builder) { @@ -106,6 +108,7 @@ public class ParametrizedMapperTests extends ESSingleNodeTestCase { this.fixed = builder.fixed.getValue(); this.fixed2 = builder.fixed2.getValue(); this.variable = builder.variable.getValue(); + this.index = builder.index.getValue(); } @Override @@ -124,7 +127,7 @@ public class ParametrizedMapperTests extends ESSingleNodeTestCase { } } - private static TestMapper fromMapping(String mapping) { + private static TestMapper fromMapping(String mapping, Version version) { Mapper.TypeParser.ParserContext pc = new Mapper.TypeParser.ParserContext(s -> null, null, s -> { if (Objects.equals("keyword", s)) { return new KeywordFieldMapper.TypeParser(); @@ -133,12 +136,16 @@ public class ParametrizedMapperTests extends ESSingleNodeTestCase { return new BinaryFieldMapper.TypeParser(); } return null; - }, Version.CURRENT, () -> null); + }, version, () -> null); return (TestMapper) new TypeParser() .parse("field", XContentHelper.convertToMap(JsonXContent.jsonXContent, mapping, true), pc) .build(new Mapper.BuilderContext(Settings.EMPTY, new ContentPath(0))); } + private static TestMapper fromMapping(String mapping) { + return fromMapping(mapping, Version.CURRENT); + } + // defaults - create empty builder config, and serialize with and without defaults public void testDefaults() throws IOException { String mapping = "{\"type\":\"test_mapper\"}"; @@ -154,7 +161,8 @@ public class ParametrizedMapperTests extends ESSingleNodeTestCase { builder.startObject(); mapper.toXContent(builder, params); builder.endObject(); - assertEquals("{\"field\":{\"type\":\"test_mapper\",\"fixed\":true,\"fixed2\":false,\"variable\":\"default\"}}", + assertEquals("{\"field\":{\"type\":\"test_mapper\",\"fixed\":true," + + "\"fixed2\":false,\"variable\":\"default\",\"index\":true}}", Strings.toString(builder)); } @@ -245,8 +253,15 @@ public class ParametrizedMapperTests extends ESSingleNodeTestCase { indexService.mapperService().merge("_doc", new CompressedXContent(mapping), MapperService.MergeReason.MAPPING_UPDATE); assertEquals(mapping, Strings.toString(indexService.mapperService().documentMapper())); + } - + public void testDeprecatedParameters() throws IOException { + // 'index' is declared explicitly, 'store' is not, but is one of the previously always-accepted params + String mapping = "{\"type\":\"test_mapper\",\"index\":false,\"store\":true}"; + TestMapper mapper = fromMapping(mapping, Version.V_7_8_0); + assertWarnings("Parameter [store] has no effect on type [test_mapper] and will be removed in future"); + assertFalse(mapper.index); + assertEquals("{\"field\":{\"type\":\"test_mapper\",\"index\":false}}", Strings.toString(mapper)); } }