diff --git a/backend/app/services/gallery_dl.py b/backend/app/services/gallery_dl.py index 9d59f3c..9d3dc92 100644 --- a/backend/app/services/gallery_dl.py +++ b/backend/app/services/gallery_dl.py @@ -40,6 +40,12 @@ class ErrorType(StrEnum): HTTP_ERROR = "http_error" UNSUPPORTED_URL = "unsupported_url" VALIDATION_FAILED = "validation_failed" + # Native Patreon ingester only (plan #697): a response parsed as JSON but + # didn't match the JSON:API shape the ingester depends on (missing `data`, + # media lacking `file_name`/`url`, etc.). Distinct from AUTH_ERROR — the fix + # is updating the ingester's field-set/parser, not rotating credentials. The + # contract test guards against silently shipping this. + API_DRIFT = "api_drift" # Run made real progress (downloaded ≥1 file) but did not finish in the # subprocess budget. Distinct from UNKNOWN_ERROR — the downstream status # mapping classifies this as "ok" because the next tick continues. diff --git a/backend/app/services/patreon_client.py b/backend/app/services/patreon_client.py index 28bfab4..a3685a9 100644 --- a/backend/app/services/patreon_client.py +++ b/backend/app/services/patreon_client.py @@ -77,16 +77,34 @@ _MAX_EXT_LEN = 16 class PatreonAPIError(Exception): - """Base for native Patreon client failures.""" + """Base for native Patreon client failures. + + `status_code` carries the HTTP status when the failure was an HTTP response + (None for transport-level / parse failures), so the ingester can map it to a + DownloadResult.error_type (429 → rate_limited, 404 → not_found, …). + """ + + def __init__(self, message: str, *, status_code: int | None = None): + super().__init__(message) + self.status_code = status_code + + +class PatreonAuthError(PatreonAPIError): + """Authentication / authorization failure — missing or expired session + cookies, an insufficient pledge tier, or an HTML login/challenge page served + where JSON was expected. DISTINCT from drift: the fix is rotating the + credential, not updating the ingester. Maps to error_type 'auth_error'. + """ class PatreonDriftError(PatreonAPIError): - """A response did not match the JSON:API shape we depend on. + """A JSON response did not match the JSON:API shape we depend on. - Raised for: a non-JSON / HTML-login response where JSON was expected, a - missing top-level `data` list, or a media resource lacking the - `file_name`/`url` fields we resolve against. Fail loud so the later import - step flags API drift instead of silently importing nothing. + Raised for: a missing top-level `data` list, `data` not a list, or a media + resource lacking the `file_name`/`url` fields we resolve against. Fail loud + so the import step flags API drift (ingester needs update) instead of + silently importing nothing. An HTML-login / non-JSON body is auth, not + drift — that raises PatreonAuthError. """ @@ -221,20 +239,32 @@ class PatreonClient: f"Patreon posts request failed (campaign_id={campaign_id}): {exc}" ) from exc + if resp.status_code in (401, 403): + # Auth rejected — expired/missing cookies or an insufficient tier. + # Actionable as "rotate credentials", so it's auth, not drift/http. + raise PatreonAuthError( + f"Patreon posts API returned HTTP {resp.status_code} — auth " + f"rejected (cookies expired or tier insufficient; " + f"campaign_id={campaign_id})", + status_code=resp.status_code, + ) if resp.status_code != 200: raise PatreonAPIError( f"Patreon posts API returned HTTP {resp.status_code} " - f"(campaign_id={campaign_id})" + f"(campaign_id={campaign_id})", + status_code=resp.status_code, ) try: payload = resp.json() except ValueError as exc: # A non-JSON body here is almost always the HTML login/challenge - # page served when cookies are missing/expired — that is drift - # from the JSON:API contract, not a transient network error. - raise PatreonDriftError( + # page served when cookies are missing/expired — that is an AUTH + # failure (rotate cookies), not API drift (update the ingester) and + # not a transient network error. + raise PatreonAuthError( "Patreon posts API returned a non-JSON response (likely an " - f"HTML login/challenge page; campaign_id={campaign_id}): {exc}" + f"HTML login/challenge page — session expired; " + f"campaign_id={campaign_id}): {exc}" ) from exc return payload diff --git a/backend/app/services/patreon_ingester.py b/backend/app/services/patreon_ingester.py index 6b26f8c..5409e81 100644 --- a/backend/app/services/patreon_ingester.py +++ b/backend/app/services/patreon_ingester.py @@ -51,6 +51,7 @@ from .gallery_dl import DownloadResult, ErrorType from .patreon_client import ( MediaItem, PatreonAPIError, + PatreonAuthError, PatreonClient, PatreonDriftError, ) @@ -242,7 +243,9 @@ class PatreonIngester: break else: reached_bottom = True - except (PatreonDriftError, PatreonAPIError) as exc: + except PatreonAPIError as exc: + # Base of PatreonAuthError + PatreonDriftError — catches every + # client-level failure; _failure_result maps it to a typed error. return self._failure_result(exc, _result) if errors: @@ -294,24 +297,35 @@ class PatreonIngester: def _failure_result(self, exc: Exception, _result) -> DownloadResult: """Map a client-level exception to a loud, typed failed DownloadResult. - Step 3 keeps this deliberately coarse — auth-ish drift vs. everything - else — so the integration is correct end-to-end. Step 4 (drift detection - + error categorization) refines the typed-error mapping and adds the - contract test. Either way we NEVER return a silent zero-download - "success": the whole point of the native ingester is to fail red when - Patreon's API shape or our auth changes. + We NEVER return a silent zero-download "success" — the whole point of the + native ingester is to fail RED when Patreon's API shape or our auth + changes. The typed mapping lets FailingSourcesCard render the right chip + and tells the operator what to do: + - PatreonAuthError → AUTH_ERROR (rotate cookies) + - PatreonDriftError → API_DRIFT (ingester field-set/parser needs update) + - HTTP 429 / 404 → RATE_LIMITED / NOT_FOUND + - other HTTP status → HTTP_ERROR; transport failure → NETWORK_ERROR + + PatreonAuthError and PatreonDriftError both subclass PatreonAPIError, so + they must be matched before the generic HTTP/transport fallthrough. """ message = str(exc) - lowered = message.lower() - if isinstance(exc, PatreonDriftError): - if "login" in lowered or "challenge" in lowered or "auth" in lowered: - error_type = ErrorType.AUTH_ERROR - else: - error_type = ErrorType.UNKNOWN_ERROR + if isinstance(exc, PatreonAuthError): + error_type = ErrorType.AUTH_ERROR + elif isinstance(exc, PatreonDriftError): + error_type = ErrorType.API_DRIFT message = f"Patreon API changed — ingester needs update: {message}" - else: - error_type = ErrorType.NETWORK_ERROR - log.warning("Patreon ingest failed: %s", message) + else: # generic PatreonAPIError: HTTP non-2xx (status_code set) or transport + status = getattr(exc, "status_code", None) + if status == 429: + error_type = ErrorType.RATE_LIMITED + elif status == 404: + error_type = ErrorType.NOT_FOUND + elif status is not None: + error_type = ErrorType.HTTP_ERROR + else: + error_type = ErrorType.NETWORK_ERROR + log.warning("Patreon ingest failed (%s): %s", error_type.value, message) return _result( success=False, return_code=1, error_type=error_type, error_message=message, diff --git a/frontend/src/components/subscriptions/FailingSourcesCard.vue b/frontend/src/components/subscriptions/FailingSourcesCard.vue index 024a4df..f456904 100644 --- a/frontend/src/components/subscriptions/FailingSourcesCard.vue +++ b/frontend/src/components/subscriptions/FailingSourcesCard.vue @@ -93,6 +93,7 @@ const ERROR_TYPE_COLOR = { unsupported_url: 'error', http_error: 'error', unknown_error: 'error', + api_drift: 'error', partial: 'info', tier_limited: 'info', no_new_content: 'info', @@ -108,6 +109,7 @@ const ERROR_TYPE_HINT = { http_error: 'Generic HTTP error — see Logs.', unsupported_url: 'gallery-dl does not support this URL pattern.', unknown_error: 'Could not classify — see Logs.', + api_drift: 'Patreon changed its API shape — the native ingester needs a code update.', } function errorTypeColor(t) { return ERROR_TYPE_COLOR[t] || 'error' } function errorTypeHint(t) { return ERROR_TYPE_HINT[t] || '' } diff --git a/tests/test_patreon_client.py b/tests/test_patreon_client.py index 02bd328..268c3bb 100644 --- a/tests/test_patreon_client.py +++ b/tests/test_patreon_client.py @@ -12,6 +12,8 @@ import pytest from backend.app.services.patreon_client import ( MediaItem, + PatreonAPIError, + PatreonAuthError, PatreonClient, PatreonDriftError, parse_cursor_from_url, @@ -184,3 +186,55 @@ def test_media_referenced_but_absent_raises_drift(client): } with pytest.raises(PatreonDriftError): client.extract_media(post, {}) + + +# -- HTTP status classification (plan #697 step 4) ------------------------- + + +class _FakeResp: + def __init__(self, status_code, *, json_data=None, raise_json=False): + self.status_code = status_code + self._json_data = json_data + self._raise_json = raise_json + + def json(self): + if self._raise_json: + raise ValueError("Expecting value: line 1 column 1") + return self._json_data + + +def _client_returning(monkeypatch, resp): + client = PatreonClient(cookies_path=None) + monkeypatch.setattr(client._session, "get", lambda *a, **k: resp) + return client + + +@pytest.mark.parametrize("status", [401, 403]) +def test_fetch_auth_status_raises_auth_error(monkeypatch, status): + client = _client_returning(monkeypatch, _FakeResp(status)) + with pytest.raises(PatreonAuthError) as ei: + client._fetch("5555", None) + assert ei.value.status_code == status + + +def test_fetch_non_json_body_is_auth_not_drift(monkeypatch): + # An HTML login/challenge page (non-JSON) means the session expired — auth, + # actionable as "rotate cookies", NOT API drift. + client = _client_returning(monkeypatch, _FakeResp(200, raise_json=True)) + with pytest.raises(PatreonAuthError): + client._fetch("5555", None) + + +@pytest.mark.parametrize("status", [404, 429, 500]) +def test_fetch_other_status_raises_api_error_with_code(monkeypatch, status): + client = _client_returning(monkeypatch, _FakeResp(status)) + with pytest.raises(PatreonAPIError) as ei: + client._fetch("5555", None) + assert ei.value.status_code == status + # Not auth — the ingester maps these by status, not to auth_error. + assert not isinstance(ei.value, PatreonAuthError) + + +def test_fetch_ok_returns_payload(monkeypatch): + client = _client_returning(monkeypatch, _FakeResp(200, json_data={"data": []})) + assert client._fetch("5555", None) == {"data": []} diff --git a/tests/test_patreon_contract.py b/tests/test_patreon_contract.py new file mode 100644 index 0000000..1f6a520 --- /dev/null +++ b/tests/test_patreon_contract.py @@ -0,0 +1,72 @@ +"""Patreon API contract test (native ingester build step 4). + +The drift tripwire the plan calls for: if the field-set we SEND or the parser we +resolve responses against drifts from what real Patreon returns, this build goes +RED — instead of the ingester silently importing nothing in production (the exact +failure mode the native ingester exists to escape). + +Two halves: + - the recorded `/api/posts` fixture must still parse end-to-end (no drift), and + - the request params must still carry every field the parser depends on (the + fixture already has the data, so trimming the request would NOT trip the + parse half — this half guards the request shape directly). + +Pure: no network, no DB. +""" + +import json +from pathlib import Path + +from backend.app.services.patreon_client import MediaItem, PatreonClient + +_FIXTURE = Path(__file__).parent / "fixtures" / "patreon_posts_page1.json" + +# Pinned media total across the 4 fixture posts: 1001→2 (gallery, image_large +# cover deduped), 1002→1 (attachment+postfile collapse), 1003→1 (inline content +# img), 1004→1 (video postfile). A parser change that drops or doubles media +# trips this. +_EXPECTED_MEDIA_TOTAL = 5 +_KNOWN_KINDS = {"images", "image_large", "attachments", "postfile", "content"} + + +def test_recorded_fixture_parses_end_to_end(): + response = json.loads(_FIXTURE.read_text()) + client = PatreonClient(cookies_path=None) + + client._validate_response(response) # top-level shape must hold + index = client._transform(response) + + total = 0 + for post in response["data"]: + items = client.extract_media(post, index) # must not raise drift + for m in items: + assert isinstance(m, MediaItem) + assert m.url, f"empty url in post {post['id']}" + assert m.filename, f"empty filename in post {post['id']}" + assert m.kind in _KNOWN_KINDS, f"unknown kind {m.kind!r}" + assert m.post_id == post["id"] + total += len(items) + + assert total == _EXPECTED_MEDIA_TOTAL + + +def test_request_field_set_carries_parser_dependencies(): + """The params we SEND must keep requesting the fields the parser resolves + against. Trimming `fields[media]=file_name`, an `include` relationship, or a + `fields[post]` key would make real responses parse to nothing — caught here, + not by the fixture parse above.""" + params = PatreonClient(cookies_path=None)._params("5555", None) + + media_fields = params["fields[media]"] + assert "file_name" in media_fields + assert "image_urls" in media_fields or "download_url" in media_fields + + includes = params["include"] + for rel in ("images", "attachments_media", "media"): + assert rel in includes, f"missing include relationship {rel}" + + post_fields = params["fields[post]"] + for field in ("content", "post_file", "image"): + assert field in post_fields, f"missing post field {field}" + + assert params["filter[campaign_id]"] == "5555" diff --git a/tests/test_patreon_ingester.py b/tests/test_patreon_ingester.py index 17ac2cf..ef356d7 100644 --- a/tests/test_patreon_ingester.py +++ b/tests/test_patreon_ingester.py @@ -13,7 +13,12 @@ from sqlalchemy.orm import sessionmaker from backend.app.models import Artist, PatreonSeenMedia, Source from backend.app.services.gallery_dl import ErrorType -from backend.app.services.patreon_client import MediaItem, PatreonDriftError +from backend.app.services.patreon_client import ( + MediaItem, + PatreonAPIError, + PatreonAuthError, + PatreonDriftError, +) from backend.app.services.patreon_downloader import MediaOutcome from backend.app.services.patreon_ingester import PatreonIngester, _ledger_key @@ -302,20 +307,63 @@ async def test_mark_seen_is_idempotent(source_id, sync_engine, tmp_path): assert _count_ledger(sync_engine, source_id) == 1 -@pytest.mark.asyncio -async def test_drift_fails_loud(source_id, sync_engine, tmp_path): - client = _FakeClient([], raise_exc=PatreonDriftError("missing top-level 'data' key")) - downloader = _FakeDownloader(tmp_path) - ing = _ingester(sync_engine, tmp_path, client, downloader) - - result = ing.run( +def _run_with_client_error(source_id, sync_engine, tmp_path, exc): + client = _FakeClient([], raise_exc=exc) + ing = _ingester(sync_engine, tmp_path, client, _FakeDownloader(tmp_path)) + return ing.run( source_id=source_id, campaign_id="c1", artist_slug="ingest", url="https://patreon.com/ingest", mode="tick", ) + + +@pytest.mark.asyncio +async def test_drift_maps_to_api_drift_and_fails_loud(source_id, sync_engine, tmp_path): + result = _run_with_client_error( + source_id, sync_engine, tmp_path, + PatreonDriftError("missing top-level 'data' key"), + ) assert result.success is False + assert result.error_type == ErrorType.API_DRIFT assert "Patreon API changed" in (result.error_message or "") +@pytest.mark.asyncio +async def test_auth_error_maps_to_auth_error(source_id, sync_engine, tmp_path): + result = _run_with_client_error( + source_id, sync_engine, tmp_path, + PatreonAuthError("HTTP 403 — auth rejected", status_code=403), + ) + assert result.success is False + assert result.error_type == ErrorType.AUTH_ERROR + + +@pytest.mark.asyncio +async def test_rate_limit_status_maps_to_rate_limited(source_id, sync_engine, tmp_path): + result = _run_with_client_error( + source_id, sync_engine, tmp_path, + PatreonAPIError("HTTP 429", status_code=429), + ) + assert result.error_type == ErrorType.RATE_LIMITED + + +@pytest.mark.asyncio +async def test_not_found_status_maps_to_not_found(source_id, sync_engine, tmp_path): + result = _run_with_client_error( + source_id, sync_engine, tmp_path, + PatreonAPIError("HTTP 404", status_code=404), + ) + assert result.error_type == ErrorType.NOT_FOUND + + +@pytest.mark.asyncio +async def test_transport_error_maps_to_network_error(source_id, sync_engine, tmp_path): + result = _run_with_client_error( + source_id, sync_engine, tmp_path, + PatreonAPIError("connection reset"), # no status_code → transport + ) + assert result.error_type == ErrorType.NETWORK_ERROR + + def test_ledger_key_video_has_no_filehash(): video = MediaItem( url="https://stream.mux.com/abc.m3u8", filename="clip.mp4",