Skip to content

HDXDSYS-2562 Update resource change detection to use CRC for zip - #8

Merged
alexandru-m-g merged 11 commits into
mainfrom
zip_detection
Jan 13, 2026
Merged

alexandru-m-g merged 11 commits into
mainfrom
zip_detection

Conversation

@mcarans

@mcarans mcarans commented Jan 5, 2026

Copy link
Copy Markdown
Collaborator

This is a rewrite to support CRC for zip. It uses get ranges to avoid downloading the whole zip for large zip files. Using head requests was no longer helpful, so I integrated it all into the get request logic. For Excel, it is still required to hash the sheets as I tested trying to CRC a subset of the xlsx XML files but it didn't work.

The failing test is styling in task_manager.py. I wasn't sure if you need that result somewhere or if I can just remove the "result =" part which appears to be unused:
result = await pipeline.execute()

@mcarans
mcarans requested a review from alexandru-m-g January 5, 2026 04:08
@github-actions

github-actions Bot commented Jan 5, 2026

Copy link
Copy Markdown

Test Results

7 tests   - 2   3 ✅ ±0   1m 40s ⏱️ - 1m 42s
1 suites ±0   4 💤 ±0 
1 files   ±0   0 ❌  - 2 

Results for commit 5246a32. ± Comparison against base commit b75777c.

This pull request removes 2 tests.
tests.test_headresults.TestHeadResults ‑ test_headresults
tests.test_headretrieval.TestHeadRetrieve ‑ test_retrieval

♻️ This comment has been updated with latest results.

@mcarans
mcarans marked this pull request as draft January 5, 2026 04:20
@mcarans

mcarans commented Jan 6, 2026

Copy link
Copy Markdown
Collaborator Author

I've changed to _ = await pipeline.execute() to fix the ruff check.

I've also added the CRC change to HDX Python API in this PR.

@mcarans
mcarans marked this pull request as ready for review January 6, 2026 03:10
@mcarans

mcarans commented Jan 6, 2026

Copy link
Copy Markdown
Collaborator Author

The logic is:

  1. Open normal get request (no range)
  2. Read first 4 bytes
  3. Check server can understand range requests
  4. If it can and we have a zip that is > 30Mb and not an xlsx, then close the get request and get the CRC using 2 more get requests to get the tail and central directory (thus avoiding reading all of a large zip file but putting a 30Mb price on the cost of 3 get requests)
  5. If not, then continue the get request (don't close it)
  6. If the file is larger than 400Mb then use etag if there is one (even if it's a zip) or don't hash (too big)
  7. If the file has an etag and is not a zip, use it
  8. Otherwise read the rest of the file and use that and the signature
  9. If it is xlsx, do MD5 hash of each sheets's values, falling back on simple MD5 if for some reason that fails
  10. If it is zip (< 30Mb) do CRC using whole file, falling back on simple MD5 if for some reason that fails
  11. If it is anything else do simple MD5

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors resource change detection to support CRC (Cyclic Redundancy Check) for ZIP files, using HTTP range requests to avoid downloading entire large ZIP files. The main changes consolidate HEAD and GET request logic into a unified retrieval approach.

Key changes:

  • Implements CRC-based change detection for ZIP files using range requests for files > 30MB
  • Removes separate HEAD request logic, integrating it into GET requests with signature and mimetype validation
  • Continues to use hash-based change detection for Excel files by hashing sheet contents

Reviewed changes

Copilot reviewed 18 out of 21 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/test_retrieval.py Updated test expectations to match new 9-element tuple format with additional metadata (signature match, mime match, size match, hash type, status code)
tests/test_results.py Expanded test assertions to validate new resource status fields including hash types, match indicators, and error handling
tests/test_headretrieval.py Removed - HEAD request functionality integrated into main retrieval
tests/test_headresults.py Removed - Results processing consolidated into single class
tests/conftest.py Added two new test URLs for XLSX and large ZIP files
src/hdx/resource/changedetection/utilities.py Updated status fields to reflect new validation checks; changed import from write_list_to_csv to save_iterable; added broken_link default
src/hdx/resource/changedetection/task_manager.py Code formatting improvements and replaced unused result variable with underscore
src/hdx/resource/changedetection/retrieval_utilities.py New utility module with signature/mimetype checking functions and HTTP size extraction
src/hdx/resource/changedetection/retrieval.py Major refactoring: integrated HEAD/GET logic, added CRC calculation for large ZIPs using ranges, hash type tracking (md5, crc, etag variations)
src/hdx/resource/changedetection/results.py Updated to process expanded result tuples with signature/mime/size match validation and hash type categorization
src/hdx/resource/changedetection/name_generator.py Reformatted tuple literals to multi-line format
src/hdx/resource/changedetection/head_retrieval.py Removed - functionality merged into retrieval.py
src/hdx/resource/changedetection/head_results.py Removed - functionality merged into results.py
src/hdx/resource/changedetection/main.py Simplified to use single Retrieval class instead of separate HEAD/GET phases
requirements.txt Updated dependency versions
requirements-test.txt Updated test dependency versions
pyproject.toml Updated minimum hdx-python-* library versions
hatch.toml Updated ruff linter version
.vscode/settings.json Added trailing newline
.vscode/launch.json Added trailing newline
.pre-commit-config.yaml Updated pre-commit hook versions

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/hdx/resource/changedetection/retrieval.py Outdated
Comment thread src/hdx/resource/changedetection/utilities.py
@alexandru-m-g
alexandru-m-g merged commit 741e164 into main Jan 13, 2026
3 checks passed
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.

3 participants