From 8118de69314a472f1755a7bf4e45415f54816e07 Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Wed, 23 Sep 2026 17:12:38 -0700 Subject: [PATCH] feat(lint): resolve qualified entity references Signed-off-by: Joe Beda --- docs/04-reading-the-diagrams.md | 8 +- docs/06-schema-reference.md | 29 ++-- internal/lint/imports.go | 123 ++++++-------- internal/lint/imports_test.go | 150 +++++++++--------- internal/lint/lint.go | 62 +++----- internal/lint/lint_test.go | 31 ---- internal/model/model.go | 5 +- internal/render/markdown/markdown.go | 35 ++-- internal/render/markdown/markdown_test.go | 24 +++ internal/render/mermaid/mermaid.go | 44 ++++- internal/render/mermaid/mermaid_test.go | 15 ++ .../testdata/qualified_targets.golden.mmd | 10 ++ internal/schema/v1/modelith.schema.json | 8 +- ...tity-references-stop-at-import-boundary.md | 3 + 14 files changed, 300 insertions(+), 247 deletions(-) create mode 100644 internal/render/mermaid/testdata/qualified_targets.golden.mmd create mode 100644 project-docs/adr/0018-qualified-entity-references-stop-at-import-boundary.md diff --git a/docs/04-reading-the-diagrams.md b/docs/04-reading-the-diagrams.md index f168074..4bd9aad 100644 --- a/docs/04-reading-the-diagrams.md +++ b/docs/04-reading-the-diagrams.md @@ -43,6 +43,10 @@ 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). +A qualified relationship target from a direct import also appears as a node, +labelled with its full `scope.Entity` name. It represents the external entity +only; its attributes and relationships remain in the imported model's rendered +document. ## The lines: relationships and cardinality @@ -201,7 +205,9 @@ them in the text: `Owner`"* is an **invariant**, listed under the entity — not something the crow's foot captures. - **Attributes, derived values, and enums** are in the per-entity tables and the - Enums section. + Enums section. A qualified `subtypeOf` is the narrow exception: it appears as + a `subtypeOf "scope.Entity"` row in the local entity's Mermaid box because + Mermaid ER has no generalization edge. - **Actions** (what can be done to an entity, and which invariants they preserve) are listed per entity. diff --git a/docs/06-schema-reference.md b/docs/06-schema-reference.md index b6853d2..6e82d46 100644 --- a/docs/06-schema-reference.md +++ b/docs/06-schema-reference.md @@ -95,7 +95,7 @@ Each key under `entities` is the entity's canonical name (PascalCase, e.g. | Field | Type | Required | Notes | |---|---|---|---| | `definition` | string | yes | Two to four sentences: what it is, what it is not. | -| `subtypeOf` | string | no | Names the entity this one is a kind of (an is-a link). Must reference a defined entity. | +| `subtypeOf` | string | no | Names the entity this one is a kind of (an is-a link). May name a defined local entity or an entity in a direct import as `scope.Entity`. | | `relationships` | list | no | See [Relationship](#relationship). | | `attributes` | list | no | See [Attribute](#attribute). | | `actions` | list | no | Mutations the system exposes. See [Action](#action). | @@ -115,20 +115,23 @@ versions in play, so the ER stays a deliberately lossy view; the Markdown text is the source of truth. Use `subtypeOf` for generalization — when one entity *is a kind of* another -(a `Card` is a `PaymentMethod`). The child declares it, and it must name a -defined entity; the linter errors on an undefined parent or a cycle. A parent's -invariants are understood to cover its subtypes, so a subtype that adds no rule -of its own is not flagged for having no invariants. The Mermaid ER diagram does -not draw the is-a link — erDiagram has no generalization notation, so the -hierarchy lives in the rendered Markdown (each child names its supertype and -each parent lists its subtypes), a deliberately lossy ER per the same principle -as derived entities. +(a `Card` is a `PaymentMethod`). The child declares it. The parent may be a +local entity or a direct import qualified as `scope.Entity`; the latter must +resolve to an entity in that import. The linter errors on an undefined local +parent, missing imported parent, or a cycle among local entities. A local +parent's invariants are understood to cover its subtypes, so a subtype that adds +no rule of its own is not flagged for having no invariants. An imported parent +is a boundary: modelith does not walk its ancestry or inherit its invariants. +The Mermaid ER diagram does not draw the is-a link — erDiagram has no +generalization notation, so the hierarchy lives in the rendered Markdown (each +child names its supertype and each local parent lists its subtypes), a +deliberately lossy ER per the same principle as derived entities. ## Relationship | Field | Type | Required | Notes | |---|---|---|---| -| `entity` | string | yes | Target entity name. Must reference a defined entity. | +| `entity` | string | yes | Target entity name. Must reference a defined local entity or an entity in a direct import as `scope.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 **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. | @@ -150,6 +153,12 @@ invert to themselves). The linter errors on a contradiction, and the renderer co a matching pair into a single edge. Declaring it once is fine; the renderer shows the edge either way. +A relationship may target an entity from a direct import as `scope.Entity`. The +linter validates that imported entity exists, and the renderer shows it as a +qualified external node. Validation stops at the import boundary: reciprocity, +pairing, and mutual-ownership checks apply only to relationships declared in +this model, even when the local declaration uses `ownership: owned`. + When there's an intuitive **parent** — the entity that owns or contains the other, or sits on the "one" side of a one-to-many — prefer declaring the relationship there (e.g. on `Project`, not `Policy`). It keeps each link in one diff --git a/internal/lint/imports.go b/internal/lint/imports.go index 346422c..639a8ef 100644 --- a/internal/lint/imports.go +++ b/internal/lint/imports.go @@ -86,26 +86,23 @@ type importLoadFailure struct { version string } -// runImports resolves the model's imports, checks every qualified attribute -// type against them, and reports an import nothing references. -// -// modelPath is the path of the model being linted; imports resolve relative to -// its directory. entityScopes are the scopes named by a cross-model reference -// in an entity position (relationship.entity, subtypeOf) — unsupported there, -// but still a real reference: an import bound to one of them is not also -// reported as unreferenced (see reportQualifiedEntityRefs). +// runImports resolves the model's imports, checks every qualified reference +// against them, and reports an import nothing references. // // vendored says the model is a copy whose home is another repository, which // silences the errors its imports list would raise here (see loadImports). -func runImports(modelPath string, m *model.Model, files Files, res *Result, entityScopes map[string]bool, vendored bool) { +func runImports(modelPath string, m *model.Model, files Files, res *Result, vendored bool) { byScope, claimed := loadImports(modelPath, m, files, res, vendored) used := checkQualifiedTypes(m, byScope, claimed, res) + for scope := range checkQualifiedEntities(m, byScope, claimed, res) { + used[scope] = true + } // An unreferenced import is a completeness finding, alongside the unused // enum and the unused glossary term: vocabulary the model declares and // nothing uses. Sharing their category means sharing their promotion under // --completeness error. for _, scope := range sortedMapKeys(byScope) { - if used[scope] || entityScopes[scope] { + if used[scope] { continue } imp := byScope[scope] @@ -326,6 +323,51 @@ func checkQualifiedTypes(m *model.Model, byScope map[string]importedModel, claim return used } +// checkQualifiedEntities resolves qualified relationship targets and subtype +// parents against direct imports. Their imported semantics end at the boundary: +// local reciprocity, ownership, and subtype traversal do not inspect that model. +func checkQualifiedEntities(m *model.Model, byScope map[string]importedModel, claimed map[string]string, res *Result) map[string]bool { + used := map[string]bool{} + check := func(path, ref, kind string) { + match := qualifiedRefRE.FindStringSubmatch(ref) + if match == nil { + return + } + scope, item := match[1], match[2] + imp, ok := byScope[scope] + if !ok { + if _, listed := claimed[scope]; listed { + used[scope] = true + return + } + res.Findings = append(res.Findings, Finding{ + Severity: SeverityError, + Category: CategorySemantic, + Path: path, + Message: fmt.Sprintf("%s %q references the scope %q, which no import binds — add the model that defines %s to `imports:`", kind, ref, scope, item), + }) + return + } + used[scope] = true + if _, ok := imp.model.Entities[item]; !ok { + res.Findings = append(res.Findings, Finding{ + Severity: SeverityError, + Category: CategorySemantic, + Path: path, + Message: fmt.Sprintf("%s %q names no entity %q in %q — check the name, or whether you meant to import a different model", kind, ref, item, imp.path), + }) + } + } + for _, name := range m.EntityNames() { + ent := m.Entities[name] + check(fmt.Sprintf("/entities/%s/subtypeOf", name), ent.SubtypeOf, "subtype parent") + for i, rel := range ent.Relationships { + check(fmt.Sprintf("/entities/%s/relationships/%d/entity", name, i), rel.Entity, "relationship target") + } + } + return used +} + // unresolvedItemMessage explains a qualified type whose scope resolved but // whose item is not there. // @@ -394,64 +436,3 @@ func malformedRefReason(typ string) string { return fmt.Sprintf("the item name %q is not PascalCase", item) } } - -// reportQualifiedEntityRefs reports a cross-model reference in an entity -// position — relationship.entity or subtypeOf. It returns the instance paths -// it reported, so the schema's own finding for the same value is suppressed, -// and the scopes those references named, so an import that exists to support -// one of them is not also reported as unreferenced (runImports) even though no -// attribute type resolves it. -// -// Both fields carry pattern ^[A-Z][A-Za-z0-9]+$, so "payments.Card" already -// fails validation with a message about a pattern. This says what is actually -// wrong, in the spirit of the unsupported-version check. Cross-model entity -// references are deferred, not planned against: ADR-0010 records why. -func reportQualifiedEntityRefs(inst any, res *Result) (reported map[string]bool, scopes map[string]bool) { - reported = map[string]bool{} - scopes = map[string]bool{} - doc, ok := inst.(map[string]any) - if !ok { - return reported, scopes - } - entities, ok := doc["entities"].(map[string]any) - if !ok { - return reported, scopes - } - report := func(path, value string) { - reported[path] = true - scope, _, _ := strings.Cut(value, ".") - scopes[scope] = true - res.Findings = append(res.Findings, Finding{ - Severity: SeverityError, - Category: CategoryStructural, - Path: path, - Message: fmt.Sprintf( - "%q is a cross-model reference, which is not supported in an entity position — only an attribute `type` can be qualified as scope.Name", - value, - ), - }) - } - for _, name := range sortedMapKeys(entities) { - ent, ok := entities[name].(map[string]any) - if !ok { - continue - } - if parent, ok := ent["subtypeOf"].(string); ok && qualifiedRefRE.MatchString(parent) { - report(fmt.Sprintf("/entities/%s/subtypeOf", name), parent) - } - rels, ok := ent["relationships"].([]any) - if !ok { - continue - } - for i, r := range rels { - rel, ok := r.(map[string]any) - if !ok { - continue - } - if target, ok := rel["entity"].(string); ok && qualifiedRefRE.MatchString(target) { - report(fmt.Sprintf("/entities/%s/relationships/%d/entity", name, i), target) - } - } - } - return reported, scopes -} diff --git a/internal/lint/imports_test.go b/internal/lint/imports_test.go index 92aaeb7..af6ec9c 100644 --- a/internal/lint/imports_test.go +++ b/internal/lint/imports_test.go @@ -366,6 +366,64 @@ func TestImports_Resolution(t *testing.T) { } } +// TestADR_0018_QualifiedEntityReferences pins direct-import entity resolution +// while leaving imported relationship and subtype semantics outside this model's +// validation boundary. +func TestADR_0018_QualifiedEntityReferences(t *testing.T) { + t.Parallel() + + const entityPath = "/entities/Visit/relationships/0/entity" + const subtypePath = "/entities/Receipt/subtypeOf" + base := func(target, parent string) string { + return fmt.Sprintf(`kind: DomainModel +version: v1 +imports: + - "./payments.modelith.yaml" +entities: + Visit: + definition: One car's stay in the garage. + relationships: + - entity: %s + cardinality: "1:1" + ownership: owned + Receipt: + definition: A record of a payment. + subtypeOf: %s +`, target, parent) + } + + cases := []struct { + name string + target string + parent string + want []wantFinding + }{ + {name: "direct imported entities resolve", target: "payments.Invoice", parent: "payments.Invoice"}, + { + name: "unbound scope", target: "shipping.Carrier", parent: "payments.Invoice", + want: []wantFinding{{SeverityError, CategorySemantic, entityPath, `references the scope "shipping", which no import binds`}}, + }, + { + name: "missing imported entity", target: "payments.Receipt", parent: "payments.Invoice", + want: []wantFinding{{SeverityError, CategorySemantic, entityPath, `names no entity "Receipt"`}}, + }, + { + name: "missing imported subtype parent", target: "payments.Invoice", parent: "payments.Receipt", + want: []wantFinding{{SeverityError, CategorySemantic, subtypePath, `names no entity "Receipt"`}}, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + res, err := Run(importerPath, []byte(base(tc.target, tc.parent)), fakeFiles{"docs/payments.modelith.yaml": paymentsModel}) + if err != nil { + t.Fatal(err) + } + assertFindings(t, importFindings(res.Findings), tc.want) + }) + } +} + // TestImports_ContainmentUsesTheFileSeam pins that the boundary is judged // against the filesystem the reads go to, not against whatever tree the tests // happen to run in. The same model and the same import resolve or are refused @@ -689,10 +747,10 @@ func (c countingFiles) ReadFile(path string) ([]byte, error) { return c.fakeFiles.ReadFile(path) } -// TestRun_QualifiedEntityReferenceIsDeferred checks the friendly error for a -// cross-model reference in an entity position, and that it replaces — rather -// than joins — the schema's pattern violation and the undefined-entity finding. -func TestRun_QualifiedEntityReferenceIsDeferred(t *testing.T) { +// TestImports_QualifiedEntityReferencesRequireBoundScope checks that entity +// positions use the same direct-import rule as attribute types. A qualified +// target is valid syntax, but its scope must be bound by this model. +func TestImports_QualifiedEntityReferencesRequireBoundScope(t *testing.T) { t.Parallel() cases := []struct { @@ -740,11 +798,11 @@ entities: } } assertFindings(t, at, []wantFinding{{ - SeverityError, CategoryStructural, tc.path, - "is a cross-model reference, which is not supported in an entity position", + SeverityError, CategorySemantic, tc.path, + `references the scope "payments", which no import binds`, }}) if !res.HasBlocking(false) { - t.Error("an unsupported cross-model entity reference must block") + t.Error("an unbound qualified entity reference must block") } if findingWithMessage(res.Findings, "undefined entity") { t.Errorf("one mistake reported twice: %+v", res.Findings) @@ -753,14 +811,8 @@ entities: } } -// TestImports_EntityPositionReferenceCountsAsUsingTheImport pins R2-3: an -// import referenced only from an entity position (subtypeOf or -// relationship.entity) used to get both the "not supported in an entity -// position" error and a "never referenced — drop it" completeness warning for -// the same import — contradictory advice, since dropping the import does not -// fix the unsupported reference. reportQualifiedEntityRefs already knows the -// scope was reached for; runImports must count that as use, the same as an -// attribute type would. +// TestImports_EntityPositionReferenceCountsAsUsingTheImport checks that a valid +// qualified entity reference is enough to use its direct import. func TestImports_EntityPositionReferenceCountsAsUsingTheImport(t *testing.T) { t.Parallel() @@ -778,70 +830,22 @@ entities: if err != nil { t.Fatal(err) } - // Only the entity-position error is expected: no "/imports/0 ... never - // referenced" finding alongside it. - assertFindings(t, importFindings(res.Findings), []wantFinding{{ - SeverityError, CategoryStructural, "/entities/Card/subtypeOf", - "is a cross-model reference, which is not supported in an entity position", - }}) -} - -// TestRun_QualifiedEntityReferenceReportedOnUnsupportedVersion pins R2-4: -// reportQualifiedEntityRefs runs inside runStructural, but used to run only -// after the unsupported-version early return, so a cross-model reference in an -// entity position went unreported on a document whose version this build -// doesn't understand — only the version error surfaced. runSemantic and -// runSubtypes independently skip a value matching the same pattern, trusting -// that reportQualifiedEntityRefs already reported it; an early return that -// skips the call makes that trust false. It must run regardless of whether the -// version is supported. -func TestRun_QualifiedEntityReferenceReportedOnUnsupportedVersion(t *testing.T) { - t.Parallel() - - src := `kind: DomainModel -version: v99 -entities: - Card: - definition: A store card. - subtypeOf: payments.Card - relationships: - - entity: payments.Card - cardinality: "1:1" -` - res, err := Run(importerPath, []byte(src), fakeFiles{}) - if err != nil { - t.Fatal(err) - } - want := []wantFinding{ - {SeverityError, CategoryStructural, "/entities/Card/relationships/0/entity", - "is a cross-model reference, which is not supported in an entity position"}, - {SeverityError, CategoryStructural, "/entities/Card/subtypeOf", - "is a cross-model reference, which is not supported in an entity position"}, - } - var got []Finding - for _, f := range res.Findings { - if f.Path == want[0].path || f.Path == want[1].path { - got = append(got, f) - } - } - assertFindings(t, got, want) - if !res.HasBlocking(false) { - t.Error("an unsupported cross-model entity reference must block even on an unsupported version") - } + // A valid qualified subtype consumes the import and has no entity-position + // error or contradictory unused-import warning. + assertFindings(t, importFindings(res.Findings), nil) } -// TestRun_QualifiedEntityReferenceDoesNotGateTheImportsLayer pins that two -// unrelated mistakes are reported in one run. The cross-model entity reference -// used to count as a structural failure, which skipped the imports layer -// entirely: the broken import below stayed invisible until the subtypeOf was -// fixed, and fixing it produced a second, unannounced round of errors. -func TestRun_QualifiedEntityReferenceDoesNotGateTheImportsLayer(t *testing.T) { +// TestImports_QualifiedEntityReferenceWithMissingImport checks that an import +// load failure speaks for a qualified entity reference in its claimed scope. +// Reporting both a missing file and an unbound scope would send the author to +// fix an import they already declared. +func TestImports_QualifiedEntityReferenceWithMissingImport(t *testing.T) { t.Parallel() src := `kind: DomainModel version: v1 imports: - - "./gone.modelith.yaml" + - {scope: payments, path: ./gone.modelith.yaml} entities: Visit: definition: One car's stay in the garage. @@ -852,8 +856,6 @@ entities: t.Fatal(err) } assertFindings(t, importFindings(res.Findings), []wantFinding{ - {SeverityError, CategoryStructural, "/entities/Visit/subtypeOf", - "is a cross-model reference, which is not supported in an entity position"}, {SeverityError, CategorySemantic, "/imports/0", `import "./gone.modelith.yaml" cannot be read`}, }) diff --git a/internal/lint/lint.go b/internal/lint/lint.go index c317900..9120689 100644 --- a/internal/lint/lint.go +++ b/internal/lint/lint.go @@ -93,7 +93,7 @@ func Run(path string, src []byte, files Files) (*Result, error) { vendored := runProvenance(path, src, res) // Layer 1: structural validation against the JSON Schema. - structuralOK, entityScopes := runStructural(src, res) + structuralOK := runStructural(src, res) // If it does not even parse into our typed model, stop — semantic and // completeness checks need a model to work with. The structural layer has @@ -121,11 +121,9 @@ func Run(path string, src []byte, files Files) (*Result, error) { // Imports resolve only against a document the schema accepted. A scope the // schema already rejected would otherwise bind anyway, and the advice that // follows would tell the author to write syntax that cannot work — the same - // reason the version check gates schema validation. A cross-model reference - // in an entity position is not one of those rejections (see runStructural), - // so it does not take the imports layer down with it. + // reason the version check gates schema validation. if structuralOK { - runImports(path, m, files, res, entityScopes, vendored) + runImports(path, m, files, res, vendored) } runRelationshipShape(m, res) runSubtypes(m, res) @@ -152,14 +150,9 @@ func Structural(data []byte) []Finding { return res.Findings } -// runStructural validates against the JSON Schema. Returns true if the schema -// accepted the document — which the cross-model entity references reported here -// do not affect, since they are a supported-feature limit rather than a shape -// the imports list depends on — plus the scopes those references named, so the -// imports layer can tell an import that exists to support one of them from one -// genuinely unreferenced (runSemantic and runSubtypes rely on every such -// reference having been reported here; see reportQualifiedEntityRefs). -func runStructural(data []byte, res *Result) (ok bool, entityScopes map[string]bool) { +// runStructural validates against the JSON Schema and returns whether it accepted +// the document. +func runStructural(data []byte, res *Result) bool { jsonBytes, err := yaml.YAMLToJSON(data) if err != nil { res.Findings = append(res.Findings, Finding{ @@ -167,7 +160,7 @@ func runStructural(data []byte, res *Result) (ok bool, entityScopes map[string]b Category: CategoryStructural, Message: fmt.Sprintf("not valid YAML: %v", err), }) - return false, nil + return false } inst, err := jsonschema.UnmarshalJSON(bytes.NewReader(jsonBytes)) @@ -177,7 +170,7 @@ func runStructural(data []byte, res *Result) (ok bool, entityScopes map[string]b Category: CategoryStructural, Message: fmt.Sprintf("could not decode document: %v", err), }) - return false, nil + return false } // Dispatch on the declared format version. modelith — not the schema — is @@ -198,13 +191,7 @@ func runStructural(data []byte, res *Result) (ok bool, entityScopes map[string]b Message: fmt.Sprintf("unsupported schema version %q; this modelith supports: %s "+ "(upgrade modelith, or set a supported version)", v, strings.Join(schema.SupportedVersions(), ", ")), }) - // A cross-model entity reference is reported here regardless of - // whether the version is one this build understands: runSemantic and - // runSubtypes skip it on the assumption it was, and an early return - // before this call would leave it unreported instead of just - // unvalidated. - _, entityScopes = reportQualifiedEntityRefs(inst, res) - return false, entityScopes + return false } } } @@ -216,7 +203,7 @@ func runStructural(data []byte, res *Result) (ok bool, entityScopes map[string]b Category: CategoryStructural, Message: fmt.Sprintf("internal: %v", err), }) - return false, nil + return false } // Say what a cross-model entity reference actually is before the schema @@ -225,33 +212,28 @@ func runStructural(data []byte, res *Result) (ok bool, entityScopes map[string]b // counted against the document's structural validity: a broken import in the // same file is an unrelated mistake, and holding the imports layer back // until this one is fixed would hide it. - qualified, entityScopes := reportQualifiedEntityRefs(inst, res) - before := len(res.Findings) if err := sch.Validate(inst); err != nil { if ve, ok := err.(*jsonschema.ValidationError); ok { - collectLeaves(ve, res, qualified) - return len(res.Findings) == before, entityScopes + collectLeaves(ve, res) + return len(res.Findings) == before } res.Findings = append(res.Findings, Finding{ Severity: SeverityError, Category: CategoryStructural, Message: err.Error(), }) - return false, entityScopes + return false } - return true, entityScopes + return true } -func collectLeaves(e *jsonschema.ValidationError, res *Result, skip map[string]bool) { +func collectLeaves(e *jsonschema.ValidationError, res *Result) { if len(e.Causes) == 0 { ptr := "/" + strings.Join(e.InstanceLocation, "/") if ptr == "/" { ptr = "" } - if skip[ptr] { - return - } msg := e.Error() if e.ErrorKind != nil { msg = e.ErrorKind.LocalizedString(printer) @@ -265,7 +247,7 @@ func collectLeaves(e *jsonschema.ValidationError, res *Result, skip map[string]b return } for _, c := range e.Causes { - collectLeaves(c, res, skip) + collectLeaves(c, res) } } @@ -336,9 +318,7 @@ func runSemantic(m *model.Model, res *Result) { for i, rel := range ent.Relationships { switch { case qualifiedRefRE.MatchString(rel.Entity): - // Already reported as an unsupported cross-model reference by - // reportQualifiedEntityRefs; calling it an undefined entity too - // would report one mistake twice. + // Qualified targets are resolved against direct imports by runImports. case !entitySet[rel.Entity]: res.Findings = append(res.Findings, Finding{ Severity: SeverityError, @@ -590,7 +570,7 @@ func runSubtypes(m *model.Model, res *Result) { continue } if qualifiedRefRE.MatchString(parent) { - continue // reported as an unsupported cross-model reference + continue // resolved against a direct import; imported ancestry stops here } if _, ok := m.Entities[parent]; !ok { res.Findings = append(res.Findings, Finding{ @@ -678,6 +658,9 @@ func runReciprocity(m *model.Model, res *Result) { byPair := map[string][]decl{} for _, name := range m.EntityNames() { for i, rel := range m.Entities[name].Relationships { + if qualifiedRefRE.MatchString(rel.Entity) { + continue + } pair := []string{name, rel.Entity} sort.Strings(pair) k := pair[0] + "\x00" + pair[1] @@ -760,6 +743,9 @@ func runReciprocity(m *model.Model, res *Result) { // from one end only resolves it. func runPairing(m *model.Model, res *Result) { for _, g := range model.EdgeGroups(m) { + if qualifiedRefRE.MatchString(g.FirstN) || qualifiedRefRE.MatchString(g.SecondN) { + continue + } if !g.AmbiguousPairing() { continue } diff --git a/internal/lint/lint_test.go b/internal/lint/lint_test.go index 567502e..349e9c9 100644 --- a/internal/lint/lint_test.go +++ b/internal/lint/lint_test.go @@ -133,37 +133,6 @@ entities: } } -// TestRunStructural_UnsupportedVersionReturnsEntityScopes locks down the named -// return runStructural hands back on the unsupported-version early-return -// path (lint.go:195). That line used to declare a new, block-scoped -// entityScopes with `:=`, shadowing the function's named return instead of -// assigning it; the explicit `return false, entityScopes` right after still -// returned the correct (shadowed) value, so the bug was latent, not live — -// but it would have gone silently wrong the moment that return became a bare -// `return`, which is exactly the idiom named returns invite. Calling -// runStructural directly (white-box, same package) is the only way to -// observe this return value: Run() never reaches this path with structuralOK -// true, so nothing downstream currently consumes it. -func TestRunStructural_UnsupportedVersionReturnsEntityScopes(t *testing.T) { - src := ` -kind: DomainModel -version: v99 -entities: - Ticket: - definition: A parking ticket. - subtypeOf: payments.Invoice -` - res := &Result{} - ok, entityScopes := runStructural([]byte(src), res) - if ok { - t.Fatal("expected ok=false for an unsupported version") - } - want := map[string]bool{"payments": true} - if len(entityScopes) != len(want) || !entityScopes["payments"] { - t.Fatalf("expected entityScopes %+v from the entity-position reference, got %+v", want, entityScopes) - } -} - func TestUndefinedRelationshipTargetIsError(t *testing.T) { src := ` kind: DomainModel diff --git a/internal/model/model.go b/internal/model/model.go index 62189af..07de5ac 100644 --- a/internal/model/model.go +++ b/internal/model/model.go @@ -130,8 +130,9 @@ type EnumValue struct { // Entity is a named concept in the domain. type Entity struct { Definition string `json:"definition"` - // SubtypeOf names the entity this one is a kind of (an is-a link). The - // parent's invariants are understood to cover this entity too. + // SubtypeOf names the entity this one is a kind of (an is-a link). It may + // name a local entity or one in a direct import as scope.Entity; imported + // ancestry and invariants are not followed. SubtypeOf string `json:"subtypeOf,omitempty"` Relationships []Relationship `json:"relationships,omitempty"` Attributes []Attribute `json:"attributes,omitempty"` diff --git a/internal/render/markdown/markdown.go b/internal/render/markdown/markdown.go index f242c3e..620a163 100644 --- a/internal/render/markdown/markdown.go +++ b/internal/render/markdown/markdown.go @@ -24,10 +24,10 @@ import ( const generatedBanner = "{/* Generated by `modelith render`. Do not edit by hand; edit the .modelith.yaml source and re-render. */}" var ( - // qualifiedTypeRE matches an attribute type that references an imported item, + // qualifiedReferenceRE matches a reference to an imported item, // "scope.Name". It mirrors the linter's pattern; the renderer only decides // whether to link, never whether the reference resolves. - qualifiedTypeRE = regexp.MustCompile(`^(` + model.ScopeSlug + `)\.([A-Z][A-Za-z0-9]*)$`) + qualifiedReferenceRE = regexp.MustCompile(`^(` + model.ScopeSlug + `)\.([A-Z][A-Za-z0-9]*)$`) // backtickRunRE finds the backtick runs a code span's fence has to clear. backtickRunRE = regexp.MustCompile("`+") ) @@ -159,16 +159,25 @@ func importLinkTarget(impPath, sourceDir, outDir string) string { return filepath.ToSlash(rel) } +// qualifiedReference renders a qualified entity or type as a deep link into the +// imported model's Markdown. A scope no import binds — a lint error — renders as +// written rather than as a link to nowhere. +func qualifiedReference(ref string, targets map[string]string) string { + match := qualifiedReferenceRE.FindStringSubmatch(ref) + if match == nil { + return codeSpan(ref) + } + target, ok := targets[match[1]] + if !ok { + return codeSpan(ref) + } + return fmt.Sprintf("[%s](%s#%s)", ref, linkTarget(target), strings.ToLower(match[2])) +} + // typeCell renders an attribute type for a table cell, linking a "scope.Name" -// reference into the imported model's Markdown. Headings render as "### `Name`", -// so the anchor is the item's name lowercased. A scope no import binds — a lint -// error — renders as written rather than as a link to nowhere. -// -// Each part is escaped for the position it lands in, before the link is -// assembled. Escaping the finished link instead would treat a URL as cell text -// and corrupt the destination. +// reference into the imported model's Markdown. func typeCell(typ string, targets map[string]string) string { - match := qualifiedTypeRE.FindStringSubmatch(typ) + match := qualifiedReferenceRE.FindStringSubmatch(typ) if match == nil { return mdCell(typ) } @@ -176,8 +185,6 @@ func typeCell(typ string, targets map[string]string) string { if !ok { return mdCell(typ) } - // typ matched qualifiedTypeRE, so the label holds nothing a cell or a link - // label reacts to. return fmt.Sprintf("[%s](%s#%s)", typ, linkTarget(target), strings.ToLower(match[2])) } @@ -249,7 +256,7 @@ func renderEntities(b *strings.Builder, m *model.Model, sourceDir, outDir string } if ent.SubtypeOf != "" { - fmt.Fprintf(b, "**Subtype of** `%s`\n\n", ent.SubtypeOf) + fmt.Fprintf(b, "**Subtype of** %s\n\n", qualifiedReference(ent.SubtypeOf, importTargets)) } if kids := subtypes[name]; len(kids) > 0 { quoted := make([]string, len(kids)) @@ -271,7 +278,7 @@ func renderEntities(b *strings.Builder, m *model.Model, sourceDir, outDir string if len(ent.Relationships) > 0 { b.WriteString("**Relationships**\n\n") for _, rel := range ent.Relationships { - parts := []string{fmt.Sprintf("`%s`", rel.Entity), rel.Cardinality} + parts := []string{qualifiedReference(rel.Entity, importTargets), rel.Cardinality} if rel.Symmetric { parts = append(parts, "symmetric") } diff --git a/internal/render/markdown/markdown_test.go b/internal/render/markdown/markdown_test.go index a2a2bd0..146180e 100644 --- a/internal/render/markdown/markdown_test.go +++ b/internal/render/markdown/markdown_test.go @@ -589,6 +589,30 @@ func TestCodeSpan_FencesAroundBackticks(t *testing.T) { } } +// TestRenderEntity_QualifiedEntityReferences links qualified relationship targets +// and subtype parents to the imported entity heading. +func TestRenderEntity_QualifiedEntityReferences(t *testing.T) { + t.Parallel() + m := &model.Model{ + Imports: []model.Import{{Scope: "payments", Path: "../payments/payments.modelith.yaml"}}, + Entities: map[string]model.Entity{ + "Receipt": {Definition: "A receipt.", SubtypeOf: "payments.Invoice"}, + "Visit": {Definition: "A visit.", Relationships: []model.Relationship{{ + Entity: "payments.Invoice", Cardinality: "1:1", Ownership: "owned", + }}}, + }, + } + got := render(m) + for _, want := range []string{ + "**Subtype of** [payments.Invoice](../payments/payments.modelith.md#invoice)\n", + "- [payments.Invoice](../payments/payments.modelith.md#invoice) — 1:1 — owned\n", + } { + if !strings.Contains(got, want) { + t.Errorf("expected %q in:\n%s", want, got) + } + } +} + // TestRenderEntity_SubtypeHierarchy checks that a child names its supertype and // a parent lists its subtypes. func TestRenderEntity_SubtypeHierarchy(t *testing.T) { diff --git a/internal/render/mermaid/mermaid.go b/internal/render/mermaid/mermaid.go index 46f23ec..e5b432b 100644 --- a/internal/render/mermaid/mermaid.go +++ b/internal/render/mermaid/mermaid.go @@ -6,6 +6,7 @@ package mermaid import ( "fmt" "regexp" + "sort" "strconv" "strings" @@ -104,8 +105,9 @@ func (e *edge) merge(from, 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). +// shown in the Markdown table instead. Self-referential relationships and +// qualified subtype parents are the only rows inside an entity block (see +// selfRows and subtypeRow). func ER(m *model.Model) string { var b strings.Builder b.WriteString("erDiagram\n") @@ -113,6 +115,9 @@ func ER(m *model.Model) string { // Declare every entity so unconnected ones still appear. for _, name := range m.EntityNames() { rows := selfRows(name, m.Entities[name].Relationships) + if row := subtypeRow(m.Entities[name].SubtypeOf); row != "" { + rows = append(rows, row) + } if len(rows) == 0 { fmt.Fprintf(&b, " %s {}\n", name) continue @@ -124,6 +129,10 @@ func ER(m *model.Model) string { b.WriteString(" }\n") } + for _, target := range qualifiedTargets(m) { + fmt.Fprintf(&b, " %s {}\n", target) + } + // 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 @@ -187,6 +196,37 @@ func ER(m *model.Model) string { return b.String() } +// qualifiedTargets returns the unique imported entities named by relationships. +// Mermaid accepts the qualified name as an entity name, so its node label remains +// the exact reference the author wrote. +func qualifiedTargets(m *model.Model) []string { + targets := map[string]bool{} + for _, name := range m.EntityNames() { + for _, rel := range m.Entities[name].Relationships { + if qualifiedEntityRE.MatchString(rel.Entity) { + targets[rel.Entity] = true + } + } + } + out := make([]string, 0, len(targets)) + for target := range targets { + out = append(out, target) + } + sort.Strings(out) + return out +} + +// subtypeRow preserves a qualified parent in the local entity's detail rows; +// Mermaid ER has no generalization edge syntax. +func subtypeRow(parent string) string { + if !qualifiedEntityRE.MatchString(parent) { + return "" + } + return fmt.Sprintf("string subtypeOf %q", parent) +} + +var qualifiedEntityRE = regexp.MustCompile(`^` + model.ScopeSlug + `\.[A-Z][A-Za-z0-9]*$`) + // 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 diff --git a/internal/render/mermaid/mermaid_test.go b/internal/render/mermaid/mermaid_test.go index 210b35a..2d49ecd 100644 --- a/internal/render/mermaid/mermaid_test.go +++ b/internal/render/mermaid/mermaid_test.go @@ -50,6 +50,21 @@ func assertGolden(t *testing.T, m *model.Model, goldenPath string) { } } +// TestER_QualifiedTargets renders qualified relationship targets as external +// nodes and qualified subtype parents as ER attribute rows. +func TestER_QualifiedTargets(t *testing.T) { + t.Parallel() + m := &model.Model{Entities: map[string]model.Entity{ + "Receipt": {Definition: "r", SubtypeOf: "payments.Invoice"}, + "Visit": {Definition: "v", Relationships: []model.Relationship{ + {Entity: "payments.Invoice", Cardinality: "1:1", Ownership: "owned"}, + {Entity: "payments.Invoice", Cardinality: "1:n"}, + {Entity: "billing.Account", Cardinality: "n:1"}, + }}, + }} + assertGolden(t, m, "testdata/qualified_targets.golden.mmd") +} + func TestERDeclaresAllEntities(t *testing.T) { m := &model.Model{Entities: map[string]model.Entity{ "Alpha": {Definition: "a"}, diff --git a/internal/render/mermaid/testdata/qualified_targets.golden.mmd b/internal/render/mermaid/testdata/qualified_targets.golden.mmd new file mode 100644 index 0000000..afa3b42 --- /dev/null +++ b/internal/render/mermaid/testdata/qualified_targets.golden.mmd @@ -0,0 +1,10 @@ +erDiagram + Receipt { + string subtypeOf "payments.Invoice" + } + Visit {} + billing.Account {} + payments.Invoice {} + Visit ||--|| payments.Invoice : "" + Visit ||..o{ payments.Invoice : "" + Visit }o..|| billing.Account : "" diff --git a/internal/schema/v1/modelith.schema.json b/internal/schema/v1/modelith.schema.json index 73f7576..a37c4fb 100644 --- a/internal/schema/v1/modelith.schema.json +++ b/internal/schema/v1/modelith.schema.json @@ -152,9 +152,9 @@ "minLength": 1 }, "subtypeOf": { - "description": "Names the entity this one is a kind of — an is-a / generalization link. Must reference a defined entity. The parent's invariants are understood to hold for this entity too.", + "description": "Names the entity this one is a kind of — an is-a / generalization link. Reference a local entity by name or an entity in a direct import as scope.Name. Imported ancestry and invariants are not followed.", "type": "string", - "pattern": "^[A-Z][A-Za-z0-9]+$" + "pattern": "^([A-Z][A-Za-z0-9]+|[a-z][a-z0-9-]*\\.[A-Z][A-Za-z0-9]+)$" }, "relationships": { "description": "How this entity relates to others. A relationship is declared under one entity and names a target; it may be declared from either end (the linter checks that the two views agree). When one entity is intuitively the parent — it owns or contains the other, or is the \"one\" side of a one-to-many — prefer declaring the relationship there.", @@ -211,9 +211,9 @@ "additionalProperties": false, "properties": { "entity": { - "description": "Name of the related entity. Must reference a defined entity.", + "description": "Name of the related entity. Reference a local entity by name or an entity in a direct import as scope.Name.", "type": "string", - "pattern": "^[A-Z][A-Za-z0-9]+$" + "pattern": "^([A-Z][A-Za-z0-9]+|[a-z][a-z0-9-]*\\.[A-Z][A-Za-z0-9]+)$" }, "cardinality": { "description": "Relationship cardinality, written \"left:right\". Each side is a multiplicity: \"1\" (exactly one), \"n\" (zero or more), an exact count like \"2\", or a range like \"0..1\", \"1..n\", \"0..5\". \"1:1\", \"1:n\", \"n:1\", and \"n:n\" are the common shorthands. Example: \"1:2\" is exactly two; \"1:1..n\" is at least one.", diff --git a/project-docs/adr/0018-qualified-entity-references-stop-at-import-boundary.md b/project-docs/adr/0018-qualified-entity-references-stop-at-import-boundary.md new file mode 100644 index 0000000..008bd1b --- /dev/null +++ b/project-docs/adr/0018-qualified-entity-references-stop-at-import-boundary.md @@ -0,0 +1,3 @@ +# Qualified entity references stop at the import boundary + +`relationship.entity` and `subtypeOf` may name a direct import's entity as `scope.Entity`. The importer validates that identity but does not traverse imported subtype ancestry, inherit imported invariants, or reconcile relationship reciprocity and ownership across the boundary. This keeps imported models independent of their importers while preserving explicit, offline direct-import resolution; speculative inherited-invariant behavior is tracked in #47 and qualified prose in #48.