Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,19 @@
import org.elasticsearch.cluster.metadata.MappingMetaData;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.unit.TimeValue;
import org.elasticsearch.common.xcontent.XContentHelper;
import org.elasticsearch.index.IndexSettings;
import org.elasticsearch.index.mapper.FieldNamesFieldMapper;
import org.elasticsearch.ingest.IngestService;
import org.elasticsearch.ingest.PipelineConfiguration;
import org.elasticsearch.xpack.core.deprecation.DeprecationIssue;

import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.stream.Collectors;

Expand Down Expand Up @@ -91,6 +95,53 @@ static DeprecationIssue checkTemplatesWithTooManyFields(ClusterState state) {
return null;
}

/**
* Check templates that use `_field_names` explicitly, which was deprecated in https://github.com/elastic/elasticsearch/pull/42854
* and will throw an error on new indices in 8.0
*/
@SuppressWarnings("unchecked")
static DeprecationIssue checkTemplatesWithFieldNamesDisabled(ClusterState state) {
Set<String> templatesContainingFieldNames = new HashSet<>();
state.getMetaData().getTemplates().forEach((templateCursor) -> {
String templateName = templateCursor.key;
templateCursor.value.getMappings().forEach((mappingCursor) -> {
String type = mappingCursor.key;
// there should be the type name at this level, but there was a bug where mappings could be stored without a type (#45120)
// to make sure, we try to detect this like we try to do in MappingMetaData#sourceAsMap()
Map<String, Object> mapping = XContentHelper.convertToMap(mappingCursor.value.compressedReference(), true).v2();
if (mapping.size() == 1 && mapping.containsKey(type)) {
// the type name is the root value, reduce it
mapping = (Map<String, Object>) mapping.get(type);
}
if (mapContainsFieldNamesDisabled(mapping)) {
templatesContainingFieldNames.add(templateName);
}
});
});

if (templatesContainingFieldNames.isEmpty() == false) {
return new DeprecationIssue(DeprecationIssue.Level.CRITICAL, "Index templates contain _field_names settings.",
"https://www.elastic.co/guide/en/elasticsearch/reference/master/breaking-changes-8.0.html#fieldnames-enabling",
"Index templates " + templatesContainingFieldNames + " use the deprecated `enable` setting for the `"
+ FieldNamesFieldMapper.NAME + "` field. Using this setting in new index mappings will throw an error "
+ "in the next major version and needs to be removed from existing mappings and templates.");
}
return null;
}

