fix(fifa17): route a partial squad PUT to a role patch, not a replacement

FIFA 17 sends two different operations to `PUT …/squad/<id>` and distinguishes them only by body shape. Across 73 captured squad PUTs in five captures there are exactly two: * 68x with `players` -- a full replacement (also carrying squadName, formation, squadType, manager, chemistry/rating, and redundantly captain/kicktakers); * 5x without `players` -- `{id, custom, captain, kicktakers}`, emitted by the captain/kick-taker screen. `players` has `#[serde(default)]`, so an absent key and an explicit `[]` collapsed to the same empty vec and every partial update was handed to Core as a replacement with zero slots. Core's empty-replacement guard refused it (400) and the host reported 502, losing the user's captain/kick-taker change. `classify_squad_put` now tests key PRESENCE on the raw JSON before deserialising, so absence ("the squad was not part of this edit") stays distinct from an explicit empty array ("replace with nothing"). An explicit `"players": []` still classifies as a replacement and still meets the guard -- the patch path is not a way around it. The patch path carries the contract correction: omitted `players`, `manager` and actives mean UNCHANGED, never cleared. That is structural -- `CoreRolePatchRequest` has no field able to express them. The extension is MERGED rather than overwritten, because the partial body carries only `custom` and `kicktakers`; overwriting would drop every kit number in the squad. `custom` IS taken from the patch, since the role screen writes per-slot values into it and the two shapes genuinely differ there. Also fixes the error mapping on this route: a Core 400 means the REQUEST was invalid, so it is reported as 400, not as a 502 that blames the server and hides a client error behind "upstream unavailable". Tests use the real captured body and assert it takes the patch path (`replace_squad` call count unchanged), that the manager survives, that kit numbers survive the merge, that an explicit empty `players` still reaches the replacement path, and that an unresolvable captain refuses the whole patch rather than half-applying the kick-takers.
2026-08-25 01:52:51 +00:00
parent a2b0c32a70
commit e7893a0162
4 changed files with 475 additions and 22 deletions
@@ -76,9 +76,13 @@ use openfut_adapter_fifa17::fut::owned_query::{
 use openfut_adapter_fifa17::fut::pack_content::GeneratedCandidate;
 use openfut_adapter_fifa17::fut::sbc as fifa17_sbc;
 use openfut_adapter_fifa17::fut::season_wire;
-use openfut_adapter_fifa17::fut::squad::{parse_squad_put, save_ack, SquadWireResolver};
+use openfut_adapter_fifa17::fut::squad::{
+    classify_squad_put, save_ack, Fifa17SquadPut, Fifa17SquadRolePatch, SquadMutation,
+    SquadWireResolver,
+};
 use openfut_adapter_fifa17::fut::squad_ext::{
-    build_squad_write, Fifa17SquadExtensionV1, SquadBuildError, EXT_NAMESPACE, EXT_SCHEMA_VERSION,
+    build_squad_write, Fifa17SquadExtensionV1, KicktakerRef, SquadBuildError, WireItemRef,
+    EXT_NAMESPACE, EXT_SCHEMA_VERSION,
 };
 use openfut_adapter_fifa17::fut::squad_projection::{
    project_squad, squad_actives, squad_list, user_mass_info_squad, ProjectionSlot, SquadExtInput,
@@ -810,6 +814,21 @@ pub struct CoreReplaceRequest {
    pub ext_payload: String,
 }

+/// A role-only squad patch: captain plus the opaque extension, nothing else.
+///
+/// Deliberately has no `slots`, `name`, `formation` or manager field — the
+/// absence is structural, so this request cannot express a squad replacement
+/// even by mistake.
+#[derive(Debug, Clone)]
+pub struct CoreRolePatchRequest {
+    /// Owned instance to flag as captain. `None` leaves the captain unchanged;
+    /// it is never a request to clear it.
+    pub captain_owned_card_id: Option<String>,
+    pub ext_namespace: String,
+    pub ext_schema_version: i64,
+    pub ext_payload: String,
+}
+
 /// Client-reported shadow evaluation carried through to Core (never Core's
 /// authoritative evaluation).
 #[derive(Debug, Clone, Default)]
@@ -878,6 +897,18 @@ pub trait CoreAccess: Send + Sync {
    /// Replace the active squad's canonical slots + opaque extension atomically.
    fn replace_squad(&self, req: &CoreReplaceRequest) -> Result<CoreReplaceResult, CoreError>;

+    /// Patch ONLY the active squad's role assignments (captain) + opaque
+    /// extension. Never touches player assignments, the manager, or actives.
+    ///
+    /// Separate from [`Self::replace_squad`] because they are different
+    /// operations: a role-only client update carries no slot array, and sending
+    /// it as a replacement presents zero slots to Core's empty-replacement
+    /// guard. Default is `Status(501)` so a transport that has not implemented
+    /// it fails loudly instead of silently degrading into a replacement.
+    fn patch_squad_roles(&self, _req: &CoreRolePatchRequest) -> Result<(), CoreError> {
+        Err(CoreError::Status(501))
+    }
+
    /// The owned instance id assigned as the active squad's **manager**, or
    /// `None` (`GET /club/manager`). Default: `None` — a transport without the
    /// endpoint simply projects no manager (non-fatal, like an absent
@@ -1050,6 +1081,29 @@ impl CoreAccess for HttpCoreClient {
        })
    }

+    fn patch_squad_roles(&self, req: &CoreRolePatchRequest) -> Result<(), CoreError> {
+        let url = format!("{}/squad/roles", self.base_url);
+        let resp = self
+            .client
+            .put(&url)
+            .header("X-OpenFUT-Game", &self.game)
+            .json(&json!({
+                "captain_owned_card_id": req.captain_owned_card_id,
+                "extension": {
+                    "namespace": req.ext_namespace,
+                    "schema_version": req.ext_schema_version,
+                    "payload": req.ext_payload,
+                },
+            }))
+            .send()
+            .map_err(|e| CoreError::Http(e.to_string()))?;
+        let status = resp.status().as_u16();
+        if !(200..300).contains(&status) {
+            return Err(CoreError::Status(status));
+        }
+        Ok(())
+    }
+
    fn get_squad_manager(&self) -> Result<Option<String>, CoreError> {
        let url = format!("{}/club/manager", self.base_url);
        let resp = self
@@ -2844,26 +2898,35 @@ fn error_response(status: u16, code: &str) -> WireResponse {
    }
 }

-/// `PUT …/squad/<n>` — parse the full replacement, reverse-resolve every wire id,
-/// AUTHORIZE every resolved item against the active club, then commit the
-/// canonical squad + FIFA extension to Core in one transaction. On any failure
-/// it returns an error and NEVER falls back to Python (no double mutation).
+/// `PUT …/squad/<n>` — dispatch on which mutation the body actually expresses.
+///
+/// FIFA 17 sends two operations down this one path and distinguishes them only
+/// by body shape (see [`SquadMutation`]). Classifying FIRST is the whole fix: a
+/// role-only update carries no `players`, and routing it into the replacement
+/// path presents zero slots to Core's empty-replacement guard, which correctly
+/// refuses it — silently losing the user's captain/kick-taker change.
 pub fn handle_put_squad(body: &[u8], deps: &SquadDeps<'_>) -> (WireResponse, SquadLog) {
-    let put = match parse_squad_put(body) {
-        Ok(p) => p,
-        Err(e) => {
-            return (
-                error_response(400, "parse_error"),
-                SquadLog {
-                    outcome: "parse_error",
-                    detail: e.to_string(),
-                },
-            )
-        }
-    };
+    match classify_squad_put(body) {
+        Ok(SquadMutation::Replace(put)) => handle_squad_replace(&put, deps),
+        Ok(SquadMutation::PatchRoles(patch)) => handle_squad_role_patch(&patch, deps),
+        Err(e) => (
+            error_response(400, "parse_error"),
+            SquadLog {
+                outcome: "parse_error",
+                detail: e.to_string(),
+            },
+        ),
+    }
+}
+
+/// The full-replacement path: reverse-resolve every wire id, AUTHORIZE every
+/// resolved item against the active club, then commit the canonical squad +
+/// FIFA extension to Core in one transaction. On any failure it returns an
+/// error and NEVER falls back to Python (no double mutation).
+fn handle_squad_replace(put: &Fifa17SquadPut, deps: &SquadDeps<'_>) -> (WireResponse, SquadLog) {
    // Reverse-resolve wire→owned and shape canonical + extension. Refuses on an
    // unresolved wire id or the same owned item placed twice.
-    let build = match build_squad_write(&put, deps.resolver) {
+    let build = match build_squad_write(put, deps.resolver) {
        Ok(b) => b,
        Err(SquadBuildError::UnresolvedWireIds(ids)) => {
            return (
@@ -3004,6 +3067,153 @@ pub fn handle_put_squad(body: &[u8], deps: &SquadDeps<'_>) -> (WireResponse, Squ
    )
 }

+/// `PUT …/squad/<n>` carrying NO `players` — the role-only patch path.
+///
+/// Observed real-client shape: `{id, custom, captain, kicktakers[5]}`, emitted
+/// by the captain/kick-taker screen. Omission is the contract here: the absent
+/// `players`, `manager` and actives mean UNCHANGED, never cleared. That is
+/// enforced structurally — [`CoreRolePatchRequest`] has no field able to express
+/// any of them, and Core's patch issues no statement that can touch them.
+///
+/// The extension is MERGED, not overwritten: the patch body carries only
+/// `custom` and `kicktakers`, so kit numbers, squad type and the client-reported
+/// evaluation are preserved from the stored copy. Overwriting would silently
+/// drop every kit number in the squad.
+fn handle_squad_role_patch(
+    patch: &Fifa17SquadRolePatch,
+    deps: &SquadDeps<'_>,
+) -> (WireResponse, SquadLog) {
+    // Start from the stored extension so omitted FIFA-only state survives. A
+    // stale extension is still the right base: its kit numbers belong to the
+    // same squad, and this patch re-anchors the fingerprint on commit.
+    let read = match deps.core.read_squad_ext(EXT_NAMESPACE) {
+        Ok(r) => r,
+        Err(e) => {
+            return (
+                error_response(502, "core_error"),
+                SquadLog {
+                    outcome: "core_error",
+                    detail: e.to_string(),
+                },
+            )
+        }
+    };
+    let mut ext = match &read.ext {
+        CoreExtState::Fresh {
+            schema_version,
+            payload,
+        }
+        | CoreExtState::Stale {
+            schema_version,
+            payload,
+        } => Fifa17SquadExtensionV1::from_payload(*schema_version, payload).unwrap_or_default(),
+        CoreExtState::Missing => Fifa17SquadExtensionV1::default(),
+    };
+
+    // Carry the patch's own fields through. `custom` genuinely differs between
+    // the two shapes -- the role screen writes per-slot values into it -- so it
+    // is taken from the patch, not preserved.
+    if patch.custom.is_some() {
+        ext.custom = patch.custom.clone();
+    }
+    ext.kicktakers = patch
+        .kicktakers
+        .iter()
+        .map(|k| KicktakerRef {
+            index: k.index,
+            item: WireItemRef {
+                id: k.id,
+                dream: k.dream,
+            },
+        })
+        .collect();
+
+    // Resolve + AUTHORIZE the captain before any write. Identity resolution is
+    // not authorization: a globally-valid wire id belonging to another profile
+    // must not become this club's captain.
+    let mut captain_owned_card_id = None;
+    if let Some(wire) = patch.captain.filter(|c| *c != 0) {
+        match deps.resolver.owned_id_for_wire(wire) {
+            Some(owned) => {
+                let owned_set: std::collections::HashSet<String> = match deps.core.all_owned() {
+                    Ok(v) => v.into_iter().map(|i| i.owned_card_id).collect(),
+                    Err(e) => {
+                        return (
+                            error_response(502, "core_error"),
+                            SquadLog {
+                                outcome: "core_error",
+                                detail: e.to_string(),
+                            },
+                        )
+                    }
+                };
+                if !owned_set.contains(&owned) {
+                    return (
+                        error_response(403, "not_owned"),
+                        SquadLog {
+                            outcome: "unauthorized_captain",
+                            detail: owned,
+                        },
+                    );
+                }
+                captain_owned_card_id = Some(owned);
+            }
+            None => {
+                // Refuse rather than silently patch the kicktakers alone: the
+                // user asked for a captain and would otherwise be told it
+                // succeeded while the captain never moved.
+                return (
+                    error_response(400, "unresolved_captain"),
+                    SquadLog {
+                        outcome: "unresolved_captain",
+                        detail: wire.to_string(),
+                    },
+                );
+            }
+        }
+    }
+
+    let req = CoreRolePatchRequest {
+        captain_owned_card_id,
+        ext_namespace: EXT_NAMESPACE.to_string(),
+        ext_schema_version: EXT_SCHEMA_VERSION,
+        ext_payload: ext.to_payload(),
+    };
+    if let Err(e) = deps.core.patch_squad_roles(&req) {
+        // A Core 400 means the REQUEST was invalid (e.g. a captain not fielded
+        // in this squad). Reporting that as 502 would blame the server for a
+        // client error and hide it behind "upstream unavailable".
+        let (status, code) = match e {
+            CoreError::Status(400) => (400, "invalid_role_patch"),
+            _ => (502, "core_error"),
+        };
+        return (
+            error_response(status, code),
+            SquadLog {
+                outcome: if status == 400 {
+                    "invalid_role_patch"
+                } else {
+                    "core_error"
+                },
+                detail: e.to_string(),
+            },
+        );
+    }
+    eprintln!(
+        "utas-host owner=RUST route=squad-roles captain={:?} kicktakers={} \
+         (players/manager/actives untouched)",
+        req.captain_owned_card_id,
+        ext.kicktakers.len()
+    );
+    (
+        json_response(&save_ack(patch.id)),
+        SquadLog {
+            outcome: "ok",
+            detail: String::new(),
+        },
+    )
+}
+
 /// `GET …/squad/list` — served from Core + the ONE projector. Stale/Missing are
 /// integrity failures for the migrated dev profile: logged prominently, degraded
 /// to an empty list, NEVER served from Python and NEVER projected from stale ext.