Add validation for dynamic templates #51233

martijnvg · 2020-01-20T17:08:12Z

Today misconfiguration of dynamic templates are discovered when indexing a document with an unmapped field. With this change the misconfiguration will be known when creating an index.

The validation logic tries to load a Mapper instance for the mapping snippet of a dynamic template. This should catch things like using an analyzer that is undefined or mapping attributes that are unused.

This is best effort:

If {{name}} placeholder is used in the mapping snippet then validation is skipped.
If match_mapping_type is not specified then validation is performed for all mapping types. If parsing succeeds with a single mapping type then this the dynamic mapping is considered valid.

If is detected that a dynamic template mapping snippet is invalid at mapping update time then the mapping update is failed for indices created on 8.0.0-alpha1 and later. For indices created
on prior version a deprecation warning is emitted instead. In 7.x clusters the mapping update
will never fail in case of an invalid dynamic template mapping snippet and a deprecation warning will always be omitted.

Closes #17411
Closes #24419

elasticmachine · 2020-01-20T17:08:15Z

Pinging @elastic/es-search (:Search/Mapping)

elasticmachine · 2020-01-20T17:08:16Z

Pinging @elastic/es-core-features (:Core/Features/Indices APIs)

jpountz

I left some quick comments but the approach looks sensible to me.

jpountz · 2020-02-07T18:21:51Z

server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java

