Document internal surfaces and the PHP Cypher query path#1045
Conversation
7f1c20e to
140b1d9
Compare
Extensions have started building on two surfaces that are reachable but internal: the rendered .ext-neowiki-view placeholders and the raw Neo4j client. NeoWiki is pre-1.0 and cannot yet declare a stable interface, so the extending docs gain an "Internal surfaces" section: the rendered HTML is marked as not an integration surface, while the internal client gets a comparison of what the documented query interfaces provide over it (read-only enforcement, resource limits, result handling, churn exposure) rather than a prohibition, plus a warning that direct graph writes do not survive re-projection or rebuilds. Since the alternative for PHP-side querying was undocumented, add a "Running Cypher queries" subsection covering newCypherQueryService(), backed by a new RedHerb example (SpecialRedHerbContentPageCount) with an integration test against real Neo4j, so the documented call shape stays working like the page's other examples. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
140b1d9 to
2ec837d
Compare
The tier mechanics are documented on the linked Query API page; naming the right here duplicated that and it is default MediaWiki behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
alistair3149
left a comment
There was a problem hiding this comment.
Reviewed the docs claims against source, plus the new example and its test. Findings inline: one contradiction with an existing RedHerb example, the rest precision issues in the new prose.
Verified locally at f4f4109: make phpunit filter=SpecialRedHerbContentPageCount and make cs both pass. I also mutation-tested the new test — dropping the WHERE page.namespaceId = $namespaceId filter makes the real Neo4j count return 3 instead of 2, so it genuinely exercises the graph and the filter is load-bearing.
Two things I checked and am deliberately not raising, to save anyone re-treading them:
- The example query's missing
wiki_idfilter.docs/reference/graph-model.md:50-53explicitly blesses filtering built-in namespaces likeNS_MAINwithout it ("Built-in namespaces ... have the same ID on every wiki"), thewiki_idpairing advice there is scoped to custom$wgExtraNamespacesnamespaces, and no example query anywhere indocs/filterswiki_id— including theMATCH (s:Subject:Person)one this PR adds. - The test's unpinned Help-namespace discriminator.
tests/phpunit/EntryPoints/NamespacedPageGraphProjectionTest.php:52-59already pinsNS_HELPprojection, and the mutation test above confirms the assertion catches the regression today.
AI-assisted review — Claude Code, Opus 4.8. Docs claims checked against source by adversarial review subagents (findings that survived a majority-refute vote); every file:line citation below re-verified against source by hand, and the test claims verified by running them locally.
|
|
||
| The `.ext-neowiki-view` placeholder elements and `data-mw-neowiki-*` attributes are the private contract | ||
| between NeoWiki's backend and its frontend for mounting Views. They are not an integration surface: do not | ||
| select these elements, read Subject IDs out of them, restyle their internals, or remove and replace them |
There was a problem hiding this comment.
This forbids exactly what the repo's own copy-me example does.
tests/RedHerb/resources/editMainSubject/init.js:11:
const MAIN_SUBJECT_SELECTOR = '.ext-neowiki-view[data-mw-neowiki-subject-id]';and :34/:38:
const el = document.querySelector( MAIN_SUBJECT_SELECTOR );
...
return el.dataset.mwNeowikiSubjectId || null;That is "select these elements" and "read Subject IDs out of them" — both named in this sentence.
It isn't a distant contradiction. Lines 16-17 say "the fastest start is to copy the relevant RedHerb file and adapt it"; line 352 links resources/editMainSubject/; this section's own escape-hatch bullet routes readers to "Mounting standalone Vue features", where that example is listed. tests/RedHerb/README.md:37 links it too. And line 15 sells RedHerb as test-backed so "its examples stay working", which gives a reader no signal that this one is the forbidden pattern.
editMainSubject is the only RedHerb example doing this — createChild and subjectFinder are clean. Either carve it out by name here, or rework that example onto a supported path.
| ([`{{#cypher_raw}}`](../authoring/parser-functions.md), [`nw.query`](../authoring/lua-api.md), the | ||
| [Query API](../api/query-api.md), and the [PHP query service](#running-cypher-queries)) provide over it: | ||
|
|
||
| - **Read-only enforcement.** The query interfaces reject write queries (keyword check plus `EXPLAIN`); |
There was a problem hiding this comment.
"reject write queries" — and "It rejects write queries" at line 196 — understates the validator. KeywordCypherQueryValidator::WRITE_KEYWORDS also contains CALL and SHOW:
private const array WRITE_KEYWORDS = [
'CREATE', 'SET', 'DELETE', 'REMOVE', 'MERGE', 'DROP',
'CALL', 'LOAD', 'FOREACH',
'GRANT', 'DENY', 'REVOKE',
'SHOW',
];So a genuinely read-only CALL {} subquery, db.labels(), or SHOW INDEXES throws WriteQueryRejectedException( 'Query is not read-only.' ). Concretely, this reads nothing but nodes and is rejected:
MATCH (p:Page) CALL { WITH p MATCH (p)-[:HasSubject]->(s:Subject) RETURN count(s) AS c } RETURN p.name, cSince this bullet lists read-only enforcement as pure upside over the raw client, the reader's rational conclusion is that their query is a write — or that they must drop to the internal client this section just forbade.
The caveat exists exactly once in the repo, at docs/authoring/parser-functions.md:148 ("including CALL (even for read-only procedures)"). Worth naming CALL/SHOW here, or linking that note.
| - **Read-only enforcement.** The query interfaces reject write queries (keyword check plus `EXPLAIN`); | ||
| with the raw client, a bug in calling code can corrupt the graph projection, which is what ADR 13 | ||
| exists to prevent. | ||
| - **Resource limits.** The query interfaces apply a configured timeout and row cap; the raw client runs |
There was a problem hiding this comment.
Only the timeout is enforced against the backend; maxRows never reaches the driver.
Neo4jClientReadQueryEngine::runReadQuery() passes just the timeout:
$transactionConfig = $timeoutSeconds === null
? null
: TransactionConfiguration::default()->withTimeout( (float)$timeoutSeconds );No LIMIT injection, no row bound. maxRows appears only in Neo4jQueryLimits and in Neo4jQueryService::buildResult(), which materializes the full SummarizedResult, counts it, normalizes every row via convertRows(), and only then array_slice()s to maxRows.
So against "the raw client runs unbounded queries", an author who ships MATCH (s:Subject) RETURN s trusting the 5,000-row default gets a PHP memory-exhaustion fatal on a large graph — not 5,000 rows with truncated: true. That is the same outcome the raw client would give, so the row cap isn't the protection this bullet implies. docs/api/query-api.md:215 already frames it accurately: "the result was cut at maxRows".
Suggest splitting the claim: the timeout is enforced; the row cap truncates the returned result, and callers should still put LIMIT in the Cypher. Same for "enforces the timeout and row cap" at line 196.
| Writing to the graph directly deserves particular caution: the graph is a projection of wiki content that | ||
| NeoWiki rewrites at will. Saving a page re-projects that page's nodes, and the `RebuildGraphDatabases` | ||
| maintenance script rebuilds the projection from scratch, so anything a third party writes into the graph | ||
| can be overwritten, orphaned, or deleted at any time. To contribute data to the graph durably, use |
There was a problem hiding this comment.
Page Property Providers don't cover the case this paragraph is about.
The paragraph's subject is an extension reaching for the raw client to write arbitrary nodes and relationships. But PagePropertyProvider::getProperties() returns array<string, mixed>, merged onto a single Page node:
/**
* @return array<string, mixed>
*/
public function getProperties( PagePropertyProviderContext $context ): array;No way to create a node, create a relationship, or attach anything to a Subject. A reader whose case is "create a node per external record and link it to the Subjects that reference it" follows this pointer and hits a dead end — for the exact case that motivated reaching for the raw client.
Suggest scoping the sentence (e.g. "for page-level key/value metadata, use Page Property Providers") and saying what covers the rest. Worth noting the honest answer is a bit awkward: Graph Database Backends (line 146) projects arbitrary structure durably, but into your own store alongside Neo4j — so NeoWiki's own Neo4j graph genuinely has no durable third-party write path for arbitrary nodes. If that's the intended message, it'd be stronger stated outright than implied.
| NeoWiki rewrites at will. Saving a page re-projects that page's nodes, and the `RebuildGraphDatabases` | ||
| maintenance script rebuilds the projection from scratch, so anything a third party writes into the graph | ||
| can be overwritten, orphaned, or deleted at any time. To contribute data to the graph durably, use | ||
| [Page Property Providers](#page-property-providers), which NeoWiki re-runs on every save and rebuild. |
There was a problem hiding this comment.
"on every save and rebuild" needs a qualifier: both paths only reach pages carrying the NeoWiki subject slot.
OnRevisionCreatedHandler::storeRevisionRecord() returns before PagePropertiesBuilder is ever called:
$neoContent = $this->getNeoContent( $revisionRecord );
if ( $neoContent === null ) {
return false;
}with getNeoContent() returning null when !$revisionRecord->hasSlot( MediaWikiSubjectRepository::SLOT_NAME ). The rebuild doesn't rescue it either: RebuildGraphDatabases::getSubjectPageIds() joins slots on slot_role_id, docblocked "Page IDs of every page whose latest revision carries the NeoWiki subject slot", so a slot-less page is never enumerated.
This section's own myext_reviewState example is exactly the case that would bite: an approval extension wanting the property on every content page finds it silently absent on every page without Subjects. The constraint is documented — at line 142, SkippedMissingSubjectSlot — but only for the manual rebuild() path, not here or in the Page Property Providers section itself.
The same qualifier applies to "Saving a page re-projects that page's nodes" on line 448.
| parameters: [ 'namespaceId' => NS_MAIN ], | ||
| limits: Neo4jQueryLimits::forUser( $this->getUser() ), | ||
| ) ); | ||
| } catch ( Exception ) { |
There was a problem hiding this comment.
catch ( Exception ) with no binding discards the exception, so an unconfigured backend and a typo in this file's own Cypher render the identical generic message with nothing logged.
On a wiki with no graph backend, newCypherQueryService() calls requireNeo4jPlugin(), which throws LogicException( 'A configured Neo4j backend is required here.' ). LogicException extends Exception, so it lands here and surfaces as "Could not count content namespace pages tracked in the graph." — no hint that Neo4j is simply unconfigured. A CypherSyntaxException from the query on line 30 gets the same treatment.
The broad catch itself looks right: the page is registered unconditionally in extension.json, so narrowing to QueryException would turn the unconfigured-wiki case into a fatal error page. The suggestion is just to bind $e and surface or log its message — which is what the repo's only other consumer of this service does in its broad fallback (ScribuntoLuaLibrary.php:133-134):
} catch ( Exception $e ) {
throw new LuaError( $e->getMessage() );
}optionally catching QueryException first for a typed message, as that same file does at line 130.
Worth slightly more than usual here because the docs added in this PR carefully distinguish the two failure types (line 208): "throws a QueryException subclass on failure; newCypherQueryService() itself throws a LogicException on a wiki with no graph backend configured." The example swallows the distinction the docs just drew.
| $this->markPageTableAsUsed(); | ||
| } | ||
|
|
||
| public function testCountsOnlyContentNamespacePagesTrackedInTheGraph(): void { |
There was a problem hiding this comment.
The error branch at SpecialRedHerbContentPageCount.php:34-37, and the redherb-content-page-count-error message, are never exercised — this is the only test method. Deleting the try/catch or renaming the i18n key leaves the suite green.
The helper already exists: wrapping executeContentPageCount() in $this->runWithoutGraphBackend( ... ) (tests/phpunit/NeoWikiIntegrationTestCase.php:142) nulls both Neo4j URLs and resets the singleton, so newCypherQueryService() throws inside the try and drives the catch. Precedent for that helper on no-backend paths: tests/phpunit/NoGraphBackendTest.php.
Minor, since RedHerb is example code — but this PR's stated purpose is keeping the documented call shape working, and extending.md:208 asserts this exact no-backend contract.
Docs: name the keyword validator's CALL and SHOW rejection, state that the row cap truncates only after the query has run (bound work with LIMIT in the Cypher), scope the durable-write pointer to page-level key/value metadata and state plainly that arbitrary graph structure has no durable third-party write path, and qualify projection on save and rebuild with the Subject-slot requirement. Example: bind the caught exception and surface its message like the Scribunto library does, cover the no-backend error branch via runWithoutGraphBackend(), and rework editMainSubject to resolve the Main Subject through getSubjectRepository()->getPageSubjects() instead of scraping the .ext-neowiki-view placeholder the docs forbid; the docs now cite it as the worked example of that path. Also exclude tests/RedHerb/node_modules from phpcs: following the RedHerb README's npm install instructions previously made make phpcs scan thousands of dependency files and run out of memory. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Extensions have started building on two surfaces that are reachable but internal: the rendered
.ext-neowiki-viewplaceholders and the raw Neo4j client. NeoWiki is pre-1.0 and cannot yet declare astable interface, so the extending docs gain an "Internal surfaces" section: the rendered HTML is marked as
not an integration surface, while the internal client gets a comparison of what the documented query
interfaces provide over it (read-only enforcement, resource limits, result handling, churn exposure) rather
than a prohibition, plus a warning that direct graph writes do not survive re-projection or rebuilds.
Since the alternative for PHP-side querying was undocumented, add a "Running Cypher queries" subsection
covering
newCypherQueryService(), backed by a new RedHerb example (SpecialRedHerbContentPageCount) withan integration test against real Neo4j, so the documented call shape stays working like the page's other
examples.
Production notes
Docs authored by
Fable 5 (max); the RedHerb example and its integration test were implemented by anOpus 4.8subagent against the docs' specified call shape, with an assertion-level red (count without theWHERE filter) before the green. Iterative branch history squashed before review; post-squash amendments are
docs wording only.