Fixes neo4j-contrib/neo4j-apoc-procedures#2826: The apoc.import.csv/graphml procedures create empty wrong empty nodes with UNIQUE constraint violation (#3006)

vga91 · vga91 · commit 9660b14a9310 · 2022-09-23T11:31:19.000+02:00
diff --git a/common/src/main/java/apoc/export/util/BatchTransaction.java b/common/src/main/java/apoc/export/util/BatchTransaction.java
@@ -41,6 +41,10 @@ public void manualCommit(boolean log) {
         doCommit(log);
     }
 
+    public void rollback() {
+        tx.rollback();
+    }
+    
     private void doCommit(boolean log) {
         tx.commit();
         tx.close();
@@ -56,7 +60,6 @@ private Transaction beginTx() {
     @Override
     public void close() {
         if (tx!=null) {
-            tx.commit();
             tx.close();
             if (reporter!=null) reporter.progress("finish after " + count + " row(s) ");
         }
diff --git a/core/src/main/java/apoc/export/csv/CsvEntityLoader.java b/core/src/main/java/apoc/export/csv/CsvEntityLoader.java
@@ -78,7 +78,8 @@ public void loadNodes(final Object fileName, final List<String> labels, final Gr
 
             final String[] loadCsvCompatibleHeader = fields.stream().map(f -> f.getName()).toArray(String[]::new);
             int lineNo = 0;
-            try (BatchTransaction btx = new BatchTransaction(db, clc.getBatchSize(), reporter)) {
+            BatchTransaction btx = new BatchTransaction(db, clc.getBatchSize(), reporter);
+            try {
                 for (String[] line : csv.readAll()) {
                     lineNo++;
 
@@ -138,6 +139,12 @@ public void loadNodes(final Object fileName, final List<String> labels, final Gr
                     }
                     reporter.update(1, 0, props++);
                 }
+                btx.commit();
+            } catch (RuntimeException e) {
+                btx.rollback();
+                throw e;
+            } finally {
+                btx.close();
             }
         }
     }
@@ -183,7 +190,8 @@ public void loadRelationships(
             final String[] loadCsvCompatibleHeader = fields.stream().map(f -> f.getName()).toArray(String[]::new);
 
             int lineNo = 0;
-            try (BatchTransaction btx = new BatchTransaction(db, clc.getBatchSize(), reporter)) {
+            BatchTransaction btx = new BatchTransaction(db, clc.getBatchSize(), reporter);
+            try {
                 for (String[] line : csv.readAll()) {
                     lineNo++;
 
@@ -225,6 +233,12 @@ public void loadRelationships(
                     }
                     reporter.update(0, 1, props);
                 }
+                btx.commit();
+            } catch (RuntimeException e) {
+                btx.rollback();
+                throw e;
+            } finally {
+                btx.close();
             }
         }
     }
diff --git a/core/src/main/java/apoc/export/graphml/XmlGraphMLReader.java b/core/src/main/java/apoc/export/graphml/XmlGraphMLReader.java
@@ -210,7 +210,8 @@ public long parseXML(Reader input) throws XMLStreamException {
         Map<String, Key> nodeKeys = new HashMap<>();
         Map<String, Key> relKeys = new HashMap<>();
         int count = 0;
-        try (BatchTransaction tx = new BatchTransaction(db, batchSize * 10, reporter)) {
+        BatchTransaction tx = new BatchTransaction(db, batchSize * 10, reporter);
+        try {
 
             while (reader.hasNext()) {
                 XMLEvent event = (XMLEvent) reader.next();
@@ -288,6 +289,12 @@ public long parseXML(Reader input) throws XMLStreamException {
                     }
                 }
             }
+            tx.commit();
+        } catch (Exception e) {
+            tx.rollback();
+            throw e;
+        } finally { 
+            tx.close();
         }
         return count;
     }
diff --git a/core/src/test/java/apoc/export/csv/ImportCsvTest.java b/core/src/test/java/apoc/export/csv/ImportCsvTest.java
@@ -218,6 +218,28 @@ private void testSkipLine(long skipLine, int nodes) {
         db.executeTransactionally("MATCH (n:SkipLine) DETACH DELETE n");
     }
 
+    @Test
+    public void issue2826WithImportCsv() {
+        db.executeTransactionally("CREATE (n:Person {name: 'John'})");
+        db.executeTransactionally("CREATE CONSTRAINT unique_person ON (n:Person) ASSERT n.name IS UNIQUE");
+        try {
+            TestUtil.testCall(db,
+                    "CALL apoc.import.csv([{fileName: $file, labels: ['Person']}], [], $config)",
+                    map("file", "file:/id.csv", "config", map("delimiter", '|')),
+                    (r) -> fail());
+        } catch (RuntimeException e) {
+            String expected = "Failed to invoke procedure `apoc.import.csv`: " +
+                    "Caused by: IndexEntryConflictException{propertyValues=( String(\"John\") ), addedNodeId=-1, existingNodeId=0}";
+            assertEquals(expected, e.getMessage());
+        }
+
+        // should return only 1 node due to constraint exception
+        TestUtil.testCall(db, "MATCH (n:Person) RETURN properties(n) AS props",
+                r -> assertEquals(Map.of("name", "John"), r.get("props")));
+
+        db.executeTransactionally("DROP CONSTRAINT unique_person");
+    }
+
     @Test
     public void testNodesAndRelsWithMultiTypes() {
         TestUtil.testCall(db,
diff --git a/core/src/test/java/apoc/export/graphml/ExportGraphMLTest.java b/core/src/test/java/apoc/export/graphml/ExportGraphMLTest.java
@@ -51,6 +51,7 @@
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 import static org.junit.Assume.assumeFalse;
 import static org.neo4j.configuration.GraphDatabaseSettings.TransactionStateMemoryAllocation.OFF_HEAP;
 import static org.neo4j.configuration.SettingValueParsers.BYTES;
@@ -342,7 +343,28 @@ public void testImportGraphMLWithEdgeWithoutDataKeys() throws Exception {
 
         TestUtil.testCall(db, "MATCH  ()-[c:RELATED]->() RETURN COUNT(c) AS c", null, (r) -> assertEquals(1L, r.get("c")));
     }
+    
+    @Test
+    public void issue2797WithImportGraphMl() {
+        db.executeTransactionally("CREATE (n:FOO {name: 'foo'})");
+        db.executeTransactionally("CREATE CONSTRAINT unique_foo ON (n:FOO) ASSERT n.name IS UNIQUE");
+        try {
+            TestUtil.testCall(db,
+                    "CALL apoc.import.graphml($file, {readLabels:true})",
+                    map("file", new File(directory, "importNodeEdges.graphml").getAbsolutePath()),
+                    (r) -> fail());
+        } catch (Exception e) {
+            String expected = "Failed to invoke procedure `apoc.import.graphml`: " +
+                    "Caused by: IndexEntryConflictException{propertyValues=( String(\"foo\") ), addedNodeId=-1, existingNodeId=3}";
+            assertEquals(expected, e.getMessage());
+        }
+
+        // should return only 1 node due to constraint exception
+        TestUtil.testCall(db, "MATCH (n:FOO) RETURN properties(n) AS props",
+                r -> assertEquals(Map.of("name", "foo"), r.get("props")));
 
+        db.executeTransactionally("DROP CONSTRAINT unique_foo");
+    }
 
     @Test
     public void testImportGraphMLWithoutCharactersDataKeys() throws Exception {

Original file line number	Diff line number	Diff line change
`@@ -78,7 +78,8 @@ public void loadNodes(final Object fileName, final List<String> labels, final Gr`
`78`	`78`
`79`	`79`	`final String[] loadCsvCompatibleHeader = fields.stream().map(f -> f.getName()).toArray(String[]::new);`
`80`	`80`	`int lineNo = 0;`
`81`		`- try (BatchTransaction btx = new BatchTransaction(db, clc.getBatchSize(), reporter)) {`
	`81`	`+ BatchTransaction btx = new BatchTransaction(db, clc.getBatchSize(), reporter);`
	`82`	`+ try {`
`82`	`83`	`for (String[] line : csv.readAll()) {`
`83`	`84`	`lineNo++;`
`84`	`85`
`@@ -138,6 +139,12 @@ public void loadNodes(final Object fileName, final List<String> labels, final Gr`
`138`	`139`	`}`
`139`	`140`	`reporter.update(1, 0, props++);`
`140`	`141`	`}`
	`142`	`+ btx.commit();`
	`143`	`+ } catch (RuntimeException e) {`
	`144`	`+ btx.rollback();`
	`145`	`+ throw e;`
	`146`	`+ } finally {`
	`147`	`+ btx.close();`
`141`	`148`	`}`
`142`	`149`	`}`
`143`	`150`	`}`
`@@ -183,7 +190,8 @@ public void loadRelationships(`
`183`	`190`	`final String[] loadCsvCompatibleHeader = fields.stream().map(f -> f.getName()).toArray(String[]::new);`
`184`	`191`
`185`	`192`	`int lineNo = 0;`
`186`		`- try (BatchTransaction btx = new BatchTransaction(db, clc.getBatchSize(), reporter)) {`
	`193`	`+ BatchTransaction btx = new BatchTransaction(db, clc.getBatchSize(), reporter);`
	`194`	`+ try {`
`187`	`195`	`for (String[] line : csv.readAll()) {`
`188`	`196`	`lineNo++;`
`189`	`197`
`@@ -225,6 +233,12 @@ public void loadRelationships(`
`225`	`233`	`}`
`226`	`234`	`reporter.update(0, 1, props);`
`227`	`235`	`}`
	`236`	`+ btx.commit();`
	`237`	`+ } catch (RuntimeException e) {`
	`238`	`+ btx.rollback();`
	`239`	`+ throw e;`
	`240`	`+ } finally {`
	`241`	`+ btx.close();`
`228`	`242`	`}`
`229`	`243`	`}`
`230`	`244`	`}`
Original file line number	Diff line number	Diff line change
`@@ -210,7 +210,8 @@ public long parseXML(Reader input) throws XMLStreamException {`
`210`	`210`	`Map<String, Key> nodeKeys = new HashMap<>();`
`211`	`211`	`Map<String, Key> relKeys = new HashMap<>();`
`212`	`212`	`int count = 0;`
`213`		`- try (BatchTransaction tx = new BatchTransaction(db, batchSize * 10, reporter)) {`
	`213`	`+ BatchTransaction tx = new BatchTransaction(db, batchSize * 10, reporter);`
	`214`	`+ try {`
`214`	`215`
`215`	`216`	`while (reader.hasNext()) {`
`216`	`217`	`XMLEvent event = (XMLEvent) reader.next();`
`@@ -288,6 +289,12 @@ public long parseXML(Reader input) throws XMLStreamException {`
`288`	`289`	`}`
`289`	`290`	`}`
`290`	`291`	`}`
	`292`	`+ tx.commit();`
	`293`	`+ } catch (Exception e) {`
	`294`	`+ tx.rollback();`
	`295`	`+ throw e;`
	`296`	`+ } finally {`
	`297`	`+ tx.close();`
`291`	`298`	`}`
`292`	`299`	`return count;`
`293`	`300`	`}`