Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate _field_names disabling #42854

Merged
merged 10 commits into from
Sep 11, 2019
Merged

Deprecate _field_names disabling #42854

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
9635484
Deprecate and ignore `_field_names` disabling
Jun 4, 2019
26e8b64
iter
Jun 5, 2019
e4620c6
Merge branch 'master' into fix-27239
Aug 5, 2019
d46bf47
Only deprecate and log
Aug 5, 2019
c2e5503
Merge branch 'master' into fix-27239
Aug 9, 2019
4308e8e
Log deprecation on any index version, inlcude name in log
Aug 9, 2019
b653c5d
iter
Aug 9, 2019
96ef8f8
Merge branch 'master' into fix-27239
Aug 9, 2019
c4a0cb0
Merge branch 'master' into fix-27239
Sep 9, 2019
c552702
Changes to deprecation message and revert removing test
Sep 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions ...les/percolator/src/test/java/org/elasticsearch/percolator/PercolatorFieldMapperTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ public void init() throws Exception {
mapperService = indexService.mapperService();

String mapper = Strings.toString(XContentFactory.jsonBuilder().startObject().startObject("doc")
.startObject("_field_names").field("enabled", false).endObject() // makes testing easier
.startObject("_field_names").endObject() // makes testing easier
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could avoid mentioning _field_names here altogether? The same thought applies to other places in this file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, we should disallow _field_names entirely for newly created indices.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really following you here, #27239 talks about disabeling the enableoption. What would disallowing the whole field mean? Do we still create the field (since its default is enabled:true?) What's the whole breaking/deprecation story around this then?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I wasn't clear I meant that we should disallow setting _field_names entirely since we're on master but it seems that the scope of this pr is only the deprecation. However I agree with Julie that we should try to avoid mentioning _field_names in this test since this shouldn't affect the behavior of the percolator. If it turns too complicated we can leave this for a follow up but we'll need to do it anyway since the goal is to disallow disallowing the _field_names entirely and this test relies on this forbidden behavior.

.startObject("properties")
.startObject("field").field("type", "text").endObject()
.startObject("field1").field("type", "text").endObject()
Expand Down Expand Up @@ -322,7 +322,7 @@ public void testExtractTermsAndRanges_partial() throws Exception {
ParseContext.Document document = parseContext.doc();

PercolatorFieldMapper.FieldType fieldType = (PercolatorFieldMapper.FieldType) fieldMapper.fieldType();
assertThat(document.getFields().size(), equalTo(3));
assertThat(document.getFields().size(), equalTo(4));
assertThat(document.getFields().get(0).binaryValue().utf8ToString(), equalTo("field\u0000term"));
assertThat(document.getField(fieldType.extractionResultField.name()).stringValue(), equalTo(EXTRACTION_PARTIAL));
}
Expand Down Expand Up @@ -590,7 +590,7 @@ public void testAllowNoAdditionalSettings() throws Exception {
public void testMultiplePercolatorFields() throws Exception {
String typeName = "doc";
String percolatorMapper = Strings.toString(XContentFactory.jsonBuilder().startObject().startObject(typeName)
.startObject("_field_names").field("enabled", false).endObject() // makes testing easier
.startObject("_field_names").endObject() // makes testing easier
.startObject("properties")
.startObject("query_field1").field("type", "percolator").endObject()
.startObject("query_field2").field("type", "percolator").endObject()
Expand All @@ -605,7 +605,8 @@ public void testMultiplePercolatorFields() throws Exception {
.field("query_field2", queryBuilder)
.endObject()),
XContentType.JSON));
assertThat(doc.rootDoc().getFields().size(), equalTo(14)); // also includes all other meta fields
System.out.println(doc.rootDoc().getFields());
cbuescher marked this conversation as resolved.
Show resolved Hide resolved
assertThat(doc.rootDoc().getFields().size(), equalTo(16)); // also includes all other meta fields
BytesRef queryBuilderAsBytes = doc.rootDoc().getField("query_field1.query_builder_field").binaryValue();
assertQueryBuilder(queryBuilderAsBytes, queryBuilder);

Expand All @@ -617,7 +618,7 @@ public void testMultiplePercolatorFields() throws Exception {
public void testNestedPercolatorField() throws Exception {
String typeName = "doc";
String percolatorMapper = Strings.toString(XContentFactory.jsonBuilder().startObject().startObject(typeName)
.startObject("_field_names").field("enabled", false).endObject() // makes testing easier
.startObject("_field_names").endObject() // makes testing easier
.startObject("properties")
.startObject("object_field")
.field("type", "object")
Expand All @@ -635,7 +636,7 @@ public void testNestedPercolatorField() throws Exception {
.field("query_field", queryBuilder)
.endObject().endObject()),
XContentType.JSON));
assertThat(doc.rootDoc().getFields().size(), equalTo(10)); // also includes all other meta fields
assertThat(doc.rootDoc().getFields().size(), equalTo(12)); // also includes all other meta fields
BytesRef queryBuilderAsBytes = doc.rootDoc().getField("object_field.query_field.query_builder_field").binaryValue();
assertQueryBuilder(queryBuilderAsBytes, queryBuilder);

Expand All @@ -646,7 +647,7 @@ public void testNestedPercolatorField() throws Exception {
.endArray()
.endObject()),
XContentType.JSON));
assertThat(doc.rootDoc().getFields().size(), equalTo(10)); // also includes all other meta fields
assertThat(doc.rootDoc().getFields().size(), equalTo(12)); // also includes all other meta fields
queryBuilderAsBytes = doc.rootDoc().getField("object_field.query_field.query_builder_field").binaryValue();
assertQueryBuilder(queryBuilderAsBytes, queryBuilder);

Expand Down
16 changes: 8 additions & 8 deletions server/src/main/java/org/elasticsearch/index/mapper/FieldNamesFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,28 +73,26 @@ public static class Defaults {
}

private static class Builder extends MetadataFieldMapper.Builder<Builder, FieldNamesFieldMapper> {
private boolean enabled = Defaults.ENABLED;

private Builder(MappedFieldType existing) {
super(Defaults.NAME, existing == null ? Defaults.FIELD_TYPE : existing, Defaults.FIELD_TYPE);
}

private Builder enabled(boolean enabled) {
this.enabled = enabled;
return this;
}

@Override
public FieldNamesFieldMapper build(BuilderContext context) {
setupFieldType(context);
fieldType.setHasDocValues(false);
FieldNamesFieldType fieldNamesFieldType = (FieldNamesFieldType)fieldType;
fieldNamesFieldType.setEnabled(enabled);
fieldNamesFieldType.setEnabled(Defaults.ENABLED);
return new FieldNamesFieldMapper(fieldType, context.indexSettings());
}
}

public static class TypeParser implements MetadataFieldMapper.TypeParser {
public static final String ENABLED_DEPRECATION_MESSAGE = "Changing the `enabled` setting for `_field_names` fields is no "
cbuescher marked this conversation as resolved.
Show resolved Hide resolved
+ "longer necassary. Disabling it has almost no benefits anymore which is why we now ignore this setting and will "
+ "remove it in a future major version.";

@Override
public MetadataFieldMapper.Builder<?,?> parse(String name, Map<String, Object> node,
ParserContext parserContext) throws MapperParsingException {
Expand All @@ -105,7 +103,9 @@ public MetadataFieldMapper.Builder<?,?> parse(String name, Map<String, Object> n
String fieldName = entry.getKey();
Object fieldNode = entry.getValue();
if (fieldName.equals("enabled")) {
builder.enabled(XContentMapValues.nodeBooleanValue(fieldNode, name + ".enabled"));
deprecationLogger.deprecatedAndMaybeLog("field_names_enabled_parameter", ENABLED_DEPRECATION_MESSAGE);
// just read the value and dump it on the floor
XContentMapValues.nodeBooleanValue(fieldNode, name + ".enabled");
iterator.remove();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should still support this field (and log the deprecation) for indices created in a version where the option is available (7x) and throw an error if the version is on or after 8.0 ?

}
}
Expand Down
25 changes: 4 additions & 21 deletions server/src/test/java/org/elasticsearch/index/mapper/FieldNamesFieldMapperTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,16 +114,17 @@ public void testExplicitEnabled() throws Exception {
XContentType.JSON));

assertFieldNames(set("field"), doc);
assertWarnings(FieldNamesFieldMapper.TypeParser.ENABLED_DEPRECATION_MESSAGE);
}

public void testDisabled() throws Exception {
public void testEnabledSettingLogsDeprecation() throws Exception {
String mapping = Strings.toString(XContentFactory.jsonBuilder().startObject().startObject("type")
.startObject("_field_names").field("enabled", false).endObject()
.endObject().endObject());
DocumentMapper docMapper = createIndex("test").mapperService().documentMapperParser()
.parse("type", new CompressedXContent(mapping));
FieldNamesFieldMapper fieldNamesMapper = docMapper.metadataMapper(FieldNamesFieldMapper.class);
assertFalse(fieldNamesMapper.fieldType().isEnabled());
assertTrue(fieldNamesMapper.fieldType().isEnabled());

ParsedDocument doc = docMapper.parse(new SourceToParse("test", "type", "1",
BytesReference.bytes(XContentFactory.jsonBuilder()
Expand All @@ -133,24 +134,6 @@ public void testDisabled() throws Exception {
XContentType.JSON));

assertNull(doc.rootDoc().get("_field_names"));
}

public void testMergingMappings() throws Exception {
cbuescher marked this conversation as resolved.
Show resolved Hide resolved
String enabledMapping = Strings.toString(XContentFactory.jsonBuilder().startObject().startObject("type")
.startObject("_field_names").field("enabled", true).endObject()
.endObject().endObject());
String disabledMapping = Strings.toString(XContentFactory.jsonBuilder().startObject().startObject("type")
.startObject("_field_names").field("enabled", false).endObject()
.endObject().endObject());
MapperService mapperService = createIndex("test").mapperService();

DocumentMapper mapperEnabled = mapperService.merge("type", new CompressedXContent(enabledMapping),
MapperService.MergeReason.MAPPING_UPDATE);
DocumentMapper mapperDisabled = mapperService.merge("type", new CompressedXContent(disabledMapping),
MapperService.MergeReason.MAPPING_UPDATE);
assertFalse(mapperDisabled.metadataMapper(FieldNamesFieldMapper.class).fieldType().isEnabled());

mapperEnabled = mapperService.merge("type", new CompressedXContent(enabledMapping), MapperService.MergeReason.MAPPING_UPDATE);
assertTrue(mapperEnabled.metadataMapper(FieldNamesFieldMapper.class).fieldType().isEnabled());
assertWarnings(FieldNamesFieldMapper.TypeParser.ENABLED_DEPRECATION_MESSAGE);
}
}
27 changes: 0 additions & 27 deletions server/src/test/java/org/elasticsearch/index/query/QueryStringQueryBuilderTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@
import org.apache.lucene.util.automaton.Automaton;
import org.apache.lucene.util.automaton.Operations;
import org.apache.lucene.util.automaton.TooComplexToDeterminizeException;
import org.elasticsearch.action.admin.indices.mapping.put.PutMappingRequest;
import org.elasticsearch.cluster.metadata.IndexMetaData;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.compress.CompressedXContent;
Expand Down Expand Up @@ -1064,32 +1063,6 @@ public void testExistsFieldQuery() throws Exception {
assertThat(query, equalTo(expected));
}

public void testDisabledFieldNamesField() throws Exception {
QueryShardContext context = createShardContext();
context.getMapperService().merge("_doc",
new CompressedXContent(
Strings.toString(PutMappingRequest.buildFromSimplifiedDef("_doc",
"foo", "type=text",
"_field_names", "enabled=false"))),
MapperService.MergeReason.MAPPING_UPDATE);
try {
QueryStringQueryBuilder queryBuilder = new QueryStringQueryBuilder("foo:*");
Query query = queryBuilder.toQuery(context);
Query expected = new WildcardQuery(new Term("foo", "*"));
assertThat(query, equalTo(expected));
} finally {
// restore mappings as they were before
context.getMapperService().merge("_doc",
new CompressedXContent(
Strings.toString(PutMappingRequest.buildFromSimplifiedDef("_doc",
"foo", "type=text",
"_field_names", "enabled=true"))),
MapperService.MergeReason.MAPPING_UPDATE);
}
}



public void testFromJson() throws IOException {
String json =
"{\n" +
Expand Down