Browse Source

Add a check for the same feature being declared regular and historical (#106285)

Simon Cooper 1 year ago
parent
commit
2a87081673

+ 5 - 0
docs/changelog/106285.yaml

@@ -0,0 +1,5 @@
+pr: 106285
+summary: Add a check for the same feature being declared regular and historical
+area: Infra/Core
+type: bug
+issues: []

+ 13 - 1
server/src/main/java/org/elasticsearch/features/FeatureData.java

@@ -41,6 +41,8 @@ public class FeatureData {
         NavigableMap<Version, Set<String>> historicalFeatures = new TreeMap<>();
         Map<String, NodeFeature> nodeFeatures = new HashMap<>();
         for (FeatureSpecification spec : specs) {
+            var specFeatures = spec.getFeatures();
+
             for (var hfe : spec.getHistoricalFeatures().entrySet()) {
                 FeatureSpecification existing = allFeatures.putIfAbsent(hfe.getKey().id(), spec);
                 // the same SPI class can be loaded multiple times if it's in the base classloader
@@ -61,10 +63,20 @@ public class FeatureData {
                     );
                 }
 
+                if (specFeatures.contains(hfe.getKey())) {
+                    throw new IllegalArgumentException(
+                        Strings.format(
+                            "Feature [%s] cannot be declared as both a regular and historical feature by [%s]",
+                            hfe.getKey().id(),
+                            spec
+                        )
+                    );
+                }
+
                 historicalFeatures.computeIfAbsent(hfe.getValue(), k -> new HashSet<>()).add(hfe.getKey().id());
             }
 
-            for (NodeFeature f : spec.getFeatures()) {
+            for (NodeFeature f : specFeatures) {
                 FeatureSpecification existing = allFeatures.putIfAbsent(f.id(), spec);
                 if (existing != null && existing.getClass() != spec.getClass()) {
                     throw new IllegalArgumentException(

+ 12 - 0
server/src/test/java/org/elasticsearch/features/FeatureServiceTests.java

@@ -84,6 +84,18 @@ public class FeatureServiceTests extends ESTestCase {
         );
     }
 
+    public void testFailsSameRegularAndHistoricalFeature() {
+        FeatureSpecification fs = new TestFeatureSpecification(
+            Set.of(new NodeFeature("f1")),
+            Map.of(new NodeFeature("f1"), Version.V_8_12_0)
+        );
+
+        assertThat(
+            expectThrows(IllegalArgumentException.class, () -> new FeatureService(List.of(fs))).getMessage(),
+            containsString("cannot be declared as both a regular and historical feature")
+        );
+    }
+
     public void testGetNodeFeaturesCombinesAllSpecs() {
         List<FeatureSpecification> specs = List.of(
             new TestFeatureSpecification(Set.of(new NodeFeature("f1"), new NodeFeature("f2")), Map.of()),