From 467c26c231050bc094149c64c068419ca312b4e4 Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Mon, 7 Jun 2021 14:51:44 +0100 Subject: [PATCH 1/3] RuntimeField.Builder should not extend FieldMapper.Builder --- .../index/mapper/FieldMapper.java | 12 ++- .../index/mapper/RuntimeField.java | 79 ++++++++++++------- .../AbstractScriptFieldTypeTestCase.java | 5 +- 3 files changed, 66 insertions(+), 30 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java index 6e173f33d1102..3c1fe6dbfa262 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java @@ -653,6 +653,10 @@ public Parameter acceptsNull() { return this; } + public boolean canAcceptNull() { + return acceptsNull; + } + /** * Adds a deprecated parameter name. * @@ -757,7 +761,13 @@ private void init(FieldMapper toInit) { setValue(initializer.apply(toInit)); } - private void parse(String field, ParserContext context, Object in) { + /** + * Parse the field value from an Object + * @param field the field name + * @param context the parser context + * @param in the object + */ + public void parse(String field, ParserContext context, Object in) { setValue(parser.apply(field, context, in)); } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RuntimeField.java b/server/src/main/java/org/elasticsearch/index/mapper/RuntimeField.java index b589069dae304..be11e18cd42a9 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RuntimeField.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RuntimeField.java @@ -8,8 +8,13 @@ package org.elasticsearch.index.mapper; +import org.elasticsearch.Version; +import org.elasticsearch.common.logging.DeprecationCategory; +import org.elasticsearch.common.logging.DeprecationLogger; +import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.common.xcontent.ToXContentFragment; import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.index.mapper.FieldMapper.Parameter; import java.io.IOException; import java.util.Collections; @@ -58,49 +63,70 @@ default XContentBuilder toXContent(XContentBuilder builder, Params params) throw /** * For runtime fields the {@link RuntimeField.Parser} returns directly the {@link MappedFieldType}. - * Internally we still create a {@link RuntimeField.Builder} so we reuse the {@link FieldMapper.Parameter} infrastructure, - * but {@link RuntimeField.Builder#init(FieldMapper)} and {@link RuntimeField.Builder#build(ContentPath)} are never called as - * {@link RuntimeField.Parser#parse(String, Map, Mapper.TypeParser.ParserContext)} calls - * {@link RuntimeField.Builder#parse(String, Mapper.TypeParser.ParserContext, Map)} and returns the corresponding - * {@link MappedFieldType}. */ - abstract class Builder extends FieldMapper.Builder { - final FieldMapper.Parameter> meta = FieldMapper.Parameter.metaParam(); + abstract class Builder implements ToXContent { + final String name; + final Parameter> meta = Parameter.metaParam(); + + private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(RuntimeField.class); protected Builder(String name) { - super(name); + this.name = name; } public Map meta() { return meta.getValue(); } - @Override - protected List> getParameters() { + protected List> getParameters() { return Collections.singletonList(meta); } - @Override - public FieldMapper.Builder init(FieldMapper initializer) { - throw new UnsupportedOperationException(); - } + protected abstract RuntimeField createRuntimeField(Mapper.TypeParser.ParserContext parserContext); @Override - public final FieldMapper build(ContentPath context) { - throw new UnsupportedOperationException(); + public final XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + boolean includeDefaults = params.paramAsBoolean("include_defaults", false); + for (Parameter parameter : getParameters()) { + parameter.toXContent(builder, includeDefaults); + } + return builder; } - protected abstract RuntimeField createRuntimeField(Mapper.TypeParser.ParserContext parserContext); - - private void validate() { - ContentPath contentPath = parentPath(name()); - FieldMapper.MultiFields multiFields = multiFieldsBuilder.build(this, contentPath); - if (multiFields.iterator().hasNext()) { - throw new IllegalArgumentException("runtime field [" + name + "] does not support [fields]"); + public final void parse(String name, Mapper.TypeParser.ParserContext parserContext, Map fieldNode) { + Map> paramsMap = new HashMap<>(); + for (Parameter param : getParameters()) { + paramsMap.put(param.name, param); } - FieldMapper.CopyTo copyTo = this.copyTo.build(); - if (copyTo.copyToFields().isEmpty() == false) { - throw new IllegalArgumentException("runtime field [" + name + "] does not support [copy_to]"); + String type = (String) fieldNode.remove("type"); + for (Iterator> iterator = fieldNode.entrySet().iterator(); iterator.hasNext();) { + Map.Entry entry = iterator.next(); + final String propName = entry.getKey(); + final Object propNode = entry.getValue(); + Parameter parameter = paramsMap.get(propName); + if (parameter == null) { + if (parserContext.isFromDynamicTemplate() && parserContext.indexVersionCreated().before(Version.V_8_0_0)) { + // The parameter is unknown, but this mapping is from a dynamic template. + // Until 7.x it was possible to use unknown parameters there, so for bwc we need to ignore it + deprecationLogger.deprecate(DeprecationCategory.API, propName, + "Parameter [{}] is used in a dynamic template mapping and has no effect on type [{}]. " + + "Usage will result in an error in future major versions and should be removed.", + propName, + type + ); + iterator.remove(); + continue; + } + throw new MapperParsingException( + "unknown parameter [" + propName + "] on mapper [" + name + "] of type [" + type + "]" + ); + } + if (propNode == null && parameter.canAcceptNull() == false) { + throw new MapperParsingException("[" + propName + "] on mapper [" + name + + "] of type [" + type + "] must not have a [null] value"); + } + parameter.parse(name, parserContext, propNode); + iterator.remove(); } } } @@ -121,7 +147,6 @@ RuntimeField parse(String name, Map node, Mapper.TypeParser.Pars RuntimeField.Builder builder = builderFunction.apply(name); builder.parse(name, parserContext, node); - builder.validate(); return builder.createRuntimeField(parserContext); } } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/AbstractScriptFieldTypeTestCase.java b/server/src/test/java/org/elasticsearch/index/mapper/AbstractScriptFieldTypeTestCase.java index fb3f05b85fa41..66e044650a801 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/AbstractScriptFieldTypeTestCase.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/AbstractScriptFieldTypeTestCase.java @@ -36,6 +36,7 @@ import java.util.Map; import java.util.function.BiConsumer; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.mockito.Matchers.anyString; import static org.mockito.Mockito.mock; @@ -106,7 +107,7 @@ public void testCopyToIsNotSupported() throws IOException { b.field("copy_to", "target"); }); MapperParsingException exception = expectThrows(MapperParsingException.class, () -> createMapperService(mapping)); - assertEquals("Failed to parse mapping: runtime field [field] does not support [copy_to]", exception.getMessage()); + assertThat(exception.getMessage(), containsString("unknown parameter [copy_to] on mapper")); } public void testMultiFieldsIsNotSupported() throws IOException { @@ -115,7 +116,7 @@ public void testMultiFieldsIsNotSupported() throws IOException { b.startObject("fields").startObject("test").field("type", "keyword").endObject().endObject(); }); MapperParsingException exception = expectThrows(MapperParsingException.class, () -> createMapperService(mapping)); - assertEquals("Failed to parse mapping: runtime field [field] does not support [fields]", exception.getMessage()); + assertThat(exception.getMessage(), containsString("unknown parameter [fields] on mapper")); } public void testStoredScriptsAreNotSupported() throws Exception { From a0abee4628cf6466d5781eb1336db8abdbaa1aab Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Mon, 7 Jun 2021 15:58:05 +0100 Subject: [PATCH 2/3] cleanup --- .../java/org/elasticsearch/index/mapper/RuntimeField.java | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RuntimeField.java b/server/src/main/java/org/elasticsearch/index/mapper/RuntimeField.java index be11e18cd42a9..7932886a1bea8 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RuntimeField.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RuntimeField.java @@ -61,9 +61,6 @@ default XContentBuilder toXContent(XContentBuilder builder, Params params) throw */ MappedFieldType asMappedFieldType(); - /** - * For runtime fields the {@link RuntimeField.Parser} returns directly the {@link MappedFieldType}. - */ abstract class Builder implements ToXContent { final String name; final Parameter> meta = Parameter.metaParam(); @@ -118,11 +115,11 @@ public final void parse(String name, Mapper.TypeParser.ParserContext parserConte continue; } throw new MapperParsingException( - "unknown parameter [" + propName + "] on mapper [" + name + "] of type [" + type + "]" + "unknown parameter [" + propName + "] on runtime field [" + name + "] of type [" + type + "]" ); } if (propNode == null && parameter.canAcceptNull() == false) { - throw new MapperParsingException("[" + propName + "] on mapper [" + name + throw new MapperParsingException("[" + propName + "] on runtime field [" + name + "] of type [" + type + "] must not have a [null] value"); } parameter.parse(name, parserContext, propNode); From b22257e4930891fb3b5215cf999d2ce9361fcdfc Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Wed, 16 Jun 2021 09:21:48 +0100 Subject: [PATCH 3/3] error message names --- .../index/mapper/AbstractScriptFieldTypeTestCase.java | 4 ++-- .../elasticsearch/index/mapper/RootObjectMapperTests.java | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/index/mapper/AbstractScriptFieldTypeTestCase.java b/server/src/test/java/org/elasticsearch/index/mapper/AbstractScriptFieldTypeTestCase.java index 66e044650a801..37351407608c9 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/AbstractScriptFieldTypeTestCase.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/AbstractScriptFieldTypeTestCase.java @@ -107,7 +107,7 @@ public void testCopyToIsNotSupported() throws IOException { b.field("copy_to", "target"); }); MapperParsingException exception = expectThrows(MapperParsingException.class, () -> createMapperService(mapping)); - assertThat(exception.getMessage(), containsString("unknown parameter [copy_to] on mapper")); + assertThat(exception.getMessage(), containsString("unknown parameter [copy_to] on runtime field")); } public void testMultiFieldsIsNotSupported() throws IOException { @@ -116,7 +116,7 @@ public void testMultiFieldsIsNotSupported() throws IOException { b.startObject("fields").startObject("test").field("type", "keyword").endObject().endObject(); }); MapperParsingException exception = expectThrows(MapperParsingException.class, () -> createMapperService(mapping)); - assertThat(exception.getMessage(), containsString("unknown parameter [fields] on mapper")); + assertThat(exception.getMessage(), containsString("unknown parameter [fields] on runtime field")); } public void testStoredScriptsAreNotSupported() throws Exception { diff --git a/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java index df12c6309468b..1a188f3f7cc35 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java @@ -376,7 +376,7 @@ public void testIllegalDynamicTemplateUnknownAttributeRuntime() throws Exception assertEquals("Failed to parse mapping: dynamic template [my_template] has invalid content [" + "{\"match_mapping_type\":\"string\",\"runtime\":{\"foo\":\"bar\",\"type\":\"keyword\"}}], " + "attempted to validate it with the following match_mapping_type: [string]", e.getMessage()); - assertEquals("unknown parameter [foo] on mapper [__dynamic__my_template] of type [keyword]", e.getRootCause().getMessage()); + assertEquals("unknown parameter [foo] on runtime field [__dynamic__my_template] of type [keyword]", e.getRootCause().getMessage()); } public void testIllegalDynamicTemplateInvalidAttribute() throws Exception { @@ -497,7 +497,7 @@ public void testIllegalDynamicTemplateNoMappingTypeRuntime() throws Exception { assertThat(e.getMessage(), containsString("Failed to parse mapping: dynamic template [my_template] has invalid content [")); assertThat(e.getMessage(), containsString("attempted to validate it with the following match_mapping_type: " + "[string, long, double, boolean, date]")); - assertEquals("unknown parameter [foo] on mapper [__dynamic__my_template] of type [date]", e.getRootCause().getMessage()); + assertEquals("unknown parameter [foo] on runtime field [__dynamic__my_template] of type [date]", e.getRootCause().getMessage()); } public void testIllegalDynamicTemplate7DotXIndex() throws Exception { @@ -680,7 +680,7 @@ public void testRuntimeSectionWrongFormat() throws IOException { public void testRuntimeSectionRemainingField() throws IOException { XContentBuilder mapping = runtimeFieldMapping(builder -> builder.field("type", "keyword").field("unsupported", "value")); MapperParsingException e = expectThrows(MapperParsingException.class, () -> createMapperService(mapping)); - assertEquals("Failed to parse mapping: unknown parameter [unsupported] on mapper [field] of type [keyword]", e.getMessage()); + assertEquals("Failed to parse mapping: unknown parameter [unsupported] on runtime field [field] of type [keyword]", e.getMessage()); } public void testTemplateWithoutMatchPredicates() throws Exception {