diff --git a/web/app.py b/web/app.py index d85cce0..ed8524d 100644 --- a/web/app.py +++ b/web/app.py @@ -2789,14 +2789,21 @@ async def serve_blob( from pathlib import Path as _Path backend = storage.get_storage() if backend.name != "filesystem": + # Presign ONLY when the object is actually in S3. Under ``dual`` the + # generic ``exists`` is disk-OR-S3, so a disk-only file (failed mirror / + # outside the migration set) would otherwise be redirected to a presigned + # URL that 404s on MinIO. Probe the S3 sub-backend specifically + # (DualBackend.s3 — or the S3 backend itself under ``s3``) so disk-only + # files fall back to a real disk read. + s3 = getattr(backend, "s3", backend) try: key = storage.normalize_key(path) - if await backend.exists(key, bucket=bucket): - url = await backend.presign_get(key, bucket=bucket, download_name=filename) + if await s3.exists(key, bucket=bucket): + url = await s3.presign_get(key, bucket=bucket, download_name=filename) return RedirectResponse(url, status_code=302) except Exception: # noqa: BLE001 — never 500 on a key/presign hiccup; fall back to disk logger.exception("serve_blob: presign failed for %s; falling back to disk", path) - # not in object storage (dual fallback) — serve the disk copy if present + # not in object storage (dual disk-fallback) — serve the disk copy if present if not _Path(path).exists(): raise HTTPException(404, "קובץ לא נמצא") return FileResponse(path, media_type=media_type, filename=filename) diff --git a/web/tests/test_serve_blob.py b/web/tests/test_serve_blob.py index 6ec8373..cf0c198 100644 --- a/web/tests/test_serve_blob.py +++ b/web/tests/test_serve_blob.py @@ -78,3 +78,34 @@ def test_s3_missing_no_disk_404(monkeypatch): with pytest.raises(HTTPException) as ei: _serve(monkeypatch, "s3", False, DATA_DIR / "audit" / "_nope.txt") assert ei.value.status_code == 404 + + +class _FakeDual: + """DualBackend stand-in: generic exists() is disk-OR-S3 (always True here), + but the .s3 sub-backend is S3-only. The fix must probe .s3, not exists().""" + name = "dual" + + def __init__(self, s3_has: bool) -> None: + self.s3 = _FakeBackend("s3", s3_has) + + async def exists(self, key, *, bucket) -> bool: # noqa: ANN001 — disk-or-S3 → True + return True + + async def presign_get(self, key, *, bucket, download_name=None) -> str: # noqa: ANN001 + return f"https://s3.example/WRONG/{key}" # must NOT be used (proves .s3 probed) + + +def test_dual_probes_s3_subbackend_not_generic_exists(blob, monkeypatch): + """Regression (DualBackend.exists is disk-OR-S3): a file on disk but NOT in + S3 must fall back to a disk FileResponse, never a presigned URL that 404s.""" + monkeypatch.setattr(storage, "get_storage", lambda: _FakeDual(s3_has=False)) + r = asyncio.new_event_loop().run_until_complete( + app.serve_blob(str(blob), media_type="text/plain", filename="x.txt")) + assert isinstance(r, FileResponse) # disk fallback, NOT a (broken) redirect + + +def test_dual_in_s3_uses_s3_subbackend(blob, monkeypatch): + monkeypatch.setattr(storage, "get_storage", lambda: _FakeDual(s3_has=True)) + r = asyncio.new_event_loop().run_until_complete( + app.serve_blob(str(blob), media_type="text/plain", filename="x.txt")) + assert isinstance(r, RedirectResponse)