Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alistair3149 merged 2 commits into from
Jul 20, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
172037f
Batch absent/deleted subject removal in the Neo4j projection
alistair3149 Jul 20, 2026
75a64de
Cover main/child flips, self-referencing removal, and relation ids
alistair3149 Jul 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 70 additions & 28 deletions src/GraphDatabasePlugins/Neo4j/Persistence/Neo4jProjectionStore.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,17 +70,18 @@ private function upsertPageNode( TransactionInterface $transaction, Page $page )

/**
* Removes the subjects that are attached to the page in the graph but are no longer present on the
* page. This reuses deleteSubject so that a removed subject still referenced by other subjects is
* page. This reuses removeSubjects so that a removed subject still referenced by other subjects is
* kept as a stub instead of being deleted, keeping the incoming relations valid.
*/
private function removeAbsentSubjects( TransactionInterface $transaction, Page $page ): void {
$presentSubjectIds = $page->getSubjects()->getAllSubjects()->getIdsAsTextArray();

foreach ( $this->getSubjectIdsByPageId( $transaction, $page->getId() ) as $attachedSubjectId ) {
if ( !in_array( $attachedSubjectId, $presentSubjectIds, true ) ) {
$this->deleteSubject( $transaction, new SubjectId( $attachedSubjectId ) );
}
}
$absentSubjectIds = array_values( array_filter(
$this->getSubjectIdsByPageId( $transaction, $page->getId() ),
fn( string $subjectId ) => !in_array( $subjectId, $presentSubjectIds, true )
) );

$this->removeSubjects( $transaction, $absentSubjectIds );
}

