Fetch billtext index - #625
Conversation
…zip files. This helps separate BILLSTATUS from BILLTEXT archives.
…ontents using the assert_files utility.
…facing CLI behavior. Strengthened test assertions on folder contents by checking the expected file structure exactly. Moved core business logic into shared utilities for reuse in fetch_bill_text_archives.
…ng further discussion.
# Conflicts: # tests/test_fetch_bill_archives_extract.py
| def assert_files(folder: Path, files: set[str] | list[str]) -> None: | ||
| """Assert the folder contains exactly the given filenames.""" | ||
| __tracebackhide__ = True | ||
| actual = {path.name for path in folder.iterdir()} | ||
| expected = set(files) | ||
| if actual != expected: | ||
| extra = actual - expected | ||
| missing = expected - actual | ||
| raise AssertionError( | ||
| "\n".join( | ||
| filter( | ||
| None, | ||
| [ | ||
| f"Unexpected file contents in folder {folder}:", | ||
| f"expected: {expected}", | ||
| f"actual: {actual}", | ||
| f"extra: {extra}" if extra else None, | ||
| f"missing: {missing}" if missing else None, | ||
| ], | ||
| ) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
This new utility is key for testing - it checks the full contents of a folder and outputs intuitive error text if expectations mismatch reality. Fetch_bill_archives unit tests make liberal use of this utility now.
Feel free to add a recursive option if needed, but I found it most intuitive to check one folder at a time instead.
| def resolve_bill_types(bill_types: list[str] | None = None) -> list[str]: | ||
| "Allow for 'all' keyword to include all bill types. Default to all types if no type are specified." | ||
| return list(BILL_TYPES) if bill_types is None or "all" in bill_types else bill_types |
There was a problem hiding this comment.
this is centrally related to adding a --type all option. It inputs raw bill types as the user types them and outputs actual bill types. It is a shared utility because I plan to reuse it for fetch_bill_text_archives in a follow-up PR.
| def download_archives( | ||
| client: httpx.Client, | ||
| urls: Iterable[str], | ||
| destination: Path, | ||
| *, | ||
| url_to_path: Callable[[str, int], Path], | ||
| skip_existing: bool = True, | ||
| ) -> list[Path]: |
There was a problem hiding this comment.
This will unify reporting and error handling between the two bulk data CLI commands
| @respx.mock | ||
| def test_single_congress_and_bill_type_download(self, tmp_path): | ||
| mock_http_requests(content=EMPTY_ZIP_BYTES) | ||
| rc = fetch_index(["--congress", "118", "--type", "hr", "--billstatus-dir", str(tmp_path)]) | ||
| assert rc == 0 | ||
| assert_files(tmp_path, {"BILLSTATUS-118-hr.zip"}) |
There was a problem hiding this comment.
The new model for unit tests:
Perform mocking. Ideally in a line or two using utilities
Invoke a function. In this case, a CLI command.
Check user-facing results. In this case, files created.
Aside from greatly simplifying the test, there is a behavioral change that required the rewrite: archive files now begin with BILLSTATUS to disambiguate them from BILLTEXT archives.
It is debatable whether we should verify zip files downloaded in any level of detail. They are an intermediate step, end their structure is not important. The important details to check are the extracted archive contents, the bill index file contents, cache behavior, and error handling.
| def extract_archive( | ||
| archive_path: Path | str, | ||
| *, | ||
| out_dir: Path | str, | ||
| files: str | re.Pattern[str] = "*", | ||
| overwrite_existing: bool = False, | ||
| file_handler: Callable[[str, int, zipfile.ZipFile], str | Path | None] = lambda filename, index, zf: filename, | ||
| file_content_handler: Callable[ | ||
| [bytes, str, int, zipfile.ZipFile], bytes | None | ||
| ] = lambda data, filename, index, zf: data, | ||
| ) -> tuple[int, ExtractArchiveDetails]: | ||
| """Extract matching ZIP members into ``out_dir``. | ||
|
|
||
| Args: | ||
| archive_path: ZIP file to read. | ||
| out_dir: Destination root for extracted files. | ||
| files: Glob string or compiled regex selecting archive members. | ||
| overwrite_existing: When false, skip members whose destination already exists. | ||
| file_handler: Maps archive member path to a path relative to ``out_dir`` to allow the file structure to be reordered. | ||
| Return ``None`` to skip the member (the member is not opened). | ||
| file_content_handler: Transforms or analyzes file contents before writing. | ||
|
|
||
| Returns: | ||
| ``(count, details)`` where ``count`` is how many files were written and | ||
| ``details`` has ``files_extracted``, ``files_skipped``, and ``errors``. | ||
| """ |
There was a problem hiding this comment.
key change: make a shared utility for iterating a zip file and extracting it into a target folder. Add handles for intercepting the zip's file names and/or file contents.
This streams archive files directly from the zip, skipping an unarchive step.
The filename handler can optionally return a destination path to customize the folder structure of extracted archives, including returning None to skip a file.
| files_skipped.append(dest) | ||
| continue | ||
| dest.parent.mkdir(parents=True, exist_ok=True) | ||
| _ensure_within_destination(out_dir, dest) |
There was a problem hiding this comment.
In a previous iteration, I had an additional check for maliciously formed zip files that use symbolic links to navigate outside of the parent. This added nontrivial complexity though.
Then I took a step back and considered that this is a developer-facing utility to gather data, not a customer-facing product, and I believe the level of detail of its logic is already more than reasonable as-is.
willhea
left a comment
willhea
left a comment
There was a problem hiding this comment.
@RolfHendriks, Thanks for putting this together and for the detailed comments explaining the refactor and testing approach. I spent some more time tracing how the three fetch tools are actually used, and that changed how I think we should approach this.
I’m requesting changes primarily because I think this PR now spans several independently reviewable decisions that we should separate.
First, please split #310 into a narrow PR. #310 is specifically about fetch_bill_text_archives.py: it should use the canonical eight bill types, reject unknown values rather than fail open, preserve non-fatal 404 behavior for genuinely absent valid archives, and allow explicit type narrowing. If we support all, it should expand to the concrete types before URL construction.
The resolve_bill_types() helper here gets us partway there, but currently expands all without validating unknown values. I’d like to get the #310 fix merged independently because it is small and directly unblocks correcting the research population.
Second, I think the shared HTTP/ZIP work is useful and worth keeping, but I’d like that reviewed as its own slice. I agree with simplifying tests and moving lower-level behavior to the shared layer. The important part is that consequential protections move rather than disappear. In particular, the owning layer should retain executable coverage that:
incomplete downloads never become valid cached archives;
failed downloads remain retryable and stale error markers clear after success;
one failed archive does not abort the rest of a batch;
archive members cannot escape the destination;
archive expansion remains bounded by both total bytes and member count.
Third, I’d set aside the rest of the fetch_bill_archives.py extract/index refactor until we settle the acquisition architecture. Digging into this showed the generated bills.csv currently has no in-repo reader, and the extracted BILLSTATUS XML has no in-repo reader either. I'm unclear on the purpose of intent of these files and would like to clarify that before further work.
More broadly, I think we need three capabilities — targeted named-bill acquisition, rule-defined bulk-text acquisition, and BILLSTATUS discovery/index support — but I’m no longer convinced those should be three separately exposed CLIs.
I wrote up the repository findings and a proposed target architecture separately (sent via Slack) so we can discuss that question explicitly rather than have this refactor decide it incidentally.
My suggested sequence is:
#310 as a self-contained fix;
shared http.py / zip.py plus the critical invariants above;
agree on the bill-acquisition architecture;
then move/retire the BILLSTATUS extraction/index pieces and consolidate the CLI/docs as appropriate.
This isn’t a rejection of the refactoring direction, I think a meaningful part of this work should survive. I’d just like to separate the pieces so we can preserve behavior where required and avoid investing in code that the architecture decision may change anyways.
2 participants
Refactors fetch_bill_archives (billstatus bulk metadata) to move shared logic to common utilities and focus unit tests on user-facing behavior instead of implementation details.
Backstory
I have a change to combine results from bulk bill text data (fetch_bill_text_archives) into results from bulk bill metadata (fetch_bill_archives). Most notably, to add bill appropriation metadata the bill index so that we can use it to search for noteworthy appropriations. However, I am blocked by the extent of unit tests and rate of change of the codebase making a merge infeasible.
I took a step back and refactored the simpler fetch_bill_archives as a practice round and to set up for future success with resolving the intended fetch_bill_text_archives changes. The idea for the next iteration of fetch_bill_text_archives will be to reuse the shared utilities developed here and reshape fetch_bill_text_archives and its tests similarly.
The unit tests as they are written were an effort multiplier for implementing these changes. Some unit tests like validating error handling added value, but many tested implementation details instead of user-facing behavior and were difficult to read and understand. See comments on unit test changes for details. We may want to discuss what is the appropriate level of unit tests for an internal command-line utility used to assemble test data.
What does this change?
Key changes:
Behavioral Changes
Added --type all option for fetch_bill_archives
Removed checks and tests for very large zip file contents. If needed, we can add them to the new extract_archive utility, which is a more appropriate place because it will ensure behavioral consistency across bulk data API commands. Related: the new algorithm skips the intermediate unzip phase and instead streamins directly from zip file contents to the output folder and/or bill index. This should greatly reduce disk usage.
Changed billstatus archive name from {congress}-{type}.zip to BILLSTATUS-{congress}-{type}.zip. I was not aware that adding the BILLSTATUS prefix was a behavioral change when I implemented it. However, now that we fetch bulk data from two sources, adding a BILLSTATUS prefix for bill metadata and BILLTEXT prefix for bill text bulk data makes sense for disambiguation.
Changed fetch_bill_archives command-line signature to be consistent with fetch-bill-text-archives (from-congress, to-congress, output-dir, zip-dir, help, overwrite-existing)
Tightened assertions for fetch_bill_archives unit tests by checking exact folder structure. Focused fetch_bill_archives unit tests at the level of responsibility of the CLI command - inputting command line options and checking resulting file contents and API calls made or not made.
Structural Changes
Moved most fetch_bill_archives business logic into http and zip utility methods. Fetch_bill_archives now focuses on handling and using command-line arguments and on logic specific to bill metadata files.
Added a shared utils.py file for unit testing. It implements folder content checks, http mocking, and zip file mocking. This sets us up for future success with refactoring fetch_bill_text_archives so that it can integrate with the bill index.
How to test
Run ./fetch_bill_archives.py --help followed by a full clean extraction of all files including a bill index and custom folders:
Verify that zip files, extracted bills, and a bill index are created.
Then run the command again. The download is instantaneous, skipping all files. The extraction phase is considerably faster because it does not write files. However, the extraction phase takes non-negligible time because it needs to iterate many large archives to read their files.
Also, pytest passes all tests.
Checklist
Closes #...)AI assistance
Used heavily supervised Cursor Grok 4.5 with small iterations for maximum oversight.