fix(solid): read a subject typed as deck and group as a deck only

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.

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 })],

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<TreeNode, { kind: "group" }> }) | undefined {
+  return locateIn(tree.children, null, id, "group") as
+    | (TreeLocation & { node: Extract<TreeNode, { kind: "group" }> })
+    | 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<string, TreeNode>();
   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);
     }

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 <https://pod.example/solid-memo/a/decks/x.ttl> ;
+  sm:reviewsDocument <https://pod.example/solid-memo/a/reviews/x.ttl> ; 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: [],

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<string>) {
+  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<string, number>();
   for (const deck of decks) {
     const position = getInteger(getThing(dataset, deck.url)!, SM.position);