diff --git a/crates/manager-core/src/apply_service.rs b/crates/manager-core/src/apply_service.rs index 6fc70e6..1f46374 100644 --- a/crates/manager-core/src/apply_service.rs +++ b/crates/manager-core/src/apply_service.rs @@ -2111,8 +2111,8 @@ impl ApplyService { for r in &bundle.routes { reject_reserved_path(&r.path)?; // Structural validity (parity with the interactive route API): - // a route whose path/host doesn't parse would otherwise be - // written and then silently dropped by `compile_routes`. + // a route whose path/host doesn't parse would otherwise be written + // and then silently dropped by the route-table compile. pattern::parse_path(r.path_kind, &r.path) .map_err(|e| ApplyError::Invalid(format!("route path `{}`: {e}", r.path)))?; pattern::parse_host(r.host_kind, &r.host, r.host_param_name.as_deref()) diff --git a/crates/manager-core/src/lib.rs b/crates/manager-core/src/lib.rs index a787bcf..a2e9c97 100644 --- a/crates/manager-core/src/lib.rs +++ b/crates/manager-core/src/lib.rs @@ -218,7 +218,9 @@ pub use repo::{ ExecutionLogRepository, NewScript, PostgresExecutionLogRepository, PostgresScriptRepository, RepoResolver, ScriptPatch, ScriptRepository, ScriptRepositoryError, }; -pub use route_admin::{compile_routes, rebuild_route_table, route_admin_router, RouteAdminState}; +pub use route_admin::{ + compile_effective_routes, rebuild_route_table, route_admin_router, RouteAdminState, +}; pub use route_repo::{NewRoute, PostgresRouteRepository, RouteRepository}; pub use sandbox::{CeilingError, SandboxCeiling}; pub use secrets_api::{secrets_router, SecretsApiError, SecretsState}; diff --git a/crates/manager-core/src/route_admin.rs b/crates/manager-core/src/route_admin.rs index 50ef8bd..f51ce83 100644 --- a/crates/manager-core/src/route_admin.rs +++ b/crates/manager-core/src/route_admin.rs @@ -411,35 +411,6 @@ pub async fn rebuild_route_table( Ok(()) } -/// Compile stored route rows into the in-memory match table. -/// -/// **Lenient by design (H1).** A row that fails to parse is *skipped with -/// a warning*, not propagated as an error. The motivating case: a path -/// that was valid when created but became reserved under a later, stricter -/// validation (e.g. the case-insensitive reserved-prefix check) — but this -/// also covers any other parse failure. A single un-compilable legacy row -/// must never take down the entire data plane: this function runs at -/// startup (where a hard error aborts boot) and on every table rebuild -/// after a route edit (where it would fail an unrelated CRUD op). A skipped -/// route simply doesn't match; the warning tells the operator to delete or -/// fix it (and migration 0044 sweeps the reserved-path offenders on -/// upgrade). -#[must_use] -pub fn compile_routes(rows: &[Route]) -> Vec { - rows.iter() - // A disabled route (§4.3) is dropped from the match table entirely, so - // a request to it 404s indistinguishably from an absent route. - .filter(|r| r.enabled) - .filter_map(|r| { - // A group-owned route TEMPLATE (`app_id` None) has no single app to - // compile into — it is materialized per descendant by the effective - // rebuild (`compile_effective_routes`), not this app-only path. - let app_id = r.app_id?; - compile_one(r, app_id) - }) - .collect() -} - /// Compile the §11 tail effective expansion into per-app match slices, applying /// **nearest-owner-wins shadowing**: for a given app, if the same binding tuple /// (method + host + path) is owned at more than one chain level — e.g. the app @@ -450,6 +421,18 @@ pub fn compile_routes(rows: &[Route]) -> Vec { /// app's `/users/:id` + the group's `/users/admin`) coexist and the matcher's /// existing precedence picks per request. /// +/// This is the single production entry point: app-only installs feed it routes +/// at `depth 0` (one per app, no templates), so it subsumes the former app-only +/// compile path. Disabled routes are dropped (§4.3) and un-compilable rows +/// skipped-with-warning (see [`compile_one`]). +/// +/// **Disabled + inherited (§4.3 semantic):** the `enabled` filter runs *before* +/// the shadow check, so a **disabled** own-route does NOT claim its binding — an +/// enabled ancestor-group template at the same binding then falls through and +/// serves (disabled = "indistinguishable from absent"). The corollary: a +/// descendant cannot 404 an inherited route by disabling a same-path own route; +/// a true per-app opt-out is deferred (§4.5). +/// /// Requires `rows` ordered by `(effective_app_id, depth ASC)` — which /// [`RouteRepository::list_effective`] guarantees — so first-seen is nearest. #[must_use] @@ -457,6 +440,8 @@ pub fn compile_effective_routes(rows: &[EffectiveRoute]) -> Vec { let mut seen: std::collections::HashSet<(AppId, String)> = std::collections::HashSet::new(); let mut out = Vec::new(); for er in rows { + // A disabled route (§4.3) is dropped from the match table entirely, so a + // request to it 404s indistinguishably from an absent route. if !er.route.enabled { continue; } @@ -495,8 +480,18 @@ fn binding_key(r: &Route) -> String { /// Parse one stored route into a `CompiledRoute` bound to `app_id` (the /// effective app — its own for app routes, the firing descendant for a group -/// template). Lenient: an un-compilable row is skipped-with-warning, never an -/// error (it must not take down the data plane or fail an unrelated rebuild). +/// template). +/// +/// **Lenient by design (H1).** A row that fails to parse is *skipped with a +/// warning*, not propagated as an error. The motivating case: a path that was +/// valid when created but became reserved under a later, stricter validation +/// (the case-insensitive reserved-prefix check) — but this also covers any +/// other parse failure. A single un-compilable row must never take down the +/// data plane: this runs at startup (where a hard error aborts boot) and on +/// every table rebuild after an edit (where it would fail an unrelated CRUD +/// op). A skipped route simply doesn't match; the warning tells the operator to +/// delete or fix it (and migration 0044 sweeps the reserved-path offenders on +/// upgrade). fn compile_one(r: &Route, app_id: AppId) -> Option { match compile_route(r, app_id) { Ok(compiled) => Some(compiled), @@ -734,18 +729,28 @@ mod tests { } } + /// Wrap an app-owned route as its own `depth 0` effective row — the shape + /// `list_effective` produces for an app-only install (no group templates). + fn eff(route: &Route) -> EffectiveRoute { + EffectiveRoute { + effective_app_id: route.app_id.expect("test route is app-owned"), + depth: 0, + route: route.clone(), + } + } + #[test] - fn compile_routes_skips_uncompilable_rows_instead_of_failing() { + fn compile_effective_skips_uncompilable_rows_instead_of_failing() { // H1 regression guard: a stored route whose path is now reserved // (creatable before the case-insensitive reserved-prefix fix) must // be skipped, not abort the whole compile — otherwise one legacy - // row bricks startup (`compile_routes` runs in `build_app`). + // row bricks startup (the effective rebuild runs in `build_app`). let good_a = route_with_path("/ok"); let bad = route_with_path("/API/v2/x"); // now reserved, case-insensitive let good_b = route_with_path("/items"); - let rows = vec![good_a.clone(), bad.clone(), good_b.clone()]; + let rows = [eff(&good_a), eff(&bad), eff(&good_b)]; - let compiled = compile_routes(&rows); + let compiled = compile_effective_routes(&rows); let ids: Vec = compiled.iter().map(|c| c.route_id).collect(); assert_eq!(compiled.len(), 2, "the reserved row must be dropped"); @@ -764,8 +769,53 @@ mod tests { let active = route_with_path("/on"); let mut disabled = route_with_path("/off"); disabled.enabled = false; - let compiled = compile_routes(&[active.clone(), disabled.clone()]); + let compiled = compile_effective_routes(&[eff(&active), eff(&disabled)]); let ids: Vec = compiled.iter().map(|c| c.route_id).collect(); assert_eq!(ids, vec![active.id], "only the enabled route compiles"); } + + #[test] + fn nearest_owner_shadows_identical_binding() { + // An app's own route (depth 0) and an ancestor-group template (depth 1) + // with the SAME binding collapse to the nearer one; a different binding + // coexists. Mirrors the live `group_route_templates` integration test + // at the pure-compile layer. + let app = AppId::from(Uuid::new_v4()); + let own = route_with_path("/x"); // app's own /x + let mut template = route_with_path("/x"); // group template, same binding + template.app_id = None; + template.group_id = Some(picloud_shared::GroupId::from(Uuid::new_v4())); + let other_template = route_with_path("/y"); // group template, different path + + let rows = [ + EffectiveRoute { + effective_app_id: app, + depth: 0, + route: own.clone(), + }, + EffectiveRoute { + effective_app_id: app, + depth: 1, + route: template.clone(), + }, + EffectiveRoute { + effective_app_id: app, + depth: 1, + route: other_template.clone(), + }, + ]; + let ids: Vec = compile_effective_routes(&rows) + .iter() + .map(|c| c.route_id) + .collect(); + assert!(ids.contains(&own.id), "the app's own /x must win"); + assert!( + !ids.contains(&template.id), + "the inherited /x template must be shadowed by the app's own" + ); + assert!( + ids.contains(&other_template.id), + "a non-identical /y template must coexist" + ); + } } diff --git a/docs/design/groups-and-project-tool.md b/docs/design/groups-and-project-tool.md index e18e289..43e4b0e 100644 --- a/docs/design/groups-and-project-tool.md +++ b/docs/design/groups-and-project-tool.md @@ -414,8 +414,12 @@ Two distinct constraints: > reparent (`groups_api`) all route through the single `rebuild_route_table` chokepoint, so a new app > under a group serves its templates instantly. Host-claim validation is skipped for a group template > (no single app; descendants serve it on whatever host they claim, so templates use `host_kind = any`). -> Authoring is `[[routes]]` on a `[group]`; read-only `pic routes ls --group`. **Deferred:** per-app -> route opt-out, and multi-node route-snapshot propagation (cluster mode). +> Authoring is `[[routes]]` on a `[group]`; read-only `pic routes ls --group`. **Disabled semantic +> (§4.3):** the shadow check runs *after* the `enabled` filter, so a **disabled** own-route does not +> claim its binding — an enabled ancestor template at the same path then falls through and serves +> ("disabled = absent"). The corollary: a descendant can't 404 an inherited route by disabling a +> same-path own route. **Deferred:** per-app route opt-out (the true way to decline an inherited +> route), and multi-node route-snapshot propagation (cluster mode). ### 4.6 Secrets & `pull`