Skip to content

Follow-ups to #1080: batch subject removal, self-loop deletion, test hardening#1111

Merged
alistair3149 merged 2 commits into
fix/544-referenced-subject-stubsfrom
review/1080-followups
Jul 20, 2026
Merged

Follow-ups to #1080: batch subject removal, self-loop deletion, test hardening#1111
alistair3149 merged 2 commits into
fix/544-referenced-subject-stubsfrom
review/1080-followups

Conversation

@alistair3149

@alistair3149 alistair3149 commented Jul 20, 2026

Copy link
Copy Markdown
Member

Addresses the review findings on #1080. Targets the PR branch (fix/544-referenced-subject-stubs), not master, so it can be folded into that PR before it merges.

What each commit does

Batch subject removalsrc/GraphDatabasePlugins/Neo4j/Persistence/Neo4jProjectionStore.php

  • removeAbsentSubjects and deletePage now route through one removeSubjects(), which classifies referenced-vs-unreferenced subjects in a single query and deletes the unreferenced ones in a single query, replacing the per-subject round-trip loop. Both call sites stay unified on the same stub/delete logic (the point of the fix in Keep referenced Subjects as stubs instead of deleting them #1080), so this does not re-introduce a savePage/deletePage split.
  • The classification excludes self-loops (other <> subject), so a subject whose only incoming relation is its own self-reference is deleted rather than left as an unreachable orphan stub.
  • reduceSubjectToStub gets WITH DISTINCT subject so the property/label reset runs once, not once per outgoing relation (the two OPTIONAL MATCHes form a cartesian product).
  • getSubjectIdsByPageId stops returning the node properties and label list that both callers discarded.

Test hardeningtests/…/Neo4jProjectionStoreTest.php

  • testFlippingASubjectBetweenMainAndChildLeavesASingleHasSubjectEdge locks detachSubjectsFromPage, which had no coverage. Because HasSubject is MERGEd with its isMain property, flipping a persisting subject's role would leave a duplicate edge without that method. Teeth-verified: the test goes red when the detachSubjectsFromPage call is removed.
  • testRemovingASelfReferencingSubjectDeletesItRatherThanStubbingIt — verified red on the pre-fix code (a self-loop subject was kept as a stub), green after.
  • assertRelationExists 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. Pinned at the four preservation call sites.

Behavioural note for the reviewer

Batching the classification changes one acknowledged-limitation edge: removing two mutually-referencing subjects in one save now leaves both as orphan stubs (deterministic) instead of one stub + one deleted (order-dependent). Both outcomes are within the acknowledged, harmless, self-healing orphan-stub limitation from #1080; the proper cascade-sweep fix is deferred and tracked in #1112. Flagging it because it is a minor semantic shift.

Validation

Run locally against the dev stack (@group Database):

  • Neo4jProjectionStore 34 ✓, Neo4jSubjectRelationUpdater 8 ✓
  • Projection consumers: ReplaceSubjectApi ✓, Neo4jPageIdentifiersLookup ✓, Neo4jSubjectLabelLookup ✓, MediaWikiPageDeleter
  • make cs (phpcs 579 files + phpstan) ✓
  • Live smoke on the dev wiki: deletePage on a referenced subject reduced it to a correct stub with its incoming relations preserved; a full graph rebuild restored the projection to baseline.

Draft because the full suite was not run locally; CI is the gate for the remainder.

AI-authored — Claude Code, Opus 4.8 (1M context), as follow-ups to a code review of #1080. Each production change validated against the touched + consumer @group Database tests locally, plus a live smoke on the dev wiki; not yet human-reviewed.

alistair3149 and others added 2 commits July 20, 2026 18:26
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>
@alistair3149
alistair3149 force-pushed the review/1080-followups branch from b676731 to 75a64de Compare July 20, 2026 22:26
@alistair3149
alistair3149 marked this pull request as ready for review July 20, 2026 22:28
@alistair3149
alistair3149 merged commit a5f1c3a into fix/544-referenced-subject-stubs Jul 20, 2026
6 checks passed
@alistair3149
alistair3149 deleted the review/1080-followups branch July 20, 2026 22:28
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.

1 participant