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

[jcscottiii]

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):

// 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()):

// 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<SpecificMapper>

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.