From d1199d4a23c75990550c71860adb1a674f1c89df Mon Sep 17 00:00:00 2001 From: Michael Simons Date: Sat, 30 Nov 2024 08:40:02 +0100 Subject: [PATCH 1/7] fix: Enhance detection for `CALL IN TRANSACTION`. 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 | 21 +++++++++++++-- .../core/DefaultCypherResourceTest.java | 26 ++++++++++++++++++- .../neo4j/migrations/core/MigrationsIT.java | 9 ++++--- .../V000__Use_call_in_tx3.cypher | 4 +++ .../V000__Use_call_in_tx4.cypher | 9 +++++++ 5 files changed, 62 insertions(+), 7 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..cd400ff2a8 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,9 @@ 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)(?[^{]+)?\\{.*}\\s*+IN(\\s++|(?.+?)?)TRANSACTIONS)(?!`)"); + private static final Pattern PATTERN_CALL_IMPORT_CLAUSE = Pattern.compile("\\(.+\\)"); + 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 -2 CONCURRENT TRANSACTIONS OF 23 ROWS RETURN m", + "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 +365,22 @@ 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" }) void shouldDetectManagedTransactionNeeds(String query) { assertThat(DefaultCypherResource.getTransactionMode(query)).isEqualTo(DefaultCypherResource.TransactionMode.MANAGED); } + + @ParameterizedTest + @ValueSource(strings = { + "MATCH (n) CALL thisisInvalidCypherIKnow {blub} IN TRANSACTIONS RETURN n", + "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 From 97a27d464dd5d4068a2ec7b12c0f44ef40d2fa06 Mon Sep 17 00:00:00 2001 From: Michael Simons Date: Sat, 30 Nov 2024 16:37:00 +0100 Subject: [PATCH 2/7] more greed --- .../ac/simons/neo4j/migrations/core/DefaultCypherResource.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 cd400ff2a8..2abae9c01a 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,7 @@ 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)(?[^{]+)?\\{.*}\\s*+IN(\\s++|(?.+?)?)TRANSACTIONS)(?!`)"); + private static final Pattern CALL_PATTERN = Pattern.compile("(?ims)(?[^{]+)?\\{.*}\\s*+IN(\\s++|(?.+?)?)TRANSACTIONS)(?!`)"); private static final Pattern PATTERN_CALL_IMPORT_CLAUSE = Pattern.compile("\\(.+\\)"); private static final Pattern PATTERN_CALL_CONCURRENCY = Pattern.compile("(?ims)(-\\d+|\\d+)?\\s*CONCURRENT"); From 60b8ced0c87605ffb829e2c75e1a4f3668030403 Mon Sep 17 00:00:00 2001 From: Michael Simons Date: Sat, 30 Nov 2024 16:37:45 +0100 Subject: [PATCH 3/7] more greed --- .../ac/simons/neo4j/migrations/core/DefaultCypherResource.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 2abae9c01a..f9f52bfa3f 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,7 @@ 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)(?[^{]+)?\\{.*}\\s*+IN(\\s++|(?.+?)?)TRANSACTIONS)(?!`)"); + private static final Pattern CALL_PATTERN = Pattern.compile("(?ims)(?[^{]++)?\\{.*}\\s*+IN(\\s++|(?.+?)?)TRANSACTIONS)(?!`)"); private static final Pattern PATTERN_CALL_IMPORT_CLAUSE = Pattern.compile("\\(.+\\)"); private static final Pattern PATTERN_CALL_CONCURRENCY = Pattern.compile("(?ims)(-\\d+|\\d+)?\\s*CONCURRENT"); From 2809bb493a5e64bf1119d8eaf2fc1fa01368a432 Mon Sep 17 00:00:00 2001 From: Michael Simons Date: Sat, 30 Nov 2024 16:53:53 +0100 Subject: [PATCH 4/7] broken --- .../ac/simons/neo4j/migrations/core/DefaultCypherResource.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 f9f52bfa3f..1cf6b689f9 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,7 @@ 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)(?[^{]++)?\\{.*}\\s*+IN(\\s++|(?.+?)?)TRANSACTIONS)(?!`)"); + private static final Pattern CALL_PATTERN = Pattern.compile("(?ims)(?[^{]++)?\\{.*}\\s*+IN(?\\s++|.+?)?TRANSACTIONS)(?!`)"); private static final Pattern PATTERN_CALL_IMPORT_CLAUSE = Pattern.compile("\\(.+\\)"); private static final Pattern PATTERN_CALL_CONCURRENCY = Pattern.compile("(?ims)(-\\d+|\\d+)?\\s*CONCURRENT"); From b2a02e189d3e9b115a7d0631c1631b5a351acf0c Mon Sep 17 00:00:00 2001 From: Michael Simons Date: Sat, 30 Nov 2024 17:30:20 +0100 Subject: [PATCH 5/7] next attempt. --- .../core/DefaultCypherResource.java | 32 +++++++++++-------- .../core/DefaultCypherResourceTest.java | 3 +- 2 files changed, 21 insertions(+), 14 deletions(-) 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 1cf6b689f9..4aa2c643fc 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,7 @@ 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)(?[^{]++)?\\{.*}\\s*+IN(?\\s++|.+?)?TRANSACTIONS)(?!`)"); + private static final Pattern CALL_PATTERN = Pattern.compile("(?ims)(?[^{]++)?\\{.*}\\s*+(?IN(?.+?)TRANSACTIONS)?)(?!`)"); private static final Pattern PATTERN_CALL_IMPORT_CLAUSE = Pattern.compile("\\(.+\\)"); private static final Pattern PATTERN_CALL_CONCURRENCY = Pattern.compile("(?ims)(-\\d+|\\d+)?\\s*CONCURRENT"); @@ -355,20 +355,26 @@ static TransactionMode getTransactionMode(String statement) { } var callMatcher = CALL_PATTERN.matcher(statement); - if (callMatcher.find()) { - var importClause = callMatcher.group("importClause"); - var concurrency = callMatcher.group("concurrency"); - if (nullOrEmpty(importClause, PATTERN_CALL_IMPORT_CLAUSE) && nullOrEmpty(concurrency, PATTERN_CALL_CONCURRENCY)) { - return TransactionMode.IMPLICIT; - } else { - throw new MigrationsException("Invalid statement: " + statement); - } + if (!callMatcher.find()) { + return TransactionMode.MANAGED; } - return TransactionMode.MANAGED; - } - private static boolean nullOrEmpty(String value, Pattern pattern) { - return value == null || pattern.matcher(value.trim()).matches(); + var importClause = callMatcher.group("importClause"); + var transactionClause = callMatcher.group("transactionClause"); + + if (importClause != null && !PATTERN_CALL_IMPORT_CLAUSE.matcher(importClause.trim()).matches()) { + throw new MigrationsException("Invalid statement: " + statement); + } + + if (transactionClause == null) { + return TransactionMode.MANAGED; + } + var concurrency = callMatcher.group("concurrency"); + if (concurrency.isBlank() || PATTERN_CALL_CONCURRENCY.matcher(concurrency.trim()).matches()) { + return TransactionMode.IMPLICIT; + } else { + throw new MigrationsException("Invalid statement: " + statement); + } } /** diff --git a/neo4j-migrations-core/src/test/java/ac/simons/neo4j/migrations/core/DefaultCypherResourceTest.java b/neo4j-migrations-core/src/test/java/ac/simons/neo4j/migrations/core/DefaultCypherResourceTest.java index 79c1190154..630984231c 100644 --- a/neo4j-migrations-core/src/test/java/ac/simons/neo4j/migrations/core/DefaultCypherResourceTest.java +++ b/neo4j-migrations-core/src/test/java/ac/simons/neo4j/migrations/core/DefaultCypherResourceTest.java @@ -342,7 +342,8 @@ void shouldDetectWrongUseStatements() { RETURN m } IN TRANSACTIONS RETURN m """, - "MATCH(n)CALL(n){MATCH (m)-->(n)RETURN m}IN -2 CONCURRENT TRANSACTIONS OF 23 ROWS 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 *" From b8b22e710d9dfaa9f6fd7a081b10fe8510a07a5b Mon Sep 17 00:00:00 2001 From: Michael Simons Date: Sat, 30 Nov 2024 17:57:19 +0100 Subject: [PATCH 6/7] next attempt. --- .../neo4j/migrations/core/DefaultCypherResource.java | 10 ++-------- .../migrations/core/DefaultCypherResourceTest.java | 9 +++++++-- 2 files changed, 9 insertions(+), 10 deletions(-) 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 4aa2c643fc..334219eb9c 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,8 +77,7 @@ 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)(?[^{]++)?\\{.*}\\s*+(?IN(?.+?)TRANSACTIONS)?)(?!`)"); - private static final Pattern PATTERN_CALL_IMPORT_CLAUSE = Pattern.compile("\\(.+\\)"); + 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)(? Date: Sat, 30 Nov 2024 18:22:46 +0100 Subject: [PATCH 7/7] Blerg. --- .../neo4j/migrations/core/DefaultCypherResource.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) 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 334219eb9c..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,13 @@ 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)?)(?!`)"); + /** + * The regex is as greedy as I can get it without parsing the statement fully. And as of now, there is no parser + * available that still supports periodic commit AND concurrent transactions with all options the latter provides + * today in latest Neo4j. + */ + @SuppressWarnings("squid:S5852") + 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)(?