From 4e5ce067015bf1cb73045fe1ab17d112beba0f0d Mon Sep 17 00:00:00 2001 From: Rahul Krishna Date: Wed, 2 Sep 2026 10:12:07 -0400 Subject: [PATCH] fix(neo4j): anchor the Bolt purge on java-owned labels The incremental push purged a changed module with two completely unlabelled statements: MATCH (x {_module: $m})-[r]->() DELETE r MATCH (x {_module: $m}) WHERE NOT coalesce(x.id, x.file_key) IN $keys DETACH DELETE x `_module` is a shared convention, not a java-private property. The sibling analyzers set it on their own nodes with the same value, the module's file key, so where a file key collided across languages in a shared database a java push deleted their nodes and relationships -- a graph this tool did not write and cannot see. Silently, unless the delete happened to be large enough to exhaust the transaction limit and roll back, which is how the same bug surfaced in codeanalyzer-typescript. CypherWriter already reasons about exactly this hazard for the snapshot path, at length, in DESCENDANTS' javadoc. That reasoning had never been applied one property over. Both statements are now anchored on CypherWriter.MODULE_OWNED, mirroring what the sibling analyzers do. The pattern is derived from the catalogs rather than hand-listed, so a label that gains `_module` later is covered without a second list to maintain, and it spans BOTH generations because BoltWriter is schema-agnostic and may be pushing either: it resolves to 15 labels, 7 of them v1-only, which a v2-only anchor would have silently stopped purging. The statements moved to constants so the anchoring is assertable without a live database. BoltWriterPurgeTest pins that neither matches an unlabelled node, that the anchor covers every `_module`-carrying label in both catalogs, and that it excludes the cross-language :Artifact/:Package/:ConfigKey and the shared :JPackage/:JAnnotation/:JExternal. Also audited the orphan prune the issue flagged and left it unchanged: it enters at :JApplication scoped by app name, hops a java-owned relationship, and expands only through DESCENDANTS, all of whose members are J_-prefixed containment. No sibling or shared node is reachable. Recorded in a comment so the check is not repeated. Closes #213 --- .../java/com/ibm/cldk/neo4j/BoltWriter.java | 37 +++++++- .../java/com/ibm/cldk/neo4j/CypherWriter.java | 33 +++++++ .../ibm/cldk/neo4j/BoltWriterPurgeTest.java | 87 +++++++++++++++++++ 3 files changed, 154 insertions(+), 3 deletions(-) create mode 100644 src/test/java/com/ibm/cldk/neo4j/BoltWriterPurgeTest.java diff --git a/src/main/java/com/ibm/cldk/neo4j/BoltWriter.java b/src/main/java/com/ibm/cldk/neo4j/BoltWriter.java index 6ea4c900..e5b41dad 100644 --- a/src/main/java/com/ibm/cldk/neo4j/BoltWriter.java +++ b/src/main/java/com/ibm/cldk/neo4j/BoltWriter.java @@ -48,6 +48,29 @@ * are shared (no {@code _module}) and are MERGE-only. */ public final class BoltWriter implements BoltSink { + /** + * Purge the edges a changed module owns, and the declarations that vanished from it. + * + *

