From 7a1bc0f38a45d0e71de881e0c4d2962d59d6f958 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 21:55:34 +0000 Subject: [PATCH] fix(odata): stop create-external-entities duplicating suffixed associations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit mxcli-formula1 §50. Re-running `create external entities from` added two associations every time, without bound: `season_2`, `season_3`, then `season_4`, `season_5`, and so on. One project had reached `season_15` before anyone noticed — `mx check` clean, every test passing, the only symptom duplicate links in Studio Pro's domain model. Association names are unique per module, so the second entity with a `season` nav property is stored as `season_2`. The dedup looks for an association matching the nav property it is about to import, and `season_2` can never match `season` by name. The index that WOULD have matched it is keyed on RemoteParentNavigationProperty — and the modelsdk reader never read the OData association source back, so that index was always empty on the default engine. The write path set the field and the legacy reader read it; only the default read dropped it, so it survived one save and vanished on the next load. assocFromGen now reads Rest$ODataRemoteAssociationSource back (nav properties, navigability, and the four capability flags). The dedup index moves into indexExistingAssociations so it can be tested directly. Not fixed by stripping a trailing _ from names: that would also match a user's genuine `season_2`, and the model already records the true origin. Existing damage is not cleaned up here — those associations are referenced by external-entity access rules, so deleting one raises CE1613. --- .claude/skills/fix-issue.md | 1 + .../modelsdk/association_odata_source_test.go | 143 ++++++++++++++++++ mdl/backend/modelsdk/domainmodel.go | 18 +++ mdl/executor/cmd_contract.go | 50 ++++-- mdl/executor/cmd_contract_reimport_test.go | 109 +++++++++++++ 5 files changed, 307 insertions(+), 14 deletions(-) create mode 100644 mdl/backend/modelsdk/association_odata_source_test.go create mode 100644 mdl/executor/cmd_contract_reimport_test.go diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index 1910e45ae..1068db708 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -449,3 +449,4 @@ extracting `OffsetExpression`/`LimitExpression`. | QUAL002 reports ~40 platform elements (`FileDocument`, `HttpRequest`, `System`) as undocumented, burying the handful of real findings; or a whole document type (pages, workflows, constants, REST services, mappings) is never reported at all no matter how undocumented it is | Two separate causes. (1) **Coverage**: QUAL002 only ever looked at entities and microflows — every other document type was unreachable from Starlark. (2) **The System leak**: `modules.Source` is `"Marketplace …"` for downloaded modules and **empty for System — exactly as it is for the user's own modules**, so the near-universal `WHERE COALESCE(m.Source,'') = ''` filter excludes Marketplace and lets all of System through | `mdl/linter/context.go` (`documentableSources`, `DocumentableElements()`, `notPlatformModule()`, `systemModuleID`), `mdl/linter/starlark.go` (`documentable_elements()` builtin), `.claude/lint-rules/missing_documentation.star` (`_DOC_KINDS`) | **Source alone does not exclude System** — only the sentinel Id `00000000-0000-0000-0000-000000000001` does (`modelsdk/meta.SystemModuleID`). Use `notPlatformModule(alias)` — **every** `LintContext` iterator now routes through it (Entities, Microflows, Pages, Enumerations, Constants, Snippets, Widgets, DatabaseConnections, JavaActions, DocumentableElements and all three `FindUnused` kinds), and `TestIterators_ExcludePlatformModules` fails if a new one is added without it. The leak was not confined to QUAL002: on a blank 9.24 project it inflated the whole run from 8 findings to 60 — CONV001 renaming System booleans, SEC001 demanding access rules on 38 System entities, DESIGN001 splitting `QueuedTask`, MPR003 splitting the System module itself. Adding the predicate can only ever narrow a result set, so the change cannot invent a finding; prove that by diffing full lint output before/after and asserting the ADDED set is empty. **One projection beats N builtins**: a rule that wants "every document" should get one `documentable_elements()` sweep driven by a table, so a new Mendix document type is two rows (one in `documentableSources`, one in `_DOC_KINDS`) rather than another builtin nobody remembers to call. **The documentation column is NOT uniform** — Mendix says `Documentation` for Java actions / REST / mappings / JSON structures and `Description` for everything else; assuming one spelling silently reports the other half as undocumented (a revert control confirmed 5 kinds go dark). **Test the catalog BUILDER, not just the query**: unit tests that INSERT rows directly cannot see that a builder never populates a column, which would flood the user with false positives — an end-to-end `exec` + `lint` on a real `.mpr` is what proved the constant/entity paths actually work. Three tables (`json_structures`, `import_mappings`, `export_mappings`) use `Id INTEGER PRIMARY KEY AUTOINCREMENT` while the rest use `Id TEXT PRIMARY KEY`, so a synthetic string id is a "datatype mismatch" on exactly those. Beware: MDL `COMMENT` and `DOCUMENTATION` are **different fields** on an entity — `CREATE ENTITY … COMMENT 'x'` does not set documentation, `ALTER ENTITY … SET DOCUMENTATION 'x'` does; a test using the wrong one looks exactly like a write-path bug. Tests `mdl/linter/starlark_javaactions_test.go`; controls reproduce for the sweep, each kind list, the doc-column split, Marketplace (both join shapes) and System | | A `LintContext` iterator suddenly yields nothing and the test says "expected ModA entities to be yielded" — no SQL error, no log line, just an empty result | The iterator's query failed (typically `no such column`) and the iterator swallows it: `rows, err := ctx.db.Query(...); if err != nil { return }`. A hand-rolled test double whose schema has drifted from the real catalog view produces exactly this. Hit when platform filtering started reading `modules.Id` and `context_test.go`'s minimal `modules` table had only `(Name, Source)` | `mdl/linter/context.go` (the `if err != nil { return }` in every iterator), `mdl/linter/context_test.go` (`setupModuleFilterDB`) | Read the failure as **"the query broke"**, not "the filter is too strict" — the two are indistinguishable from the assertion text, and the second reading sends you rewriting correct logic. Confirm by running the query by hand against the fixture DB. Test doubles that hand-roll a `CREATE TABLE` instead of using `catalog.NewFromFile` drift silently from the real schema; prefer the real schema builder for anything that joins. The swallowed error is the root problem — an iterator that cannot run its query is not the same as one with no results, and today nothing distinguishes them | | A lint run reports fewer findings than expected, or a rule that clearly should fire reports nothing — and there is no error, no warning, no log line | An iterator's catalog query failed and the failure was swallowed: `if err != nil { return }` inside an `iter.Seq[T]`, which has no error channel. The run looks successful because a linter's whole output is "here is what I found", and "found nothing" is what both a clean project and a dead query produce | `mdl/linter/context.go` (`QueryError`, `recordQueryError`, `QueryErrors`), `mdl/linter/linter.go` (`Linter.QueryErrors`), `cmd/mxcli/cmd_lint.go` (report + exit 1) | Iterators still degrade to "no rows" — one broken query must not take down the run — but they now **record** the failure, and `mxcli lint` prints it and exits 1, because silently passing CI on a run that could not read the model is the worst available outcome. All 34 sites are covered: `Query` + `return`, `Query` + `continue`, bare and inline `rows.Scan` forms, `return unused` (the non-bare return that a naive regex sweep misses — `TestQueryError_AllIteratorsReport` is what caught it), and the reader-backed `ListScheduledEvents`. **Dedupe on iterator+cause**: several rules iterate the same accessor, so one broken view is otherwise reported once per rule. Keying on the iterator alone instead hides a second, distinct failure in the same accessor. When adding an error message with a remedy, **run the remedy** — the first draft here suggested `mxcli lint --refresh`, a flag that does not exist; the shipped message names `.mxcli/catalog.db` and deleting it was verified to clear the error. Tests `mdl/linter/context_queryerror_test.go`; controls: no-op recorder (every iterator goes silent) and a single reverted iterator (only that one) | +| Re-running `create external entities from` adds two more associations every time — `season_2`, `season_3`, then `season_4`, `season_5`, … unbounded. `mx check` is clean, every test passes, and the only symptom is duplicate links in Studio Pro's domain model | The re-import dedup matches an existing association by the OData **nav-property** name, but association names are unique per MODULE, so the second entity with a `season` nav property is stored as `season_2` — which can never match `season`. The nav-property index that would have matched it is keyed on `RemoteParentNavigationProperty`, and the **modelsdk reader never read the OData source back**, so that index was always empty under the default engine. The legacy reader did read it, which is why this only bit the default path | `mdl/backend/modelsdk/domainmodel.go` (`assocFromGen` now reads `*genRest.ODataRemoteAssociationSource`), `mdl/executor/cmd_contract.go` (`indexExistingAssociations`) | **A field that the write path sets and the read path drops survives exactly one save.** Grep both directions when a round-trip matters — `RemoteParentNavigationProperty` appeared in `domainmodel_write.go`, in gen, and in the legacy parser, and was missing only from the modelsdk read, which is invisible if you only grep for the identifier. **Do not "fix" this by stripping a trailing `_` from names**: that heuristic also swallows a user's genuine `season_2`, and the model already records the true origin. Damage is cumulative and pre-existing: a project that ran the import N times carries 2N spurious associations, and they cannot simply be deleted — external-entity access rules reference them, so removing one leaves CE1613 "The selected association no longer exists". Tests `mdl/backend/modelsdk/association_odata_source_test.go` (round-trip, plus a control that a plain association is not stamped as external) and `mdl/executor/cmd_contract_reimport_test.go` (the index, incl. the assertion that the suffixed ones match via the nav index and NOT the name index — otherwise the guard passes for the wrong reason). mxcli-formula1 §50 | diff --git a/mdl/backend/modelsdk/association_odata_source_test.go b/mdl/backend/modelsdk/association_odata_source_test.go new file mode 100644 index 000000000..1d5726799 --- /dev/null +++ b/mdl/backend/modelsdk/association_odata_source_test.go @@ -0,0 +1,143 @@ +// SPDX-License-Identifier: Apache-2.0 + +package modelsdkbackend + +import ( + "testing" + + "github.com/mendixlabs/mxcli/sdk/domainmodel" +) + +// RemoteParentNavigationProperty is the only durable link from an association +// back to the OData navigation property it was generated from, and +// CREATE EXTERNAL ENTITIES dedupes a re-import on it. The write path set it and +// the read path dropped it, so the field survived one save and vanished on the +// next load. +// +// The consequence was unbounded: association names must be unique per module, so +// a second entity with a `season` nav property gets `season_2`. On a re-import +// the dedup looked for an association *named* `season` on that parent, found +// `season_2`, and created `season_4`. Two more every run, clean under +// `mx check`, visible only as duplicate links in Studio Pro. One real project +// reached `season_15` before anyone noticed (mxcli-formula1 §50). +func TestAssociation_ODataSourceRoundTrips(t *testing.T) { + proj := copyFixture(t) + b := New() + if err := b.Connect(proj); err != nil { + t.Fatalf("connect: %v", err) + } + t.Cleanup(func() { _ = b.Disconnect() }) + + mod, err := b.GetModuleByName("MyFirstModule") + if err != nil || mod == nil { + t.Fatalf("GetModuleByName: %v", err) + } + dm, err := b.GetDomainModel(mod.ID) + if err != nil { + t.Fatalf("GetDomainModel: %v", err) + } + + parent := &domainmodel.Entity{Name: "ZzStanding", Persistable: false} + child := &domainmodel.Entity{Name: "ZzSeason", Persistable: false} + if err := b.CreateEntity(dm.ID, parent); err != nil { + t.Fatalf("CreateEntity parent: %v", err) + } + if err := b.CreateEntity(dm.ID, child); err != nil { + t.Fatalf("CreateEntity child: %v", err) + } + + // Named with a suffix on purpose: this is exactly the association the + // re-import failed to recognise, because its name no longer equals the nav + // property it came from. + want := &domainmodel.Association{ + Name: "season_2", + ParentID: parent.ID, + ChildID: child.ID, + Type: "Reference", + Owner: "Default", + Source: "Rest$ODataRemoteAssociationSource", + Navigability2: "ParentToChild", + RemoteParentNavigationProperty: "season", + RemoteChildNavigationProperty: "standings", + CreatableFromParent: true, + UpdatableFromParent: true, + } + if err := b.CreateAssociation(dm.ID, want); err != nil { + t.Fatalf("CreateAssociation: %v", err) + } + + dm2, err := b.GetDomainModel(mod.ID) + if err != nil { + t.Fatalf("GetDomainModel(2): %v", err) + } + var got *domainmodel.Association + for _, a := range dm2.Associations { + if a.Name == "season_2" { + got = a + break + } + } + if got == nil { + t.Fatal("association season_2 not found after reload") + } + + if got.RemoteParentNavigationProperty != "season" { + t.Errorf("RemoteParentNavigationProperty = %q, want %q — a re-import "+ + "cannot recognise this association without it, and will duplicate it", + got.RemoteParentNavigationProperty, "season") + } + if got.Source != "Rest$ODataRemoteAssociationSource" { + t.Errorf("Source = %q, want Rest$ODataRemoteAssociationSource", got.Source) + } + if got.RemoteChildNavigationProperty != "standings" { + t.Errorf("RemoteChildNavigationProperty = %q, want %q", + got.RemoteChildNavigationProperty, "standings") + } + if got.Navigability2 != "ParentToChild" { + t.Errorf("Navigability2 = %q, want ParentToChild", got.Navigability2) + } + if !got.CreatableFromParent || !got.UpdatableFromParent { + t.Errorf("capability flags lost: CreatableFromParent=%v UpdatableFromParent=%v", + got.CreatableFromParent, got.UpdatableFromParent) + } +} + +// A plain association must not acquire an OData source it never had — the read +// has to branch on the stored type, not stamp every association. +func TestAssociation_PlainAssociationHasNoODataSource(t *testing.T) { + proj := copyFixture(t) + b := New() + if err := b.Connect(proj); err != nil { + t.Fatalf("connect: %v", err) + } + t.Cleanup(func() { _ = b.Disconnect() }) + + mod, _ := b.GetModuleByName("MyFirstModule") + dm, err := b.GetDomainModel(mod.ID) + if err != nil { + t.Fatalf("GetDomainModel: %v", err) + } + parent := &domainmodel.Entity{Name: "ZzPlainA", Persistable: true} + child := &domainmodel.Entity{Name: "ZzPlainB", Persistable: true} + _ = b.CreateEntity(dm.ID, parent) + _ = b.CreateEntity(dm.ID, child) + if err := b.CreateAssociation(dm.ID, &domainmodel.Association{ + Name: "ZzPlainB_ZzPlainA", ParentID: child.ID, ChildID: parent.ID, + Type: "Reference", Owner: "Default", + }); err != nil { + t.Fatalf("CreateAssociation: %v", err) + } + + dm2, _ := b.GetDomainModel(mod.ID) + for _, a := range dm2.Associations { + if a.Name != "ZzPlainB_ZzPlainA" { + continue + } + if a.Source != "" || a.RemoteParentNavigationProperty != "" { + t.Errorf("a plain association was stamped as external: Source=%q nav=%q", + a.Source, a.RemoteParentNavigationProperty) + } + return + } + t.Fatal("plain association not found after reload") +} diff --git a/mdl/backend/modelsdk/domainmodel.go b/mdl/backend/modelsdk/domainmodel.go index 23260e7e7..c247975d4 100644 --- a/mdl/backend/modelsdk/domainmodel.go +++ b/mdl/backend/modelsdk/domainmodel.go @@ -472,6 +472,24 @@ func assocFromGen(a *genDm.Association) *domainmodel.Association { out.ParentDeleteBehavior = &domainmodel.DeleteBehavior{Type: domainmodel.DeleteBehaviorType(db.ParentDeleteBehavior())} out.ChildDeleteBehavior = &domainmodel.DeleteBehavior{Type: domainmodel.DeleteBehaviorType(db.ChildDeleteBehavior())} } + + // Read the external (OData) source back. RemoteParentNavigationProperty in + // particular is the only durable link from an association to the OData + // navigation property it was generated from, and CREATE EXTERNAL ENTITIES + // dedupes on it. Dropping it here meant a re-import could not recognise an + // association it had itself created once the name carried a numeric suffix + // (module-wide uniqueness turns a second `season` into `season_2`), so every + // re-run appended two more — unbounded, and invisible to `mx check`. + if src, ok := a.Source().(*genRest.ODataRemoteAssociationSource); ok && src != nil { + out.Source = "Rest$ODataRemoteAssociationSource" + out.Navigability2 = src.Navigability2() + out.RemoteParentNavigationProperty = src.RemoteParentNavigationProperty() + out.RemoteChildNavigationProperty = src.RemoteChildNavigationProperty() + out.CreatableFromParent = src.CreatableFromParent() + out.CreatableFromChild = src.CreatableFromChild() + out.UpdatableFromParent = src.UpdatableFromParent() + out.UpdatableFromChild = src.UpdatableFromChild() + } return out } diff --git a/mdl/executor/cmd_contract.go b/mdl/executor/cmd_contract.go index f733c63d1..d66fc3dde 100644 --- a/mdl/executor/cmd_contract.go +++ b/mdl/executor/cmd_contract.go @@ -1007,20 +1007,7 @@ func createNavigationAssociations( // nav-property key relies on RemoteParentNavigationProperty, which the // legacy read preserves; the modelsdk read does not, so the natural // association name (== nav-property name) is the fallback skip signal. - existingAssocs := make(map[assocKey]bool) - existingNav := make(map[assocKey]bool) - for _, a := range dm.Associations { - // Find parent entity name for this association - for _, ent := range dm.Entities { - if ent.ID == a.ParentID { - existingAssocs[assocKey{ent.Name, a.Name}] = true - if a.RemoteParentNavigationProperty != "" { - existingNav[assocKey{ent.Name, a.RemoteParentNavigationProperty}] = true - } - break - } - } - } + existingAssocs, existingNav := indexExistingAssociations(dm) count := 0 for _, schema := range doc.Schemas { @@ -1134,6 +1121,41 @@ func createNavigationAssociations( return count } +// indexExistingAssociations builds the two lookup tables the re-import dedup +// needs, both keyed by (parent entity name, name): +// +// - byName — the association's own name. +// - byNav — the OData navigation property it was generated from. +// +// byNav is the one that matters, and it is not a convenience. Association names +// are unique per MODULE, so the second entity with a `season` nav property gets +// `season_2`. Such an association can never match itself by name, so without +// byNav a re-import recreates it — computing a fresh suffix each time, two more +// per run, unbounded and invisible to `mx check` (mxcli-formula1 §50). +// +// byNav is only populated for associations that carry +// RemoteParentNavigationProperty, which is why the modelsdk reader must read the +// OData source back; dropping it there silently reduced this to the name match. +func indexExistingAssociations(dm *domainmodel.DomainModel) (byName, byNav map[assocKey]bool) { + byName = make(map[assocKey]bool) + byNav = make(map[assocKey]bool) + parentName := make(map[model.ID]string, len(dm.Entities)) + for _, ent := range dm.Entities { + parentName[ent.ID] = ent.Name + } + for _, a := range dm.Associations { + p, ok := parentName[a.ParentID] + if !ok { + continue + } + byName[assocKey{p, a.Name}] = true + if a.RemoteParentNavigationProperty != "" { + byNav[assocKey{p, a.RemoteParentNavigationProperty}] = true + } + } + return byName, byNav +} + // uniqueAssocName returns a Mendix-safe association name for an OData nav // property. If the requested name collides with an existing entity name OR an // already-created association name, append a numeric suffix. diff --git a/mdl/executor/cmd_contract_reimport_test.go b/mdl/executor/cmd_contract_reimport_test.go new file mode 100644 index 000000000..160078a2b --- /dev/null +++ b/mdl/executor/cmd_contract_reimport_test.go @@ -0,0 +1,109 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/domainmodel" +) + +// f1CachedModel mirrors the shape that made the bug unbounded: three entities +// each with a `season` navigation property. Association names are unique per +// module, so only the first can be called `season`; the others carry a numeric +// suffix and no longer match the nav property they came from. +func f1CachedModel() *domainmodel.DomainModel { + races := &domainmodel.Entity{Name: "Races"} + races.ID = model.ID("e-races") + ds := &domainmodel.Entity{Name: "DriverStandings"} + ds.ID = model.ID("e-ds") + cs := &domainmodel.Entity{Name: "ConstructorStandings"} + cs.ID = model.ID("e-cs") + season := &domainmodel.Entity{Name: "Seasons"} + season.ID = model.ID("e-season") + + mk := func(id, name string, parent model.ID, nav string) *domainmodel.Association { + a := &domainmodel.Association{ + Name: name, ParentID: parent, ChildID: season.ID, + RemoteParentNavigationProperty: nav, + } + a.ID = model.ID(id) + return a + } + + return &domainmodel.DomainModel{ + Entities: []*domainmodel.Entity{races, ds, cs, season}, + Associations: []*domainmodel.Association{ + mk("a1", "season", races.ID, "season"), + mk("a2", "season_2", ds.ID, "season"), + mk("a3", "season_3", cs.ID, "season"), + }, + } +} + +// The dedup must recognise a suffixed association as the nav property it was +// generated from. Matching on the association name alone cannot: `season_2` is +// not `season`, so a re-import recreates it as `season_4`, then `season_6`, for +// ever (mxcli-formula1 §50 — one project reached season_15). +func TestIndexExistingAssociations_SuffixedAssociationMatchesItsNavProperty(t *testing.T) { + byName, byNav := indexExistingAssociations(f1CachedModel()) + + // This is the lookup the import performs, for each parent that has a + // `season` nav property. All three must be recognised as already imported. + for _, parent := range []string{"Races", "DriverStandings", "ConstructorStandings"} { + k := assocKey{parent, "season"} + if !byNav[k] && !byName[k] { + t.Errorf("%s.season is not recognised as already imported — a re-import "+ + "will create a duplicate with a fresh suffix", parent) + } + } + + // Specifically: the two suffixed ones are matched via the nav index, not the + // name index. If that ever inverts, the name match is doing work it cannot + // actually do and the guard above would pass for the wrong reason. + for _, parent := range []string{"DriverStandings", "ConstructorStandings"} { + if byName[assocKey{parent, "season"}] { + t.Errorf("%s: name index unexpectedly holds the unsuffixed name", parent) + } + if !byNav[assocKey{parent, "season"}] { + t.Errorf("%s: nav index missing — this is the entry that prevents the duplicate", parent) + } + } +} + +// Legacy data (and Studio Pro-authored associations) may carry no nav property. +// The name index must still cover the unsuffixed case, so those do not duplicate +// either. +func TestIndexExistingAssociations_FallsBackToNameWhenNavAbsent(t *testing.T) { + dm := f1CachedModel() + for _, a := range dm.Associations { + a.RemoteParentNavigationProperty = "" + } + byName, byNav := indexExistingAssociations(dm) + + if len(byNav) != 0 { + t.Errorf("nav index should be empty when no association carries a nav property: %v", byNav) + } + if !byName[assocKey{"Races", "season"}] { + t.Error("the unsuffixed association is not matched by name either") + } +} + +// An association whose parent is not in this domain model (a cross-module +// association) must not index under an empty parent name, which would collide +// with every other orphan and could suppress a legitimate import. +func TestIndexExistingAssociations_SkipsAssociationsWithUnknownParent(t *testing.T) { + dm := f1CachedModel() + orphan := &domainmodel.Association{ + Name: "elsewhere", ParentID: model.ID("not-in-this-module"), + RemoteParentNavigationProperty: "elsewhere", + } + orphan.ID = model.ID("a9") + dm.Associations = append(dm.Associations, orphan) + + byName, byNav := indexExistingAssociations(dm) + if byName[assocKey{"", "elsewhere"}] || byNav[assocKey{"", "elsewhere"}] { + t.Error("an association with an unresolvable parent was indexed under the empty name") + } +}