Enforce that non-VOID procedures must return at least a single output…

… field o This aligns procedures better with how Cypher itself handles return columns and probably should have been like this initially
neo4j · Sep 16, 2016 · cca93de · cca93de
1 parent efdc9b2
commit cca93de
Show file tree

Hide file tree

Showing 4 changed files with 33 additions and 42 deletions.
diff --git a/.../acceptance-spec-suite/src/test/resources/cypher/features/ProcedureCallAcceptance.feature b/.../acceptance-spec-suite/src/test/resources/cypher/features/ProcedureCallAcceptance.feature
@@ -142,36 +142,6 @@ Feature: ProcedureCallAcceptance
     Then the result should be empty
     And no side effects
 
-  Scenario: In-query call to procedure that takes no arguments and yields no results
-    And there exists a procedure test.doNothing() :: ():
-      |
-    When executing query:
-    """
-    CALL test.doNothing() RETURN 1
-    """
-    Then the result should be empty
-    And no side effects
-
-  Scenario: Standalone call to procedure that takes no arguments and yields no results
-    And there exists a procedure test.doNothing() :: ():
-      |
-    When executing query:
-    """
-    CALL test.doNothing()
-    """
-    Then the result should be empty
-    And no side effects
-
-  Scenario: Standalone call to procedure that takes no arguments and yields no results, called with implicit arguments
-    And there exists a procedure test.doNothing() :: ():
-      |
-    When executing query:
-    """
-    CALL test.doNothing
-    """
-    Then the result should be empty
-    And no side effects
-
   Scenario: In-query call to procedure with explicit arguments
     And there exists a procedure test.my.proc(name :: STRING?, id :: INTEGER?) :: (city :: STRING?, country_code :: INTEGER?):
       | name     | id | city      | country_code |

diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/proc/ProcedureRegistry.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/proc/ProcedureRegistry.java
@@ -57,6 +57,14 @@ public void register( CallableProcedure proc, boolean overrideCurrentImplementat
         validateSignature( descriptiveName, signature.inputSignature(), "input" );
         validateSignature( descriptiveName, signature.outputSignature(), "output" );
 
+        if ( ! signature.isVoid() && signature.outputSignature().isEmpty() )
+        {
+            throw new ProcedureException(
+                Status.Procedure.ProcedureRegistrationFailed,
+                "Procedures with zero output fields must be declared as VOID"
+            );
+        }
+
         CallableProcedure oldImplementation = procedures.get( name );
         if ( oldImplementation == null )
         {

diff --git a/...ty/kernel/src/test/java/org/neo4j/kernel/impl/api/integrationtest/ProceduresKernelIT.java b/...ty/kernel/src/test/java/org/neo4j/kernel/impl/api/integrationtest/ProceduresKernelIT.java
@@ -78,16 +78,16 @@ public void shouldGetBuiltInProcedureByName() throws Throwable
 
         // Then
         assertThat( found, equalTo( procedureSignature( procedureName( "db", "labels" ) )
-                .out(  "label", Neo4jTypes.NTString ).build() ) );
+                .out(  "label", NTString ).build() ) );
     }
 
     @Test
     public void shouldGetAllProcedures() throws Throwable
     {
         // Given
         kernel.registerProcedure( procedure );
-        kernel.registerProcedure( procedure( procedureSignature( "example", "exampleProc2" ).build() ) );
-        kernel.registerProcedure( procedure( procedureSignature( "example", "exampleProc3" ).build() ) );
+        kernel.registerProcedure( procedure( procedureSignature( "example", "exampleProc2" ).out( "name", NTString ).build() ) );
+        kernel.registerProcedure( procedure( procedureSignature( "example", "exampleProc3" ).out( "name", NTString ).build() ) );
 
         // When
         List<ProcedureSignature> signatures =
@@ -96,8 +96,19 @@ public void shouldGetAllProcedures() throws Throwable
         // Then
         assertThat( signatures, hasItems(
             procedure.signature(),
-            procedureSignature( "example", "exampleProc2" ).build(),
-            procedureSignature( "example", "exampleProc3" ).build() ) );
+            procedureSignature( "example", "exampleProc2" ).out( "name", NTString ).build(),
+            procedureSignature( "example", "exampleProc3" ).out( "name", NTString ).build() ) );
+    }
+
+    @Test
+    public void shouldRefuseToRegisterNonVoidProcedureWithoutOutputs() throws ProcedureException
+    {
+        // Then
+        exception.expect( ProcedureException.class );
+        exception.expectMessage( "Procedures with zero output fields must be declared as VOID" );
+
+        // When
+        kernel.registerProcedure( procedure( procedureSignature( "example", "exampleProc2" ).build() ) );
     }
 
     @Test

diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/proc/ProceduresTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/proc/ProceduresTest.java
@@ -44,6 +44,8 @@
 import static org.neo4j.helpers.collection.Iterators.asList;
 import static org.neo4j.kernel.api.proc.Key.key;
 import static org.neo4j.kernel.api.proc.Neo4jTypes.NTAny;
+import static org.neo4j.kernel.api.proc.Neo4jTypes.NTInteger;
+import static org.neo4j.kernel.api.proc.Neo4jTypes.NTString;
 import static org.neo4j.kernel.api.proc.ProcedureSignature.procedureSignature;
 
 public class ProceduresTest
@@ -52,7 +54,7 @@ public class ProceduresTest
     public ExpectedException exception = ExpectedException.none();
 
     private final Procedures procs = new Procedures();
-    private final ProcedureSignature signature = procedureSignature( "org", "myproc" ).build();
+    private final ProcedureSignature signature = procedureSignature( "org", "myproc" ).out( "name", NTString ).build();
     private final CallableProcedure procedure = procedure( signature );
 
     @Test
@@ -69,16 +71,16 @@ public void shouldGetRegisteredProcedure() throws Throwable
     public void shouldGetAllRegisteredProcedures() throws Throwable
     {
         // When
-        procs.register( procedure( procedureSignature( "org", "myproc1" ).build() ) );
-        procs.register( procedure( procedureSignature( "org", "myproc2" ).build() ) );
-        procs.register( procedure( procedureSignature( "org", "myproc3" ).build() ) );
+        procs.register( procedure( procedureSignature( "org", "myproc1" ).out( "age", NTInteger ).build() ) );
+        procs.register( procedure( procedureSignature( "org", "myproc2" ).out( "age", NTInteger ).build() ) );
+        procs.register( procedure( procedureSignature( "org", "myproc3" ).out( "age", NTInteger ).build() ) );
 
         // Then
         List<ProcedureSignature> signatures = Iterables.asList( procs.getAllProcedures() );
         assertThat( signatures, containsInAnyOrder(
-                procedureSignature( "org", "myproc1" ).build(),
-                procedureSignature( "org", "myproc2" ).build(),
-                procedureSignature( "org", "myproc3" ).build() ) );
+                procedureSignature( "org", "myproc1" ).out( "age", NTInteger ).build(),
+                procedureSignature( "org", "myproc2" ).out( "age", NTInteger ).build(),
+                procedureSignature( "org", "myproc3" ).out( "age", NTInteger ).build() ) );
     }
 
     @Test