From e6596c577352f40af8cd802361900c8943e63177 Mon Sep 17 00:00:00 2001 From: Ako Date: Wed, 9 Sep 2026 07:55:27 +0000 Subject: [PATCH] feat(navigation): author ON SYNC ERROR THROW|CONTINUE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Studio Pro's "Throw error when server rejects objects during synchronization", stored as the profile-level ThrowPartialSyncError. The last unauthorable field in the offline sync dialog. Spelled with the phrase MDL already uses for failure handling — a microflow's ON ERROR CONTINUE / ON ERROR ROLLBACK — so it needs no new token and reads as something already learned. The alternatives were worse for concrete reasons. camelCase (`on syncReject throw`) cannot work: MDL's lexer is case-insensitive across all 563 keyword tokens, so syncReject and syncreject lex identically and the casing would be a distinction the grammar cannot enforce; there is also no camelCase precedent, SIGN_OUT being the one compound keyword. And REJECT, the platform's own word, appears ~500 times across the examples and skills, because approve/reject is one of the commonest things a workflow models — not a word to claim as a keyword for one checkbox. Neither modelsdk/gen nor generated/metamodel declares the property (zero occurrences in each, measured on ako/TestApp), so there is no typed accessor: it is read from element.Base.Raw() and written as a raw key on both engines. Web profiles only — both of TestApp's carry it; whether a native profile does is unmeasured, so nativeNavProfileFromGen is left alone rather than given a default nothing has verified. The spec field is a POINTER. The property is a bare bool with no unset value of its own, so a non-pointer would reset the flag on every rewrite that never mentions the clause — silently, on a property nothing else reports. Absent reads as TRUE, matching every reference profile and Studio Pro's checked-by- default box, so a document lacking the key is not flipped to "do not throw". DESCRIBE emits the clause only when it is not the default, so navigation scripts do not all gain a line that says what would happen anyway. A control caught a hole in the test rather than in the code. The first fixture stored false, which is also the zero value, so a writer that ignored the spec entirely produced identical output and the assertion held either way — the control failed to fail. The test now exercises BOTH stored values, and with the pointer check removed reports "changed a stored true to false". Second time in this feature after CompatibilityMode, and recorded as a finding: when every real document agrees on a value, a fixture drawn from real documents cannot test preservation. Verified on ako/TestApp: the flag flips to false, the untouched Responsive profile stays true, a rewrite that never mentions the clause leaves false intact, and mx check reports 0 errors. Co-Authored-By: Claude Opus 5 --- .../fix-issue/findings/mdl-backend.jsonl | 1 + .../skills/mendix/manage-navigation/SKILL.md | 14 +++ CLAUDE.md | 2 +- cmd/mxcli/syntax/features_misc.go | 8 ++ .../reference/navigation/alter-navigation.md | 8 ++ .../doctype-tests/navigation-offline-sync.mdl | 5 ++ mdl/ast/ast_navigation.go | 24 +++-- mdl/backend/modelsdk/navigation_read.go | 25 ++++++ .../modelsdk/navigation_throw_sync_test.go | 89 +++++++++++++++++++ mdl/backend/modelsdk/navigation_write.go | 8 ++ mdl/backend/mpr/convert.go | 1 + mdl/backend/mpr/convert_roundtrip_test.go | 4 +- mdl/executor/cmd_navigation.go | 9 ++ mdl/grammar/MDLParser.g4 | 11 +++ mdl/types/navigation.go | 12 +++ mdl/visitor/visitor_navigation.go | 5 ++ sdk/mpr/parser_misc.go | 7 ++ sdk/mpr/writer_navigation.go | 7 ++ 18 files changed, 228 insertions(+), 12 deletions(-) create mode 100644 mdl/backend/modelsdk/navigation_throw_sync_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-backend.jsonl b/.claude/skills/fix-issue/findings/mdl-backend.jsonl index fc1a2e7945..9f98f60b9f 100644 --- a/.claude/skills/fix-issue/findings/mdl-backend.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-backend.jsonl @@ -85,3 +85,4 @@ {"area": "mdl/backend", "date": "2026-09-08", "symptom": "A property Studio Pro writes on an offline entity config (CompatibilityMode) was read from the model and silently discarded, so any future write path would have dropped it with no error and no mx check failure", "cause": "types.NavOfflineEntity carried three of the four properties Studio Pro actually writes. TestFieldCountDrift, which exists to catch exactly this on hand-copied structs, did not list NavOfflineEntity or NavigationProfile — so adding the field left the guard passing vacuously", "file": "`mdl/types/navigation.go`, `mdl/backend/mpr/convert.go`, `mdl/backend/mpr/convert_roundtrip_test.go`", "insight": "A drift guard is only worth what its list covers, and a guard that passes on a struct it does not know about is worse than none — it reads as coverage. Check the guard names your type before trusting a green run. Also: measure which properties are actually WRITTEN before deciding what to carry — gen declared six here and the reference document had four, with DownloadMode and ShouldDownload occurring zero times, so the risk was inverted from the expected one (writing a property Studio Pro fills in on load, not dropping one)", "refs": ["ako/TestApp", "ako/mxcli#413"]} {"area": "mdl/backend", "date": "2026-09-08", "symptom": "An offline navigation profile authored by mxcli builds, routes and installs as a PWA, and shows an empty app — every gate green", "cause": "MDL had no syntax for offline synchronization, so a created offline profile got an empty OfflineEntityConfigs list. A Mendix offline profile downloads nothing until each entity has a sync mode; `mx check` reports 0 errors either way because an empty list is valid", "file": "`mdl/grammar/MDLParser.g4` (navSyncDef), `mdl/backend/modelsdk/navigation_write.go`, `sdk/mpr/writer_navigation.go`", "insight": "Creating a document kind is not the same as being able to configure it, and the gap is invisible to every static check — the symptom is an empty screen at runtime. When adding a profile/document kind, ask what makes it DO anything, not just what makes it exist. The write is an overlay keyed by entity so CompatibilityMode (stored, unauthorable) survives; building the element from the spec alone would clear it silently, the access-rule defect again", "refs": ["ako/TestApp", "PROPOSAL_offline_sync_configuration.md"]} {"area": "mdl/backend", "date": "2026-09-08", "symptom": "`alter page P { set PageSize = 10 on }` errors `pluggable property \"PageSize\" not found` on a grid that `create page \u2026 (PageSize: 20)` had just written and the app really pages at. `mxcli check --references` passes the script, so it fails only at exec, after earlier statements have landed; DESCRIBE PAGE prints the same capitalised `PageSize:`, so describe \u2192 edit \u2192 exec produced a script mxcli refused to run", "cause": "A pluggable property key is lowerCamel in the widget template (`pageSize`). CREATE resolves the author's spelling case-INsensitively (widget engine `lookupProperty`, and `WidgetV3.GetStringProp` before it); ALTER went through `setPluggableWidgetPropertyMut`, which compared the template key byte-for-byte, so only the exact `pageSize` worked. Direct sequel to Findings #1 (2026-07-27), which fixed the same class for FIRST-CLASS props and deliberately left the pluggable fallback case-sensitive with the comment 'template keys must match the template exactly'", "file": "`mdl/backend/pagemutator/mutator.go` (`setPluggableWidgetPropertyMut`)", "insight": "`strings.EqualFold` against the widget's own PropertyTypes. **The disproven belief is the reusable part**: keys are STORED case-sensitively, which is not a reason to MATCH them that way \u2014 the resolver searches one object type's PropertyTypes, and across every shipped template and definition that scope holds no two keys differing only in case (96 scopes, 1208 keys, 0 collisions). Measure the ambiguity before assuming it; here there was none, and the assumption cost a whole verb. **Cheapest localiser**: run the failing statement with the template's own casing \u2014 `set pageSize` succeeded where `set PageSize` failed, in ONE measurement, on the same widget in the same project. Both engines share `pagemutator`, so an engine split says nothing here (verified: modelsdk and legacy both fixed by the one change). Tests `TestSetPluggableProperty_MatchesTemplateKeyRegardlessOfCase` (+ typo-still-errors control, + `TestPluggablePropertyKeysAreUniqueIgnoringCase` pinning the no-collision argument); repro `mdl-examples/bug-tests/alter-page-pluggable-property-casing.mdl`; verified 0 errors on `mx check` 11.13.0. Two reporter claims did NOT hold: `describe page` DOES emit PageSize, but only when it differs from the widget default 20 (deliberate, so describe round-trips) \u2014 at the default it is omitted, which reads as 'describe cannot show it'. Still open and separate: `check --references` does not resolve pluggable property names at all, so a genuine typo (`PagSize`) still checks clean and fails at exec", "refs": ["mendixlabs/mxcli#1069"]} +{"area": "mdl/backend", "date": "2026-09-09", "symptom": "A control that should have proven a guard was load-bearing PASSED with the guard removed — the test could not distinguish a correct writer from one that reset the property on every write", "cause": "The fixture stored `false` for a boolean property, and false is also the zero value. A writer that ignored the spec entirely wrote false; the correct writer preserved false. Identical output, so the assertion held either way", "file": "`mdl/backend/modelsdk/navigation_throw_sync_test.go`", "insight": "For a BOOLEAN property, a preservation test must exercise BOTH stored values — the non-zero one is the only case that can fail. This is the second time in one feature: CompatibilityMode needed a synthetic `true` because all seven reference configs carry false. The generalisation: when every real document agrees on a value, the fixture drawn from real documents cannot test preservation, and a synthetic counter-case is not optional. The tell is a control that fails to fail — if stubbing the guard leaves the suite green, the test is measuring nothing, and that is worse than no test because it reads as coverage", "refs": ["ako/mxcli#413", "ThrowPartialSyncError"]} diff --git a/.claude/skills/mendix/manage-navigation/SKILL.md b/.claude/skills/mendix/manage-navigation/SKILL.md index 79c713bce1..2c883b84c0 100644 --- a/.claude/skills/mendix/manage-navigation/SKILL.md +++ b/.claude/skills/mendix/manage-navigation/SKILL.md @@ -260,6 +260,20 @@ The `sync` row names the profile. Every mode produces one, **including the modes that download nothing**: a profile with `sync X never` still names `X`, so renaming or dropping it leaves the configuration dangling. +**Errors when the server rejects an object.** Studio Pro's *"Throw error when +server rejects objects during synchronization"* checkbox: + +```sql +create or replace navigation PhoneOffline + home page MyModule.Mobile_Dashboard + on sync error continue; -- default is `throw` +``` + +It uses the phrase MDL already has for failure handling — a microflow's +`on error continue` — rather than a keyword of its own. Omitting the clause +leaves the stored value alone; `describe navigation` emits it only when it is +not the default, so existing scripts stay quiet. + **Compatibility mode has no syntax.** mxcli reads it, preserves it across a rewrite, and `describe navigation` flags any entity that has it on — it is never silently dropped. diff --git a/CLAUDE.md b/CLAUDE.md index e291e9b03d..0433a4818e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -815,7 +815,7 @@ Full syntax tables for all MDL statements (microflows, pages, security, navigati ## Current Implementation Status **Implemented:** -- Offline synchronization (`CREATE NAVIGATION … SYNC (…)`): an offline navigation profile downloads **nothing** until each entity has a sync mode, so a profile mxcli created built, routed and installed as a PWA and showed an **empty app** — with `mxcli check`, `exec` and `mx check` all clean. The six mode words are the members Mendix stores, **not** Studio Pro's captions (its "All Objects" is `ALL`, its "By XPath" is `WHERE`), and a caption is refused rather than written — the CE0463 gallery defect wearing a different hat. `WHERE` takes the XPath in **brackets**, verbatim: the quoted form doubles every quote, and a stored constraint already carries Mendix's own escaping, so the two compose into runs of six (mendixlabs/mxcli#750, and `PROPOSAL_first_class_expressions.md`). The write is an **overlay keyed by entity**, so `CompatibilityMode` — stored, unauthorable — survives a rewrite; every reference config carries `false`, so only a synthetic `true` case distinguishes a correct writer from one that always emits `false`. `DownloadMode`/`ShouldDownload` are deliberately **not** written though gen declares them: zero occurrences in ako/TestApp, and a property Studio Pro fills in on load is one whose emission makes a document Studio Pro cannot open. Creating the *profile* stays modelsdk-only (a fourteen-key document pinned to a Studio Pro reference); the SYNC block works on both engines. Both halves are in the catalog: `CATALOG.OFFLINE_ENTITY_CONFIGS` holds one row per configured entity (the profile's `OfflineEntityCount` said how many and nothing else), and a configured entity emits a **`sync` edge** into `CATALOG.REFS` so `show references to Mod.Entity` names the profiles that download it. Every mode gets an edge, **including the ones that download nothing** — a profile with `sync X never` still names X, so renaming or dropping it leaves the config dangling, which is exactly what the edge exists to reveal. See `.claude/skills/mendix/manage-navigation/SKILL.md` and `docs/11-proposals/PROPOSAL_offline_sync_configuration.md` +- Offline synchronization (`CREATE NAVIGATION … SYNC (…)`): an offline navigation profile downloads **nothing** until each entity has a sync mode, so a profile mxcli created built, routed and installed as a PWA and showed an **empty app** — with `mxcli check`, `exec` and `mx check` all clean. The six mode words are the members Mendix stores, **not** Studio Pro's captions (its "All Objects" is `ALL`, its "By XPath" is `WHERE`), and a caption is refused rather than written — the CE0463 gallery defect wearing a different hat. `WHERE` takes the XPath in **brackets**, verbatim: the quoted form doubles every quote, and a stored constraint already carries Mendix's own escaping, so the two compose into runs of six (mendixlabs/mxcli#750, and `PROPOSAL_first_class_expressions.md`). The write is an **overlay keyed by entity**, so `CompatibilityMode` — stored, unauthorable — survives a rewrite; every reference config carries `false`, so only a synthetic `true` case distinguishes a correct writer from one that always emits `false`. `DownloadMode`/`ShouldDownload` are deliberately **not** written though gen declares them: zero occurrences in ako/TestApp, and a property Studio Pro fills in on load is one whose emission makes a document Studio Pro cannot open. Creating the *profile* stays modelsdk-only (a fourteen-key document pinned to a Studio Pro reference); the SYNC block works on both engines. `ON SYNC ERROR THROW|CONTINUE` writes `ThrowPartialSyncError`, a property **neither generated source declares** (zero occurrences in gen and in generated/metamodel), so it is read from `element.Base.Raw()` and written as a raw key. The spec field is a **pointer**: the property is a bare bool with no unset value, so a non-pointer would reset it on every rewrite that never mentions the clause. Absent reads as **true**, matching every reference profile and Studio Pro's checked-by-default box. Both halves are in the catalog: `CATALOG.OFFLINE_ENTITY_CONFIGS` holds one row per configured entity (the profile's `OfflineEntityCount` said how many and nothing else), and a configured entity emits a **`sync` edge** into `CATALOG.REFS` so `show references to Mod.Entity` names the profiles that download it. Every mode gets an edge, **including the ones that download nothing** — a profile with `sync X never` still names X, so renaming or dropping it leaves the config dangling, which is exactly what the edge exists to reveal. See `.claude/skills/mendix/manage-navigation/SKILL.md` and `docs/11-proposals/PROPOSAL_offline_sync_configuration.md` - Project brain (`mxcli brain init/capture/staged/promote/drop/check/show`): an **opt-in** store in `docs/brain/` for the project knowledge mxcli cannot compute. The governing rule is that anything derivable from the model is answered by a command and never written down — a note that transcribes the model disagrees with it silently. Records shard by **anchor scope**: an entry's first anchor names its file (`@Sales.Order` → `modules/Sales.md`), an anchorless entry is cross-cutting (`project.md`), and there is no index to maintain because the module prefix *is* the file name. That is what makes the cap per-shard rather than a project-wide budget, and lets a session load `project.md` plus the modules it is touching. `check` answers two independent questions: each anchor is **resolved / not found / not indexable** — only the middle one fails, and the third exists because the catalog's `objects` view covers the describable types only, so a scheduled event would otherwise read as *missing* (separated with `FindDocumentUnit`, which cannot miss a kind because it never asks what kind anything is). Misfiling is a **second axis, not a fourth state**: every anchor can resolve and the entry still be in the wrong file, and it is only decided when something resolved — judging it on an all-not-indexable entry reintroduced the same false staleness through the other axis (caught by a test, with the guard stubbed as the control). An agent `capture`s to a git-ignored queue and a person `promote`s; the queue is deliberately **not** sharded, because routing it would force the file decision before a human has looked at the entry. `mxcli lint` prints the unpromoted-queue count, because a report only `brain check` prints is a report nothing demands. Sizes are computed by `brain show` and never written into a committed file. A second record kind, **requirement**, lives in `plan/.md` and inverts the anchor's meaning: a decision's anchor points backward (not resolving = stale, fails), a requirement's points forward (not resolving = not built yet, passes). Measured: filed as an ordinary entry, one unbuilt requirement takes `brain check` to exit 1 — which is why it is a separate kind rather than more entries in the same files. That inversion is also what makes `brain plan` a real progress report: a requirement is *built* when its anchors resolve, so creating the microflow it names moves the count with the plan file untouched (measured 0/1 → 1/0). A status written beside a requirement is therefore refused by the skill, not just discouraged. Slices are ordered by name (`01-accounts`), span modules by design (so misfiling does not apply), and carry a generous cap that enforces the slicing discipline — a slice too long to read should be split. A third kind, **open question** (`--open`), records what is *not* decided; its anchors are deliberately **not** checked, since the question is often whether the thing should exist at all — measured, the identical anchor exits 1 as a decision and 0 as a question. `brain resolve` converts one into a decision in place, keeping its id and position and starting to check its anchors, which is the transition the kind exists for. Unanswered questions are reported by `brain check` and by `mxcli lint`. The skill also gives capture a **trigger** rather than good intentions — a correction you have had to make twice — because the decisions half otherwise under-fills while the plan half fills at bootstrap. `bootstrap-app` asks for requirements at the interview and records them by default. Package: `cmd/mxcli/brain/`. See `docs-site/src/tools/project-brain.md` and `docs/11-proposals/PROPOSAL_project_brain.md` - Default styling + runtime theme switching (`mxcli theme list/show/create/apply/remove/switcher`, `mxcli new --theme`): three embedded themes (**signal** light-first, **ledger** light-first, **console** dark-first), each a palette in `theme/web/custom-variables.scss` + a shared Atlas wiring partial + a theme partial imported from `theme/web/main.scss` (which compiles last), plus vendored fonts. **No model changes**, so it hot-applies under `run --local --watch` and cannot affect a build. Generated regions are digest-fenced: a block carrying local edits is refused rather than overwritten. Applying a theme removes the previous one. `--variant auto` (default) ships both palettes — the app follows `prefers-color-scheme` before first paint and honours a `theme-light`/`theme-dark` class on ``; `light`/`dark` bakes one. `theme switcher install` is the only part that writes to the model (JS actions + a nanoflow for a toggle button). A project can add its own themes under `theme/mxcli-themes//` (committed, not compiled); `theme create [--from ]` scaffolds one from an existing theme, renaming the identifiers built from the name and optionally seeding the palette from `--mxt-*` declarations in a design artifact. A local theme shadows a built-in of the same name. Package: `cmd/mxcli/theme/`. See `docs/11-proposals/PROPOSAL_default_styling.md` - MPR v1/v2 reading and writing diff --git a/cmd/mxcli/syntax/features_misc.go b/cmd/mxcli/syntax/features_misc.go index b1cc2d3efe..9dc32acdf4 100644 --- a/cmd/mxcli/syntax/features_misc.go +++ b/cmd/mxcli/syntax/features_misc.go @@ -154,6 +154,7 @@ DISCONNECT;`, "navigation profile", "phone profile", "tablet profile", "offline profile", "offline navigation", "sync", "synchronization", "offline sync", "offline entity", "pwa", "download mode", + "throw error", "sync error", "partial sync", "server rejects", }, Syntax: `CREATE OR REPLACE NAVIGATION HOME PAGE Module.Page @@ -164,6 +165,7 @@ DISCONNECT;`, MENU ITEM 'Label' PAGE Module.Page [ICON Module.IconCollection.Name]; MENU 'Group' [ICON Module.IconCollection.Name] ( ... ); )] + [ON SYNC ERROR THROW|CONTINUE] [SYNC ( SYNC Module.Entity ONLINE; SYNC Module.Entity ALL; @@ -214,6 +216,12 @@ DISCONNECT;`, -- and a stored constraint already carries Mendix's own escaping, so the two -- compose into runs of six quotes. DESCRIBE emits the bracket form. -- +-- ON SYNC ERROR is Studio Pro's "Throw error when server rejects objects +-- during synchronization", and defaults to THROW. It uses the phrase MDL +-- already has for failure handling (a microflow's ON ERROR CONTINUE) rather +-- than a new keyword. OMITTING it leaves the stored value alone; DESCRIBE emits +-- it only when it is not the default. +-- -- The block REPLACES the stored list, the way MENU replaces the menu. An -- entity's compatibility-mode flag has no syntax and is preserved across the -- rewrite untouched; DESCRIBE NAVIGATION flags it rather than dropping it. diff --git a/docs-site/src/reference/navigation/alter-navigation.md b/docs-site/src/reference/navigation/alter-navigation.md index 4f84cd32de..a416715e22 100644 --- a/docs-site/src/reference/navigation/alter-navigation.md +++ b/docs-site/src/reference/navigation/alter-navigation.md @@ -11,6 +11,7 @@ CREATE OR REPLACE NAVIGATION profile [ MENU ( menu_items ) ] + [ ON SYNC ERROR { THROW | CONTINUE } ] [ SYNC ( sync_rules ) ] @@ -115,6 +116,13 @@ quoted `WHERE 'xpath'` still parses, but every quote inside it must be doubled. The block replaces the stored list, the way `MENU` replaces the menu. Omitting it leaves the stored configuration alone. +`ON SYNC ERROR THROW | CONTINUE` is Studio Pro's *"Throw error when server +rejects objects during synchronization"*, and defaults to `THROW`. It reuses the +phrase MDL already has for failure handling — a microflow's `ON ERROR CONTINUE` +— rather than introducing a keyword of its own. Omitting the clause leaves the +stored value alone, and `DESCRIBE NAVIGATION` emits it only when it is not the +default. + An entity's *compatibility mode* flag has no MDL syntax. It is read, preserved across a rewrite, and reported by `DESCRIBE NAVIGATION` — never silently dropped. diff --git a/mdl-examples/doctype-tests/navigation-offline-sync.mdl b/mdl-examples/doctype-tests/navigation-offline-sync.mdl index 9ced3e3ee1..e0fb918010 100644 --- a/mdl-examples/doctype-tests/navigation-offline-sync.mdl +++ b/mdl-examples/doctype-tests/navigation-offline-sync.mdl @@ -51,6 +51,11 @@ create page "OfflineSync"."Mobile_Home" -- names plus "Offline". create or replace navigation "PhoneOffline" home page "OfflineSync"."Mobile_Home" + -- Studio Pro's "Throw error when server rejects objects during + -- synchronization". Spelled with the phrase MDL already uses for failure + -- handling (a microflow's ON ERROR CONTINUE), and omitting the clause leaves + -- the stored value alone rather than resetting it. + on sync error continue sync ( -- Fetched from the server; never held on the device. sync "OfflineSync"."Setting" online; diff --git a/mdl/ast/ast_navigation.go b/mdl/ast/ast_navigation.go index 001388e9f1..352508efdf 100644 --- a/mdl/ast/ast_navigation.go +++ b/mdl/ast/ast_navigation.go @@ -7,15 +7,21 @@ import "github.com/mendixlabs/mxcli/mdl/types" // AlterNavigationStmt represents: CREATE [OR REPLACE] NAVIGATION [clauses...] // This is a full-replacement command: omitted clauses clear that section. type AlterNavigationStmt struct { - ProfileName string // e.g. "Responsive" - HomePages []NavHomePageDef // HOME PAGE/MICROFLOW ... [FOR role] - LoginPage *QualifiedName // LOGIN PAGE ... - NotFoundPage *QualifiedName // NOT FOUND PAGE ... - MenuItems []NavMenuItemDef // MENU (...) block - HasMenuBlock bool // true if MENU (...) was present (even if empty → clears menu) - SyncEntries []NavSyncDef // SYNC (...) block — offline synchronization - HasSyncBlock bool // true if SYNC (...) was present (even if empty → clears the list) - CreateOrModify bool // true if CREATE OR REPLACE/MODIFY was used + ProfileName string // e.g. "Responsive" + HomePages []NavHomePageDef // HOME PAGE/MICROFLOW ... [FOR role] + LoginPage *QualifiedName // LOGIN PAGE ... + NotFoundPage *QualifiedName // NOT FOUND PAGE ... + MenuItems []NavMenuItemDef // MENU (...) block + HasMenuBlock bool // true if MENU (...) was present (even if empty → clears menu) + SyncEntries []NavSyncDef // SYNC (...) block — offline synchronization + HasSyncBlock bool // true if SYNC (...) was present (even if empty → clears the list) + // ThrowSyncError is ON SYNC ERROR THROW|CONTINUE, and is a POINTER so an + // omitted clause leaves the stored value alone. A plain bool would make + // every rewrite that never mentions it reset the flag to false — the + // guard-don't-drop failure, in the one property on this statement that is + // a bare boolean and so has no "unset" value of its own. + ThrowSyncError *bool + CreateOrModify bool // true if CREATE OR REPLACE/MODIFY was used } func (s *AlterNavigationStmt) isStatement() {} diff --git a/mdl/backend/modelsdk/navigation_read.go b/mdl/backend/modelsdk/navigation_read.go index 5c3cd6ac5b..b05e9dfc4c 100644 --- a/mdl/backend/modelsdk/navigation_read.go +++ b/mdl/backend/modelsdk/navigation_read.go @@ -95,6 +95,7 @@ func webNavProfileFromGen(p *genNav.NavigationProfile) *types.NavigationProfile } } appendOfflineEntities(profile, p.OfflineEntityConfigsItems()) + profile.ThrowPartialSyncError = throwPartialSyncError(p.Raw()) return profile } @@ -342,3 +343,27 @@ func textOf(el element.Element) string { } return "" } + +// throwPartialSyncError reads a property NEITHER modelsdk/gen NOR +// generated/metamodel declares — measured on ako/TestApp, zero occurrences in +// each — so there is no typed accessor to call. element.Base keeps the raw +// document, which is what makes reading it possible without a second load. +// +// Absent means TRUE: every reference profile carries true and Studio Pro's box +// is checked by default, so a document without the key must not be read as +// "do not throw". +// +// Web profiles only. Both of ako/TestApp's carry it; whether a native profile +// does is unmeasured, and nativeNavProfileFromGen is deliberately left alone +// rather than given a default nothing has verified. +func throwPartialSyncError(raw bson.Raw) bool { + v, err := raw.LookupErr("ThrowPartialSyncError") + if err != nil { + return true + } + b, ok := v.BooleanOK() + if !ok { + return true + } + return b +} diff --git a/mdl/backend/modelsdk/navigation_throw_sync_test.go b/mdl/backend/modelsdk/navigation_throw_sync_test.go new file mode 100644 index 0000000000..2ecff36d29 --- /dev/null +++ b/mdl/backend/modelsdk/navigation_throw_sync_test.go @@ -0,0 +1,89 @@ +// SPDX-License-Identifier: Apache-2.0 + +package modelsdkbackend + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/types" + // This package uses BOTH driver majors: navigation_write.go is on v1 and + // navigation_read.go on v2. The aliases are what keep a test that spans + // the write and read paths from silently mixing them. + bsonv1 "go.mongodb.org/mongo-driver/bson" + bsonv2 "go.mongodb.org/mongo-driver/v2/bson" +) + +func boolPtr(b bool) *bool { return &b } + +func throwOf(t *testing.T, doc bsonv1.D) any { + t.Helper() + for _, e := range doc { + if e.Key == "ThrowPartialSyncError" { + return e.Value + } + } + return nil +} + +// The spec field is a POINTER, and this is why. ThrowPartialSyncError is a bare +// bool with no unset value of its own, so a non-pointer spec would make every +// rewrite that never mentions the clause reset the stored flag — silently, on a +// property neither generated source declares and nothing else reports. +func TestThrowSyncErrorIsLeftAloneWhenTheStatementIsSilent(t *testing.T) { + // BOTH stored values, because false is also the zero value: a writer that + // reset the flag on every rewrite would be indistinguishable from a correct + // one if the fixture only ever stored false. Same trap as CompatibilityMode, + // and it was found by a control that failed to fail. + for _, storedValue := range []bool{true, false} { + stored := bsonv1.D{ + {Key: "Name", Value: "TabletOffline"}, + {Key: "ThrowPartialSyncError", Value: storedValue}, + } + out := navPatchWebProfile(stored, types.NavigationProfileSpec{}) + if got := throwOf(t, out); got != storedValue { + t.Errorf("a rewrite that never mentioned the clause changed a stored %v to %v", + storedValue, got) + } + } + + stored := bsonv1.D{ + {Key: "Name", Value: "TabletOffline"}, + {Key: "ThrowPartialSyncError", Value: false}, + } + + // Control in the other direction: when the statement DOES say something, + // it must actually be written — otherwise the first assertion passes for + // a writer that ignores the field entirely. + out := navPatchWebProfile(stored, types.NavigationProfileSpec{ThrowSyncError: boolPtr(true)}) + if got := throwOf(t, out); got != true { + t.Errorf("`on sync error throw` did not write: %v", got) + } + out = navPatchWebProfile(stored, types.NavigationProfileSpec{ThrowSyncError: boolPtr(false)}) + if got := throwOf(t, out); got != false { + t.Errorf("`on sync error continue` did not write: %v", got) + } +} + +// Absent must read as true, not as the zero value. Every reference profile +// carries true and Studio Pro's box is checked by default, so defaulting to +// false would silently turn off error reporting for a document that simply +// predates the key. +func TestAbsentThrowPartialSyncErrorReadsAsTrue(t *testing.T) { + empty, err := bsonv2.Marshal(bsonv2.D{{Key: "Name", Value: "Responsive"}}) + if err != nil { + t.Fatal(err) + } + if !throwPartialSyncError(bsonv2.Raw(empty)) { + t.Error("an absent key must read as true") + } + + // Control: a present false is honoured, so the default is not masking the + // read. + set, err := bsonv2.Marshal(bsonv2.D{{Key: "ThrowPartialSyncError", Value: false}}) + if err != nil { + t.Fatal(err) + } + if throwPartialSyncError(bsonv2.Raw(set)) { + t.Error("a stored false must be read as false") + } +} diff --git a/mdl/backend/modelsdk/navigation_write.go b/mdl/backend/modelsdk/navigation_write.go index 6e0d3cccd3..891b44dbe5 100644 --- a/mdl/backend/modelsdk/navigation_write.go +++ b/mdl/backend/modelsdk/navigation_write.go @@ -202,6 +202,14 @@ func navPatchWebProfile(doc bson.D, spec types.NavigationProfileSpec) bson.D { doc = navSetField(doc, "OfflineEntityConfigs", navOfflineConfigs(navGetArray(doc, "OfflineEntityConfigs"), spec.OfflineEntities)) } + + // ThrowPartialSyncError is declared by neither generated source, so it can + // only be written as a raw key — which is why nil means "the statement said + // nothing" and the stored value is left exactly as it is. A non-pointer + // would reset the flag on every rewrite that never mentions it. + if spec.ThrowSyncError != nil { + doc = navSetField(doc, "ThrowPartialSyncError", *spec.ThrowSyncError) + } return doc } diff --git a/mdl/backend/mpr/convert.go b/mdl/backend/mpr/convert.go index 7e69963b1f..77a661ea38 100644 --- a/mdl/backend/mpr/convert.go +++ b/mdl/backend/mpr/convert.go @@ -220,6 +220,7 @@ func convertNavProfile(in *mpr.NavigationProfile) *types.NavigationProfile { p.MenuItems[i] = convertNavMenuItem(mi) } } + p.ThrowPartialSyncError = in.ThrowPartialSyncError if in.OfflineEntities != nil { p.OfflineEntities = make([]*types.NavOfflineEntity, len(in.OfflineEntities)) for i, oe := range in.OfflineEntities { diff --git a/mdl/backend/mpr/convert_roundtrip_test.go b/mdl/backend/mpr/convert_roundtrip_test.go index 7c19838c81..0bb0432515 100644 --- a/mdl/backend/mpr/convert_roundtrip_test.go +++ b/mdl/backend/mpr/convert_roundtrip_test.go @@ -632,8 +632,8 @@ func TestFieldCountDrift(t *testing.T) { // CompatibilityMode to NavOfflineEntity left this test passing while the // new field was silently not carried, which is the exact drift the test // exists to catch. - assertFieldCount(t, "mpr.NavigationProfile", mpr.NavigationProfile{}, 9) - assertFieldCount(t, "types.NavigationProfile", types.NavigationProfile{}, 9) + assertFieldCount(t, "mpr.NavigationProfile", mpr.NavigationProfile{}, 10) + assertFieldCount(t, "types.NavigationProfile", types.NavigationProfile{}, 10) assertFieldCount(t, "mpr.NavOfflineEntity", mpr.NavOfflineEntity{}, 4) assertFieldCount(t, "types.NavOfflineEntity", types.NavOfflineEntity{}, 4) } diff --git a/mdl/executor/cmd_navigation.go b/mdl/executor/cmd_navigation.go index a53d52f9a6..2dc203b633 100644 --- a/mdl/executor/cmd_navigation.go +++ b/mdl/executor/cmd_navigation.go @@ -96,6 +96,7 @@ func execAlterNavigation(ctx *ExecContext, s *ast.AlterNavigationStmt) error { spec.MenuItems = append(spec.MenuItems, convertMenuItemDef(mi)) } + spec.ThrowSyncError = s.ThrowSyncError spec.HasSync = s.HasSyncBlock for _, se := range s.SyncEntries { spec.OfflineEntities = append(spec.OfflineEntities, types.NavOfflineEntitySpec{ @@ -350,6 +351,14 @@ func outputNavigationProfile(ctx *ExecContext, p *types.NavigationProfile) { fmt.Fprintln(ctx.Output, " )") } + // Only emitted when it differs from the platform default, so the clause + // appears exactly when it carries information. Describing every profile + // with `on sync error throw` would add a line to every navigation script + // that says what would happen anyway. + if !p.ThrowPartialSyncError { + fmt.Fprintln(ctx.Output, " on sync error continue") + } + // Offline entities. These are re-executable now, so they are emitted as a // SYNC block rather than as the commented-out approximation that made // describe -> exec lossy for every project using offline sync. diff --git a/mdl/grammar/MDLParser.g4 b/mdl/grammar/MDLParser.g4 index ec4265c58f..ac1d861121 100644 --- a/mdl/grammar/MDLParser.g4 +++ b/mdl/grammar/MDLParser.g4 @@ -331,6 +331,17 @@ navigationClause | NOT FOUND PAGE qualifiedName | MENU_KW LPAREN navMenuItemDef* RPAREN | SYNC LPAREN navSyncDef* RPAREN + // Studio Pro's "Throw error when server rejects objects during + // synchronization", stored as the profile-level ThrowPartialSyncError. + // + // Spelled with the phrase MDL already uses for failure handling — a + // microflow's ON ERROR CONTINUE / ON ERROR ROLLBACK — so it needs no new + // token and reads as something already learned. "Reject" is the platform's + // own word, but REJECT appears ~500 times across the examples and skills + // (approve/reject is one of the commonest things a workflow models), and + // claiming a heavily-used identifier as a keyword is not worth the closer + // paraphrase. + | ON SYNC ERROR (THROW | CONTINUE) ; // Offline synchronization, one statement per entity, mirroring the MENU block: diff --git a/mdl/types/navigation.go b/mdl/types/navigation.go index 55902136c9..eccc0c438e 100644 --- a/mdl/types/navigation.go +++ b/mdl/types/navigation.go @@ -33,6 +33,12 @@ type NavigationProfile struct { NotFoundPage string `json:"notFoundPage,omitempty"` MenuItems []*NavMenuItem `json:"menuItems,omitempty"` OfflineEntities []*NavOfflineEntity `json:"offlineEntities,omitempty"` + // ThrowPartialSyncError is stored on every WEB profile, online ones + // included, and is declared by NEITHER modelsdk/gen NOR + // generated/metamodel — measured on ako/TestApp, zero occurrences in each. + // It is therefore read and written as raw BSON rather than through the + // codec's typed accessors. + ThrowPartialSyncError bool `json:"throwPartialSyncError,omitempty"` } // NavHomePage holds a profile's default home page. @@ -201,6 +207,12 @@ type NavigationProfileSpec struct { // field alone is not enough. OfflineEntities []NavOfflineEntitySpec HasSync bool + // ThrowSyncError is Studio Pro's "Throw error when server rejects objects + // during synchronization". A POINTER, so nil means the statement said + // nothing and the stored value is left alone — the property is a bare bool + // with no unset value of its own, so a non-pointer would silently reset it + // on every rewrite. + ThrowSyncError *bool } // NavOfflineEntitySpec is one entity's offline sync rule, as MDL can express diff --git a/mdl/visitor/visitor_navigation.go b/mdl/visitor/visitor_navigation.go index 4e24c33ce0..13a83ad7d0 100644 --- a/mdl/visitor/visitor_navigation.go +++ b/mdl/visitor/visitor_navigation.go @@ -84,6 +84,11 @@ func (b *Builder) processNavigationClause(stmt *ast.AlterNavigationStmt, ctx *pa item := buildNavMenuItemDef(itemCtx) stmt.MenuItems = append(stmt.MenuItems, item) } + } else if ctx.ON() != nil && ctx.SYNC() != nil && ctx.ERROR() != nil { + // ON SYNC ERROR THROW|CONTINUE. Checked before the bare SYNC block + // because both alternatives carry a SYNC token. + throw := ctx.THROW() != nil + stmt.ThrowSyncError = &throw } else if ctx.SYNC() != nil { // SYNC (navSyncDef*) stmt.HasSyncBlock = true diff --git a/sdk/mpr/parser_misc.go b/sdk/mpr/parser_misc.go index 16f3e21a7c..690140df72 100644 --- a/sdk/mpr/parser_misc.go +++ b/sdk/mpr/parser_misc.go @@ -546,6 +546,13 @@ func parseNavigationProfile(raw map[string]any) *NavigationProfile { } } + // Studio Pro writes this on every web profile, online ones included, and + // neither gen nor generated/metamodel declares it — so it is read straight + // off the raw document. Defaulting to true matches every reference profile + // and Studio Pro's own checked-by-default box, so a document that somehow + // lacks the key is not silently flipped to "do not throw". + profile.ThrowPartialSyncError = extractBool(raw["ThrowPartialSyncError"], true) + // Offline entity configs (both web and native) for _, item := range extractBsonArray(raw["OfflineEntityConfigs"]) { if oeMap, ok := item.(map[string]any); ok { diff --git a/sdk/mpr/writer_navigation.go b/sdk/mpr/writer_navigation.go index 3efdd68fb3..317322d738 100644 --- a/sdk/mpr/writer_navigation.go +++ b/sdk/mpr/writer_navigation.go @@ -200,6 +200,13 @@ func patchWebProfile(doc bson.D, spec NavigationProfileSpec) bson.D { buildOfflineConfigsBson(getBsonArray(doc, "OfflineEntityConfigs"), spec.OfflineEntities)) } + // Kept identical to the modelsdk engine: nil leaves the stored flag alone, + // because neither generated source declares the property and a non-pointer + // would reset it on every rewrite that never mentions it. + if spec.ThrowSyncError != nil { + doc = setBsonField(doc, "ThrowPartialSyncError", *spec.ThrowSyncError) + } + return doc }