From 74f7a67be79b31e310131b3a1e33981ff6243bd2 Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Tue, 7 Jul 2026 07:37:25 +0200 Subject: [PATCH] fix(apply): preserve project name on a name-less re-apply MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `upsert_project_tx` defaulted a missing `[project].name` to the slug and then `ON CONFLICT (slug) DO UPDATE SET name = EXCLUDED.name` wrote that fabricated value unconditionally — so a re-apply from a clone whose manifest omits `name` silently clobbered the stored display name back to the slug (visible in `pic projects ls` / `pic groups ls`). Bind the raw optional name once and guard both sides on it: `VALUES ($1, COALESCE($2, $1), $3)` (first apply falls back to the slug for the NOT NULL column) and `DO UPDATE SET name = COALESCE($2, projects.name)` (a re-apply updates the name only when the manifest actually declares one). An omitted optional field now preserves persisted data instead of mutating it. Pinned by a new `reapply_without_name_preserves_project_name` journey. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/manager-core/src/apply_service.rs | 16 +++-- crates/picloud-cli/tests/apply_ownership.rs | 67 +++++++++++++++++++++ 2 files changed, 77 insertions(+), 6 deletions(-) diff --git a/crates/manager-core/src/apply_service.rs b/crates/manager-core/src/apply_service.rs index d9d7c82..e61f741 100644 --- a/crates/manager-core/src/apply_service.rs +++ b/crates/manager-core/src/apply_service.rs @@ -3430,22 +3430,26 @@ fn validate_project_slug(slug: &str) -> Result<(), ApplyError> { } /// Register (or fetch) a project by slug inside the apply tx, returning its id. -/// First apply with a new slug inserts; a re-apply updates the display name -/// (the manifest is authoritative) but preserves the original `created_by`. +/// First apply with a new slug inserts, falling back to the slug when the +/// manifest omits `name` (the column is NOT NULL). A re-apply updates the +/// display name ONLY when the manifest actually declares one — a name-less +/// `[project]` block (e.g. a clone scaffolded without a name) preserves the +/// previously-set name and `created_by` rather than clobbering the name back +/// to the slug. `$2` (the optional declared name) is bound once and referenced +/// in both the INSERT fallback and the ON CONFLICT guard. async fn upsert_project_tx( tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, decl: &ProjectDecl, actor: AdminUserId, ) -> Result { validate_project_slug(&decl.slug)?; - let name = decl.name.clone().unwrap_or_else(|| decl.slug.clone()); let (id,): (uuid::Uuid,) = sqlx::query_as( - "INSERT INTO projects (slug, name, created_by) VALUES ($1, $2, $3) \ - ON CONFLICT (slug) DO UPDATE SET name = EXCLUDED.name \ + "INSERT INTO projects (slug, name, created_by) VALUES ($1, COALESCE($2, $1), $3) \ + ON CONFLICT (slug) DO UPDATE SET name = COALESCE($2, projects.name) \ RETURNING id", ) .bind(&decl.slug) - .bind(&name) + .bind(decl.name.clone()) .bind(actor.into_inner()) .fetch_one(&mut **tx) .await diff --git a/crates/picloud-cli/tests/apply_ownership.rs b/crates/picloud-cli/tests/apply_ownership.rs index fcefc30..9003b4f 100644 --- a/crates/picloud-cli/tests/apply_ownership.rs +++ b/crates/picloud-cli/tests/apply_ownership.rs @@ -215,6 +215,73 @@ fn claim_conflict_takeover_and_app_inheritance() { ); } +/// The `name` cell (index 1: slug, NAME, owned_groups, created_at) for `project` +/// in `pic projects ls`. +fn project_name_cell(env: &common::TestEnv, project: &str) -> String { + let ls = common::pic_as(env) + .args(["projects", "ls"]) + .output() + .expect("projects ls"); + let table = String::from_utf8(ls.stdout).unwrap(); + table + .lines() + .map(common::cells) + .find(|c| c.first() == Some(&project)) + .and_then(|c| c.get(1).map(|s| (*s).to_string())) + .unwrap_or_else(|| panic!("project `{project}` not in projects ls:\n{table}")) +} + +/// A name-less `[project]` re-apply must PRESERVE the display name set on the +/// first apply — an omitted optional field never clobbers the stored name back +/// to the slug (regression for the `ON CONFLICT DO UPDATE SET name` path). +#[ignore = "needs DATABASE_URL pointing at a running Postgres"] +#[test] +fn reapply_without_name_preserves_project_name() { + let Some(fx) = common::fixture_or_skip() else { + return; + }; + let env = common::admin_env(fx); + let group = common::unique_slug("name-grp"); + let proj = common::unique_slug("name-proj"); + let _g = GroupGuard::new(&env.url, &env.token, &group); + common::pic_as(&env) + .args(["groups", "create", &group]) + .assert() + .success(); + + let dir = manifest_dir(); + let apply = |m: &str| { + fs::write(dir.path().join("picloud.toml"), m).unwrap(); + common::pic_as(&env) + .args(["apply", "--file"]) + .arg(dir.path().join("picloud.toml")) + .assert() + .success(); + }; + + // First apply declares a display name. + apply(&format!( + "[project]\nslug = \"{proj}\"\nname = \"Acme Platform\"\n\n\ + [group]\nslug = \"{group}\"\nname = \"Grp\"\n" + )); + assert_eq!( + project_name_cell(&env, &proj), + "Acme Platform", + "the first apply records the declared name" + ); + + // A re-apply that OMITS `name` must not overwrite it with the slug. + apply(&format!( + "[project]\nslug = \"{proj}\"\n\n\ + [group]\nslug = \"{group}\"\nname = \"Grp\"\n" + )); + assert_eq!( + project_name_cell(&env, &proj), + "Acme Platform", + "a name-less re-apply must preserve the stored name, not clobber it to the slug" + ); +} + /// §6/§7 M2 — `[project] parent_group` is the ceiling: applies are refused for /// any node not strictly within the attach point's subtree. #[ignore = "needs DATABASE_URL pointing at a running Postgres"]