Disallow "enabled" attribute change for types in mapping update (#33566) by cbismuth · Pull Request #33933 · elastic/elasticsearch

cbismuth · 2018-09-21T08:34:32Z

This commit adds a check for enabled attribute change for types when a RestPutMappingAction is received. A MappingException is thrown when such a change is detected.

Change are prevented in both ways: false to true and true to false.

Here is a sample cURL output:

{
  "error": {
    "root_cause": [
      {
        "type": "mapper_exception",
        "reason": "Can't update attribute for type [type1.foo.enabled] in index mapping"
      }
    ],
    "type": "mapper_exception",
    "reason": "Can't update attribute for type [type1.foo.enabled] in index mapping"
  },
  "status": 500
}

Tests are not part of this commit, they will be developed after design validation.

This commit adds a check for "enabled" attribute change for types when a RestPutMappingAction is received. A MappingException is thrown when such a change is detected. Change are prevented in both ways: "false -> true" and "true -> false". Tests are not part of this commit, they will be developed after design validation.

elasticmachine · 2018-09-21T11:10:08Z

Pinging @elastic/es-search-aggs

cbuescher

HI @cbismuth thanks for looking into #33566. I left a few very small comments and a questions. In general I think adding a few tests now would help understand which cases this fix covers, but generally I think the approach looks good.

cbuescher · 2018-09-21T11:16:23Z

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


+    public MapperException(String msg, Object... args) {
+        super(msg, args);
+    }


nit: not sure we need another constructor here, can you just insert the args into the message String in the call?

Sure.
Is LoggerMessageFormat#format preferred over String#format or maybe just string concatenation?

cbuescher · 2018-09-21T11:17:25Z

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


        for (Mapper mergeWithMapper : mergeWith) {
            Mapper mergeIntoMapper = mappers.get(mergeWithMapper.simpleName());
+            throwExceptionIfEnabledAttributeIsUpdatedForType(mergeWith, mergeWithMapper, mergeIntoMapper);


nit: ImNotABigFanOfOverlyLongMethodNamesButWhatTheHeckIfYouReallyLikeIt ;-)

I'll shorten it, I'm not a big fan neither.
As I wasn't sure of the suggested design I've chosen (very) (very) explicit name to make myself understood.

cbuescher · 2018-09-21T11:28:39Z

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

+            final ObjectMapper mergeWithObjectMapper = (ObjectMapper) mergeWithMapper;
+
+            if (mergeIntoObjectMapper.isEnabled() != mergeWithObjectMapper.isEnabled()) {
+                if (mergeIntoObjectMapper.nestedTypeFilter() instanceof TermQuery) {


I'm not quite sure I understand what the enable setting in mappings that are merged have to do with nested queries, especially with TermQuery. Maybe I'm missing sth., could you explain?

I dug into this with a debugger and the _type attribute happened to be wrapped into a TermQuery.

I've been a (tiny) bit paranoiac as I wanted to be sure to not make other things break.

I'll have another debugging session to see how things work when updating mapping with "_source": { "enabled": false }. I'll let you know.

Well, it looks like updating the _source attribute in a mapping is not allowed.

command

curl -XPUT "${ES_CLUSTER}/index1/_mapping/type1" -H "Content-Type: application/json" --data '{ "properties": { "foo": { "source": { "enabled": false }, "properties": { "bar": { "type": "text" } } } } }'

output

{ "error": { "root_cause": [ { "type": "mapper_parsing_exception", "reason": "Mapping definition for [foo] has unsupported parameters: [source : {enabled=false}]" } ], "type": "mapper_parsing_exception", "reason": "Mapping definition for [foo] has unsupported parameters: [source : {enabled=false}]" }, "status": 400 }

I'll now have a look if removing this conditional block doesn't conflict with other enabled-like metadata attributes.

No gradlew check -p server test failure after removing paranoiac conditional block 👍

cbismuth · 2018-09-21T14:14:57Z

Thank you @cbuescher 👍 I'll update this PR with your comments and add test cases.

cbismuth · 2018-09-24T11:09:49Z

Hi @cbuescher, I've added test cases to ensure added behavior is covered.

cbuescher

Hi @cbismuth, thanks a lot for the update, the test looks good and I left only a few very minor comments.

cbuescher · 2018-09-27T15:10:33Z

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

+
+            if (mergeIntoObjectMapper.isEnabled() != mergeWithObjectMapper.isEnabled()) {
+                final String path = mergeWith.fullPath() + "." + mergeWithObjectMapper.simpleName() + ".enabled";
+


nit: maybe delete this line

Yes, these two lines are related, no need to add a blank line, I'll remove it.

server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java

cbuescher · 2018-09-27T15:20:27Z

server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java

+    private static Explicit<Boolean> dateDetection = new Explicit<>(false, false);
+    private static Explicit<Boolean> numericDetection = new Explicit<>(false, false);
+    private static Explicit<FormatDateTimeFormatter[]> dynamicDateTimeFormatters = new Explicit<>(new FormatDateTimeFormatter[0], false);
+    private static Explicit<DynamicTemplate[]> dynamicTemplates = new Explicit<>(new DynamicTemplate[0], false);


I was a bit suprised by these until I saw them being used in createRootObjectMapper(), maybe you could just instantiate them there locally, even if that means a few more instances. I think it makes the test more readable although this way is certainly "better" in terms of object creation.

Yes, you're right. It would be a better balance between performances and readability. I'll do it, thank you.

cbismuth · 2018-09-27T16:45:28Z

Thank you @cbuescher for your review. I've added comments to your comments and I'll update my PR very soon.
Even with minor comments, it's always interesting to see how pieces of code are read, thank you for your time.

cbismuth · 2018-09-28T08:21:35Z

PR updated with your comments, thank you @cbuescher.

cbuescher

I left two more small comment, but generally this looks almost ready.

cbuescher · 2018-09-28T10:26:39Z

server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java

+
+    private static TextFieldMapper createTextFieldMapper(String name) {
+        final TextFieldType fieldType = new TextFieldType();
+        final Settings indexSettings = Settings.builder().put(SETTING_VERSION_CREATED, "1").build();


nit: I guess it doesn't matter here too much, but normally one of the version constants from org.elasticsearch.Version is used in these cases, usually Version.CURRENT

Yes, that's better as it's closer to production code.

cbuescher · 2018-09-28T10:45:03Z

server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java

+            dynamicDateTimeFormatters, dynamicTemplates,
+            dateDetection, numericDetection,
+            Settings.EMPTY
+        );


nit: I'm not too familiar with mapper creation (especially in tests) myself, but I saw that the RootObjectMapper.Builder already takes care of a lot of the defaults, so maybe this can be simplified to something like:

final Settings indexSettings = Settings.builder().put(SETTING_VERSION_CREATED, Version.CURRENT).build(); BuilderContext context = new BuilderContext(indexSettings, new ContentPath()); RootObjectMapper rootObjectMapper = new RootObjectMapper.Builder(fieldName).enabled(enabled).build(context); mappers.values().stream().forEach(rootObjectMapper::putMapper); return rootObjectMapper;

Or maybe even the dummy settings could be a test-wide constant.

I don't think this is necesarry but removes a bit of boilerplate code (which I admit testing the mappers seems to require quite a bit).

Yes, it's far more better, builders are far more expressive than lengthy constructors.

cbismuth · 2018-09-28T12:32:29Z

Thank you @cbuescher for your attention to details, I know it can be very time consuming.

PR is up-to-date with your comments 👍

cbuescher

@cbismuth thanks, this looks good to me. I will kick of final CI test runs now, hope they pass in one go 🤞
@elasticmachine test this please

cbuescher · 2018-09-28T12:38:17Z

@elasticmachine test this please

cbismuth · 2018-09-28T12:41:51Z

Great! Thanks @cbuescher!

cbismuth · 2018-09-29T16:57:49Z

runbld>>> Test output logs contained: Errors: 3 Failures: 72 Tests: 17654 Skipped: 297

That's a lot of failures! I've downloaded the plain text output log and I'll replay failed tests on my laptop on Monday morning.

Anyway, number of them seem to have the same stash dump, here is a sample below.

  1> [2018-09-28T10:53:55,638][INFO ][o.e.b.MixedClusterClientYamlTestSuiteIT] [test] Stash dump on test failure [{
  1>   "stash" : {
  1>     "body" : {
  1>       "_shards" : {
  1>         "total" : 40,
  1>         "successful" : 32,
  1>         "failed" : 2,
  1>         "failures" : [
  1>           {
  1>             "shard" : 2,
  1>             "index" : "test-empty",
  1>             "status" : "INTERNAL_SERVER_ERROR",
  1>             "reason" : {
  1>               "type" : "illegal_state_exception",
  1>               "reason" : "Message not fully read (request) for requestId [3973], action [indices:admin/refresh[s][r]], available [0]; resetting"
  1>             }
  1>           },
  1>           {
  1>             "shard" : 1,
  1>             "index" : "test-unmapped",
  1>             "status" : "INTERNAL_SERVER_ERROR",
  1>             "reason" : {
  1>               "type" : "illegal_state_exception",
  1>               "reason" : "Message not fully read (request) for requestId [3976], action [indices:admin/refresh[s][r]], available [0]; resetting"
  1>             }
  1>           }
  1>         ]
  1>       }
  1>     }
  1>   }
  1> }]

jasontedor · 2018-09-29T17:18:48Z

@cbismuth That's from BWC issues that can happen when another change is merged to 6.x. Those should go away if you merge master into your branch.

cbismuth · 2018-10-01T19:01:30Z

Thank you @cbuescher 👍

Would you like me to test it on a 6.5 branch and let you know?

cbuescher · 2018-10-01T19:34:50Z

@cbismuth thanks, I think it should be a straigtforward backport, the question is more if this is breaking users applications even though the feature never worked as intended. But throwing an error is different than silently ignoring. It could be considered a bugfix though, in that case if would justify the backport. But I'll see what others think.

cbismuth · 2018-10-01T19:47:50Z

It is indeed a good question 😉

cbuescher · 2018-10-02T10:31:23Z

@cbismuth I got some feedback that it would probably better not to backport the new strict checking since it would be considered breaking. However, it would be nice to change the PR slightly so that instead of throwing and error, a deprecation warning is emitted warning users that their request was silently ignored and we will throw an error here in the next major version. I will open a separate issue for this, it would be great if you would like to adapt this PR slightly to address it, but its also no problem if you don't like to do that. There a only a few thing that need changing for this I think, I can give you directions if you like to work on this as well.

cbuescher · 2018-10-02T13:03:31Z

@cbismuth I have to correct myself, the general opinion shifted to backporting this change to 6.5 and add it to the breaking changes log. I will do this with the backport.

This commit adds a check for "enabled" attribute change for types when a RestPutMappingAction is received. A MappingException is thrown when such a change is detected. Change are prevented in both ways: "false -> true" and "true -> false". Closes #33566

cbismuth · 2018-10-02T16:38:08Z

That's fine, thank you @cbuescher.

This commit adds a check for "enabled" attribute change for types when a RestPutMappingAction is received. A MappingException is thrown when such a change is detected. Change are prevented in both ways: "false -> true" and "true -> false". Closes #33566

Removed the invalid tip that enabled can be updated for existing fields and clarified instead that it cannot. Related to elastic#33566 and elastic#33933

Removed the invalid tip that enabled can be updated for existing fields and clarified instead that it cannot. Related to #33566 and #33933

In #33933 we disallowed changing the `enabled` parameter in object mappings. However, the fix didn't cover the root object mapper. This PR adjusts the change to also include the root mapper and clarifies the error message.

cbismuth added 2 commits September 21, 2018 10:31

Rename method in which mapping changes are resolved (#33566)

7e6cb90

cbismuth mentioned this pull request Sep 21, 2018

The enabled setting is not updatable (and probably shouldn't) #33566

Closed

cbuescher added the :Search Foundations/Mapping Index mappings, including merging and defining field types label Sep 21, 2018

cbuescher reviewed Sep 21, 2018

View reviewed changes

cbuescher added >enhancement v7.0.0 labels Sep 21, 2018

cbismuth added 6 commits September 22, 2018 12:47

Remove unnecessary type conditions (#33566)

ce31c85

Remove unnecessary mapper exception constructor (#33566)

696b9a3

Shorten "check 'enabled' field change" method name (#33566)

c1db69e

Revert unnecessary Git change (#33566)

e4458be

Private method can be static (#33566)

02b50dd

Add test cases to object mapper merge operations (#33566)

df57bbb

cbuescher reviewed Sep 27, 2018

View reviewed changes

cbismuth added 2 commits September 28, 2018 10:14

Delete unnecessary blank line (#33566)

1a890a4

Move Explicit allocations into method to ease readability (#33566)

51feaae

cbuescher reviewed Sep 28, 2018

View reviewed changes

cbismuth added 2 commits September 28, 2018 13:59

Use current version in index settings (#33566)

1489108

Prefer mapper builders over constructors (#33566)

760b6cc

cbuescher approved these changes Sep 28, 2018

View reviewed changes

cbuescher merged commit 2923fb5 into elastic:master Oct 1, 2018

cbuescher added the backport pending label Oct 1, 2018

cbuescher mentioned this pull request Oct 2, 2018

Emit deprecation warning on "enabled" attribute change #34219

Closed

cbuescher added >breaking v6.5.0 and removed backport pending labels Oct 2, 2018

dnhatn mentioned this pull request Oct 2, 2018

Unable to change enabled setting when update mapping #28070

Closed

cbismuth deleted the 33566_disallow_type_enabled_mapping_update branch October 25, 2018 17:39

Mpdreamz mentioned this pull request Dec 13, 2018

[meta] 6.5.0 Release elastic/elasticsearch-net#3457

Closed

droberts195 mentioned this pull request Jan 18, 2019

[ML] Update index mappings on process start, not job open #37607

Closed

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

henningandersen mentioned this pull request Jun 27, 2019

Enabled cannot be updated #43701

Merged

henningandersen added a commit that referenced this pull request Jun 28, 2019

Enabled cannot be updated (#43701)

b92de28

Removed the invalid tip that enabled can be updated for existing fields and clarified instead that it cannot. Related to #33566 and #33933

henningandersen added a commit that referenced this pull request Jun 28, 2019

Enabled cannot be updated (#43701)

632da7f

Removed the invalid tip that enabled can be updated for existing fields and clarified instead that it cannot. Related to #33566 and #33933

henningandersen added a commit that referenced this pull request Jun 28, 2019

Enabled cannot be updated (#43701)

19ee372

Removed the invalid tip that enabled can be updated for existing fields and clarified instead that it cannot. Related to #33566 and #33933

henningandersen added a commit that referenced this pull request Jun 28, 2019

Enabled cannot be updated (#43701)

4f517b4

Removed the invalid tip that enabled can be updated for existing fields and clarified instead that it cannot. Related to #33566 and #33933

jtibshirani mentioned this pull request Mar 30, 2020

Disallow changing 'enabled' on the root mapper. #54463

Merged


		if (mergeIntoObjectMapper.isEnabled() != mergeWithObjectMapper.isEnabled()) {
		final String path = mergeWith.fullPath() + "." + mergeWithObjectMapper.simpleName() + ".enabled";

Conversation

cbismuth commented Sep 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Sep 21, 2018

Uh oh!

cbuescher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbismuth commented Sep 21, 2018

Uh oh!

cbismuth commented Sep 24, 2018

Uh oh!

cbuescher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbismuth commented Sep 27, 2018

Uh oh!

cbismuth commented Sep 28, 2018

Uh oh!

cbuescher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbismuth commented Sep 28, 2018

Uh oh!

cbuescher left a comment

Choose a reason for hiding this comment

Uh oh!

cbuescher commented Sep 28, 2018

Uh oh!

cbismuth commented Sep 28, 2018

Uh oh!

cbismuth commented Sep 29, 2018

Uh oh!

jasontedor commented Sep 29, 2018

Uh oh!

cbismuth commented Oct 1, 2018

Uh oh!

cbuescher commented Oct 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cbismuth commented Oct 1, 2018

Uh oh!

cbuescher commented Oct 2, 2018

Uh oh!

cbuescher commented Oct 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

cbismuth commented Sep 21, 2018 •

edited

Loading

cbuescher commented Oct 1, 2018 •

edited

Loading

cbuescher commented Oct 2, 2018 •

edited

Loading