Skip to content

Commit

Permalink
Mapping: Fix possibility of losing meta configuration on field mappin…
Browse files Browse the repository at this point in the history
…g update

The TTL, size, timestamp and index meta properties could be lost on an
update of a single field mapping due to a wrong comparison in the
merge method (which was caused by a wrong initialization, which marked
an update as explicitely disabled instead of unset.

Closes #5053
  • Loading branch information
spinscale committed Jun 19, 2014
1 parent d18fb8b commit 9569166
Show file tree
Hide file tree
Showing 8 changed files with 222 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ public static class Defaults extends AbstractFieldMapper.Defaults {
FIELD_TYPE.freeze();
}

public static final EnabledAttributeMapper ENABLED_STATE = EnabledAttributeMapper.DISABLED;
public static final EnabledAttributeMapper ENABLED_STATE = EnabledAttributeMapper.UNSET_DISABLED;
}

public static class Builder extends AbstractFieldMapper.Builder<Builder, IndexFieldMapper> {
Expand Down Expand Up @@ -202,7 +202,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
if (includeDefaults || fieldType().stored() != Defaults.FIELD_TYPE.stored() && enabledState.enabled) {
builder.field("store", fieldType().stored());
}
if (includeDefaults || enabledState != Defaults.ENABLED_STATE) {
if (includeDefaults || enabledState.enabled != Defaults.ENABLED_STATE.enabled) {
builder.field("enabled", enabledState.enabled);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ public class SizeFieldMapper extends IntegerFieldMapper implements RootMapper {

public static class Defaults extends IntegerFieldMapper.Defaults {
public static final String NAME = CONTENT_TYPE;
public static final EnabledAttributeMapper ENABLED_STATE = EnabledAttributeMapper.DISABLED;
public static final EnabledAttributeMapper ENABLED_STATE = EnabledAttributeMapper.UNSET_DISABLED;

public static final FieldType SIZE_FIELD_TYPE = new FieldType(IntegerFieldMapper.Defaults.FIELD_TYPE);

Expand Down Expand Up @@ -161,7 +161,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
return builder;
}
builder.startObject(contentType());
if (includeDefaults || enabledState != Defaults.ENABLED_STATE) {
if (includeDefaults || enabledState.enabled != Defaults.ENABLED_STATE.enabled) {
builder.field("enabled", enabledState.enabled);
}
if (includeDefaults || fieldType().stored() != Defaults.SIZE_FIELD_TYPE.stored() && enabledState.enabled) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ public static class Defaults extends LongFieldMapper.Defaults {
TTL_FIELD_TYPE.freeze();
}

public static final EnabledAttributeMapper ENABLED_STATE = EnabledAttributeMapper.DISABLED;
public static final EnabledAttributeMapper ENABLED_STATE = EnabledAttributeMapper.UNSET_DISABLED;
public static final long DEFAULT = -1;
}

Expand Down Expand Up @@ -225,7 +225,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
return builder;
}
builder.startObject(CONTENT_TYPE);
if (includeDefaults || enabledState != Defaults.ENABLED_STATE) {
if (includeDefaults || enabledState.enabled != Defaults.ENABLED_STATE.enabled) {
builder.field("enabled", enabledState.enabled);
}
if (includeDefaults || defaultTTL != Defaults.DEFAULT && enabledState.enabled) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ public static class Defaults extends DateFieldMapper.Defaults {
FIELD_TYPE.freeze();
}

public static final EnabledAttributeMapper ENABLED = EnabledAttributeMapper.DISABLED;
public static final EnabledAttributeMapper ENABLED = EnabledAttributeMapper.UNSET_DISABLED;
public static final String PATH = null;
public static final FormatDateTimeFormatter DATE_TIME_FORMATTER = Joda.forPattern(DEFAULT_DATE_TIME_FORMAT);
}
Expand Down Expand Up @@ -230,7 +230,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
return builder;
}
builder.startObject(CONTENT_TYPE);
if (includeDefaults || enabledState != Defaults.ENABLED) {
if (includeDefaults || enabledState.enabled != Defaults.ENABLED.enabled) {
builder.field("enabled", enabledState.enabled);
}
if (enabledState.enabled) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
/*
* Licensed to Elasticsearch 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.index.mapper.index;

import org.elasticsearch.action.admin.indices.mapping.get.GetMappingsResponse;
import org.elasticsearch.action.admin.indices.mapping.put.PutMappingResponse;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.test.ElasticsearchIntegrationTest;
import org.junit.Test;

import java.io.IOException;
import java.util.Locale;
import java.util.Map;

import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
import static org.hamcrest.Matchers.*;

/**
*
*/
public class IndexTypeMapperIntegrationTests extends ElasticsearchIntegrationTest {

@Test // issue 5053
public void testThatUpdatingMappingShouldNotRemoveSizeMappingConfiguration() throws Exception {
String index = "foo";
String type = "mytype";

XContentBuilder builder = jsonBuilder().startObject().startObject("_index").field("enabled", true).endObject().endObject();
assertAcked(client().admin().indices().prepareCreate(index).addMapping(type, builder));

// check mapping again
assertIndexMappingEnabled(index, type);

// update some field in the mapping
XContentBuilder updateMappingBuilder = jsonBuilder().startObject().startObject("properties").startObject("otherField").field("type", "string").endObject().endObject();
PutMappingResponse putMappingResponse = client().admin().indices().preparePutMapping(index).setType(type).setSource(updateMappingBuilder).get();
assertAcked(putMappingResponse);

// make sure timestamp field is still in mapping
assertIndexMappingEnabled(index, type);
}

private void assertIndexMappingEnabled(String index, String type) throws IOException {
String errMsg = String.format(Locale.ROOT, "Expected index field mapping to be enabled for %s/%s", index, type);

GetMappingsResponse getMappingsResponse = client().admin().indices().prepareGetMappings(index).addTypes(type).get();
Map<String, Object> mappingSource = getMappingsResponse.getMappings().get(index).get(type).getSourceAsMap();
assertThat(errMsg, mappingSource, hasKey("_index"));
String ttlAsString = mappingSource.get("_index").toString();
assertThat(ttlAsString, is(notNullValue()));
assertThat(errMsg, ttlAsString, is("{enabled=true}"));
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
/*
* Licensed to Elasticsearch 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.index.mapper.size;

import org.elasticsearch.action.admin.indices.mapping.get.GetMappingsResponse;
import org.elasticsearch.action.admin.indices.mapping.put.PutMappingResponse;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.test.ElasticsearchIntegrationTest;
import org.junit.Test;

import java.io.IOException;
import java.util.Locale;
import java.util.Map;

import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
import static org.hamcrest.Matchers.*;

public class SizeMappingIntegrationTests extends ElasticsearchIntegrationTest {

@Test // issue 5053
public void testThatUpdatingMappingShouldNotRemoveSizeMappingConfiguration() throws Exception {
String index = "foo";
String type = "mytype";

XContentBuilder builder = jsonBuilder().startObject().startObject("_size").field("enabled", true).endObject().endObject();
assertAcked(client().admin().indices().prepareCreate(index).addMapping(type, builder));

// check mapping again
assertSizeMappingEnabled(index, type);

// update some field in the mapping
XContentBuilder updateMappingBuilder = jsonBuilder().startObject().startObject("properties").startObject("otherField").field("type", "string").endObject().endObject();
PutMappingResponse putMappingResponse = client().admin().indices().preparePutMapping(index).setType(type).setSource(updateMappingBuilder).get();
assertAcked(putMappingResponse);

// make sure timestamp field is still in mapping
assertSizeMappingEnabled(index, type);
}

private void assertSizeMappingEnabled(String index, String type) throws IOException {
String errMsg = String.format(Locale.ROOT, "Expected size field mapping to be enabled for %s/%s", index, type);

GetMappingsResponse getMappingsResponse = client().admin().indices().prepareGetMappings(index).addTypes(type).get();
Map<String, Object> mappingSource = getMappingsResponse.getMappings().get(index).get(type).getSourceAsMap();
assertThat(errMsg, mappingSource, hasKey("_size"));
String ttlAsString = mappingSource.get("_size").toString();
assertThat(ttlAsString, is(notNullValue()));
assertThat(errMsg, ttlAsString, is("{enabled=true}"));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,19 @@

package org.elasticsearch.timestamp;

import org.elasticsearch.action.admin.indices.mapping.get.GetMappingsResponse;
import org.elasticsearch.action.admin.indices.mapping.put.PutMappingResponse;
import org.elasticsearch.action.get.GetResponse;
import org.elasticsearch.cluster.metadata.MappingMetaData;
import org.elasticsearch.common.Priority;
import org.elasticsearch.common.xcontent.XContentFactory;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.test.ElasticsearchIntegrationTest;
import org.junit.Test;

import java.util.Locale;

import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
import static org.hamcrest.Matchers.*;

/**
Expand All @@ -35,7 +42,7 @@ public class SimpleTimestampTests extends ElasticsearchIntegrationTest {
public void testSimpleTimestamp() throws Exception {

client().admin().indices().prepareCreate("test")
.addMapping("type1", XContentFactory.jsonBuilder().startObject().startObject("type1").startObject("_timestamp").field("enabled", true).field("store", "yes").endObject().endObject().endObject())
.addMapping("type1", jsonBuilder().startObject().startObject("type1").startObject("_timestamp").field("enabled", true).field("store", "yes").endObject().endObject().endObject())
.execute().actionGet();
client().admin().cluster().prepareHealth().setWaitForEvents(Priority.LANGUID).setWaitForGreenStatus().execute().actionGet();

Expand Down Expand Up @@ -82,4 +89,32 @@ public void testSimpleTimestamp() throws Exception {
getResponse = client().prepareGet("test", "type1", "1").setFields("_timestamp").setRealtime(false).execute().actionGet();
assertThat(((Number) getResponse.getField("_timestamp").getValue()).longValue(), equalTo(timestamp));
}

@Test // issue 5053
public void testThatUpdatingMappingShouldNotRemoveTimestampConfiguration() throws Exception {
String index = "foo";
String type = "mytype";

XContentBuilder builder = jsonBuilder().startObject().startObject("_timestamp").field("enabled", true).endObject().endObject();
assertAcked(client().admin().indices().prepareCreate(index).addMapping(type, builder));

// check mapping again
assertTimestampMappingEnabled(index, type);

// update some field in the mapping
XContentBuilder updateMappingBuilder = jsonBuilder().startObject().startObject("properties").startObject("otherField").field("type", "string").endObject().endObject();
PutMappingResponse putMappingResponse = client().admin().indices().preparePutMapping(index).setType(type).setSource(updateMappingBuilder).get();
assertAcked(putMappingResponse);

// make sure timestamp field is still in mapping
assertTimestampMappingEnabled(index, type);
}

private void assertTimestampMappingEnabled(String index, String type) {
GetMappingsResponse getMappingsResponse = client().admin().indices().prepareGetMappings(index).addTypes(type).get();
MappingMetaData.Timestamp timestamp = getMappingsResponse.getMappings().get(index).get(type).timestamp();
assertThat(timestamp, is(notNullValue()));
String errMsg = String.format(Locale.ROOT, "Expected timestamp field mapping to be enabled for %s/%s", index, type);
assertThat(errMsg, timestamp.enabled(), is(true));
}
}
39 changes: 39 additions & 0 deletions src/test/java/org/elasticsearch/ttl/SimpleTTLTests.java
Original file line number Diff line number Diff line change
Expand Up @@ -20,18 +20,26 @@
package org.elasticsearch.ttl;

import com.google.common.base.Predicate;
import org.elasticsearch.action.admin.indices.mapping.get.GetMappingsResponse;
import org.elasticsearch.action.admin.indices.mapping.put.PutMappingResponse;
import org.elasticsearch.action.admin.indices.stats.IndicesStatsResponse;
import org.elasticsearch.action.get.GetResponse;
import org.elasticsearch.action.index.IndexResponse;
import org.elasticsearch.cluster.metadata.MappingMetaData;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentFactory;
import org.elasticsearch.test.ElasticsearchIntegrationTest;
import org.elasticsearch.test.ElasticsearchIntegrationTest.ClusterScope;
import org.junit.Test;

import java.io.IOException;
import java.util.Locale;
import java.util.Map;
import java.util.concurrent.TimeUnit;

import static org.elasticsearch.common.settings.ImmutableSettings.settingsBuilder;
import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;
import static org.elasticsearch.test.ElasticsearchIntegrationTest.*;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
import static org.hamcrest.Matchers.*;
Expand Down Expand Up @@ -192,4 +200,35 @@ public boolean apply(Object input) {
getResponse = client().prepareGet("test", "type1", "with_routing").setRouting("routing").setFields("_ttl").setRealtime(false).execute().actionGet();
assertThat(getResponse.isExists(), equalTo(false));
}

@Test // issue 5053
public void testThatUpdatingMappingShouldNotRemoveTTLConfiguration() throws Exception {
String index = "foo";
String type = "mytype";

XContentBuilder builder = jsonBuilder().startObject().startObject("_ttl").field("enabled", true).endObject().endObject();
assertAcked(client().admin().indices().prepareCreate(index).addMapping(type, builder));

// check mapping again
assertTTLMappingEnabled(index, type);

// update some field in the mapping
XContentBuilder updateMappingBuilder = jsonBuilder().startObject().startObject("properties").startObject("otherField").field("type", "string").endObject().endObject();
PutMappingResponse putMappingResponse = client().admin().indices().preparePutMapping(index).setType(type).setSource(updateMappingBuilder).get();
assertAcked(putMappingResponse);

// make sure timestamp field is still in mapping
assertTTLMappingEnabled(index, type);
}

private void assertTTLMappingEnabled(String index, String type) throws IOException {
String errMsg = String.format(Locale.ROOT, "Expected ttl field mapping to be enabled for %s/%s", index, type);

GetMappingsResponse getMappingsResponse = client().admin().indices().prepareGetMappings(index).addTypes(type).get();
Map<String, Object> mappingSource = getMappingsResponse.getMappings().get(index).get(type).getSourceAsMap();
assertThat(errMsg, mappingSource, hasKey("_ttl"));
String ttlAsString = mappingSource.get("_ttl").toString();
assertThat(ttlAsString, is(notNullValue()));
assertThat(errMsg, ttlAsString, is("{enabled=true}"));
}
}

0 comments on commit 9569166

Please sign in to comment.