Skip exhibit pages with no valid files instead of crashing - #1098
Skip exhibit pages with no valid files instead of crashing#1098rajeswari1301 wants to merge 5 commits into
Conversation
9a98bf5 to
0f68097
Compare
415b3d5 to
9fa8ea6
Compare
9fa8ea6 to
1cf2bdc
Compare
nonprofittechy
left a comment
There was a problem hiding this comment.
This looks good - wait to merge until you do your tests.
Main question is what happens if a None is able to escape--is it safe all the way up? I would try some adversarial bundles to test this.
Also: I see why this happened - we probably patched ALDocument but not ALExhibitList. Are there other descendants of ALDocument that we need to repair/guard in the same way? It would make sense to do those at the same time.
There was a problem hiding this comment.
Pull request overview
This PR prevents exhibit PDF generation from crashing when an exhibit ends up with zero loadable pages by filtering invalid pages, skipping empty exhibits, and propagating “no output” (None) up to the top-level exhibit-document assembly.
Changes:
ALExhibit.as_pdf(): filters out missing/broken pages before concatenation; returnsNonewhen no valid pages remain.ALExhibitList.as_pdf(): skips exhibits that returnNone; returnsNonewhen all exhibits are empty.ALExhibitDocument.as_pdf(): handles an empty exhibit list PDF result to avoid crashing when combining with the table of contents; return types updated toOptional[DAFile].
Suppressed comments (3)
docassemble/AssemblyLine/al_document.py:3118
ALExhibitList.as_pdf()now returnsOptional[DAFile], but the docstring doesn't mention that it can returnNonewhen there are no valid exhibits to include. Please document theNonecase explicitly.
"""
Compiles all exhibits in the list into a single PDF.
docassemble/AssemblyLine/al_document.py:3476
as_docx()delegates toas_pdf(), which can now returnNone. The docstring should mention theNonepossibility so callers don't assume aDAFileis always returned.
"""
Despite the name, renders the document as a PDF. Provided for signature compatibility.
docassemble/AssemblyLine/al_document.py:3415
ALExhibitDocument.as_pdf()now returnsOptional[DAFile], but the docstring doesn't mention that it can returnNone(e.g., when there are no exhibits and no table of contents to return). Please document theNonecase.
"""
Render the document as a PDF.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Copilot's findings all also look worth addressing prior to merging, esp. updating the docstring. ideally we could also scan for now-unsafe use in our other repos |
Good question, no not fully yet. Found a few more spots in ALDocumentBundle that assume as_pdf/as_docx never returns None, as_zip, |
Extends the same fix to the rest of ALDocumentBundle: as_pdf, as_zip, as_pdf_list, as_docx_list, as_docx, get_cacheable_documents, download_html, send_email, and str, all of which had the same gap as the exhibit classes. Also added a safety check to size_in_bytes, so a single bad page won't break the whole size calculation anymore.
Fixes a bug where a user's document download kept failing. when every page for one file failed to load, it tried to build a PDF out of nothing and crashed the whole download instead of just skipping that file.
Three small fixes at each level where this could happen:
None of these results get cached, so if something comes back on a retry (like it did in our actual case, which was a one-time blip), it's not stuck as skipped for the rest of the session.
Still need to test this against something close to the actual broken case before merging.