diff --git a/.claude/commands/mxcli-dev/review.md b/.claude/commands/mxcli-dev/review.md index 3ef98554b9..76fa4079d5 100644 --- a/.claude/commands/mxcli-dev/review.md +++ b/.claude/commands/mxcli-dev/review.md @@ -52,6 +52,7 @@ proactively. Add a row after every review that surfaces something new. | 21 | A registry that reads only one source (an `embed.FS`) has no extension point, so the only way to customise its output is to hand-edit the generated block — which a digest fence then refuses to touch on the next run. The feature ends up hostile to the exact case it was built for | API design | When a guard refuses a user's edit, ask whether the thing being edited should have been authorable. Introduce a source abstraction (`fs.FS` + root) and let a project-local directory shadow the embedded set — the walk functions usually already take a root, so the change is contained | | 22 | A test helper that expands a template cross-products every list against every reference (e.g. every `@each $weight` list against every `url()`), inventing artefacts no asset ever shipped and failing on correct input | Test coverage | Scan positionally: a reference must be expanded against the loop it actually sits under. Prove the helper both fires on a real break (delete one shipped file) and stays quiet on correct input — a helper only checked against green code has not been shown to detect anything | | 23 | A copy-to-scaffold path renames files but not the identifiers built from the name (`@mixin mxcli--`, `@import "mxcli-"`), so two artefacts collide the moment both exist — and the symptom is a rule that silently compiles to nothing | Code correctness | Assert the structural contract on the *generated* artefact, not just the shipped ones: factor the built-in's contract test into a helper and run the scaffold through it. Verify once end to end against the real toolchain and record it | +| 24 | A new splice refusal routed through the shared `cannotSplice` path changes **mdl 0** too: there a refusal falls back to the whole-flow rebuild, so a headerless script that used to splice now loses its merges, curves and `$ID`s — a silent change of meaning ADR-0011 rules out (#1508 case 6, caught by `TestFlowModify_InsertIsSpliced`) | Language versioning | Gate the new refusal on `flowRebuildRefused.Applies(ctx.LanguageVersion)` so mdl 0 keeps its old splice; add an mdl 0 case asserting "spliced, no MDL-V1-REBUILD" beside the mdl 1 refusal test | | 24 | A stacked PR's base merged at an older head: commits pushed to the base branch after its merge never reach `main`, while both PRs read as fine (ako/mxcli#1108 lost its last two commits this way) | Merge hygiene | Before merging any stacked PR, and after pushing to a branch whose PR may already be merged, check each commit with `git merge-base --is-ancestor origin/main`; retarget the next PR to `main` so it carries the stranded commits, and say so in its description | | 24 | A writer leaves out a property Studio Pro always writes (an empty `Documentation`, the empty half of an attribute/association pair) or writes one Mendix does not declare. `mx check`, the build and the runtime stay green, but Mendix's merge/diff engine compares property NAMES and throws, so the document becomes unmergeable once Studio Pro saves it (mendixlabs/mxcli#1373) | BSON writing | The doctype gate's `checkPropertySets` catches it per script: `mx convert -p` re-serializes what is not canonical. Fix the writer, or record Mendix's value with `MXCLI_PROPERTY_SETS_RECORD` + `internal/propertysets/cmd/propdefaults`; never guess a default | | 24 | A skill or command still instructs work into a package the repo has deleted (`sdk/mpr` test locations, a symptom table moved to `findings/*.jsonl` years prior) — the doc reads as authoritative and every instruction in it is a compile error or a no-op | Docs quality | When a package is deleted or a doc is restructured, grep `.claude/` for its name in the same PR. A deletion that leaves the guidance behind is worse than no guidance, because the reader trusts it | diff --git a/.claude/skills/fix-issue/findings/mdl-backend/2026-10-21-splice-shift-leaves-placed-fragment-behind.json b/.claude/skills/fix-issue/findings/mdl-backend/2026-10-21-splice-shift-leaves-placed-fragment-behind.json new file mode 100644 index 0000000000..b3a9cdf987 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-backend/2026-10-21-splice-shift-leaves-placed-fragment-behind.json @@ -0,0 +1 @@ +{"area": "mdl/backend", "date": "2026-10-21", "symptom": "create or modify of a nanoflow drawn with @position on every statement, adding an if/else after the declare without @position and editing the two calls in the next if's branch: `spliced: 1 inserted, 1 replaced, 1 dropped`, with `if $Flag` -299 -> 40, its merge 186 -> 525 and the return 530 -> 869, while the replaced calls stayed at (-124, 100)/(60, 100), left of their own decision. Re-running the same script was refused (`the connectors of the If at (40, 200) are redrawn`). With a plain if (no else) the new if took over the stored decision and the restated `if $Flag` was drawn on top of it at (-299, 200), both merges at (186, 200).", "cause": "Three rules that each held alone. makeRoom shifts every top-level node past the cut by the deficit (339); a Replace whose first statement states @position is Placed and written at its stated coordinates, never shifted; movedNode records a move only when the stated position differs from the stored one, so the shifted nodes were never moved back, and the merge is not a move candidate at all. On re-run @merge is geometry but not a position field, so the pair fell to `redrawn`. No-else: lcsStatements weighed ifConditionChanged the same as sameIfShell and the tie went to the earlier pair, so the new if became a condition edit of the stored decision and the restated if a Placed insert, which never runs checkRoom.", "file": "mdl/backend/mfmutator/splice.go (Pin, pinnedPast, makeRoom, Replace), mdl/backend/microflow_mutation.go (MicroflowMutator.Pin), mdl/executor/cmd_flow_modify_patch.go (statedPoints, patch), mdl/executor/cmd_flow_modify.go (lcsStatements weights, mdl 1 gate in planFlowModify)", "insight": "Shift-consistently cannot satisfy idempotence: once the script states -299 and the store says 40, the re-run must move it back or refuse — measured even for a plain one-activity insert into a described flow (+104, then the re-run refused on @merge). So a node at a stated point is pinned and a shift that would move one is refused, naming the room and the node; the mutator pins by point rather than $ID because the node at a stated point may be one a later Placed fragment puts there. Collect stated points from @position, @merge, @start, note and parameter positions, skipping loop bodies (relative coordinates). The control is the same script with the positions after the insert left out: spliced, everything moved by one delta, re-run Unchanged. Gate the pins on mdl 1 (flowRebuildRefused.Applies): under mdl 0 the refusal falls back to the whole-flow rebuild, which deletes the Studio Pro merges and $IDs the shift kept — TestFlowModify_InsertIsSpliced (ADR-0012's describe+insert acceptance, headerless) caught exactly that, and ADR-0011 forbids the change of meaning. Not touched: the shift still moves parameters (y 53) along with the row.", "refs": ["ako/mxcli#1508", "mendixlabs/mxcli#1598"]} diff --git a/.claude/skills/fix-issue/findings/mdl-backend/2026-10-21-splice-shift-moves-parameters.json b/.claude/skills/fix-issue/findings/mdl-backend/2026-10-21-splice-shift-moves-parameters.json new file mode 100644 index 0000000000..c7daa32352 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-backend/2026-10-21-splice-shift-moves-parameters.json @@ -0,0 +1 @@ +{"area": "mdl/backend", "date": "2026-10-21", "symptom": "A splice that makes room for an inserted fragment (create or modify / alter … insert) moved the flow's parameters along with the row: one if/else inserted into NF_Do moved $Flag from its derived (200, 53) to (539, 53), a longer replace then to (699, 53), and from then on describe printed a parameter @position no script stated. Inserting before the first statement of a two-parameter microflow moved $Count from (300, 53) to (410, 53).", "cause": "mfmutator shift (splice.go) moves every top-level node past the cut, and a Microflows$MicroflowParameter is a top-level object of the unit's object collection (the decoded model keeps parameters apart, so nothing in the executor sees them as nodes). mxcli's derived parameter row is absolute (200 + idx*pitch, 53), right of the start event, so almost every insert's cut falls left of it.", "file": "mdl/backend/mfmutator/splice.go (shifted)", "insight": "Measure the authored convention before deciding what 'the row' is: 63 of 71 Studio Pro-authored parameters (PedApp 19/20, JTSBootLogboek 44/51) sit exactly at the start event's x, all above every other node, none within 60px of the flow line — a parameter belongs to the flow, not to the stretch of line past an insert, and Studio Pro moves nothing on insert. The decoded model hides this: microflows.Microflow carries Parameters apart from ObjectCollection, so a probe that type-switches the collection for *MicroflowParameter does not compile, while the BSON the mutator walks has them inline. Excluding them in shifted also keeps them out of pinnedPast, so a stated parameter position can no longer block a splice it is not in the way of.", "refs": ["ako/mxcli#1547", "ako/mxcli#1508"]} diff --git a/.claude/skills/fix-issue/findings/mdl-backend/2026-10-22-object-list-item-address.json b/.claude/skills/fix-issue/findings/mdl-backend/2026-10-22-object-list-item-address.json new file mode 100644 index 0000000000..20f300dc3f --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-backend/2026-10-22-object-list-item-address.json @@ -0,0 +1 @@ +{"area": "mdl/backend/pagemutator", "date": "2026-10-22", "symptom": "`insert into grp1 { … }` for an Accordion group: widget \"grp1\" not found — the group stores no Name; `insert into acc1 item@2 { … }` was refused as \"not a navigation list\"; an empty group could not be filled at all", "cause": "object-list items (CustomWidgets$WidgetObject) had no address; resolveNavItem accepted only navigation lists", "file": "`mdl/backend/pagemutator/objectlist_items.go` (resolveObjectListItem, item slots, ObjectListItemValues), `column_hidden_reset.go` (ResetHiddenItemProperties, template nulling/creation), `mdl/executor/alter_hidden_item_slot.go`; grammar `alterTarget` itemSlot", "insight": "The default `docker check` normalises widgets on a temp copy and HID every defect here — run `--no-update-widgets` (the project as stored, what MxBuild and run --local see) or a hidden-slot write and a mode-switching SET both look clean while each is CE0463. Diffing a SET-switched group against a CREATE-made one (TypePointer -> PropertyKey in the JSON dump) found the cause in one step: headerText template filled vs null. Two plausible wrong turns: assuming the schema DefaultValue holds the visible default (it is \"\" — 'Header' lives in the widget package, so a revealed text with no value is refused, not invented), and a check-time hidden-slot refusal that blocked `set (headerRenderMode: 'custom'); insert into …headercontent` — the very fix its own message recommended — because check reads the stored document; it stands down when the statement sets the item first, and exec decides on live values. The item result carries the item schema like a DataGrid2 column, so SET and `.slot` reused existing paths rather than growing new ones", "refs": ["mendixlabs/mxcli#1453", "ako/mxcli#1326"], "ce": ["CE0463", "CE4899"]} diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-10-22-dropdownfilter-default-value-not-a-string-ce0117.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-22-dropdownfilter-default-value-not-a-string-ce0117.json new file mode 100644 index 0000000000..d3cc0c0265 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-22-dropdownfilter-default-value-not-a-string-ce0117.json @@ -0,0 +1 @@ +{"area":"mdl/executor","date":"2026-10-22","symptom":"`column (Attribute: IsActive) { dropdownfilter ddf (defaultValue: 'true') }` passed check and exec, then mx check failed CE0117 \"Error(s) in expression.\" at the drop-down filter (mendixlabs/mxcli#1611 B).","cause":"defaultValue is a pluggable Expression property whose template declares ReturnType String, and a quoted MDL value on an Expression property is the expression's text, so 'true' stored the Boolean expression `true`. No check typed an authored expression against the property's declared return type; the embedded schema read each property's ValueType.Type but not its ReturnType.","fix":"MDL-WIDGET49 (validate_widget_expression_return.go), beside MDL-WIDGET42 in validateWidgetProperties: for a String-returning Expression property, refuse a value that infers as a literal Boolean, Integer or Decimal (exprcheck.InferSourceKind) or is one bare name, with the String-literal spelling as the hint (`defaultValue: '''true'''`). embeddedWidgetSchema.returns records ReturnType.Type per Expression key. Runs with and without a project (the DROPDOWNFILTER definition is built in).","file":"mdl/executor/validate_widget_expression_return.go","insight":"The writer was right and the check was missing: the MDL convention (a quoted value IS the expression text) is what describe prints and what every other expression property relies on, so 'wrap the string for a String-returning property' would have silently changed the meaning of every existing script. Measure the column type separately from the property's return type: '1' failed in an Integer column too — the return type is the widget's, fixed at String, not the attribute's. Judge only what InferSourceKind is sure of: it calls [%CurrentDateTime%] a String, so DateTime is not trusted; Unknown (paths, variables, untyped functions) is never refused.","refs":["mendixlabs/mxcli#1611","ako/mxcli#1539"],"ce":["CE0117"],"rules":["MDL-WIDGET49"]} diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-10-22-view-entity-association-followed-from-target-ce8812.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-22-view-entity-association-followed-from-target-ce8812.json new file mode 100644 index 0000000000..76b0c3b320 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-22-view-entity-association-followed-from-target-ce8812.json @@ -0,0 +1 @@ +{"area":"mdl/executor","date":"2026-10-22","symptom":"A page data source following a view entity's association from the entity it points at — `listview (DataSource: DATABASE FROM $currentObject/M.Stats_Owner/M.Stats)` on a page over M.Owner, where M.Stats selects `o.ID as Stats_Owner` — passed check --references and exec, then mx check failed CE8812 \"Entity 'M.Owner' has no association 'M.Stats_Owner'\" (mendixlabs/mxcli#1611 A). Separately, its remedy `DATABASE M.Stats WHERE [M.Stats_Owner = $currentObject]` (and the microflow `retrieve … where M.Stats_Owner = $x`) was refused by check as naming 'neither an attribute nor an association' when the view entity came from the same script.","cause":"Nothing modelled that a view entity's OQL-derived association (OqlViewAssociationSource) is one-way for a page; MDL-ASSOCDS01's association index had no view flag. And scriptContext recorded only CREATE ASSOCIATION statements, so a script's view associations (its `.ID as ` columns) were invisible to every name-resolving checker.","fix":"MDL-ASSOCDS02 (validate_view_assoc_reverse.go), hooked into attributeScopeValidator.walk beside MDL-ASSOCDS01: follow the association or `database from` path hop by hop from the widget's context entity and report the first hop that follows a view association from its TO end, with the constrained query as the fix. assocShape.view from Association/CrossAssociation Source == OqlViewAssociationSource; scriptContext.recordViewAssociations registers each view association column through recordAssociation (ends, name index, shape) and marks it view.","file":"mdl/executor/validate_view_assoc_reverse.go","insight":"Measure the remedy you are about to recommend in the same shape the user works in: the constrained query built at 0 errors against a stored view entity, but the check refused it when the view was created in the same script — the reporter's flow — so the new hint would have sent users into a second false refusal. Measured on 11.14.0, one page per shape with uniquely named widgets so mx check's 'at ' attributes each CE8812: refused in list view, data grid, gallery and data view, database and in-memory form, at any hop; accepted from the view side, constrained, and in a microflow association retrieve (so the rule is page data sources only). An entity named Owner is itself CE7247 — rename repro entities before reading the result.","refs":["mendixlabs/mxcli#1611","ako/mxcli#1539"],"ce":["CE8812"],"rules":["MDL-ASSOCDS02"]} diff --git a/.claude/skills/mendix/alter-page/SKILL.md b/.claude/skills/mendix/alter-page/SKILL.md index 3538b76230..250b5f3915 100644 --- a/.claude/skills/mendix/alter-page/SKILL.md +++ b/.claude/skills/mendix/alter-page/SKILL.md @@ -17,10 +17,10 @@ description: "Modify an existing page or snippet's widget tree in place with ALT | 482-560 | Examples | | 561-651 | DataGrid 2 columns: how to address them, and what you can set | | 652-663 | Common Mistakes | -| 664-692 | Limitations — prefer binding at page creation (ledger finding #45) | -| 693-700 | Validation Checklist | -| 701-710 | Related Commands | -| 711-715 | Related Skills | +| 664-696 | Limitations — prefer binding at page creation (ledger finding #45) | +| 697-704 | Validation Checklist | +| 705-714 | Related Commands | +| 715-719 | Related Skills | # ALTER PAGE / ALTER SNIPPET - Modify Existing Pages and Snippets @@ -678,6 +678,10 @@ page and bind the buttons at creation time instead of rewiring afterwards: view: `replace dvMain.footer with { footer { … } }`, `insert into dvMain.footer { … }`, `drop dvMain.footer`. +**An Accordion group has no name either** (`group g1` keeps no `g1`): use + `acc1 item@2`, `acc1 item('One')` or `acc1 item(btnIn)`; `.headercontent` is its + header slot, refused until `set (headerRenderMode: 'custom') on acc1 item@1`. + **A pluggable widget's slot has no name either.** A Data grid 2's control bar (`controlbar { … }` in describe) is a property of the grid, so address it by the grid: `insert into dataGrid1.controlbar { actionbutton … }`, `replace diff --git a/.claude/skills/mendix/create-page/reference/widgets.md b/.claude/skills/mendix/create-page/reference/widgets.md index 7d24ce3668..2a043f26a7 100644 --- a/.claude/skills/mendix/create-page/reference/widgets.md +++ b/.claude/skills/mendix/create-page/reference/widgets.md @@ -410,6 +410,8 @@ column (caption: 'Actions') { > **A list widget needs an association that yields a list.** A list view, data grid or gallery over `$currentObject/M.Assoc` is CE8812 ("A grid association path must result in a list") when the association is a Reference followed from its FROM entity, or a Reference with owner Both from either end (one-to-one). Use a data view for the single object, or a ReferenceSet. `check` reports it as MDL-ASSOCDS01. The reverse of a default Reference (from the TO entity) is a list and fine. +> **A view entity's association is one-way for a page.** The association a view entity derives from its OQL (`select c.ID as Stats_Customer`) can be followed from the view entity only. A data source that follows it from the entity it points at, in any widget and at any hop (`database from $currentObject/M.Stats_Customer/M.Stats`, `$currentObject/M.Stats_Customer`), is CE8812 "Entity 'M.Customer' has no association 'M.Stats_Customer'". Query the view entity instead: `datasource: database M.Stats where [M.Stats_Customer = $currentObject]`. `check` reports it as MDL-ASSOCDS02. A microflow `retrieve … from $Cust/M.Stats_Customer` is accepted. + **With WHERE and SORT BY (inline in DataSource):** ```sql datagrid dgActive ( @@ -710,6 +712,18 @@ custom attribute (Studio Pro's "Attribute config: Custom"); listing two is refus attribute of the Data grid 2 column it sits in ("Auto"), so in a **gallery's** `filter { … }` bar, which has no column, the attribute must be named. +`defaultValue:` preselects an option. It is an **expression returning a String**, and a +quoted MDL value is the expression's text, so the option goes in as a String literal +inside the quotes, even for a Boolean column: + +```sql +column (Attribute: IsActive) { dropdownfilter ddfActive (defaultValue: '''true''') } +``` + +`defaultValue: 'true'` stores the Boolean expression `true`, and `'1'` or a bare name +like `'Open'` fail the same way: mxbuild reports CE0117 "Error(s) in expression." `check` +reports it as MDL-WIDGET49. `'empty'` (no preselection) is fine. + Filter by an **association** instead of an attribute — the options are the associated objects. Giving the filter a `datasource:` (the OPTION list) selects this mode; all three parts are required: diff --git a/.claude/skills/mendix/write-microflows/SKILL.md b/.claude/skills/mendix/write-microflows/SKILL.md index af58d4d850..4cf24d5edd 100644 --- a/.claude/skills/mendix/write-microflows/SKILL.md +++ b/.claude/skills/mendix/write-microflows/SKILL.md @@ -69,7 +69,7 @@ Choose the mode by who owns the microflow ([choose-edit-mode](../choose-edit-mod (targets from `describe microflow X with handles`). `create or modify` of `describe` output patches too: unchanged writes nothing; a statement (even one whose handler returns), guard clause, `return` value, `if` condition, header clause, parameter (added/retyped; removed only if unused) or stated `@position`/`@start` change is - patched in place (a move keeps the node's flows), as is a loop-body activity with new values. A redrawn + patched in place (a move keeps the node's flows), as is a loop-body activity with new values ([an insert needing room past a stated `@position`](reference/pitfalls.md#13-inserting-where-the-script-states-every-position)). A redrawn `@anchor`/`@curve`, other loop-body change, error handler or other `return` added/taken away rebuilds under mdl 0 (`MDL-V1-REBUILD`: IDs renumbered, merges and curves lost) and is refused under `mdl 1;`. **To add, remove or reorder in a loop body, `alter … replace` the whole loop** ([pitfalls](reference/pitfalls.md#11-changing-something-inside-a-loop-body)). **To rebuild deliberately, `drop` and `create` in ONE script** — the grants carry only within it ([pitfalls](reference/pitfalls.md#drop--create-is-still-a-new-document)). diff --git a/.claude/skills/mendix/write-microflows/reference/pitfalls.md b/.claude/skills/mendix/write-microflows/reference/pitfalls.md index b1df1bbddc..1848349607 100644 --- a/.claude/skills/mendix/write-microflows/reference/pitfalls.md +++ b/.claude/skills/mendix/write-microflows/reference/pitfalls.md @@ -376,6 +376,22 @@ alter microflow MyModule.GetCode { }; ``` +### 13. Inserting Where the Script States Every Position + +`describe` output states `@position` on every statement. Under `mdl 1;`, `create or +modify` never moves a node the script draws at a stated position: a statement inserted +**without** `@position` into a gap too small for it is refused, naming the room it needs +and the node in the way (mendixlabs/mxcli#1598). Moving the rest of the row along would +leave the stored flow saying something other than the script, so the next run would move +it back. Either: + +- give the new statements an `@position` where there is room for them, or +- leave the positions of what comes after them unstated (`@position`, `@merge`, a note's + `position:`), so the splice can move it along — the second run of that script is then + Unchanged. + +A headerless (mdl 0) script still moves the row along, as it always did. + ## Implicit Variable Creation (CE0111 Duplicate Variable) These statements **implicitly create a new variable** with the name on the left side: diff --git a/.claude/skills/mendix/write-nanoflows/SKILL.md b/.claude/skills/mendix/write-nanoflows/SKILL.md index 3aadc22379..6740be6af0 100644 --- a/.claude/skills/mendix/write-nanoflows/SKILL.md +++ b/.claude/skills/mendix/write-nanoflows/SKILL.md @@ -11,21 +11,21 @@ description: "Nanoflow syntax in MDL — shared with microflows, but client-side | lines | section | |---|---| | 35-44 | When to Use This Skill | -| 45-78 | Changing an Existing Nanoflow | -| 79-97 | When to Use a Nanoflow vs a Microflow | -| 98-112 | Key Differences from Microflows | -| 113-136 | Nanoflow Structure | -| 137-145 | Naming Convention | -| 146-214 | Supported Activities | -| 215-481 | Real-World Patterns | -| 482-552 | NanoflowCommons JavaScript Actions Reference | -| 553-591 | Disallowed Activities | -| 592-595 | Return Type Restrictions | -| 596-640 | Error Handling in Nanoflows | -| 641-648 | Security (GRANT/REVOKE) | -| 649-661 | Management Commands | -| 662-672 | Common Mistakes | -| 673-687 | Validation Checklist | +| 45-81 | Changing an Existing Nanoflow | +| 82-100 | When to Use a Nanoflow vs a Microflow | +| 101-115 | Key Differences from Microflows | +| 116-139 | Nanoflow Structure | +| 140-148 | Naming Convention | +| 149-217 | Supported Activities | +| 218-484 | Real-World Patterns | +| 485-555 | NanoflowCommons JavaScript Actions Reference | +| 556-594 | Disallowed Activities | +| 595-598 | Return Type Restrictions | +| 599-643 | Error Handling in Nanoflows | +| 644-651 | Security (GRANT/REVOKE) | +| 652-664 | Management Commands | +| 665-675 | Common Mistakes | +| 676-690 | Validation Checklist | # Mendix Nanoflow Skill @@ -58,7 +58,10 @@ Choose the mode by who owns the nanoflow ([choose-edit-mode](../choose-edit-mode the stored end event or decision, and a body without a trailing `return` means the stored end. The header (documentation, return type, parameters added, retyped or — when unused — removed) is set on the stored document, and a stated `@position` or `@start` moves the - stored node, keeping its flows. A change it cannot patch — anything inside a loop body + stored node, keeping its flows. Under `mdl 1;` a node at a stated position is never moved + to make room: a statement inserted without `@position` where the gap is too small is refused, naming + the node in the way — give the new statements an `@position`, or leave `@position` and + `@merge` off the statements after them. A change it cannot patch — anything inside a loop body or error handler, a redrawn `@anchor`/`@curve`, any other `return` added or taken away — rebuilds the whole nanoflow under mdl 0 (warning `MDL-V1-REBUILD`: element IDs renumbered, merges removed, curves reset) and diff --git a/cmd/mxcli/syntax/features_domain_model.go b/cmd/mxcli/syntax/features_domain_model.go index 65fe54fdc6..03f4e6dd92 100644 --- a/cmd/mxcli/syntax/features_domain_model.go +++ b/cmd/mxcli/syntax/features_domain_model.go @@ -254,6 +254,11 @@ func init() { "- THE ALIAS IS A MODULE-LEVEL NAME. It cannot collide with an entity or\n" + " enumeration in the same module, case-insensitively — Mendix: \"Duplicate\n" + " name … Entities, associations and enumerations cannot share names.\"\n\n" + + "On a page the association is ONE-WAY: a data source can follow it from the\n" + + "view entity only. From the entity it points at (`database from\n" + + "$currentObject/M.Totals_Meter/M.Totals`) mxbuild reports CE8812 \"Entity\n" + + "'M.Meter' has no association 'M.Totals_Meter'\" (MDL-ASSOCDS02). Query the\n" + + "view entity instead: `database M.Totals where [M.Totals_Meter = $currentObject]`.\n\n" + "The alternative is often better: CAST the id to a string and keep it as a\n" + "plain attribute. That is one SQL statement instead of two and materialises\n" + "no objects in the client, and the id is still there to look the real object\n" + diff --git a/docs-site/src/language/versions.md b/docs-site/src/language/versions.md index 0fa153f21a..01e6dd4a62 100644 --- a/docs-site/src/language/versions.md +++ b/docs-site/src/language/versions.md @@ -196,7 +196,7 @@ A construct with no mechanical rewrite is reported with the reason, and `fmt` re `-p app.mpr` also settles a bare `commit $X;` in a `create or modify microflow|nanoflow`, with or without the header. Since mendixlabs/mxcli#895 a bare commit means *with* events, Studio Pro's default; an older mxcli stored the same statement *without* events. Where the stored flow commits the variable without events, `fmt --upgrade -p app.mpr` writes `commit $X without events;`, so re-running the script keeps what is stored instead of turning the event handlers on (or, inside a loop under `mdl 1`, being refused). A new flow, a new commit, or a stored commit with events is left as written; where the stored flow commits the variable both ways, the statement is left and reported. Without `-p`, `fmt` prints a note (`MDL067`) for each flow with a bare commit. -`-p app.mpr` also tells the upgrade which statements `exec` would refuse once the header is there. Under `mdl 1` a `create or modify microflow|nanoflow` of a stored flow is applied as a patch, and a change the patch cannot make (inside a loop body anything but an activity replaced by one of the same kind, an error handler, a redrawn connector, a `return` added) is refused with nothing written; without the header the same statement rebuilds the whole flow (`MDL-V1-REBUILD`). No rewrite keeps that meaning, so `fmt` names each such statement and leaves the header off that file, applying the rest of the upgrade. Change those flows with `alter`, or drop and create them, and upgrade again; `--force-header` adds the header regardless. `mxcli check script.mdl -p app.mpr` reports the same statements (an `MDL-V1-REBUILD` error under `mdl 1`, the warning without the header), computed by the same code as `exec` and `diff`. +`-p app.mpr` also tells the upgrade which statements `exec` would refuse once the header is there. Under `mdl 1` a `create or modify microflow|nanoflow` of a stored flow is applied as a patch, and a change the patch cannot make (inside a loop body anything but an activity replaced by one of the same kind, an error handler, a redrawn connector, a `return` added, a statement inserted without `@position` that needs room past a node the script places) is refused with nothing written; without the header the same statement rebuilds the whole flow (`MDL-V1-REBUILD`). No rewrite keeps that meaning, so `fmt` names each such statement and leaves the header off that file, applying the rest of the upgrade. Change those flows with `alter`, or drop and create them, and upgrade again; `--force-header` adds the header regardless. `mxcli check script.mdl -p app.mpr` reports the same statements (an `MDL-V1-REBUILD` error under `mdl 1`, the warning without the header), computed by the same code as `exec` and `diff`. ## Behaviour changes to know about diff --git a/docs-site/src/reference/page/alter-page.md b/docs-site/src/reference/page/alter-page.md index ca8dae3c00..39caf9304c 100644 --- a/docs-site/src/reference/page/alter-page.md +++ b/docs-site/src/reference/page/alter-page.md @@ -159,6 +159,46 @@ alter snippet Shop.Menu { Only items go next to an item or in its place: `INSERT BEFORE` / `AFTER` an item and `REPLACE` of one take `item` blocks, and widgets go `INTO` an item. +### Pluggable Widget Item Operations + +A pluggable widget's object-list item — an Accordion group, a Pop-up menu +item — stores no name either (a name written on `group g1` is not kept), and is +addressed the same way, on the widget that owns the list: + +| Address | Means | +|---------|-------| +| `acc1 item@2` | the second group — `INSERT INTO` fills its body | +| `acc1 item('One')` | the group whose text (`headerText`) is `One` | +| `acc1 item(btnIn)` | the group holding the widget `btnIn` | +| `acc1 item@1.headercontent` | another slot of the group, by its keyword | + +The body is the slot `describe` prints as the item's children (`content`, or +the item's first widget slot); any other slot is named with `.slot`, spelled as +`describe` prints its block. + +```sql +mdl 1; +alter page Shop.Faq { + insert into acc1 item@2 { dynamictext txtAnswer (Content: 'Yes.') }; + set (headerRenderMode: 'custom') on acc1 item@1; + insert into acc1 item@1.headercontent { dynamictext txtHead (Content: 'Shipping') }; + replace acc1 item('Returns').content with { dynamictext txtRet (Content: '30 days') }; + drop acc1 item@3; +}; +``` + +- A slot the widget's configuration hides is refused, naming the property that + shows it: an Accordion hides a group's `headercontent` while + `headerRenderMode` is `'text'` (the default). Widgets written there are not + rendered and fail the build with CE0463. Set the property first — in the same + statement is fine, as above. +- A SET that switches a group's mode keeps the texts it hides or shows as + CREATE stores them. Switching back to `'text'` needs the text in the same + statement — `set (headerRenderMode: 'text', headerText: 'Shipping') on acc1 item@1` + — since the stored schema has no default to fill in. +- `INSERT BEFORE` / `AFTER` an item (adding a new group) and `REPLACE` of a + whole item are not supported; use `drop` and `create or modify page`. + ### DROP Removes one or more widgets by name. The widget and all its children are removed from the tree. diff --git a/docs-wiki/bug-patterns/flow-graph-geometry.md b/docs-wiki/bug-patterns/flow-graph-geometry.md index 5303e9f98c..3a1d12a62b 100644 --- a/docs-wiki/bug-patterns/flow-graph-geometry.md +++ b/docs-wiki/bug-patterns/flow-graph-geometry.md @@ -94,6 +94,15 @@ translated into the gap; anything that is not on the line (a note) must not size the gap, and anything the script placed absolutely must not be translated. +**Making room is a move the script did not ask for.** Shifting the row after +a gap is consistent only while nothing past the gap has a stated position: +once the script says -299 and the store says 40, the next run must move it back +or refuse, so "shift everything consistently" can never re-run Unchanged. A +node at a stated point is pinned instead, and a shift that would move one is +refused with the room it needs — under mdl 1 only, because under mdl 0 a +refusal becomes a whole-flow rebuild, which loses more than the shift did. +Check a splice fix by re-running the same script, not by describing once. + ## See also - [fix-issue findings](../../.claude/skills/fix-issue/findings/) — the formulas, diff --git a/docs/11-proposals/PROPOSAL_alter_object_list_item_address.md b/docs/11-proposals/PROPOSAL_alter_object_list_item_address.md index 716008b437..96f3475174 100644 --- a/docs/11-proposals/PROPOSAL_alter_object_list_item_address.md +++ b/docs/11-proposals/PROPOSAL_alter_object_list_item_address.md @@ -1,12 +1,12 @@ --- title: ALTER PAGE Addressing of Pluggable Object-List Items -status: proposed +status: partial date: 2026-10-17 --- # Proposal: ALTER PAGE Addressing of Pluggable Object-List Items -**Status:** Proposed +**Status:** Partial **Date:** 2026-10-17 Refs ako/mxcli#1326, mendixlabs/mxcli#1453. @@ -120,6 +120,19 @@ Shared with the existing forms, so nothing new to learn: **item** inserts an item (`insert after acc1 item@1 { group (…) { … } }`), mirroring navigation-list items. +## Implementation Status + +Done: steps 1–3, 5–7 (describe slots #1565; shared body rule; grammar; the +resolver for every selector form; INSERT INTO an item's body or slot, +REPLACE / DROP of a slot, DROP of an item, SET of an item property; the +hidden-slot refusal, Open Question 2 answered as "refuse"). Measuring the SET +path found that a mode switch must also keep the texts it hides or shows as +CREATE stores them, and does. + +Not done: inserting a NEW item (`insert after acc1 item@1 { group … }`) is +refused; step 4's selector rename was not needed; legacy table cells stay +deferred. + ## Implementation Plan Order matters: describe has to stop misplacing header widgets before an address diff --git a/docs/11-proposals/README.md b/docs/11-proposals/README.md index eb01524dd5..5bcacf7d94 100644 --- a/docs/11-proposals/README.md +++ b/docs/11-proposals/README.md @@ -30,11 +30,12 @@ for display in this README): ## Active Proposals (118) -### In Progress (partial) (15) +### In Progress (partial) (16) | Proposal | Status | Summary | |----------|--------|---------| | [Add Styling Support to MDL Pages](page-styling-support.md) | Partial | Mendix has four styling mechanisms on every widget, all stored in a BSON Forms$Appearance object: | +| [ALTER PAGE Addressing of Pluggable Object-List Items](PROPOSAL_alter_object_list_item_address.md) | Partial | Refs ako/mxcli#1326, mendixlabs/mxcli#1453. | | [Atlas Design System — a skill for visually appealing Mendix apps](PROPOSAL_atlas_design_system.md) | Partial | mxcli can generate a fully-functional Mendix app, but the default output looks bland — it leans | | [Default styling — generated apps that look designed on first boot](PROPOSAL_default_styling.md) | Partial | mxcli new produces a blank Atlas app. | | [Eval Framework for mxcli + Claude Code](proposal-eval-framework.md) | Partial | An evaluation framework that systematically tests how well Claude Code + mxcli handles real-world Mendix app generation tasks. | @@ -59,11 +60,10 @@ for display in this README): | [MDL Language Critique and Beta Syntax Freeze](PROPOSAL_mdl_beta_syntax_freeze.md) | Accepted | Related: ADR-0003; resulting decisions ADR-0010, ADR-0011, ADR-0012; design-mdl-syntax skill, PROPOSAL_workflow_microflow_syntax_alignment.m | | [Size containers from their contents, not from a statement count](PROPOSAL_container_autosize.md) | Accepted | Closes the last open item of upstream #884 — | -### Proposed (41) +### Proposed (40) | Proposal | Status | Summary | |----------|--------|---------| -| [ALTER PAGE Addressing of Pluggable Object-List Items](PROPOSAL_alter_object_list_item_address.md) | Proposed | Refs ako/mxcli#1326, mendixlabs/mxcli#1453. | | [Bootstrap hooks that fetch the mxcli the project was built with](PROPOSAL_bootstrap_source.md) | Proposed | mxcli init writes .claude/bootstrap-mxcli.sh with a hard-coded download of | | [Built-in file database for run --local (HSQLDB)](PROPOSAL_run_local_builtin_database.md) | Proposed | Depends on: the Windows process-helper fix (PR #1147) — this builds on that branch. | | [Comprehensive MDL Syntax and Grammar Improvements (v2)](PROPOSAL_mdl_syntax_improvements_v2.md) | Proposed | This document consolidates and expands upon previous proposals, offering a unified set of syntax and grammar improvements for all major Mend | diff --git a/mdl-examples/bug-tests/1453-alter-accordion-group-item-address.mdl b/mdl-examples/bug-tests/1453-alter-accordion-group-item-address.mdl new file mode 100644 index 0000000000..c13ba803ba --- /dev/null +++ b/mdl-examples/bug-tests/1453-alter-accordion-group-item-address.mdl @@ -0,0 +1,39 @@ +mdl 1; +-- mendixlabs/mxcli#1453 / ako/mxcli#1326: an Accordion group stores no Name, +-- so `insert into grp1 { … }` never resolved and an empty group could not be +-- filled. Groups are now addressed on their widget, like navigation list +-- items: `acc1 item@n`, `acc1 item('')`, `acc1 item()`, +-- with `.headercontent` for the header slot +-- (PROPOSAL_alter_object_list_item_address.md). +-- +-- Expected (describe below): the first group's body holds txtOne and +-- txtOne2, its header slot txtHead; the second group holds tbName; and +-- `mxcli docker check --no-update-widgets` reports 0 errors. +-- +-- The header slot is hidden while headerRenderMode is 'text', so the SET +-- comes first; inserting into it without that SET is refused. + +create module Bug1453b; + +create persistent entity Bug1453b.Item (Name: string(50)); + +create page Bug1453b.P (title: 'P', layout: Atlas_Core.Atlas_Default, params: ( $Item: Bug1453b.Item )) { + dataview dv (DataSource: $Item) { + accordion acc1 (advancedMode: false, collapsible: true, expandBehavior: 'singleExpanded', animate: true, showIcon: 'right', animateIcon: true) { + group (headerRenderMode: 'text', headerText: 'One') { + dynamictext txtOne (Content: 'one') + } + group (headerRenderMode: 'custom') { + } + } + } +}; + +alter page Bug1453b.P { + insert into acc1 item@2 { textbox tbName (Label: 'Name', Attribute: Name) }; + insert into acc1 item('One') { dynamictext txtOne2 (Content: 'one more') }; + set (headerRenderMode: 'custom') on acc1 item@1; + insert into acc1 item@1.headercontent { dynamictext txtHead (Content: 'Header') } +}; + +describe page Bug1453b.P; diff --git a/mdl-examples/bug-tests/1611-dropdownfilter-default-value.mdl b/mdl-examples/bug-tests/1611-dropdownfilter-default-value.mdl new file mode 100644 index 0000000000..2689cf288c --- /dev/null +++ b/mdl-examples/bug-tests/1611-dropdownfilter-default-value.mdl @@ -0,0 +1,40 @@ +mdl 1; +-- ============================================================================ +-- mendixlabs/mxcli#1611 B — a drop-down filter's defaultValue of the wrong kind +-- (CE0117) +-- ============================================================================ +-- +-- Reported (v0.27.0, Mendix 11.12.5): +-- column (Attribute: IsActive) { dropdownfilter ddf (defaultValue: 'true') } +-- passed check and exec, then mx check failed CE0117 "Error(s) in expression." +-- +-- defaultValue is an Expression returning a String, and a quoted MDL value on +-- an expression property IS the expression's text: 'true' stores the Boolean +-- expression `true`. Measured on 11.14.0, one filter per value: 'true', +-- 'false', '1' (in a String and in an Integer column), '1.5' and 'abc' are +-- CE0117; '''true''', '''abc''', '''tru''', 'empty' and '''a'' + ''b''' build. +-- check now reports the refused values as MDL-WIDGET49, with or without a +-- project. +-- +-- This file is the shape that must PASS: the option as a String literal inside +-- the quotes. As written: 0 errors from check and from mxbuild. With the first +-- filter's defaultValue changed to 'true': MDL-WIDGET49 from check, CE0117 from +-- mxbuild. +-- ============================================================================ + +CREATE MODULE Bug1611Ddf; + +CREATE OR MODIFY PERSISTENT ENTITY Bug1611Ddf.Task ( + Title: String(100), + IsActive: Boolean DEFAULT true +); + +CREATE OR MODIFY PAGE Bug1611Ddf.Task_Overview ( + Title: 'Tasks', + Layout: Atlas_Core.Atlas_Default +) { + datagrid dgTasks (DataSource: DATABASE Bug1611Ddf.Task) { + column (Attribute: IsActive) { dropdownfilter ddfActive (defaultValue: '''true''') } + column (Attribute: Title) { textfilter tfTitle } + } +}; diff --git a/mdl-examples/bug-tests/1611-view-association-reverse.mdl b/mdl-examples/bug-tests/1611-view-association-reverse.mdl new file mode 100644 index 0000000000..ed2e8b6c87 --- /dev/null +++ b/mdl-examples/bug-tests/1611-view-association-reverse.mdl @@ -0,0 +1,72 @@ +mdl 1; +-- ============================================================================ +-- mendixlabs/mxcli#1611 A — a page following a view entity's association from +-- the entity it points at (CE8812) +-- ============================================================================ +-- +-- Reported (v0.27.0, Mendix 11.12.5): a view entity M.Stats selects `e.ID as +-- Stats_Owner`; on a page over an Owner, +-- listview lv (DataSource: DATABASE FROM $currentObject/M.Stats_Owner/M.Stats) +-- passed `check --references` and exec, then mx check failed CE8812 "Entity +-- 'M.Owner' has no association 'M.Stats_Owner'". +-- +-- A view entity's association is one-way for a page. Measured on 11.14.0, one +-- page per shape: following it from the entity it points at is CE8812 in a list +-- view, data grid, gallery and data view, in the database form and the +-- in-memory form, and as a second hop. Following it from the view entity, the +-- constrained query below, and a microflow association retrieve all built at +-- 0 errors. `check` now reports the refused shapes as MDL-ASSOCDS02. +-- +-- This file is the shape that must PASS, with the view entity created in the +-- same script. The constrained query used to be refused by check in that case, +-- because the script's view associations were known to no checker. This file: +-- 0 errors from check and from mxbuild. With the list view's data source +-- changed to `DATABASE FROM $currentObject/Bug1611.Totals_Customer/Bug1611.Totals`: +-- MDL-ASSOCDS02 from check, CE8812 from mxbuild. +-- ============================================================================ + +CREATE MODULE Bug1611; + +CREATE OR MODIFY PERSISTENT ENTITY Bug1611.Customer ( Name: String(100) ); +CREATE OR MODIFY PERSISTENT ENTITY Bug1611.Purchase ( Amount: Integer ); +CREATE OR MODIFY ASSOCIATION Bug1611.Purchase_Customer FROM Bug1611.Purchase TO Bug1611.Customer; + +CREATE OR MODIFY VIEW ENTITY Bug1611.Totals ( Total: Integer ) AS ( + from Bug1611.Purchase as p + join p/Bug1611.Purchase_Customer/Bug1611.Customer as c + group by c.ID + select c.ID as Totals_Customer, sum(p.Amount) as Total +); + +-- From the customer: query the view entity, constrained on its association. +CREATE OR MODIFY PAGE Bug1611.Customer_Totals ( + Params: ($Customer: Bug1611.Customer), + Title: 'Totals', + Layout: Atlas_Core.Atlas_Default +) { + dataview dvCustomer (DataSource: $Customer) { + listview lvTotals (DataSource: DATABASE Bug1611.Totals WHERE [Bug1611.Totals_Customer = $currentObject]) { + dynamictext txtTotal (Content: 'total') + } + } +}; + +-- From the view entity: the association followed the way it points. +CREATE OR MODIFY PAGE Bug1611.Totals_Customer ( + Params: ($Totals: Bug1611.Totals), + Title: 'Customer', + Layout: Atlas_Core.Atlas_Default +) { + dataview dvTotals (DataSource: $Totals) { + dataview dvCustomer (DataSource: $currentObject/Bug1611.Totals_Customer) { + dynamictext txtName (Content: 'customer') + } + } +}; + +-- A microflow may retrieve over it from the customer. +CREATE OR MODIFY MICROFLOW Bug1611.ACT_CustomerTotals ($Customer: Bug1611.Customer) +BEGIN + RETRIEVE $Totals FROM $Customer/Bug1611.Totals_Customer; + RETRIEVE $Constrained FROM Bug1611.Totals WHERE Bug1611.Totals_Customer = $Customer; +END; diff --git a/mdl-examples/bug-tests/flow-geometry-1508-splice-keeps-stated-positions.mdl b/mdl-examples/bug-tests/flow-geometry-1508-splice-keeps-stated-positions.mdl new file mode 100644 index 0000000000..3d7471644d --- /dev/null +++ b/mdl-examples/bug-tests/flow-geometry-1508-splice-keeps-stated-positions.mdl @@ -0,0 +1,110 @@ +mdl 1; +-- ============================================================================ +-- Bug: an inserted if moved every stated node after it, but not the +-- statements replaced in its neighbour's branch +-- (ako/mxcli#1508, case 6; mendixlabs/mxcli#1598) +-- ============================================================================ +-- +-- NF_Do is drawn with a @position on every statement. The reported +-- restatement (below, commented out) keeps every @position, adds an if/else +-- after the declare without one, changes the JS argument and binds the call's +-- result. Before the fix it was spliced in as +-- +-- Modified nanoflow: MyFirstModule.NF_Do (spliced: 1 inserted, 1 replaced, 1 dropped) +-- +-- with `if $Flag` moved -299 -> 40, its merge 186 -> 525 and the return +-- 530 -> 869 to make room, while the two replaced calls stayed at the +-- positions the script states, (-124, 100) and (60, 100): left of their own +-- decision. Re-running the script was refused ("the connectors of the If at +-- (40, 200) are redrawn"). With a plain `if` (no else) the new if took over the +-- stored decision and the restated `if $Flag` was drawn on top of it. +-- +-- Fixed: under mdl 1 a node the script draws at a stated position is not +-- moved to make room. The reported restatement is refused, naming the room it +-- needs and what is in the way (a headerless script still makes room as +-- before: under mdl 0 the refusal would fall back to a whole-flow rebuild, +-- ADR-0011): +-- +-- the inserted statements need 339px more room than there is between the +-- ActionActivity at (-500, 200) and the ExclusiveSplit '$Flag' at +-- (-299, 200), which the script draws where it is; state @position on the +-- new statements, where there is room for them, or leave unstated the +-- positions of what comes after them (@position, @merge, a note's +-- position:) so that it can move along. +-- +-- The restatement that leaves those positions unstated (the last statement of +-- this file) is spliced: everything after the declare moves along by the same +-- amount, the replaced calls with their decision. +-- +-- Verify: exec, then +-- mxcli -p app.mpr -c "describe nanoflow MyFirstModule.NF_Do" +-- Expected: both calls are drawn right of `if $Flag` and left of its merge, +-- and the last statement, run again on its own, reports +-- Unchanged nanoflow: MyFirstModule.NF_Do +-- Uncomment the reported restatement to see the refusal. +-- ============================================================================ + +create javascript action MyFirstModule.JS_Do(Msg: String) returns Boolean platform web as $$ +return Promise.resolve(true); +$$; + +create or modify microflow MyFirstModule.SUB_Check() returns Boolean as $R +begin + declare $R Boolean = true; + return $R; +end; + +create or modify nanoflow MyFirstModule.NF_Do ($Flag: Boolean) returns Boolean as $Out +begin + @position(-500, 200) + declare $Out Boolean = false; + @position(-299, 200) + @merge(186, 200) + if $Flag then + @position(-124, 100) + call javascript action MyFirstModule.JS_Do(Msg = 'a'); + @position(60, 100) + call microflow MyFirstModule.SUB_Check(); + end if; + @position(530, 200) + return $Out; +end; + +-- The reported restatement: refused, nothing written. +-- create or modify nanoflow MyFirstModule.NF_Do ($Flag: Boolean) returns Boolean as $Out +-- begin +-- @position(-500, 200) +-- declare $Out Boolean = false; +-- if $Out then +-- log info node 'Probe' 'x'; +-- else +-- log info node 'Probe' 'y'; +-- end if; +-- @position(-299, 200) +-- @merge(186, 200) +-- if $Flag then +-- @position(-124, 100) +-- call javascript action MyFirstModule.JS_Do(Msg = 'b'); +-- @position(60, 100) +-- $Ok = call microflow MyFirstModule.SUB_Check(); +-- end if; +-- @position(530, 200) +-- return $Out; +-- end; + +-- The same change with the positions after the insert left unstated: spliced. +create or modify nanoflow MyFirstModule.NF_Do ($Flag: Boolean) returns Boolean as $Out +begin + @position(-500, 200) + declare $Out Boolean = false; + if $Out then + log info node 'Probe' 'x'; + else + log info node 'Probe' 'y'; + end if; + if $Flag then + call javascript action MyFirstModule.JS_Do(Msg = 'b'); + $Ok = call microflow MyFirstModule.SUB_Check(); + end if; + return $Out; +end; diff --git a/mdl-examples/bug-tests/flow-geometry-1547-splice-leaves-parameters.mdl b/mdl-examples/bug-tests/flow-geometry-1547-splice-leaves-parameters.mdl new file mode 100644 index 0000000000..5b6f8b2c9b --- /dev/null +++ b/mdl-examples/bug-tests/flow-geometry-1547-splice-leaves-parameters.mdl @@ -0,0 +1,35 @@ +mdl 1; +-- ============================================================================ +-- Bug: a splice making room moved the flow's parameters along with the row +-- (ako/mxcli#1547) +-- ============================================================================ +-- +-- The second statement inserts a log before the first one, so the splice +-- makes room by moving everything after the start event along. Before the fix +-- that included the two parameters, which mxcli draws on a row above the flow +-- (x 200 and 300, y 53): both moved right, and from then on describe printed a +-- @position on each that no script ever stated, the row drifting further with +-- every insert. +-- +-- Fixed: a parameter is not on the line the room is made on, so it stays +-- where it is. Studio Pro authors draw a parameter above the start event, and +-- inserting an activity in Studio Pro moves nothing. +-- +-- Verify: exec, then +-- mxcli -p app.mpr -c "describe microflow MyFirstModule.ACT_ParamRow" +-- Expected: the parameters carry no @position (they are still on the derived +-- row), and the new log is first. +-- ============================================================================ + +create or modify microflow MyFirstModule.ACT_ParamRow ($Name: String, $Count: Integer) +begin + log info node 'Probe' 'first ' + $Name; + log info node 'Probe' 'second ' + toString($Count); +end; + +create or modify microflow MyFirstModule.ACT_ParamRow ($Name: String, $Count: Integer) +begin + log info node 'Probe' 'inserted'; + log info node 'Probe' 'first ' + $Name; + log info node 'Probe' 'second ' + toString($Count); +end; diff --git a/mdl/backend/mfmutator/splice.go b/mdl/backend/mfmutator/splice.go index b00376e009..c0ce3e42a1 100644 --- a/mdl/backend/mfmutator/splice.go +++ b/mdl/backend/mfmutator/splice.go @@ -69,6 +69,8 @@ type Mutator struct { // removed holds the $IDs of every element an operation took out, so Save // can prove that nothing still points at one of them. removed map[string]bool + // pinned are the points the script states a node is drawn at (Pin). + pinned map[point]bool } var _ backend.MicroflowMutator = (*Mutator)(nil) @@ -341,7 +343,16 @@ func (m *Mutator) makeRoom(g *graph, fb *fragmentBox, f flowRef, x, y *node) (en b := y.pos.get(ax) - s*y.size.get(ax)/2 need := fb.length(ax) + 2*minGap if deficit := need - s*(b-a); deficit > 0 { - m.shift(g, ax, s, float64(a+b)/2, s*deficit, "") + cut := float64(a+b) / 2 + if p := m.pinnedPast(g, ax, s, cut, ""); p != nil { + blocked := ", and making it would move " + describeNode(p) + "," + if p == y { + blocked = "," + } + return 0, 0, fmt.Errorf("the inserted statements need %dpx more room than there is between %s and %s%s "+ + "which the script draws where it is; %s", deficit, describeNode(x), describeNode(y), blocked, roomAdvice) + } + m.shift(g, ax, s, cut, s*deficit, "") b += s * deficit } @@ -422,7 +433,12 @@ func (m *Mutator) Replace(target model.ID, frag *backend.MicroflowFragment) erro // past target moves along by the difference. near := x.pos.get(ax) - s*x.size.get(ax)/2 if extra := fb.length(ax) - x.size.get(ax); extra > 0 { - m.shift(g, ax, s, float64(x.pos.get(ax)), s*extra, x.id) + cut := float64(x.pos.get(ax)) + if p := m.pinnedPast(g, ax, s, cut, x.id); p != nil { + return fmt.Errorf("the replacing statements are %dpx longer than %s, and making room for them would move %s, "+ + "which the script draws where it is; %s", extra, describeNode(x), describeNode(p), roomAdvice) + } + m.shift(g, ax, s, cut, s*extra, x.id) } centre := point{} centre = centre.set(ax, near+s*fb.length(ax)/2) @@ -700,6 +716,17 @@ func (m *Mutator) Move(target model.ID, to model.Point) error { return nil } +// Pin records that the script states a node is drawn at at +// (mendixlabs/mxcli#1598, case 6). A point rather than a node: the node there +// may be one a later fragment places, at the position its statement states. +func (m *Mutator) Pin(at model.Point) error { + if m.pinned == nil { + m.pinned = map[point]bool{} + } + m.pinned[point{at.X, at.Y}] = true + return nil +} + // bodyFlows returns the flows that run inside loop's body, at any depth. They // are stored in the unit's Flows list, not in the loop, so taking the loop out // has to take them too; left behind they would point at removed objects. @@ -854,16 +881,14 @@ func (m *Mutator) markRemoved(v any) { } // shift moves every top-level node past the cut (along axis ax, in the -// direction of s) by delta. The sweep keeps everything on either side of the -// cut exactly as it was relative to its neighbours, so only the flows that -// cross the cut get longer; a flow's control vectors are relative to its ends, -// so no curve changes. skip is a node that is about to be removed. +// direction of s) by delta, but the parameters (shifted). The sweep keeps +// everything on either side of the cut exactly as it was relative to its +// neighbours, so only the flows that cross the cut get longer; a flow's +// control vectors are relative to its ends, so no curve changes. skip is a +// node that is about to be removed. func (m *Mutator) shift(g *graph, ax axis, s int, cut float64, delta int, skip string) { for _, n := range g.order { - if n.loop != "" || n.id == skip { - continue - } - if float64(s)*(float64(n.pos.get(ax))-cut) <= 0 { + if !shifted(n, ax, s, cut, skip) { continue } n.pos = n.pos.set(ax, n.pos.get(ax)+delta) @@ -871,6 +896,36 @@ func (m *Mutator) shift(g *graph, ax axis, s int, cut float64, delta int, skip s } } +// shifted reports whether shift moves n: a top-level node past the cut that +// is not a parameter. A parameter is not on the line the room is made on: it +// belongs to the flow as a whole, and Studio Pro authors draw it above the +// start event (63 of 71 in PedApp and JTSBootLogboek at the start's x, none +// within 60px of the line). Moved, it drifted right with every insert and +// describe printed a @position the script never stated (ako/mxcli#1547). +func shifted(n *node, ax axis, s int, cut float64, skip string) bool { + return n.loop == "" && n.id != skip && n.typ != "Microflows$MicroflowParameter" && + float64(s)*(float64(n.pos.get(ax))-cut) > 0 +} + +// pinnedPast returns the first node shift would move that is drawn at a point +// the script states (Pin), or nil. Moving it would make the stored flow say +// something other than the script — a later fragment placed at its own stated +// position would be left behind on the wrong side of a node that moved, and +// re-running the script would move the node back (mendixlabs/mxcli#1598, +// case 6) — so the caller refuses instead. +func (m *Mutator) pinnedPast(g *graph, ax axis, s int, cut float64, skip string) *node { + for _, n := range g.order { + if m.pinned[n.pos] && shifted(n, ax, s, cut, skip) { + return n + } + } + return nil +} + +// roomAdvice is the way out of a refused shift. +const roomAdvice = "state @position on the new statements, where there is room for them, or leave unstated the positions " + + "of what comes after them (@position, @merge, a note's position:) so that it can move along" + // checkIntegrity is the rule-1 guard: no binary anywhere in the unit may // still name an element that was removed, and no two elements may share an // $ID. Either would make the document unopenable, and neither is caught by diff --git a/mdl/backend/mfmutator/splice_parameters_test.go b/mdl/backend/mfmutator/splice_parameters_test.go new file mode 100644 index 0000000000..96c2b74ee4 --- /dev/null +++ b/mdl/backend/mfmutator/splice_parameters_test.go @@ -0,0 +1,50 @@ +// SPDX-License-Identifier: Apache-2.0 + +package mfmutator + +import ( + "testing" + + "github.com/mendixlabs/mxcli/model" +) + +// A parameter is not on the line a fragment is spliced into, so making room +// does not move it (ako/mxcli#1547). shift moved every top-level node past the +// cut, parameters included: one inserted if/else moved a nanoflow's `$Flag` +// from its derived (200, 53) to (539, 53), and from then on describe printed a +// @position the script never stated, the row drifting right with every insert. +// Studio Pro authors draw a parameter above the start event — 63 of 71 in +// PedApp and JTSBootLogboek sit at the start's x, none within 60px of the line +// — and an insert in Studio Pro moves nothing. The control is the activity +// after the gap, which does move. +func TestSplice_MakingRoomLeavesTheParameters(t *testing.T) { + positions := func(m *Mutator) map[string]point { + out := map[string]point{} + for _, n := range m.graph().order { + out[n.id] = n.pos + } + return out + } + objs := append(line(), obj("param", "Microflows$MicroflowParameter", 500, 53)) + for _, c := range []struct { + name string + splice func(m *Mutator) error + }{ + {"an insert", func(m *Mutator) error { return m.InsertAfter(model.ID(uid("a")), oneActivity()) }}, + {"a replace longer than what it replaces", func(m *Mutator) error { return m.Replace(model.ID(uid("a")), twoActivities()) }}, + } { + t.Run(c.name, func(t *testing.T) { + m, _ := newMutator(t, unit(objs, lineFlows())) + if err := c.splice(m); err != nil { + t.Fatal(err) + } + got := positions(m) + if p := got[uid("b")]; p.X <= 420 { + t.Fatalf("control: b is at x=%d, want it moved along past 420 to make room", p.X) + } + if p := got[uid("param")]; p != (point{500, 53}) { + t.Errorf("the parameter moved to (%d, %d), want it left at (500, 53)", p.X, p.Y) + } + }) + } +} diff --git a/mdl/backend/mfmutator/splice_pin_test.go b/mdl/backend/mfmutator/splice_pin_test.go new file mode 100644 index 0000000000..b8728bba8d --- /dev/null +++ b/mdl/backend/mfmutator/splice_pin_test.go @@ -0,0 +1,115 @@ +// SPDX-License-Identifier: Apache-2.0 + +package mfmutator + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/backend" + "github.com/mendixlabs/mxcli/mdl/types" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// A node whose position the script states is not moved to make room +// (mendixlabs/mxcli#1598, case 6). The splice moved every node past the gap +// along by what the fragment lacked, including ones the script draws at a +// stated @position — so the stored flow no longer said what the script said, +// a fragment placed at its own stated position was left behind on the wrong +// side of its decision, and re-running the same script moved the nodes back +// (or was refused). Such a shift is refused now, saying which node is in the +// way; the controls show the same splice still makes room when nothing past +// the gap is pinned. +func TestSplice_MakingRoomRefusesToMoveAPinnedNode(t *testing.T) { + positions := func(m *Mutator) map[string]point { + out := map[string]point{} + for _, n := range m.graph().order { + out[n.id] = n.pos + } + return out + } + // b at (420, 200) sits 50px right of a; a 120px activity needs 200. + t.Run("an insert, the node after the gap pinned", func(t *testing.T) { + m, _ := newMutator(t, unit(line(), lineFlows())) + if err := m.Pin(model.Point{X: 420, Y: 200}); err != nil { + t.Fatal(err) + } + err := m.InsertAfter(model.ID(uid("a")), oneActivity()) + if err == nil || !strings.Contains(err.Error(), "the ActionActivity at (420, 200)") || + !strings.Contains(err.Error(), "150px") { + t.Fatalf("got %v, want a refusal naming the pinned activity at (420, 200) and the 150px lacking", err) + } + if p := positions(m)[uid("b")]; p.X != 420 { + t.Errorf("b moved to x=%d before the refusal", p.X) + } + }) + t.Run("control: nothing pinned", func(t *testing.T) { + m, _ := newMutator(t, unit(line(), lineFlows())) + if err := m.InsertAfter(model.ID(uid("a")), oneActivity()); err != nil { + t.Fatal(err) + } + if p := positions(m)[uid("b")]; p.X != 570 { + t.Errorf("b is at x=%d, want it moved along by 150 to 570", p.X) + } + }) + t.Run("control: only nodes before the gap pinned", func(t *testing.T) { + m, _ := newMutator(t, unit(line(), lineFlows())) + for _, p := range []model.Point{{X: 100, Y: 200}, {X: 250, Y: 200}} { + if err := m.Pin(p); err != nil { + t.Fatal(err) + } + } + if err := m.InsertAfter(model.ID(uid("a")), oneActivity()); err != nil { + t.Fatalf("a pin before the gap refused the insert: %v", err) + } + }) + t.Run("a replace longer than what it replaces, the end pinned", func(t *testing.T) { + m, _ := newMutator(t, unit(line(), lineFlows())) + if err := m.Pin(model.Point{X: 600, Y: 200}); err != nil { + t.Fatal(err) + } + err := m.Replace(model.ID(uid("a")), twoActivities()) + if err == nil || !strings.Contains(err.Error(), "the EndEvent at (600, 200)") { + t.Fatalf("got %v, want a refusal naming the pinned end event", err) + } + }) + t.Run("an earlier fragment drawn at a stated point", func(t *testing.T) { + // The script states where the first fragment goes, so it is pinned + // like a stored node: a later insert cannot slide it along. + m, _ := newMutator(t, unit(line(), lineFlows())) + placed := oneActivity() + placed.Placed = true + placed.Objects[0].SetPosition(model.Point{X: 600, Y: 400}) + if err := m.Pin(model.Point{X: 600, Y: 400}); err != nil { + t.Fatal(err) + } + if err := m.InsertAfter(model.ID(uid("b")), placed); err != nil { + t.Fatal(err) + } + err := m.InsertAfter(model.ID(uid("a")), oneActivity()) + if err == nil || !strings.Contains(err.Error(), "(600, 400)") { + t.Fatalf("got %v, want a refusal naming the placed activity at (600, 400)", err) + } + }) +} + +// twoActivities is a two-activity fragment in builder coordinates, 290px long. +func twoActivities() *backend.MicroflowFragment { + act := func(x int) *microflows.ActionActivity { + return µflows.ActionActivity{BaseActivity: microflows.BaseActivity{BaseMicroflowObject: microflows.BaseMicroflowObject{ + BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())}, + Position: model.Point{X: x, Y: 200}, + Size: model.Size{Width: 120, Height: 60}, + }}} + } + a, b := act(360), act(530) + return &backend.MicroflowFragment{ + Objects: []microflows.MicroflowObject{a, b}, + Flows: []*microflows.SequenceFlow{{ + BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())}, + OriginID: a.ID, DestinationID: b.ID, + }}, + Entry: a.ID, Exit: b.ID, + } +} diff --git a/mdl/backend/microflow_mutation.go b/mdl/backend/microflow_mutation.go index 4217ff5d09..d381554916 100644 --- a/mdl/backend/microflow_mutation.go +++ b/mdl/backend/microflow_mutation.go @@ -66,6 +66,12 @@ type MicroflowMutator interface { // Move sets where the top-level node target is drawn, and nothing else: // its flows keep their ends, sides and curves (ako/mxcli#818). Move(target model.ID, to model.Point) error + // Pin says the script states that a node is drawn at the point at — a + // stored node, or one a fragment places there. An insert or replace that + // would move the node drawn there to make room is refused instead, so the + // stored flow keeps saying what the script says (mendixlabs/mxcli#1598, + // case 6). + Pin(at model.Point) error // SetHeader writes the document properties of declared — a // *microflows.Microflow or *microflows.Nanoflow built from the statement, // with every property it does not state carried from the stored one — onto diff --git a/mdl/backend/modelsdk/widget_pluggable_write.go b/mdl/backend/modelsdk/widget_pluggable_write.go index 8fad2a6b05..99a2710e37 100644 --- a/mdl/backend/modelsdk/widget_pluggable_write.go +++ b/mdl/backend/modelsdk/widget_pluggable_write.go @@ -240,6 +240,12 @@ func (codecChildSerializer) SerializeCustomWidgetDataSource(ds pages.DataSource) return genToV1BSON(el) } +// SerializeClientTemplate builds an empty Forms$ClientTemplate through the +// codec, for a pluggable text-template property ALTER has to fill. +func (codecChildSerializer) SerializeClientTemplate() bson.D { + return genToV1BSON(clientTemplateToGen(nil)) +} + // genToV1BSON encodes a gen element with the codec (bson v2 bytes) and decodes it // back into a v1 bson.D for the shared (v1) builder. func genToV1BSON(el element.Element) bson.D { diff --git a/mdl/backend/object_list_item_values.go b/mdl/backend/object_list_item_values.go new file mode 100644 index 0000000000..a067d82d53 --- /dev/null +++ b/mdl/backend/object_list_item_values.go @@ -0,0 +1,17 @@ +// SPDX-License-Identifier: Apache-2.0 + +package backend + +// ObjectListItemValues is what a page mutator reports about a stored +// object-list item addressed as `owner item…` (an Accordion group), so the +// executor can evaluate the widget's visibility rules against the values the +// item actually holds. Item and Widget map lower-cased property keys to their +// stored primitive or expression value. +type ObjectListItemValues struct { + WidgetID string + ListKey string + // BodySlot is the property key of the item's body slot (BodyItemSlotKey). + BodySlot string + Item map[string]string + Widget map[string]string +} diff --git a/mdl/backend/pagemutator/alter_target.go b/mdl/backend/pagemutator/alter_target.go index 1ffd1890ef..a72a8889df 100644 --- a/mdl/backend/pagemutator/alter_target.go +++ b/mdl/backend/pagemutator/alter_target.go @@ -45,7 +45,7 @@ func (m *Mutator) ResolveAlterTarget(t backend.AlterTarget) (backend.AlterTarget if result == nil { return backend.AlterTargetMatch{}, m.widgetNotFoundError(name) } - if widgetTypeName(result.widget) == navListItemType { + if widgetTypeName(result.widget) == navListItemType || result.item != nil { return backend.AlterTargetMatch{Kind: "item", Name: name}, nil } if bsonnav.DGetString(result.widget, "$Type") != objectListItemType && len(result.colPropKeys) == 0 { diff --git a/mdl/backend/pagemutator/column_hidden_reset.go b/mdl/backend/pagemutator/column_hidden_reset.go index a2f04da0fa..6a62984595 100644 --- a/mdl/backend/pagemutator/column_hidden_reset.go +++ b/mdl/backend/pagemutator/column_hidden_reset.go @@ -53,14 +53,38 @@ func (m *Mutator) ResetHiddenColumnProperties(gridRef, columnRef string, rules [ } } return resetHiddenItemProps(col.widget, col.colPropKeys, col.colPropKinds, defaults, - primitiveValuesByKey(grid.widget, buildPropKeyMap(grid.widget)), listKey, rules, explicit) + primitiveValuesByKey(grid.widget, buildPropKeyMap(grid.widget)), listKey, rules, explicit, nil) +} + +// ResetHiddenItemProperties is ResetHiddenColumnProperties for an object-list +// item addressed as `owner item…` (an Accordion group). It also nulls a +// TextTemplate the SET now hides, because that is how CREATE stores one +// (#1198): `set (headerRenderMode: 'custom') on acc1 item@1` hid headerText, +// left its template filled, and the project as stored failed with CE0463 at +// the Accordion (11.13.0). Column templates are left as they were — that case +// is not measured. +func (m *Mutator) ResetHiddenItemProperties(itemRef string, rules []types.WidgetVisibilityRule, setProps []string) ([]string, error) { + item := m.widgetFinder(m.rawData, itemRef) + if item == nil || item.item == nil { + return nil, fmt.Errorf("%q is not an object-list item", itemRef) + } + owner := item.item.owner + explicit := make(map[string]bool, len(setProps)) + for _, p := range setProps { + if key := resolveColumnPropertyKey(p, item.colPropKeys); key != "" { + explicit[key] = true + } + } + return resetHiddenItemProps(item.widget, item.colPropKeys, item.colPropKinds, + columnPropDefaults(owner, item.item.listKey), + primitiveValuesByKey(owner, buildPropKeyMap(owner)), item.item.listKey, rules, explicit, m.newTemplate) } // resetHiddenItemProps is the document-level half, separate so it can be tested // without a page. itemKeys/itemKinds map TypePointer ids to schema keys and // value kinds; defaults and widgetValues are keyed by schema key. func resetHiddenItemProps(item bson.D, itemKeys, itemKinds, defaults, widgetValues map[string]string, - listKey string, rules []types.WidgetVisibilityRule, explicit map[string]bool) ([]string, error) { + listKey string, rules []types.WidgetVisibilityRule, explicit map[string]bool, newTemplate func() bson.D) ([]string, error) { reset := map[string]bool{} // A reset can hide another property in turn (`minWidth` back to "auto" // hides `minWidthLimit`), so repeat until nothing changes. Each pass only @@ -80,10 +104,48 @@ func resetHiddenItemProps(item bson.D, itemKeys, itemKinds, defaults, widgetValu v, ok := src[c.PropertyKey] return v, ok }) + key := rule.PropertyKey + isTemplate := newTemplate != nil && keyKind(itemKeys, itemKinds, key) == "TextTemplate" + if isTemplate && determinable && !fires && !templateFilled(item, itemKeys, key) { + // Visible, stored null. With a declared default it gets a + // template holding that; without one there is no text to + // write — an empty required text is CE4899, measured on + // 11.13.0 ("Property 'Groups/2/Text' is required") — and the + // default CREATE writes ('Header') lives in the widget + // package, not in the stored schema. So the author is asked + // for it, in the statement that reveals it. + if defaults[key] == "" { + return nil, fmt.Errorf( + "property %q is shown under this item's new configuration but holds no text — set it in the "+ + "same statement, e.g. `set (…, %s: '…') on `", key, key) + } + if tmpl := newTemplate(); tmpl != nil { + if val := itemPropValue(item, itemKeys, key); val != nil { + updateClientTemplateText(tmpl, defaults[key]) + bsonnav.DSet(val, "TextTemplate", tmpl) + reset[key] = true + changed = true + } + } + continue + } if !determinable || !fires { continue } - key := rule.PropertyKey + if isTemplate { + if !templateFilled(item, itemKeys, key) { + continue + } + if explicit[key] { + return nil, fmt.Errorf( + "property %q is hidden under this item's configuration (%s), so it cannot be set — "+ + "a hidden text with a value fails the build with CE0463", key, describeConditions(rule)) + } + nullTemplate(item, itemKeys, key) + reset[key] = true + changed = true + continue + } def, hasDefault := defaults[key] cur, isPrimitive := values[key] if !hasDefault || !isPrimitive || cur == def { @@ -204,3 +266,35 @@ func columnPropDefaults(widgetDoc bson.D, listKey string) map[string]string { } return out } + +// keyKind returns the value kind of the item property with schema key key. +func keyKind(keys, kinds map[string]string, key string) string { + for id, k := range keys { + if k == key { + return kinds[id] + } + } + return "" +} + +// itemPropValue returns the Value document of the item property key. +func itemPropValue(item bson.D, keys map[string]string, key string) bson.D { + for _, p := range bsonnav.DGetArrayElements(bsonnav.DGet(item, "Properties")) { + if pd, ok := p.(bson.D); ok && keys[bsonnav.ExtractBinaryIDFromDoc(bsonnav.DGet(pd, "TypePointer"))] == key { + return bsonnav.DGetDoc(pd, "Value") + } + } + return nil +} + +// templateFilled reports whether the item property key holds a TextTemplate. +func templateFilled(item bson.D, keys map[string]string, key string) bool { + return bsonnav.DGetDoc(itemPropValue(item, keys, key), "TextTemplate") != nil +} + +// nullTemplate stores the item property key's TextTemplate as null. +func nullTemplate(item bson.D, keys map[string]string, key string) { + if val := itemPropValue(item, keys, key); val != nil { + bsonnav.DSet(val, "TextTemplate", nil) + } +} diff --git a/mdl/backend/pagemutator/column_hidden_reset_test.go b/mdl/backend/pagemutator/column_hidden_reset_test.go index 007d595459..1e11f66385 100644 --- a/mdl/backend/pagemutator/column_hidden_reset_test.go +++ b/mdl/backend/pagemutator/column_hidden_reset_test.go @@ -62,7 +62,7 @@ func widthRules() []types.WidgetVisibilityRule { func TestResetHiddenColumnProps1519_SizeGoesBackToDefault(t *testing.T) { col, keys, kinds := widthColumn("autoFill", "auto") - got, err := resetHiddenItemProps(col, keys, kinds, columnDefaults, nil, "columns", widthRules(), map[string]bool{"width": true}) + got, err := resetHiddenItemProps(col, keys, kinds, columnDefaults, nil, "columns", widthRules(), map[string]bool{"width": true}, nil) if err != nil { t.Fatalf("reset: %v", err) } @@ -78,7 +78,7 @@ func TestResetHiddenColumnProps1519_SizeGoesBackToDefault(t *testing.T) { // minWidth, and minWidth back at "auto" hides minWidthLimit. func TestResetHiddenColumnProps1519_CascadesThroughDependentRules(t *testing.T) { col, keys, kinds := widthColumn("autoFill", "manual") - if _, err := resetHiddenItemProps(col, keys, kinds, columnDefaults, nil, "columns", widthRules(), nil); err != nil { + if _, err := resetHiddenItemProps(col, keys, kinds, columnDefaults, nil, "columns", widthRules(), nil, nil); err != nil { t.Fatalf("reset: %v", err) } if v := fieldOf(t, col, idMinWidth, "PrimitiveValue"); v != "auto" { @@ -92,7 +92,7 @@ func TestResetHiddenColumnProps1519_CascadesThroughDependentRules(t *testing.T) // CONTROL: under "manual" size is visible and keeps the author's value. func TestResetHiddenColumnProps1519_VisibleSizeIsKept(t *testing.T) { col, keys, kinds := widthColumn("manual", "auto") - if _, err := resetHiddenItemProps(col, keys, kinds, columnDefaults, nil, "columns", widthRules(), nil); err != nil { + if _, err := resetHiddenItemProps(col, keys, kinds, columnDefaults, nil, "columns", widthRules(), nil, nil); err != nil { t.Fatalf("reset: %v", err) } if v := fieldOf(t, col, idSize, "PrimitiveValue"); v != "220" { @@ -105,7 +105,7 @@ func TestResetHiddenColumnProps1519_VisibleSizeIsKept(t *testing.T) { // refuses it on create. Its default is still accepted. func TestResetHiddenColumnProps1519_NamedHiddenPropertyIsRefused(t *testing.T) { col, keys, kinds := widthColumn("autoFill", "auto") - _, err := resetHiddenItemProps(col, keys, kinds, columnDefaults, nil, "columns", widthRules(), map[string]bool{"size": true}) + _, err := resetHiddenItemProps(col, keys, kinds, columnDefaults, nil, "columns", widthRules(), map[string]bool{"size": true}, nil) if err == nil || !strings.Contains(err.Error(), "CE0463") || !strings.Contains(err.Error(), `"size"`) { t.Fatalf("want a refusal naming size and CE0463, got %v", err) } diff --git a/mdl/backend/pagemutator/mutator.go b/mdl/backend/pagemutator/mutator.go index 161c3c6dc1..8517c02fab 100644 --- a/mdl/backend/pagemutator/mutator.go +++ b/mdl/backend/pagemutator/mutator.go @@ -38,6 +38,10 @@ type Deps interface { SerializeClientAction(a pages.ClientAction) bson.D // SerializeCustomWidgetDataSource converts a pluggable-widget data source to raw bson.D. SerializeCustomWidgetDataSource(ds pages.DataSource) bson.D + // SerializeClientTemplate builds an empty Forms$ClientTemplate, for a + // text-template property that is stored null and has to hold text — the + // codec's shape, not one assembled here. + SerializeClientTemplate() bson.D // BuildDataGrid2Column builds a DataGrid2 column object (raw bson.D) from a // column spec, given the column object's type ID and per-property type IDs. // Returns an error if the engine does not support DataGrid2 column ALTER (so @@ -122,6 +126,9 @@ func (m *Mutator) SetWidgetProperty(widgetRef string, prop string, value any) er if n := m.columnMatchCount(widgetRef); n > 1 { return columnAmbiguityError(widgetRef, n) } + if result.item != nil { + m.ensureItemTemplate(result, prop) + } return setColumnPropertyMut(result.widget, result.colPropKeys, result.colPropKinds, prop, value) } return setRawWidgetPropertyMut(result.widget, prop, value) @@ -516,6 +523,9 @@ func (m *Mutator) InsertWidget(widgetRef string, columnRef string, position back if result == nil { return m.widgetNotFoundError(widgetRef) } + if result.item != nil { + return m.insertIntoObjectListItem(result, widgetRef, position, widgets) + } if err := refuseObjectListItemTarget(result, widgetRef); err != nil { return err } @@ -543,6 +553,9 @@ func (m *Mutator) InsertWidget(widgetRef string, columnRef string, position back if strings.EqualFold(string(position), "into") { newContainer, err := appendChildrenToContainer(result.widget, widgetRef, newBsonWidgets) if err != nil { + if hint := objectListItemHint(result, widgetRef); hint != "" { + return fmt.Errorf("cannot INSERT INTO %q: %s", widgetRef, hint) + } if hint := pluggableSlotHint(result, widgetRef); hint != "" { return fmt.Errorf("cannot INSERT INTO %q: a pluggable widget keeps its child widgets in named slots, "+ "not in a list of its own — use %s", widgetRef, hint) @@ -704,6 +717,15 @@ func (m *Mutator) ReplaceWidget(widgetRef string, columnRef string, widgets []pa if result == nil { return m.widgetNotFoundError(widgetRef) } + if result.item != nil { + // Widgets in place of an item would be written into the item list, + // which the loader cannot read (the #935 shape). The item's content is + // a slot, and replacing that is what was meant. + body := strings.ToLower(types.BodyItemSlotKey(result.item.slotKeys)) + return fmt.Errorf("cannot replace %q with widgets: it is a %s item, not a widget — replace its "+ + "content with `replace %s.%s with { … }`, or remove the item with `drop %s`", + widgetRef, result.item.listKey, widgetRef, body, widgetRef) + } if err := refuseObjectListItemTarget(result, widgetRef); err != nil { return err } @@ -1460,6 +1482,10 @@ type bsonWidgetResult struct { // declares (Expression, TextTemplate, Boolean, …). Without it a setter // cannot tell which field of a WidgetValue to write — see columnValueField. colPropKinds map[string]string + // item is set when the result is a pluggable widget's object-list item + // addressed as `owner item…` (an Accordion group): its slots are reached + // through it, and it is never a widget to insert beside or replace. + item *objectListItemInfo } // widgetFinder is a function type for locating widgets in a raw BSON tree. diff --git a/mdl/backend/pagemutator/navlist.go b/mdl/backend/pagemutator/navlist.go index 87037f546f..106a7d002f 100644 --- a/mdl/backend/pagemutator/navlist.go +++ b/mdl/backend/pagemutator/navlist.go @@ -175,8 +175,13 @@ func resolveNavItem(rawData bson.D, find widgetFinder, sel backend.NavItemSelect return nil, fmt.Errorf("%s: navigation list %q not found", sel, sel.List) } if !isNavigationList(list.widget) { - return nil, fmt.Errorf("%s: %q is a %s, not a navigation list — `item(…)` and `item@n` address a navigation list's items", - sel, sel.List, widgetTypeName(list.widget)) + // A pluggable widget's object-list item (an Accordion group) has no + // name either, and is addressed the same way. + if bsonnav.DGetString(list.widget, "$Type") == "CustomWidgets$CustomWidget" { + return resolveObjectListItem(list, sel) + } + return nil, fmt.Errorf("%s: %q is a %s, not a navigation list — `item(…)` and `item@n` address a navigation list's "+ + "items or a pluggable widget's object-list items", sel, sel.List, widgetTypeName(list.widget)) } items := bsonnav.DGetArrayElements(bsonnav.DGet(list.widget, "Items")) var matches []int diff --git a/mdl/backend/pagemutator/object_list_item_address_test.go b/mdl/backend/pagemutator/object_list_item_address_test.go new file mode 100644 index 0000000000..bdc56bc58d --- /dev/null +++ b/mdl/backend/pagemutator/object_list_item_address_test.go @@ -0,0 +1,457 @@ +// SPDX-License-Identifier: Apache-2.0 + +package pagemutator + +import ( + "strings" + "testing" + + "go.mongodb.org/mongo-driver/v2/bson" + + "github.com/mendixlabs/mxcli/mdl/backend" + "github.com/mendixlabs/mxcli/mdl/backend/bsonnav" + "github.com/mendixlabs/mxcli/mdl/types" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/pages" +) + +// PROPOSAL_alter_object_list_item_address.md (ako/mxcli#1326, +// mendixlabs/mxcli#1453): an Accordion group stores no Name, so +// `insert into grp1 { … }` never resolved. It is addressed as an item of the +// accordion — `acc1 item@1`, `acc1 item('One')`, `acc1 item(btnIn)` — and a +// slot of it as `acc1 item@1.headercontent`. + +func tpID(b byte) bson.Binary { + return bson.Binary{Subtype: 0x04, Data: []byte{b, b, b, b, b, b, b, b, b, b, b, b, b, b, b, b}} +} + +var ( + tpGroups = tpID(0x10) + tpRenderMode = tpID(0x11) + tpHeaderText = tpID(0x12) + tpHeaderSlot = tpID(0x13) + tpContent = tpID(0x14) +) + +func textTemplate(text string) bson.D { + return bson.D{ + {Key: "$Type", Value: "Forms$ClientTemplate"}, + {Key: "Parameters", Value: bson.A{int32(2)}}, + {Key: "Template", Value: bson.D{ + {Key: "$Type", Value: "Texts$Text"}, + {Key: "Items", Value: bson.A{int32(3), bson.D{ + {Key: "$Type", Value: "Texts$Translation"}, + {Key: "LanguageCode", Value: "en_US"}, + {Key: "Text", Value: text}, + }}}, + }}, + } +} + +func namedWidget(name string) bson.D { + return bson.D{{Key: "$Type", Value: "Forms$DynamicText"}, {Key: "Name", Value: name}} +} + +func widgetList(names ...string) bson.A { + out := bson.A{int32(2)} + for _, n := range names { + out = append(out, namedWidget(n)) + } + return out +} + +// accordionGroup is one CustomWidgets$WidgetObject of the `groups` list. +func accordionGroup(header string, headerWidgets, bodyWidgets []string) bson.D { + prop := func(tp bson.Binary, value bson.D) bson.D { + return bson.D{{Key: "$Type", Value: "CustomWidgets$WidgetProperty"}, {Key: "TypePointer", Value: tp}, {Key: "Value", Value: value}} + } + return bson.D{ + {Key: "$Type", Value: "CustomWidgets$WidgetObject"}, + {Key: "Properties", Value: bson.A{int32(2), + prop(tpRenderMode, bson.D{{Key: "PrimitiveValue", Value: "custom"}, {Key: "Widgets", Value: bson.A{int32(2)}}}), + prop(tpHeaderText, bson.D{{Key: "TextTemplate", Value: textTemplate(header)}, {Key: "Widgets", Value: bson.A{int32(2)}}}), + prop(tpHeaderSlot, bson.D{{Key: "Widgets", Value: widgetList(headerWidgets...)}}), + prop(tpContent, bson.D{{Key: "Widgets", Value: widgetList(bodyWidgets...)}}), + }}, + } +} + +// accordionWithGroups is acc1 with the given groups, its schema declaring +// headerContent BEFORE content — so the body is chosen by name, not position. +func accordionWithGroups(groups ...bson.D) bson.D { + pt := func(id bson.Binary, key, kind string) bson.D { + return bson.D{{Key: "$ID", Value: id}, {Key: "PropertyKey", Value: key}, {Key: "ValueType", Value: bson.D{{Key: "Type", Value: kind}}}} + } + objects := bson.A{int32(2)} + for _, g := range groups { + objects = append(objects, g) + } + return bson.D{ + {Key: "$Type", Value: "CustomWidgets$CustomWidget"}, + {Key: "Name", Value: "acc1"}, + {Key: "Type", Value: bson.D{ + {Key: "$Type", Value: "CustomWidgets$CustomWidgetType"}, + {Key: "WidgetId", Value: "com.mendix.widget.web.accordion.Accordion"}, + {Key: "ObjectType", Value: bson.D{{Key: "PropertyTypes", Value: bson.A{int32(2), bson.D{ + {Key: "$ID", Value: tpGroups}, + {Key: "PropertyKey", Value: "groups"}, + {Key: "ValueType", Value: bson.D{ + {Key: "Type", Value: "Object"}, + {Key: "IsList", Value: true}, + {Key: "ObjectType", Value: bson.D{{Key: "PropertyTypes", Value: bson.A{int32(2), + pt(tpRenderMode, "headerRenderMode", "Enumeration"), + append(pt(tpHeaderText, "headerText", "TextTemplate")[:2], bson.E{Key: "ValueType", Value: bson.D{ + {Key: "Type", Value: "TextTemplate"}, {Key: "DefaultValue", Value: "Header"}}}), + pt(tpHeaderSlot, "headerContent", "Widgets"), + pt(tpContent, "content", "Widgets"), + }}}}, + }}, + }}}}}, + }}, + {Key: "Object", Value: bson.D{{Key: "Properties", Value: bson.A{int32(2), bson.D{ + {Key: "$Type", Value: "CustomWidgets$WidgetProperty"}, + {Key: "TypePointer", Value: tpGroups}, + {Key: "Value", Value: bson.D{{Key: "Objects", Value: objects}, {Key: "Widgets", Value: bson.A{int32(2)}}}}, + }}}}}, + } +} + +func twoGroupMutator() *Mutator { + return New(makeRawPage(accordionWithGroups( + accordionGroup("One", nil, []string{"btnIn"}), + accordionGroup("Two", nil, nil), + )), model.ID("u"), &stubWidgetDeps{}) +} + +// groupsOf returns acc1's stored groups. +func groupsOf(t *testing.T, m *Mutator) []any { + t.Helper() + acc := findWidgetForTest(t, m.rawData, "acc1") + prop := bsonnav.DGetArrayElements(bsonnav.DGet(bsonnav.DGetDoc(acc, "Object"), "Properties"))[0].(bson.D) + return bsonnav.DGetArrayElements(bsonnav.DGet(bsonnav.DGetDoc(prop, "Value"), "Objects")) +} + +// slotNames returns the widget names in group g's property tp, checking the +// list keeps its [2] marker. +func slotNames(t *testing.T, m *Mutator, g int, tp bson.Binary) []string { + t.Helper() + group := groupsOf(t, m)[g].(bson.D) + for _, p := range bsonnav.DGetArrayElements(bsonnav.DGet(group, "Properties")) { + pd := p.(bson.D) + if bsonnav.ExtractBinaryIDFromDoc(bsonnav.DGet(pd, "TypePointer")) != bsonnav.ExtractBinaryIDFromDoc(tp) { + continue + } + raw := bsonnav.ToBsonA(bsonnav.DGet(bsonnav.DGetDoc(pd, "Value"), "Widgets")) + if len(raw) == 0 || raw[0] != int32(2) { + t.Fatalf("slot lost its [2] marker: %v", raw) + } + var names []string + for _, w := range raw[1:] { + names = append(names, bsonnav.DGetString(w.(bson.D), "Name")) + } + return names + } + t.Fatalf("group %d has no such property", g) + return nil +} + +func itemButton(name string) []pages.Widget { + b := &pages.ActionButton{} + b.Name = name + return []pages.Widget{b} +} + +var into = backend.InsertPosition("into") + +func TestInsertIntoItemBody_EverySelector(t *testing.T) { + for _, c := range []struct { + ref string + group int + want string + }{ + {"acc1 item@1", 0, "btnIn,btnNew"}, + {"acc1 item@2", 1, "btnNew"}, // an EMPTY body — the case nothing could reach + {"acc1 item('Two')", 1, "btnNew"}, + {"acc1 item(btnIn)", 0, "btnIn,btnNew"}, + } { + m := twoGroupMutator() + if err := m.InsertWidget(c.ref, "", into, itemButton("btnNew")); err != nil { + t.Fatalf("insert into %s: %v", c.ref, err) + } + if got := strings.Join(slotNames(t, m, c.group, tpContent), ","); got != c.want { + t.Errorf("insert into %s: group %d content = %s, want %s", c.ref, c.group+1, got, c.want) + } + if got := slotNames(t, m, c.group, tpHeaderSlot); len(got) != 0 { + t.Errorf("insert into %s wrote into headerContent too: %v", c.ref, got) + } + } +} + +func TestInsertIntoItemSlot(t *testing.T) { + m := twoGroupMutator() + if err := m.InsertWidget("acc1 item@1", "headercontent", into, itemButton("btnHead")); err != nil { + t.Fatalf("insert into acc1 item@1.headercontent: %v", err) + } + if got := slotNames(t, m, 0, tpHeaderSlot); len(got) != 1 || got[0] != "btnHead" { + t.Errorf("headerContent = %v, want [btnHead]", got) + } + if got := strings.Join(slotNames(t, m, 0, tpContent), ","); got != "btnIn" { + t.Errorf("content = %s, want btnIn untouched", got) + } +} + +func TestReplaceAndDropItemSlot(t *testing.T) { + m := twoGroupMutator() + if err := m.ReplaceWidget("acc1 item@1", "content", itemButton("btnRep")); err != nil { + t.Fatalf("replace acc1 item@1.content: %v", err) + } + if got := strings.Join(slotNames(t, m, 0, tpContent), ","); got != "btnRep" { + t.Errorf("after replace content = %s, want btnRep", got) + } + if err := m.DropWidget([]backend.WidgetRef{{Widget: "acc1 item@1", Column: "content"}}); err != nil { + t.Fatalf("drop acc1 item@1.content: %v", err) + } + if got := slotNames(t, m, 0, tpContent); len(got) != 0 { + t.Errorf("after drop content = %v, want empty", got) + } + if n := len(groupsOf(t, m)); n != 2 { + t.Errorf("dropping a slot removed a group: %d groups left", n) + } +} + +func TestDropItem(t *testing.T) { + m := twoGroupMutator() + if err := m.DropWidget([]backend.WidgetRef{{Widget: "acc1 item@2"}}); err != nil { + t.Fatalf("drop acc1 item@2: %v", err) + } + groups := groupsOf(t, m) + if len(groups) != 1 { + t.Fatalf("%d groups left, want 1", len(groups)) + } + if got := strings.Join(slotNames(t, m, 0, tpContent), ","); got != "btnIn" { + t.Errorf("the wrong group was dropped: remaining content %s", got) + } +} + +func TestSetItemProperty(t *testing.T) { + m := twoGroupMutator() + if err := m.SetWidgetProperty("acc1 item@1", "headerText", "Renamed"); err != nil { + t.Fatalf("set headerText on acc1 item@1: %v", err) + } + if err := m.InsertWidget("acc1 item('Renamed')", "", into, itemButton("btnNew")); err != nil { + t.Fatalf("the new header text does not address the group: %v", err) + } +} + +func TestResolveAlterTargetItem(t *testing.T) { + m := twoGroupMutator() + for _, c := range []struct { + path []string + kind string + }{ + {[]string{"acc1 item@1"}, "item"}, + {[]string{"acc1 item@1", "headercontent"}, "region"}, + } { + got, err := m.ResolveAlterTarget(backend.AlterTarget{Path: c.path}) + if err != nil { + t.Fatalf("ResolveAlterTarget(%v): %v", c.path, err) + } + if got.Kind != c.kind { + t.Errorf("ResolveAlterTarget(%v) kind = %q, want %q", c.path, got.Kind, c.kind) + } + } +} + +func TestItemAddressMissAndAmbiguity(t *testing.T) { + m := New(makeRawPage(accordionWithGroups( + accordionGroup("Same", nil, nil), + accordionGroup("Same", nil, nil), + )), model.ID("u"), &stubWidgetDeps{}) + err := m.InsertWidget("acc1 item('Same')", "", into, itemButton("b")) + if err == nil || !strings.Contains(err.Error(), "ambiguous") || !strings.Contains(err.Error(), "@1") { + t.Errorf("two groups titled Same: want an ambiguity error naming @n, got %v", err) + } + err = m.InsertWidget("acc1 item@3", "", into, itemButton("b")) + if err == nil || !strings.Contains(err.Error(), "item@1") || !strings.Contains(err.Error(), "item@2") { + t.Errorf("item@3 of two: want a miss listing the addresses that resolve, got %v", err) + } + if err := m.InsertWidget("acc1 item('Same')@2", "", into, itemButton("b")); err != nil { + t.Errorf("@n among matches: %v", err) + } +} + +// Adding a new group is not part of this change; BEFORE/AFTER an item must be +// refused, not treated as INTO or written into the Objects list as a widget. +func TestInsertBesideItemIsRefused(t *testing.T) { + m := twoGroupMutator() + err := m.InsertWidget("acc1 item@1", "", backend.InsertPosition("after"), itemButton("b")) + if err == nil { + t.Fatal("insert after acc1 item@1 { widget } must be refused") + } + if n := len(groupsOf(t, m)); n != 2 { + t.Errorf("a refused insert changed the group count to %d", n) + } +} + +// The values the hidden-slot refusal evaluates come from the stored item. +func TestObjectListItemValues(t *testing.T) { + m := twoGroupMutator() + v, ok := m.ObjectListItemValues("acc1 item@1") + if !ok { + t.Fatal("ObjectListItemValues(acc1 item@1) found nothing") + } + if v.WidgetID != "com.mendix.widget.web.accordion.Accordion" || v.ListKey != "groups" || v.BodySlot != "content" { + t.Errorf("got %+v", v) + } + if v.Item["headerrendermode"] != "custom" { + t.Errorf("item values = %v, want headerrendermode=custom", v.Item) + } + if _, ok := m.ObjectListItemValues("acc1"); ok { + t.Error("a widget that is not an item must report ok=false") + } +} + +// accordionHeaderRules is the Accordion's own rule pair: headerContent hidden +// while headerRenderMode is "text", headerText hidden while it is not. +func accordionHeaderRules() []types.WidgetVisibilityRule { + cond := func(op string) *types.WidgetVisibilityCondition { + return &types.WidgetVisibilityCondition{PropertyKey: "headerRenderMode", Operator: op, Value: "text", Scope: types.ConditionScopeItem} + } + return []types.WidgetVisibilityRule{ + {PropertyKey: "headerContent", ListPropertyKey: "groups", HiddenWhen: cond("eq")}, + {PropertyKey: "headerText", ListPropertyKey: "groups", HiddenWhen: cond("ne")}, + } +} + +// headerTextTemplate returns group g's stored headerText template, or nil. +func headerTextTemplate(t *testing.T, m *Mutator, g int) any { + t.Helper() + group := groupsOf(t, m)[g].(bson.D) + for _, p := range bsonnav.DGetArrayElements(bsonnav.DGet(group, "Properties")) { + pd := p.(bson.D) + if bsonnav.ExtractBinaryIDFromDoc(bsonnav.DGet(pd, "TypePointer")) == bsonnav.ExtractBinaryIDFromDoc(tpHeaderText) { + return bsonnav.DGet(bsonnav.DGetDoc(pd, "Value"), "TextTemplate") + } + } + t.Fatal("no headerText property") + return nil +} + +// `set (headerRenderMode: 'custom') on acc1 item@1` hides headerText. A hidden +// template must be null — what CREATE stores for a custom-mode group (#1198); +// leaving it filled was CE0463 at the Accordion on 11.13.0 (project as stored). +func TestSetItemPropertyNullsTheTemplateItHides(t *testing.T) { + m := New(makeRawPage(accordionWithGroups(accordionGroup("One", nil, nil))), model.ID("u"), &stubWidgetDeps{}) + // Start in text mode, as a group created with headerText is. + if err := m.SetWidgetProperty("acc1 item@1", "headerRenderMode", "text"); err != nil { + t.Fatal(err) + } + if err := m.SetWidgetProperty("acc1 item@1", "headerRenderMode", "custom"); err != nil { + t.Fatal(err) + } + if _, err := m.ResetHiddenItemProperties("acc1 item@1", accordionHeaderRules(), []string{"headerRenderMode"}); err != nil { + t.Fatalf("ResetHiddenItemProperties: %v", err) + } + if tmpl := headerTextTemplate(t, m, 0); tmpl != nil { + t.Errorf("headerText is hidden in custom mode and must be null, got %v", tmpl) + } +} + +// Naming the hidden property in the same SET is refused, as on create. +func TestSetItemPropertyRefusesAHiddenTemplateItNames(t *testing.T) { + m := New(makeRawPage(accordionWithGroups(accordionGroup("One", nil, nil))), model.ID("u"), &stubWidgetDeps{}) + _, err := m.ResetHiddenItemProperties("acc1 item@1", accordionHeaderRules(), []string{"headerRenderMode", "headerText"}) + if err == nil || !strings.Contains(err.Error(), "headerText") { + t.Fatalf("want a refusal naming headerText, got %v", err) + } +} + +// templateDeps builds an empty Forms$ClientTemplate, as the codec does. +type templateDeps struct{ stubWidgetDeps } + +func (templateDeps) SerializeClientTemplate() bson.D { + return bson.D{ + {Key: "$Type", Value: "Forms$ClientTemplate"}, + {Key: "Fallback", Value: bson.D{{Key: "$Type", Value: "Texts$Text"}, {Key: "Items", Value: bson.A{int32(3)}}}}, + {Key: "Parameters", Value: bson.A{int32(2)}}, + {Key: "Template", Value: bson.D{{Key: "$Type", Value: "Texts$Text"}, {Key: "Items", Value: bson.A{int32(3)}}}}, + } +} + +// customGroup is a group as CREATE stores one in custom mode: headerText null. +func customGroup() bson.D { + g := accordionGroup("unused", nil, nil) + for _, p := range bsonnav.DGetArrayElements(bsonnav.DGet(g, "Properties")) { + pd := p.(bson.D) + if bsonnav.ExtractBinaryIDFromDoc(bsonnav.DGet(pd, "TypePointer")) == bsonnav.ExtractBinaryIDFromDoc(tpHeaderText) { + bsonnav.DSet(bsonnav.DGetDoc(pd, "Value"), "TextTemplate", nil) + } + } + return g +} + +// Switching a custom-mode group back to 'text' makes headerText visible with +// a null template — CE0463 the other way round. With a declared default the +// reset fills it with that, as CREATE writes a visible headerText. +func TestSetItemPropertyFillsTheTemplateItReveals(t *testing.T) { + m := New(makeRawPage(accordionWithGroups(customGroup())), model.ID("u"), &templateDeps{}) + if err := m.SetWidgetProperty("acc1 item@1", "headerRenderMode", "text"); err != nil { + t.Fatal(err) + } + if _, err := m.ResetHiddenItemProperties("acc1 item@1", accordionHeaderRules(), []string{"headerRenderMode"}); err != nil { + t.Fatalf("ResetHiddenItemProperties: %v", err) + } + if headerTextTemplate(t, m, 0) == nil { + t.Fatal("a visible headerText must not stay null") + } + if err := m.InsertWidget("acc1 item('Header')", "", into, itemButton("b")); err != nil { + t.Errorf("the revealed headerText should hold the schema default 'Header': %v", err) + } +} + +// Setting the text of an item whose template is null creates the template. +func TestSetItemTextCreatesANullTemplate(t *testing.T) { + m := New(makeRawPage(accordionWithGroups(customGroup())), model.ID("u"), &templateDeps{}) + if err := m.SetWidgetProperty("acc1 item@1", "headerRenderMode", "text"); err != nil { + t.Fatal(err) + } + if err := m.SetWidgetProperty("acc1 item@1", "headerText", "Two"); err != nil { + t.Fatalf("set headerText on a null template: %v", err) + } + if err := m.InsertWidget("acc1 item('Two')", "", into, itemButton("b")); err != nil { + t.Errorf("headerText 'Two' was not written: %v", err) + } +} + +// The real Accordion declares DefaultValue "" for headerText, so there is no +// text to fill in, and an empty required text is CE4899. The SET that reveals +// it must say so rather than write an empty template. +func TestSetItemPropertyRefusesARevealedTemplateWithNoText(t *testing.T) { + acc := accordionWithGroups(customGroup()) + // Blank out the fixture's declared default. + var clear func(v any) + clear = func(v any) { + switch d := v.(type) { + case bson.D: + for i := range d { + if d[i].Key == "DefaultValue" { + d[i].Value = "" + } + clear(d[i].Value) + } + case bson.A: + for _, e := range d { + clear(e) + } + } + } + clear(acc) + m := New(makeRawPage(acc), model.ID("u"), &templateDeps{}) + if err := m.SetWidgetProperty("acc1 item@1", "headerRenderMode", "text"); err != nil { + t.Fatal(err) + } + _, err := m.ResetHiddenItemProperties("acc1 item@1", accordionHeaderRules(), []string{"headerRenderMode"}) + if err == nil || !strings.Contains(err.Error(), "headerText") { + t.Fatalf("want a refusal asking for headerText, got %v", err) + } +} diff --git a/mdl/backend/pagemutator/objectlist_items.go b/mdl/backend/pagemutator/objectlist_items.go new file mode 100644 index 0000000000..4369233d3c --- /dev/null +++ b/mdl/backend/pagemutator/objectlist_items.go @@ -0,0 +1,357 @@ +// SPDX-License-Identifier: Apache-2.0 + +package pagemutator + +import ( + "fmt" + "strings" + + "go.mongodb.org/mongo-driver/v2/bson" + + "github.com/mendixlabs/mxcli/mdl/backend" + "github.com/mendixlabs/mxcli/mdl/backend/bsonnav" + "github.com/mendixlabs/mxcli/mdl/types" + "github.com/mendixlabs/mxcli/sdk/pages" +) + +// A pluggable widget's object-list item — an Accordion group, a Pop-up menu +// item — is a CustomWidgets$WidgetObject and stores no Name, so `insert into +// grp1` never resolved: the name a script wrote on `group grp1` was parsed and +// dropped (ako/mxcli#1326, mendixlabs/mxcli#1453). It is addressed the way a +// navigation list item is (#1388), by what it holds, what it shows or where it +// is: `acc1 item(btnIn)`, `acc1 item('One')`, `acc1 item@1`. A bare item address +// means the item's BODY slot (types.BodyItemSlotKey, the slot CREATE routes loose +// children to); `acc1 item@1.headercontent` names another slot, as +// `grid.controlbar` names a widget's. PROPOSAL_alter_object_list_item_address.md. + +// objectListItemInfo is what a resolved item carries beyond its position: the +// widget that owns the list, and the item's widget slots in schema order. +type objectListItemInfo struct { + owner bson.D + listKey string + slotKeys []string // Widgets-typed item property keys, schema order +} + +// itemList is one object-list property of a pluggable widget. +type itemList struct { + key string + typeID string + value bson.D // the property's WidgetValue, holding Objects + slotKeys []string +} + +// objectItemLists returns the widget's object-list properties: its schema's +// `Object`-typed, IsList properties that are stored with an Objects list. +func objectItemLists(widget bson.D) []itemList { + objType := bsonnav.DGetDoc(bsonnav.DGetDoc(widget, "Type"), "ObjectType") + var lists []itemList + for _, pt := range bsonnav.DGetArrayElements(bsonnav.DGet(objType, "PropertyTypes")) { + ptDoc, ok := pt.(bson.D) + if !ok { + continue + } + vt := bsonnav.DGetDoc(ptDoc, "ValueType") + if bsonnav.DGetString(vt, "Type") != "Object" || bsonnav.DGet(vt, "IsList") != true { + continue + } + l := itemList{ + key: bsonnav.DGetString(ptDoc, "PropertyKey"), + typeID: bsonnav.ExtractBinaryIDFromDoc(bsonnav.DGet(ptDoc, "$ID")), + } + for _, ipt := range bsonnav.DGetArrayElements(bsonnav.DGet(bsonnav.DGetDoc(vt, "ObjectType"), "PropertyTypes")) { + iptDoc, ok := ipt.(bson.D) + if ok && bsonnav.DGetString(bsonnav.DGetDoc(iptDoc, "ValueType"), "Type") == "Widgets" { + l.slotKeys = append(l.slotKeys, bsonnav.DGetString(iptDoc, "PropertyKey")) + } + } + for _, prop := range bsonnav.DGetArrayElements(bsonnav.DGet(bsonnav.DGetDoc(widget, "Object"), "Properties")) { + propDoc, ok := prop.(bson.D) + if ok && bsonnav.ExtractBinaryIDFromDoc(bsonnav.DGet(propDoc, "TypePointer")) == l.typeID { + l.value = bsonnav.DGetDoc(propDoc, "Value") + } + } + if l.value != nil && hasKey(l.value, "Objects") { + lists = append(lists, l) + } + } + return lists +} + +// resolveObjectListItem finds the one item of a pluggable widget a selector +// addresses, with the same matching, @n and messages as a navigation list. +func resolveObjectListItem(owner *bsonWidgetResult, sel backend.NavItemSelector) (*bsonWidgetResult, error) { + lists := objectItemLists(owner.widget) + switch { + case len(lists) == 0: + return nil, fmt.Errorf("%s: %q (%s) has no list of items — `item(…)` and `item@n` address a navigation "+ + "list's items or a pluggable widget's object-list items (an Accordion's groups)", + sel, sel.List, widgetTypeName(owner.widget)) + case len(lists) > 1: + var keys []string + for _, l := range lists { + keys = append(keys, l.key) + } + return nil, fmt.Errorf("%s: %q has more than one list of items (%s), so `item` does not say which — "+ + "not supported yet", sel, sel.List, strings.Join(keys, ", ")) + } + l := lists[0] + keys, kinds := buildColumnPropKeyMap(owner.widget, l.typeID) + items := bsonnav.DGetArrayElements(bsonnav.DGet(l.value, "Objects")) + var matches []int + for i, el := range items { + itemDoc, ok := el.(bson.D) + if !ok { + continue + } + switch { + case sel.Widget != "": + if !itemHoldsWidget(itemDoc, sel.Widget) { + continue + } + case sel.Text != "": + if !containsString(objectItemTexts(itemDoc, kinds), sel.Text) { + continue + } + } + matches = append(matches, i) + } + pick, err := pickItem(sel, matches, len(items), func() string { return objectItemList(sel.List, items, kinds) }) + if err != nil { + return nil, err + } + return &bsonWidgetResult{ + widget: items[pick].(bson.D), + parentArr: items, + parentKey: "Objects", + parentDoc: l.value, + index: pick, + colPropKeys: keys, + colPropKinds: kinds, + item: &objectListItemInfo{owner: owner.widget, listKey: l.key, slotKeys: l.slotKeys}, + }, nil +} + +// pickItem applies a selector's @n to the indexes that match it. listing names +// every item's addresses, for a message that says what would hit. +func pickItem(sel backend.NavItemSelector, matches []int, total int, listing func() string) (int, error) { + switch { + case sel.Ordinal > 0 && sel.Ordinal <= len(matches): + return matches[sel.Ordinal-1], nil + case sel.Ordinal > 0: + return -1, fmt.Errorf("%s: %s, so there is no @%d. %s", sel, countMatches(len(matches), sel), sel.Ordinal, listing()) + case len(matches) == 1: + return matches[0], nil + case len(matches) == 0: + return -1, fmt.Errorf("%s: no item matches. %s", sel, listing()) + default: + return -1, fmt.Errorf("%s is ambiguous: %s — add @n to choose one (@1 … @%d). %s", + sel, countMatches(len(matches), sel), len(matches), listing()) + } +} + +// itemHoldsWidget reports whether any slot of the item holds the named widget, +// at any depth. +func itemHoldsWidget(itemDoc bson.D, name string) bool { + for _, val := range objectListItemPropValues(itemDoc) { + if findInWidgetArray(val, "Widgets", name) != nil { + return true + } + } + return false +} + +// objectListItemPropValues returns the Value document of every property of an +// item. +func objectListItemPropValues(itemDoc bson.D) []bson.D { + var out []bson.D + for _, p := range bsonnav.DGetArrayElements(bsonnav.DGet(itemDoc, "Properties")) { + if pd, ok := p.(bson.D); ok { + if v := bsonnav.DGetDoc(pd, "Value"); v != nil { + out = append(out, v) + } + } + } + return out +} + +// objectItemTexts returns the texts an item shows: its TextTemplate properties in +// every language, and its String properties — an Accordion group's headerText. +func objectItemTexts(itemDoc bson.D, kinds map[string]string) []string { + var out []string + for _, p := range bsonnav.DGetArrayElements(bsonnav.DGet(itemDoc, "Properties")) { + pd, ok := p.(bson.D) + if !ok { + continue + } + val := bsonnav.DGetDoc(pd, "Value") + switch kinds[bsonnav.ExtractBinaryIDFromDoc(bsonnav.DGet(pd, "TypePointer"))] { + case "TextTemplate": + tmpl := bsonnav.DGetDoc(bsonnav.DGetDoc(val, "TextTemplate"), "Template") + for _, it := range bsonnav.DGetArrayElements(bsonnav.DGet(tmpl, "Items")) { + if d, ok := it.(bson.D); ok { + if t := bsonnav.DGetString(d, "Text"); t != "" && !containsString(out, t) { + out = append(out, t) + } + } + } + case "String": + if t := bsonnav.DGetString(val, "PrimitiveValue"); t != "" && !containsString(out, t) { + out = append(out, t) + } + } + } + return out +} + +// objectItemList lists each item's addresses, so a miss shows what would hit. +func objectItemList(list string, items []any, kinds map[string]string) string { + if len(items) == 0 { + return fmt.Sprintf("%q has no items", list) + } + var parts []string + for i, el := range items { + itemDoc, _ := el.(bson.D) + addrs := []string{fmt.Sprintf("item@%d", i+1)} + if texts := objectItemTexts(itemDoc, kinds); len(texts) > 0 { + addrs = append(addrs, "item('"+strings.ReplaceAll(texts[0], "'", "''")+"')") + } + parts = append(parts, strings.Join(addrs, " = ")) + } + return fmt.Sprintf("Items of %s: %s", list, strings.Join(parts, "; ")) +} + +// itemSlots returns a resolved item's widget slots, keyed as an item slot is +// spelled in MDL: the property key, lower-cased (`headercontent`), the keyword +// CREATE's item builder routes. The first returned slot is not special; the +// body is chosen with types.BodyItemSlotKey. +func itemSlots(result *bsonWidgetResult) []pluggableSlot { + var out []pluggableSlot + for _, p := range bsonnav.DGetArrayElements(bsonnav.DGet(result.widget, "Properties")) { + pd, ok := p.(bson.D) + if !ok { + continue + } + tp := bsonnav.ExtractBinaryIDFromDoc(bsonnav.DGet(pd, "TypePointer")) + if result.colPropKinds[tp] != "Widgets" { + continue + } + val := bsonnav.DGetDoc(pd, "Value") + if val == nil || !hasKey(val, "Widgets") { + continue + } + out = append(out, pluggableSlot{owner: result, keyword: strings.ToLower(result.colPropKeys[tp]), value: val}) + } + return out +} + +// itemBodySlot returns the item's body slot. +func itemBodySlot(result *bsonWidgetResult) (pluggableSlot, bool) { + body := strings.ToLower(types.BodyItemSlotKey(result.item.slotKeys)) + for _, s := range itemSlots(result) { + if s.keyword == body { + return s, true + } + } + return pluggableSlot{}, false +} + +// insertIntoObjectListItem handles a bare item target: INTO fills its body; +// BEFORE/AFTER would add a new item, which ALTER does not do yet. +func (m *Mutator) insertIntoObjectListItem(result *bsonWidgetResult, ref string, position backend.InsertPosition, widgets []pages.Widget) error { + if !strings.EqualFold(string(position), "into") { + return fmt.Errorf("cannot insert %s %q: adding a new %s item is not supported by ALTER yet — "+ + "to put widgets in this item, write `insert into %s { … }`", strings.ToLower(string(position)), + ref, result.item.listKey, ref) + } + body, ok := itemBodySlot(result) + if !ok { + return fmt.Errorf("%q has no widget slot to insert into", ref) + } + newBson, err := m.serializeWidgets(widgets) + if err != nil { + return fmt.Errorf("serialize widgets: %w", err) + } + body.set(append(append([]any{}, body.widgets()...), newBson...)) + return nil +} + +// objectListItemHint names the item addresses of a widget that has an item +// list, for the refusal of `insert into ` on the widget itself. +func objectListItemHint(owner *bsonWidgetResult, name string) string { + lists := objectItemLists(owner.widget) + if len(lists) != 1 || len(lists[0].slotKeys) == 0 { + return "" + } + return fmt.Sprintf("its widgets live in its %s: `insert into %s item@1 { … }` fills the first one, "+ + "`%s item('')` addresses one by its text", lists[0].key, name, name) +} + +// ObjectListItemValues reports a stored item's values for visibility rules. +// ok is false when itemRef does not resolve to an object-list item. +func (m *Mutator) ObjectListItemValues(itemRef string) (backend.ObjectListItemValues, bool) { + result := m.widgetFinder(m.rawData, itemRef) + if result == nil || result.item == nil { + return backend.ObjectListItemValues{}, false + } + ownerKeys := buildPropKeyMap(result.item.owner) + return backend.ObjectListItemValues{ + WidgetID: pluggableWidgetID(result.item.owner), + ListKey: result.item.listKey, + BodySlot: types.BodyItemSlotKey(result.item.slotKeys), + Item: storedScalarValues(result.widget, result.colPropKeys), + Widget: storedScalarValues(bsonnav.DGetDoc(result.item.owner, "Object"), ownerKeys), + }, true +} + +// storedScalarValues maps each property of a WidgetObject (keys: TypePointer +// id -> property key) to its stored PrimitiveValue, or its Expression when it +// has no primitive one. +func storedScalarValues(obj bson.D, keys map[string]string) map[string]string { + out := map[string]string{} + for _, p := range bsonnav.DGetArrayElements(bsonnav.DGet(obj, "Properties")) { + pd, ok := p.(bson.D) + if !ok { + continue + } + key := keys[bsonnav.ExtractBinaryIDFromDoc(bsonnav.DGet(pd, "TypePointer"))] + val := bsonnav.DGetDoc(pd, "Value") + if key == "" || val == nil { + continue + } + v := bsonnav.DGetString(val, "PrimitiveValue") + if v == "" { + v = bsonnav.DGetString(val, "Expression") + } + if v != "" { + out[strings.ToLower(key)] = v + } + } + return out +} + +// newTemplate returns an empty client template from the engine, or nil when +// the engine has none to give. +func (m *Mutator) newTemplate() bson.D { + if m.deps == nil { + return nil + } + return m.deps.SerializeClientTemplate() +} + +// ensureItemTemplate gives an item's text-template property a template when +// it is stored null, so `set (headerText: 'Two') on acc1 item@2` can write a +// group CREATE made in custom mode. Whether the text may be set at all — it +// may be hidden — is the reset pass's to say afterwards, with the refusal it +// gives a hidden property the statement named. +func (m *Mutator) ensureItemTemplate(result *bsonWidgetResult, prop string) { + key := resolveColumnPropertyKey(prop, result.colPropKeys) + if key == "" || keyKind(result.colPropKeys, result.colPropKinds, key) != "TextTemplate" || + templateFilled(result.widget, result.colPropKeys, key) { + return + } + if tmpl := m.newTemplate(); tmpl != nil { + if val := itemPropValue(result.widget, result.colPropKeys, key); val != nil { + bsonnav.DSet(val, "TextTemplate", tmpl) + } + } +} diff --git a/mdl/backend/pagemutator/pluggableslot.go b/mdl/backend/pagemutator/pluggableslot.go index eec127ebc3..8730e81d55 100644 --- a/mdl/backend/pagemutator/pluggableslot.go +++ b/mdl/backend/pagemutator/pluggableslot.go @@ -39,6 +39,9 @@ type pluggableSlot struct { // pluggableSlots lists the widget's own widgets-typed properties (not those of // its object-list items — an Accordion group's content is not the Accordion's). func pluggableSlots(owner *bsonWidgetResult) []pluggableSlot { + if owner.item != nil { + return itemSlots(owner) // `acc1 item@1.headercontent` + } if bsonnav.DGetString(owner.widget, "$Type") != "CustomWidgets$CustomWidget" { return nil } diff --git a/mdl/backend/pagemutator/probe_test.go b/mdl/backend/pagemutator/probe_test.go index b7a8b0c630..3d7cda40fc 100644 --- a/mdl/backend/pagemutator/probe_test.go +++ b/mdl/backend/pagemutator/probe_test.go @@ -17,6 +17,8 @@ type recordingDeps struct{ saves int } func (d *recordingDeps) SerializeWidget(pages.Widget) bson.D { return nil } func (d *recordingDeps) SerializeClientAction(pages.ClientAction) bson.D { return nil } +func (d *recordingDeps) SerializeClientTemplate() bson.D { return nil } + func (d *recordingDeps) SerializeCustomWidgetDataSource(pages.DataSource) bson.D { return nil } diff --git a/mdl/executor/alter_hidden_item_slot.go b/mdl/executor/alter_hidden_item_slot.go new file mode 100644 index 0000000000..6c55479639 --- /dev/null +++ b/mdl/executor/alter_hidden_item_slot.go @@ -0,0 +1,114 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "fmt" + "strings" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/backend" + "github.com/mendixlabs/mxcli/mdl/types" +) + +// objectListItemValuer is the page mutator capability the hidden-slot check +// needs; engines without it are not checked. +type objectListItemValuer interface { + ObjectListItemValues(itemRef string) (backend.ObjectListItemValues, bool) +} + +// refuseHiddenItemSlot refuses an INSERT or REPLACE that writes widgets into a +// slot of an object-list item that the widget's own configuration hides — an +// Accordion group's headerContent while headerRenderMode is 'text'. Such +// widgets are stored but never rendered, and the project as stored fails +// with CE0463 at the widget (PROPOSAL_alter_object_list_item_address.md). +// A bare item address writes into the body slot, so that is checked too. +// +// Called by exec and by check's target validation, so the refusal reaches +// the author before anything is written. +func refuseHiddenItemSlot(ctx *ExecContext, mutator any, ref ast.WidgetRef) error { + if ref.Item == nil { + return nil + } + valuer, ok := mutator.(objectListItemValuer) + if !ok { + return nil + } + vals, ok := valuer.ObjectListItemValues(ref.Widget) + if !ok { + return nil + } + registry := ctx.GetWidgetRegistry() + if registry == nil { + return nil + } + def, ok := registry.GetByWidgetID(vals.WidgetID) + if !ok { + return nil + } + slot := ref.Column + if slot == "" { + slot = vals.BodySlot + } + return itemSlotHiddenError(visibilityRulesFor(def, registry), ref.Widget, vals.ListKey, slot, vals.Item, vals.Widget) +} + +// itemSlotHiddenError evaluates the nested visibility rules for one item slot. +// A rule that cannot be decided from the stored values refuses nothing. +func itemSlotHiddenError(rules []types.WidgetVisibilityRule, itemRef, listKey, slot string, + itemValues, widgetValues map[string]string) error { + for _, rule := range rules { + if !rule.Nested() || rule.HiddenWhen == nil || + !strings.EqualFold(rule.ListPropertyKey, listKey) || !strings.EqualFold(rule.PropertyKey, slot) { + continue + } + fires, determinable := rule.Fires(func(c types.WidgetVisibilityCondition) (string, bool) { + values := widgetValues + if c.Scope == types.ConditionScopeItem { + values = itemValues + } + v, ok := values[strings.ToLower(c.PropertyKey)] + return v, ok + }) + if !determinable || !fires { + continue + } + var why []string + for _, c := range rule.Conditions() { + v := itemValues[strings.ToLower(c.PropertyKey)] + if c.Scope != types.ConditionScopeItem { + v = widgetValues[strings.ToLower(c.PropertyKey)] + } + why = append(why, fmt.Sprintf("%s is '%s'", c.PropertyKey, v)) + } + return fmt.Errorf("cannot write widgets into %s.%s: %s is hidden while %s, so the widgets would be stored "+ + "but not rendered, and the widget fails with CE0463 — change %s on %s first (`set (…) on %s`)", + itemRef, strings.ToLower(slot), rule.PropertyKey, strings.Join(why, " and "), + conditionKeys(rule), itemRef, itemRef) + } + return nil +} + +func conditionKeys(rule types.WidgetVisibilityRule) string { + var keys []string + for _, c := range rule.Conditions() { + keys = append(keys, c.PropertyKey) + } + return strings.Join(keys, ", ") +} + +// refuseHiddenItemSlotFor is refuseHiddenItemSlot for the operations that +// write widgets into a target: INSERT INTO and REPLACE of an item slot. +func refuseHiddenItemSlotFor(ctx *ExecContext, mutator any, op ast.AlterPageOperation, ref ast.WidgetRef) error { + switch o := op.(type) { + case *ast.InsertWidgetOp: + if strings.EqualFold(o.Position, "INTO") { + return refuseHiddenItemSlot(ctx, mutator, ref) + } + case *ast.ReplaceWidgetOp: + if ref.Column != "" { + return refuseHiddenItemSlot(ctx, mutator, ref) + } + } + return nil +} diff --git a/mdl/executor/alter_hidden_item_slot_test.go b/mdl/executor/alter_hidden_item_slot_test.go new file mode 100644 index 0000000000..1fc485dc4b --- /dev/null +++ b/mdl/executor/alter_hidden_item_slot_test.go @@ -0,0 +1,53 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" +) + +// An Accordion hides a group's headerContent while headerRenderMode is +// 'text', the default (`"text"===n.headerRenderMode?hidePropertyIn(…, +// "groups",…,"headerContent")`). Writing widgets there stored content Studio +// Pro does not render, and the project as stored failed with CE0463 at the +// Accordion — measured on a blank 11.13.0 app with --no-update-widgets: 0 +// errors before the write, 1 after; the normalising default check hid it +// (PROPOSAL_alter_object_list_item_address.md, "a slot the item's +// configuration hides is refused"). +func TestHiddenItemSlotIsRefused(t *testing.T) { + rules, _ := extractVisibilityRulesFromJS(accordionGetProperties) + err := itemSlotHiddenError(rules, "acc1 item@1", "groups", "headercontent", + map[string]string{"headerrendermode": "text"}, nil) + if err == nil { + t.Fatal("a write into headerContent of a text-mode group was not refused") + } + for _, want := range []string{"headerContent", "headerRenderMode", "'text'", "acc1 item@1"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("the refusal should name %s: %v", want, err) + } + } +} + +func TestVisibleItemSlotIsAccepted(t *testing.T) { + rules, _ := extractVisibilityRulesFromJS(accordionGetProperties) + if err := itemSlotHiddenError(rules, "acc1 item@1", "groups", "headercontent", + map[string]string{"headerrendermode": "custom"}, nil); err != nil { + t.Errorf("headerContent of a custom-mode group must be writable: %v", err) + } + // The body is never hidden by this rule, whatever the mode. + if err := itemSlotHiddenError(rules, "acc1 item@1", "groups", "content", + map[string]string{"headerrendermode": "text"}, nil); err != nil { + t.Errorf("content must be writable in text mode: %v", err) + } +} + +// A value the stored item does not carry is undeterminable, and an +// undeterminable rule refuses nothing — the same stance as check's +// validateWidgetItemVisibility. +func TestUndeterminableItemSlotIsAccepted(t *testing.T) { + rules, _ := extractVisibilityRulesFromJS(accordionGetProperties) + if err := itemSlotHiddenError(rules, "acc1 item@1", "groups", "headercontent", nil, nil); err != nil { + t.Errorf("no stored headerRenderMode: %v", err) + } +} diff --git a/mdl/executor/cmd_alter_page.go b/mdl/executor/cmd_alter_page.go index c94154fbf1..27bdcbd781 100644 --- a/mdl/executor/cmd_alter_page.go +++ b/mdl/executor/cmd_alter_page.go @@ -350,17 +350,55 @@ func applySetPropertyMutator(ctx *ExecContext, mutator backend.PageMutator, op * if err := applySetProperties(ctx, mutator, op, moduleName, moduleID); err != nil { return err } - if op.Target.IsColumn() { + if op.Target.IsColumn() || op.Target.Item != nil { names := make([]string, 0, len(op.Properties)) for k := range op.Properties { names = append(names, k) } sort.Strings(names) + if op.Target.Item != nil { + return resetHiddenItemProperties(ctx, mutator, op.Target, names) + } return resetHiddenColumnProperties(ctx, mutator, op.Target, names) } return nil } +// itemHiddenPropertyResetter is the page mutator capability behind +// resetHiddenItemProperties; engines without it are left as they were. +type itemHiddenPropertyResetter interface { + ResetHiddenItemProperties(itemRef string, rules []types.WidgetVisibilityRule, setProps []string) ([]string, error) +} + +// resetHiddenItemProperties is resetHiddenColumnProperties for an object-list +// item (`acc1 item@1`): after the SET, every item property the widget's +// editorConfig now hides goes back to how CREATE stores a hidden one. +// `set (headerRenderMode: 'custom')` hid the group's headerText and left its +// template filled: CE0463 at the Accordion. The rules are the installed +// package's, found from the item's own widget id. +func resetHiddenItemProperties(ctx *ExecContext, mutator backend.PageMutator, target ast.WidgetRef, setProps []string) error { + resetter, ok := mutator.(itemHiddenPropertyResetter) + if !ok { + return nil + } + valuer, ok := mutator.(objectListItemValuer) + if !ok { + return nil + } + vals, ok := valuer.ObjectListItemValues(target.Widget) + if !ok { + return nil + } + rules := resolveWidgetVisibilityRules(ctx.MprPath, vals.WidgetID) + if len(rules) == 0 { + return nil + } + if _, err := resetter.ResetHiddenItemProperties(target.Widget, rules, setProps); err != nil { + return mdlerrors.NewBackend("set on "+target.Name(), err) + } + return nil +} + // applySetProperties writes each property of a SET, without the column // follow-up — what check's per-property dry run exercises, since a column's // hidden-property rules can only be judged on the statement as a whole. @@ -545,6 +583,11 @@ func convertASTAction(ctx *ExecContext, mutator backend.PageMutator, value any, // ============================================================================ func applyInsertWidgetMutator(ctx *ExecContext, mutator backend.PageMutator, op *ast.InsertWidgetOp, moduleName string, moduleID model.ID) error { + if strings.EqualFold(op.Position, "INTO") { + if err := refuseHiddenItemSlot(ctx, mutator, op.Target); err != nil { + return err + } + } opWidgets, err := expandAlterFragments(ctx, op.Widgets, moduleName, moduleID) if err != nil { return err @@ -686,6 +729,11 @@ func applyDropWidgetMutator(mutator backend.PageMutator, op *ast.DropWidgetOp) e // ============================================================================ func applyReplaceWidgetMutator(ctx *ExecContext, mutator backend.PageMutator, op *ast.ReplaceWidgetOp, moduleName string, moduleID model.ID, storedWidgets func() map[string]*ast.WidgetV3) error { + if op.Target.Item != nil && op.Target.Column != "" { + if err := refuseHiddenItemSlot(ctx, mutator, op.Target); err != nil { + return err + } + } newWidgets, err := expandAlterFragments(ctx, op.NewWidgets, moduleName, moduleID) if err != nil { return err diff --git a/mdl/executor/cmd_flow_modify.go b/mdl/executor/cmd_flow_modify.go index e23c0989b1..d3fd5a958b 100644 --- a/mdl/executor/cmd_flow_modify.go +++ b/mdl/executor/cmd_flow_modify.go @@ -326,9 +326,18 @@ func planFlowModify(ctx *ExecContext, d *flowDecl) (*flowPlan, error) { // a member of one was written bare (ako/mxcli#885). a.declaredVarTypes = varTypes + // A node the script draws at a stated position is pinned under mdl 1 only + // (ADR-0011: a new refusal applies under the version that introduces it). + // Under mdl 0 the refused splice would fall back to rebuilding the whole + // flow, losing the merges, curves and $IDs the shift keeps, so a headerless + // script makes room as it did (mendixlabs/mxcli#1598, case 6). + var stated []model.Point + if flowRebuildRefused.Applies(ctx.LanguageVersion) { + stated = statedPoints(d) + } p := &flowPlan{a: a, ops: ops, moves: moves, storedFolder: storedFolderOf(stored)} if len(ops) > 0 || len(moves) > 0 || headerChanged { - if p.mut, err = patch(ctx, a, ops, targets, moves); err != nil { + if p.mut, err = patch(ctx, a, ops, targets, moves, stated); err != nil { return nil, cannotSplice("%v", err) } if headerChanged { @@ -1103,14 +1112,16 @@ func (pd *patchDiff) conditionEdit(declared, stored *ast.IfStmt) error { // insert (#859). A loop is paired this way only when its body can be edited in // place (editableLoopBody, ako/mxcli#1106); any other change inside a loop is // refused either way. -// -// An `if` whose condition changes is paired the same way (ako/mxcli#888): the -// condition is set on the stored decision, and a change around it must not -// merge with it into a replace of the decision. func sameBlockShell(declared, stored ast.MicroflowStatement) bool { - if _, _, ok := sameIfShell(declared, stored); ok { - return true - } + _, _, ok := sameIfShell(declared, stored) + return ok +} + +// conditionChanged reports whether two statements are the same `if` but for +// its condition (ifConditionChanged). Such a pair is matched as a pair too +// (ako/mxcli#888): the condition is set on the stored decision, and a change +// around it must not merge with it into a replace of the decision. +func conditionChanged(declared, stored ast.MicroflowStatement) bool { _, _, ok := ifConditionChanged(declared, stored) return ok } @@ -1450,6 +1461,12 @@ func insertAnchor(loc *storedLocator, stored []ast.MicroflowStatement, gapStart, // it too — it is that node, moved (ako/mxcli#818) — but an exact match weighs // more, so of two alike statements the one the script left in place keeps its // node. +// +// An `if` whose condition changes weighs least: of a new `if` and a restated +// one before the same stored decision, the restated one — the same condition +// — is that decision. Weighed alike, the tie went to the earlier pair: the new +// if took the stored decision over and the restated one was inserted as a new +// node, drawn on top of it (mendixlabs/mxcli#1598, case 6). func lcsStatements(declared, stored []ast.MicroflowStatement) [][2]int { n, m := len(declared), len(stored) w := make([][]int, n) @@ -1458,14 +1475,16 @@ func lcsStatements(declared, stored []ast.MicroflowStatement) [][2]int { for j := range stored { switch { case declaredMatches(declared[i], stored[j]): - w[i][j] = 2 + w[i][j] = 4 case sameExceptPositions(declared[i], stored[j]): - w[i][j] = 1 + w[i][j] = 2 case sameBlockShell(declared[i], stored[j]): - w[i][j] = 1 + w[i][j] = 2 case editableLoopBody(declared[i], stored[j]): // A loop edited in place inside is that loop: a change beside // it must not merge with it into a replace (ako/mxcli#1106). + w[i][j] = 2 + case conditionChanged(declared[i], stored[j]): w[i][j] = 1 } } diff --git a/mdl/executor/cmd_flow_modify_patch.go b/mdl/executor/cmd_flow_modify_patch.go index dcd6cf6eaf..908016b05a 100644 --- a/mdl/executor/cmd_flow_modify_patch.go +++ b/mdl/executor/cmd_flow_modify_patch.go @@ -5,6 +5,7 @@ package executor import ( "bytes" "fmt" + "reflect" "strings" "go.mongodb.org/mongo-driver/v2/bson" @@ -32,6 +33,10 @@ import ( // moves that node, and only it: its flows keep their ends, sides and // curves (no reconnection), and nothing around it moves. A position the // script leaves out keeps what is stored, as before; +// - a node the script draws at a stated position stays there: a fragment +// that needs more room than the stated positions leave is refused, rather +// than everything after it moved along, which left the stored flow saying +// something other than the script (mendixlabs/mxcli#1598, case 6); // - two nodes nobody placed follow the node they were derived from, because // mxcli's own layout put them there and nothing else says where they go: // a start event where the layout derives it follows the first statement @@ -56,10 +61,13 @@ const endSpacing = HorizontalSpacing + HorizontalSpacing/2 // patch opens the stored flow and applies the moves and the splice // operations. The moves go first, so a fragment the script places next to a // moved node is joined to it where it now is, and again after the operations, -// so a node an insert shifted to make room is still drawn where the script -// says (a move sets a position, so applying it twice is applying it once). +// so a node no one placed that follows another (followingEnd) is drawn where +// the script says even if an insert shifted it to make room (a move sets a +// position, so applying it twice is applying it once). Between the two, every +// point the script states a node at is pinned, so an operation that would +// have to move one to make room is refused (mendixlabs/mxcli#1598, case 6). func patch(ctx *ExecContext, a *alterFlowContext, ops []*ast.AlterFlowOperation, targets []mfmutator.Candidate, - moves []flowMove) (backend.MicroflowMutator, error) { + moves []flowMove, stated []model.Point) (backend.MicroflowMutator, error) { mut, err := ctx.Backend.OpenMicroflowForMutation(a.mf.ID) if err != nil { return nil, err @@ -75,6 +83,11 @@ func patch(ctx *ExecContext, a *alterFlowContext, ops []*ast.AlterFlowOperation, if err := move(); err != nil { return nil, err } + for _, p := range stated { + if err := mut.Pin(p); err != nil { + return nil, err + } + } if err := a.applyTo(ctx, mut, ops, targets); err != nil { return nil, err } @@ -138,6 +151,69 @@ func (pd *patchDiff) movedNode(d, s ast.MicroflowStatement) error { return nil } +// statedPoints are the canvas points the declared flow states a node is drawn +// at: each statement's @position, an if's @merge, the @start, every note's +// position and every parameter's. The splice pins whatever is drawn at one, so +// that making room for a fragment does not move a node away from where the +// script draws it (mendixlabs/mxcli#1598, case 6). A loop body is left out: +// its positions are relative to the loop box, and the splice moves no node +// inside one. +func statedPoints(d *flowDecl) []model.Point { + var out []model.Point + add := func(p *ast.Position) { + if p != nil { + out = append(out, model.Point{X: p.X, Y: p.Y}) + } + } + for _, p := range d.params { + add(p.Position) + } + var walk func(rv reflect.Value) + walk = func(rv reflect.Value) { + switch rv.Kind() { + case reflect.Interface: + if !rv.IsNil() { + walk(rv.Elem()) + } + case reflect.Pointer: + if rv.IsNil() { + return + } + switch v := rv.Interface().(type) { + case *ast.ActivityAnnotations: + add(v.Position) + add(v.Merge) + add(v.Start) + for _, notes := range [][]ast.MicroflowAnnotation{v.Notes, v.FreeNotes} { + for _, n := range notes { + add(n.Position) + } + } + return + case *ast.LoopStmt: + walk(reflect.ValueOf(v.Annotations)) + return + case *ast.WhileStmt: + walk(reflect.ValueOf(v.Annotations)) + return + } + walk(rv.Elem()) + case reflect.Struct: + for i := 0; i < rv.NumField(); i++ { + if rv.Type().Field(i).IsExported() { + walk(rv.Field(i)) + } + } + case reflect.Slice, reflect.Array: + for i := 0; i < rv.Len(); i++ { + walk(rv.Index(i)) + } + } + } + walk(reflect.ValueOf(d.body)) + return out +} + // redrawn is the refusal for a statement whose connectors the script draws // differently: the patch moves nodes, and does not reroute flows. func redrawn(st ast.MicroflowStatement) error { diff --git a/mdl/executor/flow_declared_match_test.go b/mdl/executor/flow_declared_match_test.go index 4e0882af7d..87b777ed5a 100644 --- a/mdl/executor/flow_declared_match_test.go +++ b/mdl/executor/flow_declared_match_test.go @@ -169,6 +169,37 @@ func TestLCSStatements_ExactMatchWinsOverAMove(t *testing.T) { } } +// mendixlabs/mxcli#1598, case 6: a new `if` inserted before a stored one that +// is restated with a change in its branch. The new if is the stored one but +// for its condition, and the restated if is the stored one but for its +// branch; both counted the same, and the tie went to the earlier pair, so the +// new if took over the stored decision (its condition set in place) and the +// restated if was inserted as a new node — on top of it, at the position the +// script states. The same condition is the closer match. +func TestLCSStatements_TheSameConditionWinsOverAChangedOne(t *testing.T) { + stored := parseFlowBody(t, ` @position(-500, 200) + log info node 'N' 'first'; + @position(-299, 200) + if $In = 'x' then + @position(-124, 100) + log info node 'N' 'a'; + end if;`).Body + declared := parseFlowBody(t, ` @position(-500, 200) + log info node 'N' 'first'; + if $In = 'y' then + log info node 'N' 'new'; + end if; + @position(-299, 200) + if $In = 'x' then + @position(-124, 100) + log info node 'N' 'b'; + end if;`).Body + pairs := lcsStatements(declared, stored) + if len(pairs) != 2 || pairs[0] != [2]int{0, 0} || pairs[1] != [2]int{2, 1} { + t.Fatalf("pairs %v, want [[0 0] [2 1]]: the restated if paired with the stored one, the new if inserted", pairs) + } +} + // mendixlabs/mxcli#1505: Studio Pro stores a multi-line note with CRLF line // breaks and describe prints those bytes as they are. An editor that turns // them into LF changed nothing anyone can see on the canvas, yet the free diff --git a/mdl/executor/flow_modify_stated_positions_1508_test.go b/mdl/executor/flow_modify_stated_positions_1508_test.go new file mode 100644 index 0000000000..9ad10bf23c --- /dev/null +++ b/mdl/executor/flow_modify_stated_positions_1508_test.go @@ -0,0 +1,258 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "bytes" + "strings" + "testing" + + "go.mongodb.org/mongo-driver/v2/bson" +) + +// mendixlabs/mxcli#1598 (ako/mxcli#1508), case 6, on a real project write. +// +// The flow is drawn with a @position on every statement. Restated with a new +// if/else after the declare and no @position on it, the splice made room by +// moving everything after the declare along by 339px — while the branch's two +// calls, replaced at the positions the script states, stayed where they were, +// left of their own decision: +// +// Modified nanoflow: MyFirstModule.NF_Do (spliced: 1 inserted, 1 replaced, 1 dropped) +// @position(40, 200) if $Flag was -299 +// @merge(525, 200) its merge was 186 +// @position(-124, 100) call javascript action … +// @position(869, 200) return was 530 +// +// and re-running the same script was refused ("the connectors of the If at +// (40, 200) are redrawn"). The same script with a plain `if` and no else put +// the new if on top of the stored decision. Either way the stored flow said +// something other than the script, so under mdl 1 the splice now refuses to +// move a node the script states, and the script that leaves those positions +// unstated is the control: it is applied, and its re-run is Unchanged. + +const nf1508Setup = `mdl 1; +create javascript action MyFirstModule.JS_Do(Msg: String) returns Boolean platform web as $$ +return Promise.resolve(true); +$$; +create microflow MyFirstModule.SUB_Check() returns Boolean as $R +begin + declare $R Boolean = true; + return $R; +end; +create nanoflow MyFirstModule.NF_Do ($Flag: Boolean) returns Boolean as $Out +begin + @position(-500, 200) + declare $Out Boolean = false; + @position(-299, 200) + @merge(186, 200) + if $Flag then + @position(-124, 100) + call javascript action MyFirstModule.JS_Do(Msg = 'a'); + @position(60, 100) + call microflow MyFirstModule.SUB_Check(); + end if; + @position(530, 200) + return $Out; +end; +` + +// nf1508Modify is the reported restatement: every @position kept, an if/else +// added after the declare without one, the JS argument changed and the call's +// result bound. +const nf1508Modify = `mdl 1; +create or modify nanoflow MyFirstModule.NF_Do ($Flag: Boolean) returns Boolean as $Out +begin + @position(-500, 200) + declare $Out Boolean = false; + if $Out then + log info node 'Probe' 'x'; + else + log info node 'Probe' 'y'; + end if; + @position(-299, 200) + @merge(186, 200) + if $Flag then + @position(-124, 100) + call javascript action MyFirstModule.JS_Do(Msg = 'b'); + @position(60, 100) + $Ok = call microflow MyFirstModule.SUB_Check(); + end if; + @position(530, 200) + return $Out; +end; +` + +func TestCreateOrModify_InsertDoesNotMoveStatedNodes1508(t *testing.T) { + // Each case starts from the flow as the setup draws it. + setup := func(t *testing.T) (*Executor, *bytes.Buffer, func() string) { + t.Helper() + exec, out := openPedAppFixture(t) + if err := agreeExec(t, exec, nf1508Setup); err != nil { + t.Fatalf("setup: %v\n%s", err, out.String()) + } + describe := func() string { + out.Reset() + if err := afRun(t, exec, "describe nanoflow MyFirstModule.NF_Do;"); err != nil { + return err.Error() + } + return out.String() + } + return exec, out, describe + } + refused := func(t *testing.T, src string) { + exec, out, describe := setup(t) + before := nf1508Unit(t, exec) + out.Reset() + err := agreeExec(t, exec, src) + said := out.String() + if err != nil { + said += err.Error() + } + if err == nil || !strings.Contains(said, "which the script draws where it is") || + !strings.Contains(said, "the ExclusiveSplit '$Flag' at (-299, 200)") { + t.Errorf("want a refusal naming the decision the script draws at (-299, 200), got %v\n%s", err, said) + } + if !bytes.Equal(nf1508Unit(t, exec), before) { + t.Errorf("the refused statement wrote the nanoflow:\n%s", describe()) + } + } + t.Run("the reported if/else", func(t *testing.T) { refused(t, nf1508Modify) }) + // A refusal is new meaning, so it applies under mdl 1 only (ADR-0011). + // Under mdl 0 the refused splice would fall back to rebuilding the whole + // flow — losing the merges, curves and $IDs the shift keeps — so a + // headerless script is spliced as it was. + t.Run("mdl 0 splices as before", func(t *testing.T) { + exec, out, _ := setup(t) + out.Reset() + if err := agreeExec(t, exec, strings.TrimPrefix(nf1508Modify, "mdl 1;\n")); err != nil { + t.Fatalf("%v\n%s", err, out.String()) + } + if said := out.String(); strings.Contains(said, "MDL-V1-REBUILD") || + !strings.Contains(said, "Modified nanoflow: MyFirstModule.NF_Do (spliced:") { + t.Errorf("want the headerless script spliced, not rebuilt, got:\n%s", said) + } + }) + // With no else the new if took over the stored decision (the same if but + // for its condition) and the restated `if $Flag` was inserted on top of it. + t.Run("a plain if", func(t *testing.T) { + refused(t, strings.Replace(nf1508Modify, " else\n log info node 'Probe' 'y';\n", "", 1)) + }) + + // The way out the refusal names: leave the positions after the insert + // unstated, so they can move along. Everything moves by the same amount — + // the replaced calls with their decision — and re-running reports Unchanged. + t.Run("control: no position stated after the insert", func(t *testing.T) { + exec, out, describe := setup(t) + free := nf1508Modify + for _, line := range []string{" @position(-299, 200)\n", " @merge(186, 200)\n", " @position(-124, 100)\n", + " @position(60, 100)\n", " @position(530, 200)\n"} { + free = strings.Replace(free, line, "", 1) + } + out.Reset() + if err := agreeExec(t, exec, free); err != nil { + t.Fatalf("%v\n%s", err, out.String()) + } + if !strings.Contains(out.String(), "Modified nanoflow: MyFirstModule.NF_Do") { + t.Errorf("want the nanoflow modified, got:\n%s", out.String()) + } + v := parseFlowView(t, nf1508Unit(t, exec)) + if v.objectAt(t, -500, 200, "Microflows$ActionActivity") == "" { + t.Errorf("the declare the script draws at (-500, 200) moved:\n%s", describe()) + } + // The parameter is not on the line the room was made on: it stays on + // its derived row (ako/mxcli#1547), where it moved to (539, 53). + for _, id := range v.order { + if o := v.objs[id]; afGet(o, "$Type") == "Microflows$MicroflowParameter" { + if x, y := afPoint(o); x != 200 || y != 53 { + t.Errorf("$Flag moved to (%d, %d), want it left at (200, 53):\n%s", x, y, describe()) + } + } + } + split, merge, branch := nf1508Branch(t, v, "$Flag") + if len(branch) != 2 { + t.Fatalf("`if $Flag` has %d activities in its branch, want 2:\n%s", len(branch), describe()) + } + for _, x := range branch { + if x <= split || x >= merge { + t.Errorf("a branch activity of `if $Flag` is at x=%d, outside its decision (%d) and merge (%d):\n%s", + x, split, merge, describe()) + } + } + out.Reset() + if err := agreeExec(t, exec, free); err != nil { + t.Fatalf("re-run: %v\n%s", err, out.String()) + } + if !strings.Contains(out.String(), "Unchanged nanoflow: MyFirstModule.NF_Do") { + t.Errorf("re-run: want Unchanged, got:\n%s", out.String()) + } + }) +} + +// nf1508Unit returns MyFirstModule.NF_Do's stored bytes. +func nf1508Unit(t *testing.T, exec *Executor) []byte { + t.Helper() + ctx := exec.newExecContext(t.Context()) + h, err := getHierarchy(ctx) + if err != nil { + t.Fatal(err) + } + all, err := ctx.Backend.ListNanoflows() + if err != nil { + t.Fatal(err) + } + for _, n := range all { + if n.Name == "NF_Do" && h.GetModuleName(h.FindModuleID(n.ContainerID)) == "MyFirstModule" { + raw, err := ctx.Backend.GetRawUnitBytes(n.ID) + if err != nil { + t.Fatal(err) + } + return append([]byte(nil), raw...) + } + } + t.Fatal("MyFirstModule.NF_Do not found") + return nil +} + +// nf1508Branch returns the x of the decision on cond, of the merge its paths +// join at, and of every activity on those paths. +func nf1508Branch(t *testing.T, v flowView, cond string) (split, merge int, branch []int) { + t.Helper() + var splitID string + for _, id := range v.order { + o := v.objs[id] + if afGet(o, "$Type") != "Microflows$ExclusiveSplit" { + continue + } + if sc, _ := afGet(o, "SplitCondition").(bson.D); sc != nil && afGet(sc, "Expression") == cond { + splitID = id + split, _ = afPoint(o) + } + } + if splitID == "" { + t.Fatalf("no decision on %s", cond) + } + seen := map[string]bool{splitID: true} + for frontier := []string{splitID}; len(frontier) > 0; frontier = frontier[1:] { + for _, f := range v.flows { + dest := afIDOf(afGet(f, "DestinationPointer")) + if afIDOf(afGet(f, "OriginPointer")) != frontier[0] || seen[dest] { + continue + } + seen[dest] = true + o := v.objs[dest] + x, _ := afPoint(o) + switch afGet(o, "$Type") { + case "Microflows$ExclusiveMerge": + merge = x + case "Microflows$ActionActivity": + branch = append(branch, x) + frontier = append(frontier, dest) + } + } + } + if merge == 0 { + t.Fatalf("the decision on %s reaches no merge", cond) + } + return split, merge, branch +} diff --git a/mdl/executor/validate.go b/mdl/executor/validate.go index 43a994e59a..659f2b8284 100644 --- a/mdl/executor/validate.go +++ b/mdl/executor/validate.go @@ -182,6 +182,31 @@ func (sc *scriptContext) recordEntityAttrs(s *ast.CreateEntityStmt) { // switches over the same statement types, so a case added to one and not the // other is collected on one path only — which is how this rule first shipped // firing against stored associations but not script-declared ones. +func (sc *scriptContext) recordViewAssociations(s *ast.CreateViewEntityStmt) { + // A view entity's associations are its `.ID as ` columns: the + // view entity is the FROM end, the selected entity the TO end, and the + // association lives in the view's module (oql_view_associations.go). + // + // They are registered exactly as a script's CREATE ASSOCIATION is, so every + // checker that resolves association names sees them. Without that, the + // constraint `[M.Stats_Customer = $currentObject]` on the view entity — the + // remedy MDL-ASSOCDS02 names — was refused as naming "neither an attribute + // nor an association" whenever the view came from the same script. + for _, col := range viewAssociationColumns(s.Query.RawQuery) { + to := parseQN(col.Entity) + if to.Module == "" { + continue + } + name := ast.QualifiedName{Module: s.Name.Module, Name: col.Name} + sc.recordAssociation(&ast.CreateAssociationStmt{ + Name: name, Parent: s.Name, Child: to, Type: ast.AssocReference, Owner: ast.OwnerDefault, + }) + shape := sc.associationShapes[strings.ToLower(name.String())] + shape.view = true + sc.associationShapes[strings.ToLower(name.String())] = shape + } +} + func (sc *scriptContext) recordAssociation(s *ast.CreateAssociationStmt) { if s.Name.Module == "" || s.Name.Name == "" { return @@ -246,6 +271,7 @@ func (sc *scriptContext) collectSingle(stmt ast.Statement) { if s.Name.Module != "" { sc.entities[s.Name.String()] = true sc.viewEntities[s.Name.String()] = true + sc.recordViewAssociations(s) } case *ast.CreateExternalEntityStmt: if s.Name.Module != "" { diff --git a/mdl/executor/validate_alter_page_refs.go b/mdl/executor/validate_alter_page_refs.go index 0f955d2989..9c2d3b6df2 100644 --- a/mdl/executor/validate_alter_page_refs.go +++ b/mdl/executor/validate_alter_page_refs.go @@ -124,7 +124,11 @@ func validateAlterPageReferences(ctx *ExecContext, prog *ast.Program, sc *script // The targets, against the stored document, in the order exec // resolves them. + itemSetEarlier := map[string]bool{} for _, op := range s.Operations { + if set, ok := op.(*ast.SetPropertyOp); ok && set.Target.Item != nil { + itemSetEarlier[set.Target.Widget] = true + } // A SET's target is validateAlterSetProperties' finding. if _, isSet := op.(*ast.SetPropertyOp); mutator != nil && !isSet { for _, ref := range alterPageOperationTargets(op) { @@ -136,6 +140,15 @@ func validateAlterPageReferences(ctx *ExecContext, prog *ast.Program, sc *script } if _, err := mutator.ResolveAlterTarget(alterTargetOf(ref)); err != nil { errs = append(errs, mdlerrors.NewValidationf("%s: %v", label, err)) + } else if !itemSetEarlier[ref.Widget] { + // An earlier SET on the item in this statement may be what + // makes the slot visible (`set (headerRenderMode: 'custom')`, + // the very step the refusal recommends). This pass reads the + // stored document, so it cannot see that; exec checks again + // against the values after the SET. + if err := refuseHiddenItemSlotFor(ctx, mutator, op, ref); err != nil { + errs = append(errs, mdlerrors.NewValidationf("%s: %v", label, err)) + } } } } diff --git a/mdl/executor/validate_alter_set.go b/mdl/executor/validate_alter_set.go index 0700d008ff..0f85f9403d 100644 --- a/mdl/executor/validate_alter_set.go +++ b/mdl/executor/validate_alter_set.go @@ -171,7 +171,7 @@ func checkSetOp(ctx *ExecContext, p pageProbe, label string, op *ast.SetProperty // fine one property at a time and refused as a whole, since the width hides // the size, and `ColumnWidth: manual, Size: 220` is the reverse — judged // one at a time, Size alone would be hidden (mendixlabs/mxcli#1519). - if len(errs) == 0 && op.Target.IsColumn() { + if len(errs) == 0 && (op.Target.IsColumn() || op.Target.Item != nil) { if probe, err := p.Probe(); err == nil { if err := applySetPropertyMutator(ctx, probe, op, modName, modID); err != nil && !scriptDeclaresMissing(sc, err, modName) { errs = append(errs, mdlerrors.NewValidation(fmt.Sprintf("%s: %v", label, err))) diff --git a/mdl/executor/validate_alter_set_test.go b/mdl/executor/validate_alter_set_test.go index e40d0a93ef..654abb69a7 100644 --- a/mdl/executor/validate_alter_set_test.go +++ b/mdl/executor/validate_alter_set_test.go @@ -72,6 +72,8 @@ type countingDeps struct{ saves int } func (d *countingDeps) SerializeWidget(pages.Widget) bson.D { return nil } func (d *countingDeps) SerializeClientAction(pages.ClientAction) bson.D { return nil } +func (d *countingDeps) SerializeClientTemplate() bson.D { return nil } + func (d *countingDeps) SerializeCustomWidgetDataSource(pages.DataSource) bson.D { return nil } diff --git a/mdl/executor/validate_assoc_list_source.go b/mdl/executor/validate_assoc_list_source.go index a9b7c35888..456a759caa 100644 --- a/mdl/executor/validate_assoc_list_source.go +++ b/mdl/executor/validate_assoc_list_source.go @@ -36,6 +36,10 @@ type assocShape struct { from, to string refSet bool owner string // "Default", "Both", or "" when not one of the measured two + // view marks an association a view entity derives from its OQL (an + // OqlViewAssociationSource): `from` is the view entity, and a page cannot + // follow it from `to` (MDL-ASSOCDS02, validate_view_assoc_reverse.go). + view bool } func astOwnerName(o ast.OwnerType) string { @@ -91,14 +95,16 @@ func checkAssociationShapes(ctx *ExecContext, sc *scriptContext) map[string]asso if from, to := byID[a.ParentID], byID[a.ChildID]; from != "" && to != "" { qn := mod + "." + a.Name out[strings.ToLower(qn)] = assocShape{qn: qn, from: from, to: to, - refSet: a.Type == domainmodel.AssociationTypeReferenceSet, owner: sdkOwnerName(a.Owner)} + refSet: a.Type == domainmodel.AssociationTypeReferenceSet, owner: sdkOwnerName(a.Owner), + view: a.Source == domainmodel.OqlViewAssociationSource} } } for _, a := range dm.CrossAssociations { if from := byID[a.ParentID]; from != "" && a.ChildRef != "" { qn := mod + "." + a.Name out[strings.ToLower(qn)] = assocShape{qn: qn, from: from, to: a.ChildRef, - refSet: a.Type == domainmodel.AssociationTypeReferenceSet, owner: sdkOwnerName(a.Owner)} + refSet: a.Type == domainmodel.AssociationTypeReferenceSet, owner: sdkOwnerName(a.Owner), + view: a.Source == domainmodel.OqlViewAssociationSource} } } } diff --git a/mdl/executor/validate_view_assoc_reverse.go b/mdl/executor/validate_view_assoc_reverse.go new file mode 100644 index 0000000000..d45951001c --- /dev/null +++ b/mdl/executor/validate_view_assoc_reverse.go @@ -0,0 +1,113 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "fmt" + "strings" + + "github.com/mendixlabs/mxcli/mdl/ast" +) + +// MDL-ASSOCDS02 — a page data source that follows a view entity's association +// from the entity it points at. +// +// A view entity's association (an `.ID as ` column of its OQL) is +// one-way for a page: the persistent entity at its far end does not own it. +// mxbuild refuses a data source that follows it from there with CE8812 +// "Entity 'M.Customer' has no association 'M.Stats_Customer'". `check` passed +// it and exec wrote it (mendixlabs/mxcli#1611 A). +// +// Measured on mxbuild 11.14.0, one page per shape: +// +// DATABASE FROM $currentObject/M.Stats_Customer/M.Stats list view, data grid, gallery → CE8812 +// $currentObject/M.Stats_Customer list view, data view → CE8812 +// … the same hop second in a path (from an Item) → CE8812 +// the same, followed from the view entity → ok +// DATABASE M.Stats WHERE [M.Stats_Customer = $currentObject] → ok +// a microflow `retrieve $L from $Cust/M.Stats_Customer` → ok (not judged) +// +// The walk follows each hop from the widget's context entity, so it judges only +// a path it can place: an unknown association or an entity it cannot follow +// ends the walk silently. +func (v *attributeScopeValidator) checkViewAssocReverse(w *ast.WidgetV3, enclosing dataContext) { + if v.assocs == nil { + return + } + ds := w.GetDataSource() + if ds == nil { + return + } + var path string + switch { + case ds.Type == "association": + path = ds.Reference + case ds.Type == "database" && ds.AssociationPath != "": + path = ds.AssociationPath + default: + return + } + cur := v.dataSourceContextEntity(ds, enclosing) + if cur == "" || path == "" { + return + } + for _, seg := range strings.Split(path, "/") { + seg = strings.TrimSpace(seg) + if seg == "" { + return + } + qn := seg + if !strings.Contains(qn, ".") { + mod, _, _ := strings.Cut(cur, ".") + qn = mod + "." + qn + } + shape, isAssoc := v.assocs[strings.ToLower(qn)] + if !isAssoc { + // An entity step (`/M.Stats` after the association) names where the + // hop arrived; anything else ends what can be judged. + if strings.EqualFold(qn, cur) { + continue + } + return + } + switch { + case strings.EqualFold(cur, shape.from): + cur = shape.to + case strings.EqualFold(cur, shape.to): + if shape.view { + v.errs = append(v.errs, fmt.Sprintf( + "%s data source %s: %s is an association of view entity %s, and a page cannot follow it from %s — "+ + "mxbuild rejects this with CE8812 \"Entity '%s' has no association '%s'\". Query the view entity "+ + "instead: DATABASE %s WHERE [%s = $currentObject] [MDL-ASSOCDS02]", + widgetLabel(w.Name, w.Type), dataSourceDisplay(ds), shape.qn, shape.from, cur, + cur, shape.qn, shape.from, shape.qn)) + return + } + cur = shape.from + default: + return + } + } +} + +// dataSourceContextEntity is the entity a context-relative data source starts +// from: the enclosing data container's for $currentObject, else the page or +// snippet parameter's. "" when it cannot be established. +func (v *attributeScopeValidator) dataSourceContextEntity(ds *ast.DataSourceV3, enclosing dataContext) string { + switch cv := strings.ToLower(ds.ContextVariable); cv { + case "", "currentobject": + if enclosing.unresolved || len(enclosing.entities) == 0 { + return "" + } + return enclosing.entities[len(enclosing.entities)-1] + default: + return v.pageParams[cv] + } +} + +func dataSourceDisplay(ds *ast.DataSourceV3) string { + if ds.Type == "database" { + return "DATABASE FROM $" + ds.ContextVariable + "/" + ds.AssociationPath + } + return dataSourceText(ds) +} diff --git a/mdl/executor/validate_view_assoc_reverse_test.go b/mdl/executor/validate_view_assoc_reverse_test.go new file mode 100644 index 0000000000..2b4069a237 --- /dev/null +++ b/mdl/executor/validate_view_assoc_reverse_test.go @@ -0,0 +1,144 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/backend/mock" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/domainmodel" +) + +// viewAssocCtx is a project with a persistent C1611.Customer, a C1611.Item +// pointing at it, and a view entity C1611.Stats whose OQL selects the +// customer's id as Stats_Customer — the association Mendix derives from it. +func viewAssocCtx(t *testing.T) *ExecContext { + t.Helper() + mod := &model.Module{BaseElement: model.BaseElement{ID: "mod-1611"}, Name: "C1611"} + b := &mock.MockBackend{ + IsConnectedFunc: func() bool { return true }, + ListModulesFunc: func() ([]*model.Module, error) { return []*model.Module{mod}, nil }, + ListDomainModelsFunc: func() ([]*domainmodel.DomainModel, error) { + return []*domainmodel.DomainModel{{ + BaseElement: model.BaseElement{ID: "dm-1611"}, ContainerID: mod.ID, + Entities: []*domainmodel.Entity{ + {BaseElement: model.BaseElement{ID: "e-cust"}, Name: "Customer", Persistable: true}, + {BaseElement: model.BaseElement{ID: "e-item"}, Name: "Item", Persistable: true}, + {BaseElement: model.BaseElement{ID: "e-stats"}, Name: "Stats", Persistable: true, + Source: "DomainModels$OqlViewEntitySource"}, + }, + Associations: []*domainmodel.Association{ + {BaseElement: model.BaseElement{ID: "a-item"}, Name: "Item_Customer", ParentID: "e-item", ChildID: "e-cust", + Type: domainmodel.AssociationTypeReference, Owner: domainmodel.AssociationOwnerDefault}, + {BaseElement: model.BaseElement{ID: "a-stats"}, Name: "Stats_Customer", ParentID: "e-stats", ChildID: "e-cust", + Type: domainmodel.AssociationTypeReference, Owner: domainmodel.AssociationOwnerDefault, + Source: domainmodel.OqlViewAssociationSource, ViewSourceReference: "Stats_Customer"}, + }, + }}, nil + }, + } + ctx, _ := newMockCtx(t, withBackend(b)) + ctx.widgetRegistry = &WidgetRegistry{byMDLName: map[string]*WidgetDefinition{}, byWidgetID: map[string]*WidgetDefinition{}} + ctx.widgetRegistryLoaded = true + return ctx +} + +func viewAssocPage(param, widget, ds string) string { + return `create page C1611.P (title: 'P', layout: Atlas_Core.Atlas_Default, params: ($P: C1611.` + param + `)) { + dataview dv (datasource: $P) { + ` + widget + ` w (datasource: ` + ds + `) { } + } +}` +} + +// mendixlabs/mxcli#1611 A: a page data source that follows a view entity's +// association from the entity it points at passed check and exec, then mxbuild +// failed CE8812 "Entity 'M.Owner' has no association 'M.Stats_Owner'". Each +// case is one measured shape (11.14.0, one page per shape); the controls built +// at 0 errors. +func TestViewAssociationReverse_MeasuredShapes(t *testing.T) { + ctx := viewAssocCtx(t) + for _, tc := range []struct { + name, param, widget, ds string + ce8812 bool + }{ + {"database from, the report", "Customer", "listview", "DATABASE FROM $currentObject/C1611.Stats_Customer/C1611.Stats", true}, + {"in-memory association source", "Customer", "listview", "$currentObject/C1611.Stats_Customer", true}, + {"data grid", "Customer", "datagrid", "DATABASE FROM $currentObject/C1611.Stats_Customer/C1611.Stats", true}, + {"gallery", "Customer", "gallery", "DATABASE FROM $currentObject/C1611.Stats_Customer/C1611.Stats", true}, + {"data view", "Customer", "dataview", "$currentObject/C1611.Stats_Customer", true}, + {"second hop", "Item", "listview", "DATABASE FROM $currentObject/C1611.Item_Customer/C1611.Customer/C1611.Stats_Customer/C1611.Stats", true}, + {"unqualified association", "Customer", "listview", "$currentObject/Stats_Customer", true}, + // controls + {"persistent association reversed", "Customer", "listview", "DATABASE FROM $currentObject/C1611.Item_Customer/C1611.Item", false}, + {"view association forward", "Stats", "dataview", "$currentObject/C1611.Stats_Customer", false}, + {"forward then a persistent reverse", "Stats", "listview", "DATABASE FROM $currentObject/C1611.Stats_Customer/C1611.Customer/C1611.Item_Customer/C1611.Item", false}, + {"the reporter's workaround", "Customer", "listview", "DATABASE C1611.Stats WHERE [C1611.Stats_Customer = $currentObject]", false}, + } { + t.Run(tc.name, func(t *testing.T) { + errs := assocListErrs(t, ctx, viewAssocPage(tc.param, tc.widget, tc.ds)) + var hits []string + for _, e := range errs { + if strings.Contains(e, "MDL-ASSOCDS02") { + hits = append(hits, e) + } + } + if !tc.ce8812 { + if len(errs) != 0 { + t.Errorf("want no error, got %q", errs) + } + return + } + if len(hits) != 1 || len(errs) != 1 { + t.Fatalf("want one MDL-ASSOCDS02, got %q", errs) + } + for _, want := range []string{"CE8812", "C1611.Stats_Customer", "view entity C1611.Stats", + "DATABASE C1611.Stats WHERE [C1611.Stats_Customer = "} { + if !strings.Contains(hits[0], want) { + t.Errorf("want %q in %q", want, hits[0]) + } + } + }) + } +} + +// The reporter's flow creates the view entity and the page in one script: the +// association comes from the view's OQL, not from a CREATE ASSOCIATION. +func TestViewAssociationReverse_ScriptDeclaredView(t *testing.T) { + src := `create view entity C1611.Totals (Total: Integer) as ( + from C1611.Item as i + join i/C1611.Item_Customer/C1611.Customer as c + group by c.ID + select c.ID as Totals_Customer, count(i.ID) as Total +); +` + viewAssocPage("Customer", "listview", "DATABASE FROM $currentObject/C1611.Totals_Customer/C1611.Totals") + errs := assocListErrs(t, viewAssocCtx(t), src) + if len(errs) != 1 || !strings.Contains(errs[0], "MDL-ASSOCDS02") || !strings.Contains(errs[0], "C1611.Totals_Customer") { + t.Errorf("want one MDL-ASSOCDS02 for the script's view association, got %q", errs) + } +} + +// The remedy MDL-ASSOCDS02 names — query the view entity, constrained on its +// association — must itself pass when the view entity is created by the same +// script. It did not: the script's view associations were known to no checker, +// so `[C1611.Totals_Customer = $currentObject]` was refused as naming "neither +// an attribute nor an association", on a page and in a microflow retrieve, +// while mxbuild builds both at 0 errors (measured, 11.14.0). +func TestViewAssociationReverse_RemedyPassesForScriptDeclaredView(t *testing.T) { + src := `create view entity C1611.Totals (Total: Integer) as ( + from C1611.Item as i + join i/C1611.Item_Customer/C1611.Customer as c + group by c.ID + select c.ID as Totals_Customer, count(i.ID) as Total +); +` + viewAssocPage("Customer", "listview", "DATABASE C1611.Totals WHERE [C1611.Totals_Customer = $currentObject]") + `; +create microflow C1611.MF ($C: C1611.Customer) begin retrieve $L from C1611.Totals where C1611.Totals_Customer = $C; end;` + prog := parseMDL(t, src) + for _, err := range validateProgram(viewAssocCtx(t), prog) { + if strings.Contains(err.Error(), "Totals_Customer") { + t.Errorf("the remedy was refused: %v", err) + } + } +} diff --git a/mdl/executor/validate_widget_attribute_scope.go b/mdl/executor/validate_widget_attribute_scope.go index 877889afd5..e4a46fa189 100644 --- a/mdl/executor/validate_widget_attribute_scope.go +++ b/mdl/executor/validate_widget_attribute_scope.go @@ -114,6 +114,7 @@ func (v *attributeScopeValidator) walk(w *ast.WidgetV3, enclosing dataContext) { inner := childContext(enclosing, w.GetDataSource(), v.sigs, v.pageParams) v.checkInputBinding(w, enclosing) v.checkAssocListSource(w, enclosing) + v.checkViewAssocReverse(w, enclosing) v.checkReactUnsupported(w) if v.registry == nil { // No registry: the pluggable half cannot run, and a pluggable widget's diff --git a/mdl/executor/validate_widget_embedded_schema.go b/mdl/executor/validate_widget_embedded_schema.go index 1e8737a83b..116f058df0 100644 --- a/mdl/executor/validate_widget_embedded_schema.go +++ b/mdl/executor/validate_widget_embedded_schema.go @@ -44,6 +44,9 @@ type embeddedWidgetSchema struct { // types maps a lower-cased key to its declared type ("Expression", // "TextTemplate", "Boolean", …). types map[string]string + // returns maps a lower-cased Expression key to its declared return type + // ("String", "Boolean", …); absent when the template declares none. + returns map[string]string // slotAccepts are the widget types an item's widgets slot routes, by slot // key (a column's `filter` → textfilter, …); empty for a widget's own schema. slotAccepts map[string][]string @@ -143,7 +146,7 @@ func loadEmbeddedSchema(widgetID, listKey string) *embeddedWidgetSchema { var schema *embeddedWidgetSchema if tmpl, err := widgets.GetTemplate(widgetID); err == nil && tmpl != nil { if propTypes := embeddedTemplatePropertyTypes(tmpl.Type, listKey); len(propTypes) > 0 { - schema = &embeddedWidgetSchema{Title: tmpl.Name, Version: tmpl.Version, types: map[string]string{}} + schema = &embeddedWidgetSchema{Title: tmpl.Name, Version: tmpl.Version, types: map[string]string{}, returns: map[string]string{}} if listKey != "" { schema.Title += " column" schema.slotAccepts = itemSlotAcceptedChildTypes[widgetID][listKey] @@ -156,6 +159,11 @@ func loadEmbeddedSchema(widgetID, listKey string) *embeddedWidgetSchema { kind := "" if vt, ok := pt["ValueType"].(map[string]any); ok { kind, _ = vt["Type"].(string) + if rt, ok := vt["ReturnType"].(map[string]any); ok && kind == "Expression" { + if ret, _ := rt["Type"].(string); ret != "" { + schema.returns[strings.ToLower(key)] = ret + } + } } schema.Names = append(schema.Names, key) schema.types[strings.ToLower(key)] = kind diff --git a/mdl/executor/validate_widget_expression_return.go b/mdl/executor/validate_widget_expression_return.go new file mode 100644 index 0000000000..f4b374a427 --- /dev/null +++ b/mdl/executor/validate_widget_expression_return.go @@ -0,0 +1,114 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "fmt" + "regexp" + "sort" + "strings" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/exprcheck" + "github.com/mendixlabs/mxcli/mdl/linter" +) + +// validateWidgetExpressionReturnTypes is MDL-WIDGET49: a pluggable widget's +// Expression property, declared to return a String, given an expression of +// another kind. +// +// A quoted MDL value on an Expression property IS the expression's text, so +// `dropdownfilter ddf (defaultValue: 'true')` stores the expression `true` — a +// Boolean where the drop-down filter's `defaultValue` returns a String. check +// and exec passed it and mxbuild failed CE0117 "Error(s) in expression." +// (mendixlabs/mxcli#1611 B). Measured on 11.14.0, one filter per value: +// +// 'true', 'false', '1', '1.5', 'abc' → CE0117 (Boolean, Integer, Decimal, a bare name) +// '''true''', '''abc''', 'empty' → ok +// +// Only what is certain is judged: a String-returning property, and a value whose +// kind is a literal Boolean, Integer or Decimal, or that is one bare name (which +// is no Mendix expression at all). Anything that infers no kind — an attribute +// path, a variable, a function the checker does not type — is left alone. +func validateWidgetExpressionReturnTypes(w *ast.WidgetV3, def *WidgetDefinition, + embedded *embeddedWidgetSchema, locationPrefix string) []linter.Violation { + if w == nil || len(w.Properties) == 0 { + return nil + } + schema := embedded + if def != nil { + schema = loadEmbeddedSchema(def.WidgetID, "") + } + if schema == nil || len(schema.returns) == 0 { + return nil + } + keys := make([]string, 0, len(w.Properties)) + for k := range w.Properties { + keys = append(keys, k) + } + sort.Strings(keys) + + var out []linter.Violation + for _, key := range keys { + lk := strings.ToLower(key) + if schema.types[lk] != "Expression" || schema.returns[lk] != "String" { + continue + } + var text string + switch v := w.Properties[key].(type) { + case string: + text = v + case *ast.WidgetExpressionV3: + text = v.Text + default: + continue + } + text = strings.TrimSpace(text) + got := "" + switch { + case bareExpressionNameRe.MatchString(text) && !isExpressionKeyword(text): + got = "a bare name, which is no expression" + default: + switch exprcheck.InferSourceKind(text, nil) { + case exprcheck.KindBoolean: + got = "a Boolean" + case exprcheck.KindInteger, exprcheck.KindLong: + got = "an Integer" + case exprcheck.KindDecimal: + got = "a Decimal" + } + } + if got == "" { + continue + } + out = append(out, linter.Violation{ + RuleID: "MDL-WIDGET49", + Severity: linter.SeverityError, + Message: fmt.Sprintf( + "%s: widget `%s` property `%s` is an expression returning a String, and `%s` is %s — "+ + "mxbuild rejects it with CE0117 \"Error(s) in expression.\" A quoted value on an expression "+ + "property is the expression's text, not a string", + locationPrefix, widgetLabelForError(w), key, text, got), + Suggestion: fmt.Sprintf("Write the value as a String literal inside the quotes: `%s: %s`.", + key, mdlStringOfExpressionLiteral(text)), + }) + } + return out +} + +var bareExpressionNameRe = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]*$`) + +func isExpressionKeyword(s string) bool { + switch strings.ToLower(s) { + case "true", "false", "empty": + return true + } + return false +} + +// mdlStringOfExpressionLiteral is the MDL spelling of the expression 'text': the +// Mendix string literal, quoted again for MDL with every apostrophe doubled. +func mdlStringOfExpressionLiteral(text string) string { + literal := "'" + strings.ReplaceAll(text, "'", "''") + "'" + return "'" + strings.ReplaceAll(literal, "'", "''") + "'" +} diff --git a/mdl/executor/validate_widget_expression_return_test.go b/mdl/executor/validate_widget_expression_return_test.go new file mode 100644 index 0000000000..8421fb0567 --- /dev/null +++ b/mdl/executor/validate_widget_expression_return_test.go @@ -0,0 +1,76 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" +) + +// mendixlabs/mxcli#1611 B: `dropdownfilter ddf (defaultValue: 'true')` in a +// Boolean column passed check and exec, then mxbuild failed CE0117 "Error(s) in +// expression." The property is an Expression returning String, and a quoted MDL +// value IS the expression text, so `'true'` stores the expression `true` — a +// Boolean. Each case is one measured value (11.14.0, one filter per value, the +// column type beside it); the controls built at 0 errors. +func TestWidgetExpressionReturnType_DropdownFilterDefault(t *testing.T) { + for _, tc := range []struct { + name, attr, value string + ce0117 bool + }{ + {"the report: Boolean literal", "IsActive", `'true'`, true}, + {"false", "IsActive", `'false'`, true}, + {"bare name", "Label", `'abc'`, true}, + {"Integer literal", "Label", `'1'`, true}, + {"Integer literal in an Integer column", "Size", `'1'`, true}, + {"Decimal literal", "Label", `'1.5'`, true}, + // controls + {"the workaround: a String literal", "IsActive", `'''true'''`, false}, + {"String literal", "Label", `'''abc'''`, false}, + {"a String matching no option", "IsActive", `'''tru'''`, false}, + {"empty", "Label", `'empty'`, false}, + {"a String expression", "Label", `'''a'' + ''b'''`, false}, + } { + t.Run(tc.name, func(t *testing.T) { + src := `create page F.P (Title: 'P', Layout: Atlas_Core.Atlas_Default) { + datagrid dg (DataSource: DATABASE F.Thing) { + column (Attribute: ` + tc.attr + `) { dropdownfilter ddf (defaultValue: ` + tc.value + `) } + } +};` + var hits []string + for _, v := range ValidateWidgetProperties(parseMDL(t, src), "") { + if v.RuleID == "MDL-WIDGET49" { + hits = append(hits, v.Message+" | "+v.Suggestion) + } + } + if !tc.ce0117 { + if len(hits) != 0 { + t.Errorf("want no MDL-WIDGET49, got %q", hits) + } + return + } + if len(hits) != 1 { + t.Fatalf("want one MDL-WIDGET49, got %q", hits) + } + for _, want := range []string{"defaultValue", "String", "CE0117"} { + if !strings.Contains(hits[0], want) { + t.Errorf("want %q in %q", want, hits[0]) + } + } + }) + } +} + +// The hint gives the spelling that builds: the value as a String literal. +func TestWidgetExpressionReturnType_HintQuotesTheLiteral(t *testing.T) { + src := `create page F.P (Title: 'P', Layout: Atlas_Core.Atlas_Default) { + datagrid dg (DataSource: DATABASE F.Thing) { + column (Attribute: IsActive) { dropdownfilter ddf (defaultValue: 'true') } + } +};` + for _, v := range ValidateWidgetProperties(parseMDL(t, src), "") { + if v.RuleID == "MDL-WIDGET49" && !strings.Contains(v.Suggestion, `defaultValue: '''true'''`) { + t.Errorf("hint does not give the String literal: %q", v.Suggestion) + } + } +} diff --git a/mdl/executor/validate_widgets.go b/mdl/executor/validate_widgets.go index 0693d99bdc..a6d82e32d3 100644 --- a/mdl/executor/validate_widgets.go +++ b/mdl/executor/validate_widgets.go @@ -337,6 +337,8 @@ func validateWidgetTreeIn(widgets []*ast.WidgetV3, registry *WidgetRegistry, loc out = append(out, validateWidgetExpressionValues(w, nil, mapping, true, locationPrefix)...) } else { out = append(out, validateWidgetExpressionValuesIn(w, def, nil, isObjectListItem, embedded, locationPrefix)...) + // An Expression property's value of the wrong kind (MDL-WIDGET49). + out = append(out, validateWidgetExpressionReturnTypes(w, def, embedded, locationPrefix)...) } // A generic widget type that resolved to nothing is already reported as // MDL-WIDGET25 (the kind is wrong). Validating its properties on top of diff --git a/mdl/executor/widget_engine.go b/mdl/executor/widget_engine.go index ecc3d17a88..5b268134d3 100644 --- a/mdl/executor/widget_engine.go +++ b/mdl/executor/widget_engine.go @@ -2419,24 +2419,8 @@ func defaultItemSlotKey(mapping *ObjectListMapping) string { return bodyItemSlotKey(keys) } -// bodyItemSlotKey picks an object-list item's BODY among its widget slots -// (property keys, in schema order): `content` when the item has one, else the -// first. CREATE routes an item's loose children there; DESCRIBE prints that -// slot as the item's children and every other slot as a ` { … }` -// block. One function, so the two cannot disagree about which slot loose -// widgets belong to — they did, and an Accordion group's headerContent -// round-tripped into its content (mendixlabs/mxcli#1453). -func bodyItemSlotKey(keys []string) string { - for _, k := range keys { - if k == "content" { - return k - } - } - if len(keys) == 0 { - return "" - } - return keys[0] -} +// bodyItemSlotKey is types.BodyItemSlotKey, which the page mutator reads too. +func bodyItemSlotKey(keys []string) string { return types.BodyItemSlotKey(keys) } // lookupProperty does a case-insensitive lookup against an AST property map. // MDL property names ignore case (e.g. `HeaderText` and `headerText` both match). diff --git a/mdl/grammar/MDLParser.g4 b/mdl/grammar/MDLParser.g4 index 63fda217df..b85e3eacb9 100644 --- a/mdl/grammar/MDLParser.g4 +++ b/mdl/grammar/MDLParser.g4 @@ -441,10 +441,16 @@ alterFragment // holding the widget named t1, `nav item('Home')` for the one whose text is // Home, and `nav item@2` for the second item. With a selector, @n chooses among // the items that match it. +// +// The same address reaches a pluggable widget's object-list item — an +// Accordion group, a Pop-up menu item — which stores no name either +// (PROPOSAL_alter_object_list_item_address.md): `acc1 item@1` is the group, +// INSERT INTO it fills its body slot, and `acc1 item@1.headercontent` names +// another of its slots, as `grid.controlbar` names a widget's. alterTarget : identifierOrKeyword COLUMN LPAREN (attributePathV3 | STRING_LITERAL) RPAREN (AT NUMBER_LITERAL)? - | identifierOrKeyword ITEM LPAREN (identifierOrKeyword | STRING_LITERAL) RPAREN (AT NUMBER_LITERAL)? - | identifierOrKeyword ITEM AT NUMBER_LITERAL + | identifierOrKeyword ITEM LPAREN (identifierOrKeyword | STRING_LITERAL) RPAREN (AT NUMBER_LITERAL)? (DOT itemSlot=identifierOrKeyword)? + | identifierOrKeyword ITEM AT NUMBER_LITERAL (DOT itemSlot=identifierOrKeyword)? | identifierOrKeyword (DOT identifierOrKeyword)? (AT NUMBER_LITERAL)? | STRING_LITERAL (AT NUMBER_LITERAL)? ; diff --git a/mdl/types/widget_slot_keywords.go b/mdl/types/widget_slot_keywords.go index 6429503823..9c75bafd12 100644 --- a/mdl/types/widget_slot_keywords.go +++ b/mdl/types/widget_slot_keywords.go @@ -42,3 +42,23 @@ func WidgetSlotKeyword(widgetID, propertyKey string) string { } return strings.ToUpper(propertyKey) } + +// BodyItemSlotKey picks an object-list item's BODY among its widget slots +// (property keys, in schema order): `content` when the item has one, else the +// first. CREATE routes an item's loose children there; DESCRIBE prints that +// slot as the item's children and every other slot as a ` { … }` +// block; ALTER PAGE writes there for a bare item address (`acc1 item@1`). One +// function, so the three cannot disagree about which slot loose widgets belong +// to — two of them did, and an Accordion group's headerContent round-tripped +// into its content (mendixlabs/mxcli#1453). +func BodyItemSlotKey(keys []string) string { + for _, k := range keys { + if k == "content" { + return k + } + } + if len(keys) == 0 { + return "" + } + return keys[0] +} diff --git a/mdl/visitor/visitor_alter_item_slot_test.go b/mdl/visitor/visitor_alter_item_slot_test.go new file mode 100644 index 0000000000..96dbe197e3 --- /dev/null +++ b/mdl/visitor/visitor_alter_item_slot_test.go @@ -0,0 +1,40 @@ +// SPDX-License-Identifier: Apache-2.0 + +package visitor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" +) + +// PROPOSAL_alter_object_list_item_address.md: an item address takes a slot, +// `acc1 item@1.headercontent`, carried as the target's Column exactly as +// `grid.controlbar` carries a widget's slot. The slot must not be read as the +// selector's widget — `item@1.headercontent` has two identifiers, the shape +// `item(t1)` used to be recognised by. +func TestAlterTarget_ItemSlot(t *testing.T) { + stmt := buildAlterPage(t, `alter page M.P { + insert into acc1 item@1.headercontent { dynamictext t1 (Content: 'x') }; + insert into acc1 item(btnIn).headercontent { dynamictext t2 (Content: 'x') }; + insert into acc1 item('One')@2.headercontent { dynamictext t3 (Content: 'x') }; + insert into acc1 item@1 { dynamictext t4 (Content: 'x') }; + };`) + check := func(label string, got ast.WidgetRef, want ast.NavItemRef, spelled, slot string) { + t.Helper() + if got.Item == nil || *got.Item != want { + t.Errorf("%s: Item = %+v, want %+v", label, got.Item, want) + } + if got.Widget != spelled || got.Column != slot || got.Ordinal != 0 { + t.Errorf("%s: Widget %q Column %q Ordinal %d, want %q, %q, 0", label, got.Widget, got.Column, got.Ordinal, spelled, slot) + } + } + ins := func(i int) ast.WidgetRef { return stmt.Operations[i].(*ast.InsertWidgetOp).Target } + check("item@1.slot", ins(0), ast.NavItemRef{List: "acc1", Ordinal: 1}, "acc1 item@1", "headercontent") + check("item(w).slot", ins(1), ast.NavItemRef{List: "acc1", Widget: "btnIn"}, "acc1 item(btnIn)", "headercontent") + check("item('t')@2.slot", ins(2), ast.NavItemRef{List: "acc1", Text: "One", Ordinal: 2}, "acc1 item('One')@2", "headercontent") + check("bare item", ins(3), ast.NavItemRef{List: "acc1", Ordinal: 1}, "acc1 item@1", "") + if got := ins(0).Name(); got != "acc1 item@1.headercontent" { + t.Errorf("Name() = %q, want the address as written", got) + } +} diff --git a/mdl/visitor/visitor_alter_page.go b/mdl/visitor/visitor_alter_page.go index ce0d7d30c6..fac82bc64a 100644 --- a/mdl/visitor/visitor_alter_page.go +++ b/mdl/visitor/visitor_alter_page.go @@ -281,6 +281,14 @@ func (b *Builder) buildAlterTarget(ctx parser.IAlterTargetContext) ast.WidgetRef // the item's, so it moves off the target, where it would mean a choice // among widgets. ids := tc.AllIdentifierOrKeyword() + // `acc1 item@1.headercontent` names a slot of the item. It is carried + // as Column, the way `grid.controlbar` carries a widget's slot, and is + // taken off the identifiers first: `item@1.slot` has two of them, the + // shape `item(t1)` is otherwise recognised by. + if slot := tc.GetItemSlot(); slot != nil { + ref.Column = identifierOrKeywordText(slot) + ids = ids[:len(ids)-1] + } item := &ast.NavItemRef{List: identifierOrKeywordText(ids[0]), Ordinal: ref.Ordinal} if sl := tc.STRING_LITERAL(); sl != nil { item.Text = unquoteStringLit(sl)