refactor(apply): unify divergence detection and drop the plan-path N+1
Review #3 (LOW). `divergence_preview` (plan path) compared parent slugs case-sensitively from its own `get_by_slug`, while the apply path (`reconcile_group_structure_tx`) compares resolved parent ids. On a mixed-case parent slug the two could disagree — `pic plan` reporting `diverged` while `pic apply` hard-errors "parent does not exist". `resolve_existing_group_ids` becomes `load_existing_groups`, returning the full `Group` rows it already loads; `plan_tree` derives the id map from them and passes the map to `divergence_preview`, which now reads each node's own row without a per-node `get_by_slug` (kills the N+1), resolves an in-tree parent's slug from the same map, and compares parent slugs case-insensitively so the preview agrees with the apply path's id-based check. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -1679,7 +1679,9 @@ impl ApplyService {
|
|||||||
bundle: &TreeBundle,
|
bundle: &TreeBundle,
|
||||||
project: Option<&ProjectDecl>,
|
project: Option<&ProjectDecl>,
|
||||||
) -> Result<TreePlanResult, ApplyError> {
|
) -> Result<TreePlanResult, ApplyError> {
|
||||||
let resolved = self.resolve_existing_group_ids(bundle).await?;
|
let existing = self.load_existing_groups(bundle).await?;
|
||||||
|
let resolved: HashMap<String, GroupId> =
|
||||||
|
existing.iter().map(|(k, g)| (k.clone(), g.id)).collect();
|
||||||
let (prepared, to_create, token) = self.prepare_tree(bundle, &resolved).await?;
|
let (prepared, to_create, token) = self.prepare_tree(bundle, &resolved).await?;
|
||||||
// §6/§7 M3: attach-point ceiling preview (consistent with apply_tree).
|
// §6/§7 M3: attach-point ceiling preview (consistent with apply_tree).
|
||||||
if let Some(pg_slug) = project.and_then(|p| p.parent_group.as_deref()) {
|
if let Some(pg_slug) = project.and_then(|p| p.parent_group.as_deref()) {
|
||||||
@@ -1694,7 +1696,7 @@ impl ApplyService {
|
|||||||
let ownership = self.ownership_preview(p.owner, declared).await?;
|
let ownership = self.ownership_preview(p.owner, declared).await?;
|
||||||
// §6 M2: for an existing group node, preview a structural divergence
|
// §6 M2: for an existing group node, preview a structural divergence
|
||||||
// (server parent ≠ manifest parent) before apply.
|
// (server parent ≠ manifest parent) before apply.
|
||||||
let structure = self.divergence_preview(p.node).await?;
|
let structure = self.divergence_preview(p.node, &existing).await?;
|
||||||
nodes.push(NodePlan {
|
nodes.push(NodePlan {
|
||||||
kind: p.node.kind,
|
kind: p.node.kind,
|
||||||
slug: p.node.slug.clone(),
|
slug: p.node.slug.clone(),
|
||||||
@@ -1932,12 +1934,14 @@ impl ApplyService {
|
|||||||
Ok(report)
|
Ok(report)
|
||||||
}
|
}
|
||||||
|
|
||||||
/// §6: resolve every GROUP node's slug → id on the pool, skipping ones that
|
/// §6: load every EXISTING group node (lowercased slug → its full row) on
|
||||||
/// don't exist yet (they are to-create, previewed by the plan path).
|
/// the pool, skipping ones that don't exist yet (to-create, previewed by the
|
||||||
async fn resolve_existing_group_ids(
|
/// plan path). Returns the whole `Group` so the divergence preview reads
|
||||||
|
/// `parent_id` (and resolves an in-tree parent's slug) without re-fetching.
|
||||||
|
async fn load_existing_groups(
|
||||||
&self,
|
&self,
|
||||||
bundle: &TreeBundle,
|
bundle: &TreeBundle,
|
||||||
) -> Result<HashMap<String, GroupId>, ApplyError> {
|
) -> Result<HashMap<String, picloud_shared::Group>, ApplyError> {
|
||||||
let mut map = HashMap::new();
|
let mut map = HashMap::new();
|
||||||
for n in &bundle.nodes {
|
for n in &bundle.nodes {
|
||||||
if n.kind == NodeKind::Group {
|
if n.kind == NodeKind::Group {
|
||||||
@@ -1947,7 +1951,7 @@ impl ApplyService {
|
|||||||
.await
|
.await
|
||||||
.map_err(|e| ApplyError::Backend(e.to_string()))?
|
.map_err(|e| ApplyError::Backend(e.to_string()))?
|
||||||
{
|
{
|
||||||
map.insert(n.slug.to_lowercase(), g.id);
|
map.insert(n.slug.to_lowercase(), g);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -1957,32 +1961,36 @@ impl ApplyService {
|
|||||||
/// §6 M2: preview an EXISTING group node's structural divergence — `Some`
|
/// §6 M2: preview an EXISTING group node's structural divergence — `Some`
|
||||||
/// only when the server's parent differs from the manifest's declared
|
/// only when the server's parent differs from the manifest's declared
|
||||||
/// (directory-nesting) parent. `None` for an app node, a to-create group, or
|
/// (directory-nesting) parent. `None` for an app node, a to-create group, or
|
||||||
/// an in-sync group.
|
/// an in-sync group. Reads the node's own row from the pre-loaded `existing`
|
||||||
|
/// map (no per-node `get_by_slug`), resolves the server parent's slug from
|
||||||
|
/// that map when the parent is another tree node (else one `get_by_id`), and
|
||||||
|
/// compares parent slugs **case-insensitively** so this preview agrees with
|
||||||
|
/// the apply path's id-based check (which lowercases) on a mixed-case slug.
|
||||||
async fn divergence_preview(
|
async fn divergence_preview(
|
||||||
&self,
|
&self,
|
||||||
node: &TreeNode,
|
node: &TreeNode,
|
||||||
|
existing: &HashMap<String, picloud_shared::Group>,
|
||||||
) -> Result<Option<StructurePreview>, ApplyError> {
|
) -> Result<Option<StructurePreview>, ApplyError> {
|
||||||
if node.kind != NodeKind::Group {
|
if node.kind != NodeKind::Group {
|
||||||
return Ok(None);
|
return Ok(None);
|
||||||
}
|
}
|
||||||
let Some(group) = self
|
let Some(group) = existing.get(&node.slug.to_lowercase()) else {
|
||||||
.groups
|
|
||||||
.get_by_slug(&node.slug)
|
|
||||||
.await
|
|
||||||
.map_err(|e| ApplyError::Backend(e.to_string()))?
|
|
||||||
else {
|
|
||||||
return Ok(None); // to-create — not a divergence
|
return Ok(None); // to-create — not a divergence
|
||||||
};
|
};
|
||||||
let server_parent = match group.parent_id {
|
let server_parent = match group.parent_id {
|
||||||
Some(pid) => self
|
Some(pid) => match existing.values().find(|g| g.id == pid) {
|
||||||
|
Some(g) => Some(g.slug.clone()),
|
||||||
|
None => self
|
||||||
.groups
|
.groups
|
||||||
.get_by_id(pid)
|
.get_by_id(pid)
|
||||||
.await
|
.await
|
||||||
.map_err(|e| ApplyError::Backend(e.to_string()))?
|
.map_err(|e| ApplyError::Backend(e.to_string()))?
|
||||||
.map(|g| g.slug),
|
.map(|g| g.slug),
|
||||||
|
},
|
||||||
None => None,
|
None => None,
|
||||||
};
|
};
|
||||||
if server_parent.as_deref() == node.parent.as_deref() {
|
let norm = |s: Option<&str>| s.map(str::to_lowercase);
|
||||||
|
if norm(server_parent.as_deref()) == norm(node.parent.as_deref()) {
|
||||||
return Ok(None); // in-sync
|
return Ok(None); // in-sync
|
||||||
}
|
}
|
||||||
Ok(Some(StructurePreview {
|
Ok(Some(StructurePreview {
|
||||||
|
|||||||
Reference in New Issue
Block a user