From 069a906a03f337a2f5196a912535350923035f5a Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 7 Sep 2026 07:06:00 +0000 Subject: [PATCH 1/5] feat(lint): report a module that has never been organised into folders (CONV018) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Studio Pro lets a module hold folders and every Mendix style guide expects them once a module is more than a handful of documents, but nothing in mxcli reported their absence: a project with 200 documents loose in one flat module scored clean across all three rule sets. CONV018 reports a module only when BOTH halves hold: 1. more than max_root_documents (default 20) documents sit directly in the module root, AND 2. not one document in the module is in a folder. The second half is what keeps it from nagging. A module that has started to organise itself — even one folder — is never reported, however much is still at its root: the team has evidently made a choice about where things go, and a linter guessing at the rest is noise. The first half exempts modules that are small rather than disorganised. Only kinds Studio Pro actually lets you file in a folder are counted. An association belongs to the domain model and an external entity to a consumed service, so counting them would report a module as unorganised on the strength of elements nobody can move. That list is an allow-list in the rule, where it is visible and editable, and its polarity is deliberate: a document type missing from it is undercounted, so the rule stays quiet rather than inventing a violation. The rule stands on a new documents() Starlark builtin — every element of the App Explorer tree as (kind, name, qualified_name, module_name, folder), read from the catalog's `objects` view so a new document type is covered without a second list to keep in step. It is the companion to documentable_elements(), which projects only what can carry documentation and therefore omits microflows and Java actions — the two kinds that fill up an unorganised module. No per-kind builtin exposed `folder` uniformly, which is why the rule could not be written before. Verified live, not only in unit tests: a 25-microflow flat module reports, and goes silent the moment one MOVE ... TO FOLDER lands; max_root_documents read from .claude/lint-config.yaml silences it at 40 and brings it back at 5. Tests carry their controls. The two silence assertions are paired with sibling tests that fire on the same fixture, the Marketplace exclusion is followed by the same module without the marker (which must report), and the non-foldered-kind test is followed by 40 rows of a kind that can be foldered (which must report). Stubbing check() to return [] fails four of the six. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JXnEgoc2NQP1Y2TWMCMXC4 --- .../conv018_module_folder_organization.star | 166 ++++++++++++ CHANGELOG.md | 10 + CLAUDE.md | 2 +- README.md | 2 +- docs-site/src/tools/linting.md | 2 +- docs-site/src/tools/starlark-rules.md | 48 +++- docs-site/src/tutorial/validation.md | 2 +- mdl/linter/context.go | 65 +++++ mdl/linter/report.go | 1 + mdl/linter/starlark.go | 27 ++ mdl/linter/starlark_folders_test.go | 251 ++++++++++++++++++ 11 files changed, 571 insertions(+), 5 deletions(-) create mode 100644 .claude/lint-rules/conv018_module_folder_organization.star create mode 100644 mdl/linter/starlark_folders_test.go diff --git a/.claude/lint-rules/conv018_module_folder_organization.star b/.claude/lint-rules/conv018_module_folder_organization.star new file mode 100644 index 0000000000..f2fe6eb819 --- /dev/null +++ b/.claude/lint-rules/conv018_module_folder_organization.star @@ -0,0 +1,166 @@ +# CONV018: Module Folder Organization +# +# Flags a module that has grown past a readable size while keeping every +# document loose in the module root — no folders at all. +# +# Studio Pro lets a module hold folders, and every Mendix style guide expects +# them once a module is more than a handful of documents. Nothing in mxcli +# reported their absence, so a project with 200 documents in one flat module +# scored clean. +# +# THE CONDITION IS DELIBERATELY NARROW, and both halves matter: +# +# 1. more than MAX_ROOT_DOCUMENTS documents sit directly in the module root, AND +# 2. NOT ONE document in the module is in a folder. +# +# The second half is what keeps this from becoming noise. A module that has +# started to organise itself — even one folder — is never flagged, however many +# documents remain at its root, because the team has evidently made a choice +# about where things go and a linter guessing at the rest would be nagging. The +# first half exempts genuinely small modules, where a folder tree costs more +# than it repays. Together they catch exactly one thing: a module nobody has +# ever organised. +# +# Options (.claude/lint-config.yaml): +# rules: +# CONV018: +# options: +# max_root_documents: 40 # default 20 — documents allowed at a module +# # root before folders are expected +# +# Document properties (documents()): +# .kind - catalog ObjectType: "MICROFLOW", "PAGE", "WORKFLOW", … +# .name - document name +# .qualified_name - Module.Name +# .module_name - the module holding it +# .folder - folder path, or "" when the document is in the module root + +RULE_ID = "CONV018" +RULE_NAME = "ModuleFolderOrganization" +DESCRIPTION = "Modules past a readable size should organise their documents into folders" +CATEGORY = "quality" +SEVERITY = "info" + +# Default; override with the max_root_documents option. +MAX_ROOT_DOCUMENTS = 20 + +# The kinds Studio Pro actually lets you file in a folder. +# +# `documents()` projects the whole catalog `objects` view, which includes rows +# whose Folder is hardcoded empty because the element is not a document at all — +# an association belongs to the domain model, an external entity to a consumed +# service, an entity to the Domain Model document. Counting those would report a +# module as unorganised on the strength of elements no one can move. +# +# An allow-list rather than a deny-list on purpose: a document type added to the +# catalog and forgotten here is undercounted, so the rule stays quiet. The other +# polarity would invent violations out of a new non-foldered kind. +FOLDERABLE_KINDS = [ + "MICROFLOW", + "NANOFLOW", + "RULE", + "PAGE", + "SNIPPET", + "BUILDING_BLOCK", + "LAYOUT", + "ENUMERATION", + "CONSTANT", + "JAVA_ACTION", + "JAVASCRIPT_ACTION", + "IMAGE_COLLECTION", + "ICON_COLLECTION", + "MENU", + "PAGE_TEMPLATE", + "SCHEDULED_EVENT", + "QUEUE", + "REGULAR_EXPRESSION", + "DATA_TRANSFORMER", + "WORKFLOW", + "AGENT", + "AI_MODEL", + "KNOWLEDGE_BASE", + "CONSUMED_MCP_SERVICE", + "DATABASE_CONNECTION", + "REST_CLIENT", + "PUBLISHED_REST_SERVICE", +] + +# The kinds a person recognises from the App Explorer, in the order a summary +# reads best. Anything else is counted but summarised under its own kind name. +KIND_LABELS = { + "MICROFLOW": "microflows", + "NANOFLOW": "nanoflows", + "PAGE": "pages", + "WORKFLOW": "workflows", + "SNIPPET": "snippets", + "ENUMERATION": "enumerations", + "JAVA_ACTION": "Java actions", + "JAVASCRIPT_ACTION": "JavaScript actions", + "LAYOUT": "layouts", + "RULE": "rules", +} + +def _summarise(counts): + """Render the two or three biggest kinds, so the message says what the + clutter actually IS rather than only how much of it there is.""" + pairs = [(kind, n) for kind, n in counts.items()] + pairs = sorted(pairs, key = lambda p: (-p[1], p[0])) + + parts = [] + for kind, n in pairs[:3]: + label = KIND_LABELS.get(kind, kind.lower().replace("_", " ")) + parts.append("{} {}".format(n, label)) + + if len(pairs) > 3: + parts.append("and more") + return ", ".join(parts) + +def check(): + max_root = get_option("max_root_documents", MAX_ROOT_DOCUMENTS) + + foldered = {} # module -> True once any document is in a folder + root_counts = {} # module -> {kind: count} for documents at the root + + for doc in documents(): + if doc.kind not in FOLDERABLE_KINDS: + continue + + module = doc.module_name + if module not in root_counts: + root_counts[module] = {} + foldered[module] = False + + if doc.folder != "": + foldered[module] = True + else: + counts = root_counts[module] + counts[doc.kind] = counts.get(doc.kind, 0) + 1 + + violations = [] + for module in sorted(root_counts.keys()): + if foldered[module]: + continue + + counts = root_counts[module] + total = 0 + for n in counts.values(): + total += n + + if total <= max_root: + continue + + violations.append(violation( + message = "Module '{}' keeps all {} of its documents ({}) in the module root and has no folders.".format( + module, + total, + _summarise(counts), + ), + location = location( + module = module, + document_type = "Module", + document_name = module, + ), + suggestion = "Group the documents into folders by feature or by type — `mxcli -p -c \"show structure in {}\"` to see the current layout, then MOVE documents into folders. Set CONV018.options.max_root_documents in .claude/lint-config.yaml if a flat module is intended here.".format(module), + )) + + return violations diff --git a/CHANGELOG.md b/CHANGELOG.md index b39c42e4f1..6f23672b59 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,16 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] +### Added + +- **`mxcli lint` reports a module nobody has ever organised (CONV018)** — Studio Pro lets a module hold folders and every Mendix style guide expects them, but nothing in mxcli reported their absence: a project with 200 documents loose in one flat module scored clean. It now reports one, and only when **both** halves hold — more than `max_root_documents` (default 20) documents sit directly in the module root, **and not one** document in the module is in a folder. + + The second half is what keeps it from nagging. A module that has started to organise itself — even one folder — is never reported, however much is still at its root: the team has evidently made a choice about where things go. Verified live on a 25-microflow flat module, which reports, and then goes silent the moment a single `MOVE … TO FOLDER` lands. Threshold via `.claude/lint-config.yaml`. + + Only kinds Studio Pro actually lets you file in a folder are counted — an association belongs to the domain model, an external entity to a consumed service, so counting them would report a module as unorganised on the strength of elements nobody can move. That list is an allow-list in the rule, where it is visible and editable, and its polarity is deliberate: a document type missing from it is undercounted, so the rule stays quiet rather than inventing a violation. + + Behind it is a new **`documents()`** Starlark builtin — every element of the App Explorer tree as `(kind, name, qualified_name, module_name, folder)`, read from the catalog's `objects` view so a new document type is covered without a second list to keep in step. It is the companion to `documentable_elements()`, which projects only what can carry documentation and so omits microflows and Java actions — the two kinds that fill up an unorganised module. + ### Fixed - **A Gallery with a non-default pagination was rejected as CE0463** (mendixlabs/mxcli#1035) — `pagination: 'loadMore'` and `pagination: 'virtualScrolling'` produced a widget mxbuild refuses with *"the definition of this widget has changed"*. The definition stored `pagingPosition: "below"`, which is not a member of the enumeration at all: the Gallery package declares `{bottom|top|both}`, and `"below"` is the first word of `bottom`'s **caption**, "Below grid". Mendix stores what mxcli writes and then rejects the widget. diff --git a/CLAUDE.md b/CLAUDE.md index c4e4c14b37..ab6700f5f9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -703,7 +703,7 @@ go build -o bin/mxcli ./cmd/mxcli | **Catalog queries** | `show catalog tables`, `select ... from CATALOG.table` | SQL querying of project metadata | | **Code search** | `show callers\|callees\|references\|impact\|context of ...` | Cross-reference navigation (requires `refresh catalog full`) | | **Full-text search** | `search 'keyword'` | Search across all strings and source | -| **Linting** | `mxcli lint -p app.mpr [--format json\|sarif]` | 15 built-in rules + 27 Starlark rules (MDL, SEC, QUAL, ARCH, DESIGN, CONV) | +| **Linting** | `mxcli lint -p app.mpr [--format json\|sarif]` | 15 built-in rules + 28 Starlark rules (MDL, SEC, QUAL, ARCH, DESIGN, CONV) | | **Report** | `mxcli report -p app.mpr [--format markdown\|json\|html]` | Scored best practices report with category breakdown | | **Testing** | `mxcli test tests/ -p app.mpr [--local] [--watch] [--attach]` | `.test.mdl` / `.test.md` files. `--local` runs on mxcli's own runtime (no Docker daemon), on its own ports + `_test` database, driving a **token-guarded test endpoint** (one microflow per test, invoked over HTTP — a throwing test fails only itself, results are returned not log-scraped). `--watch` keeps the runtime warm (~30s first run, then ~2s). `--attach` runs against an app already up under `run --local --test-endpoint` (no boot; uses **that app's** database) | | **Diff** | `mxcli diff -p app.mpr changes.mdl` | Compare script against project state | diff --git a/README.md b/README.md index aad286185c..6aa3679578 100644 --- a/README.md +++ b/README.md @@ -352,7 +352,7 @@ mxcli lint -p app.mpr --list-rules mxcli lint -p app.mpr --exclude System --exclude Administration ``` -14 built-in Go rules (MPR001-MPR007, SEC001-SEC003, CONV011-CONV014) plus 27 bundled Starlark rules covering security (SEC004-SEC009), architecture (ARCH001-003), quality (QUAL001-004), design (DESIGN001), and Mendix best practice conventions (CONV001-CONV010, CONV015-CONV017). Custom `.star` rules in `.claude/lint-rules/` are loaded automatically. +14 built-in Go rules (MPR001-MPR007, SEC001-SEC003, CONV011-CONV014) plus 28 bundled Starlark rules covering security (SEC004-SEC009), architecture (ARCH001-003), quality (QUAL001-004), design (DESIGN001), and Mendix best practice conventions (CONV001-CONV010, CONV015-CONV018). Custom `.star` rules in `.claude/lint-rules/` are loaded automatically. ### Best Practices Report diff --git a/docs-site/src/tools/linting.md b/docs-site/src/tools/linting.md index a884cf0d6a..422b00e69a 100644 --- a/docs-site/src/tools/linting.md +++ b/docs-site/src/tools/linting.md @@ -18,7 +18,7 @@ The linting system provides: |----------|--------|-------| | MDL | MDL001-MDL007 | Naming conventions, empty microflows, domain model size | | Security | SEC001-SEC009 | Access rules, password policy, demo users, PII exposure | -| Convention | CONV001-CONV017 | Best practice conventions, error handling | +| Convention | CONV001-CONV018 | Best practice conventions, error handling | | Quality | QUAL001-QUAL004 | Complexity, documentation, long microflows | | Architecture | ARCH001-ARCH003 | Cross-module data, entity business keys | | Design | DESIGN001 | Entity attribute count | diff --git a/docs-site/src/tools/starlark-rules.md b/docs-site/src/tools/starlark-rules.md index 8dffbaabe2..7f1265a4f0 100644 --- a/docs-site/src/tools/starlark-rules.md +++ b/docs-site/src/tools/starlark-rules.md @@ -38,7 +38,7 @@ In addition to the built-in Go rules, mxcli bundles 27 Starlark-based lint rules |------|-------------| | **DESIGN001** | Entity attribute count -- Warns when entities have too many attributes | -### Convention Rules (CONV001-CONV010, CONV015-CONV017) +### Convention Rules (CONV001-CONV010, CONV015-CONV018) | Rule | Description | |------|-------------| @@ -46,6 +46,7 @@ In addition to the built-in Go rules, mxcli bundles 27 Starlark-based lint rules | **CONV015** | Validation rules -- Checks for consistent validation patterns | | **CONV016** | Event handlers -- Validates event handler configuration | | **CONV017** | Calculated attributes -- Checks calculated attribute patterns | +| **CONV018** | Module folder organization -- A module past a readable size with every document loose in its root and no folders at all ([options](#conv018-options)) | Additional convention rules cover access rule constraints, role mapping, microflow size and content. @@ -125,6 +126,51 @@ and one in `_DOC_KINDS` (`missing_documentation.star`) giving the option name an the suggestion text. `TestQUAL002_SweepsEveryAdvertisedDocumentType` fails if the Go side advertises a kind the tests do not cover. +### CONV018 options {#conv018-options} + +Studio Pro lets a module hold folders, and every Mendix style guide expects them +once a module grows past a handful of documents. Nothing reported their absence, +so a project with 200 documents in one flat module scored clean. + +CONV018 reports a module only when **both** halves hold: + +1. more than `max_root_documents` documents sit directly in the module root, and +2. **not one** document in the module is in a folder. + +The second half is what keeps the rule from nagging. A module that has started to +organise itself — even one folder — is never reported, however much is still at +its root: the team has evidently made a choice about where things go, and a +linter guessing at the rest is noise. The first half exempts modules that are +small rather than disorganised. Together they catch one thing: a module nobody +has ever organised. + +| Option | Default | Meaning | +|--------|---------|---------| +| `max_root_documents` | `20` | Documents allowed at a module root before the module is expected to have folders | + +```yaml +rules: + CONV018: + enabled: true + options: + max_root_documents: 40 +``` + +Only the document kinds Studio Pro actually lets you file in a folder are +counted. Associations, entities and external entities are not documents — they +belong to the domain model or to a consumed service — so counting them would +report a module as unorganised on the strength of elements nobody can move. That +list is an allow-list in the rule (`FOLDERABLE_KINDS`), where it is visible and +editable: a document type missing from it is undercounted, so the rule stays +quiet rather than inventing a violation. + +The projection behind it is the `documents()` builtin — every element of the App +Explorer tree as `(kind, name, qualified_name, module_name, folder)`, read from +the catalog's `objects` view so a new document type is covered without a second +list to keep in step. It is the companion to `documentable_elements()`, which +projects only what can carry documentation and therefore leaves out microflows +and Java actions — the two kinds that fill up an unorganised module. + ## Where Starlark Rules Live When you run `mxcli init`, Starlark rules are installed to: diff --git a/docs-site/src/tutorial/validation.md b/docs-site/src/tutorial/validation.md index dccf436449..fc1c5a9a0b 100644 --- a/docs-site/src/tutorial/validation.md +++ b/docs-site/src/tutorial/validation.md @@ -157,7 +157,7 @@ For a broader set of checks across the entire project (not just a single script) mxcli lint -p app.mpr ``` -This runs 14 built-in rules plus 27 Starlark rules covering security, architecture, quality, and naming conventions. See `mxcli lint --list-rules` for the full list. +This runs 14 built-in rules plus 28 Starlark rules covering security, architecture, quality, and naming conventions. See `mxcli lint --list-rules` for the full list. For CI/CD integration, output in SARIF format: diff --git a/mdl/linter/context.go b/mdl/linter/context.go index a2012d52ee..a4f2df8bd0 100644 --- a/mdl/linter/context.go +++ b/mdl/linter/context.go @@ -1653,3 +1653,68 @@ func (ctx *LintContext) DocumentableElements() iter.Seq[Documentable] { } } } + +// Document is one element of the App Explorer tree — what it is, which module +// holds it, and the folder path it sits in ("" for the module root). +// +// Distinct from Documentable above, which projects only what can carry +// DOCUMENTATION and therefore leaves out microflows and Java actions on +// purpose. A rule asking where a document LIVES needs those two most of all: +// they are what fills up an unorganised module. +type Document struct { + Kind string // catalog ObjectType: "MICROFLOW", "PAGE", "WORKFLOW", … + Name string + QualifiedName string + ModuleName string + Folder string // "" = directly in the module root +} + +// Documents iterates every element of the catalog's `objects` view that belongs +// to a module, excluding System and Marketplace modules. +// +// It reads the view rather than a list of tables here, so a document type added +// to the catalog is covered without a second list to keep in step — the mistake +// behind #1036, where two keyword lists drifted with nothing comparing them. +// +// MODULE rows are skipped: a module is the container, not something inside one. +// Every other kind is yielded as-is, including kinds the view hardcodes to an +// empty Folder (associations, external entities). Deciding which kinds Studio +// Pro actually lets you file in a folder is left to the caller, where it is +// visible and editable, rather than baked into a second list in here. +func (ctx *LintContext) Documents() iter.Seq[Document] { + return func(yield func(Document) bool) { + rows, err := ctx.db.Query(` + SELECT o.ObjectType, o.Name, o.QualifiedName, o.ModuleName, COALESCE(o.Folder, '') + FROM objects o + LEFT JOIN modules m ON o.ModuleName = m.Name + WHERE o.ObjectType <> 'MODULE' + AND COALESCE(o.ModuleName, '') <> '' + AND ` + notPlatformModule("m") + ` + ORDER BY o.ModuleName, o.ObjectType, o.Name + `) + if err != nil { + ctx.recordQueryError("Documents(objects)", err) + return + } + defer rows.Close() + + for rows.Next() { + var d Document + var qn sql.NullString + if err := rows.Scan(&d.Kind, &d.Name, &qn, &d.ModuleName, &d.Folder); err != nil { + ctx.recordQueryError("Documents(objects) row scan", err) + continue + } + d.QualifiedName = qn.String + if d.QualifiedName == "" { + d.QualifiedName = d.Name + } + if ctx.IsExcluded(d.ModuleName) { + continue + } + if !yield(d) { + return + } + } + } +} diff --git a/mdl/linter/report.go b/mdl/linter/report.go index ab73d79f62..bf7ba526af 100644 --- a/mdl/linter/report.go +++ b/mdl/linter/report.go @@ -67,6 +67,7 @@ var categoryMapping = map[string]string{ "CONV012": "Quality", "CONV014": "Quality", "CONV015": "Quality", + "CONV018": "Quality", // Architecture "ARCH001": "Architecture", diff --git a/mdl/linter/starlark.go b/mdl/linter/starlark.go index 42c92f977a..103be381c9 100644 --- a/mdl/linter/starlark.go +++ b/mdl/linter/starlark.go @@ -318,6 +318,7 @@ func (r *StarlarkRule) buildPredeclared() starlark.StringDict { "microflows": starlark.NewBuiltin("microflows", r.builtinMicroflows), "java_actions": starlark.NewBuiltin("java_actions", r.builtinJavaActions), "documentable_elements": starlark.NewBuiltin("documentable_elements", r.builtinDocumentableElements), + "documents": starlark.NewBuiltin("documents", r.builtinDocuments), "pages": starlark.NewBuiltin("pages", r.builtinPages), "enumerations": starlark.NewBuiltin("enumerations", r.builtinEnumerations), "constants": starlark.NewBuiltin("constants", r.builtinConstants), @@ -440,6 +441,32 @@ func (r *StarlarkRule) builtinDocumentableElements(_ *starlark.Thread, _ *starla return starlark.NewList(out), nil } +// builtinDocuments returns every element of the App Explorer tree as a uniform +// (kind, name, qualified_name, module_name, folder) projection. +// +// The companion to documentable_elements for rules about where a document +// LIVES rather than what it says: it covers microflows and Java actions, which +// that projection deliberately omits, and it carries `folder`, which no +// per-kind builtin exposes uniformly. +func (r *StarlarkRule) builtinDocuments(_ *starlark.Thread, _ *starlark.Builtin, args starlark.Tuple, kwargs []starlark.Tuple) (starlark.Value, error) { + if r.ctx == nil { + return starlark.NewList(nil), nil + } + + var out []starlark.Value + for d := range r.ctx.Documents() { + out = append(out, starlarkstruct.FromStringDict(starlark.String("document"), starlark.StringDict{ + "kind": starlark.String(d.Kind), + "name": starlark.String(d.Name), + "qualified_name": starlark.String(d.QualifiedName), + "module_name": starlark.String(d.ModuleName), + "folder": starlark.String(d.Folder), + })) + } + + return starlark.NewList(out), nil +} + // builtinPages returns an iterator over pages. func (r *StarlarkRule) builtinPages(_ *starlark.Thread, _ *starlark.Builtin, args starlark.Tuple, kwargs []starlark.Tuple) (starlark.Value, error) { if r.ctx == nil { diff --git a/mdl/linter/starlark_folders_test.go b/mdl/linter/starlark_folders_test.go new file mode 100644 index 0000000000..f619d0ec01 --- /dev/null +++ b/mdl/linter/starlark_folders_test.go @@ -0,0 +1,251 @@ +// SPDX-License-Identifier: Apache-2.0 + +package linter_test + +import ( + "fmt" + "path/filepath" + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/catalog" + "github.com/mendixlabs/mxcli/mdl/linter" +) + +// folderFixture builds a catalog with one user module whose documents are laid +// out by the caller: `rootPages`/`rootFlows` land in the module root, and +// `folderedFlows` land in a folder. +// +// Rows go in directly rather than through the MPR reader — the subject is the +// `objects` projection and the rule on top of it, not the parser. +func folderFixture(t *testing.T, module string, rootPages, rootFlows, folderedFlows int) *catalog.Catalog { + t.Helper() + + cat, err := catalog.NewFromFile(filepath.Join(t.TempDir(), "cat.db")) + if err != nil { + t.Fatalf("NewFromFile: %v", err) + } + t.Cleanup(func() { cat.Close() }) + db := cat.CatalogDB() + + exec := func(q string, args ...any) { + t.Helper() + if _, err := db.Exec(q, args...); err != nil { + t.Fatalf("exec %s: %v", q, err) + } + } + + exec(`INSERT INTO modules_data (Id, Name, ProjectId, SnapshotId) VALUES (?,?,?,?)`, + "mod-"+module, module, "default", "s1") + + for i := 0; i < rootPages; i++ { + name := fmt.Sprintf("Page_%d", i) + exec(`INSERT INTO pages_data (Id, Name, QualifiedName, ModuleName, Folder, ProjectId, SnapshotId) + VALUES (?,?,?,?,?,?,?)`, + fmt.Sprintf("pg-%s-%d", module, i), name, module+"."+name, module, "", "default", "s1") + } + addFlow := func(i int, folder string) { + name := fmt.Sprintf("ACT_Flow_%d", i) + exec(`INSERT INTO microflows_data + (Id, Name, QualifiedName, ModuleName, Folder, MicroflowType, ProjectId, SnapshotId) + VALUES (?,?,?,?,?,?,?,?)`, + fmt.Sprintf("mf-%s-%d", module, i), name, module+"."+name, module, folder, + "MICROFLOW", "default", "s1") + } + for i := 0; i < rootFlows; i++ { + addFlow(i, "") + } + for i := 0; i < folderedFlows; i++ { + addFlow(1000+i, "Ordering") + } + + return cat +} + +func runFolderRule(t *testing.T, cat *catalog.Catalog, options map[string]any) []linter.Violation { + t.Helper() + // The rule that ships, loaded from disk. A copy inlined here would prove the + // builtin works and say nothing about whether CONV018 uses it. + rule, err := linter.LoadStarlarkRule("../../.claude/lint-rules/conv018_module_folder_organization.star") + if err != nil { + t.Fatalf("loading the shipped CONV018: %v", err) + } + if options != nil { + rule.Configure(options) + } + return rule.Check(linter.NewLintContext(cat, &minimalReader{})) +} + +func messages(vs []linter.Violation) string { + var out []string + for _, v := range vs { + out = append(out, v.Message) + } + return strings.Join(out, "\n") +} + +// The reported project: everything in the root of a single module, no folders. +func TestCONV018_FlagsAFlatModule(t *testing.T) { + vs := runFolderRule(t, folderFixture(t, "Sales", 15, 10, 0), nil) + + if len(vs) != 1 { + t.Fatalf("got %d violations, want 1:\n%s", len(vs), messages(vs)) + } + if vs[0].RuleID != "CONV018" { + t.Errorf("RuleID = %q, want CONV018", vs[0].RuleID) + } + got := vs[0].Message + // The count must be the real total across kinds, not one kind's worth: the + // whole point is that microflows and pages pile up together. + for _, want := range []string{"Sales", "all 25 of its documents", "15 pages", "10 microflows"} { + if !strings.Contains(got, want) { + t.Errorf("message missing %q:\n%s", want, got) + } + } +} + +// The exemption that keeps this from being noise. A module that has started to +// organise itself is left alone however much is still at its root — the team has +// made a choice about where things go. +func TestCONV018_SilentOnceAnyFolderExists(t *testing.T) { + vs := runFolderRule(t, folderFixture(t, "Sales", 15, 10, 1), nil) + if len(vs) != 0 { + t.Errorf("a module with a folder was flagged:\n%s", messages(vs)) + } +} + +// A small module is not disorganised, it is small. +func TestCONV018_SilentBelowTheThreshold(t *testing.T) { + vs := runFolderRule(t, folderFixture(t, "Sales", 10, 10, 0), nil) + if len(vs) != 0 { + t.Errorf("a 20-document module was flagged at the default threshold of 20:\n%s", messages(vs)) + } +} + +// The threshold is the knob a team reaches for first, so it has to actually be +// wired to the option and not only to the module-level default. +func TestCONV018_ThresholdIsConfigurable(t *testing.T) { + cat := folderFixture(t, "Sales", 15, 10, 0) + + if vs := runFolderRule(t, cat, map[string]any{"max_root_documents": 40}); len(vs) != 0 { + t.Errorf("raising the threshold to 40 did not silence a 25-document module:\n%s", messages(vs)) + } + // Control: the same catalog at a threshold below 25 still reports, so the + // silence above is the option taking effect and not the fixture going empty. + if vs := runFolderRule(t, cat, map[string]any{"max_root_documents": 5}); len(vs) != 1 { + t.Errorf("got %d violations at a threshold of 5, want 1:\n%s", len(vs), messages(vs)) + } +} + +// Marketplace and System modules hold documents the user did not write and +// cannot reorganise — an update replaces the module. Reporting them would bury +// the finding that is actionable. +func TestCONV018_ExcludesPlatformModules(t *testing.T) { + cat := folderFixture(t, "Sales", 15, 10, 1) // organised: the user module stays quiet + db := cat.CatalogDB() + + if _, err := db.Exec( + `INSERT INTO modules_data (Id, Name, Source, ProjectId, SnapshotId) VALUES (?,?,?,?,?)`, + "mod-mp", "CommunityCommons", "Marketplace", "default", "s1"); err != nil { + t.Fatalf("insert module: %v", err) + } + for i := 0; i < 30; i++ { + name := fmt.Sprintf("MP_Flow_%d", i) + if _, err := db.Exec(`INSERT INTO microflows_data + (Id, Name, QualifiedName, ModuleName, Folder, MicroflowType, ProjectId, SnapshotId) + VALUES (?,?,?,?,?,?,?,?)`, + fmt.Sprintf("mf-mp-%d", i), name, "CommunityCommons."+name, "CommunityCommons", "", + "MICROFLOW", "default", "s1"); err != nil { + t.Fatalf("insert microflow: %v", err) + } + } + + if vs := runFolderRule(t, cat, nil); len(vs) != 0 { + t.Errorf("a Marketplace module was flagged:\n%s", messages(vs)) + } + + // Control: the identical 30 flat microflows in a module the user owns DO + // report. Without this the test passes against a rule that never fires. + if _, err := db.Exec(`UPDATE modules_data SET Source = '' WHERE Id = ?`, "mod-mp"); err != nil { + t.Fatalf("update module source: %v", err) + } + vs := runFolderRule(t, cat, nil) + if len(vs) != 1 || !strings.Contains(vs[0].Message, "CommunityCommons") { + t.Fatalf("control failed: the same module without the Marketplace marker was not reported; got %d:\n%s", + len(vs), messages(vs)) + } +} + +// An association has no folder and cannot be given one — it belongs to the +// domain model. Counting rows the `objects` view hardcodes to an empty folder +// would report a module as unorganised on the strength of elements nobody can +// move. The fixture's user module is under the threshold on real documents, so +// any violation here is entirely made of associations. +func TestCONV018_IgnoresElementsThatCannotBeFoldered(t *testing.T) { + cat := folderFixture(t, "Sales", 5, 5, 0) + db := cat.CatalogDB() + + for i := 0; i < 40; i++ { + name := fmt.Sprintf("Sales.A_%d", i) + if _, err := db.Exec(`INSERT INTO associations_data + (Id, Name, QualifiedName, ModuleName, ProjectId, SnapshotId) + VALUES (?,?,?,?,?,?)`, + fmt.Sprintf("as-%d", i), name, name, "Sales", "default", "s1"); err != nil { + t.Fatalf("insert association: %v", err) + } + } + + if vs := runFolderRule(t, cat, nil); len(vs) != 0 { + t.Errorf("associations were counted as loose documents:\n%s", messages(vs)) + } + + // Control: 40 rows of a kind that CAN be foldered, in the same module, do + // take it over the threshold — so the silence above is the kind filter and + // not the fixture failing to reach the rule. + for i := 0; i < 40; i++ { + name := fmt.Sprintf("Extra_%d", i) + if _, err := db.Exec(`INSERT INTO pages_data + (Id, Name, QualifiedName, ModuleName, Folder, ProjectId, SnapshotId) + VALUES (?,?,?,?,?,?,?)`, + fmt.Sprintf("pg-x-%d", i), name, "Sales."+name, "Sales", "", "default", "s1"); err != nil { + t.Fatalf("insert page: %v", err) + } + } + if vs := runFolderRule(t, cat, nil); len(vs) != 1 { + t.Fatalf("control failed: 40 loose pages were not reported; got %d:\n%s", len(vs), messages(vs)) + } +} + +// Documents() is the projection the rule stands on: it must carry the folder and +// must reach microflows, which documentable_elements deliberately omits. +func TestDocuments_CarryFolderAndCoverFlows(t *testing.T) { + ctx := linter.NewLintContext(folderFixture(t, "Sales", 2, 2, 1), &minimalReader{}) + + byName := map[string]linter.Document{} + for d := range ctx.Documents() { + byName[d.QualifiedName] = d + } + + root, ok := byName["Sales.ACT_Flow_0"] + if !ok { + t.Fatalf("microflows are missing from Documents(): %v", byName) + } + if root.Kind != "MICROFLOW" || root.Folder != "" { + t.Errorf("root microflow = %+v, want kind MICROFLOW and an empty folder", root) + } + + foldered, ok := byName["Sales.ACT_Flow_1000"] + if !ok { + t.Fatal("the foldered microflow is missing from Documents()") + } + if foldered.Folder != "Ordering" { + t.Errorf("folder = %q, want %q — the folder is what the rule reads", foldered.Folder, "Ordering") + } + + // A module is a container, not something inside one. + for qn, d := range byName { + if d.Kind == "MODULE" { + t.Errorf("Documents() yielded a MODULE row: %s", qn) + } + } +} From 5164a6107b8e26a7e46dbf588ea005e2c5bb182a Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 7 Sep 2026 08:03:36 +0000 Subject: [PATCH 2/5] feat(lint): report a navigation screen that cannot be linked to (CONV019) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A Mendix page is reachable at /p/ only if it has been given a URL. Without one it exists solely at the end of a click path: it cannot be bookmarked, linked to from an email, reopened after a browser refresh, or captured with `mxcli run --local --screenshot-url`, so verifying one screen means driving a browser through login and the menu (ako/CapTrackV4 FINDINGS 014, R4). The rule reports only the pages a navigation profile ROUTES TO — the profile home page, role home pages, and menu item targets. That narrowing is the whole design, and it is measured rather than assumed: on a blank Mendix 11.12.1 app 0 of 16 pages carry a URL, Mendix's own Home page and every Administration and FeedbackModule page included. Reporting every page without one would warn about every page of every project from the day it is created, which is noise rather than a finding. The screens a profile routes to are different: they are what a user lands on, bookmarks and shares. A page reached only from a button inside another screen is never reported. Also never reported, each for its own reason: the login and not-found pages (the platform routes to those itself, so a URL buys nothing), microflow-valued menu items and home pages (the microflow decides what opens, so there is no page to be addressable), and targets in System or Marketplace modules. A page routed to several ways is one finding naming every route, not one per route. Carried by a new navigation_targets() Starlark builtin — (profile, kind, role, caption, page) for every routed-to page, where kind is home, role_home or menu. Navigation was reachable only from the Go rules through the reader's GetNavigation, so no Starlark rule could ask which pages a user can actually reach. Verified live, not only in unit tests: on a blank app it reports Home_Web and nothing else, naming both routes that reach it; two pages created in one statement — one with `Url:`, one without — separate exactly as intended, the linked one silent and the unlinked one reported. Tests carry their controls. The URL-set test asserts the other two still report, the Marketplace test asserts the three user pages still do, and the two projection tests assert the surviving targets rather than only the absent ones. Stubbing check() to return [] fails four of the six. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JXnEgoc2NQP1Y2TWMCMXC4 --- .../conv019_navigation_page_url.star | 96 +++++++ CHANGELOG.md | 8 + CLAUDE.md | 2 +- README.md | 2 +- docs-site/src/tools/linting.md | 2 +- docs-site/src/tools/starlark-rules.md | 46 +++- docs-site/src/tutorial/validation.md | 2 +- mdl/linter/context.go | 56 ++++ mdl/linter/report.go | 1 + mdl/linter/starlark.go | 26 ++ mdl/linter/starlark_navigation_url_test.go | 258 ++++++++++++++++++ 11 files changed, 493 insertions(+), 6 deletions(-) create mode 100644 .claude/lint-rules/conv019_navigation_page_url.star create mode 100644 mdl/linter/starlark_navigation_url_test.go diff --git a/.claude/lint-rules/conv019_navigation_page_url.star b/.claude/lint-rules/conv019_navigation_page_url.star new file mode 100644 index 0000000000..e2e32590e3 --- /dev/null +++ b/.claude/lint-rules/conv019_navigation_page_url.star @@ -0,0 +1,96 @@ +# CONV019: Navigation Pages Should Be Addressable +# +# Flags a page a navigation profile routes to that has no URL. +# +# A Mendix page is only reachable at `/p/` if it has been given one. Without +# it the page exists solely at the end of a click path: it cannot be bookmarked, +# linked to from an email, reopened after a refresh, or captured directly by +# `mxcli run --local --screenshot-url` — verifying one screen means driving a +# browser through login and the menu (ako/CapTrackV4 FINDINGS 014, R4). +# +# WHY ONLY NAVIGATION TARGETS. Measured on a blank Mendix 11.12.1 app: 0 of 16 +# pages carry a URL, Mendix's own included. Reporting every page without one +# would warn about every page of every project from the day it is created, which +# is noise rather than a finding. The pages a profile routes to are different: +# they are the app's top-level screens, the ones a user lands on, bookmarks and +# shares. A page reached only from a button inside another screen is not +# expected to be addressable and is never reported. +# +# Deliberately not reported: +# - the login page and the not-found page — the platform routes to those +# itself, so a URL on them buys nothing; +# - microflow-valued menu items and home pages — the microflow decides what +# opens, so there is no page here to be addressable; +# - targets in System and Marketplace modules — that is somebody else's model, +# and it drops out of the join against pages() rather than being listed here. +# +# Options (.claude/lint-config.yaml): +# rules: +# CONV019: +# enabled: false # a project that does not want deep links at all +# +# Navigation target properties (navigation_targets()): +# .profile - "Responsive", "Phone", "Tablet", … +# .kind - "home", "role_home" or "menu" +# .role - user role, for a role_home +# .caption - menu item caption, for a menu target +# .page - qualified page name +# +# Page properties (pages()): +# .qualified_name, .url - "" when the page has no URL + +RULE_ID = "CONV019" +RULE_NAME = "NavigationPageURL" +DESCRIPTION = "Pages reachable from navigation should have a URL so they can be linked to directly" +CATEGORY = "quality" +SEVERITY = "info" + +def _describe(target): + """How this page is reached, in the words the navigation document uses.""" + if target.kind == "home": + return "the home page of the '{}' profile".format(target.profile) + if target.kind == "role_home": + return "the home page for role '{}' in the '{}' profile".format(target.role, target.profile) + if target.caption != "": + return "the '{}' menu item in the '{}' profile".format(target.caption, target.profile) + return "a menu item in the '{}' profile".format(target.profile) + +def check(): + # Pages the linter can see. A target missing from this map is in a System or + # Marketplace module — not the user's page to give a URL to. + urls = {} + modules = {} + for page in pages(): + urls[page.qualified_name] = page.url + modules[page.qualified_name] = page.module_name + + # One page can be routed to several ways (home page AND a menu item). Report + # the page once, naming every route, rather than once per route. + routes = {} + for target in navigation_targets(): + if target.page not in urls: + continue + if urls[target.page] != "": + continue + routes.setdefault(target.page, []).append(_describe(target)) + + violations = [] + for page in sorted(routes.keys()): + how = routes[page] + violations.append(violation( + message = "Page '{}' is reachable from navigation ({}) but has no URL, so it cannot be linked to or bookmarked.".format( + page, + ", ".join(how), + ), + location = location( + module = modules[page], + document_type = "Page", + document_name = page, + ), + suggestion = "Give the page a URL — `Url: '{}'` on CREATE PAGE, or the URL property in Studio Pro — so it is reachable at /p/{}. Disable CONV019 in .claude/lint-config.yaml if this app deliberately has no deep links.".format( + page.split(".")[-1].lower(), + page.split(".")[-1].lower(), + ), + )) + + return violations diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f23672b59..f8dc8502fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,14 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Added +- **`mxcli lint` reports a navigation screen that cannot be linked to (CONV019)** — a Mendix page is reachable at `/p/` only if it has been given a URL; without one it exists solely at the end of a click path and cannot be bookmarked, shared, reopened after a refresh, or captured with `--screenshot-url` (ako/CapTrackV4 FINDINGS 014). + + It reports only the pages a **navigation profile routes to** — the profile home page, role home pages, and menu item targets. Measured on a blank Mendix 11.12.1 app, **0 of 16 pages carry a URL**, Mendix's own Home page included, so reporting every page without one would warn about every page of every project from the day it is created. The screens a profile routes to are the ones a user lands on and shares; a page reached only from a button inside another screen is never reported. Neither are login and not-found pages (the platform routes to those itself), microflow-valued targets, or Marketplace pages. A page routed to several ways is one finding naming every route. + + Verified live on a blank app: it reports `Home_Web` and nothing else, and two pages created in one statement — one with `Url:`, one without — separate exactly as intended. + + A new **`navigation_targets()`** Starlark builtin carries it: `(profile, kind, role, caption, page)` for every routed-to page. Navigation was previously reachable only from the Go rules, so no Starlark rule could ask which pages a user can actually reach. + - **`mxcli lint` reports a module nobody has ever organised (CONV018)** — Studio Pro lets a module hold folders and every Mendix style guide expects them, but nothing in mxcli reported their absence: a project with 200 documents loose in one flat module scored clean. It now reports one, and only when **both** halves hold — more than `max_root_documents` (default 20) documents sit directly in the module root, **and not one** document in the module is in a folder. The second half is what keeps it from nagging. A module that has started to organise itself — even one folder — is never reported, however much is still at its root: the team has evidently made a choice about where things go. Verified live on a 25-microflow flat module, which reports, and then goes silent the moment a single `MOVE … TO FOLDER` lands. Threshold via `.claude/lint-config.yaml`. diff --git a/CLAUDE.md b/CLAUDE.md index ab6700f5f9..e2187300eb 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -703,7 +703,7 @@ go build -o bin/mxcli ./cmd/mxcli | **Catalog queries** | `show catalog tables`, `select ... from CATALOG.table` | SQL querying of project metadata | | **Code search** | `show callers\|callees\|references\|impact\|context of ...` | Cross-reference navigation (requires `refresh catalog full`) | | **Full-text search** | `search 'keyword'` | Search across all strings and source | -| **Linting** | `mxcli lint -p app.mpr [--format json\|sarif]` | 15 built-in rules + 28 Starlark rules (MDL, SEC, QUAL, ARCH, DESIGN, CONV) | +| **Linting** | `mxcli lint -p app.mpr [--format json\|sarif]` | 15 built-in rules + 29 Starlark rules (MDL, SEC, QUAL, ARCH, DESIGN, CONV) | | **Report** | `mxcli report -p app.mpr [--format markdown\|json\|html]` | Scored best practices report with category breakdown | | **Testing** | `mxcli test tests/ -p app.mpr [--local] [--watch] [--attach]` | `.test.mdl` / `.test.md` files. `--local` runs on mxcli's own runtime (no Docker daemon), on its own ports + `_test` database, driving a **token-guarded test endpoint** (one microflow per test, invoked over HTTP — a throwing test fails only itself, results are returned not log-scraped). `--watch` keeps the runtime warm (~30s first run, then ~2s). `--attach` runs against an app already up under `run --local --test-endpoint` (no boot; uses **that app's** database) | | **Diff** | `mxcli diff -p app.mpr changes.mdl` | Compare script against project state | diff --git a/README.md b/README.md index 6aa3679578..624703250f 100644 --- a/README.md +++ b/README.md @@ -352,7 +352,7 @@ mxcli lint -p app.mpr --list-rules mxcli lint -p app.mpr --exclude System --exclude Administration ``` -14 built-in Go rules (MPR001-MPR007, SEC001-SEC003, CONV011-CONV014) plus 28 bundled Starlark rules covering security (SEC004-SEC009), architecture (ARCH001-003), quality (QUAL001-004), design (DESIGN001), and Mendix best practice conventions (CONV001-CONV010, CONV015-CONV018). Custom `.star` rules in `.claude/lint-rules/` are loaded automatically. +14 built-in Go rules (MPR001-MPR007, SEC001-SEC003, CONV011-CONV014) plus 29 bundled Starlark rules covering security (SEC004-SEC009), architecture (ARCH001-003), quality (QUAL001-004), design (DESIGN001), and Mendix best practice conventions (CONV001-CONV010, CONV015-CONV019). Custom `.star` rules in `.claude/lint-rules/` are loaded automatically. ### Best Practices Report diff --git a/docs-site/src/tools/linting.md b/docs-site/src/tools/linting.md index 422b00e69a..959e9e70cd 100644 --- a/docs-site/src/tools/linting.md +++ b/docs-site/src/tools/linting.md @@ -18,7 +18,7 @@ The linting system provides: |----------|--------|-------| | MDL | MDL001-MDL007 | Naming conventions, empty microflows, domain model size | | Security | SEC001-SEC009 | Access rules, password policy, demo users, PII exposure | -| Convention | CONV001-CONV018 | Best practice conventions, error handling | +| Convention | CONV001-CONV019 | Best practice conventions, error handling | | Quality | QUAL001-QUAL004 | Complexity, documentation, long microflows | | Architecture | ARCH001-ARCH003 | Cross-module data, entity business keys | | Design | DESIGN001 | Entity attribute count | diff --git a/docs-site/src/tools/starlark-rules.md b/docs-site/src/tools/starlark-rules.md index 7f1265a4f0..c3818abffe 100644 --- a/docs-site/src/tools/starlark-rules.md +++ b/docs-site/src/tools/starlark-rules.md @@ -1,6 +1,6 @@ # Starlark Rules -In addition to the built-in Go rules, mxcli bundles 27 Starlark-based lint rules. Starlark is a Python-like language that allows rules to be extended and customized without recompiling mxcli. +In addition to the built-in Go rules, mxcli bundles 29 Starlark-based lint rules. Starlark is a Python-like language that allows rules to be extended and customized without recompiling mxcli. ## Bundled Starlark Rules @@ -38,7 +38,7 @@ In addition to the built-in Go rules, mxcli bundles 27 Starlark-based lint rules |------|-------------| | **DESIGN001** | Entity attribute count -- Warns when entities have too many attributes | -### Convention Rules (CONV001-CONV010, CONV015-CONV018) +### Convention Rules (CONV001-CONV010, CONV015-CONV019) | Rule | Description | |------|-------------| @@ -47,6 +47,7 @@ In addition to the built-in Go rules, mxcli bundles 27 Starlark-based lint rules | **CONV016** | Event handlers -- Validates event handler configuration | | **CONV017** | Calculated attributes -- Checks calculated attribute patterns | | **CONV018** | Module folder organization -- A module past a readable size with every document loose in its root and no folders at all ([options](#conv018-options)) | +| **CONV019** | Navigation page URL -- A page a navigation profile routes to that has no URL, so it cannot be linked to or bookmarked ([details](#conv019-details)) | Additional convention rules cover access rule constraints, role mapping, microflow size and content. @@ -171,6 +172,47 @@ list to keep in step. It is the companion to `documentable_elements()`, which projects only what can carry documentation and therefore leaves out microflows and Java actions — the two kinds that fill up an unorganised module. +### CONV019 details {#conv019-details} + +A Mendix page is reachable at `/p/` only if it has been given a URL. +Without one the page exists solely at the end of a click path: it cannot be +bookmarked, linked to from an email, reopened after a browser refresh, or +captured directly with `mxcli run --local --screenshot-url` — verifying one +screen means driving a browser through login and the menu. + +**Why only navigation targets.** Measured on a blank Mendix 11.12.1 app, **0 of +16 pages carry a URL** — Mendix's own Home page and every Administration and +FeedbackModule page included. Reporting every page without one would warn about +every page of every project from the day it is created, which is noise rather +than a finding. The pages a profile routes to are different: they are the app's +top-level screens, the ones a user lands on, bookmarks and shares. A page +reached only from a button inside another screen is never reported. + +Also never reported: + +- the **login page** and the **not-found page** — the platform routes to those + itself, so a URL on them buys nothing; +- **microflow-valued** menu items and home pages — the microflow decides what + opens, so there is no page here to be addressable; +- targets in **System and Marketplace modules**. + +A page routed to several ways — a home page that is also a menu item — is one +finding naming every route, not one finding per route. + +The remedy is `Url: 'orders'` on `CREATE PAGE`, or the URL property in Studio +Pro. An app that deliberately has no deep links can turn the rule off: + +```yaml +rules: + CONV019: + enabled: false +``` + +Behind it is the `navigation_targets()` builtin — `(profile, kind, role, +caption, page)` for every page a navigation profile routes to, where `kind` is +`home`, `role_home` or `menu`. Navigation was previously reachable only from the +Go rules, so no Starlark rule could ask which pages a user can actually reach. + ## Where Starlark Rules Live When you run `mxcli init`, Starlark rules are installed to: diff --git a/docs-site/src/tutorial/validation.md b/docs-site/src/tutorial/validation.md index fc1c5a9a0b..c72a818c9b 100644 --- a/docs-site/src/tutorial/validation.md +++ b/docs-site/src/tutorial/validation.md @@ -157,7 +157,7 @@ For a broader set of checks across the entire project (not just a single script) mxcli lint -p app.mpr ``` -This runs 14 built-in rules plus 28 Starlark rules covering security, architecture, quality, and naming conventions. See `mxcli lint --list-rules` for the full list. +This runs 14 built-in rules plus 29 Starlark rules covering security, architecture, quality, and naming conventions. See `mxcli lint --list-rules` for the full list. For CI/CD integration, output in SARIF format: diff --git a/mdl/linter/context.go b/mdl/linter/context.go index a4f2df8bd0..a450cbc8d0 100644 --- a/mdl/linter/context.go +++ b/mdl/linter/context.go @@ -1654,6 +1654,62 @@ func (ctx *LintContext) DocumentableElements() iter.Seq[Documentable] { } } +// NavigationTarget is one page a navigation profile sends a user to: the +// profile's home page, a role-specific home page, or a menu item's target. +type NavigationTarget struct { + Profile string // "Responsive", "Phone", "Tablet", … + Kind string // "home", "role_home" or "menu" + Role string // user role, for a role_home; "" otherwise + Caption string // menu item caption, for a menu target; "" otherwise + Page string // qualified page name +} + +// NavigationTargets iterates every page a navigation profile routes to. +// +// The login page and the not-found page are deliberately NOT targets: the +// platform routes to those itself (/login.html, and the 404 handler), so they +// are not screens a user navigates or links to. +// +// Microflow-valued menu items and home pages are skipped — the microflow decides +// what to open, so there is no page here to say anything about. Targets in +// System and Marketplace modules are not filtered here; a caller that joins to +// Pages() gets that exclusion for free, and one that does not is asking about +// navigation itself. +func (ctx *LintContext) NavigationTargets() iter.Seq[NavigationTarget] { + return func(yield func(NavigationTarget) bool) { + rows, err := ctx.db.Query(` + SELECT ProfileName, 'home', '', '', HomePage + FROM navigation_profiles + WHERE HomePageType = 'PAGE' AND COALESCE(HomePage, '') <> '' + UNION ALL + SELECT ProfileName, 'role_home', UserRole, '', Page + FROM navigation_role_homes + WHERE COALESCE(Page, '') <> '' + UNION ALL + SELECT ProfileName, 'menu', '', COALESCE(Caption, ''), TargetPage + FROM navigation_menu_items + WHERE COALESCE(TargetPage, '') <> '' + ORDER BY 1, 2, 5 + `) + if err != nil { + ctx.recordQueryError("NavigationTargets", err) + return + } + defer rows.Close() + + for rows.Next() { + var t NavigationTarget + if err := rows.Scan(&t.Profile, &t.Kind, &t.Role, &t.Caption, &t.Page); err != nil { + ctx.recordQueryError("NavigationTargets row scan", err) + continue + } + if !yield(t) { + return + } + } + } +} + // Document is one element of the App Explorer tree — what it is, which module // holds it, and the folder path it sits in ("" for the module root). // diff --git a/mdl/linter/report.go b/mdl/linter/report.go index bf7ba526af..a49fc8015c 100644 --- a/mdl/linter/report.go +++ b/mdl/linter/report.go @@ -68,6 +68,7 @@ var categoryMapping = map[string]string{ "CONV014": "Quality", "CONV015": "Quality", "CONV018": "Quality", + "CONV019": "Quality", // Architecture "ARCH001": "Architecture", diff --git a/mdl/linter/starlark.go b/mdl/linter/starlark.go index 103be381c9..7e12169588 100644 --- a/mdl/linter/starlark.go +++ b/mdl/linter/starlark.go @@ -319,6 +319,7 @@ func (r *StarlarkRule) buildPredeclared() starlark.StringDict { "java_actions": starlark.NewBuiltin("java_actions", r.builtinJavaActions), "documentable_elements": starlark.NewBuiltin("documentable_elements", r.builtinDocumentableElements), "documents": starlark.NewBuiltin("documents", r.builtinDocuments), + "navigation_targets": starlark.NewBuiltin("navigation_targets", r.builtinNavigationTargets), "pages": starlark.NewBuiltin("pages", r.builtinPages), "enumerations": starlark.NewBuiltin("enumerations", r.builtinEnumerations), "constants": starlark.NewBuiltin("constants", r.builtinConstants), @@ -467,6 +468,31 @@ func (r *StarlarkRule) builtinDocuments(_ *starlark.Thread, _ *starlark.Builtin, return starlark.NewList(out), nil } +// builtinNavigationTargets returns every page a navigation profile routes to — +// the profile home page, role-specific home pages, and menu item targets. +// +// Navigation was reachable only from the Go rules (through the reader's +// GetNavigation), so no Starlark rule could ask which pages a user can actually +// reach. Login and not-found pages are excluded: the platform routes to those. +func (r *StarlarkRule) builtinNavigationTargets(_ *starlark.Thread, _ *starlark.Builtin, args starlark.Tuple, kwargs []starlark.Tuple) (starlark.Value, error) { + if r.ctx == nil { + return starlark.NewList(nil), nil + } + + var out []starlark.Value + for t := range r.ctx.NavigationTargets() { + out = append(out, starlarkstruct.FromStringDict(starlark.String("navigation_target"), starlark.StringDict{ + "profile": starlark.String(t.Profile), + "kind": starlark.String(t.Kind), + "role": starlark.String(t.Role), + "caption": starlark.String(t.Caption), + "page": starlark.String(t.Page), + })) + } + + return starlark.NewList(out), nil +} + // builtinPages returns an iterator over pages. func (r *StarlarkRule) builtinPages(_ *starlark.Thread, _ *starlark.Builtin, args starlark.Tuple, kwargs []starlark.Tuple) (starlark.Value, error) { if r.ctx == nil { diff --git a/mdl/linter/starlark_navigation_url_test.go b/mdl/linter/starlark_navigation_url_test.go new file mode 100644 index 0000000000..f34706151d --- /dev/null +++ b/mdl/linter/starlark_navigation_url_test.go @@ -0,0 +1,258 @@ +// SPDX-License-Identifier: Apache-2.0 + +package linter_test + +import ( + "path/filepath" + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/catalog" + "github.com/mendixlabs/mxcli/mdl/linter" +) + +// navFixture builds a catalog with one user module, four pages, and a +// Responsive profile that routes to three of them: a home page, a role home, +// and a menu item. The fourth page is reachable only from inside another +// screen, which is what CONV019 must stay quiet about. +func navFixture(t *testing.T) *catalog.Catalog { + t.Helper() + + cat, err := catalog.NewFromFile(filepath.Join(t.TempDir(), "cat.db")) + if err != nil { + t.Fatalf("NewFromFile: %v", err) + } + t.Cleanup(func() { cat.Close() }) + db := cat.CatalogDB() + + exec := func(q string, args ...any) { + t.Helper() + if _, err := db.Exec(q, args...); err != nil { + t.Fatalf("exec %s: %v", q, err) + } + } + + exec(`INSERT INTO modules_data (Id, Name, ProjectId, SnapshotId) VALUES (?,?,?,?)`, + "mod-1", "Sales", "default", "s1") + + addPage := func(id, name, url string) { + exec(`INSERT INTO pages_data (Id, Name, QualifiedName, ModuleName, Folder, URL, ProjectId, SnapshotId) + VALUES (?,?,?,?,?,?,?,?)`, + id, name, "Sales."+name, "Sales", "", url, "default", "s1") + } + addPage("pg-home", "Home", "") // profile home, no URL + addPage("pg-orders", "Order_Overview", "") // menu target, no URL + addPage("pg-admin", "Admin_Home", "") // role home, no URL + addPage("pg-detail", "Order_Detail", "") // NOT in navigation + + exec(`INSERT INTO navigation_profiles_data + (ProfileName, Kind, IsNative, HomePage, HomePageType, LoginPage, NotFoundPage, + MenuItemCount, RoleBasedHomeCount, OfflineEntityCount, ProjectId, SnapshotId) + VALUES (?,?,?,?,?,?,?,?,?,?,?,?)`, + "Responsive", "Responsive", 0, "Sales.Home", "PAGE", "", "", 1, 1, 0, "default", "s1") + + exec(`INSERT INTO navigation_menu_items + (ProfileName, ItemPath, Depth, Caption, ActionType, TargetPage, TargetMicroflow, + SubItemCount, ProjectId, SnapshotId) + VALUES (?,?,?,?,?,?,?,?,?,?)`, + "Responsive", "0", 0, "Orders", "PAGE", "Sales.Order_Overview", "", 0, "default", "s1") + + exec(`INSERT INTO navigation_role_homes (ProfileName, UserRole, Page, Microflow, ProjectId, SnapshotId) + VALUES (?,?,?,?,?,?)`, + "Responsive", "Administrator", "Sales.Admin_Home", "", "default", "s1") + + return cat +} + +func runNavURLRule(t *testing.T, cat *catalog.Catalog) []linter.Violation { + t.Helper() + // The rule that ships, loaded from disk. + rule, err := linter.LoadStarlarkRule("../../.claude/lint-rules/conv019_navigation_page_url.star") + if err != nil { + t.Fatalf("loading the shipped CONV019: %v", err) + } + return rule.Check(linter.NewLintContext(cat, &minimalReader{})) +} + +// The three routes a profile can take, and the page that is not on any of them. +func TestCONV019_FlagsNavigationTargetsWithoutURL(t *testing.T) { + vs := runNavURLRule(t, navFixture(t)) + + if len(vs) != 3 { + t.Fatalf("got %d violations, want 3:\n%s", len(vs), messages(vs)) + } + if vs[0].RuleID != "CONV019" { + t.Errorf("RuleID = %q, want CONV019", vs[0].RuleID) + } + + joined := messages(vs) + for _, want := range []string{ + "Sales.Admin_Home", + "the home page for role 'Administrator'", + "Sales.Home", + "the home page of the 'Responsive' profile", + "Sales.Order_Overview", + "the 'Orders' menu item", + } { + if !strings.Contains(joined, want) { + t.Errorf("missing %q:\n%s", want, joined) + } + } + + // The whole point of the narrowing: a page reached only from inside another + // screen is not expected to be addressable. + if strings.Contains(joined, "Order_Detail") { + t.Errorf("a page that is not a navigation target was flagged:\n%s", joined) + } +} + +// A page that HAS a URL is the remedy working, so it must go quiet. +func TestCONV019_SilentOnceTheURLIsSet(t *testing.T) { + cat := navFixture(t) + if _, err := cat.CatalogDB().Exec( + `UPDATE pages_data SET URL = 'orders' WHERE Id = ?`, "pg-orders"); err != nil { + t.Fatalf("update url: %v", err) + } + + vs := runNavURLRule(t, cat) + if strings.Contains(messages(vs), "Order_Overview") { + t.Errorf("a page with a URL was still flagged:\n%s", messages(vs)) + } + // Control: the other two, still without URLs, must still report — otherwise + // this passes against a rule that stopped working altogether. + if len(vs) != 2 { + t.Fatalf("got %d violations after setting one URL, want 2:\n%s", len(vs), messages(vs)) + } +} + +// One page routed to two ways is one finding naming both routes, not two +// findings for the same page. +func TestCONV019_ReportsAPageOnceAcrossRoutes(t *testing.T) { + cat := navFixture(t) + if _, err := cat.CatalogDB().Exec(`INSERT INTO navigation_menu_items + (ProfileName, ItemPath, Depth, Caption, ActionType, TargetPage, TargetMicroflow, + SubItemCount, ProjectId, SnapshotId) + VALUES (?,?,?,?,?,?,?,?,?,?)`, + "Responsive", "1", 0, "Dashboard", "PAGE", "Sales.Home", "", 0, "default", "s1"); err != nil { + t.Fatalf("insert menu item: %v", err) + } + + vs := runNavURLRule(t, cat) + if len(vs) != 3 { + t.Fatalf("got %d violations, want 3 — Sales.Home must not be reported twice:\n%s", + len(vs), messages(vs)) + } + + var home string + for _, v := range vs { + if strings.Contains(v.Message, "Sales.Home'") { + home = v.Message + } + } + if home == "" { + t.Fatalf("Sales.Home was not reported:\n%s", messages(vs)) + } + for _, want := range []string{"the home page of the 'Responsive' profile", "the 'Dashboard' menu item"} { + if !strings.Contains(home, want) { + t.Errorf("the finding does not name the %q route:\n%s", want, home) + } + } +} + +// Marketplace pages are somebody else's model. They drop out of the join +// against pages() rather than being listed, which is easy to get wrong. +func TestCONV019_IgnoresMarketplaceTargets(t *testing.T) { + cat := navFixture(t) + db := cat.CatalogDB() + + if _, err := db.Exec( + `INSERT INTO modules_data (Id, Name, Source, ProjectId, SnapshotId) VALUES (?,?,?,?,?)`, + "mod-2", "Administration", "Marketplace", "default", "s1"); err != nil { + t.Fatalf("insert module: %v", err) + } + if _, err := db.Exec(`INSERT INTO pages_data + (Id, Name, QualifiedName, ModuleName, Folder, URL, ProjectId, SnapshotId) + VALUES (?,?,?,?,?,?,?,?)`, + "pg-acct", "Account_Overview", "Administration.Account_Overview", "Administration", + "", "", "default", "s1"); err != nil { + t.Fatalf("insert page: %v", err) + } + if _, err := db.Exec(`INSERT INTO navigation_menu_items + (ProfileName, ItemPath, Depth, Caption, ActionType, TargetPage, TargetMicroflow, + SubItemCount, ProjectId, SnapshotId) + VALUES (?,?,?,?,?,?,?,?,?,?)`, + "Responsive", "2", 0, "Accounts", "PAGE", "Administration.Account_Overview", "", + 0, "default", "s1"); err != nil { + t.Fatalf("insert menu item: %v", err) + } + + vs := runNavURLRule(t, cat) + if strings.Contains(messages(vs), "Administration") { + t.Errorf("a Marketplace page was flagged:\n%s", messages(vs)) + } + // Control: the three user pages still report, so the silence above is the + // module filter and not the fixture failing to reach the rule. + if len(vs) != 3 { + t.Fatalf("got %d violations, want the 3 user pages:\n%s", len(vs), messages(vs)) + } +} + +// NavigationTargets is the projection the rule stands on. The login and +// not-found pages must NOT appear: the platform routes to those itself, so a +// URL on them buys nothing and reporting them would be pure noise. +func TestNavigationTargets_ExcludeLoginAndNotFound(t *testing.T) { + cat := navFixture(t) + if _, err := cat.CatalogDB().Exec( + `UPDATE navigation_profiles_data SET LoginPage = ?, NotFoundPage = ? WHERE ProfileName = ?`, + "Sales.Login", "Sales.NotFound", "Responsive"); err != nil { + t.Fatalf("update profile: %v", err) + } + + ctx := linter.NewLintContext(cat, &minimalReader{}) + kinds := map[string]string{} + for target := range ctx.NavigationTargets() { + kinds[target.Page] = target.Kind + if target.Page == "Sales.Login" || target.Page == "Sales.NotFound" { + t.Errorf("%s is routed by the platform and must not be a navigation target", target.Page) + } + } + + // Control: the three real targets ARE there with the right kind, so the + // absence above is the WHERE clause and not an empty query. + want := map[string]string{ + "Sales.Home": "home", + "Sales.Admin_Home": "role_home", + "Sales.Order_Overview": "menu", + } + for page, kind := range want { + if kinds[page] != kind { + t.Errorf("target %s = kind %q, want %q (targets: %v)", page, kinds[page], kind, kinds) + } + } +} + +// A microflow-valued home page has no page to be addressable, so it must not +// produce a target at all — otherwise the rule would try to look up a microflow +// name in the page map. +func TestNavigationTargets_SkipMicroflowHomePages(t *testing.T) { + cat := navFixture(t) + if _, err := cat.CatalogDB().Exec( + `UPDATE navigation_profiles_data SET HomePage = ?, HomePageType = ? WHERE ProfileName = ?`, + "Sales.ACT_Route", "MICROFLOW", "Responsive"); err != nil { + t.Fatalf("update profile: %v", err) + } + + ctx := linter.NewLintContext(cat, &minimalReader{}) + n := 0 + for target := range ctx.NavigationTargets() { + if target.Page == "Sales.ACT_Route" { + t.Errorf("a microflow home page was yielded as a page target: %+v", target) + } + n++ + } + // Control: the menu item and the role home survive, so the exclusion is the + // HomePageType filter rather than the whole query going empty. + if n != 2 { + t.Errorf("got %d targets, want the 2 that are still pages", n) + } +} From 82bb85b70bc11e90c09e8cc47c9cd60c1d1954a8 Mon Sep 17 00:00:00 2001 From: Ako Date: Mon, 7 Sep 2026 12:12:18 +0000 Subject: [PATCH 3/5] docs(proposal): offline synchronization configuration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit mxcli can create an offline navigation profile and can read every row of its sync configuration, but cannot write one. The result is a profile that builds, routes and installs as a PWA and shows an empty app, because a Mendix offline profile downloads nothing until each entity is given a synchronization mode. DESCRIBE NAVIGATION already states the gap in its own output — the rows come back commented, marked "not yet modifiable" — which also makes describe -> exec lossy for any project using offline sync, the same round-trip failure that preceded the layout and menu-icon work. Checked first, because it decides whether this is a feature or a rescue: an existing configuration SURVIVES a rewrite. Both engines patch the stored profile rather than rebuilding it, and neither write path touches OfflineEntityConfigs. This is a clean gap, not the data loss that `create or modify entity` was inflicting on access rules. Two hazards the proposal is mostly about, both with a precedent from this week: The read is lossy. gen declares six properties on the config element and mdl/types keeps three, discarding DownloadMode, ShouldDownload and CompatibilityMode — the last being the column with warning triangles in the report's screenshot. A writer built from the three the semantic model carries would drop the others, which is the access-rule defect again; so phase 1 is carrying all six on READ, before anything writes. The sync mode is an enum whose captions are not its keys. The metamodel declares All / Constrained / Never / None / NoneAndPreserveData / Online while Studio Pro shows "Online", "All Objects", "By XPath" — so the mapping cannot be read off the UI at all. That is #1035 exactly, where a caption reached gallery.def.json and mxbuild rejected every gallery with CE0463, and the guard that closed it is the one to copy. Syntax is a per-entity SYNC block matching the MENU block's shape, with WHERE implying Constrained so the invalid pairings are unspellable rather than merely diagnosable. Honest about what is not known: no project on this machine has a populated offline entity config — an earlier scan claiming five was matching the key name that every profile carries, not the element. The shape has to come from a real document, and §5 lists the four things it settles, including where the throw-on-reject setting lives, which is on neither gen's profile nor the metamodel's. Co-Authored-By: Claude Opus 5 --- .../PROPOSAL_offline_sync_configuration.md | 233 ++++++++++++++++++ 1 file changed, 233 insertions(+) create mode 100644 docs/11-proposals/PROPOSAL_offline_sync_configuration.md diff --git a/docs/11-proposals/PROPOSAL_offline_sync_configuration.md b/docs/11-proposals/PROPOSAL_offline_sync_configuration.md new file mode 100644 index 0000000000..217c744791 --- /dev/null +++ b/docs/11-proposals/PROPOSAL_offline_sync_configuration.md @@ -0,0 +1,233 @@ +--- +title: Offline synchronization configuration — the one thing an offline profile needs that MDL cannot say +status: draft +date: 2026-09-07 +related: + - navigation-support.md + - docs/13-decisions/0003-mdl-is-sql-shaped.md + - docs/13-decisions/0005-semantic-model-interface-currency.md +--- + +# Offline synchronization configuration + +> Prompted by a CapTrack screenshot of Studio Pro's **Customize offline +> synchronization** dialog: three entities, three different sync modes, one XPath +> constraint. mxcli can create the profile that dialog belongs to, and can read +> every row in it, and cannot write a single one. + +## 1. Problem + +`mxcli` can author an offline navigation profile — `PhoneOffline`, +`ResponsiveOffline` and `TabletOffline` are three of the six creatable kinds, and +`CREATE NAVIGATION PhoneOffline …` has worked since profile creation landed. + +What it cannot author is the thing that makes an offline profile *do* anything. +A Mendix offline profile downloads nothing until each entity is given a +synchronization mode, and that configuration is unreachable from MDL. The result +is a profile that builds, routes and installs as a PWA, and shows an empty app. + +`DESCRIBE NAVIGATION` is explicit about the gap, in the output itself: + +``` +-- Offline Entities (not yet modifiable): +-- SYNC CapTrack.Team MODE ByXPath where '[...]'; +``` + +That comment is the whole feature request. It also makes describe → exec lossy +for any project that uses offline sync, which is the same failure that preceded +the layout and menu-icon work: a round trip that silently returns less than it +was given. + +## 2. What exists today — measured + +| | State | +|---|---| +| Creating an offline profile | **Works.** `CanonicalProfileKind` accepts `ResponsiveOffline`, `PhoneOffline`, `TabletOffline` | +| Reading the sync rows | **Works, both engines.** `sdk/mpr/parser_misc.go:550` and `mdl/backend/modelsdk/navigation_read.go:213` | +| `DESCRIBE NAVIGATION` | Emits the rows **as comments**, marked "not yet modifiable" | +| Authoring | **Absent.** No `offline` token exists in any `.g4` file; `navigationClause` is `HOME` / `LOGIN PAGE` / `NOT FOUND PAGE` / `MENU` | +| Writing | **Absent.** `navigation_profile_add.go:122` writes `OfflineEntityConfigs` as `bson.A{int32(3)}` — the empty typed-array marker | +| Catalog | Only `OfflineEntityCount`, a **count**. "Which entities sync offline?" is not queryable | +| Validation | `MDL-OFFLINE01` already exists, and warns when a page reachable from an offline profile binds an attribute across more than one association (CE6206) | + +**An existing rewrite does not destroy the configuration**, and this was checked +first because it decides whether this is a feature or a rescue. Both engines +*patch* the stored profile rather than rebuilding it: `navPatchWebProfile` writes +`HomePage`, `HomeItems`, `LoginPageSettings`, `NotFoundHomepage` and the menu, +and never touches `OfflineEntityConfigs`; legacy goes through `readPatchWrite`. +A hand-configured sync setup therefore survives `CREATE OR REPLACE NAVIGATION` +today. This is a clean gap, not a data-loss defect — the opposite of what +`create or modify entity` was doing to access rules. + +### 2.1 The read is lossy, and that is the hazard + +`modelsdk/gen` declares **six** properties on `Navigation$OfflineEntityConfig`: + +``` +Entity DownloadMode ShouldDownload SyncMode Constraint CompatibilityMode +``` + +`mdl/types.NavOfflineEntity` keeps **three** — `Entity`, `SyncMode`, +`Constraint`. `DownloadMode`, `ShouldDownload` and `CompatibilityMode` are read +and discarded. `CompatibilityMode` is the column carrying warning triangles in +the screenshot, so it is not hypothetical. + +That matters the moment authoring exists. A writer that builds a config element +from the three fields the semantic model carries writes a document missing the +other three — which is precisely the class of defect that had +`create or modify entity` deleting access rules, and `create or modify entity` +is the more instructive precedent: the fix there was not to extend the carry +list but to **invert the direction**, starting from what is stored and +overwriting only what the statement declares. + +So this proposal's first rule: **the read must carry all six properties before +the write carries any.** MDL will be able to spell three of them; the other +three must survive a rewrite untouched. The precedent is `ruleInfoFromGen` for +validation rules, where the payload is carried on READ specifically so a +rewrite can be refused or preserved rather than silently downgraded. + +### 2.2 `generated/metamodel` and `gen` disagree, and the snapshot is why + +`generated/metamodel.NavigationOfflineEntityConfig` declares **three** +properties — `Constraint`, `Entity`, `SyncMode`. CLAUDE.md makes +`generated/metamodel` the arbiter when the two disagree, with one caveat that +applies exactly here: it is a **snapshot of 11.6.0**, so it is sound for what it +contains and says nothing about properties introduced later. + +`CompatibilityMode` appears in the Studio Pro UI of the version in the +screenshot. The likeliest reading is that it postdates the snapshot rather than +that `gen` invented it — but *likeliest* is not measured, and the rule for that +is to get a real document. + +## 3. The trap this feature is walking into + +The sync mode is an enumeration, and `generated/metamodel` declares six members: + +``` +All Constrained Never None NoneAndPreserveData Online +``` + +Studio Pro's dropdown shows **captions**: "Online", "All Objects", "By XPath". +Neither "All Objects" nor "By XPath" is a member of the enumeration. + +This is the same defect that shipped as `mendixlabs/mxcli#1035` two days ago — +`gallery.def.json` stored `pagingPosition: "below"` because "Below grid" was the +caption of the member `bottom`, and mxbuild rejected every gallery with +`CE0463`. The lesson generalises: **a value read off a Studio Pro screen is a +caption until proven otherwise**, and the six-member enum here does not map +one-to-one onto the three captions the dialog shows, so the mapping cannot even +be guessed from the UI. + +The guard that closed #1035 is the model to copy: +`TestDefJSONEnumLiteralsAreDeclaredMPKKeys` asserts every literal mxcli writes is +a key the package declares. The equivalent here asserts every MDL sync word maps +to a member of `NavigationSyncMode`, and it must fail when the mapping table +gains a caption. + +## 4. Design + +### 4.1 Syntax + +A per-entity statement, matching `navMenuItemDef`'s shape rather than the +`( key: value )` property form — this is a list of rules, not a property bag, +and the menu block is the established precedent for a list inside a profile. + +```sql +CREATE NAVIGATION PhoneOffline + HOME PAGE CapTrack.Mobile_Dashboard + MENU ( + MENU ITEM 'Overview' PAGE CapTrack.Mobile_Dashboard ICON Atlas_Core.Atlas.home; + ) + SYNC ( + SYNC CapTrack.Employee ONLINE; + SYNC CapTrack.Movement ALL; + SYNC CapTrack.Team WHERE '[Name = ''abc'']'; + SYNC CapTrack.Audit NEVER; + ); +``` + +Reading as English is ADR-0003's bar, and `SYNC CapTrack.Movement ALL` clears it +where `MODE Constrained` — the spelling the current describe comment invents — +does not. + +Two decisions inside that: + +**`WHERE` implies `Constrained`.** A constrained entity without a constraint and +a constrained constraint without the mode are both nonsense, so deriving the +mode from the clause makes the invalid pair unspellable rather than diagnosable. +The cost is that `Constrained` has no bare word, which is correct: there is +nothing to say. + +**Every mode word maps to one enum member, and the mapping is a table with a +test.** `ONLINE`→`Online`, `ALL`→`All`, `NEVER`→`Never`, `WHERE`→`Constrained`. +`None` and `NoneAndPreserveData` need words too — and need a reference document +before they get them, because the difference between them is data retention on +the device and the dialog in the screenshot does not obviously expose either. + +### 4.2 Writing + +`ALTER NAVIGATION SYNC ( … )` replaces the whole list, the way a +`MENU (…)` block replaces the menu. Per-entity `ADD` / `DROP` verbs are +deliberately not proposed: the list is small, wholly visible in one describe, +and diff-friendly as a block. + +The write is an **overlay on the stored element**, not a rebuild — §2.1. For an +entity already configured, `DownloadMode`, `ShouldDownload` and +`CompatibilityMode` are read from the stored config and written back unchanged. +For a newly added entity they take the codec's declared defaults, which is the +one case where a reference document is load-bearing: a default guessed wrong is +invisible until a device syncs. + +### 4.3 Catalog and references + +`OfflineEntityCount` becomes rows: an entity synced by an offline profile is a +reference target, so `show references to CapTrack.Movement` names the profile. +This is the same argument that made a widget a reference target — "which +profiles sync this entity?" is the question an offline change asks, and it is +currently unanswerable. + +## 5. What needs a reference document before implementation + +There is **no local project with a populated offline entity config.** Every +project on this machine carries the empty `OfflineEntityConfigs` key, which +every navigation profile has; none carries a `Navigation$OfflineEntityConfig` +element. An earlier scan of this reported five hits and was wrong — it matched +the key name, not the element. + +So the shape must come from a real document, and CapTrack in the screenshot is +one: three entities, three modes, one XPath constraint, and visible +compatibility-mode state. + +What a dump of that document settles, none of which should be guessed: + +1. **Which of the six properties Studio Pro actually writes**, and their + defaults — particularly whether `ShouldDownload` and `DownloadMode` are + written at all on a web profile or are native-only. +2. **Whether `CompatibilityMode` really exists on this version**, closing §2.2. +3. **What the caption-to-key mapping is**, confirming "All Objects"→`All` and + "By XPath"→`Constrained` rather than assuming it. +4. **Where "Throw error when server rejects objects during synchronization" + lives.** It is on neither `gen`'s `NavigationProfile` nor the metamodel's, so + it is either a later property or is not stored on the profile at all. It is + in the screenshot, so it is stored somewhere. + +## 6. Phasing + +1. **Carry all six properties on READ**, and add the round-trip test. No syntax, + no writing. On its own this changes nothing a user sees, and it is the + precondition for every later slice not being a silent downgrade. +2. **`SYNC` block in `CREATE`/`ALTER NAVIGATION`**, overlay write, the enum-key + guard test, and `DESCRIBE` emitting real MDL instead of a comment — which + closes the describe → exec round trip. +3. **Catalog rows and the reference edge.** + +`None` / `NoneAndPreserveData` words, and the throw-on-reject setting, are held +back to whichever slice the reference document lands in. + +## 7. Overlap + +[`navigation-support.md`](navigation-support.md) is `status: partial` and listed +offline entity configs in its scope. Its reading half shipped; this proposal is +the authoring half and does not restate its findings. Nothing in +`mdl-examples/doctype-tests/` covers offline sync — `navigation-profiles.mdl` +and `11-navigation-examples.mdl` are home pages and menus only. From c835e920a9b9f5979e8d5a88c86d79587ca85d14 Mon Sep 17 00:00:00 2001 From: Ako Date: Mon, 7 Sep 2026 13:17:26 +0000 Subject: [PATCH 4/5] fix(workflow): make describe output re-executable (#408) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `describe workflow` emitted MDL that `mxcli check` refused: 6 of the 14 workflows in the 9 demo apps in mx-test-projects/ failed describe -> check. Two independent defects behind one symptom. MDL-WF03 refused the enumeration value Studio Pro actually stores. The rule required a bare identifier, but every Workflows$EnumerationValueConditionOutcome in the corpus holds the qualified form Module.Enum.Value (7 of 7 non-empty), so the describer emitted what was stored and the checker rejected mxcli's own output. The rule exists to catch free text like 'Confirmed closed'; rejecting the dot was collateral, and spaces are still refused. The stored document is what settled which side to change — emitting the last segment instead would have quieted check and written a document unlike every real one. MDL could not name the activities a `jump to` targets. Mendix stores JumpToActivity.TargetActivity as an activity NAME string, not a pointer, and Studio Pro names activities by type and ordinal (decision1, split1, callMicroflow1, waitForNotification1) with no relation to the caption. MDL had a name slot only on `user task`; every other builder did act.Name = act.Caption, so the stored name had nowhere to be emitted to, two decisions sharing a caption collided on one name, and a `jump to decision1` resolved to nothing. Reaching MxBuild it became a jump to itself and surfaced as CE6681 ("not possible to jump to end activities or jump-to activities") — an error naming a different fault. This was not a describer bug: the grammar had no name slot, so the fix runs grammar -> AST -> visitor -> builder -> describer. `decision`, `parallel split`, `wait for timer` and `wait for notification` now take an optional name; `call microflow` / `call workflow` take `AS `. Without one the caption-derived name is unchanged, so existing scripts are unaffected, and DESCRIBE emits a name only where it is not derivable. Verified with a control: the pre-fix binary built from the parent commit reproduces 6/14 failing where the fixed one is 0/14. The round-tripped workflow was executed into FactoryManagement(AgenticEnterpriseEdition) and read back — decision1..3, split1, callMicroflow1..6 and waitForNotification1..2 all land and both jump targets resolve — on top of mx check at 0 errors against an untouched baseline (mxbuild 11.10.0). mx check alone would not have shown it: a workflow that jumps to itself is perfectly valid. Closes ako/mxcli#408 Co-Authored-By: Claude Opus 5 --- .../fix-issue/findings/mdl-executor.jsonl | 1 + .../skills/mendix/write-workflows/SKILL.md | 39 +++++-- CHANGELOG.md | 8 ++ cmd/mxcli/syntax/features_workflow.go | 30 ++++- docs/01-project/MDL_QUICK_REFERENCE.md | 25 ++++- .../workflow-408-describe-roundtrip.mdl | 104 ++++++++++++++++++ .../doctype-tests/24-workflow-examples.mdl | 43 ++++++++ mdl/ast/ast_workflow.go | 6 + mdl/executor/cmd_workflows.go | 64 +++++++++-- mdl/executor/cmd_workflows_describe_test.go | 13 ++- mdl/executor/cmd_workflows_write.go | 25 +++++ mdl/executor/validate_workflow.go | 19 +++- mdl/executor/validate_workflow_test.go | 59 ++++++++++ mdl/grammar/domains/MDLWorkflow.g4 | 24 +++- mdl/visitor/visitor_workflow.go | 34 +++++- 15 files changed, 445 insertions(+), 49 deletions(-) create mode 100644 mdl-examples/bug-tests/workflow-408-describe-roundtrip.mdl diff --git a/.claude/skills/fix-issue/findings/mdl-executor.jsonl b/.claude/skills/fix-issue/findings/mdl-executor.jsonl index a4fb52009a..e6f9cedaab 100644 --- a/.claude/skills/fix-issue/findings/mdl-executor.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-executor.jsonl @@ -555,3 +555,4 @@ {"area": "mdl/executor", "date": "2026-09-06", "symptom": "CE0463 \"the definition of this widget has changed\" on a Gallery authored with a non-default pagination (`loadMore` or `virtualScrolling`); the default `buttons` gallery is clean, and `mxcli docker check` reports 0 errors because it runs `mx update-widgets` first (mendixlabs/mxcli#1035)", "cause": "gallery.def.json stored `pagingPosition: \"below\"`. The Gallery .mpk declares the enumeration as {bottom|top|both}, where `bottom`'s caption is \"Below grid\" — so \"below\" was the caption's first word, not a member key. It stayed hidden for a year because `pagingPosition` is hidden when pagination is `buttons` and the hidden-property pass resets a hidden unnamed property to its declared default, so only a non-default pagination let the wrong literal reach disk", "file": "`sdk/widgets/definitions/gallery.def.json`, `modelsdk/widgets/definitions/gallery.def.json`, `.mxcli/widgets/gallery.def.json` (three copies, all needed)", "insight": "For any CE0463 on a widget mxcli authored, diff the stored properties against an `mx update-widgets` copy AFTER mapping TypePointer -> PropertyKey — the isolated difference here was one enum string. Then check the literal against the .mpk's `` set, not its caption: both sides are plain strings, so nothing else compares them. `TestDefJSONEnumLiteralsAreDeclaredMPKKeys` (mdl/executor) now does it for every definition. Two measurement traps: `mxcli docker check` needs `--no-update-widgets` to see CE0463 at all, and a widget whose wrong value is hidden in the default configuration will not reproduce — vary the property that unhides it", "ce": ["CE0463"]} {"area": "mdl/executor", "date": "2026-09-06", "symptom": "`create or modify persistent entity Mod.Thing ( ... )` on an EXISTING entity deleted every DomainModels$AccessRule on it. Output was `Modified entity: Mod.Thing` and nothing else; `mxcli check` passed; a byte-identical re-run stripped them just as completely. Measured on ako/CapTrackV3, both engines, mxbuild 11.14.0: one domain-model script over eight entities took 174 access rules to 132 and 1352 member entries to 1071 (ako/mxcli-rest FINDINGS).", "cause": "Structural, not a missing case: the CreateOrModify branch of execCreateEntity built a fresh domainmodel.Entity from the AST and swapped it in, so every field MDL has no words for went with it — access rules, the view/external-source fields, the OData remote properties. Documentation and indexes had each been carried back individually after their own earlier defect report, so the carry list was already two entries long and access rules would have been the third. Fixed by inverting the direction: mergeDeclaredOntoStoredEntity starts from the STORED entity and overwrites only the fields the statement declares (entityFieldsDeclaredByStatement), with pruneMemberAccessesForDroppedAttributes removing the member entries of attributes the rewrite actually removed.", "file": "`mdl/executor/cmd_entities.go` (mergeDeclaredOntoStoredEntity, entityFieldsDeclaredByStatement, pruneMemberAccessesForDroppedAttributes, droppedEntityMembers); tests `mdl/executor/entity_modify_preserves_test.go`; example `mdl-examples/bug-tests/entity-modify-preserves-access-rules.mdl`", "insight": "The report said `mx check` stays clean, and that is TRUE and misleading: it is clean on an entity nothing is bound to yet, and the same script at scale gave 288 errors, every one CE2729. The silence is exactly where nothing depends on the access, so the loss surfaces later, on someone else's change. Idempotence is not a defence either — the rebuilt document genuinely differs, so ADR-0008 does not elide it. Two design points. (1) Dropping the attributes a statement omits and REBUILDING the ones it names are independent: an entity can be modified in place, which is what turns an endless carry list into a closed one. A reflection test (TestMergeDeclaredOntoStoredEntity_EveryFieldHasADecision) now fails on any Entity field in neither group, and under the stubbed fix it names all 19 fields the wholesale replace was discarding. (2) The prune must run on POSITIVE evidence of removal (stored-minus-declared), not on absence from the rebuilt entity's attributes: an entity's rules also govern INHERITED members, which never appear in its own Attributes list, so the obvious predicate emptied all five rules of CapTrack.ExportDocument (extends System.FileDocument, owns no attributes) and produced CE0066 — a regression the first version of this fix shipped and only a specialisation exposes."} {"area": "mdl/executor", "date": "2026-09-06", "symptom": "A repeatable widget property written as a property value had two failure modes: `attributes: [(attributeName: 'x')]` (single key) checked CLEAN, exec'd successfully and vanished from storage; `[(k: v, k2: v2)]` (multi key) died as `missing ')' at ','`. Reported upstream as mendixlabs/mxcli#999 against FileUploader allowedFileFormats / customButtons.", "cause": "propertyValueV3's array alternative is a list of EXPRESSIONS. `(k: v)` happens to be a valid expression, so the single-key form parsed and the visitor flattened it to []string{\"(attributeName:'x')\"} — a value no widget writer claims, hence the silent drop. `(k: v, k2: v2)` is not an expression, hence the parse error. The two symptoms had ONE cause and looked unrelated.", "file": "mdl/executor/validate_widget_object_property.go", "insight": "The dangerous half was invisible to the one rule that might have caught it: MDL-WIDGET07 ('not recognized, will be silently dropped') fires only when the property is UNKNOWN, so it warned without a project and stayed correctly silent with the widget definition present — i.e. it went quiet in exactly the real-world case. Measuring a diagnostic without -p and concluding it covers the case is the recurring trap. Fix shape: parse the bad form deliberately so BOTH shapes reach one semantic error, rather than leaving the multi-key one as a cryptic parse failure — the grammar alternative exists only to be rejected, and is ordered before the expression array so the single-key form stops being flattened. Do NOT wire it to the object-list builder: the container form already works, and two spellings for one construct is the anti-pattern the design guide names. Make the message rewrite the author's own entry into the working form, so the error carries its remedy. Corpus diff was 0 of 532 scripts, which is necessary and NOT sufficient — it compares diagnostics and cannot see an ordinary array captured into the wrong AST type, so assert the untouched shapes directly.", "issue": "mendixlabs/mxcli#999"} +{"area": "mdl/executor", "date": "2026-09-07", "symptom": "`describe workflow` emitted MDL that `mxcli check` refused: 6 of the 14 workflows in the 9 demo apps in mx-test-projects/ failed describe -> check. Two rules fired: MDL-WF03 on decision outcomes like 'FactoryManagement.ENUM_InvestigationType.Engineering', and MDL-WF05 on `jump to decision1` / `jump to split1`.", "cause": "Two independent defects behind one symptom. (1) wfOutcomeIdentRe required a BARE identifier, but every Workflows$EnumerationValueConditionOutcome in the corpus stores the QUALIFIED form Module.Enum.Value (7 of 7 non-empty) — the rule was written to catch free text like 'Confirmed closed' and rejecting the dot was collateral, so the describer was right and the validator wrong. (2) MDL had a name slot only on `user task`; every other builder did act.Name = act.Caption, while Mendix resolves JumpToActivity.TargetActivity by activity NAME and Studio Pro names activities by type and ordinal (decision1, split1, callMicroflow1, userTask1, waitForNotification1, timer1) with no relation to the caption. The stored name had nowhere to be emitted to.", "file": "mdl/executor/validate_workflow.go, mdl/grammar/domains/MDLWorkflow.g4, mdl/executor/cmd_workflows.go, mdl/executor/cmd_workflows_write.go", "insight": "The second half was NOT a describer bug, which is what it looked like at first: the grammar had no name slot, so the fix ran grammar -> AST -> visitor -> builder -> describer. Two measurements settled the design and neither was guessable from the code. Studio Pro's stored outcome value decided which side of defect 1 to change — changing the describer to emit the last segment would have 'fixed' the check and written a document unlike every real one. And TargetActivity turned out to hold a NAME STRING, not an ID pointer, which is why a lost name degrades to a jump-to-itself and surfaces as CE6681 'not possible to jump to end activities or jump-to activities' — an error naming a different fault entirely. Emit the name only when it is not derivable (name != caption and != sanitizeActivityName(caption); for call activities, != the called document's short name), so mxcli-authored workflows describe unchanged and the clause appears exactly where it carries information. The control is what makes this provable: the pre-fix binary, built from HEAD~1 in a throwaway worktree, reproduces 6/14 failing where the fixed one is 0/14, and the round-tripped document was read back to confirm decision1..3 / split1 / callMicroflow1..6 landed and both jump targets resolve — mx check at 0 errors alone would NOT have shown that, since a workflow with a jump to itself is perfectly valid.", "issue": "ako/mxcli#408"} diff --git a/.claude/skills/mendix/write-workflows/SKILL.md b/.claude/skills/mendix/write-workflows/SKILL.md index 5a4fb40b2e..8e9a2ce058 100644 --- a/.claude/skills/mendix/write-workflows/SKILL.md +++ b/.claude/skills/mendix/write-workflows/SKILL.md @@ -81,32 +81,34 @@ begin page Module.ReviewPage outcomes 'Done' { }; - -- Call a microflow (server logic); optional parameter mapping + outcomes - call microflow Module.ACT_Validate + -- Call a microflow (server logic); optional name, parameter mapping + outcomes + call microflow Module.ACT_Validate as callMicroflow1 with (Module.ACT_Validate.Item = '$WorkflowContext'); - -- Decision: a boolean or enum exclusive split - decision '$WorkflowContext/Total > 1000' + -- Decision: a boolean or enum exclusive split. The name is optional; give one + -- when a `jump to` targets it. Outcome values are enumeration value + -- identifiers — bare or qualified (Module.Enum.Value). + decision decision1 '$WorkflowContext/Total > 1000' outcomes true -> { call microflow Module.ACT_Escalate; } false -> { call microflow Module.ACT_AutoApprove; }; -- Parallel split: independent branches run concurrently - parallel split + parallel split split1 path 1 { call microflow Module.ACT_Notify; } path 2 { call microflow Module.ACT_Log; }; -- Wait for a timer, then continue (duration is a Mendix expression) - wait for timer 'addHours([%CurrentDateTime%], 1)'; + wait for timer timer1 'addHours([%CurrentDateTime%], 1)'; -- Wait for an external notification (e.g. an event) - wait for notification; + wait for notification waitForNotification1; -- Jump back to an earlier activity by name (a loop) jump to Review; -- Call a sub-workflow - call workflow Module.SubProcess comment 'delegate'; + call workflow Module.SubProcess as callWorkflow1 comment 'delegate'; end workflow; ``` @@ -176,6 +178,27 @@ them at all, so they do not appear in the output and cannot be written from MDL. A workflow that has one can only be edited in Studio Pro or through `ALTER WORKFLOW` (below) — never with `CREATE OR REPLACE`. +## Activity names, and why `jump to` depends on them + +Mendix stores `JumpToActivity.TargetActivity` as an activity **name string**, not +a pointer — so a jump is only as good as the name it aims at. Every activity type +takes an optional explicit name (`as ` for the two call activities, a bare +name for the rest); without one mxcli derives it from the caption, or from the +called document for `call microflow` / `call workflow`. + +That default is fine for a workflow written from scratch, and it is why two +decisions sharing a caption used to collide on one name. It is **not** fine when +reproducing a workflow Studio Pro authored: Studio Pro names activities by type +and ordinal — `decision1`, `split1`, `callMicroflow1`, `userTask1`, +`waitForNotification1` — with no relation to the caption. `describe workflow` +emits the stored name whenever it is not derivable, so the jump wiring survives a +re-execution; before that it did not, and a `jump to decision1` reached MxBuild as +a jump to itself (**CE6681**, "not possible to jump to end activities or jump-to +activities" — an error naming a different fault). See ako/mxcli#408. + +`mxcli check` resolves every jump against the activity names the script itself +declares (**MDL-WF05**) and lists the valid targets when one misses. + ## Rewriting an existing workflow `CREATE OR REPLACE|MODIFY WORKFLOW` **rebuilds the workflow from the statement**, diff --git a/CHANGELOG.md b/CHANGELOG.md index b39c42e4f1..3cd5401c54 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,14 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **`describe workflow` output was not re-executable** (ako/mxcli#408) — 6 of the 14 workflows in the 9 demo apps in `mx-test-projects/` failed `describe` → `check`. Two independent defects behind one symptom. + + **MDL-WF03 refused the enumeration value Studio Pro actually stores.** The rule required a bare identifier, but every `Workflows$EnumerationValueConditionOutcome` in the corpus holds the qualified form `Module.Enum.Value` (7 of 7 non-empty), so the describer emitted what was stored and the checker rejected mxcli's own output. The rule exists to catch free text like `'Confirmed closed'`; rejecting the dot was collateral, and spaces are still refused. What settled which side to change was the stored document: emitting the last segment instead would have quieted `check` and written a document unlike every real one. + + **MDL could not name the activities a `jump to` targets.** Mendix stores `JumpToActivity.TargetActivity` as an activity **name string**, not a pointer, and Studio Pro names activities by type and ordinal — `decision1`, `split1`, `callMicroflow1`, `waitForNotification1` — with no relation to the caption. MDL had a name slot only on `user task`; every other builder derived the name from the caption, so the stored name had nowhere to be emitted to, two decisions sharing a caption collided on one name, and a `jump to decision1` resolved to nothing. Reaching MxBuild it became a jump to *itself* and surfaced as **CE6681** ("not possible to jump to end activities or jump-to activities") — an error naming a different fault. `decision`, `parallel split`, `wait for timer` and `wait for notification` now take an optional name, and `call microflow` / `call workflow` take `AS `; without one the caption-derived name is unchanged, so existing scripts are unaffected, and `DESCRIBE` emits a name only where it is not derivable. + + Verified with a control: the pre-fix binary reproduces 6/14 failing where the fixed one is 0/14, and the round-tripped workflow was executed into `FactoryManagement(AgenticEnterpriseEdition)` and read back — `decision1..3`, `split1`, `callMicroflow1..6` and `waitForNotification1..2` all land and both jump targets resolve, on top of `mx check` at 0 errors against an untouched baseline (mxbuild 11.10.0). `mx check` alone would not have shown it: a workflow that jumps to itself is perfectly valid. + - **A Gallery with a non-default pagination was rejected as CE0463** (mendixlabs/mxcli#1035) — `pagination: 'loadMore'` and `pagination: 'virtualScrolling'` produced a widget mxbuild refuses with *"the definition of this widget has changed"*. The definition stored `pagingPosition: "below"`, which is not a member of the enumeration at all: the Gallery package declares `{bottom|top|both}`, and `"below"` is the first word of `bottom`'s **caption**, "Below grid". Mendix stores what mxcli writes and then rejects the widget. Two things kept it hidden. `pagingPosition` is hidden when pagination is `buttons` — the default — and the hidden-property pass resets a hidden unnamed property to its declared default, so the wrong literal only reached disk once something unhid it. And `mxcli docker check` runs `mx update-widgets` before `mx check` by default, which repairs the widget: the plain command reports **0 errors** on a project that genuinely has this, so the reporter's own evidence read as a clean project. `--no-update-widgets` is the only form that can see it. diff --git a/cmd/mxcli/syntax/features_workflow.go b/cmd/mxcli/syntax/features_workflow.go index ba8027c545..fc511ae873 100644 --- a/cmd/mxcli/syntax/features_workflow.go +++ b/cmd/mxcli/syntax/features_workflow.go @@ -74,7 +74,7 @@ func init() { // outcome does not ('OK' { }). The two read alike but are separate // grammar rules, so the arrow is easy to drop — this entry did, and // taught the broken form until TestExamplesParse started checking it. - Syntax: "DECISION [''] [COMMENT '']\n OUTCOMES '' -> { } ...;", + Syntax: "DECISION [] [''] [COMMENT '']\n OUTCOMES '' -> { } ...;", Example: "DECISION 'Check amount'\n OUTCOMES\n 'Under 1000' -> { }\n 'Over 1000' -> {\n USER TASK ManagerApproval 'Manager must approve'\n OUTCOMES 'OK' { };\n };", SeeAlso: []string{"workflow.create", "workflow.parallel-split"}, }) @@ -86,7 +86,7 @@ func init() { "parallel", "concurrent", "split", "fork", "join", "parallel gateway", "AND", }, - Syntax: "PARALLEL SPLIT [COMMENT '']\n PATH 1 { }\n PATH 2 { };", + Syntax: "PARALLEL SPLIT [] [COMMENT '']\n PATH 1 { }\n PATH 2 { };", Example: "PARALLEL SPLIT\n PATH 1 {\n USER TASK LegalReview 'Legal review'\n OUTCOMES 'Done' { };\n }\n PATH 2 {\n USER TASK TechReview 'Technical review'\n OUTCOMES 'Done' { };\n };", SeeAlso: []string{"workflow.decision", "workflow.create"}, }) @@ -98,7 +98,7 @@ func init() { "call microflow", "microflow task", "automated step", "system task", }, - Syntax: "CALL MICROFLOW Module.MF [COMMENT '']\n [OUTCOMES '' { } ...];", + Syntax: "CALL MICROFLOW Module.MF [AS ] [COMMENT '']\n [OUTCOMES '' { } ...];", Example: "CALL MICROFLOW HR.SendNotification\n COMMENT 'Notify manager';", SeeAlso: []string{"workflow.create", "workflow.call-workflow"}, }) @@ -109,11 +109,33 @@ func init() { Keywords: []string{ "call workflow", "sub-workflow", "nested workflow", }, - Syntax: "CALL WORKFLOW Module.WF [COMMENT ''];", + Syntax: "CALL WORKFLOW Module.WF [AS ] [COMMENT ''];", Example: "CALL WORKFLOW HR.SubApproval COMMENT 'Delegate to sub-process';", SeeAlso: []string{"workflow.create", "workflow.call-microflow"}, }) + Register(SyntaxFeature{ + Path: "workflow.jump-to", + Summary: "Jump to another activity — and the activity names it resolves against", + Keywords: []string{ + "jump", "jump to", "goto", "loop back", "activity name", + }, + // Mendix stores JumpToActivity.TargetActivity as an activity NAME, not a + // pointer, so the jump is only as good as the name. Studio Pro names + // activities by type and ordinal regardless of caption (decision1, + // split1, callMicroflow1); mxcli derives a name when none is given, which + // is why an explicit one matters when reproducing a stored workflow. + Syntax: "JUMP TO [COMMENT ''];\n\n" + + "-- name the target so the jump resolves:\n" + + "DECISION [''] ...\nPARALLEL SPLIT ...\n" + + "WAIT FOR TIMER ...\nWAIT FOR NOTIFICATION \n" + + "CALL MICROFLOW Module.MF AS \nCALL WORKFLOW Module.WF AS ", + Example: "DECISION decision1 '$WorkflowContext/Total > 1000'\n" + + " OUTCOMES\n true -> { }\n false -> { };\n\n" + + "PARALLEL SPLIT split1\n PATH 1 { JUMP TO decision1; }\n PATH 2 { };", + SeeAlso: []string{"workflow.create", "workflow.decision", "workflow.parallel-split"}, + }) + Register(SyntaxFeature{ Path: "workflow.drop", Summary: "Delete a workflow definition", diff --git a/docs/01-project/MDL_QUICK_REFERENCE.md b/docs/01-project/MDL_QUICK_REFERENCE.md index 42638d72ea..500023d960 100644 --- a/docs/01-project/MDL_QUICK_REFERENCE.md +++ b/docs/01-project/MDL_QUICK_REFERENCE.md @@ -646,15 +646,28 @@ Nested folders use `/` separator: `'Parent/Child/Grandchild'`. Missing folders a **Workflow Activity Types:** - `user task '' [page Mod.Page] [targeting [users|groups] microflow Mod.MF] [targeting [users|groups] xpath ''] [outcomes '' { } ...];` -- `call microflow Mod.MF [comment ''] [outcomes '' { } ...];` -- `call workflow Mod.WF [comment ''];` -- `decision [''] outcomes '' { } ...;` -- `parallel split path 1 { } path 2 { };` +- `call microflow Mod.MF [as ] [comment ''] [outcomes '' { } ...];` +- `call workflow Mod.WF [as ] [comment ''];` +- `decision [] [''] outcomes '' { } ...;` +- `parallel split [] path 1 { } path 2 { };` - `jump to ;` -- `wait for timer [''];` -- `wait for notification;` +- `wait for timer [] [''];` +- `wait for notification [];` - `end;` +**Activity names.** Every activity has a name, and `jump to` resolves against it +— Mendix stores `JumpToActivity.TargetActivity` as a name string, not a pointer. +Without an explicit name mxcli derives one (from the caption, or from the called +document for `call microflow` / `call workflow`), which is fine for a workflow +written from scratch. Name activities explicitly when a `jump to` targets them, +and when reproducing a workflow Studio Pro authored: Studio Pro names activities +by type and ordinal (`decision1`, `split1`, `callMicroflow1`) regardless of +caption, so `describe workflow` emits the name whenever it is not derivable. + +**Decision outcomes** are enumeration value identifiers, bare (`Approved`) or +qualified (`Module.Enum.Approved` — the form Studio Pro stores). Free text with +spaces is rejected (`MDL-WF03`). + **Example:** ```sql create workflow Module.ApprovalFlow diff --git a/mdl-examples/bug-tests/workflow-408-describe-roundtrip.mdl b/mdl-examples/bug-tests/workflow-408-describe-roundtrip.mdl new file mode 100644 index 0000000000..e2d2b4bb2b --- /dev/null +++ b/mdl-examples/bug-tests/workflow-408-describe-roundtrip.mdl @@ -0,0 +1,104 @@ +-- ako/mxcli#408 — `describe workflow` output was not re-executable. +-- +-- Two defects, both reproduced below. Before the fix, 6 of the 14 workflows in +-- the 9 demo apps in mx-test-projects/ failed describe -> check. +-- +-- 1. MDL-WF03 required a BARE enumeration value identifier, but every +-- Workflows$EnumerationValueConditionOutcome in the corpus stores the +-- QUALIFIED form (Module.Enum.Value; 7 of 7 non-empty). The describer emits +-- what is stored, so the rule refused mxcli's own output. +-- +-- 2. MDL had a name slot only on `user task`. Every other builder derived the +-- activity name from the caption, while Mendix resolves +-- JumpToActivity.TargetActivity by activity NAME and Studio Pro names +-- activities decision1 / split1 / callMicroflow1 regardless of caption. A +-- `jump to decision1` therefore resolved to nothing (MDL-WF05), or reached +-- MxBuild as a jump to itself (CE6681). +-- +-- Verified: describe -> exec -> mx check at 0 errors on mxbuild 11.10.0 against +-- FactoryManagement(AgenticEnterpriseEdition), with the untouched app as the +-- baseline; the round-tripped document stores decision1..3, split1, +-- callMicroflow1..6, waitForNotification1..2 and both jump targets resolve. + +create module WF408; + +create enumeration WF408.Kind ( Standard, Priority ); + +create entity WF408.Order ( + Total : decimal, + Kind : enumeration(WF408.Kind) +); + +create page WF408.ReworkPage ( + title: 'Rework', + layout: Atlas_Core.Atlas_Default, + params: { $WorkflowUserTask: System.WorkflowUserTask } +) { + layoutgrid g1 { + row r1 { + column c1 (desktopwidth: 12) { + dynamictext txt1 (content: 'Rework the request', rendermode: H2) + } + } + } +} +/ + +create microflow WF408.ACT_Noop () +begin + -- nothing to do; this workflow only needs a callable target + log info 'noop'; +end; +/ + +-- Defect 1: the qualified form Studio Pro stores must be accepted. +-- +-- A decision's outcomes must match what its expression returns (CE6686), so the +-- expression yields the enumeration the outcomes name, and the outcomes cover +-- every value PLUS the empty one — Studio Pro writes that third +-- EnumerationValueConditionOutcome with Value '' on every enum decision, and +-- mxbuild refuses the decision without it. +create workflow WF408.QualifiedEnumOutcome + parameter $WorkflowContext: WF408.Order +begin + decision '$WorkflowContext/Kind' + outcomes + 'WF408.Kind.Standard' -> { } + 'WF408.Kind.Priority' -> { } + '' -> { } + ; +end workflow; +/ + +-- Defect 2: a jump resolves against an explicitly named decision and split. +-- +-- The jump sits at the end of a user-task outcome branch. MxBuild refuses one +-- that is not at the end of a path (CE6679), one that crosses a parallel-split +-- boundary (CE6682 / CE7415), and a main flow ending in a jump leaves the +-- implicit End unreachable (CE6689) — none of which is what this test is about, +-- so the workflow is shaped the way Studio Pro shapes one. +create workflow WF408.NamedJumpTargets + parameter $WorkflowContext: WF408.Order +begin + decision decision1 '$WorkflowContext/Total > 1000' + outcomes + true -> { + user task Rework 'Fix and resubmit' + page WF408.ReworkPage + outcomes + 'Resubmit' { jump to decision1; } + 'Abandon' { } + ; + } + false -> { call microflow WF408.ACT_Noop as callMicroflow1; } + ; + + parallel split split1 + path 1 { call microflow WF408.ACT_Noop as callMicroflow2; } + path 2 { } + ; + + wait for timer timer1 'addHours([%CurrentDateTime%], 1)'; + wait for notification waitForNotification1; +end workflow; +/ diff --git a/mdl-examples/doctype-tests/24-workflow-examples.mdl b/mdl-examples/doctype-tests/24-workflow-examples.mdl index c8d031e7cf..92691ce7d5 100644 --- a/mdl-examples/doctype-tests/24-workflow-examples.mdl +++ b/mdl-examples/doctype-tests/24-workflow-examples.mdl @@ -486,3 +486,46 @@ begin -- Open workflow admin page open workflow $workflow; end; + +-- ============================================================================= +-- Named activities and jump targets (ako/mxcli#408) +-- ============================================================================= +-- Mendix resolves `jump to` by JumpToActivity.TargetActivity, which stores an +-- activity NAME. Studio Pro names every activity by type and ordinal +-- (decision1, split1, callMicroflow1) independently of its caption, so an +-- activity needs an explicit name for a described workflow to be re-executable. +-- The name is optional: without one it is still derived from the caption. +-- +-- Note where the jump sits. MxBuild refuses one that is not at the end of a +-- path (CE6679), one that crosses a parallel-split boundary (CE6682 / CE7415), +-- and a main flow ending in a jump makes the implicit End unreachable (CE6689) +-- — so the jump lives at the end of a user-task outcome branch, which is where +-- Studio Pro puts it too. + +create workflow WFTest.NamedActivities + parameter $WorkflowContext: WFTest.OrderContext +begin + call microflow WFTest.ACT_Validate as callMicroflow1; + + decision decision1 '$WorkflowContext/Total > 1000' + outcomes + true -> { + user task Rework 'Fix and resubmit' + page WFTest.TaskPage + outcomes + 'Resubmit' { jump to decision1; } + 'Abandon' { } + ; + } + false -> { call microflow WFTest.ACT_Process as callMicroflow2; } + ; + + parallel split split1 + path 1 { call microflow WFTest.ACT_Notify as callMicroflow3; } + path 2 { call microflow WFTest.ACT_Review as callMicroflow4; } + ; + + wait for timer timer1 'addHours([%CurrentDateTime%], 1)'; + wait for notification waitForNotification1; +end workflow; +/ diff --git a/mdl/ast/ast_workflow.go b/mdl/ast/ast_workflow.go index 0f417f67a2..46843dc622 100644 --- a/mdl/ast/ast_workflow.go +++ b/mdl/ast/ast_workflow.go @@ -73,6 +73,7 @@ type WorkflowUserTaskOutcomeNode struct { // WorkflowCallMicroflowNode represents a CALL MICROFLOW activity. type WorkflowCallMicroflowNode struct { + Name string // explicit activity name (`as `); see ako/mxcli#408 Microflow QualifiedName Caption string Outcomes []WorkflowConditionOutcomeNode @@ -84,6 +85,7 @@ func (n *WorkflowCallMicroflowNode) workflowActivityNode() {} // WorkflowCallWorkflowNode represents a CALL WORKFLOW activity. type WorkflowCallWorkflowNode struct { + Name string // explicit activity name (`as `); see ako/mxcli#408 Workflow QualifiedName Caption string ParameterMappings []WorkflowParameterMappingNode @@ -93,6 +95,7 @@ func (n *WorkflowCallWorkflowNode) workflowActivityNode() {} // WorkflowDecisionNode represents a DECISION activity. type WorkflowDecisionNode struct { + Name string // explicit activity name; see ako/mxcli#408 Expression string // decision expression Caption string Outcomes []WorkflowConditionOutcomeNode @@ -108,6 +111,7 @@ type WorkflowConditionOutcomeNode struct { // WorkflowParallelSplitNode represents a PARALLEL SPLIT activity. type WorkflowParallelSplitNode struct { + Name string // explicit activity name; see ako/mxcli#408 Caption string Paths []WorkflowParallelPathNode } @@ -130,6 +134,7 @@ func (n *WorkflowJumpToNode) workflowActivityNode() {} // WorkflowWaitForTimerNode represents a WAIT FOR TIMER activity. type WorkflowWaitForTimerNode struct { + Name string // explicit activity name; see ako/mxcli#408 DelayExpression string Caption string } @@ -138,6 +143,7 @@ func (n *WorkflowWaitForTimerNode) workflowActivityNode() {} // WorkflowWaitForNotificationNode represents a WAIT FOR NOTIFICATION activity. type WorkflowWaitForNotificationNode struct { + Name string // explicit activity name; see ako/mxcli#408 Caption string BoundaryEvents []WorkflowBoundaryEventNode // Issue #7 } diff --git a/mdl/executor/cmd_workflows.go b/mdl/executor/cmd_workflows.go index 2b48da69e4..72c799a167 100644 --- a/mdl/executor/cmd_workflows.go +++ b/mdl/executor/cmd_workflows.go @@ -368,10 +368,11 @@ func formatWorkflowActivities(flow *workflows.Flow, indent string) []string { if a.Annotation != "" { actLines = append(actLines, formatAnnotation(a.Annotation, indent)) } + nameClause := workflowActivityNameClause(a.Name, caption) if a.DelayExpression != "" { - actLines = append(actLines, fmt.Sprintf("%swait for timer %s comment %s", indent, mdlQuoted(a.DelayExpression), mdlQuoted(caption))) + actLines = append(actLines, fmt.Sprintf("%swait for timer%s %s comment %s", indent, nameClause, mdlQuoted(a.DelayExpression), mdlQuoted(caption))) } else { - actLines = append(actLines, fmt.Sprintf("%swait for timer comment %s", indent, mdlQuoted(caption))) + actLines = append(actLines, fmt.Sprintf("%swait for timer%s comment %s", indent, nameClause, mdlQuoted(caption))) } case *workflows.WaitForNotificationActivity: caption := a.Caption @@ -381,7 +382,8 @@ func formatWorkflowActivities(flow *workflows.Flow, indent string) []string { if a.Annotation != "" { actLines = append(actLines, formatAnnotation(a.Annotation, indent)) } - actLines = append(actLines, fmt.Sprintf("%swait for notification -- %s", indent, caption)) + actLines = append(actLines, fmt.Sprintf("%swait for notification%s -- %s", indent, + workflowActivityNameClause(a.Name, caption), caption)) // BoundaryEvents actLines = append(actLines, formatBoundaryEvents(a.BoundaryEvents, indent+" ")...) case *workflows.StartWorkflowActivity: @@ -552,9 +554,11 @@ func formatCallMicroflowTask(a *workflows.CallMicroflowTask, indent string) []st } params = append(params, fmt.Sprintf("%s = %s", paramName, mdlQuoted(pm.Expression))) } - lines = append(lines, fmt.Sprintf("%scall microflow %s with (%s) -- %s", indent, mf, strings.Join(params, ", "), caption)) + lines = append(lines, fmt.Sprintf("%scall microflow %s%s with (%s) -- %s", indent, mf, + workflowActivityAsClause(a.Name, shortDocName(mf)), strings.Join(params, ", "), caption)) } else { - lines = append(lines, fmt.Sprintf("%scall microflow %s -- %s", indent, mf, caption)) + lines = append(lines, fmt.Sprintf("%scall microflow %s%s -- %s", indent, mf, + workflowActivityAsClause(a.Name, shortDocName(mf)), caption)) } // Outcomes, then boundary events — the order the grammar requires @@ -586,7 +590,8 @@ func formatSystemTask(a *workflows.SystemTask, indent string) []string { mf = "?" } - lines = append(lines, fmt.Sprintf("%scall microflow %s -- %s", indent, mf, caption)) + lines = append(lines, fmt.Sprintf("%scall microflow %s%s -- %s", indent, mf, + workflowActivityAsClause(a.Name, shortDocName(mf)), caption)) // Outcomes lines = append(lines, formatConditionOutcomes(a.Outcomes, indent)...) @@ -621,9 +626,11 @@ func formatCallWorkflowActivity(a *workflows.CallWorkflowActivity, indent string } params = append(params, fmt.Sprintf("%s = %s", paramName, mdlQuoted(pm.Expression))) } - lines = append(lines, fmt.Sprintf("%scall workflow %s comment %s with (%s)", indent, wf, mdlQuoted(caption), strings.Join(params, ", "))) + lines = append(lines, fmt.Sprintf("%scall workflow %s%s comment %s with (%s)", indent, wf, + workflowActivityAsClause(a.Name, shortDocName(wf)), mdlQuoted(caption), strings.Join(params, ", "))) } else { - lines = append(lines, fmt.Sprintf("%scall workflow %s comment %s", indent, wf, mdlQuoted(caption))) + lines = append(lines, fmt.Sprintf("%scall workflow %s%s comment %s", indent, wf, + workflowActivityAsClause(a.Name, shortDocName(wf)), mdlQuoted(caption))) } // BoundaryEvents @@ -632,6 +639,39 @@ func formatCallWorkflowActivity(a *workflows.CallWorkflowActivity, indent string return lines } +// workflowActivityNameClause renders an activity's explicit name for describe +// output, or "" when the name is what the builder would derive from the caption +// anyway. Mendix resolves `jump to` by JumpToActivity.TargetActivity, which +// stores an activity NAME, and Studio Pro names every activity by type and +// ordinal (decision1, split1) independently of its caption — so without this the +// described workflow's jump wiring did not survive re-execution (ako/mxcli#408). +// Derived names are left off so mxcli-authored workflows describe unchanged. +func workflowActivityNameClause(name, caption string) string { + if name == "" || name == caption || name == sanitizeActivityName(caption) { + return "" + } + return " " + mdlIdent(name) +} + +// shortDocName returns the document name of a qualified name. +func shortDocName(qn string) string { + if i := strings.LastIndex(qn, "."); i >= 0 { + return qn[i+1:] + } + return qn +} + +// workflowActivityAsClause renders an `as ` clause for a call activity, +// whose name is otherwise derived from the document it calls. Studio Pro names +// these callMicroflow1 / callWorkflow1, so the derived name is almost never the +// stored one. See workflowActivityNameClause. +func workflowActivityAsClause(name, derived string) string { + if name == "" || name == derived || name == sanitizeActivityName(derived) { + return "" + } + return " as " + mdlIdent(name) +} + // formatExclusiveSplit formats an exclusive split (decision) for describe output. func formatExclusiveSplit(a *workflows.ExclusiveSplitActivity, indent string) []string { var lines []string @@ -645,10 +685,11 @@ func formatExclusiveSplit(a *workflows.ExclusiveSplitActivity, indent string) [] caption = a.Name } + nameClause := workflowActivityNameClause(a.Name, caption) if a.Expression != "" { - lines = append(lines, fmt.Sprintf("%sdecision %s -- %s", indent, mdlQuoted(a.Expression), caption)) + lines = append(lines, fmt.Sprintf("%sdecision%s %s -- %s", indent, nameClause, mdlQuoted(a.Expression), caption)) } else { - lines = append(lines, fmt.Sprintf("%sdecision -- %s", indent, caption)) + lines = append(lines, fmt.Sprintf("%sdecision%s -- %s", indent, nameClause, caption)) } lines = append(lines, formatConditionOutcomes(a.Outcomes, indent)...) @@ -669,7 +710,8 @@ func formatParallelSplit(a *workflows.ParallelSplitActivity, indent string) []st caption = a.Name } - lines = append(lines, fmt.Sprintf("%sparallel split -- %s", indent, caption)) + lines = append(lines, fmt.Sprintf("%sparallel split%s -- %s", indent, + workflowActivityNameClause(a.Name, caption), caption)) for i, outcome := range a.Outcomes { lines = append(lines, fmt.Sprintf("%s path %d {", indent, i+1)) if outcome.Flow != nil && len(outcome.Flow.Activities) > 0 { diff --git a/mdl/executor/cmd_workflows_describe_test.go b/mdl/executor/cmd_workflows_describe_test.go index 23a9aea004..6061ec072c 100644 --- a/mdl/executor/cmd_workflows_describe_test.go +++ b/mdl/executor/cmd_workflows_describe_test.go @@ -143,14 +143,18 @@ func TestFormatWaitForTimer_CaptionCommentFormat(t *testing.T) { caption: "Wait 2 Hours", actName: "waitAct1", delay: "${PT2H}", - want: "wait for timer '${PT2H}' comment 'Wait 2 Hours'", + // The stored name is not derivable from the caption, so describe + // emits it — dropping it is what broke `jump to` (ako/mxcli#408). + want: "wait for timer waitAct1 '${PT2H}' comment 'Wait 2 Hours'", }, { name: "name fallback no delay", caption: "", actName: "waitAct1", delay: "", - want: "wait for timer comment 'waitAct1'", + // Caption falls back to the name here, so the name is derivable and + // the clause is suppressed: unchanged output. + want: "wait for timer comment 'waitAct1'", }, } @@ -183,13 +187,14 @@ func TestFormatCallWorkflowActivity_CaptionCommentFormat(t *testing.T) { name: "caption used", caption: "Run Sub-Workflow", actName: "callWf1", - want: "call workflow Module.SubFlow comment 'Run Sub-Workflow'", + // callWf1 is not the called workflow's name, so it is emitted. + want: "call workflow Module.SubFlow as callWf1 comment 'Run Sub-Workflow'", }, { name: "name fallback", caption: "", actName: "callWf1", - want: "call workflow Module.SubFlow comment 'callWf1'", + want: "call workflow Module.SubFlow as callWf1 comment 'callWf1'", }, } diff --git a/mdl/executor/cmd_workflows_write.go b/mdl/executor/cmd_workflows_write.go index 279bb372b0..924767f8c9 100644 --- a/mdl/executor/cmd_workflows_write.go +++ b/mdl/executor/cmd_workflows_write.go @@ -369,6 +369,13 @@ func buildCallMicroflowTask(n *ast.WorkflowCallMicroflowNode) *workflows.CallMic if task.Caption == "" { task.Caption = task.Name } + // An explicit `as ` overrides the microflow-derived name, but only + // after the caption fallback above — the caption should still read as the + // microflow, not as the activity id. Mendix resolves `jump to` by name + // (ako/mxcli#408). + if n.Name != "" { + task.Name = n.Name + } for _, outcomeNode := range n.Outcomes { outcome := buildConditionOutcome(outcomeNode) @@ -403,6 +410,9 @@ func buildCallWorkflowActivity(n *ast.WorkflowCallWorkflowNode) *workflows.CallW if act.Caption == "" { act.Caption = act.Name } + if n.Name != "" { + act.Name = n.Name + } // Auto-bind $WorkflowContext parameter expression act.ParameterExpression = "$WorkflowContext" @@ -434,7 +444,13 @@ func buildExclusiveSplit(n *ast.WorkflowDecisionNode) *workflows.ExclusiveSplitA if act.Caption == "" { act.Caption = "Decision" } + // An explicit name is the activity's identity: Mendix resolves `jump to` by + // JumpToActivity.TargetActivity, which stores a name. Falling back to the + // caption keeps existing scripts unchanged (ako/mxcli#408). act.Name = act.Caption + if n.Name != "" { + act.Name = n.Name + } // Detect boolean decision (has TRUE or FALSE outcomes). // The Mendix 11 runtime only supports BooleanConditionOutcome and @@ -499,6 +515,9 @@ func buildParallelSplit(n *ast.WorkflowParallelSplitNode) *workflows.ParallelSpl act.Caption = "Parallel split" } act.Name = act.Caption + if n.Name != "" { + act.Name = n.Name + } for _, pathNode := range n.Paths { outcome := &workflows.ParallelSplitOutcome{} @@ -557,6 +576,9 @@ func buildWaitForTimer(n *ast.WorkflowWaitForTimerNode) *workflows.WaitForTimerA act.Caption = "Wait for timer" } act.Name = act.Caption + if n.Name != "" { + act.Name = n.Name + } return act } @@ -570,6 +592,9 @@ func buildWaitForNotification(n *ast.WorkflowWaitForNotificationNode) *workflows act.Caption = "Wait for notification" } act.Name = act.Caption + if n.Name != "" { + act.Name = n.Name + } // BoundaryEvents (Issue #7) act.BoundaryEvents = buildBoundaryEvents(n.BoundaryEvents) diff --git a/mdl/executor/validate_workflow.go b/mdl/executor/validate_workflow.go index 21a1abcf37..bad0aa3bed 100644 --- a/mdl/executor/validate_workflow.go +++ b/mdl/executor/validate_workflow.go @@ -16,11 +16,18 @@ import ( "github.com/mendixlabs/mxcli/mdl/linter" ) -// wfOutcomeIdentRe matches a valid Mendix EnumerationValueIdentifier: a bare -// identifier (no spaces or punctuation). Decision / call-microflow outcome names -// must be enum value identifiers; free text like 'Confirmed closed' is rejected -// by MxBuild. -var wfOutcomeIdentRe = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]*$`) +// wfOutcomeIdentRe matches a valid Mendix EnumerationValueIdentifier: dotted +// identifier segments, no spaces or other punctuation. Decision / +// call-microflow outcome names must be enum value identifiers; free text like +// 'Confirmed closed' is rejected by MxBuild. +// +// The qualified form is what Studio Pro actually stores — every +// EnumerationValueConditionOutcome in the demo corpus holds +// Module.Enum.Value (7 of 7 non-empty), so `describe workflow` emits it and a +// bare-identifier-only rule refused mxcli's own output (ako/mxcli#408). Bare +// values stay accepted: the rule's job is to catch free text, not to pick a +// spelling. +var wfOutcomeIdentRe = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]*(\.[A-Za-z_][A-Za-z0-9_]*)*$`) // ValidateWorkflow checks a workflow for constructs that pass parsing but are // rejected by MxBuild, without requiring a project connection. @@ -98,7 +105,7 @@ func checkWorkflowOutcomeNames(outcomes []ast.WorkflowConditionOutcomeNode, kind Severity: linter.SeverityError, Location: loc, Message: fmt.Sprintf("%s outcome '%s' is not a valid enumeration value identifier — MxBuild rejects outcome names with spaces or punctuation", kind, o.Value), - Suggestion: "Use a bare identifier (e.g. 'ConfirmedClosed'); a decision branches on the enumeration returned by its expression, so outcome names must match that enum's value identifiers.", + Suggestion: "Use an enumeration value identifier — bare ('ConfirmedClosed') or qualified ('Module.Enum.ConfirmedClosed'); a decision branches on the enumeration returned by its expression, so outcome names must match that enum's value identifiers.", }) } return out diff --git a/mdl/executor/validate_workflow_test.go b/mdl/executor/validate_workflow_test.go index 34a34bb7e0..ec31b4fcdb 100644 --- a/mdl/executor/validate_workflow_test.go +++ b/mdl/executor/validate_workflow_test.go @@ -179,3 +179,62 @@ end workflow;` t.Errorf("MDL-WF04 must not fire without an annotation: %v", vs) } } + +// MDL-WF03 must accept the qualified form Studio Pro actually stores. Every +// EnumerationValueConditionOutcome in the demo corpus stores Module.Enum.Value +// (7 of 7 non-empty), so `describe workflow` emits it and the rule refused its +// own output. See ako/mxcli#408. +func TestValidateWorkflow_QualifiedEnumOutcomeAccepted(t *testing.T) { + src := wfPreamble + `create workflow WF.W parameter $Ctx: WF.Ctx +begin + decision '$Ctx/Total > 1000' + outcomes + 'FactoryManagement.ENUM_InvestigationType.Engineering' -> { } + 'FactoryManagement.ENUM_InvestigationType.Operations' -> { } + ; +end workflow;` + if vs := workflowViolations(t, src); hasRule(vs, "MDL-WF03") { + t.Fatalf("qualified enum outcome must not trigger MDL-WF03, got %v", vs) + } +} + +// The control for the case above: free text with a space is still refused, so +// widening the rule to accept dots did not turn it off. +func TestValidateWorkflow_QualifiedEnumOutcomeStillRejectsFreeText(t *testing.T) { + src := wfPreamble + `create workflow WF.W parameter $Ctx: WF.Ctx +begin + decision '$Ctx/Total > 1000' + outcomes + 'Factory Management.ENUM_Kind.A' -> { } + ; +end workflow;` + if vs := workflowViolations(t, src); !hasRule(vs, "MDL-WF03") { + t.Fatalf("dotted free text with a space must still trigger MDL-WF03, got %v", vs) + } +} + +// MDL-WF05 — a jump may target a named decision or parallel split. Mendix +// resolves JumpToActivity.TargetActivity by activity NAME, and Studio Pro names +// them decision1 / split1 regardless of caption, so MDL needs a name slot on +// every jumpable activity or a described workflow cannot be re-executed. +func TestValidateWorkflow_JumpToNamedDecisionAndSplit(t *testing.T) { + src := wfPreamble + `create workflow WF.W parameter $Ctx: WF.Ctx +begin + decision decision1 '$Ctx/Total > 1000' + outcomes + true -> { } + false -> { } + ; + parallel split split1 + path 1 { jump to decision1; } + path 2 { } + ; + wait for timer timer1 'PT1H'; + wait for notification waitForNotification1; + jump to split1; +end workflow;` + vs := workflowViolations(t, src) + if hasRule(vs, "MDL-WF05") { + t.Fatalf("jump to a named decision/split must resolve, got %v", vs) + } +} diff --git a/mdl/grammar/domains/MDLWorkflow.g4 b/mdl/grammar/domains/MDLWorkflow.g4 index 30486260e3..1904ab35ae 100644 --- a/mdl/grammar/domains/MDLWorkflow.g4 +++ b/mdl/grammar/domains/MDLWorkflow.g4 @@ -40,6 +40,18 @@ workflowActivityStmt | workflowAnnotationStmt SEMICOLON ; +/** + * An activity's explicit name. Mendix resolves `jump to` by + * JumpToActivity.TargetActivity, which stores an activity NAME, and Studio Pro + * names every activity by type and ordinal (decision1, split1, callMicroflow1) + * independently of its caption. Without a name slot a described workflow's jump + * wiring could not be re-executed. See ako/mxcli#408. + */ +workflowActivityName + : IDENTIFIER + | QUOTED_IDENTIFIER + ; + workflowUserTaskStmt : USER TASK (IDENTIFIER | QUOTED_IDENTIFIER) STRING_LITERAL (PAGE qualifiedName)? @@ -72,7 +84,7 @@ workflowUserTaskOutcome ; workflowCallMicroflowStmt - : CALL MICROFLOW qualifiedName (COMMENT STRING_LITERAL)? + : CALL MICROFLOW qualifiedName (AS workflowActivityName)? (COMMENT STRING_LITERAL)? (WITH LPAREN workflowParameterMapping (COMMA workflowParameterMapping)* RPAREN)? (OUTCOMES workflowConditionOutcome+)? (BOUNDARY EVENT workflowBoundaryEventClause+)? @@ -83,12 +95,12 @@ workflowParameterMapping ; workflowCallWorkflowStmt - : CALL WORKFLOW qualifiedName (COMMENT STRING_LITERAL)? + : CALL WORKFLOW qualifiedName (AS workflowActivityName)? (COMMENT STRING_LITERAL)? (WITH LPAREN workflowParameterMapping (COMMA workflowParameterMapping)* RPAREN)? ; workflowDecisionStmt - : DECISION STRING_LITERAL? (COMMENT STRING_LITERAL)? + : DECISION workflowActivityName? STRING_LITERAL? (COMMENT STRING_LITERAL)? (OUTCOMES workflowConditionOutcome+)? ; @@ -97,7 +109,7 @@ workflowConditionOutcome ; workflowParallelSplitStmt - : PARALLEL SPLIT (COMMENT STRING_LITERAL)? + : PARALLEL SPLIT workflowActivityName? (COMMENT STRING_LITERAL)? workflowParallelPath+ ; @@ -110,11 +122,11 @@ workflowJumpToStmt ; workflowWaitForTimerStmt - : WAIT FOR TIMER STRING_LITERAL? (COMMENT STRING_LITERAL)? + : WAIT FOR TIMER workflowActivityName? STRING_LITERAL? (COMMENT STRING_LITERAL)? ; workflowWaitForNotificationStmt - : WAIT FOR NOTIFICATION (COMMENT STRING_LITERAL)? + : WAIT FOR NOTIFICATION workflowActivityName? (COMMENT STRING_LITERAL)? (BOUNDARY EVENT workflowBoundaryEventClause+)? ; diff --git a/mdl/visitor/visitor_workflow.go b/mdl/visitor/visitor_workflow.go index fcccebdec7..ddd2524a92 100644 --- a/mdl/visitor/visitor_workflow.go +++ b/mdl/visitor/visitor_workflow.go @@ -541,10 +541,27 @@ func buildWorkflowUserTaskOutcome(ctx parser.IWorkflowUserTaskOutcomeContext) as return outcome } +// workflowActivityNameText reads an optional explicit activity name off a +// statement. Mendix resolves `jump to` by activity NAME, so this is the only way +// a described workflow's jump wiring survives a re-execution (ako/mxcli#408). +func workflowActivityNameText(ctx parser.IWorkflowActivityNameContext) string { + if ctx == nil { + return "" + } + if qid := ctx.QUOTED_IDENTIFIER(); qid != nil { + return unquoteIdentifier(qid.GetText()) + } + if id := ctx.IDENTIFIER(); id != nil { + return id.GetText() + } + return "" +} + // buildWorkflowCallMicroflow builds a WorkflowCallMicroflowNode. func buildWorkflowCallMicroflow(ctx parser.IWorkflowCallMicroflowStmtContext) *ast.WorkflowCallMicroflowNode { cmCtx := ctx.(*parser.WorkflowCallMicroflowStmtContext) node := &ast.WorkflowCallMicroflowNode{ + Name: workflowActivityNameText(cmCtx.WorkflowActivityName()), Microflow: buildQualifiedName(cmCtx.QualifiedName()), } @@ -593,6 +610,7 @@ func bareWorkflowParameterName(raw string) string { func buildWorkflowCallWorkflow(ctx parser.IWorkflowCallWorkflowStmtContext) *ast.WorkflowCallWorkflowNode { cwCtx := ctx.(*parser.WorkflowCallWorkflowStmtContext) node := &ast.WorkflowCallWorkflowNode{ + Name: workflowActivityNameText(cwCtx.WorkflowActivityName()), Workflow: buildQualifiedName(cwCtx.QualifiedName()), } @@ -616,7 +634,9 @@ func buildWorkflowCallWorkflow(ctx parser.IWorkflowCallWorkflowStmtContext) *ast // buildWorkflowDecision builds a WorkflowDecisionNode. func buildWorkflowDecision(ctx parser.IWorkflowDecisionStmtContext) *ast.WorkflowDecisionNode { dCtx := ctx.(*parser.WorkflowDecisionStmtContext) - node := &ast.WorkflowDecisionNode{} + node := &ast.WorkflowDecisionNode{ + Name: workflowActivityNameText(dCtx.WorkflowActivityName()), + } allStrings := dCtx.AllSTRING_LITERAL() stringIdx := 0 @@ -672,7 +692,9 @@ func buildWorkflowConditionOutcome(ctx parser.IWorkflowConditionOutcomeContext) // buildWorkflowParallelSplit builds a WorkflowParallelSplitNode. func buildWorkflowParallelSplit(ctx parser.IWorkflowParallelSplitStmtContext) *ast.WorkflowParallelSplitNode { psCtx := ctx.(*parser.WorkflowParallelSplitStmtContext) - node := &ast.WorkflowParallelSplitNode{} + node := &ast.WorkflowParallelSplitNode{ + Name: workflowActivityNameText(psCtx.WorkflowActivityName()), + } if psCtx.COMMENT() != nil && psCtx.STRING_LITERAL() != nil { node.Caption = unquoteString(psCtx.STRING_LITERAL().GetText()) @@ -727,7 +749,9 @@ func buildWorkflowJumpTo(ctx parser.IWorkflowJumpToStmtContext) *ast.WorkflowJum // buildWorkflowWaitForTimer builds a WorkflowWaitForTimerNode. func buildWorkflowWaitForTimer(ctx parser.IWorkflowWaitForTimerStmtContext) *ast.WorkflowWaitForTimerNode { wtCtx := ctx.(*parser.WorkflowWaitForTimerStmtContext) - node := &ast.WorkflowWaitForTimerNode{} + node := &ast.WorkflowWaitForTimerNode{ + Name: workflowActivityNameText(wtCtx.WorkflowActivityName()), + } allStrings := wtCtx.AllSTRING_LITERAL() if len(allStrings) > 0 && wtCtx.COMMENT() == nil { @@ -745,7 +769,9 @@ func buildWorkflowWaitForTimer(ctx parser.IWorkflowWaitForTimerStmtContext) *ast // buildWorkflowWaitForNotification builds a WorkflowWaitForNotificationNode. func buildWorkflowWaitForNotification(ctx parser.IWorkflowWaitForNotificationStmtContext) *ast.WorkflowWaitForNotificationNode { wnCtx := ctx.(*parser.WorkflowWaitForNotificationStmtContext) - node := &ast.WorkflowWaitForNotificationNode{} + node := &ast.WorkflowWaitForNotificationNode{ + Name: workflowActivityNameText(wnCtx.WorkflowActivityName()), + } if wnCtx.COMMENT() != nil && wnCtx.STRING_LITERAL() != nil { node.Caption = unquoteString(wnCtx.STRING_LITERAL().GetText()) From de672d63bbef499dd475928c1b249ec803aa18e9 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 7 Sep 2026 20:59:11 +0000 Subject: [PATCH 5/5] fix(security): reconcile the members a specialization inherits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ALTER ENTITY M.Gen ADD ATTRIBUTE …`, where a specialization of Gen also has an access rule, produced CE0066 "Entity access is out of date" — and `UPDATE SECURITY`, project-wide or scoped, reported "All entity access rules are up to date" and changed nothing (mendixlabs/mxcli#1047, reported against 0.21.0 on a confirmed MPR v2 project). ReconcileMemberAccesses computes the same-module ancestor set and then used it ONLY for the association pass. The attribute pass beside it walked the entity's own attributes, so a specialization's expected member set never contained the members it inherits: nothing looked missing, nothing was added, and `modified` stayed 0 — which the command prints as "up to date". A false success is worse than an error, because it ends the investigation: the reporter reasonably concluded the command was broken rather than the model. Inherited members are now included, each qualified against the entity that DECLARES it (M.Gen.AfterSpec, not M.Spec.AfterSpec — that is CE1613), with a child attribute shadowing an ancestor's of the same name, matching the executor's own member walk. An ancestor in another module is still neither added nor pruned; its domain model is not loaded here, so the existing entries are carried through by the preserve branch. Stale inherited entries are likewise still preserved — that is the opposite direction from this defect, and the report's own control says "-0 removed". Both engines had it in the same shape and both are fixed. A fix in one of these parallel writers stays latent in the other until something switches engines. Two things fixed in passing, in code this change already touches: the legacy add-loop iterated a map, so the order of new entries varied between runs; and legacy keyed coverage by bare attribute name while preserving any reference not qualified against this entity, which would have preserved AND re-added the same member once inherited ones were expected. The duplicate only appears on a second reconcile, so the regression test runs it twice. Measured on 11.12.1, both engines: the reported repro now checks clean, and a project broken by the PRE-FIX binary is repaired by the fixed one — "Reconciled 1 access rule(s) in module ProbeSecond", mx check 1 -> 0, on the project-wide and the scoped form alike. That is also the first end-to-end proof of the repair path, which an earlier fix recorded as unproven: once the write path reconciles, no MDL script can produce a CE0066 to repair. Reverting either engine's attribute walk to own-attributes-only fails its test with the reported symptom. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JXnEgoc2NQP1Y2TWMCMXC4 --- .../fix-issue/findings/mdl-backend.jsonl | 1 + CHANGELOG.md | 10 + .../access-rule-reconciliation.md | 22 ++ .../1047-inherited-member-reconcile.mdl | 79 ++++++ .../modelsdk/domainmodel_security_write.go | 64 ++++- .../modelsdk/reconcile_inherited_attr_test.go | 134 ++++++++++ sdk/mpr/writer_security.go | 231 ++++++++++++++---- sdk/mpr/writer_security_inherited_test.go | 229 +++++++++++++++++ 8 files changed, 704 insertions(+), 66 deletions(-) create mode 100644 mdl-examples/bug-tests/1047-inherited-member-reconcile.mdl create mode 100644 mdl/backend/modelsdk/reconcile_inherited_attr_test.go create mode 100644 sdk/mpr/writer_security_inherited_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-backend.jsonl b/.claude/skills/fix-issue/findings/mdl-backend.jsonl index 5278312a84..bdac0746c0 100644 --- a/.claude/skills/fix-issue/findings/mdl-backend.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-backend.jsonl @@ -81,3 +81,4 @@ {"area": "mdl/backend", "date": "2026-08-27", "symptom": "No export mapping mxcli writes matches its Studio Pro original, so every one stays in #260's silent-loss set even once its source kind is authorable", "cause": "The export writers hardcoded three properties the IMPORT twin already read off the element: `MinOccurs 0` (a schema root has **1**), `MaxLength 0`, and `IsKey` not written at all. `MaxLength` is the one that cannot be a constant — Studio Pro stores **0 for a string element and -1 for a numeric one**, mirroring the bound schema element exactly as `MaxOccurs` already did", "file": "`mdl/backend/modelsdk/mapping_write.go` (`exportValueElementToGen`, `exportObjectElementToGen`) + `mapping_read.go`, `sdk/mpr/writer_export_mapping.go`, `modelsdk/mpr/serialize_mappings.go`, `mdl/executor/cmd_export_mappings.go`, `model.ExportMappingElement`", "insight": "The export element model had **none** of these fields and the reader populated none of them, so this is four layers (model, reader, builder, three writers), not a writer patch — and a writer patch alone would have written zeros from an empty model. Clone them from the schema element where the builder already clones `MaxOccurs`. Check the import twin first when an export property looks wrong: this was a divergence between the two writers, not a considered decision, and the import side is the correct reference. Repro `mdl-examples/bug-tests/mapping-277-export-property-set.mdl`, tests `sdk/mpr/writer_export_mapping_properties_test.go`. Issue ako/mxcli#277", "refs": ["#260", "ako/mxcli#277"]} {"area": "mdl/backend", "date": "2026-08-27", "symptom": "A rebuilt mapping drops `MessageDefinition2`, so a document written by Studio Pro 11.10+ never round-trips", "cause": "The key is version-introduced — `modelsdk/gen/mappings/version.go` records `messageDefinition2` as `Introduced: \"11.10.0\"` — and gen generates **no accessor** for it, so nothing read or wrote it. A blank 11.13 app's own mappings carry it as `\"\"`; none of the older pinned fixtures has it at all", "file": "`model.ImportMapping`/`ExportMapping` (`MessageDefinition2 *string`), `mdl/backend/modelsdk/mapping_read.go` (`messageDefinition2FromRaw`) + `mapping_write.go`, `sdk/mpr/parser_*_mapping.go` + `writer_*_mapping.go`, `mdl/executor/cmd_import_mappings.go` + `cmd_export_mappings.go`", "insight": "**Carry it, do not derive it.** A pointer, because nil (absent) is NOT the same as present-and-empty — writing the key onto a pre-11.10 document is the overlay-rule mistake CLAUDE.md warns about, which mxbuild tolerates and Studio Pro refuses to open. The executor decides: carry `existing.MessageDefinition2` on an update, apply the version gate only on a CREATE where there is nothing to read it off. A plain version gate in the writer looks equivalent and is not — it re-adds the key to every older document a rewrite touches, which turned four previously-clean fixtures red. gen having no accessor means the read goes to raw BSON, the route `parameterEntityFromRaw` takes. **`MappingSourceReference` is the same family and deliberately NOT carried**: its gate (10.16) predates every project in the field, and the codec emits it through a package-level `TypeDefaults` registration, so making it conditional would touch every mapping write to preserve one pre-10.16 fixture. Issue ako/mxcli#279", "refs": ["ako/mxcli#279"]} {"area": "mdl/backend", "cause": "The **reader**, not the writer: `mdl/backend/modelsdk/navigation_read.go` type-asserted only the `$Type`s `modelsdk/gen` declares for those two slots, and neither is what the documents carry \u2014 `LoginPageSettings` is stored as `Forms$FormSettings` with the page under `Form` (gen expects `Navigation$NavigationProfileLoginFormSettings` / `LoginPage`), and `NotFoundHomepage` as `Navigation$HomePage` (gen and `generated/metamodel` both expect `Navigation$NotFoundHomePage`). A failed type assertion leaves the field empty, so the loss is silent", "date": "2026-09-01", "file": "`mdl/backend/modelsdk/navigation_read.go` (`navLoginPageOf`, `navNotFoundPageOf`), cross-check `generated/metamodel/types.go` `NavigationNavigationProfile`", "insight": "**The other engine is the control.** Legacy read the same bytes correctly throughout, which is what identifies a reader bug: `describe navigation X` on both engines must agree, and a disagreement localises the defect to the one that reads through gen. Accept the `$Type` the documents actually carry and keep gen's as a fallback branch. Note the two slots fail in **opposite directions** and want opposite fixes: for the login page a real Studio Pro document and `generated/metamodel` agree with the writers, so **gen** is wrong; for the not-found page \u2014 Studio Pro's **\"Fallback page\"** \u2014 metamodel and gen agree with each other and the three mxcli **writers** are the odd one out, emitting `Navigation$HomePage` where Studio Pro stores `Navigation$NotFoundHomePage`. ako/TestApp supplied the reference document that settled it. **Correction (2026-09-01): mxbuild does NOT accept either.** Measured on 11.13 against a build emitting the old spelling, `mx check` and `mxbuild --target=deploy` both exit 1 with \"Object of type 'Mendix.Modeler.WebUI.Navigation.HomePage' cannot be converted to type '...NotFoundHomePage'\" -- the project will not LOAD, so every downstream check is lost. What actually let it through is that nothing ever BUILT a project with a fallback page set: the automated mx-check coverage is doctype-tests/ only and no script there sets one, so the first was added by the fix itself. **Generalisable: 'the build tolerates it' is a claim that needs the same control as the fix** -- revert the writer, rebuild, and run the tool, or the reason a bug escaped gets recorded backwards and sends the next reader looking in the wrong place. Keep reading both `$Type`s regardless, but for the repair path rather than round-tripping: a pre-fix project does not build, and mxcli reads BSON directly, so accepting the old spelling is what lets it open and fix one. Repro `mdl-examples/bug-tests/navigation-describe-profile-pages.mdl`", "symptom": "`DESCRIBE NAVIGATION` prints `home page` and the menu but **silently omits `login page` and `not found page`**, so pasting its output back (the documented copy workflow) deletes both from the profile. The clauses are on disk and `MXCLI_ENGINE=legacy` prints them"} +{"area": "mdl/backend", "date": "2026-09-07", "symptom": "CE0066 \"Entity access is out of date\" after `ALTER ENTITY M.Gen ADD ATTRIBUTE ...` where a SPECIALISATION of Gen also has an access rule — and `UPDATE SECURITY`, project-wide or scoped, reported \"All entity access rules are up to date\" and changed nothing (mendixlabs/mxcli#1047, reported against 0.21.0 on MPR v2, reproduced on 11.12.1 on BOTH engines). The reporter's own tool named the one missing member — `+ Spec: ProbeSecond.Gen.AfterSpec (ReadWrite)` — and took mx check 1 -> 0.", "cause": "ReconcileMemberAccesses computes the same-module ancestor set (sameModuleAncestors -> ownerIDs) and then uses it ONLY for the association pass. The attribute pass walked the entity's OWN attributes (ent.AttributesItems() / entityDoc[\"Attributes\"]), so a specialisation's expected member set never contained an inherited attribute: nothing looked missing, nothing was added, and `modified` stayed 0 — which the command prints as \"up to date\". Fixed by walking the chain for attributes too, qualifying each against the entity that DECLARES it, with a nearer entity's attribute shadowing an ancestor's of the same name.", "file": "`mdl/backend/modelsdk/domainmodel_security_write.go` (ReconcileMemberAccesses, collectAttrs) and `sdk/mpr/writer_security.go` (entityAttrsInChain, ownAttrsOf, generalizationRefOf); tests `mdl/backend/modelsdk/reconcile_inherited_attr_test.go`, `sdk/mpr/writer_security_inherited_test.go`; example `mdl-examples/bug-tests/1047-inherited-member-reconcile.mdl`", "insight": "The bug was one line of REUSE that never happened: the ancestor set was already computed two lines above the attribute loop and only the association loop consumed it. When a function computes a set and uses it for one of two symmetric passes, check the other pass. Three things worth carrying: (1) A count that drives a message is part of the contract — reporting 0 modified is what turned a broken model into the words \"All entity access rules are up to date\", and a false success is worse than an error because it ends the investigation. Assert the count, not just the stored state. (2) Legacy keyed coverage by BARE attribute name while preserving any ref not qualified against this entity, so naively adding inherited attributes would have preserved AND re-added the same member — the duplicate only shows on the second reconcile, so the regression test runs it twice. (3) The repair path cannot be proven by an .mdl file once the write path is fixed: ALTER ENTITY reconciles as it writes, so the script ends clean either way. Break the project with a PRE-FIX binary and repair it with the fixed one — that finally showed `update security` taking a real CE0066 from 1 to 0, which an earlier fix had recorded as unproven.", "ce": ["CE0066"]} diff --git a/CHANGELOG.md b/CHANGELOG.md index 32fc01fd45..f7828dc860 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,16 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] +### Fixed + +- **An attribute added to a generalization left every specialization's access rule short a member — and `update security` said the model was fine** (mendixlabs/mxcli#1047). `ALTER ENTITY M.Gen ADD ATTRIBUTE …`, where a specialization of `Gen` also has an access rule, produced **CE0066 "Entity access is out of date"**; `UPDATE SECURITY` — project-wide or scoped, the command that exists to repair exactly that — reported **"All entity access rules are up to date"** and changed nothing. + + `ReconcileMemberAccesses` computes the same-module ancestor set and then used it **only** for associations. The attribute pass beside it walked the entity's own attributes, so a specialization's expected member set never contained what it inherits: nothing looked missing, nothing was added, and the 0 it returned is what the command prints as "up to date". A false success, which is worse than an error — it ends the investigation. Both engines had it in the same shape and both are fixed; a fix in one of these parallel writers stays latent in the other until something switches engines. + + Inherited members are now included, each qualified against the entity that **declares** it (`M.Gen.AfterSpec`, not `M.Spec.AfterSpec` — that is CE1613), with a child attribute shadowing an ancestor's of the same name. An ancestor in another module is still neither added nor pruned, since its domain model is not loaded here. + + Measured end to end on 11.12.1, both engines: the reporter's repro now checks clean, and a project broken by the **pre-fix** binary is repaired by the fixed one — `Reconciled 1 access rule(s) in module ProbeSecond`, `mx check` 1 → 0. That also closes a gap an earlier fix recorded explicitly: `update security` had never been shown repairing a real CE0066, because once the write path reconciles, no MDL script can produce one. + ### Added - **`mxcli lint` reports a navigation screen that cannot be linked to (CONV019)** — a Mendix page is reachable at `/p/` only if it has been given a URL; without one it exists solely at the end of a click path and cannot be bookmarked, shared, reopened after a refresh, or captured with `--screenshot-url` (ako/CapTrackV4 FINDINGS 014). diff --git a/docs-wiki/bug-patterns/access-rule-reconciliation.md b/docs-wiki/bug-patterns/access-rule-reconciliation.md index 34181c2bb7..30e6edac13 100644 --- a/docs-wiki/bug-patterns/access-rule-reconciliation.md +++ b/docs-wiki/bug-patterns/access-rule-reconciliation.md @@ -53,6 +53,28 @@ having "no roles" is the same class seen from the query side, and it is the shap most likely to be believed, because "no roles" reads like a finding rather than a gap. +**Reconciliation has two directions and they are fixed separately.** Every +finding above is reconciliation *removing* something. The other direction — +failing to *add* a member the entity has gained — produced the same `CE0066` +from the opposite side, and stayed open through several fixes to this function +because each one was aimed at the loss. The ancestor set was being computed and +consumed by the association pass only; the attribute pass beside it walked the +entity's own attributes and no one noticed the asymmetry. **When a function +computes a set and uses it for one of two symmetric passes, check the other.** + +**A count that drives a message is part of the contract.** The same defect made +`update security` print *"All entity access rules are up to date"* over a project +`mx check` rejects, because the reconcile reported 0 modified. That is worse than +an error: an error is investigated, a false success ends the investigation, and +the reporter reasonably concluded the command was broken rather than the model. +Assert the count a repair command returns, not only the state it leaves behind. + +**Once the write path reconciles, an MDL script can no longer prove the repair +path.** The statement that used to break the model now fixes it as it writes, so +the script ends clean against a build that never had the fix. Break the project +with a *previous* binary and repair it with the new one — that is what finally +showed this command taking a real `CE0066` from 1 to 0. + ## See also - [fix-issue findings](../../.claude/skills/fix-issue/findings/) — the member diff --git a/mdl-examples/bug-tests/1047-inherited-member-reconcile.mdl b/mdl-examples/bug-tests/1047-inherited-member-reconcile.mdl new file mode 100644 index 0000000000..8e5dba58b0 --- /dev/null +++ b/mdl-examples/bug-tests/1047-inherited-member-reconcile.mdl @@ -0,0 +1,79 @@ +-- ============================================================================ +-- CE0066 after adding an attribute to a GENERALIZATION, and `UPDATE SECURITY` +-- reporting "up to date" over it (mendixlabs/mxcli#1047) +-- ============================================================================ +-- +-- Symptom (before fix), reported against 0.21.0 on a confirmed MPR v2 project +-- and reproduced here on Mendix 11.12.1 with both engines: +-- +-- gen + grant, specialisation + grant → mx check 0 errors (control) +-- ALTER ENTITY ...Gen ADD ATTRIBUTE AfterSpec → CE0066 at the domain model +-- +-- | command | mxcli said | +-- |----------------------------------|-------------------------------------| +-- | UPDATE SECURITY (project-wide) | "All entity access rules are up to | +-- | | date" → CE0066 unchanged | +-- | UPDATE SECURITY ProbeSecond | same | +-- +-- The reporter's own tool named the one missing member and fixed it: +-- + Spec: ProbeSecond.Gen.AfterSpec (ReadWrite) → mx check 1 → 0 +-- +-- Root cause: +-- ReconcileMemberAccesses computes the ancestor set (sameModuleAncestors) and +-- used it ONLY for associations. The attribute pass walked the entity's OWN +-- attributes, so a specialisation's expected member set never contained the +-- members it inherits. Nothing looked missing, nothing was added, `modified` +-- stayed 0 — and that 0 is what the command prints as "up to date". A false +-- success, which is worse than an error: it tells you the model is fine. +-- +-- Both engines had it in the same shape. The codec engine is +-- mdl/backend/modelsdk/domainmodel_security_write.go; the legacy engine is +-- sdk/mpr/writer_security.go. Fixing one would have left the other latent +-- until something switched engines. +-- +-- After fix: +-- The inherited attribute is included, qualified against the entity that +-- DECLARES it (`ProbeSecond.Gen.AfterSpec`, not `ProbeSecond.Spec.AfterSpec` +-- — that would be CE1613). A child attribute shadows an ancestor's of the +-- same name, matching the executor's own member walk. +-- +-- Measured end to end: a project broken by the pre-fix binary is repaired by +-- the fixed one — "Reconciled 1 access rule(s) in module ProbeSecond", and +-- mx check goes 1 → 0, on both engines and on both the project-wide and the +-- scoped form. That also closes a gap an earlier fix recorded explicitly: +-- `update security` had never been shown repairing a real CE0066. +-- +-- NOTE ON RUNNING THIS FILE +-- With the fix in place, ALTER ENTITY reconciles as it writes, so this script +-- ends at 0 errors WITHOUT needing `update security` at all. It therefore +-- proves the write path, not the repair path — the repair path needs a +-- project broken by an older binary, which a .mdl file cannot produce. The +-- Go tests carry that half (both engines), and the controls are there. +-- +-- Usage: +-- mxcli exec mdl-examples/bug-tests/1047-inherited-member-reconcile.mdl -p app.mpr +-- mxcli docker check -p app.mpr --no-update-widgets -- must be 0 errors +-- MXCLI_ENGINE=legacy mxcli exec ... -- same on both engines +-- ============================================================================ + +create module ProbeSecond; +create module role ProbeSecond.User; + +create persistent entity ProbeSecond."Gen" ( Name: string(200) ); +create persistent entity ProbeSecond."Spec" extends ProbeSecond."Gen" ( Extra: string(50) ); + +grant ProbeSecond.User on ProbeSecond."Gen" (READ *, WRITE *); +grant ProbeSecond.User on ProbeSecond."Spec" (READ *, WRITE *); + +-- The control: everything above checks clean. A run that only executes the +-- ALTER below cannot tell a fixed build from a broken one. + +-- The reported trigger. The specialisation's rule needs an entry for this +-- attribute even though the specialisation does not declare it. +ALTER ENTITY ProbeSecond."Gen" ADD ATTRIBUTE IF NOT EXISTS "AfterSpec": String(50); + +-- Idempotent: the command that used to report "up to date" over a broken model +-- now reports honestly, and re-running it changes nothing. +UPDATE SECURITY ProbeSecond; + +describe entity ProbeSecond."Spec"; diff --git a/mdl/backend/modelsdk/domainmodel_security_write.go b/mdl/backend/modelsdk/domainmodel_security_write.go index b17ef10c41..f7ba8fdbeb 100644 --- a/mdl/backend/modelsdk/domainmodel_security_write.go +++ b/mdl/backend/modelsdk/domainmodel_security_write.go @@ -371,12 +371,39 @@ func (b *Backend) ReconcileMemberAccesses(unitID model.ID, moduleName string) (i // part of the chain that lives in THIS module can be walked; an ancestor // in another module is handled by preserving its references below rather // than by resolving them. + ancestors := sameModuleAncestors(ent, byName, moduleName) ownerIDs := map[string]bool{entityID: true} - for _, anc := range sameModuleAncestors(ent, byName, moduleName) { + for _, anc := range ancestors { ownerIDs[string(anc.ID())] = true } - // Attributes (in order) with calculated flags. + // Attributes (in order) with calculated flags — the entity's OWN and + // those it INHERITS from a generalization in this module. + // + // The ancestor walk above used to feed the ASSOCIATION pass only, so a + // specialization's expected attribute set was its own attributes and + // nothing else. Adding an attribute to a generalization therefore left + // every specialization's rule short of a member, which Mendix reports as + // CE0066 "Entity access is out of date" — and `UPDATE SECURITY`, the + // command that exists to repair it, found nothing missing, reported 0 + // modified, and printed "All entity access rules are up to date" over a + // project mx check rejects (mendixlabs/mxcli#1047, reported against + // 0.21.0 and reproduced on both engines). + // + // Each reference is qualified against the entity that DECLARES the + // member, which is what Mendix stores; qualifying an inherited one + // against this entity is CE1613 "The selected attribute no longer + // exists". A child attribute SHADOWS an ancestor's of the same name, as + // the executor's own member walk (EntityMembersFor) already treats it — + // emitting both would put two entries in the rule for one member. + // + // An ancestor in ANOTHER module is still not resolvable here, so its + // members are neither added nor pruned: the existing entries are carried + // through by the "preserve what cannot be checked" branch below. For the + // same reason a stale inherited entry — one whose ancestor has since + // dropped the attribute — is still preserved rather than removed; that is + // the opposite direction from this defect and #1047's own control reports + // "+1 added, -0 removed". type attrInfo struct { qn string calc bool @@ -384,19 +411,30 @@ func (b *Backend) ReconcileMemberAccesses(unitID model.ID, moduleName string) (i var attrs []attrInfo attrSet := map[string]bool{} calcSet := map[string]bool{} - for _, ae := range ent.AttributesItems() { - a, ok := ae.(*genDm.Attribute) - if !ok { - continue - } - qn := moduleName + "." + entityName + "." + a.Name() - _, isCalc := a.Value().(*genDm.CalculatedValue) - attrs = append(attrs, attrInfo{qn, isCalc}) - attrSet[qn] = true - if isCalc { - calcSet[qn] = true + claimed := map[string]bool{} + collectAttrs := func(owner *genDm.Entity, ownerName string) { + for _, ae := range owner.AttributesItems() { + a, ok := ae.(*genDm.Attribute) + if !ok { + continue + } + if claimed[a.Name()] { + continue // shadowed by a nearer entity in the chain + } + claimed[a.Name()] = true + qn := moduleName + "." + ownerName + "." + a.Name() + _, isCalc := a.Value().(*genDm.CalculatedValue) + attrs = append(attrs, attrInfo{qn, isCalc}) + attrSet[qn] = true + if isCalc { + calcSet[qn] = true + } } } + collectAttrs(ent, entityName) + for _, anc := range ancestors { // nearest first, so the nearer shadows + collectAttrs(anc, anc.Name()) + } // FROM-side associations (ParentPointer == this entity), regular + cross. var assocQNs []string diff --git a/mdl/backend/modelsdk/reconcile_inherited_attr_test.go b/mdl/backend/modelsdk/reconcile_inherited_attr_test.go new file mode 100644 index 0000000000..8f1f9419d9 --- /dev/null +++ b/mdl/backend/modelsdk/reconcile_inherited_attr_test.go @@ -0,0 +1,134 @@ +// SPDX-License-Identifier: Apache-2.0 + +// mendixlabs/mxcli#1047: adding an attribute to a GENERALIZATION left the +// project at CE0066 "Entity access is out of date", and `UPDATE SECURITY` — +// the command that exists to repair exactly that — reported "All entity access +// rules are up to date" and changed nothing. Reported against 0.21.0 on a +// confirmed MPR v2 project; reproduced on both engines. +// +// ReconcileMemberAccesses computes the ancestor set (ownerIDs, via +// sameModuleAncestors) and then uses it ONLY for associations. The attribute +// pass walked ent.AttributesItems() — the entity's own attributes — so a +// specialization's expected member set never contained the members it inherits. +// Nothing looked missing, nothing was added, and `modified` stayed 0, which is +// what the command turns into "up to date". +// +// The sibling case (a rule that HAS the inherited entry, which reconcile must +// not delete) is covered by reconcile_inherited_assoc_test.go; this is the +// other direction, the one #1047 reports. +package modelsdkbackend + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/types" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/domainmodel" +) + +// shadowFixture builds ZzShadowParent <- ZzShadowChild where BOTH declare an +// attribute called Code, so the child's shadows the ancestor's. +func shadowFixture(t *testing.T) (*Backend, *model.Module, *domainmodel.DomainModel) { + t.Helper() + b, mod, dm := inheritanceFixture(t) + + parent := &domainmodel.Entity{Name: "ZzShadowParent", Persistable: true, + Attributes: []*domainmodel.Attribute{{Name: "Code", Type: &domainmodel.StringAttributeType{}}}} + if err := b.CreateEntity(dm.ID, parent); err != nil { + t.Fatalf("CreateEntity ZzShadowParent: %v", err) + } + child := &domainmodel.Entity{Name: "ZzShadowChild", Persistable: true, + GeneralizationRef: "MyFirstModule.ZzShadowParent", + Attributes: []*domainmodel.Attribute{{Name: "Code", Type: &domainmodel.StringAttributeType{}}}} + if err := b.CreateEntity(dm.ID, child); err != nil { + t.Fatalf("CreateEntity ZzShadowChild: %v", err) + } + return b, mod, dm +} + +// The reported repro, at this layer: the ancestor gains an attribute after the +// specialization's rule was written, so the rule is missing an inherited member +// and Mendix reports CE0066. Reconcile has to add it — qualified against the +// entity that DECLARES it, which is what Mendix stores and what the reporter's +// own tool added ("+ Spec: ProbeSecond.Gen.AfterSpec (ReadWrite)"). +func TestReconcile_AddsAncestorAttributeToASpecializationsRule(t *testing.T) { + b, mod, dm := inheritanceFixture(t) + + // A rule holding only the specialization's own attribute — the state a + // project reaches when the generalization gains an attribute afterwards. + grantAll(t, b, dm.ID, "ZzDerived", []types.EntityMemberAccess{ + {AttributeRef: "MyFirstModule.ZzDerived.Extra", AccessRights: "ReadWrite"}, + }) + + modified, err := b.ReconcileMemberAccesses(dm.ID, mod.Name) + if err != nil { + t.Fatalf("ReconcileMemberAccesses: %v", err) + } + + refs := memberRefs(t, b, mod.ID, "ZzDerived") + if !hasRef(refs, "attr:MyFirstModule.ZzBase.Code") { + t.Fatalf("reconcile did not add the inherited attribute: %v\n"+ + "this is the CE0066 in mendixlabs/mxcli#1047, and the rule Mendix rejects", refs) + } + // The count is what `update security` prints. Reporting 0 here is how the + // command came to say "All entity access rules are up to date" over a + // project that mx check rejects — a false success, worse than an error. + if modified == 0 { + t.Error("reconcile added the member but reported 0 modified; `update security` would still say 'up to date'") + } + + // The specialization's own attribute must survive the change. + if !hasRef(refs, "attr:MyFirstModule.ZzDerived.Extra") { + t.Errorf("reconcile dropped the entity's own attribute: %v", refs) + } +} + +// The control that keeps the test above honest: reconcile must not invent +// members for an entity with no generalization. If it reported a change here, +// the test above would pass against a fix that simply marks everything dirty. +func TestReconcile_LeavesAnUnrelatedEntitysRuleAlone(t *testing.T) { + b, mod, dm := inheritanceFixture(t) + + grantAll(t, b, dm.ID, "ZzOther", []types.EntityMemberAccess{ + {AttributeRef: "MyFirstModule.ZzOther.Label", AccessRights: "ReadWrite"}, + }) + + before := memberRefs(t, b, mod.ID, "ZzOther") + if _, err := b.ReconcileMemberAccesses(dm.ID, mod.Name); err != nil { + t.Fatalf("ReconcileMemberAccesses: %v", err) + } + after := memberRefs(t, b, mod.ID, "ZzOther") + + if len(before) != len(after) { + t.Errorf("an entity with no generalization changed: %v -> %v", before, after) + } + for _, r := range after { + if r != "attr:MyFirstModule.ZzOther.Label" { + t.Errorf("reconcile invented a member on an unrelated entity: %v", after) + } + } +} + +// An attribute the specialization declares under a name its ancestor also uses +// SHADOWS the ancestor's, exactly as the executor's own member walk +// (EntityMembersFor) treats it. Emitting both would put two entries in the rule +// for one member the modeller sees. +func TestReconcile_ChildAttributeShadowsTheAncestorsOfTheSameName(t *testing.T) { + b, mod, dm := shadowFixture(t) + + grantAll(t, b, dm.ID, "ZzShadowChild", []types.EntityMemberAccess{ + {AttributeRef: "MyFirstModule.ZzShadowChild.Code", AccessRights: "ReadWrite"}, + }) + + if _, err := b.ReconcileMemberAccesses(dm.ID, mod.Name); err != nil { + t.Fatalf("ReconcileMemberAccesses: %v", err) + } + + refs := memberRefs(t, b, mod.ID, "ZzShadowChild") + if hasRef(refs, "attr:MyFirstModule.ZzShadowParent.Code") { + t.Errorf("the ancestor's shadowed attribute was added alongside the child's: %v", refs) + } + if !hasRef(refs, "attr:MyFirstModule.ZzShadowChild.Code") { + t.Errorf("the child's own attribute was lost: %v", refs) + } +} diff --git a/sdk/mpr/writer_security.go b/sdk/mpr/writer_security.go index 2090d7e18f..d51ba97554 100644 --- a/sdk/mpr/writer_security.go +++ b/sdk/mpr/writer_security.go @@ -1306,38 +1306,34 @@ func (w *Writer) ReconcileMemberAccesses(unitID model.ID, moduleName string) (in continue } - // Collect current attribute names and track calculated attributes - attrNames := map[string]bool{} - calculatedAttrs := map[string]bool{} - attrsArr := getBsonArray(entityDoc, "Attributes") - for _, attrItem := range attrsArr { - attrDoc, ok := attrItem.(bson.D) - if !ok { - continue - } - attrName := "" - isCalculated := false - for _, f := range attrDoc { - if f.Key == "Name" { - attrName = bsonutil.String(f.Value, "Name") - } - if f.Key == "Value" { - if valueDoc, ok := f.Value.(bson.D); ok { - for _, vf := range valueDoc { - if vf.Key == "$Type" { - if vt, ok := vf.Value.(string); ok && vt == "DomainModels$CalculatedValue" { - isCalculated = true - } - } - } - } - } - } - if attrName != "" { - attrNames[attrName] = true - if isCalculated { - calculatedAttrs[attrName] = true - } + // The attributes this entity's rules must cover: its OWN and those it + // INHERITS from a generalization in this module, each qualified + // against the entity that DECLARES it — which is what Mendix stores, + // and what makes an inherited entry's reference name an ancestor + // rather than this entity. + // + // Collecting only the entity's own attributes left every + // specialization's rule short of a member as soon as the + // generalization gained one, which Mendix reports as CE0066 "Entity + // access is out of date" — and `UPDATE SECURITY`, the command that + // exists to repair it, found nothing missing and reported "All entity + // access rules are up to date" over a project mx check rejects + // (mendixlabs/mxcli#1047, reported against 0.21.0). The codec engine + // had the same defect in the same shape; both are fixed together, + // because a fix in one of these parallel writers leaves the other + // latent until something switches engines. + // + // Keyed by full reference rather than bare name: the compare pass + // below preserves any reference not qualified against this entity, so + // a bare-name key would mark an inherited entry uncovered and ADD a + // second copy of a member the rule already has. + expectedAttrs := entityAttrsInChain(entitiesArr, entityName, moduleName) + expectedAttrRefs := map[string]bool{} + calculatedAttrRefs := map[string]bool{} + for _, ea := range expectedAttrs { + expectedAttrRefs[ea.ref] = true + if ea.calculated { + calculatedAttrRefs[ea.ref] = true } } @@ -1511,26 +1507,27 @@ func (w *Writer) ReconcileMemberAccesses(unitID model.ID, moduleName string) (in } if attrRef != "" { - // Extract attribute name from Module.Entity.AttrName - parts := splitQualifiedRef(attrRef) - // An inherited member's reference is qualified against the - // entity that DECLARES it, so it does not match this - // entity's own attribute list and used to be deleted as - // stale (mendixlabs/mxcli#758). The ancestor may live in - // another module or in System, neither loaded here, so an - // inherited reference cannot be validated at this layer — - // preserve what cannot be checked. Mirrors the codec engine - // (mdl/backend/modelsdk.attrRefBelongsTo). - if !attrRefBelongsToEntity(attrRef, moduleName, entityName) { - filtered = append(filtered, maDoc) - } else if parts != "" && attrNames[parts] { - coveredAttrs[parts] = true + switch { + case expectedAttrRefs[attrRef]: + // A member the entity has — its own, or one inherited + // from a generalization in this module. + coveredAttrs[attrRef] = true // Downgrade write rights on calculated attributes (CE6592) - if calculatedAttrs[parts] { + if calculatedAttrRefs[attrRef] { maDoc = downgradeCalculatedAttrRights(maDoc) } filtered = append(filtered, maDoc) - } else { + case !attrRefBelongsToEntity(attrRef, moduleName, entityName): + // An inherited member's reference is qualified against the + // entity that DECLARES it, so it does not match this + // entity's own attribute list and used to be deleted as + // stale (mendixlabs/mxcli#758). The ancestor may live in + // another module or in System, neither loaded here, so an + // inherited reference cannot be validated at this layer — + // preserve what cannot be checked. Mirrors the codec engine + // (mdl/backend/modelsdk.attrRefBelongsTo). + filtered = append(filtered, maDoc) + default: changed = true // stale attribute entry removed } } else if assocRef != "" { @@ -1553,19 +1550,22 @@ func (w *Writer) ReconcileMemberAccesses(unitID model.ID, moduleName string) (in } } - // Add missing attributes - for attrName := range attrNames { - if !coveredAttrs[attrName] { + // Add missing attributes, in declaration order (own first, + // then each ancestor's). Iterating the map instead made the + // order of new entries vary between runs, so two identical + // reconciles could produce different bytes. + for _, ea := range expectedAttrs { + if !coveredAttrs[ea.ref] { rights := defaultRights // Calculated attributes cannot have write rights (CE6592) - if calculatedAttrs[attrName] && (rights == "ReadWrite" || rights == "WriteOnly") { + if ea.calculated && (rights == "ReadWrite" || rights == "WriteOnly") { rights = "ReadOnly" } newMA := bson.D{ {Key: "$ID", Value: idToBsonBinary(generateUUID())}, {Key: "$Type", Value: "DomainModels$MemberAccess"}, {Key: "AccessRights", Value: rights}, - {Key: "Attribute", Value: moduleName + "." + entityName + "." + attrName}, + {Key: "Attribute", Value: ea.ref}, } filtered = append(filtered, newMA) changed = true @@ -1705,6 +1705,131 @@ func stripInvalidAccessRuleProps(doc bson.D) (bson.D, bool) { // ensure primitive import is used var _ = primitive.Binary{} +// chainAttr is one attribute of an entity's access surface: the reference +// Mendix stores for it, and whether it is calculated (which caps its rights). +type chainAttr struct { + ref string // "Module.DeclaringEntity.Attribute" + calculated bool +} + +// entityAttrsInChain returns the attributes an entity's access rules must +// cover — its own, then those of each generalization that lives in THIS module, +// nearest ancestor first — each qualified against the entity that declares it. +// +// A nearer entity's attribute SHADOWS an ancestor's of the same name, matching +// the executor's own member walk (EntityMembersFor): emitting both would put two +// entries in the rule for one member the modeller sees. +// +// The walk stops at the first ancestor outside this module (or one that cannot +// be found), because only this module's domain model is loaded here. Those +// members are neither added nor pruned — the compare pass preserves the entries +// that already reference them. +func entityAttrsInChain(entitiesArr bson.A, entityName, moduleName string) []chainAttr { + byName := map[string]bson.D{} + for _, item := range entitiesArr { + ed, ok := item.(bson.D) + if !ok { + continue + } + for _, f := range ed { + if f.Key == "Name" { + if n := bsonutil.String(f.Value, "Name"); n != "" { + byName[n] = ed + } + break + } + } + } + + var out []chainAttr + claimed := map[string]bool{} // bare attribute name -> already taken by a nearer entity + seen := map[string]bool{} // cycle guard + + for name := entityName; name != ""; { + ed, ok := byName[name] + if !ok || seen[name] { + break + } + seen[name] = true + + for _, ca := range ownAttrsOf(ed, moduleName, name) { + bare := ca.ref[strings.LastIndex(ca.ref, ".")+1:] + if claimed[bare] { + continue + } + claimed[bare] = true + out = append(out, ca) + } + + // Step to the generalization, if it is in this module. + genRef := generalizationRefOf(ed) + idx := strings.LastIndex(genRef, ".") + if idx < 0 || !strings.EqualFold(genRef[:idx], moduleName) { + break + } + name = genRef[idx+1:] + } + return out +} + +// ownAttrsOf reads one entity document's own attributes. +func ownAttrsOf(entityDoc bson.D, moduleName, entityName string) []chainAttr { + var out []chainAttr + for _, attrItem := range getBsonArray(entityDoc, "Attributes") { + attrDoc, ok := attrItem.(bson.D) + if !ok { + continue + } + attrName := "" + isCalculated := false + for _, f := range attrDoc { + if f.Key == "Name" { + attrName = bsonutil.String(f.Value, "Name") + } + if f.Key == "Value" { + if valueDoc, ok := f.Value.(bson.D); ok { + for _, vf := range valueDoc { + if vf.Key == "$Type" { + if vt, ok := vf.Value.(string); ok && vt == "DomainModels$CalculatedValue" { + isCalculated = true + } + } + } + } + } + } + if attrName != "" { + out = append(out, chainAttr{ + ref: moduleName + "." + entityName + "." + attrName, + calculated: isCalculated, + }) + } + } + return out +} + +// generalizationRefOf returns the qualified name of an entity's generalization +// ("Module.Entity"), or "" when it has none. Newer formats store the field as +// MaybeGeneralization; a NoGeneralization carries no reference. +func generalizationRefOf(entityDoc bson.D) string { + for _, f := range entityDoc { + if f.Key != "Generalization" && f.Key != "MaybeGeneralization" { + continue + } + gd, ok := f.Value.(bson.D) + if !ok { + return "" + } + for _, gf := range gd { + if gf.Key == "Generalization" { + return bsonutil.String(gf.Value, "Generalization") + } + } + return "" + } + return "" +} + // attrRefBelongsToEntity reports whether a MemberAccess attribute reference // ("Module.Entity.Attribute") names one of the given entity's OWN attributes, // rather than one inherited from an ancestor. Only an own reference can be diff --git a/sdk/mpr/writer_security_inherited_test.go b/sdk/mpr/writer_security_inherited_test.go new file mode 100644 index 0000000000..57f4a37f8d --- /dev/null +++ b/sdk/mpr/writer_security_inherited_test.go @@ -0,0 +1,229 @@ +// SPDX-License-Identifier: Apache-2.0 + +package mpr + +import ( + "database/sql" + "testing" + + "github.com/mendixlabs/mxcli/model" + "go.mongodb.org/mongo-driver/bson" +) + +// mendixlabs/mxcli#1047: adding an attribute to a GENERALIZATION left the +// project at CE0066 "Entity access is out of date", and `UPDATE SECURITY` +// reported "All entity access rules are up to date" without changing anything. +// +// ReconcileMemberAccesses computed a specialization's expected member set from +// the entity's OWN attributes, so an inherited one was never missing and never +// added. Both engines had it, in the same shape; this is the legacy half. + +// seedGeneralizationChain inserts a domain model holding Gen (attribute Name) +// and Spec (extends Gen, attribute Extra), each with one access rule. The rules +// list only what the entity declares itself, which is the state a project +// reaches when the generalization gains an attribute afterwards. +func seedGeneralizationChain(t *testing.T, db *sql.DB) model.ID { + t.Helper() + + const ( + unitIDStr = "11111111-1111-1111-1111-111111111111" + containerIDStr = "22222222-2222-2222-2222-222222222222" + genIDStr = "33333333-3333-3333-3333-333333333333" + specIDStr = "55555555-5555-5555-5555-555555555555" + ) + + attr := func(id, name string) bson.D { + return bson.D{ + {Key: "$Type", Value: "DomainModels$StoredValue"}, + {Key: "$ID", Value: idToBsonBinary(id)}, + {Key: "Name", Value: name}, + } + } + rule := func(id string, members bson.A) bson.D { + return bson.D{ + {Key: "$Type", Value: "DomainModels$AccessRule"}, + {Key: "$ID", Value: idToBsonBinary(id)}, + {Key: "AllowedModuleRoles", Value: bson.A{int32(1), "MyModule.Administrator"}}, + {Key: "DefaultMemberAccessRights", Value: "ReadWrite"}, + {Key: "MemberAccesses", Value: members}, + } + } + member := func(id, ref string) bson.D { + return bson.D{ + {Key: "$ID", Value: idToBsonBinary(id)}, + {Key: "$Type", Value: "DomainModels$MemberAccess"}, + {Key: "AccessRights", Value: "ReadWrite"}, + {Key: "Attribute", Value: ref}, + } + } + + dmBSON := bson.D{ + {Key: "$Type", Value: "DomainModels$DomainModel"}, + {Key: "$ID", Value: idToBsonBinary(unitIDStr)}, + {Key: "Entities", Value: bson.A{ + int32(3), + bson.D{ + {Key: "$Type", Value: "DomainModels$Entity"}, + {Key: "$ID", Value: idToBsonBinary(genIDStr)}, + {Key: "Name", Value: "Gen"}, + {Key: "Attributes", Value: bson.A{int32(3), attr(attrIDForIndex(0), "Name")}}, + {Key: "AccessRules", Value: bson.A{int32(3), + rule("44444444-4444-4444-4444-444444444444", bson.A{ + int32(3), member("66666666-6666-6666-6666-666666666666", "MyModule.Gen.Name"), + })}}, + }, + bson.D{ + {Key: "$Type", Value: "DomainModels$Entity"}, + {Key: "$ID", Value: idToBsonBinary(specIDStr)}, + {Key: "Name", Value: "Spec"}, + {Key: "Attributes", Value: bson.A{int32(3), attr(attrIDForIndex(1), "Extra")}}, + {Key: "Generalization", Value: bson.D{ + {Key: "$Type", Value: "DomainModels$Generalization"}, + {Key: "$ID", Value: idToBsonBinary("77777777-7777-7777-7777-777777777777")}, + {Key: "Generalization", Value: "MyModule.Gen"}, + }}, + {Key: "AccessRules", Value: bson.A{int32(3), + rule("88888888-8888-8888-8888-888888888888", bson.A{ + int32(3), member("99999999-9999-9999-9999-999999999999", "MyModule.Spec.Extra"), + })}}, + }, + }}, + {Key: "Associations", Value: bson.A{int32(3)}}, + } + + contents, err := bson.Marshal(dmBSON) + if err != nil { + t.Fatalf("marshal domain model: %v", err) + } + if _, err := db.Exec(` + INSERT INTO Unit (UnitID, ContainerID, ContainmentName, TreeConflict, ContentsHash, ContentsConflicts, Contents) + VALUES (?, ?, 'DomainModel', 0, ?, '', ?)`, + uuidToBlob(unitIDStr), uuidToBlob(containerIDStr), + contentHashBase64(contents), contents, + ); err != nil { + t.Fatalf("insert domain model unit: %v", err) + } + return model.ID(unitIDStr) +} + +// memberRefsOfEntity reads one named entity's first rule's attribute references. +func memberRefsOfEntity(t *testing.T, db *sql.DB, unitID model.ID, entityName string) []string { + t.Helper() + var contents []byte + if err := db.QueryRow(`SELECT Contents FROM Unit WHERE UnitID = ?`, + uuidToBlob(string(unitID))).Scan(&contents); err != nil { + t.Fatalf("read unit: %v", err) + } + var raw map[string]any + if err := bson.Unmarshal(contents, &raw); err != nil { + t.Fatalf("unmarshal: %v", err) + } + for _, e := range extractBsonArray(raw["Entities"]) { + ent, ok := e.(map[string]any) + if !ok || extractString(ent["Name"]) != entityName { + continue + } + rules := extractBsonArray(ent["AccessRules"]) + if len(rules) == 0 { + t.Fatalf("entity %s has no access rules", entityName) + } + var refs []string + for _, ma := range extractBsonArray(rules[0].(map[string]any)["MemberAccesses"]) { + if m, ok := ma.(map[string]any); ok { + refs = append(refs, extractString(m["Attribute"])) + } + } + return refs + } + t.Fatalf("entity %s not found", entityName) + return nil +} + +func hasString(vals []string, want string) bool { + for _, v := range vals { + if v == want { + return true + } + } + return false +} + +func TestReconcileMemberAccesses_AddsAnInheritedAttribute(t *testing.T) { + w, db := newTestWriterSecurity(t) + unitID := seedGeneralizationChain(t, db) + + // Read the seed back before asking anything of it: a fixture that failed to + // store the chain would make the assertions below meaningless. + if refs := memberRefsOfEntity(t, db, unitID, "Spec"); len(refs) != 1 || refs[0] != "MyModule.Spec.Extra" { + t.Fatalf("fixture did not store the specialization's rule as expected: %v", refs) + } + + modified, err := w.ReconcileMemberAccesses(unitID, "MyModule") + if err != nil { + t.Fatalf("ReconcileMemberAccesses: %v", err) + } + + refs := memberRefsOfEntity(t, db, unitID, "Spec") + if !hasString(refs, "MyModule.Gen.Name") { + t.Fatalf("the inherited attribute was not added: %v\n"+ + "this is the CE0066 in mendixlabs/mxcli#1047 — and the reference must name Gen, "+ + "the entity that DECLARES it, not Spec", refs) + } + if !hasString(refs, "MyModule.Spec.Extra") { + t.Errorf("the specialization's own attribute was dropped: %v", refs) + } + // The count is what `update security` turns into its message. Reporting 0 + // while adding a member is how "All entity access rules are up to date" came + // to be printed over a project mx check rejects. + if modified == 0 { + t.Error("a member was added but 0 modified was reported") + } +} + +// Running it twice must not add a second copy. The compare pass keys on the +// full reference; keying on the bare attribute name instead would leave the +// inherited entry looking uncovered on every later run. +func TestReconcileMemberAccesses_InheritedAttributeIsNotDuplicated(t *testing.T) { + w, db := newTestWriterSecurity(t) + unitID := seedGeneralizationChain(t, db) + + if _, err := w.ReconcileMemberAccesses(unitID, "MyModule"); err != nil { + t.Fatalf("first reconcile: %v", err) + } + first := memberRefsOfEntity(t, db, unitID, "Spec") + + modified, err := w.ReconcileMemberAccesses(unitID, "MyModule") + if err != nil { + t.Fatalf("second reconcile: %v", err) + } + second := memberRefsOfEntity(t, db, unitID, "Spec") + + if len(second) != len(first) { + t.Errorf("a second reconcile changed the member list: %v -> %v", first, second) + } + if modified != 0 { + t.Errorf("a second reconcile reported %d modified; an in-sync rule must be quiet", modified) + } +} + +// The generalization's own rule is already complete, so it must not change — +// otherwise the test above would pass against a fix that rewrites everything. +func TestReconcileMemberAccesses_LeavesTheGeneralizationsRuleAlone(t *testing.T) { + w, db := newTestWriterSecurity(t) + unitID := seedGeneralizationChain(t, db) + + before := memberRefsOfEntity(t, db, unitID, "Gen") + if _, err := w.ReconcileMemberAccesses(unitID, "MyModule"); err != nil { + t.Fatalf("ReconcileMemberAccesses: %v", err) + } + after := memberRefsOfEntity(t, db, unitID, "Gen") + + if len(before) != len(after) || !hasString(after, "MyModule.Gen.Name") { + t.Errorf("the generalization's rule changed: %v -> %v", before, after) + } + for _, r := range after { + if r != "MyModule.Gen.Name" { + t.Errorf("the generalization gained a member it does not declare: %v", after) + } + } +}