diff --git a/cli_tools/mcdi/README.md b/cli_tools/mcdi/README.md index ecdc1bf..dda7dd8 100644 --- a/cli_tools/mcdi/README.md +++ b/cli_tools/mcdi/README.md @@ -116,7 +116,42 @@ The data commons is auto-detected from the manifest's header row; pass | `--source {gdc,pdc}` | Skip auto-detection | | `--workers N` | Concurrent downloads (default: 4). PDC always runs at 1 to respect its rate limit, regardless of this flag. | | `--verify-checksum` | Verify each file's md5 against the manifest after download | +| `-x`, `--extract` | Extract recognized archives (`.tar.gz`, `.tgz`, `.tar.bz2`, `.tar.xz`, `.tar`, `.zip`, `.gz`, `.bz2`, `.xz`) in place after download, keeping only the extracted contents at the manifest's output path. Off by default. | | `--token-file PATH` | File containing a GDC auth token, for controlled-access files | +| `--retries N` | Extra attempts for files that fail transiently within this run (default: 2) | +| `--retry-backoff SECONDS` | Wait before each retry pass, multiplied by the attempt number (default: 5.0) | + +Some commons files are themselves archives (e.g. a `.tar.gz` bundle of slides). +`--extract` unpacks any recognized archive into the same directory it was +downloaded into, right after downloading it. On success, the archive itself +then moves to a sibling `.mcdi-archives/` directory (mirroring +`--output-dir`'s layout) — it isn't deleted, just relocated out of the way, +so `--output-dir` ends up holding only the extracted contents, not a +redundant copy of the packed archive next to them. A tool that recursively +collects everything under `--output-dir` (e.g. Galaxy's `discover_datasets`) +then only ever sees the actual extracted files. If extraction fails, the +archive is left where it was downloaded instead, so there's still something +to show for it. The archive's presence in `.mcdi-archives/` also doubles as +the idempotency marker, so reruns skip both re-downloading and +re-extracting. + +**Pre-flight access check.** Before downloading anything, every file in the +manifest is probed with a cheap ranged request (skipping ones already +correctly present locally, and — for GDC — ones a single bulk lookup already +confirms are open-access). If even one file turns out to be inaccessible +(e.g. a controlled-access file without a valid token), the whole run aborts +before downloading *any* file, naming exactly which one(s) failed and why — +rather than downloading most of a large manifest only to fail on the last +file. This always runs; there's no flag to skip it. + +**Retries.** Beyond the transport-level retries already built into every +request, a failed file (connection errors, `429`/`5xx`, or a checksum +mismatch — not permanent-looking failures like `401`/`403`/`404`) gets +`--retries` more whole-batch attempts, waiting `--retry-backoff × attempt` +seconds between passes. This matters most where nothing will manually rerun +the command for you on a failure — e.g. a Galaxy job, where a retried job +gets a fresh working directory, not the partial output of the failed attempt, +so anything not resolved within the one invocation is lost. Controlled-access GDC files need an auth token, obtained by logging into the GDC portal and downloading your token. Provide it either via the `GDC_TOKEN` @@ -136,7 +171,8 @@ Output layout: Re-running against the same manifest and output directory skips files already downloaded (verifying checksums too, if `--verify-checksum` is set), so -interrupted runs can simply be re-run. +interrupted runs can simply be re-run — the pre-flight check skips them too, +so a rerun over mostly-complete output is cheap, not another full pass. ## Runtime environment diff --git a/cli_tools/mcdi/mcdi/__init__.py b/cli_tools/mcdi/mcdi/__init__.py index 1428e26..e0192e9 100644 --- a/cli_tools/mcdi/mcdi/__init__.py +++ b/cli_tools/mcdi/mcdi/__init__.py @@ -15,7 +15,7 @@ import os -__version__ = "0.3.0" +__version__ = "0.4.0" # Build identifier baked into the container image at build time (e.g. the git # commit SHA). Lets you confirm the exact code a run used, even when the version diff --git a/cli_tools/mcdi/mcdi/download/archive.py b/cli_tools/mcdi/mcdi/download/archive.py new file mode 100644 index 0000000..ce93079 --- /dev/null +++ b/cli_tools/mcdi/mcdi/download/archive.py @@ -0,0 +1,85 @@ +"""Extraction of archives (tar/zip/gz/bz2/xz) downloaded from commons manifests.""" + +from __future__ import annotations + +import bz2 +import gzip +import lzma +import shutil +import tarfile +import zipfile +from pathlib import Path + +_TAR_SUFFIXES = { + ".tar.gz": "r:gz", + ".tgz": "r:gz", + ".tar.bz2": "r:bz2", + ".tbz2": "r:bz2", + ".tar.xz": "r:xz", + ".txz": "r:xz", + ".tar": "r:", +} +_SINGLE_FILE_OPENERS = {".gz": gzip.open, ".bz2": bz2.open, ".xz": lzma.open} + + +class ArchiveError(Exception): + """An archive could not be safely extracted.""" + + +def _tar_mode(name: str) -> str | None: + for suffix, mode in _TAR_SUFFIXES.items(): + if name.endswith(suffix): + return mode + return None + + +def is_archive(path: Path) -> bool: + name = path.name.lower() + return _tar_mode(name) is not None or name.endswith(".zip") or name.endswith(tuple(_SINGLE_FILE_OPENERS)) + + +def _check_member_path(member_name: str, dest_dir: Path) -> None: + """Reject an archive member whose path would land outside ``dest_dir`` (zip-slip).""" + target = (dest_dir / member_name).resolve() + if target != dest_dir and dest_dir not in target.parents: + raise ArchiveError(f"archive member escapes destination: {member_name!r}") + + +def extract(path: Path) -> Path: + """Extract ``path`` in place, into its own parent directory. + + Returns the directory extracted into (tar/zip), or the decompressed file's + path (bare .gz/.bz2/.xz). + """ + dest_dir = path.parent.resolve() + name = path.name.lower() + + tar_mode = _tar_mode(name) + if tar_mode is not None: + with tarfile.open(path, tar_mode) as tf: + members = tf.getmembers() + for member in members: + _check_member_path(member.name, dest_dir) + if member.issym() or member.islnk(): + raise ArchiveError(f"refusing to extract link member: {member.name!r}") + if hasattr(tarfile, "data_filter"): + tf.extractall(dest_dir, members=members, filter="data") + else: + tf.extractall(dest_dir, members=members) + return dest_dir + + if name.endswith(".zip"): + with zipfile.ZipFile(path) as zf: + for member_name in zf.namelist(): + _check_member_path(member_name, dest_dir) + zf.extractall(dest_dir) + return dest_dir + + for suffix, opener in _SINGLE_FILE_OPENERS.items(): + if name.endswith(suffix): + target = path.with_name(path.name[: -len(suffix)]) + with opener(path, "rb") as src, open(target, "wb") as dst: + shutil.copyfileobj(src, dst) + return target + + raise ArchiveError(f"unsupported archive type: {path.name!r}") diff --git a/cli_tools/mcdi/mcdi/download/cli.py b/cli_tools/mcdi/mcdi/download/cli.py index f600002..795567b 100644 --- a/cli_tools/mcdi/mcdi/download/cli.py +++ b/cli_tools/mcdi/mcdi/download/cli.py @@ -32,10 +32,30 @@ def add_arguments(subparsers: argparse._SubParsersAction) -> argparse.ArgumentPa action="store_true", help="Verify each downloaded file's md5 against the manifest", ) + parser.add_argument( + "-x", + "--extract", + action="store_true", + help="Extract recognized archives (.tar.gz, .tgz, .tar.bz2, .tar.xz, .tar, .zip, .gz, .bz2, .xz) " + "in place after download, keeping only the extracted contents at the manifest's output " + "path. On success the archive itself moves to a sibling '.mcdi-archives' " + "directory (not deleted); on failure it's left where it was downloaded. Off by default.", + ) parser.add_argument( "--token-file", help="Path to a file containing a GDC auth token (overrides GDC_TOKEN env var)", ) + parser.add_argument( + "--retries", type=int, default=2, + help="Extra attempts for files that fail transiently (network errors, 429/5xx, checksum " + "mismatches) within this run, on top of each request's own transport-level retries " + "(default: 2). Useful in contexts like a Galaxy job where nothing will rerun the " + "command for you on a retry.", + ) + parser.add_argument( + "--retry-backoff", type=float, default=5.0, + help="Seconds to wait before each retry pass, multiplied by the attempt number (default: 5.0)", + ) parser.add_argument("--verbose", action="store_true") parser.set_defaults(func=run) return parser @@ -59,6 +79,21 @@ def run(args: argparse.Namespace) -> int: return 1 log.info("Detected source: %s (%d file(s))", source_name, len(entries)) + + log.info("Checking that all %d file(s) are accessible before downloading anything...", len(entries)) + failures = engine.check_access( + entries, source, output_dir=args.output_dir, verify=args.verify_checksum, extract=args.extract, + workers=args.workers, + ) + if failures: + log.error( + "%d of %d file(s) failed the pre-flight access check; aborting before downloading anything.", + len(failures), len(entries), + ) + for failure in failures: + log.error(" %s (%s): %s", failure.entry.filename, failure.entry.file_id, failure.detail) + return 1 + args.output_dir.mkdir(parents=True, exist_ok=True) results = engine.run( entries, @@ -66,12 +101,16 @@ def run(args: argparse.Namespace) -> int: args.output_dir, workers=args.workers, verify=args.verify_checksum, + extract=args.extract, + retries=args.retries, + retry_backoff=args.retry_backoff, ) failed = [r for r in results if r.status == "error"] mismatches = [r for r in results if r.status == "checksum_mismatch"] + extract_failed = [r for r in results if r.extract_error] print( f"\nDone: {len(results)} total, {len(failed)} error(s), " - f"{len(mismatches)} checksum mismatch(es)" + f"{len(mismatches)} checksum mismatch(es), {len(extract_failed)} extraction error(s)" ) - return 1 if failed or mismatches else 0 + return 1 if failed or mismatches or extract_failed else 0 diff --git a/cli_tools/mcdi/mcdi/download/engine.py b/cli_tools/mcdi/mcdi/download/engine.py index c45afc0..188b5d7 100644 --- a/cli_tools/mcdi/mcdi/download/engine.py +++ b/cli_tools/mcdi/mcdi/download/engine.py @@ -1,8 +1,10 @@ from __future__ import annotations import hashlib +import tarfile import threading import time +import zipfile from concurrent.futures import ThreadPoolExecutor, as_completed from dataclasses import dataclass from pathlib import Path @@ -11,6 +13,7 @@ from .. import version_string from ..net import build_session as _build_session +from . import archive from .sources.base import FileEntry, RateLimit, Source CHUNK_SIZE = 1024 * 256 @@ -61,11 +64,69 @@ def _md5sum(path: Path) -> str: return digest.hexdigest() +def _dest_path(output_dir: Path, entry: FileEntry) -> Path: + return output_dir / entry.rel_dir / entry.filename + + +def _already_present(dest_path: Path, entry: FileEntry, verify: bool) -> tuple[bool, str]: + """Return ``(satisfied, detail)``: whether ``dest_path`` already correctly holds ``entry``. + + Shared by the download step and the pre-flight access check, so a file + that doesn't need (re-)downloading also doesn't need its remote + accessibility re-verified on every rerun. + """ + if not (dest_path.exists() and dest_path.stat().st_size > 0): + return False, "" + if not verify or not entry.md5: + return True, "already present" + if _md5sum(dest_path) == entry.md5: + return True, "already present, checksum ok" + return False, "" # corrupt/incomplete; caller should re-fetch + + @dataclass class DownloadResult: entry: FileEntry status: str # "downloaded", "skipped", "checksum_mismatch", "error" detail: str = "" + extracted: bool = False + extract_error: str = "" + + +def _archived_path(output_dir: Path, entry: FileEntry) -> Path: + """Where ``entry``'s archive is relocated to once successfully extracted. + + Mirrors ``entry.rel_dir``/filename under a sibling of ``output_dir`` + (``.mcdi-archives/...``). Moving the archive there - instead + of leaving it in ``output_dir`` next to what it was extracted into - + means (a) a tool that recursively collects everything under + ``output_dir`` (e.g. a Galaxy ``discover_datasets`` with ``recurse``) + only ever sees the extracted contents, not a redundant copy of the + packed archive, and (b) the archive's presence here doubles as the + idempotency marker: extraction is already done for this entry iff a file + exists here. The archive isn't deleted, just moved aside - if extraction + fails, it's left in ``output_dir`` instead, so there's still something + to show for the download. + """ + archive_root = output_dir.with_name(output_dir.name + ".mcdi-archives") + return archive_root / entry.rel_dir / entry.filename + + +def _maybe_extract(result: DownloadResult, dest_path: Path, archived_path: Path) -> None: + """If requested and ``dest_path`` looks like an archive, extract it and relocate it. + + On success, moves the archive from ``dest_path`` to ``archived_path`` (see + ``_archived_path``); on failure, leaves it at ``dest_path``. + """ + if result.status not in ("downloaded", "skipped") or not archive.is_archive(dest_path): + return + try: + archive.extract(dest_path) + archived_path.parent.mkdir(parents=True, exist_ok=True) + dest_path.rename(archived_path) + result.extracted = True + except (archive.ArchiveError, OSError, tarfile.TarError, zipfile.BadZipFile) as exc: + result.extract_error = str(exc) def _download_one( @@ -75,17 +136,27 @@ def _download_one( output_dir: Path, verify: bool, pacer: "Pacer | None", + extract: bool = False, ) -> DownloadResult: dest_dir = output_dir / entry.rel_dir dest_dir.mkdir(parents=True, exist_ok=True) dest_path = dest_dir / entry.filename + archived_path = _archived_path(output_dir, entry) if extract else None + + if archived_path is not None: + present, detail = _already_present(archived_path, entry, verify) + if present: + result = DownloadResult(entry, "skipped", detail) + result.extracted = True + return result - if dest_path.exists() and dest_path.stat().st_size > 0: - if not verify or not entry.md5: - return DownloadResult(entry, "skipped", "already present") - if _md5sum(dest_path) == entry.md5: - return DownloadResult(entry, "skipped", "already present, checksum ok") - # fall through and re-download a corrupt/incomplete file + present, detail = _already_present(dest_path, entry, verify) + if present: + result = DownloadResult(entry, "skipped", detail) + if extract: + _maybe_extract(result, dest_path, archived_path) + return result + # if not present: falls through, including to re-fetch a corrupt/incomplete file if pacer: pacer.wait_turn() @@ -110,7 +181,34 @@ def _download_one( return DownloadResult(entry, "checksum_mismatch", "md5 did not match manifest") part_path.rename(dest_path) - return DownloadResult(entry, "downloaded") + result = DownloadResult(entry, "downloaded") + if extract: + _maybe_extract(result, dest_path, archived_path) + return result + + +# HTTP statuses worth retrying at the batch level: rate limiting and +# server-side/transient failures. Anything else (401/403/404/...) is a +# permanent-looking failure that a delayed retry won't fix. +_RETRYABLE_STATUSES = {429, 500, 502, 503, 504} + + +def _is_retryable(result: DownloadResult) -> bool: + # A checksum mismatch could be transient transfer corruption, not + # necessarily a bad source file, so it's worth one more attempt too. + if result.status == "checksum_mismatch": + return True + if result.status != "error": + return False + detail = result.detail + if detail.startswith("HTTP "): + try: + return int(detail.split()[1]) in _RETRYABLE_STATUSES + except (IndexError, ValueError): + return False + # Any other "error" detail came from a requests.RequestException (timeout, + # connection reset, DNS hiccup, ...) - inherently transient. + return True def run( @@ -119,6 +217,9 @@ def run( output_dir: Path, workers: int = 4, verify: bool = False, + extract: bool = False, + retries: int = 2, + retry_backoff: float = 5.0, ) -> list[DownloadResult]: session = build_session() rate_limit = source.rate_limit() @@ -127,15 +228,114 @@ def run( # effective concurrency to 1 when a pacer is active to keep pacing honest. effective_workers = 1 if pacer else workers - results: list[DownloadResult] = [] + def _pass(batch: list[FileEntry]) -> list[DownloadResult]: + pass_results: list[DownloadResult] = [] + with ThreadPoolExecutor(max_workers=effective_workers) as pool: + futures = { + pool.submit(_download_one, session, source, entry, output_dir, verify, pacer, extract): entry + for entry in batch + } + for future in as_completed(futures): + result = future.result() + pass_results.append(result) + detail = result.detail + if result.extracted: + detail = f"{detail}, extracted".lstrip(", ") + elif result.extract_error: + detail = f"{detail}, extract failed: {result.extract_error}".lstrip(", ") + print(f"[{result.status.upper():17}] {result.entry.filename} {detail}") + return pass_results + + results_by_id = {r.entry.file_id: r for r in _pass(entries)} + + # Batch-level retry, on top of the per-request transport retry already + # inside `build_session()`. This matters most for non-interactive runs + # (e.g. a Galaxy job) where nothing will manually rerun the command on + # the same output directory if a few files fail transiently. + attempt = 0 + while attempt < retries: + retry_entries = [r.entry for r in results_by_id.values() if _is_retryable(r)] + if not retry_entries: + break + attempt += 1 + print(f"\nRetrying {len(retry_entries)} file(s) that failed transiently (attempt {attempt}/{retries})...") + time.sleep(retry_backoff * attempt) + for result in _pass(retry_entries): + results_by_id[result.entry.file_id] = result + + return [results_by_id[entry.file_id] for entry in entries] + + +@dataclass +class AccessFailure: + entry: FileEntry + detail: str + + +def _check_access_one( + session: requests.Session, + source: Source, + entry: FileEntry, + pacer: "Pacer | None", +) -> "AccessFailure | None": + if pacer: + pacer.wait_turn() + kwargs = source.request_kwargs(entry) + headers = dict(kwargs.pop("headers", None) or {}) + headers["Range"] = "bytes=0-0" + try: + resp = session.get(entry.url, headers=headers, timeout=TIMEOUT_SECONDS, **kwargs) + except requests.RequestException as exc: + return AccessFailure(entry, str(exc)) + if resp.status_code not in (200, 206): + return AccessFailure(entry, f"HTTP {resp.status_code}") + return None + + +def check_access( + entries: list[FileEntry], + source: Source, + output_dir: "Path | None" = None, + verify: bool = False, + extract: bool = False, + workers: int = 4, +) -> list[AccessFailure]: + """Probe every entry with a 1-byte ranged request; return the ones that fail. + + Meant to run before any real download, so a manifest with one + inaccessible file (e.g. controlled-access without a valid token) is + caught in seconds instead of after downloading everything else first. + Two things narrow what actually needs a round trip: entries already + correctly present in ``output_dir`` (same check the download step uses - + including, if ``extract`` is set, entries already extracted and relocated + to ``output_dir``'s sibling archive directory - so a rerun over + mostly-complete output doesn't re-verify remote access for files it isn't + going to touch anyway) skip the check outright, and entries the source + can already vouch for as open (see ``Source.known_open``) skip the + per-file probe specifically. + """ + session = build_session() + rate_limit = source.rate_limit() + pacer = Pacer(rate_limit) if rate_limit else None + effective_workers = 1 if pacer else workers + + def _needs_probe(e: FileEntry) -> bool: + if extract and _already_present(_archived_path(output_dir, e), e, verify)[0]: + return False + return not _already_present(_dest_path(output_dir, e), e, verify)[0] + + to_probe = entries + if output_dir is not None: + to_probe = [e for e in entries if _needs_probe(e)] + + open_ids = source.known_open(to_probe, session) + to_check = [e for e in to_probe if e.file_id not in open_ids] + + failures: list[AccessFailure] = [] with ThreadPoolExecutor(max_workers=effective_workers) as pool: - futures = { - pool.submit(_download_one, session, source, entry, output_dir, verify, pacer): entry - for entry in entries - } + futures = [pool.submit(_check_access_one, session, source, entry, pacer) for entry in to_check] for future in as_completed(futures): - result = future.result() - results.append(result) - print(f"[{result.status.upper():17}] {result.entry.filename} {result.detail}") - - return results + failure = future.result() + if failure is not None: + failures.append(failure) + return failures diff --git a/cli_tools/mcdi/mcdi/download/sources/base.py b/cli_tools/mcdi/mcdi/download/sources/base.py index 6fe0099..eb133b9 100644 --- a/cli_tools/mcdi/mcdi/download/sources/base.py +++ b/cli_tools/mcdi/mcdi/download/sources/base.py @@ -6,6 +6,8 @@ from pathlib import Path from typing import Optional +import requests + # Delimiters tried, in order, when matching a manifest's header against each # source's schema. CANDIDATE_DELIMITERS = ("\t", ",") @@ -51,6 +53,16 @@ def rate_limit(self) -> Optional["RateLimit"]: """Optional pacing/rate-limit policy applied to downloads from this source.""" return None + def known_open(self, entries: list[FileEntry], session: requests.Session) -> set[str]: + """Best-effort ``file_id``s known accessible without a per-file network probe. + + Lets a source short-circuit the pre-flight access check (see + ``download.engine.check_access``) for entries it can already vouch + for cheaply, e.g. via a single bulk metadata query. Default: none, + so every entry gets individually probed. + """ + return set() + @dataclass class RateLimit: diff --git a/cli_tools/mcdi/mcdi/download/sources/gdc.py b/cli_tools/mcdi/mcdi/download/sources/gdc.py index 7668bed..cad5438 100644 --- a/cli_tools/mcdi/mcdi/download/sources/gdc.py +++ b/cli_tools/mcdi/mcdi/download/sources/gdc.py @@ -1,12 +1,18 @@ from __future__ import annotations import csv +import logging from pathlib import Path from typing import Optional +import requests + from .base import FileEntry, Source +log = logging.getLogger("mcdi.download.gdc") + API_BASE = "https://api.gdc.cancer.gov" +FILES_ENDPOINT = f"{API_BASE}/files" # Columns present in every GDC manifest (TSV) generated by the portal or # the `manifest` API endpoint. @@ -46,3 +52,30 @@ def request_kwargs(self, entry: FileEntry) -> dict: if self.token: headers["X-Auth-Token"] = self.token return {"headers": headers} + + def known_open(self, entries: list[FileEntry], session: requests.Session) -> set[str]: + """Bulk-query GDC's ``access`` field for every id; no token needed for this. + + This just narrows which entries the per-file pre-flight probe has to + touch — open files never need the individual round trip. + """ + ids = [e.file_id for e in entries] + if not ids: + return set() + try: + resp = session.post( + FILES_ENDPOINT, + json={ + "filters": {"op": "in", "content": {"field": "file_id", "value": ids}}, + "fields": "file_id,access", + "size": len(ids), + "format": "JSON", + }, + timeout=60, + ) + resp.raise_for_status() + hits = resp.json()["data"]["hits"] + except (requests.RequestException, KeyError, ValueError) as exc: + log.warning("Could not bulk-check GDC access ahead of time (%s); falling back to per-file checks.", exc) + return set() + return {h["file_id"] for h in hits if h.get("access") == "open"} diff --git a/cli_tools/mcdi/tests/test_archive.py b/cli_tools/mcdi/tests/test_archive.py new file mode 100644 index 0000000..09ea06e --- /dev/null +++ b/cli_tools/mcdi/tests/test_archive.py @@ -0,0 +1,82 @@ +import gzip +import io +import tarfile +import zipfile +from pathlib import Path + +import pytest + +from mcdi.download import archive + + +def test_is_archive_recognizes_common_suffixes(): + for name in ["a.tar.gz", "a.tgz", "a.tar.bz2", "a.tar.xz", "a.tar", "a.zip", "a.gz", "a.bz2", "a.xz"]: + assert archive.is_archive(Path(name)), name + + +def test_is_archive_false_for_plain_files(): + assert not archive.is_archive(Path("a.txt")) + assert not archive.is_archive(Path("a.svs")) + + +def test_extract_tar_gz(tmp_path): + src_dir = tmp_path / "src" + src_dir.mkdir() + (src_dir / "inner.txt").write_text("hello") + + archive_path = tmp_path / "bundle.tar.gz" + with tarfile.open(archive_path, "w:gz") as tf: + tf.add(src_dir / "inner.txt", arcname="inner.txt") + + result = archive.extract(archive_path) + assert result == tmp_path.resolve() + assert (tmp_path / "inner.txt").read_text() == "hello" + + +def test_extract_zip(tmp_path): + archive_path = tmp_path / "bundle.zip" + with zipfile.ZipFile(archive_path, "w") as zf: + zf.writestr("inner.txt", "hello zip") + + archive.extract(archive_path) + assert (tmp_path / "inner.txt").read_text() == "hello zip" + + +def test_extract_bare_gz(tmp_path): + archive_path = tmp_path / "data.txt.gz" + with gzip.open(archive_path, "wb") as f: + f.write(b"plain content") + + result = archive.extract(archive_path) + assert result == tmp_path / "data.txt" + assert result.read_bytes() == b"plain content" + + +def test_extract_rejects_tar_path_traversal(tmp_path): + archive_path = tmp_path / "evil.tar" + with tarfile.open(archive_path, "w") as tf: + info = tarfile.TarInfo(name="../escaped.txt") + data = b"pwned" + info.size = len(data) + tf.addfile(info, io.BytesIO(data)) + + with pytest.raises(archive.ArchiveError): + archive.extract(archive_path) + assert not (tmp_path.parent / "escaped.txt").exists() + + +def test_extract_rejects_zip_path_traversal(tmp_path): + archive_path = tmp_path / "evil.zip" + with zipfile.ZipFile(archive_path, "w") as zf: + zf.writestr("../escaped.txt", "pwned") + + with pytest.raises(archive.ArchiveError): + archive.extract(archive_path) + assert not (tmp_path.parent / "escaped.txt").exists() + + +def test_extract_unsupported_type_raises(tmp_path): + path = tmp_path / "plain.txt" + path.write_text("not an archive") + with pytest.raises(archive.ArchiveError): + archive.extract(path) diff --git a/cli_tools/mcdi/tests/test_download_cli.py b/cli_tools/mcdi/tests/test_download_cli.py index ebac117..6e19428 100644 --- a/cli_tools/mcdi/tests/test_download_cli.py +++ b/cli_tools/mcdi/tests/test_download_cli.py @@ -1,4 +1,6 @@ import hashlib +import io +import tarfile from mcdi.cli import main @@ -10,6 +12,15 @@ def _md5(data: bytes) -> str: return hashlib.md5(data).hexdigest() +def _tar_gz_bytes(member_name: str, content: bytes) -> bytes: + buf = io.BytesIO() + with tarfile.open(fileobj=buf, mode="w:gz") as tf: + info = tarfile.TarInfo(name=member_name) + info.size = len(content) + tf.addfile(info, io.BytesIO(content)) + return buf.getvalue() + + def _write_gdc_manifest(path, rows): lines = ["id\tfilename\tmd5\tsize\tstate"] for file_id, filename, md5, size in rows: @@ -17,6 +28,14 @@ def _write_gdc_manifest(path, rows): path.write_text("\n".join(lines) + "\n") +def _mock_known_open_empty(requests_mock): + """Stub the bulk access-field lookup `check_access` makes before probing + individual files, so tests aren't asserting anything about which files + GDC would call "open" - they exercise the per-file probe fallback path + instead, which is what actually needs mocked file URLs anyway.""" + requests_mock.post("https://api.gdc.cancer.gov/files", json={"data": {"hits": []}}) + + def test_missing_manifest_exits_input_error(tmp_path): rc = main([ "download", @@ -27,6 +46,7 @@ def test_missing_manifest_exits_input_error(tmp_path): def test_full_download_with_checksum_verification(tmp_path, requests_mock): + _mock_known_open_empty(requests_mock) manifest = tmp_path / "gdc_manifest.txt" _write_gdc_manifest(manifest, [ ("uuid1", "a.txt", _md5(FILE_A), len(FILE_A)), @@ -50,7 +70,8 @@ def test_full_download_with_checksum_verification(tmp_path, requests_mock): assert a.read_bytes() == FILE_A assert b.read_bytes() == FILE_B - # Re-running should skip already-downloaded, checksum-verified files. + # Re-running should skip already-downloaded, checksum-verified files - + # including skipping their pre-flight access re-check. rc_again = main([ "download", "--manifest", str(manifest), @@ -58,10 +79,13 @@ def test_full_download_with_checksum_verification(tmp_path, requests_mock): "--verify-checksum", ]) assert rc_again == 0 - assert requests_mock.call_count == 2 # no re-fetch on the second run + # First run: 1 known_open lookup + 2 pre-flight probes + 2 real downloads. + # Second run: everything already satisfied locally, so zero new calls. + assert requests_mock.call_count == 5 def test_checksum_mismatch_reported_as_failure(tmp_path, requests_mock): + _mock_known_open_empty(requests_mock) manifest = tmp_path / "gdc_manifest.txt" _write_gdc_manifest(manifest, [("uuid1", "a.txt", "deadbeef" * 4, len(FILE_A))]) requests_mock.get("https://api.gdc.cancer.gov/data/uuid1", content=FILE_A) @@ -71,11 +95,13 @@ def test_checksum_mismatch_reported_as_failure(tmp_path, requests_mock): "--manifest", str(manifest), "--output-dir", str(tmp_path / "out"), "--verify-checksum", + "--retries", "0", # a checksum mismatch is retried by default; keep this test fast/deterministic ]) assert rc == 1 def test_gdc_token_passed_as_header(tmp_path, requests_mock): + _mock_known_open_empty(requests_mock) manifest = tmp_path / "gdc_manifest.txt" _write_gdc_manifest(manifest, [("uuid1", "a.txt", _md5(FILE_A), len(FILE_A))]) requests_mock.get("https://api.gdc.cancer.gov/data/uuid1", content=FILE_A) @@ -90,6 +116,8 @@ def test_gdc_token_passed_as_header(tmp_path, requests_mock): "--token-file", str(token_file), ]) assert rc == 0 + # Both the pre-flight probe and the real download carry the token; the + # real download happens last, so it's what last_request reflects. assert requests_mock.last_request.headers["X-Auth-Token"] == "secret-token" @@ -104,3 +132,105 @@ def test_bad_token_file_exits_input_error(tmp_path): "--token-file", str(tmp_path / "missing-token.txt"), ]) assert rc == 2 + + +def test_preflight_aborts_before_downloading_anything(tmp_path, requests_mock): + """One inaccessible file among several must stop the whole run before any + download starts - not be discovered only after the others succeed.""" + _mock_known_open_empty(requests_mock) + manifest = tmp_path / "gdc_manifest.txt" + _write_gdc_manifest(manifest, [ + ("uuid1", "a.txt", _md5(FILE_A), len(FILE_A)), + ("uuid2", "b.txt", _md5(FILE_B), len(FILE_B)), + ]) + requests_mock.get("https://api.gdc.cancer.gov/data/uuid1", content=FILE_A) + requests_mock.get("https://api.gdc.cancer.gov/data/uuid2", status_code=403, json={"message": "not authorized"}) + + output_dir = tmp_path / "out" + rc = main([ + "download", + "--manifest", str(manifest), + "--output-dir", str(output_dir), + ]) + assert rc == 1 + # Nothing was downloaded, including the file that would have succeeded. + assert not output_dir.exists() or not any(output_dir.rglob("*")) + + +def test_extract_off_by_default(tmp_path, requests_mock): + _mock_known_open_empty(requests_mock) + archive_bytes = _tar_gz_bytes("inner.txt", b"payload") + manifest = tmp_path / "gdc_manifest.txt" + _write_gdc_manifest(manifest, [("uuid1", "bundle.tar.gz", _md5(archive_bytes), len(archive_bytes))]) + requests_mock.get("https://api.gdc.cancer.gov/data/uuid1", content=archive_bytes) + + output_dir = tmp_path / "out" + rc = main([ + "download", + "--manifest", str(manifest), + "--output-dir", str(output_dir), + ]) + assert rc == 0 + + dest_dir = output_dir / "gdc" / "uuid1" + assert (dest_dir / "bundle.tar.gz").exists() + assert not (dest_dir / "inner.txt").exists() + + +def test_extract_flag_unpacks_archive(tmp_path, requests_mock): + _mock_known_open_empty(requests_mock) + archive_bytes = _tar_gz_bytes("inner.txt", b"payload") + manifest = tmp_path / "gdc_manifest.txt" + _write_gdc_manifest(manifest, [("uuid1", "bundle.tar.gz", _md5(archive_bytes), len(archive_bytes))]) + requests_mock.get("https://api.gdc.cancer.gov/data/uuid1", content=archive_bytes) + + output_dir = tmp_path / "out" + rc = main([ + "download", + "--manifest", str(manifest), + "--output-dir", str(output_dir), + "--extract", + ]) + assert rc == 0 + + dest_dir = output_dir / "gdc" / "uuid1" + # only the extracted contents remain at the manifest's output path... + assert not (dest_dir / "bundle.tar.gz").exists() + assert (dest_dir / "inner.txt").read_bytes() == b"payload" + # ...the archive moved aside to the sibling archive directory, not deleted + archived_dir = output_dir.with_name(output_dir.name + ".mcdi-archives") / "gdc" / "uuid1" + assert (archived_dir / "bundle.tar.gz").read_bytes() == archive_bytes + + # re-running with --extract again doesn't re-fetch, re-probe, or re-extract + rc_again = main([ + "download", + "--manifest", str(manifest), + "--output-dir", str(output_dir), + "--extract", + ]) + assert rc_again == 0 + assert requests_mock.call_count == 3 # 1 known_open + 1 pre-flight probe + 1 download, first run only + + +def test_extract_failure_leaves_archive_in_place(tmp_path, requests_mock): + _mock_known_open_empty(requests_mock) + # Named like a tar.gz but not actually one - extraction must fail cleanly. + bad_bytes = b"not actually a gzip stream" + manifest = tmp_path / "gdc_manifest.txt" + _write_gdc_manifest(manifest, [("uuid1", "bundle.tar.gz", _md5(bad_bytes), len(bad_bytes))]) + requests_mock.get("https://api.gdc.cancer.gov/data/uuid1", content=bad_bytes) + + output_dir = tmp_path / "out" + rc = main([ + "download", + "--manifest", str(manifest), + "--output-dir", str(output_dir), + "--extract", + ]) + assert rc == 1 # extraction errors are reported as a run failure + + dest_dir = output_dir / "gdc" / "uuid1" + # the archive was NOT moved aside, since extraction never succeeded + assert (dest_dir / "bundle.tar.gz").read_bytes() == bad_bytes + archived_dir = output_dir.with_name(output_dir.name + ".mcdi-archives") / "gdc" / "uuid1" + assert not archived_dir.exists() diff --git a/cli_tools/mcdi/tests/test_download_engine.py b/cli_tools/mcdi/tests/test_download_engine.py new file mode 100644 index 0000000..dab591a --- /dev/null +++ b/cli_tools/mcdi/tests/test_download_engine.py @@ -0,0 +1,114 @@ +import hashlib +from pathlib import Path + +from mcdi.download import engine +from mcdi.download.sources.gdc import GDCSource +from mcdi.download.sources.base import FileEntry + +FILE_A = b"hello world\n" + + +def _entry(file_id="uuid1", filename="a.txt", md5=None) -> FileEntry: + return FileEntry( + file_id=file_id, + filename=filename, + rel_dir=Path("gdc") / file_id, + url=f"https://api.gdc.cancer.gov/data/{file_id}", + md5=md5, + ) + + +def _result(status: str, detail: str = "") -> engine.DownloadResult: + return engine.DownloadResult(_entry(), status, detail) + + +def test_is_retryable_transient_http_statuses(): + for code in (429, 500, 502, 503, 504): + assert engine._is_retryable(_result("error", f"HTTP {code}")) + + +def test_is_retryable_false_for_permanent_http_statuses(): + for code in (400, 401, 403, 404): + assert not engine._is_retryable(_result("error", f"HTTP {code}")) + + +def test_is_retryable_true_for_connection_errors(): + # A requests.RequestException detail doesn't start with "HTTP ". + assert engine._is_retryable(_result("error", "Connection reset by peer")) + + +def test_is_retryable_true_for_checksum_mismatch(): + assert engine._is_retryable(_result("checksum_mismatch", "md5 did not match manifest")) + + +def test_is_retryable_false_for_downloaded_and_skipped(): + assert not engine._is_retryable(_result("downloaded")) + assert not engine._is_retryable(_result("skipped", "already present")) + + +def test_known_open_short_circuits_on_empty_list(): + # No requests_mock registration at all - this must not attempt any HTTP call. + assert GDCSource().known_open([], session=None) == set() + + +def test_check_access_skips_files_already_satisfied_locally(tmp_path, requests_mock): + # No mocks registered for anything - if check_access tried to touch the + # network here, the test would fail with a mocking error. + entry = _entry(md5="deadbeef") + dest = tmp_path / "gdc" / "uuid1" / "a.txt" + dest.parent.mkdir(parents=True) + dest.write_bytes(FILE_A) + + failures = engine.check_access([entry], GDCSource(), output_dir=tmp_path, verify=False) + assert failures == [] + assert requests_mock.call_count == 0 + + +def test_check_access_skips_known_open_files(requests_mock): + entry = _entry() + requests_mock.post("https://api.gdc.cancer.gov/files", json={"data": {"hits": [{"file_id": "uuid1", "access": "open"}]}}) + + failures = engine.check_access([entry], GDCSource()) + assert failures == [] + # Only the bulk lookup ran; no per-file probe was needed for a known-open file. + assert requests_mock.call_count == 1 + + +def test_check_access_reports_inaccessible_files(requests_mock): + open_entry = _entry(file_id="uuid1", filename="a.txt") + denied_entry = _entry(file_id="uuid2", filename="b.txt") + requests_mock.post("https://api.gdc.cancer.gov/files", json={"data": {"hits": []}}) + requests_mock.get("https://api.gdc.cancer.gov/data/uuid1", content=FILE_A) + requests_mock.get("https://api.gdc.cancer.gov/data/uuid2", status_code=403, json={"message": "denied"}) + + failures = engine.check_access([open_entry, denied_entry], GDCSource()) + assert [f.entry.file_id for f in failures] == ["uuid2"] + assert "403" in failures[0].detail + + +def test_run_retries_transient_failure_then_succeeds(tmp_path, requests_mock): + entry_md5 = hashlib.md5(FILE_A).hexdigest() + requests_mock.get( + "https://api.gdc.cancer.gov/data/uuid1", + [ + {"status_code": 503, "content": b""}, + {"status_code": 200, "content": FILE_A}, + ], + ) + entry = _entry(md5=entry_md5) + + results = engine.run([entry], GDCSource(), tmp_path, retries=1, retry_backoff=0) + assert len(results) == 1 + assert results[0].status == "downloaded" + assert (tmp_path / "gdc" / "uuid1" / "a.txt").read_bytes() == FILE_A + + +def test_run_does_not_retry_permanent_failure(tmp_path, requests_mock): + requests_mock.get("https://api.gdc.cancer.gov/data/uuid1", status_code=403, json={"message": "denied"}) + entry = _entry() + + results = engine.run([entry], GDCSource(), tmp_path, retries=2, retry_backoff=0) + assert len(results) == 1 + assert results[0].status == "error" + # One attempt only - no retry passes for a permanent-looking failure. + assert requests_mock.call_count == 1 diff --git a/tools/manifest_downloader/macros.xml b/tools/manifest_downloader/macros.xml new file mode 100644 index 0000000..d63750b --- /dev/null +++ b/tools/manifest_downloader/macros.xml @@ -0,0 +1,42 @@ + + 0.4.0 + 0 + 25.1 + + + + + quay.io/goeckslab/mcdi:@TOOL_VERSION@ + + + + + + + + + + + + diff --git a/tools/manifest_downloader/manifest_downloader.xml b/tools/manifest_downloader/manifest_downloader.xml index 815fcd5..1f00158 100644 --- a/tools/manifest_downloader/manifest_downloader.xml +++ b/tools/manifest_downloader/manifest_downloader.xml @@ -2,30 +2,20 @@ diff --git a/tools/manifest_downloader/test-data/gdc_manifest_archive.txt b/tools/manifest_downloader/test-data/gdc_manifest_archive.txt new file mode 100644 index 0000000..8bf0d38 --- /dev/null +++ b/tools/manifest_downloader/test-data/gdc_manifest_archive.txt @@ -0,0 +1,2 @@ +id filename md5 size state +b23449fb-b4e2-4816-9f9e-721eed95a124 597f3e18-9281-429c-842e-19eaaffc6842.qc_filtered_feature_bc_matrix.tar.gz 2d4e3510d40dec23524673b22bc22ecc 8519124 released diff --git a/tools/manifest_gdc/macros.xml b/tools/manifest_gdc/macros.xml index 93f0942..fb6752b 100644 --- a/tools/manifest_gdc/macros.xml +++ b/tools/manifest_gdc/macros.xml @@ -1,5 +1,5 @@ - 0.1.0 + 0.4.0 0 22.05