Fix synthetic recovery source version validation. (#118924)

martijnvg · web-flow · commit 4974336221f3 · 2024-12-18T11:25:27.000Z
The validation didn't work as expected, because created version was always 0 (which is created version default value).

Moving the validation to a place that does have access to the index version.
diff --git a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.create/20_synthetic_source.yml b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.create/20_synthetic_source.yml
@@ -2012,3 +2012,42 @@ synthetic_source with copy_to pointing inside dynamic object:
       hits.hits.2.fields:
         c.copy.keyword: [ "hello", "zap" ]
 
+---
+create index with use_synthetic_source:
+  - requires:
+      cluster_features: ["mapper.synthetic_recovery_source"]
+      reason: requires synthetic recovery source
+
+  - do:
+      indices.create:
+        index: test
+        body:
+          settings:
+            index:
+              recovery:
+                use_synthetic_source: true
+              mapping:
+                source:
+                  mode: synthetic
+
+  - do:
+      indices.get_settings: {}
+  - match: { test.settings.index.mapping.source.mode: synthetic}
+  - is_true: test.settings.index.recovery.use_synthetic_source
+
+  - do:
+      bulk:
+        index: test
+        refresh: true
+        body:
+          - '{ "create": { } }'
+          - '{ "field": "aaaa" }'
+          - '{ "create": { } }'
+          - '{ "field": "bbbb" }'
+
+  - do:
+      indices.disk_usage:
+        index: test
+        run_expensive_tasks: true
+  - gt: { test.fields.field.total_in_bytes: 0 }
+  - is_false: test.fields.field._recovery_source
diff --git a/server/src/main/java/org/elasticsearch/index/IndexSettings.java b/server/src/main/java/org/elasticsearch/index/IndexSettings.java
@@ -685,29 +685,11 @@ public void validate(Boolean enabled, Map<Setting<?>, Object> settings) {
                         );
                     }
                 }
-
-                // Verify that all nodes can handle this setting
-                var version = (IndexVersion) settings.get(SETTING_INDEX_VERSION_CREATED);
-                if (version.before(IndexVersions.USE_SYNTHETIC_SOURCE_FOR_RECOVERY)
-                    && version.between(
-                        IndexVersions.USE_SYNTHETIC_SOURCE_FOR_RECOVERY_BACKPORT,
-                        IndexVersions.UPGRADE_TO_LUCENE_10_0_0
-                    ) == false) {
-                    throw new IllegalArgumentException(
-                        String.format(
-                            Locale.ROOT,
-                            "The setting [%s] is unavailable on this cluster because some nodes are running older "
-                                + "versions that do not support it. Please upgrade all nodes to the latest version "
-                                + "and try again.",
-                            RECOVERY_USE_SYNTHETIC_SOURCE_SETTING.getKey()
-                        )
-                    );
-                }
             }
 
             @Override
             public Iterator<Setting<?>> settings() {
-                List<Setting<?>> res = List.of(INDEX_MAPPER_SOURCE_MODE_SETTING, SETTING_INDEX_VERSION_CREATED, MODE);
+                List<Setting<?>> res = List.of(INDEX_MAPPER_SOURCE_MODE_SETTING, MODE);
                 return res.iterator();
             }
         },
@@ -1050,6 +1032,24 @@ public IndexSettings(final IndexMetadata indexMetadata, final Settings nodeSetti
         indexMappingSourceMode = scopedSettings.get(INDEX_MAPPER_SOURCE_MODE_SETTING);
         recoverySourceEnabled = RecoverySettings.INDICES_RECOVERY_SOURCE_ENABLED_SETTING.get(nodeSettings);
         recoverySourceSyntheticEnabled = scopedSettings.get(RECOVERY_USE_SYNTHETIC_SOURCE_SETTING);
+        if (recoverySourceSyntheticEnabled) {
+            // Verify that all nodes can handle this setting
+            if (version.before(IndexVersions.USE_SYNTHETIC_SOURCE_FOR_RECOVERY)
+                && version.between(
+                    IndexVersions.USE_SYNTHETIC_SOURCE_FOR_RECOVERY_BACKPORT,
+                    IndexVersions.UPGRADE_TO_LUCENE_10_0_0
+                ) == false) {
+                throw new IllegalArgumentException(
+                    String.format(
+                        Locale.ROOT,
+                        "The setting [%s] is unavailable on this cluster because some nodes are running older "
+                            + "versions that do not support it. Please upgrade all nodes to the latest version "
+                            + "and try again.",
+                        RECOVERY_USE_SYNTHETIC_SOURCE_SETTING.getKey()
+                    )
+                );
+            }
+        }
 
         scopedSettings.addSettingsUpdateConsumer(
             MergePolicyConfig.INDEX_COMPOUND_FORMAT_SETTING,
diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperFeatures.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperFeatures.java
@@ -77,7 +77,8 @@ public Set<NodeFeature> getTestFeatures() {
             DocumentParser.FIX_PARSING_SUBOBJECTS_FALSE_DYNAMIC_FALSE,
             CONSTANT_KEYWORD_SYNTHETIC_SOURCE_WRITE_FIX,
             META_FETCH_FIELDS_ERROR_CODE_CHANGED,
-            SPARSE_VECTOR_STORE_SUPPORT
+            SPARSE_VECTOR_STORE_SUPPORT,
+            SourceFieldMapper.SYNTHETIC_RECOVERY_SOURCE
         );
     }
 }
