From 61110eb45cf3329ed650cbe538806dc6893d311c Mon Sep 17 00:00:00 2001 From: sksizer Date: Fri, 25 Sep 2026 19:03:56 -0500 Subject: [PATCH 1/2] fix(store): a paginated SQL list orders before it takes its page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `LIMIT`/`OFFSET` over a query with no `ORDER BY` has no defined row order. The engine is free to answer the same query differently each time, so page 2 can repeat a row page 1 already returned and skip another entirely. This shipped in 0.7.1 as part of pagination pushdown. The markdown backend never had the problem: `walk::list_record_paths` sorts, and `list_paths` and `read_all` preserve that order, so a vault pages by record id. The result was that identical generated code meant two different things by "page 2" depending on which backend was underneath — the divergence class ontogen exists to prevent. Generated SQL lists now `order_by_asc` the primary key, which is the column the markdown backend already pages by. `QueryOrder` is imported only for entities that have a primary key, since an unused import would fail the `--deny warnings` clippy gate. Cross-backend page order now agrees for string ids, which is what the markdown vault stores. It can still differ for an integer primary key, where SQL orders numerically and a vault would order lexicographically; the guarantee this makes is that each backend is internally stable and deterministic, not that a SQL page and a vault page interleave the same way for every id type. --- src/snapshots.rs | 20 +++++++++++++++++++ ..._snapshots__store_crud_complex_entity.snap | 3 ++- src/store/backends/seaorm/gen_crud.rs | 14 ++++++++++++- src/store/backends/seaorm/mod.rs | 9 ++++++++- 4 files changed, 43 insertions(+), 3 deletions(-) diff --git a/src/snapshots.rs b/src/snapshots.rs index 4b1cf973..17a169fb 100644 --- a/src/snapshots.rs +++ b/src/snapshots.rs @@ -198,6 +198,26 @@ fn store_crud_complex_entity() { insta::assert_snapshot!(code); } +#[test] +fn a_sql_list_orders_before_it_takes_a_page() { + // `LIMIT`/`OFFSET` over no `ORDER BY` has no defined row order — the engine + // may answer the same query differently each time, so page 2 can repeat a + // row page 1 already returned and skip another entirely. The markdown + // backend never had this problem, because its vault listing is sorted by + // record id, which meant identical generated code meant two different + // things by "page 2" depending on the backend underneath it. + let code = generate_store_file(&article_mtm_tags_entity()); + assert!(code.contains("QueryOrder"), "the ordering trait is in scope:\n{code}"); + + let body = &code[code.find("pub async fn list_").expect("a list method")..]; + let body = &body[..body.find("\n }").expect("the list method's closing brace")]; + assert!(body.contains(".order_by_asc(article::Column::Id)"), "the list orders by primary key:\n{body}"); + + let order_at = body.find(".order_by_asc(").expect("an order_by"); + let limit_at = body.find(".limit(").expect("a limit"); + assert!(order_at < limit_at, "the order is established before the page is taken:\n{body}"); +} + /// Self-referential `has_many` entity (the in-tree set_parent shape): /// `Node { id, label, parent_id -> Node, contains: has_many(parent_id), body }`. fn node_has_many_entity() -> EntityDef { diff --git a/src/snapshots/ontogen__snapshots__store_crud_complex_entity.snap b/src/snapshots/ontogen__snapshots__store_crud_complex_entity.snap index 2881e643..a64812ac 100644 --- a/src/snapshots/ontogen__snapshots__store_crud_complex_entity.snap +++ b/src/snapshots/ontogen__snapshots__store_crud_complex_entity.snap @@ -4,7 +4,7 @@ expression: code --- //! Generated by ontogen. DO NOT EDIT. -use sea_orm::{ActiveModelTrait, EntityTrait, PaginatorTrait, QuerySelect}; +use sea_orm::{ActiveModelTrait, EntityTrait, PaginatorTrait, QueryOrder, QuerySelect}; use crate::persistence::db::entities::article; use crate::schema::Article; @@ -50,6 +50,7 @@ impl From for Article { impl Store { pub async fn list_articles(&self, limit: Option, offset: Option) -> Result, AppError> { let mut query = article::Entity::find(); + query = query.order_by_asc(article::Column::Id); if let Some(l) = limit { query = query.limit(l); } diff --git a/src/store/backends/seaorm/gen_crud.rs b/src/store/backends/seaorm/gen_crud.rs index 5766cd75..65ffbc96 100644 --- a/src/store/backends/seaorm/gen_crud.rs +++ b/src/store/backends/seaorm/gen_crud.rs @@ -7,7 +7,9 @@ //! - Both tiers emit events via `self.emit_change()` use crate::schema::model::EntityDef; -use crate::store::helpers::{junction_source_col, junction_table_name, junction_target_col, pluralize, to_snake_case}; +use crate::store::helpers::{ + junction_source_col, junction_table_name, junction_target_col, pluralize, to_pascal_case, to_snake_case, +}; // ─── Public API ────────────────────────────────────────────────────────────── @@ -49,6 +51,16 @@ fn generate_list(code: &mut String, entity: &EntityDef, has_relations: bool) { " pub async fn list_{plural}(&self, limit: Option, offset: Option) -> Result, AppError> {{\n" )); code.push_str(&format!(" let mut query = {snake}::Entity::find();\n")); + // A page only means something over a defined order. `LIMIT`/`OFFSET` with + // no `ORDER BY` lets the engine return rows in whatever order it likes, so + // the same offset can repeat a row the previous page already returned and + // skip another entirely. Order by the primary key — the one column every + // entity has, and the one the markdown backend already pages by, since its + // vault listing is sorted by record id. + if let Some(id) = entity.id_field() { + let col = to_pascal_case(&id.name); + code.push_str(&format!(" query = query.order_by_asc({snake}::Column::{col});\n")); + } code.push_str(" if let Some(l) = limit {\n"); code.push_str(" query = query.limit(l);\n"); code.push_str(" }\n"); diff --git a/src/store/backends/seaorm/mod.rs b/src/store/backends/seaorm/mod.rs index c8dbaf6c..2906a844 100644 --- a/src/store/backends/seaorm/mod.rs +++ b/src/store/backends/seaorm/mod.rs @@ -15,7 +15,14 @@ impl StoreBackend for SeaormBackend { fn emit_preamble(&self, code: &mut String, entity: &EntityDef) { let snake = to_snake_case(&entity.name); - code.push_str("use sea_orm::{ActiveModelTrait, EntityTrait, PaginatorTrait, QuerySelect};\n\n"); + // `QueryOrder` backs the `order_by_asc` a list emits to make its page + // deterministic. An entity with no primary key emits no ordering, and an + // unused import would fail the `--deny warnings` clippy gate, so it is + // imported only where it is used. + let order_import = if entity.id_field().is_some() { ", QueryOrder" } else { "" }; + code.push_str(&format!( + "use sea_orm::{{ActiveModelTrait, EntityTrait, PaginatorTrait{order_import}, QuerySelect}};\n\n" + )); // Additional imports for entities with has_many relations if entity.has_many_relations().next().is_some() { From 056a04a17027b939e32e485375777f692af26938 Mon Sep 17 00:00:00 2001 From: sksizer Date: Thu, 1 Oct 2026 23:10:22 -0500 Subject: [PATCH 2/2] fix(store): order every generated multi-row SELECT, and stop overclaiming MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the review on #178. The cross-backend claim was wrong twice over, so it is gone from the comment, the test and the PR text. `ORDER BY` on a text column uses that column's collation: Postgres under `en_US.UTF-8` sorts `["Zeta", "alpha"]` as `alpha, Zeta` where a vault sorts byte-wise, and MySQL's default collation is case-insensitive besides. The vault also sorts by record *path*, not id, so a nested vault disagrees whatever the collation does. What this buys is determinism within a backend, which is worth having on its own; it is not parity between them. `populate_*_relations` had the identical defect one altitude down: a `has_many` field is loaded by its own `find().filter(..)` with no ordering, so it came back reshuffled between calls while the markdown backend returned it vault-sorted. Now ordered too, which makes the rule uniform — every multi-row SELECT this generator writes is ordered, and a test asserts it rather than asserting one method. Two cleanups the review asked for: - `fk_to_column_enum` was `to_pascal_case` copied, so two helpers derived `Column::` names and could drift. Deleted; replaced by `id_column`, which reads the entity's id field so a renamed id stays correct and falls back to `Id`. - The ordering and its `QueryOrder` import were gated separately on `id_field().is_some()`, so the two conditions had to stay in lockstep or clippy's `--deny warnings` would fail. Both unconditional now: `list_*` is always generated and always ordered, so the import is always used. The new test counts what it checked, so it cannot pass by never running, and bounds each chain at its own terminator — without that it finds a later query's `order_by_asc` and passes for the wrong reason. --- src/snapshots.rs | 44 ++++++++++++++++++++-- src/store/backends/seaorm/gen_crud.rs | 54 +++++++++++++++------------ src/store/backends/seaorm/mod.rs | 13 +++---- 3 files changed, 75 insertions(+), 36 deletions(-) diff --git a/src/snapshots.rs b/src/snapshots.rs index 17a169fb..49eda370 100644 --- a/src/snapshots.rs +++ b/src/snapshots.rs @@ -202,10 +202,11 @@ fn store_crud_complex_entity() { fn a_sql_list_orders_before_it_takes_a_page() { // `LIMIT`/`OFFSET` over no `ORDER BY` has no defined row order — the engine // may answer the same query differently each time, so page 2 can repeat a - // row page 1 already returned and skip another entirely. The markdown - // backend never had this problem, because its vault listing is sorted by - // record id, which meant identical generated code meant two different - // things by "page 2" depending on the backend underneath it. + // row page 1 already returned and skip another entirely. + // + // This pins determinism within the SQL backend only. It deliberately does + // not claim parity with a markdown page: collation decides text order in + // SQL, and the vault sorts by record path rather than id. let code = generate_store_file(&article_mtm_tags_entity()); assert!(code.contains("QueryOrder"), "the ordering trait is in scope:\n{code}"); @@ -218,6 +219,41 @@ fn a_sql_list_orders_before_it_takes_a_page() { assert!(order_at < limit_at, "the order is established before the page is taken:\n{body}"); } +#[test] +fn every_generated_sql_multi_row_select_is_ordered() { + // A `has_many` field is loaded by its own `find().filter(..)`, which is as + // order-free as the list was. Leaving it unordered reshuffles the field + // between calls, and the markdown backend returns it vault-sorted, so it + // is the same "identical code, two behaviours" split — just one altitude + // down. The count is exempt: a row count does not depend on row order. + let code = generate_store_file(&node_has_many_entity()); + + let mut checked = 0; + for (i, _) in code.match_indices("Entity::find()") { + let after = &code[i..]; + let all_at = after.find(".all("); + let count_at = after.find(".count("); + // A count is exempt — a row count does not depend on row order. The + // chain is a count when `.count(` closes it before any `.all(` does; + // bounding the search at the terminator matters, or a later query's + // `order_by_asc` is found and the assert passes for the wrong reason. + let is_count = match (all_at, count_at) { + (Some(a), Some(c)) => c < a, + (None, Some(_)) => true, + _ => false, + }; + if is_count { + continue; + } + let stmt = &after[..all_at.expect("a multi-row chain terminates in .all(")]; + assert!(stmt.contains(".order_by_asc("), "this SELECT returns rows with no ordering:\n{stmt}"); + checked += 1; + } + // Node has a `has_many` field, so there is the list and the relation load. + // Without this the loop could pass by never running. + assert_eq!(checked, 2, "expected to check the list and the has_many load"); +} + /// Self-referential `has_many` entity (the in-tree set_parent shape): /// `Node { id, label, parent_id -> Node, contains: has_many(parent_id), body }`. fn node_has_many_entity() -> EntityDef { diff --git a/src/store/backends/seaorm/gen_crud.rs b/src/store/backends/seaorm/gen_crud.rs index 65ffbc96..178b1c9e 100644 --- a/src/store/backends/seaorm/gen_crud.rs +++ b/src/store/backends/seaorm/gen_crud.rs @@ -46,6 +46,7 @@ fn generate_list(code: &mut String, entity: &EntityDef, has_relations: bool) { let name = &entity.name; let snake = to_snake_case(name); let plural = pluralize(&snake); + let id_col = id_column(entity); code.push_str(&format!( " pub async fn list_{plural}(&self, limit: Option, offset: Option) -> Result, AppError> {{\n" @@ -54,13 +55,17 @@ fn generate_list(code: &mut String, entity: &EntityDef, has_relations: bool) { // A page only means something over a defined order. `LIMIT`/`OFFSET` with // no `ORDER BY` lets the engine return rows in whatever order it likes, so // the same offset can repeat a row the previous page already returned and - // skip another entirely. Order by the primary key — the one column every - // entity has, and the one the markdown backend already pages by, since its - // vault listing is sorted by record id. - if let Some(id) = entity.id_field() { - let col = to_pascal_case(&id.name); - code.push_str(&format!(" query = query.order_by_asc({snake}::Column::{col});\n")); - } + // skip another entirely. + // + // This buys determinism *within* this backend. It does not make a SQL page + // and a markdown page interleave identically: `ORDER BY` on a text column + // uses that column's collation, so Postgres under `en_US.UTF-8` sorts + // `["Zeta", "alpha"]` as `alpha, Zeta` where a vault sorts byte-wise, and + // MySQL's default collation is case-insensitive besides. The vault also + // sorts by record *path*, not id, so a nested vault disagrees whatever the + // collation does. Each backend is internally stable; they are not each + // other's mirror. + code.push_str(&format!(" query = query.order_by_asc({snake}::Column::{id_col});\n")); code.push_str(" if let Some(l) = limit {\n"); code.push_str(" query = query.limit(l);\n"); code.push_str(" }\n"); @@ -326,12 +331,18 @@ fn generate_populate_relations(code: &mut String, entity: &EntityDef) { for (field, info) in entity.has_many_relations() { if let Some(ref fk) = info.foreign_key { let target_snake = to_snake_case(&info.target); - let fk_col = fk_to_column_enum(fk); + let fk_col = to_pascal_case(fk); code.push_str(&format!(" {snake}.{fname} = {{\n", fname = field.name,)); code.push_str(&format!(" use crate::persistence::db::entities::{target_snake};\n")); code.push_str(&format!(" let children = {target_snake}::Entity::find()\n")); code.push_str(&format!(" .filter({target_snake}::Column::{fk_col}.eq(&{snake}.id))\n")); + // Same reason the list orders: this is a multi-row SELECT, so + // without an `ORDER BY` the engine picks the order and a + // `has_many` field comes back shuffled between calls. The markdown + // backend returns these vault-sorted, so leaving it unordered is + // the same "identical code, two behaviours" split the list had. + code.push_str(&format!(" .order_by_asc({target_snake}::Column::Id)\n")); code.push_str(" .all(self.db())\n"); code.push_str(" .await\n"); code.push_str(" .map_err(|e| crate::schema::AppError::DbError(e.to_string()))?;\n"); @@ -390,22 +401,17 @@ fn generate_set_parent_helper(code: &mut String, entity: &EntityDef, fk: &str) { // ─── Helpers ───────────────────────────────────────────────────────────────── -/// Convert a snake_case foreign key name to its SeaORM Column enum variant. -/// E.g., `parent_id` → `ParentId`. -fn fk_to_column_enum(fk: &str) -> String { - fk.split('_') - .map(|part| { - let mut chars = part.chars(); - match chars.next() { - Some(c) => { - let mut s = c.to_uppercase().collect::(); - s.push_str(chars.as_str()); - s - } - None => String::new(), - } - }) - .collect() +/// The SeaORM `Column` variant for an entity's primary key, e.g. `Id`. +/// +/// Every other method this generator writes hardcodes `.id`, `find_by_id` or +/// `Column::Id`, so an entity with no `#[ontology(id)]` field already cannot +/// produce compiling SeaORM output — `gen_entity` would emit a +/// `DeriveEntityModel` with no `primary_key`. Reading the field keeps a +/// renamed id correct; falling back to `Id` keeps the emitted ordering +/// unconditional, so the ordering and the `QueryOrder` import it needs cannot +/// drift out of lockstep. +pub(crate) fn id_column(entity: &EntityDef) -> String { + entity.id_field().map(|f| to_pascal_case(&f.name)).unwrap_or_else(|| "Id".to_string()) } /// Map entity name to its `AppError::*NotFound` variant. diff --git a/src/store/backends/seaorm/mod.rs b/src/store/backends/seaorm/mod.rs index 2906a844..5f7123ae 100644 --- a/src/store/backends/seaorm/mod.rs +++ b/src/store/backends/seaorm/mod.rs @@ -15,14 +15,11 @@ impl StoreBackend for SeaormBackend { fn emit_preamble(&self, code: &mut String, entity: &EntityDef) { let snake = to_snake_case(&entity.name); - // `QueryOrder` backs the `order_by_asc` a list emits to make its page - // deterministic. An entity with no primary key emits no ordering, and an - // unused import would fail the `--deny warnings` clippy gate, so it is - // imported only where it is used. - let order_import = if entity.id_field().is_some() { ", QueryOrder" } else { "" }; - code.push_str(&format!( - "use sea_orm::{{ActiveModelTrait, EntityTrait, PaginatorTrait{order_import}, QuerySelect}};\n\n" - )); + // `QueryOrder` backs the `order_by_asc` every multi-row SELECT emits. + // `list_*` is always generated and always ordered (see `id_column`), so + // the import is always used — no condition here to keep in lockstep + // with the one in gen_crud. + code.push_str("use sea_orm::{ActiveModelTrait, EntityTrait, PaginatorTrait, QueryOrder, QuerySelect};\n\n"); // Additional imports for entities with has_many relations if entity.has_many_relations().next().is_some() {