diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index 596f2c9a4..7bbcb07fb 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -512,3 +512,6 @@ 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 | +| `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/.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/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"}, 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/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 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; 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..8fc0d79da --- /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. +-- +-- 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', + 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: 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/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 +} 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") + } +} 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) + } +}