From 7c17d6e363324af76424c62ca3239e3fc1cffcfc Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Sun, 28 Jun 2026 18:11:44 +0200 Subject: [PATCH] feat(hierarchies): template {var:NAME} resolves in-transaction (M6) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Route/trigger template `{var:NAME}` placeholders now resolve against vars applied in the SAME `pic apply` transaction, not committed-only state — so a var and a template referencing it converge in one apply instead of two. - config_resolver: add tx-scoped `fetch_var_candidates_tx`, sharing the SQL tail (`VAR_CANDIDATES_TAIL`) with the pool variant so they can't drift. - apply_service: `expand_route_templates_tx`/`expand_trigger_templates_tx` read vars via the tx (vars are reconciled in Phase A / the app's own reconcile, both before Phase B expansion). - templates journey: `route_template_var_resolves_in_same_apply` — would fail against the old committed-only read. - doc §4.5: strike the `{var:NAME}` committed-only limitation. Co-Authored-By: Claude Opus 4.8 --- crates/manager-core/src/apply_service.rs | 12 ++-- crates/manager-core/src/config_resolver.rs | 45 ++++++++++---- crates/picloud-cli/tests/templates.rs | 72 ++++++++++++++++++++++ docs/design/groups-and-project-tool.md | 5 +- 4 files changed, 116 insertions(+), 18 deletions(-) diff --git a/crates/manager-core/src/apply_service.rs b/crates/manager-core/src/apply_service.rs index 27183ee..68da18f 100644 --- a/crates/manager-core/src/apply_service.rs +++ b/crates/manager-core/src/apply_service.rs @@ -2184,10 +2184,11 @@ impl ApplyService { } } - // Effective vars for `{var:NAME}` — committed state (a var set in this - // same apply is visible to templates on the NEXT apply, not this one). + // Effective vars for `{var:NAME}` — read in-transaction so a var set in + // THIS same apply (reconciled earlier in the tx) is visible to the + // template now, not one apply late. let vars = { - let cands = crate::config_resolver::fetch_var_candidates(&self.pool, app_id) + let cands = crate::config_resolver::fetch_var_candidates_tx(tx, app_id) .await .map_err(|e| ApplyError::Backend(e.to_string()))?; crate::config_resolver::resolve(cands).0 @@ -2428,9 +2429,10 @@ impl ApplyService { } } - // Effective vars for `{var:NAME}` (committed state — see route expansion). + // Effective vars for `{var:NAME}` — read in-transaction (see route + // expansion): a var set in this same apply is visible now. let vars = { - let cands = crate::config_resolver::fetch_var_candidates(&self.pool, app_id) + let cands = crate::config_resolver::fetch_var_candidates_tx(tx, app_id) .await .map_err(|e| ApplyError::Backend(e.to_string()))?; crate::config_resolver::resolve(cands).0 diff --git a/crates/manager-core/src/config_resolver.rs b/crates/manager-core/src/config_resolver.rs index 42403c6..868f057 100644 --- a/crates/manager-core/src/config_resolver.rs +++ b/crates/manager-core/src/config_resolver.rs @@ -226,17 +226,7 @@ pub async fn fetch_var_candidates( pool: &PgPool, app_id: AppId, ) -> Result, sqlx::Error> { - let sql = format!( - "{CHAIN_LEVELS_CTE} \ - SELECT c.depth, \ - CASE WHEN v.app_id IS NOT NULL THEN 'app' ELSE 'group' END AS owner_kind, \ - COALESCE(v.app_id, v.group_id) AS owner_id, \ - v.environment_scope, v.key, v.value, v.is_tombstone \ - FROM chain c \ - JOIN vars v ON (v.app_id = c.app_owner OR v.group_id = c.group_owner) \ - WHERE v.environment_scope = '*' OR v.environment_scope = c.app_env \ - ORDER BY c.depth ASC" - ); + let sql = format!("{CHAIN_LEVELS_CTE} {VAR_CANDIDATES_TAIL}"); let rows = sqlx::query_as::<_, VarCandidateRow>(&sql) .bind(app_id.into_inner()) .fetch_all(pool) @@ -244,6 +234,39 @@ pub async fn fetch_var_candidates( Ok(rows.into_iter().map(Into::into).collect()) } +/// Transaction-scoped twin of [`fetch_var_candidates`]: runs the identical +/// `CHAIN_LEVELS_CTE` query against an open transaction, so it sees vars +/// **written earlier in the same transaction** (not just committed state). +/// Used by template expansion so a `{var:NAME}` placeholder resolves against +/// a var set in the *same* `pic apply` rather than landing one apply late. +/// +/// # Errors +/// Propagates sqlx errors. +pub async fn fetch_var_candidates_tx( + tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, + app_id: AppId, +) -> Result, sqlx::Error> { + let sql = format!("{CHAIN_LEVELS_CTE} {VAR_CANDIDATES_TAIL}"); + let rows = sqlx::query_as::<_, VarCandidateRow>(&sql) + .bind(app_id.into_inner()) + .fetch_all(&mut **tx) + .await?; + Ok(rows.into_iter().map(Into::into).collect()) +} + +/// The `SELECT … FROM chain …` tail appended after [`CHAIN_LEVELS_CTE`] to +/// pull env-eligible `vars` candidates, nearest-first. Shared by the pool and +/// transaction variants so the two can't drift. +const VAR_CANDIDATES_TAIL: &str = "\ + SELECT c.depth, \ + CASE WHEN v.app_id IS NOT NULL THEN 'app' ELSE 'group' END AS owner_kind, \ + COALESCE(v.app_id, v.group_id) AS owner_id, \ + v.environment_scope, v.key, v.value, v.is_tombstone \ + FROM chain c \ + JOIN vars v ON (v.app_id = c.app_owner OR v.group_id = c.group_owner) \ + WHERE v.environment_scope = '*' OR v.environment_scope = c.app_env \ + ORDER BY c.depth ASC"; + /// The masked, resolved view of one inherited secret for `config/effective`: /// which owner/level/scope supplies it — **never** the value. #[derive(Debug, Clone)] diff --git a/crates/picloud-cli/tests/templates.rs b/crates/picloud-cli/tests/templates.rs index 19ca904..d503b01 100644 --- a/crates/picloud-cli/tests/templates.rs +++ b/crates/picloud-cli/tests/templates.rs @@ -159,6 +159,78 @@ fn route_template_fans_out_per_app_idempotently_then_prunes() { ); } +/// M6 (§4.5): a `{var:NAME}` placeholder resolves against a var set in the SAME +/// apply (in-transaction), so a var and a template referencing it converge in +/// one `pic apply`. Before M6 the var (written earlier in the tx) was invisible +/// to expansion — the route would only pick it up on a *second* apply. +#[ignore = "needs DATABASE_URL pointing at a running Postgres"] +#[test] +fn route_template_var_resolves_in_same_apply() { + let Some(fx) = common::fixture_or_skip() else { + return; + }; + let env = common::admin_env(fx); + let group = common::unique_slug("tplv-g"); + let a = common::unique_slug("tplv-a"); + + let _g = GroupGuard::new(&env.url, &env.token, &group); + common::pic_as(&env) + .args(["groups", "create", &group]) + .assert() + .success(); + let _aa = AppGuard::new(&env.url, &env.token, &a); + common::pic_as(&env) + .args(["apps", "create", &a, "--group", &group]) + .assert() + .success(); + + // The group sets `region` AND declares a template referencing it — one + // manifest, one apply. + let dir = TempDir::new().expect("tempdir"); + fs::create_dir_all(dir.path().join("scripts")).unwrap(); + fs::write(dir.path().join("scripts/shared.rhai"), r#""hi""#).unwrap(); + fs::write( + dir.path().join("picloud.toml"), + format!( + "[group]\nslug = \"{group}\"\nname = \"G\"\n\n\ + [vars]\nregion = \"eu\"\n\n\ + [[scripts]]\nname = \"shared\"\nfile = \"scripts/shared.rhai\"\n\n\ + [[route_templates]]\nname = \"geo\"\nscript = \"shared\"\n\ + host_kind = \"any\"\npath_kind = \"exact\"\npath = \"/tv/{{app_slug}}/{{var:region}}\"\n" + ), + ) + .unwrap(); + fs::create_dir_all(dir.path().join(&a)).unwrap(); + fs::write( + dir.path().join(&a).join("picloud.toml"), + format!("[app]\nslug = \"{a}\"\nname = \"App\"\n"), + ) + .unwrap(); + + common::pic_as(&env) + .args(["apply", "--dir"]) + .arg(dir.path()) + .assert() + .success(); + let _gs = ScriptGuard::new(&env.url, &env.token, &group_script_id(&env, &group)); + + // The expanded route resolved `{var:region}` → `eu` in THIS apply. + let url = std::env::var("DATABASE_URL").expect("DATABASE_URL"); + let mut pg = PgClient::connect(&url, NoTls).expect("pg connect"); + let want = format!("/tv/{a}/eu"); + let rows = pg + .query( + "SELECT path FROM routes WHERE from_template IS NOT NULL AND path = $1", + &[&want], + ) + .expect("query"); + assert_eq!( + rows.len(), + 1, + "`{{var:region}}` must resolve to `eu` in the same apply (expected path {want})" + ); +} + #[ignore = "needs DATABASE_URL pointing at a running Postgres"] #[test] fn expansion_colliding_with_hand_declared_route_is_rejected() { diff --git a/docs/design/groups-and-project-tool.md b/docs/design/groups-and-project-tool.md index 331db11..b87b963 100644 --- a/docs/design/groups-and-project-tool.md +++ b/docs/design/groups-and-project-tool.md @@ -404,8 +404,9 @@ Two distinct constraints: > check (cron schedule, queue timeout, …) — so a `{var:…}`-injected reserved path, unclaimed strict > host, or malformed schedule/timeout can't slip in. **Scope:** expansion targets app NODES present in the tree apply > (the common `pic apply --dir` case), not yet every descendant in another repo — deleting a template -> leaves expansions in out-of-apply descendants until they're re-applied (the apply warns); -> `{var:NAME}` resolves against *committed* vars (a var set in the same apply lands next apply). A +> leaves expansions in out-of-apply descendants until they're re-applied (the apply warns). +> `{var:NAME}` resolves **in-transaction** (M6, 2026-06-28) — a var set in the *same* apply is visible to +> the template immediately, so var + template converge in one apply. A > trigger template whose only change is a non-identity field (e.g. dispatch_mode) is a no-op on > re-apply, mirroring the existing trigger-identity limitation (recreate to change those).