mirror of
https://github.com/mountain-loop/yaak.git
synced 2026-09-11 12:21:45 +02:00
fix(import): close three holes found in review
Checking an ignored folder selected every deletion beneath it, including the one the folder caused: a resource the source had moved into it was offered for deletion precisely because the folder was not imported, so checking the folder deleted the resource instead of keeping it for the move. A checkbox that means "keep these" can never mean "delete that". Binding a re-import to a linked source took a single shared key as proof, and two API documents naming one operation the same way is not proof — that let an unrelated import remap onto somebody else's resources. A document keeps its keys, so it now has to account for most of both sides, and only against a source from the same importer. A key the user turned down was forgotten as soon as the source stopped listing it, so the question came back the next time the source did. The row outlives the absence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
b6818b4f52
commit
a73cb245ea
@@ -264,7 +264,7 @@ function LoadedImportDataDialog({
|
||||
// say which of those are destructive. Checking anything also brings back the folders it needs
|
||||
// to live in.
|
||||
const toggleNode = (node: CheckboxTreeNode<TreeRow>, checked: boolean) => {
|
||||
const targets = new Set(togglableItems(node).map((i) => i.modelId));
|
||||
const targets = new Set(carriedItems(node).map((i) => i.modelId));
|
||||
if (checked && node.data.kind === "item") {
|
||||
const byId = new Map(items.map((i) => [i.modelId, i]));
|
||||
for (const ancestor of ancestorsOf(node.data.item, byId)) {
|
||||
@@ -776,8 +776,20 @@ function togglableItems(node: CheckboxTreeNode<TreeRow>): ImportPlanItem[] {
|
||||
.filter((i) => i != null);
|
||||
}
|
||||
|
||||
/**
|
||||
* What a row's checkbox carries: its subtree, less any deletion that exists only because a folder
|
||||
* above it isn't imported. Checking that folder is how the user keeps those resources, so it must
|
||||
* not be how they delete them — the move follows on the next import.
|
||||
*/
|
||||
function carriedItems(node: CheckboxTreeNode<TreeRow>): ImportPlanItem[] {
|
||||
const self = node.data.kind === "item" ? node.data.item.modelId : null;
|
||||
return togglableItems(node).filter(
|
||||
(i) => i.modelId === self || i.reason !== "moved_into_ignored_folder",
|
||||
);
|
||||
}
|
||||
|
||||
function nodeCheckedStatus(node: CheckboxTreeNode<TreeRow>): boolean | "indeterminate" | "hidden" {
|
||||
const covered = togglableItems(node);
|
||||
const covered = carriedItems(node);
|
||||
if (covered.length === 0) return "hidden";
|
||||
const selected = covered.filter((i) => i.selected).length;
|
||||
if (selected === covered.length) return true;
|
||||
|
||||
+91
-13
@@ -620,8 +620,11 @@ fn record_import_source(
|
||||
if incoming_keys.contains(&row.source_key) {
|
||||
continue;
|
||||
}
|
||||
// Keep only rows that back a deletion the user deselected; it will be offered again.
|
||||
let keep = match (ImportResourceType::from_str(&row.model_type), &row.model_id) {
|
||||
// A source that drops a resource for a release must not erase the answer to
|
||||
// "do you want this?", or the answer would be asked again when it returns.
|
||||
(_, None) => true,
|
||||
// Otherwise keep only rows backing a deletion the user deselected; it is offered again
|
||||
(Some(resource), Some(model_id)) => {
|
||||
items
|
||||
.get(model_id)
|
||||
@@ -662,11 +665,15 @@ fn resolve_linked_source(
|
||||
|
||||
let mut overlapping = Vec::new();
|
||||
for source in &sources {
|
||||
let overlaps = db
|
||||
.list_import_source_resources(&source.id)?
|
||||
.iter()
|
||||
.any(|row| incoming_keys.contains(&row.source_key));
|
||||
if overlaps {
|
||||
// A document keeps its keys between imports, so the same document shows up as most of
|
||||
// both key sets. A stray key in common — two API specs naming an operation the same way
|
||||
// — is a coincidence, and merging on it would rewrite somebody else's resources.
|
||||
if source.importer != importer {
|
||||
continue;
|
||||
}
|
||||
let rows = db.list_import_source_resources(&source.id)?;
|
||||
let shared = rows.iter().filter(|row| incoming_keys.contains(&row.source_key)).count();
|
||||
if shared * 2 > rows.len() && shared * 2 > incoming_keys.len() {
|
||||
overlapping.push(source.clone());
|
||||
}
|
||||
}
|
||||
@@ -2672,13 +2679,15 @@ mod tests {
|
||||
&UpdateSource::Import,
|
||||
)
|
||||
.expect("create second source");
|
||||
db.upsert_import_source_resource(&ImportSourceResource {
|
||||
import_source_id: other.id,
|
||||
source_key: "op:root".to_string(),
|
||||
model_type: "http_request".to_string(),
|
||||
..Default::default()
|
||||
})
|
||||
.expect("claim the same key");
|
||||
for key in ["env:base", "folder:src", "op:root", "op:nested"] {
|
||||
db.upsert_import_source_resource(&ImportSourceResource {
|
||||
import_source_id: other.id.clone(),
|
||||
source_key: key.to_string(),
|
||||
model_type: "http_request".to_string(),
|
||||
..Default::default()
|
||||
})
|
||||
.expect("claim the same keys");
|
||||
}
|
||||
}
|
||||
|
||||
let third =
|
||||
@@ -2698,6 +2707,75 @@ mod tests {
|
||||
assert!(warning.detail.contains("copy.yaml"), "{warning:?}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_stray_shared_key_does_not_link_an_unrelated_document() {
|
||||
let (query_manager, _blob_manager, _rx) =
|
||||
yaak_models::init_in_memory().expect("initialize database");
|
||||
let committed = first_import(&query_manager);
|
||||
let workspace_id = committed.workspaces[0].id.clone();
|
||||
let root_id = committed
|
||||
.http_requests
|
||||
.iter()
|
||||
.find(|r| r.name == "Root Request")
|
||||
.expect("root request")
|
||||
.id
|
||||
.clone();
|
||||
|
||||
// Another document that happens to name one operation the same way.
|
||||
let resources = ImportResources {
|
||||
http_requests: vec![HttpRequest {
|
||||
id: "rq_root".to_string(),
|
||||
model: "http_request".to_string(),
|
||||
workspace_id: "wk_source".to_string(),
|
||||
name: "Unrelated Request".to_string(),
|
||||
method: "GET".to_string(),
|
||||
url: "https://unrelated.example.com".to_string(),
|
||||
..Default::default()
|
||||
}],
|
||||
..Default::default()
|
||||
};
|
||||
let origin =
|
||||
ImportOrigin { origin: "/tmp/other.yaml".to_string(), label: "other.yaml".to_string() };
|
||||
|
||||
let plan = replan_from(&query_manager, &workspace_id, resources, origin);
|
||||
let unrelated = item_by_name(&plan, "Unrelated Request");
|
||||
assert_eq!(unrelated.action, ImportPlanAction::Create, "one key in common proves nothing");
|
||||
assert_ne!(unrelated.model_id, root_id, "it must not adopt another document's model");
|
||||
|
||||
commit_import_plan(&query_manager, plan).expect("commit the unrelated import");
|
||||
let db = query_manager.connect();
|
||||
assert_eq!(
|
||||
db.get_http_request(&root_id).expect("root request survives").url,
|
||||
"https://example.com/root"
|
||||
);
|
||||
assert_eq!(db.list_http_requests(&workspace_id).expect("list").len(), 3);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn an_ignored_key_stays_ignored_while_the_source_omits_it() {
|
||||
let (query_manager, _blob_manager, _rx) =
|
||||
yaak_models::init_in_memory().expect("initialize database");
|
||||
let committed = first_import(&query_manager);
|
||||
let workspace_id = committed.workspaces[0].id.clone();
|
||||
|
||||
let mut with_extra = imported_resources();
|
||||
with_extra.http_requests.push(extra_request());
|
||||
|
||||
let mut plan = replan(&query_manager, &workspace_id, with_extra.clone());
|
||||
select(&mut plan, "Extra Request", false);
|
||||
commit_import_plan(&query_manager, plan).expect("commit with the create turned down");
|
||||
|
||||
// The source drops it for a release, then brings it back.
|
||||
let plan = replan(&query_manager, &workspace_id, imported_resources());
|
||||
assert!(plan.items.iter().all(|i| i.name != "Extra Request"), "{:?}", plan.items);
|
||||
commit_import_plan(&query_manager, plan).expect("commit without it");
|
||||
|
||||
let plan = replan(&query_manager, &workspace_id, with_extra);
|
||||
let extra = item_by_name(&plan, "Extra Request");
|
||||
assert_eq!(extra.action, ImportPlanAction::Ignored, "the answer outlives the absence");
|
||||
assert!(!extra.selected);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn an_unrecognized_hash_degrades_to_a_conflict() {
|
||||
let (query_manager, _blob_manager, _rx) =
|
||||
|
||||
Reference in New Issue
Block a user