Both are anchored on {@link CypherWriter#MODULE_OWNED}, and that anchor is the whole point: + * {@code _module} is a shared convention rather than a java-private property, so the sibling + * analyzers set it on their own nodes with the same value (the module's file key). The + * unlabelled {@code MATCH (x {_module: $m})} these replaced therefore matched a sibling's nodes + * wherever a file key collided across languages in a shared database, and deleted a graph this + * tool did not write and cannot see -- silently, unless the delete happened to be large enough + * to exhaust the transaction limit and roll back (#213). + * + *

Constants rather than inline strings so the anchoring is assertable without a live + * database; {@code BoltWriterPurgeTest} pins it. + */ + static final String PURGE_MODULE_EDGES = + "MATCH (x:" + CypherWriter.MODULE_OWNED + ") WHERE x._module = $m " + + "MATCH (x)-[r]->() DELETE r"; + + /** @see #PURGE_MODULE_EDGES */ + static final String PURGE_VANISHED_NODES = + "MATCH (x:" + CypherWriter.MODULE_OWNED + ") WHERE x._module = $m " + + "AND NOT coalesce(x.id, x.file_key) IN $keys DETACH DELETE x"; + private static final int BATCH = 1000; @@ -132,9 +155,8 @@ void run(GraphRows rows, boolean fullRun) { } try (Session s = session()) { s.writeTransaction(tx -> { - tx.run("MATCH (x {_module: $m})-[r]->() DELETE r", Values.parameters("m", unit)); - tx.run("MATCH (x {_module: $m}) WHERE NOT coalesce(x.id, x.file_key) IN $keys DETACH DELETE x", - Values.parameters("m", unit, "keys", keys)); + tx.run(PURGE_MODULE_EDGES, Values.parameters("m", unit)); + tx.run(PURGE_VANISHED_NODES, Values.parameters("m", unit, "keys", keys)); return null; }); } @@ -157,6 +179,15 @@ void run(GraphRows rows, boolean fullRun) { if (fullRun) { List present = new ArrayList<>(byUnit.keySet()); String app = appNameOf(rows); + // Checked against the same hazard as the _module purges above (#213) and found + // sound, so deliberately left alone: this traversal never leaves java's own graph. + // It enters at :JApplication (java-owned, and scoped to this app by name), hops a + // java-owned relationship type, and expands only through DESCENDANTS -- every + // member of which is J_-prefixed containment. `c` and `x` are unlabelled but + // unreachable except along those edges, and DESCENDANTS deliberately excludes + // HAS_ARTIFACT, so no cross-language :Artifact/:ConfigKey and no shared + // :JPackage/:JAnnotation can be reached. Do not add a non-containment type to + // DESCENDANTS without re-checking that. try (Session s = session()) { long pruned = s.run("MATCH (:JApplication {name: $app})-[:J_HAS_UNIT|J_HAS_MODULE]->(c) " + "WHERE NOT c.file_key IN $present " diff --git a/src/main/java/com/ibm/cldk/neo4j/CypherWriter.java b/src/main/java/com/ibm/cldk/neo4j/CypherWriter.java index bc34479b..2ace072e 100644 --- a/src/main/java/com/ibm/cldk/neo4j/CypherWriter.java +++ b/src/main/java/com/ibm/cldk/neo4j/CypherWriter.java @@ -17,6 +17,7 @@ import java.io.IOException; import java.io.StringWriter; import java.util.ArrayList; +import java.util.Arrays; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -55,6 +56,38 @@ public final class CypherWriter { + "|J_HAS_CRUD_OPERATION|J_HAS_CRUD_QUERY|J_HAS_COMMENT" + "|J_DECLARES|J_HAS_METHOD|J_HAS_BODY_NODE*1..]"; + /** + * The labels this analyzer owns per module -- every label in either generation's catalog that + * carries the internal {@code _module} provenance property, as a Cypher label disjunction + * ({@code JModule|JType|JCallable|...}). + * + *

{@code _module} is NOT java-private. The sibling analyzers set the same property on their + * own nodes, using the same value (the module's file key), so an unlabelled + * {@code MATCH (x {_module: $m})} matches THEIR nodes wherever a file key collides across + * languages in a shared database -- and a delete built on it destroys a graph this tool did not + * write and cannot see. Every statement that matches on {@code _module} must be anchored here. + * The hazard is the same one {@link #DESCENDANTS} is written to avoid, one property over. + * + *

Derived from the catalogs rather than hand-listed, so a label that gains {@code _module} + * later is covered without a second list to maintain, and spans BOTH generations for the same + * reason {@code DESCENDANTS} does: {@link BoltWriter} is schema-agnostic and may be pushing + * either one. + */ + static final String MODULE_OWNED = moduleOwnedPattern(); + + private static String moduleOwnedPattern() { + List labels = new ArrayList<>(); + for (List catalog : + Arrays.asList(SchemaCatalog.NODE_LABELS, V2SchemaCatalog.NODE_LABELS)) { + for (SchemaCatalog.NodeLabel nl : catalog) { + if (nl.properties.containsKey("_module") && !labels.contains(nl.label)) { + labels.add(nl.label); + } + } + } + return String.join("|", labels); + } + private CypherWriter() {} /** diff --git a/src/test/java/com/ibm/cldk/neo4j/BoltWriterPurgeTest.java b/src/test/java/com/ibm/cldk/neo4j/BoltWriterPurgeTest.java new file mode 100644 index 00000000..a38939ab --- /dev/null +++ b/src/test/java/com/ibm/cldk/neo4j/BoltWriterPurgeTest.java @@ -0,0 +1,87 @@ +package com.ibm.cldk.neo4j; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import org.junit.jupiter.api.Test; + +/** + * The incremental push deletes, and a delete anchored on nothing deletes everything that matches. + * + *

{@code _module} is a shared convention, not a java-private property: the sibling analyzers set + * it on their own nodes with the same value (the module's file key). An unlabelled + * {@code MATCH (x {_module: $m})} therefore reaches THEIR nodes wherever a file key collides in a + * shared database (#213). These tests pin the anchor that stops it. + * + *

They assert the statement text rather than round-tripping through Neo4j deliberately: the + * live-database path needs Docker and does not run everywhere, and the property that makes the fix + * correct -- which labels the match is confined to -- is fully determined by the catalogs. + */ +class BoltWriterPurgeTest { + + private static List moduleOwnedLabels() { + List labels = new ArrayList<>(); + for (List catalog : + Arrays.asList(SchemaCatalog.NODE_LABELS, V2SchemaCatalog.NODE_LABELS)) { + for (SchemaCatalog.NodeLabel nl : catalog) { + if (nl.properties.containsKey("_module") && !labels.contains(nl.label)) { + labels.add(nl.label); + } + } + } + return labels; + } + + @Test + void neitherPurgeStatementMatchesAnUnlabelledNode() { + for (String stmt : new String[] {BoltWriter.PURGE_MODULE_EDGES, BoltWriter.PURGE_VANISHED_NODES}) { + assertFalse(stmt.contains("(x {_module"), + "an unlabelled _module match reaches a sibling analyzer's nodes: " + stmt); + assertTrue(stmt.contains("(x:" + CypherWriter.MODULE_OWNED + ")"), + "every _module match must be anchored on java-owned labels: " + stmt); + } + } + + @Test + void theAnchorCoversEveryLabelThatCarriesModule() { + // A label that carries _module but is missing from the anchor would stop being purged -- + // stale nodes surviving a push, the opposite failure from deleting a sibling's. + for (String label : moduleOwnedLabels()) { + assertTrue(Arrays.asList(CypherWriter.MODULE_OWNED.split("\\|")).contains(label), + label + " carries _module but is not in the purge anchor"); + } + } + + @Test + void theAnchorSpansBothSchemaGenerations() { + // BoltWriter is schema-agnostic and may be pushing either generation, so a v2-only anchor + // would silently stop purging a v1 graph. + List anchor = Arrays.asList(CypherWriter.MODULE_OWNED.split("\\|")); + assertTrue(anchor.contains("JCompilationUnit"), "v1's module-owned label must be covered"); + assertTrue(anchor.contains("JModule"), "v2's module-owned label must be covered"); + } + + @Test + void theAnchorExcludesCrossLanguageAndSharedNodes() { + // Artifact/Package/ConfigKey are deliberately un-prefixed cross-language merge targets, and + // JPackage/JAnnotation/JExternal are shared across modules. None carries _module, so none + // may appear in an anchor used for deletion -- see CypherWriter.DESCENDANTS' javadoc. + List anchor = Arrays.asList(CypherWriter.MODULE_OWNED.split("\\|")); + for (String shared : new String[] {"Artifact", "Package", "ConfigKey", "JPackage", + "JAnnotation", "JExternal", "JApplication"}) { + assertFalse(anchor.contains(shared), shared + " must never be reachable by a purge"); + } + } + + @Test + void theAnchorIsNotEmpty() { + // A pattern that degraded to "" would render `MATCH (x:)`, which is a syntax error rather + // than a silent over-delete -- but assert it anyway, since the value is derived. + assertFalse(CypherWriter.MODULE_OWNED.isEmpty()); + assertFalse(CypherWriter.MODULE_OWNED.startsWith("|")); + assertFalse(CypherWriter.MODULE_OWNED.endsWith("|")); + } +}