+    private static void validateDynamicTemplate(Mapper.TypeParser.ParserContext parserContext,
+                                                DynamicTemplate dynamicTemplate) {
+
+        if (containsSnippet(dynamicTemplate.getMapping(), "{name}")) {


do we need to check {dynamic_type} too?

Only {name}, because if the type hasn't specifically configured then line 360 ensures that all possible dynamic types are checked.

jpountz · 2020-02-07T18:26:24Z

server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java

@@ -621,6 +621,7 @@ private static void updateIndexMappingsAndBuildSortOrder(IndexService indexServi
        if (!mappings.isEmpty()) {
            assert mappings.size() == 1 : mappings;
            mapperService.merge(MapperService.SINGLE_MAPPING_NAME, mappings, MergeReason.MAPPING_UPDATE);
+            mapperService.validateDynamicTemplates();


This seems to only validate dynamic templates at creation time, should we move validation to MapperService#merge to make sure we cover all cases, including mapping updates that add dynamic templates on existing indices?

Good point. I will address this.

Dynamic template are now validated when parsed by root object type parser.
I think this has the same effect?

Tries to load a `Mapper.Builder` instance for the mapping snippet of a dynamic template. This should catch things like using a analyzer that is undefined or mapping attributes that are unused. This is best effort: * If `{{name}}` placeholder is used in the mapping snippet then validation is skipped. * If `match_mapping_type` is not specified then validation is performed for all mapping types. If parsing succeeds with a single mapping type then this the dynamic mapping is considered valid. This validation can be enabled on indices created on a master node with version 8.0.0 or higher. For 7.x a deprecation warning can be omitted instead. Closes elastic#17411 Closes elastic#24419

…e parsed. * Added tests and updated the documentation

jpountz

I left some minor comments but it looks good to me in general. I think that we need an important precision in the docs about the fact that we consider a template valid if there is at least one match_mapping_type that works, which means that there are still cases when we will try to create invalid mappings through dynamic templates?

docs/reference/mapping/dynamic/templates.asciidoc

jpountz · 2020-02-14T20:45:23Z

server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java

+            Map<String, Object> fieldTypeConfig = dynamicTemplate.mappingForName("__dummy__", defaultDynamicType);
+            fieldTypeConfig.remove("type");
+            try {
+                Mapper.Builder<?, ?> dummyBuilder = typeParser.parse("__dummy__", fieldTypeConfig, parserContext);


I think some fields might do extra validation when calling builder.build(), should we try to build the mapper too?

Good point. I will try to do this here.

Co-Authored-By: Adrien Grand <jpountz@gmail.com>

martijnvg · 2020-02-24T06:38:02Z

Thanks for reviewing @jpountz.

I think that we need an important precision in the docs about the fact that we consider a template valid if there is at least one match_mapping_type that works, which means that there are still cases when we will try to create invalid mappings through dynamic templates?

True. I will word the docs differently to reflect this.

that occurs during construction of a mapper instance is executed.

…pping update time.

jpountz

LGTM

Tries to load a `Mapper` instance for the mapping snippet of a dynamic template. This should catch things like using an analyzer that is undefined or mapping attributes that are unused. This is best effort: * If `{{name}}` placeholder is used in the mapping snippet then validation is skipped. * If `match_mapping_type` is not specified then validation is performed for all mapping types. If parsing succeeds with a single mapping type then this the dynamic mapping is considered valid. If is detected that a dynamic template mapping snippet is invalid at mapping update time then the mapping update is failed for indices created on 8.0.0-alpha1 and later. For indices created on prior version a deprecation warning is omitted instead. In 7.x clusters the mapping update will never fail in case of an invalid dynamic template mapping snippet and a deprecation warning will always be omitted. Closes elastic#17411 Closes elastic#24419 Co-authored-by: Adrien Grand <jpountz@gmail.com>

Backport of #51233 to the seven dot x branch. Tries to load a `Mapper` instance for the mapping snippet of a dynamic template. This should catch things like using an analyzer that is undefined or mapping attributes that are unused. This is best effort: * If `{{name}}` placeholder is used in the mapping snippet then validation is skipped. * If `match_mapping_type` is not specified then validation is performed for all mapping types. If parsing succeeds with a single mapping type then this the dynamic mapping is considered valid. If is detected that a dynamic template mapping snippet is invalid at mapping update time then the mapping update is failed for indices created on 8.0.0-alpha1 and later. For indices created on prior version a deprecation warning is omitted instead. In 7.x clusters the mapping update will never fail in case of an invalid dynamic template mapping snippet and a deprecation warning will always be omitted. Closes #17411 Closes #24419 Co-authored-by: Adrien Grand <jpountz@gmail.com>

Include the attempted 'match_mapping_type' into the message, so that it is clearer that multiple validation attempts have occurred. Dynamic template validation was recently added via elastic#51233 and there was some confusion over the deprecation message itself. (in 7.x only deprecation warning will be omitted and from 8.0 an error will be returned)

Include the attempted 'match_mapping_type' into the message, so that it is clearer that multiple validation attempts have occurred. Dynamic template validation was recently added via #51233 and there was some confusion over the deprecation message itself. (in 7.x only deprecation warning will be omitted and from 8.0 an error will be returned)

Backporting elastic#60870 to 7.x branch. Include the attempted 'match_mapping_type' into the message, so that it is clearer that multiple validation attempts have occurred. Dynamic template validation was recently added via elastic#51233 and there was some confusion over the deprecation message itself. (in 7.x only deprecation warning will be omitted and from 8.0 an error will be returned)

Backporting #60870 to 7.x branch. Include the attempted 'match_mapping_type' into the message, so that it is clearer that multiple validation attempts have occurred. Dynamic template validation was recently added via #51233 and there was some confusion over the deprecation message itself. (in 7.x only deprecation warning will be omitted and from 8.0 an error will be returned)

martijnvg added WIP :Search Foundations/Mapping Index mappings, including merging and defining field types :Data Management/Indices APIs APIs to create and manage indices and templates labels Jan 20, 2020

jpountz reviewed Feb 7, 2020

View reviewed changes

martijnvg added 4 commits February 11, 2020 09:36

moved validation logic to a better place

ca7ed5a

iter

d047b8f

* Moved validation of dynamic templates to where dynamic templates ar…

a8c1079

…e parsed. * Added tests and updated the documentation

martijnvg force-pushed the validate_dynamic_templates branch from b8b04df to a8c1079 Compare February 14, 2020 10:55

martijnvg added v7.7.0 v8.0.0 >enhancement and removed WIP labels Feb 14, 2020

martijnvg marked this pull request as ready for review February 14, 2020 10:57

martijnvg requested a review from jpountz February 14, 2020 10:57

Merge remote-tracking branch 'es/master' into validate_dynamic_templates

a63f193

jpountz reviewed Feb 21, 2020

View reviewed changes

fix typo in docs

dbce91c

Co-Authored-By: Adrien Grand <jpountz@gmail.com>

martijnvg added 3 commits February 26, 2020 09:02

Merge remote-tracking branch 'es/master' into validate_dynamic_templates

c5df937

Also build the actual mapper instance to ensure that validation

28f6d5e

that occurs during construction of a mapper instance is executed.

adjusted the wording about when and how a template is validated at ma…

a4ccb67

…pping update time.

martijnvg requested a review from jpountz February 26, 2020 10:05

jpountz approved these changes Feb 26, 2020

View reviewed changes

Merge remote-tracking branch 'es/master' into validate_dynamic_templates

44fdd1e

martijnvg added the >breaking label Feb 27, 2020

martijnvg merged commit 31b2987 into elastic:master Feb 27, 2020

martijnvg added the backport pending label Feb 27, 2020

martijnvg mentioned this pull request Feb 27, 2020

Backport: Add validation for dynamic templates #52890

Merged

martijnvg removed the backport pending label Feb 28, 2020

codebrain mentioned this pull request Apr 1, 2020

7.7.0 meta ticket elastic/elasticsearch-net#4525

Closed

38 tasks

martijnvg mentioned this pull request Aug 10, 2020

Improve error message in case of invalid dynamic templates #60870

Merged

martijnvg mentioned this pull request Nov 12, 2020

Improve error message in case of invalid dynamic templates. #65001

Merged

jakelandis mentioned this pull request Feb 22, 2021

DRAFT [META] REST Compatible API V7 completeness #68905

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

masseyke mentioned this pull request Aug 9, 2021

Update Deprecation Info API checks for 8.0 #42404

Closed

80 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add validation for dynamic templates #51233

Add validation for dynamic templates #51233

martijnvg commented Jan 20, 2020 •

edited by bpintea

Loading

elasticmachine commented Jan 20, 2020

elasticmachine commented Jan 20, 2020

jpountz left a comment

jpountz Feb 7, 2020

martijnvg Feb 10, 2020 •

edited

Loading

jpountz Feb 7, 2020

martijnvg Feb 10, 2020

martijnvg Feb 14, 2020

jpountz left a comment

jpountz Feb 14, 2020

martijnvg Feb 24, 2020

martijnvg commented Feb 24, 2020

jpountz left a comment

Add validation for dynamic templates #51233

Add validation for dynamic templates #51233

Conversation

martijnvg commented Jan 20, 2020 • edited by bpintea Loading

elasticmachine commented Jan 20, 2020

elasticmachine commented Jan 20, 2020

jpountz left a comment

Choose a reason for hiding this comment

jpountz Feb 7, 2020

Choose a reason for hiding this comment

martijnvg Feb 10, 2020 • edited Loading

Choose a reason for hiding this comment

jpountz Feb 7, 2020

Choose a reason for hiding this comment

martijnvg Feb 10, 2020

Choose a reason for hiding this comment

martijnvg Feb 14, 2020

Choose a reason for hiding this comment

jpountz left a comment

Choose a reason for hiding this comment

jpountz Feb 14, 2020

Choose a reason for hiding this comment

martijnvg Feb 24, 2020

Choose a reason for hiding this comment

martijnvg commented Feb 24, 2020

jpountz left a comment

Choose a reason for hiding this comment

martijnvg commented Jan 20, 2020 •

edited by bpintea

Loading

martijnvg Feb 10, 2020 •

edited

Loading