diff --git a/src/handlers/feed.rs b/src/handlers/feed.rs index 4c37010a..adf3a4ac 100644 --- a/src/handlers/feed.rs +++ b/src/handlers/feed.rs @@ -70,7 +70,9 @@ pub async fn get_feed( Some(name) if !name.is_empty() => { // Accept either canonical hash or descriptive_name let canonical = - crate::handlers::schema_resolution::resolve_schema_name(&processor, name).await?; + crate::handlers::schema_resolution::resolve_schema_name(&processor, name) + .await? + .into_canonical_or_err()?; vec![canonical] } _ => { diff --git a/src/handlers/mutation.rs b/src/handlers/mutation.rs index aa1e61d3..8963c46e 100644 --- a/src/handlers/mutation.rs +++ b/src/handlers/mutation.rs @@ -62,8 +62,9 @@ pub async fn execute_mutation_from_components( let processor = OperationProcessor::from_ref(node); let caller_pub_key = current_caller_pubkey(node); - let schema = - crate::handlers::schema_resolution::resolve_schema_name(&processor, &schema).await?; + let schema = crate::handlers::schema_resolution::resolve_schema_name(&processor, &schema) + .await? + .into_canonical_or_err()?; let mutation = Mutation::new( schema, diff --git a/src/handlers/query.rs b/src/handlers/query.rs index ef875acb..bbe68bad 100644 --- a/src/handlers/query.rs +++ b/src/handlers/query.rs @@ -90,7 +90,8 @@ pub async fn execute_query( query.schema_name = crate::handlers::schema_resolution::resolve_schema_name(&processor, &query.schema_name) - .await?; + .await? + .into_canonical_or_err()?; let limit = limit.unwrap_or(DEFAULT_QUERY_LIMIT).min(MAX_QUERY_LIMIT); let offset = offset.unwrap_or(0); diff --git a/src/handlers/schema_resolution.rs b/src/handlers/schema_resolution.rs index bf42eb3f..2726c9f0 100644 --- a/src/handlers/schema_resolution.rs +++ b/src/handlers/schema_resolution.rs @@ -8,49 +8,101 @@ //! by its descriptive label fails with "not found as schema or view". //! //! [`resolve_schema_name`] closes that gap: it accepts either form and -//! returns the canonical name. Ambiguous descriptive labels (two schemas -//! sharing the same `descriptive_name`) become a 400 with the matching -//! canonical hashes listed. +//! returns either a single canonical name or — when two or more active +//! schemas share the same `descriptive_name` on a user's machine — surfaces +//! the conflict to the caller so the HTTP layer can map it to a 409 with a +//! structured `{ambiguous_schemas: [...]}` body. Picking arbitrarily +//! would silently route the query at one of several schemas and the +//! caller would have no way to know. use crate::fold_node::OperationProcessor; use crate::handlers::response::{HandlerError, IntoTypedHandlerError}; -/// Resolve `requested` to a canonical runtime schema name. +/// Outcome of resolving a user-supplied schema identifier. +/// +/// `Canonical` covers both the "input was already canonical" and "exactly +/// one descriptive_name match" cases — and also the "no match" pass-through, +/// where we return the input unchanged so the downstream query executor +/// emits its own "not found as schema or view" message rather than us +/// pre-empting it. `Ambiguous` carries every candidate canonical hash so +/// the caller can pin a future query to one of them. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum SchemaResolution { + Canonical(String), + Ambiguous { + input: String, + candidates: Vec, + }, +} + +impl SchemaResolution { + /// Collapse to a single canonical name, mapping `Ambiguous` to a 400 + /// [`HandlerError`]. Use this in non-HTTP code paths (Lambda, internal + /// callers) that can't surface a 409 with a structured body — the HTTP + /// route layer should match on the enum directly instead. + pub fn into_canonical_or_err(self) -> Result { + match self { + SchemaResolution::Canonical(name) => Ok(name), + SchemaResolution::Ambiguous { input, candidates } => { + Err(HandlerError::BadRequest(format!( + "ambiguous descriptive_name '{}': matches {:?}", + input, candidates + ))) + } + } + } +} + +/// Resolve `requested` to either a single canonical runtime schema name or +/// a conflict listing the matching canonical hashes. /// /// Resolution order: -/// 1. If a schema exists with `name == requested`, return `requested`. -/// 2. Otherwise, scan active schemas for `descriptive_name == requested`. -/// - exactly one match → return its canonical name -/// - zero matches → return `requested` unchanged so downstream emits -/// its own "not found" error (preserves existing behavior for -/// truly unknown names) -/// - 2+ matches → 400 with all candidate canonical hashes listed +/// 1. If a schema exists with `name == requested`, return it as +/// [`SchemaResolution::Canonical`]. +/// 2. Otherwise, scan active (non-Blocked) schemas for matching +/// `descriptive_name`: +/// - exactly one match → [`SchemaResolution::Canonical`] with its +/// canonical name +/// - zero matches → [`SchemaResolution::Canonical`] with `requested` +/// unchanged so downstream emits its own "not found" error +/// (preserves existing behavior for truly unknown names) +/// - 2+ matches → [`SchemaResolution::Ambiguous`] listing all candidate +/// canonical hashes so the caller can disambiguate by hash pub async fn resolve_schema_name( processor: &OperationProcessor, requested: &str, -) -> Result { - if processor - .get_schema(requested) - .await - .typed_handler_err()? - .is_some() - { - return Ok(requested.to_string()); +) -> Result { + let schemas = processor.list_schemas().await.typed_handler_err()?; + + // Exact canonical match wins — `requested` is already the schema's + // runtime `name`, no substitution needed. Cannot delegate to + // [`OperationProcessor::get_schema`] here: that method does its own + // descriptive_name fallback internally and would return `Some` for the + // descriptive label too, making the canonical check spuriously succeed + // and the descriptive label pass through unchanged (the PR #975 bug + // that left descriptive_name queries failing in prod). + if schemas.iter().any(|s| s.schema.name == requested) { + return Ok(SchemaResolution::Canonical(requested.to_string())); } - let schemas = processor.list_schemas().await.typed_handler_err()?; - let matches: Vec = schemas + let mut matches: Vec = schemas .into_iter() .filter(|s| s.schema.descriptive_name.as_deref() == Some(requested)) .map(|s| s.schema.name) .collect(); match matches.len() { - 0 => Ok(requested.to_string()), - 1 => Ok(matches.into_iter().next().unwrap()), - _ => Err(HandlerError::BadRequest(format!( - "ambiguous descriptive_name '{}': matches {:?}", - requested, matches - ))), + 0 => Ok(SchemaResolution::Canonical(requested.to_string())), + 1 => Ok(SchemaResolution::Canonical(matches.pop().unwrap())), + _ => { + // Sort so the payload is stable across runs (HashMap ordering + // in the upstream cache is non-deterministic) — both for + // logging and for any caller that wants to diff two responses. + matches.sort(); + Ok(SchemaResolution::Ambiguous { + input: requested.to_string(), + candidates: matches, + }) + } } } diff --git a/src/server/routes/query.rs b/src/server/routes/query.rs index 9785d51e..91f04b5b 100644 --- a/src/server/routes/query.rs +++ b/src/server/routes/query.rs @@ -4,6 +4,7 @@ use crate::handlers::query as query_handlers; // annotation resolves to the same type registered via `components(schemas(...))`. #[allow(unused_imports)] use crate::handlers::query::QueryResponse; +use crate::handlers::schema_resolution::{resolve_schema_name, SchemaResolution}; use crate::server::http_server::AppState; use crate::server::routes::{ handler_error_to_response, handler_result_to_response, node_or_return, @@ -14,6 +15,45 @@ use fold_db::schema::types::Schema; use serde::{Deserialize, Serialize}; use serde_json::{json, Value}; +/// Run the descriptive_name resolver and convert an `Ambiguous` outcome into +/// a 409 [`HttpResponse`] with a structured body listing every candidate +/// canonical hash, so the caller can pin a future query to one of them. +/// `Canonical` returns the canonical name (or the input verbatim when no +/// match was found — downstream then emits its own "not found" error, +/// preserving prior behaviour). +/// +/// Both `execute_query` and `execute_mutation` need this exact mapping; the +/// helper exists to keep the body shape and status code in lockstep between +/// the two routes. +async fn resolve_or_conflict_response( + processor: &OperationProcessor, + requested: &str, +) -> Result { + match resolve_schema_name(processor, requested).await { + Ok(SchemaResolution::Canonical(name)) => Ok(name), + Ok(SchemaResolution::Ambiguous { input, candidates }) => { + tracing::info!( + target: "fold_node::http_server", + schema_name = %input, + candidates = ?candidates, + "rejecting ambiguous descriptive_name with 409" + ); + Err(HttpResponse::Conflict().json(json!({ + "ok": false, + "error": "ambiguous_schema_name", + "message": format!( + "descriptive_name '{}' matches {} approved schemas; pin by canonical hash", + input, + candidates.len(), + ), + "schema_name": input, + "ambiguous_schemas": candidates, + }))) + } + Err(e) => Err(handler_error_to_response(e)), + } +} + /// Collect every queryable field name the user could already see via /// `GET /api/schema/{name}` — plain fields, transform-field keys, and /// reference-field keys. This is the same surface the 400 response @@ -129,6 +169,7 @@ pub struct MutationResponse { responses( (status = 200, description = "Page of query results plus pagination metadata", body = QueryResponse), (status = 400, description = "Bad request"), + (status = 409, description = "Ambiguous descriptive_name; body lists candidate canonical hashes in `ambiguous_schemas`"), (status = 500, description = "Server error") ) )] @@ -146,7 +187,7 @@ pub async fn execute_query(body: web::Json, state: web::Data) - None => (None, None), }; - let query_inner: Query = match serde_json::from_value(body) { + let mut query_inner: Query = match serde_json::from_value(body) { Ok(q) => q, Err(e) => { tracing::warn!( @@ -171,6 +212,17 @@ pub async fn execute_query(body: web::Json, state: web::Data) - let (user_hash, node) = node_or_return!(state); + // Resolve descriptive_name → canonical hash before any downstream + // bookkeeping. A 2+-Approved-schemas-with-the-same-descriptive_name + // collision becomes a 409 here, not a silent pick that routes the + // query at one of several schemas — see + // [`crate::handlers::schema_resolution`]. + let processor = OperationProcessor::from_ref(&node); + match resolve_or_conflict_response(&processor, &query_inner.schema_name).await { + Ok(canonical) => query_inner.schema_name = canonical, + Err(response) => return response, + } + // Loud unknown-field validation: today the resolver silently drops fields // that aren't on the schema, so a typo (`title` vs `summary`) is // indistinguishable from "schema is empty". When the target resolves to a @@ -178,7 +230,6 @@ pub async fn execute_query(body: web::Json, state: web::Data) - // 400 with the legal field list. Targets that don't resolve as schemas // (views, unknown names) fall through so the resolver's own 404 still // wins. - let processor = OperationProcessor::from_ref(&node); if let Ok(Some(schema_with_state)) = processor.get_schema(&query_inner.schema_name).await { if let Some((unknown, available)) = find_unknown_fields(&schema_with_state.schema, &query_inner.fields) @@ -234,6 +285,7 @@ pub async fn execute_query(body: web::Json, state: web::Data) - responses( (status = 200, description = "Mutation accepted", body = MutationResponse), (status = 400, description = "Bad request"), + (status = 409, description = "Ambiguous descriptive_name; body lists candidate canonical hashes in `ambiguous_schemas`"), (status = 500, description = "Server error") ) )] @@ -241,7 +293,7 @@ pub async fn execute_mutation( mutation_data: web::Json, state: web::Data, ) -> impl Responder { - let (schema, fields_and_values, key_value, mutation_type) = + let (mut schema, fields_and_values, key_value, mutation_type) = match serde_json::from_value::(mutation_data.into_inner()) { Ok(Operation::Mutation { schema, @@ -272,6 +324,16 @@ pub async fn execute_mutation( let (user_hash, node) = node_or_return!(state); + // Resolve descriptive_name → canonical hash for mutation symmetry with + // execute_query — 2+ Approved schemas sharing a descriptive_name become a + // 409 here, not a silent pick that could write the molecule into the + // wrong schema. + let processor = OperationProcessor::from_ref(&node); + match resolve_or_conflict_response(&processor, &schema).await { + Ok(canonical) => schema = canonical, + Err(response) => return response, + } + // Loud unknown-field validation, parallel to execute_query above. Without // this gate the mutation pipeline silently writes unknown field names into // the molecule — a typo in `fields_and_values` is indistinguishable from @@ -281,7 +343,6 @@ pub async fn execute_mutation( // writable) and 400 with the legal field list. Targets that don't resolve // as schemas fall through so the resolver's own error wins, matching the // query-side contract. - let processor = OperationProcessor::from_ref(&node); if let Ok(Some(schema_with_state)) = processor.get_schema(&schema).await { if let Some((unknown, available)) = mutation_unknown_fields(&schema_with_state.schema, &fields_and_values) @@ -1007,4 +1068,194 @@ mod tests { }) .await; } + + // ----- descriptive_name resolution at the route layer ----- + + /// Load one Approved HashRange schema whose canonical `name` differs from + /// its `descriptive_name`, mimicking how the schema service rewrites + /// user-ingested schemas (canonical = identity hash, descriptive = human + /// label like "Contacts"). Used to drive the route-layer descriptive_name + /// tests. + async fn load_named_schema( + state: &web::Data, + canonical: &str, + descriptive: &str, + fields: &[&str], + hash_field: &str, + ) { + let node = state.node_manager.get_node("test_user").await.unwrap(); + let mut schema = DeclarativeSchemaDefinition::new( + canonical.to_string(), + DeclarativeSchemaType::HashRange, + Some(KeyConfig { + hash_field: Some(hash_field.to_string()), + range_field: Some("_rk".to_string()), + }), + Some( + fields + .iter() + .map(|f| f.to_string()) + .chain(std::iter::once("_rk".to_string())) + .collect(), + ), + None, + None, + ); + schema.descriptive_name = Some(descriptive.to_string()); + schema.populate_runtime_fields().unwrap(); + let db = node.get_fold_db().unwrap(); + db.schema_manager() + .load_schema_internal(schema) + .await + .unwrap(); + db.schema_manager() + .set_schema_state(canonical, SchemaState::Approved) + .await + .unwrap(); + } + + #[tokio::test] + async fn execute_query_accepts_descriptive_name_with_200() { + let temp_dir = tempdir().unwrap(); + let state = create_test_state(&temp_dir).await; + // Canonical name is a synthetic 64-hex identity-hash; descriptive + // name is the human label that READMEs, the UI, and `folddb query + // ` all use. + load_named_schema( + &state, + "fe331affcd23486a170a2bfb56555e114f7c2371a346b5fe58d2177746f831e3", + "Contacts", + &["full_name"], + "full_name", + ) + .await; + + fold_db::user_context::run_with_user("test_user", async move { + let body = serde_json::json!({ + "schema_name": "Contacts", + "fields": ["full_name"], + }); + let req = actix_test::TestRequest::default().to_http_request(); + let resp = execute_query(web::Json(body), state).await.respond_to(&req); + let status = resp.status(); + let body = body_json(resp).await; + assert_eq!( + status, + StatusCode::OK, + "descriptive_name must resolve to canonical hash and return 200; body was {}", + body + ); + }) + .await; + } + + #[tokio::test] + async fn execute_query_returns_409_on_ambiguous_descriptive_name() { + let temp_dir = tempdir().unwrap(); + let state = create_test_state(&temp_dir).await; + // Two Approved schemas share the descriptive_name "Contacts" — the + // real-world failure mode that motivated this 409 (see the running + // prod with 2x Contacts, 2x CalendarEvent, 2x Photography). + let hash_a = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; + let hash_b = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"; + load_named_schema(&state, hash_a, "Contacts", &["full_name"], "full_name").await; + load_named_schema(&state, hash_b, "Contacts", &["full_name"], "full_name").await; + + fold_db::user_context::run_with_user("test_user", async move { + let body = serde_json::json!({ + "schema_name": "Contacts", + "fields": ["full_name"], + }); + let req = actix_test::TestRequest::default().to_http_request(); + let resp = execute_query(web::Json(body), state).await.respond_to(&req); + assert_eq!( + resp.status(), + StatusCode::CONFLICT, + "two Approved schemas sharing a descriptive_name must surface 409, not 200 or 500" + ); + + let body = body_json(resp).await; + assert_eq!(body["ok"], serde_json::json!(false)); + assert_eq!(body["error"], serde_json::json!("ambiguous_schema_name")); + assert_eq!(body["schema_name"], serde_json::json!("Contacts")); + let mut candidates: Vec = + serde_json::from_value(body["ambiguous_schemas"].clone()) + .expect("ambiguous_schemas should be a string array"); + candidates.sort(); + assert_eq!( + candidates, + vec![hash_a.to_string(), hash_b.to_string()], + "every Approved canonical hash must appear so the caller can pin one" + ); + }) + .await; + } + + #[tokio::test] + async fn execute_mutation_accepts_descriptive_name_with_200() { + let temp_dir = tempdir().unwrap(); + let state = create_test_state(&temp_dir).await; + load_named_schema( + &state, + "fe331affcd23486a170a2bfb56555e114f7c2371a346b5fe58d2177746f831e3", + "Contacts", + &["full_name"], + "full_name", + ) + .await; + + fold_db::user_context::run_with_user("test_user", async move { + let body = mutation_body( + "Contacts", + "create", + serde_json::json!({ "full_name": "Ada Lovelace", "_rk": "ada" }), + ); + let req = actix_test::TestRequest::default().to_http_request(); + let resp = execute_mutation(web::Json(body), state) + .await + .respond_to(&req); + assert_eq!( + resp.status(), + StatusCode::OK, + "descriptive_name on mutation must resolve to canonical and return 200" + ); + }) + .await; + } + + #[tokio::test] + async fn execute_mutation_returns_409_on_ambiguous_descriptive_name() { + let temp_dir = tempdir().unwrap(); + let state = create_test_state(&temp_dir).await; + let hash_a = "1111111111111111111111111111111111111111111111111111111111111111"; + let hash_b = "2222222222222222222222222222222222222222222222222222222222222222"; + load_named_schema(&state, hash_a, "Contacts", &["full_name"], "full_name").await; + load_named_schema(&state, hash_b, "Contacts", &["full_name"], "full_name").await; + + fold_db::user_context::run_with_user("test_user", async move { + let body = mutation_body( + "Contacts", + "create", + serde_json::json!({ "full_name": "Grace Hopper", "_rk": "grace" }), + ); + let req = actix_test::TestRequest::default().to_http_request(); + let resp = execute_mutation(web::Json(body), state) + .await + .respond_to(&req); + assert_eq!( + resp.status(), + StatusCode::CONFLICT, + "ambiguous descriptive_name on mutation must surface 409, not write into one" + ); + + let body = body_json(resp).await; + assert_eq!(body["error"], serde_json::json!("ambiguous_schema_name")); + let mut candidates: Vec = + serde_json::from_value(body["ambiguous_schemas"].clone()) + .expect("ambiguous_schemas should be a string array"); + candidates.sort(); + assert_eq!(candidates, vec![hash_a.to_string(), hash_b.to_string()]); + }) + .await; + } } diff --git a/src/server/static-react/src/types/openapi.ts b/src/server/static-react/src/types/openapi.ts index e4fabeb9..5242838a 100644 --- a/src/server/static-react/src/types/openapi.ts +++ b/src/server/static-react/src/types/openapi.ts @@ -1870,6 +1870,13 @@ export interface operations { }; content?: never; }; + /** @description Ambiguous descriptive_name; body lists candidate canonical hashes in `ambiguous_schemas` */ + 409: { + headers: { + [name: string]: unknown; + }; + content?: never; + }; /** @description Server error */ 500: { headers: { @@ -1947,6 +1954,13 @@ export interface operations { }; content?: never; }; + /** @description Ambiguous descriptive_name; body lists candidate canonical hashes in `ambiguous_schemas` */ + 409: { + headers: { + [name: string]: unknown; + }; + content?: never; + }; /** @description Server error */ 500: { headers: {