diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index e3cbf1eda..58a93358a 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -16,6 +16,12 @@ to the symptom table below, so the next similar issue costs fewer reads. | Symptom | Root cause layer | First file to open | Fix pattern | |---------|-----------------|-------------------|-------------| +| `mxcli test` leaves the project mutated: Security Level changed, after-startup pointing at the deleted `MxTest.TestRunner`, and only a `Warning:` line about it. Or: an empty `MxTest` module accumulates after every run | Three defects in one teardown. (a) `getAfterStartup` trimmed quotes *before* trailing punctuation, so a DESCRIBE SETTINGS line ending in `,` yielded `Module.Flow',` and the restore statement was unparseable; (b) cleanup dropped only the microflow, not the module it created; (c) the Security Level was forced OFF and restored to a hardcoded PRODUCTION | `cmd/mxcli/testrunner/runner.go` (`parseSettingValue`, `quoteMDLString`, `projectState`, `setupCommands`, `cleanupCommands`) | Strip trailing `,;` before unquoting; re-emit via `quoteMDLString` (doubling embedded quotes, never backslashes); capture a `projectState` before the first mutation and restore from it; drop the module only when the run created it (a pre-existing `MxTest` is the user's); leave Security Level alone entirely; return cleanup errors instead of printing warnings, and fail the run when they occur. Command lists are pure functions so the restore is testable without a project or Docker. Issues #802/#803/#804 | +| `create or modify external entities` silently resets a per-entity setting the user had changed (e.g. allow-create-change-locally) | `applyExternalEntityFields` stamps every field on both the create and the update path, so anything not derivable from the OData contract was overwritten with a default | `mdl/executor/cmd_contract.go` (`applyExternalEntityFields`) | Separate contract-derived fields (Countable/Creatable/Deletable/Skip/Top — refresh from metadata) from local modelling choices (CreateChangeLocally — leave alone; a new entity arrives zero-valued, which is Mendix's default). Issue #782 | +| An **external (OData) entity** loses its remote settings on any read-modify-write — `describe external entity` says `is not an external entity (source: )`, and `alter entity … set allow_create_change_locally = true` reports success but the flag stays off. Works under `--engine legacy` | `entityFromGen` recognised only `DomainModels$OqlViewEntitySource`, so the three `Rest$OData*` sources read back as no source at all: `Source` empty, every remote field zeroed. The write path was fine — it switches on `e.Source`, which the read never populated | `mdl/backend/modelsdk/domainmodel.go` (`entityFromGen`'s source switch, `odataKeyFromGen`) | Mirror the legacy parser (`sdk/mpr/parser_domainmodel.go`) for all three flavours: RemoteEntitySource (capabilities + CreateChangeLocally + key), EntityTypeSource (type name + IsOpen + key), PrimitiveCollectionEntitySource (service only). `Updatable` has no gen accessor and the writer does not emit it — leaving it zero is symmetric. **Check the read side first when a write-path field "does not stick"**: a switch on a field the read never fills looks like a write bug. Repro `mdl-examples/bug-tests/782-external-entity-create-change-locally.mdl`. Issue #782 | +| A pluggable widget's datasource (or child widget, or client action) is **silently dropped at write time** — `exec` prints `Created page` but the widget lands with the piece missing, and only a `log` line mentions `not yet supported — rerun with MXCLI_ENGINE=legacy` | The converter *does* return an error, but it travels through `widgetobj.ChildSerializer`, whose methods return BSON with no error channel (the `TODO(shared-types)` in `mdl/backend/widgetobj/builder.go`), so the caller logged it and returned nil | `mdl/backend/modelsdk/widget_pluggable_write.go` (`recordChildSerializeErr`, `takeChildSerializeErr`) + the drains in `page_write.go` / `snippet_write.go` | Record the failure in a package-level accumulator and drain it at every page/snippet write entry point, so the statement fails instead of the write succeeding with data missing (ADR-0004: refuse, don't drop). The real fix is the deferred `ChildSerializer` interface change; until then, any **new** write entry point that builds pluggable widgets must drain too. Repro `mdl-examples/bug-tests/795b-flow-datasource-context-entity.mdl` | +| `describe page` reports the wrong context entity under a data container bound to a microflow/nanoflow — `-- Context: $currentObject (Module.GetOrders)` names the *flow* instead of the entity it returns | `widget.EntityContext = widget.DataSource.Reference` is correct for a database source (reference *is* the entity) and wrong for a flow source (reference is the flow's qualified name) | `mdl/executor/cmd_pages_describe_flowcontext.go` (`dataSourceEntityContext`, `flowReturnEntity`) + the five assignment sites in `cmd_pages_describe_parse.go` | Resolve the flow's return type via `ListMicroflows`/`ListNanoflows` + `getHierarchy().GetQualifiedName`, taking the entity from an Object/List return type; fall back to the reference when the flow is unresolvable or returns a scalar, so the result is never worse than before. Note `GetRawUnitByName` is unimplemented on the modelsdk engine — the list+hierarchy path is the one that works | +| `describe page` omits a **pluggable** widget's `DataSource` when it is bound to a microflow (`datagrid g1 {` with no DataSource), while a `database from` source describes fine — re-applying the output recreates the grid unbound | A `Forms$MicroflowSource` stores the name in the nested `Forms$MicroflowSettings` (`MicroflowSettings` → `Microflow`), which is what the write path and Studio Pro emit; the reader looked up a top-level `Microflow` key, got `""`, and returned no datasource. The describe *formatter* was correct all along — read bug only | `mdl/executor/cmd_pages_describe_pluggable.go` (`microflowSourceRef`, `nanoflowSourceRef`, `extractDataGrid2DataSource`, `extractGalleryDataSource`, `parseCustomWidgetDataSource`) | Read the nested settings with a top-level fallback, via one shared helper — there were four divergent copies of this lookup and two were wrong. `Forms$NanoflowSource` was missing entirely from the DataGrid2/Gallery switches; add it alongside. Do **not** touch `CustomWidgets$CustomWidgetNanoflowSource` (a different metamodel type whose `Nanoflow` really is top-level) or the `Forms$MicroflowAction` reads (actions, not datasources). Repro `mdl-examples/bug-tests/795-datagrid-microflow-datasource-describe.mdl`. Issue #795 | | Any `ALTER SETTINGS` (any section) reports success but the Default configuration's **Custom settings** are gone, **Tracing** is reset, and every **constant override** shows blank in Studio Pro — Integer/Long constants then fail the build | The configuration was re-serialized from `model.ServerConfiguration`, which carries only the modelled fields, so CustomSettings/Tracing/OpenAdminPort/OpenHttpPort were dropped, the list version markers downgraded 3→2, and overrides were written with a flat `Value` instead of the nested `SharedOrPrivateValue` the platform reads | `mdl/settingsoverlay/settingsoverlay.go` (`Configurations`, `ServerConfiguration`, `ConstantValues`) — called by both `mdl/backend/modelsdk/settings_write.go` and `sdk/mpr/writer_settings.go` | Overlay onto the raw document instead of rebuilding: write only the fields the read path populates, take each list's marker from what is stored, and update a constant override in the slot it already occupies (nested if nested, flat if flat). New override → nested. New configuration → clone a sibling's shape, empty its collections, mint a fresh `$ID`. Refuse the write when `RawParts` is empty. Repro `mdl-examples/bug-tests/801-alter-settings-preserves-configuration.mdl`. Issue #801 | | `ALTER SETTINGS` / `CREATE CONFIGURATION` prints "Updated …" but `DESCRIBE SETTINGS` shows the old value — an Integer property was given a non-numeric value, or a Boolean anything other than `true` | `strconv.Atoi`'s error was discarded (`if v, err := …; err == nil`) so the assignment was skipped while the caller still printed success; the boolean form compared against `"true"`, silently mapping every other spelling to false | `mdl/executor/cmd_settings.go` (`settingsInt`, `settingsBool`) + `mdl/executor/validate_settings.go` (`typedSettingsKeys`, MDL-SET01/MDL-SET02) | Parse through a helper that returns a validation error naming the setting and the offending value, and register the property in `typedSettingsKeys` so `mxcli check` and the LSP flag it before the project is opened for writing. `TestTypedSettingsKeys_MatchExecutor` guards the table against drifting from the executor's switch. Repro `mdl-examples/bug-tests/805-alter-settings-typed-values.fail.mdl`. Issue #805 | | MCP op reports `MCP error -32000: Request timed out` but the page/document/entity EXISTS in Studio Pro afterwards | Studio Pro's ~30s server-side per-call limit fires while the op still applies — a client false failure, not a server rejection | `mdl/backend/mcp/timeout.go` (`isTimeoutErr`, `timeoutVerifyDelay`, `pedUpdateVerify`, `pedDocumentExists`) | Verify-on-timeout, never blind-retry a non-idempotent op: idempotent root-replace (`pgWritePage`) retries once; creates confirm via `ped_find_document`; entity adds confirm via a shallow `/entities` read. Unverified → error with save-before-re-run guidance (a blind re-run from a fresh session duplicates elements) | diff --git a/.claude/skills/mendix/create-page.md b/.claude/skills/mendix/create-page.md index ec1e931fa..302fe8d92 100644 --- a/.claude/skills/mendix/create-page.md +++ b/.claude/skills/mendix/create-page.md @@ -444,8 +444,8 @@ column colActions (caption: 'Actions') { |--------|-------------| | `datasource: database from Module.Entity` | Direct database query | | `datasource: $Variable` | Variable bound (requires DATAVIEW parent with entity) | -| `datasource: microflow Module.GetData()` | Microflow datasource | -| `datasource: nanoflow Module.GetData()` | Nanoflow datasource (client-side, no server roundtrip) | +| `datasource: microflow Module.GetData` | Microflow datasource — no `()`, the name alone | +| `datasource: nanoflow Module.GetData` | Nanoflow datasource (client-side, no server roundtrip) — no `()` | | `datasource: selection widgetName` | Listen to selection from another widget | | `datasource: association path` | Retrieve by association from context (ByAssociation) | | `datasource: $currentObject/Module.Assoc` | Sugar for `association` — same semantics, reads more naturally | diff --git a/.claude/skills/mendix/test-microflows.md b/.claude/skills/mendix/test-microflows.md index f5a89dd2d..90420179a 100644 --- a/.claude/skills/mendix/test-microflows.md +++ b/.claude/skills/mendix/test-microflows.md @@ -110,13 +110,23 @@ mxcli test tests/ -p app.mpr --verbose The test runner uses the **after-startup microflow** pattern: 1. Parses test files and extracts test blocks with annotations -2. Generates a `MxTest.TestRunner` microflow with assertion logic -3. Sets security OFF and after-startup to `MxTest.TestRunner` +2. Records the project's current after-startup microflow, and whether an `MxTest` + module already exists +3. Generates a `MxTest.TestRunner` microflow with assertion logic and points + after-startup at it 4. Builds the project and restarts the Docker runtime 5. Captures structured `MXTEST:` log lines for pass/fail -6. Restores original security and after-startup settings +6. Restores the original after-startup setting and removes the generated runner — + the whole `MxTest` module when the runner created it, otherwise just the + `TestRunner` microflow 7. Outputs results (console, JUnit XML) +The project's **Security Level is not modified**. The after-startup microflow runs +in an administrative context and is not subject to it, and forcing it off breaks +projects whose published REST/OData services use custom authentication. If a +cleanup step fails the run reports an error and names what was left changed — +the project is modified, so it must not read as a clean pass. + --- ## Writing Good Tests diff --git a/cmd/mxcli/testrunner/runner.go b/cmd/mxcli/testrunner/runner.go index 1ed4d6ff4..f0d35c25e 100644 --- a/cmd/mxcli/testrunner/runner.go +++ b/cmd/mxcli/testrunner/runner.go @@ -6,6 +6,8 @@ import ( "bufio" "bytes" "context" + "encoding/json" + "errors" "fmt" "io" "os" @@ -17,6 +19,13 @@ import ( "github.com/mendixlabs/mxcli/cmd/mxcli/docker" ) +const ( + // mxTestModule is the module Run generates the test runner into. + mxTestModule = "MxTest" + // mxTestRunner is the generated after-startup microflow. + mxTestRunner = "MxTest.TestRunner" +) + // RunOptions configures the test runner. type RunOptions struct { // ProjectPath is the path to the .mpr file. @@ -94,9 +103,12 @@ func Run(opts RunOptions) (*SuiteResult, error) { // Step 3: Save original settings and inject test runner fmt.Fprintln(w, "Injecting test runner into project...") - origAfterStartup, err := getAfterStartup(opts.ProjectPath) + // Capture what cleanup will need to restore, before touching anything. This + // must succeed: without it cleanup cannot tell an existing MxTest module from + // the one it is about to create, nor restore the original after-startup. + state, err := captureProjectState(opts.ProjectPath) if err != nil { - fmt.Fprintf(w, " Warning: could not read original after-startup setting: %v\n", err) + return nil, fmt.Errorf("capturing project state: %w", err) } // Write the runner MDL to a temp file and execute it @@ -118,28 +130,25 @@ func Run(opts RunOptions) (*SuiteResult, error) { return nil, fmt.Errorf("injecting test runner: %w", err) } - // Set security OFF for testing - if err := execMxcliCmd(opts.ProjectPath, "ALTER PROJECT SECURITY LEVEL OFF"); err != nil { - fmt.Fprintf(w, " Warning: could not set security OFF: %v\n", err) - } - // Set after-startup microflow - if err := execMxcliCmd(opts.ProjectPath, "ALTER SETTINGS MODEL AfterStartupMicroflow = 'MxTest.TestRunner'"); err != nil { - return nil, fmt.Errorf("setting after-startup: %w", err) + for _, cmd := range setupCommands() { + if err := execMxcliCmd(opts.ProjectPath, cmd); err != nil { + return nil, fmt.Errorf("preparing project for the test run (%s): %w", cmd, err) + } } - fmt.Fprintln(w, " After-startup set to MxTest.TestRunner") + fmt.Fprintf(w, " After-startup set to %s\n", mxTestRunner) // Step 4: Build and restart dockerDir := filepath.Join(filepath.Dir(opts.ProjectPath), ".docker") if err := ensureDockerStack(opts.ProjectPath, dockerDir, w); err != nil { - cleanup(opts.ProjectPath, origAfterStartup, w) + reportCleanup(w, cleanup(opts.ProjectPath, state, w)) return nil, fmt.Errorf("docker init: %w", err) } if !opts.SkipBuild { fmt.Fprintln(w, "Building project...") if err := execMxcli(opts.ProjectPath, "docker", "build", "-p", opts.ProjectPath, "--skip-check"); err != nil { - cleanup(opts.ProjectPath, origAfterStartup, w) + reportCleanup(w, cleanup(opts.ProjectPath, state, w)) return nil, fmt.Errorf("docker build: %w", err) } } @@ -149,7 +158,7 @@ func Run(opts RunOptions) (*SuiteResult, error) { runCompose(dockerDir, "down") // Start fresh if err := runCompose(dockerDir, "up", "--detach", "--force-recreate"); err != nil { - cleanup(opts.ProjectPath, origAfterStartup, w) + reportCleanup(w, cleanup(opts.ProjectPath, state, w)) return nil, fmt.Errorf("docker up: %w", err) } @@ -157,7 +166,7 @@ func Run(opts RunOptions) (*SuiteResult, error) { fmt.Fprintf(w, "Waiting for test execution (timeout: %s)...\n", timeout) logOutput, err := captureRuntimeLogs(dockerDir, timeout, w, opts.Verbose) if err != nil { - cleanup(opts.ProjectPath, origAfterStartup, w) + reportCleanup(w, cleanup(opts.ProjectPath, state, w)) return nil, fmt.Errorf("runtime execution: %w", err) } @@ -167,7 +176,8 @@ func Run(opts RunOptions) (*SuiteResult, error) { // Step 7: Cleanup fmt.Fprintln(w, "Cleaning up...") - cleanup(opts.ProjectPath, origAfterStartup, w) + cleanupErr := cleanup(opts.ProjectPath, state, w) + reportCleanup(w, cleanupErr) // Step 8: Output results PrintResults(w, result, opts.Color) @@ -185,6 +195,11 @@ func Run(opts RunOptions) (*SuiteResult, error) { fmt.Fprintf(w, "JUnit XML written to: %s\n", opts.JUnitOutput) } + // A failed cleanup leaves the project modified, so the run must not be + // reported as clean even when every test passed. + if cleanupErr != nil { + return result, fmt.Errorf("cleanup failed, project left modified: %w", cleanupErr) + } return result, nil } @@ -254,6 +269,35 @@ func parseTestFiles(paths []string) (*TestSuite, error) { return combined, nil } +// projectState records what Run changed in the project, captured before the first +// mutation so cleanup can put things back exactly rather than guessing. +type projectState struct { + // afterStartup is the project's original after-startup microflow ("" = none). + afterStartup string + // createdMxTest reports whether Run created the MxTest module, i.e. it did not + // already exist. A pre-existing MxTest module belongs to the user and must + // survive cleanup with everything but the generated TestRunner intact. + createdMxTest bool +} + +// captureProjectState reads everything cleanup needs to restore, before anything +// is injected. +func captureProjectState(projectPath string) (projectState, error) { + var st projectState + af, err := getAfterStartup(projectPath) + if err != nil { + return st, fmt.Errorf("reading after-startup setting: %w", err) + } + st.afterStartup = af + + exists, err := moduleExists(projectPath, mxTestModule) + if err != nil { + return st, fmt.Errorf("listing modules: %w", err) + } + st.createdMxTest = !exists + return st, nil +} + // getAfterStartup reads the current after-startup microflow setting. func getAfterStartup(projectPath string) (string, error) { mxcliPath, err := findMxcli() @@ -268,46 +312,146 @@ func getAfterStartup(projectPath string) (string, error) { return "", err } - // Parse output for AfterStartupMicroflow for _, line := range strings.Split(string(output), "\n") { - line = strings.TrimSpace(line) if strings.Contains(line, "AfterStartupMicroflow") { - // Extract the value from: AfterStartupMicroflow = 'Module.Name' - parts := strings.SplitN(line, "=", 2) - if len(parts) == 2 { - val := strings.TrimSpace(parts[1]) - val = strings.Trim(val, "'\"") - val = strings.TrimSuffix(val, ";") - val = strings.TrimSpace(val) - return val, nil - } + return parseSettingValue(line), nil } } - return "", nil } -// cleanup restores original project settings after testing. -func cleanup(projectPath, origAfterStartup string, w io.Writer) { - // Restore original after-startup - if origAfterStartup != "" { - cmd := fmt.Sprintf("ALTER SETTINGS MODEL AfterStartupMicroflow = '%s'", origAfterStartup) - if err := execMxcliCmd(projectPath, cmd); err != nil { - fmt.Fprintf(w, " Warning: could not restore after-startup: %v\n", err) +// parseSettingValue extracts the value from one DESCRIBE SETTINGS line, e.g. +// +// AfterStartupMicroflow = 'Module.Name', +// +// DESCRIBE SETTINGS separates properties with commas and ends the statement with +// a semicolon, so the trailing punctuation must come off *before* the quotes: +// trimming quotes first stops at the comma and leaves `Module.Name',`, which was +// then re-interpolated into unparseable MDL and silently failed to restore the +// setting (mendixlabs/mxcli#803). +func parseSettingValue(line string) string { + val := strings.TrimSpace(line) + if _, after, found := strings.Cut(val, "="); found { + val = after + } + val = strings.TrimSpace(val) + val = strings.TrimRight(val, ",;") + val = strings.TrimSpace(val) + return strings.Trim(val, "'\"") +} + +// quoteMDLString renders a value as an MDL single-quoted literal. Mendix escapes +// an embedded quote by doubling it; a qualified name should never contain one, but +// emitting a broken literal is how #803 turned a parse slip into a mutated project. +func quoteMDLString(v string) string { + return "'" + strings.ReplaceAll(v, "'", "''") + "'" +} + +// moduleExists reports whether the project has a module with the given name. +func moduleExists(projectPath, name string) (bool, error) { + mxcliPath, err := findMxcli() + if err != nil { + return false, err + } + cmd := exec.Command(mxcliPath, "-p", projectPath, "-c", "SHOW MODULES", "--json") + cmd.Env = append(os.Environ(), "MXCLI_QUIET=1") + output, err := cmd.Output() + if err != nil { + return false, err + } + var modules []struct { + Module string `json:"Module"` + } + if err := json.Unmarshal(output, &modules); err != nil { + return false, fmt.Errorf("parsing module list: %w", err) + } + for _, m := range modules { + if strings.EqualFold(m.Module, name) { + return true, nil } + } + return false, nil +} + +// setupCommands returns the MDL statements Run issues to put the project into its +// testing state. The project's Security Level is deliberately absent: the +// after-startup microflow runs in an administrative context and is not subject to +// it, so forcing it OFF bought nothing — while breaking any project with a +// published REST/OData service using custom authentication ("App security is off, +// but custom authentication is enabled for this service"), and the restore +// hardcoded PRODUCTION, silently changing projects that run at another level +// (mendixlabs/mxcli#802). +func setupCommands() []string { + return []string{ + "ALTER SETTINGS MODEL AfterStartupMicroflow = " + quoteMDLString(mxTestRunner), + } +} + +// cleanupCommands returns the MDL statements that put the project back the way it +// was, in order. Kept separate from execution so the restore can be tested without +// a project. mxTestPresent says whether the generated module is still there — +// nothing is dropped when it is already gone, so a run that failed before the +// injection landed does not report a spurious cleanup failure. +func cleanupCommands(st projectState, mxTestPresent bool) []string { + // Restore the original after-startup microflow, or clear it if there was none. + restore := "ALTER SETTINGS MODEL AfterStartupMicroflow = ''" + if st.afterStartup != "" { + restore = "ALTER SETTINGS MODEL AfterStartupMicroflow = " + quoteMDLString(st.afterStartup) + } + cmds := []string{restore} + if !mxTestPresent { + return cmds + } + + // Remove the generated runner. Drop the whole module only when Run created it; + // a pre-existing MxTest module is the user's, so only the generated microflow + // comes out of it. + if st.createdMxTest { + cmds = append(cmds, "DROP MODULE "+mxTestModule) } else { - if err := execMxcliCmd(projectPath, "ALTER SETTINGS MODEL AfterStartupMicroflow = ''"); err != nil { - fmt.Fprintf(w, " Warning: could not clear after-startup: %v\n", err) - } + cmds = append(cmds, "DROP MICROFLOW "+mxTestRunner) } + return cmds +} - // Restore security level - if err := execMxcliCmd(projectPath, "ALTER PROJECT SECURITY LEVEL PRODUCTION"); err != nil { - fmt.Fprintf(w, " Warning: could not restore security: %v\n", err) +// cleanup restores the project to the state captured before injection and removes +// the generated test runner. +// +// Every statement is attempted even if an earlier one fails, and the failures are +// returned rather than printed as warnings: a half-restored project is left with +// its after-startup pointing at a microflow this function is about to delete, and +// that has to be loud (#803). +func cleanup(projectPath string, st projectState, w io.Writer) error { + // Re-check rather than assume: on failure fall back to attempting the drop, so + // a genuine problem still surfaces instead of being skipped. + mxTestPresent := true + if exists, err := moduleExists(projectPath, mxTestModule); err == nil { + mxTestPresent = exists + } + if mxTestPresent && !st.createdMxTest { + fmt.Fprintf(w, " %s module already existed; dropping only %s\n", mxTestModule, mxTestRunner) + } + + var errs []error + for _, cmd := range cleanupCommands(st, mxTestPresent) { + if err := execMxcliCmd(projectPath, cmd); err != nil { + errs = append(errs, fmt.Errorf("%s: %w", cmd, err)) + } } + if len(errs) > 0 { + return errors.Join(errs...) + } + return nil +} - // Drop the test runner microflow - execMxcliCmd(projectPath, "DROP MICROFLOW MxTest.TestRunner") +// reportCleanup prints a cleanup failure prominently. The project is left mutated, +// so this must not read as a passing run. +func reportCleanup(w io.Writer, err error) { + if err == nil { + return + } + fmt.Fprintf(w, "\nERROR: cleanup failed — the project has been left modified:\n%v\n", err) + fmt.Fprintf(w, "Check the after-startup microflow and the %s module before committing.\n", mxTestModule) } // captureRuntimeLogs tails the docker compose logs, waiting for MXTEST:END or timeout. diff --git a/cmd/mxcli/testrunner/runner_cleanup_test.go b/cmd/mxcli/testrunner/runner_cleanup_test.go new file mode 100644 index 000000000..4809c0c04 --- /dev/null +++ b/cmd/mxcli/testrunner/runner_cleanup_test.go @@ -0,0 +1,195 @@ +// SPDX-License-Identifier: Apache-2.0 + +// Regression tests for the three `mxcli test` cleanup defects: +// +// - #803 the after-startup value was mis-parsed, so restoring it produced +// unparseable MDL, the failure was printed as a warning, and the project was +// left with its after-startup pointing at the microflow cleanup then deleted +// - #804 cleanup dropped only the microflow, leaving an empty MxTest module +// - #802 the Security Level was forced OFF and restored to a hardcoded +// PRODUCTION, regardless of what the project actually used +package testrunner + +import ( + "strings" + "testing" +) + +// TestParseSettingValue covers the lines DESCRIBE SETTINGS actually emits. +// Properties are comma-separated and the statement ends with a semicolon, so the +// trailing punctuation has to come off before the quotes — trimming quotes first +// stops at the comma and leaves the punctuation inside the value (#803). +func TestParseSettingValue(t *testing.T) { + tests := []struct { + name string + line string + want string + }{ + { + name: "trailing comma (the reported case)", + line: " AfterStartupMicroflow = 'MyFirstModule.ASU_Startup',", + want: "MyFirstModule.ASU_Startup", + }, + { + name: "trailing semicolon (last property in the statement)", + line: " AfterStartupMicroflow = 'MyFirstModule.ASU_Startup';", + want: "MyFirstModule.ASU_Startup", + }, + { + name: "no trailing punctuation", + line: " AfterStartupMicroflow = 'MyFirstModule.ASU_Startup'", + want: "MyFirstModule.ASU_Startup", + }, + { + name: "double quotes", + line: ` AfterStartupMicroflow = "MyFirstModule.ASU_Startup",`, + want: "MyFirstModule.ASU_Startup", + }, + { + name: "empty value", + line: " AfterStartupMicroflow = '',", + want: "", + }, + { + name: "value containing an equals sign is not truncated", + line: " SomeSetting = 'a=b',", + want: "a=b", + }, + { + name: "no equals sign at all", + line: " AfterStartupMicroflow", + want: "AfterStartupMicroflow", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := parseSettingValue(tc.line); got != tc.want { + t.Errorf("parseSettingValue(%q) = %q, want %q", tc.line, got, tc.want) + } + }) + } +} + +// TestParseSettingValue_RoundTripsThroughQuoting is the property that actually +// broke: whatever is parsed out must go back in as a well-formed MDL literal. +func TestParseSettingValue_RoundTripsThroughQuoting(t *testing.T) { + for _, line := range []string{ + " AfterStartupMicroflow = 'MyFirstModule.ASU_Startup',", + " AfterStartupMicroflow = 'MyFirstModule.ASU_Startup';", + " AfterStartupMicroflow = 'Mod.Flow'", + } { + got := quoteMDLString(parseSettingValue(line)) + if strings.Count(got, "'") != 2 { + t.Errorf("re-quoting %q produced %q — not a single well-formed literal", line, got) + } + if strings.HasSuffix(got, ",'") || strings.HasSuffix(got, ";'") { + t.Errorf("punctuation leaked into the value: %q", got) + } + } +} + +func TestQuoteMDLString(t *testing.T) { + tests := []struct { + in string + want string + }{ + {"Mod.Flow", "'Mod.Flow'"}, + {"", "''"}, + // Mendix escapes an embedded quote by doubling it, never with a backslash. + {"it's", "'it''s'"}, + {"a'b'c", "'a''b''c'"}, + } + for _, tc := range tests { + if got := quoteMDLString(tc.in); got != tc.want { + t.Errorf("quoteMDLString(%q) = %q, want %q", tc.in, got, tc.want) + } + } +} + +// TestNoSecurityLevelManipulation pins #802: the Security Level is the project's +// business. Neither setup nor cleanup may touch it. +func TestNoSecurityLevelManipulation(t *testing.T) { + all := append(setupCommands(), cleanupCommands(projectState{}, true)...) + all = append(all, cleanupCommands(projectState{afterStartup: "Mod.Flow", createdMxTest: true}, true)...) + for _, cmd := range all { + if strings.Contains(strings.ToUpper(cmd), "SECURITY LEVEL") { + t.Errorf("the runner still alters the project Security Level: %q (#802)", cmd) + } + } +} + +func TestCleanupCommands(t *testing.T) { + tests := []struct { + name string + state projectState + present bool + want []string + }{ + { + name: "restores an existing after-startup and drops the module it created", + state: projectState{afterStartup: "MyFirstModule.ASU_Startup", createdMxTest: true}, + present: true, + want: []string{ + "ALTER SETTINGS MODEL AfterStartupMicroflow = 'MyFirstModule.ASU_Startup'", + "DROP MODULE MxTest", + }, + }, + { + name: "clears after-startup when the project had none", + state: projectState{createdMxTest: true}, + present: true, + want: []string{ + "ALTER SETTINGS MODEL AfterStartupMicroflow = ''", + "DROP MODULE MxTest", + }, + }, + { + // A pre-existing MxTest module belongs to the user: only the generated + // microflow may be removed, or the run destroys their work. + name: "keeps a pre-existing MxTest module", + state: projectState{afterStartup: "Mod.Flow"}, + present: true, + want: []string{ + "ALTER SETTINGS MODEL AfterStartupMicroflow = 'Mod.Flow'", + "DROP MICROFLOW MxTest.TestRunner", + }, + }, + { + // The injection never landed: restore the setting, drop nothing, and do + // not report a cleanup failure for a module that was never created. + name: "nothing to drop when the module is absent", + state: projectState{afterStartup: "Mod.Flow", createdMxTest: true}, + present: false, + want: []string{ + "ALTER SETTINGS MODEL AfterStartupMicroflow = 'Mod.Flow'", + }, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := cleanupCommands(tc.state, tc.present) + if len(got) != len(tc.want) { + t.Fatalf("cleanupCommands = %q, want %q", got, tc.want) + } + for i := range got { + if got[i] != tc.want[i] { + t.Errorf("command %d = %q, want %q", i, got[i], tc.want[i]) + } + } + }) + } +} + +// TestCleanupCommands_RestoreIsWellFormed is the end of the #803 chain: whatever +// DESCRIBE SETTINGS produced must come back as a parseable statement. +func TestCleanupCommands_RestoreIsWellFormed(t *testing.T) { + parsed := parseSettingValue(" AfterStartupMicroflow = 'MyFirstModule.ASU_Startup',") + restore := cleanupCommands(projectState{afterStartup: parsed}, true)[0] + want := "ALTER SETTINGS MODEL AfterStartupMicroflow = 'MyFirstModule.ASU_Startup'" + if restore != want { + t.Errorf("restore command = %q, want %q", restore, want) + } + if strings.Count(restore, "'") != 2 { + t.Errorf("restore command is not a single well-formed literal: %q", restore) + } +} diff --git a/mdl-examples/bug-tests/782-external-entity-create-change-locally.mdl b/mdl-examples/bug-tests/782-external-entity-create-change-locally.mdl new file mode 100644 index 000000000..1a89e43b0 --- /dev/null +++ b/mdl-examples/bug-tests/782-external-entity-create-change-locally.mdl @@ -0,0 +1,59 @@ +-- Bug #782: External Entities — allow_create_change_locally doesn't work +-- +-- Symptom: after importing entities from a consumed OData service, enabling +-- "Allow creating and changing objects locally" left the flag at false. +-- +-- Two independent causes, both fixed: +-- +-- (1) READ. entityFromGen (mdl/backend/modelsdk/domainmodel.go) recognised only +-- DomainModels$OqlViewEntitySource, so an OData external entity read back +-- with an empty Source and every remote field zeroed. `describe external +-- entity` rejected it outright ("is not an external entity (source: )"), and +-- `alter entity … set allow_create_change_locally = true` wrote the flag onto +-- a model that no longer knew the entity was external, so it was lost. The +-- legacy engine parsed all three OData source flavours, so this was a +-- modelsdk-engine gap — and modelsdk is the default engine. +-- +-- (2) RE-IMPORT. applyExternalEntityFields (mdl/executor/cmd_contract.go) stamped +-- CreateChangeLocally = false on every import, including updates of existing +-- entities, so `create or modify external entities` reset whatever the user +-- had set. Unlike Countable/Creatable/Deletable it cannot be derived from the +-- service contract — it is a local modelling choice — so it is now left alone +-- on the top-level branch. +-- +-- Manual verification (needs a project; the metadata file ships with the repo): +-- +-- cp mdl-examples/odata-local-metadata/sample-metadata.xml /path/to/app/ +-- mxcli exec 782-external-entity-create-change-locally.mdl -p app.mpr +-- +-- Expect the final DESCRIBE to report `AllowCreateChangeLocally: Yes`. Before the +-- fix the first DESCRIBE errored with "is not an external entity", and after the +-- re-import the flag was back to No. +-- +-- Adjust MetadataUrl below to wherever sample-metadata.xml sits. + +create module Issue782; +create module role Issue782.User; + +create constant Issue782.SvcUrl + type string + default 'https://services.odata.org/V4/Northwind/Northwind.svc/'; + +create odata client Issue782.Sample ( + MetadataUrl: './mdl-examples/odata-local-metadata/sample-metadata.xml', + ServiceUrl: '@Issue782.SvcUrl' +); + +create external entities from Issue782.Sample into Issue782; + +-- (1) The entity must read back as external at all. +describe external entity Issue782.Products; + +-- The reported flow: turn the flag on, and it must stick. +alter entity Issue782.Products set allow_create_change_locally = true; +describe external entity Issue782.Products; + +-- (2) A re-import refreshes the contract-derived capabilities but must not reset +-- the local-changes flag. +create or modify external entities from Issue782.Sample into Issue782; +describe external entity Issue782.Products; diff --git a/mdl-examples/bug-tests/791-loop-continue-dangling-flow.mdl b/mdl-examples/bug-tests/791-loop-continue-dangling-flow.mdl new file mode 100644 index 000000000..dde78e3ee --- /dev/null +++ b/mdl-examples/bug-tests/791-loop-continue-dangling-flow.mdl @@ -0,0 +1,60 @@ +-- Bug #791: a loop containing a split whose branch does `continue` produced a +-- project Studio Pro could not open, while mxcli reported success: +-- +-- System.Collections.Generic.KeyNotFoundException: The given key +-- '806fca46-5c4b-46f8-a890-4d24dd29c24f' was not present in the dictionary. +-- +-- Root cause: microflowObjectToGen (mdl/backend/modelsdk/microflow_write.go) had +-- no case for Microflows$BreakEvent / Microflows$ContinueEvent, and its default +-- branch returns nil. The event object was therefore dropped at serialization +-- while the SequenceFlow pointing at it was written — leaving a +-- DestinationPointer to a GUID that exists nowhere in the document, which is the +-- key Studio Pro cannot resolve. The legacy engine (sdk/mpr/writer_microflow.go) +-- serializes both, so this only affected the default (modelsdk) engine. +-- +-- The identical bug had already been found and fixed for Microflows$ErrorEvent; +-- break/continue were missed. +-- +-- Verification without Studio Pro — every *Pointer in the written microflow must +-- resolve to an object $ID in the same document: +-- +-- mxcli exec 791-loop-continue-dangling-flow.mdl -p app.mpr +-- mxcli bson dump -p app.mpr --type microflow --object Issue791.LoopSplitContinue +-- +-- Before the fix the dump contained a SequenceFlow.DestinationPointer with no +-- matching $ID, and no Microflows$ContinueEvent object at all. + +create module Issue791; +create module role Issue791.User; + +@position(100, 100) +create persistent entity Issue791.Contract ( + Name: string(200), + Active: boolean +); + +-- continue in the ELSE branch of a split inside a loop — the reported shape. +create or modify microflow Issue791.LoopSplitContinue ($ContractList: list of Issue791.Contract) +begin + loop $it in $ContractList + begin + if $it/Active then + log info node 'Issue791' 'active'; + else + continue; + end if; + end loop; +end; + +-- break has the same serialization gap and the same consequence. +create or modify microflow Issue791.LoopSplitBreak ($ContractList: list of Issue791.Contract) +begin + loop $it in $ContractList + begin + if $it/Active then + break; + else + log info node 'Issue791' 'inactive'; + end if; + end loop; +end; diff --git a/mdl-examples/bug-tests/795-datagrid-microflow-datasource-describe.mdl b/mdl-examples/bug-tests/795-datagrid-microflow-datasource-describe.mdl new file mode 100644 index 000000000..e577d90f4 --- /dev/null +++ b/mdl-examples/bug-tests/795-datagrid-microflow-datasource-describe.mdl @@ -0,0 +1,73 @@ +-- Bug #795: DESCRIBE PAGE drops a datagrid's microflow DataSource +-- +-- Symptom: a DataGrid2 bound to a microflow described as `datagrid g1 {` with no +-- DataSource at all, so re-applying the describe output recreated the grid with no +-- data source. A database-bound grid described correctly, which is what made the +-- loss look selective. +-- +-- Root cause: a Forms$MicroflowSource stores the microflow name in the nested +-- Forms$MicroflowSettings ("MicroflowSettings" -> "Microflow") — what the write path +-- emits and what Studio Pro stores. extractDataGrid2DataSource looked up a top-level +-- "Microflow" key, got "", and returned no data source. The describe formatter's +-- `case "microflow"` branch was correct all along, so this was purely a read bug. +-- +-- Fix: every datasource reader now goes through microflowSourceRef / +-- nanoflowSourceRef (mdl/executor/cmd_pages_describe_pluggable.go), which read the +-- nested settings and fall back to the legacy top-level key. The readers had four +-- divergent copies of this lookup; two of them were wrong. +-- +-- Manual verification (needs a project, so `make check-mdl` only syntax-checks this): +-- +-- mxcli exec 795-datagrid-microflow-datasource-describe.mdl -p app.mpr +-- mxcli -p app.mpr -c "describe page Issue795.Probe" +-- +-- Expect `datagrid gMicroflow (DataSource: microflow Issue795.GetBuckets)` — before +-- the fix the DataSource was absent. Feeding that describe output back through +-- `mxcli exec` and describing again must produce identical output. + +create module Issue795; +create module role Issue795.User; + +@position(100, 100) +create persistent entity Issue795.Bucket ( + BucketKey: string(200), + Size: integer +); + +create microflow Issue795.GetBuckets () +returns list of Issue795.Bucket as $items +begin + retrieve $items from Issue795.Bucket; + return $items; +end; + +create or replace page Issue795.Probe ( + Title: 'Probe', + Layout: Atlas_Core.Atlas_Default +) { + -- The regression: a microflow-bound grid lost its DataSource on describe. + datagrid gMicroflow (datasource: microflow Issue795.GetBuckets) { + column colKey (attribute: BucketKey, caption: 'Key') + } + + -- Control case: a database-bound grid always described correctly. + datagrid gDatabase (datasource: database from Issue795.Bucket) { + column colKey2 (attribute: BucketKey, caption: 'Key') + } +} + +-- A gallery exercises the same reader from a different entry point; its legacy +-- flat-shaped source was unreadable too. +create or replace page Issue795.ProbeGallery ( + Title: 'Probe Gallery', + Layout: Atlas_Core.Atlas_Default +) { + gallery galMicroflow (datasource: microflow Issue795.GetBuckets) { + template template1 { + dynamictext txtKey (content: '{1}', contentparams: [{1} = BucketKey]) + } + } +} + +describe page Issue795.Probe; +describe page Issue795.ProbeGallery; diff --git a/mdl-examples/bug-tests/795b-flow-datasource-context-entity.mdl b/mdl-examples/bug-tests/795b-flow-datasource-context-entity.mdl new file mode 100644 index 000000000..b23618887 --- /dev/null +++ b/mdl-examples/bug-tests/795b-flow-datasource-context-entity.mdl @@ -0,0 +1,73 @@ +-- Two follow-ups to #795, both found while verifying that fix. +-- +-- (1) A nanoflow datasource on a pluggable widget was DROPPED AT WRITE TIME and +-- the statement still reported success. The codec engine cannot represent a +-- Forms$NanoflowSource yet and said so, but the error travelled through +-- widgetobj.ChildSerializer — which returns BSON with no error channel — so +-- it was logged and discarded while `exec` printed `Created page`. The grid +-- landed with no data source at all. +-- +-- Now the failure is recorded and drained by the page/snippet write entry +-- points, so the statement fails with the actionable message instead +-- (ADR-0004: refuse, don't drop). Authoring one still needs +-- `MXCLI_ENGINE=legacy`, which does write it correctly. +-- +-- (2) A data container bound to a microflow or nanoflow reported the FLOW as its +-- context entity: `-- Context: $currentObject (Issue795b.GetBuckets)` rather +-- than the entity the flow returns. The datasource reference was used +-- verbatim, which is right for a database source and wrong for a flow source. +-- The flow's return type is resolved now, falling back to the reference when +-- the flow cannot be resolved or returns a scalar. +-- +-- Manual verification (needs a project): +-- +-- mxcli exec 795b-flow-datasource-context-entity.mdl -p app.mpr +-- mxcli -p app.mpr -c "describe page Issue795b.Probe" +-- +-- Expect `-- Context: $currentObject (Issue795b.Bucket)` under the grid — before +-- the fix it named Issue795b.GetBuckets. For (1), adding a +-- `datasource: nanoflow …` grid must now fail the statement rather than print +-- `Created page`; see the commented-out block at the end. + +create module Issue795b; +create module role Issue795b.User; + +@position(100, 100) +create persistent entity Issue795b.Bucket ( + BucketKey: string(200), + Size: integer +); + +create microflow Issue795b.GetBuckets () +returns list of Issue795b.Bucket as $items +begin + retrieve $items from Issue795b.Bucket; + return $items; +end; + +create or replace page Issue795b.Probe ( + Title: 'Probe', + Layout: Atlas_Core.Atlas_Default +) { + -- The context comment under this grid must name Issue795b.Bucket, not the microflow. + datagrid gMicroflow (datasource: microflow Issue795b.GetBuckets) { + column colKey (attribute: BucketKey, caption: 'Key') + } +} + +describe page Issue795b.Probe; + +-- (1) is a negative case and cannot live in a script that must pass `mxcli exec`. +-- To reproduce by hand, add a nanoflow returning a list of Issue795b.Bucket and: +-- +-- create or replace page Issue795b.NanoProbe ( +-- Title: 'Nano', Layout: Atlas_Core.Atlas_Default +-- ) { +-- datagrid gNano (datasource: nanoflow Issue795b.GetBucketsClient) { +-- column colKey (attribute: BucketKey, caption: 'Key') +-- } +-- } +-- +-- Before the fix: `Created page`, grid with no data source. +-- After: the statement fails, naming *pages.NanoflowSource and the +-- MXCLI_ENGINE=legacy workaround. diff --git a/mdl-examples/bug-tests/ledger-52-break-in-conditional.fail.mdl b/mdl-examples/bug-tests/ledger-52-break-in-conditional.mdl similarity index 100% rename from mdl-examples/bug-tests/ledger-52-break-in-conditional.fail.mdl rename to mdl-examples/bug-tests/ledger-52-break-in-conditional.mdl diff --git a/mdl/backend/modelsdk/domainmodel.go b/mdl/backend/modelsdk/domainmodel.go index 1635f8449..a8d61fa80 100644 --- a/mdl/backend/modelsdk/domainmodel.go +++ b/mdl/backend/modelsdk/domainmodel.go @@ -7,6 +7,7 @@ import ( "github.com/mendixlabs/mxcli/modelsdk/element" genDm "github.com/mendixlabs/mxcli/modelsdk/gen/domainmodels" + genRest "github.com/mendixlabs/mxcli/modelsdk/gen/rest" genTexts "github.com/mendixlabs/mxcli/modelsdk/gen/texts" "github.com/mendixlabs/mxcli/modelsdk/mprread" @@ -160,14 +161,52 @@ func entityFromGen(e *genDm.Entity) *domainmodel.Entity { out.Location = parseLocation(e.Location()) - // View entities carry an OqlViewEntitySource referencing their source - // document by qualified name; surface it so read-modify-write paths (e.g. - // MOVE ENTITY, which must reparent the source doc) can see it. - if src, ok := e.Source().(*genDm.OqlViewEntitySource); ok { + // An entity's Source says where its data comes from. Surface each flavour so + // read-modify-write paths (MOVE ENTITY reparenting a source doc, ALTER ENTITY + // flipping a remote capability) can see it — without this an external entity + // reads back looking local, DESCRIBE EXTERNAL ENTITY rejects it, and an update + // rebuilds it with no source at all (mendixlabs/mxcli#782). Mirrors the legacy + // parser in sdk/mpr/parser_domainmodel.go. + switch src := e.Source().(type) { + case *genDm.OqlViewEntitySource: out.Source = "DomainModels$OqlViewEntitySource" out.SourceObjectID = model.ID(src.ID()) out.SourceDocumentRef = src.SourceDocumentQualifiedName() out.OqlQuery = src.Oql() + + case *genRest.ODataRemoteEntitySource: + // Top-level external entity: has its own entity set, so it carries the + // CRUD/paging capabilities and the local-changes flag. + out.Source = "Rest$ODataRemoteEntitySource" + out.SourceObjectID = model.ID(src.ID()) + out.RemoteServiceName = src.SourceDocumentQualifiedName() + out.RemoteEntitySet = src.EntitySet() + out.RemoteEntityName = src.RemoteName() + out.Countable = src.Countable() + out.Creatable = src.Creatable() + out.Deletable = src.Deletable() + out.SkipSupported = src.SkipSupported() + out.TopSupported = src.TopSupported() + out.CreateChangeLocally = src.CreateChangeLocally() + out.RemoteKeyParts = odataKeyFromGen(src.Key()) + // Updatable has no gen accessor because the storage type has no such + // field — updatability is per attribute (Rest$ODataMappedValue). The + // write path does not emit it either, so leaving it zero is symmetric. + + case *genRest.ODataEntityTypeSource: + // Derived / abstract / contained target type: no entity set, no capabilities. + out.Source = "Rest$ODataEntityTypeSource" + out.SourceObjectID = model.ID(src.ID()) + out.RemoteServiceName = src.SourceDocumentQualifiedName() + out.RemoteEntityName = src.EntityTypeName() + out.IsOpen = src.IsOpen() + out.RemoteKeyParts = odataKeyFromGen(src.Key()) + + case *genRest.ODataPrimitiveCollectionEntitySource: + // NPE generated for a Collection(Edm.*) property; carries only the service. + out.Source = "Rest$ODataPrimitiveCollectionEntitySource" + out.SourceObjectID = model.ID(src.ID()) + out.RemoteServiceName = src.SourceDocumentQualifiedName() } for _, el := range e.AttributesItems() { @@ -341,6 +380,32 @@ func indexFromGen(idx *genDm.Index) *domainmodel.Index { // attributeTypeFromGen is the reverse of attributeTypeToGen: a gen attribute-type // element back to a domainmodel.AttributeType (with Length / enumeration ref). +// odataKeyFromGen converts a Rest$ODataKey to the semantic remote-key parts. The +// inverse of odataKeyToGen in domainmodel_write.go. +func odataKeyFromGen(key element.Element) []*domainmodel.RemoteKeyPart { + k, ok := key.(*genRest.ODataKey) + if !ok || k == nil { + return nil + } + var parts []*domainmodel.RemoteKeyPart + for _, el := range k.PartsItems() { + p, ok := el.(*genRest.ODataKeyPart) + if !ok { + continue + } + kp := &domainmodel.RemoteKeyPart{ + Name: p.EntityKeyPartName(), + RemoteName: p.Name(), + RemoteType: p.RemoteType(), + } + if t := p.Type(); t != nil { + kp.Type = attributeTypeFromGen(t) + } + parts = append(parts, kp) + } + return parts +} + func attributeTypeFromGen(t element.Element) domainmodel.AttributeType { switch at := t.(type) { case *genDm.StringAttributeType: diff --git a/mdl/backend/modelsdk/external_entity_read_test.go b/mdl/backend/modelsdk/external_entity_read_test.go new file mode 100644 index 000000000..556773014 --- /dev/null +++ b/mdl/backend/modelsdk/external_entity_read_test.go @@ -0,0 +1,229 @@ +// SPDX-License-Identifier: Apache-2.0 + +// mendixlabs/mxcli#782: an external entity's `allow_create_change_locally` flag +// would not stick. The write path handled it; the *read* did not. entityFromGen +// recognised only DomainModels$OqlViewEntitySource, so an OData external entity +// came back with an empty Source and every remote field zeroed. Setting the flag +// then wrote it onto a model that no longer knew it was external, and the value +// was lost — as were describe, and any other read-modify-write on such an entity. +// +// The legacy engine (sdk/mpr/parser_domainmodel.go) parsed all three OData source +// flavours, so this was a modelsdk-engine gap, not a missing feature. +package modelsdkbackend + +import ( + "testing" + + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/domainmodel" +) + +// externalEntityFixture creates an OData external entity through the write path, +// then reads the domain model back with a fresh connection. Round-tripping through +// disk is the point: the read is what #782 broke. +func externalEntityFixture(t *testing.T, mutate func(*domainmodel.Entity)) (proj string, moduleID model.ID) { + t.Helper() + proj = copyFixture(t) + + b := New() + if err := b.Connect(proj); err != nil { + t.Fatalf("connect: %v", err) + } + mod, err := b.GetModuleByName("MyFirstModule") + if err != nil || mod == nil { + t.Fatalf("GetModuleByName: %v, %v", mod, err) + } + dm, err := b.GetDomainModel(mod.ID) + if err != nil || dm == nil { + t.Fatalf("GetDomainModel: %v, %v", dm, err) + } + + ent := &domainmodel.Entity{ + Name: "Products", + Persistable: true, + Source: "Rest$ODataRemoteEntitySource", + RemoteServiceName: "MyFirstModule.Sample", + RemoteEntitySet: "Products", + RemoteEntityName: "Product", + Countable: true, + Creatable: true, + Deletable: true, + SkipSupported: true, + TopSupported: true, + CreateChangeLocally: true, + RemoteKeyParts: []*domainmodel.RemoteKeyPart{{ + Name: "ProductID", + RemoteName: "ProductID", + RemoteType: "Edm.Int32", + Type: &domainmodel.IntegerAttributeType{}, + }}, + Attributes: []*domainmodel.Attribute{ + {Name: "ProductName", Type: &domainmodel.StringAttributeType{}}, + }, + } + if mutate != nil { + mutate(ent) + } + if err := b.CreateEntity(dm.ID, ent); err != nil { + t.Fatalf("CreateEntity: %v", err) + } + if err := b.Disconnect(); err != nil { + t.Fatalf("disconnect: %v", err) + } + return proj, mod.ID +} + +func readEntity(t *testing.T, proj string, moduleID model.ID, name string) *domainmodel.Entity { + t.Helper() + b := New() + if err := b.Connect(proj); err != nil { + t.Fatalf("reconnect: %v", err) + } + t.Cleanup(func() { _ = b.Disconnect() }) + dm, err := b.GetDomainModel(moduleID) + if err != nil || dm == nil { + t.Fatalf("GetDomainModel: %v, %v", dm, err) + } + for _, e := range dm.Entities { + if e.Name == name { + return e + } + } + t.Fatalf("entity %s not found after write", name) + return nil +} + +// TestExternalEntity_RemoteSourceRoundTrip is the regression test: every field of +// a Rest$ODataRemoteEntitySource must survive a write→read cycle. +func TestExternalEntity_RemoteSourceRoundTrip(t *testing.T) { + proj, modID := externalEntityFixture(t, nil) + got := readEntity(t, proj, modID, "Products") + + if got.Source != "Rest$ODataRemoteEntitySource" { + t.Fatalf("Source = %q, want Rest$ODataRemoteEntitySource — the entity does not read back as external", got.Source) + } + if !got.CreateChangeLocally { + t.Error("CreateChangeLocally = false, want true (#782)") + } + if got.RemoteServiceName != "MyFirstModule.Sample" { + t.Errorf("RemoteServiceName = %q", got.RemoteServiceName) + } + if got.RemoteEntitySet != "Products" { + t.Errorf("RemoteEntitySet = %q", got.RemoteEntitySet) + } + if got.RemoteEntityName != "Product" { + t.Errorf("RemoteEntityName = %q", got.RemoteEntityName) + } + if !got.Countable || !got.Creatable || !got.Deletable || !got.SkipSupported || !got.TopSupported { + t.Errorf("capability flags lost: countable=%v creatable=%v deletable=%v skip=%v top=%v", + got.Countable, got.Creatable, got.Deletable, got.SkipSupported, got.TopSupported) + } + if got.SourceObjectID == "" { + t.Error("SourceObjectID empty — a read-modify-write cannot preserve the source element") + } + if len(got.RemoteKeyParts) != 1 { + t.Fatalf("RemoteKeyParts = %+v, want 1 part", got.RemoteKeyParts) + } + kp := got.RemoteKeyParts[0] + if kp.Name != "ProductID" || kp.RemoteName != "ProductID" || kp.RemoteType != "Edm.Int32" { + t.Errorf("key part = %+v", kp) + } + if _, ok := kp.Type.(*domainmodel.IntegerAttributeType); !ok { + t.Errorf("key part type = %T, want *IntegerAttributeType", kp.Type) + } +} + +// TestExternalEntity_FlagSurvivesReadModifyWrite reproduces the reported flow: +// read the entity, flip the flag (what ALTER ENTITY … SET +// ALLOW_CREATE_CHANGE_LOCALLY does), write it back, read again. +func TestExternalEntity_FlagSurvivesReadModifyWrite(t *testing.T) { + proj, modID := externalEntityFixture(t, func(e *domainmodel.Entity) { + e.CreateChangeLocally = false // imported off, as CREATE EXTERNAL ENTITIES leaves it + }) + + b := New() + if err := b.Connect(proj); err != nil { + t.Fatalf("connect: %v", err) + } + dm, err := b.GetDomainModel(modID) + if err != nil { + t.Fatalf("GetDomainModel: %v", err) + } + var ent *domainmodel.Entity + for _, e := range dm.Entities { + if e.Name == "Products" { + ent = e + } + } + if ent == nil { + t.Fatal("Products not found") + } + ent.CreateChangeLocally = true + if err := b.UpdateEntity(dm.ID, ent); err != nil { + t.Fatalf("UpdateEntity: %v", err) + } + if err := b.Disconnect(); err != nil { + t.Fatalf("disconnect: %v", err) + } + + got := readEntity(t, proj, modID, "Products") + if !got.CreateChangeLocally { + t.Error("CreateChangeLocally = false after setting it to true (#782)") + } + // The rest of the source must not have been damaged by the update. + if got.Source != "Rest$ODataRemoteEntitySource" || got.RemoteEntitySet != "Products" { + t.Errorf("source damaged by the update: Source=%q EntitySet=%q", got.Source, got.RemoteEntitySet) + } + if len(got.RemoteKeyParts) != 1 { + t.Errorf("remote key lost by the update: %+v", got.RemoteKeyParts) + } +} + +// TestExternalEntity_EntityTypeSourceRoundTrip covers the second flavour: a +// derived/abstract/contained type, which has no entity set. +func TestExternalEntity_EntityTypeSourceRoundTrip(t *testing.T) { + proj, modID := externalEntityFixture(t, func(e *domainmodel.Entity) { + e.Name = "ProductDetail" + e.Source = "Rest$ODataEntityTypeSource" + e.Persistable = false + e.IsOpen = true + e.RemoteEntitySet = "" + }) + got := readEntity(t, proj, modID, "ProductDetail") + + if got.Source != "Rest$ODataEntityTypeSource" { + t.Fatalf("Source = %q, want Rest$ODataEntityTypeSource", got.Source) + } + if got.RemoteEntityName != "Product" { + t.Errorf("RemoteEntityName = %q, want Product", got.RemoteEntityName) + } + if !got.IsOpen { + t.Error("IsOpen = false, want true") + } + if got.RemoteServiceName != "MyFirstModule.Sample" { + t.Errorf("RemoteServiceName = %q", got.RemoteServiceName) + } + if len(got.RemoteKeyParts) != 1 { + t.Errorf("RemoteKeyParts = %+v, want 1 part", got.RemoteKeyParts) + } +} + +// TestExternalEntity_PrimitiveCollectionSourceRoundTrip covers the third flavour, +// the NPE generated for a Collection(Edm.*) property. +func TestExternalEntity_PrimitiveCollectionSourceRoundTrip(t *testing.T) { + proj, modID := externalEntityFixture(t, func(e *domainmodel.Entity) { + e.Name = "ProductTag" + e.Source = "Rest$ODataPrimitiveCollectionEntitySource" + e.Persistable = false + e.RemoteEntitySet = "" + e.RemoteKeyParts = nil + }) + got := readEntity(t, proj, modID, "ProductTag") + + if got.Source != "Rest$ODataPrimitiveCollectionEntitySource" { + t.Fatalf("Source = %q, want Rest$ODataPrimitiveCollectionEntitySource", got.Source) + } + if got.RemoteServiceName != "MyFirstModule.Sample" { + t.Errorf("RemoteServiceName = %q", got.RemoteServiceName) + } +} diff --git a/mdl/backend/modelsdk/microflow_loopevent_write_test.go b/mdl/backend/modelsdk/microflow_loopevent_write_test.go new file mode 100644 index 000000000..ea455b3cf --- /dev/null +++ b/mdl/backend/modelsdk/microflow_loopevent_write_test.go @@ -0,0 +1,62 @@ +// SPDX-License-Identifier: Apache-2.0 + +// mendixlabs/mxcli#791: a microflow with a loop containing a split whose branch +// does `continue` wrote successfully but could not be opened in Studio Pro — +// "System.Collections.Generic.KeyNotFoundException: The given key '' was not +// present in the dictionary". +// +// microflowObjectToGen had no case for BreakEvent/ContinueEvent and its default +// branch returns nil, so the event object was dropped at serialization while the +// sequence flow pointing at it was written. The result is a dangling +// DestinationPointer: exactly the GUID Studio Pro cannot resolve. The legacy engine +// (sdk/mpr/writer_microflow.go) serializes both, so this was a modelsdk-engine gap. +package modelsdkbackend + +import ( + "testing" + + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +func TestMicroflowObjectToGen_LoopEvents(t *testing.T) { + tests := []struct { + name string + obj microflows.MicroflowObject + wantType string + }{ + { + name: "break", + obj: µflows.BreakEvent{BaseMicroflowObject: microflows.BaseMicroflowObject{ + BaseElement: model.BaseElement{ID: "ev-break"}, + Position: model.Point{X: 100, Y: 200}, + Size: model.Size{Width: 20, Height: 20}, + }}, + wantType: "Microflows$BreakEvent", + }, + { + name: "continue", + obj: µflows.ContinueEvent{BaseMicroflowObject: microflows.BaseMicroflowObject{ + BaseElement: model.BaseElement{ID: "ev-continue"}, + Position: model.Point{X: 300, Y: 400}, + Size: model.Size{Width: 20, Height: 20}, + }}, + wantType: "Microflows$ContinueEvent", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + g := microflowObjectToGen(tc.obj) + if g == nil { + t.Fatalf("%s dropped at serialization — any flow pointing at it is left dangling (#791)", tc.wantType) + } + if g.TypeName() != tc.wantType { + t.Errorf("TypeName = %q, want %q", g.TypeName(), tc.wantType) + } + if string(g.ID()) != string(tc.obj.GetID()) { + t.Errorf("ID = %q, want %q — a changed ID leaves the flow pointing at the old one", + g.ID(), tc.obj.GetID()) + } + }) + } +} diff --git a/mdl/backend/modelsdk/microflow_write.go b/mdl/backend/modelsdk/microflow_write.go index b80507ef5..3e06e0069 100644 --- a/mdl/backend/modelsdk/microflow_write.go +++ b/mdl/backend/modelsdk/microflow_write.go @@ -265,6 +265,22 @@ func microflowObjectToGen(obj microflows.MicroflowObject) element.Element { g.SetRelativeMiddlePoint(pointStr(o.Position)) g.SetSize(sizeStr(o.Size)) return g + case *microflows.BreakEvent: + // `break;` in a loop. Same hazard as ErrorEvent above: dropping it left the + // branch's SequenceFlow pointing at a non-existent object, so Studio Pro + // failed to open the project with KeyNotFoundException (#791). + g := genMf.NewBreakEvent() + g.SetID(element.ID(o.ID)) + g.SetRelativeMiddlePoint(pointStr(o.Position)) + g.SetSize(sizeStr(o.Size)) + return g + case *microflows.ContinueEvent: + // `continue;` in a loop — see BreakEvent above. + g := genMf.NewContinueEvent() + g.SetID(element.ID(o.ID)) + g.SetRelativeMiddlePoint(pointStr(o.Position)) + g.SetSize(sizeStr(o.Size)) + return g case *microflows.ActionActivity: g := genMf.NewActionActivity() g.SetID(element.ID(o.ID)) diff --git a/mdl/backend/modelsdk/page_write.go b/mdl/backend/modelsdk/page_write.go index 412d5649e..c2e47331f 100644 --- a/mdl/backend/modelsdk/page_write.go +++ b/mdl/backend/modelsdk/page_write.go @@ -27,6 +27,12 @@ func init() { // CreatePage inserts a new Forms$Page document unit (header, layout call, the // widget tree, parameters, and variables) via pageToGen. func (b *Backend) CreatePage(page *pages.Page) error { + // A pluggable widget's children are serialized while the executor builds the + // page, before this call; drain any failure so an unsupported construct fails + // the statement instead of silently landing as a widget with the piece missing. + if err := takeChildSerializeErr(); err != nil { + return fmt.Errorf("CreatePage: %w", err) + } if page == nil { return fmt.Errorf("CreatePage: nil page") } @@ -56,6 +62,12 @@ func (b *Backend) CreatePage(page *pages.Page) error { // full page (header + widget tree) and replace the existing unit. Serialization // is identical to CreatePage. func (b *Backend) UpdatePage(page *pages.Page) error { + // A pluggable widget's children are serialized while the executor builds the + // page, before this call; drain any failure so an unsupported construct fails + // the statement instead of silently landing as a widget with the piece missing. + if err := takeChildSerializeErr(); err != nil { + return fmt.Errorf("UpdatePage: %w", err) + } if page == nil { return fmt.Errorf("UpdatePage: nil page") } diff --git a/mdl/backend/modelsdk/snippet_write.go b/mdl/backend/modelsdk/snippet_write.go index bb5fb6c6b..b27be6d7a 100644 --- a/mdl/backend/modelsdk/snippet_write.go +++ b/mdl/backend/modelsdk/snippet_write.go @@ -7,9 +7,9 @@ import ( "github.com/mendixlabs/mxcli/model" "github.com/mendixlabs/mxcli/modelsdk/codec" + "github.com/mendixlabs/mxcli/modelsdk/element" genDT "github.com/mendixlabs/mxcli/modelsdk/gen/datatypes" genPg "github.com/mendixlabs/mxcli/modelsdk/gen/pages" - "github.com/mendixlabs/mxcli/modelsdk/element" mmpr "github.com/mendixlabs/mxcli/modelsdk/mpr" "github.com/mendixlabs/mxcli/sdk/pages" ) @@ -24,6 +24,12 @@ func init() { // CreateSnippet inserts a new Forms$Snippet document — a reusable widget tree with // its own parameters (entity-typed) and a flat Widgets list (no layout call). func (b *Backend) CreateSnippet(snippet *pages.Snippet) error { + // A pluggable widget's children are serialized while the executor builds the + // page, before this call; drain any failure so an unsupported construct fails + // the statement instead of silently landing as a widget with the piece missing. + if err := takeChildSerializeErr(); err != nil { + return fmt.Errorf("CreateSnippet: %w", err) + } if snippet == nil { return fmt.Errorf("CreateSnippet: nil snippet") } @@ -50,6 +56,12 @@ func (b *Backend) CreateSnippet(snippet *pages.Snippet) error { // UpdateSnippet rewrites a snippet document (CREATE OR REPLACE). func (b *Backend) UpdateSnippet(snippet *pages.Snippet) error { + // A pluggable widget's children are serialized while the executor builds the + // page, before this call; drain any failure so an unsupported construct fails + // the statement instead of silently landing as a widget with the piece missing. + if err := takeChildSerializeErr(); err != nil { + return fmt.Errorf("UpdateSnippet: %w", err) + } if snippet == nil { return fmt.Errorf("UpdateSnippet: nil snippet") } diff --git a/mdl/backend/modelsdk/widget_child_error_test.go b/mdl/backend/modelsdk/widget_child_error_test.go new file mode 100644 index 000000000..3f01c9252 --- /dev/null +++ b/mdl/backend/modelsdk/widget_child_error_test.go @@ -0,0 +1,83 @@ +// SPDX-License-Identifier: Apache-2.0 + +// A pluggable widget's children are serialized through widgetobj.ChildSerializer, +// which returns BSON with no error channel. A construct the codec engine cannot +// represent — a nanoflow datasource on a DataGrid2, say — was therefore logged and +// dropped, and the write still reported success: `Created page`, with a grid that +// has no data source. ADR-0004 requires the backend to refuse rather than drop. +package modelsdkbackend + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/pages" +) + +func TestChildSerializeErr_RecordedAndDrained(t *testing.T) { + t.Cleanup(func() { _ = takeChildSerializeErr() }) + if err := takeChildSerializeErr(); err != nil { + t.Fatalf("accumulator not empty at start: %v", err) + } + + // A nanoflow datasource is not yet representable by the codec engine. + got := codecChildSerializer{}.SerializeCustomWidgetDataSource( + &pages.NanoflowSource{Nanoflow: "M.GetOrders"}) + if got != nil { + t.Errorf("unsupported datasource serialized to %v, want nil", got) + } + + err := takeChildSerializeErr() + if err == nil { + t.Fatal("failure was not recorded; it would be dropped silently") + } + if !strings.Contains(err.Error(), "NanoflowSource") { + t.Errorf("error does not name the construct: %v", err) + } + + // Draining clears, so the next write is not failed by a stale error. + if err := takeChildSerializeErr(); err != nil { + t.Errorf("accumulator not cleared after drain: %v", err) + } +} + +func TestChildSerializeErr_SupportedSourceRecordsNothing(t *testing.T) { + t.Cleanup(func() { _ = takeChildSerializeErr() }) + _ = takeChildSerializeErr() + + got := codecChildSerializer{}.SerializeCustomWidgetDataSource( + &pages.MicroflowSource{Microflow: "M.GetOrders"}) + if got == nil { + t.Fatal("supported microflow datasource serialized to nil") + } + if err := takeChildSerializeErr(); err != nil { + t.Errorf("supported datasource recorded an error: %v", err) + } +} + +// TestCreatePage_FailsOnDroppedChild pins the drain: a recorded failure must fail +// the statement rather than let the page land with the piece missing. +func TestCreatePage_FailsOnDroppedChild(t *testing.T) { + t.Cleanup(func() { _ = takeChildSerializeErr() }) + proj := copyFixture(t) + b := New() + if err := b.Connect(proj); err != nil { + t.Fatalf("connect: %v", err) + } + t.Cleanup(func() { _ = b.Disconnect() }) + + // Simulate the executor building a widget tree whose child could not be + // serialized, exactly as SerializeCustomWidgetDataSource does above. + codecChildSerializer{}.SerializeCustomWidgetDataSource(&pages.NanoflowSource{Nanoflow: "M.GetOrders"}) + + page := &pages.Page{Name: "DropProbe"} + page.ID = model.ID("") + err := b.CreatePage(page) + if err == nil { + t.Fatal("CreatePage succeeded after a child was dropped") + } + if !strings.Contains(err.Error(), "NanoflowSource") { + t.Errorf("error does not explain what was dropped: %v", err) + } +} diff --git a/mdl/backend/modelsdk/widget_pluggable_write.go b/mdl/backend/modelsdk/widget_pluggable_write.go index 671dc7917..5acb6bb25 100644 --- a/mdl/backend/modelsdk/widget_pluggable_write.go +++ b/mdl/backend/modelsdk/widget_pluggable_write.go @@ -3,8 +3,10 @@ package modelsdkbackend import ( + "errors" "fmt" "log" + "sync" bsonv1 "go.mongodb.org/mongo-driver/bson" bsonv2 "go.mongodb.org/mongo-driver/v2/bson" @@ -96,6 +98,43 @@ func (b *Backend) BuildFilterWidget(spec backend.FilterWidgetSpec, projectPath s }, nil } +// Child-serialization failures cannot be returned: widgetobj.ChildSerializer +// yields BSON only, and making it error-returning is the interface change the +// TODO in mdl/backend/widgetobj/builder.go defers. Until then a failure is +// recorded here and drained by the page/snippet write entry points, so an +// unsupported construct fails the statement instead of being logged and dropped +// while the write still reports success (ADR-0004: refuse, don't drop). +// +// Widget building is synchronous and engines run sequentially — the same +// reasoning that makes widgetobj's package-level serializer hook safe — but the +// mutex keeps this honest if that ever changes. +var ( + childSerializeMu sync.Mutex + childSerializeErrs []error +) + +// recordChildSerializeErr remembers a failure for the next drain. It is also +// logged, since the log line is the only signal for callers that do not drain. +func recordChildSerializeErr(err error) { + log.Printf("modelsdk: %v", err) + childSerializeMu.Lock() + defer childSerializeMu.Unlock() + childSerializeErrs = append(childSerializeErrs, err) +} + +// takeChildSerializeErr returns the accumulated child-serialization failures as +// one error and clears them. A nil return means the widget tree serialized whole. +func takeChildSerializeErr() error { + childSerializeMu.Lock() + defer childSerializeMu.Unlock() + if len(childSerializeErrs) == 0 { + return nil + } + err := errors.Join(childSerializeErrs...) + childSerializeErrs = nil + return err +} + // codecChildSerializer implements widgetobj.ChildSerializer by routing child // content through the modelsdk codec converters, then bridging v2→v1 BSON. type codecChildSerializer struct{} @@ -103,7 +142,7 @@ type codecChildSerializer struct{} func (codecChildSerializer) SerializeWidget(w pages.Widget) bsonv1.D { el, err := widgetToGen(w) if err != nil { - log.Printf("modelsdk: serialize child widget %T: %v", w, err) + recordChildSerializeErr(fmt.Errorf("serialize child widget %T: %w", w, err)) return nil } return genToV1BSON(el) @@ -112,7 +151,7 @@ func (codecChildSerializer) SerializeWidget(w pages.Widget) bsonv1.D { func (codecChildSerializer) SerializeClientAction(a pages.ClientAction) bsonv1.D { el, err := clientActionToGen(a) if err != nil { - log.Printf("modelsdk: serialize client action %T: %v", a, err) + recordChildSerializeErr(fmt.Errorf("serialize client action %T: %w", a, err)) return nil } return genToV1BSON(el) @@ -121,7 +160,7 @@ func (codecChildSerializer) SerializeClientAction(a pages.ClientAction) bsonv1.D func (codecChildSerializer) SerializeCustomWidgetDataSource(ds pages.DataSource) bsonv1.D { el, err := customWidgetDataSourceToGen(ds) if err != nil { - log.Printf("modelsdk: serialize custom widget data source %T: %v", ds, err) + recordChildSerializeErr(fmt.Errorf("serialize custom widget data source %T: %w", ds, err)) return nil } if el == nil { @@ -135,7 +174,7 @@ func (codecChildSerializer) SerializeCustomWidgetDataSource(ds pages.DataSource) func genToV1BSON(el element.Element) bsonv1.D { out, err := (&codec.Encoder{}).Encode(el) if err != nil { - log.Printf("modelsdk: encode child element: %v", err) + recordChildSerializeErr(fmt.Errorf("encode child element %T: %w", el, err)) return nil } var d bsonv1.D diff --git a/mdl/executor/cmd_contract.go b/mdl/executor/cmd_contract.go index 6628ff7ef..0d6357cd7 100644 --- a/mdl/executor/cmd_contract.go +++ b/mdl/executor/cmd_contract.go @@ -1171,7 +1171,11 @@ func applyExternalEntityFields( ent.Updatable = false ent.SkipSupported = true ent.TopSupported = true - ent.CreateChangeLocally = false + // CreateChangeLocally is deliberately NOT set. Unlike the capability flags + // above it cannot be derived from the service contract — it is a local + // modelling choice ("Allow creating and changing objects locally"), so + // stamping it here reset the user's setting on every re-import (#782). A + // newly-created entity arrives zero-valued, which is Mendix's default. return } @@ -1188,6 +1192,8 @@ func applyExternalEntityFields( ent.Updatable = false ent.SkipSupported = false ent.TopSupported = false + // An entity-type source has no CreateChangeLocally in storage (the writer does + // not emit one), so clear it when an entity is re-imported as a derived type. ent.CreateChangeLocally = false } diff --git a/mdl/executor/cmd_contract_test.go b/mdl/executor/cmd_contract_test.go index 64c4e4abd..696142eb9 100644 --- a/mdl/executor/cmd_contract_test.go +++ b/mdl/executor/cmd_contract_test.go @@ -159,3 +159,38 @@ func TestMendixAttrTypeToEdm(t *testing.T) { } } } + +// TestApplyExternalEntityFields_PreservesCreateChangeLocally is the re-import half +// of mendixlabs/mxcli#782: "Allow creating and changing objects locally" is a local +// modelling choice, not something the OData contract describes, so a re-import must +// not reset it. The capability flags, which the contract *does* describe, are still +// refreshed from the metadata. +func TestApplyExternalEntityFields_PreservesCreateChangeLocally(t *testing.T) { + et := &types.EdmEntityType{Name: "Product"} + es := &types.EdmEntitySet{Name: "Products"} + + // An entity the user had switched the flag on for, being re-imported. + existing := &domainmodel.Entity{CreateChangeLocally: true, Creatable: true} + applyExternalEntityFields(existing, et, true /*isTopLevel*/, "Svc.Sample", es, nil, nil) + if !existing.CreateChangeLocally { + t.Error("CreateChangeLocally reset by re-import, want it preserved (#782)") + } + if existing.Creatable { + t.Error("Creatable not refreshed from the contract (no InsertRestrictions ⇒ false)") + } + + // A newly-imported entity defaults to off, matching Mendix. + fresh := &domainmodel.Entity{} + applyExternalEntityFields(fresh, et, true, "Svc.Sample", es, nil, nil) + if fresh.CreateChangeLocally { + t.Error("a newly imported entity defaulted to CreateChangeLocally = true") + } + + // A derived/entity-type source has no such field in storage; clear it so a + // re-import that reclassifies an entity does not leave a stale value behind. + derived := &domainmodel.Entity{CreateChangeLocally: true} + applyExternalEntityFields(derived, et, false /*isTopLevel*/, "Svc.Sample", nil, nil, nil) + if derived.CreateChangeLocally { + t.Error("entity-type source kept CreateChangeLocally, want it cleared") + } +} diff --git a/mdl/executor/cmd_microflows_builder_control.go b/mdl/executor/cmd_microflows_builder_control.go index 5501089d2..3529ab522 100644 --- a/mdl/executor/cmd_microflows_builder_control.go +++ b/mdl/executor/cmd_microflows_builder_control.go @@ -568,7 +568,7 @@ func (fb *flowBuilder) addLoopStatement(s *ast.LoopStmt) model.ID { fb.pendingAnnotations = nil // First, measure the loop body to determine size - bodyBounds := fb.measurer.measureStatements(s.Body) + bodyBounds := fb.measurer.measureStatementsSpan(s.Body) // Calculate loop box size with padding // Extra width for iterator icon and its label (100 pixels) @@ -868,7 +868,7 @@ func (fb *flowBuilder) addWhileStatement(s *ast.WhileStmt) model.ID { savedWhileAnnotations := fb.pendingAnnotations fb.pendingAnnotations = nil - bodyBounds := fb.measurer.measureStatements(s.Body) + bodyBounds := fb.measurer.measureStatementsSpan(s.Body) loopWidth := max(bodyBounds.Width+2*LoopPadding, MinLoopWidth) loopHeight := max(bodyBounds.Height+2*LoopPadding, MinLoopHeight) diff --git a/mdl/executor/cmd_pages_describe_flowcontext.go b/mdl/executor/cmd_pages_describe_flowcontext.go new file mode 100644 index 000000000..73547933d --- /dev/null +++ b/mdl/executor/cmd_pages_describe_flowcontext.go @@ -0,0 +1,84 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// dataSourceEntityContext returns the entity a datasource puts in scope for the +// widgets inside it. +// +// For a database or association source the datasource reference *is* the entity. +// For a microflow or nanoflow source it is the flow's own qualified name, so using +// it directly named the flow as the context entity — `-- Context: $currentObject +// (Module.GetOrders)` instead of `(Module.Order)` — and any consumer that resolves +// attributes against the context was looking up the wrong element. The flow's +// return type is resolved instead, falling back to the reference when the flow +// cannot be found or does not return an object/list, which is what the caller +// used before. +func dataSourceEntityContext(ctx *ExecContext, ds *rawDataSource) string { + if ds == nil || ds.Reference == "" { + return "" + } + switch ds.Type { + case "microflow", "nanoflow": + if entity := flowReturnEntity(ctx, ds.Type, ds.Reference); entity != "" { + return entity + } + } + return ds.Reference +} + +// flowReturnEntity resolves the entity a microflow or nanoflow returns, by object +// or list return type. Returns "" when the project is unavailable, the flow is not +// found, or it returns something other than an object/list. +func flowReturnEntity(ctx *ExecContext, kind, qualifiedName string) string { + if ctx == nil || ctx.Backend == nil || qualifiedName == "" { + return "" + } + h, err := getHierarchy(ctx) + if err != nil || h == nil { + return "" + } + switch kind { + case "microflow": + mfs, err := ctx.Backend.ListMicroflows() + if err != nil { + return "" + } + for _, mf := range mfs { + if mf != nil && h.GetQualifiedName(mf.ContainerID, mf.Name) == qualifiedName { + return dataTypeEntity(mf.ReturnType) + } + } + case "nanoflow": + nfs, err := ctx.Backend.ListNanoflows() + if err != nil { + return "" + } + for _, nf := range nfs { + if nf != nil && h.GetQualifiedName(nf.ContainerID, nf.Name) == qualifiedName { + return dataTypeEntity(nf.ReturnType) + } + } + } + return "" +} + +// dataTypeEntity returns the entity a return type refers to, for the two kinds a +// data container can bind against. Both the pointer and value forms are matched +// because the parsers are not consistent about which they produce. +func dataTypeEntity(dt microflows.DataType) string { + switch t := dt.(type) { + case *microflows.ObjectType: + return t.EntityQualifiedName + case microflows.ObjectType: + return t.EntityQualifiedName + case *microflows.ListType: + return t.EntityQualifiedName + case microflows.ListType: + return t.EntityQualifiedName + } + return "" +} diff --git a/mdl/executor/cmd_pages_describe_flowcontext_test.go b/mdl/executor/cmd_pages_describe_flowcontext_test.go new file mode 100644 index 000000000..9fcd9fba9 --- /dev/null +++ b/mdl/executor/cmd_pages_describe_flowcontext_test.go @@ -0,0 +1,121 @@ +// SPDX-License-Identifier: Apache-2.0 + +// A data container bound to a microflow or nanoflow reported the *flow* as its +// context entity — `-- Context: $currentObject (Module.GetOrders)` instead of +// `(Module.Order)` — because the datasource reference was used verbatim. That is +// correct for a database source, where the reference is the entity, and wrong for +// a flow source, where it is the flow's own name. +package executor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/backend/mock" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +func TestDataTypeEntity(t *testing.T) { + tests := []struct { + name string + in microflows.DataType + want string + }{ + {"object pointer", µflows.ObjectType{EntityQualifiedName: "M.Order"}, "M.Order"}, + {"object value", microflows.ObjectType{EntityQualifiedName: "M.Order"}, "M.Order"}, + {"list pointer", µflows.ListType{EntityQualifiedName: "M.Order"}, "M.Order"}, + {"list value", microflows.ListType{EntityQualifiedName: "M.Order"}, "M.Order"}, + {"boolean", µflows.BooleanType{}, ""}, + {"nil", nil, ""}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := dataTypeEntity(tc.in); got != tc.want { + t.Errorf("dataTypeEntity = %q, want %q", got, tc.want) + } + }) + } +} + +// TestDataSourceEntityContext_NonFlowSources: a database or association source +// already names its entity, so it must pass straight through. +func TestDataSourceEntityContext_NonFlowSources(t *testing.T) { + for _, dsType := range []string{"database", "association", "parameter"} { + t.Run(dsType, func(t *testing.T) { + ds := &rawDataSource{Type: dsType, Reference: "M.Order"} + if got := dataSourceEntityContext(nil, ds); got != "M.Order" { + t.Errorf("dataSourceEntityContext = %q, want M.Order", got) + } + }) + } + if got := dataSourceEntityContext(nil, nil); got != "" { + t.Errorf("nil datasource = %q, want empty", got) + } + if got := dataSourceEntityContext(nil, &rawDataSource{Type: "database"}); got != "" { + t.Errorf("empty reference = %q, want empty", got) + } +} + +// TestDataSourceEntityContext_ResolvesFlowReturnEntity is the fix: the flow's +// return entity, not the flow's name. +func TestDataSourceEntityContext_ResolvesFlowReturnEntity(t *testing.T) { + mod := &model.Module{BaseElement: model.BaseElement{ID: nextID("mod")}, Name: "M"} + h := mkHierarchy(mod) + + mf := µflows.Microflow{ + ContainerID: mod.ID, + Name: "GetOrders", + ReturnType: µflows.ListType{EntityQualifiedName: "M.Order"}, + } + nf := µflows.Nanoflow{ + ContainerID: mod.ID, + Name: "GetOrderClient", + ReturnType: µflows.ObjectType{EntityQualifiedName: "M.Order"}, + } + // A flow that returns something a data container cannot bind against. + scalar := µflows.Microflow{ + ContainerID: mod.ID, + Name: "CountOrders", + ReturnType: µflows.IntegerType{}, + } + + mb := &mock.MockBackend{ + IsConnectedFunc: func() bool { return true }, + ListModulesFunc: func() ([]*model.Module, error) { return []*model.Module{mod}, nil }, + ListMicroflowsFunc: func() ([]*microflows.Microflow, error) { + return []*microflows.Microflow{mf, scalar}, nil + }, + ListNanoflowsFunc: func() ([]*microflows.Nanoflow, error) { + return []*microflows.Nanoflow{nf}, nil + }, + } + ctx, _ := newMockCtx(t, withBackend(mb), withHierarchy(h)) + + tests := []struct { + name string + ds *rawDataSource + want string + }{ + {"microflow list", &rawDataSource{Type: "microflow", Reference: "M.GetOrders"}, "M.Order"}, + {"nanoflow object", &rawDataSource{Type: "nanoflow", Reference: "M.GetOrderClient"}, "M.Order"}, + // Unresolvable cases keep the reference — no worse than before the fix. + {"scalar return", &rawDataSource{Type: "microflow", Reference: "M.CountOrders"}, "M.CountOrders"}, + {"unknown flow", &rawDataSource{Type: "microflow", Reference: "M.Missing"}, "M.Missing"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := dataSourceEntityContext(ctx, tc.ds); got != tc.want { + t.Errorf("dataSourceEntityContext = %q, want %q", got, tc.want) + } + }) + } +} + +// TestDataSourceEntityContext_NoProject: with no backend the reference is kept +// rather than the context being blanked. +func TestDataSourceEntityContext_NoProject(t *testing.T) { + ds := &rawDataSource{Type: "microflow", Reference: "M.GetOrders"} + if got := dataSourceEntityContext(nil, ds); got != "M.GetOrders" { + t.Errorf("dataSourceEntityContext = %q, want the reference kept", got) + } +} diff --git a/mdl/executor/cmd_pages_describe_flowsource_test.go b/mdl/executor/cmd_pages_describe_flowsource_test.go new file mode 100644 index 000000000..1d159e1e9 --- /dev/null +++ b/mdl/executor/cmd_pages_describe_flowsource_test.go @@ -0,0 +1,149 @@ +// SPDX-License-Identifier: Apache-2.0 + +// mendixlabs/mxcli#795: DESCRIBE PAGE drops a datagrid's microflow DataSource, so +// re-applying the describe output recreates the grid with no data source at all. +// +// Root cause: a Forms$MicroflowSource stores the microflow name in the nested +// Forms$MicroflowSettings ("MicroflowSettings" → "Microflow"), which is what the +// write path emits and what Studio Pro stores. The DataGrid2 and Gallery readers +// looked up a top-level "Microflow" key instead, got "", and returned no data +// source — while the describe formatter's `case "microflow"` branch was correct all +// along. A database source was unaffected, which is why only microflow sources +// silently lost their binding. +package executor + +import "testing" + +// flowSourceWidget builds a minimal pluggable-widget map whose datasource is the +// given raw source document, mirroring the on-disk BSON. +func flowSourceWidget(ds map[string]any) map[string]any { + return map[string]any{ + "Object": map[string]any{ + "Properties": []any{ + int32(2), // BSON non-empty-array version marker + map[string]any{ + "Value": map[string]any{"DataSource": ds}, + }, + }, + }, + } +} + +// nestedMicroflowSource is the shape Studio Pro and the codec engine write. +func nestedMicroflowSource(name string) map[string]any { + return map[string]any{ + "$Type": "Forms$MicroflowSource", + "ForceFullObjects": false, + "MicroflowSettings": map[string]any{ + "$Type": "Forms$MicroflowSettings", + "Microflow": name, + }, + } +} + +// flatMicroflowSource is the legacy shape: the name directly on the source. +func flatMicroflowSource(name string) map[string]any { + return map[string]any{ + "$Type": "Forms$MicroflowSource", + "Microflow": name, + } +} + +func nestedNanoflowSource(name string) map[string]any { + return map[string]any{ + "$Type": "Forms$NanoflowSource", + "NanoflowSettings": map[string]any{ + "$Type": "Forms$NanoflowSettings", + "Nanoflow": name, + }, + } +} + +// extractors is every reader that turns a raw datasource document into a +// rawDataSource. They historically disagreed about where the microflow name lives; +// the table keeps them honest with each other. +var extractors = []struct { + name string + fn func(*ExecContext, map[string]any) *rawDataSource +}{ + {"datagrid2", extractDataGrid2DataSource}, + {"gallery", extractGalleryDataSource}, +} + +func TestPluggableDataSource_MicroflowNestedSettings(t *testing.T) { + const mf = "MyModule.DBG_ListBucketObjects" + for _, ex := range extractors { + t.Run(ex.name, func(t *testing.T) { + ds := ex.fn(nil, flowSourceWidget(nestedMicroflowSource(mf))) + if ds == nil { + t.Fatal("microflow datasource dropped: got nil") + } + if ds.Type != "microflow" { + t.Errorf("Type = %q, want microflow", ds.Type) + } + if ds.Reference != mf { + t.Errorf("Reference = %q, want %q", ds.Reference, mf) + } + }) + } +} + +// TestPluggableDataSource_MicroflowFlatLegacy keeps the legacy top-level shape +// readable so files written before the nested form still round-trip. +func TestPluggableDataSource_MicroflowFlatLegacy(t *testing.T) { + const mf = "MyModule.Legacy" + for _, ex := range extractors { + t.Run(ex.name, func(t *testing.T) { + ds := ex.fn(nil, flowSourceWidget(flatMicroflowSource(mf))) + if ds == nil || ds.Type != "microflow" || ds.Reference != mf { + t.Fatalf("legacy flat microflow source not read: %+v", ds) + } + }) + } +} + +// TestPluggableDataSource_Nanoflow covers the sibling source type, which the +// DataGrid2 and Gallery readers did not handle at all. +func TestPluggableDataSource_Nanoflow(t *testing.T) { + const nf = "MyModule.NF_ListBuckets" + for _, ex := range extractors { + t.Run(ex.name, func(t *testing.T) { + ds := ex.fn(nil, flowSourceWidget(nestedNanoflowSource(nf))) + if ds == nil { + t.Fatal("nanoflow datasource dropped: got nil") + } + if ds.Type != "nanoflow" { + t.Errorf("Type = %q, want nanoflow", ds.Type) + } + if ds.Reference != nf { + t.Errorf("Reference = %q, want %q", ds.Reference, nf) + } + }) + } +} + +// TestFlowSourceRef_Helpers pins the shared lookup the readers delegate to. +func TestFlowSourceRef_Helpers(t *testing.T) { + if got := microflowSourceRef(nestedMicroflowSource("A.B")); got != "A.B" { + t.Errorf("microflowSourceRef(nested) = %q", got) + } + if got := microflowSourceRef(flatMicroflowSource("A.B")); got != "A.B" { + t.Errorf("microflowSourceRef(flat) = %q", got) + } + if got := microflowSourceRef(map[string]any{"$Type": "Forms$MicroflowSource"}); got != "" { + t.Errorf("microflowSourceRef(empty) = %q, want empty", got) + } + // A non-map MicroflowSettings must not panic. + if got := microflowSourceRef(map[string]any{"MicroflowSettings": "junk"}); got != "" { + t.Errorf("microflowSourceRef(junk) = %q, want empty", got) + } + if got := nanoflowSourceRef(nestedNanoflowSource("A.B")); got != "A.B" { + t.Errorf("nanoflowSourceRef(nested) = %q", got) + } + if got := nanoflowSourceRef(map[string]any{"Nanoflow": "A.B"}); got != "A.B" { + t.Errorf("nanoflowSourceRef(flat) = %q", got) + } + if got := nanoflowSourceRef(map[string]any{"NanoflowSettings": 42}); got != "" { + t.Errorf("nanoflowSourceRef(junk) = %q, want empty", got) + } +} diff --git a/mdl/executor/cmd_pages_describe_output.go b/mdl/executor/cmd_pages_describe_output.go index 02d8d3786..f31d728e7 100644 --- a/mdl/executor/cmd_pages_describe_output.go +++ b/mdl/executor/cmd_pages_describe_output.go @@ -465,6 +465,8 @@ func outputWidgetMDLV3(ctx *ExecContext, w rawWidget, indent int) { props = append(props, fmt.Sprintf("DataSource: %s", dsVal)) case "microflow": props = append(props, fmt.Sprintf("DataSource: microflow %s", w.DataSource.Reference)) + case "nanoflow": + props = append(props, fmt.Sprintf("DataSource: nanoflow %s", w.DataSource.Reference)) case "parameter": props = append(props, fmt.Sprintf("DataSource: %s", w.DataSource.Reference)) } @@ -528,6 +530,8 @@ func outputWidgetMDLV3(ctx *ExecContext, w rawWidget, indent int) { props = append(props, fmt.Sprintf("DataSource: %s", dsVal)) case "microflow": props = append(props, fmt.Sprintf("DataSource: microflow %s", w.DataSource.Reference)) + case "nanoflow": + props = append(props, fmt.Sprintf("DataSource: nanoflow %s", w.DataSource.Reference)) } } // Add column counts if non-default diff --git a/mdl/executor/cmd_pages_describe_parse.go b/mdl/executor/cmd_pages_describe_parse.go index 87b9f4c9f..7f499cb77 100644 --- a/mdl/executor/cmd_pages_describe_parse.go +++ b/mdl/executor/cmd_pages_describe_parse.go @@ -249,7 +249,7 @@ func parseRawWidget(ctx *ExecContext, w map[string]any, parentEntityContext ...s case "Forms$DataView", "Pages$DataView": widget.DataSource = extractDataViewDataSource(ctx, w) if widget.DataSource != nil && widget.DataSource.Reference != "" { - widget.EntityContext = widget.DataSource.Reference + widget.EntityContext = dataSourceEntityContext(ctx, widget.DataSource) } else if inheritedCtx != "" { widget.EntityContext = inheritedCtx } @@ -313,7 +313,7 @@ func parseRawWidget(ctx *ExecContext, w map[string]any, parentEntityContext ...s // showNumberOfRows: not yet fully supported in DataGrid2, skip to avoid CE0463 widget.Selection = extractGallerySelection(ctx, w) if widget.DataSource != nil && widget.DataSource.Reference != "" { - widget.EntityContext = widget.DataSource.Reference + widget.EntityContext = dataSourceEntityContext(ctx, widget.DataSource) } else if inheritedCtx != "" { widget.EntityContext = inheritedCtx } @@ -331,7 +331,7 @@ func parseRawWidget(ctx *ExecContext, w map[string]any, parentEntityContext ...s widget.TabletColumns = extractCustomWidgetPropertyString(ctx, w, "tabletItems") widget.PhoneColumns = extractCustomWidgetPropertyString(ctx, w, "phoneItems") if widget.DataSource != nil && widget.DataSource.Reference != "" { - widget.EntityContext = widget.DataSource.Reference + widget.EntityContext = dataSourceEntityContext(ctx, widget.DataSource) } else if inheritedCtx != "" { widget.EntityContext = inheritedCtx } @@ -370,7 +370,7 @@ func parseRawWidget(ctx *ExecContext, w map[string]any, parentEntityContext ...s case "Forms$Gallery", "Pages$Gallery": widget.DataSource = extractGalleryDataSource(ctx, w) if widget.DataSource != nil && widget.DataSource.Reference != "" { - widget.EntityContext = widget.DataSource.Reference + widget.EntityContext = dataSourceEntityContext(ctx, widget.DataSource) } else if inheritedCtx != "" { widget.EntityContext = inheritedCtx } @@ -384,7 +384,7 @@ func parseRawWidget(ctx *ExecContext, w map[string]any, parentEntityContext ...s case "Forms$ListView", "Pages$ListView": widget.DataSource = extractListViewDataSource(ctx, w) if widget.DataSource != nil && widget.DataSource.Reference != "" { - widget.EntityContext = widget.DataSource.Reference + widget.EntityContext = dataSourceEntityContext(ctx, widget.DataSource) } else if inheritedCtx != "" { widget.EntityContext = inheritedCtx } @@ -579,18 +579,12 @@ func extractDataViewDataSource(ctx *ExecContext, w map[string]any) *rawDataSourc switch dsType { case "Forms$MicroflowSource": - // Extract microflow name from MicroflowSettings - if settings, ok := ds["MicroflowSettings"].(map[string]any); ok { - if mfName, ok := settings["Microflow"].(string); ok && mfName != "" { - return &rawDataSource{Type: "microflow", Reference: mfName} - } + if mf := microflowSourceRef(ds); mf != "" { + return &rawDataSource{Type: "microflow", Reference: mf} } case "Forms$NanoflowSource": - // Extract nanoflow name from NanoflowSettings - if settings, ok := ds["NanoflowSettings"].(map[string]any); ok { - if nfName, ok := settings["Nanoflow"].(string); ok && nfName != "" { - return &rawDataSource{Type: "nanoflow", Reference: nfName} - } + if nf := nanoflowSourceRef(ds); nf != "" { + return &rawDataSource{Type: "nanoflow", Reference: nf} } case "Forms$DataViewSource": // "Data from context over an association" — the DataViewSource carries an @@ -809,18 +803,11 @@ func extractListViewDataSource(ctx *ExecContext, w map[string]any) *rawDataSourc return result } case "Forms$MicroflowSource": - microflow := extractString(ds["Microflow"]) - if mfSettings, ok := ds["MicroflowSettings"].(map[string]any); ok && microflow == "" { - microflow = extractString(mfSettings["Microflow"]) - } - if microflow != "" { - return &rawDataSource{Type: "microflow", Reference: microflow} + if mf := microflowSourceRef(ds); mf != "" { + return &rawDataSource{Type: "microflow", Reference: mf} } case "Forms$NanoflowSource": - nanoflow := extractString(ds["Nanoflow"]) - if nfSettings, ok := ds["NanoflowSettings"].(map[string]any); ok && nanoflow == "" { - nanoflow = extractString(nfSettings["Nanoflow"]) - } + nanoflow := nanoflowSourceRef(ds) if nanoflow != "" { return &rawDataSource{Type: "nanoflow", Reference: nanoflow} } diff --git a/mdl/executor/cmd_pages_describe_pluggable.go b/mdl/executor/cmd_pages_describe_pluggable.go index edc5d3486..6b714463f 100644 --- a/mdl/executor/cmd_pages_describe_pluggable.go +++ b/mdl/executor/cmd_pages_describe_pluggable.go @@ -220,9 +220,12 @@ func extractDataGrid2DataSource(ctx *ExecContext, w map[string]any) *rawDataSour return result } case "Forms$MicroflowSource": - microflow := extractString(ds["Microflow"]) - if microflow != "" { - return &rawDataSource{Type: "microflow", Reference: microflow} + if mf := microflowSourceRef(ds); mf != "" { + return &rawDataSource{Type: "microflow", Reference: mf} + } + case "Forms$NanoflowSource": + if nf := nanoflowSourceRef(ds); nf != "" { + return &rawDataSource{Type: "nanoflow", Reference: nf} } case "Forms$EntityPathSource", "Forms$DataViewSource": entityPath := extractString(ds["EntityPath"]) @@ -720,9 +723,12 @@ func extractGalleryDataSource(ctx *ExecContext, w map[string]any) *rawDataSource return result } case "Forms$MicroflowSource": - microflow := extractString(ds["Microflow"]) - if microflow != "" { - return &rawDataSource{Type: "microflow", Reference: microflow} + if mf := microflowSourceRef(ds); mf != "" { + return &rawDataSource{Type: "microflow", Reference: mf} + } + case "Forms$NanoflowSource": + if nf := nanoflowSourceRef(ds); nf != "" { + return &rawDataSource{Type: "nanoflow", Reference: nf} } case "Forms$EntityPathSource", "Forms$DataViewSource": entityPath := extractString(ds["EntityPath"]) @@ -733,6 +739,34 @@ func extractGalleryDataSource(ctx *ExecContext, w map[string]any) *rawDataSource return nil } +// microflowSourceRef returns the microflow a Forms$MicroflowSource points at. +// +// Studio Pro and the codec engine store the name in the nested Forms$MicroflowSettings; +// a top-level "Microflow" key is the legacy shape, still honoured so older files +// round-trip. Reading only the top-level key made DESCRIBE PAGE drop a datagrid's +// microflow datasource entirely (mendixlabs/mxcli#795), so every reader goes through +// this helper rather than keeping its own copy of the lookup. +func microflowSourceRef(ds map[string]any) string { + if mf := extractString(ds["Microflow"]); mf != "" { + return mf + } + if settings, ok := ds["MicroflowSettings"].(map[string]any); ok { + return extractString(settings["Microflow"]) + } + return "" +} + +// nanoflowSourceRef is the Forms$NanoflowSource counterpart of microflowSourceRef. +func nanoflowSourceRef(ds map[string]any) string { + if nf := extractString(ds["Nanoflow"]); nf != "" { + return nf + } + if settings, ok := ds["NanoflowSettings"].(map[string]any); ok { + return extractString(settings["Nanoflow"]) + } + return "" +} + // parseCustomWidgetDataSource parses datasource from CustomWidget property format. func parseCustomWidgetDataSource(ctx *ExecContext, ds map[string]any) *rawDataSource { dsType := extractString(ds["$Type"]) @@ -767,20 +801,12 @@ func parseCustomWidgetDataSource(ctx *ExecContext, ds map[string]any) *rawDataSo } return result case "Forms$MicroflowSource": - // Pluggable widgets use Forms$MicroflowSource with MicroflowSettings - if settings, ok := ds["MicroflowSettings"].(map[string]any); ok { - microflow := extractString(settings["Microflow"]) - if microflow != "" { - return &rawDataSource{Type: "microflow", Reference: microflow} - } + if mf := microflowSourceRef(ds); mf != "" { + return &rawDataSource{Type: "microflow", Reference: mf} } case "Forms$NanoflowSource": - // Pluggable widgets use Forms$NanoflowSource with NanoflowSettings - if settings, ok := ds["NanoflowSettings"].(map[string]any); ok { - nanoflow := extractString(settings["Nanoflow"]) - if nanoflow != "" { - return &rawDataSource{Type: "nanoflow", Reference: nanoflow} - } + if nf := nanoflowSourceRef(ds); nf != "" { + return &rawDataSource{Type: "nanoflow", Reference: nf} } case "CustomWidgets$CustomWidgetNanoflowSource": nanoflow := extractString(ds["Nanoflow"]) diff --git a/mdl/executor/layout.go b/mdl/executor/layout.go index 27ba8c01a..b2c694aa2 100644 --- a/mdl/executor/layout.go +++ b/mdl/executor/layout.go @@ -74,6 +74,43 @@ func (m *layoutMeasurer) measureStatements(stmts []ast.MicroflowStatement) Bound return Bounds{Width: totalWidth, Height: maxHeight} } +// measureStatementsSpan returns the horizontal extent a statement run actually +// occupies once laid out, for runs where that can be derived exactly. +// +// measureStatements sums every element's full width and adds HorizontalSpacing +// between them. HorizontalSpacing is a centre-to-centre pitch — the builder does +// `posX += spacing` and centres each activity on posX — so counting it *on top of* +// each width over-measures a run of n simple activities by (n-1)*ActivityWidth. +// Sizing a loop box from that left it far wider than its contents: 880px around +// 440px of activities for a three-statement body (mendixlabs/mxcli#790). +// +// The correction applies only when every element is a simple activity, whose pitch +// is exactly HorizontalSpacing. A compound element (IF/split, nested loop) advances +// posX by geometry this function cannot reproduce without duplicating the builder — +// guessing there under-sizes the box and pushes activities outside it, which is +// worse than a box that is too wide. Such runs fall back to measureStatements. +func (m *layoutMeasurer) measureStatementsSpan(stmts []ast.MicroflowStatement) Bounds { + count := 0 + maxHeight := ActivityHeight + for _, stmt := range stmts { + b := m.measureStatement(stmt) + maxHeight = max(maxHeight, b.Height) + if b.Width == 0 { + continue + } + if b.Width != ActivityWidth { + return m.measureStatements(stmts) // compound element — cannot place exactly + } + count++ + } + if count == 0 { + return Bounds{Width: 0, Height: maxHeight} + } + // n activities centred HorizontalSpacing apart span from the first centre minus + // half a width to the last centre plus half a width. + return Bounds{Width: (count-1)*HorizontalSpacing + ActivityWidth, Height: maxHeight} +} + // measureStatement calculates the bounds for a single statement func (m *layoutMeasurer) measureStatement(stmt ast.MicroflowStatement) Bounds { switch s := stmt.(type) { diff --git a/mdl/executor/layout_span_test.go b/mdl/executor/layout_span_test.go new file mode 100644 index 000000000..b0d616e00 --- /dev/null +++ b/mdl/executor/layout_span_test.go @@ -0,0 +1,75 @@ +// SPDX-License-Identifier: Apache-2.0 + +// mendixlabs/mxcli#790: a loop box was drawn far wider than its contents. +// measureStatements sums each element's full width and adds HorizontalSpacing +// between them, but HorizontalSpacing is a centre-to-centre pitch — the builder +// centres each activity on posX and advances by exactly that — so the sum +// over-counts a run of n simple activities by (n-1)*ActivityWidth. +package executor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" +) + +// simpleStmts returns n statements that each measure to one plain activity box. +func simpleStmts(n int) []ast.MicroflowStatement { + out := make([]ast.MicroflowStatement, 0, n) + for i := 0; i < n; i++ { + out = append(out, &ast.MfCommitStmt{}) + } + return out +} + +func TestMeasureStatementsSpan_SimpleRun(t *testing.T) { + m := &layoutMeasurer{} + tests := []struct { + n int + want int + }{ + {0, 0}, + {1, ActivityWidth}, // 120 + {2, HorizontalSpacing + ActivityWidth}, // 280 + {3, 2*HorizontalSpacing + ActivityWidth}, // 440 — was 680 + } + for _, tc := range tests { + got := m.measureStatementsSpan(simpleStmts(tc.n)).Width + if got != tc.want { + t.Errorf("span of %d activities = %d, want %d", tc.n, got, tc.want) + } + if tc.n > 1 { + // The old measure is the one that over-sized the loop box. + if old := m.measureStatements(simpleStmts(tc.n)).Width; old <= got { + t.Errorf("expected measureStatements (%d) to exceed the true span (%d)", old, got) + } + } + } +} + +// TestMeasureStatementsSpan_CompoundFallsBack: a compound element advances posX by +// geometry this measure cannot reproduce. Guessing there under-sizes the box and +// pushes activities outside it, so such runs must keep the conservative measure. +func TestMeasureStatementsSpan_CompoundFallsBack(t *testing.T) { + m := &layoutMeasurer{} + body := []ast.MicroflowStatement{ + &ast.MfCommitStmt{}, + &ast.IfStmt{ThenBody: simpleStmts(1)}, + &ast.MfCommitStmt{}, + } + span := m.measureStatementsSpan(body).Width + full := m.measureStatements(body).Width + if span != full { + t.Errorf("compound run: span = %d, want the conservative %d", span, full) + } +} + +// TestMeasureStatementsSpan_ZeroWidthIgnored: a RETURN produces no box, so it must +// not contribute a pitch step. +func TestMeasureStatementsSpan_ZeroWidthIgnored(t *testing.T) { + m := &layoutMeasurer{} + withReturn := []ast.MicroflowStatement{&ast.MfCommitStmt{}, &ast.ReturnStmt{}, &ast.MfCommitStmt{}} + if got, want := m.measureStatementsSpan(withReturn).Width, m.measureStatementsSpan(simpleStmts(2)).Width; got != want { + t.Errorf("span with a zero-width statement = %d, want %d", got, want) + } +} diff --git a/mdl/executor/validate_microflow.go b/mdl/executor/validate_microflow.go index 74d86af92..a765a106e 100644 --- a/mdl/executor/validate_microflow.go +++ b/mdl/executor/validate_microflow.go @@ -299,21 +299,6 @@ func (v *microflowValidator) walkBody(body []ast.MicroflowStatement) { stmt.ListVariable), "Pass the list as a microflow parameter instead of creating an empty variable") } - // Check: a `break` nested inside a conditional within a loop currently - // serializes a dangling sequence-flow reference, producing an UNLOADABLE - // .mpr — `mx check` crashes with an unhandled AggregateException - // ("key … not present in the dictionary") rather than an error. A break - // that is a direct child of the loop body serializes fine, but the useful - // form (`if then break`) is the broken one. Reject it with the - // guard-variable workaround until the flow serialization is fixed. (#52) - if loopBodyHasConditionalBreak(stmt.Body) { - v.addViolation("MDL051", linter.SeverityError, - "a `break` inside an if/case within a loop currently produces an unloadable model — "+ - "`mx check` crashes with an unhandled exception (a dangling sequence-flow reference), "+ - "not a normal error. (A break placed directly in the loop body serializes fine.)", - "Until the serialization is fixed, use a guard variable: "+ - "`declare $Done Boolean = false;` then `loop … if not($Done) then … set $Done = true; end if; end loop`.") - } v.loopDepth++ v.walkBody(stmt.Body) v.loopDepth-- @@ -631,77 +616,6 @@ func (v *microflowValidator) checkAssociationObjectArgs(callee string, args []as } } -// loopBodyHasConditionalBreak reports whether a `break` appears inside a -// conditional (if / case / inheritance split) directly within this loop body — the -// pattern that serializes a dangling reference and crashes `mx check` (#52). A -// break that is a *direct* statement of the loop body is not flagged (it -// serializes fine). Nested loops are not descended into: a break there belongs to -// that loop and is validated when it is walked. -func loopBodyHasConditionalBreak(stmts []ast.MicroflowStatement) bool { - for _, s := range stmts { - switch n := s.(type) { - case *ast.IfStmt: - if stmtsContainBreak(n.ThenBody) || stmtsContainBreak(n.ElseBody) { - return true - } - case *ast.EnumSplitStmt: - for _, c := range n.Cases { - if stmtsContainBreak(c.Body) { - return true - } - } - if stmtsContainBreak(n.ElseBody) { - return true - } - case *ast.InheritanceSplitStmt: - for _, c := range n.Cases { - if stmtsContainBreak(c.Body) { - return true - } - } - if stmtsContainBreak(n.ElseBody) { - return true - } - } - } - return false -} - -// stmtsContainBreak reports whether a `break` belonging to the enclosing loop -// appears anywhere in these statements. Descends into conditionals but NOT into -// nested loops (a nested loop traps its own break). -func stmtsContainBreak(stmts []ast.MicroflowStatement) bool { - for _, s := range stmts { - switch n := s.(type) { - case *ast.BreakStmt: - return true - case *ast.IfStmt: - if stmtsContainBreak(n.ThenBody) || stmtsContainBreak(n.ElseBody) { - return true - } - case *ast.EnumSplitStmt: - for _, c := range n.Cases { - if stmtsContainBreak(c.Body) { - return true - } - } - if stmtsContainBreak(n.ElseBody) { - return true - } - case *ast.InheritanceSplitStmt: - for _, c := range n.Cases { - if stmtsContainBreak(c.Body) { - return true - } - } - if stmtsContainBreak(n.ElseBody) { - return true - } - } - } - return false -} - // exprIsAssociationObjectPath reports whether an expression is an attribute path // whose FINAL segment is a module-qualified association (`$obj/Module.Assoc`) — // i.e. it resolves to an associated OBJECT, not an attribute value. A final bare diff --git a/mdl/executor/validate_microflow_hints_test.go b/mdl/executor/validate_microflow_hints_test.go index 6f5959333..d482d4f19 100644 --- a/mdl/executor/validate_microflow_hints_test.go +++ b/mdl/executor/validate_microflow_hints_test.go @@ -161,38 +161,33 @@ func TestValidateMicroflow_AssociationObjectArg(t *testing.T) { } } -// TestValidateMicroflow_ConditionalBreak covers MDL051 (ledger #52): a `break` -// nested inside a conditional within a loop serializes a dangling reference that -// crashes `mx check` (unloadable model). A break directly in the loop body, or no -// break, is fine. -func TestValidateMicroflow_ConditionalBreak(t *testing.T) { - cases := []struct { - name string - body string - wantMDL bool - }{ - {"break inside if in loop", "loop $R in $L begin if $R/Active then break; end if; end loop", true}, - {"break inside nested if in loop", "loop $R in $L begin if $R/Active then if $R/Active then break; end if; end if; end loop", true}, - {"break directly in loop is fine", "loop $R in $L begin break; end loop", false}, - {"no break is fine", "loop $R in $L begin if $R/Active then set $x = 1; end if; end loop", false}, +// TestValidateMicroflow_ConditionalBreakAccepted: MDL051 rejected a `break` or +// `continue` inside a conditional within a loop, because the write path dropped the +// Break/Continue event and left a dangling sequence flow (ledger #52). That was an +// interim guard "until the serialization is fixed" — it now is (mendixlabs/mxcli#791, +// microflowObjectToGen), so the pattern must be accepted again rather than pushing +// users to a guard variable. It also only ever covered `break`, which is why the +// `continue` form reached users as a corrupt project. +func TestValidateMicroflow_ConditionalBreakAccepted(t *testing.T) { + bodies := []string{ + "loop $R in $L begin if $R/Active then break; end if; end loop", + "loop $R in $L begin if $R/Active then continue; end if; end loop", + "loop $R in $L begin if $R/Active then if $R/Active then break; end if; end if; end loop", + "loop $R in $L begin break; end loop", } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - src := "create microflow M.F ($L: list of M.R)\nreturns boolean\nbegin\n " + tc.body + "\n return true;\nend;" + for _, body := range bodies { + t.Run(body, func(t *testing.T) { + src := "create microflow M.F ($L: list of M.R)\nreturns boolean\nbegin\n " + body + "\n return true;\nend;" prog, errs := visitor.Build(src) if len(errs) > 0 { t.Fatalf("parse errors: %v", errs) } mf := prog.Statements[0].(*ast.CreateMicroflowStmt) - var got bool for _, vi := range ValidateMicroflow(mf) { if vi.RuleID == "MDL051" { - got = true + t.Errorf("MDL051 still rejects a now-serializable pattern: %s", body) } } - if got != tc.wantMDL { - t.Errorf("MDL051 fired=%v, want %v (body: %q)", got, tc.wantMDL, tc.body) - } }) } }