Merge pull request #10675 from Lojjs/3.3-missing-parameter-check

Checking parameter list explicitly

community/cypher/cypher/src/main/scala/org/neo4j/cypher/internal/ExecutionEngine.scala

@@ -25,6 +25,7 @@ import org.neo4j.cypher._
 import org.neo4j.cypher.internal.compatibility.v3_3._
 import org.neo4j.cypher.internal.compatibility.v3_3.runtime.helpers.{RuntimeJavaValueConverter, RuntimeScalaValueConverter, ValueConversion}
 import org.neo4j.cypher.internal.compiler.v3_3.prettifier.Prettifier
+import org.neo4j.cypher.internal.frontend.v3_3.ParameterNotFoundException
 import org.neo4j.cypher.internal.frontend.v3_3.phases.CompilationPhaseTracer
 import org.neo4j.cypher.internal.spi.v3_3.TransactionalContextWrapper
 import org.neo4j.cypher.internal.tracing.{CompilationTracer, TimingCompilationTracer}
@@ -73,7 +74,7 @@ class ExecutionEngine(val queryService: GraphDatabaseQueryService,
 
   private val executionMonitor = kernelMonitors.newMonitor(classOf[QueryExecutionMonitor])
 
-  private val cacheAccessor = new MonitoringCacheAccessor[String, (ExecutionPlan, Map[String, Any])](cacheMonitor)
+  private val cacheAccessor = new MonitoringCacheAccessor[String, (ExecutionPlan, Map[String, Any], Seq[String])](cacheMonitor)
 
   private val preParsedQueries = new LFUCache[String, PreParsedQuery](getPlanCacheSize)
   private val parsedQueries = new LFUCache[String, ParsedQuery](getPlanCacheSize)
@@ -93,10 +94,10 @@ class ExecutionEngine(val queryService: GraphDatabaseQueryService,
     profile(query, ValueConversion.asValues(scalaParams), context)
   }
 
-  def profile(query: String, mapValue: MapValue, context: TransactionalContext): Result = {
-    // we got deep java parameters => convert to shallow scala parameters for passing into the engine
-    val (preparedPlanExecution, wrappedContext) = planQuery(context)
-    preparedPlanExecution.profile(wrappedContext, mapValue)
+  def profile(query: String, mapParams: MapValue, context: TransactionalContext): Result = {
+    val (preparedPlanExecution, wrappedContext, queryParamNames) = planQuery(context)
+    checkParameters(queryParamNames, mapParams, preparedPlanExecution.extractedParams)
+    preparedPlanExecution.profile(wrappedContext, mapParams)
   }
 
   def execute(query: String, scalaParams: Map[String, Any], context: TransactionalContext): Result = {
@@ -112,7 +113,10 @@ class ExecutionEngine(val queryService: GraphDatabaseQueryService,
   }
 
   def execute(query: String, mapParams: MapValue, context: TransactionalContext): Result = {
-    val (preparedPlanExecution, wrappedContext) = planQuery(context)
+    val (preparedPlanExecution, wrappedContext, queryParamNames) = planQuery(context)
+    if (preparedPlanExecution.executionMode.name != "explain") {
+      checkParameters(queryParamNames, mapParams, preparedPlanExecution.extractedParams)
+    }
     preparedPlanExecution.execute(wrappedContext, mapParams)
   }
 
@@ -134,7 +138,7 @@ class ExecutionEngine(val queryService: GraphDatabaseQueryService,
     preParsedQueries.getOrElseUpdate(queryText, queryDispatcher.preParseQuery(queryText))
 
   @throws(classOf[SyntaxException])
-  protected def planQuery(transactionalContext: TransactionalContext): (PreparedPlanExecution, TransactionalContextWrapper) = {
+  protected def planQuery(transactionalContext: TransactionalContext): (PreparedPlanExecution, TransactionalContextWrapper, Seq[String]) = {
     val executingQuery = transactionalContext.executingQuery()
     val queryText = executingQuery.queryText()
     executionMonitor.startQueryExecution(executingQuery)
@@ -161,23 +165,23 @@ class ExecutionEngine(val queryService: GraphDatabaseQueryService,
         // NOTE: This will force read access mode if the current transaction did not have it
         val revertable = tc.restrictCurrentTransaction(tc.securityContext.withMode(AccessMode.Static.READ))
 
-        val ((plan: ExecutionPlan, extractedParameters), touched) = try {
+        val ((plan: ExecutionPlan, extractedParameters, queryParamNames), touched) = try {
           // fetch plan cache
-          val cache: QueryCache[String, (ExecutionPlan, Map[String, Any])] = getOrCreateFromSchemaState(tc.readOperations, {
+          val cache: QueryCache[String, (ExecutionPlan, Map[String, Any], Seq[String])] = getOrCreateFromSchemaState(tc.readOperations, {
             cacheMonitor.cacheFlushDetected(tc.statement)
-            val lruCache = new LFUCache[String, (ExecutionPlan, Map[String, Any])](getPlanCacheSize)
+            val lruCache = new LFUCache[String, (ExecutionPlan, Map[String, Any], Seq[String])](getPlanCacheSize)
             new QueryCache(cacheAccessor, lruCache)
           })
 
-          def isStale(plan: ExecutionPlan, ignored: Map[String, Any]) = plan.isStale(lastCommittedTxId, tc)
+          def isStale(plan: ExecutionPlan, ignored1: Map[String, Any], ignored2: Seq[String]) = plan.isStale(lastCommittedTxId, tc)
 
           def producePlan() = {
             val parsedQuery = parsePreParsedQuery(preParsedQuery, phaseTracer)
             parsedQuery.plan(tc, phaseTracer)
           }
 
           val stateBefore = schemaState(tc)
-          var (plan: (ExecutionPlan, Map[String, Any]), touched: Boolean) = cache.getOrElseUpdate(cacheKey, queryText, (isStale _).tupled, producePlan())
+          var (plan: (ExecutionPlan, Map[String, Any], Seq[String]), touched: Boolean) = cache.getOrElseUpdate(cacheKey, queryText, (isStale _).tupled, producePlan())
           if (!touched) {
             val labelIds: Seq[Long] = extractPlanLabels(plan, preParsedQuery.version, tc)
             if (labelIds.nonEmpty) {
@@ -206,7 +210,7 @@ class ExecutionEngine(val queryService: GraphDatabaseQueryService,
         } else {
           tc.cleanForReuse()
           tc.notifyPlanningCompleted(plan)
-          return (PreparedPlanExecution(plan, executionMode, extractedParameters), tc)
+          return (PreparedPlanExecution(plan, executionMode, extractedParameters), tc, queryParamNames)
         }
 
         n += 1
@@ -216,6 +220,16 @@ class ExecutionEngine(val queryService: GraphDatabaseQueryService,
     throw new IllegalStateException("Could not execute query due to insanely frequent schema changes")
   }
 
+  @throws(classOf[ParameterNotFoundException])
+  private def checkParameters(queryParams: Seq[String], givenParams: MapValue, extractedParams: Map[String, Any]) {
+    exceptionHandler.runSafely {
+      val missingKeys = queryParams.filter(key => !(givenParams.containsKey(key) || extractedParams.contains(key)))
+      if (missingKeys.nonEmpty) {
+        throw new ParameterNotFoundException("Expected parameter(s): " + missingKeys.mkString(", "))
+      }
+    }
+  }
+
   private def releasePlanLabels(tc: TransactionalContextWrapper, labelIds: Seq[Long]) = {
     tc.readOperations.releaseShared(ResourceTypes.LABEL, labelIds.toArray[Long]:_*)
   }
@@ -224,7 +238,7 @@ class ExecutionEngine(val queryService: GraphDatabaseQueryService,
     tc.readOperations.acquireShared(ResourceTypes.LABEL, labelIds.toArray[Long]:_*)
   }
 
-  private def extractPlanLabels(plan: (ExecutionPlan, Map[String, Any]), version: CypherVersion, tc:
+  private def extractPlanLabels(plan: (ExecutionPlan, Map[String, Any], Seq[String]), version: CypherVersion, tc:
   TransactionalContextWrapper): Seq[Long] = {
     import scala.collection.JavaConverters._
 
@@ -246,7 +260,7 @@ class ExecutionEngine(val queryService: GraphDatabaseQueryService,
   }
 
   private def schemaState(tc: TransactionalContextWrapper): QueryCache[MonitoringCacheAccessor[String,
-    (ExecutionPlan, Map[String, Any])], LFUCache[String, (ExecutionPlan, Map[String, Any])]] = {
+    (ExecutionPlan, Map[String, Any], Seq[String])], LFUCache[String, (ExecutionPlan, Map[String, Any], Seq[String])]] = {
     tc.readOperations.schemaStateGet(this)
   }
 

community/cypher/cypher/src/main/scala/org/neo4j/cypher/internal/ParsedQuery.scala

@@ -26,7 +26,7 @@ import scala.util.Try
 
 trait ParsedQuery {
   protected def trier: Try[{ def isPeriodicCommit: Boolean }]
-  def plan(transactionContext: TransactionalContextWrapper, tracer: CompilationPhaseTracer): (ExecutionPlan, Map[String, Any])
+  def plan(transactionContext: TransactionalContextWrapper, tracer: CompilationPhaseTracer): (ExecutionPlan, Map[String, Any], Seq[String])
   final def isPeriodicCommit: Boolean = trier.map(_.isPeriodicCommit).getOrElse(false)
   final def hasErrors: Boolean = trier.isFailure
   final def onError[T](f: Throwable => T): Option[T] = trier.failed.toOption.map(f)

community/cypher/cypher/src/main/scala/org/neo4j/cypher/internal/compatibility/v2_3/Compatibility.scala

@@ -79,7 +79,7 @@ trait Compatibility {
                                 Some(as2_3(preParsedQuery.offset)), tracer))
     new ParsedQuery {
       def plan(transactionalContext: TransactionalContextWrapper,
-               tracer: v3_3.phases.CompilationPhaseTracer): (org.neo4j.cypher.internal.ExecutionPlan, Map[String, Any]) = exceptionHandler
+               tracer: v3_3.phases.CompilationPhaseTracer): (org.neo4j.cypher.internal.ExecutionPlan, Map[String, Any], Seq[String]) = exceptionHandler
         .runSafely {
           val planContext: PlanContext = new TransactionBoundPlanContext(transactionalContext)
           val (planImpl, extractedParameters) = compiler
@@ -88,7 +88,7 @@ trait Compatibility {
           // Log notifications/warnings from planning
           planImpl.notifications(planContext).foreach(notificationLogger += _)
 
-          (new ExecutionPlanWrapper(planImpl, preParsingNotifications, as2_3(preParsedQuery.offset)), extractedParameters)
+          (new ExecutionPlanWrapper(planImpl, preParsingNotifications, as2_3(preParsedQuery.offset)), extractedParameters, Seq.empty[String])
         }
 
       override protected val trier = preparedQueryForV_2_3

community/cypher/cypher/src/main/scala/org/neo4j/cypher/internal/compatibility/v3_1/Compatibility.scala

@@ -77,7 +77,7 @@ trait Compatibility {
     new ParsedQuery {
       override def plan(transactionalContext: TransactionalContextWrapperV3_3,
                         tracer: frontend.v3_3.phases.CompilationPhaseTracer):
-      (ExecutionPlan, Map[String, Any]) =
+      (ExecutionPlan, Map[String, Any], Seq[String]) =
         exceptionHandler.runSafely {
           val tc = TransactionalContextWrapperV3_1(transactionalContext.tc)
           val planContext = new ExceptionTranslatingPlanContext(new TransactionBoundPlanContext(tc, notificationLogger))
@@ -89,7 +89,7 @@ trait Compatibility {
           // Log notifications/warnings from planning
           planImpl.notifications(planContext).foreach(notificationLogger += _)
 
-          (new ExecutionPlanWrapper(planImpl, preParsingNotifications, as3_1(preParsedQuery.offset)), extractedParameters)
+          (new ExecutionPlanWrapper(planImpl, preParsingNotifications, as3_1(preParsedQuery.offset)), extractedParameters, Seq.empty[String])
         }
 
       override protected val trier: Try[PreparedQuerySyntax] = preparedSyntacticQueryForV_3_1

community/cypher/cypher/src/main/scala/org/neo4j/cypher/internal/compatibility/v3_2/Compatibility.scala

@@ -77,7 +77,7 @@ trait Compatibility[C <: CompilerContext] {
     new ParsedQuery {
       override def plan(transactionalContext: TransactionalContextWrapperV3_3,
                         tracer: frontend.v3_3.phases.CompilationPhaseTracer):
-      (ExecutionPlan, Map[String, Any]) = exceptionHandler.runSafely {
+      (ExecutionPlan, Map[String, Any], Seq[String]) = exceptionHandler.runSafely {
         val tc = TransactionalContextWrapperV3_2(transactionalContext.tc)
         val planContext = new ExceptionTranslatingPlanContext(new TransactionBoundPlanContext(tc, notificationLogger))
         val syntacticQuery = preparedSyntacticQueryForV_3_2.get
@@ -98,7 +98,7 @@ trait Compatibility[C <: CompilerContext] {
           pos3_2,
           searchMonitor,
           executionMonitor)
-        (executionPlanWrapper, extractedParameters)
+        (executionPlanWrapper, extractedParameters, Seq.empty[String])
       }
 
       override protected val trier: Try[BaseState] = preparedSyntacticQueryForV_3_2

community/cypher/cypher/src/main/scala/org/neo4j/cypher/internal/compatibility/v3_3/Compatibility.scala

@@ -35,7 +35,7 @@ import org.neo4j.cypher.internal.compiler.v3_3.planner.logical.idp._
 import org.neo4j.cypher.internal.compiler.v3_3.planner.logical.{CachedMetricsFactory, QueryGraphSolver, SimpleMetricsFactory}
 import org.neo4j.cypher.internal.compiler.v3_3.spi.PlanContext
 import org.neo4j.cypher.internal.frontend.v3_3.InputPosition
-import org.neo4j.cypher.internal.frontend.v3_3.ast.Statement
+import org.neo4j.cypher.internal.frontend.v3_3.ast.{Parameter, Statement}
 import org.neo4j.cypher.internal.frontend.v3_3.helpers.rewriting.RewriterStepSequencer
 import org.neo4j.cypher.internal.frontend.v3_3.phases._
 import org.neo4j.cypher.internal.javacompat.ExecutionResult
@@ -96,16 +96,16 @@ trait Compatibility[CONTEXT <: CommunityRuntimeContext,
                          preParsingNotifications: Set[org.neo4j.graphdb.Notification]): ParsedQuery = {
     val notificationLogger = new RecordingNotificationLogger(Some(preParsedQuery.offset))
 
-    val preparedSyntacticQueryForV_3_2 =
+    val preparedSyntacticQueryForV_3_3 =
       Try(compiler.parseQuery(preParsedQuery.statement,
                               preParsedQuery.rawStatement,
                               notificationLogger, preParsedQuery.planner.name,
                               preParsedQuery.debugOptions,
                               Some(preParsedQuery.offset), tracer))
     new ParsedQuery {
       override def plan(transactionalContext: TransactionalContextWrapper, tracer: CompilationPhaseTracer):
-        (ExecutionPlan, Map[String, Any]) = exceptionHandler.runSafely {
-        val syntacticQuery = preparedSyntacticQueryForV_3_2.get
+        (ExecutionPlan, Map[String, Any], Seq[String]) = exceptionHandler.runSafely {
+        val syntacticQuery = preparedSyntacticQueryForV_3_3.get
 
         //Context used for db communication during planning
         val planContext = new ExceptionTranslatingPlanContext(new TransactionBoundPlanContext(transactionalContext, notificationLogger))
@@ -118,6 +118,7 @@ trait Compatibility[CONTEXT <: CommunityRuntimeContext,
                                                         clock, simpleExpressionEvaluator)
         //Prepare query for caching
         val preparedQuery = compiler.normalizeQuery(syntacticQuery, context)
+        val queryParamNames: Seq[String] = preparedQuery.statement().findByAllClass[Parameter].map(x => x.name)
         val cache = provideCache(cacheAccessor, cacheMonitor, planContext, planCacheFactory)
         val isStale = (plan: ExecutionPlan_v3_3) => plan.isStale(planContext.txIdProvider, planContext.statistics)
 
@@ -135,10 +136,10 @@ trait Compatibility[CONTEXT <: CommunityRuntimeContext,
         // Log notifications/warnings from planning
        executionPlan.notifications(planContext).foreach(notificationLogger.log)
 
-        (new ExecutionPlanWrapper(executionPlan, preParsingNotifications, preParsedQuery.offset), preparedQuery.extractedParams())
+        (new ExecutionPlanWrapper(executionPlan, preParsingNotifications, preParsedQuery.offset), preparedQuery.extractedParams(), queryParamNames)
       }
 
-      override protected val trier: Try[BaseState] = preparedSyntacticQueryForV_3_2
+      override protected val trier: Try[BaseState] = preparedSyntacticQueryForV_3_3
     }
   }
 

enterprise/cypher/acceptance-spec-suite/src/test/scala/org/neo4j/internal/cypher/acceptance/MatchAcceptanceTest.scala

@@ -84,7 +84,7 @@ class MatchAcceptanceTest extends ExecutionEngineFunSuite with QueryStatisticsTe
         |MATCH (rfq:Study_Dataset:RFQ) WHERE (rfq IN datasets)
         |OPTIONAL MATCH (wasi:Study_Dataset:WASI) WHERE (wasi IN datasets)
         |OPTIONAL MATCH (ypq:Study_Dataset:YPQ) WHERE (ypq IN datasets)
-        |RETURN DISTINCT derivedData AS DerivedData, subject , stai, pswq, pss, njre_q_r, iu, gad_7, fmps, bdi, wdq, treasurehunt, scid_v2, ybocs, bis, sdq, ehi, oci_r, pi_wsur, rfq, wasi, ypq""".stripMargin, Map.empty)
+        |RETURN DISTINCT derivedData AS DerivedData, subject , stai, pswq, pss, njre_q_r, iu, gad_7, fmps, bdi, wdq, treasurehunt, scid_v2, ybocs, bis, sdq, ehi, oci_r, pi_wsur, rfq, wasi, ypq""".stripMargin, Map("studyUUID" -> 1))
   }
 
   test("Should not use both pruning var expand and projections that need path info") {

enterprise/cypher/acceptance-spec-suite/src/test/scala/org/neo4j/internal/cypher/acceptance/NodeIndexSeekByRangeAcceptanceTest.scala

@@ -1051,7 +1051,7 @@ class NodeIndexSeekByRangeAcceptanceTest extends ExecutionEngineFunSuite with Cy
       planComparisonStrategy = ComparePlansWithAssertion((plan) => {
         //THEN
         plan should useOperators(IndexSeekByRange.name)
-      }, Configs.AllRulePlanners))
+      }, Configs.AllRulePlanners), params = Map("param" -> Array[Int](1, 2, 3)))
     result.toList should be(empty)
 
     an[IllegalArgumentException] should be thrownBy {

enterprise/cypher/acceptance-spec-suite/src/test/scala/org/neo4j/internal/cypher/acceptance/ParameterValuesAcceptanceTest.scala

@@ -119,4 +119,51 @@ class ParameterValuesAcceptanceTest extends ExecutionEngineFunSuite with CypherC
       r.hasProperty("name") should equal(false)
     }
   }
+
+  test("match with missing parameter should return error for empty db") {
+    // all versions of 3.3
+    val config = Configs.Version3_3 + Configs.Procs - Configs.AllRulePlanners
+
+    failWithError(config, "MATCH (n:Person {name:{name}}) RETURN n", Seq("Expected parameter(s): name"))
+  }
+
+  test("match with missing parameter should return error for non-empty db") {
+    val config = Configs.Version3_3 + Configs.Procs - Configs.AllRulePlanners - Configs.Compiled
+    failWithError(config, "CREATE (n:Person) WITH n MATCH (n:Person {name:{name}}) RETURN n", Seq("Expected parameter(s): name"))
+  }
+
+  test("match with multiple missing parameters should return error for empty db") {
+    // all versions of 3.3
+    val config = Configs.Version3_3 + Configs.Procs - Configs.AllRulePlanners
+
+    failWithError(config, "MATCH (n:Person {name:{name}, age:{age}}) RETURN n", Seq("Expected parameter(s): name, age"))
+  }
+
+  test("match with multiple missing parameters should return error for non-empty db") {
+    val config = Configs.Version3_3 + Configs.Procs - Configs.AllRulePlanners - Configs.Compiled
+    failWithError(config, "CREATE (n:Person) WITH n MATCH (n:Person {name:{name}, age:{age}}) RETURN n", Seq("Expected parameter(s): name, age"))
+  }
+
+  test("match with misspelled parameter should return error for empty db") {
+    // all versions of 3.3
+    val config = Configs.Version3_3 + Configs.Procs - Configs.AllRulePlanners
+
+    failWithError(config, "MATCH (n:Person {name:{name}}) RETURN n", Seq("Expected parameter(s): name"), params = "nam" -> "Neo")
+  }
+
+  test("match with misspelled parameter should return error for non-empty db") {
+    val config = Configs.Version3_3 + Configs.Procs - Configs.AllRulePlanners - Configs.Compiled
+    failWithError(config, "CREATE (n:Person) WITH n MATCH (n:Person {name:{name}}) RETURN n", Seq("Expected parameter(s): name"), params = "nam" -> "Neo")
+  }
+
+  test("explain with missing parameter should NOT return error for empty db") {
+    val config = Configs.All
+    executeWith(config, "EXPLAIN MATCH (n:Person {name:{name}}) RETURN n")
+  }
+
+  test("explain with missing parameter should NOT return error for non-empty db") {
+    val config = Configs.Interpreted - Configs.Cost2_3
+    executeWith(config, "EXPLAIN CREATE (n:Person) WITH n MATCH (n:Person {name:{name}}) RETURN n")
+  }
+
 }

enterprise/cypher/acceptance-spec-suite/src/test/scala/org/neo4j/internal/cypher/acceptance/QueryPlanCompactionAcceptanceTest.scala

@@ -792,7 +792,8 @@ class QueryPlanCompactionAcceptanceTest extends ExecutionEngineFunSuite with Que
         || +LoadCSV                |              1 | line                      |                       |
         |+-------------------------+----------------+---------------------------+-----------------------+
         |""".stripMargin
-    executeWith(expectedToSucceed - Configs.SlottedInterpreted, query, planComparisonStrategy = ComparePlansWithAssertion(_ should matchPlan(expectedPlan), expectPlansToFail = Configs.All - Configs.Cost3_3))
+    executeWith(expectedToSucceed - Configs.SlottedInterpreted, query, planComparisonStrategy = ComparePlansWithAssertion(_ should matchPlan(expectedPlan),
+                expectPlansToFail = Configs.All - Configs.Cost3_3), params = Map("csv_filename" -> "x"))
   }
 
   test("Don't compact query with consecutive expands due to presence of values in 'other' column") {

enterprise/cypher/spec-suite-tools/src/test/scala/cypher/feature/steps/SpecSuiteErrorHandler.scala

@@ -183,6 +183,8 @@ case class SpecSuiteErrorHandler(typ: String, phase: String, detail: String) ext
       detail should equal("InvalidNumberOfArguments")
     else if (msg.matches("Expected a parameter named .+"))
       detail should equal("MissingParameter")
+    else if (msg.matches("Expected parameter\\(s\\): .+"))
+      detail should equal("MissingParameter")
     else if (msg.startsWith("Procedure call cannot take an aggregating function as argument, please add a 'WITH' to your statement."))
       detail should equal("InvalidAggregation")
     else if (msg.startsWith("Procedure call inside a query does not support passing arguments implicitly (pass explicitly after procedure name instead)"))