Skip to content

Add explicit tests for dynamic template validation on bad mapping parameters #74177

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jun 17, 2021
Merged

Add explicit tests for dynamic template validation on bad mapping parameters #74177

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4cd8755
Dynamic templates do not correctly validate runtime field mappings
romseygeek Jun 16, 2021
6f76c8e
Error out on bad runtime dynamic mappings in 7x
romseygeek Jun 16, 2021
6266d36
Remove unused deprecation logger
romseygeek Jun 16, 2021
a184bd4
automatically set dynamic context on new parser context and rename me…
romseygeek Jun 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions server/src/main/java/org/elasticsearch/index/mapper/DynamicFieldsBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ private static boolean applyMatchingTemplate(ParseContext context,
String mappingType = dynamicTemplate.mappingType(dynamicType);
Map<String, Object> mapping = dynamicTemplate.mappingForName(name, dynamicType);
if (dynamicTemplate.isRuntimeMapping()) {
Mapper.TypeParser.ParserContext parserContext = context.parserContext(dateFormatter);
Mapper.TypeParser.ParserContext parserContext = context.dynamicTemplateParserContext(dateFormatter);
RuntimeField.Parser parser = parserContext.runtimeFieldParser(mappingType);
String fullName = context.path().pathAsText(name);
if (parser == null) {
Expand Down Expand Up @@ -225,8 +225,7 @@ private static Mapper.Builder parseDynamicTemplateMapping(String name,
Map<String, Object> mapping,
DateFormatter dateFormatter,
ParseContext context) {
Mapper.TypeParser.ParserContext parserContext = context.parserContext(dateFormatter);
parserContext = parserContext.createDynamicTemplateFieldContext(parserContext);
Mapper.TypeParser.ParserContext parserContext = context.dynamicTemplateParserContext(dateFormatter);
Mapper.TypeParser typeParser = parserContext.typeParser(mappingType);
if (typeParser == null) {
throw new MapperParsingException("failed to find type parsed [" + mappingType + "] for [" + name + "]");
Expand Down
4 changes: 2 additions & 2 deletions server/src/main/java/org/elasticsearch/index/mapper/Mapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,8 +144,8 @@ ParserContext createMultiFieldContext(ParserContext in) {
return new MultiFieldParserContext(in);
}

ParserContext createDynamicTemplateFieldContext(ParserContext in) {
return new DynamicTemplateParserContext(in);
ParserContext createDynamicTemplateFieldContext() {
Copy link
Member

@javanna javanna Jun 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the only usage of the parser context function? should we change the function argument directly to create the dynamic parser context directly instead of going through a parser context to create another one?

Copy link
Contributor Author

@romseygeek romseygeek Jun 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also used when validating the templates where we do want to throw an error rather than issuing warnings, so we need to be able to distinguish between the two cases - hence the extra step.

Copy link
Member

@javanna javanna Jun 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok :)

return new DynamicTemplateParserContext(this);
}

private static class MultiFieldParserContext extends ParserContext {
Expand Down
10 changes: 5 additions & 5 deletions server/src/main/java/org/elasticsearch/index/mapper/ParseContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,8 +164,8 @@ public Iterable<Document> nonRootDocuments() {
}

@Override
public Mapper.TypeParser.ParserContext parserContext(DateFormatter dateFormatter) {
return in.parserContext(dateFormatter);
public Mapper.TypeParser.ParserContext dynamicTemplateParserContext(DateFormatter dateFormatter) {
return in.dynamicTemplateParserContext(dateFormatter);
}

@Override
Expand Down Expand Up @@ -336,8 +336,8 @@ public InternalParseContext(MappingLookup mappingLookup,
}

@Override
public Mapper.TypeParser.ParserContext parserContext(DateFormatter dateFormatter) {
return parserContextFunction.apply(dateFormatter);
public Mapper.TypeParser.ParserContext dynamicTemplateParserContext(DateFormatter dateFormatter) {
return parserContextFunction.apply(dateFormatter).createDynamicTemplateFieldContext();
}

@Override
Expand Down Expand Up @@ -548,7 +548,7 @@ public Collection<String> getFieldNames() {
*/
public abstract Collection<String> getFieldNames();

public abstract Mapper.TypeParser.ParserContext parserContext(DateFormatter dateFormatter);
public abstract Mapper.TypeParser.ParserContext dynamicTemplateParserContext(DateFormatter dateFormatter);

/**
* Return a new context that will be within a copy-to operation.
Expand Down
17 changes: 0 additions & 17 deletions server/src/main/java/org/elasticsearch/index/mapper/RuntimeField.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,6 @@

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;
Expand Down Expand Up @@ -67,8 +64,6 @@ abstract class Builder implements ToXContent {
final String name;
final Parameter<Map<String, String>> meta = Parameter.metaParam();

private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(RuntimeField.class);

protected Builder(String name) {
this.name = name;
}
Expand Down Expand Up @@ -104,18 +99,6 @@ public final void parse(String name, Mapper.TypeParser.ParserContext parserConte
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 runtime field [" + name + "] of type [" + type + "]"
);
Expand Down
79 changes: 79 additions & 0 deletions server/src/test/java/org/elasticsearch/index/mapper/DynamicTemplatesTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,14 @@
import org.apache.lucene.index.IndexOptions;
import org.apache.lucene.index.IndexableField;
import org.apache.lucene.util.BytesRef;
import org.elasticsearch.Version;
import org.elasticsearch.index.mapper.ParseContext.Document;
import org.elasticsearch.test.VersionUtils;

import java.io.IOException;

import static org.elasticsearch.test.StreamsUtils.copyToStringFromClasspath;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.notNullValue;

Expand Down Expand Up @@ -159,4 +164,78 @@ public void testSimpleWithXContentTraverse() throws Exception {
fieldMapper = mapperService.documentMapper().mappers().getMapper("multi2.org");
assertNotNull(fieldMapper);
}

public void testDynamicMapperWithBadMapping() throws IOException {
{
// in 7.x versions this will issue a deprecation warning
Version version = VersionUtils.randomCompatibleVersion(random(), Version.V_7_0_0);
DocumentMapper mapper = createDocumentMapper(version, topMapping(b -> {
b.startArray("dynamic_templates");
{
b.startObject();
{
b.startObject("test");
{
b.field("match_mapping_type", "string");
b.startObject("mapping").field("badparam", false).endObject();
}
b.endObject();
}
b.endObject();
}
b.endArray();
}));
assertWarnings(
"dynamic template [test] has invalid content [{\"match_mapping_type\":\"string\",\"mapping\":{\"badparam\":false}}], " +
"attempted to validate it with the following match_mapping_type: [string], last error: " +
"[unknown parameter [badparam] on mapper [__dynamic__test] of type [null]]");

mapper.parse(source(b -> b.field("field", "foo")));
assertWarnings("Parameter [badparam] is used in a dynamic template mapping and has no effect on type [null]. " +
"Usage will result in an error in future major versions and should be removed.");
}

{
// in 8.x it will error out
Exception e = expectThrows(MapperParsingException.class, () -> createMapperService(topMapping(b -> {
b.startArray("dynamic_templates");
{
b.startObject();
{
b.startObject("test");
{
b.field("match_mapping_type", "string");
b.startObject("mapping").field("badparam", false).endObject();
}
b.endObject();
}
b.endObject();
}
b.endArray();
})));
assertThat(e.getMessage(), containsString("dynamic template [test] has invalid content"));
assertThat(e.getCause().getMessage(), containsString("badparam"));
}
}

public void testDynamicRuntimeWithBadMapping() {
Exception e = expectThrows(MapperParsingException.class, () -> createMapperService(topMapping(b -> {
b.startArray("dynamic_templates");
{
b.startObject();
{
b.startObject("test");
{
b.field("match_mapping_type", "string");
b.startObject("runtime").field("badparam", false).endObject();
}
b.endObject();
}
b.endObject();
}
b.endArray();
})));
assertThat(e.getMessage(), containsString("dynamic template [test] has invalid content"));
assertThat(e.getCause().getMessage(), containsString("badparam"));
}
}
2 changes: 1 addition & 1 deletion server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ private static TestMapper fromMapping(String mapping, Version version, boolean f
throw new UnsupportedOperationException();
});
if (fromDynamicTemplate) {
pc = pc.createDynamicTemplateFieldContext(pc);
pc = pc.createDynamicTemplateFieldContext();
}
return (TestMapper) new TypeParser()
.parse("field", XContentHelper.convertToMap(JsonXContent.jsonXContent, mapping, true), pc)
Expand Down