From 954329675bb30d346f37f3362887983e18b95b56 Mon Sep 17 00:00:00 2001 From: Pontus Melke Date: Thu, 11 Feb 2016 10:52:44 +0100 Subject: [PATCH] Better error message for calling procedures When the static types are wrong we should provide a nice error message. --- ...egatingProcedureExecutablePlanBuilder.scala | 13 +++++++++---- .../compiler/v3_0/spi/ProcedureSignature.scala | 4 +++- .../neo4j/kernel/impl/proc/ProceduresTest.java | 6 +++--- .../java/org/neo4j/procedure/ProcedureIT.java | 18 ++++++++++++++++++ 4 files changed, 33 insertions(+), 8 deletions(-) diff --git a/community/cypher/cypher-compiler-3.0/src/main/scala/org/neo4j/cypher/internal/compiler/v3_0/executionplan/procs/DelegatingProcedureExecutablePlanBuilder.scala b/community/cypher/cypher-compiler-3.0/src/main/scala/org/neo4j/cypher/internal/compiler/v3_0/executionplan/procs/DelegatingProcedureExecutablePlanBuilder.scala index a14b3a5d8db63..5280b35a3b7f1 100644 --- a/community/cypher/cypher-compiler-3.0/src/main/scala/org/neo4j/cypher/internal/compiler/v3_0/executionplan/procs/DelegatingProcedureExecutablePlanBuilder.scala +++ b/community/cypher/cypher-compiler-3.0/src/main/scala/org/neo4j/cypher/internal/compiler/v3_0/executionplan/procs/DelegatingProcedureExecutablePlanBuilder.scala @@ -20,7 +20,7 @@ package org.neo4j.cypher.internal.compiler.v3_0.executionplan.procs import org.neo4j.cypher.internal.compiler.v3_0.executionplan.{ExecutablePlanBuilder, ExecutionPlan, PlanFingerprint, PlanFingerprintReference, SCHEMA_WRITE} -import org.neo4j.cypher.internal.compiler.v3_0.spi.{FieldSignature, PlanContext, ProcedureName, QueryContext} +import org.neo4j.cypher.internal.compiler.v3_0.spi.{FieldSignature, PlanContext, ProcedureName, ProcedureSignature, QueryContext} import org.neo4j.cypher.internal.compiler.v3_0.{CompilationPhaseTracer, PreparedQuery} import org.neo4j.cypher.internal.frontend.v3_0.ast._ import org.neo4j.cypher.internal.frontend.v3_0.symbols.TypeSpec @@ -51,7 +51,7 @@ case class DelegatingProcedureExecutablePlanBuilder(delegate: ExecutablePlanBuil } //type check arguments args.zip(signature.inputSignature).foreach { - case (arg, field) => typeCheck(inputQuery.semanticTable)(arg, field) + case (arg, field) => typeCheck(inputQuery.semanticTable)(arg, field, signature) } } @@ -115,13 +115,18 @@ case class DelegatingProcedureExecutablePlanBuilder(delegate: ExecutablePlanBuil private def typeProp(ctx: QueryContext)(relType: RelTypeName, prop: PropertyKeyName) = (ctx.getOrCreateRelTypeId(relType.name), ctx.getOrCreatePropertyKeyId(prop.name)) - private def typeCheck(semanticTable: SemanticTable)(exp: Expression, field: FieldSignature) = { + private def typeCheck(semanticTable: SemanticTable)(exp: Expression, field: FieldSignature, proc: ProcedureSignature) = { val actual = semanticTable.types(exp).actual val expected = field.typ val intersected = actual intersectOrCoerce expected.covariant if (intersected == TypeSpec.none) throw new CypherTypeException( - s"""${field.name} expects $expected, but got ${actual.mkString(",", "or")}""") + s"""Parameter `${field.name}` for procedure `${proc.name}` + |expects value of type ${semanticTable.types(exp).actual.toShortString} but got value of type ${field.typ}. + | + |Usage: CALL ${proc.name}(${proc.inputSignature.map(s => s"<${s.name}>").mkString(", ")}) + |${proc.inputSignature.map(s => s" ${s.name} (type ${s.typ})").mkString("Parameters:\n", "\n","")} + """.stripMargin) } } diff --git a/community/cypher/cypher-compiler-3.0/src/main/scala/org/neo4j/cypher/internal/compiler/v3_0/spi/ProcedureSignature.scala b/community/cypher/cypher-compiler-3.0/src/main/scala/org/neo4j/cypher/internal/compiler/v3_0/spi/ProcedureSignature.scala index 40388cec3c241..64b1e04bdfb48 100644 --- a/community/cypher/cypher-compiler-3.0/src/main/scala/org/neo4j/cypher/internal/compiler/v3_0/spi/ProcedureSignature.scala +++ b/community/cypher/cypher-compiler-3.0/src/main/scala/org/neo4j/cypher/internal/compiler/v3_0/spi/ProcedureSignature.scala @@ -58,6 +58,8 @@ case class ProcedureSignature(name: ProcedureName, outputSignature: Seq[FieldSignature], mode: ProcedureCallMode = LazyReadOnlyCallMode) -case class ProcedureName(namespace: Seq[String], name: String) +case class ProcedureName(namespace: Seq[String], name: String) { + override def toString = s"""${namespace.mkString(".")}.$name""" +} case class FieldSignature(name: String, typ: CypherType) 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 index 520b26cc47702..af5361ef19f83 100644 --- 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 @@ -90,7 +90,7 @@ public void shouldCallRegisteredProcedure() throws Throwable } @Test - public void shouldNotAllowCallingNonExistantProcedure() throws Throwable + public void shouldNotAllowCallingNonExistingProcedure() throws Throwable { // Expect exception.expect( ProcedureException.class ); @@ -105,7 +105,7 @@ public void shouldNotAllowCallingNonExistantProcedure() throws Throwable } @Test - public void shouldNotAllowRegisteringConflicingName() throws Throwable + public void shouldNotAllowRegisteringConflictingName() throws Throwable { // Given procs.register( procedure ); @@ -145,7 +145,7 @@ public void shouldNotAllowDuplicateFieldNamesInOutput() throws Throwable } @Test - public void shouldSignalNonExistantProcedure() throws Throwable + public void shouldSignalNonExistingProcedure() throws Throwable { // Expect exception.expect( ProcedureException.class ); diff --git a/integrationtests/src/test/java/org/neo4j/procedure/ProcedureIT.java b/integrationtests/src/test/java/org/neo4j/procedure/ProcedureIT.java index 93634c16f5af5..e0096f1d66f71 100644 --- a/integrationtests/src/test/java/org/neo4j/procedure/ProcedureIT.java +++ b/integrationtests/src/test/java/org/neo4j/procedure/ProcedureIT.java @@ -80,6 +80,24 @@ public void shouldCallProcedureWithParameterMap() throws Throwable } } + @Test + public void shouldGiveNiceErrorMessageOnWrongStaticType() throws Throwable + { + //Expect + exception.expect( QueryExecutionException.class ); + exception.expectMessage( + "Parameter `name` for procedure `org.neo4j.procedure.simpleArgument`\n" + + "expects value of type String but got value of type Integer.\n\n" + + "Usage: CALL org.neo4j.procedure.simpleArgument()\n" + + "Parameters:\n" + + " name (type Integer)" ); + // When + try ( Transaction ignore = db.beginTx() ) + { + Result res = db.execute( "CALL org.neo4j.procedure.simpleArgument('42')"); + } + } + @Test public void shouldCallProcedureWithGenericArgument() throws Throwable {