GoogleChrome / GoogleChrome/webstatus.dev
Refactor lib/gcpspanner/client.go point-lookups from spanner.Statement to native Key-Set Reads (spanner.Key / Columns() []string) without SQL parsing overhead or runtime type casting
- Dominant language
- Go
- Stars
- 254
- Forks
- 62
- Avg merge
- 1d 10h
- Merged PRs (30d)
- 64
Description
### Context & Problem
Currently in `lib/gcpspanner/client.go`, our generic mapper interfaces (`readOneMapper`, `idRetrievalMapper`, `deleteByKeyMapper`) and runners (`entityReader`, `entityWriterWithIDRetrieval`) rely on constructing `spanner.Statement` (`spanner.NewStatement("SELECT ... WHERE Key = @key")` + `map[string]any`) even when retrieving a single row or ID by key.
- This requires SQL string parsing, query planning overhead, and parameter map allocations for simple point lookups.
- In `entityRemover.removeWithTransaction`, before deleting an entity by its `spanner.Key` (`DeleteKey(Key) spanner.Key`), it verifies existence by running a SQL `SelectOne(key)` statement (`txn.Query`).
- When writing SQL query strings and `map[string]any{"id": id}` parameters across 100+ files, `goconst` flags repeated map keys or column names unless `lib/gcpspanner` is excluded.
### Proposed Solution (Static Composed Interfaces & Native Key-Set Reads)
We propose refactoring point-lookup and exact key-set interfaces (`readOneMapper`, `idRetrievalMapper`, `readAllMapper`) to return `spanner.Key` / `spanner.KeySet` and `Columns() []string` directly, utilizing Spanner's native `txn.ReadRow`, `txn.ReadRowUsingIndex`, and `txn.Read` APIs.
To adhere strictly to Go generics and project architecture (`skills/webstatus-backend/SKILL.md`), all mappers and runners will use **statically composed generic interfaces** so the compiler enforces contracts at build time without any runtime type assertions or casting (`any(mapper).(...)`).
### Before vs. After Examples
#### Before (`SelectOne` generating SQL `spanner.Statement`):
```go
// Mapper interface in client.go
type readOneMapper[Key comparable] interface {
SelectOne(Key) spanner.Statement
}
// Mapper implementation in saved_searches.go
func (m savedSearchMapper) Table() string { return savedSearchesTable }
func (m savedSearchMapper) SelectOne(id string) spanner.Statement {
stmt := spanner.NewStatement(
`SELECT ID, Name, Description, Query, Scope, AuthorID, CreatedAt, UpdatedAt
FROM SavedSearches WHERE ID = @id`,
)
stmt.Params["id"] = id
return stmt
}
// Runner in client.go
func (c *entityReader[M, ExternalStruct, SpannerStruct, Key]) readRowByKeyWithTransaction(
ctx context.Context, key Key, txn transaction,
) (*SpannerStruct, error) {
var mapper M
stmt := mapper.SelectOne(key)
it := txn.Query(ctx, stmt)
defer it.Stop()
row, err := it.Next()
...
}
```
#### After (Static Composed Interfaces, `spanner.Key`, and `Columns()`):
```go
// 1. Updated Transaction Interface in client.go
type transaction interface {
Query(ctx context.Context, statement spanner.Statement) *spanner.RowIterator
Read(ctx context.Context, table string, keys spanner.KeySet, columns []string) *spanner.RowIterator
ReadRow(ctx context.Context, table string, key spanner.Key, columns []string) (*spanner.Row, error)
ReadRowUsingIndex(ctx context.Context, table, index string, key spanner.Key, columns []string) (*spanner.Row, error)
}
// 2. Static Composed Interfaces (No runtime type casting allowed)
type baseMapper interface {
Table() string
Columns() []string
}
type readOneMapper[Key comparable] interface {
baseMapper
Key(Key) spanner.Key
}
// For lookups utilizing a secondary index:
type indexReadOneMapper[Key comparable] interface {
readOneMapper[Key]
Index() string
}
type idRetrievalMapper[Key comparable] interface {
baseMapper
IDKey(Key) spanner.Key
IDColumn() string
}
// For ID lookups utilizing a secondary index:
type indexIDRetrievalMapper[Key comparable] interface {
idRetrievalMapper[Key]
IDIndex() string
}
// 3. Mapper implementation in saved_searches.go
func (m savedSearchMapper) Table() string { return savedSearchesTable }
func (m savedSearchMapper) Columns() []string {
return []string{"ID", "Name", "Description", "Query", "Scope", "AuthorID", "CreatedAt", "UpdatedAt"}
}
func (m savedSearchMapper) Key(id string) spanner.Key {
return spanner.Key{id}
}
// 4. Statically Constrained Runner for Direct Primary Key Reads (entityReader)
func (c *entityReader[M, ExternalStruct, SpannerStruct, Key]) readRowByKeyWithTransaction(
ctx context.Context, key Key, txn transaction,
) (*SpannerStruct, error) {
var mapper M
spannerKey := mapper.Key(key)
row, err := txn.ReadRow(ctx, mapper.Table(), spannerKey, mapper.Columns())
if errors.Is(err, spanner.ErrRowNotFound) {
return nil, errors.Join(ErrQueryReturnedNoResults, err)
}
if err != nil {
return nil, errors.Join(ErrInternalQueryFailure, err)
}
existing := new(SpannerStruct)
if err := row.ToStruct(existing); err != nil {
return nil, errors.Join(ErrInternalQueryFailure, err)
}
return existing, nil
}
// 5. Statically Constrained Runner for Secondary Index Lookups (indexEntityReader)
func (c *indexEntityReader[M, ExternalStruct, SpannerStruct, Key]) readRowByIndexWithTransaction(
ctx context.Context, key Key, txn transaction,
) (*SpannerStruct, error) {
var mapper M
spannerKey := mapper.Key(key)
row, err := txn.ReadRowUsingIndex(ctx, mapper.Table(), mapper.Index(), spannerKey, mapper.Columns())
if errors.Is(err, spanner.ErrRowNotFound) {
return nil, errors.Join(ErrQueryReturnedNoResults, err)
}
if err != nil {
return nil, errors.Join(ErrInternalQueryFailure, err)
}
existing := new(SpannerStruct)
if err := row.ToStruct(existing); err != nil {
return nil, errors.Join(ErrInternalQueryFailure, err)
}
return existing, nil
}
// 6. Zero-SQL ID Fetching Runners (Primary Key vs Secondary Index)
func (c *entityWriterWithIDRetrieval[M, ExternalStruct, SpannerStruct, Key, ID]) getIDByKeyWithTransaction(
ctx context.Context, key Key, txn transaction,
) (*ID, error) {
var mapper M
idKey := mapper.IDKey(key)
idCol := []string{mapper.IDColumn()}
row, err := txn.ReadRow(ctx, mapper.Table(), idKey, idCol)
if errors.Is(err, spanner.ErrRowNotFound) {
return nil, errors.Join(ErrQueryReturnedNoResults, err)
}
if err != nil {
return nil, errors.Join(ErrInternalQueryFailure, err)
}
var id ID
if err := row.Column(0, &id); err != nil {
return nil, errors.Join(ErrInternalQueryFailure, err)
}
return &id, nil
}
// For mappers like WebFeatures where ID lookup requires a secondary index (e.g. WebFeaturesByFeatureID):
func (c *indexEntityWriterWithIDRetrieval[M, ExternalStruct, SpannerStruct, Key, ID]) getIDByKeyWithTransaction(
ctx context.Context, key Key, txn transaction,
) (*ID, error) {
var mapper M
idKey := mapper.IDKey(key)
idCol := []string{mapper.IDColumn()}
row, err := txn.ReadRowUsingIndex(ctx, mapper.Table(), mapper.IDIndex(), idKey, idCol)
if errors.Is(err, spanner.ErrRowNotFound) {
return nil, errors.Join(ErrQueryReturnedNoResults, err)
}
if err != nil {
return nil, errors.Join(ErrInternalQueryFailure, err)
}
var id ID
if err := row.Column(0, &id); err != nil {
return nil, errors.Join(ErrInternalQueryFailure, err)
}
return &id, nil
}
```
### Note on Secondary Indexes & External Key Lookups
For tables where callers look up rows by external business keys rather than Spanner Primary Keys (`ID`):
- If a secondary index exists on the external key (e.g. `WebFeaturesByFeatureID` for `WebFeatures.FeatureKey`), `indexReadOneMapper` (`ReadRowUsingIndex`) works cleanly.
- If a table currently lacks a secondary index on its external lookup key (e.g. `WebDXGroups.GroupKey`), the migration must either add a unique secondary index (`CREATE UNIQUE INDEX WebDXGroupsByGroupKey ON WebDXGroups(GroupKey)`) or retain a `spanner.Statement` mapper for non-indexed lookups.
### Impact on `goconst` & Revisiting the `.golangci.yaml` Exclusion
- Moving from `spanner.Statement` (`SELECT ID, Name, CreatedAt ...`) to `Columns() []string` (`[]string{"ID", "Name", "Description", "CreatedAt", "UpdatedAt"}`) turns each column name into an individual string literal across 40+ mappers.
- Because common column names (`"ID"`, `"Name"`, `"Description"`, `"CreatedAt"`, `"UpdatedAt"`, `"UserID"`) repeat across almost every table in `lib/gcpspanner`, if `goconst` checked `lib/gcpspanner/*.go`, it would flag every repeated column name string across files.
- Currently, `lib/gcpspanner/.*\.go` is excluded from `goconst` inside `.golangci.yaml` (`path: lib/gcpspanner/.*\.go linters: [goconst]`), allowing developers to write `Columns() []string{"ID", "Name", "CreatedAt"}` cleanly in each mapper without global constant indirection.
- **Action Item (Revisit `goconst` Exclusion):** Once this refactoring issue (`Columns() []string` and `spanner.Key`) is fully implemented across all mappers, the team should **revisit the `goconst` exclusion for `lib/gcpspanner/.*\.go` in `.golangci.yaml`** (and the corresponding comment in `.golangci.yaml`) to formally evaluate whether to keep the path exclusion or transition to shared column constants across a `lib/gcpspanner/columns.go` file (`const colID = "ID"`, etc.).
### 7. Step-by-Step Incremental Migration Strategy (Zero Compilation Breakages)
To prevent breaking 32 `SelectOne` implementations across 27 files in `lib/gcpspanner/` in a single monolithic commit, this refactor **must be implemented in three incremental phases**:
#### Phase 1: Core Client Infrastructure (`lib/gcpspanner/client.go`)
1. Expand `transaction` interface (`client.go:810`) to include `ReadRow`, `ReadRowUsingIndex`, and `Read`.
*(Note: Both `*spanner.ReadOnlyTransaction` and `*spanner.ReadWriteTransaction` natively implement these exact methods — no wrapper structs needed).*
2. Define new static composed interfaces alongside existing ones:
- `keyReadOneMapper[Key comparable]` (`baseMapper` + `Key(Key) spanner.Key`)
- `indexKeyReadOneMapper[Key comparable]` (`keyReadOneMapper[Key]` + `Index() string`)
- `keyIDRetrievalMapper[Key comparable]` (`baseMapper` + `IDKey(Key) spanner.Key` + `IDColumn() string`)
- `indexKeyIDRetrievalMapper[Key comparable]` (`keyIDRetrievalMapper[Key]` + `IDIndex() string`)
- `keySetReadAllMapper` (`baseMapper`) / `keySetReadByKeysMapper[KeysContainer comparable]` (`baseMapper` + `KeySet(KeysContainer) spanner.KeySet`)
3. Update or add corresponding runners (`readRowByKeyWithTransaction`, `getIDByKeyWithTransaction`, `readInspectMutateWithTransaction`, `removeWithTransaction`, `upsertAndGetID`, `upsertWithTransaction`, `updateWithTransaction`).
- *Crucial Rule for Upsert/Update Helpers:* Where `SelectOne` was previously used to check row existence before an insert/update in `upsertAndGetID` or `upsertWithTransaction`, ensure that mappers looking up by an external key via secondary index (`indexKeyReadOneMapper` or `indexKeyIDRetrievalMapper`) call `txn.ReadRowUsingIndex`, while primary key mappers call `txn.ReadRow`.
- For `readAllMapper` (`SelectAll()`) and `readAllByKeysMapper`, `txn.Read(ctx, mapper.Table(), spanner.AllKeys(), mapper.Columns())` and `txn.Read(ctx, mapper.Table(), mapper.KeySet(keys), mapper.Columns())` replace `SelectAll` / `SelectAllByKeys`.
#### Phase 2: File-by-File Mapper Migration across `lib/gcpspanner/*.go`
Migrate mappers file-by-file. For each mapper:
1. Add `Columns() []string` specifying exact column slices (`[]string{"ID", "Name", ...}`).
2. Replace `SelectOne(key)` with `Key(key) spanner.Key` (and `Index() string` if querying a secondary index).
3. Replace `GetID(key)` with `IDKey(key) spanner.Key` + `IDColumn() string` (and `IDIndex() string` if applicable).
4. Update the corresponding `newEntityReader[...]`, `newEntityWriter[...]`, or `newEntityMutator[...]` instantiation to use the new runner/interface constraints.
5. Verify via integration tests: `go test -v ./lib/gcpspanner/... -run Test`
#### Phase 3: Cleanup, Deprecation & Revisiting `.golangci.yaml`
1. Once all 27 files in `lib/gcpspanner/` no longer reference `SelectOne`, `GetID`, `SelectAll`, or `SelectAllByKeys`, delete these legacy methods from `client.go` along with the old `readOneMapper` and `idRetrievalMapper` interfaces.
2. Revisit `.golangci.yaml` around `lib/gcpspanner/.*\.go` (`goconst`) as noted above.
### 8. Verification & Testing Checklist
- [ ] **Integration Verification:** Run `make go-test` across `lib/gcpspanner/...`. All unit/integration tests run against a real containerized local Spanner instance (`testcontainers-go`) and will verify `txn.ReadRow` / `txn.ReadRowUsingIndex` parity automatically without updating fake query matchers.
- [ ] **Linter Check:** Run `make lint` (`make go-lint`) to verify `.golangci.yaml` rules (`goconst` exclusion on `lib/gcpspanner/.*\.go` ensures `Columns() []string{...}` literals are permitted).
- [ ] **External Consumers Check:** Verify `go test ./lib/gcpspanner/spanneradapters/...` and `go test ./backend/pkg/httpserver/...` pass cleanly, confirming `*gcpspanner.Client`'s public APIs remained 100% backward compatible.
Contributor guide
Research direction
Start with lib/gcpspanner/client.go, especially the transaction interface, mapper interfaces, and runners listed in Phase 1, then trace the 32 SelectOne implementations across lib/gcpspanner/. Migrate representative mappers and run go test -v ./lib/gcpspanner/... -run Test; completion requires all mapper and runner migrations, cleanup in .golangci.yaml, and passing integration tests.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go, google-cloud
- Domain
- backend, databases
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100