From 3ebd76f9c16d34c7fac92749c0ddc0211d778869 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 13 Oct 2026 05:22:59 +0000 Subject: [PATCH 01/17] ci: add non-gating Mendix 9.24 nightly job and gate 10.x-only doctype sections Add a separate test-mendix-9 job to the nightly workflow running the integration suite against Mendix 9.24.30.51827. It is not a `test` matrix entry because the `nightly` release job needs `test`, and 9.x still has known writer bugs; continue-on-error keeps a red run from failing the workflow. The job installs libgdiplus (mxbuild 9 needs System.Drawing) and pins JAVA_HOME to the runner's JDK 17. Fold it into the matrix once it is green. nightlyMatrix in doctype_version_gating_test.go mirrors nightly.yml: add 9.24.30.51827, and the 11.14.0 / 11.15.0 entries it was missing. Gate the doctype statements that mxcli correctly refuses on 9.24: - 06c-rest-client-mendix10: whole script 10.1+ (consumed REST clients) - 22-published-rest-service: PART 2 onward 10.0+ (registry floor) - 30-module-jar-dependencies: whole script 10.0+ (JarDependencies) - 34-chart-widget: view entities and every chart page bound to them 10.18+ - microflow-document-properties: the URL / DROP URL statements 10.6+ - 14-project-settings: JavaVersion split into its own 10.8+ alter; the 10.6/10.11/10.21 runtime settings split out of Example 1.5 as 10.21+ Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01W3x8Fyvir8kLyPgJs1BDmF --- .github/workflows/nightly.yml | 57 +++++++++++++++++++ .../06c-rest-client-mendix10-examples.mdl | 8 ++- .../14-project-settings-examples.mdl | 32 +++++++++-- .../22-published-rest-service-examples.mdl | 6 ++ .../30-module-jar-dependencies.mdl | 3 + .../34-chart-widget-examples.mdl | 5 ++ .../microflow-document-properties.mdl | 11 ++++ mdl/executor/doctype_version_gating_test.go | 4 ++ 8 files changed, 117 insertions(+), 9 deletions(-) diff --git a/.github/workflows/nightly.yml b/.github/workflows/nightly.yml index e2faf8b5f3..3af281ce5a 100644 --- a/.github/workflows/nightly.yml +++ b/.github/workflows/nightly.yml @@ -70,6 +70,63 @@ jobs: run: make test-integration timeout-minutes: 60 + # Mendix 9.24 (the last 9.x LTS), kept OUT of the `test` matrix on purpose: + # `nightly` needs `test`, and 9.x still has known writer bugs, so a matrix + # entry would block every release until they are fixed. continue-on-error + # keeps a red run from failing the workflow while the failures stay visible + # in the job log. Once this job is green, fold the version into the `test` + # matrix and delete this job. + test-mendix-9: + runs-on: ubuntu-latest + continue-on-error: true + env: + MENDIX_VERSION: '9.24.30.51827' + name: test (Mendix 9.24.30.51827, non-gating) + steps: + - uses: actions/checkout@v7 + with: + submodules: true + + - uses: actions/setup-go@v7 + with: + go-version: '1.27.2' + + # mxbuild 9 draws through System.Drawing; without libgdiplus every mx call + # dies with "The type initializer for 'Gdip' threw an exception." + - name: Install libgdiplus + run: sudo apt-get update && sudo apt-get install -y libgdiplus + + # The 9.x runtime and its Gradle build need JDK 17; pin it rather than + # relying on the runner image's default JAVA_HOME. + - uses: actions/setup-java@v5 + with: + distribution: temurin + java-version: '17' + + - name: Cache ANTLR4 JAR + uses: actions/cache@v6 + with: + path: ~/.m2/repository/org/antlr/antlr4 + key: antlr4-4.13.2 + - name: Install ANTLR4 + run: pip install 'antlr4-tools==0.2.2' + - name: Generate parser + run: make grammar + env: + ANTLR4_TOOLS_ANTLR_VERSION: '4.13.2' + - name: Build + run: make build + + # No `make test`: the unit tests are version-independent and already run + # in every `test` matrix entry. + + - name: Setup mxbuild ${{ env.MENDIX_VERSION }} + run: ./bin/mxcli setup mxbuild --version ${{ env.MENDIX_VERSION }} + + - name: "Integration tests (Mendix ${{ env.MENDIX_VERSION }})" + run: make test-integration + timeout-minutes: 60 + # The upgrade property test over the WHOLE mdl-examples corpus, header-only # scripts included (MXCLI_UPGRADE_ALL). Per-PR CI executes only the scripts # the upgrade rewrites beyond the header and terminators; this is the run that diff --git a/mdl-examples/doctype-tests/06c-rest-client-mendix10-examples.mdl b/mdl-examples/doctype-tests/06c-rest-client-mendix10-examples.mdl index 03ca367639..3ac3cd220c 100644 --- a/mdl-examples/doctype-tests/06c-rest-client-mendix10-examples.mdl +++ b/mdl-examples/doctype-tests/06c-rest-client-mendix10-examples.mdl @@ -1,6 +1,7 @@ mdl 1; +-- @version: 10.1+ -- ============================================================================ --- Consumed REST clients that every supported Mendix version can hold +-- Consumed REST clients that every Mendix version with them (10.1+) can hold -- ============================================================================ -- -- 06-rest-client-examples.mdl is gated to 11.0+ as a whole (it uses query @@ -8,8 +9,9 @@ mdl 1; -- consumed REST client to a Mendix 10 project in the doctype gate — and every -- POST/PUT/PATCH there was written with Rest$JsonBody, a type Mendix 10 does not -- have: `mx check` passed, but `mx convert` (and Studio Pro) could not load the --- project at all. This script is NOT version-gated, so the gate's --- `mx convert -p` property-set check runs it on 10.24 as well. +-- project at all. This script is gated only to 10.1+ -- the version that +-- introduced the consumed REST client, which mxcli correctly refuses below it -- +-- so the gate's `mx convert -p` property-set check runs it on 10.24 as well. -- -- On Mendix 10 a JSON body is written as the string body Mendix 10 has -- (Rest$StringBody); `describe` reads it back as `body: template`. diff --git a/mdl-examples/doctype-tests/14-project-settings-examples.mdl b/mdl-examples/doctype-tests/14-project-settings-examples.mdl index 0681703bed..8c6cde17ff 100644 --- a/mdl-examples/doctype-tests/14-project-settings-examples.mdl +++ b/mdl-examples/doctype-tests/14-project-settings-examples.mdl @@ -35,13 +35,23 @@ end; alter settings runtime ( AfterStartupMicroflow: 'MyModule.ASU_Startup' ); /** - * Example 1.2: Configure hash algorithm and Java version + * Example 1.2: Configure the hash algorithm + */ +alter settings runtime ( HashAlgorithm: 'BCrypt' ); + +-- The Java version is a model setting from Mendix 10.8 (on 9.x the platform +-- picks the JDK, and mxcli refuses the alter -- mendixlabs/mxcli#1438), so it is +-- its own gated statement rather than part of Example 1.2. +-- @version: 10.8+ +/** + * Example 1.2b: Configure the Java version * * Either spelling of the Java version is accepted -- 'Java21' or '21'. Mendix * stores it as "JavaVersion" = "Java21" up to 11.6 and as "JavaMajorVersion" = * "21" from 11.12; mxcli writes whichever dialect the project already uses. */ -alter settings runtime ( HashAlgorithm: 'BCrypt', JavaVersion: 'Java21' ); +alter settings runtime ( JavaVersion: 'Java21' ); +-- @version: any /** * Example 1.3: Multiple model settings at once @@ -86,16 +96,26 @@ alter settings runtime ( EnableDataStorageOptimisticLocking: true ); * * That refusal covers the WHOLE statement, so a version-specific setting has to * be its own alter -- naming it alongside portable ones takes them down with it. - * These five are stored by every version in the test matrix (measured on a blank - * 10.24: all five present, and each accepted on its own). + * These two are stored by every version in the test matrix, 9.24 included. */ alter settings runtime ( FirstDayOfWeek: 'Monday', + DefaultTimeZoneCode: 'Europe/Amsterdam' +); + +-- UseDatabaseForeignKeyConstraints (10.6), UseOQLVersion2 (10.11) and +-- SslCertificateAlgorithm (10.21) are absent from a 9.24 project; measured on a +-- blank 10.24, all three are present and each accepted on its own. +-- @version: 10.21+ +/** + * Example 1.5b: Runtime model settings newer versions store + */ +alter settings runtime ( UseDatabaseForeignKeyConstraints: true, UseOQLVersion2: true, - SslCertificateAlgorithm: 'PKIX', - DefaultTimeZoneCode: 'Europe/Amsterdam' + SslCertificateAlgorithm: 'PKIX' ); +-- @version: any -- The @version directive opens the gated section, and everything up to the next -- directive is removed on a version that does not match. The `/** */` block diff --git a/mdl-examples/doctype-tests/22-published-rest-service-examples.mdl b/mdl-examples/doctype-tests/22-published-rest-service-examples.mdl index 801b5219b2..9ac6e06863 100644 --- a/mdl-examples/doctype-tests/22-published-rest-service-examples.mdl +++ b/mdl-examples/doctype-tests/22-published-rest-service-examples.mdl @@ -51,6 +51,12 @@ begin return $Result; end; +-- mxcli writes published REST services from Mendix 10.0 (the +-- published_rest_service floor in sdk/versions/mendix-10.yaml) and refuses them +-- below it. Everything from here on creates, alters or grants on one, so the +-- section runs to the end of the file; the backing microflows above run on +-- every version. +-- @version: 10.0+ -- ############################################################################ -- PART 2: SIMPLE PUBLISHED REST SERVICE -- ############################################################################ diff --git a/mdl-examples/doctype-tests/30-module-jar-dependencies.mdl b/mdl-examples/doctype-tests/30-module-jar-dependencies.mdl index 80dbc7a1b3..a547b2097c 100644 --- a/mdl-examples/doctype-tests/30-module-jar-dependencies.mdl +++ b/mdl-examples/doctype-tests/30-module-jar-dependencies.mdl @@ -1,4 +1,7 @@ mdl 1; +-- Projects$ModuleSettings.JarDependencies exists from Mendix 10.0; mxcli refuses +-- it on 9.x, and every statement below is about it. +-- @version: 10.0+ -- ============================================================================ -- Module JAR Dependency Examples -- ============================================================================ diff --git a/mdl-examples/doctype-tests/34-chart-widget-examples.mdl b/mdl-examples/doctype-tests/34-chart-widget-examples.mdl index 19b6935e41..32ac162012 100644 --- a/mdl-examples/doctype-tests/34-chart-widget-examples.mdl +++ b/mdl-examples/doctype-tests/34-chart-widget-examples.mdl @@ -62,6 +62,11 @@ create persistent entity ChartExamples.Sales ( Units: integer ); +-- View entities exist from Mendix 10.18, and mxcli refuses them below it. Every +-- chart page that follows binds one as its data source, so the section runs from +-- here to the end of the file; only the module and the persistent entity above +-- run on older versions. +-- @version: 10.18+ -- ---------------------------------------------------------------------------- -- OQL VIEW ENTITIES — the chart data sources (these DO build today) -- ---------------------------------------------------------------------------- diff --git a/mdl-examples/doctype-tests/microflow-document-properties.mdl b/mdl-examples/doctype-tests/microflow-document-properties.mdl index dbd4097c16..ce6ef48fc9 100644 --- a/mdl-examples/doctype-tests/microflow-document-properties.mdl +++ b/mdl-examples/doctype-tests/microflow-document-properties.mdl @@ -15,6 +15,11 @@ CREATE OR MODIFY ENTITY Deeplink.Order ( OrderNumber: String(20) ); +-- A microflow URL (deep link) exists from Mendix 10.6, and mxcli refuses it below +-- that, so each statement that sets one is gated on its own. The rest of the +-- script -- export level, concurrent execution, and the re-runs that mention no +-- URL -- runs on every version. +-- @version: 10.6+ -- Everything at once. $Order is the PATH parameter, $Tab the query argument — -- they must be different parameters (CE5612 / MDL-MF02). CREATE OR MODIFY MICROFLOW Deeplink.ACT_ShowOrder ($Order: Deeplink.Order, $Tab: String) @@ -25,6 +30,7 @@ DISALLOW CONCURRENT EXECUTION ERROR MESSAGE 'This order is already being process BEGIN LOG INFO $Tab; END; +-- @version: any -- The error handler can be a microflow instead of a message. CREATE OR MODIFY MICROFLOW Deeplink.ACT_OnBusy () @@ -38,6 +44,7 @@ BEGIN LOG INFO 'importing'; END; +-- @version: 10.6+ -- A segment may carry an attribute path: {Order/OrderNumber} binds the ORDER -- parameter by one of its attributes, so the leading identifier is what has to -- name a parameter. @@ -46,6 +53,7 @@ URL 'order/by-number/{Order/OrderNumber}' BEGIN LOG INFO 'by number'; END; +-- @version: any -- Re-running a statement that mentions NONE of the clauses must leave all four -- alone. This is the #1120 rule, and the reason every clause is optional. @@ -62,8 +70,11 @@ BEGIN LOG INFO 'importing'; END; +-- DROP URL clears a URL, and is refused with it below 10.6. +-- @version: 10.6+ CREATE OR MODIFY MICROFLOW Deeplink.ACT_ShowByNumber ($Order: Deeplink.Order) DROP URL BEGIN LOG INFO 'no longer deep-linked'; END; +-- @version: any diff --git a/mdl/executor/doctype_version_gating_test.go b/mdl/executor/doctype_version_gating_test.go index e6f03a8d76..44111b59e7 100644 --- a/mdl/executor/doctype_version_gating_test.go +++ b/mdl/executor/doctype_version_gating_test.go @@ -19,10 +19,14 @@ import ( // nightly failure on the others, reported hours later against whatever landed in // between — which is how the DecimalScale gating below was found. var nightlyMatrix = []*mxversion.ProjectVersion{ + // 9.24 runs in its own non-gating job (test-mendix-9), not the matrix. + {MajorVersion: 9, MinorVersion: 24, ProductVersion: "9.24.30.51827"}, {MajorVersion: 10, MinorVersion: 24, ProductVersion: "10.24.24.119349"}, {MajorVersion: 11, MinorVersion: 6, ProductVersion: "11.6.8"}, {MajorVersion: 11, MinorVersion: 12, ProductVersion: "11.12.2"}, {MajorVersion: 11, MinorVersion: 13, ProductVersion: "11.13.0"}, + {MajorVersion: 11, MinorVersion: 14, ProductVersion: "11.14.0"}, + {MajorVersion: 11, MinorVersion: 15, ProductVersion: "11.15.0"}, } // TestDoctypeScriptsParseAfterVersionFiltering guards the whole doctype corpus From c2c9a23c75c7a4b3855dd2f6be85ea612c056232 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 13 Oct 2026 05:26:38 +0000 Subject: [PATCH 02/17] fix: escape a business event service's ServiceName in describe describe business event service wrote ServiceName and EventNamePrefix as a hand-rolled `'%s'`, so a value with an apostrophe (`Bob's orders`) came out as a statement that does not parse, and re-executing the describe output failed. Both go through mdlQuote, which writes the literal for the describe language. cmd_businessevents.go is added to the hand-rolled-literal source scan; the new test re-parses the describe output under mdl 0 and mdl 1 and compares the values, and fails with the reported parse error when the fix is reverted. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01P65SqmwwvbWdJVRwhYiMQw --- ...-event-service-unescaped-service-name.json | 1 + CHANGELOG.md | 1 + mdl/executor/cmd_businessevents.go | 8 +- ...md_businessevents_describe_quoting_test.go | 80 +++++++++++++++++++ .../issue1006_emitter_quoting_test.go | 1 + 5 files changed, 85 insertions(+), 6 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/mdl-executor/2026-10-13-describe-business-event-service-unescaped-service-name.json create mode 100644 mdl/executor/cmd_businessevents_describe_quoting_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-10-13-describe-business-event-service-unescaped-service-name.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-13-describe-business-event-service-unescaped-service-name.json new file mode 100644 index 0000000000..5f184f364e --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-13-describe-business-event-service-unescaped-service-name.json @@ -0,0 +1 @@ +{"date": "2026-10-13", "area": "mdl/executor", "symptom": "`describe business event service` of a service whose ServiceName or EventNamePrefix contains an apostrophe (`Bob's orders`) printed `ServiceName: 'Bob's orders'`: the output does not parse, so the describe → exec round trip fails with \"mismatched input 's' expecting {',', ')'}\".", "cause": "describeBusinessEventService wrote both values with a hand-rolled `'%s'`, the quotes added by hand and the value's own quotes never doubled. cmd_businessevents.go was not on TestDescribers_HaveNoHandRolledStringLiterals' file list, so the source scan that catches this never looked at it.", "file": "mdl/executor/cmd_businessevents.go (describeBusinessEventService: mdlQuote for ServiceName and EventNamePrefix); mdl/executor/issue1006_emitter_quoting_test.go (file added to the scan)", "insight": "The `'%s'` source scan only covers the files on its list, so adding the file is the first step of a quoting fix; the test that does not depend on a list is re-parsing the describe output and comparing the values, under both describe languages (mdl 0 escapes a backslash, mdl 1 does not). Writing that re-parse test immediately found two more defects in the same describer, both empty-collection branches: a service with no message describes as `{ }` and a message with no attribute as `message M () publish;`, neither of which the grammar accepts.", "refs": ["mdl/executor/cmd_businessevents_describe_quoting_test.go", "docs-wiki/bug-patterns/describe-round-trip-gaps.md"]} diff --git a/CHANGELOG.md b/CHANGELOG.md index ae13d611de..ab8083df83 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,6 +48,7 @@ Headline: **Documents mxcli writes now survive Studio Pro's merge and diff: ever ### Fixed +- **`describe business event service` escapes the service name and event name prefix** — a value with an apostrophe (`Bob's orders`) was printed as `ServiceName: 'Bob's orders'`, which does not parse, so re-executing the describe output failed. Both values are now written as proper MDL strings (`'Bob''s orders'`). - **`check -p` on `create or modify` of stored microflows is back to v0.24.0's speed** (mendixlabs/mxcli#1272) — the passes that predict exec's verdict and resolve references looked each flow up by name several times per statement, and every lookup re-read and fully decoded every microflow file in the project, so the cost per statement grew with the project. Each check pass now reads a unit file once, and a lookup by name or ID decodes only the flows it returns. On a blank 11.14 app with 200 microflows, re-declaring 25 / 100 / 200 of them: 3.5 / 11.7 / 23.7 s → 0.8 / 1.8 / 3.0 s (v0.24.0: 0.9 / 2.6 / 3.7 s). `exec` gains the same through its preflight and its own planning of each statement; an unchanged re-run of the 200 went from 40 s to 5 s. Nothing reported changes. - **Every project's docker stack is its own Compose project instead of all sharing the name `docker`** (mendixlabs/mxcli#1364) — mxcli runs `docker compose` in the project's `.docker/` folder and Compose names a stack after that folder, so every app was the project `docker`: containers `docker-db-1` / `docker-mendix-1` and volume `docker_postgres-data` were shared, a second app's `docker up` replaced the first app's containers, and two apps pointed at one Postgres volume. `mxcli docker init` now writes a top-level `name: "mxcli-"` into `docker-compose.yml` (template v3; `Customer Portal/app.mpr` becomes `mxcli-customer-portal`; `docker compose -p` and `COMPOSE_PROJECT_NAME` still override it), so the containers are `mxcli-customer-portal-db-1` and the volume `mxcli-customer-portal_postgres-data`. **Existing projects keep their stack and their data:** a compose file written by an older mxcli has no `name:`, nothing is rewritten without `--force`, and `docker init --force` on such a project writes `name: "docker"` (or the `COMPOSE_PROJECT_NAME` from its old `.env`, which `--force` rewrites and which overrides `name:` in Compose, carried into `name:`) — the name Compose was already using — and says so; a `name:` you set yourself is kept as well. Mxcli never renames a stack on its own, because a new name makes Compose start new containers with an **empty** database and leave the old volume behind. **To opt in** on a project that has `name: "docker"`, stop the stack (`docker compose down` in `.docker/`, without `-v`), change `name:` to a unique value such as `mxcli-myapp`, then `mxcli docker up`; the database starts empty, and the old data stays in `docker_postgres-data` (to carry it over, dump it with `pg_dump` before you change the name and restore it into the new stack). To check which name a stack uses: `docker compose config` in `.docker/` prints `name:`. Two app folders with the same name still collide — edit `name:` in one of them. - **A navigation sub-menu no longer keeps the action of the page item it replaces** (mendixlabs/mxcli#1341) — turning menu item `'Work'` into sub-menu `'Work'` with `create or replace navigation` (or `create or modify menu`) paired the two by caption and carried the page item's `Forms$FormAction` onto the sub-menu, reporting it as an action "MDL cannot express"; `mx check` then failed with CE0548 "Items with subitems cannot have an action themselves." A sub-menu is now always written with no action (measured on 11.14.0). diff --git a/mdl/executor/cmd_businessevents.go b/mdl/executor/cmd_businessevents.go index 1ff7d715a2..4285057d7f 100644 --- a/mdl/executor/cmd_businessevents.go +++ b/mdl/executor/cmd_businessevents.go @@ -211,12 +211,8 @@ func describeBusinessEventService(ctx *ExecContext, name ast.QualifiedName) erro if found.Definition != nil { fmt.Fprintf(ctx.Output, "(\n") - fmt.Fprintf(ctx.Output, " ServiceName: '%s'", found.Definition.ServiceName) - if found.Definition.EventNamePrefix != "" { - fmt.Fprintf(ctx.Output, ",\n EventNamePrefix: '%s'", found.Definition.EventNamePrefix) - } else { - fmt.Fprintf(ctx.Output, ",\n EventNamePrefix: ''") - } + fmt.Fprintf(ctx.Output, " ServiceName: %s", mdlQuote(ctx, found.Definition.ServiceName)) + fmt.Fprintf(ctx.Output, ",\n EventNamePrefix: %s", mdlQuote(ctx, found.Definition.EventNamePrefix)) fmt.Fprintf(ctx.Output, "\n)\n") fmt.Fprintf(ctx.Output, "{\n") diff --git a/mdl/executor/cmd_businessevents_describe_quoting_test.go b/mdl/executor/cmd_businessevents_describe_quoting_test.go new file mode 100644 index 0000000000..51fa09b005 --- /dev/null +++ b/mdl/executor/cmd_businessevents_describe_quoting_test.go @@ -0,0 +1,80 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/backend/mock" + "github.com/mendixlabs/mxcli/mdl/langver" + "github.com/mendixlabs/mxcli/mdl/visitor" + "github.com/mendixlabs/mxcli/model" +) + +// describe business event service wrote ServiceName and EventNamePrefix as +// `'%s'`, the quotes added by hand and the value's own quotes left single. A +// service name with an apostrophe — `Bob's orders` — came out as a statement +// that does not parse, so the describe → exec round trip failed on it. +// +// The assertion is that the output re-parses to the same values, under both +// languages describe can write (mdl 0 escapes a backslash, mdl 1 does not), +// not that it contains a particular escape. +func TestDescribeBusinessEventService_QuotedValuesRoundTrip(t *testing.T) { + const serviceName, prefix = `Bob's orders`, `it's\here` + for _, lang := range []langver.Version{langver.V0, langver.V1} { + mod := mkModule("MyModule") + svc := &model.BusinessEventService{ + BaseElement: model.BaseElement{ID: nextID("bes")}, + ContainerID: mod.ID, + Name: "OrderEvents", + Definition: &model.BusinessEventDefinition{ + ServiceName: serviceName, + EventNamePrefix: prefix, + // A message with an attribute: a service with no message + // describes as `{ }` and a message with no attribute as + // `message M () publish;`, both of which the grammar refuses — + // separate defects, kept out of this test. + Channels: []*model.BusinessEventChannel{{ + ChannelName: "ch1", + Messages: []*model.BusinessEventMessage{{MessageName: "OrderCreated", + Attributes: []*model.BusinessEventAttribute{{AttributeName: "Total", AttributeType: "Decimal"}}}}, + }}, + }, + } + h := mkHierarchy(mod) + withContainer(h, svc.ContainerID, mod.ID) + mb := &mock.MockBackend{ + IsConnectedFunc: func() bool { return true }, + ListBusinessEventServicesFunc: func() ([]*model.BusinessEventService, error) { + return []*model.BusinessEventService{svc}, nil + }, + } + ctx, buf := newMockCtx(t, withBackend(mb), withHierarchy(h)) + ctx.describeLang = &lang + assertNoError(t, describeBusinessEventService(ctx, ast.QualifiedName{Module: "MyModule", Name: "OrderEvents"})) + + src := buf.String() + if lang == langver.V1 { + src = langver.HeaderFor(langver.V1) + "\n" + src + } + prog, errs := visitor.Build(src) + if len(errs) > 0 { + t.Fatalf("mdl %d: describe output does not parse: %v\n%s", lang, errs, src) + } + var stmt *ast.CreateBusinessEventServiceStmt + for _, s := range prog.Statements { + if c, ok := s.(*ast.CreateBusinessEventServiceStmt); ok { + stmt = c + } + } + if stmt == nil { + t.Fatalf("mdl %d: no business event service in:\n%s", lang, src) + } + if stmt.ServiceName != serviceName || stmt.EventNamePrefix != prefix { + t.Errorf("mdl %d: re-parsed ServiceName %q, EventNamePrefix %q; want %q, %q\n%s", + lang, stmt.ServiceName, stmt.EventNamePrefix, serviceName, prefix, strings.TrimSpace(src)) + } + } +} diff --git a/mdl/executor/issue1006_emitter_quoting_test.go b/mdl/executor/issue1006_emitter_quoting_test.go index af1f6b9cfe..339b22cd38 100644 --- a/mdl/executor/issue1006_emitter_quoting_test.go +++ b/mdl/executor/issue1006_emitter_quoting_test.go @@ -128,6 +128,7 @@ func TestDescribers_HaveNoHandRolledStringLiterals(t *testing.T) { "cmd_rest_clients.go", "cmd_agenteditor_agents.go", "cmd_navigation.go", // mendixlabs/mxcli#1343 + "cmd_businessevents.go", } { src, err := os.ReadFile(f) if err != nil { From 03fe8b564d853cbf1bd79f78bb68524680561325 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 13 Oct 2026 05:34:22 +0000 Subject: [PATCH 03/17] fix: write pluggable widget Types in the project's Mendix version shape No issue filed: found running the doctype suite on Mendix 9.24.30. Every page, snippet or alter page holding a pluggable widget failed to write: failed to create page: CreatePage: insert: unit ...: this Mendix 9.24.30 project cannot store CustomWidgets$WidgetValueType.DefaultType (Mendix 10.15.0+): the property did not exist yet in this version... The widget Type is copied from the embedded 11.6 templates, whose every CustomWidgets$WidgetValueType carries six keys Mendix introduced after 9.24 (mendixmodelsdk 4.116.0 WidgetValueType.versionInfo): SetLabel 10.5, IsLinked / IsMetaData 10.14, DefaultType 10.15, ActionVariables 10.21, AllowUpload 11.8. The property floor drops the empty ones and refuses a value: DefaultType is "None" everywhere, and IsLinked / IsMetaData / SetLabel / ActionVariables hold values on the combo box, dropdown sort and data grid filters, so they were next in line. The Type is the widget package's schema, not anything the statement said, so at template load (LoadWidgetTemplate, BuildFilterWidget) every CustomWidgets$ key the project's version does not declare is dropped, whatever it holds, using the floor's own version data (new canon.PropertyDeclared). The floor is unchanged and still refuses values in the widget's Object. codec.Encoder.OmitKeys cannot do this: the Type is decoded from raw bytes and the encoder passes clean fields through without consulting it. The 9.24 result equals Studio Pro 9.24.30's WidgetValueType key set (a Studio Pro-authored 9.24.30 app); an unreadable version and 11.8+ keep the template unchanged. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01W3x8Fyvir8kLyPgJs1BDmF --- ...3-pluggable-widget-type-keys-mendix-9.json | 1 + .../modelsdk/widget_pluggable_write.go | 85 +++++++- .../modelsdk/widget_template_version_test.go | 194 ++++++++++++++++++ modelsdk/canon/propertyfloor.go | 17 ++ 4 files changed, 295 insertions(+), 2 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/mdl-backend/2026-10-13-pluggable-widget-type-keys-mendix-9.json create mode 100644 mdl/backend/modelsdk/widget_template_version_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-backend/2026-10-13-pluggable-widget-type-keys-mendix-9.json b/.claude/skills/fix-issue/findings/mdl-backend/2026-10-13-pluggable-widget-type-keys-mendix-9.json new file mode 100644 index 0000000000..ef23a93704 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-backend/2026-10-13-pluggable-widget-type-keys-mendix-9.json @@ -0,0 +1 @@ +{"area": "mdl/backend", "date": "2026-10-13", "refs": [], "symptom": "Found running the doctype suite on Mendix 9.24.30 (no issue filed): every create page / alter page / snippet with a pluggable widget failed to write: \"failed to create page: CreatePage: insert: unit …: this Mendix 9.24.30 project cannot store CustomWidgets$WidgetValueType.DefaultType (Mendix 10.15.0+)\" (\"failed to save modified page\" for alter page), while `mxcli check` passed the script", "cause": "The pluggable widget Type is copied from the embedded 11.6 templates (sdk/widgets/templates/mendix-11.6, augmented from the project's .mpk) and written verbatim whatever the project version. Every CustomWidgets$WidgetValueType in it carries six keys Mendix introduced after 9.24 (mendixmodelsdk 4.116.0 WidgetValueType.versionInfo): SetLabel 10.5, IsLinked and IsMetaData 10.14, DefaultType 10.15, ActionVariables 10.21, AllowUpload 11.8. The property floor drops the empty ones and refuses a value: DefaultType is \"None\" on all 604 template value types, and IsLinked / IsMetaData / SetLabel are true on the combo box, dropdown sort and data grid filters, ActionVariables non-empty on some, so those were refused next", "file": "`mdl/backend/modelsdk/widget_pluggable_write.go` (templateFullBSONIDs, widgetTypeForVersion — used by LoadWidgetTemplate and BuildFilterWidget), `modelsdk/canon/propertyfloor.go` (PropertyDeclared)", "insight": "The Type is the widget package's schema, not anything the statement said, so a newer-version key in it is dropped whatever it holds — at template load, where the version is known — while the floor keeps refusing values in the Object. Studio Pro 9.24.30's WidgetValueType key set (Atlas_Core.LanguageSelectorWidget in a Studio Pro-authored 9.24 app, `mxcli bson dump --type snippet --format ndsl`) is exactly the template's minus those six, so the Model SDK versionInfo is complete for this type and the gate can use the floor's own data (canon.PropertyDeclared) instead of a hand list. codec.Encoder.OmitKeys does not reach it: the Type is decoded from raw bytes, its elements are clean, and the encoder passes clean fields through without consulting OmitKeys. Fixing DefaultType alone would have moved the refusal to IsLinked/SetLabel/ActionVariables; stubbing the gate in the unit test shows all of them at once. Not covered: 9.24 also stores CustomWidgetType.WidgetPhoneGapEnabled (deleted 10.0) that the template lacks (a property-set gap, not a refusal), and the data grid template's sample-column Object holds a 10.2+ Forms$DesignPropertyValue.Value"} diff --git a/mdl/backend/modelsdk/widget_pluggable_write.go b/mdl/backend/modelsdk/widget_pluggable_write.go index 40f69f8292..e5a34000d5 100644 --- a/mdl/backend/modelsdk/widget_pluggable_write.go +++ b/mdl/backend/modelsdk/widget_pluggable_write.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "log" + "strings" "sync" bsonv1 "go.mongodb.org/mongo-driver/bson" @@ -15,9 +16,11 @@ import ( "github.com/mendixlabs/mxcli/mdl/backend/widgetobj" "github.com/mendixlabs/mxcli/mdl/types" "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/modelsdk/canon" "github.com/mendixlabs/mxcli/modelsdk/codec" "github.com/mendixlabs/mxcli/modelsdk/element" mmpr "github.com/mendixlabs/mxcli/modelsdk/mpr" + mversion "github.com/mendixlabs/mxcli/modelsdk/version" mwidgets "github.com/mendixlabs/mxcli/modelsdk/widgets" "github.com/mendixlabs/mxcli/sdk/pages" ) @@ -27,7 +30,7 @@ import ( // serializer. The registry speaks bson v2; the builder + pages.CustomWidget speak // v1, so the template is bridged here. func (b *Backend) LoadWidgetTemplate(widgetID string, projectPath string) (backend.WidgetObjectBuilder, error) { - typeBSON, objBSON, propIDs, objectTypeID, _, err := mwidgets.GetTemplateFullBSON(widgetID, mmpr.GenerateID, projectPath) + typeBSON, objBSON, propIDs, objectTypeID, err := templateFullBSONIDs(widgetID, projectPath, b.ProjectVersion()) if err != nil { return nil, err } @@ -38,6 +41,84 @@ func (b *Backend) LoadWidgetTemplate(widgetID string, projectPath string) (backe propIDs, objectTypeID, codecChildSerializer{}), nil } +// templateFullBSONIDs loads a pluggable widget's template (augmented from the +// project's .mpk) with its Type written in the shape the project's Mendix +// version declares — see widgetTypeForVersion. Every template-built widget goes +// through it: LoadWidgetTemplate and BuildFilterWidget alike. +func templateFullBSONIDs(widgetID, projectPath string, pv *types.ProjectVersion) (bsonv2.D, bsonv2.D, map[string]mwidgets.PropertyTypeIDEntry, string, error) { + typeBSON, objBSON, propIDs, objectTypeID, _, err := mwidgets.GetTemplateFullBSON(widgetID, mmpr.GenerateID, projectPath) + if err != nil || typeBSON == nil { + return typeBSON, objBSON, propIDs, objectTypeID, err + } + return widgetTypeForVersion(typeBSON, pv), objBSON, propIDs, objectTypeID, nil +} + +// widgetTypeForVersion drops from a pluggable widget Type (the +// CustomWidgets$CustomWidgetType schema and everything under it) every key the +// project's Mendix version does not declare yet, whatever it holds. +// +// The embedded templates are extracted from Mendix 11.6, so every +// CustomWidgets$WidgetValueType in them carries SetLabel (10.5), IsLinked and +// IsMetaData (10.14), DefaultType (10.15), ActionVariables (10.21) and +// AllowUpload (11.8). DefaultType is "None" on every property, so the property +// floor (canon.StripUndeclaredProperties) refused every page with a pluggable +// widget on a Mendix 9.24 project; IsLinked / IsMetaData / SetLabel are true on +// the combo box and the data grid filters and would be refused next. +// +// Dropping a value is right here and wrong at the floor: the Type is the widget +// package's schema, copied from a newer Studio Pro, not anything the statement +// said — Studio Pro 9.24 stores none of these keys (measured on a Studio +// Pro-authored 9.24.30 app), and `mx convert` drops them. The Object (the +// widget's values) is not touched; a value it holds that the version cannot +// store is still refused by the floor. +// +// The version data is the floor's own (canon.PropertyDeclared), so the two +// cannot disagree. An unreadable version keeps the template as it is, the +// newest shape, as the floor strips nothing then either. +// +// Not codec.Encoder.OmitKeys: the Type is decoded from raw bytes and its +// elements are clean, and the encoder passes a clean element's fields through +// verbatim without consulting OmitKeys. +func widgetTypeForVersion(t bsonv2.D, pv *types.ProjectVersion) bsonv2.D { + if pv == nil || pv.MajorVersion == 0 { + return t + } + v := mversion.Version{Major: pv.MajorVersion, Minor: pv.MinorVersion, Patch: pv.PatchVersion} + return dropUndeclaredWidgetKeys(t, v) +} + +func dropUndeclaredWidgetKeys(d bsonv2.D, v mversion.Version) bsonv2.D { + typeName := "" + for _, e := range d { + if e.Key == "$Type" { + typeName, _ = e.Value.(string) + } + } + out := make(bsonv2.D, 0, len(d)) + for _, e := range d { + if strings.HasPrefix(typeName, "CustomWidgets$") && !canon.PropertyDeclared(typeName, e.Key, v) { + continue + } + e.Value = dropUndeclaredWidgetValue(e.Value, v) + out = append(out, e) + } + return out +} + +func dropUndeclaredWidgetValue(val any, v mversion.Version) any { + switch x := val.(type) { + case bsonv2.D: + return dropUndeclaredWidgetKeys(x, v) + case bsonv2.A: + out := make(bsonv2.A, len(x)) + for i := range x { + out[i] = dropUndeclaredWidgetValue(x[i], v) + } + return out + } + return val +} + // SerializeWidgetToOpaque converts a domain widget to its raw BSON form (for // embedding as a pluggable widget's child content) via the codec converters. func (b *Backend) SerializeWidgetToOpaque(w pages.Widget) any { @@ -61,7 +142,7 @@ func (b *Backend) BuildCreateAttributeObject(attributePath string, objectTypeID, // of the CustomWidget envelope (Appearance, editability, …) is added when the // widget is serialized via customWidgetToGen. func (b *Backend) BuildFilterWidget(spec backend.FilterWidgetSpec, projectPath string) (pages.Widget, error) { - typeBSON, objBSON, propertyTypeIDs, _, _, err := mwidgets.GetTemplateFullBSON(spec.WidgetID, mmpr.GenerateID, projectPath) + typeBSON, objBSON, propertyTypeIDs, _, err := templateFullBSONIDs(spec.WidgetID, projectPath, b.ProjectVersion()) if err != nil { return nil, fmt.Errorf("BuildFilterWidget: load template %s: %w", spec.WidgetID, err) } diff --git a/mdl/backend/modelsdk/widget_template_version_test.go b/mdl/backend/modelsdk/widget_template_version_test.go new file mode 100644 index 0000000000..ea77896491 --- /dev/null +++ b/mdl/backend/modelsdk/widget_template_version_test.go @@ -0,0 +1,194 @@ +// SPDX-License-Identifier: Apache-2.0 + +package modelsdkbackend + +import ( + "sort" + "strings" + "testing" + + bsonv2 "go.mongodb.org/mongo-driver/v2/bson" + + "github.com/mendixlabs/mxcli/mdl/types" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/modelsdk/canon" + "github.com/mendixlabs/mxcli/modelsdk/codec" + "github.com/mendixlabs/mxcli/modelsdk/version" + mwidgets "github.com/mendixlabs/mxcli/modelsdk/widgets" + "github.com/mendixlabs/mxcli/sdk/pages" +) + +// The embedded pluggable-widget templates are extracted from a Mendix 11.6 +// project, so their CustomWidgets$WidgetValueType carries six keys Mendix +// introduced after 9.24 (mendixmodelsdk WidgetValueType.versionInfo): +// +// SetLabel 10.5 · IsLinked, IsMetaData 10.14 · DefaultType 10.15 · +// ActionVariables 10.21 · AllowUpload 11.8 +// +// DefaultType holds "None" on every property, and IsLinked / IsMetaData / +// SetLabel are true on the combo box and the data grid filters, so the property +// floor (canon.StripUndeclaredProperties) refused every page with a pluggable +// widget on a 9.24 project: +// +// this Mendix 9.24.30 project cannot store +// CustomWidgets$WidgetValueType.DefaultType (Mendix 10.15.0+) +// +// Found running the doctype suite on Mendix 9.24.30 (no issue filed). + +// studioPro924ValueTypeKeys is the CustomWidgets$WidgetValueType key set +// Studio Pro 9.24.30 stores: Atlas_Core.LanguageSelectorWidget in a Studio +// Pro-authored 9.24.30 app (`mxcli bson dump --type snippet`). +var studioPro924ValueTypeKeys = []string{ + "$ID", "$Type", + "AllowNonPersistableEntities", "AllowedTypes", "AssociationTypes", + "DataSourceProperty", "DefaultValue", "EntityProperty", "EnumerationValues", + "IsList", "IsPath", "Multiline", "ObjectType", "OnChangeProperty", + "ParameterIsList", "PathType", "Required", "ReturnType", + "SelectableObjectsProperty", "SelectionTypes", "Translations", "Type", +} + +// encodeTemplateWidget builds a widget's Type from its template the way +// LoadWidgetTemplate / BuildFilterWidget do for a project of version pv, and +// encodes the widget. The Object is left out: the template's is a sample +// configuration the builder rewrites from the statement (a data grid's sample +// columns are replaced by the declared ones), so it is not what gets written. +func encodeTemplateWidget(t *testing.T, widgetID string, pv *types.ProjectVersion) []byte { + t.Helper() + typ, _, _, _, err := templateFullBSONIDs(widgetID, "", pv) + if err != nil { + t.Fatalf("%s: template: %v", widgetID, err) + } + if typ == nil { + t.Fatalf("%s: no template", widgetID) + } + cw := &pages.CustomWidget{ + BaseWidget: pages.BaseWidget{ + BaseElement: model.BaseElement{ID: model.ID(types.GenerateID()), TypeName: "CustomWidgets$CustomWidget"}, + Name: "w", + }, + RawType: v2ToV1BSON(typ), + } + g, err := customWidgetToGen(cw) + if err != nil { + t.Fatalf("%s: customWidgetToGen: %v", widgetID, err) + } + b, err := (&codec.Encoder{}).Encode(g) + if err != nil { + t.Fatalf("%s: encode: %v", widgetID, err) + } + return b +} + +// valueTypeKeySets returns the key set of every CustomWidgets$WidgetValueType +// in an encoded document. +func valueTypeKeySets(t *testing.T, b []byte) [][]string { + t.Helper() + var doc bsonv2.D + if err := bsonv2.Unmarshal(b, &doc); err != nil { + t.Fatal(err) + } + var out [][]string + var walk func(v any) + walk = func(v any) { + switch x := v.(type) { + case bsonv2.D: + var keys []string + isVT := false + for _, e := range x { + keys = append(keys, e.Key) + if e.Key == "$Type" && e.Value == "CustomWidgets$WidgetValueType" { + isVT = true + } + walk(e.Value) + } + if isVT { + sort.Strings(keys) + out = append(out, keys) + } + case bsonv2.A: + for _, e := range x { + walk(e) + } + } + } + walk(doc) + return out +} + +func TestTemplateWidgetMatchesMendix924ValueTypeKeys(t *testing.T) { + pv := v(9, 24, 30) + floor := version.Version{Major: 9, Minor: 24, Patch: 30} + want := append([]string(nil), studioPro924ValueTypeKeys...) + sort.Strings(want) + + for _, id := range mwidgets.ListAvailableTemplates() { + t.Run(id, func(t *testing.T) { + b := encodeTemplateWidget(t, id, pv) + // The symptom: the storage write refuses the unit. + if _, err := canon.StripUndeclaredProperties(b, &floor); err != nil { + t.Fatalf("Mendix 9.24.30 write refused: %v", err) + } + sets := valueTypeKeySets(t, b) + if len(sets) == 0 { + t.Fatal("no WidgetValueType in the encoded widget") + } + for _, got := range sets { + if strings.Join(got, ",") != strings.Join(want, ",") { + t.Fatalf("WidgetValueType keys on 9.24.30:\n got %v\n want %v (Studio Pro 9.24.30)", got, want) + } + } + }) + } +} + +// A project that declares the keys keeps the template verbatim — DefaultType +// on every value type, and the same key sets an unreadable (newest) version +// writes. +func TestTemplateWidgetKeepsValueTypeKeysFrom11_8(t *testing.T) { + for _, id := range []string{ + "com.mendix.widget.web.combobox.Combobox", + "com.mendix.widget.web.datagrid.Datagrid", + "com.mendix.widget.web.datagridtextfilter.DatagridTextFilter", + } { + t.Run(id, func(t *testing.T) { + newest := valueTypeKeySets(t, encodeTemplateWidget(t, id, nil)) + for _, pv := range []*types.ProjectVersion{v(11, 8, 0), v(11, 13, 0)} { + got := valueTypeKeySets(t, encodeTemplateWidget(t, id, pv)) + if len(got) != len(newest) { + t.Fatalf("%d.%d: %d value types, want %d", pv.MajorVersion, pv.MinorVersion, len(got), len(newest)) + } + for i := range got { + if strings.Join(got[i], ",") != strings.Join(newest[i], ",") { + t.Fatalf("%d.%d: value type %d keys changed:\n got %v\n want %v", pv.MajorVersion, pv.MinorVersion, i, got[i], newest[i]) + } + if !containsKey(got[i], "DefaultType") { + t.Fatalf("%d.%d: value type %d lost DefaultType: %v", pv.MajorVersion, pv.MinorVersion, i, got[i]) + } + } + } + }) + } +} + +// Between the floors, only the keys the version lacks go: 10.15 has DefaultType +// but not ActionVariables (10.21) or AllowUpload (11.8). +func TestTemplateWidgetValueTypeKeysAt10_15(t *testing.T) { + sets := valueTypeKeySets(t, encodeTemplateWidget(t, "com.mendix.widget.web.combobox.Combobox", v(10, 15, 0))) + for _, s := range sets { + if !containsKey(s, "DefaultType") || !containsKey(s, "IsLinked") || !containsKey(s, "SetLabel") { + t.Fatalf("10.15.0 lost a key it declares: %v", s) + } + if containsKey(s, "ActionVariables") || containsKey(s, "AllowUpload") { + t.Fatalf("10.15.0 kept a key it does not declare: %v", s) + } + } +} + +func containsKey(s []string, k string) bool { + for _, x := range s { + if x == k { + return true + } + } + return false +} diff --git a/modelsdk/canon/propertyfloor.go b/modelsdk/canon/propertyfloor.go index 2abdbe7bdb..a074f7e045 100644 --- a/modelsdk/canon/propertyfloor.go +++ b/modelsdk/canon/propertyfloor.go @@ -192,6 +192,23 @@ func isEmptyValue(v any) bool { return false } +// PropertyDeclared reports whether a project of version pv declares the BSON +// key on the storage $Type typeName, by the same version data +// StripUndeclaredProperties drops and refuses on. A key with no version data is +// declared, as it is to the floor. +// +// It exists for writers that copy a schema authored for a newer Mendix — a +// pluggable widget's Type from an embedded 11.6 template — and must write it in +// the project's shape before the floor sees it, where a value it holds is not +// something the statement said and refusing it would refuse the widget. +func PropertyDeclared(typeName, key string, pv version.Version) bool { + if gone, ok := measuredRemoved()[typeName+"."+key]; ok && pv.Compare(gone) >= 0 { + return false + } + since, ok := propertyFloor(typeName, key) + return !ok || pv.Compare(since) >= 0 +} + // propertyFloor returns the first Mendix version that declares a type's key: // the measured table first (it exists for keys the generated data lacks), then // the generated metamodel's data. From 39c286cb4b0ed43e3c11cccfb12fec0ea8b77d78 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 13 Oct 2026 06:17:00 +0000 Subject: [PATCH 04/17] fix: keep action sources in the marketplace reference cache (mendixlabs/mxcli#1441) Reported: `marketplace update 109515 --to 5.2.1` on an untouched NanoflowCommons refused with refusing to update: 44 element(s) have been changed locally and the update would discard them: JAVASCRIPT_ACTION Base64Decode and `marketplace diff` showed "Locally modified (44 of 50 elements)". The finished-reference cache kept only the model: isModelFile dropped javascriptsource/ and javasource/ as "never read". DESCRIBE JAVASCRIPT ACTION and DESCRIBE JAVA ACTION read the action source beside the .mpr and print a "source not available" placeholder when it is missing. A cache miss compared real code with real code; a cache hit compared the placeholder with the project's code, so every action read as edited. - isModelFile keeps javascriptsource/ and javasource/. - The entry name carries a format segment (refEntryFormat "f2"), so a model-only entry written before this fix is never served; old entries age out through the existing prune. - Element.Conclusive treats the omitted-source placeholder as inconclusive, so a missing source file can only ever be Unknown (which still needs --force), never "changed locally". The gate is otherwise unchanged; the GUID transplant path is untouched. Test: TestCachedReferenceDescribesActionSourcesLikeAFreshOne diffs the same untouched module against a fresh reference (miss) and the cache-restored one (hit); both must be Unchanged. With the old filter the hit reports both actions modified (unknown with the new gate). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01W3x8Fyvir8kLyPgJs1BDmF --- ...he-hit-reports-every-js-action-edited.json | 1 + cmd/mxcli/marketplace/inconclusive_test.go | 19 +++ cmd/mxcli/marketplace/refcache.go | 58 +++++--- .../marketplace/refcache_sources_test.go | 135 ++++++++++++++++++ cmd/mxcli/marketplace/refcache_test.go | 50 ++++++- cmd/mxcli/marketplace/scratch.go | 4 +- cmd/mxcli/marketplace/snapshot.go | 14 ++ 7 files changed, 257 insertions(+), 24 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/cmd-mxcli/2026-10-12-refcache-dropped-action-sources-cache-hit-reports-every-js-action-edited.json create mode 100644 cmd/mxcli/marketplace/refcache_sources_test.go diff --git a/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-12-refcache-dropped-action-sources-cache-hit-reports-every-js-action-edited.json b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-12-refcache-dropped-action-sources-cache-hit-reports-every-js-action-edited.json new file mode 100644 index 0000000000..1e93d806c6 --- /dev/null +++ b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-12-refcache-dropped-action-sources-cache-hit-reports-every-js-action-edited.json @@ -0,0 +1 @@ +{"area": "cmd/mxcli", "date": "2026-10-12", "symptom": "`marketplace update 109515 --to 5.2.1` refused an untouched NanoflowCommons with \"refusing to update: 44 element(s) have been changed locally and the update would discard them: JAVASCRIPT_ACTION Base64Decode ...\"; `marketplace diff` showed \"Locally modified (44 of 50 elements)\", every one a JavaScript action (mendixlabs/mxcli#1441). Only on a repeat run of a version - the first diff, a cache miss, was clean.", "cause": "The finished-reference cache stored the model only: isModelFile dropped javascriptsource/ and javasource/ as 'never read'. DESCRIBE JAVASCRIPT/JAVA ACTION reads //actions/.{js,java} beside the .mpr and prints a 'source not available' placeholder when absent, so a cache MISS compared real code against real code and a cache HIT compared the placeholder against the project's code - every action Modified. The filter's own comment warned of exactly this ('anything that starts reading a sibling directory ... will see it on a cache miss and not on a hit').", "file": "cmd/mxcli/marketplace/refcache.go (isModelFile keeps javascriptsource/ + javasource/; refEntryFormat 'f2' prefix in refCacheDir so pre-fix model-only entries are never served), cmd/mxcli/marketplace/snapshot.go (Conclusive: omitted-source placeholder -> Unknown, never Modified); tests cmd/mxcli/marketplace/refcache_sources_test.go, refcache_test.go, inconclusive_test.go", "insight": "**A cache bug that only shows on the second run needs a miss-then-hit test, not a hit test.** The decisive test snapshots the same untouched module against the fresh reference and then against the cache-restored one; the miss half passing proves the fixture is sound, the hit half failing locates the cache. Assert Unchanged, not 'not Modified': with the placeholder gate in place the old filter yields Unknown, which still blocks `update` without --force - the control showed exactly that. **A cache-content change needs a key change**: an old entry is complete by its own marker and would keep serving the stripped tree for ever, so the fix versions the entry name (refEntryFormat) instead of trusting a filter change to reach existing caches. **A placeholder is not content**: DESCRIBE output that substitutes a 'not available' body for data must be inconclusive in the diff, else a missing file reads as an edit. Controls: old filter -> hit reports 'unknown' (gate kept) / 'modified' (gate off); unprefixed key -> legacy entry served.", "refs": ["mendixlabs/mxcli#1441", "#731", "#637"]} diff --git a/cmd/mxcli/marketplace/inconclusive_test.go b/cmd/mxcli/marketplace/inconclusive_test.go index 8c128f24e1..5fe9bd0ec4 100644 --- a/cmd/mxcli/marketplace/inconclusive_test.go +++ b/cmd/mxcli/marketplace/inconclusive_test.go @@ -97,3 +97,22 @@ func TestSaveEdits_RefusesToWriteAnEmptyBodyAsAnEdit(t *testing.T) { t.Errorf("written = %v, want the real entity still saved", written) } } + +// mendixlabs/mxcli#1441: a side whose action source was not on disk describes +// as a placeholder. Against the other side's real code that is a missing file, +// not an edit, and must never block an update as "changed locally". +func TestClassify_AnOmittedActionSourceIsNotEvidenceOfAnEdit(t *testing.T) { + for _, tc := range []struct{ typ, placeholder string }{ + {"JAVASCRIPT_ACTION", "// JavaScript source not available from this project; body omitted by DESCRIBE."}, + {"JAVA_ACTION", "// Java source not available from this project; body omitted by DESCRIBE."}, + } { + src := "create or modify javascript action M.A()\nas $$\nreturn Promise.resolve(true);\n$$;" + omitted := strings.Replace(src, "return Promise.resolve(true);", tc.placeholder, 1) + for _, sides := range [][2]string{{src, omitted}, {omitted, src}} { + f := classify(ElementKey{Type: tc.typ, Name: "A"}, Element{MDL: sides[0]}, Element{MDL: sides[1]}, true, true) + if f.Verdict != Unknown { + t.Errorf("%s: verdict = %s, want unknown — a missing source file is not a local edit", tc.typ, f.Verdict) + } + } + } +} diff --git a/cmd/mxcli/marketplace/refcache.go b/cmd/mxcli/marketplace/refcache.go index 72200e1d6e..d510a383d9 100644 --- a/cmd/mxcli/marketplace/refcache.go +++ b/cmd/mxcli/marketplace/refcache.go @@ -85,9 +85,19 @@ func refCacheDir(versionID, mendixVersion string) (string, error) { if err != nil { return "", err } - return filepath.Join(root, "ref", safeKey(mendixVersion)+"_"+safeKey(versionID)), nil + return filepath.Join(root, "ref", refEntryFormat+"_"+safeKey(mendixVersion)+"_"+safeKey(versionID)), nil } +// refEntryFormat versions what an entry holds. Change it whenever isModelFile +// keeps something it did not before: an entry written under the old filter is +// complete by its own marker and would otherwise be served for ever, missing +// exactly what the new filter was extended for. +// +// "f2" added javascriptsource/ and javasource/ (mendixlabs/mxcli#1441). Entries +// with no prefix were model-only; they sit beside the new ones under ref/, are +// never looked up again, and age out through pruneRefCache. +const refEntryFormat = "f2" + // safeKey makes a path component out of a version string or UUID. Anything that // is not plainly alphanumeric becomes '-', so a malformed version from the API // cannot walk out of the cache directory. @@ -117,8 +127,9 @@ func safeKey(s string) string { // defaultRefCacheEntries bounds the finished-reference cache. // -// An entry is the model alone (see isModelFile) — 14 MB measured for -// Administration at 11.12.1, against 34 MB for the whole reference project. +// An entry is the model plus action sources (see isModelFile) — for +// Administration at 11.12.1, the 14 MB model and 1.6 MB of javascriptsource/ +// measured, against 34 MB for the whole reference project. // Twelve is what a six-module update sweep builds, base and target each, so the // default holds a whole sweep at ~170 MB and the `update` that follows a `diff` // still hits. @@ -257,8 +268,9 @@ func publishToCache(buildDir, cacheDir string) error { return nil } -// isModelFile reports whether a path inside a reference project is part of the -// MODEL, which is the only thing a cached reference is ever read for. +// isModelFile reports whether a path inside a reference project is something a +// cached reference is read for: the model, and the action sources DESCRIBE +// reads beside it. // // A reference project is a whole blank Mendix app, and most of it is bulk that // nothing here touches. Measured on Administration at 11.12.1, a 34 MB entry: @@ -267,25 +279,34 @@ func publishToCache(buildDir, cacheDir string) error { // widgets/ 9.6 MB never read // themesource/ 6.4 MB never read // theme-cache/ 2.1 MB never read (compiled CSS) -// javascriptsource/ 1.6 MB never read +// javascriptsource/ 1.6 MB READ — kept // -// Both consumers take the .mpr and nothing beside it: SnapshotModule opens it, -// and PerformUpdate takes the reference's model from it while taking the -// module's bundled widgets from the .mpk — deliberately, because the reference -// project's widgets/ also holds the blank template's copies. +// The .mpr is opened by SnapshotModule and PerformUpdate; PerformUpdate takes +// the module's bundled widgets from the .mpk rather than from the reference, +// deliberately, because the reference project's widgets/ also holds the blank +// template's copies. // -// THIS IS A CONSTRAINT ON FUTURE CHANGES. A cached reference is model-only, so -// anything that starts reading a sibling directory of the reference .mpr will -// see it on a cache miss and not on a hit — a difference that shows up as -// findings that come and go. Extend this filter in the same commit, or store -// the whole tree again. +// javascriptsource/ and javasource/ were once dropped here as "never read", and +// that was wrong: DESCRIBE JAVASCRIPT ACTION and DESCRIBE JAVA ACTION read +// //actions/.{js,java} next to the .mpr, and print a +// "source not available" placeholder when it is missing. A cache miss compared +// the real source against the project's; a hit compared the placeholder, and +// every action in an untouched module came out "changed locally" — 44 of them +// on NanoflowCommons, enough to make `update --to` refuse (mendixlabs/mxcli#1441). +// +// THIS IS A CONSTRAINT ON FUTURE CHANGES. Anything that starts reading a sibling +// directory of the reference .mpr will see it on a cache miss and not on a hit +// — a difference that shows up as findings that come and go. Extend this filter +// in the same commit and bump refEntryFormat, or store the whole tree again. // // mprcontents/ is kept even though `mx module-import` always collapses the // reference to MPR v1: the cost is nothing when it is absent, and the failure if // that ever changes is an unreadable model rather than a slower run. func isModelFile(rel string) bool { - if rel == "mprcontents" || strings.HasPrefix(rel, "mprcontents"+string(filepath.Separator)) { - return true + for _, dir := range []string{"mprcontents", "javascriptsource", "javasource"} { + if rel == dir || strings.HasPrefix(rel, dir+string(filepath.Separator)) { + return true + } } return !strings.ContainsRune(rel, filepath.Separator) && strings.HasSuffix(rel, ".mpr") } @@ -299,7 +320,8 @@ func copyTree(src, dst string) error { return copyTreeFiltered(src, dst, nil) } -// copyTreeModelOnly copies just the model, for the finished-reference cache. +// copyTreeModelOnly copies the model and the action sources DESCRIBE reads (see +// isModelFile), for the finished-reference cache. // The blank-project cache deliberately does NOT use this: a blank app is the // input to `mx module-import`, which reads the whole tree. func copyTreeModelOnly(src, dst string) error { diff --git a/cmd/mxcli/marketplace/refcache_sources_test.go b/cmd/mxcli/marketplace/refcache_sources_test.go new file mode 100644 index 0000000000..7aaa6b62ad --- /dev/null +++ b/cmd/mxcli/marketplace/refcache_sources_test.go @@ -0,0 +1,135 @@ +// SPDX-License-Identifier: Apache-2.0 + +package marketplace + +import ( + "os" + "path/filepath" + "strings" + "testing" + + modelsdk "github.com/mendixlabs/mxcli" +) + +// TestCachedReferenceDescribesActionSourcesLikeAFreshOne is mendixlabs/mxcli#1441. +// +// `marketplace update --to` refused an untouched NanoflowCommons with "44 +// element(s) have been changed locally", every one a JAVASCRIPT_ACTION. DESCRIBE +// of a JavaScript (or Java) action reads its source from javascriptsource/ +// (javasource/) beside the .mpr, and the finished-reference cache kept only the +// model. A cache MISS hands out the freshly built reference with its sources and +// compares clean; a cache HIT hands out a reference whose actions describe as the +// "source not available" placeholder, against the project's real code. +// +// So the test diffs the same untouched module twice — miss, then hit — and both +// must be verified Unchanged. Asserting Unchanged (not merely "not Modified") +// matters: the gate treats the placeholder as inconclusive, which would turn the +// hit's false "Modified" into an equally blocking "Unknown". +func TestCachedReferenceDescribesActionSourcesLikeAFreshOne(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("MXCLI_NO_REF_CACHE", "") + t.Setenv("MXCLI_REF_CACHE_MAX", "0") + + const module = "Administration" + // The reference built from the package: a project whose module carries + // action sources on disk, as `mx module-import` leaves it. + ref := copyFixture(t) + execMDL(t, ref, ` +create javascript action Administration.Issue1441Js() returns Boolean +as $$ +return Promise.resolve(true); +$$; +create java action Administration.Issue1441Java() returns boolean +as $$ +return true; +$$; +`) + refDir := filepath.Dir(ref) + assertSourceOnDisk(t, refDir, "javascriptsource") + assertSourceOnDisk(t, refDir, "javasource") + + // The user's project: the same module, untouched. + installedDir := t.TempDir() + if err := copyTree(refDir, installedDir); err != nil { + t.Fatalf("copy project: %v", err) + } + installed, err := SnapshotModule(filepath.Join(installedDir, filepath.Base(ref)), module, testBackend) + if err != nil { + t.Fatalf("snapshot project: %v", err) + } + + mendixVersion := stampedVersion(t, ref) + const versionID = "00000000-1441-0000-0000-000000000000" + mpk := filepath.Join(t.TempDir(), "package.mpk") + mustWrite(t, mpk, "pkg") + + // Run 1: cache miss — the fresh reference is compared, then cached. + if got := CachedReference(versionID, mendixVersion, t.TempDir(), filepath.Join(t.TempDir(), "p.mpk")); got != "" { + t.Fatalf("expected a cache miss on a fresh HOME, got %s", got) + } + fresh, err := SnapshotModule(ref, module, testBackend) + if err != nil { + t.Fatalf("snapshot fresh reference: %v", err) + } + assertUntouched(t, "cache miss", installed, fresh) + CacheReference(versionID, mendixVersion, refDir, mpk) + + // Run 2: cache hit — the restored reference must describe identically. + hitDir := t.TempDir() + hit := CachedReference(versionID, mendixVersion, hitDir, filepath.Join(t.TempDir(), "p.mpk")) + if hit == "" { + t.Fatal("expected a cache hit after CacheReference") + } + cached, err := SnapshotModule(hit, module, testBackend) + if err != nil { + t.Fatalf("snapshot cached reference: %v", err) + } + assertUntouched(t, "cache hit", installed, cached) +} + +func assertSourceOnDisk(t *testing.T, dir, sub string) { + t.Helper() + var found bool + _ = filepath.Walk(filepath.Join(dir, sub), func(p string, info os.FileInfo, err error) error { + if err == nil && !info.IsDir() && strings.Contains(p, "Issue1441") { + found = true + } + return nil + }) + if !found { + t.Fatalf("creating the action wrote no source under %s/; the test would prove nothing", sub) + } +} + +func stampedVersion(t *testing.T, mpr string) string { + t.Helper() + r, err := modelsdk.Open(mpr) + if err != nil { + t.Fatalf("open %s: %v", mpr, err) + } + defer func() { _ = r.Disconnect() }() + v, _ := r.GetMendixVersion() + return v +} + +func assertUntouched(t *testing.T, run string, installed, ref *Snapshot) { + t.Helper() + rep := Compare(installed, ref) + for _, name := range []ElementKey{ + {Type: "JAVASCRIPT_ACTION", Name: "Issue1441Js"}, + {Type: "JAVA_ACTION", Name: "Issue1441Java"}, + } { + if _, ok := ref.Elements[name]; !ok { + t.Fatalf("%s: %s not enumerated; keys=%v", run, name, ref.Keys()) + } + } + for _, f := range rep.Findings { + if f.Verdict != Unchanged { + t.Errorf("%s: %s reported %s on an untouched module (%s)", run, f.Key, f.Verdict, f.Reason) + if f.Verdict == Modified { + t.Errorf(" project: %s", f.InstalledMDL) + t.Errorf(" reference: %s", f.PackageMDL) + } + } + } +} diff --git a/cmd/mxcli/marketplace/refcache_test.go b/cmd/mxcli/marketplace/refcache_test.go index 46a40b9aeb..069c48123d 100644 --- a/cmd/mxcli/marketplace/refcache_test.go +++ b/cmd/mxcli/marketplace/refcache_test.go @@ -277,8 +277,9 @@ func TestPruneDisabledByZero(t *testing.T) { } // TestModelOnlyCopyKeepsWhatIsRead pins the filter that makes a reference entry -// ~14 MB instead of ~34 MB. The .mpr and mprcontents/ are the model; everything -// else in a reference project is bulk nothing reads. +// a fraction of the ~34 MB project. The .mpr and mprcontents/ are the model, and +// javascriptsource/ and javasource/ are read by DESCRIBE; everything else in a +// reference project is bulk nothing reads. // // The negative half matters as much as the positive: if widgets/ started being // copied again the cache would quietly double in size, and if the .mpr stopped @@ -291,7 +292,8 @@ func TestModelOnlyCopyKeepsWhatIsRead(t *testing.T) { mustWrite(t, filepath.Join(src, "widgets", "big.mpk"), "bulk") mustWrite(t, filepath.Join(src, "themesource", "atlas", "style.scss"), "bulk") mustWrite(t, filepath.Join(src, "theme-cache", "compiled.css"), "bulk") - mustWrite(t, filepath.Join(src, "javascriptsource", "a.js"), "bulk") + mustWrite(t, filepath.Join(src, "javascriptsource", "Mod", "actions", "A.js"), "js") + mustWrite(t, filepath.Join(src, "javasource", "mod", "actions", "A.java"), "java") if err := copyTreeModelOnly(src, dst); err != nil { t.Fatalf("copyTreeModelOnly: %v", err) @@ -303,7 +305,15 @@ func TestModelOnlyCopyKeepsWhatIsRead(t *testing.T) { if got := readFile(t, filepath.Join(dst, "mprcontents", "nested", "unit.mxunit")); got != "unit" { t.Errorf("mprcontents unit = %q, want %q", got, "unit") } - for _, bulk := range []string{"widgets", "themesource", "theme-cache", "javascriptsource"} { + // DESCRIBE reads action sources beside the .mpr; dropping them made every + // cache hit report each action as edited (mendixlabs/mxcli#1441). + if got := readFile(t, filepath.Join(dst, "javascriptsource", "Mod", "actions", "A.js")); got != "js" { + t.Errorf("javascriptsource action = %q, want %q", got, "js") + } + if got := readFile(t, filepath.Join(dst, "javasource", "mod", "actions", "A.java")); got != "java" { + t.Errorf("javasource action = %q, want %q", got, "java") + } + for _, bulk := range []string{"widgets", "themesource", "theme-cache"} { if _, err := os.Stat(filepath.Join(dst, bulk)); !os.IsNotExist(err) { t.Errorf("%s/ was copied into the cache entry; nothing reads it", bulk) } @@ -319,3 +329,35 @@ func TestModelOnlyCopyKeepsWhatIsRead(t *testing.T) { t.Error("the unfiltered copy dropped widgets/; a blank project needs its whole tree") } } + +// TestLegacyModelOnlyEntryIsNotServed: an entry written before javascriptsource/ +// and javasource/ were kept is complete by its own marker, so only the key can +// stop it being served — and serving it is mendixlabs/mxcli#1441 all over again. +func TestLegacyModelOnlyEntryIsNotServed(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("MXCLI_NO_REF_CACHE", "") + const versionID, mx = "2059615c-c6f1-4103-aedb-14820c077a1c", "11.12.1" + + root, err := refCacheRoot() + if err != nil { + t.Fatal(err) + } + legacy := filepath.Join(root, "ref", safeKey(mx)+"_"+safeKey(versionID)) + mustWrite(t, filepath.Join(legacy, entryProject, "PackageRef.mpr"), "model") + mustWrite(t, filepath.Join(legacy, entryPackage), "pkg") + mustWrite(t, filepath.Join(legacy, completeMarker), "") + + current, err := refCacheDir(versionID, mx) + if err != nil { + t.Fatal(err) + } + if current == legacy { + t.Fatalf("the cache key was not versioned; a model-only entry at %s would still be served", legacy) + } + if !strings.HasPrefix(filepath.Base(current), refEntryFormat+"_") { + t.Errorf("entry %s does not carry the format %q", current, refEntryFormat) + } + if got := CachedReference(versionID, mx, t.TempDir(), filepath.Join(t.TempDir(), "p.mpk")); got != "" { + t.Errorf("a pre-format entry was served: %s", got) + } +} diff --git a/cmd/mxcli/marketplace/scratch.go b/cmd/mxcli/marketplace/scratch.go index 0dda7e7a0c..1353a292ba 100644 --- a/cmd/mxcli/marketplace/scratch.go +++ b/cmd/mxcli/marketplace/scratch.go @@ -355,8 +355,8 @@ func CacheReference(versionID, mendixVersion, refDir, mpkPath string) { if err != nil { return } - // Model only: see isModelFile for what is dropped, why nothing reads it, and - // what that constrains. + // Model and action sources: see isModelFile for what is dropped, what is + // kept because DESCRIBE reads it, and what that constrains. if err := copyTreeModelOnly(refDir, filepath.Join(staging, entryProject)); err != nil { _ = os.RemoveAll(staging) return diff --git a/cmd/mxcli/marketplace/snapshot.go b/cmd/mxcli/marketplace/snapshot.go index fcc185a07d..21d665294a 100644 --- a/cmd/mxcli/marketplace/snapshot.go +++ b/cmd/mxcli/marketplace/snapshot.go @@ -83,9 +83,23 @@ func (e Element) Conclusive() (bool, string) { if emptyBody(e.MDL) { return false, "DESCRIBE produced no body, so a difference here is not evidence of an edit" } + if sourceOmitted(e.MDL) { + return false, "the action's source file was not on disk, so its body could not be compared" + } return true, "" } +// sourceOmitted spots the placeholder DESCRIBE prints for a JavaScript or Java +// action whose source file it could not read (jsSourceOmittedBody / +// javaSourceOmittedBody in mdl/executor). That body says nothing about the +// code, so a difference against it is a missing file, not an edit — the shape +// of mendixlabs/mxcli#1441, where a cached reference without sources made every +// action of an untouched module read as "changed locally". Matching the +// handlers' own shared wording keeps the two in step. +func sourceOmitted(mdl string) bool { + return strings.Contains(mdl, "source not available from this project; body omitted by DESCRIBE.") +} + // declaredReadOnly spots the comment a describe handler emits when its output // cannot be replayed. Matching the handler's own words keeps the two in step: // a handler that stops saying it is read-only has become authorable. From 26d5c38de96fc760378577f71ec340a2ae560e9c Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 13 Oct 2026 06:20:57 +0000 Subject: [PATCH 05/17] fix(sync-java-deps): check the Gradle-resolved version, not every declaration When two modules declare the same artifact at different versions (mendixlabs/mxcli#1450: ExcelImporter commons-lang3 3.18.0, CommunityCommons 3.20.0), `mx sync-java-dependencies` runs Gradle, whose conflict resolution vendors only the newest. UnvendoredJarDependencies checked each declaration, so the older one was "missing" forever: --check failed, the sync deleted the jar and errored "still missing", and run --local re-synced on every start. Collapse declarations to the newest version per group:artifact, compared the way Gradle orders versions, before checking vendorlib/. sync-java-deps lists the dropped declarations as "superseded" instead of "missing". Measured on 11.12.1 (mxcli new + two modules): vendorlib/ receives commons-lang3-3.20.0.jar only; the pre-fix binary prints the reported error verbatim, the fixed one passes --check and the sync. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01PjL2dUU7gkzBzz9SeMru7H --- ...ps-superseded-version-missing-forever.json | 1 + cmd/mxcli/cmd_sync_java_deps.go | 9 + cmd/mxcli/docker/javadeps.go | 171 +++++++++++++++++- cmd/mxcli/docker/javadeps_test.go | 110 +++++++++-- ...50-sync-java-deps-conflicting-versions.mdl | 37 ++++ 5 files changed, 309 insertions(+), 19 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/cmd-mxcli/2026-10-13-sync-java-deps-superseded-version-missing-forever.json create mode 100644 mdl-examples/bug-tests/1450-sync-java-deps-conflicting-versions.mdl diff --git a/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-13-sync-java-deps-superseded-version-missing-forever.json b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-13-sync-java-deps-superseded-version-missing-forever.json new file mode 100644 index 0000000000..b52fa4111f --- /dev/null +++ b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-13-sync-java-deps-superseded-version-missing-forever.json @@ -0,0 +1 @@ +{"area": "cmd/mxcli", "date": "2026-10-13", "symptom": "Two modules declare the same artifact at different versions (ExcelImporter commons-lang3 3.18.0, CommunityCommons 3.20.0): `sync-java-deps --check` fails with `missing org.apache.commons:commons-lang3:3.18.0`, the sync deletes that jar and errors `sync finished but 1 dependency/dependencies are still missing from vendorlib/`, and `run --local` stops at `Resolving 1 managed Java dependency` on every start (#1450)", "cause": "`UnvendoredJarDependencies` checked every declaration for `-.jar`, but `mx sync-java-dependencies` runs Gradle over all modules at once and its conflict resolution vendors only the newest version per group:artifact — the older coordinate can never appear, so the postcondition was unsatisfiable", "file": "`cmd/mxcli/docker/javadeps.go` (`resolveJarDependencies`, `compareJarVersions`, `SupersededJarDependencies`), `cmd/mxcli/cmd_sync_java_deps.go`", "insight": "A gate that checks the sync's postcondition must check what the sync PRODUCES, not what the model declares — and the two differ exactly where Gradle resolves a conflict. The tell in the report was a sync that 'succeeded' and deleted the jar it then called missing: an unsatisfiable postcondition, not a failed download. Measured on 11.12.1 with `mxcli new` + two modules: vendorlib/ gets commons-lang3-3.20.0.jar only; pre-fix binary prints the reported error verbatim, fixed one passes. Versions must compare Gradle's way (numeric parts numerically, 1.0-beta < 1.0): a string compare picks 3.9.0 over 3.20.0. To run mx sync here, Gradle needs the proxy as JVM props (JAVA_TOOL_OPTIONS -Dhttps.proxyHost/Port) — otherwise mx says 'Failed to find some hosts', which is not a Maven outage. The per-module Note in `alter module … add jar dependency` (executor `warnUnvendoredJarDependencies`) still checks one module's declarations and can name a superseded coordinate", "refs": ["mendixlabs/mxcli#1450"]} diff --git a/cmd/mxcli/cmd_sync_java_deps.go b/cmd/mxcli/cmd_sync_java_deps.go index f68ff56c2b..edbd90b7d4 100644 --- a/cmd/mxcli/cmd_sync_java_deps.go +++ b/cmd/mxcli/cmd_sync_java_deps.go @@ -30,6 +30,10 @@ from code that looks correctly configured. Requires network access (Maven) and the mx binary for the project's version; 'mxcli setup mxbuild' fetches the latter. +When two modules declare the same artifact at different versions, the sync +vendors only the newest (Gradle's conflict resolution); the older declaration is +listed as superseded, not missing. + Examples: mxcli sync-java-deps -p app.mpr mxcli sync-java-deps -p app.mpr --check # report what is missing, download nothing @@ -56,6 +60,11 @@ Examples: for _, m := range missing { fmt.Printf(" missing %s\n", m) } + // Another module's newer version wins Gradle's conflict resolution, so + // these are never vendored — and are not missing (#1450). + for _, s := range docker.SupersededJarDependencies(deps) { + fmt.Printf(" superseded %s\n", s) + } if checkOnly { if len(missing) > 0 { // A non-zero exit makes this usable as a build-gate. diff --git a/cmd/mxcli/docker/javadeps.go b/cmd/mxcli/docker/javadeps.go index d1449f6c23..4038098d2a 100644 --- a/cmd/mxcli/docker/javadeps.go +++ b/cmd/mxcli/docker/javadeps.go @@ -75,17 +75,18 @@ func SyncJavaDependencies(projectPath, mxPath, version string, w io.Writer) erro } // UnvendoredJarDependencies returns the coordinates ("group:artifact:version") -// declared by the model that have no matching jar in the project's vendorlib/, +// the sync would vendor that have no matching jar in the project's vendorlib/, // so a caller can sync only when something is actually missing. // +// Only the resolved version of each artifact counts (see +// resolveJarDependencies): checking every declaration made a coordinate that +// Gradle had dropped "missing" forever, with no sync able to supply it (#1450). +// // The vendored file name is `-.jar`, which is what // `mx sync-java-dependencies` writes. func UnvendoredJarDependencies(projectDir string, deps []JarDependencyRef) []string { var missing []string - for _, d := range deps { - if d.Artifact == "" || d.Version == "" { - continue - } + for _, d := range resolveJarDependencies(deps) { jar := filepath.Join(projectDir, "vendorlib", fmt.Sprintf("%s-%s.jar", d.Artifact, d.Version)) if _, err := os.Stat(jar); err != nil { missing = append(missing, fmt.Sprintf("%s:%s:%s", d.Group, d.Artifact, d.Version)) @@ -101,3 +102,163 @@ type JarDependencyRef struct { Artifact string Version string } + +// resolveJarDependencies collapses the declarations to the one version of each +// group:artifact that `mx sync-java-dependencies` vendors, in first-declared +// order. Incomplete coordinates are dropped: they can be neither checked nor +// resolved. +// +// The sync runs Gradle over every module's declarations at once, and Gradle's +// conflict resolution keeps the newest version of an artifact declared more +// than once. Measured in #1450: ExcelImporter declaring commons-lang3 3.18.0 and +// CommunityCommons 3.20.0 leaves vendorlib/ holding commons-lang3-3.20.0.jar +// only, and a sync deletes a 3.18.0 jar placed there by hand. +func resolveJarDependencies(deps []JarDependencyRef) []JarDependencyRef { + var out []JarDependencyRef + index := map[string]int{} // group:artifact -> position in out + for _, d := range deps { + if d.Artifact == "" || d.Version == "" { + continue + } + key := d.Group + ":" + d.Artifact + if i, ok := index[key]; ok { + if compareJarVersions(d.Version, out[i].Version) > 0 { + out[i] = d + } + continue + } + index[key] = len(out) + out = append(out, d) + } + return out +} + +// SupersededJarDependencies lists the declarations the sync will not vendor +// because another module declares a newer version of the same artifact, as +// "group:artifact:version (resolved to newer)". They are not missing, but a +// module that needs exactly that version runs against the newer one, which is +// worth saying when nothing else in the output would. +func SupersededJarDependencies(deps []JarDependencyRef) []string { + winner := map[string]string{} + for _, d := range resolveJarDependencies(deps) { + winner[d.Group+":"+d.Artifact] = d.Version + } + var out []string + seen := map[string]bool{} + for _, d := range deps { + if d.Artifact == "" || d.Version == "" { + continue + } + coord := fmt.Sprintf("%s:%s:%s", d.Group, d.Artifact, d.Version) + w := winner[d.Group+":"+d.Artifact] + if w == d.Version || seen[coord] { + continue + } + seen[coord] = true + out = append(out, fmt.Sprintf("%s (resolved to %s)", coord, w)) + } + return out +} + +// jarVersionSpecial ranks the qualifiers Gradle gives a meaning; any other +// non-numeric part ranks 0 and compares lexically against its peers. +var jarVersionSpecial = map[string]int{ + "dev": -1, "rc": 1, "snapshot": 2, "final": 3, "ga": 4, "release": 5, "sp": 6, +} + +// compareJarVersions orders two versions the way Gradle's conflict resolution +// does: split on . - _ + and at digit/letter boundaries; numeric parts compare +// numerically and outrank non-numeric ones; a longer version wins when its +// extra part is numeric and loses when it is a qualifier (1.0-beta < 1.0). +// A string comparison gets the common case wrong: "3.9.0" > "3.20.0". +func compareJarVersions(a, b string) int { + pa, pb := splitJarVersion(a), splitJarVersion(b) + for i := 0; i < len(pa) && i < len(pb); i++ { + if c := compareJarVersionPart(pa[i], pb[i]); c != 0 { + return c + } + } + switch { + case len(pa) > len(pb): + if isJarVersionNumber(pa[len(pb)]) { + return 1 + } + return -1 + case len(pb) > len(pa): + if isJarVersionNumber(pb[len(pa)]) { + return -1 + } + return 1 + } + return 0 +} + +func compareJarVersionPart(x, y string) int { + if x == y { + return 0 + } + xn, yn := isJarVersionNumber(x), isJarVersionNumber(y) + switch { + case xn && yn: + x, y = strings.TrimLeft(x, "0"), strings.TrimLeft(y, "0") + if len(x) != len(y) { + if len(x) < len(y) { + return -1 + } + return 1 + } + return strings.Compare(x, y) + case xn: + return 1 + case yn: + return -1 + } + sx, xok := jarVersionSpecial[strings.ToLower(x)] + sy, yok := jarVersionSpecial[strings.ToLower(y)] + if xok || yok { + if sx != sy { + if sx < sy { + return -1 + } + return 1 + } + } + return strings.Compare(x, y) +} + +func splitJarVersion(v string) []string { + var parts []string + var cur strings.Builder + flush := func() { + if cur.Len() > 0 { + parts = append(parts, cur.String()) + cur.Reset() + } + } + prevDigit := false + for i, r := range v { + switch { + case r == '.' || r == '-' || r == '_' || r == '+': + flush() + continue + case i > 0 && cur.Len() > 0 && (r >= '0' && r <= '9') != prevDigit: + flush() + } + prevDigit = r >= '0' && r <= '9' + cur.WriteRune(r) + } + flush() + return parts +} + +func isJarVersionNumber(s string) bool { + if s == "" { + return false + } + for _, r := range s { + if r < '0' || r > '9' { + return false + } + } + return true +} diff --git a/cmd/mxcli/docker/javadeps_test.go b/cmd/mxcli/docker/javadeps_test.go index c8592c1380..667f7a1963 100644 --- a/cmd/mxcli/docker/javadeps_test.go +++ b/cmd/mxcli/docker/javadeps_test.go @@ -30,28 +30,110 @@ func TestUnvendoredJarDependencies(t *testing.T) { deps := []JarDependencyRef{ {Group: "org.apache.commons", Artifact: "commons-lang3", Version: "3.14.0"}, // present {Group: "org.duckdb", Artifact: "duckdb_jdbc", Version: "1.5.5.1"}, // missing - // A different version of a vendored artifact is still missing: the file - // name carries the version, and the wrong jar is not the right jar. - {Group: "org.apache.commons", Artifact: "commons-lang3", Version: "3.12.0"}, } got := UnvendoredJarDependencies(dir, deps) - if len(got) != 2 { - t.Fatalf("got %d missing, want 2: %v", len(got), got) - } - for _, want := range []string{"org.duckdb:duckdb_jdbc:1.5.5.1", "org.apache.commons:commons-lang3:3.12.0"} { - found := false - for _, g := range got { - if g == want { - found = true - } + if len(got) != 1 || got[0] != "org.duckdb:duckdb_jdbc:1.5.5.1" { + t.Fatalf("got %v, want only org.duckdb:duckdb_jdbc:1.5.5.1", got) + } +} + +// A different version of a vendored artifact is still missing: the file name +// carries the version, and the wrong jar is not the right jar. +func TestUnvendoredJarDependencies_WrongVersionVendored(t *testing.T) { + dir := t.TempDir() + writeVendoredJar(t, dir, "commons-lang3-3.14.0.jar") + deps := []JarDependencyRef{{Group: "org.apache.commons", Artifact: "commons-lang3", Version: "3.12.0"}} + got := UnvendoredJarDependencies(dir, deps) + if len(got) != 1 || got[0] != "org.apache.commons:commons-lang3:3.12.0" { + t.Errorf("got %v, want the declared 3.12.0", got) + } +} + +// mendixlabs/mxcli#1450: two modules declare the same artifact at different +// versions (ExcelImporter commons-lang3 3.18.0, CommunityCommons 3.20.0). +// `mx sync-java-dependencies` runs Gradle, whose conflict resolution keeps the +// newest, so vendorlib/ holds 3.20.0 only. Checking each declaration on its own +// reported "missing org.apache.commons:commons-lang3:3.18.0" forever: the gate +// failed, sync deleted the jar and then errored "still missing", and +// `run --local` re-synced on every start. +func TestUnvendoredJarDependencies_ConflictResolvesToNewest(t *testing.T) { + dir := t.TempDir() + writeVendoredJar(t, dir, "commons-lang3-3.20.0.jar") + deps := []JarDependencyRef{ + {Group: "org.apache.commons", Artifact: "commons-lang3", Version: "3.18.0"}, + {Group: "org.apache.commons", Artifact: "commons-lang3", Version: "3.20.0"}, + } + if got := UnvendoredJarDependencies(dir, deps); len(got) != 0 { + t.Errorf("superseded declaration reported as missing: %v", got) + } + + // The winner itself is still required: with nothing vendored, the newest + // version is the one missing — never the superseded one. + empty := t.TempDir() + got := UnvendoredJarDependencies(empty, deps) + if len(got) != 1 || got[0] != "org.apache.commons:commons-lang3:3.20.0" { + t.Errorf("got %v, want only the resolved 3.20.0", got) + } +} + +func TestSupersededJarDependencies(t *testing.T) { + deps := []JarDependencyRef{ + {Group: "org.apache.commons", Artifact: "commons-lang3", Version: "3.20.0"}, + {Group: "org.apache.commons", Artifact: "commons-lang3", Version: "3.18.0"}, + {Group: "org.duckdb", Artifact: "duckdb_jdbc", Version: "1.5.5.1"}, + // Same version declared twice is one requirement, not a conflict. + {Group: "org.duckdb", Artifact: "duckdb_jdbc", Version: "1.5.5.1"}, + } + got := SupersededJarDependencies(deps) + want := "org.apache.commons:commons-lang3:3.18.0 (resolved to 3.20.0)" + if len(got) != 1 || got[0] != want { + t.Errorf("got %v, want [%s]", got, want) + } +} + +// Gradle's version ordering, which decides the conflict winner. A plain string +// comparison gets the first case wrong ("3.9.0" > "3.20.0"). +func TestCompareJarVersions(t *testing.T) { + cases := []struct{ a, b string }{ // a < b + {"3.9.0", "3.20.0"}, + {"3.18.0", "3.20.0"}, + {"1.0", "1.0.1"}, + {"1.0-beta", "1.0"}, + {"1.0-rc1", "1.0"}, + {"1.0-alpha", "1.0-beta"}, + {"1.0-dev", "1.0-alpha"}, + {"1.0-SNAPSHOT", "1.0"}, + {"1.0-rc", "1.0-SNAPSHOT"}, + {"1.0.a", "1.0.1"}, + {"1.5.5", "1.5.5.1"}, + } + for _, c := range cases { + if compareJarVersions(c.a, c.b) >= 0 { + t.Errorf("want %s < %s", c.a, c.b) } - if !found { - t.Errorf("expected %q among the missing, got: %v", want, got) + if compareJarVersions(c.b, c.a) <= 0 { + t.Errorf("want %s > %s", c.b, c.a) + } + } + for _, v := range []string{"1.0", "3.20.0", "1.0-beta"} { + if compareJarVersions(v, v) != 0 { + t.Errorf("want %s == %s", v, v) } } } +func writeVendoredJar(t *testing.T, dir, name string) { + t.Helper() + vendorlib := filepath.Join(dir, "vendorlib") + if err := os.MkdirAll(vendorlib, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(vendorlib, name), []byte("jar"), 0o644); err != nil { + t.Fatal(err) + } +} + // An incomplete coordinate cannot be checked or resolved, and must not be // reported as missing — that would send the caller into a sync that cannot help. func TestUnvendoredJarDependencies_SkipsIncompleteCoordinates(t *testing.T) { diff --git a/mdl-examples/bug-tests/1450-sync-java-deps-conflicting-versions.mdl b/mdl-examples/bug-tests/1450-sync-java-deps-conflicting-versions.mdl new file mode 100644 index 0000000000..ade90c4eb5 --- /dev/null +++ b/mdl-examples/bug-tests/1450-sync-java-deps-conflicting-versions.mdl @@ -0,0 +1,37 @@ +mdl 1; +-- mendixlabs/mxcli#1450 — two modules declare the same artifact at different +-- versions, and `sync-java-deps` reported the older one missing forever. +-- @version: 10.0+ +-- +-- `mx sync-java-dependencies` runs Gradle over every module's declarations at +-- once; conflict resolution keeps the newest version, so vendorlib/ ends up with +-- commons-lang3-3.20.0.jar only. The gate used to check each declaration on its +-- own, so 3.18.0 was "missing" after every sync, the sync exited with "still +-- missing", and `run --local` re-synced on every start. +-- +-- Run this, then: +-- +-- mxcli sync-java-deps -p app.mpr -- vendors 3.20.0 +-- mxcli sync-java-deps -p app.mpr --check -- exit 0; lists +-- superseded org.apache.commons:commons-lang3:3.18.0 (resolved to 3.20.0) + +create module Bug1450A; +create module Bug1450B; + +alter module Bug1450A + add jar dependency ( + group = 'org.apache.commons', + artifact = 'commons-lang3', + version = '3.18.0', + included = true, + ); + +alter module Bug1450B + add jar dependency ( + group = 'org.apache.commons', + artifact = 'commons-lang3', + version = '3.20.0', + included = true, + ); + +list jar dependencies; From f9dab0fabdee5721e99c8e6a4f4f517945e0bf0e Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 13 Oct 2026 06:33:57 +0000 Subject: [PATCH 06/17] fix: keep a JavaScript action's imports and EXTRA CODE on regeneration (mendixlabs/mxcli#1429) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported: "Describe javascript action keeps EXTRA CODE only as a comment; re-running `create or modify` overwrites a hand-restored `.js`" — an edited body regenerated the file from the statement alone, deleting the EXTRA CODE helpers and the import list the user code calls. WriteJavaScriptSourceFile now reads the existing .js and carries its import list (multi-line and semicolon-less statements included) and the BEGIN/END EXTRA CODE section into the regenerated file, as Studio Pro does and as the Java writer already does (ako/mxcli#705). No interface change. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01W3x8Fyvir8kLyPgJs1BDmF --- ...12-js-action-rewrite-keeps-extra-code.json | 1 + mdl/backend/modelsdk/javascript_write.go | 11 +- .../modelsdk/javascript_write_retain_test.go | 113 ++++++++++++++++++ mdl/executor/cmd_javascript_actions_write.go | 8 +- sdk/javaactions/jsretain.go | 64 ++++++++++ sdk/javaactions/jsretain_test.go | 32 +++++ sdk/javaactions/jssource.go | 16 ++- 7 files changed, 237 insertions(+), 8 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/mdl-backend/2026-10-12-js-action-rewrite-keeps-extra-code.json create mode 100644 mdl/backend/modelsdk/javascript_write_retain_test.go create mode 100644 sdk/javaactions/jsretain.go create mode 100644 sdk/javaactions/jsretain_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-backend/2026-10-12-js-action-rewrite-keeps-extra-code.json b/.claude/skills/fix-issue/findings/mdl-backend/2026-10-12-js-action-rewrite-keeps-extra-code.json new file mode 100644 index 0000000000..ea5d95825c --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-backend/2026-10-12-js-action-rewrite-keeps-extra-code.json @@ -0,0 +1 @@ +{"area": "mdl/backend", "date": "2026-10-12", "refs": ["mendixlabs/mxcli#1429", "ako/mxcli#731", "ako/mxcli#705"], "symptom": "describe javascript action shows the EXTRA CODE only as a comment; re-running `create or modify` with an edited body overwrote a hand-restored .js, deleting the EXTRA CODE helpers and the import list the user code calls", "cause": "WriteJavaScriptSourceFile regenerated the file from the statement alone: GenerateJavaScriptSource had no slot for imports or extra code, unlike the Java twin (RetainSections, #705). The #731 fix only left the file alone when the body was unchanged", "file": "`mdl/backend/modelsdk/javascript_write.go` (WriteJavaScriptSourceFile), `sdk/javaactions/jsretain.go` (JavaScriptRetainedSections)", "insight": "When one doctype's file writer gains retention, grep for its twin: the Java fix (#705) landed long before the JS one with the same shape. Retain in the backend writer, next to the file read, so no interface change is needed and every caller is covered. JS imports can span lines and omit semicolons, and marketplace bundles put them above the banner, so scan statements up to the EXTRA CODE marker rather than matching single `import ...;` lines. The control is reverting only the writer: the test then loses every import and the helper.", "test": "TestWriteJavaScriptSourceFile_EditedBodyKeepsImportsAndExtraCode"} diff --git a/mdl/backend/modelsdk/javascript_write.go b/mdl/backend/modelsdk/javascript_write.go index 5d6b0b220a..5d0e547555 100644 --- a/mdl/backend/modelsdk/javascript_write.go +++ b/mdl/backend/modelsdk/javascript_write.go @@ -171,8 +171,15 @@ func (b *Backend) WriteJavaScriptSourceFile(moduleName, actionName string, jsCod if err := os.MkdirAll(dir, 0o755); err != nil { return fmt.Errorf("WriteJavaScriptSourceFile: create dir: %w", err) } - source := javaactions.GenerateJavaScriptSource(actionName, jsCode, params, returnType) - changed, err := javaactions.WriteSourceIfChanged(filepath.Join(dir, actionName+".js"), source) + path := filepath.Join(dir, actionName+".js") + // Regenerate the way Studio Pro does: the existing file's import list and + // EXTRA CODE section are retained, since MDL cannot express either. + // Keeping only the user code deleted hand-restored helpers the user code + // calls (mendixlabs/mxcli#1429; the Java twin is ako/mxcli#705). + existing, _ := os.ReadFile(path) + imports, extraCode := javaactions.JavaScriptRetainedSections(string(existing)) + source := javaactions.GenerateJavaScriptSource(actionName, jsCode, params, returnType, imports, extraCode) + changed, err := javaactions.WriteSourceIfChanged(path, source) b.noteFileWrite(changed) if err != nil { return fmt.Errorf("WriteJavaScriptSourceFile: write: %w", err) diff --git a/mdl/backend/modelsdk/javascript_write_retain_test.go b/mdl/backend/modelsdk/javascript_write_retain_test.go new file mode 100644 index 0000000000..1330fff279 --- /dev/null +++ b/mdl/backend/modelsdk/javascript_write_retain_test.go @@ -0,0 +1,113 @@ +// SPDX-License-Identifier: Apache-2.0 + +package modelsdkbackend + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/types" +) + +// Studio Pro's regeneration of a JavaScript action keeps the import list and +// the code between BEGIN EXTRA CODE and END EXTRA CODE. MDL can express neither +// (describe prints the extra code only as a comment), so a `create or modify` +// with an edited body must carry both over from the file on disk — otherwise a +// hand-restored helper is overwritten and the user code calling it breaks +// (mendixlabs/mxcli#1429). +const storedDistanceJS = `// This file was generated by Mendix Studio Pro. +// +// WARNING: Only the following code will be retained when actions are regenerated: +// - the import list +// - the code between BEGIN USER CODE and END USER CODE +// - the code between BEGIN EXTRA CODE and END EXTRA CODE +// Other code you write will be lost the next time you deploy the project. +import "mx-global"; +import { Big } from "big.js"; +import { + sin, + cos +} from "./trig"; + +// BEGIN EXTRA CODE +function deg2rad(deg) { + return deg.times(Math.PI / 180); +} +// END EXTRA CODE + +/** + * @param {Big} latitude + * @returns {Promise.} + */ +export async function GetDistance(latitude) { + // BEGIN USER CODE + return deg2rad(latitude); + // END USER CODE +} +` + +func TestWriteJavaScriptSourceFile_EditedBodyKeepsImportsAndExtraCode(t *testing.T) { + dir := t.TempDir() + b := &Backend{path: filepath.Join(dir, "App.mpr")} + jsDir := b.jsActionSourceDir("WebMod") + if err := os.MkdirAll(jsDir, 0o755); err != nil { + t.Fatal(err) + } + path := filepath.Join(jsDir, "GetDistance.js") + if err := os.WriteFile(path, []byte(storedDistanceJS), 0o644); err != nil { + t.Fatal(err) + } + + params := []*types.JavaActionParameter{{Name: "latitude", IsRequired: true, ParameterType: &types.DecimalType{}}} + if err := b.WriteJavaScriptSourceFile("WebMod", "GetDistance", "return deg2rad(latitude).times(2);", params, &types.DecimalType{}); err != nil { + t.Fatal(err) + } + got, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + src := string(got) + for _, want := range []string{ + `import "mx-global";`, + `import { Big } from "big.js";`, + "import {\n\tsin,\n\tcos\n} from \"./trig\";", + "// BEGIN EXTRA CODE\nfunction deg2rad(deg) {\n return deg.times(Math.PI / 180);\n}\n// END EXTRA CODE", + "\treturn deg2rad(latitude).times(2);", + } { + if !strings.Contains(src, want) { + t.Errorf("regenerated source lost %q:\n%s", want, src) + } + } + // The imports precede the EXTRA CODE section, as Studio Pro writes them. + if strings.Index(src, `import { Big }`) > strings.Index(src, "// BEGIN EXTRA CODE") { + t.Errorf("imports written after the EXTRA CODE section:\n%s", src) + } + + // Regenerating again is stable: nothing duplicated. + if err := b.WriteJavaScriptSourceFile("WebMod", "GetDistance", "return deg2rad(latitude).times(2);", params, &types.DecimalType{}); err != nil { + t.Fatal(err) + } + again, _ := os.ReadFile(path) + if string(again) != src { + t.Errorf("second regeneration changed the file:\n%s", again) + } +} + +// Control: with no file on disk the stub is the plain generated one — no +// imports and an empty EXTRA CODE section. +func TestWriteJavaScriptSourceFile_NewFileHasEmptySections(t *testing.T) { + dir := t.TempDir() + b := &Backend{path: filepath.Join(dir, "App.mpr")} + if err := b.WriteJavaScriptSourceFile("WebMod", "Fresh", "return 1;", nil, nil); err != nil { + t.Fatal(err) + } + got, err := os.ReadFile(filepath.Join(b.jsActionSourceDir("WebMod"), "Fresh.js")) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(got), "\nimport ") || !strings.Contains(string(got), "// BEGIN EXTRA CODE\n// END EXTRA CODE") { + t.Errorf("new stub has unexpected sections:\n%s", got) + } +} diff --git a/mdl/executor/cmd_javascript_actions_write.go b/mdl/executor/cmd_javascript_actions_write.go index 16973201e4..7789cbd62c 100644 --- a/mdl/executor/cmd_javascript_actions_write.go +++ b/mdl/executor/cmd_javascript_actions_write.go @@ -208,10 +208,10 @@ func execCreateJavaScriptAction(ctx *ExecContext, s *ast.CreateJavaScriptActionS // code, so the file is left alone (#731; the Java twin is #637). // // Likewise when the statement restates the stored user code with the stored - // parameters, as unchanged describe output does: regenerating the file from - // it drops the import list, the EXTRA CODE section and Studio Pro's - // signature, none of which MDL can print, so user code that calls an EXTRA - // CODE helper stops working (#731). + // parameters, as unchanged describe output does: regenerating the file + // re-spells Studio Pro's signature, which MDL cannot print (#731). Any write + // that does happen keeps the file's import list and EXTRA CODE section — + // WriteJavaScriptSourceFile retains them (mendixlabs/mxcli#1429). keep := strings.TrimSpace(s.JavaScriptCode) == jsSourceOmittedBody || (existingID != "" && jsSourceUnchanged(ctx, s, storedParamNames)) if !keep { diff --git a/sdk/javaactions/jsretain.go b/sdk/javaactions/jsretain.go new file mode 100644 index 0000000000..6c4e88c43f --- /dev/null +++ b/sdk/javaactions/jsretain.go @@ -0,0 +1,64 @@ +// SPDX-License-Identifier: Apache-2.0 + +package javaactions + +import ( + "regexp" + "strings" +) + +// jsImportDone matches the end of an import statement written without a +// semicolon: `… from "x"` or a side-effect `import "x"`. +var jsImportDone = regexp.MustCompile(`(?s)(\bfrom\s*|^import\s*)["'][^"']*["']\s*$`) + +// JavaScriptRetainedSections reads the two parts of an existing JavaScript +// action source that MDL cannot express but Studio Pro's regeneration keeps — +// its banner lists "the import list" and "the code between BEGIN EXTRA CODE and +// END EXTRA CODE" next to the user code, which the statement always supplies. +// +// Imports are returned as whole statements, a multi-line `import { a, b } +// from "x";` as one entry with its line breaks. The import list is what stands +// before the EXTRA CODE section (or, without one, before the function). Marker +// matching is case-insensitive, as for Java actions. existing is "" when there +// is no source file yet, which yields neither. +// +// Before this, a `create or modify` with an edited body regenerated the file +// from the statement alone, deleting a hand-restored helper and the imports the +// user code relies on (mendixlabs/mxcli#1429). +func JavaScriptRetainedSections(existing string) (imports []string, extraCode string) { + existing = strings.ReplaceAll(existing, "\r\n", "\n") + seen := map[string]bool{} + var cur []string + for line := range strings.SplitSeq(existing, "\n") { + t := strings.TrimSpace(line) + if cur == nil { + if strings.Contains(asciiLower(t), "// begin extra code") || strings.HasPrefix(t, "export ") || + strings.HasPrefix(t, "function ") || strings.HasPrefix(t, "async function ") || strings.HasPrefix(t, "/**") { + break // the import list ends where the code begins + } + if !strings.HasPrefix(t, "import ") && !strings.HasPrefix(t, "import{") && !strings.HasPrefix(t, `import"`) { + continue + } + } + cur = append(cur, strings.TrimRight(line, " \t")) + stmt := strings.TrimSpace(strings.Join(cur, "\n")) + if strings.HasSuffix(t, ";") || jsImportDone.MatchString(stmt) { + if !seen[stmt] { + seen[stmt] = true + imports = append(imports, stmt) + } + cur = nil + } + } + if ec, ok := sliceBetweenFold(existing, "// BEGIN EXTRA CODE", "// END EXTRA CODE"); ok { + lines := strings.Split(ec, "\n") + for len(lines) > 0 && strings.TrimSpace(lines[0]) == "" { + lines = lines[1:] + } + for len(lines) > 0 && strings.TrimSpace(lines[len(lines)-1]) == "" { + lines = lines[:len(lines)-1] + } + extraCode = strings.Join(lines, "\n") + } + return imports, extraCode +} diff --git a/sdk/javaactions/jsretain_test.go b/sdk/javaactions/jsretain_test.go new file mode 100644 index 0000000000..967897ca3c --- /dev/null +++ b/sdk/javaactions/jsretain_test.go @@ -0,0 +1,32 @@ +// SPDX-License-Identifier: Apache-2.0 + +package javaactions + +import ( + "reflect" + "testing" +) + +// The marketplace NanoflowCommons ships its .js with the imports above the +// banner and no blank lines; Studio Pro's own stub puts them below it. Both, +// and imports without semicolons, are the import list (mendixlabs/mxcli#1429). +func TestJavaScriptRetainedSections(t *testing.T) { + src := "import { Big } from \"big.js\"\n" + + "import 'mx-global'\n\n" + + "// This file was generated by Mendix Studio Pro.\n" + + "// begin extra code\n" + + "function deg2rad(deg) {\n return deg;\n}\n" + + "// End Extra Code\n" + + "/**\n * @returns {Promise.}\n */\n" + + "export async function F() {\n\t// BEGIN USER CODE\n\timport('x');\n\t// END USER CODE\n}\n" + imports, extra := JavaScriptRetainedSections(src) + if want := []string{`import { Big } from "big.js"`, `import 'mx-global'`}; !reflect.DeepEqual(imports, want) { + t.Errorf("imports = %q, want %q", imports, want) + } + if want := "function deg2rad(deg) {\n return deg;\n}"; extra != want { + t.Errorf("extra = %q, want %q", extra, want) + } + if imports, extra := JavaScriptRetainedSections(""); imports != nil || extra != "" { + t.Errorf("no file: got %q, %q", imports, extra) + } +} diff --git a/sdk/javaactions/jssource.go b/sdk/javaactions/jssource.go index 5b0cd706bf..3bb3455b18 100644 --- a/sdk/javaactions/jssource.go +++ b/sdk/javaactions/jssource.go @@ -12,8 +12,10 @@ import ( // block, and an exported async function wrapping the user code between // BEGIN/END USER CODE markers. It is pure (depends only on the action's shape), // so both engines share it for byte-identical output. userCode is placed inside -// the USER CODE markers; when empty a not-implemented stub is emitted. -func GenerateJavaScriptSource(actionName string, userCode string, params []*JavaActionParameter, returnType CodeActionReturnType) string { +// the USER CODE markers; when empty a not-implemented stub is emitted. imports +// and extraCode are the sections a regeneration retains from the existing file +// (JavaScriptRetainedSections); both are empty for a new action. +func GenerateJavaScriptSource(actionName string, userCode string, params []*JavaActionParameter, returnType CodeActionReturnType, imports []string, extraCode string) string { var sb strings.Builder sb.WriteString("// This file was generated by Mendix Studio Pro.\n") @@ -24,7 +26,17 @@ func GenerateJavaScriptSource(actionName string, userCode string, params []*Java sb.WriteString("// Other code you write will be lost the next time you deploy the project.\n") sb.WriteString("// Special characters, e.g., é, ö, à, etc. are supported in comments.\n\n") + if len(imports) > 0 { + for _, imp := range imports { + sb.WriteString(imp + "\n") + } + sb.WriteString("\n") + } + sb.WriteString("// BEGIN EXTRA CODE\n") + if strings.TrimSpace(extraCode) != "" { + sb.WriteString(strings.TrimRight(extraCode, "\n") + "\n") + } sb.WriteString("// END EXTRA CODE\n\n") // JSDoc: one @param per parameter, then the @returns. From 32a394630ba1d75bbea1ba0bda0b30f1078d913e Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 13 Oct 2026 06:37:13 +0000 Subject: [PATCH 07/17] test: use testutil.SetHome in the #1441 cache tests (mendixlabs/mxcli#1441) TestNoBareHomeSetenv refuses a bare Setenv("HOME"): os.UserHomeDir reads USERPROFILE on Windows, so the override would leave the real ~/.mxcli in use. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01W3x8Fyvir8kLyPgJs1BDmF --- cmd/mxcli/marketplace/refcache_sources_test.go | 3 ++- cmd/mxcli/marketplace/refcache_test.go | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/cmd/mxcli/marketplace/refcache_sources_test.go b/cmd/mxcli/marketplace/refcache_sources_test.go index 7aaa6b62ad..19508a61d3 100644 --- a/cmd/mxcli/marketplace/refcache_sources_test.go +++ b/cmd/mxcli/marketplace/refcache_sources_test.go @@ -9,6 +9,7 @@ import ( "testing" modelsdk "github.com/mendixlabs/mxcli" + "github.com/mendixlabs/mxcli/internal/testutil" ) // TestCachedReferenceDescribesActionSourcesLikeAFreshOne is mendixlabs/mxcli#1441. @@ -26,7 +27,7 @@ import ( // matters: the gate treats the placeholder as inconclusive, which would turn the // hit's false "Modified" into an equally blocking "Unknown". func TestCachedReferenceDescribesActionSourcesLikeAFreshOne(t *testing.T) { - t.Setenv("HOME", t.TempDir()) + testutil.SetHome(t, t.TempDir()) t.Setenv("MXCLI_NO_REF_CACHE", "") t.Setenv("MXCLI_REF_CACHE_MAX", "0") diff --git a/cmd/mxcli/marketplace/refcache_test.go b/cmd/mxcli/marketplace/refcache_test.go index 069c48123d..1c5f1dce14 100644 --- a/cmd/mxcli/marketplace/refcache_test.go +++ b/cmd/mxcli/marketplace/refcache_test.go @@ -334,7 +334,7 @@ func TestModelOnlyCopyKeepsWhatIsRead(t *testing.T) { // and javasource/ were kept is complete by its own marker, so only the key can // stop it being served — and serving it is mendixlabs/mxcli#1441 all over again. func TestLegacyModelOnlyEntryIsNotServed(t *testing.T) { - t.Setenv("HOME", t.TempDir()) + testutil.SetHome(t, t.TempDir()) t.Setenv("MXCLI_NO_REF_CACHE", "") const versionID, mx = "2059615c-c6f1-4103-aedb-14820c077a1c", "11.12.1" From f8226bcc26827c2e094073102edefbdd6aa61240 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 13 Oct 2026 06:44:50 +0000 Subject: [PATCH 08/17] fix: refuse change and member paths over a list retrieve (mendixlabs/mxcli#1422) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported: "check accepts `change $E (...)` / `$E = empty` where `$E` comes from `retrieve ... limit 1` (a list under mdl 1); mx check then fails" with CE1613 "The selected attribute 'List of M.Ent.Attr' no longer exists.", and after changing `limit 1` to `first`, `create or modify` reported "spliced: 1 replaced" while the CE1613 remained. - Builder: addChangeObjectAction refuses a change over a list variable instead of qualifying the member through "List of …". - Check: new MDL-RETRIEVE03, the reverse of MDL-RETRIEVE01 — a database retrieve without the object range, then `change $E` (CE1613) or a `$E/Attr` path in any later expression (CE0117). Promoted to exec. `set $E = empty` over the list is a Change list Replace and builds (measured on mx 11.6.6), so it is not reported. - Splice: a stored change member qualified through a list type is not spliceable (like #1418's stored import without entity): mdl 0 rebuilds the flow, repairing it; mdl 1 refuses with the reason instead of keeping the stored bytes. describe adds a -- WARNING for that member. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01W3x8Fyvir8kLyPgJs1BDmF --- ...0-12-change-over-list-retrieve-ce1613.json | 1 + cmd/mxcli/syntax/features_microflow.go | 3 +- docs-site/src/appendixes/error-messages.md | 28 +++ .../1422-change-over-list-retrieve.mdl | 14 ++ mdl/executor/cmd_flow_modify.go | 38 ++++ .../cmd_microflows_builder_actions.go | 9 + mdl/executor/cmd_microflows_format_action.go | 11 ++ mdl/executor/list_used_as_object_1422_test.go | 178 ++++++++++++++++++ mdl/executor/validate.go | 4 + mdl/executor/validate_microflow.go | 1 + .../validate_microflow_retrieve_single.go | 101 ++++++++++ 11 files changed, 387 insertions(+), 1 deletion(-) create mode 100644 .claude/skills/fix-issue/findings/mdl-executor/2026-10-12-change-over-list-retrieve-ce1613.json create mode 100644 mdl-examples/bug-tests/1422-change-over-list-retrieve.mdl create mode 100644 mdl/executor/list_used_as_object_1422_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-10-12-change-over-list-retrieve-ce1613.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-12-change-over-list-retrieve-ce1613.json new file mode 100644 index 0000000000..08c6fd83c2 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-12-change-over-list-retrieve-ce1613.json @@ -0,0 +1 @@ +{"date": "2026-10-12", "area": "mdl/executor", "symptom": "mendixlabs/mxcli#1422: under `mdl 1;` `retrieve $E from M.Ent limit 1; change $E (Attr = 'x');` passed check and exec, then mx check reported CE1613 \"The selected attribute 'List of M.Ent.Attr' no longer exists\"; switching to `first` and re-running `create or modify` spliced the retrieve and kept the broken change.", "cause": "addChangeObjectAction qualified the member with fb.varTypes[var] verbatim, including the \"List of \" prefix; MDL-RETRIEVE01 only covered object-used-as-list; describe strips a member to its last dot segment, so the corrupt stored member printed as the clean statement and declaredMatches kept its bytes.", "file": "mdl/executor/cmd_microflows_builder_actions.go", "fix": "Builder refuses change over a list variable (naming CE1613 and `first`); new check rule MDL-RETRIEVE03 (database retrieve without the object range, then `change` or a `$E/Attr` path in any later expression, CE0117), promoted to exec; planFlowModify treats a stored list-qualified member as not spliceable (storedListQualifiedMember, same as #1418's storedImportWithoutEntity); describe appends a -- WARNING for it.", "test": "TestChangeOverListRetrieveRefused1422, TestChangeOverListStoredMemberIsNotKept1422, TestRetrieveListUsedAsObject1422; mdl-examples/bug-tests/1422-change-over-list-retrieve.mdl", "insight": "**describe normalising a stored value to its clean spelling makes corruption invisible to diff-then-patch** — the second instance after #1418, so any member/entity describe prints by trimming (last-dot split, prefix strip) is a candidate. The cheap repair is a stored-shape guard in planFlowModify that returns cannotSplice; corrupting one field of a correctly written unit via UpdateRawUnit reproduces the 'kept over a broken project' case without the old binary. Measured on mx 11.6.6: `set $E = empty` over the list (Change list Replace) builds at 0 errors; `return $E/Name` is CE0117 but mx check hid it behind the CE1613 in the same run — check each shape in its own project copy before deciding what to refuse."} diff --git a/cmd/mxcli/syntax/features_microflow.go b/cmd/mxcli/syntax/features_microflow.go index f82210201d..f874a51d82 100644 --- a/cmd/mxcli/syntax/features_microflow.go +++ b/cmd/mxcli/syntax/features_microflow.go @@ -166,7 +166,8 @@ func init() { "-- Notes:\n" + "-- * FIRST is the one form that binds an object, in every language version.\n" + "-- HEAD() or COUNT() over it is CE0097 at build time and a LOOP CE0100;\n" + - "-- mxcli reports both as MDL-RETRIEVE01 at check time.\n" + + "-- mxcli reports both as MDL-RETRIEVE01 at check time. The reverse,\n" + + "-- CHANGE over a list retrieve, is CE1613 and MDL-RETRIEVE03.\n" + "-- * $Obj/System.owner and $Obj/System.changedBy are not associations a\n" + "-- retrieve can follow (CE0136, MDL-RETRIEVE02). Use\n" + "-- RETRIEVE $U FROM System.User WHERE [id = $Obj/System.owner] FIRST;\n" + diff --git a/docs-site/src/appendixes/error-messages.md b/docs-site/src/appendixes/error-messages.md index 4df80423a7..27f1a70760 100644 --- a/docs-site/src/appendixes/error-messages.md +++ b/docs-site/src/appendixes/error-messages.md @@ -334,6 +334,34 @@ retrieve $u from System.User where [id = $task/System.owner] first; retrieve $u from System.User where [id = $task/System.changedBy] first; ``` +### MDL-RETRIEVE03: `change` over a list retrieve + +``` +$E was retrieved as a list (under `mdl 1;` `limit 1` is a list of one), not a single +object, so `change $E` cannot take it — mxbuild rejects the stored member with CE1613 +"The selected attribute 'List of …' no longer exists". [MDL-RETRIEVE03] +``` + +**Cause:** The reverse of MDL-RETRIEVE01. A database retrieve with no range, with +`limit n [offset n]`, or — under the `mdl 1;` header — with a bare `limit 1` binds a +**list**; a Change object activity takes one object. Before mendixlabs/mxcli#1422, `exec` +wrote such a change with the member qualified through the list type, which `mx check` +reported as CE1613 (measured on 11.6.6). A member path `$E/Attr` in any later expression is +the same mistake and is CE0117 "Error(s) in expression.". `check` and `exec` both refuse +both. `set $E = empty` over the list is a Change list Replace and builds, so it is not +reported. + +**Solution:** Retrieve the object with `first`, or change the items inside a `loop`: + +```mdl +retrieve $E from Mod.Ent first; +change $E (Attr = 'x'); +``` + +A flow already stored with such a member describes with a `-- WARNING` on the change +line. `create or modify` will not splice over it: without the header it rebuilds the flow +(which repairs it), and under `mdl 1;` it refuses with the drop-and-create advice. + ### MDL-EMAIL02 / 03: A `send email` setting Studio Pro would not allow ``` diff --git a/mdl-examples/bug-tests/1422-change-over-list-retrieve.mdl b/mdl-examples/bug-tests/1422-change-over-list-retrieve.mdl new file mode 100644 index 0000000000..881e1c5ff1 --- /dev/null +++ b/mdl-examples/bug-tests/1422-change-over-list-retrieve.mdl @@ -0,0 +1,14 @@ +mdl 1; +-- mendixlabs/mxcli#1422: under `mdl 1;` `retrieve … limit 1` binds a LIST of one. +-- `change $E (Name = 'x')` over it passed check, and exec stored the member as +-- "List of MyFirstModule.Bug1422Ent.Name" — mx check: CE1613 "The selected +-- attribute 'List of …' no longer exists". Now check/exec refuse that form as +-- MDL-RETRIEVE03. This script is the working spelling: `first` binds the object. +-- Expected: check passes, exec creates the microflow, mx check reports 0 errors. +create persistent entity MyFirstModule.Bug1422Ent ( Name: String(100) ); + +create or modify microflow MyFirstModule.MF_Bug1422_ChangeFirst () +begin + retrieve $E from MyFirstModule.Bug1422Ent first; + change $E ( Name = 'x' ); +end; diff --git a/mdl/executor/cmd_flow_modify.go b/mdl/executor/cmd_flow_modify.go index 0d96cb32d2..c28e57e33b 100644 --- a/mdl/executor/cmd_flow_modify.go +++ b/mdl/executor/cmd_flow_modify.go @@ -248,6 +248,13 @@ func planFlowModify(ctx *ExecContext, d *flowDecl) (*flowPlan, error) { if mapping := storedImportWithoutEntity(a.mf.ObjectCollection); mapping != "" { return nil, cannotSplice("its stored `import from mapping %s` activity has no result entity, which Mendix cannot load (mendixlabs/mxcli#1418)", mapping) } + // The same shape for a change activity mxcli wrote over a list before + // mendixlabs/mxcli#1422: the member is qualified through the list type + // ("List of M.E.Attr", CE1613), describe prints it as the bare attribute, + // and a splice that replaced only the retrieve kept it. + if member := storedListQualifiedMember(a.mf.ObjectCollection); member != "" { + return nil, cannotSplice("its stored change activity names the member %q, qualified through a list type, which mxbuild rejects with CE1613 (mendixlabs/mxcli#1422)", member) + } stored, err := describedFlowStmt(ctx, d, a) if err != nil { @@ -415,6 +422,37 @@ func storedImportWithoutEntity(oc *microflows.MicroflowObjectCollection) string return "" } +// storedListQualifiedMember returns the first change-object member in oc (loop +// bodies included) qualified through a list type ("List of M.E.Attr"), or "". +func storedListQualifiedMember(oc *microflows.MicroflowObjectCollection) string { + if oc == nil { + return "" + } + for _, obj := range oc.Objects { + switch o := obj.(type) { + case *microflows.ActionActivity: + if ch, ok := o.Action.(*microflows.ChangeObjectAction); ok { + for _, m := range ch.Changes { + if isListQualifiedMember(m) { + return m.AttributeQualifiedName + m.AssociationQualifiedName + } + } + } + case *microflows.LoopedActivity: + if m := storedListQualifiedMember(o.ObjectCollection); m != "" { + return m + } + } + } + return "" +} + +// isListQualifiedMember reports a member change whose name was qualified +// through a variable's list type, which no domain model member can have. +func isListQualifiedMember(m *microflows.MemberChange) bool { + return m != nil && (strings.HasPrefix(m.AttributeQualifiedName, "List of ") || strings.HasPrefix(m.AssociationQualifiedName, "List of ")) +} + // reportUnchanged reports a statement that wrote nothing, collapsing into the // run's summary like an elided write does. func reportUnchanged(ctx *ExecContext, what string) { diff --git a/mdl/executor/cmd_microflows_builder_actions.go b/mdl/executor/cmd_microflows_builder_actions.go index a51e5db312..89413afc8b 100644 --- a/mdl/executor/cmd_microflows_builder_actions.go +++ b/mdl/executor/cmd_microflows_builder_actions.go @@ -350,6 +350,15 @@ func (fb *flowBuilder) addChangeObjectAction(s *ast.ChangeObjectStmt) model.ID { if fb.varTypes != nil { entityQN = fb.varTypes[s.Variable] } + // A Change object activity takes one object. Over a list the member was + // qualified through the list type — Attribute "List of M.E.Attr", CE1613 + // at build time (mendixlabs/mxcli#1422) — so refuse it rather than write it. + if fb.isListVariable(s.Variable) && len(s.Changes) > 0 { + fb.addError("$%s is a list (%s), not an object: `change $%s` would store the member as %q, which mxbuild rejects "+ + "with CE1613 — retrieve the object with `retrieve $%s from … first;` (under `mdl 1;` `limit 1` is a list of one), "+ + "or change the list's items inside a loop", + s.Variable, entityQN, s.Variable, entityQN+"."+s.Changes[0].Attribute, s.Variable) + } // Build MemberChange items for each SET assignment for _, change := range s.Changes { diff --git a/mdl/executor/cmd_microflows_format_action.go b/mdl/executor/cmd_microflows_format_action.go index c80f6acd12..4dd6274962 100644 --- a/mdl/executor/cmd_microflows_format_action.go +++ b/mdl/executor/cmd_microflows_format_action.go @@ -472,10 +472,14 @@ func formatAction( if len(a.Changes) > 0 { var members []string var unspelled []string + var listQualified []string for _, m := range a.Changes { if m.Type == microflows.MemberChangeTypeAdd || m.Type == microflows.MemberChangeTypeRemove { unspelled = append(unspelled, fmt.Sprintf("%s (%s)", m.AssociationQualifiedName, m.Type)) } + if isListQualifiedMember(m) { + listQualified = append(listQualified, m.AttributeQualifiedName+m.AssociationQualifiedName) + } var memberName string // Check if this is an association change or an attribute change if m.AssociationQualifiedName != "" { @@ -504,6 +508,13 @@ func formatAction( "re-executing this line replaces the set — split it into `add`/`remove … $%s/` statements", strings.Join(unspelled, ", "), varName) } + if len(listQualified) > 0 { + // Written over a list before mendixlabs/mxcli#1422: the bare + // name above is not what is stored, and mxbuild reports CE1613. + stmt += fmt.Sprintf(" -- WARNING: stored member %s is qualified through a list type (CE1613); "+ + "re-create this flow with $%s retrieved as an object (`first`)", + strings.Join(listQualified, ", "), varName) + } return stmt } return fmt.Sprintf("change $%s%s%s;", varName, commitModifier(a.Commit), refreshModifier(a.RefreshInClient)) diff --git a/mdl/executor/list_used_as_object_1422_test.go b/mdl/executor/list_used_as_object_1422_test.go new file mode 100644 index 0000000000..26148a46b6 --- /dev/null +++ b/mdl/executor/list_used_as_object_1422_test.go @@ -0,0 +1,178 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "context" + "strings" + "testing" + + "go.mongodb.org/mongo-driver/bson" + + "github.com/mendixlabs/mxcli/mdl/ast" +) + +// mendixlabs/mxcli#1422: under `mdl 1;` `retrieve … limit 1` binds a list of +// one, and `change $E (…)` over it was accepted by check and written by exec +// with the member qualified through the variable's type — Attribute = +// "List of Module.Entity.Name", which mx check reports as CE1613 "The selected +// attribute 'List of …' no longer exists". + +const lao1422Entity = `create persistent entity MyFirstModule.EntL1422 ( Name: string(100) ); +` + +func lao1422Flow(rng string) string { + return `create or modify microflow MyFirstModule.MF_L1422 () +begin + retrieve $E from MyFirstModule.EntL1422 ` + rng + `; + change $E ( Name = 'x' ); +end; +` +} + +// changeItemAttributes collects the Attribute of every stored change member +// (Microflows$ChangeActionItem), and when set != nil replaces it. +func changeItemAttributes(v any, set *string, out *[]string) any { + switch x := v.(type) { + case bson.D: + typ, _ := afGet(x, "$Type").(string) + item := strings.HasSuffix(typ, "$ChangeActionItem") || strings.HasSuffix(typ, "$MemberChange") + for i := range x { + if item && x[i].Key == "Attribute" { + s, _ := x[i].Value.(string) + *out = append(*out, s) + if set != nil { + x[i].Value = *set + } + continue + } + x[i].Value = changeItemAttributes(x[i].Value, set, out) + } + return x + case bson.A: + for i := range x { + x[i] = changeItemAttributes(x[i], set, out) + } + return x + } + return v +} + +func storedChangeAttributes(t *testing.T, exec *Executor) []string { + t.Helper() + _, raw := mfUnitByName(t, exec, "MyFirstModule", "MF_L1422") + if raw == nil { + return nil + } + var d bson.D + if err := bson.Unmarshal(raw, &d); err != nil { + t.Fatal(err) + } + var got []string + changeItemAttributes(d, nil, &got) + return got +} + +// The builder refuses a change over a list instead of writing the corrupt +// member; the object range (`first`) is the control. +func TestChangeOverListRetrieveRefused1422(t *testing.T) { + exec, _ := openPedAppFixture(t) + err := agreeExec(t, exec, "mdl 1;\n"+lao1422Entity+lao1422Flow("limit 1")) + if err == nil || !strings.Contains(err.Error(), "CE1613") { + t.Fatalf("exec err = %v, want a refusal naming CE1613; stored change attributes: %q", err, storedChangeAttributes(t, exec)) + } + if id, _ := mfUnitByName(t, exec, "MyFirstModule", "MF_L1422"); id != "" { + t.Fatalf("the refused microflow was written anyway (%s)", id) + } + + if err := agreeExec(t, exec, "mdl 1;\n"+lao1422Flow("first")); err != nil { + t.Fatalf("control: `first` failed: %v", err) + } + if got := storedChangeAttributes(t, exec); len(got) != 1 || got[0] != "MyFirstModule.EntL1422.Name" { + t.Fatalf("control: stored change attributes = %q, want [MyFirstModule.EntL1422.Name]", got) + } +} + +// A project already written by the old bug: changing `limit 1` to `first` +// spliced the retrieve and kept the corrupt change activity, because describe +// prints it as the clean `change $E (Name = 'x')`. The stored shape now counts +// as a change the splice cannot make: mdl 0 rebuilds (healing it), mdl 1 +// refuses with the reason instead of keeping it. +func TestChangeOverListStoredMemberIsNotKept1422(t *testing.T) { + exec, out := openPedAppFixture(t) + if err := agreeExec(t, exec, "mdl 1;\n"+lao1422Entity+lao1422Flow("first")); err != nil { + t.Fatal(err) + } + id, raw := mfUnitByName(t, exec, "MyFirstModule", "MF_L1422") + var d bson.D + if err := bson.Unmarshal(raw, &d); err != nil { + t.Fatal(err) + } + corrupt := "List of MyFirstModule.EntL1422.Name" + var got []string + changeItemAttributes(d, &corrupt, &got) + if len(got) != 1 { + t.Fatalf("expected one change member in MF_L1422, found %q", got) + } + b, err := bson.Marshal(d) + if err != nil { + t.Fatal(err) + } + if err := exec.newExecContext(context.Background()).Backend.UpdateRawUnit(string(id), b); err != nil { + t.Fatal(err) + } + + out.Reset() + err = agreeExec(t, exec, "mdl 1;\n"+lao1422Flow("first")) + if err == nil || !strings.Contains(err.Error(), "List of") { + t.Fatalf("mdl 1 re-run over a stored list-qualified member: err = %v, output %q; want a refusal naming it", err, out.String()) + } + + out.Reset() + if err := agreeExec(t, exec, lao1422Flow("first")); err != nil { + t.Fatalf("mdl 0 re-run: %v", err) + } + if got := storedChangeAttributes(t, exec); len(got) != 1 || got[0] != "MyFirstModule.EntL1422.Name" { + t.Fatalf("after the mdl 0 re-run the stored change attributes are %q, want [MyFirstModule.EntL1422.Name] (output %q)", got, out.String()) + } +} + +// check: MDL-RETRIEVE03 is the reverse of MDL-RETRIEVE01 — a list retrieve +// used as an object by `change`. +func TestRetrieveListUsedAsObject1422(t *testing.T) { + change := &ast.ChangeObjectStmt{Variable: "E", Changes: []ast.ChangeItem{{Attribute: "Name", Value: &ast.LiteralExpr{Value: "x", Kind: ast.LiteralString}}}} + rule := func(stmt *ast.CreateMicroflowStmt) []string { + var out []string + for _, v := range ValidateMicroflow(stmt) { + if v.RuleID == retrieveListAsObjectRule { + out = append(out, v.Message) + } + } + return out + } + if got := rule(mfWith(retrieveLimit("E", "1", ""), change)); len(got) != 1 || !strings.Contains(got[0], "CE1613") { + t.Fatalf("change over an mdl 1 `limit 1` retrieve: %s = %q, want one violation naming CE1613", retrieveListAsObjectRule, got) + } + if got := rule(mfWith(retrieveFirst("E"), change)); len(got) != 0 { + t.Fatalf("control: change over `first` reported %q", got) + } + // A member path over the list is CE0117 (measured, mx 11.6.6). + ret := &ast.ReturnStmt{Value: &ast.AttributePathExpr{Variable: "E", Path: []string{"Name"}}} + if got := rule(mfWith(retrieveLimit("E", "1", ""), ret)); len(got) != 1 || !strings.Contains(got[0], "CE0117") { + t.Fatalf("return $E/Name over a list retrieve: %s = %q, want one violation naming CE0117", retrieveListAsObjectRule, got) + } + if got := rule(mfWith(retrieveFirst("E"), ret)); len(got) != 0 { + t.Fatalf("control: return $E/Name over `first` reported %q", got) + } + // `set $E = empty` over the list is a Change list Replace, which builds. + set := &ast.MfSetStmt{Target: "E", Value: &ast.LiteralExpr{Kind: ast.LiteralEmpty}} + if got := rule(mfWith(retrieveLimit("E", "1", ""), set)); len(got) != 0 { + t.Fatalf("set $E = empty over a list retrieve reported %q", got) + } + // An association retrieve may bind an object; the rule leaves it alone. + assoc := retrieveLimit("E", "", "") + assoc.StartVariable = "Owner" + if got := rule(mfWith(assoc, change)); len(got) != 0 { + t.Fatalf("change over an association retrieve reported %q", got) + } +} diff --git a/mdl/executor/validate.go b/mdl/executor/validate.go index b575d56e5b..89fa8fdbba 100644 --- a/mdl/executor/validate.go +++ b/mdl/executor/validate.go @@ -1886,6 +1886,10 @@ var execEnforcedMicroflowRules = map[string]bool{ // MDL-RETRIEVE02: a retrieve by association over System.owner / // System.changedBy is CE0136, measured on 11.12.2 (mendixlabs/mxcli#1358). "MDL-RETRIEVE02": true, + // MDL-RETRIEVE03: `change` over a list retrieve is CE1613 on the stored + // member, measured on 11.6.6 (mendixlabs/mxcli#1422). The builder refuses + // it too, for the flows this AST walk cannot type. + "MDL-RETRIEVE03": true, } // validateMicroflowRules runs the MDL0xx microflow rule set (ValidateMicroflow) diff --git a/mdl/executor/validate_microflow.go b/mdl/executor/validate_microflow.go index ad618c8945..31956cc1b4 100644 --- a/mdl/executor/validate_microflow.go +++ b/mdl/executor/validate_microflow.go @@ -120,6 +120,7 @@ func (v *microflowValidator) addViolation(ruleID string, severity linter.Severit func (v *microflowValidator) validate(body []ast.MicroflowStatement) { v.checkListOperationIterator(body) v.checkRetrieveLimitOneAsList(body) + v.checkRetrieveListAsObject(body) v.checkShowMessageType(body) v.checkRetrieveOverSystemAssociation(body) v.checkSetOnObjectVariable(v.params, body) diff --git a/mdl/executor/validate_microflow_retrieve_single.go b/mdl/executor/validate_microflow_retrieve_single.go index dfbbb64d44..b462977e1e 100644 --- a/mdl/executor/validate_microflow_retrieve_single.go +++ b/mdl/executor/validate_microflow_retrieve_single.go @@ -4,6 +4,7 @@ package executor import ( "fmt" + "reflect" "github.com/mendixlabs/mxcli/mdl/ast" "github.com/mendixlabs/mxcli/mdl/linter" @@ -63,6 +64,106 @@ func (v *microflowValidator) checkRetrieveLimitOneAsList(body []ast.MicroflowSta }) } +// retrieveListAsObjectRule is the rule ID for "a list retrieve used as an object". +const retrieveListAsObjectRule = "MDL-RETRIEVE03" + +// checkRetrieveListAsObject is MDL-RETRIEVE01 the other way round: a variable a +// database retrieve bound to a LIST — no range, `limit n [offset n]`, and under +// `mdl 1;` a bare `limit 1`, a list of one (ako/mxcli#734) — that a later +// `change` takes as the object. mxcli used to write the member qualified +// through the list type, and mxbuild answered CE1613 "The selected attribute +// 'List of Module.Entity.Attr' no longer exists" (mendixlabs/mxcli#1422). +// +// Only a database retrieve is judged: a retrieve over an association binds an +// object or a list depending on the association, which this AST walk does not +// know. A member path `$E/Attr` in any expression of a later statement is the +// same mistake (CE0117 "Error(s) in expression.", measured on 11.6.6). `set $E +// = empty` over the list is a Change list Replace and builds (measured), so it +// is not flagged. +func (v *microflowValidator) checkRetrieveListAsObject(body []ast.MicroflowStatement) { + list := map[string]bool{} + forEachMicroflowStatement(body, func(s ast.MicroflowStatement) { + if c, ok := s.(*ast.ChangeObjectStmt); ok && list[c.Variable] { + v.addViolation(retrieveListAsObjectRule, linter.SeverityError, + fmt.Sprintf("$%s was retrieved as a list (under `mdl 1;` `limit 1` is a list of one), "+ + "not a single object, so `change $%s` cannot take it — mxbuild rejects the stored member "+ + "with CE1613 \"The selected attribute 'List of …' no longer exists\".", c.Variable, c.Variable), + fmt.Sprintf("Retrieve the object with `retrieve $%s from … first;`, or keep the list and "+ + "change its items inside a `loop`.", c.Variable)) + } + reported := map[string]bool{} + for _, e := range statementOwnExpressions(s) { + for _, name := range predicatePathVariables(e) { + if !list[name] || reported[name] { + continue + } + reported[name] = true + v.addViolation(retrieveListAsObjectRule, linter.SeverityError, + fmt.Sprintf("$%s was retrieved as a list (under `mdl 1;` `limit 1` is a list of one), "+ + "not a single object, so a member path `$%s/…` has nothing to navigate — mxbuild "+ + "rejects the expression with CE0117 \"Error(s) in expression.\".", name, name), + fmt.Sprintf("Retrieve the object with `retrieve $%s from … first;`, or take one item "+ + "of the list first (`$item = head($%s);`).", name, name)) + } + } + for _, p := range statementProducedVars(s) { + delete(list, p.name) + } + if r, ok := s.(*ast.RetrieveStmt); ok && !r.First && r.StartVariable == "" && r.Variable != "" { + list[r.Variable] = true + } + }) +} + +// statementOwnExpressions returns the expressions a statement holds itself, in +// any field or slice of structs, but not those of the statements nested in its +// bodies (forEachMicroflowStatement visits those on their own). +func statementOwnExpressions(s ast.MicroflowStatement) []ast.Expression { + exprType := reflect.TypeOf((*ast.Expression)(nil)).Elem() + stmtType := reflect.TypeOf((*ast.MicroflowStatement)(nil)).Elem() + var out []ast.Expression + var walk func(rv reflect.Value, top bool) + walk = func(rv reflect.Value, top bool) { + switch rv.Kind() { + case reflect.Interface: + if rv.IsNil() { + return + } + if rv.Type() == stmtType && !top { + return + } + if rv.Type() == exprType { + out = append(out, rv.Interface().(ast.Expression)) + return + } + walk(rv.Elem(), top) + case reflect.Pointer: + if rv.IsNil() { + return + } + if !top && rv.Type().Implements(stmtType) { + return + } + walk(rv.Elem(), false) + case reflect.Struct: + for i := 0; i < rv.NumField(); i++ { + if rv.Type().Field(i).IsExported() { + walk(rv.Field(i), false) + } + } + case reflect.Slice: + if rv.Type().Elem() == stmtType { + return + } + for i := 0; i < rv.Len(); i++ { + walk(rv.Index(i), false) + } + } + } + walk(reflect.ValueOf(s), true) + return out +} + // listUseOf reports the list variable a statement consumes, and a phrase naming // what consumes it. ("", "") when the statement takes no list. func listUseOf(s ast.MicroflowStatement) (string, string) { From 7a35260d84f2c74cc4764f08dfb13a7e9dda6925 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 13 Oct 2026 06:53:17 +0000 Subject: [PATCH 09/17] fix: describe module with all emits every document type (mendixlabs/mxcli#1421) Reported: "`describe module with all` omits whole document types" -- nanoflows, snippets, JS actions, JSON structures, import/export mappings, task queues, image collections -- with no error. On PedApp it emitted 0 of FeedbackModule's 11 nanoflows (20 of its 49 documents). The WITH ALL branch was a hand-written section per document type covering 13 types, each wrapped in `if err == nil`, so a failed list or describe was skipped silently too. - One ordered registry of describable document kinds (describe_module_all.go), each rendered through describeByKind, the dispatch extracted from execDescribe, so the dump holds exactly what each document's own describe prints. Order is dependency-first: enumerations, constants, consumed OData, domain model, leaves (regex, images, icons), structures and mappings, connections and consumed services, task queues, Java/JS actions, rules, microflows, nanoflows, layouts, snippets, building blocks, pages, menus, workflows, published services, scheduled events, agent-editor documents. - A type-blind ListDocumentUnits pass prints `-- not describable: M.N` for any module unit no entry emitted (page templates today). - Read-only reports (building block, icon collection) are emitted commented out, so the dump still parses. - A list or describe failure prints `-- describe failed: : ` and fails the statement after the rest of the module is written, as the output line limit does (#1425). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01W3x8Fyvir8kLyPgJs1BDmF --- ...e-module-all-hand-list-omits-doctypes.json | 1 + mdl/executor/cmd_modules.go | 224 ++--------- mdl/executor/cmd_modules_describe_all_test.go | 236 ++++++++++++ mdl/executor/describe_module_all.go | 351 ++++++++++++++++++ mdl/executor/executor_query.go | 230 ++++++------ 5 files changed, 738 insertions(+), 304 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/mdl-executor/2026-10-12-describe-module-all-hand-list-omits-doctypes.json create mode 100644 mdl/executor/cmd_modules_describe_all_test.go create mode 100644 mdl/executor/describe_module_all.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-10-12-describe-module-all-hand-list-omits-doctypes.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-12-describe-module-all-hand-list-omits-doctypes.json new file mode 100644 index 0000000000..34e9eb1bb9 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-12-describe-module-all-hand-list-omits-doctypes.json @@ -0,0 +1 @@ +{"date": "2026-10-12", "area": "mdl/executor", "symptom": "mendixlabs/mxcli#1421: `describe module ... with all` omits whole document types (nanoflows, snippets, JS actions, JSON structures, import/export mappings, task queues, image collections) with no error; on PedApp it emitted 0 of FeedbackModule's 11 nanoflows and 20 of its 49 documents.", "cause": "describeModule's WITH ALL branch was a hand-written section per document type covering 13 types while describe handlers exist for 30-odd; every section was `if err == nil { ... }`, so a failing list or describe was skipped too.", "file": "mdl/executor/cmd_modules.go; mdl/executor/describe_module_all.go; mdl/executor/executor_query.go", "fix": "One ordered registry (moduleDocKindsBefore/AfterDomainModel) of describable document kinds, each rendered through describeByKind (the dispatch extracted from execDescribe); a type-blind ListDocumentUnits pass prints `-- not describable: M.N` for any module unit no entry emitted; read-only reports (building block, icon collection) are emitted commented out; a describe or list failure prints `-- describe failed: ...` and fails the statement after the rest is written (as #1425 does).", "test": "TestDescribeModuleWithAll_AccountsForEveryDocument (each unit's own describe must appear in the dump; reverted: FeedbackModule 20 of 49, Atlas_Web_Content 0 of 88), TestModuleDocKindsCoverDescribableTypes, TestDescribeModuleWithAll_ReportsDescribeFailure", "insight": "**A per-type list next to a type-blind enumeration is the drift signal** — this was the fourth hand-kept copy of 'which document types exist' (catalog view, describe_auto, cmd/mxcli dispatch, scriptdiff). Count against ListDocumentUnits, which cannot miss a kind because it never asks for one, and compare each document's own describe output rather than counting `create` lines: a count passes when a document is dumped under the wrong name."} diff --git a/mdl/executor/cmd_modules.go b/mdl/executor/cmd_modules.go index e90663274c..f54ef8b782 100644 --- a/mdl/executor/cmd_modules.go +++ b/mdl/executor/cmd_modules.go @@ -666,208 +666,48 @@ func describeModule(ctx *ExecContext, moduleName string, withAll bool) error { // Output separator fmt.Fprintln(ctx.Output) - // Output enumerations in this module (no dependencies between enums) - if enums, err := ctx.Backend.ListEnumerations(); err == nil { - for _, enum := range enums { - if moduleContainers[enum.ContainerID] { - if err := describeEnumeration(ctx, ast.QualifiedName{Module: moduleName, Name: enum.Name}); err == nil { - fmt.Fprintln(ctx.Output) - } - } - } - } - - // Output constants (may reference enumerations) - if constants, err := ctx.Backend.ListConstants(); err == nil { - for _, c := range constants { - if moduleContainers[c.ContainerID] { - if err := outputConstantMDL(ctx, c, moduleName); err == nil { - fmt.Fprintln(ctx.Output) - } - } - } - } - - // Output entities in dependency order (base entities before derived entities) - // and associations after all entities - if dm, err := ctx.Backend.GetDomainModel(targetModule.ID); err == nil { - // Topologically sort entities by generalization (inheritance) - sortedEntities := sortEntitiesByGeneralization(dm.Entities, moduleName) - - // Output entities in sorted order - for _, entity := range sortedEntities { - if err := describeEntity(ctx, ast.QualifiedName{Module: moduleName, Name: entity.Name}); err == nil { - fmt.Fprintln(ctx.Output) - } - } - - // Output associations (after all entities are defined) - for _, assoc := range dm.Associations { - if err := describeAssociation(ctx, ast.QualifiedName{Module: moduleName, Name: assoc.Name}); err == nil { - fmt.Fprintln(ctx.Output) + // Every document of the module, in the registry's dependency order, with + // the domain model's entities and associations after enumerations and + // constants (mendixlabs/mxcli#1421). + run := &moduleDescribeRun{ctx: ctx, module: moduleName, containers: moduleContainers, emitted: map[model.ID]bool{}} + run.emitKinds(moduleDocKindsBeforeDomainModel) + + // Entities in dependency order (base entities before derived entities), + // then associations once every entity is defined. + dm, err := ctx.Backend.GetDomainModel(targetModule.ID) + if err != nil { + run.fail("domain model "+moduleName, err) + } else if dm != nil { + for _, entity := range sortEntitiesByGeneralization(dm.Entities, moduleName) { + qn := ast.QualifiedName{Module: moduleName, Name: entity.Name} + if err := describeEntity(ctx, qn); err != nil { + run.fail("entity "+qn.String(), err) + continue } + fmt.Fprintln(ctx.Output) } // Cross-module associations are stored in a separate collection but are // owned by this module (the FROM entity lives here); emit them too. - for _, ca := range dm.CrossAssociations { - if err := describeAssociation(ctx, ast.QualifiedName{Module: moduleName, Name: ca.Name}); err == nil { - fmt.Fprintln(ctx.Output) - } + var assocNames []string + for _, a := range dm.Associations { + assocNames = append(assocNames, a.Name) } - } - - // Output microflows - if mfs, err := ctx.Backend.ListMicroflows(); err == nil { - for _, mf := range mfs { - if moduleContainers[mf.ContainerID] { - if err := describeMicroflow(ctx, ast.QualifiedName{Module: moduleName, Name: mf.Name}); err == nil { - fmt.Fprintln(ctx.Output) - } - } - } - } - - // Output java actions - if jaList, err := ctx.Backend.ListJavaActions(); err == nil { - for _, ja := range jaList { - if moduleContainers[ja.ContainerID] { - if err := describeJavaAction(ctx, ast.QualifiedName{Module: moduleName, Name: ja.Name}); err == nil { - fmt.Fprintln(ctx.Output) - } - } - } - } - - // Output pages - if pageList, err := ctx.Backend.ListPages(); err == nil { - for _, p := range pageList { - if moduleContainers[p.ContainerID] { - if err := describePage(ctx, ast.QualifiedName{Module: moduleName, Name: p.Name}); err == nil { - fmt.Fprintln(ctx.Output) - } - } - } - } - - // Output snippets - if snippets, err := ctx.Backend.ListSnippets(); err == nil { - for _, s := range snippets { - if moduleContainers[s.ContainerID] { - if err := describeSnippet(ctx, ast.QualifiedName{Module: moduleName, Name: s.Name}); err == nil { - fmt.Fprintln(ctx.Output) - } - } - } - } - - // Output layouts - if layouts, err := ctx.Backend.ListLayouts(); err == nil { - for _, l := range layouts { - if moduleContainers[l.ContainerID] { - if err := describeLayout(ctx, ast.QualifiedName{Module: moduleName, Name: l.Name}); err == nil { - fmt.Fprintln(ctx.Output) - } - } - } - } - - // Output database connections - if conns, err := ctx.Backend.ListDatabaseConnections(); err == nil { - for _, conn := range conns { - if moduleContainers[conn.ContainerID] { - if err := outputDatabaseConnectionMDL(ctx, conn, moduleName); err == nil { - fmt.Fprintln(ctx.Output) - } - } - } - } - - // Output business event services - if services, err := ctx.Backend.ListBusinessEventServices(); err == nil { - for _, svc := range services { - if moduleContainers[svc.ContainerID] { - if err := describeBusinessEventService(ctx, ast.QualifiedName{Module: moduleName, Name: svc.Name}); err == nil { - fmt.Fprintln(ctx.Output) - } - } - } - } - - // Get hierarchy for folder path resolution (used by OData sections below) - h, _ := getHierarchy(ctx) - - // Output consumed OData services (clients) - if services, err := ctx.Backend.ListConsumedODataServices(); err == nil { - for _, svc := range services { - if moduleContainers[svc.ContainerID] { - folderPath := "" - if h != nil { - folderPath = h.BuildFolderPath(svc.ContainerID) - } - if err := outputConsumedODataServiceMDL(ctx, svc, moduleName, folderPath); err == nil { - fmt.Fprintln(ctx.Output) - } - } - } - } - - // Output published OData services - if services, err := ctx.Backend.ListPublishedODataServices(); err == nil { - for _, svc := range services { - if moduleContainers[svc.ContainerID] { - folderPath := "" - if h != nil { - folderPath = h.BuildFolderPath(svc.ContainerID) - } - if err := outputPublishedODataServiceMDL(ctx, svc, moduleName, folderPath); err == nil { - fmt.Fprintln(ctx.Output) - } - } - } - } - - // Output agent-editor documents. Order matches referential dependencies: - // Model / KnowledgeBase / ConsumedMCPService first (leaves), Agent last - // (may reference the others via its TOOL / KNOWLEDGE BASE / MCP SERVICE - // blocks). - if models, err := ctx.Backend.ListAgentEditorModels(); err == nil { - for _, m := range models { - if moduleContainers[m.ContainerID] { - if err := describeAgentEditorModel(ctx, ast.QualifiedName{Module: moduleName, Name: m.Name}); err == nil { - fmt.Fprintln(ctx.Output) - } - } - } - } - if kbs, err := ctx.Backend.ListAgentEditorKnowledgeBases(); err == nil { - for _, kb := range kbs { - if moduleContainers[kb.ContainerID] { - if err := describeAgentEditorKnowledgeBase(ctx, ast.QualifiedName{Module: moduleName, Name: kb.Name}); err == nil { - fmt.Fprintln(ctx.Output) - } - } - } - } - if svcs, err := ctx.Backend.ListAgentEditorConsumedMCPServices(); err == nil { - for _, svc := range svcs { - if moduleContainers[svc.ContainerID] { - if err := describeAgentEditorConsumedMCPService(ctx, ast.QualifiedName{Module: moduleName, Name: svc.Name}); err == nil { - fmt.Fprintln(ctx.Output) - } - } + for _, ca := range dm.CrossAssociations { + assocNames = append(assocNames, ca.Name) } - } - if agents, err := ctx.Backend.ListAgentEditorAgents(); err == nil { - for _, a := range agents { - if moduleContainers[a.ContainerID] { - if err := describeAgentEditorAgent(ctx, ast.QualifiedName{Module: moduleName, Name: a.Name}); err == nil { - fmt.Fprintln(ctx.Output) - } + for _, name := range assocNames { + qn := ast.QualifiedName{Module: moduleName, Name: name} + if err := describeAssociation(ctx, qn); err != nil { + run.fail("association "+qn.String(), err) + continue } + fmt.Fprintln(ctx.Output) } } - return nil + run.emitKinds(moduleDocKindsAfterDomainModel) + run.reportUnemitted() + return run.err() } // sortEntitiesByGeneralization returns entities sorted so base entities come before derived entities. diff --git a/mdl/executor/cmd_modules_describe_all_test.go b/mdl/executor/cmd_modules_describe_all_test.go new file mode 100644 index 0000000000..8aa5d2c339 --- /dev/null +++ b/mdl/executor/cmd_modules_describe_all_test.go @@ -0,0 +1,236 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "bytes" + "context" + "errors" + "os" + "path/filepath" + "regexp" + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/backend" + "github.com/mendixlabs/mxcli/mdl/backend/mock" + modelsdkbackend "github.com/mendixlabs/mxcli/mdl/backend/modelsdk" + "github.com/mendixlabs/mxcli/mdl/types" + "github.com/mendixlabs/mxcli/mdl/visitor" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/agenteditor" + "github.com/mendixlabs/mxcli/sdk/domainmodel" + "github.com/mendixlabs/mxcli/sdk/microflows" + "github.com/mendixlabs/mxcli/sdk/security" +) + +// languageHeader matches the `mdl N;` line describe prints ahead of its MDL. +var languageHeader = regexp.MustCompile(`(?m)^mdl \d+;\n`) + +// catalogProgress matches the lines bare `describe M.N` prints while it loads +// the catalog it resolves the type through, ahead of the MDL. +var catalogProgress = regexp.MustCompile(`\A(?:(?:Loading cached catalog|Building catalog|✓ )[^\n]*\n)+`) + +// `describe module M with all` must account for every document in the module: +// each one is either in the output — exactly as its own `describe` prints it — +// (commented out, for a read-only report such as a building block) or named +// on a `-- not describable:` line. A hand-kept list of document types +// is what this replaced, and it emitted 0 of FeedbackModule's 11 nanoflows (and +// none of its JavaScript actions, JSON structures, mappings, image collections +// or its building block) with no error (mendixlabs/mxcli#1421). +// +// The documents are enumerated type-blind (ListDocumentUnits, the way +// project-tree counts a module), so a document type nobody wired into the +// registry fails here rather than going missing. +func TestDescribeModuleWithAll_AccountsForEveryDocument(t *testing.T) { + for _, tc := range []struct { + fixture, mpr, module string + // parses is false where a document's own describe does not parse + // today — a JavaScript action with a type parameter prints + // `Name` / `entity <>`, which the grammar rejects. That is the + // JavaScript action describer's defect, not the module dump's; the + // module is still checked for completeness. + parses bool + }{ + {"pedapp", "PedApp.mpr", "FeedbackModule", false}, + {"pedapp", "PedApp.mpr", "Atlas_Core", true}, // icon collections: read-only reports + {"pedapp", "PedApp.mpr", "Atlas_Web_Content", true}, // building blocks, page templates + {"pedapp", "PedApp.mpr", "Administration", true}, + {"testapp-views", "TestApp.mpr", "MyFirstModule", true}, // view entity, published OData + } { + module := tc.module + t.Run(tc.fixture+"/"+module, func(t *testing.T) { + exec, out := openFixtureCopy(t, tc.fixture, tc.mpr) + out.Reset() + if err := exec.Execute(&ast.DescribeStmt{ObjectType: ast.DescribeModule, Name: ast.QualifiedName{Module: module}, WithAll: true}); err != nil { + t.Fatalf("describe module %s with all: %v\n%s", module, err, out.String()) + } + all := languageHeader.ReplaceAllString(out.String(), "") + + // The whole output must be MDL a script can hold: read-only reports + // and failures are comments. + if _, errs := visitor.Build(all); tc.parses && len(errs) > 0 { + t.Fatalf("describe module %s with all does not parse: %v", module, errs[0]) + } + + ctx := exec.newExecContext(context.Background()) + units, err := ctx.Backend.ListDocumentUnits() + if err != nil { + t.Fatal(err) + } + mods, _ := ctx.Backend.ListModules() + var modID model.ID + for _, m := range mods { + if m.Name == module { + modID = m.ID + } + } + containers := getModuleContainers(ctx, modID) + + total, described := 0, 0 + var missing []string + for _, u := range units { + if !containers[u.ContainerID] || describedWithOwner[u.Type] { + continue + } + total++ + qn := module + "." + u.Name + out.Reset() + ownErr := exec.Execute(&ast.DescribeStmt{ObjectType: ast.DescribeAuto, Name: ast.QualifiedName{Module: module, Name: u.Name}}) + if strings.Contains(all, "-- not describable: "+u.Kind+" "+qn+"\n") { + // Only honest when describe really cannot answer for it. + if ownErr == nil { + missing = append(missing, u.Type+" "+qn+" (dumped as not describable, but its own describe works)") + } + continue + } + if ownErr != nil { + missing = append(missing, u.Type+" "+qn+" (and its own describe fails: "+ownErr.Error()+")") + continue + } + one := catalogProgress.ReplaceAllString(out.String(), "") + one = strings.TrimSpace(languageHeader.ReplaceAllString(one, "")) + if one == "" || !(strings.Contains(all, one) || strings.Contains(all, commentOutReport(one))) { + missing = append(missing, u.Type+" "+qn) + continue + } + described++ + } + if total == 0 { + t.Fatalf("module %s has no documents in the fixture; the test proves nothing", module) + } + if len(missing) > 0 { + t.Errorf("describe module %s with all accounts for %d of %d documents; missing %d:\n %s", + module, total-len(missing), total, len(missing), strings.Join(missing, "\n ")) + } + t.Logf("%s: %d documents, %d described", module, total, described) + }) + } +} + +// openFixtureCopy opens a private copy of a testdata project fixture. +func openFixtureCopy(t *testing.T, fixture, mpr string) (*Executor, *bytes.Buffer) { + t.Helper() + src := filepath.Join("..", "..", "testdata", fixture) + if _, err := os.Stat(filepath.Join(src, mpr)); err != nil { + t.Skipf("fixture %s not found: %v", fixture, err) + } + dir := t.TempDir() + if err := copyPedAppFile(filepath.Join(src, mpr), filepath.Join(dir, mpr)); err != nil { + t.Fatal(err) + } + if err := copyPedAppTree(filepath.Join(src, "mprcontents"), filepath.Join(dir, "mprcontents")); err != nil { + t.Fatal(err) + } + out := &bytes.Buffer{} + exec := New(out) + exec.SetBackendFactory(func() backend.FullBackend { return modelsdkbackend.New() }) + if err := exec.Execute(&ast.ConnectStmt{Path: filepath.Join(dir, mpr)}); err != nil { + t.Fatalf("connect: %v", err) + } + t.Cleanup(func() { _ = exec.Execute(&ast.DisconnectStmt{}) }) + return exec, out +} + +// Every document kind bare `describe M.N` can resolve is one the module dump +// emits — the two tables are what drifted apart in mendixlabs/mxcli#1421 (the +// dump knew 13 document types, describe 30-odd). The domain model's members +// are emitted from the domain model, not the registry. +func TestModuleDocKindsCoverDescribableTypes(t *testing.T) { + notDocuments := map[ast.DescribeObjectType]bool{ + ast.DescribeModule: true, // the statement itself + ast.DescribeEntity: true, // domain-model members, + ast.DescribeExternalEntity: true, // emitted by entity, + ast.DescribeAssociation: true, // and association + } + inRegistry := map[ast.DescribeObjectType]int{} + for _, k := range append(append([]moduleDocKind{}, moduleDocKindsBeforeDomainModel...), moduleDocKindsAfterDomainModel...) { + inRegistry[k.kind]++ + } + for objectType, kind := range objectTypeToDescribeKind { + if notDocuments[kind] { + continue + } + if inRegistry[kind] == 0 { + t.Errorf("%s (%s) is describable but describe module ... with all never emits it; add it to the module document registry", + objectType, describeObjectTypeLabel(kind)) + } + } + for kind, n := range inRegistry { + if n > 1 { + t.Errorf("%s is listed %d times in the module document registry", describeObjectTypeLabel(kind), n) + } + } +} + +// A document that fails to describe is reported where it would have been, as +// a comment, and fails the statement once the rest of the module is written — +// it used to be skipped by an `if err == nil` with exit 0 (mendixlabs/mxcli#1421). +// The enumeration that describes fine is the control. +func TestDescribeModuleWithAll_ReportsDescribeFailure(t *testing.T) { + mod := mkModule("Sales") + enum := mkEnumeration(mod.ID, "Status", "Open", "Closed") + for _, broken := range []bool{false, true} { + mb := &mock.MockBackend{ + IsConnectedFunc: func() bool { return true }, + ListModulesFunc: func() ([]*model.Module, error) { return []*model.Module{mod}, nil }, + GetDomainModelFunc: func(model.ID) (*domainmodel.DomainModel, error) { + return &domainmodel.DomainModel{ContainerID: mod.ID}, nil + }, + GetModuleSecurityFunc: func(model.ID) (*security.ModuleSecurity, error) { return nil, nil }, + ListEnumerationsFunc: func() ([]*model.Enumeration, error) { return []*model.Enumeration{enum}, nil }, + ListMicroflowsFunc: func() ([]*microflows.Microflow, error) { + if broken { + return nil, errors.New("unit 42 does not decode") + } + return nil, nil + }, + // The mock refuses a list it was not given; these return nothing. + ListMessageDefinitionCollectionsFunc: func() ([]*model.MessageDefinitionCollection, error) { return nil, nil }, + ListRulesFunc: func() ([]*microflows.Rule, error) { return nil, nil }, + ListNanoflowsFunc: func() ([]*microflows.Nanoflow, error) { return nil, nil }, + ListMenuDocumentsFunc: func() ([]*types.MenuDocument, error) { return nil, nil }, + ListAgentEditorModelsFunc: func() ([]*agenteditor.Model, error) { return nil, nil }, + ListAgentEditorKnowledgeBasesFunc: func() ([]*agenteditor.KnowledgeBase, error) { return nil, nil }, + ListAgentEditorConsumedMCPServicesFunc: func() ([]*agenteditor.ConsumedMCPService, error) { return nil, nil }, + ListAgentEditorAgentsFunc: func() ([]*agenteditor.Agent, error) { return nil, nil }, + } + ctx, buf := newMockCtx(t, withBackend(mb), withHierarchy(mkHierarchy(mod))) + err := describeModule(ctx, "Sales", true) + got := buf.String() + if !strings.Contains(got, "create or modify enumeration Sales.Status") { + t.Fatalf("broken=%v: the enumeration is missing:\n%s", broken, got) + } + failed := strings.Contains(got, "-- describe failed: listing microflow documents: unit 42 does not decode") + if broken != failed { + t.Errorf("broken=%v: describe-failed line present = %v:\n%s", broken, failed, got) + } + if broken != (err != nil) { + t.Errorf("broken=%v: describe module returned %v\n%s", broken, err, got) + } + if _, errs := visitor.Build(got); len(errs) > 0 { + t.Errorf("broken=%v: output does not parse: %v\n%s", broken, errs[0], got) + } + } +} diff --git a/mdl/executor/describe_module_all.go b/mdl/executor/describe_module_all.go new file mode 100644 index 0000000000..55abd89ed6 --- /dev/null +++ b/mdl/executor/describe_module_all.go @@ -0,0 +1,351 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "bytes" + "fmt" + "strings" + + "github.com/mendixlabs/mxcli/mdl/ast" + mdlerrors "github.com/mendixlabs/mxcli/mdl/errors" + "github.com/mendixlabs/mxcli/mdl/types" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/agenteditor" + "github.com/mendixlabs/mxcli/sdk/microflows" + "github.com/mendixlabs/mxcli/sdk/pages" + "github.com/mendixlabs/mxcli/sdk/workflows" +) + +// moduleDoc is one top-level document a module-wide describe emits. +type moduleDoc struct { + id, containerID model.ID + name string +} + +// moduleDocKind is one document type `describe module M with all` emits: the +// DESCRIBE kind that renders a document of it, and how to list them. +type moduleDocKind struct { + kind ast.DescribeObjectType + list func(ctx *ExecContext) ([]moduleDoc, error) +} + +// docs adapts a backend list result to moduleDocs. +func docs[T any](items []T, err error, f func(T) moduleDoc) ([]moduleDoc, error) { + if err != nil { + return nil, err + } + out := make([]moduleDoc, 0, len(items)) + for _, it := range items { + out = append(out, f(it)) + } + return out, nil +} + +// moduleDocKindsBeforeDomainModel and moduleDocKindsAfterDomainModel are the +// one registry of document types `describe module M with all` emits, in the +// order it emits them. The domain model's entities and associations are emitted +// between the two lists (they are members of one unit, not documents, and are +// ordered by generalization). +// +// It replaced a hand-written section per type that covered 13 of them, so a +// module's nanoflows, JavaScript actions, JSON structures, mappings, image +// collections, workflows, REST services, task queues and more were silently +// absent from the dump (mendixlabs/mxcli#1421). Two guards keep it whole: +// TestModuleDocKindsCoverDescribableTypes holds it against the describe +// auto-detection table, and moduleDescribeRun.reportUnemitted names every document unit +// in the module that no entry emitted, so a type missing here is visible in the +// output rather than gone. +// +// The order is dependency-first, so the dump re-executes top to bottom with as +// few forward references as the model allows: what is referenced (enumerations, +// constants, entities, structures, connections, queues, actions) before what +// references it (mappings, flows, pages, services that publish them). Flows and +// pages reference each other, so no order is cycle-free; microflows stay before +// pages as they always were. +var moduleDocKindsBeforeDomainModel = []moduleDocKind{ + {ast.DescribeEnumeration, func(ctx *ExecContext) ([]moduleDoc, error) { + l, err := ctx.Backend.ListEnumerations() + return docs(l, err, func(x *model.Enumeration) moduleDoc { return moduleDoc{x.ID, x.ContainerID, x.Name} }) + }}, + {ast.DescribeConstant, func(ctx *ExecContext) ([]moduleDoc, error) { + l, err := ctx.Backend.ListConstants() + return docs(l, err, func(x *model.Constant) moduleDoc { return moduleDoc{x.ID, x.ContainerID, x.Name} }) + }}, + // A consumed OData service precedes the domain model: its external + // entities name it, and it names only constants (its URL). + {ast.DescribeODataClient, func(ctx *ExecContext) ([]moduleDoc, error) { + l, err := ctx.Backend.ListConsumedODataServices() + return docs(l, err, func(x *model.ConsumedODataService) moduleDoc { return moduleDoc{x.ID, x.ContainerID, x.Name} }) + }}, +} + +var moduleDocKindsAfterDomainModel = []moduleDocKind{ + // Leaves: referenced by attributes, widgets and flows, reference nothing. + {ast.DescribeRegularExpression, func(ctx *ExecContext) ([]moduleDoc, error) { + l, err := ctx.Backend.ListRegularExpressions() + return docs(l, err, func(x *model.RegularExpression) moduleDoc { return moduleDoc{x.ID, x.ContainerID, x.Name} }) + }}, + {ast.DescribeImageCollection, func(ctx *ExecContext) ([]moduleDoc, error) { + l, err := ctx.Backend.ListImageCollections() + return docs(l, err, func(x *types.ImageCollection) moduleDoc { return moduleDoc{x.ID, x.ContainerID, x.Name} }) + }}, + {ast.DescribeIconCollection, func(ctx *ExecContext) ([]moduleDoc, error) { + l, err := ctx.Backend.ListIconCollections() + return docs(l, err, func(x *types.IconCollection) moduleDoc { return moduleDoc{x.ID, x.ContainerID, x.Name} }) + }}, + + // Structures, then the mappings built on them and on the entities. + {ast.DescribeJsonStructure, func(ctx *ExecContext) ([]moduleDoc, error) { + l, err := ctx.Backend.ListJsonStructures() + return docs(l, err, func(x *types.JsonStructure) moduleDoc { return moduleDoc{x.ID, x.ContainerID, x.Name} }) + }}, + {ast.DescribeMessageDefinitionCollection, func(ctx *ExecContext) ([]moduleDoc, error) { + l, err := ctx.Backend.ListMessageDefinitionCollections() + return docs(l, err, func(x *model.MessageDefinitionCollection) moduleDoc { + return moduleDoc{x.ID, x.ContainerID, x.Name} + }) + }}, + {ast.DescribeMessageDefinition, func(ctx *ExecContext) ([]moduleDoc, error) { + l, err := ctx.Backend.ListMessageDefinitionDocuments() + return docs(l, err, func(x *model.MessageDefinitionDocument) moduleDoc { + return moduleDoc{x.ID, x.ContainerID, x.Name} + }) + }}, + {ast.DescribeImportMapping, func(ctx *ExecContext) ([]moduleDoc, error) { + l, err := ctx.Backend.ListImportMappings() + return docs(l, err, func(x *model.ImportMapping) moduleDoc { return moduleDoc{x.ID, x.ContainerID, x.Name} }) + }}, + {ast.DescribeExportMapping, func(ctx *ExecContext) ([]moduleDoc, error) { + l, err := ctx.Backend.ListExportMappings() + return docs(l, err, func(x *model.ExportMapping) moduleDoc { return moduleDoc{x.ID, x.ContainerID, x.Name} }) + }}, + + // Connections and consumed services: what flows call. + {ast.DescribeDatabaseConnection, func(ctx *ExecContext) ([]moduleDoc, error) { + l, err := ctx.Backend.ListDatabaseConnections() + return docs(l, err, func(x *model.DatabaseConnection) moduleDoc { return moduleDoc{x.ID, x.ContainerID, x.Name} }) + }}, + {ast.DescribeRestClient, func(ctx *ExecContext) ([]moduleDoc, error) { + l, err := ctx.Backend.ListConsumedRestServices() + return docs(l, err, func(x *model.ConsumedRestService) moduleDoc { return moduleDoc{x.ID, x.ContainerID, x.Name} }) + }}, + {ast.DescribeBusinessEventService, func(ctx *ExecContext) ([]moduleDoc, error) { + l, err := ctx.Backend.ListBusinessEventServices() + return docs(l, err, func(x *model.BusinessEventService) moduleDoc { return moduleDoc{x.ID, x.ContainerID, x.Name} }) + }}, + {ast.DescribeDataTransformer, func(ctx *ExecContext) ([]moduleDoc, error) { + l, err := ctx.Backend.ListDataTransformers() + return docs(l, err, func(x *model.DataTransformer) moduleDoc { return moduleDoc{x.ID, x.ContainerID, x.Name} }) + }}, + // A task queue is resolved eagerly by a microflow's run-in-queue setting + // (MDL-ORDER01), so it precedes the flows. + {ast.DescribeQueue, func(ctx *ExecContext) ([]moduleDoc, error) { + l, err := ctx.Backend.ListQueues() + return docs(l, err, func(x *types.Queue) moduleDoc { return moduleDoc{x.ID, x.ContainerID, x.Name} }) + }}, + + // Actions, then the flows that call them. Rules precede microflows, whose + // decisions call them. + {ast.DescribeJavaAction, func(ctx *ExecContext) ([]moduleDoc, error) { + l, err := ctx.Backend.ListJavaActions() + return docs(l, err, func(x *types.JavaAction) moduleDoc { return moduleDoc{x.ID, x.ContainerID, x.Name} }) + }}, + {ast.DescribeJavaScriptAction, func(ctx *ExecContext) ([]moduleDoc, error) { + l, err := ctx.Backend.ListJavaScriptActions() + return docs(l, err, func(x *types.JavaScriptAction) moduleDoc { return moduleDoc{x.ID, x.ContainerID, x.Name} }) + }}, + {ast.DescribeRule, func(ctx *ExecContext) ([]moduleDoc, error) { + l, err := ctx.Backend.ListRules() + return docs(l, err, func(x *microflows.Rule) moduleDoc { return moduleDoc{x.ID, x.ContainerID, x.Name} }) + }}, + {ast.DescribeMicroflow, func(ctx *ExecContext) ([]moduleDoc, error) { + l, err := ctx.Backend.ListMicroflows() + return docs(l, err, func(x *microflows.Microflow) moduleDoc { return moduleDoc{x.ID, x.ContainerID, x.Name} }) + }}, + {ast.DescribeNanoflow, func(ctx *ExecContext) ([]moduleDoc, error) { + l, err := ctx.Backend.ListNanoflows() + return docs(l, err, func(x *microflows.Nanoflow) moduleDoc { return moduleDoc{x.ID, x.ContainerID, x.Name} }) + }}, + + // Page structure: layouts, then snippets and building blocks, then pages + // that place them, then the menus that link to pages. + {ast.DescribeLayout, func(ctx *ExecContext) ([]moduleDoc, error) { + l, err := ctx.Backend.ListLayouts() + return docs(l, err, func(x *pages.Layout) moduleDoc { return moduleDoc{x.ID, x.ContainerID, x.Name} }) + }}, + {ast.DescribeSnippet, func(ctx *ExecContext) ([]moduleDoc, error) { + l, err := ctx.Backend.ListSnippets() + return docs(l, err, func(x *pages.Snippet) moduleDoc { return moduleDoc{x.ID, x.ContainerID, x.Name} }) + }}, + {ast.DescribeBuildingBlock, func(ctx *ExecContext) ([]moduleDoc, error) { + l, err := ctx.Backend.ListBuildingBlocks() + return docs(l, err, func(x *pages.BuildingBlock) moduleDoc { return moduleDoc{x.ID, x.ContainerID, x.Name} }) + }}, + {ast.DescribePage, func(ctx *ExecContext) ([]moduleDoc, error) { + l, err := ctx.Backend.ListPages() + return docs(l, err, func(x *pages.Page) moduleDoc { return moduleDoc{x.ID, x.ContainerID, x.Name} }) + }}, + {ast.DescribeMenu, func(ctx *ExecContext) ([]moduleDoc, error) { + l, err := ctx.Backend.ListMenuDocuments() + return docs(l, err, func(x *types.MenuDocument) moduleDoc { return moduleDoc{x.ID, x.ContainerID, x.Name} }) + }}, + + // What references flows and pages: workflows, published services, schedules. + {ast.DescribeWorkflow, func(ctx *ExecContext) ([]moduleDoc, error) { + l, err := ctx.Backend.ListWorkflows() + return docs(l, err, func(x *workflows.Workflow) moduleDoc { return moduleDoc{x.ID, x.ContainerID, x.Name} }) + }}, + {ast.DescribeODataService, func(ctx *ExecContext) ([]moduleDoc, error) { + l, err := ctx.Backend.ListPublishedODataServices() + return docs(l, err, func(x *model.PublishedODataService) moduleDoc { return moduleDoc{x.ID, x.ContainerID, x.Name} }) + }}, + {ast.DescribePublishedRestService, func(ctx *ExecContext) ([]moduleDoc, error) { + l, err := ctx.Backend.ListPublishedRestServices() + return docs(l, err, func(x *model.PublishedRestService) moduleDoc { return moduleDoc{x.ID, x.ContainerID, x.Name} }) + }}, + {ast.DescribeScheduledEvent, func(ctx *ExecContext) ([]moduleDoc, error) { + l, err := ctx.Backend.ListScheduledEvents() + return docs(l, err, func(x *model.ScheduledEvent) moduleDoc { return moduleDoc{x.ID, x.ContainerID, x.Name} }) + }}, + + // Agent-editor documents: Model / KnowledgeBase / ConsumedMCPService are + // leaves, an Agent references them through its TOOL / KNOWLEDGE BASE / MCP + // SERVICE blocks. + {ast.DescribeModel, func(ctx *ExecContext) ([]moduleDoc, error) { + l, err := ctx.Backend.ListAgentEditorModels() + return docs(l, err, func(x *agenteditor.Model) moduleDoc { return moduleDoc{x.ID, x.ContainerID, x.Name} }) + }}, + {ast.DescribeKnowledgeBase, func(ctx *ExecContext) ([]moduleDoc, error) { + l, err := ctx.Backend.ListAgentEditorKnowledgeBases() + return docs(l, err, func(x *agenteditor.KnowledgeBase) moduleDoc { return moduleDoc{x.ID, x.ContainerID, x.Name} }) + }}, + {ast.DescribeConsumedMCPService, func(ctx *ExecContext) ([]moduleDoc, error) { + l, err := ctx.Backend.ListAgentEditorConsumedMCPServices() + return docs(l, err, func(x *agenteditor.ConsumedMCPService) moduleDoc { return moduleDoc{x.ID, x.ContainerID, x.Name} }) + }}, + {ast.DescribeAgent, func(ctx *ExecContext) ([]moduleDoc, error) { + l, err := ctx.Backend.ListAgentEditorAgents() + return docs(l, err, func(x *agenteditor.Agent) moduleDoc { return moduleDoc{x.ID, x.ContainerID, x.Name} }) + }}, +} + +// moduleDescribeRun accumulates what a module-wide describe emitted and what +// failed, so a failure is reported where it happened and fails the statement +// once everything else has been written. +type moduleDescribeRun struct { + ctx *ExecContext + module string + containers map[model.ID]bool + emitted map[model.ID]bool + failures int +} + +// fail reports one describe failure in the output, as a comment so the dump +// still parses, in place of the document it could not describe. +func (r *moduleDescribeRun) fail(what string, err error) { + r.failures++ + msg := strings.ReplaceAll(strings.TrimSpace(err.Error()), "\n", " ") + fmt.Fprintf(r.ctx.Output, "-- describe failed: %s: %s\n\n", what, msg) +} + +// emitKinds describes every document of the given kinds that lives in the +// module, in registry order. +func (r *moduleDescribeRun) emitKinds(kinds []moduleDocKind) { + for _, k := range kinds { + label := describeObjectTypeLabel(k.kind) + list, err := k.list(r.ctx) + if err != nil { + r.fail("listing "+label+" documents", err) + continue + } + for _, d := range list { + if !r.containers[d.containerID] { + continue + } + r.emitted[d.id] = true + qn := ast.QualifiedName{Module: r.module, Name: d.name} + if err := r.describe(k.kind, qn); err != nil { + r.fail(label+" "+qn.String(), err) + continue + } + fmt.Fprintln(r.ctx.Output) + } + } +} + +// readOnlyDescribeKinds are the document types whose describe is a report +// (a widget tree, an icon table) rather than a statement MDL can execute — +// building blocks and icon collections cannot be created from MDL. In a module +// dump their report is emitted commented out, so the document is accounted for +// and the dump still parses as a script. +var readOnlyDescribeKinds = map[ast.DescribeObjectType]bool{ + ast.DescribeBuildingBlock: true, + ast.DescribeIconCollection: true, +} + +// describe writes one document's describe output, commented out when it is a +// read-only report. +func (r *moduleDescribeRun) describe(kind ast.DescribeObjectType, qn ast.QualifiedName) error { + stmt := &ast.DescribeStmt{ObjectType: kind, Name: qn} + if !readOnlyDescribeKinds[kind] { + return describeByKind(r.ctx, stmt) + } + out := r.ctx.Output + var buf bytes.Buffer + r.ctx.Output = &buf + err := describeByKind(r.ctx, stmt) + r.ctx.Output = out + if err != nil { + return err + } + fmt.Fprint(out, commentOutReport(buf.String())) + return nil +} + +// commentOutReport prefixes every line of a report that is not already a +// comment with "-- ". +func commentOutReport(s string) string { + lines := strings.Split(strings.TrimRight(s, "\n"), "\n") + for i, ln := range lines { + if !strings.HasPrefix(ln, "--") { + lines[i] = "-- " + ln + } + } + return strings.Join(lines, "\n") + "\n" +} + +// describedWithOwner are document unit types that are not described on their +// own because another document's describe already carries them. +var describedWithOwner = map[string]bool{ + // A view entity's OQL source; `create view entity` prints and writes it. + "DomainModels$ViewEntitySourceDocument": true, +} + +// reportUnemitted names every document unit in the module that no registry +// entry emitted. The enumeration is type-blind, so a document type the registry +// does not know about (a page template, which has no describe handler, or a +// type added to Mendix later) is listed instead of silently absent. +func (r *moduleDescribeRun) reportUnemitted() { + units, err := r.ctx.Backend.ListDocumentUnits() + if err != nil { + r.fail("listing the module's documents", err) + return + } + for _, u := range units { + if !r.containers[u.ContainerID] || r.emitted[u.ID] || describedWithOwner[u.Type] { + continue + } + fmt.Fprintf(r.ctx.Output, "-- not describable: %s %s.%s\n", u.Kind, r.module, u.Name) + } +} + +// err is the statement's result: a describe failure fails it, after the rest of +// the module has been written (as the output line limit does, #1425). +func (r *moduleDescribeRun) err() error { + if r.failures == 0 { + return nil + } + return mdlerrors.NewValidationf("describe module %s with all: %d document(s) or document types could not be described; "+ + "see the '-- describe failed:' lines in the output", r.module, r.failures) +} diff --git a/mdl/executor/executor_query.go b/mdl/executor/executor_query.go index f7b8ab398e..b121a45e5d 100644 --- a/mdl/executor/executor_query.go +++ b/mdl/executor/executor_query.go @@ -223,118 +223,7 @@ func execDescribe(ctx *ExecContext, s *ast.DescribeStmt) error { fmt.Fprintf(ctx.Output, "-- %s definition (not executable)\n", report) } - describe := func() error { - switch s.ObjectType { - case ast.DescribeEnumeration: - return describeEnumeration(ctx, s.Name) - case ast.DescribeEntity: - return describeEntity(ctx, s.Name) - case ast.DescribeAssociation: - return describeAssociation(ctx, s.Name) - case ast.DescribeMicroflow: - return describeMicroflowMode(ctx, s.Name, describeMicroflowOptions{Normalized: s.Normalized, Handles: s.WithHandles}) - case ast.DescribeNanoflow: - return describeNanoflow(ctx, s.Name) - case ast.DescribeRule: - return describeRule(ctx, s.Name) - case ast.DescribeModule: - return describeModule(ctx, s.Name.Module, s.WithAll) - case ast.DescribePage: - return describePage(ctx, s.Name) - case ast.DescribeSnippet: - return describeSnippet(ctx, s.Name) - case ast.DescribeBuildingBlock: - return describeBuildingBlock(ctx, s.Name) - case ast.DescribeQueue: - // Queues and scheduled events carry their own statement types rather - // than a DescribeStmt kind, so bare `DESCRIBE Module.Name` reaches them - // by synthesizing one. Without this they are indexed in the catalog's - // objects view but unreachable without naming the type — the state - // BUILDING_BLOCK and ICON_COLLECTION were in when 43 of 251 documents - // in a marketplace project could not be described (see describe_auto.go). - return execDescribeQueue(ctx, &ast.DescribeQueueStmt{Name: s.Name}) - case ast.DescribeScheduledEvent: - return execDescribeScheduledEvent(ctx, &ast.DescribeScheduledEventStmt{Name: s.Name}) - case ast.DescribeRegularExpression: - return execDescribeRegularExpression(ctx, &ast.DescribeRegularExpressionStmt{Name: s.Name}) - case ast.DescribeLayout: - return describeLayout(ctx, s.Name) - case ast.DescribeConstant: - return describeConstant(ctx, s.Name) - case ast.DescribeJavaAction: - return describeJavaAction(ctx, s.Name) - case ast.DescribeJavaScriptAction: - return describeJavaScriptAction(ctx, s.Name) - case ast.DescribeModuleRole: - return describeModuleRole(ctx, s.Name) - case ast.DescribeUserRole: - return describeUserRole(ctx, s.Name) - case ast.DescribeDemoUser: - return describeDemoUser(ctx, s.Name.Name) - case ast.DescribeODataClient: - return describeODataClient(ctx, s.Name) - case ast.DescribeODataService: - return describeODataService(ctx, s.Name) - case ast.DescribeExternalEntity: - return describeExternalEntity(ctx, s.Name) - case ast.DescribeNavigation: - return describeNavigation(ctx, s.Name) - case ast.DescribeWidget: - return describeWidgetStmt(ctx, s.Name.Name) - case ast.DescribeWorkflow: - return describeWorkflow(ctx, s.Name) - case ast.DescribeBusinessEventService: - return describeBusinessEventService(ctx, s.Name) - case ast.DescribeDatabaseConnection: - return describeDatabaseConnection(ctx, s.Name) - case ast.DescribeSettings: - return describeSettings(ctx, s.Qualifier) - case ast.DescribeFragment: - return describeFragment(ctx, s.Name) - case ast.DescribeImageCollection: - return describeImageCollection(ctx, s.Name) - case ast.DescribeIconCollection: - return describeIconCollection(ctx, s.Name) - case ast.DescribeGlyph: - return describeGlyph(ctx, s.Qualifier) - case ast.DescribeModel: - return describeAgentEditorModel(ctx, s.Name) - case ast.DescribeAgent: - return describeAgentEditorAgent(ctx, s.Name) - case ast.DescribeKnowledgeBase: - return describeAgentEditorKnowledgeBase(ctx, s.Name) - case ast.DescribeConsumedMCPService: - return describeAgentEditorConsumedMCPService(ctx, s.Name) - case ast.DescribeRestClient: - return describeRestClient(ctx, s.Name) - case ast.DescribePublishedRestService: - return describePublishedRestService(ctx, s.Name) - case ast.DescribeDataTransformer: - return describeDataTransformer(ctx, s.Name) - case ast.DescribeContractEntity: - return describeContractEntity(ctx, s.Name, s.Format) - case ast.DescribeContractAction: - return describeContractAction(ctx, s.Name, s.Format) - case ast.DescribeContractMessage: - return describeContractMessage(ctx, s.Name) - case ast.DescribeJsonStructure: - return describeJsonStructure(ctx, s.Name) - case ast.DescribeMessageDefinitionCollection: - return execDescribeMessageDefinitionCollection(ctx, s.Name) - case ast.DescribeMessageDefinition: - return execDescribeMessageDefinition(ctx, s.Name) - case ast.DescribeImportMapping: - return describeImportMapping(ctx, s.Name) - case ast.DescribeExportMapping: - return describeExportMapping(ctx, s.Name) - case ast.DescribeMenu: - return describeMenu(ctx, s.Name) - case ast.DescribeJarDependency: - return execDescribeJarDependency(ctx, s.Name.String(), s.Qualifier) - default: - return mdlerrors.NewUnsupported("unknown describe object type") - } - } + describe := func() error { return describeByKind(ctx, s) } return writeDescribeJSONAs(ctx, name, objectType, !isReport, func() error { if isReport { return describe() @@ -343,6 +232,123 @@ func execDescribe(ctx *ExecContext, s *ast.DescribeStmt) error { }) } +// describeByKind runs the describe handler for one typed DESCRIBE statement: +// the dispatch execDescribe wraps in its header and JSON envelope, and that +// `describe module ... with all` reuses per document, so a module dump is the +// concatenation of exactly what each document's own describe prints. +func describeByKind(ctx *ExecContext, s *ast.DescribeStmt) error { + switch s.ObjectType { + case ast.DescribeEnumeration: + return describeEnumeration(ctx, s.Name) + case ast.DescribeEntity: + return describeEntity(ctx, s.Name) + case ast.DescribeAssociation: + return describeAssociation(ctx, s.Name) + case ast.DescribeMicroflow: + return describeMicroflowMode(ctx, s.Name, describeMicroflowOptions{Normalized: s.Normalized, Handles: s.WithHandles}) + case ast.DescribeNanoflow: + return describeNanoflow(ctx, s.Name) + case ast.DescribeRule: + return describeRule(ctx, s.Name) + case ast.DescribeModule: + return describeModule(ctx, s.Name.Module, s.WithAll) + case ast.DescribePage: + return describePage(ctx, s.Name) + case ast.DescribeSnippet: + return describeSnippet(ctx, s.Name) + case ast.DescribeBuildingBlock: + return describeBuildingBlock(ctx, s.Name) + case ast.DescribeQueue: + // Queues and scheduled events carry their own statement types rather + // than a DescribeStmt kind, so bare `DESCRIBE Module.Name` reaches them + // by synthesizing one. Without this they are indexed in the catalog's + // objects view but unreachable without naming the type — the state + // BUILDING_BLOCK and ICON_COLLECTION were in when 43 of 251 documents + // in a marketplace project could not be described (see describe_auto.go). + return execDescribeQueue(ctx, &ast.DescribeQueueStmt{Name: s.Name}) + case ast.DescribeScheduledEvent: + return execDescribeScheduledEvent(ctx, &ast.DescribeScheduledEventStmt{Name: s.Name}) + case ast.DescribeRegularExpression: + return execDescribeRegularExpression(ctx, &ast.DescribeRegularExpressionStmt{Name: s.Name}) + case ast.DescribeLayout: + return describeLayout(ctx, s.Name) + case ast.DescribeConstant: + return describeConstant(ctx, s.Name) + case ast.DescribeJavaAction: + return describeJavaAction(ctx, s.Name) + case ast.DescribeJavaScriptAction: + return describeJavaScriptAction(ctx, s.Name) + case ast.DescribeModuleRole: + return describeModuleRole(ctx, s.Name) + case ast.DescribeUserRole: + return describeUserRole(ctx, s.Name) + case ast.DescribeDemoUser: + return describeDemoUser(ctx, s.Name.Name) + case ast.DescribeODataClient: + return describeODataClient(ctx, s.Name) + case ast.DescribeODataService: + return describeODataService(ctx, s.Name) + case ast.DescribeExternalEntity: + return describeExternalEntity(ctx, s.Name) + case ast.DescribeNavigation: + return describeNavigation(ctx, s.Name) + case ast.DescribeWidget: + return describeWidgetStmt(ctx, s.Name.Name) + case ast.DescribeWorkflow: + return describeWorkflow(ctx, s.Name) + case ast.DescribeBusinessEventService: + return describeBusinessEventService(ctx, s.Name) + case ast.DescribeDatabaseConnection: + return describeDatabaseConnection(ctx, s.Name) + case ast.DescribeSettings: + return describeSettings(ctx, s.Qualifier) + case ast.DescribeFragment: + return describeFragment(ctx, s.Name) + case ast.DescribeImageCollection: + return describeImageCollection(ctx, s.Name) + case ast.DescribeIconCollection: + return describeIconCollection(ctx, s.Name) + case ast.DescribeGlyph: + return describeGlyph(ctx, s.Qualifier) + case ast.DescribeModel: + return describeAgentEditorModel(ctx, s.Name) + case ast.DescribeAgent: + return describeAgentEditorAgent(ctx, s.Name) + case ast.DescribeKnowledgeBase: + return describeAgentEditorKnowledgeBase(ctx, s.Name) + case ast.DescribeConsumedMCPService: + return describeAgentEditorConsumedMCPService(ctx, s.Name) + case ast.DescribeRestClient: + return describeRestClient(ctx, s.Name) + case ast.DescribePublishedRestService: + return describePublishedRestService(ctx, s.Name) + case ast.DescribeDataTransformer: + return describeDataTransformer(ctx, s.Name) + case ast.DescribeContractEntity: + return describeContractEntity(ctx, s.Name, s.Format) + case ast.DescribeContractAction: + return describeContractAction(ctx, s.Name, s.Format) + case ast.DescribeContractMessage: + return describeContractMessage(ctx, s.Name) + case ast.DescribeJsonStructure: + return describeJsonStructure(ctx, s.Name) + case ast.DescribeMessageDefinitionCollection: + return execDescribeMessageDefinitionCollection(ctx, s.Name) + case ast.DescribeMessageDefinition: + return execDescribeMessageDefinition(ctx, s.Name) + case ast.DescribeImportMapping: + return describeImportMapping(ctx, s.Name) + case ast.DescribeExportMapping: + return describeExportMapping(ctx, s.Name) + case ast.DescribeMenu: + return describeMenu(ctx, s.Name) + case ast.DescribeJarDependency: + return execDescribeJarDependency(ctx, s.Name.String(), s.Qualifier) + default: + return mdlerrors.NewUnsupported("unknown describe object type") + } +} + // definitionReportKind names the kind of definition a describe reports on, when // its answer is a report rather than runnable MDL: a widget type, a glyph, an // OData or AsyncAPI contract's entity, action or message (unless asked for as From 8f8c75564e9f45b0e5c6369194af582a65c6821a Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 13 Oct 2026 07:08:08 +0000 Subject: [PATCH 10/17] test,docs: keep list-of-one coverage valid under MDL-RETRIEVE03 (mendixlabs/mxcli#1422) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TestPedAppRoundTrip_RetrieveRange swapped `first` for `limit 1` under `mdl 1;` on a microflow that then reads `$CurrentUser/Name` — a member path over a list, CE0117, which MDL-RETRIEVE03 now refuses. The test now asserts that refusal (nothing written), then makes the list form valid by reading the name from `$currentUser` and rebuilding with drop + create (the change activity shares annotation n1 with the retrieve, so the splice refuses to replace it). Range storage, describe, PutGet and the headerless control are unchanged. The write-microflows skill's complete example retrieved `$Order` without a range and used it as an object; it now says `first`. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01W3x8Fyvir8kLyPgJs1BDmF --- .../skills/mendix/write-microflows/SKILL.md | 21 +++++++++++-------- mdl/roundtrip/retrieve_range_test.go | 20 ++++++++++++++++++ 2 files changed, 32 insertions(+), 9 deletions(-) diff --git a/.claude/skills/mendix/write-microflows/SKILL.md b/.claude/skills/mendix/write-microflows/SKILL.md index 3279491a77..19a4f76aba 100644 --- a/.claude/skills/mendix/write-microflows/SKILL.md +++ b/.claude/skills/mendix/write-microflows/SKILL.md @@ -20,13 +20,13 @@ description: "Microflow syntax reference in MDL — every activity type, control | 345-402 | Operators | | 403-419 | Logging | | 420-445 | Special Values | -| 446-492 | Complete Example | -| 493-549 | Calling Microflows | -| 550-576 | Page Navigation | -| 577-593 | Validation Checklist | -| 594-605 | Tips for Success | -| 606-612 | Related Documentation | -| 613-727 | Quick Reference | +| 446-495 | Complete Example | +| 496-552 | Calling Microflows | +| 553-579 | Page Navigation | +| 580-596 | Validation Checklist | +| 597-608 | Tips for Success | +| 609-615 | Related Documentation | +| 616-730 | Quick Reference | # Mendix Microflow Skill @@ -460,9 +460,12 @@ returns boolean as $success begin declare $success boolean = false; - -- Find the order (the retrieve establishes $Order — objects are never declared) + -- Find the order (the retrieve establishes $Order — objects are never declared). + -- `first` binds one object; without it $Order is a list, and `$Order/…` or + -- `change $Order` over a list fails the build (MDL-RETRIEVE03). retrieve $Order from Shop.Order - where OrderNumber = $OrderNumber; + where OrderNumber = $OrderNumber + first; -- Validate order exists if $Order = empty then diff --git a/mdl/roundtrip/retrieve_range_test.go b/mdl/roundtrip/retrieve_range_test.go index 5d21ed9984..f7673e6d2e 100644 --- a/mdl/roundtrip/retrieve_range_test.go +++ b/mdl/roundtrip/retrieve_range_test.go @@ -64,6 +64,26 @@ func TestPedAppRoundTrip_RetrieveRange(t *testing.T) { // --- The list form, under mdl 1 ---------------------------------------- listScript := "mdl 1;\n" + strings.Replace(described, "\n first;", "\n limit 1;", 1) + // The microflow reads `$CurrentUser/Name`, which over a list is CE0117: + // the bare swap is an invalid model, and MDL-RETRIEVE03 refuses it + // (mendixlabs/mxcli#1422). Assert that, then make the list form valid by + // reading the name from the session's `$currentUser` instead, leaving the + // retrieve — the element this test judges — as it is. + const listPath = "SubmitterDisplayName = $CurrentUser/Name" + if !strings.Contains(listScript, listPath) { + t.Fatalf("fixture changed: the microflow no longer reads %q:\n%s", listPath, listScript) + } + if err := h.exec(listScript); err == nil || !strings.Contains(err.Error(), "MDL-RETRIEVE03") { + t.Fatalf("exec of `limit 1` + `$CurrentUser/Name` under mdl 1: err = %v, want MDL-RETRIEVE03 (a member path over a list)", err) + } + if got := h.retrieveRanges(t, "PopulateUserAttributes"); strings.Join(got, "; ") != objectRange { + t.Fatalf("the refused list form was written anyway: range %q", got) + } + // The change activity shares Studio Pro's annotation n1 with the retrieve, + // so the splice refuses to replace it; drop and create in one script, which + // is the documented way to rebuild (the grant carries to the create). + listScript = strings.Replace(listScript, listPath, "SubmitterDisplayName = $currentUser/Name", 1) + listScript = strings.Replace(listScript, "mdl 1;\n", "mdl 1;\ndrop microflow FeedbackModule.PopulateUserAttributes;\n", 1) if err := h.exec(listScript); err != nil { t.Fatalf("exec the list form under mdl 1: %v", err) } From c1c595aa7a0a81b76bcd4152bbfbc95ffbace357 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 13 Oct 2026 07:21:01 +0000 Subject: [PATCH 11/17] docs: keep the write-microflows skill within the 700-line body limit The RETRIEVE03 example fix added three lines to a skill body that was exactly at the limit, failing TestLargeSkillsWereSplit (703 > 700). Same content, folded into the existing comment line and the where clause. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01W3x8Fyvir8kLyPgJs1BDmF --- .../skills/mendix/write-microflows/SKILL.md | 21 ++++++++----------- 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/.claude/skills/mendix/write-microflows/SKILL.md b/.claude/skills/mendix/write-microflows/SKILL.md index 19a4f76aba..e87d3e335d 100644 --- a/.claude/skills/mendix/write-microflows/SKILL.md +++ b/.claude/skills/mendix/write-microflows/SKILL.md @@ -20,13 +20,13 @@ description: "Microflow syntax reference in MDL — every activity type, control | 345-402 | Operators | | 403-419 | Logging | | 420-445 | Special Values | -| 446-495 | Complete Example | -| 496-552 | Calling Microflows | -| 553-579 | Page Navigation | -| 580-596 | Validation Checklist | -| 597-608 | Tips for Success | -| 609-615 | Related Documentation | -| 616-730 | Quick Reference | +| 446-492 | Complete Example | +| 493-549 | Calling Microflows | +| 550-576 | Page Navigation | +| 577-593 | Validation Checklist | +| 594-605 | Tips for Success | +| 606-612 | Related Documentation | +| 613-727 | Quick Reference | # Mendix Microflow Skill @@ -460,12 +460,9 @@ returns boolean as $success begin declare $success boolean = false; - -- Find the order (the retrieve establishes $Order — objects are never declared). - -- `first` binds one object; without it $Order is a list, and `$Order/…` or - -- `change $Order` over a list fails the build (MDL-RETRIEVE03). + -- Find the order: `first` binds one object (without it $Order is a list — MDL-RETRIEVE03) retrieve $Order from Shop.Order - where OrderNumber = $OrderNumber - first; + where OrderNumber = $OrderNumber first; -- Validate order exists if $Order = empty then From f296b3c05620cf27090bc89b839edaa34a5d0c8e Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 13 Oct 2026 07:21:23 +0000 Subject: [PATCH 12/17] fix: describe javascript action with type parameters emits MDL that parses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found while fixing mendixlabs/mxcli#1421 (PR ako/mxcli#1169), related to mendixlabs/mxcli#1404. Symptom: `describe javascript action` on an action with type parameters printed `NanoflowCommons.GetGuid` and `entity <>`, which do not parse ("mismatched input '<' expecting {FOLDER, '('}"), so `describe module FeedbackModule with all` did not parse either. - Drop the `Name` header: as for Java actions, a type parameter is declared by its `entity ` parameter. - Shapes MDL has no form for — a parameter typed by T with no `entity ` parameter (#1404), an entity selector bound to no type parameter (null TypeParameterPointer), or stored type parameters that are not the declared ones in order — are printed commented out under `-- NOT re-executable:`, so the script parses and re-running it leaves the action unchanged. - exec wrote `list of T` as a list of the entity `.T` (the JavaScript twin of #1183); it now uses listOfTypeParameter for parameters and returns. Ten javascript action entries are struck from each roundtrip allowlist. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01W3x8Fyvir8kLyPgJs1BDmF --- ...vascript-action-type-parameter-header.json | 1 + mdl/executor/cmd_javascript_actions.go | 84 +++++++++-- mdl/executor/cmd_javascript_actions_write.go | 11 +- ...ribe_javascript_action_type_params_test.go | 140 ++++++++++++++++++ mdl/roundtrip/allowlist_test.go | 10 -- mdl/roundtrip/testapp_allowlist_test.go | 10 -- 6 files changed, 223 insertions(+), 33 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/mdl-executor/2026-10-13-describe-javascript-action-type-parameter-header.json create mode 100644 mdl/executor/describe_javascript_action_type_params_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-10-13-describe-javascript-action-type-parameter-header.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-13-describe-javascript-action-type-parameter-header.json new file mode 100644 index 0000000000..5a2f6b8d54 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-13-describe-javascript-action-type-parameter-header.json @@ -0,0 +1 @@ +{"date": "2026-10-13", "area": "mdl/executor", "symptom": "`describe javascript action` of an action with type parameters (PedApp's NanoflowCommons GetGuid, GetStorageItemObject, …; FeedbackModule.JS_GetFeedbackStorageObject) printed `NanoflowCommons.GetGuid` after the action name and `entity <>` for an entity selector bound to no type parameter: neither parses (\"mismatched input '<' expecting {FOLDER, '('}\"), so `describe module FeedbackModule with all` did not parse either.", "cause": "describeJavaScriptAction printed the stored TypeParameters as a `` header the grammar never had — MDL declares a type parameter only through an `entity ` parameter, as for Java actions. Two stored shapes have no MDL form at all: a parameter typed by T with no `entity ` parameter (mendixlabs/mxcli#1404's Java shape), and an EntityTypeParameterType whose TypeParameterPointer is null with TypeParameters empty. Behind the parse error, exec also wrote `list of T` as a list of the entity `.T` (the JavaScript twin of Java's #1183, listOfTypeParameter was never called from the JS write path).", "file": "mdl/executor/cmd_javascript_actions.go (no header; jsTypeParameterGap comments the statement out under `-- NOT re-executable:`); mdl/executor/cmd_javascript_actions_write.go (listOfTypeParameter for parameters and return type)", "insight": "Fixing the parse error exposed the next law: GetStorageItemObjectList then parsed but broke getput/putget, which only the roundtrip harness (exec on a copy, unit diff) showed. For a shape MDL cannot express, a statement that parses into something else is worse than a parse error — `T` with no declarer re-executes as the entity `.T` and builds with CE1613 — so commenting the whole statement out keeps the script parseable and the stored action untouched. The commented-out documents pass every harness law vacuously; the gap is the grammar's, not closed.", "refs": ["mdl/executor/describe_javascript_action_type_params_test.go", "mdl/roundtrip/allowlist_test.go", "mdl/roundtrip/testapp_allowlist_test.go", "docs-wiki/bug-patterns/describe-round-trip-gaps.md"]} diff --git a/mdl/executor/cmd_javascript_actions.go b/mdl/executor/cmd_javascript_actions.go index db1f07d9c4..113b29f44b 100644 --- a/mdl/executor/cmd_javascript_actions.go +++ b/mdl/executor/cmd_javascript_actions.go @@ -12,6 +12,8 @@ import ( "github.com/mendixlabs/mxcli/mdl/ast" mdlerrors "github.com/mendixlabs/mxcli/mdl/errors" + "github.com/mendixlabs/mxcli/mdl/types" + "github.com/mendixlabs/mxcli/model" "github.com/mendixlabs/mxcli/sdk/javaactions" ) @@ -92,19 +94,11 @@ func describeJavaScriptAction(ctx *ExecContext, name ast.QualifiedName) error { sb.WriteString(" */\n") } - // Type parameters + // Type parameters have no clause of their own: as for Java actions, each is + // declared by an `entity ` parameter. The `Name` this used to print + // after the action name does not parse. sb.WriteString("create or modify javascript action ") sb.WriteString(qualifiedName) - if len(jsa.TypeParameters) > 0 { - sb.WriteString("<") - for i, tp := range jsa.TypeParameters { - if i > 0 { - sb.WriteString(", ") - } - sb.WriteString(tp.Name) - } - sb.WriteString(">") - } sb.WriteString(describeFolderClause(ctx, jsa.ContainerID)) sb.WriteString("(") @@ -190,7 +184,17 @@ func describeJavaScriptAction(ctx *ExecContext, name ast.QualifiedName) error { } sb.WriteString("\n$$;") - fmt.Fprintln(ctx.Output, sb.String()) + stmt := sb.String() + if gap := jsTypeParameterGap(jsa); gap != "" { + // A statement that parses into something else is worse than none: + // `EntityObject: T` with no `entity ` re-executes as the entity `.T` + // (mendixlabs/mxcli#1404). Commented out, the script still parses and + // re-running it leaves the stored action as it is. + fmt.Fprintf(ctx.Output, "-- NOT re-executable: %s; MDL has no form for this (mendixlabs/mxcli#1404), "+ + "so the statement is commented out and re-running this script leaves the action unchanged\n", gap) + stmt = "-- " + strings.ReplaceAll(stmt, "\n", "\n-- ") + } + fmt.Fprintln(ctx.Output, stmt) // Additional info as comments. if jsa.ActionDefaultReturnName != "" { @@ -215,6 +219,62 @@ func describeJavaScriptAction(ctx *ExecContext, name ast.QualifiedName) error { return nil } +// jsTypeParameterGap reports why the action's type parameters have no MDL form, +// or "" when they have one. MDL declares a type parameter only through an +// `entity ` parameter (the visitor collects them in parameter order), so the +// stored list must be exactly the declared names in that order, and every +// reference to a type parameter must name one of them. +func jsTypeParameterGap(jsa *types.JavaScriptAction) string { + var declared []string + seen := map[string]bool{} + for _, p := range jsa.Parameters { + e, ok := p.ParameterType.(*types.EntityTypeParameterType) + if !ok { + continue + } + if e.TypeParameterName == "" { + return fmt.Sprintf("parameter %s is an entity selector bound to no type parameter", p.Name) + } + if !seen[e.TypeParameterName] { + seen[e.TypeParameterName] = true + declared = append(declared, e.TypeParameterName) + } + } + refGap := func(what string, t any) string { + name, id := "", model.ID("") + switch v := t.(type) { + case *types.TypeParameter: + name, id = v.TypeParameter, v.TypeParameterID + case *types.ListType: + name, id = v.TypeParameter, v.TypeParameterID + } + switch { + case name == "" && id != "": + return what + " points at a type parameter that does not exist" + case name != "" && !seen[name]: + return fmt.Sprintf("%s is typed by type parameter %s, which no `entity <%s>` parameter declares", what, name, name) + } + return "" + } + for _, p := range jsa.Parameters { + if gap := refGap("parameter "+p.Name, p.ParameterType); gap != "" { + return gap + } + } + if gap := refGap("the return type", jsa.ReturnType); gap != "" { + return gap + } + stored := make([]string, 0, len(jsa.TypeParameters)) + for _, tp := range jsa.TypeParameters { + stored = append(stored, tp.Name) + } + if strings.Join(stored, ",") != strings.Join(declared, ",") { + return fmt.Sprintf("type parameters [%s] are not the ones its `entity ` parameters declare, in that order ([%s])", + strings.Join(stored, ", "), strings.Join(declared, ", ")) + } + return "" +} + // readJavaScriptActionSource reads the JavaScript source file and extracts user code and extra code. func readJavaScriptActionSource(mprPath, moduleName, actionName string) (userCode, extraCode string) { if mprPath == "" { diff --git a/mdl/executor/cmd_javascript_actions_write.go b/mdl/executor/cmd_javascript_actions_write.go index 7789cbd62c..6de9667fe4 100644 --- a/mdl/executor/cmd_javascript_actions_write.go +++ b/mdl/executor/cmd_javascript_actions_write.go @@ -163,7 +163,12 @@ func execCreateJavaScriptAction(ctx *ExecContext, s *ast.CreateJavaScriptActionS TypeParameter: tpName, } default: - p.ParameterType = astDataTypeToJavaActionParamType(param.Type) + // `list of T` for a declared type parameter T, as for Java (#1183). + if l := listOfTypeParameter(param.Type, typeParamNameToID); l != nil { + p.ParameterType = l + } else { + p.ParameterType = astDataTypeToJavaActionParamType(param.Type) + } } if sp := storedParams[param.Name]; sp != nil { p.Description = sp.Description @@ -179,6 +184,10 @@ func execCreateJavaScriptAction(ctx *ExecContext, s *ast.CreateJavaScriptActionS TypeParameterID: typeParamNameToID[tpName], TypeParameter: tpName, } + } else if l := listOfTypeParameter(s.ReturnType, typeParamNameToID); l != nil { + // `returns list of T` — without this it became a list of the entity `.T`, + // the JavaScript twin of the Java action's #1183. + jsa.ReturnType = l } else { jsa.ReturnType = astDataTypeToJavaActionReturnType(s.ReturnType) } diff --git a/mdl/executor/describe_javascript_action_type_params_test.go b/mdl/executor/describe_javascript_action_type_params_test.go new file mode 100644 index 0000000000..49a91213cc --- /dev/null +++ b/mdl/executor/describe_javascript_action_type_params_test.go @@ -0,0 +1,140 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/backend/mock" + "github.com/mendixlabs/mxcli/mdl/types" + "github.com/mendixlabs/mxcli/model" +) + +// describeJSA describes one mock JavaScript action and returns the output. +func describeJSA(t *testing.T, jsa *types.JavaScriptAction) string { + t.Helper() + mod := mkModule("NanoflowCommons") + jsa.BaseElement = model.BaseElement{ID: nextID("jsa")} + jsa.ContainerID = mod.ID + jsa.Platform = "All" + mb := &mock.MockBackend{ + IsConnectedFunc: func() bool { return true }, + ReadJavaScriptActionByNameFunc: func(string) (*types.JavaScriptAction, error) { + return jsa, nil + }, + } + ctx, buf := newMockCtx(t, withBackend(mb)) + assertNoError(t, describeJavaScriptAction(ctx, ast.QualifiedName{Module: "NanoflowCommons", Name: jsa.Name})) + return buf.String() +} + +func typeParam(name string) *types.TypeParameterDef { + return &types.TypeParameterDef{BaseElement: model.BaseElement{ID: nextID("tp")}, Name: name} +} + +// NanoflowCommons.GetStorageItemObject: the type parameter is declared by an +// `entity ` parameter, as for Java actions. Describe printed it a second +// time as `Name` after the action name, which the grammar has no slot +// for, so the output did not parse. +func TestDescribeJavaScriptAction_DeclaredTypeParameterReparses(t *testing.T) { + tp := typeParam("Entity") + out := describeJSA(t, &types.JavaScriptAction{ + Name: "GetStorageItemObject", + TypeParameters: []*types.TypeParameterDef{tp}, + Parameters: []*types.JavaActionParameter{ + {Name: "Key", IsRequired: true, ParameterType: &types.StringType{}}, + {Name: "Entity", IsRequired: true, ParameterType: &types.EntityTypeParameterType{TypeParameterID: tp.ID, TypeParameterName: "Entity"}}, + }, + ReturnType: &types.TypeParameter{TypeParameterID: tp.ID, TypeParameter: "Entity"}, + }) + stmt := findStmt[*ast.CreateJavaScriptActionStmt](t, reparse(t, out), out) + if len(stmt.TypeParameters) != 1 || stmt.TypeParameters[0] != "Entity" { + t.Errorf("re-parsed type parameters %q, want [Entity]\n%s", stmt.TypeParameters, out) + } + if strings.Contains(out, "NOT re-executable") { + t.Errorf("an expressible action is marked NOT re-executable:\n%s", out) + } +} + +// The shapes MDL has no spelling for must not print a statement that parses +// into something else: commented out under a NOT re-executable note, the script +// stays parseable and re-running it leaves the stored action alone. +func TestDescribeJavaScriptAction_InexpressibleTypeParameterIsCommentedOut(t *testing.T) { + cases := map[string]*types.JavaScriptAction{ + // NanoflowCommons.GetGuid: a parameter typed by a type parameter that no + // `entity ` parameter declares (mendixlabs/mxcli#1404's shape). + "undeclared type parameter": func() *types.JavaScriptAction { + tp := typeParam("TypeParameter") + return &types.JavaScriptAction{ + Name: "GetGuid", + TypeParameters: []*types.TypeParameterDef{tp}, + Parameters: []*types.JavaActionParameter{ + {Name: "EntityObject", IsRequired: true, ParameterType: &types.TypeParameter{TypeParameterID: tp.ID, TypeParameter: "TypeParameter"}}, + }, + ReturnType: &types.StringType{}, + } + }(), + // NanoflowCommons.RefreshEntity: an entity selector bound to no type + // parameter (null TypeParameterPointer, empty TypeParameters) printed as + // `entity <>`. + "unbound entity selector": { + Name: "RefreshEntity", + Parameters: []*types.JavaActionParameter{ + {Name: "EntityToRefresh", IsRequired: true, ParameterType: &types.EntityTypeParameterType{}}, + }, + ReturnType: &types.VoidType{}, + }, + } + for name, jsa := range cases { + t.Run(name, func(t *testing.T) { + out := describeJSA(t, jsa) + prog := reparse(t, out) + for _, s := range prog.Statements { + if _, ok := s.(*ast.CreateJavaScriptActionStmt); ok { + t.Errorf("an inexpressible action printed a live statement:\n%s", out) + } + } + if !strings.Contains(out, "-- NOT re-executable:") { + t.Errorf("no NOT re-executable note:\n%s", out) + } + }) + } +} + +// NanoflowCommons.GetStorageItemObjectList returns `list of Entity`, Entity a +// type parameter. exec wrote it as a list of the entity `.Entity` — the +// JavaScript twin of the Java action's #1183 — so the describe output that now +// parses still did not round-trip. +func TestCreateJavaScriptAction_ListOfTypeParameter(t *testing.T) { + mod := mkModule("NanoflowCommons") + var captured *types.JavaScriptAction + mb := &mock.MockBackend{ + IsConnectedFunc: func() bool { return true }, + ListModulesFunc: func() ([]*model.Module, error) { return []*model.Module{mod}, nil }, + ListJavaScriptActionsFunc: func() ([]*types.JavaScriptAction, error) { return nil, nil }, + CreateJavaScriptActionFunc: func(jsa *types.JavaScriptAction) error { + captured = jsa + return nil + }, + WriteJavaScriptSourceFileFunc: func(string, string, string, []*types.JavaActionParameter, types.CodeActionReturnType) error { + return nil + }, + } + ctx, _ := newMockCtx(t, withBackend(mb), withHierarchy(mkHierarchy(mod))) + src := "create javascript action NanoflowCommons.GetList(Entity: entity <\"Entity\"> not null, Items: List of \"Entity\")\n" + + " returns List of \"Entity\"\n platform All\nas $$\nreturn [];\n$$;" + stmt := findStmt[*ast.CreateJavaScriptActionStmt](t, reparse(t, src), src) + assertNoError(t, execCreateJavaScriptAction(ctx, stmt)) + if captured == nil || len(captured.TypeParameters) != 1 { + t.Fatalf("type parameters not written: %+v", captured) + } + tpID := captured.TypeParameters[0].ID + for what, got := range map[string]any{"parameter Items": captured.Parameters[1].ParameterType, "return type": captured.ReturnType} { + l, ok := got.(*types.ListType) + if !ok || l.TypeParameterID != tpID || l.Entity != "" { + t.Errorf("%s = %#v, want a list of type parameter %s", what, got, tpID) + } + } +} diff --git a/mdl/roundtrip/allowlist_test.go b/mdl/roundtrip/allowlist_test.go index f83d25a037..9d06717275 100644 --- a/mdl/roundtrip/allowlist_test.go +++ b/mdl/roundtrip/allowlist_test.go @@ -70,16 +70,6 @@ var knownFailures = map[string]knownFailure{ "building block Atlas_Web_Content.Wizard_Circle": {laws: []law{lawParse}, issue: "#721", why: "read-only, but describe prints a widget body that does not parse (#721 G)"}, "building block Atlas_Web_Content.Wizard_Circle_Step": {laws: []law{lawParse}, issue: "#721", why: "read-only, but describe prints a widget body that does not parse (#721 G)"}, "building block FeedbackModule.FeedbackWidget": {laws: []law{lawParse}, issue: "#721", why: "read-only, but describe prints a widget body that does not parse (#721 G)"}, - "javascript action FeedbackModule.JS_GetFeedbackStorageObject": {laws: []law{lawParse}, issue: "#721", why: "a parameter comma is printed after its -- comment (#721 G)"}, - "javascript action NanoflowCommons.FindObjectWithGUID": {laws: []law{lawParse}, issue: "#721", why: "the type parameter does not parse (#721 G)"}, - "javascript action NanoflowCommons.GetGuid": {laws: []law{lawParse}, issue: "#721", why: "the type parameter does not parse (#721 G)"}, - "javascript action NanoflowCommons.GetObjectByGuid": {laws: []law{lawParse}, issue: "#721", why: "the type parameter does not parse (#721 G)"}, - "javascript action NanoflowCommons.GetStorageItemObject": {laws: []law{lawParse}, issue: "#721", why: "the type parameter does not parse (#721 G)"}, - "javascript action NanoflowCommons.GetStorageItemObjectList": {laws: []law{lawParse}, issue: "#721", why: "the type parameter does not parse (#721 G)"}, - "javascript action NanoflowCommons.RefreshEntity": {laws: []law{lawParse}, issue: "#721", why: "a parameter comma is printed after its -- comment (#721 G)"}, - "javascript action NanoflowCommons.RefreshObject": {laws: []law{lawParse}, issue: "#721", why: "the type parameter does not parse (#721 G)"}, - "javascript action NanoflowCommons.SetStorageItemObject": {laws: []law{lawParse}, issue: "#721", why: "the type parameter does not parse (#721 G)"}, - "javascript action NanoflowCommons.SetStorageItemObjectList": {laws: []law{lawParse}, issue: "#721", why: "the type parameter does not parse (#721 G)"}, // #721 E: menus — struck by ako/mxcli#980 (the menu item writer's missing // AlternativeText and Items). diff --git a/mdl/roundtrip/testapp_allowlist_test.go b/mdl/roundtrip/testapp_allowlist_test.go index 0e0f2db999..0b04977498 100644 --- a/mdl/roundtrip/testapp_allowlist_test.go +++ b/mdl/roundtrip/testapp_allowlist_test.go @@ -122,16 +122,6 @@ var testAppKnownFailures = map[string]knownFailure{ "export mapping Services.OrdersExportMapping": {laws: []law{lawGetPut}, issue: "#721", why: "export mapping: breaks getput on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes"}, "import mapping Clients.SoapCustomerOrdersImportMapping": {laws: []law{lawGetPut}, issue: "#721", why: "import mapping: breaks getput on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes"}, "import mapping Clients.SoapOrdersImportMapping": {laws: []law{lawGetPut}, issue: "#721", why: "import mapping: breaks getput on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes"}, - "javascript action FeedbackModule.JS_GetFeedbackStorageObject": {laws: []law{lawParse}, issue: "#721", why: "javascript action: breaks parse on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes"}, - "javascript action NanoflowCommons.FindObjectWithGUID": {laws: []law{lawParse}, issue: "#721", why: "javascript action: breaks parse on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes"}, - "javascript action NanoflowCommons.GetGuid": {laws: []law{lawParse}, issue: "#721", why: "javascript action: breaks parse on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes"}, - "javascript action NanoflowCommons.GetObjectByGuid": {laws: []law{lawParse}, issue: "#721", why: "javascript action: breaks parse on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes"}, - "javascript action NanoflowCommons.GetStorageItemObject": {laws: []law{lawParse}, issue: "#721", why: "javascript action: breaks parse on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes"}, - "javascript action NanoflowCommons.GetStorageItemObjectList": {laws: []law{lawParse}, issue: "#721", why: "javascript action: breaks parse on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes"}, - "javascript action NanoflowCommons.RefreshEntity": {laws: []law{lawParse}, issue: "#721", why: "javascript action: breaks parse on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes"}, - "javascript action NanoflowCommons.RefreshObject": {laws: []law{lawParse}, issue: "#721", why: "javascript action: breaks parse on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes"}, - "javascript action NanoflowCommons.SetStorageItemObject": {laws: []law{lawParse}, issue: "#721", why: "javascript action: breaks parse on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes"}, - "javascript action NanoflowCommons.SetStorageItemObjectList": {laws: []law{lawParse}, issue: "#721", why: "javascript action: breaks parse on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes"}, "page Administration.Account_Overview": {laws: []law{lawGetPut}, issue: "#721", why: "page: breaks getput on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes"}, "page FeedbackModule.PopupFailure": {laws: []law{lawGetPut}, issue: "#721", why: "page: breaks getput on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes"}, "page FeedbackModule.PopupSuccess": {laws: []law{lawGetPut}, issue: "#721", why: "page: breaks getput on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes"}, From 743ed846e64ad63486129509ade46a08158a117f Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 13 Oct 2026 07:58:59 +0000 Subject: [PATCH 13/17] fix(test): @throws catches errors from CALL JAVA ACTION and other calls (#1164) The test runner attached its ON ERROR handler only to CALL MICROFLOW and CALL NANOFLOW, so an error raised by CALL JAVA ACTION escaped the generated test microflow: an @throws test was reported ERROR instead of PASS, and an unexpected error was ERROR instead of the usual "exception during execution: ..." FAIL. Both runners now wrap every call-shaped activity the grammar gives an onErrorClause: call microflow / nanoflow / java action / external action / web service / workflow / rest service, send rest request, and execute database query. The match is anchored at the statement start, so a LOG whose text mentions a call is no longer treated as one. Verified live on Mendix 11.12.1 (--local --db-type hsqldb) against the same project: unfixed runner ERROR/PASS/ERROR, fixed PASS/PASS/FAIL. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01BDw5ToBMR2AVJomKZy6KWW --- ...s-cannot-catch-call-java-action-error.json | 1 + cmd/mxcli/testrunner/generator.go | 31 ++++--- cmd/mxcli/testrunner/generator_endpoint.go | 2 +- .../testrunner/on_error_coverage_test.go | 80 +++++++++++++++++++ docs-site/src/tools/test-annotations.md | 8 ++ .../1164-throws-catches-java-action.mdl | 17 ++++ .../1164-throws-catches-java-action.test.mdl | 21 +++++ 7 files changed, 148 insertions(+), 12 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/cmd-mxcli/2026-10-13-throws-cannot-catch-call-java-action-error.json create mode 100644 cmd/mxcli/testrunner/on_error_coverage_test.go create mode 100644 mdl-examples/bug-tests/1164-throws-catches-java-action.mdl create mode 100644 mdl-examples/bug-tests/1164-throws-catches-java-action.test.mdl diff --git a/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-13-throws-cannot-catch-call-java-action-error.json b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-13-throws-cannot-catch-call-java-action-error.json new file mode 100644 index 0000000000..fae98d5779 --- /dev/null +++ b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-13-throws-cannot-catch-call-java-action-error.json @@ -0,0 +1 @@ +{"area":"cmd/mxcli","date":"2026-10-13","symptom":"`mxcli test`: a test whose body does `call java action` cannot use `@throws` — the expected Java exception is reported as `ERROR `, not PASS; without `@throws` it is also ERROR rather than the usual `FAIL exception during execution: …`","cause":"The runner attaches its `ON ERROR` handler only to statements matched by `containsCallMicroflow`, which recognised `CALL MICROFLOW` / `CALL NANOFLOW` only; every other call-shaped activity was emitted without a handler and its error escaped the generated test microflow to the endpoint's outer handler","file":"`cmd/mxcli/testrunner/generator.go` (`startsErrorRaisingCall`, used by `attachOnError`, `rewriteWithErrorHandling`, `rewriteForThrowsTest`)","insight":"The set of statements the runner wraps must be the set the grammar gives `onErrorClause` to (MDLMicroflow.g4) — enumerate it from there, not from the statements the first tests used. Unit test each kind through BOTH generators and `visitor.Build` the output, since an `ON ERROR` on a statement that cannot take one would be a parse error. The cheap workaround (wrap the call in a microflow) hides the gap, so it went unreported until a module's own Java actions were tested. Proven live on 11.12.1 `--local --db-type hsqldb`: unfixed binary ERROR/ERROR, fixed PASS/FAIL on the same project. Also anchored the match at the statement start — the old `strings.Contains` treated a LOG whose text mentioned `call microflow` as a call","refs":["#1164"]} diff --git a/cmd/mxcli/testrunner/generator.go b/cmd/mxcli/testrunner/generator.go index 5ba54e0a77..c28cb42c2e 100644 --- a/cmd/mxcli/testrunner/generator.go +++ b/cmd/mxcli/testrunner/generator.go @@ -337,7 +337,7 @@ func renameExpect(exp Expect, names map[string]bool, suffix string) Expect { return renamed } -// rewriteWithErrorHandling adds ON ERROR clauses to CALL MICROFLOW statements. +// rewriteWithErrorHandling adds ON ERROR clauses to the calls in the body. // The error handler ends with RETURN $AllPassed to properly terminate the flow path // with a return value (required when the microflow returns a type). func rewriteWithErrorHandling(lines []string, testID string) []string { @@ -347,7 +347,7 @@ func rewriteWithErrorHandling(lines []string, testID string) []string { line := lines[i] trimmed := strings.TrimSpace(line) - if containsCallMicroflow(trimmed) && strings.HasSuffix(trimmed, ";") { + if startsErrorRaisingCall(trimmed) && strings.HasSuffix(trimmed, ";") { withoutSemicolon := strings.TrimSuffix(trimmed, ";") result = append(result, withoutSemicolon+" ON ERROR BEGIN") result = append(result, fmt.Sprintf( @@ -357,7 +357,7 @@ func rewriteWithErrorHandling(lines []string, testID string) []string { result = append(result, " SET $AllPassed = false;") result = append(result, " RETURN $AllPassed;") result = append(result, "END ERROR;") - } else if containsCallMicroflow(trimmed) && !strings.HasSuffix(trimmed, ";") { + } else if startsErrorRaisingCall(trimmed) && !strings.HasSuffix(trimmed, ";") { var callLines []string callLines = append(callLines, line) for i+1 < len(lines) { @@ -387,7 +387,7 @@ func rewriteWithErrorHandling(lines []string, testID string) []string { return result } -// rewriteForThrowsTest wraps CALL MICROFLOW with ON ERROR that sets the throw flag. +// rewriteForThrowsTest wraps each call in the body with ON ERROR that sets the throw flag. // // The flag is set only when the raised message matches the annotation's, so this // runner reports a wrong expectation the same way the endpoint runner does — as @@ -400,12 +400,12 @@ func rewriteForThrowsTest(lines []string, didThrowVar string, want string) []str line := lines[i] trimmed := strings.TrimSpace(line) - if containsCallMicroflow(trimmed) && strings.HasSuffix(trimmed, ";") { + if startsErrorRaisingCall(trimmed) && strings.HasSuffix(trimmed, ";") { withoutSemicolon := strings.TrimSuffix(trimmed, ";") result = append(result, withoutSemicolon+" ON ERROR BEGIN") result = append(result, throwsFlagHandler(didThrowVar, want)...) result = append(result, "END ERROR;") - } else if containsCallMicroflow(trimmed) && !strings.HasSuffix(trimmed, ";") { + } else if startsErrorRaisingCall(trimmed) && !strings.HasSuffix(trimmed, ";") { var callLines []string callLines = append(callLines, line) for i+1 < len(lines) { @@ -444,13 +444,22 @@ func throwsFlagHandler(didThrowVar, want string) []string { } } -// containsCallMicroflow checks if a line starts a CALL MICROFLOW statement. -func containsCallMicroflow(s string) bool { - upper := strings.ToUpper(s) - return strings.Contains(upper, "CALL MICROFLOW") || - strings.Contains(upper, "CALL NANOFLOW") +// startsErrorRaisingCall reports whether a line starts a call-shaped activity +// the runner must catch an error from: one that takes an ON ERROR clause in a +// microflow (onErrorClause on its rule in MDLMicroflow.g4). +// +// It used to match CALL MICROFLOW and CALL NANOFLOW only, so an error raised by +// CALL JAVA ACTION escaped the test microflow — @throws could not catch it and +// a test without one was reported as ERROR instead of FAIL (ako/mxcli#1164). +// Anchored at the statement start so a string that mentions a call is not one. +func startsErrorRaisingCall(s string) bool { + return errorRaisingCall.MatchString(s) } +var errorRaisingCall = regexp.MustCompile(`(?i)^\s*(\$\w+\s*=\s*)?(` + + `call\s+(microflow|nanoflow|java\s+action|external\s+action|web\s+service|rest\s+service|workflow)` + + `|rest\s+call|send\s+rest\s+request|execute\s+database\s+query)\b`) + // escapeMDLString escapes single quotes for MDL string literals. func escapeMDLString(s string) string { return strings.ReplaceAll(s, "'", "''") diff --git a/cmd/mxcli/testrunner/generator_endpoint.go b/cmd/mxcli/testrunner/generator_endpoint.go index 215a9ea7c8..2d0cfb53a2 100644 --- a/cmd/mxcli/testrunner/generator_endpoint.go +++ b/cmd/mxcli/testrunner/generator_endpoint.go @@ -234,7 +234,7 @@ func attachOnError(lines, handler []string) []string { var out []string for i := 0; i < len(lines); i++ { trimmed := strings.TrimSpace(lines[i]) - if !containsCallMicroflow(trimmed) { + if !startsErrorRaisingCall(trimmed) { out = append(out, lines[i]) continue } diff --git a/cmd/mxcli/testrunner/on_error_coverage_test.go b/cmd/mxcli/testrunner/on_error_coverage_test.go new file mode 100644 index 0000000000..d190814ac8 --- /dev/null +++ b/cmd/mxcli/testrunner/on_error_coverage_test.go @@ -0,0 +1,80 @@ +// SPDX-License-Identifier: Apache-2.0 + +package testrunner + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/visitor" +) + +// errorRaisingCalls is one statement per call-shaped activity that takes an +// ON ERROR clause in a microflow — the grammar's onErrorClause on each rule in +// MDLMicroflow.g4. The test runner has to catch an error from any of them, or +// one raised by the body escapes the generated test microflow. +var errorRaisingCalls = []struct{ kind, stmt string }{ + {"call microflow", "$r = call microflow M.MF_Import(Json = '{}');"}, + {"call nanoflow", "$r = call nanoflow M.NF_Import(Json = '{}');"}, + {"call java action", "$created = call java action CsvServices.ImportCsvUrlData(Entity = 'Tests.AllTypes', CsvUrl = 'file:///etc/passwd', MaxRecords = -1, HasHeader = true, AlternativeHeader = empty, Delimiter = empty);"}, + {"call external action", "$r = call external action M.Svc.Act(P = 1);"}, + {"call web service", "$r = call web service M.WS operation M.WS.Op();"}, + {"call workflow", "$wf = call workflow M.WF($Ctx);"}, + {"call rest service", "$r = call rest service get 'https://example.com' returns String;"}, + {"send rest request", "$r = send rest request M.Client.Op;"}, + {"execute database query", "$r = execute database query M.Conn.Q;"}, +} + +// TestThrowsCatchesEveryCallKind pins that @throws attaches its handler to +// every call that can raise, not only CALL MICROFLOW / CALL NANOFLOW. +// +// A Java action that raised the expected error was reported as ERROR, not +// PASS, because the runner left the CALL JAVA ACTION without a handler and +// the error escaped the test microflow (ako/mxcli#1164). +func TestThrowsCatchesEveryCallKind(t *testing.T) { + for _, c := range errorRaisingCalls { + t.Run(c.kind, func(t *testing.T) { + tc := parseOne(t, "/**\n * @test it raises\n * @throws 'Only http and https csv urls are supported'\n */\n"+c.stmt+"\n/\n") + assertHandled(t, "endpoint runner", GenerateTestFlows(&TestSuite{Name: "s", Tests: []TestCase{tc}}), c.stmt) + assertHandled(t, "legacy runner", GenerateTestRunner(&TestSuite{Name: "s", Tests: []TestCase{tc}}), c.stmt) + }) + } +} + +// TestVerdictCatchesEveryCallKind is the same for a test without @throws: an +// error the body did not expect must become the usual "exception during +// execution: …" FAIL, not escape to the outer handler. +func TestVerdictCatchesEveryCallKind(t *testing.T) { + for _, c := range errorRaisingCalls { + t.Run(c.kind, func(t *testing.T) { + tc := parseOne(t, "/**\n * @test it runs\n */\n"+c.stmt+"\n/\n") + mdl := GenerateTestFlows(&TestSuite{Name: "s", Tests: []TestCase{tc}}) + assertHandled(t, "endpoint runner", mdl, c.stmt) + if !strings.Contains(mdl, verdictFailPrefix+"exception during execution: ") { + t.Errorf("no FAIL verdict in the handler:\n%s", mdl) + } + assertHandled(t, "legacy runner", GenerateTestRunner(&TestSuite{Name: "s", Tests: []TestCase{tc}}), c.stmt) + }) + } +} + +// TestNonCallStatementsGetNoHandler is the other side: a statement that is not +// a call is left alone, including one that merely mentions a call in a string. +func TestNonCallStatementsGetNoHandler(t *testing.T) { + tc := parseOne(t, "/**\n * @test it logs\n */\nlog info node 'T' 'about to call java action X';\n/\n") + mdl := GenerateTestFlows(&TestSuite{Name: "s", Tests: []TestCase{tc}}) + if strings.Contains(mdl, "ON ERROR BEGIN") { + t.Errorf("a LOG mentioning a call was given a handler:\n%s", mdl) + } +} + +func assertHandled(t *testing.T, runner, mdl, stmt string) { + t.Helper() + call := strings.TrimSuffix(stmt, ";") + if !strings.Contains(mdl, call+" ON ERROR BEGIN") { + t.Errorf("%s: no ON ERROR handler on %q:\n%s", runner, call, mdl) + } + if _, errs := visitor.Build(mdl); len(errs) > 0 { + t.Errorf("%s: generated MDL does not parse: %v\n%s", runner, errs[0], mdl) + } +} diff --git a/docs-site/src/tools/test-annotations.md b/docs-site/src/tools/test-annotations.md index 6ea2e6917a..2d1c3e9b83 100644 --- a/docs-site/src/tools/test-annotations.md +++ b/docs-site/src/tools/test-annotations.md @@ -145,6 +145,14 @@ $result = call microflow Sales.ACT_Submit(Order = $empty); Anything after `@throws` that is not a single-quoted string is an error, not a message — `@throws not found` is refused rather than read as an unquoted one. +The handler is attached to every **call** in the body: `call microflow`, +`call nanoflow`, `call java action`, `call external action`, `call web service`, +`call workflow`, `call rest service`, `send rest request` and +`execute database query`. An error raised by any other statement — a `commit`, +a `change`, a `create` — is not caught; move it into a microflow and call that. +The same handler turns an unexpected error in a test without `@throws` into a +FAIL reading `exception during execution: `. + ## `@setup` Names a **microflow** to call before the test's own statements — a fixture in a diff --git a/mdl-examples/bug-tests/1164-throws-catches-java-action.mdl b/mdl-examples/bug-tests/1164-throws-catches-java-action.mdl new file mode 100644 index 0000000000..732d983055 --- /dev/null +++ b/mdl-examples/bug-tests/1164-throws-catches-java-action.mdl @@ -0,0 +1,17 @@ +mdl 1; +-- ako/mxcli#1164: setup for 1164-throws-catches-java-action.test.mdl. +-- Run this against a project, then: +-- mxcli test mdl-examples/bug-tests/1164-throws-catches-java-action.test.mdl -p app.mpr --local --db-type hsqldb +-- Before the fix the @throws test reported ERROR with the Java exception's +-- message: the runner attached ON ERROR to CALL MICROFLOW / CALL NANOFLOW only, +-- so an error from CALL JAVA ACTION escaped the generated test microflow. +create module ThrowProbe; + +/** Refuses anything but an http(s) url, the shape of the reported action. */ +create java action ThrowProbe.CheckUrl(Url: string not null) returns string +as $$ +if (Url == null || !(Url.startsWith("http://") || Url.startsWith("https://"))) { + throw new IllegalArgumentException("Only http and https csv urls are supported: " + Url); +} +return "ok"; +$$; diff --git a/mdl-examples/bug-tests/1164-throws-catches-java-action.test.mdl b/mdl-examples/bug-tests/1164-throws-catches-java-action.test.mdl new file mode 100644 index 0000000000..1511346d4b --- /dev/null +++ b/mdl-examples/bug-tests/1164-throws-catches-java-action.test.mdl @@ -0,0 +1,21 @@ +/** + * ako/mxcli#1164: @throws catches an error raised by CALL JAVA ACTION, not only + * one raised by CALL MICROFLOW / CALL NANOFLOW. + * + * Setup: run 1164-throws-catches-java-action.mdl against the project first. + * + * @test java action refuses file urls + * @throws 'Only http and https csv urls are supported' + */ +$r = call java action ThrowProbe.CheckUrl(Url = 'file:///etc/passwd'); +/ + +/** + * Control: the same action with an accepted url does not throw, so the @throws + * above passes because of the url and not because the handler swallows anything. + * + * @test java action accepts https urls + * @expect $r = 'ok' + */ +$r = call java action ThrowProbe.CheckUrl(Url = 'https://example.com/data.csv'); +/ From 7c0436760c41ae6d1d5398c16467d55235457ca7 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 13 Oct 2026 08:21:07 +0000 Subject: [PATCH 14/17] test: check that FeedbackModule's with-all dump parses The exemption covered JavaScript actions with type parameters describing as unparseable MDL; #1170 fixed that, so the module is now held to the same parse assertion as the others. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01W3x8Fyvir8kLyPgJs1BDmF --- mdl/executor/cmd_modules_describe_all_test.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/mdl/executor/cmd_modules_describe_all_test.go b/mdl/executor/cmd_modules_describe_all_test.go index 8aa5d2c339..0e889d704a 100644 --- a/mdl/executor/cmd_modules_describe_all_test.go +++ b/mdl/executor/cmd_modules_describe_all_test.go @@ -46,14 +46,13 @@ var catalogProgress = regexp.MustCompile(`\A(?:(?:Loading cached catalog|Buildin func TestDescribeModuleWithAll_AccountsForEveryDocument(t *testing.T) { for _, tc := range []struct { fixture, mpr, module string - // parses is false where a document's own describe does not parse - // today — a JavaScript action with a type parameter prints - // `Name` / `entity <>`, which the grammar rejects. That is the - // JavaScript action describer's defect, not the module dump's; the - // module is still checked for completeness. + // parses is false where a document's own describe does not parse; + // the module is then still checked for completeness. FeedbackModule + // needed it until JavaScript actions with type parameters described + // as parseable MDL (#1170). parses bool }{ - {"pedapp", "PedApp.mpr", "FeedbackModule", false}, + {"pedapp", "PedApp.mpr", "FeedbackModule", true}, {"pedapp", "PedApp.mpr", "Atlas_Core", true}, // icon collections: read-only reports {"pedapp", "PedApp.mpr", "Atlas_Web_Content", true}, // building blocks, page templates {"pedapp", "PedApp.mpr", "Administration", true}, From 0b558a57b3c21c2dc1375bec7015f4c9858a3def Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 13 Oct 2026 09:23:49 +0000 Subject: [PATCH 15/17] fix: name the auto-detected project and refuse an ambiguous one (mendixlabs/mxcli#1444) Reported: "Commands without `-p` silently pick a stray `.mpr` in the working directory" -- `mxcli describe module M` run in a directory holding a stray .mpr proceeds against it with only a "Using project" notice. Auto-detecting the sole .mpr in the working directory stays: it is how init, new, theme and the LSP resolve a project, and `cd app && mxcli ...` depends on it. The defect was that the guess read exactly like an explicit choice. The notice now names the absolute path, says the project was auto-detected, and how to choose another: Using project: /abs/dir/App.mpr (auto-detected in the current directory; pass -p to use another) With several .mpr files nothing is picked (as before), but stderr now lists them and asks for -p instead of leaving only "--project (-p) is required". That note respects -q / MXCLI_QUIET. The -p value set by auto-detection is still the bare file name, so nothing downstream sees a different path, and no exit code changes. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01W3x8Fyvir8kLyPgJs1BDmF --- ...10-12-stray-mpr-autodiscovery-unnamed.json | 1 + cmd/mxcli/main.go | 39 ++++++++--- cmd/mxcli/project_discovery_test.go | 64 +++++++++++++++++++ 3 files changed, 94 insertions(+), 10 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/cmd-mxcli/2026-10-12-stray-mpr-autodiscovery-unnamed.json create mode 100644 cmd/mxcli/project_discovery_test.go diff --git a/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-12-stray-mpr-autodiscovery-unnamed.json b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-12-stray-mpr-autodiscovery-unnamed.json new file mode 100644 index 0000000000..12d7134f9b --- /dev/null +++ b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-12-stray-mpr-autodiscovery-unnamed.json @@ -0,0 +1 @@ +{"area":"cmd/mxcli","date":"2026-10-12","symptom":"A command run without `-p` (e.g. `mxcli describe module M`) silently works against a stray `.mpr` in the working directory; the only sign is `Using project: App.mpr`, which reads the same for the intended project and a stray copy. With several `.mpr` files nothing is picked and the command just says `--project (-p) is required`","cause":"`rootCmd.PersistentPreRun` auto-discovery (`discoverProjectPath`) printed only the bare file name it set `-p` to, and returned nothing at all when the directory was ambiguous","file":"`cmd/mxcli/main.go` (`discoverProject`, called from `rootCmd.PersistentPreRun`)","insight":"Auto-detecting the sole `.mpr` is the CLI's documented resolution (init, new, theme and the LSP all do it), so refusing a single match would break `cd app && mxcli ...`; the defect was that the guess was indistinguishable from an explicit choice. Name the absolute path, say it was auto-detected and how to override, and list the candidates when ambiguous. The `-p` value stays the bare file name so nothing downstream sees a different path. Unit-test the helper on a temp dir (no chdir); control: a body returning the old bare notice fails all three content assertions","refs":["mendixlabs/mxcli#1444"]} diff --git a/cmd/mxcli/main.go b/cmd/mxcli/main.go index 74d08c36e2..a68cb28872 100644 --- a/cmd/mxcli/main.go +++ b/cmd/mxcli/main.go @@ -8,6 +8,7 @@ import ( "fmt" "io" "os" + "path/filepath" "strings" "github.com/mendixlabs/mxcli/mdl/backend" @@ -84,12 +85,18 @@ func shouldSuppressWarning() bool { return false } -// discoverProjectPath looks for a single .mpr file in the current directory. -// Returns the filename if exactly one is found, otherwise returns "". -func discoverProjectPath() string { - entries, err := os.ReadDir(".") +// discoverProject looks for .mpr files in dir, for a command run without -p. +// With exactly one it returns its file name (the value -p is set to) and a +// notice naming its absolute path. With several it picks none and returns a +// note listing them and asking for -p. With none it returns ("", ""). +// +// The notice is the only sign that the project was guessed rather than given, +// so it has to name where the file is: "Using project: App.mpr" read the same +// for the intended project and a stray copy (mendixlabs/mxcli#1444). +func discoverProject(dir string) (path, note string) { + entries, err := os.ReadDir(dir) if err != nil { - return "" + return "", "" } var mprFiles []string for _, e := range entries { @@ -97,10 +104,19 @@ func discoverProjectPath() string { mprFiles = append(mprFiles, e.Name()) } } - if len(mprFiles) == 1 { - return mprFiles[0] + switch len(mprFiles) { + case 0: + return "", "" + case 1: + abs := filepath.Join(dir, mprFiles[0]) + if a, err := filepath.Abs(abs); err == nil { + abs = a + } + return mprFiles[0], fmt.Sprintf("Using project: %s (auto-detected in the current directory; pass -p to use another)", abs) + default: + return "", fmt.Sprintf("No project auto-detected: the current directory has %d .mpr files (%s); pass -p to choose one", + len(mprFiles), strings.Join(mprFiles, ", ")) } - return "" } var rootCmd = &cobra.Command{ @@ -136,9 +152,12 @@ beta language; --mdl 0 starts them in the alpha language, and in the REPL an PersistentPreRun: func(cmd *cobra.Command, args []string) { projectPath, _ := cmd.Flags().GetString("project") if projectPath == "" { - if discovered := discoverProjectPath(); discovered != "" { + discovered, note := discoverProject(".") + if discovered != "" { _ = cmd.Flags().Set("project", discovered) - fmt.Fprintf(os.Stderr, "Using project: %s\n", discovered) + fmt.Fprintln(os.Stderr, note) + } else if note != "" && !shouldSuppressWarning() { + fmt.Fprintln(os.Stderr, note) } } // A binary older than the mxcli that wrote this project's tooling is diff --git a/cmd/mxcli/project_discovery_test.go b/cmd/mxcli/project_discovery_test.go new file mode 100644 index 0000000000..b299994452 --- /dev/null +++ b/cmd/mxcli/project_discovery_test.go @@ -0,0 +1,64 @@ +package main + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// mendixlabs/mxcli#1444: a command run without -p picked up a stray .mpr in the +// working directory with only a bare "Using project: App.mpr", which does not +// say the project was guessed, where it is, or how to choose another. With +// several .mpr files it resolved nothing and said nothing about why. +func TestDiscoverProject_NamesTheGuessAndRefusesAmbiguity(t *testing.T) { + touch := func(t *testing.T, dir, name string) { + t.Helper() + if err := os.WriteFile(filepath.Join(dir, name), nil, 0o644); err != nil { + t.Fatal(err) + } + } + + t.Run("single .mpr is used and named by absolute path", func(t *testing.T) { + dir := t.TempDir() + touch(t, dir, "Stray.mpr") + path, note := discoverProject(dir) + if path != "Stray.mpr" { + t.Fatalf("path = %q, want the file name (flag value unchanged)", path) + } + abs := filepath.Join(dir, "Stray.mpr") + if !strings.Contains(note, abs) { + t.Errorf("notice %q does not name the absolute path %q", note, abs) + } + if !strings.Contains(note, "auto-detected") || !strings.Contains(note, "-p") { + t.Errorf("notice %q does not say the project was auto-detected and how to override it", note) + } + }) + + t.Run("several .mpr files are refused, listed, and -p is asked for", func(t *testing.T) { + dir := t.TempDir() + touch(t, dir, "A.mpr") + touch(t, dir, "B.mpr") + path, note := discoverProject(dir) + if path != "" { + t.Fatalf("path = %q, want none when ambiguous", path) + } + for _, want := range []string{"A.mpr", "B.mpr", "-p"} { + if !strings.Contains(note, want) { + t.Errorf("note %q does not mention %q", note, want) + } + } + }) + + t.Run("no .mpr says nothing", func(t *testing.T) { + dir := t.TempDir() + touch(t, dir, "notes.txt") + if err := os.Mkdir(filepath.Join(dir, "dir.mpr"), 0o755); err != nil { + t.Fatal(err) + } + path, note := discoverProject(dir) + if path != "" || note != "" { + t.Fatalf("got (%q, %q), want nothing", path, note) + } + }) +} From a7316295acba22be2ef1ca9e94d7a9f44ebf7e82 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 13 Oct 2026 09:32:01 +0000 Subject: [PATCH 16/17] fix: revoke resolves association members and refuses unknown ones (mendixlabs/mxcli#1225) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported: `revoke ModA.User on ModA.Tag (write (Tag_GuestGroup))` answered "No access rules found matching ModA.User on ModA.Tag" (now "nothing to revoke") with exit code 0, and the association stayed ReadWrite, while `write (Label)` on the same rule worked. The revoke resolved member names through EntityMembers, which yields attributes only, and qualified anything else as `Module.Entity.Member`. An association's MemberAccess is stored as `Module.Association`, so the ref matched nothing and the backend's 0-modified count was reported as an already-revoked right. GRANT's association enumeration (FROM side via ParentID, both ends for OWNER Both, cross-module Both, inherited) moves into accessRuleAssociations, and REVOKE resolves names against it, so both writers agree on which rule carries an association and on its stored reference. A member the rule cannot carry — a typo, or the TO end of a Default-owner association — is now refused, as GRANT refuses it (#758), instead of the silent "nothing to revoke". A known member whose right is already gone keeps the idempotent Unchanged notice. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01W3x8Fyvir8kLyPgJs1BDmF --- ...ation-member-qualified-like-attribute.json | 1 + .../1225-revoke-association-member.mdl | 47 ++++++ mdl/executor/cmd_security_write.go | 156 ++++++++++++------ .../revoke_association_member_test.go | 107 ++++++++++++ 4 files changed, 260 insertions(+), 51 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/mdl-executor/2026-10-12-revoke-association-member-qualified-like-attribute.json create mode 100644 mdl-examples/bug-tests/1225-revoke-association-member.mdl create mode 100644 mdl/executor/revoke_association_member_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-10-12-revoke-association-member-qualified-like-attribute.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-12-revoke-association-member-qualified-like-attribute.json new file mode 100644 index 0000000000..293cbd1c3e --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-12-revoke-association-member-qualified-like-attribute.json @@ -0,0 +1 @@ +{"date": "2026-10-12", "area": "mdl/executor", "symptom": "`revoke write (Tag_GuestGroup) on entity ModA.Tag from ModA.User` (or the older `revoke ModA.User on ModA.Tag (write (Tag_GuestGroup))`) answered \"No access rules found matching …\" (later \"Unchanged … nothing to revoke\"), exited 0, and the association stayed ReadWrite; `write (Label)` on the same rule worked (mendixlabs/mxcli#1225).", "cause": "execRevokeEntityAccess resolved member names through EntityMembers, which yields attributes only, and fell back to `Module.Entity.Member` for anything else. An association's MemberAccess is stored as `Module.Association` (AssociationQualifiedName), so the ref matched nothing and the backend reported 0 rules modified — indistinguishable from an already-revoked right. GRANT had its own association enumeration (FROM side via ParentID, both ends for OWNER Both, cross-module Both, inherited) that REVOKE never reused.", "file": "mdl/executor/cmd_security_write.go (accessRuleAssociations shared by grant and revoke; revoke refuses an unmatched member like grant's unmatchedGrantMembers)", "insight": "Two writers keyed on the same stored reference must share the code that builds it — the grant's association loop and the revoke's attribute-only map had drifted since #758 fixed inherited attributes in both. The idempotent 'nothing to revoke' notice hid it: a count of 0 cannot tell 'already revoked' from 'named something the rule cannot hold'. Separate those before calling the backend — resolve the name, refuse if unknown, and only then let 0 mean unchanged. Measured on a fresh 11.6.6 app: pre-fix binary prints the Unchanged notice and describe still shows write (Tag_GuestGroup); fixed binary revokes it and `mxcli docker check` reports 0 errors.", "refs": ["mdl/executor/revoke_association_member_test.go", "mdl-examples/bug-tests/1225-revoke-association-member.mdl"]} diff --git a/mdl-examples/bug-tests/1225-revoke-association-member.mdl b/mdl-examples/bug-tests/1225-revoke-association-member.mdl new file mode 100644 index 0000000000..bc7a1de9eb --- /dev/null +++ b/mdl-examples/bug-tests/1225-revoke-association-member.mdl @@ -0,0 +1,47 @@ +mdl 1; +-- ============================================================================ +-- Bug #1225 — `revoke … (write ())` said "No access rules found" +-- (later "nothing to revoke") for a rule that exists, exited 0, and left the +-- association ReadWrite. +-- +-- The revoke qualified every named member like an attribute, +-- `Issue1225.Tag.Tag_GuestGroup`, but an association's MemberAccess is stored +-- by its module-qualified name, `Issue1225.Tag_GuestGroup`, and only on the +-- rule of the FROM entity (ParentPointer). Nothing matched, so nothing changed. +-- A member the rule cannot carry (a typo, or the TO end of a Default-owner +-- association) is now refused, as GRANT refuses it, instead of answering +-- "nothing to revoke". +-- +-- mxcli exec this-file -p app.mpr +-- before: "Unchanged entity access: Issue1225.Tag (Issue1225.User) — nothing to revoke" +-- and `describe entity Issue1225.Tag` still shows Tag_GuestGroup ReadWrite +-- after: "Revoked partial access on Issue1225.Tag from Issue1225.User", +-- Label and Tag_GuestGroup both ReadOnly; `mx check` clean +-- ============================================================================ + +create module Issue1225; +create module role Issue1225.User; + +@position(100, 100) +create persistent entity Issue1225.Tag ( + Label: string(200) +); + +@position(400, 100) +create persistent entity Issue1225.GuestGroup ( + Name: string(200) +); + +create association Issue1225.Tag_GuestGroup + from Issue1225.Tag + to Issue1225.GuestGroup + type Reference; + +grant read *, write * on entity Issue1225.Tag to Issue1225.User; + +-- The attribute half always worked; the association half is the bug. +-- (The issue used the older `revoke R on M.E (write (…))` spelling — same statement.) +revoke write (Label) on entity Issue1225.Tag from Issue1225.User; +revoke write (Tag_GuestGroup) on entity Issue1225.Tag from Issue1225.User; + +describe entity Issue1225.Tag; diff --git a/mdl/executor/cmd_security_write.go b/mdl/executor/cmd_security_write.go index 6765d4ce1e..526788b28f 100644 --- a/mdl/executor/cmd_security_write.go +++ b/mdl/executor/cmd_security_write.go @@ -507,16 +507,8 @@ func execGrantEntityAccess(ctx *ExecContext, s *ast.GrantEntityAccessStmt) error } } - // Create entries for associations this entity owns. ParentID = FROM entity - // (FK owner), and for a Default-owner association MemberAccess belongs only - // on that side — adding it to the TO side triggers CE0066. - // - // `OWNER Both` is the exception: both ends own the association and Mendix - // expects a MemberAccess entry on each. Emitting it only on the FROM side - // left the TO entity's rule incomplete, which is CE0066 "Entity access is - // out of date" — the reported symptom (issuetracker #20). Verified against - // mxbuild 11.12.1: the same script with `OWNER Default` checks clean, so the - // owner mode is the trigger, not the reference set. + // Create entries for the associations this entity's rule carries — which + // side owns one, and how it is referenced, is accessRuleAssociations'. addAssociationAccess := func(name, ref string) { rights := defaultMemberAccess if writeMemberSet[name] { @@ -530,44 +522,13 @@ func execGrantEntityAccess(ctx *ExecContext, s *ast.GrantEntityAccessStmt) error AccessRights: rights, }) } - for _, assoc := range dm.Associations { - ownedHere := assoc.ParentID == entity.ID || - (assoc.Owner == domainmodel.AssociationOwnerBoth && assoc.ChildID == entity.ID) - if ownedHere { - addAssociationAccess(assoc.Name, module.Name+"."+assoc.Name) - } - } - for _, ca := range dm.CrossAssociations { - if ca.ParentID == entity.ID { - addAssociationAccess(ca.Name, module.Name+"."+ca.Name) - } - } - // A cross-module association owned by both ends is stored in the FROM - // entity's module, so this entity — the TO end — has to look for it there. - for _, other := range otherModuleBothOwnerAssociations(ctx, module.Name, entityQN) { - addAssociationAccess(other.Name, other.Ref) - } - // Associations declared on an ancestor. Mendix inheritance is multi-table: - // a specialization has ALL of its generalization's members, associations - // included, and its access rule needs an entry for each — exactly as it does - // for inherited attributes (#758). - // - // Leaving them out was CE0066 "Entity access is out of date" on the - // specialization's own module, and it made the rule OpenAIConnector ships - // impossible to express in MDL: `OpenAIDeployedModel extends - // GenAICommons.DeployedModel`, and `DeployedModel_InputModality` is declared - // on the parent, so the grant naming it was refused as "no such member" - // (mxcli-chat FINDINGS §26). Reproduced on a two-entity fixture with no - // marketplace module in sight: `GRANT … ON Derived (READ *, WRITE *)` gave - // CE0066 while the same rule on the base entity checked clean. - // - // The reference is qualified against the module that DECLARES the - // association, not this entity's — the same rule the attribute walk follows. - for _, inh := range inheritedAssociations(ctx, entityQN) { - if grantedMembers[inh.Name] { + // The enumeration lives in accessRuleAssociations so REVOKE resolves member + // names against exactly the set GRANT writes (mendixlabs/mxcli#1225). + for _, a := range accessRuleAssociations(ctx, module.Name, dm, entity, entityQN) { + if a.inherited && grantedMembers[a.Name] { continue // an association of this entity's own shadows it } - addAssociationAccess(inh.Name, inh.Ref) + addAssociationAccess(a.Name, a.Ref) } // A member named in the GRANT that matched nothing used to be dropped in @@ -685,16 +646,49 @@ func execRevokeEntityAccess(ctx *ExecContext, s *ast.RevokeEntityAccessStmt) err // not `Mod.Doc.HasContents`. Qualifying every name with this entity // matched no stored entry for an inherited member, and the revoke // answered "No access rules found" while the rule held ReadWrite. + // + // An association is stored by its module-qualified name and only on the + // rule of the entity that owns it, so it resolves through the same + // enumeration GRANT writes from (mendixlabs/mxcli#1225). + entityQN := module.Name + "." + s.Entity.Name memberRef := map[string]string{} - for _, mem := range EntityMembers(ctx, module.Name+"."+s.Entity.Name) { + for _, mem := range EntityMembers(ctx, entityQN) { memberRef[mem.Name] = mem.Ref } - qualify := func(m string) string { - if ref, ok := memberRef[m]; ok { - return ref + for _, a := range accessRuleAssociations(ctx, module.Name, dm, entity, entityQN) { + if _, shadowed := memberRef[a.Name]; !shadowed { + memberRef[a.Name] = a.Ref } - return module.Name + "." + s.Entity.Name + "." + m } + // An audit member is a real member with no MemberAccess of its own — + // naming one is accepted, and there is simply nothing to narrow. + known := map[string]bool{} + for name := range memberRef { + known[name] = true + } + for _, sys := range storedAuditMembers(entity) { + known[sys] = true + } + // A member the rule cannot carry is a mistake in the script, not an + // already-revoked right: answering "nothing to revoke" for it, exit 0, + // was the silent no-op of #1225. Refuse it as GRANT does (#758); a known + // member whose right is already gone stays the idempotent notice. + var readNamed, writeNamed []string + for _, right := range s.Rights { + switch right.Type { + case ast.EntityAccessReadMembers: + readNamed = append(readNamed, right.Members...) + case ast.EntityAccessWriteMembers: + writeNamed = append(writeNamed, right.Members...) + } + } + if unknown := unmatchedGrantMembers(readNamed, writeNamed, known); len(unknown) > 0 { + return mdlerrors.NewValidationf( + "entity %s has no member(s) %s; revoke only names members of the entity or of an entity it inherits from "+ + "(an association is a member of the entity it goes FROM, unless it is owned by both)", + entityQN, strings.Join(unknown, ", ")) + } + qualify := func(m string) string { return memberRef[m] } for _, right := range s.Rights { switch right.Type { case ast.EntityAccessCreate: @@ -1466,6 +1460,66 @@ func storedAuditMembers(e *domainmodel.Entity) []string { // qualified reference to store in a MemberAccess. type namedAssociation struct{ Name, Ref string } +// ruleAssociation is one association an entity's access rule carries a +// MemberAccess for, with the reference Mendix stores in AssociationQualifiedName. +type ruleAssociation struct { + namedAssociation + inherited bool +} + +// accessRuleAssociations lists the associations whose MemberAccess belongs on +// entity's access rule, in the order GRANT writes them. GRANT builds the rule +// from it and REVOKE resolves member names against it, so the two agree on +// which side carries an association and on its stored reference. +// +// ParentID = FROM entity (FK owner), and for a Default-owner association the +// MemberAccess belongs only on that side — adding it to the TO side triggers +// CE0066. `OWNER Both` is the exception: both ends own the association and +// Mendix expects an entry on each; emitting it only on the FROM side left the +// TO entity's rule incomplete, CE0066 (issuetracker #20, verified against +// mxbuild 11.12.1 — the same script with `OWNER Default` checks clean). +// +// The reference is qualified by the module that DECLARES the association +// ("ModA.Tag_GuestGroup"), never by the entity as an attribute's is. REVOKE +// qualifying an association like an attribute matched no stored entry and +// reported "nothing to revoke" while the rule held ReadWrite (mendixlabs/mxcli#1225). +// +// Associations declared on an ancestor come last, marked inherited. Mendix +// inheritance is multi-table: a specialization has ALL of its generalization's +// members, associations included, and its rule needs an entry for each, as for +// inherited attributes (#758). Leaving them out was CE0066 on the +// specialization's module and made OpenAIConnector's rule inexpressible +// (`OpenAIDeployedModel extends GenAICommons.DeployedModel`, whose +// `DeployedModel_InputModality` is declared on the parent — mxcli-chat FINDINGS +// §26). A caller lets one of the entity's own members shadow an inherited one. +func accessRuleAssociations(ctx *ExecContext, moduleName string, dm *domainmodel.DomainModel, entity *domainmodel.Entity, entityQN string) []ruleAssociation { + var out []ruleAssociation + own := func(name, ref string) { + out = append(out, ruleAssociation{namedAssociation: namedAssociation{Name: name, Ref: ref}}) + } + for _, assoc := range dm.Associations { + ownedHere := assoc.ParentID == entity.ID || + (assoc.Owner == domainmodel.AssociationOwnerBoth && assoc.ChildID == entity.ID) + if ownedHere { + own(assoc.Name, moduleName+"."+assoc.Name) + } + } + for _, ca := range dm.CrossAssociations { + if ca.ParentID == entity.ID { + own(ca.Name, moduleName+"."+ca.Name) + } + } + // A cross-module association owned by both ends is stored in the FROM + // entity's module, so this entity — the TO end — has to look for it there. + for _, other := range otherModuleBothOwnerAssociations(ctx, moduleName, entityQN) { + own(other.Name, other.Ref) + } + for _, inh := range inheritedAssociations(ctx, entityQN) { + out = append(out, ruleAssociation{namedAssociation: inh, inherited: true}) + } + return out +} + // otherModuleBothOwnerAssociations finds cross-module associations owned by BOTH // ends whose TO end is entityQN. They are stored in the FROM entity's module, so // scanning only this entity's own domain model misses them. diff --git a/mdl/executor/revoke_association_member_test.go b/mdl/executor/revoke_association_member_test.go new file mode 100644 index 0000000000..f4d8d0cf56 --- /dev/null +++ b/mdl/executor/revoke_association_member_test.go @@ -0,0 +1,107 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "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/security" +) + +// revokeAssocFixture is mendixlabs/mxcli#1225's model: ModA.Tag owns the +// Reference association Tag_GuestGroup (Tag -> GuestGroup), so the MemberAccess +// for it lives on Tag's rule, stored as "ModA.Tag_GuestGroup" — qualified by the +// module, not by the entity as an attribute is. +func revokeAssocFixture(t *testing.T) (*mock.MockBackend, *[]types.EntityAccessRevocation) { + t.Helper() + mod := mkModule("ModA") + tag := &domainmodel.Entity{ + BaseElement: model.BaseElement{ID: nextID("ent")}, Name: "Tag", Persistable: true, + Attributes: []*domainmodel.Attribute{{BaseElement: model.BaseElement{ID: nextID("attr")}, Name: "Label"}}, + } + group := &domainmodel.Entity{BaseElement: model.BaseElement{ID: nextID("ent")}, Name: "GuestGroup", Persistable: true} + dm := &domainmodel.DomainModel{ + BaseElement: model.BaseElement{ID: nextID("dm")}, + ContainerID: mod.ID, + Entities: []*domainmodel.Entity{tag, group}, + Associations: []*domainmodel.Association{mkAssociation(mod.ID, "Tag_GuestGroup", tag.ID, group.ID)}, + } + ms := &security.ModuleSecurity{ContainerID: mod.ID, ModuleRoles: []*security.ModuleRole{{Name: "User"}}} + var got []types.EntityAccessRevocation + mb := &mock.MockBackend{ + IsConnectedFunc: func() bool { return true }, + ListModulesFunc: func() ([]*model.Module, error) { return []*model.Module{mod}, nil }, + GetModuleByNameFunc: func(string) (*model.Module, error) { return mod, nil }, + GetDomainModelFunc: func(model.ID) (*domainmodel.DomainModel, error) { return dm, nil }, + ListDomainModelsFunc: func() ([]*domainmodel.DomainModel, error) { return []*domainmodel.DomainModel{dm}, nil }, + GetModuleSecurityFunc: func(model.ID) (*security.ModuleSecurity, error) { return ms, nil }, + ListModuleSecurityFunc: func() ([]*security.ModuleSecurity, error) { return []*security.ModuleSecurity{ms}, nil }, + RevokeEntityMemberAccessFunc: func(_ model.ID, _ string, _ []string, rev types.EntityAccessRevocation) (int, error) { + got = append(got, rev) + return 1, nil + }, + } + return mb, &got +} + +func revokeWriteOf(entity string, members ...string) *ast.RevokeEntityAccessStmt { + return &ast.RevokeEntityAccessStmt{ + Entity: ast.QualifiedName{Module: "ModA", Name: entity}, + Roles: []ast.QualifiedName{{Module: "ModA", Name: "User"}}, + Rights: []ast.EntityAccessRight{{Type: ast.EntityAccessWriteMembers, Members: members}}, + } +} + +// `revoke ModA.User on ModA.Tag (write (Tag_GuestGroup))` qualified the +// association like an attribute, "ModA.Tag.Tag_GuestGroup", which matches no +// stored MemberAccess — the revoke said "nothing to revoke", exited 0, and the +// association stayed ReadWrite (mendixlabs/mxcli#1225). +func TestRevokeWriteAssociationMemberUsesStoredRef(t *testing.T) { + mb, got := revokeAssocFixture(t) + ctx, _ := newMockCtx(t, withBackend(mb)) + assertNoError(t, execRevokeEntityAccess(ctx, revokeWriteOf("Tag", "Tag_GuestGroup", "Label"))) + if len(*got) != 1 { + t.Fatalf("backend revoke called %d times, want 1", len(*got)) + } + want := map[string]bool{"ModA.Tag_GuestGroup": true, "ModA.Tag.Label": true} + for _, ref := range (*got)[0].RevokeWriteMembers { + if !want[ref] { + t.Errorf("revoked member ref %q matches no stored MemberAccess", ref) + } + delete(want, ref) + } + for ref := range want { + t.Errorf("member %q was not revoked", ref) + } +} + +// A member the entity's rule cannot carry is a mistake in the script, not an +// already-revoked right: the TO end of a Default-owner association has no +// MemberAccess for it (CLAUDE.md, ParentPointer = FROM), and a typo has none +// either. Mirrors GRANT, which refuses an unmatched member (#758). +func TestRevokeUnknownMemberIsAnError(t *testing.T) { + for name, stmt := range map[string]*ast.RevokeEntityAccessStmt{ + "typo": revokeWriteOf("Tag", "Lable"), + "TO end": revokeWriteOf("GuestGroup", "Tag_GuestGroup"), + "assoc typo": revokeWriteOf("Tag", "Tag_Guestgroup"), + "mixed known": revokeWriteOf("Tag", "Label", "Nope"), + } { + t.Run(name, func(t *testing.T) { + mb, got := revokeAssocFixture(t) + ctx, _ := newMockCtx(t, withBackend(mb)) + err := execRevokeEntityAccess(ctx, stmt) + if err == nil || !strings.Contains(err.Error(), "has no member") { + t.Fatalf("err = %v, want a 'has no member' error", err) + } + if len(*got) != 0 { + t.Errorf("rule was modified despite the error") + } + }) + } +} From f53041b4e256ad3285f3b57d5230311d66861280 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 13 Oct 2026 09:39:21 +0000 Subject: [PATCH 17/17] fix: roll back exec when a statement fails after the pre-check (mendixlabs/mxcli#1408) Reported: "`exec` applies statements one at a time and cannot roll back." A statement that failed at write time left every earlier statement written - a created module and entity stayed, and a `drop` ahead of the failing `create` was not undone. exec is now all or nothing. A before-image journal (modelsdk/mpr/ journal.go) copies the .mpr (and any -wal/-journal) at the first write, from guardWrite, the choke point every write passes before it changes anything, and each mprcontents unit file the first time the run writes, replaces or deletes it. On failure the executor is closed (deferred writes land at Close) and the copies are put back, removing created files and directories. The write path itself is unchanged, so canon.Reconcile, write elision and identity carry behave exactly as before. Works for MPR v1 and v2. Not covered: --continue-on-error (deliberately per-statement), the REPL and -c, and generated .java/.js source beside the model. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01W3x8Fyvir8kLyPgJs1BDmF --- ...-10-12-exec-not-atomic-after-precheck.json | 1 + .claude/skills/mendix/check-syntax/SKILL.md | 30 +- cmd/mxcli/cmd_exec.go | 63 ++++- cmd/mxcli/exec_atomic_test.go | 177 ++++++++++++ cmd/mxcli/exec_preflight.go | 3 +- modelsdk/mpr/journal.go | 256 ++++++++++++++++++ modelsdk/mpr/studiopro_lock.go | 9 +- modelsdk/mpr/writer_core.go | 16 +- 8 files changed, 527 insertions(+), 28 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/cmd-mxcli/2026-10-12-exec-not-atomic-after-precheck.json create mode 100644 cmd/mxcli/exec_atomic_test.go create mode 100644 modelsdk/mpr/journal.go diff --git a/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-12-exec-not-atomic-after-precheck.json b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-12-exec-not-atomic-after-precheck.json new file mode 100644 index 0000000000..b7fbd9f414 --- /dev/null +++ b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-12-exec-not-atomic-after-precheck.json @@ -0,0 +1 @@ +{"area":"cmd/mxcli","date":"2026-10-12","symptom":"`mxcli exec`: a statement that fails at write time (after the pre-check passed) leaves every earlier statement written — a created module and entity stay, and a `drop` ahead of the failing `create` is not undone","cause":"exec applied statements one at a time, each landing on disk as it ran, with no way back; the only all-or-nothing gate was the pre-check, which cannot predict write-time failures (a property the project's Mendix version cannot store, a widget that does not build)","file":"`modelsdk/mpr/journal.go` (`BeginJournal`, `journalDB` via `guardWrite`, `journalFile` at the four mprcontents write/delete sites in `writer_core.go`); `cmd/mxcli/cmd_exec.go` (`runScriptAtomically`)","insight":"A before-image journal, not a write buffer: buffering would have to make inserts and deletes visible to later reads in the run (deferral only holds updates, writer_deferred.go), while journaling leaves the write path — reconcile, elision, identity carry — exactly as it was and only copies each file before the run first touches it. `guardWrite` is already the choke point every write passes before it changes anything, so the .mpr copy is taken there. Rollback must run after the executor is closed: a deferred access-rule write lands at Close, and the .mpr is replaced under SQLite. Test hashes the whole project tree INCLUDING directories (a created unit's XX/YY dirs must go too) on both a v1 and a v2 fixture; control: the same script minus the failing statement must change the tree","refs":["mendixlabs/mxcli#1408"]} diff --git a/.claude/skills/mendix/check-syntax/SKILL.md b/.claude/skills/mendix/check-syntax/SKILL.md index 7b20e4474c..60a1590abc 100644 --- a/.claude/skills/mendix/check-syntax/SKILL.md +++ b/.claude/skills/mendix/check-syntax/SKILL.md @@ -11,11 +11,11 @@ description: "Validate MDL with `mxcli check` before presenting or executing it, | lines | section | |---|---| | 29-35 | When to Use This Skill | -| 36-287 | `exec` refuses what `check` rejects | -| 288-423 | Pre-Flight Validation Checklist | -| 424-474 | Two rules that only real validation used to catch | -| 475-521 | Validation Workflow | -| 522-542 | Script Execution Behavior | +| 36-286 | `exec` refuses what `check` rejects | +| 287-422 | Pre-Flight Validation Checklist | +| 423-473 | Two rules that only real validation used to catch | +| 474-520 | Validation Workflow | +| 521-542 | Script Execution Behavior | | 543-603 | Script Organization | | 604-656 | Troubleshooting Parse Errors | | 657-714 | Studio Pro MCP — Verification Only | @@ -36,10 +36,9 @@ This skill ensures MDL scripts are validated before presenting them to users or ## `exec` refuses what `check` rejects `mxcli exec` runs the same semantic checks before writing anything. A script whose -checks report an **error** is not executed at all — nothing is written — because -`exec` applies statements one at a time and cannot roll back, so a known-bad -script would leave the model partly updated. Warnings are printed and do not stop -the run. +checks report an **error** is not executed at all — nothing is written. Warnings +are printed and do not stop the run. A statement that still fails while it is +written is rolled back with the rest (see Script Execution Behavior). ```bash mxcli exec script.mdl -p app.mpr # checked, then applied @@ -521,18 +520,19 @@ not close it. ## Script Execution Behavior -**IMPORTANT: Script execution is atomic per statement, NOT per script.** - -When a script fails on statement N, statements 1 through N-1 have already been committed: +**`mxcli exec` is atomic per script.** When a script fails on statement N, +statements 1 through N-1 are rolled back and the model is left as it was: ``` -Statement 1: create module ✓ (committed) -Statement 2: create entity ✓ (committed) -Statement 3: create association ✓ (committed) +Statement 1: create module ✓ (rolled back) +Statement 2: create entity ✓ (rolled back) +Statement 3: create association ✓ (rolled back) Statement 4: create view entity ✗ (failed - execution stops here) Statement 5: create page (never executed) ``` +Not restored: generated `.java`/`.js`. Not atomic: `--continue-on-error`, REPL, `-c`. + **Recommendations:** 1. Split scripts into phases when experimenting with uncertain syntax 2. Use `create or modify` to make scripts idempotent diff --git a/cmd/mxcli/cmd_exec.go b/cmd/mxcli/cmd_exec.go index 5baddbe58c..340a17a5a5 100644 --- a/cmd/mxcli/cmd_exec.go +++ b/cmd/mxcli/cmd_exec.go @@ -8,6 +8,7 @@ import ( "os" "path/filepath" + "github.com/mendixlabs/mxcli/mdl/ast" "github.com/mendixlabs/mxcli/mdl/executor" "github.com/mendixlabs/mxcli/mdl/visitor" mmpr "github.com/mendixlabs/mxcli/modelsdk/mpr" @@ -20,9 +21,8 @@ var execCmd = &cobra.Command{ Long: `Execute an MDL script file containing MDL commands. Before anything is written, the script is put through the same semantic checks -as "mxcli check". If any of them reports an error, nothing is executed: exec -applies statements one at a time and cannot roll back, so running a script with -a known error leaves the model partly updated. Warnings are printed and do not +as "mxcli check". If any of them reports an error, nothing is executed. +Warnings are printed and do not stop the run; info notes are counted on one line (--verbose prints them, as "mxcli check" does). Use --no-check to apply a script anyway. @@ -43,6 +43,12 @@ writes through it with --mcp. --force writes anyway (for a lock left behind by a crash); MXCLI_ALLOW_STUDIO_PRO_OPEN=1 does the same for every command. Reads, and re-running a script whose statements change nothing, are never refused. +A script is applied all or nothing: if a statement still fails while it is +written (after the checks passed), the writes of the statements before it are +rolled back and the project is left as it was. Only the model (.mpr and +mprcontents/) is restored; generated .java/.js source is not. With +--continue-on-error each statement stands on its own and nothing is rolled back. + Pass "-" as the file to read the script from standard input, so MDL can be piped or written inline as a heredoc without a temporary file. @@ -139,16 +145,59 @@ Example: return } - if err := exec.ExecuteProgram(prog); err != nil { - if errors.Is(err, executor.ErrExit) { - return - } + if err := runScriptAtomically(exec, prog); err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) os.Exit(1) } }, } +// runScriptAtomically executes prog so that it lands whole or not at all: when +// a statement fails, every write the earlier ones made is undone and the +// project is left as it was (mendixlabs/mxcli#1408). The pre-check refuses a +// script with a known error, but a statement can still fail at write time — a +// property the project's Mendix version cannot store, a widget that does not +// build — and before this the statements ahead of it stayed written. +// +// The executor is closed here, success or not: a deferred write lands at +// Close, so the run is not over until then, and the .mpr can only be put back +// once nothing holds it open. An `exit` statement is a successful stop. +func runScriptAtomically(exec *executor.Executor, prog *ast.Program) error { + journal, jerr := mmpr.BeginJournal() + if jerr != nil { + fmt.Fprintf(os.Stderr, "Warning: %v; a failing statement will leave the earlier ones written\n", jerr) + if err := exec.ExecuteProgram(prog); err != nil && !errors.Is(err, executor.ErrExit) { + return err + } + return nil + } + stoppedAt, err := exec.ExecuteProgramReportingStop(prog) + if errors.Is(err, executor.ErrExit) { + err = nil + } + if closeErr := exec.Close(); closeErr != nil && err == nil { + err = closeErr + } + if err == nil || !journal.Touched() { + journal.Commit() + return err + } + if rerr := journal.Rollback(); rerr != nil { + return errors.Join(err, fmt.Errorf("could not roll back the statements that ran before it, "+ + "so the project may be partly updated: %w", rerr)) + } + ran := "the script's statements" + switch { + case stoppedAt == 0: + ran = "the failing statement's partial writes" + case stoppedAt == 1: + ran = "statement 1" + case stoppedAt > 1: + ran = fmt.Sprintf("statements 1-%d", stoppedAt) + } + return fmt.Errorf("%w\nNothing was written: %s were rolled back, and the project is as it was before exec", err, ran) +} + func init() { execCmd.Flags().Bool("no-check", false, "Skip the pre-flight semantic checks and apply the script even if mxcli check would report errors") diff --git a/cmd/mxcli/exec_atomic_test.go b/cmd/mxcli/exec_atomic_test.go new file mode 100644 index 0000000000..2ec580af97 --- /dev/null +++ b/cmd/mxcli/exec_atomic_test.go @@ -0,0 +1,177 @@ +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "bytes" + "crypto/sha256" + "encoding/hex" + "io" + "io/fs" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/executor" + "github.com/mendixlabs/mxcli/mdl/visitor" +) + +// mendixlabs/mxcli#1408: "Exec is not atomic after the pre-check passes". A +// statement that fails at write time left every statement before it written — a +// created module, and a dropped document the failing statement was meant to +// replace. exec is all or nothing now: the project must come back byte-identical. + +type atomicFixture struct { + name, src, mpr, drop string // drop: a document the fixture already holds +} + +var atomicFixtures = []atomicFixture{ + {"v1", "../../modelsdk/mpr/testdata/v1-project", "App.mpr", ""}, + {"v2", "../../testdata/testapp-views", "TestApp.mpr", "drop entity MyFirstModule.Car;\ndrop microflow MyFirstModule.MyFirstLogic;\n"}, +} + +func TestExecRollsBackWhenAStatementFails(t *testing.T) { + for _, fx := range atomicFixtures { + t.Run(fx.name, func(t *testing.T) { + dir := copyFixtureTree(t, fx.src) + before := hashTree(t, dir) + + script := "create module Atom1408;\n" + + "create persistent entity Atom1408.Thing (Name: String(100));\n" + + fx.drop + + "drop entity Atom1408.DoesNotExist;\n" + err := runScriptOn(t, filepath.Join(dir, fx.mpr), script) + if err == nil { + t.Fatal("the script's last statement fails; exec reported success") + } + if !strings.Contains(err.Error(), "Nothing was written") { + t.Errorf("error does not say the writes were rolled back: %v", err) + } + if diff := diffTrees(before, hashTree(t, dir)); diff != "" { + t.Errorf("a failed exec left the project changed:\n%s", diff) + } + }) + } +} + +// THE CONTROL: the same script without the failing statement is written, so the +// test above is not passing because nothing ever reaches disk. +func TestExecWritesAScriptThatSucceeds(t *testing.T) { + for _, fx := range atomicFixtures { + t.Run(fx.name, func(t *testing.T) { + dir := copyFixtureTree(t, fx.src) + before := hashTree(t, dir) + + script := "create module Atom1408;\n" + + "create persistent entity Atom1408.Thing (Name: String(100));\n" + fx.drop + if err := runScriptOn(t, filepath.Join(dir, fx.mpr), script); err != nil { + t.Fatalf("exec: %v", err) + } + if diffTrees(before, hashTree(t, dir)) == "" { + t.Fatal("a successful exec wrote nothing") + } + var out bytes.Buffer + exec := executor.New(&out) + exec.SetBackendFactory(newBackendFactory()) + defer exec.Close() + for _, cmd := range []string{ + "connect local '" + visitor.QuoteString(filepath.Join(dir, fx.mpr)) + "';", + "show entities in Atom1408;", + } { + prog, _ := visitor.Build(cmd) + if err := exec.ExecuteProgram(prog); err != nil { + t.Fatalf("%s: %v", cmd, err) + } + } + if !strings.Contains(out.String(), "Atom1408.Thing") { + t.Errorf("the created entity is not in the project:\n%s", out.String()) + } + }) + } +} + +func runScriptOn(t *testing.T, mprPath, script string) error { + t.Helper() + exec := executor.New(io.Discard) + exec.SetBackendFactory(newBackendFactory()) + defer exec.Close() + conn, _ := visitor.Build("connect local '" + visitor.QuoteString(mprPath) + "';") + if err := exec.ExecuteProgram(conn); err != nil { + t.Fatalf("connect: %v", err) + } + prog, errs := visitor.Build(script) + if len(errs) > 0 { + t.Fatalf("parse: %v", errs) + } + return runScriptAtomically(exec, prog) +} + +func copyFixtureTree(t *testing.T, src string) string { + t.Helper() + dst := t.TempDir() + err := filepath.WalkDir(src, func(p string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + rel, _ := filepath.Rel(src, p) + target := filepath.Join(dst, rel) + if d.IsDir() { + return os.MkdirAll(target, 0o755) + } + data, err := os.ReadFile(p) + if err != nil { + return err + } + return os.WriteFile(target, data, 0o644) + }) + if err != nil { + t.Fatal(err) + } + return dst +} + +func hashTree(t *testing.T, dir string) map[string]string { + t.Helper() + out := map[string]string{} + err := filepath.WalkDir(dir, func(p string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + rel, _ := filepath.Rel(dir, p) + if d.IsDir() { + // A directory a created unit needed must go when it is rolled back. + out[rel+"/"] = "dir" + return nil + } + data, err := os.ReadFile(p) + if err != nil { + return err + } + sum := sha256.Sum256(data) + out[rel] = hex.EncodeToString(sum[:]) + return nil + }) + if err != nil { + t.Fatal(err) + } + return out +} + +func diffTrees(before, after map[string]string) string { + var b strings.Builder + for p, h := range before { + switch a, ok := after[p]; { + case !ok: + b.WriteString(" deleted " + p + "\n") + case a != h: + b.WriteString(" modified " + p + "\n") + } + } + for p := range after { + if _, ok := before[p]; !ok { + b.WriteString(" created " + p + "\n") + } + } + return b.String() +} diff --git a/cmd/mxcli/exec_preflight.go b/cmd/mxcli/exec_preflight.go index 3d25b9fb10..68dfda7189 100644 --- a/cmd/mxcli/exec_preflight.go +++ b/cmd/mxcli/exec_preflight.go @@ -49,8 +49,7 @@ func execPreflight(exec *executor.Executor, prog *ast.Program, projectPath, scri if summary := linter.Summarize(violations); summary.Errors > 0 { return fmt.Sprintf( "\nRefusing to execute: %d error(s) above. Nothing was written.\n"+ - " exec applies statements one at a time and cannot roll back, so a script\n"+ - " with a known error would leave the model partly updated.\n"+ + " A script with a known error would fail part-way and be rolled back.\n"+ " Fix them, or re-run with --no-check to apply the script anyway.\n", summary.Errors) } diff --git a/modelsdk/mpr/journal.go b/modelsdk/mpr/journal.go new file mode 100644 index 0000000000..d3f55218a9 --- /dev/null +++ b/modelsdk/mpr/journal.go @@ -0,0 +1,256 @@ +// SPDX-License-Identifier: Apache-2.0 + +package mpr + +import ( + "errors" + "fmt" + "io" + "os" + "path/filepath" + "strconv" + "sync" +) + +// Journal makes a run of writes all-or-nothing: what every Writer in the process +// changes on disk while it is open can be put back with Rollback +// (mendixlabs/mxcli#1408). +// +// # Why +// +// `mxcli exec` applies a script one statement at a time, each written as it +// runs. A statement that fails at write time — after the pre-check passed — left +// every earlier statement on disk: a created module, a dropped document that the +// failing create was meant to replace. The model is valid, so nothing downstream +// notices that it is half a script. +// +// # How +// +// It is a before-image journal, not a write buffer. Writes still land as they +// always have — reconciled against what is stored, elided when they change +// nothing, carrying stored identities (ADR-0008) — so every later statement reads +// the project the earlier ones produced, and the identity rules see exactly what +// they see without a journal. What the journal adds is a copy of each file as it +// was before the run first touched it: +// +// - the SQLite .mpr (with any -wal / -journal beside it), copied at the first +// write to that project. Every write reaches storage through guardWrite +// first, so the copy always predates the first change. +// - each mprcontents/ unit file (MPR v2), copied, or noted as absent, the first +// time the run writes, replaces or deletes it. +// +// Rollback puts the copies back and removes what the run created, which leaves +// the project byte-identical to where the run began. It must run after every +// connection to the project is closed: the .mpr is replaced underneath SQLite. +// +// Only the model is journaled. Files a statement writes beside it — the +// generated .java / .js source of a code action — are not. +type Journal struct { + mu sync.Mutex + dir string // holds the before-images + dbs map[string][]dbBackup // keyed by absolute .mpr path + files map[string]string // unit file -> its copy, "" when it did not exist + dirs []string // directories the run created, innermost last + n int + tainted error // a copy that failed; Rollback cannot be trusted past it +} + +type dbBackup struct { + path string // the live file: x.mpr, x.mpr-wal, x.mpr-journal + copy string // "" when the file did not exist at the snapshot +} + +var ( + journalMu sync.Mutex + activeJournal *Journal +) + +// BeginJournal opens a journal for every write this process makes until Commit +// or Rollback. Only one can be open. +func BeginJournal() (*Journal, error) { + journalMu.Lock() + defer journalMu.Unlock() + if activeJournal != nil { + return nil, errors.New("a write journal is already open") + } + dir, err := os.MkdirTemp("", "mxcli-journal-") + if err != nil { + return nil, fmt.Errorf("open write journal: %w", err) + } + activeJournal = &Journal{dir: dir, dbs: map[string][]dbBackup{}, files: map[string]string{}} + return activeJournal, nil +} + +func currentJournal() *Journal { + journalMu.Lock() + defer journalMu.Unlock() + return activeJournal +} + +func (j *Journal) close() { + journalMu.Lock() + if activeJournal == j { + activeJournal = nil + } + journalMu.Unlock() + _ = os.RemoveAll(j.dir) +} + +// Touched reports whether the run wrote anything that Rollback would restore. +func (j *Journal) Touched() bool { + j.mu.Lock() + defer j.mu.Unlock() + return len(j.dbs) > 0 || len(j.files) > 0 +} + +// Commit keeps what the run wrote and discards the before-images. +func (j *Journal) Commit() { j.close() } + +// Rollback restores every journaled file to its state before the run. Every +// connection to the journaled projects must be closed first. +func (j *Journal) Rollback() error { + defer j.close() + j.mu.Lock() + defer j.mu.Unlock() + var errs []error + if j.tainted != nil { + errs = append(errs, fmt.Errorf("write journal incomplete: %w", j.tainted)) + } + for path, copy := range j.files { + if err := restoreFile(path, copy); err != nil { + errs = append(errs, err) + } + } + for i := len(j.dirs) - 1; i >= 0; i-- { + _ = os.Remove(j.dirs[i]) // only succeeds when empty, as deleteUnit does + } + for _, backups := range j.dbs { + for _, b := range backups { + if err := restoreFile(b.path, b.copy); err != nil { + errs = append(errs, err) + } + } + // The shared-memory index describes the WAL just replaced; SQLite + // rebuilds it on the next open. + _ = os.Remove(backups[0].path + "-shm") + } + return errors.Join(errs...) +} + +// journalDB snapshots a project's database before its first write in the run. +func journalDB(mprPath string) error { + j := currentJournal() + if j == nil { + return nil + } + abs, err := filepath.Abs(mprPath) + if err != nil { + abs = mprPath + } + j.mu.Lock() + defer j.mu.Unlock() + if _, done := j.dbs[abs]; done { + return nil + } + var backups []dbBackup + for _, p := range []string{abs, abs + "-wal", abs + "-journal"} { + c, err := j.backup(p) + if err != nil { + j.tainted = err + return fmt.Errorf("write journal: %w", err) + } + backups = append(backups, dbBackup{path: p, copy: c}) + } + j.dbs[abs] = backups + return nil +} + +// journalFile records a unit file's state before the run first changes it. +func journalFile(path string) error { + j := currentJournal() + if j == nil { + return nil + } + j.mu.Lock() + defer j.mu.Unlock() + if _, done := j.files[path]; done { + return nil + } + // Directories the write is about to create, outermost first. + var missing []string + for d := filepath.Dir(path); ; d = filepath.Dir(d) { + if _, err := os.Stat(d); err == nil || d == filepath.Dir(d) { + break + } + missing = append([]string{d}, missing...) + } + c, err := j.backup(path) + if err != nil { + j.tainted = err + return fmt.Errorf("write journal: %w", err) + } + j.files[path] = c + j.dirs = append(j.dirs, missing...) + return nil +} + +// backup copies path into the journal and returns the copy, or "" when path +// does not exist. +func (j *Journal) backup(path string) (string, error) { + src, err := os.Open(path) + if errors.Is(err, os.ErrNotExist) { + return "", nil + } + if err != nil { + return "", err + } + defer src.Close() + j.n++ + dst := filepath.Join(j.dir, strconv.Itoa(j.n)) + if err := copyTo(dst, src); err != nil { + return "", fmt.Errorf("copy %s: %w", path, err) + } + return dst, nil +} + +// restoreFile puts a before-image back, or removes a file the run created. The +// copy is written beside the target and renamed over it, so a failure part-way +// leaves the file the run wrote rather than a truncated one. +func restoreFile(path, copy string) error { + if copy == "" { + if err := os.Remove(path); err != nil && !errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("roll back %s: %w", path, err) + } + return nil + } + src, err := os.Open(copy) + if err != nil { + return fmt.Errorf("roll back %s: %w", path, err) + } + defer src.Close() + if err := os.MkdirAll(filepath.Dir(path), 0755); err != nil { + return fmt.Errorf("roll back %s: %w", path, err) + } + tmp := path + ".rollback" + if err := copyTo(tmp, src); err != nil { + _ = os.Remove(tmp) + return fmt.Errorf("roll back %s: %w", path, err) + } + if err := os.Rename(tmp, path); err != nil { + _ = os.Remove(tmp) + return fmt.Errorf("roll back %s: %w", path, err) + } + return nil +} + +func copyTo(dst string, src io.Reader) error { + f, err := os.OpenFile(dst, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0644) + if err != nil { + return err + } + if _, err := io.Copy(f, src); err != nil { + f.Close() + return err + } + return f.Close() +} diff --git a/modelsdk/mpr/studiopro_lock.go b/modelsdk/mpr/studiopro_lock.go index 7e4f12ebc2..27a27f37a4 100644 --- a/modelsdk/mpr/studiopro_lock.go +++ b/modelsdk/mpr/studiopro_lock.go @@ -93,7 +93,12 @@ func studioProOpenGuard(mprPath string) error { return &StudioProOpenError{MprPath: mprPath, LockPath: lock, LockTime: mod} } -// guardWrite applies the guard to this writer's project. +// guardWrite applies the guard to this writer's project. Every write passes +// here before it changes anything, which is also what lets an open Journal copy +// the .mpr before the first change (journal.go). func (w *Writer) guardWrite() error { - return studioProOpenGuard(w.reader.path) + if err := studioProOpenGuard(w.reader.path); err != nil { + return err + } + return journalDB(w.reader.path) } diff --git a/modelsdk/mpr/writer_core.go b/modelsdk/mpr/writer_core.go index eb19160211..9b2640b8ef 100644 --- a/modelsdk/mpr/writer_core.go +++ b/modelsdk/mpr/writer_core.go @@ -241,12 +241,15 @@ func (wt *WriteTransaction) WriteUnit(unitID string, contents []byte) error { // Create directory if needed dir := filepath.Join(wt.writer.reader.contentsDir, swappedUUID[0:2], swappedUUID[2:4]) + finalPath := filepath.Join(dir, swappedUUID+".mxunit") + if err := journalFile(finalPath); err != nil { + return err + } if err := os.MkdirAll(dir, 0755); err != nil { return fmt.Errorf("failed to create directory: %w", err) } // Write to temp file first - finalPath := filepath.Join(dir, swappedUUID+".mxunit") tempPath := finalPath + ".tmp" if err := os.WriteFile(tempPath, contents, 0644); err != nil { @@ -569,12 +572,15 @@ func (w *Writer) insertUnit(unitID, containerID, containmentName, unitType strin // Create directory structure: mprcontents/XX/YY/ dir := filepath.Join(w.reader.contentsDir, swappedUUID[0:2], swappedUUID[2:4]) + filePath := filepath.Join(dir, swappedUUID+".mxunit") + if err := journalFile(filePath); err != nil { + return err + } if err := os.MkdirAll(dir, 0755); err != nil { return fmt.Errorf("failed to create directory %s: %w", dir, err) } // Write content file - filePath := filepath.Join(dir, swappedUUID+".mxunit") if err := os.WriteFile(filePath, contents, 0644); err != nil { return fmt.Errorf("failed to write unit file: %w", err) } @@ -680,6 +686,9 @@ func (w *Writer) updateUnit(unitID string, contents []byte, opts ...canon.Option // Direct WriteFile would overwrite the shared inode when the target // is a hard link (e.g. in test fixtures), corrupting the source file. // Rename replaces the directory entry, leaving the linked inode intact. + if err := journalFile(filePath); err != nil { + return err + } tmpPath := filePath + ".tmp" if err := os.WriteFile(tmpPath, contents, 0644); err != nil { return fmt.Errorf("failed to write unit temp file: %w", err) @@ -955,6 +964,9 @@ func (w *Writer) deleteUnit(unitID string) error { subDir1 := swappedUUID[0:2] subDir2 := swappedUUID[2:4] filePath := filepath.Join(w.reader.contentsDir, subDir1, subDir2, swappedUUID+".mxunit") + if err := journalFile(filePath); err != nil { + return err + } os.Remove(filePath) // Ignore error if file doesn't exist // Clean up empty parent directories (YY/, then XX/)