Add db-backed implementation of RegistryService

[blkt]

This change adds a db-backed implementation of the RegistryService interface.

There are multiple public interfaces added in this change, specifically

• internal/service which exposes the RegistryService interface with an Option type used to configure filtering and pagination of SQL statements.
• internal/service/db which exposes means to create a RegistryService backed by a database.

Testing is implemented as integration tests running containers in the same fashion as the internal/db/sqlc package. This might be revised in the future by using mocks, but (a) a mock based implementation is not going to be more succinct, and (b) does its job of covering most of the new code.

The change set is pretty big, but more than 65% of it are tests.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 93.02326% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.02%. Comparing base (9d4984a) to head (f1e8a0d).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/service/db/impl.go	86.15%	10 Missing and 8 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #161      +/-   ##
==========================================
+ Coverage   64.58%   67.02%   +2.43%     
==========================================
  Files          51       53       +2     
  Lines        2725     2981     +256     
==========================================
+ Hits         1760     1998     +238     
- Misses        841      851      +10     
- Partials      124      132       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[blkt]

I can split this PR by function if it's too big.

[ChrisJBurns]

@claude Using the code reviewer, golang code writer and unit test writer agents, could you provide a review on this PR. Please keep your summary as concise as possible and only mention critical improvements that should be considered now and near-term/immediately after this PR is merged.

[claude]

Claude finished @ChrisJBurns's task —— View job

---

PR Review Complete

I've reviewed this PR using the code-reviewer, golang-code-writer, and unit-test-writer agents. Overall, this is a solid implementation of the database-backed RegistryService with comprehensive integration tests (94.53% coverage). However, there are critical safety issues that should be addressed before or immediately after merge.

---

🔴 Critical Issues (Address Before Merge)

1. Nil Pointer Dereference Risk - types.go:116-125, 153-157

Impact: Production crashes if database returns NULL for optional fields.

// Current code blindly dereferences:
Description: *dbServer.Description,  // Will panic if NULL
Title:       *dbServer.Title,

Fix: Add nil checks before dereferencing:

var description string
if dbServer.Description != nil {
    description = *dbServer.Description
}
server.Description = description

Testing: Add test case with NULL optional fields (as noted by reviewer at line 399).

2. Array Bounds Check Missing - impl.go:211

Impact: Runtime panic if query returns empty result.

// Current code:
return res[0], nil  // Panics if len(res) == 0

Fix: Add bounds check:

if len(res) == 0 {
    return nil, service.ErrServerNotFound
}
return res[0], nil

3. Missing Error Wrapping - impl.go:111, 115, 223

Impact: Harder to debug issues in production; aligns with reviewer feedback.

Fix: Wrap errors with context:

return nil, fmt.Errorf("invalid cursor format: %w", err)  // Line 111
return nil, fmt.Errorf("invalid cursor timestamp: %w", err)  // Line 115
return nil, fmt.Errorf("beginning transaction: %w", err)  // Line 223

---

🟡 Near-Term Issues (Fix Soon After Merge)

4. Connection Pool Resource Leak - impl.go:48

The connection pool created in WithConnectionString is never closed, causing resource leaks.

Fix:

• Add Close() method to dbService
• Document pool lifecycle management
• Consider accepting context parameter for pool creation timeout

5. Transaction Rollback Error Handling - impl.go:225

As noted by reviewer, rollback errors are ignored. While typically safe, it hides potential issues.

Fix: Use proper rollback pattern:

defer func() {
    if rollbackErr := tx.Rollback(ctx); rollbackErr != nil &&
        !errors.Is(rollbackErr, pgx.ErrTxClosed) {
        // Log the rollback error
    }
}()

6. Add Tests for GetRegistry - impl.go:90-94

Currently returns ErrNotImplemented but has no test coverage. Should verify expected behavior.

7. Variable Shadowing - types.go:186, 198

Using string as variable name shadows built-in type. Use str or value instead.

---

📊 Architecture Notes (For Consideration)

1. Connection Pool Config (reviewer comment at line 48): Consider adding pool configuration options (max connections, timeouts) in a future PR.

2. Documentation (reviewer comment at line 214): The sharedListServers function would benefit from godoc explaining its transaction pattern and helper abstraction.

3. Code Structure: The helper struct is a pragmatic workaround for sqlc limitations - this is acceptable but could be renamed to something more descriptive like serverRow.

---

Summary

