[eqFmcQxi] Fix bug with apoc.export.cypher.* where temp. rel properties were not cleaned up. #632

Lojjs · 2024-05-29T14:47:45Z

Fixes #631

The bug was only affecting users who have set the multipleRelationshipsWithType configuration option to true.

As preparation for adding corresponding methods for relationships.

…es were not cleaned up. The bug was only affecting users who have set the `multipleRelationshipsWithType` configuration option to true.

loveleif · 2024-05-30T08:23:10Z

common/src/main/java/apoc/export/cypher/formatter/CypherFormatterUtils.java

+    /*
+     * Returns true if there exists no other relationship with the same start node, end node and type
+     */
+    public static boolean isUniqueRelationship(Relationship rel) {
+        return StreamSupport.stream(
+                        rel.getStartNode()
+                                .getRelationships(Direction.OUTGOING, rel.getType())
+                                .spliterator(),
+                        false)
+                .noneMatch(r -> !r.equals(rel) && r.getEndNode().equals(rel.getEndNode()));
+    }


Does this really do what it says it does?

I'm thinking about this situation:

create (a), (b) with a,b create (a)-[:R]->(b) with a,b create (a)-[:R]->(b)

Relationship.equals looks like it's implemented based on id:

@Override public boolean equals(Object o) { return o instanceof Relationship && this.getId() == ((Relationship) o).getId(); }

Ahh, never mind, now I see it.

loveleif · 2024-05-30T08:57:23Z

common/src/main/resources/exportAll.cypher

