From aa3bf934a9f03b2689fcf053348532b9c8e15caf Mon Sep 17 00:00:00 2001 From: Gregory Schier Date: Sun, 6 Sep 2026 09:07:59 -0700 Subject: [PATCH] refactor(models): share the content-stripping primitive with import MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Import already had this: `comparable()` in import.rs, added last week, strips identity fields off a model so a re-import can tell a change from a conflict. Request versioning arrived with its own copy of the same idea. Now both call `yaak_models::content`, which owns the mechanism and — more usefully — the reasoning. Two things had no single home before: why identity fields can never count as content, and why hashing has to sort object keys itself rather than trusting `serde_json::Map` to be a `BTreeMap` (`preserve_order` is on in some builds of this workspace and off in others). The key lists stay separate on purpose, as IDENTITY_KEYS and PLACEMENT_KEYS. Import counts a move as a change, because equality there means "same content in the same place"; versioning must not, or dragging a request around the sidebar would mint versions nobody asked for. A test on each side pins that difference, since the obvious next refactor is to collapse the two lists into one and neither behaviour would fail loudly if you did. Directory sync deliberately keeps its own Sha1 and is not folded in. It checksums the bytes of a file to notice someone edited it on disk, so it has to reflect formatting and key order — the exact things this module discards. --- crates/yaak-models/src/content.rs | 133 ++++++++++++++++++ crates/yaak-models/src/lib.rs | 1 + .../yaak-models/src/queries/model_versions.rs | 3 +- crates/yaak-models/src/versions.rs | 128 +++++------------ crates/yaak/src/import.rs | 58 ++++++-- 5 files changed, 217 insertions(+), 106 deletions(-) create mode 100644 crates/yaak-models/src/content.rs diff --git a/crates/yaak-models/src/content.rs b/crates/yaak-models/src/content.rs new file mode 100644 index 00000000..62661519 --- /dev/null +++ b/crates/yaak-models/src/content.rs @@ -0,0 +1,133 @@ +//! What counts as a model's *content*, and how content becomes a hash. +//! +//! Several features need to answer "are these two models the same?" without +//! being fooled by the fields that change every time a model is written at all: +//! request versioning asks it to decide whether to capture a new version, +//! import asks it to decide whether a re-import is a change or a conflict. +//! They ask slightly different questions — see [`PLACEMENT_KEYS`] — so what is +//! shared here is the mechanism and the reasoning, not one fixed answer. +//! +//! 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. + +use crate::error::Result; +use serde_json::Value; +use sha2::{Digest, Sha256}; + +/// Fields that say which model this is and when it was last touched, rather +/// than anything a user typed. +/// +/// 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"]; + +/// Fields that say where a model sits among its siblings. +/// +/// 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". +pub const PLACEMENT_KEYS: &[&str] = &["folderId", "sortPriority"]; + +/// A model's JSON with the named keys removed. +pub fn without_keys(mut value: Value, keys: &[&str]) -> Value { + if let Some(object) = value.as_object_mut() { + for key in keys { + object.remove(*key); + } + } + 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()))) +} + +/// Serialize with object keys in sorted order. +/// +/// 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) { + 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::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()), + } +} + +#[cfg(test)] +mod tests { + use super::*; + + /// 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 + /// also has to survive `serde_json`'s `preserve_order` feature being on in + /// one build of the workspace and off in another. + #[test] + fn key_order_does_not_change_the_hash() { + let a: Value = serde_json::from_str(r#"{"url":"a","method":"GET"}"#).unwrap(); + let b: Value = serde_json::from_str(r#"{"method":"GET","url":"a"}"#).unwrap(); + assert_eq!(content_hash(&a).unwrap(), content_hash(&b).unwrap()); + } + + #[test] + fn key_order_does_not_change_the_hash_when_nested() { + let a: Value = + serde_json::from_str(r#"{"body":{"text":"x","type":"json"},"headers":[{"a":1,"b":2}]}"#) + .unwrap(); + let b: Value = + serde_json::from_str(r#"{"headers":[{"b":2,"a":1}],"body":{"type":"json","text":"x"}}"#) + .unwrap(); + assert_eq!(content_hash(&a).unwrap(), content_hash(&b).unwrap()); + } + + /// Sorting keys must not make different documents collide. + #[test] + fn array_order_still_changes_the_hash() { + let a: Value = serde_json::from_str(r#"{"headers":[{"n":"a"},{"n":"b"}]}"#).unwrap(); + let b: Value = serde_json::from_str(r#"{"headers":[{"n":"b"},{"n":"a"}]}"#).unwrap(); + assert_ne!(content_hash(&a).unwrap(), content_hash(&b).unwrap()); + } + + #[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 object = stripped.as_object().unwrap(); + assert!(!object.contains_key("id")); + assert_eq!(object.get("url").unwrap(), "a"); + assert_eq!(object.get("name").unwrap(), "n"); + } +} diff --git a/crates/yaak-models/src/lib.rs b/crates/yaak-models/src/lib.rs index 063ff728..966ee711 100644 --- a/crates/yaak-models/src/lib.rs +++ b/crates/yaak-models/src/lib.rs @@ -10,6 +10,7 @@ use yaak_database::SqlitePool; pub mod blob_manager; pub mod client_db; +pub mod content; pub mod cookies; mod connection_or_tx; pub mod error; diff --git a/crates/yaak-models/src/queries/model_versions.rs b/crates/yaak-models/src/queries/model_versions.rs index 95edc6d2..7a1a6c06 100644 --- a/crates/yaak-models/src/queries/model_versions.rs +++ b/crates/yaak-models/src/queries/model_versions.rs @@ -6,7 +6,8 @@ use crate::models::{ }; use crate::queries::any_request::AnyRequest; use crate::util::UpdateSource; -use crate::versions::{apply_version_document, content_hash, version_document}; +use crate::content::content_hash; +use crate::versions::{apply_version_document, version_document}; use log::warn; use sea_query::{Expr, ExprTrait, Query, SqliteQueryBuilder}; use sea_query_rusqlite::RusqliteBinder; diff --git a/crates/yaak-models/src/versions.rs b/crates/yaak-models/src/versions.rs index 433e98c3..0b14dcfd 100644 --- a/crates/yaak-models/src/versions.rs +++ b/crates/yaak-models/src/versions.rs @@ -1,78 +1,25 @@ -//! Content addressing for model versions. +//! Request versioning's answer to "what is this request's content?" //! -//! A version's identity is its *content*, so the two functions here — what -//! counts as content, and how content becomes a hash — are the whole of it. -//! Everything else about versioning (when to capture, what to keep, how to -//! restore) is built on top and stays in `queries::model_versions`. +//! The mechanism lives in [`crate::content`], shared with import. What is +//! 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::error::Result; use serde::Serialize; use serde_json::{Map, Value}; -use sha2::{Digest, Sha256}; - -/// Keys that describe a model's place in the workspace rather than what the -/// user typed into it. -/// -/// Dropping them is what makes a version stable: moving a request into a -/// folder, dragging it up the sidebar, or simply saving it again all rewrite -/// these 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. -const BOOKKEEPING_KEYS: &[&str] = - &["model", "id", "createdAt", "updatedAt", "workspaceId", "folderId", "sortPriority"]; /// The editable content of a model, as the object a version stores. -pub fn version_document(model: &T) -> Result { - let mut value = serde_json::to_value(model)?; - if let Some(object) = value.as_object_mut() { - for key in BOOKKEEPING_KEYS { - object.remove(*key); - } - } - Ok(value) -} - -/// The hash a version is addressed by. -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()))) -} - -/// Serialize with object keys in sorted order. /// -/// Plain `to_string` would not do: whether `serde_json::Map` preserves -/// insertion order or sorts is a workspace-wide feature decision, 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) { - 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::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()), - } +/// 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. +pub fn version_document(model: &T) -> Result { + let stripped = [IDENTITY_KEYS, PLACEMENT_KEYS].concat(); + Ok(without_keys(serde_json::to_value(model)?, &stripped)) } /// Lay a version's document back over a live model. @@ -95,6 +42,7 @@ pub fn apply_version_document(live: &Value, document: &Value) -> Value { #[cfg(test)] mod tests { use super::*; + use crate::content::content_hash; use crate::models::{HttpRequest, HttpRequestHeader}; use chrono::Utc; @@ -125,8 +73,8 @@ mod tests { let document = version_document(&request()).unwrap(); let object = document.as_object().unwrap(); - for key in BOOKKEEPING_KEYS { - assert!(!object.contains_key(*key), "document should not carry {key}"); + for key in [IDENTITY_KEYS, PLACEMENT_KEYS].concat() { + assert!(!object.contains_key(key), "document should not carry {key}"); } assert_eq!(object.get("url").unwrap(), "https://example.com/users/1"); @@ -160,6 +108,21 @@ mod tests { assert_eq!(hash_of(&moved_workspace), base, "workspace"); } + /// 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. + #[test] + fn placement_is_not_content_here_even_though_import_says_it_is() { + let base = version_document(&request()).unwrap(); + let moved = + version_document(&HttpRequest { folder_id: Some("fl_2".into()), ..request() }).unwrap(); + let resorted = + version_document(&HttpRequest { sort_priority: 99.5, ..request() }).unwrap(); + + assert_eq!(moved, base); + assert_eq!(resorted, base); + } + #[test] fn editable_content_changes_the_hash() { let base = hash_of(&request()); @@ -175,35 +138,8 @@ mod tests { ); } - /// The hash has to survive a round trip through SQLite, which stores the - /// document as text and hands back whatever order it was written in. It - /// also has to survive `serde_json`'s `preserve_order` feature being on in - /// one build of the workspace and off in another. - #[test] - fn key_order_does_not_change_the_hash() { - let a: Value = serde_json::from_str(r#"{"url":"a","method":"GET"}"#).unwrap(); - let b: Value = serde_json::from_str(r#"{"method":"GET","url":"a"}"#).unwrap(); - assert_eq!(content_hash(&a).unwrap(), content_hash(&b).unwrap()); - } - #[test] - fn key_order_does_not_change_the_hash_when_nested() { - let a: Value = - serde_json::from_str(r#"{"body":{"text":"x","type":"json"},"headers":[{"a":1,"b":2}]}"#) - .unwrap(); - let b: Value = - serde_json::from_str(r#"{"headers":[{"b":2,"a":1}],"body":{"type":"json","text":"x"}}"#) - .unwrap(); - assert_eq!(content_hash(&a).unwrap(), content_hash(&b).unwrap()); - } - /// Sorting keys must not make different documents collide. - #[test] - fn array_order_still_changes_the_hash() { - let a: Value = serde_json::from_str(r#"{"headers":[{"n":"a"},{"n":"b"}]}"#).unwrap(); - let b: Value = serde_json::from_str(r#"{"headers":[{"n":"b"},{"n":"a"}]}"#).unwrap(); - assert_ne!(content_hash(&a).unwrap(), content_hash(&b).unwrap()); - } #[test] fn applying_a_document_keeps_the_live_model_identity() { diff --git a/crates/yaak/src/import.rs b/crates/yaak/src/import.rs index 82501f1e..a60eeeb4 100644 --- a/crates/yaak/src/import.rs +++ b/crates/yaak/src/import.rs @@ -4,6 +4,7 @@ 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, @@ -841,15 +842,18 @@ fn create_only_items(plan: &ImportPlan) -> Vec { items } -/// 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 +/// 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) } fn existing_model_json( @@ -1204,6 +1208,42 @@ 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(),