Improve checking of invalid config keys for hashgnn

Co-Authored-By: Florentin Dörre <florentin.dorre@neotechnology.com>

Author: breakanalysis

algo/src/main/java/org/neo4j/gds/embeddings/hashgnn/HashGNNConfig.java

@@ -77,7 +77,10 @@ static Optional<BinarizeFeaturesConfig> parseBinarizationConfig(Object o) {
         if (o instanceof Optional) {
             return (Optional<BinarizeFeaturesConfig>) o;
         }
-        return Optional.of(new BinarizeFeaturesConfigImpl(CypherMapWrapper.create((Map<String, Object>) o)));
+        var cypherMapWrapper = CypherMapWrapper.create((Map<String, Object>) o);
+        var binarizeFeaturesConfig = new BinarizeFeaturesConfigImpl(cypherMapWrapper);
+        cypherMapWrapper.requireOnlyKeysFrom(binarizeFeaturesConfig.configKeys());
+        return Optional.of(binarizeFeaturesConfig);
     }
 
     static Map<String, Object> toMapBinarizationConfig(BinarizeFeaturesConfig config) {
@@ -88,25 +91,14 @@ static Optional<GenerateFeaturesConfig> parseGenerateFeaturesConfig(Object o) {
         if (o instanceof Optional) {
             return (Optional<GenerateFeaturesConfig>) o;
         }
-        return Optional.of(new GenerateFeaturesConfigImpl(CypherMapWrapper.create((Map<String, Object>) o)));
+        var cypherMapWrapper = CypherMapWrapper.create((Map<String, Object>) o);
+        var generateFeaturesConfig = new GenerateFeaturesConfigImpl(cypherMapWrapper);
+        cypherMapWrapper.requireOnlyKeysFrom(generateFeaturesConfig.configKeys());
+        return Optional.of(generateFeaturesConfig);
     }
 
     static Map<String, Object> toMapGenerateFeaturesConfig(GenerateFeaturesConfig config) {
         return config.toMap();
     }
 
-    static void checkConfigKeys(CypherMapWrapper config) {
-        if (config.containsKey("binarizeFeatures")) {
-            var binarizeFeaturesMap = config.getMap("binarizeFeatures");
-            var cypherMapWrapper = CypherMapWrapper.create(binarizeFeaturesMap);
-            var binarizeFeaturesConfig = new BinarizeFeaturesConfigImpl(cypherMapWrapper);
-            cypherMapWrapper.requireOnlyKeysFrom(binarizeFeaturesConfig.configKeys());
-        }
-        if (config.containsKey("generateFeatures")) {
-            var generateFeaturesMap = config.getMap("generateFeatures");
-            var cypherMapWrapper = CypherMapWrapper.create(generateFeaturesMap);
-            var generateFeaturesConfig = new GenerateFeaturesConfigImpl(cypherMapWrapper);
-            cypherMapWrapper.requireOnlyKeysFrom(generateFeaturesConfig.configKeys());
-        }
-    }
 }

algo/src/main/java/org/neo4j/gds/embeddings/hashgnn/HashGNNMutateConfig.java

@@ -27,7 +27,6 @@
 public interface HashGNNMutateConfig extends HashGNNConfig, MutatePropertyConfig {
 
     static HashGNNMutateConfig of(CypherMapWrapper config) {
-        HashGNNConfig.checkConfigKeys(config);
         return new HashGNNMutateConfigImpl(config);
     }
 

algo/src/main/java/org/neo4j/gds/embeddings/hashgnn/HashGNNStreamConfig.java

@@ -26,7 +26,6 @@
 public interface HashGNNStreamConfig extends HashGNNConfig {
 
     static HashGNNStreamConfig of(CypherMapWrapper config) {
-        HashGNNConfig.checkConfigKeys(config);
         return new HashGNNStreamConfigImpl(config);
     }
 }

algo/src/test/java/org/neo4j/gds/embeddings/hashgnn/HashGNNConfigTest.java

@@ -20,6 +20,7 @@
 package org.neo4j.gds.embeddings.hashgnn;
 
 import org.junit.jupiter.api.Test;
+import org.neo4j.gds.core.CypherMapWrapper;
 
 import java.util.List;
 import java.util.Map;
@@ -75,4 +76,36 @@ void requiresDensityLevelAtMostDensity() {
                 .build();
         }).hasMessage("Generate features requires `densityLevel` to be at most `dimension` but was 5 > 4.");
     }
