fix: handle XLSX files with backslash zip entry names - #104
Conversation
Some exporters write XLSX archives with OS path separators, so openpyxl cannot find parts like xl/sharedStrings.xml and raises a raw KeyError. Normalize the entry names before parsing, and surface a readable error when a spreadsheet still cannot be read. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 53 minutes Limit details: You’ve used all 1 included review currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds XLSX archive normalization for backslash-separated paths while preserving valid archive content and metadata. Spreadsheet processing now validates supported file types and normalizes XLSX content before parsing. Tests cover normalization, valid content preservation, spreadsheet parsing, and readable validation errors for corrupt files. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
Confidence Score: 4/5The PR appears safe to merge, with a non-blocking need to bound or avoid full expansion of untrusted XLSX archives during normalization. The functional fix and error handling are well covered, while the newly added normalization path can spend memory and CPU expanding every member of a crafted archive. Files Needing Attention: transaction_parser/transaction_parser/utils/file_processor.py
|
| Filename | Overview |
|---|---|
| transaction_parser/transaction_parser/utils/file_processor.py | Adds backslash-entry normalization and readable parsing errors, but normalization performs unbounded full-archive decompression and recompression. |
| transaction_parser/tests/test_file_processor.py | Adds focused coverage for normalization recovery, the valid-file fast path, end-to-end parsing, and corrupt-file errors. |
Reviews (1): Last reviewed commit: "fix: handle XLSX files with backslash zi..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| for info in source.infolist(): | ||
| normalized_info = copy.copy(info) | ||
| normalized_info.filename = info.filename.replace("\\", "/") | ||
| target.writestr(normalized_info, source.read(info)) |
There was a problem hiding this comment.
An XLSX with one backslash-named entry activates normalization for the entire archive, and source.read(info) fully expands every member before recompressing it into another in-memory buffer. Large or highly compressed unused members therefore consume excessive worker memory and CPU; bound archive expansion or avoid rewriting unrelated members.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
transaction_parser/transaction_parser/utils/file_processor.py (1)
32-34: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject duplicate normalized XLSX entry names.
If two source entries normalize to the same name, reject the archive before
target.writestr; duplicate entries can cause readers to select different content based on entry order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 47989999-387f-4763-9ee8-3cd3d8321c9f
📒 Files selected for processing (1)
transaction_parser/transaction_parser/utils/file_processor.py
Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review.
| elif doc.file_type == "XLSX": | ||
| rows = read_xlsx_file_from_attached_file( | ||
| fcontent=normalize_xlsx_content(file_content) | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
target="transaction_parser/transaction_parser/utils/file_processor.py"
printf '%s\n' '--- target files ---'
git ls-files | rg '(^|/)(file_processor\.py|.*xlsx.*|.*excel.*|.*file.*processor.*)$' || true
printf '%s\n' '--- target outline ---'
if command -v ast-grep >/dev/null 2>&1; then
ast-grep outline "$target" || true
fi
printf '%s\n' '--- target size ---'
wc -l "$target"
printf '%s\n' '--- target imports and dispatch context ---'
sed -n '1,150p' "$target"
printf '%s\n' '--- relevant symbols and exception patterns ---'
rg -n -C 4 \
'normalize_xlsx_content|read_xlsx_file_from_attached_file|BadZipFile|traceback|throw|ValidationError|frappe\.throw|except .*Exception|parse.*xlsx|XLSX' \
transaction_parser || trueRepository: resilient-tech/transaction-parser
Length of output: 50391
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- transaction parser exception boundary ---'
sed -n '1,135p' transaction_parser/transaction_parser/__init__.py
printf '%s\n' '--- file processor tests ---'
wc -l transaction_parser/tests/test_file_processor.py
sed -n '1,260p' transaction_parser/tests/test_file_processor.py
printf '%s\n' '--- focused error-handling examples ---'
rg -n -C 8 \
'log_error\(|get_traceback\(|Spreadsheet|spreadsheet|Excel|XLSX|File Reading|Reading Failed|parse.*failed|Failed to.*file' \
transaction_parser/transaction_parser transaction_parser/tests \
-g '*.py' | head -n 260
printf '%s\n' '--- relevant history summary ---'
git log --oneline --all -- transaction_parser/transaction_parser/utils/file_processor.py | head -n 20
printf '%s\n' '--- standalone exception and call-path verifier ---'
python3 - <<'PY'
import ast
import io
import zipfile
from pathlib import Path
path = Path("transaction_parser/transaction_parser/utils/file_processor.py")
tree = ast.parse(path.read_text())
normalize = next(
node for node in tree.body
if isinstance(node, ast.FunctionDef) and node.name == "normalize_xlsx_content"
)
processor = next(
node for node in tree.body
if isinstance(node, ast.ClassDef) and node.name == "FileProcessor"
)
spreadsheet = next(
node for node in processor.body
if isinstance(node, ast.FunctionDef) and node.name == "process_spreadsheet"
)
print("normalize_has_try:", any(isinstance(n, ast.Try) for n in ast.walk(normalize)))
print("process_spreadsheet_has_try:", any(isinstance(n, ast.Try) for n in ast.walk(spreadsheet)))
print("xlsx_calls_normalizer:", any(
isinstance(n, ast.Call)
and isinstance(n.func, ast.Name)
and n.func.id == "normalize_xlsx_content"
for n in ast.walk(spreadsheet)
))
print("xlsx_reader_is_inside_try:", any(
isinstance(parent, ast.Try)
for parent in ast.walk(spreadsheet)
if isinstance(parent, ast.Try)
and any(
isinstance(n, ast.Call)
and isinstance(n.func, ast.Name)
and n.func.id == "read_xlsx_file_from_attached_file"
for n in ast.walk(parent)
)
))
for payload in (b"", b"not an xlsx archive", b"PK\x03\x04"):
try:
with zipfile.ZipFile(io.BytesIO(payload)):
pass
except Exception as exc:
print(type(exc).__name__, repr(str(exc)))
PYRepository: resilient-tech/transaction-parser
Length of output: 26199
🏁 Script executed:
#!/bin/bash
set -eu
file="transaction_parser/transaction_parser/utils/file_processor.py"
printf '%s\n' '--- current commit ---'
git rev-parse --short HEAD
printf '%s\n' '--- relevant commit summaries ---'
git show --stat --oneline be992f1
git show --stat --oneline 8bd09ac
printf '%s\n' '--- file before streamline commit ---'
git show be992f1^:"$file" | sed -n '65,125p'
printf '%s\n' '--- streamline commit diff for process_spreadsheet ---'
git show --format=fuller --no-ext-diff be992f1 -- "$file" | sed -n '1,220p'
printf '%s\n' '--- backslash-fix commit diff ---'
git show --format=fuller --no-ext-diff 8bd09ac -- "$file" transaction_parser/tests/test_file_processor.py | sed -n '1,260p'Repository: resilient-tech/transaction-parser
Length of output: 12124
Restore the spreadsheet error boundary.
Wrap normalization and parsing in the established try/except pattern. Re-raise frappe.ValidationError; otherwise log frappe.get_traceback() and raise Unable to read {doc.file_name}. The file may be corrupted. The current code lets zipfile.BadZipFile and parser exceptions escape, violating the existing corrupt-file test.
1 participant
Problem
Parsing a customer XLSX failed with a raw traceback:
Root cause
The archive's zip entry names use OS path separators instead of the forward slashes OOXML requires:
[Content_Types].xmlsits at the archive root (no separator), so openpyxl reads it and learns the part is at/xl/sharedStrings.xml. Every subsequent part lookup is a literalzipfile.getinfo()call with no path normalization, so it misses —sharedStrings.xmlis just the first one attempted. Excel tolerates these files; openpyxl does not.Fix
normalize_xlsx_content()rewrites the archive with forward-slash entry names before parsing. Well-formed files are returned unchanged (only the central directory is read, so there is no cost for the normal path).process_spreadsheetis wrapped so an unreadable spreadsheet raisesUnable to read {filename}. The file may be corrupted.instead of a raw traceback, with the real traceback kept in the error log.Verification
My-Store_150218_Plaud_Sharge_OT, rangeA1:L14, all product rows read correctly.transaction_parser/tests/test_file_processor.pycover recovery, the untouched-valid-file path, end-to-endprocess_spreadsheet, and the readable error. All pass.🤖 Generated with Claude Code