From 95b30b7a5128359586def2f1d2437455de036f8f Mon Sep 17 00:00:00 2001 From: Anton Wiklund Date: Sun, 8 Nov 2026 17:03:45 +0100 Subject: [PATCH] fix(solid): read a subject typed as deck and group as a deck only The catalog's decks and deck groups were read apart, so a subject that fits both shapes was read twice: once as a deck and once as a group of the same URL. The deck list showed it twice, and some moves past it or out of it failed with a raw TypeError instead of the app's own error, because the tree looked nodes up by URL alone. A deck in its own format that was also typed as a group made the whole arrangement read-only. A subject read as a deck is now a deck only; its group type and member links are ignored. The tree also looks a parent up as a group, so a deck of the same URL is never taken for it, and treeChanges refuses a tree that holds a node twice with the deck-tree-changed error. Refs: GHSA-jhx9-xcwf-9jmq --- docs/data-model.md | 9 ++-- packages/domain/src/deckTree.test.ts | 26 +++++++++++ packages/domain/src/deckTree.ts | 42 +++++++++++++----- .../solid/src/mappers/deckTreeMapper.test.ts | 44 +++++++++++++++++++ packages/solid/src/mappers/deckTreeMapper.ts | 15 ++++--- 5 files changed, 116 insertions(+), 20 deletions(-) diff --git a/docs/data-model.md b/docs/data-model.md index f5eeaef62..76cdf8f68 100644 --- a/docs/data-model.md +++ b/docs/data-model.md @@ -973,9 +973,12 @@ such). What isn't one valid tree is read as one, not refused. A link to nothing, from a group to itself, or of the wrong kind (a `dcat:dataset` -link to a group, a `dcat:catalog` link to a deck) is ignored. A node -several parents list goes to the group with the lowest URL, except a -group the catalog's `dcat:catalog` lists, which stays at the top level. +link to a group, a `dcat:catalog` link to a deck) is ignored. A +subject the app reads as a deck is a deck only, even when it's also +typed as a group: it's never a group too, and its own links to members +are ignored. A node several parents list goes to the group with the +lowest URL, except a group the catalog's `dcat:catalog` lists, which +stays at the top level. A cycle is broken by moving its lowest-URL group to the top level. A group nested deeper than 16 groups (`MAX_GROUP_DEPTH`) is read at the top level, with the groups it holds counting their depth from there. diff --git a/packages/domain/src/deckTree.test.ts b/packages/domain/src/deckTree.test.ts index 5ceab1c88..27130b117 100644 --- a/packages/domain/src/deckTree.test.ts +++ b/packages/domain/src/deckTree.test.ts @@ -249,6 +249,19 @@ describe("tree lookups", () => { expect(isDescendant(sample, "gone", "b")).toBe(false); }); + it("look a parent up as a group, past a deck of the same URL", () => { + // A tree should never hold a deck and a group of one URL; if one does, a parent is still the group. + const twins = tree(["h", "x", ["x", "a"]], ["g"]); + expect(isDescendant(twins, "x", "a")).toBe(true); + expect(shape(moveNode(twins, "a", { parent: null, after: null }))).toEqual([ + "a", + ["h", "x", ["x"]], + ["g"], + ]); + expect(shape(renameGroup(twins, "x", { en: "New" }))).toEqual(shape(twins)); + expect(shape(deleteGroup(twins, "x"))).toEqual([["h", "x", "a"], ["g"]]); + }); + it("list the decks under nodes, depth first", () => { expect(decksOf(sample.children).map((d) => d.url)).toEqual(["a", "b", "c", "d"]); expect(decksOf([sample.children[1]!]).map((d) => d.url)).toEqual(["b", "c"]); @@ -460,6 +473,14 @@ describe("graft", () => { ]); }); + it("takes the deck of a URL, past a group of the same URL", () => { + // A tree should never hold a deck and a group of one URL; if one does, a graft's deck is still the deck. + expect(shape(graft(tree(["x", "a"], "x"), [made("n", deckNode("x"))]))).toEqual([ + ["x", "a"], + ["n", "x"], + ]); + }); + it("changes nothing when done again", () => { const nodes = [made("n", deckNode("a"))]; const once = graft(tree("a", "b"), nodes); @@ -516,6 +537,11 @@ describe("applyDeckTreeEdit", () => { }); describe("treeChanges", () => { + it("refuses a tree that holds a node twice, a deck and a group of one URL", () => { + const stored = layout(["a", "x"], { groups: [group("x", { decks: ["a"] })] }); + changedError(() => treeChanges(stored, tree("a", "x", ["x"]))); + }); + it("is empty for the tree the layout states", () => { const stored = layout(["a", "b"], { groups: [group("g1", { decks: ["b"], position: 1 })], diff --git a/packages/domain/src/deckTree.ts b/packages/domain/src/deckTree.ts index e04ada860..abb64f518 100644 --- a/packages/domain/src/deckTree.ts +++ b/packages/domain/src/deckTree.ts @@ -134,11 +134,13 @@ function locateIn( nodes: readonly TreeNode[], parent: ParentId, id: string, + kind?: TreeNode["kind"], ): TreeLocation | undefined { for (const [index, node] of nodes.entries()) { - if (nodeId(node) === id) return { node, parent, index }; + if (nodeId(node) === id && (kind === undefined || node.kind === kind)) + return { node, parent, index }; if (node.kind === "group") { - const found = locateIn(node.children, node.group.url, id); + const found = locateIn(node.children, node.group.url, id, kind); if (found !== undefined) return found; } } @@ -150,17 +152,30 @@ export function locate(tree: DeckTree, id: string): TreeLocation | undefined { return locateIn(tree.children, null, id); } +/** + * Where a group is in the tree; undefined when it is not. A parent is + * always a group, so it is looked up as one: a deck of the same URL is + * not it. + */ +function locateGroup( + tree: DeckTree, + id: string, +): (TreeLocation & { node: Extract }) | undefined { + return locateIn(tree.children, null, id, "group") as + | (TreeLocation & { node: Extract }) + | undefined; +} + /** Whether `id` is inside the group `ancestor`, at any depth (not the group itself). */ export function isDescendant(tree: DeckTree, ancestor: string, id: string): boolean { - const found = locate(tree, ancestor); - return found?.node.kind === "group" && locateIn(found.node.children, ancestor, id) !== undefined; + const found = locateGroup(tree, ancestor); + return found !== undefined && locateIn(found.node.children, ancestor, id) !== undefined; } /** A parent's children; undefined when there is no such group. */ function childrenOf(tree: DeckTree, parent: ParentId): readonly TreeNode[] | undefined { if (parent === null) return tree.children; - const found = locate(tree, parent); - return found?.node.kind === "group" ? found.node.children : undefined; + return locateGroup(tree, parent)?.node.children; } /** The tree with a parent's children replaced (the parent must exist). */ @@ -243,8 +258,8 @@ export function combine( * level, no further. A group that is already gone changes nothing. */ export function deleteGroup(tree: DeckTree, url: string): DeckTree { - const at = locate(tree, url); - if (at?.node.kind !== "group") return tree; + const at = locateGroup(tree, url); + if (at === undefined) return tree; return withChildren( tree, at.parent, @@ -274,8 +289,8 @@ export function gather(tree: DeckTree, ids: readonly string[], parent: ParentId) /** The tree with a group renamed; a group that is gone means the tree changed under the edit. */ export function renameGroup(tree: DeckTree, url: string, title: LangText): DeckTree { - const at = locate(tree, url); - if (at?.node.kind !== "group") throw changed(); + const at = locateGroup(tree, url); + if (at === undefined) throw changed(); if (sameText(at.node.group.title, title)) return tree; const node: TreeNode = { ...at.node, group: { ...at.node.group, title } }; return withChildren(tree, at.parent, spliced(childrenOf(tree, at.parent)!, at.index, 1, node)); @@ -299,8 +314,8 @@ export function graft(tree: DeckTree, nodes: readonly GraftNode[]): DeckTree { list.flatMap((node): TreeNode[] => { if (node.kind === "group") return [{ kind: "group", group: node.group, children: placed(node.children) }]; - const at = locate(rest, node.url); - if (at?.node.kind !== "deck") return []; + const at = locateIn(rest.children, null, node.url, "deck"); + if (at === undefined) return []; rest = without(rest, at); return [at.node]; }); @@ -488,6 +503,8 @@ function sameMembers( * the tree it reads gives changes. An edit that changes nothing (the * same tree back from `applyDeckTreeEdit`) must therefore write nothing * without asking: the pod is set straight only alongside a real edit. + * A tree that holds a node twice (a deck and a group of one URL) states + * no one arrangement to write: the tree changed under the edit. */ export function treeChanges(stored: StoredLayout, after: DeckTree): TreeChanges { const before = levels(buildTree(stored)); @@ -498,6 +515,7 @@ export function treeChanges(stored: StoredLayout, after: DeckTree): TreeChanges const kinds = new Map(); const collect = (nodes: readonly TreeNode[]) => { for (const node of nodes) { + if (kinds.has(nodeId(node))) throw changed(); kinds.set(nodeId(node), node); if (node.kind === "group") collect(node.children); } diff --git a/packages/solid/src/mappers/deckTreeMapper.test.ts b/packages/solid/src/mappers/deckTreeMapper.test.ts index b94b61741..cd72cd9ed 100644 --- a/packages/solid/src/mappers/deckTreeMapper.test.ts +++ b/packages/solid/src/mappers/deckTreeMapper.test.ts @@ -103,6 +103,50 @@ describe("toStoredLayout", () => { expect(toStoredLayout(unreadable, CATALOG)).toMatchObject({ groups: [], readOnly: true }); }); + it("reads a subject typed as both a deck and a group as a deck only, and its edits as a deck's", async () => { + // Another app's subject that fits both shapes: no sm:formatVersion (read as 1), titles with and without a language. + const dual = (extra = "") => + document( + rootTtl("; dcat:catalog <#x>") + + deckTtl("a", "; sm:position 0 ") + + groupTtl("g", "G", "", "; sm:position 3 ") + + `<#x> a sm:Deck, dcat:Dataset, sm:DeckGroup, dcat:Catalog ; + dcterms:title "X"@en, "X" ; dcterms:description "X."@en ; dcterms:publisher <${ME}> ; + sm:cardsDocument ; + sm:reviewsDocument ; sm:position 1 ${extra}. +` + + groupTtl("h", "H", "; dcat:dataset <#x>", "; sm:position 2 "), + ); + const dataset = await dual(); + const layout = toStoredLayout(dataset, CATALOG); + expect(layout.decks.map((deck) => deck.id)).toContain("x"); + expect(layout.groups.map((entry) => entry.group.url)).toEqual([at("g"), at("h")]); + expect(ids(dataset)).toEqual([ + { kind: "deck", deck: "a" }, + { kind: "group", group: "h", children: [{ kind: "deck", deck: "x" }] }, + { kind: "group", group: "g", children: [] }, + ]); + const into = edited(dataset, { + kind: "move", + node: at("x"), + to: { parent: at("g"), after: null }, + }); + expect(getUrlAll(getThing(into.dataset, at("g"))!, DCAT.dataset)).toEqual([at("x")]); + const top = edited(dataset, { + kind: "move", + node: at("x"), + to: { parent: null, after: at("g") }, + }); + expect(buildTree(toStoredLayout(top.dataset, CATALOG))).toEqual(top.after); + // Its dcat:dataset links are no group's members: the deck it names stays where it is. + const withA = await dual("; dcat:dataset <#a> "); + const out = edited(withA, { kind: "move", node: at("a"), to: { parent: null, after: null } }); + expect(buildTree(toStoredLayout(out.dataset, CATALOG))).toEqual(out.after); + // A deck's format is not a group's: one also typed as a group does not make the layout read-only. + const typed = await document(rootTtl() + deckTtl("b", "; a sm:DeckGroup, dcat:Catalog ")); + expect(toStoredLayout(typed, CATALOG)).toMatchObject({ groups: [], readOnly: false }); + }); + it("lists no top-level groups without a catalog", async () => { expect(toStoredLayout(await document(deckTtl("a")), CATALOG)).toMatchObject({ rootGroups: [], diff --git a/packages/solid/src/mappers/deckTreeMapper.ts b/packages/solid/src/mappers/deckTreeMapper.ts index 07b93418f..788822e61 100644 --- a/packages/solid/src/mappers/deckTreeMapper.ts +++ b/packages/solid/src/mappers/deckTreeMapper.ts @@ -42,16 +42,22 @@ import { toDecks } from "./deckMapper"; * A group the app cannot read (one that does not fit its shape) is not * part of the layout and is never written: its members are at the top * level, and the catalog's `dcat:catalog` keeps listing it. A group in - * a newer format than this app's makes the whole layout read-only. + * a newer format than this app's makes the whole layout read-only. A + * subject read as a deck is a deck, never also a group, whatever else + * it is typed as: the tree holds each node once. */ -function groupThings(dataset: Dataset) { - return getThingAll(dataset).filter((thing) => getUrlAll(thing, RDF.type).includes(SM.DeckGroup)); +/** The subjects typed as groups, but for those in `decks`. */ +function groupThings(dataset: Dataset, decks: ReadonlySet) { + return getThingAll(dataset).filter( + (thing) => getUrlAll(thing, RDF.type).includes(SM.DeckGroup) && !decks.has(asUrl(thing)), + ); } /** The arrangement a catalog document (at `catalogUrl`) states. */ export function toStoredLayout(dataset: Dataset, catalogUrl: string): StoredLayout { - const things = groupThings(dataset); + const decks = toDecks(dataset, catalogUrl); + const things = groupThings(dataset, new Set(decks.map((deck) => deck.url))); const readOnly = things.some((thing) => storedVersionOf(thing) > LATEST_VERSION.deckGroup); const groups: StoredGroup[] = []; for (const thing of things) { @@ -60,7 +66,6 @@ export function toStoredLayout(dataset: Dataset, catalogUrl: string): StoredLayo const url = asUrl(thing); groups.push(deckGroupFromRecord(url, migrate("deckGroup", read.record, { subject: url }))); } - const decks = toDecks(dataset, catalogUrl); const deckPositions = new Map(); for (const deck of decks) { const position = getInteger(getThing(dataset, deck.url)!, SM.position);