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.

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 support for chained multi-fields. #42330

Merged
merged 8 commits into from
May 24, 2019
13 changes: 13 additions & 0 deletions docs/reference/migration/migrate_7_3.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,19 @@ coming[7.3.0]

// end::notable-breaking-changes[]

[[breaking_73_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.

[[breaking_73_plugin_changes]]
=== Plugins changes

Expand Down
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
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
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.");
}
}
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
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
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.WARNING,
"Multi-fields within multi-fields",
"https://www.elastic.co/guide/en/elasticsearch/reference/master/breaking-changes-8.0.html" +
"#_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
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,51 @@ public void testTooManyFieldsCheck() throws IOException {
assertEquals(0, withDefaultFieldIssues.size());
}

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

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

Choose a reason for hiding this comment

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

👍

.startObject("properties")
.startObject("invalid-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()
.startObject("valid-field")
.field("type", "keyword")
.startObject("fields")
.startObject("sub-field")
.field("type", "keyword")
.endObject()
.endObject()
.endObject()
.endObject()
.endObject();
String mapping = BytesReference.bytes(xContent).utf8ToString();

IndexMetaData simpleIndex = IndexMetaData.builder(randomAlphaOfLengthBetween(5, 10))
.settings(settings(Version.V_7_3_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.WARNING,
"Multi-fields within multi-fields",
"https://www.elastic.co/guide/en/elasticsearch/reference/master/breaking-changes-8.0.html" +
"#_defining_multi_fields_within_multi_fields",
"The names of fields that contain chained multi-fields: [[type: _doc, field: invalid-field]]");
assertEquals(singletonList(expected), issues);
}

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