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 '') || + ' } }'"/> 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