GoogleChrome / GoogleChrome/webstatus.dev
refactor(gcpspanner): standardize entityLister mappers, cursor validation, and RowIterator lifecycle
- Dominant language
- Go
- Stars
- 254
- Forks
- 62
- Avg merge
- 1d 10h
- Merged PRs (30d)
- 64
Description
### Summary of Maintenance & Architectural Follow-ups
During the investigation and review for Spanner latency optimizations ([PR #2683](https://github.com/GoogleChrome/webstatus.dev/pull/2683), [b/542984814](http://b/542984814)), several systematic legacy patterns were identified across `lib/gcpspanner/` for follow-up refactoring:
---
### 1. Migrate Timeseries & Metric Listing to `entityLister` / Mapper Pattern
* **Context:** The generic `entityLister` / `listableEntityMapper` in `client.go` is used across saved searches and notification channels, but older timeseries endpoints (`lib/gcpspanner/wpt_run_feature_metric.go` and `lib/gcpspanner/chromium_daily_usage_stat.go`) pre-date this pattern and implement custom pagination loops.
* **Goal:**
* Define typed request structs (`ListChromeDailyUsageStatsRequest`, `ListSingleFeatureWPTMetricsRequest`, `ListAggregatedWPTMetricsRequest`).
* Implement `listableEntityMapper` for timeseries entities, accommodating pre-query `WebFeatureID` resolution and `WPTMetricView` column projection.
---
### 2. Standardize `decode*Cursor` Decoders & Error Propagation
* **Context:**
* `decodeInputFeatureResultCursor` performs field validation (`offsetCursor.Offset < 0`), whereas other decoders (e.g. `decodeWPTRunCursor`, `decodeChromeDailyUsageCursor`, `decodeBrowserFeatureCountCursor`) rely solely on generic JSON parsing without zero-date / zero-timestamp checks.
* In several legacy callers (e.g. `baseline_status_count.go:84`, `browser_feature_count.go:57`), cursor errors are wrapped in `errors.Join(ErrInternalQueryFailure, err)`, masking client 400 errors as server 500s. In some listable entity mappers, cursor errors silently fall back to page 0.
* **Goal:**
* Add defensive struct validation across all 9 `decode*Cursor` decoders.
* Ensure `ErrInvalidCursorFormat` propagates cleanly so `spanneradapters` consistently maps invalid client tokens to HTTP `400 Bad Request`.
---
### 3. Spanner `RowIterator` Lifecycle Audit in Legacy Transaction Helpers
* **Context:** Several legacy transaction routines (e.g. in `create_user_saved_search.go`, `saved_search_subscription.go`, `saved_search_user_roles.go`, `user_search_bookmarks.go`) invoke single-row queries via `txn.Query(ctx, stmt).Next()` without capturing the `*spanner.RowIterator` and calling `defer it.Stop()`.
* **Goal:** Audit and update all single-row query call-sites to ensure `defer it.Stop()` is always invoked to prevent session leaks under load.
---
### 4. Parameterize Cursor Offsets in `MissingOneImplFeatureList`
* **Context:** `missing_one_implementation_feature_list.go` injects `{{ if .Offset }} OFFSET {{ .Offset }} {{ end }}` into template text rather than binding `@offset` as a query parameter, and lacks `Offset >= 0` cursor validation.
* **Goal:** Bind `@offset` as a standard Spanner query parameter and validate non-negative offset in the cursor decoder.
Contributor guide
Research direction
Start by reading lib/gcpspanner/client.go and compare its entityLister/listableEntityMapper pattern with lib/gcpspanner/wpt_run_feature_metric.go and chromium_daily_usage_stat.go. Then inspect the named cursor decoders, legacy transaction helpers, and missing_one_implementation_feature_list.go. Done means the listed mappers, cursor validation and error propagation, iterator cleanup, and offset parameterization are standardized without masking invalid cursors.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go, google-cloud
- Domain
- backend, databases
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 38/100