From 585f5f0604eb6b83c4c54cb6b3efdc32bd7be37e Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 14:29:03 +0000 Subject: [PATCH 1/8] fix(pages): read a pluggable widget's microflow datasource from MicroflowSettings (#795) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DESCRIBE PAGE emitted `datagrid g1 {` with no DataSource for a DataGrid2 bound to a microflow, so re-applying the describe output recreated the grid with no data source at all. A `database from` source described correctly, which made the loss look selective. A Forms$MicroflowSource stores the microflow name in the nested Forms$MicroflowSettings ("MicroflowSettings" -> "Microflow") — what the write path emits and what Studio Pro stores. extractDataGrid2DataSource looked up a top-level "Microflow" key, got "", and returned no datasource. The describe formatter's `case "microflow"` branch was correct all along, so nothing was ever wrong with the stored model or the write path: purely a read bug. Four readers had their own copy of this lookup and disagreed — two read the nested settings, one read the top-level key, one did both. They now share microflowSourceRef / nanoflowSourceRef, which read the nested settings and fall back to the legacy top-level key so older files still round-trip. Two gaps the regression tests found while covering this: - a Gallery whose source used the legacy flat shape was unreadable too: the correct reader it delegates to handles only the nested form, and the flat fallback in its own switch is not reached for a CustomWidget gallery - Forms$NanoflowSource was missing entirely from the DataGrid2 and Gallery switches, so a Studio-Pro-authored nanoflow datasource described as nothing Left alone deliberately: CustomWidgets$CustomWidgetNanoflowSource, a different metamodel type whose Nanoflow key really is top-level, and the Forms$MicroflowAction reads, which are actions rather than datasources. Also corrects the datasource table in the create-page skill: it documented `datasource: microflow Module.GetData()`, and the trailing `()` does not parse. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012XR649rKk68z6gBpngu6MA --- .claude/skills/fix-issue.md | 1 + .claude/skills/mendix/create-page.md | 4 +- ...datagrid-microflow-datasource-describe.mdl | 73 +++++++++ .../cmd_pages_describe_flowsource_test.go | 149 ++++++++++++++++++ mdl/executor/cmd_pages_describe_parse.go | 27 +--- mdl/executor/cmd_pages_describe_pluggable.go | 62 +++++--- 6 files changed, 276 insertions(+), 40 deletions(-) create mode 100644 mdl-examples/bug-tests/795-datagrid-microflow-datasource-describe.mdl create mode 100644 mdl/executor/cmd_pages_describe_flowsource_test.go diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index ec0c4bd03..0a4ff6aa3 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -16,6 +16,7 @@ to the symptom table below, so the next similar issue costs fewer reads. | Symptom | Root cause layer | First file to open | Fix pattern | |---------|-----------------|-------------------|-------------| +| `describe page` omits a **pluggable** widget's `DataSource` when it is bound to a microflow (`datagrid g1 {` with no DataSource), while a `database from` source describes fine — re-applying the output recreates the grid unbound | A `Forms$MicroflowSource` stores the name in the nested `Forms$MicroflowSettings` (`MicroflowSettings` → `Microflow`), which is what the write path and Studio Pro emit; the reader looked up a top-level `Microflow` key, got `""`, and returned no datasource. The describe *formatter* was correct all along — read bug only | `mdl/executor/cmd_pages_describe_pluggable.go` (`microflowSourceRef`, `nanoflowSourceRef`, `extractDataGrid2DataSource`, `extractGalleryDataSource`, `parseCustomWidgetDataSource`) | Read the nested settings with a top-level fallback, via one shared helper — there were four divergent copies of this lookup and two were wrong. `Forms$NanoflowSource` was missing entirely from the DataGrid2/Gallery switches; add it alongside. Do **not** touch `CustomWidgets$CustomWidgetNanoflowSource` (a different metamodel type whose `Nanoflow` really is top-level) or the `Forms$MicroflowAction` reads (actions, not datasources). Repro `mdl-examples/bug-tests/795-datagrid-microflow-datasource-describe.mdl`. Issue #795 | | Any `ALTER SETTINGS` (any section) reports success but the Default configuration's **Custom settings** are gone, **Tracing** is reset, and every **constant override** shows blank in Studio Pro — Integer/Long constants then fail the build | The configuration was re-serialized from `model.ServerConfiguration`, which carries only the modelled fields, so CustomSettings/Tracing/OpenAdminPort/OpenHttpPort were dropped, the list version markers downgraded 3→2, and overrides were written with a flat `Value` instead of the nested `SharedOrPrivateValue` the platform reads | `mdl/settingsoverlay/settingsoverlay.go` (`Configurations`, `ServerConfiguration`, `ConstantValues`) — called by both `mdl/backend/modelsdk/settings_write.go` and `sdk/mpr/writer_settings.go` | Overlay onto the raw document instead of rebuilding: write only the fields the read path populates, take each list's marker from what is stored, and update a constant override in the slot it already occupies (nested if nested, flat if flat). New override → nested. New configuration → clone a sibling's shape, empty its collections, mint a fresh `$ID`. Refuse the write when `RawParts` is empty. Repro `mdl-examples/bug-tests/801-alter-settings-preserves-configuration.mdl`. Issue #801 | | `ALTER SETTINGS` / `CREATE CONFIGURATION` prints "Updated …" but `DESCRIBE SETTINGS` shows the old value — an Integer property was given a non-numeric value, or a Boolean anything other than `true` | `strconv.Atoi`'s error was discarded (`if v, err := …; err == nil`) so the assignment was skipped while the caller still printed success; the boolean form compared against `"true"`, silently mapping every other spelling to false | `mdl/executor/cmd_settings.go` (`settingsInt`, `settingsBool`) + `mdl/executor/validate_settings.go` (`typedSettingsKeys`, MDL-SET01/MDL-SET02) | Parse through a helper that returns a validation error naming the setting and the offending value, and register the property in `typedSettingsKeys` so `mxcli check` and the LSP flag it before the project is opened for writing. `TestTypedSettingsKeys_MatchExecutor` guards the table against drifting from the executor's switch. Repro `mdl-examples/bug-tests/805-alter-settings-typed-values.fail.mdl`. Issue #805 | | MCP op reports `MCP error -32000: Request timed out` but the page/document/entity EXISTS in Studio Pro afterwards | Studio Pro's ~30s server-side per-call limit fires while the op still applies — a client false failure, not a server rejection | `mdl/backend/mcp/timeout.go` (`isTimeoutErr`, `timeoutVerifyDelay`, `pedUpdateVerify`, `pedDocumentExists`) | Verify-on-timeout, never blind-retry a non-idempotent op: idempotent root-replace (`pgWritePage`) retries once; creates confirm via `ped_find_document`; entity adds confirm via a shallow `/entities` read. Unverified → error with save-before-re-run guidance (a blind re-run from a fresh session duplicates elements) | diff --git a/.claude/skills/mendix/create-page.md b/.claude/skills/mendix/create-page.md index 91332e9c3..9d540f760 100644 --- a/.claude/skills/mendix/create-page.md +++ b/.claude/skills/mendix/create-page.md @@ -444,8 +444,8 @@ column colActions (caption: 'Actions') { |--------|-------------| | `datasource: database from Module.Entity` | Direct database query | | `datasource: $Variable` | Variable bound (requires DATAVIEW parent with entity) | -| `datasource: microflow Module.GetData()` | Microflow datasource | -| `datasource: nanoflow Module.GetData()` | Nanoflow datasource (client-side, no server roundtrip) | +| `datasource: microflow Module.GetData` | Microflow datasource — no `()`, the name alone | +| `datasource: nanoflow Module.GetData` | Nanoflow datasource (client-side, no server roundtrip) — no `()` | | `datasource: selection widgetName` | Listen to selection from another widget | | `datasource: association path` | Retrieve by association from context (ByAssociation) | | `datasource: $currentObject/Module.Assoc` | Sugar for `association` — same semantics, reads more naturally | diff --git a/mdl-examples/bug-tests/795-datagrid-microflow-datasource-describe.mdl b/mdl-examples/bug-tests/795-datagrid-microflow-datasource-describe.mdl new file mode 100644 index 000000000..e577d90f4 --- /dev/null +++ b/mdl-examples/bug-tests/795-datagrid-microflow-datasource-describe.mdl @@ -0,0 +1,73 @@ +-- Bug #795: DESCRIBE PAGE drops a datagrid's microflow DataSource +-- +-- Symptom: a DataGrid2 bound to a microflow described as `datagrid g1 {` with no +-- DataSource at all, so re-applying the describe output recreated the grid with no +-- data source. A database-bound grid described correctly, which is what made the +-- loss look selective. +-- +-- Root cause: a Forms$MicroflowSource stores the microflow name in the nested +-- Forms$MicroflowSettings ("MicroflowSettings" -> "Microflow") — what the write path +-- emits and what Studio Pro stores. extractDataGrid2DataSource looked up a top-level +-- "Microflow" key, got "", and returned no data source. The describe formatter's +-- `case "microflow"` branch was correct all along, so this was purely a read bug. +-- +-- Fix: every datasource reader now goes through microflowSourceRef / +-- nanoflowSourceRef (mdl/executor/cmd_pages_describe_pluggable.go), which read the +-- nested settings and fall back to the legacy top-level key. The readers had four +-- divergent copies of this lookup; two of them were wrong. +-- +-- Manual verification (needs a project, so `make check-mdl` only syntax-checks this): +-- +-- mxcli exec 795-datagrid-microflow-datasource-describe.mdl -p app.mpr +-- mxcli -p app.mpr -c "describe page Issue795.Probe" +-- +-- Expect `datagrid gMicroflow (DataSource: microflow Issue795.GetBuckets)` — before +-- the fix the DataSource was absent. Feeding that describe output back through +-- `mxcli exec` and describing again must produce identical output. + +create module Issue795; +create module role Issue795.User; + +@position(100, 100) +create persistent entity Issue795.Bucket ( + BucketKey: string(200), + Size: integer +); + +create microflow Issue795.GetBuckets () +returns list of Issue795.Bucket as $items +begin + retrieve $items from Issue795.Bucket; + return $items; +end; + +create or replace page Issue795.Probe ( + Title: 'Probe', + Layout: Atlas_Core.Atlas_Default +) { + -- The regression: a microflow-bound grid lost its DataSource on describe. + datagrid gMicroflow (datasource: microflow Issue795.GetBuckets) { + column colKey (attribute: BucketKey, caption: 'Key') + } + + -- Control case: a database-bound grid always described correctly. + datagrid gDatabase (datasource: database from Issue795.Bucket) { + column colKey2 (attribute: BucketKey, caption: 'Key') + } +} + +-- A gallery exercises the same reader from a different entry point; its legacy +-- flat-shaped source was unreadable too. +create or replace page Issue795.ProbeGallery ( + Title: 'Probe Gallery', + Layout: Atlas_Core.Atlas_Default +) { + gallery galMicroflow (datasource: microflow Issue795.GetBuckets) { + template template1 { + dynamictext txtKey (content: '{1}', contentparams: [{1} = BucketKey]) + } + } +} + +describe page Issue795.Probe; +describe page Issue795.ProbeGallery; diff --git a/mdl/executor/cmd_pages_describe_flowsource_test.go b/mdl/executor/cmd_pages_describe_flowsource_test.go new file mode 100644 index 000000000..1d159e1e9 --- /dev/null +++ b/mdl/executor/cmd_pages_describe_flowsource_test.go @@ -0,0 +1,149 @@ +// SPDX-License-Identifier: Apache-2.0 + +// mendixlabs/mxcli#795: DESCRIBE PAGE drops a datagrid's microflow DataSource, so +// re-applying the describe output recreates the grid with no data source at all. +// +// Root cause: a Forms$MicroflowSource stores the microflow name in the nested +// Forms$MicroflowSettings ("MicroflowSettings" → "Microflow"), which is what the +// write path emits and what Studio Pro stores. The DataGrid2 and Gallery readers +// looked up a top-level "Microflow" key instead, got "", and returned no data +// source — while the describe formatter's `case "microflow"` branch was correct all +// along. A database source was unaffected, which is why only microflow sources +// silently lost their binding. +package executor + +import "testing" + +// flowSourceWidget builds a minimal pluggable-widget map whose datasource is the +// given raw source document, mirroring the on-disk BSON. +func flowSourceWidget(ds map[string]any) map[string]any { + return map[string]any{ + "Object": map[string]any{ + "Properties": []any{ + int32(2), // BSON non-empty-array version marker + map[string]any{ + "Value": map[string]any{"DataSource": ds}, + }, + }, + }, + } +} + +// nestedMicroflowSource is the shape Studio Pro and the codec engine write. +func nestedMicroflowSource(name string) map[string]any { + return map[string]any{ + "$Type": "Forms$MicroflowSource", + "ForceFullObjects": false, + "MicroflowSettings": map[string]any{ + "$Type": "Forms$MicroflowSettings", + "Microflow": name, + }, + } +} + +// flatMicroflowSource is the legacy shape: the name directly on the source. +func flatMicroflowSource(name string) map[string]any { + return map[string]any{ + "$Type": "Forms$MicroflowSource", + "Microflow": name, + } +} + +func nestedNanoflowSource(name string) map[string]any { + return map[string]any{ + "$Type": "Forms$NanoflowSource", + "NanoflowSettings": map[string]any{ + "$Type": "Forms$NanoflowSettings", + "Nanoflow": name, + }, + } +} + +// extractors is every reader that turns a raw datasource document into a +// rawDataSource. They historically disagreed about where the microflow name lives; +// the table keeps them honest with each other. +var extractors = []struct { + name string + fn func(*ExecContext, map[string]any) *rawDataSource +}{ + {"datagrid2", extractDataGrid2DataSource}, + {"gallery", extractGalleryDataSource}, +} + +func TestPluggableDataSource_MicroflowNestedSettings(t *testing.T) { + const mf = "MyModule.DBG_ListBucketObjects" + for _, ex := range extractors { + t.Run(ex.name, func(t *testing.T) { + ds := ex.fn(nil, flowSourceWidget(nestedMicroflowSource(mf))) + if ds == nil { + t.Fatal("microflow datasource dropped: got nil") + } + if ds.Type != "microflow" { + t.Errorf("Type = %q, want microflow", ds.Type) + } + if ds.Reference != mf { + t.Errorf("Reference = %q, want %q", ds.Reference, mf) + } + }) + } +} + +// TestPluggableDataSource_MicroflowFlatLegacy keeps the legacy top-level shape +// readable so files written before the nested form still round-trip. +func TestPluggableDataSource_MicroflowFlatLegacy(t *testing.T) { + const mf = "MyModule.Legacy" + for _, ex := range extractors { + t.Run(ex.name, func(t *testing.T) { + ds := ex.fn(nil, flowSourceWidget(flatMicroflowSource(mf))) + if ds == nil || ds.Type != "microflow" || ds.Reference != mf { + t.Fatalf("legacy flat microflow source not read: %+v", ds) + } + }) + } +} + +// TestPluggableDataSource_Nanoflow covers the sibling source type, which the +// DataGrid2 and Gallery readers did not handle at all. +func TestPluggableDataSource_Nanoflow(t *testing.T) { + const nf = "MyModule.NF_ListBuckets" + for _, ex := range extractors { + t.Run(ex.name, func(t *testing.T) { + ds := ex.fn(nil, flowSourceWidget(nestedNanoflowSource(nf))) + if ds == nil { + t.Fatal("nanoflow datasource dropped: got nil") + } + if ds.Type != "nanoflow" { + t.Errorf("Type = %q, want nanoflow", ds.Type) + } + if ds.Reference != nf { + t.Errorf("Reference = %q, want %q", ds.Reference, nf) + } + }) + } +} + +// TestFlowSourceRef_Helpers pins the shared lookup the readers delegate to. +func TestFlowSourceRef_Helpers(t *testing.T) { + if got := microflowSourceRef(nestedMicroflowSource("A.B")); got != "A.B" { + t.Errorf("microflowSourceRef(nested) = %q", got) + } + if got := microflowSourceRef(flatMicroflowSource("A.B")); got != "A.B" { + t.Errorf("microflowSourceRef(flat) = %q", got) + } + if got := microflowSourceRef(map[string]any{"$Type": "Forms$MicroflowSource"}); got != "" { + t.Errorf("microflowSourceRef(empty) = %q, want empty", got) + } + // A non-map MicroflowSettings must not panic. + if got := microflowSourceRef(map[string]any{"MicroflowSettings": "junk"}); got != "" { + t.Errorf("microflowSourceRef(junk) = %q, want empty", got) + } + if got := nanoflowSourceRef(nestedNanoflowSource("A.B")); got != "A.B" { + t.Errorf("nanoflowSourceRef(nested) = %q", got) + } + if got := nanoflowSourceRef(map[string]any{"Nanoflow": "A.B"}); got != "A.B" { + t.Errorf("nanoflowSourceRef(flat) = %q", got) + } + if got := nanoflowSourceRef(map[string]any{"NanoflowSettings": 42}); got != "" { + t.Errorf("nanoflowSourceRef(junk) = %q, want empty", got) + } +} diff --git a/mdl/executor/cmd_pages_describe_parse.go b/mdl/executor/cmd_pages_describe_parse.go index 9fabb10b2..1930f8024 100644 --- a/mdl/executor/cmd_pages_describe_parse.go +++ b/mdl/executor/cmd_pages_describe_parse.go @@ -574,18 +574,12 @@ func extractDataViewDataSource(ctx *ExecContext, w map[string]any) *rawDataSourc switch dsType { case "Forms$MicroflowSource": - // Extract microflow name from MicroflowSettings - if settings, ok := ds["MicroflowSettings"].(map[string]any); ok { - if mfName, ok := settings["Microflow"].(string); ok && mfName != "" { - return &rawDataSource{Type: "microflow", Reference: mfName} - } + if mf := microflowSourceRef(ds); mf != "" { + return &rawDataSource{Type: "microflow", Reference: mf} } case "Forms$NanoflowSource": - // Extract nanoflow name from NanoflowSettings - if settings, ok := ds["NanoflowSettings"].(map[string]any); ok { - if nfName, ok := settings["Nanoflow"].(string); ok && nfName != "" { - return &rawDataSource{Type: "nanoflow", Reference: nfName} - } + if nf := nanoflowSourceRef(ds); nf != "" { + return &rawDataSource{Type: "nanoflow", Reference: nf} } case "Forms$DataViewSource": // "Data from context over an association" — the DataViewSource carries an @@ -804,18 +798,11 @@ func extractListViewDataSource(ctx *ExecContext, w map[string]any) *rawDataSourc return result } case "Forms$MicroflowSource": - microflow := extractString(ds["Microflow"]) - if mfSettings, ok := ds["MicroflowSettings"].(map[string]any); ok && microflow == "" { - microflow = extractString(mfSettings["Microflow"]) - } - if microflow != "" { - return &rawDataSource{Type: "microflow", Reference: microflow} + if mf := microflowSourceRef(ds); mf != "" { + return &rawDataSource{Type: "microflow", Reference: mf} } case "Forms$NanoflowSource": - nanoflow := extractString(ds["Nanoflow"]) - if nfSettings, ok := ds["NanoflowSettings"].(map[string]any); ok && nanoflow == "" { - nanoflow = extractString(nfSettings["Nanoflow"]) - } + nanoflow := nanoflowSourceRef(ds) if nanoflow != "" { return &rawDataSource{Type: "nanoflow", Reference: nanoflow} } diff --git a/mdl/executor/cmd_pages_describe_pluggable.go b/mdl/executor/cmd_pages_describe_pluggable.go index edc5d3486..6b714463f 100644 --- a/mdl/executor/cmd_pages_describe_pluggable.go +++ b/mdl/executor/cmd_pages_describe_pluggable.go @@ -220,9 +220,12 @@ func extractDataGrid2DataSource(ctx *ExecContext, w map[string]any) *rawDataSour return result } case "Forms$MicroflowSource": - microflow := extractString(ds["Microflow"]) - if microflow != "" { - return &rawDataSource{Type: "microflow", Reference: microflow} + if mf := microflowSourceRef(ds); mf != "" { + return &rawDataSource{Type: "microflow", Reference: mf} + } + case "Forms$NanoflowSource": + if nf := nanoflowSourceRef(ds); nf != "" { + return &rawDataSource{Type: "nanoflow", Reference: nf} } case "Forms$EntityPathSource", "Forms$DataViewSource": entityPath := extractString(ds["EntityPath"]) @@ -720,9 +723,12 @@ func extractGalleryDataSource(ctx *ExecContext, w map[string]any) *rawDataSource return result } case "Forms$MicroflowSource": - microflow := extractString(ds["Microflow"]) - if microflow != "" { - return &rawDataSource{Type: "microflow", Reference: microflow} + if mf := microflowSourceRef(ds); mf != "" { + return &rawDataSource{Type: "microflow", Reference: mf} + } + case "Forms$NanoflowSource": + if nf := nanoflowSourceRef(ds); nf != "" { + return &rawDataSource{Type: "nanoflow", Reference: nf} } case "Forms$EntityPathSource", "Forms$DataViewSource": entityPath := extractString(ds["EntityPath"]) @@ -733,6 +739,34 @@ func extractGalleryDataSource(ctx *ExecContext, w map[string]any) *rawDataSource return nil } +// microflowSourceRef returns the microflow a Forms$MicroflowSource points at. +// +// Studio Pro and the codec engine store the name in the nested Forms$MicroflowSettings; +// a top-level "Microflow" key is the legacy shape, still honoured so older files +// round-trip. Reading only the top-level key made DESCRIBE PAGE drop a datagrid's +// microflow datasource entirely (mendixlabs/mxcli#795), so every reader goes through +// this helper rather than keeping its own copy of the lookup. +func microflowSourceRef(ds map[string]any) string { + if mf := extractString(ds["Microflow"]); mf != "" { + return mf + } + if settings, ok := ds["MicroflowSettings"].(map[string]any); ok { + return extractString(settings["Microflow"]) + } + return "" +} + +// nanoflowSourceRef is the Forms$NanoflowSource counterpart of microflowSourceRef. +func nanoflowSourceRef(ds map[string]any) string { + if nf := extractString(ds["Nanoflow"]); nf != "" { + return nf + } + if settings, ok := ds["NanoflowSettings"].(map[string]any); ok { + return extractString(settings["Nanoflow"]) + } + return "" +} + // parseCustomWidgetDataSource parses datasource from CustomWidget property format. func parseCustomWidgetDataSource(ctx *ExecContext, ds map[string]any) *rawDataSource { dsType := extractString(ds["$Type"]) @@ -767,20 +801,12 @@ func parseCustomWidgetDataSource(ctx *ExecContext, ds map[string]any) *rawDataSo } return result case "Forms$MicroflowSource": - // Pluggable widgets use Forms$MicroflowSource with MicroflowSettings - if settings, ok := ds["MicroflowSettings"].(map[string]any); ok { - microflow := extractString(settings["Microflow"]) - if microflow != "" { - return &rawDataSource{Type: "microflow", Reference: microflow} - } + if mf := microflowSourceRef(ds); mf != "" { + return &rawDataSource{Type: "microflow", Reference: mf} } case "Forms$NanoflowSource": - // Pluggable widgets use Forms$NanoflowSource with NanoflowSettings - if settings, ok := ds["NanoflowSettings"].(map[string]any); ok { - nanoflow := extractString(settings["Nanoflow"]) - if nanoflow != "" { - return &rawDataSource{Type: "nanoflow", Reference: nanoflow} - } + if nf := nanoflowSourceRef(ds); nf != "" { + return &rawDataSource{Type: "nanoflow", Reference: nf} } case "CustomWidgets$CustomWidgetNanoflowSource": nanoflow := extractString(ds["Nanoflow"]) From 78699b8c96c111c379ad2b88a1e7b3f78396450a Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 14:51:39 +0000 Subject: [PATCH 2/8] fix(pages): fail the write when a pluggable widget's child is dropped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A construct the codec engine cannot represent — a nanoflow datasource on a DataGrid2 is the reachable case — was logged and discarded while `mxcli exec` still printed `Created page`. The grid landed with no data source at all. The converter does return an error; it just had nowhere to go. Child content is serialized through widgetobj.ChildSerializer, whose methods return BSON with no error channel — the interface change flagged by the TODO in mdl/backend/widgetobj/builder.go. Until that lands, the failure is recorded in the modelsdk backend and drained by the page/snippet write entry points, so the statement fails with the actionable message instead of the write succeeding with data missing. ADR-0004 already required this: where the codec path cannot reproduce a construct the backend refuses the op rather than dropping it. Authoring a pluggable nanoflow datasource still needs MXCLI_ENGINE=legacy, which writes it correctly — verified, since the error message says so. Also adds the missing `nanoflow` case to the DataGrid2 and Gallery describe formatters. Reading such a source works after #795, but the formatter had only database/microflow/parameter branches, so a legacy-written or Studio-Pro-authored nanoflow grid still described with no DataSource. Any new write entry point that builds pluggable widgets must drain too; the symptom table records that. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012XR649rKk68z6gBpngu6MA --- .claude/skills/fix-issue.md | 1 + mdl/backend/modelsdk/page_write.go | 12 +++ mdl/backend/modelsdk/snippet_write.go | 14 +++- .../modelsdk/widget_child_error_test.go | 83 +++++++++++++++++++ .../modelsdk/widget_pluggable_write.go | 47 ++++++++++- mdl/executor/cmd_pages_describe_output.go | 4 + 6 files changed, 156 insertions(+), 5 deletions(-) create mode 100644 mdl/backend/modelsdk/widget_child_error_test.go diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index 0a4ff6aa3..d945800e1 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -16,6 +16,7 @@ to the symptom table below, so the next similar issue costs fewer reads. | Symptom | Root cause layer | First file to open | Fix pattern | |---------|-----------------|-------------------|-------------| +| A pluggable widget's datasource (or child widget, or client action) is **silently dropped at write time** — `exec` prints `Created page` but the widget lands with the piece missing, and only a `log` line mentions `not yet supported — rerun with MXCLI_ENGINE=legacy` | The converter *does* return an error, but it travels through `widgetobj.ChildSerializer`, whose methods return BSON with no error channel (the `TODO(shared-types)` in `mdl/backend/widgetobj/builder.go`), so the caller logged it and returned nil | `mdl/backend/modelsdk/widget_pluggable_write.go` (`recordChildSerializeErr`, `takeChildSerializeErr`) + the drains in `page_write.go` / `snippet_write.go` | Record the failure in a package-level accumulator and drain it at every page/snippet write entry point, so the statement fails instead of the write succeeding with data missing (ADR-0004: refuse, don't drop). The real fix is the deferred `ChildSerializer` interface change; until then, any **new** write entry point that builds pluggable widgets must drain too. Repro `mdl-examples/bug-tests/795b-flow-datasource-context-entity.mdl` | | `describe page` omits a **pluggable** widget's `DataSource` when it is bound to a microflow (`datagrid g1 {` with no DataSource), while a `database from` source describes fine — re-applying the output recreates the grid unbound | A `Forms$MicroflowSource` stores the name in the nested `Forms$MicroflowSettings` (`MicroflowSettings` → `Microflow`), which is what the write path and Studio Pro emit; the reader looked up a top-level `Microflow` key, got `""`, and returned no datasource. The describe *formatter* was correct all along — read bug only | `mdl/executor/cmd_pages_describe_pluggable.go` (`microflowSourceRef`, `nanoflowSourceRef`, `extractDataGrid2DataSource`, `extractGalleryDataSource`, `parseCustomWidgetDataSource`) | Read the nested settings with a top-level fallback, via one shared helper — there were four divergent copies of this lookup and two were wrong. `Forms$NanoflowSource` was missing entirely from the DataGrid2/Gallery switches; add it alongside. Do **not** touch `CustomWidgets$CustomWidgetNanoflowSource` (a different metamodel type whose `Nanoflow` really is top-level) or the `Forms$MicroflowAction` reads (actions, not datasources). Repro `mdl-examples/bug-tests/795-datagrid-microflow-datasource-describe.mdl`. Issue #795 | | Any `ALTER SETTINGS` (any section) reports success but the Default configuration's **Custom settings** are gone, **Tracing** is reset, and every **constant override** shows blank in Studio Pro — Integer/Long constants then fail the build | The configuration was re-serialized from `model.ServerConfiguration`, which carries only the modelled fields, so CustomSettings/Tracing/OpenAdminPort/OpenHttpPort were dropped, the list version markers downgraded 3→2, and overrides were written with a flat `Value` instead of the nested `SharedOrPrivateValue` the platform reads | `mdl/settingsoverlay/settingsoverlay.go` (`Configurations`, `ServerConfiguration`, `ConstantValues`) — called by both `mdl/backend/modelsdk/settings_write.go` and `sdk/mpr/writer_settings.go` | Overlay onto the raw document instead of rebuilding: write only the fields the read path populates, take each list's marker from what is stored, and update a constant override in the slot it already occupies (nested if nested, flat if flat). New override → nested. New configuration → clone a sibling's shape, empty its collections, mint a fresh `$ID`. Refuse the write when `RawParts` is empty. Repro `mdl-examples/bug-tests/801-alter-settings-preserves-configuration.mdl`. Issue #801 | | `ALTER SETTINGS` / `CREATE CONFIGURATION` prints "Updated …" but `DESCRIBE SETTINGS` shows the old value — an Integer property was given a non-numeric value, or a Boolean anything other than `true` | `strconv.Atoi`'s error was discarded (`if v, err := …; err == nil`) so the assignment was skipped while the caller still printed success; the boolean form compared against `"true"`, silently mapping every other spelling to false | `mdl/executor/cmd_settings.go` (`settingsInt`, `settingsBool`) + `mdl/executor/validate_settings.go` (`typedSettingsKeys`, MDL-SET01/MDL-SET02) | Parse through a helper that returns a validation error naming the setting and the offending value, and register the property in `typedSettingsKeys` so `mxcli check` and the LSP flag it before the project is opened for writing. `TestTypedSettingsKeys_MatchExecutor` guards the table against drifting from the executor's switch. Repro `mdl-examples/bug-tests/805-alter-settings-typed-values.fail.mdl`. Issue #805 | diff --git a/mdl/backend/modelsdk/page_write.go b/mdl/backend/modelsdk/page_write.go index 412d5649e..c2e47331f 100644 --- a/mdl/backend/modelsdk/page_write.go +++ b/mdl/backend/modelsdk/page_write.go @@ -27,6 +27,12 @@ func init() { // CreatePage inserts a new Forms$Page document unit (header, layout call, the // widget tree, parameters, and variables) via pageToGen. func (b *Backend) CreatePage(page *pages.Page) error { + // A pluggable widget's children are serialized while the executor builds the + // page, before this call; drain any failure so an unsupported construct fails + // the statement instead of silently landing as a widget with the piece missing. + if err := takeChildSerializeErr(); err != nil { + return fmt.Errorf("CreatePage: %w", err) + } if page == nil { return fmt.Errorf("CreatePage: nil page") } @@ -56,6 +62,12 @@ func (b *Backend) CreatePage(page *pages.Page) error { // full page (header + widget tree) and replace the existing unit. Serialization // is identical to CreatePage. func (b *Backend) UpdatePage(page *pages.Page) error { + // A pluggable widget's children are serialized while the executor builds the + // page, before this call; drain any failure so an unsupported construct fails + // the statement instead of silently landing as a widget with the piece missing. + if err := takeChildSerializeErr(); err != nil { + return fmt.Errorf("UpdatePage: %w", err) + } if page == nil { return fmt.Errorf("UpdatePage: nil page") } diff --git a/mdl/backend/modelsdk/snippet_write.go b/mdl/backend/modelsdk/snippet_write.go index bb5fb6c6b..b27be6d7a 100644 --- a/mdl/backend/modelsdk/snippet_write.go +++ b/mdl/backend/modelsdk/snippet_write.go @@ -7,9 +7,9 @@ import ( "github.com/mendixlabs/mxcli/model" "github.com/mendixlabs/mxcli/modelsdk/codec" + "github.com/mendixlabs/mxcli/modelsdk/element" genDT "github.com/mendixlabs/mxcli/modelsdk/gen/datatypes" genPg "github.com/mendixlabs/mxcli/modelsdk/gen/pages" - "github.com/mendixlabs/mxcli/modelsdk/element" mmpr "github.com/mendixlabs/mxcli/modelsdk/mpr" "github.com/mendixlabs/mxcli/sdk/pages" ) @@ -24,6 +24,12 @@ func init() { // 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 { + // A pluggable widget's children are serialized while the executor builds the + // page, before this call; drain any failure so an unsupported construct fails + // the statement instead of silently landing as a widget with the piece missing. + if err := takeChildSerializeErr(); err != nil { + return fmt.Errorf("CreateSnippet: %w", err) + } if snippet == nil { return fmt.Errorf("CreateSnippet: nil snippet") } @@ -50,6 +56,12 @@ func (b *Backend) CreateSnippet(snippet *pages.Snippet) error { // UpdateSnippet rewrites a snippet document (CREATE OR REPLACE). func (b *Backend) UpdateSnippet(snippet *pages.Snippet) error { + // A pluggable widget's children are serialized while the executor builds the + // page, before this call; drain any failure so an unsupported construct fails + // the statement instead of silently landing as a widget with the piece missing. + if err := takeChildSerializeErr(); err != nil { + return fmt.Errorf("UpdateSnippet: %w", err) + } if snippet == nil { return fmt.Errorf("UpdateSnippet: nil snippet") } diff --git a/mdl/backend/modelsdk/widget_child_error_test.go b/mdl/backend/modelsdk/widget_child_error_test.go new file mode 100644 index 000000000..3f01c9252 --- /dev/null +++ b/mdl/backend/modelsdk/widget_child_error_test.go @@ -0,0 +1,83 @@ +// SPDX-License-Identifier: Apache-2.0 + +// A pluggable widget's children are serialized through widgetobj.ChildSerializer, +// which returns BSON with no error channel. A construct the codec engine cannot +// represent — a nanoflow datasource on a DataGrid2, say — was therefore logged and +// dropped, and the write still reported success: `Created page`, with a grid that +// has no data source. ADR-0004 requires the backend to refuse rather than drop. +package modelsdkbackend + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/pages" +) + +func TestChildSerializeErr_RecordedAndDrained(t *testing.T) { + t.Cleanup(func() { _ = takeChildSerializeErr() }) + if err := takeChildSerializeErr(); err != nil { + t.Fatalf("accumulator not empty at start: %v", err) + } + + // A nanoflow datasource is not yet representable by the codec engine. + got := codecChildSerializer{}.SerializeCustomWidgetDataSource( + &pages.NanoflowSource{Nanoflow: "M.GetOrders"}) + if got != nil { + t.Errorf("unsupported datasource serialized to %v, want nil", got) + } + + err := takeChildSerializeErr() + if err == nil { + t.Fatal("failure was not recorded; it would be dropped silently") + } + if !strings.Contains(err.Error(), "NanoflowSource") { + t.Errorf("error does not name the construct: %v", err) + } + + // Draining clears, so the next write is not failed by a stale error. + if err := takeChildSerializeErr(); err != nil { + t.Errorf("accumulator not cleared after drain: %v", err) + } +} + +func TestChildSerializeErr_SupportedSourceRecordsNothing(t *testing.T) { + t.Cleanup(func() { _ = takeChildSerializeErr() }) + _ = takeChildSerializeErr() + + got := codecChildSerializer{}.SerializeCustomWidgetDataSource( + &pages.MicroflowSource{Microflow: "M.GetOrders"}) + if got == nil { + t.Fatal("supported microflow datasource serialized to nil") + } + if err := takeChildSerializeErr(); err != nil { + t.Errorf("supported datasource recorded an error: %v", err) + } +} + +// TestCreatePage_FailsOnDroppedChild pins the drain: a recorded failure must fail +// the statement rather than let the page land with the piece missing. +func TestCreatePage_FailsOnDroppedChild(t *testing.T) { + t.Cleanup(func() { _ = takeChildSerializeErr() }) + proj := copyFixture(t) + b := New() + if err := b.Connect(proj); err != nil { + t.Fatalf("connect: %v", err) + } + t.Cleanup(func() { _ = b.Disconnect() }) + + // Simulate the executor building a widget tree whose child could not be + // serialized, exactly as SerializeCustomWidgetDataSource does above. + codecChildSerializer{}.SerializeCustomWidgetDataSource(&pages.NanoflowSource{Nanoflow: "M.GetOrders"}) + + page := &pages.Page{Name: "DropProbe"} + page.ID = model.ID("") + err := b.CreatePage(page) + if err == nil { + t.Fatal("CreatePage succeeded after a child was dropped") + } + if !strings.Contains(err.Error(), "NanoflowSource") { + t.Errorf("error does not explain what was dropped: %v", err) + } +} diff --git a/mdl/backend/modelsdk/widget_pluggable_write.go b/mdl/backend/modelsdk/widget_pluggable_write.go index 671dc7917..5acb6bb25 100644 --- a/mdl/backend/modelsdk/widget_pluggable_write.go +++ b/mdl/backend/modelsdk/widget_pluggable_write.go @@ -3,8 +3,10 @@ package modelsdkbackend import ( + "errors" "fmt" "log" + "sync" bsonv1 "go.mongodb.org/mongo-driver/bson" bsonv2 "go.mongodb.org/mongo-driver/v2/bson" @@ -96,6 +98,43 @@ func (b *Backend) BuildFilterWidget(spec backend.FilterWidgetSpec, projectPath s }, nil } +// Child-serialization failures cannot be returned: widgetobj.ChildSerializer +// yields BSON only, and making it error-returning is the interface change the +// TODO in mdl/backend/widgetobj/builder.go defers. Until then a failure is +// recorded here and drained by the page/snippet write entry points, so an +// unsupported construct fails the statement instead of being logged and dropped +// while the write still reports success (ADR-0004: refuse, don't drop). +// +// Widget building is synchronous and engines run sequentially — the same +// reasoning that makes widgetobj's package-level serializer hook safe — but the +// mutex keeps this honest if that ever changes. +var ( + childSerializeMu sync.Mutex + childSerializeErrs []error +) + +// recordChildSerializeErr remembers a failure for the next drain. It is also +// logged, since the log line is the only signal for callers that do not drain. +func recordChildSerializeErr(err error) { + log.Printf("modelsdk: %v", err) + childSerializeMu.Lock() + defer childSerializeMu.Unlock() + childSerializeErrs = append(childSerializeErrs, err) +} + +// takeChildSerializeErr returns the accumulated child-serialization failures as +// one error and clears them. A nil return means the widget tree serialized whole. +func takeChildSerializeErr() error { + childSerializeMu.Lock() + defer childSerializeMu.Unlock() + if len(childSerializeErrs) == 0 { + return nil + } + err := errors.Join(childSerializeErrs...) + childSerializeErrs = nil + return err +} + // codecChildSerializer implements widgetobj.ChildSerializer by routing child // content through the modelsdk codec converters, then bridging v2→v1 BSON. type codecChildSerializer struct{} @@ -103,7 +142,7 @@ type codecChildSerializer struct{} func (codecChildSerializer) SerializeWidget(w pages.Widget) bsonv1.D { el, err := widgetToGen(w) if err != nil { - log.Printf("modelsdk: serialize child widget %T: %v", w, err) + recordChildSerializeErr(fmt.Errorf("serialize child widget %T: %w", w, err)) return nil } return genToV1BSON(el) @@ -112,7 +151,7 @@ func (codecChildSerializer) SerializeWidget(w pages.Widget) bsonv1.D { func (codecChildSerializer) SerializeClientAction(a pages.ClientAction) bsonv1.D { el, err := clientActionToGen(a) if err != nil { - log.Printf("modelsdk: serialize client action %T: %v", a, err) + recordChildSerializeErr(fmt.Errorf("serialize client action %T: %w", a, err)) return nil } return genToV1BSON(el) @@ -121,7 +160,7 @@ func (codecChildSerializer) SerializeClientAction(a pages.ClientAction) bsonv1.D func (codecChildSerializer) SerializeCustomWidgetDataSource(ds pages.DataSource) bsonv1.D { el, err := customWidgetDataSourceToGen(ds) if err != nil { - log.Printf("modelsdk: serialize custom widget data source %T: %v", ds, err) + recordChildSerializeErr(fmt.Errorf("serialize custom widget data source %T: %w", ds, err)) return nil } if el == nil { @@ -135,7 +174,7 @@ func (codecChildSerializer) SerializeCustomWidgetDataSource(ds pages.DataSource) func genToV1BSON(el element.Element) bsonv1.D { out, err := (&codec.Encoder{}).Encode(el) if err != nil { - log.Printf("modelsdk: encode child element: %v", err) + recordChildSerializeErr(fmt.Errorf("encode child element %T: %w", el, err)) return nil } var d bsonv1.D diff --git a/mdl/executor/cmd_pages_describe_output.go b/mdl/executor/cmd_pages_describe_output.go index a194e5660..4ed32528c 100644 --- a/mdl/executor/cmd_pages_describe_output.go +++ b/mdl/executor/cmd_pages_describe_output.go @@ -465,6 +465,8 @@ func outputWidgetMDLV3(ctx *ExecContext, w rawWidget, indent int) { props = append(props, fmt.Sprintf("DataSource: %s", dsVal)) case "microflow": props = append(props, fmt.Sprintf("DataSource: microflow %s", w.DataSource.Reference)) + case "nanoflow": + props = append(props, fmt.Sprintf("DataSource: nanoflow %s", w.DataSource.Reference)) case "parameter": props = append(props, fmt.Sprintf("DataSource: %s", w.DataSource.Reference)) } @@ -528,6 +530,8 @@ func outputWidgetMDLV3(ctx *ExecContext, w rawWidget, indent int) { props = append(props, fmt.Sprintf("DataSource: %s", dsVal)) case "microflow": props = append(props, fmt.Sprintf("DataSource: microflow %s", w.DataSource.Reference)) + case "nanoflow": + props = append(props, fmt.Sprintf("DataSource: nanoflow %s", w.DataSource.Reference)) } } // Add column counts if non-default From 6e59e0e3e0b1254efc15c6ab41397cbbfee1a73c Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 14:51:49 +0000 Subject: [PATCH 3/8] fix(pages): describe a flow datasource's context as the entity it returns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A data container bound to a microflow or nanoflow reported the *flow* as its context entity: datagrid g1 (DataSource: microflow Module.GetOrders) { -- Context: $currentObject (Module.GetOrders) <- the flow, not the entity The datasource reference was used verbatim. That is correct for a database source, where the reference is the entity, and wrong for a flow source, where it is the flow's own qualified name. Besides the misleading comment, the context is passed down to the column and child-widget readers, which resolve attributes against it. The flow's return type is resolved instead, taking the entity from an Object or List return. When the flow cannot be resolved, or returns a scalar, the reference is kept — the previous behaviour, so nothing gets worse. Resolution goes through ListMicroflows/ListNanoflows plus the container hierarchy rather than GetRawUnitByName, which the flow builder's fast path uses: that method is unimplemented on the modelsdk engine, which is the default, so the list path is the one that actually resolves. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012XR649rKk68z6gBpngu6MA --- .claude/skills/fix-issue.md | 1 + .../795b-flow-datasource-context-entity.mdl | 73 +++++++++++ .../cmd_pages_describe_flowcontext.go | 84 ++++++++++++ .../cmd_pages_describe_flowcontext_test.go | 121 ++++++++++++++++++ mdl/executor/cmd_pages_describe_parse.go | 10 +- 5 files changed, 284 insertions(+), 5 deletions(-) create mode 100644 mdl-examples/bug-tests/795b-flow-datasource-context-entity.mdl create mode 100644 mdl/executor/cmd_pages_describe_flowcontext.go create mode 100644 mdl/executor/cmd_pages_describe_flowcontext_test.go diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index d945800e1..b00dcb40c 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -17,6 +17,7 @@ to the symptom table below, so the next similar issue costs fewer reads. | Symptom | Root cause layer | First file to open | Fix pattern | |---------|-----------------|-------------------|-------------| | A pluggable widget's datasource (or child widget, or client action) is **silently dropped at write time** — `exec` prints `Created page` but the widget lands with the piece missing, and only a `log` line mentions `not yet supported — rerun with MXCLI_ENGINE=legacy` | The converter *does* return an error, but it travels through `widgetobj.ChildSerializer`, whose methods return BSON with no error channel (the `TODO(shared-types)` in `mdl/backend/widgetobj/builder.go`), so the caller logged it and returned nil | `mdl/backend/modelsdk/widget_pluggable_write.go` (`recordChildSerializeErr`, `takeChildSerializeErr`) + the drains in `page_write.go` / `snippet_write.go` | Record the failure in a package-level accumulator and drain it at every page/snippet write entry point, so the statement fails instead of the write succeeding with data missing (ADR-0004: refuse, don't drop). The real fix is the deferred `ChildSerializer` interface change; until then, any **new** write entry point that builds pluggable widgets must drain too. Repro `mdl-examples/bug-tests/795b-flow-datasource-context-entity.mdl` | +| `describe page` reports the wrong context entity under a data container bound to a microflow/nanoflow — `-- Context: $currentObject (Module.GetOrders)` names the *flow* instead of the entity it returns | `widget.EntityContext = widget.DataSource.Reference` is correct for a database source (reference *is* the entity) and wrong for a flow source (reference is the flow's qualified name) | `mdl/executor/cmd_pages_describe_flowcontext.go` (`dataSourceEntityContext`, `flowReturnEntity`) + the five assignment sites in `cmd_pages_describe_parse.go` | Resolve the flow's return type via `ListMicroflows`/`ListNanoflows` + `getHierarchy().GetQualifiedName`, taking the entity from an Object/List return type; fall back to the reference when the flow is unresolvable or returns a scalar, so the result is never worse than before. Note `GetRawUnitByName` is unimplemented on the modelsdk engine — the list+hierarchy path is the one that works | | `describe page` omits a **pluggable** widget's `DataSource` when it is bound to a microflow (`datagrid g1 {` with no DataSource), while a `database from` source describes fine — re-applying the output recreates the grid unbound | A `Forms$MicroflowSource` stores the name in the nested `Forms$MicroflowSettings` (`MicroflowSettings` → `Microflow`), which is what the write path and Studio Pro emit; the reader looked up a top-level `Microflow` key, got `""`, and returned no datasource. The describe *formatter* was correct all along — read bug only | `mdl/executor/cmd_pages_describe_pluggable.go` (`microflowSourceRef`, `nanoflowSourceRef`, `extractDataGrid2DataSource`, `extractGalleryDataSource`, `parseCustomWidgetDataSource`) | Read the nested settings with a top-level fallback, via one shared helper — there were four divergent copies of this lookup and two were wrong. `Forms$NanoflowSource` was missing entirely from the DataGrid2/Gallery switches; add it alongside. Do **not** touch `CustomWidgets$CustomWidgetNanoflowSource` (a different metamodel type whose `Nanoflow` really is top-level) or the `Forms$MicroflowAction` reads (actions, not datasources). Repro `mdl-examples/bug-tests/795-datagrid-microflow-datasource-describe.mdl`. Issue #795 | | Any `ALTER SETTINGS` (any section) reports success but the Default configuration's **Custom settings** are gone, **Tracing** is reset, and every **constant override** shows blank in Studio Pro — Integer/Long constants then fail the build | The configuration was re-serialized from `model.ServerConfiguration`, which carries only the modelled fields, so CustomSettings/Tracing/OpenAdminPort/OpenHttpPort were dropped, the list version markers downgraded 3→2, and overrides were written with a flat `Value` instead of the nested `SharedOrPrivateValue` the platform reads | `mdl/settingsoverlay/settingsoverlay.go` (`Configurations`, `ServerConfiguration`, `ConstantValues`) — called by both `mdl/backend/modelsdk/settings_write.go` and `sdk/mpr/writer_settings.go` | Overlay onto the raw document instead of rebuilding: write only the fields the read path populates, take each list's marker from what is stored, and update a constant override in the slot it already occupies (nested if nested, flat if flat). New override → nested. New configuration → clone a sibling's shape, empty its collections, mint a fresh `$ID`. Refuse the write when `RawParts` is empty. Repro `mdl-examples/bug-tests/801-alter-settings-preserves-configuration.mdl`. Issue #801 | | `ALTER SETTINGS` / `CREATE CONFIGURATION` prints "Updated …" but `DESCRIBE SETTINGS` shows the old value — an Integer property was given a non-numeric value, or a Boolean anything other than `true` | `strconv.Atoi`'s error was discarded (`if v, err := …; err == nil`) so the assignment was skipped while the caller still printed success; the boolean form compared against `"true"`, silently mapping every other spelling to false | `mdl/executor/cmd_settings.go` (`settingsInt`, `settingsBool`) + `mdl/executor/validate_settings.go` (`typedSettingsKeys`, MDL-SET01/MDL-SET02) | Parse through a helper that returns a validation error naming the setting and the offending value, and register the property in `typedSettingsKeys` so `mxcli check` and the LSP flag it before the project is opened for writing. `TestTypedSettingsKeys_MatchExecutor` guards the table against drifting from the executor's switch. Repro `mdl-examples/bug-tests/805-alter-settings-typed-values.fail.mdl`. Issue #805 | diff --git a/mdl-examples/bug-tests/795b-flow-datasource-context-entity.mdl b/mdl-examples/bug-tests/795b-flow-datasource-context-entity.mdl new file mode 100644 index 000000000..b23618887 --- /dev/null +++ b/mdl-examples/bug-tests/795b-flow-datasource-context-entity.mdl @@ -0,0 +1,73 @@ +-- Two follow-ups to #795, both found while verifying that fix. +-- +-- (1) A nanoflow datasource on a pluggable widget was DROPPED AT WRITE TIME and +-- the statement still reported success. The codec engine cannot represent a +-- Forms$NanoflowSource yet and said so, but the error travelled through +-- widgetobj.ChildSerializer — which returns BSON with no error channel — so +-- it was logged and discarded while `exec` printed `Created page`. The grid +-- landed with no data source at all. +-- +-- Now the failure is recorded and drained by the page/snippet write entry +-- points, so the statement fails with the actionable message instead +-- (ADR-0004: refuse, don't drop). Authoring one still needs +-- `MXCLI_ENGINE=legacy`, which does write it correctly. +-- +-- (2) A data container bound to a microflow or nanoflow reported the FLOW as its +-- context entity: `-- Context: $currentObject (Issue795b.GetBuckets)` rather +-- than the entity the flow returns. The datasource reference was used +-- verbatim, which is right for a database source and wrong for a flow source. +-- The flow's return type is resolved now, falling back to the reference when +-- the flow cannot be resolved or returns a scalar. +-- +-- Manual verification (needs a project): +-- +-- mxcli exec 795b-flow-datasource-context-entity.mdl -p app.mpr +-- mxcli -p app.mpr -c "describe page Issue795b.Probe" +-- +-- Expect `-- Context: $currentObject (Issue795b.Bucket)` under the grid — before +-- the fix it named Issue795b.GetBuckets. For (1), adding a +-- `datasource: nanoflow …` grid must now fail the statement rather than print +-- `Created page`; see the commented-out block at the end. + +create module Issue795b; +create module role Issue795b.User; + +@position(100, 100) +create persistent entity Issue795b.Bucket ( + BucketKey: string(200), + Size: integer +); + +create microflow Issue795b.GetBuckets () +returns list of Issue795b.Bucket as $items +begin + retrieve $items from Issue795b.Bucket; + return $items; +end; + +create or replace page Issue795b.Probe ( + Title: 'Probe', + Layout: Atlas_Core.Atlas_Default +) { + -- The context comment under this grid must name Issue795b.Bucket, not the microflow. + datagrid gMicroflow (datasource: microflow Issue795b.GetBuckets) { + column colKey (attribute: BucketKey, caption: 'Key') + } +} + +describe page Issue795b.Probe; + +-- (1) is a negative case and cannot live in a script that must pass `mxcli exec`. +-- To reproduce by hand, add a nanoflow returning a list of Issue795b.Bucket and: +-- +-- create or replace page Issue795b.NanoProbe ( +-- Title: 'Nano', Layout: Atlas_Core.Atlas_Default +-- ) { +-- datagrid gNano (datasource: nanoflow Issue795b.GetBucketsClient) { +-- column colKey (attribute: BucketKey, caption: 'Key') +-- } +-- } +-- +-- Before the fix: `Created page`, grid with no data source. +-- After: the statement fails, naming *pages.NanoflowSource and the +-- MXCLI_ENGINE=legacy workaround. diff --git a/mdl/executor/cmd_pages_describe_flowcontext.go b/mdl/executor/cmd_pages_describe_flowcontext.go new file mode 100644 index 000000000..73547933d --- /dev/null +++ b/mdl/executor/cmd_pages_describe_flowcontext.go @@ -0,0 +1,84 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// dataSourceEntityContext returns the entity a datasource puts in scope for the +// widgets inside it. +// +// For a database or association source the datasource reference *is* the entity. +// For a microflow or nanoflow source it is the flow's own qualified name, so using +// it directly named the flow as the context entity — `-- Context: $currentObject +// (Module.GetOrders)` instead of `(Module.Order)` — and any consumer that resolves +// attributes against the context was looking up the wrong element. The flow's +// return type is resolved instead, falling back to the reference when the flow +// cannot be found or does not return an object/list, which is what the caller +// used before. +func dataSourceEntityContext(ctx *ExecContext, ds *rawDataSource) string { + if ds == nil || ds.Reference == "" { + return "" + } + switch ds.Type { + case "microflow", "nanoflow": + if entity := flowReturnEntity(ctx, ds.Type, ds.Reference); entity != "" { + return entity + } + } + return ds.Reference +} + +// flowReturnEntity resolves the entity a microflow or nanoflow returns, by object +// or list return type. Returns "" when the project is unavailable, the flow is not +// found, or it returns something other than an object/list. +func flowReturnEntity(ctx *ExecContext, kind, qualifiedName string) string { + if ctx == nil || ctx.Backend == nil || qualifiedName == "" { + return "" + } + h, err := getHierarchy(ctx) + if err != nil || h == nil { + return "" + } + switch kind { + case "microflow": + mfs, err := ctx.Backend.ListMicroflows() + if err != nil { + return "" + } + for _, mf := range mfs { + if mf != nil && h.GetQualifiedName(mf.ContainerID, mf.Name) == qualifiedName { + return dataTypeEntity(mf.ReturnType) + } + } + case "nanoflow": + nfs, err := ctx.Backend.ListNanoflows() + if err != nil { + return "" + } + for _, nf := range nfs { + if nf != nil && h.GetQualifiedName(nf.ContainerID, nf.Name) == qualifiedName { + return dataTypeEntity(nf.ReturnType) + } + } + } + return "" +} + +// dataTypeEntity returns the entity a return type refers to, for the two kinds a +// data container can bind against. Both the pointer and value forms are matched +// because the parsers are not consistent about which they produce. +func dataTypeEntity(dt microflows.DataType) string { + switch t := dt.(type) { + case *microflows.ObjectType: + return t.EntityQualifiedName + case microflows.ObjectType: + return t.EntityQualifiedName + case *microflows.ListType: + return t.EntityQualifiedName + case microflows.ListType: + return t.EntityQualifiedName + } + return "" +} diff --git a/mdl/executor/cmd_pages_describe_flowcontext_test.go b/mdl/executor/cmd_pages_describe_flowcontext_test.go new file mode 100644 index 000000000..9fcd9fba9 --- /dev/null +++ b/mdl/executor/cmd_pages_describe_flowcontext_test.go @@ -0,0 +1,121 @@ +// SPDX-License-Identifier: Apache-2.0 + +// A data container bound to a microflow or nanoflow reported the *flow* as its +// context entity — `-- Context: $currentObject (Module.GetOrders)` instead of +// `(Module.Order)` — because the datasource reference was used verbatim. That is +// correct for a database source, where the reference is the entity, and wrong for +// a flow source, where it is the flow's own name. +package executor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/backend/mock" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +func TestDataTypeEntity(t *testing.T) { + tests := []struct { + name string + in microflows.DataType + want string + }{ + {"object pointer", µflows.ObjectType{EntityQualifiedName: "M.Order"}, "M.Order"}, + {"object value", microflows.ObjectType{EntityQualifiedName: "M.Order"}, "M.Order"}, + {"list pointer", µflows.ListType{EntityQualifiedName: "M.Order"}, "M.Order"}, + {"list value", microflows.ListType{EntityQualifiedName: "M.Order"}, "M.Order"}, + {"boolean", µflows.BooleanType{}, ""}, + {"nil", nil, ""}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := dataTypeEntity(tc.in); got != tc.want { + t.Errorf("dataTypeEntity = %q, want %q", got, tc.want) + } + }) + } +} + +// TestDataSourceEntityContext_NonFlowSources: a database or association source +// already names its entity, so it must pass straight through. +func TestDataSourceEntityContext_NonFlowSources(t *testing.T) { + for _, dsType := range []string{"database", "association", "parameter"} { + t.Run(dsType, func(t *testing.T) { + ds := &rawDataSource{Type: dsType, Reference: "M.Order"} + if got := dataSourceEntityContext(nil, ds); got != "M.Order" { + t.Errorf("dataSourceEntityContext = %q, want M.Order", got) + } + }) + } + if got := dataSourceEntityContext(nil, nil); got != "" { + t.Errorf("nil datasource = %q, want empty", got) + } + if got := dataSourceEntityContext(nil, &rawDataSource{Type: "database"}); got != "" { + t.Errorf("empty reference = %q, want empty", got) + } +} + +// TestDataSourceEntityContext_ResolvesFlowReturnEntity is the fix: the flow's +// return entity, not the flow's name. +func TestDataSourceEntityContext_ResolvesFlowReturnEntity(t *testing.T) { + mod := &model.Module{BaseElement: model.BaseElement{ID: nextID("mod")}, Name: "M"} + h := mkHierarchy(mod) + + mf := µflows.Microflow{ + ContainerID: mod.ID, + Name: "GetOrders", + ReturnType: µflows.ListType{EntityQualifiedName: "M.Order"}, + } + nf := µflows.Nanoflow{ + ContainerID: mod.ID, + Name: "GetOrderClient", + ReturnType: µflows.ObjectType{EntityQualifiedName: "M.Order"}, + } + // A flow that returns something a data container cannot bind against. + scalar := µflows.Microflow{ + ContainerID: mod.ID, + Name: "CountOrders", + ReturnType: µflows.IntegerType{}, + } + + mb := &mock.MockBackend{ + IsConnectedFunc: func() bool { return true }, + ListModulesFunc: func() ([]*model.Module, error) { return []*model.Module{mod}, nil }, + ListMicroflowsFunc: func() ([]*microflows.Microflow, error) { + return []*microflows.Microflow{mf, scalar}, nil + }, + ListNanoflowsFunc: func() ([]*microflows.Nanoflow, error) { + return []*microflows.Nanoflow{nf}, nil + }, + } + ctx, _ := newMockCtx(t, withBackend(mb), withHierarchy(h)) + + tests := []struct { + name string + ds *rawDataSource + want string + }{ + {"microflow list", &rawDataSource{Type: "microflow", Reference: "M.GetOrders"}, "M.Order"}, + {"nanoflow object", &rawDataSource{Type: "nanoflow", Reference: "M.GetOrderClient"}, "M.Order"}, + // Unresolvable cases keep the reference — no worse than before the fix. + {"scalar return", &rawDataSource{Type: "microflow", Reference: "M.CountOrders"}, "M.CountOrders"}, + {"unknown flow", &rawDataSource{Type: "microflow", Reference: "M.Missing"}, "M.Missing"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := dataSourceEntityContext(ctx, tc.ds); got != tc.want { + t.Errorf("dataSourceEntityContext = %q, want %q", got, tc.want) + } + }) + } +} + +// TestDataSourceEntityContext_NoProject: with no backend the reference is kept +// rather than the context being blanked. +func TestDataSourceEntityContext_NoProject(t *testing.T) { + ds := &rawDataSource{Type: "microflow", Reference: "M.GetOrders"} + if got := dataSourceEntityContext(nil, ds); got != "M.GetOrders" { + t.Errorf("dataSourceEntityContext = %q, want the reference kept", got) + } +} diff --git a/mdl/executor/cmd_pages_describe_parse.go b/mdl/executor/cmd_pages_describe_parse.go index 1930f8024..cf548990c 100644 --- a/mdl/executor/cmd_pages_describe_parse.go +++ b/mdl/executor/cmd_pages_describe_parse.go @@ -248,7 +248,7 @@ func parseRawWidget(ctx *ExecContext, w map[string]any, parentEntityContext ...s case "Forms$DataView", "Pages$DataView": widget.DataSource = extractDataViewDataSource(ctx, w) if widget.DataSource != nil && widget.DataSource.Reference != "" { - widget.EntityContext = widget.DataSource.Reference + widget.EntityContext = dataSourceEntityContext(ctx, widget.DataSource) } else if inheritedCtx != "" { widget.EntityContext = inheritedCtx } @@ -312,7 +312,7 @@ func parseRawWidget(ctx *ExecContext, w map[string]any, parentEntityContext ...s // showNumberOfRows: not yet fully supported in DataGrid2, skip to avoid CE0463 widget.Selection = extractGallerySelection(ctx, w) if widget.DataSource != nil && widget.DataSource.Reference != "" { - widget.EntityContext = widget.DataSource.Reference + widget.EntityContext = dataSourceEntityContext(ctx, widget.DataSource) } else if inheritedCtx != "" { widget.EntityContext = inheritedCtx } @@ -330,7 +330,7 @@ func parseRawWidget(ctx *ExecContext, w map[string]any, parentEntityContext ...s widget.TabletColumns = extractCustomWidgetPropertyString(ctx, w, "tabletItems") widget.PhoneColumns = extractCustomWidgetPropertyString(ctx, w, "phoneItems") if widget.DataSource != nil && widget.DataSource.Reference != "" { - widget.EntityContext = widget.DataSource.Reference + widget.EntityContext = dataSourceEntityContext(ctx, widget.DataSource) } else if inheritedCtx != "" { widget.EntityContext = inheritedCtx } @@ -369,7 +369,7 @@ func parseRawWidget(ctx *ExecContext, w map[string]any, parentEntityContext ...s case "Forms$Gallery", "Pages$Gallery": widget.DataSource = extractGalleryDataSource(ctx, w) if widget.DataSource != nil && widget.DataSource.Reference != "" { - widget.EntityContext = widget.DataSource.Reference + widget.EntityContext = dataSourceEntityContext(ctx, widget.DataSource) } else if inheritedCtx != "" { widget.EntityContext = inheritedCtx } @@ -383,7 +383,7 @@ func parseRawWidget(ctx *ExecContext, w map[string]any, parentEntityContext ...s case "Forms$ListView", "Pages$ListView": widget.DataSource = extractListViewDataSource(ctx, w) if widget.DataSource != nil && widget.DataSource.Reference != "" { - widget.EntityContext = widget.DataSource.Reference + widget.EntityContext = dataSourceEntityContext(ctx, widget.DataSource) } else if inheritedCtx != "" { widget.EntityContext = inheritedCtx } From 48feadb98594a4e440674c87fab6216a80f0b770 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 15:19:38 +0000 Subject: [PATCH 4/8] fix(domainmodel): read an external entity's OData source (#782) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `alter entity X set allow_create_change_locally = true` reported success and left the flag off. So did every other remote setting, and `describe external entity` rejected the entity outright with "is not an external entity (source: )". The write path was never the problem: externalEntitySourceToGen switches on e.Source and handles all three Rest$OData* flavours. The *read* filled none of them — entityFromGen recognised only DomainModels$OqlViewEntitySource, so an external entity came back with an empty Source and every remote field zeroed. Setting a flag then wrote it onto a model that no longer knew the entity was external, and the update rebuilt the entity without its source at all. The legacy engine parses all three flavours (sdk/mpr/parser_domainmodel.go), so this was a modelsdk-engine gap — and modelsdk is the default. Under `--engine legacy` the same commands worked, which is a useful bisect signal. Now mirrored for: - Rest$ODataRemoteEntitySource — service, entity set, remote name, the capability flags, CreateChangeLocally and the remote key - Rest$ODataEntityTypeSource — service, type name, IsOpen and the remote key - Rest$ODataPrimitiveCollectionEntitySource — service Updatable is deliberately left zero: the storage type has no such field (updatability is per attribute via Rest$ODataMappedValue), there is no gen accessor, and the writer does not emit one, so read and write stay symmetric. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012XR649rKk68z6gBpngu6MA --- .claude/skills/fix-issue.md | 1 + ...-external-entity-create-change-locally.mdl | 59 +++++ mdl/backend/modelsdk/domainmodel.go | 73 +++++- .../modelsdk/external_entity_read_test.go | 229 ++++++++++++++++++ 4 files changed, 358 insertions(+), 4 deletions(-) create mode 100644 mdl-examples/bug-tests/782-external-entity-create-change-locally.mdl create mode 100644 mdl/backend/modelsdk/external_entity_read_test.go diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index ec0c4bd03..177fde49b 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -16,6 +16,7 @@ to the symptom table below, so the next similar issue costs fewer reads. | Symptom | Root cause layer | First file to open | Fix pattern | |---------|-----------------|-------------------|-------------| +| An **external (OData) entity** loses its remote settings on any read-modify-write — `describe external entity` says `is not an external entity (source: )`, and `alter entity … set allow_create_change_locally = true` reports success but the flag stays off. Works under `--engine legacy` | `entityFromGen` recognised only `DomainModels$OqlViewEntitySource`, so the three `Rest$OData*` sources read back as no source at all: `Source` empty, every remote field zeroed. The write path was fine — it switches on `e.Source`, which the read never populated | `mdl/backend/modelsdk/domainmodel.go` (`entityFromGen`'s source switch, `odataKeyFromGen`) | Mirror the legacy parser (`sdk/mpr/parser_domainmodel.go`) for all three flavours: RemoteEntitySource (capabilities + CreateChangeLocally + key), EntityTypeSource (type name + IsOpen + key), PrimitiveCollectionEntitySource (service only). `Updatable` has no gen accessor and the writer does not emit it — leaving it zero is symmetric. **Check the read side first when a write-path field "does not stick"**: a switch on a field the read never fills looks like a write bug. Repro `mdl-examples/bug-tests/782-external-entity-create-change-locally.mdl`. Issue #782 | | Any `ALTER SETTINGS` (any section) reports success but the Default configuration's **Custom settings** are gone, **Tracing** is reset, and every **constant override** shows blank in Studio Pro — Integer/Long constants then fail the build | The configuration was re-serialized from `model.ServerConfiguration`, which carries only the modelled fields, so CustomSettings/Tracing/OpenAdminPort/OpenHttpPort were dropped, the list version markers downgraded 3→2, and overrides were written with a flat `Value` instead of the nested `SharedOrPrivateValue` the platform reads | `mdl/settingsoverlay/settingsoverlay.go` (`Configurations`, `ServerConfiguration`, `ConstantValues`) — called by both `mdl/backend/modelsdk/settings_write.go` and `sdk/mpr/writer_settings.go` | Overlay onto the raw document instead of rebuilding: write only the fields the read path populates, take each list's marker from what is stored, and update a constant override in the slot it already occupies (nested if nested, flat if flat). New override → nested. New configuration → clone a sibling's shape, empty its collections, mint a fresh `$ID`. Refuse the write when `RawParts` is empty. Repro `mdl-examples/bug-tests/801-alter-settings-preserves-configuration.mdl`. Issue #801 | | `ALTER SETTINGS` / `CREATE CONFIGURATION` prints "Updated …" but `DESCRIBE SETTINGS` shows the old value — an Integer property was given a non-numeric value, or a Boolean anything other than `true` | `strconv.Atoi`'s error was discarded (`if v, err := …; err == nil`) so the assignment was skipped while the caller still printed success; the boolean form compared against `"true"`, silently mapping every other spelling to false | `mdl/executor/cmd_settings.go` (`settingsInt`, `settingsBool`) + `mdl/executor/validate_settings.go` (`typedSettingsKeys`, MDL-SET01/MDL-SET02) | Parse through a helper that returns a validation error naming the setting and the offending value, and register the property in `typedSettingsKeys` so `mxcli check` and the LSP flag it before the project is opened for writing. `TestTypedSettingsKeys_MatchExecutor` guards the table against drifting from the executor's switch. Repro `mdl-examples/bug-tests/805-alter-settings-typed-values.fail.mdl`. Issue #805 | | MCP op reports `MCP error -32000: Request timed out` but the page/document/entity EXISTS in Studio Pro afterwards | Studio Pro's ~30s server-side per-call limit fires while the op still applies — a client false failure, not a server rejection | `mdl/backend/mcp/timeout.go` (`isTimeoutErr`, `timeoutVerifyDelay`, `pedUpdateVerify`, `pedDocumentExists`) | Verify-on-timeout, never blind-retry a non-idempotent op: idempotent root-replace (`pgWritePage`) retries once; creates confirm via `ped_find_document`; entity adds confirm via a shallow `/entities` read. Unverified → error with save-before-re-run guidance (a blind re-run from a fresh session duplicates elements) | diff --git a/mdl-examples/bug-tests/782-external-entity-create-change-locally.mdl b/mdl-examples/bug-tests/782-external-entity-create-change-locally.mdl new file mode 100644 index 000000000..1a89e43b0 --- /dev/null +++ b/mdl-examples/bug-tests/782-external-entity-create-change-locally.mdl @@ -0,0 +1,59 @@ +-- Bug #782: External Entities — allow_create_change_locally doesn't work +-- +-- Symptom: after importing entities from a consumed OData service, enabling +-- "Allow creating and changing objects locally" left the flag at false. +-- +-- Two independent causes, both fixed: +-- +-- (1) READ. entityFromGen (mdl/backend/modelsdk/domainmodel.go) recognised only +-- DomainModels$OqlViewEntitySource, so an OData external entity read back +-- with an empty Source and every remote field zeroed. `describe external +-- entity` rejected it outright ("is not an external entity (source: )"), and +-- `alter entity … set allow_create_change_locally = true` wrote the flag onto +-- a model that no longer knew the entity was external, so it was lost. The +-- legacy engine parsed all three OData source flavours, so this was a +-- modelsdk-engine gap — and modelsdk is the default engine. +-- +-- (2) RE-IMPORT. applyExternalEntityFields (mdl/executor/cmd_contract.go) stamped +-- CreateChangeLocally = false on every import, including updates of existing +-- entities, so `create or modify external entities` reset whatever the user +-- had set. Unlike Countable/Creatable/Deletable it cannot be derived from the +-- service contract — it is a local modelling choice — so it is now left alone +-- on the top-level branch. +-- +-- Manual verification (needs a project; the metadata file ships with the repo): +-- +-- cp mdl-examples/odata-local-metadata/sample-metadata.xml /path/to/app/ +-- mxcli exec 782-external-entity-create-change-locally.mdl -p app.mpr +-- +-- Expect the final DESCRIBE to report `AllowCreateChangeLocally: Yes`. Before the +-- fix the first DESCRIBE errored with "is not an external entity", and after the +-- re-import the flag was back to No. +-- +-- Adjust MetadataUrl below to wherever sample-metadata.xml sits. + +create module Issue782; +create module role Issue782.User; + +create constant Issue782.SvcUrl + type string + default 'https://services.odata.org/V4/Northwind/Northwind.svc/'; + +create odata client Issue782.Sample ( + MetadataUrl: './mdl-examples/odata-local-metadata/sample-metadata.xml', + ServiceUrl: '@Issue782.SvcUrl' +); + +create external entities from Issue782.Sample into Issue782; + +-- (1) The entity must read back as external at all. +describe external entity Issue782.Products; + +-- The reported flow: turn the flag on, and it must stick. +alter entity Issue782.Products set allow_create_change_locally = true; +describe external entity Issue782.Products; + +-- (2) A re-import refreshes the contract-derived capabilities but must not reset +-- the local-changes flag. +create or modify external entities from Issue782.Sample into Issue782; +describe external entity Issue782.Products; diff --git a/mdl/backend/modelsdk/domainmodel.go b/mdl/backend/modelsdk/domainmodel.go index 1635f8449..a8d61fa80 100644 --- a/mdl/backend/modelsdk/domainmodel.go +++ b/mdl/backend/modelsdk/domainmodel.go @@ -7,6 +7,7 @@ import ( "github.com/mendixlabs/mxcli/modelsdk/element" genDm "github.com/mendixlabs/mxcli/modelsdk/gen/domainmodels" + genRest "github.com/mendixlabs/mxcli/modelsdk/gen/rest" genTexts "github.com/mendixlabs/mxcli/modelsdk/gen/texts" "github.com/mendixlabs/mxcli/modelsdk/mprread" @@ -160,14 +161,52 @@ func entityFromGen(e *genDm.Entity) *domainmodel.Entity { out.Location = parseLocation(e.Location()) - // View entities carry an OqlViewEntitySource referencing their source - // document by qualified name; surface it so read-modify-write paths (e.g. - // MOVE ENTITY, which must reparent the source doc) can see it. - if src, ok := e.Source().(*genDm.OqlViewEntitySource); ok { + // An entity's Source says where its data comes from. Surface each flavour so + // read-modify-write paths (MOVE ENTITY reparenting a source doc, ALTER ENTITY + // flipping a remote capability) can see it — without this an external entity + // reads back looking local, DESCRIBE EXTERNAL ENTITY rejects it, and an update + // rebuilds it with no source at all (mendixlabs/mxcli#782). Mirrors the legacy + // parser in sdk/mpr/parser_domainmodel.go. + switch src := e.Source().(type) { + case *genDm.OqlViewEntitySource: out.Source = "DomainModels$OqlViewEntitySource" out.SourceObjectID = model.ID(src.ID()) out.SourceDocumentRef = src.SourceDocumentQualifiedName() out.OqlQuery = src.Oql() + + case *genRest.ODataRemoteEntitySource: + // Top-level external entity: has its own entity set, so it carries the + // CRUD/paging capabilities and the local-changes flag. + out.Source = "Rest$ODataRemoteEntitySource" + out.SourceObjectID = model.ID(src.ID()) + out.RemoteServiceName = src.SourceDocumentQualifiedName() + out.RemoteEntitySet = src.EntitySet() + out.RemoteEntityName = src.RemoteName() + out.Countable = src.Countable() + out.Creatable = src.Creatable() + out.Deletable = src.Deletable() + out.SkipSupported = src.SkipSupported() + out.TopSupported = src.TopSupported() + out.CreateChangeLocally = src.CreateChangeLocally() + out.RemoteKeyParts = odataKeyFromGen(src.Key()) + // Updatable has no gen accessor because the storage type has no such + // field — updatability is per attribute (Rest$ODataMappedValue). The + // write path does not emit it either, so leaving it zero is symmetric. + + case *genRest.ODataEntityTypeSource: + // Derived / abstract / contained target type: no entity set, no capabilities. + out.Source = "Rest$ODataEntityTypeSource" + out.SourceObjectID = model.ID(src.ID()) + out.RemoteServiceName = src.SourceDocumentQualifiedName() + out.RemoteEntityName = src.EntityTypeName() + out.IsOpen = src.IsOpen() + out.RemoteKeyParts = odataKeyFromGen(src.Key()) + + case *genRest.ODataPrimitiveCollectionEntitySource: + // NPE generated for a Collection(Edm.*) property; carries only the service. + out.Source = "Rest$ODataPrimitiveCollectionEntitySource" + out.SourceObjectID = model.ID(src.ID()) + out.RemoteServiceName = src.SourceDocumentQualifiedName() } for _, el := range e.AttributesItems() { @@ -341,6 +380,32 @@ func indexFromGen(idx *genDm.Index) *domainmodel.Index { // attributeTypeFromGen is the reverse of attributeTypeToGen: a gen attribute-type // element back to a domainmodel.AttributeType (with Length / enumeration ref). +// odataKeyFromGen converts a Rest$ODataKey to the semantic remote-key parts. The +// inverse of odataKeyToGen in domainmodel_write.go. +func odataKeyFromGen(key element.Element) []*domainmodel.RemoteKeyPart { + k, ok := key.(*genRest.ODataKey) + if !ok || k == nil { + return nil + } + var parts []*domainmodel.RemoteKeyPart + for _, el := range k.PartsItems() { + p, ok := el.(*genRest.ODataKeyPart) + if !ok { + continue + } + kp := &domainmodel.RemoteKeyPart{ + Name: p.EntityKeyPartName(), + RemoteName: p.Name(), + RemoteType: p.RemoteType(), + } + if t := p.Type(); t != nil { + kp.Type = attributeTypeFromGen(t) + } + parts = append(parts, kp) + } + return parts +} + func attributeTypeFromGen(t element.Element) domainmodel.AttributeType { switch at := t.(type) { case *genDm.StringAttributeType: diff --git a/mdl/backend/modelsdk/external_entity_read_test.go b/mdl/backend/modelsdk/external_entity_read_test.go new file mode 100644 index 000000000..556773014 --- /dev/null +++ b/mdl/backend/modelsdk/external_entity_read_test.go @@ -0,0 +1,229 @@ +// SPDX-License-Identifier: Apache-2.0 + +// mendixlabs/mxcli#782: an external entity's `allow_create_change_locally` flag +// would not stick. The write path handled it; the *read* did not. entityFromGen +// recognised only DomainModels$OqlViewEntitySource, so an OData external entity +// came back with an empty Source and every remote field zeroed. Setting the flag +// then wrote it onto a model that no longer knew it was external, and the value +// was lost — as were describe, and any other read-modify-write on such an entity. +// +// The legacy engine (sdk/mpr/parser_domainmodel.go) parsed all three OData source +// flavours, so this was a modelsdk-engine gap, not a missing feature. +package modelsdkbackend + +import ( + "testing" + + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/domainmodel" +) + +// externalEntityFixture creates an OData external entity through the write path, +// then reads the domain model back with a fresh connection. Round-tripping through +// disk is the point: the read is what #782 broke. +func externalEntityFixture(t *testing.T, mutate func(*domainmodel.Entity)) (proj string, moduleID model.ID) { + t.Helper() + proj = copyFixture(t) + + b := New() + if err := b.Connect(proj); err != nil { + t.Fatalf("connect: %v", err) + } + mod, err := b.GetModuleByName("MyFirstModule") + if err != nil || mod == nil { + t.Fatalf("GetModuleByName: %v, %v", mod, err) + } + dm, err := b.GetDomainModel(mod.ID) + if err != nil || dm == nil { + t.Fatalf("GetDomainModel: %v, %v", dm, err) + } + + ent := &domainmodel.Entity{ + Name: "Products", + Persistable: true, + Source: "Rest$ODataRemoteEntitySource", + RemoteServiceName: "MyFirstModule.Sample", + RemoteEntitySet: "Products", + RemoteEntityName: "Product", + Countable: true, + Creatable: true, + Deletable: true, + SkipSupported: true, + TopSupported: true, + CreateChangeLocally: true, + RemoteKeyParts: []*domainmodel.RemoteKeyPart{{ + Name: "ProductID", + RemoteName: "ProductID", + RemoteType: "Edm.Int32", + Type: &domainmodel.IntegerAttributeType{}, + }}, + Attributes: []*domainmodel.Attribute{ + {Name: "ProductName", Type: &domainmodel.StringAttributeType{}}, + }, + } + if mutate != nil { + mutate(ent) + } + if err := b.CreateEntity(dm.ID, ent); err != nil { + t.Fatalf("CreateEntity: %v", err) + } + if err := b.Disconnect(); err != nil { + t.Fatalf("disconnect: %v", err) + } + return proj, mod.ID +} + +func readEntity(t *testing.T, proj string, moduleID model.ID, name string) *domainmodel.Entity { + t.Helper() + b := New() + if err := b.Connect(proj); err != nil { + t.Fatalf("reconnect: %v", err) + } + t.Cleanup(func() { _ = b.Disconnect() }) + dm, err := b.GetDomainModel(moduleID) + if err != nil || dm == nil { + t.Fatalf("GetDomainModel: %v, %v", dm, err) + } + for _, e := range dm.Entities { + if e.Name == name { + return e + } + } + t.Fatalf("entity %s not found after write", name) + return nil +} + +// TestExternalEntity_RemoteSourceRoundTrip is the regression test: every field of +// a Rest$ODataRemoteEntitySource must survive a write→read cycle. +func TestExternalEntity_RemoteSourceRoundTrip(t *testing.T) { + proj, modID := externalEntityFixture(t, nil) + got := readEntity(t, proj, modID, "Products") + + if got.Source != "Rest$ODataRemoteEntitySource" { + t.Fatalf("Source = %q, want Rest$ODataRemoteEntitySource — the entity does not read back as external", got.Source) + } + if !got.CreateChangeLocally { + t.Error("CreateChangeLocally = false, want true (#782)") + } + if got.RemoteServiceName != "MyFirstModule.Sample" { + t.Errorf("RemoteServiceName = %q", got.RemoteServiceName) + } + if got.RemoteEntitySet != "Products" { + t.Errorf("RemoteEntitySet = %q", got.RemoteEntitySet) + } + if got.RemoteEntityName != "Product" { + t.Errorf("RemoteEntityName = %q", got.RemoteEntityName) + } + if !got.Countable || !got.Creatable || !got.Deletable || !got.SkipSupported || !got.TopSupported { + t.Errorf("capability flags lost: countable=%v creatable=%v deletable=%v skip=%v top=%v", + got.Countable, got.Creatable, got.Deletable, got.SkipSupported, got.TopSupported) + } + if got.SourceObjectID == "" { + t.Error("SourceObjectID empty — a read-modify-write cannot preserve the source element") + } + if len(got.RemoteKeyParts) != 1 { + t.Fatalf("RemoteKeyParts = %+v, want 1 part", got.RemoteKeyParts) + } + kp := got.RemoteKeyParts[0] + if kp.Name != "ProductID" || kp.RemoteName != "ProductID" || kp.RemoteType != "Edm.Int32" { + t.Errorf("key part = %+v", kp) + } + if _, ok := kp.Type.(*domainmodel.IntegerAttributeType); !ok { + t.Errorf("key part type = %T, want *IntegerAttributeType", kp.Type) + } +} + +// TestExternalEntity_FlagSurvivesReadModifyWrite reproduces the reported flow: +// read the entity, flip the flag (what ALTER ENTITY … SET +// ALLOW_CREATE_CHANGE_LOCALLY does), write it back, read again. +func TestExternalEntity_FlagSurvivesReadModifyWrite(t *testing.T) { + proj, modID := externalEntityFixture(t, func(e *domainmodel.Entity) { + e.CreateChangeLocally = false // imported off, as CREATE EXTERNAL ENTITIES leaves it + }) + + b := New() + if err := b.Connect(proj); err != nil { + t.Fatalf("connect: %v", err) + } + dm, err := b.GetDomainModel(modID) + if err != nil { + t.Fatalf("GetDomainModel: %v", err) + } + var ent *domainmodel.Entity + for _, e := range dm.Entities { + if e.Name == "Products" { + ent = e + } + } + if ent == nil { + t.Fatal("Products not found") + } + ent.CreateChangeLocally = true + if err := b.UpdateEntity(dm.ID, ent); err != nil { + t.Fatalf("UpdateEntity: %v", err) + } + if err := b.Disconnect(); err != nil { + t.Fatalf("disconnect: %v", err) + } + + got := readEntity(t, proj, modID, "Products") + if !got.CreateChangeLocally { + t.Error("CreateChangeLocally = false after setting it to true (#782)") + } + // The rest of the source must not have been damaged by the update. + if got.Source != "Rest$ODataRemoteEntitySource" || got.RemoteEntitySet != "Products" { + t.Errorf("source damaged by the update: Source=%q EntitySet=%q", got.Source, got.RemoteEntitySet) + } + if len(got.RemoteKeyParts) != 1 { + t.Errorf("remote key lost by the update: %+v", got.RemoteKeyParts) + } +} + +// TestExternalEntity_EntityTypeSourceRoundTrip covers the second flavour: a +// derived/abstract/contained type, which has no entity set. +func TestExternalEntity_EntityTypeSourceRoundTrip(t *testing.T) { + proj, modID := externalEntityFixture(t, func(e *domainmodel.Entity) { + e.Name = "ProductDetail" + e.Source = "Rest$ODataEntityTypeSource" + e.Persistable = false + e.IsOpen = true + e.RemoteEntitySet = "" + }) + got := readEntity(t, proj, modID, "ProductDetail") + + if got.Source != "Rest$ODataEntityTypeSource" { + t.Fatalf("Source = %q, want Rest$ODataEntityTypeSource", got.Source) + } + if got.RemoteEntityName != "Product" { + t.Errorf("RemoteEntityName = %q, want Product", got.RemoteEntityName) + } + if !got.IsOpen { + t.Error("IsOpen = false, want true") + } + if got.RemoteServiceName != "MyFirstModule.Sample" { + t.Errorf("RemoteServiceName = %q", got.RemoteServiceName) + } + if len(got.RemoteKeyParts) != 1 { + t.Errorf("RemoteKeyParts = %+v, want 1 part", got.RemoteKeyParts) + } +} + +// TestExternalEntity_PrimitiveCollectionSourceRoundTrip covers the third flavour, +// the NPE generated for a Collection(Edm.*) property. +func TestExternalEntity_PrimitiveCollectionSourceRoundTrip(t *testing.T) { + proj, modID := externalEntityFixture(t, func(e *domainmodel.Entity) { + e.Name = "ProductTag" + e.Source = "Rest$ODataPrimitiveCollectionEntitySource" + e.Persistable = false + e.RemoteEntitySet = "" + e.RemoteKeyParts = nil + }) + got := readEntity(t, proj, modID, "ProductTag") + + if got.Source != "Rest$ODataPrimitiveCollectionEntitySource" { + t.Fatalf("Source = %q, want Rest$ODataPrimitiveCollectionEntitySource", got.Source) + } + if got.RemoteServiceName != "MyFirstModule.Sample" { + t.Errorf("RemoteServiceName = %q", got.RemoteServiceName) + } +} From 830263a2a724ef4f1a17ac1a587303b6b87f0c26 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 15:19:39 +0000 Subject: [PATCH 5/8] fix(odata): keep allow-create-change-locally across an external-entity re-import MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `create or modify external entities` reset "Allow creating and changing objects locally" to false on every run, so the setting survived only until the next import — the other half of #782. applyExternalEntityFields stamps every field on both the create and the update path. That is right for the capability flags (Countable / Creatable / Deletable / SkipSupported / TopSupported), which are derived from the service's capability annotations and should be refreshed from the metadata. It is wrong for CreateChangeLocally, which no OData contract describes: it is a local modelling choice, so the top-level branch now leaves it alone. A newly imported entity arrives zero-valued, which is Mendix's default. The entity-type branch still clears it, since that storage type has no such field — a re-import that reclassifies an entity as a derived type would otherwise leave a stale value in the model. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012XR649rKk68z6gBpngu6MA --- .claude/skills/fix-issue.md | 1 + mdl/executor/cmd_contract.go | 8 ++++++- mdl/executor/cmd_contract_test.go | 35 +++++++++++++++++++++++++++++++ 3 files changed, 43 insertions(+), 1 deletion(-) diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index 177fde49b..0e12250b8 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -16,6 +16,7 @@ to the symptom table below, so the next similar issue costs fewer reads. | Symptom | Root cause layer | First file to open | Fix pattern | |---------|-----------------|-------------------|-------------| +| `create or modify external entities` silently resets a per-entity setting the user had changed (e.g. allow-create-change-locally) | `applyExternalEntityFields` stamps every field on both the create and the update path, so anything not derivable from the OData contract was overwritten with a default | `mdl/executor/cmd_contract.go` (`applyExternalEntityFields`) | Separate contract-derived fields (Countable/Creatable/Deletable/Skip/Top — refresh from metadata) from local modelling choices (CreateChangeLocally — leave alone; a new entity arrives zero-valued, which is Mendix's default). Issue #782 | | An **external (OData) entity** loses its remote settings on any read-modify-write — `describe external entity` says `is not an external entity (source: )`, and `alter entity … set allow_create_change_locally = true` reports success but the flag stays off. Works under `--engine legacy` | `entityFromGen` recognised only `DomainModels$OqlViewEntitySource`, so the three `Rest$OData*` sources read back as no source at all: `Source` empty, every remote field zeroed. The write path was fine — it switches on `e.Source`, which the read never populated | `mdl/backend/modelsdk/domainmodel.go` (`entityFromGen`'s source switch, `odataKeyFromGen`) | Mirror the legacy parser (`sdk/mpr/parser_domainmodel.go`) for all three flavours: RemoteEntitySource (capabilities + CreateChangeLocally + key), EntityTypeSource (type name + IsOpen + key), PrimitiveCollectionEntitySource (service only). `Updatable` has no gen accessor and the writer does not emit it — leaving it zero is symmetric. **Check the read side first when a write-path field "does not stick"**: a switch on a field the read never fills looks like a write bug. Repro `mdl-examples/bug-tests/782-external-entity-create-change-locally.mdl`. Issue #782 | | Any `ALTER SETTINGS` (any section) reports success but the Default configuration's **Custom settings** are gone, **Tracing** is reset, and every **constant override** shows blank in Studio Pro — Integer/Long constants then fail the build | The configuration was re-serialized from `model.ServerConfiguration`, which carries only the modelled fields, so CustomSettings/Tracing/OpenAdminPort/OpenHttpPort were dropped, the list version markers downgraded 3→2, and overrides were written with a flat `Value` instead of the nested `SharedOrPrivateValue` the platform reads | `mdl/settingsoverlay/settingsoverlay.go` (`Configurations`, `ServerConfiguration`, `ConstantValues`) — called by both `mdl/backend/modelsdk/settings_write.go` and `sdk/mpr/writer_settings.go` | Overlay onto the raw document instead of rebuilding: write only the fields the read path populates, take each list's marker from what is stored, and update a constant override in the slot it already occupies (nested if nested, flat if flat). New override → nested. New configuration → clone a sibling's shape, empty its collections, mint a fresh `$ID`. Refuse the write when `RawParts` is empty. Repro `mdl-examples/bug-tests/801-alter-settings-preserves-configuration.mdl`. Issue #801 | | `ALTER SETTINGS` / `CREATE CONFIGURATION` prints "Updated …" but `DESCRIBE SETTINGS` shows the old value — an Integer property was given a non-numeric value, or a Boolean anything other than `true` | `strconv.Atoi`'s error was discarded (`if v, err := …; err == nil`) so the assignment was skipped while the caller still printed success; the boolean form compared against `"true"`, silently mapping every other spelling to false | `mdl/executor/cmd_settings.go` (`settingsInt`, `settingsBool`) + `mdl/executor/validate_settings.go` (`typedSettingsKeys`, MDL-SET01/MDL-SET02) | Parse through a helper that returns a validation error naming the setting and the offending value, and register the property in `typedSettingsKeys` so `mxcli check` and the LSP flag it before the project is opened for writing. `TestTypedSettingsKeys_MatchExecutor` guards the table against drifting from the executor's switch. Repro `mdl-examples/bug-tests/805-alter-settings-typed-values.fail.mdl`. Issue #805 | diff --git a/mdl/executor/cmd_contract.go b/mdl/executor/cmd_contract.go index 6628ff7ef..0d6357cd7 100644 --- a/mdl/executor/cmd_contract.go +++ b/mdl/executor/cmd_contract.go @@ -1171,7 +1171,11 @@ func applyExternalEntityFields( ent.Updatable = false ent.SkipSupported = true ent.TopSupported = true - ent.CreateChangeLocally = false + // CreateChangeLocally is deliberately NOT set. Unlike the capability flags + // above it cannot be derived from the service contract — it is a local + // modelling choice ("Allow creating and changing objects locally"), so + // stamping it here reset the user's setting on every re-import (#782). A + // newly-created entity arrives zero-valued, which is Mendix's default. return } @@ -1188,6 +1192,8 @@ func applyExternalEntityFields( ent.Updatable = false ent.SkipSupported = false ent.TopSupported = false + // An entity-type source has no CreateChangeLocally in storage (the writer does + // not emit one), so clear it when an entity is re-imported as a derived type. ent.CreateChangeLocally = false } diff --git a/mdl/executor/cmd_contract_test.go b/mdl/executor/cmd_contract_test.go index 64c4e4abd..696142eb9 100644 --- a/mdl/executor/cmd_contract_test.go +++ b/mdl/executor/cmd_contract_test.go @@ -159,3 +159,38 @@ func TestMendixAttrTypeToEdm(t *testing.T) { } } } + +// TestApplyExternalEntityFields_PreservesCreateChangeLocally is the re-import half +// of mendixlabs/mxcli#782: "Allow creating and changing objects locally" is a local +// modelling choice, not something the OData contract describes, so a re-import must +// not reset it. The capability flags, which the contract *does* describe, are still +// refreshed from the metadata. +func TestApplyExternalEntityFields_PreservesCreateChangeLocally(t *testing.T) { + et := &types.EdmEntityType{Name: "Product"} + es := &types.EdmEntitySet{Name: "Products"} + + // An entity the user had switched the flag on for, being re-imported. + existing := &domainmodel.Entity{CreateChangeLocally: true, Creatable: true} + applyExternalEntityFields(existing, et, true /*isTopLevel*/, "Svc.Sample", es, nil, nil) + if !existing.CreateChangeLocally { + t.Error("CreateChangeLocally reset by re-import, want it preserved (#782)") + } + if existing.Creatable { + t.Error("Creatable not refreshed from the contract (no InsertRestrictions ⇒ false)") + } + + // A newly-imported entity defaults to off, matching Mendix. + fresh := &domainmodel.Entity{} + applyExternalEntityFields(fresh, et, true, "Svc.Sample", es, nil, nil) + if fresh.CreateChangeLocally { + t.Error("a newly imported entity defaulted to CreateChangeLocally = true") + } + + // A derived/entity-type source has no such field in storage; clear it so a + // re-import that reclassifies an entity does not leave a stale value behind. + derived := &domainmodel.Entity{CreateChangeLocally: true} + applyExternalEntityFields(derived, et, false /*isTopLevel*/, "Svc.Sample", nil, nil, nil) + if derived.CreateChangeLocally { + t.Error("entity-type source kept CreateChangeLocally, want it cleared") + } +} From 648b4a5d2baeebf2e937fe60a596522356de20dd Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 19:38:12 +0000 Subject: [PATCH 6/8] fix(microflows): serialize break/continue events, unbreaking loops (#791) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A loop containing a split whose branch does `continue` wrote successfully and could not be opened in Studio Pro: System.Collections.Generic.KeyNotFoundException: The given key '806fca46-5c4b-46f8-a890-4d24dd29c24f' was not present in the dictionary. microflowObjectToGen had no case for Microflows$BreakEvent or Microflows$ContinueEvent, and its default branch returns nil. The event object was therefore dropped at serialization while the SequenceFlow pointing at it was written, leaving a DestinationPointer to a GUID that exists nowhere in the document — the key Studio Pro cannot resolve. Confirmed by dumping the written microflow and checking every *Pointer against the object IDs: one dangling pointer, and no ContinueEvent anywhere. The legacy engine serializes both, so this only affected the default engine. The identical bug had already been found and fixed for Microflows$ErrorEvent, whose case still carries the comment explaining it. Break and continue were missed. MDL051 goes with it. That rule rejected a conditional `break` at check time and pointed users at a guard-variable workaround, explicitly "until the serialization is fixed" — it now is, and keeping the rule would reject valid MDL. Note it only ever covered `break`: `continue` had no guard, which is how this reached a user as a corrupt project. Its negative fixture becomes a positive one, and its test now asserts the pattern is accepted. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012XR649rKk68z6gBpngu6MA --- .claude/skills/fix-issue.md | 1 + .../791-loop-continue-dangling-flow.mdl | 60 +++++++++++++ ...mdl => ledger-52-break-in-conditional.mdl} | 0 .../microflow_loopevent_write_test.go | 62 +++++++++++++ mdl/backend/modelsdk/microflow_write.go | 16 ++++ mdl/executor/validate_microflow.go | 86 ------------------- mdl/executor/validate_microflow_hints_test.go | 39 ++++----- 7 files changed, 156 insertions(+), 108 deletions(-) create mode 100644 mdl-examples/bug-tests/791-loop-continue-dangling-flow.mdl rename mdl-examples/bug-tests/{ledger-52-break-in-conditional.fail.mdl => ledger-52-break-in-conditional.mdl} (100%) create mode 100644 mdl/backend/modelsdk/microflow_loopevent_write_test.go diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index ec0c4bd03..0fb1ff271 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -16,6 +16,7 @@ to the symptom table below, so the next similar issue costs fewer reads. | Symptom | Root cause layer | First file to open | Fix pattern | |---------|-----------------|-------------------|-------------| +| A microflow writes and `mxcli` reports success, but Studio Pro cannot open the project: `System.Collections.Generic.KeyNotFoundException: The given key '' was not present in the dictionary`. Triggered by a `break`/`continue` inside an if/case within a loop | `microflowObjectToGen`'s `default:` branch returns nil for an unhandled object type, so the event object was dropped at serialization while the SequenceFlow pointing at it was written — a DestinationPointer to a GUID that exists nowhere. Exactly the shape already fixed once for `Microflows$ErrorEvent` | `mdl/backend/modelsdk/microflow_write.go` (`microflowObjectToGen`) — compare against `sdk/mpr/writer_microflow.go`, which handles the full set | Add the missing `case *microflows.XxxEvent:` returning the gen element with ID, position and size. **Diagnose without Studio Pro**: dump the microflow (`mxcli bson dump --type microflow --object M.F`), collect every `$ID`, and check every key ending in `Pointer` resolves — a dangling pointer is this bug. When a check-time rule exists only as a stopgap for a write-path bug (MDL051 was), remove it with the real fix, and check whether it covered every affected keyword (MDL051 covered `break`, not `continue`, which is how #791 reached users). Repro `mdl-examples/bug-tests/791-loop-continue-dangling-flow.mdl`. Issue #791, ledger #52 | | Any `ALTER SETTINGS` (any section) reports success but the Default configuration's **Custom settings** are gone, **Tracing** is reset, and every **constant override** shows blank in Studio Pro — Integer/Long constants then fail the build | The configuration was re-serialized from `model.ServerConfiguration`, which carries only the modelled fields, so CustomSettings/Tracing/OpenAdminPort/OpenHttpPort were dropped, the list version markers downgraded 3→2, and overrides were written with a flat `Value` instead of the nested `SharedOrPrivateValue` the platform reads | `mdl/settingsoverlay/settingsoverlay.go` (`Configurations`, `ServerConfiguration`, `ConstantValues`) — called by both `mdl/backend/modelsdk/settings_write.go` and `sdk/mpr/writer_settings.go` | Overlay onto the raw document instead of rebuilding: write only the fields the read path populates, take each list's marker from what is stored, and update a constant override in the slot it already occupies (nested if nested, flat if flat). New override → nested. New configuration → clone a sibling's shape, empty its collections, mint a fresh `$ID`. Refuse the write when `RawParts` is empty. Repro `mdl-examples/bug-tests/801-alter-settings-preserves-configuration.mdl`. Issue #801 | | `ALTER SETTINGS` / `CREATE CONFIGURATION` prints "Updated …" but `DESCRIBE SETTINGS` shows the old value — an Integer property was given a non-numeric value, or a Boolean anything other than `true` | `strconv.Atoi`'s error was discarded (`if v, err := …; err == nil`) so the assignment was skipped while the caller still printed success; the boolean form compared against `"true"`, silently mapping every other spelling to false | `mdl/executor/cmd_settings.go` (`settingsInt`, `settingsBool`) + `mdl/executor/validate_settings.go` (`typedSettingsKeys`, MDL-SET01/MDL-SET02) | Parse through a helper that returns a validation error naming the setting and the offending value, and register the property in `typedSettingsKeys` so `mxcli check` and the LSP flag it before the project is opened for writing. `TestTypedSettingsKeys_MatchExecutor` guards the table against drifting from the executor's switch. Repro `mdl-examples/bug-tests/805-alter-settings-typed-values.fail.mdl`. Issue #805 | | MCP op reports `MCP error -32000: Request timed out` but the page/document/entity EXISTS in Studio Pro afterwards | Studio Pro's ~30s server-side per-call limit fires while the op still applies — a client false failure, not a server rejection | `mdl/backend/mcp/timeout.go` (`isTimeoutErr`, `timeoutVerifyDelay`, `pedUpdateVerify`, `pedDocumentExists`) | Verify-on-timeout, never blind-retry a non-idempotent op: idempotent root-replace (`pgWritePage`) retries once; creates confirm via `ped_find_document`; entity adds confirm via a shallow `/entities` read. Unverified → error with save-before-re-run guidance (a blind re-run from a fresh session duplicates elements) | diff --git a/mdl-examples/bug-tests/791-loop-continue-dangling-flow.mdl b/mdl-examples/bug-tests/791-loop-continue-dangling-flow.mdl new file mode 100644 index 000000000..dde78e3ee --- /dev/null +++ b/mdl-examples/bug-tests/791-loop-continue-dangling-flow.mdl @@ -0,0 +1,60 @@ +-- Bug #791: a loop containing a split whose branch does `continue` produced a +-- project Studio Pro could not open, while mxcli reported success: +-- +-- System.Collections.Generic.KeyNotFoundException: The given key +-- '806fca46-5c4b-46f8-a890-4d24dd29c24f' was not present in the dictionary. +-- +-- Root cause: microflowObjectToGen (mdl/backend/modelsdk/microflow_write.go) had +-- no case for Microflows$BreakEvent / Microflows$ContinueEvent, and its default +-- branch returns nil. The event object was therefore dropped at serialization +-- while the SequenceFlow pointing at it was written — leaving a +-- DestinationPointer to a GUID that exists nowhere in the document, which is the +-- key Studio Pro cannot resolve. The legacy engine (sdk/mpr/writer_microflow.go) +-- serializes both, so this only affected the default (modelsdk) engine. +-- +-- The identical bug had already been found and fixed for Microflows$ErrorEvent; +-- break/continue were missed. +-- +-- Verification without Studio Pro — every *Pointer in the written microflow must +-- resolve to an object $ID in the same document: +-- +-- mxcli exec 791-loop-continue-dangling-flow.mdl -p app.mpr +-- mxcli bson dump -p app.mpr --type microflow --object Issue791.LoopSplitContinue +-- +-- Before the fix the dump contained a SequenceFlow.DestinationPointer with no +-- matching $ID, and no Microflows$ContinueEvent object at all. + +create module Issue791; +create module role Issue791.User; + +@position(100, 100) +create persistent entity Issue791.Contract ( + Name: string(200), + Active: boolean +); + +-- continue in the ELSE branch of a split inside a loop — the reported shape. +create or modify microflow Issue791.LoopSplitContinue ($ContractList: list of Issue791.Contract) +begin + loop $it in $ContractList + begin + if $it/Active then + log info node 'Issue791' 'active'; + else + continue; + end if; + end loop; +end; + +-- break has the same serialization gap and the same consequence. +create or modify microflow Issue791.LoopSplitBreak ($ContractList: list of Issue791.Contract) +begin + loop $it in $ContractList + begin + if $it/Active then + break; + else + log info node 'Issue791' 'inactive'; + end if; + end loop; +end; diff --git a/mdl-examples/bug-tests/ledger-52-break-in-conditional.fail.mdl b/mdl-examples/bug-tests/ledger-52-break-in-conditional.mdl similarity index 100% rename from mdl-examples/bug-tests/ledger-52-break-in-conditional.fail.mdl rename to mdl-examples/bug-tests/ledger-52-break-in-conditional.mdl diff --git a/mdl/backend/modelsdk/microflow_loopevent_write_test.go b/mdl/backend/modelsdk/microflow_loopevent_write_test.go new file mode 100644 index 000000000..ea455b3cf --- /dev/null +++ b/mdl/backend/modelsdk/microflow_loopevent_write_test.go @@ -0,0 +1,62 @@ +// SPDX-License-Identifier: Apache-2.0 + +// mendixlabs/mxcli#791: a microflow with a loop containing a split whose branch +// does `continue` wrote successfully but could not be opened in Studio Pro — +// "System.Collections.Generic.KeyNotFoundException: The given key '' was not +// present in the dictionary". +// +// microflowObjectToGen had no case for BreakEvent/ContinueEvent and its default +// branch returns nil, so the event object was dropped at serialization while the +// sequence flow pointing at it was written. The result is a dangling +// DestinationPointer: exactly the GUID Studio Pro cannot resolve. The legacy engine +// (sdk/mpr/writer_microflow.go) serializes both, so this was a modelsdk-engine gap. +package modelsdkbackend + +import ( + "testing" + + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +func TestMicroflowObjectToGen_LoopEvents(t *testing.T) { + tests := []struct { + name string + obj microflows.MicroflowObject + wantType string + }{ + { + name: "break", + obj: µflows.BreakEvent{BaseMicroflowObject: microflows.BaseMicroflowObject{ + BaseElement: model.BaseElement{ID: "ev-break"}, + Position: model.Point{X: 100, Y: 200}, + Size: model.Size{Width: 20, Height: 20}, + }}, + wantType: "Microflows$BreakEvent", + }, + { + name: "continue", + obj: µflows.ContinueEvent{BaseMicroflowObject: microflows.BaseMicroflowObject{ + BaseElement: model.BaseElement{ID: "ev-continue"}, + Position: model.Point{X: 300, Y: 400}, + Size: model.Size{Width: 20, Height: 20}, + }}, + wantType: "Microflows$ContinueEvent", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + g := microflowObjectToGen(tc.obj) + if g == nil { + t.Fatalf("%s dropped at serialization — any flow pointing at it is left dangling (#791)", tc.wantType) + } + if g.TypeName() != tc.wantType { + t.Errorf("TypeName = %q, want %q", g.TypeName(), tc.wantType) + } + if string(g.ID()) != string(tc.obj.GetID()) { + t.Errorf("ID = %q, want %q — a changed ID leaves the flow pointing at the old one", + g.ID(), tc.obj.GetID()) + } + }) + } +} diff --git a/mdl/backend/modelsdk/microflow_write.go b/mdl/backend/modelsdk/microflow_write.go index b80507ef5..3e06e0069 100644 --- a/mdl/backend/modelsdk/microflow_write.go +++ b/mdl/backend/modelsdk/microflow_write.go @@ -265,6 +265,22 @@ func microflowObjectToGen(obj microflows.MicroflowObject) element.Element { g.SetRelativeMiddlePoint(pointStr(o.Position)) g.SetSize(sizeStr(o.Size)) return g + case *microflows.BreakEvent: + // `break;` in a loop. Same hazard as ErrorEvent above: dropping it left the + // branch's SequenceFlow pointing at a non-existent object, so Studio Pro + // failed to open the project with KeyNotFoundException (#791). + g := genMf.NewBreakEvent() + g.SetID(element.ID(o.ID)) + g.SetRelativeMiddlePoint(pointStr(o.Position)) + g.SetSize(sizeStr(o.Size)) + return g + case *microflows.ContinueEvent: + // `continue;` in a loop — see BreakEvent above. + g := genMf.NewContinueEvent() + g.SetID(element.ID(o.ID)) + g.SetRelativeMiddlePoint(pointStr(o.Position)) + g.SetSize(sizeStr(o.Size)) + return g case *microflows.ActionActivity: g := genMf.NewActionActivity() g.SetID(element.ID(o.ID)) diff --git a/mdl/executor/validate_microflow.go b/mdl/executor/validate_microflow.go index 74d86af92..a765a106e 100644 --- a/mdl/executor/validate_microflow.go +++ b/mdl/executor/validate_microflow.go @@ -299,21 +299,6 @@ func (v *microflowValidator) walkBody(body []ast.MicroflowStatement) { stmt.ListVariable), "Pass the list as a microflow parameter instead of creating an empty variable") } - // Check: a `break` nested inside a conditional within a loop currently - // serializes a dangling sequence-flow reference, producing an UNLOADABLE - // .mpr — `mx check` crashes with an unhandled AggregateException - // ("key … not present in the dictionary") rather than an error. A break - // that is a direct child of the loop body serializes fine, but the useful - // form (`if then break`) is the broken one. Reject it with the - // guard-variable workaround until the flow serialization is fixed. (#52) - if loopBodyHasConditionalBreak(stmt.Body) { - v.addViolation("MDL051", linter.SeverityError, - "a `break` inside an if/case within a loop currently produces an unloadable model — "+ - "`mx check` crashes with an unhandled exception (a dangling sequence-flow reference), "+ - "not a normal error. (A break placed directly in the loop body serializes fine.)", - "Until the serialization is fixed, use a guard variable: "+ - "`declare $Done Boolean = false;` then `loop … if not($Done) then … set $Done = true; end if; end loop`.") - } v.loopDepth++ v.walkBody(stmt.Body) v.loopDepth-- @@ -631,77 +616,6 @@ func (v *microflowValidator) checkAssociationObjectArgs(callee string, args []as } } -// loopBodyHasConditionalBreak reports whether a `break` appears inside a -// conditional (if / case / inheritance split) directly within this loop body — the -// pattern that serializes a dangling reference and crashes `mx check` (#52). A -// break that is a *direct* statement of the loop body is not flagged (it -// serializes fine). Nested loops are not descended into: a break there belongs to -// that loop and is validated when it is walked. -func loopBodyHasConditionalBreak(stmts []ast.MicroflowStatement) bool { - for _, s := range stmts { - switch n := s.(type) { - case *ast.IfStmt: - if stmtsContainBreak(n.ThenBody) || stmtsContainBreak(n.ElseBody) { - return true - } - case *ast.EnumSplitStmt: - for _, c := range n.Cases { - if stmtsContainBreak(c.Body) { - return true - } - } - if stmtsContainBreak(n.ElseBody) { - return true - } - case *ast.InheritanceSplitStmt: - for _, c := range n.Cases { - if stmtsContainBreak(c.Body) { - return true - } - } - if stmtsContainBreak(n.ElseBody) { - return true - } - } - } - return false -} - -// stmtsContainBreak reports whether a `break` belonging to the enclosing loop -// appears anywhere in these statements. Descends into conditionals but NOT into -// nested loops (a nested loop traps its own break). -func stmtsContainBreak(stmts []ast.MicroflowStatement) bool { - for _, s := range stmts { - switch n := s.(type) { - case *ast.BreakStmt: - return true - case *ast.IfStmt: - if stmtsContainBreak(n.ThenBody) || stmtsContainBreak(n.ElseBody) { - return true - } - case *ast.EnumSplitStmt: - for _, c := range n.Cases { - if stmtsContainBreak(c.Body) { - return true - } - } - if stmtsContainBreak(n.ElseBody) { - return true - } - case *ast.InheritanceSplitStmt: - for _, c := range n.Cases { - if stmtsContainBreak(c.Body) { - return true - } - } - if stmtsContainBreak(n.ElseBody) { - return true - } - } - } - return false -} - // exprIsAssociationObjectPath reports whether an expression is an attribute path // whose FINAL segment is a module-qualified association (`$obj/Module.Assoc`) — // i.e. it resolves to an associated OBJECT, not an attribute value. A final bare diff --git a/mdl/executor/validate_microflow_hints_test.go b/mdl/executor/validate_microflow_hints_test.go index 6f5959333..d482d4f19 100644 --- a/mdl/executor/validate_microflow_hints_test.go +++ b/mdl/executor/validate_microflow_hints_test.go @@ -161,38 +161,33 @@ func TestValidateMicroflow_AssociationObjectArg(t *testing.T) { } } -// TestValidateMicroflow_ConditionalBreak covers MDL051 (ledger #52): a `break` -// nested inside a conditional within a loop serializes a dangling reference that -// crashes `mx check` (unloadable model). A break directly in the loop body, or no -// break, is fine. -func TestValidateMicroflow_ConditionalBreak(t *testing.T) { - cases := []struct { - name string - body string - wantMDL bool - }{ - {"break inside if in loop", "loop $R in $L begin if $R/Active then break; end if; end loop", true}, - {"break inside nested if in loop", "loop $R in $L begin if $R/Active then if $R/Active then break; end if; end if; end loop", true}, - {"break directly in loop is fine", "loop $R in $L begin break; end loop", false}, - {"no break is fine", "loop $R in $L begin if $R/Active then set $x = 1; end if; end loop", false}, +// TestValidateMicroflow_ConditionalBreakAccepted: MDL051 rejected a `break` or +// `continue` inside a conditional within a loop, because the write path dropped the +// Break/Continue event and left a dangling sequence flow (ledger #52). That was an +// interim guard "until the serialization is fixed" — it now is (mendixlabs/mxcli#791, +// microflowObjectToGen), so the pattern must be accepted again rather than pushing +// users to a guard variable. It also only ever covered `break`, which is why the +// `continue` form reached users as a corrupt project. +func TestValidateMicroflow_ConditionalBreakAccepted(t *testing.T) { + bodies := []string{ + "loop $R in $L begin if $R/Active then break; end if; end loop", + "loop $R in $L begin if $R/Active then continue; end if; end loop", + "loop $R in $L begin if $R/Active then if $R/Active then break; end if; end if; end loop", + "loop $R in $L begin break; end loop", } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - src := "create microflow M.F ($L: list of M.R)\nreturns boolean\nbegin\n " + tc.body + "\n return true;\nend;" + for _, body := range bodies { + t.Run(body, func(t *testing.T) { + src := "create microflow M.F ($L: list of M.R)\nreturns boolean\nbegin\n " + body + "\n return true;\nend;" prog, errs := visitor.Build(src) if len(errs) > 0 { t.Fatalf("parse errors: %v", errs) } mf := prog.Statements[0].(*ast.CreateMicroflowStmt) - var got bool for _, vi := range ValidateMicroflow(mf) { if vi.RuleID == "MDL051" { - got = true + t.Errorf("MDL051 still rejects a now-serializable pattern: %s", body) } } - if got != tc.wantMDL { - t.Errorf("MDL051 fired=%v, want %v (body: %q)", got, tc.wantMDL, tc.body) - } }) } } From 5bfe84d59c8a947f11c6873507f944b4ed5bac5f Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 18:10:40 +0000 Subject: [PATCH 7/8] fix(test): leave the project as `mxcli test` found it (#802/#803/#804) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects in one teardown, which together left a tested project permanently mutated. quotes before trailing punctuation, and DESCRIBE SETTINGS separates properties with commas, so AfterStartupMicroflow = 'MyFirstModule.ASU_Startup', parsed to `MyFirstModule.ASU_Startup',` and the restore statement became ALTER SETTINGS MODEL AfterStartupMicroflow = 'MyFirstModule.ASU_Startup',' which does not parse. The failure printed as a warning, so the run looked clean while the project was left pointing at MxTest.TestRunner — the microflow cleanup then deleted. Trailing `,;` now comes off before the quotes, the value is re-emitted through a helper that doubles embedded quotes (Mendix's escape, never backslashes), and cleanup returns its errors instead of printing them: a failed restore now fails the run and names what was left changed. so an empty module accumulated on every run. It now drops the module — but only when the run created it. CREATE MODULE is idempotent, so a project that already had its own MxTest module would otherwise have had it deleted; that case still removes just the generated microflow. Cleanup also re-checks that the module is present, so a run that failed before injection landed does not report a spurious cleanup failure. hardcoded PRODUCTION, silently changing projects that legitimately run at another level, and breaking any project whose published REST/OData services use custom authentication ("App security is off, but custom authentication is enabled for this service"). The after-startup microflow runs in an administrative context and is not subject to the Security Level, so this bought nothing. It is gone; the setting is the project's business. What cleanup must restore is now captured in a projectState before the first mutation, and the setup/cleanup statements are built by pure functions, so the restore is testable without a project or Docker. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012XR649rKk68z6gBpngu6MA --- .claude/skills/fix-issue.md | 1 + .claude/skills/mendix/test-microflows.md | 16 +- cmd/mxcli/testrunner/runner.go | 228 ++++++++++++++++---- cmd/mxcli/testrunner/runner_cleanup_test.go | 195 +++++++++++++++++ 4 files changed, 395 insertions(+), 45 deletions(-) create mode 100644 cmd/mxcli/testrunner/runner_cleanup_test.go diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index ec0c4bd03..7adb5d29d 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -16,6 +16,7 @@ to the symptom table below, so the next similar issue costs fewer reads. | Symptom | Root cause layer | First file to open | Fix pattern | |---------|-----------------|-------------------|-------------| +| `mxcli test` leaves the project mutated: Security Level changed, after-startup pointing at the deleted `MxTest.TestRunner`, and only a `Warning:` line about it. Or: an empty `MxTest` module accumulates after every run | Three defects in one teardown. (a) `getAfterStartup` trimmed quotes *before* trailing punctuation, so a DESCRIBE SETTINGS line ending in `,` yielded `Module.Flow',` and the restore statement was unparseable; (b) cleanup dropped only the microflow, not the module it created; (c) the Security Level was forced OFF and restored to a hardcoded PRODUCTION | `cmd/mxcli/testrunner/runner.go` (`parseSettingValue`, `quoteMDLString`, `projectState`, `setupCommands`, `cleanupCommands`) | Strip trailing `,;` before unquoting; re-emit via `quoteMDLString` (doubling embedded quotes, never backslashes); capture a `projectState` before the first mutation and restore from it; drop the module only when the run created it (a pre-existing `MxTest` is the user's); leave Security Level alone entirely; return cleanup errors instead of printing warnings, and fail the run when they occur. Command lists are pure functions so the restore is testable without a project or Docker. Issues #802/#803/#804 | | Any `ALTER SETTINGS` (any section) reports success but the Default configuration's **Custom settings** are gone, **Tracing** is reset, and every **constant override** shows blank in Studio Pro — Integer/Long constants then fail the build | The configuration was re-serialized from `model.ServerConfiguration`, which carries only the modelled fields, so CustomSettings/Tracing/OpenAdminPort/OpenHttpPort were dropped, the list version markers downgraded 3→2, and overrides were written with a flat `Value` instead of the nested `SharedOrPrivateValue` the platform reads | `mdl/settingsoverlay/settingsoverlay.go` (`Configurations`, `ServerConfiguration`, `ConstantValues`) — called by both `mdl/backend/modelsdk/settings_write.go` and `sdk/mpr/writer_settings.go` | Overlay onto the raw document instead of rebuilding: write only the fields the read path populates, take each list's marker from what is stored, and update a constant override in the slot it already occupies (nested if nested, flat if flat). New override → nested. New configuration → clone a sibling's shape, empty its collections, mint a fresh `$ID`. Refuse the write when `RawParts` is empty. Repro `mdl-examples/bug-tests/801-alter-settings-preserves-configuration.mdl`. Issue #801 | | `ALTER SETTINGS` / `CREATE CONFIGURATION` prints "Updated …" but `DESCRIBE SETTINGS` shows the old value — an Integer property was given a non-numeric value, or a Boolean anything other than `true` | `strconv.Atoi`'s error was discarded (`if v, err := …; err == nil`) so the assignment was skipped while the caller still printed success; the boolean form compared against `"true"`, silently mapping every other spelling to false | `mdl/executor/cmd_settings.go` (`settingsInt`, `settingsBool`) + `mdl/executor/validate_settings.go` (`typedSettingsKeys`, MDL-SET01/MDL-SET02) | Parse through a helper that returns a validation error naming the setting and the offending value, and register the property in `typedSettingsKeys` so `mxcli check` and the LSP flag it before the project is opened for writing. `TestTypedSettingsKeys_MatchExecutor` guards the table against drifting from the executor's switch. Repro `mdl-examples/bug-tests/805-alter-settings-typed-values.fail.mdl`. Issue #805 | | MCP op reports `MCP error -32000: Request timed out` but the page/document/entity EXISTS in Studio Pro afterwards | Studio Pro's ~30s server-side per-call limit fires while the op still applies — a client false failure, not a server rejection | `mdl/backend/mcp/timeout.go` (`isTimeoutErr`, `timeoutVerifyDelay`, `pedUpdateVerify`, `pedDocumentExists`) | Verify-on-timeout, never blind-retry a non-idempotent op: idempotent root-replace (`pgWritePage`) retries once; creates confirm via `ped_find_document`; entity adds confirm via a shallow `/entities` read. Unverified → error with save-before-re-run guidance (a blind re-run from a fresh session duplicates elements) | diff --git a/.claude/skills/mendix/test-microflows.md b/.claude/skills/mendix/test-microflows.md index f5a89dd2d..90420179a 100644 --- a/.claude/skills/mendix/test-microflows.md +++ b/.claude/skills/mendix/test-microflows.md @@ -110,13 +110,23 @@ mxcli test tests/ -p app.mpr --verbose The test runner uses the **after-startup microflow** pattern: 1. Parses test files and extracts test blocks with annotations -2. Generates a `MxTest.TestRunner` microflow with assertion logic -3. Sets security OFF and after-startup to `MxTest.TestRunner` +2. Records the project's current after-startup microflow, and whether an `MxTest` + module already exists +3. Generates a `MxTest.TestRunner` microflow with assertion logic and points + after-startup at it 4. Builds the project and restarts the Docker runtime 5. Captures structured `MXTEST:` log lines for pass/fail -6. Restores original security and after-startup settings +6. Restores the original after-startup setting and removes the generated runner — + the whole `MxTest` module when the runner created it, otherwise just the + `TestRunner` microflow 7. Outputs results (console, JUnit XML) +The project's **Security Level is not modified**. The after-startup microflow runs +in an administrative context and is not subject to it, and forcing it off breaks +projects whose published REST/OData services use custom authentication. If a +cleanup step fails the run reports an error and names what was left changed — +the project is modified, so it must not read as a clean pass. + --- ## Writing Good Tests diff --git a/cmd/mxcli/testrunner/runner.go b/cmd/mxcli/testrunner/runner.go index 1ed4d6ff4..f0d35c25e 100644 --- a/cmd/mxcli/testrunner/runner.go +++ b/cmd/mxcli/testrunner/runner.go @@ -6,6 +6,8 @@ import ( "bufio" "bytes" "context" + "encoding/json" + "errors" "fmt" "io" "os" @@ -17,6 +19,13 @@ import ( "github.com/mendixlabs/mxcli/cmd/mxcli/docker" ) +const ( + // mxTestModule is the module Run generates the test runner into. + mxTestModule = "MxTest" + // mxTestRunner is the generated after-startup microflow. + mxTestRunner = "MxTest.TestRunner" +) + // RunOptions configures the test runner. type RunOptions struct { // ProjectPath is the path to the .mpr file. @@ -94,9 +103,12 @@ func Run(opts RunOptions) (*SuiteResult, error) { // Step 3: Save original settings and inject test runner fmt.Fprintln(w, "Injecting test runner into project...") - origAfterStartup, err := getAfterStartup(opts.ProjectPath) + // Capture what cleanup will need to restore, before touching anything. This + // must succeed: without it cleanup cannot tell an existing MxTest module from + // the one it is about to create, nor restore the original after-startup. + state, err := captureProjectState(opts.ProjectPath) if err != nil { - fmt.Fprintf(w, " Warning: could not read original after-startup setting: %v\n", err) + return nil, fmt.Errorf("capturing project state: %w", err) } // Write the runner MDL to a temp file and execute it @@ -118,28 +130,25 @@ func Run(opts RunOptions) (*SuiteResult, error) { return nil, fmt.Errorf("injecting test runner: %w", err) } - // Set security OFF for testing - if err := execMxcliCmd(opts.ProjectPath, "ALTER PROJECT SECURITY LEVEL OFF"); err != nil { - fmt.Fprintf(w, " Warning: could not set security OFF: %v\n", err) - } - // Set after-startup microflow - if err := execMxcliCmd(opts.ProjectPath, "ALTER SETTINGS MODEL AfterStartupMicroflow = 'MxTest.TestRunner'"); err != nil { - return nil, fmt.Errorf("setting after-startup: %w", err) + for _, cmd := range setupCommands() { + if err := execMxcliCmd(opts.ProjectPath, cmd); err != nil { + return nil, fmt.Errorf("preparing project for the test run (%s): %w", cmd, err) + } } - fmt.Fprintln(w, " After-startup set to MxTest.TestRunner") + fmt.Fprintf(w, " After-startup set to %s\n", mxTestRunner) // Step 4: Build and restart dockerDir := filepath.Join(filepath.Dir(opts.ProjectPath), ".docker") if err := ensureDockerStack(opts.ProjectPath, dockerDir, w); err != nil { - cleanup(opts.ProjectPath, origAfterStartup, w) + reportCleanup(w, cleanup(opts.ProjectPath, state, w)) return nil, fmt.Errorf("docker init: %w", err) } if !opts.SkipBuild { fmt.Fprintln(w, "Building project...") if err := execMxcli(opts.ProjectPath, "docker", "build", "-p", opts.ProjectPath, "--skip-check"); err != nil { - cleanup(opts.ProjectPath, origAfterStartup, w) + reportCleanup(w, cleanup(opts.ProjectPath, state, w)) return nil, fmt.Errorf("docker build: %w", err) } } @@ -149,7 +158,7 @@ func Run(opts RunOptions) (*SuiteResult, error) { runCompose(dockerDir, "down") // Start fresh if err := runCompose(dockerDir, "up", "--detach", "--force-recreate"); err != nil { - cleanup(opts.ProjectPath, origAfterStartup, w) + reportCleanup(w, cleanup(opts.ProjectPath, state, w)) return nil, fmt.Errorf("docker up: %w", err) } @@ -157,7 +166,7 @@ func Run(opts RunOptions) (*SuiteResult, error) { fmt.Fprintf(w, "Waiting for test execution (timeout: %s)...\n", timeout) logOutput, err := captureRuntimeLogs(dockerDir, timeout, w, opts.Verbose) if err != nil { - cleanup(opts.ProjectPath, origAfterStartup, w) + reportCleanup(w, cleanup(opts.ProjectPath, state, w)) return nil, fmt.Errorf("runtime execution: %w", err) } @@ -167,7 +176,8 @@ func Run(opts RunOptions) (*SuiteResult, error) { // Step 7: Cleanup fmt.Fprintln(w, "Cleaning up...") - cleanup(opts.ProjectPath, origAfterStartup, w) + cleanupErr := cleanup(opts.ProjectPath, state, w) + reportCleanup(w, cleanupErr) // Step 8: Output results PrintResults(w, result, opts.Color) @@ -185,6 +195,11 @@ func Run(opts RunOptions) (*SuiteResult, error) { fmt.Fprintf(w, "JUnit XML written to: %s\n", opts.JUnitOutput) } + // A failed cleanup leaves the project modified, so the run must not be + // reported as clean even when every test passed. + if cleanupErr != nil { + return result, fmt.Errorf("cleanup failed, project left modified: %w", cleanupErr) + } return result, nil } @@ -254,6 +269,35 @@ func parseTestFiles(paths []string) (*TestSuite, error) { return combined, nil } +// projectState records what Run changed in the project, captured before the first +// mutation so cleanup can put things back exactly rather than guessing. +type projectState struct { + // afterStartup is the project's original after-startup microflow ("" = none). + afterStartup string + // createdMxTest reports whether Run created the MxTest module, i.e. it did not + // already exist. A pre-existing MxTest module belongs to the user and must + // survive cleanup with everything but the generated TestRunner intact. + createdMxTest bool +} + +// captureProjectState reads everything cleanup needs to restore, before anything +// is injected. +func captureProjectState(projectPath string) (projectState, error) { + var st projectState + af, err := getAfterStartup(projectPath) + if err != nil { + return st, fmt.Errorf("reading after-startup setting: %w", err) + } + st.afterStartup = af + + exists, err := moduleExists(projectPath, mxTestModule) + if err != nil { + return st, fmt.Errorf("listing modules: %w", err) + } + st.createdMxTest = !exists + return st, nil +} + // getAfterStartup reads the current after-startup microflow setting. func getAfterStartup(projectPath string) (string, error) { mxcliPath, err := findMxcli() @@ -268,46 +312,146 @@ func getAfterStartup(projectPath string) (string, error) { return "", err } - // Parse output for AfterStartupMicroflow for _, line := range strings.Split(string(output), "\n") { - line = strings.TrimSpace(line) if strings.Contains(line, "AfterStartupMicroflow") { - // Extract the value from: AfterStartupMicroflow = 'Module.Name' - parts := strings.SplitN(line, "=", 2) - if len(parts) == 2 { - val := strings.TrimSpace(parts[1]) - val = strings.Trim(val, "'\"") - val = strings.TrimSuffix(val, ";") - val = strings.TrimSpace(val) - return val, nil - } + return parseSettingValue(line), nil } } - return "", nil } -// cleanup restores original project settings after testing. -func cleanup(projectPath, origAfterStartup string, w io.Writer) { - // Restore original after-startup - if origAfterStartup != "" { - cmd := fmt.Sprintf("ALTER SETTINGS MODEL AfterStartupMicroflow = '%s'", origAfterStartup) - if err := execMxcliCmd(projectPath, cmd); err != nil { - fmt.Fprintf(w, " Warning: could not restore after-startup: %v\n", err) +// parseSettingValue extracts the value from one DESCRIBE SETTINGS line, e.g. +// +// AfterStartupMicroflow = 'Module.Name', +// +// DESCRIBE SETTINGS separates properties with commas and ends the statement with +// a semicolon, so the trailing punctuation must come off *before* the quotes: +// trimming quotes first stops at the comma and leaves `Module.Name',`, which was +// then re-interpolated into unparseable MDL and silently failed to restore the +// setting (mendixlabs/mxcli#803). +func parseSettingValue(line string) string { + val := strings.TrimSpace(line) + if _, after, found := strings.Cut(val, "="); found { + val = after + } + val = strings.TrimSpace(val) + val = strings.TrimRight(val, ",;") + val = strings.TrimSpace(val) + return strings.Trim(val, "'\"") +} + +// quoteMDLString renders a value as an MDL single-quoted literal. Mendix escapes +// an embedded quote by doubling it; a qualified name should never contain one, but +// emitting a broken literal is how #803 turned a parse slip into a mutated project. +func quoteMDLString(v string) string { + return "'" + strings.ReplaceAll(v, "'", "''") + "'" +} + +// moduleExists reports whether the project has a module with the given name. +func moduleExists(projectPath, name string) (bool, error) { + mxcliPath, err := findMxcli() + if err != nil { + return false, err + } + cmd := exec.Command(mxcliPath, "-p", projectPath, "-c", "SHOW MODULES", "--json") + cmd.Env = append(os.Environ(), "MXCLI_QUIET=1") + output, err := cmd.Output() + if err != nil { + return false, err + } + var modules []struct { + Module string `json:"Module"` + } + if err := json.Unmarshal(output, &modules); err != nil { + return false, fmt.Errorf("parsing module list: %w", err) + } + for _, m := range modules { + if strings.EqualFold(m.Module, name) { + return true, nil } + } + return false, nil +} + +// setupCommands returns the MDL statements Run issues to put the project into its +// testing state. The project's Security Level is deliberately absent: the +// after-startup microflow runs in an administrative context and is not subject to +// it, so forcing it OFF bought nothing — while breaking any project with a +// published REST/OData service using custom authentication ("App security is off, +// but custom authentication is enabled for this service"), and the restore +// hardcoded PRODUCTION, silently changing projects that run at another level +// (mendixlabs/mxcli#802). +func setupCommands() []string { + return []string{ + "ALTER SETTINGS MODEL AfterStartupMicroflow = " + quoteMDLString(mxTestRunner), + } +} + +// cleanupCommands returns the MDL statements that put the project back the way it +// was, in order. Kept separate from execution so the restore can be tested without +// a project. mxTestPresent says whether the generated module is still there — +// nothing is dropped when it is already gone, so a run that failed before the +// injection landed does not report a spurious cleanup failure. +func cleanupCommands(st projectState, mxTestPresent bool) []string { + // Restore the original after-startup microflow, or clear it if there was none. + restore := "ALTER SETTINGS MODEL AfterStartupMicroflow = ''" + if st.afterStartup != "" { + restore = "ALTER SETTINGS MODEL AfterStartupMicroflow = " + quoteMDLString(st.afterStartup) + } + cmds := []string{restore} + if !mxTestPresent { + return cmds + } + + // Remove the generated runner. Drop the whole module only when Run created it; + // a pre-existing MxTest module is the user's, so only the generated microflow + // comes out of it. + if st.createdMxTest { + cmds = append(cmds, "DROP MODULE "+mxTestModule) } else { - if err := execMxcliCmd(projectPath, "ALTER SETTINGS MODEL AfterStartupMicroflow = ''"); err != nil { - fmt.Fprintf(w, " Warning: could not clear after-startup: %v\n", err) - } + cmds = append(cmds, "DROP MICROFLOW "+mxTestRunner) } + return cmds +} - // Restore security level - if err := execMxcliCmd(projectPath, "ALTER PROJECT SECURITY LEVEL PRODUCTION"); err != nil { - fmt.Fprintf(w, " Warning: could not restore security: %v\n", err) +// cleanup restores the project to the state captured before injection and removes +// the generated test runner. +// +// Every statement is attempted even if an earlier one fails, and the failures are +// returned rather than printed as warnings: a half-restored project is left with +// its after-startup pointing at a microflow this function is about to delete, and +// that has to be loud (#803). +func cleanup(projectPath string, st projectState, w io.Writer) error { + // Re-check rather than assume: on failure fall back to attempting the drop, so + // a genuine problem still surfaces instead of being skipped. + mxTestPresent := true + if exists, err := moduleExists(projectPath, mxTestModule); err == nil { + mxTestPresent = exists + } + if mxTestPresent && !st.createdMxTest { + fmt.Fprintf(w, " %s module already existed; dropping only %s\n", mxTestModule, mxTestRunner) + } + + var errs []error + for _, cmd := range cleanupCommands(st, mxTestPresent) { + if err := execMxcliCmd(projectPath, cmd); err != nil { + errs = append(errs, fmt.Errorf("%s: %w", cmd, err)) + } } + if len(errs) > 0 { + return errors.Join(errs...) + } + return nil +} - // Drop the test runner microflow - execMxcliCmd(projectPath, "DROP MICROFLOW MxTest.TestRunner") +// reportCleanup prints a cleanup failure prominently. The project is left mutated, +// so this must not read as a passing run. +func reportCleanup(w io.Writer, err error) { + if err == nil { + return + } + fmt.Fprintf(w, "\nERROR: cleanup failed — the project has been left modified:\n%v\n", err) + fmt.Fprintf(w, "Check the after-startup microflow and the %s module before committing.\n", mxTestModule) } // captureRuntimeLogs tails the docker compose logs, waiting for MXTEST:END or timeout. diff --git a/cmd/mxcli/testrunner/runner_cleanup_test.go b/cmd/mxcli/testrunner/runner_cleanup_test.go new file mode 100644 index 000000000..4809c0c04 --- /dev/null +++ b/cmd/mxcli/testrunner/runner_cleanup_test.go @@ -0,0 +1,195 @@ +// SPDX-License-Identifier: Apache-2.0 + +// Regression tests for the three `mxcli test` cleanup defects: +// +// - #803 the after-startup value was mis-parsed, so restoring it produced +// unparseable MDL, the failure was printed as a warning, and the project was +// left with its after-startup pointing at the microflow cleanup then deleted +// - #804 cleanup dropped only the microflow, leaving an empty MxTest module +// - #802 the Security Level was forced OFF and restored to a hardcoded +// PRODUCTION, regardless of what the project actually used +package testrunner + +import ( + "strings" + "testing" +) + +// TestParseSettingValue covers the lines DESCRIBE SETTINGS actually emits. +// Properties are comma-separated and the statement ends with a semicolon, so the +// trailing punctuation has to come off before the quotes — trimming quotes first +// stops at the comma and leaves the punctuation inside the value (#803). +func TestParseSettingValue(t *testing.T) { + tests := []struct { + name string + line string + want string + }{ + { + name: "trailing comma (the reported case)", + line: " AfterStartupMicroflow = 'MyFirstModule.ASU_Startup',", + want: "MyFirstModule.ASU_Startup", + }, + { + name: "trailing semicolon (last property in the statement)", + line: " AfterStartupMicroflow = 'MyFirstModule.ASU_Startup';", + want: "MyFirstModule.ASU_Startup", + }, + { + name: "no trailing punctuation", + line: " AfterStartupMicroflow = 'MyFirstModule.ASU_Startup'", + want: "MyFirstModule.ASU_Startup", + }, + { + name: "double quotes", + line: ` AfterStartupMicroflow = "MyFirstModule.ASU_Startup",`, + want: "MyFirstModule.ASU_Startup", + }, + { + name: "empty value", + line: " AfterStartupMicroflow = '',", + want: "", + }, + { + name: "value containing an equals sign is not truncated", + line: " SomeSetting = 'a=b',", + want: "a=b", + }, + { + name: "no equals sign at all", + line: " AfterStartupMicroflow", + want: "AfterStartupMicroflow", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := parseSettingValue(tc.line); got != tc.want { + t.Errorf("parseSettingValue(%q) = %q, want %q", tc.line, got, tc.want) + } + }) + } +} + +// TestParseSettingValue_RoundTripsThroughQuoting is the property that actually +// broke: whatever is parsed out must go back in as a well-formed MDL literal. +func TestParseSettingValue_RoundTripsThroughQuoting(t *testing.T) { + for _, line := range []string{ + " AfterStartupMicroflow = 'MyFirstModule.ASU_Startup',", + " AfterStartupMicroflow = 'MyFirstModule.ASU_Startup';", + " AfterStartupMicroflow = 'Mod.Flow'", + } { + got := quoteMDLString(parseSettingValue(line)) + if strings.Count(got, "'") != 2 { + t.Errorf("re-quoting %q produced %q — not a single well-formed literal", line, got) + } + if strings.HasSuffix(got, ",'") || strings.HasSuffix(got, ";'") { + t.Errorf("punctuation leaked into the value: %q", got) + } + } +} + +func TestQuoteMDLString(t *testing.T) { + tests := []struct { + in string + want string + }{ + {"Mod.Flow", "'Mod.Flow'"}, + {"", "''"}, + // Mendix escapes an embedded quote by doubling it, never with a backslash. + {"it's", "'it''s'"}, + {"a'b'c", "'a''b''c'"}, + } + for _, tc := range tests { + if got := quoteMDLString(tc.in); got != tc.want { + t.Errorf("quoteMDLString(%q) = %q, want %q", tc.in, got, tc.want) + } + } +} + +// TestNoSecurityLevelManipulation pins #802: the Security Level is the project's +// business. Neither setup nor cleanup may touch it. +func TestNoSecurityLevelManipulation(t *testing.T) { + all := append(setupCommands(), cleanupCommands(projectState{}, true)...) + all = append(all, cleanupCommands(projectState{afterStartup: "Mod.Flow", createdMxTest: true}, true)...) + for _, cmd := range all { + if strings.Contains(strings.ToUpper(cmd), "SECURITY LEVEL") { + t.Errorf("the runner still alters the project Security Level: %q (#802)", cmd) + } + } +} + +func TestCleanupCommands(t *testing.T) { + tests := []struct { + name string + state projectState + present bool + want []string + }{ + { + name: "restores an existing after-startup and drops the module it created", + state: projectState{afterStartup: "MyFirstModule.ASU_Startup", createdMxTest: true}, + present: true, + want: []string{ + "ALTER SETTINGS MODEL AfterStartupMicroflow = 'MyFirstModule.ASU_Startup'", + "DROP MODULE MxTest", + }, + }, + { + name: "clears after-startup when the project had none", + state: projectState{createdMxTest: true}, + present: true, + want: []string{ + "ALTER SETTINGS MODEL AfterStartupMicroflow = ''", + "DROP MODULE MxTest", + }, + }, + { + // A pre-existing MxTest module belongs to the user: only the generated + // microflow may be removed, or the run destroys their work. + name: "keeps a pre-existing MxTest module", + state: projectState{afterStartup: "Mod.Flow"}, + present: true, + want: []string{ + "ALTER SETTINGS MODEL AfterStartupMicroflow = 'Mod.Flow'", + "DROP MICROFLOW MxTest.TestRunner", + }, + }, + { + // The injection never landed: restore the setting, drop nothing, and do + // not report a cleanup failure for a module that was never created. + name: "nothing to drop when the module is absent", + state: projectState{afterStartup: "Mod.Flow", createdMxTest: true}, + present: false, + want: []string{ + "ALTER SETTINGS MODEL AfterStartupMicroflow = 'Mod.Flow'", + }, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := cleanupCommands(tc.state, tc.present) + if len(got) != len(tc.want) { + t.Fatalf("cleanupCommands = %q, want %q", got, tc.want) + } + for i := range got { + if got[i] != tc.want[i] { + t.Errorf("command %d = %q, want %q", i, got[i], tc.want[i]) + } + } + }) + } +} + +// TestCleanupCommands_RestoreIsWellFormed is the end of the #803 chain: whatever +// DESCRIBE SETTINGS produced must come back as a parseable statement. +func TestCleanupCommands_RestoreIsWellFormed(t *testing.T) { + parsed := parseSettingValue(" AfterStartupMicroflow = 'MyFirstModule.ASU_Startup',") + restore := cleanupCommands(projectState{afterStartup: parsed}, true)[0] + want := "ALTER SETTINGS MODEL AfterStartupMicroflow = 'MyFirstModule.ASU_Startup'" + if restore != want { + t.Errorf("restore command = %q, want %q", restore, want) + } + if strings.Count(restore, "'") != 2 { + t.Errorf("restore command is not a single well-formed literal: %q", restore) + } +} From 13332e5a0f962e29ed1734a9c1d551f97a2850dd Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 21:03:39 +0000 Subject: [PATCH 8/8] fix(microflows): size a loop box to the space its body actually occupies (#790) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A loop box was drawn far wider than its contents — 880px around three activities spanning 440px, leaving ~290px of empty area inside the frame. measureStatements sums each element's full width and adds HorizontalSpacing between them. But HorizontalSpacing is a centre-to-centre pitch: the builder centres each activity on posX and advances by exactly that, so counting it on top of each width over-measures a run of n simple activities by (n-1)*ActivityWidth — 240px for three. The loop width is derived from that measure, so the box inherited the error. Loop sizing now uses measureStatementsSpan, which returns the true extent (n-1)*HorizontalSpacing + ActivityWidth for a run of simple activities. The example above goes from 880 to 640 (the body occupies 90..530 inside it), and nested loops shrink at both levels. A compound element (IF/split, nested loop) advances posX by merge geometry this function cannot reproduce without duplicating the builder. An earlier attempt to approximate it under-sized the box and pushed the trailing activity 30px outside the frame — worse than a box that is merely too wide — so those runs fall back to the conservative measure and are unchanged. Verified by checking that every child of a LoopedActivity lies within its box, across loop-with-if, nested-loop, single-statement and multi-statement bodies. Not addressed: the other half of #790, "position annotations sometimes change". Explicit @position annotations round-trip exactly, and three successive describe/exec cycles produce byte-identical output, so there is nothing reproducible to fix here — the reporter also noted it was inconsistent. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012XR649rKk68z6gBpngu6MA --- .claude/skills/fix-issue.md | 1 + .../cmd_microflows_builder_control.go | 4 +- mdl/executor/layout.go | 37 +++++++++ mdl/executor/layout_span_test.go | 75 +++++++++++++++++++ 4 files changed, 115 insertions(+), 2 deletions(-) create mode 100644 mdl/executor/layout_span_test.go diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index 0fb1ff271..d1942da8d 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -16,6 +16,7 @@ to the symptom table below, so the next similar issue costs fewer reads. | Symptom | Root cause layer | First file to open | Fix pattern | |---------|-----------------|-------------------|-------------| +| A generated loop box is drawn far wider than the activities inside it (e.g. 880px around 440px of content), leaving a large empty area — "the visualization is poor" | `measureStatements` sums each element's full width **and** adds `HorizontalSpacing` between them, but `HorizontalSpacing` is a centre-to-centre *pitch*: the builder centres each activity on `posX` and advances by exactly that. Counting it on top of each width over-measures a run of n simple activities by `(n-1)*ActivityWidth` | `mdl/executor/layout.go` (`measureStatementsSpan`), used by `addLoopStatement`/`addWhileStatement` in `cmd_microflows_builder_control.go` | For a run of only simple activities the true span is `(n-1)*HorizontalSpacing + ActivityWidth`. **Do not guess the advance for a compound element** (IF/split, nested loop) — its `posX` advance comes from merge geometry (`mergeX + MergeSize + HorizontalSpacing/2`), and guessing under-sizes the box so activities land *outside* it, which is worse than a box that is too wide; those runs fall back to the conservative measure. Verify with a containment check: every child of a LoopedActivity must lie within `[0, width] × [0, height]`. Issue #790 | | A microflow writes and `mxcli` reports success, but Studio Pro cannot open the project: `System.Collections.Generic.KeyNotFoundException: The given key '' was not present in the dictionary`. Triggered by a `break`/`continue` inside an if/case within a loop | `microflowObjectToGen`'s `default:` branch returns nil for an unhandled object type, so the event object was dropped at serialization while the SequenceFlow pointing at it was written — a DestinationPointer to a GUID that exists nowhere. Exactly the shape already fixed once for `Microflows$ErrorEvent` | `mdl/backend/modelsdk/microflow_write.go` (`microflowObjectToGen`) — compare against `sdk/mpr/writer_microflow.go`, which handles the full set | Add the missing `case *microflows.XxxEvent:` returning the gen element with ID, position and size. **Diagnose without Studio Pro**: dump the microflow (`mxcli bson dump --type microflow --object M.F`), collect every `$ID`, and check every key ending in `Pointer` resolves — a dangling pointer is this bug. When a check-time rule exists only as a stopgap for a write-path bug (MDL051 was), remove it with the real fix, and check whether it covered every affected keyword (MDL051 covered `break`, not `continue`, which is how #791 reached users). Repro `mdl-examples/bug-tests/791-loop-continue-dangling-flow.mdl`. Issue #791, ledger #52 | | Any `ALTER SETTINGS` (any section) reports success but the Default configuration's **Custom settings** are gone, **Tracing** is reset, and every **constant override** shows blank in Studio Pro — Integer/Long constants then fail the build | The configuration was re-serialized from `model.ServerConfiguration`, which carries only the modelled fields, so CustomSettings/Tracing/OpenAdminPort/OpenHttpPort were dropped, the list version markers downgraded 3→2, and overrides were written with a flat `Value` instead of the nested `SharedOrPrivateValue` the platform reads | `mdl/settingsoverlay/settingsoverlay.go` (`Configurations`, `ServerConfiguration`, `ConstantValues`) — called by both `mdl/backend/modelsdk/settings_write.go` and `sdk/mpr/writer_settings.go` | Overlay onto the raw document instead of rebuilding: write only the fields the read path populates, take each list's marker from what is stored, and update a constant override in the slot it already occupies (nested if nested, flat if flat). New override → nested. New configuration → clone a sibling's shape, empty its collections, mint a fresh `$ID`. Refuse the write when `RawParts` is empty. Repro `mdl-examples/bug-tests/801-alter-settings-preserves-configuration.mdl`. Issue #801 | | `ALTER SETTINGS` / `CREATE CONFIGURATION` prints "Updated …" but `DESCRIBE SETTINGS` shows the old value — an Integer property was given a non-numeric value, or a Boolean anything other than `true` | `strconv.Atoi`'s error was discarded (`if v, err := …; err == nil`) so the assignment was skipped while the caller still printed success; the boolean form compared against `"true"`, silently mapping every other spelling to false | `mdl/executor/cmd_settings.go` (`settingsInt`, `settingsBool`) + `mdl/executor/validate_settings.go` (`typedSettingsKeys`, MDL-SET01/MDL-SET02) | Parse through a helper that returns a validation error naming the setting and the offending value, and register the property in `typedSettingsKeys` so `mxcli check` and the LSP flag it before the project is opened for writing. `TestTypedSettingsKeys_MatchExecutor` guards the table against drifting from the executor's switch. Repro `mdl-examples/bug-tests/805-alter-settings-typed-values.fail.mdl`. Issue #805 | diff --git a/mdl/executor/cmd_microflows_builder_control.go b/mdl/executor/cmd_microflows_builder_control.go index 5501089d2..3529ab522 100644 --- a/mdl/executor/cmd_microflows_builder_control.go +++ b/mdl/executor/cmd_microflows_builder_control.go @@ -568,7 +568,7 @@ func (fb *flowBuilder) addLoopStatement(s *ast.LoopStmt) model.ID { fb.pendingAnnotations = nil // First, measure the loop body to determine size - bodyBounds := fb.measurer.measureStatements(s.Body) + bodyBounds := fb.measurer.measureStatementsSpan(s.Body) // Calculate loop box size with padding // Extra width for iterator icon and its label (100 pixels) @@ -868,7 +868,7 @@ func (fb *flowBuilder) addWhileStatement(s *ast.WhileStmt) model.ID { savedWhileAnnotations := fb.pendingAnnotations fb.pendingAnnotations = nil - bodyBounds := fb.measurer.measureStatements(s.Body) + bodyBounds := fb.measurer.measureStatementsSpan(s.Body) loopWidth := max(bodyBounds.Width+2*LoopPadding, MinLoopWidth) loopHeight := max(bodyBounds.Height+2*LoopPadding, MinLoopHeight) diff --git a/mdl/executor/layout.go b/mdl/executor/layout.go index 27ba8c01a..b2c694aa2 100644 --- a/mdl/executor/layout.go +++ b/mdl/executor/layout.go @@ -74,6 +74,43 @@ func (m *layoutMeasurer) measureStatements(stmts []ast.MicroflowStatement) Bound return Bounds{Width: totalWidth, Height: maxHeight} } +// measureStatementsSpan returns the horizontal extent a statement run actually +// occupies once laid out, for runs where that can be derived exactly. +// +// measureStatements sums every element's full width and adds HorizontalSpacing +// between them. HorizontalSpacing is a centre-to-centre pitch — the builder does +// `posX += spacing` and centres each activity on posX — so counting it *on top of* +// each width over-measures a run of n simple activities by (n-1)*ActivityWidth. +// Sizing a loop box from that left it far wider than its contents: 880px around +// 440px of activities for a three-statement body (mendixlabs/mxcli#790). +// +// The correction applies only when every element is a simple activity, whose pitch +// is exactly HorizontalSpacing. A compound element (IF/split, nested loop) advances +// posX by geometry this function cannot reproduce without duplicating the builder — +// guessing there under-sizes the box and pushes activities outside it, which is +// worse than a box that is too wide. Such runs fall back to measureStatements. +func (m *layoutMeasurer) measureStatementsSpan(stmts []ast.MicroflowStatement) Bounds { + count := 0 + maxHeight := ActivityHeight + for _, stmt := range stmts { + b := m.measureStatement(stmt) + maxHeight = max(maxHeight, b.Height) + if b.Width == 0 { + continue + } + if b.Width != ActivityWidth { + return m.measureStatements(stmts) // compound element — cannot place exactly + } + count++ + } + if count == 0 { + return Bounds{Width: 0, Height: maxHeight} + } + // n activities centred HorizontalSpacing apart span from the first centre minus + // half a width to the last centre plus half a width. + return Bounds{Width: (count-1)*HorizontalSpacing + ActivityWidth, Height: maxHeight} +} + // measureStatement calculates the bounds for a single statement func (m *layoutMeasurer) measureStatement(stmt ast.MicroflowStatement) Bounds { switch s := stmt.(type) { diff --git a/mdl/executor/layout_span_test.go b/mdl/executor/layout_span_test.go new file mode 100644 index 000000000..b0d616e00 --- /dev/null +++ b/mdl/executor/layout_span_test.go @@ -0,0 +1,75 @@ +// SPDX-License-Identifier: Apache-2.0 + +// mendixlabs/mxcli#790: a loop box was drawn far wider than its contents. +// measureStatements sums each element's full width and adds HorizontalSpacing +// between them, but HorizontalSpacing is a centre-to-centre pitch — the builder +// centres each activity on posX and advances by exactly that — so the sum +// over-counts a run of n simple activities by (n-1)*ActivityWidth. +package executor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" +) + +// simpleStmts returns n statements that each measure to one plain activity box. +func simpleStmts(n int) []ast.MicroflowStatement { + out := make([]ast.MicroflowStatement, 0, n) + for i := 0; i < n; i++ { + out = append(out, &ast.MfCommitStmt{}) + } + return out +} + +func TestMeasureStatementsSpan_SimpleRun(t *testing.T) { + m := &layoutMeasurer{} + tests := []struct { + n int + want int + }{ + {0, 0}, + {1, ActivityWidth}, // 120 + {2, HorizontalSpacing + ActivityWidth}, // 280 + {3, 2*HorizontalSpacing + ActivityWidth}, // 440 — was 680 + } + for _, tc := range tests { + got := m.measureStatementsSpan(simpleStmts(tc.n)).Width + if got != tc.want { + t.Errorf("span of %d activities = %d, want %d", tc.n, got, tc.want) + } + if tc.n > 1 { + // The old measure is the one that over-sized the loop box. + if old := m.measureStatements(simpleStmts(tc.n)).Width; old <= got { + t.Errorf("expected measureStatements (%d) to exceed the true span (%d)", old, got) + } + } + } +} + +// TestMeasureStatementsSpan_CompoundFallsBack: a compound element advances posX by +// geometry this measure cannot reproduce. Guessing there under-sizes the box and +// pushes activities outside it, so such runs must keep the conservative measure. +func TestMeasureStatementsSpan_CompoundFallsBack(t *testing.T) { + m := &layoutMeasurer{} + body := []ast.MicroflowStatement{ + &ast.MfCommitStmt{}, + &ast.IfStmt{ThenBody: simpleStmts(1)}, + &ast.MfCommitStmt{}, + } + span := m.measureStatementsSpan(body).Width + full := m.measureStatements(body).Width + if span != full { + t.Errorf("compound run: span = %d, want the conservative %d", span, full) + } +} + +// TestMeasureStatementsSpan_ZeroWidthIgnored: a RETURN produces no box, so it must +// not contribute a pitch step. +func TestMeasureStatementsSpan_ZeroWidthIgnored(t *testing.T) { + m := &layoutMeasurer{} + withReturn := []ast.MicroflowStatement{&ast.MfCommitStmt{}, &ast.ReturnStmt{}, &ast.MfCommitStmt{}} + if got, want := m.measureStatementsSpan(withReturn).Width, m.measureStatementsSpan(simpleStmts(2)).Width; got != want { + t.Errorf("span with a zero-width statement = %d, want %d", got, want) + } +}