fix(odata): stop create-external-entities duplicating suffixed associations (formula1 §50)

.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 `_<n>` 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 |

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")
+}

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
 }
 

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.