From 04bf491c4c9493fbf8711689105b98d93fdaafd8 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Thu, 7 Jul 2022 20:03:11 -0400 Subject: [PATCH 1/8] Restore camel case support for legacy indices --- .../metadata/IndexMetadataVerifier.java | 1 + .../common/time/LegacyFormatNames.java | 114 ++++++++++++++++++ .../index/mapper/DateFieldMapper.java | 26 +++- .../index/mapper/FieldMapper.java | 13 +- 4 files changed, 147 insertions(+), 7 deletions(-) create mode 100644 server/src/main/java/org/elasticsearch/common/time/LegacyFormatNames.java diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadataVerifier.java b/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadataVerifier.java index 0b84d0edf397d..2e4ff0ec1ce78 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadataVerifier.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadataVerifier.java @@ -197,6 +197,7 @@ public Set> entrySet() { mapperService.merge(indexMetadata, MapperService.MergeReason.MAPPING_RECOVERY); } } catch (Exception ex) { + logger.error("Failed to parse mappings for index [" + indexMetadata.getIndex() + "]", ex); // Wrap the inner exception so we have the index name in the exception message throw new IllegalStateException("Failed to parse mappings for index [" + indexMetadata.getIndex() + "]", ex); } diff --git a/server/src/main/java/org/elasticsearch/common/time/LegacyFormatNames.java b/server/src/main/java/org/elasticsearch/common/time/LegacyFormatNames.java new file mode 100644 index 0000000000000..06370078da816 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/common/time/LegacyFormatNames.java @@ -0,0 +1,114 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.common.time; + +import java.util.Arrays; +import java.util.Map; +import java.util.stream.Collectors; + +public enum LegacyFormatNames { + ISO8601(null, "iso8601"), + BASIC_DATE("basicDate", "basic_date"), + BASIC_DATE_TIME("basicDateTime", "basic_date_time"), + BASIC_DATE_TIME_NO_MILLIS("basicDateTimeNoMillis", "basic_date_time_no_millis"), + BASIC_ORDINAL_DATE("basicOrdinalDate", "basic_ordinal_date"), + BASIC_ORDINAL_DATE_TIME("basicOrdinalDateTime", "basic_ordinal_date_time"), + BASIC_ORDINAL_DATE_TIME_NO_MILLIS("basicOrdinalDateTimeNoMillis", "basic_ordinal_date_time_no_millis"), + BASIC_TIME("basicTime", "basic_time"), + BASIC_TIME_NO_MILLIS("basicTimeNoMillis", "basic_time_no_millis"), + BASIC_T_TIME("basicTTime", "basic_t_time"), + BASIC_T_TIME_NO_MILLIS("basicTTimeNoMillis", "basic_t_time_no_millis"), + BASIC_WEEK_DATE("basicWeekDate", "basic_week_date"), + BASIC_WEEK_DATE_TIME("basicWeekDateTime", "basic_week_date_time"), + BASIC_WEEK_DATE_TIME_NO_MILLIS("basicWeekDateTimeNoMillis", "basic_week_date_time_no_millis"), + DATE(null, "date"), + DATE_HOUR("dateHour", "date_hour"), + DATE_HOUR_MINUTE("dateHourMinute", "date_hour_minute"), + DATE_HOUR_MINUTE_SECOND("dateHourMinuteSecond", "date_hour_minute_second"), + DATE_HOUR_MINUTE_SECOND_FRACTION("dateHourMinuteSecondFraction", "date_hour_minute_second_fraction"), + DATE_HOUR_MINUTE_SECOND_MILLIS("dateHourMinuteSecondMillis", "date_hour_minute_second_millis"), + DATE_OPTIONAL_TIME("dateOptionalTime", "date_optional_time"), + DATE_TIME("dateTime", "date_time"), + DATE_TIME_NO_MILLIS("dateTimeNoMillis", "date_time_no_millis"), + HOUR(null, "hour"), + HOUR_MINUTE("hourMinute", "hour_minute"), + HOUR_MINUTE_SECOND("hourMinuteSecond", "hour_minute_second"), + HOUR_MINUTE_SECOND_FRACTION("hourMinuteSecondFraction", "hour_minute_second_fraction"), + HOUR_MINUTE_SECOND_MILLIS("hourMinuteSecondMillis", "hour_minute_second_millis"), + ORDINAL_DATE("ordinalDate", "ordinal_date"), + ORDINAL_DATE_TIME("ordinalDateTime", "ordinal_date_time"), + ORDINAL_DATE_TIME_NO_MILLIS("ordinalDateTimeNoMillis", "ordinal_date_time_no_millis"), + TIME(null, "time"), + TIME_NO_MILLIS("timeNoMillis", "time_no_millis"), + T_TIME("tTime", "t_time"), + T_TIME_NO_MILLIS("tTimeNoMillis", "t_time_no_millis"), + WEEK_DATE("weekDate", "week_date"), + WEEK_DATE_TIME("weekDateTime", "week_date_time"), + WEEK_DATE_TIME_NO_MILLIS("weekDateTimeNoMillis", "week_date_time_no_millis"), + WEEK_YEAR(null, "week_year"), + WEEKYEAR(null, "weekyear"), + WEEK_YEAR_WEEK("weekyearWeek", "weekyear_week"), + WEEKYEAR_WEEK_DAY("weekyearWeekDay", "weekyear_week_day"), + YEAR(null, "year"), + YEAR_MONTH("yearMonth", "year_month"), + YEAR_MONTH_DAY("yearMonthDay", "year_month_day"), + EPOCH_SECOND(null, "epoch_second"), + EPOCH_MILLIS(null, "epoch_millis"), + // strict date formats here, must be at least 4 digits for year and two for months and two for day" + STRICT_BASIC_WEEK_DATE("strictBasicWeekDate", "strict_basic_week_date"), + STRICT_BASIC_WEEK_DATE_TIME("strictBasicWeekDateTime", "strict_basic_week_date_time"), + STRICT_BASIC_WEEK_DATE_TIME_NO_MILLIS("strictBasicWeekDateTimeNoMillis", "strict_basic_week_date_time_no_millis"), + STRICT_DATE("strictDate", "strict_date"), + STRICT_DATE_HOUR("strictDateHour", "strict_date_hour"), + STRICT_DATE_HOUR_MINUTE("strictDateHourMinute", "strict_date_hour_minute"), + STRICT_DATE_HOUR_MINUTE_SECOND("strictDateHourMinuteSecond", "strict_date_hour_minute_second"), + STRICT_DATE_HOUR_MINUTE_SECOND_FRACTION("strictDateHourMinuteSecondFraction", "strict_date_hour_minute_second_fraction"), + STRICT_DATE_HOUR_MINUTE_SECOND_MILLIS("strictDateHourMinuteSecondMillis", "strict_date_hour_minute_second_millis"), + STRICT_DATE_OPTIONAL_TIME("strictDateOptionalTime", "strict_date_optional_time"), + STRICT_DATE_OPTIONAL_TIME_NANOS("strictDateOptionalTimeNanos", "strict_date_optional_time_nanos"), + STRICT_DATE_TIME("strictDateTime", "strict_date_time"), + STRICT_DATE_TIME_NO_MILLIS("strictDateTimeNoMillis", "strict_date_time_no_millis"), + STRICT_HOUR("strictHour", "strict_hour"), + STRICT_HOUR_MINUTE("strictHourMinute", "strict_hour_minute"), + STRICT_HOUR_MINUTE_SECOND("strictHourMinuteSecond", "strict_hour_minute_second"), + STRICT_HOUR_MINUTE_SECOND_FRACTION("strictHourMinuteSecondFraction", "strict_hour_minute_second_fraction"), + STRICT_HOUR_MINUTE_SECOND_MILLIS("strictHourMinuteSecondMillis", "strict_hour_minute_second_millis"), + STRICT_ORDINAL_DATE("strictOrdinalDate", "strict_ordinal_date"), + STRICT_ORDINAL_DATE_TIME("strictOrdinalDateTime", "strict_ordinal_date_time"), + STRICT_ORDINAL_DATE_TIME_NO_MILLIS("strictOrdinalDateTimeNoMillis", "strict_ordinal_date_time_no_millis"), + STRICT_TIME("strictTime", "strict_time"), + STRICT_TIME_NO_MILLIS("strictTimeNoMillis", "strict_time_no_millis"), + STRICT_T_TIME("strictTTime", "strict_t_time"), + STRICT_T_TIME_NO_MILLIS("strictTTimeNoMillis", "strict_t_time_no_millis"), + STRICT_WEEK_DATE("strictWeekDate", "strict_week_date"), + STRICT_WEEK_DATE_TIME("strictWeekDateTime", "strict_week_date_time"), + STRICT_WEEK_DATE_TIME_NO_MILLIS("strictWeekDateTimeNoMillis", "strict_week_date_time_no_millis"), + STRICT_WEEKYEAR("strictWeekyear", "strict_weekyear"), + STRICT_WEEKYEAR_WEEK("strictWeekyearWeek", "strict_weekyear_week"), + STRICT_WEEKYEAR_WEEK_DAY("strictWeekyearWeekDay", "strict_weekyear_week_day"), + STRICT_YEAR("strictYear", "strict_year"), + STRICT_YEAR_MONTH("strictYearMonth", "strict_year_month"), + STRICT_YEAR_MONTH_DAY("strictYearMonthDay", "strict_year_month_day"); + + private static final Map ALL_NAMES = Arrays.stream(values()) + .filter(n -> n.camelCaseName != null) + .collect(Collectors.toMap(n -> n.camelCaseName, n -> n.snakeCaseName)); + + private final String camelCaseName; + private final String snakeCaseName; + + LegacyFormatNames(String camelCaseName, String snakeCaseName) { + this.camelCaseName = camelCaseName; + this.snakeCaseName = snakeCaseName; + } + + public static String compatibleFormat(String format) { + return ALL_NAMES.getOrDefault(format, format); + } +} diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java index a7cadc7e40a8b..018181ac48d89 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java @@ -30,7 +30,9 @@ import org.elasticsearch.common.time.DateFormatters; import org.elasticsearch.common.time.DateMathParser; import org.elasticsearch.common.time.DateUtils; +import org.elasticsearch.common.time.LegacyFormatNames; import org.elasticsearch.common.util.LocaleUtils; +import org.elasticsearch.common.xcontent.support.XContentMapValues; import org.elasticsearch.core.Nullable; import org.elasticsearch.core.TimeValue; import org.elasticsearch.index.fielddata.IndexFieldData; @@ -269,12 +271,24 @@ public Builder( DateFormatter defaultFormat = resolution == Resolution.MILLISECONDS ? DEFAULT_DATE_TIME_FORMATTER : DEFAULT_DATE_TIME_NANOS_FORMATTER; - this.format = Parameter.stringParam( - "format", - indexCreatedVersion.isLegacyIndexVersion(), - m -> toType(m).format, - defaultFormat.pattern() - ); + + if (indexCreatedVersion.major <= Version.CURRENT.previousMajor().major) { + this.format = Parameter.stringParam( + "format", + indexCreatedVersion.isLegacyIndexVersion(), + (n, c, o) -> LegacyFormatNames.compatibleFormat(XContentMapValues.nodeStringValue(o)), + m -> toType(m).format, + defaultFormat.pattern(), + XContentBuilder::field + ); + } else { + this.format = Parameter.stringParam( + "format", + indexCreatedVersion.isLegacyIndexVersion(), + m -> toType(m).format, + defaultFormat.pattern() + ); + } if (dateFormatter != null) { this.format.setValue(dateFormatter.pattern()); this.locale.setValue(dateFormatter.locale()); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java index acf5053fd6a4e..7db4dd53fc9bf 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java @@ -878,12 +878,23 @@ public static Parameter stringParam( Function initializer, String defaultValue, Serializer serializer + ) { + return stringParam(name, updateable, (n, c, o) -> XContentMapValues.nodeStringValue(o), initializer, defaultValue, serializer); + } + + public static Parameter stringParam( + String name, + boolean updateable, + TriFunction parser, + Function initializer, + String defaultValue, + Serializer serializer ) { return new Parameter<>( name, updateable, defaultValue == null ? () -> null : () -> defaultValue, - (n, c, o) -> XContentMapValues.nodeStringValue(o), + parser, initializer, serializer, Function.identity() From 045be441d16ca7646eecc37ea8b398f946d14a86 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Fri, 8 Jul 2022 17:58:33 -0400 Subject: [PATCH 2/8] Add tests --- .../index/mapper/DateFieldMapper.java | 3 +- .../index/mapper/DateFieldMapperTests.java | 45 +++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java index 018181ac48d89..a49eddcc6e277 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java @@ -295,7 +295,8 @@ public Builder( } } - private DateFormatter buildFormatter() { + // package private for testing + DateFormatter buildFormatter() { try { return DateFormatter.forPattern(format.getValue()).withLocale(locale.getValue()); } catch (IllegalArgumentException e) { diff --git a/server/src/test/java/org/elasticsearch/index/mapper/DateFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/DateFieldMapperTests.java index 186c43616432c..60138370ad8c8 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/DateFieldMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/DateFieldMapperTests.java @@ -18,6 +18,7 @@ import org.elasticsearch.index.mapper.DateFieldMapper.DateFieldType; import org.elasticsearch.index.termvectors.TermVectorsService; import org.elasticsearch.script.DateFieldScript; +import org.elasticsearch.script.ScriptService; import org.elasticsearch.search.DocValueFormat; import org.elasticsearch.xcontent.XContentBuilder; @@ -41,6 +42,7 @@ import static org.hamcrest.Matchers.lessThan; import static org.hamcrest.Matchers.lessThanOrEqualTo; import static org.hamcrest.Matchers.notNullValue; +import static org.mockito.Mockito.mock; public class DateFieldMapperTests extends MapperTestCase { @@ -724,4 +726,47 @@ public void testLegacyField() throws Exception { assertThat(service.fieldType("mydate"), instanceOf(DateFieldType.class)); assertNotEquals(DEFAULT_DATE_TIME_FORMATTER, ((DateFieldType) service.fieldType("mydate")).dateTimeFormatter); } + + public void testLegacyDateFormatName() { + DateFieldMapper.Builder builder = new DateFieldMapper.Builder( + "format", + DateFieldMapper.Resolution.MILLISECONDS, + null, + mock(ScriptService.class), + true, + Version.CURRENT.minimumIndexCompatibilityVersion() // BWC compatible index, e.g 7.x + ); + + // Check that we allow the use of camel case date formats on 7.x indices + @SuppressWarnings("unchecked") + FieldMapper.Parameter formatParam = (FieldMapper.Parameter) builder.getParameters()[3]; + formatParam.parse("date_time_format", mock(MappingParserContext.class), "strictDateOptionalTime"); + assertEquals("strict_date_optional_time", formatParam.getValue()); + DateFormatter formatter = builder.buildFormatter(); // shouldn't throw exception + + assertEquals("format[strict_date_optional_time] locale[]", formatter.toString()); + + DateFieldMapper.Builder newFieldBuilder = new DateFieldMapper.Builder( + "format", + DateFieldMapper.Resolution.MILLISECONDS, + null, + mock(ScriptService.class), + true, + Version.CURRENT + ); + + @SuppressWarnings("unchecked") + final FieldMapper.Parameter newFormatParam = (FieldMapper.Parameter) newFieldBuilder.getParameters()[3]; + + // Check that we don't allow the use of camel case date formats on 8.x indices + assertEquals( + "Error parsing [format] on field [format]: Invalid format: [strictDateOptionalTime]: Unknown pattern letter: t", + expectThrows(IllegalArgumentException.class, () -> { + newFormatParam.parse("date_time_format", mock(MappingParserContext.class), "strictDateOptionalTime"); + assertEquals("strictDateOptionalTime", newFormatParam.getValue()); + newFieldBuilder.buildFormatter(); + }).getMessage() + ); + + } } From 622bb43e8689aad94b06f7fb03329052abd06c52 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski <6207777+grcevski@users.noreply.github.com> Date: Fri, 8 Jul 2022 18:06:40 -0400 Subject: [PATCH 3/8] Update docs/changelog/88400.yaml --- docs/changelog/88400.yaml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 docs/changelog/88400.yaml diff --git a/docs/changelog/88400.yaml b/docs/changelog/88400.yaml new file mode 100644 index 0000000000000..44ac796abdde0 --- /dev/null +++ b/docs/changelog/88400.yaml @@ -0,0 +1,6 @@ +pr: 88400 +summary: Fix unknown camel case date-time mappings +area: Infra/Core +type: bug +issues: + - 84199 From c6493b48b2f3e08bdf29dd873b537c41187c57ce Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Mon, 11 Jul 2022 13:26:25 -0400 Subject: [PATCH 4/8] WIP: adding format upgrader --- .../metadata/IndexMetadataVerifier.java | 8 +++--- .../index/mapper/MapperService.java | 27 +++++++++++++++++++ 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadataVerifier.java b/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadataVerifier.java index 2e4ff0ec1ce78..2a8298967e2e7 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadataVerifier.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadataVerifier.java @@ -90,7 +90,7 @@ public IndexMetadata verifyIndexMetadata(IndexMetadata indexMetadata, Version mi // Next we have to run this otherwise if we try to create IndexSettings // with broken settings it would fail in checkMappingsCompatibility newMetadata = archiveBrokenIndexSettings(newMetadata); - checkMappingsCompatibility(newMetadata); + newMetadata = checkMappingsCompatibility(newMetadata); return newMetadata; } @@ -128,7 +128,7 @@ private static void checkSupportedVersion(IndexMetadata indexMetadata, Version m * policy guarantees we can read mappings from previous compatible index versions. A failure here would * indicate a compatibility bug (which are unfortunately not that uncommon). */ - private void checkMappingsCompatibility(IndexMetadata indexMetadata) { + private IndexMetadata checkMappingsCompatibility(IndexMetadata indexMetadata) { try { // We cannot instantiate real analysis server or similarity service at this point because the node @@ -194,13 +194,15 @@ public Set> entrySet() { indexSettings.getMode().buildNoFieldDataIdFieldMapper(), scriptService ); - mapperService.merge(indexMetadata, MapperService.MergeReason.MAPPING_RECOVERY); + indexMetadata = mapperService.mergeAndUpgrade(indexMetadata, MapperService.MergeReason.MAPPING_RECOVERY); } } catch (Exception ex) { logger.error("Failed to parse mappings for index [" + indexMetadata.getIndex() + "]", ex); // Wrap the inner exception so we have the index name in the exception message throw new IllegalStateException("Failed to parse mappings for index [" + indexMetadata.getIndex() + "]", ex); } + + return indexMetadata; } /** diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index 1c6e36c8e2b5a..b0aa4cf7650f7 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -18,6 +18,7 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.time.DateFormatter; import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; +import org.elasticsearch.common.xcontent.XContentHelper; import org.elasticsearch.index.AbstractIndexComponent; import org.elasticsearch.index.IndexSettings; import org.elasticsearch.index.analysis.AnalysisRegistry; @@ -338,6 +339,32 @@ public void merge(IndexMetadata indexMetadata, MergeReason reason) { } } + public IndexMetadata mergeAndUpgrade(IndexMetadata indexMetadata, MergeReason reason) { + assert reason != MergeReason.MAPPING_UPDATE_PREFLIGHT; + MappingMetadata mappingMetadata = indexMetadata.mapping(); + if (mappingMetadata != null) { + var docMapper = merge(mappingMetadata.type(), mappingMetadata.source(), reason); + var mappingSource = upgradeFormats(mappingMetadata, docMapper.mappingSource()); + if (mappingMetadata.source().equals(mappingSource) == false) { + return IndexMetadata.builder(indexMetadata).putMapping(new MappingMetadata(mappingSource)).build(); + } + } + return indexMetadata; + } + + @SuppressWarnings("unchecked") + CompressedXContent upgradeFormats(MappingMetadata mappingMetadata, CompressedXContent newMappingSource) { + Map sourceMap = mappingMetadata.getSourceAsMap(); + Map newMap = XContentHelper.convertToMap(newMappingSource.compressedReference(), true).v2(); + + Map properties = (Map)sourceMap.get("properties"); + if (properties != null) { + System.out.println(properties); + } + + return mappingMetadata.source(); + } + public DocumentMapper merge(String type, CompressedXContent mappingSource, MergeReason reason) { final DocumentMapper currentMapper = this.mapper; if (currentMapper != null && currentMapper.mappingSource().equals(mappingSource)) { From 787ca2d42654826c1bbc116bffbb964d8fa784c1 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Tue, 12 Jul 2022 17:17:39 -0400 Subject: [PATCH 5/8] Patch format --- .../index/mapper/MapperService.java | 46 +++++++++++++++---- 1 file changed, 36 insertions(+), 10 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index b0aa4cf7650f7..dce348b88bc5e 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -344,25 +344,51 @@ public IndexMetadata mergeAndUpgrade(IndexMetadata indexMetadata, MergeReason re MappingMetadata mappingMetadata = indexMetadata.mapping(); if (mappingMetadata != null) { var docMapper = merge(mappingMetadata.type(), mappingMetadata.source(), reason); - var mappingSource = upgradeFormats(mappingMetadata, docMapper.mappingSource()); - if (mappingMetadata.source().equals(mappingSource) == false) { - return IndexMetadata.builder(indexMetadata).putMapping(new MappingMetadata(mappingSource)).build(); + var mappingSource = upgradeFormats(mappingMetadata.type(), mappingMetadata.source(), docMapper.mappingSource()); + Map origMap = XContentHelper.convertToMap(mappingMetadata.source().compressedReference(), true).v2(); + + if (origMap.equals(mappingSource) == false) { + return IndexMetadata.builder(indexMetadata).putMapping(new MappingMetadata(mappingMetadata.type(), mappingSource)).build(); } } return indexMetadata; } @SuppressWarnings("unchecked") - CompressedXContent upgradeFormats(MappingMetadata mappingMetadata, CompressedXContent newMappingSource) { - Map sourceMap = mappingMetadata.getSourceAsMap(); - Map newMap = XContentHelper.convertToMap(newMappingSource.compressedReference(), true).v2(); + private Map getProperties(Map sourceMap, String type) { + Map typeMap = (Map) sourceMap.get(type); + if (typeMap == null) { + return null; + } + + return (Map) typeMap.get("properties"); + } - Map properties = (Map)sourceMap.get("properties"); - if (properties != null) { - System.out.println(properties); + @SuppressWarnings({"unchecked", "rawtypes"}) + Map upgradeFormats(String type, CompressedXContent mappingSource, CompressedXContent newMappingSource) { + Map sourceMap = XContentHelper.convertToMap(mappingSource.compressedReference(), true).v2(); + Map newMap = XContentHelper.convertToMap(newMappingSource.compressedReference(), true).v2(); + + Map properties = getProperties(sourceMap, type); + Map parsedProperties = getProperties(newMap, type); + if (properties != null && parsedProperties != null) { + for (var property : properties.entrySet()) { + if (property.getValue() instanceof Map map) { + Object format = map.get("format"); + if (format != null) { + var newProperty = parsedProperties.get(property.getKey()); + if (newProperty instanceof Map parsedMap) { + Object parsedFormat = parsedMap.get("format"); + if (parsedFormat != null && parsedFormat.equals(format) == false) { + map.put("format", parsedFormat); + } + } + } + } + } } - return mappingMetadata.source(); + return sourceMap; } public DocumentMapper merge(String type, CompressedXContent mappingSource, MergeReason reason) { From fd519b98fb46345f0ac62f46d65ead5408baf70f Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Wed, 13 Jul 2022 12:42:39 -0400 Subject: [PATCH 6/8] Address feedback --- .../common/time/LegacyFormatNames.java | 4 +- .../index/mapper/DateFieldMapper.java | 2 +- .../index/mapper/MapperService.java | 51 +++---------- .../index/mapper/MapperServiceUpgraders.java | 74 +++++++++++++++++++ 4 files changed, 86 insertions(+), 45 deletions(-) create mode 100644 server/src/main/java/org/elasticsearch/index/mapper/MapperServiceUpgraders.java diff --git a/server/src/main/java/org/elasticsearch/common/time/LegacyFormatNames.java b/server/src/main/java/org/elasticsearch/common/time/LegacyFormatNames.java index 06370078da816..24097c09ccc20 100644 --- a/server/src/main/java/org/elasticsearch/common/time/LegacyFormatNames.java +++ b/server/src/main/java/org/elasticsearch/common/time/LegacyFormatNames.java @@ -60,7 +60,7 @@ public enum LegacyFormatNames { YEAR_MONTH_DAY("yearMonthDay", "year_month_day"), EPOCH_SECOND(null, "epoch_second"), EPOCH_MILLIS(null, "epoch_millis"), - // strict date formats here, must be at least 4 digits for year and two for months and two for day" + // strict date formats here, must be at least 4 digits for year and two for months and two for day STRICT_BASIC_WEEK_DATE("strictBasicWeekDate", "strict_basic_week_date"), STRICT_BASIC_WEEK_DATE_TIME("strictBasicWeekDateTime", "strict_basic_week_date_time"), STRICT_BASIC_WEEK_DATE_TIME_NO_MILLIS("strictBasicWeekDateTimeNoMillis", "strict_basic_week_date_time_no_millis"), @@ -108,7 +108,7 @@ public enum LegacyFormatNames { this.snakeCaseName = snakeCaseName; } - public static String compatibleFormat(String format) { + public static String camelCaseToSnakeCase(String format) { return ALL_NAMES.getOrDefault(format, format); } } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java index a49eddcc6e277..9160c60165a00 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java @@ -276,7 +276,7 @@ public Builder( this.format = Parameter.stringParam( "format", indexCreatedVersion.isLegacyIndexVersion(), - (n, c, o) -> LegacyFormatNames.compatibleFormat(XContentMapValues.nodeStringValue(o)), + (n, c, o) -> LegacyFormatNames.camelCaseToSnakeCase(XContentMapValues.nodeStringValue(o)), m -> toType(m).format, defaultFormat.pattern(), XContentBuilder::field diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index dce348b88bc5e..919cf82032dea 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -18,7 +18,6 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.time.DateFormatter; import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; -import org.elasticsearch.common.xcontent.XContentHelper; import org.elasticsearch.index.AbstractIndexComponent; import org.elasticsearch.index.IndexSettings; import org.elasticsearch.index.analysis.AnalysisRegistry; @@ -344,51 +343,19 @@ public IndexMetadata mergeAndUpgrade(IndexMetadata indexMetadata, MergeReason re MappingMetadata mappingMetadata = indexMetadata.mapping(); if (mappingMetadata != null) { var docMapper = merge(mappingMetadata.type(), mappingMetadata.source(), reason); - var mappingSource = upgradeFormats(mappingMetadata.type(), mappingMetadata.source(), docMapper.mappingSource()); - Map origMap = XContentHelper.convertToMap(mappingMetadata.source().compressedReference(), true).v2(); - - if (origMap.equals(mappingSource) == false) { - return IndexMetadata.builder(indexMetadata).putMapping(new MappingMetadata(mappingMetadata.type(), mappingSource)).build(); - } - } - return indexMetadata; - } - - @SuppressWarnings("unchecked") - private Map getProperties(Map sourceMap, String type) { - Map typeMap = (Map) sourceMap.get(type); - if (typeMap == null) { - return null; - } + if (indexVersionCreated.major <= Version.CURRENT.previousMajor().major) { + var upgradedSource = MapperServiceUpgraders.upgradeFormatsIfNeeded( + mappingMetadata.type(), + mappingMetadata, + docMapper.mappingSource() + ); - return (Map) typeMap.get("properties"); - } - - @SuppressWarnings({"unchecked", "rawtypes"}) - Map upgradeFormats(String type, CompressedXContent mappingSource, CompressedXContent newMappingSource) { - Map sourceMap = XContentHelper.convertToMap(mappingSource.compressedReference(), true).v2(); - Map newMap = XContentHelper.convertToMap(newMappingSource.compressedReference(), true).v2(); - - Map properties = getProperties(sourceMap, type); - Map parsedProperties = getProperties(newMap, type); - if (properties != null && parsedProperties != null) { - for (var property : properties.entrySet()) { - if (property.getValue() instanceof Map map) { - Object format = map.get("format"); - if (format != null) { - var newProperty = parsedProperties.get(property.getKey()); - if (newProperty instanceof Map parsedMap) { - Object parsedFormat = parsedMap.get("format"); - if (parsedFormat != null && parsedFormat.equals(format) == false) { - map.put("format", parsedFormat); - } - } - } + if (mappingMetadata.source().equals(upgradedSource) == false) { + return IndexMetadata.builder(indexMetadata).putMapping(new MappingMetadata(upgradedSource)).build(); } } } - - return sourceMap; + return indexMetadata; } public DocumentMapper merge(String type, CompressedXContent mappingSource, MergeReason reason) { diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperServiceUpgraders.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperServiceUpgraders.java new file mode 100644 index 0000000000000..7052128bb7c5d --- /dev/null +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperServiceUpgraders.java @@ -0,0 +1,74 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.index.mapper; + +import org.elasticsearch.cluster.metadata.MappingMetadata; +import org.elasticsearch.common.compress.CompressedXContent; +import org.elasticsearch.common.xcontent.XContentHelper; + +import java.io.IOException; +import java.util.Map; + +/** + * Contains a list of index mapping upgraders + */ +public class MapperServiceUpgraders { + @SuppressWarnings("unchecked") + // package private for testing + static Map getMappingProperties(Map mappingMap, String type) { + // Check for MappingMap without type and ignore it + if (mappingMap.size() != 1) { + return null; + } + // Fetch the mappings for the encoded type + Map mappingType = (Map) mappingMap.get(type); + if (mappingType == null) { + return null; + } + + return (Map) mappingType.get("properties"); + } + + /** + * Upgrades the format field of mapping properties + *

+ * This method checks is the new mapping source has a different format than the original mapping, and if it + * does, then it returns the mapping source with updated format field. + */ + @SuppressWarnings({ "unchecked", "rawtypes" }) + public static CompressedXContent upgradeFormatsIfNeeded(String type, MappingMetadata source, CompressedXContent newMappingSource) { + Map sourceMap = source.rawSourceAsMap(); + Map newMap = XContentHelper.convertToMap(newMappingSource.compressedReference(), true).v2(); + + Map properties = getMappingProperties(sourceMap, type); + Map parsedProperties = getMappingProperties(newMap, type); + if (properties != null && parsedProperties != null) { + for (var property : properties.entrySet()) { + if (property.getValue()instanceof Map map) { + Object format = map.get("format"); + if (format != null) { + var newProperty = parsedProperties.get(property.getKey()); + if (newProperty instanceof Map parsedMap) { + Object parsedFormat = parsedMap.get("format"); + if (parsedFormat != null && parsedFormat.equals(format) == false) { + map.put("format", parsedFormat); + } + } + } + } + } + } + + try { + return new CompressedXContent(sourceMap); + } catch (IOException e) { + throw new IllegalStateException("Unexpected error remapping source map", e); + } + } +} From a429c1d969e42dca477ee174dec9ed43dbf0f765 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Tue, 26 Jul 2022 16:48:31 -0400 Subject: [PATCH 7/8] Add tests and refactor --- .../metadata/IndexMetadataVerifier.java | 1 - .../index/mapper/MapperService.java | 6 +- .../index/mapper/MapperServiceUpgraders.java | 19 +++-- .../index/mapper/MapperServiceTests.java | 82 +++++++++++++++++++ 4 files changed, 95 insertions(+), 13 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadataVerifier.java b/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadataVerifier.java index 921fc22311c48..3b1f12a732d0f 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadataVerifier.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadataVerifier.java @@ -197,7 +197,6 @@ public Set> entrySet() { indexMetadata = mapperService.mergeAndUpgrade(indexMetadata, MapperService.MergeReason.MAPPING_RECOVERY); } } catch (Exception ex) { - logger.error("Failed to parse mappings for index [" + indexMetadata.getIndex() + "]", ex); // Wrap the inner exception so we have the index name in the exception message throw new IllegalStateException("Failed to parse mappings for index [" + indexMetadata.getIndex() + "]", ex); } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index 919cf82032dea..44d2693fe37a6 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -344,11 +344,7 @@ public IndexMetadata mergeAndUpgrade(IndexMetadata indexMetadata, MergeReason re if (mappingMetadata != null) { var docMapper = merge(mappingMetadata.type(), mappingMetadata.source(), reason); if (indexVersionCreated.major <= Version.CURRENT.previousMajor().major) { - var upgradedSource = MapperServiceUpgraders.upgradeFormatsIfNeeded( - mappingMetadata.type(), - mappingMetadata, - docMapper.mappingSource() - ); + var upgradedSource = MapperServiceUpgraders.upgradeFormatsIfNeeded(mappingMetadata, docMapper.mappingSource()); if (mappingMetadata.source().equals(upgradedSource) == false) { return IndexMetadata.builder(indexMetadata).putMapping(new MappingMetadata(upgradedSource)).build(); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperServiceUpgraders.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperServiceUpgraders.java index 7052128bb7c5d..5996c832d41bb 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperServiceUpgraders.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperServiceUpgraders.java @@ -16,7 +16,7 @@ import java.util.Map; /** - * Contains a list of index mapping upgraders + * Contains a collection of helper methods for index mapping upgrades */ public class MapperServiceUpgraders { @SuppressWarnings("unchecked") @@ -39,15 +39,20 @@ static Map getMappingProperties(Map mappingMap, * Upgrades the format field of mapping properties *

* This method checks is the new mapping source has a different format than the original mapping, and if it - * does, then it returns the mapping source with updated format field. + * does, then it returns the original mapping source with updated format field. We cannot simply replace the + * MappingMetadata mappingSource with the new parsedSource, because the parsed source contains additional + * properties which cannot be serialized and reread. + * + * @param mappingMetadata the current mapping metadata + * @param parsedSource the mapping source as it was parsed */ @SuppressWarnings({ "unchecked", "rawtypes" }) - public static CompressedXContent upgradeFormatsIfNeeded(String type, MappingMetadata source, CompressedXContent newMappingSource) { - Map sourceMap = source.rawSourceAsMap(); - Map newMap = XContentHelper.convertToMap(newMappingSource.compressedReference(), true).v2(); + public static CompressedXContent upgradeFormatsIfNeeded(MappingMetadata mappingMetadata, CompressedXContent parsedSource) { + Map sourceMap = mappingMetadata.rawSourceAsMap(); + Map newMap = XContentHelper.convertToMap(parsedSource.compressedReference(), true).v2(); - Map properties = getMappingProperties(sourceMap, type); - Map parsedProperties = getMappingProperties(newMap, type); + Map properties = getMappingProperties(sourceMap, mappingMetadata.type()); + Map parsedProperties = getMappingProperties(newMap, mappingMetadata.type()); if (properties != null && parsedProperties != null) { for (var property : properties.entrySet()) { if (property.getValue()instanceof Map map) { diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java index f3771510d8da9..c03a2df48692b 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java @@ -353,4 +353,86 @@ public void testMultiFieldChecks() throws IOException { assertFalse(mapperService.isMultiField("object.subfield1")); } + public void testLegacyFormatParsing() throws IOException { + var oldIndexSettings = Settings.builder().put("index.version.created", Version.CURRENT.minimumIndexCompatibilityVersion()).build(); + MapperService mapperServiceOldIndex = createMapperService( + Version.CURRENT.minimumIndexCompatibilityVersion(), + oldIndexSettings, + () -> true, + mapping(b -> {}) + ); + + merge(mapperServiceOldIndex, """ + { "_doc" : { + "properties" : { + "field1" : { + "type" : "date", + "format": "strictDateOptionalTime" + } + } + } + } + """); + + var content = mapperServiceOldIndex.documentMapper().mappingSource(); + assertThat(content.string(), containsString("\"format\":\"strict_date_optional_time\"")); + } + + private IndexMetadata indexMetadataWithMapping(String source) { + IndexMetadata.Builder builder = new IndexMetadata.Builder("test"); + Settings settings = Settings.builder() + .put("index.number_of_replicas", 0) + .put("index.number_of_shards", 1) + .put("index.version.created", Version.CURRENT.minimumIndexCompatibilityVersion()) + .build(); + builder.settings(settings); + builder.putMapping(source); + + return builder.build(); + } + + public void testFormatUpgraders() throws IOException { + var oldIndexSettings = Settings.builder().put("index.version.created", Version.CURRENT.minimumIndexCompatibilityVersion()).build(); + MapperService mapperServiceOldIndex = createMapperService( + Version.CURRENT.minimumIndexCompatibilityVersion(), + oldIndexSettings, + () -> true, + mapping(b -> {}) + ); + + IndexMetadata indexMetadata = indexMetadataWithMapping(""" + {"_doc" : + {"properties":{"field":{"type":"date","format":"strictDateOptionalTime","store":"true"}}} + }"""); + + assertThat(indexMetadata.mapping().source().string(), containsString("\"format\":\"strictDateOptionalTime\"")); + IndexMetadata remapped = mapperServiceOldIndex.mergeAndUpgrade(indexMetadata, MergeReason.MAPPING_RECOVERY); + assertFalse(indexMetadata.mapping().equals(remapped.mapping())); + assertThat(remapped.mapping().source().string(), containsString("\"format\":\"strict_date_optional_time\"")); + // ensure we only changed that format field, nothing else + assertEquals( + indexMetadata.mapping().source().string(), + remapped.mapping() + .source() + .string() + .replace("\"format\":\"strict_date_optional_time\"", "\"format\":\"strictDateOptionalTime\"") + ); + + // We don't remap if the document types don't match + IndexMetadata indexMetadataNoType = indexMetadataWithMapping(""" + {"properties":{"field1":{"type":"date","format":"strictDateOptionalTime","store":"true"}}}"""); + + remapped = mapperServiceOldIndex.mergeAndUpgrade(indexMetadataNoType, MergeReason.MAPPING_RECOVERY); + assertTrue(remapped == indexMetadataNoType); + + // We don't remap if there's nothing to remap + IndexMetadata indexMetadataNoFormat = indexMetadataWithMapping(""" + {"_doc" : + {"properties":{"field2":{"type":"keyword","store":"true"}}} + }"""); + + remapped = mapperServiceOldIndex.mergeAndUpgrade(indexMetadataNoFormat, MergeReason.MAPPING_RECOVERY); + assertTrue(remapped == indexMetadataNoFormat); + } + } From c13de71ecc5212bbd739dd37069f0c91ea1f5468 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Wed, 27 Jul 2022 16:05:57 -0400 Subject: [PATCH 8/8] Address feedback --- .../metadata/IndexMetadataVerifier.java | 6 +- .../index/mapper/DateFieldMapper.java | 2 +- .../index/mapper/DateScriptFieldType.java | 3 + .../index/mapper/MapperService.java | 7 +- .../index/mapper/MapperServiceUpgraders.java | 76 ++++++++++++------- .../index/mapper/MapperServiceTests.java | 43 ++++++++++- 6 files changed, 103 insertions(+), 34 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadataVerifier.java b/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadataVerifier.java index 3b1f12a732d0f..19ca7dbd97e73 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadataVerifier.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadataVerifier.java @@ -90,7 +90,7 @@ public IndexMetadata verifyIndexMetadata(IndexMetadata indexMetadata, Version mi // Next we have to run this otherwise if we try to create IndexSettings // with broken settings it would fail in checkMappingsCompatibility newMetadata = archiveBrokenIndexSettings(newMetadata); - newMetadata = checkMappingsCompatibility(newMetadata); + newMetadata = checkAndUpdateMappingsCompatibility(newMetadata); return newMetadata; } @@ -118,7 +118,7 @@ private static void checkSupportedVersion(IndexMetadata indexMetadata, Version m } /** - * Check that we can parse the mappings. + * Check that we can parse the mappings and update mapping metadata if needed. * * This is not strictly necessary, since we parse the mappings later when loading the index and will * catch issues then. But it lets us fail very quickly and clearly: if there is a mapping incompatibility, @@ -128,7 +128,7 @@ private static void checkSupportedVersion(IndexMetadata indexMetadata, Version m * policy guarantees we can read mappings from previous compatible index versions. A failure here would * indicate a compatibility bug (which are unfortunately not that uncommon). */ - private IndexMetadata checkMappingsCompatibility(IndexMetadata indexMetadata) { + private IndexMetadata checkAndUpdateMappingsCompatibility(IndexMetadata indexMetadata) { try { // We cannot instantiate real analysis server or similarity service at this point because the node diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java index 21686cdf9a145..a89b6bde0a080 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java @@ -272,7 +272,7 @@ public Builder( ? DEFAULT_DATE_TIME_FORMATTER : DEFAULT_DATE_TIME_NANOS_FORMATTER; - if (indexCreatedVersion.major <= Version.CURRENT.previousMajor().major) { + if (indexCreatedVersion.major <= Version.V_8_0_0.previousMajor().major) { this.format = Parameter.stringParam( "format", indexCreatedVersion.isLegacyIndexVersion(), diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DateScriptFieldType.java b/server/src/main/java/org/elasticsearch/index/mapper/DateScriptFieldType.java index 86a0d7d2c877f..33313c431bd26 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DateScriptFieldType.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DateScriptFieldType.java @@ -12,8 +12,10 @@ import org.elasticsearch.common.lucene.search.Queries; import org.elasticsearch.common.time.DateFormatter; import org.elasticsearch.common.time.DateMathParser; +import org.elasticsearch.common.time.LegacyFormatNames; import org.elasticsearch.common.util.LocaleUtils; import org.elasticsearch.common.util.set.Sets; +import org.elasticsearch.common.xcontent.support.XContentMapValues; import org.elasticsearch.core.Nullable; import org.elasticsearch.core.TimeValue; import org.elasticsearch.index.fielddata.DateScriptFieldData; @@ -51,6 +53,7 @@ private static class Builder extends AbstractScriptFieldType.Builder format = FieldMapper.Parameter.stringParam( "format", true, + (n, c, o) -> LegacyFormatNames.camelCaseToSnakeCase(XContentMapValues.nodeStringValue(o)), RuntimeField.initializerNotSupported(), null, (b, n, v) -> { diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index 44d2693fe37a6..a67bc724fe263 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -343,10 +343,11 @@ public IndexMetadata mergeAndUpgrade(IndexMetadata indexMetadata, MergeReason re MappingMetadata mappingMetadata = indexMetadata.mapping(); if (mappingMetadata != null) { var docMapper = merge(mappingMetadata.type(), mappingMetadata.source(), reason); - if (indexVersionCreated.major <= Version.CURRENT.previousMajor().major) { - var upgradedSource = MapperServiceUpgraders.upgradeFormatsIfNeeded(mappingMetadata, docMapper.mappingSource()); + if (indexVersionCreated.major <= Version.V_8_0_0.previousMajor().major) { + var upgradedSource = MapperServiceUpgraders.upgradeDateFormatsIfNeeded(mappingMetadata, docMapper.mappingSource()); - if (mappingMetadata.source().equals(upgradedSource) == false) { + // reference equality for performance purposes + if (mappingMetadata.source() != upgradedSource) { return IndexMetadata.builder(indexMetadata).putMapping(new MappingMetadata(upgradedSource)).build(); } } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperServiceUpgraders.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperServiceUpgraders.java index 5996c832d41bb..bebc79fdf163c 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperServiceUpgraders.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperServiceUpgraders.java @@ -20,25 +20,47 @@ */ public class MapperServiceUpgraders { @SuppressWarnings("unchecked") - // package private for testing - static Map getMappingProperties(Map mappingMap, String type) { + private static Map getMappingsForType(Map mappingMap, String type) { // Check for MappingMap without type and ignore it if (mappingMap.size() != 1) { return null; } // Fetch the mappings for the encoded type - Map mappingType = (Map) mappingMap.get(type); - if (mappingType == null) { - return null; + return (Map) mappingMap.get(type); + } + + @SuppressWarnings({ "unchecked", "rawtypes" }) + private static boolean upgradeDateFormatProperties(Map properties, Map parsedProperties) { + boolean anyChanges = false; + + if (properties != null && parsedProperties != null) { + for (var property : properties.entrySet()) { + if (property.getValue()instanceof Map map) { + var parsedProperty = parsedProperties.get(property.getKey()); + if (parsedProperty instanceof Map parsedMap) { + Object format = map.get("format"); + if (format != null) { + Object parsedFormat = parsedMap.get("format"); + if (parsedFormat != null && parsedFormat.equals(format) == false) { + map.put("format", parsedFormat); + anyChanges = true; + } + } else { + var anyNestedChanges = upgradeDateFormatProperties(map, parsedMap); + anyChanges = anyChanges || anyNestedChanges; + } + } + } + } } - return (Map) mappingType.get("properties"); + return anyChanges; } /** - * Upgrades the format field of mapping properties + * Upgrades the date format field of mapping properties *

- * This method checks is the new mapping source has a different format than the original mapping, and if it + * This method checks if the new mapping source has a different format than the original mapping, and if it * does, then it returns the original mapping source with updated format field. We cannot simply replace the * MappingMetadata mappingSource with the new parsedSource, because the parsed source contains additional * properties which cannot be serialized and reread. @@ -47,27 +69,29 @@ static Map getMappingProperties(Map mappingMap, * @param parsedSource the mapping source as it was parsed */ @SuppressWarnings({ "unchecked", "rawtypes" }) - public static CompressedXContent upgradeFormatsIfNeeded(MappingMetadata mappingMetadata, CompressedXContent parsedSource) { + public static CompressedXContent upgradeDateFormatsIfNeeded(MappingMetadata mappingMetadata, CompressedXContent parsedSource) { Map sourceMap = mappingMetadata.rawSourceAsMap(); Map newMap = XContentHelper.convertToMap(parsedSource.compressedReference(), true).v2(); - Map properties = getMappingProperties(sourceMap, mappingMetadata.type()); - Map parsedProperties = getMappingProperties(newMap, mappingMetadata.type()); - if (properties != null && parsedProperties != null) { - for (var property : properties.entrySet()) { - if (property.getValue()instanceof Map map) { - Object format = map.get("format"); - if (format != null) { - var newProperty = parsedProperties.get(property.getKey()); - if (newProperty instanceof Map parsedMap) { - Object parsedFormat = parsedMap.get("format"); - if (parsedFormat != null && parsedFormat.equals(format) == false) { - map.put("format", parsedFormat); - } - } - } - } - } + Map mappings = getMappingsForType(sourceMap, mappingMetadata.type()); + Map parsedMappings = getMappingsForType(newMap, mappingMetadata.type()); + + if (mappings == null || parsedMappings == null) { + return mappingMetadata.source(); + } + + boolean anyPropertiesChanges = upgradeDateFormatProperties( + (Map) mappings.get("properties"), + (Map) parsedMappings.get("properties") + ); + + boolean anyRuntimeChanges = upgradeDateFormatProperties( + (Map) mappings.get("runtime"), + (Map) parsedMappings.get("runtime") + ); + + if (anyRuntimeChanges == false && anyPropertiesChanges == false) { + return mappingMetadata.source(); } try { diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java index c03a2df48692b..b6a73ee5c5374 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java @@ -402,7 +402,16 @@ public void testFormatUpgraders() throws IOException { IndexMetadata indexMetadata = indexMetadataWithMapping(""" {"_doc" : - {"properties":{"field":{"type":"date","format":"strictDateOptionalTime","store":"true"}}} + { + "properties":{ + "field":{"type":"date","format":"strictDateOptionalTime","store":"true"}, + "other_field":{"type":"keyword"} + }, + "runtime" : { + "object1.subfield1" : { "type" : "keyword" }, + "object2.subfield2" : { "type" : "keyword" } + } + } }"""); assertThat(indexMetadata.mapping().source().string(), containsString("\"format\":\"strictDateOptionalTime\"")); @@ -433,6 +442,38 @@ public void testFormatUpgraders() throws IOException { remapped = mapperServiceOldIndex.mergeAndUpgrade(indexMetadataNoFormat, MergeReason.MAPPING_RECOVERY); assertTrue(remapped == indexMetadataNoFormat); + + var nestedMappingSource = new CompressedXContent(BytesReference.bytes(mapping(b -> { + b.startObject("nested"); + { + b.field("type", "nested"); + b.startObject("properties"); + { + b.startObject("field3"); + { + b.field("type", "date"); + b.field("format", "strictDateOptionalTime"); + } + b.endObject(); + } + b.endObject(); + } + b.endObject(); + }))).string(); + + IndexMetadata indexMetadataNestedMapping = indexMetadataWithMapping(nestedMappingSource); + remapped = mapperServiceOldIndex.mergeAndUpgrade(indexMetadataNestedMapping, MergeReason.MAPPING_RECOVERY); + + assertTrue(remapped != indexMetadataNoFormat); + assertThat(remapped.mapping().source().string(), containsString("\"format\":\"strict_date_optional_time\"")); + + IndexMetadata indexMetadataRuntime = indexMetadataWithMapping(""" + {"_doc":{"dynamic":"runtime","runtime":{"field2":{"type":"date","format":"strictDateOptionalTime"}}}}"""); + + remapped = mapperServiceOldIndex.mergeAndUpgrade(indexMetadataRuntime, MergeReason.MAPPING_RECOVERY); + + assertTrue(remapped != indexMetadataRuntime); + assertThat(remapped.mapping().source().string(), containsString("\"format\":\"strict_date_optional_time\"")); } }