diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index 5a984b569..51cad6f20 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -391,6 +391,12 @@ extracting `OffsetExpression`/`LimitExpression`. | A fresh clone of a project created by `mxcli new` goes dirty the first time anyone builds it: ~50 **tracked** files modified that nobody edited — every `javascriptsource/*/actions/*.js` gains a banner, `import { Big } from "big.js"` and `export async function`, plus the matching `javasource` stubs. In a cloud session with a stop-hook git check it reads as "uncommitted changes" at the end of clean work | The template ships the generated action stubs in a slightly older shape and MxBuild rewrites them all on the first build. `mx check` does **not** — only a build does — so nothing before the first `run --local` could reveal it, which is after the user has already committed | `cmd/mxcli/docker/settle.go` (`SettleGeneratedSources`), `cmd/mxcli/cmd_new.go` (step 5/6, `--skip-build`), `cmd/mxcli/init.go` (`/theme-cache/` in the generated ignore list) | Fix the *timing*, not the content: run the build while the project is still being created, so the settled form lands in the first commit. Do **not** reimplement the rewrite — it is mxbuild's generator and version-specific; run the real thing. Best-effort by contract (no JDK, no mxbuild, failed build → warning, never a failed creation), because a settled tree is a nicety and a usable project is the deliverable. The other half is gitignore: `theme-cache/` is a cache and says so. A/B on 11.12.1, both git-init'd then built: `--skip-build` → 50 dirty files, default → **0**. Tests `cmd/mxcli/docker/settle_test.go`. mxcli-todo #7 | | `mxcli check` reports `✓ Syntax OK` / `Check passed!`, then `mxcli exec` on the same script fails partway through with `failed to resolve page: page not found: Module.Page` — a button targeting a page the script creates further down. `exec` is not transactional, so the statements before the failure are already written to the .mpr | Page references are resolved in statement order at exec time. `check --references` already had an ordered pass (`validateForwardPageRefs`), but it needs `-p`; plain `check` had nothing, and plain `check` is what gets run | `mdl/executor/validate_page_order.go` (`ValidateScriptPageOrder`, MDL-PAGE01), wired in `cmd/mxcli/cmd_check.go` beside the other project-free validators | The soundness argument is what makes this work without a project: a **plain** CREATE later in the script would fail if the page already existed, so the script itself asserts the page does not exist yet and the earlier reference cannot resolve against the project either. `CREATE OR MODIFY`/`OR REPLACE` assert nothing, so they stay with `--references`, which can look. **Generalisable**: an ordering rule that seems to need project state often does not, once you find the statement that already asserts what you were going to look up. Two things the diagnostic must say and does: a cycle cannot be fixed by ordering (create one page without the linking widget, add it with `ALTER PAGE … INSERT`), and commit before executing a large script. Verified against all `mdl-examples/**/*.mdl` for false positives (0). Tests `mdl/executor/validate_page_order_test.go`, example `mdl-examples/bug-tests/todo-9-forward-page-reference.fail.mdl`. mxcli-todo #9 | | A sidebar navigation label renders truncated — "All tasks" shows as "All task" — under every theme (`signal`, `ledger`, `console`) and both variants, so switching theme does not help. Measured on the live DOM as `scrollWidth=56` inside `clientWidth=48` | Atlas's **closed** sidebar is an icon rail: `--navsidebar-width-closed: 48px` in Atlas's own `themesource/atlas_core/web/themes/_theme-default.scss`. The label is wider than the rail, and the closed rail expects an *icon*, not text. No mxcli theme sets any navigation width — the themes map colours — which is exactly why every theme reproduces it | `.claude/skills/mendix/theme-styling.md` (documented; no code change) | **A fix was written, verified, and reverted** — record the reason: adding `text-overflow: ellipsis` to the nav item helps only where Atlas also sets `white-space: nowrap`; elsewhere the label wraps to two readable lines, and the rule turns `All / tasks` into `All / t…`. Screenshotted both ways against the real compiled CSS. The answer belongs to the app (give nav items icons — what the rail is for — or keep the sidebar open), not to a theme that would impose it on every app. **Generalisable**: when a reported symptom traces to an upstream layout constant, reproduce the geometry against the real compiled CSS (48px container, real class names, Playwright measurement) — it takes minutes, tells you whose constant it is, and shows when the "obvious" CSS fix is a regression. mxcli-todo #19d | +| `create non-persistent entity X ( A: String not null error '…' )` (or `unique`) passes `mxcli check` AND `mxcli exec`, then the build fails **CE0070** "Validations rules are not allowed on entity 'X', because it is not persistable" | `not null` / `unique` ARE validation rules — Studio Pro models "required" and "uniqueness" as rules on the entity, not as column constraints — so Mendix rejects them on a non-persistable entity. Nothing in mxcli connected the attribute constraint to the entity's persistence kind | `mdl/executor/cmd_enumerations.go` (`validateNPEValidationRules`, called from `ValidateEntity`) | Add **MDL054**, error severity, fired from the CREATE path only. **Establish the construct matrix against mxbuild before writing the rule, not from the issue text** — verified on 11.6.6 that `not null` with a message, `not null` bare, AND `unique` each produce CE0070 while a plain attribute does not, so the bare form (easy to miss, since the reporter only showed the message form) is flagged too. The CREATE path is the only one that can run this: `ALTER ENTITY … ADD ATTRIBUTE` does not carry the persistence kind, the same limitation MDL020 has. Sweep `mdl-examples/` + `scripts/check-skill-mdl.sh` after adding any error-severity rule — a false positive there breaks every user with that shape. Negative test `mdl-examples/bug-tests/832-npe-validation-rules.fail.mdl` (`.fail.mdl` = must fail check, enforced by `make check-mdl`) plus `-ok.mdl` pinning the other edge; tests `TestValidateEntityNPEValidationRules`. Issue #832 | +| `retrieve $L from Mod.Entity where [Attr = $Var/Mod.Assoc/Attr]` passes `mxcli check` AND `mxcli exec`, then the build fails **CE0161** "Error(s) in XPath constraint" | Mendix XPath reaches at most ONE hop off a variable, and nothing checked the hop count | `mdl/executor/validate_microflow.go` (`checkXPathVariableTraversal`, `xpathVarTraversalRe`), called from the `*ast.RetrieveStmt` arm beside MDL047/MDL048 | Add **MDL055**, error severity, matching a `$var`-rooted path with **2+ segments**. **Establish the boundary against mxbuild first — it is narrower than it looks**: `$Var/Attr` VALID, `$Var/Mod.Assoc` VALID (one hop to the associated object), `$Var/Mod.Assoc/Attr` CE0161. A rule keying on "a module-qualified segment follows a variable" would reject the middle form, which is legal; key on hop count instead. Verified on 11.6.6 by building all three and dropping the offender to confirm the other two are clean. **Reject, don't try to serialize** — there is no valid XPath for the two-hop form, so the constraint must be restructured and only the author knows which way. **Verify the suggestion you emit**: both recommended rewrites (`retrieve $Related from $Var/Mod.Assoc;` then constrain on `$Related/Attr`; or invert to `[Mod.Assoc/Mod.Entity = $Var]`) were built and confirmed at 0 errors before the message claimed "both forms build clean". Negative test `mdl-examples/bug-tests/831-xpath-variable-traversal.fail.mdl` + `-ok.mdl`; test `TestXPathVariableTraversal`. Issue #831 | +| `mxcli check` reports a microflow rule (e.g. MDL048 `[id = $StringVar]`) but `mxcli exec` writes the same script silently — a script that skips `check` produces a project the build rejects | Two different validators. The exec path called `ValidateMicroflowBody` (semantic errors); the MDL0xx rule set lives in `ValidateMicroflow`, which was wired only into `cmd/mxcli/cmd_check.go` and the LSP. Same shape as #836, where a guard existed on every exec path but was never reached from validate | `mdl/executor/validate.go` (`validateMicroflowRules`, `execEnforcedMicroflowRules`) called from `mdl/executor/cmd_microflows_create.go` — the handler, mirroring `ValidateEntity` in `cmd_entities.go` (NOT `validateWithContext`, which would double-report under `check --references`) | Promote an **explicit allowlist** of rules verified against a real mxbuild, never the whole set. **Blanket promotion was tried and reverted**: it turns every error-severity rule into a write barrier, and `MDL009` ("enumeration splits require exactly one value per branch") was a **false positive** — a multi-value branch covering every enum value builds at 0 errors on 11.6.6, and the shipped `write-microflows` skill documents that very form (since retired, see the MDL056 row). It also broke an existing repo test whose fixture uses `else` on an enum split. `MDL008` by contrast IS correct (mxbuild: CE0079 per uncovered value + CE0773) — so before promoting any rule, build its construct and read the verdict. Warnings are never promoted. Tests `TestValidateMicroflowRules_ReachedFromExec` and `…_UnverifiedRulesNotPromoted` (the latter fails if the allowlist is widened carelessly). Issue #833 | +| `mxcli check` errors **MDL009** "enumeration splits require exactly one value per branch" on `when Open, Pending then` — but Mendix accepts it, so check rejects valid MDL and contradicts the shipped `write-microflows` skill | The rule asserted the opposite of the platform's behaviour. Nobody had built the construct: a multi-value branch covering every value **plus `(empty)`** builds at 0 errors on 11.6.6 | `mdl/executor/validate_microflow.go` (the `*ast.EnumSplitStmt` arm; `checkEnumSplitEmptyBranch`) | Retire the assertion and replace it with what actually fails: an enum split needs an outgoing flow per condition value, so a missing branch is **CE0079**. **MDL056** checks the `(empty)` branch — universal, verified to hold even on a `not null` enum attribute, so it needs no enumeration lookup and works from the statement alone. Full value coverage (the other half of CE0079) is deliberately NOT implemented: it needs the enum's member list, i.e. resolving the split variable's type against script or project, which `ValidateMicroflow` cannot see — guessing would trade one false positive for another. **Use a NEW rule ID rather than repurposing**, so anything citing the old number still means the old, wrong thing. `MDL008` (no `else`) is correct and stays — mxbuild gives CE0079 per uncovered value **plus** CE0773 on the else flow. Fix the skill in the same change: it documented the invalid `else` form. Tests `TestValidateMicroflow_EnumSplitMultipleValuesAllowed` / `…RequiresEmptyBranch`; repro `mdl-examples/bug-tests/mdl009-enum-split-empty-branch.fail.mdl` + `-ok.mdl` | +| A microflow using `split type` writes a project mxbuild cannot **load**: `KeyNotFoundException: The given key '' was not present in the dictionary` at `StreamingBsonUnitReader.ResolvePostponedProperties`. `mxcli check` ✓ and `mxcli exec` ✓; reproduced on 11.6.6 and 11.13.0 | Two gaps in the modelsdk writer, both the #791 shape. (1) `microflowObjectToGen` had no `*microflows.InheritanceSplit` case → `default: return nil`, so the split was dropped while three sequence flows kept pointing at its `$ID`. (2) `caseValueToGen` had no `InheritanceCase` case → every branch degraded to a bare `Microflows$NoCase`, losing the entity it selects on. Its value-receiver normalisation also omitted the type, so a pointer-only fix would still miss half the calls | `mdl/backend/modelsdk/microflow_write.go` (`microflowObjectToGen`, `caseValueToGen`) — mirror `sdk/mpr/writer_microflow.go` | Add both cases. **Diagnose with the #791 recipe**: `mxcli bson dump --type microflow`, collect every `$ID`, check each key ending in `Pointer` resolves (before: 27 objects / 3 dangling; after: 28 / 0). **Take field lists from the GENERATED type, not from legacy** — legacy writes `ErrorHandlingType` on the split but `initInheritanceSplit` has no such property, i.e. legacy writes a field Mendix does not define. **When adding a case-value type, update the value-receiver normalisation too.** Modelling rules confirmed on both versions while verifying: a type split needs an outgoing flow for every type INCLUDING the base (CE0090), and an `else` does NOT substitute for the base-type case. Tests `TestMicroflowRoundTrip_InheritanceSplit`, `TestCaseValueToGen_InheritanceCase{,ValueReceiver}`; repro `mdl-examples/bug-tests/split-type-dangling-pointer.mdl` | +| The `split type` docs and examples teach a shape that fails the build: `case Spec` + `else`, with no branch for the base entity → **CE0090** "The 'X' value should be configured for an outgoing flow". `mxcli check` passes, so the drift survived; `mdl-examples/bug-tests/365` and `475` both shipped it, and 475's own header claimed "mx check reports 0 errors" | `else` on an inheritance split serializes as `Microflows$NoCase` and IS accepted, so it looks like it covers the remainder — but it does not satisfy type coverage. The base entity needs its own `case` | `.claude/skills/mendix/write-microflows.md` (Type Split section) + `mdl-examples/bug-tests/365-…`, `475-…` | Cover EVERY type including the base; `else` is then redundant. Also give the split somewhere to go: branches converge on a merge continuing to the end event, so a non-void microflow needs a `return` after `end split;` (else MDL003 + **CE0067**). Matrix verified on 11.6.6 AND 11.13.0: `specs+base` 0 errors, `specs+base+else` 0 errors, `specs+else only` CE0090. **When repairing a bug-test fixture, preserve the scenario it pins** — 475 tests "exactly ONE non-split branch continues", so its added base case must TERMINATE; an empty (falling-through) body would make two branches continue and silently retire the regression. Confirmed after the edit that the post-split activity still renders outside both case bodies and the describe→exec roundtrip is mxbuild-clean. Known cosmetic artifact: DESCRIBE emits an empty `else` block that was never authored; it re-parses and builds clean | | A published OData service created purely from MDL passes `mxcli check` and then fails the build — `[CE0729] "The service name should not be empty."` and `[CE7375] "Attribute ID for entity 'X' must be published and be the key when associations are exposed as an associated object id."` — the second firing even with no associations exposed at all | Two defaults `CREATE ODATA SERVICE` never set. (1) `Name` (the document) and `ServiceName` (the name in the OData metadata document) are different properties and only the first was set; the CONSUMED path had defaulted this for CE0339 all along. (2) `PublishAssociations` defaults to false = "associations as an associated object id", which Mendix only allows when the system `ID` is published as the key — but MDL's `expose (Attr (KEY))` publishes an ordinary attribute | `mdl/executor/cmd_odata.go` (`serviceName` fallback + heal on create-or-modify; `publishAssociationsFor`; `nonPersistablePublishedEntities` warning), `mdl/ast/ast_odata.go` + `mdl/visitor/visitor_odata.go` (`PublishAssociationsSet`) | **Wider than reported**: the finding framed CE7375 as a non-persistable-entity problem. Measured on 11.12.1, the identical service with a PERSISTENT entity and a unique key builds 0 errors with `true` and CE7375 with `false` — so the default broke *every* published service, and non-persistable was just where it could not be worked around. Defaulting to true does not pick a preference; it picks the only value that can build from the MDL people write. Needs tri-state (`PublishAssociationsSet`) so an explicit `false` is still honoured, and `create or modify` no longer flips a stored value the author did not mention. **Why nothing caught it**: `mdl-examples/doctype-tests/10-odata-examples.mdl` sets both properties explicitly, so the repo's own example worked around both defaults. Tests `cmd_odata_service_name_test.go`, `cmd_odata_publish_associations_test.go`; examples `f1-10.1-…`, `f1-10.4-…`. mxcli-formula1 #10.1/#10.4 | | A typo in an OData property — `ReadMicroflow:` for `ReadMode:`, `ServiceNam:` for `ServiceName:` — passes `mxcli check` and `exec` reports success, but the model does not have the property. Hours can go into wondering why a published resource ignores its read microflow | The grammar accepts any `name: value` pair inside an OData property list, and the visitor's `switch` had no `default` — so an unrecognised name was dropped between parse and AST. The ALTER path has always answered `"unknown OData service property: %s"`; CREATE, PUBLISH ENTITY, the client and the external entity had nothing | `mdl/ast/ast_odata.go` (`UnknownProperties` on four statements), `mdl/visitor/visitor_odata.go` (four `default:` arms), `mdl/executor/validate_odata_properties.go` (`ValidateODataProperties`, MDL-ODATA01), wired in `cmd/mxcli/cmd_check.go` | The visitor is where the name is lost, so the visitor is where it must be recorded — a validator over the AST alone cannot see a key that was already discarded. Carry it as `UnknownProperties` and report at check time, before anything is written. The message names the property AND guesses the intended one (prefix/substring, then one edit), because a bare known-property list still leaves the reader diffing two spellings by eye. **Correction to the report**: `Pagesize:` is *not* silently dropped — the visitor lowercases before matching, so casing is never a typo, and the test pins that. Verified against every `mdl-examples/**/*.mdl` for false positives (0). Tests `mdl/executor/validate_odata_properties_test.go`. mxcli-formula1 suggested issue 8 | | A read-microflow-backed OData resource must declare a `System.ODataResponse` parameter and compute a count, even when the count is expensive (a full CSV scan) and nobody asked for it — with no MDL to say otherwise. Same for `$skip`/`$top` support | `Countable`, `SkipSupported` and `TopSupported` were written as literal `true` in the BSON writer's `ODataPublish$QueryOptions`; nothing above the writer could express them | `mdl/ast/ast_odata.go` + `mdl/visitor/visitor_odata.go` (`*bool` on `PublishedEntityDef`, `odataBoolPtr`), `model/types.go`, `mdl/executor/cmd_odata.go` (`astEntityDefToModel`), `mdl/backend/modelsdk/odata_write.go` (`boolOrDefault`), `odata_read_detail.go` (`falseOnly`) | Tri-state (`*bool`) is load-bearing: these default to **true**, so "unset" and "false" cannot share a representation or every existing script would silently turn them off. The reader maps a stored `true` back to nil (`falseOnly`) so DESCRIBE prints only what the author wrote instead of three defaults on every resource. Verified end to end on 11.12.1: `Countable: No` + a read microflow with **no** `$Response` parameter builds 0 errors, which is exactly the combination that was impossible before. Tests `cmd_odata_query_options_test.go`, `odata_write_test.go`. mxcli-formula1 #10.3 | diff --git a/.claude/skills/mendix/write-microflows.md b/.claude/skills/mendix/write-microflows.md index efdc57f08..326486134 100644 --- a/.claude/skills/mendix/write-microflows.md +++ b/.claude/skills/mendix/write-microflows.md @@ -421,31 +421,63 @@ Use `case` when a microflow branches on an enumeration value. case $Status when Open, Pending then return true; - when (empty) then + when Closed then return false; - else + when (empty) then return false; end case; ``` `(empty)` represents an unset enumeration value. Multiple values can share one `when` branch by separating them with commas. Case values are bare identifiers — do **not** quote them. +> **Every value needs a branch, including `(empty)` — and there is no `else`.** +> A Mendix enum split is an exclusive split with one outgoing flow per condition +> value, so an uncovered value fails the build with **CE0079** *"The 'X' condition +> value should be configured in properties for an outgoing flow."* `mxcli check` +> reports a missing `(empty)` branch as **MDL056**, and an `else` branch as +> **MDL008** (an `else` does not stand in for the missing flows: mxbuild reports +> CE0079 for each uncovered value *and* CE0773 on the else flow itself). +> +> The `(empty)` branch is required **even when the attribute is `not null`** — +> verified on Mendix 11.6.6. If several values share a path, put them in one +> branch (`when Open, Pending then`) rather than reaching for `else`. + ### Type Split And Cast Statements Use `split type` when a microflow branches on an object's runtime specialization. Use `cast` inside a type branch to create the specialized variable used by the branch body. ```mdl +declare $IsSpecialized boolean = false; split type $Input case Sample.SpecializedInput cast $SpecificInput; - return true; -else - return false; + set $IsSpecialized = true; +case Sample.BaseInput end split; +return $IsSpecialized; ``` -`case` values are qualified entity names. The optional `else` branch handles objects that do not match any listed specialization. +`case` values are qualified entity names. + +> **Every type needs a branch — including the base entity.** An object-type +> decision gets one outgoing flow per listed type, and a type with no flow fails +> the build with **CE0090** *"The 'X' value should be configured for an outgoing +> flow."* The base entity (the split variable's own type) counts: `case +> Sample.BaseInput` above is what covers "it is not any of the specializations". +> +> **`else` does not stand in for the base-type case.** It is accepted — it +> serializes as `Microflows$NoCase` — but it does not satisfy coverage, so +> `case Spec` + `else` still fails CE0090. Once every type has a branch, `else` +> is redundant. Verified on Mendix 11.6.6 and 11.13.0. +> +> **The split needs somewhere to go afterwards.** Branch bodies converge on a +> merge that continues to the microflow's end event, so a non-void microflow +> needs a `return` after `end split;` — otherwise `mxcli check` reports MDL003 +> and the build fails **CE0067** *"The 'Return value' property is required."* +> Doing the per-branch work into a variable and returning it once (above) is the +> clearest shape; returning inside every branch also works, but still needs the +> trailing `return`. **`cast` only stores the output variable.** Studio Pro persists Microflows$CastAction with a single `VariableName` field — the source variable is implicit (the type-split's input). Use `cast $SpecificName;` to give the specialized variable its name. The two-variable form `$Output = cast $Source;` parses but `$Source` is dropped on roundtrip; prefer the single-variable form. diff --git a/mdl-examples/bug-tests/365-microflow-inheritance-split.mdl b/mdl-examples/bug-tests/365-microflow-inheritance-split.mdl index b938d49c0..879a21753 100644 --- a/mdl-examples/bug-tests/365-microflow-inheritance-split.mdl +++ b/mdl-examples/bug-tests/365-microflow-inheritance-split.mdl @@ -10,17 +10,29 @@ create persistent entity InheritanceSplitExample.SpecializedInput extends Inheri ); / +-- An object-type decision needs an outgoing flow for EVERY listed type, +-- including the base entity. This example used `case Specialized` + `else`, +-- which fails the build with CE0090 ("The 'InheritanceSplitExample.BaseInput' +-- value should be configured for an outgoing flow") — `else` serializes as +-- Microflows$NoCase and is accepted, but it does not satisfy coverage. +-- +-- The branches also converge on a merge that continues to the end event, so a +-- non-void microflow needs a `return` after `end split;` (otherwise CE0067 +-- "The 'Return value' property is required", and mxcli check reports MDL003). +-- +-- Verified with mxbuild 11.6.6 and 11.13.0: 0 errors. create microflow InheritanceSplitExample.RouteInput ( $Input: InheritanceSplitExample.BaseInput ) returns boolean begin + declare $IsSpecialized boolean = false; split type $Input case InheritanceSplitExample.SpecializedInput cast $SpecializedInput; - return true; - else - return false; + set $IsSpecialized = true; + case InheritanceSplitExample.BaseInput end split; + return $IsSpecialized; end; / diff --git a/mdl-examples/bug-tests/475-inheritance-split-continuing-branch-merge.mdl b/mdl-examples/bug-tests/475-inheritance-split-continuing-branch-merge.mdl index 9c443a5d8..64d1c7cea 100644 --- a/mdl-examples/bug-tests/475-inheritance-split-continuing-branch-merge.mdl +++ b/mdl-examples/bug-tests/475-inheritance-split-continuing-branch-merge.mdl @@ -23,6 +23,13 @@ -- Validation: -- `mxcli check` parses the script. -- `mx check` against the resulting MPR reports 0 errors. +-- +-- The base-type case (`case BugTest475.Vehicle`) is required for that: an +-- object-type decision needs an outgoing flow for every listed type, and +-- without the base entity the build fails CE0090 regardless of this bug. +-- It is deliberately a TERMINATING branch — the scenario under test is +-- "exactly ONE non-split branch continues", and giving Vehicle a falling +-- -through body would make two branches continue and lose the regression. -- Roundtrip (describe → exec → describe) preserves the structure -- byte-for-byte: the post-split log activity stays outside both case -- bodies. @@ -66,6 +73,8 @@ begin case BugTest475.Boat log info node 'BugTest475' 'Dispatching boat'; return false; + case BugTest475.Vehicle + return false; end split; log info node 'BugTest475' 'Dispatched'; return true; diff --git a/mdl-examples/bug-tests/831-xpath-variable-traversal-ok.mdl b/mdl-examples/bug-tests/831-xpath-variable-traversal-ok.mdl new file mode 100644 index 000000000..59285c468 --- /dev/null +++ b/mdl-examples/bug-tests/831-xpath-variable-traversal-ok.mdl @@ -0,0 +1,56 @@ +-- ============================================================================ +-- Issue #831 — the forms MDL055 must NOT reject (positive half) +-- ============================================================================ +-- +-- The negative half is 831-xpath-variable-traversal.fail.mdl. This file pins +-- the other edge: the two restructurings MDL055's message recommends, plus the +-- one-hop forms that are valid XPath and must not be flagged. +-- +-- Verified against mxbuild 11.6.6: this file builds with 0 errors. +-- ============================================================================ + +CREATE MODULE Bug831Ok; + +CREATE OR MODIFY PERSISTENT ENTITY Bug831Ok.Category ( Name: String(50) ); + +CREATE OR MODIFY PERSISTENT ENTITY Bug831Ok.Product ( Code: String(50) ); + +CREATE OR MODIFY ASSOCIATION Bug831Ok.Product_Category + FROM Bug831Ok.Product TO Bug831Ok.Category TYPE Reference; + +-- Recommended form 1: retrieve the associated object first (one hop is a legal +-- retrieve SOURCE), then constrain on that variable's own attribute. +CREATE OR MODIFY MICROFLOW Bug831Ok.Form1 ($RefProduct: Bug831Ok.Product) +RETURNS list of Bug831Ok.Category +BEGIN + retrieve $Related from $RefProduct/Bug831Ok.Product_Category; + retrieve $Categories from Bug831Ok.Category where [Name = $Related/Name]; + return $Categories; +END; + +-- Recommended form 2: invert the constraint so the traversal starts at the +-- entity being retrieved, which XPath does support. +CREATE OR MODIFY MICROFLOW Bug831Ok.Form2 ($RefProduct: Bug831Ok.Product) +RETURNS list of Bug831Ok.Category +BEGIN + retrieve $Categories from Bug831Ok.Category + where [Bug831Ok.Product_Category/Bug831Ok.Product = $RefProduct]; + return $Categories; +END; + +-- One hop off a variable is valid and must not be flagged: an attribute… +CREATE OR MODIFY MICROFLOW Bug831Ok.OneHopAttribute ($RefProduct: Bug831Ok.Product) +RETURNS list of Bug831Ok.Category +BEGIN + retrieve $Categories from Bug831Ok.Category where [Name = $RefProduct/Code]; + return $Categories; +END; + +-- …and the associated object itself. +CREATE OR MODIFY MICROFLOW Bug831Ok.OneHopAssociation ($RefProduct: Bug831Ok.Product) +RETURNS list of Bug831Ok.Product +BEGIN + retrieve $Products from Bug831Ok.Product + where [Bug831Ok.Product_Category = $RefProduct/Bug831Ok.Product_Category]; + return $Products; +END; diff --git a/mdl-examples/bug-tests/831-xpath-variable-traversal.fail.mdl b/mdl-examples/bug-tests/831-xpath-variable-traversal.fail.mdl new file mode 100644 index 000000000..efebccd60 --- /dev/null +++ b/mdl-examples/bug-tests/831-xpath-variable-traversal.fail.mdl @@ -0,0 +1,44 @@ +-- ============================================================================ +-- Issue #831 — RETRIEVE WHERE traversing an association from a variable +-- ============================================================================ +-- +-- NEGATIVE TEST (.fail.mdl): `mxcli check` MUST reject this file. An +-- unexpected pass means MDL055 has regressed. +-- +-- `where [Name = $RefProduct/ZKT39.Product_Category/Name]` passed `mxcli check` +-- and `mxcli exec`, then the build failed: +-- +-- [error] [CE0161] "Error(s) in XPath constraint." +-- +-- Mendix XPath reaches at most ONE hop off a variable. The boundary is narrower +-- than "a qualified name after a variable" — verified against mxbuild 11.6.6: +-- +-- $Var/Attr VALID the parameter's own attribute +-- $Var/Mod.Assoc VALID one hop, the associated object +-- $Var/Mod.Assoc/Attr CE0161 two or more hops +-- +-- so the rule keys on hop count. A rule that flagged any qualified segment +-- would reject the middle form, which is valid. +-- +-- There is no valid serialization of the two-hop form, which is why this is a +-- rejection and not a writer fix: the constraint has to be restructured, and +-- only the author knows which shape they meant. Both restructurings are in +-- 831-xpath-variable-traversal-ok.mdl, which must PASS. +-- ============================================================================ + +CREATE MODULE Bug831; + +CREATE OR MODIFY PERSISTENT ENTITY Bug831.Category ( Name: String(50) ); + +CREATE OR MODIFY PERSISTENT ENTITY Bug831.Product ( Code: String(50) ); + +CREATE OR MODIFY ASSOCIATION Bug831.Product_Category + FROM Bug831.Product TO Bug831.Category TYPE Reference; + +CREATE OR MODIFY MICROFLOW Bug831.ACT_Find ($RefProduct: Bug831.Product) +RETURNS list of Bug831.Category +BEGIN + retrieve $Categories from Bug831.Category + where [Name = $RefProduct/Bug831.Product_Category/Name]; + return $Categories; +END; diff --git a/mdl-examples/bug-tests/832-npe-validation-rules-ok.mdl b/mdl-examples/bug-tests/832-npe-validation-rules-ok.mdl new file mode 100644 index 000000000..7e7c088cc --- /dev/null +++ b/mdl-examples/bug-tests/832-npe-validation-rules-ok.mdl @@ -0,0 +1,24 @@ +-- ============================================================================ +-- Issue #832 — the forms MDL054 must NOT reject (positive half) +-- ============================================================================ +-- +-- The negative half is 832-npe-validation-rules.fail.mdl. This file pins the +-- other edge: MDL054 must not fire on a validation rule that is legitimately +-- placed, or on a non-persistent entity that carries none. +-- +-- Verified against mxbuild 11.6.6: this file builds with 0 errors. +-- ============================================================================ + +CREATE MODULE Bug832Ok; + +-- A PERSISTENT entity is exactly where validation rules belong. +CREATE OR MODIFY PERSISTENT ENTITY Bug832Ok.P ( + "Name": String(100) not null error 'Name is required', + "Code": String(50) unique error 'Code must be unique' +); + +-- A non-persistent entity with no validation rule is fine. +CREATE OR MODIFY NON-PERSISTENT ENTITY Bug832Ok.NpPlain ( + "Name": String(100), + "Qty": Integer +); diff --git a/mdl-examples/bug-tests/832-npe-validation-rules.fail.mdl b/mdl-examples/bug-tests/832-npe-validation-rules.fail.mdl new file mode 100644 index 000000000..dba2caf20 --- /dev/null +++ b/mdl-examples/bug-tests/832-npe-validation-rules.fail.mdl @@ -0,0 +1,36 @@ +-- ============================================================================ +-- Issue #832 — validation rules on a non-persistent entity were accepted +-- ============================================================================ +-- +-- NEGATIVE TEST (.fail.mdl): `mxcli check` MUST reject this file. An +-- unexpected pass means MDL054 has regressed. +-- +-- Mendix refuses a validation rule on a non-persistable entity: +-- +-- [error] [CE0070] "Validations rules are not allowed on entity 'X', +-- because it is not persistable." +-- +-- `not null` and `unique` ARE validation rules — Studio Pro models "required" +-- and "uniqueness" as rules on the entity, not as column constraints — so both +-- are rejected on an NPE. `mxcli check` and `mxcli exec` both accepted them and +-- only a real build caught it, which is the worst place to find out. +-- +-- Verified against mxbuild 11.6.6: `not null` with a message, `not null` bare, +-- and `unique` each produce CE0070; a plain attribute does not. The message is +-- optional and does not change the verdict. +-- +-- The accepted counterparts — the same constraints on a PERSISTENT entity, and +-- an NPE with no constraint — are in 832-npe-validation-rules-ok.mdl, which +-- must PASS. Together they pin both edges of the rule. +-- +-- Only the CREATE path can catch this: an `ALTER ENTITY … ADD ATTRIBUTE` does +-- not carry the entity's persistence kind, so it cannot be told apart from a +-- persistent entity without a project. Same limitation as MDL020. +-- ============================================================================ + +CREATE MODULE Bug832; + +CREATE OR MODIFY NON-PERSISTENT ENTITY Bug832.Np ( + "Name": String(100) not null error 'Name is required', + "Code": String(50) unique error 'Code must be unique' +); diff --git a/mdl-examples/bug-tests/mdl009-enum-split-empty-branch-ok.mdl b/mdl-examples/bug-tests/mdl009-enum-split-empty-branch-ok.mdl new file mode 100644 index 000000000..0a759a309 --- /dev/null +++ b/mdl-examples/bug-tests/mdl009-enum-split-empty-branch-ok.mdl @@ -0,0 +1,59 @@ +-- ============================================================================ +-- MDL009 retired / MDL056 — the enum-split forms that must NOT be rejected +-- ============================================================================ +-- +-- The negative half is mdl009-enum-split-empty-branch.fail.mdl. +-- +-- Verified against mxbuild 11.6.6: this file builds with 0 errors, including +-- the multi-value branch that the retired MDL009 used to reject. +-- ============================================================================ + +CREATE MODULE BugM9Ok; + +CREATE ENUMERATION BugM9Ok.Status ( + Open caption 'Open', + Pending caption 'Pending', + Closed caption 'Closed' +); + +-- A multi-value branch is valid — this is what MDL009 wrongly rejected. +CREATE OR MODIFY MICROFLOW BugM9Ok.MultiValue ($Status: Enumeration(BugM9Ok.Status)) +RETURNS Boolean +BEGIN + case $Status + when Open, Pending then + return true; + when Closed then + return false; + when (empty) then + return false; + end case; +END; + +-- One value per branch is equally valid. +CREATE OR MODIFY MICROFLOW BugM9Ok.OnePerBranch ($Status: Enumeration(BugM9Ok.Status)) +RETURNS Boolean +BEGIN + case $Status + when Open then + return true; + when Pending then + return true; + when Closed then + return false; + when (empty) then + return false; + end case; +END; + +-- `(empty)` may share a branch with real values. +CREATE OR MODIFY MICROFLOW BugM9Ok.EmptySharesBranch ($Status: Enumeration(BugM9Ok.Status)) +RETURNS Boolean +BEGIN + case $Status + when Open, Pending then + return true; + when Closed, (empty) then + return false; + end case; +END; diff --git a/mdl-examples/bug-tests/mdl009-enum-split-empty-branch.fail.mdl b/mdl-examples/bug-tests/mdl009-enum-split-empty-branch.fail.mdl new file mode 100644 index 000000000..2cc6140ed --- /dev/null +++ b/mdl-examples/bug-tests/mdl009-enum-split-empty-branch.fail.mdl @@ -0,0 +1,43 @@ +-- ============================================================================ +-- MDL009 retired, MDL056 added — enum split branch rules +-- ============================================================================ +-- +-- NEGATIVE TEST (.fail.mdl): `mxcli check` MUST reject this file. +-- +-- MDL009 used to error on `when Open, Pending then`, claiming Mendix required +-- exactly one value per branch. That was wrong — verified on mxbuild 11.6.6, a +-- multi-value branch covering every value builds with 0 errors, and the shipped +-- write-microflows skill documents that form. The rule rejected valid MDL, so +-- it is retired. +-- +-- What actually fails the build is a MISSING branch. An enum split is an +-- exclusive split needing one outgoing flow per condition value: +-- +-- [error] [CE0079] "The '(empty)' condition value should be configured in +-- properties for an outgoing flow." +-- +-- MDL056 catches the `(empty)` case, which is universal and needs no knowledge +-- of the enumeration's members — it holds even on a `not null` attribute. +-- +-- The valid forms are in mdl009-enum-split-empty-branch-ok.mdl, which must PASS. +-- ============================================================================ + +CREATE MODULE BugM9; + +CREATE ENUMERATION BugM9.Status ( + Open caption 'Open', + Pending caption 'Pending', + Closed caption 'Closed' +); + +-- REJECTED (MDL056): every value is covered, but `(empty)` is not. +CREATE OR MODIFY MICROFLOW BugM9.NoEmptyBranch ($Status: Enumeration(BugM9.Status)) +RETURNS Boolean +BEGIN + case $Status + when Open, Pending then + return true; + when Closed then + return false; + end case; +END; diff --git a/mdl-examples/bug-tests/split-type-dangling-pointer.mdl b/mdl-examples/bug-tests/split-type-dangling-pointer.mdl new file mode 100644 index 000000000..7b42908db --- /dev/null +++ b/mdl-examples/bug-tests/split-type-dangling-pointer.mdl @@ -0,0 +1,47 @@ +-- ============================================================================ +-- `split type` wrote a project mxbuild could not LOAD +-- ============================================================================ +-- +-- `mxcli check` passed and `mxcli exec` reported success, but `mx check` died +-- before validating anything: +-- +-- ERROR: System.Collections.Generic.KeyNotFoundException: The given key +-- '' was not present in the dictionary +-- at StreamingBsonUnitReader.ResolvePostponedProperties() +-- +-- Reproduced on Mendix 11.6.6 and 11.13.0. Two gaps in the modelsdk writer, +-- both the #791 shape — an object dropped at serialization while the sequence +-- flows pointing at it were still written: +-- +-- 1. microflowObjectToGen had no *microflows.InheritanceSplit case, so the +-- split itself vanished. Three flows referenced its $ID. +-- 2. caseValueToGen had no InheritanceCase case, so every branch degraded to +-- a bare Microflows$NoCase and lost the entity it selects on. +-- +-- Diagnosed with the #791 recipe: dump the microflow, collect every $ID, and +-- check each key ending in `Pointer` resolves. Before: 27 objects, 10 pointers, +-- 3 dangling. After: 28 objects, 10 pointers, 0 dangling. +-- +-- A type split must give every type an outgoing flow, INCLUDING the base type +-- (CE0090 otherwise). An `else` does NOT substitute for the base-type case — +-- verified on both versions. +-- +-- To verify: run this script, then `mx check` — 0 errors. +-- ============================================================================ + +CREATE MODULE BugSplit; + +CREATE OR MODIFY PERSISTENT ENTITY BugSplit.Animal ( Name: String(50) ); + +CREATE OR MODIFY PERSISTENT ENTITY BugSplit.Dog EXTENDS BugSplit.Animal ( Breed: String(50) ); + +CREATE OR MODIFY MICROFLOW BugSplit.Classify ($A: BugSplit.Animal) +RETURNS String +BEGIN + split type $A + case BugSplit.Dog + cast $d; + case BugSplit.Animal + end split; + return 'done'; +END; diff --git a/mdl/backend/modelsdk/microflow_inheritance_write_test.go b/mdl/backend/modelsdk/microflow_inheritance_write_test.go new file mode 100644 index 000000000..50f9b1892 --- /dev/null +++ b/mdl/backend/modelsdk/microflow_inheritance_write_test.go @@ -0,0 +1,78 @@ +// SPDX-License-Identifier: Apache-2.0 + +package modelsdkbackend + +import ( + "testing" + + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// TestMicroflowRoundTrip_InheritanceSplit covers a corruption found while +// testing enum-split `else` across versions: `split type` produced a project +// mxbuild could not LOAD at all — +// +// KeyNotFoundException: The given key '' was not present in the +// dictionary at StreamingBsonUnitReader.ResolvePostponedProperties() +// +// on both 11.6.6 and 11.13.0, while `mxcli check` passed. Two gaps, both the +// #791 shape (an object dropped at serialization while the flows pointing at +// it were written): +// +// 1. microflowObjectToGen had no *microflows.InheritanceSplit case, so the +// split hit `default: return nil` and vanished. Three sequence flows +// referenced its $ID — that is the dangling pointer the loader trips on. +// 2. caseValueToGen had no InheritanceCase case, so every branch flow got a +// bare NoCase and the entity each branch selects on was lost. +func TestMicroflowRoundTrip_InheritanceSplit(t *testing.T) { + split := µflows.InheritanceSplit{VariableName: "A", Caption: "split"} + split.ID = model.ID("split-1") + + mf := µflows.Microflow{ + Name: "TypeSplit", + ObjectCollection: µflows.MicroflowObjectCollection{ + Objects: []microflows.MicroflowObject{split}, + }, + } + mf.ID = model.ID("mf-1") + + got := roundTripMicroflow(t, mf) + + var found *microflows.InheritanceSplit + if got.ObjectCollection != nil { + for _, obj := range got.ObjectCollection.Objects { + if s, ok := obj.(*microflows.InheritanceSplit); ok { + found = s + } + } + } + if found == nil { + t.Fatal("InheritanceSplit did not survive the round trip — the object is dropped at " + + "serialization while flows still point at its $ID, which is the KeyNotFoundException") + } + if found.VariableName != "A" { + t.Errorf("VariableName = %q, want A", found.VariableName) + } +} + +// The branch's case value must round-trip as an InheritanceCase naming the +// entity, not degrade to a NoCase. +func TestCaseValueToGen_InheritanceCase(t *testing.T) { + el := caseValueToGen(µflows.InheritanceCase{EntityQualifiedName: "SP.Dog"}) + if el == nil { + t.Fatal("caseValueToGen returned nil for an InheritanceCase") + } + if got := el.TypeName(); got != "Microflows$InheritanceCase" { + t.Fatalf("$Type = %q, want Microflows$InheritanceCase (a NoCase loses the branch entity)", got) + } +} + +// The visitor sometimes yields value receivers; those must dispatch the same +// way, exactly as the existing normalisation does for EnumerationCase. +func TestCaseValueToGen_InheritanceCaseValueReceiver(t *testing.T) { + el := caseValueToGen(microflows.InheritanceCase{EntityQualifiedName: "SP.Dog"}) + if el == nil || el.TypeName() != "Microflows$InheritanceCase" { + t.Fatalf("value-receiver InheritanceCase degraded to %v", el) + } +} diff --git a/mdl/backend/modelsdk/microflow_write.go b/mdl/backend/modelsdk/microflow_write.go index 05db0d046..d56947cb4 100644 --- a/mdl/backend/modelsdk/microflow_write.go +++ b/mdl/backend/modelsdk/microflow_write.go @@ -307,6 +307,21 @@ func microflowObjectToGen(obj microflows.MicroflowObject) element.Element { g.SetSplitCondition(sc) } return g + case *microflows.InheritanceSplit: + // Without this the split hit `default: return nil` and was dropped, while + // the sequence flows referencing its $ID were still written — a dangling + // pointer that mxbuild cannot even load ("KeyNotFoundException ... at + // StreamingBsonUnitReader.ResolvePostponedProperties"). Same shape as the + // ErrorEvent/BreakEvent gap in #791. Fields mirror the legacy serializer + // in sdk/mpr/writer_microflow.go. + g := genMf.NewInheritanceSplit() + g.SetID(element.ID(o.ID)) + g.SetCaption(o.Caption) + g.SetDocumentation(o.Documentation) + g.SetRelativeMiddlePoint(pointStr(o.Position)) + g.SetSize(sizeStr(o.Size)) + g.SetSplitVariableName(o.VariableName) + return g case *microflows.ExclusiveMerge: g := genMf.NewExclusiveMerge() g.SetID(element.ID(o.ID)) @@ -1087,6 +1102,8 @@ func caseValueToGen(cv microflows.CaseValue) element.Element { cv = &c case microflows.NoCase: cv = &c + case microflows.InheritanceCase: + cv = &c } switch c := cv.(type) { case *microflows.EnumerationCase: @@ -1099,6 +1116,14 @@ func caseValueToGen(cv microflows.CaseValue) element.Element { g.SetID(element.ID(c.ID)) g.SetValue(c.Expression) return g + case *microflows.InheritanceCase: + // A type-split branch selects on an entity. Without this it fell through + // to NoCase, so every branch lost the entity it matches on — the second + // half of the `split type` corruption. + g := genMf.NewInheritanceCase() + g.SetID(element.ID(c.ID)) + g.SetValueQualifiedName(c.EntityQualifiedName) + return g default: return genMf.NewNoCase() } diff --git a/mdl/executor/bugfix_test.go b/mdl/executor/bugfix_test.go index f29792ee0..a92522418 100644 --- a/mdl/executor/bugfix_test.go +++ b/mdl/executor/bugfix_test.go @@ -906,3 +906,89 @@ func TestExprToStringNoSpaces(t *testing.T) { }) } } + +// TestValidateEntityNPEValidationRules covers issue #832: Mendix refuses +// validation rules on a non-persistable entity with +// +// CE0070 "Validations rules are not allowed on entity 'X', because it is +// not persistable." +// +// `not null` and `unique` ARE validation rules — Studio Pro models "required" +// and "uniqueness" as rules on the entity, not as column constraints — so both +// forms are rejected on an NPE. Verified against mxbuild 11.6.6: `not null` +// with a message, `not null` bare, and `unique` each produce CE0070, while a +// plain attribute does not. Before this rule `mxcli check` and `mxcli exec` +// both accepted them and only a real build caught it. +func TestValidateEntityNPEValidationRules(t *testing.T) { + cases := []struct { + name string + input string + wantFor []string // attribute names expected to be flagged + }{ + { + "not null with message", + `create non-persistent entity Test.NP ( "Name" : String(100) not null error 'req' );`, + []string{"Name"}, + }, + { + // The message is optional; the rule exists either way, so bare + // `not null` is rejected by Mendix just the same. + "not null bare", + `create non-persistent entity Test.NP ( "Name" : String(100) not null );`, + []string{"Name"}, + }, + { + "unique", + `create non-persistent entity Test.NP ( "Code" : String(50) unique error 'dup' );`, + []string{"Code"}, + }, + { + "both constraints on separate attributes", + `create non-persistent entity Test.NP ( "Name" : String(100) not null, "Code" : String(50) unique );`, + []string{"Name", "Code"}, + }, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + prog, errs := visitor.Build(c.input) + if len(errs) > 0 { + t.Fatalf("parse error: %v", errs[0]) + } + stmt := prog.Statements[0].(*ast.CreateEntityStmt) + violations := ValidateEntity(stmt) + for _, attrName := range c.wantFor { + found := false + for _, v := range violations { + if v.RuleID == "MDL054" && strings.Contains(v.Message, "'"+attrName+"'") { + found = true + } + } + if !found { + t.Errorf("expected MDL054 for attribute %q (CE0070), got: %v", attrName, violations) + } + } + }) + } +} + +// A PERSISTENT entity may carry exactly the same constraints — that is where +// validation rules belong — so the rule must not fire there. Nor should it fire +// on an NPE attribute that carries no constraint. +func TestValidateEntityValidationRulesAllowedWhenPersistent(t *testing.T) { + cases := []string{ + `create persistent entity Test.P ( "Name" : String(100) not null error 'req', "Code" : String(50) unique );`, + `create non-persistent entity Test.NP ( "Name" : String(100), "Qty" : Integer );`, + } + for _, input := range cases { + prog, errs := visitor.Build(input) + if len(errs) > 0 { + t.Fatalf("parse error: %v", errs[0]) + } + stmt := prog.Statements[0].(*ast.CreateEntityStmt) + for _, v := range ValidateEntity(stmt) { + if v.RuleID == "MDL054" { + t.Errorf("unexpected MDL054 on %q: %s", input, v.Message) + } + } + } +} diff --git a/mdl/executor/cmd_enumerations.go b/mdl/executor/cmd_enumerations.go index 73050fa7c..43801fa90 100644 --- a/mdl/executor/cmd_enumerations.go +++ b/mdl/executor/cmd_enumerations.go @@ -450,6 +450,53 @@ func ValidateEntity(stmt *ast.CreateEntityStmt) []linter.Violation { entityName := stmt.Name.String() for _, attr := range stmt.Attributes { violations = append(violations, validateEntityAttribute(attr, persistent, entityName)...) + if !persistent { + violations = append(violations, validateNPEValidationRules(attr, entityName)...) + } + } + return violations +} + +// validateNPEValidationRules (MDL054) rejects a validation rule on a +// non-persistable entity, which Mendix refuses with +// +// CE0070 "Validations rules are not allowed on entity 'X', because it is +// not persistable." +// +// `not null` and `unique` ARE validation rules: Studio Pro models "required" +// and "uniqueness" as rules on the entity rather than as column constraints, +// so both are rejected on an NPE. Verified against mxbuild 11.6.6 — `not null` +// with a message, `not null` bare, and `unique` each produce CE0070; a plain +// attribute does not. The message is optional and does not change the verdict, +// so the bare form is flagged too. Issue #832. +// +// Only the CREATE path can run this: an `ALTER ENTITY … ADD ATTRIBUTE` does not +// carry the entity's persistence kind, so ValidateAlterEntity cannot tell an NPE +// from a persistent entity without a project. That is the same limitation +// MDL020 has, for the same reason — see ValidateAlterEntity. +func validateNPEValidationRules(attr ast.Attribute, entityName string) []linter.Violation { + var violations []linter.Violation + flag := func(constraint, mdl string) { + violations = append(violations, linter.Violation{ + RuleID: "MDL054", + Severity: linter.SeverityError, + Message: fmt.Sprintf( + "attribute '%s' declares `%s` on non-persistent entity %s — Mendix does not allow "+ + "validation rules on a non-persistable entity (CE0070), and `%s` is a validation rule", + attr.Name, constraint, entityName, constraint), + Location: linter.Location{DocumentType: "entity", DocumentName: entityName}, + Suggestion: fmt.Sprintf( + "Drop `%s` from the attribute, or make the entity persistent. To keep the check on a "+ + "non-persistent entity, enforce it in the microflow that populates it "+ + "(e.g. `if %s = empty then` … ) rather than declaring `%s`.", + mdl, attr.Name, mdl), + }) + } + if attr.NotNull { + flag("not null", "not null") + } + if attr.Unique { + flag("unique", "unique") } return violations } diff --git a/mdl/executor/cmd_microflows_create.go b/mdl/executor/cmd_microflows_create.go index 227b787dc..d48a134bc 100644 --- a/mdl/executor/cmd_microflows_create.go +++ b/mdl/executor/cmd_microflows_create.go @@ -42,6 +42,15 @@ func execCreateMicroflow(ctx *ExecContext, s *ast.CreateMicroflowStmt) error { return mdlerrors.NewValidation("microflow name must not be empty") } + // Refuse the XPath constraints Mendix rejects, before writing anything. + // `mxcli check` already reported these, but exec ran a different validator + // and wrote them anyway, so a script that skipped check produced a project + // the build fails on (issue #833). Same placement as the entity handler's + // ValidateEntity call. + if err := validateMicroflowRules(s); err != nil { + return err + } + // Find or auto-create module module, err := findOrCreateModule(ctx, s.Name.Module) if err != nil { diff --git a/mdl/executor/validate.go b/mdl/executor/validate.go index 536282f16..e58b7d281 100644 --- a/mdl/executor/validate.go +++ b/mdl/executor/validate.go @@ -12,6 +12,7 @@ import ( "github.com/mendixlabs/mxcli/mdl/ast" mdlerrors "github.com/mendixlabs/mxcli/mdl/errors" + "github.com/mendixlabs/mxcli/mdl/linter" "github.com/mendixlabs/mxcli/model" "github.com/mendixlabs/mxcli/sdk/domainmodel" ) @@ -148,12 +149,19 @@ func (sc *scriptContext) allNames() []string { // annotateForwardRef checks if a failed statement's error references an object // that is defined later in the script. If so, it appends a hint to reorder. -func annotateForwardRef(err error, _ ast.Statement, created, allDefined *scriptContext) error { +func annotateForwardRef(err error, stmt ast.Statement, created, allDefined *scriptContext) error { msg := err.Error() + // A statement's OWN name is "defined in the script but not yet created" at + // the moment it fails, so without this any validation error that names its + // own subject picked up the reorder hint — telling the author to move a + // statement before itself. Found via MDL054, whose message names the entity + // being created (#832). + self := newScriptContext() + self.collectSingle(stmt) // Check each name that is defined in the script but not yet created. for _, name := range allDefined.allNames() { - if created.has(name) { - continue // already created before this statement + if created.has(name) || self.has(name) { + continue // already created before this statement, or defined by it } if strings.Contains(msg, name) { return fmt.Errorf("%w\n hint: %s is defined later in this script — move its create statement before this one", err, name) @@ -948,3 +956,58 @@ func getErrorHandlerBody(stmt ast.MicroflowStatement) []ast.MicroflowStatement { } return nil } + +// execEnforcedMicroflowRules are the MDL rules `mxcli exec` refuses to write, +// not just report. Membership requires that the rule's claim has been verified +// against a real mxbuild — a rule that is merely plausible must not become a +// hard write barrier. +// +// All three are XPath-constraint rules whose constructs were built and confirmed +// to fail CE0161: +// +// MDL047 [Mod.Assoc = empty] — no `= empty` for an association +// MDL048 [id = $StringVar] — no id operator from an expression +// MDL055 [Attr = $Var/Mod.Assoc/Attr] — at most one hop off a variable +// +// The rest of the MDL0xx set stays check-only deliberately. Promoting all 17 +// error-severity rules was tried and rejected: MDL009 ("enumeration splits +// require exactly one value per branch") is a FALSE POSITIVE — a multi-value +// branch covering every enum value builds at 0 errors on 11.6.6, and the +// shipped write-microflows skill documents that form — so promoting the set +// wholesale would have made exec refuse valid MDL. Verify a rule before adding +// it here. +var execEnforcedMicroflowRules = map[string]bool{ + "MDL047": true, + "MDL048": true, + "MDL055": true, +} + +// validateMicroflowRules runs the MDL0xx microflow rule set (ValidateMicroflow) +// on the exec path and turns the verified subset's ERROR-severity violations +// into a failure, so `mxcli exec` refuses to write what those rules reject. +// +// Before this, ValidateMicroflow was wired only into cmd_check.go and the LSP; +// the exec path ran ValidateMicroflowBody, a different validator with a +// different rule set, so a script that skipped `check` wrote microflows the +// build would reject (issue #833, reported via MDL048). +// +// Warnings are never promoted: they are advisory and `check` itself passes with +// them. The rule ID is included so an exec failure matches what `check` prints. +func validateMicroflowRules(stmt *ast.CreateMicroflowStmt) error { + var msgs []string + for _, v := range ValidateMicroflow(stmt) { + if v.Severity != linter.SeverityError || !execEnforcedMicroflowRules[v.RuleID] { + continue + } + msg := fmt.Sprintf("[%s] %s", v.RuleID, v.Message) + if v.Suggestion != "" { + msg += "\n " + v.Suggestion + } + msgs = append(msgs, msg) + } + if len(msgs) == 0 { + return nil + } + return mdlerrors.NewValidationf("microflow '%s' has validation errors:\n - %s", + stmt.Name.String(), strings.Join(msgs, "\n - ")) +} diff --git a/mdl/executor/validate_forwardref_test.go b/mdl/executor/validate_forwardref_test.go new file mode 100644 index 000000000..13d6f2318 --- /dev/null +++ b/mdl/executor/validate_forwardref_test.go @@ -0,0 +1,49 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "errors" + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/visitor" +) + +// TestAnnotateForwardRef_SkipsOwnName covers a misfire found while adding +// MDL054: a statement's OWN name is "defined in the script but not yet +// created" at the moment it fails, so any validation error whose message names +// its own subject picked up the reorder hint — advising the author to move a +// statement before itself. +// +// A genuine forward reference (a name some LATER statement defines) must still +// be annotated. +func TestAnnotateForwardRef_SkipsOwnName(t *testing.T) { + script := `create non-persistent entity Test.NpEntity ( "Name" : String(100) not null ); +create microflow Test.Later () returns Boolean begin return true; end;` + prog, errs := visitor.Build(script) + if len(errs) > 0 { + t.Fatalf("parse error: %v", errs[0]) + } + allDefined := newScriptContext() + for _, s := range prog.Statements { + allDefined.collectSingle(s) + } + created := newScriptContext() // nothing created yet + + t.Run("own name is not a forward reference", func(t *testing.T) { + err := errors.New("attribute 'Name' declares `not null` on non-persistent entity Test.NpEntity") + got := annotateForwardRef(err, prog.Statements[0], created, allDefined) + if strings.Contains(got.Error(), "defined later in this script") { + t.Errorf("statement was annotated as referring forward to itself:\n%s", got.Error()) + } + }) + + t.Run("a genuine forward reference is still annotated", func(t *testing.T) { + err := errors.New("microflow not found: Test.Later") + got := annotateForwardRef(err, prog.Statements[0], created, allDefined) + if !strings.Contains(got.Error(), "defined later in this script") { + t.Errorf("expected a reorder hint for Test.Later, got:\n%s", got.Error()) + } + }) +} diff --git a/mdl/executor/validate_microflow.go b/mdl/executor/validate_microflow.go index c05dee69a..a198ce7b8 100644 --- a/mdl/executor/validate_microflow.go +++ b/mdl/executor/validate_microflow.go @@ -162,9 +162,9 @@ func (v *microflowValidator) walkBody(body []ast.MicroflowStatement) { v.walkBody(stmt.ThenBody) v.walkBody(stmt.ElseBody) case *ast.EnumSplitStmt: - // Mendix enumeration splits map to exclusive splits with one outgoing - // flow per enum value. Multiple values per branch and a default (else) - // flow are not supported — Studio Pro will reject both with CE errors. + // A Mendix enumeration split is an exclusive split that needs an + // outgoing flow for every enum value AND for (empty); a default flow + // is not offered. Verified on mxbuild 11.6.6. if len(stmt.ElseBody) > 0 { v.addViolation("MDL008", linter.SeverityError, fmt.Sprintf("case statement on '$%s' has an else branch; "+ @@ -173,14 +173,14 @@ func (v *microflowValidator) walkBody(body []ast.MicroflowStatement) { stmt.Variable), "Add an explicit when branch for every enum value instead of using else") } + // MDL009 used to error here on a branch listing more than one value, + // claiming Mendix required exactly one per branch. That was wrong — + // `when Open, Pending then` covering every value builds with 0 errors, + // and the write-microflows skill documents that form — so the rule + // rejected valid MDL. It is retired rather than repurposed; MDL056 + // below checks what actually fails the build. + v.checkEnumSplitEmptyBranch(stmt) for _, c := range stmt.Cases { - if len(c.Values) > 1 { - v.addViolation("MDL009", linter.SeverityError, - fmt.Sprintf("case statement on '$%s': when branch lists %d values (%s); "+ - "Mendix enumeration splits require exactly one value per branch.", - stmt.Variable, len(c.Values), strings.Join(c.Values, ", ")), - "Split into separate when branches, one per enum value") - } v.walkBody(c.Body) } v.walkBody(stmt.ElseBody) @@ -265,6 +265,7 @@ func (v *microflowValidator) walkBody(body []ast.MicroflowStatement) { xp := expressionToXPath(stmt.Where) v.checkXPathAssociationEmpty(stmt.Variable, xp) v.checkXPathIdConstraint(stmt.Variable, xp) + v.checkXPathVariableTraversal(stmt.Variable, xp) } case *ast.CallMicroflowStmt: v.checkAssociationObjectArgs("microflow "+stmt.MicroflowName.String(), stmt.Arguments) @@ -561,6 +562,43 @@ func (v *microflowValidator) checkXPathAssociationEmpty(variable, xpath string) } } +// xpathVarTraversalRe matches a path rooted at a $variable with TWO OR MORE +// segments (`$P/Mod.Assoc/Name`). One segment is deliberately not matched: both +// `$P/Code` (the parameter's own attribute) and `$P/Mod.Assoc` (one hop to the +// associated object) are valid XPath. The boundary is the hop count, not whether +// a segment is module-qualified — see checkXPathVariableTraversal. +var xpathVarTraversalRe = regexp.MustCompile(`\$(\w+)((?:/[A-Za-z_][\w.]*){2,})`) + +// checkXPathVariableTraversal flags a retrieve constraint that traverses an +// association FROM a variable (`[Name = $RefProduct/Mod.Product_Category/Name]`). +// Mendix XPath reaches at most one hop off a variable, so this fails the build +// with CE0161 while mxcli accepted it silently (issue #831). +// +// Verified against mxbuild 11.6.6 — the boundary is narrower than it looks: +// +// $Var/Attr VALID a parameter's own attribute +// $Var/Mod.Assoc VALID one hop, the associated object +// $Var/Mod.Assoc/Attr CE0161 two or more hops +// +// There is no valid serialization of the two-hop form, which is why this is a +// rejection rather than a writer fix: the constraint has to be restructured, and +// only the author knows which of the two shapes they meant. +func (v *microflowValidator) checkXPathVariableTraversal(variable, xpath string) { + for _, m := range xpathVarTraversalRe.FindAllStringSubmatch(xpath, -1) { + root, path := m[1], "$"+m[1]+m[2] + segs := strings.Split(strings.TrimPrefix(m[2], "/"), "/") + firstHop, leaf := segs[0], segs[len(segs)-1] + v.addViolation("MDL055", linter.SeverityError, + fmt.Sprintf("retrieve '$%s' constraint traverses an association from a variable (`%s`), which Mendix XPath "+ + "does not support (CE0161 \"Error(s) in XPath constraint\") — a constraint reaches at most one hop off a variable", + variable, path), + fmt.Sprintf("Retrieve the associated object first, then constrain on its own attribute: "+ + "`retrieve $Related from $%s/%s;` and use `[%s = $Related/%s]`. Or invert the constraint so the "+ + "traversal starts at the entity being retrieved: `[%s/ = $%s]`. Both forms build clean.", + root, firstHop, leaf, leaf, firstHop, root)) + } +} + // xpathIdConstraintRe matches a constraint comparing the object id against a VALUE // (`id = $strVar`, `id = '123'`, `id != 5`). It captures the right-hand operand. // Comparing `id` against an OBJECT variable (`[id != $ExistingOrder]` — the valid @@ -1206,3 +1244,40 @@ func isEmptyMessage(expr ast.Expression) bool { } return false } + +// checkEnumSplitEmptyBranch (MDL056) flags an enumeration split with no +// `(empty)` branch. A Mendix enum split needs an outgoing flow for every value +// AND for the unset case; without one the build fails with +// +// CE0079 "The '(empty)' condition value should be configured in properties +// for an outgoing flow." +// +// Verified on mxbuild 11.6.6, and the requirement is universal — it holds even +// when the split is on a `not null` enum attribute, so no nullability analysis +// is needed and the check works from the statement alone. +// +// This replaces the retired MDL009, which asserted the opposite of what Mendix +// does (see the EnumSplitStmt arm). A new ID was used rather than repurposing +// MDL009 so that anything referring to the old number still refers to the old, +// wrong meaning. +// +// Value coverage — every enum member having a branch, the other half of CE0079 — +// is deliberately NOT checked here: it needs the enumeration's member list, +// which means resolving the split variable's type against the script or the +// project. ValidateMicroflow sees only one statement. Worth adding where that +// context exists; guessing it here would trade one false positive for another. +func (v *microflowValidator) checkEnumSplitEmptyBranch(stmt *ast.EnumSplitStmt) { + for _, c := range stmt.Cases { + for _, val := range c.Values { + if strings.EqualFold(strings.TrimSpace(val), "(empty)") { + return + } + } + } + v.addViolation("MDL056", linter.SeverityError, + fmt.Sprintf("case statement on '$%s' has no `(empty)` branch; a Mendix enumeration split needs an "+ + "outgoing flow for the unset value too, so this builds as CE0079 \"The '(empty)' condition value "+ + "should be configured in properties for an outgoing flow\"", stmt.Variable), + "Add a `when (empty) then …` branch. It is required even when the attribute is `not null`. "+ + "A branch may list several values (`when Open, (empty) then …`) if they share a path.") +} diff --git a/mdl/executor/validate_microflow_enum_split_test.go b/mdl/executor/validate_microflow_enum_split_test.go index 7533cda89..7a368dbc4 100644 --- a/mdl/executor/validate_microflow_enum_split_test.go +++ b/mdl/executor/validate_microflow_enum_split_test.go @@ -65,7 +65,13 @@ func TestValidateMicroflow_EnumSplitElseForbidden(t *testing.T) { t.Fatalf("expected MDL008 for enum split with else branch, got %#v", violations) } -func TestValidateMicroflow_EnumSplitMultipleValuesForbidden(t *testing.T) { +// TestValidateMicroflow_EnumSplitMultipleValuesAllowed inverts what MDL009 used +// to assert. The old rule claimed "Mendix enumeration splits require exactly one +// value per branch" and errored on `when Open, Pending then`. That is wrong: +// verified on mxbuild 11.6.6, a multi-value branch covering every enum value +// (plus `(empty)`) builds with 0 errors, and the shipped write-microflows skill +// documents exactly that form. The rule rejected valid MDL. +func TestValidateMicroflow_EnumSplitMultipleValuesAllowed(t *testing.T) { stmt := &ast.CreateMicroflowStmt{ Name: ast.QualifiedName{Module: "Sample", Name: "Route"}, Body: []ast.MicroflowStatement{ @@ -75,18 +81,66 @@ func TestValidateMicroflow_EnumSplitMultipleValuesForbidden(t *testing.T) { {Values: []string{"Open", "Pending"}, Body: []ast.MicroflowStatement{ &ast.ReturnStmt{Value: &ast.LiteralExpr{Kind: ast.LiteralBoolean, Value: true}}, }}, + {Values: []string{"(empty)"}, Body: []ast.MicroflowStatement{ + &ast.ReturnStmt{Value: &ast.LiteralExpr{Kind: ast.LiteralBoolean, Value: false}}, + }}, }, }, }, } - violations := ValidateMicroflow(stmt) - for _, v := range violations { + for _, v := range ValidateMicroflow(stmt) { if v.RuleID == "MDL009" { - return + t.Fatalf("MDL009 rejected a multi-value branch, which Mendix accepts: %s", v.Message) + } + } +} + +// TestValidateMicroflow_EnumSplitRequiresEmptyBranch pins what MDL009 SHOULD +// have been checking. An enumeration split needs an outgoing flow for `(empty)` +// as well as for each value; without one the build fails +// +// CE0079 "The '(empty)' condition value should be configured in properties +// for an outgoing flow." +// +// Verified on mxbuild 11.6.6, and it is universal: it holds even when the split +// is on a `not null` enum attribute, so no nullability analysis is needed. +func TestValidateMicroflow_EnumSplitRequiresEmptyBranch(t *testing.T) { + mk := func(values ...[]string) *ast.CreateMicroflowStmt { + var cases []ast.EnumSplitCase + for _, vals := range values { + cases = append(cases, ast.EnumSplitCase{Values: vals, Body: []ast.MicroflowStatement{ + &ast.ReturnStmt{Value: &ast.LiteralExpr{Kind: ast.LiteralBoolean, Value: true}}, + }}) } + return &ast.CreateMicroflowStmt{ + Name: ast.QualifiedName{Module: "Sample", Name: "Route"}, + Body: []ast.MicroflowStatement{&ast.EnumSplitStmt{Variable: "Status", Cases: cases}}, + } + } + fires := func(stmt *ast.CreateMicroflowStmt) bool { + for _, v := range ValidateMicroflow(stmt) { + if v.RuleID == "MDL056" { + return true + } + } + return false + } + + if !fires(mk([]string{"Open"}, []string{"Closed"})) { + t.Error("expected MDL056 when no (empty) branch is present (CE0079)") + } + if fires(mk([]string{"Open"}, []string{"Closed"}, []string{"(empty)"})) { + t.Error("MDL056 must not fire when an (empty) branch is present") + } + // The (empty) marker may share a branch with real values. + if fires(mk([]string{"Open", "(empty)"}, []string{"Closed"})) { + t.Error("MDL056 must not fire when (empty) shares a multi-value branch") + } + // Case is not significant in the marker. + if fires(mk([]string{"Open"}, []string{"(EMPTY)"})) { + t.Error("MDL056 must accept the (empty) marker regardless of case") } - t.Fatalf("expected MDL009 for enum split with multiple values per branch, got %#v", violations) } func TestValidateMicroflow_EnumSplitBranchScopedVariable(t *testing.T) { diff --git a/mdl/executor/validate_microflow_rules_exec_test.go b/mdl/executor/validate_microflow_rules_exec_test.go new file mode 100644 index 000000000..8ee753cd6 --- /dev/null +++ b/mdl/executor/validate_microflow_rules_exec_test.go @@ -0,0 +1,146 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/visitor" +) + +// TestValidateMicroflowRules_ReachedFromExec guards issue #833, which is the +// same shape as #836: a rule that exists and fires in `mxcli check` but is +// never reached from the exec path, so `exec` writes the very construct +// `check` rejects. +// +// ValidateMicroflow (the MDL0xx rule set) was wired only into cmd_check.go and +// the LSP. The exec path called ValidateMicroflowBody, a different function +// with a different rule set, so all 17 error-severity microflow rules were +// check-only. #833 reported it through MDL048 (`[id = $StringVar]`), but the +// gap was never specific to that rule. +// +// Only a VERIFIED subset is promoted (execEnforcedMicroflowRules). Blanket +// promotion was tried and rejected — MDL009 is a false positive, so making the +// whole set a write barrier would refuse valid MDL. Warnings are never promoted. +func TestValidateMicroflowRules_ReachedFromExec(t *testing.T) { + cases := []struct { + name string + src string + wantErr string // substring; "" means exec-validation must accept + }{ + { + // MDL048: comparing the object id against a String value. + name: "id compared to a string variable is rejected", + src: `create microflow M.ACT ($GuidText: String) returns M.Item +begin + retrieve $Found from M.Item where [id = $GuidText] limit 1; + return $Found; +end;`, + wantErr: "MDL048", + }, + { + // MDL055: two-hop traversal off a variable. + name: "variable association traversal is rejected", + src: `create microflow M.ACT ($P: M.Product) returns list of M.Category +begin + retrieve $L from M.Category where [Name = $P/M.Product_Category/Name]; + return $L; +end;`, + wantErr: "MDL055", + }, + { + // The valid counterpart of the same statement must still pass. + name: "one-hop traversal is accepted", + src: `create microflow M.ACT ($P: M.Product) returns list of M.Category +begin + retrieve $L from M.Category where [Name = $P/Code]; + return $L; +end;`, + }, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + prog, errs := visitor.Build(c.src) + if len(errs) > 0 { + t.Fatalf("parse error: %v", errs[0]) + } + stmt := prog.Statements[0].(*ast.CreateMicroflowStmt) + err := validateMicroflowRules(stmt) + if c.wantErr == "" { + if err != nil { + t.Errorf("valid microflow rejected by exec validation: %v", err) + } + return + } + if err == nil { + t.Fatalf("expected exec validation to reject this (%s); `check` already does", c.wantErr) + } + if !strings.Contains(err.Error(), c.wantErr) { + t.Errorf("error should name the rule %s so it matches what check prints, got: %v", c.wantErr, err) + } + }) + } +} + +// A warning-severity rule must not block exec — only errors do. MDL001/MDL002 +// and friends are advisory, and turning them into hard exec failures would +// break scripts that check reports as passing. +func TestValidateMicroflowRules_WarningsDoNotBlockExec(t *testing.T) { + // MDL006 (warning): a loop with no body statements. + src := `create microflow M.ACT () returns Boolean +begin + declare $x integer = 1; + return true; +end;` + prog, errs := visitor.Build(src) + if len(errs) > 0 { + t.Fatalf("parse error: %v", errs[0]) + } + stmt := prog.Statements[0].(*ast.CreateMicroflowStmt) + // Whatever warnings this trips, none may become an exec error. + if err := validateMicroflowRules(stmt); err != nil { + t.Errorf("warning-only microflow must not fail exec validation, got: %v", err) + } +} + +// TestValidateMicroflowRules_UnverifiedRulesNotPromoted pins the deliberate +// narrowness of execEnforcedMicroflowRules. +// +// MDL008 is a CORRECT rule (mxbuild rejects `else` on an enum split with CE0079 +// per uncovered value plus CE0773) that is nonetheless not on the allowlist: +// membership requires a verified construct, and correctness alone is not the +// bar — every promoted rule becomes a hard write barrier. This test fails the +// moment someone widens the allowlist wholesale. +func TestValidateMicroflowRules_UnverifiedRulesNotPromoted(t *testing.T) { + src := `create microflow M.ACT ($S: Enumeration(M.Status)) returns String +begin + case $S + when Open then + return 'a'; + when (empty) then + return 'b'; + else + return 'c'; + end case; +end;` + prog, errs := visitor.Build(src) + if len(errs) > 0 { + t.Fatalf("parse error: %v", errs[0]) + } + stmt := prog.Statements[0].(*ast.CreateMicroflowStmt) + + sawInCheck := false + for _, v := range ValidateMicroflow(stmt) { + if v.RuleID == "MDL008" { + sawInCheck = true + } + } + if !sawInCheck { + t.Fatal("expected check to report MDL008 for an else branch on an enum split") + } + if err := validateMicroflowRules(stmt); err != nil { + t.Errorf("MDL008 is not on the verified allowlist and must not block exec, got: %v", err) + } +} diff --git a/mdl/executor/validate_xpath_vartraversal_test.go b/mdl/executor/validate_xpath_vartraversal_test.go new file mode 100644 index 000000000..01d0aed1a --- /dev/null +++ b/mdl/executor/validate_xpath_vartraversal_test.go @@ -0,0 +1,73 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/visitor" +) + +// TestXPathVariableTraversal covers issue #831: a retrieve constraint whose +// right-hand side traverses an association FROM a variable +// +// where [Name = $RefProduct/Mod.Product_Category/Name] +// +// passed `mxcli check` and `mxcli exec`, and the build then failed CE0161. +// +// The valid/invalid boundary was established against mxbuild 11.6.6, and it is +// narrower than "a qualified name after a variable": +// +// $Var/Attr VALID — a parameter's own attribute +// $Var/Mod.Assoc VALID — one hop to the associated object +// $Var/Mod.Assoc/Attr CE0161 — two or more hops +// +// so the rule must key on the number of segments, not on the presence of a +// module-qualified one. Flagging the middle form would reject valid MDL. +func TestXPathVariableTraversal(t *testing.T) { + cases := []struct { + name string + where string + flag bool + }{ + {"attribute of a parameter", `[Name = $P/Code]`, false}, + {"one hop to the associated object", `[BX.Product_Category = $P/BX.Product_Category]`, false}, + {"two hops — the reported form", `[Name = $P/BX.Product_Category/Name]`, true}, + {"three hops", `[Name = $P/BX.A_B/BX.B_C/Name]`, true}, + // An entity-rooted traversal is not variable-rooted and is valid XPath. + {"entity-rooted traversal", `[BX.Product_Category/BX.Product = $P]`, false}, + {"bare attribute compare", `[Name = 'x']`, false}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + src := `create microflow BX.M ($P: BX.Product) returns list of BX.Category +begin + retrieve $L from BX.Category where ` + c.where + `; + return $L; +end;` + prog, errs := visitor.Build(src) + if len(errs) > 0 { + t.Fatalf("parse error: %v", errs[0]) + } + stmt, ok := prog.Statements[0].(*ast.CreateMicroflowStmt) + if !ok { + t.Fatalf("got %T", prog.Statements[0]) + } + got := false + var msg string + for _, v := range ValidateMicroflow(stmt) { + if v.RuleID == "MDL055" { + got, msg = true, v.Message + } + } + if got != c.flag { + t.Errorf("MDL055 fired = %v, want %v (where %s)\n message: %s", got, c.flag, c.where, msg) + } + if c.flag && got && !strings.Contains(msg, "$P") { + t.Errorf("message should name the offending variable path: %s", msg) + } + }) + } +}