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

Re-add support for some metadata field parameters #118825

Merged
merged 6 commits into from
Dec 18, 2024
+110 −2
Merged

Re-add support for some metadata field parameters #118825

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
de38c9e
Re-add support for some metadata field parameters
cbuescher Dec 16, 2024
e898670
Update docs/changelog/118825.yaml
cbuescher Dec 17, 2024
e8541bc
iter
cbuescher Dec 17, 2024
2f81e13
Merge branch 'main' into revert-116944
cbuescher Dec 18, 2024
3048972
iter
cbuescher Dec 18, 2024
ab3a39b
iter
cbuescher Dec 18, 2024
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
5 changes: 3 additions & 2 deletions docs/changelog/116944.yaml → docs/changelog/118825.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
pr: 116944
pr: 118825
summary: "Remove support for type, fields, `copy_to` and boost in metadata field definition"
area: Mapping
type: breaking
issues: []
breaking:
title: "Remove support for type, fields, copy_to and boost in metadata field definition"
area: Mapping
details: The type, fields, copy_to and boost parameters are no longer supported in metadata field definition
details: The type, fields, copy_to and boost parameters are no longer
supported in metadata field definition starting with version 9.
impact: Users providing type, fields, copy_to or boost as part of metadata field definition should remove them from their mappings.
notable: false
4 changes: 4 additions & 0 deletions server/src/main/java/org/elasticsearch/index/IndexVersions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,10 @@ static Collection<IndexVersion> getAllVersions() {
return VERSION_IDS.values().stream().filter(v -> v.onOrAfter(MINIMUM_COMPATIBLE)).toList();
}

static Collection<IndexVersion> getAllReadOnlyVersions() {
return VERSION_IDS.values().stream().filter(v -> v.onOrAfter(MINIMUM_READONLY_COMPATIBLE) && v.before(MINIMUM_COMPATIBLE)).toList();
}

static final IntFunction<String> VERSION_LOOKUP = ReleaseVersions.generateVersionsLookup(IndexVersions.class, LATEST_DEFINED.id());

// no instance
Expand Down
22 changes: 22 additions & 0 deletions server/src/main/java/org/elasticsearch/index/mapper/MetadataFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,17 @@
package org.elasticsearch.index.mapper;

import org.elasticsearch.common.Explicit;
import org.elasticsearch.common.logging.DeprecationCategory;
import org.elasticsearch.common.util.Maps;
import org.elasticsearch.common.xcontent.support.XContentMapValues;
import org.elasticsearch.index.IndexVersion;
import org.elasticsearch.index.IndexVersions;
import org.elasticsearch.xcontent.XContentBuilder;

import java.io.IOException;
import java.util.Iterator;
import java.util.Map;
import java.util.Set;
import java.util.function.Function;

