From cb6810e1d28b555e08d0dd118759dc45d4e76dc7 Mon Sep 17 00:00:00 2001 From: Ako Date: Fri, 25 Sep 2026 11:16:25 +0000 Subject: [PATCH] fix(pages): refuse a bare attribute reference on every write, ALTER included `alter page FeedbackModule.ShareFeedback_Logo { insert after textBox1 { image zzImg (ImageType: imageUrl, ImageUrl: '{1}', ImageUrlParams: [{1} = ImageB64]) } }` reported "Altered page" and left a project `mx check` could not LOAD (ArgumentNullException setting 'Attribute', 11.13.0). The data view's nanoflow is missing, so nothing qualifies `ImageB64`, and the pluggable widget's template-parameter builder writes the name as given. The refusal from #678 sat in encodePage/encodeSnippet. ALTER PAGE patches the stored BSON in the page mutator and saves it through UpdateRawUnit, so it never reached that check. - The check moves to modelsdk/canon (BareAttributeRefError). The writer's updateUnit and insertUnit call it next to DuplicateElementIDError, so every raw write is covered: ALTER, styling, widget sync, layouts, templates. - encodePage/encodeSnippet keep calling it first, so a CREATE refusal still names the page and not the unit id. - It refuses every bare reference, stored ones too. Across all 374 units of a stock 11.13 project, 73 of 73 AttributeRefs are qualified: 71 in pages, 1 in a snippet, 1 in a page template, none in any other unit type. Studio Pro cannot load a bare one either, so no project it saved can hold one. Real run on a copy of that project: the ALTER above is refused, naming "ImageB64" and zzImg's path, and no unit changes. The qualified form, `set Title`, a qualified textbox insert and a Class change all apply, and `mxcli docker check` reports 0 errors. Control: with the writer guard stubbed, both new tests fail with the reported symptom. Co-Authored-By: Claude Opus 5.5 --- .../skills/fix-issue/findings/modelsdk.jsonl | 1 + .../bug-patterns/unloadable-model-writes.md | 6 +- .../alter-page-bare-attributeref-refused.mdl | 57 +++++++++ .../modelsdk/page_bare_attributeref.go | 59 --------- .../modelsdk/page_bare_attributeref_test.go | 47 -------- .../page_mutator_bare_attributeref_test.go | 113 ++++++++++++++++++ mdl/backend/modelsdk/page_write.go | 7 +- mdl/backend/modelsdk/snippet_write.go | 5 +- mdl/executor/cmd_pages_builder.go | 4 +- modelsdk/canon/attributeref.go | 96 +++++++++++++++ modelsdk/canon/attributeref_test.go | 49 ++++++++ modelsdk/mpr/writer_bare_attributeref_test.go | 95 +++++++++++++++ modelsdk/mpr/writer_core.go | 9 ++ 13 files changed, 434 insertions(+), 114 deletions(-) create mode 100644 mdl-examples/bug-tests/alter-page-bare-attributeref-refused.mdl delete mode 100644 mdl/backend/modelsdk/page_bare_attributeref.go delete mode 100644 mdl/backend/modelsdk/page_bare_attributeref_test.go create mode 100644 mdl/backend/modelsdk/page_mutator_bare_attributeref_test.go create mode 100644 modelsdk/canon/attributeref.go create mode 100644 modelsdk/canon/attributeref_test.go create mode 100644 modelsdk/mpr/writer_bare_attributeref_test.go diff --git a/.claude/skills/fix-issue/findings/modelsdk.jsonl b/.claude/skills/fix-issue/findings/modelsdk.jsonl index 29e35108d3..62b23166c7 100644 --- a/.claude/skills/fix-issue/findings/modelsdk.jsonl +++ b/.claude/skills/fix-issue/findings/modelsdk.jsonl @@ -21,3 +21,4 @@ {"area": "docs / modelsdk/mpr", "date": "2026-09-22", "symptom": "The MPR reference pages' \"Unit Types\" tables mapped BSON `$Type` to document kinds, and 15 of the rows named a spelling no unit carries: `Pages$Page`/`Pages$Layout`/`Pages$Snippet`/`Pages$BuildingBlock` (real units say `Forms$*`), and docs/05-mdl-specification/10-bson-mapping.md lowercased eleven more (`microflows$microflow`, `pages$page`, `security$ProjectSecurity`…). It also listed `CustomWidgets$customwidget` as a document type. Found while fixing mendixlabs/mxcli#1072, filed and fixed separately.", "cause": "The tables were written from the TypeScript SDK's QUALIFIED names rather than the storage names Mendix writes — the same split CLAUDE.md documents for `ShowPageAction`/`ShowFormAction`, never applied here. `CustomWidgets$CustomWidget` is a widget element inside a page's tree (mdl/catalog/builder_widget_refs.go), never a unit, so that row was removed rather than corrected.", "file": "`docs-site/src/internals/mpr-format.md`, `docs/05-mdl-specification/10-bson-mapping.md`; test `modelsdk/mpr/docs_schema_test.go` (TestDocumentedUnitTypesUseStorageNames)", "insight": "Measuring the real set is one command and settles the whole table at once: decode every `mprcontents/*/*/*.mxunit` (and every v1 `Unit.Contents` blob) and count `$Type` — 28 distinct values across a blank 11.6.6 app and a 9.24.30 one. Do NOT try to verify rows one at a time against gen, which carries BOTH spellings: `model/types.go` defines `DocumentTypePage = \"Pages$Page\"` and mdl/catalog/builder_xpath.go defensively matches `Forms$Page` AND `Pages$Page`, so grepping the codebase 'confirms' the wrong name. The fixture is the arbiter; the codebase is not. The test rule that makes this checkable without a maintenance burden keys on the LOCAL name after the `$`, case-insensitively: a fixture cannot prove a type ABSENT (a blank project has no business-event service), so demanding every documented type be present would fail correct rows — but when the fixture has a type with the same local name, the documented row must equal it exactly. That catches all four `Pages$` rows and all eleven lowercase ones with zero false positives. Its stated limit is real and cost a manual fix: a row whose local name appears nowhere in the fixtures is not checked at all, which is how `CustomWidgets$customwidget` slipped past and had to be removed by hand. One editing trap, not a Mendix one: anchoring a section replacement on `'---'` matches a markdown TABLE SEPARATOR (`|---|---|`) long before the horizontal rule you meant — the edit silently no-ops on the table you were replacing. Anchor on `'\\n---\\n'`.", "refs": ["mendixlabs/mxcli#1072"]} {"area": "modelsdk/canon", "date": "2026-09-23", "symptom": "The storage-GUID write guard (`canon.StorageGUIDChanges`) stopped refusing the MOVE ENTITY data loss it had exposed (ako/mxcli#503). With MoveEntity's carries removed, moving an association's TO side re-minted the in-place converted cross-association's GUID and the write went through silently, where the issue records a refusal.", "cause": "`sameMember` (added in 86927852 to stop the guard refusing transplant mis-pairings) required an equal `$Type` as well as an equal `Name`. MoveEntity converts `DomainModels$Association` to `DomainModels$CrossAssociation` IN PLACE, keeping `$ID` and `Name`, so the type clause made the guard skip the pair. The type clause excluded nothing the transplant can produce: `pairDoc` stops at a `$Type` mismatch (TestTransplantIgnoresMismatchedTypes).", "file": "`modelsdk/canon/storageguid.go` (`sameMember`, the note above `GUIDChange`)", "insight": "Before adding a clause to an identity test that sits on an approximate pairing, ask what error of THAT pairing the clause excludes. The transplant only mis-pairs same-type, different-name elements, so `$Type` excluded none of its errors. Its only effect was to exclude the one writer that keeps an `$ID` across a type change deliberately. Rule now: Name when both sides have one; `$Type` only when neither does; a pair with a name on one side only is not a match. How the gap was found: stub the three MoveEntity carries on main and run TestIssue503. The child-side case returned no error where the issue quotes a refusal. That mismatch between the recorded refusal and the observed silence was the tell. A guard's quiet is not evidence of a clean write, so a guard's comment must list every hole it leaves; this one listed only renames. Controls: (1) the new canon test fails on the old `sameMember` with `got 0 change(s)`; (2) with the MoveEntity carries stubbed the child-side move is refused again with the issue's exact message, and the parent-side move still goes through, because the moved element changes unit and pairs with nothing (a documented hole); (3) the 86927852 false positive does not return: `marketplace install --file mx-modules/BusinessEvents_3.12.0.mpk` into a copy of testdata/expr-checker, then `create or modify persistent entity BusinessEvents.PublishedBusinessEvent (EventId: long)` is accepted, while a build with an `$ID`-only rule refuses it (EventId paired with a removed attribute). The existing table case `DifferentType_NotAChange` pinned the wrong decision with the justification 'nothing authors this today', which was false the day it was written. Grep for the writers (`SetID(x.ID())` next to `New()`) before claiming nothing authors a shape.", "refs": ["ako/mxcli#503", "mendixlabs/mxcli#1119"]} {"area":"modelsdk/codec","date":"2026-09-25","symptom":"A compound design property (Atlas `Spacing` → `margin-bottom`, or a multiSelect toggle group) writes its `Forms$CompoundDesignPropertyValue.Properties` list with BSON array marker 3 where Studio Pro writes 2. `check`, `exec` and `mx check` all pass; a describe → exec round trip of FeedbackModule.ShareFeedback (Feedback v4.0.2, 11.13.0) turned every nested marker-2 list into 3","cause":"The codec picks a PartList's marker from the CHILD element's `$Type` only (`partListMarker` → `lookupListMarker`). The nested list and the enclosing `Forms$Appearance.DesignProperties` list (marker 3) both hold `Forms$DesignPropertyValue`, so no `RegisterListMarker` on the child type could tell them apart and both fell to the default 3","file":"`modelsdk/codec/defaults.go` (`RegisterPropertyListMarker`), `modelsdk/codec/encoder.go` (`propertyListMarker`), `mdl/backend/modelsdk/widget_write.go` (init)","insight":"When one child `$Type` sits in two lists with different markers, the marker belongs to the owner+key, not the child: `RegisterPropertyListMarker(owner, key, m)` is consulted first, for an empty list too and in the selective-rebuild path. Establish the marker by counting Studio Pro-authored BSON before changing anything: walking every mxunit gave Compound.Properties 373/373 marker 2 and Appearance.DesignProperties 1821/1821 marker 3 across pages, layouts, building blocks and page templates. Count per (owner $Type, key) — a flat grep of `Properties [marker=2]` in ndsl also matches unrelated lists. Pages, snippets and layouts share `newAppearance`, so one registration covers all. Test `TestAppearanceCompoundDesignPropertyMarkers`; bug-test `mdl-examples/bug-tests/compound-design-property-marker.mdl`. Same class, not fixed: the selective-rebuild branch of `encodeEntry` still hard-codes 3 for lists with no owner registration, ignoring a child-type `RegisterListMarker`","refs":["#668"]} +{"area":"modelsdk/mpr","date":"2026-09-25","symptom":"`alter page FeedbackModule.ShareFeedback_Logo { insert after textBox1 { image zzImg (ImageType: imageUrl, ImageUrl: '{1}', ImageUrlParams: [{1} = ImageB64]) } }` (data view over a nanoflow the project lacks, 11.13.0) reported \"Altered page\"; `mxcli docker check` then could not LOAD the project: ArgumentNullException setting 'Attribute' of an Attribute in a Page","cause":"The bare-AttributeRef refusal (#678) lived in encodePage/encodeSnippet only. ALTER PAGE patches the stored BSON in pagemutator and saves via UpdateRawUnit, never passing the encoder; with no entity in scope the pluggable-widget template-parameter builder (widgetobj) writes the name as given, so DomainModels$AttributeRef{Attribute:\"ImageB64\"} reached disk","file":"modelsdk/canon/attributeref.go; modelsdk/mpr/writer_core.go (updateUnit, insertUnit)","insight":"A guard placed in one encoder covers one write path; the page family has at least four (encodePage/Snippet, pagemutator Save, widget sync apply, layout/template raw writes). Put an unloadable-shape refusal at the writer beside DuplicateElementIDError, as that one already argued. Measured before refusing stored refs too: 73 of 73 AttributeRefs across all 374 units of a stock 11.13 project are qualified (71 page, 1 snippet, 1 page template, none elsewhere) — a stored bare one cannot have come from Studio Pro, so refusing ALL bare refs (not only new ones) blocks nothing legitimate. The textbox path does NOT reproduce it: attributeRefToGen nulls a bare name (a silent binding drop instead); the pluggable/column template builders are the ones that write it verbatim. The test goes through the real mutator + writer on the expr-checker fixture (InsertColumns with a bare CaptionParams ref).","refs":["#678"]} diff --git a/docs-wiki/bug-patterns/unloadable-model-writes.md b/docs-wiki/bug-patterns/unloadable-model-writes.md index be793e7122..4997197338 100644 --- a/docs-wiki/bug-patterns/unloadable-model-writes.md +++ b/docs-wiki/bug-patterns/unloadable-model-writes.md @@ -33,8 +33,10 @@ project rather than one page, and because the diagnostic is a stack trace. that points at the wrong thing. Mendix reconstructs each stored property into a typed identifier as it loads, and a value that cannot be parsed into that type takes the loader down. The shapes seen so far: a one-qualifier member name -written where an attribute reference is expected (an attribute is bare or -`Module.Entity.Attribute`, never `Module.Name`); an unqualified entity name in a +written where an attribute reference is expected (a stored +`DomainModels$AttributeRef` is `Module.Entity.Attribute` and nothing else — a +bare name fails to load as surely as `Module.Name`, and the writer now refuses +both for every unit, ALTER's raw patches included); an unqualified entity name in a generalization; a literal string where the property is a `ConstantIdentifier`; an empty `DestinationEntity`; an index column pointing at a GUID that no longer exists; a sequence flow dangling from a `break`; an association whose `ParentPointer` diff --git a/mdl-examples/bug-tests/alter-page-bare-attributeref-refused.mdl b/mdl-examples/bug-tests/alter-page-bare-attributeref-refused.mdl new file mode 100644 index 0000000000..1a44751049 --- /dev/null +++ b/mdl-examples/bug-tests/alter-page-bare-attributeref-refused.mdl @@ -0,0 +1,57 @@ +-- ============================================================================ +-- ALTER PAGE refuses to store a bare attribute reference +-- ============================================================================ +-- +-- Symptom: on a copy of a real 11.13.0 project, +-- alter page FeedbackModule.ShareFeedback_Logo { +-- insert after textBox1 { image zzImg (ImageType: imageUrl, ImageUrl: '{1}', +-- ImageUrlParams: [{1} = ImageB64]) } +-- } +-- reported "Altered page", and `mxcli docker check` then could not LOAD the +-- project: ArgumentNullException setting 'Attribute' of an Attribute in a Page. +-- The data view is sourced by a nanoflow the module does not ship, so there is +-- no entity to qualify `ImageB64` against, and the image's template parameter +-- was written as DomainModels$AttributeRef { Attribute: "ImageB64" }. +-- +-- Cause: CREATE PAGE refused a bare reference in its encoder (encodePage), but +-- ALTER PAGE patches the stored BSON tree in the page mutator and saves it with +-- UpdateRawUnit, which never passes that encoder. +-- +-- Fix: the refusal moved to the write choke point (modelsdk/mpr updateUnit and +-- insertUnit, canon.BareAttributeRefError), beside the duplicate-$ID guard, so +-- every raw write of any unit is covered. +-- +-- Verify: exec this script → "Altered page"; `mxcli docker check` → 0 errors. +-- Change the inserted image's parameter to `{1} = Subject` → the ALTER is +-- refused ("attribute reference not qualified as Module.Entity.Attribute … +-- "Subject" at …/imgQualified…"), and the stored page is unchanged. +-- ============================================================================ + +create entity MyFirstModule.AlterBareDraft ( + Subject: String(200), + PictureUrl: String(400) +); +/ + +@excluded +create or modify page MyFirstModule.AlterBareDraft_Example +( Title: 'Draft (example)', Layout: Atlas_Core.Atlas_Default ) +{ + dataview dv (DataSource: nanoflow MyFirstModule.DS_MissingAlterBareDraft) { + textbox txtSubject (Label: 'Subject', Attribute: MyFirstModule.AlterBareDraft.Subject) + } +} +/ + +-- Inside the data view there is no resolvable entity: only a qualified +-- reference can be stored. +alter page MyFirstModule.AlterBareDraft_Example { + insert after txtSubject { + image imgQualified ( + ImageType: imageUrl, + ImageUrl: '{1}', + ImageUrlParams: [{1} = MyFirstModule.AlterBareDraft.PictureUrl] + ) + } +}; +/ diff --git a/mdl/backend/modelsdk/page_bare_attributeref.go b/mdl/backend/modelsdk/page_bare_attributeref.go deleted file mode 100644 index ead2078258..0000000000 --- a/mdl/backend/modelsdk/page_bare_attributeref.go +++ /dev/null @@ -1,59 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 - -package modelsdkbackend - -import ( - "fmt" - "strings" - - "go.mongodb.org/mongo-driver/bson" -) - -// refuseBareAttributeRefs refuses a page or snippet whose encoded form holds a -// DomainModels$AttributeRef that is not Module.Entity.Attribute. -// -// Mendix rebuilds each stored reference into a typed identifier as it loads, -// and an attribute that does not parse as one takes the loader down before any -// validation runs: a bare `ImageB64` image parameter left `mx check` unable to -// load the project (ArgumentNullException setting 'Attribute', 11.13.0) — the -// page was excluded, which does not help, as loading is not validating. Studio -// Pro qualifies every one (72 of 72 across a stock project's pages, snippets -// and layouts). A bare name reaches here when nothing could qualify it — inside -// a data container whose flow the project lacks — so this is the last line -// under the check that refuses it first (checkUnscopedBindings). -func refuseBareAttributeRefs(contents []byte) error { - var bad []string - var walk func(v bson.RawValue, path string) - walk = func(v bson.RawValue, path string) { - switch v.Type { - case bson.TypeEmbeddedDocument: - doc := v.Document() - if t, ok := doc.Lookup("$Type").StringValueOK(); ok && t == "DomainModels$AttributeRef" { - if a, ok := doc.Lookup("Attribute").StringValueOK(); ok && a != "" && strings.Count(a, ".") < 2 { - bad = append(bad, fmt.Sprintf("%q at %s", a, path)) - } - } - name, _ := doc.Lookup("Name").StringValueOK() - elems, _ := doc.Elements() - for _, e := range elems { - p := path + "/" + e.Key() - if name != "" { - p = path + "/" + name + "." + e.Key() - } - walk(e.Value(), p) - } - case bson.TypeArray: - vals, _ := v.Array().Values() - for _, x := range vals { - walk(x, path) - } - } - } - walk(bson.RawValue{Type: bson.TypeEmbeddedDocument, Value: contents}, "") - if len(bad) == 0 { - return nil - } - return fmt.Errorf("attribute reference not qualified as Module.Entity.Attribute — Mendix cannot load a "+ - "project holding one, so it is not written: %s. Qualify it in the script; inside a data container "+ - "whose flow the project lacks there is no entity to resolve a bare name against", strings.Join(bad, "; ")) -} diff --git a/mdl/backend/modelsdk/page_bare_attributeref_test.go b/mdl/backend/modelsdk/page_bare_attributeref_test.go deleted file mode 100644 index 83c2c2f001..0000000000 --- a/mdl/backend/modelsdk/page_bare_attributeref_test.go +++ /dev/null @@ -1,47 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 - -package modelsdkbackend - -import ( - "strings" - "testing" - - "go.mongodb.org/mongo-driver/bson" -) - -// A DomainModels$AttributeRef whose Attribute is not Module.Entity.Attr makes -// the project unloadable: an image URL parameter written as a bare `ImageB64` -// (Feedback v4.0.2's ShareFeedback_Logo, under a data view whose flow the -// project lacks) left `mx check` unable to LOAD the project — -// ArgumentNullException setting 'Attribute', Mendix 11.13.0 — while the page -// was excluded. Studio Pro qualifies every one: 72 of 72 AttributeRefs across -// that project's pages, snippets and layouts. So the writer refuses the bare -// form, naming it, instead of storing it. -func TestRefuseBareAttributeRefs(t *testing.T) { - attrRef := func(a string) bson.D { - return bson.D{{Key: "$Type", Value: "DomainModels$AttributeRef"}, {Key: "Attribute", Value: a}, {Key: "EntityRef", Value: nil}} - } - doc := func(a string) []byte { - b, err := bson.Marshal(bson.D{ - {Key: "$Type", Value: "Forms$Page"}, - {Key: "Widgets", Value: bson.A{int32(2), bson.D{ - {Key: "$Type", Value: "CustomWidgets$CustomWidget"}, - {Key: "Name", Value: "image1"}, - {Key: "Params", Value: bson.A{int32(2), bson.D{{Key: "AttributeRef", Value: attrRef(a)}}}}, - }}}, - }) - if err != nil { - t.Fatal(err) - } - return b - } - err := refuseBareAttributeRefs(doc("ImageB64")) - if err == nil || !strings.Contains(err.Error(), "ImageB64") { - t.Fatalf("a bare attribute reference must be refused, naming it; got %v", err) - } - for _, ok := range []string{"FeedbackModule.Feedback.ImageB64", ""} { - if err := refuseBareAttributeRefs(doc(ok)); err != nil { - t.Errorf("%q must be accepted: %v", ok, err) - } - } -} diff --git a/mdl/backend/modelsdk/page_mutator_bare_attributeref_test.go b/mdl/backend/modelsdk/page_mutator_bare_attributeref_test.go new file mode 100644 index 0000000000..5e2ecdb607 --- /dev/null +++ b/mdl/backend/modelsdk/page_mutator_bare_attributeref_test.go @@ -0,0 +1,113 @@ +// SPDX-License-Identifier: Apache-2.0 + +package modelsdkbackend + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/backend" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/pages" +) + +// ALTER PAGE does not go through encodePage: the page mutator patches the stored +// BSON tree and saves the bytes with UpdateRawUnit. A bare attribute in a column +// template parameter (what an ALTER inside a data container with no resolvable +// entity produces — the column builder writes the name as given) reached disk +// that way, and Mendix cannot LOAD a project holding one (ArgumentNullException +// setting 'Attribute', 11.13.0). The same statement, run on a copy of a real +// project, left `mx check` unable to open it. +// +// So the refusal sits at the writer, and this test goes through the real +// mutator and the real writer — the wiring is what came undone. + +func openAccountOverviewMutator(t *testing.T) (*Backend, backend.PageMutator, model.ID) { + t.Helper() + proj := copyFixture(t) + b := New() + if err := b.Connect(proj); err != nil { + t.Fatalf("connect: %v", err) + } + t.Cleanup(func() { _ = b.Disconnect() }) + ps, err := b.ListPages() + if err != nil { + t.Fatalf("ListPages: %v", err) + } + var id model.ID + for _, p := range ps { + if p.Name == "Account_Overview" { + id = p.ID + } + } + if id == "" { + t.Fatal("fixture has no Account_Overview page") + } + m, err := b.OpenPageForMutation(id) + if err != nil { + t.Fatalf("OpenPageForMutation: %v", err) + } + return b, m, id +} + +func captionColumn(attr string) *backend.DataGridColumnSpec { + return &backend.DataGridColumnSpec{ + Caption: "{1}", + CaptionParams: []*pages.ClientTemplateParameter{{ + BaseElement: model.BaseElement{ID: model.ID("0d4b8c1e-1111-4a1a-9a1a-111111111111")}, + AttributeRef: attr, + }}, + ShowContentAs: "dynamicText", + Content: "x", + } +} + +func TestAlterPageSaveRefusesABareAttributeRef(t *testing.T) { + b, m, id := openAccountOverviewMutator(t) + before, err := b.GetRawUnitBytes(id) + if err != nil { + t.Fatalf("read stored page: %v", err) + } + + if err := m.InsertColumns("dataGrid21", "WebServiceUser", backend.InsertPosition("after"), + []*backend.DataGridColumnSpec{captionColumn("FullName")}); err != nil { + t.Fatalf("InsertColumns: %v", err) + } + err = m.Save() + if err == nil { + t.Fatal("ALTER saved a page holding a bare attribute reference; Mendix cannot load that project") + } + for _, want := range []string{`"FullName"`, "Module.Entity.Attribute"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("refusal does not name %s: %v", want, err) + } + } + + after, err := b.GetRawUnitBytes(id) + if err != nil { + t.Fatalf("read page after refusal: %v", err) + } + if string(after) != string(before) { + t.Error("the refused ALTER still changed the stored page") + } +} + +func TestAlterPageSaveAcceptsAQualifiedAttributeRef(t *testing.T) { + // The control: the same column, qualified, is an ordinary ALTER. + b, m, id := openAccountOverviewMutator(t) + before, _ := b.GetRawUnitBytes(id) + if err := m.InsertColumns("dataGrid21", "WebServiceUser", backend.InsertPosition("after"), + []*backend.DataGridColumnSpec{captionColumn("Administration.Account.FullName")}); err != nil { + t.Fatalf("InsertColumns: %v", err) + } + if err := m.Save(); err != nil { + t.Fatalf("a qualified attribute reference was refused: %v", err) + } + after, _ := b.GetRawUnitBytes(id) + if string(after) == string(before) { + t.Fatal("control did not write: the accepted ALTER must reach the stored page") + } + if !strings.Contains(string(after), "Administration.Account.FullName") { + t.Error("the qualified reference is not in the stored page") + } +} diff --git a/mdl/backend/modelsdk/page_write.go b/mdl/backend/modelsdk/page_write.go index 6c15d5b803..ed2a15d413 100644 --- a/mdl/backend/modelsdk/page_write.go +++ b/mdl/backend/modelsdk/page_write.go @@ -11,6 +11,7 @@ import ( "github.com/mendixlabs/mxcli/mdl/backend/bsonnav" "github.com/mendixlabs/mxcli/mdl/types" "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/modelsdk/canon" "github.com/mendixlabs/mxcli/modelsdk/codec" "github.com/mendixlabs/mxcli/modelsdk/element" genDT "github.com/mendixlabs/mxcli/modelsdk/gen/datatypes" @@ -260,8 +261,10 @@ func encodePage(page *pages.Page, pv *types.ProjectVersion, carry func(*genPg.Pa if err != nil { return nil, err } - if err := refuseBareAttributeRefs(contents); err != nil { - return nil, fmt.Errorf("page %q: %w", page.Name, err) + // Also refused at the writer (canon/attributeref.go); checked here first so + // the refusal names the page rather than its unit id. + if err := canon.BareAttributeRefError(fmt.Sprintf("page %q", page.Name), contents); err != nil { + return nil, err } return contents, nil } diff --git a/mdl/backend/modelsdk/snippet_write.go b/mdl/backend/modelsdk/snippet_write.go index 2921ad0025..3545aecbe4 100644 --- a/mdl/backend/modelsdk/snippet_write.go +++ b/mdl/backend/modelsdk/snippet_write.go @@ -7,6 +7,7 @@ import ( "github.com/mendixlabs/mxcli/mdl/types" "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/modelsdk/canon" "github.com/mendixlabs/mxcli/modelsdk/codec" "github.com/mendixlabs/mxcli/modelsdk/element" genPg "github.com/mendixlabs/mxcli/modelsdk/gen/pages" @@ -37,8 +38,8 @@ func encodeSnippet(snippet *pages.Snippet, pv *types.ProjectVersion) ([]byte, er if err != nil { return nil, err } - if err := refuseBareAttributeRefs(contents); err != nil { // see page_bare_attributeref.go - return nil, fmt.Errorf("snippet %q: %w", snippet.Name, err) + if err := canon.BareAttributeRefError(fmt.Sprintf("snippet %q", snippet.Name), contents); err != nil { // see encodePage + return nil, err } return contents, nil } diff --git a/mdl/executor/cmd_pages_builder.go b/mdl/executor/cmd_pages_builder.go index 77e70dc33a..35b102b570 100644 --- a/mdl/executor/cmd_pages_builder.go +++ b/mdl/executor/cmd_pages_builder.go @@ -86,8 +86,8 @@ type pageBuilder struct { // be qualified, and one written bare made `mx check` fail to LOAD the // project (ArgumentNullException setting 'Attribute', Mendix 11.13.0). // DESCRIBE writes those bindings qualified there, the check refuses a bare - // one (checkUnscopedBindings), and the page writer refuses any bare - // attribute reference as a last line (refuseBareAttributeRefs). + // one (unscopedBindings), and the writer refuses any bare attribute + // reference as a last line, ALTER included (canon.BareAttributeRefError). tolerateDanglingRefs bool // Local page/snippet variables (Variables: { $name: Type = 'default' }). diff --git a/modelsdk/canon/attributeref.go b/modelsdk/canon/attributeref.go new file mode 100644 index 0000000000..2d3a7e9907 --- /dev/null +++ b/modelsdk/canon/attributeref.go @@ -0,0 +1,96 @@ +// SPDX-License-Identifier: Apache-2.0 + +package canon + +import ( + "fmt" + "strings" + + "go.mongodb.org/mongo-driver/v2/bson" +) + +// A DomainModels$AttributeRef whose Attribute is not Module.Entity.Attribute +// cannot be LOADED. Mendix rebuilds each stored reference into a typed +// identifier as it reads the unit, and a name that does not parse as one takes +// the loader down before any validation runs: +// +// InvalidOperationException: An error occurred when trying to set the +// 'Attribute' property of a Attribute in a Page with ID ... +// ---> ArgumentNullException: Value cannot be null. (Parameter 'value') +// +// (`mx check`, Mendix 11.13.0). Excluding the document does not help — loading +// is not validating. Studio Pro qualifies every one: 73 of 73 across all 374 +// units of a stock 11.13 project (71 in pages, one each in a snippet and a +// page template; no other unit type holds one). +// +// This sits at the write choke point for the same reason DuplicateElementIDError +// does. The page and snippet encoders refused a bare reference, but ALTER PAGE +// patches the stored tree and saves it through UpdateRawUnit, and a pluggable +// widget's template parameter inside a data container with no resolvable entity +// reached disk bare that way. Any raw write can. +// +// It refuses every bare reference in the unit, stored or new. A stored one +// cannot have come from Studio Pro, which cannot load it either; writing it back +// keeps the project unloadable, and the message names it so the statement that +// rewrites the unit can drop or qualify it. + +// BareAttributeRefs names every DomainModels$AttributeRef in raw whose +// Attribute is non-empty and not Module.Entity.Attribute, with where it sits +// (the nearest named element's Name, then the property path). An empty +// Attribute is an unbound slot and is not reported. A document that cannot be +// read yields nothing, as DuplicateElementIDs does. +func BareAttributeRefs(raw []byte) []string { + var bad []string + var walk func(v bson.RawValue, path string) + walk = func(v bson.RawValue, path string) { + switch v.Type { + case bson.TypeEmbeddedDocument: + doc, ok := v.DocumentOK() + if !ok { + return + } + if t, ok := doc.Lookup("$Type").StringValueOK(); ok && t == "DomainModels$AttributeRef" { + if a, ok := doc.Lookup("Attribute").StringValueOK(); ok && a != "" && strings.Count(a, ".") < 2 { + bad = append(bad, fmt.Sprintf("%q at %s", a, path)) + } + } + name, _ := doc.Lookup("Name").StringValueOK() + elems, _ := doc.Elements() + for _, e := range elems { + p := path + "/" + e.Key() + if name != "" { + p = path + "/" + name + "." + e.Key() + } + walk(e.Value(), p) + } + case bson.TypeArray: + arr, ok := v.ArrayOK() + if !ok { + return + } + vals, _ := arr.Values() + for _, x := range vals { + walk(x, path) + } + } + } + if err := bson.Raw(raw).Validate(); err != nil { + return nil + } + walk(bson.RawValue{Type: bson.TypeEmbeddedDocument, Value: raw}, "") + return bad +} + +// BareAttributeRefError returns the error a write should fail with, or nil. +// unitLabel names the unit: an id is enough, a qualified name is better. +func BareAttributeRefError(unitLabel string, raw []byte) error { + bad := BareAttributeRefs(raw) + if len(bad) == 0 { + return nil + } + return fmt.Errorf("refusing to write unit %s: attribute reference not qualified as "+ + "Module.Entity.Attribute — Mendix cannot load a project holding one: %s. Qualify it in the "+ + "script; inside a data container whose entity cannot be resolved (e.g. its data-source flow "+ + "is missing) there is nothing to qualify a bare name against", + unitLabel, strings.Join(bad, "; ")) +} diff --git a/modelsdk/canon/attributeref_test.go b/modelsdk/canon/attributeref_test.go new file mode 100644 index 0000000000..1f5fee2224 --- /dev/null +++ b/modelsdk/canon/attributeref_test.go @@ -0,0 +1,49 @@ +// SPDX-License-Identifier: Apache-2.0 + +package canon + +import ( + "strings" + "testing" + + "go.mongodb.org/mongo-driver/v2/bson" +) + +// Moved from mdl/backend/modelsdk (page_bare_attributeref_test.go) when the +// guard moved to the write choke point. The shape is Feedback v4.0.2's +// ShareFeedback_Logo: an image URL parameter written as a bare `ImageB64` +// under a data view whose flow the project lacks, which left `mx check` unable +// to LOAD the project (ArgumentNullException setting 'Attribute', 11.13.0). +func TestBareAttributeRefError(t *testing.T) { + attrRef := func(a string) bson.D { + return bson.D{{Key: "$Type", Value: "DomainModels$AttributeRef"}, {Key: "Attribute", Value: a}, {Key: "EntityRef", Value: nil}} + } + doc := func(a string) []byte { + b, err := bson.Marshal(bson.D{ + {Key: "$Type", Value: "Forms$Page"}, + {Key: "Widgets", Value: bson.A{int32(2), bson.D{ + {Key: "$Type", Value: "CustomWidgets$CustomWidget"}, + {Key: "Name", Value: "image1"}, + {Key: "Params", Value: bson.A{int32(2), bson.D{{Key: "AttributeRef", Value: attrRef(a)}}}}, + }}}, + }) + if err != nil { + t.Fatal(err) + } + return b + } + for _, bare := range []string{"ImageB64", "Feedback.ImageB64"} { + err := BareAttributeRefError("page X", doc(bare)) + if err == nil || !strings.Contains(err.Error(), `"`+bare+`"`) || !strings.Contains(err.Error(), "image1") { + t.Fatalf("a bare attribute reference must be refused, naming it and its widget; got %v", err) + } + } + for _, ok := range []string{"FeedbackModule.Feedback.ImageB64", ""} { + if err := BareAttributeRefError("page X", doc(ok)); err != nil { + t.Errorf("%q must be accepted: %v", ok, err) + } + } + if got := BareAttributeRefs([]byte{1, 2, 3}); got != nil { + t.Errorf("unreadable bytes must yield nothing, got %v", got) + } +} diff --git a/modelsdk/mpr/writer_bare_attributeref_test.go b/modelsdk/mpr/writer_bare_attributeref_test.go new file mode 100644 index 0000000000..bd4dc52c8f --- /dev/null +++ b/modelsdk/mpr/writer_bare_attributeref_test.go @@ -0,0 +1,95 @@ +// SPDX-License-Identifier: Apache-2.0 + +package mpr + +import ( + "os" + "strings" + "testing" + + "go.mongodb.org/mongo-driver/v2/bson" +) + +// A DomainModels$AttributeRef whose Attribute is not Module.Entity.Attribute +// makes the PROJECT unloadable — `mx check` dies in the loader with +// ArgumentNullException setting 'Attribute' (Mendix 11.13.0) before any +// validation runs, and an excluded page does not help. The page and snippet +// encoders refused one, but ALTER PAGE patches the stored tree and saves it +// through UpdateRawUnit, so `alter page … insert … { image … (ImageUrlParams: +// [{1} = ImageB64]) }` inside a data view with no resolvable entity wrote one. +// The guard sits here, next to the duplicate-$ID one, and these tests go +// through the Writer because the wiring is what can come undone. + +func pageWithAttributeRef(t *testing.T, attr string) []byte { + t.Helper() + bin := func(id string) bson.Binary { return bson.Binary{Subtype: 0x00, Data: uuidToBlob(id)} } + b, err := bson.Marshal(bson.D{ + {Key: "$Type", Value: "Forms$Page"}, + {Key: "$ID", Value: bin("22222222-2222-2222-2222-222222222222")}, + {Key: "Name", Value: "P"}, + {Key: "Widget", Value: bson.D{ + {Key: "$Type", Value: "Forms$TextBox"}, + {Key: "$ID", Value: bin("33333333-3333-3333-3333-333333333333")}, + {Key: "Name", Value: "textBox1"}, + {Key: "AttributeRef", Value: bson.D{ + {Key: "$Type", Value: "DomainModels$AttributeRef"}, + {Key: "$ID", Value: bin("44444444-4444-4444-4444-444444444444")}, + {Key: "Attribute", Value: attr}, + {Key: "EntityRef", Value: nil}, + }}, + }}, + }) + if err != nil { + t.Fatalf("marshal: %v", err) + } + return b +} + +func TestUpdateUnitRefusesABareAttributeRef(t *testing.T) { + const unitID = "55555555-5555-5555-5555-555555555555" + w, unitPath := newV2WriterForCommitTest(t, unitID, pageWithAttributeRef(t, "MyModule.Customer.Name")) + before, err := os.ReadFile(unitPath) + if err != nil { + t.Fatalf("read seeded unit: %v", err) + } + + for _, bare := range []string{"Name", "Customer.Name"} { + err := w.UpdateRawUnit(unitID, pageWithAttributeRef(t, bare)) + if err == nil { + t.Fatalf("write accepted a unit holding the bare attribute reference %q", bare) + } + for _, want := range []string{unitID, `"` + bare + `"`, "textBox1", "Module.Entity.Attribute"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("message missing %q: %v", want, err) + } + } + } + + after, err := os.ReadFile(unitPath) + if err != nil { + t.Fatalf("read unit after refusal: %v", err) + } + if string(after) != string(before) { + t.Error("the refused write still changed the stored unit") + } +} + +func TestUpdateUnitAcceptsQualifiedAndEmptyAttributeRefs(t *testing.T) { + // The control. A qualified reference is what Studio Pro stores (73 of 73 + // across a real project's units), and an EMPTY Attribute is a slot the + // author has not bound yet — neither may be refused. + const unitID = "66666666-6666-6666-6666-666666666666" + w, unitPath := newV2WriterForCommitTest(t, unitID, pageWithAttributeRef(t, "MyModule.Customer.Name")) + for _, ok := range []string{"MyModule.Customer.Email", ""} { + if err := w.UpdateRawUnit(unitID, pageWithAttributeRef(t, ok)); err != nil { + t.Fatalf("Attribute %q refused: %v", ok, err) + } + } + after, err := os.ReadFile(unitPath) + if err != nil { + t.Fatalf("read unit: %v", err) + } + if strings.Contains(string(after), "MyModule.Customer.Name") { + t.Error("the accepted writes did not reach the stored unit") + } +} diff --git a/modelsdk/mpr/writer_core.go b/modelsdk/mpr/writer_core.go index 01e090bfcd..9d9e0b23cb 100644 --- a/modelsdk/mpr/writer_core.go +++ b/modelsdk/mpr/writer_core.go @@ -526,6 +526,9 @@ func (w *Writer) insertUnit(unitID, containerID, containmentName, unitType strin if err := canon.DuplicateElementIDError(unitID, contents); err != nil { return err } + if err := canon.BareAttributeRefError(unitID, contents); err != nil { + return err + } // Convert UUID strings to 16-byte blobs for database unitIDBlob := uuidToBlob(unitID) @@ -610,6 +613,12 @@ func (w *Writer) updateUnit(unitID string, contents []byte, opts ...canon.Option if err := canon.DuplicateElementIDError(unitID, contents); err != nil { return err } + // Same reasoning, same place: a bare DomainModels$AttributeRef makes the + // project unloadable, and ALTER PAGE's patches reach here without passing + // the page encoder that also refuses one (canon/attributeref.go). + if err := canon.BareAttributeRefError(unitID, contents); err != nil { + return err + } // Session mode: divert to in-memory buffer and skip all disk/SQLite work. // The caller (e.g. ImportProject) is responsible for flushing later.