feat(squad): support a role-only partial update distinct from replacement
CI / Build, lint & test (push) Successful in 3m28s
CI / Build, lint & test (push) Successful in 3m28s
`/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.
This commit is contained in:
@@ -241,6 +241,7 @@ pub async fn build(pool: Pool, cfg: Config) -> Result<Router> {
|
||||
.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))
|
||||
|
||||
@@ -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<String>,
|
||||
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<AppState>,
|
||||
game: GameId,
|
||||
Json(req): Json<RolePatchReq>,
|
||||
) -> AppResult<Json<Value>> {
|
||||
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,
|
||||
})))
|
||||
}
|
||||
|
||||
@@ -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<SquadRolesPatched> {
|
||||
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
|
||||
|
||||
@@ -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<String> = 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<String> = 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.
|
||||
///
|
||||
|
||||
Reference in New Issue
Block a user