Task27956 prevent repeat api calls

[ZachSchwartz]

https://redmine.pangaeatech.com/issues/27956

[Copilot]

🟡 Changes recommended

Unresolved deduplication, cache validation, key-collision, and malformed-response issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR adds geocoding response caching and duplicate-request suppression, with provider refactoring and expanded tests.

Changes:

• Adds SQLite cache modes and CLI integration.
• Refactors Google and Census request handling.
• Expands cache, provider, and CLI tests; ignores cache artifacts.

File summaries

File	Reviewed change	Final review note
test/google_test.py	Google caching and raw-response tests	—
test/geocoder_test.py	CLI cache tests	—
test/census_test.py	Census caching tests	—
test/cache_test.py	Cache behavior tests	—
test/api_test.py	Provider API tests	—
src/google.py	Google request and response handling	Moderate, 1 vote: Empty OK responses can be cached and fail on later runs.
src/geocoder.py	Cache-related CLI integration	—
src/census.py	Census caching and parsing	Critical, 3 votes: Delimiter-based cache keys can collide for addresses containing |.
src/cache.py	SQLite cache implementation	Moderate, 1 vote: Connection errors can escape CLI handling. Moderate, 2 votes: Existing tables are not validated against the required schema.
src/api.py	Shared caching and deduplication logic	Moderate, 1 vote: Deduplication is not shared across worksheet calls when --noCache is used.
.gitignore	Ignores SQLite cache artifacts	—

Review details

Suppressed comments (3)

src/api.py:227

• The deduplication map is local to one Provider.geocode call, but process_workbook calls the provider once per worksheet. Therefore identical queries on two sheets are still fetched twice when --noCache is used, despite the CLI help promising repeated addresses are collapsed within the run. Share an in-memory run-level cache/deduplication map across sheet calls, while keeping it non-persistent for --noCache.

        keys = [normalize_query(self.cache_key(record)) for record in records]
        responses = self.cache.lookup(self.name, keys)

src/cache.py:88

• sqlite3.connect(path) is outside the guarded block, so an invalid cache location such as a directory or a path whose parent is missing raises sqlite3.OperationalError directly. main only converts ValueError from Cache into SystemExit, so these user input errors produce an uncaught traceback instead of the intended CLI error; move the connection open into the error-wrapping path and close it on failure.

        connection = sqlite3.connect(path)

src/google.py:94

• OK responses are yielded without checking that a result exists. If Google returns an invalid empty OK envelope, Provider.geocode stores it before parse() indexes results[0], and every later run fails on the same bad cached response instead of retrying. Reject an empty results list before yielding so malformed responses are never cached.

            status = payload.get("status", "UNKNOWN")
            if status not in ("OK", "ZERO_RESULTS"):
                raise ValueError(f"Google geocoding failed with status {status!r}: {payload.get('error_message', '')}".strip())
            yield record, payload

• Files reviewed: 10/11 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[mwaddell]

Address comments and fix merge conflicts

[mwaddell]

considering the table structure, i'm worried that this file could grow indefinitely. Since the key will change whenever the version/benchmark changes for an API that would effectively "obsolete" a whole group of the entries but there would be no way to differentiate them from others.

I'm thinking that it would be useful to split up the key into an address portion (what we're sending to the API) and a version portion (the metadata about how we're calling the API).

It would probably also be more efficient to store each api in its own table and just have a single key column as the primary key. So, the API and version stuff should be specified in the init (since it won't change), then the init would make sure the corresponding api table exists and add it if it doesn't.

The table itself would contain a query primary key and a version tag. When calling "get", you would pass just the query portion. The API would do a "SELECT query, response FROM {apitable} WHERE version = {singleversion} and query in ({placeholders})" - that way it won't pull those where the version is wrong, but when you go to store them later it will overwrite any with the wrong version.

[mwaddell]

This means that cache_key would actually be identical across all APIs (so api.py can calculate it in one place) and each API implementation just needs to report it's "version id" (which would contain anything that doesn't come from the address that would change as the API itself changes)

[mwaddell]

This is never used except by api.py so it should probably live in there.

[mwaddell]

We'll need to note in the documentation that --cache and --cacheRead are ignored when --noCache is used since we don't note that this is an error