FAForever / FAForever/faf-java-api

Replay review requests from the client, via the api and RabbitMQ

Ouverte
#1,181 12 commentaires 0 réactions 0 personnes assignées Voir sur GitHub
Langage dominant
Java
Étoiles
31
Forks
30
Métriques de merge des PR
Aucune PR mergée en 30 j

Description

**Goal in one sentence:** a player presses "request replay review" in the client, and the request appears as a post in the training Discord's replay review forum channel, attributed to a FAF account the server vouched for.

This writes up Brutus5000's suggestion from Discord: lobby command → RabbitMQ → faf-qai consumes and posts. It replaces two earlier ideas that were rejected for good reasons — a webhook in the client, which cannot verify who is asking, and an HTTP write endpoint on faf-qai, which adds inbound attack surface to a service that is not closely maintained.

## Why this shape

- **Identity is free.** The client is already authenticated on the lobby socket. The server knows who is sending, so nothing has to trust a client-supplied player id.
- **No new inbound surface.** faf-qai consumes from the broker. It opens no port. This was the blocking objection to the HTTP variant.
- **Kill switch.** Unbind the queue and the feature is off — no client release, no code change, no restart.
- **Durability.** A qai restart does not lose requests; they wait in the queue.

## Step 1 — Lobby server (this repo)

A `command_request_replay_review` handler on `LobbyConnection`, following the convention of `command_invite_to_party` and `command_set_player_vetoes`. It:

1. Requires an authenticated connection (the existing `ensure_authenticated` gate covers this — no new mechanism).
2. Validates the payload against a fixed schema and rejects anything else.
3. **Stamps `player_id` and `login` from the connection, not from the body.** The client supplies content fields only. This is the property the whole design rests on; without it the impersonation hole would be back by another route.
4. Rate limits per player and rejects over-limit requests *before* publishing, so abuse never reaches the bus.
5. Publishes to `MQ_EXCHANGE_NAME` with routing key `request.replay_review.create`.

Payload:

| field | source | notes |
|---|---|---|
| `player_id`, `login` | **server** | from the connection |
| `replay_id` | client | id only, never a url — see below |
| `map`, `game_mode`, `faction`, `rating`, `played_at` | client | prefilled by the client, editable by the player |
| `goal` | client | required, free text |
| `struggle` | client | optional, free text |
| `requested_at` | server | ISO 8601 UTC |

Two deliberate choices in that table:

**Structured fields, not rendered Markdown.** Formatting belongs at the destination, so changing how a review post reads does not need a client release, and a second consumer can use the same request differently.

**A replay is named by id only.** The client also knows how to name a replay by link or by a local file. Neither belongs on the bus: a link would let the client decide what a review post points at, and a file nobody else can fetch is not a review request. Those two cases keep using the client's existing copy-and-paste path.

Routing key follows the `request.*` / `success.*` convention already used by `client_message_queue_service.py` and `avatar_change_queue_service.py`.

## Step 2 — Rate limit

There is no per-player rate limiter in the lobby server today, so this is new code. Proposal: one request per player per 24h, as a config constant next to the other tunables, held in memory in a small service.

In-memory means it does not survive a restart and each instance counts on its own — a spam guard rather than a quota. That looks like the right trade: the cost of a duplicate is a duplicate post, and the alternative is a schema migration plus a database write on a path that has no other reason to touch the database. Happy to be argued out of it.

## Step 3 — Broker

A queue for faf-qai bound to `request.replay_review.create` on `faf-rabbitmq`. Ops task, not a code change.

## Step 4 — faf-qai

A consumer on that queue that opens a forum post per request. DSharpPlus supports forum posts natively, so no REST workarounds. It renders the structured payload and writes the verified player name, so the author shown in Discord is one the server vouched for.

If the requesting player has linked their Discord account (`AccountLinkService`), a later version can mention them so the answer reaches them. Not in the first version.

## Step 5 — Client

One frame alongside the existing lobby commands. The existing copy-and-paste path stays as the fallback for a player who is offline, for a replay that has no id, and for the case where any part of this is switched off — so nothing regresses if the feature is disabled.

## Step 6 — Rollout

Ship the lobby side first with no consumer bound: the messages go nowhere and cost nothing. Bind qai's queue when both sides are deployed. The only hard ordering is lobby-before-client.

## Decisions needed

1. **Routing key name** — is `request.replay_review.create` right?
2. **Rate limit** — one per player per day, and is the in-memory spam guard acceptable, or does this want to be durable?
3. **Who creates the queue and binding?**
4. **Is faf-qai's maintainer on board, and does the bot have permission to create posts in that channel?** This one is outside both repos and it is what killed the Dostya attempt, so I would rather ask now than find out later.

A fifth, smaller one: the lobby currently sends nothing back on success and answers a rejection with the existing `notice` path, the same as `command_invite_to_party`. That means the client shows success optimistically. If an explicit acknowledgement is wanted instead, that is a new server→client command and worth deciding before clients are written against it.

## Not in scope

The generic passthrough — a mechanism letting any client message reach the bus without a per-command change. Worth building, but it is the harder problem: it puts unvalidated client input on a bus whose producers are trusted precisely because it is internal only (see the docstring in `client_message_queue_service.py`). One command with a fixed routing key and a validated schema is the safe increment, and it is a concrete first candidate to migrate once the general mechanism exists.

Fan-out itself already works: the topic exchange lets any number of services bind their own queue to the same key, which `ClientMessageQueueService` and `AvatarChangeQueueService` already do side by side. So "multiple services consuming" needs nothing new.

## What I am offering

I have written the lobby server side with tests, and the qai consumer. I need review and a deploy, not implementation time.

Guide de contribution

Aucun guide de contribution indexé pour ce dépôt

Évaluation

Cette issue n'a pas encore été évaluée.

Recevez les nouvelles issues par e-mail

Un résumé court des issues GitHub adaptées aux débutants.