The implementation is well-structured with excellent test coverage, but critical safety issues with nil pointer dereferences and bounds checking must be fixed to prevent production crashes. The error wrapping improvements align with existing reviewer feedback and should be addressed for consistency.

After addressing the critical issues, this will be a solid addition to the codebase. The integration test approach is appropriate given the database interaction complexity.

Recommendation: Fix critical issues #1-3 before merge, create tickets for near-term issues #4-7 to address in the next sprint.

--- | Branch: feat/add-db-based-service-layer

[blkt]

@claude Using the code reviewer, golang code writer and unit test writer agents, could you provide a review on this PR. Please keep your summary succinct and only mention critical improvements that should be considered now and near-term/immediately after this PR is merged.

[claude]

Claude finished @blkt's task —— View job

---

PR Review Complete

I've reviewed this PR using the code-reviewer, unit-test-writer, and golang-code-writer agents. Overall, this is a well-structured implementation with strong test coverage (93.02%). However, there are critical safety issues that must be addressed.

---

🔴 Critical Issues (Must Fix Before Merge)

1. Resource Leak - Connection Pool Never Closed (internal/service/db/impl.go:48)

The dbService creates a connection pool but provides no way to close it, causing resource leaks on shutdown.

Fix: Add a Close() method:

func (s *dbService) Close() error {
    s.pool.Close()
    return nil
}

2. Empty Cursor Panic (internal/service/db/impl.go:96-103)

Empty cursor string will cause base64 decode/time parse to fail unexpectedly.

Fix: Check for empty cursor before decoding:

if options.Cursor == "" {
    nextTime = time.Time{} // or appropriate default
} else {
    decoded, err := base64.StdEncoding.DecodeString(options.Cursor)
    // ... rest of logic
}

3. Array Bounds Check Missing (internal/service/db/impl.go:213)

Accessing res[0] without verifying slice length will panic if query returns empty results despite the comment at line 206-208.

Fix: Add explicit bounds check:

if len(res) == 0 {
    return nil, service.ErrServerNotFound
}
return res[0], nil

4. Missing NULL Field Tests (internal/service/db/impl_test.go:399)

Test data always provides non-NULL optional fields, but production databases will have NULLs. The code uses ptr.ToString() which safely handles nils (returns empty strings), but this behavior is untested.

Fix: Add test cases with NULL values for Description, Title, Website, and Repository fields to verify safe handling.

---

🟡 Near-Term Issues (Address Soon After Merge)

5. Missing Error Wrapping (internal/service/db/impl.go:98, 102, 249)

Cursor parsing and transaction errors lack context for debugging.

Fix: Add error wrapping:

return nil, fmt.Errorf("invalid cursor format: %w", err)  // Line 98
return nil, fmt.Errorf("invalid cursor timestamp: %w", err)  // Line 102
return nil, fmt.Errorf("failed to begin transaction: %w", err)  // Line 249

6. Variable Shadowing (internal/service/db/types.go:199, 211)

Using strings as parameter name shadows the built-in package.

Fix: Rename to values or items:

func toKeyValueInputs(values []string) []model.KeyValueInput
func toArguments(args []string) []model.Argument

7. Transaction Not Committed (internal/service/db/impl.go:244-257)

Read-only transaction is rolled back but never committed. While reads work without commit, explicitly committing is cleaner.

Fix: Add commit before successful return:

err = tx.Commit(ctx)
if err != nil {
    return nil, fmt.Errorf("failed to commit transaction: %w", err)
}

8. Rollback Error Handling (internal/service/db/impl.go:252-256)

Rollback errors are silently ignored per the TODO comment. While typically safe, this could hide connection pool issues.

Fix: Log rollback errors when logging is available.

9. GetRegistry Test Coverage (internal/service/db/impl.go:78-82)

Method returns ErrNotImplemented but has no test verifying this behavior.

---

✅ Positive Observations

• Excellent use of sqlc for type-safe SQL (no SQL injection risk)
• Clean transaction management patterns
• Good separation of concerns with the helper bridge struct
• Comprehensive integration test coverage for happy paths
• Proper use of ptr.ToString() for safe nil handling

---

Recommendation

Critical issues #1-4 must be fixed before merge as they will cause panics/leaks in production. The near-term issues can be addressed in follow-up PRs if needed for urgency.

The helper struct pattern is a pragmatic workaround for sqlc's limitation in detecting identical column sets across queries - this is acceptable given the constraints.

---

| Branch: feat/add-db-based-service-layer