private function detachSubjectsFromPage( TransactionInterface $transaction, PageId $pageId ): void {
Expand Down Expand Up @@ -153,9 +154,7 @@ private function updateSubjects( TransactionInterface $transaction, Page $page )

public function deletePage( PageId $pageId ): void {
$this->client->writeTransaction( function ( TransactionInterface $transaction ) use ( $pageId ): void {
foreach ( $this->getSubjectIdsByPageId( $transaction, $pageId ) as $subjectId ) {
$this->deleteSubject( $transaction, new SubjectId( $subjectId ) );
}
$this->removeSubjects( $transaction, $this->getSubjectIdsByPageId( $transaction, $pageId ) );

$this->deletePageNode( $transaction, $pageId );
} );
Expand All @@ -178,7 +177,7 @@ private function getSubjectIdsByPageId( TransactionInterface $transaction, PageI
*/
$results = $transaction->run(
'MATCH (page:Page {id: $pageId, wiki_id: $wikiId})-[:HasSubject]->(subject:Subject)
RETURN subject.id AS id, subject AS properties, labels(subject) AS labels',
RETURN subject.id AS id',
[ 'pageId' => $pageId->id, 'wikiId' => $this->wikiId ]
);

Expand All @@ -188,17 +187,68 @@ private function getSubjectIdsByPageId( TransactionInterface $transaction, PageI
);
}

private function deleteSubject( TransactionInterface $transaction, SubjectId $subjectId ): void {
if ( $this->subjectHasIncomingRelations( $transaction, $subjectId ) ) {
$this->reduceSubjectToStub( $transaction, $subjectId );
/**
* Removes the given subjects from the graph. A subject still referenced by an incoming relation from
* another subject is reduced to a stub so that reference stays valid; the rest are deleted outright.
*
* The referenced/unreferenced split is computed with a single query and the unreferenced subjects are
* deleted with a single query, rather than one round trip per subject.
*
* @param string[] $subjectIds
*/
private function removeSubjects( TransactionInterface $transaction, array $subjectIds ): void {
if ( $subjectIds === [] ) {
return;
}
else {
$transaction->run(
'MATCH (subject {id: $subjectId})
DETACH DELETE subject',
[ 'subjectId' => $subjectId->text ]
);

$referencedSubjectIds = $this->subjectIdsWithIncomingRelations( $transaction, $subjectIds );

$this->deleteSubjects( $transaction, array_values( array_diff( $subjectIds, $referencedSubjectIds ) ) );

foreach ( $referencedSubjectIds as $subjectId ) {
$this->reduceSubjectToStub( $transaction, new SubjectId( $subjectId ) );
}
}

/**
* Returns the subset of the given subject ids that still have an incoming relation from a *different*
* subject. HasSubject relations do not count, and neither does a self-loop: a subject whose only
* incoming relation is its own outgoing self-reference has no external referrer, so it is deleted
* rather than kept as an unreachable stub.
*
* @param string[] $subjectIds
* @return string[]
*/
private function subjectIdsWithIncomingRelations( TransactionInterface $transaction, array $subjectIds ): array {
/**
* @var SummarizedResult $result
*/
$result = $transaction->run(
'UNWIND $subjectIds AS subjectId
MATCH (subject {id: subjectId})<-[incomingRelation]-(other)
WHERE NOT incomingRelation:HasSubject AND other <> subject
RETURN DISTINCT subject.id AS id',
[ 'subjectIds' => $subjectIds ]
);

return array_map(
fn( $record ) => $record->get( 'id' ),
$result->toArray()
);
}

/**
* @param string[] $subjectIds
*/
private function deleteSubjects( TransactionInterface $transaction, array $subjectIds ): void {
if ( $subjectIds === [] ) {
return;
}

$transaction->run(
'MATCH (subject) WHERE subject.id IN $subjectIds DETACH DELETE subject',
[ 'subjectIds' => $subjectIds ]
);
}

/**
Expand All @@ -213,6 +263,7 @@ private function reduceSubjectToStub( TransactionInterface $transaction, Subject
OPTIONAL MATCH ()-[hasSubject:HasSubject]->(subject)
OPTIONAL MATCH (subject)-[outgoingRelation]->()
DELETE hasSubject, outgoingRelation
WITH DISTINCT subject
SET subject = {id: $subjectId, wiki_id: $wikiId}
SET subject:Subject',
[ 'subjectId' => $subjectId->text, 'wikiId' => $this->wikiId ]
Expand All @@ -229,13 +280,4 @@ private function removeNonStubLabels( TransactionInterface $transaction, Subject
);
}

private function subjectHasIncomingRelations( TransactionInterface $transaction, SubjectId $subjectId ): bool {
return $transaction->run(
'MATCH (subject {id: $subjectId})<-[incomingRelation]-()
WHERE NOT incomingRelation:HasSubject
RETURN incomingRelation',
[ 'subjectId' => $subjectId->text ]
)->isEmpty() === false;
}

}
76 changes: 69 additions & 7 deletions tests/phpunit/GraphDatabasePlugins/Neo4j/Persistence/Neo4jProjectionStoreTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,7 @@ public function testSavingPageWithoutAReferencedSubjectPreservesIncomingRelation

$store->savePage( TestPage::build( id: 1 ) );

$this->assertRelationExists( self::GUID_2, 'LocatedIn', self::GUID_1 );
$this->assertRelationExists( self::GUID_2, 'LocatedIn', self::GUID_1, 'rTestNQS1111rr1' );
}

public function testSavingPageWithoutAReferencedSubjectReducesItToAStub(): void {
Expand Down Expand Up @@ -361,7 +361,7 @@ public function testDeletingPageReducesAReferencedSubjectToAStub(): void {
$store->deletePage( new PageId( 1 ) );

$this->assertSubjectIsStub( self::GUID_1 );
$this->assertRelationExists( self::GUID_2, 'LocatedIn', self::GUID_1 );
$this->assertRelationExists( self::GUID_2, 'LocatedIn', self::GUID_1, 'rTestNQS1111rr1' );
}

public function testSavingASubjectUpgradesItsStubInPlace(): void {
Expand Down Expand Up @@ -390,7 +390,7 @@ public function testSavingASubjectUpgradesItsStubInPlace(): void {

$this->assertSame( [ 'Subject', TestSubject::DEFAULT_SCHEMA_ID ], $labels );
$this->assertSame( 'Real subject', $row['name'] );
$this->assertRelationExists( self::GUID_2, 'LocatedIn', self::GUID_1 );
$this->assertRelationExists( self::GUID_2, 'LocatedIn', self::GUID_1, 'rTestNQS1111rr1' );
}

public function testReducingReferencedSubjectToStubKeepsIncomingRelationsButStripsOutgoingRelationsAndProperties(): void {
Expand Down Expand Up @@ -435,8 +435,55 @@ public function testReducingReferencedSubjectToStubKeepsIncomingRelationsButStri

$this->assertSubjectIsStub( self::GUID_1 );
$this->assertOutgoingRelationCount( self::GUID_1, 0 );
$this->assertRelationExists( self::GUID_2, 'LocatedIn', self::GUID_1 );
$this->assertRelationExists( self::GUID_5, 'LocatedIn', self::GUID_1 );
$this->assertRelationExists( self::GUID_2, 'LocatedIn', self::GUID_1, 'rTestNQS1111rCA' );
$this->assertRelationExists( self::GUID_5, 'LocatedIn', self::GUID_1, 'rTestNQS1111rDA' );
}

public function testFlippingASubjectBetweenMainAndChildLeavesASingleHasSubjectEdge(): void {
$store = $this->newProjectionStore();

$store->savePage( TestPage::build(
id: 42,
mainSubject: TestSubject::build( id: self::GUID_1 ),
childSubjects: new SubjectMap(
TestSubject::build( id: self::GUID_2 ),
)
) );

// Swap the roles: GUID_2 becomes the main subject and GUID_1 becomes a child.
// The HasSubject relation carries the isMain flag, so re-saving must not leave a
// second, stale HasSubject edge behind for either subject.
$store->savePage( TestPage::build(
id: 42,
mainSubject: TestSubject::build( id: self::GUID_2 ),
childSubjects: new SubjectMap(
TestSubject::build( id: self::GUID_1 ),
)
) );

$this->assertPageHasSubjects(
[
[ 'id' => self::GUID_1, 'hs' => [ 'isMain' => false ] ],
[ 'id' => self::GUID_2, 'hs' => [ 'isMain' => true ] ],
],
42
);
}

public function testRemovingASelfReferencingSubjectDeletesItRatherThanStubbingIt(): void {
$store = $this->newProjectionStoreWithLocationRelation();

// GUID_1 holds a relation to itself and nothing else references it.
$store->savePage( TestPage::build(
id: 1,
mainSubject: $this->buildSubjectWithLocationRelation( self::GUID_1, self::GUID_1, 'rTestNQS1111rr1' ),
) );

// Removing it from its page must delete it: a self-loop is not an external reference,
// so keeping it as a stub would leave an unreachable orphan node.
$store->savePage( TestPage::build( id: 1 ) );

$this->assertSubjectDoesNotExist( self::GUID_1 );
}

private function newProjectionStoreWithLocationRelation( string $wikiId = self::WIKI_ID ): GraphDatabasePlugin {
Expand Down Expand Up @@ -481,16 +528,31 @@ private function buildSubjectWithLocationRelation( string $id, string $targetId,
);
}

private function assertRelationExists( string $fromSubjectId, string $relationType, string $toSubjectId ): void {
private function assertRelationExists(
string $fromSubjectId,
string $relationType,
string $toSubjectId,
?string $expectedRelationId = null
): void {
$result = $this->readGraph(
'MATCH ({id: $from})-[relation:' . $relationType . ']->({id: $to}) RETURN relation',
'MATCH ({id: $from})-[relation:' . $relationType . ']->({id: $to}) RETURN relation.id AS id',
[ 'from' => $fromSubjectId, 'to' => $toSubjectId ]
);

$this->assertFalse(
$result->isEmpty(),
"Relation {$fromSubjectId}-[{$relationType}]->{$toSubjectId} should exist"
);

// Readers reconcile relations by their id, so a relation that survives by endpoints and type
// but loses its id is broken. Assert the id is preserved when the caller pins it down.
if ( $expectedRelationId !== null ) {
$this->assertSame(
$expectedRelationId,
$result->first()->toRecursiveArray()['id'],
"Relation {$fromSubjectId}-[{$relationType}]->{$toSubjectId} should keep its id"
);
}
}

private function subjectHasProperty( string $subjectId, string $property ): bool {
Expand Down
Loading