feat: Add file_converter extension module (Issue #54)#56
Open
vaibhav45sktech wants to merge 5 commits intodbpedia:mainfrom
Open
feat: Add file_converter extension module (Issue #54)#56vaibhav45sktech wants to merge 5 commits intodbpedia:mainfrom
vaibhav45sktech wants to merge 5 commits intodbpedia:mainfrom
Conversation
- Create new file_converter.py extension module in databusclient/extensions/ - Implements FileConverter class with streaming pipeline support - Supports gzip decompression with optional checksum validation - Provides methods for compress_gzip_stream and decompress_gzip_stream - Minimal version as suggested in issue to start with gzip + checksum - Can be extended later to support more compression formats
📝 WalkthroughWalkthroughAdds a new FileConverter class in databusclient/extensions/file_converter.py offering streaming gzip compress/decompress and SHA-256 checksum validation using 8KB chunks and only Python stdlib. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@databusclient/extensions/file_converter.py`:
- Around line 38-50: The code creates source_hasher from expected_checksum but
never uses it, so remove the unused expected_checksum parameter and the
source_hasher/hash-from-compressed-stream logic from the FileConverter method
that reads gzip (the block using gzip.open, FileConverter.CHUNK_SIZE, hasher,
and output_stream), keep only validate_checksum-driven hasher for decompressed
chunks, and update the method signature and return value accordingly; then
update callers to perform checksum validation on the compressed input stream
(e.g., via validate_checksum_stream) before calling this decompression routine.
🧹 Nitpick comments (1)
databusclient/extensions/file_converter.py (1)
70-103:seek(0)assumes a seekable stream; return type is misleading.Two concerns:
Line 88:
input_stream.seek(0)will raise on non-seekable streams (e.g., network response bodies, pipes). Since the PR objective targets integration with the download pipeline, callers will need to be aware of this constraint. Consider either documenting the seekable requirement, removing theseek(0)call (let the caller manage stream position), or accepting a non-seekable stream and removing the seek.Return type: The method signature says
-> boolbut it never returnsFalse— it either returnsTrueor raisesIOError. Consider returningboolwithout raising (let the caller decide), or changing the return type toNoneand only raising on failure. Pick one contract and be consistent.Suggested approach: remove seek, return bool without raising
`@staticmethod` def validate_checksum_stream( input_stream: BinaryIO, expected_checksum: str ) -> bool: """Validate SHA256 checksum of a stream. Args: - input_stream: Input stream to validate + input_stream: Input stream to validate (must be positioned at start) expected_checksum: Expected SHA256 checksum Returns: - True if checksum matches + True if checksum matches, False otherwise - - Raises: - IOError: If checksum validation fails """ hasher = hashlib.sha256() - input_stream.seek(0) - + while True: chunk = input_stream.read(FileConverter.CHUNK_SIZE) if not chunk: break hasher.update(chunk) - + computed = hasher.hexdigest() - if computed.lower() != expected_checksum.lower(): - raise IOError( - f"Checksum mismatch: expected {expected_checksum}, " - f"got {computed}" - ) - - return True + return computed.lower() == expected_checksum.lower()
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
databusclient/extensions/file_converter.py (3)
102-105:IOErroris semantically incorrect for a checksum mismatch; considerValueError.
IOError(aliased toOSErrorin Python 3) conventionally signals operating-system-level I/O failures (file not found, disk full, permission denied). A checksum mismatch is a data-integrity error —ValueErroror a customChecksumMismatchErrorwould let callers distinguish between a genuine I/O failure and bad data without catching allOSErrors.♻️ Proposed change
- raise IOError( + raise ValueError( f"Checksum mismatch: expected {expected_checksum}, " f"got {computed}" )Also update the docstring:
- Raises: - IOError: If checksum validation fails + Raises: + ValueError: If checksum validation fails🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@databusclient/extensions/file_converter.py` around lines 102 - 105, Replace the semantically incorrect IOError raised for checksum mismatches with a more appropriate exception: change the raise in file_converter.py (the checksum-checking block that currently raises IOError("Checksum mismatch: expected..., got ...")) to raise ValueError with the same message, or define and raise a custom ChecksumMismatchError class and use that instead; also update the surrounding function/class docstring (the docstring for the checksum verification routine in file_converter.py) to document the new exception type so callers know to catch ValueError or ChecksumMismatchError.
21-21:validate_checksumparameter name should becompute_checksum.The parameter only computes and returns the digest — it performs no comparison. The docstring correctly describes the behavior as "compute", but the parameter name implies validation. The past review had proposed this rename; it wasn't carried through.
♻️ Proposed rename
def decompress_gzip_stream( input_stream: BinaryIO, output_stream: BinaryIO, - validate_checksum: bool = False, + compute_checksum: bool = False, ) -> Optional[str]: """Decompress gzip stream with optional checksum computation. ... validate_checksum: Whether to compute a SHA-256 checksum of + compute_checksum: Whether to compute a SHA-256 checksum of the decompressed output. ... """ - hasher = hashlib.sha256() if validate_checksum else None + hasher = hashlib.sha256() if compute_checksum else None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@databusclient/extensions/file_converter.py` at line 21, Rename the parameter validate_checksum to compute_checksum throughout the file to match behaviour described in the docstring: update the function/method signature(s) that currently declare validate_checksum (and any default value False) to compute_checksum: bool = False, update all internal variable references and any return/tuple keys or comments that use validate_checksum to compute_checksum, and update any call sites inside databusclient/extensions/file_converter.py (and its unit tests if present) so callers use the new name; ensure type hints, docstring example/parameter list, and any logging/messages reflect the new name.
12-107: No tests provided for the new module.The PR adds a non-trivial streaming pipeline but no unit tests. At minimum, these cases should be covered:
- Round-trip: compress → decompress restores original bytes.
validate_checksum_streampasses on a correct hash and raises on a bad hash.decompress_gzip_streamwithvalidate_checksum=Truereturns the correct hex digest.- Non-seekable stream handling for
validate_checksum_stream.Would you like me to generate a
tests/test_file_converter.pyskeleton covering the above cases, or open a follow-up issue to track this?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@databusclient/extensions/file_converter.py` around lines 12 - 107, Add unit tests for the FileConverter class to cover the streaming pipeline: create tests/test_file_converter.py and include (1) a round-trip test that writes random bytes, uses FileConverter.compress_gzip_stream to compress into a buffer and then FileConverter.decompress_gzip_stream to decompress and assert original bytes are restored; (2) tests for validate_checksum_stream that assert True on a correct SHA-256 and that an IOError is raised on a bad hash; (3) a test that calls FileConverter.decompress_gzip_stream with validate_checksum=True and asserts the returned hex digest equals the SHA-256 of the decompressed bytes; and (4) a test for non-seekable input to validate_checksum_stream using a custom non-seekable wrapper (or io.BufferedReader over a pipe-like object) to ensure validation still works without calling seek; use BinaryIO-compatible buffers (io.BytesIO) and reference FileConverter.compress_gzip_stream, FileConverter.decompress_gzip_stream, and FileConverter.validate_checksum_stream in assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@databusclient/extensions/file_converter.py`:
- Around line 91-107: validate_checksum_stream currently reads to EOF and
returns without resetting the stream, which breaks the validate-then-decompress
pattern used by decompress_gzip_stream; change validate_checksum_stream to seek
input_stream back to 0 before returning and update its docstring to state that
input_stream must be seekable (and that the function will reset the stream
position), so callers like FileConverter.decompress_gzip_stream can safely read
from the start after validation.
- Around line 12-13: The package API currently doesn't export FileConverter from
the extensions package, forcing consumers to import
databusclient.extensions.file_converter.FileConverter directly; update
extensions/__init__.py to import and export FileConverter so it can be accessed
as databusclient.extensions.FileConverter (e.g., add "from .file_converter
import FileConverter" and include it in __all__), or document that only direct
module imports are supported—modify the __init__ export to reference the
FileConverter class name to restore the expected package-level import.
---
Nitpick comments:
In `@databusclient/extensions/file_converter.py`:
- Around line 102-105: Replace the semantically incorrect IOError raised for
checksum mismatches with a more appropriate exception: change the raise in
file_converter.py (the checksum-checking block that currently raises
IOError("Checksum mismatch: expected..., got ...")) to raise ValueError with the
same message, or define and raise a custom ChecksumMismatchError class and use
that instead; also update the surrounding function/class docstring (the
docstring for the checksum verification routine in file_converter.py) to
document the new exception type so callers know to catch ValueError or
ChecksumMismatchError.
- Line 21: Rename the parameter validate_checksum to compute_checksum throughout
the file to match behaviour described in the docstring: update the
function/method signature(s) that currently declare validate_checksum (and any
default value False) to compute_checksum: bool = False, update all internal
variable references and any return/tuple keys or comments that use
validate_checksum to compute_checksum, and update any call sites inside
databusclient/extensions/file_converter.py (and its unit tests if present) so
callers use the new name; ensure type hints, docstring example/parameter list,
and any logging/messages reflect the new name.
- Around line 12-107: Add unit tests for the FileConverter class to cover the
streaming pipeline: create tests/test_file_converter.py and include (1) a
round-trip test that writes random bytes, uses
FileConverter.compress_gzip_stream to compress into a buffer and then
FileConverter.decompress_gzip_stream to decompress and assert original bytes are
restored; (2) tests for validate_checksum_stream that assert True on a correct
SHA-256 and that an IOError is raised on a bad hash; (3) a test that calls
FileConverter.decompress_gzip_stream with validate_checksum=True and asserts the
returned hex digest equals the SHA-256 of the decompressed bytes; and (4) a test
for non-seekable input to validate_checksum_stream using a custom non-seekable
wrapper (or io.BufferedReader over a pipe-like object) to ensure validation
still works without calling seek; use BinaryIO-compatible buffers (io.BytesIO)
and reference FileConverter.compress_gzip_stream,
FileConverter.decompress_gzip_stream, and FileConverter.validate_checksum_stream
in assertions.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@databusclient/extensions/file_converter.py`:
- Around line 102-109: The input_stream is only seeked back on success so when
the checksum check raises an IOError the stream stays at EOF; move the
input_stream.seek(0) to before the comparison of computed vs expected_checksum
(i.e., seek the input_stream right after computing computed = hasher.hexdigest()
and before comparing computed.lower() != expected_checksum.lower()) so the
stream is rewound regardless of an exception, and update the function docstring
to drop the word "successful" (so it documents that the stream will be rewound
even when a checksum mismatch occurs).
---
Duplicate comments:
In `@databusclient/extensions/file_converter.py`:
- Line 21: The parameter name validate_checksum is misleading because the
function only computes and returns a digest; rename the parameter to
compute_checksum across the FileConverter API in
databusclient/extensions/file_converter.py (update the function signature where
validate_checksum appears, any internal uses/branches that check it, and all
docstrings that reference "validate_checksum") and update all callers/tests to
use compute_checksum so the name matches behavior and the docstrings that
already say "compute" remain consistent.
Contributor
Author
|
Greetings @Integer-Ctrl,Could you please review this pr whenever available . I might be mistaken in some of the changes, so your guidance and feedback would be very helpful. Thank you! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR adds a new
file_converter.pyextension module to address Issue #54. The module provides a streaming pipeline for file format conversion with support for gzip decompression and checksum validation.Changes
databusclient/extensions/file_converter.pymoduleFileConverterclass with streaming supportdecompress_gzip_stream()method with optional checksum validationcompress_gzip_stream()method for gzip compressionvalidate_checksum_stream()method for SHA256 checksum validationRelated Issues
Fixes #54
Type of change
Summary by CodeRabbit