Merge pull request 'fix: targeted trailing slashes for POST endpoints (hotfix)' (#74) from fix/trailing-slash-307-redirect into main

Reviewed-on: #74

api/client.py

@@ -88,10 +88,6 @@ class APIClient:
             encoded_id = quote(str(object_id), safe="")
             path += f"/{encoded_id}"
 
-        # Ensure trailing slash to prevent 307 redirects that drop POST bodies
-        if not path.endswith("/"):
-            path += "/"
-
         return urljoin(self.base_url.rstrip("/") + "/", path)
 
     def _add_params(self, url: str, params: Optional[List[tuple]] = None) -> str:

services/decision_service.py

@@ -3,6 +3,7 @@ Decision Service
 
 Manages pitching decision operations for game submission.
 """
+
 from typing import List, Dict, Any, Optional, Tuple
 
 from utils.logging import get_contextual_logger
@@ -16,17 +17,14 @@ class DecisionService:
 
     def __init__(self):
         """Initialize decision service."""
-        self.logger = get_contextual_logger(f'{__name__}.DecisionService')
+        self.logger = get_contextual_logger(f"{__name__}.DecisionService")
         self._get_client = get_global_client
 
     async def get_client(self):
         """Get the API client."""
         return await self._get_client()
 
-    async def create_decisions_batch(
-        self,
-        decisions: List[Dict[str, Any]]
-    ) -> bool:
+    async def create_decisions_batch(self, decisions: List[Dict[str, Any]]) -> bool:
         """
         POST batch of decisions to /decisions endpoint.
 
@@ -42,8 +40,10 @@ class DecisionService:
         try:
             client = await self.get_client()
 
-            payload = {'decisions': decisions}
-            await client.post('decisions', payload)
+            payload = {"decisions": decisions}
+            # Trailing slash required: without it, the server returns a 307 redirect
+            # and aiohttp drops the POST body when following the redirect
+            await client.post("decisions/", payload)
 
             self.logger.info(f"Created {len(decisions)} decisions")
             return True
@@ -70,7 +70,7 @@ class DecisionService:
         """
         try:
             client = await self.get_client()
-            await client.delete(f'decisions/game/{game_id}')
+            await client.delete(f"decisions/game/{game_id}")
 
             self.logger.info(f"Deleted decisions for game {game_id}")
             return True
@@ -80,9 +80,10 @@ class DecisionService:
             raise APIException(f"Failed to delete decisions: {e}")
 
     async def find_winning_losing_pitchers(
-        self,
-        decisions_data: List[Dict[str, Any]]
-    ) -> Tuple[Optional[Player], Optional[Player], Optional[Player], List[Player], List[Player]]:
+        self, decisions_data: List[Dict[str, Any]]
+    ) -> Tuple[
+        Optional[Player], Optional[Player], Optional[Player], List[Player], List[Player]
+    ]:
         """
         Extract WP, LP, SV, Holds, Blown Saves from decisions list and fetch Player objects.
 
@@ -110,17 +111,17 @@ class DecisionService:
 
         # First pass: Extract IDs
         for decision in decisions_data:
-            pitcher_id = int(decision.get('pitcher_id', 0))
+            pitcher_id = int(decision.get("pitcher_id", 0))
 
-            if int(decision.get('win', 0)) == 1:
+            if int(decision.get("win", 0)) == 1:
                 wp_id = pitcher_id
-            if int(decision.get('loss', 0)) == 1:
+            if int(decision.get("loss", 0)) == 1:
                 lp_id = pitcher_id
-            if int(decision.get('is_save', 0)) == 1:
+            if int(decision.get("is_save", 0)) == 1:
                 sv_id = pitcher_id
-            if int(decision.get('hold', 0)) == 1:
+            if int(decision.get("hold", 0)) == 1:
                 hold_ids.append(pitcher_id)
-            if int(decision.get('b_save', 0)) == 1:
+            if int(decision.get("b_save", 0)) == 1:
                 bsv_ids.append(pitcher_id)
 
         # Second pass: Fetch Player objects
@@ -154,9 +155,9 @@ class DecisionService:
         """
         error_str = str(error)
 
-        if 'Player ID' in error_str and 'not found' in error_str:
+        if "Player ID" in error_str and "not found" in error_str:
             return "Invalid pitcher ID in decision data."
-        elif 'Game ID' in error_str and 'not found' in error_str:
+        elif "Game ID" in error_str and "not found" in error_str:
             return "Game not found for decisions."
         else:
             return f"Error submitting decisions: {error_str}"

services/play_service.py

@@ -3,6 +3,7 @@ Play Service
 
 Manages play-by-play data operations for game submission.
 """
+
 from typing import List, Dict, Any
 
 from utils.logging import get_contextual_logger
@@ -16,7 +17,7 @@ class PlayService:
 
     def __init__(self):
         """Initialize play service."""
-        self.logger = get_contextual_logger(f'{__name__}.PlayService')
+        self.logger = get_contextual_logger(f"{__name__}.PlayService")
         self._get_client = get_global_client
 
     async def get_client(self):
@@ -39,8 +40,10 @@ class PlayService:
         try:
             client = await self.get_client()
 
-            payload = {'plays': plays}
-            response = await client.post('plays', payload)
+            payload = {"plays": plays}
+            # Trailing slash required: without it, the server returns a 307 redirect
+            # and aiohttp drops the POST body when following the redirect
+            response = await client.post("plays/", payload)
 
             self.logger.info(f"Created {len(plays)} plays")
             return True
@@ -68,7 +71,7 @@ class PlayService:
         """
         try:
             client = await self.get_client()
-            response = await client.delete(f'plays/game/{game_id}')
+            response = await client.delete(f"plays/game/{game_id}")
 
             self.logger.info(f"Deleted plays for game {game_id}")
             return True
@@ -77,11 +80,7 @@ class PlayService:
             self.logger.error(f"Failed to delete plays for game {game_id}: {e}")
             raise APIException(f"Failed to delete plays: {e}")
 
-    async def get_top_plays_by_wpa(
-        self,
-        game_id: int,
-        limit: int = 3
-    ) -> List[Play]:
+    async def get_top_plays_by_wpa(self, game_id: int, limit: int = 3) -> List[Play]:
         """
         Get top plays by WPA (absolute value) for key plays display.
 
@@ -95,19 +94,15 @@ class PlayService:
         try:
             client = await self.get_client()
 
-            params = [
-                ('game_id', game_id),
-                ('sort', 'wpa-desc'),
-                ('limit', limit)
-            ]
+            params = [("game_id", game_id), ("sort", "wpa-desc"), ("limit", limit)]
 
-            response = await client.get('plays', params=params)
+            response = await client.get("plays", params=params)
 
-            if not response or 'plays' not in response:
-                self.logger.info(f'No plays found for game ID {game_id}')
+            if not response or "plays" not in response:
+                self.logger.info(f"No plays found for game ID {game_id}")
                 return []
 
-            plays = [Play.from_api_data(p) for p in response['plays']]
+            plays = [Play.from_api_data(p) for p in response["plays"]]
 
             self.logger.debug(f"Retrieved {len(plays)} top plays for game {game_id}")
             return plays
@@ -129,11 +124,11 @@ class PlayService:
         error_str = str(error)
 
         # Common error patterns
-        if 'Player ID' in error_str and 'not found' in error_str:
+        if "Player ID" in error_str and "not found" in error_str:
             return "Invalid player ID in scorecard data. Please check player IDs."
-        elif 'Game ID' in error_str and 'not found' in error_str:
+        elif "Game ID" in error_str and "not found" in error_str:
             return "Game not found in database. Please contact an admin."
-        elif 'validation' in error_str.lower():
+        elif "validation" in error_str.lower():
             return f"Data validation error: {error_str}"
         else:
             return f"Error submitting plays: {error_str}"

tests/test_api_client.py

@@ -40,7 +40,7 @@ class TestAPIClientWithAioresponses:
 
         with aioresponses() as m:
             m.get(
-                "https://api.example.com/v3/players/1/",
+                "https://api.example.com/v3/players/1",
                 payload=expected_data,
                 status=200,
             )
@@ -53,7 +53,7 @@ class TestAPIClientWithAioresponses:
     async def test_get_request_404(self, api_client):
         """Test GET request returning 404."""
         with aioresponses() as m:
-            m.get("https://api.example.com/v3/players/999/", status=404)
+            m.get("https://api.example.com/v3/players/999", status=404)
 
             result = await api_client.get("players", object_id=999)
 
@@ -63,7 +63,7 @@ class TestAPIClientWithAioresponses:
     async def test_get_request_401_auth_error(self, api_client):
         """Test GET request with authentication error."""
         with aioresponses() as m:
-            m.get("https://api.example.com/v3/players/", status=401)
+            m.get("https://api.example.com/v3/players", status=401)
 
             with pytest.raises(APIException, match="Authentication failed"):
                 await api_client.get("players")
@@ -72,7 +72,7 @@ class TestAPIClientWithAioresponses:
     async def test_get_request_403_forbidden(self, api_client):
         """Test GET request with forbidden error."""
         with aioresponses() as m:
-            m.get("https://api.example.com/v3/players/", status=403)
+            m.get("https://api.example.com/v3/players", status=403)
 
             with pytest.raises(APIException, match="Access forbidden"):
                 await api_client.get("players")
@@ -82,7 +82,7 @@ class TestAPIClientWithAioresponses:
         """Test GET request with server error."""
         with aioresponses() as m:
             m.get(
-                "https://api.example.com/v3/players/",
+                "https://api.example.com/v3/players",
                 status=500,
                 body="Internal Server Error",
             )
@@ -99,7 +99,7 @@ class TestAPIClientWithAioresponses:
 
         with aioresponses() as m:
             m.get(
-                "https://api.example.com/v3/players/?team_id=5&season=12",
+                "https://api.example.com/v3/players?team_id=5&season=12",
                 payload=expected_data,
                 status=200,
             )
@@ -118,7 +118,7 @@ class TestAPIClientWithAioresponses:
 
         with aioresponses() as m:
             m.post(
-                "https://api.example.com/v3/players/",
+                "https://api.example.com/v3/players",
                 payload=expected_response,
                 status=201,
             )
@@ -134,7 +134,7 @@ class TestAPIClientWithAioresponses:
 
         with aioresponses() as m:
             m.post(
-                "https://api.example.com/v3/players/", status=400, body="Invalid data"
+                "https://api.example.com/v3/players", status=400, body="Invalid data"
             )
 
             with pytest.raises(
@@ -150,7 +150,7 @@ class TestAPIClientWithAioresponses:
 
         with aioresponses() as m:
             m.put(
-                "https://api.example.com/v3/players/1/",
+                "https://api.example.com/v3/players/1",
                 payload=expected_response,
                 status=200,
             )
@@ -165,7 +165,7 @@ class TestAPIClientWithAioresponses:
         update_data = {"name": "Updated Player"}
 
         with aioresponses() as m:
-            m.put("https://api.example.com/v3/players/999/", status=404)
+            m.put("https://api.example.com/v3/players/999", status=404)
 
             result = await api_client.put("players", update_data, object_id=999)
 
@@ -175,7 +175,7 @@ class TestAPIClientWithAioresponses:
     async def test_delete_request_success(self, api_client):
         """Test successful DELETE request."""
         with aioresponses() as m:
-            m.delete("https://api.example.com/v3/players/1/", status=204)
+            m.delete("https://api.example.com/v3/players/1", status=204)
 
             result = await api_client.delete("players", object_id=1)
 
@@ -185,7 +185,7 @@ class TestAPIClientWithAioresponses:
     async def test_delete_request_404(self, api_client):
         """Test DELETE request with 404."""
         with aioresponses() as m:
-            m.delete("https://api.example.com/v3/players/999/", status=404)
+            m.delete("https://api.example.com/v3/players/999", status=404)
 
             result = await api_client.delete("players", object_id=999)
 
@@ -195,7 +195,7 @@ class TestAPIClientWithAioresponses:
     async def test_delete_request_200_success(self, api_client):
         """Test DELETE request with 200 success."""
         with aioresponses() as m:
-            m.delete("https://api.example.com/v3/players/1/", status=200)
+            m.delete("https://api.example.com/v3/players/1", status=200)
 
             result = await api_client.delete("players", object_id=1)
 
@@ -219,7 +219,7 @@ class TestAPIClientHelpers:
         with patch("api.client.get_config", return_value=mock_config):
             with aioresponses() as m:
                 m.get(
-                    "https://api.example.com/v3/test/",
+                    "https://api.example.com/v3/test",
                     payload={"success": True},
                     status=200,
                 )
@@ -279,7 +279,7 @@ class TestIntegrationScenarios:
                     "pos_1": "C",
                 }
                 m.get(
-                    "https://api.example.com/v3/players/1/",
+                    "https://api.example.com/v3/players/1",
                     payload=player_data,
                     status=200,
                 )
@@ -293,7 +293,7 @@ class TestIntegrationScenarios:
                     "season": 12,
                 }
                 m.get(
-                    "https://api.example.com/v3/teams/5/", payload=team_data, status=200
+                    "https://api.example.com/v3/teams/5", payload=team_data, status=200
                 )
 
                 client = APIClient()
