From cd0f32348be0ec9f50183f5d2ae4c5fc892fda8d Mon Sep 17 00:00:00 2001 From: Michael Simons Date: Sat, 30 Nov 2024 18:42:23 +0100 Subject: [PATCH] fix: Enhance detection for `CALL IN TRANSACTION`. (#1540) Detects now also the `CONCURRENT` keyword. Fixes #1537, but I really think mixing up a script runner and its transactional flow with concurrent database managed transaction can be quite a source of confusion. --- .../core/DefaultCypherResource.java | 29 +++++++++++++++-- .../core/DefaultCypherResourceTest.java | 32 ++++++++++++++++++- .../neo4j/migrations/core/MigrationsIT.java | 9 +++--- .../V000__Use_call_in_tx3.cypher | 4 +++ .../V000__Use_call_in_tx4.cypher | 9 ++++++ 5 files changed, 75 insertions(+), 8 deletions(-) create mode 100644 neo4j-migrations-core/src/test/resources/manual_resources/V000__Use_call_in_tx3.cypher create mode 100644 neo4j-migrations-core/src/test/resources/manual_resources/V000__Use_call_in_tx4.cypher diff --git a/neo4j-migrations-core/src/main/java/ac/simons/neo4j/migrations/core/DefaultCypherResource.java b/neo4j-migrations-core/src/main/java/ac/simons/neo4j/migrations/core/DefaultCypherResource.java index 04acac7bba..4169660e23 100644 --- a/neo4j-migrations-core/src/main/java/ac/simons/neo4j/migrations/core/DefaultCypherResource.java +++ b/neo4j-migrations-core/src/main/java/ac/simons/neo4j/migrations/core/DefaultCypherResource.java @@ -77,7 +77,14 @@ final class DefaultCypherResource implements CypherResource { private static final Pattern USE_DATABASE_PATTERN = Pattern.compile(USE_DATABASE_EXPRESSION); - private static final Pattern CALL_PATTERN = Pattern.compile("(?ims)(?IN(?.+?)TRANSACTIONS)?)(?!`)"); + private static final Pattern PATTERN_CALL_CONCURRENCY = Pattern.compile("(?ims)(-\\d+|\\d+)?\\s*CONCURRENT"); private static final Pattern USING_PERIODIC_PATTERN = Pattern.compile("(?ims)(?(n)RETURN m}IN TRANSACTIONS RETURN m", """ + MATCH (n) \ + CALL(n) { + MATCH (m)-->(n) + RETURN m + } IN TRANSACTIONS RETURN m + """, + "MATCH (n) CALL(n) {MATCH (m)-->(n)RETURN m} IN TRANSACTIONS RETURN y", + "MATCH(n)CALL(n){MATCH (m)-->(n)RETURN m}IN -2 CONCURRENT TRANSACTIONS OF 23 ROWS RETURN x", + "MATCH(n)CALL(n){MATCH (m)-->(n)RETURN m}IN-1 CONCURRENT TRANSACTIONS OF 23 ROWS RETURN m", + "MATCH(n)CALL(n){MATCH (m)-->(n)RETURN m}IN 42 CONCURRENT TRANSACTIONS OF 23 ROWS RETURN m", + "MATCH (n) CALL {RETURN 1 AS x} IN concurrent TRANSACTIONS RETURN *" }) void shouldDetectImplicitTransactionNeeds(String query) { @@ -353,10 +366,27 @@ void shouldDetectImplicitTransactionNeeds(String query) { "CREATE (a:`USING PERIODIC COMMIT `) RETURN a", "CREATE (a:`USING PERIODIC COMMIT `) RETURN a", "CREATE (a:`CALL {WITH WHATEVER} IN TRANSACTIONS`) RETURN a", - "CREATE (a: ` CALL {WITH WHATEVER} IN TRANSACTIONS`) RETURN a" + "CREATE (a: ` CALL {WITH WHATEVER} IN TRANSACTIONS`) RETURN a", + "MATCH (n) CALL {blub}concurrent IN TRANSACTIONS RETURN n", + """ + WITH apoc.date.currentTimestamp() AS start + CALL apoc.util.sleep(4000) + WITH start, apoc.date.currentTimestamp() AS end + RETURN datetime({epochmillis: start}) AS start, datetime({epochmillis: end}) AS end; + """ }) void shouldDetectManagedTransactionNeeds(String query) { assertThat(DefaultCypherResource.getTransactionMode(query)).isEqualTo(DefaultCypherResource.TransactionMode.MANAGED); } + + @ParameterizedTest + @ValueSource(strings = { + "MATCH (n) CALL {blub} IN concurrently TRANSACTIONS RETURN n" + }) + void weEvenMightThrowWhenWeDoHalfwayParsingAnyway(String query) { + + assertThatExceptionOfType(MigrationsException.class) + .isThrownBy(() -> DefaultCypherResource.getTransactionMode(query)); + } } diff --git a/neo4j-migrations-core/src/test/java/ac/simons/neo4j/migrations/core/MigrationsIT.java b/neo4j-migrations-core/src/test/java/ac/simons/neo4j/migrations/core/MigrationsIT.java index dc0b7b5de6..53975e5a36 100644 --- a/neo4j-migrations-core/src/test/java/ac/simons/neo4j/migrations/core/MigrationsIT.java +++ b/neo4j-migrations-core/src/test/java/ac/simons/neo4j/migrations/core/MigrationsIT.java @@ -804,17 +804,18 @@ void shouldApplyResourceBasedMigrations(MigrationsConfig.TransactionMode transac } } - @ParameterizedTest // GH-719 - @ValueSource(strings = {"V000__Use_call_in_tx1", "V000__Use_call_in_tx2"}) - void shouldAllowCallInTX(String script) { + @ParameterizedTest // GH-719, GH-1537 + @ValueSource(ints = {1, 2, 3, 4}) + void shouldAllowCallInTX(int idx) { try (Session session = driver.session()) { session.run("with range(1,100) as r unwind r as i create (n:F) return n").consume(); + session.run("CREATE (a:Asset)-[:ASSET_TYPE]->(type:AssetType)"); } Migrations migrations = new Migrations(MigrationsConfig.defaultConfig(), driver); int appliedMigrations = migrations.apply( - Objects.requireNonNull(MigrationsIT.class.getResource("/manual_resources/" + script + ".cypher")) + Objects.requireNonNull(MigrationsIT.class.getResource("/manual_resources/V000__Use_call_in_tx" + idx + ".cypher")) ); assertThat(appliedMigrations).isEqualTo(1); diff --git a/neo4j-migrations-core/src/test/resources/manual_resources/V000__Use_call_in_tx3.cypher b/neo4j-migrations-core/src/test/resources/manual_resources/V000__Use_call_in_tx3.cypher new file mode 100644 index 0000000000..56e228e042 --- /dev/null +++ b/neo4j-migrations-core/src/test/resources/manual_resources/V000__Use_call_in_tx3.cypher @@ -0,0 +1,4 @@ +MATCH (type:AssetType) MATCH (a:Asset)-[:ASSET_TYPE]->(type) +CALL (a, type) { +CALL apoc.create.setLabels(a, [ type.naam ] ) yield node +} IN 2 CONCURRENT TRANSACTIONS OF 1000 ROWS; \ No newline at end of file diff --git a/neo4j-migrations-core/src/test/resources/manual_resources/V000__Use_call_in_tx4.cypher b/neo4j-migrations-core/src/test/resources/manual_resources/V000__Use_call_in_tx4.cypher new file mode 100644 index 0000000000..4bc544d521 --- /dev/null +++ b/neo4j-migrations-core/src/test/resources/manual_resources/V000__Use_call_in_tx4.cypher @@ -0,0 +1,9 @@ +LOAD CSV WITH HEADERS FROM 'https://data.neo4j.com/importing-cypher/movies.csv' AS row +CALL (row) { + MERGE (m:Movie {movieId: row.movieId}) + MERGE (y:Year {year: row.year}) + MERGE (m)-[r:RELEASED_IN]->(y) +} IN 2 CONCURRENT TRANSACTIONS OF 10 ROWS ON ERROR CONTINUE REPORT STATUS as status +WITH status +WHERE status.errorMessage IS NOT NULL +RETURN status.transactionId AS transaction, status.committed AS commitStatus, status.errorMessage AS errorMessage