Skip to content

Raise dedicated HashValidationError on download/extract hash mismatch#9001

Open
vishnukannaujia wants to merge 1 commit into
Project-MONAI:devfrom
vishnukannaujia:fix/8832-hash-mismatch-exception
Open

Raise dedicated HashValidationError on download/extract hash mismatch#9001
vishnukannaujia wants to merge 1 commit into
Project-MONAI:devfrom
vishnukannaujia:fix/8832-hash-mismatch-exception

Conversation

@vishnukannaujia

Copy link
Copy Markdown
Contributor

Fixes #8832.

Summary

Follow-up to the note left in #8699: a hash-validation failure should raise something specific so it is not silently suppressed by skip_if_downloading_fails.

Today download_url and extractall raise a plain RuntimeError on a hash mismatch, with a message containing "md5 check". The test helper skip_if_downloading_fails (tests/test_utils.py) matches that substring in DOWNLOAD_FAIL_MSGS and converts the failure into a skipped test — the same treatment as transient network errors. As a result a genuine hash mismatch (data corruption, or the remote content having changed) is quietly skipped instead of surfaced.

Changes

  • Add HashValidationError(RuntimeError) in monai/apps/utils.py (exported in __all__). It is a subclass of RuntimeError, so existing except RuntimeError handlers keep working and no message text changes.
  • Raise HashValidationError at all three hash checks: existing file and downloaded file (download_url), and compressed file (extractall). Docstring Raises: entries updated accordingly.
  • skip_if_downloading_fails now re-raises HashValidationError (before the generic RuntimeError handling) instead of skipping.
  • Remove the now-redundant "md5 check" entry from DOWNLOAD_FAIL_MSGS — hash failures are represented by the dedicated type, and this entry would otherwise still swallow them.

Backward compatibility

  • HashValidationError subclasses RuntimeError; callers catching RuntimeError are unaffected.
  • Message strings are unchanged, so existing assertions like str(e).startswith("md5 check") in test_download_and_extract.py still hold.

Tests

New offline tests (no network required) in tests/apps/test_download_and_extract.py:

  • existing on-disk file with a wrong hash → HashValidationError;
  • extractall with a wrong hash → HashValidationError;
  • HashValidationError is a RuntimeError subclass;
  • skip_if_downloading_fails propagates HashValidationError rather than skipping.

Note for reviewers

Removing "md5 check" from DOWNLOAD_FAIL_MSGS is a deliberate behavior change: a fully-downloaded file whose hash does not match will now fail rather than skip. This is the intent of #8832 (distinguishing corruption from transient/incomplete downloads, which still surface as "unexpected EOF"/"network issue"). Happy to adjust if maintainers prefer to keep skipping in a specific CI path.

Fixes Project-MONAI#8832.

Hash-validation failures in download_url and extractall previously raised a
plain RuntimeError whose message contains "md5 check". The test helper
skip_if_downloading_fails matched that substring and turned genuine hash
mismatches (data corruption / content mismatch) into skipped tests, hiding
real failures behind the same path as transient network errors.

Introduce HashValidationError(RuntimeError) and raise it at all three hash
checks (existing file, downloaded file, compressed file). Subclassing
RuntimeError keeps existing `except RuntimeError` handlers working and the
message text unchanged. skip_if_downloading_fails now re-raises
HashValidationError instead of skipping, and the now-redundant "md5 check"
entry is removed from DOWNLOAD_FAIL_MSGS.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Vishnu Kannaujia <vishnu.kannaujia@gmail.com>
@coderabbitai

coderabbitai Bot commented Jul 18, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

Adds and exports HashValidationError, a RuntimeError subclass used for hash mismatches in downloads and archive extraction. Updates API documentation and ensures download-skip utilities re-raise hash errors. Tests cover mismatch handling, inheritance compatibility, and skip behavior.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed Clearly states the new HashValidationError behavior for download/extract hash mismatches.
Description check ✅ Passed It covers the issue, change summary, compatibility, and tests, though the template checkboxes are omitted.
Linked Issues check ✅ Passed The PR raises a dedicated exception for hash mismatches and prevents skip_if_downloading_fails from suppressing them, matching #8832.
Out of Scope Changes check ✅ Passed The added tests and skip-logic changes support the same hash-validation fix and are not unrelated scope.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
monai/apps/utils.py (2)

45-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Sort __all__ alphabetically.

Sort the exports to improve readability.

♻️ Proposed refactor
 __all__ = [
+    "HashValidationError",
+    "SUPPORTED_HASH_TYPES",
     "check_hash",
+    "download_and_extract",
     "download_url",
     "extractall",
-    "download_and_extract",
     "get_logger",
-    "SUPPORTED_HASH_TYPES",
-    "HashValidationError",
 ]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@monai/apps/utils.py` around lines 45 - 53, Sort the entries in __all__
alphabetically, including check_hash, download_url, extractall,
download_and_extract, get_logger, SUPPORTED_HASH_TYPES, and HashValidationError.

Source: Linters/SAST tools


235-242: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consolidate duplicate docstring entries.

HashValidationError is listed twice in the Raises section. Combine the descriptions into a single entry. As per path instructions, ensure definitions describe each raised exception appropriately in the Google-style format.

♻️ Proposed refactor
     Raises:
-        HashValidationError: When the hash validation of the ``filepath`` existing file fails.
+        HashValidationError: When the hash validation of the ``filepath`` existing file or the ``url`` downloaded file fails.
         RuntimeError: When a network issue or denied permission prevents the
             file download from ``url`` to ``filepath``.
         URLError: See urllib.request.urlretrieve.
         HTTPError: See urllib.request.urlretrieve.
         ContentTooShortError: See urllib.request.urlretrieve.
         IOError: See urllib.request.urlretrieve.
-        HashValidationError: When the hash validation of the ``url`` downloaded file fails.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@monai/apps/utils.py` around lines 235 - 242, Consolidate the duplicate
HashValidationError entries in the Raises section of the relevant download
function’s docstring into one Google-style entry that covers both existing-file
and downloaded-file hash validation failures, while preserving the other
exception descriptions.

Source: Path instructions

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@monai/apps/utils.py`:
- Around line 45-53: Sort the entries in __all__ alphabetically, including
check_hash, download_url, extractall, download_and_extract, get_logger,
SUPPORTED_HASH_TYPES, and HashValidationError.
- Around line 235-242: Consolidate the duplicate HashValidationError entries in
the Raises section of the relevant download function’s docstring into one
Google-style entry that covers both existing-file and downloaded-file hash
validation failures, while preserving the other exception descriptions.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d0887511-0050-43e0-984a-a6a0b4b95f42

📥 Commits

Reviewing files that changed from the base of the PR and between 5da2472 and ffcb43a.

📒 Files selected for processing (3)
  • monai/apps/utils.py
  • tests/apps/test_download_and_extract.py
  • tests/test_utils.py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Raise different exception when hash value of download is wrong

1 participant