From cf02538d2e7aa82367d6adc0a0db0c0ea2d8baa0 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Mon, 21 Sep 2026 16:07:51 -0400 Subject: [PATCH] fix(sports): ask the endpoint the league actually publishes for standings (#600) * fix(sports): ask the endpoint the league actually publishes for standings ESPNDataSource.fetch_standings tried /standings first regardless of league and fell back to /rankings only on a 404. College leagues answer /standings with a 200 that carries no poll, so the fallback never fired and the poll came back empty every time. Nothing failed; the rank badge simply never appeared, and anything keyed off rankings quietly did nothing. Endpoints are now ordered by whether the league publishes a poll, a 200 that lacks the key counts as a miss so a league answering both still ends up with whichever one carries the poll, and only a 404 is treated as routine -- it is how a league says it has none. A connection error, a timeout or an unparseable body is logged as an error again. This is the implementation the football, baseball and hockey boards already ship; core was the last copy still on the old one. Verified against live ESPN: mens-college-basketball returns a populated rankings key where it previously returned nothing, and nba still resolves from /standings alone. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 * fix(standings): stop the endpoint handler from swallowing its own bugs Addresses both CodeRabbit findings on #600. The handler caught `Exception`, so an AttributeError or TypeError raised while *inspecting* the payload was indistinguishable from an endpoint that failed. The loop would move on and, if the other endpoint had nothing either, return {} -- silently dropping rankings for a league that has them. That is the precise failure this function was written to fix, so the handler was able to reintroduce it. Only the request is guarded now. `requests.RequestException` covers the transport failures and `ValueError` covers a body that will not parse; payload inspection happens after the handler, where a bug surfaces instead of being logged as a missing poll. A non-dict payload is treated as a miss explicitly rather than by tripping over `.get`. Tests: the fallback paths had no coverage -- the old single-endpoint code would have passed the suite unchanged. Added order assertions for both league kinds, a 200-without-a-poll fall-through, 404 and non-404 recovery, a non-object payload, and a guard proving a bug is no longer swallowed. `test_fetch_standings_returns_empty_on_error` faked a transport failure with a bare `Exception`, which only passed because the handler caught everything. It now raises ConnectionError, which is what actually happens. Verified by mutation: restoring standings-first fails 5 tests, restoring the catch-all fails the bug-not-swallowed guard. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 --------- Co-authored-by: Claude Opus 5 (1M context) --- src/base_classes/data_sources.py | 89 ++++++++++++++++++--------- test/test_data_sources.py | 100 ++++++++++++++++++++++++++++++- 2 files changed, 160 insertions(+), 29 deletions(-) diff --git a/src/base_classes/data_sources.py b/src/base_classes/data_sources.py index ac60f638..80c6177a 100644 --- a/src/base_classes/data_sources.py +++ b/src/base_classes/data_sources.py @@ -114,35 +114,68 @@ class ESPNDataSource(DataSource): return [] def fetch_standings(self, sport: str, league: str) -> Dict: - """Fetch standings from ESPN API.""" - # Try standings endpoint first (for professional leagues like NFL, NBA, etc.) - try: - url = f"{self.base_url}/{sport}/{league}/standings" - response = self.session.get(url, headers=self.get_headers(), timeout=15) - response.raise_for_status() - - data = response.json() - self.logger.debug(f"Fetched standings for {sport}/{league}") + """Fetch standings, or the poll for leagues that have one. + + Order matters and used to be wrong. College leagues publish a poll at + /rankings and a records table at /standings; professional leagues have + only /standings. The old code tried /standings first and fell back to + /rankings only on a 404 -- but college /standings answers 200, so the + fallback never fired and college rankings came back empty forever. + Nothing failed; the AP rank badge simply never appeared, and anything + else keyed off rankings quietly did nothing. + + A 200 that lacks the key is treated as a miss, so a league answering + both endpoints still ends up with whichever one actually carries a poll. + """ + league_name = (league or "").lower() + wants_poll = "college" in league_name or "ncaa" in league_name + endpoints = ["rankings", "standings"] if wants_poll else ["standings", "rankings"] + + for endpoint in endpoints: + url = f"{self.base_url}/{sport}/{league}/{endpoint}" + # Only the request is guarded. Inspecting the payload happens + # below, outside the handler, so that a bug in this method cannot + # be mistaken for an endpoint that failed -- that mistake would + # silently drop rankings for a league that has them, which is the + # exact failure this function was written to fix. + try: + response = self.session.get( + url, headers=self.get_headers(), timeout=15 + ) + response.raise_for_status() + data = response.json() + except (requests.RequestException, ValueError) as e: + status = getattr(getattr(e, "response", None), "status_code", None) + # Only a 404 is routine -- it is how a league says "no poll + # here". Everything else is worth an error, and `status is + # None` covers the ones that matter most: ConnectionError, + # Timeout, a body that would not parse. Silencing those left a + # board that could not reach ESPN with one debug line, and the + # ranked filter running on an empty table. + if status != 404: + self.logger.error( + f"Error fetching {endpoint} from ESPN for " + f"{sport}/{league}: {e}" + ) + continue + + if not isinstance(data, dict): + # A list or a bare string is not something the callers can + # read. Treat it as a miss so the other endpoint still gets a + # turn, but say so -- this means ESPN changed shape. + self.logger.error( + f"Unexpected {endpoint} payload for {sport}/{league}: " + f"got {type(data).__name__}, expected an object" + ) + continue + if endpoint == "rankings" and not data.get("rankings"): + continue + self.logger.debug(f"Fetched {endpoint} for {sport}/{league}") return data - except Exception as e: - # If standings doesn't exist, try rankings (for college sports) - if hasattr(e, 'response') and hasattr(e.response, 'status_code') and e.response.status_code == 404: - try: - url = f"{self.base_url}/{sport}/{league}/rankings" - response = self.session.get(url, headers=self.get_headers(), timeout=15) - response.raise_for_status() - - data = response.json() - self.logger.debug(f"Fetched rankings for {sport}/{league}") - return data - except Exception: - # Both endpoints failed - standings/rankings may not be available for this sport/league - self.logger.debug(f"Standings/rankings not available for {sport}/{league} from ESPN API") - return {} - else: - # Non-404 error - log at debug level since standings are optional - self.logger.debug(f"Error fetching standings from ESPN for {sport}/{league}: {e}") - return {} + self.logger.debug( + f"Standings/rankings not available for {sport}/{league} from ESPN API" + ) + return {} class MLBAPIDataSource(DataSource): diff --git a/test/test_data_sources.py b/test/test_data_sources.py index 9aad0588..d7366923 100644 --- a/test/test_data_sources.py +++ b/test/test_data_sources.py @@ -92,10 +92,108 @@ class TestESPNDataSource: assert result == payload def test_fetch_standings_returns_empty_on_error(self): - with patch.object(self.source.session, "get", side_effect=Exception("error")): + # A transport failure is a RequestException, not a bare Exception. + # The old stand-in passed only because the handler caught everything, + # including bugs in the method under test. + with patch.object(self.source.session, "get", + side_effect=requests.ConnectionError("error")): result = self.source.fetch_standings("football", "nfl") assert result == {} + # ------------------------------------------------------------------ + # fetch_standings endpoint selection + # + # College leagues publish a poll at /rankings and a records table at + # /standings; professional leagues have only /standings. Probing them in + # the wrong order still returns 200 -- just without a poll in it -- so + # nothing failed and the rank badge simply never appeared. Order is the + # behaviour here, so these tests assert it directly. + # ------------------------------------------------------------------ + + @staticmethod + def _requested_endpoints(mock_get): + """The endpoint names probed, in the order they were requested.""" + return [call.args[0].rsplit("/", 1)[-1] for call in mock_get.call_args_list] + + def test_professional_league_asks_standings_first(self): + payload = {"standings": []} + with patch.object(self.source.session, "get", + return_value=_mock_response(payload)) as mock_get: + result = self.source.fetch_standings("football", "nfl") + assert result == payload + assert self._requested_endpoints(mock_get) == ["standings"] + + def test_college_league_asks_rankings_first(self): + poll = {"rankings": [{"name": "AP Top 25"}]} + with patch.object(self.source.session, "get", + return_value=_mock_response(poll)) as mock_get: + result = self.source.fetch_standings("football", "college-football") + assert result == poll + assert self._requested_endpoints(mock_get) == ["rankings"] + + def test_rankings_200_without_a_poll_falls_through_to_standings(self): + """A 200 is not the same as an answer. + + This is the case the old code could not see: the endpoint responded, + so nothing raised, but the body carried no poll. + """ + empty_poll = _mock_response({"rankings": []}) + table = _mock_response({"standings": [{"entries": []}]}) + with patch.object(self.source.session, "get", + side_effect=[empty_poll, table]) as mock_get: + result = self.source.fetch_standings( + "basketball", "mens-college-basketball") + assert result == {"standings": [{"entries": []}]} + assert self._requested_endpoints(mock_get) == ["rankings", "standings"] + + def test_404_on_the_first_endpoint_falls_through_quietly(self): + missing = _mock_response({}, status_code=404) + table = _mock_response({"standings": []}) + with patch.object(self.source.session, "get", + side_effect=[missing, table]) as mock_get: + result = self.source.fetch_standings("baseball", "college-baseball") + assert result == {"standings": []} + assert self._requested_endpoints(mock_get) == ["rankings", "standings"] + + def test_recovers_from_a_non_404_failure_on_the_first_endpoint(self): + table = _mock_response({"standings": [{"entries": []}]}) + with patch.object(self.source.session, "get", + side_effect=[requests.ConnectionError("reset"), table]) as mock_get: + result = self.source.fetch_standings("football", "college-football") + assert result == {"standings": [{"entries": []}]} + assert self._requested_endpoints(mock_get) == ["rankings", "standings"] + + def test_both_endpoints_failing_returns_empty(self): + with patch.object(self.source.session, "get", + side_effect=requests.ConnectionError("down")) as mock_get: + result = self.source.fetch_standings("football", "nfl") + assert result == {} + assert self._requested_endpoints(mock_get) == ["standings", "rankings"] + + def test_a_non_object_payload_is_treated_as_a_miss(self): + odd = _mock_response(["not", "an", "object"]) + table = _mock_response({"standings": []}) + with patch.object(self.source.session, "get", + side_effect=[odd, table]) as mock_get: + result = self.source.fetch_standings("football", "college-football") + assert result == {"standings": []} + assert self._requested_endpoints(mock_get) == ["rankings", "standings"] + + def test_a_bug_in_this_method_is_not_swallowed_as_a_failed_endpoint(self): + """The guard for the narrowed handler. + + An error raised while reading the payload used to be caught by the + endpoint handler and reported as 'no poll here', which would silently + drop rankings for a league that has them. It must surface instead. + """ + boom = Mock(spec=requests.Response) + boom.status_code = 200 + boom.raise_for_status = Mock() + boom.json.side_effect = TypeError("a bug, not a network failure") + with patch.object(self.source.session, "get", return_value=boom): + with pytest.raises(TypeError): + self.source.fetch_standings("football", "nfl") + def test_base_url_set_correctly(self): assert "espn.com" in self.source.base_url