diff --git a/.claude/skills/mendix/write-microflows/SKILL.md b/.claude/skills/mendix/write-microflows/SKILL.md index ac77a04dd..2b31169d5 100644 --- a/.claude/skills/mendix/write-microflows/SKILL.md +++ b/.claude/skills/mendix/write-microflows/SKILL.md @@ -42,12 +42,12 @@ 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 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 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/.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 diff --git a/mdl/executor/cmd_alter_flow.go b/mdl/executor/cmd_alter_flow.go index d8aac4309..7af925184 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 @@ -115,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 @@ -161,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) { @@ -487,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..eb8c89e0c --- /dev/null +++ b/mdl/executor/cmd_flow_modify.go @@ -0,0 +1,847 @@ +// 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 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 + 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, ctx.LanguageVersion = prevOut, prevVer + if err != nil { + return nil, err + } + prog, errs := visitor.Build(buf.String()) + 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 + } + correctAmbiguousRanges(st, a.mf.ObjectCollection) + return st, nil + } + return nil, fmt.Errorf("the description has no create statement") +} + +// 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 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])) + } + } + 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 +} + +// 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 { + 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 +// 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..d80fd6881 --- /dev/null +++ b/mdl/roundtrip/flow_modify_test.go @@ -0,0 +1,522 @@ +// 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" + 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 +// the definition changes. +func TestFlowModify_UnchangedIsByteIdentical(t *testing.T) { + h := newHarness(t) + defer h.close() + + // 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, 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("%s, header %q: the unchanged definition wrote: %s", c.name, c.header, strings.Join(changed, "; ")) + } + 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 +// 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) + } + // 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") + 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) { + 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() + + // 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", 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) + } + if changed := h.orig.diff(h.snapshot()); len(changed) != 0 { + 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) + } + 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") + } +} + +// 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) + 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 +} 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)) } 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) }