/**
Expand Down Expand Up @@ -132,6 +136,8 @@ public final MetadataFieldMapper build(MapperBuilderContext context) {
return build();
}

private static final Set<String> UNSUPPORTED_PARAMETERS_8_6_0 = Set.of("type", "fields", "copy_to", "boost");

public final void parseMetadataField(String name, MappingParserContext parserContext, Map<String, Object> fieldNode) {
final Parameter<?>[] params = getParameters();
Map<String, Parameter<?>> paramsMap = Maps.newHashMapWithExpectedSize(params.length);
Expand All @@ -144,6 +150,22 @@ public final void parseMetadataField(String name, MappingParserContext parserCon
final Object propNode = entry.getValue();
Parameter<?> parameter = paramsMap.get(propName);
if (parameter == null) {
IndexVersion indexVersionCreated = parserContext.indexVersionCreated();
if (indexVersionCreated.before(IndexVersions.UPGRADE_TO_LUCENE_10_0_0)
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 entirely sure if UPGRADE_TO_LUCENE_10_0_0 should be the point in time we should start to throw errors. We first removed support for these parameters with #116944 (8a7491c) at which point we already have more recent IndexVersions (see

elasticsearch/server/src/main/java/org/elasticsearch/index/IndexVersions.java

Line 135 in 8a7491c

public static final IndexVersion LOGSDB_DEFAULT_IGNORE_DYNAMIC_BEYOND_LIMIT = def(9_001_00_0, Version.LUCENE_10_0_0);
). To me that means indices with e.g. IndexVersion TIME_BASED_K_ORDERED_DOC_ID could still use those parameters? Please keep me honest here.

Copy link
Member

Choose a reason for hiding this comment

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

good point. Tricky question. I don't expect it to matter a lot because these unsupported params should not be common at all. The removal affects only serverless users indeed, but I'm afraid it can't be associated to a specific index version. If we meant to do so we'd have needed to add an index versions associated with the removal?

I am fine with using what you are using, or in alternative adding a new index version now.

&& UNSUPPORTED_PARAMETERS_8_6_0.contains(propName)) {
if (indexVersionCreated.onOrAfter(IndexVersions.V_8_6_0)) {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should leave the deprecation under that conditional. What do you think?

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 sure I understand entirely. Under which conditions do you think we should issue a deprecation? As I see this atm before 8.6 we silently ignore, from 8.6 to 9 (or rather IndexVersions.UPGRADE_TO_LUCENE_10_0_0) we issue a deprecation and afterwards we throw. Do you mean reorganizing code to reflect that better or different behaviour?

Copy link
Member

Choose a reason for hiding this comment

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

I think I misread, we are good then.

// silently ignore type, and a few other parameters: sadly we've been doing this for a long time
deprecationLogger.warn(
DeprecationCategory.API,
propName,
"Parameter [{}] has no effect on metadata field [{}] and will be removed in future",
propName,
name
);
}
iterator.remove();
continue;
}
throw new MapperParsingException("unknown parameter [" + propName + "] on metadata field [" + name + "]");
}
parameter.parse(name, parserContext, propNode);
Expand Down
2 changes: 2 additions & 0 deletions test/framework/src/main/java/org/elasticsearch/index/KnownIndexVersions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,6 @@ public class KnownIndexVersions {
* A sorted list of all known transport versions
*/
public static final List<IndexVersion> ALL_VERSIONS = List.copyOf(IndexVersions.getAllVersions());

public static final List<IndexVersion> ALL_READ_ONLY_VERSIONS = List.copyOf(IndexVersions.getAllReadOnlyVersions());
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 entirely sure why we need this list copy but since thats the pattern for ALL_VERSIONS I thought its save to do the same. Maybe someone know why we need that for ALL_VERSIONS?

Copy link
Contributor

Choose a reason for hiding this comment

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

By checking its history a bit, I think, the initial intention was to make a defensive copy of the list, which is immutable in the current version. The list was also unmodifiable when it was first introduced: #93384

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we need to have a list with read only versions. Could we instead just add a new method to get read only versions? Also, should we instead have a new method that returns all versions, writeable and read-only, as opposed to having special tests for read-only indices? My expectations it that the majority of the cases will need read support, and no write, hence the special case should probably be for writing and not for reading.

Copy link
Member

Choose a reason for hiding this comment

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

I think what I said above is necessarily correct. Switching randomizations to include read-only version cause quite a bit of issues. It makes sense to add for now a new list, we'll clean it up later. A question remains if that list should contain all versions including readonly or only read-only versions.

}
84 changes: 84 additions & 0 deletions test/framework/src/main/java/org/elasticsearch/index/mapper/MetadataMapperTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
import org.elasticsearch.common.compress.CompressedXContent;
import org.elasticsearch.core.CheckedConsumer;
import org.elasticsearch.index.IndexVersion;
import org.elasticsearch.index.IndexVersions;
import org.elasticsearch.index.KnownIndexVersions;
import org.elasticsearch.index.mapper.MapperService.MergeReason;
import org.elasticsearch.test.index.IndexVersionUtils;
import org.elasticsearch.xcontent.XContentBuilder;
Expand Down Expand Up @@ -142,4 +144,86 @@ public final void testFixedMetaFieldsAreNotConfigurable() throws IOException {
);
assertEquals("Failed to parse mapping: " + fieldName() + " is not configurable", exception.getMessage());
}

