fix(apply): preserve project name on a name-less re-apply
`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) <noreply@anthropic.com>
This commit is contained in:
@@ -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.
|
/// 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
|
/// First apply with a new slug inserts, falling back to the slug when the
|
||||||
/// (the manifest is authoritative) but preserves the original `created_by`.
|
/// 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(
|
async fn upsert_project_tx(
|
||||||
tx: &mut sqlx::Transaction<'_, sqlx::Postgres>,
|
tx: &mut sqlx::Transaction<'_, sqlx::Postgres>,
|
||||||
decl: &ProjectDecl,
|
decl: &ProjectDecl,
|
||||||
actor: AdminUserId,
|
actor: AdminUserId,
|
||||||
) -> Result<ProjectId, ApplyError> {
|
) -> Result<ProjectId, ApplyError> {
|
||||||
validate_project_slug(&decl.slug)?;
|
validate_project_slug(&decl.slug)?;
|
||||||
let name = decl.name.clone().unwrap_or_else(|| decl.slug.clone());
|
|
||||||
let (id,): (uuid::Uuid,) = sqlx::query_as(
|
let (id,): (uuid::Uuid,) = sqlx::query_as(
|
||||||
"INSERT INTO projects (slug, name, created_by) VALUES ($1, $2, $3) \
|
"INSERT INTO projects (slug, name, created_by) VALUES ($1, COALESCE($2, $1), $3) \
|
||||||
ON CONFLICT (slug) DO UPDATE SET name = EXCLUDED.name \
|
ON CONFLICT (slug) DO UPDATE SET name = COALESCE($2, projects.name) \
|
||||||
RETURNING id",
|
RETURNING id",
|
||||||
)
|
)
|
||||||
.bind(&decl.slug)
|
.bind(&decl.slug)
|
||||||
.bind(&name)
|
.bind(decl.name.clone())
|
||||||
.bind(actor.into_inner())
|
.bind(actor.into_inner())
|
||||||
.fetch_one(&mut **tx)
|
.fetch_one(&mut **tx)
|
||||||
.await
|
.await
|
||||||
|
|||||||
@@ -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
|
/// §6/§7 M2 — `[project] parent_group` is the ceiling: applies are refused for
|
||||||
/// any node not strictly within the attach point's subtree.
|
/// any node not strictly within the attach point's subtree.
|
||||||
#[ignore = "needs DATABASE_URL pointing at a running Postgres"]
|
#[ignore = "needs DATABASE_URL pointing at a running Postgres"]
|
||||||
|
|||||||
Reference in New Issue
Block a user