@@ -338,7 +338,7 @@ class TestIntegrationScenarios:
                 }
 
                 m.get(
-                    "https://api.example.com/v3/players/?team_id=5",
+                    "https://api.example.com/v3/players?team_id=5",
                     payload=api_response,
                     status=200,
                 )
@@ -357,14 +357,14 @@ class TestIntegrationScenarios:
             with aioresponses() as m:
                 # First request fails with 500
                 m.get(
-                    "https://api.example.com/v3/players/1/",
+                    "https://api.example.com/v3/players/1",
                     status=500,
                     body="Internal Server Error",
                 )
 
                 # Second request succeeds
                 m.get(
-                    "https://api.example.com/v3/players/2/",
+                    "https://api.example.com/v3/players/2",
                     payload={"id": 2, "name": "Working Player"},
                     status=200,
                 )
@@ -392,7 +392,7 @@ class TestIntegrationScenarios:
                 # Mock multiple endpoints
                 for i in range(1, 4):
                     m.get(
-                        f"https://api.example.com/v3/players/{i}/",
+                        f"https://api.example.com/v3/players/{i}",
                         payload={"id": i, "name": f"Player {i}"},
                         status=200,
                     )
@@ -445,8 +445,8 @@ class TestAPIClientCoverageExtras:
             # Test trailing slash handling
             client.base_url = "https://api.example.com/"
             url = client._build_url("players")
