diff --git a/crates/yaak-models/src/content.rs b/crates/yaak-models/src/content.rs index 62661519..8b481d6f 100644 --- a/crates/yaak-models/src/content.rs +++ b/crates/yaak-models/src/content.rs @@ -7,6 +7,11 @@ //! They ask slightly different questions — see [`PLACEMENT_KEYS`] — so what is //! shared here is the mechanism and the reasoning, not one fixed answer. //! +//! The implementation is lifted from the import merge work on +//! `import-remember-selection` (#619), which got here first and got it right; +//! that branch's private copy should become a call into this module when it +//! lands. +//! //! Not shared with directory sync, deliberately. Sync checksums the *bytes of a //! file* to notice that someone edited it on disk, so its hash has to reflect //! formatting and key order — exactly what this module throws away. @@ -20,19 +25,24 @@ use sha2::{Digest, Sha256}; /// /// Every model carries them and every write rewrites at least `updatedAt`, so /// leaving them in would make every model differ from every copy of itself. -pub const IDENTITY_KEYS: &[&str] = &["model", "id", "createdAt", "updatedAt", "workspaceId"]; +/// `id` is not listed because it needs [`strip_ids`], which reaches nested rows +/// too. +pub const IDENTITY_KEYS: &[&str] = &["model", "workspaceId", "createdAt", "updatedAt"]; -/// Fields that say where a model sits among its siblings. +/// Fields that say where a model sits, rather than what it holds. /// -/// Whether these are content depends on the question being asked, which is why -/// they are a separate list. Versioning drops them: dragging a request into a -/// folder or up the sidebar is not an edit and must not mint a version or show -/// up in a diff. Import keeps them: a re-import that moved a resource somewhere -/// else *is* a change worth showing, because equality there means "same content -/// in the same place". +/// `sortPriority` is not content for anybody: importers number it from source +/// order, so comparing it turns one insertion into an update of everything +/// after it, and dragging a request up the sidebar is not an edit. +/// +/// `folderId` is where the two callers actually part company, and it is a real +/// disagreement rather than an oversight. Versioning drops it: moving a request +/// into a folder is not an edit and must not mint a version. Import keeps it: +/// equality there means "same content in the same place", so a source that +/// moved a resource is showing you a change. pub const PLACEMENT_KEYS: &[&str] = &["folderId", "sortPriority"]; -/// A model's JSON with the named keys removed. +/// A model's JSON with the named top-level keys removed. pub fn without_keys(mut value: Value, keys: &[&str]) -> Value { if let Some(object) = value.as_object_mut() { for key in keys { @@ -42,54 +52,70 @@ pub fn without_keys(mut value: Value, keys: &[&str]) -> Value { value } -/// A stable hash of a document's content. -pub fn content_hash(document: &Value) -> Result { - let mut canonical = String::new(); - write_canonical(document, &mut canonical); - Ok(hex::encode(Sha256::digest(canonical.as_bytes()))) +/// Drop every `id`, at every depth. +/// +/// A header, parameter, or variable carries an `id` that identifies its row to +/// the editor rather than anything about its content, and the editor fills +/// those in the first time it touches a resource. Dropping every `id` keeps +/// that from reading as a change — otherwise merely opening a request would +/// look like an edit of all of its headers at once. +pub fn strip_ids(value: Value) -> Value { + match value { + Value::Object(object) => Value::Object( + object + .into_iter() + .filter(|(key, _)| key != "id") + .map(|(key, value)| (key, strip_ids(value))) + .collect(), + ), + Value::Array(items) => Value::Array(items.into_iter().map(strip_ids).collect()), + other => other, + } } -/// Serialize with object keys in sorted order. +/// Prefix on every hash this module writes. /// -/// Plain `to_string` would not do: whether `serde_json::Map` preserves -/// insertion order or sorts is a workspace-wide feature decision — with -/// `preserve_order` on in some builds of this workspace and off in others — and -/// a document read back from SQLite has whatever order it was written in. -/// Sorting here makes the hash depend on the content and nothing else, in every -/// build. -fn write_canonical(value: &Value, out: &mut String) { +/// A hash written by a version a build doesn't understand says nothing about +/// the content, and the caller needs to be able to tell that apart from a hash +/// that says "different". Bump it whenever the stripping or the canonical form +/// changes. +pub const CONTENT_HASH_VERSION: &str = "v1:"; + +/// A stable hash of a document's content. +pub fn content_hash(document: &Value) -> Result { + let canonical = serde_json::to_string(&sorted_keys(document.clone()))?; + Ok(format!("{CONTENT_HASH_VERSION}{:x}", Sha256::digest(canonical.as_bytes()))) +} + +/// Whether a stored hash was written by an algorithm this build understands. +pub fn hash_is_readable(hash: &str) -> bool { + hash.starts_with(CONTENT_HASH_VERSION) +} + +/// Rebuild every object with its keys in sorted order. +/// +/// Serializing straight from the input would not do: whether +/// `serde_json::Map` preserves insertion order or sorts is a workspace-wide +/// feature decision — `preserve_order` is on in some builds of this workspace +/// and off in others — and a document read back from SQLite has whatever order +/// it was written in. Sorting first makes the hash depend on the content and +/// nothing else, in every build. +fn sorted_keys(value: Value) -> Value { match value { - Value::Object(map) => { - let mut keys = map.keys().collect::>(); - keys.sort_unstable(); - out.push('{'); - for (i, key) in keys.into_iter().enumerate() { - if i > 0 { - out.push(','); - } - write_canonical(&Value::String(key.clone()), out); - out.push(':'); - write_canonical(&map[key], out); - } - out.push('}'); + Value::Object(object) => { + let mut entries = object.into_iter().collect::>(); + entries.sort_by(|(a, _), (b, _)| a.cmp(b)); + Value::Object(entries.into_iter().map(|(k, v)| (k, sorted_keys(v))).collect()) } - Value::Array(items) => { - out.push('['); - for (i, item) in items.iter().enumerate() { - if i > 0 { - out.push(','); - } - write_canonical(item, out); - } - out.push(']'); - } - scalar => out.push_str(&scalar.to_string()), + Value::Array(items) => Value::Array(items.into_iter().map(sorted_keys).collect()), + other => other, } } #[cfg(test)] mod tests { use super::*; + use serde_json::json; /// The hash has to survive a round trip through SQLite, which stores a /// document as text and hands back whatever order it was written in. It @@ -121,13 +147,39 @@ mod tests { assert_ne!(content_hash(&a).unwrap(), content_hash(&b).unwrap()); } + #[test] + fn hashes_carry_a_readable_version() { + let hash = content_hash(&json!({"url": "a"})).unwrap(); + assert!(hash_is_readable(&hash)); + assert!(!hash_is_readable("v99:deadbeef")); + assert!(!hash_is_readable("deadbeef")); + } + #[test] fn without_keys_leaves_everything_else_alone() { - let value: Value = serde_json::json!({"id": "rq_1", "url": "a", "name": "n"}); - let stripped = without_keys(value, IDENTITY_KEYS); + let stripped = without_keys(json!({"model": "http_request", "url": "a"}), IDENTITY_KEYS); let object = stripped.as_object().unwrap(); - assert!(!object.contains_key("id")); + assert!(!object.contains_key("model")); assert_eq!(object.get("url").unwrap(), "a"); - assert_eq!(object.get("name").unwrap(), "n"); + } + + /// The editor writes row ids into headers and parameters the first time it + /// touches a request, so nested ids have to go or that reads as an edit. + #[test] + fn strip_ids_reaches_nested_rows() { + let with_ids = json!({ + "id": "rq_1", + "url": "a", + "headers": [{"id": "h_1", "name": "Accept", "value": "*/*"}], + }); + let without = json!({ + "url": "a", + "headers": [{"name": "Accept", "value": "*/*"}], + }); + assert_eq!(strip_ids(with_ids.clone()), strip_ids(without.clone())); + assert_eq!( + content_hash(&strip_ids(with_ids)).unwrap(), + content_hash(&strip_ids(without)).unwrap(), + ); } } diff --git a/crates/yaak-models/src/queries/model_versions.rs b/crates/yaak-models/src/queries/model_versions.rs index 7a1a6c06..8566a2f6 100644 --- a/crates/yaak-models/src/queries/model_versions.rs +++ b/crates/yaak-models/src/queries/model_versions.rs @@ -267,6 +267,53 @@ mod tests { assert_eq!(db.list_model_versions(&request.id).unwrap().len(), 1); } + /// The pair editor writes a generated `id` into every header row the first + /// time it touches a request, and that write reaches the database like any + /// other. Without nested id stripping, merely opening a request would mint + /// a version whose diff is nothing but ids. + #[test] + fn row_ids_written_by_the_editor_do_not_mint_a_version() { + let (query_manager, _blobs, _rx) = init_in_memory().expect("Failed to init DB"); + let db = query_manager.connect(); + let (_workspace, request) = seed(&db); + + let header = |id: Option<&str>| crate::models::HttpRequestHeader { + name: "Accept".to_string(), + value: "application/json".to_string(), + id: id.map(str::to_string), + ..Default::default() + }; + + let request = db + .upsert_http_request(&HttpRequest { headers: vec![header(None)], ..request }, &source()) + .unwrap(); + let first = snapshot(&db, &request.id, ModelVersionReason::Send); + + // Opening the request in the editor fills the row id in + db.upsert_http_request( + &HttpRequest { headers: vec![header(Some("row_generated"))], ..request.clone() }, + &source(), + ) + .unwrap(); + + assert_eq!(snapshot(&db, &request.id, ModelVersionReason::Idle).id, first.id); + assert_eq!(db.list_model_versions(&request.id).unwrap().len(), 1); + + // A real edit to the same row still counts + db.upsert_http_request( + &HttpRequest { + headers: vec![crate::models::HttpRequestHeader { + value: "text/plain".to_string(), + ..header(Some("row_generated")) + }], + ..request.clone() + }, + &source(), + ) + .unwrap(); + assert_ne!(snapshot(&db, &request.id, ModelVersionReason::Idle).id, first.id); + } + #[test] fn editing_content_mints_a_version() { let (query_manager, _blobs, _rx) = init_in_memory().expect("Failed to init DB"); diff --git a/crates/yaak-models/src/versions.rs b/crates/yaak-models/src/versions.rs index 0b14dcfd..e7cd2046 100644 --- a/crates/yaak-models/src/versions.rs +++ b/crates/yaak-models/src/versions.rs @@ -4,7 +4,7 @@ //! decided here is versioning's own policy: placement is not content, and a //! restore lays a document back over the model it came from. -use crate::content::{IDENTITY_KEYS, PLACEMENT_KEYS, without_keys}; +use crate::content::{IDENTITY_KEYS, PLACEMENT_KEYS, strip_ids, without_keys}; use crate::error::Result; use serde::Serialize; use serde_json::{Map, Value}; @@ -14,12 +14,15 @@ use serde_json::{Map, Value}; /// Dropping [`PLACEMENT_KEYS`] as well as [`IDENTITY_KEYS`] is what makes a /// version stable: moving a request into a folder, dragging it up the sidebar, /// or simply saving it again rewrite those and nothing else, and none of them -/// should mint a version or show up in a diff. It is also why one rule covers -/// HTTP, gRPC and WebSocket — the three differ only in the content fields, -/// which are all kept. +/// should mint a version or show up in a diff. `strip_ids` does the same job +/// for the row ids the editor writes into headers and parameters — without it, +/// opening a request would mint a version whose diff is nothing but ids. +/// +/// One rule covers HTTP, gRPC and WebSocket, because the three differ only in +/// the content fields, which are all kept. pub fn version_document(model: &T) -> Result { let stripped = [IDENTITY_KEYS, PLACEMENT_KEYS].concat(); - Ok(without_keys(serde_json::to_value(model)?, &stripped)) + Ok(without_keys(strip_ids(serde_json::to_value(model)?), &stripped)) } /// Lay a version's document back over a live model. @@ -109,8 +112,9 @@ mod tests { } /// The other half of the split documented on [`PLACEMENT_KEYS`]. Import - /// counts a move as a change; versioning must not, or dragging a request - /// around the sidebar would mint versions nobody asked for. + /// counts a move between folders as a change; versioning must not, or + /// dragging a request around the sidebar would mint versions nobody asked + /// for. #[test] fn placement_is_not_content_here_even_though_import_says_it_is() { let base = version_document(&request()).unwrap(); diff --git a/crates/yaak/src/import.rs b/crates/yaak/src/import.rs index a60eeeb4..82501f1e 100644 --- a/crates/yaak/src/import.rs +++ b/crates/yaak/src/import.rs @@ -4,7 +4,6 @@ use log::info; use serde_json::Value; use std::collections::{BTreeMap, BTreeSet}; use yaak_models::client_db::ClientDb; -use yaak_models::content::{IDENTITY_KEYS, without_keys}; use yaak_models::models::{ AnyModel, DEFAULT_REQUEST_MESSAGE_SIZE, Environment, Folder, GrpcRequest, HttpRequest, ImportSource, ImportSourceResource, UpsertModelInfo, WebsocketRequest, Workspace, @@ -842,18 +841,15 @@ fn create_only_items(plan: &ImportPlan) -> Vec { items } -/// Strip identity fields so equality means "same content in the same place". -/// -/// Placement (`folderId`, `sortPriority`) is deliberately *kept*, which is -/// where this parts company with request versioning: a re-import that moved a -/// resource somewhere else is a change worth showing, while dragging a request -/// around the sidebar is not an edit. See [`yaak_models::content`]. -/// -/// The deprecated environment `base` flag mirrors `parentModel`, which is -/// compared already. -fn comparable(value: Value) -> Value { - let stripped = [IDENTITY_KEYS, &["base"]].concat(); - without_keys(value, &stripped) +/// Strip identity and bookkeeping fields so equality means "same content in the same place". +/// The deprecated environment `base` flag mirrors `parentModel`, which is compared already. +fn comparable(mut value: Value) -> Value { + if let Some(object) = value.as_object_mut() { + for field in ["id", "model", "workspaceId", "createdAt", "updatedAt", "base"] { + object.remove(field); + } + } + value } fn existing_model_json( @@ -1208,42 +1204,6 @@ mod tests { use serde_json::json; use yaak_models::models::{EnvironmentVariable, HttpRequestHeader}; - /// Import and request versioning share the stripping mechanism but not the - /// key list, and this is the difference. Versioning drops placement so - /// dragging a request around the sidebar is not an edit; import keeps it so - /// a source that moved a resource reads as a change. Unifying the two lists - /// would silently make a re-import stop noticing moves. - #[test] - fn comparable_treats_a_move_as_a_change_but_ignores_identity() { - let base = json!({ - "model": "http_request", - "id": "rq_1", - "workspaceId": "wk_1", - "createdAt": "2026-01-01T00:00:00", - "updatedAt": "2026-01-01T00:00:00", - "folderId": "fl_1", - "sortPriority": 1.0, - "url": "https://example.com", - }); - - let mut renamed_identity = base.clone(); - renamed_identity["id"] = json!("rq_2"); - renamed_identity["updatedAt"] = json!("2026-09-06T00:00:00"); - assert_eq!( - comparable(renamed_identity), - comparable(base.clone()), - "identity and timestamps are never content", - ); - - let mut moved = base.clone(); - moved["folderId"] = json!("fl_2"); - assert_ne!(comparable(moved), comparable(base.clone()), "a move is a change"); - - let mut resorted = base.clone(); - resorted["sortPriority"] = json!(99.0); - assert_ne!(comparable(resorted), comparable(base), "a reorder is a change"); - } - fn destination_workspace() -> Workspace { Workspace { id: "wk_destination".to_string(),