core: the v9 test pinned v8, and a blank line ktlint counted
Two CI failures from the colour removal, both mine. `a_fresh_database_reaches_v8` asserted the version the migration no longer stops at. Renamed to say what it actually guards — the LATEST version — so the next migration updates a number instead of a name that has quietly become wrong. While there, two tests the migration deserved and did not have. One asks SQLite whether `notes.color` is gone rather than reading a row back, because a SELECT that omits the column passes either way; it also asserts `labels.color` is still there, since getting that wrong would take every tag's colour with it. The other seeds three saved views and checks the sweep: one loses its colour key and keeps its query, one without the key is untouched, and one holding text that is not JSON at all comes out unchanged rather than NULL. Writing that third case is what found a real bug in the migration. The guard was `json_valid(params) AND json_extract(params, '$.color') IS NOT NULL`, which is the obvious way to write it and is a trap: SQLite does not promise to short-circuit AND, so `json_extract` can be evaluated against the very rows `json_valid` was there to exclude — and on malformed input it does not return NULL, it RAISES, which would have aborted the whole migration over one corrupt blob. It is a LIKE now, which is total over any text. The ktlint failure is a doubled blank line where `EditorAction.SetColor`'s branch used to be. #3041 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -354,7 +354,6 @@ class BoardViewModel(
|
||||
is EditorAction.SaveText ->
|
||||
mutate { it.updateNote(id, listOf(NoteEdit.Body(action.body))) }
|
||||
|
||||
|
||||
// Pinning re-sorts the board rather than emptying it, and on a phone
|
||||
// you often pin while still reading — so unlike the three below, it
|
||||
// deliberately leaves the editor open.
|
||||
|
||||
@@ -257,15 +257,21 @@ fn migrate_v8(conn: &Connection) -> rusqlite::Result<()> {
|
||||
// The saved-filter sweep is the second half and not optional. `params` is opaque JSON
|
||||
// and a stored view could carry `"color": "teal"`; with the facet gone that key would
|
||||
// sit there forever, and a view that silently filters on a field the app no longer has
|
||||
// is worse than one that lost a criterion. Guarded on `json_valid` because a corrupt
|
||||
// blob must lose a key, not become NULL.
|
||||
// is worse than one that visibly lost a criterion. Guarded on `json_valid` because a
|
||||
// corrupt blob must keep whatever it holds, not become NULL.
|
||||
//
|
||||
// The second guard is a LIKE and not `json_extract(...) IS NOT NULL`, which is the
|
||||
// obvious way to write it and is a trap: SQLite does not promise to short-circuit AND,
|
||||
// so `json_extract` can be evaluated against the very rows `json_valid` was there to
|
||||
// exclude — and on malformed input it does not return NULL, it RAISES, which would
|
||||
// abort the migration for every other row too. `LIKE` is total over any text.
|
||||
const SCHEMA_V9: &str = r#"
|
||||
ALTER TABLE notes DROP COLUMN color;
|
||||
|
||||
UPDATE saved_filters
|
||||
SET params = json_remove(params, '$.color')
|
||||
WHERE json_valid(params)
|
||||
AND json_extract(params, '$.color') IS NOT NULL;
|
||||
AND params LIKE '%"color"%';
|
||||
"#;
|
||||
|
||||
pub fn migrate(conn: &Connection) -> rusqlite::Result<()> {
|
||||
@@ -415,12 +421,79 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_fresh_database_reaches_v8() {
|
||||
fn a_fresh_database_reaches_the_latest_version() {
|
||||
let conn = Connection::open_in_memory().expect("open");
|
||||
migrate(&conn).expect("migrate");
|
||||
let version: i64 = conn
|
||||
.query_row("PRAGMA user_version", [], |r| r.get(0))
|
||||
.expect("version");
|
||||
assert_eq!(version, 8);
|
||||
assert_eq!(version, 9);
|
||||
}
|
||||
|
||||
/// The column is gone, not merely unread. Asserted by asking SQLite rather than by
|
||||
/// reading a row: a SELECT that omits `color` would pass either way.
|
||||
#[test]
|
||||
fn v9_drops_the_note_colour_column() {
|
||||
let conn = Connection::open_in_memory().expect("open");
|
||||
migrate(&conn).expect("migrate");
|
||||
let mut stmt = conn.prepare("PRAGMA table_info(notes)").expect("pragma");
|
||||
let columns: Vec<String> = stmt
|
||||
.query_map([], |r| r.get::<_, String>(1))
|
||||
.expect("query")
|
||||
.collect::<rusqlite::Result<Vec<String>>>()
|
||||
.expect("collect");
|
||||
assert!(!columns.iter().any(|c| c == "color"));
|
||||
// The one that survived. Getting this wrong would take every tag's colour with
|
||||
// it, which is the whole thing M315 was keeping.
|
||||
let mut stmt = conn.prepare("PRAGMA table_info(labels)").expect("pragma");
|
||||
let label_columns: Vec<String> = stmt
|
||||
.query_map([], |r| r.get::<_, String>(1))
|
||||
.expect("query")
|
||||
.collect::<rusqlite::Result<Vec<String>>>()
|
||||
.expect("collect");
|
||||
assert!(label_columns.iter().any(|c| c == "color"));
|
||||
}
|
||||
|
||||
/// A stored view that filtered on colour loses that criterion and keeps the rest.
|
||||
/// The alternative — leaving the key — is a lens that silently narrows on a field
|
||||
/// the app no longer has and never says why it returned nothing.
|
||||
#[test]
|
||||
fn v9_sweeps_colour_out_of_saved_filters() {
|
||||
let conn = Connection::open_in_memory().expect("open");
|
||||
conn.execute_batch("PRAGMA foreign_keys = ON;").expect("fk");
|
||||
for batch in [
|
||||
SCHEMA_V1, SCHEMA_V2, SCHEMA_V3, SCHEMA_V4, SCHEMA_V5, SCHEMA_V6, SCHEMA_V7,
|
||||
] {
|
||||
conn.execute_batch(batch).expect("schema");
|
||||
}
|
||||
conn.execute_batch("PRAGMA user_version = 8;").expect("v8");
|
||||
for (id, params) in [
|
||||
("a", r#"{"color":"teal","q":"milk"}"#),
|
||||
("b", r#"{"q":"eggs"}"#),
|
||||
// Not JSON at all. It must come out UNCHANGED rather than NULL — a blob
|
||||
// this migration cannot read is not a blob it gets to destroy.
|
||||
("c", "not json"),
|
||||
] {
|
||||
conn.execute(
|
||||
"INSERT INTO saved_filters (id, name, params, created_at)
|
||||
VALUES (?1, ?1, ?2, '2026-08-28T00:00:00.000Z')",
|
||||
params![id, params],
|
||||
)
|
||||
.expect("seed");
|
||||
}
|
||||
|
||||
migrate(&conn).expect("migrate");
|
||||
|
||||
let read = |id: &str| -> String {
|
||||
conn.query_row(
|
||||
"SELECT params FROM saved_filters WHERE id = ?1",
|
||||
[id],
|
||||
|r| r.get(0),
|
||||
)
|
||||
.expect("read")
|
||||
};
|
||||
assert_eq!(read("a"), r#"{"q":"milk"}"#);
|
||||
assert_eq!(read("b"), r#"{"q":"eggs"}"#);
|
||||
assert_eq!(read("c"), "not json");
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user