Mapping update for “date_range” field type is not idempotent

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
opensearch-project · Feb 14, 2022 · 6143aaa · 6143aaa
1 parent 27ed6fc
commit 6143aaa
Show file tree

Hide file tree

Showing 6 changed files with 54 additions and 12 deletions.
diff --git a/modules/percolator/src/main/java/org/opensearch/percolator/PercolatorFieldMapper.java b/modules/percolator/src/main/java/org/opensearch/percolator/PercolatorFieldMapper.java
@@ -195,7 +195,12 @@ static BinaryFieldMapper createQueryBuilderFieldBuilder(BuilderContext context)
         }
 
         static RangeFieldMapper createExtractedRangeFieldBuilder(String name, RangeType rangeType, BuilderContext context) {
-            RangeFieldMapper.Builder builder = new RangeFieldMapper.Builder(name, rangeType, true);
+            RangeFieldMapper.Builder builder = new RangeFieldMapper.Builder(
+                name,
+                rangeType,
+                true,
+                hasIndexCreated(context.indexSettings()) ? context.indexCreatedVersion() : null
+            );
             // For now no doc values, because in processQuery(...) only the Lucene range fields get added:
             builder.docValues(false);
             return builder.build(context);

diff --git a/server/src/main/java/org/opensearch/index/mapper/Mapper.java b/server/src/main/java/org/opensearch/index/mapper/Mapper.java
@@ -33,6 +33,8 @@
 package org.opensearch.index.mapper;
 
 import org.opensearch.Version;
+import org.opensearch.cluster.metadata.IndexMetadata;
+import org.opensearch.common.Nullable;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.common.time.DateFormatter;
 import org.opensearch.common.xcontent.ToXContentFragment;
@@ -69,6 +71,14 @@ public Settings indexSettings() {
         public Version indexCreatedVersion() {
             return Version.indexCreated(indexSettings);
         }
+
+        public Version indexCreatedVersionOrDefault(@Nullable Version defaultValue) {
+            if (defaultValue == null || hasIndexCreated(indexSettings)) {
+                return indexCreatedVersion();
+            } else {
+                return defaultValue;
+            }
+        }
     }
 
     public abstract static class Builder<T extends Builder> {
@@ -240,4 +250,12 @@ public final String simpleName() {
      */
     public abstract void validate(MappingLookup mappers);
 
+    /**
+     * Check if settings have IndexMetadata.SETTING_INDEX_VERSION_CREATED setting.
+     * @param settings settings
+     * @return "true" if settings have IndexMetadata.SETTING_INDEX_VERSION_CREATED setting, "false" otherwise
+     */
+    protected static boolean hasIndexCreated(Settings settings) {
+        return settings.hasValue(IndexMetadata.SETTING_INDEX_VERSION_CREATED.getKey());
+    }
 }
diff --git a/server/src/main/java/org/opensearch/index/mapper/RangeFieldMapper.java b/server/src/main/java/org/opensearch/index/mapper/RangeFieldMapper.java
@@ -36,6 +36,7 @@
 import org.apache.lucene.search.Query;
 import org.apache.lucene.util.BytesRef;
 import org.opensearch.OpenSearchException;
+import org.opensearch.Version;
 import org.opensearch.common.Explicit;
 import org.opensearch.common.collect.Tuple;
 import org.opensearch.common.geo.ShapeRelation;
@@ -116,15 +117,17 @@ public static class Builder extends ParametrizedFieldMapper.Builder {
         private final Parameter<Map<String, String>> meta = Parameter.metaParam();
 
         private final RangeType type;
+        private final Version indexCreatedVersion;
 
         public Builder(String name, RangeType type, Settings settings) {
-            this(name, type, COERCE_SETTING.get(settings));
+            this(name, type, COERCE_SETTING.get(settings), hasIndexCreated(settings) ? Version.indexCreated(settings) : null);
         }
 
-        public Builder(String name, RangeType type, boolean coerceByDefault) {
+        public Builder(String name, RangeType type, boolean coerceByDefault, Version indexCreatedVersion) {
             super(name);
             this.type = type;
             this.coerce = Parameter.explicitBoolParam("coerce", true, m -> toType(m).coerce, coerceByDefault);
+            this.indexCreatedVersion = indexCreatedVersion;
             if (this.type != RangeType.DATE) {
                 format.neverSerialize();
                 locale.neverSerialize();
@@ -157,8 +160,11 @@ protected RangeFieldType setupFieldType(BuilderContext context) {
                             + " type"
                     );
                 }
+
+                // The builder context may not have index created version, falling back to indexCreatedVersion
+                // property of this mapper builder.
                 DateFormatter dateTimeFormatter;
-                if (Joda.isJodaPattern(context.indexCreatedVersion(), format.getValue())) {
+                if (Joda.isJodaPattern(context.indexCreatedVersionOrDefault(indexCreatedVersion), format.getValue())) {
                     dateTimeFormatter = Joda.forPattern(format.getValue()).withLocale(locale.getValue());
                 } else {
                     dateTimeFormatter = DateFormatter.forPattern(format.getValue()).withLocale(locale.getValue());
@@ -371,6 +377,7 @@ public Query rangeQuery(
     private final Locale locale;
 
     private final boolean coerceByDefault;
+    private final Version indexCreatedVersion;
 
     private RangeFieldMapper(
         String simpleName,
@@ -389,6 +396,7 @@ private RangeFieldMapper(
         this.format = builder.format.getValue();
         this.locale = builder.locale.getValue();
         this.coerceByDefault = builder.coerce.getDefaultValue().value();
+        this.indexCreatedVersion = builder.indexCreatedVersion;
     }
 
     boolean coerce() {
@@ -397,7 +405,7 @@ boolean coerce() {
 
     @Override
     public ParametrizedFieldMapper.Builder getMergeBuilder() {
-        return new Builder(simpleName(), type, coerceByDefault).init(this);
+        return new Builder(simpleName(), type, coerceByDefault, indexCreatedVersion).init(this);
     }
 
     @Override

diff --git a/server/src/test/java/org/opensearch/index/mapper/IpRangeFieldTypeTests.java b/server/src/test/java/org/opensearch/index/mapper/IpRangeFieldTypeTests.java
@@ -46,7 +46,7 @@ public void testFetchSourceValue() throws IOException {
         Settings settings = Settings.builder().put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT.id).build();
         Mapper.BuilderContext context = new Mapper.BuilderContext(settings, new ContentPath());
 
-        RangeFieldMapper mapper = new RangeFieldMapper.Builder("field", RangeType.IP, true).build(context);
+        RangeFieldMapper mapper = new RangeFieldMapper.Builder("field", RangeType.IP, true, Version.V_EMPTY).build(context);
         Map<String, Object> range = org.opensearch.common.collect.Map.of("gte", "2001:db8:0:0:0:0:2:1");
         assertEquals(
             Collections.singletonList(org.opensearch.common.collect.Map.of("gte", "2001:db8::2:1")),

diff --git a/server/src/test/java/org/opensearch/index/mapper/RangeFieldMapperTests.java b/server/src/test/java/org/opensearch/index/mapper/RangeFieldMapperTests.java
@@ -41,6 +41,7 @@
 import org.opensearch.common.xcontent.ToXContent;
 import org.opensearch.common.xcontent.XContentBuilder;
 import org.opensearch.common.xcontent.XContentFactory;
+import org.opensearch.index.mapper.MapperService.MergeReason;
 
 import java.io.IOException;
 import java.net.InetAddress;
@@ -374,4 +375,12 @@ public void testIllegalFormatField() throws Exception {
         assertThat(e.getMessage(), containsString("Invalid format: [[test_format]]: Unknown pattern letter: t"));
     }
 
+    public void testUpdatesWithSameMappings() throws Exception {
+        for (final String type : types()) {
+            final DocumentMapper mapper = createDocumentMapper(rangeFieldMapping(type, b -> { b.field("store", true); }));
+
+            final Mapping mapping = mapper.mapping();
+            mapper.merge(mapping, MergeReason.MAPPING_UPDATE);
+        }
+    }
 }
diff --git a/server/src/test/java/org/opensearch/index/mapper/RangeFieldTypeTests.java b/server/src/test/java/org/opensearch/index/mapper/RangeFieldTypeTests.java
@@ -536,16 +536,17 @@ public void testFetchSourceValue() throws IOException {
         Settings settings = Settings.builder().put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT.id).build();
         Mapper.BuilderContext context = new Mapper.BuilderContext(settings, new ContentPath());
 
-        MappedFieldType longMapper = new RangeFieldMapper.Builder("field", RangeType.LONG, true).build(context).fieldType();
+        MappedFieldType longMapper = new RangeFieldMapper.Builder("field", RangeType.LONG, true, Version.V_EMPTY).build(context)
+            .fieldType();
         Map<String, Object> longRange = org.opensearch.common.collect.Map.of("gte", 3.14, "lt", "42.9");
         assertEquals(
             Collections.singletonList(org.opensearch.common.collect.Map.of("gte", 3L, "lt", 42L)),
             fetchSourceValue(longMapper, longRange)
         );
 
-        MappedFieldType dateMapper = new RangeFieldMapper.Builder("field", RangeType.DATE, true).format("yyyy/MM/dd||epoch_millis")
-            .build(context)
-            .fieldType();
+        MappedFieldType dateMapper = new RangeFieldMapper.Builder("field", RangeType.DATE, true, Version.V_EMPTY).format(
+            "yyyy/MM/dd||epoch_millis"
+        ).build(context).fieldType();
         Map<String, Object> dateRange = org.opensearch.common.collect.Map.of("lt", "1990/12/29", "gte", 597429487111L);
         assertEquals(
             Collections.singletonList(org.opensearch.common.collect.Map.of("lt", "1990/12/29", "gte", "1988/12/06")),
@@ -557,14 +558,15 @@ public void testParseSourceValueWithFormat() throws IOException {
         Settings settings = Settings.builder().put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT.id).build();
         Mapper.BuilderContext context = new Mapper.BuilderContext(settings, new ContentPath());
 
-        MappedFieldType longMapper = new RangeFieldMapper.Builder("field", RangeType.LONG, true).build(context).fieldType();
+        MappedFieldType longMapper = new RangeFieldMapper.Builder("field", RangeType.LONG, true, Version.V_EMPTY).build(context)
+            .fieldType();
         Map<String, Object> longRange = org.opensearch.common.collect.Map.of("gte", 3.14, "lt", "42.9");
         assertEquals(
             Collections.singletonList(org.opensearch.common.collect.Map.of("gte", 3L, "lt", 42L)),
             fetchSourceValue(longMapper, longRange)
         );
 
-        MappedFieldType dateMapper = new RangeFieldMapper.Builder("field", RangeType.DATE, true).format("strict_date_time")
+        MappedFieldType dateMapper = new RangeFieldMapper.Builder("field", RangeType.DATE, true, Version.V_EMPTY).format("strict_date_time")
             .build(context)
             .fieldType();
         Map<String, Object> dateRange = org.opensearch.common.collect.Map.of("lt", "1990-12-29T00:00:00.000Z");