From 4608ec3b10f66f32383c6744a6c51db32ecc3b11 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 7 Oct 2026 05:32:34 +0000 Subject: [PATCH] fix(check): refuse an operator inside an XPath function argument (MDL091) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `retrieve … where starts-with(Name, 'MS-' + $Key)` passed `check --references` and `exec`, then failed mx check with CE0161 "Error(s) in XPath constraint." (mendixlabs/mxcli#1326). MDL091 only matched expression-only function names in the rendered XPath string; it never looked at what a function's arguments are. Walk the retrieve constraint's AST and flag a function argument that is itself a `+` or `-` operation. MDL091 is exec-enforced, so exec now refuses it as well. Measured on mxbuild 11.14.0 with the check stubbed out to force the fault in: starts-with(Name, 'MS-' + $Key) CE0161 not(contains(Name, $Key + $Key)) CE0161 contains(Name, $Key - 'a') CE0161 Name = 'X-' + $Key 0 errors year-from-dateTime(Due) = $N - 1 0 errors starts-with(Name, $P) 0 errors `*`, `div` and `mod` are not flagged: their only possible argument is numeric, and `contains(Name, $N)` is CE0161 with no operator, so no measurement isolates the operator. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015mwKd8LDw9MRoZ8uBbbnAg --- .../fix-issue/findings/mdl-executor.jsonl | 1 + .../skills/mendix/xpath-constraints/SKILL.md | 5 +- ...-xpath-function-argument-operator.fail.mdl | 38 +++++++++ mdl/executor/validate_microflow.go | 84 +++++++++++++++++++ mdl/executor/xpath_function_operator_test.go | 83 ++++++++++++++++++ 5 files changed, 210 insertions(+), 1 deletion(-) create mode 100644 mdl-examples/bug-tests/1326-xpath-function-argument-operator.fail.mdl create mode 100644 mdl/executor/xpath_function_operator_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor.jsonl b/.claude/skills/fix-issue/findings/mdl-executor.jsonl index 78100e9361..3fcec2be08 100644 --- a/.claude/skills/fix-issue/findings/mdl-executor.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-executor.jsonl @@ -868,3 +868,4 @@ {"area": "mdl/executor", "date": "2026-10-05", "symptom": "On Mendix 11.15, `TestMxCheck_DoctypeScripts/40-message-definition-examples.mdl` fails with CE0270 \"No root element could be found in the schema\" at MsgTest.IMM_Order. On a converted or Studio Pro 11.15 project, `list message definition collections` finds nothing, and `describe import mapping` prints no source", "cause": "Mendix 11.15 replaced `MessageDefinitions$MessageDefinitionCollection` with one `MessageDefinitions$MessageDefinition2` document per definition. `mx convert` turns the collection into a `Projects$Folder` with the same unit ID and name, and the ExposedEntity tree is byte-identical apart from $IDs. mxcli read and wrote only collections, so on 11.15 there was nothing to resolve a mapping against", "file": "`mdl/backend/modelsdk/messagedefinition_document.go` (read/write, reusing the collection's exposedNodeFromGen/messageNodeToGen), `mdl/executor/cmd_messagedefinition_documents.go` (create/describe/drop/list + version refusals), `mdl/executor/mapping_messagedefinition.go` (`findMessageDefinition(ctx, ref)` returns the stored reference), `cmd_messagedefinitions.go` (alter target resolution, association-drop guard), grammar `createMessageDefinitionStatement` / `DROP|DESCRIBE MESSAGE DEFINITION` / `LIST MESSAGE DEFINITIONS`", "insight": "**`mx convert` is the oracle for a storage change.** Converting a known 11.14 project gives a Studio Pro-authored 11.15 reference for the new shape and for how references move, so no guessing is needed. **Under the revert, 11.15 fails earlier than CE0270.** A two-part name written into the old `MessageDefinition` key makes mxbuild refuse to load the project (`StorageLoadException: ... is not a valid OldMessageDefinitionIdentifier`). The old key is typed as a three-part identifier even though 11.15 no longer reads it. The doctype script uses `-- @version: ..11.14` / `11.15+` sections, and the 11.15 mapping needs a different name from the older section's, because a plain `check` sees both sections and reports MDL-DUPDEF", "refs": ["ako/mxcli#987"]} {"area": "mdl/executor", "date": "2026-10-06", "symptom": "v0.25.0: `describe microflow` 4-8x slower than v0.24.0 on a flow arranged by hand in Studio Pro, and the cost grows faster than the flow (60 if/else blocks: 4.7 s -> 35.6 s); flows laid out by mxcli barely change", "cause": "The canonical describe's layout derivation (#748) re-renders the description every round, and a hand-laid flow takes gatedLayoutRounds+2 = 8 rounds. Each render re-ran the stored flow's split/merge analysis (findSplitMergePoints, labelCrossedMerges) and the body warnings (postDominators via microflowgraph.Analyze) - superlinear, and independent of which annotations the round keeps", "file": "`mdl/executor/cmd_microflows_derived_layout.go` (flowAnalysisMemo, installed by useDerivedFlowLayout, rebuildOnly view for the rounds), `mdl/executor/cmd_microflows_show.go` (findSplitMergePoints / labels / microflowBodyWarnings go through it)", "insight": "**Profile before believing the issue's theory.** The report blamed the round count, but rounds were already capped at 8; the rebuild itself was cheap. The CPU profile showed the cost was the *render* inside each round - the stored flow's graph analysis, redone per round. Memoising it per describe (keyed on the stored collection pointer) and leaving the `-- WARNING` comment lines out of rounds (the rebuild never reads them) gave 14.2 s -> 1.7 s at 60 blocks, with the printed description byte-identical to the unfixed build. A wall-clock assertion would be flaky; the test counts split/merge analyses per describe (18 over 8 rounds before, 2 after), with the round count as its control", "refs": ["mendixlabs/mxcli#1301"]} {"area":"mdl-executor","date":"2026-10-05","symptom":"A view entity selecting a **non-localized** DateTime column (Studio Pro's \"Localize\" unticked — the normal choice for a calendar date) passes `mxcli check --references` and exec, then `mx check` fails CE6770 \"View Entity is out of sync with the OQL Query.\" The view attribute was always written LocalizeDate = true, and MDL has no spelling for the flag, so describe -> exec re-broke it on every run","cause":"execCreateViewEntity built every view attribute with convertDataType, whose DateTime is Mendix's default LocalizeDate = true; nothing looked at the attribute the column reads","file":"`mdl/executor/oql_view_localize_date.go` (viewDateTimeLocalize), `mdl/executor/cmd_entities.go` (execCreateViewEntity); test `mdl/executor/view_entity_localize_date_test.go`; bug-test `mdl-examples/bug-tests/1297-view-entity-non-localized-datetime.mdl`","insight":"**Derive, don't add syntax**: on a view entity the column's localization is a property of the query, so the fix reads it from the source attribute and describe -> exec round-trips (`Unchanged`) with nothing new to spell — same reasoning as the view association (the column is the declaration). **Measure the shapes before choosing the scope**: one project, one view per shape, mxbuild 11.12.5 — pass-through, `s/Attr`, MIN, MAX and CASE over a non-localized source are ALL CE6770 when written localized and 0 errors when patched to false, so a pass-through-only rule (the obvious one, mirroring the string-length rule) would have fixed the report and left MAX(date) broken. Sources that disagree (coalesce of a localized and a non-localized column) are unmeasured and keep the default. **Repro needs a Studio Pro-authored flag**: mxcli writes every persistent DateTime localized, so the bug is invisible from MDL alone — the reporter's pymongo patch flipping Sale.SaleDate is the cheapest stand-in. Control: stubbing the assignment fails 5 of 6 cases with `LocalizeDate = true`; pre-fix binary on the same project gives 5 × CE6770, fixed gives 0","refs":["mendixlabs/mxcli#1297"]} +{"area": "mdl-executor", "date": "2026-10-07", "symptom": "`retrieve … where starts-with(Name, 'MS-' + $Key)` passes `check --references` and `exec`, then mx check CE0161 \"Error(s) in XPath constraint.\" — the same `+` at the constraint's top level (`Name = 'X-' + $Key`) builds clean", "cause": "MDL091 only matched expression-only function NAMES (startsWith/endsWith/getKey) by regex over the rendered XPath string; an operator inside a function ARGUMENT is a different expression construct the string regex never looked for", "file": "`mdl/executor/validate_microflow.go` (`xpathFunctionArgumentOperators`, `checkXPathFunctionArgumentOperators`, MDL091)", "insight": "Walk the `RetrieveStmt.Where` AST, not the XPath string: a call's argument that IS a `+`/`-` BinaryExpr is CE0161 (after unwrapping ParenExpr AND `SourceExpr` — the retrieve where-clause is wrapped in a SourceExpr, and a walker that misses it silently matches nothing, which is how the first cut passed nothing while compiling clean). Only the unbracketed form reaches the writer: the bracketed `[…]` grammar already refuses `+` in a function argument as a parse error. Measured on mxbuild 11.14.0 with a stubbed-out check to force the fault in: `+` and `-` in a string argument CE0161 (also nested in `not()`), `year-from-dateTime(Due) = $N - 1` clean. `*`/`div`/`mod` deliberately NOT flagged — the only argument they fit is numeric, and the control `contains(Name, $N)` is already CE0161 with no operator, so no measurement isolates the operator; MDL091 is exec-enforced, so an unmeasured operator would be a write barrier on a guess. Tests `TestValidateMicroflow_XPathFunctionArgumentOperator`, `TestCheckExecAgree_RetrieveConstraintFunctionArgumentOperator`; repro `mdl-examples/bug-tests/1326-xpath-function-argument-operator.fail.mdl`", "refs": ["mendixlabs/mxcli#1326", "mendixlabs/mxcli#1213"]} diff --git a/.claude/skills/mendix/xpath-constraints/SKILL.md b/.claude/skills/mendix/xpath-constraints/SKILL.md index 50299a1034..634d04def1 100644 --- a/.claude/skills/mendix/xpath-constraints/SKILL.md +++ b/.claude/skills/mendix/xpath-constraints/SKILL.md @@ -215,7 +215,10 @@ where [Displayed = false()] Supported functions: `contains()`, `starts-with()`, `not()`, `true()`, `false()` The expression functions `startsWith()` / `endsWith()` are not XPath: in a -constraint they are CE0161, and `check` reports them as **MDL091**. A member the +constraint they are CE0161, and `check` reports them as **MDL091**. So is an +operator inside a function argument — `starts-with(Name, 'MS-' + $Key)` is +CE0161 although `Name = 'X-' + $Key` is fine: compute the value into a variable +first (`declare $P String = 'MS-' + $Key;`, then `starts-with(Name, $P)`). A member the entity does not have is a reference error in `check -p`, and so is a system member written the way `describe` prints the attribute: XPath spells it `createdDate`, `changedDate`, `owner`, `changedBy` — `[CreatedDate > …]` is CE0161. diff --git a/mdl-examples/bug-tests/1326-xpath-function-argument-operator.fail.mdl b/mdl-examples/bug-tests/1326-xpath-function-argument-operator.fail.mdl new file mode 100644 index 0000000000..5a13185c0d --- /dev/null +++ b/mdl-examples/bug-tests/1326-xpath-function-argument-operator.fail.mdl @@ -0,0 +1,38 @@ +-- Issue mendixlabs/mxcli#1326: an operator inside an XPath function argument. +-- +-- `mxcli check mdl-examples/bug-tests/1326-xpath-function-argument-operator.fail.mdl` +-- reports MDL091 on SUB_PlusInFunc, and exec refuses it. Before the fix it passed +-- check and exec, and mxbuild 11.14.0 rejected it: CE0161 "Error(s) in XPath +-- constraint." The two microflows after it are the controls, 0 errors on the +-- same mxbuild, and must stay clean. + +create or modify module G1326; + +create or modify persistent entity G1326.Item ( + Name: String(50) +); + +-- MDL091: the concatenation is inside the starts-with() argument. +create or modify microflow G1326.SUB_PlusInFunc ($Key: String) returns Integer as $N +begin + retrieve $L from G1326.Item where starts-with(Name, 'MS-' + $Key); + declare $N Integer = length($L); + return $N; +end; + +-- Clean: the same operator at the top level of the constraint. +create or modify microflow G1326.SUB_PlusTopLevel ($Key: String) returns Integer as $N +begin + retrieve $L from G1326.Item where Name = 'X-' + $Key; + declare $N Integer = length($L); + return $N; +end; + +-- Clean: the value computed beforehand — the fix MDL091 suggests. +create or modify microflow G1326.SUB_PreComputed ($Key: String) returns Integer as $N +begin + declare $P String = 'MS-' + $Key; + retrieve $L from G1326.Item where starts-with(Name, $P); + declare $N Integer = length($L); + return $N; +end; diff --git a/mdl/executor/validate_microflow.go b/mdl/executor/validate_microflow.go index 1b07758ff4..a9b6f86b50 100644 --- a/mdl/executor/validate_microflow.go +++ b/mdl/executor/validate_microflow.go @@ -392,6 +392,7 @@ func (v *microflowValidator) walkBody(body []ast.MicroflowStatement) { xp := expressionToXPath(stmt.Where) v.checkXPathAssociationEmpty(stmt.Variable, xp) v.checkXPathFunctionNames(stmt.Variable, xp) + v.checkXPathFunctionArgumentOperators(stmt.Variable, stmt.Where) v.checkXPathIdConstraint(stmt.Variable, xp) v.checkXPathVariableTraversal(stmt.Variable, xp) } @@ -829,6 +830,89 @@ func (v *microflowValidator) checkXPathFunctionNames(variable, xpath string) { } } +// xpathArithmeticOperators are the operators an XPath function argument cannot +// carry. Measured on mxbuild 11.14.0 (mendixlabs/mxcli#1326): +// +// starts-with(Name, 'MS-' + $Key) CE0161 "Error(s) in XPath constraint" +// not(contains(Name, $Key + $Key)) CE0161 +// contains(Name, $Key - 'a') CE0161 +// Name = 'X-' + $Key clean — the same operator at top level +// year-from-dateTime(Due) = $N - 1 clean — an operator beside a call, not in it +// starts-with(Name, $P) clean — the value computed beforehand +// +// `*`, `div` and `mod` are left out on purpose: the only argument they could +// appear in is numeric, and `contains(Name, $N)` with an Integer $N is already +// CE0161 with no operator at all, so no measurement isolates the operator. This +// rule blocks exec, so it covers only what was shown to fail. +var xpathArithmeticOperators = map[string]bool{"+": true, "-": true} + +// xpathFunctionArgumentOperators returns, for each XPath function call in a +// retrieve constraint whose argument is itself a `+` or `-` operation, +// the function name and the operator. Only an argument that IS the operation +// counts: `not(Price > $A + 1)` has a comparison for an argument and is not +// flagged — that shape was not measured. +func xpathFunctionArgumentOperators(expr ast.Expression) (hits [][2]string) { + var walk func(ast.Expression) + walk = func(e ast.Expression) { + switch n := e.(type) { + case *ast.FunctionCallExpr: + for _, arg := range n.Arguments { + inner := unwrapXPathArgument(arg) + if b, ok := inner.(*ast.BinaryExpr); ok && xpathArithmeticOperators[strings.ToLower(b.Operator)] { + hits = append(hits, [2]string{mendixFunctionName(n.Name), b.Operator}) + } + walk(arg) + } + case *ast.BinaryExpr: + walk(n.Left) + walk(n.Right) + case *ast.UnaryExpr: + walk(n.Operand) + case *ast.ParenExpr: + walk(n.Inner) + case *ast.SourceExpr: + walk(n.Expression) + case *ast.XPathPathExpr: + for _, s := range n.Steps { + walk(s.Expr) + walk(s.Predicate) + } + } + } + walk(expr) + return hits +} + +// unwrapXPathArgument strips the parentheses and source wrappers around an +// argument, down to the expression that is its value. +func unwrapXPathArgument(e ast.Expression) ast.Expression { + for { + switch n := e.(type) { + case *ast.ParenExpr: + e = n.Inner + case *ast.SourceExpr: + e = n.Expression + default: + return e + } + } +} + +// checkXPathFunctionArgumentOperators flags (MDL091) an operator inside an XPath +// function argument — the other way an expression construct gets into a +// retrieve constraint and out as CE0161 (mendixlabs/mxcli#1326). +func (v *microflowValidator) checkXPathFunctionArgumentOperators(variable string, where ast.Expression) { + for _, h := range xpathFunctionArgumentOperators(where) { + v.addViolation("MDL091", linter.SeverityError, + fmt.Sprintf("retrieve '$%s' constraint computes `%s` inside an argument of `%s()` — XPath does not "+ + "evaluate an operator in a function argument, and mxbuild reports CE0161 \"Error(s) in XPath constraint\"", + variable, h[1], h[0]), + fmt.Sprintf("Compute the value into a variable first, then pass the variable: "+ + "`declare $P String = ;` and `%s(, $P)`. An operator at the top "+ + "level of the constraint (`Name = 'X-' + $Key`) is fine.", h[0])) + } +} + // 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 diff --git a/mdl/executor/xpath_function_operator_test.go b/mdl/executor/xpath_function_operator_test.go new file mode 100644 index 0000000000..6b31e4625f --- /dev/null +++ b/mdl/executor/xpath_function_operator_test.go @@ -0,0 +1,83 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/visitor" +) + +// mendixlabs/mxcli#1326: an operator inside an XPath function argument passed +// check and exec, then failed the build with CE0161 on mxbuild 11.14.0 — the +// measured pairs are on xpathArithmeticOperators. The bracketed `[…]` form +// does not parse an operator in a function argument at all, so only the +// unbracketed form reached the writer. +func TestValidateMicroflow_XPathFunctionArgumentOperator(t *testing.T) { + cases := []struct { + name string + where string + wantMDL bool + }{ + {"concatenation in starts-with", "starts-with(Name, 'MS-' + $Key)", true}, + {"parenthesised argument", "contains(Name, ($Key + 'x'))", true}, + {"inside not()", "not(ends-with(Name, $Key + '-X'))", true}, + {"subtraction in contains", "contains(Name, $Key - 'a')", true}, + {"inside and", "Name != empty and ends-with(Name, '-' + $Key)", true}, + {"top-level concatenation is fine", "Name = 'X-' + $Key", false}, + {"operator beside a call is fine", "year-from-dateTime(Due) = $N - 1", false}, + {"pre-computed argument is fine", "starts-with(Name, $Key)", false}, + {"operator in a literal is text", "starts-with(Name, 'a + b')", false}, + {"comparison inside not() is fine", "not(Name = $Key)", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + src := "create microflow M.F ($Key: String, $N: Integer)\nreturns list of M.Item\nbegin\n retrieve $L from M.Item where " + tc.where + ";\n return $L;\nend;" + prog, errs := visitor.Build(src) + if len(errs) > 0 { + t.Fatalf("parse errors: %v", errs) + } + mf := prog.Statements[0].(*ast.CreateMicroflowStmt) + var msgs []string + for _, vi := range ValidateMicroflow(mf) { + if vi.RuleID == "MDL091" { + msgs = append(msgs, vi.Message) + } + } + if got := len(msgs) > 0; got != tc.wantMDL { + t.Fatalf("MDL091 fired=%v, want %v (where: %q) %v", got, tc.wantMDL, tc.where, msgs) + } + if tc.wantMDL && !strings.Contains(strings.Join(msgs, "\n"), "CE0161") { + t.Errorf("message must name CE0161: %v", msgs) + } + }) + } +} + +// check and exec must agree: the report had both accepting the constraint. +func TestCheckExecAgree_RetrieveConstraintFunctionArgumentOperator(t *testing.T) { + const head = `mdl 1; +create module ModG; +create persistent entity ModG.Item (Name: String(50)); +create microflow ModG.SUB_PlusInFunc ($Key: String) +begin + retrieve $L from ModG.Item where %s; +end;` + exec, _, dir := openPedAppCopy(t) + bad := strings.Replace(head, "%s", "starts-with(Name, 'MS-' + $Key)", 1) + got := strings.Join(agreeCheck(t, exec, dir, bad), "\n") + if !strings.Contains(got, "MDL091") || !strings.Contains(got, "starts-with()") { + t.Errorf("check -p must report MDL091 for the operator, reported:\n%s", got) + } + if err := agreeExec(t, exec, bad); err == nil || !strings.Contains(err.Error(), "MDL091") { + t.Errorf("exec must refuse the operator with MDL091, got: %v", err) + } + + // The two shapes the report measured clean must stay accepted. + for _, ok := range []string{"Name = 'X-' + $Key", "starts-with(Name, $Key)"} { + exec2, _, dir2 := openPedAppCopy(t) + assertAgree(t, exec2, dir2, strings.Replace(head, "%s", ok, 1), "") + } +}