From 058632d3499e37e14a9f7e95341665231a335bdc Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 2 Aug 2026 00:13:36 +0000 Subject: [PATCH] fix: describe silently dropped trailing XPath constraint groups (#772) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mendix stores sibling predicate groups concatenated in one XPathConstraint field: [Reminders.Task_TaskGroup/Reminders.TaskGroup[EndDate = $EndDateLimit]] [Status != 'Completed'] [CompletionDate = empty] The grammar's xpathConstraint rule matches ONE bracket group. ParseXPathConstraint removes the error listeners, so ANTLR parsed the first group, left the rest on the token stream, and still returned ok=true. enrichXPathConstraintForDescribe read that as a full parse and re-rendered only what came back — its `if !ok { return original }` fallback never fired — so describe emitted: where Reminders.Task_TaskGroup/Reminders.TaskGroup[EndDate = $EndDateLimit]; That is worse than a crash. The output looks complete while describing a materially less restrictive query than the project contains, which makes correct defensive code read as buggy — and `describe` is what an agent reads to decide whether code is right. Fixed in two layers: 1. ParseXPathConstraint reports a partial parse as a failure (require the token stream to be at EOF). That alone stops the data loss: the caller falls back to the stored string, which the render path then splits correctly. 2. visitor.SplitXPathPredicateGroups splits a constraint into its top-level groups, and each is enriched and rendered separately — so enum enrichment reaches groups after the first, not just the first. The splitter tracks nesting depth and quoting, because the previous "][" split mangled both a nested [A/B[x = 1]] and a literal containing ']'. The render path now uses it too. Verified end-to-end on a real 11.12.2 project carrying the reported constraint shape: all three groups render, Status is enriched to its qualified enum value in the second group, the output re-parses and re-executes to an identical flow, and `mx check` reports 0 errors. A/B against a pre-fix binary on the same project reproduces the two dropped groups exactly as reported. All three guards mutation-checked. Refs mendixlabs/mxcli#772 --- .claude/skills/fix-issue.md | 1 + .../bug-tests/772-xpath-constraint-groups.mdl | 68 ++++++++++++++++ mdl/executor/cmd_microflows_format_action.go | 64 ++++++++++----- mdl/executor/xpath_enrich_772_test.go | 65 +++++++++++++++ mdl/visitor/visitor_xpath_public.go | 12 +++ mdl/visitor/xpath_groups.go | 79 +++++++++++++++++++ mdl/visitor/xpath_groups_test.go | 79 +++++++++++++++++++ 7 files changed, 347 insertions(+), 21 deletions(-) create mode 100644 mdl-examples/bug-tests/772-xpath-constraint-groups.mdl create mode 100644 mdl/executor/xpath_enrich_772_test.go create mode 100644 mdl/visitor/xpath_groups.go create mode 100644 mdl/visitor/xpath_groups_test.go diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index f4b24d120..3b8136797 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -316,6 +316,7 @@ cases for these three BSON types — they fell to `default: return nil`. | `mdl/executor/cmd_microflows_format_action.go` | Added 3 formatter cases | | `mdl/executor/cmd_microflows_format_listop_test.go` | Added 4 formatter tests | | `sdk/mpr/parser_listoperation_test.go` | New file, 4 parser tests | +| `describe` (and `context` / `diff-local`) renders a Retrieve's XPath with only its **first** predicate group — `where A/B[EndDate = $X];` when the BSON holds `[A/B[EndDate = $X]][Status != 'Completed'][CompletionDate = empty]`. No warning; the output reads as a complete but materially *less restrictive* query, so correct defensive code looks buggy | The grammar's `xpathConstraint` rule matches ONE bracket group, and Mendix concatenates siblings. `ParseXPathConstraint` removes the error listeners, so ANTLR parsed group 1, left the rest on the token stream, and **still returned ok=true**; `enrichXPathConstraintForDescribe` treated that as a full parse and re-rendered only what came back. The `if !ok { return original }` fallback never fired | `mdl/visitor/visitor_xpath_public.go` (`ParseXPathConstraint`), `mdl/visitor/xpath_groups.go` (`SplitXPathPredicateGroups`), `mdl/executor/cmd_microflows_format_action.go` (`enrichXPathGroups`, and the render-path split) | Two layers. (1) Reject a partial parse — after the rule, require `stream.LA(1) == antlr.TokenEOF`; that alone stops the loss, since the caller then falls back to the stored string. (2) Split into top-level groups and enrich each, so enrichment still reaches groups after the first. The splitter must track **nesting depth and quoting**: a naive `][` split mangles a nested `[A/B[x = 1]]` and a literal containing `]`. **Generalisable**: a parser that silently accepts a prefix is worse than one that fails — any `ok` returned by a rule that can match less than its input must be checked against EOF before callers treat it as lossless. Repro `mdl-examples/bug-tests/772-xpath-constraint-groups.mdl`; A/B against a pre-fix binary on the same project shows the two dropped groups. Issue #772 | **Key insight:** `microflows$ListRange` stores offset/limit inside a nested `CustomRange` map — must cast `raw["CustomRange"].(map[string]any)` before diff --git a/mdl-examples/bug-tests/772-xpath-constraint-groups.mdl b/mdl-examples/bug-tests/772-xpath-constraint-groups.mdl new file mode 100644 index 000000000..9655c8fe4 --- /dev/null +++ b/mdl-examples/bug-tests/772-xpath-constraint-groups.mdl @@ -0,0 +1,68 @@ +-- Bug #772: describe silently dropped trailing XPath constraint groups +-- +-- Mendix stores sibling predicate groups concatenated in one XPathConstraint field: +-- +-- [Reminders.Task_TaskGroup/Reminders.TaskGroup[EndDate = $EndDateLimit]] +-- [Status != 'Completed'] +-- [CompletionDate = empty] +-- +-- The MDL grammar rule matches ONE bracket group. With the error listeners removed, +-- ANTLR parsed the first group, left the rest on the token stream, and still +-- reported success. enrichXPathConstraintForDescribe read that as a full parse and +-- re-rendered only what came back, so describe emitted: +-- +-- where Reminders.Task_TaskGroup/Reminders.TaskGroup[EndDate = $EndDateLimit]; +-- +-- The last two predicates vanished with no warning. That is worse than a crash: the +-- output looks complete while describing a materially LESS RESTRICTIVE query than +-- the project actually contains, which makes correct defensive code read as buggy. +-- +-- Two fixes, defence in depth: +-- 1. ParseXPathConstraint now reports a partial parse as a failure, so a caller +-- falls back to the stored string instead of silently truncating it. +-- 2. visitor.SplitXPathPredicateGroups splits the constraint into top-level groups +-- — tracking nesting depth and quoting, so a nested [..] or a ']' inside a +-- string literal is handled — and each group is enriched and rendered on its +-- own. Splitting on "][" mangled both of those. +-- +-- Verify: +-- 1. mxcli exec 772-xpath-constraint-groups.mdl -p app.mpr +-- 2. mxcli -p app.mpr -c "describe microflow Bug772.MF_GetOpenTasks" +-- All three groups must appear, and Status must render as the qualified +-- enum value Bug772.TaskStatus.Completed (enrichment reaches later groups too). +-- 3. Feed that describe output back through exec: it re-creates the same flow. +-- + +create module Bug772; +create module role Bug772.User; + +create enumeration Bug772.TaskStatus ( + Open 'Open', + Completed 'Completed' +); + +create persistent entity Bug772.TaskGroup ( + Name: String(100), + EndDate: DateTime +); + +create persistent entity Bug772.Task ( + Title: String(100), + Status: Enumeration(Bug772.TaskStatus), + CompletionDate: DateTime +); + +create association Bug772.Task_TaskGroup + from Bug772.Task to Bug772.TaskGroup + type Reference; + +create microflow Bug772.MF_GetOpenTasks ( + EndDateLimit: DateTime +) +returns list of Bug772.Task as $Tasks +begin + retrieve $Tasks from Bug772.Task + where '[Bug772.Task_TaskGroup/Bug772.TaskGroup[EndDate = $EndDateLimit]][Status != Bug772.TaskStatus.Completed][CompletionDate = empty]'; + return $Tasks; +end; +/ diff --git a/mdl/executor/cmd_microflows_format_action.go b/mdl/executor/cmd_microflows_format_action.go index 88e445230..464e92275 100644 --- a/mdl/executor/cmd_microflows_format_action.go +++ b/mdl/executor/cmd_microflows_format_action.go @@ -429,24 +429,18 @@ func formatAction( // (e.g. Status = 'Open' → Status = Module.OrderStatus.Open) when // the entity is known and we are connected to a project. constraint = enrichXPathConstraintForDescribe(ctx, entityName, constraint) - // XPath may contain multiple predicates like [a][b] or [a]\n[b]. - // Split them and join with MDL 'and' so the parser sees - // separate xpathConstraint nodes. - if strings.HasPrefix(constraint, "[") && strings.HasSuffix(constraint, "]") { - // Split on "][" boundary (possibly separated by \n literals), - // then re-wrap each predicate. - inner := constraint[1 : len(constraint)-1] - // Normalise real newlines between predicates: ]\n[ → ][ - inner = strings.ReplaceAll(inner, "]\n[", "][") - parts := strings.Split(inner, "][") - if len(parts) > 1 { - var wrapped []string - for _, p := range parts { - wrapped = append(wrapped, "["+strings.TrimSpace(p)+"]") - } - constraint = strings.Join(wrapped, "\n ") + // XPath may hold several sibling predicate groups — [a][b], or + // separated by newlines. Emit each on its own line so the parser + // sees separate xpathConstraint nodes. The splitter is nesting- and + // quote-aware: the previous "][" split mangled a group containing a + // nested bracket or a literal with a ']' in it (#772). + if groups := visitor.SplitXPathPredicateGroups(constraint); len(groups) > 0 { + if len(groups) > 1 { + constraint = strings.Join(groups, "\n ") } else { - constraint = parts[0] + // A lone group renders without its outer brackets, matching + // the `where ` form the MDL grammar expects. + constraint = strings.TrimSuffix(strings.TrimPrefix(groups[0], "["), "]") } } stmt += fmt.Sprintf("\n where %s", constraint) @@ -1879,10 +1873,38 @@ func enrichXPathConstraintForDescribe(ctx *ExecContext, entityQN, constraint str if len(enumAttrs) == 0 { return constraint } - expr, ok := visitor.ParseXPathConstraint(constraint) - if !ok || expr == nil { + return enrichXPathGroups(constraint, enumAttrs) +} + +// enrichXPathGroups applies enum enrichment to every top-level predicate group of a +// stored constraint. +// +// Mendix stores sibling groups concatenated (`[a][b][c]`), but the grammar rule +// matches one group. Enriching the whole string at once parsed only the first and +// re-rendered just that, silently discarding the rest — describe then showed a +// materially less restrictive query than the project actually contains +// (mendixlabs/mxcli#772). ParseXPathConstraint now refuses that partial parse, which +// stops the data loss on its own; splitting first is what keeps enrichment working +// for every group rather than only the first. +func enrichXPathGroups(constraint string, enumAttrs map[string]string) string { + groups := visitor.SplitXPathPredicateGroups(constraint) + if len(groups) == 0 { return constraint } - enriched := enrichXPathExprWithEnums(expr, enumAttrs) - return "[" + xpathExprToMDLString(enriched) + "]" + out := make([]string, 0, len(groups)) + for _, g := range groups { + out = append(out, enrichXPathGroup(g, enumAttrs)) + } + return strings.Join(out, "") +} + +// enrichXPathGroup enriches one bracket group, returning it unchanged when it does +// not parse — a group mxcli cannot read is passed through verbatim rather than +// dropped or guessed at. +func enrichXPathGroup(group string, enumAttrs map[string]string) string { + expr, ok := visitor.ParseXPathConstraint(group) + if !ok || expr == nil { + return group + } + return "[" + xpathExprToMDLString(enrichXPathExprWithEnums(expr, enumAttrs)) + "]" } diff --git a/mdl/executor/xpath_enrich_772_test.go b/mdl/executor/xpath_enrich_772_test.go new file mode 100644 index 000000000..ed6ae711e --- /dev/null +++ b/mdl/executor/xpath_enrich_772_test.go @@ -0,0 +1,65 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" +) + +// TestEnrichXPathGroups_KeepsEveryGroup is the regression test for +// mendixlabs/mxcli#772. The stored constraint holds three sibling predicate groups; +// enriching the whole string at once parsed only the first, re-rendered just that, +// and dropped the other two — so `describe` showed a materially less restrictive +// query than the project contains, with no warning. +func TestEnrichXPathGroups_KeepsEveryGroup(t *testing.T) { + enumAttrs := map[string]string{"Status": "Reminders.TaskStatus"} + + // Exactly the shape from the issue: a group with a nested bracket, then two + // flat ones, newline-separated. + const stored = "[Reminders.Task_TaskGroup/Reminders.TaskGroup[EndDate = $EndDateLimit]]\n" + + "[Status != 'Completed']\n[CompletionDate = empty]" + + got := enrichXPathGroups(stored, enumAttrs) + + for _, want := range []string{ + "Reminders.Task_TaskGroup/Reminders.TaskGroup[EndDate = $EndDateLimit]", + "CompletionDate = empty", + } { + if !strings.Contains(got, want) { + t.Errorf("enrichXPathGroups dropped %q\ngot: %s", want, got) + } + } + // The enum comparison must be enriched to a qualified value — in the *second* + // group, which is the one the old code never reached. + if !strings.Contains(got, "Status != Reminders.TaskStatus.Completed") { + t.Errorf("enum enrichment not applied to a later group\ngot: %s", got) + } + if strings.Contains(got, "'Completed'") { + t.Errorf("enum literal left unenriched\ngot: %s", got) + } +} + +// TestEnrichXPathGroups_PassesThroughUnparseable: a constraint mxcli cannot split +// into groups is returned untouched rather than mangled or dropped. +func TestEnrichXPathGroups_PassesThroughUnparseable(t *testing.T) { + enumAttrs := map[string]string{"Status": "Mod.S"} + for _, in := range []string{ + "Status = 'Open'", // no brackets + "[Status = 'Open'", // unbalanced + "[a = 1] and [b = 2]", // content between groups + } { + if got := enrichXPathGroups(in, enumAttrs); got != in { + t.Errorf("enrichXPathGroups(%q) = %q, want it returned verbatim", in, got) + } + } +} + +// TestEnrichXPathGroups_SingleGroupStillEnriched guards the fix from regressing the +// original single-group behaviour. +func TestEnrichXPathGroups_SingleGroupStillEnriched(t *testing.T) { + got := enrichXPathGroups("[Status = 'Open']", map[string]string{"Status": "Mod.S"}) + if got != "[Status = Mod.S.Open]" { + t.Errorf("enrichXPathGroups = %q, want %q", got, "[Status = Mod.S.Open]") + } +} diff --git a/mdl/visitor/visitor_xpath_public.go b/mdl/visitor/visitor_xpath_public.go index 270b466eb..7a1d0d043 100644 --- a/mdl/visitor/visitor_xpath_public.go +++ b/mdl/visitor/visitor_xpath_public.go @@ -12,6 +12,14 @@ import ( // [ ] brackets stored by Mendix in the XPathConstraint BSON field — and returns the // AST expression. Returns (nil, false) if the input cannot be parsed (e.g. empty, // malformed, or not starting with '['). +// +// The rule matches a SINGLE bracket group. Mendix stores sibling groups +// concatenated — `[a][b][c]` — and with the error listeners removed ANTLR happily +// parsed the first and left the rest on the stream, returning true. Callers read +// that as "fully parsed" and re-rendered only what came back, silently dropping +// every later group (mendixlabs/mxcli#772). A partial parse is therefore reported +// as a failure so callers fall back to the untouched string; use +// SplitXPathPredicateGroups to handle each group in turn. func ParseXPathConstraint(input string) (ast.Expression, bool) { if input == "" { return nil, false @@ -32,5 +40,9 @@ func ParseXPathConstraint(input string) (ast.Expression, bool) { if xpathExpr == nil { return nil, false } + // Anything left on the stream means the rule consumed only a prefix. + if stream.LA(1) != antlr.TokenEOF { + return nil, false + } return buildXPathExpr(xpathExpr), true } diff --git a/mdl/visitor/xpath_groups.go b/mdl/visitor/xpath_groups.go new file mode 100644 index 000000000..8eb8ca539 --- /dev/null +++ b/mdl/visitor/xpath_groups.go @@ -0,0 +1,79 @@ +// SPDX-License-Identifier: Apache-2.0 + +package visitor + +import "strings" + +// SplitXPathPredicateGroups splits a stored XPathConstraint into its top-level +// predicate groups, each returned with its enclosing brackets. Mendix concatenates +// sibling groups — `[a][b][c]`, often separated by newlines — and the whole string +// is what the BSON field holds. +// +// Splitting on "][" is not enough: a group may nest brackets of its own +// (`[Mod.Assoc/Mod.Entity[EndDate = $Limit]]`), and a string literal may contain a +// bracket (`[Name = 'a]b']`). Both appear in real projects, and mishandling either +// silently changes the meaning of a query (mendixlabs/mxcli#772). This tracks +// nesting depth and quoting instead. +// +// Returns nil when the input is not a well-formed sequence of bracket groups — +// unbalanced, empty, or with content outside the brackets — so callers can fall +// back to using the string verbatim rather than emit something they invented. +func SplitXPathPredicateGroups(constraint string) []string { + s := strings.TrimSpace(constraint) + if s == "" || !strings.HasPrefix(s, "[") { + return nil + } + + var groups []string + var depth int + var start int + var inQuote bool + + for i := 0; i < len(s); i++ { + c := s[i] + if inQuote { + // Mendix escapes a quote inside a literal by doubling it. + if c == '\'' { + if i+1 < len(s) && s[i+1] == '\'' { + i++ + continue + } + inQuote = false + } + continue + } + switch c { + case '\'': + inQuote = true + case '[': + if depth == 0 { + // Anything between groups must be whitespace only. + if strings.TrimSpace(s[start:i]) != "" { + return nil + } + start = i + } + depth++ + case ']': + depth-- + if depth < 0 { + return nil + } + if depth == 0 { + groups = append(groups, s[start:i+1]) + start = i + 1 + } + } + } + + if depth != 0 || inQuote { + return nil + } + if strings.TrimSpace(s[start:]) != "" { + return nil + } + if len(groups) == 0 { + return nil + } + return groups +} diff --git a/mdl/visitor/xpath_groups_test.go b/mdl/visitor/xpath_groups_test.go new file mode 100644 index 000000000..b7584572c --- /dev/null +++ b/mdl/visitor/xpath_groups_test.go @@ -0,0 +1,79 @@ +// SPDX-License-Identifier: Apache-2.0 + +package visitor + +import ( + "reflect" + "testing" +) + +func TestSplitXPathPredicateGroups(t *testing.T) { + tests := []struct { + name string + in string + want []string + }{ + { + // The constraint from mendixlabs/mxcli#772, verbatim: a group with a + // nested bracket followed by two flat ones, newline-separated. + name: "issue 772 — nested group plus siblings", + in: "[Reminders.Task_TaskGroup/Reminders.TaskGroup[EndDate = $EndDateLimit]]\n" + + "[Status != 'Completed']\n[CompletionDate = empty]", + want: []string{ + "[Reminders.Task_TaskGroup/Reminders.TaskGroup[EndDate = $EndDateLimit]]", + "[Status != 'Completed']", + "[CompletionDate = empty]", + }, + }, + { + name: "single group", + in: "[Status = 'Open']", + want: []string{"[Status = 'Open']"}, + }, + { + name: "adjacent groups, no separator", + in: "[a = 1][b = 2]", + want: []string{"[a = 1]", "[b = 2]"}, + }, + { + // A "][" split would cut this literal in half. + name: "bracket inside a string literal", + in: "[Name = 'a][b'][Status = 'Open']", + want: []string{"[Name = 'a][b']", "[Status = 'Open']"}, + }, + { + name: "doubled quote escape inside a literal", + in: "[Name = 'it''s]here'][Age = 3]", + want: []string{"[Name = 'it''s]here']", "[Age = 3]"}, + }, + {name: "empty", in: "", want: nil}, + {name: "not bracketed", in: "Status = 'Open'", want: nil}, + {name: "unbalanced open", in: "[a = 1", want: nil}, + {name: "unbalanced close", in: "[a = 1]]", want: nil}, + {name: "content between groups", in: "[a = 1] and [b = 2]", want: nil}, + {name: "unterminated literal", in: "[Name = 'x]", want: nil}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := SplitXPathPredicateGroups(tc.in) + if !reflect.DeepEqual(got, tc.want) { + t.Errorf("SplitXPathPredicateGroups(%q)\n got %#v\nwant %#v", tc.in, got, tc.want) + } + }) + } +} + +// TestParseXPathConstraint_RejectsPartialParse is the core of #772: the rule matches +// one bracket group, and with the error listeners removed ANTLR parsed the first and +// left the rest on the stream while still reporting success. Callers treated that as +// a full parse and re-rendered only what came back. +func TestParseXPathConstraint_RejectsPartialParse(t *testing.T) { + multi := "[Status != 'Completed'][CompletionDate = empty]" + if _, ok := ParseXPathConstraint(multi); ok { + t.Error("ParseXPathConstraint reported success on a multi-group constraint it only partly consumed") + } + // A single group must still parse. + if _, ok := ParseXPathConstraint("[Status != 'Completed']"); !ok { + t.Error("ParseXPathConstraint rejected a single well-formed group") + } +}