lhumina_code/hero_lib

Fork 1

oschema rootobject CRUD (SmartID file-store): full-replace `set` can drop fields + N+1 `list`/`find` — document or harden #157

New issue

Open

opened 2026-06-16 13:15:38 +00:00 by sameh-farouk · 4 comments

sameh-farouk commented

2026-06-16 13:15:38 +00:00

Member

Summary

The oschema [rootobject] CRUD generator (crates/derive/src/openrpc_from_oschema.rs, emitting <type>.{new,get,set,delete,list,list_full,exists,find}) is backed by a SmartID file-store (one <sid>.otoml file per object) with two sharp edges that make it unsafe/slow for services whose objects have derived/computed/array fields or whose collections are high-volume / hot-listed. This isn't a single-service bug — every oschema-first migration hits the "use generated rootobject storage vs. hand-write handlers" decision and runs into it. Filing so it's tracked at the framework level rather than re-discovered per migration.

Already-settled context (so this isn't re-litigated)

#63 (closed) already establishes the governing principle: "the root objects are to support single CRUD implementation. Any schema that wants to use its own backend should not be marked rootobject." So the high-level guidance — don't mark rootobject if you need custom storage — exists. This issue is only about the two specific, currently-undocumented failure modes below.
The other recent codegen fixes are unrelated to this: #153 (service-method description off-by-one) and #101 (OSIS SDK ClientError import path) — both fixed, different bugs.

Verified mechanism

Confirmed against the generator the migrations build against (herolib 9657556), in crates/oschema/src/collection/mod.rs. The store is filesystem OTOML, not SQL — one <sid>.otoml per object.

set(&self, obj: &mut T) (~L241): serializes the whole typed object to OTOML and atomically overwrites its file (fs::write tmp + fs::rename). Full-object replace, no merge. → Lossy only when the typed object T isn't the complete source of truth: partial sets, or fields the service maintains outside T (derived/computed/array sub-collections) get dropped with no error. If the schema type holds every field, set is safe.
list(&self) (~L287): fs::read_dir → returns SmartIds only. Materializing full objects is get() per id → N+1 file reads. Fine for dozens/hundreds; slow for high-volume / frequently-listed collections.
find / indexing: find iterates; has_index() returns false — no query engine, joins, or indexes.

Feasibility today (unchanged by the shipped codegen fixes)

✅ Usable now for self-contained, low-to-moderate-volume rootobjects not listed on a hot path (config/catalog/settings-like, low-churn entities).
❌ Keep custom handlers for objects with derived/array fields, high-volume/frequently-listed data, side-effecting creates, or relational queries — e.g. hero_collab (chose all-custom, 0 rootobjects) and hero_livekit (next migration).

Evidence

hero_collab went all-custom (0 rootobjects) for all 110 methods specifically because of the above — decision record: hero_collab/docs/superpowers/collab-oschema-review-2026-06-15.md (4-reviewer architecture review). Generated _set would have dropped message-derived fields; sid-array _list/_find reproduced the hero_planner N+1.
hero_planner surfaced the N+1 first.

Ask

Either:

Harden the generated CRUD — set merge semantics (or explicit field-scoping so derived/array fields are clearly owned by custom handlers) and a batched/indexed list/find instead of N+1 get-per-id; or
Document these two traps by name next to the #63 principle, so migrators recognize them and make the call deliberately.

Pointers

Storage runtime: crates/oschema/src/collection/mod.rs (set ~L241, list ~L287).
CRUD emission: crates/derive/src/openrpc_from_oschema.rs.
Decision record + reasoning: hero_collab/docs/superpowers/collab-oschema-review-2026-06-15.md.

## Summary The oschema `[rootobject]` CRUD generator (`crates/derive/src/openrpc_from_oschema.rs`, emitting `<type>.{new,get,set,delete,list,list_full,exists,find}`) is backed by a **SmartID file-store** (one `<sid>.otoml` file per object) with two sharp edges that make it unsafe/slow for services whose objects have **derived/computed/array fields** or whose collections are **high-volume / hot-listed**. This isn't a single-service bug — every oschema-first migration hits the "use generated rootobject storage vs. hand-write handlers" decision and runs into it. Filing so it's tracked at the framework level rather than re-discovered per migration. ## Already-settled context (so this isn't re-litigated) - **#63** (closed) already establishes the governing principle: *"the root objects are to support single CRUD implementation. **Any schema that wants to use its own backend should not be marked `rootobject`.**"* So the high-level guidance — *don't mark `rootobject` if you need custom storage* — exists. This issue is **only** about the two specific, currently-undocumented failure modes below. - The other recent codegen fixes are **unrelated** to this: **#153** (service-method description off-by-one) and **#101** (OSIS SDK `ClientError` import path) — both fixed, different bugs. ## Verified mechanism Confirmed against the generator the migrations build against (herolib `9657556`), in `crates/oschema/src/collection/mod.rs`. The store is **filesystem OTOML, not SQL** — one `<sid>.otoml` per object. 1. **`set(&self, obj: &mut T)`** (~L241): serializes the whole typed object to OTOML and atomically overwrites its file (`fs::write` tmp + `fs::rename`). **Full-object replace, no merge.** → Lossy **only when the typed object `T` isn't the complete source of truth**: partial sets, or fields the service maintains outside `T` (derived/computed/array sub-collections) get dropped with no error. If the schema type holds every field, `set` is safe. 2. **`list(&self)`** (~L287): `fs::read_dir` → returns **SmartIds only**. Materializing full objects is `get()` per id → **N+1 file reads.** Fine for dozens/hundreds; slow for high-volume / frequently-listed collections. 3. **`find` / indexing**: `find` iterates; `has_index()` returns `false` — no query engine, joins, or indexes. ## Feasibility today (unchanged by the shipped codegen fixes) - ✅ **Usable now** for **self-contained, low-to-moderate-volume** rootobjects not listed on a hot path (config/catalog/settings-like, low-churn entities). - ❌ **Keep custom handlers** for objects with derived/array fields, high-volume/frequently-listed data, side-effecting creates, or relational queries — e.g. hero_collab (chose all-custom, 0 rootobjects) and hero_livekit (next migration). ## Evidence - hero_collab went **all-custom (0 rootobjects)** for all 110 methods specifically because of the above — decision record: `hero_collab/docs/superpowers/collab-oschema-review-2026-06-15.md` (4-reviewer architecture review). Generated `_set` would have dropped message-derived fields; sid-array `_list`/`_find` reproduced the hero_planner N+1. - hero_planner surfaced the N+1 first. ## Ask Either: 1. **Harden the generated CRUD** — `set` merge semantics (or explicit field-scoping so derived/array fields are clearly owned by custom handlers) **and** a batched/indexed `list`/`find` instead of N+1 `get`-per-id; or 2. **Document these two traps by name** next to the #63 principle, so migrators recognize them and make the call deliberately. ## Pointers - Storage runtime: `crates/oschema/src/collection/mod.rs` (`set` ~L241, `list` ~L287). - CRUD emission: `crates/derive/src/openrpc_from_oschema.rs`. - Decision record + reasoning: `hero_collab/docs/superpowers/collab-oschema-review-2026-06-15.md`.

sameh-farouk commented

2026-06-16 13:17:58 +00:00

Author

Member

Correction / cross-reference (the framework already establishes the core principle).

Closed #63 ("fix / improve code generation") already states the design intent: "the root objects are to support single CRUD implementation. Any schema that wants to use its own backend should not be marked rootobject." So the high-level guidance — don't mark rootobject if you need custom storage — is already settled, and I shouldn't have framed this as wholly undocumented. (For the record, the other closed codegen issues — #153 description off-by-one, #101 OSIS SDK ClientError path — are unrelated bugs, already fixed.)

What remains genuinely open (narrowing this issue to it): the specific failure modes a developer hits if they DO mark a rich object rootobject, which #63 doesn't call out:

generated _set replaces the stored record wholesale → server-derived / computed / array / extra fields are silently dropped (no error);
generated _list/_find are N+1 (per-row fetch).

Ask for this issue: either (a) harden the generated CRUD (_set merge semantics, single-query _list), or (b) just document these two specific traps next to the #63 principle so migrators recognize them by name. If the maintainers consider #63 sufficient and don't want the extra hardening/docs, this can be closed as covered.

Not re-verified against the very latest generator beyond the hero_collab review (2026-06-15, current stack) + the hero_planner N+1 — flag if #63's follow-ups already changed _set/_list behavior, in which case close this.

**Correction / cross-reference (the framework already establishes the core principle).** Closed **#63** ("fix / improve code generation") already states the design intent: *"the root objects are to support single CRUD implementation. Any schema that wants to use its own backend should not be marked `rootobject`."* So the high-level guidance — *don't mark `rootobject` if you need custom storage* — is **already settled**, and I shouldn't have framed this as wholly undocumented. (For the record, the other closed codegen issues — #153 description off-by-one, #101 OSIS SDK ClientError path — are unrelated bugs, already fixed.) **What remains genuinely open (narrowing this issue to it):** the *specific failure modes* a developer hits if they DO mark a rich object `rootobject`, which #63 doesn't call out: 1. generated `_set` replaces the stored record wholesale → server-derived / computed / array / extra fields are **silently dropped** (no error); 2. generated `_list`/`_find` are **N+1** (per-row fetch). Ask for this issue: either (a) harden the generated CRUD (`_set` merge semantics, single-query `_list`), or (b) just **document these two specific traps next to the #63 principle** so migrators recognize them by name. If the maintainers consider #63 sufficient and don't want the extra hardening/docs, this can be closed as covered. Not re-verified against the very latest generator beyond the hero_collab review (2026-06-15, current stack) + the hero_planner N+1 — flag if #63's follow-ups already changed `_set`/`_list` behavior, in which case close this.

sameh-farouk commented

2026-06-16 13:38:31 +00:00

Author

Member

Verified against the current generator (herolib 9657556, the rev the migrations build against) — confirming the mechanism rather than the review-doc summary.

The generated rootobject persistence is a SmartID file-store, not SQL: one <sid>.otoml file per object. Source: crates/oschema/src/collection/mod.rs.

set(&self, obj: &mut T) (~L241): serializes the whole typed object to OTOML and atomically overwrites its file (fs::write tmp + fs::rename). It is a full-object replace, no merge. So this is lossy only when the typed object T isn't the complete source of truth — i.e. partial sets, or fields the service maintains outside T (derived/computed/array sub-collections). If the schema type holds every field, set is safe. (This refines the earlier "silent data loss" wording.)
list(&self) (~L287): fs::read_dir → returns SmartIds only. Materializing full objects is get() per id → N+1 file reads. Inherent to the file-store; fine for dozens/hundreds, slow for high-volume/hot-listed collections.
find / no index: iterates; has_index() returns false — no query engine, joins, or indexes.

Feasibility (unchanged by the shipped codegen fixes #63/#153/#101):

✅ Usable today for self-contained, low-to-moderate-volume rootobjects not listed on a hot path (config/catalog/settings-like).
❌ Not advisable for objects with derived/array fields, high-volume/frequently-listed data, side-effecting creates, or relational queries — keep custom handlers (matches #63's principle).

What would move the line (this issue's ask): set merge semantics (or explicit field-scoping) + a batched/indexed list instead of N+1 get-per-id. Otherwise, document these two traps by name next to the #63 guidance.

**Verified against the current generator (herolib `9657556`, the rev the migrations build against) — confirming the mechanism rather than the review-doc summary.** The generated rootobject persistence is a **SmartID file-store**, not SQL: one `<sid>.otoml` file per object. Source: `crates/oschema/src/collection/mod.rs`. - **`set(&self, obj: &mut T)`** (~L241): serializes the whole typed object to OTOML and atomically overwrites its file (`fs::write` tmp + `fs::rename`). It is a **full-object replace, no merge.** So this is lossy **only when the typed object `T` isn't the complete source of truth** — i.e. partial sets, or fields the service maintains outside `T` (derived/computed/array sub-collections). If the schema type holds every field, `set` is safe. (This refines the earlier "silent data loss" wording.) - **`list(&self)`** (~L287): `fs::read_dir` → returns **SmartIds only**. Materializing full objects is `get()` per id → **N+1 file reads.** Inherent to the file-store; fine for dozens/hundreds, slow for high-volume/hot-listed collections. - **`find` / no index**: iterates; `has_index()` returns `false` — no query engine, joins, or indexes. **Feasibility (unchanged by the shipped codegen fixes #63/#153/#101):** - ✅ Usable today for **self-contained, low-to-moderate-volume** rootobjects not listed on a hot path (config/catalog/settings-like). - ❌ Not advisable for objects with derived/array fields, high-volume/frequently-listed data, side-effecting creates, or relational queries — keep custom handlers (matches #63's principle). **What would move the line (this issue's ask):** `set` merge semantics (or explicit field-scoping) + a batched/indexed `list` instead of N+1 `get`-per-id. Otherwise, document these two traps by name next to the #63 guidance.

sameh-farouk changed title from ~~oschema rootobject CRUD codegen: generated _set can silently drop fields + _list/_find are N+1 (fix or document)~~ to oschema rootobject CRUD (SmartID file-store): full-replace set can drop fields + N+1 list/find — document or harden

2026-06-16 13:41:52 +00:00

sameh-farouk commented

2026-06-16 22:03:54 +00:00

Author

Member

Downstream consumer instance: hero_planner #30 — planner's workspace load is N+1 (listEntities reconstructs via _find+_get) precisely because the generated rootobject *_list_full stubs return empty, i.e. the consumer symptom of this generator behavior. Fixing list/find here (batched/indexed, or real *_list_full) resolves it for planner (and any other rootobject consumer).

Downstream consumer instance: **hero_planner #30** — planner's workspace load is N+1 (`listEntities` reconstructs via `_find`+`_get`) precisely because the generated rootobject `*_list_full` stubs return empty, i.e. the consumer symptom of this generator behavior. Fixing `list`/`find` here (batched/indexed, or real `*_list_full`) resolves it for planner (and any other rootobject consumer).

sameh-farouk commented

2026-06-21 08:51:49 +00:00

Author

Member

Note from the CM50 OSIS convergence work: the N+1 half of this is apparently tolerated in the canonical pattern — the worked reference hero_biz backend/.../business_impl.rs implements company_list_full as list() then get() per item (N+1) on the OSIS DBTyped store, and that is the blessed example in home#309. So the actionable/contentious core here is really the _set wholesale-replace dropping derived/array fields, not the N+1 (which the team accepts for now). Suggest reframing this issue toward the _set data-loss semantics; the N+1 is "known/accepted" unless/until a batched or indexed list_full is added.

Note from the CM50 OSIS convergence work: the **N+1 half of this is apparently tolerated in the canonical pattern** — the worked reference `hero_biz` `backend/.../business_impl.rs` implements `company_list_full` as `list()` then `get()` per item (N+1) on the OSIS `DBTyped` store, and that is the blessed example in home#309. So the actionable/contentious core here is really the **`_set` wholesale-replace dropping derived/array fields**, not the N+1 (which the team accepts for now). Suggest reframing this issue toward the `_set` data-loss semantics; the N+1 is "known/accepted" unless/until a batched or indexed `list_full` is added.