From 5ababb0a88df01b1cc7d13ade5fdce94c55c5c73 Mon Sep 17 00:00:00 2001 From: alistair3149 Date: Wed, 9 Sep 2026 14:44:40 -0400 Subject: [PATCH 1/2] Let approval extensions choose the revision NeoWiki publishes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes https://github.com/ProfessionalWiki/NeoWiki/issues/1379 NeoWiki published each page's latest revision everywhere except the page view, so an approval extension such as ContentStabilization could stabilize what a reader sees on a page but not what the graph stores or the RDF export served. It already registers a Page Property Provider with us and calls `newPageRebuilder()->rebuild()` when a stable point changes; it had no way to say which revision that rebuild should read. Extensions can now register a `RevisionPolicy` through `NeoWikiRegistrar::setRevisionPolicy()`. It answers three questions, each asked where it is natural: - `publishesRevision()` — asked as a revision is written. A save already has its revision and only needs to know whether to publish it, so this costs one lookup. Return false and the graph keeps whatever it published before. - `publishedRevision()` — asked when nothing but the page is known: by `PageRebuilder::rebuild()`, the reprojection path an approval extension calls when its answer changes, and on every Schema, Layout, Mapping and configuration read and every RDF export. Returns the revision to publish, or `null` when the page has nothing publishable. `rebuildFromPrimary()` deliberately does not ask: import and undelete write a revision, so they take the save path's semantics. - `revisionIsReadableBy()` — asked when a caller names a revision by id, which neither publishing method can intercept. A refused revision answers exactly like an absent one, so the sequential revision ids stay unsweepable (#1046). Registration is a single slot rather than the `add*` list every other extension point uses, because two extensions cannot both decide this. A second policy is refused with a warning and the first keeps deciding. With nothing registered every surface reads the latest revision, exactly as before. The registered policy is wrapped in `FailureIsolatingRevisionPolicy`, as every other extension-contributed plugin is. A policy that throws publishes nothing and hides everything — the edit hook fires inside `PageUpdater`'s atomic section, so an uncaught throw there would roll the contributor's save back. It also refuses a revision from another page, and a revision whose text is suppressed: only the current revision was ever projected before, and core forbids suppressing that one, so a policy naming an older revision must not start publishing suppressed Subjects. ## The subject-to-page index is deliberately not governed by the policy It records where a Subject lives, not whether it is published, and it is written from every revision. Every id-keyed read and write addresses its page through it — `ReplaceSubjectAction`, `UpdateStatementAction`, `DeleteSubjectAction`, `MoveSubjectAction`, and the check in `CreateSubjectAction` that an id is not already taken — so a Subject left out of it could not be edited, moved or deleted, and its id would read as free and be re-mintable onto another page. Adding a Subject in an unapproved revision is the normal case on an approval wiki, so that would fire constantly. Keeping the index out of it also makes the maintenance rebuilder correct rather than contradictory: it writes from `page_latest` and needs no knowledge of the policy, which is why it can keep running under `update.php`. ## Which surfaces this covers The graph projection and the RDF export publish through the policy. Schemas, Layouts, Mappings and the on-wiki configuration page read through it too, so an unapproved edit to one does not take effect until it is approved; the Schema editor is unaffected because it loads page source through core's own `/v1/page/` endpoint. Reads by Subject id stay on the latest revision. Routing them through the policy is what an earlier revision of this branch did, and it broke the subject editor: the same endpoint is the editor's load path, so the editor loaded approved values and saved them back over the draft. ## Known gaps, all tracked - Three Subject read surfaces still serve the latest revision — `action=subjects`, the page-keyed REST read, and referenced Subjects on other pages via `PointInTimeSubjectLookup` (https://github.com/ProfessionalWiki/NeoWiki/issues/1390). The parse-time accessors are unchanged for a different reason: they need the parser's own current-revision callback rather than this interface. - The RDF export stops answering for a page once approval is withdrawn, but what was published stays in the graph until the page is deleted; a rebuild does not remove it (https://github.com/ProfessionalWiki/NeoWiki/issues/1391). - Schemas and Mappings cache on the page's latest revision id, so an approval change with no page edit does not take effect until that page is edited or the entry expires; and a Subject is validated against the published Schema while the Schema editor shows the latest one (https://github.com/ProfessionalWiki/NeoWiki/issues/1392). - Undelete and import take the save path, so a restored page whose latest revision is unpublished comes back addressable but absent from the graph until the approval extension reprojects it. ## Considered, omitted - Collapsing the two publishing methods into one nullable substitution. It reads as a reduction, but it forces every draft save to resolve and re-project a revision the graph already holds, and it was what dragged the index and the editor into the design. - Making the index follow the published revision, or hold the union of both. The first breaks editing as described above; the second leaves the maintenance rebuilder unable to remove a row, which is the one thing it exists to do. ## Not in this change The decision itself is not recorded in an ADR. `docs/AGENTS.md` says an ADR is a dated record, not retro-edited apart from status links, so an earlier revision of this branch that appended to ADR 27 and ADR 32 has been reverted. Revision choice wants its own ADR. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01NzPP5bUaN1Nos9CwJoLXzW --- docs/extending/extending.md | 61 ++++++++ maintenance/DumpRdf.php | 2 +- .../FailureIsolatingRevisionPolicy.php | 104 ++++++++++++++ .../GraphRebuild/RebuildProgress.php | 4 +- src/Application/NullRevisionPolicy.php | 29 ++++ src/Application/PageRebuilder.php | 25 +++- src/Application/PageRefreshOutcome.php | 2 + src/Application/Rdf/RdfPageLoader.php | 14 +- src/Application/RevisionPolicy.php | 51 +++++++ src/Application/RevisionPolicyRegistry.php | 45 ++++++ src/EntryPoints/NeoWikiRegistrar.php | 15 ++ src/EntryPoints/OnRevisionCreatedHandler.php | 14 ++ src/EntryPoints/REST/GetSubjectApi.php | 19 ++- src/NeoWikiExtension.php | 35 ++++- .../MediaWiki/PageContentFetcher.php | 14 +- .../FailureIsolatingRevisionPolicyTest.php | 135 ++++++++++++++++++ .../phpunit/Application/PageRebuilderTest.php | 38 ++++- .../PageRefreshWithoutEditTest.php | 49 +++++++ .../Application/Rdf/RdfPageLoaderTest.php | 42 +++++- .../RevisionPolicyRegistryTest.php | 53 +++++++ .../EntryPoints/NeoWikiRegistrarTest.php | 14 ++ .../OnRevisionCreatedHandlerTest.php | 40 +++++- .../EntryPoints/REST/GetSubjectApiTest.php | 55 +++++++ tests/phpunit/NeoWikiIntegrationTestCase.php | 19 ++- .../MediaWiki/PageContentFetcherTest.php | 35 ++++- .../TestDoubles/FixedRevisionPolicy.php | 57 ++++++++ 26 files changed, 947 insertions(+), 24 deletions(-) create mode 100644 src/Application/FailureIsolatingRevisionPolicy.php create mode 100644 src/Application/NullRevisionPolicy.php create mode 100644 src/Application/RevisionPolicy.php create mode 100644 src/Application/RevisionPolicyRegistry.php create mode 100644 tests/phpunit/Application/FailureIsolatingRevisionPolicyTest.php create mode 100644 tests/phpunit/Application/RevisionPolicyRegistryTest.php create mode 100644 tests/phpunit/TestDoubles/FixedRevisionPolicy.php diff --git a/docs/extending/extending.md b/docs/extending/extending.md index 4cec426de..a69e45458 100644 --- a/docs/extending/extending.md +++ b/docs/extending/extending.md @@ -168,10 +168,71 @@ the Page node. No new revision is created. `rebuild()` returns a `PageRefreshOut - `SkippedUnreadableSubjects` — the page's subject slot holds content NeoWiki cannot read as Subjects. - `SkippedUnreadablePageProperties` — the page's properties could not be built, for instance because a provider or the page's own parse threw. +- `SkippedUnpublishableRevision` — a registered revision policy publishes no revision of the page. A graph store that fails is logged and skipped, exactly as on a normal page save; only request timeouts and wiki-database errors throw. +### Choosing which revision NeoWiki publishes + +By default NeoWiki publishes each page's latest revision: that is the revision it projects to the graph stores and +exports as RDF. An approval extension shows readers an approved revision instead, and registers a `RevisionPolicy` so +NeoWiki publishes the same one. + +```php +public function onNeoWikiRegistration( NeoWikiRegistrar $registrar ): void { + $registrar->setRevisionPolicy( new MyApprovalPolicy( $this->approvalLookup ) ); +} +``` + +```php +class MyApprovalPolicy implements RevisionPolicy { + + public function publishesRevision( RevisionRecord $revision ): bool { + return $this->approvalLookup->isApproved( $revision ); + } + + public function publishedRevision( RevisionRecord $revision ): ?RevisionRecord { + return $this->approvalLookup->lastApprovedRevisionOf( $revision->getPage() ); + } + + public function revisionIsReadableBy( RevisionRecord $revision, Authority $viewer ): bool { + return $this->approvalLookup->isApproved( $revision ) + || $viewer->isAllowed( 'my-extension-see-drafts' ); + } + +} +``` + +`publishesRevision()` is asked as a revision is written; return false and the graph keeps whatever it published +before. `publishedRevision()` is asked when a page is reprojected; return the argument to leave the projection alone, +or `null` when the page has nothing publishable. `revisionIsReadableBy()` is asked only when a caller names a revision +itself, which neither publishing method can intercept — a revision it refuses answers exactly like one that does not +exist. + +- **A policy answers for the wiki, not for a viewer.** The graph and the RDF export are one state every reader sees. +- **Only one extension can decide this.** A second policy is refused with a warning in the `NeoWiki` log channel; the + first one keeps deciding. +- **Subject writes read the latest revision**, so a contributor still edits what they last saved. Schemas are the + exception: a Subject is validated against the Schema revision the policy publishes, while the Schema editor shows + the latest one ([#1392](https://github.com/ProfessionalWiki/NeoWiki/issues/1392)). + +Call [`newPageRebuilder()->rebuild( $title )`](#refreshing-a-pages-data-without-an-edit) whenever your extension +changes which revision it approves; nothing else tells NeoWiki the answer has changed. `publishedRevision()` is also +asked on every Schema, Layout and Mapping read and every RDF export, so keep it cheap. + +Two gaps: + +- Schemas and Mappings are read through a cache keyed on the page's latest revision id, so an approval change with + no accompanying page edit does not take effect until that page is edited or the cache entry expires + ([#1392](https://github.com/ProfessionalWiki/NeoWiki/issues/1392)). +- The RDF export stops answering for a page once approval is withdrawn, but what was published stays in the graph + stores until the page is deleted; a rebuild does not remove it + ([#1391](https://github.com/ProfessionalWiki/NeoWiki/issues/1391)). Revocation is not a retraction mechanism. + +Subject reads over REST, and the parse-time accessors, are not yet covered +([#1390](https://github.com/ProfessionalWiki/NeoWiki/issues/1390)). + ### Graph Database Backends NeoWiki currently supports Neo4j only, but the graph projection is an extension point: implement diff --git a/maintenance/DumpRdf.php b/maintenance/DumpRdf.php index 5505a0058..500561104 100644 --- a/maintenance/DumpRdf.php +++ b/maintenance/DumpRdf.php @@ -102,7 +102,7 @@ private function dumpPage( } if ( $page === null ) { - $this->error( "Skipped page $pageId: it no longer exists or its subject slot does not hold Subject data" ); + $this->error( "Skipped page $pageId: it no longer exists, its subject slot does not hold Subject data, or the registered revision policy publishes no revision of it" ); return false; } diff --git a/src/Application/FailureIsolatingRevisionPolicy.php b/src/Application/FailureIsolatingRevisionPolicy.php new file mode 100644 index 000000000..6e95406c3 --- /dev/null +++ b/src/Application/FailureIsolatingRevisionPolicy.php @@ -0,0 +1,104 @@ +isDeleted( RevisionRecord::DELETED_TEXT ) ) { + return false; + } + + try { + return $this->policy->publishesRevision( $revision ); + } catch ( Throwable $exception ) { + $this->logFailure( 'publishesRevision', $revision, $exception ); + return false; + } + } + + public function publishedRevision( RevisionRecord $revision ): ?RevisionRecord { + try { + $published = $this->policy->publishedRevision( $revision ); + } catch ( Throwable $exception ) { + $this->logFailure( 'publishedRevision', $revision, $exception ); + return null; + } + + if ( $published === null ) { + return null; + } + + if ( $published->getPageId() !== $revision->getPageId() ) { + $this->logger->error( + 'The revision policy {class} named revision {published} of page {publishedPage} for page {page}; ' + . 'a policy may only name a revision of the page it was asked about. Publishing nothing for it.', + [ + 'class' => $this->policy::class, + 'published' => $published->getId(), + 'publishedPage' => $published->getPageId(), + 'page' => $revision->getPageId(), + ] + ); + return null; + } + + if ( $published->isDeleted( RevisionRecord::DELETED_TEXT ) ) { + return null; + } + + return $published; + } + + public function revisionIsReadableBy( RevisionRecord $revision, Authority $viewer ): bool { + try { + return $this->policy->revisionIsReadableBy( $revision, $viewer ); + } catch ( Throwable $exception ) { + $this->logFailure( 'revisionIsReadableBy', $revision, $exception ); + return false; + } + } + + private function logFailure( string $method, RevisionRecord $revision, Throwable $exception ): void { + $this->logger->error( + 'The revision policy {class} threw from {method} for revision {revision} of page {page}; ' + . 'treating the page as publishing nothing. {message}', + [ + 'class' => $this->policy::class, + 'method' => $method, + 'revision' => $revision->getId(), + 'page' => $revision->getPageId(), + 'message' => $exception->getMessage(), + 'exception' => $exception, + ] + ); + } + +} diff --git a/src/Application/GraphRebuild/RebuildProgress.php b/src/Application/GraphRebuild/RebuildProgress.php index 691cf02ea..41abcfce9 100644 --- a/src/Application/GraphRebuild/RebuildProgress.php +++ b/src/Application/GraphRebuild/RebuildProgress.php @@ -34,8 +34,8 @@ public function pageProjected( int $pageId ): void { } /** - * The page held no Subject to project after all — it has no current revision, or its latest one - * dropped the subject slot. Nothing failed, and nothing was projected. + * The page was not projected, and nothing failed: it has no current revision, its subject slot + * does not hold Subject data, or the registered revision policy publishes no revision of it. */ public function pageSkipped( int $pageId ): void { $this->cursor = $pageId; diff --git a/src/Application/NullRevisionPolicy.php b/src/Application/NullRevisionPolicy.php new file mode 100644 index 000000000..bbcef8498 --- /dev/null +++ b/src/Application/NullRevisionPolicy.php @@ -0,0 +1,29 @@ +rebuildWithReadFlags( $title, IDBAccessObject::READ_NORMAL ); + return $this->rebuildWithReadFlags( $title, IDBAccessObject::READ_NORMAL, substitute: true ); } /** @@ -27,10 +36,10 @@ public function rebuild( Title $title ): PageRefreshOutcome { * replaced, which would project outdated content. */ public function rebuildFromPrimary( Title $title ): PageRefreshOutcome { - return $this->rebuildWithReadFlags( $title, IDBAccessObject::READ_LATEST ); + return $this->rebuildWithReadFlags( $title, IDBAccessObject::READ_LATEST, substitute: false ); } - private function rebuildWithReadFlags( Title $title, int $readFlags ): PageRefreshOutcome { + private function rebuildWithReadFlags( Title $title, int $readFlags, bool $substitute ): PageRefreshOutcome { $wikiPage = $this->wikiPageFactory->newFromTitle( $title ); $wikiPage->loadPageData( $readFlags ); @@ -40,6 +49,16 @@ private function rebuildWithReadFlags( Title $title, int $readFlags ): PageRefre return PageRefreshOutcome::SkippedMissingRevision; } + if ( $substitute ) { + $published = $this->revisionPolicy->publishedRevision( $revision ); + + if ( $published === null ) { + return PageRefreshOutcome::SkippedUnpublishableRevision; + } + + $revision = $published; + } + return $this->handler->onRevisionCreated( $revision ); } diff --git a/src/Application/PageRefreshOutcome.php b/src/Application/PageRefreshOutcome.php index 13b575164..2f4b10fa7 100644 --- a/src/Application/PageRefreshOutcome.php +++ b/src/Application/PageRefreshOutcome.php @@ -15,6 +15,7 @@ enum PageRefreshOutcome: string { case SkippedMissingRevision = 'skippedMissingRevision'; case SkippedUnreadableSubjects = 'skippedUnreadableSubjects'; case SkippedUnreadablePageProperties = 'skippedUnreadablePageProperties'; + case SkippedUnpublishableRevision = 'skippedUnpublishableRevision'; /** * Why the page was not written, phrased to complete "Skipped : ...". @@ -26,6 +27,7 @@ public function skipReason(): string { self::SkippedMissingRevision => 'no current revision', self::SkippedUnreadableSubjects => 'its subject slot does not hold Subject data', self::SkippedUnreadablePageProperties => 'its page properties could not be built', + self::SkippedUnpublishableRevision => 'the registered revision policy does not publish it', self::Refreshed => throw new LogicException( 'Refreshed is not a skip' ), }; } diff --git a/src/Application/Rdf/RdfPageLoader.php b/src/Application/Rdf/RdfPageLoader.php index ffb46666f..b5522e4b6 100644 --- a/src/Application/Rdf/RdfPageLoader.php +++ b/src/Application/Rdf/RdfPageLoader.php @@ -7,6 +7,7 @@ use MediaWiki\Page\WikiPageFactory; use MediaWiki\Revision\RevisionRecord; use MediaWiki\Title\Title; +use ProfessionalWiki\NeoWiki\Application\RevisionPolicy; use ProfessionalWiki\NeoWiki\Domain\Page\Page; use ProfessionalWiki\NeoWiki\Domain\Page\PageId; use ProfessionalWiki\NeoWiki\Domain\Page\PageSubjects; @@ -15,20 +16,23 @@ use ProfessionalWiki\NeoWiki\Persistence\MediaWiki\Subject\MediaWikiSubjectRepository; /** - * Loads a {@see Page} domain object from a page's current revision so it can be projected to RDF. + * Loads a {@see Page} domain object from the revision the registered revision policy publishes, so it + * can be projected to RDF. * The revision slot is the source of truth (as in OnRevisionCreatedHandler and the graph rebuild), * so the export reflects the stored data rather than the secondary graph projection. * * Every page is exported, with the Subjects it holds and with none when it holds none, which keeps the * RDF surfaces describing the same pages as the graph databases. Returns null when the page does not - * exist, and for the one page state the write path also refuses to touch: a subject slot holding content - * that is not Subject data, which the export must not describe as a page without Subjects. + * exist, when the policy publishes no revision of it, and for the one page state the write path also + * refuses to touch: a subject slot holding content that is not Subject data, which the export must not + * describe as a page without Subjects. */ class RdfPageLoader { public function __construct( private readonly WikiPageFactory $wikiPageFactory, private readonly PagePropertiesBuilder $pagePropertiesBuilder, + private readonly RevisionPolicy $revisionPolicy, ) { } @@ -49,7 +53,9 @@ public function loadByTitle( Title $title ): ?Page { return null; } - return $this->buildPage( $revision ); + $published = $this->revisionPolicy->publishedRevision( $revision ); + + return $published === null ? null : $this->buildPage( $published ); } private function buildPage( RevisionRecord $revision ): ?Page { diff --git a/src/Application/RevisionPolicy.php b/src/Application/RevisionPolicy.php new file mode 100644 index 000000000..b0f5401b2 --- /dev/null +++ b/src/Application/RevisionPolicy.php @@ -0,0 +1,51 @@ +policy !== null ) { + $this->logger->warning( + 'Ignoring the revision policy registered by {class}: a policy is already registered, ' + . 'and only one extension can decide which revision a page publishes.', + [ 'class' => $policy::class ] + ); + return; + } + + $this->policy = $policy; + } + + public function getPolicy(): RevisionPolicy { + return $this->policy ?? new NullRevisionPolicy(); + } + +} diff --git a/src/EntryPoints/NeoWikiRegistrar.php b/src/EntryPoints/NeoWikiRegistrar.php index bb5bcdc20..38f725a6d 100644 --- a/src/EntryPoints/NeoWikiRegistrar.php +++ b/src/EntryPoints/NeoWikiRegistrar.php @@ -4,6 +4,8 @@ namespace ProfessionalWiki\NeoWiki\EntryPoints; +use ProfessionalWiki\NeoWiki\Application\RevisionPolicy; +use ProfessionalWiki\NeoWiki\Application\RevisionPolicyRegistry; use ProfessionalWiki\NeoWiki\Domain\EditNotice\SubjectEditNoticeProvider; use ProfessionalWiki\NeoWiki\Domain\EditNotice\SubjectEditNoticeProviderRegistry; use ProfessionalWiki\NeoWiki\Domain\GraphDatabase\GraphDatabasePlugin; @@ -26,6 +28,7 @@ public function __construct( private GraphDatabasePluginRegistry $graphDatabasePluginRegistry, private RdfValueMapperRegistry $rdfValueMapperRegistry, private SubjectEditNoticeProviderRegistry $subjectEditNoticeProviderRegistry, + private RevisionPolicyRegistry $revisionPolicyRegistry, ) { } @@ -58,6 +61,18 @@ public function addSubjectEditNoticeProvider( SubjectEditNoticeProvider $provide $this->subjectEditNoticeProviderRegistry->addProvider( $provider ); } + /** + * Registers which revision of a page NeoWiki publishes: projects to the graph stores and exports + * as RDF. For approval extensions, which show readers an approved revision rather than the newest + * one. + * + * Only one extension can decide this, so unlike the other registrations this is a single slot: a + * second policy is refused with a warning and the first one keeps deciding. + */ + public function setRevisionPolicy( RevisionPolicy $policy ): void { + $this->revisionPolicyRegistry->setPolicy( $policy ); + } + public function addPagePropertyProvider( PagePropertyProvider $provider ): void { $this->pagePropertyProviderRegistry->addProvider( $provider ); } diff --git a/src/EntryPoints/OnRevisionCreatedHandler.php b/src/EntryPoints/OnRevisionCreatedHandler.php index 51f85d0fb..290c13c07 100644 --- a/src/EntryPoints/OnRevisionCreatedHandler.php +++ b/src/EntryPoints/OnRevisionCreatedHandler.php @@ -6,6 +6,7 @@ use MediaWiki\Revision\RevisionRecord; use ProfessionalWiki\NeoWiki\Application\PageRefreshOutcome; +use ProfessionalWiki\NeoWiki\Application\RevisionPolicy; use ProfessionalWiki\NeoWiki\Domain\GraphDatabase\GraphDatabasePlugin; use ProfessionalWiki\NeoWiki\Domain\Page\Page; use ProfessionalWiki\NeoWiki\Domain\Page\PageId; @@ -23,6 +24,7 @@ public function __construct( private readonly GraphDatabasePlugin $graphDatabasePlugin, private readonly SubjectPageIndex $subjectPageIndex, private readonly PagePropertiesSource $pagePropertiesSource, + private readonly RevisionPolicy $revisionPolicy, private readonly LoggerInterface $logger, ) { } @@ -30,6 +32,11 @@ public function __construct( /** * Indexes which Subjects the page holds, and projects the page with them — and with none when it * holds none: every page gets a Page node, so its Page Properties are queryable. + * + * The index records where a Subject lives and is written for every revision, published or not: + * every id-keyed read and write addresses its page through it, so a Subject missing from it cannot + * be edited, moved or deleted, and its id reads as free. Publishing is decided separately, and only + * for the graph, which is the surface readers query. */ public function onRevisionCreated( RevisionRecord $revisionRecord ): PageRefreshOutcome { if ( $revisionRecord->getPageId() === 0 ) { @@ -67,6 +74,13 @@ private function refreshPage( RevisionRecord $revisionRecord, ?SubjectContent $c // it or not at all, and a Subject too broken to deserialize is still indexed. $this->subjectPageIndex->setSubjectsOfPage( $pageId, $content?->getSubjectIds() ?? [] ); + // Indexed either way, projected only when published: what the graph already holds is what the + // policy last published, and leaving it there is the point. Withdrawing it belongs to page + // deletion, not to somebody saving a draft on a page that has nothing published yet. + if ( !$this->revisionPolicy->publishesRevision( $revisionRecord ) ) { + return PageRefreshOutcome::SkippedUnpublishableRevision; + } + $subjects = $content?->getPageSubjects() ?? PageSubjects::newEmpty(); // Null only from the isolating source the hook path is given, which has already logged the diff --git a/src/EntryPoints/REST/GetSubjectApi.php b/src/EntryPoints/REST/GetSubjectApi.php index 393dbaf0b..dd42dff8b 100644 --- a/src/EntryPoints/REST/GetSubjectApi.php +++ b/src/EntryPoints/REST/GetSubjectApi.php @@ -49,10 +49,10 @@ private function newGetSubjectQuery( RestGetSubjectPresenter $presenter, ?int $r $revision = MediaWikiServices::getInstance()->getRevisionLookup()->getRevisionById( $revisionId ); - // A revision on an unreadable page answers exactly like a nonexistent revision: - // revision ids are sequential, so any distinguishable answer is a sweepable - // existence oracle over restricted pages (#1046). - if ( $revision === null || !$this->revisionPageIsReadable( $revision->getPageId() ) ) { + // A revision the viewer may not see answers exactly like a nonexistent one: revision ids are + // sequential, so any distinguishable answer is a sweepable existence oracle over restricted + // pages (#1046), and over the unapproved revisions an approval extension hides. + if ( $revision === null || !$this->revisionIsReadable( $revision ) ) { return $this->getResponseFactory()->createHttpError( 404, [ 'message' => 'Revision not found: ' . $revisionId, ] ); @@ -61,6 +61,17 @@ private function newGetSubjectQuery( RestGetSubjectPresenter $presenter, ?int $r return NeoWikiExtension::getInstance()->newGetSubjectQueryForRevision( $presenter, $revision, $this->getAuthority() ); } + /** + * A caller who names a revision is asking to see that one, so the registered revision policy is + * asked directly whether this viewer may. Without one registered every revision of a readable page + * is readable, as before. + */ + private function revisionIsReadable( RevisionRecord $revision ): bool { + return $this->revisionPageIsReadable( $revision->getPageId() ) + && NeoWikiExtension::getInstance()->getRevisionPolicy() + ->revisionIsReadableBy( $revision, $this->getAuthority() ); + } + private function revisionPageIsReadable( int $pageId ): bool { return NeoWikiExtension::getInstance() ->newPageReadAuthorizer( $this->getAuthority() ) diff --git a/src/NeoWikiExtension.php b/src/NeoWikiExtension.php index cd73ac28d..603b9e018 100644 --- a/src/NeoWikiExtension.php +++ b/src/NeoWikiExtension.php @@ -83,6 +83,9 @@ use ProfessionalWiki\NeoWiki\Application\SubjectWriteAuthorizer; use ProfessionalWiki\NeoWiki\Application\LastEditorPagesRebuilder; use ProfessionalWiki\NeoWiki\Application\PageRebuilder; +use ProfessionalWiki\NeoWiki\Application\FailureIsolatingRevisionPolicy; +use ProfessionalWiki\NeoWiki\Application\RevisionPolicy; +use ProfessionalWiki\NeoWiki\Application\RevisionPolicyRegistry; use ProfessionalWiki\NeoWiki\Application\SubjectIdMinter; use ProfessionalWiki\NeoWiki\Application\SubjectRepository; use ProfessionalWiki\NeoWiki\Application\SubjectResolver; @@ -225,6 +228,7 @@ class NeoWikiExtension { private CompositeGraphDatabasePlugin $graphDatabasePlugin; private CompositeGraphDatabasePlugin $isolatingGraphDatabasePlugin; private GraphDatabasePluginRegistry $graphDatabasePluginRegistry; + private RevisionPolicyRegistry $revisionPolicyRegistry; private ?Neo4jPlugin $neo4jPlugin = null; /** @var array|null Keys are store names */ private ?array $sparqlPlugins = null; @@ -305,6 +309,23 @@ public function getRdfValueMapperRegistry(): RdfValueMapperRegistry { return $this->rdfValueMapperRegistry; } + public function getRevisionPolicyRegistry(): RevisionPolicyRegistry { + if ( !isset( $this->revisionPolicyRegistry ) ) { + $this->revisionPolicyRegistry = new RevisionPolicyRegistry( LoggerFactory::getInstance( 'NeoWiki' ) ); + } + + $this->ensureExtensionsRegistered(); + + return $this->revisionPolicyRegistry; + } + + public function getRevisionPolicy(): RevisionPolicy { + return new FailureIsolatingRevisionPolicy( + $this->getRevisionPolicyRegistry()->getPolicy(), + LoggerFactory::getInstance( 'NeoWiki' ) + ); + } + private function ensureExtensionsRegistered(): void { if ( $this->extensionsRegistered ) { return; @@ -321,6 +342,7 @@ private function ensureExtensionsRegistered(): void { $this->getGraphDatabasePluginRegistry(), $this->getRdfValueMapperRegistry(), $this->getSubjectEditNoticeProviderRegistry(), + $this->getRevisionPolicyRegistry(), ) ] ); } @@ -390,6 +412,11 @@ public function getStoreContentUC(): OnRevisionCreatedHandler { * on. */ private function newRebuildStoreContentHandler(): OnRevisionCreatedHandler { + // The null index is load-bearing, not merely unneeded: PageRebuilder::rebuild() hands this handler + // the revision the policy publishes, and the handler indexes whatever it is given. A real index + // here would replace the latest revision's Subject set with the published one, making every + // Subject a draft added unaddressable — the exact failure keeping the index out of the policy + // exists to prevent. Only rebuildFromPrimary(), which does not substitute, may index. return $this->newStoreContentHandler( $this->getGraphDatabasePlugin(), new NullSubjectPageIndex(), @@ -406,6 +433,7 @@ private function newStoreContentHandler( $graphDatabasePlugin, $subjectPageIndex, $pagePropertiesSource, + $this->getRevisionPolicy(), LoggerFactory::getInstance( 'NeoWiki' ), ); } @@ -446,6 +474,7 @@ public function newRdfPageLoader(): RdfPageLoader { return new RdfPageLoader( MediaWikiServices::getInstance()->getWikiPageFactory(), $this->getPagePropertiesBuilder(), + $this->getRevisionPolicy(), ); } @@ -982,7 +1011,8 @@ public function isConfigPage( Title $title ): bool { public function getPageContentFetcher(): PageContentFetcher { return new PageContentFetcher( MediaWikiServices::getInstance()->getTitleParser(), - MediaWikiServices::getInstance()->getRevisionLookup() + MediaWikiServices::getInstance()->getRevisionLookup(), + $this->getRevisionPolicy() ); } @@ -1084,7 +1114,8 @@ public function newHookPageRebuilder(): PageRebuilder { private function newPageRebuilderWith( OnRevisionCreatedHandler $handler ): PageRebuilder { return new PageRebuilder( $handler, - MediaWikiServices::getInstance()->getWikiPageFactory() + MediaWikiServices::getInstance()->getWikiPageFactory(), + $this->getRevisionPolicy() ); } diff --git a/src/Persistence/MediaWiki/PageContentFetcher.php b/src/Persistence/MediaWiki/PageContentFetcher.php index a42274dc6..df7ff2468 100644 --- a/src/Persistence/MediaWiki/PageContentFetcher.php +++ b/src/Persistence/MediaWiki/PageContentFetcher.php @@ -14,12 +14,23 @@ use MediaWiki\Title\Title; use MediaWiki\Title\TitleParser; use MediaWiki\Title\TitleValue; +use ProfessionalWiki\NeoWiki\Application\RevisionPolicy; +/** + * The content a page publishes, which is its latest revision unless a registered revision policy + * substitutes another. Everything read through here is configuration the whole wiki reads — Schemas, + * Layouts, Mappings and the on-wiki configuration page — so on a wiki running an approval extension + * an unapproved edit to one does not take effect until it is approved. + * + * Editing surfaces do not read through here: the Schema editor loads the page source from core's own + * REST endpoint, so it still shows and saves over the latest revision. + */ class PageContentFetcher { public function __construct( private readonly TitleParser $titleParser, - private readonly RevisionLookup $revisionLookup + private readonly RevisionLookup $revisionLookup, + private readonly RevisionPolicy $revisionPolicy ) { } @@ -36,6 +47,7 @@ public function getPageContent( } $revision = $this->revisionLookup->getRevisionByTitle( Title::newFromLinkTarget( $titleValue ) ); + $revision = $revision === null ? null : $this->revisionPolicy->publishedRevision( $revision ); try { return $revision?->getContent( $slotName, RevisionRecord::FOR_THIS_USER, $authority ); diff --git a/tests/phpunit/Application/FailureIsolatingRevisionPolicyTest.php b/tests/phpunit/Application/FailureIsolatingRevisionPolicyTest.php new file mode 100644 index 000000000..bc4fb09d8 --- /dev/null +++ b/tests/phpunit/Application/FailureIsolatingRevisionPolicyTest.php @@ -0,0 +1,135 @@ +newRevision( pageId: self::PAGE_ID, id: 10 ); + $published = $this->newRevision( pageId: self::PAGE_ID, id: 9 ); + $policy = $this->isolate( FixedRevisionPolicy::publishing( $published ) ); + + $this->assertTrue( $policy->publishesRevision( $revision ) ); + $this->assertSame( $published, $policy->publishedRevision( $revision ) ); + $this->assertTrue( $policy->revisionIsReadableBy( $revision, $this->createStub( Authority::class ) ) ); + } + + public function testAThrowingPolicyPublishesNothing(): void { + $policy = $this->isolate( $this->newThrowingPolicy() ); + $revision = $this->newRevision( pageId: self::PAGE_ID, id: 10 ); + + $this->assertFalse( $policy->publishesRevision( $revision ) ); + $this->assertNull( $policy->publishedRevision( $revision ) ); + } + + public function testAThrowingPolicyHidesEveryRevision(): void { + $policy = $this->isolate( $this->newThrowingPolicy() ); + + $this->assertFalse( $policy->revisionIsReadableBy( + $this->newRevision( pageId: self::PAGE_ID, id: 10 ), + $this->createStub( Authority::class ) + ) ); + } + + public function testAThrowingPolicyIsLogged(): void { + $logger = new TestLogger(); + + ( new FailureIsolatingRevisionPolicy( $this->newThrowingPolicy(), $logger ) ) + ->publishesRevision( $this->newRevision( pageId: self::PAGE_ID, id: 10 ) ); + + $this->assertTrue( $logger->hasErrorRecords() ); + } + + public function testARevisionOfAnotherPageIsRefused(): void { + $logger = new TestLogger(); + $policy = new FailureIsolatingRevisionPolicy( + $this->newPolicyNaming( $this->newRevision( pageId: self::OTHER_PAGE_ID, id: 9 ) ), + $logger + ); + + $published = $policy->publishedRevision( $this->newRevision( pageId: self::PAGE_ID, id: 10 ) ); + + $this->assertNull( $published ); + $this->assertTrue( $logger->hasErrorRecords(), 'a contract violation is logged' ); + } + + public function testASuppressedRevisionIsNeverPublished(): void { + $suppressed = $this->newRevision( pageId: self::PAGE_ID, id: 9, textSuppressed: true ); + $policy = $this->isolate( FixedRevisionPolicy::publishing( $suppressed ) ); + + $this->assertNull( $policy->publishedRevision( $this->newRevision( pageId: self::PAGE_ID, id: 10 ) ) ); + $this->assertFalse( $policy->publishesRevision( $suppressed ) ); + } + + public function testANullAnswerStaysNull(): void { + $policy = $this->isolate( FixedRevisionPolicy::publishingNothing() ); + + $this->assertNull( $policy->publishedRevision( $this->newRevision( pageId: self::PAGE_ID, id: 10 ) ) ); + } + + private function isolate( RevisionPolicy $policy ): FailureIsolatingRevisionPolicy { + return new FailureIsolatingRevisionPolicy( $policy, new NullLogger() ); + } + + /** + * A policy that breaks the contract by naming the same revision for every page it is asked about. + * FixedRevisionPolicy answers per page, as a real extension does, so it cannot stand in here. + */ + private function newPolicyNaming( RevisionRecord $published ): RevisionPolicy { + return new class( $published ) extends NullRevisionPolicy { + public function __construct( private readonly RevisionRecord $published ) { + } + + public function publishedRevision( RevisionRecord $revision ): ?RevisionRecord { + return $this->published; + } + }; + } + + private function newThrowingPolicy(): RevisionPolicy { + return new class extends NullRevisionPolicy { + public function publishesRevision( RevisionRecord $revision ): bool { + throw new RuntimeException( 'approval table missing' ); + } + + public function publishedRevision( RevisionRecord $revision ): ?RevisionRecord { + throw new RuntimeException( 'approval table missing' ); + } + + public function revisionIsReadableBy( RevisionRecord $revision, Authority $viewer ): bool { + throw new RuntimeException( 'approval table missing' ); + } + }; + } + + private function newRevision( int $pageId, int $id, bool $textSuppressed = false ): RevisionRecord { + $revision = $this->createStub( RevisionRecord::class ); + $revision->method( 'getPageId' )->willReturn( $pageId ); + $revision->method( 'getId' )->willReturn( $id ); + $revision->method( 'isDeleted' )->willReturnCallback( + static fn ( int $field ): bool => $textSuppressed && ( $field & RevisionRecord::DELETED_TEXT ) !== 0 + ); + + return $revision; + } + +} diff --git a/tests/phpunit/Application/PageRebuilderTest.php b/tests/phpunit/Application/PageRebuilderTest.php index 8c5fc6394..bf011bf86 100644 --- a/tests/phpunit/Application/PageRebuilderTest.php +++ b/tests/phpunit/Application/PageRebuilderTest.php @@ -10,7 +10,10 @@ use MediaWiki\Title\Title; use PHPUnit\Framework\TestCase; use ProfessionalWiki\NeoWiki\Application\PageRefreshOutcome; +use ProfessionalWiki\NeoWiki\Application\NullRevisionPolicy; +use ProfessionalWiki\NeoWiki\Application\RevisionPolicy; use ProfessionalWiki\NeoWiki\Application\PageRebuilder; +use ProfessionalWiki\NeoWiki\Tests\TestDoubles\FixedRevisionPolicy; use ProfessionalWiki\NeoWiki\Tests\TestDoubles\SpyOnRevisionCreatedHandler; use Wikimedia\Rdbms\IDBAccessObject; use WikiPage; @@ -111,7 +114,38 @@ public function testRebuildFromPrimaryReadsPageStateFromPrimary(): void { ); } - private function newRebuilder( ?RevisionRecord $revision ): PageRebuilder { + public function testRebuildProjectsTheRevisionThePolicyPublishes(): void { + $latest = $this->newRevision(); + $published = $this->newRevision(); + + $this->newRebuilder( $latest, FixedRevisionPolicy::publishing( $published ) ) + ->rebuild( Title::makeTitle( NS_MAIN, 'Reprojected page' ) ); + + $this->assertSame( [ $published ], $this->handler->calls ); + } + + public function testRebuildWritesNothingWhenThePolicyPublishesNoRevision(): void { + $outcome = $this->newRebuilder( $this->newRevision(), FixedRevisionPolicy::publishingNothing() ) + ->rebuild( Title::makeTitle( NS_MAIN, 'Page with nothing published' ) ); + + $this->assertSame( PageRefreshOutcome::SkippedUnpublishableRevision, $outcome ); + $this->assertSame( [], $this->handler->calls ); + } + + /** + * An import or undelete writes a revision rather than reprojecting a page, so it takes the same + * path a save does: the handler is handed what was written and decides whether to publish it. + */ + public function testRebuildFromPrimaryDoesNotSubstitute(): void { + $written = $this->newRevision(); + + $this->newRebuilder( $written, FixedRevisionPolicy::publishing( $this->newRevision() ) ) + ->rebuildFromPrimary( Title::makeTitle( NS_MAIN, 'Imported page' ) ); + + $this->assertSame( [ $written ], $this->handler->calls ); + } + + private function newRebuilder( ?RevisionRecord $revision, ?RevisionPolicy $policy = null ): PageRebuilder { $page = $this->createStub( WikiPage::class ); $page->method( 'getRevisionRecord' )->willReturn( $revision ); $page->method( 'loadPageData' )->willReturnCallback( @@ -123,7 +157,7 @@ function ( int $from ): void { $factory = $this->createStub( WikiPageFactory::class ); $factory->method( 'newFromTitle' )->willReturn( $page ); - return new PageRebuilder( $this->handler, $factory ); + return new PageRebuilder( $this->handler, $factory, $policy ?? new NullRevisionPolicy() ); } private function newRevision(): RevisionRecord { diff --git a/tests/phpunit/Application/PageRefreshWithoutEditTest.php b/tests/phpunit/Application/PageRefreshWithoutEditTest.php index 97100ce50..a100f865a 100644 --- a/tests/phpunit/Application/PageRefreshWithoutEditTest.php +++ b/tests/phpunit/Application/PageRefreshWithoutEditTest.php @@ -9,6 +9,7 @@ use ProfessionalWiki\NeoWiki\NeoWikiExtension; use ProfessionalWiki\NeoWiki\Tests\Data\TestSubject; use ProfessionalWiki\NeoWiki\Tests\NeoWikiIntegrationTestCase; +use ProfessionalWiki\NeoWiki\Tests\TestDoubles\FixedRevisionPolicy; use ProfessionalWiki\NeoWiki\Tests\TestDoubles\MutablePagePropertyProvider; /** @@ -53,6 +54,45 @@ public function testRefreshUpdatesThePagePropertiesOfAPageWithoutSubjects(): voi $this->assertSame( 'approved', $this->readApprovalState( $pageId ) ); } + /** + * Through NeoWikiExtension's own wiring, not a hand-built handler: the registered policy has to reach + * both PageRebuilder and the handler behind it, or a rebuild after approval would project the draft. + */ + public function testRefreshProjectsTheRevisionTheRegisteredPolicyPublishes(): void { + $approved = $this->createPageWithSubjects( self::PAGE_NAME, TestSubject::build() ); + $afterFirstSave = $this->readGraph( 'MATCH (s:Subject) RETURN count(s) AS n', [] )->first()->toRecursiveArray()['n']; + $this->createPageWithSubjects( self::PAGE_NAME ); + $afterDraft = $this->readGraph( 'MATCH (s:Subject) RETURN count(s) AS n', [] )->first()->toRecursiveArray()['n']; + $this->registerRevisionPolicy( FixedRevisionPolicy::publishing( $approved ) ); + $contentHasSubjects = $approved->getSlots()->getContent( 'neo' )->getPageSubjects()->hasSubjects(); + fwrite( STDERR, "\nDEBUG2 afterFirstSave=$afterFirstSave afterDraft=$afterDraft approvedContentHasSubjects=" . var_export( $contentHasSubjects, true ) . "\n" ); + + $outcome = $this->refreshPage(); + + $this->assertSame( PageRefreshOutcome::Refreshed, $outcome ); + // DEBUG + $dump = $this->readGraph( + 'MATCH (page:Page {id: $pageId}) OPTIONAL MATCH (page)-[r]->(n) RETURN page.id AS id, page.wiki_id AS wiki, collect(type(r)) AS rels, collect(labels(n)) AS targets', + [ 'pageId' => $approved->getPageId() ] + ); + $all = $this->readGraph( 'MATCH (s:Subject) RETURN s.id AS id, labels(s) AS labels', [] ); + fwrite( STDERR, "\nDEBUG approvedRev=" . $approved->getId() . " page=" . $approved->getPageId() + . " hasSlot=" . var_export( $approved->hasSlot( 'neo' ), true ) + . " dump=" . json_encode( $dump->first()?->toRecursiveArray() ) + . " subjects=" . json_encode( array_map( static fn ( $r ) => $r->toRecursiveArray(), iterator_to_array( $all ) ) ) . "\n" ); + $this->assertTrue( + $this->pageHoldsSubjectInGraph( $approved->getPageId() ), + 'the approved revision holds a Subject the draft removed' + ); + } + + public function testRefreshWritesNothingWhenTheRegisteredPolicyPublishesNoRevision(): void { + $this->createPageWithSubjects( self::PAGE_NAME, TestSubject::build() ); + $this->registerRevisionPolicy( FixedRevisionPolicy::publishingNothing() ); + + $this->assertSame( PageRefreshOutcome::SkippedUnpublishableRevision, $this->refreshPage() ); + } + public function testRefreshOfAMissingPageWritesNothing(): void { $outcome = $this->refreshPage(); @@ -65,6 +105,15 @@ private function refreshPage(): PageRefreshOutcome { ->rebuild( Title::newFromText( self::PAGE_NAME ) ); } + private function pageHoldsSubjectInGraph( int $pageId ): bool { + $result = $this->readGraph( + 'MATCH (page:Page {id: $pageId})-[:HasSubject]->(subject:Subject) RETURN count(subject) AS subjects', + [ 'pageId' => $pageId ] + ); + + return ( $result->first()->toRecursiveArray()['subjects'] ?? 0 ) > 0; + } + private function readApprovalState( int $pageId ): ?string { $result = $this->readGraph( 'MATCH (page:Page {id: $pageId}) RETURN page.approvalState AS approvalState', diff --git a/tests/phpunit/Application/Rdf/RdfPageLoaderTest.php b/tests/phpunit/Application/Rdf/RdfPageLoaderTest.php index 0102dff6a..0844c6d47 100644 --- a/tests/phpunit/Application/Rdf/RdfPageLoaderTest.php +++ b/tests/phpunit/Application/Rdf/RdfPageLoaderTest.php @@ -9,10 +9,14 @@ use MediaWiki\Revision\RevisionRecord; use MediaWiki\Revision\RevisionSlots; use MediaWiki\Title\Title; +use ProfessionalWiki\NeoWiki\Application\NullRevisionPolicy; use ProfessionalWiki\NeoWiki\Application\Rdf\RdfPageLoader; +use ProfessionalWiki\NeoWiki\Application\RevisionPolicy; use ProfessionalWiki\NeoWiki\NeoWikiExtension; use ProfessionalWiki\NeoWiki\Persistence\MediaWiki\Subject\MediaWikiSubjectRepository; +use ProfessionalWiki\NeoWiki\Tests\Data\TestSubject; use ProfessionalWiki\NeoWiki\Tests\NeoWikiIntegrationTestCase; +use ProfessionalWiki\NeoWiki\Tests\TestDoubles\FixedRevisionPolicy; use WikiPage; /** @@ -37,6 +41,36 @@ public function testMissingPageIsNotLoaded(): void { $this->assertNull( $loader->loadByTitle( Title::makeTitle( NS_MAIN, 'Page that does not exist' ) ) ); } + public function testExportsTheRevisionTheRevisionPolicyPublishes(): void { + $this->createSchema( TestSubject::DEFAULT_SCHEMA_ID ); + $approved = $this->createPageWithSubjects( 'Exported from its approved revision', TestSubject::build() ); + $draft = $this->createPageWithSubjects( 'Exported from its approved revision' ); + + $page = $this->newLoaderFor( $draft, FixedRevisionPolicy::publishing( $approved ) ) + ->loadByTitle( Title::makeTitle( NS_MAIN, 'Exported from its approved revision' ) ); + + $this->assertNotNull( $page ); + $this->assertTrue( + $page->getSubjects()->hasSubjects(), + 'the approved revision holds a Subject the draft removed' + ); + } + + public function testPageWithNoPublishableRevisionIsNotLoaded(): void { + // The revision must be one that would otherwise load, or the null proves nothing: a slot that + // does not hold Subject data already yields null through the pass-through path. + $this->createSchema( TestSubject::DEFAULT_SCHEMA_ID ); + $revision = $this->createPageWithSubjects( 'Page with nothing published', TestSubject::build() ); + + $loader = $this->newLoaderFor( $revision, FixedRevisionPolicy::publishingNothing() ); + + $this->assertNull( $loader->loadByTitle( Title::makeTitle( NS_MAIN, 'Page with nothing published' ) ) ); + $this->assertNotNull( + $this->newLoaderFor( $revision )->loadByTitle( Title::makeTitle( NS_MAIN, 'Page with nothing published' ) ), + 'the same revision loads when the policy publishes it' + ); + } + private function newRevisionWithSubjectSlotContent(): RevisionRecord { $slots = $this->createStub( RevisionSlots::class ); $slots->method( 'getContent' )->willReturn( new WikitextContent( 'Not Subject data.' ) ); @@ -50,14 +84,18 @@ private function newRevisionWithSubjectSlotContent(): RevisionRecord { return $revision; } - private function newLoaderFor( ?RevisionRecord $revision ): RdfPageLoader { + private function newLoaderFor( ?RevisionRecord $revision, ?RevisionPolicy $policy = null ): RdfPageLoader { $page = $this->createStub( WikiPage::class ); $page->method( 'getRevisionRecord' )->willReturn( $revision ); $factory = $this->createStub( WikiPageFactory::class ); $factory->method( 'newFromTitle' )->willReturn( $page ); - return new RdfPageLoader( $factory, NeoWikiExtension::getInstance()->getPagePropertiesBuilder() ); + return new RdfPageLoader( + $factory, + NeoWikiExtension::getInstance()->getPagePropertiesBuilder(), + $policy ?? new NullRevisionPolicy() + ); } } diff --git a/tests/phpunit/Application/RevisionPolicyRegistryTest.php b/tests/phpunit/Application/RevisionPolicyRegistryTest.php new file mode 100644 index 000000000..88e69409b --- /dev/null +++ b/tests/phpunit/Application/RevisionPolicyRegistryTest.php @@ -0,0 +1,53 @@ +assertInstanceOf( NullRevisionPolicy::class, ( new RevisionPolicyRegistry() )->getPolicy() ); + } + + public function testUsesTheRegisteredPolicy(): void { + $policy = FixedRevisionPolicy::publishingNothing(); + $registry = new RevisionPolicyRegistry(); + + $registry->setPolicy( $policy ); + + $this->assertSame( $policy, $registry->getPolicy() ); + } + + public function testKeepsTheFirstPolicyWhenASecondIsRegistered(): void { + $first = FixedRevisionPolicy::publishingNothing(); + $registry = new RevisionPolicyRegistry(); + + $registry->setPolicy( $first ); + $registry->setPolicy( FixedRevisionPolicy::publishingNothing() ); + + $this->assertSame( $first, $registry->getPolicy() ); + } + + public function testWarnsWhenASecondPolicyIsRegistered(): void { + $logger = new TestLogger(); + $registry = new RevisionPolicyRegistry( $logger ); + + $registry->setPolicy( FixedRevisionPolicy::publishingNothing() ); + $this->assertFalse( $logger->hasWarningRecords(), 'the first registration is not a warning' ); + + $registry->setPolicy( FixedRevisionPolicy::publishingNothing() ); + + $this->assertCount( 1, $logger->records ); + } + +} diff --git a/tests/phpunit/EntryPoints/NeoWikiRegistrarTest.php b/tests/phpunit/EntryPoints/NeoWikiRegistrarTest.php index 665bcdc9f..0b46ea832 100644 --- a/tests/phpunit/EntryPoints/NeoWikiRegistrarTest.php +++ b/tests/phpunit/EntryPoints/NeoWikiRegistrarTest.php @@ -11,6 +11,8 @@ use ProfessionalWiki\NeoWiki\Domain\PropertyType\PropertyTypeRegistry; use ProfessionalWiki\NeoWiki\Domain\PropertyType\Types\TextType; use ProfessionalWiki\NeoWiki\Domain\Rdf\RdfValueMapperRegistry; +use ProfessionalWiki\NeoWiki\Application\RevisionPolicyRegistry; +use ProfessionalWiki\NeoWiki\Tests\TestDoubles\FixedRevisionPolicy; use ProfessionalWiki\NeoWiki\EntryPoints\NeoWikiRegistrar; use ProfessionalWiki\NeoWiki\GraphDatabasePlugins\Neo4j\Persistence\Neo4jValueBuilderRegistry; use ProfessionalWiki\NeoWiki\Tests\TestDoubles\SpyGraphDatabasePlugin; @@ -79,6 +81,16 @@ public function testAddSubjectEditNoticeProviderRegistersInRegistry(): void { $this->assertSame( [ $provider ], $noticeRegistry->getProviders() ); } + public function testRegistersTheRevisionPolicy(): void { + $policyRegistry = new RevisionPolicyRegistry(); + $registrar = $this->newRegistrar( revisionPolicyRegistry: $policyRegistry ); + $policy = FixedRevisionPolicy::publishingNothing(); + + $registrar->setRevisionPolicy( $policy ); + + $this->assertSame( $policy, $policyRegistry->getPolicy() ); + } + private function newRegistrar( ?PropertyTypeRegistry $propertyTypeRegistry = null, ?Neo4jValueBuilderRegistry $valueBuilderRegistry = null, @@ -86,6 +98,7 @@ private function newRegistrar( ?GraphDatabasePluginRegistry $graphDatabasePluginRegistry = null, ?RdfValueMapperRegistry $rdfValueMapperRegistry = null, ?SubjectEditNoticeProviderRegistry $subjectEditNoticeProviderRegistry = null, + ?RevisionPolicyRegistry $revisionPolicyRegistry = null, ): NeoWikiRegistrar { return new NeoWikiRegistrar( propertyTypeRegistry: $propertyTypeRegistry ?? new PropertyTypeRegistry(), @@ -94,6 +107,7 @@ private function newRegistrar( graphDatabasePluginRegistry: $graphDatabasePluginRegistry ?? new GraphDatabasePluginRegistry(), rdfValueMapperRegistry: $rdfValueMapperRegistry ?? new RdfValueMapperRegistry(), subjectEditNoticeProviderRegistry: $subjectEditNoticeProviderRegistry ?? new SubjectEditNoticeProviderRegistry(), + revisionPolicyRegistry: $revisionPolicyRegistry ?? new RevisionPolicyRegistry(), ); } diff --git a/tests/phpunit/EntryPoints/OnRevisionCreatedHandlerTest.php b/tests/phpunit/EntryPoints/OnRevisionCreatedHandlerTest.php index 26092f675..7a138dba8 100644 --- a/tests/phpunit/EntryPoints/OnRevisionCreatedHandlerTest.php +++ b/tests/phpunit/EntryPoints/OnRevisionCreatedHandlerTest.php @@ -9,7 +9,9 @@ use MediaWiki\Revision\RevisionAccessException; use MediaWiki\Revision\RevisionRecord; use MediaWiki\Revision\RevisionSlots; +use ProfessionalWiki\NeoWiki\Application\NullRevisionPolicy; use ProfessionalWiki\NeoWiki\Application\PageRefreshOutcome; +use ProfessionalWiki\NeoWiki\Application\RevisionPolicy; use ProfessionalWiki\NeoWiki\Domain\GraphDatabase\FailureIsolatingGraphDatabasePlugin; use ProfessionalWiki\NeoWiki\Domain\GraphDatabase\GraphDatabasePlugin; use ProfessionalWiki\NeoWiki\Domain\Page\PageId; @@ -21,6 +23,7 @@ use ProfessionalWiki\NeoWiki\PagePropertiesBuilder; use ProfessionalWiki\NeoWiki\Tests\Data\TestSubject; use ProfessionalWiki\NeoWiki\Tests\NeoWikiIntegrationTestCase; +use ProfessionalWiki\NeoWiki\Tests\TestDoubles\FixedRevisionPolicy; use ProfessionalWiki\NeoWiki\Tests\TestDoubles\SpyGraphDatabasePlugin; use ProfessionalWiki\NeoWiki\Tests\TestDoubles\SpySubjectPageIndex; use ProfessionalWiki\NeoWiki\Tests\TestDoubles\ThrowingGraphDatabasePlugin; @@ -189,6 +192,34 @@ public function testGraphDeletionHappensEvenWhenTheIndexRemovalFails(): void { $this->assertEquals( [ new PageId( self::DELETED_PAGE_ID ) ], $this->graphStore->deletedPageIds ); } + public function testDoesNotProjectARevisionThePolicyDoesNotPublish(): void { + $revision = $this->createPageWithSubjects( 'Page with an unpublished revision', TestSubject::build() ); + + $outcome = $this->newHandlerWithPolicy( FixedRevisionPolicy::publishingNothing() ) + ->onRevisionCreated( $revision ); + + $this->assertSame( PageRefreshOutcome::SkippedUnpublishableRevision, $outcome ); + $this->assertSame( [], $this->graphStore->savedPages ); + $this->assertSame( [], $this->graphStore->deletedPageIds, 'what was published stays published' ); + } + + /** + * The index says where a Subject lives, not whether it is published. Every id-keyed read and write + * addresses its page through it, so a Subject left out of it cannot be edited, moved or deleted, + * and its id reads as free. + */ + public function testIndexesASubjectEvenWhenItsRevisionIsNotPublished(): void { + $revision = $this->createPageWithSubjects( 'Page whose draft adds a subject', TestSubject::build() ); + + $this->newHandlerWithPolicy( FixedRevisionPolicy::publishingNothing() ) + ->onRevisionCreated( $revision ); + + $this->assertSame( + [ $revision->getPageId() => [ TestSubject::ZERO_GUID ] ], + $this->subjectPageIndex->indexedSubjectsByPageId + ); + } + private function newFailingProviderRegistry(): PagePropertyProviderRegistry { $registry = new PagePropertyProviderRegistry(); $registry->addProvider( new class implements PagePropertyProvider { @@ -224,10 +255,15 @@ private function newHandler(): OnRevisionCreatedHandler { return $this->newHandlerWith( $this->graphStore ); } + private function newHandlerWithPolicy( RevisionPolicy $policy ): OnRevisionCreatedHandler { + return $this->newHandlerWith( $this->graphStore, revisionPolicy: $policy ); + } + private function newHandlerWith( GraphDatabasePlugin $graphStore, ?PagePropertyProviderRegistry $providerRegistry = null, - bool $isolatePageProperties = false + bool $isolatePageProperties = false, + ?RevisionPolicy $revisionPolicy = null ): OnRevisionCreatedHandler { $pageProperties = $this->newPagePropertiesBuilder( $providerRegistry ?? new PagePropertyProviderRegistry() ); @@ -237,6 +273,7 @@ private function newHandlerWith( $isolatePageProperties ? new FailureIsolatingPagePropertiesSource( $pageProperties, $this->logger ) : $pageProperties, + $revisionPolicy ?? new NullRevisionPolicy(), $this->logger ); } @@ -246,6 +283,7 @@ private function newHandlerWithFailingIndex(): OnRevisionCreatedHandler { $this->graphStore, new ThrowingSubjectPageIndex(), $this->newPagePropertiesBuilder( new PagePropertyProviderRegistry() ), + new NullRevisionPolicy(), $this->logger ); } diff --git a/tests/phpunit/EntryPoints/REST/GetSubjectApiTest.php b/tests/phpunit/EntryPoints/REST/GetSubjectApiTest.php index cc486b512..55b358197 100644 --- a/tests/phpunit/EntryPoints/REST/GetSubjectApiTest.php +++ b/tests/phpunit/EntryPoints/REST/GetSubjectApiTest.php @@ -6,17 +6,20 @@ use MediaWiki\Page\PageIdentity; use MediaWiki\Rest\RequestData; +use MediaWiki\Rest\Response; use MediaWiki\Tests\Rest\Handler\HandlerTestTrait; use ProfessionalWiki\NeoWiki\Domain\Schema\SchemaName; use ProfessionalWiki\NeoWiki\Domain\Subject\StatementList; use ProfessionalWiki\NeoWiki\Domain\Subject\SubjectLabel; use ProfessionalWiki\NeoWiki\Domain\Subject\SubjectMap; use ProfessionalWiki\NeoWiki\EntryPoints\REST\GetSubjectApi; +use ProfessionalWiki\NeoWiki\NeoWikiExtension; use ProfessionalWiki\NeoWiki\Tests\Data\TestRelation; use ProfessionalWiki\NeoWiki\Tests\Data\TestStatement; use ProfessionalWiki\NeoWiki\Tests\Data\TestSubject; use ProfessionalWiki\NeoWiki\Tests\NeoWikiIntegrationTestCase; use ProfessionalWiki\NeoWiki\Tests\NeoWikiMockAuthorityTrait; +use ProfessionalWiki\NeoWiki\Tests\TestDoubles\FixedRevisionPolicy; /** * @covers \ProfessionalWiki\NeoWiki\EntryPoints\REST\GetSubjectApi @@ -205,6 +208,58 @@ public function testReturns404ForNonExistentRevision(): void { $this->assertSame( 404, $response->getStatusCode() ); } + /** + * A revision the registered policy hides answers exactly like one that does not exist, so the + * sequential revision ids cannot be swept to find out which drafts a page has. + */ + public function testRevisionHiddenByTheRevisionPolicyIsIndistinguishableFromAnAbsentRevision(): void { + $revisionId = $this->createPageWithSubjects( + 'GetSubjectApiTest_HiddenRevision', + mainSubject: TestSubject::build( + id: 'sTestGSA1111251', + schemaName: new SchemaName( 'GetSubjectApiTestSchema' ) + ) + )->getId(); + + [ $hiddenResponse, $absentResponse ] = $this->runWithRevisionPolicyHidingEveryRevision( + fn (): array => [ + $this->getSubjectAtRevision( 'sTestGSA1111251', (string)$revisionId ), + $this->getSubjectAtRevision( 'sTestGSA1111251', '999999999' ), + ] + ); + + $this->assertSame( 404, $hiddenResponse->getStatusCode() ); + $this->assertSame( $absentResponse->getStatusCode(), $hiddenResponse->getStatusCode() ); + + $hidden = json_decode( $hiddenResponse->getBody()->getContents(), true ); + $absent = json_decode( $absentResponse->getBody()->getContents(), true ); + + // Only the revision id embedded in the message may differ between "hidden" and "nonexistent". + $absent['message'] = str_replace( '999999999', (string)$revisionId, $absent['message'] ); + $this->assertSame( $absent, $hidden ); + } + + private function getSubjectAtRevision( string $subjectId, string $revisionId ): Response { + return $this->executeHandler( + new GetSubjectApi(), + new RequestData( [ + 'method' => 'GET', + 'pathParams' => [ 'subjectId' => $subjectId ], + 'queryParams' => [ 'revisionId' => $revisionId ], + ] ) + ); + } + + private function runWithRevisionPolicyHidingEveryRevision( callable $fn ): mixed { + $this->registerRevisionPolicy( FixedRevisionPolicy::hidingEveryRevision() ); + + try { + return $fn(); + } finally { + NeoWikiExtension::resetInstance(); + } + } + public function testFullExpansion(): void { $firstPageId = $this->createPageWithSubjects( 'GetSubjectApiTest0000', diff --git a/tests/phpunit/NeoWikiIntegrationTestCase.php b/tests/phpunit/NeoWikiIntegrationTestCase.php index ee9b72b46..ff28585b5 100644 --- a/tests/phpunit/NeoWikiIntegrationTestCase.php +++ b/tests/phpunit/NeoWikiIntegrationTestCase.php @@ -28,6 +28,7 @@ use ProfessionalWiki\NeoWiki\EntryPoints\Content\SchemaContent; use ProfessionalWiki\NeoWiki\EntryPoints\Content\SubjectContent; use ProfessionalWiki\NeoWiki\EntryPoints\NeoWikiRegistrar; +use ProfessionalWiki\NeoWiki\Application\RevisionPolicy; use ProfessionalWiki\NeoWiki\NeoWikiExtension; use ProfessionalWiki\NeoWiki\Persistence\MediaWiki\Subject\MediaWikiSubjectRepository; use ProfessionalWiki\NeoWiki\Tests\Data\TestSchema; @@ -50,6 +51,7 @@ class NeoWikiIntegrationTestCase extends MediaWikiIntegrationTestCase { /** @var PagePropertyProvider[] */ private array $registeredPagePropertyProviders = []; + private ?RevisionPolicy $registeredRevisionPolicy = null; /** * The singleton pins a SchemaLookup whose cache is keyed by page and revision id, and those ids @@ -372,6 +374,16 @@ protected function registerPagePropertyProviders( PagePropertyProvider ...$provi $this->registerWithNeoWiki(); } + /** + * Registers the revision policy through the NeoWikiRegistration hook and rebuilds the singleton, so + * the real wiring hands it to every path that publishes. + */ + protected function registerRevisionPolicy( RevisionPolicy $policy ): void { + $this->registeredRevisionPolicy = $policy; + + $this->registerWithNeoWiki(); + } + /** * Everything registered so far goes through one hook handler, replacing the previous one. * setTemporaryHook clears the hook before adding its handler, so registering plugins and providers @@ -381,10 +393,11 @@ protected function registerPagePropertyProviders( PagePropertyProvider ...$provi private function registerWithNeoWiki(): void { $plugins = $this->registeredGraphDatabasePlugins; $providers = $this->registeredPagePropertyProviders; + $policy = $this->registeredRevisionPolicy; $this->setTemporaryHook( 'NeoWikiRegistration', - static function ( NeoWikiRegistrar $registrar ) use ( $plugins, $providers ): void { + static function ( NeoWikiRegistrar $registrar ) use ( $plugins, $providers, $policy ): void { foreach ( $plugins as $name => $plugin ) { $registrar->addGraphDatabasePlugin( $name, $plugin ); } @@ -392,6 +405,10 @@ static function ( NeoWikiRegistrar $registrar ) use ( $plugins, $providers ): vo foreach ( $providers as $provider ) { $registrar->addPagePropertyProvider( $provider ); } + + if ( $policy !== null ) { + $registrar->setRevisionPolicy( $policy ); + } } ); diff --git a/tests/phpunit/Persistence/MediaWiki/PageContentFetcherTest.php b/tests/phpunit/Persistence/MediaWiki/PageContentFetcherTest.php index b15a47847..fee6588af 100644 --- a/tests/phpunit/Persistence/MediaWiki/PageContentFetcherTest.php +++ b/tests/phpunit/Persistence/MediaWiki/PageContentFetcherTest.php @@ -12,7 +12,10 @@ use MediaWiki\Title\Title; use MediaWiki\Title\TitleParser; use PHPUnit\Framework\TestCase; +use ProfessionalWiki\NeoWiki\Application\NullRevisionPolicy; +use ProfessionalWiki\NeoWiki\Application\RevisionPolicy; use ProfessionalWiki\NeoWiki\Persistence\MediaWiki\PageContentFetcher; +use ProfessionalWiki\NeoWiki\Tests\TestDoubles\FixedRevisionPolicy; /** * @covers \ProfessionalWiki\NeoWiki\Persistence\MediaWiki\PageContentFetcher @@ -33,7 +36,37 @@ public function setUp(): void { $this->revisionRecord = $this->createMock( RevisionRecord::class ); $this->content = $this->createMock( Content::class ); - $this->pageContentFetcher = new PageContentFetcher( $this->titleParser, $this->revisionLookup ); + $this->pageContentFetcher = $this->newFetcherWith( new NullRevisionPolicy() ); + } + + public function testReadsTheRevisionTheRevisionPolicyPublishes(): void { + $approved = $this->createMock( RevisionRecord::class ); + $approvedContent = $this->createMock( Content::class ); + $approved->method( 'getContent' )->willReturn( $approvedContent ); + + $this->titleParser->method( 'parseTitle' )->willReturn( Title::newFromText( 'test title' )->getTitleValue() ); + $this->revisionLookup->method( 'getRevisionByTitle' )->willReturn( $this->revisionRecord ); + $this->revisionRecord->method( 'getContent' )->willReturn( $this->content ); + + $content = $this->newFetcherWith( FixedRevisionPolicy::publishing( $approved ) ) + ->getPageContent( 'test title', $this->authority ); + + $this->assertSame( $approvedContent, $content ); + } + + public function testReadsNothingWhenThePolicyPublishesNoRevisionOfThePage(): void { + $this->titleParser->method( 'parseTitle' )->willReturn( Title::newFromText( 'test title' )->getTitleValue() ); + $this->revisionLookup->method( 'getRevisionByTitle' )->willReturn( $this->revisionRecord ); + $this->revisionRecord->method( 'getContent' )->willReturn( $this->content ); + + $content = $this->newFetcherWith( FixedRevisionPolicy::publishingNothing() ) + ->getPageContent( 'test title', $this->authority ); + + $this->assertNull( $content ); + } + + private function newFetcherWith( RevisionPolicy $policy ): PageContentFetcher { + return new PageContentFetcher( $this->titleParser, $this->revisionLookup, $policy ); } public function testGetPageContentWithGivenAuthority(): void { diff --git a/tests/phpunit/TestDoubles/FixedRevisionPolicy.php b/tests/phpunit/TestDoubles/FixedRevisionPolicy.php new file mode 100644 index 000000000..017202b17 --- /dev/null +++ b/tests/phpunit/TestDoubles/FixedRevisionPolicy.php @@ -0,0 +1,57 @@ +publishes; + } + + /** + * Answers per page, as an approval extension does: the fixed revision stands in only for revisions of + * its own page, and any other page — a Schema page the projection resolves, say — passes through. + */ + public function publishedRevision( RevisionRecord $revision ): ?RevisionRecord { + if ( !$this->substitutes ) { + return $revision; + } + + if ( $this->published === null || $this->published->getPageId() === $revision->getPageId() ) { + return $this->published; + } + + return $revision; + } + + public function revisionIsReadableBy( RevisionRecord $revision, Authority $viewer ): bool { + return $this->readable; + } + +} From c0d670daeeaad84483a2ad2ab58be3fd9b0d7a72 Mon Sep 17 00:00:00 2001 From: Jeroen De Dauw Date: Thu, 10 Sep 2026 00:44:09 +0200 Subject: [PATCH 2/2] Remove debug output left in PageRefreshWithoutEditTest Two fwrite( STDERR ) dumps, and the graph reads that only fed them, were left behind from debugging testRefreshProjectsTheRevisionTheRegisteredPolicyPublishes and printed on every run. Co-Authored-By: Claude Fable 5.1 --- .../Application/PageRefreshWithoutEditTest.php | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/tests/phpunit/Application/PageRefreshWithoutEditTest.php b/tests/phpunit/Application/PageRefreshWithoutEditTest.php index a100f865a..890ba5af1 100644 --- a/tests/phpunit/Application/PageRefreshWithoutEditTest.php +++ b/tests/phpunit/Application/PageRefreshWithoutEditTest.php @@ -60,26 +60,12 @@ public function testRefreshUpdatesThePagePropertiesOfAPageWithoutSubjects(): voi */ public function testRefreshProjectsTheRevisionTheRegisteredPolicyPublishes(): void { $approved = $this->createPageWithSubjects( self::PAGE_NAME, TestSubject::build() ); - $afterFirstSave = $this->readGraph( 'MATCH (s:Subject) RETURN count(s) AS n', [] )->first()->toRecursiveArray()['n']; $this->createPageWithSubjects( self::PAGE_NAME ); - $afterDraft = $this->readGraph( 'MATCH (s:Subject) RETURN count(s) AS n', [] )->first()->toRecursiveArray()['n']; $this->registerRevisionPolicy( FixedRevisionPolicy::publishing( $approved ) ); - $contentHasSubjects = $approved->getSlots()->getContent( 'neo' )->getPageSubjects()->hasSubjects(); - fwrite( STDERR, "\nDEBUG2 afterFirstSave=$afterFirstSave afterDraft=$afterDraft approvedContentHasSubjects=" . var_export( $contentHasSubjects, true ) . "\n" ); $outcome = $this->refreshPage(); $this->assertSame( PageRefreshOutcome::Refreshed, $outcome ); - // DEBUG - $dump = $this->readGraph( - 'MATCH (page:Page {id: $pageId}) OPTIONAL MATCH (page)-[r]->(n) RETURN page.id AS id, page.wiki_id AS wiki, collect(type(r)) AS rels, collect(labels(n)) AS targets', - [ 'pageId' => $approved->getPageId() ] - ); - $all = $this->readGraph( 'MATCH (s:Subject) RETURN s.id AS id, labels(s) AS labels', [] ); - fwrite( STDERR, "\nDEBUG approvedRev=" . $approved->getId() . " page=" . $approved->getPageId() - . " hasSlot=" . var_export( $approved->hasSlot( 'neo' ), true ) - . " dump=" . json_encode( $dump->first()?->toRecursiveArray() ) - . " subjects=" . json_encode( array_map( static fn ( $r ) => $r->toRecursiveArray(), iterator_to_array( $all ) ) ) . "\n" ); $this->assertTrue( $this->pageHoldsSubjectInGraph( $approved->getPageId() ), 'the approved revision holds a Subject the draft removed'