From 48b28df7b1e3c5bc500bf0958dc67534a413ef44 Mon Sep 17 00:00:00 2001 From: fickludd Date: Tue, 12 Jun 2018 16:03:34 +0200 Subject: [PATCH] Fix: revert to ignoring erroneous parameter values if they are not used Core API users used to be allowed to pass in arbitrary objects as parameters. If these were not used by Cypher, we would silently ignore them. From 3.4.0 this was changed to always fail the query if any parameter was not convertible to a cypher type. This commits reverts to the previous lenient behaviour, as this change caused problems for some users. --- .../org/neo4j/cypher/ExecutionEngineIT.scala | 35 +++++++- .../impl/factory/GraphDatabaseFacade.java | 4 +- .../neo4j/kernel/impl/util/ValueUtils.java | 21 ++++- .../neo4j/shell/kernel/apps/cypher/Start.java | 2 +- .../org/neo4j/values/virtual/ErrorValue.java | 79 +++++++++++++++++++ .../values/virtual/VirtualValueGroup.java | 1 + .../neo4j/values/virtual/VirtualValues.java | 5 ++ .../ParameterValuesAcceptanceTest.scala | 3 +- 8 files changed, 140 insertions(+), 10 deletions(-) create mode 100644 community/values/src/main/java/org/neo4j/values/virtual/ErrorValue.java diff --git a/community/cypher/cypher/src/test/scala/org/neo4j/cypher/ExecutionEngineIT.scala b/community/cypher/cypher/src/test/scala/org/neo4j/cypher/ExecutionEngineIT.scala index 2f095d3b2d524..699333b8b0a4d 100644 --- a/community/cypher/cypher/src/test/scala/org/neo4j/cypher/ExecutionEngineIT.scala +++ b/community/cypher/cypher/src/test/scala/org/neo4j/cypher/ExecutionEngineIT.scala @@ -24,7 +24,7 @@ import org.neo4j.cypher.internal.util.v3_4.test_helpers.CypherFunSuite import org.neo4j.cypher.internal.javacompat.GraphDatabaseCypherService import org.neo4j.cypher.internal.planner.v3_4.spi.CostBasedPlannerName import org.neo4j.graphdb.factory.GraphDatabaseSettings -import org.neo4j.graphdb.{ExecutionPlanDescription, GraphDatabaseService, Result} +import org.neo4j.graphdb.{ExecutionPlanDescription, GraphDatabaseService, QueryExecutionException, Result} import org.neo4j.test.TestGraphDatabaseFactory import scala.collection.immutable.Map @@ -195,6 +195,37 @@ class ExecutionEngineIT extends CypherFunSuite with GraphIcing { db.execute("EXPLAIN MERGE (a:A) ON MATCH SET a.prop = 42 RETURN *").close() } + test("should crash of erroneous parameters values if they are used") { + // given + db = new TestGraphDatabaseFactory().newImpermanentDatabase() + + // when + val params = new java.util.HashMap[String, AnyRef]() + params.put("erroneous", ErroneousParameterValue()) + + val result = db.execute("RETURN $erroneous AS x", params) + + // then + intercept[QueryExecutionException] { + result.columnAs[Int]("x").next() + } + } + + test("should ignore erroneous parameters values if they are not used") { + // given + db = new TestGraphDatabaseFactory().newImpermanentDatabase() + + // when + val params = new java.util.HashMap[String, AnyRef]() + params.put("valid", Integer.valueOf(42)) + params.put("erroneous", ErroneousParameterValue()) + + val result = db.execute("RETURN $valid AS x", params) + + // then + result.columnAs[Int]("x").next() should equal(42) + } + private implicit class RichDb(db: GraphDatabaseCypherService) { def planDescriptionForQuery(query: String): ExecutionPlanDescription = { val res = db.execute(query) @@ -210,4 +241,6 @@ class ExecutionEngineIT extends CypherFunSuite with GraphIcing { def execute(query: String, params: Map[String, Any]): Result = engine.execute(query, params, engine.queryService.transactionalContext(query = query -> params)) } + + private case class ErroneousParameterValue() } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/factory/GraphDatabaseFacade.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/factory/GraphDatabaseFacade.java index f3ee003094f05..ee48d641a0c08 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/factory/GraphDatabaseFacade.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/factory/GraphDatabaseFacade.java @@ -408,7 +408,7 @@ public Result execute( String query, Map parameters ) throws Quer InternalTransaction transaction = beginTransaction( KernelTransaction.Type.implicit, AUTH_DISABLED ); - return execute( transaction, query, ValueUtils.asMapValue( parameters ) ); + return execute( transaction, query, ValueUtils.asParameterMapValue( parameters ) ); } @Override @@ -417,7 +417,7 @@ public Result execute( String query, Map parameters, long timeout { InternalTransaction transaction = beginTransaction( KernelTransaction.Type.implicit, AUTH_DISABLED, timeout, unit ); - return execute( transaction, query, ValueUtils.asMapValue( parameters ) ); + return execute( transaction, query, ValueUtils.asParameterMapValue( parameters ) ); } public Result execute( InternalTransaction transaction, String query, MapValue parameters ) diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/util/ValueUtils.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/util/ValueUtils.java index a202e3daf884f..5bd817e0d1f61 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/util/ValueUtils.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/util/ValueUtils.java @@ -233,18 +233,31 @@ public static ListValue asListOfEdges( Relationship[] rels ) public static MapValue asMapValue( Map map ) { - return map( mapValues( map ) ); + HashMap newMap = new HashMap<>( map.size() ); + for ( Map.Entry entry : map.entrySet() ) + { + newMap.put( entry.getKey(), of( entry.getValue() ) ); + } + + return map( newMap ); } - private static Map mapValues( Map map ) + public static MapValue asParameterMapValue( Map map ) { HashMap newMap = new HashMap<>( map.size() ); for ( Map.Entry entry : map.entrySet() ) { - newMap.put( entry.getKey(), of( entry.getValue() ) ); + try + { + newMap.put( entry.getKey(), of( entry.getValue() ) ); + } + catch ( IllegalArgumentException e ) + { + newMap.put( entry.getKey(), VirtualValues.error( e ) ); + } } - return newMap; + return map( newMap ); } public static NodeValue fromNodeProxy( Node node ) diff --git a/community/shell/src/main/java/org/neo4j/shell/kernel/apps/cypher/Start.java b/community/shell/src/main/java/org/neo4j/shell/kernel/apps/cypher/Start.java index e7d4724826944..a1e8b69566ad5 100644 --- a/community/shell/src/main/java/org/neo4j/shell/kernel/apps/cypher/Start.java +++ b/community/shell/src/main/java/org/neo4j/shell/kernel/apps/cypher/Start.java @@ -205,7 +205,7 @@ private TransactionalContext createTransactionContext( String queryText, Map. + */ +package org.neo4j.values.virtual; + + +import java.util.Comparator; + +import org.neo4j.values.AnyValue; +import org.neo4j.values.AnyValueWriter; +import org.neo4j.values.ValueMapper; +import org.neo4j.values.VirtualValue; +import org.neo4j.values.utils.InvalidValuesArgumentException; + +/** + * The ErrorValue allow delaying errors in value creation until runtime, which is useful + * if it turns out that the value is never used. + */ +public final class ErrorValue extends VirtualValue +{ + private final InvalidValuesArgumentException e; + + ErrorValue( Exception e ) + { + this.e = new InvalidValuesArgumentException( e.getMessage() ); + } + + @Override + public boolean equals( VirtualValue other ) + { + throw e; + } + + @Override + public VirtualValueGroup valueGroup() + { + return VirtualValueGroup.ERROR; + } + + @Override + public int compareTo( VirtualValue other, Comparator comparator ) + { + throw e; + } + + @Override + protected int computeHash() + { + throw e; + } + + @Override + public void writeTo( AnyValueWriter writer ) + { + throw e; + } + + @Override + public T map( ValueMapper mapper ) + { + throw e; + } +} diff --git a/community/values/src/main/java/org/neo4j/values/virtual/VirtualValueGroup.java b/community/values/src/main/java/org/neo4j/values/virtual/VirtualValueGroup.java index 4c3f7e38014ac..5b32c92bd469c 100644 --- a/community/values/src/main/java/org/neo4j/values/virtual/VirtualValueGroup.java +++ b/community/values/src/main/java/org/neo4j/values/virtual/VirtualValueGroup.java @@ -31,4 +31,5 @@ public enum VirtualValueGroup LIST, PATH, NO_VALUE, + ERROR } diff --git a/community/values/src/main/java/org/neo4j/values/virtual/VirtualValues.java b/community/values/src/main/java/org/neo4j/values/virtual/VirtualValues.java index 066e05d605a62..dcab4abf71600 100644 --- a/community/values/src/main/java/org/neo4j/values/virtual/VirtualValues.java +++ b/community/values/src/main/java/org/neo4j/values/virtual/VirtualValues.java @@ -164,6 +164,11 @@ public static MapValue map( Map map ) return new MapValue( map ); } + public static ErrorValue error( Exception e ) + { + return new ErrorValue( e ); + } + @SafeVarargs public static MapValue copy( MapValue map, Pair... moreEntries ) { diff --git a/enterprise/cypher/acceptance-spec-suite/src/test/scala/org/neo4j/internal/cypher/acceptance/ParameterValuesAcceptanceTest.scala b/enterprise/cypher/acceptance-spec-suite/src/test/scala/org/neo4j/internal/cypher/acceptance/ParameterValuesAcceptanceTest.scala index 2d44dceb15ebc..2c6f3bc959840 100644 --- a/enterprise/cypher/acceptance-spec-suite/src/test/scala/org/neo4j/internal/cypher/acceptance/ParameterValuesAcceptanceTest.scala +++ b/enterprise/cypher/acceptance-spec-suite/src/test/scala/org/neo4j/internal/cypher/acceptance/ParameterValuesAcceptanceTest.scala @@ -38,7 +38,7 @@ class ParameterValuesAcceptanceTest extends ExecutionEngineFunSuite with CypherC // Not TCK material below; sending graph types or characters as parameters is not supported - test("ANY should be able to use varibels from the horizon") { + test("ANY should be able to use variables from the horizon") { val query = """ WITH 1 AS node, [] AS nodes1 @@ -168,5 +168,4 @@ class ParameterValuesAcceptanceTest extends ExecutionEngineFunSuite with CypherC val config = Configs.Interpreted - Configs.Cost2_3 executeWith(config, "EXPLAIN CREATE (n:Person) WITH n MATCH (n:Person {name:{name}}) RETURN n") } - }