From 01eeb7e5f403331b6ccf1fad9280dfb728f81966 Mon Sep 17 00:00:00 2001 From: Ako Date: Sat, 31 Oct 2026 20:56:08 +0000 Subject: [PATCH 01/10] feat(lint): expose the catalog and settings data guideline rules need (#1453) Starlark rules could not read data the catalog or the project settings already hold: - microflow `excluded` and `apply_entity_access` (new ApplyEntityAccess column; always false for a nanoflow) - enumeration `values`, each with qualified name, caption and ordinal, so a rule can resolve getCaption(Module.Enum.Value) (#1301) - `constant_values()`: the run-configuration constant values - `project_settings()`: default language, OQL version, Java version, hash algorithm, time zone, rounding, decimal scale, optimistic locking, foreign keys and web client Catalog schema 30: the linter selects the new column, so a catalog cached by an older build must be rebuilt rather than answer every Microflows() query with nothing. The bump also covers #1781's message-definition edges, which shipped without one. The lint skill moves its security and settings structs to security-and-settings.md to stay under the 700-line body limit. Co-Authored-By: Claude Opus 5.5 --- .../skills/mendix/write-lint-rules/SKILL.md | 104 +++++------------ .../write-lint-rules/security-and-settings.md | 94 +++++++++++++++ CHANGELOG.md | 1 + mdl/catalog/builder_microflows.go | 7 +- mdl/catalog/tables.go | 13 ++- mdl/linter/context.go | 53 ++++++++- mdl/linter/context_catalog_tables.go | 44 +++++++ mdl/linter/context_document_filter_test.go | 2 +- mdl/linter/context_test.go | 5 +- mdl/linter/rules/empty_test.go | 3 +- mdl/linter/starlark.go | 96 +++++++++++++++ mdl/linter/starlark_catalog_data_1453_test.go | 109 ++++++++++++++++++ .../starlark_catalog_mode_guard_test.go | 2 + mdl/linter/starlark_documented_values_test.go | 5 +- mdl/linter/starlark_flow_noun_test.go | 5 +- mdl/linter/starlark_qual004_refs_test.go | 5 +- 16 files changed, 457 insertions(+), 91 deletions(-) create mode 100644 .claude/skills/mendix/write-lint-rules/security-and-settings.md create mode 100644 mdl/linter/starlark_catalog_data_1453_test.go diff --git a/.claude/skills/mendix/write-lint-rules/SKILL.md b/.claude/skills/mendix/write-lint-rules/SKILL.md index 67adc624be..965754769b 100644 --- a/.claude/skills/mendix/write-lint-rules/SKILL.md +++ b/.claude/skills/mendix/write-lint-rules/SKILL.md @@ -11,11 +11,11 @@ description: "Write custom Starlark lint rules in .claude/lint-rules/ that run b | lines | section | |---|---| | 25-67 | Rule File Structure | -| 68-161 | Available Query Functions | -| 162-606 | Object Properties | -| 607-620 | Helper Functions | -| 621-707 | Common Patterns | -| 708-716 | Validation | +| 68-163 | Available Query Functions | +| 164-558 | Object Properties | +| 559-572 | Helper Functions | +| 573-659 | Common Patterns | +| 660-668 | Validation | # Writing Custom Starlark Lint Rules @@ -74,6 +74,7 @@ Without this, a rule that reads a full-only table under a fast build gets | `pages()` | list of page | All non-system pages | | `enumerations()` | list of enumeration | All non-system enumerations | | `constants()` | list of constant | All non-system constants | +| `constant_values()` | list of constant_value | The constant values the run configurations set (Settings → Configurations), in constant then configuration order. A configuration that does not override a constant has no entry: it runs on the constant's `default_value` | | `widgets()` | list of widget | All non-system page and snippet widgets (full catalog — auto-detected) | | `snippets()` | list of snippet | All non-system snippets | | `scheduled_events()` | list of scheduled_event | All non-system scheduled events (requires MPR reader) | @@ -95,6 +96,7 @@ Without this, a rule that reads a full-only table under a fast build gets | `module_roles()` | list of module_role | All module roles (deduplicated from role mappings) | | `role_mappings()` | list of role_mapping | User role to module role assignments | | `project_security()` | project_security or None | Project-level security settings (requires MPR reader) | +| `project_settings()` | project_settings or None | Project-wide settings a guideline rule checks: default language, OQL version, Java version, hash algorithm, time zone, rounding, decimal scale, optimistic locking, foreign keys, web client (requires MPR reader; `None` without one) | | `languages()` | list of language | The languages **enabled** in the project settings, in settings order (requires MPR reader; `[]` without one). Use it to scope per-language checks: `strings()` has a row for every stored translation, including languages the project never enabled | | `xpath_expressions()` | list of xpath_expression | All XPath constraint expressions in the catalog (access rules, retrieve actions, widgets) (full catalog — auto-detected) | | `modules()` | list of module | The user's modules (not System, not Marketplace), with their domain model's documentation | @@ -228,6 +230,8 @@ def check(): | `complexity` | int | McCabe cyclomatic complexity | | `document_noun` | string | `"microflow"`, `"nanoflow"` or `"rule"` — for mid-sentence use in a message | | `document_noun_title` | string | `"Microflow"`, `"Nanoflow"` or `"Rule"` — for `document_type=` and a message that opens with it | +| `excluded` | bool | "Exclude from project": the flow is not built or run. Most guideline rules should skip it | +| `apply_entity_access` | bool | The flow's "Apply entity access" setting. Always `false` for a nanoflow, which has no such setting (entity access always applies there) | ### page | Property | Type | Example | @@ -252,6 +256,15 @@ def check(): | `folder` | string | `"enumerations"` — folder path within module | | `description` | string | Documentation text | | `value_count` | int | Number of enum values | +| `values` | list of enumeration_value | The values in ordinal order; `[]` for an enumeration without any | + +### enumeration_value +| Property | Type | Example | +|----------|------|---------| +| `name` | string | `"Slow"` | +| `qualified_name` | string | `"Sales.ApiTimeout.Slow"` — the name `refs_from()` and an expression's `getCaption(Sales.ApiTimeout.Slow)` use | +| `caption` | string | Caption in the project's default language: `"400"` | +| `ordinal` | int | Position in the enumeration, from 0 | ### constant | Property | Type | Example | @@ -265,6 +278,15 @@ def check(): | `default_value` | string | `"https://example.com"` | | `exposed_to_client` | bool | `true` if constant is exposed to client | +### constant_value +Returned by `constant_values()`, one per constant a run configuration overrides. + +| Property | Type | Example | +|----------|------|---------| +| `constant` | string | `"MyModule.AppBaseUrl"` — the constant's qualified name | +| `configuration` | string | `"Default"` — the run configuration's name | +| `value` | string | `"https://test.example.com"` | + **widget** — the struct returned by `widgets()` — identity, references, tree position (`parent_widget_id`, `depth`), appearance (`class_name`, `style`) and primary action (`action_type`, `has_confirmation`) — is documented in @@ -532,77 +554,7 @@ Returned by `permissions()` (all types) or `permissions_for()` (entity-specific) | `is_constrained` | bool | True if XPath constraint is set | | `default_member_access_rights` | string | The rule's "default rights for new members": `"None"`, `"ReadOnly"` or `"ReadWrite"`. Empty for non-entity permissions | -### user_role -| Property | Type | Example | -|----------|------|---------| -| `name` | string | `"Administrator"` | -| `description` | string | The role's documentation in Project Security; `""` when none | -| `is_anonymous` | bool | True if this is the anonymous/guest role | -| `module_roles` | list of string | `["Sales.Admin", "HR.Viewer"]` | -| `check_security` | bool | Studio Pro's per-role "Check security" flag | -| `manage_all_roles` | bool | The role may hand out every user role | -| `manage_users_without_roles` | bool | The role may manage users that have no role | -| `manageable_roles` | list of string | The user roles this role may hand out, when not all | - -### module_role -| Property | Type | Example | -|----------|------|---------| -| `name` | string | `"Sales.Admin"` — qualified module role name | -| `module_name` | string | `"Sales"` | -| `description` | string | Module role description | - -### role_mapping -| Property | Type | Example | -|----------|------|---------| -| `user_role_name` | string | `"Administrator"` | -| `module_role_name` | string | `"Sales.Admin"` | -| `module_name` | string | `"Sales"` | - -### reference -| Property | Type | Example | -|----------|------|---------| -| `source_type` | string | The document the edge comes FROM, upper-case: `"MICROFLOW"`, `"NANOFLOW"`, `"RULE"`, `"PAGE"`, `"SNIPPET"`, `"ENTITY"`, `"ASSOCIATION"`, `"WORKFLOW"`, `"NAVIGATION"`, `"SCHEDULED_EVENT"`, `"PUBLISHED_REST_OPERATION"`, `"PUBLISHED_REST_SERVICE"`, `"PUBLISHED_ODATA_SERVICE"`, `"CONSUMED_ODATA_SERVICE"`, `"BUSINESS_EVENT_SERVICE"`, `"PROJECT_SETTINGS"`, `"IMPORT_MAPPING"`, `"EXPORT_MAPPING"`, `"MESSAGE_DEFINITION_COLLECTION"`, `"MESSAGE_DEFINITION"` | -| `source_id` | string | Source UUID | -| `source_name` | string | `"Sales.ACT_Customer_Create"` | -| `target_type` | string | What it points AT, upper-case: `"ENTITY"`, `"ASSOCIATION"`, `"MICROFLOW"`, `"NANOFLOW"`, `"RULE"`, `"PAGE"`, `"LAYOUT"`, `"WORKFLOW"`, `"WIDGET"`, `"JAVA_ACTION"`, `"JAVASCRIPT_ACTION"`, `"REST_OPERATION"`, `"REGULAR_EXPRESSION"`, `"ATTRIBUTE"`, `"ENUMERATION"`, `"ENUMERATION_VALUE"`. `LAYOUT`, `WIDGET`, `ATTRIBUTE`, `ENUMERATION` and `ENUMERATION_VALUE` are only ever targets; `SCHEDULED_EVENT`, `PROJECT_SETTINGS` and the service types only ever sources | -| `target_id` | string | Target UUID | -| `target_name` | string | `"Sales.Customer"`; three-part for an attribute or an enumeration value: `"Sales.Order.Total"`, `"Sales.OrderStatus.Open"` | -| `ref_kind` | string | How it references: `"call"`, `"create"`, `"retrieve"`, `"change"`, `"delete"`, `"commit"` (a commit action, or a create/change that commits — beside its `"create"`/`"change"` edge; a commit of a variable whose entity the flow cannot tell has no edge), `"show_page"`, `"datasource"`, `"action"`, `"layout"`, `"parameter"`, `"return"`, `"generalize"`, `"associate"`, `"home_page"`, `"login_page"`, `"menu_item"`, `"calculate"`, `"schedule"`, `"validate"`, `"settings"`, `"widget"`, `"sync"`, `"publish"` (a published REST operation, OData resource or action, or a service's authentication microflow), `"event"` (an entity event or a business event handler), `"member"` (binds/reads/writes an attribute or navigates an association), `"xpath"` (an XPath constraint names it), `"type"` (typed as an enumeration), `"value"` (an expression names an enumeration value), `"mapping"` (an import/export mapping maps the entity, or a message definition exposes it), `"split"` (a decision branches on an object being the entity) — lower-case, unlike the types above. Attribute names used only through a variable in a free-text expression (`$Order/Total`) have no edge | -| `module_name` | string | Source module | - -### project_security - -Returned by `project_security()`. Returns `none` if no MPR reader is available. - -| Property | Type | Description | -|----------|------|-------------| -| `security_level` | string | `"CheckNothing"` (Off), `"CheckFormsAndMicroflows"` (Prototype), `"CheckEverything"` (Production) | -| `enable_demo_users` | bool | Whether demo users are enabled | -| `enable_guest_access` | bool | Whether anonymous/guest access is enabled | -| `check_security` | bool | Whether security checking is active | -| `strict_mode` | bool | Strict security mode | -| `anonymous_user_role` | string | Name of the project's guest user role, the role anonymous users get. Read `enable_guest_access` too: the role name can stay set while guest access is off | -| `admin_user_name` | string | Name of the administrator account: `"MxAdmin"`. Its password is deliberately not exposed | -| `admin_user_role` | string | The administrator account's user role: `"Administrator"` | -| `password_policy` | struct | Nested password policy settings | - -#### password_policy (nested in project_security) -| Property | Type | Description | -|----------|------|-------------| -| `min_length` | int | Minimum password length | -| `require_digit` | bool | Must contain a digit | -| `require_mixed_case` | bool | Must contain upper and lower case | -| `require_symbol` | bool | Must contain a symbol | - -### language - -Returned by `languages()`, one per language enabled in the project settings. A per-language check skips `strings()` rows whose `language` is not among these codes. - -| Property | Type | Description | -|----------|------|-------------| -| `code` | string | Language code: `"en_US"`, `"nl_NL"` | -| `is_default` | bool | Whether this is the project's default language | -| `check_completeness` | bool | Whether Studio Pro checks this language's translations for completeness | +The structs `user_roles()`, `module_roles()`, `role_mappings()`, `project_security()`, `languages()` and `project_settings()` return — `user_role`, `module_role`, `role_mapping`, `project_security`, `language`, `project_settings` — are in [security-and-settings.md](security-and-settings.md). ## Helper Functions diff --git a/.claude/skills/mendix/write-lint-rules/security-and-settings.md b/.claude/skills/mendix/write-lint-rules/security-and-settings.md new file mode 100644 index 0000000000..6cf2bff7e5 --- /dev/null +++ b/.claude/skills/mendix/write-lint-rules/security-and-settings.md @@ -0,0 +1,94 @@ +# Security and settings structs + +The structs returned by the security and settings query functions of the +[write-lint-rules](SKILL.md) skill. They read the project itself through the +MPR reader, so they need no particular catalog depth. + +### user_role +| Property | Type | Example | +|----------|------|---------| +| `name` | string | `"Administrator"` | +| `description` | string | The role's documentation in Project Security; `""` when none | +| `is_anonymous` | bool | True if this is the anonymous/guest role | +| `module_roles` | list of string | `["Sales.Admin", "HR.Viewer"]` | +| `check_security` | bool | Studio Pro's per-role "Check security" flag | +| `manage_all_roles` | bool | The role may hand out every user role | +| `manage_users_without_roles` | bool | The role may manage users that have no role | +| `manageable_roles` | list of string | The user roles this role may hand out, when not all | + +### module_role +| Property | Type | Example | +|----------|------|---------| +| `name` | string | `"Sales.Admin"` — qualified module role name | +| `module_name` | string | `"Sales"` | +| `description` | string | Module role description | + +### role_mapping +| Property | Type | Example | +|----------|------|---------| +| `user_role_name` | string | `"Administrator"` | +| `module_role_name` | string | `"Sales.Admin"` | +| `module_name` | string | `"Sales"` | + +### reference +| Property | Type | Example | +|----------|------|---------| +| `source_type` | string | The document the edge comes FROM, upper-case: `"MICROFLOW"`, `"NANOFLOW"`, `"RULE"`, `"PAGE"`, `"SNIPPET"`, `"ENTITY"`, `"ASSOCIATION"`, `"WORKFLOW"`, `"NAVIGATION"`, `"SCHEDULED_EVENT"`, `"PUBLISHED_REST_OPERATION"`, `"PUBLISHED_REST_SERVICE"`, `"PUBLISHED_ODATA_SERVICE"`, `"CONSUMED_ODATA_SERVICE"`, `"BUSINESS_EVENT_SERVICE"`, `"PROJECT_SETTINGS"`, `"IMPORT_MAPPING"`, `"EXPORT_MAPPING"`, `"MESSAGE_DEFINITION_COLLECTION"`, `"MESSAGE_DEFINITION"` | +| `source_id` | string | Source UUID | +| `source_name` | string | `"Sales.ACT_Customer_Create"` | +| `target_type` | string | What it points AT, upper-case: `"ENTITY"`, `"ASSOCIATION"`, `"MICROFLOW"`, `"NANOFLOW"`, `"RULE"`, `"PAGE"`, `"LAYOUT"`, `"WORKFLOW"`, `"WIDGET"`, `"JAVA_ACTION"`, `"JAVASCRIPT_ACTION"`, `"REST_OPERATION"`, `"REGULAR_EXPRESSION"`, `"ATTRIBUTE"`, `"ENUMERATION"`, `"ENUMERATION_VALUE"`. `LAYOUT`, `WIDGET`, `ATTRIBUTE`, `ENUMERATION` and `ENUMERATION_VALUE` are only ever targets; `SCHEDULED_EVENT`, `PROJECT_SETTINGS` and the service types only ever sources | +| `target_id` | string | Target UUID | +| `target_name` | string | `"Sales.Customer"`; three-part for an attribute or an enumeration value: `"Sales.Order.Total"`, `"Sales.OrderStatus.Open"` | +| `ref_kind` | string | How it references: `"call"`, `"create"`, `"retrieve"`, `"change"`, `"delete"`, `"commit"` (a commit action, or a create/change that commits — beside its `"create"`/`"change"` edge; a commit of a variable whose entity the flow cannot tell has no edge), `"show_page"`, `"datasource"`, `"action"`, `"layout"`, `"parameter"`, `"return"`, `"generalize"`, `"associate"`, `"home_page"`, `"login_page"`, `"menu_item"`, `"calculate"`, `"schedule"`, `"validate"`, `"settings"`, `"widget"`, `"sync"`, `"publish"` (a published REST operation, OData resource or action, or a service's authentication microflow), `"event"` (an entity event or a business event handler), `"member"` (binds/reads/writes an attribute or navigates an association), `"xpath"` (an XPath constraint names it), `"type"` (typed as an enumeration), `"value"` (an expression names an enumeration value), `"mapping"` (an import/export mapping maps the entity, or a message definition exposes it), `"split"` (a decision branches on an object being the entity) — lower-case, unlike the types above. Attribute names used only through a variable in a free-text expression (`$Order/Total`) have no edge | +| `module_name` | string | Source module | + +### project_security + +Returned by `project_security()`. Returns `none` if no MPR reader is available. + +| Property | Type | Description | +|----------|------|-------------| +| `security_level` | string | `"CheckNothing"` (Off), `"CheckFormsAndMicroflows"` (Prototype), `"CheckEverything"` (Production) | +| `enable_demo_users` | bool | Whether demo users are enabled | +| `enable_guest_access` | bool | Whether anonymous/guest access is enabled | +| `check_security` | bool | Whether security checking is active | +| `strict_mode` | bool | Strict security mode | +| `anonymous_user_role` | string | Name of the project's guest user role, the role anonymous users get. Read `enable_guest_access` too: the role name can stay set while guest access is off | +| `admin_user_name` | string | Name of the administrator account: `"MxAdmin"`. Its password is deliberately not exposed | +| `admin_user_role` | string | The administrator account's user role: `"Administrator"` | +| `password_policy` | struct | Nested password policy settings | + +#### password_policy (nested in project_security) +| Property | Type | Description | +|----------|------|-------------| +| `min_length` | int | Minimum password length | +| `require_digit` | bool | Must contain a digit | +| `require_mixed_case` | bool | Must contain upper and lower case | +| `require_symbol` | bool | Must contain a symbol | + +### language + +Returned by `languages()`, one per language enabled in the project settings. A per-language check skips `strings()` rows whose `language` is not among these codes. + +| Property | Type | Description | +|----------|------|-------------| +| `code` | string | Language code: `"en_US"`, `"nl_NL"` | +| `is_default` | bool | Whether this is the project's default language | +| `check_completeness` | bool | Whether Studio Pro checks this language's translations for completeness | + +### project_settings + +Returned by `project_settings()`. A setting the project does not store reads as `""`, `0` or `false`. + +| Property | Type | Description | +|----------|------|-------------| +| `default_language` | string | Default language code: `"en_US"` | +| `use_oql_version_2` | bool | Runtime → *Use OQL version 2* | +| `java_version` | string | `"Java21"` | +| `hash_algorithm` | string | `"BCrypt"`, `"SSHA256"`, … | +| `default_time_zone` | string | `"Europe/Amsterdam"`; `""` when unset | +| `rounding_mode` | string | `"HalfUp"`, `"HalfEven"` | +| `decimal_scale` | int | Scale of Decimal attributes | +| `optimistic_locking` | bool | Runtime → *Enable optimistic locking* | +| `foreign_key_constraints` | bool | Runtime → *Use database foreign key constraints* | +| `web_client` | string | Web UI → client: `"Yes"` (React), `"No"` (classic), `"MigrationMode"` | diff --git a/CHANGELOG.md b/CHANGELOG.md index aaf3dfdc3a..fcd52a50e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Added +- **Lint rules can read the data guideline rules asked for** (ako/mxcli#1453, ako/mxcli#1301) — a microflow's `excluded` and `apply_entity_access`, an enumeration's `values` with their captions (so a rule can resolve `getCaption(Module.Enum.Value)`), `constant_values()` for the run-configuration values, and `project_settings()` (default language, OQL version, Java version, hash algorithm, time zone, rounding, decimal scale, optimistic locking, foreign keys, web client). The catalog schema moves to version 30, so a cached catalog is rebuilt once. - **Optional page parameters** (mendixlabs/mxcli#1640, ako/mxcli#1649) — `Params: ( $Sale: Shop.Sale optional )` is Studio Pro's unticked *Required* (`Forms$PageParameter.IsRequired = false`, Mendix 11.5+): the page can be opened with no argument, from a menu item for example, where a required parameter is CE1571. `create page`, `alter page … add parameters` and `describe page` all spell it. `describe navigation` no longer invents `(Sale = $currentObject)` for a menu item opening such a page, which made its output one `exec` refused (MDL-PAGEARG01) and left the navigation profile unchangeable from MDL. Below 11.5 `optional` is refused (MDL-VERSION01), and a snippet parameter cannot be optional (MDL087). ### Changed diff --git a/mdl/catalog/builder_microflows.go b/mdl/catalog/builder_microflows.go index 1e19493d33..9acaab08d2 100644 --- a/mdl/catalog/builder_microflows.go +++ b/mdl/catalog/builder_microflows.go @@ -51,8 +51,8 @@ func (b *Builder) buildMicroflows() error { mfStmt, err := b.tx.Prepare(` INSERT INTO microflows_data (Id, Name, QualifiedName, ModuleName, Folder, MicroflowType, Description, ReturnType, ParameterCount, ActivityCount, TotalActivityCount, - Complexity, Excluded, ProjectId, SnapshotId) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + Complexity, Excluded, ApplyEntityAccess, ProjectId, SnapshotId) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) `) if err != nil { return err @@ -170,6 +170,7 @@ func (b *Builder) buildMicroflows() error { countFlowActivities(mf.ObjectCollection, true), complexity, mf.Excluded, + mf.ApplyEntityAccess, projectID, snapshotID, ) if err != nil { @@ -227,6 +228,7 @@ func (b *Builder) buildMicroflows() error { countFlowActivities(nf.ObjectCollection, true), complexity, nf.Excluded, + false, // a nanoflow runs client-side: entity access always applies, there is no setting projectID, snapshotID, ) if err != nil { @@ -281,6 +283,7 @@ func (b *Builder) buildMicroflows() error { countFlowActivities(rule.ObjectCollection, true), calculateRuleComplexity(rule), rule.Excluded, + rule.ApplyEntityAccess, projectID, snapshotID, ) if err != nil { diff --git a/mdl/catalog/tables.go b/mdl/catalog/tables.go index 4d2c3aeed6..c4e9f8f6df 100644 --- a/mdl/catalog/tables.go +++ b/mdl/catalog/tables.go @@ -7,6 +7,14 @@ package catalog // // History: // +// 30 (flow settings, message definition edges): microflows_data gains +// ApplyEntityAccess (ako/mxcli#1453), which the linter now selects with +// Excluded — against a cached catalog without the column every +// Microflows() query fails and every flow rule reports nothing. Also +// covers refs gaining the edges from message definitions to the +// entities, attributes and associations they expose (ako/mxcli#1781, +// which shipped without a bump): a cached catalog kept answering "no +// references" for them. // 29 (service microflow edges): refs gains the edges from an integration // service to the microflows it runs — a published OData resource's // read/change microflows, its actions and authentication microflow, a @@ -156,7 +164,7 @@ package catalog // SnapshotSource / SourceId / SourceBranch / SourceRevision columns // from every row (issue #576). // 1 — initial flat schema with denormalized snapshot columns on every row. -const CatalogSchemaVersion = "29" +const CatalogSchemaVersion = "30" // MetaSchemaVersion is the catalog_meta key that records the schema version // the cache was built against. @@ -344,6 +352,9 @@ func (c *Catalog) createTables() error { TotalActivityCount INTEGER DEFAULT 0, Complexity INTEGER DEFAULT 1, Excluded BOOLEAN DEFAULT 0, + -- The flow's "Apply entity access" setting; always 0 for a nanoflow, + -- which has no such setting (ako/mxcli#1453). + ApplyEntityAccess BOOLEAN DEFAULT 0, ProjectId TEXT, SnapshotId TEXT )`, diff --git a/mdl/linter/context.go b/mdl/linter/context.go index aa9ccd6c83..b9a566d2ab 100644 --- a/mdl/linter/context.go +++ b/mdl/linter/context.go @@ -621,6 +621,12 @@ type Microflow struct { // at any depth (mendixlabs/mxcli#1266). TotalActivityCount int Complexity int // McCabe cyclomatic complexity + // Excluded is "Exclude from project": the flow is not built or run, so a + // guideline rule usually skips it (ako/mxcli#1453). + Excluded bool + // ApplyEntityAccess is the flow's "Apply entity access" setting. Always + // false for a nanoflow, which has no such setting. + ApplyEntityAccess bool } // DocumentNoun is what to call this document in a lint message and in @@ -656,7 +662,8 @@ func (ctx *LintContext) Microflows() iter.Seq[Microflow] { rows, err := ctx.db.Query(fmt.Sprintf(` SELECT mf.Id, mf.Name, mf.QualifiedName, mf.ModuleName, mf.Folder, mf.MicroflowType, mf.Description, mf.ReturnType, - mf.ParameterCount, mf.ActivityCount, mf.TotalActivityCount, mf.Complexity + mf.ParameterCount, mf.ActivityCount, mf.TotalActivityCount, mf.Complexity, + COALESCE(mf.Excluded, 0), COALESCE(mf.ApplyEntityAccess, 0) FROM microflows mf LEFT JOIN modules m ON mf.ModuleName = m.Name WHERE %s AND %s @@ -672,7 +679,8 @@ func (ctx *LintContext) Microflows() iter.Seq[Microflow] { var mf Microflow var desc, retType, folder sql.NullString err := rows.Scan(&mf.ID, &mf.Name, &mf.QualifiedName, &mf.ModuleName, &folder, - &mf.MicroflowType, &desc, &retType, &mf.ParameterCount, &mf.ActivityCount, &mf.TotalActivityCount, &mf.Complexity) + &mf.MicroflowType, &desc, &retType, &mf.ParameterCount, &mf.ActivityCount, &mf.TotalActivityCount, &mf.Complexity, + &mf.Excluded, &mf.ApplyEntityAccess) if err != nil { ctx.recordQueryError("Microflows (row scan)", err) continue @@ -758,11 +766,51 @@ type Enumeration struct { Folder string Description string ValueCount int + // Values in ordinal order. Without them a rule could not resolve + // getCaption(Module.Enum.Value) (mendixlabs/mxcli#1310, ako/mxcli#1453). + Values []EnumerationValue +} + +// EnumerationValue is one value of an enumeration, with its caption in the +// project's default language. +type EnumerationValue struct { + Name string + QualifiedName string // Module.Enumeration.Value, as refs_from() names it + Caption string + Ordinal int +} + +// enumerationValues reads every enumeration's values in one query, keyed by +// the enumeration's qualified name. Read before Enumerations iterates, so no +// second query runs while its rows are open. +func (ctx *LintContext) enumerationValues() map[string][]EnumerationValue { + out := map[string][]EnumerationValue{} + rows, err := ctx.db.Query(` + SELECT EnumerationQualifiedName, Name, COALESCE(Caption, ''), COALESCE(Ordinal, 0) + FROM enumeration_values + ORDER BY EnumerationQualifiedName, Ordinal, Name`) + if err != nil { + ctx.recordQueryError("EnumerationValues", err) + return out + } + defer rows.Close() + for rows.Next() { + var enum string + var v EnumerationValue + if err := rows.Scan(&enum, &v.Name, &v.Caption, &v.Ordinal); err != nil { + ctx.recordQueryError("EnumerationValues (row scan)", err) + continue + } + v.QualifiedName = enum + "." + v.Name + out[enum] = append(out[enum], v) + } + return out } // Enumerations returns an iterator over all enumerations (excluding system modules). func (ctx *LintContext) Enumerations() iter.Seq[Enumeration] { return func(yield func(Enumeration) bool) { + values := ctx.enumerationValues() rows, err := ctx.db.Query(fmt.Sprintf(` SELECT en.Id, en.Name, en.QualifiedName, en.ModuleName, en.Folder, en.Description, en.ValueCount @@ -788,6 +836,7 @@ func (ctx *LintContext) Enumerations() iter.Seq[Enumeration] { } en.Folder = folder.String en.Description = desc.String + en.Values = values[en.QualifiedName] if ctx.IsExcluded(en.ModuleName) { continue diff --git a/mdl/linter/context_catalog_tables.go b/mdl/linter/context_catalog_tables.go index 54d3a331b2..e7279540fc 100644 --- a/mdl/linter/context_catalog_tables.go +++ b/mdl/linter/context_catalog_tables.go @@ -6,6 +6,7 @@ import ( "database/sql" "fmt" "iter" + "strings" ) // Typed iterators over catalog tables that existed before any Starlark builtin @@ -449,3 +450,46 @@ func (ctx *LintContext) Modules() iter.Seq[Module] { } } } + +// ConstantValue is a constant's value in one run configuration +// (Settings → Configurations), which overrides the constant's default there. +type ConstantValue struct { + Constant string // Module.Constant + Configuration string + Value string +} + +// ConstantValues iterates the constant values the run configurations set, for +// constants outside System and the Marketplace (ako/mxcli#1453). A +// configuration that does not override a constant has no row: it runs on the +// constant's default_value. +func (ctx *LintContext) ConstantValues() iter.Seq[ConstantValue] { + return func(yield func(ConstantValue) bool) { + rows, err := ctx.db.Query(fmt.Sprintf(` + SELECT cv.ConstantName, cv.ConfigurationName, COALESCE(cv.Value, '') + FROM constant_values cv + LEFT JOIN modules m ON substr(cv.ConstantName, 1, instr(cv.ConstantName, '.') - 1) = m.Name + WHERE %s + ORDER BY cv.ConstantName, cv.ConfigurationName + `, notPlatformModule("m"))) + if err != nil { + ctx.recordQueryError("ConstantValues", err) + return + } + defer rows.Close() + + for rows.Next() { + var c ConstantValue + if err := rows.Scan(&c.Constant, &c.Configuration, &c.Value); err != nil { + ctx.recordQueryError("ConstantValues (row scan)", err) + continue + } + if module, _, ok := strings.Cut(c.Constant, "."); ok && ctx.IsExcluded(module) { + continue + } + if !yield(c) { + return + } + } + } +} diff --git a/mdl/linter/context_document_filter_test.go b/mdl/linter/context_document_filter_test.go index 48543b2247..63027a64e3 100644 --- a/mdl/linter/context_document_filter_test.go +++ b/mdl/linter/context_document_filter_test.go @@ -31,7 +31,7 @@ import ( func twoInOneModuleDB(t *testing.T) catalog.CatalogDB { t.Helper() db := setupModuleFilterDB(t) - if _, err := db.Exec(`INSERT INTO microflows VALUES (?, ?, ?, ?, '', 'Microflow', '', '', 0, 0, 0, 0)`, + if _, err := db.Exec(`INSERT INTO microflows (Id, Name, QualifiedName, ModuleName, Folder, MicroflowType, Description, ReturnType, ParameterCount, ActivityCount, TotalActivityCount, Complexity) VALUES (?, ?, ?, ?, '', 'Microflow', '', '', 0, 0, 0, 0)`, "ModB_mf2", "ModB_Sibling", "ModB.Sibling", "ModB"); err != nil { t.Fatalf("insert sibling microflow: %v", err) } diff --git a/mdl/linter/context_test.go b/mdl/linter/context_test.go index 72ea32cf65..fae8b945f1 100644 --- a/mdl/linter/context_test.go +++ b/mdl/linter/context_test.go @@ -44,7 +44,8 @@ func setupModuleFilterDB(t *testing.T) catalog.CatalogDB { _, err = db.Exec(`CREATE TABLE microflows ( Id TEXT, Name TEXT, QualifiedName TEXT, ModuleName TEXT, Folder TEXT, MicroflowType TEXT, Description TEXT, ReturnType TEXT, - ParameterCount INTEGER, ActivityCount INTEGER, TotalActivityCount INTEGER DEFAULT 0, Complexity INTEGER + ParameterCount INTEGER, ActivityCount INTEGER, TotalActivityCount INTEGER DEFAULT 0, Complexity INTEGER, + Excluded INTEGER DEFAULT 0, ApplyEntityAccess INTEGER DEFAULT 0 )`) if err != nil { t.Fatalf("create microflows table: %v", err) @@ -63,7 +64,7 @@ func setupModuleFilterDB(t *testing.T) catalog.CatalogDB { mod+"_e", mod+"_Entity", mod+".Entity", mod); err != nil { t.Fatalf("insert entity for %s: %v", mod, err) } - if _, err := db.Exec(`INSERT INTO microflows VALUES (?, ?, ?, ?, '', 'Microflow', '', '', 0, 0, 0, 0)`, + if _, err := db.Exec(`INSERT INTO microflows (Id, Name, QualifiedName, ModuleName, Folder, MicroflowType, Description, ReturnType, ParameterCount, ActivityCount, TotalActivityCount, Complexity) VALUES (?, ?, ?, ?, '', 'Microflow', '', '', 0, 0, 0, 0)`, mod+"_mf", mod+"_Flow", mod+".Flow", mod); err != nil { t.Fatalf("insert microflow for %s: %v", mod, err) } diff --git a/mdl/linter/rules/empty_test.go b/mdl/linter/rules/empty_test.go index b19960af61..e51fcd96a7 100644 --- a/mdl/linter/rules/empty_test.go +++ b/mdl/linter/rules/empty_test.go @@ -33,7 +33,8 @@ func setupMicroflowsDB(t *testing.T, rows [][]any) catalog.CatalogDB { _, err = db.Exec(`CREATE TABLE microflows ( Id TEXT, Name TEXT, QualifiedName TEXT, ModuleName TEXT, Folder TEXT, MicroflowType TEXT, Description TEXT, ReturnType TEXT, - ParameterCount INTEGER, ActivityCount INTEGER, TotalActivityCount INTEGER DEFAULT 0, Complexity INTEGER + ParameterCount INTEGER, ActivityCount INTEGER, TotalActivityCount INTEGER DEFAULT 0, Complexity INTEGER, + Excluded INTEGER DEFAULT 0, ApplyEntityAccess INTEGER DEFAULT 0 )`) if err != nil { t.Fatalf("failed to create microflows table: %v", err) diff --git a/mdl/linter/starlark.go b/mdl/linter/starlark.go index 3d3522ec28..094fc1862f 100644 --- a/mdl/linter/starlark.go +++ b/mdl/linter/starlark.go @@ -408,6 +408,7 @@ func (r *StarlarkRule) buildPredeclared() starlark.StringDict { "pages": starlark.NewBuiltin("pages", r.builtinPages), "enumerations": starlark.NewBuiltin("enumerations", r.builtinEnumerations), "constants": starlark.NewBuiltin("constants", r.builtinConstants), + "constant_values": starlark.NewBuiltin("constant_values", r.builtinConstantValues), "widgets": starlark.NewBuiltin("widgets", r.builtinWidgets), "refs_to": starlark.NewBuiltin("refs_to", r.builtinRefsTo), "refs_from": starlark.NewBuiltin("refs_from", r.builtinRefsFrom), @@ -448,6 +449,7 @@ func (r *StarlarkRule) buildPredeclared() starlark.StringDict { "role_mappings": starlark.NewBuiltin("role_mappings", r.builtinRoleMappings), "project_security": starlark.NewBuiltin("project_security", r.builtinProjectSecurity), "languages": starlark.NewBuiltin("languages", r.builtinLanguages), + "project_settings": starlark.NewBuiltin("project_settings", r.builtinProjectSettings), // XPath / expression analysis "xpath_expressions": starlark.NewBuiltin("xpath_expressions", r.builtinXPathExpressions), @@ -638,6 +640,79 @@ func (r *StarlarkRule) builtinConstants(_ *starlark.Thread, _ *starlark.Builtin, return starlark.NewList(constants), nil } +// builtinConstantValues returns the constant values the run configurations set +// (ako/mxcli#1453). A configuration that does not override a constant has no +// entry: it runs on the constant's default_value. +func (r *StarlarkRule) builtinConstantValues(_ *starlark.Thread, b *starlark.Builtin, args starlark.Tuple, kwargs []starlark.Tuple) (starlark.Value, error) { + if err := starlark.UnpackArgs(b.Name(), args, kwargs); err != nil { + return nil, err + } + if r.ctx == nil { + return starlark.NewList(nil), nil + } + var out []starlark.Value + for c := range r.ctx.ConstantValues() { + out = append(out, starlarkstruct.FromStringDict(starlark.String("constant_value"), starlark.StringDict{ + "constant": starlark.String(c.Constant), + "configuration": starlark.String(c.Configuration), + "value": starlark.String(c.Value), + })) + } + return starlark.NewList(out), nil +} + +// builtinProjectSettings returns the project-wide settings a guideline rule +// checks (ako/mxcli#1453), or None when no project is open. Like languages(), +// it reads the settings from the project rather than a catalog table, so it +// needs no particular catalog depth; a failed read fails the rule rather than +// answering defaults a rule would report as the project's. +func (r *StarlarkRule) builtinProjectSettings(_ *starlark.Thread, b *starlark.Builtin, args starlark.Tuple, kwargs []starlark.Tuple) (starlark.Value, error) { + if err := starlark.UnpackArgs(b.Name(), args, kwargs); err != nil { + return nil, err + } + if r.ctx == nil || r.ctx.Reader() == nil { + return starlark.None, nil + } + ps, err := r.ctx.Reader().GetProjectSettings() + if err != nil { + return nil, fmt.Errorf("%s: reading project settings: %w", b.Name(), err) + } + if ps == nil { + return starlark.None, nil + } + d := starlark.StringDict{ + "default_language": starlark.String(""), + "use_oql_version_2": starlark.Bool(false), + "java_version": starlark.String(""), + "hash_algorithm": starlark.String(""), + "default_time_zone": starlark.String(""), + "rounding_mode": starlark.String(""), + "decimal_scale": starlark.MakeInt(0), + "optimistic_locking": starlark.Bool(false), + "foreign_key_constraints": starlark.Bool(false), + "web_client": starlark.String(""), + } + if m := ps.Model; m != nil { + d["use_oql_version_2"] = starlark.Bool(m.UseOQLVersion2) + d["java_version"] = starlark.String(m.JavaVersion) + d["hash_algorithm"] = starlark.String(m.HashAlgorithm) + d["default_time_zone"] = starlark.String(m.DefaultTimeZoneCode) + d["rounding_mode"] = starlark.String(m.RoundingMode) + d["decimal_scale"] = starlark.MakeInt(m.DecimalScale) + d["optimistic_locking"] = starlark.Bool(m.EnableDataStorageOptimisticLocking) + d["foreign_key_constraints"] = starlark.Bool(m.UseDatabaseForeignKeyConstraints) + } + if ps.Language != nil { + d["default_language"] = starlark.String(ps.Language.DefaultLanguageCode) + } + if ps.WebUI != nil { + // Web UI → UseOptimizedClient: "Yes" is the React client, "No" the + // classic one, "MigrationMode" Mendix's mixed mode. + d["web_client"] = starlark.String(ps.WebUI.UseOptimizedClient) + } + return starlarkstruct.FromStringDict(starlark.String("project_settings"), d), nil +} + // builtinWidgets returns an iterator over widgets. func (r *StarlarkRule) builtinWidgets(_ *starlark.Thread, _ *starlark.Builtin, args starlark.Tuple, kwargs []starlark.Tuple) (starlark.Value, error) { if r.ctx == nil { @@ -1120,6 +1195,10 @@ func microflowToStarlark(mf Microflow) starlark.Value { // Title case matches the document_type spelling Starlark rules use. "document_noun": starlark.String(mf.DocumentNoun()), "document_noun_title": starlark.String(mf.DocumentNounTitle()), + // "Exclude from project": a guideline rule usually skips such a flow, + // and could not tell it apart (ako/mxcli#1453). + "excluded": starlark.Bool(mf.Excluded), + "apply_entity_access": starlark.Bool(mf.ApplyEntityAccess), }) } @@ -1179,9 +1258,26 @@ func enumerationToStarlark(e Enumeration) starlark.Value { "folder": starlark.String(e.Folder), "description": starlark.String(e.Description), "value_count": starlark.MakeInt(e.ValueCount), + "values": enumerationValuesToStarlark(e.Values), }) } +// enumerationValuesToStarlark lists an enumeration's values in ordinal order, +// each with the qualified name refs_from() uses for it, so a rule can resolve +// getCaption(Module.Enum.Value) (mendixlabs/mxcli#1310). +func enumerationValuesToStarlark(values []EnumerationValue) *starlark.List { + out := make([]starlark.Value, 0, len(values)) + for _, v := range values { + out = append(out, starlarkstruct.FromStringDict(starlark.String("enumeration_value"), starlark.StringDict{ + "name": starlark.String(v.Name), + "qualified_name": starlark.String(v.QualifiedName), + "caption": starlark.String(v.Caption), + "ordinal": starlark.MakeInt(v.Ordinal), + })) + } + return starlark.NewList(out) +} + // constantToStarlark converts a LintConstant to a Starlark struct. func constantToStarlark(c LintConstant) starlark.Value { return starlarkstruct.FromStringDict(starlark.String("constant"), starlark.StringDict{ diff --git a/mdl/linter/starlark_catalog_data_1453_test.go b/mdl/linter/starlark_catalog_data_1453_test.go new file mode 100644 index 0000000000..db8f39432a --- /dev/null +++ b/mdl/linter/starlark_catalog_data_1453_test.go @@ -0,0 +1,109 @@ +// SPDX-License-Identifier: Apache-2.0 + +package linter_test + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/linter" + "github.com/mendixlabs/mxcli/model" +) + +// ako/mxcli#1453 (mendixlabs/mxcli#1556): porting ~90 company guideline rules +// to Starlark, rules could not be written although the catalog or the project +// settings hold the data — rules reported excluded flows, could not resolve an +// enumeration value's caption (#1301), and could not read run-configuration +// constant values or the project settings. + +func runDump(t *testing.T, ctx *linter.LintContext, body string) []string { + t.Helper() + vs, _ := runSrc(t, ctx, "def check():\n"+body) + var got []string + for _, v := range vs { + got = append(got, v.Message) + } + return got +} + +func TestStarlarkMicroflowCarriesExcludedAndApplyEntityAccess(t *testing.T) { + cat := emptyCatalog(t) + for _, stmt := range []string{ + `INSERT INTO modules_data (Id, Name, QualifiedName, ModuleName, Source) VALUES ('m1', 'Shop', 'Shop', 'Shop', '')`, + `INSERT INTO microflows_data (Id, Name, QualifiedName, ModuleName, MicroflowType, Excluded, ApplyEntityAccess) VALUES + ('f1', 'ACT_Live', 'Shop.ACT_Live', 'Shop', 'MICROFLOW', 0, 1), + ('f2', 'ACT_Old', 'Shop.ACT_Old', 'Shop', 'MICROFLOW', 1, 0)`, + } { + if _, err := cat.CatalogDB().Exec(stmt); err != nil { + t.Fatalf("%s: %v", stmt, err) + } + } + got := runDump(t, linter.NewLintContext(cat, nil), + ` return [violation(message = "%s|%s|%s" % (m.name, m.excluded, m.apply_entity_access)) for m in microflows()]`) + want := "ACT_Live|False|True,ACT_Old|True|False" + if strings.Join(got, ",") != want { + t.Errorf("microflows() = %v, want %s", got, want) + } +} + +func TestStarlarkEnumerationCarriesItsValuesAndCaptions(t *testing.T) { + cat := emptyCatalog(t) + for _, stmt := range []string{ + `INSERT INTO modules_data (Id, Name, QualifiedName, ModuleName, Source) VALUES ('m1', 'Shop', 'Shop', 'Shop', '')`, + `INSERT INTO enumerations_data (Id, Name, QualifiedName, ModuleName, ValueCount) VALUES + ('e1', 'ApiTimeout', 'Shop.ApiTimeout', 'Shop', 2), ('e2', 'Empty', 'Shop.Empty', 'Shop', 0)`, + `INSERT INTO enumeration_values_data (Id, EnumerationId, EnumerationQualifiedName, ModuleName, Name, Caption, Ordinal) VALUES + ('v2', 'e1', 'Shop.ApiTimeout', 'Shop', 'Slow', '400', 1), + ('v1', 'e1', 'Shop.ApiTimeout', 'Shop', 'Quick', '30', 0)`, + } { + if _, err := cat.CatalogDB().Exec(stmt); err != nil { + t.Fatalf("%s: %v", stmt, err) + } + } + got := runDump(t, linter.NewLintContext(cat, nil), ` out = [] + for e in enumerations(): + out.append(violation(message = "%s:%s" % (e.name, ",".join(["%s=%s@%d" % (v.qualified_name, v.caption, v.ordinal) for v in e.values])))) + return out`) + // In ordinal order, not insertion order; an enumeration without values has []. + want := "ApiTimeout:Shop.ApiTimeout.Quick=30@0,Shop.ApiTimeout.Slow=400@1|Empty:" + if strings.Join(got, "|") != want { + t.Errorf("enumerations() values = %v, want %s", got, want) + } +} + +func TestStarlarkConstantValuesAndProjectSettings(t *testing.T) { + cat := emptyCatalog(t) + if _, err := cat.CatalogDB().Exec(`INSERT INTO constant_values (ConstantName, ConfigurationName, Value) VALUES + ('Shop.ApiUrl', 'Default', 'https://test'), ('Shop.ApiUrl', 'Prod', 'https://prod')`); err != nil { + t.Fatal(err) + } + reader := &settingsReader{ps: &model.ProjectSettings{ + Model: &model.ModelSettings{UseOQLVersion2: true, JavaVersion: "Java21", HashAlgorithm: "BCrypt", + DefaultTimeZoneCode: "Europe/Amsterdam", RoundingMode: "HalfUp", DecimalScale: 8, + EnableDataStorageOptimisticLocking: true, UseDatabaseForeignKeyConstraints: true}, + Language: &model.LanguageSettings{DefaultLanguageCode: "nl_NL"}, + WebUI: &model.WebUISettings{UseOptimizedClient: "Yes"}, + }} + ctx := linter.NewLintContext(cat, reader) + + got := runDump(t, ctx, + ` return [violation(message = "%s|%s|%s" % (c.constant, c.configuration, c.value)) for c in constant_values()]`) + if want := "Shop.ApiUrl|Default|https://test,Shop.ApiUrl|Prod|https://prod"; strings.Join(got, ",") != want { + t.Errorf("constant_values() = %v, want %s", got, want) + } + + got = runDump(t, ctx, ` s = project_settings() + return [violation(message = "%s|%s|%s|%s|%s|%s|%s|%s|%s|%s" % (s.default_language, s.use_oql_version_2, + s.java_version, s.hash_algorithm, s.default_time_zone, s.rounding_mode, s.decimal_scale, + s.optimistic_locking, s.foreign_key_constraints, s.web_client))]`) + if want := "nl_NL|True|Java21|BCrypt|Europe/Amsterdam|HalfUp|8|True|True|Yes"; strings.Join(got, ",") != want { + t.Errorf("project_settings() = %v, want %s", got, want) + } + + // No reader (a rule run without a project): None, so a rule can test for it. + got = runDump(t, linter.NewLintContext(cat, nil), + ` return [violation(message = "none")] if project_settings() == None else []`) + if strings.Join(got, ",") != "none" { + t.Errorf("project_settings() without a reader = %v, want None", got) + } +} diff --git a/mdl/linter/starlark_catalog_mode_guard_test.go b/mdl/linter/starlark_catalog_mode_guard_test.go index edc26c2c5c..0e2f918cc8 100644 --- a/mdl/linter/starlark_catalog_mode_guard_test.go +++ b/mdl/linter/starlark_catalog_mode_guard_test.go @@ -46,6 +46,7 @@ var builtinModes = map[string]builtinProbe{ "pages": {CatalogFast, "pages()"}, "enumerations": {CatalogFast, "enumerations()"}, "constants": {CatalogFast, "constants()"}, + "constant_values": {CatalogFast, "constant_values()"}, "snippets": {CatalogFast, "snippets()"}, "scheduled_events": {CatalogFast, "scheduled_events()"}, "queues": {CatalogFast, "queues()"}, @@ -57,6 +58,7 @@ var builtinModes = map[string]builtinProbe{ "role_mappings": {CatalogFast, "role_mappings()"}, "project_security": {CatalogFast, "[project_security()]"}, "languages": {CatalogFast, "languages()"}, + "project_settings": {CatalogFast, "[project_settings()]"}, "attributes_for": {CatalogFast, "[a for n in ENTITY_NAMES for a in attributes_for(n)]"}, "modules": {CatalogFast, "modules()"}, "associations": {CatalogFast, "associations()"}, diff --git a/mdl/linter/starlark_documented_values_test.go b/mdl/linter/starlark_documented_values_test.go index 4a3ddd8a7a..a1665742be 100644 --- a/mdl/linter/starlark_documented_values_test.go +++ b/mdl/linter/starlark_documented_values_test.go @@ -118,8 +118,9 @@ func everyKindFixtureDB(t *testing.T) catalog.CatalogDB { `CREATE TABLE microflows ( Id TEXT, Name TEXT, QualifiedName TEXT, ModuleName TEXT, Folder TEXT, MicroflowType TEXT, Description TEXT, ReturnType TEXT, - ParameterCount INTEGER, ActivityCount INTEGER, TotalActivityCount INTEGER DEFAULT 0, Complexity INTEGER)`, - `INSERT INTO microflows VALUES + ParameterCount INTEGER, ActivityCount INTEGER, TotalActivityCount INTEGER DEFAULT 0, Complexity INTEGER, + Excluded INTEGER DEFAULT 0, ApplyEntityAccess INTEGER DEFAULT 0)`, + `INSERT INTO microflows (Id, Name, QualifiedName, ModuleName, Folder, MicroflowType, Description, ReturnType, ParameterCount, ActivityCount, TotalActivityCount, Complexity) VALUES ('f1','MF','Sales.MF','Sales','','MICROFLOW','','',0,0,0,1), ('f2','NF','Sales.NF','Sales','','NANOFLOW','','',0,0,0,1), ('f3','RU','Sales.RU','Sales','','RULE','','',0,0,0,1)`, diff --git a/mdl/linter/starlark_flow_noun_test.go b/mdl/linter/starlark_flow_noun_test.go index 6a6ff2ba9a..5a1bc9576f 100644 --- a/mdl/linter/starlark_flow_noun_test.go +++ b/mdl/linter/starlark_flow_noun_test.go @@ -115,8 +115,9 @@ func flowKindsFixtureDB(t *testing.T) catalog.CatalogDB { `CREATE TABLE microflows ( Id TEXT, Name TEXT, QualifiedName TEXT, ModuleName TEXT, Folder TEXT, MicroflowType TEXT, Description TEXT, ReturnType TEXT, - ParameterCount INTEGER, ActivityCount INTEGER, TotalActivityCount INTEGER DEFAULT 0, Complexity INTEGER)`, - `INSERT INTO microflows VALUES + ParameterCount INTEGER, ActivityCount INTEGER, TotalActivityCount INTEGER DEFAULT 0, Complexity INTEGER, + Excluded INTEGER DEFAULT 0, ApplyEntityAccess INTEGER DEFAULT 0)`, + `INSERT INTO microflows (Id, Name, QualifiedName, ModuleName, Folder, MicroflowType, Description, ReturnType, ParameterCount, ActivityCount, TotalActivityCount, Complexity) VALUES ('f1','Big_MF','Sales.Big_MF','Sales','','MICROFLOW','','',0,100,100,50), ('f2','Big_NF','Sales.Big_NF','Sales','','NANOFLOW','','',0,100,100,50), ('f3','Big_RU','Sales.Big_RU','Sales','','RULE','','',0,100,100,50), diff --git a/mdl/linter/starlark_qual004_refs_test.go b/mdl/linter/starlark_qual004_refs_test.go index 55f9d3672f..bcdaa95314 100644 --- a/mdl/linter/starlark_qual004_refs_test.go +++ b/mdl/linter/starlark_qual004_refs_test.go @@ -41,8 +41,9 @@ func qual004FixtureDB(t *testing.T, refs []qual004Ref) catalog.CatalogDB { `CREATE TABLE microflows ( Id TEXT, Name TEXT, QualifiedName TEXT, ModuleName TEXT, Folder TEXT, MicroflowType TEXT, Description TEXT, ReturnType TEXT, - ParameterCount INTEGER, ActivityCount INTEGER, TotalActivityCount INTEGER DEFAULT 0, Complexity INTEGER)`, - `INSERT INTO microflows VALUES + ParameterCount INTEGER, ActivityCount INTEGER, TotalActivityCount INTEGER DEFAULT 0, Complexity INTEGER, + Excluded INTEGER DEFAULT 0, ApplyEntityAccess INTEGER DEFAULT 0)`, + `INSERT INTO microflows (Id, Name, QualifiedName, ModuleName, Folder, MicroflowType, Description, ReturnType, ParameterCount, ActivityCount, TotalActivityCount, Complexity) VALUES ('f1','NAV_Home','M.NAV_Home','M','','MICROFLOW','','',0,1,1,1), ('f2','NAV_Orders','M.NAV_Orders','M','','MICROFLOW','','',0,1,1,1), ('f3','Unused','M.Unused','M','','MICROFLOW','','',0,1,1,1), From b00bc28a8d0279675fb73e960eb4ad9eab08255e Mon Sep 17 00:00:00 2001 From: Ako Date: Sat, 31 Oct 2026 21:08:08 +0000 Subject: [PATCH 02/10] feat(lint): permission rows name the access rule they come from (#1437) permissions() and permissions_for() returned one row per module role and right with no id of the entity access rule behind it, so a rule could not count an entity's access rules per setting or list one rule's module roles exactly; grouping on XPath, default and rights merges two rules that agree on them. Entity and member rows now carry access_rule_id and access_rule_index (the rule's position on the entity); a page, flow or service row has "" and -1. Catalog schema 30, because the linter selects the new columns. The permission struct doc moves to catalog-tables.md to keep SKILL.md under its size limit. Co-Authored-By: Claude Opus 5.5 --- .../skills/mendix/write-lint-rules/SKILL.md | 25 ++---- .../mendix/write-lint-rules/catalog-tables.md | 19 +++++ CHANGELOG.md | 1 + mdl/catalog/builder_permissions.go | 37 ++++++--- .../builder_permissions_rule_id_test.go | 82 +++++++++++++++++++ mdl/catalog/tables.go | 14 +++- mdl/linter/context.go | 22 ++++- mdl/linter/starlark.go | 5 ++ mdl/linter/starlark_conv006_test.go | 5 +- .../starlark_permission_rule_id_1437_test.go | 53 ++++++++++++ mdl/linter/starlark_sec008_test.go | 5 +- 11 files changed, 226 insertions(+), 42 deletions(-) create mode 100644 mdl/catalog/builder_permissions_rule_id_test.go create mode 100644 mdl/linter/starlark_permission_rule_id_1437_test.go diff --git a/.claude/skills/mendix/write-lint-rules/SKILL.md b/.claude/skills/mendix/write-lint-rules/SKILL.md index 67adc624be..d1c161d2d3 100644 --- a/.claude/skills/mendix/write-lint-rules/SKILL.md +++ b/.claude/skills/mendix/write-lint-rules/SKILL.md @@ -12,10 +12,10 @@ description: "Write custom Starlark lint rules in .claude/lint-rules/ that run b |---|---| | 25-67 | Rule File Structure | | 68-161 | Available Query Functions | -| 162-606 | Object Properties | -| 607-620 | Helper Functions | -| 621-707 | Common Patterns | -| 708-716 | Validation | +| 162-591 | Object Properties | +| 592-605 | Helper Functions | +| 606-692 | Common Patterns | +| 693-701 | Validation | # Writing Custom Starlark Lint Rules @@ -515,22 +515,7 @@ def count_not(node): | `timeout` | int | Configured timeout in milliseconds; `0` when none is set | | `module_name` | string | `"Sales"` | -### permission - -Returned by `permissions()` (all types) or `permissions_for()` (entity-specific). - -| Property | Type | Example | -|----------|------|---------| -| `module_role_name` | string | `"Sales.Admin"` — qualified, so it compares directly against `user_role.module_roles` | -| `element_type` | string | `"ENTITY"`, `"MICROFLOW"`, `"NANOFLOW"`, `"PAGE"`, `"ODATA_SERVICE"` (from `permissions()` only) | -| `element_name` | string | `"Sales.Customer"` | -| `module_name` | string | `"Sales"` | -| `entity_name` | string | `"Sales.Customer"` (from `permissions_for()` only) | -| `access_type` | string | `"CREATE"`, `"READ"`, `"WRITE"`, `"DELETE"` (entity), `"EXECUTE"` (microflow, nanoflow), `"VIEW"` (page), `"ACCESS"` (OData service), `"MEMBER_READ"`, `"MEMBER_WRITE"` | -| `member_name` | string | Attribute name (for MEMBER_READ/MEMBER_WRITE) | -| `xpath_constraint` | string | XPath constraint or empty | -| `is_constrained` | bool | True if XPath constraint is set | -| `default_member_access_rights` | string | The rule's "default rights for new members": `"None"`, `"ReadOnly"` or `"ReadWrite"`. Empty for non-entity permissions | +The `permission` struct `permissions()` and `permissions_for()` return is in [catalog-tables.md](catalog-tables.md#permission). ### user_role | Property | Type | Example | diff --git a/.claude/skills/mendix/write-lint-rules/catalog-tables.md b/.claude/skills/mendix/write-lint-rules/catalog-tables.md index 8fbc175f81..3a8dc6ff44 100644 --- a/.claude/skills/mendix/write-lint-rules/catalog-tables.md +++ b/.claude/skills/mendix/write-lint-rules/catalog-tables.md @@ -157,3 +157,22 @@ def check(): location=loc)) return out ``` + +### permission + +Returned by `permissions()` (all types) or `permissions_for()` (entity-specific). + +| Property | Type | Example | +|----------|------|---------| +| `module_role_name` | string | `"Sales.Admin"` — qualified, so it compares directly against `user_role.module_roles` | +| `element_type` | string | `"ENTITY"`, `"MICROFLOW"`, `"NANOFLOW"`, `"PAGE"`, `"ODATA_SERVICE"` (from `permissions()` only) | +| `element_name` | string | `"Sales.Customer"` | +| `module_name` | string | `"Sales"` | +| `entity_name` | string | `"Sales.Customer"` (from `permissions_for()` only) | +| `access_type` | string | `"CREATE"`, `"READ"`, `"WRITE"`, `"DELETE"` (entity), `"EXECUTE"` (microflow, nanoflow), `"VIEW"` (page), `"ACCESS"` (OData service), `"MEMBER_READ"`, `"MEMBER_WRITE"` | +| `member_name` | string | Attribute name (for MEMBER_READ/MEMBER_WRITE) | +| `xpath_constraint` | string | XPath constraint or empty | +| `is_constrained` | bool | True if XPath constraint is set | +| `default_member_access_rights` | string | The rule's "default rights for new members": `"None"`, `"ReadOnly"` or `"ReadWrite"`. Empty for non-entity permissions | +| `access_rule_id` | string | The entity access rule the row comes from: rows sharing it are one rule, so a rule can count an entity's access rules or list one rule's module roles exactly. Empty for a page, flow or service permission | +| `access_rule_index` | int | That access rule's position on the entity, from 0; `-1` when `access_rule_id` is empty | diff --git a/CHANGELOG.md b/CHANGELOG.md index aaf3dfdc3a..f2413ff40a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Added +- **Lint permission rows name their access rule** (ako/mxcli#1437, mendixlabs/mxcli#1539) — `permissions()` and `permissions_for()` rows carry `access_rule_id` and `access_rule_index`, so a rule can count an entity's access rules per setting or list one rule's module roles exactly, instead of guessing by grouping on XPath, default and rights. The catalog schema moves to version 30, so a cached catalog is rebuilt once. - **Optional page parameters** (mendixlabs/mxcli#1640, ako/mxcli#1649) — `Params: ( $Sale: Shop.Sale optional )` is Studio Pro's unticked *Required* (`Forms$PageParameter.IsRequired = false`, Mendix 11.5+): the page can be opened with no argument, from a menu item for example, where a required parameter is CE1571. `create page`, `alter page … add parameters` and `describe page` all spell it. `describe navigation` no longer invents `(Sale = $currentObject)` for a menu item opening such a page, which made its output one `exec` refused (MDL-PAGEARG01) and left the navigation profile unchangeable from MDL. Below 11.5 `optional` is refused (MDL-VERSION01), and a snippet parameter cannot be optional (MDL087). ### Changed diff --git a/mdl/catalog/builder_permissions.go b/mdl/catalog/builder_permissions.go index fcf056c7d0..da7e59e3a7 100644 --- a/mdl/catalog/builder_permissions.go +++ b/mdl/catalog/builder_permissions.go @@ -69,11 +69,23 @@ func (b *Builder) buildPermissions() error { } defer stmt.Close() + // Entity rows also name the access rule they come from, so a rule can + // group them per access rule instead of guessing by XPath and rights + // (mendixlabs/mxcli#1539, ako/mxcli#1437). + ruleStmt, err := b.tx.Prepare(` + INSERT INTO permissions (ModuleRoleName, ElementType, ElementName, MemberName, AccessType, XPathConstraint, DefaultMemberAccessRights, ModuleName, ProjectId, SnapshotId, AccessRuleId, AccessRuleIndex) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + `) + if err != nil { + return err + } + defer ruleStmt.Close() + projectID := b.catalog.projectID snapshotID := b.snapshot.ID permCount := 0 - permCount += b.buildEntityPermissions(stmt, projectID, snapshotID) + permCount += b.buildEntityPermissions(ruleStmt, projectID, snapshotID) permCount += b.buildMicroflowPermissions(stmt, projectID, snapshotID) permCount += b.buildNanoflowPermissions(stmt, projectID, snapshotID) permCount += b.buildPagePermissions(stmt, projectID, snapshotID) @@ -102,7 +114,8 @@ func (b *Builder) buildEntityPermissions(stmt *Stmt, projectID, snapshotID strin for _, ent := range dm.Entities { entityQN := moduleName + "." + ent.Name - for _, rule := range ent.AccessRules { + for ruleIndex, rule := range ent.AccessRules { + ruleID := string(rule.ID) // Each access rule can apply to multiple module roles roleNames := rule.ModuleRoleNames if len(roleNames) == 0 { @@ -125,24 +138,24 @@ func (b *Builder) buildEntityPermissions(stmt *Stmt, projectID, snapshotID strin for _, roleName := range roleNames { // Entity-level permissions if rule.AllowCreate { - stmt.Exec(roleName, PermissionElementEntity, entityQN, nil, AccessTypeCreate, xpath, defaultRights, moduleName, projectID, snapshotID) + stmt.Exec(roleName, PermissionElementEntity, entityQN, nil, AccessTypeCreate, xpath, defaultRights, moduleName, projectID, snapshotID, ruleID, ruleIndex) count++ } if hasRead { - stmt.Exec(roleName, PermissionElementEntity, entityQN, nil, AccessTypeRead, xpath, defaultRights, moduleName, projectID, snapshotID) + stmt.Exec(roleName, PermissionElementEntity, entityQN, nil, AccessTypeRead, xpath, defaultRights, moduleName, projectID, snapshotID, ruleID, ruleIndex) count++ } if hasWrite { - stmt.Exec(roleName, PermissionElementEntity, entityQN, nil, AccessTypeWrite, xpath, defaultRights, moduleName, projectID, snapshotID) + stmt.Exec(roleName, PermissionElementEntity, entityQN, nil, AccessTypeWrite, xpath, defaultRights, moduleName, projectID, snapshotID, ruleID, ruleIndex) count++ } if rule.AllowDelete { - stmt.Exec(roleName, PermissionElementEntity, entityQN, nil, AccessTypeDelete, xpath, defaultRights, moduleName, projectID, snapshotID) + stmt.Exec(roleName, PermissionElementEntity, entityQN, nil, AccessTypeDelete, xpath, defaultRights, moduleName, projectID, snapshotID, ruleID, ruleIndex) count++ } // Member-level permissions - count += b.emitMemberPermissions(stmt, rule, ent, roleName, entityQN, xpath, defaultRights, moduleName, projectID, snapshotID) + count += b.emitMemberPermissions(stmt, rule, ruleID, ruleIndex, ent, roleName, entityQN, xpath, defaultRights, moduleName, projectID, snapshotID) } } } @@ -182,7 +195,7 @@ func entityAccessFromMemberRights(rule *domainmodel.AccessRule) (hasRead, hasWri // emitMemberPermissions emits MEMBER_READ/MEMBER_WRITE rows for entity attributes and associations. // When MemberAccesses is non-empty, use explicit per-member rights. // When MemberAccesses is empty, expand DefaultMemberAccessRights to all attributes. -func (b *Builder) emitMemberPermissions(stmt *Stmt, rule *domainmodel.AccessRule, ent *domainmodel.Entity, +func (b *Builder) emitMemberPermissions(stmt *Stmt, rule *domainmodel.AccessRule, ruleID string, ruleIndex int, ent *domainmodel.Entity, roleName, entityQN, xpath, defaultRights, moduleName, projectID, snapshotID string) int { count := 0 @@ -198,11 +211,11 @@ func (b *Builder) emitMemberPermissions(stmt *Stmt, rule *domainmodel.AccessRule } if ma.AccessRights == domainmodel.MemberAccessRightsReadOnly || ma.AccessRights == domainmodel.MemberAccessRightsReadWrite { - stmt.Exec(roleName, PermissionElementEntity, entityQN, memberName, AccessTypeMemberRead, xpath, defaultRights, moduleName, projectID, snapshotID) + stmt.Exec(roleName, PermissionElementEntity, entityQN, memberName, AccessTypeMemberRead, xpath, defaultRights, moduleName, projectID, snapshotID, ruleID, ruleIndex) count++ } if ma.AccessRights == domainmodel.MemberAccessRightsReadWrite { - stmt.Exec(roleName, PermissionElementEntity, entityQN, memberName, AccessTypeMemberWrite, xpath, defaultRights, moduleName, projectID, snapshotID) + stmt.Exec(roleName, PermissionElementEntity, entityQN, memberName, AccessTypeMemberWrite, xpath, defaultRights, moduleName, projectID, snapshotID, ruleID, ruleIndex) count++ } } @@ -210,11 +223,11 @@ func (b *Builder) emitMemberPermissions(stmt *Stmt, rule *domainmodel.AccessRule // Expand default to all attributes for _, attr := range ent.Attributes { if rule.DefaultMemberAccessRights == domainmodel.MemberAccessRightsReadOnly || rule.DefaultMemberAccessRights == domainmodel.MemberAccessRightsReadWrite { - stmt.Exec(roleName, PermissionElementEntity, entityQN, attr.Name, AccessTypeMemberRead, xpath, defaultRights, moduleName, projectID, snapshotID) + stmt.Exec(roleName, PermissionElementEntity, entityQN, attr.Name, AccessTypeMemberRead, xpath, defaultRights, moduleName, projectID, snapshotID, ruleID, ruleIndex) count++ } if rule.DefaultMemberAccessRights == domainmodel.MemberAccessRightsReadWrite { - stmt.Exec(roleName, PermissionElementEntity, entityQN, attr.Name, AccessTypeMemberWrite, xpath, defaultRights, moduleName, projectID, snapshotID) + stmt.Exec(roleName, PermissionElementEntity, entityQN, attr.Name, AccessTypeMemberWrite, xpath, defaultRights, moduleName, projectID, snapshotID, ruleID, ruleIndex) count++ } } diff --git a/mdl/catalog/builder_permissions_rule_id_test.go b/mdl/catalog/builder_permissions_rule_id_test.go new file mode 100644 index 0000000000..4791c1b92c --- /dev/null +++ b/mdl/catalog/builder_permissions_rule_id_test.go @@ -0,0 +1,82 @@ +// SPDX-License-Identifier: Apache-2.0 + +package catalog + +import ( + "fmt" + "sort" + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/backend/mock" + "github.com/mendixlabs/mxcli/mdl/types" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/domainmodel" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// Every row an entity access rule produces names that rule and its position on +// the entity (ako/mxcli#1437): two rules that agree on XPath, default rights and +// access are still two rules. A microflow grant comes from no access rule. +func TestPermissionRowsCarryTheirAccessRule(t *testing.T) { + const mod = model.ID("mod-t") + rule := func(id string, role string) *domainmodel.AccessRule { + return &domainmodel.AccessRule{BaseElement: model.BaseElement{ID: model.ID(id)}, + ModuleRoleNames: []string{role}, AllowCreate: true, + DefaultMemberAccessRights: domainmodel.MemberAccessRightsReadWrite} + } + be := &mock.MockBackend{ + IsConnectedFunc: func() bool { return true }, + GetProjectSettingsFunc: func() (*model.ProjectSettings, error) { return &model.ProjectSettings{}, nil }, + ListModuleSettingsFunc: func() ([]*types.ModuleSettings, error) { return nil, nil }, + ListModulesFunc: func() ([]*model.Module, error) { + return []*model.Module{{BaseElement: model.BaseElement{ID: mod}, Name: "T"}}, nil + }, + ListDomainModelsFunc: func() ([]*domainmodel.DomainModel, error) { + return []*domainmodel.DomainModel{{BaseElement: model.BaseElement{ID: "dm-1"}, ContainerID: mod, + Entities: []*domainmodel.Entity{{BaseElement: model.BaseElement{ID: "ent-1"}, Name: "Order", + Attributes: []*domainmodel.Attribute{{BaseElement: model.BaseElement{ID: "a-1"}, Name: "Code"}}, + AccessRules: []*domainmodel.AccessRule{rule("rule-a", "T.User"), rule("rule-b", "T.Admin")}}}}}, nil + }, + ListRulesFunc: func() ([]*microflows.Rule, error) { return nil, nil }, + ListNanoflowsFunc: func() ([]*microflows.Nanoflow, error) { return nil, nil }, + GetNavigationFunc: func() (*types.NavigationDocument, error) { return &types.NavigationDocument{}, nil }, + ListMicroflowsFunc: func() ([]*microflows.Microflow, error) { + return []*microflows.Microflow{{BaseElement: model.BaseElement{ID: "mf-1"}, ContainerID: mod, + Name: "ACT_Save", AllowedModuleRoles: []model.ID{"T.User"}}}, nil + }, + } + cat, err := New() + if err != nil { + t.Fatalf("catalog.New: %v", err) + } + t.Cleanup(func() { cat.Close() }) + b := NewBuilder(cat, be) + b.SetFullMode(true) + if err := b.Build(nil); err != nil { + t.Fatalf("catalog build: %v", err) + } + + var got []string + for _, row := range queryRows(t, cat, `SELECT ElementType, ModuleRoleName, AccessType, + COALESCE(AccessRuleId, ''), COALESCE(AccessRuleIndex, -1) FROM permissions`) { + got = append(got, fmt.Sprintf("%v %v %v %v@%v", row...)) + } + sort.Strings(got) + want := []string{ + "ENTITY T.Admin CREATE rule-b@1", + "ENTITY T.Admin MEMBER_READ rule-b@1", + "ENTITY T.Admin MEMBER_WRITE rule-b@1", + "ENTITY T.Admin READ rule-b@1", + "ENTITY T.Admin WRITE rule-b@1", + "ENTITY T.User CREATE rule-a@0", + "ENTITY T.User MEMBER_READ rule-a@0", + "ENTITY T.User MEMBER_WRITE rule-a@0", + "ENTITY T.User READ rule-a@0", + "ENTITY T.User WRITE rule-a@0", + "MICROFLOW T.User EXECUTE @-1", + } + if strings.Join(got, "\n") != strings.Join(want, "\n") { + t.Errorf("permission rows:\n%s\nwant:\n%s", strings.Join(got, "\n"), strings.Join(want, "\n")) + } +} diff --git a/mdl/catalog/tables.go b/mdl/catalog/tables.go index 4d2c3aeed6..1786aa10a5 100644 --- a/mdl/catalog/tables.go +++ b/mdl/catalog/tables.go @@ -7,6 +7,11 @@ package catalog // // History: // +// 30 (access rule id): permissions gains AccessRuleId and AccessRuleIndex, +// which the linter selects — against a cached catalog without them every +// permission query fails and the security rules report nothing +// (ako/mxcli#1437). Also covers refs gaining the edges from message +// definitions (ako/mxcli#1781, which shipped without a bump). // 29 (service microflow edges): refs gains the edges from an integration // service to the microflows it runs — a published OData resource's // read/change microflows, its actions and authentication microflow, a @@ -156,7 +161,7 @@ package catalog // SnapshotSource / SourceId / SourceBranch / SourceRevision columns // from every row (issue #576). // 1 — initial flat schema with denormalized snapshot columns on every row. -const CatalogSchemaVersion = "29" +const CatalogSchemaVersion = "30" // MetaSchemaVersion is the catalog_meta key that records the schema version // the cache was built against. @@ -1249,7 +1254,12 @@ func (c *Catalog) createTables() error { DefaultMemberAccessRights TEXT, ModuleName TEXT, ProjectId TEXT, - SnapshotId TEXT + SnapshotId TEXT, + -- The entity access rule a row comes from, and that rule's position + -- on the entity (from 0). Empty / NULL for a page, flow or service + -- row, which no access rule produces (ako/mxcli#1437). + AccessRuleId TEXT, + AccessRuleIndex INTEGER )`, // constants — partial denormalization diff --git a/mdl/linter/context.go b/mdl/linter/context.go index aa9ccd6c83..21d9cb796e 100644 --- a/mdl/linter/context.go +++ b/mdl/linter/context.go @@ -392,6 +392,12 @@ type Permission struct { // DefaultMemberAccessRights is the rule's "default rights for new members" // setting: None, ReadOnly or ReadWrite. Empty when not applicable. DefaultMemberAccessRights string + // AccessRuleID is the $ID of the entity access rule this row comes from, + // and AccessRuleIndex that rule's position on the entity (from 0): rows + // sharing them are one rule (ako/mxcli#1437). "" and -1 for a row no access + // rule produces. + AccessRuleID string + AccessRuleIndex int } // PermissionsFor returns an iterator over all permissions for a given entity. @@ -399,7 +405,8 @@ func (ctx *LintContext) PermissionsFor(entityQualifiedName string) iter.Seq[Perm return func(yield func(Permission) bool) { rows, err := ctx.db.Query(` SELECT ModuleRoleName, ElementName, MemberName, AccessType, XPathConstraint, - COALESCE(DefaultMemberAccessRights, ''), ModuleName + COALESCE(DefaultMemberAccessRights, ''), ModuleName, + COALESCE(AccessRuleId, ''), COALESCE(AccessRuleIndex, -1) FROM permissions WHERE ElementType = 'ENTITY' AND ElementName = ? ORDER BY ModuleRoleName, AccessType @@ -414,7 +421,7 @@ func (ctx *LintContext) PermissionsFor(entityQualifiedName string) iter.Seq[Perm var p Permission var memberName, xpathConstraint, moduleName sql.NullString err := rows.Scan(&p.ModuleRoleName, &p.EntityName, &memberName, &p.AccessType, &xpathConstraint, - &p.DefaultMemberAccessRights, &moduleName) + &p.DefaultMemberAccessRights, &moduleName, &p.AccessRuleID, &p.AccessRuleIndex) if err != nil { ctx.recordQueryError("PermissionsFor (row scan)", err) continue @@ -444,6 +451,12 @@ type AllPermission struct { // DefaultMemberAccessRights is the rule's "default rights for new members" // setting: None, ReadOnly or ReadWrite. Empty for non-entity permissions. DefaultMemberAccessRights string + // AccessRuleID is the $ID of the entity access rule this row comes from, + // and AccessRuleIndex that rule's position on the entity (from 0): rows + // sharing them are one rule (ako/mxcli#1437). "" and -1 for a row no access + // rule produces. + AccessRuleID string + AccessRuleIndex int } // Permissions returns an iterator over all permissions in the catalog. @@ -456,7 +469,8 @@ func (ctx *LintContext) Permissions() iter.Seq[AllPermission] { SELECT ModuleRoleName, ElementType, ElementName, COALESCE(MemberName, ''), AccessType, COALESCE(XPathConstraint, ''), COALESCE(DefaultMemberAccessRights, ''), - COALESCE(ModuleName, '') + COALESCE(ModuleName, ''), + COALESCE(AccessRuleId, ''), COALESCE(AccessRuleIndex, -1) FROM permissions ORDER BY ElementType, ElementName, ModuleRoleName, AccessType `) @@ -470,7 +484,7 @@ func (ctx *LintContext) Permissions() iter.Seq[AllPermission] { var p AllPermission if err := rows.Scan(&p.ModuleRoleName, &p.ElementType, &p.ElementName, &p.MemberName, &p.AccessType, &p.XPathConstraint, - &p.DefaultMemberAccessRights, &p.ModuleName); err != nil { + &p.DefaultMemberAccessRights, &p.ModuleName, &p.AccessRuleID, &p.AccessRuleIndex); err != nil { continue } p.IsConstrained = p.XPathConstraint != "" diff --git a/mdl/linter/starlark.go b/mdl/linter/starlark.go index 3d3522ec28..71f4c6a45a 100644 --- a/mdl/linter/starlark.go +++ b/mdl/linter/starlark.go @@ -1271,6 +1271,8 @@ func allPermissionToStarlark(p AllPermission) starlark.Value { "is_constrained": starlark.Bool(p.IsConstrained), "module_name": starlark.String(p.ModuleName), "default_member_access_rights": starlark.String(p.DefaultMemberAccessRights), + "access_rule_id": starlark.String(p.AccessRuleID), + "access_rule_index": starlark.MakeInt(p.AccessRuleIndex), }) } @@ -1285,6 +1287,9 @@ func permissionToStarlark(p Permission) starlark.Value { "xpath_constraint": starlark.String(p.XPathConstraint), "is_constrained": starlark.Bool(p.IsConstrained), "default_member_access_rights": starlark.String(p.DefaultMemberAccessRights), + // Rows sharing an access_rule_id are one access rule (ako/mxcli#1437). + "access_rule_id": starlark.String(p.AccessRuleID), + "access_rule_index": starlark.MakeInt(p.AccessRuleIndex), }) } diff --git a/mdl/linter/starlark_conv006_test.go b/mdl/linter/starlark_conv006_test.go index 3ccbb5fd9e..cc40bf75ba 100644 --- a/mdl/linter/starlark_conv006_test.go +++ b/mdl/linter/starlark_conv006_test.go @@ -39,9 +39,10 @@ func TestCONV006GroupsByEntity(t *testing.T) { ('e3','Draft','App.Draft','App','','NON_PERSISTENT','','',1,1,0,0,0, 0,0,0,0)`, `CREATE TABLE permissions (ModuleRoleName TEXT, ElementType TEXT, ElementName TEXT, MemberName TEXT, AccessType TEXT, XPathConstraint TEXT, - DefaultMemberAccessRights TEXT, ModuleName TEXT)`, + DefaultMemberAccessRights TEXT, ModuleName TEXT, + AccessRuleId TEXT, AccessRuleIndex INTEGER)`, // Two access rules for App.User on Logboek: its CREATE must be listed once. - `INSERT INTO permissions VALUES + `INSERT INTO permissions (ModuleRoleName, ElementType, ElementName, MemberName, AccessType, XPathConstraint, DefaultMemberAccessRights, ModuleName) VALUES ('App.Admin','ENTITY','App.Logboek',NULL,'CREATE','','ReadWrite','App'), ('App.Admin','ENTITY','App.Logboek',NULL,'DELETE','','ReadWrite','App'), ('App.Admin','ENTITY','App.Logboek',NULL,'READ','','ReadWrite','App'), diff --git a/mdl/linter/starlark_permission_rule_id_1437_test.go b/mdl/linter/starlark_permission_rule_id_1437_test.go new file mode 100644 index 0000000000..5523e90ca1 --- /dev/null +++ b/mdl/linter/starlark_permission_rule_id_1437_test.go @@ -0,0 +1,53 @@ +// SPDX-License-Identifier: Apache-2.0 + +package linter_test + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/linter" +) + +// ako/mxcli#1437 (mendixlabs/mxcli#1539): permissions_for() returned one row +// per module role and right with no id of the access rule it came from, so a +// rule could not count an entity's access rules per setting or list one rule's +// module roles exactly — it had to guess by grouping on XPath, default and +// rights, which merges two rules that happen to agree on those. +func TestPermissionRowsNameTheirAccessRule(t *testing.T) { + cat := emptyCatalog(t) + if _, err := cat.CatalogDB().Exec(`INSERT INTO permissions + (ModuleRoleName, ElementType, ElementName, AccessType, XPathConstraint, DefaultMemberAccessRights, ModuleName, AccessRuleId, AccessRuleIndex) VALUES + -- Two rules identical in every grouping key a rule could use before. + ('Shop.User', 'ENTITY', 'Shop.Order', 'READ', '', 'ReadWrite', 'Shop', 'rule-a', 0), + ('Shop.Admin', 'ENTITY', 'Shop.Order', 'READ', '', 'ReadWrite', 'Shop', 'rule-b', 1), + ('Shop.Admin', 'ENTITY', 'Shop.Order', 'WRITE', '', 'ReadWrite', 'Shop', 'rule-b', 1), + -- A page permission comes from no access rule. + ('Shop.User', 'PAGE', 'Shop.Order_Overview', 'VIEW', NULL, NULL, 'Shop', NULL, NULL)`); err != nil { + t.Fatal(err) + } + ctx := linter.NewLintContext(cat, nil) + + got := runDump1437(t, ctx, ` rules = {} + for p in permissions_for("Shop.Order"): + rules.setdefault("%s@%d" % (p.access_rule_id, p.access_rule_index), []).append(p.module_role_name) + return [violation(message = "%s=%s" % (k, ",".join(sorted({r: True for r in v}.keys())))) for k, v in sorted(rules.items())]`) + if want := "rule-a@0=Shop.User|rule-b@1=Shop.Admin"; strings.Join(got, "|") != want { + t.Errorf("permissions_for grouped by access rule = %v, want %s", got, want) + } + + got = runDump1437(t, ctx, ` return [violation(message = "%s|%s|%d" % (p.element_name, p.access_rule_id, p.access_rule_index)) for p in permissions() if p.element_type == "PAGE"]`) + if want := "Shop.Order_Overview||-1"; strings.Join(got, ",") != want { + t.Errorf("a page permission = %v, want %s (no access rule: empty id, index -1)", got, want) + } +} + +func runDump1437(t *testing.T, ctx *linter.LintContext, body string) []string { + t.Helper() + vs, _ := runSrc(t, ctx, "def check():\n"+body) + var got []string + for _, v := range vs { + got = append(got, v.Message) + } + return got +} diff --git a/mdl/linter/starlark_sec008_test.go b/mdl/linter/starlark_sec008_test.go index 0623e50bcb..1ee79993c6 100644 --- a/mdl/linter/starlark_sec008_test.go +++ b/mdl/linter/starlark_sec008_test.go @@ -42,11 +42,12 @@ func TestSEC008CountsOnlyRolesThatReadAPiiMember(t *testing.T) { ('a2','Email','e1','FS.Engineer','FS','String',200,0,0,'',0,'')`, `CREATE TABLE permissions (ModuleRoleName TEXT, ElementType TEXT, ElementName TEXT, MemberName TEXT, AccessType TEXT, XPathConstraint TEXT, - DefaultMemberAccessRights TEXT, ModuleName TEXT)`, + DefaultMemberAccessRights TEXT, ModuleName TEXT, + AccessRuleId TEXT, AccessRuleIndex INTEGER)`, // Coordinator reads Email (explicit, qualified member name), Admin reads // it through default rights (bare member name), FieldEngineer reads only // FullName, Requester reads Email but row-scoped. - `INSERT INTO permissions VALUES + `INSERT INTO permissions (ModuleRoleName, ElementType, ElementName, MemberName, AccessType, XPathConstraint, DefaultMemberAccessRights, ModuleName) VALUES ('FS.Coordinator','ENTITY','FS.Engineer',NULL,'READ','','None','FS'), ('FS.Coordinator','ENTITY','FS.Engineer','FS.Engineer.FullName','MEMBER_READ','','None','FS'), ('FS.Coordinator','ENTITY','FS.Engineer','FS.Engineer.Email','MEMBER_READ','','None','FS'), From 2675472473a206330702aaa8a495e8e7dd261e7f Mon Sep 17 00:00:00 2001 From: Ako Date: Sat, 31 Oct 2026 21:12:45 +0000 Subject: [PATCH 03/10] feat(lint): --baseline reports only findings not in a recorded baseline (#1328) On a mature app lint's totals are the backlog, not the change just made. `mxcli lint --baseline --update-baseline` records the current findings; `mxcli lint --baseline ` then reports only the new ones, with an exit code that follows them alone and an "N new, M already known, K fixed" summary on stderr. A finding is keyed on its rule and document (the document id where the rule gives one, so a rename keeps it), not its message: within a key, findings match by identical message first and then by count, so a reworded rule is not new while a further finding of the same rule in the same document is. Rule failures are never baselined. A scoped run (-m/-d/-r) reports new findings but not fixed ones, and cannot update the baseline. Co-Authored-By: Claude Opus 5.5 --- .claude/commands/mendix/lint.md | 4 + CHANGELOG.md | 1 + cmd/mxcli/cmd_lint.go | 61 ++++++++- cmd/mxcli/main.go | 2 + docs-site/src/tools/mxcli-lint.md | 35 ++++++ mdl/linter/baseline.go | 200 ++++++++++++++++++++++++++++++ mdl/linter/baseline_test.go | 115 +++++++++++++++++ 7 files changed, 416 insertions(+), 2 deletions(-) create mode 100644 mdl/linter/baseline.go create mode 100644 mdl/linter/baseline_test.go diff --git a/.claude/commands/mendix/lint.md b/.claude/commands/mendix/lint.md index 0894d3f077..ce4e601023 100644 --- a/.claude/commands/mendix/lint.md +++ b/.claude/commands/mendix/lint.md @@ -27,6 +27,10 @@ mxcli lint -p app.mpr --list-rules # Exclude modules mxcli lint -p app.mpr --exclude System --exclude Administration + +# Only findings not in a recorded baseline (record it once with --update-baseline) +mxcli lint -p app.mpr --baseline lint-baseline.json --update-baseline +mxcli lint -p app.mpr --baseline lint-baseline.json ``` ## Built-in Go Rules diff --git a/CHANGELOG.md b/CHANGELOG.md index aaf3dfdc3a..bed73c20d5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Added +- **`mxcli lint --baseline ` reports only new findings** (ako/mxcli#1328, mendixlabs/mxcli#1455) — record a baseline with `--update-baseline`, commit it, and later runs report only the findings it does not have, with an exit code that follows them alone and a `N new, M already known, K fixed` summary on stderr. Findings are matched on rule and document, not message, so rewording a rule does not make its findings new; a scoped run (`-m`, `-d`, `-r`) reports new findings but cannot update the baseline. - **Optional page parameters** (mendixlabs/mxcli#1640, ako/mxcli#1649) — `Params: ( $Sale: Shop.Sale optional )` is Studio Pro's unticked *Required* (`Forms$PageParameter.IsRequired = false`, Mendix 11.5+): the page can be opened with no argument, from a menu item for example, where a required parameter is CE1571. `create page`, `alter page … add parameters` and `describe page` all spell it. `describe navigation` no longer invents `(Sale = $currentObject)` for a menu item opening such a page, which made its output one `exec` refused (MDL-PAGEARG01) and left the navigation profile unchangeable from MDL. Below 11.5 `optional` is refused (MDL-VERSION01), and a snippet parameter cannot be optional (MDL087). ### Changed diff --git a/cmd/mxcli/cmd_lint.go b/cmd/mxcli/cmd_lint.go index d0ff36e519..66a2ab5d1d 100644 --- a/cmd/mxcli/cmd_lint.go +++ b/cmd/mxcli/cmd_lint.go @@ -93,6 +93,17 @@ Examples: mxcli lint -p app.mpr -m MyModule -m AnotherModule mxcli lint -p app.mpr -d Sales.ACT_Order mxcli lint -p app.mpr -d Sales.ACT_Order -r CONV011 + mxcli lint -p app.mpr --baseline lint-baseline.json --update-baseline + mxcli lint -p app.mpr --baseline lint-baseline.json + +--baseline reports only the findings that are not in a recorded baseline, so the +result on a mature app is about the change just made, not the backlog; the exit +code follows the new findings alone. --update-baseline records the current +findings there instead (commit the file). A finding is matched on its rule and +its document, not its message, so rewording a rule does not make its findings +new. A run scoped with --modules, --documents or --rules sees only part of the +project: it reports new findings but not how many were fixed, and cannot update +the baseline. --documents scopes a run to named documents, so a structural question about one microflow costs a fraction of a project-wide lint and needs no baseline diff to @@ -108,6 +119,31 @@ modules, and findings that are not about those documents are not reported. onlyRules, _ := cmd.Flags().GetStringSlice("rules") moduleFilter, _ := cmd.Flags().GetStringSlice("modules") documentFilter, _ := cmd.Flags().GetStringSlice("documents") + baselinePath, _ := cmd.Flags().GetString("baseline") + updateBaseline, _ := cmd.Flags().GetBool("update-baseline") + scoped := len(onlyRules) > 0 || len(moduleFilter) > 0 || len(documentFilter) > 0 + if updateBaseline && baselinePath == "" { + fmt.Fprintln(os.Stderr, "Error: --update-baseline needs --baseline to write to") + os.Exit(1) + } + // A scoped run sees part of the project; recording it would drop every + // other finding from the baseline, and the next full run would report + // them all as new. + if updateBaseline && scoped { + fmt.Fprintln(os.Stderr, "Error: --update-baseline records the whole project; drop --modules, --documents and --rules") + os.Exit(1) + } + // Loaded before connecting, for the same reason as the --documents check + // below: a typo in the path should not wait for a catalog build. + var baseline *linter.Baseline + if baselinePath != "" && !updateBaseline { + b, err := linter.LoadBaseline(baselinePath) + if err != nil { + fmt.Fprintf(os.Stderr, "Error: %v\n", err) + os.Exit(1) + } + baseline = b + } // Checked before connecting: a malformed filter is the caller's typo, and // making them wait for a catalog build to hear about it is the slow half // of the problem --documents exists to fix. @@ -277,6 +313,26 @@ modules, and findings that are not about those documents are not reported. violations = keepDocumentViolations(violations, documentFilter) } + // Baseline: record the findings, or keep only the new ones. On stderr, + // so the summary cannot corrupt --format json/sarif. + if updateBaseline { + if err := linter.WriteBaseline(baselinePath, violations); err != nil { + fmt.Fprintf(os.Stderr, "Error writing lint baseline: %v\n", err) + os.Exit(1) + } + fmt.Fprintf(os.Stderr, "Recorded %d finding(s) in the lint baseline %s\n", len(violations), baselinePath) + } else if baseline != nil { + cmp := baseline.Compare(violations) + violations = cmp.New + if scoped { + fmt.Fprintf(os.Stderr, "Lint baseline %s: %d new, %d already known (a scoped run: fixed findings are not counted)\n", + baselinePath, len(cmp.New), cmp.Unchanged) + } else { + fmt.Fprintf(os.Stderr, "Lint baseline %s: %d new, %d already known, %d fixed\n", + baselinePath, len(cmp.New), cmp.Unchanged, cmp.Fixed) + } + } + // Output results outputFormat := linter.OutputFormat(format) formatter := linter.GetFormatter(outputFormat, useColor) @@ -311,9 +367,10 @@ modules, and findings that are not about those documents are not reported. // corrupt --format json/sarif. reportBrainGap(projectDir) - // Exit with error if there are errors + // Exit with error if there are errors — the new ones, against a + // baseline. Recording a baseline accepts what it records. summary := linter.Summarize(violations) - if summary.Errors > 0 { + if summary.Errors > 0 && !updateBaseline { os.Exit(1) } }, diff --git a/cmd/mxcli/main.go b/cmd/mxcli/main.go index ce1dba1fc6..926bd612a3 100644 --- a/cmd/mxcli/main.go +++ b/cmd/mxcli/main.go @@ -418,6 +418,8 @@ func init() { lintCmd.Flags().StringSliceP("rules", "r", nil, "Only run these rule IDs (e.g. -r MPR001 -r SEC001)") lintCmd.Flags().StringSliceP("modules", "m", nil, "Only lint the specified modules (comma-separated or repeated)") lintCmd.Flags().StringSliceP("documents", "d", nil, "Only lint these documents, by qualified name (e.g. Sales.ACT_Order); comma-separated or repeated") + lintCmd.Flags().String("baseline", "", "Report only findings not recorded in this baseline file (exit code follows the new ones)") + lintCmd.Flags().Bool("update-baseline", false, "Record the current findings in the --baseline file instead of comparing") // Report command flags reportCmd.Flags().StringP("format", "f", "markdown", "Output format: markdown, json, html") diff --git a/docs-site/src/tools/mxcli-lint.md b/docs-site/src/tools/mxcli-lint.md index 4a2ad410da..6d111b6ca3 100644 --- a/docs-site/src/tools/mxcli-lint.md +++ b/docs-site/src/tools/mxcli-lint.md @@ -64,6 +64,41 @@ mxcli lint -p app.mpr --exclude System --exclude Administration mxcli lint -p app.mpr --color ``` +## Baseline: Report Only New Findings + +On a mature app the existing backlog buries the findings about the change you +just made. Record a baseline once, commit it, and compare against it afterwards: + +```bash +# Record today's findings +mxcli lint -p app.mpr --baseline lint-baseline.json --update-baseline + +# Later: report only what is not in the baseline +mxcli lint -p app.mpr --baseline lint-baseline.json +``` + +The comparison prints a summary on stderr, so `--format json` and `--format sarif` +stay clean: + +``` +Lint baseline lint-baseline.json: 1 new, 1518 already known, 0 fixed +``` + +- Only the **new** findings are reported, and the exit code follows them alone. +- A finding is matched on its **rule and document** (the document's id where the + rule gives one, so renaming a document keeps its findings), not on its message. + Rewording a rule therefore does not make its findings new, while a further + finding of the same rule in the same document is. +- A rule that **fails to run** is never absorbed by the baseline. +- A run scoped with `--modules`, `--documents` or `--rules` sees only part of the + project: it reports new findings but not how many were fixed, and + `--update-baseline` refuses it, since recording it would drop every finding + outside the scope. +- A missing or unreadable baseline file is an error, not "everything is new". + +The file is sorted JSON, so re-recording an unchanged project rewrites the same +bytes and a committed baseline diffs only where findings changed. + ## CI Integration ### GitHub Actions diff --git a/mdl/linter/baseline.go b/mdl/linter/baseline.go new file mode 100644 index 0000000000..f943edc76d --- /dev/null +++ b/mdl/linter/baseline.go @@ -0,0 +1,200 @@ +// SPDX-License-Identifier: Apache-2.0 + +package linter + +import ( + "encoding/json" + "fmt" + "os" + "sort" +) + +// A baseline records a project's lint findings at one moment, so a later run +// can report only what is new (mendixlabs/mxcli#1455, ako/mxcli#1328). On a +// mature app the existing backlog buries the findings about the change just +// made; teams worked around it from SARIF output, each with their own +// fingerprint, and those broke whenever a rule's message text changed. +// +// # What makes two findings the same +// +// A finding is keyed on its rule and its document — the document's id where the +// rule gives one, so a rename does not make its findings new, and its qualified +// name otherwise. The message is NOT part of the key, because it is exactly what +// changes when a rule is reworded. +// +// One rule can report one document several times (one finding per loop, per +// attribute), so within a key the findings are matched in two passes: first by +// identical message, then — for the ones left over on both sides — by count, +// taking an unmatched baseline finding as the reworded form of an unmatched +// current one. What remains on the current side is new. A third CONV011 in a +// flow that had two is therefore new, while a reworded QUAL001 is not. +// +// A rule failure is never baselined: it says nothing about the model, and +// absorbing it would let a broken rule read as "nothing new". + +// baselineVersion is the file format. A file of another version is refused +// rather than half-read. +const baselineVersion = 1 + +// Baseline is a loaded baseline file. +type Baseline struct { + byKey map[string][]string // key -> messages, one per recorded finding + total int +} + +// BaselineComparison is what a run found against a baseline. +type BaselineComparison struct { + New []Violation // not in the baseline, in the order the run reported them + Unchanged int // current findings the baseline already had + Fixed int // baseline findings the run no longer reports +} + +type baselineFile struct { + Version int `json:"version"` + Findings []baselineFinding `json:"findings"` +} + +type baselineFinding struct { + Rule string `json:"rule"` + Document string `json:"document"` // qualified name, for a reader + ID string `json:"documentId,omitempty"` // the key when present + Type string `json:"documentType,omitempty"` + Message string `json:"message"` +} + +func baselineKey(rule, docType, id, document string) string { + if id != "" { + return rule + "\x00id\x00" + id + } + return rule + "\x00" + docType + "\x00" + document +} + +func keyOf(v Violation) string { + return baselineKey(v.RuleID, v.Location.DocumentType, v.Location.DocumentID, v.Location.QualifiedName()) +} + +// WriteBaseline records findings as a baseline at path. Rule failures are left +// out. The file is sorted, so re-recording an unchanged project rewrites the +// same bytes and a committed baseline diffs only where findings changed. +func WriteBaseline(path string, findings []Violation) error { + f := baselineFile{Version: baselineVersion, Findings: []baselineFinding{}} + for _, v := range findings { + if v.RuleFailure { + continue + } + f.Findings = append(f.Findings, baselineFinding{ + Rule: v.RuleID, + Document: v.Location.QualifiedName(), + ID: v.Location.DocumentID, + Type: v.Location.DocumentType, + Message: v.Message, + }) + } + sort.Slice(f.Findings, func(i, j int) bool { + a, b := f.Findings[i], f.Findings[j] + if a.Rule != b.Rule { + return a.Rule < b.Rule + } + if a.Document != b.Document { + return a.Document < b.Document + } + if a.ID != b.ID { + return a.ID < b.ID + } + return a.Message < b.Message + }) + data, err := json.MarshalIndent(f, "", " ") + if err != nil { + return err + } + return os.WriteFile(path, append(data, '\n'), 0o644) +} + +// LoadBaseline reads a baseline written by WriteBaseline. A missing, corrupt or +// other-version file is an error: comparing against nothing would report the +// whole backlog as new, or — worse, with a half-read file — part of it. +func LoadBaseline(path string) (*Baseline, error) { + data, err := os.ReadFile(path) + if err != nil { + return nil, fmt.Errorf("reading lint baseline: %w", err) + } + var f baselineFile + if err := json.Unmarshal(data, &f); err != nil { + return nil, fmt.Errorf("lint baseline %s is not a baseline file: %w", path, err) + } + if f.Version != baselineVersion { + return nil, fmt.Errorf("lint baseline %s has format version %d; this mxcli reads version %d — re-record it with --update-baseline", + path, f.Version, baselineVersion) + } + b := &Baseline{byKey: map[string][]string{}, total: len(f.Findings)} + for _, x := range f.Findings { + k := baselineKey(x.Rule, x.Type, x.ID, x.Document) + b.byKey[k] = append(b.byKey[k], x.Message) + } + return b, nil +} + +// Len is the number of findings the baseline records. +func (b *Baseline) Len() int { return b.total } + +// Compare splits a run's findings into new and already-known ones, and counts +// the baseline findings the run no longer reports. +func (b *Baseline) Compare(findings []Violation) BaselineComparison { + var out BaselineComparison + + groups := map[string][]int{} // key -> indexes into findings + var order []string + for i, v := range findings { + if v.RuleFailure { + continue + } + k := keyOf(v) + if _, seen := groups[k]; !seen { + order = append(order, k) + } + groups[k] = append(groups[k], i) + } + + isNew := make([]bool, len(findings)) + for i, v := range findings { + isNew[i] = v.RuleFailure + } + matched := 0 + for _, k := range order { + // Pass 1: identical messages. + left := map[string]int{} + for _, m := range b.byKey[k] { + left[m]++ + } + remainingBaseline := len(b.byKey[k]) + var unmatched []int + for _, i := range groups[k] { + if left[findings[i].Message] > 0 { + left[findings[i].Message]-- + remainingBaseline-- + matched++ + continue + } + unmatched = append(unmatched, i) + } + // Pass 2: an unmatched baseline finding is taken as the reworded form + // of an unmatched current one; the rest are new. + for _, i := range unmatched { + if remainingBaseline > 0 { + remainingBaseline-- + matched++ + continue + } + isNew[i] = true + } + } + + for i, v := range findings { + if isNew[i] { + out.New = append(out.New, v) + } + } + out.Unchanged = matched + out.Fixed = b.total - matched + return out +} diff --git a/mdl/linter/baseline_test.go b/mdl/linter/baseline_test.go new file mode 100644 index 0000000000..babf970a46 --- /dev/null +++ b/mdl/linter/baseline_test.go @@ -0,0 +1,115 @@ +// SPDX-License-Identifier: Apache-2.0 + +package linter + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// mendixlabs/mxcli#1455 (ako/mxcli#1328): on a mature app lint reports a large +// existing backlog, so its totals say nothing about the change just made. A +// baseline records today's findings; a later run reports only what is new. + +func v(rule, module, doc, id, msg string) Violation { + return Violation{RuleID: rule, Severity: SeverityWarning, Message: msg, + Location: Location{Module: module, DocumentType: "microflow", DocumentName: doc, DocumentID: id}} +} + +func roundTrip(t *testing.T, findings []Violation) *Baseline { + t.Helper() + path := filepath.Join(t.TempDir(), "lint-baseline.json") + if err := WriteBaseline(path, findings); err != nil { + t.Fatalf("WriteBaseline: %v", err) + } + b, err := LoadBaseline(path) + if err != nil { + t.Fatalf("LoadBaseline: %v", err) + } + return b +} + +func messages(vs []Violation) string { + var out []string + for _, x := range vs { + out = append(out, x.RuleID+" "+x.Location.QualifiedName()+" "+x.Message) + } + return strings.Join(out, " | ") +} + +func TestBaselineReportsOnlyNewFindings(t *testing.T) { + old := []Violation{ + v("QUAL001", "Sales", "ACT_A", "id-a", "ACT_A has no documentation"), + v("CONV011", "Sales", "ACT_A", "id-a", "loop over $Orders commits inside"), + v("CONV011", "Sales", "ACT_A", "id-a", "loop over $Lines commits inside"), + v("SEC001", "Sales", "ACT_Gone", "id-gone", "fixed since"), + } + b := roundTrip(t, old) + + now := []Violation{ + // Unchanged. + v("QUAL001", "Sales", "ACT_A", "id-a", "ACT_A has no documentation"), + v("CONV011", "Sales", "ACT_A", "id-a", "loop over $Orders commits inside"), + v("CONV011", "Sales", "ACT_A", "id-a", "loop over $Lines commits inside"), + // A third finding of the same rule in the same document is new, even + // though the rule and the document are already in the baseline. + v("CONV011", "Sales", "ACT_A", "id-a", "loop over $Items commits inside"), + // A new document. + v("QUAL001", "Sales", "ACT_B", "id-b", "ACT_B has no documentation"), + } + got := b.Compare(now) + if want := "CONV011 Sales.ACT_A loop over $Items commits inside | QUAL001 Sales.ACT_B ACT_B has no documentation"; messages(got.New) != want { + t.Errorf("new = %q,\nwant %q", messages(got.New), want) + } + if got.Fixed != 1 || got.Unchanged != 3 { + t.Errorf("fixed = %d, unchanged = %d; want 1 and 3", got.Fixed, got.Unchanged) + } +} + +// The reason teams' own SARIF fingerprints broke: a rule's wording changes. +// A reworded finding in the same place is the same finding. +func TestBaselineSurvivesARewordedMessage(t *testing.T) { + b := roundTrip(t, []Violation{v("QUAL001", "Sales", "ACT_A", "id-a", "Microflow has no documentation")}) + got := b.Compare([]Violation{v("QUAL001", "Sales", "ACT_A", "id-a", "ACT_A has no documentation — add a description")}) + if len(got.New) != 0 || got.Unchanged != 1 { + t.Errorf("a reworded message read as new: new = %q, unchanged = %d", messages(got.New), got.Unchanged) + } +} + +// A renamed document keeps its id, so its findings are not new. +func TestBaselineFollowsADocumentByItsID(t *testing.T) { + b := roundTrip(t, []Violation{v("QUAL001", "Sales", "ACT_Old", "id-a", "no documentation")}) + if got := b.Compare([]Violation{v("QUAL001", "Sales", "ACT_New", "id-a", "no documentation")}); len(got.New) != 0 { + t.Errorf("a renamed document's finding read as new: %q", messages(got.New)) + } + // Control: without an id the name is the key, and a different document is new. + b = roundTrip(t, []Violation{v("QUAL001", "Sales", "ACT_Old", "", "no documentation")}) + if got := b.Compare([]Violation{v("QUAL001", "Sales", "ACT_Other", "", "no documentation")}); len(got.New) != 1 { + t.Errorf("a finding on another document was matched to the baseline: new = %q", messages(got.New)) + } +} + +// A rule that failed to run says nothing about the model: it is never +// absorbed by a baseline, or a broken rule would read as "nothing new". +func TestBaselineNeverAbsorbsARuleFailure(t *testing.T) { + fail := Violation{RuleID: "CUST01", Severity: SeverityError, Message: "rule failed", RuleFailure: true} + b := roundTrip(t, []Violation{fail}) + if got := b.Compare([]Violation{fail}); len(got.New) != 1 { + t.Errorf("a rule failure was absorbed by the baseline: %q", messages(got.New)) + } +} + +func TestLoadBaselineRejectsAFileItCannotRead(t *testing.T) { + path := filepath.Join(t.TempDir(), "b.json") + if err := os.WriteFile(path, []byte("not json"), 0o644); err != nil { + t.Fatal(err) + } + if _, err := LoadBaseline(path); err == nil { + t.Error("a corrupt baseline loaded; a gate must not silently compare against nothing") + } + if _, err := LoadBaseline(filepath.Join(t.TempDir(), "missing.json")); err == nil { + t.Error("a missing baseline loaded; it must be an error, or every finding would read as new without saying why") + } +} From 0804473ce5732e24df77dde3502d22fa2983c321 Mon Sep 17 00:00:00 2001 From: Ako Date: Sun, 1 Nov 2026 13:19:44 +0000 Subject: [PATCH 04/10] fix(mcp): treat "Error executing tool handler" as a failed tool call MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Studio Pro answers a tool whose handler throws with "Error executing tool handler: …" and isError:false (measured on 11.16.0 for ped_delete_document). Only the create/update callers looked at the text, so any other caller trusting the flag read the failure as success. CallTool now sets IsError for that text. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + mdl/backend/mcp/client.go | 13 ++++++++++++- mdl/backend/mcp/handler_error_test.go | 20 ++++++++++++++++++++ 3 files changed, 33 insertions(+), 1 deletion(-) create mode 100644 mdl/backend/mcp/handler_error_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index a4234f5012..f31b06bd74 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **A PED tool whose handler throws is reported as a failure** — Studio Pro answers `Error executing tool handler: …` with `isError:false` (measured on 11.16.0), which a caller trusting the flag read as success; the MCP client now classifies that text as an error. - **MPR012 follows the app's Web UI client** (ako/mxcli#1287, mendixlabs/mxcli#1226) — a Mendix 11 app can still run the classic client, where the legacy static and dynamic image widgets build clean, but MPR012 warned on each of them that "mxbuild reports CE0582". On the classic client or in migration mode it is now an info-level note that the widget needs converting before the app switches; under the React client it stays a warning. - **Every `mxcli init --tool ` installs the bundled lint rules, and `lint` says when it runs without them** (ako/mxcli#1292, mendixlabs/mxcli#1242) — only `claude` and `opencode` wrote `.claude/lint-rules/`, so `init --tool cursor` (or aider, continue, copilot, vibe, windsurf) left `mxcli lint` running the built-in Go rules alone, silently. `lint` and `report` now print a warning when no `.claude/lint-rules/` is found, naming how many rules run instead. - **`show references to` an entity, attribute or association lists the message definitions that expose it** (#1781) — a message definition (the pre-11.15 collection, `MESSAGE_DEFINITION_COLLECTION`, and the 11.15+ document, `MESSAGE_DEFINITION`) was not walked for references, so impact analysis called an exposed entity or attribute unused. The entity edges are `mapping`, the attribute and association edges `member`, as for an import or export mapping. diff --git a/mdl/backend/mcp/client.go b/mdl/backend/mcp/client.go index 5e8d345d7f..259ee749e3 100644 --- a/mdl/backend/mcp/client.go +++ b/mdl/backend/mcp/client.go @@ -314,7 +314,18 @@ func (c *Client) CallTool(name string, arguments any) (*ToolResult, error) { } sb.WriteString(blk.Text) } - return &ToolResult{IsError: tc.IsError, Text: sb.String()}, nil + text := sb.String() + return &ToolResult{IsError: tc.IsError || handlerFailed(text), Text: text}, nil +} + +// handlerFailedPrefix opens the text Studio Pro returns when a tool's handler +// throws. It arrives with isError:false (measured on 11.16.0, ped_delete_document: +// "Error executing tool handler: Session 'mcp-server' not found"), so a caller +// that trusts the flag would read a failed call as a success. +const handlerFailedPrefix = "Error executing tool handler:" + +func handlerFailed(text string) bool { + return strings.HasPrefix(strings.TrimSpace(text), handlerFailedPrefix) } // call sends a JSON-RPC request and returns the matching response, handling diff --git a/mdl/backend/mcp/handler_error_test.go b/mdl/backend/mcp/handler_error_test.go new file mode 100644 index 0000000000..47f69f8149 --- /dev/null +++ b/mdl/backend/mcp/handler_error_test.go @@ -0,0 +1,20 @@ +// SPDX-License-Identifier: Apache-2.0 + +package mcp + +import "testing" + +// handlerSessionError is Studio Pro 11.16.0's verbatim ped_delete_document +// failure, measured live: the tool ran, failed, and still said isError:false. +const handlerSessionError = "Error executing tool handler: Session 'mcp-server' not found" + +func TestCallTool_HandlerErrorIsAnError(t *testing.T) { + f := newFakePED(t, func(string, map[string]any) (string, bool) { return handlerSessionError, false }) + res, err := f.connectClient(t).CallTool("ped_delete_document", map[string]any{}) + if err != nil { + t.Fatalf("CallTool: %v", err) + } + if !res.IsError { + t.Fatalf("a %q result must be reported as an error, got IsError=false", handlerSessionError) + } +} From 91d26f73a60cad209d1828db5e9e5d97510eb7b0 Mon Sep 17 00:00:00 2001 From: Ako Date: Sun, 1 Nov 2026 13:20:44 +0000 Subject: [PATCH 05/10] feat(mcp): DROP through ped_delete_document when Studio Pro has it Studio Pro 11.16 ships ped_delete_document, but every DROP of a standalone document (constant, enumeration, microflow, nanoflow, page, workflow, business-event service) still demanded --mcp-concord. deleteDocument asks the native tool when tools/list advertises it (Client.HasTool) and keeps Concord for older servers, and as a fallback when the native call fails. The tool's success reply has never been observed (on 11.16.0 it fails outside Studio Pro's agent session with "Session 'mcp-server' not found"), so an unrecognised reply is settled with ped_find_document rather than by guessing the text. Co-Authored-By: Claude Opus 5.5 --- ...-mcp-drop-ignores-ped-delete-document.json | 1 + CHANGELOG.md | 1 + docs/01-project/MDL_FEATURE_MATRIX.md | 4 +- mdl/backend/mcp/businessevent.go | 6 +- mdl/backend/mcp/capabilities.go | 4 + mdl/backend/mcp/capabilities.yaml | 2 +- mdl/backend/mcp/client.go | 13 ++ mdl/backend/mcp/concord.go | 4 +- mdl/backend/mcp/constant.go | 6 +- mdl/backend/mcp/delete.go | 82 +++++++++++++ mdl/backend/mcp/delete_test.go | 114 ++++++++++++++++++ mdl/backend/mcp/enumeration.go | 6 +- mdl/backend/mcp/microflow.go | 6 +- mdl/backend/mcp/nanoflow.go | 6 +- mdl/backend/mcp/page.go | 6 +- mdl/backend/mcp/workflow.go | 6 +- 16 files changed, 241 insertions(+), 26 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/mdl-backend/2026-11-01-mcp-drop-ignores-ped-delete-document.json create mode 100644 mdl/backend/mcp/delete.go create mode 100644 mdl/backend/mcp/delete_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-backend/2026-11-01-mcp-drop-ignores-ped-delete-document.json b/.claude/skills/fix-issue/findings/mdl-backend/2026-11-01-mcp-drop-ignores-ped-delete-document.json new file mode 100644 index 0000000000..6f0537c8dc --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-backend/2026-11-01-mcp-drop-ignores-ped-delete-document.json @@ -0,0 +1 @@ +{"area": "mdl/backend/mcp", "date": "2026-11-01", "symptom": "Over `--mcp` on Studio Pro 11.16, every DROP of a standalone document (constant, enumeration, microflow, nanoflow, page, workflow, business-event service) demanded `--mcp-concord`, though 11.16 ships `ped_delete_document`. Separately, a PED tool whose handler throws answers `Error executing tool handler: …` with `isError:false`, which any caller trusting the flag reads as success", "cause": "`concordDeleteDocument` was the only DROP path, written when PED had no delete tool. `Client.CallTool` passed `isError` through verbatim, and only the create/update callers (`pedOpError`'s `SUCCESS` prefix) noticed failures reported in the text", "file": "`mdl/backend/mcp/delete.go` (new `deleteDocument`: native when `HasTool(\"ped_delete_document\")`, Concord fallback), `client.go` (`HasTool`, `handlerFailed` sets `IsError`), the seven `Delete*` call sites, `capabilities.yaml`/`.go` report text", "insight": "**Probe tool presence, never the version**: `serverInfo.version` was 1.0.0 on 11.12.0 and 11.16.0 reports itself, but the delete tool is the fact that matters. **On 11.16.0 the native delete fails for every caller outside Studio Pro's own agent session** (`Session 'mcp-server' not found`, measured from a devcontainer through four clientInfo names), so the success reply has never been seen. The code therefore settles an unrecognised reply by `ped_find_document` instead of guessing the text, and keeps Concord as a fallback. **Revert check**: stubbing `handlerFailed` fails `TestCallTool_HandlerErrorIsAnError`, and forcing the Concord path fails three `TestDeleteDocument_*`. Live runs are in mx-test-projects/runs/native-delete/"} diff --git a/CHANGELOG.md b/CHANGELOG.md index f31b06bd74..d55f6db6cb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Added +- **DROP over `--mcp` uses Studio Pro 11.16's `ped_delete_document`** — DROP of a constant, enumeration, microflow, nanoflow, page, workflow or business-event service no longer needs `--mcp-concord` when Studio Pro advertises the native delete tool; Concord stays as the fallback for older servers and for a native call that fails. - **Lint permission rows name their access rule** (ako/mxcli#1437, mendixlabs/mxcli#1539) — `permissions()` and `permissions_for()` rows carry `access_rule_id` and `access_rule_index`, so a rule can count an entity's access rules per setting or list one rule's module roles exactly, instead of guessing by grouping on XPath, default and rights. The catalog schema moves to version 31, so a cached catalog is rebuilt once. - **Lint rules can read the data guideline rules asked for** (ako/mxcli#1453, ako/mxcli#1301) — a microflow's `excluded` and `apply_entity_access`, an enumeration's `values` with their captions (so a rule can resolve `getCaption(Module.Enum.Value)`), `constant_values()` for the run-configuration values, and `project_settings()` (default language, OQL version, Java version, hash algorithm, time zone, rounding, decimal scale, optimistic locking, foreign keys, web client). The catalog schema moves to version 30, so a cached catalog is rebuilt once. - **Configurable microflow and nanoflow layout** (ako/mxcli#1832) — a committed `layout-config.yaml` (in `.claude/` or next to the `.mpr`) chooses how `create microflow`/`create nanoflow` and `mxcli layout flows` place what a script leaves unpositioned: preset `happy-line` (the default, unchanged) or `true-right-false-down`, and the options `if-else: true-right-else-column` (ELSE stacked as a column under its decision), `case-split: first-right-rest-columns` (a CASE's or type split's first outcome on the line, the rest as columns), `guard: true-right`, `wrap: off` and `max-row-width`. A flow laid out in a non-default style records it as an `@mxcli-layout:` line in its documentation, which DESCRIBE and `create or modify` keep using for that flow, so its description stays free of `@position` when the project style changes. `mxcli layout config` shows the style in force; `mxcli layout flows --style ` tries one. Without a config file nothing changes: measured on 137 flows, the geometry and DESCRIBE output are identical to before. diff --git a/docs/01-project/MDL_FEATURE_MATRIX.md b/docs/01-project/MDL_FEATURE_MATRIX.md index 03029c7bb7..b7d4b9b59e 100644 --- a/docs/01-project/MDL_FEATURE_MATRIX.md +++ b/docs/01-project/MDL_FEATURE_MATRIX.md @@ -110,7 +110,7 @@ live distinction is **MPR vs MCP**. | **Modules** | Y | Y | Y | P | CREATE only (flushes to disk); no module DROP | | **Entities** | Y | Y | Y | Y | CREATE/DROP; ALTER add/drop/rename/docs — MODIFY *type* rejected (PED can't migrate) | | **Associations** | Y | Y | Y | P | Within-module CREATE/DROP; no custom delete behavior; cross-module / external N | -| **Enumerations** | Y | Y | Y | P | CREATE; DROP via Concord; ALTER (modify values) N | +| **Enumerations** | Y | Y | Y | P | CREATE; DROP via `ped_delete_document` (Studio Pro 11.16+) or Concord; ALTER (modify values) N | | **Microflows** | Y | Y | Y | P | Broad CREATE (most actions + control flow); rejects show-page, cast, retrieve sort/range, inheritance/rule splits | | **Nanoflows** | Y | Y | Y | N | **Not creatable** — PED's `ped_create_document` rejects `Microflows$Nanoflow` (create whitelist covers microflows, not nanoflows); CREATE/ALTER rejected with a clear error. DROP of an existing nanoflow works via Concord. | | **Pages** | Y | Y | Y | P | CREATE + ALTER via `pg_*`; widget/data-source coverage grows one type at a time | @@ -122,7 +122,7 @@ live distinction is **MPR vs MCP**. | **Constants** | Y | Y | Y | P | CREATE / CREATE OR MODIFY (value + exposed-to-client) / DROP (via Concord); type limited to String/Integer/Decimal/Boolean/DateTime; in-place type change rejected; documentation not carried | | **OData Clients / Services** | Y | Y | Y | N | Not wired | | **External Entities** | Y | Y | Y | N | Not wired | -| **Business Events** | Y | Y | Y | P | SHOW/DESCRIBE read the local `.mpr`; DROP via Concord. **CREATE/ALTER blocked** — PED won't create `BusinessEvents$BusinessEventService` (off the create whitelist). The supporting domain model (published-event entities `extends BusinessEvents.PublishedBusinessEvent`, constants) IS creatable. | +| **Business Events** | Y | Y | Y | P | SHOW/DESCRIBE read the local `.mpr`; DROP via `ped_delete_document` (Studio Pro 11.16+) or Concord. **CREATE/ALTER blocked** — PED won't create `BusinessEvents$BusinessEventService` (off the create whitelist). The supporting domain model (published-event entities `extends BusinessEvents.PublishedBusinessEvent`, constants) IS creatable. | | **Navigation** | Y | Y | Y | N | Not wired | | **Project Settings** | Y | Y | Y | N | Not wired | diff --git a/mdl/backend/mcp/businessevent.go b/mdl/backend/mcp/businessevent.go index cb8324d698..aa42daef08 100644 --- a/mdl/backend/mcp/businessevent.go +++ b/mdl/backend/mcp/businessevent.go @@ -43,8 +43,8 @@ func (b *Backend) UpdateBusinessEventService(svc *model.BusinessEventService) er return errCreatePathUnbuilt("business event service", svc.Name) } -// DeleteBusinessEventService drops an existing service via Concord's delete_document -// (PED has no delete tool), like other standalone documents. Requires --mcp-concord. +// DeleteBusinessEventService drops an existing service via deleteDocument: ped_delete_document where Studio Pro has it (11.16+), else +// Concord (--mcp-concord). func (b *Backend) DeleteBusinessEventService(id model.ID) error { services, err := b.reader.ListBusinessEventServices() if err != nil { @@ -56,7 +56,7 @@ func (b *Backend) DeleteBusinessEventService(id model.ID) error { if err != nil { return fmt.Errorf("resolve module for business event service %q: %w", s.Name, err) } - return b.concordDeleteDocument(modName, s.Name) + return b.deleteDocument(businessEventServiceDocType, modName, s.Name) } } return fmt.Errorf("business event service %s not found", id) diff --git a/mdl/backend/mcp/capabilities.go b/mdl/backend/mcp/capabilities.go index df00c2805f..89256b1e43 100644 --- a/mdl/backend/mcp/capabilities.go +++ b/mdl/backend/mcp/capabilities.go @@ -5,6 +5,7 @@ package mcp import ( _ "embed" "fmt" + "slices" "sort" "strconv" "strings" @@ -242,6 +243,9 @@ func (b *Backend) CapabilityReport() string { fmt.Fprintf(&sb, "Studio Pro MCP server : %s %s\n", orUnknown(caps.ServerName), orUnknown(caps.ServerVersion)) fmt.Fprintf(&sb, "Project Mendix version: %s\n", orUnknown(caps.ProjectVersion)) concord := "not connected — DROP of standalone docs (enum/microflow/page/…) is unavailable" + if slices.Contains(caps.Tools, "ped_delete_document") { + concord = "not connected — DROP of standalone docs uses ped_delete_document" + } if caps.ConcordConnected { concord = "connected" } diff --git a/mdl/backend/mcp/capabilities.yaml b/mdl/backend/mcp/capabilities.yaml index 959a530a8e..d961bc5ff9 100644 --- a/mdl/backend/mcp/capabilities.yaml +++ b/mdl/backend/mcp/capabilities.yaml @@ -51,7 +51,7 @@ features: feature: "Enumerations" available: true requires_tools: [ped_create_document] - note: "CREATE (DROP via Concord)" + note: "CREATE / DROP (DROP via ped_delete_document on Studio Pro 11.16+, else Concord)" - key: constants feature: "Constants" available: true diff --git a/mdl/backend/mcp/client.go b/mdl/backend/mcp/client.go index 259ee749e3..5385d9c0b4 100644 --- a/mdl/backend/mcp/client.go +++ b/mdl/backend/mcp/client.go @@ -288,6 +288,19 @@ func (c *Client) SupportsToolArg(tool, arg string) bool { return c.toolArgs[tool][arg] } +// HasTool reports whether the connected server advertises the named tool, from +// the same cached tools/list probe as SupportsToolArg (false when the probe fails). +func (c *Client) HasTool(tool string) bool { + if c.toolArgs == nil { + if _, err := c.ListTools(); err != nil { + c.toolArgs = map[string]map[string]bool{} + return false + } + } + _, ok := c.toolArgs[tool] + return ok +} + func (c *Client) CallTool(name string, arguments any) (*ToolResult, error) { if c.trace.Enabled() { target, detail := summarizeToolCall(name, arguments) diff --git a/mdl/backend/mcp/concord.go b/mdl/backend/mcp/concord.go index af1f0ad320..65ba261e73 100644 --- a/mdl/backend/mcp/concord.go +++ b/mdl/backend/mcp/concord.go @@ -53,8 +53,8 @@ func (b *Backend) SaveAll() error { } // concordDeleteDocument removes a standalone document (enumeration, microflow, -// page) via Concord's delete_document. PED has no delete tool, so DROP of these -// document types is only possible through Concord. +// page) via Concord's delete_document — the DROP path for servers without +// ped_delete_document (before 11.16), and deleteDocument's fallback. func (b *Backend) concordDeleteDocument(moduleName, docName string) error { _, err := b.concordCall("delete_document", map[string]any{ "module_name": moduleName, diff --git a/mdl/backend/mcp/constant.go b/mdl/backend/mcp/constant.go index 870384f92f..abf673a083 100644 --- a/mdl/backend/mcp/constant.go +++ b/mdl/backend/mcp/constant.go @@ -102,8 +102,8 @@ func (b *Backend) liveConstantType(qn string) (string, error) { return k, nil } -// DeleteConstant drops a constant via Concord's delete_document (PED has no delete -// tool). Requires --mcp-concord; errors clearly otherwise. +// DeleteConstant drops a constant via deleteDocument: ped_delete_document where Studio Pro has it (11.16+), else +// Concord (--mcp-concord). func (b *Backend) DeleteConstant(id model.ID) error { c, err := b.GetConstant(id) if err != nil { @@ -113,7 +113,7 @@ func (b *Backend) DeleteConstant(id model.ID) error { if err != nil { return fmt.Errorf("resolve module for constant %q: %w", c.Name, err) } - if err := b.concordDeleteDocument(modName, c.Name); err != nil { + if err := b.deleteDocument(constantDocType, modName, c.Name); err != nil { return err } b.forgetSessionConstant(modName, c.Name) diff --git a/mdl/backend/mcp/delete.go b/mdl/backend/mcp/delete.go new file mode 100644 index 0000000000..a0f63fe2be --- /dev/null +++ b/mdl/backend/mcp/delete.go @@ -0,0 +1,82 @@ +// SPDX-License-Identifier: Apache-2.0 + +package mcp + +import ( + "fmt" + "strings" + "time" +) + +// The document types DROP addresses that no create path names. +const ( + nanoflowDocType = "Microflows$Nanoflow" + pageDocType = "Pages$Page" + businessEventServiceDocType = "BusinessEvents$BusinessEventService" +) + +// deleteDocument drops a standalone named document (constant, enumeration, +// microflow, nanoflow, page, workflow, business-event service). +// +// Studio Pro 11.16 added ped_delete_document, so a server that advertises it is +// asked directly; older servers have no delete tool and need Concord. A native +// failure falls back to Concord when one is connected: on 11.16.0 the tool fails +// for every call made outside Studio Pro's own agent session ("Session +// 'mcp-server' not found"), measured live. +func (b *Backend) deleteDocument(docType, moduleName, docName string) error { + if b.client == nil || !b.client.HasTool("ped_delete_document") { + return b.concordDeleteDocument(moduleName, docName) + } + err := b.pedDeleteDocument(docType, moduleName, docName) + if err == nil || b.concord == nil { + return err + } + if cerr := b.concordDeleteDocument(moduleName, docName); cerr != nil { + return fmt.Errorf("%w; Concord fallback: %v", err, cerr) + } + return nil +} + +// pedDeleteDocument calls ped_delete_document for one document. Its success +// reply has not been observed live, so a reply that is neither PED's usual +// "SUCCESS…" nor an error is settled by looking: the delete happened iff the +// document is gone. A timeout is settled the same way, as for creates. +func (b *Backend) pedDeleteDocument(docType, moduleName, docName string) error { + target := moduleName + "." + docName + res, err := b.client.CallTool("ped_delete_document", map[string]any{ + "documents": []map[string]any{{ + "documentType": docType, + "moduleName": moduleName, + "documentName": docName, + }}, + }) + if isTimeoutErr(err) { + time.Sleep(timeoutVerifyDelay) + if exists, verr := b.pedDocumentExists(moduleName, docType, docName); verr == nil && !exists { + timeoutNotice(fmt.Sprintf("%s %s", docType, target)) + return nil + } + return timeoutUnverified(err, fmt.Sprintf("%s %s", docType, target)) + } + if err != nil { + return err + } + opErr := pedOpError("ped_delete_document", target, res) + if opErr == nil || res.IsError || pedReportsFailure(res.Text) { + return opErr + } + exists, verr := b.pedDocumentExists(moduleName, docType, docName) + if verr != nil { + return fmt.Errorf("%w (and confirming the delete failed: %v)", opErr, verr) + } + if exists { + return opErr + } + return nil +} + +// pedReportsFailure recognises PED's failure texts, which arrive with +// isError:false ("Deleting documents failed (1 of 1):\nERROR: …"). +func pedReportsFailure(text string) bool { + return strings.Contains(text, "failed (") || strings.Contains(text, "\nERROR:") || strings.HasPrefix(strings.TrimSpace(text), "ERROR") +} diff --git a/mdl/backend/mcp/delete_test.go b/mdl/backend/mcp/delete_test.go new file mode 100644 index 0000000000..61206591cb --- /dev/null +++ b/mdl/backend/mcp/delete_test.go @@ -0,0 +1,114 @@ +// SPDX-License-Identifier: Apache-2.0 + +package mcp + +import ( + "encoding/json" + "strings" + "testing" +) + +func TestDeleteDocument_NativeWhenAdvertised(t *testing.T) { + f := newFakePED(t, func(name string, _ map[string]any) (string, bool) { + if name == "ped_delete_document" { + return "SUCCESS: Deleting documents (1)", false + } + return "unexpected", true + }) + f.tools = map[string][]string{"ped_delete_document": {"documents"}} + b := &Backend{client: f.connectClient(t)} // no Concord: native must not need it + + if err := b.deleteDocument(constantDocType, "MyFirstModule", "Gone"); err != nil { + t.Fatalf("deleteDocument: %v", err) + } + call, ok := f.callByName("ped_delete_document") + if !ok { + t.Fatal("ped_delete_document was not called") + } + raw, _ := json.Marshal(call.Args["documents"]) + want := `[{"documentName":"Gone","documentType":"Constants$Constant","moduleName":"MyFirstModule"}]` + if string(raw) != want { + t.Errorf("documents = %s, want %s", raw, want) + } +} + +// The success text of ped_delete_document has not been observed live (the tool +// failed on every 11.16.0 attempt), so an unrecognised reply is settled by +// looking: the delete succeeded iff the document is gone. +func TestDeleteDocument_UnrecognisedReplyVerifiedByLookup(t *testing.T) { + for _, tc := range []struct { + name string + found string + wantErr bool + }{ + {"gone", `{"foundDocuments":[]}`, false}, + {"still there", `{"foundDocuments":[{"qualifiedName":"MyFirstModule.Gone"}]}`, true}, + } { + t.Run(tc.name, func(t *testing.T) { + f := newFakePED(t, func(name string, _ map[string]any) (string, bool) { + if name == "ped_find_document" { + return tc.found, false + } + return "Deleted 1 document.", false + }) + f.tools = map[string][]string{"ped_delete_document": {"documents"}} + b := &Backend{client: f.connectClient(t)} + err := b.deleteDocument(constantDocType, "MyFirstModule", "Gone") + if (err != nil) != tc.wantErr { + t.Fatalf("deleteDocument err = %v, wantErr %v", err, tc.wantErr) + } + }) + } +} + +func TestDeleteDocument_NativeFailureSurfaces(t *testing.T) { + f := newFakePED(t, func(name string, _ map[string]any) (string, bool) { + if name == "ped_find_document" { + return `{"foundDocuments":[{"qualifiedName":"MyFirstModule.Gone"}]}`, false + } + return handlerSessionError, false + }) + f.tools = map[string][]string{"ped_delete_document": {"documents"}} + b := &Backend{client: f.connectClient(t)} + + err := b.deleteDocument(constantDocType, "MyFirstModule", "Gone") + if err == nil { + t.Fatal("a failed native delete must not report success") + } + if !strings.Contains(err.Error(), "Session 'mcp-server' not found") { + t.Errorf("error should carry Studio Pro's message, got: %v", err) + } +} + +func TestDeleteDocument_NativeFailureFallsBackToConcord(t *testing.T) { + ped := newFakePED(t, func(name string, _ map[string]any) (string, bool) { + if name == "ped_find_document" { + return `{"foundDocuments":[{"qualifiedName":"MyFirstModule.Gone"}]}`, false + } + return handlerSessionError, false + }) + ped.tools = map[string][]string{"ped_delete_document": {"documents"}} + concord := newFakePED(t, func(string, map[string]any) (string, bool) { return `{"success":true}`, false }) + b := &Backend{client: ped.connectClient(t), concord: concord.connectClient(t)} + + if err := b.deleteDocument(constantDocType, "MyFirstModule", "Gone"); err != nil { + t.Fatalf("deleteDocument should succeed through Concord: %v", err) + } + if _, ok := concord.callByName("delete_document"); !ok { + t.Fatal("Concord delete_document was not called after the native delete failed") + } +} + +func TestDeleteDocument_NoNativeToolNeedsConcord(t *testing.T) { + f := newFakePED(t, nil) + f.tools = map[string][]string{"ped_create_document": {"documents"}} // pre-11.16 surface + b := &Backend{client: f.connectClient(t)} + + err := b.deleteDocument(constantDocType, "MyFirstModule", "Gone") + if err == nil || !strings.Contains(err.Error(), "--mcp-concord") { + t.Fatalf("without ped_delete_document the error should point at --mcp-concord, got: %v", err) + } + if _, ok := f.callByName("ped_delete_document"); ok { + t.Error("ped_delete_document must not be called when the server does not advertise it") + } +} diff --git a/mdl/backend/mcp/enumeration.go b/mdl/backend/mcp/enumeration.go index 74220766d4..10d02d1b12 100644 --- a/mdl/backend/mcp/enumeration.go +++ b/mdl/backend/mcp/enumeration.go @@ -43,8 +43,8 @@ func (b *Backend) UpdateEnumeration(enum *model.Enumeration) error { return fmt.Errorf("modifying enumeration %q is not yet supported by the MCP backend (create a new one, or edit it in Studio Pro)", enum.Name) } -// DeleteEnumeration drops an enumeration via Concord's delete_document (PED has -// no delete tool). Requires --mcp-concord; errors clearly otherwise. +// DeleteEnumeration drops an enumeration via deleteDocument: ped_delete_document where Studio Pro has it (11.16+), else +// Concord (--mcp-concord). func (b *Backend) DeleteEnumeration(id model.ID) error { enum, err := b.GetEnumeration(id) if err != nil { @@ -54,7 +54,7 @@ func (b *Backend) DeleteEnumeration(id model.ID) error { if err != nil { return fmt.Errorf("resolve module for enumeration %q: %w", enum.Name, err) } - if err := b.concordDeleteDocument(modName, enum.Name); err != nil { + if err := b.deleteDocument(enumerationDocType, modName, enum.Name); err != nil { return err } b.forgetSessionEnum(modName, enum.Name) diff --git a/mdl/backend/mcp/microflow.go b/mdl/backend/mcp/microflow.go index c6c2050f00..705f4a0d5e 100644 --- a/mdl/backend/mcp/microflow.go +++ b/mdl/backend/mcp/microflow.go @@ -372,8 +372,8 @@ func (b *Backend) ListMicroflows() ([]*microflows.Microflow, error) { return out, nil } -// DeleteMicroflow drops a microflow via Concord's delete_document (PED has no -// delete tool). Requires --mcp-concord; errors clearly otherwise. +// DeleteMicroflow drops a microflow via deleteDocument: ped_delete_document where Studio Pro has it (11.16+), else +// Concord (--mcp-concord). func (b *Backend) DeleteMicroflow(id model.ID) error { mf, err := b.GetMicroflow(id) if err != nil { @@ -383,7 +383,7 @@ func (b *Backend) DeleteMicroflow(id model.ID) error { if err != nil { return fmt.Errorf("resolve module for microflow %q: %w", mf.Name, err) } - return b.concordDeleteDocument(modName, mf.Name) + return b.deleteDocument(microflowDocType, modName, mf.Name) } // GetMicroflow resolves by ID, preferring session-created microflows. diff --git a/mdl/backend/mcp/nanoflow.go b/mdl/backend/mcp/nanoflow.go index c4cd07e75e..1a04317ee8 100644 --- a/mdl/backend/mcp/nanoflow.go +++ b/mdl/backend/mcp/nanoflow.go @@ -35,8 +35,8 @@ func (b *Backend) UpdateNanoflow(nf *microflows.Nanoflow) error { return errCreatePathUnbuilt("nanoflow", nf.Name) } -// DeleteNanoflow drops an existing nanoflow via Concord's delete_document (PED has -// no delete tool), like microflows. Requires --mcp-concord. +// DeleteNanoflow drops an existing nanoflow via deleteDocument: ped_delete_document where Studio Pro has it (11.16+), else +// Concord (--mcp-concord). func (b *Backend) DeleteNanoflow(id model.ID) error { nf, err := b.GetNanoflow(id) if err != nil { @@ -46,7 +46,7 @@ func (b *Backend) DeleteNanoflow(id model.ID) error { if err != nil { return fmt.Errorf("resolve module for nanoflow %q: %w", nf.Name, err) } - return b.concordDeleteDocument(modName, nf.Name) + return b.deleteDocument(nanoflowDocType, modName, nf.Name) } // Nanoflow reads delegate to the local reader (nanoflows can't be created over MCP, diff --git a/mdl/backend/mcp/page.go b/mdl/backend/mcp/page.go index 45f2630cfc..99e5ea13d3 100644 --- a/mdl/backend/mcp/page.go +++ b/mdl/backend/mcp/page.go @@ -105,8 +105,8 @@ func (b *Backend) ListPages() ([]*pages.Page, error) { return out, nil } -// DeletePage drops a page via Concord's delete_document (PED has no delete tool). -// Requires --mcp-concord; errors clearly otherwise. +// DeletePage drops a page via deleteDocument: ped_delete_document where Studio Pro has it (11.16+), else +// Concord (--mcp-concord). func (b *Backend) DeletePage(id model.ID) error { page, err := b.GetPage(id) if err != nil { @@ -116,7 +116,7 @@ func (b *Backend) DeletePage(id model.ID) error { if err != nil { return fmt.Errorf("resolve module for page %q: %w", page.Name, err) } - return b.concordDeleteDocument(modName, page.Name) + return b.deleteDocument(pageDocType, modName, page.Name) } // Page-related reads delegate to the local reader (the executor's page builder diff --git a/mdl/backend/mcp/workflow.go b/mdl/backend/mcp/workflow.go index c436122365..08f089d6e1 100644 --- a/mdl/backend/mcp/workflow.go +++ b/mdl/backend/mcp/workflow.go @@ -281,8 +281,8 @@ func (b *Backend) upsertSessionWorkflow(wf *workflows.Workflow) { b.sessionWorkflows = append(b.sessionWorkflows, wf) } -// DeleteWorkflow drops a workflow via Concord's delete_document (PED has no delete -// tool). Requires --mcp-concord. +// DeleteWorkflow drops a workflow via deleteDocument: ped_delete_document where Studio Pro has it (11.16+), else +// Concord (--mcp-concord). func (b *Backend) DeleteWorkflow(id model.ID) error { wf, err := b.GetWorkflow(id) if err != nil { @@ -292,7 +292,7 @@ func (b *Backend) DeleteWorkflow(id model.ID) error { if err != nil { return fmt.Errorf("resolve module for workflow %q: %w", wf.Name, err) } - return b.concordDeleteDocument(modName, wf.Name) + return b.deleteDocument(workflowDocType, modName, wf.Name) } // ListWorkflows merges local (on-disk) workflows with those created this session. From fbc93fcfd04f04fe2407b8b5ca473c010d59a3f1 Mon Sep 17 00:00:00 2001 From: Ako Date: Sun, 1 Nov 2026 14:54:29 +0100 Subject: [PATCH 06/10] fix(widgets): read editorConfig visibility rules in linear time (#1848) The extractor ran end-anchored regexps over everything before each hide call, so its time grew with the square of the editorConfig.js: 0.5 s for the 400 KB one in Maps, on every catalog refresh. Suffix scans replace them; the rules are byte-identical on 256 widgets across five projects. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011oK82ttaGzay5CwiC2V7cf --- mdl/executor/editorconfig_extract.go | 58 ++++++++++++---- mdl/executor/editorconfig_scale_test.go | 91 +++++++++++++++++++++++++ mdl/executor/editorconfig_switch.go | 10 +-- 3 files changed, 140 insertions(+), 19 deletions(-) create mode 100644 mdl/executor/editorconfig_scale_test.go diff --git a/mdl/executor/editorconfig_extract.go b/mdl/executor/editorconfig_extract.go index 2450a8b721..0b54742f9e 100644 --- a/mdl/executor/editorconfig_extract.go +++ b/mdl/executor/editorconfig_extract.go @@ -29,10 +29,22 @@ type editorConfigExtractStats struct { // the group properties that make an authored widget fail CE0463 (upstream #931). var hideCallRE = regexp.MustCompile(`hide(?:Property|Properties|NestedProperties)In\(`) -// hideCallTailRE matches a hide call's NAME where it ends a string, so -// stripGroupSiblings can tell a hide sibling from any other comma-separated +// hideCallTail returns where a hide call's NAME starts when it ends s, or -1, +// so stripGroupSiblings can tell a hide sibling from any other comma-separated // expression. -var hideCallTailRE = regexp.MustCompile(`hide(?:Property|Properties|NestedProperties)In$`) +// +// It is a suffix test rather than a `…$` regexp on purpose: s is everything +// before the call, and an end-anchored regexp still scans it from the start. +// Run once per hide call, that made the extractor quadratic in the size of the +// editorConfig.js — 0.5 s for the 400 KB one in Maps, on every refresh (#1848). +func hideCallTail(s string) int { + for _, name := range [...]string{"hidePropertyIn", "hidePropertiesIn", "hideNestedPropertiesIn"} { + if strings.HasSuffix(s, name) { + return len(s) - len(name) + } + } + return -1 +} // aliasAssignRE finds `IDENT=OBJ.PROP` (a `var x=e.selection`-style alias). // Resolution is scoped to the enclosing function body (see enclosingAliases), @@ -684,10 +696,28 @@ func enclosingOpener(js string, pos int) int { return -1 } -// nsPrefixRE matches the widget-editor namespace prefix before a hide call — the -// minifier names it per widget (`_.`, `D.`, `M.`, `j.`, `A.`, …), so it must be -// matched generically rather than hard-coded to `_.`. -var nsPrefixRE = regexp.MustCompile(`[A-Za-z_$][\w$]*\.$`) +// nsPrefix returns where the widget-editor namespace prefix before a hide call +// starts when it ends s (`.`), or -1. The minifier names it per widget +// (`_.`, `D.`, `M.`, `j.`, `A.`, …), so it must be matched generically rather +// than hard-coded to `_.`. A suffix scan, not a regexp, for the reason given +// at hideCallTail. +func nsPrefix(s string) int { + if !strings.HasSuffix(s, ".") { + return -1 + } + start := len(s) - 1 + for start > 0 && isIdentByte(s[start-1]) { + start-- + } + // An identifier does not start with a digit: skip leading ones. + for start < len(s)-1 && s[start] >= '0' && s[start] <= '9' { + start++ + } + if start == len(s)-1 { + return -1 + } + return start +} // parseGuard reads the guard expression immediately preceding a hide call and // converts it to a WidgetVisibilityCondition. callStart points at the hide @@ -704,8 +734,8 @@ var nsPrefixRE = regexp.MustCompile(`[A-Za-z_$][\w$]*\.$`) func parseGuard(js string, callStart int, itemIdent string) (types.WidgetVisibilityCondition, string, bool, bool, []types.WidgetVisibilityCondition) { pre := strings.TrimRight(js[:callStart], " ") // Strip the widget-editor namespace prefix (any `.`, not just `_.`). - if loc := nsPrefixRE.FindStringIndex(pre); loc != nil { - pre = pre[:loc[0]] + if at := nsPrefix(pre); at >= 0 { + pre = pre[:at] } pre = strings.TrimRight(pre, " ") // Strip an optional grouping paren: `cond && ( hide(...), … )` groups several @@ -1716,13 +1746,13 @@ func stripGroupSiblingsRaw(pre string) string { return pre } head := strings.TrimRight(body[:open], " ") - loc := hideCallTailRE.FindStringIndex(head) - if loc == nil || loc[1] != len(head) { + at := hideCallTail(head) + if at < 0 { return pre // not a hide call — do not cross this comma } - pre = strings.TrimRight(head[:loc[0]], " ") - if l := nsPrefixRE.FindStringIndex(pre); l != nil { - pre = strings.TrimRight(pre[:l[0]], " ") + pre = strings.TrimRight(head[:at], " ") + if l := nsPrefix(pre); l >= 0 { + pre = strings.TrimRight(pre[:l], " ") } } } diff --git a/mdl/executor/editorconfig_scale_test.go b/mdl/executor/editorconfig_scale_test.go new file mode 100644 index 0000000000..bcb48b06c0 --- /dev/null +++ b/mdl/executor/editorconfig_scale_test.go @@ -0,0 +1,91 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "fmt" + "os" + "regexp" + "strings" + "testing" + "time" +) + +// The end-anchored regexps the suffix scans replaced (#1848). Kept here as the +// reference the scans must agree with. +var ( + refHideCallTailRE = regexp.MustCompile(`hide(?:Property|Properties|NestedProperties)In$`) + refNsPrefixRE = regexp.MustCompile(`[A-Za-z_$][\w$]*\.$`) +) + +func TestSuffixScansMatchTheRegexps(t *testing.T) { + inputs := []string{ + "", ".", "a.", "_.", "$.", "9.", "a9.", "9a.", "x 99a.", "e.foo&&_.", "M.", "ab.cd.", + "hidePropertyIn", "_.hidePropertiesIn", "x,hideNestedPropertiesIn", "hidePropertyInx", + "ehidePropertyIn", "hidePropertyIn ", "a.b", + } + js, err := os.ReadFile("testdata/datagrid.editorConfig.js") + if err != nil { + t.Fatal(err) + } + // Every prefix of a real editorConfig that ends where a scan is applied. + for i := range js { + if js[i] == '.' || js[i] == 'n' { + inputs = append(inputs, string(js[:i+1])) + } + } + for _, in := range inputs { + want := -1 + if loc := refNsPrefixRE.FindStringIndex(in); loc != nil { + want = loc[0] + } + if got := nsPrefix(in); got != want { + t.Fatalf("nsPrefix(%q) = %d, regexp says %d", tail(in), got, want) + } + want = -1 + if loc := refHideCallTailRE.FindStringIndex(in); loc != nil { + want = loc[0] + } + if got := hideCallTail(in); got != want { + t.Fatalf("hideCallTail(%q) = %d, regexp says %d", tail(in), got, want) + } + } +} + +// The extractor ran an end-anchored regexp over everything before each hide +// call, so its time grew with the square of the editorConfig's size: 0.5 s +// for Maps' 400 KB one, on every refresh (#1848). Growing the input 8× must +// not grow the time anywhere near 64×. +// +// The input is shaped like minified editorConfig: many small functions with a +// hide each. Work per hide that is bounded by its own function (the alias scan) +// stays out of the measurement; work over the whole file does not. +func TestExtractVisibilityRulesIsLinear(t *testing.T) { + gen := func(n int) string { + var b strings.Builder + for i := range n { + fmt.Fprintf(&b, `function g%d(e,t){var r=e.mode;return "v%d"===r&&_.hidePropertyIn(t,e,"p%d"),t}`, i, i, i) + } + return b.String() + } + timeOf := func(js string) time.Duration { + best := time.Duration(1 << 62) + for range 3 { + start := time.Now() + extractVisibilityRulesFromJS(js) + best = min(best, time.Since(start)) + } + return best + } + small, large := timeOf(gen(300)), timeOf(gen(2400)) + if large > 20*small && large > 200*time.Millisecond { + t.Errorf("8× the input took %v against %v: the extractor is superlinear again", large, small) + } +} + +func tail(s string) string { + if len(s) > 40 { + return "…" + s[len(s)-40:] + } + return s +} diff --git a/mdl/executor/editorconfig_switch.go b/mdl/executor/editorconfig_switch.go index 056bff4dd5..393f7bcb09 100644 --- a/mdl/executor/editorconfig_switch.go +++ b/mdl/executor/editorconfig_switch.go @@ -322,17 +322,17 @@ func onlyHideStatementsBefore(prefix string) bool { return false } name := strings.TrimRight(call[:open], " ") - loc := hideCallTailRE.FindStringIndex(name) - if loc == nil || loc[1] != len(name) { + at := hideCallTail(name) + if at < 0 { return false } - p = trimNamespacePrefix(strings.TrimRight(name[:loc[0]], " ")) + p = trimNamespacePrefix(strings.TrimRight(name[:at], " ")) } } func trimNamespacePrefix(p string) string { - if loc := nsPrefixRE.FindStringIndex(p); loc != nil { - return strings.TrimRight(p[:loc[0]], " ") + if at := nsPrefix(p); at >= 0 { + return strings.TrimRight(p[:at], " ") } return p } From f9c6fd45094f02e44f78941e28b15988560e3c00 Mon Sep 17 00:00:00 2001 From: Ako Date: Sun, 1 Nov 2026 14:54:30 +0100 Subject: [PATCH 07/10] fix(widgets): name colliding widgets apart and skip an unchanged widgets/ (#1848) KeyboardShortcut.mpk and itvisors.KeyboardShortcut.mpk both derived KEYBOARDSHORTCUT and rewrote one .def.json in turn: one widget had no definition, and every refresh reported "2 refreshed" and regenerated the widget docs. Names are now assigned over the whole project (the pluggable widget keeps the keyword), and the catalog pairs definitions by widget ID. A refresh also re-read every .mpk although nothing had changed. It now records the stats of the packages and definitions, and skips the step when they and the mxcli build are unchanged. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011oK82ttaGzay5CwiC2V7cf --- ...01-widget-defs-reparsed-every-refresh.json | 1 + CHANGELOG.md | 1 + docs-site/src/guides/pluggable-widgets.md | 10 + mdl/catalog/builder_widget_definitions.go | 51 ++- .../builder_widget_definitions_test.go | 107 +++++ mdl/executor/widget_defs.go | 402 +++++++++++++----- mdl/executor/widget_defs_refresh_test.go | 170 ++++++++ 7 files changed, 628 insertions(+), 114 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/mdl-executor/2026-11-01-widget-defs-reparsed-every-refresh.json create mode 100644 mdl/catalog/builder_widget_definitions_test.go create mode 100644 mdl/executor/widget_defs_refresh_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-11-01-widget-defs-reparsed-every-refresh.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-11-01-widget-defs-reparsed-every-refresh.json new file mode 100644 index 0000000000..3c94d85d33 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-11-01-widget-defs-reparsed-every-refresh.json @@ -0,0 +1 @@ +{"area": "mdl/executor", "date": "2026-11-01", "symptom": "refresh catalog after one model edit spent about 3 s of 5 s on widget definitions (Windows, FactoryManagement): `Widget definitions: 0 new, 2 refreshed` on every run, and the widget docs regenerated each time.", "cause": "Three causes stacked. KeyboardShortcut.mpk and itvisors.KeyboardShortcut.mpk both derived KEYBOARDSHORTCUT and rewrote one .def.json in turn, so the refresh always found drift and regenerated docs (which re-ran the whole refresh). The editorConfig visibility extractor ran end-anchored regexps (`…$`) over everything before each hide call, quadratic in the file: 0.5 s for Maps' 400 KB editorConfig.js. And nothing recorded that widgets/ was unchanged, so every refresh re-read every .mpk.", "file": "mdl/executor/widget_defs.go, mdl/executor/editorconfig_extract.go", "insight": "Profile before caching: the issue blamed unzipping, but parsing all 54 .mpk took 45 ms; one 400 KB file in the extractor took 535 ms. Go's regexp has no backward search, so a `$`-anchored pattern on a growing prefix scans the whole prefix every time; use a suffix scan. A name derived from an ID must be assigned over the whole set, and every consumer (defs, docs, catalog) must look up by ID, not re-derive the file name.", "issue": "ako/mxcli#1848"} diff --git a/CHANGELOG.md b/CHANGELOG.md index 619faabcc3..f7dc8e6079 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **Refreshing the catalog after a model edit no longer spends seconds on widget definitions** (ako/mxcli#1848) — `refresh catalog` re-read every widget package on every run, and on FactoryManagement it took about 3 s of a 5 s refresh on Windows. The step is now skipped when no `.mpk` or `.def.json` file has changed since the last refresh by the same mxcli build, and reading a widget's editor rules is no longer quadratic in the size of its `editorConfig.js` (Maps' took 0.5 s and now takes 15 ms, with identical rules on 256 widgets). Two widgets that derive the same keyword, such as `KeyboardShortcut.mpk` and `itvisors.KeyboardShortcut.mpk`, no longer overwrite each other's definition: the pluggable one keeps `KEYBOARDSHORTCUT` and the other becomes `CUSTOMKEYBOARDSHORTCUT`. Measured on a Mac, a refresh after one edit went from 2.8–3.2 s to 1.0–1.4 s. - **MPR012 follows the app's Web UI client** (ako/mxcli#1287, mendixlabs/mxcli#1226) — a Mendix 11 app can still run the classic client, where the legacy static and dynamic image widgets build clean, but MPR012 warned on each of them that "mxbuild reports CE0582". On the classic client or in migration mode it is now an info-level note that the widget needs converting before the app switches; under the React client it stays a warning. - **Every `mxcli init --tool ` installs the bundled lint rules, and `lint` says when it runs without them** (ako/mxcli#1292, mendixlabs/mxcli#1242) — only `claude` and `opencode` wrote `.claude/lint-rules/`, so `init --tool cursor` (or aider, continue, copilot, vibe, windsurf) left `mxcli lint` running the built-in Go rules alone, silently. `lint` and `report` now print a warning when no `.claude/lint-rules/` is found, naming how many rules run instead. - **`show references to` an entity, attribute or association lists the message definitions that expose it** (#1781) — a message definition (the pre-11.15 collection, `MESSAGE_DEFINITION_COLLECTION`, and the 11.15+ document, `MESSAGE_DEFINITION`) was not walked for references, so impact analysis called an exposed entity or attribute unused. The entity edges are `mapping`, the attribute and association edges `member`, as for an import or export mapping. diff --git a/docs-site/src/guides/pluggable-widgets.md b/docs-site/src/guides/pluggable-widgets.md index 4374598ddb..041e0f31b7 100644 --- a/docs-site/src/guides/pluggable-widgets.md +++ b/docs-site/src/guides/pluggable-widgets.md @@ -254,4 +254,14 @@ mxcli widget extract --mpk widgets/MyWidget.mpk # writes a .def.json mxcli widget init -p app.mpr # extract defs for all project widgets ``` +A widget's MDL keyword is the last segment of its ID (`com.mendix.widget.web.combobox.Combobox` +→ `COMBOBOX`). When two installed widgets derive the same keyword, the pluggable one keeps it +and the other is named with its vendor in front (`itvisors.keyboardshortcut.KeyboardShortcut` +→ `ITVISORSKEYBOARDSHORTCUT`), or with `CUSTOM` in front for a legacy custom widget +(`KeyboardShortcut.widget.KeyboardShortcut` → `CUSTOMKEYBOARDSHORTCUT`). `mxcli widget list` +shows which name each widget got. + +`refresh catalog` refreshes these definitions too, but skips that step when no `.mpk` or +`.def.json` file has changed since the last refresh by the same mxcli build. + See [Marketplace Content](marketplace.md) for downloading and installing packages. diff --git a/mdl/catalog/builder_widget_definitions.go b/mdl/catalog/builder_widget_definitions.go index 7de0d33e89..d7ac669b19 100644 --- a/mdl/catalog/builder_widget_definitions.go +++ b/mdl/catalog/builder_widget_definitions.go @@ -107,6 +107,7 @@ func (b *Builder) buildWidgetDefinitions() error { if err != nil { return fmt.Errorf("scan widgets directory: %w", err) } + defs := defFilesByWidgetID(defsDir) for _, mpkPath := range matches { mpkDefs, err := mpk.ParseMPKAll(mpkPath) if err != nil { @@ -114,9 +115,19 @@ func (b *Builder) buildWidgetDefinitions() error { } // A single .mpk can bundle many widgets (e.g. Charts.mpk). for _, mpkDef := range mpkDefs { + // The definition is found by widget ID, not by a file name + // derived from it: two widgets can derive one name, and the + // definition generator then names one of them differently + // (#1848). mdlName := strings.ToUpper(lastSegment(mpkDef.ID)) defPath := filepath.Join(defsDir, strings.ToLower(mdlName)+".def.json") - defFields := loadDefFields(defPath) + var defFields defFields + if d, ok := defs[mpkDef.ID]; ok { + defPath, defFields = d.path, d.fields + if d.mdlName != "" { + mdlName = strings.ToUpper(d.mdlName) + } + } kind := "custom" if mpkDef.IsPluggable { kind = "pluggable" @@ -167,15 +178,38 @@ type defFields struct { ObjectLists []WidgetDefinitionSlotMeta } -// loadDefFields reads a .def.json file and extracts its childSlots and -// objectLists. Returns empty values if the file is missing or malformed. -func loadDefFields(path string) defFields { - var out defFields +// projectDef is one .def.json of the project. +type projectDef struct { + path string + mdlName string + fields defFields +} + +// defFilesByWidgetID reads every .def.json in dir and indexes it by the widget +// ID it defines. +func defFilesByWidgetID(dir string) map[string]projectDef { + out := map[string]projectDef{} + files, _ := filepath.Glob(filepath.Join(dir, "*.def.json")) + for _, path := range files { + id, mdlName, fields := loadDefFields(path) + if id != "" { + out[id] = projectDef{path: path, mdlName: mdlName, fields: fields} + } + } + return out +} + +// loadDefFields reads a .def.json file and extracts the widget it defines, its +// MDL name, and its childSlots and objectLists. Returns empty values if the +// file is missing or malformed. +func loadDefFields(path string) (widgetID, mdlName string, out defFields) { data, err := os.ReadFile(path) if err != nil { - return out + return } var parsed struct { + WidgetID string `json:"widgetId"` + MDLName string `json:"mdlName"` ChildSlots []struct { PropertyKey string `json:"propertyKey"` MDLContainer string `json:"mdlContainer"` @@ -186,8 +220,9 @@ func loadDefFields(path string) defFields { } `json:"objectLists"` } if jsonErr := json.Unmarshal(data, &parsed); jsonErr != nil { - return out + return } + widgetID, mdlName = parsed.WidgetID, parsed.MDLName for _, s := range parsed.ChildSlots { out.ChildSlots = append(out.ChildSlots, WidgetDefinitionSlotMeta{ PropertyKey: s.PropertyKey, @@ -200,7 +235,7 @@ func loadDefFields(path string) defFields { MdlKeyword: ol.MDLContainer, }) } - return out + return } // mpkProperties translates mpk.PropertyDef into the catalog's prop meta. diff --git a/mdl/catalog/builder_widget_definitions_test.go b/mdl/catalog/builder_widget_definitions_test.go new file mode 100644 index 0000000000..3a671a3047 --- /dev/null +++ b/mdl/catalog/builder_widget_definitions_test.go @@ -0,0 +1,107 @@ +// SPDX-License-Identifier: Apache-2.0 + +package catalog + +import ( + "archive/zip" + "io" + "os" + "path/filepath" + "testing" + + "github.com/mendixlabs/mxcli/mdl/backend/mock" +) + +// #1848: two widgets that derive one MDL name get definitions under different +// names. The catalog looked a definition up by the name derived from the +// widget ID, so both widgets were paired with the same file. +func TestWidgetDefinitionsPairsDefinitionsByWidgetID(t *testing.T) { + dir := t.TempDir() + const legacy = "KeyboardShortcut.widget.KeyboardShortcut" + const pluggable = "itvisors.keyboardshortcut.KeyboardShortcut" + writeCatalogTestMPK(t, filepath.Join(dir, "widgets", "KeyboardShortcut.mpk"), legacy, false) + writeCatalogTestMPK(t, filepath.Join(dir, "widgets", "itvisors.KeyboardShortcut.mpk"), pluggable, true) + defs := filepath.Join(dir, ".mxcli", "widgets") + writeCatalogTestFile(t, filepath.Join(defs, "keyboardshortcut.def.json"), + `{"widgetId":"`+pluggable+`","mdlName":"KEYBOARDSHORTCUT","childSlots":[{"propertyKey":"content","mdlContainer":"CONTENT"}]}`) + writeCatalogTestFile(t, filepath.Join(defs, "customkeyboardshortcut.def.json"), + `{"widgetId":"`+legacy+`","mdlName":"CUSTOMKEYBOARDSHORTCUT"}`) + + cat, err := New() + if err != nil { + t.Fatal(err) + } + defer cat.Close() + tx, err := cat.CatalogDB().Begin() + if err != nil { + t.Fatal(err) + } + b := &Builder{ + catalog: cat, + reader: &mock.MockBackend{PathFunc: func() string { return filepath.Join(dir, "App.mpr") }}, + snapshot: &Snapshot{ID: "snap"}, + tx: wrapTx(tx), + } + if err := b.buildWidgetDefinitions(); err != nil { + t.Fatalf("buildWidgetDefinitions: %v", err) + } + if err := tx.Commit(); err != nil { + t.Fatal(err) + } + + names := queryStrings(t, cat, `SELECT WidgetId, MdlName FROM widget_definitions_data`) + paths := queryStrings(t, cat, `SELECT WidgetId, DefPath FROM widget_definitions_data`) + slots := queryStrings(t, cat, `SELECT WidgetId, CAST(ChildSlotCount AS TEXT) FROM widget_definitions_data`) + for id, want := range map[string][3]string{ + pluggable: {"KEYBOARDSHORTCUT", ".mxcli/widgets/keyboardshortcut.def.json", "1"}, + legacy: {"CUSTOMKEYBOARDSHORTCUT", ".mxcli/widgets/customkeyboardshortcut.def.json", "0"}, + } { + if names[id] != want[0] || filepath.ToSlash(paths[id]) != want[1] || slots[id] != want[2] { + t.Errorf("%s: name %q, def %q, child slots %q; want %v", id, names[id], paths[id], slots[id], want) + } + } +} + +func writeCatalogTestMPK(t *testing.T, path, widgetID string, pluggable bool) { + t.Helper() + plugin := "" + if pluggable { + plugin = ` pluginWidget="true"` + } + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + f, err := os.Create(path) + if err != nil { + t.Fatal(err) + } + zw := zip.NewWriter(f) + for name, body := range map[string]string{ + "package.xml": ``, + "W.xml": `WCaption`, + } { + w, err := zw.Create(name) + if err != nil { + t.Fatal(err) + } + if _, err := io.WriteString(w, body); err != nil { + t.Fatal(err) + } + } + if err := zw.Close(); err != nil { + t.Fatal(err) + } + if err := f.Close(); err != nil { + t.Fatal(err) + } +} + +func writeCatalogTestFile(t *testing.T, path, body string) { + t.Helper() + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte(body), 0o644); err != nil { + t.Fatal(err) + } +} diff --git a/mdl/executor/widget_defs.go b/mdl/executor/widget_defs.go index 54db7ce262..6fc855a0f7 100644 --- a/mdl/executor/widget_defs.go +++ b/mdl/executor/widget_defs.go @@ -19,8 +19,10 @@ import ( "fmt" "io" "log" + "maps" "os" "path/filepath" + "sort" "strings" "github.com/mendixlabs/mxcli/mdl/types" @@ -34,6 +36,9 @@ type WidgetDefRefreshStats struct { Refreshed int // content drifted — overwrote stale def UpToDate int // content matched — no write Skipped int // skipped (built-in or unparseable mpk) + // Unchanged reports that nothing was looked at: widgets/ and the + // definitions are as the same mxcli build left them (#1848). + Unchanged bool } // RefreshWidgetDefinitions scans projectDir/widgets/ for .mpk files and @@ -64,84 +69,275 @@ func RefreshWidgetDefinitions(projectPath string, force bool, output io.Writer) return stats, fmt.Errorf("failed to create output directory: %w", err) } + if !force && widgetStateUnchanged(projectDir) { + stats.Unchanged = true + return stats, nil + } + // Built-in registry — definitions hand-crafted in sdk/widgets/definitions/ // (COMBOBOX, GALLERY, DATAGRID, filters, …). Skip those when extracting // from the project; the built-in def overrides any .mpk-derived one. builtinRegistry, _ := NewWidgetRegistry() - for _, mpkPath := range matches { - mpkDefs, err := mpk.ParseMPKAll(mpkPath) + widgets, unparseable := scanProjectWidgets(matches) + stats.Skipped += unparseable + + for _, w := range widgets { + mpkPath, mpkDef, mdlName := w.mpkPath, w.def, w.mdlName + filename := strings.ToLower(mdlName) + ".def.json" + outPath := filepath.Join(outputDir, filename) + + if builtinRegistry != nil { + if _, ok := builtinRegistry.GetByWidgetID(mpkDef.ID); ok { + stats.Skipped++ + continue + } + } + + defJSON := GenerateDefJSON(mpkDef, mdlName) + // Lift property-visibility rules from the widget's editorConfig.js + // (#574 Phase 2) so the generated .def.json carries the version- + // specific applicability logic. Merge with the hand-transcribed table + // rather than replace it: the static extractor skips some guard + // shapes, so the hand-authored fallback fills the keys it misses. + // Extracted rules win on conflict (they're version-specific). + if rules := extractVisibilityRulesFromMPK(mpkPath, mpkDef.ID); len(rules) > 0 { + defJSON.PropertyVisibility = mergeVisibilityRules(rules, widgetVisibilityRules[mpkDef.ID]) + } + freshData, err := json.MarshalIndent(defJSON, "", " ") if err != nil { - log.Printf("warning: skipping %s: %v", filepath.Base(mpkPath), err) + log.Printf("warning: skipping %s: %v", mpkDef.ID, err) stats.Skipped++ continue } - - // A single .mpk can bundle many widgets (e.g. Charts.mpk); emit a def - // for each, not just the first. - for _, mpkDef := range mpkDefs { - mdlName := DeriveMDLName(mpkDef.ID) - filename := strings.ToLower(mdlName) + ".def.json" - outPath := filepath.Join(outputDir, filename) - - if builtinRegistry != nil { - if _, ok := builtinRegistry.GetByWidgetID(mpkDef.ID); ok { - stats.Skipped++ - continue - } + freshData = append(freshData, '\n') + + existingData, existsErr := os.ReadFile(outPath) + switch { + case existsErr != nil: + stats.Extracted++ + case bytes.Equal(existingData, freshData): + if force { + stats.Refreshed++ + } else { + stats.UpToDate++ + continue } + default: + stats.Refreshed++ + } - defJSON := GenerateDefJSON(mpkDef, mdlName) - // Lift property-visibility rules from the widget's editorConfig.js - // (#574 Phase 2) so the generated .def.json carries the version- - // specific applicability logic. Merge with the hand-transcribed table - // rather than replace it: the static extractor skips some guard - // shapes, so the hand-authored fallback fills the keys it misses. - // Extracted rules win on conflict (they're version-specific). - if rules := extractVisibilityRulesFromMPK(mpkPath, mpkDef.ID); len(rules) > 0 { - defJSON.PropertyVisibility = mergeVisibilityRules(rules, widgetVisibilityRules[mpkDef.ID]) + if err := writeDefinitionFile(outPath, freshData); err != nil { + return stats, fmt.Errorf("failed to write %s: %w", outPath, err) + } + if output != nil { + kind := "custom" + if mpkDef.IsPluggable { + kind = "pluggable" } - freshData, err := json.MarshalIndent(defJSON, "", " ") - if err != nil { - log.Printf("warning: skipping %s: %v", mpkDef.ID, err) - stats.Skipped++ + marker := "+" + if existsErr == nil { + marker = "~" + } + fmt.Fprintf(output, " %s %-12s %-20s %s\n", marker, kind, mdlName, mpkDef.ID) + } + } + + saveWidgetState(projectDir) + return stats, nil +} + +// projectWidget is one widget of a project's widgets/*.mpk, with the MDL name +// its definition is written under. +type projectWidget struct { + mpkPath string + def *mpk.WidgetDefinition + mdlName string +} + +// scanProjectWidgets parses the given .mpk files and names every widget in +// them. A package bundles one widget or many (Charts.mpk). The second count +// is the packages that could not be parsed. +// +// Names are assigned over the whole project rather than per widget, because +// two widget IDs can derive the same name: KeyboardShortcut.mpk, a legacy +// custom widget, and itvisors.KeyboardShortcut.mpk, its pluggable successor, +// are both KEYBOARDSHORTCUT. Each overwrote the other's definition on every +// refresh, so one widget had none and every refresh rewrote both (#1848). +// Everything that maps a widget to its definition file must name it here. +func scanProjectWidgets(mpkPaths []string) ([]projectWidget, int) { + var widgets []projectWidget + unparseable := 0 + seenID := map[string]bool{} + for _, mpkPath := range mpkPaths { + defs, err := mpk.ParseMPKAll(mpkPath) + if err != nil { + log.Printf("warning: skipping %s: %v", filepath.Base(mpkPath), err) + unparseable++ + continue + } + for _, def := range defs { + // The same widget installed twice is one widget. + if seenID[def.ID] { continue } - freshData = append(freshData, '\n') - - existingData, existsErr := os.ReadFile(outPath) - switch { - case existsErr != nil: - stats.Extracted++ - case bytes.Equal(existingData, freshData): - if force { - stats.Refreshed++ - } else { - stats.UpToDate++ - continue - } - default: - stats.Refreshed++ + seenID[def.ID] = true + widgets = append(widgets, projectWidget{mpkPath: mpkPath, def: def, mdlName: DeriveMDLName(def.ID)}) + } + } + + byName := map[string][]int{} + taken := map[string]bool{} + for i, w := range widgets { + byName[w.mdlName] = append(byName[w.mdlName], i) + taken[w.mdlName] = true + } + names := make([]string, 0, len(byName)) + for name, idx := range byName { + if len(idx) > 1 { + names = append(names, name) + } + } + sort.Strings(names) + for _, name := range names { + idx := byName[name] + // The pluggable widget keeps the keyword (the one people build with + // today), then the lowest ID, so the outcome does not depend on the + // order the packages are read in. + sort.Slice(idx, func(a, b int) bool { + wa, wb := widgets[idx[a]].def, widgets[idx[b]].def + if wa.IsPluggable != wb.IsPluggable { + return wa.IsPluggable } + return wa.ID < wb.ID + }) + for _, i := range idx[1:] { + widgets[i].mdlName = qualifiedMDLName(widgets[i].def, taken) + taken[widgets[i].mdlName] = true + log.Printf("info: widgets %s and %s both derive %s; %s is registered as %s", + widgets[idx[0]].def.ID, widgets[i].def.ID, name, widgets[i].def.ID, widgets[i].mdlName) + } + } + return widgets, unparseable +} + +// genericIDSegments are widget-ID segments that say nothing about whose widget +// it is, so they do not make a name distinct. +var genericIDSegments = map[string]bool{"com": true, "mendix": true, "widget": true, "widgets": true, "web": true, "native": true} + +// qualifiedMDLName names a widget whose derived name another widget keeps: its +// vendor segment in front (itvisors.keyboardshortcut.KeyboardShortcut → +// ITVISORSKEYBOARDSHORTCUT), CUSTOM in front for a legacy custom widget with no +// such segment (KeyboardShortcut.widget.KeyboardShortcut → +// CUSTOMKEYBOARDSHORTCUT), and a number behind as the last resort. +func qualifiedMDLName(def *mpk.WidgetDefinition, taken map[string]bool) string { + base := DeriveMDLName(def.ID) + parts := strings.Split(def.ID, ".") + last := parts[len(parts)-1] + var candidates []string + for _, seg := range parts[:len(parts)-1] { + seg = identifierPart(seg) + if seg == "" || strings.EqualFold(seg, last) || genericIDSegments[strings.ToLower(seg)] { + continue + } + candidates = append(candidates, strings.ToUpper(seg)+base) + break + } + if !def.IsPluggable { + candidates = append(candidates, "CUSTOM"+base) + } + for _, c := range candidates { + if !taken[c] { + return c + } + } + for n := 2; ; n++ { + if c := fmt.Sprintf("%s%d", base, n); !taken[c] { + return c + } + } +} + +// identifierPart keeps the letters, digits and underscores of s. +func identifierPart(s string) string { + return strings.Map(func(r rune) rune { + if r == '_' || (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9') { + return r + } + return -1 + }, s) +} - if err := writeDefinitionFile(outPath, freshData); err != nil { - return stats, fmt.Errorf("failed to write %s: %w", outPath, err) +// widgetStateFile records what the last refresh saw, beside the definitions. +// It does not end in .def.json, so no definition scan reads it. +const widgetStateFile = "refresh-state.json" + +// widgetStateFingerprint names the running mxcli build. A refresh is skipped +// only for the build that wrote the state: another may generate different +// definitions from the same packages. "" never skips. Tests replace it. +var widgetStateFingerprint = sourceFingerprint + +type widgetRefreshState struct { + Fingerprint string `json:"fingerprint"` + Files map[string]string `json:"files"` // project-relative path → size|mtime +} + +// widgetFiles stats every widgets/*.mpk and .mxcli/widgets/*.def.json. A +// stat, unlike a read, does not make Windows Defender scan the file. +func widgetFiles(projectDir string) map[string]string { + files := map[string]string{} + for _, pattern := range []string{ + filepath.Join(projectDir, "widgets", "*.mpk"), + filepath.Join(projectDir, ".mxcli", "widgets", "*.def.json"), + } { + matches, _ := filepath.Glob(pattern) + for _, m := range matches { + fi, err := os.Stat(m) + if err != nil { + continue } - if output != nil { - kind := "custom" - if mpkDef.IsPluggable { - kind = "pluggable" - } - marker := "+" - if existsErr == nil { - marker = "~" - } - fmt.Fprintf(output, " %s %-12s %-20s %s\n", marker, kind, mdlName, mpkDef.ID) + rel, err := filepath.Rel(projectDir, m) + if err != nil { + rel = m } + files[filepath.ToSlash(rel)] = fmt.Sprintf("%d|%d", fi.Size(), fi.ModTime().UnixNano()) } } + return files +} - return stats, nil +// widgetStateUnchanged reports whether the packages and definitions are +// exactly as the last refresh by this mxcli build left them, so refreshing +// again would only re-read every .mpk to rewrite nothing. That is the normal +// case for a refresh after a model edit (#1848). +func widgetStateUnchanged(projectDir string) bool { + fp := widgetStateFingerprint() + if fp == "" { + return false + } + data, err := os.ReadFile(filepath.Join(projectDir, ".mxcli", "widgets", widgetStateFile)) + if err != nil { + return false + } + var prev widgetRefreshState + if json.Unmarshal(data, &prev) != nil || prev.Fingerprint != fp { + return false + } + return maps.Equal(prev.Files, widgetFiles(projectDir)) +} + +// saveWidgetState records what this refresh left behind. Best-effort: without +// it the next refresh just does the work again. +func saveWidgetState(projectDir string) { + fp := widgetStateFingerprint() + if fp == "" { + return + } + data, err := json.Marshal(widgetRefreshState{Fingerprint: fp, Files: widgetFiles(projectDir)}) + if err != nil { + return + } + _ = writeDefinitionFile(filepath.Join(projectDir, ".mxcli", "widgets", widgetStateFile), data) } // writeDefinitionFile replaces a .def.json so that a reader sees the old @@ -751,60 +947,54 @@ func RegenerateWidgetDocs(projectPath string) (int, error) { var widgetNames []string var withoutRouting []string - for _, mpkPath := range matches { - // A bundled .mpk (e.g. Charts.mpk) contains many widgetFiles; ParseMPK - // returns only the first, so docs previously omitted all but one chart - // widget. ParseMPKAll documents every widget in the bundle (issue #679 - // applied the same fix to the def-generation loop above; bug 9a). - mpkDefs, err := mpk.ParseMPKAll(mpkPath) - if err != nil { - continue - } - for _, mpkDef := range mpkDefs { - mdlName := DeriveMDLName(mpkDef.ID) - filename := strings.ToLower(mdlName) + ".md" - - // Load the matching .def.json, falling back to the embedded definition. - // - // Built-in widgets like COMBOBOX and GALLERY have hand-crafted - // definitions in sdk/widgets/definitions/ and are deliberately not - // extracted per-project, so their .def.json never exists. Reading only - // from disk therefore documented nine widgets — combobox, gallery, the - // four data-grid filters, dropdownsort, image, barcodescanner — with an - // MDL example missing every child block, as if extraction had failed. - // The registry has had their routing all along. - var def *WidgetDefinition - defPath := filepath.Join(defsDir, strings.ToLower(mdlName)+".def.json") - if data, err := os.ReadFile(defPath); err == nil { - def = &WidgetDefinition{} - if jsonErr := json.Unmarshal(data, def); jsonErr != nil { - def = nil - } + // A bundled .mpk (e.g. Charts.mpk) contains many widgetFiles; every one is + // documented, not just the first (issue #679, bug 9a). The names are the + // ones the definitions were written under. + widgets, _ := scanProjectWidgets(matches) + for _, w := range widgets { + mpkDef, mdlName := w.def, w.mdlName + filename := strings.ToLower(mdlName) + ".md" + + // Load the matching .def.json, falling back to the embedded definition. + // + // Built-in widgets like COMBOBOX and GALLERY have hand-crafted + // definitions in sdk/widgets/definitions/ and are deliberately not + // extracted per-project, so their .def.json never exists. Reading only + // from disk therefore documented nine widgets — combobox, gallery, the + // four data-grid filters, dropdownsort, image, barcodescanner — with an + // MDL example missing every child block, as if extraction had failed. + // The registry has had their routing all along. + var def *WidgetDefinition + defPath := filepath.Join(defsDir, strings.ToLower(mdlName)+".def.json") + if data, err := os.ReadFile(defPath); err == nil { + def = &WidgetDefinition{} + if jsonErr := json.Unmarshal(data, def); jsonErr != nil { + def = nil } - if def == nil && registry != nil { - if embedded, ok := registry.Get(mdlName); ok { - def = embedded - } + } + if def == nil && registry != nil { + if embedded, ok := registry.Get(mdlName); ok { + def = embedded } + } - doc := widgetDocMarkdown(mpkDef, def, mdlName) - if writeErr := writeToAll(docsDirs, filename, doc); writeErr != nil { - log.Printf("warning: failed to write %s: %v", filename, writeErr) - continue - } - if def == nil { - withoutRouting = append(withoutRouting, mdlName) - } + doc := widgetDocMarkdown(mpkDef, def, mdlName) + if writeErr := writeToAll(docsDirs, filename, doc); writeErr != nil { + log.Printf("warning: failed to write %s: %v", filename, writeErr) + continue + } + if def == nil { + withoutRouting = append(withoutRouting, mdlName) + } - kind := "CUSTOMWIDGET" - if mpkDef.IsPluggable { - kind = "PLUGGABLEWIDGET" - } - indexEntries = append(indexEntries, fmt.Sprintf("| `%s` | [%s](%s) | `%s` | %s | %d |", - kind, mdlName, filename, mpkDef.ID, mpkDef.Name, len(mpkDef.Properties))) - widgetNames = append(widgetNames, mpkDef.Name) - generated++ + kind := "CUSTOMWIDGET" + if mpkDef.IsPluggable { + kind = "PLUGGABLEWIDGET" } + indexEntries = append(indexEntries, fmt.Sprintf("| `%s` | [%s](%s) | `%s` | %s | %d |", + kind, mdlName, filename, mpkDef.ID, mpkDef.Name, len(mpkDef.Properties))) + widgetNames = append(widgetNames, mpkDef.Name) + generated++ } skill := widgetSkillMarkdown(indexEntries, widgetNames, withoutRouting) diff --git a/mdl/executor/widget_defs_refresh_test.go b/mdl/executor/widget_defs_refresh_test.go new file mode 100644 index 0000000000..700ffd1cbe --- /dev/null +++ b/mdl/executor/widget_defs_refresh_test.go @@ -0,0 +1,170 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "archive/zip" + "encoding/json" + "io" + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +// writeWidgetMPK writes a one-widget .mpk into projectDir/widgets. +func writeWidgetMPK(t *testing.T, projectDir, file, widgetID string, pluggable bool) string { + t.Helper() + plugin := "" + if pluggable { + plugin = ` pluginWidget="true"` + } + entries := []struct{ name, body string }{ + {"package.xml", ``}, + {"W.xml", `WCaption`}, + } + dir := filepath.Join(projectDir, "widgets") + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + path := filepath.Join(dir, file) + f, err := os.Create(path) + if err != nil { + t.Fatal(err) + } + zw := zip.NewWriter(f) + for _, e := range entries { + w, err := zw.Create(e.name) + if err != nil { + t.Fatal(err) + } + if _, err := io.WriteString(w, e.body); err != nil { + t.Fatal(err) + } + } + if err := zw.Close(); err != nil { + t.Fatal(err) + } + if err := f.Close(); err != nil { + t.Fatal(err) + } + return path +} + +// defsByWidgetID reads every .def.json in the project and maps widget ID to +// its mdlName. +func defsByWidgetID(t *testing.T, projectDir string) map[string]string { + t.Helper() + files, _ := filepath.Glob(filepath.Join(projectDir, ".mxcli", "widgets", "*.def.json")) + out := map[string]string{} + for _, f := range files { + data, err := os.ReadFile(f) + if err != nil { + t.Fatal(err) + } + var def WidgetDefinition + if err := json.Unmarshal(data, &def); err != nil { + t.Fatalf("%s: %v", f, err) + } + out[def.WidgetID] = strings.ToUpper(def.MDLName) + } + return out +} + +// #1848: KeyboardShortcut.mpk (a legacy custom widget) and +// itvisors.KeyboardShortcut.mpk (its pluggable successor) both derive +// KEYBOARDSHORTCUT. They wrote one file in turn, so one widget had no +// definition and every refresh reported "2 refreshed" and regenerated the +// widget docs. +func TestRefreshWidgetDefinitions_CollidingNamesKeepBoth(t *testing.T) { + withWidgetStateFingerprint(t, "") + dir := t.TempDir() + const legacy = "KeyboardShortcut.widget.KeyboardShortcut" + const pluggable = "itvisors.keyboardshortcut.KeyboardShortcut" + writeWidgetMPK(t, dir, "KeyboardShortcut.mpk", legacy, false) + writeWidgetMPK(t, dir, "itvisors.KeyboardShortcut.mpk", pluggable, true) + project := filepath.Join(dir, "App.mpr") + + first, err := RefreshWidgetDefinitions(project, false, nil) + if err != nil { + t.Fatal(err) + } + if first.Extracted != 2 { + t.Errorf("first refresh: %+v, want 2 extracted", first) + } + + defs := defsByWidgetID(t, dir) + if defs[pluggable] != "KEYBOARDSHORTCUT" { + t.Errorf("pluggable widget should keep the bare keyword, got %q (defs %v)", defs[pluggable], defs) + } + if name := defs[legacy]; name == "" || name == "KEYBOARDSHORTCUT" { + t.Errorf("legacy widget needs a definition under its own name, got %q (defs %v)", name, defs) + } + + second, err := RefreshWidgetDefinitions(project, false, nil) + if err != nil { + t.Fatal(err) + } + if second.Extracted != 0 || second.Refreshed != 0 { + t.Errorf("second refresh rewrote definitions: %+v", second) + } +} + +// #1848: a refresh after a model edit re-read every .mpk although widgets/ +// had not changed; on Windows each read is also a Defender scan. +func TestRefreshWidgetDefinitions_SkipsUnchangedWidgets(t *testing.T) { + withWidgetStateFingerprint(t, "build-1") + dir := t.TempDir() + mpkPath := writeWidgetMPK(t, dir, "Badge.mpk", "com.example.badge.Badge", true) + project := filepath.Join(dir, "App.mpr") + + if _, err := RefreshWidgetDefinitions(project, false, nil); err != nil { + t.Fatal(err) + } + stats, err := RefreshWidgetDefinitions(project, false, nil) + if err != nil { + t.Fatal(err) + } + if !stats.Unchanged { + t.Fatalf("second refresh of an unchanged widgets/ folder did the work again: %+v", stats) + } + + // Controls: each change below must make the refresh look again. + t.Run("changed mpk", func(t *testing.T) { + later := time.Now().Add(time.Minute) + if err := os.Chtimes(mpkPath, later, later); err != nil { + t.Fatal(err) + } + if stats, _ := RefreshWidgetDefinitions(project, false, nil); stats.Unchanged { + t.Error("a touched .mpk was not looked at") + } + }) + t.Run("deleted def", func(t *testing.T) { + if err := os.Remove(filepath.Join(dir, ".mxcli", "widgets", "badge.def.json")); err != nil { + t.Fatal(err) + } + stats, _ := RefreshWidgetDefinitions(project, false, nil) + if stats.Unchanged || stats.Extracted != 1 { + t.Errorf("a deleted definition was not regenerated: %+v", stats) + } + }) + t.Run("another mxcli build", func(t *testing.T) { + withWidgetStateFingerprint(t, "build-2") + if stats, _ := RefreshWidgetDefinitions(project, false, nil); stats.Unchanged { + t.Error("another mxcli build reused the previous build's definitions") + } + }) + t.Run("force", func(t *testing.T) { + if stats, _ := RefreshWidgetDefinitions(project, true, nil); stats.Unchanged { + t.Error("force skipped the refresh") + } + }) +} + +func withWidgetStateFingerprint(t *testing.T, fp string) { + t.Helper() + prev := widgetStateFingerprint + widgetStateFingerprint = func() string { return fp } + t.Cleanup(func() { widgetStateFingerprint = prev }) +} From 036883095df051be595d2f6efad212921cd76dab Mon Sep 17 00:00:00 2001 From: Ako Date: Sun, 1 Nov 2026 15:49:44 +0100 Subject: [PATCH 08/10] fix(describe): decode only the documents a describe prints (#1850) describe found its document, and the snippets, pages, layouts and flows it names, by listing every document of the type, and on the modelsdk backend a listing decodes each one. A source catalog build describes every document, so it paid that once per document. Lookups now go through headers and decode one unit by ID: snippets, nanoflows, pages and layouts by name or ID, and GetPage / GetNanoflow / GetLayout read only their unit. layoutCheckBackend forwards the header listings, which its embedded interface hid from the type assertion. FactoryManagement, first `refresh catalog full source`: 33 s / 145 s CPU before, 19 s / 38 s after, with identical text for all 3,718 documents. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011oK82ttaGzay5CwiC2V7cf --- ...11-01-describe-decodes-every-document.json | 1 + CHANGELOG.md | 1 + mdl/backend/modelsdk/microflow.go | 18 +- mdl/backend/modelsdk/page.go | 142 +++++++------ mdl/backend/modelsdk/units.go | 14 ++ mdl/executor/cmd_microflows_build.go | 3 +- .../cmd_microflows_derived_layout_backend.go | 41 ++++ mdl/executor/cmd_microflows_show.go | 44 ++-- mdl/executor/cmd_pages_describe.go | 10 +- .../cmd_pages_describe_flowcontext.go | 3 +- mdl/executor/cmd_pages_describe_output.go | 11 +- mdl/executor/cmd_pages_describe_parse.go | 5 +- .../describe_decode_budget_pedapp_test.go | 92 +++++++++ mdl/executor/document_lookup.go | 195 ++++++++++++++++++ mdl/executor/helpers.go | 11 +- 15 files changed, 469 insertions(+), 122 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/mdl-executor/2026-11-01-describe-decodes-every-document.json create mode 100644 mdl/executor/describe_decode_budget_pedapp_test.go create mode 100644 mdl/executor/document_lookup.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-11-01-describe-decodes-every-document.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-11-01-describe-decodes-every-document.json new file mode 100644 index 0000000000..d23bbde9b7 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-11-01-describe-decodes-every-document.json @@ -0,0 +1 @@ +{"area": "mdl/executor", "date": "2026-11-01", "symptom": "A first `refresh catalog full source` on FactoryManagement (3,718 documents) took 33 s and 145 s of CPU; a CPU profile put 26% in describeSnippet -> ListSnippets and 13% in describeNanoflow -> ListMicroflows, both decoding custom-widget and flow trees.", "cause": "describe found its document, and the snippets, pages, layouts and flows it names, by listing every document of the type, and a listing decodes each one. The source build describes every document, so the work grew with the square of the project. The nanoflow path still decoded every nanoflow after the direct fix: describe rebuilds the flow through layoutCheckBackend, a memo that embeds the FullBackend interface and so hid ListNanoflowHeaders from the type assertion.", "file": "mdl/executor/document_lookup.go, mdl/executor/cmd_microflows_derived_layout_backend.go", "insight": "Measure with mprread.UnitsDecoded() against a fixture you then grow: what a describe decodes must not change when documents it does not print are added. When an optional backend capability is detected by type assertion, every wrapper that embeds the backend interface must forward it, or the fast path silently falls back (mendixlabs/mxcli#1624 was the same trap for microflows); a temporary stack dump in decodeUnits for listings over 5 units finds the caller in one run.", "issue": "ako/mxcli#1850"} diff --git a/CHANGELOG.md b/CHANGELOG.md index 619faabcc3..00126555e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **`describe` and the source catalog no longer decode every document of a type to find one** (ako/mxcli#1850) — describing a snippet decoded every snippet of the project, a nanoflow every microflow and nanoflow, a page every page, and so did resolving the snippets, pages, layouts and flows a description names. A source catalog build describes every document, so it paid that once per document: on FactoryManagement a first `refresh catalog full source` took 33 s and 145 s of CPU, and now takes 19 s and 38 s, with identical text for all 3,718 documents. `create nanoflow` likewise decodes only the nanoflows of its own name. - **MPR012 follows the app's Web UI client** (ako/mxcli#1287, mendixlabs/mxcli#1226) — a Mendix 11 app can still run the classic client, where the legacy static and dynamic image widgets build clean, but MPR012 warned on each of them that "mxbuild reports CE0582". On the classic client or in migration mode it is now an info-level note that the widget needs converting before the app switches; under the React client it stays a warning. - **Every `mxcli init --tool ` installs the bundled lint rules, and `lint` says when it runs without them** (ako/mxcli#1292, mendixlabs/mxcli#1242) — only `claude` and `opencode` wrote `.claude/lint-rules/`, so `init --tool cursor` (or aider, continue, copilot, vibe, windsurf) left `mxcli lint` running the built-in Go rules alone, silently. `lint` and `report` now print a warning when no `.claude/lint-rules/` is found, naming how many rules run instead. - **`show references to` an entity, attribute or association lists the message definitions that expose it** (#1781) — a message definition (the pre-11.15 collection, `MESSAGE_DEFINITION_COLLECTION`, and the 11.15+ document, `MESSAGE_DEFINITION`) was not walked for references, so impact analysis called an exposed entity or attribute unused. The entity edges are `mapping`, the attribute and association edges `member`, as for an import or export mapping. diff --git a/mdl/backend/modelsdk/microflow.go b/mdl/backend/modelsdk/microflow.go index 9dcf735212..2837572047 100644 --- a/mdl/backend/modelsdk/microflow.go +++ b/mdl/backend/modelsdk/microflow.go @@ -127,17 +127,19 @@ func (b *Backend) ListNanoflows() ([]*microflows.Nanoflow, error) { return out, nil } +// GetNanoflow decodes the nanoflow whose ID is id, and no other (#1850). func (b *Backend) GetNanoflow(id model.ID) (*microflows.Nanoflow, error) { - units, err := listUnits[*genMf.Nanoflow](b) - if err != nil { + u, ok, err := mprread.UnitByID[*genMf.Nanoflow](b.reader, id) + if err != nil || !ok { return nil, err } - for _, u := range units { - if model.ID(u.Element.ID()) == id { - return nanoflowFromGen(u.Element, u.ContainerID), nil - } - } - return nil, nil + return nanoflowFromGen(u.Element, u.ContainerID), nil +} + +// ListNanoflowHeaders lists every nanoflow's ID, container, name and Excluded +// flag without decoding any flow. +func (b *Backend) ListNanoflowHeaders() ([]types.DocumentHeader, error) { + return unitHeaders[*genMf.Nanoflow](b) } // nanoflowFromGen mirrors microflowFromGen — nanoflows share the same parameter, diff --git a/mdl/backend/modelsdk/page.go b/mdl/backend/modelsdk/page.go index 9dc9a98ec7..2fd5fd2105 100644 --- a/mdl/backend/modelsdk/page.go +++ b/mdl/backend/modelsdk/page.go @@ -27,15 +27,7 @@ import ( // ListPageHeaders lists every page's ID, container, name and Excluded flag // without decoding any page. func (b *Backend) ListPageHeaders() ([]types.DocumentHeader, error) { - hs, err := mprread.ListUnitHeaders[*genPg.Page](b.reader) - if err != nil { - return nil, err - } - out := make([]types.DocumentHeader, len(hs)) - for i, h := range hs { - out[i] = types.DocumentHeader(h) - } - return out, nil + return unitHeaders[*genPg.Page](b) } func (b *Backend) ListPages() ([]*pages.Page, error) { @@ -78,17 +70,13 @@ func pageTemplateFromGen(pt *genPg.PageTemplate, containerID model.ID) *pages.Pa return out } +// GetPage decodes the page whose ID is id, and no other (#1850). func (b *Backend) GetPage(id model.ID) (*pages.Page, error) { - units, err := listUnits[*genPg.Page](b) - if err != nil { + u, ok, err := mprread.UnitByID[*genPg.Page](b.reader, id) + if err != nil || !ok { return nil, err } - for _, u := range units { - if model.ID(u.Element.ID()) == id { - return pageFromGen(u.Element, u.ContainerID), nil - } - } - return nil, nil + return pageFromGen(u.Element, u.ContainerID), nil } // ListSnippets reads snippet units (count + identity is what SHOW MODULES needs; @@ -100,39 +88,65 @@ func (b *Backend) ListSnippets() ([]*pages.Snippet, error) { } out := make([]*pages.Snippet, 0, len(units)) for _, u := range units { - s := &pages.Snippet{ - ContainerID: u.ContainerID, - Name: u.Element.Name(), - Documentation: u.Element.Documentation(), - Excluded: u.Element.Excluded(), + out = append(out, snippetFromGen(u)) + } + return out, nil +} + +// GetSnippet decodes the snippet whose ID is id, and no other (#1850). nil when +// there is none. +func (b *Backend) GetSnippet(id model.ID) (*pages.Snippet, error) { + u, ok, err := mprread.UnitByID[*genPg.Snippet](b.reader, id) + if err != nil || !ok { + return nil, err + } + return snippetFromGen(u), nil +} + +// ListSnippetHeaders lists every snippet's ID, container, name and Excluded +// flag without decoding any snippet. +func (b *Backend) ListSnippetHeaders() ([]types.DocumentHeader, error) { + return unitHeaders[*genPg.Snippet](b) +} + +// ListLayoutHeaders lists every layout's ID, container, name and Excluded flag +// without decoding any layout. +func (b *Backend) ListLayoutHeaders() ([]types.DocumentHeader, error) { + return unitHeaders[*genPg.Layout](b) +} + +func snippetFromGen(u mprread.Unit[*genPg.Snippet]) *pages.Snippet { + s := &pages.Snippet{ + ContainerID: u.ContainerID, + Name: u.Element.Name(), + Documentation: u.Element.Documentation(), + Excluded: u.Element.Excluded(), + } + s.ID = model.ID(u.Element.ID()) + // Populate declared parameters — the page builder reads these to validate + // and wire SNIPPETCALL argument mappings (without them every parameterised + // snippet call writes an empty argument → CE1571 in Studio Pro). + for _, el := range u.Element.ParametersItems() { + sp, ok := el.(*genPg.SnippetParameter) + if !ok { + continue } - s.ID = model.ID(u.Element.ID()) - // Populate declared parameters — the page builder reads these to validate - // and wire SNIPPETCALL argument mappings (without them every parameterised - // snippet call writes an empty argument → CE1571 in Studio Pro). - for _, el := range u.Element.ParametersItems() { - sp, ok := el.(*genPg.SnippetParameter) - if !ok { - continue - } - p := &pages.SnippetParameter{Name: sp.Name()} - p.ID = model.ID(sp.ID()) - // Entity or primitive — a snippet parameter's ParameterType is the - // polymorphic DataTypes$DataType, the same as a page parameter's. - // Reading only the ObjectType arm made every primitive parameter - // read back as an untyped one (mendixlabs/mxcli#1028). - if pt := sp.ParameterType(); pt != nil { - if ot, ok := pt.(*genDT.ObjectType); ok { - p.EntityName = ot.EntityQualifiedName() - } else { - p.Type = pt.TypeName() - } + p := &pages.SnippetParameter{Name: sp.Name()} + p.ID = model.ID(sp.ID()) + // Entity or primitive — a snippet parameter's ParameterType is the + // polymorphic DataTypes$DataType, the same as a page parameter's. + // Reading only the ObjectType arm made every primitive parameter + // read back as an untyped one (mendixlabs/mxcli#1028). + if pt := sp.ParameterType(); pt != nil { + if ot, ok := pt.(*genDT.ObjectType); ok { + p.EntityName = ot.EntityQualifiedName() + } else { + p.Type = pt.TypeName() } - s.Parameters = append(s.Parameters, p) } - out = append(out, s) + s.Parameters = append(s.Parameters, p) } - return out, nil + return s } // ListBuildingBlocks reads Pages$BuildingBlock units. Read-only: the codec engine @@ -250,20 +264,24 @@ func (b *Backend) ListLayouts() ([]*pages.Layout, error) { } out := make([]*pages.Layout, 0, len(units)) for _, u := range units { - l := &pages.Layout{ - ContainerID: u.ContainerID, - Name: u.Element.Name(), - Documentation: u.Element.Documentation(), - LayoutType: layoutTypeOf(u.Element), - Native: layoutIsNative(u.Element), - Class: appearanceClassOf(u.Element), - } - l.ID = model.ID(u.Element.ID()) - out = append(out, l) + out = append(out, layoutFromGen(u)) } return out, nil } +func layoutFromGen(u mprread.Unit[*genPg.Layout]) *pages.Layout { + l := &pages.Layout{ + ContainerID: u.ContainerID, + Name: u.Element.Name(), + Documentation: u.Element.Documentation(), + LayoutType: layoutTypeOf(u.Element), + Native: layoutIsNative(u.Element), + Class: appearanceClassOf(u.Element), + } + l.ID = model.ID(u.Element.ID()) + return l +} + // appearanceClassOf reads the layout's own CSS class. // // Not decoration: Atlas scopes its layout chrome to `.layout-atlas` and @@ -308,17 +326,13 @@ func layoutIsNative(l *genPg.Layout) bool { } // GetLayout returns a single layout by ID (shallow). +// GetLayout decodes the layout whose ID is id, and no other (#1850). func (b *Backend) GetLayout(id model.ID) (*pages.Layout, error) { - layouts, err := b.ListLayouts() - if err != nil { + u, ok, err := mprread.UnitByID[*genPg.Layout](b.reader, id) + if err != nil || !ok { return nil, err } - for _, l := range layouts { - if l.ID == id { - return l, nil - } - } - return nil, nil + return layoutFromGen(u), nil } // LayoutPlaceholders returns the placeholder names a layout declares. diff --git a/mdl/backend/modelsdk/units.go b/mdl/backend/modelsdk/units.go index 0ac3afa2fa..ae1cb91a0b 100644 --- a/mdl/backend/modelsdk/units.go +++ b/mdl/backend/modelsdk/units.go @@ -86,6 +86,20 @@ func listUnits[T element.Element](b *Backend) ([]mprread.Unit[T], error) { return mprread.ListUnitsWithContainer[T](b.reader) } +// unitHeaders lists the ID, container, name and Excluded flag of every unit of +// type T without decoding any of them. +func unitHeaders[T element.Element](b *Backend) ([]types.DocumentHeader, error) { + hs, err := mprread.ListUnitHeaders[T](b.reader) + if err != nil { + return nil, err + } + out := make([]types.DocumentHeader, len(hs)) + for i, h := range hs { + out[i] = types.DocumentHeader(h) + } + return out, nil +} + func (b *Backend) ListUnits() ([]*types.UnitInfo, error) { us, err := b.reader.ListUnits() if err != nil { diff --git a/mdl/executor/cmd_microflows_build.go b/mdl/executor/cmd_microflows_build.go index 964bd1e2a8..8e903a5916 100644 --- a/mdl/executor/cmd_microflows_build.go +++ b/mdl/executor/cmd_microflows_build.go @@ -539,7 +539,8 @@ func buildNanoflowFromStmt(ctx *ExecContext, s *ast.CreateNanoflowStmt, opts bui // "Mark as used" has no MDL spelling; the rebuild hardcoded false, so a // rewrite cleared it. A new nanoflow still starts unmarked. existingMarkAsUsed := false - existingNanoflows, err := ctx.Backend.ListNanoflows() + // Only the nanoflows of this name are decoded, not the project's (#1850). + existingNanoflows, err := nanoflowsNamed(ctx.Backend, s.Name.Name) if err != nil { return nil, mdlerrors.NewBackend("check existing nanoflows", err) } diff --git a/mdl/executor/cmd_microflows_derived_layout_backend.go b/mdl/executor/cmd_microflows_derived_layout_backend.go index 184cd5c722..4c80d9baa9 100644 --- a/mdl/executor/cmd_microflows_derived_layout_backend.go +++ b/mdl/executor/cmd_microflows_derived_layout_backend.go @@ -10,6 +10,7 @@ import ( "github.com/mendixlabs/mxcli/sdk/domainmodel" "github.com/mendixlabs/mxcli/sdk/javaactions" "github.com/mendixlabs/mxcli/sdk/microflows" + "github.com/mendixlabs/mxcli/sdk/pages" ) // layoutCheckBackend memoises the lookups the flow builder repeats on every @@ -34,6 +35,10 @@ type layoutCheckBackend struct { domainModels *memoResult[[]*domainmodel.DomainModel] microflowHeaders *memoResult[[]types.DocumentHeader] + nanoflowHeaders *memoResult[[]types.DocumentHeader] + pageHeaders *memoResult[[]types.DocumentHeader] + snippetHeaders *memoResult[[]types.DocumentHeader] + layoutHeaders *memoResult[[]types.DocumentHeader] moduleByName map[string]memoResult[*model.Module] microflowsNamed map[string]memoResult[[]*microflows.Microflow] @@ -151,6 +156,42 @@ func (b *layoutCheckBackend) ListMicroflowHeaders() ([]types.DocumentHeader, err }) } +// The header listings and GetSnippet keep the real backend's by-header +// lookups visible through the memo, for the same reason as the microflow ones +// above: without them a describe that rebuilds a nanoflow to derive its layout +// decoded every nanoflow of the project (#1850). +func (b *layoutCheckBackend) ListNanoflowHeaders() ([]types.DocumentHeader, error) { + return memoOnce(&b.nanoflowHeaders, func() ([]types.DocumentHeader, error) { return nanoflowHeaders(b.FullBackend) }) +} + +func (b *layoutCheckBackend) ListPageHeaders() ([]types.DocumentHeader, error) { + return memoOnce(&b.pageHeaders, func() ([]types.DocumentHeader, error) { return pageHeadersOf(b.FullBackend) }) +} + +func (b *layoutCheckBackend) ListSnippetHeaders() ([]types.DocumentHeader, error) { + return memoOnce(&b.snippetHeaders, func() ([]types.DocumentHeader, error) { return snippetHeaders(b.FullBackend) }) +} + +func (b *layoutCheckBackend) ListLayoutHeaders() ([]types.DocumentHeader, error) { + return memoOnce(&b.layoutHeaders, func() ([]types.DocumentHeader, error) { return layoutHeaders(b.FullBackend) }) +} + +func (b *layoutCheckBackend) GetSnippet(id model.ID) (*pages.Snippet, error) { + if l, ok := b.FullBackend.(snippetLookup); ok { + return l.GetSnippet(id) + } + all, err := b.ListSnippets() + if err != nil { + return nil, err + } + for _, s := range all { + if s.ID == id { + return s, nil + } + } + return nil, nil +} + func (b *layoutCheckBackend) GetModuleByName(name string) (*model.Module, error) { return memoKeyed(b.moduleByName, name, func() (*model.Module, error) { return b.FullBackend.GetModuleByName(name) }) } diff --git a/mdl/executor/cmd_microflows_show.go b/mdl/executor/cmd_microflows_show.go index 2327a7c336..a254fd3cb5 100644 --- a/mdl/executor/cmd_microflows_show.go +++ b/mdl/executor/cmd_microflows_show.go @@ -391,25 +391,17 @@ func describeNanoflow(ctx *ExecContext, name ast.QualifiedName) error { } // Build microflow/nanoflow name lookup (used for call actions) - microflowNames := make(map[model.ID]string) - allMicroflows, err := ctx.Backend.ListMicroflows() + microflowNames, err := flowNames(ctx, h, true) if err != nil { - return mdlerrors.NewBackend("list microflows", err) - } - for _, mf := range allMicroflows { - microflowNames[mf.ID] = h.GetQualifiedName(mf.ContainerID, mf.Name) + return mdlerrors.NewBackend("list flows", err) } - // Find the nanoflow - allNanoflows, err := ctx.Backend.ListNanoflows() + // Find the nanoflow: only the nanoflows of this name are decoded (#1850). + allNanoflows, err := nanoflowsNamed(ctx.Backend, name.Name) if err != nil { return mdlerrors.NewBackend("list nanoflows", err) } - for _, nf := range allNanoflows { - microflowNames[nf.ID] = h.GetQualifiedName(nf.ContainerID, nf.Name) - } - // Describe the live nanoflow, not an excluded twin of the same name (#914). targetNf, _ := pickDescribed(ctx, allNanoflows, func(nf *microflows.Nanoflow) model.ID { return nf.ID }, @@ -536,13 +528,13 @@ func describeMicroflowToString(ctx *ExecContext, name ast.QualifiedName) (string } } - microflowNames := make(map[model.ID]string) - allMicroflows, err := ctx.Backend.ListMicroflows() + microflowNames, err := flowNames(ctx, h, false) if err != nil { return "", nil, mdlerrors.NewBackend("list microflows", err) } - for _, mf := range allMicroflows { - microflowNames[mf.ID] = h.GetQualifiedName(mf.ContainerID, mf.Name) + allMicroflows, err := microflowsNamed(ctx, name.Name) + if err != nil { + return "", nil, mdlerrors.NewBackend("list microflows", err) } // Describe the live microflow: a module may hold an excluded twin of this @@ -583,22 +575,14 @@ func describeNanoflowToString(ctx *ExecContext, name ast.QualifiedName) (string, } } - microflowNames := make(map[model.ID]string) - allMicroflows, err := ctx.Backend.ListMicroflows() + microflowNames, err := flowNames(ctx, h, true) if err != nil { - return "", nil, mdlerrors.NewBackend("list microflows", err) - } - for _, mf := range allMicroflows { - microflowNames[mf.ID] = h.GetQualifiedName(mf.ContainerID, mf.Name) + return "", nil, mdlerrors.NewBackend("list flows", err) } - - allNanoflows, err := ctx.Backend.ListNanoflows() + allNanoflows, err := nanoflowsNamed(ctx.Backend, name.Name) if err != nil { return "", nil, mdlerrors.NewBackend("list nanoflows", err) } - for _, nf := range allNanoflows { - microflowNames[nf.ID] = h.GetQualifiedName(nf.ContainerID, nf.Name) - } // Describe the live nanoflow, not an excluded twin of the same name (#914). targetNf, _ := pickLive(allNanoflows, @@ -1553,14 +1537,10 @@ func describeRule(ctx *ExecContext, name ast.QualifiedName) error { // A rule's body can call microflows, so the call-target lookup is the same // one the microflow describer builds. - microflowNames := make(map[model.ID]string) - allMicroflows, err := ctx.Backend.ListMicroflows() + microflowNames, err := flowNames(ctx, h, false) if err != nil { return mdlerrors.NewBackend("list microflows", err) } - for _, mf := range allMicroflows { - microflowNames[mf.ID] = h.GetQualifiedName(mf.ContainerID, mf.Name) - } allRules, err := ctx.Backend.ListRules() if err != nil { diff --git a/mdl/executor/cmd_pages_describe.go b/mdl/executor/cmd_pages_describe.go index 947c1f0fad..a618be018b 100644 --- a/mdl/executor/cmd_pages_describe.go +++ b/mdl/executor/cmd_pages_describe.go @@ -27,8 +27,8 @@ func describePage(ctx *ExecContext, name ast.QualifiedName) error { return mdlerrors.NewBackend("build hierarchy", err) } - // Find the page - allPages, err := ctx.Backend.ListPages() + // Find the page: only the pages of this name are decoded (#1850). + allPages, err := pagesWhere(ctx.Backend, func(n string) bool { return n == name.Name }) if err != nil { return mdlerrors.NewBackend("list pages", err) } @@ -241,8 +241,8 @@ func describeSnippet(ctx *ExecContext, name ast.QualifiedName) error { return mdlerrors.NewBackend("build hierarchy", err) } - // Find the snippet - allSnippets, err := ctx.Backend.ListSnippets() + // Find the snippet: only the snippets of this name are decoded (#1850). + allSnippets, err := snippetsNamed(ctx.Backend, name.Name) if err != nil { return mdlerrors.NewBackend("list snippets", err) } @@ -639,7 +639,7 @@ func getBsonArrayMaps(v any) []map[string]any { // resolveLayoutName resolves a layout ID to its qualified name. func resolveLayoutName(ctx *ExecContext, layoutID model.ID) string { - layouts, err := ctx.Backend.ListLayouts() + layouts, err := layoutHeaders(ctx.Backend) if err != nil { return string(layoutID) } diff --git a/mdl/executor/cmd_pages_describe_flowcontext.go b/mdl/executor/cmd_pages_describe_flowcontext.go index dc3da1deb6..9892069d30 100644 --- a/mdl/executor/cmd_pages_describe_flowcontext.go +++ b/mdl/executor/cmd_pages_describe_flowcontext.go @@ -111,7 +111,8 @@ func flowReturnEntity(ctx *ExecContext, kind, qualifiedName string) string { } } case "nanoflow": - nfs, err := ctx.Backend.ListNanoflows() + _, local, _ := strings.Cut(qualifiedName, ".") + nfs, err := nanoflowsNamed(ctx.Backend, local) if err != nil { return "" } diff --git a/mdl/executor/cmd_pages_describe_output.go b/mdl/executor/cmd_pages_describe_output.go index 75c45d2f0f..e823664e12 100644 --- a/mdl/executor/cmd_pages_describe_output.go +++ b/mdl/executor/cmd_pages_describe_output.go @@ -1808,7 +1808,9 @@ func getPageQualifiedName(ctx *ExecContext, pageID model.ID) string { if pageID == "" { return "" } - allPages, err := ctx.Backend.ListPages() + // Headers, not decoded pages (#1850), and not the cached name map, which a + // page created earlier in the same script is missing from. + hs, err := pageHeaders(ctx) if err != nil { return "" } @@ -1816,10 +1818,9 @@ func getPageQualifiedName(ctx *ExecContext, pageID model.ID) string { if err != nil { return "" } - for _, p := range allPages { + for _, p := range hs { if p.ID == pageID { - modName := h.GetModuleName(h.FindModuleID(p.ContainerID)) - return modName + "." + p.Name + return h.GetQualifiedName(p.ContainerID, p.Name) } } return "" @@ -1867,7 +1868,7 @@ func targetPageParameterNames(ctx *ExecContext, qualifiedName string) []string { if !ok || module == "" || name == "" { return nil } - allPages, err := ctx.Backend.ListPages() + allPages, err := pagesWhere(ctx.Backend, func(n string) bool { return strings.EqualFold(n, name) }) if err != nil { return nil } diff --git a/mdl/executor/cmd_pages_describe_parse.go b/mdl/executor/cmd_pages_describe_parse.go index 9f3f3deb0a..3146ce76b2 100644 --- a/mdl/executor/cmd_pages_describe_parse.go +++ b/mdl/executor/cmd_pages_describe_parse.go @@ -1471,8 +1471,9 @@ func extractSnippetRef(ctx *ExecContext, w map[string]any) string { } // Try binary ID and resolve to name if formID := extractBinaryID(formCall["Form"]); formID != "" { - // Try to resolve the snippet name from ID - snippets, err := ctx.Backend.ListSnippets() + // Try to resolve the snippet name from ID: by its header, + // without decoding every snippet (#1850). + snippets, err := snippetHeaders(ctx.Backend) if err == nil { for _, s := range snippets { if string(s.ID) == formID { diff --git a/mdl/executor/describe_decode_budget_pedapp_test.go b/mdl/executor/describe_decode_budget_pedapp_test.go new file mode 100644 index 0000000000..84b26c2f2a --- /dev/null +++ b/mdl/executor/describe_decode_budget_pedapp_test.go @@ -0,0 +1,92 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "bytes" + "fmt" + "strings" + "testing" + + "github.com/mendixlabs/mxcli/modelsdk/mprread" +) + +// A describe found its document, and the snippets, flows and pages it names, +// by listing every document of the type, and the listing decoded each one. A +// source catalog build describes every document, so it paid that once per +// document: 40% of a first build's CPU on FactoryManagement (#1850). +// +// The measure: describe each kind of document, add more documents of every +// kind, describe again. What a describe decodes must not grow with documents +// it does not print. +func TestDescribeDecodesOnlyWhatItPrints(t *testing.T) { + exec, out, _ := openPedAppCopy(t) + if err := agreeExec(t, exec, `mdl 1; +create persistent entity MyFirstModule.DescProbe (Name: String(50)); +create snippet MyFirstModule.DescSnippet ( Params: ( $P: MyFirstModule.DescProbe ) ) { + dynamictext txtName (Content: 'probe') +}; +create microflow MyFirstModule.DescFlow () returns Boolean as $Ok +begin + declare $Ok Boolean = true; + return $Ok; +end; +create nanoflow MyFirstModule.DescNano () returns Boolean as $Ok +begin + declare $Ok Boolean = true; + return $Ok; +end; +create page MyFirstModule.DescPage (Title: 'P', Layout: Atlas_Core.Atlas_Default, Params: ( $P: MyFirstModule.DescProbe )) { + snippetcall sc1 (Snippet: MyFirstModule.DescSnippet, Params: (P = $P)) + actionbutton btn1 (Caption: 'Go', Action: call nanoflow MyFirstModule.DescNano) +}; +`); err != nil { + t.Fatalf("setup: %v\n%s", err, out.String()) + } + + kinds := []string{ + "snippet MyFirstModule.DescSnippet", + "page MyFirstModule.DescPage", + "microflow MyFirstModule.DescFlow", + "nanoflow MyFirstModule.DescNano", + } + measure := func() map[string]int64 { + decoded := map[string]int64{} + for _, k := range kinds { + before := mprread.UnitsDecoded() + var buf bytes.Buffer + exec.output = &buf + if err := agreeExec(t, exec, "describe "+k+";"); err != nil { + t.Fatalf("describe %s: %v", k, err) + } + decoded[k] = mprread.UnitsDecoded() - before + if !strings.Contains(buf.String(), strings.Fields(k)[1]) { + t.Fatalf("control: describe %s printed %q", k, buf.String()) + } + } + exec.output = out + return decoded + } + base := measure() + + const extra = 15 + var more strings.Builder + more.WriteString("mdl 1;\n") + for i := range extra { + fmt.Fprintf(&more, "create snippet MyFirstModule.DescFiller%d { dynamictext t (Content: 'x') };\n", i) + fmt.Fprintf(&more, "create page MyFirstModule.DescFillerPage%d (Title: 'P', Layout: Atlas_Core.Atlas_Default) { dynamictext t (Content: 'x') };\n", i) + fmt.Fprintf(&more, "create microflow MyFirstModule.DescFillerFlow%d () begin end;\n", i) + fmt.Fprintf(&more, "create nanoflow MyFirstModule.DescFillerNano%d () begin end;\n", i) + } + if err := agreeExec(t, exec, more.String()); err != nil { + t.Fatalf("add documents: %v\n%s", err, out.String()) + } + + after := measure() + for _, k := range kinds { + if after[k] > base[k] { + t.Errorf("describe %s decoded %d units, and %d after adding %d documents of each kind it does not print", + k, base[k], after[k], extra) + } + } +} diff --git a/mdl/executor/document_lookup.go b/mdl/executor/document_lookup.go new file mode 100644 index 0000000000..9fadc9c750 --- /dev/null +++ b/mdl/executor/document_lookup.go @@ -0,0 +1,195 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "github.com/mendixlabs/mxcli/mdl/backend" + "github.com/mendixlabs/mxcli/mdl/types" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" + "github.com/mendixlabs/mxcli/sdk/pages" +) + +// Lookups of one document by name or ID that decode that document only. +// +// A describe used to find its document, and every snippet, flow, page and +// layout it names, by listing all documents of the type, and on the modelsdk +// backend a listing decodes each one. A source catalog build describes every +// document, so it paid that once per document: 40% of a first build's CPU on +// FactoryManagement (#1850). The modelsdk backend lists headers without +// decoding and decodes one unit by ID; another backend falls back to its +// listing. + +type snippetLookup interface { + ListSnippetHeaders() ([]types.DocumentHeader, error) + GetSnippet(id model.ID) (*pages.Snippet, error) +} + +type nanoflowHeaderLister interface { + ListNanoflowHeaders() ([]types.DocumentHeader, error) +} + +type pageHeaderLister interface { + ListPageHeaders() ([]types.DocumentHeader, error) +} + +type layoutHeaderLister interface { + ListLayoutHeaders() ([]types.DocumentHeader, error) +} + +// decodeWhere decodes, with get, the documents whose header keep accepts. +func decodeWhere[E any](hs []types.DocumentHeader, keep func(types.DocumentHeader) bool, get func(model.ID) (*E, error)) ([]*E, error) { + var out []*E + for _, h := range hs { + if !keep(h) { + continue + } + d, err := get(h.ID) + if err != nil { + return nil, err + } + if d != nil { + out = append(out, d) + } + } + return out, nil +} + +// snippetHeaders is every snippet's ID, container and name. +func snippetHeaders(be backend.FullBackend) ([]types.DocumentHeader, error) { + if l, ok := be.(snippetLookup); ok { + return l.ListSnippetHeaders() + } + all, err := be.ListSnippets() + if err != nil { + return nil, err + } + out := make([]types.DocumentHeader, len(all)) + for i, s := range all { + out[i] = types.DocumentHeader{ID: s.ID, ContainerID: s.ContainerID, Name: s.Name, Excluded: s.Excluded} + } + return out, nil +} + +// snippetsNamed is the project's snippets whose local name is name. +func snippetsNamed(be backend.FullBackend, name string) ([]*pages.Snippet, error) { + if l, ok := be.(snippetLookup); ok { + hs, err := l.ListSnippetHeaders() + if err != nil { + return nil, err + } + return decodeWhere(hs, func(h types.DocumentHeader) bool { return h.Name == name }, l.GetSnippet) + } + all, err := be.ListSnippets() + if err != nil { + return nil, err + } + var out []*pages.Snippet + for _, s := range all { + if s.Name == name { + out = append(out, s) + } + } + return out, nil +} + +// nanoflowHeaders is every nanoflow's ID, container and name. +func nanoflowHeaders(be backend.FullBackend) ([]types.DocumentHeader, error) { + if l, ok := be.(nanoflowHeaderLister); ok { + return l.ListNanoflowHeaders() + } + all, err := be.ListNanoflows() + if err != nil { + return nil, err + } + out := make([]types.DocumentHeader, len(all)) + for i, nf := range all { + out[i] = types.DocumentHeader{ID: nf.ID, ContainerID: nf.ContainerID, Name: nf.Name, Excluded: nf.Excluded} + } + return out, nil +} + +// nanoflowsNamed is the project's nanoflows whose local name is name. +func nanoflowsNamed(be backend.FullBackend, name string) ([]*microflows.Nanoflow, error) { + if l, ok := be.(nanoflowHeaderLister); ok { + hs, err := l.ListNanoflowHeaders() + if err != nil { + return nil, err + } + return decodeWhere(hs, func(h types.DocumentHeader) bool { return h.Name == name }, be.GetNanoflow) + } + all, err := be.ListNanoflows() + if err != nil { + return nil, err + } + var out []*microflows.Nanoflow + for _, nf := range all { + if nf.Name == name { + out = append(out, nf) + } + } + return out, nil +} + +// pagesWhere is the project's pages whose local name keep accepts. +func pagesWhere(be backend.FullBackend, keep func(name string) bool) ([]*pages.Page, error) { + if l, ok := be.(pageHeaderLister); ok { + hs, err := l.ListPageHeaders() + if err != nil { + return nil, err + } + return decodeWhere(hs, func(h types.DocumentHeader) bool { return keep(h.Name) }, be.GetPage) + } + all, err := be.ListPages() + if err != nil { + return nil, err + } + var out []*pages.Page + for _, p := range all { + if keep(p.Name) { + out = append(out, p) + } + } + return out, nil +} + +// layoutHeaders is every layout's ID, container and name. +func layoutHeaders(be backend.FullBackend) ([]types.DocumentHeader, error) { + if l, ok := be.(layoutHeaderLister); ok { + return l.ListLayoutHeaders() + } + all, err := be.ListLayouts() + if err != nil { + return nil, err + } + out := make([]types.DocumentHeader, len(all)) + for i, l := range all { + out[i] = types.DocumentHeader{ID: l.ID, ContainerID: l.ContainerID, Name: l.Name} + } + return out, nil +} + +// flowNames maps every microflow and nanoflow ID to its qualified name: what a +// flow's call actions resolve against. Read from headers, so no flow is +// decoded, and fresh: the cached microflow names miss a flow created earlier in +// the same script. +func flowNames(ctx *ExecContext, h *ContainerHierarchy, withNanoflows bool) (map[model.ID]string, error) { + out := map[model.ID]string{} + mfs, err := microflowHeaders(ctx) + if err != nil { + return nil, err + } + for _, mf := range mfs { + out[mf.ID] = h.GetQualifiedName(mf.ContainerID, mf.Name) + } + if withNanoflows { + nfs, err := nanoflowHeaders(ctx.Backend) + if err != nil { + return nil, err + } + for _, nf := range nfs { + out[nf.ID] = h.GetQualifiedName(nf.ContainerID, nf.Name) + } + } + return out, nil +} diff --git a/mdl/executor/helpers.go b/mdl/executor/helpers.go index 1dcca687e2..9b89b18930 100644 --- a/mdl/executor/helpers.go +++ b/mdl/executor/helpers.go @@ -10,6 +10,7 @@ import ( "strings" "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/backend" mdlerrors "github.com/mendixlabs/mxcli/mdl/errors" "github.com/mendixlabs/mxcli/mdl/folderpath" "github.com/mendixlabs/mxcli/mdl/types" @@ -632,12 +633,14 @@ func microflowHeaders(ctx *ExecContext) ([]types.DocumentHeader, error) { // pageHeaders lists every page's ID, container and name: without decoding a // page on the modelsdk backend. func pageHeaders(ctx *ExecContext) ([]types.DocumentHeader, error) { - if l, ok := ctx.Backend.(interface { - ListPageHeaders() ([]types.DocumentHeader, error) - }); ok { + return pageHeadersOf(ctx.Backend) +} + +func pageHeadersOf(be backend.FullBackend) ([]types.DocumentHeader, error) { + if l, ok := be.(pageHeaderLister); ok { return l.ListPageHeaders() } - all, err := ctx.Backend.ListPages() + all, err := be.ListPages() if err != nil { return nil, err } From 93f94786f783c668d8d7f2fee890ce0b7c6526fd Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 1 Nov 2026 17:30:03 +0000 Subject: [PATCH 09/10] fix(mcp): --mcp-trace lists the PED calls (#1833) `--mcp-trace` / `--mcp-verbose` printed each MDL statement but never a tool call, on exec, -c, the REPL and `mcp capabilities`. The executor got the tracer; the MCP backend the CLI builds did not, because WithTracer had no caller outside tests. A run that sent every write to Studio Pro therefore read as one that sent nothing, which is how mendixlabs/mxcli#1684 concluded that `exec --mcp` wrote the .mpr instead. TestMCPTrace_ListsThePEDCallsOfAWrite drives the CLI's executor builder against a fake Studio Pro. Before: the fake received ped_get_schema, ped_create_document and six ped_check_errors while stderr held only the two statement headers. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01NUbjJ5DeS2vysn9hfKir4Y --- ...026-11-01-mcp-trace-lists-no-ped-call.json | 1 + CHANGELOG.md | 1 + cmd/mxcli/engine.go | 7 +- cmd/mxcli/mcp.go | 2 +- cmd/mxcli/mcp_trace_test.go | 157 ++++++++++++++++++ 5 files changed, 166 insertions(+), 2 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/cmd-mxcli/2026-11-01-mcp-trace-lists-no-ped-call.json create mode 100644 cmd/mxcli/mcp_trace_test.go diff --git a/.claude/skills/fix-issue/findings/cmd-mxcli/2026-11-01-mcp-trace-lists-no-ped-call.json b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-11-01-mcp-trace-lists-no-ped-call.json new file mode 100644 index 0000000000..3257c1a702 --- /dev/null +++ b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-11-01-mcp-trace-lists-no-ped-call.json @@ -0,0 +1 @@ +{"area": "cmd/mxcli", "date": "2026-11-01", "symptom": "`mxcli exec probe.mdl --mcp --mcp-trace` (and `-c`, the REPL, `mcp capabilities`) printed `▸ connect local …` and `▸ CreateConstantStmt` but no PED tool call, so a reporter concluded the write went to the .mpr on disk instead of Studio Pro (mendixlabs/mxcli#1684). A fake Studio Pro received ped_get_schema, ped_create_document and six ped_check_errors in the same run", "cause": "Tracing has two halves: the executor prints statements (Executor.SetTracer, wired in newLoggedExecutorTo) and the MCP client prints calls from Backend.tracer, set only by mcpbackend.WithTracer. newBackendFactory and the mcp capabilities command built the backend with mcpbackend.New alone; WithTracer had no caller outside tests, although executor.go's SetTracer comment says the backend must get it too", "file": "cmd/mxcli/engine.go (newBackendFactory); cmd/mxcli/mcp.go", "fix": "Build the MCP backend with .WithTracer(mcpTracer()) in both places", "insight": "A diagnostic flag that prints half its output is worse than none: the missing half reads as a negative result. When a report's evidence is a trace or a log line that is absent, check that the thing producing that line is wired before believing the absence; a logging fake server on the other end settled it in one run. The test drives the CLI's own executor builder against a fake PED and compares the trace with what the fake received", "refs": "ako/mxcli#1833, mendixlabs/mxcli#1684", "test": "cmd/mxcli/mcp_trace_test.go"} diff --git a/CHANGELOG.md b/CHANGELOG.md index c0344d3bb7..d34bfcd2ba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **`--mcp-trace` and `--mcp-verbose` list the PED calls** (mendixlabs/mxcli#1684, ako/mxcli#1833) — they printed each MDL statement but never a tool call, on `exec`, `-c`, the REPL and `mxcli mcp capabilities` alike. A run that sent every write to Studio Pro read as one that sent none, which is how a report concluded that `exec --mcp` wrote the `.mpr` instead. The backend the CLI builds for `--mcp` now gets the tracer too, so each statement is followed by its `→ ped_…` calls. - **A PED tool whose handler throws is reported as a failure** — Studio Pro answers `Error executing tool handler: …` with `isError:false` (measured on 11.16.0), which a caller trusting the flag read as success; the MCP client now classifies that text as an error. - **`describe` and the source catalog no longer decode every document of a type to find one** (ako/mxcli#1850) — describing a snippet decoded every snippet of the project, a nanoflow every microflow and nanoflow, a page every page, and so did resolving the snippets, pages, layouts and flows a description names. A source catalog build describes every document, so it paid that once per document: on FactoryManagement a first `refresh catalog full source` took 33 s and 145 s of CPU, and now takes 19 s and 38 s, with identical text for all 3,718 documents. `create nanoflow` likewise decodes only the nanoflows of its own name. - **Refreshing the catalog after a model edit no longer spends seconds on widget definitions** (ako/mxcli#1848) — `refresh catalog` re-read every widget package on every run, and on FactoryManagement it took about 3 s of a 5 s refresh on Windows. The step is now skipped when no `.mpk` or `.def.json` file has changed since the last refresh by the same mxcli build, and reading a widget's editor rules is no longer quadratic in the size of its `editorConfig.js` (Maps' took 0.5 s and now takes 15 ms, with identical rules on 256 widgets). Two widgets that derive the same keyword, such as `KeyboardShortcut.mpk` and `itvisors.KeyboardShortcut.mpk`, no longer overwrite each other's definition: the pluggable one keeps `KEYBOARDSHORTCUT` and the other becomes `CUSTOMKEYBOARDSHORTCUT`. Measured on a Mac, a refresh after one edit went from 2.8–3.2 s to 1.0–1.4 s. diff --git a/cmd/mxcli/engine.go b/cmd/mxcli/engine.go index 992d4ce567..74a17c9059 100644 --- a/cmd/mxcli/engine.go +++ b/cmd/mxcli/engine.go @@ -56,8 +56,13 @@ func newBackendFactory() func() backend.FullBackend { url, dial := globalMCPURL, globalMCPDial concordURL, concordDial := globalMCPConcord, globalMCPConcordDial saveOnExit, checkOnExit, runOnExit := globalMCPSave, globalMCPCheck, globalMCPRun + // --mcp-trace / --mcp-verbose: the executor prints each statement, the + // backend's client each PED call under it. Without the backend's half + // the trace showed statements only, which reads as "nothing was sent to + // Studio Pro" (mendixlabs/mxcli#1684). + tracer := mcpTracer() return func() backend.FullBackend { - b := mcpbackend.New(url, dial) + b := mcpbackend.New(url, dial).WithTracer(tracer) if concordURL != "" || saveOnExit || checkOnExit || runOnExit { b = b.WithConcord(mcpbackend.ConcordConfig{ URL: concordURL, Dial: concordDial, diff --git a/cmd/mxcli/mcp.go b/cmd/mxcli/mcp.go index 016eb42f4e..14171ec99a 100644 --- a/cmd/mxcli/mcp.go +++ b/cmd/mxcli/mcp.go @@ -37,7 +37,7 @@ Example: fmt.Fprintln(os.Stderr, "Error: mcp capabilities requires -p (reads come from the local project)") os.Exit(1) } - b := mcpbackend.New(globalMCPURL, globalMCPDial) + b := mcpbackend.New(globalMCPURL, globalMCPDial).WithTracer(mcpTracer()) if globalMCPConcord != "" { b = b.WithConcord(mcpbackend.ConcordConfig{URL: globalMCPConcord, Dial: globalMCPConcordDial}) } diff --git a/cmd/mxcli/mcp_trace_test.go b/cmd/mxcli/mcp_trace_test.go new file mode 100644 index 0000000000..9de5c2a0ac --- /dev/null +++ b/cmd/mxcli/mcp_trace_test.go @@ -0,0 +1,157 @@ +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "bytes" + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "sync" + "testing" + + "github.com/mendixlabs/mxcli/mdl/visitor" +) + +// mendixlabs/mxcli#1684 (ako/mxcli#1833): "`--mcp-trace` lists no tool call", +// read as proof that `exec --mcp` never reached Studio Pro. The calls were +// made; the trace could not show them. The executor got the tracer and printed +// each statement, but the MCP backend the factory built never did +// (mcpbackend.WithTracer had no caller), so its client printed no call lines. + +// fakeStudioPro answers the PED tools a `create constant` makes and records +// the tool names it was asked for. +func fakeStudioPro(t *testing.T) (*httptest.Server, func() []string) { + t.Helper() + var mu sync.Mutex + var calls []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, _ := io.ReadAll(r.Body) + var req struct { + ID *int `json:"id"` + Method string `json:"method"` + Params struct { + Name string `json:"name"` + } `json:"params"` + } + _ = json.Unmarshal(body, &req) + w.Header().Set("Mcp-Session-Id", "test") + if req.ID == nil { + w.WriteHeader(http.StatusAccepted) + return + } + var result any + switch req.Method { + case "initialize": + result = map[string]any{"protocolVersion": "2025-06-18", + "serverInfo": map[string]any{"name": "fake-studio-pro", "version": "1.0.0"}} + case "tools/list": + result = map[string]any{"tools": []any{}} + case "tools/call": + mu.Lock() + calls = append(calls, req.Params.Name) + mu.Unlock() + text := "SUCCESS" + if req.Params.Name == "ped_check_errors" { + text = "No errors found." + } + result = map[string]any{"isError": false, "content": []map[string]any{{"type": "text", "text": text}}} + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{"jsonrpc": "2.0", "id": req.ID, "result": result}) + })) + t.Cleanup(srv.Close) + return srv, func() []string { + mu.Lock() + defer mu.Unlock() + return append([]string(nil), calls...) + } +} + +// withMCPFlags sets the --mcp globals for one test. +func withMCPFlags(t *testing.T, url, dial string, trace bool) { + t.Helper() + saved := []any{globalMCPURL, globalMCPDial, globalMCPTrace, globalMCPVerbose} + globalMCPURL, globalMCPDial, globalMCPTrace, globalMCPVerbose = url, dial, trace, false + t.Cleanup(func() { + globalMCPURL, globalMCPDial = saved[0].(string), saved[1].(string) + globalMCPTrace, globalMCPVerbose = saved[2].(bool), saved[3].(bool) + }) +} + +// stderrOf runs fn with os.Stderr captured; the tracers read os.Stderr when +// they are built, so the executor must be built inside fn. +func stderrOf(t *testing.T, fn func()) string { + t.Helper() + r, w, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + saved := os.Stderr + os.Stderr = w + var buf bytes.Buffer + done := make(chan struct{}) + go func() { _, _ = io.Copy(&buf, r); close(done) }() + defer func() { + os.Stderr = saved + _ = w.Close() + <-done + }() + fn() + _ = w.Close() + <-done + os.Stderr = saved + return buf.String() +} + +func TestMCPTrace_ListsThePEDCallsOfAWrite(t *testing.T) { + srv, calls := fakeStudioPro(t) + withMCPFlags(t, srv.URL+"/mcp", strings.TrimPrefix(srv.URL, "http://"), true) + dst := t.TempDir() + if err := os.CopyFS(dst, os.DirFS("../../testdata/expr-checker")); err != nil { + t.Fatalf("copy fixture: %v", err) + } + + var out bytes.Buffer + stderr := stderrOf(t, func() { + exec, logger := newLoggedExecutorTo("test", &out) + defer logger.Close() + defer exec.Close() + prog, errs := visitor.Build("CONNECT LOCAL '" + visitor.QuoteString(filepath.Join(dst, "minimal.mpr")) + "';\n" + + "create constant MyFirstModule.TraceProbe (Type: String, DefaultValue: 'x');") + if len(errs) > 0 { + t.Fatal(errs) + } + for _, stmt := range prog.Statements { + if err := exec.Execute(stmt); err != nil { + t.Fatalf("%v\n%s", err, out.String()) + } + } + }) + + got := calls() + if len(got) == 0 || !contains(got, "ped_create_document") { + t.Fatalf("the fake Studio Pro received %v, want a ped_create_document", got) + } + if !strings.Contains(stderr, "▸ CreateConstantStmt") { + t.Fatalf("--mcp-trace printed no statement header, so tracing is off altogether:\n%s", stderr) + } + for _, tool := range []string{"ped_create_document", "ped_check_errors"} { + if !strings.Contains(stderr, "→ "+tool) { + t.Errorf("--mcp-trace does not list %s, which Studio Pro received (%v):\n%s", tool, got, stderr) + } + } +} + +func contains(xs []string, x string) bool { + for _, s := range xs { + if s == x { + return true + } + } + return false +} From bf4e7531bfd56ee25bd34651a8b54f949788a0b4 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 1 Nov 2026 17:33:27 +0000 Subject: [PATCH 10/10] fix(mcp): dial loopback before host.docker.internal (#1833) Without --mcp-dial, a localhost/127.0.0.1 endpoint was dialled as host.docker.internal: whenever that name resolved, taken to mean "inside a container". Docker Desktop for Windows puts the name in the host's own hosts file, pointing at the LAN address, and Studio Pro listens on loopback only, so `--mcp http://localhost:7782/mcp` timed out on the machine Studio Pro runs on (`dial tcp :7782: i/o timeout`, mendixlabs/mxcli#1684). The client now dials a plan: --mcp-dial alone when given, otherwise the URL host first and host.docker.internal only as the fallback, reporting every failed attempt. A devcontainer pays one refused loopback connection. TestClient_LocalhostReachesStudioProOnTheHostWhenTheDockerNameResolves forces the name to count as resolving and connects to a loopback fake; with the old rewrite restored it fails dialling host.docker.internal. cmd/mcpprobe keeps its own always-rewrite dial: it is a devcontainer probe. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01NUbjJ5DeS2vysn9hfKir4Y --- ...ost-dials-docker-name-on-windows-host.json | 1 + CHANGELOG.md | 1 + docs-site/src/tools/mcp-connect.md | 10 +- .../bug-tests/1833-mcp-trace-and-dial.mdl | 20 ++++ mdl/backend/mcp/client.go | 105 ++++++++++++------ mdl/backend/mcp/client_dial_test.go | 93 ++++++++++++++-- mdl/backend/mcp/connect_reads_test.go | 9 +- 7 files changed, 188 insertions(+), 51 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/mdl-backend/2026-11-01-mcp-localhost-dials-docker-name-on-windows-host.json create mode 100644 mdl-examples/bug-tests/1833-mcp-trace-and-dial.mdl diff --git a/.claude/skills/fix-issue/findings/mdl-backend/2026-11-01-mcp-localhost-dials-docker-name-on-windows-host.json b/.claude/skills/fix-issue/findings/mdl-backend/2026-11-01-mcp-localhost-dials-docker-name-on-windows-host.json new file mode 100644 index 0000000000..f49df86c42 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-backend/2026-11-01-mcp-localhost-dials-docker-name-on-windows-host.json @@ -0,0 +1 @@ +{"area": "mdl/backend", "date": "2026-11-01", "symptom": "On Windows with Docker Desktop, `mxcli mcp capabilities -p App.mpr --mcp http://localhost:7782/mcp` failed with `dial tcp :7782: i/o timeout` while Studio Pro listened on 127.0.0.1; `--mcp-dial 127.0.0.1:7782` connected. The reporter blamed localhost resolving to the LAN address (mendixlabs/mxcli#1684)", "cause": "mdl/backend/mcp/client.go rewrote a localhost/127.0.0.1 endpoint to host.docker.internal: whenever net.LookupHost(host.docker.internal) succeeded, as a 'running in a devcontainer' test. Docker Desktop for Windows writes host.docker.internal into the host's own hosts file (the LAN IP), so the rewrite also fired on the host, for 127.0.0.1 URLs too", "file": "mdl/backend/mcp/client.go (dialPlan, dialFirst, newHTTPClient)", "fix": "Dial a plan instead of one address: an explicit --mcp-dial alone, otherwise the URL host first and host.docker.internal: as the fallback when it resolves; every failure is reported when none connects", "insight": "'A name resolves' is evidence about DNS, not about where the process runs; the same Docker name is injected into containers and into Windows hosts. Prefer trying the likely address and falling back over guessing the environment: a refused loopback connection in a container costs nothing. The address in the error (a LAN IP for a localhost URL) pointed at the rewrite, not at localhost resolution. Test it by forcing the cached 'resolves' answer true and connecting to a loopback fake", "refs": "ako/mxcli#1833, mendixlabs/mxcli#1684", "test": "mdl/backend/mcp/client_dial_test.go"} diff --git a/CHANGELOG.md b/CHANGELOG.md index d34bfcd2ba..a1d1d7dff3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **`--mcp http://localhost:/mcp` reaches Studio Pro on a Windows machine with Docker Desktop** (mendixlabs/mxcli#1684, ako/mxcli#1833) — without `--mcp-dial`, mxcli dialled `host.docker.internal` whenever that name resolved, taking it to mean "inside a container". Docker Desktop for Windows puts the name in the hosts file, pointing at the LAN address, and Studio Pro listens on loopback only, so the connection timed out (`dial tcp :7782: i/o timeout`) on the very machine Studio Pro runs on. mxcli now dials the URL's own host first and tries `host.docker.internal` only when that fails, so the same command still works from a devcontainer. `--mcp-dial` is unchanged: when given, it is the only address tried. - **`--mcp-trace` and `--mcp-verbose` list the PED calls** (mendixlabs/mxcli#1684, ako/mxcli#1833) — they printed each MDL statement but never a tool call, on `exec`, `-c`, the REPL and `mxcli mcp capabilities` alike. A run that sent every write to Studio Pro read as one that sent none, which is how a report concluded that `exec --mcp` wrote the `.mpr` instead. The backend the CLI builds for `--mcp` now gets the tracer too, so each statement is followed by its `→ ped_…` calls. - **A PED tool whose handler throws is reported as a failure** — Studio Pro answers `Error executing tool handler: …` with `isError:false` (measured on 11.16.0), which a caller trusting the flag read as success; the MCP client now classifies that text as an error. - **`describe` and the source catalog no longer decode every document of a type to find one** (ako/mxcli#1850) — describing a snippet decoded every snippet of the project, a nanoflow every microflow and nanoflow, a page every page, and so did resolving the snippets, pages, layouts and flows a description names. A source catalog build describes every document, so it paid that once per document: on FactoryManagement a first `refresh catalog full source` took 33 s and 145 s of CPU, and now takes 19 s and 38 s, with identical text for all 3,718 documents. `create nanoflow` likewise decodes only the nanoflows of its own name. diff --git a/docs-site/src/tools/mcp-connect.md b/docs-site/src/tools/mcp-connect.md index 8386daeef1..8c43aa6d40 100644 --- a/docs-site/src/tools/mcp-connect.md +++ b/docs-site/src/tools/mcp-connect.md @@ -41,9 +41,13 @@ of routing through MCP, see | `--mcp-verbose` | Print each PED tool call the backend makes (to stderr). | | `--mcp-trace` | Print each MDL command with the PED tool calls it makes (implies `--mcp-verbose`). | -If `--mcp-dial` is omitted and the URL host is `localhost`/`127.0.0.1`, mxcli -defaults the dial target to `host.docker.internal:` — the right behaviour -from inside a devcontainer. +If `--mcp-dial` is omitted, mxcli connects to the URL's own host first. For a +`localhost`/`127.0.0.1` URL it then falls back to `host.docker.internal:` +when that name resolves — the Docker host, which is where Studio Pro is when +mxcli runs in a devcontainer. Loopback comes first because the Docker name +resolves on more machines than containers: Docker Desktop for Windows adds it to +the hosts file, pointing at the LAN address, where Studio Pro does not listen. +With `--mcp-dial` given, that address is the only one tried. ## From a devcontainer (the common case) diff --git a/mdl-examples/bug-tests/1833-mcp-trace-and-dial.mdl b/mdl-examples/bug-tests/1833-mcp-trace-and-dial.mdl new file mode 100644 index 0000000000..c43960cc48 --- /dev/null +++ b/mdl-examples/bug-tests/1833-mcp-trace-and-dial.mdl @@ -0,0 +1,20 @@ +-- ako/mxcli#1833 (mendixlabs/mxcli#1684): two defects of `--mcp`, found while +-- checking a report that `exec --mcp` wrote the .mpr instead of Studio Pro. +-- +-- 1. `--mcp-trace` / `--mcp-verbose` printed each statement but no PED call: +-- the MCP backend the CLI builds never got the tracer. A run that sent the +-- write to Studio Pro read as one that sent nothing. +-- 2. Without `--mcp-dial`, a `localhost` URL was dialled as +-- host.docker.internal whenever that name resolved. Docker Desktop for +-- Windows adds it to the hosts file (the LAN address), so on the machine +-- Studio Pro runs on the connection timed out: `dial tcp :7782`. +-- +-- To verify, with Studio Pro open on a throwaway app and its MCP server on 7782: +-- mxcli exec 1833-mcp-trace-and-dial.mdl -p App.mpr --mcp http://localhost:7782/mcp --mcp-trace +-- The trace lists `→ ped_create_document` and `→ ped_check_errors` under the +-- statement, and the connection needs no --mcp-dial (on Windows with Docker +-- Desktop installed too). The constant appears in Studio Pro's App Explorer. + +mdl 1; + +create constant MyFirstModule.McpTraceProbe (Type: String, DefaultValue: 'probe'); diff --git a/mdl/backend/mcp/client.go b/mdl/backend/mcp/client.go index 5385d9c0b4..9194e79fcb 100644 --- a/mdl/backend/mcp/client.go +++ b/mdl/backend/mcp/client.go @@ -54,8 +54,9 @@ type ClientOptions struct { URL string // Dial overrides the TCP address actually connected to, independent of the // URL host. From a devcontainer this is typically host.docker.internal:7782. - // When empty it defaults to the URL host, or to host.docker.internal: - // when the URL host is localhost/127.0.0.1. + // When empty the client dials the URL host, and for a localhost/127.0.0.1 + // URL falls back to host.docker.internal: when that fails and the + // name resolves (dialPlan). Dial string // Protocol is the MCP protocol version advertised (default 2025-06-18). Protocol string @@ -72,10 +73,7 @@ func NewClient(opts ClientOptions) (*Client, error) { if u.Host == "" { return nil, fmt.Errorf("MCP URL %q has no host", opts.URL) } - dial := opts.Dial - if dial == "" { - dial = defaultDial(u.Host) - } + plan := dialPlan(u.Host, opts.Dial, opts.Dial == "" && hostDockerInternalReachable()) protocol := opts.Protocol if protocol == "" { protocol = "2025-06-18" @@ -87,36 +85,73 @@ func NewClient(opts ClientOptions) (*Client, error) { timeout = 90 * time.Second } return &Client{ - http: newHTTPClient(dial, timeout), + http: newHTTPClient(plan, timeout), endpoint: opts.URL, protocol: protocol, }, nil } -// defaultDial maps a localhost endpoint to the Docker host gateway so the client -// works from inside a devcontainer, while the Host header stays localhost. The -// rewrite happens only when host.docker.internal is actually reachable — i.e. -// inside a container. On the host (e.g. macOS, where host.docker.internal does -// not resolve) it dials localhost directly, so `--mcp http://localhost:PORT/mcp` -// works without an explicit --mcp-dial. -func defaultDial(host string) string { - return dialFor(host, hostDockerInternalReachable()) +// dialPlan is the decision behind the client's dialer: the TCP addresses to +// try, in order. An explicit --mcp-dial is the only one. Otherwise the URL's +// own host comes first, and a localhost/127.0.0.1 endpoint falls back to +// host.docker.internal: when that name resolves, so the same command +// works from a devcontainer, where loopback is the container and Studio Pro is +// on the Docker host. +// +// The Docker name is the FALLBACK, never the first choice: "it resolves" does +// not mean "inside a container". Docker Desktop for Windows writes the name +// into the host's own hosts file, pointing at the machine's LAN address, and +// Studio Pro listens on loopback only, so dialling it first timed out on the +// very machine Studio Pro runs on (mendixlabs/mxcli#1684). Loopback first costs +// a container one refused connection, which fails at once. +func dialPlan(urlHost, explicit string, dockerResolves bool) []string { + if explicit != "" { + return []string{explicit} + } + h, port, err := net.SplitHostPort(urlHost) + if err != nil { // no port + h, port = urlHost, "80" + } + plan := []string{net.JoinHostPort(h, port)} + if (h == "localhost" || h == "127.0.0.1") && dockerResolves { + plan = append(plan, net.JoinHostPort(dockerHostName, port)) + } + return plan } -// dialFor is the pure decision behind defaultDial: rewrite a localhost endpoint to -// the docker host gateway only when that gateway is reachable. Split out so both -// branches are unit-testable without depending on the runtime environment. -func dialFor(host string, dockerReachable bool) string { - h, port, err := net.SplitHostPort(host) - if err != nil { // no port - h, port = host, "80" +// dialFirst connects to the first address in plan that accepts, and reports +// every failure when none does. +func dialFirst(ctx context.Context, plan []string, dial func(ctx context.Context, addr string) (net.Conn, error)) (net.Conn, error) { + var errs dialErrors + for _, addr := range plan { + conn, err := dial(ctx, addr) + if err == nil { + return conn, nil + } + if ctx.Err() != nil { + return nil, err + } + errs = append(errs, err) + } + if len(errs) == 1 { + return nil, errs[0] } - if (h == "localhost" || h == "127.0.0.1") && dockerReachable { - return net.JoinHostPort(dockerHostName, port) + return nil, errs +} + +// dialErrors is every failed attempt of one dialFirst, in order. +type dialErrors []error + +func (e dialErrors) Error() string { + parts := make([]string, len(e)) + for i, err := range e { + parts[i] = err.Error() } - return host + return strings.Join(parts, "; then ") } +func (e dialErrors) Unwrap() []error { return e } + // dockerHostName is the gateway hostname Docker injects for reaching the host. const dockerHostName = "host.docker.internal" @@ -125,9 +160,11 @@ var dockerHostOnce struct { reachable bool } -// hostDockerInternalReachable reports whether host.docker.internal resolves -// (true inside a Docker/devcontainer with the host-gateway mapping, false on a -// bare host). Resolved once and cached. +// hostDockerInternalReachable reports whether host.docker.internal resolves: +// inside a Docker/devcontainer with the host-gateway mapping, and also on a +// Windows host with Docker Desktop, which puts the name in the hosts file. So +// it decides whether a fallback exists, not where mxcli runs (dialPlan). +// Resolved once and cached. func hostDockerInternalReachable() bool { dockerHostOnce.Do(func() { _, err := net.LookupHost(dockerHostName) @@ -136,14 +173,16 @@ func hostDockerInternalReachable() bool { return dockerHostOnce.reachable } -func newHTTPClient(dialAddr string, timeout time.Duration) *http.Client { - // Pin every connection to dialAddr regardless of the request URL host, so - // the URL (and thus the Host header) can stay "localhost" while we actually - // connect to the host gateway. +func newHTTPClient(plan []string, timeout time.Duration) *http.Client { + // Pin every connection to the plan's addresses regardless of the request URL + // host, so the URL (and thus the Host header) can stay "localhost" while we + // may actually connect to the host gateway. tr := &http.Transport{ DialContext: func(ctx context.Context, network, _ string) (net.Conn, error) { d := net.Dialer{Timeout: 5 * time.Second} - return d.DialContext(ctx, network, dialAddr) + return dialFirst(ctx, plan, func(ctx context.Context, addr string) (net.Conn, error) { + return d.DialContext(ctx, network, addr) + }) }, ForceAttemptHTTP2: false, } diff --git a/mdl/backend/mcp/client_dial_test.go b/mdl/backend/mcp/client_dial_test.go index 2e0dd839d2..dddc00ad62 100644 --- a/mdl/backend/mcp/client_dial_test.go +++ b/mdl/backend/mcp/client_dial_test.go @@ -2,26 +2,99 @@ package mcp -import "testing" +import ( + "context" + "errors" + "net" + "strings" + "testing" + "time" +) -func TestDialFor(t *testing.T) { +func TestDialPlan(t *testing.T) { cases := []struct { name string host string + dial string dockerable bool - want string + want []string }{ - {"localhost in container → docker gateway", "localhost:7784", true, "host.docker.internal:7784"}, - {"localhost on host → localhost (Issue 8)", "localhost:7784", false, "localhost:7784"}, - {"127.0.0.1 on host → unchanged", "127.0.0.1:7782", false, "127.0.0.1:7782"}, - {"explicit host always passthrough", "myhost:9000", true, "myhost:9000"}, - {"localhost no port in container", "localhost", true, "host.docker.internal:80"}, + {"localhost where the docker name resolves: loopback, then the gateway", "localhost:7784", "", true, []string{"localhost:7784", "host.docker.internal:7784"}}, + {"localhost where it does not (macOS host, Issue 8)", "localhost:7784", "", false, []string{"localhost:7784"}}, + {"127.0.0.1 where the docker name resolves", "127.0.0.1:7782", "", true, []string{"127.0.0.1:7782", "host.docker.internal:7782"}}, + {"127.0.0.1 where it does not", "127.0.0.1:7782", "", false, []string{"127.0.0.1:7782"}}, + {"another host has no fallback", "myhost:9000", "", true, []string{"myhost:9000"}}, + {"no port is port 80", "localhost", "", true, []string{"localhost:80", "host.docker.internal:80"}}, + {"--mcp-dial is the only address", "localhost:7782", "127.0.0.1:7782", true, []string{"127.0.0.1:7782"}}, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { - if got := dialFor(c.host, c.dockerable); got != c.want { - t.Errorf("dialFor(%q, %v) = %q, want %q", c.host, c.dockerable, got, c.want) + got := dialPlan(c.host, c.dial, c.dockerable) + if strings.Join(got, ",") != strings.Join(c.want, ",") { + t.Errorf("dialPlan(%q, %q, %v) = %v, want %v", c.host, c.dial, c.dockerable, got, c.want) } }) } } + +func TestDialFirst_TriesThePlanInOrderAndReportsEveryFailure(t *testing.T) { + var tried []string + refuse := func(_ context.Context, addr string) (net.Conn, error) { + tried = append(tried, addr) + return nil, errors.New("dial " + addr + ": refused") + } + _, err := dialFirst(context.Background(), []string{"a:1", "b:1"}, refuse) + if strings.Join(tried, ",") != "a:1,b:1" { + t.Errorf("tried %v, want a:1 then b:1", tried) + } + if err == nil || !strings.Contains(err.Error(), "a:1") || !strings.Contains(err.Error(), "b:1") { + t.Errorf("the error does not name both attempts: %v", err) + } + + tried = nil + client, server := net.Pipe() + defer server.Close() + acceptSecond := func(_ context.Context, addr string) (net.Conn, error) { + tried = append(tried, addr) + if addr == "b:1" { + return client, nil + } + return nil, errors.New("refused") + } + conn, err := dialFirst(context.Background(), []string{"a:1", "b:1", "c:1"}, acceptSecond) + if err != nil || conn != client { + t.Fatalf("dialFirst = %v, %v; want the second address's connection", conn, err) + } + if strings.Join(tried, ",") != "a:1,b:1" { + t.Errorf("tried %v, want to stop at the first that accepts", tried) + } +} + +// pretendDockerHostResolves makes host.docker.internal count as resolving, as +// it does on a Windows host with Docker Desktop installed: Docker Desktop +// writes the name into the hosts file, pointing at the machine's LAN address. +func pretendDockerHostResolves(t *testing.T) { + t.Helper() + dockerHostOnce.Do(func() {}) + saved := dockerHostOnce.reachable + dockerHostOnce.reachable = true + t.Cleanup(func() { dockerHostOnce.reachable = saved }) +} + +// mendixlabs/mxcli#1684 (ako/mxcli#1833): on Windows with Docker Desktop, +// `--mcp http://localhost:7782/mcp` without --mcp-dial failed with +// `dial tcp :7782: i/o timeout` while Studio Pro listened on +// 127.0.0.1. "host.docker.internal resolves" was taken to mean "inside a +// container", so the client dialled the Docker name instead of loopback. +func TestClient_LocalhostReachesStudioProOnTheHostWhenTheDockerNameResolves(t *testing.T) { + pretendDockerHostResolves(t) + ped := newFakePED(t, nil) + port := ped.srv.URL[strings.LastIndex(ped.srv.URL, ":")+1:] + c, err := NewClient(ClientOptions{URL: "http://localhost:" + port + "/mcp", Timeout: 10 * time.Second}) + if err != nil { + t.Fatal(err) + } + if _, err := c.Initialize(); err != nil { + t.Fatalf("a Studio Pro listening on loopback was not reached: %v", err) + } +} diff --git a/mdl/backend/mcp/connect_reads_test.go b/mdl/backend/mcp/connect_reads_test.go index 1434dcb004..35add397e7 100644 --- a/mdl/backend/mcp/connect_reads_test.go +++ b/mdl/backend/mcp/connect_reads_test.go @@ -36,11 +36,10 @@ func localProject(t *testing.T) string { // backendFor returns an unconnected Backend pointed at the fake Studio Pro. // -// The dial address is passed explicitly, as connectClient does. An empty one -// lets defaultDial rewrite 127.0.0.1 to host.docker.internal wherever that name -// resolves — correct for a real Studio Pro on the host, but inside a devcontainer -// it sends the request to the Docker host instead of the httptest listener in -// this container, and Connect fails with "connection refused". +// The dial address is passed explicitly, as connectClient does, so the test +// reaches the httptest listener whatever the environment. (Without it the +// client dials the URL host first and falls back to host.docker.internal only +// if that fails; see dialPlan.) func backendFor(ped *fakePED) *Backend { return New(ped.srv.URL+"/mcp", strings.TrimPrefix(ped.srv.URL, "http://")) }