From d73a6663b7e773032b207f16fc9dd27883b9eadd Mon Sep 17 00:00:00 2001 From: Igor Motov Date: Thu, 3 Jan 2013 13:24:28 -0500 Subject: [PATCH] Changing non-nested object mapping to nested should fail Fixes #2518 --- .../index/mapper/object/ObjectMapper.java | 12 +++ .../mapper/merge/Test1MergeMapperTests.java | 57 ------------ .../mapper/merge/TestMergeMapperTests.java | 86 +++++++++++++++++++ .../test/unit/index/mapper/merge/stage1.json | 9 -- .../test/unit/index/mapper/merge/stage2.json | 19 ---- 5 files changed, 98 insertions(+), 85 deletions(-) delete mode 100644 src/test/java/org/elasticsearch/test/unit/index/mapper/merge/Test1MergeMapperTests.java create mode 100644 src/test/java/org/elasticsearch/test/unit/index/mapper/merge/TestMergeMapperTests.java delete mode 100644 src/test/java/org/elasticsearch/test/unit/index/mapper/merge/stage1.json delete mode 100644 src/test/java/org/elasticsearch/test/unit/index/mapper/merge/stage2.json diff --git a/src/main/java/org/elasticsearch/index/mapper/object/ObjectMapper.java b/src/main/java/org/elasticsearch/index/mapper/object/ObjectMapper.java index b3a996f1f5d..b4d3980ebda 100644 --- a/src/main/java/org/elasticsearch/index/mapper/object/ObjectMapper.java +++ b/src/main/java/org/elasticsearch/index/mapper/object/ObjectMapper.java @@ -784,6 +784,18 @@ public class ObjectMapper implements Mapper, AllFieldMapper.IncludeInAll { } ObjectMapper mergeWithObject = (ObjectMapper) mergeWith; + if (nested().isNested()) { + if (!mergeWithObject.nested().isNested()) { + mergeContext.addConflict("object mapping [" + name() + "] can't be changed from nested to non-nested"); + return; + } + } else { + if (mergeWithObject.nested().isNested()) { + mergeContext.addConflict("object mapping [" + name() + "] can't be changed from non-nested to nested"); + return; + } + } + doMerge(mergeWithObject, mergeContext); List mappersToTraverse = new ArrayList(); diff --git a/src/test/java/org/elasticsearch/test/unit/index/mapper/merge/Test1MergeMapperTests.java b/src/test/java/org/elasticsearch/test/unit/index/mapper/merge/Test1MergeMapperTests.java deleted file mode 100644 index 8c0908c2070..00000000000 --- a/src/test/java/org/elasticsearch/test/unit/index/mapper/merge/Test1MergeMapperTests.java +++ /dev/null @@ -1,57 +0,0 @@ -/* - * Licensed to ElasticSearch and Shay Banon under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. ElasticSearch licenses this - * file to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -package org.elasticsearch.test.unit.index.mapper.merge; - -import org.elasticsearch.index.mapper.DocumentMapper; -import org.elasticsearch.test.unit.index.mapper.MapperTests; -import org.testng.annotations.Test; - -import static org.elasticsearch.common.io.Streams.copyToStringFromClasspath; -import static org.elasticsearch.index.mapper.DocumentMapper.MergeFlags.mergeFlags; -import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.*; - -/** - * - */ -@Test -public class Test1MergeMapperTests { - - @Test - public void test1Merge() throws Exception { - String stage1Mapping = copyToStringFromClasspath("/org/elasticsearch/test/unit/index/mapper/merge/stage1.json"); - DocumentMapper stage1 = MapperTests.newParser().parse(stage1Mapping); - String stage2Mapping = copyToStringFromClasspath("/org/elasticsearch/test/unit/index/mapper/merge/stage2.json"); - DocumentMapper stage2 = MapperTests.newParser().parse(stage2Mapping); - - DocumentMapper.MergeResult mergeResult = stage1.merge(stage2, mergeFlags().simulate(true)); - assertThat(mergeResult.hasConflicts(), equalTo(false)); - // since we are simulating, we should not have the age mapping - assertThat(stage1.mappers().smartName("age"), nullValue()); - assertThat(stage1.mappers().smartName("obj1.prop1"), nullValue()); - // now merge, don't simulate - mergeResult = stage1.merge(stage2, mergeFlags().simulate(false)); - // there is still merge failures - assertThat(mergeResult.hasConflicts(), equalTo(false)); - // but we have the age in - assertThat(stage1.mappers().smartName("age"), notNullValue()); - assertThat(stage1.mappers().smartName("obj1.prop1"), notNullValue()); - } -} diff --git a/src/test/java/org/elasticsearch/test/unit/index/mapper/merge/TestMergeMapperTests.java b/src/test/java/org/elasticsearch/test/unit/index/mapper/merge/TestMergeMapperTests.java new file mode 100644 index 00000000000..f866f4609a3 --- /dev/null +++ b/src/test/java/org/elasticsearch/test/unit/index/mapper/merge/TestMergeMapperTests.java @@ -0,0 +1,86 @@ +/* + * Licensed to ElasticSearch and Shay Banon under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. ElasticSearch licenses this + * file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.test.unit.index.mapper.merge; + +import org.elasticsearch.common.xcontent.XContentFactory; +import org.elasticsearch.index.mapper.DocumentMapper; +import org.elasticsearch.test.unit.index.mapper.MapperTests; +import org.testng.annotations.Test; + +import static org.elasticsearch.index.mapper.DocumentMapper.MergeFlags.mergeFlags; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.*; + +/** + * + */ +@Test +public class TestMergeMapperTests { + + @Test + public void test1Merge() throws Exception { + + String stage1Mapping = XContentFactory.jsonBuilder().startObject().startObject("person").startObject("properties") + .startObject("name").field("type", "string").endObject() + .endObject().endObject().endObject().string(); + DocumentMapper stage1 = MapperTests.newParser().parse(stage1Mapping); + String stage2Mapping = XContentFactory.jsonBuilder().startObject().startObject("person").startObject("properties") + .startObject("name").field("type", "string").endObject() + .startObject("age").field("type", "integer").endObject() + .startObject("obj1").startObject("properties").startObject("prop1").field("type", "integer").endObject().endObject().endObject() + .endObject().endObject().endObject().string(); + DocumentMapper stage2 = MapperTests.newParser().parse(stage2Mapping); + + DocumentMapper.MergeResult mergeResult = stage1.merge(stage2, mergeFlags().simulate(true)); + assertThat(mergeResult.hasConflicts(), equalTo(false)); + // since we are simulating, we should not have the age mapping + assertThat(stage1.mappers().smartName("age"), nullValue()); + assertThat(stage1.mappers().smartName("obj1.prop1"), nullValue()); + // now merge, don't simulate + mergeResult = stage1.merge(stage2, mergeFlags().simulate(false)); + // there is still merge failures + assertThat(mergeResult.hasConflicts(), equalTo(false)); + // but we have the age in + assertThat(stage1.mappers().smartName("age"), notNullValue()); + assertThat(stage1.mappers().smartName("obj1.prop1"), notNullValue()); + } + + + @Test + public void testMergeObjectAndNested() throws Exception { + String objectMapping = XContentFactory.jsonBuilder().startObject().startObject("type1").startObject("properties") + .startObject("obj").field("type", "object").endObject() + .endObject().endObject().endObject().string(); + DocumentMapper objectMapper = MapperTests.newParser().parse(objectMapping); + String nestedMapping = XContentFactory.jsonBuilder().startObject().startObject("type1").startObject("properties") + .startObject("obj").field("type", "nested").endObject() + .endObject().endObject().endObject().string(); + DocumentMapper nestedMapper = MapperTests.newParser().parse(nestedMapping); + + DocumentMapper.MergeResult mergeResult = objectMapper.merge(nestedMapper, mergeFlags().simulate(true)); + assertThat(mergeResult.hasConflicts(), equalTo(true)); + assertThat(mergeResult.conflicts().length, equalTo(1)); + assertThat(mergeResult.conflicts()[0], equalTo("object mapping [obj] can't be changed from non-nested to nested")); + + mergeResult = nestedMapper.merge(objectMapper, mergeFlags().simulate(true)); + assertThat(mergeResult.conflicts().length, equalTo(1)); + assertThat(mergeResult.conflicts()[0], equalTo("object mapping [obj] can't be changed from nested to non-nested")); + } +} diff --git a/src/test/java/org/elasticsearch/test/unit/index/mapper/merge/stage1.json b/src/test/java/org/elasticsearch/test/unit/index/mapper/merge/stage1.json deleted file mode 100644 index 1c963d73b68..00000000000 --- a/src/test/java/org/elasticsearch/test/unit/index/mapper/merge/stage1.json +++ /dev/null @@ -1,9 +0,0 @@ -{ - "person":{ - "properties":{ - "name":{ - "type":"string" - } - } - } -} \ No newline at end of file diff --git a/src/test/java/org/elasticsearch/test/unit/index/mapper/merge/stage2.json b/src/test/java/org/elasticsearch/test/unit/index/mapper/merge/stage2.json deleted file mode 100644 index a7b93df9bfa..00000000000 --- a/src/test/java/org/elasticsearch/test/unit/index/mapper/merge/stage2.json +++ /dev/null @@ -1,19 +0,0 @@ -{ - "person":{ - "properties":{ - "name":{ - "type":"string" - }, - "age":{ - "type":"integer" - }, - "obj1":{ - "properties":{ - "prop1":{ - "type":"string" - } - } - } - } - } -} \ No newline at end of file