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
Prev Previous commit
Next Next commit
Update the docs and deprecation info API.
jtibshirani committed May 22, 2019
commit 22d00edeb8e5a63669bba20a3b04944e1bf4b419
13 changes: 13 additions & 0 deletions docs/reference/migration/migrate_7_2.asciidoc
Original file line number Diff line number Diff line change
@@ -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.
Original file line number Diff line number Diff line change
@@ -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 =
Original file line number Diff line number Diff line change
@@ -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

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

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

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

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

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 {
Original file line number Diff line number Diff line change
@@ -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;
@@ -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

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("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);