From 20e281e0cf03e6b829b936e8da22308c91ba8e28 Mon Sep 17 00:00:00 2001 From: funman300 Date: Tue, 25 Aug 2026 01:52:36 +0000 Subject: [PATCH] feat(squad): support a role-only partial update distinct from replacement `/squad/replace` is a full replacement: it deletes every assignment and reinserts the supplied slots, and 9bdc163 correctly made it refuse a replacement carrying no slots so a broken client cannot write its emptiness back. That guard is load-bearing and is not touched here. But FIFA 17 sends TWO operations down one wire path. Its captain/kick-taker screen emits a body with no `players` at all -- `{id, custom, captain, kicktakers}` -- and the host presented that to `/squad/replace` as a replacement with zero slots. The guard did exactly its job and refused it, so every captain/kick-taker change died with a 400 (surfaced to the client as a 502) and the user's edit was silently lost. Confirmed by bisect against the captures: the same body returned 200 on 08-24 18:06:59 and 502 at 20:05:30, either side of the Core deploy carrying the guard. The operation was mis-described, so the fix is to stop mis-describing it, not to relax the guard. `patch_squad_roles` updates only the captain flag and the opaque extension, in one transaction, issuing no statement that can insert, delete or reorder an assignment row -- player slots, the squad manager and club actives are untouched by construction rather than by care. Two details that matter: * the captain is part of `squad_fingerprint`, so a captain move MUST re-anchor the extension or every later read reports it stale; * the captain is validated against THIS squad's assignments before any write, so an invalid target leaves captain AND extension unapplied rather than half-applying the patch. Tests cover both halves: the captain moves without disturbing assignments and re-anchors the fingerprint, and an unfielded captain is refused with the prior captain and the prior extension payload both intact. The empty-replacement guard regression test continues to pass unchanged. --- src/app.rs | 1 + src/routes/squad.rs | 42 ++++++++++ src/services/squad.rs | 125 ++++++++++++++++++++++++++++ tests/integration_test.rs | 168 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 336 insertions(+) diff --git a/src/app.rs b/src/app.rs index a6b127a..9fecbe3 100644 --- a/src/app.rs +++ b/src/app.rs @@ -241,6 +241,7 @@ pub async fn build(pool: Pool, cfg: Config) -> Result { .route("/squad", post(routes::squad::post_squad)) .route("/squad/ext", get(routes::squad::get_squad_ext)) .route("/squad/replace", put(routes::squad::put_squad_replace)) + .route("/squad/roles", put(routes::squad::put_squad_roles)) .route("/squads", get(routes::squad::get_squads)) .route("/squads/:squad_id", get(routes::squad::get_squad_by_id)) .route("/squads/:squad_id", delete(routes::squad::delete_squad)) diff --git a/src/routes/squad.rs b/src/routes/squad.rs index e136a71..b45742c 100644 --- a/src/routes/squad.rs +++ b/src/routes/squad.rs @@ -235,3 +235,45 @@ pub async fn put_squad_replace( "slots_written": out.slots_written, }))) } + +#[derive(Deserialize)] +pub struct RolePatchReq { + /// Owned card to flag as captain. Omitted means "leave the captain alone" — + /// it is NEVER a request to clear it. Clearing has no established client + /// semantics and is deliberately not invented here. + #[serde(default)] + pub captain_owned_card_id: Option, + pub extension: OpaqueExtensionWrite, +} + +/// `PUT /squad/roles` — patch ONLY role assignments (captain) plus the opaque +/// game extension, atomically. +/// +/// Distinct from `/squad/replace` on purpose. A role-only update carries no slot +/// array, and describing it as a replacement with zero slots trips the +/// empty-replacement guard — which is correct behaviour for a replacement and +/// wrong for a patch. This route never touches player assignments, the squad +/// manager, or club actives. +pub async fn put_squad_roles( + State(state): State, + game: GameId, + Json(req): Json, +) -> AppResult> { + let profile = profile_svc::get_active_profile(&state.pool, game.as_str()).await?; + let club = club_svc::get_club_by_profile(&state.pool, &profile.id).await?; + + let out = squad_svc::patch_squad_roles( + &state.pool, + game.as_str(), + &club.id, + req.captain_owned_card_id.as_deref(), + &req.extension, + ) + .await?; + + Ok(Json(json!({ + "squad_id": out.squad.id, + "canonical_fingerprint": out.canonical_fingerprint, + "captain_changed": out.captain_changed, + }))) +} diff --git a/src/services/squad.rs b/src/services/squad.rs index 3d16a1f..cc04dde 100644 --- a/src/services/squad.rs +++ b/src/services/squad.rs @@ -586,6 +586,131 @@ pub async fn read_squad_with_ext( Ok((squad, players, state)) } +/// Outcome of a role-only squad patch. +pub struct SquadRolesPatched { + pub squad: Squad, + /// Re-anchored fingerprint of the committed canonical state. The captain + /// flag is part of the fingerprint, so a captain change MUST re-anchor the + /// extension or every later read reports it stale. + pub canonical_fingerprint: String, + /// Whether the captain flag actually moved (false when it was already set). + pub captain_changed: bool, +} + +/// Patch ONLY a squad's role assignments plus its opaque game extension, in one +/// transaction. Never inserts, deletes or reorders a single assignment row. +/// +/// This exists because a full replacement and a role-only update are different +/// operations that the FIFA 17 client sends down the same wire path. Routing a +/// role-only update through [`replace_squad_with_extension`] means presenting it +/// as a replacement carrying zero slots, which the empty-replacement guard +/// correctly refuses — the client's captain/kick-taker change was being lost +/// with a 400. The fix is to stop mis-describing the operation, NOT to relax the +/// guard: that guard is load-bearing and stays exactly as strict. +/// +/// Player assignments, the squad manager and club actives are untouched by +/// construction — this function issues no statement that can affect them. +/// +/// `captain_owned_card_id` must already be assigned to this squad. Anything else +/// is refused before any write, so an invalid target leaves the whole patch +/// unapplied (captain AND extension), never half-applied. +pub async fn patch_squad_roles( + pool: &Pool, + game_id: &str, + club_id: &str, + captain_owned_card_id: Option<&str>, + ext: &OpaqueExtensionWrite, +) -> AppResult { + let now = chrono::Utc::now().to_rfc3339(); + let mut tx = pool.begin().await?; + + // Resolve the club's active squad. A role patch NEVER creates a squad: with + // no squad there is nothing to assign a captain within, and inventing one + // here would let a stray patch materialise empty canonical state. + let squad = sqlx::query_as::<_, Squad>( + "SELECT id, club_id, name, formation, created_at, updated_at FROM squads \ + WHERE club_id = ? ORDER BY updated_at DESC LIMIT 1", + ) + .bind(club_id) + .fetch_optional(&mut *tx) + .await? + .ok_or_else(|| AppError::NotFound("no squad found for this club".into()))?; + + let assigned = sqlx::query_as::<_, (String, i64, bool, bool)>( + "SELECT owned_card_id, position_index, is_captain, is_on_bench \ + FROM squad_players WHERE squad_id = ?", + ) + .bind(&squad.id) + .fetch_all(&mut *tx) + .await?; + + let mut captain_changed = false; + if let Some(captain) = captain_owned_card_id { + // Validate against THIS squad's assignments, not the whole collection: + // a captain the user does not field is not a captain, and accepting an + // arbitrary owned card here would let a patch reference any inventory + // item. + // Validated BEFORE any write, so an invalid target aborts the whole + // patch — captain and extension both — rather than half-applying it. + if !assigned.iter().any(|(owned, _, _, _)| owned == captain) { + return Err(AppError::BadRequest(format!( + "captain '{captain}' is not assigned to squad '{}'", + squad.id + ))); + } + let already_captain = assigned + .iter() + .any(|(owned, _, cap, _)| owned == captain && *cap); + let someone_else_captain = assigned + .iter() + .any(|(owned, _, cap, _)| *cap && owned != captain); + captain_changed = !already_captain || someone_else_captain; + sqlx::query("UPDATE squad_players SET is_captain = (owned_card_id = ?) WHERE squad_id = ?") + .bind(captain) + .bind(&squad.id) + .execute(&mut *tx) + .await?; + } + + // Re-anchor to the state as it now stands, applying the captain move to the + // in-memory view rather than re-reading: same transaction, same result, one + // fewer round trip. + let canonical_fingerprint = squad_fingerprint( + &squad.id, + &squad.formation, + assigned.iter().map(|(owned, slot, cap, bench)| { + let is_cap = match captain_owned_card_id { + Some(c) => owned.as_str() == c, + None => *cap, + }; + (*slot, owned.as_str(), is_cap, *bench) + }), + ); + + sqlx::query( + "INSERT OR REPLACE INTO game_entity_ext \ + (game_id, entity_kind, entity_id, namespace, schema_version, canonical_fingerprint, payload, updated_at) \ + VALUES (?, 'squad', ?, ?, ?, ?, ?, ?)", + ) + .bind(game_id) + .bind(&squad.id) + .bind(&ext.namespace) + .bind(ext.schema_version) + .bind(&canonical_fingerprint) + .bind(&ext.payload) + .bind(&now) + .execute(&mut *tx) + .await?; + + tx.commit().await?; + + Ok(SquadRolesPatched { + squad, + canonical_fingerprint, + captain_changed, + }) +} + /// Compatibility wrapper over [`replace_squad`]. /// /// Kept so the existing Core REST route keeps working, but it no longer has its diff --git a/tests/integration_test.rs b/tests/integration_test.rs index b0c6152..b6ef623 100644 --- a/tests/integration_test.rs +++ b/tests/integration_test.rs @@ -3098,6 +3098,174 @@ async fn test_squad_replace_refuses_to_empty_a_populated_squad() { ); } +/// A role-only patch must move the captain and re-anchor the extension WITHOUT +/// disturbing a single assignment. +/// +/// Regression: FIFA 17's captain/kick-taker screen sends a body with no +/// `players`, which the host presented to `/squad/replace` as a replacement +/// carrying zero slots. The empty-replacement guard correctly refused it, so +/// every captain change died with a 400 (surfaced to the client as 502). The +/// operation, not the guard, was wrong. +#[tokio::test] +async fn test_squad_roles_patch_moves_captain_without_touching_assignments() { + let app = build_test_app().await; + auth(&app, "RolePatchUser").await; + + let (_, packs) = json_get(&app, "/packs").await; + let pack_id = packs["packs"][0]["pack_id"].as_str().unwrap().to_string(); + json_post( + &app, + &format!("/packs/open/{pack_id}"), + serde_json::json!({}), + ) + .await; + let (_, coll) = json_get(&app, "/collection").await; + let ids: Vec = coll["collection"] + .as_array() + .unwrap() + .iter() + .take(2) + .map(|c| c["owned_card_id"].as_str().unwrap().to_string()) + .collect(); + + let client_reported = serde_json::json!({ + "client_reported_chemistry": 52, + "client_reported_rating": 90, + "client_reported_star_rating": 90 + }); + let (s, put) = json_put( + &app, + "/squad/replace", + serde_json::json!({ + "name": "OpenFUT", + "formation": "f442", + "slots": [ + {"owned_card_id": ids[0], "slot": 0, "is_captain": true, "is_on_bench": false}, + {"owned_card_id": ids[1], "slot": 1, "is_captain": false, "is_on_bench": false}, + ], + "client_reported": client_reported, + "extension": {"namespace": "fifa17.squad", "schema_version": 1, + "payload": "{\"custom\":\"[1]\",\"kit_numbers\":{\"a\":7}}"}, + }), + ) + .await; + assert_eq!(s, StatusCode::OK, "{put}"); + let before_fp = put["canonical_fingerprint"].as_str().unwrap().to_string(); + + // Move the captain to the second player, carrying a new opaque payload. + let (s, patched) = json_put( + &app, + "/squad/roles", + serde_json::json!({ + "captain_owned_card_id": ids[1], + "extension": {"namespace": "fifa17.squad", "schema_version": 1, + "payload": "{\"custom\":\"[0,8,16]\",\"kit_numbers\":{\"a\":7}}"}, + }), + ) + .await; + assert_eq!(s, StatusCode::OK, "{patched}"); + assert_eq!(patched["captain_changed"], true); + assert_ne!( + patched["canonical_fingerprint"].as_str().unwrap(), + before_fp, + "the captain is part of the fingerprint, so a captain move MUST re-anchor it" + ); + + let (s, ext) = json_get(&app, "/squad/ext?namespace=fifa17.squad").await; + assert_eq!(s, StatusCode::OK); + let players = ext["players"].as_array().unwrap(); + assert_eq!(players.len(), 2, "a role patch must not add or drop slots"); + let captain_of = |owned: &str| -> bool { + players + .iter() + .find(|p| p["owned_card_id"] == owned) + .map(|p| p["is_captain"] == true) + .unwrap_or(false) + }; + assert!(captain_of(&ids[1]), "the new captain is flagged"); + assert!(!captain_of(&ids[0]), "the previous captain is cleared"); + // Fresh, not stale: the patch re-anchored the extension it wrote. + assert_eq!( + ext["extension"]["payload"], "{\"custom\":\"[0,8,16]\",\"kit_numbers\":{\"a\":7}}", + "the patch's payload is the one stored" + ); +} + +/// A role patch naming a captain who is not in the squad must change NOTHING — +/// not the captain, not the extension. All-or-nothing, validated before any write. +#[tokio::test] +async fn test_squad_roles_patch_rejects_unfielded_captain_and_rolls_back() { + let app = build_test_app().await; + auth(&app, "RolePatchRollbackUser").await; + + let (_, packs) = json_get(&app, "/packs").await; + let pack_id = packs["packs"][0]["pack_id"].as_str().unwrap().to_string(); + json_post( + &app, + &format!("/packs/open/{pack_id}"), + serde_json::json!({}), + ) + .await; + let (_, coll) = json_get(&app, "/collection").await; + let ids: Vec = coll["collection"] + .as_array() + .unwrap() + .iter() + .take(3) + .map(|c| c["owned_card_id"].as_str().unwrap().to_string()) + .collect(); + + let original_payload = "{\"custom\":\"[1]\"}"; + let (s, _) = json_put( + &app, + "/squad/replace", + serde_json::json!({ + "name": "OpenFUT", + "formation": "f442", + "slots": [ + {"owned_card_id": ids[0], "slot": 0, "is_captain": true, "is_on_bench": false}, + {"owned_card_id": ids[1], "slot": 1, "is_captain": false, "is_on_bench": false}, + ], + "client_reported": serde_json::json!({}), + "extension": {"namespace": "fifa17.squad", "schema_version": 1, + "payload": original_payload}, + }), + ) + .await; + assert_eq!(s, StatusCode::OK); + + // ids[2] is owned but NOT fielded — a patch must not accept it. + let (s, err) = json_put( + &app, + "/squad/roles", + serde_json::json!({ + "captain_owned_card_id": ids[2], + "extension": {"namespace": "fifa17.squad", "schema_version": 1, + "payload": "{\"custom\":\"[9,9,9]\"}"}, + }), + ) + .await; + assert_eq!( + s, + StatusCode::BAD_REQUEST, + "a captain not assigned to the squad must be refused: {err}" + ); + + let (s, ext) = json_get(&app, "/squad/ext?namespace=fifa17.squad").await; + assert_eq!(s, StatusCode::OK); + let players = ext["players"].as_array().unwrap(); + assert!( + players + .iter() + .any(|p| p["owned_card_id"] == ids[0].as_str() && p["is_captain"] == true), + "the original captain survives a refused patch" + ); + assert_eq!( + ext["extension"]["payload"], original_payload, + "the extension must NOT be written when the captain is refused" + ); +} + /// `PUT /club/manager` must keep three states apart: absent = say nothing, /// explicit null = remove, id = assign. ///