Skip to content

Keep referenced Subjects as stubs instead of deleting them#1080

Merged
alistair3149 merged 5 commits into
masterfrom
fix/544-referenced-subject-stubs
Jul 20, 2026
Merged

Keep referenced Subjects as stubs instead of deleting them#1080
alistair3149 merged 5 commits into
masterfrom
fix/544-referenced-subject-stubs

Conversation

@JeroenDeDauw

@JeroenDeDauw JeroenDeDauw commented Jul 17, 2026

Copy link
Copy Markdown
Member

Fixes #544

savePage() DETACH DELETEd Subjects that were removed from a page without checking whether other Subjects still held relations to them, silently breaking those relations. deleteSubject() (used by deletePage) already guarded against this via subjectHasIncomingRelations(). savePage() now reuses deleteSubject(), so a removed-but-still-referenced Subject is preserved rather than deleted.

This also unifies the three kinds of "referenced but absent" node onto a single stub shape: a node with only the id and wiki_id properties and the Subject label (no name, no Schema label, no other properties). The paths:

  • deleteSubject()'s keep path now strips the node to a stub, resolving the clear-properties / clear-labels TODOs (it previously kept the full stale node).
  • savePage()'s new keep path, via deleteSubject().
  • The placeholder MERGEd by Neo4jSubjectRelationUpdater when a relation targets a not-yet-existing Subject now gets the Subject label and wiki_id on create (it was previously a bare, unlabelled node).

When the real Subject is later saved, the normal save path upgrades the stub in place. All the MERGEs that reach these nodes match by id alone, so both the new labelled stubs and any pre-existing bare placeholders upgrade without creating a duplicate node.

Placeholder wiki_id is stamped with the current wiki's wiki_id, which is correct under Subject Sources v1 where relation targets are restricted to resolvable local sources. The graph is a rebuildable projection and NeoWiki is not in production, so any lingering old-shape placeholder nodes are acceptable; a graph rebuild normalises them and the id-based upgrade path handles them regardless.

docs/api/graph-model.md documents the stub shape.

Covered by integration tests (@group Database): savePage keeping a still-referenced removed Subject as a stub (regression), savePage still fully deleting an unreferenced removed Subject, deleteSubject producing a stub with cleared properties and Schema labels, the relation placeholder carrying the Subject label and wiki_id, and a real-Subject save upgrading a stub in place without duplication.

AI-authored — Claude Code, Opus 4.8 (max); batch execution of the Relations Phase 1 plan directed by @JeroenDeDauw, autonomous subagent run (no questions asked); diff not yet human-reviewed; TDD regression seen red then green, all new tests confirmed failing on reverted production, full PHPUnit suite (1798 tests) and make cs (phpcs + phpstan) green locally, CI green.

Review notes (batch pr-review pass)

A fresh-session review pass added the missing stub coverage (63cdc49): a 2-incoming×2-outgoing stub test proving outgoing relations and statement-derived properties are stripped while incoming relations survive, teeth-verified by mutating the production clause and watching it fail. Known limitation, accepted for now: removing two mutually-referencing subjects in one save can leave an orphan stub (order-dependent; harmless — no reader surfaces it — and self-healing on RebuildGraphDatabases.php). A robust fix needs cascade sweeping, so it is left for a follow-up.

Accuracy correction to the attribution note above: two of the original tests (…UnreferencedSubjectDeletesIt, …UpgradesItsStubInPlace) are guard tests that also pass on master; the revert-verification claim holds for the four fix-proving tests.

Review-pass notes added by Claude Code, Fable 5 (max), summarizing a fresh-session Opus 4.8 review agent's verified findings; not yet human-reviewed.

@JeroenDeDauw
JeroenDeDauw marked this pull request as ready for review July 18, 2026 23:25
JeroenDeDauw added a commit that referenced this pull request Jul 20, 2026
Follows-up to #1080

The stub reduction added in #1080 gave Neo4jProjectionStore a getSubjectLabels() and
removeNonStubLabels() that near-verbatim duplicated Neo4jSubjectUpdater's getNodeLabels()
and the remove branch of updateNodeLabels(): the same read-labels query, the same empty
guard, and the same array_diff + Cypher::buildLabelList + MATCH ... REMOVE orchestration.

