Skip to content

fix: use chunked read for integration and preset manifest hash - #3843

Open
Quratulain-bilal wants to merge 2 commits into
github:mainfrom
Quratulain-bilal:fix/integrations-hash-chunked-read
Open

fix: use chunked read for integration and preset manifest hash#3843
Quratulain-bilal wants to merge 2 commits into
github:mainfrom
Quratulain-bilal:fix/integrations-hash-chunked-read

Conversation

@Quratulain-bilal

Copy link
Copy Markdown
Contributor

Summary

Replace unbounded f.read() with chunked iteration to prevent excessive memory allocation.

Changes

  • integrations/catalog.py: Use chunked read in get_hash()
  • presets/__init__.py: Use chunked read in get_hash()

Replace unbounded fh.read() with chunked iteration to prevent excessive
memory allocation on large or corrupted manifest files. Applies to both
integrations/catalog.py and presets/__init__.py get_hash() methods.

Copilot AI 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.

Pull request overview

Updates manifest hashing to process files incrementally, reducing peak memory usage.

Changes:

  • Hash integration descriptors in 8 KiB chunks.
  • Hash preset manifests in 8 KiB chunks.
Show a summary per file
File Description
src/specify_cli/integrations/catalog.py Adds chunked descriptor hashing.
src/specify_cli/presets/__init__.py Adds chunked preset manifest hashing.

Review details

Tip

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

  • Files reviewed: 2/2 changed files
  • Comments generated: 2
  • Review effort level: Medium

Comment thread src/specify_cli/presets/__init__.py
Comment thread src/specify_cli/integrations/catalog.py
The existing tests only checked the sha256: prefix, which would pass
even if the chunked hash was broken. Now verify the complete hash
matches hashlib.sha256(content).hexdigest() to exercise the multi-chunk
path introduced by the chunked read change.
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