From af01ccee93e8073c9daf80fa86ba1befc50f8478 Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Thu, 3 Sep 2020 09:20:26 +0100 Subject: [PATCH] Add specific test for serializing all mapping parameter values (#61844) (#61877) This commit adds a test to MapperTestCase that explicitly checks that a mapper can serialize all its default values, and that this serialization can then be re-parsed. Note that the test is disabled for non-parametrized mappers as their serialization may in some cases output parameters that are not accepted. Gradually moving all mappers to parametrized form will address this. The commit also contains a fix to keyword mappers, which were not correctly serializing the similarity parameter; this partially addresses #61563. It also enables `null` as a value for `null_value` on `scaled_float`, as a follow-up to #61798 --- .../index/mapper/ScaledFloatFieldMapper.java | 2 +- .../index/mapper/KeywordFieldMapper.java | 4 +++- .../org/elasticsearch/index/mapper/TypeParsers.java | 7 +++++-- .../index/mapper/KeywordFieldMapperTests.java | 12 ++++++++++++ .../index/mapper/FieldMapperTestCase2.java | 5 +++++ .../index/mapper/MapperServiceTestCase.java | 5 +++++ .../elasticsearch/index/mapper/MapperTestCase.java | 12 ++++++++++++ 7 files changed, 43 insertions(+), 4 deletions(-) diff --git a/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/ScaledFloatFieldMapper.java b/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/ScaledFloatFieldMapper.java index 656656a5265..e4cd28b3705 100644 --- a/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/ScaledFloatFieldMapper.java +++ b/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/ScaledFloatFieldMapper.java @@ -93,7 +93,7 @@ public class ScaledFloatFieldMapper extends ParametrizedFieldMapper { } }); private final Parameter nullValue = new Parameter<>("null_value", false, () -> null, - (n, c, o) -> XContentMapValues.nodeDoubleValue(o), m -> toType(m).nullValue); + (n, c, o) -> o == null ? null : XContentMapValues.nodeDoubleValue(o), m -> toType(m).nullValue).acceptsNull(); private final Parameter> meta = Parameter.metaParam(); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/KeywordFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/KeywordFieldMapper.java index d4a683f79b3..8b90d865615 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/KeywordFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/KeywordFieldMapper.java @@ -100,7 +100,9 @@ public final class KeywordFieldMapper extends ParametrizedFieldMapper { private final Parameter hasNorms = Parameter.boolParam("norms", false, m -> toType(m).fieldType.omitNorms() == false, false); private final Parameter similarity = new Parameter<>("similarity", false, () -> null, - (n, c, o) -> TypeParsers.resolveSimilarity(c, n, o.toString()), m -> toType(m).similarity); + (n, c, o) -> TypeParsers.resolveSimilarity(c, n, o), m -> toType(m).similarity) + .setSerializer((b, f, v) -> b.field(f, v == null ? null : v.name()), v -> v == null ? null : v.name()) + .acceptsNull(); private final Parameter normalizer = Parameter.stringParam("normalizer", false, m -> toType(m).normalizerName, "default"); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java b/server/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java index a1e257ba159..4100d25e338 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java @@ -390,8 +390,11 @@ public class TypeParsers { return copyFields; } - public static SimilarityProvider resolveSimilarity(Mapper.TypeParser.ParserContext parserContext, String name, String value) { - SimilarityProvider similarityProvider = parserContext.getSimilarity(value); + public static SimilarityProvider resolveSimilarity(Mapper.TypeParser.ParserContext parserContext, String name, Object value) { + if (value == null) { + return null; // use default + } + SimilarityProvider similarityProvider = parserContext.getSimilarity(value.toString()); if (similarityProvider == null) { throw new MapperParsingException("Unknown Similarity type [" + value + "] for field [" + name + "]"); } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/KeywordFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/KeywordFieldMapperTests.java index 05909b61313..2a5474c6f35 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/KeywordFieldMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/KeywordFieldMapperTests.java @@ -250,6 +250,18 @@ public class KeywordFieldMapperTests extends MapperTestCase { assertEquals(0, fieldNamesFields.length); } + public void testConfigureSimilarity() throws IOException { + MapperService mapperService = createMapperService( + fieldMapping(b -> b.field("type", "keyword").field("similarity", "boolean")) + ); + MappedFieldType ft = mapperService.documentMapper().fieldTypes().get("field"); + assertEquals("boolean", ft.getTextSearchInfo().getSimilarity().name()); + + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, + () -> merge(mapperService, fieldMapping(b -> b.field("type", "keyword").field("similarity", "BM25")))); + assertThat(e.getMessage(), containsString("Cannot update parameter [similarity] from [boolean] to [BM25]")); + } + public void testNormalizer() throws IOException { DocumentMapper mapper = createDocumentMapper(fieldMapping(b -> b.field("type", "keyword").field("normalizer", "lowercase"))); ParsedDocument doc = mapper.parse(source(b -> b.field("field", "AbC"))); diff --git a/test/framework/src/main/java/org/elasticsearch/index/mapper/FieldMapperTestCase2.java b/test/framework/src/main/java/org/elasticsearch/index/mapper/FieldMapperTestCase2.java index 523264fd074..1fbab6811df 100644 --- a/test/framework/src/main/java/org/elasticsearch/index/mapper/FieldMapperTestCase2.java +++ b/test/framework/src/main/java/org/elasticsearch/index/mapper/FieldMapperTestCase2.java @@ -230,4 +230,9 @@ public abstract class FieldMapperTestCase2> ext : ToXContent.EMPTY_PARAMS; return mapping(b -> builder.toXContent(b, params)); } + + @Override + public void testMinimalToMaximal() { + assumeFalse("`include_defaults` includes unsupported properties in non-parametrized mappers", false); + } } diff --git a/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperServiceTestCase.java b/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperServiceTestCase.java index 0e4a55e7df0..d9d12c391d2 100644 --- a/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperServiceTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperServiceTestCase.java @@ -30,6 +30,7 @@ import org.elasticsearch.common.CheckedConsumer; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.compress.CompressedXContent; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.common.xcontent.XContentType; @@ -49,6 +50,7 @@ import org.elasticsearch.test.ESTestCase; import java.io.IOException; import java.util.Collection; +import java.util.Collections; import static java.util.Collections.emptyList; import static java.util.Collections.emptyMap; @@ -63,6 +65,9 @@ public abstract class MapperServiceTestCase extends ESTestCase { protected static final Settings SETTINGS = Settings.builder().put("index.version.created", Version.CURRENT).build(); + protected static final ToXContent.Params INCLUDE_DEFAULTS + = new ToXContent.MapParams(Collections.singletonMap("include_defaults", "true")); + protected Collection getPlugins() { return emptyList(); } diff --git a/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java b/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java index 7145a045bf2..d96a5137b69 100644 --- a/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java @@ -63,6 +63,18 @@ public abstract class MapperTestCase extends MapperServiceTestCase { assertParseMinimalWarnings(); } + // TODO make this final once we remove FieldMapperTestCase2 + public void testMinimalToMaximal() throws IOException { + XContentBuilder orig = JsonXContent.contentBuilder().startObject(); + createMapperService(fieldMapping(this::minimalMapping)).documentMapper().mapping().toXContent(orig, INCLUDE_DEFAULTS); + orig.endObject(); + XContentBuilder parsedFromOrig = JsonXContent.contentBuilder().startObject(); + createMapperService(orig).documentMapper().mapping().toXContent(parsedFromOrig, INCLUDE_DEFAULTS); + parsedFromOrig.endObject(); + assertEquals(Strings.toString(orig), Strings.toString(parsedFromOrig)); + assertParseMinimalWarnings(); + } + protected void assertParseMinimalWarnings() { // Most mappers don't emit any warnings }