Validate config options for CypherAggregation

knutwalker · knutwalker · commit 97eb0a7b74fa · 2022-12-14T09:57:29.000+01:00
diff --git a/annotations/src/main/java/org/neo4j/gds/core/ConfigKeyValidation.java b/annotations/src/main/java/org/neo4j/gds/core/ConfigKeyValidation.java
@@ -32,9 +32,14 @@ public final class ConfigKeyValidation {
 
     private ConfigKeyValidation() {}
 
-    public static void requireOnlyKeysFrom(Collection<String> allowedKeys, Collection<String> configKeys) {
+    public static void requireOnlyKeysFrom(Collection<String> allowedKeys, Iterable<String> configKeys) {
         var unexpectedKeys = new TreeSet<>(String.CASE_INSENSITIVE_ORDER);
-        unexpectedKeys.addAll(configKeys);
+        if (configKeys instanceof Collection<?>) {
+            unexpectedKeys.addAll((Collection<String>) configKeys);
+        } else {
+            configKeys.forEach(unexpectedKeys::add);
+        }
+
 
         // We are doing the equivalent of `removeAll` here.
         // As of jdk16, TreeSet does not have a specialized removeAll implementation and reverts to use
@@ -54,15 +59,25 @@ public static void requireOnlyKeysFrom(Collection<String> allowedKeys, Collectio
         List<String> suggestions = unexpectedKeys.stream()
             .map(invalid -> {
                 List<String> candidates = StringSimilarity.similarStringsIgnoreCase(invalid, allowedKeys);
-                candidates.removeAll(configKeys);
+                if (configKeys instanceof Collection<?>) {
+                    candidates.removeAll((Collection<String>) configKeys);
+                } else {
+                    configKeys.forEach(candidates::remove);
+                }
+
 
                 if (candidates.isEmpty()) {
                     return invalid;
                 }
                 if (candidates.size() == 1) {
                     return String.format(Locale.ENGLISH, "%s (Did you mean [%s]?)", invalid, candidates.get(0));
                 }
-                return String.format(Locale.ENGLISH, "%s (Did you mean one of [%s]?)", invalid, String.join(", ", candidates));
+                return String.format(
+                    Locale.ENGLISH,
+                    "%s (Did you mean one of [%s]?)",
+                    invalid,
+                    String.join(", ", candidates)
+                );
             })
             .collect(Collectors.toList());
 
diff --git a/cypher-aggregation/src/main/java/org/neo4j/gds/projection/CypherAggregation.java b/cypher-aggregation/src/main/java/org/neo4j/gds/projection/CypherAggregation.java
@@ -21,6 +21,7 @@
 
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
+import org.jetbrains.annotations.TestOnly;
 import org.neo4j.gds.NodeLabel;
 import org.neo4j.gds.Orientation;
 import org.neo4j.gds.RelationshipType;
@@ -39,6 +40,7 @@
 import org.neo4j.gds.api.schema.RelationshipSchema;
 import org.neo4j.gds.config.GraphProjectConfig;
 import org.neo4j.gds.core.Aggregation;
+import org.neo4j.gds.core.ConfigKeyValidation;
 import org.neo4j.gds.core.CypherMapWrapper;
 import org.neo4j.gds.core.Username;
 import org.neo4j.gds.core.compress.AdjacencyCompressor;
@@ -87,6 +89,7 @@
 import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReentrantLock;
 
@@ -121,6 +124,8 @@ public static class GraphAggregator {
         private final DatabaseId databaseId;
         private final String username;
 
+        private final ConfigValidator configValidator;
+
         // #result() may be called twice, we cache the result of the first call to return it again in the second invocation
         private @Nullable AggregationResult result;
 
@@ -137,6 +142,7 @@ public static class GraphAggregator {
             this.databaseId = databaseId;
             this.username = username;
             this.lock = new ReentrantLock();
+            this.configValidator = new ConfigValidator();
         }
 
         @UserAggregationUpdate
@@ -168,15 +174,7 @@ public void update(
                     targetNodeLabels = labelsConfig("targetNodeLabels", (MapValue) nodesConfig);
                 }
 
-                // TODO:
-//                if (!nodesConfig.isEmpty()) {
-//                    CypherMapWrapper.create(nodesConfig).requireOnlyKeysFrom(List.of(
-//                        "sourceNodeProperties",
-//                        "sourceNodeLabels",
-//                        "targetNodeProperties",
-//                        "targetNodeLabels"
-//                    ));
-//                }
+                this.configValidator.validateNodesConfig((MapValue) nodesConfig);
             }
 
             var data = initGraphData(
@@ -196,7 +194,8 @@ public void update(
                 targetNodePropertyValues,
                 sourceNodeLabels,
                 targetNodeLabels,
-                relationshipConfig
+                relationshipConfig,
+                this.configValidator
             );
         }
 
@@ -238,7 +237,6 @@ private LazyImporter initGraphData(
         }
 
         private static NodeLabelToken labelsConfig(String nodeLabelKey, @NotNull MapValue nodesConfig) {
-            // TODO: was remove
             var nodeLabelsEntry = nodesConfig.get(nodeLabelKey);
             return tryLabelsConfig(nodeLabelsEntry, nodeLabelKey);
         }
@@ -400,7 +398,8 @@ void update(
             @Nullable MapValue targetNodePropertyValues,
             NodeLabelToken sourceNodeLabels,
             NodeLabelToken targetNodeLabels,
-            AnyValue relationshipConfig
+            AnyValue relationshipConfig,
+            ConfigValidator configValidator
         ) {
             MapValue relationshipProperties = null;
             RelationshipType relationshipType = RelationshipType.ALL_RELATIONSHIPS;
@@ -409,13 +408,7 @@ void update(
                 relationshipProperties = propertiesConfig("properties", (MapValue) relationshipConfig);
                 relationshipType = typeConfig("relationshipType", (MapValue) relationshipConfig);
 
-                // TODO:
-//                if (!relationshipConfig.isEmpty()) {
-//                    CypherMapWrapper.create(relationshipConfig).requireOnlyKeysFrom(List.of(
-//                        "properties",
-//                        "relationshipType"
-//                    ));
-//                }
+                configValidator.validateRelationshipsConfig((MapValue) relationshipConfig);
             }
 
             var intermediateSourceId = loadNode(sourceNode, sourceNodeLabels, sourceNodePropertyValues);
@@ -678,7 +671,6 @@ private static MapValue propertiesConfig(
             String propertyKey,
             @NotNull MapValue propertiesConfig
         ) {
-            // TODO: was remove
             var nodeProperties = propertiesConfig.get(propertyKey);
             if (nodeProperties instanceof MapValue) {
                 return (MapValue) nodeProperties;
@@ -698,7 +690,6 @@ private static RelationshipType typeConfig(
             @SuppressWarnings("SameParameterValue") String relationshipTypeKey,
             @NotNull MapValue relationshipConfig
         ) {
-            // TODO: was remove
             var relationshipTypeEntry = relationshipConfig.get(relationshipTypeKey);
             if (relationshipTypeEntry instanceof TextValue) {
                 return RelationshipType.of(((TextValue) relationshipTypeEntry).stringValue());
@@ -719,6 +710,34 @@ private static long extractNodeId(@NotNull AnyValue node) {
         }
     }
 
+    private static final class ConfigValidator {
+        private static final Set<String> NODES_CONFIG_KEYS = Set.of(
+            "sourceNodeProperties",
+            "sourceNodeLabels",
+            "targetNodeProperties",
+            "targetNodeLabels"
+        );
+
+        private static final Set<String> RELATIONSHIPS_CONFIG_KEYS = Set.of(
+            "properties",
+            "relationshipType"
+        );
+
+        private final AtomicBoolean validateNodes = new AtomicBoolean(true);
+        private final AtomicBoolean validateRelationships = new AtomicBoolean(true);
+
+        void validateNodesConfig(MapValue nodesConfig) {
+            if (this.validateNodes.getAndSet(false)) {
+                ConfigKeyValidation.requireOnlyKeysFrom(NODES_CONFIG_KEYS, nodesConfig.keySet());
+            }
+        }
+
+        void validateRelationshipsConfig(MapValue relationshipConfig) {
+            if (this.validateRelationships.getAndSet(false)) {
+                ConfigKeyValidation.requireOnlyKeysFrom(RELATIONSHIPS_CONFIG_KEYS, relationshipConfig.keySet());
+            }
+        }
+    }
 
     @ValueClass
     @Configuration
@@ -770,6 +789,7 @@ static GraphProjectFromCypherAggregationConfig of(String userName, String graphN
             );
         }
 
+        @TestOnly
         static GraphProjectFromCypherAggregationConfig of(
             String userName,
             String graphName,
diff --git a/cypher-aggregation/src/test/java/org/neo4j/gds/projection/CypherAggregationTest.java b/cypher-aggregation/src/test/java/org/neo4j/gds/projection/CypherAggregationTest.java
@@ -334,9 +334,9 @@ void testEmptyNodeLabel() {
     }
 
     @Test
-    void testDirectLabelMapping() {
-        assertThatThrownBy(() -> runQuery(
-            "MATCH (s) RETURN gds.alpha.graph.project('g', s, null, { sourceNodeLabels: true })"))
+    void testInvalidDirectLabelMapping() {
+        var query = "MATCH (s) RETURN gds.alpha.graph.project('g', s, null, { sourceNodeLabels: true })";
+        assertThatThrownBy(() -> runQuery(query))
             .rootCause()
             .isInstanceOf(IllegalArgumentException.class)
             .hasMessage("Using `true` to load all labels is deprecated, use `{ sourceNodeLabels: labels(s) }` instead");
@@ -711,4 +711,38 @@ void testReadConcurrencyIsParsedCorrectly() {
         assertThat(config.readConcurrency()).isEqualTo(2);
     }
 
+    @Test
+    void testUnknownNodeConfigKeys() {
+        var query = "MATCH (s) RETURN gds.alpha.graph.project('g', s, null, {foo: 'bar'})";
+        assertThatThrownBy(() -> runQuery(query))
+            .rootCause()
+            .hasMessage("Unexpected configuration key: foo");
+    }
+
+    @Test
+    void testUnknownNodeConfigKeysWithSuggestion() {
+        var query = "MATCH (s) RETURN gds.alpha.graph.project('g', s, null, {sourceNode: 'bar'})";
+        assertThatThrownBy(() -> runQuery(query))
+            .rootCause()
+            .hasMessage(
+                "Unexpected configuration key: sourceNode (Did you mean one of [sourceNodeLabels, sourceNodeProperties]?)");
+    }
+
+    @Test
+    void testUnknownRelationshipConfigKeys() {
+        var query = "MATCH (s)-->(t) RETURN gds.alpha.graph.project('g', s, t, null, {foo: 'bar'})";
+        assertThatThrownBy(() -> runQuery(query))
+            .rootCause()
+            .hasMessage("Unexpected configuration key: foo");
+    }
+
+    @Test
+    void testUnknownRelationshipConfigKeysWithSuggestion() {
+        var query = "MATCH (s)--(t) RETURN gds.alpha.graph.project('g', s, null, null, {property: 'bar'})";
+        assertThatThrownBy(() -> runQuery(query))
+            .rootCause()
+            .hasMessage(
+                "Unexpected configuration key: property (Did you mean [properties]?)");
+    }
+
 }
