diff --git a/.claude/hooks/session-start.sh b/.claude/hooks/session-start.sh new file mode 100755 index 000000000..4d60baf86 --- /dev/null +++ b/.claude/hooks/session-start.sh @@ -0,0 +1,65 @@ +#!/bin/bash +# +# SessionStart hook — bootstrap a Claude Code on the web session for mxcli. +# +# The devcontainer (.devcontainer/Dockerfile) installs these prerequisites, but +# web sessions do not use the devcontainer, so a fresh container has no ANTLR4 +# and `make build` fails at the `make grammar` step: +# +# *** ANTLR4 not found. Install with: brew install antlr4 ... Stop. +# +# The generated parser in mdl/grammar/parser/ is deliberately not committed, so +# ANTLR4 is a hard build dependency, not an optional extra. +# +# Idempotent and non-interactive: safe to re-run on resume/clear/compact. +set -euo pipefail + +# Pinned to match .github/workflows/push-test.yml. The antlr4 wrapper resolves +# the jar version from this variable, so it must be set for every build, not +# just this script — hence the CLAUDE_ENV_FILE export below. +ANTLR_VERSION='4.13.2' +ANTLR_TOOLS_VERSION='0.2.2' + +# Local (devcontainer / laptop) setups already have these via the Dockerfile. +if [ "${CLAUDE_CODE_REMOTE:-}" != "true" ]; then + exit 0 +fi + +cd "${CLAUDE_PROJECT_DIR:-$(dirname "$0")/../..}" + +# 1. ANTLR4 — required by `make grammar`, which `make build` always runs. +if ! command -v antlr4 >/dev/null 2>&1; then + echo "Installing antlr4-tools==${ANTLR_TOOLS_VERSION}..." + pip install --break-system-packages --quiet "antlr4-tools==${ANTLR_TOOLS_VERSION}" +fi + +# The antlr4 wrapper downloads its jar on first use. Doing it here keeps that +# ~2MB fetch (and its JDK probe) out of the first `make build`. +export ANTLR4_TOOLS_ANTLR_VERSION="${ANTLR_VERSION}" +if [ ! -d "${HOME}/.m2/repository/org/antlr/antlr4/${ANTLR_VERSION}" ]; then + echo "Fetching ANTLR ${ANTLR_VERSION} jar..." + antlr4 >/dev/null 2>&1 || true +fi + +# Persist for the session so `make build` works from any later shell. +if [ -n "${CLAUDE_ENV_FILE:-}" ]; then + echo "export ANTLR4_TOOLS_ANTLR_VERSION=${ANTLR_VERSION}" >> "${CLAUDE_ENV_FILE}" +fi + +# 2. Go modules — warms the module cache so the first build is not a ~1GB fetch. +echo "Downloading Go modules..." +go mod download + +# 3. MxBuild — the Mendix toolchain that validates projects mxcli writes +# (`mx check`). Opt-in: the CDN tarball is ~820MB and unpacks to ~1.6GB, too +# slow to pull into every session start. Set the version to enable, e.g. +# MXCLI_HOOK_MXBUILD_VERSION=11.13.0 (the newest in the nightly CI matrix). +# Otherwise fetch on demand: mxcli setup mxbuild --version 11.13.0 +if [ -n "${MXCLI_HOOK_MXBUILD_VERSION:-}" ]; then + if [ ! -d "${HOME}/.mxcli/mxbuild/${MXCLI_HOOK_MXBUILD_VERSION}" ]; then + echo "Downloading MxBuild ${MXCLI_HOOK_MXBUILD_VERSION} (~820MB)..." + go run ./cmd/mxcli setup mxbuild --version "${MXCLI_HOOK_MXBUILD_VERSION}" + fi +fi + +echo "Session bootstrap complete. Build with: make build" diff --git a/.claude/settings.json b/.claude/settings.json new file mode 100644 index 000000000..e06b0338e --- /dev/null +++ b/.claude/settings.json @@ -0,0 +1,14 @@ +{ + "hooks": { + "SessionStart": [ + { + "hooks": [ + { + "type": "command", + "command": "$CLAUDE_PROJECT_DIR/.claude/hooks/session-start.sh" + } + ] + } + ] + } +} diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index da6f88926..7057b7419 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -324,10 +324,17 @@ cases for these three BSON types — they fell to `default: return nil`. | A user reports CE0463 "the definition of this widget has changed" on DataGrid2 / Gallery / filters after upgrading the **Data Widgets** marketplace module. Reads like template drift; on real projects it is usually **not an mxcli bug at all** | A widget package that DROPS a property leaves every *stored* instance carrying a property the new definition lacks — which is precisely what CE0463 reports, and what its own message ("Update all widgets") tells you to fix. Ledger on 11.12: 0 errors at Data Widgets 3.4 (as authored) → 36 CE0463 at 3.11.3 → **0 again after `mx update-widgets`**. Single cause: `key="advanced"` is in `Datagrid.xml` at 3.4 and gone at 3.10/3.11 | no code change — diagnostic. See `docs/03-development/WIDGET_BSON_VERSION_COMPATIBILITY.md` "CE0463 after a widget-package upgrade" | **Two controls settle it, neither optional.** (1) Do **Studio Pro's own** widgets fail too? A blank project's `dataGrid2_*`/`gallery1,2`/`drop_downFilter1,2` are Mendix-authored — if they fail alongside mxcli's, the tool is not the variable (29 of Ledger's 36 were these). (2) Does **`mx update-widgets` clear it**? If yes, mxcli's BSON was structurally valid and correct for the version it was written against; genuine template bugs do NOT clear this way (the Image stale default and the number-filter markerless array both needed template fixes). **The real mxcli defect is the residue after those controls**: author FRESH against the new package (`widget init` + author + `mx check`) — on 3.10/3.11 that leaves DataGrid2 **clean** and only Gallery + DatagridDropdownFilter failing, i.e. far narrower than the issue as filed. **Trap that cost a full round-trip**: measuring with the doctype fixtures alone mixes both cases, because their pages live in a blank project whose own template widgets are already failing — subtract by widget NAME against a control project that ran no mxcli command. Issue #716 | | Freshly authored **Gallery** widgets fail `mx check` with **CE0463** on Data Widgets 3.10+, while the 3.4 package bundled with Mendix 11.12/11.13 is clean — so it looks like ordinary post-upgrade staleness. Every schema-level explanation is disproven (property sets, list markers, ordering, pointer topology, `GenerateFromMPK`) | `syncDefinitionAttrs` reconciled a surviving property's definition attributes (`Required`, `OnChangeProperty`) from the installed `.mpk` onto the `CustomWidgets$WidgetPropertyType` node — but they live one level down, on its **`ValueType`**. The "only update a key that already exists" guard then never fired, making the entire pass a **silent no-op**, so the Gallery kept the embedded template's `OnChangeProperty = "onConfigurationChange"` where 3.10 expects `""` | `sdk/widgets/augment.go` + `modelsdk/widgets/augment.go` (`syncDefinitionAttrs`) | Descend to `ValueType` before the update (`getMapField(ptMap, "ValueType")`, falling back to the PropertyType). **Diagnosis method that found it**: diff against `mx update-widgets` output at the PATH level — the differing paths were `Type/ObjectType/PropertyTypes[N]/ValueType/OnChangeProperty`, and the path told me which node to write. **Generalisable — the trap that cost the most here**: a guarded update (`if _, ok := m[k]; ok`) aimed at the wrong node is *invisible*. It cannot fail loudly, so measurements read as "the fix didn't help" rather than "the fix never ran". When a change measurably does nothing, verify it executed before concluding the hypothesis was wrong. Result: fresh-authoring CE0463 on 3.10 went 6 → 2; bundled 3.4 stayed at 0. **Still open**: the two datagrid dropdown filters, whose residual diff is `ValueType/AllowUpload` (absent from the modelsdk template copy, present in sdk's — the engines' template sets have diverged); syncing them does not clear it. Repro `mdl-examples/bug-tests/716-widget-package-upgrade.mdl`. Issue #716 | | Every authored **Data grid 2** fails `mx check` with **CE0463** on the *bundled* Data Widgets 3.4 (Mendix 11.12/11.13) — but only on the **legacy** engine; `modelsdk` is clean on the same script. Appeared the moment `syncDefinitionAttrs` was corrected to write to `ValueType` (the row above), i.e. the moment that pass first actually executed | The Mendix pluggable-widget XML schema defaults `required` to **true**; only an explicit `required="false"` is optional. `sdk/widgets/mpk` read a missing attribute as `false` (`p.Required == "true"`), so the now-live sync overwrote 24 correct `true`s with `false` on DataGrid2 3.4 (which omits `required=` on 24 of its 40 properties). `modelsdk/widgets/mpk` already read it correctly (`p.Required != "false"`, fixed under #600) — the engines had silently diverged on the default | `sdk/widgets/mpk/mpk.go` (all three `PropertyDef` construction sites: `walkPropertyGroup` top-level + nested-direct, `collectNestedProperties`) | Read `p.Required != "false"` so absent means true, matching `modelsdk` and `mx update-widgets`. **Generalisable — the shape to look for**: when two engines carry parallel copies of a parser, a fix applied to one leaves a *latent* divergence in the other that stays invisible until some unrelated change starts consuming the value. Grep the sibling package for the same expression before assuming a defect is engine-specific. **Diagnosis trap hit here**: an A/B ran out of disk mid-run, the exec silently created no widgets, and `mx check` reported 0 CE0463 — reading as "the pre-fix binary is clean". Always assert the artifact exists (`show widgets | grep `) before trusting a zero. Unit repro `sdk/widgets/mpk/required_default_test.go`; integration `TestMxCheck_DataGridPage`/`TestMxCheck_DataGridNoColumns`. Issue #716 | +| `mxcli widget sync` appears to succeed — project loads, CE0463 drops, `mprcontents/` survives — but afterwards **`mx update-widgets` cannot SAVE**: `System.InvalidOperationException: Duplicate Guid in unit page template '…'`. Worse, update-widgets collapses `mprcontents/` *before* it fails, leaving the project flattened AND unloadable (`Root unit not found`). A one-way door: sync silently forecloses the only complete remediation | `AugmentTemplate` adds a property to an object-list property (DataGrid2 `columns`) by giving **every list entry a copy of the same constructed node**, so one placeholder id appears N times. The sync's placeholder→UUID remap was keyed **by value**, so all N copies received the *same* fresh UUID. 3 added properties x 3 nodes each = 9 duplicated GUIDs per widget, x N columns — 18 units and 432 excess occurrences on the reference fixture. The template pipeline never hit it because a template has exactly one list entry | `mdl/executor/widget_convert.go` (`ensureUniqueWidgetIDs`, `widgetIDsAreUnique`) + `mdl/executor/widget_sync_apply.go` | Make `$ID` unique **per occurrence**, scoping the reference rewrite to the list entry: a `TypePointer` repeated across sibling entries is *correct* (every column's property points at the one shared `WidgetPropertyType`) and must not be rewritten. Then **refuse to write** any unit that still contains a duplicate. **Generalisable — the failure class**: Mendix validates GUID uniqueness only at SAVE, so `mx check` passing proves nothing about it; and the tool you would reach for to recover destroys the multi-file layout before discovering the problem. Any writer that duplicates nodes needs a pre-write uniqueness assertion, not a post-hoc check. **Testing trap hit here**: the unit test calls the fix directly, so it still passed with the call removed from the apply path — it proves the function, not the wiring. The pre-write guard is what actually caught the regression during the mutation check. Repro: run sync, then `python3` count duplicate `$ID`s per `.mxunit`. Reported against PR #89 | | Freshly authored **drop-down filter** widgets fail `mx check` with **CE0463** on Data Widgets 3.10 while every other widget in the same script is clean. The stored BSON is missing `ValueType/AllowUpload` on all 25 ValueTypes and carries `Required=false` on `refCaption`/`refCaptionExp`, which the 3.10 XML declares `required="true"` — so the embedded template looks like the culprit, but replacing it changes nothing | `AugmentTemplate` never ran on this widget. Its opening guard — "nothing to add or remove at top level, and no nested children" — was written when the function only synced the property SET, and returns before the **six value-level passes** bolted on later (`reconcileEnumValues`, `reconcilePropertyMetadata`, `reconcileValueTypesFromMPK`, `completeValueTypeEnvelope`, `reorderPropertyTypes`, `syncDefinitionAttrs`). DataGrid2 never hit it because `columns` has nested children; the drop-down filter declares exactly the 25 keys the 11.6-era template already has, so it took the exit every time | `modelsdk/widgets/augment.go` + `sdk/widgets/augment.go` (`AugmentTemplate`) | Wrap the add/remove block in `if len(missing) > 0 \|\| len(stale) > 0 \|\| hasNestedChildren { … }` instead of returning early, so the value-level passes always run. **Generalisable — the shape to look for**: an early return placed correctly for a function's original job silently disables everything appended after it. When a pass "does nothing" for one input and works for others, check whether it *reached* the pass before theorising about the data (same root shape as the `ValueType` no-op two rows up). The `hasNestedChildren` clause in that guard is the tell: someone had already patched around the same bug for one widget rather than fixing it. **Diagnosis method**: a probe calling `augmentFromMPK` directly and counting `ValueTypes with AllowUpload` — 0/25 for the drop-down filter vs 44/44 for Gallery — localised it to "augment did not run" in one step, before any BSON theorising. Result: modelsdk on DW 3.10 went 2 → **0**. Tests `TestAugmentTemplate_MatchingKeysStillReconcilesValues` in both engines. Issue #716 | | A **Decimal/DateTime** in `dynamictext` always rendered with the hardcoded default format ("5068.38000000"); no MDL way to set the per-parameter Format, and a widget-level `decimalPrecision:` was **silently dropped** (mxcli check ✓, exec ✓, but gone) | The model always stored `ClientTemplateParameter.FormattingInfo`, but **all three writers hardcoded it** (`DecimalPrecision:2, GroupDigits:false, DateFormat:Date, EnumFormat:Text`) and ignored `param.FormattingInfo`; the grammar had no syntax to set it and DESCRIBE dropped it on read too | grammar `mdl/grammar/domains/MDLPage.g4` (`paramAssignmentV3` + `paramFormatV3`), `mdl/ast/ast_page_v3.go` (`ParamFormatV3`), `mdl/visitor/visitor_page_v3.go` (`buildParamFormatV3`), builder `mdl/executor/cmd_pages_builder_v3_widgets.go` (`formattingInfoFromParamFormat`), writers `mdl/backend/modelsdk/widget_write.go` (`formattingInfoToGen`) + `sdk/mpr/writer_widgets.go` (`serializeClientTemplateParameter`), describe `cmd_pages_describe_output.go` (`formatParamFormatSuffix`), validate `validate_widgets.go` (`validateDynamicTextFormatting`/MDL-WIDGET18) | Add a per-param `FORMAT (decimalPrecision: N, groupDigits: bool, dateFormat: …, customDateFormat: '…', enumFormat: …)` block: `{1} = Amount format (decimalPrecision: 2, groupDigits: true)`. The **FORMAT keyword is required** — a bare `(…)` after the value is ambiguous with a function call because `:` is OQL division in expressions. Writers use the param's FormattingInfo when set, else the same hardcoded defaults (nil → byte-identical to before, zero risk to existing widgets). MDL-WIDGET18 turns a widget-level format key into an actionable error (no more silent drop) and validates keys/enums. Verified: exec → `mx check` (11.12.1) 0 errors + DESCRIBE round-trip. Repro `mdl-examples/bug-tests/ledger-75-dynamictext-formatting.mdl`. Ledger #75 | | dynamic-text `format (…)` writes valid FormattingInfo, `mx check` ✓, but **renders unformatted** at runtime — a Decimal shows `-12` not `-12.00`, dates ignore the format | The parameter was serialized as `Expression: toString($currentObject/Attr)` (a non-String attribute was wrapped in `toString()`), and Mendix applies FormattingInfo **only to attribute-bound** params — an Expression param bypasses it. The BSON was valid but inert; `mx check` and DESCRIBE can't catch a *render* problem (the exact trap the runtime-verify skill exists for — I shipped #75 without it and the tester caught this) | `mdl/executor/cmd_pages_builder_v3.go` (`resolveTemplateAttributePathFull`, the bare-attribute branch) | Bind a bare non-String attribute as a structured **`AttributeRef`**, not a `toString()` Expression — the runtime then renders it through FormattingInfo, exactly as Studio Pro does. `toString()` was never required by mxbuild (AttributeRef for a Decimal/DateTime in a text template passes `mx check` → 0 errors). Engine read-parity holds. Note the `$param.Attr` non-String path (same function) still uses `toString()` — rarer, left for follow-up. Ledger #76 | | A **DataGrid2 dynamic-text column** (`column x (ShowContentAs: dynamicText, Content: '{1}', ContentParams: [{1} = Attr format (…)])`) fails to open with **CE0463**, *and* its `format (…)` block is silently dropped. `mxcli docker check` hides the CE0463 because it runs `mx update-widgets` first; raw `mx check` and `mxbuild --serve` (run --local) surface it | Two independent gaps in the full-page object-list column path. (1) The shared `buildClientTemplateParams` never read the parsed `p.Format`, and the column-scoped serializer `SerializeColumnClientTemplateParameter` **hardcoded** FormattingInfo — so a column param's format was dropped at both write points. (2) A dynamic-text column has no attribute and no content widgets, so `detectObjectListItemKind` classified it as the **default** kind, which has no empty-ClientTemplate rules → its `tooltip` serialized as `TextTemplate:null`. Studio Pro stores an **empty `Forms$ClientTemplate`** there (as for an attribute column), so the widget failed to load | `mdl/executor/cmd_pages_builder_v3_widgets.go` (`buildClientTemplateParams` → apply `formattingInfoFromParamFormat(p.Format)`), `mdl/backend/widgetobj/builder.go` (`SerializeColumnClientTemplateParameter` honours `param.FormattingInfo`; new `itemKindDynamicText` + `emptyClientTemplateRules` tooltip entry; `detectObjectListItemKind`), describe `mdl/executor/cmd_pages_describe_pluggable.go` (`extractTextTemplateParameters` zips the format suffix) | Route the FORMAT block through the *shared* params helper (fixes the object-list column path **and** the ALTER PAGE column path at once) and stop hardcoding FormattingInfo in the column serializer. Classify a `showContentAs: dynamicText` column as its own item kind and give it the attribute column's `tooltip → empty CT` rule (exportValue stays null). **Diagnosis method that found the CE0463**: `update-widgets` on a *copy* cleared it → Case B (our BSON); a path-level flatten-diff of the datagrid subtree, mine vs the reconciled reference, isolated the single differing path `columns[dynamicText]/tooltip/TextTemplate` null↔empty. **Trap**: `docker check`'s built-in `update-widgets` masks the very defect you're hunting — measure with raw `mx check` or the serve build. Repro `mdl-examples/bug-tests/ledger-77-datagrid-dynamictext-column.mdl`; verified end-to-end (raw `mx check` 0 errors + Playwright cell renders `-1,234.50`). Ledger #77 | +| A `create json structure … snippet` produces a structure whose **import mappings silently import zero objects** — the REST call succeeds, the mapping looks right, and no data arrives. No validation error, `mx check` passes. Dumping the stored BSON shows every element at `MinOccurs=0, MaxOccurs=0` | Mendix reads `MaxOccurs=0` **literally as "never occurs"**, not as "unspecified". The snippet→element builder hardcoded `0/0` at all nine construction sites; `cmd_import_mappings.go` then copies MinOccurs/MaxOccurs straight onto the mapping elements, so the dead bound propagates from the structure into every mapping bound to it | `mdl/types/json_utils.go` (`BuildJsonElementsFromSnippet` + `buildElementFromRawObject` / `buildElementFromRawRootArray` / `buildElementFromRawArray` / `buildValueElement`) | Derive occurrences from the JSON shape: root `1..1`, Object/Value `0..1`, Array `0..1` with its **item** child `0..*` (`MaxOccurs = -1`), primitive-array Wrapper `0..*`. Added the named constant `occursUnbounded` so `-1` is not a bare literal. **Generalisable — the shape to look for**: when a tree comes out uniformly wrong except for *one* node, that node proves the writer is capable of the correct value and localises the bug to the construction sites that hardcode it — here the nested object-array item was already `0..-1` while the root-array item beside it was `0..0`, i.e. the same construct written two ways in two builders. **Trap**: `0` looks like a harmless default for a numeric field, so this reads as correct in review and survives every checker; only a BSON dump or a runtime import reveals it. **Follow-up that the first cut missed**: Mendix cross-validates every *mapping* element's occurrence against its bound *schema* element and reports **CE5015** ("Attribute 'MaxOccurs' does not match schema element") on a mismatch. Import writers already propagated occurrences; all **three** export writers (`modelsdk/mpr/serialize_mappings.go`, `sdk/mpr/writer_export_mapping.go`, `mdl/backend/modelsdk/mapping_write.go` — the last is the one the default engine actually uses) hardcoded `MaxOccurs: 0` on value elements, so raising the schema broke every export mapping. **Generalisable — the shape to look for**: changing a value that another document is validated *against* is never a one-sided edit; grep every writer that emits the same key, and expect more than one engine to have its own copy. **Verification trap that caused the miss**: the repro created JSON structures but no mapping bound to one, so it passed `mx check` while the integration suite went red — when a fix changes a field that other documents reference, the repro must instantiate a *referencing* document, not just the changed one. Repro `mdl-examples/bug-tests/841-json-structure-occurrences.mdl` (now includes an import+export mapping); verified end-to-end (`mx check` 11.13.0 0 errors on the repro and on both doctype scripts that failed CI). Issue #841 | +| A workflow **`DECISION`** whose expression uses the documented lowercase `$workflowContext` fails `mx check` with **`[error] [CE0117] "Error(s) in expression." at Decision 'Decision'`**, while the *same spelling* in a `CALL MICROFLOW … WITH` clause works. `mxcli check` and `mxcli exec` both report success | The context parameter is named `WorkflowContext` and Mendix expressions are case-sensitive on 11.9+, so `$workflowContext` is an undefined variable. `normalizeWorkflowContextExpr` existed and was well-tested, but was only *applied* in `autoBindCallMicroflow` (the FINDINGS #39 fix) — `buildExclusiveSplit` stored `n.Expression` verbatim. The working WITH clause is what disguised it: the user reasonably concludes the spelling is fine | `mdl/executor/cmd_workflows_write.go` (`buildExclusiveSplit`, and the sibling `buildWaitForTimer` whose delay may reference a context date attribute) | Run the authored expression through the existing `normalizeWorkflowContextExpr` at every site that accepts a user expression — there are three in the workflow writer, and only the parameter-mapping one was covered. **Generalisable — the shape to look for**: a *normalizer that exists and is unit-tested* is not evidence it is *called*; grep the call sites, not the helper. When one input spelling works and an identical one fails, compare the two code paths before questioning the data. **Also fix the docs that teach the broken form** — `.claude/skills/mendix/write-workflows.md` showed lowercase in its DECISION example and is synced into user projects by `mxcli init` via `cmd/mxcli/skills/`, so the bug propagated to every generated project. Repro `mdl-examples/bug-tests/845-workflow-decision-context-casing.mdl`; verified end-to-end (`mx check` 11.13.0: 1 error → 0). Issue #845 | + +| `mxcli check … --references` reports **"All references valid" / "Check passed!"** for a script whose GRANT names a module role from a different module than the document; `exec` then fails with **CE0148** — after the preceding statements have already been applied, leaving the project half-modified | The guard (`checkDocumentAccessRolesSameModule`) existed and was wired into all five exec paths, but **no validate path ever called it**. mxcli does not run a script in a single transaction, so a failure that only surfaces at exec time is exactly what a pre-flight check exists to prevent | `mdl/executor/validate_grant_roles.go` (`ValidateGrantRoles`, MDL-GRANT01), `mdl/executor/cmd_security_defaults.go` (`validateCrossModuleGrant`), wired in `cmd/mxcli/cmd_check.go` | Reuse the existing exec-time guard from the **no-project** violations pass, covering all five document-access grants (microflow, nanoflow, page, OData service, published REST service). Take the document's module from the **statement's own qualified name**, not the resolved document — same comparison, and it works before the document exists (it is often created earlier in the same script). **Put it in the no-project pass, not under `--references`**: the check compares two names already in the script, so requiring `-p` withholds an answer mxcli can always give, and a plain `mxcli check` now catches it. **Generalisable — the shape to look for**: when exec rejects something a checker accepts, the bug is usually not a missing rule but a rule wired into only one of the two paths — grep the guard's callers before writing a new one. Repro `mdl-examples/bug-tests/836-check-cross-module-grant.fail.mdl`; verified end-to-end (check exits 1 naming the statement and CE0148, project left untouched; same-module variant still passes and gives 0 errors under mx check). Issue #836 | + +| `DESCRIBE MICROFLOW` emits **`on error rollback`** on activities authored with no error-handling clause at all, growing the diff on every round-trip. No checker flags it — `"Rollback"` is structurally valid, so `mx check` and every mxcli validator pass | `Rollback` is what `convertErrorHandlingType(nil)` stores for an activity with no clause **and** what the parser falls back to when `ErrorHandlingType` is absent from the BSON. The stored value therefore cannot distinguish an authored clause from the default, and read-back guessed "authored" | `mdl/executor/cmd_microflows_show_helpers.go` (`formatErrorHandlingSuffix`) | Drop the `Rollback` case so it falls through to no suffix. **The asymmetry is the whole argument**: omitting it is lossless (re-executing stores `Rollback` again, so the model is unchanged), while emitting it is lossy in the direction that matters — it puts a clause in the user's script that they never wrote. `Continue` / `Custom` / `CustomWithoutRollback` are never defaults, so they still round-trip. **Generalisable — the shape to look for**: when a formatter renders an enum whose zero/fallback value is also a legal authored value, read-back cannot invert the write; render only the values that are *never* defaults. Ask "what does the parser fall back to?" before trusting a stored enum to mean the author chose it. Repro `mdl-examples/bug-tests/840-describe-invents-on-error-rollback.mdl`; verified end-to-end (describe → exec → describe byte-identical, `mx check` 11.13.0 0 errors). Issue #840 | **Key insight:** `microflows$ListRange` stores offset/limit inside a nested `CustomRange` map — must cast `raw["CustomRange"].(map[string]any)` before diff --git a/.claude/skills/mendix/write-workflows.md b/.claude/skills/mendix/write-workflows.md index da27bcb90..0c2549faf 100644 --- a/.claude/skills/mendix/write-workflows.md +++ b/.claude/skills/mendix/write-workflows.md @@ -69,10 +69,10 @@ begin -- Call a microflow (server logic); optional parameter mapping + outcomes call microflow Module.ACT_Validate - with (Module.ACT_Validate.Item = '$workflowContext'); + with (Module.ACT_Validate.Item = '$WorkflowContext'); -- Decision: a boolean or enum exclusive split - decision '$workflowContext/Total > 1000' + decision '$WorkflowContext/Total > 1000' outcomes true -> { call microflow Module.ACT_Escalate; } false -> { call microflow Module.ACT_AutoApprove; }; @@ -182,6 +182,9 @@ documented in `system-module.md`. - A user task / decision with a single outcome and no activity can trip `CE1876` — give each branch a body or a distinct outcome. - The context **Parameter entity must be persistent**. +- Write the context variable as **`$WorkflowContext`**, matching the parameter + name exactly. Mendix expressions are case-sensitive on 11.9+, so a lowercase + `$workflowContext` is an undefined variable and yields `CE0117`. ## Validate before presenting diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b8654f259..c2daa36ff 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -23,6 +23,9 @@ Thank you for your interest in contributing to mxcli! This document explains wha - Go 1.26+ - Git - Make +- ANTLR4 and a JDK — `make build` regenerates the parser, and the generated + files in `mdl/grammar/parser/` are not committed. The dev container installs + both; on a local machine see "Option 2" below. - (Optional) Docker or Podman 4.7+ for dev container - (Optional) Claude Code / Cursor for agentic development @@ -40,10 +43,24 @@ cd mxcli ```bash git clone https://github.com/mendixlabs/mxcli cd mxcli + +# ANTLR4 — required by `make grammar`, which `make build` always runs. +# The version must match CI; the antlr4 wrapper reads it from this variable. +pip install 'antlr4-tools==0.2.2' +export ANTLR4_TOOLS_ANTLR_VERSION=4.13.2 + make build ./bin/mxcli --help ``` +**Option 3: Claude Code on the web** + +Web sessions do not use the dev container, so `.claude/hooks/session-start.sh` +installs ANTLR4 and warms the Go module cache at session start. To also +pre-cache MxBuild (needed for `mx check`), set +`MXCLI_HOOK_MXBUILD_VERSION=11.13.0` in the environment; otherwise fetch it on +demand with `mxcli setup mxbuild --version 11.13.0`. + ### Common Tasks ```bash diff --git a/cmd/mxcli/cmd_check.go b/cmd/mxcli/cmd_check.go index 2776c8b78..153268ba3 100644 --- a/cmd/mxcli/cmd_check.go +++ b/cmd/mxcli/cmd_check.go @@ -166,6 +166,11 @@ Examples: // not row-scoped, so the argument is unbound (CE1571) at build time. violations = append(violations, executor.ValidatePageButtonContext(prog)...) + // Flag a document-access GRANT naming a role from another module — Mendix + // rejects it with CE0148. Needs no project, so it runs here rather than + // under --references, where it would only fire with -p (#836). + violations = append(violations, executor.ValidateGrantRoles(prog)...) + if isStructured { // Always emit structured output (even when clean) formatter.Format(violations, os.Stderr) diff --git a/cmd/mxcli/cmd_widget_sync.go b/cmd/mxcli/cmd_widget_sync.go index c592aa31d..4a59df98d 100644 --- a/cmd/mxcli/cmd_widget_sync.go +++ b/cmd/mxcli/cmd_widget_sync.go @@ -21,6 +21,9 @@ import ( // widget Type it writes is byte-identical to Mendix's own output — but a value-level // difference not yet identified keeps DataGrid2 and Gallery instances erroring. The // help text says so rather than implying the command finishes the job. +// +// Both engines are supported: the modelsdk and legacy backends produce structurally +// identical output (verified over ~31k paths on four widgets). var widgetSyncCmd = &cobra.Command{ Use: "sync", @@ -33,7 +36,11 @@ fixture it clears 7 of 40 CE0463 errors; Mendix's own 'mx update-widgets' clears all 40 but destroys the mprcontents/ folder on MPR v2 projects, which this does not. Preview with --dry-run and verify with 'mx check' before relying on it. -Applying currently requires MXCLI_ENGINE=legacy; --dry-run works on both engines. +Runs on either engine; both produce identical output. + +Every unit is checked for duplicate GUIDs before it is written, and the run aborts +rather than persisting one: Mendix accepts a duplicate on load and on 'mx check', +then refuses to save the project. mxcli writes a widget instance correctly for the package installed at authoring time. When that package is later upgraded, the stored instances go stale and diff --git a/docs-site/src/guides/pluggable-widgets.md b/docs-site/src/guides/pluggable-widgets.md index 1655fca66..7e11f1702 100644 --- a/docs-site/src/guides/pluggable-widgets.md +++ b/docs-site/src/guides/pluggable-widgets.md @@ -193,8 +193,15 @@ installed rather than guessing. clears 7 of 40 CE0463 errors where `mx update-widgets` clears all 40. The widget *type* it writes is byte-identical to Mendix's own output; a value-level difference that has not yet been identified keeps DataGrid2 and Gallery instances erroring. -Preview with `--dry-run` and confirm with `mx check` before relying on it. Applying -currently needs `MXCLI_ENGINE=legacy`; `--dry-run` works on either engine. +Preview with `--dry-run` and confirm with `mx check` before relying on it. It runs on +either engine — the modelsdk and legacy backends produce structurally identical +output. + +Each unit is verified for duplicate GUIDs before being written, and the run aborts +rather than writing one. This matters because Mendix validates GUID uniqueness only +when *saving*: a duplicate loads fine and passes `mx check`, then breaks the next save +— and `mx update-widgets` collapses `mprcontents/` before it discovers the problem, +leaving the project both flattened and unloadable. ## Marketplace and custom widgets diff --git a/docs/11-proposals/PROPOSAL_widget_instance_reconciliation.md b/docs/11-proposals/PROPOSAL_widget_instance_reconciliation.md index 9744ad247..54045230c 100644 --- a/docs/11-proposals/PROPOSAL_widget_instance_reconciliation.md +++ b/docs/11-proposals/PROPOSAL_widget_instance_reconciliation.md @@ -17,7 +17,7 @@ Shipped and verified: | Coverage | pages, snippets, **building blocks**, page templates, layouts; **zero misses** vs `update-widgets` | | Widget `Type` written | **byte-identical** to `update-widgets` output (0 differing paths) | | Idempotent | second run reports "already matches its installed package" | -| Engines | plan runs on both and they agree; **apply needs `MXCLI_ENGINE=legacy`** | +| Engines | plan **and apply** run on both; output is structurally identical (~31k paths compared) | Where it stops: with the `Type` exact, 17 DataGrid2 and Gallery instances still report CE0463, so the trigger is value-level. Four candidate value migrations were @@ -28,9 +28,10 @@ migration). The untested fourth is a TextTemplate null scoped to added propertie only — the blanket version takes 33 → 127. Next moves, in order: the scoped TextTemplate null; if that fails, the splice -bisection from [`diagnose-ce0463.md`](../../.claude/skills/diagnose-ce0463.md); -then `GetRawUnitBytes`/`UpdateRawUnit` on the modelsdk backend to drop the engine -gate. +bisection from [`diagnose-ce0463.md`](../../.claude/skills/diagnose-ce0463.md). + +The modelsdk engine gate is CLOSED — `GetRawUnitBytes`/`UpdateRawUnit` are exposed on +that backend and apply runs on either engine. Open question 2 (retyped property) and 3 (implicit sync during build) remain untouched. Open question 1 (naming) resolved as `sync`. diff --git a/mdl-examples/bug-tests/836-check-cross-module-grant.fail.mdl b/mdl-examples/bug-tests/836-check-cross-module-grant.fail.mdl new file mode 100644 index 000000000..cff9b57b3 --- /dev/null +++ b/mdl-examples/bug-tests/836-check-cross-module-grant.fail.mdl @@ -0,0 +1,46 @@ +-- Bug #836: `check --references` passed a GRANT that fails at exec with CE0148. +-- +-- Granting a document access to a module role from a DIFFERENT module is +-- rejected by Mendix with CE0148 ("reselect roles"). mxcli had a guard for this +-- (checkDocumentAccessRolesSameModule) wired into all five exec paths, but the +-- validate path never called it, so: +-- +-- mxcli check script.mdl -p app.mpr --references +-- -> "✓ All references valid" / "Check passed!" +-- +-- mxcli exec script.mdl -p app.mpr +-- -> Created module: ZKT27A +-- Created module role: ZKT27A.Role1 +-- Created module: ZKT27B +-- Created module role: ZKT27B.RoleB +-- Created microflow: ZKT27B.MF_Test +-- Error: cannot grant microflow ZKT27B.MF_Test access to ZKT27A.Role1 ... +-- +-- Note where the error lands: five statements had already been applied. mxcli +-- does not run a script in a single transaction, so a failure that only shows +-- up at exec time leaves the project half-modified — which is exactly what a +-- pre-flight check exists to prevent. +-- +-- Fix: validateWithContext calls the same guard via validateCrossModuleGrant, +-- covering all five document-access grants (microflow, nanoflow, page, OData +-- service, published REST service). The document's module comes from the +-- statement's own qualified name, so the check works before the document +-- exists — it may be created earlier in the same script, as it is here. +-- +-- This is a NEGATIVE test: `make check-mdl` expects it to fail. +-- Verified: check now exits 1 naming statement 6 and CE0148, and the project is +-- left untouched. Swapping the role to ZKT27B.RoleB passes check, executes, and +-- gives 0 errors under mx check. + +create module ZKT27A; +create module role ZKT27A.Role1; + +create module ZKT27B; +create module role ZKT27B.RoleB; +create microflow ZKT27B."MF_Test" () +begin + declare $X Boolean = true; +end; + +-- ZKT27A.Role1 belongs to a different module than the microflow -> CE0148. +grant execute on microflow ZKT27B."MF_Test" to ZKT27A.Role1; diff --git a/mdl-examples/bug-tests/840-describe-invents-on-error-rollback.mdl b/mdl-examples/bug-tests/840-describe-invents-on-error-rollback.mdl new file mode 100644 index 000000000..a2978ddc1 --- /dev/null +++ b/mdl-examples/bug-tests/840-describe-invents-on-error-rollback.mdl @@ -0,0 +1,47 @@ +-- Bug #840: DESCRIBE MICROFLOW invented an `on error rollback` clause. +-- +-- An activity authored with no error-handling clause came back from describe +-- carrying one: +-- +-- authored: call microflow Issue840.MF_Leaf(); +-- described: call microflow Issue840.MF_Leaf() on error rollback; +-- +-- "Rollback" is what convertErrorHandlingType(nil) stores for an activity with +-- no clause, and what the parser falls back to when ErrorHandlingType is absent +-- from the BSON. So the stored value cannot distinguish "the author wrote +-- `on error rollback`" from "the author wrote nothing". +-- +-- Nothing could catch it: "Rollback" is structurally valid, so mx check and +-- every mxcli validator pass. It simply grew the diff on each describe +-- round-trip and put a clause in the user's script that they never wrote. +-- +-- Fix (mdl/executor/cmd_microflows_show_helpers.go): formatErrorHandlingSuffix +-- no longer emits a suffix for Rollback. This is the lossless direction — +-- re-executing the output stores Rollback again, so the model is unchanged. +-- Continue / Custom / CustomWithoutRollback are never defaults, so they still +-- round-trip: their presence always means the author wrote them. +-- +-- Manual verification (needs a project, so `make check-mdl` only syntax-checks this): +-- +-- mxcli exec 840-describe-invents-on-error-rollback.mdl -p app.mpr +-- mxcli -p app.mpr -c "describe microflow Issue840.MF_Caller" +-- +-- Expect the first call to describe WITHOUT any on-error clause, and the second +-- to keep `on error continue`. Feeding that output back through `mxcli exec` +-- and describing again must produce byte-identical output. + +create module Issue840; +create module role Issue840.User; + +create microflow Issue840."MF_Leaf" () +begin + declare $X Boolean = true; +end; + +create microflow Issue840."MF_Caller" () +begin + -- No clause: must NOT come back as `on error rollback`. + call microflow Issue840.MF_Leaf(); + -- Explicit non-default: must survive the round-trip. + call microflow Issue840.MF_Leaf() on error continue; +end; diff --git a/mdl-examples/bug-tests/841-json-structure-occurrences.mdl b/mdl-examples/bug-tests/841-json-structure-occurrences.mdl new file mode 100644 index 000000000..07e258a5f --- /dev/null +++ b/mdl-examples/bug-tests/841-json-structure-occurrences.mdl @@ -0,0 +1,84 @@ +-- Bug #841: `create json structure` wrote every element's occurrence as `0..0`. +-- +-- Mendix reads MaxOccurs=0 literally as "never occurs", so every import mapping +-- bound to the structure silently imported zero objects: the REST call +-- succeeded, the mapping looked correct, and no data arrived. Nothing flagged +-- it — no validation error, and mx check passed. +-- +-- Observed before the fix, for the snippet below: +-- +-- element type min..max +-- Page Value 0..0 +-- Pagination Object 0..0 +-- ItemsItem Object 0..-1 <- the one element that was right +-- Items Array 0..0 +-- Root Object 0..0 +-- +-- The lone correct row is the tell: the nested object-array item was written +-- with MaxOccurs=-1, so the writer could always emit unbounded — every other +-- construction site just hardcoded 0. +-- +-- Fix (mdl/types/json_utils.go): occurrences follow the JSON shape. +-- Root 1..1 (a document root occurs exactly once) +-- Object / Value 0..1 (optional, at most once) +-- Array 0..1 (the array occurs once; its item repeats) +-- Array item 0..* (MaxOccurs = -1, unbounded) +-- Wrapper 0..* (the repeating unit of a primitive array) +-- +-- Manual verification (needs a project, so `make check-mdl` only syntax-checks this): +-- +-- mxcli exec 841-json-structure-occurrences.mdl -p app.mpr +-- mx check -p app.mpr +-- +-- Expect 0 errors, and no element in the stored BSON with MaxOccurs=0. + +create module Issue841; +create module role Issue841.User; + +-- Object root with a nested object, an object array, and a primitive array. +create json structure Issue841."JSON_SearchRoutes" + snippet $${"pagination":{"page":1,"pageSize":1,"totalItems":10,"totalPages":10},"items":[{"id":"a","name":"b"}],"tags":["x","y"]}$$; + +-- Array root: the other top-level shape, which used a separate builder. +create json structure Issue841."JSON_RouteList" + snippet $$[{"id":"a","name":"b"}]$$; + +-- Mappings BOUND to a structure. This half guards the follow-up defect: Mendix +-- cross-validates every mapping element's occurrence against its schema element +-- and reports CE5015 on a mismatch — +-- +-- [error] [CE5015] "The mapping does not align with the underlying schema +-- anymore. Details: Attribute 'MaxOccurs' does not match schema element +-- '(Object)/id'." at Value mapping element '_id' +-- +-- The first cut of this fix changed the schema side only. Import writers already +-- propagated occurrences, but all three EXPORT writers hardcoded `MaxOccurs: 0` +-- on value elements, so raising the schema to 0..1 broke every export mapping. +-- Structures alone cannot catch this: the original repro created no mappings, so +-- it passed mx check while the doctype suite went red. +create json structure Issue841."JSON_Route" + snippet $${"id":"a","name":"b"}$$; + +@position(100, 300) +create non-persistent entity Issue841.Route ( + RouteId: string(200), + RouteName: string(200) +); + +create import mapping Issue841."IMM_Route" + with json structure Issue841.JSON_Route +{ + create Issue841.Route { + RouteId = id, + RouteName = name + } +}; + +create export mapping Issue841."EMM_Route" + with json structure Issue841.JSON_Route +{ + Issue841.Route { + id = RouteId, + name = RouteName + } +}; diff --git a/mdl-examples/bug-tests/845-workflow-decision-context-casing.mdl b/mdl-examples/bug-tests/845-workflow-decision-context-casing.mdl new file mode 100644 index 000000000..eab949f4b --- /dev/null +++ b/mdl-examples/bug-tests/845-workflow-decision-context-casing.mdl @@ -0,0 +1,47 @@ +-- Bug #845: workflow DECISION expressions were not case-normalized. +-- +-- The workflow context parameter is stored as "WorkflowContext". Mendix +-- expressions are case-sensitive on 11.9+, so a user-written `$workflowContext` +-- is an undefined variable. CALL MICROFLOW `WITH` expressions were already +-- normalized (FINDINGS #39), but DECISION expressions were stored verbatim: +-- +-- "Expression": "$workflowContext/IsExclusive" +-- +-- mx check then reported, against an otherwise clean project: +-- +-- [error] [CE0117] "Error(s) in expression." at Decision 'Decision' +-- +-- The inconsistency is what hid the bug — the same spelling works in a WITH +-- clause and fails in a DECISION. The shipped write-workflows skill documented +-- the lowercase form in its DECISION example, so following the docs produced a +-- project that would not build. +-- +-- Fix: buildExclusiveSplit (and its sibling buildWaitForTimer, whose delay may +-- reference a context date attribute) run the authored expression through +-- normalizeWorkflowContextExpr, the same helper autoBindCallMicroflow uses. +-- +-- Manual verification (needs a project, so `make check-mdl` only syntax-checks this): +-- +-- mxcli exec 845-workflow-decision-context-casing.mdl -p app.mpr +-- mx check -p app.mpr +-- +-- Expect 0 errors. Before the fix this produced CE0117 on the decision. + +create module Issue845; +create module role Issue845.User; + +@position(100, 100) +create persistent entity Issue845.Ctx ( + IsExclusive: boolean, + Total: decimal +); + +-- Lowercase on purpose: this is the form the docs used to show. +create workflow Issue845."WF_DecisionCasing" + parameter $WorkflowContext: Issue845.Ctx +begin + decision '$workflowContext/IsExclusive' + outcomes + true -> { } + false -> { }; +end workflow; diff --git a/mdl/backend/modelsdk/mapping_write.go b/mdl/backend/modelsdk/mapping_write.go index d2aae2fc9..0e053a9d3 100644 --- a/mdl/backend/modelsdk/mapping_write.go +++ b/mdl/backend/modelsdk/mapping_write.go @@ -361,7 +361,10 @@ func exportValueElementToGen(id string, elem *model.ExportMappingElement, parent addStr(g, "XmlPath", "") addPart(g, "Type", mappingValueDataTypeToGen(elem.DataType)) addInt32(g, "MinOccurs", 0) - addInt32(g, "MaxOccurs", 0) + // Mirror the bound schema element: Mendix cross-validates the two and + // reports CE5015 on any mismatch. Hardcoding 0 only worked while the JSON + // structure also wrote 0 for every element (#841). + addInt32(g, "MaxOccurs", int32(elem.MaxOccurs)) addBool(g, "Nillable", true) addBool(g, "IsDefaultType", false) addStr(g, "ElementType", "Value") diff --git a/mdl/backend/modelsdk/units.go b/mdl/backend/modelsdk/units.go index d26b1a607..665a1e487 100644 --- a/mdl/backend/modelsdk/units.go +++ b/mdl/backend/modelsdk/units.go @@ -20,6 +20,22 @@ func (b *Backend) GetRawUnit(id model.ID) (map[string]any, error) { return b.reader.GetRawUnit(id) } +// GetRawUnitBytes returns a unit's raw BSON. Both this and UpdateRawUnit already +// existed on the reader/writer and are used throughout this package; they were simply +// never exposed as Backend methods, so the embedded `unimplemented` stub answered and +// every caller going through the interface got "not implemented yet — rerun with +// MXCLI_ENGINE=legacy". That is what gated `mxcli widget sync --apply` to the legacy +// engine while its read-only plan ran on both. +func (b *Backend) GetRawUnitBytes(id model.ID) ([]byte, error) { + return b.reader.GetRawUnitBytes(string(id)) +} + +// UpdateRawUnit replaces a unit's contents. Takes a string ID to match the SDK writer +// layer convention (see backend.RawUnitBackend). +func (b *Backend) UpdateRawUnit(unitID string, contents []byte) error { + return b.writer.UpdateRawUnit(unitID, contents) +} + // ListRawUnitsByType returns every unit whose $Type has the given prefix, with // resolved raw contents — the catalog uses this for document types that have no // dedicated typed reader (e.g. JavaScript actions, data transformers). Delegates diff --git a/mdl/executor/cmd_microflows_show_helpers.go b/mdl/executor/cmd_microflows_show_helpers.go index 08ef5a547..55b6491a3 100644 --- a/mdl/executor/cmd_microflows_show_helpers.go +++ b/mdl/executor/cmd_microflows_show_helpers.go @@ -1749,18 +1749,26 @@ func objectTerminatesBeforeMerge( // formatErrorHandlingSuffix returns the ON ERROR suffix for an activity based on its ErrorHandlingType. // Returns empty string if no special error handling. +// +// Rollback is deliberately absent: it is what convertErrorHandlingType(nil) +// stores for an activity written with no clause, and what the parser falls back +// to when ErrorHandlingType is missing from the BSON. Read-back therefore cannot +// tell an authored `on error rollback` from the default, and emitting it invents +// a clause on activities that never had one (#840). Dropping it is lossless — +// re-executing the output stores Rollback again — while emitting it is not. +// +// The remaining values are only ever set explicitly, so their presence in the +// model always means the author wrote them. func formatErrorHandlingSuffix(errType microflows.ErrorHandlingType) string { switch errType { case microflows.ErrorHandlingTypeContinue: return " on error continue" - case microflows.ErrorHandlingTypeRollback: - return " on error rollback" case microflows.ErrorHandlingTypeCustom: return " on error" // Will be followed by block case microflows.ErrorHandlingTypeCustomWithoutRollback: return " on error without rollback" // Will be followed by block default: - return "" // Abort is the default, no suffix needed + return "" // Rollback (the stored default) and Abort emit nothing } } diff --git a/mdl/executor/cmd_microflows_show_helpers_test.go b/mdl/executor/cmd_microflows_show_helpers_test.go index 4ce2bedae..c2bb1ac50 100644 --- a/mdl/executor/cmd_microflows_show_helpers_test.go +++ b/mdl/executor/cmd_microflows_show_helpers_test.go @@ -227,13 +227,18 @@ func TestPrependFreeAnnotationLines_ModelAnnotationsStayFree(t *testing.T) { // formatErrorHandlingSuffix // ============================================================================= +// Rollback is the value stored when the author wrote no clause at all +// (convertErrorHandlingType(nil)) and the value the parser falls back to when +// the BSON key is absent. It therefore carries no information about what the +// author wrote, and read-back must not emit it — see issue #840 and +// TestFormatErrorHandlingSuffix_RollbackIsNotEmitted below. func TestFormatErrorHandlingSuffix(t *testing.T) { tests := []struct { errType microflows.ErrorHandlingType want string }{ {microflows.ErrorHandlingTypeContinue, " on error continue"}, - {microflows.ErrorHandlingTypeRollback, " on error rollback"}, + {microflows.ErrorHandlingTypeRollback, ""}, {microflows.ErrorHandlingTypeCustom, " on error"}, {microflows.ErrorHandlingTypeCustomWithoutRollback, " on error without rollback"}, {microflows.ErrorHandlingTypeAbort, ""}, @@ -502,3 +507,38 @@ func TestFormatActivity_ErrorEvent(t *testing.T) { t.Errorf("got %q, want %q", got, "raise error;") } } + +// TestFormatErrorHandlingSuffix_RollbackIsNotEmitted guards issue #840. +// +// DESCRIBE MICROFLOW invented an `on error rollback` clause on activities that +// were never written with one: +// +// call microflow TFC.MF_Leaf(); -- authored +// call microflow TFC.MF_Leaf() on error rollback; -- described +// +// "Rollback" is what convertErrorHandlingType(nil) stores for an activity with +// no clause, and what the parser falls back to when ErrorHandlingType is absent +// from the BSON. It is structurally valid, so no checker can flag the invented +// clause — it just quietly grows the diff on every describe round-trip. +// +// Emitting nothing for Rollback is lossless: re-executing describe output +// without the clause stores Rollback again, so the model round-trips exactly. +// Emitting it is the lossy direction, because it adds a clause to the script +// that the author did not write. +func TestFormatErrorHandlingSuffix_RollbackIsNotEmitted(t *testing.T) { + if got := formatErrorHandlingSuffix(microflows.ErrorHandlingTypeRollback); got != "" { + t.Errorf("Rollback suffix = %q, want \"\" — Rollback is the stored default and cannot be distinguished from an authored clause", got) + } + // The explicit-only values must still round-trip: none of them is ever a + // default, so their presence always means the author wrote them. + explicit := map[microflows.ErrorHandlingType]string{ + microflows.ErrorHandlingTypeContinue: " on error continue", + microflows.ErrorHandlingTypeCustom: " on error", + microflows.ErrorHandlingTypeCustomWithoutRollback: " on error without rollback", + } + for errType, want := range explicit { + if got := formatErrorHandlingSuffix(errType); got != want { + t.Errorf("formatErrorHandlingSuffix(%q) = %q, want %q", errType, got, want) + } + } +} diff --git a/mdl/executor/cmd_security_defaults.go b/mdl/executor/cmd_security_defaults.go index 160924727..7cc8e9275 100644 --- a/mdl/executor/cmd_security_defaults.go +++ b/mdl/executor/cmd_security_defaults.go @@ -40,6 +40,34 @@ func checkDocumentAccessRolesSameModule(docKind, docModule, docName string, role return nil } +// validateCrossModuleGrant applies the CE0148 guard above to a GRANT statement +// without touching the project, so `check --references` reports it instead of +// letting exec fail partway through a script (#836). mxcli does not run a script +// in a single transaction, so a grant that only fails at exec time leaves every +// preceding statement applied — exactly what a pre-flight check exists to avoid. +// +// The document's module is taken from the statement's own qualified name rather +// than from the resolved document: the comparison is the same, and this keeps +// the check usable before the document exists (it may be created earlier in the +// same script). +// +// Statements other than the five document-access grants return nil. +func validateCrossModuleGrant(stmt ast.Statement) error { + switch s := stmt.(type) { + case *ast.GrantMicroflowAccessStmt: + return checkDocumentAccessRolesSameModule("microflow", s.Microflow.Module, s.Microflow.Name, s.Roles) + case *ast.GrantNanoflowAccessStmt: + return checkDocumentAccessRolesSameModule("nanoflow", s.Nanoflow.Module, s.Nanoflow.Name, s.Roles) + case *ast.GrantPageAccessStmt: + return checkDocumentAccessRolesSameModule("page", s.Page.Module, s.Page.Name, s.Roles) + case *ast.GrantODataServiceAccessStmt: + return checkDocumentAccessRolesSameModule("OData service", s.Service.Module, s.Service.Name, s.Roles) + case *ast.GrantPublishedRestServiceAccessStmt: + return checkDocumentAccessRolesSameModule("published REST service", s.Service.Module, s.Service.Name, s.Roles) + } + return nil +} + const ( autoDocumentRoleName = "User" autoDocumentRoleDescription = "Auto-created default role for mxcli document access" diff --git a/mdl/executor/cmd_workflows_decision_test.go b/mdl/executor/cmd_workflows_decision_test.go new file mode 100644 index 000000000..aae6c9904 --- /dev/null +++ b/mdl/executor/cmd_workflows_decision_test.go @@ -0,0 +1,46 @@ +package executor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" +) + +// TestBuildExclusiveSplit_NormalizesWorkflowContext guards issue #845. +// +// The workflow context parameter is stored as "WorkflowContext", and Mendix +// expressions are case-sensitive on 11.9+, so a user-written `$workflowContext` +// is an undefined variable. autoBindCallMicroflow already normalizes CALL +// MICROFLOW `WITH` expressions (FINDINGS #39), but DECISION expressions were +// stored verbatim, so the lowercase form reached the .mpr and mx check reported +// +// [error] [CE0117] "Error(s) in expression." at Decision 'Decision' +// +// The inconsistency is what made this hard to spot: the same `$workflowContext` +// spelling works in a WITH clause and fails in a DECISION. +func TestBuildExclusiveSplit_NormalizesWorkflowContext(t *testing.T) { + cases := map[string]string{ + "$workflowContext/IsExclusive": "$WorkflowContext/IsExclusive", + "$WORKFLOWCONTEXT/Total > 100": "$WorkflowContext/Total > 100", + "$WorkflowContext/IsExclusive": "$WorkflowContext/IsExclusive", + "$Other/Field": "$Other/Field", + } + for in, want := range cases { + act := buildExclusiveSplit(&ast.WorkflowDecisionNode{Expression: in}) + if act.Expression != want { + t.Errorf("buildExclusiveSplit(%q).Expression = %q, want %q", in, act.Expression, want) + } + } +} + +// TestBuildWaitForTimer_NormalizesWorkflowContext covers the same defect in the +// sibling expression site: a WAIT FOR TIMER delay may reference a date attribute +// on the workflow context, and was likewise stored verbatim. +func TestBuildWaitForTimer_NormalizesWorkflowContext(t *testing.T) { + act := buildWaitForTimer(&ast.WorkflowWaitForTimerNode{ + DelayExpression: "$workflowContext/DueDate", + }) + if want := "$WorkflowContext/DueDate"; act.DelayExpression != want { + t.Errorf("buildWaitForTimer.DelayExpression = %q, want %q", act.DelayExpression, want) + } +} diff --git a/mdl/executor/cmd_workflows_write.go b/mdl/executor/cmd_workflows_write.go index 1cad00368..2247c35fe 100644 --- a/mdl/executor/cmd_workflows_write.go +++ b/mdl/executor/cmd_workflows_write.go @@ -355,7 +355,10 @@ func buildCallWorkflowActivity(n *ast.WorkflowCallWorkflowNode) *workflows.CallW func buildExclusiveSplit(n *ast.WorkflowDecisionNode) *workflows.ExclusiveSplitActivity { act := &workflows.ExclusiveSplitActivity{} act.ID = model.ID(generateWorkflowUUID()) - act.Expression = n.Expression + // Same case-sensitivity trap as CALL MICROFLOW parameter mappings (#845): + // the context parameter is named "WorkflowContext", so a user-written + // `$workflowContext` is an undefined variable and mx check reports CE0117. + act.Expression = normalizeWorkflowContextExpr(n.Expression) act.Caption = n.Caption if act.Caption == "" { @@ -459,7 +462,8 @@ func buildJumpTo(n *ast.WorkflowJumpToNode) *workflows.JumpToActivity { func buildWaitForTimer(n *ast.WorkflowWaitForTimerNode) *workflows.WaitForTimerActivity { act := &workflows.WaitForTimerActivity{} act.ID = model.ID(generateWorkflowUUID()) - act.DelayExpression = n.DelayExpression + // A delay may reference a date attribute on the workflow context (#845). + act.DelayExpression = normalizeWorkflowContextExpr(n.DelayExpression) act.Caption = n.Caption if act.Caption == "" { diff --git a/mdl/executor/validate.go b/mdl/executor/validate.go index 5371d4b49..536282f16 100644 --- a/mdl/executor/validate.go +++ b/mdl/executor/validate.go @@ -271,6 +271,14 @@ func (e *Executor) CheckProjectConflicts(prog *ast.Program) []error { // validateWithContext validates a statement, considering objects defined in the script. func validateWithContext(ctx *ExecContext, stmt ast.Statement, sc *scriptContext) error { + // Cross-module document-access grants (CE0148) are rejected at exec time by + // checkDocumentAccessRolesSameModule. Run the same guard here so the failure + // surfaces during --references instead of partway through a script (#836). + // The check needs no project state, so it runs before the switch. + if err := validateCrossModuleGrant(stmt); err != nil { + return err + } + switch s := stmt.(type) { // Statements that reference modules case *ast.CreateEntityStmt: diff --git a/mdl/executor/validate_grant_crossmodule_test.go b/mdl/executor/validate_grant_crossmodule_test.go new file mode 100644 index 000000000..020b71156 --- /dev/null +++ b/mdl/executor/validate_grant_crossmodule_test.go @@ -0,0 +1,80 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" +) + +// TestValidateStatement_CrossModuleGrant guards issue #836. +// +// `mxcli check --references` reported "All references valid / Check passed!" +// for a script whose GRANT names a role from a different module than the +// document. Execution then failed with the CE0148 guard — but only after the +// preceding statements had already been applied, because mxcli does not run a +// script in a single transaction. The whole point of --references is to catch +// this before anything is written. +// +// The guard existed (checkDocumentAccessRolesSameModule) and was called from +// all five exec paths; it was simply never reached from the validate path. +func TestValidateStatement_CrossModuleGrant(t *testing.T) { + crossModule := []ast.QualifiedName{{Module: "ZKT27A", Name: "Role1"}} + sameModule := []ast.QualifiedName{{Module: "ZKT27B", Name: "RoleB"}} + + cases := []struct { + name string + cross ast.Statement + same ast.Statement + }{ + { + "microflow", + &ast.GrantMicroflowAccessStmt{Microflow: ast.QualifiedName{Module: "ZKT27B", Name: "MF_Test"}, Roles: crossModule}, + &ast.GrantMicroflowAccessStmt{Microflow: ast.QualifiedName{Module: "ZKT27B", Name: "MF_Test"}, Roles: sameModule}, + }, + { + "nanoflow", + &ast.GrantNanoflowAccessStmt{Nanoflow: ast.QualifiedName{Module: "ZKT27B", Name: "NF_Test"}, Roles: crossModule}, + &ast.GrantNanoflowAccessStmt{Nanoflow: ast.QualifiedName{Module: "ZKT27B", Name: "NF_Test"}, Roles: sameModule}, + }, + { + "page", + &ast.GrantPageAccessStmt{Page: ast.QualifiedName{Module: "ZKT27B", Name: "P_Test"}, Roles: crossModule}, + &ast.GrantPageAccessStmt{Page: ast.QualifiedName{Module: "ZKT27B", Name: "P_Test"}, Roles: sameModule}, + }, + { + "odata service", + &ast.GrantODataServiceAccessStmt{Service: ast.QualifiedName{Module: "ZKT27B", Name: "Svc"}, Roles: crossModule}, + &ast.GrantODataServiceAccessStmt{Service: ast.QualifiedName{Module: "ZKT27B", Name: "Svc"}, Roles: sameModule}, + }, + { + "published rest service", + &ast.GrantPublishedRestServiceAccessStmt{Service: ast.QualifiedName{Module: "ZKT27B", Name: "Svc"}, Roles: crossModule}, + &ast.GrantPublishedRestServiceAccessStmt{Service: ast.QualifiedName{Module: "ZKT27B", Name: "Svc"}, Roles: sameModule}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := validateCrossModuleGrant(tc.cross) + if err == nil { + t.Fatal("cross-module grant should be reported by validation, not only at exec time") + } + if !strings.Contains(err.Error(), "CE0148") { + t.Errorf("error should name CE0148 so the user can search for it, got: %v", err) + } + if err := validateCrossModuleGrant(tc.same); err != nil { + t.Errorf("same-module grant must stay valid, got: %v", err) + } + }) + } +} + +// Statements that are not grants must be ignored by the check. +func TestValidateCrossModuleGrant_IgnoresOtherStatements(t *testing.T) { + if err := validateCrossModuleGrant(&ast.CreateEntityStmt{}); err != nil { + t.Errorf("non-grant statement should be ignored, got: %v", err) + } +} diff --git a/mdl/executor/validate_grant_roles.go b/mdl/executor/validate_grant_roles.go new file mode 100644 index 000000000..3a30f0bda --- /dev/null +++ b/mdl/executor/validate_grant_roles.go @@ -0,0 +1,37 @@ +// SPDX-License-Identifier: Apache-2.0 + +// Check-time (no-project) validation for document-access GRANT statements. +// Mendix stores document access as references to the document's OWN module roles +// only, so granting a page/microflow/nanoflow/service access to a role from a +// different module builds with CE0148 ("reselect roles"). The comparison is +// purely between the two qualified names in the statement, so it needs no +// project — see issue #836. +package executor + +import ( + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/linter" +) + +// ValidateGrantRoles reports (MDL-GRANT01) a GRANT that names a module role from +// a different module than the document it targets. +// +// This lives in the no-project pass rather than the --references pass on +// purpose: the check compares two names already present in the script, so +// requiring -p would withhold an answer mxcli can always give. It also means a +// plain `mxcli check` catches it, not only `check --references`. +func ValidateGrantRoles(prog *ast.Program) []linter.Violation { + var out []linter.Violation + for _, stmt := range prog.Statements { + if err := validateCrossModuleGrant(stmt); err != nil { + out = append(out, linter.Violation{ + RuleID: "MDL-GRANT01", + Severity: linter.SeverityError, + Message: err.Error(), + Suggestion: "Grant a module role from the document's own module, then map the user role to it " + + "(`alter user role add .`).", + }) + } + } + return out +} diff --git a/mdl/executor/validate_grant_roles_test.go b/mdl/executor/validate_grant_roles_test.go new file mode 100644 index 000000000..0e228606a --- /dev/null +++ b/mdl/executor/validate_grant_roles_test.go @@ -0,0 +1,58 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/linter" +) + +// ValidateGrantRoles is what `mxcli check` calls. It must fire without a +// project: the check compares two names already present in the script, so +// requiring -p would withhold an answer mxcli can always give (#836). +func TestValidateGrantRoles_ReportsCrossModuleWithoutProject(t *testing.T) { + prog := &ast.Program{Statements: []ast.Statement{ + &ast.CreateModuleStmt{Name: "ZKT27B"}, + &ast.GrantMicroflowAccessStmt{ + Microflow: ast.QualifiedName{Module: "ZKT27B", Name: "MF_Test"}, + Roles: []ast.QualifiedName{{Module: "ZKT27A", Name: "Role1"}}, + }, + }} + got := ValidateGrantRoles(prog) + if len(got) != 1 { + t.Fatalf("got %d violations, want 1", len(got)) + } + if got[0].RuleID != "MDL-GRANT01" { + t.Errorf("RuleID = %q, want MDL-GRANT01", got[0].RuleID) + } + if got[0].Severity != linter.SeverityError { + t.Errorf("Severity = %v, want error — this fails the build, not a style nit", got[0].Severity) + } + if !strings.Contains(got[0].Message, "CE0148") { + t.Errorf("message should name CE0148, got: %s", got[0].Message) + } + if got[0].Suggestion == "" { + t.Error("a violation the user must act on needs a suggestion") + } +} + +// A same-module grant must not be reported — the guard has to be silent on the +// correct form, or it just trains people to ignore it. +func TestValidateGrantRoles_SameModuleIsClean(t *testing.T) { + prog := &ast.Program{Statements: []ast.Statement{ + &ast.GrantMicroflowAccessStmt{ + Microflow: ast.QualifiedName{Module: "ZKT27B", Name: "MF_Test"}, + Roles: []ast.QualifiedName{{Module: "ZKT27B", Name: "RoleB"}}, + }, + &ast.GrantPageAccessStmt{ + Page: ast.QualifiedName{Module: "Sales", Name: "Overview"}, + Roles: []ast.QualifiedName{{Module: "Sales", Name: "User"}, {Module: "Sales", Name: "Admin"}}, + }, + }} + if got := ValidateGrantRoles(prog); len(got) != 0 { + t.Errorf("same-module grants must not be reported, got %d: %+v", len(got), got) + } +} diff --git a/mdl/executor/widget_convert.go b/mdl/executor/widget_convert.go index fda85048a..189401c92 100644 --- a/mdl/executor/widget_convert.go +++ b/mdl/executor/widget_convert.go @@ -10,6 +10,7 @@ import ( "go.mongodb.org/mongo-driver/bson/primitive" "github.com/mendixlabs/mxcli/mdl/bsonutil" + "github.com/mendixlabs/mxcli/mdl/types" ) // widget_convert.go moves a stored widget subtree between the two representations the @@ -107,3 +108,105 @@ func mapValueToWidgetBSON(key string, v any) any { func isWidgetIDField(key string) bool { return key == "$ID" || strings.HasSuffix(key, "Pointer") } + +// ensureUniqueWidgetIDs guarantees every $ID in a widget subtree is distinct. +// +// This is not defensive tidying — without it `mxcli widget sync` CORRUPTS the project. +// AugmentTemplate adds a property to an object-list property (a DataGrid2 column set) +// by giving every list entry a copy of the same constructed node. Those copies carry +// the same placeholder ID, so remapping placeholder->UUID by VALUE assigns all of them +// one UUID. The result loads and passes `mx check`, then fails at save time with +// +// System.InvalidOperationException: Duplicate Guid in unit page template '...' +// +// and — because `mx update-widgets` collapses mprcontents/ BEFORE it fails to save — +// leaves the project both flattened and unloadable ("Root unit not found"). Reported +// against PR #89 on a real project; the template pipeline never hit it because a +// template has exactly one list entry. +// +// Scoping matters. A TypePointer that repeats across list entries is CORRECT: every +// column's WidgetProperty for a given key points at the one shared WidgetPropertyType. +// So only $ID is made unique, and when an $ID is regenerated, references to it are +// rewritten only within the same subtree — never across siblings. +func ensureUniqueWidgetIDs(v any, seen map[string]bool) any { + return uniquifyIDs(v, seen, map[string]string{}) +} + +func uniquifyIDs(v any, seen map[string]bool, scope map[string]string) any { + switch t := v.(type) { + case map[string]any: + out := make(map[string]any, len(t)) + // The node's own $ID first, so children can be rewritten against it. + if id, ok := t["$ID"].(string); ok { + if seen[id] { + fresh := types.GenerateID() + scope[id] = fresh + out["$ID"] = fresh + seen[fresh] = true + } else { + seen[id] = true + out["$ID"] = id + } + } + for k, val := range t { + if k == "$ID" { + continue + } + if s, ok := val.(string); ok && isWidgetIDField(k) { + if fresh, remapped := scope[s]; remapped { + out[k] = fresh + continue + } + out[k] = s + continue + } + out[k] = uniquifyIDs(val, seen, scope) + } + return out + case []any: + out := make([]any, len(t)) + for i, item := range t { + // Each list entry is its own reference scope: sibling entries legitimately + // share pointers to the schema, but must not share $IDs. + child := map[string]string{} + for k, val := range scope { + child[k] = val + } + out[i] = uniquifyIDs(item, seen, child) + } + return out + } + return v +} + +// widgetIDsAreUnique reports the first $ID that occurs more than once in an encoded +// unit — the check that must pass before anything is written to disk. +func widgetIDsAreUnique(doc bson.D) (string, bool) { + seen := map[string]bool{} + var dup string + var walk func(any) + walk = func(v any) { + if dup != "" { + return + } + switch t := v.(type) { + case bson.D: + if id, ok := idOf(t); ok { + if seen[id] { + dup = id + return + } + seen[id] = true + } + for _, e := range t { + walk(e.Value) + } + case bson.A: + for _, item := range t { + walk(item) + } + } + } + walk(doc) + return dup, dup == "" +} diff --git a/mdl/executor/widget_convert_test.go b/mdl/executor/widget_convert_test.go index 3541d6985..345687b7c 100644 --- a/mdl/executor/widget_convert_test.go +++ b/mdl/executor/widget_convert_test.go @@ -145,3 +145,112 @@ func TestWidgetRoundTripPreservesTypePointerBinding(t *testing.T) { t.Errorf("pairing broken: $ID %s != TypePointer %s", gotID, gotPtr) } } + +// AugmentTemplate adds a property to an object-list property (DataGrid2 columns) by +// giving every list entry a copy of the same constructed node, so a placeholder that +// appears once per entry must become a DIFFERENT id per entry. Remapping by value gave +// them all one id, which Mendix accepts on load and on `mx check` and then rejects at +// SAVE time with "Duplicate Guid in unit page template" — after `mx update-widgets` has +// already collapsed mprcontents/, leaving the project flattened and unloadable. +// Reported against PR #89; 18 units and 432 excess occurrences on the reference fixture. +func TestEnsureUniqueWidgetIDsSeparatesListEntries(t *testing.T) { + // Three list entries that each carry a copy of the same node, all pointing at the + // one shared PropertyType — which is correct and must NOT be rewritten. + const sharedPT = "aaaaaaaaaaaa4aaaaaaaaaaaaaaaaaaa" + entry := func() map[string]any { + return map[string]any{ + "$ID": "bbbbbbbbbbbb4bbbbbbbbbbbbbbbbbbb", + "$Type": "CustomWidgets$WidgetProperty", + "TypePointer": sharedPT, + "Value": map[string]any{ + "$ID": "cccccccccccc4ccccccccccccccccccc", + "$Type": "CustomWidgets$WidgetValue", + }, + } + } + obj := map[string]any{ + "$ID": "dddddddddddd4ddddddddddddddddddd", + "$Type": "CustomWidgets$WidgetObject", + "Objects": []any{ + float64(2), + map[string]any{"$ID": "e1", "Properties": []any{float64(2), entry()}}, + map[string]any{"$ID": "e2", "Properties": []any{float64(2), entry()}}, + map[string]any{"$ID": "e3", "Properties": []any{float64(2), entry()}}, + }, + } + + out := ensureUniqueWidgetIDs(obj, map[string]bool{}) + + ids := map[string]int{} + pointers := map[string]int{} + var walk func(any) + walk = func(v any) { + switch n := v.(type) { + case map[string]any: + if id, ok := n["$ID"].(string); ok { + ids[id]++ + } + if p, ok := n["TypePointer"].(string); ok { + pointers[p]++ + } + for _, val := range n { + walk(val) + } + case []any: + for _, item := range n { + walk(item) + } + } + } + walk(out) + + for id, n := range ids { + if n > 1 { + t.Errorf("$ID %q appears %d times — Mendix refuses to save a duplicate GUID", id, n) + } + } + // 1 object + 3 entries + 3 properties + 3 values = 10 distinct ids. + if len(ids) != 10 { + t.Errorf("got %d distinct $IDs, want 10", len(ids)) + } + // The shared schema pointer is legitimately repeated and must survive untouched. + if pointers[sharedPT] != 3 { + t.Errorf("TypePointer to the shared PropertyType survived %d of 3 times (%v) — "+ + "rewriting it would unbind each column from its schema", pointers[sharedPT], pointers) + } +} + +// The test above exercises ensureUniqueWidgetIDs directly, so it passes even if the +// call is removed from applyToWidget — it proves the function works, not that it is +// wired in. widgetIDsAreUnique is the backstop that catches the wiring: it runs on the +// encoded unit immediately before the write, and apply refuses rather than persisting +// a document Mendix would accept now and reject at save time. Verified by deleting the +// ensureUniqueWidgetIDs call and re-running the fixture, which then aborts with +// "refusing to write" instead of corrupting the project. +func TestWidgetIDsAreUniqueDetectsDuplicates(t *testing.T) { + dup := primitive.Binary{Subtype: 0x00, Data: bytes.Repeat([]byte{0x7C}, 16)} + other := primitive.Binary{Subtype: 0x00, Data: bytes.Repeat([]byte{0x2E}, 16)} + + clean := bson.D{ + {Key: "$ID", Value: dup}, + {Key: "Widgets", Value: bson.A{bson.D{{Key: "$ID", Value: other}}}}, + } + if _, ok := widgetIDsAreUnique(clean); !ok { + t.Error("reported a duplicate in a document that has none") + } + + dirty := bson.D{ + {Key: "$ID", Value: dup}, + {Key: "Widgets", Value: bson.A{ + bson.D{{Key: "$ID", Value: other}}, + bson.D{{Key: "$ID", Value: dup}}, // same id as the root + }}, + } + got, ok := widgetIDsAreUnique(dirty) + if ok { + t.Fatal("missed a duplicate GUID — this is the check that stops the project being corrupted") + } + if got == "" { + t.Error("did not name the offending id") + } +} diff --git a/mdl/executor/widget_sync_apply.go b/mdl/executor/widget_sync_apply.go index 9ea140dbb..df4154aef 100644 --- a/mdl/executor/widget_sync_apply.go +++ b/mdl/executor/widget_sync_apply.go @@ -123,7 +123,24 @@ func ApplyWidgetSync(b backend.RawUnitBackend, projectPath string, opts SyncOpti if changed == 0 { continue } - encoded, err := bson.Marshal(out) + + // Never write a unit carrying a duplicate GUID. Mendix accepts one on load and + // on `mx check`, then refuses to SAVE the project — and `mx update-widgets` + // collapses mprcontents/ before it discovers that, leaving the project both + // flattened and unloadable. A corruption that only surfaces later, after the + // safety net has been destroyed, has to be caught before it reaches disk. + outDoc, isDoc := out.(bson.D) + if !isDoc { + return nil, plan, fmt.Errorf("unit %s: reconciliation did not produce a document", unitID) + } + if dup, ok := widgetIDsAreUnique(outDoc); !ok { + return nil, plan, fmt.Errorf( + "unit %s (%s): reconciliation would write duplicate GUID %s — refusing to write, "+ + "no changes have been made to this unit; please report this project shape", + unitID, bsonString(doc, "Name"), dup) + } + + encoded, err := bson.Marshal(outDoc) if err != nil { return nil, plan, fmt.Errorf("encode unit %s: %w", unitID, err) } @@ -190,6 +207,14 @@ func applyToWidget(widget bson.D, def *mpk.WidgetDefinition) (bson.D, bool) { newType := rewriteWidgetIDs(tmpl.Type, remap) newObj := rewriteWidgetIDs(tmpl.Object, remap) + // Remapping by VALUE is not sufficient: AugmentTemplate gives every entry of an + // object-list property a copy of the same constructed node, so one placeholder + // becomes N nodes that would all receive the same UUID. See ensureUniqueWidgetIDs + // — this is the duplicate-Guid corruption reported against PR #89. + seen := map[string]bool{} + newType = ensureUniqueWidgetIDs(newType, seen) + newObj = ensureUniqueWidgetIDs(newObj, seen) + widget = setField(widget, "Type", mapToWidgetDoc(newType)) widget = setField(widget, "Object", mapToWidgetDoc(newObj)) return widget, true diff --git a/mdl/types/json_occurrence_test.go b/mdl/types/json_occurrence_test.go new file mode 100644 index 000000000..9d00f021f --- /dev/null +++ b/mdl/types/json_occurrence_test.go @@ -0,0 +1,151 @@ +package types + +import ( + "testing" +) + +// walkElements visits every element in the tree, depth-first. +func walkElements(elems []*JsonElement, visit func(*JsonElement)) { + for _, e := range elems { + if e == nil { + continue + } + visit(e) + walkElements(e.Children, visit) + } +} + +// TestBuildJsonElementsFromSnippet_NoZeroMaxOccurs guards issue #841. +// +// Every element was written with MinOccurs=0, MaxOccurs=0. Mendix reads that +// literally as "never occurs", so an import mapping bound to the structure +// silently produced zero objects — the REST call succeeded and no data arrived. +// Nothing flagged it: no validation error, and mx check passed. +// +// MaxOccurs=0 is never a legitimate value. This is the invariant that matters +// most, independent of the exact bound chosen for any one element. +func TestBuildJsonElementsFromSnippet_NoZeroMaxOccurs(t *testing.T) { + snippets := map[string]string{ + "object root": `{"pagination":{"page":1},"items":[{"id":"a","name":"b"}]}`, + "array root": `[{"id":"a"}]`, + "primitive array": `{"tags":["a","b"]}`, + "scalars": `{"s":"x","n":1,"b":true,"nil":null}`, + } + for name, snippet := range snippets { + t.Run(name, func(t *testing.T) { + elems, err := BuildJsonElementsFromSnippet(snippet, nil) + if err != nil { + t.Fatalf("BuildJsonElementsFromSnippet: %v", err) + } + if len(elems) == 0 { + t.Fatal("no elements produced") + } + walkElements(elems, func(e *JsonElement) { + if e.MaxOccurs == 0 { + t.Errorf("element %q (%s) has MaxOccurs=0 — Mendix reads this as \"never occurs\"", e.ExposedName, e.ElementType) + } + }) + }) + } +} + +// TestBuildJsonElementsFromSnippet_RootOccursOnce pins the root to 0..1. +// +// MaxOccurs=1 is the fix: the root previously carried MaxOccurs=0, which Mendix +// reads as "never occurs". +// +// MinOccurs deliberately stays 0. Mendix cross-validates a mapping element +// against its schema element, and the mapping serializers hardcode MinOccurs=0 +// (only MaxOccurs is propagated). Writing 1 here makes every mapping bound to +// the structure fail with CE5015 "Attribute 'MinOccurs' does not match schema +// element '(Object)'" — caught by the integration suite, not by unit tests, +// because it needs a mapping bound to the structure and a real mx check. +func TestBuildJsonElementsFromSnippet_RootOccursOnce(t *testing.T) { + for name, snippet := range map[string]string{ + "object root": `{"a":1}`, + "array root": `[{"a":1}]`, + } { + t.Run(name, func(t *testing.T) { + elems, err := BuildJsonElementsFromSnippet(snippet, nil) + if err != nil { + t.Fatalf("BuildJsonElementsFromSnippet: %v", err) + } + root := elems[0] + if root.MaxOccurs != 1 { + t.Errorf("root MaxOccurs = %d, want 1 (0 means \"never occurs\")", root.MaxOccurs) + } + if root.MinOccurs != 0 { + t.Errorf("root MinOccurs = %d, want 0 — the mapping serializers hardcode 0, "+ + "and a mismatch fails every bound mapping with CE5015", root.MinOccurs) + } + }) + } +} + +// TestBuildJsonElementsFromSnippet_RepeatingElementsUnbounded pins the elements +// that actually repeat to unbounded (-1). +// +// A nested object array's item already got this right; the root-array item and +// the primitive-array wrapper did not, which is what made the bug look +// intermittent — one element in the tree came out correct. +func TestBuildJsonElementsFromSnippet_RepeatingElementsUnbounded(t *testing.T) { + t.Run("nested object array item", func(t *testing.T) { + elems, err := BuildJsonElementsFromSnippet(`{"items":[{"id":"a"}]}`, nil) + if err != nil { + t.Fatalf("BuildJsonElementsFromSnippet: %v", err) + } + item := findByName(elems, "ItemsItem") + if item == nil { + t.Fatal("ItemsItem not found") + } + if item.MaxOccurs != -1 { + t.Errorf("array item MaxOccurs = %d, want -1 (unbounded)", item.MaxOccurs) + } + }) + + // A root array follows the same rule as a nested one: the array element + // occurs once, its item child repeats. Before the fix the root-array item + // was 0..0 while the nested-array item was already 0..-1 — the same + // construct written two different ways in two builders. + t.Run("root array item", func(t *testing.T) { + elems, err := BuildJsonElementsFromSnippet(`[{"id":"a"}]`, nil) + if err != nil { + t.Fatalf("BuildJsonElementsFromSnippet: %v", err) + } + root := elems[0] + if root.ElementType != "Array" { + t.Fatalf("root ElementType = %q, want Array", root.ElementType) + } + item := findByName(elems, "JsonObject") + if item == nil { + t.Fatal("JsonObject item not found") + } + if item.MaxOccurs != -1 { + t.Errorf("root array item MaxOccurs = %d, want -1 (unbounded)", item.MaxOccurs) + } + }) + + t.Run("primitive array wrapper", func(t *testing.T) { + elems, err := BuildJsonElementsFromSnippet(`{"tags":["a","b"]}`, nil) + if err != nil { + t.Fatalf("BuildJsonElementsFromSnippet: %v", err) + } + wrapper := findByName(elems, "Tag") + if wrapper == nil { + t.Fatal("Tag wrapper not found") + } + if wrapper.MaxOccurs != -1 { + t.Errorf("primitive array wrapper MaxOccurs = %d, want -1 (unbounded)", wrapper.MaxOccurs) + } + }) +} + +func findByName(elems []*JsonElement, name string) *JsonElement { + var found *JsonElement + walkElements(elems, func(e *JsonElement) { + if found == nil && e.ExposedName == name { + found = e + } + }) + return found +} diff --git a/mdl/types/json_utils.go b/mdl/types/json_utils.go index 2bcdff961..0dac28d61 100644 --- a/mdl/types/json_utils.go +++ b/mdl/types/json_utils.go @@ -12,6 +12,31 @@ import ( "unicode" ) +// occursUnbounded is the MaxOccurs value Mendix reads as "unbounded" (`0..*`). +// Note that 0 is NOT a stand-in for "unspecified": Mendix reads MaxOccurs=0 +// literally as "never occurs", so an element written that way is silently +// skipped by every import mapping bound to the structure (#841). +const occursUnbounded = -1 + +// rootMinOccurs is the MinOccurs written for the root element. +// +// It stays 0 even though a document root arguably "always occurs once", because +// Mendix cross-validates a *mapping* element against its *schema* element and +// the mapping serializers hardcode `MinOccurs: 0` (serExportMappingElement / +// serImportMappingElement in both engines) — only MaxOccurs is propagated from +// the schema. Writing 1 here makes the two sides disagree and every mapping +// bound to the structure fails to build: +// +// [error] [CE5015] "The mapping does not align with the underlying schema +// anymore. Details: Attribute 'MinOccurs' does not match schema element +// '(Object)'." at Object mapping element 'Root' +// +// MinOccurs=0 ("optional") is harmless — the #841 defect was MaxOccurs=0, which +// Mendix reads as "never occurs". Making the root genuinely 1..1 would mean +// propagating MinOccurs through both mapping serializers as well; that is a +// larger change and wants a Studio Pro reference to confirm the target shape. +const rootMinOccurs = 0 + // iso8601Pattern matches common ISO 8601 datetime strings that Mendix Studio Pro // recognizes as DateTime primitive types in JSON structures. var iso8601Pattern = regexp.MustCompile( @@ -85,15 +110,17 @@ func BuildJsonElementsFromSnippet(snippet string, customNameMap map[string]strin switch tok { case json.Delim('{'): root := b.buildElementFromRawObject("Root", "(Object)", snippet, tracker) - root.MinOccurs = 0 - root.MaxOccurs = 0 + root.MinOccurs = rootMinOccurs + root.MaxOccurs = 1 root.Nillable = true return []*JsonElement{root}, nil case json.Delim('['): root := b.buildElementFromRawRootArray("Root", "(Array)", snippet, tracker) - root.MinOccurs = 0 - root.MaxOccurs = 0 + // The array element itself occurs once; its item child carries the + // repetition, mirroring the nested-array case. + root.MinOccurs = rootMinOccurs + root.MaxOccurs = 1 root.Nillable = true return []*JsonElement{root}, nil @@ -165,7 +192,7 @@ func (b *snippetBuilder) buildElementFromRawObject(exposedName, path, rawJSON st ElementType: "Object", PrimitiveType: "Unknown", MinOccurs: 0, - MaxOccurs: 0, + MaxOccurs: 1, Nillable: true, MaxLength: -1, FractionDigits: -1, @@ -256,7 +283,7 @@ func (b *snippetBuilder) buildElementFromRawRootArray(exposedName, path, rawJSON ElementType: "Array", PrimitiveType: "Unknown", MinOccurs: 0, - MaxOccurs: 0, + MaxOccurs: 1, Nillable: true, MaxLength: -1, FractionDigits: -1, @@ -279,13 +306,13 @@ func (b *snippetBuilder) buildElementFromRawRootArray(exposedName, path, rawJSON if len(trimmed) > 0 && trimmed[0] == '{' { itemElem := b.buildElementFromRawObject("JsonObject", itemPath, trimmed, tracker) itemElem.MinOccurs = 0 - itemElem.MaxOccurs = 0 + itemElem.MaxOccurs = occursUnbounded itemElem.Nillable = true arrayElem.Children = append(arrayElem.Children, itemElem) } else { child := b.buildElementFromRawValue("JsonObject", itemPath, "", firstItem, tracker) child.MinOccurs = 0 - child.MaxOccurs = 0 + child.MaxOccurs = occursUnbounded arrayElem.Children = append(arrayElem.Children, child) } } @@ -302,7 +329,7 @@ func (b *snippetBuilder) buildElementFromRawArray(exposedName, path, jsonKey, ra ElementType: "Array", PrimitiveType: "Unknown", MinOccurs: 0, - MaxOccurs: 0, + MaxOccurs: 1, Nillable: true, MaxLength: -1, FractionDigits: -1, @@ -328,7 +355,7 @@ func (b *snippetBuilder) buildElementFromRawArray(exposedName, path, jsonKey, ra itemPath := path + "|(Object)" itemElem := b.buildElementFromRawObject(itemName, itemPath, trimmed, tracker) itemElem.MinOccurs = 0 - itemElem.MaxOccurs = -1 + itemElem.MaxOccurs = occursUnbounded itemElem.Nillable = true arrayElem.Children = append(arrayElem.Children, itemElem) } else { @@ -342,7 +369,7 @@ func (b *snippetBuilder) buildElementFromRawArray(exposedName, path, jsonKey, ra ElementType: "Wrapper", PrimitiveType: "Unknown", MinOccurs: 0, - MaxOccurs: 0, + MaxOccurs: occursUnbounded, Nillable: true, MaxLength: -1, FractionDigits: -1, @@ -350,7 +377,7 @@ func (b *snippetBuilder) buildElementFromRawArray(exposedName, path, jsonKey, ra } valueElem := b.buildElementFromRawValue("Value", wrapperPath+"|", jsonKey, firstItem, tracker) valueElem.MinOccurs = 0 - valueElem.MaxOccurs = 0 + valueElem.MaxOccurs = 1 wrapper.Children = append(wrapper.Children, valueElem) arrayElem.Children = append(arrayElem.Children, wrapper) } @@ -380,7 +407,7 @@ func buildValueElement(exposedName, path, primitiveType, originalValue string) * ElementType: "Value", PrimitiveType: primitiveType, MinOccurs: 0, - MaxOccurs: 0, + MaxOccurs: 1, Nillable: true, MaxLength: maxLength, FractionDigits: -1, diff --git a/model/types.go b/model/types.go index 3b0fc206b..4b7a89fa7 100644 --- a/model/types.go +++ b/model/types.go @@ -1022,7 +1022,10 @@ type ExportMappingElement struct { Entity string `json:"entity,omitempty"` // qualified entity name Association string `json:"association,omitempty"` // qualified association name ObjectHandling string `json:"objectHandling,omitempty"` // "Parameter" for root, "Find" for children - MaxOccurs int `json:"maxOccurs,omitempty"` // 1 for Object, -1 for Array; 0 = default (1) + // 1 for Object, -1 for Array (unbounded). NOT "0 = default": Mendix reads + // MaxOccurs=0 literally as "never occurs", and cross-validates this against + // the bound JSON structure element (CE5015). Mirror the schema (#841). + MaxOccurs int `json:"maxOccurs,omitempty"` // Value mapping fields Attribute string `json:"attribute,omitempty"` // qualified attribute name (Module.Entity.Attr) DataType string `json:"dataType,omitempty"` // "String", "Integer", "Boolean", etc. diff --git a/modelsdk/mpr/serialize_mappings.go b/modelsdk/mpr/serialize_mappings.go index b515d1591..6956a1cf3 100644 --- a/modelsdk/mpr/serialize_mappings.go +++ b/modelsdk/mpr/serialize_mappings.go @@ -256,7 +256,10 @@ func serExportValueElement(id string, elem *model.ExportMappingElement, parentPa {Key: "XmlPath", Value: ""}, {Key: "Type", Value: dataType}, {Key: "MinOccurs", Value: int32(0)}, - {Key: "MaxOccurs", Value: int32(0)}, + // Mirror the bound schema element: Mendix cross-validates the two and + // reports CE5015 on any mismatch. Hardcoding 0 only worked while the + // JSON structure also wrote 0 for every element (#841). + {Key: "MaxOccurs", Value: int32(elem.MaxOccurs)}, {Key: "Nillable", Value: true}, {Key: "IsDefaultType", Value: false}, {Key: "ElementType", Value: "Value"}, diff --git a/sdk/mpr/writer_export_mapping.go b/sdk/mpr/writer_export_mapping.go index 46c80baea..4d2a779ee 100644 --- a/sdk/mpr/writer_export_mapping.go +++ b/sdk/mpr/writer_export_mapping.go @@ -165,7 +165,10 @@ func serializeExportValueElement(id string, elem *model.ExportMappingElement, pa {Key: "XmlPath", Value: ""}, {Key: "Type", Value: dataType}, {Key: "MinOccurs", Value: int32(0)}, - {Key: "MaxOccurs", Value: int32(0)}, + // Mirror the bound schema element: Mendix cross-validates the two and + // reports CE5015 on any mismatch. Hardcoding 0 only worked while the + // JSON structure also wrote 0 for every element (#841). + {Key: "MaxOccurs", Value: int32(elem.MaxOccurs)}, {Key: "Nillable", Value: true}, {Key: "IsDefaultType", Value: false}, {Key: "ElementType", Value: "Value"},