From 5b46be05105b71a53553075657204a48035e5c78 Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Sat, 25 Jul 2026 19:05:47 -0700 Subject: [PATCH 1/4] fix(render): self-relationships in-box, ownership as line style MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rendering a real 13-entity model surfaced two ER defects. A self-referential relationship was emitted as an ordinary edge. Mermaid's dagre ER layout has no self-loop handling and draws a runaway arc that swamps the canvas, independent of the label text. It now renders as a row inside the entity's own block, carrying the target-side cardinality, the word `owned` when owned, and the role (issue #26). Relationship labels fell back from role to ownership to raw cardinality. Long prose roles collided, and an owned relationship that also had a role lost its ownership signal entirely. Ownership now rides on the connector — solid (identifying) for owned, dashed otherwise — and the role is the only label. Ownership folds across a pair declared from both ends, so a parent's `owned` and a child's `referenced` draw one solid line. A new semantic lint warning steers prose out of `role` and into `note` (issue #27). The golden example gains a self-referential relationship, which is why neither defect was caught before. Conventions recorded in ADR-0008. Signed-off-by: Joe Beda --- docs/04-reading-the-diagrams.md | 141 +++++++++------ docs/05-parking-garage/garage.modelith.md | 20 +-- docs/05-parking-garage/index.md | 22 +-- docs/06-schema-reference.md | 21 ++- examples/example.modelith.md | 11 +- examples/example.modelith.yaml | 8 + internal/lint/lint.go | 29 ++++ internal/lint/lint_test.go | 65 +++++++ internal/render/mermaid/mermaid.go | 126 +++++++++++--- internal/render/mermaid/mermaid_test.go | 162 ++++++++++++++++-- plugin/skills/domain-model-author/SKILL.md | 4 + plugin/skills/domain-model-lint/SKILL.md | 3 +- .../adr/0008-er-diagram-conventions.md | 51 ++++++ 13 files changed, 554 insertions(+), 109 deletions(-) create mode 100644 project-docs/adr/0008-er-diagram-conventions.md diff --git a/docs/04-reading-the-diagrams.md b/docs/04-reading-the-diagrams.md index b1a2f8e..a4218a7 100644 --- a/docs/04-reading-the-diagrams.md +++ b/docs/04-reading-the-diagrams.md @@ -22,8 +22,8 @@ The ER diagram shows **two things**: the entities (the named concepts) and the entity — its attributes, actions, and the invariants that govern it — lives in the Markdown sections *below* the diagram, not inside it. -So the boxes are intentionally **empty** — each entity is just a plain labeled -box with no rows inside it: +So the boxes are usually **empty** — each entity is just a plain labeled box +with no rows inside it: ```mermaid erDiagram @@ -41,6 +41,9 @@ look at the raw Mermaid *source* behind the diagram, each entity is written won't see the braces in the rendered picture — just the empty box.) **The diagram is the structure; the text is the detail.** +The one exception is an entity related to *itself*, which appears as a row +inside its own box — see [Self-relationships](#self-relationships-live-inside-the-box). + ## The lines: relationships and cardinality A line between two entities is a relationship. The **symbols at each end** tell @@ -56,13 +59,15 @@ modelith uses just two endpoint symbols: | `>○` (crow's foot + circle) | **zero or many** | Combine the two ends and you get the four cardinalities a model can declare. -Each example below is exactly what modelith emits for that cardinality. +Each example below is exactly what modelith emits for that cardinality when the +relationship declares no role and no ownership — which is why the lines are +dashed and the labels empty. Both are explained in the next two sections. ### `1:1` — one to one ```mermaid erDiagram - User ||--|| Profile : "has" + User ||..|| Profile : "" ``` A bar (`|`) at both ends: **one** `User` relates to **one** `Profile`, and vice @@ -72,7 +77,7 @@ versa. ```mermaid erDiagram - Project ||--o{ Policy : "owned" + Project ||..o{ Policy : "" ``` A bar at `Project`, a crow's foot at `Policy`: **one** `Project` relates to @@ -83,7 +88,7 @@ A bar at `Project`, a crow's foot at `Policy`: **one** `Project` relates to ```mermaid erDiagram - Policy }o--|| Project : "referenced" + Policy }o..|| Project : "" ``` The mirror of the above, declared from the *many* side. **Many** `Policies` to @@ -94,34 +99,77 @@ which one you see just reflects which entity declared it. ```mermaid erDiagram - User }o--o{ Project : "Owner or Member" + User }o..o{ Project : "" ``` A crow's foot at both ends: **many** `Users` relate to **many** `Projects`. A `User` can be in several `Projects`; a `Project` can have several `Users`. +## The line style: owned vs referenced + +The line itself is either **solid** or **dashed**, and that is where a +relationship's `ownership` shows up: + +| Line | Means | +| --- | --- | +| **solid** (`--`) | **`owned`** (composition, an *identifying* relationship): the related entity is a *part of* this one and can't exist without it — delete the parent and it goes too. | +| **dashed** (`..`) | **`referenced`** (a *non-identifying* relationship): the related entity is independent; this one merely points at it. This is the default when a relationship doesn't say. | + +```mermaid +erDiagram + Project ||--o{ Policy : "" + Project }o..o{ User : "" +``` + +A `Policy` is `owned` by its `Project` — solid. A `Project` merely *references* +the `Users` on it — dashed; deleting the `Project` doesn't delete the `Users`. + +Ownership belongs to the relationship, not to the end that declared it. If a +`Project` says it owns its `Policies` and the `Policy` says it references its +`Project`, that's one identifying relationship seen from both ends, so it draws +as a single solid line. + ## The labels on the lines -Every line carries a quoted label. It comes from the first of these that the -model provides, so a label means one of three things: - -1. **A role** describing the relationship — e.g. `"Owner or Member"`. The most - descriptive label; written when the relationship plays a named part in the - domain. -2. **Ownership** — `"owned"` or `"referenced"`: - - **`owned`** (composition): the related entity is a *part of* this one and - can't exist without it — delete the parent and it goes too. A `Policy` is - `owned` by its `Project`. - - **`referenced`**: the related entity is independent; this one merely points - at it. A `Project` *references* the `Users` on it — deleting the `Project` - doesn't delete the `Users`. -3. **The raw cardinality** (e.g. `"1:n"`) — a fallback when neither a role nor - ownership was specified. - -Crow's-foot notation has no glyph for ownership, so **the label is the only -place owned-vs-referenced appears in the diagram.** Worth internalizing: two -lines can look identical and mean very different things depending on whether the -label says `owned` or `referenced`. +A label on a line is the relationship's **`role`** — the part the related entity +plays, e.g. `"Owner or Member"`. Nothing else is ever written there: a +relationship with no role gets an empty label. + +```mermaid +erDiagram + Project }o..o{ User : "Owner or Member" +``` + +That's a deliberate diet. Ownership is in the line style, and the exact +cardinality (`1:0..1`, `1:2`) is in the per-entity table below the diagram — +spending label space on either would crowd out the roles and, for long text, +collide with neighbouring lines. Keep a `role` to a short role name, ideally a +glossary term; `modelith lint` warns when a role reads as prose and points you +at the relationship's `note` field instead. + +## Self-relationships live inside the box + +When an entity relates to *itself* — a `Project` that replaced an earlier one, a +`Task` that blocks another `Task` — the relationship is drawn as a **row inside +that entity's box** rather than as a line looping back on it: + +```mermaid +erDiagram + Project { + Project self "0..1 — Predecessor" + } + Policy {} + Project ||--o{ Policy : "" +``` + +Read the row as: this `Project` relates to `0..1` other `Project`, which plays +the role `Predecessor`. Because there's no line to carry it, the row spells out +the cardinality, the word `owned` when the relationship is owned, and the role. +An entity with several self-relationships gets one row each (`self`, `self2`, …). + +This is a layout necessity, not a modeling statement: Mermaid's ER layout has no +self-loop handling and draws an arc that swamps the rest of the diagram. See +[ADR-0008](https://github.com/stacklok/modelith/blob/main/project-docs/adr/0008-er-diagram-conventions.md). ## What the diagram can't tell you @@ -144,30 +192,27 @@ Here is the diagram modelith renders for the [worked example](https://github.com ```mermaid erDiagram Policy {} - Project {} + Project { + Project self "0..1 — Predecessor" + } User {} - Policy }o--|| Project : "referenced" - Project }o--o{ User : "Owner or Member" - Project ||--o{ Policy : "owned" + Policy }o--|| Project : "" + Project }o..o{ User : "Owner or Member" ``` -Reading each line: - -- **`Policy }o--|| Project : "referenced"`** — zero-or-many `Policies` point at - exactly one `Project`; from the `Policy` side, this is a reference to its - owning project. -- **`Project }o--o{ User : "Owner or Member"`** — many-to-many between - `Projects` and `Users`, where a `User`'s role is `Owner` or `Member`. -- **`Project ||--o{ Policy : "owned"`** — one `Project` owns zero-or-many - `Policies`; the `owned` label says the `Policies` are part of the `Project` and - die with it. - -Notice the `Project`–`Policy` pair has **two lines** (`referenced` and `owned`): -the example declares that relationship from *both* entities, each with its own -label. modelith keeps both because their labels differ — it's showing you both -points of view. Usually you'll declare a relationship from one side and see a -single line. (The two declarations must agree on cardinality, or `modelith lint` -flags a contradiction.) +Reading it: + +- **`Policy }o--|| Project : ""`** — zero-or-many `Policies` to exactly one + `Project`, on a **solid** line: the `Policies` are part of the `Project` and + die with it. The example declares this relationship from *both* entities + (`Project` owns `Policy`; `Policy` references its `Project`) — one + relationship seen from two ends, so it draws once. The two declarations must + agree on cardinality, or `modelith lint` flags a contradiction. +- **`Project }o..o{ User : "Owner or Member"`** — many-to-many between + `Projects` and `Users` on a **dashed** line: a `User`'s role is `Owner` or + `Member`, and neither entity is part of the other. +- **the `Project self` row** — a `Project` optionally points at the archived + `Project` it replaced, its `Predecessor`. To go deeper on the underlying fields, see the [Schema Reference](./06-schema-reference.md). diff --git a/docs/05-parking-garage/garage.modelith.md b/docs/05-parking-garage/garage.modelith.md index 9e00707..4559d09 100644 --- a/docs/05-parking-garage/garage.modelith.md +++ b/docs/05-parking-garage/garage.modelith.md @@ -231,16 +231,16 @@ erDiagram Spot {} Ticket {} Visit {} - Account ||--o{ Keycard : "owned" - Account ||--o{ Car : "owned" - Garage ||--o{ Spot : "owned" - Garage ||--o{ Kiosk : "owned" - Spot }o--|| Account : "referenced" - Ticket }o--|| Kiosk : "referenced" - Visit }o--|| Spot : "referenced" - Visit }o--|| Car : "referenced" - Visit }o--|| Keycard : "referenced" - Visit ||--|| Ticket : "owned" + Account ||--o{ Keycard : "" + Account ||--o{ Car : "" + Garage ||--o{ Spot : "" + Garage ||--o{ Kiosk : "" + Spot }o..|| Account : "" + Ticket }o..|| Kiosk : "" + Visit }o..|| Spot : "" + Visit }o..|| Car : "" + Visit }o..|| Keycard : "" + Visit ||--|| Ticket : "" ``` ## Scenarios diff --git a/docs/05-parking-garage/index.md b/docs/05-parking-garage/index.md index 38302c2..9e99ece 100644 --- a/docs/05-parking-garage/index.md +++ b/docs/05-parking-garage/index.md @@ -80,16 +80,16 @@ erDiagram Spot {} Ticket {} Visit {} - Account ||--o{ Keycard : "owned" - Account ||--o{ Car : "owned" - Garage ||--o{ Spot : "owned" - Garage ||--o{ Kiosk : "owned" - Spot ||--|| Account : "referenced" - Ticket }o--|| Kiosk : "referenced" - Visit }o--|| Spot : "referenced" - Visit }o--|| Car : "referenced" - Visit }o--|| Keycard : "referenced" - Visit ||--|| Ticket : "owned" + Account ||--o{ Keycard : "" + Account ||--o{ Car : "" + Garage ||--o{ Spot : "" + Garage ||--o{ Kiosk : "" + Spot ||..|| Account : "" + Ticket }o..|| Kiosk : "" + Visit }o..|| Spot : "" + Visit }o..|| Car : "" + Visit }o..|| Keycard : "" + Visit ||--|| Ticket : "" ``` > **Agent:** This is the minimum useful model — honest to stop here and @@ -105,7 +105,7 @@ erDiagram > **You:** Let's go `n:1` for spot→account. Any of the cars on an account can > park in any of its spots. -A one-line change to the cardinality; the diagram updates to `Spot }o--|| Account`. +A one-line change to the cardinality; the diagram updates to `Spot }o..|| Account`. ## Pass 2 — the behavior diff --git a/docs/06-schema-reference.md b/docs/06-schema-reference.md index dad29fe..87ac66e 100644 --- a/docs/06-schema-reference.md +++ b/docs/06-schema-reference.md @@ -129,7 +129,7 @@ as derived entities. | `entity` | string | yes | Target entity name. Must reference a defined entity. | | `cardinality` | string | yes | Written `left:right` (see below). `1:1`, `1:n`, `n:1`, `n:n` are the common shorthands. | | `symmetric` | boolean | no | The relationship carries no inherent order: `(a, b)` is the same as `(b, a)`. Only valid on a self-referential relationship or one whose target side is more than one. | -| `role` | string | no | The role the related entity plays. Backtick entity names. | +| `role` | string | no | The **short** role the related entity plays (`Owner`, `Predecessor`) — ideally a glossary term. Backtick entity and glossary names. It is the only label the diagram draws, so prose belongs in `note`; the linter warns on a role that reads as a sentence. | | `ownership` | enum | no | Is the related entity *part of* this one? `owned` = it can't exist independently (composition: created within, and deleted with, this entity); `referenced` = an independent entity this one points at. Omitted ⇒ `referenced`. | | `note` | string | no | Freeform note. | @@ -154,6 +154,22 @@ relationship there (e.g. on `Project`, not `Policy`). It keeps each link in one obvious place and reads the way the domain does. Declare from both ends only when both views genuinely add clarity. +**How a relationship draws.** Three conventions, recorded in +[ADR-0008](https://github.com/stacklok/modelith/blob/main/project-docs/adr/0008-er-diagram-conventions.md) +and covered in full in [Reading the Diagrams](./04-reading-the-diagrams.md): + +- **`ownership` is the line style** — solid (identifying) for `owned`, dashed + (non-identifying) for `referenced` and for an omitted `ownership`. It costs no + label space. Ownership belongs to the relationship rather than the end that + declared it, so a parent's `owned` and the child's `referenced` fold into one + solid line. +- **`role` is the only label** — `ownership` and `cardinality` are never written + on a line. Keep the role short; put the explanation in `note`. +- **A self-referential relationship becomes a row inside the entity's box** + (`Project self "0..1 — Predecessor"`), not a line looping back on it. Mermaid's + ER layout has no self-loop handling, and the arc it draws swamps the diagram. + The row carries the cardinality, `owned` when owned, and the role. + ## Attribute | Field | Type | Required | Notes | @@ -282,6 +298,9 @@ The JSON Schema covers structure. [`modelith lint`](./07-cli.md) adds: term, role, or actor; - a relationship `role` that resolves to neither an entity nor a glossary term — define it in the glossary; + - a relationship `role` that reads as prose (more than four words, or + sentence punctuation) — the role is the only label on the rendered + diagram line, so the explanation belongs in `note`; - an attribute `type` that looks like an enum reference (PascalCase) but names no defined enum; - an action `actor` that is neither a defined entity nor a glossary term. diff --git a/examples/example.modelith.md b/examples/example.modelith.md index 48f5e29..346deb7 100644 --- a/examples/example.modelith.md +++ b/examples/example.modelith.md @@ -8,6 +8,7 @@ A small, illustrative model used in the docs and as a golden fixture for the too - **`Member`** — A `User` granted access to a `Project` without ownership rights. - **`Owner`** — A `User` with full control of a `Project` — can transfer ownership, archive it, and manage its `Policies`. +- **`Predecessor`** — An archived `Project` that a newer one replaced, kept so the history of the work isn't lost. ## Enums @@ -50,6 +51,7 @@ A container for a set of related `Policies`, owned by at least one `User`. `Proj - `User` — n:n — referenced — `Owner` or `Member` — Must always have at least one `Owner` - `Policy` — 1:n — owned +- `Project` — 1:0..1 — referenced — `Predecessor` — The archived `Project` this one replaced, if any **Attributes** @@ -88,11 +90,12 @@ A human principal who can own or belong to `Projects`. Identity is managed exter ```mermaid erDiagram Policy {} - Project {} + Project { + Project self "0..1 — Predecessor" + } User {} - Policy }o--|| Project : "referenced" - Project }o--o{ User : "Owner or Member" - Project ||--o{ Policy : "owned" + Policy }o--|| Project : "" + Project }o..o{ User : "Owner or Member" ``` ## Invariants diff --git a/examples/example.modelith.yaml b/examples/example.modelith.yaml index dd67e5f..3d71bd6 100644 --- a/examples/example.modelith.yaml +++ b/examples/example.modelith.yaml @@ -13,6 +13,9 @@ glossary: it, and manage its `Policies`. Member: > A `User` granted access to a `Project` without ownership rights. + Predecessor: > + An archived `Project` that a newer one replaced, kept so the history of the + work isn't lost. enums: ProjectStatus: @@ -37,6 +40,11 @@ entities: - entity: Policy cardinality: "1:n" ownership: owned + - entity: Project + cardinality: "1:0..1" + role: "`Predecessor`" + ownership: referenced + note: The archived `Project` this one replaced, if any attributes: - name: status type: ProjectStatus diff --git a/internal/lint/lint.go b/internal/lint/lint.go index 2142136..d328299 100644 --- a/internal/lint/lint.go +++ b/internal/lint/lint.go @@ -300,6 +300,20 @@ func runSemantic(m *model.Model, res *Result) { Message: fmt.Sprintf("entity %q owns %q, which is derived — composing an ephemeral, never-persisted entity is usually a modeling error", name, rel.Entity), }) } + // A prose role wrecks the diagram: it is the only label on the + // rendered relationship line (ADR-0008), so a sentence there + // collides with its neighbours. `note` is the field for prose. + if readsAsProse(rel.Role) { + res.Findings = append(res.Findings, Finding{ + Severity: SeverityWarning, + Category: CategorySemantic, + Path: fmt.Sprintf("/entities/%s/relationships/%d/role", name, i), + Message: fmt.Sprintf( + "role %q reads as prose — keep the role to a short role name (ideally a glossary term) and move the explanation to the relationship's note", + rel.Role, + ), + }) + } // A role names a non-entity vocabulary term; it should resolve to an // entity or a glossary term (the DDD-1 payoff — undefined roles). for _, base := range entityRefs(rel.Role) { @@ -791,6 +805,21 @@ func entityRefs(text string) []string { return out } +// readsAsProse reports whether a relationship role is written as a sentence +// rather than a role name. The heuristic is deliberately loose — more than four +// words, or sentence punctuation — because it only ever raises a warning: +// "`Owner` or `Member`" passes, "the record this one supersedes" does not. +func readsAsProse(role string) bool { + role = strings.TrimSpace(role) + if role == "" { + return false + } + if strings.ContainsAny(role, ",.;") { + return true + } + return len(strings.Fields(role)) > 4 +} + // plural is a naive English pluralizer good enough to match entity names like // Policy -> Policies and Project -> Projects. func plural(s string) string { diff --git a/internal/lint/lint_test.go b/internal/lint/lint_test.go index 3a25ca6..826f396 100644 --- a/internal/lint/lint_test.go +++ b/internal/lint/lint_test.go @@ -2,6 +2,7 @@ package lint import ( "os" + "strconv" "strings" "testing" ) @@ -577,6 +578,70 @@ scenarios: } } +// A prose role is the only label on the rendered relationship line (ADR-0008), +// so it collides with its neighbours in the diagram. The linter steers the +// prose to `note` — as a warning, never an error. +func TestProseRoleIsWarning(t *testing.T) { + t.Parallel() + cases := []struct { + name string + role string + want bool + }{ + {name: "role name", role: "`Owner`", want: false}, + {name: "two role names", role: "`Owner` or `Member`", want: false}, + {name: "four words", role: "primary contact for escalation", want: false}, + {name: "five words", role: "the record this one supersedes", want: true}, + {name: "comma", role: "owner, or member", want: true}, + {name: "full stop", role: "The owning project.", want: true}, + {name: "semicolon", role: "owner; also billing", want: true}, + {name: "empty", role: "", want: false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + src := ` +kind: DomainModel +version: v1 +entities: + Project: + definition: A container. + relationships: + - entity: User + cardinality: "n:n" + role: ` + strconv.Quote(tc.role) + ` + User: + definition: A principal. +` + res, err := Run([]byte(src)) + if err != nil { + t.Fatal(err) + } + got := findingWithMessage(res.Findings, "reads as prose") + if got != tc.want { + t.Fatalf("role %q: prose warning = %v, want %v; findings: %+v", tc.role, got, tc.want, res.Findings) + } + if !got { + return + } + for _, f := range res.Findings { + if !strings.Contains(f.Message, "reads as prose") { + continue + } + if f.Severity != SeverityWarning || f.Category != CategorySemantic { + t.Errorf("expected a semantic warning, got %s/%s", f.Severity, f.Category) + } + if want := "/entities/Project/relationships/0/role"; f.Path != want { + t.Errorf("path = %q, want %q", f.Path, want) + } + } + if res.HasBlocking(false) { + t.Error("a prose role is advisory, not blocking") + } + }) + } +} + func TestActionPreservesUnknownInvariantIsError(t *testing.T) { src := ` kind: DomainModel diff --git a/internal/render/mermaid/mermaid.go b/internal/render/mermaid/mermaid.go index 2b2bc05..49434ca 100644 --- a/internal/render/mermaid/mermaid.go +++ b/internal/render/mermaid/mermaid.go @@ -6,6 +6,7 @@ package mermaid import ( "fmt" "sort" + "strconv" "strings" "github.com/stacklok/modelith/internal/model" @@ -17,12 +18,27 @@ import ( // precise count lives in the Markdown table and role instead (see ADR-0002). // An unparseable cardinality falls back to many-to-many, matching the schema's // pre-validation expectation that it is already a structural error. -func erMarkers(card string) string { +// +// owned selects the connector: an identifying line ("--") for composition, a +// non-identifying one ("..") otherwise, so ownership costs no label space +// (ADR-0008). +func erMarkers(card string, owned bool) string { + line := connector(owned) left, right, ok := model.ParseCardinality(card) if !ok { - return "}o--o{" + return "}o" + line + "o{" } - return leftMarker(left) + "--" + rightMarker(right) + return leftMarker(left) + line + rightMarker(right) +} + +// connector is the Mermaid relationship line: identifying (solid) when the +// related entity is owned, non-identifying (dashed) otherwise. The schema's +// default for an omitted ownership is "referenced", so omitted draws dashed. +func connector(owned bool) string { + if owned { + return "--" + } + return ".." } // minChar is the innermost glyph (nearest the relationship line): "o" for a @@ -58,23 +74,45 @@ func rightMarker(m model.Multiplicity) string { return minChar(m) + "|" } -// ER renders the model as a Mermaid erDiagram. Attributes are intentionally -// omitted: their freeform conceptual types (e.g. "enum[active, archived]") -// aren't valid erDiagram attribute types. Attributes are shown in the Markdown -// table instead. +// edge is one rendered relationship line, accumulated before emission so that +// declarations of the same relationship from both ends fold into it. +type edge struct { + from, to string + card string // as declared by `from`, so the markers read left to right + label string + owned bool +} + +// ER renders the model as a Mermaid erDiagram. Ordinary attributes are +// intentionally omitted: their freeform conceptual types (e.g. +// "enum[active, archived]") aren't valid erDiagram attribute types, so they are +// shown in the Markdown table instead. The one thing inside an entity block is +// its self-referential relationships (see selfRows). func ER(m *model.Model) string { var b strings.Builder b.WriteString("erDiagram\n") // Declare every entity so unconnected ones still appear. for _, name := range m.EntityNames() { - fmt.Fprintf(&b, " %s {}\n", name) + rows := selfRows(name, m.Entities[name].Relationships) + if len(rows) == 0 { + fmt.Fprintf(&b, " %s {}\n", name) + continue + } + fmt.Fprintf(&b, " %s {\n", name) + for _, row := range rows { + fmt.Fprintf(&b, " %s\n", row) + } + b.WriteString(" }\n") } - seen := map[string]bool{} + var edges []*edge + byKey := map[string]*edge{} for _, name := range m.EntityNames() { for _, rel := range m.Entities[name].Relationships { - notation := erMarkers(rel.Cardinality) + if rel.Entity == name { + continue // already rendered inside the entity's block + } label := relationshipLabel(rel) // Dedupe edges declared from both sides of the same pair. The key @@ -96,26 +134,70 @@ func ER(m *model.Model) string { // ("1:n" and "0..n:1") dedupes to one edge. card = model.CanonicalCardinality(card) key := pair[0] + "\x00" + pair[1] + "\x00" + card + "\x00" + label - if seen[key] { + if e, ok := byKey[key]; ok { + // Ownership belongs to the relationship, not to the end that + // declared it: a parent declaring `owned` and the child + // declaring `referenced` are one identifying relationship seen + // from two sides, so the folded edge stays solid (ADR-0008). + e.owned = e.owned || rel.Ownership == "owned" continue } - seen[key] = true - - fmt.Fprintf(&b, " %s %s %s : %q\n", name, notation, rel.Entity, label) + e := &edge{from: name, to: rel.Entity, card: rel.Cardinality, label: label, owned: rel.Ownership == "owned"} + byKey[key] = e + edges = append(edges, e) } } + + for _, e := range edges { + fmt.Fprintf(&b, " %s %s %s : %q\n", e.from, erMarkers(e.card, e.owned), e.to, e.label) + } return b.String() } -func relationshipLabel(rel model.Relationship) string { - switch { - case rel.Role != "": - return sanitize(rel.Role) - case rel.Ownership != "": - return rel.Ownership - default: - return rel.Cardinality +// selfRows renders an entity's self-referential relationships as rows inside +// its own block. Mermaid's dagre ER layout has no self-loop handling, so an +// edge from an entity to itself draws a runaway arc that swamps the canvas +// (issue #26); the row carries the same information without a line (ADR-0008). +// Row names are self, self2, self3… — distinct, since Mermaid does not +// disambiguate two attributes sharing a name. +func selfRows(name string, rels []model.Relationship) []string { + var rows []string + for _, rel := range rels { + if rel.Entity != name { + continue + } + attr := "self" + if n := len(rows) + 1; n > 1 { + attr = "self" + strconv.Itoa(n) + } + rows = append(rows, fmt.Sprintf("%s %s %q", name, attr, selfComment(rel))) + } + return rows +} + +// selfComment is the comment column of a self-relationship row: the target-side +// cardinality, the ownership when owned (the line style that carries it +// elsewhere has no line here), then the role. +func selfComment(rel model.Relationship) string { + target := rel.Cardinality + if _, right, ok := strings.Cut(rel.Cardinality, ":"); ok { + target = right } + out := target + if rel.Ownership == "owned" { + out += " owned" + } + if rel.Role != "" { + out += " — " + rel.Role + } + return sanitize(out) +} + +// relationshipLabel is the quoted text on a relationship line: the role, or +// nothing. Ownership rides on the line style instead, and the precise +// cardinality lives in the Markdown table (ADR-0002, ADR-0008). +func relationshipLabel(rel model.Relationship) string { + return sanitize(rel.Role) } // sanitize strips or replaces characters that would break a quoted Mermaid diff --git a/internal/render/mermaid/mermaid_test.go b/internal/render/mermaid/mermaid_test.go index 7499b9e..6c3744d 100644 --- a/internal/render/mermaid/mermaid_test.go +++ b/internal/render/mermaid/mermaid_test.go @@ -23,6 +23,9 @@ func TestERDeclaresAllEntities(t *testing.T) { } } +// TestERCardinalityNotation covers the crow's-foot glyphs. Every case declares +// ownership so the connector stays solid ("--") and the assertion is about the +// end markers alone; the connector itself is TestADR_0008_OwnershipIsLineStyle. func TestERCardinalityNotation(t *testing.T) { cases := map[string]string{ "1:1": "||--||", @@ -33,7 +36,7 @@ func TestERCardinalityNotation(t *testing.T) { } for card, want := range cases { m := &model.Model{Entities: map[string]model.Entity{ - "A": {Definition: "a", Relationships: []model.Relationship{{Entity: "B", Cardinality: card}}}, + "A": {Definition: "a", Relationships: []model.Relationship{{Entity: "B", Cardinality: card, Ownership: "owned"}}}, "B": {Definition: "b"}, }} out := ER(m) @@ -84,9 +87,9 @@ func TestERRendersConflictingReciprocalEdges(t *testing.T) { } } -func TestERLabelPrecedenceAndSanitize(t *testing.T) { - // role wins over ownership wins over cardinality; backticks/brackets/newlines - // are neutralized so the quoted label stays valid Mermaid. +func TestERLabelSanitizesRole(t *testing.T) { + // backticks/brackets/newlines are neutralized so the quoted label stays + // valid Mermaid. m := &model.Model{Entities: map[string]model.Entity{ "A": {Definition: "a", Relationships: []model.Relationship{ {Entity: "B", Cardinality: "1:n", Role: "`Owner` [x]\nmore", Ownership: "owned"}, @@ -99,24 +102,159 @@ func TestERLabelPrecedenceAndSanitize(t *testing.T) { } } -func TestERLabelFallsBackToOwnershipThenCardinality(t *testing.T) { +// TestADR_0008_LabelIsRoleOrEmpty pins the label half of ADR-0008: the role is +// the only thing that labels a line. Ownership rides on the line style and the +// precise cardinality lives in the Markdown table, so neither is a fallback. +func TestADR_0008_LabelIsRoleOrEmpty(t *testing.T) { m := &model.Model{Entities: map[string]model.Entity{ "A": {Definition: "a", Relationships: []model.Relationship{{Entity: "B", Cardinality: "1:n", Ownership: "owned"}}}, - "B": {Definition: "b", Relationships: []model.Relationship{{Entity: "A", Cardinality: "n:1"}}}, + "B": {Definition: "b", Relationships: []model.Relationship{{Entity: "C", Cardinality: "n:1"}}}, + "C": {Definition: "c"}, }} out := ER(m) - if !strings.Contains(out, `: "owned"`) { - t.Errorf("expected ownership label when role is absent; got:\n%s", out) + for _, want := range []string{" A ||--o{ B : \"\"\n", " B }o..|| C : \"\"\n"} { + if !strings.Contains(out, want) { + t.Errorf("expected empty label line %q; got:\n%s", want, out) + } + } + for _, unwanted := range []string{`"owned"`, `"referenced"`, `"1:n"`, `"n:1"`} { + if strings.Contains(out, unwanted) { + t.Errorf("expected no %s label; got:\n%s", unwanted, out) + } + } +} + +// TestADR_0008_OwnershipIsLineStyle pins the connector half of ADR-0008: owned +// draws an identifying (solid) line, referenced and omitted a non-identifying +// (dashed) one. +func TestADR_0008_OwnershipIsLineStyle(t *testing.T) { + cases := map[string]string{ + "owned": " A ||--o{ B : \"\"\n", + "referenced": " A ||..o{ B : \"\"\n", + "": " A ||..o{ B : \"\"\n", // omitted defaults to referenced + } + for ownership, want := range cases { + m := &model.Model{Entities: map[string]model.Entity{ + "A": {Definition: "a", Relationships: []model.Relationship{{Entity: "B", Cardinality: "1:n", Ownership: ownership}}}, + "B": {Definition: "b"}, + }} + if out := ER(m); !strings.Contains(out, want) { + t.Errorf("ownership %q: expected %q in:\n%s", ownership, want, out) + } + } +} + +// TestADR_0008_OwnershipFoldsAcrossDeclarations pins the dedupe half of +// ADR-0008: ownership belongs to the relationship, not to the end that declared +// it, so a parent's `owned` and the child's `referenced` fold into one solid +// edge rather than a contradictory solid-plus-dashed pair. +func TestADR_0008_OwnershipFoldsAcrossDeclarations(t *testing.T) { + m := &model.Model{Entities: map[string]model.Entity{ + "Parent": {Definition: "p", Relationships: []model.Relationship{{Entity: "Child", Cardinality: "1:n", Ownership: "owned"}}}, + "Child": {Definition: "c", Relationships: []model.Relationship{{Entity: "Parent", Cardinality: "n:1", Ownership: "referenced"}}}, + }} + out := ER(m) + if want := " Child }o--|| Parent : \"\"\n"; !strings.Contains(out, want) { + t.Errorf("expected folded identifying edge %q; got:\n%s", want, out) } - if !strings.Contains(out, `: "n:1"`) { - t.Errorf("expected cardinality label when role and ownership are absent; got:\n%s", out) + if n := strings.Count(out, " Parent : "); n != 1 { + t.Errorf("expected one edge between the pair, got %d:\n%s", n, out) + } +} + +// TestADR_0008_SelfRelationshipRendersInEntityBlock pins the self-relationship +// half of ADR-0008: Mermaid's ER layout has no self-loop handling (issue #26), +// so the relationship becomes a row in the entity's own block and no edge. +func TestADR_0008_SelfRelationshipRendersInEntityBlock(t *testing.T) { + m := &model.Model{Entities: map[string]model.Entity{ + "Record": {Definition: "r", Relationships: []model.Relationship{ + {Entity: "Record", Cardinality: "1:0..1", Role: "`Predecessor`"}, + {Entity: "Note", Cardinality: "1:n", Ownership: "owned"}, + }}, + "Note": {Definition: "n"}, + }} + out := ER(m) + want := " Record {\n Record self \"0..1 — Predecessor\"\n }\n" + if !strings.Contains(out, want) { + t.Errorf("expected self-relationship row %q; got:\n%s", want, out) + } + if strings.Contains(out, "Record ||--o| Record") || strings.Contains(out, "Record ||..o| Record") { + t.Errorf("expected no self edge; got:\n%s", out) + } + if !strings.Contains(out, " Note {}\n") { + t.Errorf("expected an entity with no self-relationships to stay {}; got:\n%s", out) + } +} + +func TestERSelfRelationshipRowContents(t *testing.T) { + cases := []struct { + name string + rel model.Relationship + want string + }{ + { + name: "cardinality only", + rel: model.Relationship{Entity: "Record", Cardinality: "1:n"}, + want: `Record self "n"`, + }, + { + name: "owned is spelled out, since there is no line to carry it", + rel: model.Relationship{Entity: "Record", Cardinality: "1:n", Ownership: "owned", Role: "`Part`"}, + want: `Record self "n owned — Part"`, + }, + { + name: "referenced stays implicit, matching the dashed default", + rel: model.Relationship{Entity: "Record", Cardinality: "1:0..1", Ownership: "referenced", Role: "Predecessor"}, + want: `Record self "0..1 — Predecessor"`, + }, + { + name: "a quote in the role cannot break out of the comment", + rel: model.Relationship{Entity: "Record", Cardinality: "n:n", Role: `a "quoted" [role]`}, + want: `Record self "n — a 'quoted' (role)"`, + }, + { + name: "a cardinality with no colon is shown whole", + rel: model.Relationship{Entity: "Record", Cardinality: "bogus"}, + want: `Record self "bogus"`, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + m := &model.Model{Entities: map[string]model.Entity{ + "Record": {Definition: "r", Relationships: []model.Relationship{tc.rel}}, + }} + if out := ER(m); !strings.Contains(out, tc.want) { + t.Errorf("expected row %q in:\n%s", tc.want, out) + } + }) + } +} + +// TestERMultipleSelfRelationships guards the row names: Mermaid does not +// disambiguate two attributes sharing a name, so each row gets its own. +func TestERMultipleSelfRelationships(t *testing.T) { + m := &model.Model{Entities: map[string]model.Entity{ + "Record": {Definition: "r", Relationships: []model.Relationship{ + {Entity: "Record", Cardinality: "1:0..1", Role: "`Predecessor`"}, + {Entity: "Record", Cardinality: "1:n", Ownership: "owned", Role: "`Part`"}, + {Entity: "Record", Cardinality: "n:n", Symmetric: true, Role: "`Peer`"}, + }}, + }} + want := " Record {\n" + + " Record self \"0..1 — Predecessor\"\n" + + " Record self2 \"n owned — Part\"\n" + + " Record self3 \"n — Peer\"\n" + + " }\n" + if out := ER(m); !strings.Contains(out, want) { + t.Errorf("expected distinct self rows:\n%s\ngot:\n%s", want, out) } } // TestADR_0003_BoundedCardinalityRendersNearestGlyph pins the render half of // ADR-0003 and the capture-first principle in ADR-0002: Mermaid has no numeric // bound, so exact and bounded counts render as the nearest crow's-foot glyph -// (the precise count lives in the Markdown, not the diagram). +// (the precise count lives in the Markdown, not the diagram). Ownership is +// declared so the connector stays solid and the glyphs read unchanged. func TestADR_0003_BoundedCardinalityRendersNearestGlyph(t *testing.T) { cases := map[string]string{ "1:2": "||--|{", // exactly two -> one-or-many @@ -126,7 +264,7 @@ func TestADR_0003_BoundedCardinalityRendersNearestGlyph(t *testing.T) { } for card, want := range cases { m := &model.Model{Entities: map[string]model.Entity{ - "A": {Definition: "a", Relationships: []model.Relationship{{Entity: "B", Cardinality: card}}}, + "A": {Definition: "a", Relationships: []model.Relationship{{Entity: "B", Cardinality: card, Ownership: "owned"}}}, "B": {Definition: "b"}, }} if out := ER(m); !strings.Contains(out, want) { diff --git a/plugin/skills/domain-model-author/SKILL.md b/plugin/skills/domain-model-author/SKILL.md index 4188d99..b1f160f 100644 --- a/plugin/skills/domain-model-author/SKILL.md +++ b/plugin/skills/domain-model-author/SKILL.md @@ -114,6 +114,10 @@ Follow the format exactly (see the schema reference). Key conventions: relationship from both entities, the two cardinalities must be inverses (`1:n` ⇄ `n:1`; `1:1`/`n:n` symmetric) — a contradiction is a lint **error**. Declaring it from one side only is fine and usually clearer. +- **`role` is a short role name** (`` `Owner` ``, `` `Predecessor` ``), not a + sentence: it is the only label drawn on the diagram line, so prose there + collides with neighbouring lines and warns. Put the explanation in `note`. + `ownership` needs no label — it draws as a solid vs dashed line. - **`glossary`** (top level) defines non-entity vocabulary — roles like `Owner`, states, domain nouns — as `Term: "definition"`. Define any role or actor name here; an undefined role warns, and an unused glossary term warns. diff --git a/plugin/skills/domain-model-lint/SKILL.md b/plugin/skills/domain-model-lint/SKILL.md index ca3a619..5b030ff 100644 --- a/plugin/skills/domain-model-lint/SKILL.md +++ b/plugin/skills/domain-model-lint/SKILL.md @@ -48,7 +48,8 @@ the flag. that aren't inverses. Warnings: a backticked term resolves to no entity, role, or actor; an action `actor` that's neither an entity nor a glossary term; a PascalCase attribute `type` that names no defined enum — usually a typo or a - concept that was never named. Decide which it is and propose the fix. + concept that was never named; a relationship `role` written as prose, which + belongs in `note`. Decide which it is and propose the fix. - **Completeness (advisory warning).** Gaps, not bugs: an entity with no invariants, an entity no scenario exercises, a glossary term defined but never referenced, or an enum no attribute uses. These are what `--completeness error` diff --git a/project-docs/adr/0008-er-diagram-conventions.md b/project-docs/adr/0008-er-diagram-conventions.md new file mode 100644 index 0000000..ff47331 --- /dev/null +++ b/project-docs/adr/0008-er-diagram-conventions.md @@ -0,0 +1,51 @@ +# ER diagram conventions: line style, labels, and self-relationships + +Rendering a real 13-entity model made two defects obvious (issues +[#26](https://github.com/stacklok/modelith/issues/26), +[#27](https://github.com/stacklok/modelith/issues/27)): a self-referential +relationship drew a runaway arc that swamped the canvas, and long prose roles +collided with each other while `owned` relationships that also carried a role +lost their ownership signal entirely. Three conventions fix both. They refine +ADR-0002's "deliberately lossy view" rather than overturning it: nothing here +fakes structure the ER cannot honestly show. + +## Decision + +1. **`ownership` is the line style.** `owned` draws the identifying (solid) + connector `--`; `referenced` and an omitted `ownership` draw the + non-identifying (dashed) `..`. That is standard ER semantics for composition + and costs no label space. Ownership is a property of the relationship, not of + the end that declared it, so when a parent's `owned` and a child's + `referenced` fold into one edge, the folded edge stays solid. +2. **`role` is the only label.** The `ownership` and `cardinality` fallbacks are + gone; a relationship with no role gets an empty label. Precise counts already + live in the Markdown table per ADR-0002, and spending label space on them + crowds out the roles. A semantic lint *warning* (never an error) fires when a + role reads as prose — more than four words, or sentence punctuation — and + names `note` as the field for the explanation. +3. **A self-referential relationship is a row inside the entity's block**, not + an edge: `Project self "0..1 — Predecessor"`. The row carries the target-side + cardinality, the word `owned` when owned (there is no line to carry it), and + the role. Rows are named `self`, `self2`, … because Mermaid does not + disambiguate two attributes sharing a name. + +## Evidence + +Verified against `@mermaid-js/mermaid-cli@11.16.0` and GitHub's renderer, July +2026. Mermaid's dagre ER layout has no self-loop handling: `Record ||--o| Record` +draws an arc that dominates or overflows the canvas, independent of the label +text, so no label-side workaround exists. `layout: elk` fixes the arc but is +rejected — GitHub does not bundle `@mermaid-js/layout-elk`. Mermaid accepts a +PascalCase entity name as an attribute type, so the in-box row is valid source +for both renderers. + +## Consequences + +Point 3 is the one place a *relationship* is drawn inside a box, so the +"attributes are intentionally omitted from the diagram" rule from ADR-0002 now +reads: ordinary attributes are omitted, self-relationships are not. Ownership +becomes visible at a glance across the whole diagram, which makes an incorrect +`ownership` easier to spot than the old word label did. + +Pinned by `TestADR_0008_*` in `internal/render/mermaid` and `TestProseRoleIsWarning` +in `internal/lint`. From b47bd363f24e369c6612c6f1aba41dd15101dd31 Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Sat, 25 Jul 2026 19:36:11 -0700 Subject: [PATCH 2/4] fix(render): fold only genuine reciprocals; keep both cardinality sides MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-1 review fixes for #28. The ownership fold was wrong in four directions: it fired on two declarations from the same end that differed only in `ownership` (one edge silently vanished), it swallowed mutual `owned` with no diagnostic, it left a reciprocal pair with differing roles drawing one solid and one dashed line, and `ER()`'s comment plus ADR-0008 asserted a guarantee the code did not deliver. Two declarations now merge in exactly two cases: an exact duplicate from the same end, and a genuine reciprocal (opposite ends, inverse cardinality, same label, at most one end claiming `owned`). Everything else draws both lines, so no declaration disappears. The contradictions the renderer refuses to swallow become lint errors: mutual `owned`, and a reciprocal pair whose ends disagree on ownership under different roles. Also: - Self-relationship rows carry the declared cardinality whole, not the substring after the colon, so `0..5:1` no longer renders as `1`. - Identical self-relationship declarations render one row, not two. - The prose-role heuristic judges the role as the diagram draws it: length, word count, `;` or a trailing `.` — no longer any comma or full stop, which false-positived on role lists, accessors and version numbers, and no longer blind to a 66-character single word. - A role raises at most one finding; the undefined-term check waits until the role is no longer prose. - `sanitize` drops backslashes, so `%q` cannot double-escape them. - ADR-0008, `docs/04`, `docs/06` and the plugin skills describe what the code actually does; the `docs/04` snippets match real CLI output. - `TestProseRoleIsWarning` renamed to `TestADR_0008_ProseRoleIsWarning`; the cosmetic `Symmetric` render case is replaced by real coverage of the Markdown marker. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Joe Beda --- docs/04-reading-the-diagrams.md | 34 ++-- docs/06-schema-reference.md | 22 ++- examples/example.modelith.md | 2 +- internal/lint/lint.go | 130 ++++++++++++--- internal/lint/lint_test.go | 150 +++++++++++++++++- internal/render/markdown/markdown_test.go | 20 +++ internal/render/mermaid/mermaid.go | 94 ++++++++--- internal/render/mermaid/mermaid_test.go | 140 ++++++++++++++-- plugin/skills/domain-model-author/SKILL.md | 4 +- plugin/skills/domain-model-lint/SKILL.md | 3 +- .../adr/0008-er-diagram-conventions.md | 47 ++++-- 11 files changed, 552 insertions(+), 94 deletions(-) diff --git a/docs/04-reading-the-diagrams.md b/docs/04-reading-the-diagrams.md index a4218a7..f73eca4 100644 --- a/docs/04-reading-the-diagrams.md +++ b/docs/04-reading-the-diagrams.md @@ -27,9 +27,9 @@ with no rows inside it: ```mermaid erDiagram + Policy {} Project {} User {} - Policy {} ``` Three entities, no connections drawn yet. In a generic ER diagram a box would @@ -127,7 +127,20 @@ the `Users` on it — dashed; deleting the `Project` doesn't delete the `Users`. Ownership belongs to the relationship, not to the end that declared it. If a `Project` says it owns its `Policies` and the `Policy` says it references its `Project`, that's one identifying relationship seen from both ends, so it draws -as a single solid line. +as a single solid line — provided the two declarations otherwise match (inverse +cardinalities and the same role). That is the only merge the renderer makes on +ownership. Two declarations it can't reconcile stay two lines, so nothing you +wrote vanishes from the picture: + +- If both ends claim `owned`, that's a contradiction — a relationship is owned + by at most one end. Both lines draw, and `modelith lint` reports it as an + error. +- If the two ends disagree on ownership but give the relationship different + roles, the renderer can't tell whether you meant one relationship or two. + Both lines draw, and `modelith lint` reports that as an error too. Give both + ends the same role, or declare the relationship from one end only. +- If one entity declares two relationships to the same entity that differ only + in `ownership`, those are two relationships, and both draw. ## The labels on the lines @@ -155,17 +168,19 @@ that entity's box** rather than as a line looping back on it: ```mermaid erDiagram + Policy {} Project { - Project self "0..1 — Predecessor" + Project self "1:0..1 — Predecessor" } - Policy {} Project ||--o{ Policy : "" ``` -Read the row as: this `Project` relates to `0..1` other `Project`, which plays +Read the row as: one `Project` relates to `0..1` other `Project`, which plays the role `Predecessor`. Because there's no line to carry it, the row spells out -the cardinality, the word `owned` when the relationship is owned, and the role. -An entity with several self-relationships gets one row each (`self`, `self2`, …). +the declared cardinality in full — both sides, since the two ends of a line are +what the row replaces — the word `owned` when the relationship is owned, and the +role. An entity with several self-relationships gets one row each (`self`, +`self2`, …). This is a layout necessity, not a modeling statement: Mermaid's ER layout has no self-loop handling and draws an arc that swamps the rest of the diagram. See @@ -193,7 +208,7 @@ Here is the diagram modelith renders for the [worked example](https://github.com erDiagram Policy {} Project { - Project self "0..1 — Predecessor" + Project self "1:0..1 — Predecessor" } User {} Policy }o--|| Project : "" @@ -207,7 +222,8 @@ Reading it: die with it. The example declares this relationship from *both* entities (`Project` owns `Policy`; `Policy` references its `Project`) — one relationship seen from two ends, so it draws once. The two declarations must - agree on cardinality, or `modelith lint` flags a contradiction. + agree — inverse cardinalities, the same role, and at most one end claiming + `owned` — or `modelith lint` flags the contradiction and both lines draw. - **`Project }o..o{ User : "Owner or Member"`** — many-to-many between `Projects` and `Users` on a **dashed** line: a `User`'s role is `Owner` or `Member`, and neither entity is part of the other. diff --git a/docs/06-schema-reference.md b/docs/06-schema-reference.md index 87ac66e..0b8448c 100644 --- a/docs/06-schema-reference.md +++ b/docs/06-schema-reference.md @@ -162,13 +162,16 @@ and covered in full in [Reading the Diagrams](./04-reading-the-diagrams.md): (non-identifying) for `referenced` and for an omitted `ownership`. It costs no label space. Ownership belongs to the relationship rather than the end that declared it, so a parent's `owned` and the child's `referenced` fold into one - solid line. + solid line — when the two declarations otherwise match (inverse cardinalities, + the same role). Declarations the renderer can't reconcile draw as separate + lines, and mutual `owned` or an unreconcilable disagreement is a lint error. - **`role` is the only label** — `ownership` and `cardinality` are never written on a line. Keep the role short; put the explanation in `note`. - **A self-referential relationship becomes a row inside the entity's box** - (`Project self "0..1 — Predecessor"`), not a line looping back on it. Mermaid's - ER layout has no self-loop handling, and the arc it draws swamps the diagram. - The row carries the cardinality, `owned` when owned, and the role. + (`Project self "1:0..1 — Predecessor"`), not a line looping back on it. + Mermaid's ER layout has no self-loop handling, and the arc it draws swamps the + diagram. The row carries the declared cardinality in full (both sides — the + two ends of the line it replaces), `owned` when owned, and the role. ## Attribute @@ -289,6 +292,11 @@ The JSON Schema covers structure. [`modelith lint`](./07-cli.md) adds: - a relationship target that doesn't reference a defined entity; - a relationship declared from both sides with cardinalities that aren't inverses (e.g. `Project`→`Policy` `1:n` but `Policy`→`Project` `1:1`); + - a relationship declared from both sides where both ends claim + `ownership: owned` — a relationship is owned by at most one end; + - a relationship declared from both sides whose ends disagree on + `ownership` and give it different roles, so the diagram would draw one + solid and one dashed line for the same relationship; - a duplicate invariant `id` (across entity-level *and* model-level invariants — they share one namespace); - a scenario `invariants_touched` or an action `preserves` that references an @@ -298,9 +306,9 @@ The JSON Schema covers structure. [`modelith lint`](./07-cli.md) adds: term, role, or actor; - a relationship `role` that resolves to neither an entity nor a glossary term — define it in the glossary; - - a relationship `role` that reads as prose (more than four words, or - sentence punctuation) — the role is the only label on the rendered - diagram line, so the explanation belongs in `note`; + - a relationship `role` that reads as prose (too long for a label, more + than four words, or ending a sentence) — the role is the only label on + the rendered diagram line, so the explanation belongs in `note`; - an attribute `type` that looks like an enum reference (PascalCase) but names no defined enum; - an action `actor` that is neither a defined entity nor a glossary term. diff --git a/examples/example.modelith.md b/examples/example.modelith.md index 346deb7..55a59e7 100644 --- a/examples/example.modelith.md +++ b/examples/example.modelith.md @@ -91,7 +91,7 @@ A human principal who can own or belong to `Projects`. Identity is managed exter erDiagram Policy {} Project { - Project self "0..1 — Predecessor" + Project self "1:0..1 — Predecessor" } User {} Policy }o--|| Project : "" diff --git a/internal/lint/lint.go b/internal/lint/lint.go index d328299..0c3a011 100644 --- a/internal/lint/lint.go +++ b/internal/lint/lint.go @@ -303,6 +303,11 @@ func runSemantic(m *model.Model, res *Result) { // A prose role wrecks the diagram: it is the only label on the // rendered relationship line (ADR-0008), so a sentence there // collides with its neighbours. `note` is the field for prose. + // + // One role raises one finding. Rewriting a prose role is the fix + // that comes first, and the terms buried in the sentence are likely + // to change with it, so the undefined-term check waits until the + // role is a role name. if readsAsProse(rel.Role) { res.Findings = append(res.Findings, Finding{ Severity: SeverityWarning, @@ -313,19 +318,21 @@ func runSemantic(m *model.Model, res *Result) { rel.Role, ), }) - } - // A role names a non-entity vocabulary term; it should resolve to an - // entity or a glossary term (the DDD-1 payoff — undefined roles). - for _, base := range entityRefs(rel.Role) { - if allowed[base] != "" || glossary[base] { - continue + } else { + // A role names a non-entity vocabulary term; it should resolve + // to an entity or a glossary term (the DDD-1 payoff — undefined + // roles). + for _, base := range entityRefs(rel.Role) { + if allowed[base] != "" || glossary[base] { + continue + } + res.Findings = append(res.Findings, Finding{ + Severity: SeverityWarning, + Category: CategorySemantic, + Path: fmt.Sprintf("/entities/%s/relationships/%d/role", name, i), + Message: fmt.Sprintf("role term %q is not a defined entity or glossary term — define it in the glossary", base), + }) } - res.Findings = append(res.Findings, Finding{ - Severity: SeverityWarning, - Category: CategorySemantic, - Path: fmt.Sprintf("/entities/%s/relationships/%d/role", name, i), - Message: fmt.Sprintf("role term %q is not a defined entity or glossary term — define it in the glossary", base), - }) } } } @@ -586,20 +593,32 @@ func inheritsInvariants(m *model.Model, name string) bool { return false } -// runReciprocity checks that a relationship declared from both sides agrees: +// runReciprocity checks that a relationship declared from both sides agrees, on +// cardinality and on ownership. +// // B→A must declare the inverse of A→B's cardinality. A contradiction (e.g. A // says "1:n" B while B says "1:1" A) is an error — the model can't be both, and // the renderer would otherwise draw two conflicting edges. // +// Ownership belongs to the relationship, not to the end that declared it, so at +// most one end may claim `owned`; both claiming it is a contradiction. And when +// the two ends disagree on ownership, the renderer only reconciles them into +// one line if they are otherwise the same drawn edge (ADR-0008). When they are +// not, the diagram shows one solid and one dashed line for what the model calls +// a single relationship, so that is an error too. +// // Only pairs with exactly one declaration in each direction are checked. // Multiple edges between the same pair (a legitimate pattern — e.g. a User is // both `Owner` and `Member` of a Project) can't be paired up unambiguously, so // they're left alone rather than guessed at. func runReciprocity(m *model.Model, res *Result) { type decl struct { - from, to string - card string - path string + from, to string + card string + role string + owned bool + path string + ownerPath string } byPair := map[string][]decl{} for _, name := range m.EntityNames() { @@ -608,10 +627,13 @@ func runReciprocity(m *model.Model, res *Result) { sort.Strings(pair) k := pair[0] + "\x00" + pair[1] byPair[k] = append(byPair[k], decl{ - from: name, - to: rel.Entity, - card: rel.Cardinality, - path: fmt.Sprintf("/entities/%s/relationships/%d/cardinality", name, i), + from: name, + to: rel.Entity, + card: rel.Cardinality, + role: rel.Role, + owned: rel.Ownership == "owned", + path: fmt.Sprintf("/entities/%s/relationships/%d/cardinality", name, i), + ownerPath: fmt.Sprintf("/entities/%s/relationships/%d/ownership", name, i), }) } } @@ -633,6 +655,21 @@ func runReciprocity(m *model.Model, res *Result) { continue } f, r := fwd[0], rev[0] + + // Mutual ownership is a contradiction whatever the cardinalities say, + // so it is checked before they are parsed. + if f.owned && r.owned { + res.Findings = append(res.Findings, Finding{ + Severity: SeverityError, + Category: CategorySemantic, + Path: f.ownerPath, + Message: fmt.Sprintf( + "mutual ownership: %s→%s and %s→%s both declare ownership \"owned\" — a relationship is owned by at most one end, so make the other end \"referenced\" (or omit it)", + f.from, f.to, r.from, r.to, + ), + }) + } + // Reciprocity is checked only for structurally valid cardinalities; an // invalid one is already reported (by the schema, and by // runRelationshipShape). Compare the parsed multiplicities, not the raw @@ -653,10 +690,39 @@ func runReciprocity(m *model.Model, res *Result) { f.from, f.to, f.card, r.from, r.to, r.card, model.InvertCardinality(f.card), ), }) + continue // the conflict is the thing to fix; don't pile on + } + + // The cardinalities are inverses, so the two ends describe one line. If + // they also disagree on ownership, the renderer folds them into a + // single solid line only when they carry the same role; otherwise it + // draws one solid and one dashed line for the same relationship + // (ADR-0008). Say so rather than let the diagram contradict itself. + if f.owned != r.owned && normalizeRole(f.role) != normalizeRole(r.role) { + owner, other := f, r + if r.owned { + owner, other = r, f + } + res.Findings = append(res.Findings, Finding{ + Severity: SeverityError, + Category: CategorySemantic, + Path: owner.ownerPath, + Message: fmt.Sprintf( + "reciprocal ownership conflict: %s→%s declares ownership \"owned\" but %s→%s does not, and their roles differ (%q vs %q), so the diagram draws one solid and one dashed line for the same relationship — give both ends the same role, or declare the relationship from one end only", + owner.from, owner.to, other.from, other.to, owner.role, other.role, + ), + }) } } } +// normalizeRole reduces a role to the text the diagram labels a line with, so +// two declarations of one relationship are compared the way the renderer +// compares them: backticks are markup, and surrounding space is not content. +func normalizeRole(role string) string { + return strings.TrimSpace(strings.ReplaceAll(role, "`", "")) +} + func runCompleteness(m *model.Model, res *Result) { // Entities with no invariants — unless a supertype's invariants cover them. for _, name := range m.EntityNames() { @@ -805,16 +871,30 @@ func entityRefs(text string) []string { return out } +// roleLabelMax is the longest a role may be before it crowds the diagram line +// it labels, whatever its shape. Measured in runes, so a non-ASCII role is not +// judged by its byte count. +const roleLabelMax = 40 + // readsAsProse reports whether a relationship role is written as a sentence -// rather than a role name. The heuristic is deliberately loose — more than four -// words, or sentence punctuation — because it only ever raises a warning: -// "`Owner` or `Member`" passes, "the record this one supersedes" does not. +// rather than a role name. It judges the role as the diagram will draw it, so +// backticks — markup, not label text — come off first. +// +// Three signals, each of which alone wrecks a diagram label: too long, too many +// words, or a sentence terminator. Sentence punctuation is deliberately *not* +// tested character-by-character: a comma separates a short list of role names +// ("`Owner`, `Member`"), and a full stop is as likely to be an accessor +// ("`Project`.owner") or a version ("v1.0 owner") as the end of a sentence. +// "`Owner` or `Member`" passes; "the record this one supersedes" does not. func readsAsProse(role string) bool { - role = strings.TrimSpace(role) + role = normalizeRole(role) if role == "" { return false } - if strings.ContainsAny(role, ",.;") { + if len([]rune(role)) > roleLabelMax { + return true + } + if strings.Contains(role, ";") || strings.HasSuffix(role, ".") { return true } return len(strings.Fields(role)) > 4 diff --git a/internal/lint/lint_test.go b/internal/lint/lint_test.go index 826f396..f8b0b57 100644 --- a/internal/lint/lint_test.go +++ b/internal/lint/lint_test.go @@ -581,7 +581,7 @@ scenarios: // A prose role is the only label on the rendered relationship line (ADR-0008), // so it collides with its neighbours in the diagram. The linter steers the // prose to `note` — as a warning, never an error. -func TestProseRoleIsWarning(t *testing.T) { +func TestADR_0008_ProseRoleIsWarning(t *testing.T) { t.Parallel() cases := []struct { name string @@ -590,9 +590,12 @@ func TestProseRoleIsWarning(t *testing.T) { }{ {name: "role name", role: "`Owner`", want: false}, {name: "two role names", role: "`Owner` or `Member`", want: false}, + {name: "a comma-separated pair of role names is a list, not prose", role: "`Owner`, `Member`", want: false}, + {name: "a field accessor is not a sentence", role: "`Project`.owner", want: false}, + {name: "a version number is not a sentence", role: "v1.0 owner", want: false}, {name: "four words", role: "primary contact for escalation", want: false}, {name: "five words", role: "the record this one supersedes", want: true}, - {name: "comma", role: "owner, or member", want: true}, + {name: "one word too long for a label", role: "pneumonoultramicroscopicsilicovolcanoconiosisadministrator", want: true}, {name: "full stop", role: "The owning project.", want: true}, {name: "semicolon", role: "owner; also billing", want: true}, {name: "empty", role: "", want: false}, @@ -642,6 +645,149 @@ entities: } } +// TestProseRoleRaisesOneFinding pins that a role produces at most one finding. +// A prose role that also buries an undefined backticked term used to raise both +// warnings at the same path; rewriting the role is the fix that comes first and +// will likely change the term with it. +func TestProseRoleRaisesOneFinding(t *testing.T) { + t.Parallel() + src := ` +kind: DomainModel +version: v1 +entities: + Project: + definition: A container. + relationships: + - entity: User + cardinality: "n:n" + role: "the ` + "`Widget`" + ` this one supersedes" + User: + definition: A principal. +` + res, err := Run([]byte(src)) + if err != nil { + t.Fatal(err) + } + var atRole []Finding + for _, f := range res.Findings { + if f.Path == "/entities/Project/relationships/0/role" { + atRole = append(atRole, f) + } + } + if len(atRole) != 1 { + t.Fatalf("expected exactly one finding on the role, got %d: %+v", len(atRole), atRole) + } + if !strings.Contains(atRole[0].Message, "reads as prose") { + t.Errorf("expected the prose finding to be the one kept, got %q", atRole[0].Message) + } +} + +// TestADR_0008_ReciprocalOwnershipConflictIsError pins the diagnostics half of +// ADR-0008's fold rule. The renderer folds two declarations into one line only +// when they are one relationship seen from two sides; the cases it cannot +// reconcile draw two contradictory lines, so the linter names them instead of +// letting either the diagram or the fold swallow the disagreement. +func TestADR_0008_ReciprocalOwnershipConflictIsError(t *testing.T) { + t.Parallel() + cases := []struct { + name string + aOwn, bOwn string + aRole string + bRole string + want string // "" means no ownership finding + }{ + { + name: "mutual owned is a contradiction", + aOwn: "owned", bOwn: "owned", + want: "mutual ownership", + }, + { + name: "disagreement the fold cannot resolve, because the roles differ", + aOwn: "owned", bOwn: "referenced", + aRole: "part", bRole: "whole", + want: "reciprocal ownership conflict", + }, + { + name: "disagreement the fold resolves into one solid line", + aOwn: "owned", bOwn: "referenced", + aRole: "`Part`", bRole: "`Part`", + want: "", + }, + { + name: "an omitted ownership on the other end is the same as referenced", + aOwn: "owned", bOwn: "", + want: "", + }, + { + name: "backticks are markup, so the roles still match", + aOwn: "owned", bOwn: "referenced", + aRole: "`Part`", bRole: "Part", + want: "", + }, + { + name: "neither end claims ownership", + aOwn: "", bOwn: "", + aRole: "part", bRole: "whole", + want: "", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + field := func(name, val string) string { + if val == "" { + return "" + } + return "\n " + name + ": " + strconv.Quote(val) + } + src := ` +kind: DomainModel +version: v1 +glossary: + Part: "A component." +entities: + Alpha: + definition: A container. + relationships: + - entity: Beta + cardinality: "1:n"` + field("ownership", tc.aOwn) + field("role", tc.aRole) + ` + Beta: + definition: A component. + relationships: + - entity: Alpha + cardinality: "n:1"` + field("ownership", tc.bOwn) + field("role", tc.bRole) + ` +` + res, err := Run([]byte(src)) + if err != nil { + t.Fatal(err) + } + for _, msg := range []string{"mutual ownership", "reciprocal ownership conflict"} { + got := findingWithMessage(res.Findings, msg) + if got != (msg == tc.want) { + t.Fatalf("%q finding = %v, want %v; findings: %+v", msg, got, msg == tc.want, res.Findings) + } + } + if tc.want == "" { + return + } + for _, f := range res.Findings { + if !strings.Contains(f.Message, tc.want) { + continue + } + if f.Severity != SeverityError || f.Category != CategorySemantic { + t.Errorf("expected a semantic error, got %s/%s", f.Severity, f.Category) + } + if want := "/entities/Alpha/relationships/0/ownership"; f.Path != want { + t.Errorf("path = %q, want %q", f.Path, want) + } + } + if !res.HasBlocking(false) { + t.Error("an ownership contradiction must block") + } + }) + } +} + func TestActionPreservesUnknownInvariantIsError(t *testing.T) { src := ` kind: DomainModel diff --git a/internal/render/markdown/markdown_test.go b/internal/render/markdown/markdown_test.go index d7f2364..84dfdd5 100644 --- a/internal/render/markdown/markdown_test.go +++ b/internal/render/markdown/markdown_test.go @@ -104,6 +104,26 @@ func TestRenderEntity_DerivedMarker(t *testing.T) { } } +// TestRenderEntity_SymmetricRelationship pins where a `symmetric` marker +// surfaces. The ER diagram has no notation for it (ADR-0008 leaves the line to +// ownership and the label to the role), so the Markdown relationship line is +// the only place a reader sees it. +func TestRenderEntity_SymmetricRelationship(t *testing.T) { + m := &model.Model{Entities: map[string]model.Entity{ + "Node": {Definition: "A node.", Relationships: []model.Relationship{ + {Entity: "Node", Cardinality: "n:n", Symmetric: true, Role: "`Peer`"}, + {Entity: "Node", Cardinality: "1:0..1", Role: "`Predecessor`"}, + }}, + }} + got := Render(m) + if want := "- `Node` — n:n — symmetric — `Peer`\n"; !strings.Contains(got, want) { + t.Fatalf("expected a symmetric marker %q, got:\n%s", want, got) + } + if want := "- `Node` — 1:0..1 — `Predecessor`\n"; !strings.Contains(got, want) { + t.Fatalf("expected no marker on a non-symmetric relationship %q, got:\n%s", want, got) + } +} + // TestGoldenExample renders the committed example and compares it to the // checked-in Markdown. This is the same invariant `modelith render --check` enforces // in CI: if you change the renderer or the example YAML, regenerate the .md. diff --git a/internal/render/mermaid/mermaid.go b/internal/render/mermaid/mermaid.go index 49434ca..43a3e6b 100644 --- a/internal/render/mermaid/mermaid.go +++ b/internal/render/mermaid/mermaid.go @@ -83,6 +83,29 @@ type edge struct { owned bool } +// folds reports whether a declaration made by `from`, with the given ownership, +// is the same drawn line as e rather than a second one. It is only ever asked +// about declarations that already agree on the pair, the canonical cardinality +// (normalized to the sorted-pair orientation, so inverses match) and the label. +// Two such declarations merge in exactly two cases (ADR-0008): +// +// - the same end declared it twice with the same ownership — an exact +// duplicate, which would draw two indistinguishable lines; +// - opposite ends declared it and at most one of them claims `owned` — one +// relationship seen from two sides, whose ownership belongs to the +// relationship rather than to the end that named it. +// +// Everything else stays a distinct edge, so no declaration disappears from the +// diagram: two declarations from the same end differing only in `ownership` are +// two relationships, and mutual `owned` is a contradiction. `modelith lint` +// reports the contradictions. +func (e *edge) folds(from string, owned bool) bool { + if e.from == from { + return e.owned == owned + } + return !e.owned || !owned +} + // ER renders the model as a Mermaid erDiagram. Ordinary attributes are // intentionally omitted: their freeform conceptual types (e.g. // "enum[active, archived]") aren't valid erDiagram attribute types, so they are @@ -107,22 +130,23 @@ func ER(m *model.Model) string { } var edges []*edge - byKey := map[string]*edge{} + byKey := map[string][]*edge{} for _, name := range m.EntityNames() { for _, rel := range m.Entities[name].Relationships { if rel.Entity == name { continue // already rendered inside the entity's block } label := relationshipLabel(rel) + owned := rel.Ownership == "owned" - // Dedupe edges declared from both sides of the same pair. The key - // includes the cardinality normalized to the sorted-pair - // orientation, so a relationship declared from both sides with - // inverse cardinalities (A "1:n" B, B "n:1" A) collapses to one - // edge — while genuinely distinct edges (GO-3) or contradictory - // reciprocal declarations (GO-1) keep distinct keys and both - // render, surfacing the conflict instead of silently dropping one. - // `modelith lint` reports the contradiction as an error. + // Group candidate declarations of one drawn line. The key includes + // the cardinality normalized to the sorted-pair orientation, so a + // relationship declared from both sides with inverse cardinalities + // (A "1:n" B, B "n:1" A) is a candidate to fold — while genuinely + // distinct edges (GO-3) or contradictory reciprocal declarations + // (GO-1) get distinct keys and both render, surfacing the conflict + // instead of silently dropping one. Matching the key is necessary + // but not sufficient: edge.folds decides. pair := []string{name, rel.Entity} sort.Strings(pair) card := rel.Cardinality @@ -134,16 +158,25 @@ func ER(m *model.Model) string { // ("1:n" and "0..n:1") dedupes to one edge. card = model.CanonicalCardinality(card) key := pair[0] + "\x00" + pair[1] + "\x00" + card + "\x00" + label - if e, ok := byKey[key]; ok { + + folded := false + for _, e := range byKey[key] { + if !e.folds(name, owned) { + continue + } // Ownership belongs to the relationship, not to the end that // declared it: a parent declaring `owned` and the child // declaring `referenced` are one identifying relationship seen // from two sides, so the folded edge stays solid (ADR-0008). - e.owned = e.owned || rel.Ownership == "owned" + e.owned = e.owned || owned + folded = true + break + } + if folded { continue } - e := &edge{from: name, to: rel.Entity, card: rel.Cardinality, label: label, owned: rel.Ownership == "owned"} - byKey[key] = e + e := &edge{from: name, to: rel.Entity, card: rel.Cardinality, label: label, owned: owned} + byKey[key] = append(byKey[key], e) edges = append(edges, e) } } @@ -159,31 +192,39 @@ func ER(m *model.Model) string { // edge from an entity to itself draws a runaway arc that swamps the canvas // (issue #26); the row carries the same information without a line (ADR-0008). // Row names are self, self2, self3… — distinct, since Mermaid does not -// disambiguate two attributes sharing a name. +// disambiguate two attributes sharing a name. Declarations that would render +// the same row are emitted once: two indistinguishable rows carry no more +// information than one. func selfRows(name string, rels []model.Relationship) []string { var rows []string + seen := map[string]bool{} for _, rel := range rels { if rel.Entity != name { continue } + comment := selfComment(rel) + if seen[comment] { + continue + } + seen[comment] = true attr := "self" if n := len(rows) + 1; n > 1 { attr = "self" + strconv.Itoa(n) } - rows = append(rows, fmt.Sprintf("%s %s %q", name, attr, selfComment(rel))) + rows = append(rows, fmt.Sprintf("%s %s %q", name, attr, comment)) } return rows } -// selfComment is the comment column of a self-relationship row: the target-side +// selfComment is the comment column of a self-relationship row: the declared // cardinality, the ownership when owned (the line style that carries it // elsewhere has no line here), then the role. +// +// Both sides of the cardinality are shown. The row stands in for an edge whose +// two end markers encoded both ends, so showing only the target side would drop +// the declaring side — the one piece of information the row exists to preserve. func selfComment(rel model.Relationship) string { - target := rel.Cardinality - if _, right, ok := strings.Cut(rel.Cardinality, ":"); ok { - target = right - } - out := target + out := rel.Cardinality if rel.Ownership == "owned" { out += " owned" } @@ -202,14 +243,19 @@ func relationshipLabel(rel model.Relationship) string { // sanitize strips or replaces characters that would break a quoted Mermaid // label. Square brackets are replaced with parentheses because Mermaid uses -// them for node/attribute syntax; backticks and quotes are neutralized and -// newlines collapsed. Entity names interpolated elsewhere are constrained to -// PascalCase by the schema, so they need no escaping. +// them for node/attribute syntax; backticks, quotes and backslashes are +// neutralized and newlines collapsed. Dropping the backslash also keeps the %q +// the callers emit with from doubling it into a visible "\\". Entity names +// interpolated elsewhere are constrained to PascalCase by the schema, so they +// need no escaping. func sanitize(s string) string { s = strings.ReplaceAll(s, "`", "") s = strings.ReplaceAll(s, "\"", "'") + s = strings.ReplaceAll(s, "\\", "") s = strings.ReplaceAll(s, "[", "(") s = strings.ReplaceAll(s, "]", ")") s = strings.ReplaceAll(s, "\n", " ") + s = strings.ReplaceAll(s, "\r", " ") + s = strings.ReplaceAll(s, "\t", " ") return strings.TrimSpace(s) } diff --git a/internal/render/mermaid/mermaid_test.go b/internal/render/mermaid/mermaid_test.go index 6c3744d..491ee69 100644 --- a/internal/render/mermaid/mermaid_test.go +++ b/internal/render/mermaid/mermaid_test.go @@ -149,6 +149,7 @@ func TestADR_0008_OwnershipIsLineStyle(t *testing.T) { // it, so a parent's `owned` and the child's `referenced` fold into one solid // edge rather than a contradictory solid-plus-dashed pair. func TestADR_0008_OwnershipFoldsAcrossDeclarations(t *testing.T) { + t.Parallel() m := &model.Model{Entities: map[string]model.Entity{ "Parent": {Definition: "p", Relationships: []model.Relationship{{Entity: "Child", Cardinality: "1:n", Ownership: "owned"}}}, "Child": {Definition: "c", Relationships: []model.Relationship{{Entity: "Parent", Cardinality: "n:1", Ownership: "referenced"}}}, @@ -162,6 +163,85 @@ func TestADR_0008_OwnershipFoldsAcrossDeclarations(t *testing.T) { } } +// TestADR_0008_FoldsOnlyGenuineReciprocals pins the fold predicate of ADR-0008. +// Two declarations become one line only when they are one relationship seen +// from two sides — opposite ends, inverse cardinality, the same label, and at +// most one end claiming `owned` — or when one is an exact duplicate of the +// other. Everything else draws both lines, so no declaration vanishes. +func TestADR_0008_FoldsOnlyGenuineReciprocals(t *testing.T) { + t.Parallel() + cases := []struct { + name string + a, b model.Relationship // a declared by A; b declared by B unless bFromA + bFrom string // entity that declares b: "A" or "B" + want []string + }{ + { + name: "opposite ends, one owned: one solid line", + a: model.Relationship{Entity: "B", Cardinality: "1:n", Ownership: "owned"}, + b: model.Relationship{Entity: "A", Cardinality: "n:1", Ownership: "referenced"}, + bFrom: "B", + want: []string{" A ||--o{ B : \"\"\n"}, + }, + { + name: "opposite ends, neither owned: one dashed line", + a: model.Relationship{Entity: "B", Cardinality: "1:n"}, + b: model.Relationship{Entity: "A", Cardinality: "n:1"}, + bFrom: "B", + want: []string{" A ||..o{ B : \"\"\n"}, + }, + { + name: "opposite ends, both owned: a contradiction, so both lines draw", + a: model.Relationship{Entity: "B", Cardinality: "1:n", Ownership: "owned"}, + b: model.Relationship{Entity: "A", Cardinality: "n:1", Ownership: "owned"}, + bFrom: "B", + want: []string{" A ||--o{ B : \"\"\n", " B }o--|| A : \"\"\n"}, + }, + { + name: "same end, ownership differs: two relationships, so both lines draw", + a: model.Relationship{Entity: "B", Cardinality: "1:n", Ownership: "owned"}, + b: model.Relationship{Entity: "B", Cardinality: "1:n", Ownership: "referenced"}, + bFrom: "A", + want: []string{" A ||--o{ B : \"\"\n", " A ||..o{ B : \"\"\n"}, + }, + { + name: "same end, identical: an exact duplicate draws once", + a: model.Relationship{Entity: "B", Cardinality: "1:n", Ownership: "owned"}, + b: model.Relationship{Entity: "B", Cardinality: "1:n", Ownership: "owned"}, + bFrom: "A", + want: []string{" A ||--o{ B : \"\"\n"}, + }, + { + name: "opposite ends, roles differ: two labels, so both lines draw", + a: model.Relationship{Entity: "B", Cardinality: "1:n", Ownership: "owned", Role: "part"}, + b: model.Relationship{Entity: "A", Cardinality: "n:1", Role: "whole"}, + bFrom: "B", + want: []string{" A ||--o{ B : \"part\"\n", " B }o..|| A : \"whole\"\n"}, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + a := model.Entity{Definition: "a", Relationships: []model.Relationship{tc.a}} + b := model.Entity{Definition: "b"} + if tc.bFrom == "A" { + a.Relationships = append(a.Relationships, tc.b) + } else { + b.Relationships = []model.Relationship{tc.b} + } + out := ER(&model.Model{Entities: map[string]model.Entity{"A": a, "B": b}}) + if n := strings.Count(out, " : "); n != len(tc.want) { + t.Errorf("expected %d edge(s), got %d:\n%s", len(tc.want), n, out) + } + for _, want := range tc.want { + if !strings.Contains(out, want) { + t.Errorf("expected edge %q; got:\n%s", want, out) + } + } + }) + } +} + // TestADR_0008_SelfRelationshipRendersInEntityBlock pins the self-relationship // half of ADR-0008: Mermaid's ER layout has no self-loop handling (issue #26), // so the relationship becomes a row in the entity's own block and no edge. @@ -174,7 +254,7 @@ func TestADR_0008_SelfRelationshipRendersInEntityBlock(t *testing.T) { "Note": {Definition: "n"}, }} out := ER(m) - want := " Record {\n Record self \"0..1 — Predecessor\"\n }\n" + want := " Record {\n Record self \"1:0..1 — Predecessor\"\n }\n" if !strings.Contains(out, want) { t.Errorf("expected self-relationship row %q; got:\n%s", want, out) } @@ -186,7 +266,13 @@ func TestADR_0008_SelfRelationshipRendersInEntityBlock(t *testing.T) { } } -func TestERSelfRelationshipRowContents(t *testing.T) { +// TestADR_0008_SelfRowCarriesBothCardinalitySides pins the no-information-loss +// half of ADR-0008's in-box rows: the row replaces an edge whose two end +// markers encoded both sides of the cardinality, so the row shows both. The +// declaring side is exactly what a target-side-only row would drop — "1:n" and +// "0..5:1" would both collapse to their right-hand side alone. +func TestADR_0008_SelfRowCarriesBothCardinalitySides(t *testing.T) { + t.Parallel() cases := []struct { name string rel model.Relationship @@ -195,22 +281,32 @@ func TestERSelfRelationshipRowContents(t *testing.T) { { name: "cardinality only", rel: model.Relationship{Entity: "Record", Cardinality: "1:n"}, - want: `Record self "n"`, + want: `Record self "1:n"`, + }, + { + name: "a bounded declaring side survives", + rel: model.Relationship{Entity: "Record", Cardinality: "0..5:1"}, + want: `Record self "0..5:1"`, }, { name: "owned is spelled out, since there is no line to carry it", rel: model.Relationship{Entity: "Record", Cardinality: "1:n", Ownership: "owned", Role: "`Part`"}, - want: `Record self "n owned — Part"`, + want: `Record self "1:n owned — Part"`, }, { name: "referenced stays implicit, matching the dashed default", rel: model.Relationship{Entity: "Record", Cardinality: "1:0..1", Ownership: "referenced", Role: "Predecessor"}, - want: `Record self "0..1 — Predecessor"`, + want: `Record self "1:0..1 — Predecessor"`, }, { name: "a quote in the role cannot break out of the comment", rel: model.Relationship{Entity: "Record", Cardinality: "n:n", Role: `a "quoted" [role]`}, - want: `Record self "n — a 'quoted' (role)"`, + want: `Record self "n:n — a 'quoted' (role)"`, + }, + { + name: "a backslash in the role is dropped, not doubled by %q", + rel: model.Relationship{Entity: "Record", Cardinality: "n:n", Role: `a\b`}, + want: `Record self "n:n — ab"`, }, { name: "a cardinality with no colon is shown whole", @@ -220,6 +316,7 @@ func TestERSelfRelationshipRowContents(t *testing.T) { } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { + t.Parallel() m := &model.Model{Entities: map[string]model.Entity{ "Record": {Definition: "r", Relationships: []model.Relationship{tc.rel}}, }} @@ -233,23 +330,46 @@ func TestERSelfRelationshipRowContents(t *testing.T) { // TestERMultipleSelfRelationships guards the row names: Mermaid does not // disambiguate two attributes sharing a name, so each row gets its own. func TestERMultipleSelfRelationships(t *testing.T) { + t.Parallel() m := &model.Model{Entities: map[string]model.Entity{ "Record": {Definition: "r", Relationships: []model.Relationship{ {Entity: "Record", Cardinality: "1:0..1", Role: "`Predecessor`"}, {Entity: "Record", Cardinality: "1:n", Ownership: "owned", Role: "`Part`"}, - {Entity: "Record", Cardinality: "n:n", Symmetric: true, Role: "`Peer`"}, + {Entity: "Record", Cardinality: "n:n", Role: "`Peer`"}, }}, }} want := " Record {\n" + - " Record self \"0..1 — Predecessor\"\n" + - " Record self2 \"n owned — Part\"\n" + - " Record self3 \"n — Peer\"\n" + + " Record self \"1:0..1 — Predecessor\"\n" + + " Record self2 \"1:n owned — Part\"\n" + + " Record self3 \"n:n — Peer\"\n" + " }\n" if out := ER(m); !strings.Contains(out, want) { t.Errorf("expected distinct self rows:\n%s\ngot:\n%s", want, out) } } +// TestERSelfRelationshipsDedupe guards the row list against a declaration +// repeated verbatim: two rows that would read identically carry no more than +// one, and the numbering stays contiguous. Rows that differ in any rendered +// part are kept, since the reader can tell them apart. +func TestERSelfRelationshipsDedupe(t *testing.T) { + t.Parallel() + m := &model.Model{Entities: map[string]model.Entity{ + "Record": {Definition: "r", Relationships: []model.Relationship{ + {Entity: "Record", Cardinality: "0..1:0..1", Role: "`Predecessor`"}, + {Entity: "Record", Cardinality: "0..1:0..1", Role: "`Predecessor`"}, + {Entity: "Record", Cardinality: "0..1:0..1", Ownership: "owned", Role: "`Predecessor`"}, + }}, + }} + want := " Record {\n" + + " Record self \"0..1:0..1 — Predecessor\"\n" + + " Record self2 \"0..1:0..1 owned — Predecessor\"\n" + + " }\n" + if out := ER(m); !strings.Contains(out, want) { + t.Errorf("expected deduped self rows:\n%s\ngot:\n%s", want, out) + } +} + // TestADR_0003_BoundedCardinalityRendersNearestGlyph pins the render half of // ADR-0003 and the capture-first principle in ADR-0002: Mermaid has no numeric // bound, so exact and bounded counts render as the nearest crow's-foot glyph diff --git a/plugin/skills/domain-model-author/SKILL.md b/plugin/skills/domain-model-author/SKILL.md index b1f160f..5270487 100644 --- a/plugin/skills/domain-model-author/SKILL.md +++ b/plugin/skills/domain-model-author/SKILL.md @@ -113,7 +113,9 @@ Follow the format exactly (see the schema reference). Key conventions: - `cardinality` is one of `1:1`, `1:n`, `n:1`, `n:n`. If you declare the same relationship from both entities, the two cardinalities must be inverses (`1:n` ⇄ `n:1`; `1:1`/`n:n` symmetric) — a contradiction is a lint **error**. - Declaring it from one side only is fine and usually clearer. + So is a pair where both ends claim `ownership: owned`, or where the ends + disagree on ownership under different roles. Declaring it from one side only + is fine and usually clearer. - **`role` is a short role name** (`` `Owner` ``, `` `Predecessor` ``), not a sentence: it is the only label drawn on the diagram line, so prose there collides with neighbouring lines and warns. Put the explanation in `note`. diff --git a/plugin/skills/domain-model-lint/SKILL.md b/plugin/skills/domain-model-lint/SKILL.md index 5b030ff..9794d92 100644 --- a/plugin/skills/domain-model-lint/SKILL.md +++ b/plugin/skills/domain-model-lint/SKILL.md @@ -45,7 +45,8 @@ the flag. `invariants_touched` or an action's `preserves` names an invariant id no entity or top-level `invariants` entry declares; the same invariant id is declared twice (entity-level and top-level invariants share one id namespace); reciprocal cardinalities - that aren't inverses. Warnings: a backticked term resolves to no entity, role, + that aren't inverses; reciprocal declarations that both claim `ownership: owned`, + or that disagree on ownership under different roles. Warnings: a backticked term resolves to no entity, role, or actor; an action `actor` that's neither an entity nor a glossary term; a PascalCase attribute `type` that names no defined enum — usually a typo or a concept that was never named; a relationship `role` written as prose, which diff --git a/project-docs/adr/0008-er-diagram-conventions.md b/project-docs/adr/0008-er-diagram-conventions.md index ff47331..6bc4c5c 100644 --- a/project-docs/adr/0008-er-diagram-conventions.md +++ b/project-docs/adr/0008-er-diagram-conventions.md @@ -14,20 +14,35 @@ fakes structure the ER cannot honestly show. 1. **`ownership` is the line style.** `owned` draws the identifying (solid) connector `--`; `referenced` and an omitted `ownership` draw the non-identifying (dashed) `..`. That is standard ER semantics for composition - and costs no label space. Ownership is a property of the relationship, not of - the end that declared it, so when a parent's `owned` and a child's - `referenced` fold into one edge, the folded edge stays solid. -2. **`role` is the only label.** The `ownership` and `cardinality` fallbacks are + and costs no label space. +2. **Two declarations become one line only when they are one relationship.** The + renderer merges a pair of declarations in exactly two cases: an exact + duplicate from the same end, and a genuine reciprocal — the same pair, + inverse cardinality, the same label, declared from *opposite* ends, with at + most one end claiming `owned`. A genuine reciprocal draws solid if either end + said `owned`, because ownership belongs to the relationship rather than to + the end that named it. Every other case draws both lines, so no declaration + disappears from the diagram: two declarations from the same end that differ + only in `ownership` are two relationships, and mutual `owned` is a + contradiction, not a fold. + + The contradictions the renderer refuses to swallow are lint **errors**: + mutual `owned` from both ends, and a reciprocal pair that disagrees on + ownership without folding (their roles differ, so the diagram would draw one + solid and one dashed line for the same relationship). +3. **`role` is the only label.** The `ownership` and `cardinality` fallbacks are gone; a relationship with no role gets an empty label. Precise counts already live in the Markdown table per ADR-0002, and spending label space on them crowds out the roles. A semantic lint *warning* (never an error) fires when a - role reads as prose — more than four words, or sentence punctuation — and - names `note` as the field for the explanation. -3. **A self-referential relationship is a row inside the entity's block**, not - an edge: `Project self "0..1 — Predecessor"`. The row carries the target-side - cardinality, the word `owned` when owned (there is no line to carry it), and - the role. Rows are named `self`, `self2`, … because Mermaid does not - disambiguate two attributes sharing a name. + role reads as prose — too long for a label, more than four words, or a + sentence terminator — and names `note` as the field for the explanation. +4. **A self-referential relationship is a row inside the entity's block**, not + an edge: `Project self "1:0..1 — Predecessor"`. The row carries the declared + cardinality (both sides — the edge it replaces encoded both in its two end + markers), the word `owned` when owned (there is no line to carry it), and the + role. Rows are named `self`, `self2`, … because Mermaid does not + disambiguate two attributes sharing a name; declarations that would render an + identical row are emitted once. ## Evidence @@ -41,11 +56,15 @@ for both renderers. ## Consequences -Point 3 is the one place a *relationship* is drawn inside a box, so the +Point 4 is the one place a *relationship* is drawn inside a box, so the "attributes are intentionally omitted from the diagram" rule from ADR-0002 now reads: ordinary attributes are omitted, self-relationships are not. Ownership becomes visible at a glance across the whole diagram, which makes an incorrect `ownership` easier to spot than the old word label did. -Pinned by `TestADR_0008_*` in `internal/render/mermaid` and `TestProseRoleIsWarning` -in `internal/lint`. +Point 2 trades a tidier diagram for an honest one. A model whose two ends +disagree gets two lines and an error rather than a single line that quietly +picks a winner — the disagreement is a modeling question only the author can +settle. + +Pinned by `TestADR_0008_*` in `internal/render/mermaid` and in `internal/lint`. From 8b4b8aecc9edae9b80a82f2464c6857a94862912 Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Sat, 25 Jul 2026 19:44:38 -0700 Subject: [PATCH 3/4] fix(render): fold a reciprocal composition whatever roles its ends name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 review fix. Round 1 put label equality into the fold predicate and added a "reciprocal ownership disagreement" error to cover the contradiction that produced. Both were wrong: a parent owning a child while the child references the parent back, each naming its own end's role, is the textbook composition pattern and precisely the genuine reciprocal the fold exists for. It was drawing one solid and one dashed line and failing lint with advice ("give both ends the same role") that argues against the pattern itself. The predicate is now what was settled: same pair, inverse cardinality, opposite ends, at most one end claiming `owned`. Labels are out of it. A fold picks the label deterministically — the owning end's role, or with neither end owning, the role from the end whose entity sorts first. The other role is dropped from the diagram only; the Markdown lists each entity's own relationships, so both survive in the document (ADR-0002's lossy view). The reciprocal-ownership-disagreement error is deleted. With labels out of the predicate no unresolvable case remains: exactly one `owned` folds, both `owned` is the mutual-ownership error, neither `owned` folds to a dashed edge. Labels still separate two declarations from the same end, so a User who is both `Owner` and `Member` of a Project keeps two lines. An edge folds in at most one opposite-end declaration, mirroring the linter, which reconciles a pair only when each direction declares it exactly once. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Joe Beda --- docs/04-reading-the-diagrams.md | 25 +++--- docs/06-schema-reference.md | 10 +-- internal/lint/lint.go | 38 ++------ internal/lint/lint_test.go | 87 +++++++++++++----- internal/render/mermaid/mermaid.go | 90 ++++++++++++------- internal/render/mermaid/mermaid_test.go | 78 ++++++++++++++-- plugin/skills/domain-model-author/SKILL.md | 7 +- plugin/skills/domain-model-lint/SKILL.md | 4 +- .../adr/0008-er-diagram-conventions.md | 42 +++++---- 9 files changed, 250 insertions(+), 131 deletions(-) diff --git a/docs/04-reading-the-diagrams.md b/docs/04-reading-the-diagrams.md index f73eca4..d93d93a 100644 --- a/docs/04-reading-the-diagrams.md +++ b/docs/04-reading-the-diagrams.md @@ -127,20 +127,21 @@ the `Users` on it — dashed; deleting the `Project` doesn't delete the `Users`. Ownership belongs to the relationship, not to the end that declared it. If a `Project` says it owns its `Policies` and the `Policy` says it references its `Project`, that's one identifying relationship seen from both ends, so it draws -as a single solid line — provided the two declarations otherwise match (inverse -cardinalities and the same role). That is the only merge the renderer makes on -ownership. Two declarations it can't reconcile stay two lines, so nothing you -wrote vanishes from the picture: +as a single solid line — provided the two cardinalities are inverses. The two +ends may name *different* roles, and usually do: the `Project` end calls it +`Policies`, the `Policy` end calls it `Project`. The line is labelled with the +owning end's role (with neither end owning, the role from the entity that sorts +first). The other role isn't lost — it's in that entity's relationship list in +the Markdown below the diagram. + +Anything the renderer can't reduce to one relationship stays two lines, so +nothing you wrote vanishes from the picture: - If both ends claim `owned`, that's a contradiction — a relationship is owned by at most one end. Both lines draw, and `modelith lint` reports it as an error. -- If the two ends disagree on ownership but give the relationship different - roles, the renderer can't tell whether you meant one relationship or two. - Both lines draw, and `modelith lint` reports that as an error too. Give both - ends the same role, or declare the relationship from one end only. -- If one entity declares two relationships to the same entity that differ only - in `ownership`, those are two relationships, and both draw. +- If one entity declares two relationships to the same entity that differ in + `ownership` or in role, those are two relationships, and both draw. ## The labels on the lines @@ -222,8 +223,8 @@ Reading it: die with it. The example declares this relationship from *both* entities (`Project` owns `Policy`; `Policy` references its `Project`) — one relationship seen from two ends, so it draws once. The two declarations must - agree — inverse cardinalities, the same role, and at most one end claiming - `owned` — or `modelith lint` flags the contradiction and both lines draw. + have inverse cardinalities and at most one end claiming `owned`, or + `modelith lint` flags the contradiction and both lines draw. - **`Project }o..o{ User : "Owner or Member"`** — many-to-many between `Projects` and `Users` on a **dashed** line: a `User`'s role is `Owner` or `Member`, and neither entity is part of the other. diff --git a/docs/06-schema-reference.md b/docs/06-schema-reference.md index 0b8448c..397d246 100644 --- a/docs/06-schema-reference.md +++ b/docs/06-schema-reference.md @@ -162,9 +162,10 @@ and covered in full in [Reading the Diagrams](./04-reading-the-diagrams.md): (non-identifying) for `referenced` and for an omitted `ownership`. It costs no label space. Ownership belongs to the relationship rather than the end that declared it, so a parent's `owned` and the child's `referenced` fold into one - solid line — when the two declarations otherwise match (inverse cardinalities, - the same role). Declarations the renderer can't reconcile draw as separate - lines, and mutual `owned` or an unreconcilable disagreement is a lint error. + solid line when their cardinalities are inverses — even when the two ends name + different roles, in which case the owning end's role labels the line and the + other stays in the Markdown. Declarations the renderer can't reduce to one + relationship draw as separate lines, and mutual `owned` is a lint error. - **`role` is the only label** — `ownership` and `cardinality` are never written on a line. Keep the role short; put the explanation in `note`. - **A self-referential relationship becomes a row inside the entity's box** @@ -294,9 +295,6 @@ The JSON Schema covers structure. [`modelith lint`](./07-cli.md) adds: inverses (e.g. `Project`→`Policy` `1:n` but `Policy`→`Project` `1:1`); - a relationship declared from both sides where both ends claim `ownership: owned` — a relationship is owned by at most one end; - - a relationship declared from both sides whose ends disagree on - `ownership` and give it different roles, so the diagram would draw one - solid and one dashed line for the same relationship; - a duplicate invariant `id` (across entity-level *and* model-level invariants — they share one namespace); - a scenario `invariants_touched` or an action `preserves` that references an diff --git a/internal/lint/lint.go b/internal/lint/lint.go index 0c3a011..ba8778e 100644 --- a/internal/lint/lint.go +++ b/internal/lint/lint.go @@ -601,11 +601,10 @@ func inheritsInvariants(m *model.Model, name string) bool { // the renderer would otherwise draw two conflicting edges. // // Ownership belongs to the relationship, not to the end that declared it, so at -// most one end may claim `owned`; both claiming it is a contradiction. And when -// the two ends disagree on ownership, the renderer only reconciles them into -// one line if they are otherwise the same drawn edge (ADR-0008). When they are -// not, the diagram shows one solid and one dashed line for what the model calls -// a single relationship, so that is an error too. +// most one end may claim `owned`; both claiming it is a contradiction. One end +// claiming it is the ordinary composition pattern — the renderer folds the two +// declarations into a single identifying line (ADR-0008), whatever roles the +// two ends give it. // // Only pairs with exactly one declaration in each direction are checked. // Multiple edges between the same pair (a legitimate pattern — e.g. a User is @@ -615,7 +614,6 @@ func runReciprocity(m *model.Model, res *Result) { type decl struct { from, to string card string - role string owned bool path string ownerPath string @@ -630,7 +628,6 @@ func runReciprocity(m *model.Model, res *Result) { from: name, to: rel.Entity, card: rel.Cardinality, - role: rel.Role, owned: rel.Ownership == "owned", path: fmt.Sprintf("/entities/%s/relationships/%d/cardinality", name, i), ownerPath: fmt.Sprintf("/entities/%s/relationships/%d/ownership", name, i), @@ -690,35 +687,12 @@ func runReciprocity(m *model.Model, res *Result) { f.from, f.to, f.card, r.from, r.to, r.card, model.InvertCardinality(f.card), ), }) - continue // the conflict is the thing to fix; don't pile on - } - - // The cardinalities are inverses, so the two ends describe one line. If - // they also disagree on ownership, the renderer folds them into a - // single solid line only when they carry the same role; otherwise it - // draws one solid and one dashed line for the same relationship - // (ADR-0008). Say so rather than let the diagram contradict itself. - if f.owned != r.owned && normalizeRole(f.role) != normalizeRole(r.role) { - owner, other := f, r - if r.owned { - owner, other = r, f - } - res.Findings = append(res.Findings, Finding{ - Severity: SeverityError, - Category: CategorySemantic, - Path: owner.ownerPath, - Message: fmt.Sprintf( - "reciprocal ownership conflict: %s→%s declares ownership \"owned\" but %s→%s does not, and their roles differ (%q vs %q), so the diagram draws one solid and one dashed line for the same relationship — give both ends the same role, or declare the relationship from one end only", - owner.from, owner.to, other.from, other.to, owner.role, other.role, - ), - }) } } } -// normalizeRole reduces a role to the text the diagram labels a line with, so -// two declarations of one relationship are compared the way the renderer -// compares them: backticks are markup, and surrounding space is not content. +// normalizeRole reduces a role to the text the diagram labels a line with: +// backticks are markup, and surrounding space is not content. func normalizeRole(role string) string { return strings.TrimSpace(strings.ReplaceAll(role, "`", "")) } diff --git a/internal/lint/lint_test.go b/internal/lint/lint_test.go index f8b0b57..f46a8eb 100644 --- a/internal/lint/lint_test.go +++ b/internal/lint/lint_test.go @@ -682,12 +682,13 @@ entities: } } -// TestADR_0008_ReciprocalOwnershipConflictIsError pins the diagnostics half of -// ADR-0008's fold rule. The renderer folds two declarations into one line only -// when they are one relationship seen from two sides; the cases it cannot -// reconcile draw two contradictory lines, so the linter names them instead of -// letting either the diagram or the fold swallow the disagreement. -func TestADR_0008_ReciprocalOwnershipConflictIsError(t *testing.T) { +// TestADR_0008_MutualOwnershipIsError pins the one ownership contradiction a +// reciprocal pair can hold: both ends claiming to own the other. A relationship +// is owned by at most one end, so the renderer refuses to fold it and the +// linter names it. Exactly one end owning is the ordinary composition pattern +// and must stay clean whatever roles the two ends give it — see +// TestADR_0008_ReciprocalCompositionLintsClean. +func TestADR_0008_MutualOwnershipIsError(t *testing.T) { t.Parallel() cases := []struct { name string @@ -702,15 +703,15 @@ func TestADR_0008_ReciprocalOwnershipConflictIsError(t *testing.T) { want: "mutual ownership", }, { - name: "disagreement the fold cannot resolve, because the roles differ", - aOwn: "owned", bOwn: "referenced", + name: "mutual owned under differing roles is still a contradiction", + aOwn: "owned", bOwn: "owned", aRole: "part", bRole: "whole", - want: "reciprocal ownership conflict", + want: "mutual ownership", }, { - name: "disagreement the fold resolves into one solid line", + name: "one end owning, the other referencing", aOwn: "owned", bOwn: "referenced", - aRole: "`Part`", bRole: "`Part`", + aRole: "part", bRole: "whole", want: "", }, { @@ -718,12 +719,6 @@ func TestADR_0008_ReciprocalOwnershipConflictIsError(t *testing.T) { aOwn: "owned", bOwn: "", want: "", }, - { - name: "backticks are markup, so the roles still match", - aOwn: "owned", bOwn: "referenced", - aRole: "`Part`", bRole: "Part", - want: "", - }, { name: "neither end claims ownership", aOwn: "", bOwn: "", @@ -761,13 +756,13 @@ entities: if err != nil { t.Fatal(err) } - for _, msg := range []string{"mutual ownership", "reciprocal ownership conflict"} { - got := findingWithMessage(res.Findings, msg) - if got != (msg == tc.want) { - t.Fatalf("%q finding = %v, want %v; findings: %+v", msg, got, msg == tc.want, res.Findings) - } + if got := findingWithMessage(res.Findings, "mutual ownership"); got != (tc.want != "") { + t.Fatalf("mutual-ownership finding = %v, want %v; findings: %+v", got, tc.want != "", res.Findings) } if tc.want == "" { + if res.HasBlocking(false) { + t.Fatalf("expected no error on a legitimate pair, got: %+v", res.Findings) + } return } for _, f := range res.Findings { @@ -788,6 +783,54 @@ entities: } } +// TestADR_0008_ReciprocalCompositionLintsClean is the linter half of the +// regression guard on the textbook composition pattern: a parent owns a child, +// the child references the parent back, and each end names its own role. It is +// one relationship seen from two sides, which the renderer folds into a single +// solid line (TestADR_0008_ReciprocalCompositionFoldsToOneEdge), so it must +// produce no finding at all — not a warning, and certainly not an error that +// would fail a CI gate on a correct model. +func TestADR_0008_ReciprocalCompositionLintsClean(t *testing.T) { + t.Parallel() + src := ` +kind: DomainModel +version: v1 +title: Composition declared from both ends +entities: + Alpha: + definition: A container. + invariants: + - id: alpha-1 + statement: An Alpha always has a name. + relationships: + - entity: Beta + cardinality: "1:n" + ownership: owned + role: part + Beta: + definition: A component. + invariants: + - id: beta-1 + statement: A Beta always belongs to an Alpha. + relationships: + - entity: Alpha + cardinality: "n:1" + ownership: referenced + role: whole +scenarios: + - name: compose + actors: [Alpha] + steps: ["an ` + "`Alpha`" + ` gains a ` + "`Beta`" + `"] +` + res, err := Run([]byte(src)) + if err != nil { + t.Fatal(err) + } + if len(res.Findings) != 0 { + t.Fatalf("expected no findings on a reciprocal composition, got %d: %+v", len(res.Findings), res.Findings) + } +} + func TestActionPreservesUnknownInvariantIsError(t *testing.T) { src := ` kind: DomainModel diff --git a/internal/render/mermaid/mermaid.go b/internal/render/mermaid/mermaid.go index 43a3e6b..ca2fbf2 100644 --- a/internal/render/mermaid/mermaid.go +++ b/internal/render/mermaid/mermaid.go @@ -77,33 +77,63 @@ func rightMarker(m model.Multiplicity) string { // edge is one rendered relationship line, accumulated before emission so that // declarations of the same relationship from both ends fold into it. type edge struct { - from, to string - card string // as declared by `from`, so the markers read left to right - label string - owned bool + from, to string + card string // as declared by `from`, so the markers read left to right + label string + owned bool + foldedFrom string // the opposite end already folded in, if any } -// folds reports whether a declaration made by `from`, with the given ownership, -// is the same drawn line as e rather than a second one. It is only ever asked -// about declarations that already agree on the pair, the canonical cardinality -// (normalized to the sorted-pair orientation, so inverses match) and the label. -// Two such declarations merge in exactly two cases (ADR-0008): +// folds reports whether a declaration made by `from`, with the given ownership +// and label, is the same drawn line as e rather than a second one. It is only +// ever asked about declarations that already agree on the pair and on the +// canonical cardinality (normalized to the sorted-pair orientation, so inverses +// match). Two such declarations merge in exactly two cases (ADR-0008): // -// - the same end declared it twice with the same ownership — an exact -// duplicate, which would draw two indistinguishable lines; // - opposite ends declared it and at most one of them claims `owned` — one // relationship seen from two sides, whose ownership belongs to the -// relationship rather than to the end that named it. +// relationship rather than to the end that named it. The two ends may name +// different roles: that is the ordinary composition pattern (a parent's +// "part" is a child's "whole"), not a conflict, so the label is chosen +// rather than compared (see merge). Only one opposite-end declaration +// folds in, mirroring the linter, which reconciles a pair only when each +// direction declares it exactly once. +// - the same end declared it twice with the same ownership and the same label +// — an exact duplicate, which would draw two indistinguishable lines. The +// label matters here: two declarations from one end with different roles +// (a User who is both `Owner` and `Member` of a Project) are two +// relationships. // // Everything else stays a distinct edge, so no declaration disappears from the // diagram: two declarations from the same end differing only in `ownership` are -// two relationships, and mutual `owned` is a contradiction. `modelith lint` -// reports the contradictions. -func (e *edge) folds(from string, owned bool) bool { +// two relationships, and mutual `owned` is a contradiction `modelith lint` +// reports as an error. +func (e *edge) folds(from, label string, owned bool) bool { if e.from == from { - return e.owned == owned + return e.owned == owned && e.label == label } - return !e.owned || !owned + return e.foldedFrom == "" && (!e.owned || !owned) +} + +// merge folds a declaration into e, choosing the label the single drawn line +// carries. The owning end wins, because its role names the relationship the +// composition is about; with neither end owning, the end whose entity sorts +// first wins, so the choice never depends on iteration order. The role the +// diagram drops is still in that entity's relationship list in the Markdown — +// the diagram is a declaredly lossy view (ADR-0002). +func (e *edge) merge(from, label string, owned bool) { + switch { + case owned && !e.owned: + e.label = label + case e.owned && !owned: + // The existing edge is the owning end; its label stands. + case from < e.from: + e.label = label + } + if from != e.from { + e.foldedFrom = from + } + e.owned = e.owned || owned } // ER renders the model as a Mermaid erDiagram. Ordinary attributes are @@ -139,14 +169,14 @@ func ER(m *model.Model) string { label := relationshipLabel(rel) owned := rel.Ownership == "owned" - // Group candidate declarations of one drawn line. The key includes - // the cardinality normalized to the sorted-pair orientation, so a - // relationship declared from both sides with inverse cardinalities - // (A "1:n" B, B "n:1" A) is a candidate to fold — while genuinely - // distinct edges (GO-3) or contradictory reciprocal declarations - // (GO-1) get distinct keys and both render, surfacing the conflict - // instead of silently dropping one. Matching the key is necessary - // but not sufficient: edge.folds decides. + // Group candidate declarations of one drawn line. The key is the + // pair plus the cardinality normalized to the sorted-pair + // orientation, so a relationship declared from both sides with + // inverse cardinalities (A "1:n" B, B "n:1" A) is a candidate to + // fold — while genuinely distinct edges (GO-3) or contradictory + // reciprocal declarations (GO-1) get distinct keys and both render, + // surfacing the conflict instead of silently dropping one. Matching + // the key is necessary but not sufficient: edge.folds decides. pair := []string{name, rel.Entity} sort.Strings(pair) card := rel.Cardinality @@ -157,18 +187,14 @@ func ER(m *model.Model) string { // semantically equal but differently written cardinalities // ("1:n" and "0..n:1") dedupes to one edge. card = model.CanonicalCardinality(card) - key := pair[0] + "\x00" + pair[1] + "\x00" + card + "\x00" + label + key := pair[0] + "\x00" + pair[1] + "\x00" + card folded := false for _, e := range byKey[key] { - if !e.folds(name, owned) { + if !e.folds(name, label, owned) { continue } - // Ownership belongs to the relationship, not to the end that - // declared it: a parent declaring `owned` and the child - // declaring `referenced` are one identifying relationship seen - // from two sides, so the folded edge stays solid (ADR-0008). - e.owned = e.owned || owned + e.merge(name, label, owned) folded = true break } diff --git a/internal/render/mermaid/mermaid_test.go b/internal/render/mermaid/mermaid_test.go index 491ee69..e06cb52 100644 --- a/internal/render/mermaid/mermaid_test.go +++ b/internal/render/mermaid/mermaid_test.go @@ -165,9 +165,11 @@ func TestADR_0008_OwnershipFoldsAcrossDeclarations(t *testing.T) { // TestADR_0008_FoldsOnlyGenuineReciprocals pins the fold predicate of ADR-0008. // Two declarations become one line only when they are one relationship seen -// from two sides — opposite ends, inverse cardinality, the same label, and at -// most one end claiming `owned` — or when one is an exact duplicate of the -// other. Everything else draws both lines, so no declaration vanishes. +// from two sides — opposite ends, inverse cardinality, and at most one end +// claiming `owned` — or when one is an exact duplicate of the other. Roles are +// not part of the predicate: the two ends of a composition naturally name +// different roles. Everything else draws both lines, so no declaration +// vanishes. func TestADR_0008_FoldsOnlyGenuineReciprocals(t *testing.T) { t.Parallel() cases := []struct { @@ -212,11 +214,25 @@ func TestADR_0008_FoldsOnlyGenuineReciprocals(t *testing.T) { want: []string{" A ||--o{ B : \"\"\n"}, }, { - name: "opposite ends, roles differ: two labels, so both lines draw", + name: "opposite ends, roles differ: one line, labelled by the owning end", a: model.Relationship{Entity: "B", Cardinality: "1:n", Ownership: "owned", Role: "part"}, b: model.Relationship{Entity: "A", Cardinality: "n:1", Role: "whole"}, bFrom: "B", - want: []string{" A ||--o{ B : \"part\"\n", " B }o..|| A : \"whole\"\n"}, + want: []string{" A ||--o{ B : \"part\"\n"}, + }, + { + name: "opposite ends, roles differ, neither owns: the first entity's role labels it", + a: model.Relationship{Entity: "B", Cardinality: "1:n", Role: "part"}, + b: model.Relationship{Entity: "A", Cardinality: "n:1", Role: "whole"}, + bFrom: "B", + want: []string{" A ||..o{ B : \"part\"\n"}, + }, + { + name: "same end, roles differ: an Owner and a Member are two relationships", + a: model.Relationship{Entity: "B", Cardinality: "1:n", Role: "Owner"}, + b: model.Relationship{Entity: "B", Cardinality: "1:n", Role: "Member"}, + bFrom: "A", + want: []string{" A ||..o{ B : \"Owner\"\n", " A ||..o{ B : \"Member\"\n"}, }, } for _, tc := range cases { @@ -242,6 +258,58 @@ func TestADR_0008_FoldsOnlyGenuineReciprocals(t *testing.T) { } } +// TestADR_0008_ReciprocalCompositionFoldsToOneEdge is the regression guard for +// the textbook composition pattern: a parent owns a child and the child +// references the parent back, each naming its own end's role. That is one +// relationship seen from two sides — the case the fold exists for — so it must +// draw exactly one solid line, labelled by the owning end. Requiring the two +// roles to match instead would split it into a solid line and a dashed one. +// `TestADR_0008_ReciprocalCompositionLintsClean` pins the linter's half. +func TestADR_0008_ReciprocalCompositionFoldsToOneEdge(t *testing.T) { + t.Parallel() + m := &model.Model{Entities: map[string]model.Entity{ + "Alpha": {Definition: "one", Relationships: []model.Relationship{ + {Entity: "Beta", Cardinality: "1:n", Ownership: "owned", Role: "part"}, + }}, + "Beta": {Definition: "two", Relationships: []model.Relationship{ + {Entity: "Alpha", Cardinality: "n:1", Ownership: "referenced", Role: "whole"}, + }}, + }} + out := ER(m) + if want := " Alpha ||--o{ Beta : \"part\"\n"; !strings.Contains(out, want) { + t.Errorf("expected one solid edge %q; got:\n%s", want, out) + } + if n := strings.Count(out, " : "); n != 1 { + t.Errorf("expected exactly one edge, got %d:\n%s", n, out) + } +} + +// TestERFoldsOneReciprocalPerEdge guards the fold against absorbing more than +// one declaration from the opposite end. Three declarations on one pair are not +// one relationship, and the linter deliberately leaves such a pair alone, so the +// renderer must not merge them all into a single line. +func TestERFoldsOneReciprocalPerEdge(t *testing.T) { + t.Parallel() + m := &model.Model{Entities: map[string]model.Entity{ + "Alpha": {Definition: "one", Relationships: []model.Relationship{ + {Entity: "Beta", Cardinality: "n:n", Role: "Owner"}, + }}, + "Beta": {Definition: "two", Relationships: []model.Relationship{ + {Entity: "Alpha", Cardinality: "n:n", Role: "Member"}, + {Entity: "Alpha", Cardinality: "n:n", Role: "Watcher"}, + }}, + }} + out := ER(m) + if n := strings.Count(out, " : "); n != 2 { + t.Errorf("expected two edges, got %d:\n%s", n, out) + } + for _, want := range []string{" Alpha }o..o{ Beta : \"Owner\"\n", " Beta }o..o{ Alpha : \"Watcher\"\n"} { + if !strings.Contains(out, want) { + t.Errorf("expected edge %q; got:\n%s", want, out) + } + } +} + // TestADR_0008_SelfRelationshipRendersInEntityBlock pins the self-relationship // half of ADR-0008: Mermaid's ER layout has no self-loop handling (issue #26), // so the relationship becomes a row in the entity's own block and no edge. diff --git a/plugin/skills/domain-model-author/SKILL.md b/plugin/skills/domain-model-author/SKILL.md index 5270487..817ed00 100644 --- a/plugin/skills/domain-model-author/SKILL.md +++ b/plugin/skills/domain-model-author/SKILL.md @@ -113,9 +113,10 @@ Follow the format exactly (see the schema reference). Key conventions: - `cardinality` is one of `1:1`, `1:n`, `n:1`, `n:n`. If you declare the same relationship from both entities, the two cardinalities must be inverses (`1:n` ⇄ `n:1`; `1:1`/`n:n` symmetric) — a contradiction is a lint **error**. - So is a pair where both ends claim `ownership: owned`, or where the ends - disagree on ownership under different roles. Declaring it from one side only - is fine and usually clearer. + So is a pair where both ends claim `ownership: owned` — a relationship is + owned by at most one end. The two ends may name different roles; the diagram + folds them into one line either way. Declaring it from one side only is fine + and usually clearer. - **`role` is a short role name** (`` `Owner` ``, `` `Predecessor` ``), not a sentence: it is the only label drawn on the diagram line, so prose there collides with neighbouring lines and warns. Put the explanation in `note`. diff --git a/plugin/skills/domain-model-lint/SKILL.md b/plugin/skills/domain-model-lint/SKILL.md index 9794d92..aac5fd6 100644 --- a/plugin/skills/domain-model-lint/SKILL.md +++ b/plugin/skills/domain-model-lint/SKILL.md @@ -45,8 +45,8 @@ the flag. `invariants_touched` or an action's `preserves` names an invariant id no entity or top-level `invariants` entry declares; the same invariant id is declared twice (entity-level and top-level invariants share one id namespace); reciprocal cardinalities - that aren't inverses; reciprocal declarations that both claim `ownership: owned`, - or that disagree on ownership under different roles. Warnings: a backticked term resolves to no entity, role, + that aren't inverses; reciprocal declarations that both claim + `ownership: owned`. Warnings: a backticked term resolves to no entity, role, or actor; an action `actor` that's neither an entity nor a glossary term; a PascalCase attribute `type` that names no defined enum — usually a typo or a concept that was never named; a relationship `role` written as prose, which diff --git a/project-docs/adr/0008-er-diagram-conventions.md b/project-docs/adr/0008-er-diagram-conventions.md index 6bc4c5c..a8776db 100644 --- a/project-docs/adr/0008-er-diagram-conventions.md +++ b/project-docs/adr/0008-er-diagram-conventions.md @@ -16,20 +16,27 @@ fakes structure the ER cannot honestly show. non-identifying (dashed) `..`. That is standard ER semantics for composition and costs no label space. 2. **Two declarations become one line only when they are one relationship.** The - renderer merges a pair of declarations in exactly two cases: an exact - duplicate from the same end, and a genuine reciprocal — the same pair, - inverse cardinality, the same label, declared from *opposite* ends, with at - most one end claiming `owned`. A genuine reciprocal draws solid if either end - said `owned`, because ownership belongs to the relationship rather than to - the end that named it. Every other case draws both lines, so no declaration - disappears from the diagram: two declarations from the same end that differ - only in `ownership` are two relationships, and mutual `owned` is a - contradiction, not a fold. + renderer merges a pair of declarations in exactly two cases: a genuine + reciprocal — the same pair, inverse cardinality, declared from *opposite* + ends, with at most one end claiming `owned` — and an exact duplicate from the + same end. A genuine reciprocal draws solid if either end said `owned`, + because ownership belongs to the relationship rather than to the end that + named it. - The contradictions the renderer refuses to swallow are lint **errors**: - mutual `owned` from both ends, and a reciprocal pair that disagrees on - ownership without folding (their roles differ, so the diagram would draw one - solid and one dashed line for the same relationship). + **Roles are not part of the predicate.** The two ends of a composition + naturally name different roles — a parent's `part` is a child's `whole` — + and that is the pattern the fold exists for, not a conflict. The single line + is labelled by the owning end's role; with neither end owning, by the role + from the end whose entity sorts first, so the choice never depends on + iteration order. The other role is dropped from the *diagram* only: the + Markdown lists each entity's own relationships, so both roles remain in the + document. That is ADR-0002's declaredly lossy view, not information loss. + + Every other case draws both lines, so no declaration disappears: two + declarations from the same end that differ in `ownership` or in role are two + relationships, and mutual `owned` is a contradiction, not a fold. Mutual + `owned` is the one contradiction a reciprocal pair can hold, and it is a lint + **error**. 3. **`role` is the only label.** The `ownership` and `cardinality` fallbacks are gone; a relationship with no role gets an empty label. Precise counts already live in the Markdown table per ADR-0002, and spending label space on them @@ -62,9 +69,10 @@ reads: ordinary attributes are omitted, self-relationships are not. Ownership becomes visible at a glance across the whole diagram, which makes an incorrect `ownership` easier to spot than the old word label did. -Point 2 trades a tidier diagram for an honest one. A model whose two ends -disagree gets two lines and an error rather than a single line that quietly -picks a winner — the disagreement is a modeling question only the author can -settle. +Point 2 draws two lines where it cannot prove there is one. That is deliberate: +a diagram that quietly picks a winner among declarations hides a modeling +question only the author can settle. The one place it does pick — which role +labels a folded reciprocal — is a label choice inside a single line the model +does say is one relationship, and the dropped role is still in the Markdown. Pinned by `TestADR_0008_*` in `internal/render/mermaid` and in `internal/lint`. From 0629f96d43943ba94f52f2673033b2c86d258118 Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Sat, 25 Jul 2026 20:25:00 -0700 Subject: [PATCH 4/4] fix(render): draw every declaration when the reciprocal pairing is ambiguous MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-4 review fix. Round 2's `foldedFrom` guard stopped an edge absorbing a second reciprocal declaration but still absorbed the first, and which one that was depended on list order. A Project declaring `defaults` and `overrides` to Policy, with Policy declaring `parent` back, rendered `parent` and `overrides` and dropped `defaults` — swap the two Project declarations and `overrides` went instead. Both linted clean. The single declaration is the reciprocal of one of the two, and the format cannot say which. So the fold is now gated on that ambiguity: a fold requires each end to declare the line at most once. Where it does not hold, nothing folds and every declaration draws its own line — lossless and independent of declaration order. `foldedFrom` is gone; with the gate it can never fire. A lint warning, never an error, names such a pair: the diagram will show more lines than the author probably means, and declaring each relationship from one end only resolves it. It fires on no existing fixture and neither shipped example. The grouping both the renderer and the linter need now lives in one place, `internal/model/edges.go`, so the fold rule and the diagnostic cannot drift apart — which is what produced the last two rounds. ADR-0008's "no declaration disappears" claim was falsified by this defect. It is replaced with the guarantee stated exactly: every declaration draws, except one indistinguishable from an earlier declaration by the same entity, and the role of the non-labelling end of a folded reciprocal, which the Markdown still carries. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Joe Beda --- docs/04-reading-the-diagrams.md | 4 + docs/06-schema-reference.md | 12 +- internal/lint/lint.go | 41 +++- internal/lint/lint_test.go | 184 +++++++++++++++++- internal/model/edges.go | 126 ++++++++++++ internal/render/mermaid/mermaid.go | 123 +++++------- internal/render/mermaid/mermaid_test.go | 117 +++++++++-- plugin/skills/domain-model-author/SKILL.md | 6 +- plugin/skills/domain-model-lint/SKILL.md | 4 +- .../adr/0008-er-diagram-conventions.md | 45 +++-- 10 files changed, 543 insertions(+), 119 deletions(-) create mode 100644 internal/model/edges.go diff --git a/docs/04-reading-the-diagrams.md b/docs/04-reading-the-diagrams.md index d93d93a..f168074 100644 --- a/docs/04-reading-the-diagrams.md +++ b/docs/04-reading-the-diagrams.md @@ -142,6 +142,10 @@ nothing you wrote vanishes from the picture: error. - If one entity declares two relationships to the same entity that differ in `ownership` or in role, those are two relationships, and both draw. +- If one end declares the same relationship *twice* and the other declares it + back once, there's no way to tell which two of the three are the pair. Every + declaration draws its own line, and `modelith lint` warns that the pairing is + ambiguous. Declaring each relationship from one end only clears it up. ## The labels on the lines diff --git a/docs/06-schema-reference.md b/docs/06-schema-reference.md index 397d246..de9c3ef 100644 --- a/docs/06-schema-reference.md +++ b/docs/06-schema-reference.md @@ -162,10 +162,11 @@ and covered in full in [Reading the Diagrams](./04-reading-the-diagrams.md): (non-identifying) for `referenced` and for an omitted `ownership`. It costs no label space. Ownership belongs to the relationship rather than the end that declared it, so a parent's `owned` and the child's `referenced` fold into one - solid line when their cardinalities are inverses — even when the two ends name - different roles, in which case the owning end's role labels the line and the - other stays in the Markdown. Declarations the renderer can't reduce to one - relationship draw as separate lines, and mutual `owned` is a lint error. + solid line when their cardinalities are inverses and each end declares it + once — even when the two ends name different roles, in which case the owning + end's role labels the line and the other stays in the Markdown. Declarations + the renderer can't reduce to one relationship draw as separate lines: mutual + `owned` is a lint error, and a pairing it can't resolve is a lint warning. - **`role` is the only label** — `ownership` and `cardinality` are never written on a line. Keep the role short; put the explanation in `note`. - **A self-referential relationship becomes a row inside the entity's box** @@ -307,6 +308,9 @@ The JSON Schema covers structure. [`modelith lint`](./07-cli.md) adds: - a relationship `role` that reads as prose (too long for a label, more than four words, or ending a sentence) — the role is the only label on the rendered diagram line, so the explanation belongs in `note`; + - a pair where one end declares the same relationship more than once and + the other declares it back, so which is the reciprocal of which can't be + determined — the diagram draws every declaration as its own line; - an attribute `type` that looks like an enum reference (PascalCase) but names no defined enum; - an action `actor` that is neither a defined entity nor a glossary term. diff --git a/internal/lint/lint.go b/internal/lint/lint.go index ba8778e..4a7a385 100644 --- a/internal/lint/lint.go +++ b/internal/lint/lint.go @@ -107,6 +107,7 @@ func Run(data []byte) (*Result, error) { runRelationshipShape(m, res) runSubtypes(m, res) runReciprocity(m, res) + runPairing(m, res) runCompleteness(m, res) sortFindings(res) @@ -691,10 +692,40 @@ func runReciprocity(m *model.Model, res *Result) { } } -// normalizeRole reduces a role to the text the diagram labels a line with: -// backticks are markup, and surrounding space is not content. -func normalizeRole(role string) string { - return strings.TrimSpace(strings.ReplaceAll(role, "`", "")) +// runPairing warns where a relationship declared from both ends cannot be +// paired up: one end declares the same line more than once, so which +// declaration is the reciprocal of which is undeterminable — the format has no +// way to say. The renderer refuses to guess and draws every declaration as its +// own line (ADR-0008), which is lossless but shows more lines than the author +// probably means, so it is worth naming. +// +// A warning, never an error: the model may be exactly right, and there is no +// wording of it the linter could demand instead. Declaring each relationship +// from one end only resolves it. +func runPairing(m *model.Model, res *Result) { + for _, g := range model.EdgeGroups(m) { + if !g.AmbiguousPairing() { + continue + } + // Point at the first declaration on the crowded end — the one an author + // would edit — preferring the alphabetically earlier entity when both + // ends are crowded, so the finding is stable. + at := g.First + if len(at) < 2 && len(g.Second) > 1 { + at = g.Second + } + d := at[0] + res.Findings = append(res.Findings, Finding{ + Severity: SeverityWarning, + Category: CategorySemantic, + Path: fmt.Sprintf("/entities/%s/relationships/%d", d.From, d.Index), + Message: fmt.Sprintf( + "ambiguous reciprocal pairing between %q and %q at cardinality %q: %s declares it %d time(s) and %s %d time(s), so which declaration is the reciprocal of which cannot be determined — the diagram draws every declaration as its own line; declare each relationship from one end only to pair them up", + g.FirstN, g.SecondN, g.First[0].Rel.Cardinality, + g.FirstN, len(g.First), g.SecondN, len(g.Second), + ), + }) + } } func runCompleteness(m *model.Model, res *Result) { @@ -861,7 +892,7 @@ const roleLabelMax = 40 // ("`Project`.owner") or a version ("v1.0 owner") as the end of a sentence. // "`Owner` or `Member`" passes; "the record this one supersedes" does not. func readsAsProse(role string) bool { - role = normalizeRole(role) + role = model.NormalizeRole(role) if role == "" { return false } diff --git a/internal/lint/lint_test.go b/internal/lint/lint_test.go index f46a8eb..d5a5491 100644 --- a/internal/lint/lint_test.go +++ b/internal/lint/lint_test.go @@ -706,13 +706,13 @@ func TestADR_0008_MutualOwnershipIsError(t *testing.T) { name: "mutual owned under differing roles is still a contradiction", aOwn: "owned", bOwn: "owned", aRole: "part", bRole: "whole", - want: "mutual ownership", + want: "mutual ownership", }, { name: "one end owning, the other referencing", aOwn: "owned", bOwn: "referenced", aRole: "part", bRole: "whole", - want: "", + want: "", }, { name: "an omitted ownership on the other end is the same as referenced", @@ -723,7 +723,7 @@ func TestADR_0008_MutualOwnershipIsError(t *testing.T) { name: "neither end claims ownership", aOwn: "", bOwn: "", aRole: "part", bRole: "whole", - want: "", + want: "", }, } for _, tc := range cases { @@ -831,6 +831,182 @@ scenarios: } } +// TestADR_0008_AmbiguousPairingIsWarning pins the diagnostic for a pair whose +// reciprocal declarations cannot be matched up: one end declares the same line +// twice and the other declares it once, so which is the reciprocal of which is +// undeterminable. The renderer refuses to guess and draws all of them, which is +// lossless but shows more lines than the author probably means. +// +// A warning, never an error: the model may be exactly right. It must stay +// silent whenever each direction declares the line at most once, which is every +// shipped example and every other fixture. +func TestADR_0008_AmbiguousPairingIsWarning(t *testing.T) { + t.Parallel() + cases := []struct { + name string + rels string + want bool + path string + }{ + { + name: "two forward, one back", + rels: ` + Project: + definition: A container. + relationships: + - entity: Policy + cardinality: "1:n" + role: defaults + - entity: Policy + cardinality: "1:n" + role: overrides + Policy: + definition: A rule. + relationships: + - entity: Project + cardinality: "n:1" + role: parent + ownership: owned +`, + want: true, + path: "/entities/Project/relationships/0", + }, + { + name: "one forward, two back", + rels: ` + Project: + definition: A container. + relationships: + - entity: Policy + cardinality: "1:n" + role: parent + ownership: owned + Policy: + definition: A rule. + relationships: + - entity: Project + cardinality: "n:1" + role: defaults + - entity: Project + cardinality: "n:1" + role: overrides +`, + want: true, + path: "/entities/Policy/relationships/0", + }, + { + name: "two forward, none back: nothing to pair, so nothing ambiguous", + rels: ` + Project: + definition: A container. + relationships: + - entity: Policy + cardinality: "1:n" + role: defaults + - entity: Policy + cardinality: "1:n" + role: overrides + Policy: + definition: A rule. +`, + want: false, + }, + { + name: "one each way: the ordinary reciprocal", + rels: ` + Project: + definition: A container. + relationships: + - entity: Policy + cardinality: "1:n" + role: parent + ownership: owned + Policy: + definition: A rule. + relationships: + - entity: Project + cardinality: "n:1" + role: child +`, + want: false, + }, + { + name: "two forward at different cardinalities, one back: distinct lines", + rels: ` + Project: + definition: A container. + relationships: + - entity: Policy + cardinality: "1:n" + role: defaults + - entity: Policy + cardinality: "1:1" + role: primary + Policy: + definition: A rule. + relationships: + - entity: Project + cardinality: "n:1" + role: parent + ownership: owned +`, + want: false, + }, + { + name: "the second forward declaration is an exact duplicate of the first", + rels: ` + Project: + definition: A container. + relationships: + - entity: Policy + cardinality: "1:n" + role: defaults + - entity: Policy + cardinality: "1:n" + role: defaults + Policy: + definition: A rule. + relationships: + - entity: Project + cardinality: "n:1" + role: parent + ownership: owned +`, + want: false, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + res, err := Run([]byte("kind: DomainModel\nversion: v1\nentities:" + tc.rels)) + if err != nil { + t.Fatal(err) + } + got := findingWithMessage(res.Findings, "ambiguous reciprocal pairing") + if got != tc.want { + t.Fatalf("ambiguous-pairing warning = %v, want %v; findings: %+v", got, tc.want, res.Findings) + } + if !got { + return + } + for _, f := range res.Findings { + if !strings.Contains(f.Message, "ambiguous reciprocal pairing") { + continue + } + if f.Severity != SeverityWarning || f.Category != CategorySemantic { + t.Errorf("expected a semantic warning, got %s/%s", f.Severity, f.Category) + } + if f.Path != tc.path { + t.Errorf("path = %q, want %q", f.Path, tc.path) + } + } + if res.HasBlocking(false) { + t.Error("an ambiguous pairing is advisory, not blocking") + } + }) + } +} + func TestActionPreservesUnknownInvariantIsError(t *testing.T) { src := ` kind: DomainModel @@ -1257,7 +1433,7 @@ entities: definition: A way to pay. invariants: - id: pm-usable - statement: A `+"`PaymentMethod`"+` is either active or revoked. + statement: A ` + "`PaymentMethod`" + ` is either active or revoked. Card: definition: A payment method backed by a card; adds no rule of its own. subtypeOf: PaymentMethod diff --git a/internal/model/edges.go b/internal/model/edges.go new file mode 100644 index 0000000..9e32cbb --- /dev/null +++ b/internal/model/edges.go @@ -0,0 +1,126 @@ +package model + +import ( + "sort" + "strings" +) + +// This file holds the one definition of "which relationship line does this +// declaration describe". The renderer uses it to decide what to fold into one +// line; the linter uses it to warn when that decision cannot be made. Keeping +// both on the same code is deliberate: a diagram whose fold rule and diagnostic +// disagree is worse than either alone. + +// NormalizeRole reduces a role to its text: backticks are markup, and +// surrounding or repeated whitespace is not content. Two declarations whose +// roles normalize equal are the same role. +func NormalizeRole(role string) string { + return strings.Join(strings.Fields(strings.ReplaceAll(role, "`", "")), " ") +} + +// EdgeKey identifies the relationship line a declaration would draw: the +// unordered entity pair plus the cardinality canonicalized to that pair's +// orientation. Two declarations share a key exactly when they could be the same +// line — including one relationship declared from both ends with inverse +// cardinalities ("1:n" one way, "n:1" the other). A self-referential +// declaration has no key; those are drawn inside the entity's own block. +func EdgeKey(from string, rel Relationship) string { + pair := []string{from, rel.Entity} + sort.Strings(pair) + card := rel.Cardinality + if from != pair[0] { + card = InvertCardinality(card) + } + return pair[0] + "\x00" + pair[1] + "\x00" + CanonicalCardinality(card) +} + +// DeclarationKey identifies a declaration precisely enough that two sharing it +// are indistinguishable wherever the model is drawn: the line, the end that +// declared it, whether that end claims ownership, and the role. +func DeclarationKey(from string, rel Relationship) string { + owned := "0" + if rel.Ownership == "owned" { + owned = "1" + } + return EdgeKey(from, rel) + "\x00" + from + "\x00" + owned + "\x00" + NormalizeRole(rel.Role) +} + +// Declaration is one relationship declaration together with where it was made, +// so a caller can point a diagnostic at it. +type Declaration struct { + From string // the entity whose relationships list holds it + Index int // its position in that list + Rel Relationship +} + +// EdgeGroup is every declaration that could draw one relationship line. First +// holds the declarations made by the alphabetically earlier entity of the pair, +// Second those made by the later one; either may be empty. +type EdgeGroup struct { + Key string + First, Second []Declaration + FirstN, SecondN string // the two entity names, First's and Second's +} + +// AmbiguousPairing reports whether the group's declarations cannot be paired up +// across the two ends. Both ends declare the line, but at least one of them +// declares it more than once, so which declaration is the reciprocal of which +// is undeterminable — the format has no way to say. A caller must not guess: +// picking one pairing silently drops the role of whichever declaration it +// overwrote, and which one that is would depend on declaration order. +func (g *EdgeGroup) AmbiguousPairing() bool { + if len(g.First) == 0 || len(g.Second) == 0 { + return false + } + return len(g.First) > 1 || len(g.Second) > 1 +} + +// EdgeGroups groups a model's non-self relationship declarations by the line +// each would draw, dropping any declaration indistinguishable from an earlier +// one made by the same entity (an exact duplicate carries no information the +// first does not). Groups come back in sorted-key order, and the declarations +// within a group in declaration order — entity name, then position — so a +// caller's output is deterministic. +func EdgeGroups(m *Model) []*EdgeGroup { + byKey := map[string]*EdgeGroup{} + seen := map[string]bool{} + for _, name := range m.EntityNames() { + for i, rel := range m.Entities[name].Relationships { + if rel.Entity == name { + continue // drawn inside the entity's own block, never as a line + } + dk := DeclarationKey(name, rel) + if seen[dk] { + continue + } + seen[dk] = true + + key := EdgeKey(name, rel) + g, ok := byKey[key] + if !ok { + pair := []string{name, rel.Entity} + sort.Strings(pair) + g = &EdgeGroup{Key: key, FirstN: pair[0], SecondN: pair[1]} + byKey[key] = g + } + d := Declaration{From: name, Index: i, Rel: rel} + if name == g.FirstN { + g.First = append(g.First, d) + } else { + g.Second = append(g.Second, d) + } + } + } + + keys := make([]string, 0, len(byKey)) + for k := range byKey { + keys = append(keys, k) + } + sort.Strings(keys) + + out := make([]*EdgeGroup, 0, len(byKey)) + for _, key := range keys { + out = append(out, byKey[key]) + } + return out +} diff --git a/internal/render/mermaid/mermaid.go b/internal/render/mermaid/mermaid.go index ca2fbf2..c0a605e 100644 --- a/internal/render/mermaid/mermaid.go +++ b/internal/render/mermaid/mermaid.go @@ -5,7 +5,6 @@ package mermaid import ( "fmt" - "sort" "strconv" "strings" @@ -77,50 +76,18 @@ func rightMarker(m model.Multiplicity) string { // edge is one rendered relationship line, accumulated before emission so that // declarations of the same relationship from both ends fold into it. type edge struct { - from, to string - card string // as declared by `from`, so the markers read left to right - label string - owned bool - foldedFrom string // the opposite end already folded in, if any + from, to string + card string // as declared by `from`, so the markers read left to right + label string + owned bool } -// folds reports whether a declaration made by `from`, with the given ownership -// and label, is the same drawn line as e rather than a second one. It is only -// ever asked about declarations that already agree on the pair and on the -// canonical cardinality (normalized to the sorted-pair orientation, so inverses -// match). Two such declarations merge in exactly two cases (ADR-0008): -// -// - opposite ends declared it and at most one of them claims `owned` — one -// relationship seen from two sides, whose ownership belongs to the -// relationship rather than to the end that named it. The two ends may name -// different roles: that is the ordinary composition pattern (a parent's -// "part" is a child's "whole"), not a conflict, so the label is chosen -// rather than compared (see merge). Only one opposite-end declaration -// folds in, mirroring the linter, which reconciles a pair only when each -// direction declares it exactly once. -// - the same end declared it twice with the same ownership and the same label -// — an exact duplicate, which would draw two indistinguishable lines. The -// label matters here: two declarations from one end with different roles -// (a User who is both `Owner` and `Member` of a Project) are two -// relationships. -// -// Everything else stays a distinct edge, so no declaration disappears from the -// diagram: two declarations from the same end differing only in `ownership` are -// two relationships, and mutual `owned` is a contradiction `modelith lint` -// reports as an error. -func (e *edge) folds(from, label string, owned bool) bool { - if e.from == from { - return e.owned == owned && e.label == label - } - return e.foldedFrom == "" && (!e.owned || !owned) -} - -// merge folds a declaration into e, choosing the label the single drawn line -// carries. The owning end wins, because its role names the relationship the -// composition is about; with neither end owning, the end whose entity sorts -// first wins, so the choice never depends on iteration order. The role the -// diagram drops is still in that entity's relationship list in the Markdown — -// the diagram is a declaredly lossy view (ADR-0002). +// merge folds a reciprocal declaration into e, choosing the label the single +// drawn line carries. The owning end wins, because its role names the +// relationship the composition is about; with neither end owning, the end whose +// entity sorts first wins, so the choice never depends on declaration order. +// The role the diagram drops is still in that entity's relationship list in the +// Markdown — the diagram is a declaredly lossy view (ADR-0002). func (e *edge) merge(from, label string, owned bool) { switch { case owned && !e.owned: @@ -130,9 +97,6 @@ func (e *edge) merge(from, label string, owned bool) { case from < e.from: e.label = label } - if from != e.from { - e.foldedFrom = from - } e.owned = e.owned || owned } @@ -159,47 +123,56 @@ func ER(m *model.Model) string { b.WriteString(" }\n") } + // A fold is a claim that two declarations are one relationship seen from two + // sides. That claim is only safe when each end declares the line at most + // once: with two declarations on one end, which is the reciprocal of which + // is undeterminable, and folding either would drop the other's role + // depending on declaration order. model.EdgeGroups makes that call, and + // `modelith lint` warns about the same groups (ADR-0008). + ambiguous := map[string]bool{} + for _, g := range model.EdgeGroups(m) { + if g.AmbiguousPairing() { + ambiguous[g.Key] = true + } + } + var edges []*edge byKey := map[string][]*edge{} + seen := map[string]bool{} for _, name := range m.EntityNames() { for _, rel := range m.Entities[name].Relationships { if rel.Entity == name { continue // already rendered inside the entity's block } + // Two declarations from one end that agree on everything draw two + // indistinguishable lines; the second carries nothing. + dk := model.DeclarationKey(name, rel) + if seen[dk] { + continue + } + seen[dk] = true + label := relationshipLabel(rel) owned := rel.Ownership == "owned" + key := model.EdgeKey(name, rel) - // Group candidate declarations of one drawn line. The key is the - // pair plus the cardinality normalized to the sorted-pair - // orientation, so a relationship declared from both sides with - // inverse cardinalities (A "1:n" B, B "n:1" A) is a candidate to - // fold — while genuinely distinct edges (GO-3) or contradictory - // reciprocal declarations (GO-1) get distinct keys and both render, - // surfacing the conflict instead of silently dropping one. Matching - // the key is necessary but not sufficient: edge.folds decides. - pair := []string{name, rel.Entity} - sort.Strings(pair) - card := rel.Cardinality - if name != pair[0] { - card = model.InvertCardinality(card) - } - // Canonicalize so a relationship declared from both sides with - // semantically equal but differently written cardinalities - // ("1:n" and "0..n:1") dedupes to one edge. - card = model.CanonicalCardinality(card) - key := pair[0] + "\x00" + pair[1] + "\x00" + card - - folded := false - for _, e := range byKey[key] { - if !e.folds(name, label, owned) { + // A reciprocal folds when the pairing is unambiguous, it comes from + // the opposite end, and at most one end claims `owned` — mutual + // `owned` is a contradiction, not a fold, and stays two lines with + // a lint error against it. + if !ambiguous[key] { + folded := false + for _, e := range byKey[key] { + if e.from == name || (e.owned && owned) { + continue + } + e.merge(name, label, owned) + folded = true + break + } + if folded { continue } - e.merge(name, label, owned) - folded = true - break - } - if folded { - continue } e := &edge{from: name, to: rel.Entity, card: rel.Cardinality, label: label, owned: owned} byKey[key] = append(byKey[key], e) diff --git a/internal/render/mermaid/mermaid_test.go b/internal/render/mermaid/mermaid_test.go index e06cb52..73555c9 100644 --- a/internal/render/mermaid/mermaid_test.go +++ b/internal/render/mermaid/mermaid_test.go @@ -1,6 +1,8 @@ package mermaid import ( + "slices" + "sort" "strings" "testing" @@ -284,29 +286,112 @@ func TestADR_0008_ReciprocalCompositionFoldsToOneEdge(t *testing.T) { } } -// TestERFoldsOneReciprocalPerEdge guards the fold against absorbing more than -// one declaration from the opposite end. Three declarations on one pair are not -// one relationship, and the linter deliberately leaves such a pair alone, so the -// renderer must not merge them all into a single line. -func TestERFoldsOneReciprocalPerEdge(t *testing.T) { +// edgeLines returns the rendered relationship lines, sorted. Sorting drops the +// one thing about the output that legitimately follows the source: the order +// the author wrote the declarations in. What must not vary is the set of lines. +func edgeLines(out string) []string { + var lines []string + for _, l := range strings.Split(out, "\n") { + if strings.Contains(l, " : ") { + lines = append(lines, l) + } + } + sort.Strings(lines) + return lines +} + +// TestADR_0008_AmbiguousPairingKeepsEveryDeclaration is the regression guard for +// the defect ADR-0008's earlier "no declaration disappears" wording promised but +// did not deliver. When one end declares the same line twice and the other +// declares it once, no pairing exists in the format: the single declaration is +// the reciprocal of one of the two, and nothing says which. Folding it into +// either drops the other's role, and which one depends on declaration order. +// So nothing folds — every declaration draws. +func TestADR_0008_AmbiguousPairingKeepsEveryDeclaration(t *testing.T) { t.Parallel() m := &model.Model{Entities: map[string]model.Entity{ - "Alpha": {Definition: "one", Relationships: []model.Relationship{ - {Entity: "Beta", Cardinality: "n:n", Role: "Owner"}, + "Project": {Definition: "a container", Relationships: []model.Relationship{ + {Entity: "Policy", Cardinality: "1:n", Role: "defaults", Ownership: "referenced"}, + {Entity: "Policy", Cardinality: "1:n", Role: "overrides", Ownership: "referenced"}, }}, - "Beta": {Definition: "two", Relationships: []model.Relationship{ - {Entity: "Alpha", Cardinality: "n:n", Role: "Member"}, - {Entity: "Alpha", Cardinality: "n:n", Role: "Watcher"}, + "Policy": {Definition: "a rule", Relationships: []model.Relationship{ + {Entity: "Project", Cardinality: "n:1", Role: "parent", Ownership: "owned"}, }}, }} out := ER(m) - if n := strings.Count(out, " : "); n != 2 { - t.Errorf("expected two edges, got %d:\n%s", n, out) + want := []string{ + ` Policy }o--|| Project : "parent"`, + ` Project ||..o{ Policy : "defaults"`, + ` Project ||..o{ Policy : "overrides"`, } - for _, want := range []string{" Alpha }o..o{ Beta : \"Owner\"\n", " Beta }o..o{ Alpha : \"Watcher\"\n"} { - if !strings.Contains(out, want) { - t.Errorf("expected edge %q; got:\n%s", want, out) - } + if got := edgeLines(out); !slices.Equal(got, want) { + t.Errorf("expected every declaration drawn:\n%v\ngot:\n%v\nin:\n%s", want, got, out) + } +} + +// TestERAmbiguousPairingIsOrderIndependent pins the property the round-3 defect +// broke: reordering an entity's relationship list must not change which +// declarations survive or how they draw. Only the order the lines are listed in +// follows the source, so the comparison is on the sorted set. +func TestERAmbiguousPairingIsOrderIndependent(t *testing.T) { + t.Parallel() + cases := []struct { + name string + alpha, bet []model.Relationship + swapAlpha bool // swap alpha's two declarations rather than beta's + }{ + { + name: "two on the first end, one owned, one back", + alpha: []model.Relationship{ + {Entity: "Beta", Cardinality: "1:n", Role: "P", Ownership: "owned"}, + {Entity: "Beta", Cardinality: "1:n", Role: "Q", Ownership: "referenced"}, + }, + bet: []model.Relationship{{Entity: "Alpha", Cardinality: "n:1", Role: "R", Ownership: "referenced"}}, + swapAlpha: true, + }, + { + name: "two on the first end, the owning declaration coming back", + alpha: []model.Relationship{ + {Entity: "Beta", Cardinality: "1:n", Role: "P", Ownership: "referenced"}, + {Entity: "Beta", Cardinality: "1:n", Role: "Q", Ownership: "referenced"}, + }, + bet: []model.Relationship{{Entity: "Alpha", Cardinality: "n:1", Role: "R", Ownership: "owned"}}, + swapAlpha: true, + }, + { + name: "one on the first end, two coming back", + alpha: []model.Relationship{{Entity: "Beta", Cardinality: "1:n", Role: "P", Ownership: "owned"}}, + bet: []model.Relationship{ + {Entity: "Alpha", Cardinality: "n:1", Role: "R1", Ownership: "referenced"}, + {Entity: "Alpha", Cardinality: "n:1", Role: "R2", Ownership: "referenced"}, + }, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + build := func(swapped bool) string { + alpha, bet := slices.Clone(tc.alpha), slices.Clone(tc.bet) + if swapped { + if tc.swapAlpha { + alpha[0], alpha[1] = alpha[1], alpha[0] + } else { + bet[0], bet[1] = bet[1], bet[0] + } + } + return ER(&model.Model{Entities: map[string]model.Entity{ + "Alpha": {Definition: "one", Relationships: alpha}, + "Beta": {Definition: "two", Relationships: bet}, + }}) + } + asDeclared, swapped := build(false), build(true) + if got, want := edgeLines(swapped), edgeLines(asDeclared); !slices.Equal(got, want) { + t.Errorf("swapping the declaration order changed the edges:\nas declared: %v\nswapped: %v", want, got) + } + if n := len(edgeLines(asDeclared)); n != len(tc.alpha)+len(tc.bet) { + t.Errorf("expected every declaration drawn (%d), got %d:\n%s", len(tc.alpha)+len(tc.bet), n, asDeclared) + } + }) } } diff --git a/plugin/skills/domain-model-author/SKILL.md b/plugin/skills/domain-model-author/SKILL.md index 817ed00..40bd071 100644 --- a/plugin/skills/domain-model-author/SKILL.md +++ b/plugin/skills/domain-model-author/SKILL.md @@ -115,8 +115,10 @@ Follow the format exactly (see the schema reference). Key conventions: (`1:n` ⇄ `n:1`; `1:1`/`n:n` symmetric) — a contradiction is a lint **error**. So is a pair where both ends claim `ownership: owned` — a relationship is owned by at most one end. The two ends may name different roles; the diagram - folds them into one line either way. Declaring it from one side only is fine - and usually clearer. + folds them into one line either way. But if one entity declares *two* + relationships to another and that other declares one back, the pairing is + ambiguous (a lint warning) and the diagram draws all three lines. Declaring + each relationship from one side only is fine and usually clearer. - **`role` is a short role name** (`` `Owner` ``, `` `Predecessor` ``), not a sentence: it is the only label drawn on the diagram line, so prose there collides with neighbouring lines and warns. Put the explanation in `note`. diff --git a/plugin/skills/domain-model-lint/SKILL.md b/plugin/skills/domain-model-lint/SKILL.md index aac5fd6..8e9d557 100644 --- a/plugin/skills/domain-model-lint/SKILL.md +++ b/plugin/skills/domain-model-lint/SKILL.md @@ -50,7 +50,9 @@ the flag. or actor; an action `actor` that's neither an entity nor a glossary term; a PascalCase attribute `type` that names no defined enum — usually a typo or a concept that was never named; a relationship `role` written as prose, which - belongs in `note`. Decide which it is and propose the fix. + belongs in `note`; an ambiguous reciprocal pairing, where one end declares the + same relationship twice and the other declares it back. Decide which it is and + propose the fix. - **Completeness (advisory warning).** Gaps, not bugs: an entity with no invariants, an entity no scenario exercises, a glossary term defined but never referenced, or an enum no attribute uses. These are what `--completeness error` diff --git a/project-docs/adr/0008-er-diagram-conventions.md b/project-docs/adr/0008-er-diagram-conventions.md index a8776db..c3409ea 100644 --- a/project-docs/adr/0008-er-diagram-conventions.md +++ b/project-docs/adr/0008-er-diagram-conventions.md @@ -18,25 +18,38 @@ fakes structure the ER cannot honestly show. 2. **Two declarations become one line only when they are one relationship.** The renderer merges a pair of declarations in exactly two cases: a genuine reciprocal — the same pair, inverse cardinality, declared from *opposite* - ends, with at most one end claiming `owned` — and an exact duplicate from the - same end. A genuine reciprocal draws solid if either end said `owned`, - because ownership belongs to the relationship rather than to the end that - named it. + ends, with at most one end claiming `owned`, **and each end declaring that + line only once** — and an exact duplicate from the same end. A genuine + reciprocal draws solid if either end said `owned`, because ownership belongs + to the relationship rather than to the end that named it. **Roles are not part of the predicate.** The two ends of a composition naturally name different roles — a parent's `part` is a child's `whole` — and that is the pattern the fold exists for, not a conflict. The single line is labelled by the owning end's role; with neither end owning, by the role from the end whose entity sorts first, so the choice never depends on - iteration order. The other role is dropped from the *diagram* only: the - Markdown lists each entity's own relationships, so both roles remain in the - document. That is ADR-0002's declaredly lossy view, not information loss. + declaration order. - Every other case draws both lines, so no declaration disappears: two - declarations from the same end that differ in `ownership` or in role are two - relationships, and mutual `owned` is a contradiction, not a fold. Mutual - `owned` is the one contradiction a reciprocal pair can hold, and it is a lint - **error**. + **The once-per-end condition is what makes the fold safe.** If one end + declares the same line twice and the other declares it once, that one + declaration is the reciprocal of one of the two — and the format cannot say + which. Folding it into either drops the other's role, and which one depends + on the order the declarations happen to be written in. So nothing folds for + that pair: every declaration draws its own line. A lint **warning** names the + pair, since more lines appear than the author probably means. + + Mutual `owned` is the one contradiction a reciprocal pair can hold — a + relationship is owned by at most one end — and it is a lint **error**. The + two lines both draw. + + **The guarantee, stated exactly.** Every declaration draws a line, except: + one indistinguishable from an earlier declaration by the same entity (same + line, same ownership, same role), which carries nothing the first does not; + and the non-labelling end of a folded reciprocal, whose *role* is dropped + from the diagram — the entity's own relationship list in the Markdown still + carries it. That is ADR-0002's declaredly lossy view. Nothing else is + dropped, and no output depends on declaration order beyond the order the + lines are listed in. 3. **`role` is the only label.** The `ownership` and `cardinality` fallbacks are gone; a relationship with no role gets an empty label. Precise counts already live in the Markdown table per ADR-0002, and spending label space on them @@ -75,4 +88,12 @@ question only the author can settle. The one place it does pick — which role labels a folded reciprocal — is a label choice inside a single line the model does say is one relationship, and the dropped role is still in the Markdown. +The cost is that a pair with an ambiguous pairing renders more lines than its +author probably wants, with no way to collapse them short of editing the model. +That is the right side to err on: the alternative, a fold that guesses, is a +diagram whose content changes when the YAML is reordered. `model.EdgeGroups` +holds the one definition of which declarations could be the same line, and both +the renderer and the linter read it, so the fold rule and the diagnostic cannot +drift apart. + Pinned by `TestADR_0008_*` in `internal/render/mermaid` and in `internal/lint`.