@@ -27,75 +27,75 @@ UNWIND [{_id:160, properties:{born:1967, name:"Julia Roberts", id:"a81fe3a2-68a0
 CREATE (n:`UNIQUE IMPORT LABEL`{`UNIQUE IMPORT ID`: row._id}) SET n += row.properties SET n:Person:Other;
 :commit
 :begin
-UNWIND [{start: {_id:38}, id: 0, end: {_id:0}, properties:{roles:["Neo"], `UNIQUE IMPORT ID REL`:0}}, {start: {_id:39}, id: 1, end: {_id:0}, properties:{roles:["Trinity"], `UNIQUE IMPORT ID REL`:1}}, {start: {_id:40}, id: 2, end: {_id:0}, properties:{roles:["Morpheus"], `UNIQUE IMPORT ID REL`:2}}, {start: {_id:41}, id: 3, end: {_id:0}, properties:{roles:["Agent Smith"], `UNIQUE IMPORT ID REL`:3}}, {start: {_id:45}, id: 4, end: {_id:0}, properties:{roles:["Emil"], `UNIQUE IMPORT ID REL`:7}}, {start: {_id:38}, id: 5, end: {_id:1}, properties:{roles:["Neo"], `UNIQUE IMPORT ID REL`:8}}, {start: {_id:39}, id: 6, end: {_id:1}, properties:{roles:["Trinity"], `UNIQUE IMPORT ID REL`:9}}, {start: {_id:40}, id: 7, end: {_id:1}, properties:{roles:["Morpheus"], `UNIQUE IMPORT ID REL`:10}}, {start: {_id:41}, id: 8, end: {_id:1}, properties:{roles:["Agent Smith"], `UNIQUE IMPORT ID REL`:11}}, {start: {_id:38}, id: 9, end: {_id:2}, properties:{roles:["Neo"], `UNIQUE IMPORT ID REL`:15}}, {start: {_id:39}, id: 10, end: {_id:2}, properties:{roles:["Trinity"], `UNIQUE IMPORT ID REL`:16}}, {start: {_id:40}, id: 11, end: {_id:2}, properties:{roles:["Morpheus"], `UNIQUE IMPORT ID REL`:17}}, {start: {_id:41}, id: 12, end: {_id:2}, properties:{roles:["Agent Smith"], `UNIQUE IMPORT ID REL`:18}}, {start: {_id:38}, id: 13, end: {_id:3}, properties:{roles:["Kevin Lomax"], `UNIQUE IMPORT ID REL`:22}}, {start: {_id:46}, id: 14, end: {_id:3}, properties:{roles:["Mary Ann Lomax"], `UNIQUE IMPORT ID REL`:23}}, {start: {_id:47}, id: 15, end: {_id:3}, properties:{roles:["John Milton"], `UNIQUE IMPORT ID REL`:24}}, {start: {_id:49}, id: 16, end: {_id:4}, properties:{roles:["Lt. Daniel Kaffee"], `UNIQUE IMPORT ID REL`:26}}, {start: {_id:50}, id: 17, end: {_id:4}, properties:{roles:["Col. Nathan R. Jessup"], `UNIQUE IMPORT ID REL`:27}}, {start: {_id:51}, id: 18, end: {_id:4}, properties:{roles:["Lt. Cdr. JoAnne Galloway"], `UNIQUE IMPORT ID REL`:28}}, {start: {_id:52}, id: 19, end: {_id:4}, properties:{roles:["Capt. Jack Ross"], `UNIQUE IMPORT ID REL`:29}}] AS row
+UNWIND [{start: {_id:38}, id: 0, end: {_id:0}, properties:{roles:["Neo"]}}, {start: {_id:39}, id: 1, end: {_id:0}, properties:{roles:["Trinity"]}}, {start: {_id:40}, id: 2, end: {_id:0}, properties:{roles:["Morpheus"]}}, {start: {_id:41}, id: 3, end: {_id:0}, properties:{roles:["Agent Smith"]}}, {start: {_id:45}, id: 4, end: {_id:0}, properties:{roles:["Emil"]}}, {start: {_id:38}, id: 5, end: {_id:1}, properties:{roles:["Neo"]}}, {start: {_id:39}, id: 6, end: {_id:1}, properties:{roles:["Trinity"]}}, {start: {_id:40}, id: 7, end: {_id:1}, properties:{roles:["Morpheus"]}}, {start: {_id:41}, id: 8, end: {_id:1}, properties:{roles:["Agent Smith"]}}, {start: {_id:38}, id: 9, end: {_id:2}, properties:{roles:["Neo"]}}, {start: {_id:39}, id: 10, end: {_id:2}, properties:{roles:["Trinity"]}}, {start: {_id:40}, id: 11, end: {_id:2}, properties:{roles:["Morpheus"]}}, {start: {_id:41}, id: 12, end: {_id:2}, properties:{roles:["Agent Smith"]}}, {start: {_id:38}, id: 13, end: {_id:3}, properties:{roles:["Kevin Lomax"]}}, {start: {_id:46}, id: 14, end: {_id:3}, properties:{roles:["Mary Ann Lomax"]}}, {start: {_id:47}, id: 15, end: {_id:3}, properties:{roles:["John Milton"]}}, {start: {_id:49}, id: 16, end: {_id:4}, properties:{roles:["Lt. Daniel Kaffee"]}}, {start: {_id:50}, id: 17, end: {_id:4}, properties:{roles:["Col. Nathan R. Jessup"]}}, {start: {_id:51}, id: 18, end: {_id:4}, properties:{roles:["Lt. Cdr. JoAnne Galloway"]}}, {start: {_id:52}, id: 19, end: {_id:4}, properties:{roles:["Capt. Jack Ross"]}}] AS row


I did not follow, why did this change?

Is it that the bug was that UNIQUE IMPORT ID REL properties are supposed to be removed?

Yes, I also got confused by this. I think to get the roundtrip to work despite the bug someone (@gem-neo4j ?) had coded this file as if the user already had UNIQUE IMPORT ID REL properties in their data before calling APOC (like if they had added it manually). One could argue that in this unlikely event the code is broken after the bug fix as we throw away their properties. However it is the same for nodes and there is a setting to turn off this behaviour, so I would say it is OK

loveleif · 2024-05-30T08:57:50Z

common/src/main/resources/exportAll.cypher

@@ -27,75 +27,75 @@ UNWIND [{_id:160, properties:{born:1967, name:"Julia Roberts", id:"a81fe3a2-68a0
 CREATE (n:`UNIQUE IMPORT LABEL`{`UNIQUE IMPORT ID`: row._id}) SET n += row.properties SET n:Person:Other;
 :commit
 :begin
-UNWIND [{start: {_id:38}, id: 0, end: {_id:0}, properties:{roles:["Neo"], `UNIQUE IMPORT ID REL`:0}}, {start: {_id:39}, id: 1, end: {_id:0}, properties:{roles:["Trinity"], `UNIQUE IMPORT ID REL`:1}}, {start: {_id:40}, id: 2, end: {_id:0}, properties:{roles:["Morpheus"], `UNIQUE IMPORT ID REL`:2}}, {start: {_id:41}, id: 3, end: {_id:0}, properties:{roles:["Agent Smith"], `UNIQUE IMPORT ID REL`:3}}, {start: {_id:45}, id: 4, end: {_id:0}, properties:{roles:["Emil"], `UNIQUE IMPORT ID REL`:7}}, {start: {_id:38}, id: 5, end: {_id:1}, properties:{roles:["Neo"], `UNIQUE IMPORT ID REL`:8}}, {start: {_id:39}, id: 6, end: {_id:1}, properties:{roles:["Trinity"], `UNIQUE IMPORT ID REL`:9}}, {start: {_id:40}, id: 7, end: {_id:1}, properties:{roles:["Morpheus"], `UNIQUE IMPORT ID REL`:10}}, {start: {_id:41}, id: 8, end: {_id:1}, properties:{roles:["Agent Smith"], `UNIQUE IMPORT ID REL`:11}}, {start: {_id:38}, id: 9, end: {_id:2}, properties:{roles:["Neo"], `UNIQUE IMPORT ID REL`:15}}, {start: {_id:39}, id: 10, end: {_id:2}, properties:{roles:["Trinity"], `UNIQUE IMPORT ID REL`:16}}, {start: {_id:40}, id: 11, end: {_id:2}, properties:{roles:["Morpheus"], `UNIQUE IMPORT ID REL`:17}}, {start: {_id:41}, id: 12, end: {_id:2}, properties:{roles:["Agent Smith"], `UNIQUE IMPORT ID REL`:18}}, {start: {_id:38}, id: 13, end: {_id:3}, properties:{roles:["Kevin Lomax"], `UNIQUE IMPORT ID REL`:22}}, {start: {_id:46}, id: 14, end: {_id:3}, properties:{roles:["Mary Ann Lomax"], `UNIQUE IMPORT ID REL`:23}}, {start: {_id:47}, id: 15, end: {_id:3}, properties:{roles:["John Milton"], `UNIQUE IMPORT ID REL`:24}}, {start: {_id:49}, id: 16, end: {_id:4}, properties:{roles:["Lt. Daniel Kaffee"], `UNIQUE IMPORT ID REL`:26}}, {start: {_id:50}, id: 17, end: {_id:4}, properties:{roles:["Col. Nathan R. Jessup"], `UNIQUE IMPORT ID REL`:27}}, {start: {_id:51}, id: 18, end: {_id:4}, properties:{roles:["Lt. Cdr. JoAnne Galloway"], `UNIQUE IMPORT ID REL`:28}}, {start: {_id:52}, id: 19, end: {_id:4}, properties:{roles:["Capt. Jack Ross"], `UNIQUE IMPORT ID REL`:29}}] AS row
+UNWIND [{start: {_id:38}, id: 0, end: {_id:0}, properties:{roles:["Neo"]}}, {start: {_id:39}, id: 1, end: {_id:0}, properties:{roles:["Trinity"]}}, {start: {_id:40}, id: 2, end: {_id:0}, properties:{roles:["Morpheus"]}}, {start: {_id:41}, id: 3, end: {_id:0}, properties:{roles:["Agent Smith"]}}, {start: {_id:45}, id: 4, end: {_id:0}, properties:{roles:["Emil"]}}, {start: {_id:38}, id: 5, end: {_id:1}, properties:{roles:["Neo"]}}, {start: {_id:39}, id: 6, end: {_id:1}, properties:{roles:["Trinity"]}}, {start: {_id:40}, id: 7, end: {_id:1}, properties:{roles:["Morpheus"]}}, {start: {_id:41}, id: 8, end: {_id:1}, properties:{roles:["Agent Smith"]}}, {start: {_id:38}, id: 9, end: {_id:2}, properties:{roles:["Neo"]}}, {start: {_id:39}, id: 10, end: {_id:2}, properties:{roles:["Trinity"]}}, {start: {_id:40}, id: 11, end: {_id:2}, properties:{roles:["Morpheus"]}}, {start: {_id:41}, id: 12, end: {_id:2}, properties:{roles:["Agent Smith"]}}, {start: {_id:38}, id: 13, end: {_id:3}, properties:{roles:["Kevin Lomax"]}}, {start: {_id:46}, id: 14, end: {_id:3}, properties:{roles:["Mary Ann Lomax"]}}, {start: {_id:47}, id: 15, end: {_id:3}, properties:{roles:["John Milton"]}}, {start: {_id:49}, id: 16, end: {_id:4}, properties:{roles:["Lt. Daniel Kaffee"]}}, {start: {_id:50}, id: 17, end: {_id:4}, properties:{roles:["Col. Nathan R. Jessup"]}}, {start: {_id:51}, id: 18, end: {_id:4}, properties:{roles:["Lt. Cdr. JoAnne Galloway"]}}, {start: {_id:52}, id: 19, end: {_id:4}, properties:{roles:["Capt. Jack Ross"]}}] AS row


Not related to PR, but shouldn't this file be in test/resources?

Yes, probably. I can try to move it

It looks like it is potentially used from extended so I don't think it is worth the risk to move it

core/src/main/java/apoc/export/cypher/MultiStatementCypherSubGraphExporter.java

loveleif · 2024-05-30T09:25:10Z

core/src/main/java/apoc/export/cypher/MultiStatementCypherSubGraphExporter.java

+        return artificialUniques;
+    }
+
+    private long countArtificialUniqueRels(Relationship rel) {


This method can also be a single return.

loveleif · 2024-05-30T09:27:36Z

core/src/main/java/apoc/export/cypher/MultiStatementCypherSubGraphExporter.java

+        if (!CypherFormatterUtils.isUniqueRelationship(rel)) {
+            artificialUniques++;
+        }


If it's not unique we increase number of uniques?

Yes this is what I was confused about in our Slack channel here: https://neo4j.slack.com/archives/C032XMUBAHG/p1716906509842369. The Cypher query we build up will add the UNIQUE IMPORT ID REL to all relationships which are not already unique, and we want to calculate how many that will be (i.e. how many we make unique artificially), thus the inversion. This info is then used to know how many batches of deletion we need. Does that make sense? I can add a code comment trying to explain this as we both got confused.

Still a bit confusing, but if you feel confident I'm happy :).

core/src/main/java/apoc/export/cypher/MultiStatementCypherSubGraphExporter.java

…raphExporter.java Co-authored-by: Love Kristofer Leifland <love.leifland@neotechnology.com>

loveleif

Great work!

[eqFmcQxi] Rename artificial uniques to mention nodes.

7e42076

As preparation for adding corresponding methods for relationships.

Lojjs added bug Something isn't working team-cypher-surface dev labels May 29, 2024

Lojjs force-pushed the dev-bug-cleanup-temp-properties branch from 6124736 to 49c7d4d Compare May 29, 2024 14:53

[eqFmcQxi] Fix bug with apoc.export.cypher.* where temp. rel properti…

e6897f7

…es were not cleaned up. The bug was only affecting users who have set the `multipleRelationshipsWithType` configuration option to true.

Lojjs force-pushed the dev-bug-cleanup-temp-properties branch from 49c7d4d to e6897f7 Compare May 30, 2024 06:49

Lojjs assigned loveleif May 30, 2024

loveleif reviewed May 30, 2024

View reviewed changes

Update core/src/main/java/apoc/export/cypher/MultiStatementCypherSubG…

31aaf84

…raphExporter.java Co-authored-by: Love Kristofer Leifland <love.leifland@neotechnology.com>

Lojjs force-pushed the dev-bug-cleanup-temp-properties branch from 9496e29 to 31aaf84 Compare May 31, 2024 07:26

[eqFmcQxi] Fix review comments

64e71e9

loveleif approved these changes May 31, 2024

View reviewed changes

Lojjs merged commit a31c119 into dev May 31, 2024
19 of 20 checks passed

Lojjs deleted the dev-bug-cleanup-temp-properties branch May 31, 2024 09:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[eqFmcQxi] Fix bug with apoc.export.cypher.* where temp. rel properties were not cleaned up. #632

[eqFmcQxi] Fix bug with apoc.export.cypher.* where temp. rel properties were not cleaned up. #632

Lojjs commented May 29, 2024

loveleif May 30, 2024

loveleif May 30, 2024

loveleif May 30, 2024

loveleif May 30, 2024

Lojjs May 31, 2024

loveleif May 30, 2024

Lojjs May 31, 2024

Lojjs May 31, 2024

loveleif May 30, 2024

loveleif May 30, 2024

Lojjs May 31, 2024

loveleif May 31, 2024

loveleif left a comment

[eqFmcQxi] Fix bug with apoc.export.cypher.* where temp. rel properties were not cleaned up. #632

[eqFmcQxi] Fix bug with apoc.export.cypher.* where temp. rel properties were not cleaned up. #632

Conversation

Lojjs commented May 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loveleif left a comment

Choose a reason for hiding this comment