Skip to content

fix: surface nba api http response errors - #679

Open
vikramdurai wants to merge 1 commit into
swar:masterfrom
vikramdurai:issue-678-live-http-error-handling
Open

fix: surface nba api http response errors#679
vikramdurai wants to merge 1 commit into
swar:masterfrom
vikramdurai:issue-678-live-http-error-handling

Conversation

@vikramdurai

@vikramdurai vikramdurai commented Jun 16, 2026

Copy link
Copy Markdown

Summary

Fixes #678.

This updates the shared HTTP layer so NBA API response failures are surfaced before JSON parsing turns them into misleading JSONDecodeErrors. It also adds the live CDN Referer header needed for current nba_api.live requests.

This includes the live header fix from #671, then extends the error handling beyond the narrower approaches in #659 and #657: HTTP status is checked before parsing, structured response context is preserved, partial-content responses are rejected, and JSON decode failures include useful response metadata. #675 touches session/test isolation but does not address the HTTP status / non-JSON response handling in #678.

Changes

  • Add NBAError, NBAHTTPError, NBARequestError, and NBAJSONDecodeError
  • Raise NBAHTTPError for non-2xx responses before parsing response bodies
  • Raise NBAHTTPError for 206 Partial Content
  • Wrap JSON parse failures in NBAJSONDecodeError with status, URL, content type, body preview, and reason hint
  • Wrap request/transport failures in NBARequestError
  • Add Referer: https://www.nba.com/ to live endpoint default headers
  • Add unit coverage for observed NBA/CDN response shapes

Verification

PYTHONPATH=src /private/tmp/nba_api_contrib_venv/bin/python -m pytest tests/unit
PYTHONPATH=src /private/tmp/nba_api_contrib_venv/bin/python -m ruff check src tests/unit/http/test_legacy_debug_usage.py

Result:

  • 528 passed, 1 warning
  • Ruff passed

Also manually verified live behavior:

  • ScoreBoard() succeeds with default headers
  • Removing the live Referer raises NBAHTTPError with 403 text/html access_denied instead of surfacing as a JSON parse failure

@vikramdurai
vikramdurai requested a review from swar as a code owner June 16, 2026 14:09
@DigitalCurrensy

DigitalCurrensy commented Jul 26, 2026

Copy link
Copy Markdown

Applied this branch on top of master and ran it against the existing suite. The design direction is right — checking HTTP status before parsing catches failures the narrower approaches structurally can't see, and reason_hint + content_type are genuinely useful for triage. Three findings, in severity order.

1. except json.JSONDecodeError stops working

nba_api has raised bare json.JSONDecodeError from NBAResponse.get_dict() since 2018 (#5), so callers in the wild catch it. NBAJSONDecodeError(NBAError, ValueError) doesn't inherit from json.JSONDecodeError, so those handlers break.

Reproduction:

r = NBAResponse(response="<html>403 Access Denied</html>", status_code=403,
                url="https://stats.nba.com/x", headers={"Content-Type": "text/html"})
try:
    r.get_dict()
except json.JSONDecodeError:
    print("caught")

Result:

except json.JSONDecodeError  -> ESCAPED as NBAJSONDecodeError
except ValueError            -> CAUGHT

One-line fix that keeps everything else in this PR intact:

class NBAJSONDecodeError(NBAError, json.JSONDecodeError):
    pass

json.JSONDecodeError already subclasses ValueError, so that's strictly wider than the current base list — nothing that catches it today stops catching it.

One caveat if you take that change: json.JSONDecodeError overrides __reduce__ to return only (cls, (msg, doc, pos)), dropping __dict__. So subclassing it would silently lose status_code, url, content_type, reason_hint, and body_preview through pickle and copy.deepcopy — relevant for anyone running requests in multiprocessing or celery workers. An explicit __reduce__ on NBAError avoids it. As written today this PR has no such problem; it only appears if you adopt the inheritance change above.

2. NBAHTTPError escapes except ValueError too

NBAHTTPError(NBAError) inherits only from Exception, and it's raised for any non-2xx before parsing. So on a 403 — the most common real failure against stats.nba.com and cdn.nba.com — existing error handling escapes entirely, not just JSONDecodeError handlers:

issubclass(NBAHTTPError, ValueError) -> False

Worth deciding deliberately rather than by inheritance default. If the intent is that a transport/status failure is a different category from a parse failure, that's defensible — but it is a behaviour change for every caller currently wrapping endpoint construction in except ValueError.

3. Response body and URL query string end up in the exception message

_get_body_preview puts up to BODY_PREVIEW_LENGTH = 300 characters of the raw body into the exception message, _format_error_context appends the full URL, and self.headers retains the whole response header mapping. Anything that logs the traceback — Sentry, CloudWatch, plain logging.exception — captures all three:

leaky = NBAResponse(
    response='NOT-JSON session=SECRET_TOKEN_abc123 user_email=someone@example.com',
    status_code=403,
    url="https://stats.nba.com/x?api_key=SUPERSECRET",
    headers={"Content-Type": "text/plain",
             "Set-Cookie": "session=SECRET_TOKEN_abc123; HttpOnly"},
)
message contains body bytes: True
message contains url query : True
Set-Cookie retained on exc : True

Suggestions, cheapest first: strip the query string in _format_error_context; keep body_preview on the attribute but leave it out of the message, or shorten it considerably; and allowlist the headers you retain rather than storing the mapping wholesale.

Semver note

This is labelled fix, and [tool.semantic_release.commit_parser_options] maps fix to a patch bump. If finding 1 or 2 ships as-is, 1.11.4 → 1.11.5 would break exception handling that's been in place since 2018. Either feat! or fixing the inheritance resolves that.

Happy to open a PR against this branch for any of these if that's easier than patching them inline — whichever you prefer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@swar swar Awaiting requested review from swar swar is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: nba_api.live silently swallows 403s and raises a misleading JSONDecodeError

2 participants

@vikramdurai @DigitalCurrensy