Disable date field mapping changing by PnPie · Pull Request #25285 · elastic/elasticsearch

PnPie · 2017-06-18T21:02:45Z

Disable date field changing in mapping.

I also modified a little DocumentMapperMergeTests in format.

Make date field mapping unchangeable. Closes elastic#25271

elasticmachine · 2017-06-18T21:02:47Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

jpountz

Thanks for picking this one up! I left some minor comments.

jpountz · 2017-06-22T11:14:37Z

core/src/test/java/org/elasticsearch/index/mapper/DocumentMapperMergeTests.java

        assertThat(e.getMessage(), containsString("The _parent field's type option can't be changed: [null]->[parent]"));
    }
+
+    public void testMergeDate() throws IOException {


Can you move this test to DateFieldMapperTests instead and undo changes to this file?

jpountz · 2017-06-22T11:16:07Z

core/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java

    protected void doMerge(Mapper mergeWith, boolean updateAllTypes) {
+        final DateFieldMapper other = (DateFieldMapper) mergeWith;
+        if (!this.fieldType().equals(other.fieldType()))
+            throw new IllegalArgumentException("date field cannot be updated");


Could you only check the format property and make the error message more specific?

it is currently a bit confusing since eg. ignore_malformed can be updated

Thx for your comments, I modified the error message and specified it's date field's format which cannot be updated. And the fieldType()'s equals() method has been re-implemented to just compare the format of a Date, isn't it ?

it also checks for other properties through the super.equals call

jpountz · 2017-06-22T11:16:25Z

core/src/test/java/org/elasticsearch/index/mapper/DocumentMapperMergeTests.java

+            .endObject().endObject().endObject().string();
+        DocumentMapper updateFormatMapper = parser.parse("movie", new CompressedXContent(updateFormatMapping));
+
+        Exception e = expectThrows(IllegalArgumentException.class, () -> initMapper.merge(updateFormatMapper.mapping(), false));


++, thanks for using expectThrows

jpountz · 2017-06-22T13:47:58Z

Sorry but I just realized there is a better way of doing that, which is by changing DateFieldType.checkCompatibility and moving the format check out of the if (strict) block (and changing the conflict message). This is how we generally ensure that some options cannot change. And compare to your way of doing things, it also does the right thing when multiple types have a field with the same name by requiring that they all have the same format. You'll probably need to modify DateFieldTypeTests to pass false to strictOnly instead of true. By the way, we should do the same for the locale parameter since it affects date parsing as well.

PnPie · 2017-06-23T08:30:24Z

Yes I noticed that we also did this in ParentFieldMapper, so I retrieved both format and locale check from if (strict) block and modified the error message like in super class.
For ParentFieldMapper I put the checkCompatibility before super.doMerge because the super.doMerge copies target fieldType to the current one so the check should be done before I think ?

jpountz · 2017-07-04T08:46:50Z

@PnPie Sorry for the late reply, I was absent lately. Your PR looks good but I think it is not necessary to call checkComtpatibility as part of doMerge as it is checked higher up in the stack by MapperService. If you remove this call, your tests should still pass.

PnPie · 2017-07-05T20:43:45Z

I deleted the checkCompatibility call in DateFieldMapper but the testMergeDate didn't pass it said no expected exception thrown.
And I've had a look and it seems that checkCompatibility hasn't been called in higher level ? And in MapperService I found checkObjectsCompatibility but it's for ObjectMapper and it just did a merge to see if there are exceptions.

jpountz · 2017-07-06T07:51:11Z

I think you need to change the test to call MapperService.merge which performs this check:

diff --git a/core/src/test/java/org/elasticsearch/index/mapper/DateFieldMapperTests.java b/core/src/test/java/org/elasticsearch/index/mapper/DateFieldMapperTests.java
index 448538b..fac4e7c 100644
--- a/core/src/test/java/org/elasticsearch/index/mapper/DateFieldMapperTests.java
+++ b/core/src/test/java/org/elasticsearch/index/mapper/DateFieldMapperTests.java
@@ -25,6 +25,7 @@ import org.elasticsearch.common.compress.CompressedXContent;
 import org.elasticsearch.common.xcontent.XContentFactory;
 import org.elasticsearch.common.xcontent.XContentType;
 import org.elasticsearch.index.IndexService;
+import org.elasticsearch.index.mapper.MapperService.MergeReason;
 import org.elasticsearch.plugins.Plugin;
 import org.elasticsearch.test.ESSingleNodeTestCase;
 import org.elasticsearch.test.InternalSettingsPlugin;
@@ -352,7 +353,8 @@ public class DateFieldMapperTests extends ESSingleNodeTestCase {
             .startObject("properties")
             .startObject("release_date").field("type", "date").field("format", "yyyy/MM/dd").endObject()
             .endObject().endObject().endObject().string();
-        DocumentMapper initMapper = parser.parse("movie", new CompressedXContent(initMapping));
+        DocumentMapper initMapper = indexService.mapperService().merge("movie", new CompressedXContent(initMapping),
+                MergeReason.MAPPING_UPDATE, randomBoolean());
 
         assertThat(initMapper.mappers().getMapper("release_date"), notNullValue());
         assertFalse(initMapper.mappers().getMapper("release_date").fieldType().stored());
@@ -361,9 +363,11 @@ public class DateFieldMapperTests extends ESSingleNodeTestCase {
             .startObject("properties")
             .startObject("release_date").field("type", "date").field("format", "epoch_millis").endObject()
             .endObject().endObject().endObject().string();
-        DocumentMapper updateFormatMapper = parser.parse("movie", new CompressedXContent(updateFormatMapping));
 
-        Exception e = expectThrows(IllegalArgumentException.class, () -> initMapper.merge(updateFormatMapper.mapping(), false));
+
+        Exception e = expectThrows(IllegalArgumentException.class,
+                () -> indexService.mapperService().merge("movie", new CompressedXContent(updateFormatMapping),
+                        MergeReason.MAPPING_UPDATE, randomBoolean()));
         assertThat(e.getMessage(), containsString("Merge conflits: [mapper [release_date] has different [format] values]"));
     }
 }

PnPie · 2017-07-06T13:07:21Z

@jpountz Sorry I saw that the checkCompatibility was called in MapperService's merge method by calling FieldTypeLookup's same method. I removed it in doMerge.

jpountz

No need to be sorry! It looks good, I'll check that tests pass and merge. Thanks!

jpountz · 2017-07-06T13:47:15Z

@elasticmachine please test it

jpountz · 2017-07-07T09:48:53Z

The test failure looks unrelated so I'll merge. Thanks @PnPie!

Make date field mapping unchangeable. Closes #25271

* master: (42 commits) Harden global checkpoint tracker Remove deprecated created and found from index, delete and bulk (elastic#25516) fix testEnsureVersionCompatibility for 5.5.0 release fix Version.v6_0_0 min compatibility version to 5.5.0 Add bwc indices for 5.5.0 Add v5_5_1 constant [DOCS] revise high level client Search Scroll API docs (elastic#25599) Improve REST error handling when endpoint does not support HTTP verb, add OPTIONS support (elastic#24437) Avoid SecurityException in repository-S3 on DefaultS3OutputStream.flush() (elastic#25254) [Tests] Add tests for CompletionSuggestionBuilder#build() (elastic#25575) Enable cross-setting validation [Docs] Fix typo in bootstrap-checks.asciidoc (elastic#25597) Index ids in binary form. (elastic#25352) bwc checkout should fetch from all remotes IndexingIT should check for global checkpoints regardless of master version [Tests] Add tests for PhraseSuggestionBuilder#build() (elastic#25571) Remove unused class MinimalMap (elastic#25590) [Docs] Document Scroll API for Java High Level REST Client (elastic#25554) Disable date field mapping changing (elastic#25285) Allow BWC Testing against a specific branch (elastic#25510) ...

With this commit we remove a leftover in the docs about the `format` field being updateable. This is not true since we removed support for updates in elastic#25285. Closes elastic#33986 Relates elastic#25285

With this commit we remove a leftover in the docs about the `format` field being updatable. This is not true since we removed support for updates in #25285. Closes #33986 Relates #25285 Relates #34006

Disable date field mapping changing

0caed2b

Make date field mapping unchangeable. Closes elastic#25271

jasontedor requested a review from jpountz June 18, 2017 21:06

jpountz suggested changes Jun 22, 2017

View reviewed changes

modification

7712a3a

apply checkCompatibility for DateFieldType doMerge

f9b5e88

remove checkCompatibility for doMerge

7e09855

jpountz approved these changes Jul 6, 2017

View reviewed changes

jpountz merged commit 2e5e451 into elastic:master Jul 7, 2017

jpountz added :Search Foundations/Mapping Index mappings, including merging and defining field types >bug v5.6.0 v6.0.0 labels Jul 7, 2017

jpountz pushed a commit that referenced this pull request Jul 7, 2017

Disable date field mapping changing (#25285)

5dee640

Make date field mapping unchangeable. Closes #25271

clintongormley added v6.0.0-beta1 and removed v6.0.0 labels Jul 25, 2017

danielmitterdorfer mentioned this pull request Sep 24, 2018

Can't change date format on existing properties #33986

Closed

danielmitterdorfer mentioned this pull request Sep 24, 2018

Remove leftover doc about format being updateable #34006

Merged

Conversation

PnPie commented Jun 18, 2017

Uh oh!

elasticmachine commented Jun 18, 2017

Uh oh!

jpountz left a comment

Choose a reason for hiding this comment

Uh oh!

jpountz Jun 22, 2017

Choose a reason for hiding this comment

Uh oh!

jpountz Jun 22, 2017

Choose a reason for hiding this comment

Uh oh!

jpountz Jun 22, 2017

Choose a reason for hiding this comment

Uh oh!

PnPie Jun 22, 2017

Choose a reason for hiding this comment

Uh oh!

jpountz Jun 22, 2017

Choose a reason for hiding this comment

Uh oh!

jpountz Jun 22, 2017

Choose a reason for hiding this comment

Uh oh!

jpountz commented Jun 22, 2017

Uh oh!

PnPie commented Jun 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jpountz commented Jul 4, 2017

Uh oh!

PnPie commented Jul 5, 2017

Uh oh!

jpountz commented Jul 6, 2017

Uh oh!

PnPie commented Jul 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jpountz left a comment

Choose a reason for hiding this comment

Uh oh!

jpountz commented Jul 6, 2017

Uh oh!

jpountz commented Jul 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

PnPie commented Jun 23, 2017 •

edited

Loading

PnPie commented Jul 6, 2017 •

edited

Loading