diff --git a/src/scribe/models/__init__.py b/src/scribe/models/__init__.py index f4528a8..0135af2 100644 --- a/src/scribe/models/__init__.py +++ b/src/scribe/models/__init__.py @@ -43,6 +43,4 @@ from scribe.models.rulebook import ( # noqa: E402, F401 ) from scribe.models.repo_binding import RepoBinding # noqa: E402, F401 from scribe.models.system import System, RecordSystem # noqa: E402, F401 -from scribe.models.design_system import ( # noqa: E402, F401 - BASE_MODE, DesignSystem, DesignToken, -) +from scribe.models.design_system import DesignSystem, DesignToken # noqa: E402, F401 diff --git a/src/scribe/models/design_system.py b/src/scribe/models/design_system.py index 7c91a02..6df3680 100644 --- a/src/scribe/models/design_system.py +++ b/src/scribe/models/design_system.py @@ -22,11 +22,6 @@ from sqlalchemy.orm import Mapped, mapped_column from scribe.models import Base from scribe.models.base import SoftDeleteMixin, TimestampMixin -# The mode key that applies when no more specific one does. Emitted CSS puts it -# on the base selector and every other key on a mode selector, mirroring how a -# stylesheet is actually written: light on `:root`, dark layered over it. -BASE_MODE = "base" - class DesignSystem(Base, TimestampMixin, SoftDeleteMixin): __tablename__ = "design_systems" diff --git a/src/scribe/services/design_cascade.py b/src/scribe/services/design_cascade.py index 8d7f187..fd016f0 100644 --- a/src/scribe/services/design_cascade.py +++ b/src/scribe/services/design_cascade.py @@ -9,7 +9,13 @@ Being pure is also what makes the cascade rule testable without a database: these take a plain `{id: parent_id}` map, so a test states the shape of a hierarchy in one literal instead of building one. """ -from collections.abc import Mapping +from collections.abc import Mapping, Sequence +from dataclasses import dataclass + +# The mode key that applies when no more specific one does. Emitted CSS puts it +# on the base selector and every other key on a mode selector, mirroring how a +# stylesheet is actually written: light on `:root`, dark layered over it. +BASE_MODE = "base" def ancestry(system_id: int, parents: Mapping[int, int | None]) -> list[int]: @@ -59,3 +65,146 @@ def would_cycle( if proposed_parent_id == system_id: return True return system_id in ancestry(proposed_parent_id, parents) + + +# --------------------------------------------------------------------------- +# Resolution — flattening a chain into an effective token set +# --------------------------------------------------------------------------- + +@dataclass(frozen=True) +class Contribution: + """One system's offer for one token in one mode.""" + system_id: int + value: str + + +@dataclass(frozen=True) +class ResolvedToken: + """A token after the cascade, carrying the whole argument rather than the verdict. + + `contributions` holds every system that supplied a value, per mode, DEEPEST + FIRST — so `[0]` is the winner and `[1:]` are what it shadowed. Storing the + contest rather than a winner plus a separate provenance field means there is + nothing to keep in sync: "which system supplied this?" and "what did it + override?" are both reads of the same list, and they cannot disagree. + + Provenance is per MODE, not per token, because overriding is. A system that + deepens one accent for light backgrounds while leaving dark alone owns the + base value and inherits the dark one, and a token-level "overridden here" + flag would have to lie about one of them. + """ + name: str + contributions: dict[str, tuple[Contribution, ...]] + group_name: str | None + purpose: str | None + order_index: int + + @property + def value_by_mode(self) -> dict[str, str]: + """The effective value for each mode — the winner of each contest.""" + return {mode: entries[0].value for mode, entries in self.contributions.items()} + + @property + def origin_by_mode(self) -> dict[str, int]: + """Which system supplied each mode's effective value.""" + return {mode: entries[0].system_id for mode, entries in self.contributions.items()} + + def value_for(self, mode: str) -> str | None: + """The value to render in `mode`, falling back to the base mode. + + This is the read rule the storage shape implies: a token that is not + mode-dependent carries only `base`, and asking it for "dark" must yield + the base value rather than nothing. + """ + entries = self.contributions.get(mode) or self.contributions.get(BASE_MODE) + return entries[0].value if entries else None + + def is_overridden_in(self, system_id: int) -> bool: + """Does `system_id` win any mode of this token AND shadow something? + + The distinction the UI needs: a token this system introduced is not an + override, and a token it merely inherits is not either. + """ + return any( + len(entries) > 1 and entries[0].system_id == system_id + for entries in self.contributions.values() + ) + + +def _sort_key(token: ResolvedToken) -> tuple: + # Ungrouped tokens sort last rather than first: a design system that has + # started grouping should read as its groups, with the not-yet-filed + # remainder at the end. + return (token.group_name is None, token.group_name or "", token.order_index, token.name) + + +def resolve_tokens( + system_id: int, + parents: Mapping[int, int | None], + tokens_by_system: Mapping[int, Sequence], +) -> list[ResolvedToken]: + """Flatten a system's inheritance chain into its effective token set. + + Walks from `system_id` up to the root and applies tokens by name, deepest + winning. That is the CSS cascade — precedence by name along a parent chain — + rather than an analogy to it, which is why the storage model and the + stylesheet model came out the same shape. + + `tokens_by_system` maps a system id to its own token rows. Any object with + `.name`, `.value_by_mode`, `.group_name`, `.purpose` and `.order_index` will + do, so a test can state a hierarchy in literals and the service can pass ORM + rows to the same function. + + The result includes tokens the system never mentions — inheriting one is + what puts it in the effective set. A system with no tokens of its own + resolves to its parent's set entire, which is the correct answer for an app + that has not departed from the family yet. + + Merging is per (name, MODE): a child that supplies only a dark value + overrides only dark and keeps inheriting base. Metadata (`group_name`, + `purpose`, `order_index`) cascades separately by the same deepest-wins rule, + since a child overriding a value routinely leaves the family's description + of what the token is FOR untouched — and inheriting it beats blanking it. + """ + chain = ancestry(system_id, parents) + + contributions: dict[str, dict[str, list[Contribution]]] = {} + metadata: dict[str, dict[str, object]] = {} + + # Deepest first, so the first contribution seen for a (name, mode) wins and + # every later one is a shadowed ancestor appended behind it. + for depth_system_id in chain: + for token in tokens_by_system.get(depth_system_id) or (): + per_mode = contributions.setdefault(token.name, {}) + for mode, value in (token.value_by_mode or {}).items(): + per_mode.setdefault(mode, []).append( + Contribution(system_id=depth_system_id, value=value) + ) + + meta = metadata.setdefault( + token.name, {"group_name": None, "purpose": None, "order_index": None} + ) + for field in ("group_name", "purpose"): + if meta[field] is None: + meta[field] = getattr(token, field, None) + # order_index alone treats 0 as UNSTATED rather than "first", + # because 0 is the column default. Reading it as a real value would + # let any child override drag its token to the top of the group and + # lose the family's ordering — a visible reshuffle in return for a + # change that only touched a colour. + if not meta["order_index"]: + meta["order_index"] = getattr(token, "order_index", 0) or None + + resolved = [ + ResolvedToken( + name=name, + contributions={ + mode: tuple(entries) for mode, entries in per_mode.items() + }, + group_name=metadata[name]["group_name"], + purpose=metadata[name]["purpose"], + order_index=metadata[name]["order_index"] or 0, + ) + for name, per_mode in contributions.items() + ] + return sorted(resolved, key=_sort_key) diff --git a/src/scribe/services/design_systems.py b/src/scribe/services/design_systems.py index 4588d5c..2aafbd3 100644 --- a/src/scribe/services/design_systems.py +++ b/src/scribe/services/design_systems.py @@ -21,7 +21,12 @@ from scribe.models import async_session from scribe.models.design_system import DesignSystem, DesignToken from scribe.models.project import Project from scribe.services import access -from scribe.services.design_cascade import would_cycle +from scribe.services.design_cascade import ( + ResolvedToken, + ancestry, + resolve_tokens, + would_cycle, +) logger = logging.getLogger(__name__) @@ -36,17 +41,23 @@ class DesignSystemCycle(ValueError): """ -async def _parent_map(session, user_id: int) -> dict[int, int | None]: - """`{id: parent_id}` for the caller's live systems — the hierarchy's shape. +async def _parent_map(session, owner_user_id: int) -> dict[int, int | None]: + """`{id: parent_id}` for one owner's live systems — the hierarchy's shape. - Owner-scoped, which is also the constraint on parenting: you can only build - a chain out of systems you own. A borrowed link would let someone else's - delete or re-parent silently restyle your app. + Scoped to the OWNER of the systems, not the caller, and the distinction is + load-bearing on the read path: a caller reading through a shared project + does not own any link in the chain, and a caller-scoped map would hand them + an empty forest and a cascade truncated to one system. They would get a page + that renders with plausible wrong values and no error anywhere. + + Owner-scoping is also the constraint on parenting: a chain may only be built + from systems its owner controls, or someone else's delete or re-parent would + silently restyle your app. """ rows = ( await session.execute( select(DesignSystem.id, DesignSystem.parent_id).where( - DesignSystem.owner_user_id == user_id, + DesignSystem.owner_user_id == owner_user_id, DesignSystem.deleted_at.is_(None), ) ) @@ -131,7 +142,9 @@ async def update_design_system( if not await access.can_write_design_system(user_id, parent_id): return None if would_cycle( - design_system_id, parent_id, await _parent_map(session, user_id) + design_system_id, + parent_id, + await _parent_map(session, system.owner_user_id), ): raise DesignSystemCycle( f"Design system {design_system_id} cannot inherit from " @@ -213,6 +226,47 @@ async def list_tokens(user_id: int, design_system_id: int) -> list[DesignToken]: return list(rows.scalars().all()) +async def resolve_design_system( + user_id: int, design_system_id: int +) -> list[ResolvedToken] | None: + """A system's EFFECTIVE token set — everything it inherits, with its own on top. + + None when the caller may not read the system; an empty list when the chain + genuinely holds no tokens, which is an ordinary state for a system that has + just been created. + + Two queries regardless of how deep the chain runs: one for the hierarchy's + shape, one for every token in it. The flattening itself is + `design_cascade.resolve_tokens` — pure, so the cascade rule is tested + without a database and this function is only the loading. + """ + if not await access.can_read_design_system(user_id, design_system_id): + return None + async with async_session() as session: + system = await session.get(DesignSystem, design_system_id) + if system is None or system.deleted_at is not None: + return None + + parents = await _parent_map(session, system.owner_user_id) + chain = ancestry(design_system_id, parents) + + rows = ( + await session.execute( + select(DesignToken) + .where( + DesignToken.design_system_id.in_(chain), + DesignToken.deleted_at.is_(None), + ) + .order_by(DesignToken.order_index.asc(), DesignToken.name.asc()) + ) + ).scalars().all() + + tokens_by_system: dict[int, list[DesignToken]] = {} + for token in rows: + tokens_by_system.setdefault(token.design_system_id, []).append(token) + return resolve_tokens(design_system_id, parents, tokens_by_system) + + async def update_token( user_id: int, token_id: int, **fields: object ) -> DesignToken | None: diff --git a/tests/test_design_cascade.py b/tests/test_design_cascade.py index b00936c..99e6169 100644 --- a/tests/test_design_cascade.py +++ b/tests/test_design_cascade.py @@ -4,7 +4,9 @@ Pure functions over a `{id: parent_id}` literal, so a whole hierarchy is one lin of setup and no database is involved. That is the reason the cascade lives in its own import-free module — see services/design_cascade.py. """ -from scribe.services.design_cascade import ancestry, would_cycle +from types import SimpleNamespace + +from scribe.services.design_cascade import ancestry, resolve_tokens, would_cycle # --- ancestry --------------------------------------------------------------- @@ -92,3 +94,219 @@ def test_the_guard_survives_a_hierarchy_that_is_already_corrupt(): parents = {1: 2, 2: 1, 3: None} assert would_cycle(3, 1, parents) is False assert would_cycle(1, 2, parents) is True + + +# --- resolution ------------------------------------------------------------- +# +# The cascade proper (milestone #254 step 2). Hierarchies are stated as literals +# because resolve_tokens is pure and duck-typed — which is the whole reason it +# lives here rather than inside the service. + +def _token(name, value_by_mode, group_name=None, purpose=None, order_index=0): + return SimpleNamespace( + name=name, value_by_mode=value_by_mode, group_name=group_name, + purpose=purpose, order_index=order_index, + ) + + +# A family (1) and an app inheriting from it (2) — the shape the model exists for. +FAMILY, APP = 1, 2 +PARENTS = {FAMILY: None, APP: FAMILY} + + +def _by_name(tokens): + return {t.name: t for t in tokens} + + +def test_a_system_with_no_tokens_of_its_own_inherits_the_whole_family_set(): + """The correct answer for an app that has not departed from the family yet — + and the state every app system starts in.""" + resolved = resolve_tokens( + APP, PARENTS, + {FAMILY: [_token("--fs-obsidian", {"base": "#14171a"})], APP: []}, + ) + assert [t.name for t in resolved] == ["--fs-obsidian"] + assert resolved[0].value_by_mode == {"base": "#14171a"} + assert resolved[0].origin_by_mode == {"base": FAMILY} + + +def test_the_deepest_system_wins_and_says_what_it_overrode(): + """Provenance is the point of the whole model: the effective set is just a + list without it, and "where does this app depart from the family?" becomes a + diff someone has to compute.""" + resolved = _by_name(resolve_tokens( + APP, PARENTS, + { + FAMILY: [_token("--fs-accent", {"base": "#6b2118"})], + APP: [_token("--fs-accent", {"base": "#5b4a8a"})], + }, + )) + accent = resolved["--fs-accent"] + assert accent.value_by_mode == {"base": "#5b4a8a"} + assert accent.origin_by_mode == {"base": APP} + # The shadowed ancestor is still there, behind the winner. + assert [c.system_id for c in accent.contributions["base"]] == [APP, FAMILY] + assert accent.contributions["base"][1].value == "#6b2118" + + +def test_overriding_one_mode_leaves_the_others_inherited(): + """LOAD-BEARING, and the argument that decided the storage shape. An accent + deepened for light backgrounds while dark is left alone must own `base` and + still inherit `dark` — which is why merging is per (name, MODE) and why the + value column is a map rather than a pair of columns.""" + resolved = _by_name(resolve_tokens( + APP, PARENTS, + { + FAMILY: [_token("--fs-accent", {"base": "#34a877", "dark": "#34a877"})], + APP: [_token("--fs-accent", {"base": "#15803d"})], + }, + )) + accent = resolved["--fs-accent"] + assert accent.value_by_mode == {"base": "#15803d", "dark": "#34a877"} + assert accent.origin_by_mode == {"base": APP, "dark": FAMILY} + + +def test_a_token_only_the_app_defines_is_not_an_override(): + """Introducing a token and overriding one are different acts, and a UI that + labelled both "overridden here" would misdescribe the first.""" + resolved = _by_name(resolve_tokens( + APP, PARENTS, + {FAMILY: [], APP: [_token("--fs-editor-caret", {"base": "#5b4a8a"})]}, + )) + caret = resolved["--fs-editor-caret"] + assert caret.origin_by_mode == {"base": APP} + assert caret.is_overridden_in(APP) is False + + +def test_is_overridden_in_is_true_only_for_the_system_that_shadowed(): + resolved = _by_name(resolve_tokens( + APP, PARENTS, + { + FAMILY: [_token("--fs-accent", {"base": "#6b2118"})], + APP: [_token("--fs-accent", {"base": "#5b4a8a"})], + }, + )) + accent = resolved["--fs-accent"] + assert accent.is_overridden_in(APP) is True + assert accent.is_overridden_in(FAMILY) is False + + +def test_three_levels_stack_nearest_first(): + """A chain deeper than family->app, to prove the walk isn't a single hop.""" + parents = {1: None, 2: 1, 3: 2} + resolved = _by_name(resolve_tokens( + 3, parents, + { + 1: [_token("--fs-bg", {"base": "a"})], + 2: [_token("--fs-bg", {"base": "b"})], + 3: [_token("--fs-bg", {"base": "c"})], + }, + )) + bg = resolved["--fs-bg"] + assert bg.value_by_mode == {"base": "c"} + assert [c.value for c in bg.contributions["base"]] == ["c", "b", "a"] + + +def test_resolving_the_family_itself_ignores_its_children(): + """Inheritance runs one way. A family system resolved on its own must not + pick up an app's overrides — otherwise every app would restyle the house.""" + resolved = _by_name(resolve_tokens( + FAMILY, PARENTS, + { + FAMILY: [_token("--fs-accent", {"base": "#6b2118"})], + APP: [_token("--fs-accent", {"base": "#5b4a8a"})], + }, + )) + assert resolved["--fs-accent"].value_by_mode == {"base": "#6b2118"} + + +def test_value_for_falls_back_to_the_base_mode(): + """A token that isn't mode-dependent carries only `base`, and asking it for + "dark" must yield that rather than nothing — the read rule the storage shape + implies.""" + resolved = _by_name(resolve_tokens( + FAMILY, PARENTS, {FAMILY: [_token("--fs-radius-md", {"base": "8px"})]}, + )) + radius = resolved["--fs-radius-md"] + assert radius.value_for("dark") == "8px" + assert radius.value_for("base") == "8px" + + +def test_value_for_prefers_an_explicit_mode_over_the_fallback(): + resolved = _by_name(resolve_tokens( + FAMILY, PARENTS, + {FAMILY: [_token("--fs-bg", {"base": "#f7f5ef", "dark": "#14171a"})]}, + )) + assert resolved["--fs-bg"].value_for("dark") == "#14171a" + + +def test_metadata_is_inherited_when_the_override_leaves_it_blank(): + """A child overriding a colour routinely says nothing about what the token is + FOR. Inheriting the family's description beats blanking it — the override was + about the value, not the meaning.""" + resolved = _by_name(resolve_tokens( + APP, PARENTS, + { + FAMILY: [_token( + "--fs-obsidian", {"base": "#14171a"}, + group_name="surface", purpose="page bg, deepest surface", + )], + APP: [_token("--fs-obsidian", {"base": "#101317"})], + }, + )) + obsidian = resolved["--fs-obsidian"] + assert obsidian.group_name == "surface" + assert obsidian.purpose == "page bg, deepest surface" + assert obsidian.value_by_mode == {"base": "#101317"} # the value still won + + +def test_an_override_that_states_metadata_wins_it_too(): + resolved = _by_name(resolve_tokens( + APP, PARENTS, + { + FAMILY: [_token("--fs-x", {"base": "a"}, purpose="family says")], + APP: [_token("--fs-x", {"base": "b"}, purpose="app says")], + }, + )) + assert resolved["--fs-x"].purpose == "app says" + + +def test_an_override_at_default_order_keeps_the_familys_position(): + """order_index 0 is the column DEFAULT, so it reads as unstated. Treating it + as "first" would let a colour-only override drag its token to the top of the + group — a visible reshuffle nobody asked for.""" + resolved = _by_name(resolve_tokens( + APP, PARENTS, + { + FAMILY: [_token("--fs-x", {"base": "a"}, order_index=7)], + APP: [_token("--fs-x", {"base": "b"})], + }, + )) + assert resolved["--fs-x"].order_index == 7 + + +def test_the_effective_set_is_ordered_by_group_then_position_with_ungrouped_last(): + resolved = resolve_tokens( + FAMILY, PARENTS, + {FAMILY: [ + _token("--fs-z", {"base": "1"}), # ungrouped + _token("--fs-b", {"base": "2"}, group_name="text", order_index=1), + _token("--fs-a", {"base": "3"}, group_name="surface", order_index=2), + _token("--fs-c", {"base": "4"}, group_name="surface", order_index=1), + ]}, + ) + assert [t.name for t in resolved] == ["--fs-c", "--fs-a", "--fs-b", "--fs-z"] + + +def test_resolution_terminates_on_a_corrupt_hierarchy(): + """Resolution inherits ancestry's visited-set. A loop reaching this function + must produce a truncated set, not a hung request — the defensive half of the + cycle guard, exercised end to end.""" + parents = {1: 2, 2: 1} + resolved = _by_name(resolve_tokens( + 1, parents, + {1: [_token("--fs-a", {"base": "one"})], 2: [_token("--fs-b", {"base": "two"})]}, + )) + assert set(resolved) == {"--fs-a", "--fs-b"} + # Each system contributes exactly once, not endlessly. + assert len(resolved["--fs-a"].contributions["base"]) == 1 diff --git a/tests/test_services_design_systems.py b/tests/test_services_design_systems.py index 9d8bdc5..ebbc94d 100644 --- a/tests/test_services_design_systems.py +++ b/tests/test_services_design_systems.py @@ -192,3 +192,47 @@ async def test_clearing_a_projects_design_system_skips_the_system_acl(): assert project.design_system_id is None acc.can_read_design_system.assert_not_awaited() + + +# --- resolution (step 2) ---------------------------------------------------- + +@pytest.mark.asyncio +async def test_resolve_denied_returns_none_not_an_empty_set(): + """None and [] mean different things here: "you may not see this" versus + "this chain genuinely holds no tokens". A caller that got [] for a denial + would render an empty design system as though it were real.""" + with patch("scribe.services.design_systems.access") as acc: + acc.can_read_design_system = AsyncMock(return_value=False) + from scribe.services.design_systems import resolve_design_system + assert await resolve_design_system(user_id=1, design_system_id=3) is None + + +@pytest.mark.asyncio +async def test_resolve_scopes_the_hierarchy_to_the_systems_OWNER_not_the_caller(): + """LOAD-BEARING for shared projects. A caller reading through a project + shared with them owns no link in the chain, so a caller-scoped parent map + returns an empty forest and the cascade truncates to one system — a page + that renders with plausible wrong values and no error anywhere. + + The ACL already grants read on the whole chain (reachability propagates + upward); this is the loading side keeping that promise. + """ + owner, caller = 42, 7 + system = MagicMock(deleted_at=None, owner_user_id=owner) + mock_session = _make_mock_session() + mock_session.get = AsyncMock(return_value=system) + mock_session.execute = AsyncMock( + return_value=MagicMock(scalars=MagicMock(return_value=MagicMock(all=lambda: []))) + ) + parent_map = AsyncMock(return_value={3: None}) + + with patch("scribe.services.design_systems.async_session") as mock_cls, \ + patch("scribe.services.design_systems._parent_map", parent_map), \ + patch("scribe.services.design_systems.access") as acc: + acc.can_read_design_system = AsyncMock(return_value=True) + mock_cls.return_value = mock_session + from scribe.services.design_systems import resolve_design_system + result = await resolve_design_system(user_id=caller, design_system_id=3) + + assert result == [] # empty chain, not None + assert parent_map.await_args.args[1] == owner # NOT `caller`