diff --git a/server/src/main/java/org/elasticsearch/index/mapper/SourceFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/SourceFieldMapper.java
@@ -56,6 +56,7 @@ public class SourceFieldMapper extends MetadataFieldMapper {
         "mapper.source.remove_synthetic_source_only_validation"
     );
     public static final NodeFeature SOURCE_MODE_FROM_INDEX_SETTING = new NodeFeature("mapper.source.mode_from_index_setting");
+    public static final NodeFeature SYNTHETIC_RECOVERY_SOURCE = new NodeFeature("mapper.synthetic_recovery_source");
 
     public static final String NAME = "_source";
     public static final String RECOVERY_SOURCE_NAME = "_recovery_source";
diff --git a/server/src/test/java/org/elasticsearch/index/mapper/SourceFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/SourceFieldMapperTests.java
@@ -465,60 +465,6 @@ public void testRecoverySourceWitInvalidSettings() {
                 )
             );
         }
-        {
-            Settings settings = Settings.builder()
-                .put(SourceFieldMapper.INDEX_MAPPER_SOURCE_MODE_SETTING.getKey(), SourceFieldMapper.Mode.SYNTHETIC.toString())
-                .put(IndexSettings.RECOVERY_USE_SYNTHETIC_SOURCE_SETTING.getKey(), true)
-                .build();
-            IllegalArgumentException exc = expectThrows(
-                IllegalArgumentException.class,
-                () -> createMapperService(
-                    IndexVersionUtils.randomPreviousCompatibleVersion(random(), IndexVersions.USE_SYNTHETIC_SOURCE_FOR_RECOVERY_BACKPORT),
-                    settings,
-                    () -> false,
-                    topMapping(b -> {})
-                )
-            );
-            assertThat(
-                exc.getMessage(),
-                containsString(
-                    String.format(
-                        Locale.ROOT,
-                        "The setting [%s] is unavailable on this cluster",
-                        IndexSettings.RECOVERY_USE_SYNTHETIC_SOURCE_SETTING.getKey()
-                    )
-                )
-            );
-        }
-        {
-            Settings settings = Settings.builder()
-                .put(SourceFieldMapper.INDEX_MAPPER_SOURCE_MODE_SETTING.getKey(), SourceFieldMapper.Mode.SYNTHETIC.toString())
-                .put(IndexSettings.RECOVERY_USE_SYNTHETIC_SOURCE_SETTING.getKey(), true)
-                .build();
-            IllegalArgumentException exc = expectThrows(
-                IllegalArgumentException.class,
-                () -> createMapperService(
-                    IndexVersionUtils.randomVersionBetween(
-                        random(),
-                        IndexVersions.UPGRADE_TO_LUCENE_10_0_0,
-                        IndexVersions.DEPRECATE_SOURCE_MODE_MAPPER
-                    ),
-                    settings,
-                    () -> false,
-                    topMapping(b -> {})
-                )
-            );
-            assertThat(
-                exc.getMessage(),
-                containsString(
-                    String.format(
-                        Locale.ROOT,
-                        "The setting [%s] is unavailable on this cluster",
-                        IndexSettings.RECOVERY_USE_SYNTHETIC_SOURCE_SETTING.getKey()
-                    )
-                )
-            );
-        }
     }
 
     public void testRecoverySourceWithSyntheticSource() throws IOException {

Original file line number	Diff line number	Diff line change
`@@ -77,7 +77,8 @@ public Set<NodeFeature> getTestFeatures() {`
`77`	`77`	`DocumentParser.FIX_PARSING_SUBOBJECTS_FALSE_DYNAMIC_FALSE,`
`78`	`78`	`CONSTANT_KEYWORD_SYNTHETIC_SOURCE_WRITE_FIX,`
`79`	`79`	`META_FETCH_FIELDS_ERROR_CODE_CHANGED,`
`80`		`- SPARSE_VECTOR_STORE_SUPPORT`
	`80`	`+ SPARSE_VECTOR_STORE_SUPPORT,`
	`81`	`+ SourceFieldMapper.SYNTHETIC_RECOVERY_SOURCE`
`81`	`82`	`);`
`82`	`83`	`}`
`83`	`84`	`}`