From 36c16aa8c780e3ff9e2e08b7c98891fab59692c7 Mon Sep 17 00:00:00 2001
From: Ilion Beyst <ilion.beyst@gmail.com>
Date: Thu, 15 Sep 2022 21:59:03 +0200
Subject: [PATCH] log bad commands

---
 planetwars-matchrunner/src/match_log.rs |  6 +++
 planetwars-matchrunner/src/pw_match.rs  | 64 ++++++++++++++++++-------
 planetwars-rules/src/protocol.rs        | 28 -----------
 3 files changed, 52 insertions(+), 46 deletions(-)

diff --git a/planetwars-matchrunner/src/match_log.rs b/planetwars-matchrunner/src/match_log.rs
index 30751fd..087f2a9 100644
--- a/planetwars-matchrunner/src/match_log.rs
+++ b/planetwars-matchrunner/src/match_log.rs
@@ -13,6 +13,12 @@ pub enum MatchLogMessage {
     GameState(State),
     #[serde(rename = "stderr")]
     StdErr(StdErrMessage),
+    #[serde(rename = "bad_command")]
+    BadCommand {
+        player_id: u32,
+        command: String,
+        error: String,
+    },
 }
 
 #[derive(Serialize, Deserialize, Debug)]
diff --git a/planetwars-matchrunner/src/pw_match.rs b/planetwars-matchrunner/src/pw_match.rs
index f5b803c..56d38d1 100644
--- a/planetwars-matchrunner/src/pw_match.rs
+++ b/planetwars-matchrunner/src/pw_match.rs
@@ -12,7 +12,7 @@ use std::convert::TryInto;
 
 pub use planetwars_rules::config::{Config, Map};
 
-use planetwars_rules::protocol::{self as proto, PlayerAction};
+use planetwars_rules::protocol as proto;
 use planetwars_rules::serializer as pw_serializer;
 use planetwars_rules::{PlanetWars, PwConfig};
 
@@ -44,12 +44,8 @@ impl PwMatch {
             let player_messages = self.prompt_players().await;
 
             for (player_id, turn) in player_messages {
-                let res = self.execute_action(player_id, turn);
-                if let Some(err) = action_errors(res) {
-                    let _info_str = serde_json::to_string(&err).unwrap();
-                    // TODO
-                    // println!("player {}: {}", player_id, info_str);
-                }
+                let player_action = self.execute_action(player_id, turn);
+                self.log_player_action(player_id, player_action);
             }
             self.match_state.step();
 
@@ -88,18 +84,14 @@ impl PwMatch {
             .await
     }
 
-    fn execute_action(
-        &mut self,
-        player_num: usize,
-        turn: RequestResult<Vec<u8>>,
-    ) -> proto::PlayerAction {
-        let turn = match turn {
-            Err(_timeout) => return proto::PlayerAction::Timeout,
+    fn execute_action(&mut self, player_num: usize, turn: RequestResult<Vec<u8>>) -> PlayerAction {
+        let data = match turn {
+            Err(_timeout) => return PlayerAction::Timeout,
             Ok(data) => data,
         };
 
-        let action: proto::Action = match serde_json::from_slice(&turn) {
-            Err(err) => return proto::PlayerAction::ParseError(err.to_string()),
+        let action: proto::Action = match serde_json::from_slice(&data) {
+            Err(error) => return PlayerAction::ParseError { data, error },
             Ok(action) => action,
         };
 
@@ -108,15 +100,51 @@ impl PwMatch {
             .into_iter()
             .map(|command| {
                 let res = self.match_state.execute_command(player_num, &command);
-                proto::PlayerCommand {
+                PlayerCommand {
                     command,
                     error: res.err(),
                 }
             })
             .collect();
 
-        proto::PlayerAction::Commands(commands)
+        PlayerAction::Commands(commands)
     }
+
+    fn log_player_action(&mut self, player_id: usize, player_action: PlayerAction) {
+        match player_action {
+            PlayerAction::ParseError { data, error } => {
+                // TODO: can this be handled better?
+                let command =
+                    String::from_utf8(data).unwrap_or_else(|_| "<invalid utf-8>".to_string());
+
+                self.match_ctx.log(MatchLogMessage::BadCommand {
+                    player_id: player_id as u32,
+                    command,
+                    error: error.to_string(),
+                });
+            }
+            // TODO: handle other action types
+            _ => {}
+        }
+    }
+}
+
+#[derive(Debug, Clone, Serialize, Deserialize)]
+pub struct PlayerCommand {
+    pub command: proto::Command,
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub error: Option<proto::CommandError>,
+}
+
+/// the action a player performed.
+// TODO: can we name this better? Is this a "play"?
+pub enum PlayerAction {
+    Timeout,
+    ParseError {
+        data: Vec<u8>,
+        error: serde_json::Error,
+    },
+    Commands(Vec<PlayerCommand>),
 }
 
 fn action_errors(action: PlayerAction) -> Option<PlayerAction> {
diff --git a/planetwars-rules/src/protocol.rs b/planetwars-rules/src/protocol.rs
index 23612d0..d93d088 100644
--- a/planetwars-rules/src/protocol.rs
+++ b/planetwars-rules/src/protocol.rs
@@ -49,31 +49,3 @@ pub enum CommandError {
     OriginDoesNotExist,
     DestinationDoesNotExist,
 }
-
-#[derive(Debug, Clone, Serialize, Deserialize)]
-pub struct PlayerCommand {
-    pub command: Command,
-    #[serde(skip_serializing_if = "Option::is_none")]
-    pub error: Option<CommandError>,
-}
-
-#[derive(Debug, Clone, Serialize, Deserialize)]
-#[serde(rename_all = "snake_case")]
-#[serde(tag = "type", content = "value")]
-pub enum PlayerAction {
-    Timeout,
-    ParseError(String),
-    Commands(Vec<PlayerCommand>),
-}
-
-#[derive(Debug, Clone, Serialize, Deserialize)]
-#[serde(rename_all = "snake_case")]
-#[serde(tag = "type", content = "content")]
-pub enum ServerMessage {
-    /// Game state in current turn
-    GameState(State),
-    /// The action that was performed
-    PlayerAction(PlayerAction),
-    /// The game is over, and this is the concluding state.
-    FinalState(State),
-}