From 3269d1b48649026cf386ffaac14ead82bbf39e33 Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Wed, 2 Sep 2020 20:03:36 +0100 Subject: [PATCH] Add specific test for serializing all mapping parameter values (#61844) 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 | 3 +++ .../elasticsearch/index/mapper/MapperTestCase.java | 12 ++++++++++++ 7 files changed, 41 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 6276042c0f31c..f3975b6d57e56 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 @@ -92,7 +92,7 @@ public static class Builder extends ParametrizedFieldMapper.Builder { } }); 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 cf9b8840d7a7c..cfe1a9151c839 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/KeywordFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/KeywordFieldMapper.java @@ -99,7 +99,9 @@ public static class Builder extends ParametrizedFieldMapper.Builder { 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 a6c384249679b..bf7239d47a7fb 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java @@ -398,8 +398,11 @@ public static List parseCopyFields(Object propNode) { 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 a5424e11efd1c..e653fb8726ec0 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 void testEnableNorms() throws IOException { 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 d7021591bf961..6a313ace211d8 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 @@ -228,4 +228,9 @@ private XContentBuilder mappingsToJson(ToXContent builder, boolean includeDefaul ToXContent.Params params = includeDefaults ? new ToXContent.MapParams(Map.of("include_defaults", "true")) : 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 bfd8b8948a228..17d8a15b65d07 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.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; @@ -63,6 +64,8 @@ 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(Map.of("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 be92d8fe51a6f..b1737093f1fe2 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 @@ -64,6 +64,18 @@ public final void testMinimalSerializesToItself() throws IOException { 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 }