diff --git a/src/snapshots.rs b/src/snapshots.rs index 4b1cf973..49eda370 100644 --- a/src/snapshots.rs +++ b/src/snapshots.rs @@ -198,6 +198,62 @@ 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. + // + // 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}"); + + 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}"); +} + +#[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/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..178b1c9e 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 ────────────────────────────────────────────────────────────── @@ -44,11 +46,26 @@ 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" )); 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. + // + // 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"); @@ -314,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"); @@ -378,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 c8dbaf6c..5f7123ae 100644 --- a/src/store/backends/seaorm/mod.rs +++ b/src/store/backends/seaorm/mod.rs @@ -15,7 +15,11 @@ 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` 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() {