diff --git a/src/database.rs b/src/database.rs index ca12221..a831d3a 100644 --- a/src/database.rs +++ b/src/database.rs @@ -372,7 +372,13 @@ impl Database { .foreign_keys(true) .journal_mode(SqliteJournalMode::Wal) .synchronous(SqliteSynchronous::Normal) - .busy_timeout(config.sqlite_busy_timeout); + .busy_timeout(config.sqlite_busy_timeout) + // Numbers in names by value, for listings paged in SQL; see + // `natural_order` for why no schema object may name it. + .collation( + crate::natural_order::COLLATION, + crate::natural_order::compare, + ); let pool = SqlitePoolOptions::new() .max_connections(config.db_max_connections) diff --git a/src/lib.rs b/src/lib.rs index 6e01d1f..e10ea41 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -9,6 +9,7 @@ pub mod database; pub mod enrichment; pub mod lyrics; pub mod media; +pub mod natural_order; pub mod oauth; pub mod outbound; pub mod pid; diff --git a/src/natural_order.rs b/src/natural_order.rs new file mode 100644 index 0000000..9bc5131 --- /dev/null +++ b/src/natural_order.rs @@ -0,0 +1,121 @@ +//! Natural order for names a person reads: runs of digits compare by value, +//! so `2 Song.flac` comes before `10 Song.flac` and `100 Song.flac`. +//! +//! Registered on every pooled connection as the SQLite collation +//! [`COLLATION`], so a listing paged in SQL is ordered in SQL. It is a +//! collation of this binary only: it must never appear in a schema object — +//! an index, a view, a `CHECK` — or the database would no longer open in a +//! tool that lacks it. Queries name it; the schema never does. + +use std::cmp::Ordering; +use std::iter::Peekable; +use std::str::Chars; + +/// The name queries use: `ORDER BY x COLLATE natural_order`. +/// Not `natural`, which SQLite reads as the keyword of `NATURAL JOIN`. +pub const COLLATION: &str = "natural_order"; + +/// Digits by value, everything else case-folded, character by character. +/// +/// A total order, as SQLite requires of a collation: two names that compare +/// equal by those rules — `07` and `7`, `Abc` and `abc` — are then ordered by +/// their bytes, so the only names that compare equal are identical ones. +pub fn compare(a: &str, b: &str) -> Ordering { + natural(a, b).then_with(|| a.cmp(b)) +} + +fn natural(a: &str, b: &str) -> Ordering { + let (mut left, mut right) = (a.chars().peekable(), b.chars().peekable()); + loop { + match (left.peek().copied(), right.peek().copied()) { + (None, None) => return Ordering::Equal, + (None, Some(_)) => return Ordering::Less, + (Some(_), None) => return Ordering::Greater, + (Some(x), Some(y)) if x.is_ascii_digit() && y.is_ascii_digit() => { + let (m, n) = (digits(&mut left), digits(&mut right)); + let (m, n) = (m.trim_start_matches('0'), n.trim_start_matches('0')); + // Without leading zeros, a longer run is a larger number; runs + // of one length compare as their digits do. No parsing, so a + // run longer than any integer type still has its place. + match m.len().cmp(&n.len()).then_with(|| m.cmp(n)) { + Ordering::Equal => {} + unequal => return unequal, + } + } + (Some(x), Some(y)) => { + left.next(); + right.next(); + match x.to_lowercase().cmp(y.to_lowercase()) { + Ordering::Equal => {} + unequal => return unequal, + } + } + } + } +} + +fn digits(chars: &mut Peekable>) -> String { + let mut run = String::new(); + while let Some(digit) = chars.next_if(char::is_ascii_digit) { + run.push(digit); + } + run +} + +#[cfg(test)] +mod tests { + use super::*; + + fn sorted(names: &[&str]) -> Vec { + let mut names: Vec = names.iter().map(|name| (*name).to_owned()).collect(); + names.sort_by(|a, b| compare(a, b)); + names + } + + #[test] + fn numbers_sort_by_value() { + assert_eq!( + sorted(&[ + "100 Snooze.m4a", + "2 Paradise.m4a", + "10 Ballad.m4a", + "1 Morning.mp3" + ]), + [ + "1 Morning.mp3", + "2 Paradise.m4a", + "10 Ballad.m4a", + "100 Snooze.m4a" + ] + ); + assert_eq!( + sorted(&["Disc 10", "Disc 9", "Disc 1"]), + ["Disc 1", "Disc 9", "Disc 10"] + ); + } + + #[test] + fn letters_fold_case_beyond_ascii() { + assert_eq!( + sorted(&["b", "Été", "A", "étoile"]), + ["A", "b", "étoile", "Été"] + ); + assert_eq!(compare("abc", "ABD"), Ordering::Less); + } + + #[test] + fn only_identical_names_compare_equal() { + assert_eq!(compare("7", "7"), Ordering::Equal); + assert_ne!(compare("07", "7"), Ordering::Equal); + assert_ne!(compare("Abc", "abc"), Ordering::Equal); + assert_eq!(compare("07", "7"), compare("07", "7")); + assert_eq!(compare("07", "7"), compare("7", "07").reverse()); + } + + #[test] + fn a_run_longer_than_any_integer_still_sorts() { + let huge = "9".repeat(40); + assert_eq!(compare("1", &huge), Ordering::Less); + assert_eq!(compare(&format!("{huge}0"), &huge), Ordering::Greater); + } +} diff --git a/src/services/library_folders.rs b/src/services/library_folders.rs index b416c13..f4b0891 100644 --- a/src/services/library_folders.rs +++ b/src/services/library_folders.rs @@ -46,7 +46,8 @@ pub struct FiledFolder { /// A library's whole filed tree, as one flat list a client draws from. #[derive(Debug, Clone, Serialize, ToSchema)] pub struct FiledTree { - /// Every folder, siblings by name, case-insensitively. + /// Every folder, siblings by name in natural order: numbers by value, + /// letters case-folded. pub folders: Vec, /// The available tracks filed nowhere: the root's **Unfiled** entry. pub unfiled_track_count: i64, @@ -267,7 +268,7 @@ impl DomainServices { (SELECT COUNT(*) FROM track_placement p JOIN track t ON t.id=p.track_id \ WHERE p.folder_id=f.id AND t.is_available=1) AS track_count \ FROM library_folder f WHERE f.library_id=? \ - ORDER BY f.name_key, f.name, f.id", + ORDER BY f.name COLLATE natural_order, f.id", ) .bind(library_id.to_string()) .fetch_all(self.db.pool()) diff --git a/src/services/library_tree.rs b/src/services/library_tree.rs index 73b7898..2981ecd 100644 --- a/src/services/library_tree.rs +++ b/src/services/library_tree.rs @@ -125,8 +125,9 @@ impl DomainServices { /// member of, and for a folder no available track runs through; the root /// of an empty library is an empty listing. `Invalid` for a malformed path. /// - /// Folders sort by name, case-insensitively. Tracks sort by file name, - /// except in the received folder, whose file names are hashes: there they + /// Folders and tracks sort by name in natural order — numbers by value, + /// letters case-folded (`crate::natural_order`) — so `2 …` comes before + /// `10 …`. Tracks sort by file name, except in the received folder, whose file names are hashes: there they /// sort by artist then title, a missing artist last. pub async fn disk_tree( &self, @@ -160,7 +161,7 @@ impl DomainServices { SUM(instr(rest, '/') > 0 \ AND instr(substr(rest, instr(rest, '/') + 1), '/') = 0) AS direct \ FROM beneath GROUP BY name \ - ORDER BY name IS NOT NULL, name COLLATE NOCASE, name", + ORDER BY name IS NOT NULL, name COLLATE natural_order", ) .bind(tree.rest_from()) .bind(user_id.to_string()) @@ -221,7 +222,7 @@ impl DomainServices { concat!( song_select!(), directly_in!(), - " ORDER BY t.relative_path COLLATE NOCASE, t.relative_path LIMIT ? OFFSET ?" + " ORDER BY t.relative_path COLLATE natural_order LIMIT ? OFFSET ?" ) }; tracks = sqlx::query(sql) diff --git a/tests/native_api.rs b/tests/native_api.rs index 2471b47..30fa1d4 100644 --- a/tests/native_api.rs +++ b/tests/native_api.rs @@ -5115,3 +5115,125 @@ async fn filed_folders_sort_a_library_in_the_database_only() { StatusCode::CREATED ); } + +/// Names with numbers in them sort by value, in SQL, so a page cut there cuts +/// the order a person reads: `2 …` before `10 …` before `100 …`, `Disc 2` +/// before `Disc 10`, in the disk tree and among filed folders alike. +#[tokio::test] +async fn numbered_names_sort_by_value_in_both_trees() { + let (_temp, config, state) = test_app().await; + let password = uuid::Uuid::new_v4().to_string(); + let hash = security::hash_password(&password).unwrap(); + let owner = state + .db + .create_account("natural-owner", &hash, AccountRole::User, now_ms()) + .await + .unwrap(); + let root = std::fs::canonicalize(&config.data_dir).unwrap(); + let library_id = state + .db + .create_library( + owner, + "Natural", + &root, + LibraryVisibility::Private, + now_ms(), + ) + .await + .unwrap(); + let scan_id = state + .db + .create_scan_job(library_id, Some(owner), "manual") + .await + .unwrap(); + let paths = [ + "100 Snooze.m4a", + "2 Paradise.m4a", + "10 Ballad.m4a", + "1 Morning.mp3", + "Disc 10/a.flac", + "Disc 2/a.flac", + "disc 1/a.flac", + ]; + state + .db + .start_scan_job(scan_id, paths.len() as i64, false) + .await + .unwrap(); + for (index, path) in paths.iter().enumerate() { + let mut input = browse_input(40_000 + index, path, "Album", "Artist", None, None); + input.relative_path = (*path).to_owned(); + state + .db + .apply_catalog_track(library_id, scan_id, &input, None, false) + .await + .unwrap(); + } + state.db.finish_scan_job(scan_id, 0).await.unwrap(); + + let router = waveflow_server::app(&config, state.clone()); + let token = login_token(&router, "natural-owner", &password).await; + let get = |uri: String| { + let router = router.clone(); + let token = token.clone(); + async move { + json_body( + router + .oneshot( + Request::get(uri) + .header("authorization", format!("Bearer {token}")) + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(), + ) + .await + } + }; + let names = |listing: &serde_json::Value, key: &str| -> Vec { + listing[key] + .as_array() + .unwrap() + .iter() + .map(|entry| entry["name"].as_str().unwrap().to_owned()) + .collect() + }; + + let top = get(format!("/api/v2/libraries/{library_id}/tree")).await; + assert_eq!(names(&top, "folders"), ["disc 1", "Disc 2", "Disc 10"]); + assert_eq!( + names(&top, "tracks"), + [ + "1 Morning.mp3", + "2 Paradise.m4a", + "10 Ballad.m4a", + "100 Snooze.m4a" + ] + ); + // The page is cut in that order: past the three folders, the second and + // third tracks are 2 and 10, not 10 and 100. + let page = get(format!( + "/api/v2/libraries/{library_id}/tree?offset=4&limit=2" + )) + .await; + assert_eq!(names(&page, "tracks"), ["2 Paradise.m4a", "10 Ballad.m4a"]); + + let folders_uri = format!("/api/v2/libraries/{library_id}/folders"); + for name in ["Vol 10", "Vol 2", "vol 1"] { + let created = router + .clone() + .oneshot( + Request::post(folders_uri.clone()) + .header("authorization", format!("Bearer {token}")) + .header("content-type", "application/json") + .body(Body::from(serde_json::json!({ "name": name }).to_string())) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(created.status(), StatusCode::CREATED); + } + let filed = get(folders_uri).await; + assert_eq!(names(&filed, "folders"), ["vol 1", "Vol 2", "Vol 10"]); +} diff --git a/webapp/src/api/folders.test.ts b/webapp/src/api/folders.test.ts index ea5b25e..836645e 100644 --- a/webapp/src/api/folders.test.ts +++ b/webapp/src/api/folders.test.ts @@ -15,11 +15,12 @@ const folder = (id: string, parent: string | null, name = id): FiledFolder => ({ }); describe("the filed tree, read on the client", () => { + // In the order the server answers: every folder by name, natural order. const tree = [ - folder("rock", null, "Rock"), + folder("ambient", null, "ambient"), folder("indie", "rock", "Indie"), folder("lofi", "indie", "lo-fi"), - folder("ambient", null, "ambient"), + folder("rock", null, "Rock"), ]; it("walks from the root down to a folder", () => { @@ -37,7 +38,7 @@ describe("the filed tree, read on the client", () => { expect(folderOutline(looped)).toEqual([]); }); - it("outlines siblings by name, case aside, each before its children", () => { + it("outlines siblings in the server's order, each before its children", () => { expect( folderOutline(tree).map(({ folder: entry, depth }) => [ entry.name, @@ -51,6 +52,21 @@ describe("the filed tree, read on the client", () => { ]); }); + it("keeps the server's sibling order rather than sorting again", () => { + // `natural_order` puts these as listed; a locale sort would move "Ä 2" + // to the front, and the picker would disagree with the cards. + const answered = [ + folder("a2", null, "a 2"), + folder("a10", null, "A 10"), + folder("umlaut", null, "Ä 2"), + folder("v2", null, "Vol 2"), + folder("v10", null, "Vol 10"), + ]; + expect( + folderOutline(answered).map(({ folder: entry }) => entry.name), + ).toEqual(["a 2", "A 10", "Ä 2", "Vol 2", "Vol 10"]); + }); + it("knows what sits beneath a folder", () => { expect(isWithin(tree, "lofi", "rock")).toBe(true); expect(isWithin(tree, "rock", "rock")).toBe(true); diff --git a/webapp/src/api/folders.ts b/webapp/src/api/folders.ts index 6d753ed..ad6a92c 100644 --- a/webapp/src/api/folders.ts +++ b/webapp/src/api/folders.ts @@ -121,8 +121,14 @@ export function folderTrail( } /** - * The tree flattened in display order — siblings by name, each followed by - * its own sub-folders — with each folder's depth, the root's children at 0. + * The tree flattened in display order — siblings as the server ordered them, + * each followed by its own sub-folders — with each folder's depth, the + * root's children at 0. + * + * **No sort here.** The server orders folders by its `natural_order` + * collation, and the cards beside this list draw them in that order; a second + * sort written in JavaScript would be a second rule, and two rules drift — the + * picker and the cards would disagree on where "Ä 2" goes. */ export function folderOutline( folders: FiledFolder[], @@ -136,10 +142,7 @@ export function folderOutline( const outline: Array<{ folder: FiledFolder; depth: number }> = []; const seen = new Set(); const walk = (parent: string | null, depth: number) => { - const siblings = [...(children.get(parent) ?? [])].sort((a, b) => - a.name.localeCompare(b.name, undefined, { sensitivity: "base" }), - ); - for (const folder of siblings) { + for (const folder of children.get(parent) ?? []) { if (seen.has(folder.id)) continue; seen.add(folder.id); outline.push({ folder, depth });