public void testTypeAndFriendsAreAcceptedBefore_8_6_0() throws IOException {
assumeTrue("Metadata field " + fieldName() + " isn't configurable", isConfigurable());
IndexVersion previousVersion = IndexVersionUtils.getPreviousVersion(IndexVersions.V_8_6_0);
// we randomly also pick read-only versions to test that we can still parse the parameters for them
IndexVersion version = randomBoolean()
? randomFrom(KnownIndexVersions.ALL_READ_ONLY_VERSIONS)
: IndexVersionUtils.randomVersionBetween(random(), IndexVersions.MINIMUM_COMPATIBLE, previousVersion);
assumeTrue("Metadata field " + fieldName() + " is not supported on version " + version, isSupportedOn(version));
MapperService mapperService = createMapperService(version, mapping(b -> {}));
// these parameters were previously silently ignored, they will still be ignored in existing indices
String[] unsupportedParameters = new String[] { "fields", "copy_to", "boost", "type" };
for (String param : unsupportedParameters) {
String mappingAsString = "{\n"
+ " \"_doc\" : {\n"
+ " \""
+ fieldName()
+ "\" : {\n"
+ " \""
+ param
+ "\" : \"any\"\n"
+ " }\n"
+ " }\n"
+ "}";
assertNotNull(mapperService.parseMapping("_doc", MergeReason.MAPPING_UPDATE, new CompressedXContent(mappingAsString)));
}
}

public void testTypeAndFriendsAreDeprecatedFrom_8_6_0_TO_9_0_0() throws IOException {
assumeTrue("Metadata field " + fieldName() + " isn't configurable", isConfigurable());
IndexVersion previousVersion = IndexVersionUtils.getPreviousVersion(IndexVersions.UPGRADE_TO_LUCENE_10_0_0);
IndexVersion version = IndexVersionUtils.randomVersionBetween(random(), IndexVersions.V_8_6_0, previousVersion);
assumeTrue("Metadata field " + fieldName() + " is not supported on version " + version, isSupportedOn(version));
MapperService mapperService = createMapperService(version, mapping(b -> {}));
// these parameters were deprecated, they now should throw an error in new indices
String[] unsupportedParameters = new String[] { "fields", "copy_to", "boost", "type" };
for (String param : unsupportedParameters) {
String mappingAsString = "{\n"
+ " \"_doc\" : {\n"
+ " \""
+ fieldName()
+ "\" : {\n"
+ " \""
+ param
+ "\" : \"any\"\n"
+ " }\n"
+ " }\n"
+ "}";
assertNotNull(mapperService.parseMapping("_doc", MergeReason.MAPPING_UPDATE, new CompressedXContent(mappingAsString)));
assertWarnings("Parameter [" + param + "] has no effect on metadata field [" + fieldName() + "] and will be removed in future");
}
}

public void testTypeAndFriendsThrow_After_9_0_0() throws IOException {
assumeTrue("Metadata field " + fieldName() + " isn't configurable", isConfigurable());
IndexVersion version = IndexVersionUtils.randomVersionBetween(
random(),
IndexVersions.UPGRADE_TO_LUCENE_10_0_0,
IndexVersion.current()
);
assumeTrue("Metadata field " + fieldName() + " is not supported on version " + version, isSupportedOn(version));
MapperService mapperService = createMapperService(version, mapping(b -> {}));
// these parameters were previously silently ignored, they are now deprecated in new indices
String[] unsupportedParameters = new String[] { "fields", "copy_to", "boost", "type" };
for (String param : unsupportedParameters) {
String mappingAsString = "{\n"
+ " \"_doc\" : {\n"
+ " \""
+ fieldName()
+ "\" : {\n"
+ " \""
+ param
+ "\" : \"any\"\n"
+ " }\n"
+ " }\n"
+ "}";
expectThrows(
MapperParsingException.class,
() -> mapperService.parseMapping("_doc", MergeReason.MAPPING_UPDATE, new CompressedXContent(mappingAsString))
);
}
}
}