Validate images actually load (fix overcounted coverage)
A stored og:image isn't proof it renders: signed/hotlink-protected URLs (e.g. the Guardian's i.guim.co.uk) 401 on a direct browser load, so they counted toward coverage yet always fell back. Now fetch_og_image confirms the image truly returns 200 + image/* (requested no-referrer, same SSRF-safe redirect handling) before storing it. Add prune_broken_images() to clear already-stored URLs that no longer load, so coverage is honest and those cards show the placeholder cleanly. The browser onerror→placeholder remains the final safety net. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
+61
-1
@@ -163,10 +163,70 @@ def fetch_og_image(url: str | None) -> str | None:
|
|||||||
body = response.read(MAX_BYTES)
|
body = response.read(MAX_BYTES)
|
||||||
finally:
|
finally:
|
||||||
response.close()
|
response.close()
|
||||||
return og_image_from_html(body)
|
image = og_image_from_html(body)
|
||||||
|
# A stored URL is not proof it renders — confirm it actually loads.
|
||||||
|
return image if (image and _image_loads(image)) else None
|
||||||
return None # too many redirects
|
return None # too many redirects
|
||||||
|
|
||||||
|
|
||||||
|
def _image_loads(url: str) -> bool:
|
||||||
|
"""Confirm an image URL truly returns an image (HTTP 200 + image/* type).
|
||||||
|
|
||||||
|
Many publishers serve a signed or hotlink-protected og:image that 401/403s
|
||||||
|
on a direct request (e.g. the Guardian's i.guim.co.uk), so storing the URL
|
||||||
|
would overstate coverage and the card would never render it. We request as
|
||||||
|
the browser does — no referrer — with the same per-hop host safety as the
|
||||||
|
page fetch. Returns False on any error.
|
||||||
|
"""
|
||||||
|
opener = urllib.request.build_opener(_NoRedirect)
|
||||||
|
for _ in range(MAX_REDIRECTS + 1):
|
||||||
|
if not url:
|
||||||
|
return False
|
||||||
|
parts = urlsplit(url)
|
||||||
|
if parts.scheme not in ("http", "https") or not _host_is_public(parts.hostname):
|
||||||
|
return False
|
||||||
|
request = urllib.request.Request(url, headers={"User-Agent": USER_AGENT, "Accept": "image/*,*/*"})
|
||||||
|
try:
|
||||||
|
response = opener.open(request, timeout=TIMEOUT)
|
||||||
|
except (urllib.error.URLError, OSError, ValueError):
|
||||||
|
return False
|
||||||
|
try:
|
||||||
|
status = getattr(response, "status", 200) or 200
|
||||||
|
if status in (301, 302, 303, 307, 308):
|
||||||
|
location = response.headers.get("Location")
|
||||||
|
if not location:
|
||||||
|
return False
|
||||||
|
url = urljoin(url, location)
|
||||||
|
continue
|
||||||
|
ctype = (response.headers.get("Content-Type") or "").lower()
|
||||||
|
return status == 200 and ctype.startswith("image/")
|
||||||
|
finally:
|
||||||
|
response.close()
|
||||||
|
return False
|
||||||
|
|
||||||
|
|
||||||
|
def prune_broken_images(conn: sqlite3.Connection, check=_image_loads, limit: int = 3000) -> int:
|
||||||
|
"""Clear stored image URLs that no longer load (signed/expired/hotlink-
|
||||||
|
protected), so coverage is honest and those cards fall back to the calm
|
||||||
|
placeholder cleanly instead of attempting a doomed fetch. Returns count cleared.
|
||||||
|
"""
|
||||||
|
rows = conn.execute(
|
||||||
|
"SELECT id, image_url FROM articles WHERE image_url IS NOT NULL AND image_url != '' "
|
||||||
|
"ORDER BY id DESC LIMIT ?",
|
||||||
|
(limit,),
|
||||||
|
).fetchall()
|
||||||
|
cleared = 0
|
||||||
|
for row in rows:
|
||||||
|
if not check(row["image_url"]):
|
||||||
|
conn.execute(
|
||||||
|
"UPDATE articles SET image_url = NULL, image_checked_at = CURRENT_TIMESTAMP WHERE id = ?",
|
||||||
|
(row["id"],),
|
||||||
|
)
|
||||||
|
cleared += 1
|
||||||
|
conn.commit()
|
||||||
|
return cleared
|
||||||
|
|
||||||
|
|
||||||
def enrich_brief_images(
|
def enrich_brief_images(
|
||||||
conn: sqlite3.Connection, brief_date: str, fetch=fetch_og_image, limit: int = 7, retry_days: int = 2
|
conn: sqlite3.Connection, brief_date: str, fetch=fetch_og_image, limit: int = 7, retry_days: int = 2
|
||||||
) -> int:
|
) -> int:
|
||||||
|
|||||||
@@ -47,6 +47,18 @@ def test_enrich_records_checked_even_when_none_found(tmp_path):
|
|||||||
assert again is False # still within retry window
|
assert again is False # still within retry window
|
||||||
|
|
||||||
|
|
||||||
|
def test_prune_clears_only_broken_images(tmp_path):
|
||||||
|
c = _setup(tmp_path)
|
||||||
|
_add(c, 1, image="http://good/a.jpg")
|
||||||
|
_add(c, 2, image="http://broken/b.jpg")
|
||||||
|
_add(c, 3, image=None) # nothing to check
|
||||||
|
# Pretend only the 'good' host loads.
|
||||||
|
cleared = enrich.prune_broken_images(c, check=lambda u: "good" in u)
|
||||||
|
assert cleared == 1
|
||||||
|
assert c.execute("SELECT image_url FROM articles WHERE id=1").fetchone()[0] == "http://good/a.jpg"
|
||||||
|
assert c.execute("SELECT image_url FROM articles WHERE id=2").fetchone()[0] is None
|
||||||
|
|
||||||
|
|
||||||
def test_backfill_only_targets_summarized_accepted_imageless(tmp_path):
|
def test_backfill_only_targets_summarized_accepted_imageless(tmp_path):
|
||||||
c = _setup(tmp_path)
|
c = _setup(tmp_path)
|
||||||
_add(c, 1, image=None, summarized=True, accepted=1) # eligible
|
_add(c, 1, image=None, summarized=True, accepted=1) # eligible
|
||||||
|
|||||||
Reference in New Issue
Block a user