Both operations now live in a small stateless Neo4jNodeLabels helper (read() and remove()),
which the Store's removeNonStubLabels() and the Updater's updateNodeLabels() both call. The
Updater still reads its labels once and keeps its add branch inline, since that branch is not
duplicated anywhere.

Why this clears the original "grows beyond a safe move" objection: the two classes hold their
transaction differently -- the Updater receives it in its constructor, the Store threads it
through its methods from writeTransaction() -- but neither needs to own the transaction to run
these queries. Neo4jNodeLabels takes the TransactionInterface as a parameter, so it is agnostic
to ownership, and as a static utility (like the sibling Cypher class) it is not a DI
collaborator: nothing to inject or wire.

No behavior change and no test-expectation edits. Neo4jProjectionStoreTest gains a @Covers for
the new helper -- it exercises it through both the stub path and, via savePage, the Updater
path; the Updater unit test mocks the transaction and never runs the label queries, so it does
not claim coverage. The now-dead getNodeLabels phpstan-baseline entries are dropped.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
JeroenDeDauw and others added 3 commits July 20, 2026 21:26
Fixes #544

savePage() DETACH DELETEd Subjects that were removed from a page without
checking whether other Subjects still held relations to them, silently
breaking those relations. deleteSubject() (used by deletePage) already
guarded against this via subjectHasIncomingRelations(). savePage() now
reuses deleteSubject(), so a removed-but-still-referenced Subject is
preserved rather than deleted.

This also unifies the three kinds of "referenced but absent" node onto a
single stub shape: a node with only the id and wiki_id properties and the
Subject label (no name, no Schema label, no other properties). The paths:

- deleteSubject()'s keep path now strips the node to a stub, resolving the
  clear-properties / clear-labels TODOs (it previously kept the full stale
  node).
- savePage()'s new keep path, via deleteSubject().
- The placeholder MERGEd by Neo4jSubjectRelationUpdater when a relation
  targets a not-yet-existing Subject now gets the Subject label and wiki_id
  on create (it was previously a bare, unlabelled node).

When the real Subject is later saved, the normal save path upgrades the
stub in place. All the MERGEs that reach these nodes match by id alone, so
both the new labelled stubs and any pre-existing bare placeholders upgrade
without creating a duplicate node.

Placeholder wiki_id is stamped with the current wiki's wiki_id, which is
correct under Subject Sources v1 where relation targets are restricted to
resolvable local sources. The graph is a rebuildable projection and NeoWiki
is not in production, so any lingering old-shape placeholder nodes are
acceptable; a graph rebuild normalises them and the id-based upgrade path
handles them regardless.

docs/api/graph-model.md documents the stub shape.

Covered by integration tests (@group Database): savePage keeping a
still-referenced removed Subject as a stub (regression), savePage still
fully deleting an unreferenced removed Subject, deleteSubject producing a
stub with cleared properties and Schema labels, the relation placeholder
carrying the Subject label and wiki_id, and a real-Subject save upgrading a
stub in place without duplication.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The existing stub tests only reduced subjects that had neither outgoing
relations nor statement-derived properties, so the outgoing-relation
stripping in reduceSubjectToStub, and the property stripping that
assertSubjectIsStub's property count is meant to prove, were never
actually exercised. Removing the outgoing-relation DELETE clause left
every test green.

Add a test that reduces a subject with two incoming relations, two
outgoing relations, and a scalar property to a stub, asserting the
incoming relations are kept while the outgoing relations and properties
are stripped. This also exercises the multiple-incoming/multiple-outgoing
row combination in reduceSubjectToStub. Verified it fails when the
outgoing-relation DELETE clause is removed.

Also guard the stub-target assertion in the relation-updater test against
an empty result before dereferencing the first row.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Follows-up to #1080

The stub reduction added in #1080 gave Neo4jProjectionStore a getSubjectLabels() and
removeNonStubLabels() that near-verbatim duplicated Neo4jSubjectUpdater's getNodeLabels()
and the remove branch of updateNodeLabels(): the same read-labels query, the same empty
guard, and the same array_diff + Cypher::buildLabelList + MATCH ... REMOVE orchestration.

