From 2e1b53ebf30fce5ed4ffde39b2599fcb7fb5bbbe Mon Sep 17 00:00:00 2001 From: Rahul Krishna Date: Wed, 2 Sep 2026 12:11:50 -0400 Subject: [PATCH 1/2] fix(neo4j)!: scope the v2 purge on the can:// id prefix; retire _module from v2 The incremental purge scoped deletion by `_module`, a bare project-relative file key stamped onto every node. That key carries no language and no application, so after #213 anchored it on java-owned labels it was still application-blind: two java applications in one database sharing src/main/java/Foo.java have identical labels and an identical file key, and only identity distinguishes them. The v2 purge now scopes on the module's own can:// id -- the module by equality, its declarations by `id STARTS WITH + '/'`. The id is a path, so a prefix match is containment, and it is simultaneously scoped to one language, one application and one module. Cross-language and shared nodes fall outside by construction: artifacts are can://artifact/..., packages are pkg:..., and name-keyed nodes have no id to match. `_module` retires from the v2 contract. It survives as NodeRow.moduleKey, an in-memory field the writer uses to group nodes -- the graph never needed telling, since a node's owning module is already its id prefix. RowBuilder lifts it off the emitted properties in one place, so no projector call site changes. v1 keeps exactly what it had. Its ids are FQN-shaped (com.l4.Chain#a(int)@5:16) with no application segment, so there is no prefix to scope by; that path keeps `_module` and the #213 label anchor, and BoltWriter branches on whether the module's id is a can:// id. MODULE_OWNED is therefore now the v1 vocabulary alone, which its test asserts rather than assumes. Nodes keyed on a can:// id gain a :JCanNode marker, declared as a marker label and indexed. It exists only so the prefix predicate can seek: Neo4j property indexes are label-scoped, so the match would otherwise scan the whole node store once per changed module. It makes no semantic claim, unlike :JSymbol, and being java-namespaced means an anchoring mistake still cannot reach another language. The descendant separator is a named function rather than an inlined `+ "/"`, because dropping it is silent: `can://java/app/src/Foo.java` also prefixes `can://java/app/src/Foo.javaX`. Graph contract 2.2.0 -> 3.0.0: a property is removed. analysis.json is untouched and byte-identical on daytrader8; `_module` was always projection-only. Part of codellm-devkit/.github#50. Closes #219. Closes #218. Closes #217. --- schema.neo4j.json | 22 ++---- .../java/com/ibm/cldk/neo4j/BoltWriter.java | 78 +++++++++++++++++-- .../java/com/ibm/cldk/neo4j/GraphRows.java | 17 ++++ .../java/com/ibm/cldk/neo4j/RowBuilder.java | 36 ++++++++- src/main/java/com/ibm/cldk/neo4j/Schema.java | 7 +- .../com/ibm/cldk/neo4j/V2SchemaCatalog.java | 29 ++++--- .../com/ibm/cldk/CodeAnalyzerV2CliTest.java | 4 +- .../ibm/cldk/neo4j/BoltWriterPurgeTest.java | 33 +++++++- 8 files changed, 188 insertions(+), 38 deletions(-) diff --git a/schema.neo4j.json b/schema.neo4j.json index f6a5f630..6148f9d3 100644 --- a/schema.neo4j.json +++ b/schema.neo4j.json @@ -1,8 +1,9 @@ { - "schema_version": "2.2.0", + "schema_version": "3.0.0", "generator": "codeanalyzer-java", "marker_labels": [ - "JEntrypoint" + "JEntrypoint", + "JCanNode" ], "node_labels": [ { @@ -24,8 +25,7 @@ "id": "string", "file_key": "string", "package": "string", - "content_hash": "string", - "_module": "string" + "content_hash": "string" } }, { @@ -41,7 +41,6 @@ "interfaces": "string[]", "docstring": "string", "is_entrypoint": "boolean", - "_module": "string", "start_line": "integer", "end_line": "integer" } @@ -67,7 +66,6 @@ "accessed_fields": "string[]", "is_implicit": "boolean", "is_entrypoint": "boolean", - "_module": "string", "start_line": "integer", "end_line": "integer" } @@ -94,7 +92,6 @@ "initializer": "string", "modifiers": "string[]", "docstring": "string", - "_module": "string", "start_line": "integer", "end_line": "integer" } @@ -108,7 +105,6 @@ "name": "string", "type": "string", "initializer": "string", - "_module": "string", "start_line": "integer", "end_line": "integer" } @@ -121,8 +117,7 @@ "id": "string", "name": "string", "arguments": "string[]", - "docstring": "string", - "_module": "string" + "docstring": "string" } }, { @@ -135,8 +130,7 @@ "type": "string", "modifiers": "string[]", "is_variadic": "boolean", - "docstring": "string", - "_module": "string" + "docstring": "string" } }, { @@ -155,7 +149,6 @@ "is_static_call": "boolean", "argument_types": "string[]", "argument_expr": "string[]", - "_module": "string", "var": "string", "call_node": "string", "start_line": "integer", @@ -518,6 +511,7 @@ "CREATE INDEX j_callable_name IF NOT EXISTS FOR (c:JCallable) ON (c.name)", "CREATE INDEX j_type_name IF NOT EXISTS FOR (t:JType) ON (t.name)", "CREATE INDEX j_annotation_name_idx IF NOT EXISTS FOR (an:JAnnotation) ON (an.name)", - "CREATE FULLTEXT INDEX j_code_fts IF NOT EXISTS FOR (c:JCallable) ON EACH [c.code, c.docstring]" + "CREATE FULLTEXT INDEX j_code_fts IF NOT EXISTS FOR (c:JCallable) ON EACH [c.code, c.docstring]", + "CREATE INDEX j_can_node_id IF NOT EXISTS FOR (n:JCanNode) ON (n.id)" ] } diff --git a/src/main/java/com/ibm/cldk/neo4j/BoltWriter.java b/src/main/java/com/ibm/cldk/neo4j/BoltWriter.java index e5b41dad..4c6dab84 100644 --- a/src/main/java/com/ibm/cldk/neo4j/BoltWriter.java +++ b/src/main/java/com/ibm/cldk/neo4j/BoltWriter.java @@ -66,6 +66,56 @@ public final class BoltWriter implements BoltSink { "MATCH (x:" + CypherWriter.MODULE_OWNED + ") WHERE x._module = $m " + "MATCH (x)-[r]->() DELETE r"; + /** + * The v2 purge: scoped by the module's own {@code can://} id rather than by a bare file key. + * + *

