From 8324eb8187e5df4c5a4e7217a37efe76bd32611a Mon Sep 17 00:00:00 2001 From: Jeroen De Dauw Date: Thu, 20 Aug 2026 18:36:07 +0200 Subject: [PATCH] Resolve Subject ids through a MediaWiki table MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes https://github.com/ProfessionalWiki/NeoWiki/issues/1040 Subject ids are not derived from page titles, so finding the page holding a Subject takes a query, and the only index answering it was the Neo4j projection. That put identity resolution in storage that is allowed to lag — graph failures during a write are swallowed by design — so a Subject written during an outage stayed unfindable, and therefore uneditable, until an administrator rebuilt. It also made Neo4j a requirement for Subject CRUD, `{{#view}}`, `{{#neowiki_value}}` and the `mw.neowiki.*` getters. The mapping is now authoritative in `neowiki_subject_page`. A write that changes what a page holds replaces that page's rows; the edit, delete, undelete and import paths all reach it, and a save that changes no Subjects leaves the index untouched. An edit indexes from `RevisionFromEditComplete`, inside PageUpdater's atomic section, so the index commits with the revision or not at all, outside the projection's failure isolation. Rows are deleted by primary key rather than by page id, so concurrent saves do not contend on gap locks in the page-id index — the same reason core deletes `page_restrictions` that way (T214035). Ids come from the slot's raw JSON rather than the deserializer, so a Subject too broken to deserialize stays findable. Reads join `page`, so moves need no index maintenance and the rows a deleted page leaves behind resolve to nothing. Duplicate ids from cross-wiki transfer resolve to the lowest page id. A wiki with no graph backend configured is now a supported mode. Query surfaces stay registered per backend; relation-target suggestions still come from Neo4j and degrade to none without it. Because identity now resolves authoritatively, an unresolvable Subject is refused rather than measured against the wiki-global `edit` right. Creating a Subject authorizes against the page the request names, so it is unaffected; deleting one the index cannot resolve answers 404, like the other endpoints keyed by Subject id. `RebuildSubjectPageIndex.php` backfills the table and repairs it, registered to run from `update.php`. The decision is recorded as ADR 32, added here. ## Worth a close look - The rebuild sweeps with one unbatched `DELETE` and loads revisions a page at a time. Free on the initial backfill, not on a later repair run. - `DELETE /subject/{id}` still has no per-page read gate, unlike its siblings. Pre-existing, and this PR changes the same method, so it is filed as https://github.com/ProfessionalWiki/NeoWiki/issues/1312 ## Considered, omitted - **`ArticleMergeComplete` coverage** for a history merge that leaves the source page as a redirect — the one write neither the index nor the graph projection is told about. The rebuild script repairs both. - **Subject Sources** (https://github.com/ProfessionalWiki/NeoWiki/issues/993) beyond keying on the bare local nanoid. Co-Authored-By: Claude Opus 5 Co-Authored-By: Claude Fable 5 --- composer.json | 5 +- docs/adr/032-subject-page-index.md | 91 +++++ docs/operations/installation.md | 31 +- docs/operations/maintenance.md | 18 +- extension.json | 4 + maintenance/RebuildSubjectPageIndex.php | 70 ++++ sql/mysql/neowiki_subject_page.sql | 10 + sql/neowiki_subject_page.json | 40 ++ sql/postgres/neowiki_subject_page.sql | 11 + sql/sqlite/neowiki_subject_page.sql | 11 + .../CreateSubject/CreateSubjectAction.php | 8 +- .../DeleteSubject/DeleteSubjectAction.php | 10 +- src/Application/PageIdentifiersResolver.php | 5 +- .../ValidateSubjectUpdateQuery.php | 2 +- src/Application/SubjectPermissionHints.php | 6 +- src/Application/SubjectWriteAuthorizer.php | 2 +- .../Validation/SubjectValidator.php | 9 +- .../GraphBackendNotConfiguredException.php | 10 +- src/EntryPoints/Content/SubjectContent.php | 8 + src/EntryPoints/NeoWikiHooks.php | 37 +- src/EntryPoints/OnRevisionCreatedHandler.php | 65 +-- src/EntryPoints/REST/DeleteSubjectApi.php | 9 +- .../Neo4jPageIdentifiersLookup.php | 93 ----- .../AuthorityBasedPageReadAuthorizer.php | 4 +- .../AuthorityBasedSubjectAuthorizer.php | 34 +- src/NeoWikiConfig.php | 8 + src/NeoWikiExtension.php | 41 +- .../DatabasePageIdentifiersLookup.php | 73 ++++ .../MediaWiki/DatabaseSubjectPageIndex.php | 128 ++++++ .../DatabaseSubjectPageIndexRebuilder.php | 133 ++++++ .../SubjectContentDataDeserializer.php | 26 ++ src/Persistence/NullSubjectPageIndex.php | 24 ++ src/Persistence/SubjectPageIndex.php | 25 ++ .../Actions/DeleteSubjectActionTest.php | 44 +- .../GetPageSubjectsQueryTest.php | 2 +- tests/phpunit/Data/TestSubject.php | 10 + .../OnRevisionCreatedHandlerTest.php | 70 +++- .../EntryPoints/REST/CreateSubjectApiTest.php | 9 +- .../EntryPoints/REST/DeleteSubjectApiTest.php | 14 + .../REST/ValidateSubjectUpdateApiTest.php | 32 +- .../EntryPoints/SubjectContentTest.php | 18 + .../Neo4jPageIdentifiersLookupTest.php | 200 --------- .../AuthorityBasedPageReadAuthorizerTest.php | 4 +- .../AuthorityBasedSubjectAuthorizerTest.php | 44 +- tests/phpunit/NoGraphBackendTest.php | 196 +++++++-- ...sSchemaTest.php => DatabaseSchemaTest.php} | 44 +- .../DatabasePageIdentifiersLookupTest.php | 115 ++++++ .../MediaWikiSubjectRepositoryTest.php | 1 - tests/phpunit/SubjectPageIndexTest.php | 385 ++++++++++++++++++ .../TestDoubles/SpySubjectPageIndex.php | 33 ++ .../TestDoubles/SpySubjectWriteAuthorizer.php | 2 +- .../TestDoubles/ThrowingSubjectPageIndex.php | 25 ++ 52 files changed, 1756 insertions(+), 543 deletions(-) create mode 100644 docs/adr/032-subject-page-index.md create mode 100644 maintenance/RebuildSubjectPageIndex.php create mode 100644 sql/mysql/neowiki_subject_page.sql create mode 100644 sql/neowiki_subject_page.json create mode 100644 sql/postgres/neowiki_subject_page.sql create mode 100644 sql/sqlite/neowiki_subject_page.sql delete mode 100644 src/GraphDatabasePlugins/Neo4j/Persistence/Neo4jPageIdentifiersLookup.php create mode 100644 src/Persistence/MediaWiki/DatabasePageIdentifiersLookup.php create mode 100644 src/Persistence/MediaWiki/DatabaseSubjectPageIndex.php create mode 100644 src/Persistence/MediaWiki/DatabaseSubjectPageIndexRebuilder.php create mode 100644 src/Persistence/NullSubjectPageIndex.php create mode 100644 src/Persistence/SubjectPageIndex.php delete mode 100644 tests/phpunit/GraphDatabasePlugins/Neo4j/Persistence/Neo4jPageIdentifiersLookupTest.php rename tests/phpunit/Persistence/{RebuildRunsSchemaTest.php => DatabaseSchemaTest.php} (66%) create mode 100644 tests/phpunit/Persistence/MediaWiki/DatabasePageIdentifiersLookupTest.php create mode 100644 tests/phpunit/SubjectPageIndexTest.php create mode 100644 tests/phpunit/TestDoubles/SpySubjectPageIndex.php create mode 100644 tests/phpunit/TestDoubles/ThrowingSubjectPageIndex.php diff --git a/composer.json b/composer.json index ba419a2c2..0a3321989 100644 --- a/composer.json +++ b/composer.json @@ -53,7 +53,10 @@ "php ../../maintenance/generateSchemaSql.php --json sql/neowiki_rebuild_runs.json --sql sql/postgres/neowiki_rebuild_runs.sql --type postgres", "php ../../maintenance/generateSchemaChangeSql.php --json sql/abstractSchemaChanges/patch-neowiki_rebuild_runs-nwrr_phase.json --sql sql/mysql/patch-neowiki_rebuild_runs-nwrr_phase.sql --type mysql", "php ../../maintenance/generateSchemaChangeSql.php --json sql/abstractSchemaChanges/patch-neowiki_rebuild_runs-nwrr_phase.json --sql sql/sqlite/patch-neowiki_rebuild_runs-nwrr_phase.sql --type sqlite", - "php ../../maintenance/generateSchemaChangeSql.php --json sql/abstractSchemaChanges/patch-neowiki_rebuild_runs-nwrr_phase.json --sql sql/postgres/patch-neowiki_rebuild_runs-nwrr_phase.sql --type postgres" + "php ../../maintenance/generateSchemaChangeSql.php --json sql/abstractSchemaChanges/patch-neowiki_rebuild_runs-nwrr_phase.json --sql sql/postgres/patch-neowiki_rebuild_runs-nwrr_phase.sql --type postgres", + "php ../../maintenance/generateSchemaSql.php --json sql/neowiki_subject_page.json --sql sql/mysql/neowiki_subject_page.sql --type mysql", + "php ../../maintenance/generateSchemaSql.php --json sql/neowiki_subject_page.json --sql sql/sqlite/neowiki_subject_page.sql --type sqlite", + "php ../../maintenance/generateSchemaSql.php --json sql/neowiki_subject_page.json --sql sql/postgres/neowiki_subject_page.sql --type postgres" ] }, "scripts-descriptions": { diff --git a/docs/adr/032-subject-page-index.md b/docs/adr/032-subject-page-index.md new file mode 100644 index 000000000..4b7dac8c5 --- /dev/null +++ b/docs/adr/032-subject-page-index.md @@ -0,0 +1,91 @@ +# Subject-to-Page Index + +Date: 2026-08-20 + +Status: Accepted + +## Context + +Subject ids are not derived from the page title ([ADR 5](005-subject-guids.md)), so finding the page that holds a +Subject takes a query. The only index answering it was the Neo4j projection: `HasSubject` edges walked backwards. +Subject CRUD, `{{#view}}`, `{{#neowiki_value}}` and the `mw.neowiki.*` getters all resolved ids that way, so a wiki +with no graph backend, or with only a SPARQL store, could not store or read Subjects at all. + +Identity resolution has stronger requirements than the projection it was living in. A graph store is a rebuildable +query projection ([ADR 19](019-graph-database-architecture.md)) and its failures during edit, delete and undelete are +swallowed by design so the user's operation still commits. A Subject written while a backend was unreachable would +then be unfindable, and so uneditable, until an administrator rebuilt that store. With more than one backend +configured, per-backend failure isolation makes it worse: two stores can hold different answers to "which page holds +this Subject", and which one replies becomes an accident of wiring order. + +## Decision + +The subject-to-page mapping is authoritative in a MediaWiki table, `neowiki_subject_page`, keyed on the Subject id +and the page id. It is not a fallback and not a cache: it is the implementation of `PageIdentifiersLookup`. + +**Written with the revision it derives from.** `RevisionFromEditComplete` fires inside `PageUpdater`'s atomic section, +so indexing an edit on the wiki's primary connection joins the transaction that writes the revision: it commits with +that revision or not at all. The index can therefore never be staler than the subject slot it reads, and replica reads +inherit the revision's own session-consistency guarantees. It sits outside the projection's failure isolation, so an +index write that fails aborts the edit rather than being logged and passed over. + +Delete, undelete and import are indexed from hooks outside that atomic section. A web request still has its +transaction open, so a failure there takes the operation down with it, as on the edit path. A maintenance script has +no such transaction, so there the index write lands after the operation is already done: a failure leaves the index +wrong until a rebuild. Only a delete can strand rows that way, and those resolve to nothing anyway. + +**Ids come from the slot's raw JSON**, never from the deserializer. An invalid Subject is a persisted, supported state +([ADR 21](021-add-backend-validation.md), [ADR 26](026-validation-severity-levels.md)), and the lookup is what gets an +editor to the page holding it, so a Subject too broken to deserialize must still be findable. Ids that are not +well-formed are left out, since no caller could ask about them. + +**Reads join `page`.** The title and namespace are the page's current ones, so a move needs no index maintenance, and +the rows a deleted page leaves behind resolve to nothing. + +**Duplicate ids resolve to the lowest page id.** Cross-wiki transfer may bring one id onto two pages, which must not +fail either page's save ([ADR 5](005-subject-guids.md)), so the key is not unique. Every reader getting the same +answer matters more than which page wins. Ids are keyed bare, as stored ([ADR 22](022-multi-wiki-node-identity.md), +[ADR 23](023-subject-sources.md)): `null` means "no local page holds this", not "does not exist". + +Graph backends are unchanged: still rebuildable query projections, each registering its query surfaces only when it is +configured. What changes is that Subjects no longer need one. One editing feature still does: relation-target +suggestions are read from Neo4j, and without it the field offers none. + +Because identity is now resolved authoritatively, an unresolvable Subject is refused rather than measured against the +wiki-global `edit` right: every right a Subject write needs is a right on the page holding it, so with no page there +is nothing to allow. + +## Consequences + +* NeoWiki works with no graph backend configured. That is a supported mode, not a misconfiguration. +* A NeoWiki table is now on the correctness path, whereas `neowiki_rebuild_runs` only carries bookkeeping. Every save + writes the index, so a wiki that has not run `update.php` cannot be edited at all, rather than degrading to + Subjects being unfindable. +* `RebuildSubjectPageIndex.php` backfills an existing wiki, registered to run from `update.php`. MediaWiki's web + updater runs no post-update scripts, so a wiki upgraded that way gets an empty table, leaving every Subject that + predates the upgrade unresolvable until the script is run by hand. +* Resolving an id is a primary-key read against a local table rather than a network round-trip, on a path taken once + per Subject per page render. +* The index shares the projection's blind spot: a history merge that leaves the source page as a redirect writes that + revision without firing a hook either covers. The source page keeps its rows and still exists, so its Subjects go on + resolving to a page that no longer holds them, and a write to one lands there. The rebuild script is the repair + path. The impact is worse than for a projection, which only misreports a query. +* A graph rebuild does not write the index. It walks the wiki from a replica, so the revision it projects may already + have been superseded — eventual consistency the index must not inherit. +* The index rebuild is a plain walk, with none of the graph rebuild's per-store machinery: no resume cursor, no run + record, no way to start it from the wiki. It runs once at upgrade and rarely after, so a run that dies starts over. + +## Alternatives Considered + +* **`page_props`**: only indexed by `pp_propname`, so the Subject id would have to be encoded into the property name, + one name per Subject. `Special:PagesWithProp` and `list=pagepropnames` enumerate distinct property names wiki-wide, + so that breaks core surfaces NeoWiki cannot patch. `page_props` is also written POSTSEND, deliberately outside + ChronologyProtector: an eventually-consistent index under a strongly-consistent write path. +* **The search index**: optional infrastructure and eventually consistent, so it cannot carry a correctness + dependency. +* **Scanning content at query time**: revision content offers no indexed access and may be compressed or external, so + every lookup would load every Subject page's blob. +* **Making the id the page title**, as Wikidata does: dissolves the problem, but reverses + [ADR 5](005-subject-guids.md) and the [several-Subjects-per-page model](007-multiple-subjects-per-page.md). +* **A cache in front of the graph**: cannot answer a miss authoritatively, so it optimizes an implementation rather + than being one. diff --git a/docs/operations/installation.md b/docs/operations/installation.md index 20f3675b8..5a9d7c7f2 100644 --- a/docs/operations/installation.md +++ b/docs/operations/installation.md @@ -64,7 +64,7 @@ Use this to add NeoWiki to a MediaWiki you already run. You provide the surround | MediaWiki 1.43.0 or later | | | PHP 8.3 with `ext-json` | | | Composer | Installs NeoWiki's runtime dependencies. No `vendor/` is shipped. | -| Neo4j 5.x over Bolt | The graph backend. Required to use NeoWiki's structured-data features; the wiki boots without it. | +| Neo4j 5.x over Bolt | Optional. The graph backend behind Cypher queries and relation-target suggestions. | | Node.js 24 or later | Needed only to build the frontend bundle in step 2. | These extensions are recommended. NeoWiki runs without them, but you lose the matching functionality: @@ -122,8 +122,8 @@ wfLoadExtension( 'CodeEditor' ); wfLoadExtension( 'ParserFunctions' ); ``` -Without both Neo4j URLs set, the wiki still loads and ordinary pages render, but NeoWiki's structured-data features -and the query surfaces (`{{#cypher_raw}}`, `nw.query`, `POST /neowiki/v0/query/cypher`) stay disabled. +Without both Neo4j URLs set, NeoWiki's structured-data features still work. You lose the Cypher query surfaces +(`{{#cypher_raw}}`, `nw.query`, `POST /neowiki/v0/query/cypher`) and relation-target suggestions. ### 4. Run the updater @@ -151,14 +151,16 @@ php maintenance/run.php NeoWiki:RebuildGraphDatabases {{#view:}} ``` -4. **Query the graph.** Only this step checks the Neo4j projection. On any page, add a Cypher query that lists the - stored pages, independent of your data model: - ``` - {{#cypher_raw: MATCH (p:Page) RETURN p.name }} - ``` - The result renders as JSON. If Neo4j is unreachable, it renders an error instead. +Your install is complete once those three steps work. -If all four steps work, your install is complete. +With Neo4j configured, one more step checks its projection. On any page, add a Cypher query that lists the stored +pages, independent of your data model: + +``` +{{#cypher_raw: MATCH (p:Page) RETURN p.name }} +``` + +The result renders as JSON. If Neo4j is unreachable, it renders an error instead. ### Optional: Pretty URLs for the Data tab @@ -175,8 +177,8 @@ These are the settings you are most likely to change. For the full list with des | Setting | Purpose | Default | Required | |---|---|---|---| -| `$wgNeoWikiNeo4jInternalWriteUrl` | Bolt URL for writing the graph projection | _none_ | For features | -| `$wgNeoWikiNeo4jInternalReadUrl` | Bolt URL for read and query traffic | _none_ | For features | +| `$wgNeoWikiNeo4jInternalWriteUrl` | Bolt URL for writing the graph projection | _none_ | For Neo4j features | +| `$wgNeoWikiNeo4jInternalReadUrl` | Bolt URL for read and query traffic | _none_ | For Neo4j features | | `$wgNeoWikiEnableDevelopmentUI` | Enables development-only UIs | `false` | No | | `$wgNeoWikiEnforceValidation` | Rejects writes that introduce new `error`-severity violations | `false` | No | | `$wgNeoWikiAutoRenderMainSubject` | Automatically renders a page's Main Subject as an infobox | `true` | No | @@ -218,13 +220,10 @@ content model, validated, or read. ## Optional: SPARQL graph stores -Alongside Neo4j, NeoWiki can keep one or more SPARQL 1.1 graph stores in sync with page changes. This works with +With or without Neo4j, NeoWiki can keep one or more SPARQL 1.1 graph stores in sync with page changes. This works with QLever, Oxigraph, Fuseki and any other SPARQL 1.1 store. Each configured store receives the NeoWiki data as RDF: every page becomes a named graph, replaced on each edit and dropped on deletion. -A SPARQL store does not yet replace Neo4j: NeoWiki's interactive features (the Subject editing UIs, views, and value -accessors) still require a configured Neo4j backend. - Configure the stores with `$wgNeoWikiSparqlStores`, a list of objects: ```php diff --git a/docs/operations/maintenance.md b/docs/operations/maintenance.md index 5d242c1c5..57e5dca9e 100644 --- a/docs/operations/maintenance.md +++ b/docs/operations/maintenance.md @@ -102,11 +102,27 @@ store holding that projection in the background. It is off by default: such a re wiki, and it lets anyone who may edit Mapping pages set that going — work `neowiki-admin` otherwise gates. A rebuild somebody started by hand is left to finish rather than restarted; the store shows up stale once it ends. +## Rebuilding the subject index + +NeoWiki keeps an index of which page holds which Subject. Editing, deletion, undeletion and import all keep it +current and `update.php` fills it, so this is rarely needed. Run it when Subjects are not found on the pages holding +them — after merging page histories, or after upgrading through MediaWiki's web updater, which does not fill the +index: + +```sh +php maintenance/run.php NeoWiki:RebuildSubjectPageIndex --force +``` + +`--force` re-runs it after `update.php` has recorded it as done. + +Neither [rebuilding the graph](#rebuilding-the-graph) nor a null edit repairs this index. + ## What happens during a Neo4j outage - **Editing pages works.** Edits, deletions and undeletions all commit. NeoWiki logs the projection failure on the `NeoWiki` channel. -- **Editing and displaying Subjects fails**, along with queries and anything else that reads the graph. +- **Editing and displaying Subjects works.** +- **Queries fail**, along with relation-target suggestions and anything else that reads the graph. Once Neo4j is back, [rebuild the graph](#rebuilding-the-graph): it repairs both a failed save and a failed delete. diff --git a/extension.json b/extension.json index 2650a879a..e69b22893 100644 --- a/extension.json +++ b/extension.json @@ -30,6 +30,10 @@ "ProfessionalWiki\\NeoWiki\\": "src/" }, + "AutoloadClasses": { + "ProfessionalWiki\\NeoWiki\\Maintenance\\RebuildSubjectPageIndex": "maintenance/RebuildSubjectPageIndex.php" + }, + "TestAutoloadNamespaces": { "ProfessionalWiki\\NeoWiki\\Tests\\": "tests/phpunit", "ProfessionalWiki\\RedHerb\\": "tests/RedHerb/src" diff --git a/maintenance/RebuildSubjectPageIndex.php b/maintenance/RebuildSubjectPageIndex.php new file mode 100644 index 000000000..096e08bd0 --- /dev/null +++ b/maintenance/RebuildSubjectPageIndex.php @@ -0,0 +1,70 @@ + page index from the subject slots it derives from. + * + * update.php runs this once, which is what gets a wiki that predates the index a filled one. It is also + * the repair path afterwards, for a history merge that leaves the source page as a redirect: that writes + * a revision without firing a hook the index is built on. Such a run needs --force, since the update is + * logged as done. + */ +class RebuildSubjectPageIndex extends LoggedUpdateMaintenance { + + public function __construct() { + parent::__construct(); + + $this->setBatchSize( DatabaseSubjectPageIndexRebuilder::DEFAULT_BATCH_SIZE ); + $this->requireExtension( 'NeoWiki' ); + $this->addDescription( + 'Rebuilds the subject -> page index from the Subjects every page currently holds. ' . + 'Run this after merging page histories, which can write a revision the index is not told about.' + ); + } + + protected function getUpdateKey(): string { + return 'neowiki-rebuild-subject-page-index'; + } + + protected function doDBUpdates(): bool { + $this->output( "Rebuilding the NeoWiki subject -> page index...\n" ); + + // Left at 0 when there is nothing to index, so the closing line is right either way. + $indexed = 0; + + foreach ( $this->newRebuilder()->rebuild() as $indexed ) { + $this->output( "...$indexed pages indexed\n" ); + $this->waitForReplication(); + } + + $this->output( "Done. Indexed $indexed pages holding Subjects.\n" ); + + return true; + } + + /** + * Built from MediaWiki's services alone: update.php runs this on wikis whose NeoWiki configuration + * is not readable yet, and nothing here depends on that configuration. + */ + private function newRebuilder(): DatabaseSubjectPageIndexRebuilder { + return new DatabaseSubjectPageIndexRebuilder( + $this->getPrimaryDB(), + $this->getServiceContainer()->getRevisionLookup(), + $this->getBatchSize() + ); + } + +} + +$maintClass = RebuildSubjectPageIndex::class; +require_once RUN_MAINTENANCE_IF_MAIN; diff --git a/sql/mysql/neowiki_subject_page.sql b/sql/mysql/neowiki_subject_page.sql new file mode 100644 index 000000000..14970f8d2 --- /dev/null +++ b/sql/mysql/neowiki_subject_page.sql @@ -0,0 +1,10 @@ +-- This file is automatically generated using maintenance/generateSchemaSql.php. +-- Source: sql/neowiki_subject_page.json +-- Do not modify this file directly. +-- See https://www.mediawiki.org/wiki/Manual:Schema_changes +CREATE TABLE /*_*/neowiki_subject_page ( + nwsp_subject_id VARBINARY(32) NOT NULL, + nwsp_page_id INT UNSIGNED NOT NULL, + INDEX nwsp_page_id (nwsp_page_id), + PRIMARY KEY(nwsp_subject_id, nwsp_page_id) +) /*$wgDBTableOptions*/; diff --git a/sql/neowiki_subject_page.json b/sql/neowiki_subject_page.json new file mode 100644 index 000000000..f6266f470 --- /dev/null +++ b/sql/neowiki_subject_page.json @@ -0,0 +1,40 @@ +[ + { + "name": "neowiki_subject_page", + "comment": "Which page holds which Subject. Written with the revision that changes a page's Subjects, so it is never staler than the slot it derives from. Reads join page, so page moves need no maintenance here and rows left by a deleted page resolve to nothing.", + "columns": [ + { + "name": "nwsp_subject_id", + "comment": "Subject id as stored in the subject slot", + "type": "binary", + "options": { + "notnull": true, + "length": 32 + } + }, + { + "name": "nwsp_page_id", + "comment": "Page holding the Subject. Not a foreign key: a row for a page that no longer exists is inert, since reads join page.", + "type": "integer", + "options": { + "notnull": true, + "unsigned": true + } + } + ], + "indexes": [ + { + "name": "nwsp_page_id", + "comment": "Index for reading and replacing everything a page holds", + "columns": [ + "nwsp_page_id" + ], + "unique": false + } + ], + "pk": [ + "nwsp_subject_id", + "nwsp_page_id" + ] + } +] diff --git a/sql/postgres/neowiki_subject_page.sql b/sql/postgres/neowiki_subject_page.sql new file mode 100644 index 000000000..1ea05393a --- /dev/null +++ b/sql/postgres/neowiki_subject_page.sql @@ -0,0 +1,11 @@ +-- This file is automatically generated using maintenance/generateSchemaSql.php. +-- Source: sql/neowiki_subject_page.json +-- Do not modify this file directly. +-- See https://www.mediawiki.org/wiki/Manual:Schema_changes +CREATE TABLE neowiki_subject_page ( + nwsp_subject_id TEXT NOT NULL, + nwsp_page_id INT NOT NULL, + PRIMARY KEY(nwsp_subject_id, nwsp_page_id) +); + +CREATE INDEX nwsp_page_id ON neowiki_subject_page (nwsp_page_id); diff --git a/sql/sqlite/neowiki_subject_page.sql b/sql/sqlite/neowiki_subject_page.sql new file mode 100644 index 000000000..c5f6ef1a7 --- /dev/null +++ b/sql/sqlite/neowiki_subject_page.sql @@ -0,0 +1,11 @@ +-- This file is automatically generated using maintenance/generateSchemaSql.php. +-- Source: sql/neowiki_subject_page.json +-- Do not modify this file directly. +-- See https://www.mediawiki.org/wiki/Manual:Schema_changes +CREATE TABLE /*_*/neowiki_subject_page ( + nwsp_subject_id BLOB NOT NULL, + nwsp_page_id INTEGER UNSIGNED NOT NULL, + PRIMARY KEY(nwsp_subject_id, nwsp_page_id) +); + +CREATE INDEX nwsp_page_id ON /*_*/neowiki_subject_page (nwsp_page_id); diff --git a/src/Application/Actions/CreateSubject/CreateSubjectAction.php b/src/Application/Actions/CreateSubject/CreateSubjectAction.php index 78cb8da75..4d167acff 100644 --- a/src/Application/Actions/CreateSubject/CreateSubjectAction.php +++ b/src/Application/Actions/CreateSubject/CreateSubjectAction.php @@ -103,8 +103,7 @@ public function createSubject( CreateSubjectRequest $request ): void { } // The page identifiers come from the page id the request named, not from the subject -> page - // index: that index is the graph projection, which a read replica may not carry yet for the - // revision just written. + // index, which is read from a replica that may not carry the revision just written. $this->presenter->presentCreated( GetSubjectResponseItem::fromSubject( $subject, @@ -151,8 +150,9 @@ private function buildSubject( CreateSubjectRequest $request, ?Schema $schema ): } /** - * Best-effort global uniqueness check: the subject -> page index lags slot writes, so this can - * miss a very recently created Subject; ID entropy carries the rest (same posture as relation IDs). + * Best-effort global uniqueness check: the subject -> page index is read from a replica, so this + * can miss a Subject another request just created; ID entropy carries the rest (same posture as + * relation IDs). */ private function subjectIdIsInUse( SubjectId $id ): bool { return $this->pageIdentifiersLookup->getPageIdOfSubject( $id ) !== null; diff --git a/src/Application/Actions/DeleteSubject/DeleteSubjectAction.php b/src/Application/Actions/DeleteSubject/DeleteSubjectAction.php index 546fd94b5..fb3102171 100644 --- a/src/Application/Actions/DeleteSubject/DeleteSubjectAction.php +++ b/src/Application/Actions/DeleteSubject/DeleteSubjectAction.php @@ -7,6 +7,7 @@ use ProfessionalWiki\NeoWiki\Application\PageIdentifiersLookup; use ProfessionalWiki\NeoWiki\Application\SubjectWriteAuthorizer; use ProfessionalWiki\NeoWiki\Application\SubjectRepository; +use ProfessionalWiki\NeoWiki\Application\Subject\Exception\SubjectNotFoundException; use ProfessionalWiki\NeoWiki\Domain\Subject\SubjectId; use RuntimeException; @@ -20,11 +21,14 @@ public function __construct( } public function deleteSubject( SubjectId $subjectId, ?string $comment ): void { - // A null pageId (unresolvable Subject) makes the authorizer fall back to the global 'edit' right. - // This cannot bypass page protection: the repository resolves the page via the same lookup, so an - // unresolvable Subject results in a no-op delete rather than a write to a protected page. $pageId = $this->pageIdentifiersLookup->getPageIdOfSubject( $subjectId )?->getId(); + // A Subject on no page has no page rights to check, so it is answered as absent rather than as + // forbidden. + if ( $pageId === null ) { + throw SubjectNotFoundException::forId( $subjectId ); + } + if ( !$this->writeAuthorizer->authorize( $pageId ) ) { throw new RuntimeException( 'You do not have the necessary permissions to delete this subject' ); } diff --git a/src/Application/PageIdentifiersResolver.php b/src/Application/PageIdentifiersResolver.php index 33eb7ffc3..5079ec2eb 100644 --- a/src/Application/PageIdentifiersResolver.php +++ b/src/Application/PageIdentifiersResolver.php @@ -8,9 +8,8 @@ use ProfessionalWiki\NeoWiki\Domain\Page\PageIdentifiers; /** - * Answers what a page id identifies, from the wiki itself rather than from the graph projection. - * Callers that hold a page id use this instead of {@see PageIdentifiersLookup}: it needs no graph - * round trip, and it answers for a page written moments ago, which a graph read replica may not. + * Answers what a page id identifies: the page's current title and namespace. For callers that hold a + * page id, where {@see PageIdentifiersLookup} is for callers that hold a Subject id. */ interface PageIdentifiersResolver { diff --git a/src/Application/Queries/ValidateSubjectUpdate/ValidateSubjectUpdateQuery.php b/src/Application/Queries/ValidateSubjectUpdate/ValidateSubjectUpdateQuery.php index 2f8d8fe06..84bcebac9 100644 --- a/src/Application/Queries/ValidateSubjectUpdate/ValidateSubjectUpdateQuery.php +++ b/src/Application/Queries/ValidateSubjectUpdate/ValidateSubjectUpdateQuery.php @@ -43,7 +43,7 @@ public function validate( string $subjectId, string $label, array $statements ): $pageIdentifiers = $this->pageIdentifiersLookup->getPageIdOfSubject( $id ); if ( $pageIdentifiers === null ) { - // No owning page in the graph means the repository cannot load the Subject either. + // No owning page means the repository cannot load the Subject either. throw SubjectNotFoundException::forId( $id ); } diff --git a/src/Application/SubjectPermissionHints.php b/src/Application/SubjectPermissionHints.php index 8d4901b85..cacd70b20 100644 --- a/src/Application/SubjectPermissionHints.php +++ b/src/Application/SubjectPermissionHints.php @@ -13,10 +13,10 @@ */ interface SubjectPermissionHints { - public function canCreateMainSubject( ?PageId $pageId ): bool; + public function canCreateMainSubject( PageId $pageId ): bool; - public function canCreateChildSubject( ?PageId $pageId ): bool; + public function canCreateChildSubject( PageId $pageId ): bool; - public function canEditSubject( ?PageId $pageId ): bool; + public function canEditSubject( PageId $pageId ): bool; } diff --git a/src/Application/SubjectWriteAuthorizer.php b/src/Application/SubjectWriteAuthorizer.php index 17ff954eb..625ba7d44 100644 --- a/src/Application/SubjectWriteAuthorizer.php +++ b/src/Application/SubjectWriteAuthorizer.php @@ -16,6 +16,6 @@ */ interface SubjectWriteAuthorizer { - public function authorize( ?PageId $pageId ): bool; + public function authorize( PageId $pageId ): bool; } diff --git a/src/Application/Validation/SubjectValidator.php b/src/Application/Validation/SubjectValidator.php index 330f1841e..0cecde641 100644 --- a/src/Application/Validation/SubjectValidator.php +++ b/src/Application/Validation/SubjectValidator.php @@ -115,11 +115,10 @@ private function validateStatement( * not the declared targetSchema is a blocking `relation-target-schema-mismatch` error. * * The Schema compared is the target's own writer's-schema, read from its revision slot rather - * than from a graph node property. Reaching that slot still resolves the target id through the - * subject -> page index, which lives only in the graph projection (see - * {@see \ProfessionalWiki\NeoWiki\NeoWikiExtension::getPageIdentifiersLookup()}), so an - * unrebuilt or stale graph reports an existing target as not found. That is the same - * degradation the read path has, and the reason not-found is non-blocking. + * than from a graph node property. Reaching that slot resolves the target id through the + * subject -> page index, which a read replica may not carry yet for a target minted moments + * earlier elsewhere, so such a target reports as not found. That is the same degradation the + * read path has, and the reason not-found is non-blocking. * * @return Violation[] */ diff --git a/src/Domain/GraphDatabase/GraphBackendNotConfiguredException.php b/src/Domain/GraphDatabase/GraphBackendNotConfiguredException.php index e9330b849..ef15f4a76 100644 --- a/src/Domain/GraphDatabase/GraphBackendNotConfiguredException.php +++ b/src/Domain/GraphDatabase/GraphBackendNotConfiguredException.php @@ -9,15 +9,15 @@ /** * Thrown when code that needs a graph database backend is reached on a wiki that has none configured. * - * NeoWiki requires a configured graph backend to provide its structured-data features; a wiki with no - * backend is a misconfiguration, not a supported operating mode (see ADR 019, which defers full - * no-backend operation). This is a catchable, expected-runtime-state signal (unlike the LogicException - * guards on genuinely gated surfaces), so degradation boundaries can turn it into a clear notice. + * A wiki with no backend is a supported mode: the structured-data features work without one, and the + * query surfaces a backend brings are simply not registered (ADR 32). Reaching this means a caller got + * past that gating. It is a catchable, expected-runtime-state signal (unlike the LogicException guards + * on genuinely gated surfaces), so degradation boundaries can turn it into a clear notice. */ class GraphBackendNotConfiguredException extends RuntimeException { public function __construct( - string $message = 'NeoWiki requires a configured graph database backend. Configure the Neo4j read and write Bolt URLs.' + string $message = 'This feature needs a graph database backend, and this wiki has none configured.' ) { parent::__construct( $message ); } diff --git a/src/EntryPoints/Content/SubjectContent.php b/src/EntryPoints/Content/SubjectContent.php index 4c1123c15..65af1e74f 100644 --- a/src/EntryPoints/Content/SubjectContent.php +++ b/src/EntryPoints/Content/SubjectContent.php @@ -8,6 +8,7 @@ use MediaWiki\Content\JsonContent; use ProfessionalWiki\NeoWiki\Domain\Page\PageSubjects; use ProfessionalWiki\NeoWiki\NeoWikiExtension; +use ProfessionalWiki\NeoWiki\Persistence\MediaWiki\Subject\SubjectContentDataDeserializer; use ProfessionalWiki\NeoWiki\Persistence\MediaWiki\Subject\SubjectContentDataSerializer; class SubjectContent extends JsonContent { @@ -52,6 +53,13 @@ public function getPageSubjects(): PageSubjects { return NeoWikiExtension::getInstance()->newSubjectContentDataDeserializer()->deserialize( $this->getText() ); } + /** + * @return string[] + */ + public function getSubjectIds(): array { + return SubjectContentDataDeserializer::deserializeSubjectIds( $this->getText() ); + } + /** * @param callable(PageSubjects):void $mutator */ diff --git a/src/EntryPoints/NeoWikiHooks.php b/src/EntryPoints/NeoWikiHooks.php index 043c85c0b..e33508428 100644 --- a/src/EntryPoints/NeoWikiHooks.php +++ b/src/EntryPoints/NeoWikiHooks.php @@ -34,6 +34,7 @@ use ProfessionalWiki\NeoWiki\Application\SubjectResolver; use ProfessionalWiki\NeoWiki\EntryPoints\Actions\SubjectsAction; use ProfessionalWiki\NeoWiki\EntryPoints\Scribunto\ScribuntoLuaLibrary; +use ProfessionalWiki\NeoWiki\Maintenance\RebuildSubjectPageIndex; use ProfessionalWiki\NeoWiki\NeoWikiExtension; use ProfessionalWiki\NeoWiki\Persistence\MediaWiki\Subject\MediaWikiSubjectRepository; use ProfessionalWiki\NeoWiki\Presentation\PageToolsBuilder; @@ -61,14 +62,7 @@ private static function isContentPage( OutputPage $out ): bool { } private static function handleContentPage( OutputPage $out, Skin $skin ): void { - // Skip injection and warn loudly instead of 500ing every content page, so plain content pages - // still render on a wiki with no graph backend. Pages whose wikitext uses the graph-backed - // surfaces ({{#neowiki_value}}, the mw.neowiki getters) still fail their parse until the - // no-backend degradation work (#895); {{#view}} degrades to its client-side placeholder per component. - if ( NeoWikiExtension::getInstance()->getNeo4jPlugin() === null ) { - self::logMissingGraphBackend(); - return; - } + self::warnAboutHalfConfiguredNeo4j(); NeoWikiExtension::getInstance()->newFrontendModuleLoader()->load( $out, $skin ); $out->addHtml( self::getNeoWikiAppHtml( $out ) ); @@ -87,15 +81,21 @@ private static function handleContentPage( OutputPage $out, Skin $skin ): void { $out->addHTML( $html ); } - private static function logMissingGraphBackend(): void { - $config = NeoWikiExtension::getInstance()->config; - $onlyOneUrlSet = ( $config->neo4jInternalReadUrl !== null ) !== ( $config->neo4jInternalWriteUrl !== null ); - - $message = $onlyOneUrlSet - ? 'NeoWiki: only one of the Neo4j read/write Bolt URLs is configured; both are required. NeoWiki features are disabled.' - : 'NeoWiki: no graph database backend configured; NeoWiki features are disabled. Configure the Neo4j read and write Bolt URLs.'; + /** + * A wiki with no graph backend is a supported configuration: Subjects, Schemas, Views and the value + * accessors all work without one, and only the query surfaces a backend brings are absent. Half a + * Neo4j configuration is not a configuration, though — it reads as a backend that was meant to be + * there, so it is still reported. + */ + private static function warnAboutHalfConfiguredNeo4j(): void { + if ( !NeoWikiExtension::getInstance()->config->hasHalfConfiguredNeo4j() ) { + return; + } - LoggerFactory::getInstance( 'NeoWiki' )->warning( $message ); + LoggerFactory::getInstance( 'NeoWiki' )->warning( + 'NeoWiki: only one of the Neo4j read/write Bolt URLs is configured; both are required. ' + . 'Neo4j is disabled.' + ); } private static function getNeoWikiAppHtml( OutputPage $out ): string { @@ -199,6 +199,11 @@ public static function onLoadExtensionSchemaUpdates( DatabaseUpdater $updater ): 'nwrr_phase', $sqlDirectory . '/patch-neowiki_rebuild_runs-nwrr_phase.sql' ); + $updater->addExtensionTable( 'neowiki_subject_page', $sqlDirectory . '/neowiki_subject_page.sql' ); + + // Between creating the table and filling it, no Subject that existed before resolves to its page, + // so the backfill runs in the same update.php as the table it fills. + $updater->addPostDatabaseUpdateMaintenance( RebuildSubjectPageIndex::class ); $updater->addExtensionUpdate( [ [ self::class, 'initializeGraphDatabases' ] ] ); } diff --git a/src/EntryPoints/OnRevisionCreatedHandler.php b/src/EntryPoints/OnRevisionCreatedHandler.php index c54899672..9a469b0c2 100644 --- a/src/EntryPoints/OnRevisionCreatedHandler.php +++ b/src/EntryPoints/OnRevisionCreatedHandler.php @@ -14,6 +14,7 @@ use ProfessionalWiki\NeoWiki\EntryPoints\Content\SubjectContent; use ProfessionalWiki\NeoWiki\PagePropertiesSource; use ProfessionalWiki\NeoWiki\Persistence\MediaWiki\Subject\MediaWikiSubjectRepository; +use ProfessionalWiki\NeoWiki\Persistence\SubjectPageIndex; use Psr\Log\LoggerInterface; use RuntimeException; @@ -21,23 +22,33 @@ class OnRevisionCreatedHandler { public function __construct( private readonly GraphDatabasePlugin $graphDatabasePlugin, + private readonly SubjectPageIndex $subjectPageIndex, private readonly PagePropertiesSource $pagePropertiesSource, private readonly LoggerInterface $logger, ) { } /** - * Projects the page of the given revision, with the Subjects it holds and with none when it holds - * none: every page gets a Page node, so its Page Properties are queryable. + * 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. */ public function onRevisionCreated( RevisionRecord $revisionRecord, ?UserIdentity $user ): PageRefreshOutcome { if ( $revisionRecord->getPageId() === 0 ) { throw new RuntimeException( 'Page ID should not be 0' ); } - $subjects = $this->getPageSubjects( $revisionRecord ); + if ( !$revisionRecord->hasSlot( MediaWikiSubjectRepository::SLOT_NAME ) ) { + return $this->refreshPage( $revisionRecord, $user, null ); + } + + // The slot exists; a read failure here is a genuine error and must propagate — + // the refresh contract treats genuine failures as exceptions, not skips. + $content = $revisionRecord->getSlots()->getContent( MediaWikiSubjectRepository::SLOT_NAME ); - if ( $subjects === null ) { + // The slot holds something that is not Subject content, which happens when its content model is + // not registered, or an import wrote something else into it. Reading such a page as holding no + // Subjects would drop the Subjects it does hold, so nothing is written for it at all. + if ( !$content instanceof SubjectContent ) { $this->logSkip( $revisionRecord, 'its subject slot holds content that is not Subject data, so projecting the page would drop ' @@ -46,6 +57,23 @@ public function onRevisionCreated( RevisionRecord $revisionRecord, ?UserIdentity return PageRefreshOutcome::SkippedUnreadableSubjects; } + return $this->refreshPage( $revisionRecord, $user, $content ); + } + + private function refreshPage( + RevisionRecord $revisionRecord, + ?UserIdentity $user, + ?SubjectContent $content + ): PageRefreshOutcome { + $pageId = new PageId( $revisionRecord->getPageId() ); + + // Indexed before the Subjects are read as Subjects, and outside the projection's failure + // isolation: the index is authoritative (ADR 32), so it commits with the revision that changes + // it or not at all, and a Subject too broken to deserialize is still indexed. + $this->subjectPageIndex->setSubjectsOfPage( $pageId, $content?->getSubjectIds() ?? [] ); + + $subjects = $content?->getPageSubjects() ?? PageSubjects::newEmpty(); + // Null only from the isolating source the hook path is given, which has already logged the // cause. The rebuild path is given the propagating one, so there the failure surfaces to the // maintenance script instead, which reports it against the page. @@ -57,7 +85,7 @@ public function onRevisionCreated( RevisionRecord $revisionRecord, ?UserIdentity $this->graphDatabasePlugin->savePage( new Page( - id: new PageId( $revisionRecord->getPageId() ), + id: $pageId, properties: $properties, subjects: $subjects ) @@ -66,24 +94,6 @@ public function onRevisionCreated( RevisionRecord $revisionRecord, ?UserIdentity return PageRefreshOutcome::Refreshed; } - /** - * The Subjects the revision holds, none for a page without the subject slot, and null when the slot - * is present but does not hold Subject content — which happens when its content model is not - * registered, or an import wrote something else into it. Projecting such a page as holding no - * Subjects would wipe the Subjects it does hold from the graph, so nothing is written for it. - */ - private function getPageSubjects( RevisionRecord $revisionRecord ): ?PageSubjects { - if ( !$revisionRecord->hasSlot( MediaWikiSubjectRepository::SLOT_NAME ) ) { - return PageSubjects::newEmpty(); - } - - // The slot exists; a read failure here is a genuine error and must propagate — - // the refresh contract treats genuine failures as exceptions, not skips. - $content = $revisionRecord->getSlots()->getContent( MediaWikiSubjectRepository::SLOT_NAME ); - - return $content instanceof SubjectContent ? $content->getPageSubjects() : null; - } - private function logSkip( RevisionRecord $revisionRecord, string $reason ): void { $this->logger->warning( 'NeoWiki did not project page ' . $revisionRecord->getPageId() . ' because ' . $reason @@ -93,7 +103,14 @@ private function logSkip( RevisionRecord $revisionRecord, string $reason ): void } public function onPageDelete( int $pageId ): void { - $this->graphDatabasePlugin->deletePage( new PageId( $pageId ) ); + $page = new PageId( $pageId ); + + // The graph goes first because only the index removal can propagate: the projection write is + // failure-isolated, so the other order lets a database fault in the index skip the graph + // deletion and leave the deleted page's Subject values queryable. Rows the index keeps when + // this order fails are inert, since reads join `page`. + $this->graphDatabasePlugin->deletePage( $page ); + $this->subjectPageIndex->removePage( $page ); } } diff --git a/src/EntryPoints/REST/DeleteSubjectApi.php b/src/EntryPoints/REST/DeleteSubjectApi.php index 60f942b99..3458780cb 100644 --- a/src/EntryPoints/REST/DeleteSubjectApi.php +++ b/src/EntryPoints/REST/DeleteSubjectApi.php @@ -7,9 +7,11 @@ use MediaWiki\Rest\HttpException; use MediaWiki\Rest\Response; use MediaWiki\Rest\SimpleHandler; +use ProfessionalWiki\NeoWiki\Application\Subject\Exception\SubjectNotFoundException; use ProfessionalWiki\NeoWiki\Domain\Subject\SubjectId; use ProfessionalWiki\NeoWiki\NeoWikiExtension; use ProfessionalWiki\NeoWiki\Presentation\CsrfValidator; +use RuntimeException; use Wikimedia\ParamValidator\ParamValidator; class DeleteSubjectApi extends SimpleHandler { @@ -34,7 +36,12 @@ public function run( string $subjectId ): Response { new SubjectId( $subjectId ), $comment ); - } catch ( \RuntimeException $e ) { + } catch ( SubjectNotFoundException $e ) { + return $this->getResponseFactory()->createHttpError( 404, [ + 'status' => 'error', + 'message' => $e->getMessage(), + ] ); + } catch ( RuntimeException $e ) { return $this->getResponseFactory()->createHttpError( 403, [ 'status' => 'error', 'message' => $e->getMessage(), diff --git a/src/GraphDatabasePlugins/Neo4j/Persistence/Neo4jPageIdentifiersLookup.php b/src/GraphDatabasePlugins/Neo4j/Persistence/Neo4jPageIdentifiersLookup.php deleted file mode 100644 index ba5c22dff..000000000 --- a/src/GraphDatabasePlugins/Neo4j/Persistence/Neo4jPageIdentifiersLookup.php +++ /dev/null @@ -1,93 +0,0 @@ -getPageIdsOfSubjects( new SubjectIdList( [ $subjectId ] ) )[$subjectId->text] ?? null; - } - - /** - * Pages are reached by traversing HasSubject from globally-unique Subject ids, which is why the - * traversal is not wiki-scoped. Subject nodes are merged on id alone and detached per page, so - * more than one page can hold a HasSubject edge to the same Subject; which of them a Subject id - * then resolves to is unspecified. - * - * @return array - */ - public function getPageIdsOfSubjects( SubjectIdList $subjectIds ): array { - $ids = $subjectIds->asStringArray(); - - if ( $ids === [] ) { - return []; - } - - return $this->client->readTransaction( - function ( TransactionInterface $transaction ) use ( $ids ): array { - /** - * @var SummarizedResult $result - */ - $result = $transaction->run( - ' - MATCH (page:Page)-[:HasSubject]->(subject:Subject) - WHERE subject.id IN $subjectIds - RETURN subject.id AS subjectId, page.id AS id, page.name AS name, page.namespaceId AS namespaceId', - [ 'subjectIds' => $ids ] - ); - - return $this->newPageIdentifiersMap( $result->getResults()->toRecursiveArray() ); - } - ); - } - - /** - * @param array $rows - * @return array - */ - private function newPageIdentifiersMap( array $rows ): array { - $pageIdentifiers = []; - - foreach ( $rows as $row ) { - if ( is_array( $row ) && $this->hasAllColumns( $row ) ) { - $pageIdentifiers[(string)$row['subjectId']] = new PageIdentifiers( - id: new PageId( (int)$row['id'] ), - title: $row['name'], - namespaceId: (int)$row['namespaceId'], - ); - } - } - - return $pageIdentifiers; - } - - /** - * @param array $row - */ - private function hasAllColumns( array $row ): bool { - foreach ( [ 'subjectId', 'id', 'name', 'namespaceId' ] as $column ) { - if ( !array_key_exists( $column, $row ) ) { - return false; - } - } - - return true; - } - -} diff --git a/src/Infrastructure/AuthorityBasedPageReadAuthorizer.php b/src/Infrastructure/AuthorityBasedPageReadAuthorizer.php index 8ee782b85..2bd5e82ed 100644 --- a/src/Infrastructure/AuthorityBasedPageReadAuthorizer.php +++ b/src/Infrastructure/AuthorityBasedPageReadAuthorizer.php @@ -23,8 +23,8 @@ public function __construct( public function authorizeReadByPageId( PageId $pageId ): bool { $title = $this->titleFactory->newFromID( $pageId->id ); - // Unlike the write side, reads have no global-right fallback: content is only reachable - // through a resolved page, so an unresolvable one has nothing to authorize. + // Content is only reachable through a resolved page, so a page id this wiki cannot resolve + // has nothing to authorize and is denied. return $title !== null && $this->authorizeReadByPageTitle( $title ); } diff --git a/src/Infrastructure/AuthorityBasedSubjectAuthorizer.php b/src/Infrastructure/AuthorityBasedSubjectAuthorizer.php index 008fbc791..6f533df45 100644 --- a/src/Infrastructure/AuthorityBasedSubjectAuthorizer.php +++ b/src/Infrastructure/AuthorityBasedSubjectAuthorizer.php @@ -19,49 +19,41 @@ public function __construct( ) { } - public function canCreateMainSubject( ?PageId $pageId ): bool { + public function canCreateMainSubject( PageId $pageId ): bool { return $this->canEditPage( $pageId ); } - public function canCreateChildSubject( ?PageId $pageId ): bool { + public function canCreateChildSubject( PageId $pageId ): bool { return $this->canEditPage( $pageId ); } - public function canEditSubject( ?PageId $pageId ): bool { + public function canEditSubject( PageId $pageId ): bool { return $this->canEditPage( $pageId ); } - private function canEditPage( ?PageId $pageId ): bool { + private function canEditPage( PageId $pageId ): bool { $title = $this->newTitle( $pageId ); - if ( $title === null ) { - return $this->authority->isAllowed( 'edit' ); - } - // definitelyCan reads permissions from a replica and only peeks at the edit rate limit. - return $this->authority->definitelyCan( 'edit', $title ); + return $title !== null && $this->authority->definitelyCan( 'edit', $title ); } - public function authorize( ?PageId $pageId ): bool { + public function authorize( PageId $pageId ): bool { $title = $this->newTitle( $pageId ); - if ( $title === null ) { - return $this->authority->isAllowed( 'edit' ); - } - // authorizeWrite enforces page protection and blocks against the primary database, and // counts the write against the edit rate limit. - return $this->authority->authorizeWrite( 'edit', $title ); + return $title !== null && $this->authority->authorizeWrite( 'edit', $title ); } /** - * Null when there is no page, or when the page could not be resolved (for instance because the - * Subject is not indexed). Callers then fall back to the wiki-global edit right, so that - * authorization never fails open. Such writes are a no-op, so page protection cannot be - * bypassed by suppressing the page. + * Null when the Subject is on no page this wiki has. Every right a Subject write needs is a right on + * the page holding it, so with no page there is nothing to check and nothing to allow — the write is + * refused rather than measured against the wiki-global edit right (ADR 32). Creating a Subject is + * authorized against the page the request names, so it is unaffected. */ - private function newTitle( ?PageId $pageId ): ?Title { - return $pageId === null ? null : $this->titleFactory->newFromID( $pageId->id ); + private function newTitle( PageId $pageId ): ?Title { + return $this->titleFactory->newFromID( $pageId->id ); } } diff --git a/src/NeoWikiConfig.php b/src/NeoWikiConfig.php index 048373438..6a9086f24 100644 --- a/src/NeoWikiConfig.php +++ b/src/NeoWikiConfig.php @@ -31,6 +31,14 @@ public static function neo4jConfigured( ?string $readUrl, ?string $writeUrl ): b return $readUrl !== null && $writeUrl !== null; } + /** + * One of the two Neo4j Bolt URLs set and the other not, which is no backend rather than half of one. + */ + public function hasHalfConfiguredNeo4j(): bool { + return !$this->hasNeo4jBackend() + && ( $this->neo4jInternalReadUrl !== null || $this->neo4jInternalWriteUrl !== null ); + } + /** * Whether the store the SPARQL read surfaces query — the first configured one, see * {@see NeoWikiExtension::getFirstSparqlPlugin()} — holds the given projection, either as its own or diff --git a/src/NeoWikiExtension.php b/src/NeoWikiExtension.php index 359d9aa8c..1122ecafb 100644 --- a/src/NeoWikiExtension.php +++ b/src/NeoWikiExtension.php @@ -131,7 +131,9 @@ use ProfessionalWiki\NeoWiki\Infrastructure\AuthorityBasedSubjectAuthorizer; use ProfessionalWiki\NeoWiki\Infrastructure\TitleBasedPageIdentifiersResolver; use ProfessionalWiki\NeoWiki\Persistence\MediaWiki\DatabaseDeletedPageIdsLookup; +use ProfessionalWiki\NeoWiki\Persistence\MediaWiki\DatabasePageIdentifiersLookup; use ProfessionalWiki\NeoWiki\Persistence\MediaWiki\DatabasePageIdsLookup; +use ProfessionalWiki\NeoWiki\Persistence\MediaWiki\DatabaseSubjectPageIndex; use ProfessionalWiki\NeoWiki\Persistence\MediaWiki\DatabaseSchemaNameLookup; use ProfessionalWiki\NeoWiki\Persistence\MediaWiki\MediaWikiWikiConfigSource; use ProfessionalWiki\NeoWiki\Persistence\MediaWiki\PageContentFetcher; @@ -151,7 +153,6 @@ use ProfessionalWiki\NeoWiki\Persistence\MediaWiki\WikiPageSchemaLookup; use ProfessionalWiki\NeoWiki\Persistence\MediaWiki\WikiPageLayoutLookup; use ProfessionalWiki\NeoWiki\Persistence\MappingNameLookup; -use ProfessionalWiki\NeoWiki\GraphDatabasePlugins\Neo4j\Persistence\Neo4jPageIdentifiersLookup; use ProfessionalWiki\NeoWiki\GraphDatabasePlugins\Neo4j\Neo4jPlugin; use ProfessionalWiki\NeoWiki\GraphDatabasePlugins\Neo4j\Persistence\Neo4jSubjectLabelLookup; use ProfessionalWiki\NeoWiki\GraphDatabasePlugins\Neo4j\Persistence\Neo4jValueBuilderRegistry; @@ -161,7 +162,9 @@ use ProfessionalWiki\NeoWiki\GraphDatabasePlugins\Sparql\EntryPoints\REST\SparqlRouteRegistration; use ProfessionalWiki\NeoWiki\GraphDatabasePlugins\Sparql\SparqlPlugin; use ProfessionalWiki\NeoWiki\Persistence\DeletedPageIdsLookup; +use ProfessionalWiki\NeoWiki\Persistence\NullSubjectPageIndex; use ProfessionalWiki\NeoWiki\Persistence\PageIdsLookup; +use ProfessionalWiki\NeoWiki\Persistence\SubjectPageIndex; use ProfessionalWiki\NeoWiki\Application\GraphRebuild\GraphRebuildCoordinator; use ProfessionalWiki\NeoWiki\Application\GraphRebuild\GraphRebuildExecutor; use ProfessionalWiki\NeoWiki\Application\GraphRebuild\GraphStoreStatusLookup; @@ -348,15 +351,19 @@ public function getPagePropertyProviderRegistry(): PagePropertyProviderRegistry } /** - * Hook-facing write path (edit/delete/undelete). Both halves of a projection are isolated and + * Hook-facing write path (edit/delete/undelete/import). The projection's two halves are isolated and * logged, so a failure never aborts the triggering user operation: each backend, so one failing * backend does not starve the others, and the page-properties build, since it parses the page and * runs extension-contributed providers. See FailureIsolatingGraphDatabasePlugin and * FailureIsolatingPagePropertiesSource. + * + * The subject -> page index is deliberately outside that isolation: it is authoritative (ADR 32), so + * it is written on the connection the operation is already using and fails the operation with it. */ public function getStoreContentUC(): OnRevisionCreatedHandler { return $this->newStoreContentHandler( $this->getIsolatingGraphDatabasePlugin(), + $this->newSubjectPageIndex(), new FailureIsolatingPagePropertiesSource( $this->getPagePropertiesBuilder(), LoggerFactory::getInstance( 'NeoWiki' ) @@ -372,21 +379,30 @@ public function getStoreContentUC(): OnRevisionCreatedHandler { private function newRebuildStoreContentHandler(): OnRevisionCreatedHandler { return $this->newStoreContentHandler( $this->getGraphDatabasePlugin(), + new NullSubjectPageIndex(), $this->getPagePropertiesBuilder() ); } private function newStoreContentHandler( GraphDatabasePlugin $graphDatabasePlugin, - PagePropertiesSource $pagePropertiesSource + SubjectPageIndex $subjectPageIndex, + PagePropertiesSource $pagePropertiesSource, ): OnRevisionCreatedHandler { return new OnRevisionCreatedHandler( $graphDatabasePlugin, + $subjectPageIndex, $pagePropertiesSource, LoggerFactory::getInstance( 'NeoWiki' ), ); } + private function newSubjectPageIndex(): SubjectPageIndex { + return new DatabaseSubjectPageIndex( + MediaWikiServices::getInstance()->getConnectionProvider()->getPrimaryDatabase() + ); + } + public function getPagePropertiesBuilder(): PagePropertiesBuilder { return new PagePropertiesBuilder( revisionStore: MediaWikiServices::getInstance()->getRevisionStore(), @@ -1000,13 +1016,18 @@ public function newPageRebuilder(): PageRebuilder { */ public function newPageRebuilderFor( GraphDatabasePlugin $store ): PageRebuilder { return $this->newPageRebuilderWith( - $this->newStoreContentHandler( $store, $this->getPagePropertiesBuilder() ) + $this->newStoreContentHandler( + $store, + new NullSubjectPageIndex(), + $this->getPagePropertiesBuilder() + ) ); } /** * Import and undelete paths: 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 operation. + * These are revision writes rather than reprojections, so they index the page as an edit does. */ public function newImportPageRebuilder(): PageRebuilder { return $this->newPageRebuilderWith( $this->getStoreContentUC() ); @@ -1198,15 +1219,11 @@ public function getStatementListBuilder(): StatementListBuilder { ); } - // NeoWiki requires a configured graph backend: the subject -> page reverse index lives only in Neo4j, - // so this lookup (and Subject CRUD, {{#view}}, {{#neowiki_value}}, the mw.neowiki.* getters) needs one. - // A no-backend wiki is a misconfiguration, surfaced loudly rather than silently degraded: - // getReadOnlyNeo4jClient() throws GraphBackendNotConfiguredException, and the content-page render path - // (NeoWikiHooks::handleContentPage) short-circuits with a warning instead of failing the page. Making - // these work without a graph backend needs a MediaWiki-native reverse index; that is future work - // (#586 / #895), only worthwhile if a deliberate storage-only product is chosen (ADR 019 defers it). private function getPageIdentifiersLookup(): PageIdentifiersLookup { - return new Neo4jPageIdentifiersLookup( $this->getReadOnlyNeo4jClient() ); + return new DatabasePageIdentifiersLookup( + MediaWikiServices::getInstance()->getConnectionProvider()->getReplicaDatabase(), + MediaWikiServices::getInstance()->getTitleFormatter() + ); } public function newDeleteSubjectAction( Authority $authority ): DeleteSubjectAction { diff --git a/src/Persistence/MediaWiki/DatabasePageIdentifiersLookup.php b/src/Persistence/MediaWiki/DatabasePageIdentifiersLookup.php new file mode 100644 index 000000000..058ed7e4a --- /dev/null +++ b/src/Persistence/MediaWiki/DatabasePageIdentifiersLookup.php @@ -0,0 +1,73 @@ +getPageIdsOfSubjects( new SubjectIdList( [ $subjectId ] ) )[$subjectId->text] ?? null; + } + + /** + * @return array + */ + public function getPageIdsOfSubjects( SubjectIdList $subjectIds ): array { + $ids = $subjectIds->asStringArray(); + + if ( $ids === [] ) { + return []; + } + + $result = $this->db->newSelectQueryBuilder() + ->select( [ 'nwsp_subject_id', 'page_id', 'page_namespace', 'page_title' ] ) + ->from( DatabaseSubjectPageIndex::TABLE ) + ->join( 'page', null, 'page_id = nwsp_page_id' ) + ->where( [ 'nwsp_subject_id' => $ids ] ) + ->orderBy( [ 'nwsp_subject_id', 'nwsp_page_id' ] ) + ->caller( __METHOD__ ) + ->fetchResultSet(); + + $pageIdentifiers = []; + + /** @var stdClass $row */ + foreach ( $result as $row ) { + // Ordered by page id, so the first row of each Subject is the lowest page id holding it. + // An id on more than one page is what cross-wiki transfer produces (ADR 5); resolving it + // the same way for every reader matters more than which page wins. + $pageIdentifiers[$row->nwsp_subject_id] ??= new PageIdentifiers( + id: new PageId( (int)$row->page_id ), + title: $this->titleFormatter->getPrefixedText( + new TitleValue( (int)$row->page_namespace, $row->page_title ) + ), + namespaceId: (int)$row->page_namespace, + ); + } + + return $pageIdentifiers; + } + +} diff --git a/src/Persistence/MediaWiki/DatabaseSubjectPageIndex.php b/src/Persistence/MediaWiki/DatabaseSubjectPageIndex.php new file mode 100644 index 000000000..8001bc0bc --- /dev/null +++ b/src/Persistence/MediaWiki/DatabaseSubjectPageIndex.php @@ -0,0 +1,128 @@ +subjectsOfPage( $pageId ); + + // Most pages hold no Subjects and most edits change none, and this runs inside the transaction + // that writes the revision. A page whose rows already say this is left alone, so the common edit + // touches the index not at all. + // + // Reading them takes no lock, which is safe because PageUpdater derives the parent revision from + // an equally unlocked READ_LATEST read taken first, then CAS-compares it against + // lockAndGetLatest() before writing. A read stale enough to matter here therefore implies a + // stale parent revision, which is already refused as an edit conflict. + if ( $stored === $wanted ) { + return; + } + + // Replacing the rows is one step. The maintenance rebuild runs under CLI without DBO_TRX, so + // without this the delete and the insert commit separately, leaving a window in which the page's + // Subjects resolve to nothing. On the edit path this nests inside PageUpdater's atomic section, + // where it is bookkeeping that issues no SQL of its own. + $this->db->startAtomic( __METHOD__ ); + + if ( $stored !== [] ) { + $this->removeSubjectsOfPage( $pageId, $stored ); + } + + if ( $wanted !== [] ) { + $this->insertSubjectsOfPage( $pageId, $wanted ); + } + + $this->db->endAtomic( __METHOD__ ); + } + + /** + * Deletes by primary key rather than by page id, which is what {@see removePage} does. Deleting by + * page id alone walks the `nwsp_page_id` index and gap-locks the range around it, so two pages saved + * at once whose ids sit next to each other in that index block one another, and can deadlock. Naming + * both key columns turns each row into its own lookup, and Subject ids are random, so the rows a page + * holds are scattered rather than adjacent. MediaWiki core does the same for `page_restrictions` + * (T214035). + * + * @param string[] $subjectIds + */ + private function removeSubjectsOfPage( PageId $pageId, array $subjectIds ): void { + $this->db->newDeleteQueryBuilder() + ->deleteFrom( self::TABLE ) + ->where( [ 'nwsp_subject_id' => $subjectIds, 'nwsp_page_id' => $pageId->id ] ) + ->caller( __METHOD__ ) + ->execute(); + } + + /** + * @param string[] $subjectIds + */ + private function insertSubjectsOfPage( PageId $pageId, array $subjectIds ): void { + $this->db->newInsertQueryBuilder() + ->insertInto( self::TABLE ) + ->rows( array_map( + static fn ( string $subjectId ): array => [ + 'nwsp_subject_id' => $subjectId, + 'nwsp_page_id' => $pageId->id, + ], + $subjectIds + ) ) + ->caller( __METHOD__ ) + ->execute(); + } + + /** + * @return string[] Sorted here rather than by the database, so the comparison does not depend on + * the column's collation. + */ + private function subjectsOfPage( PageId $pageId ): array { + $stored = $this->db->newSelectQueryBuilder() + ->select( 'nwsp_subject_id' ) + ->from( self::TABLE ) + ->where( [ 'nwsp_page_id' => $pageId->id ] ) + ->caller( __METHOD__ ) + ->fetchFieldValues(); + + sort( $stored ); + + return $stored; + } + + public function removePage( PageId $pageId ): void { + $this->db->newDeleteQueryBuilder() + ->deleteFrom( self::TABLE ) + ->where( [ 'nwsp_page_id' => $pageId->id ] ) + ->caller( __METHOD__ ) + ->execute(); + } + +} diff --git a/src/Persistence/MediaWiki/DatabaseSubjectPageIndexRebuilder.php b/src/Persistence/MediaWiki/DatabaseSubjectPageIndexRebuilder.php new file mode 100644 index 000000000..fcc082cad --- /dev/null +++ b/src/Persistence/MediaWiki/DatabaseSubjectPageIndexRebuilder.php @@ -0,0 +1,133 @@ +index = new DatabaseSubjectPageIndex( $db ); + } + + /** + * @return iterable How many pages have been reindexed so far, once per batch. + */ + public function rebuild(): iterable { + $this->removeRowsOfPagesHoldingNoSubjectSlot(); + + $lastPageId = 0; + $indexed = 0; + + do { + $pageIds = $this->pageIdsHoldingSubjectSlotAfter( $lastPageId ); + + if ( $pageIds === [] ) { + return; + } + + foreach ( $pageIds as $pageId ) { + $lastPageId = $pageId; + $subjectIds = $this->subjectIdsOfPage( $pageId ); + + if ( $subjectIds !== null ) { + $this->index->setSubjectsOfPage( new PageId( $pageId ), $subjectIds ); + $indexed++; + } + } + + yield $indexed; + } while ( count( $pageIds ) === $this->batchSize ); + } + + /** + * Covers both a page that has been deleted and one whose slot an unhooked write removed: neither is + * reached by the walk below, so neither would have its rows replaced. + */ + private function removeRowsOfPagesHoldingNoSubjectSlot(): void { + $this->db->newDeleteQueryBuilder() + ->deleteFrom( DatabaseSubjectPageIndex::TABLE ) + ->where( 'nwsp_page_id NOT IN (' . $this->pagesHoldingSubjectSlot()->getSQL() . ')' ) + ->caller( __METHOD__ ) + ->execute(); + } + + /** + * @return int[] + */ + private function pageIdsHoldingSubjectSlotAfter( int $afterPageId ): array { + return array_map( 'intval', $this->pagesHoldingSubjectSlot() + ->where( $this->db->buildComparison( '>', [ 'page_id' => $afterPageId ] ) ) + ->orderBy( 'page_id' ) + ->limit( $this->batchSize ) + ->caller( __METHOD__ ) + ->fetchFieldValues() ); + } + + /** + * The pages whose current revision has a subject slot. Every other page holds no Subjects, so the + * index has nothing to say about it. The caller is left unset, so that embedding this as a subquery + * marks it as one rather than reporting it as a query of its own. + */ + private function pagesHoldingSubjectSlot(): SelectQueryBuilder { + return $this->db->newSelectQueryBuilder() + ->select( 'page_id' ) + ->from( 'page' ) + ->join( 'slots', null, 'slot_revision_id = page_latest' ) + ->join( 'slot_roles', null, 'slot_role_id = role_id' ) + ->where( [ 'role_name' => MediaWikiSubjectRepository::SLOT_NAME ] ); + } + + /** + * @return string[]|null Null when the page has to be left alone: its subject slot holds content that + * is not Subject data, so what it holds cannot be read, and reindexing it as holding nothing would + * drop the Subjects it does hold. The hook path skips such a page for the same reason. + */ + private function subjectIdsOfPage( int $pageId ): ?array { + // Read from the primary, like the walk that named the page: on a replica the page may not have + // its current revision yet, and indexing it from a stale one would file the wrong Subjects. + $revision = $this->revisionLookup->getRevisionByPageId( $pageId, 0, IDBAccessObject::READ_LATEST ); + + // Re-checked rather than taken from the walk that named the page: an edit between the two can + // have left the page without the slot, and asking a revision for a slot it lacks throws. + if ( $revision === null || !$revision->hasSlot( MediaWikiSubjectRepository::SLOT_NAME ) ) { + return []; + } + + // Read past revision deletion: what a page holds is what the index answers with, and hiding a + // revision from readers does not move its Subjects to another page. + $content = $revision->getContent( MediaWikiSubjectRepository::SLOT_NAME, RevisionRecord::RAW ); + + if ( !$content instanceof SubjectContent ) { + return null; + } + + return $content->getSubjectIds(); + } + +} diff --git a/src/Persistence/MediaWiki/Subject/SubjectContentDataDeserializer.php b/src/Persistence/MediaWiki/Subject/SubjectContentDataDeserializer.php index e17b27f04..fcfb0c05f 100644 --- a/src/Persistence/MediaWiki/Subject/SubjectContentDataDeserializer.php +++ b/src/Persistence/MediaWiki/Subject/SubjectContentDataDeserializer.php @@ -75,4 +75,30 @@ private function buildStatementList( array $jsonArray ): StatementList { return new StatementList( $statements ); } + /** + * The ids the JSON holds, read without deserializing, so that a Subject too broken to deserialize + * still has an id. Ids no caller could ask about are left out, since nothing can be answered with + * them. + * + * Static and dependency-free: update.php rebuilds the subject -> page index on wikis whose NeoWiki + * configuration is not readable yet. + * + * @return string[] + */ + public static function deserializeSubjectIds( string $json ): array { + // Cast so that content that is not a JSON object becomes an array without the key, leaving one + // thing to check. + $subjects = ( (array)json_decode( $json, true ) )['subjects'] ?? null; + + if ( !is_array( $subjects ) ) { + return []; + } + + // A Subject id that looks like a decimal integer comes back from json_decode as an int key. + return array_values( array_filter( + array_map( 'strval', array_keys( $subjects ) ), + SubjectId::isValid( ... ) + ) ); + } + } diff --git a/src/Persistence/NullSubjectPageIndex.php b/src/Persistence/NullSubjectPageIndex.php new file mode 100644 index 000000000..5d45ca774 --- /dev/null +++ b/src/Persistence/NullSubjectPageIndex.php @@ -0,0 +1,24 @@ + page index that {@see \ProfessionalWiki\NeoWiki\Application\PageIdentifiersLookup} + * reads (ADR 32). + */ +interface SubjectPageIndex { + + /** + * Records that the page holds exactly these Subjects, dropping whatever it held before. Idempotent, + * so the same page can be indexed as often as it is written. + * + * @param string[] $subjectIds Without duplicates: a page holds each of its Subjects once. + */ + public function setSubjectsOfPage( PageId $pageId, array $subjectIds ): void; + + public function removePage( PageId $pageId ): void; + +} diff --git a/tests/phpunit/Application/Actions/DeleteSubjectActionTest.php b/tests/phpunit/Application/Actions/DeleteSubjectActionTest.php index d2512b297..307c372e9 100644 --- a/tests/phpunit/Application/Actions/DeleteSubjectActionTest.php +++ b/tests/phpunit/Application/Actions/DeleteSubjectActionTest.php @@ -6,7 +6,10 @@ use PHPUnit\Framework\TestCase; use ProfessionalWiki\NeoWiki\Application\Actions\DeleteSubject\DeleteSubjectAction; +use ProfessionalWiki\NeoWiki\Application\PageIdentifiersLookup; use ProfessionalWiki\NeoWiki\Application\SubjectRepository; +use ProfessionalWiki\NeoWiki\Application\SubjectWriteAuthorizer; +use ProfessionalWiki\NeoWiki\Application\Subject\Exception\SubjectNotFoundException; use ProfessionalWiki\NeoWiki\Domain\Page\PageId; use ProfessionalWiki\NeoWiki\Domain\Page\PageIdentifiers; use ProfessionalWiki\NeoWiki\Domain\Subject\SubjectId; @@ -25,7 +28,7 @@ class DeleteSubjectActionTest extends TestCase { public function testDeleteSubjectRemovesSubjectFromRepository(): void { $repository = $this->newRepositoryWithSubject(); - $this->newAction( $repository )->deleteSubject( new SubjectId( self::SUBJECT_ID ), null ); + $this->newAllowingAction( $repository )->deleteSubject( new SubjectId( self::SUBJECT_ID ), null ); $this->assertNull( $repository->getSubject( new SubjectId( self::SUBJECT_ID ) ) ); } @@ -33,14 +36,15 @@ public function testDeleteSubjectRemovesSubjectFromRepository(): void { public function testDeleteSubjectPassesCommentThrough(): void { $repository = $this->newRepositoryWithSubject(); - $this->newAction( $repository )->deleteSubject( new SubjectId( self::SUBJECT_ID ), 'Removed by curator' ); + $this->newAllowingAction( $repository ) + ->deleteSubject( new SubjectId( self::SUBJECT_ID ), 'Removed by curator' ); $this->assertSame( 'Removed by curator', $repository->comments[self::SUBJECT_ID] ); } public function testAuthorizesAgainstTheSubjectsResolvedPage(): void { $authorizer = new SpySubjectWriteAuthorizer( allowed: true ); - $action = new DeleteSubjectAction( + $action = $this->newAction( $this->newRepositoryWithSubject(), $authorizer, new InMemoryPageIdentifiersLookup( [ @@ -54,7 +58,7 @@ public function testAuthorizesAgainstTheSubjectsResolvedPage(): void { } public function testThrowsWhenUserMayNotDeleteSubject(): void { - $action = new DeleteSubjectAction( + $action = $this->newAction( new InMemorySubjectRepository(), new SpySubjectWriteAuthorizer( allowed: false ), $this->pageIdentifiersLookupWithSubject() @@ -66,20 +70,48 @@ public function testThrowsWhenUserMayNotDeleteSubject(): void { $action->deleteSubject( new SubjectId( self::SUBJECT_ID ), null ); } + /** + * A Subject on no page has no page rights to check, so it is answered as absent rather than as + * forbidden, like the write endpoints keyed by Subject id do. + */ + public function testUnresolvableSubjectIsReportedAsNotFound(): void { + $action = $this->newAction( + $this->newRepositoryWithSubject(), + new SpySubjectWriteAuthorizer( allowed: true ), + new InMemoryPageIdentifiersLookup() + ); + + $this->expectException( SubjectNotFoundException::class ); + + $action->deleteSubject( new SubjectId( self::SUBJECT_ID ), null ); + } + private function newRepositoryWithSubject(): InMemorySubjectRepository { $repository = new InMemorySubjectRepository(); $repository->updateSubject( TestSubject::build( id: self::SUBJECT_ID ) ); return $repository; } - private function newAction( SubjectRepository $repository ): DeleteSubjectAction { - return new DeleteSubjectAction( + /** + * For the cases about deletion itself rather than about the checks around it: the caller may write, + * and the Subject resolves to a page. + */ + private function newAllowingAction( SubjectRepository $repository ): DeleteSubjectAction { + return $this->newAction( $repository, new SpySubjectWriteAuthorizer( allowed: true ), $this->pageIdentifiersLookupWithSubject() ); } + private function newAction( + SubjectRepository $repository, + SubjectWriteAuthorizer $authorizer, + PageIdentifiersLookup $pageIdentifiersLookup, + ): DeleteSubjectAction { + return new DeleteSubjectAction( $repository, $authorizer, $pageIdentifiersLookup ); + } + private function pageIdentifiersLookupWithSubject(): InMemoryPageIdentifiersLookup { return new InMemoryPageIdentifiersLookup( [ [ new SubjectId( self::SUBJECT_ID ), new PageIdentifiers( new PageId( 1 ), 'Test page', 0 ) ] diff --git a/tests/phpunit/Application/Queries/GetPageSubjects/GetPageSubjectsQueryTest.php b/tests/phpunit/Application/Queries/GetPageSubjects/GetPageSubjectsQueryTest.php index a2ec0ea50..50b3e4c3e 100644 --- a/tests/phpunit/Application/Queries/GetPageSubjects/GetPageSubjectsQueryTest.php +++ b/tests/phpunit/Application/Queries/GetPageSubjects/GetPageSubjectsQueryTest.php @@ -490,7 +490,7 @@ public function testReferencedSubjectOnThePageIsExcludedByTheCollectedIdFilter() /** * The counterpart of GetSubjectQueryTest::testDeniedRequestResolvesNoReferencedSubjects. A * denied page yields no Subjects to reach targets from, so both batch calls go out empty and - * cost nothing - but only because Neo4jPageIdentifiersLookup and PointInTimeSubjectLookup guard + * cost nothing - but only because DatabasePageIdentifiersLookup and PointInTimeSubjectLookup guard * the empty list. Asserting zero lookups here pins that rather than leaving it to those guards. */ public function testDeniedPageResolvesNoReferencedSubjects(): void { diff --git a/tests/phpunit/Data/TestSubject.php b/tests/phpunit/Data/TestSubject.php index e9d7d2317..5332ce967 100644 --- a/tests/phpunit/Data/TestSubject.php +++ b/tests/phpunit/Data/TestSubject.php @@ -48,6 +48,16 @@ public static function newMap(): SubjectMap { ); } + /** + * Subject slot JSON holding one Subject the deserializer throws on, because the empty string is not + * a usable Schema name. Shared by every test that depends on that, so a change to what makes + * deserialization fail cannot leave one of them passing vacuously. + */ + public static function jsonThatDoesNotDeserialize( string $subjectId ): string { + return '{"mainSubject":"' . $subjectId . '","subjects":{"' . $subjectId + . '":{"label":"Broken","schema":"","statements":{}}}}'; + } + /** * Generates a new GUID */ diff --git a/tests/phpunit/EntryPoints/OnRevisionCreatedHandlerTest.php b/tests/phpunit/EntryPoints/OnRevisionCreatedHandlerTest.php index 0012ada76..40b0c7ef4 100644 --- a/tests/phpunit/EntryPoints/OnRevisionCreatedHandlerTest.php +++ b/tests/phpunit/EntryPoints/OnRevisionCreatedHandlerTest.php @@ -13,6 +13,7 @@ use ProfessionalWiki\NeoWiki\Application\PageRefreshOutcome; use ProfessionalWiki\NeoWiki\Domain\GraphDatabase\FailureIsolatingGraphDatabasePlugin; use ProfessionalWiki\NeoWiki\Domain\GraphDatabase\GraphDatabasePlugin; +use ProfessionalWiki\NeoWiki\Domain\Page\PageId; use ProfessionalWiki\NeoWiki\Domain\Page\PagePropertyProvider; use ProfessionalWiki\NeoWiki\Domain\Page\PagePropertyProviderContext; use ProfessionalWiki\NeoWiki\Domain\Page\PagePropertyProviderRegistry; @@ -22,7 +23,9 @@ use ProfessionalWiki\NeoWiki\Tests\Data\TestSubject; use ProfessionalWiki\NeoWiki\Tests\NeoWikiIntegrationTestCase; use ProfessionalWiki\NeoWiki\Tests\TestDoubles\SpyGraphDatabasePlugin; +use ProfessionalWiki\NeoWiki\Tests\TestDoubles\SpySubjectPageIndex; use ProfessionalWiki\NeoWiki\Tests\TestDoubles\ThrowingGraphDatabasePlugin; +use ProfessionalWiki\NeoWiki\Tests\TestDoubles\ThrowingSubjectPageIndex; use Psr\Log\NullLogger; use Psr\Log\Test\TestLogger; use RuntimeException; @@ -33,7 +36,10 @@ */ class OnRevisionCreatedHandlerTest extends NeoWikiIntegrationTestCase { + private const int DELETED_PAGE_ID = 42; + private SpyGraphDatabasePlugin $graphStore; + private SpySubjectPageIndex $subjectPageIndex; private TestLogger $logger; protected function setUp(): void { @@ -41,6 +47,7 @@ protected function setUp(): void { $this->setUpNeo4j(); $this->createSchema( TestSubject::DEFAULT_SCHEMA_ID ); $this->graphStore = new SpyGraphDatabasePlugin(); + $this->subjectPageIndex = new SpySubjectPageIndex(); $this->logger = new TestLogger(); } @@ -53,6 +60,17 @@ public function testSavesPageWithSubjects(): void { $this->assertCount( 1, $this->graphStore->savedPages ); } + public function testIndexesTheSubjectsThePageHolds(): void { + $revision = $this->createPageWithSubjects( 'Page with indexed subject', TestSubject::build() ); + + $this->newHandler()->onRevisionCreated( $revision, new UserIdentityValue( 1, 'Tester' ) ); + + $this->assertSame( + [ $revision->getPageId() => [ TestSubject::ZERO_GUID ] ], + $this->subjectPageIndex->indexedSubjectsByPageId + ); + } + public function testSavesPageWithoutSubjects(): void { $revision = $this->newPlainPageRevision( 'Plain page' ); @@ -115,6 +133,7 @@ public function testWritesNothingWhenTheSubjectSlotDoesNotHoldSubjectContent(): $this->assertSame( PageRefreshOutcome::SkippedUnreadableSubjects, $outcome ); $this->assertSame( [], $this->graphStore->savedPages ); $this->assertSame( [], $this->graphStore->deletedPageIds ); + $this->assertSame( [], $this->subjectPageIndex->indexedSubjectsByPageId, 'the index should be left alone' ); $this->assertTrue( $this->logger->hasWarningRecords(), 'the skipped page should be logged' ); } @@ -150,6 +169,27 @@ public function testUnreachableBackendDoesNotHardFailRevisionHandling(): void { ); } + public function testDeletingAPageRemovesItFromTheGraph(): void { + $this->newHandler()->onPageDelete( self::DELETED_PAGE_ID ); + + $this->assertEquals( [ new PageId( self::DELETED_PAGE_ID ) ], $this->graphStore->deletedPageIds ); + } + + /** + * Only the index removal can propagate — the projection write is failure-isolated — so the graph has + * to be deleted first. The other order lets a database fault in the index skip the graph deletion, + * leaving the deleted page's Subject values queryable through the Cypher surfaces. + */ + public function testGraphDeletionHappensEvenWhenTheIndexRemovalFails(): void { + try { + $this->newHandlerWithFailingIndex()->onPageDelete( self::DELETED_PAGE_ID ); + } catch ( RuntimeException ) { + // Propagating is the index's contract; what matters is what already ran. + } + + $this->assertEquals( [ new PageId( self::DELETED_PAGE_ID ) ], $this->graphStore->deletedPageIds ); + } + private function newFailingProviderRegistry(): PagePropertyProviderRegistry { $registry = new PagePropertyProviderRegistry(); $registry->addProvider( new class implements PagePropertyProvider { @@ -190,17 +230,11 @@ private function newHandlerWith( ?PagePropertyProviderRegistry $providerRegistry = null, bool $isolatePageProperties = false ): OnRevisionCreatedHandler { - $services = $this->getServiceContainer(); - - $pageProperties = new PagePropertiesBuilder( - revisionStore: $services->getRevisionStore(), - contentHandlerFactory: $services->getContentHandlerFactory(), - titleFormatter: $services->getTitleFormatter(), - providerRegistry: $providerRegistry ?? new PagePropertyProviderRegistry(), - ); + $pageProperties = $this->newPagePropertiesBuilder( $providerRegistry ?? new PagePropertyProviderRegistry() ); return new OnRevisionCreatedHandler( $graphStore, + $this->subjectPageIndex, $isolatePageProperties ? new FailureIsolatingPagePropertiesSource( $pageProperties, $this->logger ) : $pageProperties, @@ -208,4 +242,24 @@ private function newHandlerWith( ); } + private function newHandlerWithFailingIndex(): OnRevisionCreatedHandler { + return new OnRevisionCreatedHandler( + $this->graphStore, + new ThrowingSubjectPageIndex(), + $this->newPagePropertiesBuilder( new PagePropertyProviderRegistry() ), + $this->logger + ); + } + + private function newPagePropertiesBuilder( PagePropertyProviderRegistry $providerRegistry ): PagePropertiesBuilder { + $services = $this->getServiceContainer(); + + return new PagePropertiesBuilder( + revisionStore: $services->getRevisionStore(), + contentHandlerFactory: $services->getContentHandlerFactory(), + titleFormatter: $services->getTitleFormatter(), + providerRegistry: $providerRegistry, + ); + } + } diff --git a/tests/phpunit/EntryPoints/REST/CreateSubjectApiTest.php b/tests/phpunit/EntryPoints/REST/CreateSubjectApiTest.php index 54f963ba7..4ff7e860d 100644 --- a/tests/phpunit/EntryPoints/REST/CreateSubjectApiTest.php +++ b/tests/phpunit/EntryPoints/REST/CreateSubjectApiTest.php @@ -37,13 +37,6 @@ class CreateSubjectApiTest extends NeoWikiIntegrationTestCase { // A page id far above anything a fresh test database mints, so it resolves to no page. private const int NONEXISTENT_PAGE_ID = 999999; - protected function setUp(): void { - parent::setUp(); - // Clear the graph so the client-supplied-id tests, which use fixed ids, start from a clean - // subject -> page index rather than nodes projected by earlier tests or runs. - $this->setUpNeo4j(); - } - public function testCreatesSubject(): void { $this->createSchema( 'Employee' ); @@ -487,7 +480,7 @@ public function testSuppliedIdAlreadyUsedOnAnotherPageReturns409(): void { $firstResponse = $this->executeCreate( $this->pageIdOfNewPage( 'CreateSubjectApiTestOtherPage' ), $firstBody, isMainSubject: false ); $this->assertSame( 201, $firstResponse->getStatusCode() ); - // Reusing it on a different page is rejected via the graph's subject -> page index. + // Reusing it on a different page is rejected via the subject -> page index. $secondBody = $this->validBody(); $secondBody['id'] = $suppliedId; $secondResponse = $this->executeCreate( $this->getIdOfExistingPage(), $secondBody, isMainSubject: false ); diff --git a/tests/phpunit/EntryPoints/REST/DeleteSubjectApiTest.php b/tests/phpunit/EntryPoints/REST/DeleteSubjectApiTest.php index 0cd1986f0..ab6f75005 100644 --- a/tests/phpunit/EntryPoints/REST/DeleteSubjectApiTest.php +++ b/tests/phpunit/EntryPoints/REST/DeleteSubjectApiTest.php @@ -80,6 +80,20 @@ public function testDeleteWithComment(): void { $this->assertSame( 200, $response->getStatusCode() ); } + /** + * A Subject the index does not resolve is answered as absent. Pinned at this layer because + * SubjectNotFoundException is a RuntimeException: catching the two in the other order would answer + * the 403 below instead, which no action-level test can see. + */ + public function testUnresolvableSubjectIsNotFound(): void { + $response = $this->executeHandler( + $this->newDeleteSubjectApi(), + $this->createValidRequestData() + ); + + $this->assertSame( 404, $response->getStatusCode() ); + } + public function testPermissionDenied(): void { $this->createPages(); diff --git a/tests/phpunit/EntryPoints/REST/ValidateSubjectUpdateApiTest.php b/tests/phpunit/EntryPoints/REST/ValidateSubjectUpdateApiTest.php index c1644e5f8..ed46d9cc8 100644 --- a/tests/phpunit/EntryPoints/REST/ValidateSubjectUpdateApiTest.php +++ b/tests/phpunit/EntryPoints/REST/ValidateSubjectUpdateApiTest.php @@ -7,6 +7,7 @@ use MediaWiki\Rest\RequestData; use MediaWiki\Tests\Rest\Handler\HandlerTestTrait; use ProfessionalWiki\NeoWiki\Application\Subject\Exception\SubjectNotFoundException; +use ProfessionalWiki\NeoWiki\Domain\Schema\SchemaName; use ProfessionalWiki\NeoWiki\Domain\Subject\Subject; use ProfessionalWiki\NeoWiki\Domain\Subject\SubjectId; use ProfessionalWiki\NeoWiki\Domain\Subject\SubjectLabel; @@ -174,16 +175,31 @@ public function testNoWriteOccurs(): void { $this->assertSame( 'Test subject sTestSU11111111', $subject->getLabel()->text ); } + /** + * The index is written from the subject slot, which knows nothing about Schemas, so a Subject whose + * Schema never existed resolves like any other and validation reaches the schema check rather than + * answering 404 before it. + */ public function testSubjectWithMissingSchemaReturnsSchemaNotFoundViolation(): void { - $this->markTestSkipped( - 'The schema-not-found path requires a Subject to be findable via the Neo4j page-identifier ' - . 'lookup AND for its Schema to be absent. In practice, a Subject created with a never-existing ' - . 'Schema cannot be projected into Neo4j, so the lookup returns null and the handler responds 404 ' - . 'before reaching the schema check. The defensive schema-not-found path remains in the code as ' - . 'belt-and-suspenders for the rare scenario where a Schema page is deleted after Subject creation; ' - . 'reaching it in a test requires a fixture sequence (create schema -> create subject -> rebuild ' - . 'projection -> delete schema page) that this test harness does not currently support.' + $this->createPageWithSubjects( + 'ValidateSubjectUpdateApiMissingSchemaTest', + mainSubject: TestSubject::build( + id: 'sTestSU11111111', + label: new SubjectLabel( 'Test subject sTestSU11111111' ), + schemaName: new SchemaName( 'SchemaThatWasNeverCreated' ), + ) ); + + $response = $this->executeHandler( + $this->newValidateSubjectUpdateApi(), + $this->createRequestData( 'sTestSU11111111', $this->validBody() ) + ); + + $responseBody = json_decode( $response->getBody()->getContents(), true ); + + $this->assertSame( 200, $response->getStatusCode() ); + $this->assertCount( 1, $responseBody['violations'] ); + $this->assertSame( 'schema-not-found', $responseBody['violations'][0]['code'] ); } public function testNeedsWriteAccessReturnsFalse(): void { diff --git a/tests/phpunit/EntryPoints/SubjectContentTest.php b/tests/phpunit/EntryPoints/SubjectContentTest.php index e35662e15..92f6c579c 100644 --- a/tests/phpunit/EntryPoints/SubjectContentTest.php +++ b/tests/phpunit/EntryPoints/SubjectContentTest.php @@ -73,4 +73,22 @@ public function testMutatePageSubjects(): void { $this->assertNull( $content->getPageSubjects()->getMainSubject() ); } + public function testSubjectIdsAreReadWithoutDeserializing(): void { + $content = new SubjectContent( TestSubject::jsonThatDoesNotDeserialize( TestSubject::ZERO_GUID ) ); + + $this->assertSame( [ TestSubject::ZERO_GUID ], $content->getSubjectIds() ); + } + + public function testSubjectIdsLeaveOutKeysThatAreNotSubjectIds(): void { + $content = new SubjectContent( + '{"subjects":{"not an id":{},"' . TestSubject::ZERO_GUID . '":{}}}' + ); + + $this->assertSame( [ TestSubject::ZERO_GUID ], $content->getSubjectIds() ); + } + + public function testContentThatIsNotSubjectJsonHoldsNoSubjectIds(): void { + $this->assertSame( [], ( new SubjectContent( 'not json at all' ) )->getSubjectIds() ); + } + } diff --git a/tests/phpunit/GraphDatabasePlugins/Neo4j/Persistence/Neo4jPageIdentifiersLookupTest.php b/tests/phpunit/GraphDatabasePlugins/Neo4j/Persistence/Neo4jPageIdentifiersLookupTest.php deleted file mode 100644 index 0212bc8bb..000000000 --- a/tests/phpunit/GraphDatabasePlugins/Neo4j/Persistence/Neo4jPageIdentifiersLookupTest.php +++ /dev/null @@ -1,200 +0,0 @@ -setUpNeo4j(); - } - - public function testReturnsNullOnEmptyGraph(): void { - $this->assertNull( $this->newLookup()->getPageIdOfSubject( new SubjectId( self::GUID_404 ) ) ); - } - - private function newLookup( ?ClientInterface $client = null ): Neo4jPageIdentifiersLookup { - return new Neo4jPageIdentifiersLookup( - client: $client ?? $this->getClient() - ); - } - - private function getClient(): ClientInterface { - return NeoWikiExtension::getInstance()->getNeo4jClient(); - } - - public function testFindsIdOfPage(): void { - $this->savePages(); - - $this->assertEquals( - new PageIdentifiers( new PageId( 42 ), 'Bar', 12 ), - $this->newLookup( $this->getClient() )->getPageIdOfSubject( new SubjectId( self::GUID_2 ) ) - ); - } - - public function testFindsThePageOfEveryRequestedSubject(): void { - $this->savePages(); - - $this->assertEquals( - [ - self::GUID_4 => new PageIdentifiers( new PageId( 1 ), 'Foo', 0 ), - self::GUID_2 => new PageIdentifiers( new PageId( 42 ), 'Bar', 12 ), - self::GUID_5 => new PageIdentifiers( new PageId( 32202 ), 'Baz', 0 ), - ], - $this->newLookup()->getPageIdsOfSubjects( - new SubjectIdList( [ - new SubjectId( self::GUID_4 ), - new SubjectId( self::GUID_2 ), - new SubjectId( self::GUID_5 ), - ] ) - ) - ); - } - - public function testOmitsSubjectsNoPageHosts(): void { - $this->savePages(); - - $this->assertSame( - [ self::GUID_2 ], - array_keys( $this->newLookup()->getPageIdsOfSubjects( - new SubjectIdList( [ - new SubjectId( self::GUID_404 ), - new SubjectId( self::GUID_2 ), - ] ) - ) ) - ); - } - - /** - * Callers index the result by Subject id rather than by request position, so a caller that - * happens to collect its ids in a different order must not see a different result. - */ - public function testRequestOrderDoesNotChangeTheResult(): void { - $this->savePages(); - - $ids = [ new SubjectId( self::GUID_1 ), new SubjectId( self::GUID_3 ), new SubjectId( self::GUID_5 ) ]; - - $this->assertEquals( - $this->newLookup()->getPageIdsOfSubjects( new SubjectIdList( $ids ) ), - $this->newLookup()->getPageIdsOfSubjects( new SubjectIdList( array_reverse( $ids ) ) ) - ); - } - - /** - * Subject nodes are merged on id alone and detached per page, so an id can carry a HasSubject - * edge from more than one page. Which of them it resolves to is unspecified, but the map is - * keyed by Subject id, so the second edge must not add a second entry or disturb the other ids. - */ - public function testSubjectHostedByTwoPagesYieldsOneEntry(): void { - $this->savePages(); - $this->savePageHostingSubject( 77, self::GUID_4, 'Qux' ); - - $this->assertSame( - 2, - $this->countHostingEdges( self::GUID_4 ), - 'the fixture must leave two pages hosting the Subject, or this test asserts nothing' - ); - - $identifiers = $this->newLookup()->getPageIdsOfSubjects( - new SubjectIdList( [ new SubjectId( self::GUID_4 ), new SubjectId( self::GUID_2 ) ] ) - ); - - $this->assertCount( 2, $identifiers ); - $this->assertArrayHasKey( self::GUID_4, $identifiers ); - $this->assertContains( $identifiers[self::GUID_4]->getId()->id, [ 1, 77 ] ); - $this->assertEquals( - new PageIdentifiers( new PageId( 42 ), 'Bar', 12 ), - $identifiers[self::GUID_2] ?? null - ); - } - - private function countHostingEdges( string $subjectId ): int { - return $this->readGraph( - 'MATCH (:Page)-[:HasSubject]->(subject:Subject { id: $subjectId }) RETURN count(*) AS count', - [ 'subjectId' => $subjectId ] - )->first()->toRecursiveArray()['count']; - } - - /** - * Validating a Subject that holds no relations asks for no ids at all, which is the common case, - * so it must not cost a query. - */ - public function testAsksTheGraphNothingWhenNoIdsAreRequested(): void { - $client = $this->createStub( ClientInterface::class ); - $client->method( 'readTransaction' )->willThrowException( new RuntimeException( 'queried the graph' ) ); - - $this->assertSame( [], $this->newLookup( $client )->getPageIdsOfSubjects( new SubjectIdList( [] ) ) ); - } - - /** - * Only sound for a page the graph does not hold yet: nothing is removed or detached, so the - * edge an earlier page already holds to the same Subject stays. Re-saving an existing page is - * not equivalent - dropping a Subject from a page that used to host it removes that Subject - * graph-wide, every other page's edge to it included. - */ - private function savePageHostingSubject( int $pageId, string $subjectId, string $title ): void { - $this->newProjectionStore()->savePage( TestPage::build( - id: $pageId, - properties: TestPageProperties::build( title: $title ), - childSubjects: new SubjectMap( - TestSubject::build( id: $subjectId ), - ) - ) ); - } - - private function savePages(): void { - $projectionStore = $this->newProjectionStore(); - - $projectionStore->savePage( TestPage::build( - id: 1, - properties: TestPageProperties::build( title: 'Foo' ), - childSubjects: new SubjectMap( - TestSubject::build( id: self::GUID_4 ), - ) - ) ); - - $projectionStore->savePage( TestPage::build( - id: 42, - properties: TestPageProperties::build( title: 'Bar', namespaceId: 12 ), - childSubjects: new SubjectMap( - TestSubject::build( id: self::GUID_1 ), - TestSubject::build( id: self::GUID_2 ), - TestSubject::build( id: self::GUID_3 ), - ) - ) ); - - $projectionStore->savePage( TestPage::build( - id: 32202, - properties: TestPageProperties::build( title: 'Baz' ), - childSubjects: new SubjectMap( - TestSubject::build( id: self::GUID_5 ), - ) - ) ); - } - -} diff --git a/tests/phpunit/Infrastructure/AuthorityBasedPageReadAuthorizerTest.php b/tests/phpunit/Infrastructure/AuthorityBasedPageReadAuthorizerTest.php index 730cea73e..98688cd8b 100644 --- a/tests/phpunit/Infrastructure/AuthorityBasedPageReadAuthorizerTest.php +++ b/tests/phpunit/Infrastructure/AuthorityBasedPageReadAuthorizerTest.php @@ -46,8 +46,8 @@ public function testReadByPageIdIsAllowedWhenThePageCanBeRead(): void { } public function testReadByPageIdDeniesWhenThePageCannotBeResolved(): void { - // Unlike the write side there is no global-right fallback: content is only reachable - // through a resolved page, so an unresolvable one has nothing to authorize. + // Content is only reachable through a resolved page, so an unresolvable page id has nothing + // to authorize — denied even for an authority that may read everything. $authorizer = new AuthorityBasedPageReadAuthorizer( $this->authorityThatAllowsEverything(), $this->titleFactoryReturningNull(), diff --git a/tests/phpunit/Infrastructure/AuthorityBasedSubjectAuthorizerTest.php b/tests/phpunit/Infrastructure/AuthorityBasedSubjectAuthorizerTest.php index cbbb14a2c..b4c5348a2 100644 --- a/tests/phpunit/Infrastructure/AuthorityBasedSubjectAuthorizerTest.php +++ b/tests/phpunit/Infrastructure/AuthorityBasedSubjectAuthorizerTest.php @@ -46,57 +46,31 @@ public function testEditIsAllowedWhenThePageCanBeEdited(): void { $this->assertTrue( $authorizer->canEditSubject( new PageId( self::PAGE_ID ) ) ); } - public function testFallsBackToGlobalEditRightWhenThePageCannotBeResolved(): void { + /** + * A Subject on no page this wiki has offers no page rights to check, so it is refused however much + * the caller may edit elsewhere. + */ + public function testDeniesUnresolvablePageEvenWithTheGlobalEditRight(): void { $authorizer = new AuthorityBasedSubjectAuthorizer( $this->authorityThatCanEditEveryPage(), $this->titleFactoryReturningNull() ); - $this->assertTrue( $authorizer->canEditSubject( new PageId( self::PAGE_ID ) ) ); - } - - public function testDeniesUnresolvablePageWhenUserLacksGlobalEditRight(): void { - $authorizer = new AuthorityBasedSubjectAuthorizer( - $this->authorityWithoutAnyPermissions(), - $this->titleFactoryReturningNull() - ); - $this->assertFalse( $authorizer->canEditSubject( new PageId( self::PAGE_ID ) ) ); } - public function testFallsBackToGlobalEditRightWhenNoPageIsGiven(): void { - $authorizer = $this->newAuthorizer( $this->authorityThatCanEditEveryPage() ); - - $this->assertTrue( $authorizer->canEditSubject( null ) ); - } - - public function testDeniesWhenNoPageIsGivenAndUserLacksGlobalEditRight(): void { - $authorizer = $this->newAuthorizer( $this->authorityWithoutAnyPermissions() ); - - $this->assertFalse( $authorizer->canEditSubject( null ) ); - } - public function testAuthorizeIsDeniedWhenThePageCannotBeEdited(): void { $authorizer = $this->newAuthorizer( $this->authorityWithGlobalEditButNoPageEdit() ); $this->assertFalse( $authorizer->authorize( new PageId( self::PAGE_ID ) ) ); } - public function testAuthorizeFallsBackToGlobalEditRightWhenThePageCannotBeResolved(): void { + public function testAuthorizeDeniesUnresolvablePageEvenWithTheGlobalEditRight(): void { $authorizer = new AuthorityBasedSubjectAuthorizer( $this->authorityThatCanEditEveryPage(), $this->titleFactoryReturningNull() ); - $this->assertTrue( $authorizer->authorize( new PageId( self::PAGE_ID ) ) ); - } - - public function testAuthorizeDeniesUnresolvablePageWhenUserLacksGlobalEditRight(): void { - $authorizer = new AuthorityBasedSubjectAuthorizer( - $this->authorityWithoutAnyPermissions(), - $this->titleFactoryReturningNull() - ); - $this->assertFalse( $authorizer->authorize( new PageId( self::PAGE_ID ) ) ); } @@ -139,12 +113,6 @@ private function authorityThatCanEditEveryPage(): Authority { return $this->mockRegisteredAuthority( $allowEverything ); } - private function authorityWithoutAnyPermissions(): Authority { - $denyEverything = static fn ( string $permission, ?PageIdentity $page = null ): bool => false; - - return $this->mockRegisteredAuthority( $denyEverything ); - } - private function titleFactoryReturningPage(): TitleFactory { $factory = $this->createStub( TitleFactory::class ); $factory->method( 'newFromID' )->willReturn( Title::makeTitle( NS_MAIN, 'Protected page' ) ); diff --git a/tests/phpunit/NoGraphBackendTest.php b/tests/phpunit/NoGraphBackendTest.php index d03ab5b8a..86e3dcbf8 100644 --- a/tests/phpunit/NoGraphBackendTest.php +++ b/tests/phpunit/NoGraphBackendTest.php @@ -9,20 +9,37 @@ use MediaWiki\Context\RequestContext; use MediaWiki\MediaWikiServices; use MediaWiki\Output\OutputPage; +use MediaWiki\Parser\ParserOptions; use MediaWiki\Title\Title; use ProfessionalWiki\NeoWiki\Application\NullSubjectLabelLookup; +use ProfessionalWiki\NeoWiki\Application\SubjectResolver; use ProfessionalWiki\NeoWiki\Domain\GraphDatabase\GraphBackendNotConfiguredException; +use ProfessionalWiki\NeoWiki\Domain\Schema\PropertyName; +use ProfessionalWiki\NeoWiki\Domain\Statement; +use ProfessionalWiki\NeoWiki\Domain\Subject\StatementList; +use ProfessionalWiki\NeoWiki\Domain\Subject\Subject; +use ProfessionalWiki\NeoWiki\Domain\Subject\SubjectId; +use ProfessionalWiki\NeoWiki\Domain\Value\StringValue; use ProfessionalWiki\NeoWiki\EntryPoints\NeoWikiHooks; +use ProfessionalWiki\NeoWiki\EntryPoints\Scribunto\SubjectDataLookup; use ProfessionalWiki\NeoWiki\GraphDatabasePlugins\Neo4j\Persistence\Neo4jSubjectLabelLookup; use ProfessionalWiki\NeoWiki\NeoWikiExtension; +use ProfessionalWiki\NeoWiki\Tests\Data\TestSubject; use TestLogger; /** + * A wiki with no graph backend configured is a supported mode: Subjects are stored, found, read and + * rendered without one, and only the query surfaces a backend brings are absent (#1040). + * * @covers \ProfessionalWiki\NeoWiki\NeoWikiExtension * @group Database */ class NoGraphBackendTest extends NeoWikiIntegrationTestCase { + private const SUBJECT_ID = 's1zz1111111azz1'; + private const PROPERTY = 'Motto'; + private const VALUE = 'Beyond the graph'; + public function testGetNeo4jPluginIsNullWithoutBackend(): void { $plugin = $this->runWithoutGraphBackend( static fn() => NeoWikiExtension::getInstance()->getNeo4jPlugin() @@ -48,6 +65,134 @@ public function testEditSucceedsWithoutBackend(): void { } ); } + public function testSubjectIsStoredAndFoundByIdWithoutBackend(): void { + $this->runWithoutGraphBackend( function (): void { + $this->createPageWithMottoSubject( 'NoBackendSubjectPage' ); + + $subject = NeoWikiExtension::getInstance()->getSubjectRepository() + ->getSubject( new SubjectId( self::SUBJECT_ID ) ); + + $this->assertNotNull( $subject ); + $this->assertSame( self::SUBJECT_ID, $subject->id->text ); + } ); + } + + public function testSubjectIsEditedThroughTheRepositoryWithoutBackend(): void { + $this->runWithoutGraphBackend( function (): void { + $this->createPageWithMottoSubject( 'NoBackendEditedSubjectPage' ); + $repository = NeoWikiExtension::getInstance()->getSubjectRepository(); + + $repository->updateSubject( + $this->mottoSubject( 'Rewritten' ), + 'no-backend subject edit' + ); + + $this->assertSame( 'Rewritten', $this->mottoOfIndexedSubject() ); + } ); + } + + public function testSubjectIsDeletedThroughTheRepositoryWithoutBackend(): void { + $this->runWithoutGraphBackend( function (): void { + $this->createPageWithMottoSubject( 'NoBackendDeletedSubjectPage' ); + $repository = NeoWikiExtension::getInstance()->getSubjectRepository(); + + $repository->deleteSubject( new SubjectId( self::SUBJECT_ID ), 'no-backend subject delete' ); + + $this->assertNull( $repository->getSubject( new SubjectId( self::SUBJECT_ID ) ) ); + } ); + } + + public function testValueParserFunctionReadsAValueWithoutBackend(): void { + $html = $this->runWithoutGraphBackend( function (): string { + $this->createPageWithMottoSubject( 'NoBackendValuePage' ); + + return $this->parse( + 'NoBackendValuePage', + '{{#neowiki_value: ' . self::PROPERTY . ' | subject=' . self::SUBJECT_ID . ' }}' + ); + } ); + + $this->assertStringContainsString( self::VALUE, $html ); + } + + /** + * The engine behind the mw.neowiki.* getters, reached by Subject id, which is the path that needed + * the reverse index. + */ + public function testLuaGetterReadsAValueBySubjectIdWithoutBackend(): void { + $value = $this->runWithoutGraphBackend( function (): array { + $this->createPageWithMottoSubject( 'NoBackendLuaPage' ); + $extension = NeoWikiExtension::getInstance(); + + return ( new SubjectDataLookup( + new SubjectResolver( $extension->newSubjectContentRepository(), $extension->getSubjectRepository() ) + ) )->getValue( + Title::newFromText( 'NoBackendLuaPage' ), + self::PROPERTY, + [ 'subject' => self::SUBJECT_ID ] + ); + } ); + + $this->assertSame( [ self::VALUE ], $value ); + } + + public function testContentPageRenderInjectsTheAppWithoutBackend(): void { + $out = $this->newContentPageOutput( 'NoBackendAppPage' ); + + $this->runWithoutGraphBackend( static function () use ( $out ): void { + NeoWikiHooks::onBeforePageDisplay( $out, $out->getSkin() ); + } ); + + $this->assertStringContainsString( 'ext-neowiki-app', $out->getHTML() ); + } + + public function testContentPageRenderInjectsTheAppWhenConfigured(): void { + $out = $this->newContentPageOutput( 'ConfiguredBackendViewPage' ); + + NeoWikiHooks::onBeforePageDisplay( $out, $out->getSkin() ); + + $this->assertStringContainsString( 'ext-neowiki-app', $out->getHTML() ); + } + + public function testContentPageRenderIsSilentWithoutBackend(): void { + $out = $this->newContentPageOutput( 'NoBackendQuietPage' ); + $logger = new TestLogger( true ); + $this->setLogger( 'NeoWiki', $logger ); + + $this->runWithoutGraphBackend( static function () use ( $out ): void { + NeoWikiHooks::onBeforePageDisplay( $out, $out->getSkin() ); + } ); + + $this->assertSame( [], $logger->getBuffer() ); + } + + /** + * Half a Neo4j configuration is not a supported mode but an unfinished one, so it is still reported. + */ + public function testHalfConfiguredNeo4jIsReported(): void { + $out = $this->newContentPageOutput( 'HalfConfiguredPage' ); + $logger = new TestLogger( true ); + $this->setLogger( 'NeoWiki', $logger ); + + $this->runWithoutGraphBackend( function () use ( $out ): void { + $this->overrideConfigValue( 'NeoWikiNeo4jInternalReadUrl', 'bolt://neo:7687' ); + NeoWikiExtension::resetInstance(); + + NeoWikiHooks::onBeforePageDisplay( $out, $out->getSkin() ); + } ); + + $this->assertStringContainsString( 'only one of the Neo4j read/write Bolt URLs', self::loggedText( $logger ) ); + } + + public function testRelationTargetSuggestionsAreEmptyWithoutBackend(): void { + $suggestions = $this->runWithoutGraphBackend( + static fn() => NeoWikiExtension::getInstance()->getSubjectLabelLookup() + ->getSubjectLabelsMatching( 'anything', 10, TestSubject::DEFAULT_SCHEMA_ID ) + ); + + $this->assertSame( [], $suggestions ); + } + public function testSubjectLabelLookupIsNullObjectWithoutBackend(): void { $lookup = $this->runWithoutGraphBackend( static fn() => NeoWikiExtension::getInstance()->getSubjectLabelLookup() @@ -85,45 +230,38 @@ public function testReadOnlyClientThrowsGraphBackendNotConfiguredExceptionWithou ); } - public function testContentPageRenderDoesNotFailWithoutBackend(): void { - $out = $this->newContentPageOutput( 'NoBackendViewPage' ); - - $this->runWithoutGraphBackend( static function () use ( $out ): void { - NeoWikiHooks::onBeforePageDisplay( $out, $out->getSkin() ); - } ); + private function createPageWithMottoSubject( string $pageName ): void { + $this->assertNotNull( $this->createPageWithSubjects( $pageName, $this->mottoSubject( self::VALUE ) ) ); + } - // The guard short-circuits before getNeoWikiAppHtml() injects the app div. - $this->assertStringNotContainsString( 'ext-neowiki-app', $out->getHTML() ); + private function mottoSubject( string $motto ): Subject { + return TestSubject::build( + id: self::SUBJECT_ID, + statements: new StatementList( [ + new Statement( new PropertyName( self::PROPERTY ), 'text', new StringValue( $motto ) ), + ] ) + ); } - public function testContentPageRenderInjectsAppDivWhenConfigured(): void { - $out = $this->newContentPageOutput( 'ConfiguredBackendViewPage' ); + private function mottoOfIndexedSubject(): ?string { + $subject = NeoWikiExtension::getInstance()->getSubjectRepository() + ->getSubject( new SubjectId( self::SUBJECT_ID ) ); - NeoWikiHooks::onBeforePageDisplay( $out, $out->getSkin() ); + $value = $subject?->getStatements()->getStatement( new PropertyName( self::PROPERTY ) )?->getValue(); - $this->assertStringContainsString( 'ext-neowiki-app', $out->getHTML() ); + return $value instanceof StringValue ? $value->toScalars()[0] : null; } - public function testContentPageRenderLogsWarningWithoutBackend(): void { - $out = $this->newContentPageOutput( 'NoBackendWarningPage' ); - - $logger = new TestLogger( true ); - $this->setLogger( 'NeoWiki', $logger ); + private function parse( string $pageName, string $wikitext ): string { + $parserOptions = ParserOptions::newFromAnon(); - $this->runWithoutGraphBackend( static function () use ( $out ): void { - NeoWikiHooks::onBeforePageDisplay( $out, $out->getSkin() ); - } ); - - $buffer = $logger->getBuffer(); - $this->assertCount( 1, $buffer ); - $this->assertSame( 'warning', $buffer[0][0] ); - $this->assertStringContainsString( 'no graph database backend configured', $buffer[0][1] ); + return $this->getServiceContainer()->getParserFactory()->getInstance()->parse( + $wikitext, + Title::newFromText( $pageName ), + $parserOptions + )->runOutputPipeline( $parserOptions, [] )->getContentHolderText(); } - /** - * An edit-capable user on the latest revision triggers the subject-creator path, which builds the - * SubjectRepository (the Neo4j-backed reverse index) — the exact path that 500s without the guard. - */ private function newContentPageOutput( string $pageName ): OutputPage { $page = $this->getExistingTestPage( $pageName ); diff --git a/tests/phpunit/Persistence/RebuildRunsSchemaTest.php b/tests/phpunit/Persistence/DatabaseSchemaTest.php similarity index 66% rename from tests/phpunit/Persistence/RebuildRunsSchemaTest.php rename to tests/phpunit/Persistence/DatabaseSchemaTest.php index e1f5e6d63..150c614a0 100644 --- a/tests/phpunit/Persistence/RebuildRunsSchemaTest.php +++ b/tests/phpunit/Persistence/DatabaseSchemaTest.php @@ -7,7 +7,6 @@ use GenerateSchemaChangeSql; use GenerateSchemaSql; use MediaWikiIntegrationTestCase; -use ProfessionalWiki\NeoWiki\Domain\GraphDatabase\GraphStoreName; /** * The per-DBMS SQL that update.php applies is generated from the abstract schema and committed @@ -16,40 +15,18 @@ * * @coversNothing */ -class RebuildRunsSchemaTest extends MediaWikiIntegrationTestCase { +class DatabaseSchemaTest extends MediaWikiIntegrationTestCase { /** - * The longest name a store may be called and the width of the column its runs are filed under are - * declared in two places that know nothing about each other. Narrowing the column alone would leave - * accepted names the records cannot hold whole, and every lookup for such a store would then match - * nothing. Read off the abstract schema rather than off a per-DBMS file, so this holds wherever the - * suite runs — only MySQL materialises the width at all. + * @dataProvider tableProvider */ - public function testTheStoreNameLimitIsTheWidthOfTheColumnItIsFiledUnder(): void { - $schema = json_decode( - (string)file_get_contents( dirname( __DIR__, 3 ) . '/sql/neowiki_rebuild_runs.json' ), - true - ); - - $columns = array_column( $schema[0]['columns'], null, 'name' ); - - $this->assertSame( - GraphStoreName::MAX_LENGTH, - $columns['nwrr_store']['type'] === 'binary' ? $columns['nwrr_store']['options']['length'] : null, - 'GraphStoreName::MAX_LENGTH and the nwrr_store column width have to agree' - ); - } - - /** - * @dataProvider databaseTypeProvider - */ - public function testGeneratedSqlMatchesTheAbstractSchema( string $databaseType ): void { + public function testGeneratedSqlMatchesTheAbstractSchema( string $table, string $databaseType ): void { $extensionPath = dirname( __DIR__, 3 ); $generatedPath = $this->getNewTempFile(); $script = new GenerateSchemaSql(); $script->loadWithArgv( [ - '--json=' . $extensionPath . '/sql/neowiki_rebuild_runs.json', + '--json=' . $extensionPath . '/sql/' . $table . '.json', '--sql=' . $generatedPath, '--type=' . $databaseType, '--quiet', @@ -58,10 +35,10 @@ public function testGeneratedSqlMatchesTheAbstractSchema( string $databaseType ) $this->assertSame( self::withoutSourcePath( (string)file_get_contents( - $extensionPath . '/sql/' . $databaseType . '/neowiki_rebuild_runs.sql' + $extensionPath . '/sql/' . $databaseType . '/' . $table . '.sql' ) ), self::withoutSourcePath( (string)file_get_contents( $generatedPath ) ), - 'run `make dbschema` to regenerate the ' . $databaseType . ' schema' + 'run `make dbschema` to regenerate the ' . $databaseType . ' schema of ' . $table ); } @@ -93,6 +70,15 @@ public function testGeneratedPhasePatchMatchesTheAbstractSchemaChange( string $d ); } + public function tableProvider(): iterable { + yield 'rebuild runs, mysql' => [ 'neowiki_rebuild_runs', 'mysql' ]; + yield 'rebuild runs, sqlite' => [ 'neowiki_rebuild_runs', 'sqlite' ]; + yield 'rebuild runs, postgres' => [ 'neowiki_rebuild_runs', 'postgres' ]; + yield 'subject page, mysql' => [ 'neowiki_subject_page', 'mysql' ]; + yield 'subject page, sqlite' => [ 'neowiki_subject_page', 'sqlite' ]; + yield 'subject page, postgres' => [ 'neowiki_subject_page', 'postgres' ]; + } + public function databaseTypeProvider(): iterable { yield 'mysql' => [ 'mysql' ]; yield 'sqlite' => [ 'sqlite' ]; diff --git a/tests/phpunit/Persistence/MediaWiki/DatabasePageIdentifiersLookupTest.php b/tests/phpunit/Persistence/MediaWiki/DatabasePageIdentifiersLookupTest.php new file mode 100644 index 000000000..240d35001 --- /dev/null +++ b/tests/phpunit/Persistence/MediaWiki/DatabasePageIdentifiersLookupTest.php @@ -0,0 +1,115 @@ +createPage( 'Help:Indexed page' ); + $this->index( self::SUBJECT_ID, $pageId ); + + $identifiers = $this->newLookup()->getPageIdOfSubject( new SubjectId( self::SUBJECT_ID ) ); + + $this->assertNotNull( $identifiers ); + $this->assertSame( $pageId, $identifiers->getId()->id ); + $this->assertSame( 'Help:Indexed page', $identifiers->getTitle() ); + $this->assertSame( NS_HELP, $identifiers->getNamespaceId() ); + } + + public function testUnindexedSubjectResolvesToNothing(): void { + $this->index( self::SUBJECT_ID, $this->createPage( 'Indexed page' ) ); + + $this->assertNull( + $this->newLookup()->getPageIdOfSubject( new SubjectId( self::UNINDEXED_SUBJECT_ID ) ) + ); + } + + /** + * A row naming a page the wiki does not have has nothing to join, which is what makes the rows a + * deletion the index was not told about leaves behind inert rather than misleading. + */ + public function testSubjectOfAMissingPageResolvesToNothing(): void { + $this->index( self::SUBJECT_ID, self::MISSING_PAGE_ID ); + + $this->assertNull( + $this->newLookup()->getPageIdOfSubject( new SubjectId( self::SUBJECT_ID ) ) + ); + } + + /** + * Cross-wiki transfer can bring the same Subject id onto a second page (ADR 5), which must not fail + * either page's save. The id then resolves to the lowest page id, so every reader gets the same page. + */ + public function testDuplicateSubjectResolvesToTheLowestPageId(): void { + $firstPageId = $this->createPage( 'First holder' ); + $secondPageId = $this->createPage( 'Second holder' ); + $this->index( self::SUBJECT_ID, $secondPageId ); + $this->index( self::SUBJECT_ID, $firstPageId ); + + $identifiers = $this->newLookup()->getPageIdOfSubject( new SubjectId( self::SUBJECT_ID ) ); + + $this->assertNotNull( $identifiers ); + $this->assertSame( $firstPageId, $identifiers->getId()->id ); + } + + public function testSubjectsAreLookedUpTogether(): void { + $firstPageId = $this->createPage( 'First page' ); + $secondPageId = $this->createPage( 'Second page' ); + $this->index( self::SUBJECT_ID, $firstPageId ); + $this->index( self::OTHER_SUBJECT_ID, $secondPageId ); + + $identifiers = $this->newLookup()->getPageIdsOfSubjects( new SubjectIdList( [ + new SubjectId( self::SUBJECT_ID ), + new SubjectId( self::OTHER_SUBJECT_ID ), + new SubjectId( self::UNINDEXED_SUBJECT_ID ), + ] ) ); + + $this->assertSame( + [ self::SUBJECT_ID => $firstPageId, self::OTHER_SUBJECT_ID => $secondPageId ], + array_map( static fn ( $item ): int => $item->getId()->id, $identifiers ) + ); + } + + public function testNoSubjectsAreLookedUpAtAll(): void { + $this->assertSame( [], $this->newLookup()->getPageIdsOfSubjects( new SubjectIdList( [] ) ) ); + } + + private function newLookup(): PageIdentifiersLookup { + return new DatabasePageIdentifiersLookup( + $this->getDb(), + $this->getServiceContainer()->getTitleFormatter() + ); + } + + private function createPage( string $title ): int { + return $this->getExistingTestPage( Title::newFromText( $title ) )->getId(); + } + + private function index( string $subjectId, int $pageId ): void { + $this->getDb()->newInsertQueryBuilder() + ->insertInto( DatabaseSubjectPageIndex::TABLE ) + ->row( [ 'nwsp_subject_id' => $subjectId, 'nwsp_page_id' => $pageId ] ) + ->caller( __METHOD__ ) + ->execute(); + } + +} diff --git a/tests/phpunit/Persistence/MediaWiki/Subject/MediaWikiSubjectRepositoryTest.php b/tests/phpunit/Persistence/MediaWiki/Subject/MediaWikiSubjectRepositoryTest.php index bc88a2508..9f8b7de58 100644 --- a/tests/phpunit/Persistence/MediaWiki/Subject/MediaWikiSubjectRepositoryTest.php +++ b/tests/phpunit/Persistence/MediaWiki/Subject/MediaWikiSubjectRepositoryTest.php @@ -20,7 +20,6 @@ /** * @covers \ProfessionalWiki\NeoWiki\Persistence\MediaWiki\Subject\MediaWikiSubjectRepository - * @covers \ProfessionalWiki\NeoWiki\GraphDatabasePlugins\Neo4j\Persistence\Neo4jPageIdentifiersLookup * @group Database */ class MediaWikiSubjectRepositoryTest extends NeoWikiIntegrationTestCase { diff --git a/tests/phpunit/SubjectPageIndexTest.php b/tests/phpunit/SubjectPageIndexTest.php new file mode 100644 index 000000000..991cfbbd2 --- /dev/null +++ b/tests/phpunit/SubjectPageIndexTest.php @@ -0,0 +1,385 @@ + page index over the page lifecycle: what a wiki operation leaves the index holding, + * and what a Subject id then resolves to. + * + * @covers \ProfessionalWiki\NeoWiki\Persistence\MediaWiki\DatabaseSubjectPageIndex + * @covers \ProfessionalWiki\NeoWiki\Persistence\MediaWiki\DatabaseSubjectPageIndexRebuilder + * @covers \ProfessionalWiki\NeoWiki\EntryPoints\OnRevisionCreatedHandler + * @group Database + */ +class SubjectPageIndexTest extends NeoWikiIntegrationTestCase { + + private const FIRST_ID = 's1zz1111111azz1'; + private const SECOND_ID = 's1zz1111111azz2'; + private const THIRD_ID = 's1zz1111111azz3'; + + public function testCreatingAPageIndexesTheSubjectsItHolds(): void { + $pageId = $this->createPageHolding( 'Created page', self::FIRST_ID, self::SECOND_ID ); + + $this->assertSame( [ self::FIRST_ID, self::SECOND_ID ], $this->indexedSubjectsOf( $pageId ) ); + } + + public function testAPageWithoutSubjectsIndexesNothing(): void { + $page = $this->getExistingTestPage( 'Plain page' ); + + $this->assertSame( [], $this->indexedSubjectsOf( $page->getId() ) ); + } + + public function testAddingASubjectIndexesIt(): void { + $pageId = $this->createPageHolding( 'Growing page', self::FIRST_ID ); + + $this->createPageHolding( 'Growing page', self::FIRST_ID, self::SECOND_ID ); + + $this->assertSame( [ self::FIRST_ID, self::SECOND_ID ], $this->indexedSubjectsOf( $pageId ) ); + } + + public function testRemovingASubjectUnindexesIt(): void { + $pageId = $this->createPageHolding( 'Shrinking page', self::FIRST_ID, self::SECOND_ID ); + + $this->createPageHolding( 'Shrinking page', self::FIRST_ID ); + + $this->assertSame( [ self::FIRST_ID ], $this->indexedSubjectsOf( $pageId ) ); + } + + public function testDeletingAPageUnindexesItsSubjects(): void { + $pageId = $this->createPageHolding( 'Doomed page', self::FIRST_ID ); + + $this->deletePageByName( 'Doomed page' ); + + $this->assertSame( [], $this->indexedSubjectsOf( $pageId ) ); + } + + /** + * Imported revisions reach the index through AfterImportPage, which every import path fires, so an + * imported Subject is findable without a rebuild. + */ + public function testImportingAPageIndexesTheSubjectsItHolds(): void { + $this->createPageHolding( 'Exported page', self::FIRST_ID ); + $xml = $this->exportPageToXml( 'Exported page' ); + $this->deletePageByName( 'Exported page' ); + + $this->importXml( $xml ); + + $identifiers = $this->newLookup()->getPageIdOfSubject( new SubjectId( self::FIRST_ID ) ); + $this->assertNotNull( $identifiers ); + $this->assertSame( 'Exported page', $identifiers->getTitle() ); + } + + /** + * The title comes from the page table on every read, so a move leaves the index untouched and the + * Subject resolves to where the page now is. + */ + public function testMovingAPageLeavesItsSubjectsResolvingToIt(): void { + $pageId = $this->createPageHolding( 'Before the move', self::FIRST_ID ); + + $this->movePage( 'Before the move', 'After the move' ); + + $identifiers = $this->newLookup()->getPageIdOfSubject( new SubjectId( self::FIRST_ID ) ); + $this->assertNotNull( $identifiers ); + $this->assertSame( $pageId, $identifiers->getId()->id ); + $this->assertSame( 'After the move', $identifiers->getTitle() ); + } + + /** + * Restoring archived revisions onto a page that has since been recreated leaves a newer revision + * current, so the page is indexed from what it holds now rather than from the revision restored. + */ + public function testPartiallyUndeletingAPageIndexesWhatItCurrentlyHolds(): void { + $this->createPageHolding( 'Recreated page', self::FIRST_ID ); + $this->deletePageByName( 'Recreated page' ); + $pageId = $this->createPageHolding( 'Recreated page', self::SECOND_ID ); + + $this->undeleteArchivedRevisions( 'Recreated page' ); + + $this->assertSame( [ self::SECOND_ID ], $this->indexedSubjectsOf( $pageId ) ); + } + + public function testUndeletingADeletedPageIndexesItsSubjectsAgain(): void { + $pageId = $this->createPageHolding( 'Restored page', self::FIRST_ID ); + $this->deletePageByName( 'Restored page' ); + + $this->undeleteArchivedRevisions( 'Restored page' ); + + $this->assertSame( [ self::FIRST_ID ], $this->indexedSubjectsOf( $pageId ) ); + } + + /** + * The rebuild is the repair path for writes MediaWiki gives no usable hook for, so it indexes a page + * whose Subjects nothing told the index about. + */ + public function testRebuildingIndexesAPageWrittenWithoutTheHook(): void { + $pageId = $this->createPageWithoutTellingNeoWiki( + 'Unhooked page', + $this->subjectSlotJson( self::FIRST_ID ) + ); + + $this->rebuildIndex(); + + $this->assertSame( [ self::FIRST_ID ], $this->indexedSubjectsOf( $pageId ) ); + } + + /** + * A Subject too broken to deserialize is a persisted, supported state, and the index is what gets an + * editor to the page holding it — so its id is read from the raw JSON and indexed like any other. + */ + public function testRebuildingIndexesASubjectThatDoesNotDeserialize(): void { + $json = TestSubject::jsonThatDoesNotDeserialize( self::FIRST_ID ); + $this->assertDoesNotDeserialize( $json ); + + $pageId = $this->createPageWithoutTellingNeoWiki( 'Broken subject page', $json ); + + $this->rebuildIndex(); + + $this->assertSame( [ self::FIRST_ID ], $this->indexedSubjectsOf( $pageId ) ); + } + + /** + * The walk is batched, so it has to carry on past the first batch and reach every page holding + * Subjects — not only the ones the first query returned. + */ + public function testRebuildingReachesEveryPageAcrossBatches(): void { + $firstPageId = $this->createPageWithoutTellingNeoWiki( 'Batched page 1', $this->subjectSlotJson( self::FIRST_ID ) ); + $secondPageId = $this->createPageWithoutTellingNeoWiki( 'Batched page 2', $this->subjectSlotJson( self::SECOND_ID ) ); + $thirdPageId = $this->createPageWithoutTellingNeoWiki( 'Batched page 3', $this->subjectSlotJson( self::THIRD_ID ) ); + + $this->rebuildIndex( batchSize: 1 ); + + $this->assertSame( [ self::FIRST_ID ], $this->indexedSubjectsOf( $firstPageId ) ); + $this->assertSame( [ self::SECOND_ID ], $this->indexedSubjectsOf( $secondPageId ) ); + $this->assertSame( [ self::THIRD_ID ], $this->indexedSubjectsOf( $thirdPageId ) ); + } + + public function testRebuildingUnindexesAPageDeletedWithoutTheHook(): void { + $pageId = $this->createPageHolding( 'Silently deleted page', self::FIRST_ID ); + $this->clearHook( 'PageDeleteComplete' ); + $this->deletePageByName( 'Silently deleted page' ); + + $this->rebuildIndex(); + + $this->assertSame( [], $this->indexedSubjectsOf( $pageId ) ); + } + + public function testRebuildingUnindexesAPageThatLostItsSubjectsWithoutTheHook(): void { + $pageId = $this->createPageHolding( 'Emptied page', self::FIRST_ID ); + $this->createPageWithoutTellingNeoWiki( 'Emptied page', $this->subjectSlotJson() ); + + $this->rebuildIndex(); + + $this->assertSame( [], $this->indexedSubjectsOf( $pageId ) ); + } + + /** + * A page that lost the slot itself is not reached by the walk at all, so only the sweep can drop + * what it used to hold. + */ + public function testRebuildingUnindexesAPageThatLostItsSubjectSlotWithoutTheHook(): void { + $pageId = $this->createPageHolding( 'Slotless page', self::FIRST_ID ); + $this->removeSubjectSlotWithoutTellingNeoWiki( 'Slotless page' ); + + $this->rebuildIndex(); + + $this->assertSame( [], $this->indexedSubjectsOf( $pageId ) ); + } + + /** + * A slot holding content that is not Subject data cannot be read as Subjects, so the page is left as + * it is rather than reindexed as holding none — which would drop the Subjects it does hold. The hook + * path skips such a page for the same reason. + */ + public function testRebuildingLeavesAPageWhoseSlotDoesNotHoldSubjectContentAlone(): void { + $pageId = $this->createPageHolding( 'Unreadable slot page', self::FIRST_ID ); + + $this->runRebuild( + $this->revisionLookupWithSlotContent( new FallbackContent( '{"subjects":{}}', 'unregistered-model' ) ), + DatabaseSubjectPageIndexRebuilder::DEFAULT_BATCH_SIZE + ); + + $this->assertSame( [ self::FIRST_ID ], $this->indexedSubjectsOf( $pageId ) ); + } + + /** + * A graph rebuild walks the wiki from a replica, so the revision it projects may already have been + * superseded. The index is not allowed to lag that way, so reprojecting a store leaves it alone. + */ + public function testRebuildingAGraphStoreDoesNotWriteTheIndex(): void { + $pageId = $this->createPageHolding( 'Reprojected page', self::FIRST_ID ); + $this->emptyIndexOf( $pageId ); + + $outcome = NeoWikiExtension::getInstance() + ->newPageRebuilderFor( new SpyGraphDatabasePlugin() ) + ->rebuild( Title::newFromText( 'Reprojected page' ) ); + + $this->assertSame( PageRefreshOutcome::Refreshed, $outcome, 'the page should have been reprojected' ); + $this->assertSame( [], $this->indexedSubjectsOf( $pageId ) ); + } + + private function revisionLookupWithSlotContent( Content $content ): RevisionLookup { + $revision = $this->createStub( RevisionRecord::class ); + $revision->method( 'hasSlot' )->willReturn( true ); + $revision->method( 'getContent' )->willReturn( $content ); + + $revisionLookup = $this->createStub( RevisionLookup::class ); + $revisionLookup->method( 'getRevisionByPageId' )->willReturn( $revision ); + + return $revisionLookup; + } + + private function emptyIndexOf( int $pageId ): void { + $this->getDb()->newDeleteQueryBuilder() + ->deleteFrom( DatabaseSubjectPageIndex::TABLE ) + ->where( [ 'nwsp_page_id' => $pageId ] ) + ->caller( __METHOD__ ) + ->execute(); + + $this->assertSame( [], $this->indexedSubjectsOf( $pageId ) ); + } + + private function assertDoesNotDeserialize( string $subjectJson ): void { + try { + ( new SubjectContent( $subjectJson ) )->getPageSubjects(); + } catch ( Throwable ) { + return; + } + + $this->fail( 'Expected Subject data too broken to deserialize' ); + } + + private function createPageHolding( string $pageName, string ...$subjectIds ): int { + $subjects = array_map( + static fn ( string $id ) => TestSubject::build( id: $id, label: 'Subject ' . $id ), + $subjectIds + ); + + $revision = $this->createPageWithSubjects( + $pageName, + array_shift( $subjects ), + new SubjectMap( ...$subjects ) + ); + + $this->assertNotNull( $revision ); + + return $revision->getPageId(); + } + + /** + * Saves a revision with NeoWiki's revision hook removed, which is the shape of the write it is not + * told about: a history merge that leaves the source page as a redirect. + */ + private function createPageWithoutTellingNeoWiki( string $pageName, string $subjectJson ): int { + $this->clearHook( 'RevisionFromEditComplete' ); + + $wikiPage = MediaWikiServices::getInstance()->getWikiPageFactory() + ->newFromTitle( Title::newFromText( $pageName ) ); + + $updater = $wikiPage->newPageUpdater( $this->getTestSysop()->getUser() ); + $updater->setContent( 'main', new TextContent( '' ) ); + $updater->setContent( MediaWikiSubjectRepository::SLOT_NAME, new SubjectContent( $subjectJson ) ); + + $revision = $updater->saveRevision( CommentStoreComment::newUnsavedComment( 'unhooked write' ) ); + $this->assertNotNull( $revision ); + + return $revision->getPageId(); + } + + private function removeSubjectSlotWithoutTellingNeoWiki( string $pageName ): void { + $this->clearHook( 'RevisionFromEditComplete' ); + + $updater = MediaWikiServices::getInstance()->getWikiPageFactory() + ->newFromTitle( Title::newFromText( $pageName ) ) + ->newPageUpdater( $this->getTestSysop()->getUser() ); + + $updater->removeSlot( MediaWikiSubjectRepository::SLOT_NAME ); + + $this->assertNotNull( + $updater->saveRevision( CommentStoreComment::newUnsavedComment( 'unhooked slot removal' ) ) + ); + } + + private function subjectSlotJson( ?string $subjectId = null ): string { + if ( $subjectId === null ) { + return SubjectContent::newFromData( PageSubjects::newEmpty() )->getText(); + } + + return SubjectContent::newFromData( + new PageSubjects( TestSubject::build( id: $subjectId ), new SubjectMap() ) + )->getText(); + } + + private function rebuildIndex( int $batchSize = DatabaseSubjectPageIndexRebuilder::DEFAULT_BATCH_SIZE ): void { + $this->runRebuild( $this->getServiceContainer()->getRevisionLookup(), $batchSize ); + } + + private function runRebuild( RevisionLookup $revisionLookup, int $batchSize ): void { + $rebuilder = new DatabaseSubjectPageIndexRebuilder( $this->getDb(), $revisionLookup, $batchSize ); + + iterator_to_array( $rebuilder->rebuild() ); + } + + private function newLookup(): DatabasePageIdentifiersLookup { + return new DatabasePageIdentifiersLookup( + $this->getDb(), + $this->getServiceContainer()->getTitleFormatter() + ); + } + + /** + * @return string[] The Subject ids the index holds for the page, in id order. + */ + private function indexedSubjectsOf( int $pageId ): array { + return $this->getDb()->newSelectQueryBuilder() + ->select( 'nwsp_subject_id' ) + ->from( DatabaseSubjectPageIndex::TABLE ) + ->where( [ 'nwsp_page_id' => $pageId ] ) + ->orderBy( 'nwsp_subject_id' ) + ->caller( __METHOD__ ) + ->fetchFieldValues(); + } + + private function movePage( string $from, string $to ): void { + $this->assertStatusGood( + $this->getServiceContainer()->getMovePageFactory()->newMovePage( + Title::newFromText( $from ), + Title::newFromText( $to ) + )->move( $this->getTestSysop()->getUser(), 'test move', false ) + ); + } + + private function undeleteArchivedRevisions( string $pageName ): void { + $this->assertStatusGood( + $this->getServiceContainer()->getUndeletePageFactory()->newUndeletePage( + MediaWikiServices::getInstance()->getWikiPageFactory() + ->newFromTitle( Title::newFromText( $pageName ) )->getTitle()->toPageIdentity(), + $this->getTestSysop()->getAuthority() + )->undeleteUnsafe( 'test restore' ) + ); + } + +} diff --git a/tests/phpunit/TestDoubles/SpySubjectPageIndex.php b/tests/phpunit/TestDoubles/SpySubjectPageIndex.php new file mode 100644 index 000000000..4e79b32a8 --- /dev/null +++ b/tests/phpunit/TestDoubles/SpySubjectPageIndex.php @@ -0,0 +1,33 @@ + The Subject ids each page was indexed with, keyed by page id + */ + public array $indexedSubjectsByPageId = []; + + /** + * @var int[] + */ + public array $removedPageIds = []; + + /** + * @param string[] $subjectIds + */ + public function setSubjectsOfPage( PageId $pageId, array $subjectIds ): void { + $this->indexedSubjectsByPageId[$pageId->id] = $subjectIds; + } + + public function removePage( PageId $pageId ): void { + $this->removedPageIds[] = $pageId->id; + } + +} diff --git a/tests/phpunit/TestDoubles/SpySubjectWriteAuthorizer.php b/tests/phpunit/TestDoubles/SpySubjectWriteAuthorizer.php index d042b4f32..9e700699f 100644 --- a/tests/phpunit/TestDoubles/SpySubjectWriteAuthorizer.php +++ b/tests/phpunit/TestDoubles/SpySubjectWriteAuthorizer.php @@ -19,7 +19,7 @@ public function __construct( ) { } - public function authorize( ?PageId $pageId ): bool { + public function authorize( PageId $pageId ): bool { $this->authorizedPageId = $pageId; return $this->allowed; } diff --git a/tests/phpunit/TestDoubles/ThrowingSubjectPageIndex.php b/tests/phpunit/TestDoubles/ThrowingSubjectPageIndex.php new file mode 100644 index 000000000..eaf224dd8 --- /dev/null +++ b/tests/phpunit/TestDoubles/ThrowingSubjectPageIndex.php @@ -0,0 +1,25 @@ +