/**
* check for "_field_names" entries in the map that contain another property "enabled" in the sub-map
*/
static boolean mapContainsFieldNamesDisabled(Map<?, ?> map) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to use a recursive function here, or could we just check for it directly since we know _field_names will be at the top level of the mapping? Also, there is a convenient method IndexDeprecationChecks#findInPropertiesRecursively for iterating over a mapping.

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 opted for the recursive function just to make sure we catch all type-vs-no-type cases (see also the test cases). If we always have (or don't have) a type-level in the internally stored mappings in the templates, then we can probably do without but I wanted to check with you on this, it wasn't completely clear to me from reading the code. The mapping maps are quite forgiving for what you store in them, so it depends on the caller and I didn't completely follow the internal logic.

there is a convenient method IndexDeprecationChecks#findInPropertiesRecursively

I saw and tried to use this, but ran into problems because it assumes things are stored under a properties key which I think the "_field_nams" : { "enabled" : ...} is not (correct me if I'm wrong).

Let me if you know for sure how the internal map structure is supposed to look like (at which level to expect the "_field_names" key) and I can update the tests accordingly and try making this non-recursive.

Copy link
Contributor

Choose a reason for hiding this comment

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

I saw and tried to use this, but ran into problems because it assumes things are stored under a properties key

Ah, I missed this -- then findInPropertiesRecursively is indeed not a good fit.

Let me if you know for sure how the internal map structure is supposed to look like

I left some comments on your individual questions, let me know if anything is still unclear!

Object fieldNamesMapping = map.get(FieldNamesFieldMapper.NAME);
if (fieldNamesMapping != null) {
if (((Map<?, ?>) fieldNamesMapping).keySet().contains("enabled")) {
return true;
}
}
return false;
}

static DeprecationIssue checkPollIntervalTooLow(ClusterState state) {
String pollIntervalString = state.metaData().settings().get(LIFECYCLE_POLL_INTERVAL_SETTING.getKey());
if (Strings.isNullOrEmpty(pollIntervalString)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ private DeprecationChecks() {
Collections.unmodifiableList(Arrays.asList(
ClusterDeprecationChecks::checkUserAgentPipelines,
ClusterDeprecationChecks::checkTemplatesWithTooManyFields,
ClusterDeprecationChecks::checkPollIntervalTooLow
ClusterDeprecationChecks::checkPollIntervalTooLow,
ClusterDeprecationChecks::checkTemplatesWithFieldNamesDisabled
));


Expand All @@ -51,7 +52,8 @@ private DeprecationChecks() {
IndexDeprecationChecks::tooManyFieldsCheck,
IndexDeprecationChecks::chainedMultiFieldsCheck,
IndexDeprecationChecks::deprecatedDateTimeFormat,
IndexDeprecationChecks::translogRetentionSettingCheck
IndexDeprecationChecks::translogRetentionSettingCheck,
IndexDeprecationChecks::fieldNamesDisabledCheck
));

static List<BiFunction<DatafeedConfig, NamedXContentRegistry, DeprecationIssue>> ML_SETTINGS_CHECKS =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@


import com.carrotsearch.hppc.cursors.ObjectCursor;

import org.elasticsearch.Version;
import org.elasticsearch.cluster.metadata.IndexMetaData;
import org.elasticsearch.cluster.metadata.MappingMetaData;
Expand Down Expand Up @@ -185,6 +186,21 @@ private static boolean containsChainedMultiFields(Map<?, ?> property) {
return false;
}

/**
* warn about existing explicit "_field_names" settings in existing mappings
*/
static DeprecationIssue fieldNamesDisabledCheck(IndexMetaData indexMetaData) {
MappingMetaData mapping = indexMetaData.mapping();
if ((mapping != null) && ClusterDeprecationChecks.mapContainsFieldNamesDisabled(mapping.getSourceAsMap())) {
return new DeprecationIssue(DeprecationIssue.Level.WARNING,
"Index mapping contains explicit `_field_names` enabling settings.",
"https://www.elastic.co/guide/en/elasticsearch/reference/master/breaking-changes-8.0.html" +
"#fieldnames-enabling",
"The index mapping contains a deprecated `enabled` setting for `_field_names` that should be removed moving foward.");
}
return null;
}

private static final Set<String> TYPES_THAT_DONT_COUNT;
static {
HashSet<String> typesThatDontCount = new HashSet<>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.index.IndexSettings;
import org.elasticsearch.index.mapper.FieldNamesFieldMapper;
import org.elasticsearch.ingest.IngestService;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.xpack.core.deprecation.DeprecationIssue;
Expand Down Expand Up @@ -158,6 +159,84 @@ public void testTemplateWithTooManyFields() throws IOException {
assertEquals(singletonList(expected), issues);
}

public void testTemplatesWithFieldNamesDisabled() throws IOException {
XContentBuilder goodMappingBuilder = jsonBuilder();
goodMappingBuilder.startObject();
{
goodMappingBuilder.startObject("_doc");
{
goodMappingBuilder.startObject("properties");
{
addRandomFields(10, goodMappingBuilder);
}
goodMappingBuilder.endObject();
}
goodMappingBuilder.endObject();
}
goodMappingBuilder.endObject();
assertFieldNamesEnabledTemplate(goodMappingBuilder, false);

XContentBuilder badMappingBuilder = jsonBuilder();
badMappingBuilder.startObject();
{
// we currently always store a type level internally
badMappingBuilder.startObject("_doc");
{
badMappingBuilder.startObject(FieldNamesFieldMapper.NAME);
{
badMappingBuilder.field("enabled", randomBoolean());
}
badMappingBuilder.endObject();
}
badMappingBuilder.endObject();
}
badMappingBuilder.endObject();
assertFieldNamesEnabledTemplate(badMappingBuilder, true);

// however, there was a bug where mappings could be stored without a type (#45120)
// so we also should try to check these cases

XContentBuilder badMappingWithoutTypeBuilder = jsonBuilder();
badMappingWithoutTypeBuilder.startObject();
{
badMappingWithoutTypeBuilder.startObject(FieldNamesFieldMapper.NAME);
{
badMappingWithoutTypeBuilder.field("enabled", randomBoolean());
}
badMappingWithoutTypeBuilder.endObject();
}
badMappingWithoutTypeBuilder.endObject();
assertFieldNamesEnabledTemplate(badMappingWithoutTypeBuilder, true);
}

private void assertFieldNamesEnabledTemplate(XContentBuilder templateBuilder, boolean expectIssue) throws IOException {
String badTemplateName = randomAlphaOfLength(5);
final ClusterState state = ClusterState.builder(new ClusterName(randomAlphaOfLength(5)))
.metaData(MetaData.builder()
.put(IndexTemplateMetaData.builder(badTemplateName)
.patterns(Collections.singletonList(randomAlphaOfLength(5)))
.putMapping("_doc", Strings.toString(templateBuilder))
.build())
.build())
.build();

List<DeprecationIssue> issues = DeprecationChecks.filterChecks(CLUSTER_SETTINGS_CHECKS, c -> c.apply(state));
if (expectIssue) {
assertEquals(1, issues.size());
DeprecationIssue issue = issues.get(0);
assertEquals(DeprecationIssue.Level.CRITICAL, issue.getLevel());
assertEquals("https://www.elastic.co/guide/en/elasticsearch/reference/master/breaking-changes-8.0.html#fieldnames-enabling"
, issue.getUrl());
assertEquals("Index templates contain _field_names settings.", issue.getMessage());
assertEquals("Index templates [" + badTemplateName + "] "
+ "use the deprecated `enable` setting for the `" + FieldNamesFieldMapper.NAME +
"` field. Using this setting in new index mappings will throw an error in the next major version and " +
"needs to be removed from existing mappings and templates.", issue.getDetails());
} else {
assertTrue(issues.isEmpty());
}
}

public void testPollIntervalTooLow() {
{
final String tooLowInterval = randomTimeValue(1, 999, "ms", "micros", "nanos");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,11 @@
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.joda.JodaDeprecationPatterns;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentFactory;
import org.elasticsearch.index.IndexSettings;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.index.mapper.FieldNamesFieldMapper;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.test.VersionUtils;
import org.elasticsearch.xpack.core.deprecation.DeprecationIssue;
Expand All @@ -27,8 +28,8 @@
import static java.util.Collections.singletonList;
import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;
import static org.elasticsearch.xpack.deprecation.DeprecationChecks.INDEX_SETTINGS_CHECKS;
import static org.hamcrest.Matchers.hasItem;
import static org.hamcrest.Matchers.empty;
import static org.hamcrest.Matchers.hasItem;
import static org.hamcrest.collection.IsIterableContainingInOrder.contains;

public class IndexDeprecationChecksTests extends ESTestCase {
Expand Down Expand Up @@ -161,6 +162,7 @@ public void testChainedMultiFields() throws IOException {
"The names of fields that contain chained multi-fields: [[type: _doc, field: invalid-field]]");
assertEquals(singletonList(expected), issues);
}

public void testDefinedPatternsDoNotWarn() throws IOException {
String simpleMapping = "{\n" +
"\"properties\" : {\n" +
Expand Down Expand Up @@ -412,4 +414,30 @@ public void testDefaultTranslogRetentionSettings() {
List<DeprecationIssue> issues = DeprecationChecks.filterChecks(INDEX_SETTINGS_CHECKS, c -> c.apply(indexMetaData));
assertThat(issues, empty());
}

public void testFieldNamesEnabling() throws IOException {
XContentBuilder xContent = XContentFactory.jsonBuilder().startObject()
.startObject(FieldNamesFieldMapper.NAME)
.field("enabled", randomBoolean())
.endObject()
.endObject();
String mapping = BytesReference.bytes(xContent).utf8ToString();

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

DeprecationIssue issue = issues.get(0);
assertEquals(DeprecationIssue.Level.WARNING, issue.getLevel());
assertEquals("https://www.elastic.co/guide/en/elasticsearch/reference/master/breaking-changes-8.0.html#fieldnames-enabling"
, issue.getUrl());
assertEquals("Index mapping contains explicit `_field_names` enabling settings.", issue.getMessage());
assertEquals("The index mapping contains a deprecated `enabled` setting for `_field_names` that should be removed moving foward.",
issue.getDetails());
}
}