diff --git a/.claude/skills/fix-issue/findings/mdl-executor.jsonl b/.claude/skills/fix-issue/findings/mdl-executor.jsonl index 6147f322b2..599e15f989 100644 --- a/.claude/skills/fix-issue/findings/mdl-executor.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-executor.jsonl @@ -502,5 +502,6 @@ {"area": "mdl/executor", "date": "2026-09-01", "symptom": "`mxcli diff` against the UNMODIFIED output of `mxcli describe` reports the script would delete activities the executor round-trips exactly \u2014 every `call java action`, `download file`, `show message`, and every @position/@start/@curve line renders as `-` with no `+`. Retrieve constraints re-render as a Go struct pointer (`where &{0x236139b542d0 index=0}`). An entity dump comes back 'modified' purely on `create PERSISTENT entity` vs `create persistent entity`. Users concluded describe\u2192exec was lossy and stopped trusting the gate.", "cause": "diff rendered its two sides with TWO DIFFERENT RENDERERS. The project side went through the real describer (renderMicroflowMDL, shared with diff-local); the script side went through microflowStmtToMDL in cmd_diff_mdl.go, a second AST\u2192MDL renderer. Its statement switch covered 18 of 43 microflow statement types and had NO default case, so an unhandled activity emitted zero lines silently; it never emitted canvas annotations at all; and diffExpressionToString ended in `default: fmt.Sprintf(\"%v\", expr)`, which prints `&{\u2026}` for the ast.SourceExpr / ast.IdentifierExpr cases it lacked. Separately, diffStatement's `default: return nil, nil // Skip unsupported statements` covered 6 of 158 top-level statement types.", "file": "mdl/executor/cmd_microflows_build.go (new: buildMicroflowFromStmt/buildNanoflowFromStmt split out of the create handlers), mdl/executor/cmd_diff_render.go (new: renderFlowFromModel), mdl/executor/cmd_diff.go, mdl/executor/cmd_diff_mdl.go (376 lines of duplicate renderer deleted)", "insight": "Fix the mechanism, not the cases: build the model from the AST the way exec does (minus the write) and render BOTH sides with the one describer, which makes the false-deletion class unrepresentable. Adding the 25 missing cases would have fixed the symptom and left the drift. Two traps in the build/write split: (1) the create handler's build phase MUTATES \u2014 findOrCreateModule and resolveFolder create documents, consumeDroppedMicroflow consumes session state \u2014 so the dry-run path needs an AllowCreate flag and a read-only lookupFolder, or a read-only `diff` starts writing; (2) exec-only refusals (guard-don't-drop on queued calls, the already-exists error, validateMicroflowRules) must be skipped in dry-run, or a refusal aborts the build and the user gets NO diff instead of a diff plus the warning exec will give them anyway. Silence is the worse half of this bug: a script of statements diff cannot compare summarised as `0 new, 0 modified, 0 unchanged` even when it would add a document \u2014 a wrong count is at least visible. Measuring note: the reporter's control (exec, then OS-diff the re-describe) is good, but `exec` reporting `Unchanged` is stronger \u2014 ADR-0008 elision proves the parsed script is semantically EQUAL to what is stored, i.e. a no-op, not merely lossless. Deleting a renderer orphans its unit tests: #913's split-indentation test drove the dead renderer and was retargeted at formatMicroflowActivities, where the rule now lives.", "refs": ["ako/mxcli#997", "#913"], "rules": []} {"area": "mdl/executor", "cause": "Not the resolver -- the walk. iconRefsInStatement visited only CreatePageStmtV3.Widgets, AlterPageStmt operations and AlterNavigationStmt, so four icon-bearing shapes were never inspected at all, each holding its widgets in a field the walk did not visit: widgets in a `placeholder X { ... }` block (CreatePageStmtV3.Placeholders, held apart from the bare body in .Widgets), CreateSnippetStmtV3, CreateLayoutStmt, and CreateMenuStmt (whose NavMenuItemDef items carry icons exactly as a profile menu's do).", "ce": ["CE1613"], "date": "2026-09-01", "file": "mdl/executor/validate_icon_refs.go (iconRefsInStatement); cross-check mdl/ast/ast_page_v3.go for every widget-bearing field on a statement and mdl/ast/ast_navigation.go for item-bearing ones", "insight": "Enumerate the AST fields; do not trust the obvious one. `Widgets` is not a page's only widget slot. Grep the ast package for statements holding []*WidgetV3 or []NavMenuItemDef and confirm each appears in the switch. Test at the walker, which is where the omission lives -- a test of the resolver passes either way. Pair every refusal with a POSITIVE control: the same four shapes with a VALID icon must still pass --references, exec, and build to 0 errors, or an over-eager walk has traded a silent miss for a false refusal. Measured on 11.13: all four silent at exit 0 and each producing its own CE1613 before, all four caught after, valid variants 0 errors. Note the report was against 0.19.0 while the resolver shipped in 0.18.0 -- when a reported case reproduces as FIXED, probe the neighbouring shapes rather than closing it, because the reporter hit a sibling of what they described. Repro mdl-examples/bug-tests/icon-refs-1008-placeholder-snippet-layout-menu.mdl", "refs": ["mendixlabs/mxcli#1008"], "symptom": "`mxcli check --references` passes and `exec` succeeds on an icon reference naming an IMAGE collection (Images$ImageCollection) instead of an ICON collection, and the first sign is CE1613 \"The selected custom icon 'Mod.Icons_SVG.cmdFilter24' no longer exists.\" from MxBuild. The same bad reference IS caught when the button sits directly in a page body, which makes it read as a resolver bug rather than a coverage gap."} {"area": "mdl/executor", "cause": "Nothing resolved the role. cmd_navigation.go turned the AST's ForRole into a string with QualifiedName.String() and all three navigation writers dropped it into the BSON UserRole key unexamined, so whatever was typed reached disk. The confusion the docs encoded is real: a Mendix USER role is project-level and its identifier is a BARE name, while a MODULE role is module-scoped -- and they share names, so a blank app has a user role Administrator plus module roles called Administrator in three modules. mxcli's own docs, skill and `mxcli syntax` output all recommended the module-qualified form.", "ce": ["CE1613", "StorageLoadException"], "date": "2026-09-01", "file": "mdl/executor/validate_navigation_roles.go (new); wired at mdl/executor/validate.go (--references pass) and mdl/executor/cmd_navigation.go (exec, before any write). Writers that pass the value through: sdk/mpr/writer_navigation.go, mdl/backend/modelsdk/navigation_write.go, modelsdk/mpr/nav_patch.go", "insight": "Grade the failure by SHAPE, not by whether it is caught -- the three values behave differently and only measurement separates them (blank 11.13 app): `for Administrator` 0 errors; `for Supervisor` CE1613, an ordinary build error; `for MyFirstModule.Administrator` a StorageLoadException at LOAD, before checking runs. The load failure is a tier worse and is the one the docs recommended. It also explains a reporting trap seen twice now (also mendixlabs/mxcli#1000): a load failure suppresses the \"The app contains: N errors\" line while still exiting 1, so people read `mx check` as having SUCCEEDED. Never conclude success from the absence of that line; read the exit code. Two controls are load-bearing: (a) exec must refuse AND leave the .mpr byte-identical (md5 before/after) -- a refusal after a partial write is not a fix; (b) a script that CREATEs a user role and then uses it must still pass, because collectDefinitions does not track user roles, so the validator has to scan the program for CreateUserRoleStmt itself or it refuses the ordinary way to write one. Role matching is case-sensitive in Mendix (measured: `for administrator` is CE1613), so report the declared casing rather than silently normalising. gofmt trap: Go 1.19+ doc-comment normalisation rewrites '' into a curly quote, corrupting a quoted Mendix message -- put verbatim error text in an indented (tab) block. Precedent to copy for any user-role reference: applyGuestAccess in mdl/executor/cmd_security_write.go. Repro mdl-examples/bug-tests/navigation-1001-home-page-for-user-role.mdl **The last lesson is the sharpest: a blocker nobody has tried is a guess with a citation.** Seven doctypes sat in the pending list with confident reasons — \"agent editor needs AgentEditorCommons and Mendix 11.9+\", \"needs a reachable $metadata\", \"needs an OpenAPI spec\" — each written by me into a test file where it read as a finding. Tested directly, ALL SEVEN author fine offline against an ordinary 11.13 project: an OData client is created with a warning when $metadata is unreachable, a REST client takes an inline operation with no spec, and the four agent-editor documents need no extra module. The corpus is for measurements; an untested reason belongs in it only when labelled as untested.", "refs": ["mendixlabs/mxcli#1001"], "symptom": "`create or replace navigation ... home page X for MyFirstModule.Administrator` passes `mxcli check --references`, execs cleanly, and produces a project Mendix CANNOT LOAD: \"StorageLoadException: Role based home page in has an invalid value '' for property UserRole. The text 'MyFirstModule.Administrator' is not a valid UserRoleIdentifier.\" `mx check` exits 1 but prints no error count, so it reads as success. The documented syntax was the module-qualified form."} +{"area": "mdl/executor", "date": "2026-09-03", "symptom": "describe -> exec of a mapping silently drops OriginalValue (the sample parsed from the JSON structure's snippet), and reformats the structure's snippet from one line to multi-line. No build error either way — pure diff churn against a Studio Pro original.", "cause": "OriginalValue was written empty on every element (import carried whatever the caller set, which was nothing; export hardcoded \"\"), on the strength of #882's measurement over TWO mappings a blank app ships. describe pretty-prints the snippet and exec stored the pretty form.", "file": "mdl/executor/mapping_original_value.go, mdl/executor/cmd_jsonstructures.go (sameJSONContent), model/types.go (ExportMappingElement.OriginalValue), mdl/backend/modelsdk/mapping_write.go, sdk/mpr/writer_export_mapping.go", "insight": "**Neither global default was right, and measuring the SPLIT is what showed it.** Across 3,042 value elements whose structure carries a sample, 2,322 (76%) store it and 720 do not — but the split is PER DOCUMENT: 145 mappings carry it on every element, 107 on none, 2 mixed. So which one a mapping gets is a property of how and when it was authored, not something derivable. Always-copy is wrong for 107 mappings; always-empty (the old behaviour) is wrong for 145. **A REWRITE does not have to choose** — it knows what was stored, so it carries it (guard-don't-drop, ADR-0005), matching stored to rebuilt by JsonPath because names and order can change while the schema binding cannot. That leaves #882's actual decision intact: a NEWLY authored mapping still writes empty, which is what that issue was about. The general lesson: when a measurement says 'always X' from a small sample and a wider one says 'sometimes X', check whether the split is per-document before picking a default — a per-document split usually means the answer is 'preserve', not 'choose'. The snippet half is the same shape: keep the stored formatting when the JSON is semantically equal (compare decoded values, not strings), so describe -> exec is a no-op instead of a reformat.", "refs": ["ako/mxcli#379", "ako/mxcli#882"]} {"area": "mdl/executor", "date": "2026-09-03", "symptom": "Implementing a new document type from a corpus census alone produces a document that builds at 0 errors and still differs from Studio Pro's in five places — Path, the typed-array marker, an empty mandatory list, PrimitiveType, and a dropped authored field.", "cause": "The census was 36 marketplace-module collections. A module author and someone building an app by hand exercise different parts of a document, so a census over shipped modules misses whatever only hand-authoring sets, and averages away anything the modules happen not to use.", "file": "mdl/backend/modelsdk/messagedefinition_write.go, mdl/executor/cmd_messagedefinitions.go, testdata/TestApp.OrderMessageDefinitions.bson", "insight": "**One hand-authored reference document is worth more than a large census of marketplace modules.** ako/TestApp's OrderMessageDefinitions found five things a 36-collection / 4,686-element census had not: (1) Path is a chain of ORIGINAL names, not exposed ones, and an ASSOCIATION contributes TWO segments — `Order|OrderLine_Order|OrderLine|Amount` — confirmed afterwards at 4,707/4,707 once we knew to look; (2) typed-array marker is 2, the codec defaults to 3; (3) every element serializes Children even when empty (the bare [2], same MandatoryLists rule as a rule document's Flows); (4) PrimitiveType is MAPPED not passed through — Long->Integer, AutoNumber->Integer, Enumeration->String, 279 corpus elements a pass-through gets wrong; (5) Example is author-set — empty in 4,686/4,686 of the corpus, set in TestApp, so hardcoding it empty silently drops the one that exists. **The round-trip test is what finds these**: read a REAL stored document into the semantic model, re-encode, diff against the STORED BYTES. Do not diff against a re-encoding of the decoded original — a lazily-decoded element that was never marked dirty encodes as an empty document, so that baseline passes by comparing nothing to nothing. **A hand-authored document also tends to carry natural controls**: this one uses the same association in both directions, which is exactly the control the cardinality rule needed. **After a CREATE that resolves a folder, invalidate the hierarchy cache** — the cached hierarchy predates the new folder, so a later lookup by module fails and CREATE OR MODIFY writes a DUPLICATE (CE0122). The update branch gets this free from applyDocumentFolder; a create branch has to say it.", "refs": ["ako/mxcli#272"], "ce": ["CE0122", "CE1613"]} {"area": "mdl/executor", "cause": "Two DataTypes$ sub-documents the writer never emitted, both on Microflows$CallExternalAction. (1) edmReturnTypeToKind mapped only EDM primitives and returned \"\" for anything else -- documented in-code as \"Complex / collection / entity-typed returns aren't yet mapped\" -- so an action returning an ENTITY got no VariableDataType at all. (2) ExternalActionParameterMapping.ParameterType was never written, though generated/metamodel declares it WITHOUT omitempty. Separately, mdl/catalog/builder_external.go catalogued only entities whose Source is Rest$ODataRemoteEntitySource, skipping every Rest$ODataEntityTypeSource -- the derived, abstract, contained and action parameter/return types that have no entity set.", "ce": ["CE7252", "CE7269", "CE0117", "CE7251"], "date": "2026-09-03", "file": "mdl/executor/cmd_microflows_builder_calls.go (resolveExternalActionReturnKind, resolveExternalActionParameterKinds, edmBareTypeName); sdk/mpr/writer_microflow_actions.go + mdl/backend/modelsdk/microflow_external_action_write.go (both writers); sdk/microflows/microflows_actions.go (ResultEntity, ParameterDataType/ParameterEntity); mdl/catalog/builder_external.go (isODataEntitySource); new validator mdl/executor/validate_external_action_calls.go", "insight": "**Read the CE code out of Mendix's own assemblies before theorising about it.** `strings Mendix.Modeler.Texts.dll | grep CE7252` gives the symbol, the English text AND the LOCATION comment -- here CallExternalAction.cs for both codes, which settles in one command that the entity import can never fix them and that the reporter was pulling the wrong lever. Same technique found CE7253 and the CE7251 constraint (Mendix's `call external action` takes OData ACTIONS only, not Functions, and an unbound action needs an in the EntityContainer or it is not callable at all). **A missing mandatory sub-document is the recurring shape here**: generated/metamodel omitting `omitempty` on a pointer property is the tell, and the same fix pattern applied twice in one bug. **The reporter's evidence was an artifact of OUR tool**: contract_entities.UsedByExternalEntity is an mxcli catalog column filled by joining external_entities on RemoteName, so while that table skipped type-sourced entities the column was structurally always empty for exactly the entities in question -- it read as 'not linked' whether or not the import had worked. When a report cites one of our own derived columns as evidence, verify the column can be non-empty for that case before believing it. **Verification without a fixture**: no $metadata in the repo declared an action, so the contract was served from `python3 -m http.server` on 127.0.0.1 and MetadataUrl pointed at it -- a local HTTP contract makes the whole consumed-OData path testable end to end. Controls: reverting the return resolver reproduces CE7269 verbatim; before the ParameterType fix, ANY parameter of ANY type produced CE7252 + one CE0117 per argument; after, 0 errors on all three shapes. Repro mdl-examples/bug-tests/odata-1020-external-action-types.mdl", "refs": ["mendixlabs/mxcli#1020"], "symptom": "CE7252 \"The parameters for remote action '' have changed\" and CE7269 \"The return type for remote action '' has changed\" persist after CREATE OR MODIFY EXTERNAL ENTITIES, which reports success and changes nothing. A SQL query over CATALOG.contract_entities shows UsedByExternalEntity empty for the action's parameter/response entities while entity-set entities populate it, which reads as a broken link between the imported entity and the contract."} diff --git a/mdl/backend/modelsdk/mapping_read.go b/mdl/backend/modelsdk/mapping_read.go index 2f4c523fb1..3250620166 100644 --- a/mdl/backend/modelsdk/mapping_read.go +++ b/mdl/backend/modelsdk/mapping_read.go @@ -340,6 +340,7 @@ func exportMappingElementFromGen(el element.Element) *model.ExportMappingElement e.ExposedName = o.ExposedName() e.JsonPath = o.JsonPath() e.XmlPath = o.XmlPath() + e.OriginalValue = o.OriginalValue() e.MinOccurs = int(o.MinOccurs()) e.MaxOccurs = int(o.MaxOccurs()) e.MaxLength = int(o.MaxLength()) diff --git a/mdl/backend/modelsdk/mapping_write.go b/mdl/backend/modelsdk/mapping_write.go index 8f9fc18ca6..df1fb81b49 100644 --- a/mdl/backend/modelsdk/mapping_write.go +++ b/mdl/backend/modelsdk/mapping_write.go @@ -436,7 +436,10 @@ func exportValueElementToGen(id string, elem *model.ExportMappingElement, parent addBool(g, "IsKey", elem.IsKey) addBool(g, "IsContent", false) addBool(g, "IsXmlAttribute", false) - addStr(g, "OriginalValue", "") + // Carried, not hardcoded: whether a mapping stores the structure's sample is + // a per-document property, so a rewrite preserves what was there rather than + // deleting it (ako/mxcli#379). A newly authored mapping still gets "". + addStr(g, "OriginalValue", elem.OriginalValue) addStr(g, "XmlPrimitiveType", xmlPrimitiveTypeName(elem.DataType)) return g } diff --git a/mdl/executor/cmd_export_mappings.go b/mdl/executor/cmd_export_mappings.go index 1134573045..a1e172fa9c 100644 --- a/mdl/executor/cmd_export_mappings.go +++ b/mdl/executor/cmd_export_mappings.go @@ -410,6 +410,9 @@ func finishExportMapping(ctx *ExecContext, s *ast.CreateExportMappingStmt, ) error { if existing != nil { em.ID = existing.ID + // A rewrite must not delete the samples the stored document carries + // (ako/mxcli#379). + carryExportOriginalValues(em, existing) if err := ctx.Backend.UpdateExportMapping(em); err != nil { return mdlerrors.NewBackend("update export mapping", err) } diff --git a/mdl/executor/cmd_import_mappings.go b/mdl/executor/cmd_import_mappings.go index d73a2b0710..5644c7ebf9 100644 --- a/mdl/executor/cmd_import_mappings.go +++ b/mdl/executor/cmd_import_mappings.go @@ -512,6 +512,9 @@ func finishImportMapping(ctx *ExecContext, s *ast.CreateImportMappingStmt, } if existing != nil { im.ID = existing.ID + // A rewrite must not delete the samples the stored document carries + // (ako/mxcli#379). + carryImportOriginalValues(im, existing) if err := ctx.Backend.UpdateImportMapping(im); err != nil { return mdlerrors.NewBackend("update import mapping", err) } @@ -609,14 +612,14 @@ func buildImportMappingElementModel(moduleName string, def *ast.ImportMappingEle elem.MinOccurs = jsElem.MinOccurs elem.MaxOccurs = jsElem.MaxOccurs elem.Nillable = jsElem.Nillable - // OriginalValue is deliberately NOT cloned. It is the sample value parsed - // out of the JSON structure's snippet ("42", "\"Widget\""), and it belongs - // to the STRUCTURE — Studio Pro leaves it empty on every mapping element. - // Measured across the two Studio-Pro-authored mappings a blank app ships - // (FeedbackModule's IMM_PostResponse and EMM_PostFeedback, ~15 value - // elements between them): all "", while their structures carry 17 non-empty - // samples. Copying the sample in makes an mxcli-written mapping differ from - // a Studio-Pro-written one over the same structure. (issue #882) + // OriginalValue is deliberately NOT cloned from the structure — a NEW + // mapping gets an empty one (#882). That decision stands, but its + // original measurement was too narrow: it read two mappings a blank app + // ships, and at corpus scale 2,322 of 3,042 value elements DO carry the + // sample. The split is per document (145 mappings all, 107 none, 2 + // mixed), so it is not derivable — which is why a REWRITE carries the + // stored value forward instead of choosing. See + // carryImportOriginalValues (ako/mxcli#379). elem.FractionDigits = jsElem.FractionDigits elem.TotalDigits = jsElem.TotalDigits elem.MaxLength = jsElem.MaxLength diff --git a/mdl/executor/cmd_jsonstructures.go b/mdl/executor/cmd_jsonstructures.go index d1a736d48e..c2c0cd5e6f 100644 --- a/mdl/executor/cmd_jsonstructures.go +++ b/mdl/executor/cmd_jsonstructures.go @@ -4,7 +4,9 @@ package executor import ( + "encoding/json" "fmt" + "reflect" "sort" "strings" "unicode" @@ -295,6 +297,14 @@ func execCreateJsonStructure(ctx *ExecContext, s *ast.CreateJsonStructureStmt) e JsonSnippet: types.PrettyPrintJSON(s.JsonSnippet), Elements: elements, } + // Keep the stored snippet's FORMATTING when the content is the same. mxcli + // pretty-prints on describe, so describe -> exec — how a document is copied + // — otherwise rewrote a snippet Studio Pro had stored on one line into a + // multi-line one. Same JSON, different bytes, and a diff against the + // original for nothing (ako/mxcli#379). + if existing != nil && sameJSONContent(existing.JsonSnippet, js.JsonSnippet) { + js.JsonSnippet = existing.JsonSnippet + } // A rewrite that carried no doc comment keeps the stored one (#1018). if existing != nil { js.Documentation = carriedDocumentation(s.DocumentationSet, s.Documentation, existing.Documentation) @@ -365,3 +375,23 @@ func findJsonStructure(ctx *ExecContext, moduleName, structName string) *types.J } return nil } + +// sameJSONContent reports whether two snippets carry the same JSON, ignoring +// whitespace. Comparing the decoded values rather than the strings is the point: +// the question is whether a rewrite would change anything that matters. +// +// Anything that does not parse is treated as different, so a malformed snippet +// is replaced rather than silently kept. +func sameJSONContent(a, b string) bool { + if a == b { + return true + } + var va, vb any + if err := json.Unmarshal([]byte(a), &va); err != nil { + return false + } + if err := json.Unmarshal([]byte(b), &vb); err != nil { + return false + } + return reflect.DeepEqual(va, vb) +} diff --git a/mdl/executor/mapping_original_value.go b/mdl/executor/mapping_original_value.go new file mode 100644 index 0000000000..7e6cbc26a2 --- /dev/null +++ b/mdl/executor/mapping_original_value.go @@ -0,0 +1,116 @@ +// SPDX-License-Identifier: Apache-2.0 + +// Carrying a mapping value element's OriginalValue through a rewrite +// (ako/mxcli#379). +// +// OriginalValue is the sample parsed out of the JSON structure's snippet +// ("42", "\"Widget\""). mxcli wrote it empty on every element, on the strength +// of a measurement over two mappings a blank app ships (#882) — and a rewrite +// therefore DELETED it from every mapping that had one. +// +// The wider measurement says neither "always empty" nor "always copy the +// sample" is right. Across 3,042 value elements whose structure carries a +// sample, 2,322 (76%) store it and 720 do not — and the split is PER DOCUMENT, +// not per element: +// +// 145 mappings carry the sample on EVERY element +// 107 carry it on NONE +// 2 are mixed +// +// So which one a mapping gets is a property of how and when it was authored, +// which mxcli cannot compute. Choosing a global default is wrong for roughly +// half the corpus either way. +// +// A REWRITE does not have to choose. It knows what was stored, so it carries it +// — guard-don't-drop, ADR-0005. That leaves #882's actual decision intact: a +// NEWLY created mapping still writes empty, which is what that issue was about. +package executor + +import "github.com/mendixlabs/mxcli/model" + +// carryImportOriginalValues copies each stored element's OriginalValue onto the +// rebuilt element at the same JsonPath. +// +// Matching is by JsonPath because that is what identifies an element against +// the schema: names can be renamed and order can change, but a value element +// bound to a different path is a different element. +func carryImportOriginalValues(rebuilt, stored *model.ImportMapping) { + if rebuilt == nil || stored == nil { + return + } + byPath := map[string]string{} + collectImportOriginalValues(stored.Elements, byPath) + if len(byPath) == 0 { + return + } + applyImportOriginalValues(rebuilt.Elements, byPath) +} + +func collectImportOriginalValues(elems []*model.ImportMappingElement, out map[string]string) { + for _, e := range elems { + if e == nil { + continue + } + if e.OriginalValue != "" { + out[e.JsonPath] = e.OriginalValue + } + collectImportOriginalValues(e.Children, out) + } +} + +func applyImportOriginalValues(elems []*model.ImportMappingElement, byPath map[string]string) { + for _, e := range elems { + if e == nil { + continue + } + // Only fill an element the rebuild left empty: a statement that somehow + // set one should win over what was stored. + if e.OriginalValue == "" { + if v, ok := byPath[e.JsonPath]; ok { + e.OriginalValue = v + } + } + applyImportOriginalValues(e.Children, byPath) + } +} + +// carryExportOriginalValues is the export twin. An export mapping's value +// elements hardcoded "" in the codec writer rather than carrying the field at +// all, so this needed the semantic type to reach the writer as well. +func carryExportOriginalValues(rebuilt, stored *model.ExportMapping) { + if rebuilt == nil || stored == nil { + return + } + byPath := map[string]string{} + collectExportOriginalValues(stored.Elements, byPath) + if len(byPath) == 0 { + return + } + applyExportOriginalValues(rebuilt.Elements, byPath) +} + +func collectExportOriginalValues(elems []*model.ExportMappingElement, out map[string]string) { + for _, e := range elems { + if e == nil { + continue + } + if e.OriginalValue != "" { + out[e.JsonPath] = e.OriginalValue + } + collectExportOriginalValues(e.Children, out) + } +} + +func applyExportOriginalValues(elems []*model.ExportMappingElement, byPath map[string]string) { + for _, e := range elems { + if e == nil { + continue + } + if e.OriginalValue == "" { + if v, ok := byPath[e.JsonPath]; ok { + e.OriginalValue = v + } + } + applyExportOriginalValues(e.Children, byPath) + } +} diff --git a/mdl/executor/mapping_original_value_test.go b/mdl/executor/mapping_original_value_test.go new file mode 100644 index 0000000000..bfc3a822ba --- /dev/null +++ b/mdl/executor/mapping_original_value_test.go @@ -0,0 +1,148 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/model" +) + +// OriginalValue is the sample parsed out of a JSON structure's snippet. mxcli +// wrote it empty on every element, so a rewrite DELETED it from every mapping +// that had one — 2,322 of 3,042 value elements in the demo corpus carry one +// (ako/mxcli#379). +// +// Neither global default is right. The split is PER DOCUMENT: 145 mappings +// carry the sample on every element, 107 on none, 2 mixed. So a rewrite +// preserves what was stored instead of choosing, and a NEWLY authored mapping +// still writes empty — which is what #882 actually decided. + +func importElem(path, value string, kids ...*model.ImportMappingElement) *model.ImportMappingElement { + return &model.ImportMappingElement{JsonPath: path, OriginalValue: value, Children: kids} +} + +func TestImportOriginalValuesAreCarriedThroughARewrite(t *testing.T) { + stored := &model.ImportMapping{Elements: []*model.ImportMappingElement{ + importElem("(Object)", "", + importElem("(Object)|title", `"hello"`), + importElem("(Object)|nested", "", + importElem("(Object)|nested|qty", `"3"`)), + ), + }} + rebuilt := &model.ImportMapping{Elements: []*model.ImportMappingElement{ + importElem("(Object)", "", + importElem("(Object)|title", ""), + importElem("(Object)|nested", "", + importElem("(Object)|nested|qty", "")), + ), + }} + + carryImportOriginalValues(rebuilt, stored) + + root := rebuilt.Elements[0] + if got := root.Children[0].OriginalValue; got != `"hello"` { + t.Errorf("title = %q, want \"hello\"", got) + } + // Nesting matters: a mapping's samples are not all at the top level. + if got := root.Children[1].Children[0].OriginalValue; got != `"3"` { + t.Errorf("nested qty = %q, want \"3\"", got) + } +} + +// TestOriginalValuesMatchOnJsonPath pins the matching key. Names can be renamed +// and order can change; a value element bound to a different path is a +// different element, so the path is what identifies it against the schema. +func TestOriginalValuesMatchOnJsonPath(t *testing.T) { + stored := &model.ImportMapping{Elements: []*model.ImportMappingElement{ + importElem("(Object)", "", importElem("(Object)|title", `"hello"`)), + }} + rebuilt := &model.ImportMapping{Elements: []*model.ImportMappingElement{ + importElem("(Object)", "", importElem("(Object)|somethingElse", "")), + }} + + carryImportOriginalValues(rebuilt, stored) + + if got := rebuilt.Elements[0].Children[0].OriginalValue; got != "" { + t.Errorf("carried %q onto a different path — the sample belongs to the element it was measured on", got) + } +} + +// TestARewriteDoesNotOverwriteAnExplicitValue pins the precedence: what the +// rebuild produced wins, and the stored value only fills a gap. +func TestARewriteDoesNotOverwriteAnExplicitValue(t *testing.T) { + stored := &model.ImportMapping{Elements: []*model.ImportMappingElement{ + importElem("(Object)|x", `"old"`), + }} + rebuilt := &model.ImportMapping{Elements: []*model.ImportMappingElement{ + importElem("(Object)|x", `"new"`), + }} + + carryImportOriginalValues(rebuilt, stored) + + if got := rebuilt.Elements[0].OriginalValue; got != `"new"` { + t.Errorf("OriginalValue = %q, want the rebuilt value", got) + } +} + +// TestANewMappingKeepsEmptyOriginalValues is the control for #882. With no +// stored document there is nothing to carry, so a newly authored mapping still +// writes empty — the decision that issue made, left intact. +func TestANewMappingKeepsEmptyOriginalValues(t *testing.T) { + rebuilt := &model.ImportMapping{Elements: []*model.ImportMappingElement{ + importElem("(Object)|title", ""), + }} + + carryImportOriginalValues(rebuilt, nil) + + if got := rebuilt.Elements[0].OriginalValue; got != "" { + t.Errorf("OriginalValue = %q on a new mapping, want empty", got) + } +} + +// TestExportOriginalValuesAreCarriedToo pins the export twin, whose codec +// writer hardcoded "" rather than carrying the field at all. +func TestExportOriginalValuesAreCarriedToo(t *testing.T) { + stored := &model.ExportMapping{Elements: []*model.ExportMappingElement{ + {JsonPath: "(Object)", Children: []*model.ExportMappingElement{ + {JsonPath: "(Object)|title", OriginalValue: `"hello"`}, + }}, + }} + rebuilt := &model.ExportMapping{Elements: []*model.ExportMappingElement{ + {JsonPath: "(Object)", Children: []*model.ExportMappingElement{ + {JsonPath: "(Object)|title"}, + }}, + }} + + carryExportOriginalValues(rebuilt, stored) + + if got := rebuilt.Elements[0].Children[0].OriginalValue; got != `"hello"` { + t.Errorf("title = %q, want \"hello\"", got) + } +} + +// TestSameJSONContentIgnoresFormatting pins the snippet half. describe +// pretty-prints, so describe -> exec rewrote a one-line snippet into a +// multi-line one — same JSON, different bytes, a diff for nothing. +func TestSameJSONContentIgnoresFormatting(t *testing.T) { + cases := []struct { + name string + a, b string + want bool + }{ + {"formatting only", `{"title": "hello", "qty": "3"}`, "{\n \"title\": \"hello\",\n \"qty\": \"3\"\n}", true}, + {"key order", `{"a":1,"b":2}`, `{"b":2,"a":1}`, true}, + {"different value", `{"a":1}`, `{"a":2}`, false}, + {"added key", `{"a":1}`, `{"a":1,"b":2}`, false}, + // Anything that does not parse counts as different, so a malformed + // snippet is replaced rather than silently kept. + {"malformed", `{"a":`, `{"a":1}`, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := sameJSONContent(tc.a, tc.b); got != tc.want { + t.Errorf("sameJSONContent = %v, want %v", got, tc.want) + } + }) + } +} diff --git a/model/types.go b/model/types.go index e497fbde09..1395509e3a 100644 --- a/model/types.go +++ b/model/types.go @@ -1359,6 +1359,11 @@ type ExportMappingElement struct { // Shared fields ExposedName string `json:"exposedName,omitempty"` JsonPath string `json:"jsonPath,omitempty"` + // OriginalValue is the sample parsed out of the JSON structure's snippet. + // Carried rather than derived: whether a mapping stores it is a per-document + // property mxcli cannot compute, so a rewrite preserves what was there + // instead of choosing (ako/mxcli#379). + OriginalValue string `json:"originalValue,omitempty"` // XmlPath — see the note on ImportMappingElement. XmlPath string `json:"xmlPath,omitempty"` Children []*ExportMappingElement `json:"children,omitempty"` diff --git a/sdk/mpr/parser_export_mapping.go b/sdk/mpr/parser_export_mapping.go index abfc2ef9a2..62b64b26e7 100644 --- a/sdk/mpr/parser_export_mapping.go +++ b/sdk/mpr/parser_export_mapping.go @@ -151,6 +151,9 @@ func parseExportValueMappingElement(raw map[string]any) *model.ExportMappingElem if v, ok := raw["Converter"].(string); ok { elem.Converter = v } + if v, ok := raw["OriginalValue"].(string); ok { + elem.OriginalValue = v + } // Extract the primitive type from the nested Type object if typeObj, ok := raw["Type"].(map[string]any); ok { diff --git a/sdk/mpr/writer_export_mapping.go b/sdk/mpr/writer_export_mapping.go index ebb67d482e..a1e7d767f3 100644 --- a/sdk/mpr/writer_export_mapping.go +++ b/sdk/mpr/writer_export_mapping.go @@ -199,7 +199,7 @@ func serializeExportValueElement(id string, elem *model.ExportMappingElement, pa {Key: "IsKey", Value: elem.IsKey}, {Key: "IsContent", Value: false}, {Key: "IsXmlAttribute", Value: false}, - {Key: "OriginalValue", Value: ""}, + {Key: "OriginalValue", Value: elem.OriginalValue}, {Key: "XmlPrimitiveType", Value: xmlPrimitiveTypeName(elem.DataType)}, } }