From e63e5ef2033bd32501b986f8608e7ddaf074461b Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 1 Aug 2026 05:28:14 +0000 Subject: [PATCH] fix: correct HTTP adapter response handling Co-authored-by: Matthew Spah --- mlbstatsapi/mlb_dataadapter.py | 95 ++++++++++++++-------- tests/test_mlb_dataadapter.py | 139 ++++++++++++++++++++++----------- tests/test_mlb_result.py | 35 ++++----- 3 files changed, 168 insertions(+), 101 deletions(-) diff --git a/mlbstatsapi/mlb_dataadapter.py b/mlbstatsapi/mlb_dataadapter.py index 8648fa6..8c3aa4e 100644 --- a/mlbstatsapi/mlb_dataadapter.py +++ b/mlbstatsapi/mlb_dataadapter.py @@ -18,13 +18,18 @@ class MlbResult: JSON Data received from request """ - def __init__(self, status_code: int, message: str, data: Dict = {}): + def __init__( + self, + status_code: int, + message: str, + data: Dict | None = None, + ): self.status_code = int(status_code) self.message = str(message) - self.data = data - if 'copyright' in data: - del data['copyright'] + # Copy so caller-owned dictionaries are not mutated when copyright is removed. + self.data = dict(data) if data is not None else {} + self.data.pop("copyright", None) class MlbDataAdapter: @@ -44,7 +49,6 @@ class MlbDataAdapter: def __init__(self, hostname: str = 'statsapi.mlb.com', ver: str = 'v1', logger: logging.Logger = None): self.url = f'https://{hostname}/api/{ver}/' self._logger = logger or logging.getLogger(__name__) - self._logger.setLevel(logging.DEBUG) def get(self, endpoint: str, ep_params: Dict = None, data: Dict = None) -> MlbResult: """ @@ -76,32 +80,57 @@ def get(self, endpoint: str, ep_params: Dict = None, data: Dict = None) -> MlbRe self._logger.error(msg=(str(e))) raise TheMlbStatsApiException('Request failed') from e - try: - data = response.json() - - except (ValueError, requests.JSONDecodeError) as e: - self._logger.error(msg=(str(e))) - raise TheMlbStatsApiException('Bad JSON in response') from e - - if response.status_code <= 200 and response.status_code <= 299: - self._logger.debug(msg=logline_post.format('success', - response.status_code, response.reason, response.url)) - - return MlbResult(response.status_code, message=response.reason, data=data) - - elif response.status_code >= 400 and response.status_code <= 499: - self._logger.error(msg=logline_post.format('Invalid Request', - response.status_code, response.reason, response.url)) - - # return MlbResult with 404 and empty data - return MlbResult(response.status_code, message=response.reason, data={}) - - elif response.status_code >= 500 and response.status_code <= 599: - - self._logger.error(msg=logline_post.format('Internal error occurred', - response.status_code, response.reason, response.url)) - - raise TheMlbStatsApiException(f"{response.status_code}: {response.reason}") - + status_code = response.status_code + + if 400 <= status_code <= 499: + self._logger.error(msg=logline_post.format( + 'Invalid Request', + status_code, + response.reason, + response.url, + )) + return MlbResult( + status_code=status_code, + message=response.reason, + data={}, + ) + + if 500 <= status_code <= 599: + self._logger.error(msg=logline_post.format( + 'Internal error occurred', + status_code, + response.reason, + response.url, + )) + raise TheMlbStatsApiException( + f"{status_code}: {response.reason}" + ) + + if not 200 <= status_code <= 299: + raise TheMlbStatsApiException( + f"{status_code}: {response.reason}" + ) + + self._logger.debug(msg=logline_post.format( + 'success', + status_code, + response.reason, + response.url, + )) + + if not response.content: + response_data = {} else: - raise TheMlbStatsApiException(f"{response.status_code}: {response.reason}") + try: + response_data = response.json() + except (ValueError, requests.JSONDecodeError) as exc: + self._logger.error(msg=(str(exc))) + raise TheMlbStatsApiException( + "Bad JSON in response" + ) from exc + + return MlbResult( + status_code, + message=response.reason, + data=response_data, + ) diff --git a/tests/test_mlb_dataadapter.py b/tests/test_mlb_dataadapter.py index a826a53..d4af196 100644 --- a/tests/test_mlb_dataadapter.py +++ b/tests/test_mlb_dataadapter.py @@ -1,10 +1,12 @@ -"""Offline characterization tests for MlbDataAdapter. +"""Offline regression tests for MlbDataAdapter. -These tests document current HTTP adapter behavior with requests-mock before -the transport is refactored in version 0.8.0. Known defects use strict xfail -markers so a future fix causes XPASS until the marker is removed. +These tests protect HTTP adapter response-handling behavior for version 0.8.0, +including successful 2xx handling, status classification before JSON decoding, +and empty successful bodies. """ +import logging + import requests import pytest @@ -38,6 +40,49 @@ def test_successful_json_response_returns_exact_values(adapter, requests_mock): assert result.data == {"sports": [{"id": 1, "name": "Major League Baseball"}]} +@pytest.mark.parametrize( + ("status_code", "reason", "payload"), + [ + (201, "Created", {"id": 42, "name": "created"}), + (299, "Custom Success", {"ok": True}), + ], +) +def test_other_successful_json_statuses_return_exact_values( + adapter, + requests_mock, + status_code, + reason, + payload, +): + requests_mock.get( + f"{BASE_URL}sports", + json=payload, + status_code=status_code, + reason=reason, + ) + + result = adapter.get(endpoint="sports") + + assert result.status_code == status_code + assert result.message == reason + assert result.data == payload + + +def test_empty_successful_response_returns_empty_data(adapter, requests_mock): + requests_mock.get( + f"{BASE_URL}sports", + text="", + status_code=204, + reason="No Content", + ) + + result = adapter.get(endpoint="sports") + + assert result.status_code == 204 + assert result.message == "No Content" + assert result.data == {} + + def test_ep_params_are_sent_as_query_parameters(adapter, requests_mock): requests_mock.get( f"{BASE_URL}teams/133/stats", @@ -107,6 +152,23 @@ def test_json_404_returns_empty_data(adapter, requests_mock): result = adapter.get(endpoint="teams/19990") assert result.status_code == 404 + assert result.message == "Not Found" + assert result.data == {} + + +def test_non_json_404_returns_empty_data(adapter, requests_mock): + requests_mock.get( + f"{BASE_URL}teams/19990", + text="Not Found", + status_code=404, + reason="Not Found", + headers={"Content-Type": "text/html"}, + ) + + result = adapter.get(endpoint="teams/19990") + + assert result.status_code == 404 + assert result.message == "Not Found" assert result.data == {} @@ -132,6 +194,21 @@ def test_json_500_raises_the_mlb_stats_api_exception(adapter, requests_mock): assert str(exc_info.value) == "500: Internal Server Error" +def test_html_502_raises_the_mlb_stats_api_exception(adapter, requests_mock): + requests_mock.get( + f"{BASE_URL}sports", + text="Bad Gateway", + status_code=502, + reason="Bad Gateway", + headers={"Content-Type": "text/html"}, + ) + + with pytest.raises(TheMlbStatsApiException, match=r"^502: Bad Gateway$") as exc_info: + adapter.get(endpoint="sports") + + assert str(exc_info.value) == "502: Bad Gateway" + + def test_connection_failure_raises_request_failed(adapter, requests_mock): requests_mock.get( f"{BASE_URL}sports", @@ -163,6 +240,17 @@ def test_invalid_json_on_successful_response_raises_bad_json(adapter, requests_m assert str(exc_info.value) == "Bad JSON in response" +def test_constructor_does_not_change_logger_level(): + logger = logging.Logger( + "mlbstatsapi-test", + level=logging.WARNING, + ) + + MlbDataAdapter(logger=logger) + + assert logger.level == logging.WARNING + + def test_mlb_result_optional_data_argument_omitted(): result = MlbResult(200, "OK") @@ -184,46 +272,3 @@ def test_mlb_result_type_coercion(): assert result.status_code == 200 assert result.message == "123" - - -@pytest.mark.xfail( - strict=True, - reason=( - "Adapter always JSON-decodes before status handling; " - "fix/http-adapter-correctness should return empty data for empty successful bodies" - ), -) -def test_empty_successful_response_returns_empty_data(adapter, requests_mock): - requests_mock.get( - f"{BASE_URL}sports", - text="", - status_code=204, - reason="No Content", - ) - - result = adapter.get(endpoint="sports") - - assert result.status_code == 204 - assert result.data == {} - - -@pytest.mark.xfail( - strict=True, - reason=( - "Adapter JSON-decodes before HTTP status handling; " - "fix/http-adapter-correctness should return empty data for non-JSON 404 bodies" - ), -) -def test_non_json_404_returns_empty_data(adapter, requests_mock): - requests_mock.get( - f"{BASE_URL}teams/19990", - text="Not Found", - status_code=404, - reason="Not Found", - headers={"Content-Type": "text/html"}, - ) - - result = adapter.get(endpoint="teams/19990") - - assert result.status_code == 404 - assert result.data == {} diff --git a/tests/test_mlb_result.py b/tests/test_mlb_result.py index 9443b1a..8ebc2b0 100644 --- a/tests/test_mlb_result.py +++ b/tests/test_mlb_result.py @@ -1,11 +1,9 @@ -"""Offline characterization tests for MlbResult. +"""Offline regression tests for MlbResult. -These tests document current constructor behavior before the HTTP adapter -correctness work in version 0.8.0. Known defects use strict xfail markers. +These tests protect MlbResult constructor behavior for version 0.8.0, +including independent data dictionaries and caller-owned dictionary safety. """ -import pytest - from mlbstatsapi import MlbResult @@ -68,33 +66,28 @@ def test_accepts_explicitly_supplied_dictionary(): assert result.data == {"teams": [{"id": 133}]} -@pytest.mark.xfail( - strict=True, - reason=( - "MlbResult uses a shared mutable default for data; " - "fix/http-adapter-correctness should give each instance its own dict" - ), -) def test_each_instance_gets_independent_data_dict(): first = MlbResult(200, "OK") second = MlbResult(200, "OK") + first.data["changed"] = True + + assert second.data == {} assert first.data is not second.data -@pytest.mark.xfail( - strict=True, - reason=( - "MlbResult deletes copyright from the caller-owned dictionary; " - "fix/http-adapter-correctness should leave the input unchanged" - ), -) def test_does_not_mutate_caller_owned_dictionary(): payload = { "copyright": "MLB", "sports": [], } - MlbResult(200, "OK", payload) + result = MlbResult(200, "OK", payload) - assert "copyright" in payload + assert payload == { + "copyright": "MLB", + "sports": [], + } + assert result.data == { + "sports": [], + }