diff --git a/alembic/versions/0031_previews_bump_note_revision.py b/alembic/versions/0031_previews_bump_note_revision.py new file mode 100644 index 0000000..62710f4 --- /dev/null +++ b/alembic/versions/0031_previews_bump_note_revision.py @@ -0,0 +1,38 @@ +"""a link preview's insert, update or delete bumps its note's sync revision + +Revision ID: 0031 +Revises: 0030 +Create Date: 2026-10-07 + +0015 made every child table bump its parent note's `sync_revision`, so a note syncs +as a whole. `note_link_previews` arrived later (0020) and was never added, and two +things have been missing on every linked device since: + +- A preview fetched in the background after a save (`unfurl_queue.py`) never reached + a device that had already pulled the note. The note's revision was assigned when + the TEXT was saved, before the preview existed, and nothing moved it afterwards. +- A preview dismissed on the web stayed on every other device, for the same reason. + +The trigger is the same function 0015 installs on the other child tables. + +## Downgrade + +Drops the trigger. Previews go back to not propagating; nothing is lost. +""" +from alembic import op + +revision = "0031" +down_revision = "0030" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.execute( + "CREATE TRIGGER trg_note_link_previews_bump_note AFTER INSERT OR UPDATE OR DELETE " + "ON note_link_previews FOR EACH ROW EXECUTE PROCEDURE ts_bump_parent_note_revision()" + ) + + +def downgrade() -> None: + op.execute("DROP TRIGGER IF EXISTS trg_note_link_previews_bump_note ON note_link_previews") diff --git a/android/ffi/src/models.rs b/android/ffi/src/models.rs index 6e07ff5..9967788 100644 --- a/android/ffi/src/models.rs +++ b/android/ffi/src/models.rs @@ -167,6 +167,8 @@ pub struct Attachment { pub mime: String, pub size: Option, pub sha256: Option, + /// Why the server refused a file attached on this device. None otherwise. + pub upload_error: Option, } #[derive(Debug, Clone, uniffi::Record)] @@ -264,6 +266,7 @@ impl From for Attachment { mime, size, sha256, + upload_error, } = value; Attachment { id, @@ -272,6 +275,7 @@ impl From for Attachment { mime, size, sha256, + upload_error, } } } @@ -637,6 +641,10 @@ pub struct PushSummary { /// realistic case). Silently retrying forever would be the wrong shape. pub rejected: u64, pub errors: Vec, + /// Files attached on this device that reached the server this cycle. + pub uploaded: u64, + /// Files that didn't; their reasons are in `errors`. + pub upload_failed: u64, } #[derive(Debug, Clone, uniffi::Record)] @@ -668,6 +676,8 @@ impl From for PushSummary { noop, rejected, errors, + uploaded, + upload_failed, } = value; PushSummary { batches: batches as u64, @@ -678,6 +688,8 @@ impl From for PushSummary { noop: noop as u64, rejected: rejected as u64, errors, + uploaded: uploaded as u64, + upload_failed: upload_failed as u64, } } } diff --git a/core/src/local/models.rs b/core/src/local/models.rs index 97234b6..29920c2 100644 --- a/core/src/local/models.rs +++ b/core/src/local/models.rs @@ -55,6 +55,10 @@ pub struct Attachment { pub mime: String, pub size: Option, pub sha256: Option, + /// Why the server refused a file attached on this device, in words to show on it. + /// Absent for everything else, including a file still waiting to upload. + #[serde(skip_serializing_if = "Option::is_none")] + pub upload_error: Option, } #[derive(Serialize)] diff --git a/core/src/local/schema.rs b/core/src/local/schema.rs index 7cd9867..221fc40 100644 --- a/core/src/local/schema.rs +++ b/core/src/local/schema.rs @@ -274,6 +274,23 @@ UPDATE saved_filters AND params LIKE '%"color"%'; "#; +// v10 (#5168): an attachment can be created on THIS device. +// +// Until now every attachment row arrived on the delta feed, so every one was already on +// the server. A file attached here, offline, is not, and `uploaded = 0` is the queue +// push drains once the note has landed. The rows already here all came from the +// server, which is why the default is 1. +// +// `upload_error` holds a refusal that retrying won't fix — over the size limit, an id +// already in use, bytes that don't match their hash. A row carrying one leaves the +// queue: a file refused for its size would otherwise be re-sent in full on every +// cycle, every five minutes, for as long as the app was open. The message is what the +// editor shows on the file instead. +const SCHEMA_V10: &str = r#" +ALTER TABLE attachments ADD COLUMN uploaded INTEGER NOT NULL DEFAULT 1; +ALTER TABLE attachments ADD COLUMN upload_error TEXT; +"#; + pub fn migrate(conn: &Connection) -> rusqlite::Result<()> { conn.execute_batch("PRAGMA foreign_keys = ON;")?; let version: i64 = conn.query_row("PRAGMA user_version", [], |r| r.get(0))?; @@ -313,6 +330,10 @@ pub fn migrate(conn: &Connection) -> rusqlite::Result<()> { conn.execute_batch(SCHEMA_V9)?; conn.execute_batch("PRAGMA user_version = 9;")?; } + if version < 10 { + conn.execute_batch(SCHEMA_V10)?; + conn.execute_batch("PRAGMA user_version = 10;")?; + } Ok(()) } @@ -427,7 +448,34 @@ mod tests { let version: i64 = conn .query_row("PRAGMA user_version", [], |r| r.get(0)) .expect("version"); - assert_eq!(version, 9); + assert_eq!(version, 10); + } + + /// Every attachment that predates v10 came down the feed, so it is already on the + /// server. Defaulting it to "waiting to upload" would re-send every file once. + #[test] + fn v10_counts_existing_attachments_as_already_on_the_server() { + let conn = v7_db(); + migrate_v8(&conn).expect("v8"); + conn.execute_batch(SCHEMA_V9).expect("v9"); + conn.execute_batch("PRAGMA user_version = 9;").expect("v9"); + add_note(&conn, "n", "a note"); + conn.execute( + "INSERT INTO attachments (id, note_id, url) VALUES ('a', 'n', '/x')", + [], + ) + .expect("seed"); + + migrate(&conn).expect("migrate"); + let (uploaded, error): (bool, Option) = conn + .query_row( + "SELECT uploaded, upload_error FROM attachments WHERE id = 'a'", + [], + |r| Ok((r.get(0)?, r.get(1)?)), + ) + .expect("row"); + assert!(uploaded); + assert_eq!(error, None); } /// The column is gone, not merely unread. Asserted by asking SQLite rather than by diff --git a/core/src/local/store.rs b/core/src/local/store.rs index 075db4d..55891a9 100644 --- a/core/src/local/store.rs +++ b/core/src/local/store.rs @@ -92,7 +92,7 @@ fn items_of(body: &str) -> Vec { fn load_attachments(conn: &Connection, note_id: &str) -> rusqlite::Result> { let mut stmt = conn.prepare( - "SELECT id, url, filename, mime, size, sha256 FROM attachments WHERE note_id = ?1 ORDER BY position ASC", + "SELECT id, url, filename, mime, size, sha256, upload_error FROM attachments WHERE note_id = ?1 ORDER BY position ASC", )?; let rows = stmt.query_map([note_id], |r| { let server_url: String = r.get(1)?; @@ -117,6 +117,7 @@ fn load_attachments(conn: &Connection, note_id: &str) -> rusqlite::Result rusqlite::Resu set_body(conn, id, derive::remove_item(&body, index)) } +/// The stored form of a declared content type: the bare media type, lowercase. +/// Matches the server's `normalize_mime`, so both sides file a type the same way. +fn normalize_mime(raw: &str) -> String { + let bare = raw + .split(';') + .next() + .unwrap_or("") + .trim() + .to_ascii_lowercase(); + if bare.is_empty() { + "application/octet-stream".to_string() + } else { + bare + } +} + +/// A file's name reduced to its last path component, as the server's +/// `_safe_filename` does — the name is for display and download, never a path. +fn safe_filename(raw: &str) -> String { + let base = raw.trim().replace('\\', "/"); + let base = base.rsplit('/').next().unwrap_or("").trim(); + let capped: String = base.chars().take(255).collect(); + if capped.is_empty() { + "file".to_string() + } else { + capped + } +} + +/// Attach a file on this device, linked or not. +/// +/// The bytes go into the blob store under their hash, and the row waits with +/// `uploaded = 0` until push sends it — after the note itself has landed, since the +/// server files an attachment under its note. The note is touched, so it is dirty +/// too: that is what a background sync keys on to send it promptly. +pub fn add_attachment( + conn: &Connection, + blobs: &crate::sync::blobs::BlobStore, + note_id: &str, + filename: &str, + mime: &str, + bytes: &[u8], +) -> Result { + let exists: Option = conn + .query_row("SELECT 1 FROM notes WHERE id = ?1", [note_id], |r| r.get(0)) + .optional() + .map_err(|e| e.to_string())?; + if exists.is_none() { + return Err("That note no longer exists.".to_string()); + } + let sha256 = blobs.put(bytes)?; + let id = new_id(); + conn.execute( + "INSERT INTO attachments (id, note_id, url, filename, mime, size, sha256, position, uploaded) + VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, + (SELECT COALESCE(MAX(position) + 1, 0) FROM attachments WHERE note_id = ?2), + 0)", + params![ + id, + note_id, + // The server's route for it, which is what the row holds once it has + // synced too. Nothing renders it: `load_attachments` serves the local bytes. + format!("/api/notes/{note_id}/attachments/{id}"), + safe_filename(filename), + normalize_mime(mime), + bytes.len() as i64, + sha256, + ], + ) + .map_err(|e| e.to_string())?; + touch(conn, note_id).map_err(|e| e.to_string())?; + load_note(conn, note_id).map_err(|e| e.to_string()) +} + pub fn delete_attachment(conn: &Connection, id: &str, att_id: &str) -> rusqlite::Result { + let uploaded: Option = conn + .query_row( + "SELECT uploaded FROM attachments WHERE id = ?1 AND note_id = ?2", + params![att_id, id], + |r| r.get(0), + ) + .optional()?; conn.execute( "DELETE FROM attachments WHERE id = ?1 AND note_id = ?2", params![att_id, id], )?; + // Only a file the server holds needs a tombstone; without one the next pull would + // bring it straight back. One still waiting to upload leaves the queue with its row. + if uploaded == Some(true) { + record_pending_delete(conn, "attachment", att_id)?; + } touch(conn, id)?; load_note(conn, id) } pub fn delete_preview(conn: &Connection, id: &str, preview_id: &str) -> rusqlite::Result { - conn.execute( + let removed = conn.execute( "DELETE FROM link_previews WHERE id = ?1 AND note_id = ?2", params![preview_id, id], )?; + // Previews are made by the server, so every one it has is one it would send back. + if removed > 0 { + record_pending_delete(conn, "preview", preview_id)?; + } touch(conn, id)?; load_note(conn, id) } @@ -1460,4 +1551,126 @@ mod tests { trash(&conn, &binned.id).expect("trash"); assert_eq!(list_labels(&conn).expect("labels")[0].count, Some(1)); } + + fn blobs(tag: &str) -> crate::sync::blobs::BlobStore { + let dir = std::env::temp_dir().join(format!("ts-store-blobs-{}-{tag}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + crate::sync::blobs::BlobStore::new(dir).expect("blobs") + } + + fn tombstones(conn: &Connection) -> Vec<(String, String)> { + let mut stmt = conn + .prepare("SELECT entity, id FROM pending_deletes ORDER BY entity, id") + .expect("prepare"); + let rows = stmt + .query_map([], |r| Ok((r.get(0)?, r.get(1)?))) + .expect("query"); + rows.collect::>().expect("rows") + } + + #[test] + fn an_attached_file_is_kept_here_and_queued_to_upload() { + let conn = db(); + let blobs = blobs("attach"); + let n = note(&conn, "with a receipt"); + conn.execute("UPDATE notes SET dirty = 0", []) + .expect("clean"); + + let got = add_attachment( + &conn, + &blobs, + &n.id, + "C:\\scans\\receipt.PDF", + "Application/PDF; name=x", + b"%PDF", + ) + .expect("attach"); + + let att = &got.attachments[0]; + assert_eq!( + att.filename.as_deref(), + Some("receipt.PDF"), + "a name, not a path" + ); + assert_eq!(att.mime, "application/pdf"); + assert_eq!(att.size, Some(4)); + let hash = att.sha256.clone().expect("hashed"); + assert!(blobs.has(&hash), "the bytes are on this device"); + assert!(att.url.contains(&hash), "and served from here: {}", att.url); + assert_eq!(att.upload_error, None); + let (uploaded, dirty): (bool, bool) = conn + .query_row( + "SELECT a.uploaded, n.dirty FROM attachments a JOIN notes n ON n.id = a.note_id", + [], + |r| Ok((r.get(0)?, r.get(1)?)), + ) + .expect("row"); + assert!(!uploaded, "it waits for push"); + assert!( + dirty, + "the note is touched, which is what starts a background sync" + ); + } + + #[test] + fn attaching_to_a_note_that_is_gone_writes_nothing() { + let conn = db(); + let blobs = blobs("gone"); + assert!(add_attachment(&conn, &blobs, "missing", "a.txt", "text/plain", b"hi").is_err()); + let rows: i64 = conn + .query_row("SELECT COUNT(*) FROM attachments", [], |r| r.get(0)) + .expect("count"); + assert_eq!(rows, 0); + let files = std::fs::read_dir(blobs.root()).expect("dir").count(); + assert_eq!(files, 0, "and no bytes were filed for it"); + } + + #[test] + fn only_a_file_the_server_holds_leaves_a_tombstone_when_removed() { + let conn = db(); + let blobs = blobs("remove"); + let n = note(&conn, "two files"); + conn.execute( + "INSERT INTO attachments (id, note_id, url) VALUES ('synced', ?1, '/x')", + [&n.id], + ) + .expect("synced row"); + let with_local = + add_attachment(&conn, &blobs, &n.id, "new.txt", "text/plain", b"hi").expect("attach"); + let local = with_local + .attachments + .iter() + .find(|a| a.id != "synced") + .expect("local") + .id + .clone(); + + delete_attachment(&conn, &n.id, "synced").expect("remove synced"); + delete_attachment(&conn, &n.id, &local).expect("remove local"); + + // Without the tombstone the next pull would put the synced file straight back. + // The local one never reached the server, so there is nothing to tell it. + assert_eq!( + tombstones(&conn), + vec![("attachment".to_string(), "synced".to_string())] + ); + } + + #[test] + fn dismissing_a_preview_leaves_a_tombstone() { + let conn = db(); + let n = note(&conn, "https://example.com"); + conn.execute( + "INSERT INTO link_previews (id, note_id, url) VALUES ('p1', ?1, 'https://example.com')", + [&n.id], + ) + .expect("preview"); + + delete_preview(&conn, &n.id, "p1").expect("dismiss"); + delete_preview(&conn, &n.id, "not-there").expect("a miss is fine"); + assert_eq!( + tombstones(&conn), + vec![("preview".to_string(), "p1".to_string())] + ); + } } diff --git a/core/src/sync/blobs.rs b/core/src/sync/blobs.rs index 8a0c8f4..0f46167 100644 --- a/core/src/sync/blobs.rs +++ b/core/src/sync/blobs.rs @@ -78,6 +78,15 @@ impl BlobStore { Ok(path) } + /// File bytes this device produced (a file attached here) and return their hash. + /// The hash is computed from the bytes, so unlike [`store`](Self::store) there is + /// nothing to verify against. + pub fn put(&self, bytes: &[u8]) -> Result { + let hash = digest(bytes); + self.store(&hash, bytes)?; + Ok(hash) + } + pub fn read(&self, sha256: &str) -> Option> { fs::read(self.path(sha256)?).ok() } @@ -140,7 +149,9 @@ fn urlencode(value: &str) -> String { out } -fn urldecode(value: &str) -> String { +/// Undo percent-encoding. Also used for the filename the desktop's attach command +/// receives in a header, which can only carry ASCII. +pub fn urldecode(value: &str) -> String { let bytes = value.as_bytes(); let mut out: Vec = Vec::with_capacity(bytes.len()); let mut i = 0; @@ -163,12 +174,14 @@ fn urldecode(value: &str) -> String { /// /// The mime rides in the URL and this scheme is an origin of its own, so echoing an /// arbitrary type would let an attachment claiming `text/html` run as a document -/// there. Echoing is safe only because of the FAMILY check: nothing starting with -/// `image/` can name a scriptable type. Everything else is served as an opaque -/// download — the right treatment for an arbitrary file regardless. +/// there. Echoing is safe only for the families that can't carry script, and +/// `image/` is not quite one of them: `image/svg+xml` is a document that runs its own +/// `