mirror of
https://github.com/mountain-loop/yaak.git
synced 2026-09-08 02:41:54 +02:00
fix(models): don't mint a version for row ids the editor writes
#619 got here first and got more of it right. Two things adopted from it, and one backed out. **Adopted, and this one is a bug fix.** The pair editor generates an `id` for every header and parameter row the first time it touches a request, and that write reaches the database like any other. Stripping only the top-level `id` left those in, so merely opening a request minted a version whose diff was nothing but ids. `strip_ids` drops every `id` at every depth. The regression test fails without it. **Adopted.** Hashes carry a `v1:` prefix, so a hash written by an algorithm a build doesn't understand is recognizable as unreadable rather than silently read as "different". And `sorted_keys` + `to_string` replaces the hand-rolled canonical writer — same guarantee, less code, and byte-identical to the implementation that is landing. **Backed out.** `comparable()` in import.rs goes back to its own copy. #619 rewrites that function and most of the file around it, including dropping `sortPriority` from the comparison, so pointing it at the shared module now would collide in the hottest region of that branch to save six lines. This module is shaped to be what #619 calls when it lands, and says so; the convergence is one deletion away rather than a reconciliation. That leaves `folderId` as the only real disagreement between the two callers, which PLACEMENT_KEYS now documents as such: import counts a move between folders as a change, versioning must not.
This commit is contained in:
@@ -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<String> {
|
||||
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<String> {
|
||||
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::<Vec<_>>();
|
||||
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::<Vec<_>>();
|
||||
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(),
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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");
|
||||
|
||||
@@ -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<T: Serialize>(model: &T) -> Result<Value> {
|
||||
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();
|
||||
|
||||
@@ -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<ImportPlanItem> {
|
||||
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(),
|
||||
|
||||
Reference in New Issue
Block a user