Conversation
`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.
Deploying ontogen with
|
| Latest commit: |
61110eb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://24310ab6.ontogen.pages.dev |
| Branch Preview URL: | https://fix-paginated-list-ordering.ontogen.pages.dev |
|
Correction to finding 2 in the description above. I claimed regenerating The metadata is intact — 26 field keys and 4 I had read the first 20 lines of a diff whose leading change was an indentation shift, which re-diffed the whole file line-by-line; the What is actually there is two real things, both now handled in #179:
Finding 1 in the description stands and is fixed in #179. |
| // 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")); |
There was a problem hiding this comment.
Cross-backend order still diverges for string ids on Postgres/MySQL (collation). The comment and PR say this is "the one the markdown backend already pages by", but the vault sorts by Rust Path/byte order while ORDER BY id on a text PK uses the column collation. On Postgres with the usual en_US.UTF-8 default, ids ["Zeta", "alpha"] sort alpha, Zeta in SQL and Zeta, alpha in the vault. x-2 and x2 also come out differently, because locale collations ignore punctuation at the first level. MySQL utf8mb4_0900_ai_ci is case-insensitive too. Only SQLite (BINARY) agrees. So page 2 still means different rows across backends: the divergence this PR is meant to close. Either order with a byte collation (COLLATE "C" / BINARY) or narrow the claim in this comment to "each backend is internally deterministic".
| // 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() { |
There was a problem hiding this comment.
Markdown order is by file path, not by record id, so nested vaults disagree with this ORDER BY id. walk::list_record_paths recurses (max_depth: None) and sorts by extension-stripped full path, while read_all takes the id from file_stem(). With workouts/b.md and workouts/2024/a.md, the vault lists 2024/a before b only because of the directory name. With workouts/z/a.md and workouts/b.md, the vault gives b, a and SQL gives a, b. The "same key the markdown backend already pages by" claim (here and in the new test comment) holds only for flat directories.
| // 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); |
There was a problem hiding this comment.
The same nondeterminism is left in the other generated SELECT (altitude). populate_{snake}_relations (gen_crud.rs:331-337) emits {target}::Entity::find().filter(Column::{fk}.eq(..)).all(..) with no ORDER BY. So every has_many Vec (e.g. node.contains) comes back in engine-dependent order on SQL, while the markdown backend returns it in vault-sorted order. That is the same "identical generated code means two different things" class this PR fixes for list_*. Consider a single helper that emits .order_by_asc(Column::Id) for every multi-row find() the generator writes, rather than patching only generate_list.
| // 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 { "" }; |
There was a problem hiding this comment.
The id_field().is_some() gate is duplicated and guards a case that cannot compile anyway. The import is gated here and the order_by_asc is gated separately in gen_crud.rs:60, so the two conditions must stay in lockstep or clippy --deny warnings fails. An entity with no #[ontology(id)] field cannot reach working SeaORM output: gen_entity would emit a DeriveEntityModel with no primary_key, and generate_create/populate_relations hardcode {snake}.id / Column::Id / find_by_id. Importing QueryOrder unconditionally and emitting the order unconditionally (as Column::Id, like the rest of the generator) removes the second condition. Alternatively, return a generation error when id_field() is None.
| 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, |
There was a problem hiding this comment.
Reuse: this file already has a copy of to_pascal_case. fk_to_column_enum (gen_crud.rs:395) is the same function as ontogen_core::naming::to_pascal_case (split on _, uppercase each parts first char). Now that to_pascal_case is imported here, the file has two helpers that turn a field name into a Column:: variant. Delete fk_to_column_enum and call to_pascal_case at line 331 so the two column-name derivations cannot drift.
| // 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")); |
There was a problem hiding this comment.
Checked-in example output still ships the bug. examples/iron-log/src-tauri/src/store/generated/{workout,exercise,tag,workout_set}.rs still emit Entity::find() then .limit/.offset with no ordering. The PR intentionally leaves regeneration out, but the one SQL example users copy from still pages nondeterministically. Any examples drift check (see the ci/examples-drift-check work) will also flag it. Worth a tracked follow-up, or at least hand-applying the two-line diff to those four files.
| // 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() { |
There was a problem hiding this comment.
Minor efficiency: the ORDER BY is emitted even when neither limit nor offset is given. An unpaginated list_* call (limit=None, offset=None; the non-paginated transport path) now asks the engine to return the whole table sorted. On a text PK in Postgres that is either a full index scan with heap fetches or a seq scan plus sort, instead of a plain seq scan. If cross-backend parity for full lists is not the goal, emit the order inside the limit.is_some() || offset.is_some() case. If parity is the goal, say so in the comment.
1 participant
Fixes a correctness defect that shipped in 0.7.1 with pagination pushdown.
The bug
src/store/backends/seaorm/gen_crud.rsemitted:LIMIT/OFFSETover a query with noORDER BYhas no defined row order. The engine may answer the same query differently each time — a plan flip to an index-only or parallel scan, a concurrent insert, aVACUUM. So page 2 can repeat a row page 1 already returned, and skip another entirely.It looks fine on a small, quiet table and starts dropping records under exactly the conditions pagination exists for: large tables with live writes.
Why it matters beyond the SQL
The markdown backend never had this problem.
walk::list_record_pathssorts (crates/markdown-store/src/walk.rs:82), andlist_pathsandread_allboth preserve that order — so a vault pages by record id, deterministically.The result was that identical generated code meant two different things by "page 2" depending on which backend sat underneath, with nothing to catch it. A consumer who developed against a markdown vault and deployed against Postgres got different behaviour from the same source. That is the divergence class ontogen exists to prevent.
The fix
Order by the primary key — the one column every entity has, via the existing
EntityDef::id_field()helper, and the same key the markdown backend already pages by:QueryOrderis imported only for entities that have a primary key — an entity without one emits no ordering, and an unused import would fail the--deny warningsclippy gate.What this does and does not guarantee
Cross-backend page order now agrees for string ids, which is what a markdown vault stores. It can still differ for an integer primary key, where SQL orders numerically (
2 < 10) and a vault would order lexicographically ("10" < "2").So the guarantee is: each backend is internally stable and deterministic. Not: a SQL page and a vault page interleave identically for every id type. Forcing the latter would mean
ORDER BY CAST(id AS TEXT), which discards the index. Stating the weaker guarantee honestly seemed better than buying the stronger one at that price.No parity test asserts runtime row order, so this does not conflict with
tests/backend_parity.rs.Tests
New
a_sql_list_orders_before_it_takes_a_pageinsrc/snapshots.rs, mirroring the existingmarkdown_count_walks_the_directory_without_parsing_recordsprecedent: asserts the trait is in scope, that the list orders by primary key, and that the ordering is established before the page is taken. One snapshot updated, diff is two lines.just full-checkgreen: fmt, clippy, the Rust workspace, and 43 vitest tests.Two findings this turned up, deliberately left out
Regenerating
examples/iron-logto keep its checked-in output current swept in unrelated drift, so I reverted it. Both are worth their own PRs:count_*methods andPaginatorTraitthat the pagination work never wrote back.iron-logdegrades itsadmin-registry.ts— thefields:metadata disappears, which is the documented failure mode forClientsConfig::schema_entitiesbeing empty ("silently strips the field metadata... so the admin layer renders blank tables"). Itsbuild.rsbuildsClientsConfigmanually and hands it toPipeline, which is documented to forwardschema.entitiesautomatically. Either that forwarding does not apply to a caller-supplied config, or it regressed. Committing the regeneration would have silently broken that example's admin UI.