diff --git a/docs/handbuch/dynamic-groups.md b/docs/handbuch/dynamic-groups.md index c1b57d7..135834d 100644 --- a/docs/handbuch/dynamic-groups.md +++ b/docs/handbuch/dynamic-groups.md @@ -6,7 +6,7 @@ sources: - src/engine/dynamic.ts - src/engine/synthetic.ts - src/application/operations/adopt-group.ts -sources_hash: e38b8c0f6032d5cc +sources_hash: 3872161fc3777e2b reviewed: 2026-08-28 --- @@ -382,15 +382,16 @@ sibling of `query`, not an argument to `churchQuery(...)`. `q` (`src/config/query.ts`) emits a JSONLogic tree: -| helper | emits | -| -------------------------- | --------------------------------------- | -| `q.and(...nodes)` | `{ and: nodes }` | -| `q.or(...nodes)` | `{ or: nodes }` | -| `q.not(node)` | `{ "!": [node] }` | -| `q.var(name)` | `{ var: name }` | -| `q.eq(varName, value)` | `{ "==": [{ var: varName }, value] }` | -| `q.oneof(varName, values)` | `{ oneof: [{ var: varName }, values] }` | -| `q.isnull(varName)` | `{ isnull: [{ var: varName }] }` | +| helper | emits | +| -------------------------- | -------------------------------------------------------- | +| `q.and(...nodes)` | `{ and: nodes }` | +| `q.or(...nodes)` | `{ or: nodes }` | +| `q.not(node)` | `{ "!": [node] }` | +| `q.var(name)` | `{ var: name }` | +| `q.eq(varName, value)` | `{ "==": [{ var: varName }, value] }` | +| `q.oneof(varName, values)` | `{ oneof: [{ var: varName }, values] }` | +| `q.isnull(varName)` | `{ isnull: [{ var: varName }] }` | +| `q.memberOf(node)` | `{ dterm: [{ stereotype: ["groupmembership"] }, node] }` | `var` values may be **logical references** or **raw ids**. Prefer a reference so the ruleset is portable across hosts (#20): `q.eq("ctgroup.campusId", @@ -439,8 +440,51 @@ const ruleset = { ChurchTools' stored rulesets carry cosmetic noise that would otherwise show up as permanent phantom diffs on every `ct plan`: -- **`dterm: [label, expr]` wrappers** — a cosmetic UI label around a - subtree. Never evaluated by ChurchTools; stripped down to `expr`. +- **`dterm: [label, expr]` wrappers** — a UI label around a subtree. A string + or `{ title }` label is cosmetic and stripped down to `expr`. A label that + carries a **`stereotype`** (`{ stereotype: ["groupmembership"], title }`) is + **not** cosmetic and is kept: it makes ChurchTools evaluate the wrapped group + condition per person ("is / is not a member of group X") rather than per + membership row. Without it, a negated group condition such as + `!(member of "Unsubscribed")` still matches everyone who has any other + membership, so it excludes no one. When you author a group condition — + especially a negated one — wrap it the way the ChurchTools editor does. With + the typed builder that is `q.memberOf`: + + ```ts + q.not(q.memberOf(q.oneof("ctgroup.id", [ref.group("unsubscribed")]))); + ``` + + and in a JSON ruleset file: + + ```json + { + "!": [ + { + "dterm": [ + { "stereotype": ["groupmembership"], "title": "Mitgliedschaft in einer Gruppe" }, + { + "oneof": [{ "var": "ctgroup.id" }, [{ "__ctRef": true, "kind": "group", "key": "unsubscribed" }]] + } + ] + } + ] + } + ``` + + Up to v4.1.0 the normalizer stripped these wrappers too, and because `apply` + writes the normalized ruleset, applying a ruleset silently removed them. Two + consequences when you upgrade: + + - Rulesets captured by `ct adopt` before the fix lack the wrappers. `ct plan` + now warns (`applying would remove N "stereotype" dterm wrapper(s)`) for each + group whose live ruleset still has them. Re-adopt those rulesets before you + apply. + - A group that an earlier `apply` already stripped on the server has no + wrapper left to re-adopt, and `ct plan` shows nothing. Add the wrapper back + by hand (above), or re-save the condition in the ChurchTools editor and + re-adopt. + - **int/string inconsistency** — the same logical id shows up as both `1` and `"1"` across (and even within) a single ruleset. Numeric-looking strings inside the `query` subtree are coerced to numbers. diff --git a/docs/handbuch/group-member-fields.md b/docs/handbuch/group-member-fields.md index b59bf8a..0d19842 100644 --- a/docs/handbuch/group-member-fields.md +++ b/docs/handbuch/group-member-fields.md @@ -1,5 +1,5 @@ --- -sources_hash: 802017b4678885e0 +sources_hash: ab2983f780a57d6d title: Group member fields sources: - src/engine/member-fields.ts diff --git a/src/config/query.ts b/src/config/query.ts index e133065..b3930c2 100644 --- a/src/config/query.ts +++ b/src/config/query.ts @@ -28,6 +28,10 @@ export const q = { eq: (varName: string, value: unknown): QueryNode => ({ "==": [{ var: varName }, value] }), oneof: (varName: string, values: unknown[]): QueryNode => ({ oneof: [{ var: varName }, values] }), isnull: (varName: string): QueryNode => ({ isnull: [{ var: varName }] }), + /** Evaluate a group condition per person ("is a member of …"), not per membership row — wrap + * every `ctgroup.*` condition you negate: `q.not(q.oneof("ctgroup.id", …))` alone excludes no + * one who has any other membership. Same `groupmembership` stereotype the CT editor writes. */ + memberOf: (n: QueryNode): QueryNode => ({ dterm: [{ stereotype: ["groupmembership"] }, n] }), }; /** Wrap a JSONLogic filter in the ChurchQuery envelope the ruleset `query` field expects. diff --git a/src/engine/dynamic.ts b/src/engine/dynamic.ts index 0753f90..d21b74b 100644 --- a/src/engine/dynamic.ts +++ b/src/engine/dynamic.ts @@ -13,13 +13,40 @@ import type { DynamicStatus } from "./types.js"; const READ_ONLY_KEYS = new Set(["dynamicGroupUpdateStarted", "dynamicGroupUpdateFinished"]); -/** Recursively unwrap `dterm: [label, expr]` cosmetic wrappers to their `expr`. */ +/** + * Whether a `dterm` label is pure display text: a string, or an object carrying nothing but a + * `title`. Everything else is kept. In particular a `stereotype` label (in practice + * `["groupmembership"]`) is NOT cosmetic: it tells ChurchTools to evaluate the wrapped group + * condition per person ("is/is not a member of group X") instead of per membership row. Without + * it, `!(member of X)` is true for every person who also has any other membership row, so a + * negated group condition excludes no one. Measured read-only against 72 live rulesets on CT 3.137 + * (`POST /churchquery/debug/export`): keeping these labels reproduces every live result exactly; + * dropping them changes 18 of them. An allowlist, so a label key ChurchTools adds later is kept + * rather than silently stripped. + */ +export function isCosmeticLabel(label: unknown): boolean { + if (label === null || label === undefined || typeof label === "string") return true; + if (typeof label !== "object" || Array.isArray(label)) return false; + return Object.keys(label).every((k) => k === "title"); +} + +function isLabelWrapper(obj: Record): obj is { dterm: [unknown, unknown] } { + return Array.isArray(obj.dterm) && obj.dterm.length === 2 && Object.keys(obj).length === 1; +} + +/** + * Recursively unwrap cosmetic `dterm: [label, expr]` wrappers to their `expr`. A wrapper whose + * label is not cosmetic (see `isCosmeticLabel`) changes what the query selects and is kept, its + * label copied verbatim and its `expr` normalized in place. + */ export function stripCosmeticLabels(node: unknown): unknown { if (Array.isArray(node)) return node.map(stripCosmeticLabels); if (node && typeof node === "object") { const obj = node as Record; - if (Array.isArray(obj.dterm) && obj.dterm.length === 2 && Object.keys(obj).length === 1) { - return stripCosmeticLabels(obj.dterm[1]); // keep the expression, drop the label + if (isLabelWrapper(obj)) { + const [label, expr] = obj.dterm; + if (!isCosmeticLabel(label)) return { dterm: [structuredClone(label), stripCosmeticLabels(expr)] }; + return stripCosmeticLabels(expr); // keep the expression, drop the label } const out: Record = {}; for (const [k, v] of Object.entries(obj)) out[k] = stripCosmeticLabels(v); @@ -28,6 +55,17 @@ export function stripCosmeticLabels(node: unknown): unknown { return node; } +/** The non-cosmetic `dterm` labels in a (normalized) query, in document order. */ +export function semanticLabels(node: unknown): unknown[] { + if (Array.isArray(node)) return node.flatMap(semanticLabels); + if (node && typeof node === "object") { + const obj = node as Record; + const own = isLabelWrapper(obj) && !isCosmeticLabel(obj.dterm[0]) ? [obj.dterm[0]] : []; + return [...own, ...Object.values(obj).flatMap(semanticLabels)]; + } + return []; +} + /** * Coerce numeric-string leaves to numbers (CT is int/string-inconsistent for `var` values). * @@ -41,8 +79,12 @@ export function stripCosmeticLabels(node: unknown): unknown { export function coerceScalars(node: unknown): unknown { if (Array.isArray(node)) return node.map(coerceScalars); if (node && typeof node === "object") { + const obj = node as Record; + // A kept `dterm` label is free text written back to ChurchTools verbatim: a title "2024" must + // stay a string. Only the wrapped expression carries the int/string-inconsistent ids. + if (isLabelWrapper(obj)) return { dterm: [structuredClone(obj.dterm[0]), coerceScalars(obj.dterm[1])] }; const out: Record = {}; - for (const [k, v] of Object.entries(node as Record)) out[k] = coerceScalars(v); + for (const [k, v] of Object.entries(obj)) out[k] = coerceScalars(v); return out; } if (typeof node === "string" && /^(-?[1-9]\d*|0)$/.test(node)) { diff --git a/src/engine/synthetic.ts b/src/engine/synthetic.ts index 9051201..a5572a8 100644 --- a/src/engine/synthetic.ts +++ b/src/engine/synthetic.ts @@ -15,7 +15,14 @@ import { deepEqual } from "./plan.js"; import { mapConcurrent } from "../util/concurrency.js"; import { info, warn, formatError } from "../ui.js"; import { slug } from "../resources/registry.js"; -import { normalizeDynamic, normalizeRuleset, putRulesetBody, resolveRulesetRef } from "./dynamic.js"; +import { + normalizeDynamic, + normalizeRuleset, + putRulesetBody, + resolveRulesetRef, + semanticLabels, + type NormalizedDynamic, +} from "./dynamic.js"; import { formatPortablizeWarnings, scanUnportablized } from "../config/query-refs.js"; import { actualMemberFieldProps, @@ -638,6 +645,19 @@ const dynamicField: SyntheticField = { for (const line of details) info(` ${line}`); } const dynamic = normalizeDynamic({ status: d.dynamic.status, ruleset: resolvedRuleset }); + // A ruleset adopted while the normalizer still stripped `stereotype` labels lacks them, and + // applying it would PUT the stripped query back — which turns a negated group condition into + // one that excludes no one. Say so before apply does it; re-adopting restores the wrappers. + const live = (actual.get(d.key)?.dynamic as NormalizedDynamic | undefined)?.ruleset; + const dropped = semanticLabels(live?.query).length - semanticLabels(dynamic.ruleset.query).length; + if (dropped > 0) { + const headline = + `dynamic group "${d.key}": applying would remove ${dropped} "stereotype" dterm wrapper(s) ` + + `the live ruleset has — a negated group condition without one excludes no one. ` + + `Re-adopt the ruleset (or add the wrapper) before applying.`; + warnings.push(headline); + warn(headline); + } return { ...d, fields: { ...d.fields, dynamic } }; }); return { desired: augmented, errors, unreadable, ...(warnings.length > 0 ? { warnings } : {}) }; diff --git a/tests/dynamic.test.ts b/tests/dynamic.test.ts index 514ebe0..e007668 100644 --- a/tests/dynamic.test.ts +++ b/tests/dynamic.test.ts @@ -6,6 +6,7 @@ import { coerceScalars, normalizeDynamic, putRulesetBody, + semanticLabels, } from "../src/engine/dynamic.js"; describe("stripCosmeticLabels", () => { @@ -13,18 +14,43 @@ describe("stripCosmeticLabels", () => { const input = { and: [{ dterm: ["Campus", { "==": [{ var: "ctgroup.campusId" }, 1] }] }] }; expect(stripCosmeticLabels(input)).toEqual({ and: [{ "==": [{ var: "ctgroup.campusId" }, 1] }] }); }); - it("unwraps a dterm with an object label (title/stereotype/i18n key)", () => { + it("unwraps a dterm with a title-only object label (i18n key)", () => { const input = { - dterm: [ - { title: "group.x.title", stereotype: ["groupmembership"] }, - { isnull: [{ var: "person.dateOfDeath" }] }, - ], + dterm: [{ title: "group.x.title" }, { isnull: [{ var: "person.dateOfDeath" }] }], }; expect(stripCosmeticLabels(input)).toEqual({ isnull: [{ var: "person.dateOfDeath" }] }); }); + it("keeps a groupmembership stereotype wrapper: it changes what a negated group condition selects", () => { + // `!(member of X)` excludes no one once apply has written it without this wrapper — the label + // is part of the query, not decoration. + const label = { stereotype: ["groupmembership"], title: "Mitgliedschaft in einer Gruppe" }; + const input = { + "!": [ + { dterm: [label, { and: [{ dterm: ["Merkmal", { oneof: [{ var: "ctgroup.id" }, ["42"]] }] }] }] }, + ], + }; + const out = stripCosmeticLabels(input); + expect(out).toEqual({ + "!": [{ dterm: [label, { and: [{ oneof: [{ var: "ctgroup.id" }, ["42"]] }] }] }], + }); + expect(stripCosmeticLabels(out)).toEqual(out); // idempotent + expect((out as { "!": [{ dterm: [unknown] }] })["!"][0].dterm[0]).not.toBe(label); // copied + }); + it("keeps a label with any key besides title: unknown label keys are not assumed cosmetic", () => { + const input = { dterm: [{ title: "t", hint: "perperson" }, { isnull: [{ var: "person.dateOfDeath" }] }] }; + expect(stripCosmeticLabels(input)).toEqual(input); + }); }); describe("coerceScalars", () => { + it("leaves a kept dterm label verbatim: a numeric-looking title is free text, not an id", () => { + const input = { + dterm: [{ stereotype: ["groupmembership"], title: "2024" }, { oneof: [{ var: "ctgroup.id" }, ["5"]] }], + }; + expect(coerceScalars(input)).toEqual({ + dterm: [{ stereotype: ["groupmembership"], title: "2024" }, { oneof: [{ var: "ctgroup.id" }, [5]] }], + }); + }); it("coerces numeric strings to numbers so int/string drift is not spurious", () => { expect(coerceScalars({ "==": [{ var: "ctgroup.campusId" }, "1"] })).toEqual({ "==": [{ var: "ctgroup.campusId" }, 1], @@ -95,12 +121,17 @@ describe("normalizeRuleset", () => { expect(JSON.stringify(normalizeRuleset(once))).toBe(JSON.stringify(once)); // byte-identical + idempotent }); - it("read-then-normalize of every live fixture is stable and label-free", () => { + it("read-then-normalize of every live fixture is stable and keeps only stereotype labels", () => { for (const name of ["ruleset-683", "ruleset-2022", "ruleset-1092"]) { const raw = JSON.parse(readFileSync(`tests/fixtures/dynamic/${name}.get.json`, "utf8")); // array shape const once = normalizeRuleset(raw); expect(normalizeRuleset(once)).toEqual(once); // idempotent - expect(JSON.stringify(once)).not.toContain("dterm"); // cosmetic labels stripped + // cosmetic labels stripped, every stereotype wrapper the fixture carries survives + const wrappers = (JSON.stringify(once).match(/"dterm"/g) ?? []).length; + const stereotypes = (JSON.stringify(raw).match(/"stereotype"/g) ?? []).length; + expect(stereotypes).toBeGreaterThan(0); + expect(semanticLabels(once.query)).toHaveLength(stereotypes); + expect(wrappers).toBe(stereotypes); } }); }); diff --git a/tests/query.test.ts b/tests/query.test.ts index 1f85b2b..998e1c8 100644 --- a/tests/query.test.ts +++ b/tests/query.test.ts @@ -1,6 +1,6 @@ import { describe, it, expect } from "vitest"; import { q, churchQuery } from "../src/config/query.js"; -import { normalizeRuleset } from "../src/engine/dynamic.js"; +import { normalizeRuleset, stripCosmeticLabels } from "../src/engine/dynamic.js"; describe("typed query builder", () => { it("builds a JSONLogic tree", () => { @@ -22,6 +22,14 @@ describe("typed query builder", () => { expect(q.not(operand)).toEqual({ "!": [{ isnull: [{ var: "person.dateOfDeath" }] }] }); }); + it("q.memberOf wraps in the groupmembership stereotype, which normalization keeps", () => { + const node = q.memberOf(q.oneof("ctgroup.id", [7])); + expect(node).toEqual({ + dterm: [{ stereotype: ["groupmembership"] }, { oneof: [{ var: "ctgroup.id" }, [7]] }], + }); + expect(stripCosmeticLabels(node)).toEqual(node); + }); + it("q.or", () => { const a = q.eq("ctgroup.campusId", 1); const b = q.eq("person.isArchived", false); diff --git a/tests/synthetic-dynamic.test.ts b/tests/synthetic-dynamic.test.ts index 267abdb..a355b6a 100644 --- a/tests/synthetic-dynamic.test.ts +++ b/tests/synthetic-dynamic.test.ts @@ -533,3 +533,61 @@ describe("dynamic synthetic field — un-portablized ruleset reporting (#101)", expect(errs.join("")).not.toContain("host-specific"); }); }); + +describe("dynamic synthetic field — dropped stereotype wrappers", () => { + /** + * A ruleset adopted while the normalizer still stripped `stereotype` labels lacks them. Applying + * it would PUT the stripped query over a live one that has them, so a negated group condition + * silently stops excluding anyone. Plan time is where the author can still re-adopt. + */ + const membership = (expr: unknown) => ({ dterm: [{ stereotype: ["groupmembership"], title: "t" }, expr] }); + const negated = { oneof: [{ var: "ctgroup.id" }, [7]] }; + const state: State = { + version: 1, + host: "h", + resources: { + g: { type: "group", id: 5, key: "g", fields: { name: "G" }, adoptedAt: "t", updatedAt: "t" }, + }, + }; + const fold = async (live: unknown, declared: unknown) => { + const actual = new Map>([["g", { name: "G" }]]); + const desired: DesiredResource[] = [ + { + type: "group", + key: "g", + fields: { name: "G" }, + dependsOn: [], + dynamic: { status: "active", ruleset: declared }, + }, + ]; + const client = { + get: vi.fn(async (p: string) => (p.endsWith("/ruleset") ? live : { dynamicGroupStatus: "active" })), + }; + const errs: string[] = []; + const spy = vi.spyOn(process.stderr, "write").mockImplementation((s) => { + errs.push(String(s)); + return true; + }); + try { + const result = await dynamicField().fold({ client: getClient(client), state, desired, actual }); + return { result, out: errs.join("") }; + } finally { + spy.mockRestore(); + } + }; + + it("warns when the declared ruleset lacks a wrapper the live one has", async () => { + const { result, out } = await fold([{ query: { "!": [membership(negated)] } }], { + query: { "!": [negated] }, + }); + expect(out).toContain('dynamic group "g": applying would remove 1 "stereotype" dterm wrapper(s)'); + expect(result?.warnings?.join("\n")).toContain("Re-adopt the ruleset"); + }); + + it("stays silent when both sides carry the wrapper", async () => { + const ruleset = { query: { "!": [membership(negated)] } }; + const { result, out } = await fold([ruleset], ruleset); + expect(out).not.toContain("stereotype"); + expect(result?.warnings?.join("\n") ?? "").not.toContain("stereotype"); + }); +});