diff --git a/src/GraphDatabasePlugins/Neo4j/Persistence/Neo4jProjectionStore.php b/src/GraphDatabasePlugins/Neo4j/Persistence/Neo4jProjectionStore.php index b6ba790b1..5b6a06cd5 100644 --- a/src/GraphDatabasePlugins/Neo4j/Persistence/Neo4jProjectionStore.php +++ b/src/GraphDatabasePlugins/Neo4j/Persistence/Neo4jProjectionStore.php @@ -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 { @@ -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 ); } ); @@ -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 ] ); @@ -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 ] + ); } /** @@ -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 ] @@ -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; - } - } diff --git a/tests/phpunit/GraphDatabasePlugins/Neo4j/Persistence/Neo4jProjectionStoreTest.php b/tests/phpunit/GraphDatabasePlugins/Neo4j/Persistence/Neo4jProjectionStoreTest.php index 17495895e..0f8ab9a22 100644 --- a/tests/phpunit/GraphDatabasePlugins/Neo4j/Persistence/Neo4jProjectionStoreTest.php +++ b/tests/phpunit/GraphDatabasePlugins/Neo4j/Persistence/Neo4jProjectionStoreTest.php @@ -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 { @@ -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 { @@ -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 { @@ -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 { @@ -481,9 +528,14 @@ 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 ] ); @@ -491,6 +543,16 @@ private function assertRelationExists( string $fromSubjectId, string $relationTy $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 {