From cd2b01e87f7f4fb464991d5c19e05e260180f3e7 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 21:43:32 +0000 Subject: [PATCH 1/6] docs: propose deriving container Size from contents (#884 problem 1) A LoopedActivity's Size is computed from a pre-pass over the AST before its body is built, so it is a function of statement count alone and the children's real positions -- @position included -- have no effect on the box meant to contain them. Measured on 11.6.6: two activities at x=150/310, at x=1500/2000, and at x=160/170 all yield Size 480;160; only changing the statement count to four moves it (800;160). In the x=1500/2000 case both children sit entirely outside their own container and mx check reports no additional error, so nothing catches it short of opening the flow in Studio Pro. The fix is not a one-liner: children are placed relative to an inner origin derived from the size, so deriving the size from the children closes a cycle. Proposes build-first / size-after / translate-once, with the translation as a single post-pass over the nested builder's objects -- the same single-choke-point shape used for @curve and @merge. Includes the four-case repro as a bug-test example, and notes the two consequences worth a release note: every flow containing a loop changes geometry once, and under ADR-0008 that means one round of writes users did not author. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_013uQvFDd5R4eNqqita59jM8 --- .../PROPOSAL_container_autosize.md | 188 ++++++++++++++++++ .../bug-tests/container-autosize-884.mdl | 59 ++++++ 2 files changed, 247 insertions(+) create mode 100644 docs/11-proposals/PROPOSAL_container_autosize.md create mode 100644 mdl-examples/bug-tests/container-autosize-884.mdl diff --git a/docs/11-proposals/PROPOSAL_container_autosize.md b/docs/11-proposals/PROPOSAL_container_autosize.md new file mode 100644 index 000000000..a5e4f06c2 --- /dev/null +++ b/docs/11-proposals/PROPOSAL_container_autosize.md @@ -0,0 +1,188 @@ +--- +title: Size containers from their contents, not from a statement count +status: draft +--- + +# Proposal: Size containers from their contents, not from a statement count + +Closes the last open item of upstream [#884](https://github.com/mendixlabs/mxcli/issues/884) — +problem 1, "container `Size` is unrelated to the positions of the activities inside it". + +## Problem + +A `LoopedActivity` is the only microflow element mxcli sizes rather than stamping +with a constant. Its `Size` is computed **before** its body is built, from a +pre-pass over the AST that counts statements. The nested builder that actually +places the children runs afterwards, by which time the box is frozen — so child +positions, including explicit `@position` overrides, have **no** influence on the +box that is supposed to contain them. + +### Measured + +Four microflows, one project, blank Mendix 11.6.6 app, mxcli `bcc3406` +(`mdl-examples/bug-tests/container-autosize-884.mdl`; `LOOP` body varied, +everything else identical): + +| | Body | Child centres (X) | Loop `Size` | +|---|---|---|---| +| **A** | 2 activities, default placement | 150, 310 | `480;160` | +| **B** | same 2, `@position(1500,60)` / `@position(2000,60)` | 1500, 2000 | `480;160` | +| **C** | same 2, `@position(160,60)` / `@position(170,60)` | 160, 170 | `480;160` | +| **D** | 4 activities, default placement | 150 … 630 | `800;160` | + +A, B and C are the same box around contents spanning 160px, 500px and 10px. +D differs from A only in statement count, and only D changes the box. Width is +exactly `(n-1)·HorizontalSpacing + ActivityWidth + 2·LoopPadding + iteratorSpace` +— a function of `n` alone. + +**B is a correctness problem, not a cosmetic one.** The box interior spans +x ∈ [0, 480]; both children sit at 1500 and 2000, entirely outside their own +container. `mx check` reports the *same* error count before and after the script +runs — validation does not model geometry, so nothing catches it. It is visible +only by opening the flow in Studio Pro. + +This is what the reporter observed as "identical child positions, different +`Size`" — the two quantities are simply not connected. + +### Where it comes from + +`measureStatementsSpan` (`mdl/executor/layout.go:92`) takes +`[]ast.MicroflowStatement` and returns `(count-1)*HorizontalSpacing + ActivityWidth`. +It is called at exactly two places, both of which size a container: + +- `mdl/executor/cmd_microflows_builder_control.go:572` — `addLoopStatement` +- `mdl/executor/cmd_microflows_builder_control.go:896` — `addWhileStatement` + +Neither has any child object at that point. `loopBuilder` — the nested +`flowBuilder` whose `objects` carry the real positions — is constructed about +twenty lines later. + +The comment on `measureStatementsSpan` is honest about the half-fix it already +is (#790): the count-based span was introduced to *stop over-sizing* boxes, and +it falls back to the older `measureStatements` for compound bodies precisely +because it "cannot reproduce the builder's geometry without duplicating it". +That is the tell — the pre-pass is trying to predict a computation that has not +run yet. + +## Why this is not a one-line change + +The current ordering contains a cycle that the obvious fix walks straight into: + +```go +loopHeight := max(bodyBounds.Height+2*LoopPadding, MinLoopHeight) // needs body +innerStartY := loopHeight / 2 // body needs this +loopLeftX := fb.posX +loopCenterX := loopLeftX + loopWidth/2 // needs width +``` + +Children are placed relative to an inner origin derived **from the size**, and +under the fix the size must be derived **from the children**. Anything that +computes the size from real positions has to break that dependency first, which +is why this is a layout-engine change rather than a patch. + +## Proposal + +**Build first, size after, translate once.** + +1. **Build the body at a provisional origin** — run `loopBuilder` from `(0, 0)`. + Nothing in the body placement genuinely needs the box; it needs *an* origin. +2. **Take the real bounding box** from `loopBuilder.objects` + (`Position ± Size/2` over every object, recursively for nested containers). + This is ground truth, and it accounts for `@position` for free — no annotation + plumbing required. +3. **Size the box** as bbox + `LoopPadding` on all sides, plus `iteratorSpace` on + the left for `LOOP`, floored at `MinLoopWidth` / `MinLoopHeight`. +4. **Translate the body once** — a single pass over `loopBuilder.objects` adding + the delta between the provisional origin and the final inner origin, mutating + `Position` only. + +Step 4 is what keeps this safe. No builder call site learns about sizing; the +translation is a pure post-pass over objects that already exist. That is the +same single-choke-point shape used for `@curve` (`applyFlowCurves`) and `@merge` +(`mergePosition`) — chosen for the same reason: a rule threaded through N +placement sites fails silently at the one site that was missed. + +### Nested containers + +The bbox must be computed bottom-up: an inner loop has to be sized before the +outer loop measures it. This already falls out of the recursion — `addStatement` +sizes the inner `LoopedActivity` before it is appended to +`loopBuilder.objects` — but it becomes load-bearing under this change and should +be asserted by a test with two nesting levels, not left to hold by accident. + +### Semantics of `@position` inside a container + +Two readings, and they must be decided explicitly rather than emerging from the +implementation: + +- **(a) container-relative, translated with everything else** — the box grows to + contain the annotated child, and the child keeps its position *relative to the + body*. +- **(b) authoritative, exempt from translation** — the annotated child stays put + and unannotated siblings move around it. + +**Recommend (a).** Positions inside a `LoopedActivity` are already stored +relative to the container, so (a) matches the storage model; (b) would leave two +adjacent children obeying different origins, which is unexplainable in a doc and +unpredictable in a diff. Whichever is chosen, it belongs in +`.claude/skills/mendix/` alongside the `@position` reference. + +## Non-goals + +- **Splits.** `IF` / enum split / inheritance split have no `Size` property in + Mendix storage — there is no box, so there is nothing to fix. `measureStatements` + stays: it is still needed to reserve horizontal room for branches *before* they + are laid out, which is a genuinely predictive use. +- **Re-flowing a body to fit a box.** Out of scope. The box follows the contents, + never the reverse. + +## Alternative considered: an `@size(w, h)` escape hatch + +The reporter asked for this directly, and it is far cheaper — one annotation, one +field, no layout change. It is rejected as the *primary* fix for two reasons: it +makes the author responsible for a number the engine can compute, and it leaves +the default behaviour wrong for everyone who does not use it. + +Worth revisiting **after** the derived size lands, if a real case wants a box +deliberately larger than its contents. Note the coupling introduced by the #884 +annotation work: `@size` is currently **rejected** by MDL059, so adding it means +adding it to `knownActivityAnnotations` in `mdl/executor/validate_microflow.go`. +The drift test compares the visitor's case labels against that list in both +directions and fails if only one side changes — which is the intent. + +## Risk + +**Every generated flow containing a `LOOP` or `WHILE` changes geometry.** That is +unavoidable for a fix of this shape, and has two consequences worth stating in +the release note: + +- Fixtures and golden BSON comparisons churn once, in a single commit. +- Under idempotent writes (ADR-0008), the first re-run of any existing script + against an existing project **writes** the affected microflows; subsequent runs + go quiet again. Users will see one round of version-control changes they did + not author. + +Neither is a reason not to do it, but a silent geometry shift across an existing +project is exactly the kind of thing that gets reported as a new bug. + +## Verification + +- **Regression test from the table above.** Cases A–D as a builder-level test + asserting the invariant directly: *every child's bounding box lies inside its + parent's*. That is the property; the specific pixel values are not. +- **Nesting test** — loop inside a loop, both boxes containing their contents. +- **`@position` test** — case B: the box must grow to contain a child pushed to + x=2000, not leave it outside. +- **Control run.** Per the standing rule, the test must be shown to fail against + a pre-fix binary with the reported symptom — a same-size box around different + contents — and not merely pass against fixed code. +- **`mx check`** on every fixture, both engines. + +### Candidate lint rule: MPR009 + +The measurement above shows `mx check` is blind to this. A rule +*"every child of a `LoopedActivity` lies within its parent's box"* would have +caught the entire class from the outside, and would keep catching it if a future +layout change reintroduces it. It sits naturally beside MPR008 (which, after the +#884 work, already partitions objects by canvas and therefore has the container +geometry in hand). Proposed as a follow-up, not part of this change. diff --git a/mdl-examples/bug-tests/container-autosize-884.mdl b/mdl-examples/bug-tests/container-autosize-884.mdl new file mode 100644 index 000000000..66201f237 --- /dev/null +++ b/mdl-examples/bug-tests/container-autosize-884.mdl @@ -0,0 +1,59 @@ +-- upstream #884 problem 1: a LoopedActivity's Size is a function of statement +-- count alone -- child positions, including explicit @position, have no effect. +-- +-- Measured on Mendix 11.6.6 (mxcli bcc3406): A, B and C all produce Size +-- 480;160 while their contents span 160px, 500px and 10px respectively. In B +-- both children sit outside the box (interior x in [0,480], children at 1500 +-- and 2000) and `mx check` reports no additional error. +-- +-- See docs/11-proposals/PROPOSAL_container_autosize.md + +CREATE ENTITY MyFirstModule.SzItem ( + Name: String(100) +); + +-- A: default child placement +CREATE MICROFLOW MyFirstModule.MF_SizeA () +BEGIN + RETRIEVE $Items FROM MyFirstModule.SzItem; + LOOP $Item IN $Items BEGIN + LOG INFO 'one'; + LOG INFO 'two'; + END LOOP; +END; + +-- B: identical body, children pushed far outside the box +CREATE MICROFLOW MyFirstModule.MF_SizeB () +BEGIN + RETRIEVE $Items FROM MyFirstModule.SzItem; + LOOP $Item IN $Items BEGIN + @position(1500, 60) + LOG INFO 'one'; + @position(2000, 60) + LOG INFO 'two'; + END LOOP; +END; + +-- C: identical body, children pulled tightly together +CREATE MICROFLOW MyFirstModule.MF_SizeC () +BEGIN + RETRIEVE $Items FROM MyFirstModule.SzItem; + LOOP $Item IN $Items BEGIN + @position(160, 60) + LOG INFO 'one'; + @position(170, 60) + LOG INFO 'two'; + END LOOP; +END; + +-- D: control -- only the statement count differs from A, and only D resizes +CREATE MICROFLOW MyFirstModule.MF_SizeD () +BEGIN + RETRIEVE $Items FROM MyFirstModule.SzItem; + LOOP $Item IN $Items BEGIN + LOG INFO 'one'; + LOG INFO 'two'; + LOG INFO 'three'; + LOG INFO 'four'; + END LOOP; +END; From 899134489fa869b6145279e748c021e5866bb4b7 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 05:51:00 +0000 Subject: [PATCH 2/6] fix(microflow): correct a false claim about how Studio Pro stores a retrieve Range MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit parseRange carried the comment "Studio Pro stores custom ranges as ConstantRange with LimitExpression/OffsetExpression", and a branch acting on it. Measured against a real document — ako/TestApp MyFirstModule.RetrieveExamples on Mendix 11.13.0, three retrieves, one per UI option: All ConstantRange {SingleObject: false} First ConstantRange {SingleObject: true} Custom (limit 4, off 2) CustomRange {LimitExpression: "4", OffsetExpression: "2"} LimitExpression never appears on a ConstantRange. Studio Pro stores exactly what generated/metamodel and modelsdk/gen declare, so the branch has never fired. The comment was not harmless. The two engines differ on that input — modelsdk's rangeFromGen cannot read it at all, because gen binds only SingleObject on ConstantRange — so a reader trusting the comment concludes the default engine silently drops a bounded retrieve's limit. It does not. The claim nearly became a bug report against correct code. The branch is kept, because one version's evidence justifies correcting a false claim but not deleting tolerance for pre-11 formats that have not been sampled. It is now labelled as unobserved, with the asymmetry and its consequences recorded: if such a document ever appears the fix belongs in gen, not here. Tests pin the three measured shapes, and name the tolerance case so a future discovery lands somewhere with context. Also verified, on both engines, that a describe -> exec round trip reproduces all three byte-for-byte — DESCRIBE renders First as `limit 1`, which maps back to ConstantRange{SingleObject:true} rather than a CustomRange, so the object-vs-list distinction survives. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01LUToAkUx54bNkNjsBpufRH --- sdk/mpr/parser_microflow.go | 21 +++++++- sdk/mpr/parser_range_test.go | 95 ++++++++++++++++++++++++++++++++++++ 2 files changed, 115 insertions(+), 1 deletion(-) create mode 100644 sdk/mpr/parser_range_test.go diff --git a/sdk/mpr/parser_microflow.go b/sdk/mpr/parser_microflow.go index a417af5e1..4d2947f99 100644 --- a/sdk/mpr/parser_microflow.go +++ b/sdk/mpr/parser_microflow.go @@ -914,12 +914,31 @@ func parseRange(raw map[string]any) *microflows.Range { switch typeName { case "Microflows$ConstantRange": + // MEASURED (Mendix 11.13.0, ako/TestApp MyFirstModule.RetrieveExamples — + // three retrieves, one per UI option): + // + // All ConstantRange{SingleObject:false} + // First ConstantRange{SingleObject:true} + // Custom CustomRange{LimitExpression, OffsetExpression} + // + // So a ConstantRange carries ONLY SingleObject, exactly as + // generated/metamodel and modelsdk/gen declare it, and the Limit/Offset + // read below has never been observed to fire. It is kept as tolerance + // for formats we have not sampled (no pre-11 document has been checked), + // NOT because Studio Pro is known to write that shape — an earlier + // comment here asserted it did, which is false for 11.13 and nearly cost + // a phantom bug report against the other engine. + // + // The engines differ on this input and that is deliberate: modelsdk's + // rangeFromGen cannot read it at all, because gen binds only + // SingleObject on ConstantRange. If a real document ever turns up with + // Limit on a ConstantRange, that asymmetry becomes a data-loss bug and + // gen needs a property override — see CLAUDE.md on gen's wrong keys. r.Limit = extractString(raw["LimitExpression"]) r.Offset = extractString(raw["OffsetExpression"]) if singleObject := extractBool(raw["SingleObject"], false); singleObject { r.RangeType = microflows.RangeTypeFirst } else if r.Limit != "" || r.Offset != "" { - // Studio Pro stores custom ranges as ConstantRange with LimitExpression/OffsetExpression r.RangeType = microflows.RangeTypeCustom } else { r.RangeType = microflows.RangeTypeAll diff --git a/sdk/mpr/parser_range_test.go b/sdk/mpr/parser_range_test.go new file mode 100644 index 000000000..134997f89 --- /dev/null +++ b/sdk/mpr/parser_range_test.go @@ -0,0 +1,95 @@ +// SPDX-License-Identifier: Apache-2.0 + +package mpr + +import ( + "testing" + + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// TestParseRange_StudioProShapes pins how Studio Pro actually stores a database +// retrieve's Range, measured rather than assumed. +// +// Source: ako/TestApp, MyFirstModule.RetrieveExamples on Mendix 11.13.0 — one +// retrieve per UI option, dumped with `mxcli bson dump --type microflow`: +// +// All Microflows$ConstantRange {SingleObject:false} +// First Microflows$ConstantRange {SingleObject:true} +// Custom (limit 4, off 2) Microflows$CustomRange {LimitExpression:"4", OffsetExpression:"2"} +// +// This matters beyond the parser. Range is a POLYMORPHIC child, the shape that +// has produced repeated data loss when a reader pulls one scalar out of it +// without dispatching on $Type (DomainModels$RuleInfo, and the import-mapping +// Range in #881). Pinning the real variants is what makes the dispatch here +// checkable instead of folkloric. +func TestParseRange_StudioProShapes(t *testing.T) { + tests := []struct { + name string + raw map[string]any + wantType microflows.RangeType + wantLimit string + wantOffset string + }{ + { + name: "All", + raw: map[string]any{"$Type": "Microflows$ConstantRange", "SingleObject": false}, + wantType: microflows.RangeTypeAll, + }, + { + name: "First", + raw: map[string]any{"$Type": "Microflows$ConstantRange", "SingleObject": true}, + wantType: microflows.RangeTypeFirst, + }, + { + name: "Custom", + raw: map[string]any{ + "$Type": "Microflows$CustomRange", + "LimitExpression": "4", + "OffsetExpression": "2", + }, + wantType: microflows.RangeTypeCustom, + wantLimit: "4", + wantOffset: "2", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := parseRange(tt.raw) + if got == nil { + t.Fatal("parseRange returned nil") + } + if got.RangeType != tt.wantType { + t.Errorf("RangeType = %v, want %v", got.RangeType, tt.wantType) + } + if got.Limit != tt.wantLimit { + t.Errorf("Limit = %q, want %q", got.Limit, tt.wantLimit) + } + if got.Offset != tt.wantOffset { + t.Errorf("Offset = %q, want %q", got.Offset, tt.wantOffset) + } + }) + } +} + +// TestParseRange_ConstantRangeWithLimitIsUnobserved documents the tolerance +// branch rather than endorsing it. +// +// No Studio Pro document has been seen storing Limit/Offset on a ConstantRange; +// the branch exists only for formats we have not sampled. modelsdk cannot read +// this shape at all (gen binds only SingleObject on ConstantRange), so if it +// ever turns up in a real project the engines diverge and the fix belongs in +// gen, not here. This test exists so that discovery lands on a named case. +func TestParseRange_ConstantRangeWithLimitIsUnobserved(t *testing.T) { + got := parseRange(map[string]any{ + "$Type": "Microflows$ConstantRange", + "SingleObject": false, + "LimitExpression": "10", + }) + if got.RangeType != microflows.RangeTypeCustom || got.Limit != "10" { + t.Errorf("legacy tolerance changed: got %v/%q — if this is now intended, "+ + "check whether modelsdk's rangeFromGen was taught to read it too", + got.RangeType, got.Limit) + } +} From 64935d48b2d682fc26c4b8e46dac4991389be2fb Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 07:11:09 +0000 Subject: [PATCH 3/6] chore(lsp): regenerate completions for the FIRST keyword The import-mapping Range work (#881) added FIRST to the lexer but the committed generated completion list was never refreshed, so `make build` left the tree dirty on a clean checkout. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_013uQvFDd5R4eNqqita59jM8 --- cmd/mxcli/lsp_completions_gen.go | 1 + 1 file changed, 1 insertion(+) diff --git a/cmd/mxcli/lsp_completions_gen.go b/cmd/mxcli/lsp_completions_gen.go index bdbf53f4b..51df6704e 100644 --- a/cmd/mxcli/lsp_completions_gen.go +++ b/cmd/mxcli/lsp_completions_gen.go @@ -86,6 +86,7 @@ var mdlGeneratedKeywords = []protocol.CompletionItem{ {Label: "FROM", Kind: protocol.CompletionItemKindKeyword, Detail: "Query keyword"}, {Label: "WHERE", Kind: protocol.CompletionItemKindKeyword, Detail: "Query keyword"}, {Label: "HAVING", Kind: protocol.CompletionItemKindKeyword, Detail: "Query keyword"}, + {Label: "FIRST", Kind: protocol.CompletionItemKindKeyword, Detail: "Query keyword"}, {Label: "OFFSET", Kind: protocol.CompletionItemKindKeyword, Detail: "Query keyword"}, {Label: "LIMIT", Kind: protocol.CompletionItemKindKeyword, Detail: "Query keyword"}, {Label: "AS", Kind: protocol.CompletionItemKindKeyword, Detail: "Query keyword"}, From cad70ccafda9828e5cd027c11fa8d86f1f2b509b Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 11:23:33 +0000 Subject: [PATCH 4/6] build: Go toolchain 1.26.5 -> 1.26.6 for six stdlib advisories The `Vulnerability scan` step (govulncheck ./...) started failing with exit 3 on every push. Nothing in mxcli changed: all six findings are standard-library advisories reported as `Found in: @go1.26.5` / `Fixed in: @go1.26.6`, and go1.26.6 was published between the last green run and the first red one. govulncheck@latest re-resolves the vuln database on each run, so the pinned toolchain went stale underneath a workflow that had not been touched. GO-2026-6218 net/url quadratic complexity in resolvePath GO-2026-6090 crypto/tls unbounded post-handshake messages GO-2026-6089 net/http ReadHeaderTimeout on the h2c check GO-2026-6088 encoding/xml missing recursion depth guard GO-2026-5972 encoding/asn1 missing recursion depth limit GO-2026-5026 net/http idna punycode label rejection Bumps every pin together -- go.mod's toolchain plus push-test, release and nightly (two jobs) -- so a release binary is not still linked against the vulnerable standard library after CI goes green. Same treatment as the 1.26.4 -> 1.26.5 bump for GO-2026-5856. Verified by running the scan under both toolchains: go1.26.5 reports the six above and exits 3, go1.26.6 reports "No vulnerabilities found" and exits 0. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_013uQvFDd5R4eNqqita59jM8 --- .github/workflows/nightly.yml | 4 ++-- .github/workflows/push-test.yml | 2 +- .github/workflows/release.yml | 2 +- CHANGELOG.md | 4 ++++ go.mod | 2 +- 5 files changed, 9 insertions(+), 5 deletions(-) diff --git a/.github/workflows/nightly.yml b/.github/workflows/nightly.yml index 4e021ad4e..377422b41 100644 --- a/.github/workflows/nightly.yml +++ b/.github/workflows/nightly.yml @@ -22,7 +22,7 @@ jobs: - uses: actions/setup-go@v7 with: - go-version: '1.26.5' + go-version: '1.26.6' - name: Cache ANTLR4 JAR uses: actions/cache@v6 @@ -58,7 +58,7 @@ jobs: - uses: actions/setup-go@v7 with: - go-version: '1.26.5' + go-version: '1.26.6' - uses: oven-sh/setup-bun@v2 diff --git a/.github/workflows/push-test.yml b/.github/workflows/push-test.yml index 455d4cdf1..e8b377a50 100644 --- a/.github/workflows/push-test.yml +++ b/.github/workflows/push-test.yml @@ -12,7 +12,7 @@ jobs: - uses: actions/checkout@v7 - uses: actions/setup-go@v7 with: - go-version: '1.26.5' + go-version: '1.26.6' - name: Cache ANTLR4 JAR uses: actions/cache@v6 with: diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 78df8e1d9..d430adb50 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -17,7 +17,7 @@ jobs: fetch-depth: 0 - uses: actions/setup-go@v7 with: - go-version: '1.26.5' + go-version: '1.26.6' - uses: oven-sh/setup-bun@v2 - name: Cache ANTLR4 JAR uses: actions/cache@v6 diff --git a/CHANGELOG.md b/CHANGELOG.md index 58c9e2c6a..dde975e12 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] +### Changed + +- **Go toolchain 1.26.5 → 1.26.6** for GO-2026-6218 (`net/url`), GO-2026-6090 (`crypto/tls`), GO-2026-6089 (`net/http`), GO-2026-6088 (`encoding/xml`), GO-2026-5972 (`encoding/asn1`) and GO-2026-5026 (`net/http`, via `golang.org/x/net/idna`). All six are standard-library advisories fixed in go1.26.6; no mxcli code changed. Bumped in `go.mod` and in all three workflows (`push-test`, `release`, `nightly`) together, so released binaries are not still linked against the vulnerable standard library. + ## [0.17.0] - 2026-08-10 Headline: **A full Mendix build-and-test loop that fits on an iPad** — you can now design, build, run, observe, and debug a multi-app Mendix solution end-to-end from Claude Code on the web, on a phone or tablet, with no local IDE. Two capabilities make it possible: an **external browser preview** that reverse-tunnels a locally-running app out to a public URL from an egress-only container, and a **short agentic feedback loop** — a warm Docker-free runtime, sub-second microflow unit tests, live log/metric/trace observation, and a name-based microflow debugger — so an agent gets an answer in seconds instead of a build round-trip. diff --git a/go.mod b/go.mod index ab948e40c..5058003a9 100644 --- a/go.mod +++ b/go.mod @@ -2,7 +2,7 @@ module github.com/mendixlabs/mxcli go 1.26.0 -toolchain go1.26.5 +toolchain go1.26.6 require ( github.com/alecthomas/chroma/v2 v2.26.1 From 9d0ac4686a79e72fa9cce08c419d71a0a715c297 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 11:29:39 +0000 Subject: [PATCH 5/6] Round-trip a pluggable widget through DESCRIBE PAGE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A page carrying a pluggable widget could not be described and re-applied: `mxcli check` on the description failed from the first widget onward with `extraneous input ':'`. That takes any such page out of the DESCRIBE-edit-CREATE OR REPLACE workflow mxcli itself documents as the way to change an element you did not author — and a widget has no other route, having never been hand-authorable in Studio Pro terms. Two independent defects in the same path: - Emit. Explicit properties went out through a raw %s, so every string lost its quotes. A JSON spec then broke the parse at its first brace. - Read. extractExplicitProperties skipped any value of "true"/"false" as a "common default". A widget's default may be either, the document only stores what the author set, and the property was simply gone. Both are fixed together because fixing one is worse than fixing neither: quote without emitting booleans and the description re-parses cleanly while silently dropping a property — a wrong page that validates. Quoting is decided by the DECLARED type, never the value's shape. Each property's ValueType.Type is already in the widget's Type.ObjectType.PropertyTypes, the array buildPropertyTypeKeyMap walks for PropertyKey and throws away; a String holding "30" is indistinguishable from an Integer once it is a string in BSON, and has to come back quoted. Where no type is declared the shape is all that is left, and the fallback quotes anything not plainly numeric or boolean — an unquoted arbitrary string may not parse at all, while a quoted literal still round-trips. Verified end to end on 11.12.1, which is the test rather than the output: describe -> check -> exec -> describe is byte-identical, and mx check reports 0 errors. Before the fix the first step did not parse. Uncovered while verifying and NOT fixed here, because it is a write-path defect with no DESCRIBE involved: giving a property in a conditionally shown group a non-default value writes a widget Mendix rejects with CE0463. On ProgressCircle both `showLabel: true` and `labelType: 'percentage'` do it, while the same widget's General-group properties take non-default values happily. The example keeps that group at its defaults so it isolates the round trip; the symptom table records the repro. Reported in mxcli-ledger FINDINGS #104. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JXnEgoc2NQP1Y2TWMCMXC4 --- .claude/skills/fix-issue.md | 1 + .../pluggable-describe-roundtrip.mdl | 39 +++++ mdl/executor/cmd_pages_describe.go | 6 + mdl/executor/cmd_pages_describe_output.go | 73 ++++++++- mdl/executor/cmd_pages_describe_pluggable.go | 56 ++++++- ...pages_describe_pluggable_roundtrip_test.go | 149 ++++++++++++++++++ 6 files changed, 316 insertions(+), 8 deletions(-) create mode 100644 mdl-examples/bug-tests/pluggable-describe-roundtrip.mdl create mode 100644 mdl/executor/cmd_pages_describe_pluggable_roundtrip_test.go diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index 596f2c9a4..ea81ee81a 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -512,3 +512,4 @@ extracting `OffsetExpression`/`LimitExpression`. | A hand-curved sequence flow does not survive a rewrite — `DESCRIBE` output is identical before and after the curve is drawn in Studio Pro, and re-running unchanged MDL straightens the edge | Mendix stores NO waypoints. A flow's shape is two bezier control vectors on its `Microflows$BezierCurve` line (`OriginControlVector`/`DestinationControlVector`, `"x;y"`). Both writers already emitted them and the legacy parser already read them, but nothing could SET them, so they defaulted to `"0;0"` and every rebuild flattened the curve — measured by patching `"40;-90"` into the stored line and re-executing the same script, which returned `"0;0"` | `mdl/ast/ast_microflow.go` (`FlowCurve`, `ActivityAnnotations.Curve`/`InvalidCurves`), `mdl/visitor/visitor_microflow_statements.go` (`parseCurveAnnotation`, `annotationPointValue`), `mdl/executor/cmd_microflows_builder_annotations.go` (`curveByOrigin`, `applyFlowCurves`, and the `mergeStatementAnnotations` copy), `cmd_microflows_builder_graph.go` (the one call), `cmd_microflows_show_helpers.go` (`emitCurveAnnotation`), `mdl/backend/modelsdk/microflow.go` (read the Line back) | **Check what the storage can represent before designing the syntax** — the request asked for "edge waypoints", which do not exist; `SequenceFlow` has two control vectors, so `@curve(from: (x, y), to: (x, y))` is the only shape that maps. Same lesson as #872's anchors. **Look for an existing annotation shape before touching the grammar**: `name: (x, y)` is already `annotationParenValue`, so `@curve` needed ZERO grammar changes. **Record against the ACTIVITY and stamp flows in one pass** — threading a curve alongside the anchor would mean editing all seven sites that create a flow (`previousStmtAnchor`, `nextFlowAnchor`, branch and loop variants) and missing one silently straightens that edge; `applyPendingAnnotations` already runs at every activity, so there is exactly one place to get right. **A new field on `ActivityAnnotations` must be copied in `mergeStatementAnnotations`** — it is an explicit field-by-field copy, so the first cut parsed the curve and wrote nothing. **Unit tests that call the function directly do NOT prove the wiring**: deleting both call sites left `TestEmitCurveAnnotation` and `TestApplyFlowCurves…` green, which is the "a test that only passes against fixed code" trap in its purest form — `TestCurveReachesTheFlowThroughTheBuilder` (MDL text → real builder) and `TestCurveIsEmittedByTheAnnotationEmitter` (through `emitObjectAnnotations`) fail when unwired. **Still open**: a curve drawn in Studio Pro is preserved only once the script names it, since a rebuilt flow has no stable identity to match on; DESCRIBE now surfaces it so it can be captured. Tests `mdl/executor/cmd_microflows_curve_test.go`, example `mdl-examples/bug-tests/884-annotations-and-lint-planes.mdl`. upstream #884 | | `mxcli run --local --ensure-db` fails on a freshly built `mxcli init` dev container — there is no PostgreSQL service to start and no `postgres` superuser — even though `psql` is on PATH | The generated Dockerfile installed `postgresql-client` **only**. `EnsureDatabase` starts a local service (`service postgresql start`) and provisions the role + database through `sudo -u postgres psql`, both of which need the **server** package. `psql` being present makes the container look correctly provisioned | `cmd/mxcli/tool_templates.go` (`generateDockerfile`) | Install `postgresql` alongside `postgresql-client`. **Generalisable**: when a feature shells out to a service, assert the *server* package in the image template, not the client that happens to satisfy a `LookPath` check — the CI/web image having the server is what hid this (`/usr/lib/postgresql/16` is present there but not in the generated dev container). Guarded by `TestGenerateDockerfile_PostgresServer`, which asserts the server package for both the docker and podman variants | | The implicit merge that closes a split — the end-if join — is placed by the layout pass and cannot be moved; it routinely lands on top of a neighbouring activity, and `@position` does not address it | The statement's own `@position` belongs to the SPLIT, so the merge had no annotation of its own. Three split builders computed `mergeX/centerY` with no override (`addIfStatement`, `addEnumSplit`, `addStructuredInheritanceSplit`), and DESCRIBE emitted nothing for it, so even a hand-moved merge was recomputed on the next exec | `mdl/ast/ast_microflow.go` (`ActivityAnnotations.Merge`), `mdl/visitor/visitor_microflow_statements.go` (`case "merge"`), `mdl/executor/cmd_microflows_builder_annotations.go` (`mergePosition`, and the `mergeStatementAnnotations` copy), the three split builders, `cmd_microflows_show_helpers.go` (`emitMergeAnnotation`, `commonMergeAfter`) | **Authoring without the DESCRIBE half is not a fix** — the first attempt shipped `@merge` writing correctly and was REVERTED, because the describer drops it and the layout pass then recomputes the merge on the next exec; that is the same round-trip data loss as #872/#881/#882, introduced by the change meant to help. **Find the relationship from data already in scope rather than threading a map**: the describer's `splitMergeMap` is not available at `emitObjectAnnotations`, and threading it would mean editing ten-plus call sites (the multi-site trap); `commonMergeAfter` walks the split's branches to the nearest merge reachable from ALL of them, using the `flowsByOrigin` and `activityMap` already passed in. **Bound any walk over a flow graph** — a retry loop makes it cyclic, so the walk carries a per-branch visited set and a node cap, pinned by `TestCommonMergeAfterTerminatesOnACycle`. **One helper for every site that places the merge**, so the override cannot be honoured at one split type and ignored at another. **Test the WIRING, not the helper**: `mergePosition` and `emitMergeAnnotation` called directly pass with every call site removed — `TestMergeReachesTheCanvasThroughTheBuilder` (MDL text → real builder) and `TestMergeIsEmittedByTheAnnotationEmitter` (through `emitObjectAnnotations`) fail when unwired, verified by removing each. Tests `mdl/executor/cmd_microflows_merge_test.go`, example `mdl-examples/bug-tests/884-annotations-and-lint-planes.mdl`. upstream #884 | +| `DESCRIBE PAGE` output will not re-parse for any page carrying a **pluggable widget**: `mxcli check` on it fails with `extraneous input ':'` / `extraneous input '('` from the first widget onward. Separately, a boolean property the author set is missing from the description entirely | Two independent defects in the same path. **Emit**: explicit properties were written with a raw `%s`, so every string lost its quotes — a JSON `spec: {"a": 1}` then broke the parse at its first brace. **Read**: `extractExplicitProperties` skipped any value of `"true"`/`"false"` as a "common default", so booleans never reached the output | `mdl/executor/cmd_pages_describe_output.go` (`explicitPropValue`, `isBareLiteral`), `mdl/executor/cmd_pages_describe_pluggable.go` (`buildPropertyValueTypeMap`, `extractExplicitProperties`), `mdl/executor/cmd_pages_describe.go` (`rawExplicitProp.ValueType`) | **Fixing one half alone is worse than the bug.** Quote without emitting booleans and the description re-parses cleanly while silently dropping a property — a wrong page that validates. Both halves ship together or neither. **Quote by the DECLARED type, never the value's shape**: `ValueType.Type` sits in the widget's `Type.ObjectType.PropertyTypes`, the same array `buildPropertyTypeKeyMap` already walks for `PropertyKey` and throws away; a String property holding `"30"` or `"true"` is indistinguishable from a number once it is a string in BSON, and must still come back quoted. Where no type is declared, fall back to the value's shape and quote anything not plainly numeric or boolean — quoting is the safe direction, since an unquoted arbitrary string may not parse at all. **The round trip is the test, not the output**: describe → `check` → `exec` → describe must be byte-identical and leave `mx check` at 0 errors. Tests `mdl/executor/cmd_pages_describe_pluggable_roundtrip_test.go`; example `mdl-examples/bug-tests/pluggable-describe-roundtrip.mdl`, verified end to end on 11.12.1. **Uncovered while verifying, NOT fixed**: giving a property in a conditionally shown group a non-default value writes a widget Mendix rejects with CE0463 — ProgressCircle's `showLabel: true` and `labelType: 'percentage'` both do it with no DESCRIBE involved, while the same widget's General-group properties take non-default values happily. Reported in mxcli-ledger FINDINGS #104 | diff --git a/mdl-examples/bug-tests/pluggable-describe-roundtrip.mdl b/mdl-examples/bug-tests/pluggable-describe-roundtrip.mdl new file mode 100644 index 000000000..fe22dbf5d --- /dev/null +++ b/mdl-examples/bug-tests/pluggable-describe-roundtrip.mdl @@ -0,0 +1,39 @@ +-- Ledger #104: DESCRIBE PAGE must round-trip a pluggable widget. +-- +-- Before the fix, describing this page emitted every property with no quotes +-- (a JSON `spec: {"a": 1}` broke the re-parse at its first brace) and dropped +-- every boolean, so the description could not be fed back through `exec`. +-- +-- Verify: +-- mxcli exec pluggable-describe-roundtrip.mdl -p app.mpr +-- mxcli -p app.mpr -c "DESCRIBE PAGE MyFirstModule.BT_PluggableRoundTrip" \ +-- | sed -n '/^create or modify page/,$p' > rt.mdl +-- mxcli check rt.mdl # passes; before the fix it did not parse +-- mxcli exec rt.mdl -p app.mpr # and re-describing is byte-identical +-- mx check app.mpr # 0 errors +-- +-- The boolean is the half a quoting-only fix would have missed: `showLabel` +-- must appear in the description, or re-executing it silently turns the +-- property off. +-- +-- SEPARATE DEFECT, deliberately not exercised here: giving a property in a +-- CONDITIONALLY SHOWN group a non-default value makes mx check report CE0463 — +-- ProgressCircle's `showLabel: true` or `labelType: 'percentage'` both do it, +-- with no DESCRIBE involved, while the same widget's General-group properties +-- take non-default values happily. Keep this example to defaults in that group +-- so it isolates the DESCRIBE round trip. + +create or replace page MyFirstModule.BT_PluggableRoundTrip ( + title: 'Pluggable widget round trip', + layout: Atlas_Core.Atlas_Default, + url: 'bt_pluggable_roundtrip' +) { + pluggablewidget 'com.mendix.widget.custom.progresscircle.ProgressCircle' pwRoundTrip ( + type: 'static', + staticCurrentValue: 42, + staticMinValue: 0, + staticMaxValue: 100, + showLabel: false + ) +} +/ diff --git a/mdl/executor/cmd_pages_describe.go b/mdl/executor/cmd_pages_describe.go index f58d68b18..2ee9faf63 100644 --- a/mdl/executor/cmd_pages_describe.go +++ b/mdl/executor/cmd_pages_describe.go @@ -652,6 +652,12 @@ type rawExplicitProp struct { Key string Value string // attribute short name or primitive value IsRef bool // true if this is an attribute reference, false for primitive + // ValueType is the property's DECLARED type from the widget package + // ("String", "Boolean", "Integer", "Enumeration", ...), and is what decides + // whether the emitted value is quoted. The value's own shape cannot: a + // String property holding "30" or "true" still has to come back quoted. + // Empty when the widget's schema is not in the document (ledger #104). + ValueType string } // rawDesignProp represents a parsed design property from BSON. diff --git a/mdl/executor/cmd_pages_describe_output.go b/mdl/executor/cmd_pages_describe_output.go index 14ccd060b..9eea959b1 100644 --- a/mdl/executor/cmd_pages_describe_output.go +++ b/mdl/executor/cmd_pages_describe_output.go @@ -7,6 +7,7 @@ import ( "fmt" "io" "regexp" + "strconv" "strings" "github.com/mendixlabs/mxcli/model" @@ -26,6 +27,76 @@ func mdlQuote(s string) string { return "'" + escaped + "'" } +// explicitPropValue renders a pluggable widget's property value as MDL. +// +// Every value used to be emitted raw, so a string lost its quotes and a JSON +// spec broke the re-parse at its first '{' — which took any page carrying a +// pluggable widget out of the DESCRIBE-edit-CREATE OR REPLACE workflow that +// mxcli itself documents (ledger #104). +// +// The DECLARED type decides, not the value's shape: a String property holding +// "30" or "true" must still come back quoted, or re-executing it writes a +// number where the author wrote text. Where no type is declared — the widget's +// schema is not in the document — the shape is the only signal left, and the +// fallback quotes anything that is not plainly a number or boolean, because an +// unquoted arbitrary string may not parse at all while a quoted literal still +// round-trips as text. +func explicitPropValue(p rawExplicitProp) string { + if p.IsRef { + return p.Value // an attribute name is an identifier, never a literal + } + switch p.ValueType { + case "Boolean", "Integer", "Decimal": + return p.Value + case "": + if isBareLiteral(p.Value) { + return p.Value + } + return mdlQuote(p.Value) + default: + return mdlQuote(p.Value) + } +} + +// isBareLiteral reports whether a value can be emitted without quotes when the +// property's declared type is unknown: a boolean, or a plain decimal number. +func isBareLiteral(s string) bool { + if s == "true" || s == "false" { + return true + } + if _, err := strconv.ParseFloat(s, 64); err == nil { + // ParseFloat also accepts "NaN", "Inf" and hex/exponent forms, none of + // which is a number an MDL author would have written; require the text + // to be made only of digits, one sign and one point. + return looksNumeric(s) + } + return false +} + +func looksNumeric(s string) bool { + if s == "" { + return false + } + body := strings.TrimPrefix(strings.TrimPrefix(s, "-"), "+") + if body == "" { + return false + } + dots := 0 + for _, r := range body { + switch { + case r >= '0' && r <= '9': + case r == '.': + dots++ + if dots > 1 { + return false + } + default: + return false + } + } + return body != "." +} + // appendDataGridPagingProps appends non-default paging properties for DataGrid2. func appendDataGridPagingProps(props []string, w rawWidget) []string { if w.PageSize != "" && w.PageSize != "20" { @@ -628,7 +699,7 @@ func outputWidgetMDLV3(ctx *ExecContext, w rawWidget, indent int) { props = append(props, fmt.Sprintf("Label: %s", mdlQuote(w.Caption))) } for _, ep := range w.ExplicitProperties { - props = append(props, fmt.Sprintf("%s: %s", ep.Key, ep.Value)) + props = append(props, fmt.Sprintf("%s: %s", ep.Key, explicitPropValue(ep))) } // onClick action (ledger #67 — reported on CustomChart) if w.OnClick != "" { diff --git a/mdl/executor/cmd_pages_describe_pluggable.go b/mdl/executor/cmd_pages_describe_pluggable.go index ac8ac2dae..219896abf 100644 --- a/mdl/executor/cmd_pages_describe_pluggable.go +++ b/mdl/executor/cmd_pages_describe_pluggable.go @@ -41,6 +41,45 @@ func buildPropertyTypeKeyMap(w map[string]any, withFallback bool) map[string]str return propTypeKeyMap } +// buildPropertyValueTypeMap maps a PropertyType's $ID to its DECLARED value +// type ("String", "Boolean", "Integer", "Enumeration", ...). +// +// It walks the same PropertyTypes array as buildPropertyTypeKeyMap, which reads +// only the key and throws the type away. The type is what tells DESCRIBE +// whether to quote a value; the value's own shape cannot, because a String +// property holding "30" is indistinguishable from an Integer holding 30 once it +// is a string in BSON (ledger #104). +// +// An absent entry is not an error — a document whose widget schema is missing +// still describes, with the emitter falling back to the value's shape. +func buildPropertyValueTypeMap(w map[string]any) map[string]string { + out := make(map[string]string) + widgetType, ok := w["Type"].(map[string]any) + if !ok { + return out + } + objType, ok := widgetType["ObjectType"].(map[string]any) + if !ok { + return out + } + for _, pt := range getBsonArrayElements(objType["PropertyTypes"]) { + ptMap, ok := pt.(map[string]any) + if !ok { + continue + } + id := extractBinaryID(ptMap["$ID"]) + if id == "" { + continue + } + if vt, ok := ptMap["ValueType"].(map[string]any); ok { + if t := extractString(vt["Type"]); t != "" { + out[id] = t + } + } + } + return out +} + // extractCustomWidgetAttribute extracts the attribute from a CustomWidget (e.g., ComboBox). // Specifically looks for attributeAssociation or attributeEnumeration properties by key, // avoiding false matches from other properties that also have AttributeRef (e.g., CaptionAttribute). @@ -1191,6 +1230,7 @@ func extractExplicitProperties(ctx *ExecContext, w map[string]any) []rawExplicit if len(propTypeKeyMap) == 0 { return nil } + valueTypes := buildPropertyValueTypeMap(w) var result []rawExplicitProp props := getBsonArrayElements(obj["Properties"]) @@ -1221,15 +1261,17 @@ func extractExplicitProperties(ctx *ExecContext, w map[string]any) []rawExplicit } } - // Check for non-default PrimitiveValue + // Check for a PrimitiveValue. + // + // Booleans used to be dropped here as "common defaults". They are not: + // a widget's default may be either, the document only stores what the + // author set, and discarding one made DESCRIBE emit a page whose + // re-execution silently turned the property off (ledger #104). if pv := extractString(value["PrimitiveValue"]); pv != "" { - // Skip common defaults - if pv == "true" || pv == "false" { - continue - } result = append(result, rawExplicitProp{ - Key: propKey, - Value: pv, + Key: propKey, + Value: pv, + ValueType: valueTypes[typePointerID], }) } } diff --git a/mdl/executor/cmd_pages_describe_pluggable_roundtrip_test.go b/mdl/executor/cmd_pages_describe_pluggable_roundtrip_test.go new file mode 100644 index 000000000..1b0b9e1ca --- /dev/null +++ b/mdl/executor/cmd_pages_describe_pluggable_roundtrip_test.go @@ -0,0 +1,149 @@ +// SPDX-License-Identifier: Apache-2.0 + +// Ledger finding #104: DESCRIBE PAGE does not round-trip a pluggable widget. +// +// Two independent defects, and fixing either one alone makes things worse: +// +// - Every explicit property was emitted with a raw %s, so a string value lost +// its quotes. A JSON spec then broke the re-parse at its first '{'. +// - extractExplicitProperties skipped any value of "true"/"false" as a +// "common default", so booleans never reached the output at all. +// +// Fix the quoting alone and the description re-parses cleanly while silently +// dropping a property — a worse failure than one that errors. So both halves +// are pinned here, and by the widget's DECLARED property type rather than by +// the shape of the value: a String property whose value happens to be "30" or +// "true" must still come back quoted. +package executor + +import ( + "testing" +) + +// buildVegaLikeWidget mirrors the ledger's probe: one pluggable widget carrying +// a string, an enumeration, an integer and a boolean, with the declared +// ValueTypes the widget package ships in Type.ObjectType.PropertyTypes. +func buildVegaLikeWidget() map[string]any { + const ( + idSpec = "type-id-spec" + idDatasetName = "type-id-dataset" + idRenderer = "type-id-renderer" + idHeight = "type-id-height" + idShowActions = "type-id-showactions" + ) + + widgetType := map[string]any{ + "WidgetId": "ledger.widget.web.vegachart.VegaChart", + "ObjectType": map[string]any{ + "PropertyTypes": []any{ + map[string]any{"$ID": idSpec, "PropertyKey": "spec", + "ValueType": map[string]any{"Type": "String"}}, + map[string]any{"$ID": idDatasetName, "PropertyKey": "datasetName", + "ValueType": map[string]any{"Type": "String"}}, + map[string]any{"$ID": idRenderer, "PropertyKey": "renderer", + "ValueType": map[string]any{"Type": "Enumeration"}}, + map[string]any{"$ID": idHeight, "PropertyKey": "chartHeight", + "ValueType": map[string]any{"Type": "Integer"}}, + map[string]any{"$ID": idShowActions, "PropertyKey": "showActions", + "ValueType": map[string]any{"Type": "Boolean"}}, + }, + }, + } + + properties := []any{ + map[string]any{"TypePointer": idSpec, + "Value": map[string]any{"PrimitiveValue": `{"a": 1}`}}, + map[string]any{"TypePointer": idDatasetName, + "Value": map[string]any{"PrimitiveValue": "table"}}, + map[string]any{"TypePointer": idRenderer, + "Value": map[string]any{"PrimitiveValue": "svg"}}, + map[string]any{"TypePointer": idHeight, + "Value": map[string]any{"PrimitiveValue": "30"}}, + map[string]any{"TypePointer": idShowActions, + "Value": map[string]any{"PrimitiveValue": "true"}}, + } + + return map[string]any{ + "Type": widgetType, + "Object": map[string]any{"Properties": properties}, + } +} + +// TestExplicitPropertiesKeepBooleans is the read half. A boolean is a value the +// author wrote, not noise: "true" was discarded on the way out, so DESCRIBE +// could not show it and a round-trip silently turned it off. +func TestExplicitPropertiesKeepBooleans(t *testing.T) { + props := extractExplicitProperties(nil, buildVegaLikeWidget()) + + byKey := map[string]rawExplicitProp{} + for _, p := range props { + byKey[p.Key] = p + } + + got, ok := byKey["showActions"] + if !ok { + t.Fatalf("showActions was dropped; got keys %v", explicitKeys(byKey)) + } + if got.Value != "true" { + t.Errorf("showActions = %q, want %q", got.Value, "true") + } + if got.ValueType != "Boolean" { + t.Errorf("showActions ValueType = %q, want %q — the emitter needs it to leave the literal bare", + got.ValueType, "Boolean") + } + // Everything the author set must survive, not just the boolean. + for _, want := range []string{"spec", "datasetName", "renderer", "chartHeight"} { + if _, ok := byKey[want]; !ok { + t.Errorf("%s was dropped; got keys %v", want, explicitKeys(byKey)) + } + } +} + +// TestExplicitValueQuotingByDeclaredType is the emit half. Strings and +// enumerations are quoted; numbers and booleans are not; and an attribute +// reference stays bare because it is an identifier, not a literal. +func TestExplicitValueQuotingByDeclaredType(t *testing.T) { + cases := []struct { + name string + in rawExplicitProp + want string + }{ + {"string", rawExplicitProp{Key: "datasetName", Value: "table", ValueType: "String"}, "'table'"}, + {"json string", rawExplicitProp{Key: "spec", Value: `{"a": 1}`, ValueType: "String"}, `'{"a": 1}'`}, + {"enumeration", rawExplicitProp{Key: "renderer", Value: "svg", ValueType: "Enumeration"}, "'svg'"}, + {"integer", rawExplicitProp{Key: "chartHeight", Value: "30", ValueType: "Integer"}, "30"}, + {"decimal", rawExplicitProp{Key: "ratio", Value: "1.5", ValueType: "Decimal"}, "1.5"}, + {"boolean", rawExplicitProp{Key: "showActions", Value: "true", ValueType: "Boolean"}, "true"}, + {"attribute ref", rawExplicitProp{Key: "value", Value: "Amount", IsRef: true}, "Amount"}, + + // A String property whose value looks like a literal is exactly why the + // declared type is used instead of the value's shape. + {"numeric-looking string", rawExplicitProp{Key: "label", Value: "30", ValueType: "String"}, "'30'"}, + {"boolean-looking string", rawExplicitProp{Key: "label", Value: "true", ValueType: "String"}, "'true'"}, + + // No declared type: fall back to the value's shape, quoting anything that + // is not plainly numeric or boolean, since an unquoted arbitrary string + // may not parse at all. + {"untyped text", rawExplicitProp{Key: "k", Value: "svg"}, "'svg'"}, + {"untyped number", rawExplicitProp{Key: "k", Value: "30"}, "30"}, + {"untyped bool", rawExplicitProp{Key: "k", Value: "false"}, "false"}, + + // A quote inside a value must be escaped, not emitted raw. + {"embedded quote", rawExplicitProp{Key: "k", Value: "it's", ValueType: "String"}, "'it''s'"}, + } + + for _, tc := range cases { + if got := explicitPropValue(tc.in); got != tc.want { + t.Errorf("%s: explicitPropValue(%q/%q) = %s, want %s", + tc.name, tc.in.Value, tc.in.ValueType, got, tc.want) + } + } +} + +func explicitKeys(m map[string]rawExplicitProp) []string { + out := make([]string, 0, len(m)) + for k := range m { + out = append(out, k) + } + return out +} From c8a483dbb58784225d8ef054a9e8bbcb7e61d3ae Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 12:07:32 +0000 Subject: [PATCH 6/6] Write a valid widget when a conditional property is shown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Authoring a pluggable widget property that lives in a conditionally shown group wrote a widget Mendix rejects with CE0463. On ProgressCircle both `showLabel: true` and `labelType: 'percentage'` did it; `showLabel: false` and the same widget's General-group properties were clean. No DESCRIBE involved — three lines of MDL reproduce it. Two gaps on the same axis, and closing one alone inverts the bug: - Serialization. #574 nulls the TextTemplate of a HIDDEN conditional property, but left a VISIBLE one null, where Mendix stores an empty Forms$ClientTemplate. Null and empty are each invalid in the other's state, so nulling hidden ones was only half the rule. - Extraction. The editorConfig reader did not understand a ternary's ELSE branch — `cond ? (…) : hidePropertiesIn([…])` — so ProgressCircle's `showLabel` gate was never seen and labelText read as visible whenever labelType was "text", its default. Filling visible templates without the missing gate simply moved which case failed: `showLabel: false` began failing where `true` had. Both are measured, both ways round, before and after. Only CONDITIONAL properties are filled. Studio Pro's convention for an unset TextTemplate is not uniform — a DataGrid custom-content column stores null for `tooltip` and an empty template for `exportValue`, which is what emptyClientTemplateRules exists for — so filling every unset one would trade this bug for its mirror image. Diagnosis followed .claude/skills/diagnose-ce0463.md. Handing the failing project to Mendix's own `mx update-widgets` on a copy produced the reference, and diffing it against mxcli's output named one meaningful path out of 969: Object/Properties[N]/Value/TextTemplate, null against a Forms$ClientTemplate. Authoring the same widget with the boolean both ways gave two documents differing in exactly one path, so nothing else needed eliminating. The extractor's preamble is why the first attempt read as "unsupported shape": the ternary follows a whole switch statement, and walking back past the `?` to the start of the function hands the guard parser a fragment with an unbalanced `}`. trailingExpr bounds it to the statement. Verified on 11.12.1: both showLabel states 0 errors, `showLabel: true` with `labelType: 'percentage'` 0 errors, and the DESCRIBE round trip of a widget carrying the boolean parses, re-executes and re-describes byte-identically. The widgetdemo showcase applies with 0 CE0463 (its 4 CE1613 are a pre-existing attribute reference in the example). Reported in mxcli-ledger FINDINGS #104 follow-on. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JXnEgoc2NQP1Y2TWMCMXC4 --- .claude/skills/fix-issue.md | 2 + .../pluggable-describe-roundtrip.mdl | 14 +-- mdl/backend/widgetobj/builder.go | 57 +++++++++- .../widgetobj/widget_visibility_test.go | 102 ++++++++++++++++++ mdl/executor/editorconfig_extract.go | 84 +++++++++++++++ mdl/executor/editorconfig_extract_test.go | 68 ++++++++++++ 6 files changed, 315 insertions(+), 12 deletions(-) diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index ea81ee81a..7bbcb07fb 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -513,3 +513,5 @@ extracting `OffsetExpression`/`LimitExpression`. | `mxcli run --local --ensure-db` fails on a freshly built `mxcli init` dev container — there is no PostgreSQL service to start and no `postgres` superuser — even though `psql` is on PATH | The generated Dockerfile installed `postgresql-client` **only**. `EnsureDatabase` starts a local service (`service postgresql start`) and provisions the role + database through `sudo -u postgres psql`, both of which need the **server** package. `psql` being present makes the container look correctly provisioned | `cmd/mxcli/tool_templates.go` (`generateDockerfile`) | Install `postgresql` alongside `postgresql-client`. **Generalisable**: when a feature shells out to a service, assert the *server* package in the image template, not the client that happens to satisfy a `LookPath` check — the CI/web image having the server is what hid this (`/usr/lib/postgresql/16` is present there but not in the generated dev container). Guarded by `TestGenerateDockerfile_PostgresServer`, which asserts the server package for both the docker and podman variants | | The implicit merge that closes a split — the end-if join — is placed by the layout pass and cannot be moved; it routinely lands on top of a neighbouring activity, and `@position` does not address it | The statement's own `@position` belongs to the SPLIT, so the merge had no annotation of its own. Three split builders computed `mergeX/centerY` with no override (`addIfStatement`, `addEnumSplit`, `addStructuredInheritanceSplit`), and DESCRIBE emitted nothing for it, so even a hand-moved merge was recomputed on the next exec | `mdl/ast/ast_microflow.go` (`ActivityAnnotations.Merge`), `mdl/visitor/visitor_microflow_statements.go` (`case "merge"`), `mdl/executor/cmd_microflows_builder_annotations.go` (`mergePosition`, and the `mergeStatementAnnotations` copy), the three split builders, `cmd_microflows_show_helpers.go` (`emitMergeAnnotation`, `commonMergeAfter`) | **Authoring without the DESCRIBE half is not a fix** — the first attempt shipped `@merge` writing correctly and was REVERTED, because the describer drops it and the layout pass then recomputes the merge on the next exec; that is the same round-trip data loss as #872/#881/#882, introduced by the change meant to help. **Find the relationship from data already in scope rather than threading a map**: the describer's `splitMergeMap` is not available at `emitObjectAnnotations`, and threading it would mean editing ten-plus call sites (the multi-site trap); `commonMergeAfter` walks the split's branches to the nearest merge reachable from ALL of them, using the `flowsByOrigin` and `activityMap` already passed in. **Bound any walk over a flow graph** — a retry loop makes it cyclic, so the walk carries a per-branch visited set and a node cap, pinned by `TestCommonMergeAfterTerminatesOnACycle`. **One helper for every site that places the merge**, so the override cannot be honoured at one split type and ignored at another. **Test the WIRING, not the helper**: `mergePosition` and `emitMergeAnnotation` called directly pass with every call site removed — `TestMergeReachesTheCanvasThroughTheBuilder` (MDL text → real builder) and `TestMergeIsEmittedByTheAnnotationEmitter` (through `emitObjectAnnotations`) fail when unwired, verified by removing each. Tests `mdl/executor/cmd_microflows_merge_test.go`, example `mdl-examples/bug-tests/884-annotations-and-lint-planes.mdl`. upstream #884 | | `DESCRIBE PAGE` output will not re-parse for any page carrying a **pluggable widget**: `mxcli check` on it fails with `extraneous input ':'` / `extraneous input '('` from the first widget onward. Separately, a boolean property the author set is missing from the description entirely | Two independent defects in the same path. **Emit**: explicit properties were written with a raw `%s`, so every string lost its quotes — a JSON `spec: {"a": 1}` then broke the parse at its first brace. **Read**: `extractExplicitProperties` skipped any value of `"true"`/`"false"` as a "common default", so booleans never reached the output | `mdl/executor/cmd_pages_describe_output.go` (`explicitPropValue`, `isBareLiteral`), `mdl/executor/cmd_pages_describe_pluggable.go` (`buildPropertyValueTypeMap`, `extractExplicitProperties`), `mdl/executor/cmd_pages_describe.go` (`rawExplicitProp.ValueType`) | **Fixing one half alone is worse than the bug.** Quote without emitting booleans and the description re-parses cleanly while silently dropping a property — a wrong page that validates. Both halves ship together or neither. **Quote by the DECLARED type, never the value's shape**: `ValueType.Type` sits in the widget's `Type.ObjectType.PropertyTypes`, the same array `buildPropertyTypeKeyMap` already walks for `PropertyKey` and throws away; a String property holding `"30"` or `"true"` is indistinguishable from a number once it is a string in BSON, and must still come back quoted. Where no type is declared, fall back to the value's shape and quote anything not plainly numeric or boolean — quoting is the safe direction, since an unquoted arbitrary string may not parse at all. **The round trip is the test, not the output**: describe → `check` → `exec` → describe must be byte-identical and leave `mx check` at 0 errors. Tests `mdl/executor/cmd_pages_describe_pluggable_roundtrip_test.go`; example `mdl-examples/bug-tests/pluggable-describe-roundtrip.mdl`, verified end to end on 11.12.1. **Uncovered while verifying, NOT fixed**: giving a property in a conditionally shown group a non-default value writes a widget Mendix rejects with CE0463 — ProgressCircle's `showLabel: true` and `labelType: 'percentage'` both do it with no DESCRIBE involved, while the same widget's General-group properties take non-default values happily. Reported in mxcli-ledger FINDINGS #104 | +| `DESCRIBE PAGE` output will not re-parse for any page carrying a **pluggable widget**: `mxcli check` on it fails with `extraneous input ':'` / `extraneous input '('` from the first widget onward. Separately, a boolean property the author set is missing from the description entirely | Two independent defects in the same path. **Emit**: explicit properties were written with a raw `%s`, so every string lost its quotes — a JSON `spec: {"a": 1}` then broke the parse at its first brace. **Read**: `extractExplicitProperties` skipped any value of `"true"`/`"false"` as a "common default", so booleans never reached the output | `mdl/executor/cmd_pages_describe_output.go` (`explicitPropValue`, `isBareLiteral`), `mdl/executor/cmd_pages_describe_pluggable.go` (`buildPropertyValueTypeMap`, `extractExplicitProperties`), `mdl/executor/cmd_pages_describe.go` (`rawExplicitProp.ValueType`) | **Fixing one half alone is worse than the bug.** Quote without emitting booleans and the description re-parses cleanly while silently dropping a property — a wrong page that validates. Both halves ship together or neither. **Quote by the DECLARED type, never the value's shape**: `ValueType.Type` sits in the widget's `Type.ObjectType.PropertyTypes`, the same array `buildPropertyTypeKeyMap` already walks for `PropertyKey` and throws away; a String property holding `"30"` or `"true"` is indistinguishable from a number once it is a string in BSON, and must still come back quoted. Where no type is declared, fall back to the value's shape and quote anything not plainly numeric or boolean — quoting is the safe direction, since an unquoted arbitrary string may not parse at all. **The round trip is the test, not the output**: describe → `check` → `exec` → describe must be byte-identical and leave `mx check` at 0 errors. Tests `mdl/executor/cmd_pages_describe_pluggable_roundtrip_test.go`; example `mdl-examples/bug-tests/pluggable-describe-roundtrip.mdl`, verified end to end on 11.12.1. Reported in mxcli-ledger FINDINGS #104 | +| Authoring a pluggable widget property that lives in a **conditionally shown group** writes a widget Mendix rejects with **CE0463**, while the same widget's other properties take non-default values happily. On ProgressCircle both `showLabel: true` and `labelType: 'percentage'` do it; `showLabel: false` and General-group properties are clean. No DESCRIBE involved | Two gaps on the same axis. **Serialization**: #574 nulls the TextTemplate of a HIDDEN conditional property, but left a VISIBLE one null — and Mendix stores an empty `Forms$ClientTemplate` there. Null and empty are each invalid in the other's state, so nulling hidden ones was only half the rule. **Extraction**: the editorConfig reader did not understand a ternary's ELSE branch (`cond ? (…) : hidePropertiesIn([…])`), so the `showLabel` gate was never seen and `labelText` read as visible whenever `labelType` was `"text"` — its default | `mdl/backend/widgetobj/builder.go` (`ApplyVisibilityRules`, `bsonFieldIsNil`), `mdl/executor/editorconfig_extract.go` (`parseGuard` `:` case, `ternaryCondition`, `trailingExpr`) | **Fixing one gap alone inverts the bug rather than closing it** — filling visible templates without the missing gate made `showLabel: false` fail where `true` had, because mxcli still thought labelText was visible. Measured both ways round before and after; a fix that moves which case fails is not a fix. **Let Mendix say what the shape should be**: `mx update-widgets` on a COPY of the failing project reconciles the widget, and diffing that against mxcli's output named the single meaningful path (`Object/Properties[N]/Value/TextTemplate` null vs `Forms$ClientTemplate`) out of 969. **The good/bad control does the isolation for free**: authoring the same widget with the boolean both ways gave two documents differing in exactly ONE path, so no other candidate needed testing. **Only CONDITIONAL properties are filled** — Studio Pro's convention for an unset TextTemplate is not uniform (a DataGrid custom-content column stores null for `tooltip` and an empty template for `exportValue`, per `emptyClientTemplateRules`), so filling every unset one would trade this bug for its mirror image. **The extractor's preamble matters**: the ternary is preceded by a whole `switch`, and walking back past the `?` to the function start yields a fragment with an unbalanced `}` that parses to nothing and looks like "unsupported shape" — hence `trailingExpr`. Regression signal: the widgetdemo showcase applies with **0 CE0463** (its 4 CE1613 are a pre-existing attribute reference). Tests `mdl/backend/widgetobj/widget_visibility_test.go`, `mdl/executor/editorconfig_extract_test.go`; example `mdl-examples/bug-tests/pluggable-describe-roundtrip.mdl` now exercises the group. Reported in mxcli-ledger FINDINGS #104 follow-on | diff --git a/mdl-examples/bug-tests/pluggable-describe-roundtrip.mdl b/mdl-examples/bug-tests/pluggable-describe-roundtrip.mdl index fe22dbf5d..8fc0d79da 100644 --- a/mdl-examples/bug-tests/pluggable-describe-roundtrip.mdl +++ b/mdl-examples/bug-tests/pluggable-describe-roundtrip.mdl @@ -16,12 +16,11 @@ -- must appear in the description, or re-executing it silently turns the -- property off. -- --- SEPARATE DEFECT, deliberately not exercised here: giving a property in a --- CONDITIONALLY SHOWN group a non-default value makes mx check report CE0463 — --- ProgressCircle's `showLabel: true` or `labelType: 'percentage'` both do it, --- with no DESCRIBE involved, while the same widget's General-group properties --- take non-default values happily. Keep this example to defaults in that group --- so it isolates the DESCRIBE round trip. +-- The conditional group is exercised on purpose. `showLabel: true` moves +-- `labelText` from hidden to visible, and a visible TextTemplate must carry an +-- empty ClientTemplate rather than null — the other half of #574. Both this and +-- `labelType: 'percentage'` used to write a widget Mendix rejects with CE0463, +-- with no DESCRIBE involved. create or replace page MyFirstModule.BT_PluggableRoundTrip ( title: 'Pluggable widget round trip', @@ -33,7 +32,8 @@ create or replace page MyFirstModule.BT_PluggableRoundTrip ( staticCurrentValue: 42, staticMinValue: 0, staticMaxValue: 100, - showLabel: false + showLabel: true, + labelType: 'percentage' ) } / diff --git a/mdl/backend/widgetobj/builder.go b/mdl/backend/widgetobj/builder.go index 7e692ec90..27a00a25b 100644 --- a/mdl/backend/widgetobj/builder.go +++ b/mdl/backend/widgetobj/builder.go @@ -638,21 +638,68 @@ func ApplyVisibilityRules(object bson.D, propertyTypeIDs map[string]pages.Proper return object } values := primitiveValuesOf(object, propertyTypeIDs) + + // Both directions, because null and empty are each invalid in the other's + // state. #574 covered hidden→null; a VISIBLE TextTemplate left null is the + // other half, and it is what made an authored ProgressCircle `showLabel: + // true` fail CE0463: `labelText` is visible whenever `labelType` is "text" + // (its default), and mxcli stored null where Mendix stores an empty + // ClientTemplate. Confirmed by handing the failing project to Mendix's own + // `mx update-widgets`, whose reconciliation writes exactly that template and + // changes nothing else of substance (ledger #104 follow-on). + // + // Only properties the widget's schema declares CONDITIONAL are touched. A + // TextTemplate with no rule is left exactly as it was: Studio Pro's + // convention for an unset one is not uniform — a DataGrid custom-content + // column stores null for `tooltip` and an empty ClientTemplate for + // `exportValue` — so filling every unset template would trade this bug for + // its mirror image. Those per-column conventions live in + // emptyClientTemplateRules and are reached by a different path. + hidden := make(map[string]bool, len(rules)) + conditional := make([]string, 0, len(rules)) for _, rule := range rules { - if !rule.HiddenWhen.Hidden(values) { - continue - } entry, ok := propertyTypeIDs[rule.PropertyKey] if !ok || entry.ValueType != "TextTemplate" { continue } - object = updateWidgetPropertyValue(object, propertyTypeIDs, rule.PropertyKey, func(val bson.D) bson.D { - return setBSONField(val, "TextTemplate", nil) + if _, seen := hidden[rule.PropertyKey]; !seen { + conditional = append(conditional, rule.PropertyKey) + hidden[rule.PropertyKey] = false + } + // Several rules may govern one property; any one of them hiding it wins. + if rule.HiddenWhen.Hidden(values) { + hidden[rule.PropertyKey] = true + } + } + // Sorted, because the object is serialized and map order is not stable. + sort.Strings(conditional) + + for _, key := range conditional { + isHidden := hidden[key] + object = updateWidgetPropertyValue(object, propertyTypeIDs, key, func(val bson.D) bson.D { + if isHidden { + return setBSONField(val, "TextTemplate", nil) + } + // Never clobber real content — only fill in the absent template. + if bsonFieldIsNil(val, "TextTemplate") { + return setBSONField(val, "TextTemplate", BuildEmptyClientTemplate()) + } + return val }) } return object } +// bsonFieldIsNil reports whether a field is absent or explicitly nil. +func bsonFieldIsNil(val bson.D, field string) bool { + for _, elem := range val { + if elem.Key == field { + return elem.Value == nil + } + } + return true +} + // primitiveValuesOf maps each known property key to its current comparable value // string in the object (e.g. type → "expression", itemSelection → "Single"). // Properties absent from the object map to "". diff --git a/mdl/backend/widgetobj/widget_visibility_test.go b/mdl/backend/widgetobj/widget_visibility_test.go index 975d3aca3..b3c20ba54 100644 --- a/mdl/backend/widgetobj/widget_visibility_test.go +++ b/mdl/backend/widgetobj/widget_visibility_test.go @@ -3,6 +3,7 @@ package widgetobj import ( + "fmt" "testing" "go.mongodb.org/mongo-driver/bson" @@ -189,3 +190,104 @@ func normalizeID(id string) string { } return string(out) } + +// TestVisibleConditionalTextTemplateGetsEmptyTemplate is the other half of #574, +// and the half that made an authored ProgressCircle `showLabel: true` fail +// CE0463: a CONDITIONAL TextTemplate that is currently VISIBLE must carry an +// empty ClientTemplate, not null. Null and empty are each invalid in the other's +// state, so nulling hidden ones was only ever half the rule. +// +// A property with no visibility rule is deliberately left alone: Studio Pro's +// convention for an unset TextTemplate is not uniform (a DataGrid custom-content +// column stores null for `tooltip` and an empty template for `exportValue`), so +// filling every unset one would trade this bug for its mirror image. +func TestVisibleConditionalTextTemplateGetsEmptyTemplate(t *testing.T) { + const ( + gateID = "44444444-4444-4444-4444-444444444444" // boolean "showLabel" + labelTextID = "55555555-5555-5555-5555-555555555555" // TextTemplate "labelText" (conditional) + unrelatedID = "66666666-6666-6666-6666-666666666666" // TextTemplate "caption" (no rule) + ) + + mkProp := func(id, primitiveVal string, tt any) bson.D { + return bson.D{ + {Key: "$Type", Value: "CustomWidgets$WidgetProperty"}, + {Key: "TypePointer", Value: types.UUIDToBlob(id)}, + {Key: "Value", Value: bson.D{ + {Key: "$Type", Value: "CustomWidgets$WidgetValue"}, + {Key: "PrimitiveValue", Value: primitiveVal}, + {Key: "TextTemplate", Value: tt}, + }}, + } + } + ids := map[string]pages.PropertyTypeIDEntry{ + "showLabel": {PropertyTypeID: gateID, ValueType: "Boolean"}, + "labelText": {PropertyTypeID: labelTextID, ValueType: "TextTemplate"}, + "caption": {PropertyTypeID: unrelatedID, ValueType: "TextTemplate"}, + } + rules := []types.WidgetVisibilityRule{{ + PropertyKey: "labelText", + HiddenWhen: &types.WidgetVisibilityCondition{PropertyKey: "showLabel", Operator: "falsy"}, + }} + + object := func(gate string) bson.D { + return bson.D{{Key: "Properties", Value: bson.A{ + int32(2), + mkProp(gateID, gate, nil), + mkProp(labelTextID, "", nil), + mkProp(unrelatedID, "", nil), + }}} + } + + // Gate on: labelText is visible, so its absent template must be filled. + got := ApplyVisibilityRules(object("true"), ids, rules) + if tt := templateOf(t, got, labelTextID); tt == nil { + t.Error("a VISIBLE conditional TextTemplate was left null; Mendix rejects that with CE0463") + } + // The property with no rule keeps its null — the conventions differ per widget. + if tt := templateOf(t, got, unrelatedID); tt != nil { + t.Error("a TextTemplate with no visibility rule was filled in; only conditional ones are touched") + } + + // Gate off: labelText is hidden, so it must be null (the original #574 half). + got = ApplyVisibilityRules(object("false"), ids, rules) + if tt := templateOf(t, got, labelTextID); tt != nil { + t.Error("a HIDDEN conditional TextTemplate was filled in; #574 requires null") + } +} + +// templateOf returns the TextTemplate value of the property pointing at id. +func templateOf(t *testing.T, object bson.D, id string) any { + t.Helper() + want := types.UUIDToBlob(id) + for _, elem := range object { + if elem.Key != "Properties" { + continue + } + for _, item := range elem.Value.(bson.A) { + prop, ok := item.(bson.D) + if !ok { + continue + } + var matches bool + for _, e := range prop { + if e.Key == "TypePointer" && fmt.Sprint(e.Value) == fmt.Sprint(want) { + matches = true + } + } + if !matches { + continue + } + for _, e := range prop { + if e.Key != "Value" { + continue + } + for _, v := range e.Value.(bson.D) { + if v.Key == "TextTemplate" { + return v.Value + } + } + } + } + } + return nil +} diff --git a/mdl/executor/editorconfig_extract.go b/mdl/executor/editorconfig_extract.go index 80838ff1c..1e98bf11f 100644 --- a/mdl/executor/editorconfig_extract.go +++ b/mdl/executor/editorconfig_extract.go @@ -238,6 +238,26 @@ func parseGuard(js string, callStart int) (types.WidgetVisibilityCondition, bool case strings.HasSuffix(pre, "||"): pre = pre[:len(pre)-2] falsy = true + case strings.HasSuffix(pre, ":"): + // The ELSE branch of a ternary: `cond ? (…hides…) : hide(…)`. The hide + // fires when cond is falsy, and cond is not the text next to the `:` — + // it sits before the matching `?`, past the whole then-branch. + // + // ProgressCircle 3.3.2 is the case that made this matter: + // + // return e.showLabel + // ? ("text" !== e.labelType && hidePropertyIn(t,e,"labelText")) + // : hidePropertiesIn(t,e,["customLabel","labelText","labelType"]) + // + // Without this branch only the inner `!==` rule was seen, so labelText + // read as VISIBLE whenever labelType was "text" — its default — even + // with showLabel false. See the CE0463 that produced (ledger #104). + cond, ok := ternaryCondition(pre[:len(pre)-1]) + if !ok { + return types.WidgetVisibilityCondition{}, false + } + pre = cond + falsy = true default: return types.WidgetVisibilityCondition{}, false } @@ -262,6 +282,70 @@ func parseGuard(js string, callStart int) (types.WidgetVisibilityCondition, bool return guardToCondition(guard, falsy, aliases) } +// ternaryCondition returns the text preceding the `?` that matches a trailing +// `:`, i.e. the condition of the ternary whose else-branch is about to start. +// +// It walks backwards balancing brackets, and counts nested ternaries: every +// further `:` seen at depth 0 needs its own `?` before the one we want. A `?` +// that never arrives means this `:` was not a ternary at all (a label, or an +// object literal key), and the caller emits no rule — which is the safe +// direction, since a wrong rule hides a property the user set. +func ternaryCondition(pre string) (string, bool) { + depth, pending := 0, 0 + for i := len(pre) - 1; i >= 0; i-- { + switch pre[i] { + case ')', ']', '}': + depth++ + case '(', '[', '{': + if depth == 0 { + return "", false // ran out of enclosing expression + } + depth-- + case ':': + if depth == 0 { + pending++ + } + case '?': + if depth == 0 { + if pending == 0 { + return trailingExpr(pre[:i]), true + } + pending-- + } + } + } + return "", false +} + +// trailingExpr returns the last expression in s, bounded by a statement +// separator. The ternary is usually not the first thing in its function — +// ProgressCircle's is preceded by a whole `switch` — and returning everything +// back to the start hands the caller a fragment with an unbalanced `}` that no +// guard parser can read. +func trailingExpr(s string) string { + depth := 0 + for i := len(s) - 1; i >= 0; i-- { + switch s[i] { + case ')', ']': + depth++ + case '(', '[': + if depth == 0 { + return strings.TrimSpace(s[i+1:]) + } + depth-- + case '}', '{', ';', ',': + // A brace at depth 0 closes or opens a preceding BLOCK, so the + // expression starts after it. Braces are not counted as nesting here + // for that reason — an object literal inside the condition would be + // bounded by its own parens. + if depth == 0 { + return strings.TrimSpace(s[i+1:]) + } + } + } + return strings.TrimSpace(s) +} + // stripReturnPrefix removes a leading `return` keyword from a guard expression. // A widget's getProperties body often starts `return && hide(…), …`, so // the first guard is prefixed with `return` (minified: `return"none"===…`). diff --git a/mdl/executor/editorconfig_extract_test.go b/mdl/executor/editorconfig_extract_test.go index 3e8a062ab..f0f4f0294 100644 --- a/mdl/executor/editorconfig_extract_test.go +++ b/mdl/executor/editorconfig_extract_test.go @@ -148,3 +148,71 @@ func TestExtractVisibility_NamespaceAndReturn(t *testing.T) { }) } } + +// TestTernaryElseBranchHideRule covers the `cond ? (…) : hidePropertiesIn([…])` +// shape, where the hide fires when the condition is FALSY and the condition sits +// before the matching `?`, past the whole then-branch. +// +// The snippet is ProgressCircle 3.3.2's real getProperties, minified, INCLUDING +// the switch statement that precedes the ternary. That preamble is not +// decoration: a first attempt at this walked back past the `?` to the start of +// the function and handed the guard parser a fragment with an unbalanced `}`, +// which parsed to nothing and looked exactly like "unsupported shape". +// +// Without the rule, labelText read as visible whenever labelType was "text" — +// its default — even with showLabel false, and the widget failed CE0463 either +// way round (ledger #104 follow-on). +func TestTernaryElseBranchHideRule(t *testing.T) { + js := `function getProperties(e,t,r){` + + `switch(e.type){case"dynamic":a.hidePropertiesIn(t,e,[].concat(n(b.static),n(b.expression)));break;` + + `case"static":a.hidePropertiesIn(t,e,[].concat(n(b.dynamic),n(b.expression)));break;` + + `case"expression":a.hidePropertiesIn(t,e,[].concat(n(b.static),n(b.dynamic)))}` + + `return e.showLabel?("custom"!==e.labelType&&a.hidePropertyIn(t,e,"customLabel"),` + + `"text"!==e.labelType&&a.hidePropertyIn(t,e,"labelText")):` + + `a.hidePropertiesIn(t,e,["customLabel","labelText","labelType"]),t}` + + rules, _ := extractVisibilityRulesFromJS(js) + + want := map[string]bool{"customLabel": false, "labelText": false, "labelType": false} + for _, r := range rules { + if r.HiddenWhen != nil && r.HiddenWhen.PropertyKey == "showLabel" && r.HiddenWhen.Operator == "falsy" { + if _, ok := want[r.PropertyKey]; ok { + want[r.PropertyKey] = true + } + } + } + for key, found := range want { + if !found { + t.Errorf("missing rule: %s hidden when showLabel is falsy", key) + } + } + + // The then-branch rule must survive too — a property can carry several rules, + // and labelText is hidden by EITHER gate. + var sawInner bool + for _, r := range rules { + if r.PropertyKey == "labelText" && r.HiddenWhen != nil && + r.HiddenWhen.PropertyKey == "labelType" && r.HiddenWhen.Operator == "ne" && + r.HiddenWhen.Value == "text" { + sawInner = true + } + } + if !sawInner { + t.Error("the inner `\"text\" !== labelType` rule was lost") + } +} + +// TestTernaryConditionRejectsNonTernaryColon checks the safe direction: a `:` +// that is not a ternary (an object literal, a label) must yield no rule rather +// than a guessed one, since a wrong rule hides a property the user set. +func TestTernaryConditionRejectsNonTernaryColon(t *testing.T) { + for _, s := range []string{`{foo:`, `return{a:1,b:`, ``} { + if got, ok := ternaryCondition(s); ok { + t.Errorf("ternaryCondition(%q) = %q, true; want no match", s, got) + } + } + // And a real ternary is still found, past a nested one. + if got, ok := ternaryCondition(`x?a?b:c:`[:len(`x?a?b:c:`)-1]); !ok || got != "x" { + t.Errorf("nested ternary: got %q, %v; want %q, true", got, ok, "x") + } +}