From 1fa3abf2714618676ecc2dd5982b9606cdf24d41 Mon Sep 17 00:00:00 2001 From: Jeroen De Dauw Date: Fri, 17 Jul 2026 01:01:52 +0200 Subject: [PATCH] Project imported pages into the graph Fixes https://github.com/ProfessionalWiki/NeoWiki/issues/1022 Imported revisions bypass the edit path, so RevisionFromEditComplete never fires for them and their Subjects stayed invisible to queries until RebuildGraphDatabases.php ran. The cause holds for only part of the import surface. Special:Import and the import API already projected, because ImportReporter creates a null revision on top of an import that added revisions, and fires RevisionFromEditComplete for it. importDump.php wraps the importer in no reporter, so nothing projected there. Handling AfterImportPage covers every path at once rather than adding a third special case. It is fired once per page, from WikiImporter itself, once the page's revisions are all in. A revision-insert hook was rejected because imports can add revisions to the middle of a page's history, so it would project non-current revisions. The handler reuses SubjectPageRebuilder, which RebuildGraphDatabases.php (the documented workaround) already uses to project the current revision of a title. It gets the isolating graph plugin, so a projection failure cannot abort the import, and reads the page from the primary database, since a replica can be missing the page or still carry the revision the import replaced. Core's own finishImportPage reads the primary for the same reason. On Special:Import the null revision now reprojects content this hook already projected. That is redundant rather than harmful: both writes carry the same slots and savePage merges, so the outcome is unchanged. Verified on a dev wiki: a page exported with dumpBackup.php and imported under a new title via importDump.php now reaches Neo4j with its Subject, where before it had no node at all. Co-Authored-By: Claude Opus 4.8 (1M context) --- extension.json | 1 + src/Application/SubjectPageRebuilder.php | 19 ++- src/EntryPoints/NeoWikiHooks.php | 23 ++++ src/NeoWikiExtension.php | 14 +- .../Application/SubjectPageRebuilderTest.php | 30 +++++ .../HookPathIsolatesProjectionFailureTest.php | 21 ++- .../EntryPoints/ImportGraphProjectionTest.php | 120 ++++++++++++++++++ tests/phpunit/NeoWikiIntegrationTestCase.php | 35 +++++ 8 files changed, 258 insertions(+), 5 deletions(-) create mode 100644 tests/phpunit/EntryPoints/ImportGraphProjectionTest.php diff --git a/extension.json b/extension.json index cbc95fdde..7b5a98974 100644 --- a/extension.json +++ b/extension.json @@ -43,6 +43,7 @@ "RevisionFromEditComplete": "ProfessionalWiki\\NeoWiki\\EntryPoints\\NeoWikiHooks::onRevisionFromEditComplete", "PageDeleteComplete": "ProfessionalWiki\\NeoWiki\\EntryPoints\\NeoWikiHooks::onPageDeleteComplete", "RevisionUndeleted": "ProfessionalWiki\\NeoWiki\\EntryPoints\\NeoWikiHooks::onRevisionUndeleted", + "AfterImportPage": "ProfessionalWiki\\NeoWiki\\EntryPoints\\NeoWikiHooks::onAfterImportPage", "CodeEditorGetPageLanguage": "ProfessionalWiki\\NeoWiki\\EntryPoints\\NeoWikiHooks::onCodeEditorGetPageLanguage", diff --git a/src/Application/SubjectPageRebuilder.php b/src/Application/SubjectPageRebuilder.php index 724fa80f5..f3506e014 100644 --- a/src/Application/SubjectPageRebuilder.php +++ b/src/Application/SubjectPageRebuilder.php @@ -7,6 +7,7 @@ use MediaWiki\Page\WikiPageFactory; use MediaWiki\Title\Title; use ProfessionalWiki\NeoWiki\EntryPoints\OnRevisionCreatedHandler; +use Wikimedia\Rdbms\IDBAccessObject; class SubjectPageRebuilder { @@ -17,7 +18,23 @@ public function __construct( } public function rebuild( Title $title ): PageRefreshOutcome { - $revision = $this->wikiPageFactory->newFromTitle( $title )->getRevisionRecord(); + return $this->rebuildWithReadFlags( $title, IDBAccessObject::READ_NORMAL ); + } + + /** + * Rebuilds from the primary database. Needed when rebuilding right after a write, such as on the + * import path: a replica can still be missing the page, or still carry the revision the import + * replaced, which would project outdated content. + */ + public function rebuildFromPrimary( Title $title ): PageRefreshOutcome { + return $this->rebuildWithReadFlags( $title, IDBAccessObject::READ_LATEST ); + } + + private function rebuildWithReadFlags( Title $title, int $readFlags ): PageRefreshOutcome { + $wikiPage = $this->wikiPageFactory->newFromTitle( $title ); + $wikiPage->loadPageData( $readFlags ); + + $revision = $wikiPage->getRevisionRecord(); if ( $revision === null ) { return PageRefreshOutcome::SkippedMissingRevision; diff --git a/src/EntryPoints/NeoWikiHooks.php b/src/EntryPoints/NeoWikiHooks.php index 8ee10f858..9784b8189 100644 --- a/src/EntryPoints/NeoWikiHooks.php +++ b/src/EntryPoints/NeoWikiHooks.php @@ -13,6 +13,7 @@ use MediaWiki\Permissions\Authority; use MediaWiki\Revision\RevisionRecord; use MediaWiki\Revision\SlotRoleRegistry; +use MediaWiki\Title\ForeignTitle; use MediaWiki\Title\Title; use MediaWiki\User\UserIdentity; use ProfessionalWiki\NeoWiki\Domain\Page\PageId; @@ -184,6 +185,28 @@ public static function onRevisionFromEditComplete( $wikiPage->doPurge(); // clear cache } + /** + * Projects imported pages, which RevisionFromEditComplete does not cover, as imported revisions do + * not go through the edit path. WikiImporter fires this hook for every import path, once per page and + * only once all of the page's revisions are in, so one handler replaces what would otherwise be a + * special case per import path. Special:Import and the import API additionally project through + * RevisionFromEditComplete, because ImportReporter creates a null revision on top of the import; that + * reprojects the same content, making it redundant rather than harmful. + * + * @see AfterImportPageHook + * + * @param array $pageInfo + */ + public static function onAfterImportPage( + Title $title, + ForeignTitle $foreignTitle, + int $revCount, + int $sRevCount, + array $pageInfo + ): void { + NeoWikiExtension::getInstance()->newImportSubjectPageRebuilder()->rebuildFromPrimary( $title ); + } + public static function onCodeEditorGetPageLanguage( Title $title, ?string &$lang, ?string $model, ?string $format ): void { if ( in_array( $model, [ SubjectContent::CONTENT_MODEL_ID, SchemaContent::CONTENT_MODEL_ID, LayoutContent::CONTENT_MODEL_ID, MappingContent::CONTENT_MODEL_ID ] ) ) { $lang = 'json'; diff --git a/src/NeoWikiExtension.php b/src/NeoWikiExtension.php index cc9c3b111..ba8ec6b64 100644 --- a/src/NeoWikiExtension.php +++ b/src/NeoWikiExtension.php @@ -632,8 +632,20 @@ public function newSubjectContentRepository(): SubjectContentRepository { } public function newSubjectPageRebuilder(): SubjectPageRebuilder { + return $this->newSubjectPageRebuilderWith( $this->newRebuildStoreContentHandler() ); + } + + /** + * Import path: projects the current revision of a page like the rebuild path, but with the hook + * path's failure isolation, since a projection failure must not abort the user's import. + */ + public function newImportSubjectPageRebuilder(): SubjectPageRebuilder { + return $this->newSubjectPageRebuilderWith( $this->getStoreContentUC() ); + } + + private function newSubjectPageRebuilderWith( OnRevisionCreatedHandler $handler ): SubjectPageRebuilder { return new SubjectPageRebuilder( - $this->newRebuildStoreContentHandler(), + $handler, MediaWikiServices::getInstance()->getWikiPageFactory() ); } diff --git a/tests/phpunit/Application/SubjectPageRebuilderTest.php b/tests/phpunit/Application/SubjectPageRebuilderTest.php index 86a7a81f3..c2efb47c6 100644 --- a/tests/phpunit/Application/SubjectPageRebuilderTest.php +++ b/tests/phpunit/Application/SubjectPageRebuilderTest.php @@ -13,6 +13,7 @@ use ProfessionalWiki\NeoWiki\Application\PageRefreshOutcome; use ProfessionalWiki\NeoWiki\Application\SubjectPageRebuilder; use ProfessionalWiki\NeoWiki\Tests\TestDoubles\SpyOnRevisionCreatedHandler; +use Wikimedia\Rdbms\IDBAccessObject; use WikiPage; /** @@ -22,8 +23,14 @@ class SubjectPageRebuilderTest extends TestCase { private SpyOnRevisionCreatedHandler $handler; + /** + * @var int[] + */ + private array $pageDataReads; + protected function setUp(): void { $this->handler = new SpyOnRevisionCreatedHandler(); + $this->pageDataReads = []; } public function testReturnsRefreshedWhenHandlerWritesPage(): void { @@ -68,9 +75,32 @@ public function testPassesRevisionAuthorToHandler(): void { $this->assertSame( $author, $this->handler->calls[0]['user'] ); } + public function testRebuildReadsPageStateFromReplica(): void { + $this->newRebuilder( $this->newRevisionByUser( new UserIdentityValue( 42, 'RevisionAuthor' ) ) ) + ->rebuild( Title::makeTitle( NS_MAIN, 'AnyPage' ) ); + + $this->assertSame( [ IDBAccessObject::READ_NORMAL ], $this->pageDataReads ); + } + + public function testRebuildFromPrimaryReadsPageStateFromPrimary(): void { + $this->newRebuilder( $this->newRevisionByUser( new UserIdentityValue( 42, 'RevisionAuthor' ) ) ) + ->rebuildFromPrimary( Title::makeTitle( NS_MAIN, 'AnyPage' ) ); + + $this->assertSame( + [ IDBAccessObject::READ_LATEST ], + $this->pageDataReads, + 'a rebuild right after a write must not be served the revision the write replaced' + ); + } + private function newRebuilder( ?RevisionRecord $revision ): SubjectPageRebuilder { $page = $this->createStub( WikiPage::class ); $page->method( 'getRevisionRecord' )->willReturn( $revision ); + $page->method( 'loadPageData' )->willReturnCallback( + function ( int $from ): void { + $this->pageDataReads[] = $from; + } + ); $factory = $this->createStub( WikiPageFactory::class ); $factory->method( 'newFromTitle' )->willReturn( $page ); diff --git a/tests/phpunit/EntryPoints/HookPathIsolatesProjectionFailureTest.php b/tests/phpunit/EntryPoints/HookPathIsolatesProjectionFailureTest.php index dbe6c5db7..6a79aa206 100644 --- a/tests/phpunit/EntryPoints/HookPathIsolatesProjectionFailureTest.php +++ b/tests/phpunit/EntryPoints/HookPathIsolatesProjectionFailureTest.php @@ -16,9 +16,11 @@ use StatusValue; /** - * Guards the hook-facing write path at the wiring level: an edit and a delete must survive a backend - * that is down, and a failing backend must not starve the backends composed after it. Undeletes run - * through the same getStoreContentUC() wiring, so they are covered by the same guard. + * Guards the hook-facing write path at the wiring level: an edit, a delete and an import must survive a + * backend that is down, and a failing backend must not starve the backends composed after it. Undeletes + * run through the same getStoreContentUC() wiring, so they are covered by the same guard. Imports get + * their own case: they reach that wiring through a second factory method, which could be rewired to the + * propagating rebuild handler on its own. * * The counterpart of RebuildPathPropagatesProjectionFailureTest, which guards the opposite need on * the maintenance path. Both are wiring tests on purpose: the decorator has its own unit tests, but @@ -62,6 +64,19 @@ public function testDeleteCommitsWhenABackendIsDown(): void { $this->assertStatusGood( $status, 'the deletion should still commit while a backend is unreachable' ); } + public function testImportCommitsWhenABackendIsDown(): void { + $this->createPageWithSubjects( 'Import during outage', TestSubject::build() ); + $xml = $this->exportPageToXml( 'Import during outage' ); + $this->registerGraphDatabasePlugins( new ThrowingGraphDatabasePlugin() ); + + $this->importXml( str_replace( 'Import during outage', 'Imported during outage', $xml ) ); + + $this->assertTrue( + Title::newFromText( 'Imported during outage' )->exists(), + 'the import should still commit while a backend is unreachable' + ); + } + public function testFailingBackendDoesNotStarveTheBackendsAfterIt(): void { $spy = new SpyGraphDatabasePlugin(); $this->registerGraphDatabasePlugins( new ThrowingGraphDatabasePlugin(), $spy ); diff --git a/tests/phpunit/EntryPoints/ImportGraphProjectionTest.php b/tests/phpunit/EntryPoints/ImportGraphProjectionTest.php new file mode 100644 index 000000000..dd8944891 --- /dev/null +++ b/tests/phpunit/EntryPoints/ImportGraphProjectionTest.php @@ -0,0 +1,120 @@ +setUpNeo4j(); + $this->createSchema( TestSubject::DEFAULT_SCHEMA_ID ); + $this->markPageTableAsUsed(); + } + + public function testImportedPageProjectsItsSubjects(): void { + $this->createPageWithSubjects( + 'Import source', + TestSubject::build( label: 'Imported subject' ) + ); + $xml = $this->exportPageToXml( 'Import source' ); + $this->emptyTheGraph(); + + $this->importXml( str_replace( 'Import source', 'Import target', $xml ) ); + + $this->assertSame( + [ 'Imported subject' ], + $this->readSubjectLabels( $this->pageIdOf( 'Import target' ) ) + ); + } + + public function testImportedPageNodeUsesTheTitleItWasImportedUnder(): void { + $this->createPageWithSubjects( 'Rename source', TestSubject::build() ); + $xml = $this->exportPageToXml( 'Rename source' ); + $this->emptyTheGraph(); + + $this->importXml( str_replace( 'Rename source', 'Rename target', $xml ) ); + + $this->assertSame( 'Rename target', $this->readPageNodeName( $this->pageIdOf( 'Rename target' ) ) ); + } + + public function testImportingNewerRevisionUpdatesProjectionOfExistingPage(): void { + $this->createPageWithSubjects( 'Update source', TestSubject::build( label: 'Before import' ) ); + $outdatedXml = $this->exportPageToXml( 'Update source' ); + $this->createPageWithSubjects( 'Update source', TestSubject::build( label: 'After import' ) ); + $currentXml = $this->exportPageToXml( 'Update source' ); + $this->emptyTheGraph(); + + $this->importXml( str_replace( 'Update source', 'Update target', $outdatedXml ) ); + $this->importXml( str_replace( 'Update source', 'Update target', $currentXml ) ); + + $this->assertSame( + [ 'After import' ], + $this->readSubjectLabels( $this->pageIdOf( 'Update target' ) ), + 'the projection must follow the newly imported revision, not the one it replaced' + ); + } + + public function testImportedPageWithoutSubjectSlotIsNotProjected(): void { + $this->insertPage( 'Plain source', 'Just wikitext, no subjects.' ); + $xml = $this->exportPageToXml( 'Plain source' ); + $this->emptyTheGraph(); + + $this->importXml( str_replace( 'Plain source', 'Plain target', $xml ) ); + + $this->assertSame( 0, $this->countPageNodes( $this->pageIdOf( 'Plain target' ) ) ); + } + + /** + * Stands in for importing into another wiki, whose graph knows nothing of the exported page. Without + * this the source page's own projection stays behind, and since Subject nodes are keyed globally, the + * source and target pages would share one Subject node — leaving the assertions unable to tell a + * subject the import projected from one the source page had already put there. + */ + private function emptyTheGraph(): void { + $this->writeGraph( 'MATCH (n) DETACH DELETE n' ); + } + + private function pageIdOf( string $pageName ): int { + $pageId = Title::newFromText( $pageName )->getArticleID(); + + $this->assertNotSame( 0, $pageId, "the import should have created {$pageName}" ); + + return $pageId; + } + + /** + * @return string[] + */ + private function readSubjectLabels( int $pageId ): array { + $result = $this->readGraph( + 'MATCH (page:Page {id: $pageId})-[:HasSubject]->(subject:Subject) RETURN subject.name AS name ORDER BY name', + [ 'pageId' => $pageId ] + ); + + return array_column( $result->toRecursiveArray(), 'name' ); + } + + private function countPageNodes( int $pageId ): int { + $result = $this->readGraph( + 'MATCH (page:Page {id: $pageId}) RETURN count(page) AS count', + [ 'pageId' => $pageId ] + ); + + return (int)$result->first()->toRecursiveArray()['count']; + } + +} diff --git a/tests/phpunit/NeoWikiIntegrationTestCase.php b/tests/phpunit/NeoWikiIntegrationTestCase.php index a9819fdaa..326e3e0f5 100644 --- a/tests/phpunit/NeoWikiIntegrationTestCase.php +++ b/tests/phpunit/NeoWikiIntegrationTestCase.php @@ -4,9 +4,12 @@ namespace ProfessionalWiki\NeoWiki\Tests; +use DumpStringOutput; +use ImportStringSource; use Laudis\Neo4j\Databags\SummarizedResult; use MediaWiki\CommentStore\CommentStoreComment; use MediaWiki\Content\TextContent; +use MediaWiki\Deferred\DeferredUpdates; use MediaWiki\MediaWikiServices; use MediaWiki\Revision\RevisionRecord; use MediaWiki\Title\Title; @@ -24,6 +27,7 @@ use ProfessionalWiki\NeoWiki\Tests\Data\TestSchema; use ProfessionalWiki\NeoWiki\Tests\Data\TestSubject; use ProfessionalWiki\NeoWiki\Tests\TestDoubles\InMemorySchemaLookup; +use WikiExporter; class NeoWikiIntegrationTestCase extends MediaWikiIntegrationTestCase { @@ -89,6 +93,37 @@ protected function createMapping( string $name, string $json ): ?RevisionRecord return $updater->saveRevision( CommentStoreComment::newUnsavedComment( 'TODO' ) ); } + protected function exportPageToXml( string $pageName ): string { + $exporter = $this->getServiceContainer()->getWikiExporterFactory()->getWikiExporter( + $this->getDb(), + WikiExporter::FULL + ); + + $sink = new DumpStringOutput(); + $exporter->setOutputSink( $sink ); + $exporter->openStream(); + $exporter->pageByName( $pageName ); + $exporter->closeStream(); + + return (string)$sink; + } + + /** + * Imports without a reporter, as importDump.php does. Special:Import and the import API wrap the + * importer in an ImportReporter, which creates a null revision on top of the import and so happens + * to fire RevisionFromEditComplete as well. Importing bare keeps tests on the import path only. + */ + protected function importXml( string $xml ): void { + $importer = $this->getServiceContainer()->getWikiImporterFactory()->getWikiImporter( + new ImportStringSource( $xml ), + $this->getTestSysop()->getAuthority() + ); + + $importer->doImport(); + + DeferredUpdates::doUpdates(); + } + protected function markPageTableAsUsed(): void { if ( !in_array( 'page', $this->tablesUsed ) ) { $this->tablesUsed[] = 'page';