Both operations now live in a small stateless Neo4jNodeLabels helper (read() and remove()),
which the Store's removeNonStubLabels() and the Updater's updateNodeLabels() both call. The
Updater still reads its labels once and keeps its add branch inline, since that branch is not
duplicated anywhere.

Why this clears the original "grows beyond a safe move" objection: the two classes hold their
transaction differently -- the Updater receives it in its constructor, the Store threads it
through its methods from writeTransaction() -- but neither needs to own the transaction to run
these queries. Neo4jNodeLabels takes the TransactionInterface as a parameter, so it is agnostic
to ownership, and as a static utility (like the sibling Cypher class) it is not a DI
collaborator: nothing to inject or wire.

No behavior change and no test-expectation edits. Neo4jProjectionStoreTest gains a @Covers for
the new helper -- it exercises it through both the stub path and, via savePage, the Updater
path; the Updater unit test mocks the transaction and never runs the label queries, so it does
not claim coverage. The now-dead getNodeLabels phpstan-baseline entries are dropped.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@JeroenDeDauw
JeroenDeDauw force-pushed the fix/544-referenced-subject-stubs branch from b6bb7e7 to 67427dd Compare July 20, 2026 19:30
@alistair3149 alistair3149 self-assigned this Jul 20, 2026
alistair3149 added a commit that referenced this pull request Jul 20, 2026
Follow-up to #1080.

- removeAbsentSubjects and deletePage now route through a single
  removeSubjects() that classifies referenced-vs-unreferenced subjects
  in one query and deletes the unreferenced ones in one query, instead
  of one round trip per subject. Both call sites stay unified on the
  same stub/delete logic.
- subjectIdsWithIncomingRelations excludes self-loops, so a subject
  whose only incoming relation is its own self-reference is deleted
  rather than left as an unreachable orphan stub.
- reduceSubjectToStub collapses the OPTIONAL MATCH cartesian product
  with WITH DISTINCT so the property reset runs once, not once per
  outgoing relation.
- getSubjectIdsByPageId no longer fetches the node properties and
  labels that both callers discard.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
alistair3149 added a commit that referenced this pull request Jul 20, 2026
Follow-up to #1080.

- testFlippingASubjectBetweenMainAndChildLeavesASingleHasSubjectEdge
  locks detachSubjectsFromPage: without it, flipping a subject's isMain
  flag leaves a duplicate HasSubject edge (verified red when the call is
  removed).
- testRemovingASelfReferencingSubjectDeletesItRatherThanStubbingIt
  guards the self-loop deletion.
- assertRelationExists now optionally asserts the preserved relation's
  id, so a regression that keeps an incoming relation by endpoints and
  type but drops its id is caught.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
alistair3149 and others added 2 commits July 20, 2026 18:28
Follow-up to #1080.

- removeAbsentSubjects and deletePage now route through a single
  removeSubjects() that classifies referenced-vs-unreferenced subjects
  in one query and deletes the unreferenced ones in one query, instead
  of one round trip per subject. Both call sites stay unified on the
  same stub/delete logic.
- subjectIdsWithIncomingRelations excludes self-loops, so a subject
  whose only incoming relation is its own self-reference is deleted
  rather than left as an unreachable orphan stub.
- reduceSubjectToStub collapses the OPTIONAL MATCH cartesian product
  with WITH DISTINCT so the property reset runs once, not once per
  outgoing relation.
- getSubjectIdsByPageId no longer fetches the node properties and
  labels that both callers discard.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Follow-up to #1080.

- testFlippingASubjectBetweenMainAndChildLeavesASingleHasSubjectEdge
  locks detachSubjectsFromPage: without it, flipping a subject's isMain
  flag leaves a duplicate HasSubject edge (verified red when the call is
  removed).
- testRemovingASelfReferencingSubjectDeletesItRatherThanStubbingIt
  guards the self-loop deletion.
- assertRelationExists now optionally asserts the preserved relation's
  id, so a regression that keeps an incoming relation by endpoints and
  type but drops its id is caught.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

savePage() DETACH DELETEs subjects without checking incoming relations

2 participants