From 4da40c471d07bcb4d2e44a30144f3ee5126c32a6 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Tue, 20 Oct 2026 15:24:30 +0200 Subject: [PATCH] fix: isolate an unreadable original per platform A cached bios_db.json.zip that was not a zip raised BadZipFile out of run(): --all stopped and the platforms after it were never exported. fetch() also cached any 200 reply, so an error page replaced a good archive and was served on every later run, and an IncompleteRead or an oversized reply escaped the same way. An archive is now read before it is cached, and an unreadable file fails its own platform with the --refresh-cache remedy. --- scripts/export_native.py | 67 ++++++++++++++++++++++++++++------------ tests/test_exporters.py | 59 +++++++++++++++++++++++++++++++++++ 2 files changed, 106 insertions(+), 20 deletions(-) diff --git a/scripts/export_native.py b/scripts/export_native.py index 526e7478..b50c7ef5 100644 --- a/scripts/export_native.py +++ b/scripts/export_native.py @@ -18,11 +18,15 @@ from __future__ import annotations import argparse import contextlib import functools +import http.client import re import json import sys -import urllib.error +import tarfile import urllib.request +import zipfile +import zlib +from collections.abc import Callable from dataclasses import dataclass, field from pathlib import Path @@ -82,8 +86,26 @@ def load_sources(upstream_dir: Path) -> dict[str, str]: return _load_sources(upstream_dir / SOURCES_INDEX) +class OriginalUnreadable(ValueError): + """A platform's file that is not what its format says it is.""" + + +def _read_archive(unpack: Callable[[bytes], dict[str, str]], payload: bytes) -> dict[str, str]: + """Unpack an original, naming the remedy when its bytes are not one.""" + try: + return unpack(payload) + except (zipfile.BadZipFile, tarfile.TarError, EOFError, zlib.error, KeyError) as exc: + raise OriginalUnreadable( + f"archive unreadable ({exc}): --refresh-cache downloads it again" + ) from exc + + def fetch( - url: str, destination: Path, index: Path | None = None, refresh: bool = False + url: str, + destination: Path, + index: Path | None = None, + refresh: bool = False, + check: Callable[[bytes], object] | None = None, ) -> bytes: """Download an original once, then read it from the cache. @@ -122,7 +144,11 @@ def fetch( with urllib.request.urlopen(request, timeout=60) as response: payload = response.read(_MAX_BYTES + 1) if len(payload) > _MAX_BYTES: - raise ValueError(f"{url}: response larger than {_MAX_BYTES} bytes") + raise OriginalUnreadable(f"{url}: response larger than {_MAX_BYTES} bytes") + # An error page served with 200 replaced a good archive and was then + # served on every run: what does not read is never cached. + if check is not None: + check(payload) destination.parent.mkdir(parents=True, exist_ok=True) with locked(): write_bytes_atomic(str(destination), payload) @@ -261,26 +287,27 @@ def collect_originals( index = upstream_dir / SOURCES_INDEX originals: dict[str, str] = {} missing: list[str] = [] + unpack = getattr(exporter, "unpack", None) for relative, url in wanted.items(): path = root / relative - payload: bytes | None = None - if allow_fetch: - try: - payload = fetch(url, path, index, refresh) - except (urllib.error.URLError, urllib.error.HTTPError, OSError) as exc: - missing.append(f"{relative}: {exc}") + archive = callable(unpack) and relative.endswith((".zip", ".tar.gz")) + check = (lambda raw: _read_archive(unpack, raw)) if archive else None + try: + if allow_fetch: + payload = fetch(url, path, index, refresh, check) + elif path.exists(): + payload = path.read_bytes() + else: + missing.append(f"{relative}: absent and fetching is off") continue - elif path.exists(): - payload = path.read_bytes() - else: - missing.append(f"{relative}: absent and fetching is off") - continue - - unpack = getattr(exporter, "unpack", None) - if callable(unpack) and relative.endswith((".zip", ".tar.gz")): - originals.update(unpack(payload)) - else: - originals[relative] = payload.decode("utf-8", errors="replace") + if archive: + originals.update(_read_archive(unpack, payload)) + else: + originals[relative] = payload.decode("utf-8", errors="replace") + # One platform's unreadable file fails that platform: raised, it + # stopped --all before the platforms after it. + except (OSError, http.client.HTTPException, OriginalUnreadable) as exc: + missing.append(f"{relative}: {exc}") return originals, missing diff --git a/tests/test_exporters.py b/tests/test_exporters.py index 9d4d229b..b3e4dc8d 100644 --- a/tests/test_exporters.py +++ b/tests/test_exporters.py @@ -15,6 +15,7 @@ and skip when it has not been populated (python scripts/export_native.py from __future__ import annotations import difflib +import io import json import os import re @@ -23,6 +24,7 @@ import sys import tempfile import unittest import xml.etree.ElementTree as ET +import zipfile from collections import OrderedDict from pathlib import Path @@ -709,6 +711,18 @@ class _OneFileExporter: return {"list.txt": self._url} +class _ArchiveExporter(_OneFileExporter): + """One whose original is an archive it unpacks, as MiSTer's is.""" + + def native_sources(self) -> dict[str, str]: + return {"db.zip": self._url} + + @staticmethod + def unpack(raw: bytes) -> dict[str, str]: + with zipfile.ZipFile(io.BytesIO(raw)) as archive: + return {"db.json": archive.read("db.json").decode("utf-8")} + + class CachedOriginal(unittest.TestCase): """The cached original is the file the platform data was read from.""" @@ -792,6 +806,51 @@ class CachedOriginal(unittest.TestCase): fetch(url, target, self.index) self.assertEqual(held, [True]) + def test_an_unreadable_cached_archive_fails_its_platform_only(self): + """A cached bios_db.json.zip that was not a zip raised BadZipFile + out of run(): --all stopped and the next platforms were never + exported.""" + target = self.cache / "plat" / "db.zip" + target.parent.mkdir(parents=True) + target.write_bytes(b"not a zip") + originals, missing = collect_originals( + _ArchiveExporter("file:///nowhere/db.zip"), {}, self.cache, False + ) + self.assertEqual(originals, {}) + self.assertEqual(len(missing), 1) + self.assertIn("--refresh-cache", missing[0]) + + def test_a_download_that_is_not_the_archive_keeps_the_cache(self): + """fetch() cached any 200 reply, so an error page replaced a good + archive and was served on every later run.""" + good = self.root / "db.zip" + with zipfile.ZipFile(good, "w") as archive: + archive.writestr("db.json", "{}") + url = good.as_uri() + exporter = _ArchiveExporter(url) + collect_originals(exporter, {}, self.cache, True) + cached = (self.cache / "plat" / "db.zip").read_bytes() + good.write_bytes(b"error") + originals, missing = collect_originals(exporter, {}, self.cache, True, refresh=True) + self.assertEqual(originals, {}) + self.assertIn("--refresh-cache", missing[0]) + self.assertEqual((self.cache / "plat" / "db.zip").read_bytes(), cached) + + def test_an_incomplete_read_is_reported(self): + import http.client # noqa: PLC0415 + from unittest import mock # noqa: PLC0415 + + import export_native # noqa: PLC0415 + + def cut(*_args, **_kwargs): + raise http.client.IncompleteRead(b"par") + + with mock.patch.object(export_native, "fetch", cut): + originals, missing = collect_originals( + _OneFileExporter("https://example.invalid/list.txt"), {}, self.cache, True + ) + self.assertEqual((originals, len(missing)), ({}, 1)) + def test_the_index_names_a_file_the_same_from_any_directory(self): target = self.cache / "plat" / "list.txt" self.assertEqual(source_key(target, self.index), "plat/list.txt")