From 5efb068b9f0543c5dec85255c88df6155a9394ea Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Tue, 30 Jun 2026 19:04:43 +0200 Subject: [PATCH] =?UTF-8?q?feat(cli):=20kind-aware=20shared-collection=20r?= =?UTF-8?q?econcile=20+=20string-or-table=20manifest=20(=C2=A711.6=20docs?= =?UTF-8?q?=20C4)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make the declarative collection surface carry a `kind` so docs (and future kinds) are declarable. Back-compatible: the shipped `collections = ["catalog"]` form still means kv. - manifest: `collections` entries are now `CollectionDecl` — an untagged enum of a bare string (kv shorthand) or a `{ name, kind }` table (kind ∈ kv/docs). `Manifest::collections()` normalizes to `(name, kind)` pairs; build_bundle emits `[{name, kind}]`. Still `[group]`-only. Unit test covers the string-or-table mix + app rejection. - apply: `Bundle.collections: Vec` ({name, kind=default "kv"}); `CurrentState.collections: Vec<(name,kind)>` via list_all_for_owner; `diff_collections` keys each change by name with `detail = kind` and identity `(LOWER(name), kind)` (so a kv and a docs collection of the same name are distinct); reconcile reads the kind from `detail`; state_token folds `coll|{kind}|{name}`; validate_bundle rejects unknown kinds + dups by (name,kind); `collection_report`/CollectionInfo + `pic collections ls` gain a kind column. Existing kv journeys + nearest-wins still green (the bare-string form normalizes to kind=kv end to end). Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/manager-core/src/apply_service.rs | 131 +++++++++++++-------- crates/picloud-cli/src/client.rs | 2 + crates/picloud-cli/src/cmds/collections.rs | 4 +- crates/picloud-cli/src/cmds/plan.rs | 6 +- crates/picloud-cli/src/manifest.rs | 101 ++++++++++++++-- 5 files changed, 181 insertions(+), 63 deletions(-) diff --git a/crates/manager-core/src/apply_service.rs b/crates/manager-core/src/apply_service.rs index 0885f11..07383f9 100644 --- a/crates/manager-core/src/apply_service.rs +++ b/crates/manager-core/src/apply_service.rs @@ -88,14 +88,30 @@ pub struct Bundle { /// optional default body is a co-located `kind = module` script. #[serde(default)] pub extension_points: Vec, - /// Declared shared group-collection *names* (§11.6) — KV collections this - /// node offers as cross-app-shared. Name-only, like `extension_points`; the - /// data lives in `group_kv_entries`. Authored on `[group]` nodes only (the - /// CLI rejects it on `[app]`); the apply engine stays owner-generic. + /// Declared shared group collections (§11.6) — `(name, kind)` markers this + /// node offers as cross-app-shared (`kind` ∈ `kv`/`docs`). The data lives in + /// the per-kind store (`group_kv_entries` / `group_docs`). Authored on + /// `[group]` nodes only (the CLI rejects it on `[app]`). #[serde(default)] - pub collections: Vec, + pub collections: Vec, } +/// One declared shared-collection marker on the wire: a name + its store kind. +#[derive(Debug, Clone, Deserialize)] +pub struct CollectionSpec { + pub name: String, + /// Storage kind; defaults to `kv` for back-compat with the name-only form. + #[serde(default = "default_collection_kind")] + pub kind: String, +} + +fn default_collection_kind() -> String { + "kv".to_string() +} + +/// The shared-collection kinds the apply engine accepts. +const COLLECTION_KINDS: &[&str] = &["kv", "docs"]; + #[derive(Debug, Clone, Deserialize)] pub struct BundleScript { pub name: String, @@ -422,8 +438,9 @@ pub struct CurrentState { pub vars: Vec<(String, serde_json::Value)>, /// Extension-point marker names declared directly at this node (§5.5). pub extension_point_names: Vec, - /// Shared group-collection names declared directly at this node (§11.6). - pub collection_names: Vec, + /// Shared group collections declared directly at this node (§11.6), as + /// `(name, kind)` pairs. + pub collections: Vec<(String, String)>, } /// One row of the read-only extension-point report (§5.5). @@ -441,6 +458,7 @@ pub struct ExtensionPointInfo { #[derive(Debug, Clone, Serialize, Deserialize)] pub struct CollectionInfo { pub name: String, + pub kind: String, } // ---------------------------------------------------------------------------- @@ -846,11 +864,12 @@ impl ApplyService { // (that dies only with the owning group). for ch in &plan.collections { if ch.op == Op::Create { + let kind = ch.detail.as_deref().unwrap_or("kv"); crate::group_collection_repo::insert_collection_tx( &mut *tx, owner.as_script_owner(), &ch.key, - "kv", + kind, ) .await .map_err(|e| ApplyError::Backend(e.to_string()))?; @@ -953,11 +972,12 @@ impl ApplyService { // owning group is deleted. for ch in &plan.collections { if ch.op == Op::Delete { + let kind = ch.detail.as_deref().unwrap_or("kv"); crate::group_collection_repo::delete_collection_tx( &mut *tx, owner.as_script_owner(), &ch.key, - "kv", + kind, ) .await .map_err(|e| ApplyError::Backend(e.to_string()))?; @@ -1793,21 +1813,21 @@ impl ApplyService { } } - /// Read-only §11.6 shared-collection report for a node: the shared KV - /// collection names declared directly at it. Group-only in practice (the - /// CLI rejects app-declared collections); an app node returns its own - /// (degenerate) declarations, if any. Backs `pic collections ls`. + /// Read-only §11.6 shared-collection report for a node: the shared + /// collections (`name` + `kind`) declared directly at it. Group-only in + /// practice (the CLI rejects app-declared collections). Backs + /// `pic collections ls`. pub async fn collection_report( &self, owner: ApplyOwner, ) -> Result, ApplyError> { - let names = - crate::group_collection_repo::list_for_owner(&self.pool, owner.as_script_owner(), "kv") + let rows = + crate::group_collection_repo::list_all_for_owner(&self.pool, owner.as_script_owner()) .await .map_err(|e| ApplyError::Backend(e.to_string()))?; - Ok(names + Ok(rows .into_iter() - .map(|name| CollectionInfo { name }) + .map(|(name, kind)| CollectionInfo { name, kind }) .collect()) } @@ -2008,19 +2028,26 @@ impl ApplyService { } } - // Shared group collections (§11.6): unique names. No reserved-name - // guard — a collection name is data namespace, not an importable - // module, so it can't shadow an SDK namespace. - let mut collection_names: HashSet = HashSet::new(); - for name in &bundle.collections { - if name.is_empty() { + // Shared group collections (§11.6): a known kind + unique (name, kind). + // No reserved-name guard — a collection name is a data namespace, not an + // importable module, so it can't shadow an SDK namespace. + let mut seen_collections: HashSet<(String, &str)> = HashSet::new(); + for c in &bundle.collections { + if c.name.is_empty() { return Err(ApplyError::Invalid( "shared collection name must not be empty".into(), )); } - if !collection_names.insert(name.to_lowercase()) { + if !COLLECTION_KINDS.contains(&c.kind.as_str()) { return Err(ApplyError::Invalid(format!( - "duplicate shared collection `{name}`" + "shared collection `{}`: unknown kind `{}` (want one of {COLLECTION_KINDS:?})", + c.name, c.kind + ))); + } + if !seen_collections.insert((c.name.to_lowercase(), c.kind.as_str())) { + return Err(ApplyError::Invalid(format!( + "duplicate shared collection `{}` (kind `{}`)", + c.name, c.kind ))); } } @@ -2076,9 +2103,10 @@ impl ApplyService { crate::extension_point_repo::list_for_owner(&self.pool, owner.as_script_owner()) .await .map_err(|e| ApplyError::Backend(e.to_string()))?; - // Shared group-collection markers declared directly at this node (§11.6). - let collection_names = - crate::group_collection_repo::list_for_owner(&self.pool, owner.as_script_owner(), "kv") + // Shared group-collection markers declared directly at this node + // (§11.6), all kinds. + let collections = + crate::group_collection_repo::list_all_for_owner(&self.pool, owner.as_script_owner()) .await .map_err(|e| ApplyError::Backend(e.to_string()))?; Ok(CurrentState { @@ -2088,7 +2116,7 @@ impl ApplyService { secret_names, vars, extension_point_names, - collection_names, + collections, }) } @@ -2669,36 +2697,42 @@ fn diff_extension_points(current: &CurrentState, bundle: &Bundle) -> Vec Vec { - let live: HashSet<&str> = current - .collection_names + let live: HashSet<(String, &str)> = current + .collections .iter() - .map(String::as_str) + .map(|(n, k)| (n.to_lowercase(), k.as_str())) + .collect(); + let declared: HashSet<(String, &str)> = bundle + .collections + .iter() + .map(|c| (c.name.to_lowercase(), c.kind.as_str())) .collect(); - let declared: HashSet<&str> = bundle.collections.iter().map(String::as_str).collect(); let mut out = Vec::new(); - for name in &bundle.collections { + for c in &bundle.collections { out.push(ResourceChange { - op: if live.contains(name.as_str()) { + op: if live.contains(&(c.name.to_lowercase(), c.kind.as_str())) { Op::NoOp } else { Op::Create }, - key: name.clone(), - detail: None, + key: c.name.clone(), + detail: Some(c.kind.clone()), }); } - for name in ¤t.collection_names { - if !declared.contains(name.as_str()) { + for (name, kind) in ¤t.collections { + if !declared.contains(&(name.to_lowercase(), kind.as_str())) { out.push(ResourceChange { op: Op::Delete, key: name.clone(), - detail: Some("on server, not declared".into()), + detail: Some(kind.clone()), }); } } @@ -3170,7 +3204,7 @@ fn state_token_with_names( + current.secret_names.len() + current.vars.len() + current.extension_point_names.len() - + current.collection_names.len(), + + current.collections.len(), ); for s in ¤t.scripts { parts.push(format!( @@ -3229,9 +3263,10 @@ fn state_token_with_names( for n in ¤t.extension_point_names { parts.push(format!("ep|{n}")); } - // Shared group-collection markers are name-only too (§11.6). - for n in ¤t.collection_names { - parts.push(format!("coll|{n}")); + // Shared group-collection markers carry a kind (§11.6), so a kind change + // moves the token. + for (name, kind) in ¤t.collections { + parts.push(format!("coll|{kind}|{name}")); } // Order-independent: sort the per-resource tokens before hashing. parts.sort_unstable(); diff --git a/crates/picloud-cli/src/client.rs b/crates/picloud-cli/src/client.rs index cabbb44..718f4ad 100644 --- a/crates/picloud-cli/src/client.rs +++ b/crates/picloud-cli/src/client.rs @@ -1352,6 +1352,8 @@ impl Client { #[derive(Debug, Deserialize)] pub struct CollectionInfoDto { pub name: String, + #[serde(default)] + pub kind: String, } // ---------- DTOs (CLI-local, wire-shape-matched) ---------- diff --git a/crates/picloud-cli/src/cmds/collections.rs b/crates/picloud-cli/src/cmds/collections.rs index 23531fb..48ab4f2 100644 --- a/crates/picloud-cli/src/cmds/collections.rs +++ b/crates/picloud-cli/src/cmds/collections.rs @@ -14,9 +14,9 @@ pub async fn ls(group: &str, mode: OutputMode) -> Result<()> { let client = Client::from_creds(&creds)?; let items = client.collections_list(group).await?; - let mut table = Table::new(["name"]); + let mut table = Table::new(["name", "kind"]); for c in &items { - table.row([c.name.clone()]); + table.row([c.name.clone(), c.kind.clone()]); } table.print(mode); Ok(()) diff --git a/crates/picloud-cli/src/cmds/plan.rs b/crates/picloud-cli/src/cmds/plan.rs index e814e14..d71de6b 100644 --- a/crates/picloud-cli/src/cmds/plan.rs +++ b/crates/picloud-cli/src/cmds/plan.rs @@ -163,7 +163,11 @@ pub fn build_bundle(manifest: &Manifest, base_dir: &Path) -> Result { "secrets": manifest.secrets.names, "vars": vars, "extension_points": manifest.extension_points(), - "collections": manifest.collections(), + "collections": manifest + .collections() + .into_iter() + .map(|(name, kind)| json!({ "name": name, "kind": kind })) + .collect::>(), })) } diff --git a/crates/picloud-cli/src/manifest.rs b/crates/picloud-cli/src/manifest.rs index cece35d..89ba1d9 100644 --- a/crates/picloud-cli/src/manifest.rs +++ b/crates/picloud-cli/src/manifest.rs @@ -111,14 +111,19 @@ impl Manifest { } } - /// This node's declared shared group-collection names (§11.6). Authored on - /// `[group]` nodes only — an `[app]` manifest carrying `collections` is a - /// hard parse error (`ManifestApp` has no such field + `deny_unknown_fields`). + /// This node's declared shared group collections (§11.6), normalized to + /// `(name, kind)` pairs. Authored on `[group]` nodes only — an `[app]` + /// manifest carrying `collections` is a hard parse error (`ManifestApp` has + /// no such field + `deny_unknown_fields`). #[must_use] - pub fn collections(&self) -> &[String] { + pub fn collections(&self) -> Vec<(String, String)> { match &self.group { - Some(g) => &g.collections, - None => &[], + Some(g) => g + .collections + .iter() + .map(CollectionDecl::normalized) + .collect(), + None => Vec::new(), } } @@ -249,13 +254,60 @@ pub struct ManifestGroup { /// See [`ManifestApp::extension_points`]. A key of the `[group]` table. #[serde(default, skip_serializing_if = "Vec::is_empty")] pub extension_points: Vec, - /// `collections = ["catalog", …]` (§11.6) — shared KV collection names this - /// group offers as cross-app-shared (read by any app in the subtree, - /// written by an authenticated editor+). Group-only: there is no - /// `collections` key on `[app]`. The data lives in `group_kv_entries`; - /// pruning a name removes the declaration, not the data. + /// `collections = [...]` (§11.6) — shared collections this group offers as + /// cross-app-shared (read by any app in the subtree, written by an + /// authenticated editor+). Each entry is either a bare string (a `kv` + /// collection) or a `{ name, kind }` table (`kind` ∈ `kv`/`docs`). + /// Group-only: there is no `collections` key on `[app]`. #[serde(default, skip_serializing_if = "Vec::is_empty")] - pub collections: Vec, + pub collections: Vec, +} + +/// The store kind of a shared collection (§11.6). +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "lowercase")] +pub enum CollectionKind { + Kv, + Docs, +} + +impl CollectionKind { + #[must_use] + pub fn as_str(self) -> &'static str { + match self { + Self::Kv => "kv", + Self::Docs => "docs", + } + } +} + +/// One `collections` entry: a bare string (`kv` shorthand) or a `{ name, kind }` +/// table. Untagged so the shipped `collections = ["catalog"]` form keeps working +/// alongside `{ name = "articles", kind = "docs" }`. +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +#[serde(untagged)] +pub enum CollectionDecl { + Name(String), + Full { + name: String, + #[serde(default = "kv_kind")] + kind: CollectionKind, + }, +} + +fn kv_kind() -> CollectionKind { + CollectionKind::Kv +} + +impl CollectionDecl { + /// `(name, kind-as-string)` — a bare string normalizes to `kind = "kv"`. + #[must_use] + pub fn normalized(&self) -> (String, String) { + match self { + Self::Name(n) => (n.clone(), "kv".to_string()), + Self::Full { name, kind } => (name.clone(), kind.as_str().to_string()), + } + } } #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] @@ -668,6 +720,31 @@ mod tests { assert_eq!(m.extension_points(), ["theme"]); } + #[test] + fn collections_string_or_table_normalizes_kind() { + // §11.6: a bare string is a `kv` collection (the shipped form); a + // `{ name, kind }` table sets an explicit kind. Both coexist. + let m = Manifest::parse( + "[group]\nslug = \"acme\"\nname = \"ACME\"\n\n\ + collections = [\"catalog\", { name = \"articles\", kind = \"docs\" }]\n", + ) + .expect("string-or-table collections must parse"); + assert_eq!( + m.collections(), + vec![ + ("catalog".to_string(), "kv".to_string()), + ("articles".to_string(), "docs".to_string()), + ] + ); + // An app manifest carrying `collections` is rejected (no such field + + // deny_unknown_fields). + let app = "[app]\nslug = \"x\"\nname = \"X\"\ncollections = [\"catalog\"]\n"; + assert!( + Manifest::parse(app).is_err(), + "collections on [app] must be rejected" + ); + } + #[test] fn group_manifest_parses_and_rejects_app_only_blocks() { // A [group] node: scripts + vars, no [app].