From 3887aead06e366e61b6c4c2e88e70dca41991d88 Mon Sep 17 00:00:00 2001 From: Ako Date: Sun, 27 Sep 2026 13:37:52 +0000 Subject: [PATCH 1/7] roundtrip: shard the upgrade property test with MXCLI_UPGRADE_SHARD Round-robin over the scripts that reach execution; every shard still upgrades and checks every script. TestShardsPartition proves the shards together execute each script exactly once. Locally 75+74+75 = 224, the unsharded count. Part of #757. Co-Authored-By: Claude Opus 5.5 --- mdl/roundtrip/shard_test.go | 65 ++++++++++++++++++++++++++ mdl/roundtrip/upgrade_property_test.go | 33 +++++++++++-- mdl/roundtrip/upgrade_shard_test.go | 35 ++++++++++++++ 3 files changed, 128 insertions(+), 5 deletions(-) create mode 100644 mdl/roundtrip/shard_test.go create mode 100644 mdl/roundtrip/upgrade_shard_test.go diff --git a/mdl/roundtrip/shard_test.go b/mdl/roundtrip/shard_test.go new file mode 100644 index 000000000..4d18c3487 --- /dev/null +++ b/mdl/roundtrip/shard_test.go @@ -0,0 +1,65 @@ +// SPDX-License-Identifier: Apache-2.0 + +package roundtrip + +import ( + "fmt" + "testing" +) + +// TestParseShard pins the MXCLI_UPGRADE_SHARD spelling CI passes. +func TestParseShard(t *testing.T) { + for _, tc := range []struct { + in string + i, n int + bad bool + }{ + {in: "", i: 1, n: 1}, + {in: "1/1", i: 1, n: 1}, + {in: "2/3", i: 2, n: 3}, + {in: " 3/3 ", i: 3, n: 3}, + {in: "0/3", bad: true}, + {in: "4/3", bad: true}, + {in: "1/0", bad: true}, + {in: "3", bad: true}, + {in: "a/b", bad: true}, + {in: "1/2/3", bad: true}, + } { + s, err := parseShard(tc.in) + if tc.bad { + if err == nil { + t.Errorf("parseShard(%q) = %+v, want an error", tc.in, s) + } + continue + } + if err != nil || s.index != tc.i || s.count != tc.n { + t.Errorf("parseShard(%q) = %+v, %v; want %d/%d", tc.in, s, err, tc.i, tc.n) + } + } +} + +// TestShardsPartition: CI runs the shards as separate jobs and never the +// unsharded test, so the shards together must execute every script exactly +// once. A script in no shard would silently leave per-PR CI. +func TestShardsPartition(t *testing.T) { + for n := 1; n <= 7; n++ { + const items = 101 + seen := make([]int, items) + for i := 1; i <= n; i++ { + s, err := parseShard(fmt.Sprintf("%d/%d", i, n)) + if err != nil { + t.Fatal(err) + } + for k := 0; k < items; k++ { + if s.has(k) { + seen[k]++ + } + } + } + for k, c := range seen { + if c != 1 { + t.Fatalf("n=%d: item %d is in %d shards, want exactly 1", n, k, c) + } + } + } +} diff --git a/mdl/roundtrip/upgrade_property_test.go b/mdl/roundtrip/upgrade_property_test.go index 0ef29a964..fe508c0fc 100644 --- a/mdl/roundtrip/upgrade_property_test.go +++ b/mdl/roundtrip/upgrade_property_test.go @@ -56,7 +56,12 @@ import ( // Executing the whole corpus takes about 15 minutes, which on its own exceeds // what the CI integration step has left (ako/mxcli#742 timed out there). // MXCLI_UPGRADE_ALL=1 executes every script, header-only ones included; run it -// when a langver.Change or a gated rewrite lands. +// when a langver.Change or a gated rewrite lands. The nightly workflow runs it. +// +// MXCLI_UPGRADE_SHARD=i/n executes only every n-th executable script, starting +// at the i-th, so per-PR CI can split the executions across parallel jobs +// (ako/mxcli#757); the shards together execute each script exactly once +// (TestShardsPartition). Unset is the whole set. func TestUpgradeExecutesToTheSameModel(t *testing.T) { a, b := newHarness(t), newHarness(t) defer a.close() @@ -64,7 +69,12 @@ func TestUpgradeExecutesToTheSameModel(t *testing.T) { scripts := upgradeExampleScripts(t) all := os.Getenv("MXCLI_UPGRADE_ALL") != "" - var same, outOfScope, unparsed, headerOnly, keptVersion []string + sh, err := parseShard(os.Getenv("MXCLI_UPGRADE_SHARD")) + if err != nil { + t.Fatal(err) + } + executable := 0 // scripts that reached execution, in every shard + var same, outOfScope, unparsed, headerOnly, keptVersion, otherShard []string for _, path := range scripts { src, err := os.ReadFile(path) if err != nil { @@ -100,6 +110,14 @@ func TestUpgradeExecutesToTheSameModel(t *testing.T) { headerOnly = append(headerOnly, rel) return } + // Every shard upgrades and checks every script above, which is + // cheap; only the execution below, which is not, is split. + k := executable + executable++ + if !sh.has(k) { + otherShard = append(otherShard, rel) + return + } errA, errB, diff := executeBoth(t, a, b, string(src), res.Source) if len(diff) > 0 { @@ -120,6 +138,10 @@ func TestUpgradeExecutesToTheSameModel(t *testing.T) { } t.Logf("%d scripts execute on PedApp and upgrade to the same model", len(same)) + if sh.count > 1 { + t.Logf("shard %s: %d of %d executable scripts belong to other shards and were not executed here", + sh, len(otherShard), executable) + } t.Logf("%d scripts are out of scope: their original does not execute cleanly on PedApp:\n %s", len(outOfScope), strings.Join(outOfScope, "\n ")) if !all { @@ -130,10 +152,11 @@ func TestUpgradeExecutesToTheSameModel(t *testing.T) { len(keptVersion), strings.Join(keptVersion, "\n ")) t.Logf("%d scripts do not parse (negative tests) and cannot be upgraded:\n %s", len(unparsed), strings.Join(unparsed, "\n ")) - if os.Getenv("MXCLI_UPGRADE_EXAMPLES") == "" && len(same) < 50 { + if floor := 50 / sh.count; os.Getenv("MXCLI_UPGRADE_EXAMPLES") == "" && len(same) < floor { // Hundreds execute today; a handful means the harness broke, and a - // property checked on nothing passes. - t.Errorf("only %d scripts executed cleanly — the harness is not exercising the property", len(same)) + // property checked on nothing passes. A shard sees 1/n of them. + t.Errorf("only %d scripts executed cleanly (shard %s) — the harness is not exercising the property", + len(same), sh) } } diff --git a/mdl/roundtrip/upgrade_shard_test.go b/mdl/roundtrip/upgrade_shard_test.go new file mode 100644 index 000000000..69778d2d0 --- /dev/null +++ b/mdl/roundtrip/upgrade_shard_test.go @@ -0,0 +1,35 @@ +// SPDX-License-Identifier: Apache-2.0 + +package roundtrip + +import ( + "fmt" + "strconv" + "strings" +) + +// shard selects every count-th item starting at index-1: MXCLI_UPGRADE_SHARD +// ("i/n", 1-based) splits the upgrade property test's executions across CI +// jobs (ako/mxcli#757). Round-robin rather than contiguous ranges, because +// the corpus is sorted by path and neighbouring scripts cost alike. +type shard struct{ index, count int } + +// parseShard reads "i/n"; empty is the whole set, 1/1. +func parseShard(s string) (shard, error) { + s = strings.TrimSpace(s) + if s == "" { + return shard{1, 1}, nil + } + a, b, ok := strings.Cut(s, "/") + i, errI := strconv.Atoi(a) + n, errN := strconv.Atoi(b) + if !ok || errI != nil || errN != nil || n < 1 || i < 1 || i > n { + return shard{}, fmt.Errorf("MXCLI_UPGRADE_SHARD=%q: want i/n with 1 <= i <= n", s) + } + return shard{i, n}, nil +} + +// has reports whether the k-th item (0-based) belongs to this shard. +func (s shard) has(k int) bool { return k%s.count == s.index-1 } + +func (s shard) String() string { return fmt.Sprintf("%d/%d", s.index, s.count) } From 4cca3ad14859f5ef92299c009fda133ec89b8c9c Mon Sep 17 00:00:00 2001 From: Ako Date: Sun, 27 Sep 2026 13:37:52 +0000 Subject: [PATCH 2/7] ci: split integration tests into parallel suites; full upgrade corpus nightly The single make test-integration step took ~22 of its 30 minutes, its wall time being mdl/roundtrip alone. Per-PR CI now runs executor, roundtrip, upgrade (3 shards) and other as parallel jobs, with an integration-passed aggregate. Nightly raises its step to 60 min and runs MXCLI_UPGRADE_ALL in 3 shards once. Closes #757. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/nightly.yml | 44 ++++++++++++++- .github/workflows/push-test.yml | 97 ++++++++++++++++++++++++++++++--- Makefile | 44 ++++++++++++++- 3 files changed, 174 insertions(+), 11 deletions(-) diff --git a/.github/workflows/nightly.yml b/.github/workflows/nightly.yml index 14a26cb78..d30612c0c 100644 --- a/.github/workflows/nightly.yml +++ b/.github/workflows/nightly.yml @@ -54,9 +54,51 @@ jobs: - name: Setup mxbuild ${{ matrix.mendix-version }} run: ./bin/mxcli setup mxbuild --version ${{ matrix.mendix-version }} + # One step, every package: go test runs the packages in parallel, so the + # wall time is the slowest one's (mdl/roundtrip, ~21 min on this runner in + # ako/mxcli#757's measurement) and grows as the round trip gains fixtures. + # Per-PR CI splits this into parallel jobs instead (push-test.yml). - name: "Integration tests (Mendix ${{ matrix.mendix-version }})" run: make test-integration - timeout-minutes: 30 + 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 + # proves langver's gating on the rest (ako/mxcli#757). Independent of the + # Mendix version — it runs on the committed PedApp fixture and never calls + # mx — so it runs once, not per matrix entry. It does not gate the release. + upgrade-full-corpus: + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + shard: ['1/3', '2/3', '3/3'] + name: upgrade full corpus (${{ matrix.shard }}) + steps: + - uses: actions/checkout@v7 + - uses: actions/setup-go@v7 + with: + go-version: '1.26.6' + - 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 + - name: Upgrade property test, full corpus (${{ matrix.shard }}) + run: make test-integration-upgrade + env: + MXCLI_UPGRADE_ALL: '1' + MXCLI_UPGRADE_SHARD: ${{ matrix.shard }} + timeout-minutes: 45 nightly: needs: test diff --git a/.github/workflows/push-test.yml b/.github/workflows/push-test.yml index 262c5d23f..1d5f323fe 100644 --- a/.github/workflows/push-test.yml +++ b/.github/workflows/push-test.yml @@ -167,17 +167,98 @@ jobs: run: ./scripts/check-skill-mdl.sh ./bin/mxcli .claude/skills/mendix - name: Check docs-site MDL blocks run: ./scripts/check-skill-mdl.sh ./bin/mxcli docs-site/src - - name: Setup mxbuild - run: ./bin/mxcli setup mxbuild --version 11.12.2 - # One engine since the legacy sdk/mpr backend was deleted - # (docs/plans/2026-09-14-retire-legacy-engine.md), so there is no matrix to - # narrow here any more and MXCLI_TEST_ENGINES is left unset. - - name: Integration tests - run: make test-integration - timeout-minutes: 30 + # The integration tests are the `integration` job below. - name: Lint Go run: make lint-go - name: Vulnerability scan run: | go install golang.org/x/vuln/cmd/govulncheck@latest govulncheck ./... + + # The integration tests, split into suites that run as parallel jobs + # (ako/mxcli#757). As one `make test-integration` step they took ~22 of its 30 + # minutes once the upgrade property test landed, and the step's wall time was + # mdl/roundtrip's alone (~21 min) — go test already ran the packages in + # parallel, so no timeout below 30 min was going to hold as the round trip + # grows (#743 adds TestApp to it). + # + # Measured per package on the ubuntu runner (run 36319810164) and what each + # suite is expected to take, including ~3 min of checkout/build/mxbuild: + # executor mdl/executor ~15.5 min -> ~19 min + # roundtrip mdl/roundtrip, round-trip laws ~1.7 min -> ~4 min + # upgrade upgrade property test, 3 shards ~19 min -> ~10 min each + # other cmd/mxcli{,/docker,/marketplace} ~5.7 min -> ~8 min + # The full-corpus upgrade run (MXCLI_UPGRADE_ALL) is nightly, not here. + # + # Nothing here is dropped from per-PR CI: the round-trip laws and the + # execute-both upgrade test run at their default scope, the shards together + # executing every script exactly once (TestShardsPartition). Only the unit + # tests of packages WITHOUT integration tests are no longer re-run under the + # tag — `make test` in build-and-test runs those. + integration: + name: integration (${{ matrix.suite }}${{ matrix.shard && format(' {0}', matrix.shard) || '' }}) + runs-on: ubuntu-latest + # Each suite's go test has its own -timeout (Makefile); this is the job's + # backstop, with room for the setup steps. + timeout-minutes: 45 + strategy: + fail-fast: false + matrix: + include: + - suite: executor + mxbuild: true + - suite: roundtrip + - suite: upgrade + shard: 1/3 + - suite: upgrade + shard: 2/3 + - suite: upgrade + shard: 3/3 + - suite: other + mxbuild: true + steps: + # #743 adds TestApp as a git submodule to the round trip; its checkout + # will need `submodules: true` here for the roundtrip suite. + - uses: actions/checkout@v7 + - uses: actions/setup-go@v7 + with: + go-version: '1.26.6' + - 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 + # mdl/roundtrip runs on the committed PedApp fixture and never calls mx. + # One engine since the legacy sdk/mpr backend was deleted + # (docs/plans/2026-09-14-retire-legacy-engine.md), so MXCLI_TEST_ENGINES + # is left unset. + - name: Setup mxbuild + if: matrix.mxbuild + run: ./bin/mxcli setup mxbuild --version 11.12.2 + - name: Integration tests (${{ matrix.suite }}) + run: make test-integration-${{ matrix.suite }} + env: + MXCLI_UPGRADE_SHARD: ${{ matrix.shard }} + timeout-minutes: 30 + + # One stable check name for "every integration suite passed", whatever the + # matrix above becomes — a branch rule can require this instead of each leg. + integration-passed: + if: always() + needs: integration + runs-on: ubuntu-latest + steps: + - name: All integration suites passed + run: | + if [ "${{ needs.integration.result }}" != "success" ]; then + echo "integration: ${{ needs.integration.result }}" + exit 1 + fi diff --git a/Makefile b/Makefile index ba84eece8..8a9792fd1 100644 --- a/Makefile +++ b/Makefile @@ -35,7 +35,7 @@ GO_BUILD_FLAGS = -trimpath # Clean version for VS Code extension (must be valid semver: major.minor.patch) VSCE_VERSION = $(shell echo "$(VERSION)" | sed 's/^v//; s/-.*//' | grep -E '^[0-9]+\.[0-9]+\.[0-9]+$$' || echo "0.0.0") -.PHONY: build build-debug size release clean test test-mdl check-mdl check-skill-mdl check-skill-pack-js check-findings check-wiki-pages digest-status check-tunnel-deps check-widget-versions grammar completions sync-skills sync-skill-packs sync-commands sync-lint-rules sync-changelog sync-all docs documentation docs-site docs-serve vscode-ext vscode-install source-tree sbom sbom-report lint lint-go lint-ts fmt fmt-check vet +.PHONY: build build-debug size release clean test test-mdl check-mdl check-skill-mdl check-skill-pack-js check-findings check-wiki-pages digest-status check-tunnel-deps check-widget-versions test-integration test-integration-executor test-integration-roundtrip test-integration-upgrade test-integration-other grammar completions sync-skills sync-skill-packs sync-commands sync-lint-rules sync-changelog sync-all docs documentation docs-site docs-serve vscode-ext vscode-install source-tree sbom sbom-report lint lint-go lint-ts fmt fmt-check vet # Helper: copy file only if content differs (avoids mtime updates that invalidate go build cache) # Usage: $(call copy-if-changed,src,dst) @@ -313,7 +313,47 @@ check-tunnel-deps: # therefore left unset everywhere; it survives only so that a stale # `MXCLI_TEST_ENGINES=legacy` is fatal rather than silently selecting nothing. test-integration: - CGO_ENABLED=0 go test -tags integration -count=1 -timeout 30m ./... + CGO_ENABLED=0 go test -tags integration -count=1 -timeout 60m ./... + +# The same integration tests split into suites that CI runs as parallel jobs +# (ako/mxcli#757): one step running them all spent ~22 of its 30 minutes once +# the upgrade property test landed, with mdl/roundtrip alone the critical path. +# +# Unlike test-integration, the suites run only the packages that HAVE +# integration-tagged tests. Every other package's tests are identical with and +# without the tag, and `make test` already runs them. +# +# test-integration-executor mdl/executor: doctype scripts through exec + mx check +# test-integration-roundtrip mdl/roundtrip: the round-trip laws, the upgrade controls +# test-integration-upgrade mdl/roundtrip: the upgrade property test alone; +# MXCLI_UPGRADE_SHARD=i/n runs one of n shards, +# MXCLI_UPGRADE_ALL=1 the full corpus (nightly) +# test-integration-other every other package with integration tests, +# found by build tag so a new one cannot be missed +# (hidden dirs are skipped: nested worktrees live there) +# +# `-skip` / `-run` split mdl/roundtrip by test name, so a new round-trip test +# lands in the roundtrip suite without editing this file. +INTEGRATION_PKGS = $(shell grep -rl --include='*_test.go' --exclude-dir='.?*' --exclude-dir=reference --exclude-dir=mx-test-projects \ + '^//go:build.*integration' . | xargs -n1 dirname | sed 's|^\./||; s|^|./|' | sort -u) +INTEGRATION_SPLIT = ./mdl/executor ./mdl/roundtrip +UPGRADE_PROPERTY = ^TestUpgradeExecutesToTheSameModel$$ +INTEGRATION_GO_TEST = CGO_ENABLED=0 go test -tags integration -count=1 + +test-integration-executor: + $(INTEGRATION_GO_TEST) -timeout 40m ./mdl/executor/ + +test-integration-roundtrip: + $(INTEGRATION_GO_TEST) -timeout 40m -skip '$(UPGRADE_PROPERTY)' ./mdl/roundtrip/ + +test-integration-upgrade: + $(INTEGRATION_GO_TEST) -timeout 40m -run '$(UPGRADE_PROPERTY)' ./mdl/roundtrip/ + +test-integration-other: + @pkgs="$(filter-out $(INTEGRATION_SPLIT),$(INTEGRATION_PKGS))"; \ + if [ -z "$$pkgs" ]; then echo "no integration packages outside the split suites"; exit 1; fi; \ + echo "integration packages: $$pkgs"; \ + $(INTEGRATION_GO_TEST) -timeout 40m $$pkgs # Run MDL integration tests (requires Docker and a Mendix project) # Usage: make test-mdl MPR=path/to/app.mpr From de4f8e8b14179937248d7af4e05fd5355065792a Mon Sep 17 00:00:00 2001 From: Ako Date: Sun, 27 Sep 2026 14:47:20 +0000 Subject: [PATCH 3/7] executor: extract alter microflow's operation loop into alterFlowContext.apply A pure refactor so create or modify can apply a derived patch through the same checks and splice an alter statement uses (#747). No behaviour change. Co-Authored-By: Claude Opus 5.5 --- mdl/executor/cmd_alter_flow.go | 37 +++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/mdl/executor/cmd_alter_flow.go b/mdl/executor/cmd_alter_flow.go index d8aac4309..a2deb95ac 100644 --- a/mdl/executor/cmd_alter_flow.go +++ b/mdl/executor/cmd_alter_flow.go @@ -45,31 +45,48 @@ func execAlterFlow(ctx *ExecContext, s *ast.AlterFlowStmt) error { targets[i] = c } + mut, err := a.apply(ctx, s.Operations, targets) + if err != nil { + return err + } + if err := mut.Save(); err != nil { + return mdlerrors.NewBackend("save altered "+s.Kind(), err) + } + fmt.Fprintf(ctx.Output, "Altered %s %s\n", s.Kind(), s.Name) + return nil +} + +// apply opens the stored flow for splicing and applies ops, each aimed at the +// candidate at the same index of targets (resolved against the flow as stored). +// It returns the mutator unsaved: the caller saves, or discards it on error so +// nothing is written. +func (a *alterFlowContext) apply(ctx *ExecContext, ops []*ast.AlterFlowOperation, targets []mfmutator.Candidate) (backend.MicroflowMutator, error) { + s := a.stmt mut, err := ctx.Backend.OpenMicroflowForMutation(a.mf.ID) if err != nil { - return mdlerrors.NewBackend("open "+s.Kind()+" for alter", err) + return nil, mdlerrors.NewBackend("open "+s.Kind()+" for alter", err) } - for i, op := range s.Operations { + for i, op := range ops { target := targets[i] fail := func(err error) error { return mdlerrors.NewValidation(fmt.Sprintf("alter %s %s: %s %s: %v", s.Kind(), s.Name, op.Op, op.Target, err)) } if op.Op == ast.AlterFlowDrop { if err := a.checkOutputUnused(target, nil); err != nil { - return fail(err) + return nil, fail(err) } if err := mut.Drop(target.ID); err != nil { - return fail(err) + return nil, fail(err) } a.noteRemoved(target, nil) continue } frag, err := a.buildFragment(ctx, op.Body) if err != nil { - return fail(err) + return nil, fail(err) } if err := a.checkFragmentScope(ctx, op, target, frag); err != nil { - return fail(err) + return nil, fail(err) } switch op.Op { case ast.AlterFlowInsertAfter: @@ -84,18 +101,14 @@ func execAlterFlow(ctx *ExecContext, s *ast.AlterFlowStmt) error { err = fmt.Errorf("unknown operation") } if err != nil { - return fail(err) + return nil, fail(err) } if op.Op == ast.AlterFlowReplace { a.noteRemoved(target, frag) } a.noteFragment(ctx, frag) } - if err := mut.Save(); err != nil { - return mdlerrors.NewBackend("save altered "+s.Kind(), err) - } - fmt.Fprintf(ctx.Output, "Altered %s %s\n", s.Kind(), s.Name) - return nil + return mut, nil } // alterFlowContext is what the operations of one statement share: the stored From e4d81bd953f3197e3cc42c8a00f28c578c7092e0 Mon Sep 17 00:00:00 2001 From: Ako Date: Sun, 27 Sep 2026 15:08:29 +0000 Subject: [PATCH 4/7] executor: create or modify microflow|nanoflow is diff-then-patch (#747) An existing flow is no longer rebuilt. The stored flow is described and parsed back, compared with the declared definition statement by statement (LCS over declaredMatches, recursing into if branches), and the minimal insert/replace/drop is applied through the #739 splice via the same alterFlowContext an alter statement uses. An unchanged definition writes nothing and reports Unchanged. A change the splice cannot express (header, loop or error-handler body, a moved node) is refused under mdl 1 and still rebuilt under mdl 0 with the MDL-V1-REBUILD warning (ADR-0011). Strikes all #721 class A microflow and nanoflow entries from the PedApp round-trip allowlist and the two nanoflow entries from studioProKnownLossy. Co-Authored-By: Claude Opus 5.5 --- .../skills/mendix/write-microflows/SKILL.md | 16 +- .../skills/mendix/write-nanoflows/SKILL.md | 16 +- mdl/executor/cmd_alter_flow.go | 9 +- mdl/executor/cmd_flow_modify.go | 815 ++++++++++++++++++ mdl/executor/cmd_microflows_create.go | 12 + mdl/executor/cmd_nanoflows_create.go | 15 + mdl/executor/flow_declared_match.go | 137 +++ mdl/executor/flow_declared_match_test.go | 107 +++ mdl/executor/studiopro_roundtrip_test.go | 5 - mdl/roundtrip/allowlist_test.go | 33 - mdl/roundtrip/flow_modify_test.go | 394 +++++++++ 11 files changed, 1507 insertions(+), 52 deletions(-) create mode 100644 mdl/executor/cmd_flow_modify.go create mode 100644 mdl/executor/flow_declared_match.go create mode 100644 mdl/executor/flow_declared_match_test.go create mode 100644 mdl/roundtrip/flow_modify_test.go diff --git a/.claude/skills/mendix/write-microflows/SKILL.md b/.claude/skills/mendix/write-microflows/SKILL.md index ac77a04dd..4f05786ab 100644 --- a/.claude/skills/mendix/write-microflows/SKILL.md +++ b/.claude/skills/mendix/write-microflows/SKILL.md @@ -42,12 +42,16 @@ Choose the mode by who owns the microflow ([choose-edit-mode](../choose-edit-mod - **Created by your MDL scripts, and not edited in Studio Pro since:** edit the script (or fresh `describe` output) and re-run `create or modify`. -- **Authored in Studio Pro:** there is **no `alter microflow` yet**, and re-emitting it - with `create or modify` renumbers element IDs, removes merges and resets connector - curves even for a one-line change. Keep the change minimal: put new logic in a new - sub-microflow and change the existing flow only to call it. Commit first, then - `describe` it again after `exec` and diff it against the original output. Anything - that differs and that you did not change is a loss. +- **Authored in Studio Pro:** prefer `alter microflow X { insert/replace/drop … }` + (targets from `describe microflow X with handles`). `create or modify` of `describe` + output also works as a patch: an unchanged definition writes nothing, and an inserted, + replaced or dropped statement (at the top level or in an `if` branch) is spliced in, + leaving every other node, merge and curve as stored. A change it cannot splice — the + header, anything inside a loop body or error handler, a moved node — rebuilds the + whole microflow under mdl 0 + (warning `MDL-V1-REBUILD`: element IDs renumbered, merges removed, curves reset) and + is refused under `mdl 1;` (header and loop-body changes have no splice yet; move + nodes in Studio Pro). ## When to Use a Microflow vs a Nanoflow diff --git a/.claude/skills/mendix/write-nanoflows/SKILL.md b/.claude/skills/mendix/write-nanoflows/SKILL.md index 1b03588b7..afcae59da 100644 --- a/.claude/skills/mendix/write-nanoflows/SKILL.md +++ b/.claude/skills/mendix/write-nanoflows/SKILL.md @@ -23,12 +23,16 @@ Choose the mode by who owns the nanoflow ([choose-edit-mode](../choose-edit-mode - **Created by your MDL scripts, and not edited in Studio Pro since:** edit the script (or fresh `describe` output) and re-run `create or modify`. -- **Authored in Studio Pro:** there is **no `alter nanoflow` yet**, and re-emitting it - with `create or modify` has dropped annotation links and changed the export level on - Studio Pro nanoflows, even with no edit at all. Keep the change minimal: put new logic - in a new nanoflow and change the existing one only to call it. Commit first, then - `describe` it again after `exec` and diff it against the original output. Anything - that differs and that you did not change is a loss. +- **Authored in Studio Pro:** prefer `alter nanoflow X { insert/replace/drop … }` + (targets by output variable, caption or statement pattern). `create or modify` of `describe` + output also works as a patch: an unchanged definition writes nothing, and an inserted, + replaced or dropped statement (at the top level or in an `if` branch) is spliced in, + leaving every other node, merge and curve as stored. A change it cannot splice — the + header, anything inside a loop body or error handler, a moved node — rebuilds the + whole nanoflow under mdl 0 + (warning `MDL-V1-REBUILD`: element IDs renumbered, merges removed, curves reset) and + is refused under `mdl 1;` (header and loop-body changes have no splice yet; move + nodes in Studio Pro). ## When to Use a Nanoflow vs a Microflow diff --git a/mdl/executor/cmd_alter_flow.go b/mdl/executor/cmd_alter_flow.go index a2deb95ac..7af925184 100644 --- a/mdl/executor/cmd_alter_flow.go +++ b/mdl/executor/cmd_alter_flow.go @@ -128,11 +128,15 @@ type alterFlowContext struct { declaredByOps map[string]bool readByOps map[string][]string removedByOps map[string]bool + // removedIDs are the stored activities earlier operations took out; what + // they read no longer counts as a use. + removedIDs map[model.ID]bool } // noteRemoved records that target's output is gone, unless the fragment that // replaces it declares it again. func (a *alterFlowContext) noteRemoved(target mfmutator.Candidate, replacement *backend.MicroflowFragment) { + a.removedIDs[target.ID] = true v := target.OutputVariable if v == "" || fragmentDeclares(replacement, v) { return @@ -174,7 +178,8 @@ func loadAlterFlow(ctx *ExecContext, s *ast.AlterFlowStmt) (*alterFlowContext, e return nil, mdlerrors.NewBackend("build hierarchy", err) } a := &alterFlowContext{stmt: s, entityNames: getEntityNames(ctx, h), - declaredByOps: map[string]bool{}, readByOps: map[string][]string{}, removedByOps: map[string]bool{}} + declaredByOps: map[string]bool{}, readByOps: map[string][]string{}, removedByOps: map[string]bool{}, + removedIDs: map[model.ID]bool{}} // A copy: nanoflow names are added below, and the cached map is shared. a.microflowNames = map[model.ID]string{} for id, n := range getMicroflowNames(ctx, h) { @@ -500,7 +505,7 @@ func (a *alterFlowContext) checkOutputUnused(target mfmutator.Candidate, replace ref := regexp.MustCompile(`\$` + regexp.QuoteMeta(v) + `\b`) var users []string for _, c := range a.cands { - if c.ID == target.ID { + if c.ID == target.ID || a.removedIDs[c.ID] { continue } for _, text := range append([]string{c.Statement}, c.Alternates...) { diff --git a/mdl/executor/cmd_flow_modify.go b/mdl/executor/cmd_flow_modify.go new file mode 100644 index 000000000..2fdf0c369 --- /dev/null +++ b/mdl/executor/cmd_flow_modify.go @@ -0,0 +1,815 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "bytes" + "errors" + "fmt" + "reflect" + "strings" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/backend/mfmutator" + mdlerrors "github.com/mendixlabs/mxcli/mdl/errors" + "github.com/mendixlabs/mxcli/mdl/langver" + "github.com/mendixlabs/mxcli/mdl/visitor" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// # create or modify microflow|nanoflow as diff-then-patch (plan item 4.2g) +// +// ADR-0012 decision 3: `create or modify` on a flow that exists compares the +// declared definition with the stored document, derives the minimal patch and +// applies it with the splice engine `alter microflow` uses (mfmutator). An +// empty patch writes nothing. +// +// The stored side is the flow as `describe` prints it, parsed back: that is the +// one rendering of a stored flow MDL already guarantees to re-parse, and it +// puts both sides in the same AST, so "the same definition" is a structural +// comparison rather than a second renderer (the #997 lesson). Statements are +// matched with declaredMatches — the whole statement, so its signature and its +// output variable — by a longest common subsequence; each run of unmatched +// statements between two matched ones becomes one insert, replace or drop, +// aimed at the stored activity by its @position and applied through the same +// alterFlowContext an `alter` statement uses. An `if` that differs only inside +// its branches is diffed branch by branch. +// +// What the splice cannot express — a changed header, a change inside a loop +// body or an error handler, a moved node — is not rebuilt under `mdl 1`: the whole-document rebuild is what reset curves, dropped merges and +// moved element IDs on Studio Pro-authored flows (#721 class A). It is refused +// with the reason instead. Under mdl 0 the rebuild still runs, with the +// MDL-V1-REBUILD warning (ADR-0011: a new refusal applies only under the +// header that opts into it). + +// flowRebuildRefused is the language change: the lossy fallback becomes a +// refusal under mdl 1. It is gated in the executor rather than the visitor +// because whether a statement needs the fallback depends on what is stored, so +// no parse of the script can tell. +var flowRebuildRefused = langver.Change{ + Code: "MDL-V1-REBUILD", + Since: langver.V1, + Old: "`create or modify microflow|nanoflow` rebuilds the whole stored flow when the change cannot be " + + "spliced in, which resets curves, drops merges and renumbers element IDs", + New: "a refusal that names the change the splice cannot make, with nothing written", +} + +// flowDecl is what diff-then-patch needs of a CREATE MICROFLOW or CREATE +// NANOFLOW statement. +type flowDecl struct { + nanoflow bool + name ast.QualifiedName + body []ast.MicroflowStatement + folder string + // header returns the declared and the stored statement with body, folder + // and name cleared, after the rules by which an absent clause keeps what + // is stored have been applied, ready for declaredMatches. + header func(stored ast.Statement) (declared, storedHeader any) +} + +func (d *flowDecl) kind() string { + if d.nanoflow { + return "nanoflow" + } + return "microflow" +} + +// notSpliceable is the reason a declared change has to fall back. +type notSpliceable struct{ reason string } + +func (e *notSpliceable) Error() string { return e.reason } + +func cannotSplice(format string, args ...any) error { + return ¬Spliceable{reason: fmt.Sprintf(format, args...)} +} + +// modifyFlowInPlace applies a `create or modify` of an existing flow as a +// patch. handled is false when the statement is not this path's to apply — no +// such flow yet, or a change the splice cannot make under mdl 0 — and the +// caller then runs the create / rebuild path. +func modifyFlowInPlace(ctx *ExecContext, d *flowDecl) (handled bool, err error) { + if _, err := findModule(ctx, d.name.Module); err != nil { + return false, nil // the create path makes the module + } + alter := &ast.AlterFlowStmt{Nanoflow: d.nanoflow, Name: d.name} + a, err := loadAlterFlow(ctx, alter) + if err != nil { + var nf *mdlerrors.NotFoundError + if errors.As(err, &nf) { + return false, nil // a create + } + return fallBack(ctx, d, cannotSplice("the stored %s cannot be read for splicing: %v", d.kind(), err)) + } + + stored, err := describedFlowStmt(ctx, d, a) + if err != nil { + return fallBack(ctx, d, cannotSplice("its description cannot be compared: %v", err)) + } + decl, storedHeader := d.header(stored) + if !declaredMatches(decl, storedHeader) { + return fallBack(ctx, d, cannotSplice("the header changes (parameters, return type or document properties); "+ + "the splice edits the flow's activities only")) + } + + ops, targets, err := diffFlowBody(a, d.body, storedBody(stored)) + if err != nil { + return fallBack(ctx, d, err) + } + + storedFolder := storedFolderOf(stored) + if len(ops) > 0 { + mut, err := a.apply(ctx, ops, targets) + if err != nil { + return fallBack(ctx, d, cannotSplice("%v", err)) + } + if err := mut.Save(); err != nil { + return true, mdlerrors.NewBackend("save modified "+d.kind(), err) + } + } + containerID := a.mf.ContainerID + if d.folder != storedFolder { + mod, err := findModule(ctx, d.name.Module) + if err != nil { + return true, err + } + to := mod.ID + if d.folder != "" { + if to, err = resolveFolder(ctx, mod.ID, d.folder); err != nil { + return true, mdlerrors.NewBackend("resolve folder "+d.folder, err) + } + } + if _, err := applyDocumentFolder(ctx, a.mf.ID, a.mf.ContainerID, to); err != nil { + return true, err + } + containerID = to + } + + switch { + case len(ops) > 0: + ctx.ReportMutation("Modified", "%s: %s (%s)", d.kind(), d.name, patchSummary(ops)) + case d.folder != storedFolder: + ctx.ReportMutation("Moved", "%s: %s", d.kind(), d.name) + default: + reportUnchanged(ctx, fmt.Sprintf("%s: %s", d.kind(), d.name)) + } + + returnEntity := extractEntityFromReturnType(a.mf.ReturnType) + if d.nanoflow { + ctx.trackCreatedNanoflow(d.name.Module, d.name.Name, a.mf.ID, containerID, returnEntity) + } else { + ctx.trackCreatedMicroflow(d.name.Module, d.name.Name, a.mf.ID, containerID, returnEntity) + } + invalidateHierarchy(ctx) + return true, nil +} + +// fallBack decides what a change the splice cannot make does: refused under +// mdl 1, the whole-document rebuild with a warning under mdl 0. +func fallBack(ctx *ExecContext, d *flowDecl, why error) (bool, error) { + if flowRebuildRefused.Applies(ctx.LanguageVersion) { + return true, mdlerrors.NewValidation(fmt.Sprintf( + "create or modify %s %s: this change cannot be spliced into the stored flow: %v. "+ + "Nothing was written: rebuilding the whole flow instead would reset what Studio Pro drew "+ + "(curves, merges, element IDs). Change activities with `alter %s %s { … }`; "+ + "to rebuild the flow deliberately, drop the %s and create it", + d.kind(), d.name, why, d.kind(), d.name, d.kind())) + } + fmt.Fprintf(ctx.progress(), "Warning [%s]: %s %s is rebuilt as a whole: %v. %s\n", + flowRebuildRefused.Code, d.kind(), d.name, why, flowRebuildRefused.Warning(ctx.LanguageVersion)) + return false, nil +} + +// reportUnchanged reports a statement that wrote nothing, collapsing into the +// run's summary like an elided write does. +func reportUnchanged(ctx *ExecContext, what string) { + line := fmt.Sprintf("Unchanged %s\n", what) + if ctx.tally.countUnchanged(line) { + return + } + fmt.Fprint(ctx.Output, line) +} + +// describedFlowStmt describes the stored flow and parses the description +// under the language version of the script being run, so both sides are read +// by the same rules. See correctAmbiguousRanges for the one stored state the +// description cannot state under mdl 0. +func describedFlowStmt(ctx *ExecContext, d *flowDecl, a *alterFlowContext) (ast.Statement, error) { + var buf bytes.Buffer + prev := ctx.Output + ctx.Output = &buf + var err error + if d.nanoflow { + err = describeNanoflow(ctx, d.name) + } else { + err = describeMicroflow(ctx, d.name) + } + ctx.Output = prev + if err != nil { + return nil, err + } + src := buf.String() + if v := ctx.LanguageVersion; v > langver.V0 { + src = v.String() + ";\n" + src + } + prog, errs := visitor.Build(src) + if len(errs) > 0 { + return nil, fmt.Errorf("the description does not parse: %v", errs[0]) + } + for _, st := range prog.Statements { + switch st.(type) { + case *ast.CreateMicroflowStmt: + if d.nanoflow { + continue + } + case *ast.CreateNanoflowStmt: + if !d.nanoflow { + continue + } + default: + continue + } + if !limitOneIsListUnder(ctx.LanguageVersion) { + correctAmbiguousRanges(st, a.mf.ObjectCollection) + } + return st, nil + } + return nil, fmt.Errorf("the description has no create statement") +} + +// limitOneIsListUnder reports whether `limit 1` reads as a list of one under v +// (the visitor's MDL-V1-LIMIT1 change). +func limitOneIsListUnder(v langver.Version) bool { return v >= langver.V1 } + +// correctAmbiguousRanges fixes the stored side where its mdl 0 description +// says something other than what is stored. +// +// A retrieve with a Custom range of `limit 1` and no offset — a list of one — +// is described as `limit 1`, which mdl 0 reads as the object range +// (ako/mxcli#734): mdl 0 has no spelling for that exact state. Parsed as is, +// the stored side would claim the object range, and a script stating the +// object range would look unchanged against a flow that holds a list; so the +// parsed statement is set back to the list it stands for. The statement is +// found by its output variable and its @position. +func correctAmbiguousRanges(st ast.Statement, oc *microflows.MicroflowObjectCollection) { + type key struct { + v string + p model.Point + } + lists := map[key]bool{} + var collect func(oc *microflows.MicroflowObjectCollection) + collect = func(oc *microflows.MicroflowObjectCollection) { + if oc == nil { + return + } + for _, obj := range oc.Objects { + if loop, ok := obj.(*microflows.LoopedActivity); ok { + collect(loop.ObjectCollection) + continue + } + act, ok := obj.(*microflows.ActionActivity) + if !ok { + continue + } + r, ok := act.Action.(*microflows.RetrieveAction) + if !ok { + continue + } + src, ok := r.Source.(*microflows.DatabaseRetrieveSource) + if !ok || src.Range == nil || src.Range.RangeType != microflows.RangeTypeCustom || + src.Range.Limit != "1" || src.Range.Offset != "" { + continue + } + lists[key{r.OutputVariable, act.Position}] = true + } + } + collect(oc) + if len(lists) == 0 { + return + } + walkStatements(st, func(r *ast.RetrieveStmt) { + if !r.First || r.Annotations == nil || r.Annotations.Position == nil { + return + } + k := key{r.Variable, model.Point{X: r.Annotations.Position.X, Y: r.Annotations.Position.Y}} + if lists[k] { + r.First, r.Limit, r.Offset = false, "1", "" + } + }) +} + +// walkStatements calls fn for every retrieve statement in v, at any depth. +func walkStatements(v any, fn func(*ast.RetrieveStmt)) { + var walk func(rv reflect.Value) + walk = func(rv reflect.Value) { + switch rv.Kind() { + case reflect.Interface: + if !rv.IsNil() { + walk(rv.Elem()) + } + case reflect.Pointer: + if rv.IsNil() { + return + } + if r, ok := rv.Interface().(*ast.RetrieveStmt); ok { + fn(r) + } + walk(rv.Elem()) + case reflect.Struct: + for i := 0; i < rv.NumField(); i++ { + if rv.Type().Field(i).IsExported() { + walk(rv.Field(i)) + } + } + case reflect.Slice, reflect.Array: + for i := 0; i < rv.Len(); i++ { + walk(rv.Index(i)) + } + } + } + walk(reflect.ValueOf(v)) +} + +func storedBody(st ast.Statement) []ast.MicroflowStatement { + switch s := st.(type) { + case *ast.CreateMicroflowStmt: + return s.Body + case *ast.CreateNanoflowStmt: + return s.Body + } + return nil +} + +func storedFolderOf(st ast.Statement) string { + switch s := st.(type) { + case *ast.CreateMicroflowStmt: + return s.Folder + case *ast.CreateNanoflowStmt: + return s.Folder + } + return "" +} + +// microflowDecl adapts a CREATE MICROFLOW statement. +func microflowDecl(s *ast.CreateMicroflowStmt) *flowDecl { + return &flowDecl{ + name: s.Name, body: s.Body, folder: s.Folder, + header: func(stored ast.Statement) (any, any) { + st, _ := stored.(*ast.CreateMicroflowStmt) + if st == nil { + return s, nil + } + dh, sh := *s, *st + for _, h := range []*ast.CreateMicroflowStmt{&dh, &sh} { + h.Body, h.Folder, h.Name, h.CreateOrModify = nil, "", ast.QualifiedName{}, false + } + // Absent means "keep what is stored" for these (see the field + // comments on CreateMicroflowStmt), so absent is not a difference. + if !dh.DocumentationSet { + dh.Documentation = sh.Documentation + } + dh.DocumentationSet, sh.DocumentationSet = false, false + if !dh.Excluded { + dh.Excluded = sh.Excluded + } + if dh.ApplyEntityAccess == nil { + dh.ApplyEntityAccess = sh.ApplyEntityAccess + } + if dh.Expose == nil { + dh.Expose = sh.Expose + } + if dh.URL == nil { + dh.URL = sh.URL + } + if dh.URLSearchParameters == nil { + dh.URLSearchParameters = sh.URLSearchParameters + } + if dh.ExportLevel == nil { + dh.ExportLevel = sh.ExportLevel + } + if dh.Concurrency == nil { + dh.Concurrency = sh.Concurrency + } + return &dh, &sh + }, + } +} + +// nanoflowDecl adapts a CREATE NANOFLOW statement. +func nanoflowDecl(s *ast.CreateNanoflowStmt) *flowDecl { + return &flowDecl{ + nanoflow: true, name: s.Name, body: s.Body, folder: s.Folder, + header: func(stored ast.Statement) (any, any) { + st, _ := stored.(*ast.CreateNanoflowStmt) + if st == nil { + return s, nil + } + dh, sh := *s, *st + for _, h := range []*ast.CreateNanoflowStmt{&dh, &sh} { + h.Body, h.Folder, h.Name, h.CreateOrModify = nil, "", ast.QualifiedName{}, false + } + if !dh.DocumentationSet { + dh.Documentation = sh.Documentation + } + dh.DocumentationSet, sh.DocumentationSet = false, false + if !dh.Excluded { + dh.Excluded = sh.Excluded + } + if dh.Expose == nil { + dh.Expose = sh.Expose + } + return &dh, &sh + }, + } +} + +// diffFlowBody derives the splice operations that turn the stored top-level +// statements into the declared ones. Matched statements are left alone; each +// run of unmatched statements between two matched ones becomes one operation: +// +// - declared statements where none are stored: insert after the stored +// statement before the run (or before the one after it); +// - stored statements where none are declared: drop each; +// - both: replace the first stored one with the declared run, drop the rest. +// +// Targets are the stored activities, located by the @position describe printed +// for each statement. A run whose stored end cannot be located, or that the +// splice cannot express, is a notSpliceable error. +func diffFlowBody(a *alterFlowContext, declared, stored []ast.MicroflowStatement) ([]*ast.AlterFlowOperation, []mfmutator.Candidate, error) { + // Free annotations — notes wired to nothing — belong to the flow, not to + // the statement describe happens to print them above (the first one), so + // they are compared as a whole and kept out of the statement match: an + // insert at the top would otherwise carry them onto a new statement and + // write them a second time. + declared, declaredFree := withoutFreeNotes(declared) + stored, storedFree := withoutFreeNotes(stored) + if !declaredMatches(declaredFree, storedFree) { + return nil, nil, cannotSplice("the free annotations change; the splice edits activities only") + } + + pd := &patchDiff{loc: newStoredLocator(a)} + if err := pd.statements(declared, stored); err != nil { + return nil, nil, err + } + // Drops go last. Each operation's scope check runs against the flow as the + // earlier operations left it, so a stored activity whose output only a + // replaced statement read can be dropped once that statement is replaced, + // and not before. The splice itself does not care about the order: every + // target is a stored activity no other operation touches. + var ops []*ast.AlterFlowOperation + var targets []mfmutator.Candidate + for _, drops := range []bool{false, true} { + for i, op := range pd.ops { + if (op.Op == ast.AlterFlowDrop) == drops { + ops = append(ops, op) + targets = append(targets, pd.targets[i]) + } + } + } + return ops, targets, nil +} + +// patchDiff accumulates the operations of one diff. +type patchDiff struct { + loc *storedLocator + ops []*ast.AlterFlowOperation + targets []mfmutator.Candidate +} + +func (pd *patchDiff) add(op ast.AlterFlowOpKind, c mfmutator.Candidate, body []ast.MicroflowStatement) { + pd.ops = append(pd.ops, &ast.AlterFlowOperation{Op: op, Target: targetLabel(c), Body: body}) + pd.targets = append(pd.targets, c) +} + +// statements diffs one statement list: the flow's top level, or a branch of +// an `if`. The activities in an `if` branch are top-level nodes of the stored +// graph (only a loop nests a collection), so a branch is spliced the same way. +func (pd *patchDiff) statements(declared, stored []ast.MicroflowStatement) error { + pairs := lcsStatements(declared, stored) + di, si := 0, 0 + for p := 0; p <= len(pairs); p++ { + dEnd, sEnd := len(declared), len(stored) + if p < len(pairs) { + dEnd, sEnd = pairs[p][0], pairs[p][1] + } + if err := pd.gap(declared[di:dEnd], stored, si, sEnd); err != nil { + return err + } + if p < len(pairs) { + di, si = pairs[p][0]+1, pairs[p][1]+1 + } + } + return nil +} + +// gap turns one run of unmatched statements — ins declared where stored[si:sEnd] +// is stored — into operations. +func (pd *patchDiff) gap(ins []ast.MicroflowStatement, stored []ast.MicroflowStatement, si, sEnd int) error { + del := stored[si:sEnd] + switch { + case len(ins) == 0 && len(del) == 0: + return nil + case len(del) == 0: + op, c, err := insertAnchor(pd.loc, stored, si, sEnd) + if err != nil { + return err + } + pd.add(op, c, ins) + return nil + } + if len(ins) == 1 && len(del) == 1 { + if d, s, ok := sameIfShell(ins[0], del[0]); ok { + // The same `if` with a change in a branch: splice the branches. + if err := pd.statements(d.ThenBody, s.ThenBody); err != nil { + return err + } + return pd.statements(d.ElseBody, s.ElseBody) + } + if sameIgnoringLayout(ins[0], del[0]) { + p := statementAnnotations(del[0]) + where := "" + if p != nil && p.Position != nil { + where = fmt.Sprintf(" at (%d, %d)", p.Position.X, p.Position.Y) + } + return cannotSplice("the %s%s is moved or its connectors are redrawn; the splice places new nodes only and does not move stored ones", + statementKind(del[0]), where) + } + } + cands := make([]mfmutator.Candidate, len(del)) + for i, st := range del { + c, err := pd.loc.locate(st) + if err != nil { + return err + } + cands[i] = c + } + rest, restStmts := cands, del + if len(ins) > 0 { + body, err := keepStoredNotes(del[0], ins) + if err != nil { + return err + } + pd.add(ast.AlterFlowReplace, cands[0], body) + rest, restStmts = cands[1:], del[1:] + } + for i, c := range rest { + if ann := statementAnnotations(restStmts[i]); ann != nil && len(ann.Notes) > 0 { + return cannotSplice("the %s dropped at (%d, %d) carries an annotation, which would be left behind unattached", + statementKind(restStmts[i]), c.Object.GetPosition().X, c.Object.GetPosition().Y) + } + pd.add(ast.AlterFlowDrop, c, nil) + } + return nil +} + +// sameIfShell reports whether two statements are the same `if` — condition, +// annotations, whether it has an else — differing at most inside its branches. +func sameIfShell(declared, stored ast.MicroflowStatement) (*ast.IfStmt, *ast.IfStmt, bool) { + d, ok1 := declared.(*ast.IfStmt) + s, ok2 := stored.(*ast.IfStmt) + if !ok1 || !ok2 { + return nil, nil, false + } + dShell, sShell := *d, *s + dShell.ThenBody, dShell.ElseBody, sShell.ThenBody, sShell.ElseBody = nil, nil, nil, nil + if !declaredMatches(&dShell, &sShell) { + return nil, nil, false + } + return d, s, true +} + +// keepStoredNotes prepares the declared statements that replace a stored one. +// The splice keeps the stored activity's annotation notes and attaches them to +// the replacement's first activity, so the declared statement must carry the +// same notes — and they are taken off it, or the builder would draw each one a +// second time. A replaced statement with other notes than the stored one is a +// change of annotations, which the splice does not make. +func keepStoredNotes(stored ast.MicroflowStatement, declared []ast.MicroflowStatement) ([]ast.MicroflowStatement, error) { + var storedNotes []ast.MicroflowAnnotation + if ann := statementAnnotations(stored); ann != nil { + storedNotes = ann.Notes + } + if len(storedNotes) == 0 { + return declared, nil + } + var declaredNotes []ast.MicroflowAnnotation + if ann := statementAnnotations(declared[0]); ann != nil { + declaredNotes = ann.Notes + } + if !declaredMatches(declaredNotes, storedNotes) { + return nil, cannotSplice("the annotations on the replaced %s change; the splice keeps a replaced activity's notes as stored", + statementKind(stored)) + } + out := append([]ast.MicroflowStatement(nil), declared...) + out[0] = withAnnotations(declared[0], func(a *ast.ActivityAnnotations) { a.Notes = nil }) + return out, nil +} + +// withoutFreeNotes returns the statements with their free annotations taken +// off (as copies; the script's own statements are not modified, since the +// rebuild may still need them), and the free annotations in order. +func withoutFreeNotes(stmts []ast.MicroflowStatement) ([]ast.MicroflowStatement, []ast.MicroflowAnnotation) { + var free []ast.MicroflowAnnotation + out := make([]ast.MicroflowStatement, len(stmts)) + for i, st := range stmts { + out[i] = st + if ann := statementAnnotations(st); ann != nil && len(ann.FreeNotes) > 0 { + free = append(free, ann.FreeNotes...) + out[i] = withAnnotations(st, func(a *ast.ActivityAnnotations) { a.FreeNotes = nil }) + } + } + return out, free +} + +// withAnnotations returns a shallow copy of st whose annotations are a copy +// edited by edit. st itself is left as it was. +func withAnnotations(st ast.MicroflowStatement, edit func(*ast.ActivityAnnotations)) ast.MicroflowStatement { + v := reflectElem(st) + if !v.IsValid() { + return st + } + cp := reflect.New(v.Type()) + cp.Elem().Set(v) + f := cp.Elem().FieldByName("Annotations") + ann, ok := f.Interface().(*ast.ActivityAnnotations) + if !ok || ann == nil { + return st + } + annCopy := *ann + edit(&annCopy) + f.Set(reflect.ValueOf(&annCopy)) + out, ok := cp.Interface().(ast.MicroflowStatement) + if !ok { + return st + } + return out +} + +// insertAnchor chooses where a run of new statements goes: after the stored +// activity before it when that is an activity (one flow leaves it), else +// before the stored statement after it. gapStart is the index of the first +// stored statement after the run's predecessor; next is the index of the +// matched statement after the run. +func insertAnchor(loc *storedLocator, stored []ast.MicroflowStatement, gapStart, next int) (ast.AlterFlowOpKind, mfmutator.Candidate, error) { + if gapStart > 0 { + if c, err := loc.locate(stored[gapStart-1]); err == nil { + if _, ok := c.Object.(*microflows.ActionActivity); ok { + return ast.AlterFlowInsertAfter, c, nil + } + } + } + if next < len(stored) { + c, err := loc.locate(stored[next]) + if err != nil { + return "", mfmutator.Candidate{}, err + } + return ast.AlterFlowInsertBefore, c, nil + } + if gapStart > 0 { + c, err := loc.locate(stored[gapStart-1]) + if err != nil { + return "", mfmutator.Candidate{}, err + } + return ast.AlterFlowInsertAfter, c, nil + } + return "", mfmutator.Candidate{}, cannotSplice("the stored flow has no statement to insert next to") +} + +// lcsStatements pairs declared and stored statements that match, as a longest +// common subsequence; each pair is {declared index, stored index}, ascending. +func lcsStatements(declared, stored []ast.MicroflowStatement) [][2]int { + n, m := len(declared), len(stored) + eq := make([][]bool, n) + for i := range declared { + eq[i] = make([]bool, m) + for j := range stored { + eq[i][j] = declaredMatches(declared[i], stored[j]) + } + } + // l[i][j] is the LCS length of declared[i:] and stored[j:]. + l := make([][]int, n+1) + for i := range l { + l[i] = make([]int, m+1) + } + for i := n - 1; i >= 0; i-- { + for j := m - 1; j >= 0; j-- { + switch { + case eq[i][j]: + l[i][j] = l[i+1][j+1] + 1 + case l[i+1][j] >= l[i][j+1]: + l[i][j] = l[i+1][j] + default: + l[i][j] = l[i][j+1] + } + } + } + var pairs [][2]int + for i, j := 0, 0; i < n && j < m; { + switch { + case eq[i][j] && l[i][j] == l[i+1][j+1]+1: + pairs = append(pairs, [2]int{i, j}) + i++ + j++ + case l[i+1][j] >= l[i][j+1]: + i++ + default: + j++ + } + } + return pairs +} + +// storedLocator finds the stored activity a statement of the stored flow's +// description stands for. describe prints each activity's @position, which is +// its RelativeMiddlePoint; among the top-level objects that is unique in any +// flow drawn so that nodes do not sit on top of each other. +type storedLocator struct { + byPos map[model.Point][]mfmutator.Candidate +} + +func newStoredLocator(a *alterFlowContext) *storedLocator { + top := map[model.ID]bool{} + for _, obj := range a.mf.ObjectCollection.Objects { + top[obj.GetID()] = true + } + loc := &storedLocator{byPos: map[model.Point][]mfmutator.Candidate{}} + for _, c := range a.cands { + if top[c.ID] && c.Object != nil { + p := c.Object.GetPosition() + loc.byPos[p] = append(loc.byPos[p], c) + } + } + return loc +} + +func (l *storedLocator) locate(st ast.MicroflowStatement) (mfmutator.Candidate, error) { + ann := statementAnnotations(st) + if ann == nil || ann.Position == nil { + return mfmutator.Candidate{}, cannotSplice("the stored statement %q has no activity to address (a join, merge or end of a branch)", statementKind(st)) + } + p := model.Point{X: ann.Position.X, Y: ann.Position.Y} + switch cs := l.byPos[p]; len(cs) { + case 1: + return cs[0], nil + case 0: + return mfmutator.Candidate{}, cannotSplice("no top-level activity is drawn at (%d, %d), where the stored %s is", p.X, p.Y, statementKind(st)) + default: + return mfmutator.Candidate{}, cannotSplice("%d activities are drawn at (%d, %d); cannot tell which one the stored %s is", len(cs), p.X, p.Y, statementKind(st)) + } +} + +// statementAnnotations returns a statement's annotations: every microflow +// statement that has them carries them in a field named Annotations. +func statementAnnotations(st ast.MicroflowStatement) *ast.ActivityAnnotations { + v := reflectElem(st) + if !v.IsValid() { + return nil + } + f := v.FieldByName("Annotations") + if !f.IsValid() { + return nil + } + ann, _ := f.Interface().(*ast.ActivityAnnotations) + return ann +} + +func statementKind(st ast.MicroflowStatement) string { + return strings.TrimSuffix(strings.TrimPrefix(fmt.Sprintf("%T", st), "*ast."), "Stmt") +} + +// targetLabel is how an operation's target is named in a message: its alter +// handle when it has one, else its statement. +func targetLabel(c mfmutator.Candidate) string { + if c.OutputVariable != "" { + return "$" + c.OutputVariable + } + if c.Statement != "" { + return c.Statement + } + return string(c.ID) +} + +// patchSummary says what a patch did, e.g. "1 replaced, 2 inserted". +func patchSummary(ops []*ast.AlterFlowOperation) string { + var ins, rep, drop int + for _, op := range ops { + switch op.Op { + case ast.AlterFlowInsertAfter, ast.AlterFlowInsertBefore: + ins++ + case ast.AlterFlowReplace: + rep++ + case ast.AlterFlowDrop: + drop++ + } + } + var parts []string + for _, p := range []struct { + n int + verb string + }{{ins, "inserted"}, {rep, "replaced"}, {drop, "dropped"}} { + if p.n > 0 { + parts = append(parts, fmt.Sprintf("%d %s", p.n, p.verb)) + } + } + return "spliced: " + strings.Join(parts, ", ") +} diff --git a/mdl/executor/cmd_microflows_create.go b/mdl/executor/cmd_microflows_create.go index ac5e6b36c..664416679 100644 --- a/mdl/executor/cmd_microflows_create.go +++ b/mdl/executor/cmd_microflows_create.go @@ -5,6 +5,7 @@ package executor import ( "fmt" + "strings" "github.com/mendixlabs/mxcli/mdl/ast" mdlerrors "github.com/mendixlabs/mxcli/mdl/errors" @@ -34,6 +35,17 @@ func execCreateMicroflow(ctx *ExecContext, s *ast.CreateMicroflowStmt) error { return mdlerrors.NewNotConnectedWrite() } + // An existing flow is modified as a patch of what is stored, not rebuilt + // (ADR-0012 decision 3); see cmd_flow_modify.go. + if s.CreateOrModify && strings.TrimSpace(s.Name.Name) != "" { + if err := validateMicroflowRules(s); err != nil { + return err + } + if handled, err := modifyFlowInPlace(ctx, microflowDecl(s)); handled || err != nil { + return err + } + } + built, err := buildMicroflowFromStmt(ctx, s, buildFlowOpts{AllowCreate: true}) if err != nil { return err diff --git a/mdl/executor/cmd_nanoflows_create.go b/mdl/executor/cmd_nanoflows_create.go index 7e56a8909..6d56e92d6 100644 --- a/mdl/executor/cmd_nanoflows_create.go +++ b/mdl/executor/cmd_nanoflows_create.go @@ -5,6 +5,7 @@ package executor import ( "fmt" + "strings" "github.com/mendixlabs/mxcli/mdl/ast" mdlerrors "github.com/mendixlabs/mxcli/mdl/errors" @@ -16,6 +17,20 @@ func execCreateNanoflow(ctx *ExecContext, s *ast.CreateNanoflowStmt) error { return mdlerrors.NewNotConnectedWrite() } + // An existing flow is modified as a patch of what is stored, not rebuilt + // (ADR-0012 decision 3); see cmd_flow_modify.go. + if s.CreateOrModify && strings.TrimSpace(s.Name.Name) != "" { + if err := refuseExposeOnFlavour(s.Expose, "nanoflow", s.Name.Module+"."+s.Name.Name); err != nil { + return err + } + if err := validateNanoflowRules(s); err != nil { + return err + } + if handled, err := modifyFlowInPlace(ctx, nanoflowDecl(s)); handled || err != nil { + return err + } + } + built, err := buildNanoflowFromStmt(ctx, s, buildFlowOpts{AllowCreate: true}) if err != nil { return err diff --git a/mdl/executor/flow_declared_match.go b/mdl/executor/flow_declared_match.go new file mode 100644 index 000000000..326d361f5 --- /dev/null +++ b/mdl/executor/flow_declared_match.go @@ -0,0 +1,137 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "reflect" + + "github.com/mendixlabs/mxcli/mdl/ast" +) + +// declaredMatches reports whether a declared microflow AST value (from the +// script) states the same thing as the stored flow's AST value (from parsing +// what `describe` prints for it). It is the equality diff-then-patch matches +// statements by (ADR-0012 decision 3; plan item 4.2g). +// +// It is structural equality with one asymmetry: canvas GEOMETRY the script +// leaves out is not a difference. A statement written without `@position`, +// `@curve`, `@anchor`, `@merge` or `@start` has no opinion about where its node +// sits or how its flow bends, so it matches the stored statement wherever that +// is drawn — `create or modify` changes only what differs (R1), and a layout +// nobody stated is not a difference. A statement that DOES state geometry must +// state the stored one: a moved node is a real change, and one the splice does +// not make (it only places new nodes), so it must not pass as unchanged. +// +// Everything else is compared exactly, including captions, colours and notes, +// which are content Studio Pro shows rather than layout. +func declaredMatches(declared, stored any) bool { + return matchValue(reflect.ValueOf(declared), reflect.ValueOf(stored), false) +} + +// sameIgnoringLayout reports whether two statements differ in canvas geometry +// at most — a node moved, a connector redrawn — which is a change the splice +// does not make. +func sameIgnoringLayout(declared, stored any) bool { + return matchValue(reflect.ValueOf(declared), reflect.ValueOf(stored), true) +} + +// The geometry is what stripFlowLayout clears for `mxcli layout flows`; a +// note's size is included here because the writer defaults it when absent. +var ( + annotationsStructType = reflect.TypeOf(ast.ActivityAnnotations{}) + noteStructType = reflect.TypeOf(ast.MicroflowAnnotation{}) + paramStructType = reflect.TypeOf(ast.MicroflowParam{}) +) + +// geometryFields names, per AST type, the fields that hold canvas geometry and +// that a script may therefore leave nil without that being a difference. +var geometryFields = map[reflect.Type]map[string]bool{ + annotationsStructType: { + "Position": true, "Anchor": true, "TrueBranchAnchor": true, "FalseBranchAnchor": true, + "IteratorAnchor": true, "BodyTailAnchor": true, "Curve": true, "Merge": true, "Start": true, + }, + noteStructType: {"Position": true, "Size": true}, + paramStructType: {"Position": true}, +} + +func matchValue(d, s reflect.Value, anyLayout bool) bool { + if !d.IsValid() || !s.IsValid() { + return d.IsValid() == s.IsValid() + } + if d.Type() != s.Type() { + return false + } + switch d.Kind() { + case reflect.Pointer: + if d.IsNil() && s.IsNil() { + return true + } + if d.Type().Elem() == annotationsStructType { + // A statement written with no annotations at all states no + // geometry, and no caption, colour or note either. + if d.IsNil() { + d = reflect.New(annotationsStructType) + } + if s.IsNil() { + s = reflect.New(annotationsStructType) + } + } + if d.IsNil() || s.IsNil() { + return false + } + return matchValue(d.Elem(), s.Elem(), anyLayout) + case reflect.Interface: + if d.IsNil() || s.IsNil() { + return d.IsNil() == s.IsNil() + } + return matchValue(d.Elem(), s.Elem(), anyLayout) + case reflect.Struct: + geo := geometryFields[d.Type()] + for i := 0; i < d.NumField(); i++ { + df := d.Field(i) + if geo[d.Type().Field(i).Name] && df.Kind() == reflect.Pointer && (anyLayout || df.IsNil()) { + continue + } + if !matchValue(df, s.Field(i), anyLayout) { + return false + } + } + return true + case reflect.Slice, reflect.Array: + // nil and empty are the same list. + if d.Len() != s.Len() { + return false + } + for i := 0; i < d.Len(); i++ { + if !matchValue(d.Index(i), s.Index(i), anyLayout) { + return false + } + } + return true + case reflect.Map: + if d.Len() != s.Len() { + return false + } + for _, k := range d.MapKeys() { + sv := s.MapIndex(k) + if !sv.IsValid() || !matchValue(d.MapIndex(k), sv, anyLayout) { + return false + } + } + return true + case reflect.Func, reflect.Chan, reflect.UnsafePointer: + return d.IsNil() == s.IsNil() + default: + return d.Equal(s) + } +} + +// reflectElem returns the struct a statement pointer points at, or the zero +// Value when it is not a pointer to a struct. +func reflectElem(v any) reflect.Value { + rv := reflect.ValueOf(v) + if rv.Kind() != reflect.Pointer || rv.IsNil() || rv.Elem().Kind() != reflect.Struct { + return reflect.Value{} + } + return rv.Elem() +} diff --git a/mdl/executor/flow_declared_match_test.go b/mdl/executor/flow_declared_match_test.go new file mode 100644 index 000000000..425e1b3b3 --- /dev/null +++ b/mdl/executor/flow_declared_match_test.go @@ -0,0 +1,107 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/visitor" +) + +func parseFlowBody(t *testing.T, body string) *ast.CreateMicroflowStmt { + t.Helper() + prog, errs := visitor.Build("create or modify microflow M.F ($In: String)\nbegin\n" + body + "\nend;\n") + if len(errs) > 0 { + t.Fatalf("parse: %v", errs[0]) + } + s, ok := prog.Statements[0].(*ast.CreateMicroflowStmt) + if !ok { + t.Fatalf("got %T", prog.Statements[0]) + } + return s +} + +// The stored side is what describe prints: geometry on every statement. +const storedFlowBody = ` @position(200, 200) + @curve(from: (30, 0), to: (-15, 0)) + @caption 'Say hello' + log info node 'N' 'hello'; + @position(400, 200) + if $In = 'x' then + @position(400, 300) + @anchor(from: bottom, to: top) + log info node 'N' 'x'; + end if;` + +func TestDeclaredMatches_OmittedGeometryIsNotADifference(t *testing.T) { + stored := parseFlowBody(t, storedFlowBody) + cases := []struct { + name string + body string + want bool + }{ + {"identical", storedFlowBody, true}, + {"no geometry at all", ` @caption 'Say hello' + log info node 'N' 'hello'; + if $In = 'x' then + log info node 'N' 'x'; + end if;`, true}, + {"a node moved", ` @position(210, 200) + @caption 'Say hello' + log info node 'N' 'hello'; + if $In = 'x' then + log info node 'N' 'x'; + end if;`, false}, + {"a caption dropped", ` log info node 'N' 'hello'; + if $In = 'x' then + log info node 'N' 'x'; + end if;`, false}, + {"a nested statement changed", ` @caption 'Say hello' + log info node 'N' 'hello'; + if $In = 'x' then + log info node 'N' 'y'; + end if;`, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + declared := parseFlowBody(t, tc.body) + if got := declaredMatches(declared.Body, stored.Body); got != tc.want { + t.Errorf("declaredMatches = %v, want %v", got, tc.want) + } + }) + } + + // The asymmetry: geometry the STORED side lacks is not a wildcard. A + // declared position against a stored flow drawn elsewhere is a move. + bare := parseFlowBody(t, ` @caption 'Say hello' + log info node 'N' 'hello';`) + placed := parseFlowBody(t, ` @position(200, 200) + @caption 'Say hello' + log info node 'N' 'hello';`) + if declaredMatches(placed.Body, bare.Body) { + t.Error("a declared @position matched a statement stored without one") + } +} + +func TestLCSStatements_PairsTheUnchangedRuns(t *testing.T) { + stored := parseFlowBody(t, ` log info node 'N' 'a'; + log info node 'N' 'b'; + log info node 'N' 'c'; + log info node 'N' 'd';`) + declared := parseFlowBody(t, ` log info node 'N' 'a'; + log info node 'N' 'new'; + log info node 'N' 'b'; + log info node 'N' 'C'; + log info node 'N' 'd';`) + got := lcsStatements(declared.Body, stored.Body) + want := [][2]int{{0, 0}, {2, 1}, {4, 3}} + if len(got) != len(want) { + t.Fatalf("pairs = %v, want %v", got, want) + } + for i := range want { + if got[i] != want[i] { + t.Fatalf("pairs = %v, want %v", got, want) + } + } +} diff --git a/mdl/executor/studiopro_roundtrip_test.go b/mdl/executor/studiopro_roundtrip_test.go index 9b1be58c4..718a2bca0 100644 --- a/mdl/executor/studiopro_roundtrip_test.go +++ b/mdl/executor/studiopro_roundtrip_test.go @@ -66,11 +66,6 @@ var studioProKnownLossy = map[string]string{ "page Administration.Account_Overview|document": "DataGrid2 object rebuilt from the " + "widget template (property order, column texts, filter captions), layout-grid " + "column weights 12 -> -1, tab-container nulls", - "nanoflow FeedbackModule.ACT_Feedback_UploadImage|document": "activity Size and the " + - "auto-caption 'Activity' are not authorable (upstream #884); a CaseValues-less " + - "flow gains a NoCase", - "nanoflow FeedbackModule.SUB_Feedback_GetOrCreate|document": "the flow builder regenerates merges " + - "(3 ExclusiveMerges -> 1, control flow equivalent), plus activity Size and auto-captions", "association Administration.AccountPasswordData_Account|document": "the domain-model rewrite drops empty " + "MemberAccess refs and NoGeneralization flags (the storage is carried since #704's flip became reachable)", diff --git a/mdl/roundtrip/allowlist_test.go b/mdl/roundtrip/allowlist_test.go index 019cc914b..9ca591be7 100644 --- a/mdl/roundtrip/allowlist_test.go +++ b/mdl/roundtrip/allowlist_test.go @@ -106,39 +106,6 @@ var knownFailures = map[string]knownFailure{ "menu Atlas_Core.Phone_Menu": {laws: []law{lawGetPut}, issue: "#721", why: "nested menu items dropped (#721 E)"}, "menu Atlas_Core.Tablet_Menu": {laws: []law{lawGetPut}, issue: "#721", why: "nested menu items dropped (#721 E)"}, - // #721 A: the microflow rebuild (ADR-0012 decision 3, plan 4.2g). - "microflow Administration.ChangeMyPassword": {laws: []law{lawGetPut, lawPutGet}, issue: "#721", why: "whole-document rebuild: curves, merges, case values (#721 A)"}, - "microflow Administration.ChangePassword": {laws: []law{lawGetPut, lawPutGet}, issue: "#721", why: "whole-document rebuild: curves, merges, case values (#721 A)"}, - "microflow Administration.ManageMyAccount": {laws: []law{lawGetPut, lawPutGet}, issue: "#721", why: "whole-document rebuild: curves, merges, case values (#721 A)"}, - "microflow Administration.NewAccount": {laws: []law{lawGetPut}, issue: "#721", why: "whole-document rebuild: curves, merges, case values (#721 A)"}, - "microflow Administration.NewWebServiceAccount": {laws: []law{lawGetPut}, issue: "#721", why: "whole-document rebuild: curves, merges, case values (#721 A)"}, - "microflow Administration.SaveNewAccount": {laws: []law{lawGetPut, lawPutGet}, issue: "#721", why: "whole-document rebuild: curves, merges, case values (#721 A)"}, - "microflow Administration.ShowMyPasswordForm": {laws: []law{lawGetPut}, issue: "#721", why: "whole-document rebuild: curves, merges, case values (#721 A)"}, - "microflow Administration.ShowPasswordForm": {laws: []law{lawGetPut}, issue: "#721", why: "whole-document rebuild: curves, merges, case values (#721 A)"}, - "microflow FeedbackModule.ConvertBase64String": {laws: []law{lawGetPut, lawPutGet}, issue: "#721", why: "whole-document rebuild: curves, merges, case values (#721 A)"}, - "microflow FeedbackModule.ConvertUUIDToURL": {laws: []law{lawGetPut}, issue: "#721", why: "whole-document rebuild: curves, merges, case values (#721 A)"}, - "microflow FeedbackModule.PopulateUserAttributes": {laws: []law{lawGetPut}, issue: "#721", why: "whole-document rebuild: curves, merges, case values (#721 A)"}, - "microflow FeedbackModule.SUB_Feedback_PostToAppInsights": {laws: []law{lawGetPut}, issue: "#721", why: "whole-document rebuild: curves, merges, case values (#721 A); putget fixed by #718 (expression whitespace)"}, - "microflow FeedbackModule.SUB_Feedback_Sanitize": {laws: []law{lawGetPut}, issue: "#721", why: "whole-document rebuild: curves, merges, case values (#721 A)"}, - "microflow FeedbackModule.SUB_Feedback_SendToServer": {laws: []law{lawGetPut, lawPutGet}, issue: "#721", why: "whole-document rebuild: curves, merges, case values (#721 A)"}, - "microflow FeedbackModule.VAL_Feedback": {laws: []law{lawGetPut, lawPutGet}, issue: "#721", why: "whole-document rebuild: curves, merges, case values (#721 A)"}, - "microflow MyFirstModule.MyFirstLogic": {laws: []law{lawGetPut}, issue: "#721", why: "whole-document rebuild: curves, merges, case values (#721 A)"}, - - // Nanoflows: #705 item 2 plus the rebuild in #721 A. - "nanoflow Atlas_Web_Content.ACT_Login": {laws: []law{lawGetPut, lawPutGet}, issue: "#705 #721", why: "export level and annotations (#705 item 2); whole-document rebuild (#721 A)"}, - "nanoflow Atlas_Web_Content.DS_LoginContext": {laws: []law{lawGetPut}, issue: "#705 #721", why: "export level and annotations (#705 item 2); whole-document rebuild (#721 A)"}, - "nanoflow FeedbackModule.ACT_Feedback_ClearForm": {laws: []law{lawGetPut}, issue: "#705 #721", why: "export level and annotations (#705 item 2); whole-document rebuild (#721 A)"}, - "nanoflow FeedbackModule.ACT_Feedback_ClearImage": {laws: []law{lawGetPut}, issue: "#705 #721", why: "export level and annotations (#705 item 2); whole-document rebuild (#721 A)"}, - "nanoflow FeedbackModule.ACT_Feedback_TriggerScreenshotMode": {laws: []law{lawGetPut, lawPutGet}, issue: "#705 #721", why: "export level and annotations (#705 item 2); whole-document rebuild (#721 A)"}, - "nanoflow FeedbackModule.ACT_Feedback_UploadImage": {laws: []law{lawGetPut, lawPutGet}, issue: "#705 #721", why: "export level and annotations (#705 item 2); whole-document rebuild (#721 A)"}, - "nanoflow FeedbackModule.ACT_Open_Feedback_Modal": {laws: []law{lawGetPut}, issue: "#705 #721", why: "export level and annotations (#705 item 2); whole-document rebuild (#721 A)"}, - "nanoflow FeedbackModule.ACT_SubmitFeedback": {laws: []law{lawGetPut, lawPutGet}, issue: "#705 #721", why: "export level and annotations (#705 item 2); whole-document rebuild (#721 A)"}, - "nanoflow FeedbackModule.DS_Feedback_Populate": {laws: []law{lawGetPut}, issue: "#705 #721", why: "export level and annotations (#705 item 2); whole-document rebuild (#721 A)"}, - "nanoflow FeedbackModule.Get_And_Set_Feedback_NPE": {laws: []law{lawGetPut, lawPutGet}, issue: "#705 #721", why: "export level and annotations (#705 item 2); whole-document rebuild (#721 A)"}, - "nanoflow FeedbackModule.OCH_Feedback_SaveToLocalStorage": {laws: []law{lawGetPut}, issue: "#705 #721", why: "export level and annotations (#705 item 2); whole-document rebuild (#721 A)"}, - "nanoflow FeedbackModule.SUB_Feedback_GetOrCreate": {laws: []law{lawGetPut, lawPutGet}, issue: "#705 #721", why: "export level and annotations (#705 item 2); whole-document rebuild (#721 A)"}, - "nanoflow FeedbackModule.SUB_Feedback_ResetLocalStorage": {laws: []law{lawGetPut}, issue: "#721", why: "whole-document rebuild (#721 A); export level and annotations fixed by #705"}, - // Pages: #705 item 1 plus #721 C. "page Administration.Account_Edit": {laws: []law{lawGetPut}, issue: "#705 #721", why: "texts and translations (#705 item 1); widget properties (#721 C)"}, "page Administration.Account_New": {laws: []law{lawGetPut}, issue: "#705 #721", why: "texts and translations (#705 item 1); widget properties (#721 C)"}, diff --git a/mdl/roundtrip/flow_modify_test.go b/mdl/roundtrip/flow_modify_test.go new file mode 100644 index 000000000..bae87bac1 --- /dev/null +++ b/mdl/roundtrip/flow_modify_test.go @@ -0,0 +1,394 @@ +// SPDX-License-Identifier: Apache-2.0 + +//go:build integration + +package roundtrip + +import ( + "bytes" + "strings" + "testing" + + "go.mongodb.org/mongo-driver/v2/bson" +) + +// ako/mxcli#747 (plan item 4.2g): `create or modify microflow` on an existing +// flow is diff-then-patch on top of the #739 splice. Every flow here is Studio +// Pro-authored (PedApp), because an mxcli-authored flow is laid out the way the +// rebuild lays it out and cannot show what the rebuild loses. + +const valFeedback = "microflow FeedbackModule.VAL_Feedback" + +// An unchanged describe -> create or modify writes nothing: the unit's bytes +// are identical. The controls below prove the same pipeline does write when +// the definition changes. +func TestFlowModify_UnchangedIsByteIdentical(t *testing.T) { + h := newHarness(t) + defer h.close() + + described := h.mustDescribe(t, valFeedback) + before := h.flowUnit(t, "VAL_Feedback") + for _, header := range []string{"", "mdl 1;\n"} { + if err := h.exec(header + described); err != nil { + t.Fatalf("header %q: exec unchanged describe output: %v", header, err) + } + if got := h.flowUnit(t, "VAL_Feedback"); !bytes.Equal(got, before) { + t.Fatalf("header %q: the unchanged definition rewrote the unit", header) + } + if changed := h.orig.diff(h.snapshot()); len(changed) != 0 { + t.Fatalf("header %q: the unchanged definition wrote: %s", header, strings.Join(changed, "; ")) + } + if !strings.Contains(h.out.String(), "Unchanged microflow: FeedbackModule.VAL_Feedback") { + t.Errorf("header %q: want an Unchanged report, got:\n%s", header, h.out.String()) + } + } +} + +// Control: an inserted statement is written, as a splice. Every element the +// stored unit had keeps its $ID and stays in the unit — including the merges +// describe cannot show and the rebuild deleted (#721 A) — and only the new +// activity and one new flow are added. +func TestFlowModify_InsertIsSpliced(t *testing.T) { + h := newHarness(t) + defer h.close() + + described := h.mustDescribe(t, valFeedback) + const anchor = " declare $ValidFeedback Boolean = true;\n" + const inserted = " log info node 'Feedback' 'validating feedback';\n" + edited := strings.Replace(described, anchor, anchor+inserted, 1) + if edited == described { + t.Fatalf("describe output has no %q — the fixture changed:\n%s", anchor, described) + } + before := h.flowUnit(t, "VAL_Feedback") + if err := h.exec(edited); err != nil { + t.Fatalf("exec edited definition: %v", err) + } + if !strings.Contains(h.out.String(), "Modified microflow: FeedbackModule.VAL_Feedback (spliced: 1 inserted)") { + t.Errorf("want a splice report, got:\n%s", h.out.String()) + } + after := h.flowUnit(t, "VAL_Feedback") + if bytes.Equal(after, before) { + t.Fatal("an inserted statement wrote nothing") + } + requireKept(t, before, after, "") + if added := len(elementIDs(t, after)) - len(elementIDs(t, before)); added <= 0 { + t.Errorf("the insert added %d elements", added) + } + if got, want := countType(t, after, "Microflows$ExclusiveMerge"), countType(t, before, "Microflows$ExclusiveMerge"); got != want { + t.Errorf("merges: %d after the insert, %d before — the rebuild ran", got, want) + } + again := h.mustDescribe(t, valFeedback) + if !strings.Contains(again, "log info node 'Feedback' 'validating feedback';") { + t.Errorf("describe does not show the inserted statement:\n%s", again) + } + + // Executing the new description writes nothing more: the splice's own + // output is a fixed point too. + settled := h.flowUnit(t, "VAL_Feedback") + if err := h.exec(again); err != nil { + t.Fatalf("exec the description after the insert: %v", err) + } + if !bytes.Equal(h.flowUnit(t, "VAL_Feedback"), settled) { + t.Error("re-executing the description of the spliced flow rewrote it") + } +} + +// A replaced statement that carries a shared annotation keeps the one stored +// note: the splice re-attaches it, and the declared note is not drawn again. +func TestFlowModify_ReplaceKeepsSharedNote(t *testing.T) { + h := newHarness(t) + defer h.close() + + const target = "microflow FeedbackModule.PopulateUserAttributes" + described := h.mustDescribe(t, target) + const old = "SubmitterDisplayName = $CurrentUser/Name)" + edited := strings.Replace(described, old, "SubmitterDisplayName = 'anonymous')", 1) + if edited == described { + t.Fatalf("describe output has no %q:\n%s", old, described) + } + before := h.flowUnit(t, "PopulateUserAttributes") + if err := h.exec(edited); err != nil { + t.Fatalf("exec edited definition: %v", err) + } + after := h.flowUnit(t, "PopulateUserAttributes") + if bytes.Equal(after, before) { + t.Fatal("a replaced statement wrote nothing") + } + if got := countType(t, after, "Microflows$Annotation"); got != 1 { + t.Errorf("%d annotation notes after the replace, want the 1 stored", got) + } + again := h.mustDescribe(t, target) + if !strings.Contains(again, "SubmitterDisplayName = 'anonymous'") || strings.Count(again, "@annotation(id: n1") != 2 { + t.Errorf("want the new statement with the shared note on both activities:\n%s", again) + } + // Only the replaced activity (and what it contains) is gone. + requireKept(t, before, after, "Microflows$ChangeAction") // ChangeObjectAction's storage name +} + +// Dropping a statement whose output only a statement changed in the same +// definition read: the scope check sees the flow as the replace left it, so +// the drop is not refused for a use that is about to go. +func TestFlowModify_DropAndReplace(t *testing.T) { + h := newHarness(t) + defer h.close() + + const target = "microflow FeedbackModule.SUB_Feedback_Sanitize" + described := h.mustDescribe(t, target) + edited := described + for _, cut := range []string{ + " @position(368, 200)\n @curve(from: (30, 0), to: (-30, 0))\n $SanitizedPageName = call java action FeedbackModule.XSS_Sanitizer(stringToSanitize = $Feedback/PageName);\n", + " PageName = $SanitizedPageName,", + } { + next := strings.Replace(edited, cut, "", 1) + if next == edited { + t.Fatalf("describe output has no %q:\n%s", cut, described) + } + edited = next + } + before := h.flowUnit(t, "SUB_Feedback_Sanitize") + if err := h.exec("mdl 1;\n" + edited); err != nil { + t.Fatalf("exec edited definition: %v", err) + } + if !strings.Contains(h.out.String(), "(spliced: 1 replaced, 1 dropped)") { + t.Errorf("want a splice report, got:\n%s", h.out.String()) + } + after := h.flowUnit(t, "SUB_Feedback_Sanitize") + if got, want := countType(t, after, "Microflows$JavaActionCallAction"), countType(t, before, "Microflows$JavaActionCallAction")-1; got != want { + t.Errorf("%d java action calls after, want %d", got, want) + } + again := h.mustDescribe(t, target) + if strings.Contains(again, "SanitizedPageName") { + t.Errorf("the dropped statement or its use is still described:\n%s", again) + } +} + +// A statement changed inside an `if` branch is spliced too: the branch's +// activities are nodes of the stored graph like any other. +func TestFlowModify_BranchEditIsSpliced(t *testing.T) { + h := newHarness(t) + defer h.close() + + described := h.mustDescribe(t, valFeedback) + const old = "message 'Subject is required';" + edited := strings.Replace(described, old, "message 'A subject is required';", 1) + if edited == described { + t.Fatalf("describe output has no %q:\n%s", old, described) + } + before := h.flowUnit(t, "VAL_Feedback") + for _, header := range []string{"mdl 1;\n", ""} { + if err := h.exec(header + edited); err != nil { + t.Fatalf("header %q: exec edited definition: %v", header, err) + } + } + after := h.flowUnit(t, "VAL_Feedback") + if bytes.Equal(after, before) { + t.Fatal("a statement changed in a branch wrote nothing") + } + // The one replaced activity may go; every other element stays. + if got, want := countType(t, after, "Microflows$ValidationFeedbackAction"), countType(t, before, "Microflows$ValidationFeedbackAction"); got != want { + t.Errorf("%d validation feedback actions after the replace, %d before", got, want) + } + requireKeptBut(t, before, after, "Microflows$ValidationFeedbackAction", "Subject is required") + if got, want := countType(t, after, "Microflows$ExclusiveMerge"), countType(t, before, "Microflows$ExclusiveMerge"); got != want { + t.Errorf("merges: %d after, %d before — the rebuild ran", got, want) + } + if !strings.Contains(h.mustDescribe(t, valFeedback), "'A subject is required'") { + t.Error("describe does not show the change") + } +} + +// A change the splice cannot make — here a node moved — is refused under +// mdl 1 with nothing written, and under mdl 0 still rebuilds, with the +// MDL-V1-REBUILD warning. +func TestFlowModify_UnspliceableChange(t *testing.T) { + h := newHarness(t) + defer h.close() + + described := h.mustDescribe(t, valFeedback) + const old = "@position(-390, 200)" + edited := strings.Replace(described, old, "@position(-380, 200)", 1) + if edited == described { + t.Fatalf("describe output has no %q:\n%s", old, described) + } + + err := h.exec("mdl 1;\n" + edited) + if err == nil || !strings.Contains(err.Error(), "cannot be spliced") || !strings.Contains(err.Error(), "moved") { + t.Fatalf("under mdl 1 want a refusal naming the move, got %v", err) + } + if changed := h.orig.diff(h.snapshot()); len(changed) != 0 { + t.Fatalf("the refused statement wrote: %s", strings.Join(changed, "; ")) + } + + if err := h.exec(edited); err != nil { + t.Fatalf("under mdl 0: %v", err) + } + if !strings.Contains(h.out.String(), "Warning [MDL-V1-REBUILD]") { + t.Errorf("under mdl 0 want the MDL-V1-REBUILD warning, got:\n%s", h.out.String()) + } + if !strings.Contains(h.mustDescribe(t, valFeedback), "@position(-380, 200)") { + t.Error("under mdl 0 the rebuild did not write the move") + } +} + +func (h *harness) mustDescribe(t *testing.T, target string) string { + t.Helper() + out, err := h.describe(target) + if err != nil || strings.TrimSpace(out) == "" { + t.Fatalf("describe %s: %v (output %q)", target, err, out) + } + return out +} + +// flowUnit returns the raw bytes of the microflow or nanoflow named name. +func (h *harness) flowUnit(t *testing.T, name string) []byte { + t.Helper() + for _, b := range h.snapshot().units { + typ, n := typeAndName(b) + if n == name && (typ == "Microflows$Microflow" || typ == "Microflows$Nanoflow") { + return b + } + } + t.Fatalf("flow %s not found", name) + return nil +} + +// requireKept fails when an element of before is missing from after, or is +// no longer the same element type. except names the action $Type of the one +// activity a replace is expected to take out: that activity and everything +// in it may go, nothing else. +func requireKept(t *testing.T, before, after []byte, except string) { + t.Helper() + a := elementIDs(t, after) + b := elementIDsExcept(t, before, except) + if except != "" && len(b) == len(elementIDs(t, before)) { + t.Fatalf("no activity holds a %s — the fixture changed", except) + } + for id, typ := range b { + got, ok := a[id] + switch { + case ok && got == typ: + case ok: + t.Errorf("$ID %s was a %s and is now a %s", uuidOf([]byte(id)), typ, got) + default: + t.Errorf("the %s with $ID %s is gone", typ, uuidOf([]byte(id))) + } + } +} + +// elementIDs maps every element $ID in a unit to its $Type. +func elementIDs(t *testing.T, raw []byte) map[string]string { + return elementIDsExcept(t, raw, "") +} + +// elementIDsExcept is elementIDs without the activities whose action is a +// skipAction, and without anything they contain. +func elementIDsExcept(t *testing.T, raw []byte, skipAction string) map[string]string { + t.Helper() + var doc bson.D + if err := bson.Unmarshal(raw, &doc); err != nil { + t.Fatalf("decode unit: %v", err) + } + out := map[string]string{} + var walk func(v any) + walk = func(v any) { + switch x := v.(type) { + case bson.D: + if skipAction != "" { + for _, e := range x { + if act, ok := e.Value.(bson.D); ok && e.Key == "Action" { + for _, ae := range act { + if ae.Key == "$Type" && ae.Value == skipAction { + return + } + } + } + } + } + var id, typ string + for _, e := range x { + switch e.Key { + case "$ID": + if b, ok := e.Value.(bson.Binary); ok { + id = string(b.Data) + } + case "$Type": + typ, _ = e.Value.(string) + default: + walk(e.Value) + } + } + if id != "" { + out[id] = typ + } + case bson.A: + for _, el := range x { + walk(el) + } + } + } + walk(doc) + return out +} + +// requireKeptBut is requireKept for a replace of the one activity whose +// action is a skipAction and whose subtree contains text. +func requireKeptBut(t *testing.T, before, after []byte, skipAction, text string) { + t.Helper() + var doc bson.D + if err := bson.Unmarshal(before, &doc); err != nil { + t.Fatalf("decode unit: %v", err) + } + gone := map[string]bool{} + var walk func(v any, inside bool) + walk = func(v any, inside bool) { + switch x := v.(type) { + case bson.D: + if !inside { + for _, e := range x { + if act, ok := e.Value.(bson.D); ok && e.Key == "Action" { + raw, _ := bson.Marshal(act) + for _, ae := range act { + if ae.Key == "$Type" && ae.Value == skipAction && bytes.Contains(raw, []byte(text)) { + inside = true + } + } + } + } + } + for _, e := range x { + if b, ok := e.Value.(bson.Binary); ok && e.Key == "$ID" && inside { + gone[string(b.Data)] = true + } + walk(e.Value, inside) + } + case bson.A: + for _, el := range x { + walk(el, inside) + } + } + } + walk(doc, false) + if len(gone) == 0 { + t.Fatalf("no %s holds %q — the fixture changed", skipAction, text) + } + a := elementIDs(t, after) + for id, typ := range elementIDs(t, before) { + if gone[id] { + continue + } + if got, ok := a[id]; !ok || got != typ { + t.Errorf("the %s with $ID %s is gone or changed type (now %q)", typ, uuidOf([]byte(id)), got) + } + } +} + +func countType(t *testing.T, raw []byte, typ string) int { + t.Helper() + n := 0 + for _, got := range elementIDs(t, raw) { + if got == typ { + n++ + } + } + return n +} From e521e2478d206d41d799c0e88a82d94556954baa Mon Sep 17 00:00:00 2001 From: Ako Date: Sun, 27 Sep 2026 15:36:06 +0000 Subject: [PATCH 5/7] executor: read the stored flow as mdl 0 when diffing under mdl 1 (#747) create or modify re-parsed the stored flow's description under the script's header. describe writes mdl 0, so under `mdl 1;` a stored line break described as `\n` read as a backslash and an n; a script stating exactly that then matched, reported Unchanged and wrote nothing, although the value it states differs from the stored one. The stored side is now described and parsed as mdl 0 always; the AST holds values, so it compares with a declared side parsed under any version. correctAmbiguousRanges applies in every case. Tests that exercised mdl 1 on VAL_Feedback relied on the misreading and now use a flow without a backslash escape for their mdl 1 leg; TestPedAppListActivitiesUnderMdl1 asserts the mdl 1 description of a flow of list activities is Unchanged. Co-Authored-By: Claude Opus 5.5 --- mdl/executor/cmd_flow_modify.go | 51 +++++++------- mdl/roundtrip/flow_modify_test.go | 102 ++++++++++++++++++++++------ mdl/roundtrip/list_activity_test.go | 3 + 3 files changed, 110 insertions(+), 46 deletions(-) diff --git a/mdl/executor/cmd_flow_modify.go b/mdl/executor/cmd_flow_modify.go index 2fdf0c369..ad17d3c31 100644 --- a/mdl/executor/cmd_flow_modify.go +++ b/mdl/executor/cmd_flow_modify.go @@ -190,29 +190,31 @@ func reportUnchanged(ctx *ExecContext, what string) { fmt.Fprint(ctx.Output, line) } -// describedFlowStmt describes the stored flow and parses the description -// under the language version of the script being run, so both sides are read -// by the same rules. See correctAmbiguousRanges for the one stored state the -// description cannot state under mdl 0. +// describedFlowStmt describes the stored flow and parses the description as +// mdl 0, the language describe writes, whatever the script's own header: the +// AST holds values (a string literal's text, not its spelling), so the stored +// side read by the rules it was written in compares with a declared side read +// by the script's. Re-parsing the description under the script's header +// instead misreads it wherever the two versions spell a value differently — a +// stored line break described as `\n` would read as a backslash and an n under +// mdl 1, agree with a script stating exactly that, and let the change pass as +// Unchanged. See correctAmbiguousRanges for the one stored state mdl 0 cannot +// state at all. func describedFlowStmt(ctx *ExecContext, d *flowDecl, a *alterFlowContext) (ast.Statement, error) { var buf bytes.Buffer - prev := ctx.Output - ctx.Output = &buf + prevOut, prevVer := ctx.Output, ctx.LanguageVersion + ctx.Output, ctx.LanguageVersion = &buf, langver.V0 var err error if d.nanoflow { err = describeNanoflow(ctx, d.name) } else { err = describeMicroflow(ctx, d.name) } - ctx.Output = prev + ctx.Output, ctx.LanguageVersion = prevOut, prevVer if err != nil { return nil, err } - src := buf.String() - if v := ctx.LanguageVersion; v > langver.V0 { - src = v.String() + ";\n" + src - } - prog, errs := visitor.Build(src) + prog, errs := visitor.Build(buf.String()) if len(errs) > 0 { return nil, fmt.Errorf("the description does not parse: %v", errs[0]) } @@ -229,18 +231,12 @@ func describedFlowStmt(ctx *ExecContext, d *flowDecl, a *alterFlowContext) (ast. default: continue } - if !limitOneIsListUnder(ctx.LanguageVersion) { - correctAmbiguousRanges(st, a.mf.ObjectCollection) - } + correctAmbiguousRanges(st, a.mf.ObjectCollection) return st, nil } return nil, fmt.Errorf("the description has no create statement") } -// limitOneIsListUnder reports whether `limit 1` reads as a list of one under v -// (the visitor's MDL-V1-LIMIT1 change). -func limitOneIsListUnder(v langver.Version) bool { return v >= langver.V1 } - // correctAmbiguousRanges fixes the stored side where its mdl 0 description // says something other than what is stored. // @@ -526,13 +522,8 @@ func (pd *patchDiff) gap(ins []ast.MicroflowStatement, stored []ast.MicroflowSta return pd.statements(d.ElseBody, s.ElseBody) } if sameIgnoringLayout(ins[0], del[0]) { - p := statementAnnotations(del[0]) - where := "" - if p != nil && p.Position != nil { - where = fmt.Sprintf(" at (%d, %d)", p.Position.X, p.Position.Y) - } - return cannotSplice("the %s%s is moved or its connectors are redrawn; the splice places new nodes only and does not move stored ones", - statementKind(del[0]), where) + return cannotSplice("the %s is moved or its connectors are redrawn; the splice places new nodes only and does not move stored ones", + describeAt(del[0])) } } cands := make([]mfmutator.Candidate, len(del)) @@ -578,6 +569,14 @@ func sameIfShell(declared, stored ast.MicroflowStatement) (*ast.IfStmt, *ast.IfS return d, s, true } +// describeAt names a stored statement and where it is drawn, for a message. +func describeAt(st ast.MicroflowStatement) string { + if p := statementAnnotations(st); p != nil && p.Position != nil { + return fmt.Sprintf("%s at (%d, %d)", statementKind(st), p.Position.X, p.Position.Y) + } + return statementKind(st) +} + // keepStoredNotes prepares the declared statements that replace a stored one. // The splice keeps the stored activity's annotation notes and attaches them to // the replacement's first activity, so the declared statement must carry the diff --git a/mdl/roundtrip/flow_modify_test.go b/mdl/roundtrip/flow_modify_test.go index bae87bac1..8944bf587 100644 --- a/mdl/roundtrip/flow_modify_test.go +++ b/mdl/roundtrip/flow_modify_test.go @@ -17,7 +17,10 @@ import ( // Pro-authored (PedApp), because an mxcli-authored flow is laid out the way the // rebuild lays it out and cannot show what the rebuild loses. -const valFeedback = "microflow FeedbackModule.VAL_Feedback" +const ( + valFeedback = "microflow FeedbackModule.VAL_Feedback" + sendToServer = "microflow FeedbackModule.SUB_Feedback_SendToServer" +) // An unchanged describe -> create or modify writes nothing: the unit's bytes // are identical. The controls below prove the same pipeline does write when @@ -26,24 +29,70 @@ func TestFlowModify_UnchangedIsByteIdentical(t *testing.T) { h := newHarness(t) defer h.close() - described := h.mustDescribe(t, valFeedback) - before := h.flowUnit(t, "VAL_Feedback") - for _, header := range []string{"", "mdl 1;\n"} { - if err := h.exec(header + described); err != nil { - t.Fatalf("header %q: exec unchanged describe output: %v", header, err) + // VAL_Feedback stores a line break in a message, which mdl 1 has no + // spelling for (its description's `\n` is a backslash and an n there; see + // MDL1ReadsStoredStringsAsStored), so its mdl 1 leg uses a flow without + // one: SUB_Feedback_SendToServer, with merges and an error handler. + for _, c := range []struct{ header, target, name string }{ + {"", valFeedback, "VAL_Feedback"}, + {"", sendToServer, "SUB_Feedback_SendToServer"}, + {"mdl 1;\n", sendToServer, "SUB_Feedback_SendToServer"}, + } { + described := h.mustDescribe(t, c.target) + before := h.flowUnit(t, c.name) + if err := h.exec(c.header + described); err != nil { + t.Fatalf("%s, header %q: exec unchanged describe output: %v", c.name, c.header, err) } - if got := h.flowUnit(t, "VAL_Feedback"); !bytes.Equal(got, before) { - t.Fatalf("header %q: the unchanged definition rewrote the unit", header) + if got := h.flowUnit(t, c.name); !bytes.Equal(got, before) { + t.Fatalf("%s, header %q: the unchanged definition rewrote the unit", c.name, c.header) } if changed := h.orig.diff(h.snapshot()); len(changed) != 0 { - t.Fatalf("header %q: the unchanged definition wrote: %s", header, strings.Join(changed, "; ")) + t.Fatalf("%s, header %q: the unchanged definition wrote: %s", c.name, c.header, strings.Join(changed, "; ")) } - if !strings.Contains(h.out.String(), "Unchanged microflow: FeedbackModule.VAL_Feedback") { - t.Errorf("header %q: want an Unchanged report, got:\n%s", header, h.out.String()) + if want := "Unchanged microflow: FeedbackModule." + c.name; !strings.Contains(h.out.String(), want) { + t.Errorf("%s, header %q: want an Unchanged report, got:\n%s", c.name, c.header, h.out.String()) } } } +// Under mdl 1 a backslash in a string is an ordinary character, so the +// description's `'…characters\n'` (mdl 0 for a line break, which is what +// VAL_Feedback stores) states a backslash and an n. The stored side must be +// read as stored — not by re-parsing its mdl 0 description under the script's +// header — or the two misreadings agree, the statement reports Unchanged and +// the value the script states is silently not written. +func TestFlowModify_MDL1ReadsStoredStringsAsStored(t *testing.T) { + h := newHarness(t) + defer h.close() + + described := h.mustDescribe(t, valFeedback) + const mdl0 = "characters\\n';" + if !strings.Contains(described, mdl0) { + t.Fatalf("describe output has no %q — the fixture changed:\n%s", mdl0, described) + } + // Control: under mdl 0 the same text is the stored value. + if err := h.exec(described); err != nil { + t.Fatalf("mdl 0: %v", err) + } + if changed := h.orig.diff(h.snapshot()); len(changed) != 0 { + t.Fatalf("mdl 0: the unchanged definition wrote: %s", strings.Join(changed, "; ")) + } + + before := h.flowUnit(t, "VAL_Feedback") + if err := h.exec("mdl 1;\n" + described); err != nil { + t.Fatalf("mdl 1: %v", err) + } + if bytes.Equal(h.flowUnit(t, "VAL_Feedback"), before) { + t.Fatal("mdl 1: a string that states another value than the stored one wrote nothing") + } + if !strings.Contains(h.out.String(), "(spliced: 1 replaced)") { + t.Errorf("mdl 1: want the one statement replaced by a splice, got:\n%s", h.out.String()) + } + if again := h.mustDescribe(t, valFeedback); !strings.Contains(again, "characters\\\\n';") { + t.Errorf("mdl 1: want the backslash stored as written:\n%s", again) + } +} + // Control: an inserted statement is written, as a splice. Every element the // stored unit had keeps its $ID and stays in the unit — including the merges // describe cannot show and the rebuild deleted (#721 A) — and only the new @@ -174,11 +223,15 @@ func TestFlowModify_BranchEditIsSpliced(t *testing.T) { if edited == described { t.Fatalf("describe output has no %q:\n%s", old, described) } + // mdl 0 only: under mdl 1 this flow's description also restates its + // stored line break as a backslash and an n (MDL1ReadsStoredStringsAsStored + // covers a branch splice under mdl 1). before := h.flowUnit(t, "VAL_Feedback") - for _, header := range []string{"mdl 1;\n", ""} { - if err := h.exec(header + edited); err != nil { - t.Fatalf("header %q: exec edited definition: %v", header, err) - } + if err := h.exec(edited); err != nil { + t.Fatalf("exec edited definition: %v", err) + } + if !strings.Contains(h.out.String(), "(spliced: 1 replaced)") { + t.Errorf("want a splice report, got:\n%s", h.out.String()) } after := h.flowUnit(t, "VAL_Feedback") if bytes.Equal(after, before) { @@ -204,13 +257,15 @@ func TestFlowModify_UnspliceableChange(t *testing.T) { h := newHarness(t) defer h.close() - described := h.mustDescribe(t, valFeedback) - const old = "@position(-390, 200)" - edited := strings.Replace(described, old, "@position(-380, 200)", 1) + // mdl 1: refused, nothing written. (On SUB_Feedback_SendToServer, whose + // description means under mdl 1 what it means under mdl 0; see + // UnchangedIsByteIdentical.) + described := h.mustDescribe(t, sendToServer) + const oldV1 = "@position(-730, -50)" + edited := strings.Replace(described, oldV1, "@position(-720, -50)", 1) if edited == described { - t.Fatalf("describe output has no %q:\n%s", old, described) + t.Fatalf("describe output has no %q:\n%s", oldV1, described) } - err := h.exec("mdl 1;\n" + edited) if err == nil || !strings.Contains(err.Error(), "cannot be spliced") || !strings.Contains(err.Error(), "moved") { t.Fatalf("under mdl 1 want a refusal naming the move, got %v", err) @@ -219,6 +274,13 @@ func TestFlowModify_UnspliceableChange(t *testing.T) { t.Fatalf("the refused statement wrote: %s", strings.Join(changed, "; ")) } + // mdl 0: rebuilt, with the warning. + described = h.mustDescribe(t, valFeedback) + const old = "@position(-390, 200)" + edited = strings.Replace(described, old, "@position(-380, 200)", 1) + if edited == described { + t.Fatalf("describe output has no %q:\n%s", old, described) + } if err := h.exec(edited); err != nil { t.Fatalf("under mdl 0: %v", err) } diff --git a/mdl/roundtrip/list_activity_test.go b/mdl/roundtrip/list_activity_test.go index cb3b55fc2..c5477da30 100644 --- a/mdl/roundtrip/list_activity_test.go +++ b/mdl/roundtrip/list_activity_test.go @@ -73,6 +73,9 @@ func TestPedAppListActivitiesUnderMdl1(t *testing.T) { if err := h.exec("mdl 1;\n" + first); err != nil { t.Fatalf("exec the mdl 1 description: %v\n%s", err, first) } + if !strings.Contains(h.out.String(), "Unchanged microflow: MyFirstModule.ListActivities") { + t.Errorf("GetPut: the mdl 1 description of an unchanged flow must write nothing, got:\n%s", h.out.String()) + } if again := h.describeUnder("mdl 1;", listActivityTarget); again != first { t.Errorf("describe -> exec -> describe changed the microflow:\n%s", lineDiff(first, again)) } From c6b1f0e9296952856535b9f0190f184527b5c163 Mon Sep 17 00:00:00 2001 From: Ako Date: Sun, 27 Sep 2026 15:36:10 +0000 Subject: [PATCH 6/7] executor: refuse a change inside a loop body instead of replacing the loop (#747) A declared loop that differed from the stored one only inside its body became a replace of the whole loop: every node in it rebuilt with new element IDs, and merges describe cannot show dropped - the rebuild's loss, confined to the loop, and under mdl 1 without the refusal the design and the skills promise for it. It is now a change the splice cannot make: refused under mdl 1, the warned rebuild under mdl 0. A changed loop iteration is still a replace. Co-Authored-By: Claude Opus 5.5 --- mdl/executor/cmd_flow_modify.go | 33 ++++++++++++++++ mdl/roundtrip/flow_modify_test.go | 66 +++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+) diff --git a/mdl/executor/cmd_flow_modify.go b/mdl/executor/cmd_flow_modify.go index ad17d3c31..eb8c89e0c 100644 --- a/mdl/executor/cmd_flow_modify.go +++ b/mdl/executor/cmd_flow_modify.go @@ -521,6 +521,14 @@ func (pd *patchDiff) gap(ins []ast.MicroflowStatement, stored []ast.MicroflowSta } return pd.statements(d.ElseBody, s.ElseBody) } + if sameLoopShell(ins[0], del[0]) { + // The splice does not edit inside a loop, and the engine would + // take this as a replace of the whole loop: every node in it + // rebuilt, renumbered and redrawn — the rebuild's loss, confined + // to the loop but no less silent. + return cannotSplice("the %s changes inside its body; the splice does not edit inside a loop, "+ + "and replacing the whole loop would rebuild every node it holds", describeAt(del[0])) + } if sameIgnoringLayout(ins[0], del[0]) { return cannotSplice("the %s is moved or its connectors are redrawn; the splice places new nodes only and does not move stored ones", describeAt(del[0])) @@ -569,6 +577,31 @@ func sameIfShell(declared, stored ast.MicroflowStatement) (*ast.IfStmt, *ast.IfS return d, s, true } +// sameLoopShell reports whether two statements are the same loop — the same +// kind and iteration — differing at most in its body and its geometry. (A loop +// moved as well as edited inside is still a change inside the loop.) +func sameLoopShell(declared, stored ast.MicroflowStatement) bool { + switch d := declared.(type) { + case *ast.LoopStmt: + s, ok := stored.(*ast.LoopStmt) + if !ok { + return false + } + dShell, sShell := *d, *s + dShell.Body, sShell.Body = nil, nil + return sameIgnoringLayout(&dShell, &sShell) + case *ast.WhileStmt: + s, ok := stored.(*ast.WhileStmt) + if !ok { + return false + } + dShell, sShell := *d, *s + dShell.Body, sShell.Body = nil, nil + return sameIgnoringLayout(&dShell, &sShell) + } + return false +} + // describeAt names a stored statement and where it is drawn, for a message. func describeAt(st ast.MicroflowStatement) string { if p := statementAnnotations(st); p != nil && p.Position != nil { diff --git a/mdl/roundtrip/flow_modify_test.go b/mdl/roundtrip/flow_modify_test.go index 8944bf587..d80fd6881 100644 --- a/mdl/roundtrip/flow_modify_test.go +++ b/mdl/roundtrip/flow_modify_test.go @@ -292,6 +292,72 @@ func TestFlowModify_UnspliceableChange(t *testing.T) { } } +// A change inside a loop body is not a splice: the engine does not edit +// inside a loop, and replacing the whole loop would renumber and redraw every +// node it holds — the rebuild's loss, confined to the loop but just as silent. +// So under mdl 1 it is refused, and under mdl 0 it takes the warned rebuild. +// Control: a change after the loop in the same flow is spliced. +func TestFlowModify_LoopBodyChangeIsNotSpliced(t *testing.T) { + h := newHarness(t) + defer h.close() + + // PedApp has no loop, so this flow is mxcli-authored; the test is about + // what is refused, not about identity. + const create = `create microflow MyFirstModule.LoopFlow ( + $Items: List of FeedbackModule.Feedback +) +returns Integer +begin + declare $N Integer = 0; + loop $It in $Items begin + if $It/Subject != empty then + set $N = $N + 1; + end if; + log info node 'X' 'in loop'; + end loop; + log info node 'X' 'after loop'; + return $N; +end;` + if err := h.exec(create); err != nil { + t.Fatalf("create: %v", err) + } + const target = "microflow MyFirstModule.LoopFlow" + described := h.mustDescribe(t, target) + inLoop := strings.Replace(described, "'in loop'", "'in the loop'", 1) + if inLoop == described { + t.Fatalf("describe output has no 'in loop':\n%s", described) + } + before := h.flowUnit(t, "LoopFlow") + + err := h.exec("mdl 1;\n" + inLoop) + if err == nil || !strings.Contains(err.Error(), "cannot be spliced") || !strings.Contains(err.Error(), "loop") { + t.Fatalf("under mdl 1 want a refusal naming the loop, got %v", err) + } + if !bytes.Equal(h.flowUnit(t, "LoopFlow"), before) { + t.Fatal("the refused statement wrote") + } + + if err := h.exec(inLoop); err != nil { + t.Fatalf("under mdl 0: %v", err) + } + if !strings.Contains(h.out.String(), "Warning [MDL-V1-REBUILD]") { + t.Errorf("under mdl 0 want the MDL-V1-REBUILD warning, got:\n%s", h.out.String()) + } + if !strings.Contains(h.mustDescribe(t, target), "'in the loop'") { + t.Error("under mdl 0 the rebuild did not write the change") + } + + // Control: after the loop, the same kind of change is spliced. + described = h.mustDescribe(t, target) + after := strings.Replace(described, "'after loop'", "'after the loop'", 1) + if err := h.exec("mdl 1;\n" + after); err != nil { + t.Fatalf("control under mdl 1: %v", err) + } + if !strings.Contains(h.out.String(), "(spliced: 1 replaced)") { + t.Errorf("control: want a splice, got:\n%s", h.out.String()) + } +} + func (h *harness) mustDescribe(t *testing.T, target string) string { t.Helper() out, err := h.describe(target) From 9034b8ec8cac617ae979be36d1600b3a405b44b9 Mon Sep 17 00:00:00 2001 From: Ako Date: Sun, 27 Sep 2026 16:10:41 +0000 Subject: [PATCH 7/7] skills: condense write-microflows' Studio Pro guidance to stay within the 700-line skill budget Co-Authored-By: Claude Opus 5.5 --- .claude/skills/mendix/write-microflows/SKILL.md | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/.claude/skills/mendix/write-microflows/SKILL.md b/.claude/skills/mendix/write-microflows/SKILL.md index 4f05786ab..2b31169d5 100644 --- a/.claude/skills/mendix/write-microflows/SKILL.md +++ b/.claude/skills/mendix/write-microflows/SKILL.md @@ -44,14 +44,10 @@ Choose the mode by who owns the microflow ([choose-edit-mode](../choose-edit-mod (or fresh `describe` output) and re-run `create or modify`. - **Authored in Studio Pro:** prefer `alter microflow X { insert/replace/drop … }` (targets from `describe microflow X with handles`). `create or modify` of `describe` - output also works as a patch: an unchanged definition writes nothing, and an inserted, - replaced or dropped statement (at the top level or in an `if` branch) is spliced in, - leaving every other node, merge and curve as stored. A change it cannot splice — the - header, anything inside a loop body or error handler, a moved node — rebuilds the - whole microflow under mdl 0 - (warning `MDL-V1-REBUILD`: element IDs renumbered, merges removed, curves reset) and - is refused under `mdl 1;` (header and loop-body changes have no splice yet; move - nodes in Studio Pro). + output patches too: unchanged writes nothing; a top-level or `if`-branch statement + change is spliced in. Other changes (header, loop body, error handler, moved node) + rebuild the flow under mdl 0 (`MDL-V1-REBUILD`: IDs renumbered, merges and curves + lost) and are refused under `mdl 1;`. ## When to Use a Microflow vs a Nanoflow