diff --git a/.claude/skills/fix-issue/findings/mdl-backend.jsonl b/.claude/skills/fix-issue/findings/mdl-backend.jsonl index 0542e5a82..b42032884 100644 --- a/.claude/skills/fix-issue/findings/mdl-backend.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-backend.jsonl @@ -148,3 +148,4 @@ {"area": "mdl/backend", "date": "2026-09-29", "symptom": "ALTER ASSOCIATION / CREATE OR MODIFY ASSOCIATION / RENAME / mxcli layout on a Studio Pro module turns every API-exported entity in the module Hidden and rewrites every entity's access rules (50 of 93 elements in TestApp WorkflowCommons changed for a no-op write); ALTER ENTITY and MOVE ENTITY reset the target's (and its attributes') export level the same way. mx check stays clean (#801)", "cause": "entityToGen / attributeToGen / assocToGen SET ExportLevel \"Hidden\", and a property the rebuild sets is dirty and wins over the carried raw bytes; UpdateDomainModel rebuilt every entity and association, not only the one named. Member-access writers set only the reference in use, while Studio Pro writes both Attribute and Association keys (the unused one as \"\"), so every rebuilt rule differed from its stored self", "file": "mdl/backend/modelsdk/domainmodel_carry.go", "fix": "UpdateDomainModel passes an entity/association through as its stored element when the semantic model equals the stored one read back (entityUnchanged/assocUnchanged, ID- and nil/empty-insensitive semanticEqual). Rebuilds of existing entities (UpdateEntity, UpdateDomainModel, MoveEntity) go through carryStoredEntity: raw + child identity + stored ExportLevel (entity and carried attributes) + unchanged access rules substituted by the stored element, changed ones keep $ID/caption/documentation. carryStoredAssociation: raw + ExportLevel + unchanged delete behaviour. DomainModels$MemberAccess registers EmptyStringFields Attribute/Association", "insight": "A raw carry only preserves what the converter does NOT set; every constant a converter writes (here ExportLevel \"Hidden\") is still reset on the carried element, so audit the converter's Set* constants, not only unmodelled keys. The whole-list rebuild shape needs an 'unchanged -> pass the stored element' short-circuit or its blast radius is the unit. Invisible on mxcli-authored content, where the stored value IS the constant: TestApp is the only fixture with API entities (WorkflowCommons) and has no API attribute, so the attribute case was set up on top of it. The roundtrip harness had 87 getput entries (#721 B and TestApp associations) that were this bug", "refs": ["ako/mxcli#801", "ako/mxcli#721"], "test": "TestIssue801_UpdateDomainModelChangesOnlyTheAlteredElement, TestIssue801_OtherEntityRebuildsKeepExportLevel"} {"area": "mdl/backend", "date": "2026-09-29", "symptom": "describe -> exec, CREATE OR MODIFY or an ALTER that rebuilds the document turns an API-exported document Hidden: enumerations, pages, layouts, rules, view-entity OQL source documents, import/export mappings, JSON structures, published and consumed REST services, scheduled events, workflows, database connections, business event services, data transformers, queues, regular expressions and agent-editor documents. The run reports success, mx check is clean; the module's public surface silently shrinks. A workflow's own `export level API` clause was a no-op on create and on rewrite.", "cause": "Each rewrite converter builds a fresh document and writes ExportLevel as a constant (\"Hidden\"), or passes the semantic model's value where the executor itself filled in \"Hidden\" (mappings, database connection, business events), and the unit is replaced wholesale. MDL has no export-level spelling for most of these kinds, so describe cannot print it and the executed script cannot restore it. workflowToGen ignored wf.ExportLevel entirely. The round-trip harness could not see it: every document in TestApp and PedApp is Hidden, the constant itself.", "file": "mdl/backend/modelsdk/export_level_carry.go", "fix": "One byte-level carry, keepStoredExportLevel(unitID, contents): replaces only the top-level ExportLevel element of the freshly encoded rewrite with the stored value, copying every other element verbatim, and never adds the key. Wired into every Update path that writes ExportLevel (UpdateEnumeration/Rule/Layout/ImportMapping/ExportMapping/JsonStructure/PublishedRestService/ConsumedRestService/DataTransformer/DatabaseConnection/BusinessEventService, writeCustomBlob update, WriteViewEntitySourceDocument update; page via carryStoredPageHeader). Kinds with an MDL spelling (workflow, scheduled event, queue, regular expression) use keepStoredExportLevelUnlessSet: an authored level wins. workflowToGen now writes orDefault(wf.ExportLevel, \"Hidden\").", "insight": "A fixture-driven round trip is blind to any constant that happens to equal every fixture value: 775 TestApp documents round-tripped while 10 kinds hid API documents. Set the subject to the non-default value first (here: patch ExportLevel to API on the working copy) and run both the plain describe output and an edited one, because an elided unchanged write passes a converter that still writes the constant. Carrying at the encoded-bytes level covers gen-typed, newElem-built and hand-serialized writers with one helper, where a gen setter per converter would have needed three mechanisms.", "refs": ["ako/mxcli#816", "ako/mxcli#801", "ako/mxcli#812"], "test": "mdl/backend/modelsdk/issue816_export_level_test.go (TestUpdatePaths_KeepStoredExportLevel, 18 kinds); mdl/roundtrip/export_level_test.go (TestTestAppExportLevelSurvivesRoundTrip, -tags integration)"} {"date": "2026-09-30", "area": "mdl/backend", "symptom": "ako/mxcli#859 review of PR #864: after the built comparison landed, changing or adding `show page M.P with title = 'X'` in a `create or modify microflow` reported \"Unchanged microflow\" and wrote nothing, under mdl 0 and mdl 1 (main spliced it). Nothing warned.", "cause": "builtAsStored compares the declared flow and the stored flow both READ BACK through the codec, so any property the reader drops compares equal whatever either side holds. The ShowFormAction reader never read FormSettings.TitleOverride. Probing encode(built) against encode(readback(built)) over mdl-examples found the reader also dropped ExclusiveSplit/LoopedActivity ErrorHandlingType and a REST call's bound output variable (ResultHandling.ResultVariableName -> RestCallAction.OutputVariable), plus CallWebServiceAction (#861). Before the built comparison such a loss was a visible phantom re-splice; after it, a silently dropped edit.", "fix": "ReadBackMicroflow/ReadBackNanoflow re-encode what they read back and refuse (error -> statement diff, the pre-#859 path) when it is not the document first written, $IDs aside (sameWritten). The reader now reads TitleOverride, the split's and loop's ErrorHandlingType, and a bound REST call's OutputVariable, so those flows keep matching.", "insight": "A comparison made on both sides through the same lossy reader cannot see what the reader loses; the lost property becomes a change that is never written. When equality is decided after a decode, prove the decode lossless for the value at hand (write it again and compare bytes) and fall back when it is not. The probe that found the fields: diff encode(x) with encode(decode(encode(x))) over every mdl-examples flow.", "issue": "ako/mxcli#859", "file": "mdl/backend/modelsdk/microflow_readback.go, mdl/backend/modelsdk/microflow_read_actions.go, mdl/backend/modelsdk/microflow.go, mdl/roundtrip/flow_idempotent_shapes_test.go"} +{"date": "2026-09-30", "area": "mdl/backend", "symptom": "ako/mxcli#843 (rehearsal M2): under mdl 1, `create or modify nanoflow … returns Boolean as $Done` over a nanoflow stored without a return variable refuses \"the stored document has no ReturnVariableName property … set it in Studio Pro\"; the same statement on a microflow reports \"set: ReturnVariableName\".", "cause": "mfmutator.SetHeader refuses any stated header key the stored document lacks (a key the project version does not declare makes the document unopenable). mxcli's nanoflow writer omits ReturnVariableName when the statement has no `as $Var`, while the microflow writer always writes it on 10+, so only nanoflows hit the refusal.", "fix": "Optional mfmutator.PropertyDeclarer on Deps; the codec deps answer from the metamodel version data (type, then Microflows$MicroflowBase; ReturnVariableName is 10.12+) against the project version, and SetHeader inserts the key after its predecessor in the encoder's order. No answer (MCP, unknown version) keeps the refusal.", "insight": "A refusal keyed on 'the stored document lacks the key' conflates 'this version has no such property' with 'the writer left it out'; the metamodel version data separates the two. Studio Pro 11 stores ReturnVariableName on every nanoflow (PedApp: 13 of 13), so adding it matches what Studio Pro writes.", "issue": "ako/mxcli#843", "file": "mdl/backend/mfmutator/header.go"} diff --git a/.claude/skills/fix-issue/findings/mdl-executor.jsonl b/.claude/skills/fix-issue/findings/mdl-executor.jsonl index f39ef2e30..7e1983e9d 100644 --- a/.claude/skills/fix-issue/findings/mdl-executor.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-executor.jsonl @@ -772,3 +772,12 @@ {"date": "2026-09-30", "area": "mdl/executor", "symptom": "ako/mxcli#859 (rehearsal V1): a view entity attribute declared Long over an AutoNumber source column was accepted on the run that created the source entity and refused on every later run as a reference error (\"declared as Long but OQL expression returns AutoNumber. Fix: change to AutoNumber\") — under mdl 0 too, a new rejection. The suggested AutoNumber gives CE6770 \"View Entity is out of sync\"; Long passes mx check (measured on PedApp, 11.13).", "cause": "typesCompatible compared the source attribute's AutoNumber kind exactly; on the first run the source did not exist yet so inference returned Unknown and nothing was checked.", "fix": "An AutoNumber column is compatible with a declared Long (and what Long is compatible with), the suggestion names Long (viewColumnType); a declared AutoNumber stays accepted as before (refusing it would be a new mdl 0 rejection).", "insight": "A check that only runs once its inputs exist behaves differently on the first and second run of the same script; measure the suggested fix with mx check before trusting the check's type table.", "issue": "ako/mxcli#859", "file": "mdl/executor/oql_type_inference.go"} {"date": "2026-09-30", "area": "mdl/executor", "symptom": "ako/mxcli#859 (rehearsal M4): replacing `set $Url = '/odata/v1?$filter=' + $F` by create or modify under mdl 1 (and alter microflow) was refused: \"the fragment uses $filter, which is not declared\".", "cause": "The splice's scope checks (checkFragmentScope, noteFragment, checkOutputUnused) find variables with a `\\$name` regexp over the activity's MDL text, string literals included.", "fix": "withoutStringLiterals blanks '…' literal contents (doubled quotes handled) before the regexp at all three call sites.", "insight": "Regexp scans over rendered MDL must skip literals; enumerate every call site of the same regexp, not just the one in the report.", "issue": "ako/mxcli#859", "file": "mdl/executor/cmd_alter_flow.go"} {"date": "2026-09-30", "area": "mdl/executor", "symptom": "ako/mxcli#859 (S3, review of #863): an identical re-run still re-spliced `change $X (M.E.Attr = …)` / `find $L by M.E.Attr = …` every run when $X or $L came from a microflow call, a retrieve over an association, or a loop over such a list (\"spliced: 3 replaced, 1 dropped\"), under mdl 0 and mdl 1.", "cause": "describedMemberSpellings typed variables only from statements that name an entity (retrieve from an entity, create, declare, parameters); a variable whose declaring statement names none had no entity, so its members kept the qualified spelling and never matched describe's short one.", "fix": "builtFlow/builtNanoflow expose the builder's varTypes; planFlowModify builds the declared body (reusing the header-change build when there is one) and seeds the speller with those types for variables the statements leave untyped.", "insight": "A compare-only normalizer that needs types must take them from the builder, not re-derive a subset; probe it with variables whose type only the builder can infer (call results, association retrieves).", "issue": "ako/mxcli#859", "file": "mdl/executor/flow_member_spelling.go"} +{"date": "2026-09-30", "area": "mdl/executor", "symptom": "ako/mxcli#859 (rehearsal M3): `create or modify microflow|nanoflow` refused under mdl 1 with \"the annotations on the replaced change; the splice keeps a replaced activity's notes as stored\" whenever a statement added, reworded or took off a note (ledger BUILD_ScatterUrl), and on the Studio Pro PedApp nanoflows ACT_Feedback_TriggerScreenshotMode / _UploadImage when their mdl 0 description (a note with `\\r\\n`) was run under the `mdl 1;` header. Refused on every run, so the script could never be re-run; `the dropped carries an annotation` was the same limit for a drop.", "cause": "keepStoredNotes only knew how to keep a replaced activity's stored notes (the mutator's Replace re-points their lines to the fragment entry); any other notes on the declared statement were refused, and a dropped activity with notes was refused because its notes would be left unattached.", "fix": "A new MicroflowMutator.RemoveNotes(target) takes out the annotations attached to an activity and their lines (refusing a note also attached to another object); create or modify marks a replace/drop whose declared notes differ (AlterFlowOperation.ReplaceNotes), the applier removes the stored notes first, and the fragment builder draws the declared ones. A note describe gives an id (shared) is still refused.", "insight": "The mdl 0 description run under the mdl 1 header is a real change wherever a string holds a backslash escape, so a describe -> exec property that only feeds a flow its OWN mdl 1 description can never see it; TestSpliceRerun_PedAppNanoflowsUnderTheHeader feeds the header-only upgrade. A refusal of a legitimate change is indistinguishable, to a user re-running a script, from non-idempotence.", "issue": "ako/mxcli#859", "file": "mdl/executor/cmd_flow_modify.go"} +{"date": "2026-09-30", "area": "mdl/executor", "symptom": "ako/mxcli#859: `commit $E on error rollback` (any activity with an explicit `on error rollback`) never matched its stored activity in the create or modify statement diff, so whenever anything else in the flow changed the activity was dropped and written again (\"spliced: 1 replaced, 1 dropped\" for a one-activity edit): new $ID, curves redrawn. An identical re-run was saved only by the built-graph comparison.", "cause": "describe never prints `on error rollback` (formatErrorHandlingSuffix: it is the stored default and cannot be told from no clause), so the stored side's ErrorHandling is nil while the declared side has a Rollback clause.", "fix": "matchValue treats a bare `on error rollback` clause (no body) as nil on both sides (withoutDefaultErrorHandling in flow_declared_match.go).", "insight": "Every default describe omits needs the same normalisation on the declared side of the statement diff, or the statement re-splices with every sibling change; the built-graph comparison hides it on identical re-runs, so probe with a sibling edit and assert the splice summary, not just the second run.", "issue": "ako/mxcli#859", "file": "mdl/executor/flow_declared_match.go"} +{"date": "2026-09-30", "area": "mdl/executor", "symptom": "ako/mxcli#869: `describe microflow` printed `show page M.P` without its `with title = '\u2026'` override, so describe -> exec dropped it and taking the override out of a `create or modify microflow` reported \"Unchanged microflow\" and wrote nothing, under mdl 0 and mdl 1.", "cause": "formatAction's ShowPageAction case ignored OverridePageTitle; the statement diff compares against describe, and the built comparison sees no difference once the declared side has no override and describe's side has none either.", "fix": "Describe appends ` with title = ` when the override has text in the describe language; a textless override has no statement form and is left out.", "insight": "A \"removed\" edit is the one only describe can see: the reader carrying a property is not enough when the statement diff is fed by describe. Test add, change AND take-out for every property the reader learns.", "issue": "ako/mxcli#869", "file": "mdl/executor/cmd_microflows_format_action.go"} +{"date": "2026-09-30", "area": "mdl/executor", "symptom": "ako/mxcli#861: an identical re-run of a `call web service` statement re-spliced the activity on every run (\"spliced: 1 replaced\"), under mdl 0 and mdl 1; the built comparison reported `RawBSON[14]: 196 built, 139 stored`. mdl-examples' raw-payload call (`call web service raw '\u2026'` without ErrorHandlingType) was re-spliced too, hidden behind the same rerunKnownFailures entry.", "cause": "sameBuiltFlow compared WebServiceCallAction.RawBSON as bytes, and the raw document carries the $IDs every build mints. Separately, webServiceActionRequiresRawBSON admitted a stored call that lacked keys the structured writer emits (ErrorHandlingType, \u2026) or held a by-ID ImportedService, so describe printed a structured form that writes a different document, and the read-back guard sent the flow to the statement diff where raw and structured never match.", "fix": "sameBuiltFlow compares a []byte raw document with every $ID dropped at any depth (sameRawModuloIDs); webServiceActionRequiresRawBSON requires raw when a key of webServiceWrittenKeys is absent or ImportedService is not a string, and a test pins that list to the writer's output.", "insight": "A listed known failure hides every other defect in the same script: taking an entry off the list surfaced a second, unrelated re-splice. Opaque payloads in the model need the same ID-agnostic comparison as the objects around them.", "issue": "ako/mxcli#861", "file": "mdl/executor/flow_built_match.go"} +{"date": "2026-09-30", "area": "mdl/executor", "symptom": "ako/mxcli#843 (rehearsal M5): a microflow or nanoflow that calls itself fails `exec` with \"CALL MICROFLOW 'M.F': microflow not found in the project\" while `check` passes the same script; the stub-then-real workaround scripts used instead is refused under mdl 1 by the splice (\"insert before return false: the fragment returns\").", "cause": "flowBuilder resolved a call target (existence and return type) against the project only; on a first create the flow being built is not stored yet. check resolves names against the script's own declarations, so the two disagreed.", "fix": "buildMicroflowFromStmt/buildNanoflowFromStmt give the builder a selfFlow (qualified name, kind, declared return type); microflowExists/nanoflowExists and lookup*ReturnType answer from it for a call of the same kind, and the loop and error-handler sub-builders carry it.", "insight": "A resolver that consults only the store cannot see the document the statement is creating; any statement that may reference itself (recursion) needs the statement's own name in scope. Test the control too: a missing non-self target is still refused, and a nanoflow does not resolve a microflow of its own name.", "issue": "ako/mxcli#843", "file": "mdl/executor/cmd_microflows_builder.go"} +{"date": "2026-09-30", "area": "mdl/executor", "symptom": "ako/mxcli#872 (rehearsal W3): a 'reset, then authoritative grants' section (`revoke all on entity E from R;` then the grants meant to hold) rewrote the domain model on every re-run under mdl 0 and mdl 1, re-minting the access rule's $ID and moving LastTransactionID although the rules it ended with were the ones it began with (CapTrack 02-security / 30-export: 2 files written on every second run).", "cause": "Each statement wrote the domain model on its own and was reconciled against disk at that moment: the revoke removed the rule, and the grant's write was reconciled against a unit that no longer had it, so canon.Reconcile had no stored rule to carry identity from. The elision is per write; the no-op here only exists across two writes.", "fix": "A run of consecutive entity grant/revoke statements in ExecuteProgram / ExecuteProgramContinueOnError is deferred (executor.accessRuleRun -> Backend.DeferUnitWrites / FlushDeferredWrites -> mpr.Writer): unit updates are held in memory and served through the reader overlay, which the unit listings (readMprContents, v1 listing) now honour too, then written once at the run's end through updateUnit, reconciled against the pre-run bytes. Any other statement, the end of the program, a failure and Disconnect/Close end the run.", "insight": "No-op elision answers 'is this write a no-op against disk now?', which is the wrong question for a sequence whose steps undo each other: two individually real writes can net to nothing, and the second can only keep the first's identities if it is compared with the state before the first. Measure a re-run with the transaction id as well as the unit bytes: the Studio Pro-authored control (PedApp Administration.Account's two User rules) is what showed the identity carry, since an mxcli-created rule's $ID is whatever the last run minted. The reader overlay existed for the import buffer but the listings bypassed it, so a held write was invisible to GetDomainModel; any deferral must be visible to every read path, not only GetRawUnitBytes.", "issue": "ako/mxcli#872", "file": "mdl/executor/access_rule_run.go, modelsdk/mpr/writer_deferred.go, modelsdk/mpr/reader_units.go, mdl/backend/modelsdk/backend.go"} +{"date": "2026-09-30", "area": "mdl/executor", "symptom": "ako/mxcli#874: `retrieve $L from M.Emp where [M.Emp.Name = 'y']` (and `where M.Emp.Name = 'y'`, and a page datasource `where [M.Emp.Name = 'y']`) stored ['Name' = 'y'] — a comparison of two string constants, so the retrieve silently returned every row or none; mxcli check and mx check both passed. An access rule and a workflow targeting XPath stored the qualified name verbatim (CE0161), and FormatXPathConstraint turned it into the literal once the constraint was long enough to re-lay out.", "cause": "Every XPath serializer (qualifiedNameToXPath and normalizeXPathEnumRefs in the executor, xpathExprToString in the visitor, which the formatter also uses) read any three-part name as Module.Enum.Value -> 'Value'. None of them knew the entity the constraint is evaluated on, so Module.Entity.Attribute took the enum reading.", "fix": "mdl/executor/xpath_member_names.go storedXPathConstraint(xpath, entity): outside string literals, Module.Entity.Attribute whose Module.Entity is the constrained entity or an entity step of the constraint's own path -> Attribute, then the remaining three-part names -> 'Value'. Wired into all four writers (retrieve via expressionToXPathNames, page datasource, access rule, workflow targeting on System.User / System.WorkflowGroup), into sameStoredConstraint (each side against its own retrieve's entity, so the re-run matches describe's `where Name = 'y'`), and into the page member validator (which otherwise refused the re-run with CE1613 on `M.Emp.Name`). The visitor serializer now writes qualified names as written, so the formatter no longer decides. Evidence: mx check 11.14.0 on a TestApp copy, microflow XpathConstraint patched: [Name = 'y'] 0 errors; [M.Emp.Name = 'y'] CE0161; [Kind = M.EmpKind.Staff] CE0161. After the fix: exec twice under mdl 0 and mdl 1 writes nothing on the re-run (no file under mprcontents newer than a marker), and the bare-name spelling of the same script is also Unchanged against it. Tests: mdl/executor/xpath_qualified_member_test.go; mdl/visitor/visitor_expr_quoting_test.go TestDatasourceWhere_KeepsThreePartNamesForTheWriter; validate_widget_member_refs_test.go qualified case", "insight": "A lexical rewrite of a name needs the scope the name is resolved in; the string-level enum pass had none, so it guessed. The scope here is syntactic (the constrained entity plus path steps), which keeps the writer and the create-or-modify comparison in agreement with no project open. Not covered: an attribute qualified with a generalization of the constrained entity keeps the enum reading.", "issue": "ako/mxcli#874", "file": "mdl/executor/xpath_member_names.go"} +{"date": "2026-09-30", "area": "mdl/executor", "symptom": "ako/mxcli#874 review: after the first fix, `grant read * on entity Administration.Account to Administration.User where [System.User.Name = 'x']` stored ['Name' = 'x'] and mx check reported 0 errors. On main the short constraint was stored verbatim and failed loudly with CE0161; the fix's enumeration reading of every unplaced three-part name turned it into a silent constant comparison. Workflow targeting `[Administration.Account.IsLocalUser = true]` had the same loud->silent change.", "cause": "storedXPathConstraint knows only the constrained entity and the path steps; an attribute qualified with a generalization, or with any other entity, falls through to normalizeXPathEnumRefs. For retrieve and page datasources that was already the behaviour, but the access-rule and workflow writers had stored such names verbatim.", "fix": "mdl/executor/xpath_member_names.go storedModelXPathConstraint(ctx, xpath, entity): owners are the entity's generalizationChain; a remaining three-part name whose prefix is an entity of the model stays as written, anything else becomes 'Value'. Used by accessRuleXPathConstraint. Workflow targeting (built without ctx) only resolves System.User / System.WorkflowGroup names via resolveXPathMemberNames and leaves the rest verbatim, as on main.", "test": "mdl/executor/xpath_qualified_member_test.go TestAccessRuleXPath_GeneralizationAndForeignEntityNames, TestWorkflowTargetingXPath_LeavesUnplacedNamesAsWritten; failed with ['Name' = 'x'] / ['IsLocalUser' = true] before the fix. TestApp copy: grant stored [Name = 'x'], re-run wrote nothing, mx check 0 errors.", "insight": "When a fix replaces a writer's verbatim output with a guess, check what the guess does to inputs that used to fail loudly: a guessed string literal is valid XPath, so the error disappears instead of the defect.", "file": "mdl/executor/xpath_member_names.go"} +{"date": "2026-10-01", "area": "mdl/executor", "symptom": "ako/mxcli#885 (rehearsal C1): `create or modify microflow|nanoflow` that splices a replacement `change` on a variable bound by a retrieve, a microflow call or a list operation wrote the member bare (`Name`, not `System.User.Name`); `exec` reported `Modified ... (spliced: 1 replaced)` and the project no longer loaded (mx check StorageLoadException: \"The text 'Name' is not a valid AttributeIdentifier\"). Same path: a spliced `find $L by Name = \u2026` on such a list became a FindByExpression `Name = 'y'`, an aggregate `by Attr` lost its attribute, a sort was refused by the AttributeRef write guard. Parameter-bound variables were fine. Both language versions.", "cause": "The splice builds each replacement with alterFlowContext.fragmentBuilder, seeded by storedVariables, which typed only parameters, declare and create outputs; every other stored output was 'Unknown'. With no entity, resolveMemberChange / the sort and aggregate builders fall back to the bare name. The full build types those variables as it goes (registerResultVariableType, retrieve/list-op/loop handlers), so splice and create wrote different members for the same statement. #863's describedMemberSpellings was not the cause: it only respells to describe's short form, which the full build qualifies.", "file": "mdl/executor/cmd_alter_flow.go, mdl/executor/cmd_flow_modify.go, mdl/executor/cmd_microflows_builder_actions.go, modelsdk/canon/attributeref.go", "fix": "mdl/executor/cmd_flow_modify.go planFlowModify hands the declared body's full-build varTypes to the fragment builder (alterFlowContext.declaredVarTypes, overlaid in fragmentBuilder), so the splice writes what create writes. storedVariables also types database retrieves (first -> object, else list) and microflow-call results (called flow's return type) for `alter microflow`. Guards: flowBuilder.qualifiedMembersOnly (set for every fragment) refuses a change/create/find/filter member, sort item or aggregate attribute it cannot qualify (refuseUnqualifiedAttribute) instead of writing it bare; modelsdk/canon/attributeref.go now also refuses a bare Microflows$ChangeActionItem.Attribute at the write choke point.", "test": "mdl/roundtrip/flow_splice_member_qualified_test.go TestFlowModify_SplicedMembersAreQualified (integration, PedApp: retrieve/call/list-op-bound change, sort/find/aggregate over a call-bound list, parameter control, loop-iterator case; both headers; parity of written members with a full build); mdl/executor/flow_splice_member_qualified_test.go TestSpliceFragment_*; modelsdk/canon/attributeref_test.go TestBareAttributeRefError_ChangeActionItem. Revert check: without the fix the roundtrip test reports bare `Name` for retrieve/call/list-op changes, FindByExpression, an empty aggregate attribute; without only the declared-type overlay the list-op-bound change is refused by the guard (mdl 1). mx check on the repro copy: StorageLoadException before, pristine PedApp's 1002 baseline errors and nothing else after.", "insight": "A splice must be built with the same knowledge as the full build of the same statement; the stored flow is a weaker source of variable types than the declared body. A green `Modified (spliced)` plus a passing `mxcli check` proves nothing about loadability - compare the splice's written members against a full build, and check every member-writing activity, not just the reported one."} diff --git a/.claude/skills/fix-issue/findings/mdl-other.jsonl b/.claude/skills/fix-issue/findings/mdl-other.jsonl index 50d0511d0..2414fe340 100644 --- a/.claude/skills/fix-issue/findings/mdl-other.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-other.jsonl @@ -75,3 +75,4 @@ {"area": "mdl/catalog", "date": "2026-09-25", "symptom": "java_actions.ReturnType showed a type-parameter return as the type parameter's own name: 'TypeParameter', 'TypeParEntity', 'FileTypeDocument' depending on what the modeler called it, and a type parameter named `String` (Studio Pro allows it) read back as 'String', identical to the primitive. java_action_parameters.ParameterType had the same ambiguity for both the object parameter and the entity-type selector.", "cause": "The catalog stored TypeString(), which is DESCRIBE's MDL rendering: a bare type-parameter reference IS its name in MDL syntax, so the value carried no marker that it was a type parameter at all.", "file": "mdl/catalog/builder_modules.go (catalogCodeActionType)", "insight": "The report reads like three inconsistent conventions ('TypeParameter' / 'TypeParEntity' / a name) but it is one: every value was the modeler's chosen name, and 'TypeParameter' is merely Studio Pro's default. Fix the encoding in the catalog only (`TypeParameter:`, `EntityTypeParameter:`, the `Kind:Name` shape microflows_data already uses; no primitive contains a colon) \u2014 changing TypeString() would change DESCRIBE output, where the bare name is the syntax. Test with the name colliding with a primitive and a primitive action as control: a unit test on the builder with MockBackend.ListJavaActionsFullFunc, plus an end-to-end catalog query on testdata/expr-checker (copy the whole dir; the .mpr alone is v2 without mprcontents/). MDL itself cannot declare a type parameter named String (`entity ` is a parse error), so the colliding case is only reachable via Studio Pro-authored models.", "refs": ["mendixlabs/mxcli#1183"]} {"area": "mdl/catalog", "date": "2026-09-26", "symptom": "`impact Module.Entity.Attr` answered \"(no impact - element is not referenced)\" for an attribute a change activity sets and a page displays (Evora Factory Management: DigitalTwin.Machine.NumberOfIncidents); `impact` on an enumeration said the same; `callers` of a workflow started by a microflow said \"(no callers found)\". An agent auditing the app read these as safe-to-delete.", "cause": "The refs graph stopped at documents: no ATTRIBUTE / ENUMERATION / ENUMERATION_VALUE targets at all, and no edge for WorkflowCallAction, for a page navigating an association, or for an import/export mapping mapping an entity. Page/snippet XPath constraints also had no TargetEntity, because resolveEntityRefFromBSON read EntityRef.QualifiedName, a key no stored DirectEntityRef carries (it is `Entity`; an IndirectEntityRef ends on its last step's DestinationEntity).", "file": "mdl/catalog/builder_member_refs.go (memberRefsInUnit, scanPaths, extractXPathRefs, extractEnumerationTypeRefs), builder_references.go (microflowActionRef WorkflowCallAction), builder_xpath.go (resolveEntityRefFromBSON), catalogdb.go (CatalogTx.Query)", "insight": "Member references are found by a RAW-document walk matching every string value against the names the model declares (whole-string = structured ref: MemberChange.Attribute, AttributeRef.Attribute, EntityRefStep.Association, EnumerationType.Enumeration, ObjectMappingElement.Entity; path tokens inside expressions = association paths and qualified enum values). A typed walk would reach only the sites someone wrote a case for; the raw walk reached 4043 attribute bindings on Evora with no per-type code, and exact-set matching means prose cannot produce an edge (Documentation is skipped anyway; the test's control is an unused attribute that must stay unreferenced). XPath is resolved separately because its context entity IS known: bare names resolve against the target entity and its generalizations, predicates after a path switch context, and an enum attribute compared to a literal names the value. What stays invisible is a bare member through a variable in an expression ($Order/Total) \u2014 so the executor must not say 'not referenced' (see the executor finding). New member kinds deliberately stay OUT of graphRefKinds and off graph_god_nodes' asset side: attributes are members, not assets, and reusing change/create/retrieve for them would have pulled every attribute into communities/centrality. Bump CatalogSchemaVersion for any new edge (refs are only written by REFRESH CATALOG FULL). Verified the lint output on Evora is byte-identical in counts before/after, so no rule changed verdicts silently."} {"area": "cmd/mxcli/syntax", "date": "2026-09-28", "symptom": "`mxcli syntax database-connection` documented `DROP DATABASE CONNECTION Module.Name;`, which was a parse error; `SHOW REFERENCES OF` in docs-site was never valid (the grammar is `REFERENCES TO`).", "cause": "The syntax reference and docs-site are hand-written and nothing parses their statements against the grammar; the drop was documented ahead of an implementation that never came.", "file": "cmd/mxcli/syntax/features_integration.go, mdl/grammar/MDLParser.g4, docs-site/src/tools/references-impact.md", "insight": "Implemented `drop database connection [if exists]` (and `drop validation rule`, `drop external entity`) and corrected the docs. `make check-skill-mdl` only checks fenced blocks it judges runnable; a documented statement inside prose or a Syntax template is not checked, so grep the syntax entries for each documented statement keyword when adding or removing a form. ako/mxcli#755.", "refs": ["#755"]} +{"area": "mdl/upgrade", "date": "2026-09-30", "symptom": "Re-running an upgraded script against a flow an older mxcli stored: `create or modify microflow Ledger.ACT_ApplyRules: this change cannot be spliced into the stored flow: the Loop at (2960, 200) changes inside its body` on every run (6 flows in mxcli-ledger), and outside a loop the re-run silently turned event handlers on (`ACT_MoveRuleUp/Down` spliced).", "cause": "mendixlabs/mxcli#895 changed what a bare `commit $X;` means (without events -> with events, Studio Pro's default) in every version, not behind the header. The stored flows hold `WithEvents=false`; the unchanged script now declares true, so diff-then-patch sees a change inside the loop body. fmt --upgrade had nothing to rewrite: the script alone cannot know which meaning is stored.", "file": "`mdl/visitor/visitor_flow_commits.go` (ast.Program.FlowCommits, with the insertion Fix), `mdl/upgrade/commit_events.go` (pinCommitEvents, matched by variable), `mdl/executor/flow_commit_events.go` (StoredCommitEvents, loop bodies included), `cmd/mxcli/cmd_fmt.go` (-p opens the project with or without --header), tests `mdl/upgrade/commit_events_test.go`, `mdl/roundtrip/upgrade_commit_events_test.go`", "insight": "A default change that is NOT version-gated still breaks the migration: every stored flow built before it disagrees with its own script. The upgrade is the one place that can reconcile it, and only with the project, so `-p` must be read even when no header is added. Match script commits to stored Commit activities by variable, not position: explicit flags consume their stored twin, the bare ones are pinned only when every remaining stored commit of the variable is without events and none are left over; anything else is a note, never a guess. Control in the test: the unpinned script is refused (mdl 1) / rewrites the flow (mdl 0) against the same setup.", "refs": ["ako/mxcli#873", "ako/mxcli#714", "mendixlabs/mxcli#895"]} diff --git a/.claude/skills/fix-issue/findings/mdl-visitor.jsonl b/.claude/skills/fix-issue/findings/mdl-visitor.jsonl index baa5e455f..0fa6a2dfd 100644 --- a/.claude/skills/fix-issue/findings/mdl-visitor.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-visitor.jsonl @@ -49,3 +49,4 @@ {"date": "2026-09-30", "area": "mdl/visitor", "symptom": "A headerless (mdl 0) script with `dynamicclasses: 'if $currentObject/X then ''on'' else '''''` (or DynamicCellClass, or an OData client's `HttpUsername: '''admin'''` / `HttpPassword: '@Mod.C'`) failed `check` and `exec` with MDL-WIDGET33 / MDL-ODATA07 after #750; mxcli-demo-2 (15 widgets) and CapTrackV6 (22) stopped running.", "cause": "#750 changed the meaning of a quoted value in an expression property (the expression's text -> a Mendix string) and added the refusal of the old spelling as an executor validator, ungated: the validators run on the AST and never consult the language version, so ADR-0011's 'new rejections only under the header' was not applied.", "fix": "langver.Change MDL-V1-QUOTEDEXPR in mdl/visitor/visitor_quoted_expression.go: under mdl 0 a lone string literal whose content is expression text (the WIDGET33 / ODATA07 tests, shared via visitor.IsLegacy*ExpressionText) builds its content, the old meaning, and notes the change with a rewrite to the bare content; under mdl 1 the literal reaches the validators unchanged and is refused. upgrade.versionNeutral applies the rewrite in plain `fmt --upgrade` too, since the bare form reads the same under both versions.", "insight": "A refusal added in the executor is invisible to the header gate, which lives in the visitor: when a change of meaning lands, decide per version in the visitor (languageVersionOf / Builder.gate) so the executor only ever sees the meaning of the script's own version. Audit hint: grep the change's commit for new SeverityError validators and ask whether an mdl 0 script used the construct before.", "issue": "ako/mxcli#836", "file": "mdl/visitor/visitor_quoted_expression.go, mdl/visitor/visitor_odata_expression.go, mdl/upgrade/gated.go, mdl/executor/validate_widget_expression_list.go"} {"date": "2026-09-30", "area": "mdl/visitor", "symptom": "After the first #836 fix, a headerless script still silently changed meaning for the old describe's quoting of a compound expression: header `'Authorization': '''Bearer '' + @M.Token'`, `HttpPassword: '@M.A + @M.B'`, `dynamicclasses: 'toLowerCase(@M.Theme)'` stored the TEXT as a string instead of the expression; mdl 1 did not refuse the OData ones either.", "cause": "The 'is this expression text' test anchored the constant reference to the whole content (`^@Mod.C$`) and the OData test required doubled quotes at BOTH ends; the old describe (formatExprValue) quoted every stored expression whole, so a compound expression is doubled at the start only, and one with no `$` / quote / `if` matched nothing.", "fix": "visitor.quotedConstantRefRe matches `@Mod.C` anywhere not preceded by a word char, dot or `@` (an e-mail user name and a Tailwind `@container` stay strings); MDL-ODATA07 gains a third case that asks visitor.IsLegacyODataExpressionText, so mdl 1 refuses what mdl 0 converts.", "insight": "When a heuristic decides which old spelling keeps its meaning, derive its inputs from what the OLD describe printed for Studio Pro-authored values, not only from hand-written examples: describe output is the largest body of mdl 0 scripts, and it quoted every expression whole.", "issue": "ako/mxcli#836", "file": "mdl/visitor/visitor_quoted_expression.go, mdl/executor/validate_odata_properties.go"} {"date": "2026-09-30", "area": "mdl/visitor", "symptom": "`fmt --upgrade --header` refused the header over `$Hit = find($Regions, $currentObject = $X)` when `$Regions` was assigned by `call microflow` (MDL-V1-LIST \"depends on a type the script does not state\"), although the called flow's return type settles it — in the same script (CapTrackV6 11-viewstate) or in the project (26-admin-goals)", "cause": "operandKind counted every `$x = call …` as an unknown definition: the visitor has no project, and never looked at the flows the script itself creates. The mdl 0 flow builder decides at exec time from `declaredVars[$x] == \"String\"`, which registerResultVariableType sets from the called flow's return type, looked up in the model as the earlier statements left it", "file": "mdl/visitor/visitor_upgrade_fixes.go (collectOperandDefs, operandReading, scriptFlowReturn), mdl/visitor/visitor_list_activities.go (both call sites: ExitListOperationStatement, setCallFix), mdl/ast/ast.go (LanguageNote.Operand / OperandChoice), mdl/upgrade/upgrade.go (Options.Flows, resolveOperand), mdl/executor/flow_return_types.go (FlowReturnTypes), cmd/mxcli/cmd_fmt.go (-p)", "fix": "Resolve in exec's order: a flow an earlier statement of the script creates (b.statements, the statements built so far) answers statically; otherwise the note carries both fixes as an OperandChoice and `fmt --upgrade -p` resolves it through executor.FlowReturnTypes, which calls the builder's own lookupMicroflowReturnType + registerResultVariableType. A drop/rename/move/`if not exists` of the callee earlier in the script, a callee the project lacks, and definitions that disagree stay refused", "insight": "Answer the upgrade's type question with the builder's resolver, not a copy (duplicate-resolver-drift). The execute-both control only discriminates in one direction: the builder turns a List operation over a declared String into the string function under EVERY version (addListOperationAction), so misreading a String as a list writes the same model; the dangerous misreading is a list read as a String (`set $x = find(…)` is always the string function under mdl 1). Choose the control from the direction that can hurt, or a green execute-both proves nothing. Verified on the CapTrackV6 rehearsal: 11-viewstate upgrades with no project, 26-admin-goals refuses without -p and upgrades with it, and its upgraded SUB_Admin_GoalScope execs as Unchanged on the project, same as the original", "issue": "ako/mxcli#860"} +{"area": "mdl/visitor", "date": "2026-09-30", "symptom": "`LastImport: date,` in a headerless script (mxcli-ledger 01-domain-model.mdl) -> `type `date` is not supported \u2014 Mendix has no date-only type; mxcli silently stored it as DateTime` from fmt, check and exec alike; `fmt --upgrade` could not parse the file, so the migration needed a hand edit although `date` -> `DateTime` is exactly what every earlier build stored.", "cause": "#706 refused `date` together with `float`/`currency` in every version. The two cases differ: float/currency stored a String (a wrong model, refusal right), `date` stored the DateTime the author could only have meant \u2014 a respelling, which ADR-0011 makes a deprecated alias, not an error.", "file": "`mdl/visitor/visitor_silent_drops.go` (recordDateType), `mdl/visitor/visitor_helpers.go` + `visitor_microflow.go` (DATE_TYPE builds TypeDateTime), `mdl/grammar/domains/MDLDomainModel.g4` (`@alias MDL-DEPR160` on dataType and nonListDataType), `mdl/deprecation/deprecation.go` (DateType, RemovedIn 1), tests `mdl/visitor/visitor_date_alias_test.go`, `mdl/upgrade/date_alias_test.go`", "insight": "Sort a #706-style 'silently stored as X' refusal by what X was: if X is what the author meant (date -> DateTime), the form is an alias and belongs in the registry (warn, fmt rewrite, refuse under mdl 1); only when X is wrong (float -> String, dropped association options) is a refusal in every version right. For the alias to be provable, the builder must produce the canonical AST (TypeDateTime, not TypeDate), or the registry's Example/CanonicalExample same-AST test fails. Upgrading the ledger's pre-migration 01 now reproduces its hand-finished committed file byte for byte.", "refs": ["ako/mxcli#714", "ako/mxcli#706"]} diff --git a/CHANGELOG.md b/CHANGELOG.md index f4435502e..265b69b78 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,6 +48,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **`create or modify` of a flow can change an activity's notes, and matches an explicit `on error rollback`** (ako/mxcli#859, part) — a statement that added, reworded or took off an `@annotation` on an activity (or dropped an annotated activity) was refused under the `mdl 1;` header ("the annotations on the replaced … change"; "… carries an annotation, which would be left behind") on every run, and rebuilt the whole flow without it. The stored notes of that activity are now replaced by the ones the statement states; a note shared with another activity (one `describe` gives an `id:`) is still refused. This is what made the Studio Pro-authored PedApp nanoflows `ACT_Feedback_TriggerScreenshotMode` and `ACT_Feedback_UploadImage` unrunnable when their plain description was put under `mdl 1;`: a note's `\r\n` is two characters under mdl 1, so it is a real change, now written once. Separately, `commit $E on error rollback` (any activity with a bare `on error rollback`, which `describe` never prints because it is the stored default) did not match its own stored activity, so it was dropped and written again whenever anything else in the flow changed. +- **`describe microflow` prints a Show Page action's title override** (ako/mxcli#869) — `show page M.P with title = '…'` used to describe without its override, so describe → exec dropped it, and taking the override out of a `create or modify microflow` reported "Unchanged" and wrote nothing, under mdl 0 and mdl 1. +- **Re-running a `call web service` statement unchanged writes nothing** (ako/mxcli#861) — the call's activity was re-spliced on every run, because its stored raw document holds the element IDs each build mints and was compared byte for byte; it is now compared with those IDs set aside. A stored call that lacks a key mxcli's structured form writes (such as `ErrorHandlingType`), or that refers to its service by ID, now describes as `call web service raw '…'` instead of a structured form that would write a different document. +- **`date` as a type is a deprecated alias of `DateTime` again, and `fmt --upgrade` rewrites it** (ako/mxcli#714 rehearsal U1, ako/mxcli#706) — Mendix has no date-only type, and mxcli always stored `date` as a DateTime; #706 made it an error in every script, which stopped headerless scripts that ran (mxcli-ledger's domain model) and left `fmt --upgrade` unable to parse them. Without the `mdl 1;` header `date` (on an attribute, a parameter, a return type, a `declare`) now builds exactly what `DateTime` builds and warns **MDL-DEPR160**; `fmt --upgrade` writes `DateTime`; under `mdl 1;` it is refused. `float` and `currency` stay refused, and so does the parenthesised association form: their old reading stored something else. +- **`fmt --upgrade -p app.mpr` keeps a stored commit without events** (ako/mxcli#873) — since mendixlabs/mxcli#895 a bare `commit $X;` means with events (Studio Pro's default, MDL067); a flow an older mxcli stored from the same script commits without events, so re-running the script turned its event handlers on, and inside a loop the `mdl 1` splice refused it on every run. Where the stored `create or modify` flow commits the variable without events, the upgrade now writes `commit $X without events;` (before any `refresh`), with or without `--header`; a new flow or commit, or one stored with events, is left as written. A variable the stored flow commits both ways is left and reported. Without `-p`, `fmt --upgrade` prints a note per flow with a bare commit. Measured on mxcli-ledger's pre-migration model: 12 commits in 3 scripts pinned; the rehearsal's six refusals on each pass are gone, two flows whose handlers the re-run silently turned on (`ACT_MoveRuleUp`/`Down`) keep what is stored, the second pass writes nothing, and `mx check` reports 0 errors. +- **A "reset, then grant" security section re-run writes nothing** (ako/mxcli#872, rehearsal W3). A script that runs `revoke all on entity E from R;` and then the grants that should hold used to rewrite the domain model on every run, under both language versions. Each run minted the access rule again with a new `$ID` and moved the project's transaction id, even though the rules at the end were the rules at the start. Now a run of consecutive entity `grant` / `revoke` statements is written once, when the run ends, and is reconciled against what was stored before it. A run that ends where it started writes nothing. A run that changes a rule keeps the stored identity of every rule it grants back, including rules Studio Pro authored (PedApp's `Administration.Account`). Measured on a CapTrack copy: `02-security` and `30-export` wrote 2 files each on their second run with the previous build and write none now, and `mx check` still reports 0 errors. - **Running a `create or modify microflow|nanoflow` script a second time writes nothing, under `mdl 1` as under mdl 0** (ako/mxcli#859, part) — under the `mdl 1;` header the identical second run of a flow mxcli had just created was refused ("a return is added where the stored flow does not end a path", "the stored statement \"Join\" has no activity to address", "the Return at (x, y) is taken out", "the fragment returns"), and without the header it was silently rebuilt with MDL-V1-REBUILD. The statement was compared with `describe`'s rendering of the stored flow, which spells a guard clause directly before the final return as an if/else, a nested guard with a `join`/`merge` pair, and a wrapped row as crossed branches. The declared flow is now built and compared with the stored graph itself (read back through the codec, element IDs aside), so an unchanged statement is unchanged however its flow is described. The comparison is made only when the flow reads back as it was written, so a change the reader would not see — a `show page … with title` override, now read — is still written. For an edited flow, a guard clause and its if/else form now match, and a change before a guard and one inside its then-branch are spliced separately instead of as one fragment that returns. Measured on CapTrackV6's upgraded scripts: the second pass refused 36 statements in 12 scripts before and 4 in 4 after, none of them control flow (a description that does not parse, #843's recursive stub, a source-language translation). - **`create constant … private` parses again without the header, and `fmt --upgrade` removes it** (ako/mxcli#865) — up to v0.24.0 the trailing `private` parsed only because it began a help statement of its own, which built nothing: the modifier was never stored, and the database-connections skill taught it for credentials. R7 closed that catch-all and made it a parse error, which `fmt --upgrade` could not get past. Without the `mdl 1;` header it is now a no-op that warns **MDL-DEPR138** (it was never stored; keep a secret out of the model with `mxcli constant set`), in the clause form and after a property list, in any letter case; `fmt --upgrade` deletes it; under `mdl 1;` it is refused. `private` is not reserved and still works as a name. diff --git a/Makefile b/Makefile index f26e9b130..22fca2754 100644 --- a/Makefile +++ b/Makefile @@ -354,8 +354,10 @@ INTEGRATION_PKGS = $(shell grep -rl --include='*_test.go' --exclude-dir='.?*' -- INTEGRATION_SPLIT = ./mdl/executor ./mdl/roundtrip UPGRADE_PROPERTY = ^TestUpgradeExecutesToTheSameModel$$ # The TestApp splice-parity property (#839) runs describe → exec on every -# TestApp flow under both language versions (~7 min): its own CI suite. -SPLICE_PARITY = ^TestTestAppFlowSpliceParity$$ +# TestApp flow under both language versions (~7 min): its own CI suite. The +# TestSpliceRerun_ tests (#859) run there too, to keep the roundtrip suite +# under its time limit (#870). +SPLICE_PARITY = ^(TestTestAppFlowSpliceParity|TestSpliceRerun_.*)$$ INTEGRATION_GO_TEST = CGO_ENABLED=0 go test -tags integration -count=1 test-integration-executor: diff --git a/cmd/mxcli/cmd_fmt.go b/cmd/mxcli/cmd_fmt.go index b710c9dd7..fec791943 100644 --- a/cmd/mxcli/cmd_fmt.go +++ b/cmd/mxcli/cmd_fmt.go @@ -66,6 +66,14 @@ Upgrading (--upgrade): before the call is looked up in the project. Without a project such a call blocks the header and fmt says so. The project is only read. + The project also settles a bare commit (with or without --header): since + #895 "commit $X;" means WITH events, and an older mxcli stored the same + statement without events. In a "create or modify" flow whose stored flow + commits the variable without events, --upgrade -p writes + "commit $X without events;" so that re-running the script keeps what is + stored. Without -p the script is left as written and fmt prints a note + (MDL067) for each flow with a bare commit. + A test file (.test.mdl, .test.md) is upgraded the way check reads it: the statements in its blocks are rewritten, and its doc comments (@test, @expect, …), separators and prose are kept byte for byte. It takes no @@ -160,14 +168,11 @@ Upgrading (--upgrade): if cmd.Flags().Changed("header") { opts.AddHeader = addHeader } - if opts.AddHeader { - flows, closeProject, err := openUpgradeProject(cmd) - if err != nil { - return err - } - defer closeProject() - opts.Flows = flows + closeProject, err := openUpgradeProject(cmd, &opts) + if err != nil { + return err } + defer closeProject() res, err := upgrade.Upgrade(string(data), opts) if err != nil { return fmt.Errorf("%s: %w", label, err) @@ -183,18 +188,24 @@ Upgrading (--upgrade): } // openUpgradeProject opens the -p project read-only for the upgrade to read -// flow return types from (ako/mxcli#860). With no project it returns nil, and -// the constructs that need one block the header as before. -func openUpgradeProject(cmd *cobra.Command) (upgrade.FlowTypes, func(), error) { +// from: the flow return types a header-gated `find(…)` depends on +// (ako/mxcli#860), and the stored commit flags a bare `commit` is pinned to +// (ako/mxcli#873). With no project it sets neither, and the constructs that +// need one block the header or are reported, as before. +func openUpgradeProject(cmd *cobra.Command, opts *upgrade.Options) (func(), error) { projectPath, _ := cmd.Flags().GetString("project") if projectPath == "" { - return nil, func() {}, nil + return func() {}, nil } b := modelsdkbackend.New() if err := b.ConnectReadOnly(projectPath); err != nil { - return nil, nil, fmt.Errorf("cannot read the project %s, which --upgrade reads flow return types from: %w", projectPath, err) + return nil, fmt.Errorf("cannot read the project %s, which --upgrade reads flows from: %w", projectPath, err) + } + if opts.AddHeader { + opts.Flows = executor.NewFlowReturnTypes(b) } - return executor.NewFlowReturnTypes(b), func() { _ = b.Disconnect() }, nil + opts.Commits = executor.NewStoredCommitEvents(b) + return func() { _ = b.Disconnect() }, nil } // writeFmtResult writes fmt's output: in place with -w, else to stdout. An @@ -243,6 +254,13 @@ func reportUpgrade(w io.Writer, label string, res upgrade.Result) { if res.HeaderAdded { fmt.Fprintf(w, "%s: added the language header\n", label) } + if res.CommitsPinned > 0 { + fmt.Fprintf(w, "%s: stated `without events` on %d bare commit(s), as the project's stored flows have them (MDL067)\n", + label, res.CommitsPinned) + } + for _, n := range res.Notes { + fmt.Fprintf(w, "%s:%d: note: %s\n", label, n.Line, n.Message) + } for _, d := range res.Unrewritten { msg := d.Code if e, ok := deprecation.Lookup(d.Code); ok { diff --git a/cmd/mxcli/cmd_fmt_upgrade_test.go b/cmd/mxcli/cmd_fmt_upgrade_test.go index c477b5372..59e0342fd 100644 --- a/cmd/mxcli/cmd_fmt_upgrade_test.go +++ b/cmd/mxcli/cmd_fmt_upgrade_test.go @@ -193,3 +193,26 @@ func copyTree(src, dst string) error { return os.WriteFile(target, data, 0o644) }) } + +// ako/mxcli#873: without a project, a bare commit in a `create or modify` +// flow is left as written and reported — its meaning changed with #895, and +// only the stored flow says which one the script should keep. +func TestFmtUpgrade_BareCommitWithoutProjectIsANote(t *testing.T) { + path := filepath.Join(t.TempDir(), "s.mdl") + src := "create or modify microflow M.F ($A: M.E)\nbegin\n commit $A;\nend;\n" + if err := os.WriteFile(path, []byte(src), 0o644); err != nil { + t.Fatal(err) + } + out, err := runFmt(t, "--upgrade", "-w", path) + if err != nil { + t.Fatal(err) + } + if got, _ := os.ReadFile(path); string(got) != src { + t.Fatalf("the file changed without a project:\n%s", got) + } + for _, want := range []string{"s.mdl:3: note: M.F:", "MDL067", "-p app.mpr"} { + if !strings.Contains(out, want) { + t.Errorf("output does not mention %q:\n%s", want, out) + } + } +} diff --git a/cmd/mxcli/syntax/features_domain_model.go b/cmd/mxcli/syntax/features_domain_model.go index c64b08f4a..084856256 100644 --- a/cmd/mxcli/syntax/features_domain_model.go +++ b/cmd/mxcli/syntax/features_domain_model.go @@ -458,7 +458,7 @@ func init() { "datetime", "autonumber", "binary", "hashedstring", "enumeration type", "currency", "float", }, - Syntax: "String(n) Variable-length text up to n characters\nInteger Whole number (-2B to 2B)\nLong Large whole number\nDecimal Precise decimal for currency/calculations\nBoolean True or false\nDateTime Date and time combined (there is no date-only type)\nAutoNumber Auto-incrementing integer\nBinary Binary data (files, images)\nHashedString Securely hashed string (passwords)\nEnumeration(Name) Reference to an enumeration\nAutoOwner System.owner (auto-set on create)\nAutoChangedBy System.changedBy (auto-set on commit)\nAutoCreatedDate DateTime (auto-set on create)\nAutoChangedDate DateTime (auto-set on commit)\n\nNot types: Date (use DateTime), Float and Currency (use Decimal) - refused.", + Syntax: "String(n) Variable-length text up to n characters\nInteger Whole number (-2B to 2B)\nLong Large whole number\nDecimal Precise decimal for currency/calculations\nBoolean True or false\nDateTime Date and time combined (there is no date-only type)\nAutoNumber Auto-incrementing integer\nBinary Binary data (files, images)\nHashedString Securely hashed string (passwords)\nEnumeration(Name) Reference to an enumeration\nAutoOwner System.owner (auto-set on create)\nAutoChangedBy System.changedBy (auto-set on commit)\nAutoCreatedDate DateTime (auto-set on create)\nAutoChangedDate DateTime (auto-set on commit)\n\nNot types: Float and Currency (use Decimal) - refused. Date (use DateTime) - refused under mdl 1; without the header a deprecated alias of DateTime (MDL-DEPR160).", Example: "CREATE PERSISTENT ENTITY MyModule.Customer (\n Name: String(100) NOT NULL,\n Age: Integer,\n Balance: Decimal,\n IsActive: Boolean DEFAULT true,\n CreatedAt: DateTime,\n Status: Enumeration(MyModule.Status)\n);", SeeAlso: []string{"domain-model.entity.attributes"}, }) diff --git a/docs-site/src/internals/idempotent-writes.md b/docs-site/src/internals/idempotent-writes.md index f6ddc30ab..0e0ea9ff3 100644 --- a/docs-site/src/internals/idempotent-writes.md +++ b/docs-site/src/internals/idempotent-writes.md @@ -74,6 +74,26 @@ write choke point of **both** the default `modelsdk` engine and the `legacy` engine. Which engine ran is an `--engine` flag, and it must not be visible in your diff. +## A run of grants is compared as a whole + +Most statements write the document they change, and that write is compared with +what is on disk at that moment. A sequence of entity `grant` / `revoke` +statements works differently. The run is held in memory and written once, when +the next statement of another kind starts or the script ends, and the write is +compared with what was stored before the run. This matters for a "reset, then +grant" section: + +```sql +revoke all on entity Shop.Order from Shop.User; +grant read *, write * on entity Shop.Order to Shop.User; +``` + +Written one statement at a time, the grant would be compared with a domain model +the revoke had already emptied. It would get a new access rule every run. Judged +as one run, a reset that grants back what was there writes nothing, and a reset +that changes a rule keeps the identity of every rule it grants back (#872). +Statements inside the run read the run's own writes. + ## Turning it off ```bash diff --git a/docs-site/src/language/basics.md b/docs-site/src/language/basics.md index 780f2886b..8a634560f 100644 --- a/docs-site/src/language/basics.md +++ b/docs-site/src/language/basics.md @@ -152,6 +152,8 @@ mxcli fmt --upgrade --header -w script.mdl # also add `mdl 1;` A construct with no mechanical rewrite is reported with the reason, and `fmt` refuses to add the header rather than change the script's meaning: an unknown or mis-shaped property (`MDL-V1-PROP`, `MDL-V1-PROPVALUE`), `create or replace view entity` (`MDL-V1-REPLACE01`), a session command in a script (`MDL-V1-SESSION`: move it to the command line or the REPL), a nested list operation such as `count(filter(…))`, `find`/`contains` on a variable whose type the script does not state (when the variable holds a microflow or nanoflow call's result, the called flow's return type decides: a flow the script creates earlier is read from the script, and any other from the project given with `-p app.mpr`, so `fmt --upgrade --header -p app.mpr` rewrites it), and an escaped line break (`\n`) inside an expression. An escaped line break in a text template's literal is rewritten: the break is written into the literal, which under `mdl 1` is still the template text. While `mdl 1` is a preview, the header is added only when asked. Running `fmt --upgrade` on its own output changes nothing. +`-p app.mpr` also settles a bare `commit $X;` in a `create or modify microflow|nanoflow`, with or without `--header`. Since mendixlabs/mxcli#895 a bare commit means *with* events, Studio Pro's default; an older mxcli stored the same statement *without* events. Where the stored flow commits the variable without events, `fmt --upgrade -p app.mpr` writes `commit $X without events;`, so re-running the script keeps what is stored instead of turning the event handlers on (or, inside a loop under `mdl 1`, being refused). A new flow, a new commit, or a stored commit with events is left as written; where the stored flow commits the variable both ways, the statement is left and reported. Without `-p`, `fmt` prints a note (`MDL067`) for each flow with a bare commit. + The design is in [ADR-0011](https://github.com/mendixlabs/mxcli/blob/main/docs/13-decisions/0011-mdl-language-versioning.md); `mxcli syntax language-header` has the details. ## Re-runnable Creates: `or modify` and `if not exists` diff --git a/docs-site/src/language/primitive-types.md b/docs-site/src/language/primitive-types.md index 38a19ded2..1e90a31d4 100644 --- a/docs-site/src/language/primitive-types.md +++ b/docs-site/src/language/primitive-types.md @@ -107,9 +107,14 @@ DateTime values include both date and time components. To show only the date, fo Mendix has no date-only attribute type: a date is a `DateTime`, and showing only its date part is a formatting choice on the widget. `Float` and `Currency` were -removed in Mendix 7 in favour of `Decimal`. MDL refuses all three with the type to -write instead — earlier versions accepted them and silently stored a `DateTime` -(for `date`) or a `String` (for `float` and `currency`). +removed in Mendix 7 in favour of `Decimal`. MDL refuses `float` and `currency` with +the type to write instead — earlier versions accepted them and silently stored a +`String`. + +`date` was always stored as a `DateTime`, so in a script without the `mdl 1;` +header it is a deprecated spelling of `DateTime`: it builds a `DateTime` and warns +`MDL-DEPR160`, and `mxcli fmt --upgrade` rewrites it to `DateTime`. Under `mdl 1;` +it is an error. ## AutoNumber diff --git a/docs-site/src/reference/microflow/create-microflow.md b/docs-site/src/reference/microflow/create-microflow.md index dc59012fa..83df45c45 100644 --- a/docs-site/src/reference/microflow/create-microflow.md +++ b/docs-site/src/reference/microflow/create-microflow.md @@ -83,6 +83,8 @@ $Result = CALL JAVA ACTION Module.Name ( Param = value ); Call another microflow, nanoflow, or Java action. Parameters are passed by name. The result can be assigned to a variable when the callee has a return type. If no return value is needed, omit the `$Result =` prefix. +A flow may call itself: the target resolves to the flow the statement creates, with the return type it declares, so a recursive microflow or nanoflow is written in one statement — no stub created first. + **UI Actions** ```sql diff --git a/docs-wiki/bug-patterns/scripts-that-cannot-rerun.md b/docs-wiki/bug-patterns/scripts-that-cannot-rerun.md index 4bf47680a..c1a02b282 100644 --- a/docs-wiki/bug-patterns/scripts-that-cannot-rerun.md +++ b/docs-wiki/bug-patterns/scripts-that-cannot-rerun.md @@ -92,6 +92,28 @@ compares equal on both sides, so an edit to it was reported Unchanged (a lossless — written again, it must be the document first written — and falls back to the statement diff when it is not. +**The statement diff still decides every flow that does change**, and there +two more ways to be unrunnable showed up (#859). A default describe omits — +`on error rollback` — has to be read as absent on the declared side too, or the +activity is re-spliced with every sibling edit; the built-graph match hides that +on an identical re-run, so the probe is a sibling edit and the splice summary. +And a refusal of a *legitimate* change is, to someone re-running a script, +indistinguishable from non-idempotence: re-annotating an activity was refused on +every run until the splice could replace an activity's notes. The header-only +upgrade (a mdl 0 description under `mdl 1;`) is where both surface on Studio +Pro content, because a backslash escape in a note is a real change under mdl 1. + +**Some re-runs only reach a no-op across two statements.** `revoke all` followed +by the grant that should hold ends with the rules it started with, but each +statement is a real write of its own. The grant's write was compared with a unit +the revoke had already emptied, so it had no stored rule to carry an identity +from, and the rule was minted again on every run (#872). Making each statement +more idempotent does not help here. What fixed it was judging the run by where +it ends: the writes are held, and they are reconciled once against the state +from before the run. Read the transaction id as well as the unit bytes to see +that a run wrote nothing. Check the identity carry on a Studio Pro-authored rule, +because an mxcli-created one has whatever `$ID` the previous run minted. + ## See also - [fix-issue findings](../../.claude/skills/fix-issue/findings/) — the guards, the diff --git a/mdl/ast/ast.go b/mdl/ast/ast.go index 0c1f0578e..ef0a5335d 100644 --- a/mdl/ast/ast.go +++ b/mdl/ast/ast.go @@ -74,6 +74,27 @@ type Program struct { // LanguageNotes are the constructs kept at their older meaning because of // LanguageVersion, one per occurrence, for check and exec to warn on. LanguageNotes []LanguageNote + // FlowCommits are the `commit` statements written in the body of a + // `create or modify microflow|nanoflow`, in source order, for + // `fmt --upgrade -p` to pin a bare one to what the stored flow holds + // (ako/mxcli#873). A bare `commit $X;` means WITH events since #895; a + // flow an older mxcli stored from the same script has it without. + FlowCommits []FlowCommit +} + +// FlowCommit is one `commit $X` in a `create or modify` flow. +type FlowCommit struct { + Line int // 1-based source line of the `commit` + Flow QualifiedName // the flow being created or modified + Nanoflow bool + Variable string // committed variable, without the `$` + // Bare is set when neither `with events` nor `without events` is written. + // Otherwise WithoutEvents says which. + Bare bool + WithoutEvents bool + // PinWithoutEvents inserts ` without events` after the variable, in the + // letter case of the `commit` keyword. Set only on a bare commit. + PinWithoutEvents *Fix } // DeprecatedSpelling is one use of a deprecated spelling in the source. diff --git a/mdl/ast/ast_alter_flow.go b/mdl/ast/ast_alter_flow.go index b6534dbd4..69dd36463 100644 --- a/mdl/ast/ast_alter_flow.go +++ b/mdl/ast/ast_alter_flow.go @@ -51,4 +51,10 @@ type AlterFlowOperation struct { Target string // Body is the fragment, for insert and replace. Body []MicroflowStatement + // ReplaceNotes, on a replace or drop, says the notes attached to the + // target go with it instead of being kept on the replacement (or left + // behind): Body carries the notes the statement states. Set only by + // `create or modify`, whose declared statements state every activity's + // notes (ako/mxcli#859); an `alter` replace keeps them. + ReplaceNotes bool } diff --git a/mdl/backend/mcp/microflow_mutator.go b/mdl/backend/mcp/microflow_mutator.go index 6d9e98356..0d4c46649 100644 --- a/mdl/backend/mcp/microflow_mutator.go +++ b/mdl/backend/mcp/microflow_mutator.go @@ -79,6 +79,10 @@ func (m *mcpFlowMutator) Drop(model.ID) error { return fmt.Errorf("drop is not supported by the MCP backend yet: Studio Pro does not roll back a removal when an update fails; run without --mcp to alter %s in the .mpr", m.qn) } +func (m *mcpFlowMutator) RemoveNotes(model.ID) error { + return fmt.Errorf("removing notes is not supported by the MCP backend yet: Studio Pro does not roll back a removal when an update fails; run without --mcp to alter %s in the .mpr", m.qn) +} + // SetReturnValue is refused like a replace: the patch Save sends carries // positions, pointers and added or removed elements only, so a changed value // would not reach Studio Pro and the edit would be lost without a word. diff --git a/mdl/backend/mfmutator/header.go b/mdl/backend/mfmutator/header.go index fb234c48c..89d94ecfa 100644 --- a/mdl/backend/mfmutator/header.go +++ b/mdl/backend/mfmutator/header.go @@ -77,8 +77,13 @@ func (m *Mutator) SetHeader(declared any) ([]string, error) { if isZero(nv) { continue } - return nil, fmt.Errorf("the stored document has no %s property, so the value the statement gives it cannot be "+ - "patched in; set it in Studio Pro", key) + if !m.declares(key) { + return nil, fmt.Errorf("the stored document has no %s property, so the value the statement gives it cannot be "+ + "patched in; set it in Studio Pro", key) + } + m.doc = insertAfterPredecessor(m.doc, d, key, nv) + changed = append(changed, strings.Replace(key, "Concurreny", "Concurrency", 1)) + continue } carried, same, err := carryValue(key, nv, ov) if err != nil { @@ -98,6 +103,55 @@ func (m *Mutator) SetHeader(declared any) ([]string, error) { return append(changed, params...), nil } +// PropertyDeclarer is implemented by a Deps that can tell whether the +// project's metamodel declares a document property. SetHeader adds a stated +// header property the stored document lacks only when it does: the key is +// then simply one the writer of the stored document left out — mxcli's own +// nanoflow writer omits ReturnVariableName when the statement has no +// `as $Var`, and so did older builds (#843) — while a key the project's +// version does not declare makes the document unopenable, so without an +// answer the absence is refused as before. +type PropertyDeclarer interface { + DeclaresProperty(docType, key string) bool +} + +func (m *Mutator) declares(key string) bool { + pd, ok := m.deps.(PropertyDeclarer) + return ok && pd.DeclaresProperty(dString(m.doc, "$Type"), key) +} + +// insertAfterPredecessor adds key to stored after the nearest property that +// precedes it in the declared encoding and is stored too, so the key lands +// where the writer puts it; with none, it is appended. +func insertAfterPredecessor(stored, declared bson.D, key string, v any) bson.D { + at := len(stored) + for i := range declared { + if declared[i].Key != key { + continue + } + for j := i - 1; j >= 0; j-- { + if k := indexOf(stored, declared[j].Key); k >= 0 { + at = k + 1 + break + } + } + break + } + out := make(bson.D, 0, len(stored)+1) + out = append(out, stored[:at]...) + out = append(out, bson.E{Key: key, Value: v}) + return append(out, stored[at:]...) +} + +func indexOf(d bson.D, key string) int { + for i := range d { + if d[i].Key == key { + return i + } + } + return -1 +} + // carryValue prepares a declared property value for the stored document: the // languages of a text the statement could not state are carried from the // stored one, and every element that corresponds to a stored one keeps the diff --git a/mdl/backend/mfmutator/header_test.go b/mdl/backend/mfmutator/header_test.go index e1fc91b05..9826a441b 100644 --- a/mdl/backend/mfmutator/header_test.go +++ b/mdl/backend/mfmutator/header_test.go @@ -272,3 +272,103 @@ func TestSplice_PlacedFragmentStaysWhereStated(t *testing.T) { } } } + +// declaringDeps answers DeclaresProperty for the keys it lists. +type declaringDeps struct { + fakeDeps + declared map[string]bool +} + +func (d *declaringDeps) DeclaresProperty(docType, key string) bool { + return d.declared[docType+"."+key] +} + +// asNanoflow turns a header fixture into a nanoflow, and optionally states a +// return variable as the encoder writes it: after MicroflowReturnType. +func asNanoflow(d bson.D, returnVar string) bson.D { + var out bson.D + for _, e := range d { + if e.Key == "$Type" { + e.Value = "Microflows$Nanoflow" + } + out = append(out, e) + if e.Key == "MicroflowReturnType" && returnVar != "" { + out = append(out, bson.E{Key: "ReturnVariableName", Value: returnVar}) + } + } + return out +} + +func mutatorWith(t *testing.T, doc bson.D, deps Deps) *Mutator { + t.Helper() + raw, _ := bson.Marshal(doc) + var d bson.D + if err := bson.Unmarshal(raw, &d); err != nil { + t.Fatal(err) + } + m, err := New(d, "unit", deps) + if err != nil { + t.Fatal(err) + } + return m +} + +// #843 (rehearsal M2): a nanoflow stored without ReturnVariableName — mxcli's +// own writer omits it when the statement has no `as $Var` — takes the one a +// later statement states, when the project's metamodel declares the property. +// The refusal ("set it in Studio Pro") made the mdl 1 script un-re-runnable. +func TestSetHeader_AddsAStatedPropertyTheProjectDeclares(t *testing.T) { + stored := asNanoflow(headerUnit(param("A", "DataTypes$BooleanType", 200)), "") + declared := asNanoflow(declaredDoc("Hidden", "DataTypes$VoidType", freshParam("A", "DataTypes$BooleanType", 200)), "Done") + deps := &declaringDeps{declared: map[string]bool{"Microflows$Nanoflow.ReturnVariableName": true}} + m := mutatorWith(t, stored, deps) + + changed, err := m.SetHeader(declared) + if err != nil { + t.Fatalf("the stated return variable is refused: %v", err) + } + if got := strings.Join(changed, ","); got != "ReturnVariableName" { + t.Fatalf("changed %q, want ReturnVariableName only", got) + } + if v := dString(m.doc, "ReturnVariableName"); v != "Done" { + t.Fatalf("ReturnVariableName = %q", v) + } + if i, j := indexOf(m.doc, "MicroflowReturnType"), indexOf(m.doc, "ReturnVariableName"); j != i+1 { + t.Errorf("ReturnVariableName stored at %d, want after MicroflowReturnType (%d), where the writer puts it", j, i) + } + if v, _ := dGet(m.doc, "MarkAsUsed").(bool); !v { + t.Error("MarkAsUsed, which no statement states, was overwritten") + } + out, err := m.Bytes() + if err != nil { + t.Fatalf("integrity: %v", err) + } + + // The second run finds it stored and writes nothing. + var again bson.D + if err := bson.Unmarshal(out, &again); err != nil { + t.Fatal(err) + } + m2 := mutatorWith(t, again, deps) + changed, err = m2.SetHeader(declared) + if err != nil || len(changed) != 0 { + t.Fatalf("second run: changed %v, err %v; want nothing", changed, err) + } +} + +// The control: where the project's metamodel is not known to declare the +// property, its absence is still refused — a key the version does not have +// makes the document unopenable. +func TestSetHeader_RefusesAPropertyTheProjectDoesNotDeclare(t *testing.T) { + stored := asNanoflow(headerUnit(param("A", "DataTypes$BooleanType", 200)), "") + declared := asNanoflow(declaredDoc("Hidden", "DataTypes$VoidType", freshParam("A", "DataTypes$BooleanType", 200)), "Done") + for name, deps := range map[string]Deps{ + "no answer": &fakeDeps{}, + "not declared": &declaringDeps{declared: map[string]bool{}}, + } { + m := mutatorWith(t, stored, deps) + if _, err := m.SetHeader(declared); err == nil || !strings.Contains(err.Error(), "has no ReturnVariableName") { + t.Errorf("%s: err = %v, want the refusal", name, err) + } + } +} diff --git a/mdl/backend/mfmutator/splice.go b/mdl/backend/mfmutator/splice.go index cc7902885..1e50c877f 100644 --- a/mdl/backend/mfmutator/splice.go +++ b/mdl/backend/mfmutator/splice.go @@ -444,6 +444,47 @@ func (m *Mutator) Replace(target model.ID, frag *backend.MicroflowFragment) erro return m.removeObject(x) } +// RemoveNotes takes out the annotations attached to target, with their +// lines. A note also attached to another object is refused: it would be taken +// from that object too. +func (m *Mutator) RemoveNotes(target model.ID) error { + g := m.graph() + x, err := g.node(target) + if err != nil { + return err + } + drop, seen := map[string]bool{}, map[string]bool{} + var notes []*node + for _, af := range g.annotationFlows(x.id) { + other := af.origin + if other == x.id { + other = af.dest + } + n := g.nodes[other] + if n == nil || n.typ != "Microflows$Annotation" { + return fmt.Errorf("the annotation line %s of %s does not lead to an annotation", af.id, describeNode(x)) + } + for _, f := range g.annotationFlows(n.id) { + if f.origin != x.id && f.dest != x.id { + return fmt.Errorf("a note on %s is also attached to another object; changing it here would change it there too — "+ + "edit that note in Studio Pro", describeNode(x)) + } + drop[f.id] = true + } + if !seen[n.id] { + seen[n.id] = true + notes = append(notes, n) + } + } + m.removeFlows(drop) + for _, n := range notes { + if err := m.removeObject(n); err != nil { + return err + } + } + return nil +} + // Drop removes target and joins the flows that entered it to the object it // led to. Annotation lines attached to it go with it; the annotations stay. func (m *Mutator) Drop(target model.ID) error { diff --git a/mdl/backend/mfmutator/splice_test.go b/mdl/backend/mfmutator/splice_test.go index c1425116e..fcc4cbb4a 100644 --- a/mdl/backend/mfmutator/splice_test.go +++ b/mdl/backend/mfmutator/splice_test.go @@ -435,3 +435,71 @@ func TestSplice_SetReturnValueEditsTheEndEventOnly(t *testing.T) { t.Errorf("want a refusal for an activity, got %v", err) } } + +func annotationFlow(name, from, to string) bson.D { + return bson.D{ + {Key: "$ID", Value: bin(name)}, + {Key: "$Type", Value: "Microflows$AnnotationFlow"}, + {Key: "DestinationPointer", Value: bin(to)}, + {Key: "OriginPointer", Value: bin(from)}, + } +} + +// ako/mxcli#859 (rehearsal M3): create or modify re-annotating an activity +// takes the notes attached to it out before the replace (RemoveNotes), instead +// of the replace moving them onto the fragment's entry, where the fragment's +// own notes would make two of each. A note another activity shares is refused: +// taking it out would take it from that activity too. Without RemoveNotes the +// stored notes move onto the entry as before (the control). +func TestSplice_RemoveNotesBeforeAReplace(t *testing.T) { + build := func(shared bool) bson.D { + objs := append(line(), obj("note", "Microflows$Annotation", 250, 330)) + flows := append(lineFlows(), annotationFlow("af", "note", "a")) + if shared { + flows = append(flows, annotationFlow("af2", "note", "b")) + } + return unit(objs, flows) + } + t.Run("the notes go, the replacement has none of them", func(t *testing.T) { + m, deps := newMutator(t, build(false)) + if err := m.RemoveNotes(model.ID(uid("a"))); err != nil { + t.Fatalf("remove notes: %v", err) + } + if err := m.Replace(model.ID(uid("a")), oneActivity()); err != nil { + t.Fatalf("replace: %v", err) + } + if err := m.Save(); err != nil { + t.Fatalf("save: %v", err) + } + for _, gone := range []string{"a", "note", "af"} { + if bytes.Contains(deps.saved, types.UUIDToBlob(uid(gone))) { + t.Errorf("%s is still in the unit", gone) + } + } + }) + t.Run("control: without it they move onto the entry", func(t *testing.T) { + m, deps := newMutator(t, build(false)) + frag := oneActivity() + if err := m.Replace(model.ID(uid("a")), frag); err != nil { + t.Fatalf("replace: %v", err) + } + if err := m.Save(); err != nil { + t.Fatalf("save: %v", err) + } + if !bytes.Contains(deps.saved, types.UUIDToBlob(uid("note"))) { + t.Error("the stored note was taken out") + } + for _, f := range m.graph().flows { + if f.id == uid("af") && f.dest != string(frag.Entry) { + t.Errorf("the note's line points at %s, want the fragment's entry", f.dest) + } + } + }) + t.Run("a shared note is refused", func(t *testing.T) { + m, _ := newMutator(t, build(true)) + err := m.RemoveNotes(model.ID(uid("a"))) + if err == nil || !strings.Contains(err.Error(), "also attached to") { + t.Fatalf("replace of an activity whose note another shares: got %v, want a refusal", err) + } + }) +} diff --git a/mdl/backend/microflow_mutation.go b/mdl/backend/microflow_mutation.go index fd52fb336..803d7b150 100644 --- a/mdl/backend/microflow_mutation.go +++ b/mdl/backend/microflow_mutation.go @@ -42,6 +42,11 @@ type MicroflowMutator interface { Replace(target model.ID, frag *MicroflowFragment) error // Drop removes target and joins its incoming flows to its successor. Drop(target model.ID) error + // RemoveNotes removes the annotations attached to target, with their + // lines, so that a Replace or Drop after it does not keep them + // (ako/mxcli#859: a `create or modify` states an activity's notes). A + // note also attached to another object is refused. + RemoveNotes(target model.ID) error // SetReturnValue sets the expression the end event target returns, in // place; "" is no value. SetReturnValue(target model.ID, value string) error diff --git a/mdl/backend/modelsdk/backend.go b/mdl/backend/modelsdk/backend.go index 77766aece..75b8eff1b 100644 --- a/mdl/backend/modelsdk/backend.go +++ b/mdl/backend/modelsdk/backend.go @@ -130,14 +130,38 @@ func (b *Backend) ConnectReadOnly(path string) error { return nil } -// Disconnect closes the modelsdk reader. +// Disconnect closes the modelsdk reader, writing first any unit update a +// deferred run still holds. func (b *Backend) Disconnect() error { if b.reader == nil { return nil } + flushErr := b.FlushDeferredWrites() err := b.reader.Close() b.reader = nil - return err + if err != nil { + return err + } + return flushErr +} + +// DeferUnitWrites holds unit updates in memory until FlushDeferredWrites, so a +// run of statements is written once and judged by its net result +// (mmpr.Writer.DeferUnitWrites, ako/mxcli#872). Reads during the run see the +// held bytes. A no-op on a read-only connection. +func (b *Backend) DeferUnitWrites() { + if b.writer != nil { + b.writer.DeferUnitWrites() + } +} + +// FlushDeferredWrites writes what a deferred run holds, each unit once and +// reconciled against what was stored before the run. +func (b *Backend) FlushDeferredWrites() error { + if b.writer == nil { + return nil + } + return b.writer.FlushDeferredWrites() } // Commit is a no-op for the read-only slice. diff --git a/mdl/backend/modelsdk/microflow_mutator_write.go b/mdl/backend/modelsdk/microflow_mutator_write.go index a9aeffd63..43ff761ee 100644 --- a/mdl/backend/modelsdk/microflow_mutator_write.go +++ b/mdl/backend/modelsdk/microflow_mutator_write.go @@ -4,6 +4,7 @@ package modelsdkbackend import ( "fmt" + "strings" "go.mongodb.org/mongo-driver/bson" @@ -13,6 +14,7 @@ import ( "github.com/mendixlabs/mxcli/modelsdk/codec" "github.com/mendixlabs/mxcli/modelsdk/element" genMf "github.com/mendixlabs/mxcli/modelsdk/gen/microflows" + "github.com/mendixlabs/mxcli/modelsdk/version" "github.com/mendixlabs/mxcli/sdk/microflows" ) @@ -40,6 +42,31 @@ func (b *Backend) OpenMicroflowForMutation(unitID model.ID) (backend.MicroflowMu type codecMicroflowDeps struct{ b *Backend } var _ mfmutator.Deps = codecMicroflowDeps{} +var _ mfmutator.PropertyDeclarer = codecMicroflowDeps{} + +// DeclaresProperty reports whether the project's Mendix version declares key +// on a flow document of docType, from the metamodel's version data: the +// property's own type first, then Microflows$MicroflowBase, which both flow +// kinds extend (ReturnVariableName, introduced in 10.12, lives there). A +// property with no version data is declared at every version. An unknown +// project version declares nothing, so SetHeader keeps refusing (#843). +func (d codecMicroflowDeps) DeclaresProperty(docType, key string) bool { + pv := d.b.ProjectVersion() + if pv == nil || pv.ProductVersion == "" { + return false + } + v := version.Parse(pv.ProductVersion) + if v.IsZero() { + return false + } + prop := strings.ToLower(key[:1]) + key[1:] + for _, typ := range []string{docType, "Microflows$MicroflowBase"} { + if info, ok := genMf.VersionInfos[typ].Properties[prop]; ok { + return info.IsAvailableIn(v) + } + } + return true +} func (d codecMicroflowDeps) SerializeObject(obj microflows.MicroflowObject) (bson.D, error) { el := microflowObjectToGen(obj) diff --git a/mdl/backend/modelsdk/microflow_read_actions.go b/mdl/backend/modelsdk/microflow_read_actions.go index b3fc0d991..8e49cd756 100644 --- a/mdl/backend/modelsdk/microflow_read_actions.go +++ b/mdl/backend/modelsdk/microflow_read_actions.go @@ -952,6 +952,15 @@ func readMappingCall(doc, imc bson.Raw) (h *microflows.ResultHandlingMapping, fo // // So each of those six is admitted only AT that value. Anything else keeps the // raw fallback, which round-trips byte for byte. +// webServiceWrittenKeys are the keys webServiceCallActionToGen writes, beside +// $ID and $Type. +var webServiceWrittenKeys = []string{ + "ErrorHandlingType", "HttpConfiguration", "ImportedService", "IsValidationRequired", + "NewResultHandling", "OperationName", "ProxyConfiguration", "RequestBodyHandling", + "RequestHeaderHandling", "RequestProxyType", "ServiceName", "TimeOutExpression", + "UseRequestTimeOut", +} + func webServiceActionRequiresRawBSON(raw bson.Raw) bool { // Keys the structured form carries in full, whatever their value. represented := map[string]bool{ @@ -972,6 +981,20 @@ func webServiceActionRequiresRawBSON(raw bson.Raw) bool { if err != nil { return true } + // Every key the structured writer emits must be stored: one it adds is a + // change the round trip makes. A stored call without ErrorHandlingType + // reads back as Rollback and was rewritten on every re-run + // (ako/mxcli#861). + for _, key := range webServiceWrittenKeys { + if raw.Lookup(key).Type == 0 { + return true + } + } + // A by-ID service reference has no structured spelling: describe would + // print it as '' and exec write that. + if raw.Lookup("ImportedService").Type != bson.TypeString { + return true + } for _, el := range els { key := el.Key() if represented[key] { diff --git a/mdl/backend/modelsdk/microflow_webservice_test.go b/mdl/backend/modelsdk/microflow_webservice_test.go index b0d2a162b..587698a88 100644 --- a/mdl/backend/modelsdk/microflow_webservice_test.go +++ b/mdl/backend/modelsdk/microflow_webservice_test.go @@ -35,19 +35,36 @@ func TestActionFromGen_WebServiceCall_Raw(t *testing.T) { } } -// TestActionFromGen_WebServiceCall_NoRaw confirms a fully-structured action (only -// describable fields) does NOT set RawBSON, so the renderer uses the readable -// `call web service …` form, matching legacy's supported-key set. +// TestActionFromGen_WebServiceCall_NoRaw confirms an action carrying exactly +// what the structured writer emits does NOT set RawBSON, so the renderer uses +// the readable `call web service …` form. func TestActionFromGen_WebServiceCall_NoRaw(t *testing.T) { + raw, err := bson.Marshal(referenceSoapActionMap()) + if err != nil { + t.Fatal(err) + } + var doc bson.D + if err := bson.Unmarshal(raw, &doc); err != nil { + t.Fatal(err) + } + ws := decodeAction(t, doc).(*microflows.WebServiceCallAction) + if len(ws.RawBSON) != 0 { + t.Errorf("RawBSON set, want empty (the writer's own form → structured form)") + } +} + +// TestActionFromGen_WebServiceCall_MissingKeysRaw: an action without the keys +// the structured writer adds keeps RawBSON, since writing its structured form +// back would add them — ErrorHandlingType among them, which reads back as +// Rollback when absent (ako/mxcli#861). +func TestActionFromGen_WebServiceCall_MissingKeysRaw(t *testing.T) { act := decodeAction(t, bson.D{ {Key: "$ID", Value: "ws-2"}, {Key: "$Type", Value: "Microflows$CallWebServiceAction"}, - {Key: "ErrorHandlingType", Value: "Rollback"}, {Key: "ImportedService", Value: "Mod.Service"}, {Key: "OperationName", Value: "Op"}, }) - ws := act.(*microflows.WebServiceCallAction) - if len(ws.RawBSON) != 0 { - t.Errorf("RawBSON set, want empty (all fields are supported → structured form)") + if ws := act.(*microflows.WebServiceCallAction); len(ws.RawBSON) == 0 { + t.Errorf("RawBSON empty, want set (the structured form would write a different document)") } } diff --git a/mdl/backend/modelsdk/microflow_webservice_write_test.go b/mdl/backend/modelsdk/microflow_webservice_write_test.go index a80139fc3..b9ba283de 100644 --- a/mdl/backend/modelsdk/microflow_webservice_write_test.go +++ b/mdl/backend/modelsdk/microflow_webservice_write_test.go @@ -3,6 +3,7 @@ package modelsdkbackend import ( + "strings" "testing" bsonv1 "go.mongodb.org/mongo-driver/bson" @@ -562,6 +563,18 @@ func TestWebServiceActionRequiresRawBSON_AgreesWithLegacy(t *testing.T) { pms[1].(bsonv2.M)["ParameterPath"] = "http%3A//www.example.com/:GetOrder" }}, {"unknown key entirely", func(m bsonv2.M) { m["SomethingNew"] = int32(1) }}, + // A key the structured form writes but the stored call lacks: written + // back, the call would gain it — the ErrorHandlingType mdl-examples' + // raw payload leaves out reads back as Rollback, so describe -> exec + // changed it, and a re-run re-spliced the call (ako/mxcli#861). + {"ErrorHandlingType absent", func(m bsonv2.M) { delete(m, "ErrorHandlingType") }}, + {"IsValidationRequired absent", func(m bsonv2.M) { delete(m, "IsValidationRequired") }}, + {"HttpConfiguration absent", func(m bsonv2.M) { delete(m, "HttpConfiguration") }}, + {"NewResultHandling absent", func(m bsonv2.M) { delete(m, "NewResultHandling") }}, + // A by-ID service reference: the structured form prints it as ''. + {"ImportedService is not a name", func(m bsonv2.M) { + m["ImportedService"] = bsonv2.Binary{Subtype: 0, Data: make([]byte, 16)} + }}, } { t.Run(tc.name, func(t *testing.T) { m := referenceSoapActionMap() @@ -572,3 +585,19 @@ func TestWebServiceActionRequiresRawBSON_AgreesWithLegacy(t *testing.T) { }) } } + +// webServiceWrittenKeys is what the structured writer emits: a key it adds or +// drops without the list following makes the raw decision wrong one way or the +// other. +func TestWebServiceWrittenKeys_AreTheWritersKeys(t *testing.T) { + doc := encodeMicroflowAction(t, µflows.WebServiceCallAction{ServiceID: "M.S", OperationName: "Op"}) + var got []string + for _, e := range doc { + if e.Key != "$ID" && e.Key != "$Type" { + got = append(got, e.Key) + } + } + if strings.Join(got, ",") != strings.Join(webServiceWrittenKeys, ",") { + t.Errorf("the writer emits %v; webServiceWrittenKeys lists %v", got, webServiceWrittenKeys) + } +} diff --git a/mdl/backend/modelsdk/nanoflow_carry_test.go b/mdl/backend/modelsdk/nanoflow_carry_test.go index 747f73bb4..3ccd699d2 100644 --- a/mdl/backend/modelsdk/nanoflow_carry_test.go +++ b/mdl/backend/modelsdk/nanoflow_carry_test.go @@ -235,3 +235,65 @@ func TestUpdateNanoflow_WritesNoKeyTheStoredDocumentLacks(t *testing.T) { } } } + +// #843 (rehearsal M2): a nanoflow the writer stored without ReturnVariableName +// (created from `returns Boolean`, no `as $Var`) takes the one a later +// `create or modify … returns Boolean as $Done` states, through the header +// patch — which refused it ("set it in Studio Pro"), so no mdl 1 script could +// ever name the return variable of such a nanoflow. +func TestSetHeader_AddsReturnVariableToAStoredNanoflow(t *testing.T) { + b, nf := nanoflowFixture(t) + if _, ok := storedKeys(t, b, nf.ID)["ReturnVariableName"]; ok { + t.Fatal("precondition: the fixture nanoflow already stores ReturnVariableName") + } + if pv := b.ProjectVersion(); pv == nil || !pv.IsAtLeast(10, 12) { + t.Fatalf("precondition: the fixture must be 10.12+, where Nanoflow declares ReturnVariableName; got %+v", pv) + } + + declared := *nf + declared.ReturnVariableName = "Done" + m, err := b.OpenMicroflowForMutation(nf.ID) + if err != nil { + t.Fatalf("OpenMicroflowForMutation: %v", err) + } + changed, err := m.SetHeader(&declared) + if err != nil { + t.Fatalf("SetHeader: %v", err) + } + if len(changed) != 1 || changed[0] != "ReturnVariableName" { + t.Fatalf("changed %v, want [ReturnVariableName]", changed) + } + if err := m.Save(); err != nil { + t.Fatalf("Save: %v", err) + } + if got := storedKeys(t, b, nf.ID)["ReturnVariableName"]; got != "Done" { + t.Fatalf("stored ReturnVariableName = %#v, want Done", got) + } + + // The second statement finds it stored: nothing to patch. + m, err = b.OpenMicroflowForMutation(nf.ID) + if err != nil { + t.Fatalf("OpenMicroflowForMutation: %v", err) + } + if changed, err := m.SetHeader(&declared); err != nil || len(changed) != 0 { + t.Fatalf("second run: changed %v, err %v; want nothing", changed, err) + } +} + +// DeclaresProperty follows the metamodel's version data: ReturnVariableName +// (Microflows$MicroflowBase, 10.12) is declared on the fixture's version, a +// property with no version data is declared, and without a project version +// nothing is. +func TestDeclaresProperty_FollowsTheProjectVersion(t *testing.T) { + b, _ := nanoflowFixture(t) + d := codecMicroflowDeps{b: b} + if !d.DeclaresProperty("Microflows$Nanoflow", "ReturnVariableName") { + t.Error("ReturnVariableName is not declared on a 10.12+ project") + } + if !d.DeclaresProperty("Microflows$Nanoflow", "Documentation") { + t.Error("Documentation, which has no version data, is not declared") + } + if (codecMicroflowDeps{b: New()}).DeclaresProperty("Microflows$Nanoflow", "ReturnVariableName") { + t.Error("a backend with no project version declares a property") + } +} diff --git a/mdl/deprecation/deprecation.go b/mdl/deprecation/deprecation.go index 953f7c802..69cf53731 100644 --- a/mdl/deprecation/deprecation.go +++ b/mdl/deprecation/deprecation.go @@ -229,6 +229,14 @@ const ( // refused from mdl 1 rather than 2, because mdl 1 never had it. ConstantPrivate = "MDL-DEPR138" + // Codes 160-169 are the migration aliases the beta dress rehearsal found + // (ako/mxcli#714), numbered apart so the parallel fixes do not collide. + + // DateType is `date` as a type: Mendix has no date-only type, and mxcli + // always stored it as a DateTime (ako/mxcli#706, rehearsal U1). Refused + // from mdl 1 rather than 2, because mdl 1 never had it. + DateType = "MDL-DEPR160" + // Codes 080–089 are the rest of R5 (ako/mxcli#753): expressions bare, one // constant reference, and the revoke that mirrors the grant. @@ -1261,6 +1269,17 @@ var r8Entries = []Entry{ Example: "create microflow M.F () begin call rest service get 'https://example.com' returns none; end;", CanonicalExample: "create microflow M.F () begin call rest service get 'https://example.com' returns nothing; end;", }, + { + Code: DateType, + Old: "date", + Canonical: "DateTime", + Rewrite: Rewrite{Structural: "type `date` as the type it was stored as: `DateTime`"}, + RemovedIn: 1, + Note: "Mendix has no date-only type: `date` was always stored as a DateTime, and still is. " + + "To show only the date, give the widget a date format.", + Example: "create persistent entity M.Account ( LastImport: date );", + CanonicalExample: "create persistent entity M.Account ( LastImport: DateTime );", + }, } // All returns every registered entry, in code order. diff --git a/mdl/deprecation/deprecation_test.go b/mdl/deprecation/deprecation_test.go index 7d4bcbb31..a69128be9 100644 --- a/mdl/deprecation/deprecation_test.go +++ b/mdl/deprecation/deprecation_test.go @@ -79,4 +79,5 @@ func TestParsePolicy(t *testing.T) { // refusedFromMdl1 are the entries whose old form mdl 1 never accepted. var refusedFromMdl1 = map[string]bool{ ConstantPrivate: true, // a no-op the old catch-all swallowed (ako/mxcli#865) + DateType: true, // refused in every version by #706 before mdl 1 existed (rehearsal U1) } diff --git a/mdl/executor/access_rule_run.go b/mdl/executor/access_rule_run.go new file mode 100644 index 000000000..35ee6bedc --- /dev/null +++ b/mdl/executor/access_rule_run.go @@ -0,0 +1,73 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "github.com/mendixlabs/mxcli/mdl/ast" + mdlerrors "github.com/mendixlabs/mxcli/mdl/errors" +) + +// unitWriteDeferrer is the optional backend capability a run of access-rule +// statements is written through. A backend without it (the MCP backend, the +// mock) writes each statement as it always has. +type unitWriteDeferrer interface { + DeferUnitWrites() + FlushDeferredWrites() error +} + +// accessRuleRun writes a run of consecutive entity access-rule statements once, +// at its end, so it is judged by the rules it leaves rather than by each step. +// +// A "reset, then authoritative grants" section — `revoke all on entity E from R;` +// followed by the grants meant to hold — rewrote the domain model on every run +// and re-minted the rule, although it ended with the rules it began with: the +// revoke was written, and the grant was reconciled against a unit that no longer +// had the rule to carry its identity from (ako/mxcli#872, rehearsal W3). Held +// until the run ends, the domain model is reconciled once against what was stored +// before the run: a run that nets to nothing writes nothing, and one that changes +// a rule keeps the $IDs of the rules it re-grants. +// +// The run is only GRANT/REVOKE on an entity. They write the domain model through +// one path and read it back through the reader, which serves the held bytes; any +// other statement ends the run first, so nothing that writes another way ever +// sees — or is shadowed by — a held unit. +type accessRuleRun struct { + open unitWriteDeferrer +} + +func isAccessRuleStmt(stmt ast.Statement) bool { + switch stmt.(type) { + case *ast.GrantEntityAccessStmt, *ast.RevokeEntityAccessStmt: + return true + } + return false +} + +// step opens the run before an access-rule statement and ends it before any +// other statement. +func (r *accessRuleRun) step(e *Executor, stmt ast.Statement) error { + if !isAccessRuleStmt(stmt) { + return r.end() + } + if r.open != nil || e.backend == nil || !e.backend.IsConnected() { + return nil + } + if d, ok := e.backend.(unitWriteDeferrer); ok { + d.DeferUnitWrites() + r.open = d + } + return nil +} + +// end writes what the run holds. Safe to call with no run open. +func (r *accessRuleRun) end() error { + if r.open == nil { + return nil + } + d := r.open + r.open = nil + if err := d.FlushDeferredWrites(); err != nil { + return mdlerrors.NewBackend("write access rules", err) + } + return nil +} diff --git a/mdl/executor/access_rule_run_test.go b/mdl/executor/access_rule_run_test.go new file mode 100644 index 000000000..fab24bd41 --- /dev/null +++ b/mdl/executor/access_rule_run_test.go @@ -0,0 +1,78 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "bytes" + "errors" + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/backend/mock" +) + +// deferringMock is a mock backend with the deferred-write capability whose +// flush fails, standing in for a held write that cannot land. +type deferringMock struct { + *mock.MockBackend + opened, flushed int + flushErr error +} + +func (d *deferringMock) DeferUnitWrites() { d.opened++ } +func (d *deferringMock) FlushDeferredWrites() error { + d.flushed++ + return d.flushErr +} + +// ako/mxcli#872: a run of access-rule statements is written at its end. When a +// statement of the run fails, the run is still flushed — and if that flush fails +// too, the earlier statements' writes did not land although each printed that it +// had. That failure must be returned with the statement's, not discarded. +func TestAccessRuleRun_FlushErrorOnFailedStatementIsReturned(t *testing.T) { + for _, tc := range []struct { + name string + run func(e *Executor, prog *ast.Program) error + }{ + {"ExecuteProgram", func(e *Executor, prog *ast.Program) error { return e.ExecuteProgram(prog) }}, + {"ExecuteProgramContinueOnError", func(e *Executor, prog *ast.Program) error { + var out bytes.Buffer + _, err := e.ExecuteProgramContinueOnError(prog, &out) + return err + }}, + } { + t.Run(tc.name, func(t *testing.T) { + var buf bytes.Buffer + e := New(&buf) + d := &deferringMock{ + MockBackend: &mock.MockBackend{IsConnectedFunc: func() bool { return true }}, + flushErr: errors.New("held write could not land"), + } + e.backend = d + // The mock knows no module, so the grant fails after the run opened. + prog := &ast.Program{Statements: []ast.Statement{ + &ast.GrantEntityAccessStmt{Entity: ast.QualifiedName{Module: "M", Name: "E"}}, + }} + err := tc.run(e, prog) + if d.opened != 1 || d.flushed != 1 { + t.Fatalf("run opened %d / flushed %d times, want 1 / 1", d.opened, d.flushed) + } + if err == nil || !strings.Contains(err.Error(), "held write could not land") { + t.Errorf("the failed flush was discarded; got error %v", err) + } + }) + } + + // Control: a flush that succeeds adds nothing to the statement's error. + var buf bytes.Buffer + e := New(&buf) + d := &deferringMock{MockBackend: &mock.MockBackend{IsConnectedFunc: func() bool { return true }}} + e.backend = d + err := e.ExecuteProgram(&ast.Program{Statements: []ast.Statement{ + &ast.GrantEntityAccessStmt{Entity: ast.QualifiedName{Module: "M", Name: "E"}}, + }}) + if err == nil || strings.Contains(err.Error(), "write access rules") { + t.Errorf("control: want only the statement's own error, got %v", err) + } +} diff --git a/mdl/executor/cmd_alter_flow.go b/mdl/executor/cmd_alter_flow.go index dae1194e5..af6620e89 100644 --- a/mdl/executor/cmd_alter_flow.go +++ b/mdl/executor/cmd_alter_flow.go @@ -79,6 +79,13 @@ func (a *alterFlowContext) applyTo(ctx *ExecContext, mut backend.MicroflowMutato fail := func(err error) error { return mdlerrors.NewValidation(fmt.Sprintf("alter %s %s: %s %s: %v", s.Kind(), s.Name, op.Op, op.Target, err)) } + if op.ReplaceNotes { + // The statement states the activity's notes, so the stored ones + // go with it rather than stay (ako/mxcli#859). + if err := mut.RemoveNotes(target.ID); err != nil { + return fail(err) + } + } if op.Op == ast.AlterFlowDrop { if err := a.checkOutputUnused(target, nil); err != nil { return fail(err) @@ -157,6 +164,14 @@ type alterFlowContext struct { // removedIDs are the stored activities earlier operations took out; what // they read no longer counts as a use. removedIDs map[model.ID]bool + + // declaredVarTypes is the entity each variable holds as a full build of + // the declared flow resolves it ("Module.Entity" or "List of + // Module.Entity"), when the statement is a `create or modify` whose + // declared body builds. A spliced fragment is built with it, so it writes + // the members a full build of the same statement writes (ako/mxcli#885): + // the stored flow alone does not say what every variable holds. + declaredVarTypes map[string]string } // noteRemoved records that target's output is gone, unless the fragment that @@ -290,6 +305,10 @@ func (a *alterFlowContext) returnValue(ctx *ExecContext, ret *ast.ReturnStmt) (s // variables the stored flow declares. func (a *alterFlowContext) fragmentBuilder(ctx *ExecContext) *flowBuilder { varTypes, declared := a.storedVariables(ctx) + for name, t := range a.declaredVarTypes { + varTypes[name] = t + delete(declared, name) + } hierarchy, _ := getHierarchy(ctx) restServices, _ := loadRestServices(ctx) return &flowBuilder{ @@ -305,6 +324,10 @@ func (a *alterFlowContext) fragmentBuilder(ctx *ExecContext) *flowBuilder { hierarchy: hierarchy, restServices: restServices, isNanoflow: a.stmt.Nanoflow, + + // A fragment is spliced into a stored flow: a member it cannot + // qualify is refused, never written bare (ako/mxcli#885). + qualifiedMembersOnly: true, } } @@ -433,6 +456,37 @@ func (a *alterFlowContext) storedVariables(ctx *ExecContext) (varTypes, declared } else { declared[c.OutputVariable] = "Object" } + case *microflows.RetrieveAction: + // A database retrieve names its entity; one over an association + // depends on which side it starts from and stays untyped here. + src, ok := x.Source.(*microflows.DatabaseRetrieveSource) + entity := "" + if ok { + entity = src.EntityQualifiedName + if entity == "" { + entity = a.entityNames[src.EntityID] + } + } + switch { + case entity == "": + declared[c.OutputVariable] = "Unknown" + case src.Range != nil && src.Range.RangeType == microflows.RangeTypeFirst: + varTypes[c.OutputVariable] = entity + default: + varTypes[c.OutputVariable] = "List of " + entity + } + case *microflows.MicroflowCallAction: + // A call's result has the called microflow's return type. + var rt microflows.DataType + if x.MicroflowCall != nil { + rt = (&flowBuilder{backend: ctx.Backend}).lookupMicroflowReturnType(x.MicroflowCall.Microflow) + } + switch rt.(type) { + case *microflows.ObjectType, *microflows.ListType: + add(c.OutputVariable, rt) + default: + declared[c.OutputVariable] = "Unknown" + } default: declared[c.OutputVariable] = "Unknown" } diff --git a/mdl/executor/cmd_flow_modify.go b/mdl/executor/cmd_flow_modify.go index 23efb5103..4cb2aef22 100644 --- a/mdl/executor/cmd_flow_modify.go +++ b/mdl/executor/cmd_flow_modify.go @@ -254,6 +254,12 @@ func planFlowModify(ctx *ExecContext, d *flowDecl) (*flowPlan, error) { } moves = append(moves, parameterMoves(d, storedParams)...) + // The fragments are built knowing what each variable holds in the declared + // flow, as the full build does: the stored flow does not type a variable + // bound by a retrieve over an association, a list operation or a loop, and + // a member of one was written bare (ako/mxcli#885). + a.declaredVarTypes = varTypes + p := &flowPlan{a: a, ops: ops, moves: moves, storedFolder: storedFolderOf(stored)} if len(ops) > 0 || len(moves) > 0 || headerChanged { if p.mut, err = patch(ctx, a, ops, targets, moves); err != nil { @@ -822,19 +828,31 @@ func (pd *patchDiff) gap(ins []ast.MicroflowStatement, stored []ast.MicroflowSta } rest, restStmts := cands, del if len(ins) > 0 { - body, err := keepStoredNotes(del[0], ins) + body, replaceNotes, err := keepStoredNotes(del[0], ins) if err != nil { return err } pd.add(ast.AlterFlowReplace, cands[0], body) + pd.ops[len(pd.ops)-1].ReplaceNotes = replaceNotes rest, restStmts = cands[1:], del[1:] } for i, c := range rest { - if ann := statementAnnotations(restStmts[i]); ann != nil && len(ann.Notes) > 0 { - return cannotSplice("the %s dropped at (%d, %d) carries an annotation, which would be left behind unattached", - statementKind(restStmts[i]), c.Object.GetPosition().X, c.Object.GetPosition().Y) + // A dropped activity's notes go with it: the declared statements + // state every note they keep, and draw it (ako/mxcli#859). One shared + // with another activity would go from there too. + var notes []ast.MicroflowAnnotation + if ann := statementAnnotations(restStmts[i]); ann != nil { + notes = ann.Notes + } + for _, n := range notes { + if n.Label != "" { + return cannotSplice("the %s dropped at (%d, %d) carries an annotation shared with another activity (id: %s), "+ + "which dropping it would take from that activity too", + statementKind(restStmts[i]), c.Object.GetPosition().X, c.Object.GetPosition().Y, n.Label) + } } pd.add(ast.AlterFlowDrop, c, nil) + pd.ops[len(pd.ops)-1].ReplaceNotes = len(notes) > 0 } return nil } @@ -932,30 +950,42 @@ func describeAt(st ast.MicroflowStatement) string { } // keepStoredNotes prepares the declared statements that replace a stored one. -// The splice keeps the stored activity's annotation notes and attaches them to -// the replacement's first activity, so the declared statement must carry the -// same notes — and they are taken off it, or the builder would draw each one a -// second time. A replaced statement with other notes than the stored one is a -// change of annotations, which the splice does not make. -func keepStoredNotes(stored ast.MicroflowStatement, declared []ast.MicroflowStatement) ([]ast.MicroflowStatement, error) { +// When the declared statement carries the stored activity's notes, the splice +// keeps the stored notes and attaches them to the replacement's first +// activity, so they are taken off the declared statement — or the builder +// would draw each one a second time. +// +// When it carries other notes — one added, reworded or taken off — the +// declared statement keeps them, the builder draws them, and replaceNotes says +// the stored notes go out with the activity (ako/mxcli#859): the statement +// states the activity's notes, like the rest of it. A note the stored flow +// shares with another activity (describe gives it an id) is refused: changing +// it here would change it there too. +func keepStoredNotes(stored ast.MicroflowStatement, declared []ast.MicroflowStatement) (out []ast.MicroflowStatement, replaceNotes bool, err error) { var storedNotes []ast.MicroflowAnnotation if ann := statementAnnotations(stored); ann != nil { storedNotes = ann.Notes } if len(storedNotes) == 0 { - return declared, nil + return declared, false, nil } var declaredNotes []ast.MicroflowAnnotation if ann := statementAnnotations(declared[0]); ann != nil { declaredNotes = ann.Notes } if !declaredMatches(declaredNotes, storedNotes) { - return nil, cannotSplice("the annotations on the replaced %s change; the splice keeps a replaced activity's notes as stored", - statementKind(stored)) + for _, n := range append(append([]ast.MicroflowAnnotation(nil), storedNotes...), declaredNotes...) { + if n.Label != "" { + return nil, false, cannotSplice("the annotations on the replaced %s change, and one of them is shared "+ + "with another activity (id: %s); the splice changes the notes of one activity only", + statementKind(stored), n.Label) + } + } + return declared, true, nil } - out := append([]ast.MicroflowStatement(nil), declared...) + out = append([]ast.MicroflowStatement(nil), declared...) out[0] = withAnnotations(declared[0], func(a *ast.ActivityAnnotations) { a.Notes = nil }) - return out, nil + return out, false, nil } // withoutFreeNotes returns the statements with their free annotations taken diff --git a/mdl/executor/cmd_microflows_build.go b/mdl/executor/cmd_microflows_build.go index 4414c8db4..be063d3f6 100644 --- a/mdl/executor/cmd_microflows_build.go +++ b/mdl/executor/cmd_microflows_build.go @@ -426,6 +426,7 @@ func buildMicroflowFromStmt(ctx *ExecContext, s *ast.CreateMicroflowStmt, opts b quiet: opts.Quiet, hierarchy: hierarchy, restServices: restServices, + self: &selfFlow{qualifiedName: s.Name.Module + "." + s.Name.Name, returnType: mf.ReturnType}, } mf.ObjectCollection = builder.buildFlowGraph(s.Body, s.ReturnType) @@ -723,6 +724,7 @@ func buildNanoflowFromStmt(ctx *ExecContext, s *ast.CreateNanoflowStmt, opts bui restServices: restServices, isNanoflow: true, quiet: opts.Quiet, + self: &selfFlow{qualifiedName: s.Name.Module + "." + s.Name.Name, nanoflow: true, returnType: nf.ReturnType}, } nf.ObjectCollection = builder.buildFlowGraph(s.Body, s.ReturnType) diff --git a/mdl/executor/cmd_microflows_builder.go b/mdl/executor/cmd_microflows_builder.go index 3b01bf81e..c425d7b36 100644 --- a/mdl/executor/cmd_microflows_builder.go +++ b/mdl/executor/cmd_microflows_builder.go @@ -40,6 +40,13 @@ type flowBuilder struct { // from the script, while still reserving the name against a second create of // the same entity (see freshCreateVariable). generatedVars map[string]bool + // qualifiedMembersOnly refuses an entity member the builder cannot + // qualify — for want of the entity its variable holds — instead of writing + // it bare. Set for a fragment spliced into a stored flow (ako/mxcli#885): + // a bare change member makes the project unloadable ("not a valid + // AttributeIdentifier"), a bare find member becomes a find by an invalid + // expression, a bare aggregate attribute is dropped. + qualifiedMembersOnly bool // textLang is the language a bare message/caption string is stored under // (mendixlabs/mxcli#970). Empty means en_US, which keeps a zero-value // flowBuilder — validateFlowBody builds one — behaving as before. @@ -95,6 +102,10 @@ type flowBuilder struct { nanoflowsCacheLoaded bool manualLoopBackTarget model.ID isNanoflow bool // true when building a nanoflow — default error handling is "" not "Rollback" + // self is the flow the statement creates. A call to it resolves to the + // statement itself, not the project: on a first create the document does + // not exist yet, so a recursive flow was refused "not found" (#843). + self *selfFlow // Pending custom error-handler routing uses two representations: the // currently active handler lives in the flat fields below, while handlers // postponed across branch boundaries are queued in pendingErrorHandlers. @@ -255,7 +266,23 @@ func (fb *flowBuilder) registerResultVariableType(varName string, dt microflows. // lookupMicroflowReturnType resolves the return type of a called microflow by // qualified name so downstream activities can infer variable types. +// selfFlow names the flow a create statement builds and what it returns. +type selfFlow struct { + qualifiedName string + nanoflow bool + returnType microflows.DataType +} + +// isSelf reports whether qualifiedName is the flow being built, of the kind +// the call names: a nanoflow never calls a microflow of its own name. +func (fb *flowBuilder) isSelf(qualifiedName string, nanoflow bool) bool { + return fb.self != nil && fb.self.nanoflow == nanoflow && fb.self.qualifiedName == qualifiedName +} + func (fb *flowBuilder) lookupMicroflowReturnType(qualifiedName string) microflows.DataType { + if fb.isSelf(qualifiedName, false) { + return fb.self.returnType + } if fb.backend == nil || qualifiedName == "" { return nil } @@ -304,6 +331,9 @@ func (fb *flowBuilder) lookupMicroflowReturnType(qualifiedName string) microflow } func (fb *flowBuilder) lookupNanoflowReturnType(qualifiedName string) microflows.DataType { + if fb.isSelf(qualifiedName, true) { + return fb.self.returnType + } if fb.backend == nil || qualifiedName == "" { return nil } @@ -355,7 +385,7 @@ func (fb *flowBuilder) lookupNanoflowReturnType(qualifiedName string) microflows // in the connected project. Returns true (no error) when no backend is available // or when backend calls fail, so offline / syntax-check mode is unaffected. func (fb *flowBuilder) microflowExists(qualifiedName string) bool { - if fb.backend == nil { + if fb.backend == nil || fb.isSelf(qualifiedName, false) { return true } // Fast path: name-indexed lookup; succeeds without loading the full list. @@ -397,7 +427,7 @@ func (fb *flowBuilder) microflowExists(qualifiedName string) bool { // nanoflowExists returns true if qualifiedName refers to a nanoflow present // in the connected project. Same fallback-to-true semantics as microflowExists. func (fb *flowBuilder) nanoflowExists(qualifiedName string) bool { - if fb.backend == nil { + if fb.backend == nil || fb.isSelf(qualifiedName, true) { return true } if rawUnit, err := fb.backend.GetRawUnitByName("nanoflow", qualifiedName); err == nil && rawUnit != nil { diff --git a/mdl/executor/cmd_microflows_builder_actions.go b/mdl/executor/cmd_microflows_builder_actions.go index 5b7670d7f..24dd44f0d 100644 --- a/mdl/executor/cmd_microflows_builder_actions.go +++ b/mdl/executor/cmd_microflows_builder_actions.go @@ -1208,7 +1208,7 @@ func (fb *flowBuilder) addRetrieveAction(s *ast.RetrieveStmt) model.ID { // Convert WHERE expression if present // XPath constraints are stored with square brackets in BSON: [expression] if s.Where != nil { - dbSource.XPathConstraint = retrieveXPathConstraint(s.Where) + dbSource.XPathConstraint = retrieveXPathConstraint(s.Where, s.Source.String()) } // Convert SORT BY columns if present @@ -1364,8 +1364,11 @@ func (fb *flowBuilder) addRetrieveAction(s *ast.RetrieveStmt) model.ID { return activity.ID } -func retrieveXPathConstraint(expr ast.Expression) string { - xpath := normalizeXPathEnumRefs(expressionToXPath(expr)) +// retrieveXPathConstraint is the constraint a database retrieve of entity stores +// for its where clause. A qualified attribute of entity is stored bare and an +// enumeration value as a string literal (storedXPathConstraint, #874). +func retrieveXPathConstraint(expr ast.Expression, entity string) string { + xpath := storedXPathConstraint(expressionToXPathNames(expr), entity) if strings.HasPrefix(strings.TrimSpace(xpath), "[") && strings.HasSuffix(strings.TrimSpace(xpath), "]") { return visitor.FormatXPathConstraint(strings.TrimSpace(xpath)) } @@ -1710,6 +1713,9 @@ func (fb *flowBuilder) addListOperationAction(s *ast.ListOperationStmt) model.ID if entityType != "" && !strings.Contains(spec.Attribute, ".") { attrQN = entityType + "." + spec.Attribute } + if fb.qualifiedMembersOnly { + fb.refuseUnqualifiedAttribute(attrQN, spec.Attribute, entityType) + } sortItems = append(sortItems, µflows.SortItem{ BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())}, AttributeQualifiedName: attrQN, @@ -1929,6 +1935,9 @@ func (fb *flowBuilder) addAggregateListAction(s *ast.AggregateListStmt) model.ID action.AttributeQualifiedName = entityType + "." + s.Attribute } } + if fb.qualifiedMembersOnly && action.AttributeQualifiedName == "" { + fb.refuseUnqualifiedAttribute(s.Attribute, s.Attribute, fb.listElementEntity(s.InputVariable)) + } } activity := µflows.ActionActivity{ @@ -2093,6 +2102,9 @@ func isValidMemberIdentifier(name string) bool { } func (fb *flowBuilder) resolveMemberChange(mc *microflows.MemberChange, memberName string, entityQN string) { + if fb.qualifiedMembersOnly { + defer func() { fb.refuseUnqualifiedAttribute(mc.AttributeQualifiedName, memberName, entityQN) }() + } // Guard against a malformed member identifier reaching the writer, where it // would serialize as an invalid Attribute/Association value that passes // `mxcli check` but fails to load in MxBuild/Studio Pro (StorageLoadException: @@ -2199,6 +2211,23 @@ func (fb *flowBuilder) resolveMemberChange(mc *microflows.MemberChange, memberNa resolveMemberChangeFallback(mc, memberName, entityQN) } +// refuseUnqualifiedAttribute records an error when attrQN — the attribute a +// member resolved to — is not Module.Entity.Attribute, under +// qualifiedMembersOnly. entityQN is the entity the member was resolved +// against, "" when the variable's entity is not known. +func (fb *flowBuilder) refuseUnqualifiedAttribute(attrQN, memberName, entityQN string) { + if attrQN == "" || strings.Count(attrQN, ".") >= 2 { + return + } + if entityQN == "" { + fb.addError("cannot qualify member %q: the entity its variable holds is not known here, and a bare "+ + "attribute name would make the project unloadable (\"not a valid AttributeIdentifier\"); "+ + "name it in full as Module.Entity.%s", memberName, memberName) + return + } + fb.addError("cannot qualify member %q on %s; name it in full as Module.Entity.Attribute", memberName, entityQN) +} + func (fb *flowBuilder) resolveAttributeInEntityHierarchy(entityQN, attrName string) (string, bool) { if fb == nil || fb.backend == nil || entityQN == "" || attrName == "" { return "", false diff --git a/mdl/executor/cmd_microflows_builder_control.go b/mdl/executor/cmd_microflows_builder_control.go index 7333155ca..c398c24dd 100644 --- a/mdl/executor/cmd_microflows_builder_control.go +++ b/mdl/executor/cmd_microflows_builder_control.go @@ -659,6 +659,7 @@ func (fb *flowBuilder) addLoopStatement(s *ast.LoopStmt) model.ID { hierarchy: fb.hierarchy, // Share hierarchy restServices: fb.restServices, // Share REST services for parameter classification isNanoflow: fb.isNanoflow, + self: fb.self, // Share the note registry, so a note declared outside the loop and // referenced on a body activity attaches to the SAME Annotation rather // than being refused. The describer emits exactly that (its label state @@ -1016,6 +1017,7 @@ func (fb *flowBuilder) addWhileStatement(s *ast.WhileStmt) model.ID { hierarchy: fb.hierarchy, restServices: fb.restServices, isNanoflow: fb.isNanoflow, + self: fb.self, // Share the note registry, so a note declared outside the loop and // referenced on a body activity attaches to the SAME Annotation rather // than being refused. The describer emits exactly that (its label state diff --git a/mdl/executor/cmd_microflows_builder_flows.go b/mdl/executor/cmd_microflows_builder_flows.go index dceddcb13..8a49574f2 100644 --- a/mdl/executor/cmd_microflows_builder_flows.go +++ b/mdl/executor/cmd_microflows_builder_flows.go @@ -743,6 +743,7 @@ func (fb *flowBuilder) addErrorHandlerFlow(sourceActivityID model.ID, sourceX in hierarchy: fb.hierarchy, restServices: fb.restServices, isNanoflow: fb.isNanoflow, + self: fb.self, // A handler's activities are merged into the PARENT's object collection // below, so a note declared outside the handler and referenced inside it // (or the reverse) lands in one collection — sharing the registry is diff --git a/mdl/executor/cmd_microflows_format_action.go b/mdl/executor/cmd_microflows_format_action.go index ec1331bc4..689150bf6 100644 --- a/mdl/executor/cmd_microflows_format_action.go +++ b/mdl/executor/cmd_microflows_format_action.go @@ -875,7 +875,15 @@ func formatAction( if len(params) > 0 { paramStr = "(" + strings.Join(params, ", ") + ")" } - return fmt.Sprintf("show page %s%s;", pageName, paramStr) + // The page title override. Left out, describe -> exec dropped it, and + // taking it out of a create or modify compared as no change + // (ako/mxcli#869). An override without text has no statement form: + // `with title = ''` builds none. + titleStr := "" + if title := pickTextTranslation(a.OverridePageTitle, describeDefaultLanguage(ctx)); title != "" { + titleStr = " with title = " + mdlQuote(ctx, title) + } + return fmt.Sprintf("show page %s%s%s;", pageName, paramStr, titleStr) case *microflows.ClosePageAction: if a.NumberOfPages > 1 { diff --git a/mdl/executor/cmd_microflows_format_action_test.go b/mdl/executor/cmd_microflows_format_action_test.go index 92dd0969b..4d0476922 100644 --- a/mdl/executor/cmd_microflows_format_action_test.go +++ b/mdl/executor/cmd_microflows_format_action_test.go @@ -532,6 +532,29 @@ func TestFormatAction_ShowPage_WithParams(t *testing.T) { } } +// A page title override is described, so describe -> exec keeps it and taking +// it out of a create or modify is a difference (ako/mxcli#869). +func TestFormatAction_ShowPage_WithTitle(t *testing.T) { + e := newTestExecutor() + action := µflows.ShowPageAction{ + PageName: "MyModule.OrderDetail", + PageParameterMappings: []*microflows.PageParameterMapping{ + {Parameter: "MyModule.OrderDetail.Order", Argument: "$Order"}, + }, + OverridePageTitle: &model.Text{Translations: map[string]string{"en_US": "Order's details"}}, + } + got := e.formatAction(action, nil, nil) + want := "show page MyModule.OrderDetail(Order = $Order) with title = 'Order''s details';" + if got != want { + t.Errorf("got %q, want %q", got, want) + } + // An override with no text states nothing the statement can carry. + action.OverridePageTitle = &model.Text{} + if got := e.formatAction(action, nil, nil); got != "show page MyModule.OrderDetail(Order = $Order);" { + t.Errorf("empty override: got %q", got) + } +} + func TestFormatAction_ClosePage(t *testing.T) { e := newTestExecutor() action := µflows.ClosePageAction{NumberOfPages: 1} diff --git a/mdl/executor/cmd_microflows_helpers.go b/mdl/executor/cmd_microflows_helpers.go index 937a8ae82..53ec4016c 100644 --- a/mdl/executor/cmd_microflows_helpers.go +++ b/mdl/executor/cmd_microflows_helpers.go @@ -84,6 +84,18 @@ func isWordByte(c byte) bool { return mendixexpr.IsWordBy // Unlike expressionToString (for Mendix expressions), XPath requires Mendix // tokens like [%CurrentDateTime%] to be quoted: '[%CurrentDateTime%]'. func expressionToXPath(expr ast.Expression) string { + return xpathOf(expr, false) +} + +// expressionToXPathNames is expressionToXPath with every qualified name left as +// written, for a writer that decides what a three-part name is itself — an +// attribute of the constrained entity or an enumeration value — with +// storedXPathConstraint (ako/mxcli#874). +func expressionToXPathNames(expr ast.Expression) string { + return xpathOf(expr, true) +} + +func xpathOf(expr ast.Expression, keepNames bool) string { if expr == nil { return "" } @@ -95,29 +107,29 @@ func expressionToXPath(expr ast.Expression) string { case *ast.TokenExpr: return "'[%" + e.Token + "%]'" case *ast.BinaryExpr: - left := expressionToXPath(e.Left) - right := expressionToXPath(e.Right) + left := xpathOf(e.Left, keepNames) + right := xpathOf(e.Right, keepNames) op := strings.ToLower(e.Operator) return left + " " + op + " " + right case *ast.UnaryExpr: - operand := expressionToXPath(e.Operand) + operand := xpathOf(e.Operand, keepNames) op := strings.ToLower(e.Operator) // For 'not' with parenthesized operand, output as not(expr) if op == "not" { if p, ok := e.Operand.(*ast.ParenExpr); ok { - return "not(" + expressionToXPath(p.Inner) + ")" + return "not(" + xpathOf(p.Inner, keepNames) + ")" } return "not(" + operand + ")" } return op + " " + operand case *ast.ParenExpr: - return "(" + expressionToXPath(e.Inner) + ")" + return "(" + xpathOf(e.Inner, keepNames) + ")" case *ast.XPathPathExpr: - return xpathPathExprToString(e) + return xpathPathOf(e, keepNames) case *ast.FunctionCallExpr: var args []string for _, arg := range e.Arguments { - args = append(args, expressionToXPath(arg)) + args = append(args, xpathOf(arg, keepNames)) } return mendixFunctionName(e.Name) + "(" + strings.Join(args, ", ") + ")" case *ast.LiteralExpr: @@ -126,12 +138,15 @@ func expressionToXPath(expr ast.Expression) string { } return expressionToString(expr) case *ast.QualifiedNameExpr: + if keepNames { + return e.QualifiedName.String() + } return qualifiedNameToXPath(e) case *ast.SourceExpr: if e.Source != "" { return e.Source } - return expressionToXPath(e.Expression) + return xpathOf(e.Expression, keepNames) default: // For all other expression types, the standard serialization is correct return expressionToString(expr) @@ -412,11 +427,15 @@ func xpathExprToMDLString(expr ast.Expression) string { // xpathPathExprToString serializes an XPathPathExpr to an XPath path string. func xpathPathExprToString(path *ast.XPathPathExpr) string { + return xpathPathOf(path, false) +} + +func xpathPathOf(path *ast.XPathPathExpr, keepNames bool) string { var parts []string for _, step := range path.Steps { - s := expressionToXPath(step.Expr) + s := xpathOf(step.Expr, keepNames) if step.Predicate != nil { - s += "[" + expressionToXPath(step.Predicate) + "]" + s += "[" + xpathOf(step.Predicate, keepNames) + "]" } parts = append(parts, s) } diff --git a/mdl/executor/cmd_pages_builder_v3.go b/mdl/executor/cmd_pages_builder_v3.go index 01e8c5010..7aef506c4 100644 --- a/mdl/executor/cmd_pages_builder_v3.go +++ b/mdl/executor/cmd_pages_builder_v3.go @@ -1079,8 +1079,8 @@ func (pb *pageBuilder) applyDatabaseClausesV3(dbSource *pages.DatabaseSource, ds // joints (upstream #979). One that already fits is returned unchanged, so // this does not churn existing pages. if ds.Where != "" { - dbSource.XPathConstraint = visitor.FormatXPathConstraint( - pb.expandXPathAssociationPath(ds.Where, entity)) + dbSource.XPathConstraint = visitor.FormatXPathConstraint(storedXPathConstraint( + pb.expandXPathAssociationPath(ds.Where, entity), entity)) } // Handle ORDER BY diff --git a/mdl/executor/cmd_security_write.go b/mdl/executor/cmd_security_write.go index 4d30e6e77..c1579688e 100644 --- a/mdl/executor/cmd_security_write.go +++ b/mdl/executor/cmd_security_write.go @@ -595,7 +595,7 @@ func execGrantEntityAccess(ctx *ExecContext, s *ast.GrantEntityAccessStmt) error // ONCE, here, because the stored text is also what identifies the rule: a rule // is keyed by role plus constraint (#936), so echoing the result back with the // unformatted spelling would look for a rule that is not there. - xpathConstraint := visitor.FormatXPathConstraint(s.XPathConstraint) + xpathConstraint := accessRuleXPathConstraint(ctx, s.XPathConstraint, s.Entity.String()) if err := ctx.Backend.AddEntityAccessRule(backend.EntityAccessRuleParams{ UnitID: dm.ID, @@ -2009,3 +2009,11 @@ func refuseSystemEntityGrant(moduleName, entityName string) error { "record who acted when they act.", moduleName, entityName) } + +// accessRuleXPathConstraint is the constraint an access rule on entity stores: +// a qualified attribute of entity (or of a generalization of it) bare and an +// enumeration value as a string literal (storedModelXPathConstraint, +// ako/mxcli#874), laid out for Studio Pro's editor. +func accessRuleXPathConstraint(ctx *ExecContext, xpath, entity string) string { + return visitor.FormatXPathConstraint(storedModelXPathConstraint(ctx, xpath, entity)) +} diff --git a/mdl/executor/cmd_workflows_write.go b/mdl/executor/cmd_workflows_write.go index a9c1dd88f..c309543c9 100644 --- a/mdl/executor/cmd_workflows_write.go +++ b/mdl/executor/cmd_workflows_write.go @@ -523,8 +523,14 @@ func buildUserTask(n *ast.WorkflowUserTaskNode) *workflows.UserTask { Microflow: n.Targeting.Microflow.Module + "." + n.Targeting.Microflow.Name, } case "xpath": + // Targeting XPath is evaluated on System.User, group targeting on + // System.WorkflowGroup (ako/mxcli#874). Only the names qualified with + // that entity (or a path step) are resolved: with no project here, any + // other three-part name — an enumeration value, or an attribute of the + // configured workflow user entity — is left as written, as it always + // was, rather than guessed into a string literal that fails silently. task.UserSource = &workflows.XPathBasedUserSource{ - XPath: n.Targeting.XPath, + XPath: resolveXPathMemberNames(n.Targeting.XPath, "System.User"), } case "group_microflow": task.UserSource = &workflows.MicroflowGroupSource{ @@ -532,7 +538,7 @@ func buildUserTask(n *ast.WorkflowUserTaskNode) *workflows.UserTask { } case "group_xpath": task.UserSource = &workflows.XPathGroupSource{ - XPath: n.Targeting.XPath, + XPath: resolveXPathMemberNames(n.Targeting.XPath, "System.WorkflowGroup"), } } diff --git a/mdl/executor/executor.go b/mdl/executor/executor.go index 3f5442ca7..1b898d3be 100644 --- a/mdl/executor/executor.go +++ b/mdl/executor/executor.go @@ -366,7 +366,7 @@ func (e *Executor) Execute(stmt ast.Statement) error { } // ExecuteProgram runs all statements in a program. -func (e *Executor) ExecuteProgram(prog *ast.Program) error { +func (e *Executor) ExecuteProgram(prog *ast.Program) (err error) { if e.beginTally() { defer e.flushTally() } @@ -379,12 +379,30 @@ func (e *Executor) ExecuteProgram(prog *ast.Program) error { // Track which names have been created so far. created := newScriptContext() + // A run of access-rule statements is written once, at its end (#872). The + // deferred end also covers a statement that fails mid-run: what the run's + // earlier statements did still lands, as it did when each wrote itself. If + // it cannot land, that is returned with the statement's error — those + // statements already reported success. + var rules accessRuleRun + defer func() { + if ferr := rules.end(); ferr != nil { + err = errors.Join(err, ferr) + } + }() + for _, stmt := range prog.Statements { + if err := rules.step(e, stmt); err != nil { + return err + } if err := e.Execute(stmt); err != nil { return annotateForwardRef(err, stmt, created, allDefined) } created.collectSingle(stmt) } + if err := rules.end(); err != nil { + return err + } return e.finalizeProgramExecution() } @@ -424,9 +442,15 @@ func (e *Executor) ExecuteProgramContinueOnError(prog *ast.Program, w io.Writer) allDefined.collectDefinitions(prog) created := newScriptContext() + var rules accessRuleRun // see ExecuteProgram + defer func() { _ = rules.end() }() + var res ExecuteProgramResult for i, stmt := range prog.Statements { res.Total++ + if err := rules.step(e, stmt); err != nil { + return res, err + } if err := e.Execute(stmt); err != nil { if errors.Is(err, ErrExit) { return res, err @@ -438,6 +462,9 @@ func (e *Executor) ExecuteProgramContinueOnError(prog *ast.Program, w io.Writer) res.Succeeded++ created.collectSingle(stmt) } + if err := rules.end(); err != nil { + return res, err + } if err := e.finalizeProgramExecution(); err != nil { return res, err } diff --git a/mdl/executor/flow_built_match.go b/mdl/executor/flow_built_match.go index 7022e284c..d98ee5da5 100644 --- a/mdl/executor/flow_built_match.go +++ b/mdl/executor/flow_built_match.go @@ -3,12 +3,14 @@ package executor import ( + "bytes" "fmt" "reflect" "sort" "github.com/mendixlabs/mxcli/model" "github.com/mendixlabs/mxcli/sdk/microflows" + "go.mongodb.org/mongo-driver/bson" ) // # The declared flow, built, against the stored one (ako/mxcli#859) @@ -190,6 +192,7 @@ var ( idType = reflect.TypeOf(model.ID("")) baseElementType = reflect.TypeOf(model.BaseElement{}) collectionType = reflect.TypeOf(microflows.MicroflowObjectCollection{}) + rawBSONType = reflect.TypeOf([]byte(nil)) ) // value compares two values of the model exactly, except that an element's own @@ -234,6 +237,12 @@ func (c *builtComparer) value(b, s reflect.Value, path string) bool { } return true case reflect.Slice, reflect.Array: + if b.Type() == rawBSONType { + if !sameRawModuloIDs(b.Bytes(), s.Bytes()) { + return c.fail("%s: the raw documents differ", path) + } + return true + } if b.Len() != s.Len() { return c.fail("%s: %d built, %d stored", path, b.Len(), s.Len()) } @@ -271,3 +280,45 @@ func (c *builtComparer) value(b, s reflect.Value, path string) bool { return true } } + +// sameRawModuloIDs compares two raw BSON documents — the one a call web +// service activity keeps when the structured form cannot reproduce it — with +// every element's own $ID set aside, at any depth. Like the objects around it, +// each build mints its own, so comparing the bytes made two builds of one +// statement a difference on every run (ako/mxcli#861). Anything that is not a +// BSON document is compared as bytes. +func sameRawModuloIDs(b, s []byte) bool { + if bytes.Equal(b, s) { + return true + } + var bd, sd bson.D + if bson.Unmarshal(b, &bd) != nil || bson.Unmarshal(s, &sd) != nil { + return false + } + bn, berr := bson.Marshal(withoutIDs(bd)) + sn, serr := bson.Marshal(withoutIDs(sd)) + return berr == nil && serr == nil && bytes.Equal(bn, sn) +} + +// withoutIDs is v with the $ID key dropped from every document in it. +func withoutIDs(v any) any { + switch x := v.(type) { + case bson.D: + out := make(bson.D, 0, len(x)) + for _, e := range x { + if e.Key == "$ID" { + continue + } + out = append(out, bson.E{Key: e.Key, Value: withoutIDs(e.Value)}) + } + return out + case bson.A: + out := make(bson.A, len(x)) + for i, e := range x { + out[i] = withoutIDs(e) + } + return out + default: + return v + } +} diff --git a/mdl/executor/flow_built_match_test.go b/mdl/executor/flow_built_match_test.go index fc7fac191..bcfda0f72 100644 --- a/mdl/executor/flow_built_match_test.go +++ b/mdl/executor/flow_built_match_test.go @@ -3,6 +3,7 @@ package executor import ( + "go.mongodb.org/mongo-driver/bson" "strings" "testing" @@ -104,3 +105,52 @@ func TestSameBuiltFlow_CollidingPositionsNeverMatch(t *testing.T) { t.Fatalf("same=%v why=%q, want a refusal to pair colliding nodes", same, why) } } + +// soapGraph is guardGraph with a call web service activity whose raw document +// carries the element IDs of its own build, as the reader keeps it whenever the +// call is not one the structured form reproduces. +func soapGraph(t *testing.T, prefix, timeout string) *microflows.MicroflowObjectCollection { + t.Helper() + oc := guardGraph(prefix) + raw, err := bson.Marshal(bson.D{ + {Key: "$ID", Value: prefix + "action"}, + {Key: "$Type", Value: "Microflows$CallWebServiceAction"}, + {Key: "NewResultHandling", Value: bson.D{ + {Key: "$ID", Value: prefix + "rh"}, + {Key: "$Type", Value: "Microflows$ResultHandling"}, + {Key: "VariableType", Value: bson.D{{Key: "$ID", Value: prefix + "vt"}, {Key: "$Type", Value: "DataTypes$VoidType"}}}, + }}, + {Key: "RequestHeaderHandling", Value: bson.D{ + {Key: "$ID", Value: prefix + "hh"}, + {Key: "ParameterMappings", Value: bson.A{int32(2), bson.D{{Key: "$ID", Value: prefix + "pm"}, {Key: "Expression", Value: "1"}}}}, + }}, + {Key: "TimeOutExpression", Value: timeout}, + }) + if err != nil { + t.Fatal(err) + } + oc.Objects = append(oc.Objects, µflows.ActionActivity{ + BaseActivity: microflows.BaseActivity{BaseMicroflowObject: microflows.BaseMicroflowObject{ + BaseElement: model.BaseElement{ID: model.ID(prefix + "act")}, Position: model.Point{X: 700, Y: 200}}}, + Action: µflows.WebServiceCallAction{BaseElement: model.BaseElement{ID: model.ID(prefix + "action")}, + TimeoutExpression: timeout, RawBSON: raw}, + }) + return oc +} + +// A call web service activity's raw document holds the element IDs its build +// minted, so two builds of one statement never had equal bytes, and every +// re-run re-spliced the call (ako/mxcli#861). The IDs are no difference; the +// document's content is. +func TestSameBuiltFlow_WebServiceRawIDsAreNotADifference(t *testing.T) { + if same, why := sameBuiltFlow(soapGraph(t, "b-", "300"), soapGraph(t, "s-", "300")); !same { + t.Fatalf("two builds of the same call web service differ: %s", why) + } + // Control: a changed value inside the raw document is a difference. + built, stored := soapGraph(t, "b-", "300"), soapGraph(t, "s-", "300") + act := built.Objects[len(built.Objects)-1].(*microflows.ActionActivity).Action.(*microflows.WebServiceCallAction) + act.RawBSON = soapGraph(t, "b-", "30").Objects[4].(*microflows.ActionActivity).Action.(*microflows.WebServiceCallAction).RawBSON + if same, why := sameBuiltFlow(built, stored); same || !strings.Contains(why, "RawBSON") { + t.Fatalf("same=%v why=%q: a changed raw call compared as the stored one", same, why) + } +} diff --git a/mdl/executor/flow_commit_events.go b/mdl/executor/flow_commit_events.go new file mode 100644 index 000000000..f926353dc --- /dev/null +++ b/mdl/executor/flow_commit_events.go @@ -0,0 +1,103 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + + "github.com/mendixlabs/mxcli/mdl/backend" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// StoredCommitEvents answers, for `mxcli fmt --upgrade -p app.mpr`, what each +// Commit activity of a stored flow does with events (ako/mxcli#873): a bare +// `commit $X;` means WITH events since #895, and a flow an older mxcli stored +// from the same script commits without them. The upgrade pins the stored flag +// onto the script so that re-running it keeps what is stored. +type StoredCommitEvents struct { + b backend.FullBackend +} + +// NewStoredCommitEvents reads stored flows from a connected backend. +func NewStoredCommitEvents(b backend.FullBackend) *StoredCommitEvents { + return &StoredCommitEvents{b: b} +} + +// CommitEvents returns, per committed variable, the WithEvents flag of each +// Commit activity in the stored microflow (the nanoflow when nanoflow is set) +// named qualifiedName, loop bodies included. found is false when the project +// has no such flow. +func (s *StoredCommitEvents) CommitEvents(nanoflow bool, qualifiedName string) (map[string][]bool, bool) { + objs, found := s.flowObjects(nanoflow, qualifiedName) + if !found { + return nil, false + } + events := map[string][]bool{} + var walk func(*microflows.MicroflowObjectCollection) + walk = func(oc *microflows.MicroflowObjectCollection) { + if oc == nil { + return + } + for _, o := range oc.Objects { + switch a := o.(type) { + case *microflows.ActionActivity: + if c, ok := a.Action.(*microflows.CommitObjectsAction); ok { + events[c.CommitVariable] = append(events[c.CommitVariable], c.WithEvents) + } + case *microflows.LoopedActivity: + walk(a.ObjectCollection) + } + } + } + walk(objs) + return events, true +} + +// flowObjects reads the stored flow's object collection. +func (s *StoredCommitEvents) flowObjects(nanoflow bool, qualifiedName string) (*microflows.MicroflowObjectCollection, bool) { + kind := "microflow" + if nanoflow { + kind = "nanoflow" + } + if raw, err := s.b.GetRawUnitByName(kind, qualifiedName); err == nil && raw != nil && len(raw.Contents) > 0 { + if mf, err := s.b.ParseMicroflowBSON(raw.Contents, model.ID(raw.ID), ""); err == nil && mf != nil { + return mf.ObjectCollection, true + } + } + // The lookup by name failed: find the flow by its module and name. + moduleName, name, ok := strings.Cut(qualifiedName, ".") + if !ok { + return nil, false + } + module, err := s.b.GetModuleByName(moduleName) + if err != nil || module == nil { + return nil, false + } + h, err := NewContainerHierarchyFromBackend(s.b) + if err != nil { + return nil, false + } + if nanoflow { + nfs, err := s.b.ListNanoflows() + if err != nil { + return nil, false + } + for _, nf := range nfs { + if nf != nil && nf.Name == name && h.FindModuleID(nf.ContainerID) == module.ID { + return nf.ObjectCollection, true + } + } + return nil, false + } + mfs, err := s.b.ListMicroflows() + if err != nil { + return nil, false + } + for _, mf := range mfs { + if mf != nil && mf.Name == name && h.FindModuleID(mf.ContainerID) == module.ID { + return mf.ObjectCollection, true + } + } + return nil, false +} diff --git a/mdl/executor/flow_declared_match.go b/mdl/executor/flow_declared_match.go index d94c7b68b..fed008ede 100644 --- a/mdl/executor/flow_declared_match.go +++ b/mdl/executor/flow_declared_match.go @@ -90,6 +90,13 @@ func matchValue(d, s reflect.Value, mode matchMode) bool { } switch d.Kind() { case reflect.Pointer: + if d.Type() == errorHandlingPtrType { + // `on error rollback` is what an activity with no clause stores, + // so describe never prints it (formatErrorHandlingSuffix): the + // stored side never has it, and a declared one that states it is + // the same activity (ako/mxcli#859). + d, s = withoutDefaultErrorHandling(d), withoutDefaultErrorHandling(s) + } if d.IsNil() && s.IsNil() { return true } @@ -132,7 +139,7 @@ func matchValue(d, s reflect.Value, mode matchMode) bool { continue } if d.Type() == retrieveStructType && name == "Where" { - if !sameStoredConstraint(df, s.Field(i)) { + if !sameStoredConstraint(df, s.Field(i), retrieveEntity(d), retrieveEntity(s)) { return false } continue @@ -171,6 +178,21 @@ func matchValue(d, s reflect.Value, mode matchMode) bool { } } +var errorHandlingPtrType = reflect.TypeOf((*ast.ErrorHandlingClause)(nil)) + +// withoutDefaultErrorHandling returns v, an *ast.ErrorHandlingClause, as nil +// when it is a bare `on error rollback`. In a microflow that is the stored +// default; in a nanoflow, whose default is Abort, describe prints neither +// (both emit nothing), so no comparison with a described flow can tell them +// apart either way. +func withoutDefaultErrorHandling(v reflect.Value) reflect.Value { + if eh, ok := v.Interface().(*ast.ErrorHandlingClause); ok && eh != nil && + eh.Type == ast.ErrorHandlingRollback && len(eh.Body) == 0 { + return reflect.Zero(v.Type()) + } + return v +} + // reflectElem returns the struct a statement pointer points at, or the zero // Value when it is not a pointer to a struct. func reflectElem(v any) reflect.Value { @@ -194,10 +216,22 @@ func reflectElem(v any) reflect.Value { // out over several lines itself (FormatXPathConstraint), and a line break // between two tokens states nothing. Whitespace inside a string literal is // data and is compared as written. -func sameStoredConstraint(d, s reflect.Value) bool { +// +// Each side is resolved against its own retrieve's entity, as the writer does, +// so `[M.Emp.Name = 'y']` is the `Name = 'y'` describe prints for what it +// stored (ako/mxcli#874). +func sameStoredConstraint(d, s reflect.Value, dEntity, sEntity string) bool { de, _ := d.Interface().(ast.Expression) se, _ := s.Interface().(ast.Expression) - return xpathTokens(retrieveXPathConstraint(de)) == xpathTokens(retrieveXPathConstraint(se)) + return xpathTokens(retrieveXPathConstraint(de, dEntity)) == xpathTokens(retrieveXPathConstraint(se, sEntity)) +} + +// retrieveEntity is the entity a retrieve statement's struct value reads from. +func retrieveEntity(retrieve reflect.Value) string { + if q, ok := retrieve.FieldByName("Source").Interface().(ast.QualifiedName); ok { + return q.String() + } + return "" } // xpathTokens is an XPath constraint with the whitespace between its tokens diff --git a/mdl/executor/flow_modify_notes_test.go b/mdl/executor/flow_modify_notes_test.go new file mode 100644 index 000000000..42ef01dc7 --- /dev/null +++ b/mdl/executor/flow_modify_notes_test.go @@ -0,0 +1,75 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "errors" + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" +) + +// ako/mxcli#859: describe never prints `on error rollback` — it is what an +// activity with no clause stores — so a statement stating it must match the +// stored statement without it. Any other clause is still a difference (the +// controls), and so is a custom handler that happens to roll back. +func TestDeclaredMatches_OnErrorRollbackIsTheDescribedDefault(t *testing.T) { + stored := parseFlowBody(t, ` commit $In;`) + for body, want := range map[string]bool{ + ` commit $In on error rollback;`: true, + ` commit $In;`: true, + ` commit $In on error continue;`: false, + " commit $In on error begin\n log info node 'N' 'x';\n end error;": false, + } { + if got := declaredMatches(parseFlowBody(t, body).Body, stored.Body); got != want { + t.Errorf("%q: declaredMatches = %v, want %v", body, got, want) + } + } +} + +// ako/mxcli#859 (rehearsal M3): a replaced statement whose notes differ from +// the stored activity's keeps its own notes, for the builder to draw, and asks +// for the stored ones to go; one with the stored notes has them taken off (the +// splice keeps the stored ones). A note shared with another activity (it has +// an id) cannot be changed from one of them. +func TestKeepStoredNotes(t *testing.T) { + stmt := func(body string) ast.MicroflowStatement { + t.Helper() + return parseFlowBody(t, body).Body[0] + } + notes := func(st ast.MicroflowStatement) int { + if ann := statementAnnotations(st); ann != nil { + return len(ann.Notes) + } + return 0 + } + stored := stmt(" @annotation 'Old.'\n log info node 'N' 'x';") + cases := []struct { + name, body string + wantReplace bool + wantNotes int + }{ + {"the same note", " @annotation 'Old.'\n log info node 'N' 'y';", false, 0}, + {"reworded", " @annotation 'New.'\n log info node 'N' 'y';", true, 1}, + {"one added", " @annotation 'Old.'\n @annotation 'Added.'\n log info node 'N' 'y';", true, 2}, + {"taken off", " log info node 'N' 'y';", true, 0}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + out, replace, err := keepStoredNotes(stored, []ast.MicroflowStatement{stmt(c.body)}) + if err != nil { + t.Fatalf("keepStoredNotes: %v", err) + } + if replace != c.wantReplace || notes(out[0]) != c.wantNotes { + t.Errorf("replace %v with %d notes, want %v with %d", replace, notes(out[0]), c.wantReplace, c.wantNotes) + } + }) + } + shared := stmt(" @annotation(id: n1, text: 'Shared.')\n log info node 'N' 'x';") + _, _, err := keepStoredNotes(shared, []ast.MicroflowStatement{stmt(" @annotation 'Mine.'\n log info node 'N' 'x';")}) + var why *notSpliceable + if !errors.As(err, &why) || !strings.Contains(err.Error(), "shared") { + t.Errorf("re-annotating an activity whose note is shared: got %v, want a splice refusal", err) + } +} diff --git a/mdl/executor/flow_splice_member_qualified_test.go b/mdl/executor/flow_splice_member_qualified_test.go new file mode 100644 index 000000000..b7a2a550d --- /dev/null +++ b/mdl/executor/flow_splice_member_qualified_test.go @@ -0,0 +1,171 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/backend/mfmutator" + "github.com/mendixlabs/mxcli/mdl/backend/mock" + "github.com/mendixlabs/mxcli/mdl/types" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/domainmodel" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// ako/mxcli#885: a fragment spliced into a stored flow was built knowing only +// the types of the stored flow's parameters, declared variables and created +// objects, so a member of a variable bound by a retrieve or a microflow call +// was written bare — `Name`, not `Mod.Item.Name` — and Mendix cannot load the +// project. These tests build fragments the way the splice does. + +// spliceFixture is a stored flow over Mod.Item (attribute Name) with a +// parameter $P, a database retrieve $R (first), a call $C of Mod.GetItem +// (returns Mod.Item), a call $Cs of Mod.GetItems (returns a list) and a retrieve +// over an association $A, whose entity the stored flow does not say. +func spliceFixture(t *testing.T) (*ExecContext, *alterFlowContext) { + t.Helper() + mod := mkModule("Mod") + item := mkEntity(mod.ID, "Item") + item.Attributes = []*domainmodel.Attribute{{BaseElement: model.BaseElement{ID: nextID("attr")}, Name: "Name"}} + dm := mkDomainModel(mod.ID, item) + returns := map[string]microflows.DataType{ + "Mod.GetItem": µflows.ObjectType{EntityQualifiedName: "Mod.Item"}, + "Mod.GetItems": µflows.ListType{EntityQualifiedName: "Mod.Item"}, + } + mb := &mock.MockBackend{ + IsConnectedFunc: func() bool { return true }, + GetModuleByNameFunc: func(name string) (*model.Module, error) { + if name == "Mod" { + return mod, nil + } + return nil, nil + }, + GetDomainModelFunc: func(model.ID) (*domainmodel.DomainModel, error) { return dm, nil }, + GetRawUnitByNameFunc: func(_, qn string) (*types.RawUnitInfo, error) { + if _, ok := returns[qn]; ok { + return &types.RawUnitInfo{ID: qn, QualifiedName: qn, Contents: []byte(qn)}, nil + } + return nil, nil + }, + ParseMicroflowBSONFunc: func(contents []byte, _, _ model.ID) (*microflows.Microflow, error) { + return µflows.Microflow{ReturnType: returns[string(contents)]}, nil + }, + } + ctx, _ := newMockCtx(t, withBackend(mb), withHierarchy(mkHierarchy(mod))) + + activity := func(a microflows.MicroflowAction) microflows.MicroflowObject { + return µflows.ActionActivity{Action: a} + } + call := func(qn string) microflows.MicroflowAction { + return µflows.MicroflowCallAction{MicroflowCall: µflows.MicroflowCall{Microflow: qn}} + } + a := &alterFlowContext{ + stmt: &ast.AlterFlowStmt{}, + mf: µflows.Microflow{Parameters: []*microflows.MicroflowParameter{ + {Name: "P", Type: µflows.ObjectType{EntityQualifiedName: "Mod.Item"}}, + }}, + entityNames: map[model.ID]string{item.ID: "Mod.Item"}, + cands: []mfmutator.Candidate{ + {OutputVariable: "R", Object: activity(µflows.RetrieveAction{Source: µflows.DatabaseRetrieveSource{ + EntityID: item.ID, Range: µflows.Range{RangeType: microflows.RangeTypeFirst}}})}, + {OutputVariable: "C", Object: activity(call("Mod.GetItem"))}, + {OutputVariable: "Cs", Object: activity(call("Mod.GetItems"))}, + {OutputVariable: "A", Object: activity(µflows.RetrieveAction{Source: µflows.AssociationRetrieveSource{ + StartVariable: "P", AssociationQualifiedName: "Mod.Item_Item"}})}, + }, + } + return ctx, a +} + +func changeName(variable string) *ast.ChangeObjectStmt { + return &ast.ChangeObjectStmt{Variable: variable, Changes: []ast.ChangeItem{ + {Attribute: "Name", Value: &ast.LiteralExpr{Value: "x", Kind: ast.LiteralString}}, + }} +} + +// changedAttribute builds a one-change fragment and returns the attribute it +// writes, or the build error. +func changedAttribute(t *testing.T, ctx *ExecContext, a *alterFlowContext, variable string) (string, error) { + t.Helper() + frag, err := a.buildFragment(ctx, []ast.MicroflowStatement{changeName(variable)}) + if err != nil { + return "", err + } + for _, obj := range frag.Objects { + if act, ok := obj.(*microflows.ActionActivity); ok { + if ch, ok := act.Action.(*microflows.ChangeObjectAction); ok && len(ch.Changes) == 1 { + return ch.Changes[0].AttributeQualifiedName, nil + } + } + } + t.Fatalf("the fragment of change $%s has no change activity", variable) + return "", nil +} + +func TestSpliceFragment_ChangeMemberIsQualified(t *testing.T) { + ctx, a := spliceFixture(t) + for _, v := range []string{ + "P", // control: a parameter was always typed + "R", // a database retrieve + "C", // a microflow call + } { + got, err := changedAttribute(t, ctx, a, v) + if err != nil { + t.Errorf("change $%s: %v", v, err) + continue + } + if got != "Mod.Item.Name" { + t.Errorf("change $%s writes attribute %q, want Mod.Item.Name", v, got) + } + } +} + +// A variable the stored flow does not type — a retrieve over an association — +// is typed by the declared flow's full build when the statement is a +// `create or modify`, and refused, never written bare, when nothing types it. +func TestSpliceFragment_UntypedVariable(t *testing.T) { + ctx, a := spliceFixture(t) + if _, err := changedAttribute(t, ctx, a, "A"); err == nil || !strings.Contains(err.Error(), `cannot qualify member "Name"`) { + t.Fatalf("change on an untyped variable: want a refusal naming the member, got %v", err) + } + + a.declaredVarTypes = map[string]string{"A": "Mod.Item"} + got, err := changedAttribute(t, ctx, a, "A") + if err != nil || got != "Mod.Item.Name" { + t.Fatalf("change on a variable the declared flow types: got %q, %v; want Mod.Item.Name", got, err) + } +} + +// The other activities that name a member of a list variable's entity refuse +// one they cannot qualify instead of writing it bare (a sort), as a find by +// expression (a find), or not at all (an aggregate). Control: over the typed +// list $Cs each builds. +func TestSpliceFragment_ListMembersAreRefusedWhenUntyped(t *testing.T) { + ctx, a := spliceFixture(t) + stmts := func(list string) map[string]ast.MicroflowStatement { + return map[string]ast.MicroflowStatement{ + "sort": &ast.ListOperationStmt{OutputVariable: "S", Operation: ast.ListOpSort, InputVariable: list, + SortSpecs: []ast.SortSpec{{Attribute: "Name", Ascending: true}}}, + "find": &ast.ListOperationStmt{OutputVariable: "F", Operation: ast.ListOpFind, InputVariable: list, + Condition: &ast.BinaryExpr{Left: &ast.IdentifierExpr{Name: "Name"}, Operator: "=", + Right: &ast.LiteralExpr{Value: "x", Kind: ast.LiteralString}}}, + "aggregate": &ast.AggregateListStmt{OutputVariable: "M", Operation: ast.AggregateMaximum, InputVariable: list, + Attribute: "Name"}, + } + } + for name, st := range stmts("Cs") { + if _, err := a.buildFragment(ctx, []ast.MicroflowStatement{st}); err != nil { + t.Errorf("%s over a typed list: %v", name, err) + } + } + a.cands = append(a.cands, mfmutator.Candidate{OutputVariable: "As", Object: µflows.ActionActivity{ + Action: µflows.RetrieveAction{Source: µflows.AssociationRetrieveSource{StartVariable: "P"}}}}) + for name, st := range stmts("As") { + if _, err := a.buildFragment(ctx, []ast.MicroflowStatement{st}); err == nil || !strings.Contains(err.Error(), "cannot qualify member") { + t.Errorf("%s over an untyped list: want a refusal, got %v", name, err) + } + } +} diff --git a/mdl/executor/recursive_flow_test.go b/mdl/executor/recursive_flow_test.go new file mode 100644 index 000000000..d8a2768e3 --- /dev/null +++ b/mdl/executor/recursive_flow_test.go @@ -0,0 +1,126 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/backend/mock" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// ako/mxcli#843: a flow that calls itself is created in one statement. The +// call target was resolved against the project only, where the flow being +// created does not exist yet, so exec refused "microflow not found" while +// check passed the same script — and the stub-then-real workaround is refused +// by the mdl 1 splice. + +const recursiveModuleID = model.ID("module-1") + +func findCall[T any](objs []microflows.MicroflowObject) (T, bool) { + var zero T + for _, o := range objs { + if a, ok := o.(*microflows.ActionActivity); ok { + if c, ok := a.Action.(T); ok { + return c, true + } + } + } + return zero, false +} + +func TestCreateMicroflow_CallsItself(t *testing.T) { + ctx, written := microflowWriteProbe(t, nil, recursiveModuleID) + stmt := firstStatement[*ast.CreateMicroflowStmt](t, `create or modify microflow MyModule.Countdown ($N: Integer) +returns Boolean as $Done +begin + if $N <= 0 then + return true; + end if; + $Below = call microflow MyModule.Countdown(N = $N - 1); + return $Below; +end;`) + if err := execCreateMicroflow(ctx, stmt); err != nil { + t.Fatalf("a self-recursive microflow is refused: %v", err) + } + if *written == nil { + t.Fatal("no microflow was written") + } + call, ok := findCall[*microflows.MicroflowCallAction]((*written).ObjectCollection.Objects) + if !ok { + t.Fatal("the written microflow has no call activity") + } + if got := call.MicroflowCall.Microflow; got != "MyModule.Countdown" { + t.Errorf("the call targets %q, want the microflow itself", got) + } +} + +// The control: the self-name is the only name the statement adds. A call to a +// microflow that is neither stored nor the one being created is still refused. +func TestCreateMicroflow_CallToAMissingMicroflowStillRefused(t *testing.T) { + ctx, _ := microflowWriteProbe(t, nil, recursiveModuleID) + stmt := firstStatement[*ast.CreateMicroflowStmt](t, `create or modify microflow MyModule.Countdown ($N: Integer) +returns Boolean +begin + $Below = call microflow MyModule.Elsewhere(N = $N - 1); + return $Below; +end;`) + err := execCreateMicroflow(ctx, stmt) + if err == nil || !strings.Contains(err.Error(), "MyModule.Elsewhere") { + t.Fatalf("a call to a missing microflow must still be refused, got %v", err) + } +} + +// A nanoflow cannot call a microflow of its own name: the self-name resolves +// only for the same kind of flow. +func TestCreateMicroflow_SelfNameIsNotANanoflow(t *testing.T) { + var written *microflows.Nanoflow + mb := &mock.MockBackend{ + IsConnectedFunc: func() bool { return true }, + ListModulesFunc: func() ([]*model.Module, error) { + return []*model.Module{{BaseElement: model.BaseElement{ID: recursiveModuleID}, Name: "MyModule"}}, nil + }, + GetModuleByNameFunc: func(name string) (*model.Module, error) { + if name != "MyModule" { + return nil, nil + } + return &model.Module{BaseElement: model.BaseElement{ID: recursiveModuleID}, Name: "MyModule"}, nil + }, + ListMicroflowsFunc: func() ([]*microflows.Microflow, error) { return nil, nil }, + ListNanoflowsFunc: func() ([]*microflows.Nanoflow, error) { return nil, nil }, + CreateNanoflowFunc: func(nf *microflows.Nanoflow) error { written = nf; return nil }, + UpdateNanoflowFunc: func(nf *microflows.Nanoflow) error { written = nf; return nil }, + } + ctx, _ := newMockCtx(t, withBackend(mb)) + + self := firstStatement[*ast.CreateNanoflowStmt](t, `create or modify nanoflow MyModule.CountdownNf ($N: Integer) +returns Boolean as $Done +begin + if $N <= 0 then + return true; + end if; + $Below = call nanoflow MyModule.CountdownNf(N = $N - 1); + return $Below; +end;`) + if err := execCreateNanoflow(ctx, self); err != nil { + t.Fatalf("a self-recursive nanoflow is refused: %v", err) + } + if written == nil { + t.Fatal("no nanoflow was written") + } + call, ok := findCall[*microflows.NanoflowCallAction](written.ObjectCollection.Objects) + if !ok || call.NanoflowCall.Nanoflow != "MyModule.CountdownNf" { + t.Fatalf("the nanoflow does not call itself: %+v", call) + } + + cross := firstStatement[*ast.CreateNanoflowStmt](t, `create or modify nanoflow MyModule.CountdownNf ($N: Integer) +begin + call microflow MyModule.CountdownNf(N = $N - 1); +end;`) + if err := execCreateNanoflow(ctx, cross); err == nil || !strings.Contains(err.Error(), "microflow not found") { + t.Fatalf("a nanoflow calling a microflow of its own name must be refused, got %v", err) + } +} diff --git a/mdl/executor/validate_widget_member_refs.go b/mdl/executor/validate_widget_member_refs.go index 2d115a488..2bc8a3d9c 100644 --- a/mdl/executor/validate_widget_member_refs.go +++ b/mdl/executor/validate_widget_member_refs.go @@ -185,7 +185,9 @@ func validateXPathMembers(ctx *ExecContext, prog *ast.Program) []error { if entityQN == "" || !strings.Contains(entityQN, ".") { return } - for _, bad := range unresolvableXPathSteps(ctx, m, ds.Where, entityQN) { + // Checked as it is stored: a qualified attribute of the entity is + // the attribute, not a step to resolve (ako/mxcli#874). + for _, bad := range unresolvableXPathSteps(ctx, m, storedXPathConstraint(ds.Where, entityQN), entityQN) { errs = append(errs, mdlerrors.NewValidation(fmt.Sprintf( "%s: the constraint on %s names %q, which is neither an attribute nor an "+ "association of it — mxbuild reports this as CE1613 \"The selected %s "+ diff --git a/mdl/executor/validate_widget_member_refs_test.go b/mdl/executor/validate_widget_member_refs_test.go index f59e40241..798d34498 100644 --- a/mdl/executor/validate_widget_member_refs_test.go +++ b/mdl/executor/validate_widget_member_refs_test.go @@ -195,6 +195,10 @@ func TestValidateXPathMembers_AcceptsWhatResolves(t *testing.T) { {"an association hop, then the target's attribute", "Shop.Order", "[Shop.Order_Customer/Shop.Customer/Name = 'x']"}, {"no constraint at all", "Shop.Order", ""}, + // ako/mxcli#874: stored as `Status`, so it is checked as `Status`. + // Checked as written it was a step that resolves to nothing, and the + // re-run of a script that had just created the page was refused. + {"an attribute qualified with the entity", "Shop.Order", "[Shop.Order.Status = 'Open']"}, } { t.Run(tc.name, func(t *testing.T) { if errs := validateXPathMembers(ctx, pageWith( diff --git a/mdl/executor/xpath_member_names.go b/mdl/executor/xpath_member_names.go new file mode 100644 index 000000000..2be8e5721 --- /dev/null +++ b/mdl/executor/xpath_member_names.go @@ -0,0 +1,155 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "regexp" + "strings" +) + +// A three-part name in an XPath constraint is one of two things, and what it +// is decides what is stored (ako/mxcli#874): +// +// - Module.Entity.Attribute — an attribute of the entity the constraint is +// evaluated on. XPath names an attribute bare, so it is stored as +// `Attribute`; Studio Pro reports the qualified spelling as CE0161. +// - Module.Enumeration.Value — an enumeration value. XPath compares an +// enumeration attribute with the value's name as a string, so it is stored +// as `'Value'` (normalizeXPathEnumRefs). +// +// Every writer used to take the second reading for both, so a qualified +// attribute became a string literal and `[M.Emp.Name = 'y']` compared two +// constants: a constraint that passes every check and filters nothing. +// +// Which entity a name is qualified with is read off the constraint's context, +// not the model, so both the writer and the comparison create or modify makes +// against describe's form reach the same answer with no project open: the +// constrained entity itself, and every entity named as a step of a path in the +// constraint (a predicate on that step is evaluated on it). A three-part name +// qualified with anything else keeps the enumeration reading. + +// xpathQualifiedNameRe matches a run of dot-joined identifiers; the callers +// look at how many parts it has and what surrounds it. +var xpathQualifiedNameRe = regexp.MustCompile(`[A-Za-z_][A-Za-z0-9_]*(?:\.[A-Za-z_][A-Za-z0-9_]*)+`) + +// storedXPathConstraint is the constraint an XPath writer stores for xpath +// evaluated on entity: qualified attributes bare, enumeration values as string +// literals. String literals are data and are left as written. +func storedXPathConstraint(xpath, entity string) string { + return normalizeXPathEnumRefs(resolveXPathMemberNames(xpath, entity)) +} + +// storedModelXPathConstraint is storedXPathConstraint for a writer that has the +// project in hand, and it places the names storedXPathConstraint has to guess +// at: an attribute qualified with a generalization of entity is the attribute +// too, and a name qualified with any other entity of the model is left as +// written rather than read as an enumeration value. +// +// Guessing there turned a constraint that failed loudly into a silent one. An +// access rule stored a short constraint verbatim, so +// `grant … on Administration.Account … where [System.User.Name = 'x']` was +// CE0161; the enumeration reading made it ['Name' = 'x'], which compares two +// constants and passes mx check (measured, 11.14.0). +// +// Without a project it is storedXPathConstraint. +func storedModelXPathConstraint(ctx *ExecContext, xpath, entity string) string { + if ctx == nil || !ctx.Connected() { + return storedXPathConstraint(xpath, entity) + } + b, _ := ctx.Backend.(entityLookupBackend) + owners, _ := generalizationChain(ctx, entity) + if len(owners) == 0 { + owners = []string{entity} + } + return rewriteXPathNames(resolveXPathMemberNamesOf(xpath, owners), func(name string) string { + if strings.Count(name, ".") != 2 { + return name + } + dot := strings.LastIndex(name, ".") + if _, isEntity := findEntityByQN(b, name[:dot]); isEntity { + return name + } + return enumRefToLiteral(name) + }) +} + +// resolveXPathMemberNames rewrites each Module.Entity.Attribute whose +// Module.Entity is entity, or an entity step of the constraint, to Attribute. +func resolveXPathMemberNames(xpath, entity string) string { + return resolveXPathMemberNamesOf(xpath, []string{entity}) +} + +// resolveXPathMemberNamesOf is resolveXPathMemberNames for a constraint +// evaluated on any of entities (an entity and its generalizations). +func resolveXPathMemberNamesOf(xpath string, entities []string) string { + if !strings.Contains(xpath, ".") { + return xpath + } + owners := map[string]bool{} + for _, entity := range entities { + if entity != "" { + owners[entity] = true + } + } + forEachXPathName(xpath, func(name string) { + if strings.Count(name, ".") == 1 { + owners[name] = true + } + }) + return rewriteXPathNames(xpath, func(name string) string { + if strings.Count(name, ".") != 2 { + return name + } + dot := strings.LastIndex(name, ".") + if owners[name[:dot]] { + return name[dot+1:] + } + return name + }) +} + +// forEachXPathName calls fn with every qualified name outside a string literal. +func forEachXPathName(xpath string, fn func(string)) { + rewriteXPathNames(xpath, func(name string) string { + fn(name) + return name + }) +} + +// rewriteXPathNames replaces each qualified name outside a string literal with +// what fn returns for it. A name glued to a `$` or `%` (a variable member, a +// `[%Token%]`) is not a model name and is passed over. +func rewriteXPathNames(xpath string, fn func(string) string) string { + var b strings.Builder + b.Grow(len(xpath)) + rewrite := func(seg string) { + prev := 0 + for _, loc := range xpathQualifiedNameRe.FindAllStringIndex(seg, -1) { + start, end := loc[0], loc[1] + b.WriteString(seg[prev:start]) + if start > 0 && strings.ContainsRune("$%", rune(seg[start-1])) { + b.WriteString(seg[start:end]) + } else { + b.WriteString(fn(seg[start:end])) + } + prev = end + } + b.WriteString(seg[prev:]) + } + for i := 0; i < len(xpath); { + if xpath[i] != '\'' { + j := strings.IndexByte(xpath[i:], '\'') + if j < 0 { + rewrite(xpath[i:]) + break + } + rewrite(xpath[i : i+j]) + i += j + continue + } + end := xpathLiteralEnd(xpath, i) + b.WriteString(xpath[i:end]) + i = end + } + return b.String() +} diff --git a/mdl/executor/xpath_qualified_member_test.go b/mdl/executor/xpath_qualified_member_test.go new file mode 100644 index 000000000..a56e5b3d6 --- /dev/null +++ b/mdl/executor/xpath_qualified_member_test.go @@ -0,0 +1,223 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/backend/mock" + "github.com/mendixlabs/mxcli/mdl/visitor" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/domainmodel" + "github.com/mendixlabs/mxcli/sdk/microflows" + "github.com/mendixlabs/mxcli/sdk/pages" + "github.com/mendixlabs/mxcli/sdk/workflows" +) + +// ako/mxcli#874. A module-qualified attribute inside an XPath constraint, +// +// retrieve $L from M.Emp where [M.Emp.Name = 'y']; +// +// was stored as ['Name' = 'y']: every XPath writer read a three-part name as an +// enumeration value (M.Enum.Value -> 'Value') without knowing which entity the +// constraint is evaluated on. The stored constraint compares two constants, so +// the retrieve silently returns every row or none, and mxcli check and mx check +// both pass it. Measured on mx check 11.14.0: [Name = 'y'] is clean, +// [M.Emp.Name = 'y'] is CE0161, and so is [Kind = M.Enum.Value] — so the +// attribute is stored bare and an enumeration value stays a string literal. +// +// A silent wrong write, fixed under mdl 0 and mdl 1 alike (ADR-0011). + +// retrieveConstraintOf builds the first retrieve of a microflow from MDL source +// and returns the XPath constraint the writer stores for it. +func retrieveConstraintOf(t *testing.T, src string) string { + t.Helper() + prog, errs := visitor.Build(src) + if len(errs) > 0 { + t.Fatalf("%q: %v", src, errs) + } + for _, s := range prog.Statements { + mf, ok := s.(*ast.CreateMicroflowStmt) + if !ok { + continue + } + r := mf.Body[0].(*ast.RetrieveStmt) + fb := &flowBuilder{varTypes: map[string]string{}} + fb.addRetrieveAction(r) + if len(fb.errors) > 0 { + t.Fatalf("%q: builder errors: %v", src, fb.errors) + } + act := fb.objects[0].(*microflows.ActionActivity).Action.(*microflows.RetrieveAction) + return act.Source.(*microflows.DatabaseRetrieveSource).XPathConstraint + } + t.Fatalf("%q: no microflow", src) + return "" +} + +func TestRetrieveXPath_QualifiedAttributeIsStoredAsTheAttribute(t *testing.T) { + cases := []struct{ name, where, want string }{ + {"bracketed", "[M.Emp.Name = 'y']", "[Name = 'y']"}, + {"expression form", "M.Emp.Name = 'y'", "[Name = 'y']"}, + {"beside an enum value", "M.Emp.Name = 'y' and M.Emp.Kind = M.EmpKind.Staff", + "[Name = 'y' and Kind = 'Staff']"}, + {"bracketed, beside an enum value", "[M.Emp.Kind = M.EmpKind.Staff or M.Emp.Name = 'y']", + "[Kind = 'Staff' or Name = 'y']"}, + // Inside a predicate on a path step the constraint is evaluated on that + // step's entity, so its attributes are qualified with it. + {"nested predicate", "[M.Emp_Dept/M.Dept[M.Dept.Code = 'x']]", "[M.Emp_Dept/M.Dept[Code = 'x']]"}, + {"inside a function", "[contains(M.Emp.Name, 'y')]", "[contains(Name, 'y')]"}, + // CONTROLS: what the fix must not touch. + {"enum value", "[Kind = M.EmpKind.Staff]", "[Kind = 'Staff']"}, + {"enum value, expression form", "Kind = M.EmpKind.Staff", "[Kind = 'Staff']"}, + {"string literal naming the attribute", "[Name = 'M.Emp.Name']", "[Name = 'M.Emp.Name']"}, + {"bare attribute", "[Name = 'y']", "[Name = 'y']"}, + } + for _, header := range []string{"", "mdl 1;\n"} { + for _, tc := range cases { + t.Run(strings.TrimSpace(header+" "+tc.name), func(t *testing.T) { + src := header + "create microflow M.F () begin\n retrieve $L from M.Emp where " + tc.where + ";\nend;" + if got := retrieveConstraintOf(t, src); got != tc.want { + t.Errorf("stored constraint\n got %q\n want %q", got, tc.want) + } + }) + } + } +} + +// The re-run: describe prints the stored constraint with the bare attribute, so +// a script that spells it qualified must still be the same retrieve — otherwise +// create or modify re-splices it on every run (the twice-exec rule). +func TestRetrieveXPath_QualifiedAttributeMatchesItsDescribeForm(t *testing.T) { + parse := func(src string) *ast.RetrieveStmt { + t.Helper() + prog, errs := visitor.Build("mdl 1;\ncreate microflow M.F () begin\n " + src + "\nend;") + if len(errs) > 0 { + t.Fatalf("%q: %v", src, errs) + } + for _, s := range prog.Statements { + if mf, ok := s.(*ast.CreateMicroflowStmt); ok { + return mf.Body[0].(*ast.RetrieveStmt) + } + } + t.Fatalf("%q: no microflow", src) + return nil + } + stored := parse("retrieve $L from M.Emp\n where Name = 'y';") + for _, declared := range []string{ + "retrieve $L from M.Emp where [M.Emp.Name = 'y'];", + "retrieve $L from M.Emp where M.Emp.Name = 'y';", + } { + if !declaredMatches(parse(declared), stored) { + t.Errorf("%s\n does not match its describe form `where Name = 'y'`", declared) + } + } + // CONTROL: a different attribute is a different retrieve. + if declaredMatches(parse("retrieve $L from M.Emp where [M.Emp.Code = 'y'];"), stored) { + t.Error("[M.Emp.Code = 'y'] matched `where Name = 'y'`") + } +} + +// The other three writers take the constraint as text. +func TestStoredXPath_EveryWriterResolvesQualifiedAttributes(t *testing.T) { + t.Run("page datasource", func(t *testing.T) { + pb := &pageBuilder{} + ds := &ast.DataSourceV3{Type: "database", Reference: "M.Emp", Where: "[M.Emp.Name = 'y' and Kind = M.EmpKind.Staff]"} + src := &pages.DatabaseSource{} + if err := pb.applyDatabaseClausesV3(src, ds, "M.Emp"); err != nil { + t.Fatal(err) + } + if want := "[Name = 'y' and Kind = 'Staff']"; src.XPathConstraint != want { + t.Errorf("page datasource constraint\n got %q\n want %q", src.XPathConstraint, want) + } + }) + t.Run("access rule", func(t *testing.T) { + if got, want := accessRuleXPathConstraint(nil, "[M.Emp.Name = 'y']", "M.Emp"), "[Name = 'y']"; got != want { + t.Errorf("access rule constraint\n got %q\n want %q", got, want) + } + }) + t.Run("workflow targeting", func(t *testing.T) { + users := buildUserTask(&ast.WorkflowUserTaskNode{Name: "t", + Targeting: ast.WorkflowTargetingNode{Kind: "xpath", XPath: "[System.User.Name = 'admin']"}}) + if xp, ok := users.UserSource.(*workflows.XPathBasedUserSource); !ok || xp.XPath != "[Name = 'admin']" { + t.Errorf("user targeting = %#v, want XPath [Name = 'admin']", users.UserSource) + } + groups := buildUserTask(&ast.WorkflowUserTaskNode{Name: "t", + Targeting: ast.WorkflowTargetingNode{Kind: "group_xpath", XPath: "[System.WorkflowGroup.Name = 'G']"}}) + if xp, ok := groups.UserSource.(*workflows.XPathGroupSource); !ok || xp.XPath != "[Name = 'G']" { + t.Errorf("group targeting = %#v, want XPath [Name = 'G']", groups.UserSource) + } + }) +} + +// The formatter re-reads a constraint that does not fit on one line and prints +// it again. It must not decide what a three-part name is: reading it as an enum +// value turned a qualified attribute into a string literal on the way (the same +// defect, for long constraints only). +func TestFormatXPathConstraint_KeepsThreePartNames(t *testing.T) { + in := "[M.Emp.Name = 'a long enough value to force a line break' and Kind = M.EmpKind.Staff and Code != empty]" + got := visitor.FormatXPathConstraint(in) + for _, name := range []string{"M.Emp.Name", "M.EmpKind.Staff"} { + if !strings.Contains(got, name) { + t.Errorf("FormatXPathConstraint dropped %s:\n%s", name, got) + } + } +} + +// An access rule's constraint used to be stored verbatim when it was short, so a +// three-part name the writer cannot place — an attribute qualified with a +// generalization of the entity, or with an unrelated entity — failed loudly +// (CE0161). Reading every such name as an enumeration value instead would turn +// that error into ['Name' = 'x']: a constraint that compares two constants and +// passes mx check (measured, 11.14.0, grant on Administration.Account where +// [System.User.Name = 'x']). The writer has the project, so it resolves the +// generalization chain and leaves a name qualified with any other entity as +// written. +func TestAccessRuleXPath_GeneralizationAndForeignEntityNames(t *testing.T) { + sales, admin, system := mkModule("Sales"), mkModule("Administration"), mkModule("System") + account := mkEntity(admin.ID, "Account") + account.GeneralizationRef = "System.User" + dms := map[model.ID]*domainmodel.DomainModel{ + sales.ID: mkDomainModel(sales.ID, mkEntity(sales.ID, "Order")), + admin.ID: mkDomainModel(admin.ID, account), + system.ID: mkDomainModel(system.ID, mkEntity(system.ID, "User")), + } + mods := map[string]*model.Module{"Sales": sales, "Administration": admin, "System": system} + mb := &mock.MockBackend{ + IsConnectedFunc: func() bool { return true }, + GetModuleByNameFunc: func(n string) (*model.Module, error) { return mods[n], nil }, + GetDomainModelFunc: func(id model.ID) (*domainmodel.DomainModel, error) { return dms[id], nil }, + } + ctx, _ := newMockCtx(t, withBackend(mb)) + + for _, tc := range []struct{ in, want string }{ + // Inherited attribute, qualified with the generalization: the attribute. + {"[System.User.Name = 'x']", "[Name = 'x']"}, + // The entity itself, as before. + {"[Administration.Account.Email = 'x']", "[Email = 'x']"}, + // Qualified with an entity that is neither: left as written (CE0161), + // never the literal 'Total'. + {"[Sales.Order.Total = 'x']", "[Sales.Order.Total = 'x']"}, + // Not an entity: an enumeration value, stored as a string literal. + {"[Kind = Sales.Kind.Gold]", "[Kind = 'Gold']"}, + } { + if got := accessRuleXPathConstraint(ctx, tc.in, "Administration.Account"); got != tc.want { + t.Errorf("access rule on Administration.Account, %s\n got %q\n want %q", tc.in, got, tc.want) + } + } +} + +// Workflow targeting has no project in hand when it is built, so it cannot tell +// an enumeration value from an attribute of another entity (the configured +// workflow user entity may be a specialization, e.g. Administration.Account). +// It resolves the names it can place and leaves every other three-part name as +// written — as it always stored them — rather than guessing a string literal. +func TestWorkflowTargetingXPath_LeavesUnplacedNamesAsWritten(t *testing.T) { + task := buildUserTask(&ast.WorkflowUserTaskNode{Name: "t", + Targeting: ast.WorkflowTargetingNode{Kind: "xpath", XPath: "[Administration.Account.IsLocalUser = true]"}}) + xp, ok := task.UserSource.(*workflows.XPathBasedUserSource) + if !ok || xp.XPath != "[Administration.Account.IsLocalUser = true]" { + t.Errorf("user targeting = %#v, want the constraint as written", task.UserSource) + } +} diff --git a/mdl/grammar/domains/MDLDomainModel.g4 b/mdl/grammar/domains/MDLDomainModel.g4 index 0caca74a3..22ce9e74b 100644 --- a/mdl/grammar/domains/MDLDomainModel.g4 +++ b/mdl/grammar/domains/MDLDomainModel.g4 @@ -105,7 +105,7 @@ dataType | DECIMAL_TYPE | BOOLEAN_TYPE | DATETIME_TYPE - | DATE_TYPE + | DATE_TYPE /* @alias MDL-DEPR160 */ // stored as DateTime: Mendix has no date-only type | AUTONUMBER_TYPE | AUTOOWNER_TYPE | AUTOCHANGEDBY_TYPE @@ -137,7 +137,7 @@ nonListDataType | DECIMAL_TYPE | BOOLEAN_TYPE | DATETIME_TYPE - | DATE_TYPE + | DATE_TYPE /* @alias MDL-DEPR160 */ // stored as DateTime: Mendix has no date-only type | AUTONUMBER_TYPE | AUTOOWNER_TYPE | AUTOCHANGEDBY_TYPE diff --git a/mdl/roundtrip/flow_idempotent_shapes_test.go b/mdl/roundtrip/flow_idempotent_shapes_test.go index 9a65c0c3b..da0f7e27d 100644 --- a/mdl/roundtrip/flow_idempotent_shapes_test.go +++ b/mdl/roundtrip/flow_idempotent_shapes_test.go @@ -269,11 +269,15 @@ func TestFlowModify_PropertyEditsAreWritten(t *testing.T) { {"a changed page title override", "show page Administration.Account_Overview with title = 'First';", "show page Administration.Account_Overview with title = 'Second';"}, - // Taking an override out is not seen on main either: neither the - // reader nor describe carries TitleOverride, so no side holds it. {"a page title override added", "show page Administration.Account_Overview;", "show page Administration.Account_Overview with title = 'First';"}, + // Taken out, the override is seen only once describe prints it: + // the statement diff compares against the description + // (ako/mxcli#869). + {"a page title override taken out", + "show page Administration.Account_Overview with title = 'First';", + "show page Administration.Account_Overview;"}, } { name := "mdl 0" if header != "" { @@ -304,3 +308,50 @@ func TestFlowModify_PropertyEditsAreWritten(t *testing.T) { } } } + +// A call web service activity the structured form does not reproduce keeps its +// raw document, which carries the element IDs of its own build: re-running the +// statement unchanged re-spliced it on every run (ako/mxcli#861). A dangling +// receive mapping is such a call — its result type stays Void. The control: a +// changed timeout is still written. +func TestFlowModify_WebServiceCallRerunsClean(t *testing.T) { + h := newHarness(t) + defer h.close() + + const setup = "create module SampleSOAP;\ncreate entity SampleSOAP.OrderResponse (Status : string(50));\n" + const flow = `create or modify microflow SampleSOAP.Idem_Soap () +returns SampleSOAP.OrderResponse as $Root +begin + $Root = call web service SampleSOAP.OrderService + operation FetchSampleItems + receive mapping SampleSOAP.OrderResponse + timeout %s; + return $Root; +end; +` + for _, header := range []string{"mdl 1;\n", ""} { + name := "mdl 0" + if header != "" { + name = "mdl 1" + } + t.Run(name, func(t *testing.T) { + h.restore() + if err := h.exec(header + setup + fmt.Sprintf(flow, "30")); err != nil { + t.Fatalf("create: %v\n%s", err, h.out.String()) + } + before := h.snapshot() + if err := h.exec(header + fmt.Sprintf(flow, "30")); err != nil { + t.Fatalf("re-run: %v\n%s", err, h.out.String()) + } + if changed := before.diff(h.snapshot()); len(changed) != 0 { + t.Errorf("an identical re-run wrote:\n %s\n%s", strings.Join(changed, "\n "), h.out.String()) + } + if err := h.exec(header + fmt.Sprintf(flow, "60")); err != nil { + t.Fatalf("the edit was refused: %v\n%s", err, h.out.String()) + } + if strings.Contains(h.out.String(), "Unchanged ") || len(before.diff(h.snapshot())) == 0 { + t.Errorf("a changed timeout wrote nothing:\n%s", h.out.String()) + } + }) + } +} diff --git a/mdl/roundtrip/flow_modify_notes_test.go b/mdl/roundtrip/flow_modify_notes_test.go new file mode 100644 index 000000000..c6d47b168 --- /dev/null +++ b/mdl/roundtrip/flow_modify_notes_test.go @@ -0,0 +1,256 @@ +// SPDX-License-Identifier: Apache-2.0 + +//go:build integration + +package roundtrip + +import ( + "strings" + "testing" +) + +// ako/mxcli#859 (rehearsal M3): `create or modify` of a flow whose statement +// changes the notes on an activity — a note added, reworded or taken off — was +// refused under mdl 1 ("the annotations on the replaced … change"), because +// the splice kept a replaced activity's stored notes and could only refuse +// others. The rehearsal hit it on mxcli-ledger's BUILD_ScatterUrl (a second +// note added to a `set`), and on Studio Pro-authored PedApp nanoflows whose +// note text differs under mdl 1 (a mdl 0 description, `\r\n` escapes and all, +// run under the mdl 1 header). The refusal was on every run, so the script +// could never be re-run. +// +// A re-annotated activity is now replaced together with its notes: the stored +// notes go, the declared ones are drawn. The second run writes nothing. +const notesFlow = `create or modify microflow MyFirstModule.Rerun_Notes ($Filter: String) +returns String as $Url +begin + declare $Url String = ''; + @annotation 'Built here so it can be escaped.' + set $Url = '/odata/v1?$filter=' + $Filter; + log info node 'Notes' 'built'; + return $Url; +end; +` + +func TestSpliceRerun_ReannotatedActivity(t *testing.T) { + h := newHarness(t) + defer h.close() + if err := h.exec(notesFlow); err != nil { + t.Fatalf("create: %v\n%s", err, h.out.String()) + } + + // Control: the statement as created is unchanged, under either header. + for _, header := range []string{"", "mdl 1;\n"} { + before := h.snapshot() + if err := h.exec(header + notesFlow); err != nil { + t.Fatalf("re-run under %q: %v", header, err) + } + if changed := before.diff(h.snapshot()); len(changed) != 0 { + t.Fatalf("the unchanged statement under %q wrote %d unit(s)", header, len(changed)) + } + } + + const note = "@annotation 'Built here so it can be escaped.'\n" + cases := []struct { + name, edit string + want, gone []string + }{ + {"a note reworded", + strings.Replace(notesFlow, "so it can be escaped.", "so it can be escaped, as OData needs.", 1), + []string{"so it can be escaped, as OData needs."}, []string{"so it can be escaped.'"}}, + {"a second note added (ledger BUILD_ScatterUrl)", + strings.Replace(notesFlow, note, note+" @annotation 'A relative path, so it is same-origin.'\n", 1), + []string{"so it can be escaped.", "A relative path, so it is same-origin."}, nil}, + {"the note taken off", + strings.Replace(notesFlow, " "+note, "", 1), + nil, []string{"so it can be escaped."}}, + {"a note put on another activity", + strings.Replace(strings.Replace(notesFlow, " "+note, "", 1), " log info", " @annotation 'Logged once.'\n log info", 1), + []string{"Logged once."}, []string{"so it can be escaped."}}, + // The drop path: the annotated activity goes, and its note with it. + // Left behind, the note became a free annotation the second run + // refused ("the free annotations change"). + {"the annotated activity dropped", + strings.Replace(notesFlow, " "+note+" set $Url = '/odata/v1?$filter=' + $Filter;\n", "", 1), + nil, []string{"so it can be escaped."}}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + script := "mdl 1;\n" + c.edit + if c.edit == notesFlow { + t.Fatal("the edit changed nothing") + } + before := h.snapshot() + if err := h.exec(script); err != nil { + t.Fatalf("exec 1: %v\n%s", err, h.out.String()) + } + if !strings.Contains(h.out.String(), "Modified microflow: MyFirstModule.Rerun_Notes (spliced:") { + t.Errorf("exec 1 must splice the change in:\n%s", h.out.String()) + } + if len(before.diff(h.snapshot())) == 0 { + t.Fatal("exec 1 wrote nothing") + } + got := h.describeUnder("mdl 1;", "microflow MyFirstModule.Rerun_Notes") + for _, w := range c.want { + if !strings.Contains(got, w) { + t.Errorf("the description lacks %q:\n%s", w, got) + } + } + for _, g := range c.gone { + if strings.Contains(got, g) { + t.Errorf("the description still has %q:\n%s", g, got) + } + } + if n := strings.Count(got, "@annotation"); n != len(c.want) { + t.Errorf("%d notes described, want %d:\n%s", n, len(c.want), got) + } + // The twice-exec rule. + second := h.snapshot() + if err := h.exec(script); err != nil { + t.Fatalf("exec 2: %v\n%s", err, h.out.String()) + } + if changed := second.diff(h.snapshot()); len(changed) != 0 { + t.Errorf("exec 2 wrote %d unit(s):\n%s", len(changed), h.out.String()) + } + // Back to the created statement, for the next case. + if err := h.exec("mdl 1;\n" + notesFlow); err != nil { + t.Fatalf("restore the statement: %v\n%s", err, h.out.String()) + } + }) + } +} + +// The Studio Pro-authored form of M3: a PedApp nanoflow whose note, described +// by mdl 0 (`\r\n` escapes), is run under the mdl 1 header, where a backslash +// is a character — so the note's text differs, and was refused on every run. +// It is a real change of the note (mdl 1 reads the text as written), written +// once; the second run writes nothing. +func TestSpliceRerun_ReannotatedStudioProNanoflow(t *testing.T) { + h := newHarness(t) + defer h.close() + const target = "nanoflow FeedbackModule.ACT_Feedback_TriggerScreenshotMode" + mdl0 := h.mustDescribe(t, target) + if !strings.Contains(mdl0, `\r\n`) { + t.Fatalf("the fixture's note no longer holds a line break; the case is gone:\n%s", mdl0) + } + script := "mdl 1;\n" + mdl0 + before := h.snapshot() + if err := h.exec(script); err != nil { + t.Fatalf("exec 1: %v\n%s", err, h.out.String()) + } + if changed := before.diff(h.snapshot()); len(changed) != 1 { + t.Errorf("exec 1 wrote %d unit(s), want the nanoflow alone", len(changed)) + } + got := h.describeUnder("mdl 1;", target) + if !strings.Contains(got, `Feedback Widget. \r\nThe widget`) { + t.Errorf("the note is not the text the mdl 1 script states:\n%s", got) + } + second := h.snapshot() + if err := h.exec(script); err != nil { + t.Fatalf("exec 2: %v\n%s", err, h.out.String()) + } + if changed := second.diff(h.snapshot()); len(changed) != 0 { + t.Errorf("exec 2 wrote %d unit(s):\n%s", len(changed), h.out.String()) + } + // Control: the nanoflow's own mdl 1 description is no change at all. + h.restore() + own := "mdl 1;\n" + h.describeUnder("mdl 1;", target) + fresh := h.snapshot() + if err := h.exec(own); err != nil { + t.Fatalf("exec of its own mdl 1 description: %v", err) + } + if changed := fresh.diff(h.snapshot()); len(changed) != 0 { + t.Errorf("its own mdl 1 description wrote %d unit(s)", len(changed)) + } +} + +// ako/mxcli#859: `on error rollback` is what an activity with no clause stores +// in a microflow, so describe never prints it (formatErrorHandlingSuffix) — and +// a statement that states it never matched its own stored activity. Whenever +// the flow changed anywhere, the activity was dropped and written again as +// part of the change: a new $ID, its curves redrawn. +func TestSpliceRerun_OnErrorRollbackMatchesItsActivity(t *testing.T) { + h := newHarness(t) + defer h.close() + if err := h.exec("create persistent entity MyFirstModule.RerunRbThing (Name: String(100));"); err != nil { + t.Fatalf("create the entity: %v", err) + } + const flows = `create or modify microflow MyFirstModule.Rerun_Rollback ($E: MyFirstModule.RerunRbThing) +begin + log info node 'Rb' 'one'; + commit $E on error rollback; +end; +create or modify nanoflow MyFirstModule.Rerun_RollbackNf ($E: MyFirstModule.RerunRbThing) +begin + change $E (Name = 'one'); + commit $E on error rollback; +end; +` + if err := h.exec(flows); err != nil { + t.Fatalf("create: %v\n%s", err, h.out.String()) + } + edited := "mdl 1;\n" + strings.Replace(strings.Replace(flows, "'Rb' 'one'", "'Rb' 'two'", 1), "Name = 'one'", "Name = 'two'", 1) + before := h.snapshot() + if err := h.exec(edited); err != nil { + t.Fatalf("exec 1: %v\n%s", err, h.out.String()) + } + for _, want := range []string{ + "Modified microflow: MyFirstModule.Rerun_Rollback (spliced: 1 replaced)", + "Modified nanoflow: MyFirstModule.Rerun_RollbackNf (spliced: 1 replaced)", + } { + // The control is in the same line: the change itself is written. + if !strings.Contains(h.out.String(), want) { + t.Errorf("want %q — the commit must match its stored activity:\n%s", want, h.out.String()) + } + } + if len(before.diff(h.snapshot())) == 0 { + t.Fatal("exec 1 wrote nothing") + } + second := h.snapshot() + if err := h.exec(edited); err != nil { + t.Fatalf("exec 2: %v\n%s", err, h.out.String()) + } + if changed := second.diff(h.snapshot()); len(changed) != 0 { + t.Errorf("exec 2 wrote %d unit(s):\n%s", len(changed), h.out.String()) + } +} + +// The rehearsal's form of M3, over every PedApp nanoflow: its mdl 0 +// description put under the `mdl 1;` header — the upgrade a user makes by +// adding the header alone. The parity property (TestPedAppFlowSpliceParity) +// runs each flow's OWN mdl 1 description, which never differs from the stored +// flow; this one can (a note's `\r\n` is two characters under mdl 1), and two +// of the thirteen were refused on every run. Each must now be written at most +// once: executed a second time, it writes nothing. +func TestSpliceRerun_PedAppNanoflowsUnderTheHeader(t *testing.T) { + h := newHarness(t) + defer h.close() + n := 0 + for _, d := range h.documents() { + if d.keyword != "nanoflow" { + continue + } + n++ + t.Run(d.key(), func(t *testing.T) { + defer h.restore() + mdl0, err := h.describe(d.target()) + if err != nil { + t.Fatalf("describe: %v", err) + } + script := "mdl 1;\n" + mdl0 + if err := h.exec(script); err != nil { + t.Fatalf("exec 1: %v", err) + } + second := h.snapshot() + if err := h.exec(script); err != nil { + t.Fatalf("exec 2: %v", err) + } + if changed := second.diff(h.snapshot()); len(changed) != 0 { + t.Errorf("exec 2 wrote %d unit(s):\n%s", len(changed), h.out.String()) + } + }) + } + if n == 0 { + t.Fatal("PedApp has no nanoflow — the enumeration is broken") + } +} diff --git a/mdl/roundtrip/flow_rerun_property_test.go b/mdl/roundtrip/flow_rerun_property_test.go index 02a85a4bc..10038ca0b 100644 --- a/mdl/roundtrip/flow_rerun_property_test.go +++ b/mdl/roundtrip/flow_rerun_property_test.go @@ -152,9 +152,7 @@ func TestFlowRerunProperty(t *testing.T) { // rerunKnownFailures are scripts whose flows do not re-run clean yet, each with // the issue that tracks it. The list may only shrink: a listed script that // re-runs clean fails the test until it is taken off. -var rerunKnownFailures = map[string]string{ - "mdl-examples/doctype-tests/06b-soap-examples.mdl": "ako/mxcli#861: a call web service activity is re-spliced on every run", -} +var rerunKnownFailures = map[string]string{} // flowRerun is the second execution of a script: its flow statements, each as // `create or modify`, under the script's language header — the last definition diff --git a/mdl/roundtrip/flow_self_call_test.go b/mdl/roundtrip/flow_self_call_test.go new file mode 100644 index 000000000..16fc63837 --- /dev/null +++ b/mdl/roundtrip/flow_self_call_test.go @@ -0,0 +1,96 @@ +// SPDX-License-Identifier: Apache-2.0 + +//go:build integration + +package roundtrip + +import ( + "bytes" + "testing" +) + +// ako/mxcli#843, found by the beta dress rehearsal (M5, M2). Both made an +// mdl 1 script un-authorable or un-re-runnable: +// +// - a flow that calls itself failed exec with "microflow not found" (check +// passed), and the stub-then-real workaround is refused by the splice; +// - a nanoflow stored without ReturnVariableName refused the `as $Var` a +// later statement states ("set it in Studio Pro"). +// +// Each statement is executed twice: the first writes, the second writes +// nothing (the re-run rule). One harness for all of it — this suite is near +// its time limit (#870). +func TestFlowCreate_SelfCallAndNanoflowReturnVariable(t *testing.T) { + h := newHarness(t) + defer h.close() + + twice := func(t *testing.T, name, script string) []byte { + t.Helper() + if err := h.exec(script); err != nil { + t.Fatalf("exec 1: %v", err) + } + first := h.flowUnit(t, name) + if err := h.exec(script); err != nil { + t.Fatalf("exec 2: %v", err) + } + if again := h.flowUnit(t, name); !bytes.Equal(again, first) { + t.Fatalf("exec 2 rewrote %s: %v", name, strictDiff(t, first, again)) + } + return first + } + + t.Run("recursive microflow", func(t *testing.T) { + twice(t, "Rt843_Countdown", `mdl 1; +create or modify microflow MyFirstModule.Rt843_Countdown ($N: Integer) +returns Boolean as $Done +begin + if $N <= 0 then + return true; + end if; + $Below = call microflow MyFirstModule.Rt843_Countdown(N = $N - 1); + return $Below; +end;`) + }) + + t.Run("recursive nanoflow", func(t *testing.T) { + twice(t, "Rt843_CountdownNf", `mdl 1; +create or modify nanoflow MyFirstModule.Rt843_CountdownNf ($N: Integer) +returns Boolean as $Done +begin + if $N <= 0 then + return true; + end if; + $Below = call nanoflow MyFirstModule.Rt843_CountdownNf(N = $N - 1); + return $Below; +end;`) + }) + + t.Run("nanoflow return variable added", func(t *testing.T) { + if err := h.exec(`create or modify nanoflow MyFirstModule.Rt843_ReturnName ($Flag: Boolean) +returns Boolean +begin + return true; +end;`); err != nil { + t.Fatalf("setup: %v", err) + } + stored := h.flowUnit(t, "Rt843_ReturnName") + if bytes.Contains(stored, []byte("ReturnVariableName")) { + t.Fatal("precondition: the setup nanoflow already stores ReturnVariableName") + } + // The control is exec 1 itself: it must change the unit, so an + // unchanged exec 2 is the re-run rule and not a check that sees nothing. + after := twice(t, "Rt843_ReturnName", `mdl 1; +create or modify nanoflow MyFirstModule.Rt843_ReturnName ($Flag: Boolean) +returns Boolean as $Done +begin + return true; +end;`) + diff := strictDiff(t, stored, after) + if len(diff) != 1 { + t.Fatalf("want exactly the added ReturnVariableName, got %v", diff) + } + if out := h.mustDescribe(t, "nanoflow MyFirstModule.Rt843_ReturnName"); !bytes.Contains([]byte(out), []byte("returns Boolean as $Done")) { + t.Fatalf("describe does not state the return variable:\n%s", out) + } + }) +} diff --git a/mdl/roundtrip/flow_splice_member_qualified_test.go b/mdl/roundtrip/flow_splice_member_qualified_test.go new file mode 100644 index 000000000..36f1a91f2 --- /dev/null +++ b/mdl/roundtrip/flow_splice_member_qualified_test.go @@ -0,0 +1,201 @@ +// SPDX-License-Identifier: Apache-2.0 + +//go:build integration + +package roundtrip + +import ( + "bytes" + "fmt" + "slices" + "strings" + "testing" + + "go.mongodb.org/mongo-driver/v2/bson" +) + +// ako/mxcli#885: `create or modify` that splices a replacement activity built +// it with a builder that only knew the types of the stored flow's parameters, +// declared variables and created objects. A variable bound by a retrieve, a +// microflow call or a list operation was untyped, so the replacement's members +// were written without their entity: a change's member as the bare `Name` +// instead of `System.User.Name` — and Mendix cannot load a project holding one +// ("The text 'Name' is not a valid AttributeIdentifier") — a find's `Name = 'y'` +// as a find by that (invalid) expression, an aggregate's attribute as nothing. +// A parameter-bound variable was typed and is the control. +// +// The splice must write what a full build of the same statement writes, so each +// case compares the members the splice wrote with the members `create` writes +// for the edited flow under another name. +const memberQualifiedSetup = `mdl 1; +create or modify microflow MyFirstModule.T885_GetUser () +returns System.User as $U +begin + retrieve $U from System.User first; + return $U; +end; +create or modify microflow MyFirstModule.T885_GetUsers () +returns List of System.User as $Us +begin + retrieve $Us from System.User; + return $Us; +end; +` + +const memberQualifiedFlow = `create or modify microflow MyFirstModule.T885_Flow ($P: System.User) +begin + retrieve $R from System.User first; + change $R (Name = 'r'); + $C = call microflow MyFirstModule.T885_GetUser(); + change $C (Name = 'c'); + $L = call microflow MyFirstModule.T885_GetUsers(); + $S = SORT $L BY Name ASC; + $F = FIND $L BY Name = 'x'; + $Sum = SUM $L BY FailedLogins; + $H = HEAD $L; + change $H (Name = 'h'); + change $P (Name = 'p'); + loop $It in $L + begin + change $It (Name = 'i'); + end loop; +end; +` + +func TestFlowModify_SplicedMembersAreQualified(t *testing.T) { + h := newHarness(t) + defer h.close() + + cases := []struct { + name, from, to string + // inLoop: a change inside a loop body is not spliced (see + // TestFlowModify_LoopBodyChangeIsNotSpliced): mdl 1 refuses it and + // mdl 0 rebuilds, which must qualify the iterator's members too. + inLoop bool + }{ + {name: "change on a retrieve-bound variable", from: "(Name = 'r')", to: "(Name = 'r2')"}, + {name: "change on a call-bound variable", from: "(Name = 'c')", to: "(Name = 'c2')"}, + {name: "change on a list-operation-bound variable", from: "(Name = 'h')", to: "(Name = 'h2')"}, + {name: "sort of a call-bound list", from: "BY Name ASC", to: "BY Name DESC"}, + {name: "find in a call-bound list", from: "BY Name = 'x'", to: "BY Name = 'y'"}, + {name: "aggregate of a call-bound list", from: "SUM $L", to: "MAXIMUM $L"}, + {name: "control: change on a parameter", from: "(Name = 'p')", to: "(Name = 'p2')"}, + {name: "change on a loop iterator", from: "(Name = 'i')", to: "(Name = 'i2')", inLoop: true}, + } + for _, header := range []string{"mdl 1;\n", ""} { + for _, c := range cases { + label := fmt.Sprintf("%s (header %q)", c.name, strings.TrimSpace(header)) + h.restore() + if err := h.exec(memberQualifiedSetup + memberQualifiedFlow); err != nil { + t.Fatalf("%s: setup: %v", label, err) + } + edited := strings.Replace(memberQualifiedFlow, c.from, c.to, 1) + if edited == memberQualifiedFlow { + t.Fatalf("%s: the flow has no %q", label, c.from) + } + before := h.flowUnit(t, "T885_Flow") + err := h.exec(header + edited) + switch { + case c.inLoop && header != "": + if err == nil || !strings.Contains(err.Error(), "cannot be spliced") { + t.Errorf("%s: want the loop-body change refused, got %v", label, err) + } + if !bytes.Equal(h.flowUnit(t, "T885_Flow"), before) { + t.Errorf("%s: the refused statement wrote", label) + } + continue + case err != nil: + t.Errorf("%s: exec: %v", label, err) + continue + case !c.inLoop && !strings.Contains(h.out.String(), "(spliced: 1 replaced)"): + t.Errorf("%s: want one statement replaced by a splice, got:\n%s", label, h.out.String()) + } + spliced := flowMembers(t, h.flowUnit(t, "T885_Flow")) + for _, m := range spliced { + if bare, ok := strings.CutPrefix(m, "bare "); ok { + t.Errorf("%s: the splice wrote an unqualified member %s", label, bare) + } + } + + // Parity: what `create` writes for the edited flow. + full := strings.Replace(edited, "MyFirstModule.T885_Flow", "MyFirstModule.T885_Full", 1) + if err := h.exec(header + full); err != nil { + t.Fatalf("%s: full build: %v", label, err) + } + if want := flowMembers(t, h.flowUnit(t, "T885_Full")); !slices.Equal(spliced, want) { + t.Errorf("%s: the splice wrote other members than a full build\n only spliced: %v\n only full: %v", + label, without(spliced, want), without(want, spliced)) + } + } + } +} + +// flowMembers lists, sorted, every entity member a flow unit names — a change +// or create item's attribute or association, a list operation's, an +// aggregate's, a sort item's — with the $Type that holds it. A list operation +// that names its member in an expression is listed by its $Type, so a find by +// member that turns into a find by expression is a difference. A non-empty +// attribute that is not Module.Entity.Attribute is listed as "bare …". +func flowMembers(t *testing.T, raw []byte) []string { + t.Helper() + var doc bson.D + if err := bson.Unmarshal(raw, &doc); err != nil { + t.Fatalf("decode unit: %v", err) + } + var out []string + var walk func(v any) + walk = func(v any) { + switch x := v.(type) { + case bson.D: + typ := "" + fields := map[string]string{} + for _, e := range x { + if s, ok := e.Value.(string); ok { + fields[e.Key] = s + } + } + typ = fields["$Type"] + switch typ { + case "Microflows$ChangeActionItem", "DomainModels$AttributeRef", "Microflows$Find", "Microflows$Filter", + "Microflows$AggregateAction": + for _, k := range []string{"Attribute", "Association"} { + s := fields[k] + if k == "Attribute" && s != "" && strings.Count(s, ".") < 2 { + out = append(out, fmt.Sprintf("bare %s.%s=%q", typ, k, s)) + continue + } + out = append(out, fmt.Sprintf("%s.%s=%q", typ, k, s)) + } + case "Microflows$FindByExpression", "Microflows$FilterByExpression": + out = append(out, typ) + } + for _, e := range x { + walk(e.Value) + } + case bson.A: + for _, e := range x { + walk(e) + } + } + } + walk(doc) + slices.Sort(out) + return out +} + +// without returns a minus b, as multisets. +func without(a, b []string) []string { + left := map[string]int{} + for _, s := range b { + left[s]++ + } + var out []string + for _, s := range a { + if left[s] > 0 { + left[s]-- + continue + } + out = append(out, s) + } + return out +} diff --git a/mdl/roundtrip/revoke_grant_rerun_test.go b/mdl/roundtrip/revoke_grant_rerun_test.go new file mode 100644 index 000000000..4b031f44f --- /dev/null +++ b/mdl/roundtrip/revoke_grant_rerun_test.go @@ -0,0 +1,230 @@ +// SPDX-License-Identifier: Apache-2.0 + +//go:build integration + +package roundtrip + +import ( + "database/sql" + "encoding/hex" + "strings" + "testing" + + "go.mongodb.org/mongo-driver/v2/bson" + _ "modernc.org/sqlite" +) + +// ako/mxcli#872 (rehearsal W3): a "reset, then authoritative grants" section, +// `revoke all on entity E from R;` followed by the grants that are meant to hold, +// wrote the domain model on every run with a freshly minted access rule, although +// the rules it ended with were the ones it started with. Each statement wrote on +// its own: the revoke removed the rule, the grant built a new one, and the grant's +// write had nothing left on disk to carry the old rule's identity from. +// +// The net state is compared now: a run of access-rule statements is written once, +// at its end, and a run that ends where it started writes nothing. +// +// One harness for both cases (the roundtrip suite is near its time limit, #870). +func TestRevokeGrantRerun(t *testing.T) { + h := newHarness(t) + defer h.close() + t.Run("an identical re-run writes nothing", func(t *testing.T) { revokeGrantRerunWritesNothing(t, h) }) + h.restore() + t.Run("Studio Pro rules keep their identity", func(t *testing.T) { revokeGrantKeepsStudioProIdentity(t, h) }) +} + +func revokeGrantRerunWritesNothing(t *testing.T, h *harness) { + + const script = `mdl 1; +create or modify persistent entity MyFirstModule.RerunTx ( + TxDate: DateTime, + Amount: Decimal +); +revoke all on entity MyFirstModule.RerunTx from MyFirstModule.User; +grant read *, write * on entity MyFirstModule.RerunTx to MyFirstModule.User; +` + if err := h.exec(script); err != nil { + t.Fatalf("first run: %v\n%s", err, h.out.String()) + } + first, tx := h.snapshot(), h.lastTransactionID() + if err := h.exec(script); err != nil { + t.Fatalf("second run: %v\n%s", err, h.out.String()) + } + if changed := first.diff(h.snapshot()); len(changed) != 0 { + t.Errorf("the identical second run wrote %d unit(s):\n %s", len(changed), strings.Join(changed, "\n ")) + } + if got := h.lastTransactionID(); got != tx { + t.Errorf("the identical second run moved the project's transaction id (%s -> %s): something was written", tx, got) + } + if got := h.mustDescribe(t, "entity MyFirstModule.RerunTx"); !strings.Contains(got, + "grant read *, write * on entity MyFirstModule.RerunTx to MyFirstModule.User;") { + t.Errorf("the rule is not there after the second run:\n%s", got) + } + + // Control: the same reset with a narrower grant is a change, and is written. + narrower := strings.Replace(script, "grant read *, write *", "grant read *", 1) + if err := h.exec(narrower); err != nil { + t.Fatalf("narrower grant: %v\n%s", err, h.out.String()) + } + if len(first.diff(h.snapshot())) == 0 { + t.Error("a reset to a narrower grant wrote nothing") + } + if got := h.mustDescribe(t, "entity MyFirstModule.RerunTx"); !strings.Contains(got, + "grant read * on entity MyFirstModule.RerunTx to MyFirstModule.User;") { + t.Errorf("a reset to a narrower grant: want the narrower rule:\n%s", got) + } + // Control: a revoke that is not followed by a grant still removes the rule. + if err := h.exec("mdl 1;\nrevoke all on entity MyFirstModule.RerunTx from MyFirstModule.User;\n"); err != nil { + t.Fatalf("revoke alone: %v\n%s", err, h.out.String()) + } + if got := h.mustDescribe(t, "entity MyFirstModule.RerunTx"); strings.Contains(got, "grant ") { + t.Errorf("a revoke alone left a rule:\n%s", got) + } +} + +// The same reset over rules Studio Pro authored: PedApp's Administration.Account +// carries two rules for Administration.User, one of them XPath-constrained. A +// reset that grants both back keeps the stored rules' identity — every rule and +// member-access $ID Studio Pro gave them — and the identical second run writes +// nothing. An mxcli-created rule could not show the first half: its identity is +// whatever the previous run minted. +// +// (The first run is a write: describe's spelling of these two rules does not +// reproduce Studio Pro's bytes — the stored default member access of the first +// is ReadOnly — so the reset states them in mxcli's spelling. What must not move +// is their identity.) +func revokeGrantKeepsStudioProIdentity(t *testing.T, h *harness) { + + const reset = `mdl 1; +revoke all on entity Administration.Account from Administration.User; +grant read (FullName, Email) on entity Administration.Account to Administration.User; +grant read (FullName), write (FullName) on entity Administration.Account to Administration.User where [id='[%CurrentUser%]']; +` + stored := accessRuleIDs(t, h.orig, "Account") + if len(stored) != 3 { + t.Fatalf("fixture: want Account's 3 Studio Pro rules, got %d", len(stored)) + } + if err := h.exec(reset); err != nil { + t.Fatalf("reset: %v\n%s", err, h.out.String()) + } + first := h.snapshot() + if got := accessRuleIDs(t, first, "Account"); !equalIDLists(got, stored) { + t.Errorf("the reset re-minted Studio Pro's rules:\n stored %v\n now %v", stored, got) + } + tx := h.lastTransactionID() + if err := h.exec(reset); err != nil { + t.Fatalf("second reset: %v\n%s", err, h.out.String()) + } + if changed := first.diff(h.snapshot()); len(changed) != 0 { + t.Errorf("the identical second reset wrote %d unit(s):\n %s", len(changed), strings.Join(changed, "\n ")) + } + if got := h.lastTransactionID(); got != tx { + t.Errorf("the identical second reset moved the transaction id (%s -> %s)", tx, got) + } + + // Control: a reset that no longer grants the constrained rule removes it. + withoutXPath := reset[:strings.LastIndex(reset, "grant read (FullName), write")] + if err := h.exec(withoutXPath); err != nil { + t.Fatalf("reset without the constrained rule: %v\n%s", err, h.out.String()) + } + if len(first.diff(h.snapshot())) == 0 { + t.Error("a reset that drops a rule wrote nothing") + } + if got := h.mustDescribe(t, "entity Administration.Account"); strings.Contains(got, "CurrentUser") { + t.Errorf("the constrained rule survived a reset that does not grant it:\n%s", got) + } +} + +// accessRuleIDs lists, per access rule of the named entity, the rule's $ID +// followed by its member accesses' $IDs, in stored order. +func accessRuleIDs(t *testing.T, s snapshot, entity string) [][]string { + t.Helper() + for _, raw := range s.units { + var doc bson.D + if bson.Unmarshal(raw, &doc) != nil || docString(doc, "$Type") != "DomainModels$DomainModel" { + continue + } + for _, ev := range docArray(doc, "Entities") { + ent, ok := ev.(bson.D) + if !ok || docString(ent, "Name") != entity { + continue + } + var out [][]string + for _, rv := range docArray(ent, "AccessRules") { + rule, ok := rv.(bson.D) + if !ok { + continue + } + ids := []string{docID(rule)} + for _, mv := range docArray(rule, "MemberAccesses") { + if ma, ok := mv.(bson.D); ok { + ids = append(ids, docID(ma)) + } + } + out = append(out, ids) + } + return out + } + } + t.Fatalf("no entity %s in any domain model", entity) + return nil +} + +func docString(d bson.D, key string) string { + for _, e := range d { + if e.Key == key { + s, _ := e.Value.(string) + return s + } + } + return "" +} + +func docArray(d bson.D, key string) bson.A { + for _, e := range d { + if e.Key == key { + a, _ := e.Value.(bson.A) + return a + } + } + return nil +} + +func docID(d bson.D) string { + for _, e := range d { + if e.Key == "$ID" { + if b, ok := e.Value.(bson.Binary); ok { + return hex.EncodeToString(b.Data) + } + } + } + return "" +} + +func equalIDLists(a, b [][]string) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if strings.Join(a[i], ",") != strings.Join(b[i], ",") { + return false + } + } + return true +} + +// lastTransactionID is the value Studio Pro compares to decide the project +// changed on disk; any write that lands moves it. +func (h *harness) lastTransactionID() string { + h.t.Helper() + db, err := sql.Open("sqlite", "file:"+h.mpr+"?mode=ro") + if err != nil { + h.t.Fatalf("open %s: %v", h.mpr, err) + } + defer db.Close() + var id string + if err := db.QueryRow(`SELECT LastTransactionID FROM _Transaction`).Scan(&id); err != nil { + h.t.Fatalf("read LastTransactionID: %v", err) + } + return id +} diff --git a/mdl/roundtrip/upgrade_commit_events_test.go b/mdl/roundtrip/upgrade_commit_events_test.go new file mode 100644 index 000000000..68f28eac7 --- /dev/null +++ b/mdl/roundtrip/upgrade_commit_events_test.go @@ -0,0 +1,94 @@ +// SPDX-License-Identifier: Apache-2.0 + +//go:build integration + +package roundtrip + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/executor" + "github.com/mendixlabs/mxcli/mdl/upgrade" +) + +// Rehearsal M1 (ako/mxcli#873): a flow stored by an mxcli older than #895, +// when a bare `commit` meant without events. The same script now means WITH +// events, so re-running it changes the stored flow — and inside a loop the +// mdl 1 splice refuses it on every run. `fmt --upgrade -p` reads the stored +// flag and writes it into the script; the upgraded script then re-runs as +// Unchanged, twice, under both language versions. +// +// The repro is the rehearsal's commit-default-in-loop.setup.mdl + .mdl. +func TestUpgradePinsStoredCommitEvents_ReRunIsUnchanged(t *testing.T) { + const setup = `create or modify persistent entity MyFirstModule.Tx ( + Amount: Decimal +); +create or modify microflow MyFirstModule.Repro_CommitInLoop ($Items: List of MyFirstModule.Tx) +begin + loop $T in $Items + begin + change $T (Amount = 0); + commit $T without events; + end loop; +end; +` + const script = `create or modify microflow MyFirstModule.Repro_CommitInLoop ($Items: List of MyFirstModule.Tx) +begin + loop $T in $Items + begin + change $T (Amount = 0); + commit $T; + end loop; +end; +` + h := newHarness(t) + defer h.close() + + for _, header := range []string{"", "mdl 1;\n"} { + name := map[string]string{"": "mdl 0", "mdl 1;\n": "mdl 1"}[header] + t.Run(name, func(t *testing.T) { + h.restore() + if err := h.exec(setup); err != nil { + t.Fatalf("setup: %v\n%s", err, h.out.String()) + } + stored := h.snapshot() + + // Control: the script as written does not re-run as Unchanged + // against this flow — it is refused (mdl 1) or rewrites the flow + // with events on (mdl 0). Without this the checks below could pass + // against a build where the bare commit still meant without events. + err := h.exec(header + script) + if err == nil && len(stored.diff(h.snapshot())) == 0 { + t.Fatalf("control: the unpinned script re-ran as Unchanged:\n%s", h.out.String()) + } + t.Logf("control (%s): err=%v", name, err) + h.restore() + if err := h.exec(setup); err != nil { + t.Fatalf("setup: %v", err) + } + stored = h.snapshot() + + res, err := upgrade.Upgrade(header+script, upgrade.Options{ + Commits: executor.NewStoredCommitEvents(h.exe.Backend()), + }) + if err != nil { + t.Fatal(err) + } + if res.CommitsPinned != 1 || !strings.Contains(res.Source, " commit $T without events;\n") { + t.Fatalf("the upgrade did not pin the stored flag (%d pinned):\n%s", res.CommitsPinned, res.Source) + } + for run := 1; run <= 2; run++ { + if err := h.exec(res.Source); err != nil { + t.Fatalf("run %d of the upgraded script: %v\n%s", run, err, h.out.String()) + } + if !strings.Contains(h.out.String(), "Unchanged ") { + t.Errorf("run %d did not report Unchanged:\n%s", run, h.out.String()) + } + if changed := stored.diff(h.snapshot()); len(changed) != 0 { + t.Errorf("run %d wrote %d unit(s):\n %s", run, len(changed), strings.Join(changed, "\n ")) + } + } + }) + } +} diff --git a/mdl/upgrade/commit_events.go b/mdl/upgrade/commit_events.go new file mode 100644 index 000000000..9c7103cd1 --- /dev/null +++ b/mdl/upgrade/commit_events.go @@ -0,0 +1,209 @@ +// SPDX-License-Identifier: Apache-2.0 + +package upgrade + +import ( + "fmt" + "strings" + + "github.com/mendixlabs/mxcli/mdl/ast" +) + +// StoredCommits is the project a script runs against, as far as the commit +// pin needs it (ako/mxcli#873): the events flag of each Commit activity a +// stored flow holds. `fmt --upgrade -p app.mpr` passes one backed by the +// project; tests pass a map. +type StoredCommits interface { + // CommitEvents returns, per committed variable (without the `$`), the + // WithEvents flag of each Commit activity in the stored microflow (the + // nanoflow when nanoflow is set) named qualifiedName, loop bodies included. + // found is false when the project has no such flow. + CommitEvents(nanoflow bool, qualifiedName string) (events map[string][]bool, found bool) +} + +// Note is something the upgrade did not change and the author should know: +// printed by fmt, never an error. +type Note struct { + Line int + Code string // the rule the note is about, e.g. MDL067 + Message string +} + +// commitNoteCode is the check that reports the #895 default change. +const commitNoteCode = "MDL067" + +// pinCommitEvents states the stored events flag on the bare commits of every +// `create or modify` flow whose stored flow commits the variable without +// events (ako/mxcli#873). +// +// A bare `commit $X;` means WITH events since #895, Studio Pro's default; an +// older mxcli stored the same statement WITHOUT. Upgrading a script must not +// change what it builds, and re-running it against its own project must not +// either, so where the project says the stored commit has no events, the +// statement says so: `commit $X without events;`. A new flow, a new commit, or +// a stored commit with events is left as written: the current default already +// describes it. +// +// The script's commits are matched to the stored Commit activities by +// variable. A commit whose flag is written matches a stored activity with the +// same flag; the bare ones are pinned only when every remaining stored commit +// of the variable is without events and there are no more bare ones than +// stored ones. Anything else — the stored flow commits the variable both ways, +// or the script adds a commit — cannot be matched by variable alone, so it is +// left and reported. Without a project, every flow with a bare commit is +// reported. +func pinCommitEvents(prog *ast.Program, stored StoredCommits) (edits []Edit, pinned int, notes []Note) { + type flowKey struct { + nanoflow bool + name string + } + var order []flowKey + byFlow := map[flowKey][]ast.FlowCommit{} + for _, c := range prog.FlowCommits { + k := flowKey{c.Nanoflow, c.Flow.String()} + if _, seen := byFlow[k]; !seen { + order = append(order, k) + } + byFlow[k] = append(byFlow[k], c) + } + + for _, k := range order { + commits := byFlow[k] + var bare []ast.FlowCommit + for _, c := range commits { + if c.Bare { + bare = append(bare, c) + } + } + if len(bare) == 0 { + continue + } + if stored == nil { + notes = append(notes, Note{Line: bare[0].Line, Code: commitNoteCode, Message: fmt.Sprintf( + "%s: %s mean%s WITH events since #895 (%s); if an older mxcli stored this flow, it holds them "+ + "without events — pass -p app.mpr to state the stored flag, or write `with events` / "+ + "`without events` by hand", + k.name, countOf(len(bare), "bare `commit` statement"), plural(len(bare), "s", ""), commitNoteCode)}) + continue + } + events, found := stored.CommitEvents(k.nanoflow, k.name) + if !found { + continue // a new flow: the default describes it + } + for _, v := range variablesOf(bare) { + remaining := append([]bool(nil), events[v]...) + var mine []ast.FlowCommit + total, unmatched := 0, 0 + for _, c := range commits { + if c.Variable != v { + continue + } + total++ + if c.Bare { + mine = append(mine, c) + continue + } + var ok bool + if remaining, ok = removeOne(remaining, !c.WithoutEvents); !ok { + unmatched++ // a commit the stored flow does not have with this flag + } + } + withEvents, without := 0, 0 + for _, e := range remaining { + if e { + withEvents++ + } else { + without++ + } + } + switch { + case without == 0: + // Stored with events, or not stored: the default describes it. + case withEvents == 0 && len(mine)+unmatched <= without: + for _, c := range mine { + edits = append(edits, c.PinWithoutEvents.Edits...) + pinned++ + } + default: + notes = append(notes, Note{Line: mine[0].Line, Code: commitNoteCode, Message: fmt.Sprintf( + "%s: the stored flow commits $%s %s, and the script commits it %s, %d bare; which bare "+ + "commit is which stored one cannot be told, and since #895 (%s) a bare commit means WITH "+ + "events — write `with events` or `without events` on each by hand", + k.name, v, storedSummary(len(events[v])-countTrue(events[v]), countTrue(events[v])), + times(total), len(mine), commitNoteCode)}) + } + } + } + return edits, pinned, notes +} + +// variablesOf lists the committed variables in first-use order. +func variablesOf(cs []ast.FlowCommit) []string { + var out []string + seen := map[string]bool{} + for _, c := range cs { + if !seen[c.Variable] { + seen[c.Variable] = true + out = append(out, c.Variable) + } + } + return out +} + +// removeOne removes one occurrence of v from s, and reports whether there +// was one. +func removeOne(s []bool, v bool) ([]bool, bool) { + for i, e := range s { + if e == v { + return append(s[:i], s[i+1:]...), true + } + } + return s, false +} + +func countTrue(s []bool) int { + n := 0 + for _, e := range s { + if e { + n++ + } + } + return n +} + +func times(n int) string { + switch n { + case 1: + return "once" + case 2: + return "twice" + } + return fmt.Sprintf("%d times", n) +} + +// storedSummary says how often a variable is committed without and with +// events. +func storedSummary(without, withEvents int) string { + var parts []string + if without > 0 { + parts = append(parts, times(without)+" without events") + } + if withEvents > 0 { + parts = append(parts, times(withEvents)+" with events") + } + return strings.Join(parts, " and ") +} + +func countOf(n int, what string) string { + if n == 1 { + return "1 " + what + } + return fmt.Sprintf("%d %ss", n, what) +} + +func plural(n int, one, many string) string { + if n == 1 { + return one + } + return many +} diff --git a/mdl/upgrade/commit_events_test.go b/mdl/upgrade/commit_events_test.go new file mode 100644 index 000000000..5f2ccf25e --- /dev/null +++ b/mdl/upgrade/commit_events_test.go @@ -0,0 +1,147 @@ +// SPDX-License-Identifier: Apache-2.0 + +package upgrade + +import ( + "strings" + "testing" +) + +// storedFlows is an upgrade.StoredCommits with fixed answers: flow name -> +// variable -> the WithEvents flag of each stored Commit activity. +type storedFlows map[string]map[string][]bool + +func (s storedFlows) CommitEvents(_ bool, qn string) (map[string][]bool, bool) { + e, ok := s[qn] + return e, ok +} + +// The rehearsal's M1 repro (ako/mxcli#873): the stored flow — built by an +// older mxcli, when a bare commit meant without events — commits $T without +// events inside a loop; the script, unchanged, now means WITH events. With the +// project, the upgrade states the stored flag, so the script builds what is +// stored and the re-run is Unchanged instead of a refused loop-body change. +const commitInLoop = `create or modify microflow MyFirstModule.Repro_CommitInLoop ($Items: List of MyFirstModule.Tx) +begin + loop $T in $Items + begin + change $T (Amount = 0); + commit $T; + end loop; +end; +` + +func TestUpgrade_PinsTheStoredCommitWithoutEvents(t *testing.T) { + stored := storedFlows{"MyFirstModule.Repro_CommitInLoop": {"T": {false}}} + res := mustUpgrade(t, commitInLoop, Options{Commits: stored}) + want := strings.Replace(commitInLoop, "commit $T;", "commit $T without events;", 1) + if res.Source != want { + t.Fatalf("got:\n%s\nwant:\n%s", res.Source, want) + } + if res.CommitsPinned != 1 || !res.Changed() { + t.Errorf("CommitsPinned = %d, Changed = %v; want 1, true", res.CommitsPinned, res.Changed()) + } + if len(res.Notes) != 0 { + t.Errorf("Notes = %+v, want none", res.Notes) + } + // Idempotent: the pinned statement is no longer bare. + if again := mustUpgrade(t, res.Source, Options{Commits: stored}); again.Changed() || len(again.Notes) != 0 { + t.Errorf("second upgrade: changed=%v notes=%+v", again.Changed(), again.Notes) + } + // With the header too: the pin is not gated. + hdr := mustUpgrade(t, commitInLoop, Options{AddHeader: true, Commits: stored}) + if !strings.Contains(hdr.Source, "commit $T without events;") || !hdr.HeaderAdded { + t.Errorf("with the header:\n%s", hdr.Source) + } +} + +// The controls: the pin follows the stored flag, never the script alone. +func TestUpgrade_CommitPinFollowsTheStoredFlow(t *testing.T) { + two := `create or modify microflow M.F ($A: M.E, $B: M.E) +begin + commit $A; + COMMIT $B REFRESH; + commit $A with events; +end; +create or modify nanoflow M.N ($A: M.E) begin commit $A; end; +create microflow M.New ($A: M.E) begin commit $A; end; +alter microflow M.F { + insert after $A begin commit $B; end; +}; +` + for _, c := range []struct { + name string + stored storedFlows + want []string // substrings of the output + pinned int + notes []string + }{ + {"stored with events: left as written", + storedFlows{"M.F": {"A": {true, true}, "B": {true}}, "M.N": {"A": {true}}}, + []string{" commit $A;\n", " COMMIT $B REFRESH;\n"}, 0, nil}, + {"the flow is not in the project: left as written", + storedFlows{}, []string{" commit $A;\n"}, 0, nil}, + {"the variable is not committed in the stored flow: left as written", + storedFlows{"M.F": {"X": {false}}}, []string{" commit $A;\n"}, 0, nil}, + {"stored without events: pinned, in the keyword's case, before refresh", + storedFlows{"M.F": {"A": {false, true}, "B": {false}}, "M.N": {"A": {false}}}, + []string{" commit $A without events;\n", " COMMIT $B WITHOUT EVENTS REFRESH;\n", + " commit $A with events;\n", "begin commit $A without events; end;\ncreate microflow M.New ($A: M.E) begin commit $A; end;", + "begin commit $B; end;"}, 3, nil}, + {"stored both ways for the bare ones: reported", + storedFlows{"M.F": {"A": {false, true, true}, "B": {true}}}, + []string{" commit $A;\n"}, 0, []string{"M.F: the stored flow commits $A once without events and twice with events, and the script commits it twice, 1 bare"}}, + {"the script commits more often than the stored flow: reported", + storedFlows{"M.F": {"A": {false}, "B": {false}}}, + []string{" commit $A;\n", " COMMIT $B WITHOUT EVENTS REFRESH;\n"}, 1, + []string{"M.F: the stored flow commits $A once without events, and the script commits it twice, 1 bare"}}, + } { + t.Run(c.name, func(t *testing.T) { + res := mustUpgrade(t, two, Options{Commits: c.stored}) + for _, w := range c.want { + if !strings.Contains(res.Source, w) { + t.Errorf("want %q in:\n%s", w, res.Source) + } + } + if res.CommitsPinned != c.pinned { + t.Errorf("CommitsPinned = %d, want %d", res.CommitsPinned, c.pinned) + } + var got []string + for _, n := range res.Notes { + got = append(got, n.Message) + if n.Code != "MDL067" { + t.Errorf("note code %q, want MDL067", n.Code) + } + } + if len(got) != len(c.notes) { + t.Fatalf("notes %q, want %d", got, len(c.notes)) + } + for i, w := range c.notes { + if !strings.Contains(got[i], w) { + t.Errorf("note %q does not contain %q", got[i], w) + } + } + }) + } +} + +// Without a project the script is left as written, and each flow with a bare +// commit is reported once, naming MDL067 and -p. +func TestUpgrade_BareCommitWithoutProjectIsReported(t *testing.T) { + res := mustUpgrade(t, commitInLoop+"create or modify microflow M.G ($A: M.E) begin commit $A without events; end;\n", Options{}) + if res.Changed() || res.CommitsPinned != 0 { + t.Errorf("changed without a project:\n%s", res.Source) + } + if len(res.Notes) != 1 { + t.Fatalf("notes = %+v, want one", res.Notes) + } + n := res.Notes[0] + for _, w := range []string{"MyFirstModule.Repro_CommitInLoop", "1 bare `commit` statement means WITH events", "MDL067", "-p"} { + if !strings.Contains(n.Message, w) { + t.Errorf("note %q does not mention %q", n.Message, w) + } + } + if n.Line != 6 { + t.Errorf("note line %d, want 6", n.Line) + } +} diff --git a/mdl/upgrade/date_alias_test.go b/mdl/upgrade/date_alias_test.go new file mode 100644 index 000000000..45ae9381b --- /dev/null +++ b/mdl/upgrade/date_alias_test.go @@ -0,0 +1,56 @@ +// SPDX-License-Identifier: Apache-2.0 + +package upgrade + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/deprecation" +) + +// Rehearsal U1 (ako/mxcli#714, #706): `date` as a type stopped every run of +// mxcli-ledger's domain model, and fmt --upgrade could not get past it, so the +// migration needed a hand edit. It is a deprecated alias of DateTime again, and +// the upgrade writes DateTime — what was always stored. The two lines are the +// ledger's own (01-domain-model.mdl lines 95 and 188 before its migration), and +// the expected output is what the ledger committed after its hand edit. +func TestUpgrade_DateTypeBecomesDateTime(t *testing.T) { + src := "create or modify persistent entity Ledger.Account (\n" + + " /** Date of the most recent successful import */\n" + + " LastImport: date,\n" + + " SortOrder: integer default 0\n" + + ");\n" + + "create or modify persistent entity Ledger.Transaction (\n" + + " TxDate: date not null error 'Transaction date is required',\n" + + " Amount: decimal default 0\n" + + ");\n" + + "create microflow Ledger.F ($D: DATE) returns Date begin declare $x date = $D; return $x; end;\n" + want := "create or modify persistent entity Ledger.Account (\n" + + " /** Date of the most recent successful import */\n" + + " LastImport: DateTime,\n" + + " SortOrder: integer default 0\n" + + ");\n" + + "create or modify persistent entity Ledger.Transaction (\n" + + " TxDate: DateTime not null error message 'Transaction date is required',\n" + + " Amount: decimal default 0\n" + + ");\n" + + "create microflow Ledger.F ($D: DATETIME) returns DateTime begin declare $x DateTime = $D; return $x; end;\n" + res := mustUpgrade(t, src, Options{}) + if res.Source != want { + t.Fatalf("got:\n%s\nwant:\n%s", res.Source, want) + } + if n := res.Rewritten[deprecation.DateType]; n != 5 { + t.Errorf("Rewritten[%s] = %d, want 5 (all: %v)", deprecation.DateType, n, res.Rewritten) + } + if len(res.Unrewritten) != 0 { + t.Errorf("Unrewritten = %+v, want none", res.Unrewritten) + } + if again := mustUpgrade(t, res.Source, Options{}); again.Changed() { + t.Errorf("second upgrade changed the script again: %v", again.Rewritten) + } + // And with the header: the upgraded script is valid mdl 1. + withHeader := mustUpgrade(t, src, Options{AddHeader: true}) + if !withHeader.HeaderAdded { + t.Errorf("the header was not added:\n%s", withHeader.Source) + } +} diff --git a/mdl/upgrade/upgrade.go b/mdl/upgrade/upgrade.go index 8f6ab023f..22cd630b1 100644 --- a/mdl/upgrade/upgrade.go +++ b/mdl/upgrade/upgrade.go @@ -4,7 +4,7 @@ // behind `mxcli fmt --upgrade` (ADR-0011 decision 1, plan item 1.3 of // PROPOSAL_mdl_beta_syntax_freeze.md). // -// It does two kinds of rewrite, and nothing else: +// It does three kinds of rewrite, and nothing else: // // - Every use of a deprecated spelling registered in mdl/deprecation becomes // its canonical spelling. A deprecated spelling is a respelling, so the @@ -15,6 +15,10 @@ // langver.Change) is rewritten to the spelling that keeps its OLD meaning // under the new header, and `mdl ;` is added. The rewrites live in // gatedRewriters (gated.go); the changes with none, in unrewritable. +// - When the caller passes the project (Options.Commits), a bare `commit` +// in a `create or modify` flow whose stored flow commits without events is +// given that flag, so the upgraded script builds what is stored +// (commit_events.go, ako/mxcli#873). // // A rewrite that is more than a keyword swap — a structural deprecation such as // a list operation's call form, or a header-gated construct — is computed from @@ -61,6 +65,12 @@ type Options struct { // a call's result, ako/mxcli#860). Nil when no project is given: such a // construct then blocks the header, as before. Flows FlowTypes + // Commits answers what the project's stored flows commit, for the bare + // `commit $X;` whose meaning #895 changed (ako/mxcli#873): where the + // stored flow commits without events, the upgrade says so. Nil when no + // project is given: each such flow is then reported in Notes instead. + // It applies with and without the header — the change is not gated. + Commits StoredCommits } // FlowTypes is the project a script runs against, as far as the upgrade needs @@ -91,11 +101,16 @@ type Result struct { // Unrewritten lists the deprecated uses left in place because their // registry entry carries no rewrite. Unrewritten []ast.DeprecatedSpelling + // CommitsPinned counts the bare commits given the stored flow's + // `without events` (ako/mxcli#873). + CommitsPinned int + // Notes are what the upgrade left and the author should know. + Notes []Note } // Changed reports whether the upgrade rewrote anything. func (r Result) Changed() bool { - return r.HeaderAdded || len(r.Rewritten) > 0 || len(r.GatedRewritten) > 0 + return r.HeaderAdded || len(r.Rewritten) > 0 || len(r.GatedRewritten) > 0 || r.CommitsPinned > 0 } // Upgrade rewrites src to the canonical language. It returns an error when src @@ -195,6 +210,10 @@ func (u upgrader) upgradeProg(src string, opts Options, parse func(string) (*ast } } + pins, pinned, notes := pinCommitEvents(prog, opts.Commits) + edits = append(edits, pins...) + res.CommitsPinned, res.Notes = pinned, notes + out, err := text.apply(edits) if err != nil { return res, err diff --git a/mdl/visitor/silent_drops_706_test.go b/mdl/visitor/silent_drops_706_test.go index 4966caaf7..5495cd404 100644 --- a/mdl/visitor/silent_drops_706_test.go +++ b/mdl/visitor/silent_drops_706_test.go @@ -61,18 +61,19 @@ func TestRaiseErrorStillParses(t *testing.T) { buildOK(t, "create microflow M.MF () begin raise error; end;") } -// --- 2. float / currency / date --------------------------------------------- +// --- 2. float / currency ---------------------------------------------------- // `float` and `currency` fell through buildDataType to String(unlimited) on an -// attribute and to Void in a microflow; `date` was written as DateTime. +// attribute and to Void in a microflow. `date` was written as DateTime, which +// is what it now means again: a deprecated alias, refused only under mdl 1 +// (visitor_date_alias_test.go, rehearsal U1). func TestRemovedPrimitiveTypesAreRejected(t *testing.T) { for _, tc := range []struct{ name, script, hint string }{ {"float attribute", "create persistent entity M.E ( Amount: float );", "Decimal"}, {"currency attribute", "create persistent entity M.E ( Amount: currency );", "Decimal"}, - {"date attribute", "create persistent entity M.E ( Born: date );", "DateTime"}, {"alter add attribute", "alter entity M.E add attribute Amount: Float;", "Decimal"}, {"microflow parameter", "create microflow M.MF ($A: currency) begin log 'x'; end;", "Decimal"}, - {"microflow return", "create microflow M.MF () returns Date begin return empty; end;", "DateTime"}, + {"microflow return", "create microflow M.MF () returns Currency begin return empty; end;", "Decimal"}, {"declare", "create microflow M.MF () begin declare $d Float = 1; end;", "Decimal"}, {"constant", "create constant M.C type float default 1;", "Decimal"}, } { diff --git a/mdl/visitor/visitor.go b/mdl/visitor/visitor.go index f71551607..ae94e4921 100644 --- a/mdl/visitor/visitor.go +++ b/mdl/visitor/visitor.go @@ -515,6 +515,8 @@ type Builder struct { // deprecations collects every use of a deprecated spelling — see // visitor_deprecations.go. deprecations []ast.DeprecatedSpelling + // flowCommits are the commits in create-or-modify flows (visitor_flow_commits.go). + flowCommits []ast.FlowCommit // widgetNameSpans remembers where each widget's name was written, until its // parent decides whether the model stores it — see // visitor_unstored_widget_name.go. @@ -617,6 +619,7 @@ func build(input string, listen func(*Builder) antlr.ParseTreeListener) (*ast.Pr LanguageVersion: builder.langVersion, LanguageHeaderLine: builder.langHeaderLine, LanguageNotes: builder.langNotes, + FlowCommits: builder.flowCommits, }, allErrors } diff --git a/mdl/visitor/visitor_businessevents.go b/mdl/visitor/visitor_businessevents.go index 89e7a9812..e9ba2ffd1 100644 --- a/mdl/visitor/visitor_businessevents.go +++ b/mdl/visitor/visitor_businessevents.go @@ -108,6 +108,7 @@ func dataTypeSimpleName(ctx parser.IDataTypeContext) string { canonical := map[string]string{ "string": "String", "integer": "Integer", "long": "Long", "decimal": "Decimal", "boolean": "Boolean", "datetime": "DateTime", + "date": "DateTime", // MDL-DEPR160: Mendix has no date-only type "binary": "Binary", } if c, ok := canonical[strings.ToLower(text)]; ok { diff --git a/mdl/visitor/visitor_date_alias_test.go b/mdl/visitor/visitor_date_alias_test.go new file mode 100644 index 000000000..001fd4ea3 --- /dev/null +++ b/mdl/visitor/visitor_date_alias_test.go @@ -0,0 +1,107 @@ +// SPDX-License-Identifier: Apache-2.0 + +package visitor + +import ( + "reflect" + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/deprecation" +) + +// `date` as a type (ako/mxcli#714 rehearsal U1, ako/mxcli#706). Mendix has no +// date-only attribute type, and mxcli always stored `date` as a DateTime. #706 +// made it a hard error in every version, so a headerless script that ran — +// mxcli-ledger's domain model — stopped running, and `fmt --upgrade` could not +// get past it although `date` -> `DateTime` is exactly what was stored. +// +// Without the header it is a deprecated alias of DateTime again (MDL-DEPR160): +// it builds the statement `DateTime` builds, and warns. Under mdl 1 it is +// refused. +func TestDateTypeIsADateTimeAlias(t *testing.T) { + cases := []struct { + old, canonical string + uses int + }{ + {"create persistent entity M.E ( Born: date );", + "create persistent entity M.E ( Born: DateTime );", 1}, + {"create persistent entity M.E ( TxDate: date not null error message 'required', LastImport: Date );", + "create persistent entity M.E ( TxDate: DateTime not null error message 'required', LastImport: DateTime );", 2}, + {"alter entity M.E add attribute Born: DATE;", + "alter entity M.E add attribute Born: DateTime;", 1}, + {"create microflow M.MF ($D: date) returns Date begin return $D; end;", + "create microflow M.MF ($D: DateTime) returns DateTime begin return $D; end;", 2}, + {"create microflow M.MF () begin declare $d date = empty; end;", + "create microflow M.MF () begin declare $d DateTime = empty; end;", 1}, + {"create microflow M.MF () begin $d = create date; end;", + "create microflow M.MF () begin $d = create DateTime; end;", 1}, + } + for _, c := range cases { + t.Run(c.old, func(t *testing.T) { + old := mustBuild(t, c.old) + canon := mustBuild(t, c.canonical) + want := deprecationCodes(canon) + for i := 0; i < c.uses; i++ { + want = append(want, deprecation.DateType) + } + if got := deprecationCodes(old); !sameCodes(got, want) { + t.Errorf("old form recorded %v, want %v", got, want) + } + if !reflect.DeepEqual(old.Statements, canon.Statements) { + t.Errorf("different statements:\n old: %#v\n canon: %#v", old.Statements, canon.Statements) + } + }) + } +} + +// The attribute is built as a DateTime: the type every build since `date` +// existed has stored. +func TestDateTypeBuildsDateTime(t *testing.T) { + e := mustBuild(t, "create persistent entity M.E ( Born: date );").Statements[0].(*ast.CreateEntityStmt) + if got := e.Attributes[0].Type.Kind; got != ast.TypeDateTime { + t.Errorf("attribute kind %v, want DateTime", got) + } +} + +// Under mdl 1 the alias is refused: the header is the opt-in to a language +// that never had it. +func TestDateTypeRefusedUnderMdl1(t *testing.T) { + for _, stmt := range []string{ + "create persistent entity M.E ( Born: date );", + "create microflow M.MF ($D: Date) begin log info 'x'; end;", + } { + _, errs := Build("mdl 1;\n" + stmt) + if len(errs) == 0 { + t.Fatalf("%s: no error under mdl 1", stmt) + } + msg := errs[0].Error() + for _, want := range []string{"line 2", "DateTime", deprecation.DateType} { + if !strings.Contains(msg, want) { + t.Errorf("%s: error %q does not mention %q", stmt, msg, want) + } + } + } +} + +// The fix fmt --upgrade applies replaces exactly the word. +func TestDateTypeFixReplacesTheWord(t *testing.T) { + src := "create persistent entity M.E (\n LastImport: date,\n TxDate: DATE not null\n);" + prog := mustBuild(t, src) + runes := []rune(src) + var got []string + for _, d := range prog.Deprecations { + if d.Code != deprecation.DateType { + continue + } + if d.Fix == nil || len(d.Fix.Edits) != 1 { + t.Fatalf("line %d: want one edit, got %+v", d.Line, d.Fix) + } + e := d.Fix.Edits[0] + got = append(got, string(runes[e.Start:e.Stop])+"->"+e.Text) + } + if want := []string{"date->DateTime", "DATE->DATETIME"}; !reflect.DeepEqual(got, want) { + t.Errorf("edits %v, want %v", got, want) + } +} diff --git a/mdl/visitor/visitor_expr_quoting_test.go b/mdl/visitor/visitor_expr_quoting_test.go index 28d80ecc7..cc424742b 100644 --- a/mdl/visitor/visitor_expr_quoting_test.go +++ b/mdl/visitor/visitor_expr_quoting_test.go @@ -158,3 +158,24 @@ func findWidget(ws []*ast.WidgetV3, name string) *ast.WidgetV3 { } return nil } + +// ako/mxcli#874. The visitor does not know which entity a datasource +// constraint is on, so it must not decide what a three-part name is: it read +// `M.Expense.Status` as an enumeration value and made it the string literal +// 'Status', so the constraint compared two constants. The name is handed to the +// executor as written; the page writer resolves it (storedXPathConstraint). +func TestDatasourceWhere_KeepsThreePartNamesForTheWriter(t *testing.T) { + prog, errs := Build(`create page M.P (title: 'T', layout: Atlas_Core.Atlas_Default) { + gallery g (datasource: database from M.Expense where [M.Expense.Status = M.ExpenseStatus.Submitted]) { + dynamictext t (content: 'x') + } +}`) + if len(errs) > 0 { + t.Fatalf("parse errors: %v", errs) + } + page := prog.Statements[0].(*ast.CreatePageStmtV3) + ds := findWidget(page.Widgets, "g").Properties["DataSource"].(*ast.DataSourceV3) + if want := "[M.Expense.Status = M.ExpenseStatus.Submitted]"; ds.Where != want { + t.Errorf("datasource Where = %q, want %q", ds.Where, want) + } +} diff --git a/mdl/visitor/visitor_flow_commits.go b/mdl/visitor/visitor_flow_commits.go new file mode 100644 index 000000000..db34ecd09 --- /dev/null +++ b/mdl/visitor/visitor_flow_commits.go @@ -0,0 +1,76 @@ +// SPDX-License-Identifier: Apache-2.0 + +package visitor + +import ( + "strings" + + "github.com/antlr4-go/antlr/v4" + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/grammar/parser" +) + +// ExitCommitStatement records a `commit $X` written in the body of a +// `create or modify microflow|nanoflow` (ast.Program.FlowCommits) for +// `fmt --upgrade -p` (ako/mxcli#873). +// +// Since #895 a bare `commit $X;` means WITH events, Studio Pro's default; an +// older mxcli stored the same statement without events. Re-running such a +// script therefore changes what the stored flow does, and inside a loop the +// mdl 1 splice refuses it outright. Which of the two a bare commit should be +// is in the stored flow, not in the script, so the visitor only records where +// each commit is and how to spell the stored flag; the upgrade asks the +// project. +// +// A commit in an `alter microflow` fragment or a plain `create` is not +// recorded: it is new code, which the current default describes. +func (b *Builder) ExitCommitStatement(ctx *parser.CommitStatementContext) { + v := ctx.VARIABLE() + if v == nil { + return + } + flow, nanoflow, ok := enclosingModifiedFlow(ctx) + if !ok { + return + } + c := ast.FlowCommit{ + Line: ctx.GetStart().GetLine(), + Flow: flow, + Nanoflow: nanoflow, + Variable: strings.TrimPrefix(v.GetText(), "$"), + Bare: ctx.WITH() == nil && ctx.WITHOUT() == nil, + WithoutEvents: ctx.WITHOUT() != nil, + } + if c.Bare { + text := " without events" + if kw := ctx.COMMIT().GetText(); kw == strings.ToUpper(kw) { + text = strings.ToUpper(text) + } + at := v.GetSymbol().GetStop() + 1 + c.PinWithoutEvents = &ast.Fix{Edits: []ast.TextEdit{{Start: at, Stop: at, Text: text}}} + } + b.flowCommits = append(b.flowCommits, c) +} + +// enclosingModifiedFlow names the flow a statement is written in when that is +// a `create or modify` (or its alias `create or replace`) microflow or +// nanoflow. +func enclosingModifiedFlow(ctx antlr.RuleContext) (name ast.QualifiedName, nanoflow, ok bool) { + for p := ctx.GetParent(); p != nil; p = p.GetParent() { + var qn parser.IQualifiedNameContext + switch f := p.(type) { + case *parser.CreateMicroflowStatementContext: + qn = f.QualifiedName() + case *parser.CreateNanoflowStatementContext: + qn, nanoflow = f.QualifiedName(), true + default: + continue + } + cs := findParentCreateStatement(p.(antlr.RuleContext)) + if qn == nil || cs == nil || cs.OR() == nil || (cs.MODIFY() == nil && cs.REPLACE() == nil) { + return ast.QualifiedName{}, false, false + } + return buildQualifiedName(qn), nanoflow, true + } + return ast.QualifiedName{}, false, false +} diff --git a/mdl/visitor/visitor_helpers.go b/mdl/visitor/visitor_helpers.go index aee7d31e8..03def101c 100644 --- a/mdl/visitor/visitor_helpers.go +++ b/mdl/visitor/visitor_helpers.go @@ -408,7 +408,9 @@ func buildDataType(ctx parser.IDataTypeContext) ast.DataType { return ast.DataType{Kind: ast.TypeDateTime} } if strings.HasPrefix(text, "DATE") { - return ast.DataType{Kind: ast.TypeDate} + // `date` (MDL-DEPR160): Mendix has no date-only type, and a `date` + // was always stored as a DateTime. + return ast.DataType{Kind: ast.TypeDateTime} } if strings.HasPrefix(text, "AUTONUMBER") { return ast.DataType{Kind: ast.TypeAutoNumber} diff --git a/mdl/visitor/visitor_microflow.go b/mdl/visitor/visitor_microflow.go index 524a9a95d..570a9cc27 100644 --- a/mdl/visitor/visitor_microflow.go +++ b/mdl/visitor/visitor_microflow.go @@ -227,7 +227,9 @@ func buildMicroflowDataType(ctx parser.IDataTypeContext) ast.DataType { return ast.DataType{Kind: ast.TypeDateTime} } if strings.HasPrefix(text, "DATE") { - return ast.DataType{Kind: ast.TypeDate} + // `date` (MDL-DEPR160): Mendix has no date-only type, and a `date` + // was always stored as a DateTime. + return ast.DataType{Kind: ast.TypeDateTime} } if strings.HasPrefix(text, "BINARY") { return ast.DataType{Kind: ast.TypeBinary} @@ -283,7 +285,9 @@ func buildNonListDataType(ctx parser.INonListDataTypeContext) ast.DataType { return ast.DataType{Kind: ast.TypeDateTime} } if strings.HasPrefix(text, "DATE") { - return ast.DataType{Kind: ast.TypeDate} + // `date` (MDL-DEPR160): Mendix has no date-only type, and a `date` + // was always stored as a DateTime. + return ast.DataType{Kind: ast.TypeDateTime} } if strings.HasPrefix(text, "BINARY") { return ast.DataType{Kind: ast.TypeBinary} diff --git a/mdl/visitor/visitor_page_v3.go b/mdl/visitor/visitor_page_v3.go index 507605165..0d354e8d7 100644 --- a/mdl/visitor/visitor_page_v3.go +++ b/mdl/visitor/visitor_page_v3.go @@ -1953,11 +1953,13 @@ func xpathExprToString(expr ast.Expression) string { case *ast.IdentifierExpr: return e.Name case *ast.QualifiedNameExpr: - // XPath constraints run at the database level; enum values must be string literals. - // 3-part names (Module.EnumName.Value) → 'Value'; 2-part names pass through. - if dotIdx := strings.LastIndex(e.QualifiedName.Name, "."); dotIdx >= 0 { - return "'" + e.QualifiedName.Name[dotIdx+1:] + "'" - } + // Written as it is. A three-part name is either an attribute of the + // constrained entity (stored bare) or an enumeration value (stored as a + // string literal), and only the writer knows which entity the + // constraint is on — so the executor decides (storedXPathConstraint). + // Deciding here made `M.Emp.Name` the literal 'Name', in page + // datasources and in every constraint this formatter re-laid out + // (ako/mxcli#874). return e.QualifiedName.String() default: return "" diff --git a/mdl/visitor/visitor_silent_drops.go b/mdl/visitor/visitor_silent_drops.go index b948723e3..8185cb05f 100644 --- a/mdl/visitor/visitor_silent_drops.go +++ b/mdl/visitor/visitor_silent_drops.go @@ -7,11 +7,14 @@ import ( "strings" "github.com/antlr4-go/antlr/v4" + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/deprecation" "github.com/mendixlabs/mxcli/mdl/grammar/parser" ) // Forms that parsed, passed `check`, and were then dropped or stored as -// something else (ako/mxcli#706). Each is refused here, where the parse tree +// something else (ako/mxcli#706). Each is refused here (`date` excepted, see +// recordDateType), where the parse tree // says exactly what was written, rather than removed from the grammar: the // words involved are also keywords-as-identifiers (`Currency`, `Date`, `throw`), // so deleting the alternative would let several of them re-parse as something @@ -44,17 +47,14 @@ func (b *Builder) ExitThrowStatement(ctx *parser.ThrowStatementContext) { // have, or "" when the token is a real type. // // float and currency were Mendix 6 attribute types, removed in Mendix 7 in -// favour of Decimal. There is no date-only attribute type — Date is a DateTime. -// mxcli mapped float/currency to String(unlimited) on an attribute (Void in a -// microflow) and date to DateTime, all without a word. -func removedPrimitiveType(floatTok, currencyTok, dateTok antlr.TerminalNode) (word, replacement string) { +// favour of Decimal. mxcli mapped them to String(unlimited) on an attribute +// (Void in a microflow) without a word, so they are refused. +func removedPrimitiveType(floatTok, currencyTok antlr.TerminalNode) (word, replacement string) { switch { case floatTok != nil: return floatTok.GetText(), "Decimal" case currencyTok != nil: return currencyTok.GetText(), "Decimal" - case dateTok != nil: - return dateTok.GetText(), "DateTime" } return "", "" } @@ -63,25 +63,45 @@ func (b *Builder) rejectRemovedPrimitiveType(ctx antlr.ParserRuleContext, word, if word == "" { return } - why := "Mendix has no " + word + " type; it was removed in Mendix 7 and mxcli stored it as the wrong type" - if replacement == "DateTime" { - why = "Mendix has no date-only type; mxcli silently stored it as DateTime" + b.addError(fmt.Errorf("%s: type `%s` is not supported — Mendix has no %s type; it was removed in Mendix 7 "+ + "and mxcli stored it as the wrong type.\n Write `%s` instead.", + ctxPos(ctx), word, word, replacement)) +} + +// recordDateType records `date` as a type (MDL-DEPR160). There is no date-only +// type in Mendix: `date` was always stored as a DateTime, so it is a +// respelling of DateTime and builds exactly what DateTime builds. #706 refused +// it in every version, which stopped scripts that ran; it now warns without +// the header, fmt --upgrade writes DateTime, and mdl 1 refuses it +// (recordDeprecation, RemovedIn 1). +func (b *Builder) recordDateType(dateTok antlr.TerminalNode, subject string) { + if dateTok == nil { + return + } + tok := dateTok.GetSymbol() + b.recordDeprecation(deprecation.DateType, tok, subject) + repl := "DateTime" + if w := tok.GetText(); w == strings.ToUpper(w) { + repl = "DATETIME" } - b.addError(fmt.Errorf("%s: type `%s` is not supported — %s.\n Write `%s` instead.", - ctxPos(ctx), word, why, replacement)) + b.fixLastDeprecation(deprecation.DateType, &ast.Fix{Edits: []ast.TextEdit{ + {Start: tok.GetStart(), Stop: tok.GetStop() + 1, Text: repl}, + }}, "") } // EnterDataType covers every place a type is written: attributes, microflow // parameters and return types, declare, constants, and the service rules. func (b *Builder) EnterDataType(ctx *parser.DataTypeContext) { - word, repl := removedPrimitiveType(ctx.FLOAT_TYPE(), ctx.CURRENCY_TYPE(), ctx.DATE_TYPE()) + word, repl := removedPrimitiveType(ctx.FLOAT_TYPE(), ctx.CURRENCY_TYPE()) b.rejectRemovedPrimitiveType(ctx, word, repl) + b.recordDateType(ctx.DATE_TYPE(), "") } // EnterNonListDataType is the same check for the create-object type slot. func (b *Builder) EnterNonListDataType(ctx *parser.NonListDataTypeContext) { - word, repl := removedPrimitiveType(ctx.FLOAT_TYPE(), ctx.CURRENCY_TYPE(), ctx.DATE_TYPE()) + word, repl := removedPrimitiveType(ctx.FLOAT_TYPE(), ctx.CURRENCY_TYPE()) b.rejectRemovedPrimitiveType(ctx, word, repl) + b.recordDateType(ctx.DATE_TYPE(), "") } // rejectParenthesisedAssociation refuses `association X (from … to …, opt, …)`. diff --git a/mdl/visitor/visitor_xpath_test.go b/mdl/visitor/visitor_xpath_test.go index 8fd3d8676..f10253835 100644 --- a/mdl/visitor/visitor_xpath_test.go +++ b/mdl/visitor/visitor_xpath_test.go @@ -350,9 +350,12 @@ func TestXPath_EnumValueReference(t *testing.T) { want string }{ { - "3-part enum value becomes string literal for database XPath", + // The serializer does not know the constrained entity, so it cannot + // tell an enumeration value from a qualified attribute; the writer + // makes the value 'Rectified' (storedXPathConstraint, #874). + "3-part name kept for the writer to resolve", + "[Status = BST.ComplianceStatus.Rectified]", "[Status = BST.ComplianceStatus.Rectified]", - "[Status = 'Rectified']", }, { "2-part qualified name preserved", diff --git a/modelsdk/canon/attributeref.go b/modelsdk/canon/attributeref.go index 2d3a7e990..b09d83e8d 100644 --- a/modelsdk/canon/attributeref.go +++ b/modelsdk/canon/attributeref.go @@ -29,12 +29,27 @@ import ( // widget's template parameter inside a data container with no resolvable entity // reached disk bare that way. Any raw write can. // +// A Microflows$ChangeActionItem — a change or create activity's member — holds +// the same kind of identifier and fails the same way: a spliced change on a +// variable whose entity the builder did not know wrote `Name` for +// `System.User.Name`, and `mx check` (11.13.0) could not load the project +// ("Change in has an invalid value '' for property Attribute. The text 'Name' +// is not a valid AttributeIdentifier", ako/mxcli#885). +// // It refuses every bare reference in the unit, stored or new. A stored one // cannot have come from Studio Pro, which cannot load it either; writing it back // keeps the project unloadable, and the message names it so the statement that // rewrites the unit can drop or qualify it. -// BareAttributeRefs names every DomainModels$AttributeRef in raw whose +// attributeIdentifierHolders are the element types whose Attribute property +// is an attribute identifier Mendix parses as it loads the unit. +var attributeIdentifierHolders = map[string]bool{ + "DomainModels$AttributeRef": true, + "Microflows$ChangeActionItem": true, +} + +// BareAttributeRefs names every DomainModels$AttributeRef (or +// Microflows$ChangeActionItem) in raw whose // Attribute is non-empty and not Module.Entity.Attribute, with where it sits // (the nearest named element's Name, then the property path). An empty // Attribute is an unbound slot and is not reported. A document that cannot be @@ -49,7 +64,7 @@ func BareAttributeRefs(raw []byte) []string { if !ok { return } - if t, ok := doc.Lookup("$Type").StringValueOK(); ok && t == "DomainModels$AttributeRef" { + if t, ok := doc.Lookup("$Type").StringValueOK(); ok && attributeIdentifierHolders[t] { if a, ok := doc.Lookup("Attribute").StringValueOK(); ok && a != "" && strings.Count(a, ".") < 2 { bad = append(bad, fmt.Sprintf("%q at %s", a, path)) } @@ -91,6 +106,7 @@ func BareAttributeRefError(unitLabel string, raw []byte) error { return fmt.Errorf("refusing to write unit %s: attribute reference not qualified as "+ "Module.Entity.Attribute — Mendix cannot load a project holding one: %s. Qualify it in the "+ "script; inside a data container whose entity cannot be resolved (e.g. its data-source flow "+ - "is missing) there is nothing to qualify a bare name against", + "is missing), or on a flow variable whose entity is not known, there is nothing to qualify a "+ + "bare name against", unitLabel, strings.Join(bad, "; ")) } diff --git a/modelsdk/canon/attributeref_test.go b/modelsdk/canon/attributeref_test.go index 1f5fee222..35c0beea9 100644 --- a/modelsdk/canon/attributeref_test.go +++ b/modelsdk/canon/attributeref_test.go @@ -47,3 +47,42 @@ func TestBareAttributeRefError(t *testing.T) { t.Errorf("unreadable bytes must yield nothing, got %v", got) } } + +// ako/mxcli#885: a change activity's member is the same kind of identifier. A +// spliced change wrote `Name` for `System.User.Name`, and `mx check` (11.13.0) +// could not load the project ("The text 'Name' is not a valid +// AttributeIdentifier"). +func TestBareAttributeRefError_ChangeActionItem(t *testing.T) { + doc := func(attr, assoc string) []byte { + b, err := bson.Marshal(bson.D{ + {Key: "$Type", Value: "Microflows$Microflow"}, + {Key: "Name", Value: "Repro_ChangeState"}, + {Key: "ObjectCollection", Value: bson.D{{Key: "Objects", Value: bson.A{int32(2), bson.D{ + {Key: "$Type", Value: "Microflows$ActionActivity"}, + {Key: "Action", Value: bson.D{ + {Key: "$Type", Value: "Microflows$ChangeAction"}, + {Key: "Items", Value: bson.A{int32(2), bson.D{ + {Key: "$Type", Value: "Microflows$ChangeActionItem"}, + {Key: "Attribute", Value: attr}, + {Key: "Association", Value: assoc}, + }}}, + }}, + }}}}}, + }) + if err != nil { + t.Fatal(err) + } + return b + } + err := BareAttributeRefError("microflow X", doc("Name", "")) + if err == nil || !strings.Contains(err.Error(), `"Name"`) || !strings.Contains(err.Error(), "Repro_ChangeState") { + t.Fatalf("a bare change member must be refused, naming it and its flow; got %v", err) + } + // Control: a qualified attribute, and an association member (whose + // Attribute is empty), are written. + for _, ok := range [][2]string{{"System.User.Name", ""}, {"", "System.UserRoles"}} { + if err := BareAttributeRefError("microflow X", doc(ok[0], ok[1])); err != nil { + t.Errorf("%v must be accepted: %v", ok, err) + } + } +} diff --git a/modelsdk/mpr/reader.go b/modelsdk/mpr/reader.go index 7a1eda175..d3cf0c288 100644 --- a/modelsdk/mpr/reader.go +++ b/modelsdk/mpr/reader.go @@ -336,6 +336,15 @@ func (r *Reader) SetOverlay(unitID string, data []byte) { r.overlay[unitID] = data } +// overlaid returns the in-memory bytes registered for unitID, if any. +func (r *Reader) overlaid(unitID string) ([]byte, bool) { + if len(r.overlay) == 0 { + return nil, false + } + data, ok := r.overlay[unitID] + return data, ok +} + // ClearOverlay removes a single unitID from the overlay. func (r *Reader) ClearOverlay(unitID string) { delete(r.overlay, unitID) diff --git a/modelsdk/mpr/reader_units.go b/modelsdk/mpr/reader_units.go index 294a0b78a..0b2ee1ea7 100644 --- a/modelsdk/mpr/reader_units.go +++ b/modelsdk/mpr/reader_units.go @@ -109,6 +109,9 @@ func (r *Reader) listUnitsByTypeV1(typeName string) ([]rawUnit, error) { return nil, fmt.Errorf("failed to scan unit row: %w", err) } + if held, ok := r.overlaid(blobToUUID(unitID)); ok { + contents = held + } unitType := getTypeFromContents(contents) if typeName == "" || unitType == typeName { units = append(units, rawUnit{ @@ -223,6 +226,13 @@ func (r *Reader) readMprContents(unitUUID string) ([]byte, error) { return nil, fmt.Errorf("invalid unit UUID: %s", unitUUID) } + // Bytes held in memory (an import buffer, or a deferred run of writes) are + // what the unit currently is; the listings read through here too, so a + // held write is seen by every read, not only by GetRawUnitBytes. + if data, ok := r.overlaid(unitUUID); ok { + return data, nil + } + // Fast path: content cache hit (persistent daemon only). if r.contentCache != nil { if data, ok := r.contentCache[unitUUID]; ok { diff --git a/modelsdk/mpr/writer_core.go b/modelsdk/mpr/writer_core.go index ec92cb7b8..983cb2a30 100644 --- a/modelsdk/mpr/writer_core.go +++ b/modelsdk/mpr/writer_core.go @@ -56,6 +56,11 @@ type Writer struct { // RE-INSERTED under the same ID can be reconciled against what it replaced. // See carryIdentityFromRemovedUnit. removedUnits map[string]removedUnit + + // deferred holds unit updates while a run of statements is judged by its + // net result (writer_deferred.go); nil when no run is open. + deferred map[string]deferredWrite + deferredOrder []string } // removedUnit is everything about a deleted unit that a re-insert has to be @@ -126,9 +131,13 @@ func NewWriterWithReader(r *Reader) *Writer { return &Writer{reader: r} } -// Close closes the writer. +// Close closes the writer, writing any update a deferred run still holds. func (w *Writer) Close() error { - return w.reader.Close() + flushErr := w.FlushDeferredWrites() + if err := w.reader.Close(); err != nil { + return err + } + return flushErr } // Reader returns the underlying reader as a UnitReader interface. @@ -625,6 +634,11 @@ func (w *Writer) updateUnit(unitID string, contents []byte, opts ...canon.Option if w.sessionBuf != nil { return w.sessionBuf(unitID, contents) } + // A run whose writes are judged by its net result holds the update; the + // run's flush reconciles it against what was stored before the run. + if w.holdDeferred(unitID, contents, opts) { + return nil + } contents, unchanged, err := w.reconcileWithStored(unitID, contents, opts...) if err != nil { @@ -905,6 +919,7 @@ func (w *Writer) deleteUnit(unitID string) error { if unitIDBlob == nil { return fmt.Errorf("invalid unit ID: %s", unitID) } + w.dropDeferred(unitID) w.rememberRemovedUnit(unitID) if w.reader.version == MPRVersionV2 { diff --git a/modelsdk/mpr/writer_deferred.go b/modelsdk/mpr/writer_deferred.go new file mode 100644 index 000000000..086fefb4c --- /dev/null +++ b/modelsdk/mpr/writer_deferred.go @@ -0,0 +1,99 @@ +// SPDX-License-Identifier: Apache-2.0 + +package mpr + +import "github.com/mendixlabs/mxcli/modelsdk/canon" + +// Deferred unit writes: a run of statements that each rewrite the same unit is +// judged by where the run ENDS, not by every step on the way. +// +// # Why +// +// Each write is reconciled against what is on disk at the moment it lands +// (ADR-0008). That is the right question for one statement and the wrong one for +// a sequence whose steps undo each other. The measured case is a "reset, then +// authoritative grants" section (ako/mxcli#872, rehearsal W3): +// +// revoke all on entity M.E from M.R; +// grant read *, write * on entity M.E to M.R; +// +// The revoke removes the rule and is written; the grant builds the rule afresh +// and is written against a unit that no longer has it, so there is nothing to +// carry the old rule's $IDs from. Net: the domain model is rewritten on every +// run, the rule re-minted, the project's transaction id moved — although the +// rules it ends with are the ones it started with. +// +// # How +// +// While deferral is on, a unit update is held in memory instead of reaching +// storage, and the reader serves the held bytes (GetRawUnitBytes and the unit +// listings alike), so every later read in the run sees the run's own writes. On +// flush each held unit goes through updateUnit once, and is reconciled against +// what was on disk BEFORE the run: a run that nets to nothing is elided like any +// other no-op, and one that changes something carries the stored identities onto +// what it writes. +// +// Only updates are held. An insert or a delete is not something a later +// statement of the run can undo into a no-op, and deleting a unit drops its held +// write. The caller decides what forms a run and must flush before anything that +// writes through another path (see executor.accessRuleRun). +type deferredWrite struct { + contents []byte + opts []canon.Option +} + +// DeferUnitWrites starts holding unit updates until FlushDeferredWrites. It is +// idempotent; a run already open stays open. +func (w *Writer) DeferUnitWrites() { + if w.deferred == nil { + w.deferred = map[string]deferredWrite{} + } +} + +// FlushDeferredWrites ends the run: every held unit is written once, through the +// ordinary path (reconciliation, elision, the storage-GUID guard), in the order +// it was first written. The first error is returned; the remaining units are +// still attempted, since each is a complete document of its own. +func (w *Writer) FlushDeferredWrites() error { + held, order := w.deferred, w.deferredOrder + w.deferred, w.deferredOrder = nil, nil + var first error + for _, id := range order { + d, ok := held[id] + if !ok { + continue // deleted during the run + } + w.reader.ClearOverlay(id) + if err := w.updateUnit(id, d.contents, d.opts...); err != nil && first == nil { + first = err + } + } + return first +} + +// holdDeferred records an update while a run is open and reports whether it did. +// The latest bytes of a unit win, with the options of the write that produced +// them. +func (w *Writer) holdDeferred(unitID string, contents []byte, opts []canon.Option) bool { + if w.deferred == nil { + return false + } + if _, seen := w.deferred[unitID]; !seen { + w.deferredOrder = append(w.deferredOrder, unitID) + } + held := append([]byte(nil), contents...) + w.deferred[unitID] = deferredWrite{contents: held, opts: opts} + w.reader.SetOverlay(unitID, held) + return true +} + +// dropDeferred forgets a held update for a unit that is being deleted. +func (w *Writer) dropDeferred(unitID string) { + if w.deferred == nil { + return + } + if _, ok := w.deferred[unitID]; ok { + delete(w.deferred, unitID) + w.reader.ClearOverlay(unitID) + } +} diff --git a/modelsdk/mpr/writer_deferred_test.go b/modelsdk/mpr/writer_deferred_test.go new file mode 100644 index 000000000..dfbc3f8b3 --- /dev/null +++ b/modelsdk/mpr/writer_deferred_test.go @@ -0,0 +1,168 @@ +// SPDX-License-Identifier: Apache-2.0 + +package mpr + +import ( + "os" + "testing" + + "go.mongodb.org/mongo-driver/v2/bson" +) + +// ako/mxcli#872: a run of writes that undo each other — a revoke that removes an +// element and a grant that builds it again — is judged by where it ends. Written +// one by one, the second write was reconciled against a unit that no longer had +// the element, so it landed with a freshly minted $ID and moved the transaction +// id although the unit ended as it began. + +const deferredUnitID = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee" + +// ruleDoc stands in for a domain model holding one access rule; withRule=false +// is the same unit after the rule was revoked. +func ruleDoc(t *testing.T, withRule bool, ruleID, template string) []byte { + t.Helper() + doc := bson.D{ + {Key: "$Type", Value: "Rest$ConsumedRestService"}, + {Key: "$ID", Value: bson.Binary{Subtype: 0x00, Data: uuidToBlob("11111111-1111-1111-1111-111111111111")}}, + {Key: "Name", Value: "Svc"}, + } + if withRule { + doc = append(doc, bson.E{Key: "BaseUrl", Value: bson.D{ + {Key: "$Type", Value: "Rest$ValueTemplate"}, + {Key: "$ID", Value: bson.Binary{Subtype: 0x00, Data: uuidToBlob(ruleID)}}, + {Key: "Template", Value: template}, + }}) + } + b, err := bson.Marshal(doc) + if err != nil { + t.Fatalf("marshal: %v", err) + } + return b +} + +func TestDeferredRunThatNetsToNothingWritesNothing(t *testing.T) { + stored := ruleDoc(t, true, "33333333-3333-3333-3333-333333333333", "https://a.test") + w, unitPath := newV2WriterForCommitTest(t, deferredUnitID, stored) + seedTransactionTable(t, w, "before-the-run") + seedUnitRow(t, w, deferredUnitID, "22222222-2222-2222-2222-222222222222", "Documents") + + w.DeferUnitWrites() + revoked := ruleDoc(t, false, "", "") + if err := w.UpdateRawUnit(deferredUnitID, revoked); err != nil { + t.Fatalf("revoke: %v", err) + } + // Every read in the run sees the run's own write: the direct read and the + // listing the executor's GetDomainModel goes through. + if got, _ := w.reader.GetRawUnitBytes(deferredUnitID); string(got) != string(revoked) { + t.Error("GetRawUnitBytes does not see the held write") + } + refs, err := w.reader.ListUnitsByType("Rest$ConsumedRestService") + if err != nil || len(refs) != 1 || string(refs[0].Contents) != string(revoked) { + t.Errorf("the unit listing does not see the held write (err %v, %d units)", err, len(refs)) + } + if onDisk, _ := os.ReadFile(unitPath); string(onDisk) != string(stored) { + t.Error("a held write reached disk before the run ended") + } + + regranted := ruleDoc(t, true, "44444444-4444-4444-4444-444444444444", "https://a.test") + if err := w.UpdateRawUnit(deferredUnitID, regranted); err != nil { + t.Fatalf("grant: %v", err) + } + if err := w.FlushDeferredWrites(); err != nil { + t.Fatalf("flush: %v", err) + } + + if onDisk, _ := os.ReadFile(unitPath); string(onDisk) != string(stored) { + t.Errorf("a run that ended where it began rewrote %d bytes", differingBytes(onDisk, stored)) + } + if got := transactionID(t, w); got != "before-the-run" { + t.Errorf("LastTransactionID = %q after a run that changed nothing", got) + } + if _, written := w.WriteStats(); written != 0 { + t.Errorf("WriteStats counts %d landed writes for a run that changed nothing", written) + } + if _, held := w.reader.overlaid(deferredUnitID); held { + t.Error("the flush left the held bytes in the reader's overlay") + } +} + +// CONTROL: a run that ends somewhere else is written — once — and keeps the +// stored identity of the element it rebuilt. +func TestDeferredRunThatChangesSomethingLandsWithStoredIdentity(t *testing.T) { + stored := ruleDoc(t, true, "33333333-3333-3333-3333-333333333333", "https://a.test") + w, unitPath := newV2WriterForCommitTest(t, deferredUnitID, stored) + seedTransactionTable(t, w, "before-the-run") + + w.DeferUnitWrites() + if err := w.UpdateRawUnit(deferredUnitID, ruleDoc(t, false, "", "")); err != nil { + t.Fatalf("revoke: %v", err) + } + if err := w.UpdateRawUnit(deferredUnitID, ruleDoc(t, true, "44444444-4444-4444-4444-444444444444", "https://b.test")); err != nil { + t.Fatalf("grant: %v", err) + } + if err := w.FlushDeferredWrites(); err != nil { + t.Fatalf("flush: %v", err) + } + + want := ruleDoc(t, true, "33333333-3333-3333-3333-333333333333", "https://b.test") + if onDisk, _ := os.ReadFile(unitPath); string(onDisk) != string(want) { + t.Errorf("want the new template under the stored $ID; %d bytes differ", differingBytes(onDisk, want)) + } + if got := transactionID(t, w); got == "before-the-run" { + t.Error("a run that changed the unit did not move the transaction id") + } + if _, written := w.WriteStats(); written != 1 { + t.Errorf("WriteStats counts %d landed writes, want 1 for the whole run", written) + } +} + +// A unit deleted during the run takes its held write with it. +func TestDeferredWriteIsDroppedWithItsUnit(t *testing.T) { + stored := ruleDoc(t, true, "33333333-3333-3333-3333-333333333333", "https://a.test") + w, unitPath := newV2WriterForCommitTest(t, deferredUnitID, stored) + seedTransactionTable(t, w, "before-the-run") + + w.DeferUnitWrites() + if err := w.UpdateRawUnit(deferredUnitID, ruleDoc(t, false, "", "")); err != nil { + t.Fatalf("revoke: %v", err) + } + if err := w.deleteUnit(deferredUnitID); err != nil { + t.Fatalf("delete: %v", err) + } + if err := w.FlushDeferredWrites(); err != nil { + t.Fatalf("flush: %v", err) + } + if _, err := os.Stat(unitPath); !os.IsNotExist(err) { + t.Errorf("the flush wrote back a deleted unit (stat err %v)", err) + } +} + +// The v1 listing reads contents inline from the Unit table, not through +// readMprContents, so it needs the overlay check of its own. Without it a v1 +// project's GetDomainModel reads what was on disk before the run while +// GetRawUnitBytes reads the run's writes — two views of one unit in one run. +func TestDeferredWriteIsSeenByTheV1Listing(t *testing.T) { + stored := ruleDoc(t, true, "33333333-3333-3333-3333-333333333333", "https://a.test") + r := newTestReaderV1WithUnit(t, deferredUnitID, stored) + if _, err := r.db.Exec(`UPDATE Unit SET ContainerID = ?, ContainmentName = 'Documents' WHERE UnitID = ?`, + uuidToBlob("22222222-2222-2222-2222-222222222222"), uuidToBlob(deferredUnitID)); err != nil { + t.Fatalf("seed unit row: %v", err) + } + + held := ruleDoc(t, false, "", "") + r.SetOverlay(deferredUnitID, held) + refs, err := r.ListUnitsByType("Rest$ConsumedRestService") + if err != nil || len(refs) != 1 { + t.Fatalf("listing: err %v, %d units", err, len(refs)) + } + if string(refs[0].Contents) != string(held) { + t.Error("the v1 unit listing does not see the held write") + } + + // Control: with the overlay cleared the listing reads the stored row again. + r.ClearOverlay(deferredUnitID) + refs, err = r.ListUnitsByType("Rest$ConsumedRestService") + if err != nil || len(refs) != 1 || string(refs[0].Contents) != string(stored) { + t.Errorf("after ClearOverlay the v1 listing does not read the stored row (err %v)", err) + } +}