diff --git a/src/http.rs b/src/http.rs index 4ef245b..175b729 100644 --- a/src/http.rs +++ b/src/http.rs @@ -20,7 +20,7 @@ use crate::health; use crate::llm::{EmbedModel, OrchestratorModel, RerankModel}; use crate::profile::VaultProfile; use crate::search; -use crate::serve::RecentWrites; +use crate::serve::{FrontmatterOpInput, FrontmatterOpKind, RecentWrites}; use crate::store::Store; use crate::writer::{ self, AppendInput, CreateNoteInput, DeleteMode, EditFrontmatterInput, EditInput, EditMode, @@ -295,7 +295,7 @@ struct RewriteBody { #[derive(Debug, Deserialize)] struct EditFrontmatterBody { file: String, - operations: Vec, + operations: Vec, } #[derive(Debug, Deserialize)] @@ -633,76 +633,54 @@ async fn record_write(recent_writes: &RecentWrites, path: &std::path::Path) { } } -/// Parse a JSON operations array into `Vec`. -fn parse_frontmatter_ops(operations: &[serde_json::Value]) -> Result, ApiError> { +/// Convert typed operation inputs into `Vec`. +fn parse_frontmatter_ops( + operations: &[FrontmatterOpInput], +) -> Result, ApiError> { let mut ops = Vec::with_capacity(operations.len()); - for op_val in operations { - let op_str = op_val.get("op").and_then(|v| v.as_str()).ok_or_else(|| { - ApiError::bad_request("each operation must have an \"op\" string field") - })?; - match op_str { - "set" => { - let key = op_val.get("key").and_then(|v| v.as_str()).ok_or_else(|| { + for input in operations { + let op = match input.op { + FrontmatterOpKind::Set => { + let key = input.key.as_deref().ok_or_else(|| { ApiError::bad_request("\"set\" operation requires a \"key\" field") })?; - let value = op_val - .get("value") - .and_then(|v| v.as_str()) - .ok_or_else(|| { - ApiError::bad_request("\"set\" operation requires a \"value\" field") - })?; - ops.push(FrontmatterOp::Set(key.to_string(), value.to_string())); + let value = input.value.as_deref().ok_or_else(|| { + ApiError::bad_request("\"set\" operation requires a \"value\" field") + })?; + FrontmatterOp::Set(key.to_string(), value.to_string()) } - "remove" => { - let key = op_val.get("key").and_then(|v| v.as_str()).ok_or_else(|| { + FrontmatterOpKind::Remove => { + let key = input.key.as_deref().ok_or_else(|| { ApiError::bad_request("\"remove\" operation requires a \"key\" field") })?; - ops.push(FrontmatterOp::Remove(key.to_string())); - } - "add_tag" => { - let value = op_val - .get("value") - .and_then(|v| v.as_str()) - .ok_or_else(|| { - ApiError::bad_request("\"add_tag\" operation requires a \"value\" field") - })?; - ops.push(FrontmatterOp::AddTag(value.to_string())); + FrontmatterOp::Remove(key.to_string()) } - "remove_tag" => { - let value = op_val - .get("value") - .and_then(|v| v.as_str()) - .ok_or_else(|| { - ApiError::bad_request("\"remove_tag\" operation requires a \"value\" field") - })?; - ops.push(FrontmatterOp::RemoveTag(value.to_string())); + FrontmatterOpKind::AddTag => { + let value = input.value.as_deref().ok_or_else(|| { + ApiError::bad_request("\"add_tag\" operation requires a \"value\" field") + })?; + FrontmatterOp::AddTag(value.to_string()) } - "add_alias" => { - let value = op_val - .get("value") - .and_then(|v| v.as_str()) - .ok_or_else(|| { - ApiError::bad_request("\"add_alias\" operation requires a \"value\" field") - })?; - ops.push(FrontmatterOp::AddAlias(value.to_string())); + FrontmatterOpKind::RemoveTag => { + let value = input.value.as_deref().ok_or_else(|| { + ApiError::bad_request("\"remove_tag\" operation requires a \"value\" field") + })?; + FrontmatterOp::RemoveTag(value.to_string()) } - "remove_alias" => { - let value = op_val - .get("value") - .and_then(|v| v.as_str()) - .ok_or_else(|| { - ApiError::bad_request( - "\"remove_alias\" operation requires a \"value\" field", - ) - })?; - ops.push(FrontmatterOp::RemoveAlias(value.to_string())); + FrontmatterOpKind::AddAlias => { + let value = input.value.as_deref().ok_or_else(|| { + ApiError::bad_request("\"add_alias\" operation requires a \"value\" field") + })?; + FrontmatterOp::AddAlias(value.to_string()) } - unknown => { - return Err(ApiError::bad_request(&format!( - "unknown frontmatter operation: \"{unknown}\"" - ))); + FrontmatterOpKind::RemoveAlias => { + let value = input.value.as_deref().ok_or_else(|| { + ApiError::bad_request("\"remove_alias\" operation requires a \"value\" field") + })?; + FrontmatterOp::RemoveAlias(value.to_string()) } - } + }; + ops.push(op); } Ok(ops) } diff --git a/src/serve.rs b/src/serve.rs index a778699..691cafb 100644 --- a/src/serve.rs +++ b/src/serve.rs @@ -170,12 +170,35 @@ pub struct RewriteParams { pub preserve_frontmatter: Option, } +/// Operation kind for frontmatter edits. +#[derive(Debug, Clone, Copy, Deserialize, JsonSchema)] +#[serde(rename_all = "snake_case")] +pub enum FrontmatterOpKind { + Set, + Remove, + AddTag, + RemoveTag, + AddAlias, + RemoveAlias, +} + +/// A single frontmatter operation. +#[derive(Debug, Clone, Deserialize, JsonSchema)] +pub struct FrontmatterOpInput { + /// Operation type. + pub op: FrontmatterOpKind, + /// Property key (required for "set" and "remove"). + pub key: Option, + /// Value (required for "set", "add_tag", "remove_tag", "add_alias", "remove_alias"). + pub value: Option, +} + #[derive(Debug, Deserialize, JsonSchema)] pub struct EditFrontmatterParams { /// Target note: file path, basename, or #docid. pub file: String, /// Operations to apply. Array of objects like {"op": "add_tag", "value": "rust"} or {"op": "set", "key": "status", "value": "done"} or {"op": "remove", "key": "status"} or {"op": "remove_tag", "value": "old"}. - pub operations: Vec, + pub operations: Vec, } #[derive(Debug, Deserialize, JsonSchema)] @@ -266,108 +289,82 @@ async fn record_write(recent_writes: &RecentWrites, path: &Path) { } } -/// Parse a JSON operations array into `Vec`. -fn parse_frontmatter_ops(operations: &[serde_json::Value]) -> Result, McpError> { +/// Convert typed operation inputs into `Vec`. +fn parse_frontmatter_ops( + operations: &[FrontmatterOpInput], +) -> Result, McpError> { let mut ops = Vec::with_capacity(operations.len()); - for op_val in operations { - let op_str = op_val.get("op").and_then(|v| v.as_str()).ok_or_else(|| { - McpError::new( - rmcp::model::ErrorCode::INVALID_PARAMS, - "each operation must have an \"op\" string field", - None::, - ) - })?; - match op_str { - "set" => { - let key = op_val.get("key").and_then(|v| v.as_str()).ok_or_else(|| { + for input in operations { + let op = match input.op { + FrontmatterOpKind::Set => { + let key = input.key.as_deref().ok_or_else(|| { McpError::new( rmcp::model::ErrorCode::INVALID_PARAMS, "\"set\" operation requires a \"key\" field", None::, ) })?; - let value = op_val - .get("value") - .and_then(|v| v.as_str()) - .ok_or_else(|| { - McpError::new( - rmcp::model::ErrorCode::INVALID_PARAMS, - "\"set\" operation requires a \"value\" field", - None::, - ) - })?; - ops.push(FrontmatterOp::Set(key.to_string(), value.to_string())); + let value = input.value.as_deref().ok_or_else(|| { + McpError::new( + rmcp::model::ErrorCode::INVALID_PARAMS, + "\"set\" operation requires a \"value\" field", + None::, + ) + })?; + FrontmatterOp::Set(key.to_string(), value.to_string()) } - "remove" => { - let key = op_val.get("key").and_then(|v| v.as_str()).ok_or_else(|| { + FrontmatterOpKind::Remove => { + let key = input.key.as_deref().ok_or_else(|| { McpError::new( rmcp::model::ErrorCode::INVALID_PARAMS, "\"remove\" operation requires a \"key\" field", None::, ) })?; - ops.push(FrontmatterOp::Remove(key.to_string())); - } - "add_tag" => { - let value = op_val - .get("value") - .and_then(|v| v.as_str()) - .ok_or_else(|| { - McpError::new( - rmcp::model::ErrorCode::INVALID_PARAMS, - "\"add_tag\" operation requires a \"value\" field", - None::, - ) - })?; - ops.push(FrontmatterOp::AddTag(value.to_string())); + FrontmatterOp::Remove(key.to_string()) } - "remove_tag" => { - let value = op_val - .get("value") - .and_then(|v| v.as_str()) - .ok_or_else(|| { - McpError::new( - rmcp::model::ErrorCode::INVALID_PARAMS, - "\"remove_tag\" operation requires a \"value\" field", - None::, - ) - })?; - ops.push(FrontmatterOp::RemoveTag(value.to_string())); + FrontmatterOpKind::AddTag => { + let value = input.value.as_deref().ok_or_else(|| { + McpError::new( + rmcp::model::ErrorCode::INVALID_PARAMS, + "\"add_tag\" operation requires a \"value\" field", + None::, + ) + })?; + FrontmatterOp::AddTag(value.to_string()) } - "add_alias" => { - let value = op_val - .get("value") - .and_then(|v| v.as_str()) - .ok_or_else(|| { - McpError::new( - rmcp::model::ErrorCode::INVALID_PARAMS, - "\"add_alias\" operation requires a \"value\" field", - None::, - ) - })?; - ops.push(FrontmatterOp::AddAlias(value.to_string())); + FrontmatterOpKind::RemoveTag => { + let value = input.value.as_deref().ok_or_else(|| { + McpError::new( + rmcp::model::ErrorCode::INVALID_PARAMS, + "\"remove_tag\" operation requires a \"value\" field", + None::, + ) + })?; + FrontmatterOp::RemoveTag(value.to_string()) } - "remove_alias" => { - let value = op_val - .get("value") - .and_then(|v| v.as_str()) - .ok_or_else(|| { - McpError::new( - rmcp::model::ErrorCode::INVALID_PARAMS, - "\"remove_alias\" operation requires a \"value\" field", - None::, - ) - })?; - ops.push(FrontmatterOp::RemoveAlias(value.to_string())); + FrontmatterOpKind::AddAlias => { + let value = input.value.as_deref().ok_or_else(|| { + McpError::new( + rmcp::model::ErrorCode::INVALID_PARAMS, + "\"add_alias\" operation requires a \"value\" field", + None::, + ) + })?; + FrontmatterOp::AddAlias(value.to_string()) } - unknown => { - return Err(McpError::new( - rmcp::model::ErrorCode::INVALID_PARAMS, - format!("unknown frontmatter operation: \"{unknown}\""), - None::, - )); + FrontmatterOpKind::RemoveAlias => { + let value = input.value.as_deref().ok_or_else(|| { + McpError::new( + rmcp::model::ErrorCode::INVALID_PARAMS, + "\"remove_alias\" operation requires a \"value\" field", + None::, + ) + })?; + FrontmatterOp::RemoveAlias(value.to_string()) } - } + }; + ops.push(op); } Ok(ops) } @@ -1165,3 +1162,59 @@ pub async fn run_serve( Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + + /// Regression test for . + #[test] + fn edit_frontmatter_operations_schema_has_object_items() { + let schema = schemars::schema_for!(EditFrontmatterParams); + let json = serde_json::to_value(&schema).unwrap(); + + let items = &json["properties"]["operations"]["items"]; + assert!( + items.is_object(), + "operations.items must be an object schema, got: {items}" + ); + + // schemars may inline properties or use a $ref to $defs; both are + // valid object schemas that OpenAI accepts. + let has_properties = items.get("properties").is_some(); + let has_ref = items.get("$ref").is_some(); + assert!( + has_properties || has_ref, + "operations.items must define properties or $ref, got: {items}" + ); + } + + #[test] + fn frontmatter_op_input_deserializes_all_variants() { + let cases = [ + (r#"{"op":"set","key":"status","value":"done"}"#, "set"), + (r#"{"op":"remove","key":"status"}"#, "remove"), + (r#"{"op":"add_tag","value":"rust"}"#, "add_tag"), + (r#"{"op":"remove_tag","value":"old"}"#, "remove_tag"), + (r#"{"op":"add_alias","value":"eng"}"#, "add_alias"), + (r#"{"op":"remove_alias","value":"eng"}"#, "remove_alias"), + ]; + for (json, label) in cases { + let input: FrontmatterOpInput = serde_json::from_str(json) + .unwrap_or_else(|e| panic!("failed to deserialize {label}: {e}")); + // Verify the parsed input converts to a valid FrontmatterOp. + let ops = parse_frontmatter_ops(&[input]); + assert!(ops.is_ok(), "{label} should produce a valid op: {:?}", ops); + } + } + + #[test] + fn frontmatter_op_input_rejects_unknown_variant() { + let json = r#"{"op":"unknown_op","value":"x"}"#; + let result: Result = serde_json::from_str(json); + assert!( + result.is_err(), + "unknown op variant should fail deserialization" + ); + } +}