Prevent using wrapper with primitive properties

Co-Authored-By: Max Kiessling <[email protected]>

Author: jjaderberg

algo/src/main/java/org/neo4j/gds/similarity/knn/metrics/NullCheckingNodeProperties.java

@@ -27,13 +27,19 @@
 
 import static org.neo4j.gds.utils.StringFormatting.formatWithLocale;
 
-public class NullCheckingNodeProperties implements NodeProperties {
+public final class NullCheckingNodeProperties implements NodeProperties {
+
+    public static NullCheckingNodeProperties create(NodeProperties properties, String name, IdMap idMap) {
+        var valueType = properties.valueType();
+        assert valueType != ValueType.DOUBLE && valueType != ValueType.LONG: "Don't use NullCheckingNodeProperties for primitive properties";
+        return new NullCheckingNodeProperties(properties, name, idMap);
+    }
 
     private final NodeProperties properties;
     private final String name;
     private final IdMap idMap;
 
-    NullCheckingNodeProperties(NodeProperties properties, String name, IdMap idMap) {
+    private NullCheckingNodeProperties(NodeProperties properties, String name, IdMap idMap) {
         this.properties = properties;
         this.name = name;
         this.idMap = idMap;

algo/src/main/java/org/neo4j/gds/similarity/knn/metrics/SimilarityComputer.java

@@ -78,19 +78,19 @@ static SimilarityComputer ofProperty(
             case DOUBLE_ARRAY:
                 return ofDoubleArrayProperty(
                     name,
-                    new NullCheckingNodeProperties(properties, name, idMap),
+                    NullCheckingNodeProperties.create(properties, name, idMap),
                     defaultSimilarityMetric
                 );
             case FLOAT_ARRAY:
                 return ofFloatArrayProperty(
                     name,
-                    new NullCheckingNodeProperties(properties, name, idMap),
+                    NullCheckingNodeProperties.create(properties, name, idMap),
                     defaultSimilarityMetric
                 );
             case LONG_ARRAY:
                 return ofLongArrayProperty(
                     name,
-                    new SortedLongArrayProperties(new NullCheckingNodeProperties(properties, name, idMap)),
+                    new SortedLongArrayProperties(NullCheckingNodeProperties.create(properties, name, idMap)),
                     defaultSimilarityMetric
                 );
             default:

algo/src/test/java/org/neo4j/gds/similarity/knn/metrics/NullCheckingNodePropertiesTest.java

@@ -43,7 +43,7 @@ class NullCheckingNodePropertiesTest {
     @Test
     void shouldWrapDoubleArrayPropertyAccess() {
         var props = new DoubleArrayTestProperties(nodeId -> new double[]{nodeId});
-        var wrappedProps = new NullCheckingNodeProperties(props, "propertyName", graph);
+        var wrappedProps = NullCheckingNodeProperties.create(props, "propertyName", graph);
         graph.forEachNode(nodeId -> {
                 assertThat(wrappedProps.doubleArrayValue(nodeId))
                     .isEqualTo(props.doubleArrayValue(nodeId));
@@ -55,7 +55,7 @@ void shouldWrapDoubleArrayPropertyAccess() {
     @Test
     void shouldWrapFloatArrayPropertyAccess() {
         var props = new FloatArrayTestProperties(nodeId -> new float[]{nodeId});
-        var wrappedProps = new NullCheckingNodeProperties(props, "propertyName", graph);
+        var wrappedProps = NullCheckingNodeProperties.create(props, "propertyName", graph);
         graph.forEachNode(nodeId -> {
                 assertThat(wrappedProps.floatArrayValue(nodeId))
                     .isEqualTo(props.floatArrayValue(nodeId));
@@ -67,7 +67,7 @@ void shouldWrapFloatArrayPropertyAccess() {
     @Test
     void shouldWrapLongArrayPropertyAccess() {
         var props = new LongArrayTestProperties(nodeId -> new long[]{nodeId});
-        var wrappedProps = new NullCheckingNodeProperties(props, "propertyName", graph);
+        var wrappedProps = NullCheckingNodeProperties.create(props, "propertyName", graph);
         graph.forEachNode(nodeId -> {
                 assertThat(wrappedProps.longArrayValue(nodeId))
                     .isEqualTo(props.longArrayValue(nodeId));
@@ -79,7 +79,7 @@ void shouldWrapLongArrayPropertyAccess() {
     @Test
     void shouldThrowUsefullyForNullDoubleArrayValues() {
         var nullProps = new DoubleArrayTestProperties(nodeId -> null);
-        var nonNullProps = new NullCheckingNodeProperties(nullProps, "propertyName", graph);
+        var nonNullProps = NullCheckingNodeProperties.create(nullProps, "propertyName", graph);
         assertThatThrownBy(() -> nonNullProps.doubleArrayValue(1))
             .isInstanceOf(IllegalArgumentException.class)
             .hasMessageContaining("Missing `List of Float` node property `propertyName` for node with id `1`.");
@@ -88,7 +88,7 @@ void shouldThrowUsefullyForNullDoubleArrayValues() {
     @Test
     void shouldThrowUsefullyForNullFloatArrayValues() {
         var nullProps = new FloatArrayTestProperties(nodeId -> null);
-        var nonNullProps = new NullCheckingNodeProperties(nullProps, "propertyName", graph);
+        var nonNullProps = NullCheckingNodeProperties.create(nullProps, "propertyName", graph);
         assertThatThrownBy(() -> nonNullProps.floatArrayValue(1))
             .isInstanceOf(IllegalArgumentException.class)
             .hasMessageContaining("Missing `List of Float` node property `propertyName` for node with id `1`.");
@@ -97,9 +97,18 @@ void shouldThrowUsefullyForNullFloatArrayValues() {
     @Test
     void shouldThrowUsefullyForNullLongArrayValues() {
         var nullProps = new LongArrayTestProperties(nodeId -> null);
-        var nonNullProps = new NullCheckingNodeProperties(nullProps, "propertyName", graph);
+        var nonNullProps = NullCheckingNodeProperties.create(nullProps, "propertyName", graph);
         assertThatThrownBy(() -> nonNullProps.longArrayValue(1))
             .isInstanceOf(IllegalArgumentException.class)
             .hasMessageContaining("Missing `List of Integer` node property `propertyName` for node with id `1`.");
     }
+
+    @Test
+    void shouldThrowNormallyIfTheWrongPropertyIsAccessed() {
+        var longProps = new LongArrayTestProperties(i -> new long[]{i});
+        var wrappedProps = NullCheckingNodeProperties.create(longProps, "propertyName", graph);
+        assertThatThrownBy(() -> wrappedProps.doubleArrayValue(0))
+            .isInstanceOf(UnsupportedOperationException.class)
+            .hasMessage("Tried to retrieve a value of type DOUBLE_ARRAY value from properties of type LONG_ARRAY");
+    }
 }