-            assert url == "https://api.example.com/v3/players/"
-            assert "//" not in url.replace("https://", "").replace("//", "")
+            assert url == "https://api.example.com/v3/players"
+            assert "//" not in url.replace("https://", "")
 
     @pytest.mark.asyncio
     async def test_parameter_handling_edge_cases(self, mock_config):
@@ -473,7 +473,7 @@ class TestAPIClientCoverageExtras:
             # Test timeout using aioresponses exception parameter
             with aioresponses() as m:
                 m.get(
-                    "https://api.example.com/v3/players/",
+                    "https://api.example.com/v3/players",
                     exception=asyncio.TimeoutError("Request timed out"),
                 )
 
@@ -493,7 +493,7 @@ class TestAPIClientCoverageExtras:
             # Test generic exception
             with aioresponses() as m:
                 m.get(
-                    "https://api.example.com/v3/players/",
+                    "https://api.example.com/v3/players",
                     exception=Exception("Generic error"),
                 )
 
@@ -511,7 +511,7 @@ class TestAPIClientCoverageExtras:
             # Test that the client recreates session when needed
             with aioresponses() as m:
                 m.get(
-                    "https://api.example.com/v3/players/",
+                    "https://api.example.com/v3/players",
                     payload={"success": True},
                     status=200,
                 )