From 346b137b8be60946d6281d23336cb3d55adc309b Mon Sep 17 00:00:00 2001 From: cdxiaodong <84082748+cdxiaodong@users.noreply.github.com> Date: Fri, 14 Aug 2026 21:34:07 +0800 Subject: [PATCH] fix(cron): accept nested GET-response shape in POST /api/cron/jobs The create endpoint rejected every JSON body with 400 "Invalid JSON request body." because CreateCronJobRequest used #[serde(deny_unknown_fields)] over a legacy flat schema (top-level message/conversation_id/created_by), while agents and the CLI build create bodies by mirroring the nested shape returned by GET /api/cron/jobs (target.payload.text, target.execution_mode, enabled). The unknown fields tripped deny_unknown_fields and serde rejected the body before any field validation, so even {} failed. PUT /{id} was unaffected because UpdateCronJobRequest has no deny_unknown_fields and all-optional fields. Make the create DTO accept both shapes: - add optional target (reusing CronJobTargetDto) and enabled fields - make conversation_id/created_by optional (created_by defaults to "agent" in the service layer; conversation_id is still required at the service layer for existing execution mode) - drop deny_unknown_fields so a body valid for PUT also deserializes for create The service layer normalizes target.payload.text -> message and target.execution_mode -> execution_mode, with flat fields taking precedence, and honors the enabled flag instead of hardcoding true. Fixes iOfficeAI/AionUi#4042 --- crates/aionui-api-types/src/cron.rs | 87 ++++++++++++++++--- crates/aionui-cron/src/service.rs | 31 +++++-- .../aionui-cron/tests/service_integration.rs | 62 ++++++------- 3 files changed, 131 insertions(+), 49 deletions(-) diff --git a/crates/aionui-api-types/src/cron.rs b/crates/aionui-api-types/src/cron.rs index 94eb82098..bcb282b5e 100644 --- a/crates/aionui-api-types/src/cron.rs +++ b/crates/aionui-api-types/src/cron.rs @@ -143,8 +143,16 @@ pub struct CronJobResponse { // D. Create / Update request DTOs // --------------------------------------------------------------------------- +/// Create request for `POST /api/cron/jobs`. +/// +/// Accepts both the legacy flat shape (`message` / `conversation_id` / +/// `created_by` as top-level fields) and the nested shape returned by +/// `GET /api/cron/jobs` (`target.payload.text` / `target.execution_mode`, +/// `enabled`). Agents typically build a create body by mirroring the GET +/// response, so the nested form must deserialize here. Unknown fields are +/// ignored rather than rejected so a body that is valid for `PUT /{id}` also +/// works for create; see issue iOfficeAI/AionUi#4042. #[derive(Debug, Clone, Deserialize)] -#[serde(deny_unknown_fields)] pub struct CreateCronJobRequest { pub name: String, #[serde(default)] @@ -154,10 +162,22 @@ pub struct CreateCronJobRequest { pub prompt: Option, #[serde(default)] pub message: Option, - pub conversation_id: String, + /// Nested message/execution target, mirroring the GET response shape. + /// `target.payload.text` is used as the job message and + /// `target.execution_mode` as the execution mode when the corresponding + /// flat fields are absent. + #[serde(default)] + pub target: Option, + /// Initial enabled state. Defaults to `true` to preserve prior behavior. + #[serde(default)] + pub enabled: Option, + #[serde(default)] + pub conversation_id: Option, #[serde(default)] pub conversation_title: Option, - pub created_by: String, + /// Defaults to `"agent"` when omitted (the CLI/agent create path). + #[serde(default)] + pub created_by: Option, #[serde(default)] pub execution_mode: Option, #[serde(default)] @@ -711,8 +731,8 @@ mod tests { let req: CreateCronJobRequest = serde_json::from_value(raw).unwrap(); assert_eq!(req.name, "Daily task"); assert_eq!(req.message.as_deref(), Some("Do the thing")); - assert_eq!(req.conversation_id, "conv_1"); - assert_eq!(req.created_by, "user"); + assert_eq!(req.conversation_id.as_deref(), Some("conv_1")); + assert_eq!(req.created_by.as_deref(), Some("user")); assert_eq!(req.execution_mode.as_deref(), Some("new_conversation")); assert!(req.agent_config.is_some()); } @@ -733,6 +753,46 @@ mod tests { assert!(req.agent_config.is_none()); } + /// The nested shape returned by `GET /api/cron/jobs` must also deserialize, + /// since agents build create bodies by mirroring it. Regression test for + /// iOfficeAI/AionUi#4042. + #[test] + fn create_request_nested_response_shape() { + let raw = json!({ + "name": "probe", + "enabled": true, + "schedule": {"kind": "cron", "expr": "17 9 * * *", "tz": "Asia/Shanghai"}, + "target": { + "payload": {"kind": "message", "text": "hi"}, + "execution_mode": "new_conversation" + } + }); + let req: CreateCronJobRequest = serde_json::from_value(raw).unwrap(); + assert_eq!(req.name, "probe"); + assert_eq!(req.enabled, Some(true)); + let target = req.target.expect("target should deserialize"); + assert_eq!(target.execution_mode.as_deref(), Some("new_conversation")); + assert_eq!(target.payload, CronJobPayloadDto::Message { text: "hi".to_owned() }); + // Flat fields are absent in the nested shape. + assert!(req.message.is_none()); + assert!(req.conversation_id.is_none()); + assert!(req.created_by.is_none()); + } + + /// Unknown fields must be ignored rather than rejected with a 400, so a + /// body valid for `PUT /{id}` also works for create. + #[test] + fn create_request_ignores_unknown_fields() { + let raw = json!({ + "name": "Ping", + "schedule": {"kind": "every", "every_ms": 60000}, + "some_future_field": {"nested": true}, + "metadata": {"agent_config": {"name": "Claude"}} + }); + let req: CreateCronJobRequest = serde_json::from_value(raw).unwrap(); + assert_eq!(req.name, "Ping"); + } + #[test] fn create_request_with_prompt() { let raw = json!({ @@ -769,16 +829,21 @@ mod tests { #[test] fn create_request_missing_conversation_id() { + // conversation_id is optional: only required at the service layer for + // `existing` execution mode, not at deserialize time. See #4042. let raw = json!({ "name": "X", "schedule": {"kind": "every", "every_ms": 1000}, "created_by": "user" }); - assert!(serde_json::from_value::(raw).is_err()); + let req: CreateCronJobRequest = serde_json::from_value(raw).unwrap(); + assert!(req.conversation_id.is_none()); } #[test] - fn create_request_rejects_legacy_agent_type() { + fn create_request_ignores_legacy_agent_type() { + // Unknown / deprecated fields are ignored rather than rejected, so a + // body shaped like the GET response (or a PUT body) still deserializes. let raw = json!({ "name": "X", "schedule": {"kind": "every", "every_ms": 1000}, @@ -786,18 +851,20 @@ mod tests { "agent_type": "acp", "created_by": "user" }); - let err = serde_json::from_value::(raw).expect_err("legacy agent_type must be rejected"); - assert!(err.to_string().contains("agent_type")); + let req: CreateCronJobRequest = serde_json::from_value(raw).unwrap(); + assert_eq!(req.name, "X"); } #[test] fn create_request_missing_created_by() { + // created_by is optional and defaults to "agent" in the service layer. let raw = json!({ "name": "X", "schedule": {"kind": "every", "every_ms": 1000}, "conversation_id": "c1", }); - assert!(serde_json::from_value::(raw).is_err()); + let req: CreateCronJobRequest = serde_json::from_value(raw).unwrap(); + assert!(req.created_by.is_none()); } // -- E. UpdateCronJobRequest ---------------------------------------------- diff --git a/crates/aionui-cron/src/service.rs b/crates/aionui-cron/src/service.rs index 4d04257c5..04206c52a 100644 --- a/crates/aionui-cron/src/service.rs +++ b/crates/aionui-cron/src/service.rs @@ -3,8 +3,8 @@ use std::str::FromStr; use std::sync::Arc; use aionui_api_types::{ - CreateConversationCronRequest, CreateConversationCronResponse, CreateCronJobRequest, CronJobResponse, - CronScheduleDto, HasSkillResponse, ListCronJobsQuery, RunNowResponse, SaveCronSkillRequest, + CreateConversationCronRequest, CreateConversationCronResponse, CreateCronJobRequest, CronJobPayloadDto, + CronJobResponse, CronScheduleDto, HasSkillResponse, ListCronJobsQuery, RunNowResponse, SaveCronSkillRequest, UpdateConversationCronRequest, UpdateCronJobRequest, }; use aionui_common::{ @@ -121,9 +121,11 @@ impl CronService { schedule: schedule_dto, prompt: None, message: Some(req.message), - conversation_id: conversation_id.to_owned(), + target: None, + enabled: None, + conversation_id: Some(conversation_id.to_owned()), conversation_title, - created_by: "agent".to_owned(), + created_by: Some("agent".to_owned()), execution_mode: Some("existing".to_owned()), queue_enabled: false, agent_config, @@ -268,10 +270,21 @@ impl CronService { }; validate_aionrs_agent_config(&resolved_agent_type, req.agent_config.as_ref())?; - let execution_mode = parse_execution_mode(req.execution_mode.as_deref())?; - let created_by = CreatedBy::from_str(&req.created_by)?; - let message = req.message.or(req.prompt).unwrap_or_default(); - let conversation_id = req.conversation_id.trim(); + // Normalize the nested create shape (mirrors the GET response) onto the + // flat internal fields. Flat fields take precedence when both are set; + // see issue iOfficeAI/AionUi#4042. + let target = req.target; + let execution_mode_raw = req + .execution_mode + .or_else(|| target.as_ref().and_then(|t| t.execution_mode.clone())); + let execution_mode = parse_execution_mode(execution_mode_raw.as_deref())?; + let created_by = CreatedBy::from_str(req.created_by.as_deref().unwrap_or("agent"))?; + let target_text = target.map(|t| match t.payload { + CronJobPayloadDto::Message { text } => text, + }); + let message = req.message.or(req.prompt).or(target_text).unwrap_or_default(); + let conversation_id = req.conversation_id.unwrap_or_default(); + let conversation_id = conversation_id.trim(); if matches!(execution_mode, ExecutionMode::Existing) { self.require_existing_conversation_scope(user_id, conversation_id) .await?; @@ -297,7 +310,7 @@ impl CronService { id: generate_prefixed_id("cron"), user_id: user_id.to_owned(), name: req.name, - enabled: true, + enabled: req.enabled.unwrap_or(true), schedule, message, execution_mode, diff --git a/crates/aionui-cron/tests/service_integration.rs b/crates/aionui-cron/tests/service_integration.rs index 652c99ce1..5ad98551c 100644 --- a/crates/aionui-cron/tests/service_integration.rs +++ b/crates/aionui-cron/tests/service_integration.rs @@ -1117,14 +1117,16 @@ async fn setup_with_assistant_repos() -> ( fn make_create_req(name: &str, schedule: CronScheduleDto) -> CreateCronJobRequest { CreateCronJobRequest { + target: None, + enabled: None, name: name.into(), description: Some("test description".into()), schedule, prompt: None, message: Some("test message".into()), - conversation_id: "conv_1".into(), + conversation_id: Some("conv_1".to_string()), conversation_title: Some("Test Conv".into()), - created_by: "user".into(), + created_by: Some("user".to_string()), execution_mode: None, queue_enabled: false, agent_config: Some(aionui_api_types::CronAgentConfigWriteDto { @@ -1168,9 +1170,9 @@ fn make_cron_row_with_workspace(id: &str, user_id: &str, conversation_id: &str, }) .unwrap(), ), - conversation_id: conversation_id.into(), + conversation_id: conversation_id.to_string(), conversation_title: Some("Test Conv".into()), - created_by: "user".into(), + created_by: "user".to_string(), skill_content: None, description: None, created_at: now, @@ -1590,9 +1592,9 @@ async fn list_jobs_allows_legacy_custom_agent_id_without_assistant_id() { }) .to_string(), ), - conversation_id: "conv_1".into(), + conversation_id: "conv_1".to_string(), conversation_title: None, - created_by: "user".into(), + created_by: "user".to_string(), skill_content: None, description: None, created_at: now_ms(), @@ -1625,15 +1627,15 @@ async fn cj7_list_by_conversation() { let (svc, _, _) = setup().await; let mut req1 = make_create_req("Job A", every_60s()); - req1.conversation_id = "conv_target".into(); + req1.conversation_id = Some("conv_target".into()); svc.add_job("u1", req1).await.unwrap(); let mut req2 = make_create_req("Job B", every_60s()); - req2.conversation_id = "conv_target".into(); + req2.conversation_id = Some("conv_target".into()); svc.add_job("u1", req2).await.unwrap(); let mut req3 = make_create_req("Job C", every_60s()); - req3.conversation_id = "conv_other".into(); + req3.conversation_id = Some("conv_other".into()); svc.add_job("u1", req3).await.unwrap(); let query = ListCronJobsQuery { @@ -1648,7 +1650,7 @@ async fn cj7b_add_job_binds_existing_conversation_to_job() { let (svc, _, _, conv_repo) = setup_with_conv_repo().await; let mut req = make_create_req("Bound Existing Conversation", every_60s()); - req.conversation_id = "conv_existing_bind".into(); + req.conversation_id = Some("conv_existing_bind".into()); let job = svc.add_job("u1", req).await.unwrap(); @@ -1779,7 +1781,7 @@ async fn update_existing_conversation_job_rejects_agent_config_even_when_switchi async fn update_existing_job_to_new_conversation_keeps_owner_anchor() { let (svc, cron_repo, _, conv_repo) = setup_with_conv_repo().await; let mut create_req = make_create_req("Mode Switch Clears Binding", every_60s()); - create_req.conversation_id = "conv_mode_switch".into(); + create_req.conversation_id = Some("conv_mode_switch".into()); create_req.execution_mode = Some("existing".into()); let created = svc.add_job("u1", create_req).await.unwrap(); @@ -1837,7 +1839,7 @@ async fn update_existing_job_to_new_conversation_clears_previous_auto_workspace( ); let mut create_req = make_create_req("Mode Switch Clears Workspace", every_60s()); - create_req.conversation_id = conversation_id.clone(); + create_req.conversation_id = Some(conversation_id.clone()); create_req.execution_mode = Some("existing".into()); create_req.agent_config.as_mut().unwrap().workspace = Some(auto_workspace); let created = svc.add_job("u1", create_req).await.unwrap(); @@ -1890,7 +1892,7 @@ async fn update_existing_job_to_new_conversation_preserves_custom_workspace() { ); let mut create_req = make_create_req("Mode Switch Preserves Workspace", every_60s()); - create_req.conversation_id = conversation_id.clone(); + create_req.conversation_id = Some(conversation_id.clone()); create_req.execution_mode = Some("existing".into()); create_req.agent_config.as_mut().unwrap().workspace = Some(custom_workspace.clone()); let created = svc.add_job("u1", create_req).await.unwrap(); @@ -1923,7 +1925,7 @@ async fn update_existing_job_to_new_conversation_preserves_custom_workspace() { async fn update_team_conversation_job_rejects_execution_mode_change() { let (svc, _, _, conv_repo) = setup_with_conv_repo().await; let mut create_req = make_create_req("Team Cron Mode Lock", every_60s()); - create_req.conversation_id = "conv_team_cron".into(); + create_req.conversation_id = Some("conv_team_cron".into()); conv_repo.set_conversation_extra( "conv_team_cron", serde_json::json!({ @@ -2140,7 +2142,7 @@ async fn sk1_1_save_skill_marks_related_skill_suggest_artifacts_saved() { conv_repo.upsert_artifact_row(aionui_db::ConversationArtifactRow { id: format!("conv_1:skill_suggest:{}", job.id), - conversation_id: "conv_1".into(), + conversation_id: "conv_1".to_string(), cron_job_id: Some(job.id.clone()), kind: "skill_suggest".into(), status: "active".into(), @@ -2542,7 +2544,7 @@ async fn oc1_rejects_lazy_existing_jobs() { let (svc, _repo, _) = setup().await; let mut req = make_create_req("Lazy Existing", every_60s()); - req.conversation_id = "".into(); + req.conversation_id = Some("".into()); req.execution_mode = Some("existing".into()); let err = svc.add_job("u1", req).await.unwrap_err(); assert!(matches!(err, aionui_cron::error::CronError::Conversation(_))); @@ -2553,14 +2555,14 @@ async fn oc1b_allows_new_conversation_jobs_without_owner_anchor() { let (svc, _repo, _) = setup().await; let mut empty_req = make_create_req("New-conv empty", every_60s()); - empty_req.conversation_id = "".into(); + empty_req.conversation_id = Some("".into()); empty_req.execution_mode = Some("new_conversation".into()); let empty_job = svc.add_job("u1", empty_req).await.unwrap(); assert_eq!(empty_job.user_id, "u1"); assert_eq!(empty_job.conversation_id, ""); let mut stale_req = make_create_req("New-conv with stale id", every_60s()); - stale_req.conversation_id = "missing-conv-that-no-longer-exists".into(); + stale_req.conversation_id = Some("missing-conv-that-no-longer-exists".into()); stale_req.execution_mode = Some("new_conversation".into()); let stale_job = svc.add_job("u1", stale_req).await.unwrap(); assert_eq!(stale_job.user_id, "u1"); @@ -2572,7 +2574,7 @@ async fn oc2_rejects_existing_jobs_with_missing_conversation() { let (svc, _repo, _) = setup().await; let mut missing_req = make_create_req("Missing Conversation", every_60s()); - missing_req.conversation_id = "missing-conv-1".into(); + missing_req.conversation_id = Some("missing-conv-1".into()); let err = svc.add_job("u1", missing_req).await.unwrap_err(); assert!(matches!(err, aionui_cron::error::CronError::Conversation(_))); } @@ -2610,7 +2612,7 @@ async fn oc2b_rejects_existing_jobs_with_cross_user_conversation_code() { .unwrap(); let mut req = make_create_req("Cross User Conversation", every_60s()); - req.conversation_id = "conv_user_b".into(); + req.conversation_id = Some("conv_user_b".into()); let err = svc.add_job("u1", req).await.unwrap_err(); assert!(matches!( @@ -2625,7 +2627,7 @@ async fn existing_job_with_missing_conversation_is_rejected() { let (svc, _repo, _bc, _conv_repo) = setup_with_conv_repo().await; let mut req = make_create_req("Missing Existing RunNow", every_60s()); - req.conversation_id = "missing-conv-run-now".into(); + req.conversation_id = Some("missing-conv-run-now".into()); req.execution_mode = Some("existing".into()); let err = svc.add_job("u1", req).await.unwrap_err(); assert!(matches!(err, aionui_cron::error::CronError::Conversation(_))); @@ -2636,7 +2638,7 @@ async fn run_now_on_running_existing_conversation_returns_active_conversation_wi let (svc, cron_repo, bc, _conv_repo, conv_service) = setup_with_conv_runtime().await; let mut req = make_create_req("Running Existing RunNow", every_60s()); - req.conversation_id = "conv-running-run-now".into(); + req.conversation_id = Some("conv-running-run-now".into()); req.execution_mode = Some("existing".into()); let job = svc.add_job("u1", req).await.unwrap(); bc.take_events(); @@ -3376,15 +3378,15 @@ async fn cd1_delete_by_conversation_preserves_jobs() { let (svc, _repo, bc) = setup().await; let mut req_a = make_create_req("Cascade A", every_60s()); - req_a.conversation_id = "conv_cascade".into(); + req_a.conversation_id = Some("conv_cascade".into()); let job_a = svc.add_job("u1", req_a).await.unwrap(); let mut req_b = make_create_req("Cascade B", every_60s()); - req_b.conversation_id = "conv_cascade".into(); + req_b.conversation_id = Some("conv_cascade".into()); let job_b = svc.add_job("u1", req_b).await.unwrap(); let mut req_c = make_create_req("Unrelated", every_60s()); - req_c.conversation_id = "conv_other".into(); + req_c.conversation_id = Some("conv_other".into()); let _job_c = svc.add_job("u1", req_c).await.unwrap(); bc.take_events(); @@ -3434,7 +3436,7 @@ async fn cd3_on_conversation_delete_trait_preserves_jobs() { let (svc, _repo, bc) = setup().await; let mut req = make_create_req("Trait Cascade", every_60s()); - req.conversation_id = "conv_trait_del".into(); + req.conversation_id = Some("conv_trait_del".into()); let job = svc.add_job("u1", req).await.unwrap(); bc.take_events(); @@ -3465,7 +3467,7 @@ async fn cd3b_on_conversation_delete_clears_deleted_workspace_from_jobs() { ); let mut req = make_create_req("Clears Deleted Workspace", every_60s()); - req.conversation_id = conversation_id.clone(); + req.conversation_id = Some(conversation_id.clone()); req.agent_config.as_mut().unwrap().workspace = Some(deleted_workspace); let job = svc.add_job("u1", req).await.unwrap(); bc.take_events(); @@ -3578,7 +3580,7 @@ async fn cd3c_on_conversation_delete_preserves_custom_workspace_on_jobs() { ); let mut req = make_create_req("Preserves Custom Workspace", every_60s()); - req.conversation_id = conversation_id.clone(); + req.conversation_id = Some(conversation_id.clone()); req.agent_config.as_mut().unwrap().workspace = Some(custom_workspace.clone()); let job = svc.add_job("u1", req).await.unwrap(); bc.take_events(); @@ -3600,12 +3602,12 @@ async fn cd4_on_conversation_delete_preserves_all_cron_jobs() { let (svc, _repo, bc) = setup().await; let mut new_conversation_req = make_create_req("Generated Run History", every_60s()); - new_conversation_req.conversation_id = "conv_generated_run".into(); + new_conversation_req.conversation_id = Some("conv_generated_run".into()); new_conversation_req.execution_mode = Some("new_conversation".into()); let new_conversation_job = svc.add_job("u1", new_conversation_req).await.unwrap(); let mut existing_req = make_create_req("Existing Bound Job", every_60s()); - existing_req.conversation_id = "conv_generated_run".into(); + existing_req.conversation_id = Some("conv_generated_run".into()); existing_req.execution_mode = Some("existing".into()); let existing_job = svc.add_job("u1", existing_req).await.unwrap();