diff --git a/openfut-core b/openfut-core index 9bdc163..8819cc7 160000 --- a/openfut-core +++ b/openfut-core @@ -1 +1 @@ -Subproject commit 9bdc1633a0b11ec5badcf3a742cdcfe0f97e6e6b +Subproject commit 8819cc76a14c2da5986455c3704f6221ad29619f diff --git a/openfut-utas-host/src/lib.rs b/openfut-utas-host/src/lib.rs index abcacba..25e71f1 100644 --- a/openfut-utas-host/src/lib.rs +++ b/openfut-utas-host/src/lib.rs @@ -852,11 +852,19 @@ pub trait CoreAccess: Send + Sync { Ok(None) } - /// Assign (`Some`) or clear (`None`) the active squad's **manager** - /// (`PUT /club/manager`). Default: unimplemented — the production - /// `HttpCoreClient` overrides it; a transport that cannot persist the - /// assignment MUST fail loudly rather than silently drop it. - fn set_squad_manager(&self, _owned_card_id: Option<&str>) -> Result<(), CoreError> { + /// ASSIGN the active squad's **manager** (`PUT /club/manager`). Default: + /// unimplemented — the production `HttpCoreClient` overrides it; a transport + /// that cannot persist the assignment MUST fail loudly rather than silently + /// drop it. + /// + /// Deliberately takes `&str`, NOT `Option<&str>`: there is no FIFA 17 wire + /// shape that removes a manager. The client always sends a manager ref, so + /// "no ownership-backed manager in this save" means the ref was missing or + /// unmappable — never that the user cleared the slot. Passing `None` here + /// used to be forwarded as an explicit null and DELETED the assignment; that + /// is how a client with a destroyed squad model wiped a real manager row. + /// The capability is gone at the type level so the host cannot express it. + fn set_squad_manager(&self, _owned_card_id: &str) -> Result<(), CoreError> { Err(CoreError::Parse( "Core squad manager write is not implemented".into(), )) @@ -1039,7 +1047,7 @@ impl CoreAccess for HttpCoreClient { } } - fn set_squad_manager(&self, owned_card_id: Option<&str>) -> Result<(), CoreError> { + fn set_squad_manager(&self, owned_card_id: &str) -> Result<(), CoreError> { let url = format!("{}/club/manager", self.base_url); let resp = self .client @@ -2925,17 +2933,31 @@ pub fn handle_put_squad(body: &[u8], deps: &SquadDeps<'_>) -> (WireResponse, Squ // Persist the ownership-backed manager assignment (migration 0023). It was // authorized above and Core re-validates club ownership; fail loudly on a // transport error rather than silently dropping the manager. - if let Err(e) = deps - .core - .set_squad_manager(build.canonical.manager_owned_card_id.as_deref()) - { - return ( - error_response(502, "core_error"), - SquadLog { - outcome: "manager_error", - detail: e.to_string(), - }, - ); + // Only written when this save actually carried an ownership-backed manager. + // + // A save with no resolvable manager is NOT a request to remove the current + // one. FIFA 17 has no "remove manager" wire shape — the client always sends a + // ref — so `None` here means the ref was absent, zero, or unmappable, i.e. + // this save says nothing about the manager. Forwarding that absence as an + // explicit clear is exactly how a client whose squad model had been destroyed + // deleted a real manager row (WAL commit 468, squad_managers 1 -> 0), so the + // assignment is left untouched instead. + match build.canonical.manager_owned_card_id.as_deref() { + Some(mgr) => { + if let Err(e) = deps.core.set_squad_manager(mgr) { + return ( + error_response(502, "core_error"), + SquadLog { + outcome: "manager_error", + detail: e.to_string(), + }, + ); + } + } + None => eprintln!( + "utas-host owner=RUST route=squad-replace manager_write_skipped \ + (save carried no ownership-backed manager; existing assignment left unchanged)" + ), } ( json_response(&save_ack(put.id)), diff --git a/openfut-utas-host/tests/host_test.rs b/openfut-utas-host/tests/host_test.rs index 59f74a4..cdefcd2 100644 --- a/openfut-utas-host/tests/host_test.rs +++ b/openfut-utas-host/tests/host_test.rs @@ -198,11 +198,11 @@ impl CoreAccess for FakeCore { Ok(self.manager.lock().clone()) } - fn set_squad_manager(&self, owned_card_id: Option<&str>) -> Result<(), CoreError> { + fn set_squad_manager(&self, owned_card_id: &str) -> Result<(), CoreError> { if self.return_err { return Err(CoreError::Status(500)); } - *self.manager.lock() = owned_card_id.map(str::to_string); + *self.manager.lock() = Some(owned_card_id.to_string()); Ok(()) } } @@ -1181,10 +1181,17 @@ fn put_full_replacement_commits_canonical_and_extension_and_acks_id0() { } /// The squad's manager is an ownership-backed assignment, not an opaque wire -/// echo: a save assigns the owned instance behind the ref, and a later save -/// without a manager CLEARS it (a full replacement replaces the manager too). +/// echo: a save assigns the owned instance behind the ref. A later save that +/// carries NO manager ref does NOT clear it. +/// +/// This test previously asserted the opposite ("a full replacement replaces the +/// manager too"). That was the bug: FIFA 17 has no wire shape that removes a +/// manager — the client always sends a ref — so an absent ref means the save +/// says nothing about the manager, not that the user cleared the slot. A client +/// whose squad model had been destroyed sent exactly that shape and deleted a +/// real manager row (WAL commit 468, squad_managers 1 -> 0). #[test] -fn put_assigns_the_owned_manager_and_a_later_save_clears_it() { +fn put_assigns_the_owned_manager_and_a_later_save_without_one_leaves_it() { let items = vec![gk(), st()]; let (resolver, w) = resolver_with_wires(&items, ASSETS); let core = FakeCore::new(items.clone(), 2); @@ -1211,14 +1218,18 @@ fn put_assigns_the_owned_manager_and_a_later_save_clears_it() { "manager persisted as the Core owned id, never the wire id" ); + // The exact destructive shape: `"manager": []`. let without_manager = put_body("f442", w["oc-a"], &[(0, w["oc-a"], 1)], "[]", None); let (resp, log) = handle_put_squad(&without_manager, &deps); assert_eq!(resp.status, 200, "{log:?}"); assert_eq!( - core.manager(), - None, - "a full replacement without a manager clears the assignment" + core.manager().as_deref(), + Some("oc-b"), + "a save carrying no manager ref must LEAVE the existing assignment alone" ); + + // And the squad itself still committed — the manager decision is separate. + assert_eq!(core.replace_calls(), 2, "both saves committed their slots"); } /// The REAL client always sends a manager ref, and on a real profile it does not