diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index cde50298e..a515e6135 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -28,10 +28,6 @@ type or subsystem. The rules, from `AGENTS.md`: - **If a change invalidates any part of a spec, update the spec in the same change** — never the other way around. Where `docs/ARCHITECTURE.md` (the cross-cutting map) and a spec disagree, the spec wins. -- **Planned work** lives in `docs/planned/`, one spec per feature, each with a - `Status: planned — not yet implemented` banner; `docs/todo.md` is the - prioritised index. When you implement one: build against the spec, then flip - its banner and rewrite it to present tense, and update `ARCHITECTURE.md`. - **Feature docs** (`docs/superpowers/`) are compressed reference docs — one file per feature, under 500 lines, present tense only, no changelogs or migration notes (git history covers that). diff --git a/cmake/morph_demote_interface_includes.cmake b/cmake/morph_demote_interface_includes.cmake index 2516a1190..3ff315cf2 100644 --- a/cmake/morph_demote_interface_includes.cmake +++ b/cmake/morph_demote_interface_includes.cmake @@ -10,7 +10,7 @@ # # ── Why this exists ────────────────────────────────────────────────────────── # -# The pinned Lightweight (bbb972a78e1962b968a2c6ad93f7dade736eaa01) resolves +# The pinned Lightweight (17c4e70d7fb4ead01376be0f46f201ad41c9a701) resolves # unixODBC with `pkg_check_modules(ODBC REQUIRED odbc)` and then propagates the # result as a PUBLIC *compile option*: # diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index f4993a590..751d7cf0b 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -761,3 +761,23 @@ documented behavior. | `required` derived from types + one opt-out list | The same declaration drives the schema, the client submit gate, and `validate()` — required-ness cannot drift between server and GUI. | | Combo-box options declared as an action reference (`Choice`) | Option lists are living data, so the single source is the action that serves them; the schema only carries the *reference*, and every renderer resolves it through the same dispatch seam as submits. | | Strict (non-clamping) datetime codec | `Rational` clamps hostile input because any int pair still denotes a value; a malformed timestamp denotes nothing — rejecting the read beats fabricating an epoch. | + +## Non-goals + +What morph deliberately does not build, and why. morph is a UI bridge with one server that owns neither the store nor the tooling; each row below follows from that. + +| Not building | Why | +|---|---| +| **Entity metamodel / naked objects** (Causeway) | morph's unit is the *action*, deliberately. Rows plus row-bound actions already are an object UI; what is missing is metadata on the row, not a second parallel metamodel. | +| **Server-authoritative per-instance action availability** ("see it, use it, do it") | A real gap — a client cannot ask "may I?" before invoking an action — but not worth the surface area it would add. | +| **Query invalidation / live lists** | Instance subscriptions already give shared instances a change channel, without a query-invalidation vocabulary. | +| **Paging / sorting contract on view schemas** | `views.md` already records "no server-side query language" as a non-goal; paging stays a field on the query action. | +| **Result-type presentation metadata** (money, enum labels, badge severity) | Every GUI controller hand-writes a `toMap()` projection. Genuine duplication, but it is a forms-layer concern, not a model-layer one. | +| **Field- and row-level permissions** (Jmix, Causeway SecMan) | They own the ORM and the whole app; morph owns a seam. Per-principal schema redaction would break the one-cached-schema-per-type design. The model is the right place, and `bank`'s `loadOwned` guard shows it works. | +| **BPM / BPMN engine** (Axelor, Jmix) | A process engine without a store is meaningless, and morph does not own the store. | +| **Grain call filters / interceptor pipeline** (Orleans) | morph already has validator + authorizer + `observe` + journal on every dispatch path. No observed friction. | +| **Clustering, silos, placement, grain versioning, stateless workers, distributed ACID transactions** (Orleans) | morph is a UI bridge with one server, not a distributed runtime. | +| **Managed streams with cursors and checkpoints** (Orleans) | Instance subscriptions are best-effort and unbuffered by design; durability here is a distributed-runtime concern. | +| **Reporting engine** (BIRT, JasperReports), **ORM + schema migration**, **IDE Studio** | Out of identity. morph owns neither the store nor the tooling. | +| **Runtime custom fields / dynamic attributes** (Axelor, Jmix) | Hard against compile-time reflection, and no observed need. | +| **REST / GraphQL facade, blob transfer, tabular export, multi-tenancy discriminator** | Plausible, no observed need. Revisit when one exists. | diff --git a/docs/LIGHTWEIGHT-CONSTRAINTS.md b/docs/LIGHTWEIGHT-CONSTRAINTS.md deleted file mode 100644 index cc2d65863..000000000 --- a/docs/LIGHTWEIGHT-CONSTRAINTS.md +++ /dev/null @@ -1,351 +0,0 @@ -# Lightweight constraints an example author hits - -The ladder's persistence layer is the [LASTRADA Lightweight](https://github.com/LASTRADA-Software/Lightweight) -ORM, pinned to one commit SHA by `examples/common/CMakeLists.txt` and, in -lockstep, `examples/bank/CMakeLists.txt`. Some of what that revision can and -cannot do is not obvious from its headers, and every rung that -rediscovered a limit wrote it down in **its own plan document** — so the next -rung rediscovered it again. This page is the single place those belong. - -**Scope.** This is not morph's design (that lives in `docs/spec/`) and not a -plan (those live in `docs/superpowers/`). It is a record of *someone else's* -library's behaviour, kept here because example authors are the people who trip -over it. - -**What every entry carries**, and what the plan documents did not: - -- the constraint, stated as something you can act on; -- **the pinned revision it was verified at**, and how it was verified; -- the file and line in Lightweight that decides it; -- the workaround actually used in this tree; -- **what would retire it** — the condition under which the entry should be - deleted. - -**An entry is only as good as its last verification.** When the pin moves, -re-run the probes; two of the three entries below were folklore that the pin -had already retired, and nobody noticed because nothing re-checked them. - -Pinned revision at the time of writing: -`bbb972a78e1962b968a2c6ad93f7dade736eaa01`. - ---- - -## 1. `Where()` cannot bind a binary value — **live** - -**Constraint.** The fluent query builder's literal `Where()` overload cannot -take a `SqlBinary` or `SqlDynamicBinary`. A comparison against a binary -column has to be expressed some other way. - -**Verified at** `bbb972a78e1962b968a2c6ad93f7dade736eaa01`, by compiling it -(`g++ 16.2.1`, `-std=c++23 -fsyntax-only`, non-reflection build): - -```cpp -Lightweight::SqlDynamicBinary<32> blob; -mapper.Query().Where(Lightweight::FieldNameOf<&Child::label>, "=", blob).All(); -``` - -``` -/usr/include/c++/16/bits/alloc_traits.h:716:28: error: no matching function for call to - 'construct_at(Lightweight::SqlVariant*&, const Lightweight::SqlDynamicBinary<32>&)' -``` - -The identical call with `std::string{"x"}` in place of `blob` compiles, so the -refusal is about the value type and not about the call shape. - -**Why.** `Where()`'s literal path stores the bound value in a -`std::vector` (`src/Lightweight/SqlQuery/Core.hpp:137`), and -`SqlVariant::InnerType` (`src/Lightweight/DataBinder/SqlVariant.hpp:49`) has -**zero** binary alternatives — the variant runs `SqlNullType`, `SqlGuid`, -`bool`, the integer and floating types, the string types, `SqlText`, `SqlDate`, -`SqlTime`, `SqlDateTime`, and stops. `SqlDataBinder>` exists -and works everywhere else; `Where()` simply does not go through it. - -**Workaround used in this tree.** Store the value in a text column as -lower-case hex and compare against the hex string. -`examples/bank`'s offline queue does exactly that for -`OfflineQueueRecord::idempotencyKeyHex`, because enqueue-time dedup needs a -`WHERE` on that column. Note what it does *not* do: the adjacent `payload` -column stays a real `SqlDynamicBinary`, because nothing ever puts it in a -`Where()`. The constraint is on the *comparison*, not on binary storage, so hex -only where a `WHERE` reaches. Hex is NUL-free by construction, so the text -column carries an arbitrary byte string without truncating it or colliding two -keys that share a NUL prefix, and the conformance round-trip -(`tests/offline_queue_conformance.hpp`'s `checkNulPayloadRoundTrip`) passes -unedited. - -**What retires this.** A Lightweight release that either adds a binary -alternative to `SqlVariant::InnerType` or routes `Where()`'s literal path -through `SqlDataBinder<>`. Tracked upstream as -[LASTRADA-Software/Lightweight#618](https://github.com/LASTRADA-Software/Lightweight/issues/618). -When it lands, the hex column can become a plain `SqlDynamicBinary` again. - ---- - -## 2. `Query()` has no `Update()` — **live, but not the constraint it was recorded as** - -**Constraint.** The record-typed fluent builder -(`mapper.Query().Where(...)`) offers `All()`, `First()`, `Count()`, -`Delete()` and friends, but **no `Update()`**. To update, use -`DataMapper::Update(record)` on a fetched record, or drop to the untyped -builder on the connection. - -**Verified at** the pin, by compiling it: - -```cpp -mapper.Query().Where(Lightweight::FieldNameOf<&Parent::id>, "=", 1).Update(); -``` - -``` -error: 'class Lightweight::SqlAllFieldsQueryBuilder' has no member named 'Update' -``` - -**This corrects what the plan archive says.** Three plan documents record this -as *"fluent `Query`/`Update` refuse `HasMany`-bearing records in the -non-reflection build"* (`docs/superpowers/plans/2026-08-16-kanban-backend.md`, -`docs/superpowers/specs/2026-08-16-kanban-rung4-design.md`, -`docs/superpowers/plans/2026-08-19-ledger-rung5.md`). At the pinned revision -that is **false in both halves**, and the control probes say so: - -| probe | at the pin | -| --- | --- | -| `DataMapper::Update(parent)` where `Parent` has a `HasMany` member | **compiles** | -| `mapper.Query().Where(...).All()` where `Parent` has a `HasMany` member | **compiles** | -| `mapper.Query().Where(...).Update()` (`Parent` has `HasMany`) | fails | -| `mapper.Query().Where(...).Update()` (`Child` has **no** `HasMany`) | **fails identically** | - -The last row is the one that matters: the refusal has nothing to do with -`HasMany`. `DataMapper::Update` skips relations explicitly — *"Relations -(HasMany, HasManyThrough, HasOneThrough, ...) have no column of their own"*, -`src/Lightweight/DataMapper/DataMapper.hpp:2047` and three sibling sites — so a -`HasMany` member costs an update nothing. - -**Workaround.** `DataMapper::Update(record)`, which is what every ladder rung -already does. - -**What retires this.** A Lightweight release that adds `Update()` to -`SqlAllFieldsQueryBuilder`. Re-run the probe when the pin moves. - ---- - -## 3. `HasMany` resolves its foreign key by ordinal member index — **retired; do not believe this** - -**This constraint is no longer true**, and it is recorded here only because -three plan documents still assert it and a fourth rung would otherwise design -around a limit that no longer exists. - -**What the plans say.** That `HasMany` locates the child's foreign key by -matching **member position**, so reordering a record's members silently -repoints the relation. - -**What the pinned revision does.** Matches by relationship **type**. -`src/Lightweight/DataMapper/Record.hpp:242-255` says so in its own words — -*"This is how `HasMany`, `HasManyThrough` and `HasOneThrough` locate their -foreign key column: by matching the relationship type, never by member -position"* — and `detail::InverseBelongsToResolver` (`Record.hpp:213-238`) -carries three `static_assert`s that turn every failure mode into a named -compile error: no `BelongsTo` at all, a `BelongsTo` whose column name does not -match, and an ambiguous pair of them. - -**Verified at** the pin, with the child's `BelongsTo` declared **last**, which -positional resolution would get wrong: - -```cpp -static_assert(Lightweight::InverseBelongsToIndexOf == 2); -static_assert(Lightweight::InverseBelongsToFieldNameOf == std::string_view{"parent"}); -``` - -Both hold. - -**What would re-open this.** A `HasMany` relation resolving to the wrong column -at a pin where the `static_assert`s above are absent. If that happens, this -entry becomes live again and the pin's `Record.hpp` is the first thing to read. - ---- - -## 4. The migration DSL cannot express a partial index — **live** - -**Constraint.** `SqlMigrationQueryBuilder`'s index API takes a name, a table -and a column list, and nothing else. There is no predicate, so -`CREATE UNIQUE INDEX … WHERE ` — a partial index — has no spelling in the -DSL. - -**Verified at** the pin, by reading the API surface. Every overload, in -`src/Lightweight/SqlQuery/Migrate.hpp:553-582`: - -```cpp -CreateIndex(std::string indexName, std::string tableName, std::vector columns, bool unique = false); -CreateUniqueIndex(std::string indexName, std::string tableName, std::vector columns); -CreateIndex(std::string tableName, std::vector columns, IndexType type = IndexType::NonUnique); -``` - -Also `SqlCreateTableQueryBuilder::Unique()` / `::UniqueIndex()` -(`Migrate.hpp:74`, `:80`) and `SqlAlterTableQueryBuilder::AddUniqueIndex(column)` -(`:207`) — none takes a predicate. `MigrationPlan.cpp` renders a trailing -`WHERE` only for UPDATE and DELETE plans (`:56-71`), never for an index. - -**Why it bites.** Dedup on an optional key wants exactly a partial index: -`UNIQUE(key) WHERE key <> ''`. An *unconditional* unique index is not a -substitute — it makes the second empty-key row fail, which -`IOfflineQueue::enqueue` forbids. - -**Workaround used in this tree.** `examples/bank`'s offline queue does the -check as a `SELECT` under its own mutex instead, and says so in -`LightweightOfflineQueue`'s class comment: as strong as `FileOfflineQueue`'s -linear scan (single process), deliberately weaker than -`SqliteOfflineQueue`'s real index (any writer). The other option, not taken -there, is `SqlMigrationQueryBuilder::Native(callback)` (`Migrate.hpp:594`), -which hands the plan a raw SQL string — an escape hatch that gives up the DSL's -cross-dialect rendering for that one statement. - -**What retires this.** A `CreateIndex` overload taking a predicate expression, -or an equivalent in the plan renderer. - ---- - -## 5. SQLite connections are rollback-journal, with a 60 s busy timeout — **live** - -**Constraint.** Lightweight sets **one** SQLite pragma of its own, and it is -not the one people assume. Every connection it opens to a SQLite data source -gets `busy_timeout = 60000`; `journal_mode` it deliberately leaves alone, so -the database stays in SQLite's default **rollback-journal** mode unless -something in this tree sets it. - -Two consequences an example author gets wrong: - -- **`BEGIN DEFERRED` is not a snapshot here.** Under `journal_mode=WAL` a - deferred read transaction lets writers continue; under a rollback journal it - escalates to a `SHARED` lock on first read and blocks every writer on the - file until it is released. A long read transaction is therefore a *write - barrier*, not a free consistent view. -- **The busy timeout is 60 s, not whatever the connection string says.** - `Timeout=5000` in an ODBC connection string is the ODBC login/query timeout; - it does not reach SQLite's `busy_timeout`, which `PostConnect()` has already - set to 60000 ms. A contended writer that is going to fail stalls for a - minute first. - -**Verified at** `bbb972a78e1962b968a2c6ad93f7dade736eaa01`, by reading -`src/Lightweight/SqlConnection.cpp:388-398` — the whole of what `PostConnect()` -does for SQLite: - -```cpp -if (m_serverType == SqlServerType::SQLITE) -{ - // Set a busy timeout to prevent "database is locked" errors during concurrent access. - // 60 seconds should be sufficient for most operations. - SqlStatement stmt(*this); - [[maybe_unused]] auto cursor = stmt.ExecuteDirect("PRAGMA busy_timeout = 60000"); - - // We could also enable WAL mode here, but that changes the database file structure. - // However, for high-concurrency restoration, it is highly recommended. - // Let's stick to busy_timeout for now as it's purely a runtime behavior change. -} -``` - -and by grepping this tree for the pragma nobody issues: - -``` -$ git grep -n "journal_mode" d03c66f3 -- examples/ledger examples/crm examples/bookmarks examples/polls scripts/scenario -$ echo $? -1 -``` - -(pinned to `d03c66f3`, the revision before morph#739's own comments added -the word to `ledger_model.cpp`; exit 1 is "no match"). - -The only `journal_mode` under `examples/` is in kanban's *tests* -(`examples/kanban/tests/test_kanban_offline.cpp:629`), which issue it -optionally, for one fixture, and whose own comments record that it is -per-database-file rather than per-connection. `morph::offline::SqliteOfflineQueue` -does set WAL, but on its **own** sqlite3 handle and its own queue file — not on -the ODBC database the rung models use. - -**Workaround used in this tree.** Name the thing after what it does in the mode -that is actually in force: `examples/ledger/src/models/ledger_model.cpp`'s -`DeferredReadTransactionGuard` was called `WalSnapshotGuard` until morph#739, -and the old name described the opposite of its real contention behaviour. Keep -raw read transactions as narrow as the aggregation that needs them. - -**What retires this.** A rung that sets `journal_mode=WAL` on the ODBC database -deliberately (a decision with its own consequences — WAL lives in the database -file header, so it reaches every other opener and the scenario runner's -fresh-database-per-run assumption), or a Lightweight revision whose -`PostConnect()` sets it. Re-read `PostConnect()` when the pin moves. - ---- - -## 6. `DataMapperPool::Return` performs no transaction cleanup — **live** - -**Constraint.** Returning a pooled `DataMapper` to -`Lightweight::GlobalDataMapperPool()` does **not** end a transaction open on -its connection. All three growth-strategy overloads of `Pool::Return` -do the same three things — drop the async backend, announce the hand-off to -the `SqlLogger`, park the mapper — and none of them issues `SQLEndTran` or -restores `SQL_ATTR_AUTOCOMMIT`: - -```cpp -void Return(std::unique_ptr dm) noexcept -{ - DropAsyncBackend(*dm); // = dm.Connection().DisableAsync(); - SqlLogger::GetLogger().OnConnectionIdle(dm->Connection()); - std::scoped_lock lock(_mutex); - _idleDataMappers.push_back(std::move(dm)); -} -``` - -A mapper parked with autocommit still `OFF` therefore carries an open -transaction into the pool, and the next — unrelated — borrower inherits it. -Combined with entry 5, the cost of that lands sixty seconds later, on a caller -that opened no transaction, as `database is locked`. - -What keeps this tree safe is **declaration order, not the library**: -`SqlTransaction::Commit()` and `::Rollback()` restore autocommit themselves and -immediately, and a handler that relies on the destructor instead is safe only -because its `SqlTransaction` local is declared *after* the pooled mapper and so -destructs *before* it. Hoisting the transaction into a member, a -`std::optional`, or any longer-lived scope silently undoes that. - -**Verified at** `bbb972a78e1962b968a2c6ad93f7dade736eaa01`, by reading -`src/Lightweight/DataMapper/Pool.hpp:139-146` (the `UnboundedGrow` overload -quoted above), `:150-162` (`BoundedWait`) and `:198-209` (`BoundedOverflow`, -which is the one actually in force — `DefaultPoolConfig` at `:173-177` selects -it), `:133-136` (`DropAsyncBackend`, the whole of what a returned connection is -cleaned with), and `src/Lightweight/SqlTransaction.cpp:52-93` -(`TryRollback()`/`TryCommit()` restoring autocommit there and then). - -**Workaround used in this tree.** morph#740's `PoolTransactionAudit` -(`examples/common/db/pool_transaction_audit.hpp`) installs itself as the -process-wide `SqlLogger` and reads `SQL_ATTR_AUTOCOMMIT` at every hand-off, so -a leak aborts at the leak instead of stalling an innocent caller a minute -later. All seven ladder rungs' `db::setup()`/`db::configure()` and -`testkit/DbFixture` install it. - -**`examples/bank` deliberately does not, and that is not a coverage gap** -(morph#752). Bank does not use the pool at all: `bank::db::WithMapper::mapper()` -(`examples/bank/include/bank/db/db_model.hpp`) constructs a `DataMapper` -directly, one per model, and `LightweightOfflineQueue` owns another. -`OnConnectionIdle` and `OnConnectionReuse` — the only two hooks the audit reads -— are emitted from `Pool.hpp` and from nowhere else at this pin, so an audit -installed in bank would inspect zero hand-offs and could never fail. Measured -rather than argued, by `examples/bank/tests/test_pool_scope.cpp`: the same -counter records **2** hand-offs for one deliberate `Acquire()`/return and **0** -across a bank `OpenAccount` + `Deposit` + `Transfer`, the two latter being the -paths that open a `SqlTransaction`. Mutating `TransactionModel::execute(Deposit)` -to acquire from the pool turns that 0 into a 2 and fails the case, so the zero -is a measurement and not an unwired counter. - -**What retires this.** A Lightweight revision whose `Pool::Return` ends the -transaction (or asserts that none is open), at which point the audit and this -entry both go. Re-read all three `Return` overloads when the pin moves. Bank's -half of the entry retires the moment any bank code names -`GlobalDataMapperPool()` — `test_pool_scope.cpp` fails when it does. - ---- - -## Adding an entry - -File the upstream issue first, so the entry has a retirement condition somebody -is tracking, then add a section here with the five bullets from the top of this -page. An entry with no verification revision is worth less than no entry: it -cannot be re-checked, and it will outlive the constraint it describes — which -is exactly how entries 2 and 3 above became folklore. diff --git a/docs/analysis/qml-bridge-boilerplate.md b/docs/analysis/qml-bridge-boilerplate.md index 41ecc2e96..ae572b6c5 100644 --- a/docs/analysis/qml-bridge-boilerplate.md +++ b/docs/analysis/qml-bridge-boilerplate.md @@ -35,7 +35,7 @@ Instead do the small, boring thing the measurements actually point at: the drift guard, and it is worth having *whether or not* anything is ever generated — it is what caught the `Q_MOC_RUN` hazard in PR #155. 3. **Close #86** with the measurements below, and record in - `docs/todo.md`'s "Considered and refused" table that the projection half + the non-goals table in `docs/ARCHITECTURE.md` that the projection half is per-model by construction. The reasoning is that the premise the issue rests on does not survive @@ -392,7 +392,7 @@ keys the UI needs, would silently unmask a PII field, and pushes formatting into a layer the project's own rules forbid it in.** The cost side is overstated in the issue and the benefit side is smaller still. -Note also that `docs/todo.md`'s "Considered and refused" table already +Note also that the non-goals table in `docs/ARCHITECTURE.md` already carries this judgment: *"Result-type presentation metadata (money, enum labels, badge severity) — every GUI controller hand-writes a `toMap()` projection. Genuine duplication, but it is a forms-layer concern."* diff --git a/docs/spec/VERSIONING.md b/docs/spec/VERSIONING.md index f11837ec4..da22f85de 100644 --- a/docs/spec/VERSIONING.md +++ b/docs/spec/VERSIONING.md @@ -85,9 +85,9 @@ same: so a behavior change that contradicts a spec is a major even when the C++ signature is unchanged. - **Minor (`x`.Y.0):** additive and source-compatible — a new public symbol, a - new optional parameter with a default, new opt-in behavior. Every item in - [todo.md](../todo.md)'s roadmap is deliberately "opt-in or backward - compatible by default," so the entire planned roadmap fits inside 1.x. + new optional parameter with a default, new opt-in behavior. New features are + required to be opt-in or backward compatible by default, so they fit inside + 1.x. - **Patch (`x`.`y`.Z):** bug fixes and doc corrections that touch neither the stable surface nor its documented behavior. @@ -190,8 +190,8 @@ piece of the policy is enforced by review discipline alone. - **Not a promise about `detail`.** See "The stable surface" above. - **Not a feature freeze.** The policy governs *how* the surface evolves (additive in minors, breaking only in majors with a deprecation window), not - *whether* it evolves — the entire [todo.md](../todo.md) roadmap is - 1.x-compatible by its own "opt-in or backward compatible by default" rule. + *whether* it evolves — new features stay 1.x-compatible by the "opt-in or + backward compatible by default" rule. - **No existing behavior changes.** This policy is governance plus tooling — a version header, a policy document, `[[deprecated]]` discipline, and CI enforcement — not a change to any existing public symbol's behavior. diff --git a/docs/todo.md b/docs/todo.md deleted file mode 100644 index dbab4e004..000000000 --- a/docs/todo.md +++ /dev/null @@ -1,365 +0,0 @@ -# Program tracker - -Two programs live here: - -- **[§F — Stateful models](#f-stateful-models-shipped)** came out of - [issue #18](https://github.com/LASTRADA-Software/morph/issues/18) ("compare - against Axelor / Jmix / Causeway / Orleans and find out what we are missing"). - All three items have shipped. -- **§A–§E — Production hardening & GUI generation** is **shipped**, kept for - the rationale (priority, dependency order) that motivated the work. - Present-tense designs for those are in [`docs/spec/`](spec). - ---- - -# §F. Stateful models (shipped) - -## What the issue #18 survey actually found - -The four projects named in the issue are not comparable to morph as a set: -Axelor, Jmix and Causeway are full-stack Java business-application platforms; -Orleans is a .NET distributed actor runtime. Reading their feature lists side by -side produces a backlog of ~25 items, most of which morph should never build — -it does not own a database, a process engine, or an IDE. - -So the survey was done the other way round: against **use cases in -`examples/bank`**, morph's largest worked example, asking where a developer hits -a wall rather than which boxes are unticked. That produced one finding that -subsumes most of the others. - -**morph's models are anonymous and stateless, so the model layer is doing -nothing.** Every bank model's only member is the `std::optional` it -inherits from `WithMapper` — a connection, not domain state. The per-instance -strand that morph advertises as its core service therefore protects nothing; -every action is a full round trip to SQLite; and because a `BridgeHandler` -registers one instance, the desktop GUI holds five `AccountModel` instances (and -five SQLite connections) for what is logically one thing. - -Orleans is the comparator that names this directly — a grain is *identity + -behaviour + state*, and morph has only behaviour. But the fix is not to become -an actor runtime. It is to make morph's existing model layer do the job it -already claims: hold state, be identified, be reachable. - -## Accepted items - -### F1 — Reshape `examples/bank` onto stateful models · P0 · shipped - -Split the per-domain models into models keyed by the entity they are named -after, holding that entity in memory, hydrated on activation and written -through on mutation. This is first deliberately: it is what introduces the main -idea of the library, and F2/F3 are unverifiable without it — a primary key -identifies nothing when instances carry nothing. - -`AccountModel` holds one account in memory, keyed by account id; `CustomerModel` -took the per-owner half (`ListAccounts`/`OpenAccount`), which was never -account-scoped. Cross-model ledger writes settle on another model's connection, -so `bank/db/row_versions.hpp` bumps a per-row counter and cached readers -re-hydrate on a stale version — the example's sharpest edge, shown rather than -arranged away. WASM shadow models and the five GUI controllers moved with it. -See [`examples/bank/README.md`](../examples/bank/README.md), "Stateful, keyed -models". - -### F2 — Keyed, shareable model instances · P0 · shipped - -One `BRIDGE_MODEL_KEY(Model, Action, &Action::field)` line beside the existing -registrations declares a model's key — the type is deduced from the field, so -the model class itself is untouched; `BridgeHandler` opts a handler -into a **server-side** instance directory, so instances are reusable across -clients. `instances()` enumerates the live keys. A keyed action re-points a -handler rather than re-keying an instance, so key collisions do not arise. - -Carried a change to shipped behaviour: A7's `closeConnection` now decrements a -reference count rather than erasing, since otherwise one client's disconnect -destroys an instance another client is using. - -See [`spec/core/shared_instances.md`](spec/core/shared_instances.md). - -### F3 — Instance subscriptions · P1 · shipped - -`subscribe(cb)` keyed on the **result/state** type, firing whenever an `R` is -produced on the instance the handler is attached to — by this handler, by -another handler sharing it, or by another screen entirely. - -**Removed** the reactive-draft mechanism (`set<&A::field>`, `reset`, the old -action-keyed `subscribe`, in-flight coalescing), whose job stateful models do -better by holding the draft themselves. `morph::flows::FlowSession` already -owned its own draft tuple and merely mirrored into the handler's, so it now -gates on `ActionValidator` and dispatches directly; its public API, the -`w-*`/`app-*` schema, and `WizardView.qml` are unchanged. - -`ActionValidator` survives with its A1 server-side role, losing only its -draft-readiness one. See -[`spec/core/bridge.md`](spec/core/bridge.md#subscription-semantics) and -[`ARCHITECTURE.md`](ARCHITECTURE.md#instance-subscriptions). - -**Not included:** cross-*client* push. Fan-out is per `Bridge`, so two handlers -in one process — the case the bank GUI actually has — see each other's work, -while two separate clients do not. A server-initiated `notify` frame would need -both transports to grow an unsolicited-message path; that is its own item. - -## Considered and refused - -Recorded so the survey does not get re-run and so the boundary is explicit. - -| Not building | Why | -|---|---| -| **Entity metamodel / naked objects** (Causeway) | morph's unit is the *action*, deliberately. Rows plus row-bound actions already are an object UI; what is missing is metadata on the row, not a second parallel metamodel. | -| **Server-authoritative per-instance action availability** ("see it, use it, do it") | Real gap — `CloseAccount` is implemented, tested and exposed in no GUI because the client cannot ask "may I?". Judged not worth the surface area now. | -| **Query invalidation / live lists** | Superseded in part by F3, which gives shared instances a change channel without a query-invalidation vocabulary. | -| **Paging / sorting contract on view schemas** | `views.md` already records "no server-side query language" as a non-goal; paging stays a field on the query action. | -| **Result-type presentation metadata** (money, enum labels, badge severity) | Every GUI controller hand-writes a `toMap()` projection. Genuine duplication, but it is a forms-layer concern, not a model-layer one. | -| **Field- and row-level permissions** (Jmix, Causeway SecMan) | They own the ORM and the whole app; morph owns a seam. Per-principal schema redaction would break the one-cached-schema-per-type design. The model is the right place, and `bank`'s `loadOwned` guard shows it works. | -| **BPM / BPMN engine** (Axelor, Jmix) | A process engine without a store is meaningless, and morph does not own the store. | -| **Grain call filters / interceptor pipeline** (Orleans) | morph already has validator + authorizer + `observe` + journal on every dispatch path. No observed friction. | -| **Clustering, silos, placement, grain versioning, stateless workers, distributed ACID transactions** (Orleans) | morph is a UI bridge with one server, not a distributed runtime. | -| **Managed streams with cursors and checkpoints** (Orleans) | F3 is best-effort and unbuffered by design; durability here is a distributed-runtime concern. | -| **Reporting engine** (BIRT, JasperReports), **ORM + schema migration**, **IDE Studio** | Out of identity. morph owns neither the store nor the tooling. | -| **Runtime custom fields / dynamic attributes** (Axelor, Jmix) | Hard against compile-time reflection, and no observed need. | -| **REST / GraphQL facade, blob transfer, tabular export, multi-tenancy discriminator** | Plausible, no observed need. Revisit when one exists. | - -## Also surfaced - -- **`README.md`'s "Status & limitations" is stale.** It still claims the wire - protocol has no version negotiation, that `RemoteServer` model ids are - guessable sequential integers, and that only an in-memory offline queue - ships — all fixed by §A/§B below. Corrected as part of this program. - ---- - -# Production hardening & GUI generation (shipped) - -This tracked a design-approved, prioritized checklist of production-hardening -and GUI-generation work. **Every item in §A–§E has shipped.** The authoritative, -present-tense design for each is in `docs/spec/`; this section is a record of -what landed and why, kept for the rationale (priority, dependency order) that -motivated the work. - -Readiness depended on deployment mode: - -- **Local mode** (models in-process, no network): was already near - production-ready; only the operational items (§C) and a couple of §B - robustness items applied. -- **Remote mode** (`RemoteServer` over a network): needed the §A hardening - milestone before public/multi-tenant exposure. All of §A is now shipped. - -Priority key (as originally assigned): **P0** must-have before that mode -ships, **P1** strongly recommended, **P2** nice-to-have. - ---- - -## A. Remote-mode hardening (do before networked/public/multi-tenant use) - -### A1 — Server-side action validation · P0 · shipped -`ActionValidator::ready` (+ declared-precision reconciliation) now runs -inside the dispatcher runner and `Bridge::executeVia`'s `localOp`, before -`Model::execute`. A `false` result rejects the call as `morph::model::ValidationError` -(`err`/`onError`). See `spec/core/registry.md` and `spec/core/bridge.md`. - -### A2 — Register authorization & opaque model ids · P0 · shipped -`IAuthorizer::authorizeRegister` (default allow-all) is enforced in -`RemoteServer`; model ids are opaque (non-sequential) rather than a -guessable counter. See `spec/session/session.md` ("The register-authorization -hook", "Opaque model ids") and `spec/core/backend.md`. - -### A3 — Transport-level resource limits · P0 · shipped -`RemoteServer::LimitPolicy` (execute timeout, max live models, max in-flight) -and `QtWebSocketServerConfig` (max connections, per-frame size, per-connection -rate, idle/handshake timeouts) are implemented, all opt-in/default-off. See -`spec/core/backend.md#limitpolicy--opt-in-resource-limits` and -`spec/security.md`'s hardening checklist. - -### A4 — TLS + peer verification as the enforced default · P0 · shipped -`qt_tls.hpp` ships `tlsVerifyingConfig()`/`tlsPinnedConfig()`/`tlsInsecureNoVerify()` -as the documented client-side path, `examples/qt_tls_client/` demonstrates -pinned-cert acceptance and MITM rejection end to end, and -`QtWebSocketServerConfig::bindAddress`/`allowPlaintextExposure` make -`QtWebSocketServer::listen()` refuse a silent non-loopback plaintext bind -unless explicitly overridden. See `spec/security.md#the-qt-websocket-transport-morphqt` -and `spec/core/backend.md#qtwebsocketserver--server-side-websocket-transport`. - -### A5 — Inject a vetted HMAC for production · P1 · shipped -`examples/vetted_hmac/` ships libsodium and OpenSSL `MacFunction` adapters -(each with a known-answer + interop test) and the opt-in -`MORPH_REQUIRE_VETTED_HMAC` build option, which drops the `mac = hmacSha256` -default argument on `TokenIssuer`/`TokenVerifier`/`SigningAuthorizer` so a -build relying on it fails to compile. See `spec/security.md`, "MAC-primitive -recommended wiring". - -### A6 — Protocol / action-schema versioning · P1 · shipped -A `"hello"` handshake negotiates a protocol version range between client and -server (opt-in, explicit — not automatic-on-connect); `BRIDGE_REGISTER_ACTION`'s -generated `fromJson`/`resultFromJson` now decode leniently, making the -additive-only action-evolution policy actually hold. See `spec/core/wire.md`'s -"Protocol version negotiation" and "Action-evolution policy" sections. - -### A7 — Connection-scoped model cleanup · P0 · shipped -`RemoteServer` has an opt-in `ConnectionId` scope (`openConnection`/ -`closeConnection` + a scoped `handle(msg, reply, cid)` overload), and -`QtWebSocketServer` opts every client into it end to end, so a client crash or -dropped socket now reclaims its models instead of stranding them until process -exit. `closeConnection` is server housekeeping — it bypasses `IAuthorizer` by -design, not a synthesized wire `deregister`. See `spec/core/backend.md#connection-scopes`. - -> **F2 changes this.** Cross-client instance sharing requires `closeConnection` -> to decrement a reference count rather than erase. See -> [`spec/core/shared_instances.md`](spec/core/shared_instances.md). - ---- - -## B. Durability & data-integrity (both modes, if you persist) - -### B1 — Durable offline queue & cross-restart dead-lettering · P1 · shipped -`QueueItem::attempts` (durable) + `IOfflineQueue::setAttempts` write-back hook -and an optional `SyncWorker::DeadLetterSink` are implemented — `InMemoryOfflineQueue` -overrides `setAttempts` in-memory (unchanged behavior); a durable queue (see B3) -persists the count across restarts. See `spec/offline/offline.md`. - -### B2 — Transactional outbox (journal + store atomicity) · P1 · shipped -`LogEntry::idempotencyKey` + dedup in `InMemoryActionLog`/`FileActionLog::append()`, -`IModelHolder::setOutboxManaged`/`isOutboxManaged` (suppresses the automatic -journal append), and `journal::OutboxRelay` (drains an outbox table into a -durable `IActionLog` in the model's own transaction) are implemented. See -`spec/journal/journal.md` and `spec/core/registry.md`. - -### B3 — A shipped durable `IOfflineQueue` implementation · P2 · shipped -`FileOfflineQueue` (zero-dependency NDJSON, ships in the default `morph` -target) and `SqliteOfflineQueue` (opt-in via `MORPH_BUILD_OFFLINE_SQLITE`) -both persist `payload` + `idempotencyKey` + `attempts` across restarts. See -`spec/offline/offline.md`. - -### B4 — Journal format versioning & retention · P2 · shipped -`journal::fromJson` decodes leniently (matching the wire's forward-compatible -stance); every `LogEntry` carries a `v` line-format version, rejecting a -future format a reader doesn't understand instead of silently misreading it; -`FileActionLog::rotate(sealedPath)` gives hosts a retention seam. See -`spec/journal/journal.md`. - ---- - -## C. Operational readiness (needed for any production, esp. remote) - -### C1 — Observability: metrics, tracing, health · P1 · shipped -A lightweight, injectable `morph::observe` seam (metrics + trace spans, -mirroring the logger's replaceable-sink pattern) is wired into `RemoteServer`, -`LocalBackend`, `SyncWorker`, and `ReconnectCoordinator`; `RemoteServer::health()`/ -`ServerConfig::healthHandler` expose a readiness snapshot, flipped by `beginShutdown()` -(see C5). See `spec/core/observability.md`. - -### C2 — Non-Qt transport option · P2 · shipped -`morph::net` (`SocketBackend`/`SocketServer`, opt-in via `MORPH_BUILD_NET`) is -a raw-socket RFC 6455 WebSocket transport with no Qt dependency, wire-interoperable -with `QtWebSocketBackend`/`QtWebSocketServer` (verified both directions). No -TLS in this reference transport — pairs with a TLS-terminating proxy, or use -the Qt transport's TLS support (A4) directly. See `spec/core/backend.md`'s -`SocketBackend`/`SocketServer` section. - -### C3 — Load / soak / fuzz testing · P1 · shipped -`fuzz_wire_decode`/`fuzz_dispatch_execute` (`MORPH_BUILD_FUZZERS=ON`), the -switchBackend/reconnect soak tests and the throughput/latency benchmark -(`MORPH_BUILD_LOAD_TESTS=ON`), and the adversarial cross-socket run -(`MORPH_BUILD_QT=ON`) are implemented, all opt-in/default-off. The fuzz -harness surfaced two real `morph::wire` bugs on first run (a `skip_ws` -heap-buffer-overflow, and unescaped control bytes breaking the `err`-reply -round-trip) — both are now fixed; see `spec/testing_strategy.md`'s "Known -findings" for details and the permanent regression cases under -`tests/fuzz/findings/`. - -### C4 — Compile-time `onBackendChanged` dispatch · P2 · shipped -`LocalBackend::notifyBackendChanged` no longer `dynamic_cast`s every live -model; backend-change-awareness is captured at registration time -(`IModelHolder::isBackendChangeAware()`) and driven from a maintained set — -a pure, behavior-preserving refactor (parity-tested). See -`spec/core/backend.md` and `spec/core/registry.md`. - -### C5 — Graceful shutdown & drain · P1 · shipped -`RemoteServer::beginShutdown()` + `drainedWithin(deadline)` (reject new -`register`/`execute` with `err "server shutting down"`, drain the shared -in-flight counter, flip `health().ready` to `false`) and -`QtWebSocketServer::closeGracefully(deadline)` (close frames, then hard stop) -are implemented, opt-in/default-off — a server that never calls either -behaves byte-for-byte as before. See `spec/core/backend.md#graceful-shutdown-beginshutdown--drainedwithin`. - ---- - -## D. Process / project - -### D1 — Spec ↔ code drift guard (CI) · P1 · shipped -A CI check pins the mechanical facts that had drifted before (enum -cardinalities, key constants, canonical error-message strings, glaze -`error_on_unknown_keys` behavior) via `docs/spec/pinned_facts.toml`, -`tests/test_pinned_facts.cpp`, and a gate removed on 2026-09-23 — future -drift now fails the build. See `CONTRIBUTING.md`, "Quality gates". - -### D2 — API stability / 1.0 commitment · P2 · shipped -A concrete versioning/deprecation/compat policy is published, plus -`morph::version` constants cross-checked against `CMakeLists.txt` and a CI -lint on `[[deprecated]]` marker format. See `docs/spec/VERSIONING.md`. - ---- - -## E. GUI enhancement program (rapid + flexible GUI development) — shipped - -A layered program that generates GUIs from the user's model + action types -with minimal declaration, while keeping the result flexible. The guiding -principle (*infer by default, declare to override*; all new `x-*` keys are -additive/unversioned; Qt/QML is the reference renderer, the schema contract -stays renderer-agnostic) is now documented in -[forms.md](spec/forms/forms.md#design-principle-infer-by-default-declare-to-override). - -### Tier 1 — richer forms (additive metadata/logic on the single-action form) - -- **E-G1 — Field metadata** · P1 · shipped — `spec/forms/forms.md` - ("Field metadata — `FieldMeta`"). -- **E-G2 — Layout & grouping** · P1 · shipped — `spec/forms/forms.md` - ("Layout & grouping — sections, tabs, spans"). -- **E-G3 — Widget hints** · P1 · shipped — `spec/forms/widget_hints.md` - (`Multiline`/`Ranged`, `x-widget`/`x-min`/`x-max`/`x-step`). -- **E-G4 — Cross-field rules** · P1 · shipped — `spec/forms/forms.md` - ("Cross-field rules — the `x-rules` vocabulary"), shared with the - server-side validator (`spec/core/registry.md`). -- **E-G5 — Computed fields** · P2 · shipped — `spec/forms/forms.md` - ("Computed fields"). -- **E-G6 — Dependent choices** · P2 · shipped — `spec/forms/choice.md` - ("Dependent (cascading) options"). -- **E-G10 — Localisation (i18n)** · P1 · shipped — `spec/forms/forms.md` - ("Localisation — message keys and the catalog seam"), `FieldMeta::i18nKey`. - -### Tier 2 — app generation (a view/app schema layer above the action schema) - -- **E-G7 — Collections & views** · P2 · shipped — `spec/forms/views.md` - (`morph::views::viewSchemaJson`, `BRIDGE_REGISTER_VIEW`, `ViewRegistry`) - and the `src/qt/forms` `CollectionView.qml` reference renderer. -- **E-G8 — Workflows & navigation** · P2 · shipped — `spec/forms/workflows_navigation.md` - (`morph::flows::Wizard`/`FlowSession`, `morph::app::App`, - `BRIDGE_REGISTER_WIZARD`/`BRIDGE_REGISTER_APP`) and the `src/qt/forms` - `WizardView.qml` reference renderer plus the demo's `AppShell.qml`. - - > **F3 reworks this.** `FlowSession` is built on the reactive-draft mechanism - > F3 removes; it must be re-expressed as a stateful, keyed flow model first. - -### Ecosystem - -- **E-G9 — Renderer toolkit** · P1 · shipped — `spec/forms/forms.md` - ("Shipped Qt/QML reference renderer", "Renderer conformance kit", - "Theming / component-override registry"). - -## Fast reference — minimum bars (all now met) - -- **Local, trusted, in-process:** ship + C1 (observability) + C3 (soak) for - confidence. B1/B2 if you persist. All shipped. -- **Remote, internal/trusted network:** A1 + A2 + A3 + A4 + A7 at minimum; - B1/B2 if you persist; C1 + C3. All shipped. -- **Remote, public / multi-tenant:** all of §A, all of §B if persisting, all - of §C, D1. All shipped. - -## Notes - -- Every §A–§E item landed opt-in or backward compatible by default — none change - existing behavior unless enabled. **§F breaks this pattern deliberately:** F2 - changes A7's cleanup semantics and F3 removes a public API. -- Two items surfaced *by* the §A–§E program, not originally on it, and fixed - before it closed out: C3's fuzz harness found two real bugs in `morph::wire`'s - glaze-based parsing (a heap-buffer-overflow reachable by a 5-byte input, and - a case where `RemoteServer`'s own error reply didn't round-trip through - `encode`/`decode`). See `docs/spec/testing_strategy.md`'s "Known findings" - section. diff --git a/examples/IMPLEMENTATION.md b/examples/IMPLEMENTATION.md index cd7b13629..0798cf5db 100644 --- a/examples/IMPLEMENTATION.md +++ b/examples/IMPLEMENTATION.md @@ -10,13 +10,11 @@ could have provided is a defect in the stress test.** If the framework can't provide it, that inability is a *finding* — record it per [`AGENTS.md`](../AGENTS.md)'s filing bar, don't quietly code around it. -**Before you design around a Lightweight limitation, read -[`docs/LIGHTWEIGHT-CONSTRAINTS.md`](../docs/LIGHTWEIGHT-CONSTRAINTS.md)** — one -page, one entry per constraint, each with the pinned revision it was verified -at and what would retire it. It exists because three rungs in a row -rediscovered the same limits and recorded them in their own plan documents, -where the next rung could not find them and where two of them quietly went -stale. If you hit a new one, add it there rather than to your plan. +**Before you design around a Lightweight limitation, check +[Lightweight's issues](https://github.com/LASTRADA-Software/Lightweight/issues)**, +and file one there if it is not already recorded. A limitation written into a +plan document goes stale the moment the pin moves; one filed upstream is fixed +or stays visible. **The promotion rule (rule-of-three, from the round-7 review):** an app-built answer to a framework gap (the polling helper with its timeout, diff --git a/examples/bank/CMakeLists.txt b/examples/bank/CMakeLists.txt index 90347ea50..0f0d5894c 100644 --- a/examples/bank/CMakeLists.txt +++ b/examples/bank/CMakeLists.txt @@ -57,7 +57,7 @@ morph_use_cpm(Lightweight) CPMAddPackage( NAME Lightweight GITHUB_REPOSITORY LASTRADA-Software/Lightweight - GIT_TAG bbb972a78e1962b968a2c6ad93f7dade736eaa01) + GIT_TAG 17c4e70d7fb4ead01376be0f46f201ad41c9a701) set(CMAKE_SKIP_INSTALL_RULES ${_morph_saved_skip_install_rules}) unset(_morph_saved_skip_install_rules) @@ -208,10 +208,6 @@ if(MORPH_BUILD_TESTS) tests/test_lightweight_offline_queue.cpp tests/test_relations.cpp tests/test_stateful_account.cpp - # Not a behavioural test of bank: the record of why - # examples/common/db/pool_transaction_audit.hpp is deliberately - # not linked here. See the file's own header. - tests/test_pool_scope.cpp ) # ${PROJECT_SOURCE_DIR}/tests is morph's own test directory, and it is # on the include path for exactly one header: diff --git a/examples/bank/include/bank/db/user_ops.hpp b/examples/bank/include/bank/db/user_ops.hpp index fae63abf7..42987eaaf 100644 --- a/examples/bank/include/bank/db/user_ops.hpp +++ b/examples/bank/include/bank/db/user_ops.hpp @@ -3,9 +3,11 @@ #include #include +#include #include #include #include +#include #include "bank/core/errors.hpp" #include "bank/db/user_entity.hpp" @@ -43,6 +45,24 @@ namespace bank::db { throw Unauthorized{"unknown user: " + std::string{username}}; } +/// @brief Unwraps a relation access on a record this model just read. +/// +/// The record came from a live query on the current connection, so an +/// unavailable relation means the database failed under us, not that the +/// caller did something wrong. +/// @tparam T What the relation access yields. +/// @param result The relation accessor's result. +/// @param relation The relation's name, for the message. +/// @return The loaded value. +/// @throws BankError when the relation could not be loaded. +template +[[nodiscard]] T requireLoaded(Lightweight::RelationResult result, std::string_view relation) { + if (!result.has_value()) { + throw BankError{std::format("{} could not be loaded: {}", relation, Lightweight::to_string(result.error()))}; + } + return std::move(*result); +} + /// @brief Returns the id of the user named @p username, creating a minimal row /// if none exists yet (demo convenience used by `App::login`). /// diff --git a/examples/bank/src/db/schema.cpp b/examples/bank/src/db/schema.cpp index eb23d8f3b..0080396e9 100644 --- a/examples/bank/src/db/schema.cpp +++ b/examples/bank/src/db/schema.cpp @@ -31,6 +31,10 @@ void applyMigrations() { auto& migrations = Lightweight::SqlMigration::MigrationManager::GetInstance(); migrations.CreateMigrationHistory(); migrations.ApplyPendingMigrations(); + // The migration manager is a static that keeps a DataMapper open between calls; + // one still open at exit is destroyed after Lightweight's default logger and + // aborts the process. Close it once the migrations are applied. + migrations.CloseDataMapper(); } void setup(const std::string& connectionString) { diff --git a/examples/bank/src/models/customer_model.cpp b/examples/bank/src/models/customer_model.cpp index 0dae7f6a8..5c38fabb7 100644 --- a/examples/bank/src/models/customer_model.cpp +++ b/examples/bank/src/models/customer_model.cpp @@ -74,8 +74,9 @@ dto::AccountList CustomerModel::execute(const dto::ListAccounts& action) { } dto::AccountList out; - out.accounts.reserve(user->accounts.Count()); - for (const auto& account : user->accounts.All()) { + const auto& accounts = db::requireLoaded(user->accounts.All(), "user accounts").get(); + out.accounts.reserve(accounts.size()); + for (const auto& account : accounts) { out.accounts.push_back(db::toAccountInfo(*account, owner)); } return out; diff --git a/examples/bank/src/models/statement_model.cpp b/examples/bank/src/models/statement_model.cpp index 4648dd115..c79a6c037 100644 --- a/examples/bank/src/models/statement_model.cpp +++ b/examples/bank/src/models/statement_model.cpp @@ -26,7 +26,7 @@ dto::Statement StatementModel::execute(const dto::GenerateStatement& action) { // Reach the owner's accounts through the `UserRecord::accounts` relation. const auto userId = db::requireUserId(mapper(), owner); auto user = mapper().QuerySingle(userId).value(); - const auto& accounts = user.accounts.All(); + const auto& accounts = db::requireLoaded(user.accounts.All(), "user accounts").get(); dto::Statement statement; statement.owner = owner; diff --git a/examples/bank/tests/test_pool_scope.cpp b/examples/bank/tests/test_pool_scope.cpp deleted file mode 100644 index ccfc3a94e..000000000 --- a/examples/bank/tests/test_pool_scope.cpp +++ /dev/null @@ -1,137 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 - -#include -#include -#include -#include -#include -#include - -#include "bank/app/app.hpp" -#include "bank/core/types.hpp" -#include "bank/dto/account_dto.hpp" -#include "bank/dto/transaction_dto.hpp" -#include "bank/models/customer_model.hpp" -#include "bank/models/transaction_model.hpp" -#include "bank_test_support.hpp" - -/// @file -/// Why `examples/common/db/pool_transaction_audit.hpp` is **not** installed in -/// bank. -/// -/// `PoolTransactionAudit` detects a pooled `Lightweight::DataMapper` handed on -/// with `SQL_ATTR_AUTOCOMMIT` still `OFF`. It is installed from all seven ladder -/// rungs and from `testkit/DbFixture`, so extending it to bank looks like an -/// oversight closed -- on the premise that "bank's models acquire from -/// `Lightweight::GlobalDataMapperPool()` like every rung does". -/// -/// That premise is false, and this file is the measurement that says so. -/// `bank::db::WithMapper::mapper()` (`bank/db/db_model.hpp`) constructs a -/// `Lightweight::DataMapper` directly, one per model, and -/// `LightweightOfflineQueue` owns another; nothing under `examples/bank/` -/// names the pool. At the pinned Lightweight revision -/// (`bbb972a78e1962b968a2c6ad93f7dade736eaa01`) the two hooks the audit reads, -/// `SqlLogger::OnConnectionIdle` and `::OnConnectionReuse`, are emitted from -/// `src/Lightweight/DataMapper/Pool.hpp` and from nowhere else -- so an audit -/// installed in bank would inspect zero hand-offs and could not fail. That is -/// a control measuring nothing, which is the failure mode `AGENTS.md` names -/// first, so bank keeps the audit out and keeps this instead. -/// -/// The test is in two halves on purpose. A bare "bank performs no pool -/// hand-offs" assertion is indistinguishable from a counter that was never -/// wired up, so the first half drives a real hand-off through the same counter -/// and requires it to move. Only then is the second half's zero evidence. -/// -/// It is also the tripwire the audit would have been: if bank ever moves its -/// models onto `GlobalDataMapperPool()`, this case fails and installing the -/// audit here becomes worth doing. - -namespace { - -/// @brief Counts the two pool hand-off hooks and leaves every other one a no-op. -/// -/// Installs itself as the process-wide `Lightweight::SqlLogger` and restores -/// the previous one on destruction, exactly as `PoolTransactionAudit` does. -/// The counter is atomic because bank runs its models on a worker strand, so -/// the hooks fire on a thread other than the one reading the count. -class PoolHandoffCounter : public Lightweight::SqlLogger::Null { -public: - PoolHandoffCounter() : _previous{&Lightweight::SqlLogger::GetLogger()} { - Lightweight::SqlLogger::SetLogger(*this); - } - - PoolHandoffCounter(const PoolHandoffCounter&) = delete; - PoolHandoffCounter& operator=(const PoolHandoffCounter&) = delete; - PoolHandoffCounter(PoolHandoffCounter&&) = delete; - PoolHandoffCounter& operator=(PoolHandoffCounter&&) = delete; - - ~PoolHandoffCounter() override { - if (&Lightweight::SqlLogger::GetLogger() == this) { - Lightweight::SqlLogger::SetLogger(*_previous); - } - } - - void OnConnectionIdle(const Lightweight::SqlConnection& /*connection*/) override { _handoffs.fetch_add(1); } - - void OnConnectionReuse(const Lightweight::SqlConnection& /*connection*/) override { _handoffs.fetch_add(1); } - - [[nodiscard]] unsigned long handoffs() const noexcept { return _handoffs.load(); } - - void reset() noexcept { _handoffs.store(0); } - -private: - std::atomic _handoffs{0}; - Lightweight::SqlLogger* _previous; -}; - -} // namespace - -TEST_CASE("bank's persistence never hands a DataMapper through Lightweight's pool", "[bank][pool]") { - bank::testing::ensureDatabase(); - - PoolHandoffCounter counter; - - // ── Half one: the instrument, proven on a real hand-off ────────────────── - // Acquiring from the global pool and returning the mapper drives - // OnConnectionReuse and/or OnConnectionIdle. If this does not move, the - // zero below would mean nothing. - { - auto pooled = Lightweight::GlobalDataMapperPool().Acquire(); - REQUIRE(pooled.Get().Connection().NativeHandle() != nullptr); - } // <- returned to the pool here - REQUIRE(counter.handoffs() > 0); - - // ── Half two: the measurement ──────────────────────────────────────────── - counter.reset(); - - bank::app::App app{bank::testing::connectionString()}; - app.login("pool-scope-probe"); - morph::bridge::BridgeHandler customer{app.bridge(), app.gui()}; - morph::bridge::BridgeHandler txns{app.bridge(), app.gui()}; - - // The two money-movement paths that open a `SqlTransaction` over - // `mapper().Connection()` -- `TransactionModel::execute(Deposit)` and - // `execute(Transfer)` -- are precisely what the audit would police, so they - // are what is driven here. - const auto source = bank::testing::await(customer.execute(bank::dto::OpenAccount{ - .kind = static_cast(bank::AccountKind::Checking), - .currency = static_cast(bank::Currency::EUR), - }), - app.guiLoop()); - const auto destination = bank::testing::await(customer.execute(bank::dto::OpenAccount{ - .kind = static_cast(bank::AccountKind::Checking), - .currency = static_cast(bank::Currency::EUR), - }), - app.guiLoop()); - - bank::testing::await( - txns.execute(bank::dto::Deposit{.accountId = source.id, .amountMinor = 5000, .description = "probe"}), - app.guiLoop()); - bank::testing::await( - txns.execute(bank::dto::Transfer{ - .fromAccountId = source.id, .toAccountId = destination.id, .amountMinor = 2500, .description = "probe"}), - app.guiLoop()); - - // Zero, and it is evidence because half one showed the same counter moving. - CHECK(counter.handoffs() == 0); -} diff --git a/examples/bank/tests/test_relations.cpp b/examples/bank/tests/test_relations.cpp index 8b349cb8b..1a0e70ac7 100644 --- a/examples/bank/tests/test_relations.cpp +++ b/examples/bank/tests/test_relations.cpp @@ -95,10 +95,14 @@ TEST_CASE("ORM relations: BelongsTo navigation and HasMany inverses", "[relation SECTION("HasMany: user.accounts contains the opened accounts") { auto user = dm.QuerySingle(userId).value(); - REQUIRE(user.accounts.Count() >= 2); + const auto count = user.accounts.Count(); + REQUIRE(count.has_value()); + REQUIRE(*count >= 2); + auto accounts = user.accounts.All(); + REQUIRE(accounts.has_value()); bool sawChecking = false; bool sawSavings = false; - for (const auto& acct : user.accounts.All()) { + for (const auto& acct : accounts->get()) { const auto id = static_cast(acct->id.Value()); sawChecking = sawChecking || id == checking.id; sawSavings = sawSavings || id == savings.id; diff --git a/examples/bookmarks/src/db/schema.cpp b/examples/bookmarks/src/db/schema.cpp index b4284bb22..ede8abd22 100644 --- a/examples/bookmarks/src/db/schema.cpp +++ b/examples/bookmarks/src/db/schema.cpp @@ -2,25 +2,19 @@ #include #include #include -#include #include "bookmarks/db/database.hpp" namespace bookmarks::db { void setup(const std::string& connectionString) { - // Nothing in Lightweight stops a pooled DataMapper from being returned with - // a transaction still open on it -- `DataMapperPool::Return` does no - // transaction cleanup, and the cost lands on the next, unrelated borrower - // as a 60s stall and a `database is locked` it did not cause. Installing - // the audit here, at the one point every rung's process configures its - // database, turns that into an abort at the leak. See - // examples/common/db/pool_transaction_audit.hpp. - (void)::morph::ladder::db::installPoolTransactionAudit(); - Lightweight::SqlConnection::SetDefaultConnectionString(Lightweight::SqlConnectionString{connectionString}); Lightweight::SqlMigration::MigrationManager::GetInstance().CreateMigrationHistory(); Lightweight::SqlMigration::MigrationManager::GetInstance().ApplyPendingMigrations(); + // The migration manager is a static that keeps a DataMapper open between calls; + // one still open at exit is destroyed after Lightweight's default logger and + // aborts the process. Close it once the migrations are applied. + Lightweight::SqlMigration::MigrationManager::GetInstance().CloseDataMapper(); } } // namespace bookmarks::db diff --git a/examples/common/CMakeLists.txt b/examples/common/CMakeLists.txt index fcec8f230..0c8cd5673 100644 --- a/examples/common/CMakeLists.txt +++ b/examples/common/CMakeLists.txt @@ -151,7 +151,7 @@ morph_use_cpm(Lightweight) CPMAddPackage( NAME Lightweight GITHUB_REPOSITORY LASTRADA-Software/Lightweight - GIT_TAG bbb972a78e1962b968a2c6ad93f7dade736eaa01) + GIT_TAG 17c4e70d7fb4ead01376be0f46f201ad41c9a701) set(CMAKE_SKIP_INSTALL_RULES ${_morph_saved_skip_install_rules}) unset(_morph_saved_skip_install_rules) @@ -247,19 +247,12 @@ endif() # ── ladder_common_tests: the testkit's own self-test suite ────────────────── add_executable(ladder_common_tests testkit/testkit_main.cpp - # Not a test: the self-containment check for db/pool_transaction_audit.hpp, - # and the translation unit that gives examples/common/db/ a compile command - # for the clang-tidy-diff gate to interpolate from. See its own @file - # comment. It rides in this target because this is the only one that - # already links Lightweight and compiles everything under examples/common. - db/pool_transaction_audit.cpp testkit/test_pump.cpp testkit/test_clock.cpp testkit/test_db_fixture.cpp testkit/test_db_fault_fixture.cpp testkit/test_db_busy_fixture.cpp testkit/test_db_pool_drain.cpp - testkit/test_pool_transaction_audit.cpp testkit/test_backend_rig.cpp testkit/test_presenter.cpp testkit/test_error_text.cpp diff --git a/examples/common/db/pool_transaction_audit.cpp b/examples/common/db/pool_transaction_audit.cpp deleted file mode 100644 index 6196c5727..000000000 --- a/examples/common/db/pool_transaction_audit.cpp +++ /dev/null @@ -1,36 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 -#include "db/pool_transaction_audit.hpp" - -/// @file -/// The self-containment check for `db/pool_transaction_audit.hpp`, and the -/// only translation unit in this directory. -/// -/// Both of those are load-bearing, and the second is the less obvious one. -/// `clang-tidy` resolves a file's compile command from -/// `compile_commands.json`, and a header never has an entry of its own: for a -/// changed header, `clang-tidy-diff.py` interpolates the *nearest* entry and -/// analyses it with that. `.github/workflows/ci.yml`'s changed-source filter -/// deliberately does not skip headers, so the interpolation is what the -/// clang-tidy-diff gate actually runs. With no translation unit anywhere in -/// this directory the nearest entry was one that does not carry Lightweight's -/// include path, and analysing the header with it produced -/// `'Lightweight/SqlConnection.hpp' file not found` -- a -/// `clang-diagnostic-error` that fails the gate, plus a scatter of findings -/// from the broken parse (measured before this file existed: a -/// `cppcoreguidelines-pro-type-member-init` naming two fields the constructor -/// visibly initialises, and a `readability-identifier-naming` on a member -/// spelled exactly like every other member in the tree). -/// -/// This file gives the directory an entry whose command is right, which is -/// what the interpolation then picks up. It earns its place on its own terms -/// too: compiling the header alone, first, is how a missing `#include` in it -/// is caught here rather than in whichever consumer happens to include it -/// second. -/// -/// It is deliberately **not** a Catch2 translation unit and deliberately not -/// in `examples/common/testkit/`: that directory's `.clang-tidy` subtracts -/// `bugprone-chained-comparison` on an argument about Catch2's `REQUIRE` -/// expansion, and a library .cpp placed there would inherit that subtraction -/// on the strength of an argument that does not apply to it. Nothing enforces -/// the split, so it is kept by hand. The audit's behavioural tests live in -/// `testkit/test_pool_transaction_audit.cpp`, where they belong. diff --git a/examples/common/db/pool_transaction_audit.hpp b/examples/common/db/pool_transaction_audit.hpp deleted file mode 100644 index 291e37400..000000000 --- a/examples/common/db/pool_transaction_audit.hpp +++ /dev/null @@ -1,210 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 -#pragma once - -#include -#include -#include -#include -#include -#include -#include -#include - -/// @file -/// A process-wide check that no pooled `Lightweight::DataMapper` is ever -/// returned to `Lightweight::GlobalDataMapperPool()` with a transaction still -/// open on its connection. -/// -/// @par The defect this exists to catch -/// `Lightweight::Pool::Return` (`src/Lightweight/DataMapper/Pool.hpp` -/// at the pinned revision `bbb972a78e1962b968a2c6ad93f7dade736eaa01`) performs -/// **no transaction cleanup** on a returned connection. All three -/// growth-strategy overloads do the same three things: -/// -/// @code -/// void Return(std::unique_ptr dm) noexcept -/// { -/// DropAsyncBackend(*dm); -/// SqlLogger::GetLogger().OnConnectionIdle(dm->Connection()); -/// std::scoped_lock lock(_mutex); -/// _idleDataMappers.push_back(std::move(dm)); -/// } -/// @endcode -/// -/// and `DropAsyncBackend` is `dm.Connection().DisableAsync();` and nothing -/// else (`Pool.hpp:133-136`), which is `m_data->asyncBackend.reset();` -/// (`SqlConnection.cpp:214-217`). No `SQLEndTran`, no autocommit reset. -/// -/// A connection returned with `SQL_ATTR_AUTOCOMMIT` still `OFF` therefore -/// carries an open transaction into the pool, where it is handed to whichever -/// **unrelated** caller acquires next. That caller inherits a transaction it -/// never began, and the first write it attempts blocks for the full SQLite -/// busy timeout -- **60 seconds**, the value -/// `Lightweight::SqlConnection::PostConnect()` sets, not the `Timeout=5000` in -/// a connection string -- before reporting `database is locked`. The failure -/// lands on code that did nothing wrong, with no diagnostic naming the leak. -/// -/// @par What actually prevents it today, and why that is not enough -/// `Lightweight::SqlTransaction::Commit()` and `::Rollback()` each restore -/// autocommit themselves, immediately (`SqlTransaction.cpp:52-91`), so a -/// handler that commits explicitly is safe from that line onward. A handler -/// that does *not* -- one relying on the destructor's default -/// `COMMIT`/`ROLLBACK` -- is safe only because the `SqlTransaction` local is -/// declared **after** the pooled mapper and so destructs **before** it. That -/// is a correctness property held by declaration order, which nothing checks -/// and one refactor undoes: moving the transaction into a member, a -/// `std::optional`, or any longer-lived scope breaks it silently. -/// -/// @par How this catches it -/// `Pool::Return` and `Pool::Acquire` both announce every hand-off through the -/// process-wide `Lightweight::SqlLogger` -- `OnConnectionIdle` when a mapper is -/// parked, `OnConnectionReuse` when one changes hands. At each of those points -/// the connection belongs to nobody, so its autocommit flag must be `ON`. -/// `PoolTransactionAudit` installs itself as that logger and reads -/// `SQL_ATTR_AUTOCOMMIT` at every hand-off; an `OFF` fails **at the leak** -/// rather than sixty seconds later in someone else's call. -/// -/// @par What it does not catch -/// A raw `BEGIN` issued through `SqlStatement` (rather than through -/// `SqlTransaction`) leaves ODBC's autocommit flag `ON` while SQLite holds a -/// transaction open, and no ODBC connection attribute reports that. The ledger -/// rung's `DeferredReadTransactionGuard` is the one place in this tree that -/// does it, and it is RAII-scoped around a single aggregation; see its own doc -/// comment. - -namespace morph::ladder::db { - -/// @brief What `SQL_ATTR_AUTOCOMMIT` says about a connection. -enum class AutocommitState : unsigned char { - On, ///< Autocommit is on: no ODBC-level transaction is open. - Off, ///< Autocommit is off: a transaction is open on this connection. - Unknown, ///< The driver refused the query; nothing can be concluded. -}; - -/// @brief Reads `SQL_ATTR_AUTOCOMMIT` off @p connection. -/// -/// Driver-local: `SQLGetConnectAttr` for this attribute answers from the -/// driver's own state and issues no statement, so this is cheap enough to run -/// on every pool hand-off. -/// -/// @param connection The connection to interrogate. -/// @return `On`, `Off`, or `Unknown` if the driver did not answer. -[[nodiscard]] inline AutocommitState autocommitStateOf(const ::Lightweight::SqlConnection& connection) noexcept { - SQLUINTEGER value = 0; - const SQLRETURN status = SQLGetConnectAttr(connection.NativeHandle(), SQL_ATTR_AUTOCOMMIT, &value, 0, nullptr); - if (status != SQL_SUCCESS && status != SQL_SUCCESS_WITH_INFO) { - return AutocommitState::Unknown; - } - return value == SQL_AUTOCOMMIT_ON ? AutocommitState::On : AutocommitState::Off; -} - -/// @brief A `Lightweight::SqlLogger` that fails a pooled hand-off carrying an -/// open transaction. -/// -/// Installs itself as the process-wide logger on construction and restores -/// whatever was there before on destruction (only if it is still the installed -/// one, so nesting is safe). Every other hook stays the `SqlLogger::Null` -/// no-op: this is a check wearing a logger's clothes, not a log. -class PoolTransactionAudit : public ::Lightweight::SqlLogger::Null { -public: - /// @brief What to do when a hand-off is found carrying a transaction. - /// - /// Takes the diagnostic message. The default aborts; a test passes its own - /// to observe the detection instead of dying of it. - using LeakHandler = std::function; - - /// @brief Installs this audit as the process-wide `SqlLogger`. - /// @param onLeak Called with a diagnostic naming the hand-off, once per - /// detection. Empty selects the default: write the diagnostic to - /// `stderr` and `std::abort()`, so a leak fails at the leak. - explicit PoolTransactionAudit(LeakHandler onLeak = {}) - : _onLeak{std::move(onLeak)}, _previous{&::Lightweight::SqlLogger::GetLogger()} { - ::Lightweight::SqlLogger::SetLogger(*this); - } - - PoolTransactionAudit(const PoolTransactionAudit&) = delete; - PoolTransactionAudit& operator=(const PoolTransactionAudit&) = delete; - PoolTransactionAudit(PoolTransactionAudit&&) = delete; - PoolTransactionAudit& operator=(PoolTransactionAudit&&) = delete; - - /// @brief Restores the previously installed logger, if this one is still - /// the installed one. - ~PoolTransactionAudit() override { - if (&::Lightweight::SqlLogger::GetLogger() == this) { - ::Lightweight::SqlLogger::SetLogger(*_previous); - } - } - - /// @brief Checks the connection `Pool::Return` is about to park. - /// @param connection The connection being idled. - void OnConnectionIdle(const ::Lightweight::SqlConnection& connection) override { - check(connection, "idled into the pool"); - } - - /// @brief Checks the connection the pool is about to hand to another - /// owner (`Pool::Return` to a waiter, or `Pool::Acquire` off the - /// idle list). - /// @param connection The connection changing hands. - void OnConnectionReuse(const ::Lightweight::SqlConnection& connection) override { - check(connection, "handed to another borrower"); - } - - /// @brief How many hand-offs this audit has rejected. - /// @return The detection count since construction. - [[nodiscard]] unsigned long detections() const noexcept { return _detections; } - -private: - // NOLINTNEXTLINE(bugprone-easily-swappable-parameters) -- a connection and a phrase, not interchangeable - void check(const ::Lightweight::SqlConnection& connection, std::string_view what) { - if (autocommitStateOf(connection) != AutocommitState::Off) { - // `On` is the good case. `Unknown` means the driver would not - // answer -- reporting that as a leak would turn an unsupported - // driver into a spurious abort, so it is deliberately not one. - return; - } - ++_detections; - std::string message = "PoolTransactionAudit: a pooled DataMapper was "; - message += what; - message += - " with SQL_ATTR_AUTOCOMMIT still OFF -- a transaction is open on it. " - "DataMapperPool::Return performs no transaction cleanup, so the next, unrelated borrower of this " - "connection inherits the transaction and stalls for the full 60s busy_timeout on its first write. " - "Commit or roll back before the pooled mapper goes out of scope, and do not rely on the " - "SqlTransaction local being declared after the mapper."; - if (_onLeak) { - _onLeak(message); - return; - } - (void)std::fputs(message.c_str(), stderr); - (void)std::fputc('\n', stderr); - std::abort(); - } - - LeakHandler _onLeak; - ::Lightweight::SqlLogger* _previous; - unsigned long _detections = 0; -}; - -/// @brief Installs a process-wide `PoolTransactionAudit` with the default -/// fatal handler, exactly once per process. -/// -/// Idempotent and safe to call from every rung's `db::configure()`: the second -/// and later calls do nothing. The audit outlives every pooled mapper because -/// pooled mappers are function locals, and it de-registers itself if it is -/// destroyed at exit while still installed. -/// -/// Returns the audit rather than `void` because the object is genuinely -/// non-`const` -- `Lightweight::SqlLogger::SetLogger` stores a non-`const` -/// pointer to it and the pool calls back through that to mutate its detection -/// count -- and a `void` version left the local looking like a `const` -/// candidate to `misc-const-correctness`, which it is not: making it `const` -/// would leave the pool writing to a `const` object. -/// -/// @return The process-wide audit. Callers that only want it installed may -/// discard it. -inline PoolTransactionAudit& installPoolTransactionAudit() { - static PoolTransactionAudit audit; - return audit; -} - -} // namespace morph::ladder::db diff --git a/examples/common/testkit/db_fixture.hpp b/examples/common/testkit/db_fixture.hpp index 0fd16a2ec..57a9f8a4e 100644 --- a/examples/common/testkit/db_fixture.hpp +++ b/examples/common/testkit/db_fixture.hpp @@ -16,8 +16,6 @@ #include #include -#include "db/pool_transaction_audit.hpp" - /// @file /// Real on-disk SQLite database, shared per test binary — mirrors /// Lightweight's own `SqlTestFixture` (Lightweight/src/tests/Utils.hpp) and @@ -48,6 +46,10 @@ class DbFixture { clearAndReport("a non-std::exception was thrown"); // never returns } ::Lightweight::SqlMigration::MigrationManager::GetInstance().ApplyPendingMigrations(); + // The migration manager is a static that keeps a DataMapper open between calls; + // one still open at exit is destroyed after Lightweight's default logger and + // aborts the process. Close it once the migrations are applied. + ::Lightweight::SqlMigration::MigrationManager::GetInstance().CloseDataMapper(); } DbFixture(const DbFixture&) = delete; @@ -159,14 +161,6 @@ class DbFixture { /// no branch of its own left to miss. static void ensureConnectionConfigured() { static const bool once = [] { - // Installed here as well as in every rung's own - // `db::setup()`/`db::configure()`, because no ladder test goes - // through those -- this fixture points Lightweight at the test - // database itself. Without it the audit would be live only in - // binaries the suite never runs, which is a control that measures - // nothing. With it, every ladder test case in the suite runs under - // the check. - (void)::morph::ladder::db::installPoolTransactionAudit(); ::Lightweight::SqlConnection::SetDefaultConnectionString( ::Lightweight::SqlConnectionString{activeConnectionString()}); ::Lightweight::SqlMigration::MigrationManager::GetInstance().CreateMigrationHistory(); @@ -225,13 +219,10 @@ class DbFixture { /// @brief Reads every user table out of SQLite's own catalogue, with the /// DDL that created it. /// - /// Deliberately not `SqlSchema::ReadAllTables`: that is the function whose - /// failure brings us here. It resolves every foreign key's target through - /// a name map built from the tables it enumerated, so a foreign key naming - /// a table that is no longer there reaches a `map::at` and throws - /// `std::out_of_range("map::at")`. `sqlite_master` resolves nothing and - /// cannot fail that way, which is exactly the property a recovery path - /// needs. + /// Deliberately not `SqlSchema::ReadAllTables`: this is the recovery path + /// for when that read fails, so it must not depend on it. `sqlite_master` + /// resolves nothing -- no foreign key targets, no column types -- which is + /// exactly the property a recovery path needs. /// /// @param stmt A usable statement on the shared test connection. /// @return Every table except SQLite's own `sqlite_%` bookkeeping ones. @@ -363,9 +354,9 @@ class DbFixture { /// handle is not one test's problem: it is every later test's, in this run /// and in every run after it, until somebody deletes a file that nothing /// told them about. That is the expensive half of the failure; the cheap - /// half is a throw that says only `map::at`, attributed to the `TEST_CASE` - /// line, naming neither the fixture, nor the file, nor the fact that the - /// state is on disk at all. + /// half is a bare exception message attributed to the `TEST_CASE` line, + /// naming neither the fixture, nor the file, nor the fact that the state + /// is on disk at all. /// /// Emptying the database quietly would trade that for something worse: a /// schema defect that reproduces every run becomes one that reproduces @@ -422,9 +413,7 @@ class DbFixture { "\n" "That database is a real file, shared by every test in this\n" "binary and kept between runs by design, so a state the drop\n" - "sweep cannot handle outlives the run that created it. `map::at`\n" - "is Lightweight's SqlSchema::ReadAllTables failing to resolve a\n" - "foreign key whose target table is missing.\n"; + "sweep cannot handle outlives the run that created it.\n"; if (overridden) { report += diff --git a/examples/common/testkit/test_db_fixture.cpp b/examples/common/testkit/test_db_fixture.cpp index 09bdd95d1..df0734bc4 100644 --- a/examples/common/testkit/test_db_fixture.cpp +++ b/examples/common/testkit/test_db_fixture.cpp @@ -2,7 +2,6 @@ #include #include #include -#include #include #include #include @@ -115,66 +114,27 @@ TEST_CASE("DbFixture::computeConnectionString uses ODBC_CONNECTION_STRING verbat "DRIVER=PostgreSQL;Database=whatever"); } -// When the shared database holds a foreign key whose target table is gone, -// Lightweight's SqlSchema::ReadAllTables throws std::out_of_range("map::at") -// out of DbFixture's constructor — before anything has been dropped, so the bad -// state survives the run that reported it and every test after it fails the -// same way, across invocations, until somebody deletes a file nothing names. -// -// Two test cases below, because the requirement has two halves and either can -// hold without the other: the message has to name the fixture, the file and -// the remedy, *and* the database has to be usable afterwards. A fixture that -// only improved the message would satisfy the first and fail the second. -namespace { - -/// @brief Leaves the shared database holding a foreign key whose target table -/// does not exist, constructs a `DbFixture` over it, and returns what -/// that constructor threw. -/// @return The exception's text, or an empty string if it did not throw — -/// which is itself a failure, and each caller asserts on it. -std::string reportFromPoisonedDatabase() { +// SQLite accepts a foreign key naming a table that does not exist -- the +// target is resolved at DML time, not at CREATE time -- so a test can leave +// one behind in the shared database file, where it survives into the next +// run. The fixture has to clear it rather than carry it into the next test. +TEST_CASE("DbFixture clears a dangling foreign key, so the next test is not a casualty of it", + "[ladder][testkit][db]") { { const morph::ladder::testkit::DbFixture fixture; Lightweight::SqlStatement stmt; - // SQLite accepts a foreign key naming a table that does not exist: - // the target is resolved at DML time, not at CREATE time. That is why - // this state is reachable at all, and why it survives on disk. (void)stmt.ExecuteDirect( "CREATE TABLE ladder_dangling_child " "(id INTEGER PRIMARY KEY, parent_id INTEGER REFERENCES ladder_absent_parent(id))"); } - try { - const morph::ladder::testkit::DbFixture poisoned; - } catch (const std::exception& error) { - return error.what(); - } - return {}; -} - -} // namespace - -TEST_CASE("DbFixture names itself, the database file and the remedy when the schema is unusable", - "[ladder][testkit][db]") { - const std::string reported = reportFromPoisonedDatabase(); - // Names the fixture, so the report is not read as the test's own fault... - CHECK_THAT(reported, Catch::Matchers::ContainsSubstring("DbFixture")); - CHECK_THAT(reported, Catch::Matchers::ContainsSubstring("NOT a failure of the test case")); - // ...names the file, which is the thing a reader has to act on... - CHECK_THAT(reported, Catch::Matchers::ContainsSubstring("morph_ladder_test.db")); - // ...names what was wrong, and what it destroyed in saying so... - CHECK_THAT(reported, Catch::Matchers::ContainsSubstring("ladder_dangling_child -> ladder_absent_parent")); - CHECK_THAT(reported, Catch::Matchers::ContainsSubstring("CREATE TABLE ladder_dangling_child")); - // ...and names the remedy. - CHECK_THAT(reported, Catch::Matchers::ContainsSubstring("Remedy:")); -} - -TEST_CASE("DbFixture leaves an unusable database usable, so the next test is not a casualty of it", - "[ladder][testkit][db]") { - REQUIRE_FALSE(reportFromPoisonedDatabase().empty()); - // The half that costs the most when it is missing: the next fixture - // constructs, because the bad state is gone rather than still on disk. REQUIRE_NOTHROW([] { const morph::ladder::testkit::DbFixture fixture; }()); const morph::ladder::testkit::DbFixture fixture; + { + Lightweight::SqlStatement stmt; + auto cursor = stmt.ExecuteDirect("SELECT COUNT(*) FROM sqlite_master WHERE name = 'ladder_dangling_child'"); + REQUIRE(cursor.FetchRow()); + CHECK(cursor.GetColumn(1) == 0); + } Lightweight::DataMapper mapper; LadderTestkitProbe row; row.label = "usable again"; diff --git a/examples/common/testkit/test_pool_transaction_audit.cpp b/examples/common/testkit/test_pool_transaction_audit.cpp deleted file mode 100644 index 1e180b425..000000000 --- a/examples/common/testkit/test_pool_transaction_audit.cpp +++ /dev/null @@ -1,114 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 -#include -#include -#include -#include -#include -#include -#include - -#include "db/pool_transaction_audit.hpp" -#include "testkit/db_fixture.hpp" - -// The audit's own acceptance. The claim under test is not "the suite passes" -// -- it is that a mapper deliberately returned to the pool mid-transaction is -// named by the audit, and that the audit is silent when nothing leaks. Both -// directions are asserted below, because a check that never fires and a check -// that always fires are indistinguishable from one that measures nothing. - -using morph::ladder::db::AutocommitState; -using morph::ladder::db::autocommitStateOf; -using morph::ladder::db::PoolTransactionAudit; - -namespace { - -/// @brief Collects the audit's diagnostics instead of aborting on them. -struct LeakCollector { - std::vector messages; - - [[nodiscard]] PoolTransactionAudit::LeakHandler handler() { - return [this](std::string_view message) { messages.emplace_back(message); }; - } -}; - -} // namespace - -TEST_CASE("autocommitStateOf reports what SqlTransaction does to a connection", "[ladder][testkit][db][pool]") { - const morph::ladder::testkit::DbFixture fixture; - - auto mapper = ::Lightweight::GlobalDataMapperPool().Acquire(); - - // The premise every other assertion in this file rests on: the SQLite ODBC - // driver answers SQLGetConnectAttr(SQL_ATTR_AUTOCOMMIT) at all. If it - // returned Unknown, the audit would be a control that measures nothing. - REQUIRE(autocommitStateOf(mapper->Connection()) == AutocommitState::On); - - { - const ::Lightweight::SqlTransaction transaction{mapper->Connection(), - ::Lightweight::SqlTransactionMode::ROLLBACK}; - CHECK(autocommitStateOf(mapper->Connection()) == AutocommitState::Off); - } - CHECK(autocommitStateOf(mapper->Connection()) == AutocommitState::On); - - // And the part that is easy to get backwards: Commit() restores - // autocommit itself, there and then -- it does not wait for the - // destructor. Every statement after an explicit Commit() is therefore - // already back in autocommit, however long the SqlTransaction local - // stays in scope. - { - ::Lightweight::SqlTransaction transaction{mapper->Connection(), ::Lightweight::SqlTransactionMode::ROLLBACK}; - REQUIRE(autocommitStateOf(mapper->Connection()) == AutocommitState::Off); - transaction.Commit(); - CHECK(autocommitStateOf(mapper->Connection()) == AutocommitState::On); - } -} - -TEST_CASE("PoolTransactionAudit is silent when a pooled mapper is returned cleanly", "[ladder][testkit][db][pool]") { - const morph::ladder::testkit::DbFixture fixture; - - LeakCollector collector; - const PoolTransactionAudit audit{collector.handler()}; - - // The ordinary, correct shape: the mapper is acquired first, the - // transaction declared after it, so the transaction destructs (restoring - // autocommit) before the mapper is returned. - { - auto mapper = ::Lightweight::GlobalDataMapperPool().Acquire(); - ::Lightweight::SqlTransaction transaction{mapper->Connection(), ::Lightweight::SqlTransactionMode::ROLLBACK}; - transaction.Commit(); - } - - CHECK(collector.messages.empty()); - CHECK(audit.detections() == 0); -} - -TEST_CASE("PoolTransactionAudit names a pooled mapper returned mid-transaction", "[ladder][testkit][db][pool]") { - const morph::ladder::testkit::DbFixture fixture; - - LeakCollector collector; - const PoolTransactionAudit audit{collector.handler()}; - - // The leak, built deliberately. `transaction` is declared *before* - // `mapper`, so `mapper` is destroyed first: the pooled DataMapper goes - // back into the pool while the SqlTransaction is still alive and - // autocommit is still OFF. This is exactly what a refactor that hoists a - // transaction into a member or a longer-lived scope produces, and on - // master nothing reports it. - { - std::optional<::Lightweight::SqlTransaction> transaction; - { - auto mapper = ::Lightweight::GlobalDataMapperPool().Acquire(); - transaction.emplace(mapper->Connection(), ::Lightweight::SqlTransactionMode::ROLLBACK); - REQUIRE(autocommitStateOf(mapper->Connection()) == AutocommitState::Off); - } // <- mapper returned to the pool here, transaction still open - - CHECK(audit.detections() >= 1); - REQUIRE_FALSE(collector.messages.empty()); - CHECK(collector.messages.front().contains("PoolTransactionAudit")); - CHECK(collector.messages.front().contains("SQL_ATTR_AUTOCOMMIT still OFF")); - - // Roll back before leaving, so the leaked connection is clean again - // for every test that runs after this one in the same process. - transaction->Rollback(); - } -} diff --git a/examples/crm/src/db/schema.cpp b/examples/crm/src/db/schema.cpp index 676bcab87..76ce66a7c 100644 --- a/examples/crm/src/db/schema.cpp +++ b/examples/crm/src/db/schema.cpp @@ -1,22 +1,12 @@ // SPDX-License-Identifier: Apache-2.0 #include #include -#include #include "crm/db/database.hpp" namespace crm::db { void configure(const std::string& connectionString) { - // Nothing in Lightweight stops a pooled DataMapper from being returned with - // a transaction still open on it -- `DataMapperPool::Return` does no - // transaction cleanup, and the cost lands on the next, unrelated borrower - // as a 60s stall and a `database is locked` it did not cause. Installing - // the audit here, at the one point every rung's process configures its - // database, turns that into an abort at the leak. See - // examples/common/db/pool_transaction_audit.hpp. - (void)::morph::ladder::db::installPoolTransactionAudit(); - Lightweight::SqlConnection::SetDefaultConnectionString(Lightweight::SqlConnectionString{connectionString}); } @@ -24,6 +14,10 @@ void applyMigrations() { auto& migrations = Lightweight::SqlMigration::MigrationManager::GetInstance(); migrations.CreateMigrationHistory(); migrations.ApplyPendingMigrations(); + // The migration manager is a static that keeps a DataMapper open between calls; + // one still open at exit is destroyed after Lightweight's default logger and + // aborts the process. Close it once the migrations are applied. + migrations.CloseDataMapper(); } void setup(const std::string& connectionString) { diff --git a/examples/kanban/src/db/schema.cpp b/examples/kanban/src/db/schema.cpp index 527ea6e55..63080427d 100644 --- a/examples/kanban/src/db/schema.cpp +++ b/examples/kanban/src/db/schema.cpp @@ -2,25 +2,19 @@ #include #include #include -#include #include "kanban/db/database.hpp" namespace kanban::db { void setup(const std::string& connectionString) { - // Nothing in Lightweight stops a pooled DataMapper from being returned with - // a transaction still open on it -- `DataMapperPool::Return` does no - // transaction cleanup, and the cost lands on the next, unrelated borrower - // as a 60s stall and a `database is locked` it did not cause. Installing - // the audit here, at the one point every rung's process configures its - // database, turns that into an abort at the leak. See - // examples/common/db/pool_transaction_audit.hpp. - (void)::morph::ladder::db::installPoolTransactionAudit(); - Lightweight::SqlConnection::SetDefaultConnectionString(Lightweight::SqlConnectionString{connectionString}); Lightweight::SqlMigration::MigrationManager::GetInstance().CreateMigrationHistory(); Lightweight::SqlMigration::MigrationManager::GetInstance().ApplyPendingMigrations(); + // The migration manager is a static that keeps a DataMapper open between calls; + // one still open at exit is destroyed after Lightweight's default logger and + // aborts the process. Close it once the migrations are applied. + Lightweight::SqlMigration::MigrationManager::GetInstance().CloseDataMapper(); } } // namespace kanban::db diff --git a/examples/ledger/src/db/schema.cpp b/examples/ledger/src/db/schema.cpp index 0763ce979..b3b60cbc3 100644 --- a/examples/ledger/src/db/schema.cpp +++ b/examples/ledger/src/db/schema.cpp @@ -1,22 +1,12 @@ // SPDX-License-Identifier: Apache-2.0 #include #include -#include #include "ledger/db/database.hpp" namespace ledger::db { void configure(const std::string& connectionString) { - // Nothing in Lightweight stops a pooled DataMapper from being returned with - // a transaction still open on it -- `DataMapperPool::Return` does no - // transaction cleanup, and the cost lands on the next, unrelated borrower - // as a 60s stall and a `database is locked` it did not cause. Installing - // the audit here, at the one point every rung's process configures its - // database, turns that into an abort at the leak. See - // examples/common/db/pool_transaction_audit.hpp. - (void)::morph::ladder::db::installPoolTransactionAudit(); - Lightweight::SqlConnection::SetDefaultConnectionString(Lightweight::SqlConnectionString{connectionString}); } @@ -24,6 +14,10 @@ void applyMigrations() { auto& migrations = Lightweight::SqlMigration::MigrationManager::GetInstance(); migrations.CreateMigrationHistory(); migrations.ApplyPendingMigrations(); + // The migration manager is a static that keeps a DataMapper open between calls; + // one still open at exit is destroyed after Lightweight's default logger and + // aborts the process. Close it once the migrations are applied. + migrations.CloseDataMapper(); } void setup(const std::string& connectionString) { diff --git a/examples/lims/src/db/schema.cpp b/examples/lims/src/db/schema.cpp index e1fe2bf4f..1e5371b97 100644 --- a/examples/lims/src/db/schema.cpp +++ b/examples/lims/src/db/schema.cpp @@ -1,22 +1,12 @@ // SPDX-License-Identifier: Apache-2.0 #include #include -#include #include "lims/db/database.hpp" namespace lims::db { void configure(const std::string& connectionString) { - // Nothing in Lightweight stops a pooled DataMapper from being returned with - // a transaction still open on it -- `DataMapperPool::Return` does no - // transaction cleanup, and the cost lands on the next, unrelated borrower - // as a 60s stall and a `database is locked` it did not cause. Installing - // the audit here, at the one point every rung's process configures its - // database, turns that into an abort at the leak. See - // examples/common/db/pool_transaction_audit.hpp. - (void)::morph::ladder::db::installPoolTransactionAudit(); - Lightweight::SqlConnection::SetDefaultConnectionString(Lightweight::SqlConnectionString{connectionString}); } @@ -24,6 +14,10 @@ void applyMigrations() { auto& migrations = Lightweight::SqlMigration::MigrationManager::GetInstance(); migrations.CreateMigrationHistory(); migrations.ApplyPendingMigrations(); + // The migration manager is a static that keeps a DataMapper open between calls; + // one still open at exit is destroyed after Lightweight's default logger and + // aborts the process. Close it once the migrations are applied. + migrations.CloseDataMapper(); } void setup(const std::string& connectionString) { diff --git a/examples/pastebin/src/db/schema.cpp b/examples/pastebin/src/db/schema.cpp index 992e0b50b..b9a354872 100644 --- a/examples/pastebin/src/db/schema.cpp +++ b/examples/pastebin/src/db/schema.cpp @@ -2,25 +2,19 @@ #include #include #include -#include #include "pastebin/db/database.hpp" namespace pastebin::db { void setup(const std::string& connectionString) { - // Nothing in Lightweight stops a pooled DataMapper from being returned with - // a transaction still open on it -- `DataMapperPool::Return` does no - // transaction cleanup, and the cost lands on the next, unrelated borrower - // as a 60s stall and a `database is locked` it did not cause. Installing - // the audit here, at the one point every rung's process configures its - // database, turns that into an abort at the leak. See - // examples/common/db/pool_transaction_audit.hpp. - (void)::morph::ladder::db::installPoolTransactionAudit(); - Lightweight::SqlConnection::SetDefaultConnectionString(Lightweight::SqlConnectionString{connectionString}); Lightweight::SqlMigration::MigrationManager::GetInstance().CreateMigrationHistory(); Lightweight::SqlMigration::MigrationManager::GetInstance().ApplyPendingMigrations(); + // The migration manager is a static that keeps a DataMapper open between calls; + // one still open at exit is destroyed after Lightweight's default logger and + // aborts the process. Close it once the migrations are applied. + Lightweight::SqlMigration::MigrationManager::GetInstance().CloseDataMapper(); } } // namespace pastebin::db diff --git a/examples/polls/src/db/schema.cpp b/examples/polls/src/db/schema.cpp index 047428878..c8bc1038a 100644 --- a/examples/polls/src/db/schema.cpp +++ b/examples/polls/src/db/schema.cpp @@ -2,25 +2,19 @@ #include #include #include -#include #include "polls/db/database.hpp" namespace polls::db { void setup(const std::string& connectionString) { - // Nothing in Lightweight stops a pooled DataMapper from being returned with - // a transaction still open on it -- `DataMapperPool::Return` does no - // transaction cleanup, and the cost lands on the next, unrelated borrower - // as a 60s stall and a `database is locked` it did not cause. Installing - // the audit here, at the one point every rung's process configures its - // database, turns that into an abort at the leak. See - // examples/common/db/pool_transaction_audit.hpp. - (void)::morph::ladder::db::installPoolTransactionAudit(); - Lightweight::SqlConnection::SetDefaultConnectionString(Lightweight::SqlConnectionString{connectionString}); Lightweight::SqlMigration::MigrationManager::GetInstance().CreateMigrationHistory(); Lightweight::SqlMigration::MigrationManager::GetInstance().ApplyPendingMigrations(); + // The migration manager is a static that keeps a DataMapper open between calls; + // one still open at exit is destroyed after Lightweight's default logger and + // aborts the process. Close it once the migrations are applied. + Lightweight::SqlMigration::MigrationManager::GetInstance().CloseDataMapper(); } } // namespace polls::db diff --git a/tests/test_journal_format_versioning.cpp b/tests/test_journal_format_versioning.cpp index de734ddc1..3208e589e 100644 --- a/tests/test_journal_format_versioning.cpp +++ b/tests/test_journal_format_versioning.cpp @@ -1,6 +1,6 @@ // SPDX-License-Identifier: Apache-2.0 // -// Coverage for journal format versioning & retention (todo.md B4): +// Coverage for journal format versioning & retention: // journal::fromJson's reader leniency, LogEntry::v / kLogFormatVersion, and // FileActionLog::rotate(). Companion to test_action_log.cpp and // test_action_log_phase2.cpp, which cover the rest of action_log.hpp and diff --git a/tests/test_outbox.cpp b/tests/test_outbox.cpp index 4acc9a14b..1e9f5563c 100644 --- a/tests/test_outbox.cpp +++ b/tests/test_outbox.cpp @@ -1,6 +1,6 @@ // SPDX-License-Identifier: Apache-2.0 // -// Coverage for the transactional outbox (todo.md B2, docs/planned/outbox.md): +// Coverage for the transactional outbox: // LogEntry::idempotencyKey dedup in InMemoryActionLog/FileActionLog, // IModelHolder::setOutboxManaged/isOutboxManaged suppressing recordIfAttached, // and journal::OutboxRelay.