From 8a7606e20df30f86bd0efb012d57c7e925cbf944 Mon Sep 17 00:00:00 2001 From: jay Date: Tue, 30 Jun 2026 14:01:11 -0400 Subject: [PATCH] images: fix two fetcher bugs + add source-level image-rights policy (Codex) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fetcher (the two remaining bugs Codex found): - Real redirects are now followed. _NoRedirect makes urllib RAISE HTTPError on 3xx, so the old status-branch was dead code (mocked tests masked it). Handle 301/302/303/307/308 HTTPError as redirects (re-validate the destination); classify 4xx≠429 as PERMANENT (negative-cached), 429/5xx/network as transient. Real-opener redirect + 404/5xx tests. - The megapixel ceiling is now enforced: explicit `w*h > _MAX_PIXELS` check BEFORE load() (Pillow only warns at MAX_IMAGE_PIXELS). Test with a lowered ceiling. Image-rights policy (per Codex + owner decision — only cache what's cleared): - sources.image_policy: 'cache' (re-host a downscaled copy — license/permission/PD only), 'remote' (hotlink the publisher's image — the conservative DEFAULT), 'none' (no image). - newsimg.display_url resolves the display URL per policy; applied in Article.from_row so feed/brief/history return the right URL, and in share.py (og/twitter still reference the publisher's own image, never re-hosted). warm() + /api/img both gated on 'cache'. - Frontend uses the server-resolved image_url (reverted the hardcoded /api/img); the graceful retry covers remote hotlinks too. Admin: per-source image-policy selector + POST /api/admin/sources/{id}/image-policy. Default 'remote' → nothing re-hosted until a source is explicitly cleared. 445 backend + 36 frontend tests pass. Co-Authored-By: Claude Opus 4.8 --- .../src/lib/components/ArticleCard.svelte | 4 +- frontend/src/routes/+page.svelte | 7 +- frontend/src/routes/admin/+page.svelte | 18 +++ goodnews/api.py | 29 +++- goodnews/db.py | 5 + goodnews/newsimg.py | 62 ++++++--- goodnews/queries.py | 2 + goodnews/share.py | 26 ++-- tests/test_admin.py | 18 +++ tests/test_newsimg.py | 129 ++++++++++++++---- 10 files changed, 238 insertions(+), 62 deletions(-) diff --git a/frontend/src/lib/components/ArticleCard.svelte b/frontend/src/lib/components/ArticleCard.svelte index edaded5..a69a064 100644 --- a/frontend/src/lib/components/ArticleCard.svelte +++ b/frontend/src/lib/components/ArticleCard.svelte @@ -100,8 +100,8 @@ > {#if showImage} - - + { failed = true; onimageerror?.(); }} /> {:else if usePlaceholder} diff --git a/frontend/src/routes/+page.svelte b/frontend/src/routes/+page.svelte index 79cdb71..5d6d3c8 100644 --- a/frontend/src/routes/+page.svelte +++ b/frontend/src/routes/+page.svelte @@ -112,9 +112,10 @@ const q = P.param(prefs.data); // the reader's boundaries (excluded topics, ceilings) try { const it = (await getJSON(`/api/brief?limit=1${homeq}${q ? '&' + q : ''}`))?.items?.[0]; - // Serve the photo from our own cached/downscaled copy (/api/img/) rather than - // hotlinking the source — keep image_url only as the "has a photo?" signal. - if (it) news = { id: it.id, title: it.title, summary: it.summary || it.description || '', image: it.image_url ? `/api/img/${it.id}` : null, topic: it.topic || null, source_read_minutes: it.source_read_minutes }; + // image_url is already resolved server-side per the source's image policy (our + // cached copy for cleared sources, else the publisher URL); the probe below loads + // it with retries either way. + if (it) news = { id: it.id, title: it.title, summary: it.summary || it.description || '', image: it.image_url || null, topic: it.topic || null, source_read_minutes: it.source_read_minutes }; // News images are hotlinked from the source, so a single fetch can transiently // fail (slow CDN, rate limit, hiccup) and leave a blank plate until you refresh. // Probe with a couple of retries and reveal the photo only once it's truly loaded diff --git a/frontend/src/routes/admin/+page.svelte b/frontend/src/routes/admin/+page.svelte index a0de8f5..b429694 100644 --- a/frontend/src/routes/admin/+page.svelte +++ b/frontend/src/routes/admin/+page.svelte @@ -511,6 +511,16 @@ } catch (e) { s._artErr = e?.message || 'Could not set the override.'; } } + // Per-source image rights policy: 'cache' (re-host a downscaled copy — only for sources + // cleared by license/permission/PD), 'remote' (hotlink the publisher image — default), + // 'none' (no image). Conservative by default; caching is opt-in. + async function setImagePolicy(s, policy) { + const prev = s.image_policy; + s.image_policy = policy; // optimistic + try { await postJSON(`/api/admin/sources/${s.id}/image-policy`, { policy }); } + catch (e) { s.image_policy = prev; s._artErr = e?.message || 'Could not set the image policy.'; } + } + // --- Source candidates: supervised "add a source" pipeline --- let candidates = $state([]); let newFeedUrl = $state(''); @@ -1025,6 +1035,14 @@ +
+ Images: {s.image_policy || 'remote'} + +
{/if}
{#each ART_FILTERS as [key, label] (key)} diff --git a/goodnews/api.py b/goodnews/api.py index 46b99fc..cbed5ec 100644 --- a/goodnews/api.py +++ b/goodnews/api.py @@ -351,7 +351,9 @@ class Article(BaseModel): title=row["title"], description=row.get("description"), url=row["canonical_url"], - image_url=row.get("image_url"), + # Resolve per the source's image policy: our cached copy, the publisher's URL + # (hotlink), or none — so we never re-host an image we haven't cleared. + image_url=newsimg.display_url(row["id"], row.get("image_policy"), row.get("image_url")), published_at=row.get("published_at"), source=row["source_name"], source_id=row.get("source_id"), @@ -591,6 +593,10 @@ class SourcePaywallBody(BaseModel): override: str | None = None # None = use domain rule · 'free' · 'paywalled' +class SourceImagePolicyBody(BaseModel): + policy: str = "remote" # 'cache' · 'remote' (default) · 'none' + + class CandidateSuggestBody(BaseModel): feed_url: str = "" name: str | None = None @@ -1111,7 +1117,7 @@ def create_app() -> FastAPI: with get_conn() as conn: row = conn.execute( "SELECT a.id, a.title, a.description, a.image_url, a.canonical_url, " - "a.duplicate_of, a.source_id, src.name AS source_name, s.reason_text, s.accepted, " + "a.duplicate_of, a.source_id, src.name AS source_name, src.image_policy, s.reason_text, s.accepted, " "(SELECT group_concat(t.tag) FROM article_tags t WHERE t.article_id = a.id) AS tags " "FROM articles a JOIN sources src ON src.id = a.source_id " "LEFT JOIN article_scores s ON s.article_id = a.id WHERE a.id = ?", @@ -1416,6 +1422,22 @@ def create_app() -> FastAPI: conn.commit() return {"ok": True, "override": ov} + @app.post("/api/admin/sources/{sid}/image-policy") + def admin_source_image_policy(sid: int, body: SourceImagePolicyBody, request: Request) -> dict: + # Image rights policy: 'cache' (re-host a downscaled copy — only for sources we've + # cleared: open license / permission / public-domain), 'remote' (hotlink the + # publisher's image), 'none' (no image). Default is the conservative 'remote'. + pol = body.policy + if pol not in ("cache", "remote", "none"): + raise HTTPException(status_code=422, detail="policy must be 'cache', 'remote', or 'none'") + with get_conn() as conn: + _require_admin(conn, request) + cur = conn.execute("UPDATE sources SET image_policy = ? WHERE id = ?", (pol, sid)) + if cur.rowcount == 0: + raise HTTPException(status_code=404, detail="source not found") + conn.commit() + return {"ok": True, "policy": pol} + # --- Source candidates (supervised add-a-source pipeline) ---------------- def _candidate_dict(row) -> dict: @@ -2346,7 +2368,8 @@ def create_app() -> FastAPI: with get_conn() as conn: row = conn.execute( "SELECT a.image_url FROM articles a JOIN article_scores s ON s.article_id = a.id " - "WHERE a.id = ? AND s.accepted = 1 AND a.duplicate_of IS NULL", + "JOIN sources src ON src.id = a.source_id " + "WHERE a.id = ? AND s.accepted = 1 AND a.duplicate_of IS NULL AND src.image_policy = 'cache'", (article_id,), ).fetchone() url = row["image_url"] if row else None diff --git a/goodnews/db.py b/goodnews/db.py index 58187d9..e5de816 100644 --- a/goodnews/db.py +++ b/goodnews/db.py @@ -632,6 +632,11 @@ def _migrate(conn: sqlite3.Connection) -> None: conn.execute("ALTER TABLE sources ADD COLUMN content_visible INTEGER NOT NULL DEFAULT 1") if "retry_after_at" not in source_cols: conn.execute("ALTER TABLE sources ADD COLUMN retry_after_at TEXT") + # Image rights policy per source: 'cache' (cleared to re-host a downscaled copy), + # 'remote' (hotlink the publisher's image — the conservative DEFAULT), 'none' (no + # image). Caching is opt-in; unknown/new sources are never re-hosted. + if "image_policy" not in source_cols: + conn.execute("ALTER TABLE sources ADD COLUMN image_policy TEXT NOT NULL DEFAULT 'remote'") # Daily Art columns added after the tables first shipped. pool_cols = {row["name"] for row in conn.execute("PRAGMA table_info(art_pool)")} diff --git a/goodnews/newsimg.py b/goodnews/newsimg.py index 9b8d1ce..281ce10 100644 --- a/goodnews/newsimg.py +++ b/goodnews/newsimg.py @@ -63,6 +63,23 @@ def _key(url: str) -> str: return hashlib.sha1(url.encode("utf-8")).hexdigest() +def display_url(article_id: int, image_policy: str | None, raw_url: str | None) -> str | None: + """The image URL the frontend should use, honoring the SOURCE's image policy: + 'cache' → our locally-cached copy (/api/img/) — only for sources we've cleared + to re-host (open license / explicit permission / public-domain). + 'remote' → the publisher's own URL (hotlinked + the frontend's graceful retry). The + conservative DEFAULT: we display but never re-host. + 'none' → no image (typographic cover). + Returns None when there's no image or the policy is 'none'.""" + if not raw_url: + return None + if image_policy == "cache": + return f"/api/img/{article_id}" + if image_policy == "none": + return None + return raw_url # 'remote' (default) — hotlink, never re-hosted + + class _FetchError(Exception): """permanent=True → negative-cache (won't retry soon); False → transient, retry.""" def __init__(self, msg: str, permanent: bool): @@ -83,16 +100,21 @@ def _safe_fetch(url: str, timeout: int = 12) -> tuple[bytes, str]: req = urllib.request.Request(current, headers=_UA) try: resp = opener.open(req, timeout=timeout) + except urllib.error.HTTPError as exc: + # _NoRedirect makes urllib RAISE on 3xx (rather than return a response), so + # redirects arrive here. Re-validate the destination on the next loop. 4xx + # (except 429) is a permanent miss → negative-cache; 429/5xx → transient. + if exc.code in (301, 302, 303, 307, 308): + loc = exc.headers.get("Location") + exc.close() + if not loc: + raise _FetchError("redirect without location", permanent=True) from exc + current = urljoin(current, loc) + continue + permanent = 400 <= exc.code < 500 and exc.code != 429 + raise _FetchError(f"http {exc.code}", permanent=permanent) from exc except (urllib.error.URLError, OSError, ValueError) as exc: raise _FetchError(f"fetch failed: {exc}", permanent=False) from exc - status = getattr(resp, "status", 200) or 200 - if status in (301, 302, 303, 307, 308): - loc = resp.headers.get("Location") - resp.close() - if not loc: - raise _FetchError("redirect without location", permanent=True) - current = urljoin(current, loc) - continue try: return resp.read(_MAX_FETCH_BYTES + 1), (resp.headers.get("Content-Type") or "") finally: @@ -106,11 +128,14 @@ def _encode(data: bytes) -> bytes | None: dimensions — the caller then REJECTS it (never stores arbitrary bytes).""" try: from PIL import Image - Image.MAX_IMAGE_PIXELS = _MAX_PIXELS # raise DecompressionBombError past this - im = Image.open(io.BytesIO(data)) - im.load() # forces decode → catches truncated/bomb here - if im.width > _MAX_DIM or im.height > _MAX_DIM or im.width < 1 or im.height < 1: + Image.MAX_IMAGE_PIXELS = _MAX_PIXELS # backstop; Pillow only WARNS at this, raises ~2x + im = Image.open(io.BytesIO(data)) # lazy: header (size) read without decoding pixels + # Enforce the pixel/dimension ceiling BEFORE load() so a decompression bomb is never + # actually decoded (Pillow's own MAX_IMAGE_PIXELS only warns at the threshold). + if (im.width * im.height > _MAX_PIXELS or im.width > _MAX_DIM or im.height > _MAX_DIM + or im.width < 1 or im.height < 1): return None + im.load() # decode now (also catches truncated data) if im.mode not in ("RGB", "RGBA"): im = im.convert("RGBA" if ("A" in im.mode or im.mode == "P") else "RGB") if im.width > DISPLAY_WIDTH: @@ -192,13 +217,16 @@ def fetch_and_cache(url: str | None) -> Path | None: def warm(conn, limit: int = 200) -> int: - """Pre-fetch display copies for the newest ACCEPTED, CANONICAL articles that have an - image, so the API only ever serves cache hits. Bounded; skips already-cached and - recently-failed URLs. Returns how many it newly cached.""" + """Pre-fetch display copies for the newest ACCEPTED, CANONICAL articles whose SOURCE + is cleared to cache (image_policy='cache'), so the API only ever serves cache hits. + Bounded; skips already-cached and recently-failed URLs. Returns how many it newly + cached. Sources default to 'remote' (hotlink, never re-hosted), so this caches + nothing until a source is explicitly set to 'cache'.""" rows = conn.execute( "SELECT DISTINCT a.image_url FROM article_scores s JOIN articles a ON a.id = s.article_id " - "WHERE s.accepted=1 AND a.duplicate_of IS NULL AND a.image_url IS NOT NULL " - "AND a.image_url != '' ORDER BY a.id DESC LIMIT ?", + "JOIN sources src ON src.id = a.source_id " + "WHERE s.accepted=1 AND a.duplicate_of IS NULL AND src.image_policy='cache' " + "AND a.image_url IS NOT NULL AND a.image_url != '' ORDER BY a.id DESC LIMIT ?", (limit,), ).fetchall() made = 0 diff --git a/goodnews/queries.py b/goodnews/queries.py index 43e514e..65e4e18 100644 --- a/goodnews/queries.py +++ b/goodnews/queries.py @@ -55,6 +55,7 @@ _ARTICLE_COLUMNS = f""" s.reason_text, s.model_name, src.paywall_override AS paywall_override, + src.image_policy AS image_policy, a.source_words, (SELECT group_concat(t.tag) FROM article_tags t WHERE t.article_id = a.id) AS tags, {RANK_SCORE_SQL} AS rank_score @@ -525,6 +526,7 @@ def source_health(conn: sqlite3.Connection) -> list[dict]: s.id, s.name, s.feed_url, s.homepage_url, s.default_category AS category, s.active, s.status, s.content_visible, s.retry_after_at, s.consecutive_failures AS failures, s.review_flag, s.review_reason, s.paywall_override, + s.image_policy, s.poll_interval_minutes AS interval_minutes, s.last_success_at, s.last_error_at, substr(s.last_error, 1, 160) AS last_error, (SELECT MAX(r.finished_at) FROM ingest_runs r diff --git a/goodnews/share.py b/goodnews/share.py index daa7390..8bad5eb 100644 --- a/goodnews/share.py +++ b/goodnews/share.py @@ -10,6 +10,8 @@ from __future__ import annotations from html import escape +from .newsimg import display_url + def _tag(name: str, content: str | None, attr: str = "property") -> str: if not content: @@ -149,11 +151,17 @@ def render_share_page(article: dict, base_url: str, summary: str | None = None, ) src_url = article.get("canonical_url") or base_url image = article.get("image_url") + policy = article.get("image_policy") + # What WE show, honoring the source's image policy (cache → our copy; remote → the + # publisher's URL; none → nothing). og/twitter reference the publisher's own image + # (a link, not re-hosting) whenever we'd show anything; 'none' omits it entirely. + display = display_url(aid, policy, image) + og_image = image if (image and policy != "none") else None page_url = f"{base_url}/a/{aid}" # With an image: a large-image card. Without: a clean text unfurl (title + # why + brand) — tidy, not a broken/muddy preview. (A branded fallback PNG # can replace this later.) - twitter_card = "summary_large_image" if image else "summary" + twitter_card = "summary_large_image" if display else "summary" meta = "\n".join(filter(None, [ _tag("og:site_name", "upbeatBytes"), @@ -161,22 +169,20 @@ def render_share_page(article: dict, base_url: str, summary: str | None = None, _tag("og:title", title), _tag("og:description", why), _tag("og:url", page_url), - _tag("og:image", image), + _tag("og:image", og_image), _tag("twitter:card", twitter_card, attr="name"), _tag("twitter:title", title, attr="name"), _tag("twitter:description", why, attr="name"), - _tag("twitter:image", image, attr="name"), + _tag("twitter:image", og_image, attr="name"), ])) - # The visible image is served from our cached/downscaled copy (not a hotlink), so a - # flaky source CDN can't blank it. og:image/twitter:image above stay the source URL - # so social crawlers fetch the canonical image directly. - # Served from our cache (/api/img/); if it isn't cached yet / fails, drop the - # element so the page degrades to the clean text unfurl rather than a broken icon. + # The visible image is whatever the policy resolved to (our cached copy for 'cache' + # sources, else the publisher's URL for 'remote'). If it isn't cached yet / fails to + # load, drop the element so the page degrades to the clean text unfurl, not a broken icon. media = ( - f'' - if image else "" + if display else "" ) raw_tags = (article.get("tags") or "") diff --git a/tests/test_admin.py b/tests/test_admin.py index 3406c28..4827e9e 100644 --- a/tests/test_admin.py +++ b/tests/test_admin.py @@ -585,3 +585,21 @@ def test_source_paywall_override(tmp_path, monkeypatch): # validation + 404 assert tc.post("/api/admin/sources/2/paywall", json={"override": "bogus"}).status_code == 422 assert tc.post("/api/admin/sources/999/paywall", json={"override": "free"}).status_code == 404 + + +def test_source_image_policy(tmp_path, monkeypatch): + import sqlite3, os + app, api = _make(tmp_path, monkeypatch, admin_email="boss@x.com") + c = sqlite3.connect(os.environ["GOODNEWS_DB"]) + c.execute("INSERT INTO sources (id,name,feed_url) VALUES (2,'Gov','http://g/f')") + c.commit(); c.close() + anon = TestClient(app) + assert anon.post("/api/admin/sources/2/image-policy", json={"policy": "cache"}).status_code == 401 # gated + tc = _signin(app, api, "boss@x.com") + assert tc.post("/api/admin/sources/2/image-policy", json={"policy": "cache"}).json()["policy"] == "cache" + c = sqlite3.connect(os.environ["GOODNEWS_DB"]) + assert c.execute("SELECT image_policy FROM sources WHERE id=2").fetchone()[0] == "cache" + c.close() + assert tc.post("/api/admin/sources/2/image-policy", json={"policy": "remote"}).json()["policy"] == "remote" + assert tc.post("/api/admin/sources/2/image-policy", json={"policy": "bogus"}).status_code == 422 + assert tc.post("/api/admin/sources/999/image-policy", json={"policy": "cache"}).status_code == 404 diff --git a/tests/test_newsimg.py b/tests/test_newsimg.py index 7d51153..3976847 100644 --- a/tests/test_newsimg.py +++ b/tests/test_newsimg.py @@ -16,6 +16,30 @@ from goodnews.db import connect, init_db _REAL_SAFE_FETCH = newsimg._safe_fetch +class _Resp: + """Minimal non-redirect (2xx) response for a fake opener.""" + def __init__(self, headers, body=b""): + self.status, self.headers, self._b = 200, headers, body + def read(self, n=-1): + return self._b + def close(self): + pass + + +def _http_error(url, code, location=None): + """A real urllib HTTPError, as _NoRedirect raises for 3xx (and urllib for 4xx/5xx).""" + import urllib.error + hdrs = {"Location": location} if location else {} + return urllib.error.HTTPError(url, code, "x", hdrs, io.BytesIO(b"")) + + +def _fake_opener(handler): + class _Op: + def open(self, req, timeout=None): + return handler(req.full_url) + return lambda *a, **k: _Op() + + def _png(w=1600, h=1000, color=(40, 130, 173)): out = io.BytesIO() Image.new("RGB", (w, h), color).save(out, format="PNG") @@ -64,30 +88,64 @@ def test_private_host_refused_and_negative_cached(cache, monkeypatch): def test_redirect_to_private_is_refused(cache, monkeypatch): - monkeypatch.setattr(newsimg, "_safe_fetch", _REAL_SAFE_FETCH) # exercise real redirect re-validation + monkeypatch.setattr(newsimg, "_safe_fetch", _REAL_SAFE_FETCH) hosts = {"good.example": True, "evil.internal": False} monkeypatch.setattr(newsimg, "_host_is_public", lambda h: hosts.get(h, False)) - class _Resp: - def __init__(self, status, headers, body=b""): - self.status, self.headers, self._b = status, headers, body - def read(self, n=-1): - return self._b - def close(self): - pass - - class _Opener: - def open(self, req, timeout=None): - if "good.example" in req.full_url: # public → 302 to private - return _Resp(302, {"Location": "http://evil.internal/x.png"}) - return _Resp(200, {"Content-Type": "image/png"}, _png()) # should never be reached - monkeypatch.setattr(newsimg.urllib.request, "build_opener", lambda *a: _Opener()) + def handle(url): # real 302 (HTTPError) → private dest + if "good.example" in url: + raise _http_error(url, 302, "http://evil.internal/x.png") + return _Resp({"Content-Type": "image/png"}, _png()) # must never be reached + monkeypatch.setattr(newsimg.urllib.request, "build_opener", _fake_opener(handle)) url = "http://good.example/p.png" assert newsimg.fetch_and_cache(url) is None # redirect hop re-validated → refused assert newsimg._failed_recently(url) +def test_real_redirect_followed_to_public(cache, monkeypatch): + """Regression for the _NoRedirect bug: 3xx arrive as HTTPError, must be followed.""" + monkeypatch.setattr(newsimg, "_safe_fetch", _REAL_SAFE_FETCH) + monkeypatch.setattr(newsimg, "_host_is_public", lambda h: True) + seen = [] + + def handle(url): + seen.append(url) + if url == "http://a.example/p.png": + raise _http_error(url, 302, "http://b.example/final.png") + return _Resp({"Content-Type": "image/png"}, _png()) + monkeypatch.setattr(newsimg.urllib.request, "build_opener", _fake_opener(handle)) + + p = newsimg.fetch_and_cache("http://a.example/p.png") + assert p and p.suffix == ".webp" # followed the redirect, cached the dest + assert seen == ["http://a.example/p.png", "http://b.example/final.png"] + + +def test_404_is_permanent_but_5xx_is_transient(cache, monkeypatch): + monkeypatch.setattr(newsimg, "_safe_fetch", _REAL_SAFE_FETCH) + monkeypatch.setattr(newsimg, "_host_is_public", lambda h: True) + monkeypatch.setattr(newsimg.urllib.request, "build_opener", + _fake_opener(lambda url: (_ for _ in ()).throw(_http_error(url, 404)))) + gone = "http://x.example/missing.png" + assert newsimg.fetch_and_cache(gone) is None + assert newsimg._failed_recently(gone) # 404 → permanent, negative-cached + + monkeypatch.setattr(newsimg.urllib.request, "build_opener", + _fake_opener(lambda url: (_ for _ in ()).throw(_http_error(url, 503)))) + down = "http://x.example/down.png" + assert newsimg.fetch_and_cache(down) is None + assert not newsimg._failed_recently(down) # 5xx → transient, retried next cycle + + +def test_megapixel_ceiling_enforced_before_decode(cache, monkeypatch): + monkeypatch.setattr(newsimg, "_MAX_PIXELS", 1000) # a 1600x1000 image (1.6M px) exceeds this + monkeypatch.setattr(newsimg, "_safe_fetch", lambda url, timeout=12: (_png(1600, 1000), "image/png")) + url = "https://x.example/huge.png" + assert newsimg.fetch_and_cache(url) is None # rejected by the explicit w*h check + assert not list(cache.glob("*.webp")) + assert newsimg._failed_recently(url) + + def test_transient_failure_is_not_negative_cached(cache, monkeypatch): def boom(url, timeout=12): raise newsimg._FetchError("network down", permanent=False) @@ -109,16 +167,29 @@ def test_prune_evicts_least_recently_used_over_cap(cache): assert paths[2].exists() and paths[4].exists() -def test_warm_skips_cached_and_failed(cache, monkeypatch): +def test_display_url_resolves_per_policy(): + assert newsimg.display_url(7, "cache", "https://x/p.jpg") == "/api/img/7" # our copy + assert newsimg.display_url(7, "remote", "https://x/p.jpg") == "https://x/p.jpg" # hotlink + assert newsimg.display_url(7, "none", "https://x/p.jpg") is None # typographic cover + assert newsimg.display_url(7, None, "https://x/p.jpg") == "https://x/p.jpg" # default = remote + assert newsimg.display_url(7, "cache", None) is None # no image + + +def test_warm_only_caches_cache_policy_sources(cache, monkeypatch): conn = connect(":memory:"); init_db(conn) - conn.execute("INSERT INTO sources (id,name,feed_url) VALUES (1,'S','http://s/f')") - for aid, img, acc, dup in ((1, "https://x/1.jpg", 1, None), (2, "https://x/2.jpg", 1, None), - (3, "https://x/3.jpg", 0, None), (4, "https://x/4.jpg", 1, 1)): + conn.execute("INSERT INTO sources (id,name,feed_url,image_policy) VALUES (1,'C','http://c/f','cache')") + conn.execute("INSERT INTO sources (id,name,feed_url,image_policy) VALUES (2,'R','http://r/f','remote')") + # source 1 (cache): two accepted+canonical+image; plus not-accepted + duplicate (skipped) + # source 2 (remote): accepted+image but must NOT be cached (re-hosting not cleared) + rows = ((1, 1, "https://x/1.jpg", 1, None), (2, 1, "https://x/2.jpg", 1, None), + (3, 1, "https://x/3.jpg", 0, None), (4, 1, "https://x/4.jpg", 1, 1), + (5, 2, "https://x/5.jpg", 1, None)) + for aid, sid, img, acc, dup in rows: conn.execute("INSERT INTO articles (id,source_id,canonical_url,title,url_hash,image_url,duplicate_of) " - "VALUES (?,1,?,?,?,?,?)", (aid, f"http://s/{aid}", f"t{aid}", f"h{aid}", img, dup)) + "VALUES (?,?,?,?,?,?,?)", (aid, sid, f"http://s/{aid}", f"t{aid}", f"h{aid}", img, dup)) conn.execute("INSERT INTO article_scores (article_id, accepted) VALUES (?,?)", (aid, acc)) conn.commit() - assert newsimg.warm(conn) == 2 # only the 2 accepted, canonical, with image + assert newsimg.warm(conn) == 2 # only source 1's accepted+canonical+image assert newsimg.warm(conn) == 0 # idempotent — already cached @@ -129,13 +200,16 @@ def client(tmp_path, monkeypatch): monkeypatch.setattr(newsimg, "_host_is_public", lambda h: True) monkeypatch.setattr(newsimg, "_safe_fetch", lambda url, timeout=12: (_png(), "image/png")) conn = connect(tmp_path / "t.sqlite3"); init_db(conn) - conn.execute("INSERT INTO sources (id,name,feed_url) VALUES (1,'S','http://s/f')") - # 1 = accepted+canonical+image, 2 = no image, 3 = NOT accepted, 4 = duplicate - rows = ((1, "https://x/pic.jpg", 1, None), (2, "", 1, None), - (3, "https://x/p3.jpg", 0, None), (4, "https://x/p4.jpg", 1, 1)) - for aid, img, acc, dup in rows: + conn.execute("INSERT INTO sources (id,name,feed_url,image_policy) VALUES (1,'C','http://c/f','cache')") + conn.execute("INSERT INTO sources (id,name,feed_url,image_policy) VALUES (2,'R','http://r/f','remote')") + # src1 (cache): 1=accepted+image, 2=no image, 3=NOT accepted, 4=duplicate. + # src2 (remote): 5=accepted+image but NOT cleared to cache → endpoint must 404. + rows = ((1, 1, "https://x/pic.jpg", 1, None), (2, 1, "", 1, None), + (3, 1, "https://x/p3.jpg", 0, None), (4, 1, "https://x/p4.jpg", 1, 1), + (5, 2, "https://x/p5.jpg", 1, None)) + for aid, sid, img, acc, dup in rows: conn.execute("INSERT INTO articles (id,source_id,canonical_url,title,url_hash,image_url,duplicate_of) " - "VALUES (?,1,?,?,?,?,?)", (aid, f"http://s/{aid}", f"t{aid}", f"h{aid}", img, dup)) + "VALUES (?,?,?,?,?,?,?)", (aid, sid, f"http://s/{aid}", f"t{aid}", f"h{aid}", img, dup)) conn.execute("INSERT INTO article_scores (article_id, accepted) VALUES (?,?)", (aid, acc)) conn.commit() # the cycle owns fetching — pre-warm so the endpoint has a hit to serve @@ -152,6 +226,7 @@ def test_img_endpoint_serves_cached_and_restricts(client): assert client.get("/api/img/2").status_code == 404 # no image assert client.get("/api/img/3").status_code == 404 # not accepted assert client.get("/api/img/4").status_code == 404 # duplicate (non-canonical) + assert client.get("/api/img/5").status_code == 404 # remote-policy source (not cleared to cache) assert client.get("/api/img/999").status_code == 404 # unknown id