fix(fifa17): never turn a missing manager ref into a manager deletion

The host called set_squad_manager unconditionally on every squad save, passing
the resolved manager or None. None was serialised as {"owned_card_id": null},
an EXPLICIT removal, so a save that merely said nothing about the manager
deleted the assignment. That is how a client whose squad model had been
destroyed wiped a real manager row (WAL commit 468, squad_managers 1 -> 0).

FIFA 17 has no wire shape that removes a manager: the client always sends a
ref. So None never means "the user cleared the slot" - it means the ref was
absent, zero, or unmappable, i.e. this save carries no manager decision. The
assignment is now left untouched and the skip is logged.

The capability is removed at the TYPE level: CoreAccess::set_squad_manager
takes &str, not Option<&str>, so the host cannot express a clear at all. Core
still supports deliberate removal via an explicit null for other callers.

The existing test asserted the destructive behaviour as intended ("a later save
without a manager CLEARS it"). That contract was the bug; it now asserts the
manager survives and that both saves still commit their slots. With the fix
reverted the test fails.

Live-proven on staging against the real route (PUT /ut/game/fifa17/squad/<n>;
squad/active is a GET-only tail and falls through to the dead Python upstream,
which is why an earlier replay attempt proved nothing):

  exact original shape (no resolvable players, manager: [])
    -> 502 core_error, core returned status 400, nothing mutated
  valid 23-player save carrying manager: []
    -> 200 {"id":0}, manager_write_skipped logged, manager PRESERVED
  valid 23-player save carrying the real manager ref
    -> 200 {"id":0}, manager assigned

Players 23/23, manager 1, captain, active club items, coins, integrity and FK
identical before and after, and again after a Core+host restart.
This commit is contained in:
funman300
2026-08-24 19:59:09 +00:00
parent 09bb2dc306
commit 6bbc0eaf4f
3 changed files with 59 additions and 26 deletions
+19 -8
View File
@@ -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