From c4ee6c28288685d45544d222d1014763ca19bf6f Mon Sep 17 00:00:00 2001 From: Hamish Downer Date: Fri, 16 Oct 2026 15:06:27 +0100 Subject: [PATCH 1/6] docs: add plan for reducing Google Sheets API reads Co-Authored-By: Claude Opus 5.5 --- docs/agent/974-reduce-sheets-reads.md | 295 ++++++++++++++++++++++++++ 1 file changed, 295 insertions(+) create mode 100644 docs/agent/974-reduce-sheets-reads.md diff --git a/docs/agent/974-reduce-sheets-reads.md b/docs/agent/974-reduce-sheets-reads.md new file mode 100644 index 0000000..361948f --- /dev/null +++ b/docs/agent/974-reduce-sheets-reads.md @@ -0,0 +1,295 @@ +# 974: Reduce Google Sheets API reads + +## Aim + +A service using this library should be less likely to hit the Google Sheets +rate limit of **60 read requests per minute per user** (there is a separate +60 per minute limit for writes). The "user" is the service account, so every +check and every selection the service runs shares one budget. + +Drive API calls have their own, much larger quota and are not the concern here. + +Call counts below are from reading gspread 6.2.1 (the version in `uv.lock`). + +## Where the reads go today + +| Operation | Sheets reads | Why | +| ------------------------------------------- | ------------ | -------------------------------------------------------------------------------------------------------- | +| Access check via `open_gsheet()` + tab list | 2 | `open_by_key()` fetches metadata (1), `worksheets()` fetches it again (1) | +| `GSheetDataSource` open by URL | 1 | `open_gsheet()` → `open_by_key()` | +| `GSheetDataSource` open by title | 2 | `client.open()` builds a `Spreadsheet` (1), then `open_gsheet(found.id)` builds another (1) | +| `read_feature_data` | 4 | `_tab_exists` → `worksheets()`, `worksheet(name)` (metadata again), `row_values(1)`, `get_all_records()` | +| `read_people_data` | 4 | same pattern | +| `read_already_selected_data` | 3 | `_tab_exists` → `worksheets()`, `worksheet(name)`, `get()` | +| `write_selected` | 1 | `_get_tab_titles()` → `worksheets()` | +| `write_remaining` | 0 | reuses the suffix chosen in `write_selected` | +| `highlight_dupes` | 1 | `_get_tab()` → `worksheets()` | +| `delete_old_output_tabs` | 1 | `worksheets()` | + +A full selection by URL with an already-selected tab costs about **14 Sheets +reads** (1 open, 11 reading the tabs, 2 writing output). Most of them re-fetch the same spreadsheet metadata. + +Relevant gspread facts: + +- `Spreadsheet.__init__` calls `fetch_sheet_metadata()`, keeps only + `properties` and discards the tab list. +- `Spreadsheet.worksheets()` and `Spreadsheet.worksheet(title)` each call + `fetch_sheet_metadata()` again. +- `Worksheet.row_values(n)` and `Worksheet.get_all_records()` each make one + `values.get` request. +- `Worksheet(spreadsheet, properties, spreadsheet_id, client)` makes no + request. `Spreadsheet.worksheet()` builds them this way from the metadata. + +## Option A: one Sheets read per access check (`inspect_gsheet`) + +A successful check today costs **2 Sheets reads plus 1 Drive read**: + +1. Drive `files.get` (mimetype, `canEdit`). +2. Sheets `spreadsheets.get`, inside `client.open_by_key()`. +3. Sheets `spreadsheets.get` again, in `spreadsheet.worksheets()`, to get the + tab list back. + +Reads 2 and 3 fetch the same thing. + +### Proposal + +Add to `sortition_algorithms/gsheet.py`: + +```python +@dataclass(frozen=True) +class GSheetSummary: + """What the service account can see of a Google Sheet, without opening it for use.""" + + file_id: str + title: str + url: str + mimetype: str + can_edit: bool + worksheet_titles: tuple[str, ...] + # when the Sheets metadata was fetched, so a caller that keeps the summary + # can decide when it is too old to trust + fetched_at: datetime = field(compare=False) + service_account_email: str = "" + + +def inspect_gsheet(client: gspread.client.Client, url_or_id: str) -> GSheetSummary: + """ + Report what the service account can do with a Google Sheet, using one Drive + and one Sheets request. + """ +``` + +`fetched_at` records when the summary was taken. A caller that caches it (for +example, the service showing a "sheet is ready" status) can then decide when to +check again without wrapping the summary in its own timestamped record. + +- Use a timezone-aware UTC `datetime` (`datetime.now(UTC)`). A wall-clock time + is the right choice here, unlike Option B's cache, which uses the monotonic + clock. The summary may be stored, logged or shown to a person, and a + monotonic value means nothing outside the process. +- Set it after the Sheets request returns, so it never claims to be fresher + than the data. +- `field(compare=False)` keeps equality about the sheet's contents: two + summaries of an unchanged sheet taken a minute apart compare equal. +- The library doesn't decide what "stale" means. That depends on the caller. +- Tests: assert `fetched_at` falls between timestamps taken just before and just + after the call, rather than mocking the clock. + +- It shares the Drive call and `_classify_drive_not_found()` with + `open_gsheet()`. Factor the first half of `open_gsheet()` (id extraction, + the Drive request, the 404 classification and the mimetype check) into a + private helper that both call. +- Instead of `open_by_key()`, it calls + `client.http_client.fetch_sheet_metadata(file_id, params={"includeGridData": "false", "fields": "properties.title,sheets.properties.title"})`. + Passing `params` replaces gspread's default of `includeGridData=false`, so + include it explicitly. The `fields` mask keeps the response small for a + sheet with many tabs. +- Build `url` as `SPREADSHEET_DRIVE_URL % file_id` (from `gspread.urls`), + which is what `Spreadsheet.url`, and so `GSheetInfo.url`, returns. Do not use + the API's `spreadsheetUrl`: it ends in `/edit`, so the two functions would + return different URLs for the same sheet. +- The tab list includes hidden tabs, as `worksheets()` does by default. +- It doesn't build a `gspread.Spreadsheet`, because building one costs a + request. Callers that need to read or write (`GSheetDataSource`, the export + adapter) keep using `open_gsheet()`. +- On the failure path the cost is the same as today (Drive 404, then one Sheets + call to classify it). + +### Errors + +The docstring should list the errors precisely rather than saying "the same as +`open_gsheet()`". There is one difference. When Drive succeeds but Sheets +refuses, `open_by_key()` turns a 403 into a bare `PermissionError` and a 404 +into `gspread.SpreadsheetNotFound`. A direct `fetch_sheet_metadata()` raises +`gspread.exceptions.APIError`. This happens for real when the Sheets API is not +enabled on the GCP project (403 `SERVICE_DISABLED`). `APIError` matches what +`open_gsheet()`'s docstring promises for "anything else from Google", so +`inspect_gsheet()` is arguably the more correct of the two. Decide whether +`open_gsheet()` should be brought in line (separate change). + +### Why a new function and not a change to `GSheetInfo` + +Adding `worksheet_titles` to `GSheetInfo` seems more natural. But the tab list +is only available from the metadata request that `Spreadsheet.__init__` makes +and then discards, so it can't be kept without a second request (which saves +nothing) or reaching into gspread internals. A separate function that never +builds a `Spreadsheet` is the clean version. + +### Also needed + +- Export `GSheetSummary` and `inspect_gsheet` from `sortition_algorithms/__init__.py`. +- Document them in `docs/adapters.md` ("Opening a sheet and checking access") + and `docs/api-reference.md`. +- Tests in `tests/test_gsheet.py` alongside the `open_gsheet` ones: success + (title, url, tabs, `can_edit`), bare file id, each Drive failure + classification, not-native mimetype, and that exactly one Sheets request is + made on success. + +**Result: 1 Sheets read per successful check, down from 2.** + +## Option B: fetch the tab list once per open in `GSheetDataSource` + +Removes the repeated `worksheets()` and `worksheet(name)` calls. + +- When `info` opens the spreadsheet, also fetch the metadata once + (`spreadsheet.fetch_sheet_metadata()`) and build a cache of + `Worksheet(spreadsheet, item["properties"], spreadsheet.id, spreadsheet.client)` + keyed by title. That mirrors how `Spreadsheet.worksheet()` builds them, and + constructing a `Worksheet` makes no request. +- `_get_tab`, `_tab_exists` and `_get_tab_titles` read from the cache. The + readers use the cached `Worksheet` instead of calling + `spreadsheet.worksheet(name)`. +- Keep the cache in step with our own changes: add to it in `_create_tab`, + remove from it in `delete_old_output_tabs`. Clear it wherever `_info` is + reset (`set_g_sheet_name`, and the name-change branch of `info`). +- Opening still costs 1 read for `Spreadsheet.__init__` plus 1 for the + metadata fetch. Avoiding that second read means building `Spreadsheet` + without its constructor, which relies on gspread internals. Not recommended. + +### Keeping the cache fresh + +Reading happens at the start of a run, so the cache is fresh then. Writing +happens after the selection algorithm, which usually takes seconds but has +taken half an hour on real selections. During that time someone may add, +rename or delete tabs, including the output tabs from an earlier run. A stale +cache then causes two concrete failures: + +- `find_unused_tab_suffix` picks a suffix someone has since used, and + `add_worksheet` fails because the tab name already exists. +- `delete_old_output_tabs` tries to delete a tab that is already gone, or + misses one created since. + +So: + +- Record when the cache was filled, using `time.monotonic()` (the age is all + we need, and it ignores wall-clock jumps). +- Add `_refresh_tab_cache_if_older_than(max_age_seconds)`, which re-fetches the + metadata (1 read) only when the cache is older than that. +- Call it with a class attribute `tab_cache_max_age_seconds = 60`. Match + `new_tab_default_size_rows`, so a caller or test can change it on an instance. +- Call it at the two points where tab names are decided: + - at the start of `write_selected`, before `find_unused_tab_suffix`. + `write_remaining` reuses that suffix, so it needs no refresh of its own. + - at the start of `delete_old_output_tabs`. That is normally a separate CLI + command, so the cache is usually fresh anyway. +- `highlight_dupes` runs straight after `write_remaining`, and the tab it needs + is one we created and cached ourselves, so it needs no refresh. +- If a read finds a tab missing, re-fetch once before raising "tab not found". + This covers someone fixing a tab name in the sheet and re-running in the + same process (for example a long-lived service holding the data source). +- Tests: set `tab_cache_max_age_seconds = 0` to force a refresh, and leave it + large to show none happens. Count requests on the client to check both cases. + +A slow selection pays 1 extra read. A fast one pays nothing. + +### Saving + +Saves 2 each on the features, people and already-selected tabs (the +`worksheets()` check and the `worksheet()` call both go), and 1 each in +`write_selected` and `highlight_dupes`. That is offset by the extra metadata +read on open, and by 1 refresh read when the selection is slow. +**About 14 → 7 reads per selection (8 for a slow one).** + +## Option C: read each tab with one `values.get` + +`read_feature_data` and `read_people_data` call `row_values(1)` and then +`get_all_records()`, which is two requests for the same data. +`read_already_selected_data` already does it in one `get()`. + +- Replace both with one `worksheet.get(...)` and take the header from row 0, + then build the records from the remaining rows. +- Must match `get_all_records()` behaviour exactly: + - formatted values (its default `value_render_option`) + - `numericise_ignore=["all"]` for people, numericising for features (the + features path currently numericises, then `_stringify_records` turns the + numbers back into strings; check whether that round trip changes anything, + e.g. `"007"` → `7` → `"7"`, and whether keeping it is intended) + - `expected_headers=[]` handling of duplicate headers + - padding short rows and dropping trailing empty rows +- Existing tests for the readers should pin this down. Add tests for the edge + cases above before changing the code. + +Saves 1 read per tab: **2 per selection**. + +## Option D: open by title without building a `Spreadsheet` twice + +`info` calls `client.open(title)`, which searches Drive and then builds a +`Spreadsheet` (1 Sheets read), only to pass `found.id` to `open_gsheet()`, +which builds another. + +- Use `client.list_spreadsheet_files(title)` (Drive only) to find the id, + raising `SpreadsheetNotFoundError` when it returns nothing, then call + `open_gsheet()` as now. +- Check how `client.open()` picks between several files with the same title, + and keep the same choice. + +Saves **1 read** when opening by title. + +## Option E: fetch all input tabs in one request + +Use `spreadsheet.values_batch_get([features_range, people_range, already_selected_range])` +to get every input tab in one request, then hand each reader its slice. + +- Brings a whole selection's reads down to about 3 (open, metadata, batch), + from about 5 after B and C. +- Needs a restructure: today each `read_*_data` context manager fetches its + own tab when it is entered, and they are called separately by the selection + code. The data source would need a "prefetch" step, or to fetch everything + on the first read and cache the result. +- Only worth it after B and C, and only if B and C prove not to be enough. + +## Option F: fewer writes + +Writes have their own 60 per minute quota, so these matter less, but they are +cheap to do: + +- `delete_old_output_tabs` sends one `del_worksheet` (a `batch_update`) per + tab. Send one `spreadsheet.batch_update` with a `deleteSheet` request per tab. +- `highlight_dupes` looks up the remaining tab again. Keep the `Worksheet` + created in `write_remaining` and reuse it. (Option B also removes the read.) +- `write_selected` and `write_remaining` each make 3 writes (`add_worksheet`, + `update`, `format`). These could be combined into one `batch_update`, but + that means writing raw API requests instead of using gspread helpers. Not + recommended. + +## Outside this library + +The service could avoid repeat work itself, for example by not re-reading a +sheet it checked seconds earlier, and by spacing out selections. Retries count +against the quota too: `BackOffHTTPClient` retries on 429, and each retry is +another request. + +## Summary + +| Option | Change | Reads saved | Effort | Risk | +| ------ | ---------------------------------- | ------------------------------ | ------ | ---------------------------- | +| A | `inspect_gsheet()` | 1 per check | Small | Low | +| B | Cache tab list per open | ~7 per selection | Medium | Low (refreshed when stale) | +| C | One `get()` per tab | 2 per selection | Small | Medium (parsing equivalence) | +| D | Drive-only title lookup | 1 per open by title | Small | Low | +| E | One batch read for all tabs | 2 more per selection after B+C | Large | Medium | +| F | Batch tab deletes, reuse worksheet | Writes only | Small | Low | + +Suggested order: A as its own PR (it is what the check needs). Then B + D in a +second PR, and C in a third, as C is the one most likely to change behaviour +subtly. Revisit E only if real usage still hits the limit. From f70d9c9dd10689d0b58cdf9e7fd0457eff3e066e Mon Sep 17 00:00:00 2001 From: Hamish Downer Date: Mon, 19 Oct 2026 09:27:59 +0100 Subject: [PATCH 2/6] feat: add inspect_gsheet to check a sheet with one Sheets read Share the Drive access check between open_gsheet and inspect_gsheet, and document the network and auth errors both can raise. Co-Authored-By: Claude Opus 5.5 --- docs/adapters.md | 31 +++++ src/sortition_algorithms/__init__.py | 4 +- src/sortition_algorithms/gsheet.py | 109 +++++++++++++--- tests/test_gsheet.py | 181 ++++++++++++++++++++++++++- 4 files changed, 308 insertions(+), 17 deletions(-) diff --git a/docs/adapters.md b/docs/adapters.md index a46a8fd..2a4da73 100644 --- a/docs/adapters.md +++ b/docs/adapters.md @@ -292,12 +292,43 @@ spreadsheet = info.spreadsheet # already opened, no need to open it again an unshared file as "not found", so on a Drive 404 the helper asks the Sheets API, which distinguishes "not shared" (403) from "does not exist" (404). Any other Google error propagates unchanged as `gspread.exceptions.APIError`. + Network failures (`requests.exceptions.RequestException`, such as a timeout) + and access-token refresh failures (`google.auth.exceptions.GoogleAuthError`) + are not Google API errors, and propagate unwrapped from `open_gsheet()` and + `inspect_gsheet()` alike. - `GSheetInfo` is a frozen dataclass: `spreadsheet`, `file_id`, `title`, `url`, `mimetype`, `can_edit` and `service_account_email`. Read-only access is data, not an error, because reading a Viewer-shared sheet is legitimate; `require_writable()` raises `SpreadsheetReadOnlyError` for the callers that will write. +To check a sheet without opening it for use (for example, to show a "sheet is +ready" status before a selection), use `inspect_gsheet()` instead. It raises the +same errors, and also returns the tab titles, using one Drive request and one +Sheets request. Opening with `open_gsheet()` and then calling `worksheets()` +costs two Sheets requests. That matters because Google allows a service account +only 60 Sheets read requests per minute, shared by everything that uses it. + +```python +from sortition_algorithms import GSheetSummary, inspect_gsheet + +summary: GSheetSummary = inspect_gsheet(client, "https://docs.google.com/spreadsheets/d/.../edit") +if "Respondents" not in summary.worksheet_titles: + ... +``` + +- `GSheetSummary` is a frozen dataclass: `file_id`, `title`, `url`, `mimetype`, + `can_edit`, `worksheet_titles` (a tuple, including hidden tabs), + `fetched_at` and `service_account_email`. It has no `spreadsheet`, so use + `open_gsheet()` when you need to read or write. +- `fetched_at` is a timezone-aware UTC `datetime` taken when the Sheets + response arrived. If you keep a summary around, use it to decide when to + check again. It is ignored when comparing summaries, so two summaries of an + unchanged sheet are equal. +- If Drive allows the file but the Sheets API refuses it (for example, the + Sheets API is not enabled for the Google Cloud project), the error propagates + as `gspread.exceptions.APIError`. + All four errors are `SelectionError` subclasses with an `error_code` (`spreadsheet_not_found`, `spreadsheet_not_shared`, `spreadsheet_read_only`, `not_native_gsheet`) and `error_params` for translation - see the diff --git a/src/sortition_algorithms/__init__.py b/src/sortition_algorithms/__init__.py index 82d024a..fcfacc5 100644 --- a/src/sortition_algorithms/__init__.py +++ b/src/sortition_algorithms/__init__.py @@ -31,7 +31,7 @@ report_min_max_error_details_structured, write_features, ) -from sortition_algorithms.gsheet import GSheetInfo, make_gsheet_client, open_gsheet +from sortition_algorithms.gsheet import GSheetInfo, GSheetSummary, inspect_gsheet, make_gsheet_client, open_gsheet from sortition_algorithms.people import ( FeatureValueCountCheck, check_people_per_feature_value, @@ -50,6 +50,7 @@ "FeatureValueCountCheck", "GSheetDataSource", "GSheetInfo", + "GSheetSummary", "InfeasibleQuotasCantRelaxError", "InfeasibleQuotasError", "MinMaxCrossFeatureIssue", @@ -65,6 +66,7 @@ "check_people_per_feature_value", "count_people_per_feature_value", "find_random_sample", + "inspect_gsheet", "make_gsheet_client", "open_gsheet", "read_in_features", diff --git a/src/sortition_algorithms/gsheet.py b/src/sortition_algorithms/gsheet.py index 0aca5ef..22c5b2a 100644 --- a/src/sortition_algorithms/gsheet.py +++ b/src/sortition_algorithms/gsheet.py @@ -1,12 +1,13 @@ # ABOUTME: Helpers for opening a Google Sheet by URL or file id with precise access-failure classification. # ABOUTME: Also holds the gspread client factory shared by GSheetDataSource and other callers. -from dataclasses import dataclass +from dataclasses import dataclass, field +from datetime import UTC, datetime from pathlib import Path from typing import Any import gspread -from gspread.urls import DRIVE_FILES_API_V3_URL +from gspread.urls import DRIVE_FILES_API_V3_URL, SPREADSHEET_DRIVE_URL from gspread.utils import extract_id_from_url from oauth2client.service_account import ServiceAccountCredentials @@ -105,15 +106,19 @@ def _classify_drive_not_found( raise drive_error -def open_gsheet(client: gspread.client.Client, url_or_id: str) -> GSheetInfo: - """ - Open a Google Sheet by URL or file id and report what the service account can do with it. +@dataclass(frozen=True) +class _DriveFile: + """What the Drive API says about a file that is a native Google Sheet.""" - Raises: - - SpreadsheetNotFoundError - the id does not exist - - SpreadsheetNotSharedError - exists, but not shared with this service account - - NotNativeGoogleSheetError - the Drive file is not a native Google Sheet (eg an uploaded .xlsx) - - gspread.exceptions.APIError - anything else from Google (quota, 5xx, bad credentials) + file_id: str + mimetype: str + can_edit: bool + + +def _check_drive_file(client: gspread.client.Client, url_or_id: str) -> _DriveFile: + """ + Ask the Drive API about the file and check it is a native Google Sheet. + Raises the access errors documented on open_gsheet(). """ file_id = extract_id_from_url(url_or_id) if url_or_id.startswith("https://") else url_or_id try: @@ -130,13 +135,89 @@ def open_gsheet(client: gspread.client.Client, url_or_id: str) -> GSheetInfo: mimetype = metadata.get("mimeType", "") if mimetype != NotNativeGoogleSheetError.NATIVE_GSHEET_MIMETYPE: raise NotNativeGoogleSheetError(mimetype=mimetype, file_name=metadata.get("name", "")) - spreadsheet = client.open_by_key(file_id) + return _DriveFile( + file_id=file_id, + mimetype=mimetype, + can_edit=bool(metadata.get("capabilities", {}).get("canEdit", False)), + ) + + +def open_gsheet(client: gspread.client.Client, url_or_id: str) -> GSheetInfo: + """ + Open a Google Sheet by URL or file id and report what the service account can do with it. + + Raises: + - SpreadsheetNotFoundError - the id does not exist + - SpreadsheetNotSharedError - exists, but not shared with this service account + - NotNativeGoogleSheetError - the Drive file is not a native Google Sheet (eg an uploaded .xlsx) + - gspread.exceptions.APIError - anything else from Google (quota, 5xx, bad credentials) + - requests.exceptions.RequestException - network failures (connection errors, timeouts), + propagated unwrapped from requests + - google.auth.exceptions.GoogleAuthError - failure to refresh the access token, + propagated unwrapped from google-auth + """ + drive_file = _check_drive_file(client, url_or_id) + spreadsheet = client.open_by_key(drive_file.file_id) return GSheetInfo( spreadsheet=spreadsheet, - file_id=file_id, + file_id=drive_file.file_id, title=spreadsheet.title, url=spreadsheet.url, - mimetype=mimetype, - can_edit=bool(metadata.get("capabilities", {}).get("canEdit", False)), + mimetype=drive_file.mimetype, + can_edit=drive_file.can_edit, + service_account_email=service_account_email(client), + ) + + +@dataclass(frozen=True) +class GSheetSummary: + """What the service account can see of a Google Sheet, without opening it for use.""" + + file_id: str + title: str + url: str + mimetype: str + can_edit: bool + worksheet_titles: tuple[str, ...] + # when the Sheets metadata was fetched, so a caller that keeps the summary + # can decide when it is too old to trust + fetched_at: datetime = field(compare=False) + service_account_email: str = "" + + +def inspect_gsheet(client: gspread.client.Client, url_or_id: str) -> GSheetSummary: + """ + Report what the service account can do with a Google Sheet, including its tab + titles, using one Drive and one Sheets request. Unlike open_gsheet() it does not + build a gspread.Spreadsheet, which would cost a further Sheets request. + + Raises: + - SpreadsheetNotFoundError - the id does not exist + - SpreadsheetNotSharedError - exists, but not shared with this service account + - NotNativeGoogleSheetError - the Drive file is not a native Google Sheet (eg an uploaded .xlsx) + - gspread.exceptions.APIError - anything else from Google (quota, 5xx, bad credentials), + including the Sheets API refusing a file that Drive allowed (eg the Sheets API + is not enabled for the project) + - requests.exceptions.RequestException - network failures (connection errors, timeouts), + propagated unwrapped from requests + - google.auth.exceptions.GoogleAuthError - failure to refresh the access token, + propagated unwrapped from google-auth + """ + drive_file = _check_drive_file(client, url_or_id) + metadata = client.http_client.fetch_sheet_metadata( + drive_file.file_id, + params={"includeGridData": "false", "fields": "properties.title,sheets.properties.title"}, + ) + fetched_at = datetime.now(UTC) + # the Sheets API leaves out fields that hold their default value, so an empty + # title arrives as a missing key + return GSheetSummary( + file_id=drive_file.file_id, + title=metadata.get("properties", {}).get("title", ""), + url=SPREADSHEET_DRIVE_URL % drive_file.file_id, + mimetype=drive_file.mimetype, + can_edit=drive_file.can_edit, + worksheet_titles=tuple(sheet.get("properties", {}).get("title", "") for sheet in metadata.get("sheets", [])), + fetched_at=fetched_at, service_account_email=service_account_email(client), ) diff --git a/tests/test_gsheet.py b/tests/test_gsheet.py index 60e0d2b..b2904c0 100644 --- a/tests/test_gsheet.py +++ b/tests/test_gsheet.py @@ -1,20 +1,25 @@ """ -Tests for opening a Google Sheet via open_gsheet() with precise failure classification. +Tests for opening a Google Sheet via open_gsheet(), and summarising one via +inspect_gsheet(), with precise failure classification. No live Google API: the Drive and Sheets responses are the bodies recorded against the real API (gspread 6.2.1) for each case. """ import json +from dataclasses import replace +from datetime import UTC, datetime, timedelta from pathlib import Path from typing import Any from unittest.mock import MagicMock, patch +import google.auth.exceptions import gspread import pytest import requests from gspread.urls import DRIVE_FILES_API_V3_URL +import sortition_algorithms from sortition_algorithms import gsheet from sortition_algorithms.errors import ( NotNativeGoogleSheetError, @@ -22,7 +27,15 @@ SpreadsheetNotSharedError, SpreadsheetReadOnlyError, ) -from sortition_algorithms.gsheet import GSHEET_SCOPE, GSheetInfo, make_gsheet_client, open_gsheet, service_account_email +from sortition_algorithms.gsheet import ( + GSHEET_SCOPE, + GSheetInfo, + GSheetSummary, + inspect_gsheet, + make_gsheet_client, + open_gsheet, + service_account_email, +) FILE_ID = "1B-S6esBj7rqbSJulqAZUh4-x1FXU-rPdNabMVDtsynM" URL = f"https://docs.google.com/spreadsheets/d/{FILE_ID}/edit" @@ -50,6 +63,22 @@ "error": {"code": 403, "message": "The caller does not have permission", "status": "PERMISSION_DENIED"} } SHEETS_NOT_FOUND_BODY = {"error": {"code": 404, "message": "Requested entity was not found.", "status": "NOT_FOUND"}} +# the shape of a Sheets spreadsheets.get body under inspect_gsheet()'s fields mask +SHEETS_METADATA_BODY = { + "properties": {"title": "Demo Sheet"}, + "sheets": [ + {"properties": {"title": "Categories"}}, + {"properties": {"title": "Respondents"}}, + {"properties": {"title": "Original Selected - output - 0"}}, + ], +} +SHEETS_SERVICE_DISABLED_BODY = { + "error": { + "code": 403, + "message": "Google Sheets API has not been used in project 123 before or it is disabled.", + "status": "PERMISSION_DENIED", + } +} DRIVE_QUOTA_BODY = { "error": { "code": 403, @@ -107,6 +136,7 @@ def _make_client( spreadsheet.url = f"https://docs.google.com/spreadsheets/d/{FILE_ID}" client.open_by_key.return_value = spreadsheet client.http_client.auth.service_account_email = email + client.http_client.fetch_sheet_metadata.return_value = SHEETS_METADATA_BODY return client @@ -275,3 +305,150 @@ def test_make_gsheet_client_accepts_a_fail_fast_http_client(): ): make_gsheet_client(Path("/some/auth.json"), http_client=gspread.HTTPClient) authorize.assert_called_once_with(from_keyfile.return_value, http_client=gspread.HTTPClient) + + +def test_inspect_gsheet_shared_editable(): + client = _make_client(_drive_body()) + before = datetime.now(UTC) + summary = inspect_gsheet(client, URL) + after = datetime.now(UTC) + assert isinstance(summary, GSheetSummary) + assert summary.file_id == FILE_ID + assert summary.title == "Demo Sheet" + assert summary.url == f"https://docs.google.com/spreadsheets/d/{FILE_ID}" + assert summary.mimetype == NATIVE + assert summary.can_edit is True + assert summary.worksheet_titles == ("Categories", "Respondents", "Original Selected - output - 0") + assert summary.service_account_email == EMAIL + assert before <= summary.fetched_at <= after + client.http_client.request.assert_called_once_with( + "get", + f"{DRIVE_FILES_API_V3_URL}/{FILE_ID}", + params={"supportsAllDrives": True, "fields": "mimeType,name,capabilities/canEdit"}, + ) + # one Sheets request, and no Spreadsheet built (which would cost another) + client.http_client.fetch_sheet_metadata.assert_called_once_with( + FILE_ID, + params={"includeGridData": "false", "fields": "properties.title,sheets.properties.title"}, + ) + client.open_by_key.assert_not_called() + + +def test_inspect_gsheet_url_matches_open_gsheet(): + summary = inspect_gsheet(_make_client(_drive_body()), URL) + info = open_gsheet(_make_client(_drive_body()), URL) + assert summary.url == info.url + + +def test_inspect_gsheet_shared_viewer_is_read_only(): + client = _make_client(_drive_body(can_edit=False)) + summary = inspect_gsheet(client, URL) + assert summary.can_edit is False + + +def test_inspect_gsheet_accepts_bare_file_id(): + client = _make_client(_drive_body()) + summary = inspect_gsheet(client, FILE_ID) + assert summary.file_id == FILE_ID + assert client.http_client.request.call_args.args[1] == f"{DRIVE_FILES_API_V3_URL}/{FILE_ID}" + assert client.http_client.fetch_sheet_metadata.call_args.args[0] == FILE_ID + + +def test_inspect_gsheet_not_shared(): + drive_error = _api_error(404, DRIVE_NOT_FOUND_BODY) + client = _make_client(drive_error, sheets=_permission_error()) + with pytest.raises(SpreadsheetNotSharedError) as excinfo: + inspect_gsheet(client, URL) + assert excinfo.value.error_params == {"spreadsheet_name": URL, "service_account_email": EMAIL} + # one Sheets request to classify the Drive 404, and no metadata fetch + client.open_by_key.assert_called_once_with(FILE_ID) + client.http_client.fetch_sheet_metadata.assert_not_called() + + +def test_inspect_gsheet_missing(): + drive_error = _api_error(404, DRIVE_NOT_FOUND_BODY) + client = _make_client(drive_error, sheets=_spreadsheet_not_found()) + with pytest.raises(SpreadsheetNotFoundError) as excinfo: + inspect_gsheet(client, URL) + assert excinfo.value.error_params == {"spreadsheet_name": URL} + client.http_client.fetch_sheet_metadata.assert_not_called() + + +def test_inspect_gsheet_xlsx_is_not_native(): + client = _make_client(_drive_body(mimetype=XLSX, name="upload.xlsx")) + with pytest.raises(NotNativeGoogleSheetError) as excinfo: + inspect_gsheet(client, URL) + assert excinfo.value.file_name == "upload.xlsx" + client.http_client.fetch_sheet_metadata.assert_not_called() + + +def test_inspect_gsheet_non_404_drive_error_propagates(): + drive_error = _api_error(403, DRIVE_QUOTA_BODY) + client = _make_client(drive_error) + with pytest.raises(gspread.exceptions.APIError) as excinfo: + inspect_gsheet(client, URL) + assert excinfo.value is drive_error + client.http_client.fetch_sheet_metadata.assert_not_called() + + +def test_inspect_gsheet_sheets_refusal_after_drive_success_propagates(): + """Drive allows the file but the Sheets API refuses, eg it is not enabled for the project.""" + client = _make_client(_drive_body()) + sheets_error = _api_error(403, SHEETS_SERVICE_DISABLED_BODY) + client.http_client.fetch_sheet_metadata.side_effect = sheets_error + with pytest.raises(gspread.exceptions.APIError) as excinfo: + inspect_gsheet(client, URL) + assert excinfo.value is sheets_error + + +def test_inspect_gsheet_tolerates_fields_left_out_of_the_response(): + """The Sheets API leaves out default-valued fields, eg an empty title.""" + client = _make_client(_drive_body()) + client.http_client.fetch_sheet_metadata.return_value = {"sheets": [{"properties": {}}, {}]} + summary = inspect_gsheet(client, URL) + assert summary.title == "" + assert summary.worksheet_titles == ("", "") + + +def test_inspect_gsheet_tolerates_a_response_with_no_tabs(): + client = _make_client(_drive_body()) + client.http_client.fetch_sheet_metadata.return_value = {"properties": {"title": "Demo Sheet"}} + summary = inspect_gsheet(client, URL) + assert summary.title == "Demo Sheet" + assert summary.worksheet_titles == () + + +def test_inspect_gsheet_network_error_after_drive_success_propagates(): + client = _make_client(_drive_body()) + timeout = requests.exceptions.ReadTimeout("read timed out") + client.http_client.fetch_sheet_metadata.side_effect = timeout + with pytest.raises(requests.exceptions.ReadTimeout) as excinfo: + inspect_gsheet(client, URL) + assert excinfo.value is timeout + + +def test_inspect_gsheet_token_refresh_error_after_drive_success_propagates(): + client = _make_client(_drive_body()) + refresh_error = google.auth.exceptions.RefreshError("invalid_grant") + client.http_client.fetch_sheet_metadata.side_effect = refresh_error + with pytest.raises(google.auth.exceptions.RefreshError) as excinfo: + inspect_gsheet(client, URL) + assert excinfo.value is refresh_error + + +def test_gsheet_summary_equality_ignores_fetched_at(): + summary = inspect_gsheet(_make_client(_drive_body()), URL) + later = replace(summary, fetched_at=summary.fetched_at + timedelta(minutes=1)) + assert later == summary + assert replace(summary, can_edit=False) != summary + + +def test_gsheet_summary_is_frozen(): + summary = inspect_gsheet(_make_client(_drive_body()), URL) + with pytest.raises(AttributeError): + summary.can_edit = False # type: ignore[misc] + + +def test_inspect_gsheet_is_exported_from_package(): + assert sortition_algorithms.inspect_gsheet is inspect_gsheet + assert sortition_algorithms.GSheetSummary is GSheetSummary From eb03a50f8d5410847e6753fbfef81fd5e48f69bd Mon Sep 17 00:00:00 2001 From: Hamish Downer Date: Mon, 19 Oct 2026 10:06:58 +0100 Subject: [PATCH 3/6] perf: fetch the Google Sheet tab list once per GSheetDataSource Reuse one tab list fetch for reading input tabs and naming output tabs, re-fetching it when older than tab_cache_max_age_seconds or when an input tab is missing. Cuts a selection from about 14 Sheets reads to about 7. Co-Authored-By: Claude Opus 5.5 --- docs/adapters.md | 15 +++ docs/agent/974-reduce-sheets-reads.md | 18 +++ src/sortition_algorithms/adapters.py | 114 +++++++++------- tests/test_adapters.py | 186 ++++++++++++++++++++++++++ 4 files changed, 284 insertions(+), 49 deletions(-) diff --git a/docs/adapters.md b/docs/adapters.md index 2a4da73..6faee4d 100644 --- a/docs/adapters.md +++ b/docs/adapters.md @@ -340,6 +340,21 @@ Celery failure callback. property, `get_title()` and the `read_*` methods raise the errors above for every access failure. +#### How often `GSheetDataSource` reads the tab list + +To stay well inside the 60 reads per minute, `GSheetDataSource` fetches the +spreadsheet's tab list once (one Sheets request) and reuses it to find the +input tabs and to choose names for the output tabs. Tabs it creates or deletes +itself are kept in step with the list. + +The list is fetched again before use once it is older than +`data_source.tab_cache_max_age_seconds` (60 by default), because people may add +or remove tabs while a slow selection runs. A fast selection therefore reads the +tab list once, and a slow one twice. If an input tab is missing from the list, +the list is fetched once more before the "tab not found" error is raised, in +case someone has just fixed the tab name. Set `tab_cache_max_age_seconds = 0` +to fetch the list every time it is used. + ## Writing custom Data Source classes You can create custom data source classes for other data sources like Excel files, SQL databases, or APIs. diff --git a/docs/agent/974-reduce-sheets-reads.md b/docs/agent/974-reduce-sheets-reads.md index 361948f..11825a6 100644 --- a/docs/agent/974-reduce-sheets-reads.md +++ b/docs/agent/974-reduce-sheets-reads.md @@ -202,6 +202,24 @@ So: A slow selection pays 1 extra read. A fast one pays nothing. +### As implemented + +Two changes from the above, both simplifications: + +- The tab list is fetched lazily, on first use, not when `info` opens the + spreadsheet. Callers that only want `get_title()` or `can_edit` don't pay + for it. +- Rather than calling a refresh at chosen points, every use of the tab list + re-fetches it if it is at least `tab_cache_max_age_seconds` old. That gives + the same read counts for a selection, and also covers a long-lived service + that reads the input tabs again more than a minute later. + `tab_cache_max_age_seconds` is an instance attribute set in `__init__`, as + `new_tab_default_size_rows` is. A value of 0 means fetch on every use. + +The fetch uses the `fields` mask `sheets.properties`. The "tab not found" +re-fetch costs an extra read even when the list was fetched a moment earlier, +which is fine on an error path. + ### Saving Saves 2 each on the features, people and already-selected tabs (the diff --git a/src/sortition_algorithms/adapters.py b/src/sortition_algorithms/adapters.py index cfe3ad5..14785b2 100644 --- a/src/sortition_algorithms/adapters.py +++ b/src/sortition_algorithms/adapters.py @@ -7,6 +7,7 @@ import abc import csv import logging +import time from collections import Counter, defaultdict from collections.abc import Generator, Iterable, Sequence from contextlib import contextmanager @@ -544,6 +545,12 @@ def __init__( self._info: GSheetInfo | None = None self.new_tab_default_size_rows = 2 self.new_tab_default_size_cols = 40 + # the tab list is fetched once and reused, to save Sheets API reads. + # Once it is older than this it is fetched again before use, as people + # may have changed the tabs while a slow selection was running. + self.tab_cache_max_age_seconds: float = 60 + self._tab_cache: dict[str, gspread.Worksheet] | None = None + self._tab_cache_filled_at = 0.0 self._g_sheet_name = "" self._open_g_sheet_name = "" self.selected_tab_name = "" @@ -577,6 +584,7 @@ def info(self) -> GSheetInfo: if self._open_g_sheet_name != self._g_sheet_name: # reset the spreadsheet if the name changed self._info = None + self._tab_cache = None self.tab_namer.reset() if self._info is None: if self._g_sheet_name.startswith("https://"): @@ -607,34 +615,76 @@ def require_writable(self) -> None: """Raise SpreadsheetReadOnlyError if the service account cannot write to the spreadsheet.""" self.info.require_writable() + def _refresh_tab_cache(self) -> None: + """ + Fetch the tab list with one Sheets read and keep a Worksheet for each tab. + + This builds the Worksheets the same way Spreadsheet.worksheet() does, + which makes no further request, rather than calling worksheets() or + worksheet(name), each of which re-fetches the metadata. + """ + spreadsheet = self.spreadsheet + metadata = spreadsheet.fetch_sheet_metadata(params={"includeGridData": "false", "fields": "sheets.properties"}) + self._tab_cache = { + item["properties"]["title"]: gspread.Worksheet( + spreadsheet, item["properties"], spreadsheet.id, spreadsheet.client + ) + for item in metadata.get("sheets", []) + } + self._tab_cache_filled_at = time.monotonic() + + def _get_tab_cache(self) -> dict[str, gspread.Worksheet]: + cache_age = time.monotonic() - self._tab_cache_filled_at + if self._tab_cache is None or cache_age >= self.tab_cache_max_age_seconds: + self._refresh_tab_cache() + assert self._tab_cache is not None + return self._tab_cache + def _get_tab(self, tab_name: str) -> gspread.Worksheet | None: if not self._g_sheet_name: return None - tab_list = self.spreadsheet.worksheets() - try: - return next(tab for tab in tab_list if tab.title == tab_name) - except StopIteration: - return None + return self._get_tab_cache().get(tab_name) - def _tab_exists(self, tab_name: str) -> bool: - return bool(self._get_tab(tab_name)) + def _get_input_tab(self, tab_name: str) -> gspread.Worksheet: + """ + Find a tab to read from, or raise a "tab_not_found" SelectionError. + + If the tab isn't in the tab list, fetch the list again before giving up, + in case someone has fixed the tab name since the list was fetched. + """ + tab = self._get_tab(tab_name) + if tab is None and self._g_sheet_name: + self._refresh_tab_cache() + tab = self._get_tab(tab_name) + if tab is None: + msg = f"Error in Google sheet: no tab called '{tab_name}' found in spreadsheet '{self.spreadsheet.title}'." + raise SelectionError( + message=msg, + error_code="tab_not_found", + error_params={"tab_name": tab_name, "spreadsheet_title": self.spreadsheet.title}, + ) + return tab def _get_tab_titles(self) -> list[str]: if not self._g_sheet_name: return [] - return [tab.title for tab in self.spreadsheet.worksheets()] + return list(self._get_tab_cache()) def _create_tab(self, tab_name: str) -> gspread.Worksheet: - return self.spreadsheet.add_worksheet( + tab = self.spreadsheet.add_worksheet( title=tab_name, rows=self.new_tab_default_size_rows, cols=self.new_tab_default_size_cols, ) + if self._tab_cache is not None: + self._tab_cache[tab.title] = tab + return tab def set_g_sheet_name(self, g_sheet_name: str) -> None: # if we're changing spreadsheet, reset the spreadsheet object if self._g_sheet_name != g_sheet_name: self._info = None + self._tab_cache = None self._g_sheet_name = g_sheet_name self.tab_namer.reset() @@ -646,17 +696,7 @@ def read_feature_data( self, report: RunReport ) -> Generator[tuple[Iterable[str], Iterable[dict[str, str]]], None, None]: self._report = report - if not self._tab_exists(self.feature_tab_name): - msg = ( - f"Error in Google sheet: no tab called '{self.feature_tab_name}' " - f"found in spreadsheet '{self.spreadsheet.title}'." - ) - raise SelectionError( - message=msg, - error_code="tab_not_found", - error_params={"tab_name": self.feature_tab_name, "spreadsheet_title": self.spreadsheet.title}, - ) - tab_features = self.spreadsheet.worksheet(self.feature_tab_name) + tab_features = self._get_input_tab(self.feature_tab_name) feature_head = tab_features.row_values(1) feature_body = _stringify_records(tab_features.get_all_records(expected_headers=[])) yield feature_head, feature_body @@ -666,18 +706,7 @@ def read_people_data( self, report: RunReport ) -> Generator[tuple[Iterable[str], Iterable[dict[str, str]]], None, None]: self._report = report - if not self._tab_exists(self.people_tab_name): - msg = ( - f"Error in Google sheet: no tab called '{self.people_tab_name}' " - f"found in spreadsheet '{self.spreadsheet.title}'." - ) - raise SelectionError( - message=msg, - error_code="tab_not_found", - error_params={"tab_name": self.people_tab_name, "spreadsheet_title": self.spreadsheet.title}, - ) - - tab_people = self.spreadsheet.worksheet(self.people_tab_name) + tab_people = self._get_input_tab(self.people_tab_name) # if we don't read this in here we can't check if there are 2 columns with the same name people_head = tab_people.row_values(1) # the numericise_ignore doesn't convert the phone numbers to ints... @@ -764,21 +793,7 @@ def read_already_selected_data( yield [], [] return - if not self._tab_exists(self.already_selected_tab_name): - msg = ( - f"Error in Google sheet: no tab called '{self.already_selected_tab_name}' " - f"found in spreadsheet '{self.spreadsheet.title}'." - ) - raise SelectionError( - message=msg, - error_code="tab_not_found", - error_params={ - "tab_name": self.already_selected_tab_name, - "spreadsheet_title": self.spreadsheet.title, - }, - ) - - tab_already_selected = self.spreadsheet.worksheet(self.already_selected_tab_name) + tab_already_selected = self._get_input_tab(self.already_selected_tab_name) # Get all values from the sheet all_values = tab_already_selected.get(value_render_option=ValueRenderOption.unformatted, pad_values=True) @@ -845,10 +860,10 @@ def delete_old_output_tabs(self, dry_run: bool = False) -> list[str]: if not self._g_sheet_name: return [] - all_tabs = self.spreadsheet.worksheets() + all_tabs = self._get_tab_cache() tabs_to_delete: list[gspread.Worksheet] = [] - for tab in all_tabs: + for tab in all_tabs.values(): if self.tab_namer.matches_stubs(tab.title): tabs_to_delete.append(tab) @@ -857,6 +872,7 @@ def delete_old_output_tabs(self, dry_run: bool = False) -> list[str]: deleted_names.append(tab.title) if not dry_run: self.spreadsheet.del_worksheet(tab) + del all_tabs[tab.title] return deleted_names diff --git a/tests/test_adapters.py b/tests/test_adapters.py index f8069a8..43d5e1d 100644 --- a/tests/test_adapters.py +++ b/tests/test_adapters.py @@ -995,6 +995,192 @@ def test_not_native_gsheet_error_mimetype_map_contents(): assert names["text/tab-separated-values"] == "TSV file" +def _make_gsheet_source_with_tabs(tabs: dict[str, list[list[str]]]) -> tuple[GSheetDataSource, MagicMock]: + """ + A source whose spreadsheet has these tabs, with the cell data behind them. + + Changing `tabs` afterwards changes the sheet, as if someone had edited it. + Returns the source and the fake spreadsheet, whose fetch_sheet_metadata + call count is the number of tab list reads. + """ + source = _make_gsheet_source_with_mocked_client("application/vnd.google-apps.spreadsheet", file_name="My Sheet") + source.set_g_sheet_name("https://docs.google.com/spreadsheets/d/abc123/edit") + spreadsheet = source._client.open_by_key.return_value + spreadsheet.client = MagicMock(spec=gspread.http_client.HTTPClient) + + def fake_properties(title: str) -> dict[str, Any]: + sheet_id = list(tabs).index(title) + return {"sheetId": sheet_id, "title": title, "index": sheet_id, "gridProperties": {"rowCount": 100}} + + def fake_metadata(params: dict[str, str] | None = None) -> dict[str, Any]: + return {"sheets": [{"properties": fake_properties(title)} for title in tabs]} + + def fake_values_get(spreadsheet_id: str, range_name: str, params: Any = None) -> dict[str, Any]: + title = range_name.split("!")[0].strip("'") + return {"range": range_name, "majorDimension": "ROWS", "values": tabs[title]} + + def fake_add_worksheet(title: str, rows: int, cols: int) -> gspread.Worksheet: + tabs[title] = [] + return gspread.Worksheet(spreadsheet, fake_properties(title), spreadsheet.id, spreadsheet.client) + + def fake_del_worksheet(worksheet: gspread.Worksheet) -> None: + del tabs[worksheet.title] + + spreadsheet.fetch_sheet_metadata.side_effect = fake_metadata + spreadsheet.client.values_get.side_effect = fake_values_get + spreadsheet.add_worksheet.side_effect = fake_add_worksheet + spreadsheet.del_worksheet.side_effect = fake_del_worksheet + return source, spreadsheet + + +def _input_tabs() -> dict[str, list[list[str]]]: + return { + "Categories": [["feature", "value", "min", "max"], ["gender", "female", "1", "2"]], + "Respondents": [["id", "name", "gender"], ["p1", "Ann", "female"]], + "Selected": [["id", "name", "gender", "age", "area", "phone"], ["p2", "Bea", "female", "30", "north", "123"]], + } + + +def _read_all_input_tabs(source: GSheetDataSource) -> None: + with source.read_feature_data(RunReport()): + pass + with source.read_people_data(RunReport()): + pass + with source.read_already_selected_data(RunReport()): + pass + + +def test_gsheet_reading_all_input_tabs_fetches_tab_list_once(): + source, spreadsheet = _make_gsheet_source_with_tabs(_input_tabs()) + source.already_selected_tab_name = "Selected" + source.id_column = "id" + with source.read_feature_data(RunReport()) as (head, body): + assert list(head) == ["feature", "value", "min", "max"] + assert list(body) == [{"feature": "gender", "value": "female", "min": "1", "max": "2"}] + with source.read_people_data(RunReport()) as (head, body): + assert list(head) == ["id", "name", "gender"] + assert list(body) == [{"id": "p1", "name": "Ann", "gender": "female"}] + with source.read_already_selected_data(RunReport()) as (head, body): + assert list(head) == ["id", "name", "gender", "age", "area", "phone"] + assert [row["id"] for row in body] == ["p2"] + assert spreadsheet.fetch_sheet_metadata.call_count == 1 + spreadsheet.worksheets.assert_not_called() + spreadsheet.worksheet.assert_not_called() + + +def test_gsheet_tab_list_fetch_asks_only_for_tab_properties(): + source, spreadsheet = _make_gsheet_source_with_tabs(_input_tabs()) + with source.read_feature_data(RunReport()): + pass + spreadsheet.fetch_sheet_metadata.assert_called_once_with( + params={"includeGridData": "false", "fields": "sheets.properties"} + ) + + +def test_gsheet_missing_tab_refetches_tab_list_once_then_raises(): + tabs = _input_tabs() + del tabs["Categories"] + source, spreadsheet = _make_gsheet_source_with_tabs(tabs) + with pytest.raises(SelectionError) as excinfo, source.read_feature_data(RunReport()): + pass + assert excinfo.value.error_code == "tab_not_found" + assert excinfo.value.error_params == {"tab_name": "Categories", "spreadsheet_title": "My Sheet"} + assert "no tab called 'Categories' found in spreadsheet 'My Sheet'" in str(excinfo.value) + assert spreadsheet.fetch_sheet_metadata.call_count == 2 + + +def test_gsheet_missing_tab_is_found_after_someone_adds_it(): + tabs = _input_tabs() + categories = tabs.pop("Categories") + source, spreadsheet = _make_gsheet_source_with_tabs(tabs) + with source.read_people_data(RunReport()): + pass + # someone fixes the sheet after the tab list was fetched + tabs["Categories"] = categories + with source.read_feature_data(RunReport()) as (head, _): + assert list(head) == ["feature", "value", "min", "max"] + assert spreadsheet.fetch_sheet_metadata.call_count == 2 + + +def test_gsheet_writing_after_fast_selection_uses_cached_tab_list(): + source, spreadsheet = _make_gsheet_source_with_tabs(_input_tabs()) + _read_all_input_tabs(source) + source.write_selected([["id"], ["p1"]], RunReport()) + source.write_remaining([["id"], ["p3"], ["p4"]], RunReport()) + source.highlight_dupes([1]) + assert source.selected_tab_name == "Original Selected - output - 0" + assert source.remaining_tab_name == "Remaining - output - 0" + assert spreadsheet.fetch_sheet_metadata.call_count == 1 + + +def test_gsheet_writing_after_slow_selection_refetches_tab_list(): + tabs = _input_tabs() + source, spreadsheet = _make_gsheet_source_with_tabs(tabs) + _read_all_input_tabs(source) + # while the selection runs, someone else writes output tabs + tabs["Original Selected - output - 0"] = [] + tabs["Remaining - output - 0"] = [] + source.tab_cache_max_age_seconds = 0 + source.write_selected([["id"], ["p1"]], RunReport()) + assert source.selected_tab_name == "Original Selected - output - 1" + assert spreadsheet.fetch_sheet_metadata.call_count == 2 + + +def test_gsheet_cached_tab_list_includes_tabs_we_create(): + source, spreadsheet = _make_gsheet_source_with_tabs(_input_tabs()) + source.write_selected([["id"], ["p1"]], RunReport()) + source.write_remaining([["id"], ["p3"]], RunReport()) + assert source._get_tab_titles() == [ + "Categories", + "Respondents", + "Selected", + "Original Selected - output - 0", + "Remaining - output - 0", + ] + assert spreadsheet.fetch_sheet_metadata.call_count == 1 + + +def test_gsheet_delete_old_output_tabs_updates_cached_tab_list(): + tabs = _input_tabs() + tabs["Original Selected - output - 0"] = [] + tabs["Remaining - output - 0"] = [] + source, spreadsheet = _make_gsheet_source_with_tabs(tabs) + deleted = source.delete_old_output_tabs() + assert deleted == ["Original Selected - output - 0", "Remaining - output - 0"] + assert source._get_tab_titles() == ["Categories", "Respondents", "Selected"] + assert spreadsheet.del_worksheet.call_count == 2 + assert spreadsheet.fetch_sheet_metadata.call_count == 1 + + +def test_gsheet_delete_old_output_tabs_dry_run_leaves_cached_tab_list_alone(): + tabs = _input_tabs() + tabs["Remaining - output - 0"] = [] + source, spreadsheet = _make_gsheet_source_with_tabs(tabs) + assert source.delete_old_output_tabs(dry_run=True) == ["Remaining - output - 0"] + assert "Remaining - output - 0" in source._get_tab_titles() + spreadsheet.del_worksheet.assert_not_called() + + +def test_gsheet_delete_old_output_tabs_refetches_stale_tab_list(): + tabs = _input_tabs() + source, spreadsheet = _make_gsheet_source_with_tabs(tabs) + _read_all_input_tabs(source) + tabs["Remaining - output - 3"] = [] + source.tab_cache_max_age_seconds = 0 + assert source.delete_old_output_tabs() == ["Remaining - output - 3"] + assert spreadsheet.fetch_sheet_metadata.call_count == 2 + + +def test_gsheet_changing_sheet_name_clears_cached_tab_list(): + source, spreadsheet = _make_gsheet_source_with_tabs(_input_tabs()) + with source.read_feature_data(RunReport()): + pass + source.set_g_sheet_name("https://docs.google.com/spreadsheets/d/def456/edit") + with source.read_feature_data(RunReport()): + pass + assert spreadsheet.fetch_sheet_metadata.call_count == 2 + + # Tests for GSheetDataAdapter.find_header_row() From 10e7d32c074ef8f6eeb9b243de3600406306080e Mon Sep 17 00:00:00 2001 From: Hamish Downer Date: Mon, 19 Oct 2026 10:07:57 +0100 Subject: [PATCH 4/6] perf: find a Google Sheet by title with a Drive search only client.open() builds a Spreadsheet, costing a Sheets read, only for us to reopen it by id. Search Drive with list_spreadsheet_files() instead, taking the first exact name match as client.open() does. Co-Authored-By: Claude Opus 5.5 --- docs/agent/974-reduce-sheets-reads.md | 4 ++++ src/sortition_algorithms/adapters.py | 13 ++++++---- tests/test_adapters.py | 34 +++++++++++++++++++++++---- 3 files changed, 41 insertions(+), 10 deletions(-) diff --git a/docs/agent/974-reduce-sheets-reads.md b/docs/agent/974-reduce-sheets-reads.md index 11825a6..bc96c34 100644 --- a/docs/agent/974-reduce-sheets-reads.md +++ b/docs/agent/974-reduce-sheets-reads.md @@ -263,6 +263,10 @@ which builds another. Saves **1 read** when opening by title. +As implemented: `client.open()` takes the first file in the Drive results whose +`name` is exactly the title (the Drive search is limited to native Sheets), and +`info` now does the same with the `list_spreadsheet_files()` results. + ## Option E: fetch all input tabs in one request Use `spreadsheet.values_batch_get([features_range, people_range, already_selected_range])` diff --git a/src/sortition_algorithms/adapters.py b/src/sortition_algorithms/adapters.py index 14785b2..31d0351 100644 --- a/src/sortition_algorithms/adapters.py +++ b/src/sortition_algorithms/adapters.py @@ -593,11 +593,14 @@ def info(self) -> GSheetInfo: # open-by-title is a Drive search, so an unshared or missing # sheet simply has no match. Once found, feed the id through # the helper so the native check and can_edit come for free. - try: - found = self.client.open(self._g_sheet_name) - except gspread.SpreadsheetNotFound as err: - raise SpreadsheetNotFoundError(spreadsheet_name=self._g_sheet_name) from err - self._info = open_gsheet(self.client, found.id) + # We search Drive directly rather than use client.open(), which + # would spend a Sheets read building a Spreadsheet we discard. + # Like client.open(), take the first file with exactly this name. + files = self.client.list_spreadsheet_files(self._g_sheet_name) + found = next((file for file in files if file["name"] == self._g_sheet_name), None) + if found is None: + raise SpreadsheetNotFoundError(spreadsheet_name=self._g_sheet_name) + self._info = open_gsheet(self.client, found["id"]) self._open_g_sheet_name = self._g_sheet_name self._report.add_message_and_log("opened_gsheet", logging.INFO, title=self._info.title) return self._info diff --git a/tests/test_adapters.py b/tests/test_adapters.py index 43d5e1d..5317527 100644 --- a/tests/test_adapters.py +++ b/tests/test_adapters.py @@ -863,13 +863,14 @@ def _make_gsheet_source_with_mocked_client(mimetype: str, file_name: str = "Some mock_response = MagicMock() mock_response.json.return_value = {"mimeType": mimetype, "name": file_name} mock_client.http_client.request.return_value = mock_response - # open_by_key / open both return a fake Spreadsheet + # the Drive search for open-by-title finds the file + mock_client.list_spreadsheet_files.return_value = [{"id": "abc123", "name": file_name}] + # open_by_key returns a fake Spreadsheet fake_spreadsheet = MagicMock() fake_spreadsheet.id = "abc123" fake_spreadsheet.title = file_name fake_spreadsheet.url = "https://docs.google.com/spreadsheets/d/abc123" mock_client.open_by_key.return_value = fake_spreadsheet - mock_client.open.return_value = fake_spreadsheet source._client = mock_client return source @@ -894,15 +895,38 @@ def test_gsheet_spreadsheet_by_title_goes_through_open_gsheet(): source.set_g_sheet_name("My Sheet") # not a URL -> title branch sheet = source.spreadsheet assert sheet.title == "My Sheet" - # the title search finds the id, then the helper does the Drive check and opens by key - source._client.open.assert_called_once_with("My Sheet") + # the Drive title search finds the id, then the helper does the Drive check and opens by key + source._client.list_spreadsheet_files.assert_called_once_with("My Sheet") source._client.open_by_key.assert_called_once_with("abc123") assert source._client.http_client.request.call_count == 1 + # client.open() would build a second Spreadsheet, costing a Sheets read + source._client.open.assert_not_called() + + +def test_gsheet_spreadsheet_by_title_takes_first_exact_name_match(): + source = _make_gsheet_source_with_mocked_client("application/vnd.google-apps.spreadsheet", file_name="My Sheet") + source._client.list_spreadsheet_files.return_value = [ + {"id": "wrong-case", "name": "my sheet"}, + {"id": "first", "name": "My Sheet"}, + {"id": "second", "name": "My Sheet"}, + ] + source.set_g_sheet_name("My Sheet") + _ = source.spreadsheet + source._client.open_by_key.assert_called_once_with("first") + + +def test_gsheet_spreadsheet_by_title_ignores_inexact_matches(): + source = _make_gsheet_source_with_mocked_client("application/vnd.google-apps.spreadsheet") + source._client.list_spreadsheet_files.return_value = [{"id": "abc123", "name": "my sheet"}] + source.set_g_sheet_name("My Sheet") + with pytest.raises(SpreadsheetNotFoundError): + _ = source.spreadsheet + source._client.open_by_key.assert_not_called() def test_gsheet_spreadsheet_by_title_not_found(): source = _make_gsheet_source_with_mocked_client("application/vnd.google-apps.spreadsheet") - source._client.open.side_effect = gspread.SpreadsheetNotFound() + source._client.list_spreadsheet_files.return_value = [] source.set_g_sheet_name("No Such Sheet") with pytest.raises(SpreadsheetNotFoundError) as excinfo: _ = source.spreadsheet From 21feea558e9538065068b921e81a904ed0a06552 Mon Sep 17 00:00:00 2001 From: Hamish Downer Date: Mon, 19 Oct 2026 10:24:36 +0100 Subject: [PATCH 5/6] perf: delete old output tabs in one Sheets request Send a single batch_update with a deleteSheet request per tab, instead of one del_worksheet() call each. The batch is atomic, so a failed deletion leaves every tab in place. Co-Authored-By: Claude Opus 5.5 --- docs/agent/974-reduce-sheets-reads.md | 7 ++++ src/sortition_algorithms/adapters.py | 13 ++++--- tests/test_adapters.py | 53 +++++++++++++++++++++++---- 3 files changed, 61 insertions(+), 12 deletions(-) diff --git a/docs/agent/974-reduce-sheets-reads.md b/docs/agent/974-reduce-sheets-reads.md index bc96c34..59cddef 100644 --- a/docs/agent/974-reduce-sheets-reads.md +++ b/docs/agent/974-reduce-sheets-reads.md @@ -294,6 +294,13 @@ cheap to do: that means writing raw API requests instead of using gspread helpers. Not recommended. +As implemented: only the batched delete. It is atomic, so if one tab can't be +deleted (for example, someone deleted it by hand after the tab list was +fetched) none are, where before the tabs ahead of it in the list were deleted. +`highlight_dupes` is left alone: after Option B, `_get_tab()` finds the +remaining tab in the cache, which `write_remaining` added it to, so keeping the +`Worksheet` would save nothing. + ## Outside this library The service could avoid repeat work itself, for example by not re-reading a diff --git a/src/sortition_algorithms/adapters.py b/src/sortition_algorithms/adapters.py index 31d0351..c389707 100644 --- a/src/sortition_algorithms/adapters.py +++ b/src/sortition_algorithms/adapters.py @@ -870,11 +870,14 @@ def delete_old_output_tabs(self, dry_run: bool = False) -> list[str]: if self.tab_namer.matches_stubs(tab.title): tabs_to_delete.append(tab) - deleted_names: list[str] = [] - for tab in tabs_to_delete: - deleted_names.append(tab.title) - if not dry_run: - self.spreadsheet.del_worksheet(tab) + deleted_names = [tab.title for tab in tabs_to_delete] + if tabs_to_delete and not dry_run: + # one request for all the tabs, rather than one del_worksheet() each. + # The batch is atomic, so if one deletion fails none are made. + self.spreadsheet.batch_update({ + "requests": [{"deleteSheet": {"sheetId": tab.id}} for tab in tabs_to_delete] + }) + for tab in tabs_to_delete: del all_tabs[tab.title] return deleted_names diff --git a/tests/test_adapters.py b/tests/test_adapters.py index 5317527..21c1114 100644 --- a/tests/test_adapters.py +++ b/tests/test_adapters.py @@ -1032,9 +1032,13 @@ def _make_gsheet_source_with_tabs(tabs: dict[str, list[list[str]]]) -> tuple[GSh spreadsheet = source._client.open_by_key.return_value spreadsheet.client = MagicMock(spec=gspread.http_client.HTTPClient) + # Sheets keeps a tab's id when other tabs are deleted, so give each title a fixed id + sheet_ids: dict[str, int] = {} + def fake_properties(title: str) -> dict[str, Any]: - sheet_id = list(tabs).index(title) - return {"sheetId": sheet_id, "title": title, "index": sheet_id, "gridProperties": {"rowCount": 100}} + sheet_id = sheet_ids.setdefault(title, len(sheet_ids)) + index = list(tabs).index(title) + return {"sheetId": sheet_id, "title": title, "index": index, "gridProperties": {"rowCount": 100}} def fake_metadata(params: dict[str, str] | None = None) -> dict[str, Any]: return {"sheets": [{"properties": fake_properties(title)} for title in tabs]} @@ -1047,13 +1051,15 @@ def fake_add_worksheet(title: str, rows: int, cols: int) -> gspread.Worksheet: tabs[title] = [] return gspread.Worksheet(spreadsheet, fake_properties(title), spreadsheet.id, spreadsheet.client) - def fake_del_worksheet(worksheet: gspread.Worksheet) -> None: - del tabs[worksheet.title] + def fake_batch_update(body: dict[str, Any]) -> None: + titles_by_id = {sheet_id: title for title, sheet_id in sheet_ids.items()} + for request in body["requests"]: + del tabs[titles_by_id[request["deleteSheet"]["sheetId"]]] spreadsheet.fetch_sheet_metadata.side_effect = fake_metadata spreadsheet.client.values_get.side_effect = fake_values_get spreadsheet.add_worksheet.side_effect = fake_add_worksheet - spreadsheet.del_worksheet.side_effect = fake_del_worksheet + spreadsheet.batch_update.side_effect = fake_batch_update return source, spreadsheet @@ -1137,6 +1143,36 @@ def test_gsheet_writing_after_fast_selection_uses_cached_tab_list(): assert spreadsheet.fetch_sheet_metadata.call_count == 1 +def test_gsheet_highlight_dupes_formats_rows_on_remaining_tab(): + source, spreadsheet = _make_gsheet_source_with_tabs(_input_tabs()) + source.write_selected([["id"], ["p1"]], RunReport()) + source.write_remaining([["id"], ["p3"], ["p4"]], RunReport()) + spreadsheet.client.batch_update.reset_mock() + source.highlight_dupes([1, 2]) + # the remaining tab is in the cached tab list, so no read is needed + assert spreadsheet.fetch_sheet_metadata.call_count == 1 + # one format request, on the remaining tab, covering rows 2 and 3 + spreadsheet.client.batch_update.assert_called_once() + requests = spreadsheet.client.batch_update.call_args.args[1]["requests"] + remaining_tab_id = source._get_tab("Remaining - output - 0").id + assert [request["repeatCell"]["range"]["sheetId"] for request in requests] == [remaining_tab_id] * 2 + assert [request["repeatCell"]["range"]["startRowIndex"] for request in requests] == [1, 2] + + +def test_gsheet_highlight_dupes_before_writing_output_fails(): + source, _ = _make_gsheet_source_with_tabs(_input_tabs()) + with pytest.raises(SelectionError) as excinfo: + source.highlight_dupes([1]) + assert excinfo.value.error_code == "logic_error_tab_suffix" + + +def test_gsheet_highlight_dupes_with_no_dupes_does_nothing(): + source, spreadsheet = _make_gsheet_source_with_tabs(_input_tabs()) + source.highlight_dupes([]) + spreadsheet.fetch_sheet_metadata.assert_not_called() + spreadsheet.client.batch_update.assert_not_called() + + def test_gsheet_writing_after_slow_selection_refetches_tab_list(): tabs = _input_tabs() source, spreadsheet = _make_gsheet_source_with_tabs(tabs) @@ -1172,7 +1208,10 @@ def test_gsheet_delete_old_output_tabs_updates_cached_tab_list(): deleted = source.delete_old_output_tabs() assert deleted == ["Original Selected - output - 0", "Remaining - output - 0"] assert source._get_tab_titles() == ["Categories", "Respondents", "Selected"] - assert spreadsheet.del_worksheet.call_count == 2 + # one request deletes both tabs + spreadsheet.batch_update.assert_called_once() + spreadsheet.del_worksheet.assert_not_called() + assert list(tabs) == ["Categories", "Respondents", "Selected"] assert spreadsheet.fetch_sheet_metadata.call_count == 1 @@ -1182,7 +1221,7 @@ def test_gsheet_delete_old_output_tabs_dry_run_leaves_cached_tab_list_alone(): source, spreadsheet = _make_gsheet_source_with_tabs(tabs) assert source.delete_old_output_tabs(dry_run=True) == ["Remaining - output - 0"] assert "Remaining - output - 0" in source._get_tab_titles() - spreadsheet.del_worksheet.assert_not_called() + spreadsheet.batch_update.assert_not_called() def test_gsheet_delete_old_output_tabs_refetches_stale_tab_list(): From 02709cda4c4e0dc3adc14752d334c0c43ce8c380 Mon Sep 17 00:00:00 2001 From: Hamish Downer Date: Mon, 19 Oct 2026 10:51:32 +0100 Subject: [PATCH 6/6] remove tests that don't really add anything --- tests/test_gsheet.py | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/tests/test_gsheet.py b/tests/test_gsheet.py index b2904c0..06275fe 100644 --- a/tests/test_gsheet.py +++ b/tests/test_gsheet.py @@ -19,7 +19,6 @@ import requests from gspread.urls import DRIVE_FILES_API_V3_URL -import sortition_algorithms from sortition_algorithms import gsheet from sortition_algorithms.errors import ( NotNativeGoogleSheetError, @@ -441,14 +440,3 @@ def test_gsheet_summary_equality_ignores_fetched_at(): later = replace(summary, fetched_at=summary.fetched_at + timedelta(minutes=1)) assert later == summary assert replace(summary, can_edit=False) != summary - - -def test_gsheet_summary_is_frozen(): - summary = inspect_gsheet(_make_client(_drive_body()), URL) - with pytest.raises(AttributeError): - summary.can_edit = False # type: ignore[misc] - - -def test_inspect_gsheet_is_exported_from_package(): - assert sortition_algorithms.inspect_gsheet is inspect_gsheet - assert sortition_algorithms.GSheetSummary is GSheetSummary