Skip to content

Deprecate support for chained multi-fields. #42330

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

Merged
merged 8 commits into from
May 24, 2019
Merged

Deprecate support for chained multi-fields. #42330

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d86f83e
Deprecate support for chained multi-fields. (#41926)
jtibshirani May 21, 2019
22d00ed
Update the docs and deprecation info API.
jtibshirani May 21, 2019
7fb498e
Lower the deprecation check level to WARNING.
jtibshirani May 22, 2019
d83200c
Test that single-level multi-fields don't produce a deprecation warning.
jtibshirani May 22, 2019
afc2ca4
Make sure to link to the 8.0 breaking changes docs.
jtibshirani May 22, 2019
25eecd8
Update IndexDeprecationChecksTests.
jtibshirani May 22, 2019
a5e6dfa
Merge remote-tracking branch 'upstream/7.x' into chained-multifields
jtibshirani May 24, 2019
8380cb6
Update version references to 7.3 now that the branch is cut.
jtibshirani May 24, 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
13 changes: 13 additions & 0 deletions docs/reference/migration/migrate_7_2.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,16 @@ In earlier versions you could include a range of ports in entries in the
unexpectedly ignored the rest. For instance if you set `discovery.seed_hosts:
"10.11.12.13:9300-9310"` then {es} would only use `10.11.12.13:9300` for
discovery. Seed host addresses containing port ranges are now rejected.

[[breaking_72_mapping_changes]]
=== Mapping changes

[float]
==== Defining multi-fields within multi-fields

Previously, it was possible to define a multi-field within a multi-field.
Defining chained multi-fields is now deprecated and will no longer be supported
in 8.0. To resolve the issue, all instances of `fields` that occur within a
`fields` block should be removed from the mappings, either by flattening the
chained `fields` blocks into a single level, or by switching to `copy_to` if
appropriate.
8 changes: 4 additions & 4 deletions server/src/main/java/org/elasticsearch/index/mapper/Mapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,17 +136,17 @@ public Supplier<QueryShardContext> queryShardContextSupplier() {
protected Function<String, SimilarityProvider> similarityLookupService() { return similarityLookupService; }

public ParserContext createMultiFieldContext(ParserContext in) {
return new MultiFieldParserContext(in) {
@Override
public boolean isWithinMultiField() { return true; }
};
return new MultiFieldParserContext(in);
}

static class MultiFieldParserContext extends ParserContext {
MultiFieldParserContext(ParserContext in) {
super(in.type(), in.similarityLookupService(), in.mapperService(), in.typeParsers(),
in.indexVersionCreated(), in.queryShardContextSupplier());
}

@Override
public boolean isWithinMultiField() { return true; }
}

}
Expand Down
14 changes: 12 additions & 2 deletions server/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,10 @@

package org.elasticsearch.index.mapper;

import org.apache.logging.log4j.LogManager;
import org.apache.lucene.index.IndexOptions;
import org.elasticsearch.ElasticsearchParseException;
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.common.time.DateFormatter;
import org.elasticsearch.common.xcontent.support.XContentMapValues;
import org.elasticsearch.index.analysis.AnalysisMode;
Expand All @@ -37,6 +39,7 @@
import static org.elasticsearch.common.xcontent.support.XContentMapValues.nodeStringValue;

public class TypeParsers {
private static final DeprecationLogger deprecationLogger = new DeprecationLogger(LogManager.getLogger(TypeParsers.class));

public static final String DOC_VALUES = "doc_values";
public static final String INDEX_OPTIONS_DOCS = "docs";
Expand Down Expand Up @@ -214,11 +217,18 @@ public static void parseField(FieldMapper.Builder builder, String name, Map<Stri

public static boolean parseMultiField(FieldMapper.Builder builder, String name, Mapper.TypeParser.ParserContext parserContext,
String propName, Object propNode) {
parserContext = parserContext.createMultiFieldContext(parserContext);
if (propName.equals("fields")) {
if (parserContext.isWithinMultiField()) {
deprecationLogger.deprecatedAndMaybeLog("multifield_within_multifield", "At least one multi-field, [" + name + "], was " +
"encountered that itself contains a multi-field. Defining multi-fields within a multi-field is deprecated and will " +
"no longer be supported in 8.0. To resolve the issue, all instances of [fields] that occur within a [fields] block " +
"should be removed from the mappings, either by flattening the chained [fields] blocks into a single level, or " +
"switching to [copy_to] if appropriate.");
}

final Map<String, Object> multiFieldsPropNodes;
parserContext = parserContext.createMultiFieldContext(parserContext);

final Map<String, Object> multiFieldsPropNodes;
if (propNode instanceof List && ((List<?>) propNode).isEmpty()) {
multiFieldsPropNodes = Collections.emptyMap();
} else if (propNode instanceof Map) {
Expand Down
12 changes: 12 additions & 0 deletions server/src/test/java/org/elasticsearch/index/mapper/ExternalFieldMapperTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,12 @@ public void testExternalValuesWithMultifield() throws Exception {

assertThat(raw, notNullValue());
assertThat(raw.binaryValue(), is(new BytesRef("foo")));

assertWarnings("At least one multi-field, [field], was " +
"encountered that itself contains a multi-field. Defining multi-fields within a multi-field is deprecated and will " +
"no longer be supported in 8.0. To resolve the issue, all instances of [fields] that occur within a [fields] block " +
"should be removed from the mappings, either by flattening the chained [fields] blocks into a single level, or " +
"switching to [copy_to] if appropriate.");
}

public void testExternalValuesWithMultifieldTwoLevels() throws Exception {
Expand Down Expand Up @@ -235,5 +241,11 @@ public void testExternalValuesWithMultifieldTwoLevels() throws Exception {

assertThat(doc.rootDoc().getField("field.raw"), notNullValue());
assertThat(doc.rootDoc().getField("field.raw").stringValue(), is("foo"));

assertWarnings("At least one multi-field, [field], was " +
"encountered that itself contains a multi-field. Defining multi-fields within a multi-field is deprecated and will " +
"no longer be supported in 8.0. To resolve the issue, all instances of [fields] that occur within a [fields] block " +
"should be removed from the mappings, either by flattening the chained [fields] blocks into a single level, or " +
"switching to [copy_to] if appropriate.");
}
}
37 changes: 37 additions & 0 deletions server/src/test/java/org/elasticsearch/index/mapper/TypeParsersTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,11 @@
import org.apache.lucene.analysis.standard.StandardAnalyzer;
import org.elasticsearch.Version;
import org.elasticsearch.cluster.metadata.IndexMetaData;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentFactory;
import org.elasticsearch.common.xcontent.XContentHelper;
import org.elasticsearch.index.IndexSettings;
import org.elasticsearch.index.analysis.AbstractTokenFilterFactory;
import org.elasticsearch.index.analysis.AnalysisMode;
Expand All @@ -36,6 +40,7 @@
import org.elasticsearch.index.analysis.TokenFilterFactory;
import org.elasticsearch.test.ESTestCase;

import java.io.IOException;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
Expand Down Expand Up @@ -157,6 +162,38 @@ public void testParseTextFieldCheckAnalyzerWithSearchAnalyzerAnalysisMode() {
TypeParsers.parseTextField(builder, "name", new HashMap<>(fieldNode), parserContext);
}

public void testMultiFieldWithinMultiField() throws IOException {
TextFieldMapper.Builder builder = new TextFieldMapper.Builder("textField");

XContentBuilder mapping = XContentFactory.jsonBuilder().startObject()
.field("type", "keyword")
.startObject("fields")
.startObject("sub-field")
.field("type", "keyword")
.startObject("fields")
.startObject("sub-sub-field")
.field("type", "keyword")
.endObject()
.endObject()
.endObject()
.endObject()
.endObject();

Map<String, Object> fieldNode = XContentHelper.convertToMap(
BytesReference.bytes(mapping), true, mapping.contentType()).v2();

Mapper.TypeParser typeParser = new KeywordFieldMapper.TypeParser();
Mapper.TypeParser.ParserContext parserContext = new Mapper.TypeParser.ParserContext("type",
null, null, type -> typeParser, Version.CURRENT, null);

TypeParsers.parseField(builder, "some-field", fieldNode, parserContext);
assertWarnings("At least one multi-field, [sub-field], was " +
"encountered that itself contains a multi-field. Defining multi-fields within a multi-field is deprecated and will " +
"no longer be supported in 8.0. To resolve the issue, all instances of [fields] that occur within a [fields] block " +
"should be removed from the mappings, either by flattening the chained [fields] blocks into a single level, or " +
"switching to [copy_to] if appropriate.");
}

private Analyzer createAnalyzerWithMode(String name, AnalysisMode mode) {
TokenFilterFactory tokenFilter = new AbstractTokenFilterFactory(indexSettings, name, Settings.EMPTY) {
@Override
Expand Down
3 changes: 2 additions & 1 deletion ...ugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/DeprecationChecks.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ private DeprecationChecks() {
static List<Function<IndexMetaData, DeprecationIssue>> INDEX_SETTINGS_CHECKS =
Collections.unmodifiableList(Arrays.asList(
IndexDeprecationChecks::oldIndicesCheck,
IndexDeprecationChecks::tooManyFieldsCheck
IndexDeprecationChecks::tooManyFieldsCheck,
IndexDeprecationChecks::chainedMultiFieldsCheck
));

static List<BiFunction<DatafeedConfig, NamedXContentRegistry, DeprecationIssue>> ML_SETTINGS_CHECKS =
Expand Down
26 changes: 26 additions & 0 deletions ...deprecation/src/main/java/org/elasticsearch/xpack/deprecation/IndexDeprecationChecks.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,32 @@ static DeprecationIssue tooManyFieldsCheck(IndexMetaData indexMetaData) {
return null;
}

static DeprecationIssue chainedMultiFieldsCheck(IndexMetaData indexMetaData) {
List<String> issues = new ArrayList<>();
fieldLevelMappingIssue(indexMetaData, ((mappingMetaData, sourceAsMap) -> issues.addAll(
findInPropertiesRecursively(mappingMetaData.type(), sourceAsMap, IndexDeprecationChecks::containsChainedMultiFields))));
if (issues.size() > 0) {
return new DeprecationIssue(DeprecationIssue.Level.CRITICAL,
Copy link
Contributor

@gwbrown gwbrown May 22, 2019

Choose a reason for hiding this comment

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

Will 8.0 fail to open an index which has chained multi-fields that was created in 7.x? Usually we support all indices created in ($MAJOR-1), even if they use deprecated features.

If 8.0 will be able to open indices created in 7.x that have chained multi-fields, this should be Level.WARNING rather than Level.CRITICAL.

Copy link
Contributor Author

@jtibshirani jtibshirani May 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for pointing this out, I will make sure to update the 8.0 PR to still allow chained multi-fields on indices created prior to 8.0. With that change, I'll also be able to lower this to Level.WARNING.

"Multi-fields within multi-fields",
"https://www.elastic.co/guide/en/elasticsearch/reference/7.2/breaking-changes-7.2.html" +
Copy link
Contributor

@gwbrown gwbrown May 22, 2019

Choose a reason for hiding this comment

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

Historically, we've usually linked to the next major version breaking changes list (8.0 in this case), but this also has a few issues: Until the 8.0 branch is cut, the only way to link to these docs is by using master as the version in the URL, which 404s when 8.0 is actually released.

While having this in the breaking changes list for 7.2 and linking to it there would resolve this problem, I don't think it's how we've typically organized the breaking changes list, and might cause issues keeping up to date if we change the deprecation plan.

Basically there's a bunch of problems with how the breaking changes lists and links to them have work at the moment and there's no good options, all I'm pointing out here is that this is different from what we usually do.

Copy link
Contributor Author

@jtibshirani jtibshirani May 22, 2019

Choose a reason for hiding this comment

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

Would you suggest I change this to master for now, for consistency with other deprecation issues we'll be adding? Then I guess we will update all of these links at once when the 8.0 branch is cut?

Copy link
Contributor

@gwbrown gwbrown May 22, 2019

Choose a reason for hiding this comment

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

Yes, I think that's what I would recommend for now, that way if we make a change to the docs to make the situation better it'll be easier to do all at once.

"#_defining_multi_fields_within_multi_fields",
"The names of fields that contain chained multi-fields: " + issues.toString());
}
return null;
}

private static boolean containsChainedMultiFields(Map<?, ?> property) {
if (property.containsKey("fields")) {
Map<?, ?> fields = (Map<?, ?>) property.get("fields");
for (Object rawSubField: fields.values()) {
Map<?, ?> subField = (Map<?, ?>) rawSubField;
if (subField.containsKey("fields")) {
return true;
}
}
}
return false;
}

private static final Set<String> TYPES_THAT_DONT_COUNT;
static {
Expand Down
39 changes: 39 additions & 0 deletions ...cation/src/test/java/org/elasticsearch/xpack/deprecation/IndexDeprecationChecksTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@
import org.elasticsearch.Version;
import org.elasticsearch.cluster.metadata.IndexMetaData;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentFactory;
import org.elasticsearch.index.IndexSettings;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.test.VersionUtils;
Expand Down Expand Up @@ -110,6 +112,43 @@ public void testTooManyFieldsCheck() throws IOException {
assertEquals(0, withDefaultFieldIssues.size());
}

public void testChainedMultiFields() throws IOException {
XContentBuilder xContent = XContentFactory.jsonBuilder().startObject()
Copy link
Contributor

@gwbrown gwbrown May 22, 2019

Choose a reason for hiding this comment

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

I'd like this test case to include at least one field that has a non-chained multi-field to verify that the warning message only contains the fields with a problem.

Copy link
Contributor Author

@jtibshirani jtibshirani May 22, 2019

Choose a reason for hiding this comment

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

👍

.startObject("properties")
.startObject("field")
.field("type", "keyword")
.startObject("fields")
.startObject("sub-field")
.field("type", "keyword")
.startObject("fields")
.startObject("sub-sub-field")
.field("type", "keyword")
.endObject()
.endObject()
.endObject()
.endObject()
.endObject()
.endObject()
.endObject();
String mapping = BytesReference.bytes(xContent).utf8ToString();

IndexMetaData simpleIndex = IndexMetaData.builder(randomAlphaOfLengthBetween(5, 10))
.settings(settings(Version.V_7_2_0))
.numberOfShards(1)
.numberOfReplicas(1)
.putMapping("_doc", mapping)
.build();
List<DeprecationIssue> issues = DeprecationChecks.filterChecks(INDEX_SETTINGS_CHECKS, c -> c.apply(simpleIndex));
assertEquals(1, issues.size());

DeprecationIssue expected = new DeprecationIssue(DeprecationIssue.Level.CRITICAL,
"Multi-fields within multi-fields",
"https://www.elastic.co/guide/en/elasticsearch/reference/7.2/breaking-changes-7.2.html" +
"#_defining_multi_fields_within_multi_fields",
"The names of fields that contain chained multi-fields: [[type: _doc, field: field]]");
assertEquals(singletonList(expected), issues);
}

static void addRandomFields(final int fieldLimit,
XContentBuilder mappingBuilder) throws IOException {
AtomicInteger fieldCount = new AtomicInteger(0);
Expand Down