The id is a path — {@code can://java//} — so matching the module itself by + * equality and its declarations by {@code id STARTS WITH + '/'} is containment, and it is + * simultaneously scoped to one language, one application and one module. That last one is what + * neither a label anchor nor {@code _module} could give: two java applications sharing + * {@code src/main/java/Foo.java} have identical labels and an identical file key, and only the + * id distinguishes them (.github#50). + * + *

The trailing slash is not cosmetic. A bare {@code STARTS WITH } would also match + * {@code Xtra}, so descendants are matched on the separator and the module on equality. + * + *

{@code :JCanNode} is present only so the prefix predicate can seek: Neo4j property indexes + * are label-scoped, and without a label this scans the whole node store. + */ + static final String PURGE_MODULE_EDGES_V2 = + "MATCH (x:" + RowBuilder.CAN_NODE + ") WHERE x.id = $mid OR x.id STARTS WITH $pre " + + "MATCH (x)-[r]->() DELETE r"; + + /** @see #PURGE_MODULE_EDGES_V2 */ + static final String PURGE_VANISHED_NODES_V2 = + "MATCH (x:" + RowBuilder.CAN_NODE + ") WHERE (x.id = $mid OR x.id STARTS WITH $pre) " + + "AND NOT x.id IN $keys DETACH DELETE x"; + + /** + * The prefix that matches a module's declarations but not a sibling module whose path merely + * starts the same way. The separator is the whole point: {@code can://java/app/src/Foo.java} + * is also a prefix of {@code can://java/app/src/Foo.javaX}, so a bare {@code STARTS WITH} on + * the module id would purge a different module's nodes. The module itself is matched by + * equality instead, since its own id does not end in a separator. + */ + static String descendantPrefix(String moduleId) { + return moduleId + "/"; + } + + /** + * The {@code can://} id of the module these rows belong to, or {@code null} on the v1 path. + * Taken from the module's own row rather than parsed out of a declaration's id, because a file + * key may itself contain {@code /} and so cannot be recovered by splitting. + */ + private static String canModuleIdOf(List nodes) { + for (NodeRow n : nodes) { + if (n.labels.contains("JModule") && RowBuilder.isCanId(n.value)) { + return n.value; + } + } + return null; + } + /** @see #PURGE_MODULE_EDGES */ static final String PURGE_VANISHED_NODES = "MATCH (x:" + CypherWriter.MODULE_OWNED + ") WHERE x._module = $m " @@ -115,10 +165,9 @@ void run(GraphRows rows, boolean fullRun) { List shared = new ArrayList<>(); Map unitOf = new HashMap<>(); // node value → owning unit for (NodeRow n : rows.nodes) { - Object m = n.props.get("_module"); - if (m instanceof String) { - byUnit.computeIfAbsent((String) m, x -> new ArrayList<>()).add(n); - unitOf.put(n.value, (String) m); + if (n.moduleKey != null) { + byUnit.computeIfAbsent(n.moduleKey, x -> new ArrayList<>()).add(n); + unitOf.put(n.value, n.moduleKey); } else { shared.add(n); } @@ -155,8 +204,25 @@ void run(GraphRows rows, boolean fullRun) { } try (Session s = session()) { s.writeTransaction(tx -> { - tx.run(PURGE_MODULE_EDGES, Values.parameters("m", unit)); - tx.run(PURGE_VANISHED_NODES, Values.parameters("m", unit, "keys", keys)); + String moduleId = canModuleIdOf(nodes); + if (moduleId != null) { + // v2: the module's own can:// id scopes this to one module of one + // application of one language, because the id is a path and a prefix + // match on it is containment. `_module` cannot do that -- it is a bare + // project-relative file key, so two applications sharing a source path + // purged each other (.github#50). + Map args = new LinkedHashMap<>(); + args.put("mid", moduleId); + args.put("pre", descendantPrefix(moduleId)); + args.put("keys", keys); + tx.run(PURGE_MODULE_EDGES_V2, args); + tx.run(PURGE_VANISHED_NODES_V2, args); + } else { + // v1 ids are FQN-shaped and carry no application segment, so there is no + // prefix to scope by. That path keeps the label anchor from #213. + tx.run(PURGE_MODULE_EDGES, Values.parameters("m", unit)); + tx.run(PURGE_VANISHED_NODES, Values.parameters("m", unit, "keys", keys)); + } return null; }); } diff --git a/src/main/java/com/ibm/cldk/neo4j/GraphRows.java b/src/main/java/com/ibm/cldk/neo4j/GraphRows.java index 0e2e40c9..4946838c 100644 --- a/src/main/java/com/ibm/cldk/neo4j/GraphRows.java +++ b/src/main/java/com/ibm/cldk/neo4j/GraphRows.java @@ -56,8 +56,25 @@ public static final class NodeRow { public final String keyProp; public final String value; public final Map props; + /** + * The file key of the module owning this node, or {@code null} for a shared node. + * + *

In memory only — deliberately NOT a property, so it is never written to the graph. The + * incremental writer needs to group nodes by module; the graph does not need to be told, + * because a node's owning module is already the prefix of its {@code can://} id. Emitting it + * was how a delete came to be scoped by a bare file key that carries no language and no + * application, which is what made two applications sharing a path delete each other's nodes + * (codellm-devkit/.github#50). + */ + public final String moduleKey; public NodeRow(List labels, String keyProp, String value, Map props) { + this(labels, keyProp, value, props, null); + } + + public NodeRow(List labels, String keyProp, String value, Map props, + String moduleKey) { + this.moduleKey = moduleKey; this.labels = labels; this.keyProp = keyProp; this.value = value; diff --git a/src/main/java/com/ibm/cldk/neo4j/RowBuilder.java b/src/main/java/com/ibm/cldk/neo4j/RowBuilder.java index 32b17156..efbf2db9 100644 --- a/src/main/java/com/ibm/cldk/neo4j/RowBuilder.java +++ b/src/main/java/com/ibm/cldk/neo4j/RowBuilder.java @@ -78,13 +78,47 @@ public NodeRef node(List labels, String keyProp, String value, Map(labels), keyProp, value, new LinkedHashMap<>(props))); + Map p = new LinkedHashMap<>(props); + // Lift the owning module off the emitted properties and onto the row. The writer needs + // it to group nodes; the graph does not, because a v2 node's owning module is already + // the prefix of its can:// id. See NodeRow.moduleKey. + Object m = p.remove(MODULE_PROP); + String moduleKey = m instanceof String ? (String) m : null; + List allLabels = new ArrayList<>(labels); + if (isCanId(value)) { + // The index anchor for prefix-scoped statements: Neo4j property indexes are + // label-scoped, so `WHERE n.id STARTS WITH $p` needs a label to seek on. Carries no + // safety claim of its own -- the prefix does that -- and is java-namespaced so an + // anchoring mistake still cannot reach another language's nodes. + allLabels.add(CAN_NODE); + } else if (moduleKey != null) { + // v1 ids are FQN-shaped and carry no application segment, so they cannot be + // prefix-scoped. That path keeps the property it has always had; only the v2 + // contract drops it. + p.put(MODULE_PROP, moduleKey); + } + nodes.put(id, new NodeRow(allLabels, keyProp, value, p, moduleKey)); } keys.add(value); return new NodeRef(labels.get(0), keyProp, value); } + /** The marker label carried by every node keyed on a {@code can://} id. */ + public static final String CAN_NODE = "JCanNode"; + + private static final String MODULE_PROP = "_module"; + + /** Whether a node key is a v2 {@code can://} id, as opposed to a v1 FQN-shaped one. */ + static boolean isCanId(String value) { + return value != null && value.startsWith("can://"); + } + /** An edge whose endpoints are known to exist (both ends emitted as nodes this run). */ public void edge(String type, NodeRef from, NodeRef to, Map props) { edges.add(new EdgeRow(type, from, to, props)); diff --git a/src/main/java/com/ibm/cldk/neo4j/Schema.java b/src/main/java/com/ibm/cldk/neo4j/Schema.java index 31991334..b84e3cf2 100644 --- a/src/main/java/com/ibm/cldk/neo4j/Schema.java +++ b/src/main/java/com/ibm/cldk/neo4j/Schema.java @@ -57,5 +57,10 @@ private Schema() {} "CREATE INDEX j_callable_name IF NOT EXISTS FOR (c:JCallable) ON (c.name)", "CREATE INDEX j_type_name IF NOT EXISTS FOR (t:JType) ON (t.name)", "CREATE INDEX j_annotation_name_idx IF NOT EXISTS FOR (an:JAnnotation) ON (an.name)", - "CREATE FULLTEXT INDEX j_code_fts IF NOT EXISTS FOR (c:JCallable) ON EACH [c.code, c.docstring]"); + "CREATE FULLTEXT INDEX j_code_fts IF NOT EXISTS FOR (c:JCallable) ON EACH [c.code, c.docstring]", + // The incremental purge matches `id STARTS WITH `. Neo4j property indexes are + // label-scoped, so without this the prefix seek degrades to a scan of every node in the + // store, once per changed module. STARTS WITH is index-backed on a range index; CONTAINS + // and ENDS WITH are not, which is why the predicate is written this way. + "CREATE INDEX j_can_node_id IF NOT EXISTS FOR (n:" + RowBuilder.CAN_NODE + ") ON (n.id)"); } diff --git a/src/main/java/com/ibm/cldk/neo4j/V2SchemaCatalog.java b/src/main/java/com/ibm/cldk/neo4j/V2SchemaCatalog.java index ab65465b..c3ace8d3 100644 --- a/src/main/java/com/ibm/cldk/neo4j/V2SchemaCatalog.java +++ b/src/main/java/com/ibm/cldk/neo4j/V2SchemaCatalog.java @@ -45,10 +45,20 @@ private V2SchemaCatalog() {} // 2.2.0: additive MINOR — the repository-artifact layer (#197): Artifact/Package/ConfigKey // reserved at 2.0.0, now actually emitted, plus HAS_ARTIFACT/DEFINES_CONFIG/ // DECLARES_DEPENDENCY/LOCKS. - public static final String SCHEMA_VERSION = "2.2.0"; + public static final String SCHEMA_VERSION = "3.0.0"; /** Labels layered onto a node in addition to its merge + specific labels. */ - public static final List MARKER_LABELS = Arrays.asList("JEntrypoint"); + /** + * Additional labels a node may carry beyond its declared and merge labels. + * + *

{@code JCanNode} is on every node keyed by a {@code can://} id and exists purely so a + * prefix predicate can seek — Neo4j property indexes are label-scoped, so + * {@code WHERE n.id STARTS WITH $p} scans the whole store without one. It makes no semantic + * claim, unlike {@code JSymbol}, and is java-namespaced so an anchoring mistake still cannot + * reach another language's nodes. + */ + public static final List MARKER_LABELS = + Arrays.asList("JEntrypoint", RowBuilder.CAN_NODE); /** Tiny ordered-map builder for property declarations. */ private static final class P { @@ -84,13 +94,13 @@ private static List buildNodeLabels() { n.add(node("JModule", "JModule", "id", new P().put("id", "string").put("file_key", "string").put("package", "string") - .put("content_hash", "string").put("_module", "string").done())); + .put("content_hash", "string").done())); n.add(node("JType", "JSymbol", "id", lines(new P().put("id", "string").put("name", "string").put("kind", "string") .put("modifiers", "string[]").put("base_types", "string[]") .put("interfaces", "string[]").put("docstring", "string") - .put("is_entrypoint", "boolean").put("_module", "string")))); + .put("is_entrypoint", "boolean")))); n.add(node("JCallable", "JSymbol", "id", lines(new P().put("id", "string").put("name", "string").put("signature", "string") @@ -100,7 +110,7 @@ private static List buildNodeLabels() { .put("cyclomatic_complexity", "integer") .put("referenced_types", "string[]").put("accessed_fields", "string[]") .put("is_implicit", "boolean").put("is_entrypoint", "boolean") - .put("_module", "string")))); + ))); n.add(node("JExternal", "JSymbol", "id", new P().put("id", "string").put("kind", "string").put("signature", "string") @@ -109,20 +119,20 @@ private static List buildNodeLabels() { n.add(node("JField", "JField", "id", lines(new P().put("id", "string").put("name", "string").put("type", "string") .put("initializer", "string").put("modifiers", "string[]") - .put("docstring", "string").put("_module", "string")))); + .put("docstring", "string")))); n.add(node("JVariable", "JVariable", "id", lines(new P().put("id", "string").put("name", "string").put("type", "string") - .put("initializer", "string").put("_module", "string")))); + .put("initializer", "string")))); n.add(node("JEnumConstant", "JEnumConstant", "id", new P().put("id", "string").put("name", "string").put("arguments", "string[]") - .put("docstring", "string").put("_module", "string").done())); + .put("docstring", "string").done())); n.add(node("JRecordComponent", "JRecordComponent", "id", new P().put("id", "string").put("name", "string").put("type", "string") .put("modifiers", "string[]").put("is_variadic", "boolean") - .put("docstring", "string").put("_module", "string").done())); + .put("docstring", "string").done())); n.add(node("JBodyNode", "JBodyNode", "id", lines(new P().put("id", "string").put("kind", "string").put("method_name", "string") @@ -130,7 +140,6 @@ private static List buildNodeLabels() { .put("return_type", "string").put("accessibility", "string") .put("is_constructor_call", "boolean").put("is_static_call", "boolean") .put("argument_types", "string[]").put("argument_expr", "string[]") - .put("_module", "string") // L4 SDG synthetic-vertex payload. .put("var", "string").put("call_node", "string")))); diff --git a/src/test/java/com/ibm/cldk/CodeAnalyzerV2CliTest.java b/src/test/java/com/ibm/cldk/CodeAnalyzerV2CliTest.java index e28cec0e..5321bae9 100644 --- a/src/test/java/com/ibm/cldk/CodeAnalyzerV2CliTest.java +++ b/src/test/java/com/ibm/cldk/CodeAnalyzerV2CliTest.java @@ -184,10 +184,10 @@ void emitSchemaAlwaysEmitsTheV2Catalog(@TempDir Path tmp) throws IOException { assertEquals(0, run("--emit", "schema", "-o", out.toString())); JsonObject doc = JsonParser.parseString(Files.readString(out.resolve("schema.neo4j.json"))) .getAsJsonObject(); - assertEquals("2.2.0", doc.get("schema_version").getAsString()); + assertEquals("3.0.0", doc.get("schema_version").getAsString()); assertEquals(0, run("--emit", "schema", "-o", out.toString(), "--schema", "v1"), "--emit schema ignores --schema"); - assertEquals("2.2.0", JsonParser.parseString(Files.readString(out.resolve("schema.neo4j.json"))) + assertEquals("3.0.0", JsonParser.parseString(Files.readString(out.resolve("schema.neo4j.json"))) .getAsJsonObject().get("schema_version").getAsString()); } diff --git a/src/test/java/com/ibm/cldk/neo4j/BoltWriterPurgeTest.java b/src/test/java/com/ibm/cldk/neo4j/BoltWriterPurgeTest.java index a38939ab..bd01e24c 100644 --- a/src/test/java/com/ibm/cldk/neo4j/BoltWriterPurgeTest.java +++ b/src/test/java/com/ibm/cldk/neo4j/BoltWriterPurgeTest.java @@ -56,12 +56,37 @@ void theAnchorCoversEveryLabelThatCarriesModule() { } @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. + void theLabelAnchorNowCoversTheLegacyPathOnly() { + // v2 nodes no longer carry `_module` at all -- they are scoped by can:// id prefix instead, + // which is language-, application- AND module-scoped where a label anchor is none of those. + // So MODULE_OWNED is now exactly the v1 vocabulary, and stays only because v1 ids are + // FQN-shaped and have no prefix to scope by. 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"); + assertFalse(anchor.contains("JModule"), + "v2 is prefix-scoped now; a v2 label here would mean _module is still being emitted"); + } + + @Test + void theVersionTwoPurgeIsScopedByCanIdPrefixNotByLabelOrModule() { + for (String stmt : new String[] {BoltWriter.PURGE_MODULE_EDGES_V2, BoltWriter.PURGE_VANISHED_NODES_V2}) { + assertFalse(stmt.contains("_module"), + "the v2 purge must not reference the retired property: " + stmt); + assertTrue(stmt.contains("x.id = $mid"), "the module itself is matched by equality: " + stmt); + assertTrue(stmt.contains("STARTS WITH $pre"), + "descendants are matched on the id prefix: " + stmt); + assertTrue(stmt.contains("(x:" + RowBuilder.CAN_NODE + ")"), + "a label is still needed so the prefix predicate can seek: " + stmt); + } + } + + @Test + void theDescendantPrefixCarriesASeparatorSoASiblingPathCannotMatch() { + // Without the trailing slash, `can://java/app/src/Foo.java` also prefixes + // `can://java/app/src/Foo.javaX` and would purge a different module's nodes. + assertTrue(BoltWriter.PURGE_VANISHED_NODES_V2.contains("$pre")); + assertTrue(BoltWriter.descendantPrefix("can://java/app/src/Foo.java").endsWith("/"), + "the descendant prefix must end with the path separator"); } @Test From 4a47fffe6a2457c6cdc5c5b841354f2ff29c0e92 Mon Sep 17 00:00:00 2001 From: Rahul Krishna Date: Wed, 2 Sep 2026 12:25:31 -0400 Subject: [PATCH 2/2] refactor(neo4j): drop the v1 purge path rather than carry two scoping mechanisms v2 is the schema as of 3.0.0. Its ids are can:// paths, so a prefix match scopes a purge to one language, one application and one module at once. v1 ids are FQN-shaped and carry no application segment, so they cannot be scoped that way at all -- and rather than keep a second, weaker mechanism alive for a legacy path, the purge is now skipped for v1 rows and says so in the log. A v1 push still upserts; it just never deletes. That retires MODULE_OWNED and both label-anchored statements from #213, which existed only to make the _module match safe. Nothing anchors on _module any more, so there is nothing left for them to protect. The purge test now pins the properties that make the new scoping correct -- the predicate shape, the label that lets the prefix seek, the separator that stops a sibling path matching, and the can:// recognition that selects the path -- rather than the label vocabulary that no longer guards anything. --- .../java/com/ibm/cldk/neo4j/BoltWriter.java | 59 +++------ .../java/com/ibm/cldk/neo4j/CypherWriter.java | 33 ----- .../ibm/cldk/neo4j/BoltWriterPurgeTest.java | 124 ++++++------------ 3 files changed, 60 insertions(+), 156 deletions(-) diff --git a/src/main/java/com/ibm/cldk/neo4j/BoltWriter.java b/src/main/java/com/ibm/cldk/neo4j/BoltWriter.java index 4c6dab84..e20d0242 100644 --- a/src/main/java/com/ibm/cldk/neo4j/BoltWriter.java +++ b/src/main/java/com/ibm/cldk/neo4j/BoltWriter.java @@ -48,24 +48,6 @@ * 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"; - /** * The v2 purge: scoped by the module's own {@code can://} id rather than by a bare file key. * @@ -116,11 +98,6 @@ private static String canModuleIdOf(List nodes) { return null; } - /** @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; @@ -205,24 +182,26 @@ void run(GraphRows rows, boolean fullRun) { try (Session s = session()) { s.writeTransaction(tx -> { String moduleId = canModuleIdOf(nodes); - if (moduleId != null) { - // v2: the module's own can:// id scopes this to one module of one - // application of one language, because the id is a path and a prefix - // match on it is containment. `_module` cannot do that -- it is a bare - // project-relative file key, so two applications sharing a source path - // purged each other (.github#50). - Map args = new LinkedHashMap<>(); - args.put("mid", moduleId); - args.put("pre", descendantPrefix(moduleId)); - args.put("keys", keys); - tx.run(PURGE_MODULE_EDGES_V2, args); - tx.run(PURGE_VANISHED_NODES_V2, args); - } else { - // v1 ids are FQN-shaped and carry no application segment, so there is no - // prefix to scope by. That path keeps the label anchor from #213. - tx.run(PURGE_MODULE_EDGES, Values.parameters("m", unit)); - tx.run(PURGE_VANISHED_NODES, Values.parameters("m", unit, "keys", keys)); + if (moduleId == null) { + // Legacy v1 rows: FQN-shaped ids carry no application segment, so there + // is no prefix to scope by. v2 is the schema as of 3.0.0, so rather than + // carry a second scoping mechanism for a legacy path, the purge is + // skipped and said so. Nodes are still upserted -- a v1 push updates, it + // just never deletes. + Log.info("neo4j(bolt): legacy v1 rows for " + unit + + " - purge skipped (v1 ids carry no application scope)"); + return null; } + // The module's own can:// id scopes this to one module of one application of + // one language at once, because the id is a path and a prefix match on it is + // containment. `_module` could not: it is a bare project-relative file key, + // so two applications sharing a source path purged each other (.github#50). + Map args = new LinkedHashMap<>(); + args.put("mid", moduleId); + args.put("pre", descendantPrefix(moduleId)); + args.put("keys", keys); + tx.run(PURGE_MODULE_EDGES_V2, args); + tx.run(PURGE_VANISHED_NODES_V2, args); return null; }); } diff --git a/src/main/java/com/ibm/cldk/neo4j/CypherWriter.java b/src/main/java/com/ibm/cldk/neo4j/CypherWriter.java index 2ace072e..bc34479b 100644 --- a/src/main/java/com/ibm/cldk/neo4j/CypherWriter.java +++ b/src/main/java/com/ibm/cldk/neo4j/CypherWriter.java @@ -17,7 +17,6 @@ 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; @@ -56,38 +55,6 @@ 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 index bd01e24c..713e6140 100644 --- a/src/test/java/com/ibm/cldk/neo4j/BoltWriterPurgeTest.java +++ b/src/test/java/com/ibm/cldk/neo4j/BoltWriterPurgeTest.java @@ -3,110 +3,68 @@ 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. + * The incremental push deletes, and what it is scoped by decides whose data it deletes. * - *

{@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. + *

Scoping by {@code _module} — a bare project-relative file key — was wrong twice over. It is set + * by the sibling analyzers too, so an unlabelled match reached their graphs (#213); and it carries + * no application, so even once labelled it could not tell two java applications sharing + * {@code src/main/java/Foo.java} apart. No label can: their nodes are identical in every respect + * except identity. * - *

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. + *

Scoping on the module's own {@code can://} id fixes both at once, because the id is a path — + * {@code can://java//} — so a prefix match is language-, application- and module-scoped + * simultaneously. + * + *

Asserted on the statement text rather than through a live database: that path needs Docker and + * does not run everywhere, while the property that makes the fix correct is entirely in the + * predicate. */ 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 theLabelAnchorNowCoversTheLegacyPathOnly() { - // v2 nodes no longer carry `_module` at all -- they are scoped by can:// id prefix instead, - // which is language-, application- AND module-scoped where a label anchor is none of those. - // So MODULE_OWNED is now exactly the v1 vocabulary, and stays only because v1 ids are - // FQN-shaped and have no prefix to scope by. - List anchor = Arrays.asList(CypherWriter.MODULE_OWNED.split("\\|")); - assertTrue(anchor.contains("JCompilationUnit"), "v1's module-owned label must be covered"); - assertFalse(anchor.contains("JModule"), - "v2 is prefix-scoped now; a v2 label here would mean _module is still being emitted"); - } - - @Test - void theVersionTwoPurgeIsScopedByCanIdPrefixNotByLabelOrModule() { + void thePurgeIsScopedByCanIdPrefixAndNotByTheRetiredProperty() { for (String stmt : new String[] {BoltWriter.PURGE_MODULE_EDGES_V2, BoltWriter.PURGE_VANISHED_NODES_V2}) { assertFalse(stmt.contains("_module"), - "the v2 purge must not reference the retired property: " + stmt); - assertTrue(stmt.contains("x.id = $mid"), "the module itself is matched by equality: " + stmt); + "the purge must not reference the retired property: " + stmt); + assertTrue(stmt.contains("x.id = $mid"), + "the module itself is matched by equality: " + stmt); assertTrue(stmt.contains("STARTS WITH $pre"), - "descendants are matched on the id prefix: " + stmt); - assertTrue(stmt.contains("(x:" + RowBuilder.CAN_NODE + ")"), - "a label is still needed so the prefix predicate can seek: " + stmt); + "its declarations are matched by id prefix: " + stmt); } } @Test - void theDescendantPrefixCarriesASeparatorSoASiblingPathCannotMatch() { - // Without the trailing slash, `can://java/app/src/Foo.java` also prefixes - // `can://java/app/src/Foo.javaX` and would purge a different module's nodes. - assertTrue(BoltWriter.PURGE_VANISHED_NODES_V2.contains("$pre")); - assertTrue(BoltWriter.descendantPrefix("can://java/app/src/Foo.java").endsWith("/"), - "the descendant prefix must end with the path separator"); + void thePurgeCarriesALabelSoThePrefixPredicateCanSeek() { + // Neo4j property indexes are label-scoped: without a label this scans every node in the + // store, once per changed module. The label is a performance anchor, not a safety one -- + // the prefix does the scoping -- but it is java-namespaced anyway, so an anchoring mistake + // still cannot reach another language's nodes. + for (String stmt : new String[] {BoltWriter.PURGE_MODULE_EDGES_V2, BoltWriter.PURGE_VANISHED_NODES_V2}) { + assertTrue(stmt.contains("(x:" + RowBuilder.CAN_NODE + ")"), stmt); + } } @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"); - } + void theDescendantPrefixEndsWithTheSeparator() { + // Dropping the separator is silent and wrong: can://java/app/src/Foo.java also prefixes + // can://java/app/src/Foo.javaX, so a bare STARTS WITH would purge a different module. + String prefix = BoltWriter.descendantPrefix("can://java/app/src/Foo.java"); + assertTrue(prefix.endsWith("/"), prefix); + assertFalse("can://java/app/src/Foo.javaX".startsWith(prefix), + "a sibling module whose path merely starts the same way must not match"); + assertTrue("can://java/app/src/Foo.java/Foo/m()".startsWith(prefix), + "a real declaration of this module must match"); } @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("|")); + void aCanIdIsRecognizedAndAVersionOneIdIsNot() { + // What selects the purge path at all: v1 ids are FQN-shaped and carry no application + // segment, so there is nothing to prefix and the purge is skipped rather than mis-scoped. + assertTrue(RowBuilder.isCanId("can://java/app/src/Foo.java")); + assertFalse(RowBuilder.isCanId("com.l4.Arity#caller(int, int)@22:16-22:25")); + assertFalse(RowBuilder.isCanId(null)); } }