Skip to content

Commit

Permalink
[root mappers] fix conflict when updating mapping with _all disabled
Browse files Browse the repository at this point in the history 
_all reports a conflict since elastic#7377. However, it was not checked if _all
was actually configured in the updated mapping. Therefore whenever _all
was disabled a mapping could not be updated unless _all was again added to the
updated mapping.
Also, add enabled setting to mapping always whenever enabled was set explicitely.

closes elastic#8423
closes elastic#8426
  • Loading branch information
@brwe
brwe committed Nov 20, 2014
1 parent ecc28fa commit d5136a6
Show file tree
Hide file tree
Showing 3 changed files with 62 additions and 13 deletions.
24 changes: 12 additions & 12 deletions src/main/java/org/elasticsearch/index/mapper/internal/AllFieldMapper.java
View file
Expand Up @@ -75,7 +75,7 @@ public interface IncludeInAll extends Mapper {
public static class Defaults extends AbstractFieldMapper.Defaults {
public static final String NAME = AllFieldMapper.NAME;
public static final String INDEX_NAME = AllFieldMapper.NAME;
public static final boolean ENABLED = true;
public static final EnabledAttributeMapper ENABLED = EnabledAttributeMapper.UNSET_ENABLED;

public static final FieldType FIELD_TYPE = new FieldType();

Expand All @@ -88,7 +88,7 @@ public static class Defaults extends AbstractFieldMapper.Defaults {

public static class Builder extends AbstractFieldMapper.Builder<Builder, AllFieldMapper> {

private boolean enabled = Defaults.ENABLED;
private EnabledAttributeMapper enabled = Defaults.ENABLED;

// an internal flag, automatically set if we encounter boosting
boolean autoBoost = false;
Expand All @@ -99,7 +99,7 @@ public Builder() {
indexName = Defaults.INDEX_NAME;
}

public Builder enabled(boolean enabled) {
public Builder enabled(EnabledAttributeMapper enabled) {
this.enabled = enabled;
return this;
}
Expand All @@ -123,7 +123,7 @@ public Mapper.Builder parse(String name, Map<String, Object> node, ParserContext
String fieldName = Strings.toUnderscoreCase(entry.getKey());
Object fieldNode = entry.getValue();
if (fieldName.equals("enabled")) {
builder.enabled(nodeBooleanValue(fieldNode));
builder.enabled(nodeBooleanValue(fieldNode) ? EnabledAttributeMapper.ENABLED : EnabledAttributeMapper.DISABLED);
} else if (fieldName.equals("auto_boost")) {
builder.autoBoost = nodeBooleanValue(fieldNode);
}
Expand All @@ -133,7 +133,7 @@ public Mapper.Builder parse(String name, Map<String, Object> node, ParserContext
}


private boolean enabled;
private EnabledAttributeMapper enabledState;
// The autoBoost flag is automatically set based on indexed docs on the mappings
// if a doc is indexed with a specific boost value and part of _all, it is automatically
// set to true. This allows to optimize (automatically, which we like) for the common case
Expand All @@ -146,21 +146,21 @@ public AllFieldMapper() {
}

protected AllFieldMapper(String name, FieldType fieldType, NamedAnalyzer indexAnalyzer, NamedAnalyzer searchAnalyzer,
boolean enabled, boolean autoBoost, PostingsFormatProvider postingsProvider,
EnabledAttributeMapper enabled, boolean autoBoost, PostingsFormatProvider postingsProvider,
DocValuesFormatProvider docValuesProvider, SimilarityProvider similarity, Loading normsLoading,
@Nullable Settings fieldDataSettings, Settings indexSettings) {
super(new Names(name, name, name, name), 1.0f, fieldType, null, indexAnalyzer, searchAnalyzer, postingsProvider, docValuesProvider,
similarity, normsLoading, fieldDataSettings, indexSettings);
if (hasDocValues()) {
throw new MapperParsingException("Field [" + names.fullName() + "] is always tokenized and cannot have doc values");
}
this.enabled = enabled;
this.enabledState = enabled;
this.autoBoost = autoBoost;

}

public boolean enabled() {
return this.enabled;
return this.enabledState.enabled;
}

@Override
Expand Down Expand Up @@ -210,7 +210,7 @@ public boolean includeInObject() {

@Override
protected void parseCreateField(ParseContext context, List<Field> fields) throws IOException {
if (!enabled) {
if (!enabledState.enabled) {
return;
}
// reset the entries
Expand Down Expand Up @@ -277,8 +277,8 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
}

private void innerToXContent(XContentBuilder builder, boolean includeDefaults) throws IOException {
if (includeDefaults || enabled != Defaults.ENABLED) {
builder.field("enabled", enabled);
if (includeDefaults || enabledState != Defaults.ENABLED) {
builder.field("enabled", enabledState.enabled);
}
if (includeDefaults || autoBoost != false) {
builder.field("auto_boost", autoBoost);
Expand Down Expand Up @@ -347,7 +347,7 @@ private void innerToXContent(XContentBuilder builder, boolean includeDefaults) t

@Override
public void merge(Mapper mergeWith, MergeContext mergeContext) throws MergeMappingException {
if (((AllFieldMapper)mergeWith).enabled() != this.enabled()) {
if (((AllFieldMapper)mergeWith).enabled() != this.enabled() && ((AllFieldMapper)mergeWith).enabledState != Defaults.ENABLED) {
mergeContext.addConflict("mapper [" + names.fullName() + "] enabled is " + this.enabled() + " now encountering "+ ((AllFieldMapper)mergeWith).enabled());
}
super.merge(mergeWith, mergeContext);
Expand Down
49 changes: 49 additions & 0 deletions src/test/java/org/elasticsearch/index/mapper/update/UpdateMappingOnClusterTests.java
View file
Expand Up @@ -30,6 +30,7 @@
import org.junit.Test;

import java.io.IOException;
import java.util.HashMap;
import java.util.LinkedHashMap;

import static org.elasticsearch.common.io.Streams.copyToStringFromClasspath;
Expand Down Expand Up @@ -70,6 +71,54 @@ public void test_all_conflicts() throws Exception {
testConflict(mapping, mappingUpdate, errorMessage);
}


@Test
public void test_all_with_default() throws Exception {
String defaultMapping = jsonBuilder().startObject().startObject("_default_")
.startObject("_all")
.field("enabled", false)
.endObject()
.endObject().endObject().string();
client().admin().indices().prepareCreate("index").addMapping("_default_", defaultMapping).get();
String docMapping = jsonBuilder().startObject()
.startObject("doc")
.endObject()
.endObject().string();
PutMappingResponse response = client().admin().indices().preparePutMapping("index").setType("doc").setSource(docMapping).get();
assertTrue(response.isAcknowledged());
String docMappingUpdate = jsonBuilder().startObject().startObject("doc")
.startObject("properties")
.startObject("text")
.field("type", "string")
.endObject()
.endObject()
.endObject()
.endObject().string();
response = client().admin().indices().preparePutMapping("index").setType("doc").setSource(docMappingUpdate).get();
assertTrue(response.isAcknowledged());
String docMappingAllExplicitEnabled = jsonBuilder().startObject()
.startObject("doc_all_enabled")
.startObject("_all")
.field("enabled", true)
.endObject()
.endObject()
.endObject().string();
response = client().admin().indices().preparePutMapping("index").setType("doc_all_enabled").setSource(docMappingAllExplicitEnabled).get();
assertTrue(response.isAcknowledged());

GetMappingsResponse mapping = client().admin().indices().prepareGetMappings("index").get();
HashMap props = (HashMap)mapping.getMappings().get("index").get("doc").getSourceAsMap().get("_all");
assertThat((Boolean)props.get("enabled"), equalTo(false));
props = (HashMap)mapping.getMappings().get("index").get("doc").getSourceAsMap().get("properties");
assertNotNull(props);
assertNotNull(props.get("text"));
props = (HashMap)mapping.getMappings().get("index").get("doc_all_enabled").getSourceAsMap().get("_all");
assertThat((Boolean)props.get("enabled"), equalTo(true));
props = (HashMap)mapping.getMappings().get("index").get("_default_").getSourceAsMap().get("_all");
assertThat((Boolean)props.get("enabled"), equalTo(false));

}

@Test
public void test_doc_valuesInvalidMapping() throws Exception {
String mapping = jsonBuilder().startObject().startObject("mappings").startObject(TYPE).startObject("_all").startObject("fielddata").field("format", "doc_values").endObject().endObject().endObject().endObject().endObject().string();
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/org/elasticsearch/index/mapper/update/UpdateMappingTests.java
View file
Expand Up @@ -64,7 +64,7 @@ public void test_all_disabled_after_default_enabled() throws Exception {
public void test_all_enabled_after_enabled() throws Exception {
XContentBuilder mapping = XContentFactory.jsonBuilder().startObject().startObject("_all").field("enabled", true).endObject().endObject();
XContentBuilder mappingUpdate = XContentFactory.jsonBuilder().startObject().startObject("_all").field("enabled", true).endObject().startObject("properties").startObject("text").field("type", "string").endObject().endObject().endObject();
XContentBuilder expectedMapping = XContentFactory.jsonBuilder().startObject().startObject("type").startObject("properties").startObject("text").field("type", "string").endObject().endObject().endObject().endObject();
XContentBuilder expectedMapping = XContentFactory.jsonBuilder().startObject().startObject("type").startObject("_all").field("enabled", true).endObject().startObject("properties").startObject("text").field("type", "string").endObject().endObject().endObject().endObject();
testNoConflictWhileMergingAndMappingChanged(mapping, mappingUpdate, expectedMapping);
}

Expand Down

0 comments on commit d5136a6

Please sign in to comment.