+
+    @Test
+    void failsOnInvalidBinarizationKeys() {
+        assertThatThrownBy(() -> {
+            new HashGNNConfigImpl(CypherMapWrapper.create(
+                Map.of(
+                    "mutateProperty", "foo",
+                    "featureProperties", List.of("x"),
+                    "binarizeFeatures", Map.of("dimension", 100, "treshold", 2.0),
+                    "embeddingDensity", 4,
+                    "iterations", 100
+                )
+            ));
+
+        }).isInstanceOf(IllegalArgumentException.class)
+            .hasMessage("Unexpected configuration key: treshold (Did you mean [threshold]?)");
+    }
+
+    @Test
+    void failsOnInvalidGenerateFeaturesKeys() {
+        assertThatThrownBy(() -> {
+            new HashGNNConfigImpl(CypherMapWrapper.create(
+                Map.of(
+                    "generateFeatures", Map.of("dimension", 100, "densityElfen", 2),
+                    "embeddingDensity", 4,
+                    "iterations", 100
+                )
+            ));
+
+        }).isInstanceOf(IllegalArgumentException.class)
+            .hasMessage("No value specified for the mandatory configuration parameter `densityLevel` (a similar parameter exists: [densityElfen])");
+    }
 }

proc/embeddings/src/test/java/org/neo4j/gds/embeddings/hashgnn/HashGNNMutateProcTest.java

@@ -19,17 +19,11 @@
  */
 package org.neo4j.gds.embeddings.hashgnn;
 
-import org.junit.jupiter.api.Test;
 import org.neo4j.gds.AlgoBaseProc;
 import org.neo4j.gds.MutateNodePropertyTest;
 import org.neo4j.gds.api.nodeproperties.ValueType;
 import org.neo4j.gds.core.CypherMapWrapper;
 
-import java.util.List;
-import java.util.Map;
-
-import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy;
-
 class HashGNNMutateProcTest extends HashGNNProcTest<HashGNNMutateConfig> implements
     MutateNodePropertyTest<HashGNN, HashGNNMutateConfig, HashGNN.HashGNNResult> {
 
@@ -72,36 +66,4 @@ public String expectedMutatedGraph() {
         ", (b)-[:R1]->(a)" +
         ", (b)-[:R2]->(c)";
     }
-
-    @Test
-    void failsOnInvalidBinarizationKeys() {
-        assertThatThrownBy(() -> {
-            HashGNNMutateConfig.of(CypherMapWrapper.create(
-                Map.of(
-                    "mutateProperty", "foo",
-                    "featureProperties", List.of("x"),
-                    "binarizeFeatures", Map.of("dimension", 100, "treshold", 2.0),
-                    "embeddingDensity", 4,
-                    "iterations", 100
-                )
-            ));
-
-        }).isInstanceOf(IllegalArgumentException.class)
-            .hasMessage("Unexpected configuration key: treshold (Did you mean [threshold]?)");
-    }
-
-    @Test
-    void failsOnInvalidGenerateFeaturesKeys() {
-        assertThatThrownBy(() -> {
-            HashGNNMutateConfig.of(CypherMapWrapper.create(
-                Map.of(
-                    "generateFeatures", Map.of("dimension", 100, "densityElfen", 2),
-                    "embeddingDensity", 4,
-                    "iterations", 100
-                )
-            ));
-
-        }).isInstanceOf(IllegalArgumentException.class)
-            .hasMessage("No value specified for the mandatory configuration parameter `densityLevel` (a similar parameter exists: [densityElfen])");
-    }
 }

proc/embeddings/src/test/java/org/neo4j/gds/embeddings/hashgnn/HashGNNStreamProcTest.java

@@ -25,10 +25,8 @@
 import org.neo4j.gds.core.CypherMapWrapper;
 
 import java.util.List;
-import java.util.Map;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy;
 
 @SuppressWarnings("unchecked")
 class HashGNNStreamProcTest extends HashGNNProcTest<HashGNNStreamConfig> {
@@ -70,37 +68,5 @@ void shouldComputeNonZeroEmbeddings() {
                 .hasSize(3)
                 .anyMatch(value -> value != 0.0);
         });
-
-    }
-
-    @Test
-    void failsOnInvalidBinarizationKeys() {
-        assertThatThrownBy(() -> {
-            HashGNNStreamConfig.of(CypherMapWrapper.create(
-                Map.of(
-                    "featureProperties", List.of("x"),
-                    "binarizeFeatures", Map.of("dimension", 100, "treshold", 2.0),
-                    "embeddingDensity", 4,
-                    "iterations", 100
-                )
-            ));
-
-        }).isInstanceOf(IllegalArgumentException.class)
-            .hasMessage("Unexpected configuration key: treshold (Did you mean [threshold]?)");
-    }
-
-    @Test
-    void failsOnInvalidGenerateFeaturesKeys() {
-        assertThatThrownBy(() -> {
-            HashGNNStreamConfig.of(CypherMapWrapper.create(
-                Map.of(
-                    "generateFeatures", Map.of("dimension", 100, "densityElfen", 2),
-                    "embeddingDensity", 4,
-                    "iterations", 100
-                )
-            ));
-
-        }).isInstanceOf(IllegalArgumentException.class)
-            .hasMessage("No value specified for the mandatory configuration parameter `densityLevel` (a similar parameter exists: [densityElfen])");
     }
 }