From 29beb7501de9166aba990c61aef140a85fa5f44e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martynas=20Jusevi=C4=8Dius?= Date: Fri, 18 Sep 2026 13:24:58 +0200 Subject: [PATCH 1/2] Bound the tree children query and CONSTRUCT only what a node renders ldh:tree-children-query emitted an unbounded DESCRIBE ?child, so opening the drawer tree fetched the full description of every child of a container. It fires on every page render, not on a click: ldh:NavigationUpdate walks down to the current document and fetches each level on the way. Measured against the Rebrickable dataspace (~158k documents, one entity per named graph): container children DESCRIBE CONSTRUCT taxonomies/colors/ 275 202 KB - sets/ 28,356 20.5 MB 6.6 MB parts/ 64,647 502 18.1 MB Both large containers exceeded the platform response limit, so the tree answered 502 on every page beneath them. A node renders four things - @rdf:about, a label, rdf:type for the icon, and whether the tree relation is present for the disclosure - so a CONSTRUCT of just those is both correct and several times smaller. Size independent of container size needs the bound as well: ldh:tree-page-size (default 1000) limits the child page, and an app stylesheet can raise it. A bound alone would break the descent. ldh:doctree-descend picks the next step out of the rendered children and falls through silently when it is absent, which with a LIMIT it usually would be. The query now takes the document being opened to and includes the child leading to it whatever else the page holds, so the tree still opens to and highlights the current document. Verified on that dataspace: sets/ 343 KB (was 20.5 MB), parts/ and inventories/ 200, the tree opens to parts/3001/, and the SKOS trees from the editor/taxonomy package inherit the same bound unchanged - they call this function, and their top concepts and narrower concepts both fetch through the new CONSTRUCT. Co-Authored-By: Claude Opus 5 (1M context) --- .../linkeddatahub/xsl/client/navigation.xsl | 13 ++++- .../linkeddatahub/xsl/client/tree.xsl | 54 +++++++++++++++---- 2 files changed, 54 insertions(+), 13 deletions(-) diff --git a/src/main/webapp/static/com/atomgraph/linkeddatahub/xsl/client/navigation.xsl b/src/main/webapp/static/com/atomgraph/linkeddatahub/xsl/client/navigation.xsl index a2e4712a8..a27ee38dd 100644 --- a/src/main/webapp/static/com/atomgraph/linkeddatahub/xsl/client/navigation.xsl +++ b/src/main/webapp/static/com/atomgraph/linkeddatahub/xsl/client/navigation.xsl @@ -269,7 +269,16 @@ ORDER BY DESC(?created) - + + + + + + + + + + + + + + + + + + + + + + @@ -57,25 +75,39 @@ exclude-result-prefixes="#all" ldh:tree-children-query requires at least one parent or child property + + + + - + parent side. Nothing needed the parse - this query is generated, not authored or edited. --> + + 'CONSTRUCT { ?child a ?Type . ?child ?labelProp ?label . ?child ?treeProp ?treeValue } ' || + 'WHERE { { SELECT DISTINCT ?child WHERE { GRAPH ?linkGraph { ' || $links || ' } } ORDER BY ?child LIMIT ' || $ldh:tree-page-size || ' }' || + (if (exists($path-uri)) then ' UNION { GRAPH ?pathGraph { ' || $links || ' } FILTER (strstarts("' || $path-uri || '", str(?child))) }' else '') || + ' GRAPH ?childGraph { ?child a ?Type' || + ' OPTIONAL { ?child ?labelProp ?label FILTER (?labelProp IN (' || string-join(for $property in $ldh:label-properties return '<' || $property || '>', ', ') || ')) }' || + (if (exists($tree-properties)) then ' OPTIONAL { ?child ?treeProp ?treeValue FILTER (?treeProp IN (' || string-join(for $property in $tree-properties return '<' || $property || '>', ', ') || ')) }' else '') || + ' } }'"/> From 5ecd54d5dece6899c98996f3d141007ba78c5a33 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martynas=20Jusevi=C4=8Dius?= Date: Fri, 18 Sep 2026 13:55:35 +0200 Subject: [PATCH 2/2] Track the children query's new shape in the UI suite, and cover the bound Three places matched the children query by its verb, and bounding it broke all of them - two silently, which is the reason this is its own commit rather than a line in the last one. tree-children-failure CHILDREN_QUERY routes the injected 403. A route pattern that matches nothing fails open: the refusal never happens and every assertion passes against a tree that was never refused anything. concept-tree counts.down stopped counting, so "the DOM is the cache" compared 0 against 0 and passed asserting nothing. lib/console the allowance for the 502 the unlimited DESCRIBE produced. That 502 is gone, so the entry became a blind spot over exactly the request the fix was about, and is removed rather than updated. Both patterns now match the projection rather than the verb: CONSTRUCT alone is not distinctive, where `?child a ?Type` is only ever this query. Two specs cover what the fix introduced: asks for a bounded page of children, and only what a node renders - a LIMIT is present and a DESCRIBE is not. Both halves matter: a CONSTRUCT that lost its bound would still grow without limit, and a bounded DESCRIBE would still carry every triple of every child it returned. opens the path even when it falls outside the page of children - rewrites the LIMIT to 0 on the wire, so a small fixture container behaves like one holding more children than a fetch returns. The bound applies to the inner subselect alone, so at zero the page contributes nothing and every row still rendered got there through the path branch. Verified against a 64k-child container, including the negative control that makes the second spec worth having: baseline container expanded, target active, 1001 children LIMIT 0 container expanded, target active, 1 child LIMIT 0, path branch removed no container row, target absent, not active So the spec passes for the reason it claims to, and fails when the branch it is written about is taken away. The suite itself was not run end to end here: it wants an instance where nothing is readable by default, and the stack available to me grants anonymous read across the dataspace, which its preflight rejects by design. Co-Authored-By: Claude Opus 5 (1M context) --- ui-tests/lib/console.mjs | 10 ++-- ui-tests/specs/concept-tree.spec.mjs | 10 +++- ui-tests/specs/document-tree.spec.mjs | 53 +++++++++++++++++++ ui-tests/specs/tree-children-failure.spec.mjs | 7 ++- 4 files changed, 72 insertions(+), 8 deletions(-) diff --git a/ui-tests/lib/console.mjs b/ui-tests/lib/console.mjs index d181c0a21..80e00cb26 100644 --- a/ui-tests/lib/console.mjs +++ b/ui-tests/lib/console.mjs @@ -20,11 +20,11 @@ const seen = [endUserBase, adminBase].map(base => ({ // Noise that is not this build's fault. Excluded by pattern and listed here with its // reason, rather than by loosening the assertion for everything. -const preexisting = [ - // The unlimited DESCRIBE over a large container 502s on the dev stack, and reproduces - // on builds predating this suite. Tracked separately; see commit 3305741ac. - { pattern: /sparql\?query=DESCRIBE[\s\S]*has_parent/i, reason: 'pre-existing 502 on unlimited DESCRIBE' }, -]; +// Empty, and worth keeping so: it held one entry, for the 502 the unlimited DESCRIBE of a +// large container produced. That query is now a bounded CONSTRUCT, so the 502 is gone and the +// suppression with it - an allowance that outlives its defect stops being an allowance and +// becomes a blind spot over exactly the request the fix was about. +const preexisting = []; const allowed = (text, declared) => [...preexisting, ...declared].some(({ pattern }) => pattern.test(text)); diff --git a/ui-tests/specs/concept-tree.spec.mjs b/ui-tests/specs/concept-tree.spec.mjs index c5ebe4d2a..e7b18e59c 100644 --- a/ui-tests/specs/concept-tree.spec.mjs +++ b/ui-tests/specs/concept-tree.spec.mjs @@ -26,14 +26,20 @@ const rowsFor = (page, name) => const disclosureOf = (page, name) => rowsFor(page, name).locator('> div.tree-row > button'); // Every concept-tree fetch, counted: the hops up are a SELECT for ?parent, the levels down -// are the shared DESCRIBE of a node's children. +// are the shared CONSTRUCT of a node's children. +// +// The down pattern matches the projection, not the verb: the verb stopped being distinctive +// when the children query became a CONSTRUCT, and CONSTRUCT alone would also count whatever +// else the page constructs. Matching what only this query projects is also what keeps the +// counter honest - while it matched nothing, `the DOM is the cache` compared 0 against 0 and +// passed without asserting anything. function countQueries(page) { const counts = { up: 0, down: 0 }; page.on('response', response => { const url = decodeURIComponent(response.url()); if (!url.includes('/sparql?')) return; if (/SELECT DISTINCT \?parent WHERE/.test(url)) counts.up++; - else if (/DESCRIBE \?child/.test(url)) counts.down++; + else if (/CONSTRUCT \{ \?child a \?Type/.test(url)) counts.down++; }); return counts; } diff --git a/ui-tests/specs/document-tree.spec.mjs b/ui-tests/specs/document-tree.spec.mjs index 0247471ba..56171046f 100644 --- a/ui-tests/specs/document-tree.spec.mjs +++ b/ui-tests/specs/document-tree.spec.mjs @@ -46,6 +46,20 @@ async function slowChildren(page, ms = 1500) { }); } +// Empties the children page on the wire, so a fixture container behaves like one holding more +// children than a single fetch returns - without needing thousands of documents to build one. +// +// LIMIT 0 rather than a small number because the bound applies to the inner subselect alone: +// with it at zero the page contributes nothing and every row that still appears got there +// through the branch that pins the document being opened to. That makes the spec below a +// statement about that branch rather than about the page size, and it fails the way the +// missing branch would - a tree that stops before it reaches you. +async function emptyChildrenPage(page) { + await page.route( + url => url.href.includes('/sparql?') && decodeURIComponent(url.href).includes('has_parent'), + route => route.continue({ url: route.request().url().replace(/LIMIT%20\d+/i, 'LIMIT%200') })); +} + test.describe('document tree', () => { test('opens the path down to the document being read and marks it', async ({ page }) => { await goto(page, itemUri(1)); @@ -57,6 +71,45 @@ test.describe('document tree', () => { .toHaveAttribute('aria-current', 'page'); }); + test('asks for a bounded page of children, and only what a node renders', async ({ page }) => { + const queries = []; + page.on('request', request => { + const url = decodeURIComponent(request.url()); + if (url.includes('/sparql?') && url.includes('has_parent')) queries.push(url); + }); + + await goto(page, itemUri(1)); + await openDrawer(page); + await expect(rowsFor(page, itemUri(1))).toHaveClass(/is-active/); + + // The query this replaced was an unbounded DESCRIBE, which returns whole child + // documents: 20.5 MB over a container of 28k, and past the response limit over one of + // 65k, so the tree answered 502 on every page beneath it. Both halves matter - a + // CONSTRUCT that lost its bound would still grow without limit, and a bounded DESCRIBE + // would still carry every triple of every child it returned. + expect(queries.length, 'the descent should have fetched children').toBeGreaterThan(0); + for (const query of queries) { + expect(query, 'the children query must stay bounded').toMatch(/LIMIT \d+/); + expect(query, 'a DESCRIBE returns whole child documents').not.toMatch(/DESCRIBE/); + } + }); + + test('opens the path even when it falls outside the page of children', async ({ page }) => { + await emptyChildrenPage(page); + + await goto(page, itemUri(1)); + await openDrawer(page); + + // ldh:doctree-descend picks its next step out of the rendered children and falls + // through silently when it is absent - so bounding the page without pinning the path + // would leave the reader at the top of a hierarchy with no sign of where they are, + // and nothing in the console to say so. + await expect(disclosureOf(page, fixtures.container)).toHaveAttribute('aria-expanded', 'true'); + await expect(rowsFor(page, itemUri(1))).toHaveClass(/is-active/); + await expect(rowsFor(page, itemUri(1)).locator('> div.tree-row > a')) + .toHaveAttribute('aria-current', 'page'); + }); + test('indents each level it opens', async ({ page }) => { await goto(page, itemUri(1)); await openDrawer(page); diff --git a/ui-tests/specs/tree-children-failure.spec.mjs b/ui-tests/specs/tree-children-failure.spec.mjs index 4f4e50a55..6b291139b 100644 --- a/ui-tests/specs/tree-children-failure.spec.mjs +++ b/ui-tests/specs/tree-children-failure.spec.mjs @@ -32,7 +32,12 @@ import { goto } from '../lib/settle.mjs'; import { fixtures, itemTitle, itemUri } from '../lib/fixtures.mjs'; // The query the tree asks for one node's children, as ldh:TreeChildrenFetch sends it. -const CHILDREN_QUERY = /\/sparql\?query=DESCRIBE[\s\S]*has_parent/i; +// +// It routes on the wire form, so it has to track the query's shape: this matched DESCRIBE +// until the children query became a bounded CONSTRUCT, and a route pattern that matches +// nothing fails open - the 403 below is never injected and every assertion here passes +// against a tree that was never refused anything. +const CHILDREN_QUERY = /\/sparql\?query=CONSTRUCT[\s\S]*has_parent/i; // What the server really answers when the endpoint is refused: an http:Response whose subject is a // blank node. Served verbatim so the client meets the body it would meet in production, not an