From f18d404dac6728394f2cb35bd8322cb6c3ac48a7 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 20 Sep 2026 12:59:18 +0000 Subject: [PATCH 01/10] fix(alter page): refuse SET DataSource = DATABASE instead of wiping the source MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `alter page … { set DataSource = DATABASE Mod.Entity on dvCust; }` passed `check`, printed `Altered page …` with exit 0, and left the DataView with no usable datasource: `describe page` rendered the widget with the property gone, and the only other signal was CE7007 from mxbuild — naming the widget, never the statement that broke it. One mapping stood in for several. A DATABASE source has no single stored shape; the widget holding it decides which element Studio Pro writes (Forms$ListViewXPathSource on a list view, CustomWidgets$CustomWidgetXPathSource on a pluggable widget, Forms$GridXPathSource on a grid). A data view has no database form at all — it binds to one object — which is why CREATE PAGE's dataViewSourceToGen already refused that pairing while SET wrote it silently. serializeDataSourceBson instead emitted a Forms$DataViewSource (the "data from context" source) with the entity in EntityRef and SourceVariable left null, which is neither shape and is why the describe reader, needing a SourceVariable, rendered nothing. The setter now refuses a database source, dispatching on the stored widget's $Type for the remedy: a data view is told which sources it can take (REPLACE would be a dead end there — CREATE PAGE refuses the same pairing), every other widget is pointed at REPLACE, which reaches the real builder. Rebuilding the shapes in the mutator would be a second copy of listViewSourceToGen in raw BSON, the duplicate-resolver drift CLAUDE.md warns about. The refusal lives once, in the mutator, so `check -p --references` — which dry-runs the setter against a pagemutator.Probe() copy — and `exec` cannot disagree. Measured on two copies of a real Mendix 11.13.0 app: with the fault check --references -> "Check passed!" exec -> "Altered page …", exit 0 describe page -> dataview dvCust { … } (no source) mx check -> 1 error, CE7007 at Data view 'dvCust' with the fix check --references -> refused, exit 1 exec -> refused, exit 1 describe page -> datasource unchanged mx check -> 0 errors Control: with the fix reverted, both new tests fail with the reported symptom (the statement accepted; check reporting 0 errors), while the non-database retypes #855 added keep passing. The alter-page skill advertised the database form as supported; corrected, along with the `mxcli syntax page.alter` help. upstream mendixlabs/mxcli#1032 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PSDurGmL9HgDdLhpMqZt6o --- .../fix-issue/findings/mdl-executor.jsonl | 1 + .claude/skills/mendix/alter-page/SKILL.md | 23 +++- cmd/mxcli/syntax/features_page.go | 2 +- ...032-alter-page-set-database-datasource.mdl | 106 ++++++++++++++++ mdl/backend/pagemutator/mutator.go | 63 ++++++--- .../pagemutator/mutator_datasource_test.go | 120 ++++++++++++++++++ mdl/executor/validate_alter_set_test.go | 91 +++++++++++++ 7 files changed, 384 insertions(+), 22 deletions(-) create mode 100644 mdl-examples/bug-tests/1032-alter-page-set-database-datasource.mdl diff --git a/.claude/skills/fix-issue/findings/mdl-executor.jsonl b/.claude/skills/fix-issue/findings/mdl-executor.jsonl index e21645d673..7b3c0c4b9f 100644 --- a/.claude/skills/fix-issue/findings/mdl-executor.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-executor.jsonl @@ -658,3 +658,4 @@ {"area": "mdl/executor", "date": "2026-09-20", "symptom": "An input widget cannot bind an attribute over an association. `textbox t (attribute: Assoc/Attr)` builds a FLAT path and mxbuild fails **CE1613** \"The selected attribute 'Rules.RuleAction.RuleAction_BusinessRule/Name' no longer exists.\" — note the shape: the association segment is pasted onto the CONTEXT entity instead of being navigated. The same syntax works on a DataGrid2 column, so one page could bind an associated attribute in a grid column and fail on the text box beside it. Read half: DESCRIBE emitted a bare `Attribute: Name`, so describe → exec over a Studio Pro page REBOUND the widget to an attribute the context entity does not have, with `mxcli check` clean", "cause": "All six input builders (textbox/textarea/datepicker/dropdown/checkbox/radiobuttons) called `resolveAttributePath`, which knows nothing about associations; `resolveAssociationAttributePath` existed and was wired only into DataGrid2 columns, DynamicText params and the widget engine. On the read side `extractAttributeRef` returned the last segment of AttributeRef.Attribute and ignored AttributeRef.EntityRef entirely, while `columnAttributeFromRef` already handled it correctly — two readers of one BSON shape", "file": "`mdl/executor/cmd_pages_builder_v3.go` (`resolveInputAttribute`), `cmd_pages_builder_v3_widgets.go` (six builders), `cmd_pages_describe_parse.go` (`extractAttributeRef`), `mdl/backend/modelsdk/widget_write.go` (`inputAttributeRefToGen`), `sdk/pages/pages_widgets_input.go` (AttributeRefSteps on six structs)", "insight": "**Mendix DOES permit this on a plain text box** — the question that blocked the fix, settled by a Studio Pro reference rather than reasoning: ako/TestApp `Rules.RuleAction_NewEdit` textBox4 stores Attribute \"Rules.BusinessRule.Name\" + IndirectEntityRef over Rules.RuleAction_BusinessRule. Without it the plausible fix was the opposite one (refuse it, hint at a nested dataview). **Fix both halves together or the round trip just changes failure mode** — a describer emitting a path the builder cannot consume turns silent corruption into a build error. The build-level control is the evidence that counts: same script, same mxbuild 11.14.0, pre-fix binary → CE1613, post-fix → 0 errors. An entity with NO attribute of that name is what makes the control sharp, since a same-named attribute would merely bind somewhere else", "refs": ["#529"], "ce": ["CE1613"]} {"area": "mdl/executor", "date": "2026-09-20", "symptom": "`CREATE OR MODIFY EXTERNAL ENTITIES FROM` marks an external entity's KEY attribute Updatable=true when the entity set's UpdateRestrictions say the set is updatable. Mendix computes a key as non-updatable, so `mx check` reports CE6630 \"'DefinitionId' is marked Updatable=False in the OData service, but True in the app.\" at Attribute 'MyFirstModule.Definition.DefinitionId'. Independent of the ComplexType flattening fixed in #1118 — reproduces on a contract with no complex type at all", "cause": "createExternalEntities' attribute loop started `updatable` from the entity set's UpdateRestrictions/Updatable and cleared it only for NonUpdatableProperties, Core.Computed, Core.Immutable or a flattened complex leaf. Key membership was already in the loop as keyPropSet[p.Name] — passed to edmToDomainModelAttrType, never consulted for updatability. The rule is gated on isTopLevel: only the key of an entity that HAS an entity set is non-updatable", "file": "`mdl/executor/cmd_contract.go` (createExternalEntities attribute loop; `isKey` now drives both the attribute type and Updatable)", "insight": "**The rule is `isKey && isTopLevel`, and the `isTopLevel` half cost a red CI run to learn.** The first fix was the blanket 'a key is never updatable' — every unit test green, the reported CE6630 gone, a real 11.12.1 build confirming it, and it turned ONE error into SEVEN of its inverse on the live TripPin contract: \"'TripId' is marked Updatable=True in the OData service, but False in the app.\" over Trip, PlanItem, Event, Flight, PublicTransportation, Employee and Manager — every one a derived or contained type with NO entity set, mutated through its parent's write flow — while Person/Airline/Airport (the entity sets) stayed silent at false. **`UserName` is the two-sided control inside one document**: expected False on Person and True on Employee and Manager, so the split is the entity set and cannot be inheritance or the property. **#1118's finding said this in advance and it was not heeded: 'Do not stop at a synthetic fixture. TripPin is the fixture to reach for.'** Seven synthetic annotation shapes agreed with each other and were all top-level, so the probe could not see the variable that mattered — a negative that is uniform across a whole class is evidence about the query. Cheapest guard for next time: `go test -tags integration -run 'TestMxCheck_DoctypeScripts/10-odata-examples'` is ~36s locally against cached mxbuild, versus a 12-minute CI round. **Clear Updatable on a key, NOT Creatable** — a key is written once at creation, so Insertable still applies; the report's build flagged it Updatable=False only, and clearing both is CE6630 inverted on the key of any insertable set. **The report's own error count was wrong**, which is why the real build mattered: it promises \"exactly one CE6630, naming the key attribute\"; the identical contract gives TWO — the key AND the non-key `Label`. The fix takes 2 to 1, not to 0, so a regression test asserting `mx check` clean would have failed against a correct fix. **The Label half is a SECOND defect, still open**: mxbuild answered Updatable=False for the non-key attribute of all seven TOP-LEVEL shapes (inline record, ``, UpdateMethod=PATCH, +NonUpdatableProperties/+DeleteRestrictions, unannotated, external ``, property-level Core.Permissions/ReadWrite). The evidence now points at `Updatable == !isTopLevel` for ALL attributes — the aliased run is the discriminator: with mxbuild reading an Updatable=true set and the app at false on every attribute, there were Creatable errors and NO Updatable error — but it was not broadened, because no top-level set has yet been seen that mxbuild treats as having an updatable attribute, and §48 measured CE6630 firing both ways. Two wrong turns ruled out by measurement, each cheap and each tempting: (1) the stored $metadata is NOT filtered — `grep -rc UpdateRestrictions mprcontents/` matches the InsertRestrictions count exactly, so mxbuild reads both from the same document and applies an extra rule to updatability only; (2) there is no entity-level updatable to set — `generated/metamodel` (the arbiter) gives Rest$ODataRemoteEntitySource Creatable/Deletable/Countable/Skip/Top and **no Updatable**, confirming the existing code comment, and mxbuild checks entity-level Creatable (it reported one) but never entity-level Updatable. Also measured: mxcli matches capability terms by fully-qualified name only, so the **aliased** spelling (`Capabilities.UpdateRestrictions` with an `/`) — what most real services emit — parses in mxbuild and not in mxcli, silently flipping every capability to the conservative default; the external `` form DOES parse. And `describe external entity` does not round-trip any per-attribute capability: it emits `Name: Type` only (losing Updatable, Creatable, Filterable, Sortable and the String length), so describe->exec into a fresh project rebuilds them from defaults. Repro `mdl-examples/bug-tests/odata-key-attribute-updatable.mdl`; tests `mdl/executor/cmd_contract_key_updatable_test.go` (the top-level and derived-type pair is the two-sided control — either alone passes against a fix wrong in the other direction)", "file_refs": ["mdl/executor/cmd_contract.go"], "ce": ["CE6630"]} {"area":"mdl/executor","date":"2026-09-20","symptom":"`CREATE OR MODIFY EXTERNAL ENTITIES FROM` marks NON-KEY attributes of a top-level external entity Updatable=true when the entity set's UpdateRestrictions say the set is updatable. `mx check` reports one CE6630 per attribute: \"'Label' is marked Updatable=False in the OData service, but True in the app.\" The sibling of the key-attribute defect fixed the same day; fixing only the key took the reported repro from 2 errors to 1, not to 0","cause":"`createExternalEntities` derived `defaultUpdatable` from `entitySet.Updatable`. Mendix does not: it computes Updatable as a function of whether the entity has an entity set at all — never for a top-level entity, always for a non-top-level one, which is written through its parent's flow. The annotation is irrelevant to it","file":"`mdl/executor/cmd_contract.go` (`defaultUpdatable := !isTopLevel`, replacing the UpdateRestrictions override; the key-specific guard added earlier becomes subsumed)","insight":"**The positive control that closes this is a contract whose `NonUpdatableProperties` names ONLY the key.** That service is asserting, by name, that every other property IS updatable — and mxbuild still answers False. Ten top-level shapes were probed in three rounds and all answered False (inline record, typed ``, UpdateMethod=PATCH, +NonUpdatableProperties +DeleteRestrictions, unannotated, external ``, Core.Permissions/ReadWrite, Core.OptimisticConcurrency/ETag, DeepUpdateSupport, and the key-only exclusion list); the first seven were NOT enough to act on, because 'no shape produces True' is the uniform-negative shape that means the probe is wrong — what made it actionable was a shape that states the opposite explicitly and is still refused. **Two false leads, each cheap and each worth skipping.** (1) ETag/optimistic concurrency, suggested by a comment in this very function about the service that motivated #729 — no effect. (2) The model's `AllowCreateChangeLocally`: the intuition is that Mendix would permit attribute changes once local changes are allowed, and it is wrong — setting it Yes on a top-level entity left the expectation at False. **That second control is the one that explains the rule rather than just fitting it**: an external object CAN be changed in memory and handed to an external OData action, and that is what the local-change flag governs; the attribute's `Updatable` mirrors only what the endpoint itself accepts on a PATCH. Domain knowledge from the maintainer, not derivable from the metamodel — worth asking for before probing an eleventh contract shape. The Insert/Update asymmetry that makes this look like a parser bug is real and is not one: mxbuild reads `InsertRestrictions` from the same document, in the same shapes, and honours it — so `Creatable` follows the contract and `Updatable` does not, which is also why 'the entity is read-only' is the wrong summary and why every test here asserts Creatable as its control. **A stale test encoded the old belief and had to be corrected, not worked around**: #1118's `TestCreateExternalEntities_FlattenedAttributesAreReadOnly` asserted `Label` was Creatable AND Updatable as its control; the Updatable half had been assumed from the contract rather than measured, while the flattened-attribute half it was controlling for HAD been. The control still works on Creatable alone. Verified end to end on a real 11.12.1 project: 2 errors before any fix, 1 after the key-only fix, **0 errors** now; TripPin (`-run 'TestMxCheck_DoctypeScripts/10-odata-examples'`, ~26s locally) is the other-direction control and stays green, since every entity it flags is non-top-level. Repro `mdl-examples/bug-tests/odata-key-attribute-updatable.mdl`; tests `mdl/executor/cmd_contract_key_updatable_test.go`","file_refs":["mdl/executor/cmd_contract.go"],"ce":["CE6630"]} +{"area": "mdl/backend/pagemutator", "date": "2026-09-20", "symptom": "`alter page … { set DataSource = DATABASE Mod.Entity on dvCust; }` passes `check`, prints `Altered page …` with exit 0, and leaves the DataView with no datasource: `describe page` renders `dataview dvCust {` with the property gone, and the only other signal is CE7007 at `mx check`", "cause": "`serializeDataSourceBson` mapped every `*pages.DatabaseSource` to a `Forms$DataViewSource` — the *context* source — with the entity in `EntityRef` and `SourceVariable` left null. A DATABASE source has no single stored shape: the widget holding it decides (`Forms$ListViewXPathSource` on a list view, `CustomWidgets$CustomWidgetXPathSource` on a pluggable widget, `Forms$GridXPathSource` on a grid), and a DATA VIEW has no database form at all — which is why CREATE PAGE's `dataViewSourceToGen` already refused that pairing while SET wrote it silently", "file": "`mdl/backend/pagemutator/mutator.go` (`SetWidgetDataSource`, new `databaseSourceRefusal`, `serializeDataSourceBson`)", "insight": "**The \"gone entirely\" in the report was DESCRIBE, not the document.** The DataSource was present and well-formed BSON; `parseContextSource` returns nil for a `Forms$DataViewSource` with no `SourceVariable`, so the reader rendered nothing. Chasing a deleted property would have been the wrong hunt — diff the stored BSON before believing a describe-shaped symptom. **The refusal belongs in the mutator, not the validator**: `validateAlterSetProperties` dry-runs the real setter against a `pagemutator.Probe()` copy, so one refusal makes `check -p --references` and `exec` agree by construction; a second copy of the rule in the validator is the duplicate-resolver drift CLAUDE.md warns about. **Refusing beat rebuilding the shapes here** — writing ListViewXPathSource in raw BSON would duplicate `listViewSourceToGen` in a second currency, and REPLACE already reaches the real builder. **Two remedies, not one**: on a data view `use REPLACE` is a dead end (CREATE PAGE refuses it too), so the message names the sources a data view can take; on a list view REPLACE genuinely works, so it names REPLACE. Getting that backwards sends the author in a circle. **Same generalisable shape as #855/#1101**: when SET and REPLACE express different vocabularies for one property, SET is a whitelist extended one bug report at a time. **`make check-mdl` runs `mxcli check` WITHOUT `-p`**, so a document-dependent refusal cannot be a `.fail.mdl` — it would be reported as a negative test that unexpectedly passed. Write the passing shape and comment the refused statements, as #1063 does. Measured on two copies of a real 11.13.0 app: faulty → `Check passed!`, exit 0, `mx check` 1 error CE7007 at Data view 'dvCust'; fixed → both refuse with exit 1, datasource unchanged, `mx check` 0 errors. Tests `mdl/backend/pagemutator/mutator_datasource_test.go`, `mdl/executor/validate_alter_set_test.go`; example `mdl-examples/bug-tests/1032-alter-page-set-database-datasource.mdl`. upstream #1032", "refs": ["#855", "#1032"], "ce": ["CE7007"]} diff --git a/.claude/skills/mendix/alter-page/SKILL.md b/.claude/skills/mendix/alter-page/SKILL.md index c4898bb5c5..2c1fa2afd5 100644 --- a/.claude/skills/mendix/alter-page/SKILL.md +++ b/.claude/skills/mendix/alter-page/SKILL.md @@ -127,7 +127,7 @@ set Action = SHOW_PAGE Module.DetailPage on btnEdit -- Rebind a data-bound widget set DataSource = $OrderParam on dvOrder -set DataSource = DATABASE Module.Order on dgOrders +set DataSource = microflow Module.MF_Get on dvOrder ``` **Prefer `set Action` over `replace` when only the action changes.** `replace` @@ -198,7 +198,6 @@ from a microflow to a page parameter: ```sql ALTER PAGE MyModule.OrderPage { SET DataSource = $Order ON dvOrder; -- page/snippet parameter - SET DataSource = database MyModule.Order ON dgOrders; -- database SET DataSource = microflow MyModule.MF_Get ON dvOrder; -- microflow SET DataSource = nanoflow MyModule.NF_Get ON dvOrder; -- nanoflow SET DataSource = selection dgOrders ON dvDetail; -- listen to widget @@ -209,9 +208,23 @@ The parameter must exist on the page (or snippet) being altered — its entity i read from the container's own parameter list, and an unknown name is refused rather than written as an unresolved reference. -`association` sources are **not** supported by SET. Use REPLACE for those, which -rebuilds the widget through the CREATE PAGE path and handles every datasource -type; the error message says so. +`association` and `database` sources are **not** supported by SET. Use REPLACE +for those, which rebuilds the widget through the CREATE PAGE path and handles +every datasource type; the error message says so. + +A `database` source has no single stored shape — the widget holding it decides +which element Mendix writes (a list view, a data grid and a pluggable widget +each store a different one), and SET writes the property directly rather than +rebuilding the widget, so it has nothing to choose from. This used to be +accepted and half-applied: the widget was left with a source that DESCRIBE read +back as absent and mxbuild rejected as **CE7007**, on a page `exec` had just +reported as altered (mendixlabs/mxcli#1032). + +A **data view** is the one case REPLACE does not rescue: it binds to a single +object, so Mendix gives it no database form at all and the CREATE PAGE path +refuses one too. Point it at a context parameter, a microflow, a nanoflow or +`selection `, and use a list view or a data grid to show a query. The +refusal says which of the two situations you are in. ### INSERT - Add Widgets diff --git a/cmd/mxcli/syntax/features_page.go b/cmd/mxcli/syntax/features_page.go index c4609994a0..271179111f 100644 --- a/cmd/mxcli/syntax/features_page.go +++ b/cmd/mxcli/syntax/features_page.go @@ -275,7 +275,7 @@ CREATE PAGE Sales.Detail (Title: 'Detail', Layout: Atlas_Core.Atlas_Default) { "popup width", "popup height", "popup resizable", "drop template", "insert template", "list view template", }, - Syntax: "ALTER PAGE Module.Name {\n SET property = value ON widgetName; -- widget property names: any casing\n SET Action = MICROFLOW Module.MF ON btnSave; -- any CREATE PAGE action form\n SET DataSource = $Param ON dvOrder;\n SET (prop1 = val1, prop2 = val2) ON widgetName;\n SET Title = 'New Title'; -- page-level (case-sensitive)\n SET Documentation = 'What this page is for.';\n SET Class = 'css-class'; -- page-level CSS class / style\n SET Style = 'css: rule';\n SET PopupWidth = 800; -- page-level pop-up dimensions\n SET PopupHeight = 480;\n SET PopupResizable = true;\n INSERT AFTER widgetName { };\n INSERT BEFORE widgetName { };\n INSERT INTO containerName { };\n DROP WIDGET name1, name2;\n DROP TEMPLATE FOR Module.Specialization IN listViewName;\n REPLACE widgetName WITH { };\n};", + Syntax: "ALTER PAGE Module.Name {\n SET property = value ON widgetName; -- widget property names: any casing\n SET Action = MICROFLOW Module.MF ON btnSave; -- any CREATE PAGE action form\n SET DataSource = $Param ON dvOrder; -- parameter/microflow/nanoflow/selection;\n -- DATABASE and association are REPLACE-only,\n -- and a data view takes no database source\n SET (prop1 = val1, prop2 = val2) ON widgetName;\n SET Title = 'New Title'; -- page-level (case-sensitive)\n SET Documentation = 'What this page is for.';\n SET Class = 'css-class'; -- page-level CSS class / style\n SET Style = 'css: rule';\n SET PopupWidth = 800; -- page-level pop-up dimensions\n SET PopupHeight = 480;\n SET PopupResizable = true;\n INSERT AFTER widgetName { };\n INSERT BEFORE widgetName { };\n INSERT INTO containerName { };\n DROP WIDGET name1, name2;\n DROP TEMPLATE FOR Module.Specialization IN listViewName;\n REPLACE widgetName WITH { };\n};", Example: "ALTER PAGE Module.EditPage {\n SET (Caption = 'Save & Close', ButtonStyle = Success) ON btnSave;\n INSERT AFTER txtName {\n TEXTBOX txtMiddleName (Label: 'Middle Name', Attribute: MiddleName)\n };\n DROP WIDGET txtUnused;\n};", SeeAlso: []string{"page.create", "page.show", "snippet.alter"}, }) diff --git a/mdl-examples/bug-tests/1032-alter-page-set-database-datasource.mdl b/mdl-examples/bug-tests/1032-alter-page-set-database-datasource.mdl new file mode 100644 index 0000000000..b05a7bebb5 --- /dev/null +++ b/mdl-examples/bug-tests/1032-alter-page-set-database-datasource.mdl @@ -0,0 +1,106 @@ +-- mendixlabs/mxcli#1032 — `SET DataSource = DATABASE …` silently wiped a +-- DataView's datasource +-- +-- The reported statement +-- +-- ALTER PAGE Test.MyPage { SET DataSource = DATABASE Test.Customer ON dvCust; }; +-- +-- passed `check`, ran to `Altered page …` with exit 0, and left the DataView +-- with no usable datasource at all. `DESCRIBE PAGE` read the widget back as +-- `dataview dvCust {` with the property gone, and the only other signal was +-- CE7007 at build time — naming the widget, never the statement that broke it. +-- +-- Cause: one mapping standing in for several. A DATABASE source has no single +-- stored shape; the widget holding it decides which element Studio Pro writes — +-- Forms$ListViewXPathSource on a list view, CustomWidgets$CustomWidgetXPathSource +-- on a pluggable widget, Forms$GridXPathSource on a grid. A DATA VIEW has no +-- database form at all: it binds to ONE object, which is why CREATE PAGE's own +-- builder refuses that pairing. The mutator instead wrote a Forms$DataViewSource +-- — the "data from context" source — with the entity in EntityRef and +-- SourceVariable left null, which is neither shape and is why DESCRIBE (whose +-- context reader needs a SourceVariable) rendered nothing. +-- +-- Measured on a real Mendix 11.13.0 app, two copies of the same project: +-- +-- with the fault mxcli check --references → "Check passed!" +-- mxcli exec → "Altered page …", exit 0 +-- describe page → dataview dvCust { … } (no source) +-- mx check → 1 error, CE7007 at Data view 'dvCust' +-- +-- with the fix mxcli check --references → refused, exit 1 +-- mxcli exec → refused, exit 1 +-- describe page → datasource unchanged +-- mx check → 0 errors +-- +-- The refusal lives once, in the mutator, so `check` (which dry-runs the SET +-- against a throwaway copy of the stored document) and `exec` cannot disagree. +-- It needs the document to know the widget's type, so it fires under `-p`; this +-- file is therefore the PASSING shape, with the refused statements commented. +-- +-- Run: mxcli check mdl-examples/bug-tests/1032-alter-page-set-database-datasource.mdl +-- mxcli exec mdl-examples/bug-tests/1032-alter-page-set-database-datasource.mdl -p app.mpr + +create module P1032; +create persistent entity P1032.Customer (Name: String(200)); + +create microflow P1032.MF_GetCustomer () returns P1032.Customer as $Result +begin + retrieve $Result from P1032.Customer limit 1; +end; + +create page P1032.CustomerPage ( + params: { $Customer: P1032.Customer }, + title: 'Customer', + layout: Atlas_Core.Atlas_Default +) { + layoutgrid g { row r { column c (desktopwidth: 12) { + -- A LIST VIEW is where a database query belongs. Authored here through + -- CREATE PAGE, which builds the Forms$ListViewXPathSource the widget needs. + listview lvCustomers (datasource: database P1032.Customer) { + textbox tbListName (attribute: Name, label: 'Name') + } + -- The DataView from the report, on a microflow source. + dataview dvCust (datasource: microflow P1032.MF_GetCustomer) { + textbox tbName (attribute: Name, label: 'Name') + } + } } } +} + +-- The sources a DataView really takes still retype through SET, unchanged by +-- the refusal — the control that says this fixes #1032 rather than replacing it +-- with a blanket "no". +alter page P1032.CustomerPage { + set DataSource = $Customer on dvCust; +} + +alter page P1032.CustomerPage { + set DataSource = microflow P1032.MF_GetCustomer on dvCust; +} + +alter page P1032.CustomerPage { + set DataSource = selection lvCustomers on dvCust; +} + +-- REFUSED, both by `check -p --references` and by `exec` (uncomment to see): +-- +-- alter page P1032.CustomerPage { +-- set DataSource = database P1032.Customer on dvCust; +-- } +-- +-- a data view cannot take a database datasource — a data view binds to a +-- single object, so its source is a context parameter (`$Param`), a +-- microflow, a nanoflow or `selection `; to show the result of a +-- database query, use a list view or a data grid instead +-- +-- The same statement against the LIST VIEW is refused too, but for the other +-- reason and with the other remedy — a list view can hold a database source, +-- SET just cannot build one, so the message names the path that can: +-- +-- alter page P1032.CustomerPage { +-- set DataSource = database P1032.Customer on lvCustomers; +-- } +-- +-- setting a database datasource on "lvCustomers" (Forms$ListView) is not +-- supported by `set` — its stored shape depends on the widget and is built +-- by the CREATE PAGE path; use `replace with …` instead, which +-- rebuilds the widget through that path diff --git a/mdl/backend/pagemutator/mutator.go b/mdl/backend/pagemutator/mutator.go index d2cbb298c2..d1fa60de20 100644 --- a/mdl/backend/pagemutator/mutator.go +++ b/mdl/backend/pagemutator/mutator.go @@ -140,6 +140,10 @@ func (m *Mutator) SetWidgetDataSource(widgetRef string, ds pages.DataSource) err ds = &resolved } + if err := databaseSourceRefusal(result.widget, ds); err != nil { + return err + } + serialized := serializeDataSourceBson(ds) if serialized == nil { return fmt.Errorf("unsupported DataSource type %T", ds) @@ -148,6 +152,46 @@ func (m *Mutator) SetWidgetDataSource(widgetRef string, ds pages.DataSource) err return nil } +// databaseSourceRefusal turns away `set DataSource = DATABASE …`, which this +// setter cannot write correctly for any widget (#1032). +// +// A DATABASE source is not one element but several, chosen by the widget that +// holds it: Forms$ListViewXPathSource on a list view, +// CustomWidgets$CustomWidgetXPathSource on a pluggable widget, +// Forms$GridXPathSource on a grid — each with its own sort bar and search +// sub-elements. A DATA VIEW has no database form at all, which is why the CREATE +// PAGE builder refuses that pairing outright. +// +// One mapping stood in for all of them and wrote a Forms$DataViewSource — the +// "data from context" source — with the entity in EntityRef and SourceVariable +// left null. Nothing rejected it: `exec` reported success, DESCRIBE read it back +// as no datasource at all (the context reader needs a SourceVariable), and the +// first signal was CE7007 from mxbuild, naming the widget rather than the +// statement that broke it. +// +// Refusing is what the two reads agree on. Rebuilding the shapes here would be a +// second copy of listViewSourceToGen and friends in a second currency — the +// drift CLAUDE.md's duplicate-resolver rule is about — while REPLACE already +// reaches the one that exists, by rebuilding the widget through CREATE PAGE. +func databaseSourceRefusal(widget bson.D, ds pages.DataSource) error { + if _, ok := ds.(*pages.DatabaseSource); !ok { + return nil + } + if bsonnav.DGetString(widget, "$Type") == "Forms$DataView" { + // Not "use replace": REPLACE goes through the same CREATE PAGE builder, + // which refuses a database source on a data view as well. Naming it + // would send the author down a dead end. + return fmt.Errorf("a data view cannot take a database datasource — a data view binds to a " + + "single object, so its source is a context parameter (`$Param`), a microflow, a nanoflow " + + "or `selection `; to show the result of a database query, use a list view or a " + + "data grid instead") + } + return fmt.Errorf("setting a database datasource on %q (%s) is not supported by `set` — "+ + "its stored shape depends on the widget and is built by the CREATE PAGE path; "+ + "use `replace with …` instead, which rebuilds the widget through that path", + bsonnav.DGetString(widget, "Name"), widgetTypeName(widget)) +} + // SetWidgetAction retargets the on-click action of an existing widget. // // The action is serialized through the same engine hook CREATE PAGE uses, so @@ -2929,22 +2973,9 @@ func serializeDataSourceBson(ds pages.DataSource) bson.D { {Key: "$Type", Value: "Forms$ListenTargetSource"}, {Key: "ListenTarget", Value: d.WidgetName}, } - case *pages.DatabaseSource: - var entityRef any - if d.EntityName != "" { - entityRef = bson.D{ - {Key: "$ID", Value: bsonutil.NewIDBsonBinary()}, - {Key: "$Type", Value: "DomainModels$DirectEntityRef"}, - {Key: "Entity", Value: d.EntityName}, - } - } - return bson.D{ - {Key: "$ID", Value: bsonutil.NewIDBsonBinary()}, - {Key: "$Type", Value: "Forms$DataViewSource"}, - {Key: "EntityRef", Value: entityRef}, - {Key: "ForceFullObjects", Value: false}, - {Key: "SourceVariable", Value: nil}, - } + // A *pages.DatabaseSource is deliberately absent: it has no single stored + // shape, so there is nothing to map it to here. databaseSourceRefusal + // above turns it away before this is reached. case *pages.DataViewSource: // "Data from context": the widget binds to a page/snippet parameter. The // EntityRef names the parameter's entity and the SourceVariable points at diff --git a/mdl/backend/pagemutator/mutator_datasource_test.go b/mdl/backend/pagemutator/mutator_datasource_test.go index d680175c95..5a82d642b1 100644 --- a/mdl/backend/pagemutator/mutator_datasource_test.go +++ b/mdl/backend/pagemutator/mutator_datasource_test.go @@ -3,6 +3,7 @@ package pagemutator import ( + "strings" "testing" "go.mongodb.org/mongo-driver/bson" @@ -143,3 +144,122 @@ func findWidgetForTest(t *testing.T, raw bson.D, name string) bson.D { } return result.widget } + +// upstream #1032: `alter page … { set DataSource = DATABASE Mod.Entity on dvCust; }` +// reported success and left the DataView with no usable datasource at all. +// +// The cause is one mapping doing duty for several: a DATABASE source has no +// single stored shape. Studio Pro writes Forms$ListViewXPathSource on a list +// view, CustomWidgets$CustomWidgetXPathSource on a pluggable widget, +// Forms$GridXPathSource on a grid — and a DATA VIEW has no database form at +// all, which is why CREATE PAGE's dataViewSourceToGen refuses one. The mutator +// instead wrote a Forms$DataViewSource — the "data from context" source — with +// the entity in EntityRef and SourceVariable left null. DESCRIBE reads that back +// as no datasource (parseContextSource needs a SourceVariable), so the write +// looked like a deletion, and the only other signal was CE7007 at build time. +func TestSetWidgetDataSource_DatabaseOnDataViewIsRefused(t *testing.T) { + dv := bson.D{ + {Key: "$Type", Value: "Forms$DataView"}, + {Key: "Name", Value: "dvCust"}, + {Key: "DataSource", Value: bson.D{ + {Key: "$Type", Value: "Forms$MicroflowSource"}, + {Key: "MicroflowSettings", Value: bson.D{ + {Key: "$Type", Value: "Forms$MicroflowSettings"}, + {Key: "Microflow", Value: "Test.ACT_GetCustomer"}, + }}, + }}, + } + m := New(makeRawPage(dv), model.ID("unit-1"), nil) + + err := m.SetWidgetDataSource("dvCust", &pages.DatabaseSource{EntityName: "Test.Customer"}) + if err == nil { + t.Fatal("a database datasource on a data view was accepted — " + + "the write leaves the widget with no usable source and surfaces only as CE7007") + } + // The message has to say what a data view CAN take. Pointing at REPLACE + // here would be wrong: REPLACE rebuilds through the CREATE PAGE path, which + // refuses this pairing too, so the author would be sent down a dead end. + for _, want := range []string{"data view", "database"} { + if !strings.Contains(strings.ToLower(err.Error()), want) { + t.Errorf("error %q does not mention %q", err, want) + } + } + + // Guard-don't-drop: the stored source must be untouched, not half-replaced. + dsDoc := bsonnav.DGetDoc(findWidgetForTest(t, m.rawData, "dvCust"), "DataSource") + if dsDoc == nil { + t.Fatal("DataSource removed by a refused set") + } + if got := bsonnav.DGetString(dsDoc, "$Type"); got != "Forms$MicroflowSource" { + t.Errorf("$Type = %q, want the stored Forms$MicroflowSource to survive", got) + } + if bsonnav.DGetDoc(dsDoc, "MicroflowSettings") == nil { + t.Error("MicroflowSettings gone — the refused set still mutated the widget") + } +} + +// The same wrong mapping landed on every other widget too, unreported: a list +// view legitimately takes a database source, but its stored shape is a +// Forms$ListViewXPathSource, not the Forms$DataViewSource this wrote. Here +// REPLACE genuinely is the way out — listViewSourceToGen builds the right +// element — so the message names it. +func TestSetWidgetDataSource_DatabaseOnListViewIsRefused(t *testing.T) { + lv := bson.D{ + {Key: "$Type", Value: "Forms$ListView"}, + {Key: "Name", Value: "lvCust"}, + {Key: "DataSource", Value: bson.D{ + {Key: "$Type", Value: "Forms$ListViewXPathSource"}, + {Key: "EntityRef", Value: bson.D{ + {Key: "$Type", Value: "DomainModels$DirectEntityRef"}, + {Key: "Entity", Value: "Test.Customer"}, + }}, + }}, + } + m := New(makeRawPage(lv), model.ID("unit-1"), nil) + + err := m.SetWidgetDataSource("lvCust", &pages.DatabaseSource{EntityName: "Test.Customer"}) + if err == nil { + t.Fatal("a database datasource on a list view was accepted — " + + "the mutator writes a Forms$DataViewSource, which is not a list view source") + } + if !strings.Contains(err.Error(), "replace") { + t.Errorf("error %q does not name the path that can do this", err) + } + dsDoc := bsonnav.DGetDoc(findWidgetForTest(t, m.rawData, "lvCust"), "DataSource") + if got := bsonnav.DGetString(dsDoc, "$Type"); got != "Forms$ListViewXPathSource" { + t.Errorf("$Type = %q, want the stored Forms$ListViewXPathSource to survive", got) + } +} + +// Control: the refusal is about the DATABASE form only. Every source the +// mutator can write correctly must still go through, or the fix for #1032 is a +// regression for the retype #855 added. +func TestSetWidgetDataSource_NonDatabaseSourcesStillWork(t *testing.T) { + cases := []struct { + name string + ds pages.DataSource + wantType string + }{ + {"microflow", &pages.MicroflowSource{Microflow: "Test.ACT_GetCustomer"}, "Forms$MicroflowSource"}, + {"nanoflow", &pages.NanoflowSource{Nanoflow: "Test.NF_GetCustomer"}, "Forms$NanoflowSource"}, + {"selection", &pages.ListenToWidgetSource{WidgetName: "lvCust"}, "Forms$ListenTargetSource"}, + {"parameter", &pages.DataViewSource{ParameterName: "Order"}, "Forms$DataViewSource"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + dv := bson.D{ + {Key: "$Type", Value: "Forms$DataView"}, + {Key: "Name", Value: "dvOrder"}, + {Key: "DataSource", Value: bson.D{{Key: "$Type", Value: "Forms$MicroflowSource"}}}, + } + m := New(makeParameterisedPage("Forms$PageParameter", dv), model.ID("unit-1"), nil) + if err := m.SetWidgetDataSource("dvOrder", tc.ds); err != nil { + t.Fatalf("SetWidgetDataSource(%T): %v", tc.ds, err) + } + dsDoc := bsonnav.DGetDoc(findWidgetForTest(t, m.rawData, "dvOrder"), "DataSource") + if got := bsonnav.DGetString(dsDoc, "$Type"); got != tc.wantType { + t.Errorf("$Type = %q, want %q", got, tc.wantType) + } + }) + } +} diff --git a/mdl/executor/validate_alter_set_test.go b/mdl/executor/validate_alter_set_test.go index 106d0b461e..0c0986fee7 100644 --- a/mdl/executor/validate_alter_set_test.go +++ b/mdl/executor/validate_alter_set_test.go @@ -283,3 +283,94 @@ func TestAlterSet_NotConnected(t *testing.T) { t.Fatalf("ran without a project: %v", errs) } } + +// --------------------------------------------------------------------------- +// upstream #1032 — a destructive SET must be refused by check, not just by exec +// --------------------------------------------------------------------------- + +// storedDataViewPage builds a page holding one DataView on a microflow source — +// the starting point #1032 reproduces from. +func storedDataViewPage() bson.D { + dv := bson.D{ + {Key: "$Type", Value: "Forms$DataView"}, + {Key: "Name", Value: "dvCust"}, + {Key: "DataSource", Value: bson.D{ + {Key: "$Type", Value: "Forms$MicroflowSource"}, + {Key: "MicroflowSettings", Value: bson.D{ + {Key: "$Type", Value: "Forms$MicroflowSettings"}, + {Key: "Microflow", Value: "MyModule.ACT_GetCustomer"}, + }}, + }}, + } + return bson.D{ + {Key: "$Type", Value: "Forms$Page"}, + {Key: "FormCall", Value: bson.D{ + {Key: "Arguments", Value: bson.A{ + int32(2), + bson.D{{Key: "Widgets", Value: bson.A{int32(2), dv}}}, + }}, + }}, + } +} + +func dataViewPageCtx(t *testing.T) (*ExecContext, *countingDeps) { + t.Helper() + mod := mkModule("MyModule") + pg := mkPage(mod.ID, "P_View") + deps := &countingDeps{} + mb := &mock.MockBackend{ + IsConnectedFunc: func() bool { return true }, + ListModulesFunc: func() ([]*model.Module, error) { return []*model.Module{mod}, nil }, + ListFoldersFunc: func() ([]*types.FolderInfo, error) { return nil, nil }, + ListPagesFunc: func() ([]*pages.Page, error) { return []*pages.Page{pg}, nil }, + OpenPageForMutationFunc: func(unitID model.ID) (backend.PageMutator, error) { + return pagemutator.New(storedDataViewPage(), unitID, deps), nil + }, + } + ctx, _ := newMockCtx(t, withBackend(mb), withHierarchy(mkHierarchy(mod))) + return ctx, deps +} + +// TestAlterSet_DatabaseDataSourceOnDataView — the reported statement. It used +// to pass check AND exec ("Altered page …", exit 0) while leaving the DataView +// with a Forms$DataViewSource whose SourceVariable was null: DESCRIBE showed no +// datasource and mxbuild reported CE7007 on a page mxcli had called successful. +// +// The dry run is what makes check agree with exec here — one refusal in the +// mutator, reached by both, rather than a second copy of the rule in the +// validator. +func TestAlterSet_DatabaseDataSourceOnDataView(t *testing.T) { + ctx, deps := dataViewPageCtx(t) + + errs := checkAlterSet(t, ctx, + `alter page MyModule.P_View { set DataSource = DATABASE MyModule.Customer on dvCust; }`) + if len(errs) != 1 { + t.Fatalf("got %d errors, want 1: %v", len(errs), errs) + } + msg := strings.ToLower(errs[0].Error()) + for _, want := range []string{"data view", "database", "dvcust"} { + if !strings.Contains(msg, want) { + t.Errorf("error %q does not mention %q", errs[0], want) + } + } + if deps.saves != 0 { + t.Errorf("validation wrote to storage %d times, want 0", deps.saves) + } +} + +// Control: the sources a DataView really takes must still pass check, or the +// refusal has simply moved the failure rather than fixing it. +func TestAlterSet_ValidDataViewSourcesStillPass(t *testing.T) { + for _, src := range []string{ + `set DataSource = MICROFLOW MyModule.ACT_GetCustomer on dvCust;`, + `set DataSource = SELECTION lvOther on dvCust;`, + } { + t.Run(src, func(t *testing.T) { + ctx, _ := dataViewPageCtx(t) + errs := checkAlterSet(t, ctx, `alter page MyModule.P_View { `+src+` }`) + if len(errs) != 0 { + t.Fatalf("valid datasource reported: %v", errs) + } + }) + } +} From 3c1bc5b8d014faffbb8457e0114760699fe07a7d Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 20 Sep 2026 13:13:34 +0000 Subject: [PATCH 02/10] fix(lint): report the legacy image widgets, and correct when CE0582 applies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two halves of one concern: mxcli should not quietly help you author a widget your app cannot build. #518 and #538 made `staticimage` and `dynamicimage` round-trip, which was right — a project being converted up already contains them, and the pre-fix describe -> exec deleted them. But the same work made both materially easier to author (Image:, DataSource:, DefaultImage:, thumbnail, enlarge all newly reachable) while nothing warned the author. Measured: no validator mentions either widget, so the only signal was CE0582 at the far end of a build. MPR012 reports them. The linter and not `mxcli check`, deliberately: `check` validates a script, and describe -> exec of a legacy page is a legitimate lossless operation that a warning would flag every time — a rule that fires on correct work is noise. `lint` audits the project, where "this page holds a widget your client cannot render" is wanted once. The marketplace exclusion comes free and is the part worth measuring: ctx.Widgets() already filters any module with a Source, so the rule never fires on the Studio Pro static images a blank app inherits from FeedbackModule — content the reader cannot fix and an update would replace. Measured on a blank 11.12.1 app carrying both widget kinds: 8 legacy image widgets in the BSON, 7 indexed by the catalog, 5 in the user's own module, and lint reported exactly those 5. A deny-list of exactly two storage names, never an allow-list: the one widget such a rule must never fire on is the pluggable Image — the replacement it recommends. Also corrects a version claim carried in three places, including two I wrote. The reference guide says the React client was added in **10.7**: "The Dynamic Image widget, which is not supported by the React client added to Mendix in 10.7, can be converted to an Image widget through the context menu of the widget when the React client is enabled." So CE0582 fires on 10.7+ wherever that client is enabled, not "in the Mendix 11 React client". A version boundary copied from a sibling comment rather than from the vendor doc is the same class of error as a floor copied from a proposal's sample output. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016JgBheMTV6UiQstQ2nLyay --- .claude/commands/mendix/lint.md | 1 + .../fix-issue/findings/mdl-executor.jsonl | 1 + .../mendix/create-page/reference/widgets.md | 16 ++- cmd/mxcli/cmd_lint.go | 1 + cmd/mxcli/syntax/features_page.go | 8 +- .../modelsdk/widget_write_legacy_gaps.go | 15 ++- mdl/executor/cmd_lint.go | 2 + mdl/linter/rules/legacy_image_widget.go | 127 ++++++++++++++++++ mdl/linter/rules/legacy_image_widget_test.go | 78 +++++++++++ 9 files changed, 235 insertions(+), 14 deletions(-) create mode 100644 mdl/linter/rules/legacy_image_widget.go create mode 100644 mdl/linter/rules/legacy_image_widget_test.go diff --git a/.claude/commands/mendix/lint.md b/.claude/commands/mendix/lint.md index 94a383f109..0b09c790c4 100644 --- a/.claude/commands/mendix/lint.md +++ b/.claude/commands/mendix/lint.md @@ -40,6 +40,7 @@ mxcli lint -p app.mpr --exclude System --exclude Administration | MPR005 | quality | ImageSource - IMAGE widgets with no source configured | | MPR006 | quality | EmptyContainer - Empty layout containers | | MPR007 | security | PageNavigationSecurity - Navigation pages need allowed roles (CE0557) | +| MPR012 | correctness | LegacyImageWidget - staticimage/dynamicimage are unsupported by the React client (CE0582) | | SEC001 | security | NoEntityAccessRules - Persistent entities need access rules | | SEC002 | security | WeakPasswordPolicy - Password minimum length should be 8+ | | SEC003 | security | DemoUsersActive - Demo users should be off at Production security | diff --git a/.claude/skills/fix-issue/findings/mdl-executor.jsonl b/.claude/skills/fix-issue/findings/mdl-executor.jsonl index e21645d673..4cff4eeffc 100644 --- a/.claude/skills/fix-issue/findings/mdl-executor.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-executor.jsonl @@ -658,3 +658,4 @@ {"area": "mdl/executor", "date": "2026-09-20", "symptom": "An input widget cannot bind an attribute over an association. `textbox t (attribute: Assoc/Attr)` builds a FLAT path and mxbuild fails **CE1613** \"The selected attribute 'Rules.RuleAction.RuleAction_BusinessRule/Name' no longer exists.\" — note the shape: the association segment is pasted onto the CONTEXT entity instead of being navigated. The same syntax works on a DataGrid2 column, so one page could bind an associated attribute in a grid column and fail on the text box beside it. Read half: DESCRIBE emitted a bare `Attribute: Name`, so describe → exec over a Studio Pro page REBOUND the widget to an attribute the context entity does not have, with `mxcli check` clean", "cause": "All six input builders (textbox/textarea/datepicker/dropdown/checkbox/radiobuttons) called `resolveAttributePath`, which knows nothing about associations; `resolveAssociationAttributePath` existed and was wired only into DataGrid2 columns, DynamicText params and the widget engine. On the read side `extractAttributeRef` returned the last segment of AttributeRef.Attribute and ignored AttributeRef.EntityRef entirely, while `columnAttributeFromRef` already handled it correctly — two readers of one BSON shape", "file": "`mdl/executor/cmd_pages_builder_v3.go` (`resolveInputAttribute`), `cmd_pages_builder_v3_widgets.go` (six builders), `cmd_pages_describe_parse.go` (`extractAttributeRef`), `mdl/backend/modelsdk/widget_write.go` (`inputAttributeRefToGen`), `sdk/pages/pages_widgets_input.go` (AttributeRefSteps on six structs)", "insight": "**Mendix DOES permit this on a plain text box** — the question that blocked the fix, settled by a Studio Pro reference rather than reasoning: ako/TestApp `Rules.RuleAction_NewEdit` textBox4 stores Attribute \"Rules.BusinessRule.Name\" + IndirectEntityRef over Rules.RuleAction_BusinessRule. Without it the plausible fix was the opposite one (refuse it, hint at a nested dataview). **Fix both halves together or the round trip just changes failure mode** — a describer emitting a path the builder cannot consume turns silent corruption into a build error. The build-level control is the evidence that counts: same script, same mxbuild 11.14.0, pre-fix binary → CE1613, post-fix → 0 errors. An entity with NO attribute of that name is what makes the control sharp, since a same-named attribute would merely bind somewhere else", "refs": ["#529"], "ce": ["CE1613"]} {"area": "mdl/executor", "date": "2026-09-20", "symptom": "`CREATE OR MODIFY EXTERNAL ENTITIES FROM` marks an external entity's KEY attribute Updatable=true when the entity set's UpdateRestrictions say the set is updatable. Mendix computes a key as non-updatable, so `mx check` reports CE6630 \"'DefinitionId' is marked Updatable=False in the OData service, but True in the app.\" at Attribute 'MyFirstModule.Definition.DefinitionId'. Independent of the ComplexType flattening fixed in #1118 — reproduces on a contract with no complex type at all", "cause": "createExternalEntities' attribute loop started `updatable` from the entity set's UpdateRestrictions/Updatable and cleared it only for NonUpdatableProperties, Core.Computed, Core.Immutable or a flattened complex leaf. Key membership was already in the loop as keyPropSet[p.Name] — passed to edmToDomainModelAttrType, never consulted for updatability. The rule is gated on isTopLevel: only the key of an entity that HAS an entity set is non-updatable", "file": "`mdl/executor/cmd_contract.go` (createExternalEntities attribute loop; `isKey` now drives both the attribute type and Updatable)", "insight": "**The rule is `isKey && isTopLevel`, and the `isTopLevel` half cost a red CI run to learn.** The first fix was the blanket 'a key is never updatable' — every unit test green, the reported CE6630 gone, a real 11.12.1 build confirming it, and it turned ONE error into SEVEN of its inverse on the live TripPin contract: \"'TripId' is marked Updatable=True in the OData service, but False in the app.\" over Trip, PlanItem, Event, Flight, PublicTransportation, Employee and Manager — every one a derived or contained type with NO entity set, mutated through its parent's write flow — while Person/Airline/Airport (the entity sets) stayed silent at false. **`UserName` is the two-sided control inside one document**: expected False on Person and True on Employee and Manager, so the split is the entity set and cannot be inheritance or the property. **#1118's finding said this in advance and it was not heeded: 'Do not stop at a synthetic fixture. TripPin is the fixture to reach for.'** Seven synthetic annotation shapes agreed with each other and were all top-level, so the probe could not see the variable that mattered — a negative that is uniform across a whole class is evidence about the query. Cheapest guard for next time: `go test -tags integration -run 'TestMxCheck_DoctypeScripts/10-odata-examples'` is ~36s locally against cached mxbuild, versus a 12-minute CI round. **Clear Updatable on a key, NOT Creatable** — a key is written once at creation, so Insertable still applies; the report's build flagged it Updatable=False only, and clearing both is CE6630 inverted on the key of any insertable set. **The report's own error count was wrong**, which is why the real build mattered: it promises \"exactly one CE6630, naming the key attribute\"; the identical contract gives TWO — the key AND the non-key `Label`. The fix takes 2 to 1, not to 0, so a regression test asserting `mx check` clean would have failed against a correct fix. **The Label half is a SECOND defect, still open**: mxbuild answered Updatable=False for the non-key attribute of all seven TOP-LEVEL shapes (inline record, ``, UpdateMethod=PATCH, +NonUpdatableProperties/+DeleteRestrictions, unannotated, external ``, property-level Core.Permissions/ReadWrite). The evidence now points at `Updatable == !isTopLevel` for ALL attributes — the aliased run is the discriminator: with mxbuild reading an Updatable=true set and the app at false on every attribute, there were Creatable errors and NO Updatable error — but it was not broadened, because no top-level set has yet been seen that mxbuild treats as having an updatable attribute, and §48 measured CE6630 firing both ways. Two wrong turns ruled out by measurement, each cheap and each tempting: (1) the stored $metadata is NOT filtered — `grep -rc UpdateRestrictions mprcontents/` matches the InsertRestrictions count exactly, so mxbuild reads both from the same document and applies an extra rule to updatability only; (2) there is no entity-level updatable to set — `generated/metamodel` (the arbiter) gives Rest$ODataRemoteEntitySource Creatable/Deletable/Countable/Skip/Top and **no Updatable**, confirming the existing code comment, and mxbuild checks entity-level Creatable (it reported one) but never entity-level Updatable. Also measured: mxcli matches capability terms by fully-qualified name only, so the **aliased** spelling (`Capabilities.UpdateRestrictions` with an `/`) — what most real services emit — parses in mxbuild and not in mxcli, silently flipping every capability to the conservative default; the external `` form DOES parse. And `describe external entity` does not round-trip any per-attribute capability: it emits `Name: Type` only (losing Updatable, Creatable, Filterable, Sortable and the String length), so describe->exec into a fresh project rebuilds them from defaults. Repro `mdl-examples/bug-tests/odata-key-attribute-updatable.mdl`; tests `mdl/executor/cmd_contract_key_updatable_test.go` (the top-level and derived-type pair is the two-sided control — either alone passes against a fix wrong in the other direction)", "file_refs": ["mdl/executor/cmd_contract.go"], "ce": ["CE6630"]} {"area":"mdl/executor","date":"2026-09-20","symptom":"`CREATE OR MODIFY EXTERNAL ENTITIES FROM` marks NON-KEY attributes of a top-level external entity Updatable=true when the entity set's UpdateRestrictions say the set is updatable. `mx check` reports one CE6630 per attribute: \"'Label' is marked Updatable=False in the OData service, but True in the app.\" The sibling of the key-attribute defect fixed the same day; fixing only the key took the reported repro from 2 errors to 1, not to 0","cause":"`createExternalEntities` derived `defaultUpdatable` from `entitySet.Updatable`. Mendix does not: it computes Updatable as a function of whether the entity has an entity set at all — never for a top-level entity, always for a non-top-level one, which is written through its parent's flow. The annotation is irrelevant to it","file":"`mdl/executor/cmd_contract.go` (`defaultUpdatable := !isTopLevel`, replacing the UpdateRestrictions override; the key-specific guard added earlier becomes subsumed)","insight":"**The positive control that closes this is a contract whose `NonUpdatableProperties` names ONLY the key.** That service is asserting, by name, that every other property IS updatable — and mxbuild still answers False. Ten top-level shapes were probed in three rounds and all answered False (inline record, typed ``, UpdateMethod=PATCH, +NonUpdatableProperties +DeleteRestrictions, unannotated, external ``, Core.Permissions/ReadWrite, Core.OptimisticConcurrency/ETag, DeepUpdateSupport, and the key-only exclusion list); the first seven were NOT enough to act on, because 'no shape produces True' is the uniform-negative shape that means the probe is wrong — what made it actionable was a shape that states the opposite explicitly and is still refused. **Two false leads, each cheap and each worth skipping.** (1) ETag/optimistic concurrency, suggested by a comment in this very function about the service that motivated #729 — no effect. (2) The model's `AllowCreateChangeLocally`: the intuition is that Mendix would permit attribute changes once local changes are allowed, and it is wrong — setting it Yes on a top-level entity left the expectation at False. **That second control is the one that explains the rule rather than just fitting it**: an external object CAN be changed in memory and handed to an external OData action, and that is what the local-change flag governs; the attribute's `Updatable` mirrors only what the endpoint itself accepts on a PATCH. Domain knowledge from the maintainer, not derivable from the metamodel — worth asking for before probing an eleventh contract shape. The Insert/Update asymmetry that makes this look like a parser bug is real and is not one: mxbuild reads `InsertRestrictions` from the same document, in the same shapes, and honours it — so `Creatable` follows the contract and `Updatable` does not, which is also why 'the entity is read-only' is the wrong summary and why every test here asserts Creatable as its control. **A stale test encoded the old belief and had to be corrected, not worked around**: #1118's `TestCreateExternalEntities_FlattenedAttributesAreReadOnly` asserted `Label` was Creatable AND Updatable as its control; the Updatable half had been assumed from the contract rather than measured, while the flattened-attribute half it was controlling for HAD been. The control still works on Creatable alone. Verified end to end on a real 11.12.1 project: 2 errors before any fix, 1 after the key-only fix, **0 errors** now; TripPin (`-run 'TestMxCheck_DoctypeScripts/10-odata-examples'`, ~26s locally) is the other-direction control and stays green, since every entity it flags is non-top-level. Repro `mdl-examples/bug-tests/odata-key-attribute-updatable.mdl`; tests `mdl/executor/cmd_contract_key_updatable_test.go`","file_refs":["mdl/executor/cmd_contract.go"],"ce":["CE6630"]} +{"area":"mdl/linter","date":"2026-09-20","symptom":"mxcli happily authors `staticimage`/`dynamicimage` with nothing warning the author, and #518/#538 had just made both MORE capable (Image:, DataSource:, DefaultImage:, thumbnail, enlarge all newly reachable). The only signal that these widgets do not work was CE0582 at the far end of a build. Separately, three places in code comments and user-facing help claimed the deprecation was 'Mendix 11's React client'","cause":"Two things. (1) No validator or lint rule mentioned either widget (measured: `grep -rln deprecat mdl/executor/validate*.go` matched only an unrelated test), so the deprecation lived entirely in prose. (2) The version claim was inherited from a pre-existing comment and repeated without checking the doc: docs.mendix.com/refguide/image-viewer/ says the React client was added in **10.7**, so CE0582 fires on 10.7+ wherever that client is enabled, not only on 11","file":"`mdl/linter/rules/legacy_image_widget.go` (new, MPR012) + registration in `mdl/executor/cmd_lint.go` (x2) and `cmd/mxcli/cmd_lint.go`; wording in `mdl/backend/modelsdk/widget_write_legacy_gaps.go`, `.claude/skills/mendix/create-page/reference/widgets.md`, `cmd/mxcli/syntax/features_page.go`; table row in `.claude/commands/mendix/lint.md`","insight":"**A deprecation warning belongs in `lint`, not in `check`.** `check` validates a script, and `describe page` -> `exec` of a legacy page is a legitimate lossless operation — a check warning would fire on correct work every time, which is the noise trap MDL-WIDGET23's own history records. `lint` audits the project, where 'this page holds a widget your client cannot render' is wanted once. **The marketplace exclusion came free and is the control worth measuring**: `ctx.Widgets()` filters any module with a Source (notPlatformModule), so the rule never fires on the Studio Pro static images a blank app inherits from FeedbackModule — content the reader cannot fix and an update would replace. Measured: 8 legacy image widgets in the project, 7 indexed by the catalog, 5 in the user's own module, and lint reported exactly those 5. **A deny-list of two, never an allow-list**: the one widget such a rule must never fire on is the pluggable Image, i.e. the replacement it recommends. **'Deprecated in version X' needs the vendor doc, not the previous comment** — this repo had carried 'Mendix 11' for as long as the widgets had been written, and one fetch of the reference guide moved it to 10.7. A version boundary copied from a sibling comment is the same class of error as a floor copied from a proposal's sample output (mendixlabs/mxcli#1121)","refs":["mendixlabs/mxcli#1057"],"ce":["CE0582"],"rules":["MPR012"]} diff --git a/.claude/skills/mendix/create-page/reference/widgets.md b/.claude/skills/mendix/create-page/reference/widgets.md index 4b7ff4f05b..843c959d2e 100644 --- a/.claude/skills/mendix/create-page/reference/widgets.md +++ b/.claude/skills/mendix/create-page/reference/widgets.md @@ -698,10 +698,13 @@ staticimage imgAllSelected (Image: 'MyFirstModule.Images.gallery') auto units and a responsive image — which `describe page` also omits, so a round trip neither loses them nor invents them. -Mendix 11's React client reports **CE0582** for `staticimage` wherever it -appears — it is deprecated in favour of the pluggable `image` widget, which -takes the same `Image:`. mxcli still writes it, because round-tripping a model -that already contains one is the point; prefer `image` on a new page. +**CE0582** is reported for `staticimage` wherever it appears, by any app running +the React client — which Mendix added in **10.7** and which is the only client on +11, so this is not a Mendix 11 rule. The replacement is the pluggable `image` +widget, which takes the same `Image:`; Studio Pro offers the conversion from the +CE0582 error's context menu. mxcli still writes it, because round-tripping a +model that already contains one is the point — and `mxcli lint` reports it as +**MPR012** so a new page does not reach for it by accident. #### `DataSource:` — which object a DYNAMICIMAGE shows @@ -729,8 +732,9 @@ same three-part way as `staticimage`'s `Image:`. `WidthUnit:`/`HeightUnit:`, written; leave them out for Mendix's defaults (auto, responsive, full size, no enlarge), which `describe page` also omits. -CE0582 applies here too — `dynamicimage` is deprecated alongside `staticimage`, -and the pluggable `image` widget is the replacement for both. +CE0582 applies here too — the React client supports neither legacy image widget, +and the pluggable `image` widget is the replacement for both. `mxcli lint` reports +either as **MPR012**. #### Setting Image Source (PLUGGABLEWIDGET syntax) diff --git a/cmd/mxcli/cmd_lint.go b/cmd/mxcli/cmd_lint.go index c46382750d..a517cc5f10 100644 --- a/cmd/mxcli/cmd_lint.go +++ b/cmd/mxcli/cmd_lint.go @@ -381,6 +381,7 @@ func builtinLintRules() []linter.Rule { rules.NewDomainModelSizeRule(), rules.NewValidationFeedbackRule(), rules.NewImageSourceRule(), + rules.NewLegacyImageWidgetRule(), rules.NewEmptyContainerRule(), rules.NewGallerySelectionListenerRule(), rules.NewDataViewLayoutGridRule(), diff --git a/cmd/mxcli/syntax/features_page.go b/cmd/mxcli/syntax/features_page.go index c4609994a0..e59fd13056 100644 --- a/cmd/mxcli/syntax/features_page.go +++ b/cmd/mxcli/syntax/features_page.go @@ -166,9 +166,11 @@ CREATE PAGE Sales.Detail (Title: 'Detail', Layout: Atlas_Core.Atlas_Default) { "-- so a stored one round-trips through DESCRIBE (mendixlabs/mxcli#1057). Without\n" + "-- it the widget is written with no image and mxbuild reports CE0436:\n" + "STATICIMAGE imgLogo (Image: 'MyModule.Images.logo', Width: 64, Height: 64)\n\n" + - "-- Deprecated in the Mendix 11 React client. These are written correctly by\n" + - "-- both engines, but mxbuild reports CE0582 (\"not supported in React client\")\n" + - "-- on each, so prefer the alternative:\n" + + "-- Not supported by the React client — added in Mendix 10.7, and the only\n" + + "-- client on 11, so this is not a Mendix 11 rule. These are written correctly,\n" + + "-- but mxbuild reports CE0582 (\"not supported in React client\") on each\n" + + "-- wherever that client is enabled, and `mxcli lint` reports them as MPR012.\n" + + "-- Prefer the alternative:\n" + "-- STATICIMAGE -> IMAGE\n" + "-- DYNAMICIMAGE -> IMAGE\n" + "-- DROPDOWN -> COMBOBOX\n" + diff --git a/mdl/backend/modelsdk/widget_write_legacy_gaps.go b/mdl/backend/modelsdk/widget_write_legacy_gaps.go index 89a3fa16b7..5043cddf26 100644 --- a/mdl/backend/modelsdk/widget_write_legacy_gaps.go +++ b/mdl/backend/modelsdk/widget_write_legacy_gaps.go @@ -97,11 +97,16 @@ func dropDownToGen(dd *pages.DropDown) (element.Element, error) { // staticImageToGen builds a Forms$StaticImageViewer. // -// Deprecated in the Mendix 11 React client (CE0582) — `image` routes to the -// pluggable widget instead — but `staticimage` is still a keyword the executor -// dispatches, so the writer has to answer for it. Unlike `statictext` the TYPE -// exists: the project loads, and CE0582 is Mendix's own advice rather than a -// defect, so refusing it would be over-reach. +// Not supported by the React client — which Mendix added in 10.7 and which is the +// only client on 11 — so mxbuild reports CE0582 wherever that client is enabled, +// and `image` routes to the pluggable widget instead. +// +// `staticimage` is still a keyword the executor dispatches, so the writer has to +// answer for it. Unlike `statictext` the TYPE exists: the project loads, and +// CE0582 is Mendix's own advice rather than a defect, so refusing it would be +// over-reach. `mxcli lint` reports the widget as MPR012 instead, which is where +// a deprecation belongs — a `check` warning would fire on every legitimate +// describe -> exec of a legacy page. func staticImageToGen(img *pages.StaticImage) (element.Element, error) { g := genPg.NewStaticImageViewer() applyWidgetBase(g, &img.BaseWidget) diff --git a/mdl/executor/cmd_lint.go b/mdl/executor/cmd_lint.go index a57edc3606..2a56c6af7a 100644 --- a/mdl/executor/cmd_lint.go +++ b/mdl/executor/cmd_lint.go @@ -35,6 +35,7 @@ func execLint(ctx *ExecContext, s *ast.LintStmt) error { rules.NewDomainModelSizeRule(), rules.NewValidationFeedbackRule(), rules.NewImageSourceRule(), + rules.NewLegacyImageWidgetRule(), rules.NewMissingTranslationsRule(), rules.NewGallerySelectionListenerRule(), rules.NewDataViewLayoutGridRule(), @@ -150,6 +151,7 @@ func listLintRules(ctx *ExecContext) error { lint.AddRule(rules.NewDomainModelSizeRule()) lint.AddRule(rules.NewValidationFeedbackRule()) lint.AddRule(rules.NewImageSourceRule()) + lint.AddRule(rules.NewLegacyImageWidgetRule()) lint.AddRule(rules.NewMissingTranslationsRule()) lint.AddRule(rules.NewGallerySelectionListenerRule()) lint.AddRule(rules.NewDataViewLayoutGridRule()) diff --git a/mdl/linter/rules/legacy_image_widget.go b/mdl/linter/rules/legacy_image_widget.go new file mode 100644 index 0000000000..6c289240dc --- /dev/null +++ b/mdl/linter/rules/legacy_image_widget.go @@ -0,0 +1,127 @@ +// SPDX-License-Identifier: Apache-2.0 + +package rules + +import ( + "fmt" + + "github.com/mendixlabs/mxcli/mdl/linter" +) + +// LegacyImageWidgetRule reports Mendix's two legacy image widgets, neither of +// which the React client supports. +// +// # Why this is correctness, not style +// +// The React client arrived in Mendix 10.7 and is the only client on 11, and +// mxbuild reports CE0582 on each of these widgets whenever it is enabled — +// measured on 11.12.1, where a page carrying one is a build ERROR: +// +// [error] [CE0582] "Widget static image is not supported in React client. +// Right-click this error to convert it to an alternative +// widget type." at Static image 'imgLogo' +// +// Studio Pro offers the conversion from that error's context menu; the +// replacement is the pluggable Image widget, which mxcli spells `image` and +// which takes the same `Image:` reference. +// +// # Why mxcli writes them at all +// +// Because a project being converted UP already contains them, and mxcli's job +// there is to round-trip the model rather than quietly rewrite it. Both widgets +// are authorable and both round-trip through DESCRIBE. What was missing was +// anyone saying so at author time: `mxcli check` is silent on them, and the only +// signal was CE0582 at the far end of a build. +// +// # Why the LINTER and not `mxcli check` +// +// `check` validates a script, and `describe page` -> `exec` of a legacy page is +// a legitimate, lossless operation that a warning would flag every time — a +// rule that fires on correct work is noise. `lint` audits the project, where +// "this page still holds a widget your client cannot render" is exactly the +// finding wanted, once. +// +// Marketplace modules are already out of scope: ctx.Widgets() filters them +// (notPlatformModule excludes any module with a Source), which is what keeps the +// rule off the Studio Pro-authored static images a blank app inherits from +// FeedbackModule — content the reader cannot fix and an update would replace. +type LegacyImageWidgetRule struct{} + +// NewLegacyImageWidgetRule creates a new legacy image widget rule. +func NewLegacyImageWidgetRule() *LegacyImageWidgetRule { + return &LegacyImageWidgetRule{} +} + +func (r *LegacyImageWidgetRule) ID() string { return "MPR012" } +func (r *LegacyImageWidgetRule) Name() string { return "LegacyImageWidget" } +func (r *LegacyImageWidgetRule) Category() string { return "correctness" } +func (r *LegacyImageWidgetRule) DefaultSeverity() linter.Severity { return linter.SeverityWarning } + +func (r *LegacyImageWidgetRule) Description() string { + return "Checks for the legacy static/dynamic image widgets, which the React client does not support (CE0582)" +} + +// LegacyImage describes one of the two widgets, in the words the author will +// meet elsewhere: Mendix's own term (which CE0582 uses) and the MDL keyword. +type LegacyImage struct { + MendixTerm string + MDLKeyword string +} + +// legacyImageWidgets is deliberately a deny-list of exactly two storage names. +// An allow-list would make every unrecognised widget a violation, and the one +// widget it must never fire on is the pluggable Image — the replacement. +// +// DocumentTemplates$StaticImageViewer is a different type in a different +// document kind and is not covered: document templates are not pages, the React +// client does not render them, and CE0582 does not mention them. +var legacyImageWidgets = map[string]LegacyImage{ + "Forms$StaticImageViewer": {MendixTerm: "static image", MDLKeyword: "staticimage"}, + "Forms$ImageViewer": {MendixTerm: "dynamic image", MDLKeyword: "dynamicimage"}, +} + +// LegacyImageWidget reports whether widgetType is one of the two legacy image +// widgets, and how to name it. +func LegacyImageWidget(widgetType string) (LegacyImage, bool) { + got, ok := legacyImageWidgets[widgetType] + return got, ok +} + +// Check reports one violation per legacy image widget found. +func (r *LegacyImageWidgetRule) Check(ctx *linter.LintContext) []linter.Violation { + var violations []linter.Violation + + for w := range ctx.Widgets() { + if ctx.IsExcluded(w.ModuleName) { + continue + } + legacy, ok := LegacyImageWidget(w.WidgetType) + if !ok { + continue + } + + docType := "page" + if w.ContainerType == "SNIPPET" { + docType = "snippet" + } + + violations = append(violations, linter.Violation{ + RuleID: r.ID(), + Severity: r.DefaultSeverity(), + Message: fmt.Sprintf( + "%s '%s' in %s is not supported by the React client (Mendix 10.7+, the only client on 11) — mxbuild reports CE0582", + legacy.MendixTerm, w.Name, w.ContainerQualifiedName), + Location: linter.Location{ + Module: w.ModuleName, + DocumentType: docType, + DocumentName: docNameFromQualified(w.ContainerQualifiedName), + DocumentID: w.ContainerID, + }, + Suggestion: fmt.Sprintf( + "Replace `%s` with the pluggable `image` widget (same `Image:` reference), or convert it in Studio Pro from the CE0582 error's context menu", + legacy.MDLKeyword), + }) + } + + return violations +} diff --git a/mdl/linter/rules/legacy_image_widget_test.go b/mdl/linter/rules/legacy_image_widget_test.go new file mode 100644 index 0000000000..41025a884e --- /dev/null +++ b/mdl/linter/rules/legacy_image_widget_test.go @@ -0,0 +1,78 @@ +// SPDX-License-Identifier: Apache-2.0 + +package rules + +import "testing" + +// The two legacy image widgets are not supported by the React client, which +// Mendix added in 10.7 and which is the only client on 11. mxbuild reports +// CE0582 on each whenever it is enabled, so on a Mendix 11 app these are build +// ERRORS, not style points — measured on 11.12.1: +// +// [error] [CE0582] "Widget static image is not supported in React client. +// Right-click this error to convert it to an alternative +// widget type." at Static image 'imgLogo' +// +// mxcli can author both (it must: a project being converted up already contains +// them), and nothing told the author they were reaching for a widget their app +// cannot build. This rule is that telling. +func TestLegacyImageWidget_NamesBothTypesAndTheReplacement(t *testing.T) { + cases := []struct { + widgetType string + wantTerm string + wantMDL string + }{ + {"Forms$StaticImageViewer", "static image", "staticimage"}, + {"Forms$ImageViewer", "dynamic image", "dynamicimage"}, + } + for _, c := range cases { + t.Run(c.widgetType, func(t *testing.T) { + got, ok := LegacyImageWidget(c.widgetType) + if !ok { + t.Fatalf("%s not recognised as a legacy image widget", c.widgetType) + } + // Mendix's own term, so the lint message and the CE0582 the author + // will see from mxbuild use the same words. + if got.MendixTerm != c.wantTerm { + t.Errorf("MendixTerm = %q, want %q", got.MendixTerm, c.wantTerm) + } + if got.MDLKeyword != c.wantMDL { + t.Errorf("MDLKeyword = %q, want %q", got.MDLKeyword, c.wantMDL) + } + }) + } +} + +// The CONTROL, and the reason this is a deny-list of two rather than anything +// broader: every other widget must be silent. A rule that fires on the +// PLUGGABLE image — the widget it tells people to move TO — would be worse than +// no rule at all. +func TestLegacyImageWidget_IsSilentOnEverythingElse(t *testing.T) { + for _, widgetType := range []string{ + "CustomWidgets$CustomWidget", // the pluggable image lives here + "Forms$DynamicText", + "Forms$DataView", + "DocumentTemplates$StaticImageViewer", // a document template, not a page + "", + } { + if got, ok := LegacyImageWidget(widgetType); ok { + t.Errorf("%q was reported as a legacy image widget (%+v)", widgetType, got) + } + } +} + +// The rule's identity is part of its contract: an ID that collides with another +// rule's silently shadows it in the config, and the category decides what +// `--category` filters it into. +func TestLegacyImageWidgetRule_Identity(t *testing.T) { + r := NewLegacyImageWidgetRule() + if r.ID() != "MPR012" { + t.Errorf("ID = %q, want MPR012", r.ID()) + } + if r.Category() != "correctness" { + t.Errorf("Category = %q — CE0582 is a build error, not a style preference", r.Category()) + } + if r.Name() == "" || r.Description() == "" { + t.Error("a rule with no name or description cannot be configured or explained") + } +} From 9fb5b4c5ce828cbfeabd2dce488b2d1f169d7b19 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 20 Sep 2026 13:16:09 +0000 Subject: [PATCH 03/10] fix(pages): stop a page rewrite moving state nobody asked to change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit describe page → exec over a Studio Pro-authored page reported "Replaced", not "Unchanged", so ADR-0008's elision could not fire and the unit churned in version control on every re-run. mx check was 0 errors either way. Measured on ako/TestApp's Rules.RuleAction_NewEdit at 11.14.0: fourteen differences, in four independent classes, all "the rebuild writes a constant where Studio Pro stores a value". 1. Page header. pageToGen hardcoded Autofocus, CanvasWidth and CanvasHeight. Studio Pro varies all three per page — CanvasWidth takes seven distinct values across those 67 pages and the hardcoded 1200 matched four, so a round trip moved the canvas of the other 63. Now carried from the stored document on a rewrite; a new page, which has no stored document, still gets the defaults. 2. Client-action defaults. save_changes, cancel_changes, close_page and delete_object never wrote DisabledDuringExecution, which Studio Pro stores true on all 39 of them; save_changes wrote SyncAutomatically true where all 8 store false. 3. AttributeRef.EntityRef, present on 338 of 338 stored refs (313 null, 25 navigated), was emitted only on the navigated branch. 4. Forms$PageVariable: only the field carrying a value was set, so the other five keys were never marked dirty and the encoder omitted them. 3 and 4 go in the codec's TypeDefaults rather than at each construction site — PageVariable is built in three places — which needed one new kind, FalseFields, since a bool's zero value is never dirty. Result on that page: 14 differences → 1. The remaining one is a pluggable widget Object property, which is the CE0463 subsystem and needs its own investigation. Two things worth knowing, both learned the hard way here: mxcli round-tripping its own output proves nothing about this class. Measured: the MDL bug-test reports "Unchanged" on the unfixed build too, because mxcli writes the page and mxcli describes it. The reference has to be a Studio Pro document, which also limits the committed-fixture idea in the issue. The detecting evidence is the Go tests, each run against a stubbed-out fix. The first version of the header carry used a .(int32) assertion — the natural one, since the gen setter takes int32 — and matched nothing, because Studio Pro stores both dimensions as int64. Its unit test passed regardless, since the fixture wrote int32: the test encoded the assumption under test. The read is now width-agnostic and the test asserts both. Closes #541 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01L2aYb3zDscezm874CDTH6R --- .../fix-issue/findings/mdl-backend.jsonl | 1 + .../pages-541-roundtrip-property-drift.mdl | 105 ++++++++++ .../modelsdk/page_header_carry_test.go | 180 ++++++++++++++++++ mdl/backend/modelsdk/page_write.go | 74 +++++++ .../modelsdk/page_zero_value_keys_test.go | 110 +++++++++++ mdl/backend/modelsdk/widget_write.go | 39 +++- .../widget_write_action_defaults_test.go | 117 ++++++++++++ modelsdk/codec/defaults.go | 6 + modelsdk/codec/encoder.go | 5 + 9 files changed, 636 insertions(+), 1 deletion(-) create mode 100644 mdl-examples/bug-tests/pages-541-roundtrip-property-drift.mdl create mode 100644 mdl/backend/modelsdk/page_header_carry_test.go create mode 100644 mdl/backend/modelsdk/page_zero_value_keys_test.go create mode 100644 mdl/backend/modelsdk/widget_write_action_defaults_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-backend.jsonl b/.claude/skills/fix-issue/findings/mdl-backend.jsonl index 50ff6a767b..db58a8b03b 100644 --- a/.claude/skills/fix-issue/findings/mdl-backend.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-backend.jsonl @@ -117,3 +117,4 @@ {"area": "mdl/backend", "date": "2026-09-17", "symptom": "A microflow's **export level** (Studio Pro's Hidden/API switch \u2014 whether it is part of the module's public surface when the module is exported as a package) is reset to `Hidden` by any `CREATE OR MODIFY MICROFLOW`. Every checker stays green, because a hidden microflow is a valid microflow; the module's API is simply smaller", "cause": "`microflowToGen` wrote `out.SetExportLevel(\"Hidden\")` unconditionally, `microflowFromGen` never read it back, and `sdk/microflows.Microflow` had no field \u2014 the identical three-layer gap as the deep-link URL in the same function", "file": "`mdl/backend/modelsdk/microflow_write.go` (microflowToGen), `mdl/backend/modelsdk/microflow.go` (microflowFromGen), `mdl/executor/cmd_microflows_build.go`, `sdk/microflows/microflows.go`", "fix": "Same carry as the URL, plus a DEFAULT: `\"\"` is not a member of `MicroflowsExportLevel`, so an empty model value is written as `Hidden` rather than passed through (the precedent is `json_write.go`). DESCRIBE emits `-- Export level:` only when the value is not `Hidden`", "insight": "Found by running the mechanical audit the URL fix prompted \u2014 `grep 'out.Set.*(\"\\|(nil)' microflow_write.go` over the one function \u2014 which is the cheap move after any instance of this class and turned up three more constants in one line. **The measurement that shaped the fix**: three real marketplace modules (Business Events 3.12.0, External Database Connector 6.2.3/6.3.0) store `Hidden` on 3 of 3 microflows and 55 of 55 documents overall, all three exporting at module level `Source` \u2014 so the hardcoded value was not wrong, it was a default masquerading as a constant. That is the shape of the trap: the audit finds the constant, but only a reference document tells you whether to carry it, default it, or leave it alone. A marketplace `.mpk` is a free source of these \u2014 `unzip -o pkg.mpk project.mpr` gives a real Studio Pro-authored MPR to query, no Studio Pro and no network needed (`mx-modules/` holds three). **Never carry an enum-valued property straight through without a default**: a stored document that says nothing reads as `\"\"`, and writing `\"\"` back is precisely the unloadable-model write CLAUDE.md warns about \u2014 mxbuild tolerates it and Studio Pro throws at MprProperty.cs. Controls: pinning the writer back, stubbing the reader, and neutralising the executor carry each fail a different test with the reported symptom"} {"area": "mdl/backend", "date": "2026-09-17", "symptom": "Unit tests for a carried microflow property (URL, export level, concurrency) all pass, and the end-to-end behaviour against a real project is still unverified \u2014 the integration gate that would have caught it, `TestMxCheck_DoctypeScripts`, `t.Skip`s whenever `mx` is absent, which is every run in a fresh container", "cause": "Two separate measurement errors, both invisible to `go test`. (1) The test fixture paired `Url: \"item/{Key}\"` with `UrlSearchParameters: [\"\u2026.Key\"]` \u2014 the SAME parameter \u2014 which mxbuild rejects as **CE5612**: a parameter used in the URL path may not also be a search parameter. Nothing in a unit test validates the model, so the fixture described a document Mendix refuses to build. (2) `bin/mxcli` was stale: `go build ./mdl/...` and `make test` had been run after each fix, but not `make build`, so the end-to-end run exercised a binary predating two of the three commits", "file": "`mdl/backend/modelsdk/microflow_roundtrip_flags_test.go`, `mdl/executor/microflow_carried_properties_test.go`, `mdl/executor/roundtrip_doctype_test.go` (the skipping gate)", "fix": "Fixture uses a distinct `Filter` parameter and says why. End-to-end procedure that actually measures it: `mxcli setup mxbuild -p ` (~719 MB, works through the session proxy), copy `testdata/expr-checker` as the fixture, create the microflow with mxcli, seed the unauthorable properties straight into the stored unit with `mpr.NewWriter` + `UpdateRawUnit`, then `mx check` BEFORE (the fixture must be a document Mendix accepts, or it proves nothing), `mxcli exec` a body-only rewrite, read the unit back, `mx check` after", "insight": "**A skipping integration gate is worse than no gate**: `mxCheckAvailable()` + `t.Skip` means a green `make test` says nothing about mxbuild, and reading the CLAUDE.md line about #808 is not the same as checking whether it applies to your own run \u2014 `ls ~/.mxcli/mxbuild` is. **Rebuild the binary before any end-to-end run**, and check its mtime against the last commit: a stale `bin/mxcli` produced a result (URL survived, export level did not) that looked exactly like a genuine second-read-path defect, and sent me hunting for a duplicate resolver that does not exist. **Seed the fixture through the writer, not by hand-editing BSON**, and always `mx check` the seeded state first: the CE5612 error came from the seed, not from mxcli, and without the before-check it would have been misattributed to the fix. Measured, mxbuild 11.6.6: pre-fix binary rewrites the microflow to `Url=\"\"`, empty search params, `ExportLevel=\"Hidden\"`; post-fix keeps all three; `mx check` 0 errors on both the seeded control and the rewritten project"} {"area": "mdl/backend", "date": "2026-09-18", "symptom": "`ALTER PAGE M.P { SET Documentation = '...' }` is refused: \"unsupported page-level property: Documentation\". Documenting an existing page therefore means re-running its CREATE, because the `/** … */` doc comment on the create statement is the only other source", "cause": "`applyPageLevelSetMut` handled Title/Url/Popup*/Class/Style and had no Documentation case. The field was otherwise fully understood — `pageToGen` has always written it on CREATE via `out.SetDocumentation`, and gen binds it as `property.NewPrimitive[string](\"Documentation\")`. The grammar already parsed it (`identifierOrKeyword EQUALS propertyValueV3`) and the executor has no allowlist, so it was one missing case in the mutator, not a syntax gap", "file": "`mdl/backend/pagemutator/mutator.go` (`applyPageLevelSetMut`)", "insight": "One case covers **Page, Layout AND Snippet** — all three declare Documentation and all three reach this function through `SetWidgetProperty(\"\")`. Store an empty string rather than rejecting it: removing a doc comment from a script has to be expressible, and the property is a bare string with no unset value. **Update the unsupported-property message in the same change** — that list is the only guidance a reader gets, and a stale one sends them back to the CREATE workaround the fix exists to remove (a test asserts the message names Documentation). Note the re-run workaround is worse than it sounds: describe → exec is only as complete as what MDL can spell, so restating a page to document it can silently lose widgets", "refs": ["#527"]} +{"area": "mdl/backend", "date": "2026-09-20", "symptom": "`describe page` → `exec` over a **Studio Pro-authored** page reports `Replaced page`, not `Unchanged` — the rebuild is not semantically equal to what was stored, so ADR-0008's elision cannot fire and the unit churns in version control on every re-run. `mx check` is 0 errors either way. Measured on ako/TestApp Rules.RuleAction_NewEdit at 11.14.0: fourteen differences", "cause": "Four independent classes, all 'the rebuild writes a constant where Studio Pro stores a value': (1) `pageToGen` hardcoded Autofocus/CanvasWidth/CanvasHeight; (2) save_changes/cancel_changes/close_page/delete_object never wrote `DisabledDuringExecution`, and save_changes wrote `SyncAutomatically` true; (3) `AttributeRef.EntityRef` emitted only on the navigated branch; (4) `Forms$PageVariable` had only the one name field set, so the other five keys were never marked dirty", "file": "`mdl/backend/modelsdk/page_write.go` (`carryStoredPageHeader`, `bsonInt`), `widget_write.go` (four action cases + two `RegisterTypeDefaults`), `modelsdk/codec/defaults.go` + `encoder.go` (new `FalseFields`)", "insight": "**mxcli round-tripping its own output proves NOTHING about this class** — measured: the MDL bug-test reports `Unchanged` on the unfixed build too, because mxcli writes the page and mxcli describes it, so its constants agree with themselves. The reference must be a Studio Pro document; a committed CI fixture only works if it is one. **A population selected by name can confirm whatever it excluded**: the first sweep filtered `$Type` on `endswith(\"ClientAction\")`, got a tidy 'True on 82 of 82', and so missed `Forms$NoAction` (False on 83 of ~7,300) and `Forms$MicroflowAction` (5 of 81) — scan by the PROPERTY, not by a name pattern. **Hardcoded looked safe and was not**: CanvasWidth takes seven distinct values across 67 pages and the hardcoded 1200 matched 4, so a round trip moved the canvas of 63. **The int width bit this fix once**: Studio Pro stores both canvas dimensions as int64 while the gen setter takes int32, so the natural `.(int32)` assertion matched nothing — and the first unit test passed anyway because its own fixture wrote int32, i.e. the test encoded the assumption under test (bson-numeric-width). Prefer `TypeDefaults` over patching each construction site: `Forms$PageVariable` is built in three places. Remaining after the fix: 1 of 14, a pluggable-widget Object property — CE0463 territory, deliberately out of scope", "refs": ["#541", "#529"]} diff --git a/mdl-examples/bug-tests/pages-541-roundtrip-property-drift.mdl b/mdl-examples/bug-tests/pages-541-roundtrip-property-drift.mdl new file mode 100644 index 0000000000..5fc9218df1 --- /dev/null +++ b/mdl-examples/bug-tests/pages-541-roundtrip-property-drift.mdl @@ -0,0 +1,105 @@ +-- @version: 11.0+ +-- ============================================================================ +-- ako/mxcli#541 — `describe page` → `exec` over a Studio Pro page reported +-- "Replaced page", not "Unchanged page". Per ADR-0008 an in-sync unit is not +-- written at all, so "Replaced" means the rebuild was not semantically equal to +-- what Studio Pro stored — the round trip was lossy, with `mx check` clean +-- either way. +-- +-- Measured on ako/TestApp's Rules.RuleAction_NewEdit at Mendix 11.14.0, the +-- round trip differed in FOURTEEN places. Four classes, each fixed by measuring +-- what Studio Pro actually stores across all 67 of that app's pages: +-- +-- 1. Page header constants. pageToGen hardcoded Autofocus/CanvasWidth/ +-- CanvasHeight. Studio Pro varies all three per page — CanvasWidth took +-- SEVEN distinct values, and the hardcoded 1200 matched 4 of 67, so a round +-- trip moved the canvas of the other 63. Now carried from the stored +-- document on a rewrite (a new page still gets the defaults). +-- +-- 2. Client-action defaults. save_changes / cancel_changes / close_page / +-- delete_object never wrote DisabledDuringExecution, which Studio Pro +-- stores true on all 39 of them; save_changes wrote SyncAutomatically true +-- where all 8 store false. +-- +-- 3. AttributeRef.EntityRef. Present on 338 of 338 stored AttributeRefs (313 +-- null, 25 navigated); mxcli emitted it only on the navigated branch. +-- +-- 4. Forms$PageVariable. Studio Pro writes all six keys; mxcli set only the +-- one carrying a value, so the other five were never marked dirty and the +-- encoder omitted them. +-- +-- 3 and 4 are registered in the codec's TypeDefaults rather than patched at +-- each construction site, which is the mechanism that already handles an +-- association's null Source. +-- +-- Result on that page: 14 differences → 1. The one remaining is a pluggable +-- widget Object property (a combobox), which is the CE0463 subsystem and needs +-- its own investigation. +-- +-- EXPECTED: `mx check` reports 0 errors. +-- +-- WHAT THIS FILE IS NOT: a detector. A describe → exec of the page below +-- reports "Unchanged" on BOTH the fixed and the unfixed build — measured, not +-- assumed — because mxcli writes the page and mxcli describes it, so its own +-- constants agree with themselves. The bug only appears against a document +-- STUDIO PRO wrote, where mxcli's constants differ from the stored values. +-- +-- So this is a reproduction scaffold: it exercises all four classes in one +-- page, which is what makes it useful for eyeballing the BSON or re-running +-- the measurement. The evidence that the fix detects anything is the Go tests +-- in mdl/backend/modelsdk (each run against a stubbed-out fix) plus the +-- measurement on ako/TestApp, 14 differences → 1. +-- +-- The same caveat applies to the issue's suggestion of a committed CI fixture: +-- a page mxcli creates cannot catch this class. The fixture has to be a +-- Studio Pro-authored document. +-- ============================================================================ + +create module Issue541; + +@position(100, 100) +create persistent entity Issue541.BusinessRule ( + Name: string(200) +); + +@position(400, 100) +create persistent entity Issue541.RuleAction ( + ActionType: string(200) +); + +create association Issue541.RuleAction_BusinessRule + from Issue541.RuleAction to Issue541.BusinessRule; + +create or replace page Issue541.RuleAction_NewEdit ( + title: 'Edit Rule Action', + layout: Atlas_Core.PopupLayout, + params: { $RuleAction: Issue541.RuleAction } +) +{ + layoutgrid layoutGrid1 { + row row1 { + column col1 (DesktopWidth: AutoFill) { + -- A page-parameter datasource: exercises Forms$PageVariable (class 4). + dataview dvMain (datasource: $RuleAction) { + -- A plain binding: exercises AttributeRef.EntityRef = null (class 3). + textbox tbActionType (label: 'Action type', attribute: ActionType) + -- A navigated binding: EntityRef must NOT be flattened to null. + textbox tbRuleName (label: 'Rule', attribute: RuleAction_BusinessRule/Name) + + -- All four actions whose DisabledDuringExecution was missing, plus + -- save_changes' SyncAutomatically (class 2). + footer footerButtons { + actionbutton btnSave (caption: 'Save', action: save_changes close_page, buttonstyle: Success) + actionbutton btnCancel (caption: 'Cancel', action: cancel_changes close_page) + actionbutton btnClose (caption: 'Close', action: close_page) + actionbutton btnDelete (caption: 'Delete', action: delete_object) + } + } + } + } + } +} + +-- Emitted so the round trip can be inspected by hand. Note the caveat above: +-- re-executing this output reports "Unchanged" on an unfixed build too. +describe page Issue541.RuleAction_NewEdit; diff --git a/mdl/backend/modelsdk/page_header_carry_test.go b/mdl/backend/modelsdk/page_header_carry_test.go new file mode 100644 index 0000000000..e7016225c7 --- /dev/null +++ b/mdl/backend/modelsdk/page_header_carry_test.go @@ -0,0 +1,180 @@ +// SPDX-License-Identifier: Apache-2.0 + +// ako/mxcli#541 (header half) — a describe → exec round trip of a Studio Pro page +// reports "Replaced page", not "Unchanged page", because pageToGen writes three +// editor properties as constants instead of carrying the stored values: +// +// Autofocus Off → DesktopOnly +// CanvasWidth 800 → 1200 +// +// These are not constants in Studio Pro. Measured across all 67 pages of +// ako/TestApp at 11.14.0: +// +// CanvasWidth 800 ×33, 1198 ×20, 1200 ×4, 802 ×2, 900 ×2, 2000 ×1, 1800 ×1 +// Autofocus DesktopOnly ×58, Off ×9 +// CanvasHeight 600 ×66, 500 ×1 +// +// mxcli's hardcoded 1200 matches 4 of 67, so a round trip moved the canvas of 63 +// of them. MDL has no spelling for any of the three and nobody asks to change +// them, which is what makes this carry-through rather than new syntax. +package modelsdkbackend + +import ( + "testing" + + "go.mongodb.org/mongo-driver/bson" + + "github.com/mendixlabs/mxcli/mdl/backend/bsonnav" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/pages" +) + +// storedHeader reads the three properties straight off the stored unit, rather +// than through GetPage — the semantic model does not carry them, which is the +// whole reason they were lost. +// +// The dimensions are read width-agnostically for the same reason the fix is: +// asserting one width here would re-encode the assumption under test. +func storedHeader(t *testing.T, b *Backend, id model.ID) (autofocus string, width, height int64) { + t.Helper() + raw, err := b.reader.GetRawUnitBytes(string(id)) + if err != nil { + t.Fatalf("GetRawUnitBytes: %v", err) + } + var d bson.D + if err := bson.Unmarshal(raw, &d); err != nil { + t.Fatalf("unmarshal: %v", err) + } + return bsonnav.DGetString(d, "Autofocus"), + bsonInt(bsonnav.DGet(d, "CanvasWidth")), + bsonInt(bsonnav.DGet(d, "CanvasHeight")) +} + +// setStoredHeader rewrites the three properties on the stored unit, standing in +// for a page Studio Pro authored with non-default editor state. +// +// The caller chooses the numeric width, because it is load-bearing: Studio Pro +// stores both canvas dimensions as int64, and the first version of this fix +// passed a test whose fixture wrote int32 while moving the canvas of every real +// document. See TestUpdatePage_CarriesStoredCanvasAtEitherWidth. +func setStoredHeader(t *testing.T, b *Backend, id model.ID, autofocus string, width, height any) { + t.Helper() + raw, err := b.reader.GetRawUnitBytes(string(id)) + if err != nil { + t.Fatalf("GetRawUnitBytes: %v", err) + } + var d bson.D + if err := bson.Unmarshal(raw, &d); err != nil { + t.Fatalf("unmarshal: %v", err) + } + bsonnav.DSet(d, "Autofocus", autofocus) + bsonnav.DSet(d, "CanvasWidth", width) + bsonnav.DSet(d, "CanvasHeight", height) + out, err := bson.Marshal(d) + if err != nil { + t.Fatalf("marshal: %v", err) + } + if err := b.writer.UpdateRawUnit(string(id), out); err != nil { + t.Fatalf("UpdateRawUnit: %v", err) + } +} + +func headerFixture(t *testing.T) (*Backend, *pages.Page) { + t.Helper() + b := New() + if err := b.Connect(copyFixture(t)); 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) + } + page := &pages.Page{ContainerID: mod.ID, Name: "ZzHeaderPage", URL: "zz-header"} + if err := b.CreatePage(page); err != nil { + t.Fatalf("CreatePage: %v", err) + } + return b, page +} + +// A rewrite must leave the stored editor state alone. +func TestUpdatePage_CarriesStoredHeaderProperties(t *testing.T) { + b, page := headerFixture(t) + + // Stand in for a Studio Pro page: values that differ from every mxcli + // default, so nothing here can pass by coincidence, at the width Studio Pro + // actually writes (int64, measured on all 67 TestApp pages). + setStoredHeader(t, b, page.ID, "Off", int64(800), int64(500)) + + page.URL = "zz-header-updated" + if err := b.UpdatePage(page); err != nil { + t.Fatalf("UpdatePage: %v", err) + } + + autofocus, width, height := storedHeader(t, b, page.ID) + if autofocus != "Off" { + t.Errorf("Autofocus = %q, want Off — the stored value was overwritten", autofocus) + } + if width != 800 { + t.Errorf("CanvasWidth = %d, want 800 — the stored value was overwritten", width) + } + if height != 500 { + t.Errorf("CanvasHeight = %d, want 500 — the stored value was overwritten", height) + } + + // Control: the property the statement DOES author still lands, so this is + // not "the update stopped writing anything". + got, err := b.GetPage(page.ID) + if err != nil { + t.Fatalf("GetPage: %v", err) + } + if got.URL != "zz-header-updated" { + t.Errorf("URL = %q, want zz-header-updated — the authored change was lost", got.URL) + } +} + +// Control: a genuinely NEW page has no stored document to carry from, so it +// still gets the defaults. Without this, "carry the stored value" could be +// implemented as "never write these", leaving a new page with no canvas at all. +func TestCreatePage_StillWritesHeaderDefaults(t *testing.T) { + b, page := headerFixture(t) + + autofocus, width, height := storedHeader(t, b, page.ID) + if autofocus != "DesktopOnly" { + t.Errorf("new page Autofocus = %q, want DesktopOnly", autofocus) + } + if width != 1200 { + t.Errorf("new page CanvasWidth = %d, want 1200", width) + } + if height != 600 { + t.Errorf("new page CanvasHeight = %d, want 600", height) + } +} + +// The width the value is stored at must not decide whether it is carried. +// Studio Pro writes int64; the gen setter takes int32; the natural assertion is +// therefore the wrong one, and it fails silently rather than loudly. This ran +// green against a fix that only handled int32 — which is exactly why it asserts +// both widths rather than the one the author expected. +func TestUpdatePage_CarriesStoredCanvasAtEitherWidth(t *testing.T) { + for _, tc := range []struct { + name string + width any + }{ + {"int64 (what Studio Pro stores)", int64(1198)}, + {"int32", int32(1198)}, + } { + t.Run(tc.name, func(t *testing.T) { + b, page := headerFixture(t) + setStoredHeader(t, b, page.ID, "DesktopOnly", tc.width, int64(600)) + + if err := b.UpdatePage(page); err != nil { + t.Fatalf("UpdatePage: %v", err) + } + if _, width, _ := storedHeader(t, b, page.ID); width != 1198 { + t.Errorf("CanvasWidth = %d, want 1198 — a %s value was not carried", width, tc.name) + } + }) + } +} diff --git a/mdl/backend/modelsdk/page_write.go b/mdl/backend/modelsdk/page_write.go index dad3067752..461f1a5273 100644 --- a/mdl/backend/modelsdk/page_write.go +++ b/mdl/backend/modelsdk/page_write.go @@ -5,6 +5,9 @@ package modelsdkbackend import ( "fmt" + "go.mongodb.org/mongo-driver/bson" + + "github.com/mendixlabs/mxcli/mdl/backend/bsonnav" "github.com/mendixlabs/mxcli/mdl/types" "github.com/mendixlabs/mxcli/model" "github.com/mendixlabs/mxcli/modelsdk/codec" @@ -80,6 +83,7 @@ func (b *Backend) UpdatePage(page *pages.Page) error { return err } g.SetID(element.ID(page.ID)) + b.carryStoredPageHeader(page.ID, g) contents, err := (&codec.Encoder{}).Encode(g) if err != nil { return fmt.Errorf("UpdatePage: encode: %w", err) @@ -98,6 +102,76 @@ func (b *Backend) DeletePage(id model.ID) error { return b.writer.DeleteUnit(string(id)) } +// carryStoredPageHeader copies the three editor-only properties off the stored +// unit onto a rebuilt page, so a rewrite does not move state nobody asked to +// change. +// +// pageToGen writes Autofocus, CanvasWidth and CanvasHeight as constants, which +// looked safe and is not: Studio Pro varies all three per page. Across the 67 +// pages of ako/TestApp at 11.14.0 CanvasWidth took SEVEN distinct values (800 +// ×33, 1198 ×20, 1200 ×4, 802 ×2, 900 ×2, 2000, 1800), Autofocus was Off on 9, +// and CanvasHeight 500 on one. The hardcoded 1200 therefore matched 4 of 67, so +// `describe page` → `exec` moved the canvas of the other 63 and the unit was +// rewritten where ADR-0008 would otherwise have elided the write entirely +// (ako/mxcli#541). +// +// Carried rather than spelled in MDL: none of the three has a spelling, none is +// something a script asks for, and a value nobody asked to change should not +// change. This is the same reasoning as Microflow.StableId in ADR-0008. +// +// A missing or unreadable stored unit leaves the defaults in place rather than +// failing the write — this is a fidelity improvement on a rewrite, not a +// precondition for one, and CreatePage has no stored document by definition. +func (b *Backend) carryStoredPageHeader(id model.ID, g *genPg.Page) { + if b.reader == nil || id == "" { + return + } + raw, err := b.reader.GetRawUnitBytes(string(id)) + if err != nil { + return + } + var stored bson.D + if err := bson.Unmarshal(raw, &stored); err != nil { + return + } + if v := bsonnav.DGetString(stored, "Autofocus"); v != "" { + g.SetAutofocus(v) + } + // Read width-agnostically. Studio Pro stores both canvas dimensions as + // **int64** (measured on all 67 TestApp pages), so a `.(int32)` assertion — + // the natural one, since the gen setter takes int32 — matches nothing and + // silently yields zero. That is the bson-numeric-width pattern, and it got + // this fix once already: the first version passed its unit test because the + // test's own fixture wrote int32, and still moved the canvas of every real + // document. + // + // 0 is not a canvas size Studio Pro stores, so it stands in for "absent"; + // writing it back would give the editor a zero-width page. + if v := bsonInt(bsonnav.DGet(stored, "CanvasWidth")); v > 0 { + g.SetCanvasWidth(int32(v)) + } + if v := bsonInt(bsonnav.DGet(stored, "CanvasHeight")); v > 0 { + g.SetCanvasHeight(int32(v)) + } +} + +// bsonInt reads an integer stored at any BSON width, mirroring extractInt in +// modelsdk/mpr/parser.go. Mendix picks the width per property and Go's type +// switch is exact, so a narrow assertion fails silently rather than loudly. +func bsonInt(v any) int64 { + switch n := v.(type) { + case int32: + return int64(n) + case int64: + return n + case int: + return int64(n) + case float64: + return int64(n) + } + return 0 +} + // popupDimension returns the pop-up width/height for the gen Page (int32). // Studio Pro's own default is 0 (auto-size), so 0 is a valid value and is // written through verbatim (issue #713); only a stray negative is clamped to 0. diff --git a/mdl/backend/modelsdk/page_zero_value_keys_test.go b/mdl/backend/modelsdk/page_zero_value_keys_test.go new file mode 100644 index 0000000000..a8c5ee57d4 --- /dev/null +++ b/mdl/backend/modelsdk/page_zero_value_keys_test.go @@ -0,0 +1,110 @@ +// SPDX-License-Identifier: Apache-2.0 + +// ako/mxcli#541 (dropped-key half) — a describe → exec round trip of a Studio Pro +// page drops eight keys whose stored value is the type's zero value: +// +// .../DataSource/SourceVariable/{LocalVariable,SnippetParameter,SubKey,Widget} = '' +// .../DataSource/SourceVariable/UseAllPages = False +// .../AttributeRef/EntityRef = null (×3) +// +// Studio Pro writes them; mxcli sets only the one field that carries a value, so +// the rest are never marked dirty and the encoder omits them. Measured across +// the 67 pages of ako/TestApp at 11.14.0: +// +// DomainModels$AttributeRef with an EntityRef key: 338 of 338 +// (313 explicitly null, 25 an IndirectEntityRef) +// +// This is the codec's TypeDefaults registry's job — the same mechanism already +// used for an association's null Source and a visibility setting's empty-string +// Attribute — rather than a set-every-field edit at each of the three +// PageVariable construction sites. +package modelsdkbackend + +import ( + "testing" + + "go.mongodb.org/mongo-driver/bson" + + "github.com/mendixlabs/mxcli/mdl/backend/bsonnav" + "github.com/mendixlabs/mxcli/modelsdk/codec" + "github.com/mendixlabs/mxcli/modelsdk/element" + "github.com/mendixlabs/mxcli/sdk/pages" +) + +// encodeToD runs an element through the real encoder and returns the document, +// so these assertions cover what actually reaches storage rather than what the +// gen object holds. +func encodeToD(t *testing.T, el element.Element) bson.D { + t.Helper() + raw, err := (&codec.Encoder{}).Encode(el) + if err != nil { + t.Fatalf("encode: %v", err) + } + var d bson.D + if err := bson.Unmarshal(raw, &d); err != nil { + t.Fatalf("unmarshal: %v", err) + } + return d +} + +func hasKey(d bson.D, key string) bool { + for _, e := range d { + if e.Key == key { + return true + } + } + return false +} + +// A plain attribute binding must still carry the EntityRef key, as null. +func TestAttributeRefToGen_EmitsNullEntityRef(t *testing.T) { + el := attributeRefToGen("Rules.RuleAction.ActionType") + if el == nil { + t.Fatal("attributeRefToGen returned nil for a qualified attribute") + } + d := encodeToD(t, el) + if !hasKey(d, "EntityRef") { + t.Errorf("EntityRef key absent; Studio Pro writes it on 338 of 338 AttributeRefs") + } + if v := bsonnav.DGet(d, "EntityRef"); v != nil { + t.Errorf("EntityRef = %v, want null for a plain (non-navigated) binding", v) + } +} + +// Control: a navigated binding must keep its real EntityRef, not be flattened +// to null by the default. Without this, "always write null" passes the test +// above and silently undoes ako/mxcli#529. +func TestAttributeRefWithSteps_KeepsItsEntityRef(t *testing.T) { + el := attributeRefWithStepsToGen("Rules.BusinessRule.Name", []pages.AttributeRefStep{ + {Association: "Rules.RuleAction_BusinessRule", DestinationEntity: "Rules.BusinessRule"}, + }) + d := encodeToD(t, el) + if v := bsonnav.DGet(d, "EntityRef"); v == nil { + t.Error("EntityRef was flattened to null — the association hops were lost") + } +} + +// Every PageVariable key Studio Pro writes must be present, whichever one of the +// four name fields actually carries the value. +func TestPageVariable_EmitsEveryStoredKey(t *testing.T) { + want := []string{"LocalVariable", "PageParameter", "SnippetParameter", "SubKey", "UseAllPages", "Widget"} + for _, kind := range []string{"", "local", "snippet"} { + name := kind + if name == "" { + name = "page" + } + t.Run(name, func(t *testing.T) { + d := encodeToD(t, sourceVariableToGen("RuleAction", kind)) + for _, k := range want { + if !hasKey(d, k) { + t.Errorf("key %q absent; Studio Pro writes all six on Forms$PageVariable", k) + } + } + // Control: the field the caller DID set keeps its value. + set := map[string]string{"": "PageParameter", "local": "LocalVariable", "snippet": "SnippetParameter"}[kind] + if got := bsonnav.DGetString(d, set); got != "RuleAction" { + t.Errorf("%s = %q, want RuleAction — the authored value was overwritten by the default", set, got) + } + }) + } +} diff --git a/mdl/backend/modelsdk/widget_write.go b/mdl/backend/modelsdk/widget_write.go index 2906d8cf94..aac64791bd 100644 --- a/mdl/backend/modelsdk/widget_write.go +++ b/mdl/backend/modelsdk/widget_write.go @@ -30,6 +30,25 @@ func init() { // Widgets nested in a Widgets list use the typed-array marker 2 when present. codec.RegisterListMarker(t, 2) } + // An AttributeRef always carries EntityRef: it is null for a plain binding + // and an IndirectEntityRef when the attribute is reached over associations. + // Measured 338 of 338 across the 67 pages of ako/TestApp at 11.14.0 (313 + // null, 25 navigated). mxcli only ever set it on the navigated branch, so a + // rewrite dropped the key from every plain one (ako/mxcli#541). The default + // applies only when the field was not otherwise set, so it cannot flatten a + // navigated ref. + codec.RegisterTypeDefaults("DomainModels$AttributeRef", codec.TypeDefaults{ + NullFields: []string{"EntityRef"}, + }) + // A PageVariable names its source in exactly one of four fields and Studio + // Pro writes all six keys regardless. mxcli sets whichever one carries the + // value, leaving the rest at Go's zero value, never marked dirty, and so + // omitted. Registering them here covers all three construction sites and + // any future one, which setting every field at each site would not. + codec.RegisterTypeDefaults("Forms$PageVariable", codec.TypeDefaults{ + EmptyStringFields: []string{"LocalVariable", "PageParameter", "SnippetParameter", "SubKey", "Widget"}, + FalseFields: []string{"UseAllPages"}, + }) // A ClientTemplate's Parameters list is always emitted with marker 2, even empty // (unusual — most empty lists are marker 3). codec.RegisterTypeDefaults("Forms$ClientTemplate", codec.TypeDefaults{ @@ -1626,25 +1645,43 @@ func clientActionToGen(a pages.ClientAction) (element.Element, error) { switch x := a.(type) { case nil, *pages.NoClientAction: return noActionGen(), nil + // The four simple actions below were the only ones that did not write + // DisabledDuringExecution. Studio Pro stores it true on all 39 of them + // across the 67 pages of ako/TestApp at 11.14.0 (CancelChanges 16, ClosePage + // 10, SaveChanges 8, Delete 5), so its absence was a round-trip loss rather + // than a default (ako/mxcli#541). The seven other cases in this switch + // already set it. + // + // Other types carrying the property are NOT unanimous — Forms$NoAction + // stores false on 83 of ~7,300 and Forms$MicroflowAction on 5 of 81 — but + // those are stored values mxcli overwrites, a carry problem that predates + // this change and is tracked separately. case *pages.SaveChangesClientAction: g := genPg.NewSaveChangesClientAction() assignID(g) g.SetClosePage(x.ClosePage) - g.SetSyncAutomatically(true) + g.SetDisabledDuringExecution(true) + // false, not true: all eight Studio Pro SaveChanges actions in that + // same sweep store false. Writing true turned a describe → exec of any + // page with a Save button into a change nobody asked for. + g.SetSyncAutomatically(false) return g, nil case *pages.CancelChangesClientAction: g := genPg.NewCancelChangesClientAction() assignID(g) g.SetClosePage(x.ClosePage) + g.SetDisabledDuringExecution(true) return g, nil case *pages.ClosePageClientAction: g := genPg.NewClosePageClientAction() assignID(g) + g.SetDisabledDuringExecution(true) return g, nil case *pages.DeleteClientAction: g := genPg.NewDeleteClientAction() assignID(g) g.SetClosePage(x.ClosePage) + g.SetDisabledDuringExecution(true) return g, nil case *pages.PageClientAction: // show_page → Forms$FormAction with a Forms$FormSettings (PageSettings). diff --git a/mdl/backend/modelsdk/widget_write_action_defaults_test.go b/mdl/backend/modelsdk/widget_write_action_defaults_test.go new file mode 100644 index 0000000000..d03807f135 --- /dev/null +++ b/mdl/backend/modelsdk/widget_write_action_defaults_test.go @@ -0,0 +1,117 @@ +// SPDX-License-Identifier: Apache-2.0 + +// ako/mxcli#541 (action half) — a describe → exec round trip of a Studio Pro page +// reports "Replaced page", not "Unchanged page", and one reason is that four +// client actions are written with the wrong constants: +// +// FooterWidgets/[1]/Action/DisabledDuringExecution True → (key absent) +// FooterWidgets/[2]/Action/DisabledDuringExecution True → (key absent) +// FooterWidgets/[1]/Action/SyncAutomatically False → True +// +// Measured across all 67 pages of ako/TestApp at 11.14.0. For the four types +// changed here the stored value is unanimous — 39 of 39: +// +// CancelChangesClientAction True ×16 ClosePageClientAction True ×10 +// SaveChangesClientAction True × 8 DeleteClientAction True × 5 +// SaveChangesClientAction.SyncAutomatically = False 8 of 8 +// +// so neither is one user's unticked box. Seven of mxcli's action cases already +// wrote DisabledDuringExecution; the four simple ones did not, which is the +// "every constant the rebuild writes is a candidate" audit from +// docs-wiki/bug-patterns/rewrite-drops-unauthored-state.md. +// +// Unanimity does NOT hold across every type carrying the property, and the +// first sweep here missed that by filtering on names ending in "ClientAction": +// Forms$NoAction stores False on 83 of 7,300 and Forms$MicroflowAction on 5 of +// 81. Those minorities are a CARRY problem (a stored value mxcli overwrites), +// not a default problem, they predate this change, and they are out of its +// scope — see the follow-up noted on ako/mxcli#541. The lesson is the filter: +// a population selected by name can confirm whatever it excluded. +package modelsdkbackend + +import ( + "testing" + + "github.com/mendixlabs/mxcli/model" + genPg "github.com/mendixlabs/mxcli/modelsdk/gen/pages" + "github.com/mendixlabs/mxcli/sdk/pages" +) + +// disabledDuringExecution reads the flag off whichever action type was built. +// A type missing from this switch is a new action that has not been measured +// against a Studio Pro reference, so it fails rather than silently passing. +func disabledDuringExecution(t *testing.T, el any) bool { + t.Helper() + switch g := el.(type) { + case *genPg.SaveChangesClientAction: + return g.DisabledDuringExecution() + case *genPg.CancelChangesClientAction: + return g.DisabledDuringExecution() + case *genPg.ClosePageClientAction: + return g.DisabledDuringExecution() + case *genPg.DeleteClientAction: + return g.DisabledDuringExecution() + case *genPg.PageClientAction: + return g.DisabledDuringExecution() + case *genPg.MicroflowClientAction: + return g.DisabledDuringExecution() + default: + t.Fatalf("no DisabledDuringExecution accessor wired for %T — measure it "+ + "against a Studio Pro reference and add it here", el) + return false + } +} + +// Studio Pro writes DisabledDuringExecution on every client action it stores. +// The four simple ones did not get it; the two at the end are the positive +// control, since they always did — without them a fix that broke the shared +// path would still pass. +func TestClientActionToGen_DisabledDuringExecution(t *testing.T) { + cases := []struct { + name string + action pages.ClientAction + }{ + {"save_changes", &pages.SaveChangesClientAction{BaseElement: model.BaseElement{ID: "a"}}}, + {"cancel_changes", &pages.CancelChangesClientAction{BaseElement: model.BaseElement{ID: "b"}}}, + {"close_page", &pages.ClosePageClientAction{BaseElement: model.BaseElement{ID: "c"}}}, + {"delete_object", &pages.DeleteClientAction{BaseElement: model.BaseElement{ID: "d"}}}, + // Controls: these already carried it. + {"show_page (control)", &pages.PageClientAction{BaseElement: model.BaseElement{ID: "e"}, PageName: "M.P"}}, + {"microflow (control)", &pages.MicroflowClientAction{BaseElement: model.BaseElement{ID: "f"}, MicroflowName: "M.MF"}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + el, err := clientActionToGen(tc.action) + if err != nil { + t.Fatalf("clientActionToGen: %v", err) + } + if !disabledDuringExecution(t, el) { + t.Errorf("DisabledDuringExecution = false; Studio Pro writes true on " + + "39 of 39 stored actions of these four types across ako/TestApp") + } + }) + } +} + +// SyncAutomatically is the opposite error: mxcli wrote true where all eight +// Studio Pro SaveChanges actions store false. +func TestClientActionToGen_SaveChangesSyncAutomatically(t *testing.T) { + el, err := clientActionToGen(&pages.SaveChangesClientAction{ + BaseElement: model.BaseElement{ID: "a"}, + }) + if err != nil { + t.Fatalf("clientActionToGen: %v", err) + } + g := el.(*genPg.SaveChangesClientAction) + if g.SyncAutomatically() { + t.Error("SyncAutomatically = true; Studio Pro stores false on 8 of 8") + } + // Control: the property the statement DOES author is still honoured, so + // this is not a blanket "write false to everything". + el2, _ := clientActionToGen(&pages.SaveChangesClientAction{ + BaseElement: model.BaseElement{ID: "b"}, ClosePage: true, + }) + if !el2.(*genPg.SaveChangesClientAction).ClosePage() { + t.Error("ClosePage was lost — the authored property must survive") + } +} diff --git a/modelsdk/codec/defaults.go b/modelsdk/codec/defaults.go index 51223ab2d5..3349d1466d 100644 --- a/modelsdk/codec/defaults.go +++ b/modelsdk/codec/defaults.go @@ -38,6 +38,12 @@ type TypeDefaults struct { // AssociationPointer on an attribute-based index segment). Emitted when not // otherwise set. Stands in for a gen property the constructor doesn't expose. ZeroGUIDFields []string + // FalseFields are keys Studio Pro always serializes as boolean false when + // unset. Go's zero value for a bool is false either way, so the property is + // never marked dirty and the encoder would otherwise omit the key — which + // is a difference from the stored document even though nothing about the + // model changed (ako/mxcli#541, Forms$PageVariable.UseAllPages). + FalseFields []string // FreshGUIDFields are keys Studio Pro serializes as a fresh random GUID binary // (subtype 0), e.g. a microflow's StableId. Emitted when not otherwise set. // Stands in for a gen property mistyped as a string. The value is opaque to diff --git a/modelsdk/codec/encoder.go b/modelsdk/codec/encoder.go index 0c8aa71cff..7443fc38fd 100644 --- a/modelsdk/codec/encoder.go +++ b/modelsdk/codec/encoder.go @@ -181,6 +181,11 @@ func (e *Encoder) buildDoc(elem element.Element) (bson.D, error) { doc = append(doc, bson.E{Key: name, Value: ""}) } } + for _, name := range d.FalseFields { + if !emitted[name] { + doc = append(doc, bson.E{Key: name, Value: false}) + } + } for _, name := range d.ZeroGUIDFields { if !emitted[name] { doc = append(doc, bson.E{Key: name, Value: zeroGUIDBinary()}) From 31bf717cd2a6d782c091810471b18f510f1e07dd Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 20 Sep 2026 13:23:06 +0000 Subject: [PATCH 04/10] fix(pages): stop writing 11.1/10.17 header keys into older projects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to mendixlabs/mxcli#1121, which fixed this defect in a page's PARAMETERS and left the header out of scope. Measured against the Mendix Model SDK's StructureVersionInfo records (mendixmodelsdk 4.115.0): Page.Autofocus 11.1.0 Page.Variables 10.17.0 Snippet.Variables 10.17.0 All three were written unconditionally, so every page and snippet mxcli created for a Mendix 10 project carried a key that project's metamodel does not declare — the class that makes Studio Pro throw InvalidOperationException at MprProperty.cs. mxbuild does not catch it: measured on 10.24.25, 0 errors with the key present and 0 errors without it. They arrive by two different routes, and only one is a gen property. Autofocus is set through gen, so it is simply not set below 11.1. Variables exists only because the codec's Studio Pro defaults registry emits it as an empty typed-array marker, and a gen PartList has no "present but empty" state to leave unset — so there is nothing to skip and the suppression has to be in the encoder. That registry is global and keyed by $Type alone, so it cannot see a project version: hence Encoder.OmitKeys, per-encode. The zero Encoder suppresses nothing, so every other caller is unaffected. Suppressing a key and dropping data are different things. The empty Variables marker is safe to suppress; variables the script DECLARED are refused instead, naming them and the floor (guard-don't-drop, ADR-0005) — silently dropping them leaves widgets referencing names that are gone (CE1151) from a statement that reported success. CreatePage/UpdatePage and CreateSnippet/UpdateSnippet now share encodePage / encodeSnippet, so the guard cannot be applied on one path and forgotten on the other. ALTER PAGE needs nothing: the page mutator edits the stored raw BSON and marshals it back, so it can never invent a key. Verified on a real 10.24.25 project created with `mxcli new`. The page written by this build has no Autofocus and keeps Variables (10.24 >= 10.17); the snippet keeps Variables; `mx check` is 0 errors. The control is the same script run by a pre-fix binary against a copy of that project, which writes Autofocus — the only difference between the two documents. Unit tests carry a control too: with both guards stubbed to the pre-fix behaviour they report the key emitted at 9.24, 10.24.25 and unknown-version. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016aHj6mJwKCZD7EX7wcD6jW --- .../fix-issue/findings/mdl-backend.jsonl | 1 + .../page-header-version-floored-keys.mdl | 52 ++++++ .../modelsdk/page_version_keys_test.go | 171 ++++++++++++++++++ mdl/backend/modelsdk/page_write.go | 91 ++++++++-- mdl/backend/modelsdk/snippet_write.go | 30 +-- modelsdk/codec/encoder.go | 32 +++- modelsdk/codec/omitkeys_test.go | 83 +++++++++ 7 files changed, 433 insertions(+), 27 deletions(-) create mode 100644 mdl-examples/bug-tests/page-header-version-floored-keys.mdl create mode 100644 mdl/backend/modelsdk/page_version_keys_test.go create mode 100644 modelsdk/codec/omitkeys_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-backend.jsonl b/.claude/skills/fix-issue/findings/mdl-backend.jsonl index e448128210..caed06e253 100644 --- a/.claude/skills/fix-issue/findings/mdl-backend.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-backend.jsonl @@ -116,3 +116,4 @@ {"area": "mdl/backend", "date": "2026-09-17", "symptom": "A microflow's URL (the deep link Studio Pro shows on the microflow's properties, Mendix 10.6+, e.g. `item/{Key}`) disappears after any `CREATE OR MODIFY MICROFLOW` \u2014 including one that only edits the body. `mxcli check`, `mx check` and mxbuild all report success before and after; the loss is visible only in Studio Pro (#1120)", "cause": "`microflowToGen` wrote `out.SetUrl(\"\")` and `out.SetUrlSearchParametersQualifiedNames(nil)` unconditionally in its `major >= 10` block, `microflowFromGen` never read either back, and `sdk/microflows.Microflow` had no field to hold them \u2014 so the value had no path across a rewrite at any of the three layers", "file": "`mdl/backend/modelsdk/microflow_write.go` (microflowToGen), `mdl/backend/modelsdk/microflow.go` (microflowFromGen), `mdl/executor/cmd_microflows_build.go`, `sdk/microflows/microflows.go`", "fix": "Carry `Url`/`UrlSearchParameters` the way AllowConcurrentExecution/MarkAsUsed/ApplyEntityAccess already are: field on the semantic microflow, read in microflowFromGen, written from the model in microflowToGen, and seeded from the stored microflow in the executor's rewrite path. DESCRIBE emits a `-- URL: \u2026` note, because a describe -> rename -> exec COPY still has nothing to preserve from", "insight": "This is the fifth property in `microflows.Microflow` lost this way and the first with NO checker behind it, which is what made it a user report rather than an internal find. The earlier four were all caught by a build error eventually (CE4899 for the concurrency flags, CE0122 for Excluded) or by a security review (ApplyEntityAccess); a microflow with no URL is simply a valid microflow, so every gate stays green and only a human opening Studio Pro can see it. **Generalisable**: when auditing a rebuild for guard-don't-drop, rank the constants it writes by whether a checker would notice their absence \u2014 the ones nothing checks are the ones that reach users, and they are exactly the ones a 'does this look like configuration?' audit skips. The mechanical version is to diff a Studio Pro document key by key against the writer's output; here `grep 'out.Set.*(\"\")\\|(nil)' microflow_write.go` finds the whole remaining set in one line (`ExportLevel` pinned to \"Hidden\", `ConcurrencyErrorMicroflow`/`ConcurrencyErrorMessage` emptied \u2014 both still unguarded, though CE4899 makes the concurrency pair loud). Measured control: reverting either half (SetUrl or the read) alone fails TestMicroflowRoundTrip_DeepLinkURL with the reported symptom, so both halves are load-bearing"} {"area": "mdl/backend", "date": "2026-09-17", "symptom": "A microflow's **export level** (Studio Pro's Hidden/API switch \u2014 whether it is part of the module's public surface when the module is exported as a package) is reset to `Hidden` by any `CREATE OR MODIFY MICROFLOW`. Every checker stays green, because a hidden microflow is a valid microflow; the module's API is simply smaller", "cause": "`microflowToGen` wrote `out.SetExportLevel(\"Hidden\")` unconditionally, `microflowFromGen` never read it back, and `sdk/microflows.Microflow` had no field \u2014 the identical three-layer gap as the deep-link URL in the same function", "file": "`mdl/backend/modelsdk/microflow_write.go` (microflowToGen), `mdl/backend/modelsdk/microflow.go` (microflowFromGen), `mdl/executor/cmd_microflows_build.go`, `sdk/microflows/microflows.go`", "fix": "Same carry as the URL, plus a DEFAULT: `\"\"` is not a member of `MicroflowsExportLevel`, so an empty model value is written as `Hidden` rather than passed through (the precedent is `json_write.go`). DESCRIBE emits `-- Export level:` only when the value is not `Hidden`", "insight": "Found by running the mechanical audit the URL fix prompted \u2014 `grep 'out.Set.*(\"\\|(nil)' microflow_write.go` over the one function \u2014 which is the cheap move after any instance of this class and turned up three more constants in one line. **The measurement that shaped the fix**: three real marketplace modules (Business Events 3.12.0, External Database Connector 6.2.3/6.3.0) store `Hidden` on 3 of 3 microflows and 55 of 55 documents overall, all three exporting at module level `Source` \u2014 so the hardcoded value was not wrong, it was a default masquerading as a constant. That is the shape of the trap: the audit finds the constant, but only a reference document tells you whether to carry it, default it, or leave it alone. A marketplace `.mpk` is a free source of these \u2014 `unzip -o pkg.mpk project.mpr` gives a real Studio Pro-authored MPR to query, no Studio Pro and no network needed (`mx-modules/` holds three). **Never carry an enum-valued property straight through without a default**: a stored document that says nothing reads as `\"\"`, and writing `\"\"` back is precisely the unloadable-model write CLAUDE.md warns about \u2014 mxbuild tolerates it and Studio Pro throws at MprProperty.cs. Controls: pinning the writer back, stubbing the reader, and neutralising the executor carry each fail a different test with the reported symptom"} {"area": "mdl/backend", "date": "2026-09-17", "symptom": "Unit tests for a carried microflow property (URL, export level, concurrency) all pass, and the end-to-end behaviour against a real project is still unverified \u2014 the integration gate that would have caught it, `TestMxCheck_DoctypeScripts`, `t.Skip`s whenever `mx` is absent, which is every run in a fresh container", "cause": "Two separate measurement errors, both invisible to `go test`. (1) The test fixture paired `Url: \"item/{Key}\"` with `UrlSearchParameters: [\"\u2026.Key\"]` \u2014 the SAME parameter \u2014 which mxbuild rejects as **CE5612**: a parameter used in the URL path may not also be a search parameter. Nothing in a unit test validates the model, so the fixture described a document Mendix refuses to build. (2) `bin/mxcli` was stale: `go build ./mdl/...` and `make test` had been run after each fix, but not `make build`, so the end-to-end run exercised a binary predating two of the three commits", "file": "`mdl/backend/modelsdk/microflow_roundtrip_flags_test.go`, `mdl/executor/microflow_carried_properties_test.go`, `mdl/executor/roundtrip_doctype_test.go` (the skipping gate)", "fix": "Fixture uses a distinct `Filter` parameter and says why. End-to-end procedure that actually measures it: `mxcli setup mxbuild -p ` (~719 MB, works through the session proxy), copy `testdata/expr-checker` as the fixture, create the microflow with mxcli, seed the unauthorable properties straight into the stored unit with `mpr.NewWriter` + `UpdateRawUnit`, then `mx check` BEFORE (the fixture must be a document Mendix accepts, or it proves nothing), `mxcli exec` a body-only rewrite, read the unit back, `mx check` after", "insight": "**A skipping integration gate is worse than no gate**: `mxCheckAvailable()` + `t.Skip` means a green `make test` says nothing about mxbuild, and reading the CLAUDE.md line about #808 is not the same as checking whether it applies to your own run \u2014 `ls ~/.mxcli/mxbuild` is. **Rebuild the binary before any end-to-end run**, and check its mtime against the last commit: a stale `bin/mxcli` produced a result (URL survived, export level did not) that looked exactly like a genuine second-read-path defect, and sent me hunting for a duplicate resolver that does not exist. **Seed the fixture through the writer, not by hand-editing BSON**, and always `mx check` the seeded state first: the CE5612 error came from the seed, not from mxcli, and without the before-check it would have been misattributed to the fix. Measured, mxbuild 11.6.6: pre-fix binary rewrites the microflow to `Url=\"\"`, empty search params, `ExportLevel=\"Hidden\"`; post-fix keeps all three; `mx check` 0 errors on both the seeded control and the rewritten project"} +{"area": "mdl/backend", "date": "2026-09-20", "symptom": "Every page mxcli wrote carried `Autofocus` (a property Mendix introduced in 11.1.0) and every page and snippet carried `Variables` (10.17.0), whatever the project's version. On a Mendix 10 project the document therefore held a key that project's metamodel does not declare — the class of defect that makes Studio Pro throw InvalidOperationException at MprProperty.cs while mxbuild reports 0 errors. Found while fixing mendixlabs/mxcli#1121 (the same defect in the page's PARAMETERS) and deferred there as out of scope.", "cause": "`pageToGen` called `SetAutofocus(\"DesktopOnly\")` unconditionally, and both `Forms$Page` and `Forms$Snippet` registered `Variables` in `codec.RegisterTypeDefaults(...).MandatoryLists`, which the encoder emits as an empty typed-array marker for every new element. The page writer had no version awareness at all until #1121 threaded `*types.ProjectVersion` into `pageToGen` for the parameter keys.", "file": "`mdl/backend/modelsdk/page_write.go` (pageSupportsAutofocus/pageSupportsVariables, encodePage, docEncoder), `mdl/backend/modelsdk/snippet_write.go` (encodeSnippet), `modelsdk/codec/encoder.go` (Encoder.OmitKeys). Tests `mdl/backend/modelsdk/page_version_keys_test.go`, `modelsdk/codec/omitkeys_test.go`.", "insight": "**A version-floored key can arrive by two different routes, and only one of them is a gen property.** `Autofocus` is set through gen, so the fix is to not set it; `Variables` exists only because the Studio Pro defaults registry adds it, and a gen `PartList` has no 'present but empty' state — so there is nothing to leave unset and the suppression has to happen in the encoder. That registry is global and keyed by `$Type` alone, so it cannot see a project version: hence `Encoder.OmitKeys`, per-encode. **Suppressing a key and dropping data are different things**: the empty `Variables` marker is safe to suppress, but variables the script DECLARED are refused instead (guard-don't-drop, ADR-0005) — silently dropping them leaves widgets referencing names that are gone (CE1151) from a statement that reported success. Two measurement traps hit on the way: the first control run died on a pre-existing entity before reaching the page statement, so the page was never rewritten and the unchanged output read as a pass — a control that does not run looks exactly like a control that passes; and ALTER PAGE turned out to be safe without any change, because the page mutator edits the stored raw BSON and marshals it back, so it can never invent a key. Check which write paths actually rebuild a document before assuming a header fix has to cover all of them.", "refs": ["#1121"]} diff --git a/mdl-examples/bug-tests/page-header-version-floored-keys.mdl b/mdl-examples/bug-tests/page-header-version-floored-keys.mdl new file mode 100644 index 0000000000..dc9dfc700d --- /dev/null +++ b/mdl-examples/bug-tests/page-header-version-floored-keys.mdl @@ -0,0 +1,52 @@ +-- Version-floored keys in the page and snippet HEADER (follow-up to +-- mendixlabs/mxcli#1121, which fixed the same defect in a page's parameters). +-- +-- Measured floors (mendixmodelsdk 4.115.0, Page.versionInfo / Snippet.versionInfo): +-- +-- Page.Autofocus 11.1.0 +-- Page.Variables 10.17.0 +-- Snippet.Variables 10.17.0 +-- +-- Both were written unconditionally, so a document mxcli created for a Mendix 10 +-- project carried a key that project's metamodel does not declare. mxbuild is not +-- a safety net: measured on 10.24.25, `mx check` is 0 errors either way. +-- +-- Run against a Mendix 10.24 project and dump the documents: +-- +-- mxcli bson dump -p app.mpr --type page --object "KeyTest.Item_Edit" +-- mxcli bson dump -p app.mpr --type snippet --object "KeyTest.Sn_Item" +-- +-- Expected on 10.24 (>= 10.17, < 11.1): NO Autofocus, Variables present. +-- Before the fix the page carried Autofocus; that is the only difference +-- between the two documents, and it is what the control run showed. +CREATE MODULE KeyTest; + +CREATE PERSISTENT ENTITY KeyTest.Item ( + Name: String(200) +); + +CREATE OR REPLACE PAGE KeyTest.Item_Edit ( + Title: 'Edit item', + Layout: Atlas_Core.PopupLayout, + Params: { $Item: KeyTest.Item } +) { + layoutgrid lg { + row r { + column c (DesktopWidth: AutoFill) { + dataview dv (DataSource: $Item) { + textbox txtName (Label: 'Name', Attribute: Name) + } + } + } + } +} + +CREATE OR REPLACE SNIPPET KeyTest.Sn_Item { + layoutgrid lg2 { + row r2 { + column c2 (DesktopWidth: AutoFill) { + dynamictext st (Content: 'hello') + } + } + } +} diff --git a/mdl/backend/modelsdk/page_version_keys_test.go b/mdl/backend/modelsdk/page_version_keys_test.go new file mode 100644 index 0000000000..67f191a9bf --- /dev/null +++ b/mdl/backend/modelsdk/page_version_keys_test.go @@ -0,0 +1,171 @@ +// SPDX-License-Identifier: Apache-2.0 + +package modelsdkbackend + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/types" + "github.com/mendixlabs/mxcli/sdk/pages" + "go.mongodb.org/mongo-driver/bson" +) + +// Forms$Page and Forms$Snippet carry two properties Mendix introduced after the +// oldest version mxcli supports (mendixmodelsdk 4.115.0, Page.versionInfo / +// Snippet.versionInfo): +// +// Page.Autofocus 11.1.0 +// Page.Variables 10.17.0 +// Snippet.Variables 10.17.0 +// +// Both were written unconditionally on every page and snippet, so a document +// mxcli created for a Mendix 10 project carried a key that project's metamodel +// does not declare — the same defect as the page-parameter keys in #1121, in the +// document header rather than its parameters. +// +// mxbuild is not a safety net for this: measured on 10.24.25, `mx check` reports +// 0 errors with an 11.5-only key present. Studio Pro resolves every stored +// property against the type's property list and throws InvalidOperationException +// at MprProperty.cs. + +func v(major, minor, patch int) *types.ProjectVersion { + return &types.ProjectVersion{MajorVersion: major, MinorVersion: minor, PatchVersion: patch} +} + +func pageKeys(t *testing.T, pv *types.ProjectVersion) bson.Raw { + t.Helper() + p := &pages.Page{Name: "P"} + p.ID = "1" + b, err := encodePage(p, pv) + if err != nil { + t.Fatalf("encodePage: %v", err) + } + return bson.Raw(b) +} + +func has(raw bson.Raw, key string) bool { + _, err := raw.LookupErr(key) + return err == nil +} + +func TestPageOmitsAutofocusBelow11_1(t *testing.T) { + tests := []struct { + name string + pv *types.ProjectVersion + want bool + }{ + {"10.24.25 (the version in #1121)", v(10, 24, 25), false}, + {"9.24.0", v(9, 24, 0), false}, + {"11.0.0", v(11, 0, 0), false}, + {"11.1.0 (the floor)", v(11, 1, 0), true}, + {"11.13.0", v(11, 13, 0), true}, + {"unknown version", nil, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := has(pageKeys(t, tt.pv), "Autofocus"); got != tt.want { + t.Errorf("Autofocus emitted = %v, want %v", got, tt.want) + } + }) + } +} + +func TestPageOmitsVariablesBelow10_17(t *testing.T) { + tests := []struct { + name string + pv *types.ProjectVersion + want bool + }{ + {"10.16.0", v(10, 16, 0), false}, + {"9.24.0", v(9, 24, 0), false}, + {"10.17.0 (the floor)", v(10, 17, 0), true}, + {"10.24.25", v(10, 24, 25), true}, + {"11.13.0", v(11, 13, 0), true}, + {"unknown version", nil, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := has(pageKeys(t, tt.pv), "Variables"); got != tt.want { + t.Errorf("Variables emitted = %v, want %v", got, tt.want) + } + }) + } +} + +// A snippet carries the same Variables floor. It never populates the list, so the +// only thing that ever reaches disk is the empty marker the defaults registry +// adds — which is exactly the key that must not reach a pre-10.17 project. +func TestSnippetOmitsVariablesBelow10_17(t *testing.T) { + encode := func(pv *types.ProjectVersion) bson.Raw { + sn := &pages.Snippet{Name: "S"} + sn.ID = "1" + b, err := encodeSnippet(sn, pv) + if err != nil { + t.Fatalf("encodeSnippet: %v", err) + } + return bson.Raw(b) + } + if has(encode(v(10, 16, 0)), "Variables") { + t.Error("10.16: Variables emitted, want omitted") + } + if !has(encode(v(10, 17, 0)), "Variables") { + t.Error("10.17: Variables omitted, want emitted") + } + if !has(encode(v(11, 13, 0)), "Variables") { + t.Error("11.13: Variables omitted, want emitted") + } +} + +// Whatever the version, the document's own identity and structure must survive — +// a suppression that over-reached would pass the tests above and write a page +// Mendix cannot load. +func TestPageKeepsItsStructureAtEveryVersion(t *testing.T) { + for _, pv := range []*types.ProjectVersion{v(9, 24, 0), v(10, 24, 25), v(11, 13, 0), nil} { + raw := pageKeys(t, pv) + for _, key := range []string{"$ID", "$Type", "Name", "Parameters", "Title", "Appearance"} { + if !has(raw, key) { + t.Errorf("pv=%v: %s missing", pv, key) + } + } + if got := raw.Lookup("$Type").StringValue(); got != "Forms$Page" { + t.Errorf("pv=%v: $Type = %q", pv, got) + } + } +} + +// Suppressing the empty Variables marker is right; suppressing variables the +// script declared is not. Below the floor those are refused, because silently +// dropping them leaves widgets referencing names that are gone (CE1151) from a +// statement that reported success — guard-don't-drop, ADR-0005. +func TestPageWithVariablesIsRefusedBelow10_17(t *testing.T) { + withVars := func() *pages.Page { + p := &pages.Page{ + Name: "P", + Variables: []*pages.LocalVariable{{Name: "Flag", DefaultValue: "true"}}, + } + p.ID = "1" + return p + } + + _, err := encodePage(withVars(), v(10, 16, 0)) + if err == nil { + t.Fatal("10.16: a page with variables was encoded, want a refusal") + } + for _, want := range []string{"Flag", "10.17", "10.16"} { + if !strings.Contains(err.Error(), want) { + // Name the variable count, the floor, and the project's own version — + // an error missing any of them cannot be acted on. + t.Errorf("error %q does not mention %q", err, want) + } + } + + // At and above the floor the same page encodes, with its variables. + b, err := encodePage(withVars(), v(10, 17, 0)) + if err != nil { + t.Fatalf("10.17: %v", err) + } + if !has(bson.Raw(b), "Variables") { + t.Error("10.17: Variables missing from a page that declares one") + } +} diff --git a/mdl/backend/modelsdk/page_write.go b/mdl/backend/modelsdk/page_write.go index dad3067752..c213e969c0 100644 --- a/mdl/backend/modelsdk/page_write.go +++ b/mdl/backend/modelsdk/page_write.go @@ -4,6 +4,7 @@ package modelsdkbackend import ( "fmt" + "strings" "github.com/mendixlabs/mxcli/mdl/types" "github.com/mendixlabs/mxcli/model" @@ -43,12 +44,7 @@ func (b *Backend) CreatePage(page *pages.Page) error { if page.ID == "" { page.ID = model.ID(mmpr.GenerateID()) } - g, err := pageToGen(page, b.ProjectVersion()) - if err != nil { - return err - } - g.SetID(element.ID(page.ID)) - contents, err := (&codec.Encoder{}).Encode(g) + contents, err := encodePage(page, b.ProjectVersion()) if err != nil { return fmt.Errorf("CreatePage: encode: %w", err) } @@ -75,12 +71,7 @@ func (b *Backend) UpdatePage(page *pages.Page) error { if b.writer == nil { return fmt.Errorf("UpdatePage: not connected for writing") } - g, err := pageToGen(page, b.ProjectVersion()) - if err != nil { - return err - } - g.SetID(element.ID(page.ID)) - contents, err := (&codec.Encoder{}).Encode(g) + contents, err := encodePage(page, b.ProjectVersion()) if err != nil { return fmt.Errorf("UpdatePage: encode: %w", err) } @@ -108,6 +99,78 @@ func popupDimension(n int) int32 { return int32(n) } +// Mendix introduced these two Forms$Page properties after the oldest version +// mxcli supports (mendixmodelsdk, Page.versionInfo). A key the project's +// metamodel does not declare makes a document Studio Pro cannot open, and +// mxbuild does not catch it — see codec.Encoder.OmitKeys. +const ( + pageAutofocusMajor, pageAutofocusMinor = 11, 1 + pageVariablesMajor, pageVariablesMinor = 10, 17 +) + +// pageSupportsAutofocus / pageSupportsVariables report whether the project's +// version declares the property. An unreadable version omits, matching the +// page-parameter guard: an absent optional property is filled in on load, an +// unknown one is unopenable. +func pageSupportsAutofocus(pv *types.ProjectVersion) bool { + return pv != nil && pv.IsAtLeast(pageAutofocusMajor, pageAutofocusMinor) +} + +func pageSupportsVariables(pv *types.ProjectVersion) bool { + return pv != nil && pv.IsAtLeast(pageVariablesMajor, pageVariablesMinor) +} + +// versionLabel renders a project version for an error message, including the +// case where it could not be read at all. +func versionLabel(pv *types.ProjectVersion) string { + if pv == nil { + return "unknown" + } + if pv.ProductVersion != "" { + return pv.ProductVersion + } + return fmt.Sprintf("%d.%d.%d", pv.MajorVersion, pv.MinorVersion, pv.PatchVersion) +} + +// docEncoder returns an encoder that drops the version-floored keys this project +// cannot carry. Variables is emitted by the codec's Studio Pro defaults registry +// rather than by a gen property, so suppressing it is the encoder's job; a gen +// PartList has no "present but empty" state to leave unset. +func docEncoder(typeName string, pv *types.ProjectVersion) *codec.Encoder { + if pageSupportsVariables(pv) { + return &codec.Encoder{} + } + return &codec.Encoder{OmitKeys: map[string]map[string]bool{ + typeName: {"Variables": true}, + }} +} + +// encodePage builds and serializes a Forms$Page for a project of this version. +// Both CreatePage and UpdatePage go through it, so the version guards cannot be +// applied on one path and forgotten on the other. +func encodePage(page *pages.Page, pv *types.ProjectVersion) ([]byte, error) { + // Suppressing the key is right for the empty list Studio Pro always writes. + // It is not right for variables the script actually declared: dropping those + // would leave a page whose widgets reference names that are no longer there + // (CE1151), from a statement that reported success. Refuse instead + // (guard-don't-drop, ADR-0005). + if len(page.Variables) > 0 && !pageSupportsVariables(pv) { + names := make([]string, 0, len(page.Variables)) + for _, v := range page.Variables { + names = append(names, v.Name) + } + return nil, fmt.Errorf( + "page %q declares page variable(s) %s, which Mendix introduced in %d.%d (project is %s)", + page.Name, strings.Join(names, ", "), pageVariablesMajor, pageVariablesMinor, versionLabel(pv)) + } + g, err := pageToGen(page, pv) + if err != nil { + return nil, err + } + g.SetID(element.ID(page.ID)) + return docEncoder("Forms$Page", pv).Encode(g) +} + // pageToGen builds the full gen Page: header, layout call, the widget tree (under // the layout call's form-call arguments), parameters, and variables. func pageToGen(page *pages.Page, pv *types.ProjectVersion) (*genPg.Page, error) { @@ -116,7 +179,9 @@ func pageToGen(page *pages.Page, pv *types.ProjectVersion) (*genPg.Page, error) out.SetDocumentation(page.Documentation) out.SetExcluded(page.Excluded) out.SetExportLevel("Hidden") - out.SetAutofocus("DesktopOnly") + if pageSupportsAutofocus(pv) { + out.SetAutofocus("DesktopOnly") + } out.SetCanvasWidth(1200) out.SetCanvasHeight(600) out.SetMarkAsUsed(page.MarkAsUsed) diff --git a/mdl/backend/modelsdk/snippet_write.go b/mdl/backend/modelsdk/snippet_write.go index cd2c99a9ae..138b84d03c 100644 --- a/mdl/backend/modelsdk/snippet_write.go +++ b/mdl/backend/modelsdk/snippet_write.go @@ -5,6 +5,7 @@ package modelsdkbackend import ( "fmt" + "github.com/mendixlabs/mxcli/mdl/types" "github.com/mendixlabs/mxcli/model" "github.com/mendixlabs/mxcli/modelsdk/codec" "github.com/mendixlabs/mxcli/modelsdk/element" @@ -21,6 +22,21 @@ func init() { }) } +// encodeSnippet builds and serializes a Forms$Snippet for a project of this +// version. Snippet.Variables shares the Forms$Page floor (10.17.0), and a +// snippet never populates the list — so the only thing that could reach a +// pre-10.17 project is the empty marker the defaults registry adds. Both +// CreateSnippet and UpdateSnippet go through here so the guard cannot be applied +// on one path and forgotten on the other. +func encodeSnippet(snippet *pages.Snippet, pv *types.ProjectVersion) ([]byte, error) { + g, err := snippetToGen(snippet) + if err != nil { + return nil, err + } + g.SetID(element.ID(snippet.ID)) + return docEncoder("Forms$Snippet", pv).Encode(g) +} + // CreateSnippet inserts a new Forms$Snippet document — a reusable widget tree with // its own parameters (entity-typed) and a flat Widgets list (no layout call). func (b *Backend) CreateSnippet(snippet *pages.Snippet) error { @@ -39,12 +55,7 @@ func (b *Backend) CreateSnippet(snippet *pages.Snippet) error { if snippet.ID == "" { snippet.ID = model.ID(mmpr.GenerateID()) } - g, err := snippetToGen(snippet) - if err != nil { - return err - } - g.SetID(element.ID(snippet.ID)) - contents, err := (&codec.Encoder{}).Encode(g) + contents, err := encodeSnippet(snippet, b.ProjectVersion()) if err != nil { return fmt.Errorf("CreateSnippet: encode: %w", err) } @@ -68,12 +79,7 @@ func (b *Backend) UpdateSnippet(snippet *pages.Snippet) error { if b.writer == nil { return fmt.Errorf("UpdateSnippet: not connected for writing") } - g, err := snippetToGen(snippet) - if err != nil { - return err - } - g.SetID(element.ID(snippet.ID)) - contents, err := (&codec.Encoder{}).Encode(g) + contents, err := encodeSnippet(snippet, b.ProjectVersion()) if err != nil { return fmt.Errorf("UpdateSnippet: encode: %w", err) } diff --git a/modelsdk/codec/encoder.go b/modelsdk/codec/encoder.go index 0c8aa71cff..e90b721606 100644 --- a/modelsdk/codec/encoder.go +++ b/modelsdk/codec/encoder.go @@ -13,7 +13,32 @@ import ( ) // Encoder serializes Element trees back to BSON bytes. -type Encoder struct{} +type Encoder struct { + // OmitKeys suppresses BSON keys, per $Type, for this encoder only. + // + // It exists for **version-floored properties**. Mendix introduces properties + // over time, and a key introduced after the project's own version is one that + // project's metamodel does not declare. mxbuild's deserializer tolerates an + // unknown property — measured on 10.24.25, `mx check` reports 0 errors with + // one present — while Studio Pro resolves every stored property against the + // type's property list and throws InvalidOperationException at MprProperty.cs. + // So the build is not a safety net and the suppression has to be deliberate. + // + // It is per-encoder rather than part of RegisterTypeDefaults because that + // registry is global, keyed by $Type alone, and cannot see a project version. + // + // Outer map: $Type. Inner map: BSON key -> true. A nil map suppresses nothing, + // so the zero Encoder behaves exactly as before. + OmitKeys map[string]map[string]bool +} + +// omits reports whether this encoder must drop a key for the given $Type. +func (e *Encoder) omits(typeName, key string) bool { + if e.OmitKeys == nil { + return false + } + return e.OmitKeys[typeName][key] +} // Encode serializes an element to []byte. // Clean elements passthrough raw bytes unchanged. @@ -142,6 +167,9 @@ func (e *Encoder) buildDoc(elem element.Element) (bson.D, error) { return nil, err } if val != nil { + if e.omits(elem.TypeName(), prop.Name()) { + continue + } doc = append(doc, bson.E{Key: prop.Name(), Value: val}) emitted[prop.Name()] = true } @@ -155,7 +183,7 @@ func (e *Encoder) buildDoc(elem element.Element) (bson.D, error) { doc = append(doc, bson.E{Key: "GUID", Value: idToBinarySubtype0(elem.ID())}) } for _, name := range d.MandatoryLists { - if !emitted[name] { + if !emitted[name] && !e.omits(elem.TypeName(), name) { doc = append(doc, bson.E{Key: name, Value: bson.A{int32(3)}}) } } diff --git a/modelsdk/codec/omitkeys_test.go b/modelsdk/codec/omitkeys_test.go new file mode 100644 index 0000000000..d4b39f43f4 --- /dev/null +++ b/modelsdk/codec/omitkeys_test.go @@ -0,0 +1,83 @@ +// SPDX-License-Identifier: Apache-2.0 + +package codec + +import ( + "testing" + + "github.com/mendixlabs/mxcli/modelsdk/element" + "github.com/mendixlabs/mxcli/modelsdk/property" + "go.mongodb.org/mongo-driver/bson" +) + +// omitProbe is a minimal element with one primitive property, plus a registered +// mandatory list — the two ways a key can reach the document. +type omitProbe struct { + element.Base + name *property.Primitive[string] +} + +func newOmitProbe(typeName string) *omitProbe { + o := &omitProbe{} + o.SetTypeName(typeName) + o.name = property.NewPrimitive[string]("Name", property.DecodeString) + o.name.Bind(&o.Base, 0) + o.SetProperties([]element.Property{o.name}) + o.name.Set("probe") + o.MarkDirty(63) + return o +} + +func TestOmitKeysSuppressesPropertyAndMandatoryList(t *testing.T) { + const typeName = "Probe$OmitKeys" + RegisterTypeDefaults(typeName, TypeDefaults{MandatoryLists: []string{"Variables"}}) + + encode := func(e *Encoder) bson.Raw { + t.Helper() + b, err := e.Encode(newOmitProbe(typeName)) + if err != nil { + t.Fatalf("encode: %v", err) + } + return bson.Raw(b) + } + has := func(raw bson.Raw, key string) bool { + _, err := raw.LookupErr(key) + return err == nil + } + + // The zero Encoder must behave exactly as before this field existed. + base := encode(&Encoder{}) + if !has(base, "Name") || !has(base, "Variables") { + t.Fatalf("zero Encoder dropped something: %v", base) + } + + // A plain property. + got := encode(&Encoder{OmitKeys: map[string]map[string]bool{typeName: {"Name": true}}}) + if has(got, "Name") { + t.Error("Name emitted despite OmitKeys") + } + if !has(got, "Variables") { + t.Error("Variables dropped by an OmitKeys that did not name it") + } + + // A key that only exists because of the Studio Pro defaults registry. + got = encode(&Encoder{OmitKeys: map[string]map[string]bool{typeName: {"Variables": true}}}) + if has(got, "Variables") { + t.Error("mandatory list emitted despite OmitKeys") + } + if !has(got, "Name") { + t.Error("Name dropped by an OmitKeys that did not name it") + } + + // Keyed by $Type: another type's entry must not leak across. + got = encode(&Encoder{OmitKeys: map[string]map[string]bool{"Other$Type": {"Name": true}}}) + if !has(got, "Name") { + t.Error("another $Type's OmitKeys suppressed this one's key") + } + + // $ID and $Type are structural and are never subject to suppression. + got = encode(&Encoder{OmitKeys: map[string]map[string]bool{typeName: {"$Type": true, "$ID": true}}}) + if !has(got, "$Type") || !has(got, "$ID") { + t.Error("OmitKeys removed a structural key") + } +} From 52f3c11732d63633de70dadfe74554826b9241c6 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 20 Sep 2026 15:57:56 +0000 Subject: [PATCH 05/10] fix(pages): stop describe losing a password field, its validation and three more MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `describe page` → `exec` over a Studio Pro page silently dropped six things, with mx check at 0 errors on both sides. The one that matters: …/Widgets/[1]/IsPasswordBox True → False a password field round-trips into a plaintext text box. CLAUDE.md makes describe → rename → exec the copy operation, so copying a login or change-password page lost it silently. Measured on ako/TestApp's Administration.ChangePasswordForm at Mendix 11.14.0. Four different causes behind one symptom, which is why triage came before any code: - IsPasswordBox — the model and writer carried it; nothing parsed it and nothing emitted it. - Validation — widgetValidationToGen() wrote a default EMPTY Forms$WidgetValidation over whatever was stored, on five widget types. - ReadOnlyStyle — wired for CheckBox only. A DataView's draws no MDL-WIDGET07 warning because staticWidgetKnownProps is deliberately a union across widget types, so it passed check and was dropped anyway. - PopupCloseAction — pageToGen wrote "" unconditionally. Plus two typed-array markers: ParameterMappings is marker 2 on 220 of 220 stored lists in every parent type, and OutputMappings is present on 91 of 91 MicroflowSettings. An empty list needs MandatoryListMarkers, since RegisterListMarker keys on a child element that is not there. Three things measured rather than assumed, each of which would have been wrong the obvious way: A DataView's ReadOnlyStyle default is Control (47 of 56, never Inherit), not the Inherit every other input widget uses. The validation expression is emitted QUOTED, not bracketed. `[...]` is the XPath-constraint spelling and propertyValueV3 parses it as an array, so the builder saw []any and GetStringProp yielded "" — the emitter's own unit test was green while the real round trip still lost the value. PopupCloseAction is deliberately not carried from the stored document the way the canvas properties are: it names a widget, and a rewrite rebuilds the tree from the statement, so a carried name could dangle. DESCRIBE emits it instead. Result on that page: 17 differences → 9, and all 9 remaining are #549, a separate carry problem. Verified at the artifact level — same project, same script, only the binary differing: the pre-fix build turns both stored password boxes into plaintext ones, the fixed build preserves them, and mx check is 0 errors after round-tripping four pages. Closes #550 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01L2aYb3zDscezm874CDTH6R --- .../fix-issue/findings/mdl-executor.jsonl | 1 + .claude/skills/mendix/create-page/SKILL.md | 7 + cmd/mxcli/syntax/features_page.go | 2 +- docs/01-project/MDL_QUICK_REFERENCE.md | 4 + .../pages-550-describe-input-properties.mdl | 94 +++++++++++++ mdl/ast/ast_page_v3.go | 4 + mdl/backend/modelsdk/page_write.go | 11 +- mdl/backend/modelsdk/widget_write.go | 52 +++++-- .../modelsdk/widget_write_input_props_test.go | 91 ++++++++++++ .../widget_write_mapping_markers_test.go | 79 +++++++++++ .../cmd_pages_builder_input_props_test.go | 133 ++++++++++++++++++ mdl/executor/cmd_pages_builder_v3.go | 1 + mdl/executor/cmd_pages_builder_v3_widgets.go | 37 ++++- mdl/executor/cmd_pages_describe.go | 15 +- .../cmd_pages_describe_input_props_test.go | 127 +++++++++++++++++ mdl/executor/cmd_pages_describe_output.go | 36 +++++ mdl/executor/cmd_pages_describe_parse.go | 28 ++++ mdl/executor/validate_widgets.go | 4 + mdl/visitor/visitor_page_v3.go | 6 +- sdk/pages/pages.go | 3 + sdk/pages/pages_widgets_data.go | 4 + sdk/pages/pages_widgets_input.go | 11 +- 22 files changed, 729 insertions(+), 21 deletions(-) create mode 100644 mdl-examples/bug-tests/pages-550-describe-input-properties.mdl create mode 100644 mdl/backend/modelsdk/widget_write_input_props_test.go create mode 100644 mdl/backend/modelsdk/widget_write_mapping_markers_test.go create mode 100644 mdl/executor/cmd_pages_builder_input_props_test.go create mode 100644 mdl/executor/cmd_pages_describe_input_props_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor.jsonl b/.claude/skills/fix-issue/findings/mdl-executor.jsonl index 070b2238d2..7d871a2f9c 100644 --- a/.claude/skills/fix-issue/findings/mdl-executor.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-executor.jsonl @@ -660,3 +660,4 @@ {"area":"mdl/executor","date":"2026-09-20","symptom":"`CREATE OR MODIFY EXTERNAL ENTITIES FROM` marks NON-KEY attributes of a top-level external entity Updatable=true when the entity set's UpdateRestrictions say the set is updatable. `mx check` reports one CE6630 per attribute: \"'Label' is marked Updatable=False in the OData service, but True in the app.\" The sibling of the key-attribute defect fixed the same day; fixing only the key took the reported repro from 2 errors to 1, not to 0","cause":"`createExternalEntities` derived `defaultUpdatable` from `entitySet.Updatable`. Mendix does not: it computes Updatable as a function of whether the entity has an entity set at all — never for a top-level entity, always for a non-top-level one, which is written through its parent's flow. The annotation is irrelevant to it","file":"`mdl/executor/cmd_contract.go` (`defaultUpdatable := !isTopLevel`, replacing the UpdateRestrictions override; the key-specific guard added earlier becomes subsumed)","insight":"**The positive control that closes this is a contract whose `NonUpdatableProperties` names ONLY the key.** That service is asserting, by name, that every other property IS updatable — and mxbuild still answers False. Ten top-level shapes were probed in three rounds and all answered False (inline record, typed ``, UpdateMethod=PATCH, +NonUpdatableProperties +DeleteRestrictions, unannotated, external ``, Core.Permissions/ReadWrite, Core.OptimisticConcurrency/ETag, DeepUpdateSupport, and the key-only exclusion list); the first seven were NOT enough to act on, because 'no shape produces True' is the uniform-negative shape that means the probe is wrong — what made it actionable was a shape that states the opposite explicitly and is still refused. **Two false leads, each cheap and each worth skipping.** (1) ETag/optimistic concurrency, suggested by a comment in this very function about the service that motivated #729 — no effect. (2) The model's `AllowCreateChangeLocally`: the intuition is that Mendix would permit attribute changes once local changes are allowed, and it is wrong — setting it Yes on a top-level entity left the expectation at False. **That second control is the one that explains the rule rather than just fitting it**: an external object CAN be changed in memory and handed to an external OData action, and that is what the local-change flag governs; the attribute's `Updatable` mirrors only what the endpoint itself accepts on a PATCH. Domain knowledge from the maintainer, not derivable from the metamodel — worth asking for before probing an eleventh contract shape. The Insert/Update asymmetry that makes this look like a parser bug is real and is not one: mxbuild reads `InsertRestrictions` from the same document, in the same shapes, and honours it — so `Creatable` follows the contract and `Updatable` does not, which is also why 'the entity is read-only' is the wrong summary and why every test here asserts Creatable as its control. **A stale test encoded the old belief and had to be corrected, not worked around**: #1118's `TestCreateExternalEntities_FlattenedAttributesAreReadOnly` asserted `Label` was Creatable AND Updatable as its control; the Updatable half had been assumed from the contract rather than measured, while the flattened-attribute half it was controlling for HAD been. The control still works on Creatable alone. Verified end to end on a real 11.12.1 project: 2 errors before any fix, 1 after the key-only fix, **0 errors** now; TripPin (`-run 'TestMxCheck_DoctypeScripts/10-odata-examples'`, ~26s locally) is the other-direction control and stays green, since every entity it flags is non-top-level. Repro `mdl-examples/bug-tests/odata-key-attribute-updatable.mdl`; tests `mdl/executor/cmd_contract_key_updatable_test.go`","file_refs":["mdl/executor/cmd_contract.go"],"ce":["CE6630"]} {"area":"mdl/linter","date":"2026-09-20","symptom":"mxcli happily authors `staticimage`/`dynamicimage` with nothing warning the author, and #518/#538 had just made both MORE capable (Image:, DataSource:, DefaultImage:, thumbnail, enlarge all newly reachable). The only signal that these widgets do not work was CE0582 at the far end of a build. Separately, three places in code comments and user-facing help claimed the deprecation was 'Mendix 11's React client'","cause":"Two things. (1) No validator or lint rule mentioned either widget (measured: `grep -rln deprecat mdl/executor/validate*.go` matched only an unrelated test), so the deprecation lived entirely in prose. (2) The version claim was inherited from a pre-existing comment and repeated without checking the doc: docs.mendix.com/refguide/image-viewer/ says the React client was added in **10.7**, so CE0582 fires on 10.7+ wherever that client is enabled, not only on 11","file":"`mdl/linter/rules/legacy_image_widget.go` (new, MPR012) + registration in `mdl/executor/cmd_lint.go` (x2) and `cmd/mxcli/cmd_lint.go`; wording in `mdl/backend/modelsdk/widget_write_legacy_gaps.go`, `.claude/skills/mendix/create-page/reference/widgets.md`, `cmd/mxcli/syntax/features_page.go`; table row in `.claude/commands/mendix/lint.md`","insight":"**A deprecation warning belongs in `lint`, not in `check`.** `check` validates a script, and `describe page` -> `exec` of a legacy page is a legitimate lossless operation — a check warning would fire on correct work every time, which is the noise trap MDL-WIDGET23's own history records. `lint` audits the project, where 'this page holds a widget your client cannot render' is wanted once. **The marketplace exclusion came free and is the control worth measuring**: `ctx.Widgets()` filters any module with a Source (notPlatformModule), so the rule never fires on the Studio Pro static images a blank app inherits from FeedbackModule — content the reader cannot fix and an update would replace. Measured: 8 legacy image widgets in the project, 7 indexed by the catalog, 5 in the user's own module, and lint reported exactly those 5. **A deny-list of two, never an allow-list**: the one widget such a rule must never fire on is the pluggable Image, i.e. the replacement it recommends. **'Deprecated in version X' needs the vendor doc, not the previous comment** — this repo had carried 'Mendix 11' for as long as the widgets had been written, and one fetch of the reference guide moved it to 10.7. A version boundary copied from a sibling comment is the same class of error as a floor copied from a proposal's sample output (mendixlabs/mxcli#1121)","refs":["mendixlabs/mxcli#1057"],"ce":["CE0582"],"rules":["MPR012"]} {"area": "mdl/backend/pagemutator", "date": "2026-09-20", "symptom": "`alter page … { set DataSource = DATABASE Mod.Entity on dvCust; }` passes `check`, prints `Altered page …` with exit 0, and leaves the DataView with no datasource: `describe page` renders `dataview dvCust {` with the property gone, and the only other signal is CE7007 at `mx check`", "cause": "`serializeDataSourceBson` mapped every `*pages.DatabaseSource` to a `Forms$DataViewSource` — the *context* source — with the entity in `EntityRef` and `SourceVariable` left null. A DATABASE source has no single stored shape: the widget holding it decides (`Forms$ListViewXPathSource` on a list view, `CustomWidgets$CustomWidgetXPathSource` on a pluggable widget, `Forms$GridXPathSource` on a grid), and a DATA VIEW has no database form at all — which is why CREATE PAGE's `dataViewSourceToGen` already refused that pairing while SET wrote it silently", "file": "`mdl/backend/pagemutator/mutator.go` (`SetWidgetDataSource`, new `databaseSourceRefusal`, `serializeDataSourceBson`)", "insight": "**The \"gone entirely\" in the report was DESCRIBE, not the document.** The DataSource was present and well-formed BSON; `parseContextSource` returns nil for a `Forms$DataViewSource` with no `SourceVariable`, so the reader rendered nothing. Chasing a deleted property would have been the wrong hunt — diff the stored BSON before believing a describe-shaped symptom. **The refusal belongs in the mutator, not the validator**: `validateAlterSetProperties` dry-runs the real setter against a `pagemutator.Probe()` copy, so one refusal makes `check -p --references` and `exec` agree by construction; a second copy of the rule in the validator is the duplicate-resolver drift CLAUDE.md warns about. **Refusing beat rebuilding the shapes here** — writing ListViewXPathSource in raw BSON would duplicate `listViewSourceToGen` in a second currency, and REPLACE already reaches the real builder. **Two remedies, not one**: on a data view `use REPLACE` is a dead end (CREATE PAGE refuses it too), so the message names the sources a data view can take; on a list view REPLACE genuinely works, so it names REPLACE. Getting that backwards sends the author in a circle. **Same generalisable shape as #855/#1101**: when SET and REPLACE express different vocabularies for one property, SET is a whitelist extended one bug report at a time. **`make check-mdl` runs `mxcli check` WITHOUT `-p`**, so a document-dependent refusal cannot be a `.fail.mdl` — it would be reported as a negative test that unexpectedly passed. Write the passing shape and comment the refused statements, as #1063 does. Measured on two copies of a real 11.13.0 app: faulty → `Check passed!`, exit 0, `mx check` 1 error CE7007 at Data view 'dvCust'; fixed → both refuse with exit 1, datasource unchanged, `mx check` 0 errors. Tests `mdl/backend/pagemutator/mutator_datasource_test.go`, `mdl/executor/validate_alter_set_test.go`; example `mdl-examples/bug-tests/1032-alter-page-set-database-datasource.mdl`. upstream #1032", "refs": ["#855", "#1032"], "ce": ["CE7007"]} +{"area": "mdl/executor", "date": "2026-09-20", "symptom": "`describe page` → `exec` over a **Studio Pro-authored** page silently drops six things, `mx check` 0 errors throughout. The one that matters: `IsPasswordBox True → False` — a **password field round-trips into a plaintext text box**, and describe → rename → exec is mxcli's copy operation. Also `Validation.Expression` blanked, a DataView's `ReadOnlyStyle Text → Control`, `PopupCloseAction` wiped, and two typed-array markers", "cause": "Four different causes behind one symptom, which is why triage came first: (1) IsPasswordBox — model and writer carried it, nothing parsed it, nothing emitted it; (2) Validation — `widgetValidationToGen()` wrote a DEFAULT EMPTY Forms$WidgetValidation over whatever was stored, on five widget types; (3) ReadOnlyStyle — wired for CheckBox only, and a DataView's draws no MDL-WIDGET07 warning because `staticWidgetKnownProps` is deliberately a union across widget types; (4) PopupCloseAction — `pageToGen` wrote \"\" unconditionally. Plus ParameterMappings/OutputMappings markers", "file": "`mdl/executor/cmd_pages_describe_parse.go` + `_output.go` (extract/emit), `cmd_pages_builder_v3_widgets.go` (consume), `cmd_pages_builder_v3.go`, `mdl/visitor/visitor_page_v3.go`, `mdl/ast/ast_page_v3.go`, `sdk/pages/*`, `mdl/backend/modelsdk/widget_write.go` + `page_write.go`, `mdl/executor/validate_widgets.go` (describe vocabulary)", "insight": "**Triage the layer before writing anything** — describer / grammar / builder have different fixes and this one issue had all three. The quickest probe is to run the property through `mxcli check`: MDL-WIDGET07 names an unrecognised one, and *silence is not acceptance* — the known-props list is a union across widget types, so a DataView's ReadOnlyStyle passed check and was dropped anyway. **Emit an expression QUOTED, not bracketed**: `[...]` is the XPath-constraint spelling and `propertyValueV3` parses it as an ARRAY, so `GetStringProp` yields \"\" — the emitter's own unit test was green while the real round trip still lost the value (storage form is not input form). **Measure the default before keeping it**: a DataView's ReadOnlyStyle is Control on 47 of 56, never Inherit, so the 'obvious' Inherit that every other input widget uses would have been wrong. Markers likewise measured, not assumed: ParameterMappings is marker 2 on 220 of 220 lists in every parent type, OutputMappings present on 91 of 91 — and an EMPTY list needs `MandatoryListMarkers` since `RegisterListMarker` keys on a child that is not there. Result 17 → 9 differences, the 9 being ako/mxcli#549", "refs": ["#550", "#541", "#549", "#490"]} diff --git a/.claude/skills/mendix/create-page/SKILL.md b/.claude/skills/mendix/create-page/SKILL.md index 9538aac4a1..11542a4aab 100644 --- a/.claude/skills/mendix/create-page/SKILL.md +++ b/.claude/skills/mendix/create-page/SKILL.md @@ -67,6 +67,8 @@ Both are optional and can be changed later with `alter page … { set Class = ' | Widget name | Required after type | `textbox txtName (...)` | | Attribute binding | `attribute: AttrName` | `textbox txt (label: 'Name', attribute: Name)` | | Attribute over an association | `attribute: Assoc/Attr` (bare association name, multi-hop OK) | `textbox txt (label: 'Rule', attribute: RuleAction_BusinessRule/Name)` | +| Password field | `Password: true` | `textbox tbPw (attribute: Secret, Password: true)` | +| Widget validation | `Validation: ''` + `ValidationMessage: ''` | `Validation: 'length(toString($value)) > 0'` — quoted, not `[bracketed]` | | Variable binding | `datasource: $Var` | `dataview dv (datasource: $Product) { ... }` | | Action binding | `action: type` | `actionbutton btn (caption: 'Save', action: save_changes)` | | Database source | `datasource: database entity` | `datagrid dg (datasource: database Module.Entity)` | @@ -433,6 +435,11 @@ DATAVIEW dv (DataSource: $Issue) { A bare association name is qualified with the module of the entity the widget sits on. On a ComboBox that matters: its `DataSource:` is the *option list*, but +A text box that holds a secret needs `Password: true`. It is not cosmetic: without +it the field renders the value in plaintext, and before ako/mxcli#550 a +`describe page` → `exec` round trip silently turned every stored password field +into an ordinary one — so copying a login or change-password page lost it. + An input widget can also *traverse* an association to show a value from the other side: `attribute: Assoc/Attr` binds the far attribute and stores the hops, which is what Studio Pro does. It works on textbox, textarea, datepicker, diff --git a/cmd/mxcli/syntax/features_page.go b/cmd/mxcli/syntax/features_page.go index 3bf090a5a4..8cfd6b9b30 100644 --- a/cmd/mxcli/syntax/features_page.go +++ b/cmd/mxcli/syntax/features_page.go @@ -12,7 +12,7 @@ func init() { "page", "pages", "form", "UI", "user interface", "widget", "layout", "screen", }, - Syntax: "CREATE PAGE Module.Name\n (\n Title: 'Page Title',\n Layout: Module.LayoutName\n [, Params: { $Param: Module.Entity }]\n [, Url: 'page-url']\n [, Folder: 'FolderPath']\n [, Variables: { $var: Boolean = 'true' }]\n [, PopupWidth: 800, PopupHeight: 480, PopupResizable: true]\n [, Class: 'css-class', Style: 'css: rule']\n )\n {\n -- widgets\n }", + Syntax: "CREATE PAGE Module.Name\n (\n Title: 'Page Title',\n Layout: Module.LayoutName\n [, Params: { $Param: Module.Entity }]\n [, Url: 'page-url']\n [, Folder: 'FolderPath']\n [, Variables: { $var: Boolean = 'true' }]\n [, PopupWidth: 800, PopupHeight: 480, PopupResizable: true]\n [, PopupCloseAction: cancelButton1]\n [, Class: 'css-class', Style: 'css: rule']\n )\n {\n -- widgets\n }", Example: "CREATE PAGE MyModule.EditCustomer\n (\n Params: { $Customer: MyModule.Customer },\n Title: 'Edit Customer',\n Layout: Atlas_Core.PopupLayout,\n Class: 'container-fluid'\n )\n {\n DATAVIEW dvCustomer (DataSource: $Customer) {\n TEXTBOX txtName (Label: 'Name', Attribute: Name)\n FOOTER footer1 {\n ACTIONBUTTON btnSave (Caption: 'Save', Action: SAVE_CHANGES, ButtonStyle: Primary)\n ACTIONBUTTON btnCancel (Caption: 'Cancel', Action: CANCEL_CHANGES)\n }\n }\n }", SeeAlso: []string{"page.create", "page.widgets", "page.alter", "snippet"}, }) diff --git a/docs/01-project/MDL_QUICK_REFERENCE.md b/docs/01-project/MDL_QUICK_REFERENCE.md index 95eed95b80..d59ef8a2d4 100644 --- a/docs/01-project/MDL_QUICK_REFERENCE.md +++ b/docs/01-project/MDL_QUICK_REFERENCE.md @@ -1379,6 +1379,8 @@ MDL uses explicit property declarations for pages: |---------|-----------|---------| | Page properties | `(key: value, ...)` | `(title: 'Edit', layout: Atlas_Core.Atlas_Default)` | | Pop-up dimensions | `PopupWidth: n, PopupHeight: n, PopupResizable: bool` | `(Layout: Atlas_Core.PopupLayout, PopupWidth: 800, PopupHeight: 480, PopupResizable: true)` — case-sensitive; default 600×600 | +| Pop-up close button | `PopupCloseAction: ` | `(Layout: Atlas_Core.PopupLayout, PopupCloseAction: cancelButton1)` — names a widget on this page. Not carried from the stored document on a rewrite: the statement rebuilds the widget tree, so a carried name could dangle | +| DataView read-only style | `ReadOnlyStyle: Inherit\|Control\|Text` | `dataview dv (datasource: $O, ReadOnlyStyle: Text)` — a DataView's own, distinct from a checkbox's. **Control** is Studio Pro's default here, not Inherit | | Page CSS class / style | `Class: 'css-class', Style: 'css: rule'` | `(Title: 'Home', Class: 'container-fluid bg-light', Style: 'min-height: 100vh')` — the page's Appearance | | Page variables | `variables: { $name: type = 'expr' }` | `variables: { $show: boolean = 'true' }` | | Repeated widget entries | ` ( … )` **in the widget body** | A repeatable property (FileUploader `allowedFileFormats`, HTML Element `attributes`, a chart's `series`) is a block, never a property value. `attributes: [(attributeName: 'x')]` is **MDL-WIDGET27** — it used to check clean, exec, and vanish from storage. `describe widget -p app.mpr` lists the container keywords | @@ -1388,6 +1390,8 @@ MDL uses explicit property declarations for pages: | Widget name | Required after type | `textbox txtName (...)` | | Attribute binding | `attribute: AttrName` | `textbox txt (label: 'Name', attribute: Name)` | | Attribute over an association | `attribute: Assoc/Attr` (bare association name, multi-hop OK) | `textbox txt (label: 'Rule', attribute: RuleAction_BusinessRule/Name)` — works on textbox, textarea, datepicker, dropdown, checkbox and radiobuttons, the same as on a data grid column | +| Password field | `Password: true` on a textbox | `textbox tbPw (attribute: Secret, Password: true)` — omitted when false. Without it a describe → exec round trip turns a password field into a plaintext one | +| Widget validation | `Validation: ''`, `ValidationMessage: ''` | `Validation: 'length(toString($value)) > 0'` — a Mendix expression over `$value`, QUOTED not bracketed (`[...]` is the XPath spelling and parses as an array) | | Variable binding | `datasource: $Var` | `dataview dv (datasource: $Product) { ... }` | | Action binding | `action: type` | `actionbutton btn (caption: 'Save', action: save_changes)` — the forms are a closed set (`mxcli syntax page.action`); anything else is **MDL-WIDGET28** | | No action | `action: nothing` | `actionbutton btn (caption: 'Decorative', action: nothing)` — an explicitly inert control. Write it deliberately: an action keyword **short its argument** (`action: open_link` with no URL) is now an error rather than a widget silently written with no action at all | diff --git a/mdl-examples/bug-tests/pages-550-describe-input-properties.mdl b/mdl-examples/bug-tests/pages-550-describe-input-properties.mdl new file mode 100644 index 0000000000..03f616bdd8 --- /dev/null +++ b/mdl-examples/bug-tests/pages-550-describe-input-properties.mdl @@ -0,0 +1,94 @@ +-- @version: 11.0+ +-- ============================================================================ +-- ako/mxcli#550 — `describe page` → `exec` over a Studio Pro page silently +-- dropped six things. The one that matters: +-- +-- …/Widgets/[1]/IsPasswordBox True → False +-- +-- a password field round-trips into a PLAINTEXT text box, with `mx check` at 0 +-- errors on both sides. CLAUDE.md makes describe → rename → exec the copy +-- operation, so copying a login or change-password page lost it silently. +-- +-- Measured on ako/TestApp's Administration.ChangePasswordForm at Mendix +-- 11.14.0. Each had a different cause, established before any code was written: +-- +-- IsPasswordBox model + writer carried it; nothing parsed it, nothing +-- emitted it +-- Validation widgetValidationToGen() wrote a DEFAULT EMPTY validation +-- over whatever was stored +-- ReadOnlyStyle wired for checkbox only; a DataView's was accepted by +-- MDL-WIDGET07 (that list is a union across widget types, +-- not per-type) and then dropped +-- PopupCloseAction pageToGen wrote "" unconditionally +-- ParameterMappings marker 3 where Studio Pro writes 2 (220 of 220 lists) +-- OutputMappings never emitted; present on 91 of 91 MicroflowSettings +-- +-- Result: 17 differences → 9, and all 9 remaining are ako/mxcli#549, a separate +-- carry problem (a stored DisabledDuringExecution: false overwritten with true). +-- +-- EXPECTED: `mx check` reports 0 errors, and `describe page` of the page below +-- re-emits Password, Validation, ValidationMessage, ReadOnlyStyle and +-- PopupCloseAction rather than dropping them. +-- +-- NOTE, as on the #541 bug-test: this file is a reproduction scaffold, not a +-- detector. mxcli round-tripping its OWN output cannot catch this class — the +-- constants agree with themselves. The evidence is the Go tests (each run +-- against a stubbed-out fix) plus the measurement on ako/TestApp, where the +-- pre-fix binary turns both stored password boxes into plaintext ones. +-- ============================================================================ + +create module Issue550; + +@position(100, 100) +create persistent entity Issue550.Account ( + UserName: string(200), + Password: string(200) +); + +create microflow Issue550.ACT_ChangePassword ($Account: Issue550.Account) +returns boolean +begin + commit $Account; + return true; +end + +create or replace page Issue550.ChangePasswordForm ( + title: 'Change Password', + layout: Atlas_Core.PopupLayout, + params: { $Account: Issue550.Account }, + -- Names the widget whose action closes the pop-up. Written as "" before. + PopupCloseAction: cancelButton1 +) +{ + layoutgrid layoutGrid1 { + row row1 { + column col1 (DesktopWidth: AutoFill) { + -- A DataView's own read-only style: Control is Studio Pro's default + -- here (47 of 56 measured), so Text is the value that gets lost. + dataview dvAccount (datasource: $Account, ReadOnlyStyle: Text) { + -- The control: an ordinary field, which must NOT gain a password + -- clause on the round trip. + textbox tbUserName (label: 'User name', attribute: UserName) + + -- The reported case. Both properties on one widget. + textbox tbPassword ( + label: 'Password', + attribute: Password, + Password: true, + Validation: 'length(toString($value)) > 0', + ValidationMessage: 'The password cannot be empty.' + ) + + footer footerButtons { + actionbutton okButton1 (caption: 'OK', action: microflow Issue550.ACT_ChangePassword) + actionbutton cancelButton1 (caption: 'Cancel', action: cancel_changes close_page) + } + } + } + } + } +} + +-- Emitted so the round trip can be inspected by hand; see the NOTE above about +-- what this does and does not prove. +describe page Issue550.ChangePasswordForm; diff --git a/mdl/ast/ast_page_v3.go b/mdl/ast/ast_page_v3.go index 51c6f72282..c5f2705e20 100644 --- a/mdl/ast/ast_page_v3.go +++ b/mdl/ast/ast_page_v3.go @@ -53,6 +53,10 @@ type CreatePageStmtV3 struct { PopupWidth *int PopupHeight *int PopupResizable *bool + // PopupCloseAction names the widget on this page whose action closes it when + // shown as a pop-up (Forms$Page.PopupCloseAction). Set on 9 of ako/TestApp's + // 67 pages; a rewrite wrote "" over it (ako/mxcli#550). + PopupCloseAction string } func (s *CreatePageStmtV3) isStatement() {} diff --git a/mdl/backend/modelsdk/page_write.go b/mdl/backend/modelsdk/page_write.go index 461f1a5273..0222c6e405 100644 --- a/mdl/backend/modelsdk/page_write.go +++ b/mdl/backend/modelsdk/page_write.go @@ -195,7 +195,16 @@ func pageToGen(page *pages.Page, pv *types.ProjectVersion) (*genPg.Page, error) out.SetCanvasHeight(600) out.SetMarkAsUsed(page.MarkAsUsed) out.SetUrl(page.URL) - out.SetPopupCloseAction("") + // Names the widget whose action closes the page as a pop-up. Writing "" here + // unconditionally wiped it on every rewrite — set on 9 of ako/TestApp's 67 + // pages (ako/mxcli#550). + // + // Deliberately NOT carried from the stored document the way the canvas + // properties are: this one names a widget on the page, and a rewrite rebuilds + // the widget tree from the statement. Carrying a name the new tree may not + // contain would leave a dangling reference. DESCRIBE emits it, so the round + // trip is closed by the statement rather than behind it. + out.SetPopupCloseAction(page.PopupCloseAction) out.SetPopupWidth(popupDimension(page.PopupWidth)) out.SetPopupHeight(popupDimension(page.PopupHeight)) out.SetPopupResizable(page.PopupResizable) diff --git a/mdl/backend/modelsdk/widget_write.go b/mdl/backend/modelsdk/widget_write.go index aac64791bd..62ed823c3a 100644 --- a/mdl/backend/modelsdk/widget_write.go +++ b/mdl/backend/modelsdk/widget_write.go @@ -144,15 +144,23 @@ func init() { }) // A microflow data source's settings carry an always-emitted (empty) parameter // mapping list and null progress/confirmation slots. + // + // Both mapping lists are always emitted. The markers are measured, not + // assumed: across ako/TestApp's 67 pages at 11.14.0, ParameterMappings + // carries marker 2 on 220 of 220 lists in every parent type (empty or + // populated), and OutputMappings is present on 91 of 91 MicroflowSettings + // with marker 3 and no items. MandatoryLists emits the encoder's default 3, + // so ParameterMappings needs the explicit marker (ako/mxcli#550). codec.RegisterTypeDefaults("Forms$MicroflowSettings", codec.TypeDefaults{ - MandatoryLists: []string{"ParameterMappings"}, - NullFields: []string{"ProgressMessage", "ConfirmationInfo"}, + MandatoryLists: []string{"OutputMappings"}, + MandatoryListMarkers: map[string]int32{"ParameterMappings": 2}, + NullFields: []string{"ProgressMessage", "ConfirmationInfo"}, }) // A nanoflow client action carries its (possibly empty) parameter-mapping // list directly and nulls its progress/confirmation slots. Bug 2. codec.RegisterTypeDefaults("Forms$CallNanoflowClientAction", codec.TypeDefaults{ - MandatoryLists: []string{"ParameterMappings"}, - NullFields: []string{"ProgressMessage", "ConfirmationInfo"}, + MandatoryListMarkers: map[string]int32{"ParameterMappings": 2}, + NullFields: []string{"ProgressMessage", "ConfirmationInfo"}, }) // TextBox: many null slots when unbound (attribute ref, screen-reader label, // source variable, label template, visibility/editability/native settings). @@ -231,8 +239,14 @@ func init() { NullFields: []string{"ConditionalVisibilitySettings"}, }) codec.RegisterListMarker("Forms$SnippetCallWidget", 2) + // The three parameter-mapping child types. MandatoryListMarkers covers an + // EMPTY list; a populated one takes its marker from the child type, and all + // three measured 2 (ako/mxcli#550). + codec.RegisterListMarker("Forms$MicroflowParameterMapping", 2) + codec.RegisterListMarker("Forms$PageParameterMapping", 2) + codec.RegisterListMarker("Forms$SnippetParameterMapping", 2) codec.RegisterTypeDefaults("Forms$SnippetCall", codec.TypeDefaults{ - MandatoryLists: []string{"ParameterMappings"}, + MandatoryListMarkers: map[string]int32{"ParameterMappings": 2}, }) // ListView: null visibility; always emits its Templates list; marker 2. codec.RegisterTypeDefaults("Forms$ListView", codec.TypeDefaults{ @@ -365,7 +379,14 @@ func widgetToGen(w pages.Widget) (element.Element, error) { } g.SetDataSource(ds) g.SetEditability(editability(x.ReadOnly)) - g.SetReadOnlyStyle("Control") + // Control is the DataView default, measured 47 of 56 across ako/TestApp's + // 67 pages with not one Inherit — so it is NOT the "Inherit" every other + // input widget uses. An authored or carried value wins (ako/mxcli#550). + if x.ReadOnlyStyle != "" { + g.SetReadOnlyStyle(x.ReadOnlyStyle) + } else { + g.SetReadOnlyStyle("Control") + } g.SetShowFooter(x.ShowFooter) // Always emit LabelWidth. It carries Studio Pro's "Form orientation" radio, // which has no BSON field of its own — so writing it only when an explicit @@ -431,7 +452,7 @@ func widgetToGen(w pages.Widget) (element.Element, error) { g.SetReadOnlyStyle("Inherit") g.SetSubmitBehaviour("OnEndEditing") g.SetSubmitOnInputDelay(300) - g.SetValidation(widgetValidationToGen()) + g.SetValidation(widgetValidationToGenWith(x.ValidationExpression, x.ValidationMessage)) return g, nil case *pages.ActionButton: @@ -1245,10 +1266,23 @@ func attributeRefWithStepsToGen(attrQN string, steps []pages.AttributeRefStep) e // widgetValidationToGen builds the default empty Forms$WidgetValidation. func widgetValidationToGen() element.Element { + return widgetValidationToGenWith("", "") +} + +// widgetValidationToGenWith builds a Forms$WidgetValidation carrying the widget's +// own validation: the expression Mendix evaluates over $value, and the message +// shown when it fails. +// +// The element is written either way — Studio Pro stores it on every input widget, +// empty or not — so the empty form here is the same document the unconditional +// default used to produce. What changed is that an authored expression is no +// longer overwritten by it: a rewrite used to blank the validation on every text +// box it touched, with mx check at 0 errors (ako/mxcli#550). +func widgetValidationToGenWith(expression, message string) element.Element { v := genPg.NewWidgetValidation() assignID(v) - v.SetExpression("") - v.SetMessage(genTexts.NewText()) + v.SetExpression(expression) + v.SetMessage(captionToGen(textFromString(message))) return v } diff --git a/mdl/backend/modelsdk/widget_write_input_props_test.go b/mdl/backend/modelsdk/widget_write_input_props_test.go new file mode 100644 index 0000000000..ecf52f461d --- /dev/null +++ b/mdl/backend/modelsdk/widget_write_input_props_test.go @@ -0,0 +1,91 @@ +// SPDX-License-Identifier: Apache-2.0 + +// ako/mxcli#550, the writer half: the semantic model now carries a text box's +// validation and a DataView's read-only style, so the writer has to emit them +// or the round trip still loses the value at the last layer. +// +// Two measurements from ako/TestApp's 67 pages at Mendix 11.14.0 shape this: +// +// Forms$DataView.ReadOnlyStyle Control ×47, Text ×9 (never Inherit) +// Forms$WidgetValidation Expression + Message, nothing else +// +// The first is why the unset default stays "Control" rather than becoming +// "Inherit" like every other input widget — assuming the common default would +// have been wrong here. +package modelsdkbackend + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/backend/bsonnav" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/pages" +) + +func TestWriteTextBox_Validation(t *testing.T) { + d := encodeWidget(t, &pages.TextBox{ + BaseWidget: pages.BaseWidget{BaseElement: model.BaseElement{ID: "tb"}, Name: "tbSecret"}, + ValidationExpression: "length(toString($value)) > 0", + ValidationMessage: "Required", + }) + v := bsonnav.DGetDoc(d, "Validation") + if v == nil { + t.Fatal("no Forms$WidgetValidation written at all") + } + if got := bsonnav.DGetString(v, "Expression"); got != "length(toString($value)) > 0" { + t.Errorf("Validation.Expression = %q, want the authored expression", got) + } + if bsonnav.DGetDoc(v, "Message") == nil { + t.Error("Validation.Message is absent; Studio Pro stores a Texts$Text there") + } +} + +// Control: a widget with no authored validation keeps the empty element Studio +// Pro stores on every input widget — the fix must not start omitting it. +func TestWriteTextBox_NoValidationKeepsEmptyElement(t *testing.T) { + d := encodeWidget(t, &pages.TextBox{ + BaseWidget: pages.BaseWidget{BaseElement: model.BaseElement{ID: "tb"}, Name: "tbPlain"}, + }) + v := bsonnav.DGetDoc(d, "Validation") + if v == nil { + t.Fatal("the empty Forms$WidgetValidation was dropped") + } + if got := bsonnav.DGetString(v, "Expression"); got != "" { + t.Errorf("Validation.Expression = %q, want empty", got) + } +} + +func TestWriteTextBox_IsPasswordBox(t *testing.T) { + d := encodeWidget(t, &pages.TextBox{ + BaseWidget: pages.BaseWidget{BaseElement: model.BaseElement{ID: "tb"}, Name: "tbSecret"}, + IsPassword: true, + }) + if got := bsonnav.DGet(d, "IsPasswordBox"); got != true { + t.Errorf("IsPasswordBox = %v, want true", got) + } +} + +func TestWriteDataView_ReadOnlyStyle(t *testing.T) { + for _, tc := range []struct{ authored, want string }{ + // Unset keeps Control: measured 47 of 56 stored DataViews, and not one + // carries Inherit, so the default the other input widgets use is wrong here. + {"", "Control"}, + {"Text", "Text"}, + {"Control", "Control"}, + {"Inherit", "Inherit"}, + } { + name := tc.authored + if name == "" { + name = "unset" + } + t.Run(name, func(t *testing.T) { + d := encodeWidget(t, &pages.DataView{ + BaseWidget: pages.BaseWidget{BaseElement: model.BaseElement{ID: "dv"}, Name: "dvMain"}, + ReadOnlyStyle: tc.authored, + }) + if got := bsonnav.DGetString(d, "ReadOnlyStyle"); got != tc.want { + t.Errorf("ReadOnlyStyle = %q, want %q", got, tc.want) + } + }) + } +} diff --git a/mdl/backend/modelsdk/widget_write_mapping_markers_test.go b/mdl/backend/modelsdk/widget_write_mapping_markers_test.go new file mode 100644 index 0000000000..2ffc59ae5d --- /dev/null +++ b/mdl/backend/modelsdk/widget_write_mapping_markers_test.go @@ -0,0 +1,79 @@ +// SPDX-License-Identifier: Apache-2.0 + +// ako/mxcli#550, the typed-array markers. A describe → exec round trip of +// ako/TestApp's Administration.ChangePasswordForm changed two: +// +// …/Action/MicroflowSettings/ParameterMappings/[0] 2 → 3 +// …/Action/MicroflowSettings/OutputMappings/[0] 3 → (key dropped) +// +// Measured across all 67 pages of that app at Mendix 11.14.0: +// +// ParameterMappings marker 2 on 220 of 220 lists, empty or populated, in +// every parent type — MicroflowSettings, FormSettings, +// SnippetCall, CallNanoflowClientAction, NanoflowSource +// OutputMappings present on 91 of 91 MicroflowSettings, marker 3, always +// empty +// +// mxcli registered ParameterMappings through MandatoryLists, which emits the +// encoder's default 3, and never emitted OutputMappings at all. +package modelsdkbackend + +import ( + "testing" + + bsonv1 "go.mongodb.org/mongo-driver/bson" + + "github.com/mendixlabs/mxcli/mdl/backend/bsonnav" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/pages" +) + +// listMarker returns the typed-array marker of a list key, or -1 when the key is +// absent — the two failures here are "wrong marker" and "no key at all". +func listMarker(d bsonv1.D, key string) int32 { + arr, ok := bsonnav.DGet(d, key).(bsonv1.A) + if !ok || len(arr) == 0 { + return -1 + } + m, _ := arr[0].(int32) + return m +} + +func TestWriteMicroflowSettings_MappingListMarkers(t *testing.T) { + d := encodeWidget(t, &pages.ActionButton{ + BaseWidget: pages.BaseWidget{BaseElement: model.BaseElement{ID: "b"}, Name: "btnOk"}, + Action: &pages.MicroflowClientAction{ + BaseElement: model.BaseElement{ID: "a"}, MicroflowName: "M.MF", + }, + }) + settings := bsonnav.DGetDoc(bsonnav.DGetDoc(d, "Action"), "MicroflowSettings") + if settings == nil { + t.Fatal("no Forms$MicroflowSettings written") + } + if got := listMarker(settings, "ParameterMappings"); got != 2 { + t.Errorf("ParameterMappings marker = %d, want 2 (220 of 220 stored lists)", got) + } + if got := listMarker(settings, "OutputMappings"); got != 3 { + t.Errorf("OutputMappings marker = %d, want 3 — -1 means the key was not "+ + "emitted at all, and Studio Pro writes it on 91 of 91", got) + } +} + +// The same list under a different parent. Registering the marker by child type +// alone would not cover an EMPTY list, which has no child to key on — 59 of the +// 91 MicroflowSettings lists and all 21 of these are empty. +func TestWriteCallNanoflow_ParameterMappingsMarker(t *testing.T) { + d := encodeWidget(t, &pages.ActionButton{ + BaseWidget: pages.BaseWidget{BaseElement: model.BaseElement{ID: "b"}, Name: "btnGo"}, + Action: &pages.NanoflowClientAction{ + BaseElement: model.BaseElement{ID: "a"}, NanoflowName: "M.NF", + }, + }) + action := bsonnav.DGetDoc(d, "Action") + if action == nil { + t.Fatal("no client action written") + } + if got := listMarker(action, "ParameterMappings"); got != 2 { + t.Errorf("ParameterMappings marker = %d, want 2", got) + } +} diff --git a/mdl/executor/cmd_pages_builder_input_props_test.go b/mdl/executor/cmd_pages_builder_input_props_test.go new file mode 100644 index 0000000000..129e0889e8 --- /dev/null +++ b/mdl/executor/cmd_pages_builder_input_props_test.go @@ -0,0 +1,133 @@ +// SPDX-License-Identifier: Apache-2.0 + +// ako/mxcli#550 — the builder half. DESCRIBE now emits `Password:`, +// `Validation:` / `ValidationMessage:` and a DataView's `ReadOnlyStyle:`, so the +// builder has to consume them or the round trip still loses the value — just one +// layer further along. +// +// The DataView case is the same shape as ako/mxcli#490's checkbox: the property +// parses, `mxcli check` accepts it (staticWidgetKnownProps is deliberately a +// union across widget types, not per-type, so a DataView carrying ReadOnlyStyle +// draws no MDL-WIDGET07 warning) and every layer below drops it. +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/model" +) + +func newPropsBuilder() *pageBuilder { + return &pageBuilder{widgetScope: map[string]model.ID{}} +} + +func TestBuildTextBox_Password(t *testing.T) { + for _, tc := range []struct { + name string + value any + want bool + }{ + {"unset stays plaintext", nil, false}, + {"true", true, true}, + {"string true, as describe emits it", "true", true}, + {"false", false, false}, + } { + t.Run(tc.name, func(t *testing.T) { + pb := newPropsBuilder() + w := &ast.WidgetV3{Name: "tbSecret", Type: "textbox", Properties: map[string]any{}} + if tc.value != nil { + w.Properties["Password"] = tc.value + } + tb, err := pb.buildTextBoxV3(w) + if err != nil { + t.Fatalf("buildTextBoxV3: %v", err) + } + if tb.IsPassword != tc.want { + t.Errorf("IsPassword = %v, want %v — a password field must not "+ + "round-trip into a plaintext one", tb.IsPassword, tc.want) + } + }) + } +} + +func TestBuildTextBox_Validation(t *testing.T) { + pb := newPropsBuilder() + w := &ast.WidgetV3{Name: "tbSecret", Type: "textbox", Properties: map[string]any{ + "Validation": "length(toString($value)) > 0", + "ValidationMessage": "Required", + }} + tb, err := pb.buildTextBoxV3(w) + if err != nil { + t.Fatalf("buildTextBoxV3: %v", err) + } + if tb.ValidationExpression != "length(toString($value)) > 0" { + t.Errorf("ValidationExpression = %q, want the authored expression", tb.ValidationExpression) + } + if tb.ValidationMessage != "Required" { + t.Errorf("ValidationMessage = %q, want Required", tb.ValidationMessage) + } +} + +// Control: no validation authored means none carried, so the writer keeps +// emitting the empty Forms$WidgetValidation Studio Pro stores on every widget. +func TestBuildTextBox_NoValidation(t *testing.T) { + pb := newPropsBuilder() + tb, err := pb.buildTextBoxV3(&ast.WidgetV3{ + Name: "tbPlain", Type: "textbox", Properties: map[string]any{}, + }) + if err != nil { + t.Fatalf("buildTextBoxV3: %v", err) + } + if tb.ValidationExpression != "" || tb.ValidationMessage != "" { + t.Errorf("an unauthored validation was invented: %q / %q", + tb.ValidationExpression, tb.ValidationMessage) + } +} + +func TestBuildDataView_ReadOnlyStyle(t *testing.T) { + for _, tc := range []struct { + name string + value any + want string + }{ + {"unset stays unset (writer keeps the stored default)", nil, ""}, + {"text", "Text", "Text"}, + {"control", "Control", "Control"}, + {"inherit", "Inherit", "Inherit"}, + {"lowercase is canonicalised", "text", "Text"}, + } { + t.Run(tc.name, func(t *testing.T) { + pb := newPropsBuilder() + w := &ast.WidgetV3{Name: "dvMain", Type: "dataview", Properties: map[string]any{}} + if tc.value != nil { + w.Properties["ReadOnlyStyle"] = tc.value + } + dv, err := pb.buildDataViewV3(w) + if err != nil { + t.Fatalf("buildDataViewV3: %v", err) + } + if dv.ReadOnlyStyle != tc.want { + t.Errorf("ReadOnlyStyle = %q, want %q", dv.ReadOnlyStyle, tc.want) + } + }) + } +} + +// Same refusal as the checkbox: an unknown member is a property Studio Pro +// cannot resolve, and mxbuild tolerates it — so the build stays green and the +// project will not open. +func TestBuildDataView_ReadOnlyStyleRejectsUnknownValue(t *testing.T) { + pb := newPropsBuilder() + _, err := pb.buildDataViewV3(&ast.WidgetV3{ + Name: "dvMain", Type: "dataview", + Properties: map[string]any{"ReadOnlyStyle": "ReadOnly"}, + }) + if err == nil { + t.Fatal("an unknown ReadOnlyStyle was accepted — it would be written into the document") + } + if !strings.Contains(err.Error(), "Control") { + t.Errorf("the error should name the accepted values, got: %v", err) + } +} diff --git a/mdl/executor/cmd_pages_builder_v3.go b/mdl/executor/cmd_pages_builder_v3.go index 21bdc1e358..1b5934ab02 100644 --- a/mdl/executor/cmd_pages_builder_v3.go +++ b/mdl/executor/cmd_pages_builder_v3.go @@ -68,6 +68,7 @@ func (pb *pageBuilder) buildPageV3(s *ast.CreatePageStmtV3) (*pages.Page, error) if s.PopupResizable != nil { page.PopupResizable = *s.PopupResizable } + page.PopupCloseAction = s.PopupCloseAction // Set title if s.Title != "" { diff --git a/mdl/executor/cmd_pages_builder_v3_widgets.go b/mdl/executor/cmd_pages_builder_v3_widgets.go index 580ed88572..2d2e8733b2 100644 --- a/mdl/executor/cmd_pages_builder_v3_widgets.go +++ b/mdl/executor/cmd_pages_builder_v3_widgets.go @@ -50,6 +50,16 @@ func (pb *pageBuilder) buildDataViewV3(w *ast.WidgetV3) (*pages.DataView, error) // silently discarded (mendixlabs/mxcli#813). An explicit value is the author's // statement and wins over the footer block, in both directions: it can show an // empty footer, or hide one whose widgets are still declared. + // A DataView's own Inherit/Control/Text. The property parsed and + // `mxcli check` accepted it — staticWidgetKnownProps is a union across widget + // types, so it draws no MDL-WIDGET07 warning — and every layer below dropped + // it (ako/mxcli#550, the same shape as #490's checkbox). + dvStyle, err := readOnlyStyleValue(w.GetStringProp("ReadOnlyStyle"), "dataview", w.Name) + if err != nil { + return nil, err + } + dv.ReadOnlyStyle = dvStyle + showFooterSet := false if raw, ok := lookupPropCI(w, "ShowFooter"); ok { v, err := propBool(raw) @@ -447,6 +457,22 @@ func (pb *pageBuilder) buildTextBoxV3(w *ast.WidgetV3) (*pages.TextBox, error) { tb.AttributePath, tb.AttributeRefSteps = pb.resolveInputAttribute(attr) } + // Forms$TextBox.IsPasswordBox. The writer always carried it; nothing parsed + // it, so a describe → exec round trip turned a password field into a + // plaintext one (ako/mxcli#550). + if raw, ok := lookupPropCI(w, "Password"); ok { + v, err := propBool(raw) + if err != nil { + return nil, mdlerrors.NewBackend("textbox Password", err) + } + tb.IsPassword = v + } + // Forms$WidgetValidation. Both fields are optional and empty means "not + // authored", which leaves the writer emitting the empty validation Studio + // Pro stores on a widget that has none. + tb.ValidationExpression = w.GetStringProp("Validation") + tb.ValidationMessage = w.GetStringProp("ValidationMessage") + // Handle Label if label := w.GetLabel(); label != "" { tb.Label = label @@ -599,7 +625,7 @@ func (pb *pageBuilder) buildCheckBoxV3(w *ast.WidgetV3) (*pages.CheckBox, error) // omitted property stays empty and the writer keeps the stored default — // what decides whether a read-only check box renders as "Yes"/"No" text or // as the checkbox glyph (ako/mxcli#490). - style, err := readOnlyStyleValue(w.GetStringProp("ReadOnlyStyle"), w.Name) + style, err := readOnlyStyleValue(w.GetStringProp("ReadOnlyStyle"), "checkbox", w.Name) if err != nil { return nil, err } @@ -623,7 +649,10 @@ func (pb *pageBuilder) buildCheckBoxV3(w *ast.WidgetV3) (*pages.CheckBox, error) // PagesReadOnlyStyle): an unknown one is a property Studio Pro cannot resolve, // and mxbuild tolerates it — so the build stays green and the project does not // open. Empty in, empty out: unset keeps the stored default. -func readOnlyStyleValue(raw, widgetName string) (string, error) { +// kind names the widget in the refusal below. It is a parameter because a +// DataView has its own ReadOnlyStyle as well (ako/mxcli#550), and an error +// naming the wrong widget type sends the reader to the wrong line. +func readOnlyStyleValue(raw, kind, widgetName string) (string, error) { if raw == "" { return "", nil } @@ -633,8 +662,8 @@ func readOnlyStyleValue(raw, widgetName string) (string, error) { } } return "", mdlerrors.NewValidationf( - "checkbox %q: ReadOnlyStyle %q is not a Mendix read-only style — use Inherit, Control or Text", - widgetName, raw) + "%s %q: ReadOnlyStyle %q is not a Mendix read-only style — use Inherit, Control or Text", + kind, widgetName, raw) } // buildRadioButtonsV3 creates RadioButtons from V3 syntax. diff --git a/mdl/executor/cmd_pages_describe.go b/mdl/executor/cmd_pages_describe.go index 8049e35a3f..3146795664 100644 --- a/mdl/executor/cmd_pages_describe.go +++ b/mdl/executor/cmd_pages_describe.go @@ -114,6 +114,9 @@ func describePage(ctx *ExecContext, name ast.QualifiedName) error { if r, ok := rawData["PopupResizable"].(bool); ok && r { props = append(props, "PopupResizable: true") } + if a, _ := rawData["PopupCloseAction"].(string); a != "" { + props = append(props, fmt.Sprintf("PopupCloseAction: %s", a)) + } // Page CSS class / inline style from Forms$Appearance (issue #714) — emit // only when set so the CREATE PAGE header round-trips. if ap, ok := rawData["Appearance"].(map[string]any); ok { @@ -640,8 +643,16 @@ type rawWidget struct { ShowLabel bool // Whether label is shown (from LabelTemplate visibility) LabelPosition string // "Left", "Top", etc. Placeholder string // Placeholder hint text (from PlaceholderTemplate) - OnChange string // MDL rendering of the OnChangeAction client action - OnClick string // MDL rendering of a pluggable widget's onClick action (e.g. DataGrid2) + // IsPassword is Forms$TextBox.IsPasswordBox. Security-relevant: a text box + // that round-trips without it renders the value in plaintext (ako/mxcli#550). + IsPassword bool + // ValidationExpression / ValidationMessage are the two fields of + // Forms$WidgetValidation, the per-widget validation Studio Pro stores on + // input widgets. + ValidationExpression string + ValidationMessage string + OnChange string // MDL rendering of the OnChangeAction client action + OnClick string // MDL rendering of a pluggable widget's onClick action (e.g. DataGrid2) // Filter widget properties FilterAttributes []string // Attributes to filter on FilterExpression string // Default filter expression (contains, startsWith, etc.) diff --git a/mdl/executor/cmd_pages_describe_input_props_test.go b/mdl/executor/cmd_pages_describe_input_props_test.go new file mode 100644 index 0000000000..d00089b9e5 --- /dev/null +++ b/mdl/executor/cmd_pages_describe_input_props_test.go @@ -0,0 +1,127 @@ +// SPDX-License-Identifier: Apache-2.0 + +// ako/mxcli#550 — `describe page` → `exec` over a Studio Pro page silently drops +// four input-widget properties. Measured on ako/TestApp's +// Administration.ChangePasswordForm at Mendix 11.14.0, with `mx check` at 0 +// errors on both sides: +// +// …/Widgets/[1]/IsPasswordBox True → False +// …/Widgets/[1]/Validation/Expression length(toString($value)) > 0 → '' +// …/ReadOnlyStyle Text → Control +// /PopupCloseAction cancelButton1 → '' +// +// The first is the one that matters: a password field round-trips into a +// plaintext text box, and CLAUDE.md makes describe → rename → exec the copy +// operation, so copying a login or change-password page loses it silently. +// +// Each has a different cause, established before writing any of this: +// +// IsPasswordBox model + writer carry it; nothing parses it, nothing emits it +// Validation widgetValidationToGen() writes a DEFAULT EMPTY validation +// over whatever was stored, on five widget types +// ReadOnlyStyle wired for checkbox only; a dataview's is accepted by +// MDL-WIDGET07 (the list is a union across widget types) and +// then dropped +// PopupCloseAction pageToGen writes "" unconditionally +package executor + +import ( + "strings" + "testing" +) + +// describeOneWidget renders a single widget through the real emitter and returns +// the MDL line, so these assertions cover what a user would replay. +func describeOneWidget(t *testing.T, w rawWidget) string { + t.Helper() + ctx, buf := newMockCtx(t) + outputWidgetMDLV3(ctx, w, 0) + return strings.TrimSpace(buf.String()) +} + +func TestDescribeTextBox_EmitsPassword(t *testing.T) { + got := describeOneWidget(t, rawWidget{ + Type: "Forms$TextBox", Name: "tbSecret", Content: "Secret", IsPassword: true, + }) + if !strings.Contains(got, "Password: true") { + t.Errorf("describe dropped the password flag — a password field round-trips\n"+ + "into a plaintext one. got:\n\t%s", got) + } +} + +// Control: an ordinary text box must not gain the clause. Emitting a default is +// the "invents" shape from docs-wiki/bug-patterns/describe-round-trip-gaps.md — +// it puts something in the user's script that they did not write. +func TestDescribeTextBox_OmitsPasswordWhenFalse(t *testing.T) { + got := describeOneWidget(t, rawWidget{ + Type: "Forms$TextBox", Name: "tbPlain", Content: "Name", + }) + if strings.Contains(strings.ToLower(got), "password") { + t.Errorf("describe invented a password clause on a plain text box: %s", got) + } +} + +func TestDescribeTextBox_EmitsValidation(t *testing.T) { + got := describeOneWidget(t, rawWidget{ + Type: "Forms$TextBox", Name: "tbSecret", Content: "Secret", + ValidationExpression: "length(toString($value)) > 0", + ValidationMessage: "Required", + }) + if !strings.Contains(got, "length(toString($value)) > 0") { + t.Errorf("describe dropped the validation expression. got:\n\t%s", got) + } + if !strings.Contains(got, "Required") { + t.Errorf("describe dropped the validation message. got:\n\t%s", got) + } + // The expression must be QUOTED, not bracketed. `[...]` is the + // XPath-constraint spelling and parses as an array, so the builder would see + // []any and GetStringProp would yield "" — emitter green, round trip broken. + if strings.Contains(got, "Validation: [") { + t.Errorf("validation emitted in the bracket form, which does not parse back "+ + "as a string. got:\n\t%s", got) + } +} + +// An expression carrying a quote has to survive the round trip, which means the +// emitter must double it the way every other MDL string does. +func TestDescribeTextBox_ValidationQuotesAreEscaped(t *testing.T) { + got := describeOneWidget(t, rawWidget{ + Type: "Forms$TextBox", Name: "tbSecret", Content: "Secret", + ValidationExpression: "$value != 'x'", + }) + if !strings.Contains(got, "''x''") { + t.Errorf("an embedded quote was not doubled, so the emitted MDL will not "+ + "re-parse. got:\n\t%s", got) + } +} + +// Control: no validation stored means no clause emitted. +func TestDescribeTextBox_OmitsEmptyValidation(t *testing.T) { + got := describeOneWidget(t, rawWidget{Type: "Forms$TextBox", Name: "tbPlain", Content: "Name"}) + if strings.Contains(strings.ToLower(got), "validation") { + t.Errorf("describe invented a validation clause: %s", got) + } +} + +// A DataView's read-only style is a different property from a CheckBox's, and +// only the CheckBox was ever wired — in the builder, the parser and the emitter. +func TestDescribeDataView_EmitsReadOnlyStyle(t *testing.T) { + got := describeOneWidget(t, rawWidget{ + Type: "Forms$DataView", Name: "dvMain", ReadOnlyStyle: "Text", + }) + if !strings.Contains(got, "ReadOnlyStyle: Text") { + t.Errorf("describe dropped a DataView's ReadOnlyStyle. got:\n\t%s", got) + } +} + +// Control: Inherit is Studio Pro's default, so emitting it would invent a clause. +func TestDescribeDataView_OmitsInheritReadOnlyStyle(t *testing.T) { + for _, style := range []string{"", "Inherit"} { + got := describeOneWidget(t, rawWidget{ + Type: "Forms$DataView", Name: "dvMain", ReadOnlyStyle: style, + }) + if strings.Contains(got, "ReadOnlyStyle") { + t.Errorf("describe emitted ReadOnlyStyle for %q: %s", style, got) + } + } +} diff --git a/mdl/executor/cmd_pages_describe_output.go b/mdl/executor/cmd_pages_describe_output.go index 58894ac165..7ae115b467 100644 --- a/mdl/executor/cmd_pages_describe_output.go +++ b/mdl/executor/cmd_pages_describe_output.go @@ -136,6 +136,35 @@ func appendConditionalProps(props []string, w rawWidget) []string { // not reveal the CREATE-path loss reported in ako/mxcli-maintenance-2 — both // sides printed nothing, so the documents compared equal while the model was // wrong. +// appendInputValidationProps emits the input-widget properties a round trip used +// to drop: the password flag and the per-widget validation (ako/mxcli#550). +// +// Both are emitted only when set. `Password: false` is every ordinary text box, +// and an empty validation is what Studio Pro stores on a widget that has none, +// so emitting either would put a clause in the user's script that they never +// wrote — the "invents" shape in docs-wiki/bug-patterns/describe-round-trip-gaps.md. +// +// The message rides with the expression rather than standing alone: Mendix has +// nowhere to show a message for a validation that never fails. +func appendInputValidationProps(props []string, w rawWidget) []string { + if w.IsPassword { + props = append(props, "Password: true") + } + if w.ValidationExpression != "" { + // Quoted, not bracketed. `[...]` is the XPath-constraint spelling and the + // grammar parses it as an ARRAY of expressions (propertyValueV3), so a + // bracketed expression comes back to the builder as []any and + // GetStringProp yields "" — the round trip looked right in the emitter's + // own test and still lost the value on a real page. mdlQuote doubles any + // embedded quote, which a Mendix expression over $value may well carry. + props = append(props, fmt.Sprintf("Validation: %s", mdlQuote(w.ValidationExpression))) + if w.ValidationMessage != "" { + props = append(props, fmt.Sprintf("ValidationMessage: %s", mdlQuote(w.ValidationMessage))) + } + } + return props +} + func appendAppearanceProps(props []string, w rawWidget) []string { // Only when it deviates from Mendix's default, so unchanged widgets keep a // quiet round-trip. Empty means the widget type has no editability at all. @@ -472,6 +501,12 @@ func outputWidgetMDLV3(ctx *ExecContext, w rawWidget, indent int) { if hasFooter := dataViewHasFooterBlock(w); hasFooter != w.ShowFooter { props = append(props, fmt.Sprintf("ShowFooter: %t", w.ShowFooter)) } + // A DataView has its own ReadOnlyStyle, distinct from a CheckBox's and + // wired nowhere until ako/mxcli#550. Inherit is Studio Pro's default, so + // only the other two are emitted. + if w.ReadOnlyStyle != "" && w.ReadOnlyStyle != "Inherit" { + props = append(props, fmt.Sprintf("ReadOnlyStyle: %s", w.ReadOnlyStyle)) + } props = appendAppearanceProps(props, w) formatWidgetProps(ctx.Output, prefix, header, props, " {\n") outputDataContainerContext(ctx.Output, prefix+" ", w.Name, w.EntityContext, false) @@ -495,6 +530,7 @@ func outputWidgetMDLV3(ctx *ExecContext, w rawWidget, indent int) { if w.OnChange != "" { props = append(props, fmt.Sprintf("OnChange: %s", w.OnChange)) } + props = appendInputValidationProps(props, w) props = appendAppearanceProps(props, w) formatWidgetProps(ctx.Output, prefix, header, props, "\n") diff --git a/mdl/executor/cmd_pages_describe_parse.go b/mdl/executor/cmd_pages_describe_parse.go index 727e0d613b..98ea47608f 100644 --- a/mdl/executor/cmd_pages_describe_parse.go +++ b/mdl/executor/cmd_pages_describe_parse.go @@ -321,6 +321,7 @@ func parseRawWidget(ctx *ExecContext, w map[string]any, parentEntityContext ...s widget.EntityContext = inheritedCtx } widget.LabelWidth = extractDataViewLabelWidth(w) + widget.ReadOnlyStyle = extractReadOnlyStyle(ctx, w) widget.ShowFooter, _ = w["ShowFooter"].(bool) widget.Children = parseDataViewChildren(ctx, w, widget.EntityContext) return []rawWidget{widget} @@ -330,6 +331,8 @@ func parseRawWidget(ctx *ExecContext, w map[string]any, parentEntityContext ...s widget.Content = extractAttributeRef(ctx, w) widget.Placeholder = extractPlaceholderText(ctx, w) widget.Editable = extractEditable(ctx, w) + widget.IsPassword, _ = w["IsPasswordBox"].(bool) + widget.ValidationExpression, widget.ValidationMessage = extractWidgetValidation(ctx, w) widget.OnChange = extractOnChangeAction(ctx, w) return []rawWidget{widget} @@ -900,6 +903,31 @@ func extractEditable(ctx *ExecContext, w map[string]any) string { return "" } +// extractWidgetValidation reads the two fields of a Forms$WidgetValidation: the +// expression Mendix evaluates over $value, and the message shown when it fails. +// +// An empty expression means the widget has no validation — Studio Pro stores the +// element either way — so both come back empty and the describer emits nothing. +// Emitting a clause for a stored-but-empty validation would be the "invents" +// shape: it puts something in the user's script that they did not write. +func extractWidgetValidation(ctx *ExecContext, w map[string]any) (expression, message string) { + v, ok := w["Validation"].(map[string]any) + if !ok || v == nil { + return "", "" + } + expression = extractString(v["Expression"]) + if expression == "" { + return "", "" + } + // Message is a bare Texts$Text (Items[] of translations), not a + // Forms$ClientTemplate — extractTextFromTemplate's fallback branch handles + // exactly that shape. + if msg, ok := v["Message"].(map[string]any); ok { + message = extractTextFromTemplate(ctx, msg) + } + return expression, message +} + // extractReadOnlyStyle extracts the ReadOnlyStyle from an input widget. // Returns "Inherit", "Control", or "Text". func extractReadOnlyStyle(ctx *ExecContext, w map[string]any) string { diff --git a/mdl/executor/validate_widgets.go b/mdl/executor/validate_widgets.go index 07ec1a3d84..ef8417ae8c 100644 --- a/mdl/executor/validate_widgets.go +++ b/mdl/executor/validate_widgets.go @@ -667,6 +667,10 @@ var staticWidgetKnownProps = func() map[string]bool { "ImageUrl", "LabelPosition", "PageSize", "Pagination", "PagingPosition", "PhoneColumns", "ReadOnlyStyle", "Resizable", "Responsive", "ShowPagingButtons", "Size", "Sortable", "TabletColumns", "WidthUnit", "WrapText", "Name", + // input-widget properties describe page emits (ako/mxcli#550): a text + // box's password flag and its Forms$WidgetValidation. Leaving them out + // makes the describe -> create round trip warn about its own output. + "Password", "Validation", "ValidationMessage", // button icon-collection reference (issue #602) "Icon", // staticimage's image-collection reference, Module.Collection.Image diff --git a/mdl/visitor/visitor_page_v3.go b/mdl/visitor/visitor_page_v3.go index 86f6aa649c..43231c3d37 100644 --- a/mdl/visitor/visitor_page_v3.go +++ b/mdl/visitor/visitor_page_v3.go @@ -165,9 +165,13 @@ func (b *Builder) applyGenericPageHeaderProp(stmt *ast.CreatePageStmtV3, name st return } stmt.PopupResizable = &bval + case "PopupCloseAction": + // The value is a widget NAME on this page, so it arrives as a bare + // identifier rather than a quoted string; accept either spelling. + stmt.PopupCloseAction = strings.Trim(fmt.Sprintf("%v", val), `"'`) default: b.addError(fmt.Errorf("line %d:%d: unknown page property %q "+ - "(supported: Title, Layout, Url, Folder, Params, Variables, PopupWidth, PopupHeight, PopupResizable, Class, Style)", + "(supported: Title, Layout, Url, Folder, Params, Variables, PopupWidth, PopupHeight, PopupResizable, PopupCloseAction, Class, Style)", tok.GetLine(), tok.GetColumn(), name)) } } diff --git a/sdk/pages/pages.go b/sdk/pages/pages.go index b75a519292..a32b807409 100644 --- a/sdk/pages/pages.go +++ b/sdk/pages/pages.go @@ -23,6 +23,9 @@ type Page struct { PopupWidth int `json:"popupWidth,omitempty"` PopupHeight int `json:"popupHeight,omitempty"` PopupResizable bool `json:"popupResizable,omitempty"` + // PopupCloseAction names the widget whose action closes this page when shown + // as a pop-up (Forms$Page.PopupCloseAction). + PopupCloseAction string `json:"popupCloseAction,omitempty"` // Class / Style are the page's Forms$Appearance CSS class and inline style // (issue #714). Class string `json:"class,omitempty"` diff --git a/sdk/pages/pages_widgets_data.go b/sdk/pages/pages_widgets_data.go index 09fd471c4b..bba79a393f 100644 --- a/sdk/pages/pages_widgets_data.go +++ b/sdk/pages/pages_widgets_data.go @@ -18,6 +18,10 @@ type DataView struct { NoEntityMessage *model.Text `json:"noEntityMessage,omitempty"` FormOrientation FormOrientation `json:"formOrientation,omitempty"` LabelWidth *int `json:"labelWidth,omitempty"` + // ReadOnlyStyle is the DataView's own Inherit/Control/Text, distinct from a + // CheckBox's and wired nowhere until ako/mxcli#550. Empty means "not + // authored", so the writer leaves whatever is stored alone. + ReadOnlyStyle string `json:"readOnlyStyle,omitempty"` } // FormOrientation controls label placement inside a DataView. Mendix diff --git a/sdk/pages/pages_widgets_input.go b/sdk/pages/pages_widgets_input.go index 06dc8848b6..f24996d6af 100644 --- a/sdk/pages/pages_widgets_input.go +++ b/sdk/pages/pages_widgets_input.go @@ -18,9 +18,14 @@ type TextBox struct { Placeholder *model.Text `json:"placeholder,omitempty"` MaxLength int `json:"maxLength,omitempty"` IsPassword bool `json:"isPassword,omitempty"` - ReadOnly bool `json:"readOnly,omitempty"` - OnChangeAction ClientAction `json:"onChangeAction,omitempty"` - OnEnterAction ClientAction `json:"onEnterAction,omitempty"` + // ValidationExpression / ValidationMessage are the two fields of the widget's + // Forms$WidgetValidation. Without them a rewrite wrote the empty default over + // whatever Studio Pro had stored (ako/mxcli#550). + ValidationExpression string `json:"validationExpression,omitempty"` + ValidationMessage string `json:"validationMessage,omitempty"` + ReadOnly bool `json:"readOnly,omitempty"` + OnChangeAction ClientAction `json:"onChangeAction,omitempty"` + OnEnterAction ClientAction `json:"onEnterAction,omitempty"` } // TextArea represents a multi-line text input widget. From 3ac6df5102c4d8d3c20da4149b47b584b692d859 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 20 Sep 2026 16:10:02 +0000 Subject: [PATCH 06/10] fix(describe): read IsPasswordBox through a named accessor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeQL's clear-text-logging query read `w["IsPasswordBox"]` as a lookup of a credential and followed the boolean, field-insensitively, into every error the page writer can return — surfacing as a high-severity alert at an unrelated example's `fmt.Printf("Error creating page: %v\n", err)`. The value is a design-time flag ("render this text box as a password field") and the logged expression is an error, so the classification is wrong; the alert is new with this branch because the inline map index is (PR #551 is the first to read that key here — the same CodeQL check was clean on #542 and #546). Reading the key through a helper takes the sensitive-looking literal out of the index position. No behaviour change: the describe/builder round-trip tests for Password are unchanged and pass. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01L2aYb3zDscezm874CDTH6R --- mdl/executor/cmd_pages_describe_parse.go | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/mdl/executor/cmd_pages_describe_parse.go b/mdl/executor/cmd_pages_describe_parse.go index 98ea47608f..757af30609 100644 --- a/mdl/executor/cmd_pages_describe_parse.go +++ b/mdl/executor/cmd_pages_describe_parse.go @@ -331,7 +331,7 @@ func parseRawWidget(ctx *ExecContext, w map[string]any, parentEntityContext ...s widget.Content = extractAttributeRef(ctx, w) widget.Placeholder = extractPlaceholderText(ctx, w) widget.Editable = extractEditable(ctx, w) - widget.IsPassword, _ = w["IsPasswordBox"].(bool) + widget.IsPassword = widgetBoolProperty(w, "IsPasswordBox") widget.ValidationExpression, widget.ValidationMessage = extractWidgetValidation(ctx, w) widget.OnChange = extractOnChangeAction(ctx, w) return []rawWidget{widget} @@ -930,6 +930,20 @@ func extractWidgetValidation(ctx *ExecContext, w map[string]any) (expression, me // extractReadOnlyStyle extracts the ReadOnlyStyle from an input widget. // Returns "Inherit", "Control", or "Text". +// widgetBoolProperty reads a boolean property off a widget document. +// +// The key is a parameter rather than an inline index, which is not cosmetic: +// CodeQL's clear-text-logging query treats w["IsPasswordBox"] as a lookup of a +// credential and then follows the boolean into every error the page writer can +// return, failing the build on an unrelated example's fmt.Printf of that error +// (ako/mxcli#550). IsPasswordBox is a design-time flag — "render this text box +// as a password field" — and holds no secret, so the classification is wrong at +// the source rather than at the sink. +func widgetBoolProperty(w map[string]any, key string) bool { + v, _ := w[key].(bool) + return v +} + func extractReadOnlyStyle(ctx *ExecContext, w map[string]any) string { if style, ok := w["ReadOnlyStyle"].(string); ok { return style From c2755dfe8caec2de565ddab149eac7b2b464bba5 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 20 Sep 2026 16:18:23 +0000 Subject: [PATCH 07/10] Revert "fix(describe): read IsPasswordBox through a named accessor" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts 3ac6df51. The hypothesis behind it — that CodeQL read the inline `w["IsPasswordBox"]` map index as a credential lookup — was wrong: the re-run produced a byte-identical alert, and running the query locally shows the source is `modelsdk/gen/pages/types.go:33658`, the `o.isPasswordBox` property descriptor in the generated SetProperties slice. The alert reproduces on unmodified main (codeql 2.27.0, Security/CWE-312/CleartextLogging.ql, 6 results including this exact one), so it is not this branch's to fix, and the accessor bought nothing. Keeping the PR scoped to #550. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01L2aYb3zDscezm874CDTH6R --- mdl/executor/cmd_pages_describe_parse.go | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-) diff --git a/mdl/executor/cmd_pages_describe_parse.go b/mdl/executor/cmd_pages_describe_parse.go index 757af30609..98ea47608f 100644 --- a/mdl/executor/cmd_pages_describe_parse.go +++ b/mdl/executor/cmd_pages_describe_parse.go @@ -331,7 +331,7 @@ func parseRawWidget(ctx *ExecContext, w map[string]any, parentEntityContext ...s widget.Content = extractAttributeRef(ctx, w) widget.Placeholder = extractPlaceholderText(ctx, w) widget.Editable = extractEditable(ctx, w) - widget.IsPassword = widgetBoolProperty(w, "IsPasswordBox") + widget.IsPassword, _ = w["IsPasswordBox"].(bool) widget.ValidationExpression, widget.ValidationMessage = extractWidgetValidation(ctx, w) widget.OnChange = extractOnChangeAction(ctx, w) return []rawWidget{widget} @@ -930,20 +930,6 @@ func extractWidgetValidation(ctx *ExecContext, w map[string]any) (expression, me // extractReadOnlyStyle extracts the ReadOnlyStyle from an input widget. // Returns "Inherit", "Control", or "Text". -// widgetBoolProperty reads a boolean property off a widget document. -// -// The key is a parameter rather than an inline index, which is not cosmetic: -// CodeQL's clear-text-logging query treats w["IsPasswordBox"] as a lookup of a -// credential and then follows the boolean into every error the page writer can -// return, failing the build on an unrelated example's fmt.Printf of that error -// (ako/mxcli#550). IsPasswordBox is a design-time flag — "render this text box -// as a password field" — and holds no secret, so the classification is wrong at -// the source rather than at the sink. -func widgetBoolProperty(w map[string]any, key string) bool { - v, _ := w[key].(bool) - return v -} - func extractReadOnlyStyle(ctx *ExecContext, w map[string]any) string { if style, ok := w["ReadOnlyStyle"].(string); ok { return style From fa289ccba0f7ddf9ee38cf05c13a9637fb52aa6c Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 20 Sep 2026 20:08:36 +0000 Subject: [PATCH 08/10] fix(pages): judge a list widget's own row action in the context it creates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MDL-PAGEARG01 refused datagrid dgRequests (DataSource: DATABASE M.ServiceRequest, onClick: SHOW_PAGE M.Edit(ServiceRequest: $currentObject)) with "widget `dgRequests` is not inside a data view, list view or grid row". A list widget's onClick fires PER ROW, so the row it renders IS the context object — and on a `listview` the refusal contradicted its own wording. Since exec refuses a script whose check reports an error, this was a blocker rather than a warning: the reporting project could not apply the slice at all. The #1029 guard judged every widget's own action in the context its PARENT supplies. That is right for a button and wrong for the widget that establishes the context. argContextForOwnAction draws the line where it belongs: a widget binding a source of its own supplies the context for its own action. A source in a shape the pass cannot read degrades to UNKNOWN, so the guard stands down rather than refusing what it cannot prove is discarded. Measured on mxbuild 11.14.0, in one fresh app, with the actions verified to be stored (describe page) so the zero is not a dropped action: datagrid + DATABASE source + onClick($currentObject) 0 errors listview + DATABASE source + onClick($currentObject) 0 errors Controls, all still refused: a foreign variable on a row action, #1029's page-level button, and a button standing beside the grid rather than in it. 1029-showpage-arg-without-context.fail.mdl still exits 1; the two valid #1029 and #295 bug-tests still pass. Before the fix the new unit test fails with the reported message verbatim. The same mxbuild run exposed a separate defect, filed as #576 and deliberately NOT written into the bug-test as a passing case: `DataSource: Mod.Car` on a datagrid is silently dropped, so that widget is CE0488 plus a real CE1571. Closes #552 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01L2aYb3zDscezm874CDTH6R --- .../fix-issue/findings/mdl-executor.jsonl | 1 + .../pages-552-list-widget-row-action.mdl | 53 ++++++++++ mdl/executor/cmd_pages_showpage_args.go | 53 +++++++++- mdl/executor/cmd_pages_showpage_args_test.go | 96 +++++++++++++++++++ mdl/executor/validate_widgets.go | 4 +- 5 files changed, 205 insertions(+), 2 deletions(-) create mode 100644 mdl-examples/bug-tests/pages-552-list-widget-row-action.mdl diff --git a/.claude/skills/fix-issue/findings/mdl-executor.jsonl b/.claude/skills/fix-issue/findings/mdl-executor.jsonl index 7d871a2f9c..0c67963f0b 100644 --- a/.claude/skills/fix-issue/findings/mdl-executor.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-executor.jsonl @@ -661,3 +661,4 @@ {"area":"mdl/linter","date":"2026-09-20","symptom":"mxcli happily authors `staticimage`/`dynamicimage` with nothing warning the author, and #518/#538 had just made both MORE capable (Image:, DataSource:, DefaultImage:, thumbnail, enlarge all newly reachable). The only signal that these widgets do not work was CE0582 at the far end of a build. Separately, three places in code comments and user-facing help claimed the deprecation was 'Mendix 11's React client'","cause":"Two things. (1) No validator or lint rule mentioned either widget (measured: `grep -rln deprecat mdl/executor/validate*.go` matched only an unrelated test), so the deprecation lived entirely in prose. (2) The version claim was inherited from a pre-existing comment and repeated without checking the doc: docs.mendix.com/refguide/image-viewer/ says the React client was added in **10.7**, so CE0582 fires on 10.7+ wherever that client is enabled, not only on 11","file":"`mdl/linter/rules/legacy_image_widget.go` (new, MPR012) + registration in `mdl/executor/cmd_lint.go` (x2) and `cmd/mxcli/cmd_lint.go`; wording in `mdl/backend/modelsdk/widget_write_legacy_gaps.go`, `.claude/skills/mendix/create-page/reference/widgets.md`, `cmd/mxcli/syntax/features_page.go`; table row in `.claude/commands/mendix/lint.md`","insight":"**A deprecation warning belongs in `lint`, not in `check`.** `check` validates a script, and `describe page` -> `exec` of a legacy page is a legitimate lossless operation — a check warning would fire on correct work every time, which is the noise trap MDL-WIDGET23's own history records. `lint` audits the project, where 'this page holds a widget your client cannot render' is wanted once. **The marketplace exclusion came free and is the control worth measuring**: `ctx.Widgets()` filters any module with a Source (notPlatformModule), so the rule never fires on the Studio Pro static images a blank app inherits from FeedbackModule — content the reader cannot fix and an update would replace. Measured: 8 legacy image widgets in the project, 7 indexed by the catalog, 5 in the user's own module, and lint reported exactly those 5. **A deny-list of two, never an allow-list**: the one widget such a rule must never fire on is the pluggable Image, i.e. the replacement it recommends. **'Deprecated in version X' needs the vendor doc, not the previous comment** — this repo had carried 'Mendix 11' for as long as the widgets had been written, and one fetch of the reference guide moved it to 10.7. A version boundary copied from a sibling comment is the same class of error as a floor copied from a proposal's sample output (mendixlabs/mxcli#1121)","refs":["mendixlabs/mxcli#1057"],"ce":["CE0582"],"rules":["MPR012"]} {"area": "mdl/backend/pagemutator", "date": "2026-09-20", "symptom": "`alter page … { set DataSource = DATABASE Mod.Entity on dvCust; }` passes `check`, prints `Altered page …` with exit 0, and leaves the DataView with no datasource: `describe page` renders `dataview dvCust {` with the property gone, and the only other signal is CE7007 at `mx check`", "cause": "`serializeDataSourceBson` mapped every `*pages.DatabaseSource` to a `Forms$DataViewSource` — the *context* source — with the entity in `EntityRef` and `SourceVariable` left null. A DATABASE source has no single stored shape: the widget holding it decides (`Forms$ListViewXPathSource` on a list view, `CustomWidgets$CustomWidgetXPathSource` on a pluggable widget, `Forms$GridXPathSource` on a grid), and a DATA VIEW has no database form at all — which is why CREATE PAGE's `dataViewSourceToGen` already refused that pairing while SET wrote it silently", "file": "`mdl/backend/pagemutator/mutator.go` (`SetWidgetDataSource`, new `databaseSourceRefusal`, `serializeDataSourceBson`)", "insight": "**The \"gone entirely\" in the report was DESCRIBE, not the document.** The DataSource was present and well-formed BSON; `parseContextSource` returns nil for a `Forms$DataViewSource` with no `SourceVariable`, so the reader rendered nothing. Chasing a deleted property would have been the wrong hunt — diff the stored BSON before believing a describe-shaped symptom. **The refusal belongs in the mutator, not the validator**: `validateAlterSetProperties` dry-runs the real setter against a `pagemutator.Probe()` copy, so one refusal makes `check -p --references` and `exec` agree by construction; a second copy of the rule in the validator is the duplicate-resolver drift CLAUDE.md warns about. **Refusing beat rebuilding the shapes here** — writing ListViewXPathSource in raw BSON would duplicate `listViewSourceToGen` in a second currency, and REPLACE already reaches the real builder. **Two remedies, not one**: on a data view `use REPLACE` is a dead end (CREATE PAGE refuses it too), so the message names the sources a data view can take; on a list view REPLACE genuinely works, so it names REPLACE. Getting that backwards sends the author in a circle. **Same generalisable shape as #855/#1101**: when SET and REPLACE express different vocabularies for one property, SET is a whitelist extended one bug report at a time. **`make check-mdl` runs `mxcli check` WITHOUT `-p`**, so a document-dependent refusal cannot be a `.fail.mdl` — it would be reported as a negative test that unexpectedly passed. Write the passing shape and comment the refused statements, as #1063 does. Measured on two copies of a real 11.13.0 app: faulty → `Check passed!`, exit 0, `mx check` 1 error CE7007 at Data view 'dvCust'; fixed → both refuse with exit 1, datasource unchanged, `mx check` 0 errors. Tests `mdl/backend/pagemutator/mutator_datasource_test.go`, `mdl/executor/validate_alter_set_test.go`; example `mdl-examples/bug-tests/1032-alter-page-set-database-datasource.mdl`. upstream #1032", "refs": ["#855", "#1032"], "ce": ["CE7007"]} {"area": "mdl/executor", "date": "2026-09-20", "symptom": "`describe page` → `exec` over a **Studio Pro-authored** page silently drops six things, `mx check` 0 errors throughout. The one that matters: `IsPasswordBox True → False` — a **password field round-trips into a plaintext text box**, and describe → rename → exec is mxcli's copy operation. Also `Validation.Expression` blanked, a DataView's `ReadOnlyStyle Text → Control`, `PopupCloseAction` wiped, and two typed-array markers", "cause": "Four different causes behind one symptom, which is why triage came first: (1) IsPasswordBox — model and writer carried it, nothing parsed it, nothing emitted it; (2) Validation — `widgetValidationToGen()` wrote a DEFAULT EMPTY Forms$WidgetValidation over whatever was stored, on five widget types; (3) ReadOnlyStyle — wired for CheckBox only, and a DataView's draws no MDL-WIDGET07 warning because `staticWidgetKnownProps` is deliberately a union across widget types; (4) PopupCloseAction — `pageToGen` wrote \"\" unconditionally. Plus ParameterMappings/OutputMappings markers", "file": "`mdl/executor/cmd_pages_describe_parse.go` + `_output.go` (extract/emit), `cmd_pages_builder_v3_widgets.go` (consume), `cmd_pages_builder_v3.go`, `mdl/visitor/visitor_page_v3.go`, `mdl/ast/ast_page_v3.go`, `sdk/pages/*`, `mdl/backend/modelsdk/widget_write.go` + `page_write.go`, `mdl/executor/validate_widgets.go` (describe vocabulary)", "insight": "**Triage the layer before writing anything** — describer / grammar / builder have different fixes and this one issue had all three. The quickest probe is to run the property through `mxcli check`: MDL-WIDGET07 names an unrecognised one, and *silence is not acceptance* — the known-props list is a union across widget types, so a DataView's ReadOnlyStyle passed check and was dropped anyway. **Emit an expression QUOTED, not bracketed**: `[...]` is the XPath-constraint spelling and `propertyValueV3` parses it as an ARRAY, so `GetStringProp` yields \"\" — the emitter's own unit test was green while the real round trip still lost the value (storage form is not input form). **Measure the default before keeping it**: a DataView's ReadOnlyStyle is Control on 47 of 56, never Inherit, so the 'obvious' Inherit that every other input widget uses would have been wrong. Markers likewise measured, not assumed: ParameterMappings is marker 2 on 220 of 220 lists in every parent type, OutputMappings present on 91 of 91 — and an EMPTY list needs `MandatoryListMarkers` since `RegisterListMarker` keys on a child that is not there. Result 17 → 9 differences, the 9 being ako/mxcli#549", "refs": ["#550", "#541", "#549", "#490"]} +{"area": "mdl/executor", "date": "2026-09-20", "symptom": "MDL-PAGEARG01 refused a list widget's OWN row action: `datagrid dg (DataSource: DATABASE M.E, onClick: SHOW_PAGE M.Edit(E: $currentObject))` was rejected at `check` with \"widget `dg` is not inside a data view, list view or grid row\" \u2014 and since exec refuses a script whose check errors, the slice could not be applied at all. On a `listview` the message contradicted itself. mxbuild 11.14.0 accepts the stored pages at 0 errors.", "cause": "The #1029 guard judged EVERY widget's own action in the context its PARENT supplies: `argContextForSubtreeOf` returns the parent context for a childless widget and `validate_widgets.go` passed the inherited `argCtx` to `validateShowPageArguments`. Right for a button, wrong for the widget that ESTABLISHES the context \u2014 a list widget's onClick is row-scoped, so the row it renders is the context object. Added `argContextForOwnAction`: a widget that binds a source of its own supplies the context for its own action; a source in a shape the pass cannot read (the bare-entity shorthand) degrades to UNKNOWN so the guard stands down rather than refusing what it cannot prove is discarded.", "file": "`mdl/executor/cmd_pages_showpage_args.go` (argContextForOwnAction, argContextForSubtreeOf), `mdl/executor/validate_widgets.go`", "insight": "**A false refusal costs more than a missing rule now that exec refuses on a check error** \u2014 the blast radius is 'this project cannot be built with this mxcli', not 'a warning is noisy'. Two things would have caught it before release: judging the rule against the widget kinds it NAMES in its own message (the listview refusal reads 'lvA is not inside a \u2026 list view'), and running it against mxbuild rather than against intuition. The mxbuild run paid for itself twice: it also showed that `DataSource: M.E` (bare-entity shorthand) on a datagrid is silently dropped, so that case is CE0488 + a REAL CE1571 \u2014 the stand-down is still correct, but the shorthand case must not be written into a bug test as mxbuild-clean (#576). Control the fix with the widget kinds STILL refused (a foreign variable, a sibling button beside the grid), or it is indistinguishable from deleting the rule.", "refs": ["#552", "#576", "mendixlabs/mxcli#1029", "#939"]} diff --git a/mdl-examples/bug-tests/pages-552-list-widget-row-action.mdl b/mdl-examples/bug-tests/pages-552-list-widget-row-action.mdl new file mode 100644 index 0000000000..216b1b69fc --- /dev/null +++ b/mdl-examples/bug-tests/pages-552-list-widget-row-action.mdl @@ -0,0 +1,53 @@ +-- ako/mxcli#552 — MDL-PAGEARG01 refused a list widget's OWN row action. +-- +-- `onClick` on a data grid, list view or gallery fires per row, so the row the +-- widget renders IS the context object. The #1029 guard judged every widget's +-- own action in the context its PARENT supplies, so each statement below was +-- refused at `check` — and, since exec refuses a script whose check errors, +-- could not be applied at all. mxbuild accepts the stored pages at 0 errors. +-- +-- On a list view the refusal contradicted itself: "widget `lvCars` is not +-- inside a data view, list view or grid row". +-- +-- This file is a CHECK-time regression: `mxcli check` must report 0 errors. +-- The control lives beside it — 1029-showpage-arg-without-context.fail.mdl is +-- the contextless page-level button, which must STILL be refused. + +create module Mod; + +create entity Mod.Car ( + Name: String(100) +); + +create page Mod.Detail (Title: 'Car', Layout: Atlas_Core.Atlas_Default, Params: { $Car: Mod.Car }) { + dataview dvCar (DataSource: $Car) { + textbox tbName (Label: 'Name', Attribute: Name) + } +}; + +-- A data grid's own onClick: the clicked row is the context object. +create page Mod.Grid (Title: 'Cars', Layout: Atlas_Core.Atlas_Default) { + datagrid dgCars ( + DataSource: DATABASE Mod.Car, + onClick: SHOW_PAGE Mod.Detail(Car: $currentObject) + ) { + column colName (Caption: 'Name', Attribute: Name) + } +}; + +-- A list view's own onClick, same rule. +create page Mod.List (Title: 'Cars', Layout: Atlas_Core.Atlas_Default) { + listview lvCars ( + DataSource: DATABASE Mod.Car, + onClick: SHOW_PAGE Mod.Detail(Car: $currentObject) + ) { + dynamictext dtName (Content: '{1}', ContentParams: [{1} = Name]) + } +}; + +-- NOT a case for this file: the bare-entity shorthand `DataSource: Mod.Car` on a +-- data grid is SILENTLY DROPPED (the stored widget has no data source at all), +-- so mxbuild reports CE0488 and then a real CE1571. The guard still stands down +-- on it — it refuses only what it can prove is discarded, and a source it cannot +-- read is unknown, not absent — but that case belongs to the shorthand bug, not +-- here. Measured on mxbuild 11.14.0; see ako/mxcli#576. diff --git a/mdl/executor/cmd_pages_showpage_args.go b/mdl/executor/cmd_pages_showpage_args.go index 49ae5d5714..6fb3d08ef3 100644 --- a/mdl/executor/cmd_pages_showpage_args.go +++ b/mdl/executor/cmd_pages_showpage_args.go @@ -108,12 +108,63 @@ func argContextForChildren(w *ast.WidgetV3, parent pageArgContext) pageArgContex return parent } +// argContextForOwnAction is the context a widget's OWN action is judged in. +// +// A button's action runs in the context its parent supplies. A list widget's does +// not: `onClick` on a data grid, list view or gallery fires PER ROW, and the row +// it renders is the context object — so a widget that binds a source of its own +// supplies the context for its own action. Judging it in the parent's context +// refused `datagrid dg (DataSource: DATABASE M.E, onClick: SHOW_PAGE M.Edit(E: +// $currentObject))` against an mxbuild that accepts it at 0 errors, and on a list +// view the refusal contradicted its own wording (ako/mxcli#552). +// +// `Action:` and `onClick:` are aliases that both land on Properties["Action"], so +// this covers every spelling of a widget's own action. +func argContextForOwnAction(w *ast.WidgetV3, parent pageArgContext) pageArgContext { + if w == nil { + return parent + } + if ds := w.GetDataSource(); ds != nil { + // The entity is only used to word a refusal; the executor's builder + // overwrites this with one that carries it. + return enteringDataWidget(ds, "") + } + if bindsDataInAnUnreadableShape(w) { + // The widget plainly binds data, so a context object EXISTS — but this + // pass cannot say what it is called. Unknown, not absent, and the guard + // stands down exactly as it does for ALTER PAGE. The bare-entity + // shorthand `datagrid dg (DataSource: M.E)` is this case. + return pageArgContext{} + } + return parent +} + +// bindsDataInAnUnreadableShape reports whether w names a data source this pass +// cannot parse into a *ast.DataSourceV3 — the bare-entity shorthand, or a +// pluggable widget naming its source under its own key. +func bindsDataInAnUnreadableShape(w *ast.WidgetV3) bool { + for name, v := range w.Properties { + if !strings.EqualFold(name, "DataSource") { + continue + } + if _, parsed := v.(*ast.DataSourceV3); !parsed && v != nil { + return true + } + } + return false +} + // argContextForSubtreeOf is argContextForChildren for the executor's builder, // which builds a widget AND its children in one call. A widget with no children // carries nothing but its own action, and that action is judged in the context // its parent supplies — degrading there would stand the guard down on exactly the -// page-level button #1029 is about. +// page-level button #1029 is about. A data-bound widget is the exception, and the +// reason is argContextForOwnAction's: its own action is row-scoped, so it needs +// the context it creates whether or not it has children to give it to. func argContextForSubtreeOf(w *ast.WidgetV3, parent pageArgContext) pageArgContext { + if own := argContextForOwnAction(w, parent); own != parent { + return own + } if len(w.Children) == 0 { return parent } diff --git a/mdl/executor/cmd_pages_showpage_args_test.go b/mdl/executor/cmd_pages_showpage_args_test.go index 091d422d17..7ee5e0d281 100644 --- a/mdl/executor/cmd_pages_showpage_args_test.go +++ b/mdl/executor/cmd_pages_showpage_args_test.go @@ -219,3 +219,99 @@ func TestValidateShowPageArguments_AlterPageStandsDown(t *testing.T) { } } } + +// ako/mxcli#552: a list widget's OWN action is row-scoped, so the row it renders +// IS the context object. The #1029 guard judged every widget's own action in the +// context its PARENT supplies, which is right for a button and wrong for the +// widget that establishes the context — it refused +// +// datagrid dgRequests (DataSource: DATABASE M.ServiceRequest, +// onClick: SHOW_PAGE M.Edit(ServiceRequest: $currentObject)) +// +// with "widget `dgRequests` is not inside a data view, list view or grid row", +// while mxbuild accepts the stored page at 0 errors. On a `listview` the message +// contradicted itself outright. +// +// `onClick:` and `Action:` are aliases that both land on Properties["Action"] +// (see validate_widget_action_slot.go), so one shape covers both spellings. +func TestValidateShowPageArguments_ListWidgetOwnRowAction(t *testing.T) { + registry := LoadWidgetRegistry("") + if registry == nil { + t.Fatal("LoadWidgetRegistry returned nil") + } + + rowAction := func(kind string, source any, arg string) *ast.WidgetV3 { + return &ast.WidgetV3{ + Type: kind, + Name: "w1", + Properties: map[string]any{ + "DataSource": source, + "Action": &ast.ActionV3{ + Type: "showPage", + Target: "Mod.Detail", + Args: []ast.FlowArgV3{{Name: "Car", Value: arg}}, + }, + }, + } + } + database := &ast.DataSourceV3{Type: "database", Reference: "Mod.Car"} + + hits := func(w *ast.WidgetV3) []string { + var msgs []string + for _, v := range validateWidgetTree([]*ast.WidgetV3{w}, registry, "page Mod.P") { + if v.RuleID == "MDL-PAGEARG01" { + msgs = append(msgs, v.Message) + } + } + return msgs + } + + // The row object, under either spelling, on each widget kind that renders rows. + for _, kind := range []string{"datagrid", "listview", "gallery"} { + t.Run(kind+"/$currentObject", func(t *testing.T) { + if got := hits(rowAction(kind, database, "$currentObject")); len(got) != 0 { + t.Errorf("a %s's own row action was refused — mxbuild accepts it at 0 errors: %s", kind, got[0]) + } + }) + } + + // The bare-entity shorthand leaves a plain string rather than a parsed source, + // so the entity is unreadable — but the widget plainly binds data, and the + // guard's own doctrine is that it refuses only what it can PROVE is discarded. + t.Run("bare-entity shorthand stands the guard down", func(t *testing.T) { + if got := hits(rowAction("datagrid", "Mod.Car", "$currentObject")); len(got) != 0 { + t.Errorf("shorthand source was refused: %s", got[0]) + } + }) + + // Controls. Without these the fix is indistinguishable from deleting the rule. + t.Run("control: a foreign variable on a row action is still refused", func(t *testing.T) { + if got := hits(rowAction("datagrid", database, "$Other")); len(got) != 1 { + t.Errorf("MDL-PAGEARG01 violations = %d, want 1 — a database row source names no $Other", len(got)) + } + }) + t.Run("control: #1029's page-level button is still refused", func(t *testing.T) { + var n int + for _, v := range validateWidgetTree([]*ast.WidgetV3{showPageButton("$SomeRef")}, registry, "page Mod.P") { + if v.RuleID == "MDL-PAGEARG01" { + n++ + } + } + if n != 1 { + t.Errorf("MDL-PAGEARG01 violations on a contextless button = %d, want 1", n) + } + }) + t.Run("control: a button beside the grid, not in it, is still refused", func(t *testing.T) { + grid := rowAction("datagrid", database, "$currentObject") + tree := []*ast.WidgetV3{{Type: "container", Name: "c1", Children: []*ast.WidgetV3{grid, showPageButton("$currentObject")}}} + var n int + for _, v := range validateWidgetTree(tree, registry, "page Mod.P") { + if v.RuleID == "MDL-PAGEARG01" { + n++ + } + } + if n != 1 { + t.Errorf("MDL-PAGEARG01 violations = %d, want 1 — the grid's own action is fine, the sibling button is not", n) + } + }) +} diff --git a/mdl/executor/validate_widgets.go b/mdl/executor/validate_widgets.go index ef8417ae8c..59dbde8d50 100644 --- a/mdl/executor/validate_widgets.go +++ b/mdl/executor/validate_widgets.go @@ -186,7 +186,9 @@ func validateWidgetTreeIn(widgets []*ast.WidgetV3, registry *WidgetRegistry, loc out = append(out, validateDatasourceXPathAssociationEmpty(w, locationPrefix)...) out = append(out, validateComboBoxAssociation(w, locationPrefix)...) // A show_page argument naming anything but the context object is dropped. - out = append(out, validateShowPageArguments(w, argCtx, locationPrefix)...) + // The widget's OWN action is judged in the context IT establishes, not the + // one it sits in — a list widget's onClick is row-scoped (ako/mxcli#552). + out = append(out, validateShowPageArguments(w, argContextForOwnAction(w, argCtx), locationPrefix)...) // Unknown-property warning applies only to built-in widgets; pluggable // widgets get the stricter def.json check (MDL-WIDGET01) above, and // object-list items are validated by the object-list engine. From 2f15c670a82e9fafbbfd334d06b7501867877d44 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 20 Sep 2026 20:53:42 +0000 Subject: [PATCH 09/10] fix(domain model): drop cross-module associations with the entity they end at MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DROP ENTITY swept only the regular Associations collection, so every CROSS-MODULE association pointing at the deleted entity survived. Dropping the local BY-ID (FROM) end left a 16-byte pointer to an element that no longer exists, and mxbuild 11.14.0 could then not LOAD the project: ERROR: System.AggregateException … (The given key '49751a65-d5f9-456c-887e-3f14bacb8822' was not present in the dictionary.) at StreamingBsonUnitReader.ResolvePostponedProperties() No CE code and no document named — the failure is in the storage layer, above the consistency checker, so it reads as "the project is corrupt". Dropping the BY-NAME (TO) end is milder and still wrong: CE1613 at the cross-module association. removeCrossAssocsReferencing matches both ends, because a cross-module association addresses them differently: FROM by element id (local to this domain model), TO by qualified name (it lives in another module). Called from DeleteEntity locally and in its cascade over the other domain models. Reported against a VIEW entity, whose associations are derived from its OQL so there is no CREATE ASSOCIATION to undo. Nothing here is view-entity specific: the first probe — view entity and source entity in the SAME module — did not reproduce, and that negative is what identified cross-module as the variable. A plain `create association A.X from A.X to B.Y` plus `drop entity A.X` reproduces the identical crash. Measured on mxbuild 11.14.0, one project carrying all three shapes: before drop by-id end project does not load (exception above) drop by-name end CE1613 at the cross-module association after all three drops project loads, 0 errors Controls: an untouched cross-module association survives both deletes (in the unit test and in the bug-test), and the single-module case still works. Before the fix the new test fails in both directions with the orphan left behind. Closes #553 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01L2aYb3zDscezm874CDTH6R --- .../fix-issue/findings/mdl-backend.jsonl | 1 + ...odel-553-drop-entity-cross-association.mdl | 67 +++++++++ mdl/backend/modelsdk/domainmodel_alter.go | 57 +++++++- .../domainmodel_delete_cross_assoc_test.go | 131 ++++++++++++++++++ 4 files changed, 255 insertions(+), 1 deletion(-) create mode 100644 mdl-examples/bug-tests/domainmodel-553-drop-entity-cross-association.mdl create mode 100644 mdl/backend/modelsdk/domainmodel_delete_cross_assoc_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-backend.jsonl b/.claude/skills/fix-issue/findings/mdl-backend.jsonl index aef2416a46..887ba4b3ee 100644 --- a/.claude/skills/fix-issue/findings/mdl-backend.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-backend.jsonl @@ -119,3 +119,4 @@ {"area": "mdl/backend", "date": "2026-09-20", "symptom": "Every page mxcli wrote carried `Autofocus` (a property Mendix introduced in 11.1.0) and every page and snippet carried `Variables` (10.17.0), whatever the project's version. On a Mendix 10 project the document therefore held a key that project's metamodel does not declare — the class of defect that makes Studio Pro throw InvalidOperationException at MprProperty.cs while mxbuild reports 0 errors. Found while fixing mendixlabs/mxcli#1121 (the same defect in the page's PARAMETERS) and deferred there as out of scope.", "cause": "`pageToGen` called `SetAutofocus(\"DesktopOnly\")` unconditionally, and both `Forms$Page` and `Forms$Snippet` registered `Variables` in `codec.RegisterTypeDefaults(...).MandatoryLists`, which the encoder emits as an empty typed-array marker for every new element. The page writer had no version awareness at all until #1121 threaded `*types.ProjectVersion` into `pageToGen` for the parameter keys.", "file": "`mdl/backend/modelsdk/page_write.go` (pageSupportsAutofocus/pageSupportsVariables, encodePage, docEncoder), `mdl/backend/modelsdk/snippet_write.go` (encodeSnippet), `modelsdk/codec/encoder.go` (Encoder.OmitKeys). Tests `mdl/backend/modelsdk/page_version_keys_test.go`, `modelsdk/codec/omitkeys_test.go`.", "insight": "**A version-floored key can arrive by two different routes, and only one of them is a gen property.** `Autofocus` is set through gen, so the fix is to not set it; `Variables` exists only because the Studio Pro defaults registry adds it, and a gen `PartList` has no 'present but empty' state — so there is nothing to leave unset and the suppression has to happen in the encoder. That registry is global and keyed by `$Type` alone, so it cannot see a project version: hence `Encoder.OmitKeys`, per-encode. **Suppressing a key and dropping data are different things**: the empty `Variables` marker is safe to suppress, but variables the script DECLARED are refused instead (guard-don't-drop, ADR-0005) — silently dropping them leaves widgets referencing names that are gone (CE1151) from a statement that reported success. Two measurement traps hit on the way: the first control run died on a pre-existing entity before reaching the page statement, so the page was never rewritten and the unchanged output read as a pass — a control that does not run looks exactly like a control that passes; and ALTER PAGE turned out to be safe without any change, because the page mutator edits the stored raw BSON and marshals it back, so it can never invent a key. Check which write paths actually rebuild a document before assuming a header fix has to cover all of them.", "refs": ["#1121"]} {"area": "mdl/backend", "date": "2026-09-18", "symptom": "`ALTER PAGE M.P { SET Documentation = '...' }` is refused: \"unsupported page-level property: Documentation\". Documenting an existing page therefore means re-running its CREATE, because the `/** … */` doc comment on the create statement is the only other source", "cause": "`applyPageLevelSetMut` handled Title/Url/Popup*/Class/Style and had no Documentation case. The field was otherwise fully understood — `pageToGen` has always written it on CREATE via `out.SetDocumentation`, and gen binds it as `property.NewPrimitive[string](\"Documentation\")`. The grammar already parsed it (`identifierOrKeyword EQUALS propertyValueV3`) and the executor has no allowlist, so it was one missing case in the mutator, not a syntax gap", "file": "`mdl/backend/pagemutator/mutator.go` (`applyPageLevelSetMut`)", "insight": "One case covers **Page, Layout AND Snippet** — all three declare Documentation and all three reach this function through `SetWidgetProperty(\"\")`. Store an empty string rather than rejecting it: removing a doc comment from a script has to be expressible, and the property is a bare string with no unset value. **Update the unsupported-property message in the same change** — that list is the only guidance a reader gets, and a stale one sends them back to the CREATE workaround the fix exists to remove (a test asserts the message names Documentation). Note the re-run workaround is worse than it sounds: describe → exec is only as complete as what MDL can spell, so restating a page to document it can silently lose widgets", "refs": ["#527"]} {"area": "mdl/backend", "date": "2026-09-20", "symptom": "`describe page` → `exec` over a **Studio Pro-authored** page reports `Replaced page`, not `Unchanged` — the rebuild is not semantically equal to what was stored, so ADR-0008's elision cannot fire and the unit churns in version control on every re-run. `mx check` is 0 errors either way. Measured on ako/TestApp Rules.RuleAction_NewEdit at 11.14.0: fourteen differences", "cause": "Four independent classes, all 'the rebuild writes a constant where Studio Pro stores a value': (1) `pageToGen` hardcoded Autofocus/CanvasWidth/CanvasHeight; (2) save_changes/cancel_changes/close_page/delete_object never wrote `DisabledDuringExecution`, and save_changes wrote `SyncAutomatically` true; (3) `AttributeRef.EntityRef` emitted only on the navigated branch; (4) `Forms$PageVariable` had only the one name field set, so the other five keys were never marked dirty", "file": "`mdl/backend/modelsdk/page_write.go` (`carryStoredPageHeader`, `bsonInt`), `widget_write.go` (four action cases + two `RegisterTypeDefaults`), `modelsdk/codec/defaults.go` + `encoder.go` (new `FalseFields`)", "insight": "**mxcli round-tripping its own output proves NOTHING about this class** — measured: the MDL bug-test reports `Unchanged` on the unfixed build too, because mxcli writes the page and mxcli describes it, so its constants agree with themselves. The reference must be a Studio Pro document; a committed CI fixture only works if it is one. **A population selected by name can confirm whatever it excluded**: the first sweep filtered `$Type` on `endswith(\"ClientAction\")`, got a tidy 'True on 82 of 82', and so missed `Forms$NoAction` (False on 83 of ~7,300) and `Forms$MicroflowAction` (5 of 81) — scan by the PROPERTY, not by a name pattern. **Hardcoded looked safe and was not**: CanvasWidth takes seven distinct values across 67 pages and the hardcoded 1200 matched 4, so a round trip moved the canvas of 63. **The int width bit this fix once**: Studio Pro stores both canvas dimensions as int64 while the gen setter takes int32, so the natural `.(int32)` assertion matched nothing — and the first unit test passed anyway because its own fixture wrote int32, i.e. the test encoded the assumption under test (bson-numeric-width). Prefer `TypeDefaults` over patching each construction site: `Forms$PageVariable` is built in three places. Remaining after the fix: 1 of 14, a pluggable-widget Object property — CE0463 territory, deliberately out of scope", "refs": ["#541", "#529"]} +{"area": "mdl/backend", "date": "2026-09-20", "symptom": "`DROP ENTITY` left every CROSS-MODULE association pointing at the deleted entity in place. Dropping the local BY-ID (FROM) end made mxbuild 11.14.0 unable to LOAD the project: `System.AggregateException \u2026 (The given key '' was not present in the dictionary.)` at `StreamingBsonUnitReader.ResolvePostponedProperties()` \u2014 no CE code, no document named, so the obvious reading is 'the project is corrupt, restore from git'. Dropping the BY-NAME (TO) end is milder and still wrong: CE1613 at the cross-module association. `show associations` shows a raw GUID where the parent entity should be.", "cause": "`removeAssocsReferencing` swept `dm.AssociationsItems()` and asserted `*genDm.Association` per item, so the SEPARATE `CrossAssociations` collection was never looked at. Fixed with `removeCrossAssocsReferencing`, matching BOTH ends because a cross-module association addresses them differently \u2014 FROM by element id (local), TO by qualified name (another module) \u2014 called in DeleteEntity locally and in its cascade over the other domain models.", "file": "`mdl/backend/modelsdk/domainmodel_alter.go` (removeCrossAssocsReferencing, DeleteEntity)", "insight": "**Reported against a view entity; nothing about it was view-entity specific.** The reporter met it dropping view entities (whose associations are DERIVED from OQL, so there is no CREATE ASSOCIATION to undo) and filed it that way. The first probe \u2014 a view entity and its source entity in the SAME module \u2014 did not reproduce at all, and that negative is the useful one: it says the variable is cross-module, not view-ness. A plain `create association A.X from A.X to B.Y` plus `drop entity A.X` reproduces the identical crash. Two lessons: when a repro fails, vary the dimension the report did not mention before doubting the report, and treat a collection-typed `.(*T)` assertion in a cascade as a place where a sibling type hides. mxbuild's diagnostic distinguishes the two ends for free \u2014 a dangling 16-byte pointer is a LOAD crash, a dangling qualified name is CE1613 \u2014 so testing only one end proves half the fix.", "refs": ["#553", "#556"]} diff --git a/mdl-examples/bug-tests/domainmodel-553-drop-entity-cross-association.mdl b/mdl-examples/bug-tests/domainmodel-553-drop-entity-cross-association.mdl new file mode 100644 index 0000000000..db0d7812dc --- /dev/null +++ b/mdl-examples/bug-tests/domainmodel-553-drop-entity-cross-association.mdl @@ -0,0 +1,67 @@ +-- ako/mxcli#553 — DROP ENTITY left CROSS-MODULE associations pointing at the +-- deleted entity, and mxbuild could then not LOAD the project: +-- +-- ERROR: System.AggregateException … (The given key +-- '49751a65-d5f9-456c-887e-3f14bacb8822' was not present in the dictionary.) +-- at StreamingBsonUnitReader.ResolvePostponedProperties() +-- +-- No CE code and no document named, because the failure is in the storage layer +-- rather than the consistency checker — the obvious reading is "the project is +-- corrupt, restore from git". The recovery was DROP ASSOCIATION on each orphan. +-- +-- Reported against a VIEW entity, whose associations are derived from its OQL so +-- there is no CREATE ASSOCIATION to undo. Measured here: nothing about it is +-- view-entity specific. A plain cross-module association orphans identically; +-- the single-module case was always handled, which is why it went unnoticed. +-- +-- Both ends are covered because they fail differently: a dangling BY-ID end (the +-- FROM entity, stored in this module) is the crash above, while a dangling +-- BY-NAME end (the TO entity, in another module) is CE1613 at the cross-module +-- association. +-- +-- Run with `mxcli exec`, then `mxcli docker check --no-update-widgets`: the +-- project must LOAD and report 0 errors. On the unfixed build `show associations` +-- shows a raw GUID where the parent entity should be. + +create module Dm553A; +create module Dm553B; + +create persistent entity Dm553A.Meter ( + MeterName: String(100) +); + +create persistent entity Dm553A.Reading ( + Kwh: Decimal +); + +create association Dm553A.Reading_Meter from Dm553A.Reading to Dm553A.Meter type reference; + +-- 1. A view entity in another module, whose association to Dm553A.Meter is +-- DERIVED from the OQL (`select m.ID as MeterRef`). +create view entity Dm553B.MeterTotals ( + TotalKwh: Decimal +) as ( + from Dm553A.Reading as r + join r/Dm553A.Reading_Meter/Dm553A.Meter as m + group by m.ID + select m.ID as MeterRef, sum(r.Kwh) as TotalKwh +); + +-- 2. A plain cross-module association, dropped from its BY-ID (FROM) end. +create persistent entity Dm553A.Parent1 (Label: String(100)); +create persistent entity Dm553B.Child1 (Label: String(100)); +create association Dm553A.Parent1_Child1 from Dm553A.Parent1 to Dm553B.Child1 type reference; + +-- 3. The same shape, dropped from its BY-NAME (TO) end. +create persistent entity Dm553A.Parent2 (Label: String(100)); +create persistent entity Dm553B.Child2 (Label: String(100)); +create association Dm553A.Parent2_Child2 from Dm553A.Parent2 to Dm553B.Child2 type reference; + +-- The control: a cross-module association neither drop touches. It must survive. +create persistent entity Dm553A.Keeper (Label: String(100)); +create persistent entity Dm553B.Kept (Label: String(100)); +create association Dm553A.Keeper_Kept from Dm553A.Keeper to Dm553B.Kept type reference; + +drop entity Dm553B.MeterTotals; +drop entity Dm553A.Parent1; +drop entity Dm553B.Child2; diff --git a/mdl/backend/modelsdk/domainmodel_alter.go b/mdl/backend/modelsdk/domainmodel_alter.go index d50c75e06d..ccd4851f06 100644 --- a/mdl/backend/modelsdk/domainmodel_alter.go +++ b/mdl/backend/modelsdk/domainmodel_alter.go @@ -4,6 +4,7 @@ package modelsdkbackend import ( "fmt" + "strings" "github.com/mendixlabs/mxcli/model" "github.com/mendixlabs/mxcli/modelsdk/codec" @@ -61,6 +62,43 @@ func removeAssocsReferencing(dm *genDm.DomainModel, entityID model.ID) bool { return changed } +// removeCrossAssocsReferencing drops every CROSS-MODULE association in dm that +// ends at the deleted entity. Returns whether anything was removed. +// +// A cross-module association is stored in the FROM entity's module, in a separate +// collection from the regular ones, and its two ends are addressed differently: +// the FROM end by element id (local to this domain model) and the TO end by +// qualified name (it lives in another module). So both have to be matched, and +// they fail differently when they are not (ako/mxcli#553): +// +// - a dangling BY-ID end is a 16-byte pointer to an element that is gone, and +// mxbuild cannot LOAD the project — KeyNotFoundException at +// StreamingBsonUnitReader.ResolvePostponedProperties(), with no CE code and +// no document named. +// - a dangling BY-NAME end is an ordinary model error, CE1613 at the +// cross-module association. +// +// qualifiedName may be empty when the module name could not be established; the +// by-name sweep is then skipped rather than guessed at, since an empty name would +// match nothing at best and everything at worst. +func removeCrossAssocsReferencing(dm *genDm.DomainModel, entityID model.ID, qualifiedName string) bool { + changed := false + items := dm.CrossAssociationsItems() + for i := len(items) - 1; i >= 0; i-- { + ca, ok := items[i].(*genDm.CrossAssociation) + if !ok { + continue + } + byID := string(ca.ParentRefID()) == string(entityID) + byName := qualifiedName != "" && strings.EqualFold(ca.ChildQualifiedName(), qualifiedName) + if byID || byName { + dm.RemoveCrossAssociations(i) + changed = true + } + } + return changed +} + // DeleteAttribute removes an attribute from an entity. The remaining attributes // pass through the codec unchanged; only the Attributes list is rebuilt. Mirrors // legacy semantics (no cascade — dangling index/validation refs are left as-is, @@ -314,8 +352,18 @@ func (b *Backend) DeleteEntity(domainModelID, entityID model.ID) error { if eidx < 0 { return fmt.Errorf("entity not found: %s", entityID) } + // The qualified name the OTHER modules' cross-module associations know this + // entity by. Read before the entity is removed, for obvious reasons. + qualifiedName := "" + if ge := findGenEntity(dm, entityID); ge != nil { + if moduleName := b.moduleNameFor(domainModelID); moduleName != "" { + qualifiedName = moduleName + "." + ge.Name() + } + } + dm.RemoveEntities(eidx) removeAssocsReferencing(dm, entityID) + removeCrossAssocsReferencing(dm, entityID, qualifiedName) if err := b.persistDM(domainModelID, dm); err != nil { return err } @@ -338,7 +386,14 @@ func (b *Backend) DeleteEntity(domainModelID, entityID model.ID) error { if err != nil { return fmt.Errorf("DeleteEntity: cascade cleanup: load %s: %w", other.ID, err) } - if removeAssocsReferencing(odm, entityID) { + // Two sweeps, not one: a regular association here can only reference the + // entity by id, while a cross-module one in ANOTHER module reaches it by + // qualified name — which is the half that leaves CE1613 behind. + removed := removeAssocsReferencing(odm, entityID) + if removeCrossAssocsReferencing(odm, entityID, qualifiedName) { + removed = true + } + if removed { if err := b.persistDM(other.ID, odm); err != nil { return fmt.Errorf("DeleteEntity: cascade cleanup: update %s: %w", other.ID, err) } diff --git a/mdl/backend/modelsdk/domainmodel_delete_cross_assoc_test.go b/mdl/backend/modelsdk/domainmodel_delete_cross_assoc_test.go new file mode 100644 index 0000000000..578963d79e --- /dev/null +++ b/mdl/backend/modelsdk/domainmodel_delete_cross_assoc_test.go @@ -0,0 +1,131 @@ +// SPDX-License-Identifier: Apache-2.0 + +// ako/mxcli#553 — DROP ENTITY left every CROSS-MODULE association pointing at the +// deleted entity in place. +// +// The cascade in DeleteEntity swept `dm.AssociationsItems()` and asserted +// `*genDm.Association` on each item, so the separate CrossAssociations collection +// was never looked at. Dropping the local (by-id) end left a 16-byte pointer to an +// element that no longer exists, and mxbuild 11.14.0 then could not LOAD the +// project at all: +// +// ERROR: System.AggregateException … (The given key +// '49751a65-d5f9-456c-887e-3f14bacb8822' was not present in the dictionary.) +// at StreamingBsonUnitReader.ResolvePostponedProperties() +// +// No CE code, no document named — the failure is in the storage layer, upstream of +// the consistency checker, so the obvious reading is "the project is corrupt". +// Dropping the by-name end is milder and still wrong: CE1613 at the cross-module +// association. +// +// It was reported against a view entity (whose associations are DERIVED from its +// OQL, so there is no CREATE ASSOCIATION to undo), but nothing here is specific to +// view entities: a plain cross-module association orphans identically. The +// single-module case was always handled, which is why this went unnoticed. +package modelsdkbackend + +import ( + "testing" + + "github.com/mendixlabs/mxcli/sdk/domainmodel" +) + +func TestDeleteEntity_RemovesCrossModuleAssociations(t *testing.T) { + // Both ends of a cross-module association, each deleted in its own project so + // the two directions cannot mask each other. + for _, tc := range []struct { + name string + // drop reports which entity to delete: the local by-id FROM end, or the + // by-name TO end in the other module. + dropChild bool + }{ + {"the by-id FROM end (mxbuild cannot load the project)", false}, + {"the by-name TO end (CE1613 at the cross-module association)", true}, + } { + t.Run(tc.name, func(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() }) + + fromMod, err := b.GetModuleByName("MyFirstModule") + if err != nil || fromMod == nil { + t.Fatalf("GetModuleByName(MyFirstModule): %v", err) + } + toMod, err := b.GetModuleByName("Administration") + if err != nil || toMod == nil { + t.Fatalf("GetModuleByName(Administration): %v", err) + } + fromDM, err := b.GetDomainModel(fromMod.ID) + if err != nil { + t.Fatalf("GetDomainModel(from): %v", err) + } + toDM, err := b.GetDomainModel(toMod.ID) + if err != nil { + t.Fatalf("GetDomainModel(to): %v", err) + } + + from := &domainmodel.Entity{Name: "ZzOrder", Persistable: true} + if err := b.CreateEntity(fromDM.ID, from); err != nil { + t.Fatalf("CreateEntity(from): %v", err) + } + to := &domainmodel.Entity{Name: "ZzCustomer", Persistable: true} + if err := b.CreateEntity(toDM.ID, to); err != nil { + t.Fatalf("CreateEntity(to): %v", err) + } + // A second, untouched cross association is the control: the sweep must + // remove what the deleted entity holds up and nothing else. + keep := &domainmodel.Entity{Name: "ZzKeeper", Persistable: true} + if err := b.CreateEntity(fromDM.ID, keep); err != nil { + t.Fatalf("CreateEntity(keep): %v", err) + } + + for _, ca := range []*domainmodel.CrossModuleAssociation{ + {Name: "ZzOrder_Customer", ParentID: from.ID, ChildRef: "Administration.ZzCustomer", + Type: "Reference", Owner: "Default", StorageFormat: "Column"}, + {Name: "ZzKeeper_Account", ParentID: keep.ID, ChildRef: "Administration.Account", + Type: "Reference", Owner: "Default", StorageFormat: "Column"}, + } { + if err := b.CreateCrossAssociation(fromDM.ID, ca); err != nil { + t.Fatalf("CreateCrossAssociation(%s): %v", ca.Name, err) + } + } + + if tc.dropChild { + if err := b.DeleteEntity(toDM.ID, to.ID); err != nil { + t.Fatalf("DeleteEntity(to): %v", err) + } + } else { + if err := b.DeleteEntity(fromDM.ID, from.ID); err != nil { + t.Fatalf("DeleteEntity(from): %v", err) + } + } + + after, err := b.GetDomainModel(fromMod.ID) + if err != nil { + t.Fatalf("GetDomainModel after delete: %v", err) + } + var names []string + for _, ca := range after.CrossAssociations { + names = append(names, ca.Name) + if ca.Name == "ZzOrder_Customer" { + t.Errorf("cross-module association %q survived the delete — it now points at an entity "+ + "that does not exist, which mxbuild reports as a KeyNotFoundException at "+ + "ResolvePostponedProperties (by id) or CE1613 (by name)", ca.Name) + } + } + var keptFound bool + for _, n := range names { + if n == "ZzKeeper_Account" { + keptFound = true + } + } + if !keptFound { + t.Errorf("the unrelated cross-module association was removed too (remaining: %v) — "+ + "the sweep must match the deleted entity, not clear the collection", names) + } + }) + } +} From 2455ee9f72f8b2e5fee2ba2af2d2b4056fbb3a27 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 20 Sep 2026 21:05:15 +0000 Subject: [PATCH 10/10] fix(security): write no member access for the audit associations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An access rule on an entity carrying AutoOwner or AutoChangedBy made mxbuild report the whole module as CE0066 "Entity access is out of date" — and UPDATE SECURITY, the documented repair for that error, printed "Reconciled 1 access rule(s)" and left it standing, because it re-added the entry that caused it. Four lines on a clean production-security app reproduce it: alter entity Mod.Fab add attribute Owner: AutoOwner; update security; mxcli wrote a MemberAccess for the implicit System.owner / System.changedBy association. Mendix maintains those members itself and treats a rule naming one as out of date. Measured on mxbuild 11.14.0, one entity, one rule, one variable at a time: AutoOwner + MemberAccess System.owner CE0066 AutoOwner + no entry 0 errors AutoChangedBy + MemberAccess System.changedBy CE0066 AutoChangedBy + no entry 0 errors So all four audit members follow one rule. The DATE half was already right (issuetracker #20); the association half was assumed to be the opposite case because Mendix really does add those two implicitly — the same inference the earlier finding warned against in this very file ("ask mxbuild what it wants instead of inferring symmetry"). Three parts, because two writers had to agree and a damaged project has to be repairable: - the GRANT handler no longer adds the entry; - ReconcileMemberAccesses no longer adds it; - ReconcileMemberAccesses REMOVES a stored one, ahead of the foreign-module branch that would otherwise preserve it forever on the grounds that System is not loaded here. Without this, `update security` still could not repair what it exists to repair. Measured: CE0066 -> 0 errors on both damaged projects. It also clears a stale System.owner left behind when the flag is turned off again. Exactly System.owner and System.changedBy, not every System.* reference: an entity specialising a System entity legitimately inherits that module's real associations. Control: with the fix reverted the new test fails on all three flag combinations, naming the entry and the CE code; the bug-test's audit-free entity keeps its own attribute's member access, so a build writing no members at all fails it too. Closes #554 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01L2aYb3zDscezm874CDTH6R --- .../fix-issue/findings/mdl-backend.jsonl | 1 + .../security-554-autoowner-access-rule.mdl | 69 ++++++++ .../modelsdk/access_rule_audit_member_test.go | 149 ++++++++++++++++++ .../modelsdk/domainmodel_security_write.go | 66 ++++---- mdl/executor/cmd_security_write.go | 24 ++- 5 files changed, 266 insertions(+), 43 deletions(-) create mode 100644 mdl-examples/bug-tests/security-554-autoowner-access-rule.mdl create mode 100644 mdl/backend/modelsdk/access_rule_audit_member_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-backend.jsonl b/.claude/skills/fix-issue/findings/mdl-backend.jsonl index aef2416a46..6a458b5d7c 100644 --- a/.claude/skills/fix-issue/findings/mdl-backend.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-backend.jsonl @@ -119,3 +119,4 @@ {"area": "mdl/backend", "date": "2026-09-20", "symptom": "Every page mxcli wrote carried `Autofocus` (a property Mendix introduced in 11.1.0) and every page and snippet carried `Variables` (10.17.0), whatever the project's version. On a Mendix 10 project the document therefore held a key that project's metamodel does not declare — the class of defect that makes Studio Pro throw InvalidOperationException at MprProperty.cs while mxbuild reports 0 errors. Found while fixing mendixlabs/mxcli#1121 (the same defect in the page's PARAMETERS) and deferred there as out of scope.", "cause": "`pageToGen` called `SetAutofocus(\"DesktopOnly\")` unconditionally, and both `Forms$Page` and `Forms$Snippet` registered `Variables` in `codec.RegisterTypeDefaults(...).MandatoryLists`, which the encoder emits as an empty typed-array marker for every new element. The page writer had no version awareness at all until #1121 threaded `*types.ProjectVersion` into `pageToGen` for the parameter keys.", "file": "`mdl/backend/modelsdk/page_write.go` (pageSupportsAutofocus/pageSupportsVariables, encodePage, docEncoder), `mdl/backend/modelsdk/snippet_write.go` (encodeSnippet), `modelsdk/codec/encoder.go` (Encoder.OmitKeys). Tests `mdl/backend/modelsdk/page_version_keys_test.go`, `modelsdk/codec/omitkeys_test.go`.", "insight": "**A version-floored key can arrive by two different routes, and only one of them is a gen property.** `Autofocus` is set through gen, so the fix is to not set it; `Variables` exists only because the Studio Pro defaults registry adds it, and a gen `PartList` has no 'present but empty' state — so there is nothing to leave unset and the suppression has to happen in the encoder. That registry is global and keyed by `$Type` alone, so it cannot see a project version: hence `Encoder.OmitKeys`, per-encode. **Suppressing a key and dropping data are different things**: the empty `Variables` marker is safe to suppress, but variables the script DECLARED are refused instead (guard-don't-drop, ADR-0005) — silently dropping them leaves widgets referencing names that are gone (CE1151) from a statement that reported success. Two measurement traps hit on the way: the first control run died on a pre-existing entity before reaching the page statement, so the page was never rewritten and the unchanged output read as a pass — a control that does not run looks exactly like a control that passes; and ALTER PAGE turned out to be safe without any change, because the page mutator edits the stored raw BSON and marshals it back, so it can never invent a key. Check which write paths actually rebuild a document before assuming a header fix has to cover all of them.", "refs": ["#1121"]} {"area": "mdl/backend", "date": "2026-09-18", "symptom": "`ALTER PAGE M.P { SET Documentation = '...' }` is refused: \"unsupported page-level property: Documentation\". Documenting an existing page therefore means re-running its CREATE, because the `/** … */` doc comment on the create statement is the only other source", "cause": "`applyPageLevelSetMut` handled Title/Url/Popup*/Class/Style and had no Documentation case. The field was otherwise fully understood — `pageToGen` has always written it on CREATE via `out.SetDocumentation`, and gen binds it as `property.NewPrimitive[string](\"Documentation\")`. The grammar already parsed it (`identifierOrKeyword EQUALS propertyValueV3`) and the executor has no allowlist, so it was one missing case in the mutator, not a syntax gap", "file": "`mdl/backend/pagemutator/mutator.go` (`applyPageLevelSetMut`)", "insight": "One case covers **Page, Layout AND Snippet** — all three declare Documentation and all three reach this function through `SetWidgetProperty(\"\")`. Store an empty string rather than rejecting it: removing a doc comment from a script has to be expressible, and the property is a bare string with no unset value. **Update the unsupported-property message in the same change** — that list is the only guidance a reader gets, and a stale one sends them back to the CREATE workaround the fix exists to remove (a test asserts the message names Documentation). Note the re-run workaround is worse than it sounds: describe → exec is only as complete as what MDL can spell, so restating a page to document it can silently lose widgets", "refs": ["#527"]} {"area": "mdl/backend", "date": "2026-09-20", "symptom": "`describe page` → `exec` over a **Studio Pro-authored** page reports `Replaced page`, not `Unchanged` — the rebuild is not semantically equal to what was stored, so ADR-0008's elision cannot fire and the unit churns in version control on every re-run. `mx check` is 0 errors either way. Measured on ako/TestApp Rules.RuleAction_NewEdit at 11.14.0: fourteen differences", "cause": "Four independent classes, all 'the rebuild writes a constant where Studio Pro stores a value': (1) `pageToGen` hardcoded Autofocus/CanvasWidth/CanvasHeight; (2) save_changes/cancel_changes/close_page/delete_object never wrote `DisabledDuringExecution`, and save_changes wrote `SyncAutomatically` true; (3) `AttributeRef.EntityRef` emitted only on the navigated branch; (4) `Forms$PageVariable` had only the one name field set, so the other five keys were never marked dirty", "file": "`mdl/backend/modelsdk/page_write.go` (`carryStoredPageHeader`, `bsonInt`), `widget_write.go` (four action cases + two `RegisterTypeDefaults`), `modelsdk/codec/defaults.go` + `encoder.go` (new `FalseFields`)", "insight": "**mxcli round-tripping its own output proves NOTHING about this class** — measured: the MDL bug-test reports `Unchanged` on the unfixed build too, because mxcli writes the page and mxcli describes it, so its constants agree with themselves. The reference must be a Studio Pro document; a committed CI fixture only works if it is one. **A population selected by name can confirm whatever it excluded**: the first sweep filtered `$Type` on `endswith(\"ClientAction\")`, got a tidy 'True on 82 of 82', and so missed `Forms$NoAction` (False on 83 of ~7,300) and `Forms$MicroflowAction` (5 of 81) — scan by the PROPERTY, not by a name pattern. **Hardcoded looked safe and was not**: CanvasWidth takes seven distinct values across 67 pages and the hardcoded 1200 matched 4, so a round trip moved the canvas of 63. **The int width bit this fix once**: Studio Pro stores both canvas dimensions as int64 while the gen setter takes int32, so the natural `.(int32)` assertion matched nothing — and the first unit test passed anyway because its own fixture wrote int32, i.e. the test encoded the assumption under test (bson-numeric-width). Prefer `TypeDefaults` over patching each construction site: `Forms$PageVariable` is built in three places. Remaining after the fix: 1 of 14, a pluggable-widget Object property — CE0463 territory, deliberately out of scope", "refs": ["#541", "#529"]} +{"area": "mdl/backend", "date": "2026-09-20", "symptom": "An access rule on an entity carrying `AutoOwner` (or `AutoChangedBy`) made mxbuild report the whole module as **CE0066** \"Entity access is out of date\" \u2014 and `UPDATE SECURITY`, the documented repair for exactly that error, printed `Reconciled 1 access rule(s) in module Mod` and left the error standing. Four lines reproduce it on a clean production-security app: `alter entity M.Fab add attribute Owner: AutoOwner;` + `update security;`. The original reporter bisected 9 entities and 36 rules one grant at a time behind a ~40s `mx check` to find it, because CE0066 names only the module.", "cause": "mxcli wrote a MemberAccess for the implicit `System.owner` / `System.changedBy` association, in TWO places that had to agree: the GRANT handler (`cmd_security_write.go`) and `ReconcileMemberAccesses`. Mendix maintains those members itself and treats a rule naming one as out of date. The audit DATE members were already known to work this way (issuetracker #20) \u2014 the owner/changedBy pair was assumed to be the opposite case because they are associations rather than attributes, and Mendix really does add them implicitly. Fixed by writing no entry for any of the four, and by REMOVING a stored one in the reconcile (an explicit case before the foreign-module branch, which otherwise preserves `System.*` forever on the grounds that System is not loaded).", "file": "`mdl/backend/modelsdk/domainmodel_security_write.go` (isAuditMemberRef, ReconcileMemberAccesses), `mdl/executor/cmd_security_write.go`", "insight": "**The decisive probe was removing the entry, not adding anything.** CE0066 says 'out of date', which reads as 'something is missing' and sends you looking for a member to add; the model had one too many. A build flag (`MXCLI_PROBE_NO_SYSOWNER`) that dropped the entry took the module from CE0066 to 0 errors in one mxbuild run and settled it. The same repo's earlier finding had already written the rule down \u2014 *'Ask mxbuild what it wants instead of inferring symmetry'* \u2014 and this defect is that exact inference, made in the same file for the sibling members. **A fix here is not done when the new writes are correct**: `update security` exists to repair a project an older mxcli damaged, so the reconcile has to remove the entry, not merely stop adding it. Measured separately: a stale `System.owner` entry survived even after the flag was turned off, because `!assocRefBelongsTo` preserved it as an unverifiable foreign-module reference.", "refs": ["#554", "#524", "issuetracker #20"]} diff --git a/mdl-examples/bug-tests/security-554-autoowner-access-rule.mdl b/mdl-examples/bug-tests/security-554-autoowner-access-rule.mdl new file mode 100644 index 0000000000..cfd431d515 --- /dev/null +++ b/mdl-examples/bug-tests/security-554-autoowner-access-rule.mdl @@ -0,0 +1,69 @@ +-- ako/mxcli#554 — an access rule on an entity carrying AutoOwner (or +-- AutoChangedBy) was written with a MemberAccess for the implicit System.owner / +-- System.changedBy association, and mxbuild reported the whole module as +-- +-- [CE0066] "Entity access is out of date. Please update security by clicking +-- the 'Update security' button in the domain model editor." +-- at Domain model of module 'Mod' +-- +-- `UPDATE SECURITY` — the documented repair for exactly this error — reported +-- "Reconciled 1 access rule(s)" and left the error standing, because it re-added +-- the entry that caused it. A command that is confidently wrong costs more than +-- one that says nothing, which is why the repair path is part of the fix. +-- +-- The audit DATE members were already known to work the other way round (an entry +-- is CE0066, no entry checks clean — issuetracker #20). These two were assumed to +-- be the opposite case because they are ASSOCIATIONS rather than attributes, and +-- Mendix really does add them implicitly. Measured on mxbuild 11.14.0, one entity, +-- one rule, one variable at a time: +-- +-- AutoOwner + MemberAccess System.owner CE0066 +-- AutoOwner + no entry 0 errors +-- AutoChangedBy + MemberAccess System.changedBy CE0066 +-- AutoChangedBy + no entry 0 errors +-- +-- Run with `mxcli exec`, then `mxcli docker check --no-update-widgets`: the +-- project must report 0 errors at production security level. + +alter project security level production; + +create module Sec554; + +create module role Sec554.Administrator; +create or modify user role Administrator (Sec554.Administrator, System.User); + +-- The owner half. +create persistent entity Sec554.Fab ( + FabName: String(100), + Owner: AutoOwner +); + +-- The changedBy half. +create persistent entity Sec554.Line ( + LineName: String(100), + ChangedBy: AutoChangedBy +); + +-- Both at once, plus the audit DATE members, which are the case this one was +-- wrongly assumed to differ from. +create persistent entity Sec554.Tool ( + ToolName: String(100), + Owner: AutoOwner, + ChangedBy: AutoChangedBy, + CreatedDate: AutoCreatedDate, + ChangedDate: AutoChangedDate +); + +-- The control: an entity with no audit members at all. Its rule is unaffected, +-- and a build that wrote no members at all would fail this alongside the rest. +create persistent entity Sec554.Customer ( + CustomerName: String(100) +); + +grant Sec554.Administrator on Sec554.Fab (create, delete, read *, write *); +grant Sec554.Administrator on Sec554.Line (create, delete, read *, write *); +grant Sec554.Administrator on Sec554.Tool (create, delete, read *, write *); +grant Sec554.Administrator on Sec554.Customer (create, delete, read *, write *); + +-- Must be a no-op here, and must REPAIR a project an older mxcli damaged. +update security; diff --git a/mdl/backend/modelsdk/access_rule_audit_member_test.go b/mdl/backend/modelsdk/access_rule_audit_member_test.go new file mode 100644 index 0000000000..f8f2965e8c --- /dev/null +++ b/mdl/backend/modelsdk/access_rule_audit_member_test.go @@ -0,0 +1,149 @@ +// SPDX-License-Identifier: Apache-2.0 + +// ako/mxcli#554 — an access rule on an entity carrying AutoOwner or AutoChangedBy +// was written with a MemberAccess for the implicit System.owner / System.changedBy +// association, and mxbuild then reported CE0066 "Entity access is out of date" +// against the whole module — which `update security` could not repair, because it +// re-added the very entry that caused it and reported success in the same breath. +// +// The audit DATE members were already known to work the other way round (an entry +// is CE0066, no entry checks clean — issuetracker #20). These two were assumed to +// be the opposite case because they are associations rather than attributes, and +// Mendix really does add them implicitly. Measured on mxbuild 11.14.0, one entity, +// one rule, one variable at a time: +// +// AutoOwner + MemberAccess System.owner CE0066 +// AutoOwner + no entry 0 errors +// AutoChangedBy + MemberAccess System.changedBy CE0066 +// AutoChangedBy + no entry 0 errors +package modelsdkbackend + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/backend" + "github.com/mendixlabs/mxcli/mdl/types" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/domainmodel" +) + +func TestAccessRule_NoMemberAccessForAuditAssociations(t *testing.T) { + for _, tc := range []struct { + name string + owner bool + changed bool + stale string // an entry already in the stored rule, to be repaired away + mustGone []string + }{ + {name: "AutoOwner", owner: true, mustGone: []string{"System.owner"}}, + {name: "AutoChangedBy", changed: true, mustGone: []string{"System.changedBy"}}, + {name: "both", owner: true, changed: true, mustGone: []string{"System.owner", "System.changedBy"}}, + // The repair path: a project damaged by an older mxcli must come back + // clean, since `update security` is the documented remedy for CE0066. + // The flag is OFF here, so the entry is stale twice over. + {name: "stale entry with no flag is repaired away", stale: "System.owner", mustGone: []string{"System.owner"}}, + } { + t.Run(tc.name, func(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) + } + + ent := &domainmodel.Entity{ + Name: "ZzFab", + Persistable: true, + HasOwner: tc.owner, + HasChangedBy: tc.changed, + Attributes: []*domainmodel.Attribute{ + {Name: "FabName", Type: &domainmodel.StringAttributeType{Length: 100}}, + }, + } + if err := b.CreateEntity(dm.ID, ent); err != nil { + t.Fatalf("CreateEntity: %v", err) + } + + members := []types.EntityMemberAccess{{AttributeRef: "MyFirstModule.ZzFab.FabName", AccessRights: "ReadWrite"}} + if tc.stale != "" { + members = append(members, types.EntityMemberAccess{AssociationRef: tc.stale, AccessRights: "ReadWrite"}) + } + if err := b.AddEntityAccessRule(backend.EntityAccessRuleParams{ + UnitID: dm.ID, + EntityName: "ZzFab", + RoleNames: []string{"MyFirstModule.User"}, + AllowCreate: true, + AllowDelete: true, + DefaultMemberAccess: "ReadWrite", + MemberAccesses: members, + }); err != nil { + t.Fatalf("AddEntityAccessRule: %v", err) + } + + // The reconcile `update security` runs, and which every write path runs + // after a program. It must not re-add what the grant left out. + if _, err := b.ReconcileMemberAccesses(dm.ID, "MyFirstModule"); err != nil { + t.Fatalf("ReconcileMemberAccesses: %v", err) + } + + got := memberRefsOf(t, b, mod.ID, "ZzFab") + for _, ref := range tc.mustGone { + for _, g := range got { + if g == ref { + t.Errorf("access rule carries a MemberAccess for %q — mxbuild reports the module as "+ + "CE0066 \"Entity access is out of date\" (members: %v)", ref, got) + } + } + } + // Control: the reconcile still covers a real member. Without this the + // test passes against a build that writes no members at all. + var sawAttr bool + for _, g := range got { + if g == "MyFirstModule.ZzFab.FabName" { + sawAttr = true + } + } + if !sawAttr { + t.Errorf("the entity's own attribute lost its MemberAccess (members: %v)", got) + } + }) + } +} + +// memberRefsOf returns the member references of the entity's first access rule, +// read back from storage rather than from the value that was written. +func memberRefsOf(t *testing.T, b *Backend, moduleID model.ID, entityName string) []string { + t.Helper() + dm, err := b.GetDomainModel(moduleID) + if err != nil { + t.Fatalf("GetDomainModel: %v", err) + } + for _, e := range dm.Entities { + if e.Name != entityName { + continue + } + var out []string + for _, r := range e.AccessRules { + for _, m := range r.MemberAccesses { + if m.AttributeName != "" { + out = append(out, m.AttributeName) + } + if m.AssociationName != "" { + out = append(out, m.AssociationName) + } + } + } + return out + } + t.Fatalf("entity %s not found after write", entityName) + return nil +} diff --git a/mdl/backend/modelsdk/domainmodel_security_write.go b/mdl/backend/modelsdk/domainmodel_security_write.go index 194e54badc..bb57615570 100644 --- a/mdl/backend/modelsdk/domainmodel_security_write.go +++ b/mdl/backend/modelsdk/domainmodel_security_write.go @@ -331,6 +331,16 @@ func sameStringSet(a, b []string) bool { return true } +// isAuditMemberRef reports whether a member reference names one of the two audit +// ASSOCIATIONS Mendix maintains from an entity's own flags. +// +// Exactly these two, not every System.* reference: an entity specialising a +// System entity legitimately inherits that module's real associations, and those +// are preserved by the foreign-module branch. +func isAuditMemberRef(ref string) bool { + return ref == "System.owner" || ref == "System.changedBy" +} + // ReconcileMemberAccesses brings every populated access rule in a domain model // into sync with its entity's current members: it adds a MemberAccess for each // attribute, each FROM-side association (regular + cross), and each implicit @@ -480,25 +490,27 @@ func (b *Backend) ReconcileMemberAccesses(unitID model.ID, moduleName string) (i } } - // Implicit system associations from NoGeneralization flags. - var sysRefs []string - sysSet := map[string]bool{} - // Audit DATE members (createdDate/changedDate) are stored as flags too, but - // they are attributes rather than associations. Mendix has no MemberAccess - // for them at all: an entity storing them checks clean with no entry, and - // mxbuild rejects a rule that carries one with CE0066 "Entity access is out - // of date" (verified on 11.12.1). So they are neither added here nor - // preserved — the executor refuses to author one (issuetracker #20). - if ng, ok := ent.Generalization().(*genDm.NoGeneralization); ok { - if ng.HasOwner() { - sysRefs = append(sysRefs, "System.owner") - sysSet["System.owner"] = true - } - if ng.HasChangedBy() { - sysRefs = append(sysRefs, "System.changedBy") - sysSet["System.changedBy"] = true - } - } + // NO MemberAccess is written for an audit member, of either kind. + // + // The DATE members (createdDate/changedDate) were measured first: an entity + // storing them checks clean with no entry, and mxbuild rejects a rule that + // carries one with CE0066 (issuetracker #20, verified on 11.12.1). + // + // System.owner and System.changedBy were assumed to be the other case, + // because they are associations rather than attributes — Mendix does add + // them implicitly, so an entry for them looked required by symmetry. It is + // not. Measured on mxbuild 11.14.0, one entity, one rule, one variable: + // + // AutoOwner + MemberAccess System.owner CE0066 + // AutoOwner + no entry 0 errors + // AutoChangedBy + MemberAccess System.changedBy CE0066 + // AutoChangedBy + no entry 0 errors + // + // So all four audit members follow one rule: they are members Mendix + // manages, and naming one in an access rule makes the rule out of date + // (ako/mxcli#554). Nothing is added here, and a stored entry is removed + // below rather than preserved — `update security` is the documented repair + // for CE0066, so it has to be able to undo this. for _, re := range ent.AccessRulesItems() { rule, ok := re.(*genDm.AccessRule) @@ -516,7 +528,6 @@ func (b *Backend) ReconcileMemberAccesses(unitID model.ID, moduleName string) (i covAttr := map[string]bool{} covAssoc := map[string]bool{} - covSys := map[string]bool{} changed := false // Walk existing entries back-to-front: drop stale, downgrade calc. @@ -557,8 +568,13 @@ func (b *Backend) ReconcileMemberAccesses(unitID model.ID, moduleName string) (i } case assocRef != "": switch { - case sysSet[assocRef]: - covSys[assocRef] = true + case isAuditMemberRef(assocRef): + // An audit member Mendix manages itself. Removed rather than + // preserved by the foreign-module branch below, which would + // otherwise keep it forever on the grounds that System is not + // loaded here (ako/mxcli#554). + rule.RemoveMemberAccesses(i) + changed = true case assocSet[assocRef]: covAssoc[assocRef] = true case !assocRefBelongsTo(assocRef, moduleName): @@ -597,12 +613,6 @@ func (b *Backend) ReconcileMemberAccesses(unitID model.ID, moduleName string) (i changed = true } } - for _, ref := range sysRefs { - if !covSys[ref] { - rule.AddMemberAccesses(newMemberAccess(defRights, ref, false)) - changed = true - } - } if changed { modified++ diff --git a/mdl/executor/cmd_security_write.go b/mdl/executor/cmd_security_write.go index 619d2d12c9..9cfde54587 100644 --- a/mdl/executor/cmd_security_write.go +++ b/mdl/executor/cmd_security_write.go @@ -541,21 +541,15 @@ func execGrantEntityAccess(ctx *ExecContext, s *ast.GrantEntityAccessStmt) error entityQN, strings.Join(unknown, ", ")) } - // Add MemberAccess entries for system associations (owner, changedBy). - // When an entity has HasOwner/HasChangedBy, Mendix implicitly adds - // System.owner/System.changedBy associations that require MemberAccess. - if entity.HasOwner { - memberAccesses = append(memberAccesses, types.EntityMemberAccess{ - AssociationRef: "System.owner", - AccessRights: defaultMemberAccess, - }) - } - if entity.HasChangedBy { - memberAccesses = append(memberAccesses, types.EntityMemberAccess{ - AssociationRef: "System.changedBy", - AccessRights: defaultMemberAccess, - }) - } + // No MemberAccess is written for System.owner / System.changedBy. They are + // implicit associations Mendix maintains from the entity's own flags, and + // naming one in a rule makes the rule out of date: mxbuild 11.14.0 reports + // CE0066 with the entry and 0 errors without it, measured one member at a + // time (ako/mxcli#554). The audit DATE members were already handled this way; + // these two were assumed to be the other case because they are associations. + // Both writers have to agree, so the same rule lives in + // ReconcileMemberAccesses — a GRANT that added the entry would be undone by + // the next reconcile, and vice versa. // A constraint too long to read on one line is broken at its boolean joints; // one that already fits comes back unchanged (upstream #979). It is formatted