Skip to content

refactor: add getAffectedRows() helper to eliminate result extraction branching#2374

Merged
Yeraze merged 1 commit intomainfrom
refactor/get-affected-rows-helper
Mar 22, 2026
Merged

refactor: add getAffectedRows() helper to eliminate result extraction branching#2374
Yeraze merged 1 commit intomainfrom
refactor/get-affected-rows-helper

Conversation

@Yeraze
Copy link
Copy Markdown
Owner

@Yeraze Yeraze commented Mar 22, 2026

Summary

Phase 2 of the Drizzle refactor — adds BaseRepository.getAffectedRows(result) helper that normalizes affected row counts across all three database backends.

The helper

protected getAffectedRows(result: any): number {
  // SQLite: result.changes
  // PostgreSQL: result.rowCount
  // MySQL: result[0].affectedRows
}

Methods simplified

File Method Before After
misc.ts clearPacketLogs 8-line 3-way branch 1 line
misc.ts cleanupOldPacketLogs 8-line 3-way branch 1 line
auth.ts deactivateApiToken 3 typed-db paths (33 lines) Single this.db path (10 lines)
auth.ts revokeAllUserApiTokens 3 typed-db paths (33 lines) Single this.db path (10 lines)
messages.ts insertMessage 3 typed-db paths MySQL branch + shared path

Not changed (yet)

notifications.ts methods use a different pattern where MySQL results are already destructured from the tuple. These need a different approach and are deferred.

Files Changed

File Change
src/db/repositories/base.ts Add getAffectedRows() helper
src/db/repositories/misc.ts Simplify 2 methods
src/db/repositories/auth.ts Simplify 2 methods from 3-way to single path
src/db/repositories/messages.ts Simplify insertMessage result extraction

Test plan

  • 3061 tests pass, 0 failures

🤖 Generated with Claude Code

… branching

Adds BaseRepository.getAffectedRows(result) that normalizes affected row
counts across SQLite (.changes), PostgreSQL (.rowCount), and MySQL
([0].affectedRows). Replaces 3-way branching in:

- misc.ts: clearPacketLogs, cleanupOldPacketLogs
- auth.ts: deactivateApiToken, revokeAllUserApiTokens
- messages.ts: insertMessage

Auth methods also simplified from 3 typed-db paths to single this.db path.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 22, 2026

Claude finished @Yeraze's task —— View job


PR Review: getAffectedRows() Helper Implementation

Review Checklist:

  • Read and analyze changed files
  • Review helper method implementation
  • Check simplified method usage
  • Assess potential bugs and edge cases
  • Evaluate performance and security
  • Provide comprehensive feedback

Overall Assessment

This PR successfully introduces a clean abstraction for extracting affected row counts across database backends. The implementation is solid and the code reduction is meaningful (net -57 lines).

Code Quality Review

✅ Helper Method Implementation (src/db/repositories/base.ts:162-172)

The getAffectedRows() helper is well-implemented:

  • Correct database-specific logic: SQLite result.changes, PostgreSQL result.rowCount, MySQL result[0].affectedRows
  • Defensive null/undefined handling: Uses ?. and ?? 0 patterns
  • Proper type coercion: Number() wrapper ensures consistent return type
  • Good documentation: Clear JSDoc explaining the purpose and return formats

✅ Method Simplifications

misc.ts:1033, 1052 - clearPacketLogs() & cleanupOldPacketLogs():

  • Previous 8-line branching eliminated to single this.getAffectedRows(results) call
  • Clean, readable implementation

auth.ts:533, 549 - revokeApiToken() & revokeAllUserApiTokens():

  • Excellent consolidation from 3 typed database paths (33 lines each) down to unified logic (10 lines each)
  • Uses the unified this.db approach with helper extraction

messages.ts:68 - insertMessage():

  • MySQL branch retained (different upsert syntax) but result extraction unified
  • Appropriate balance of abstraction vs. dialect-specific requirements

Potential Issues & Considerations

⚠️ Type Safety Concerns

The helper accepts any parameter, which could mask type issues:

protected getAffectedRows(result: any): number {

Recommendation: Consider a union type for better type safety:

type DatabaseResult = 
  | { changes?: number } // SQLite
  | { rowCount?: number } // PostgreSQL  
  | [{ affectedRows?: number }] // MySQL

protected getAffectedRows(result: DatabaseResult): number {

⚠️ MySQL Array Destructuring

The MySQL path assumes result[0] exists. While this should always be true for mutation operations, consider defensive programming:

// Current
return Number((result as any)?.[0]?.affectedRows ?? 0);

// More defensive
return Number(Array.isArray(result) && result.length > 0 ? result[0]?.affectedRows ?? 0 : 0);

✅ Error Handling

The helper gracefully handles edge cases:

  • Undefined results → 0
  • Missing properties → 0
  • Non-numeric values → Number() coercion

Performance & Security

✅ Performance

  • Positive impact: Eliminates redundant branching logic across multiple methods
  • No overhead: Simple property access with minimal computation
  • Code size reduction: -57 lines improves maintainability

✅ Security

  • No security concerns: Pure data transformation without external inputs
  • Defensive coding: Null-safe access patterns prevent runtime errors

Test Coverage

✅ Test Status

  • All 3061 tests pass ✓
  • Existing test coverage validates the refactor works correctly
  • No additional tests needed as this is pure refactoring of internal logic

Architecture & Maintainability

✅ Excellent Abstraction

  • Single Responsibility: Helper has one clear purpose
  • DRY Principle: Eliminates code duplication effectively
  • Extensibility: New methods can easily use the helper
  • Consistency: Standardizes affected row handling across repositories

✅ Future-Proofing

  • Centralizes database result format knowledge in one place
  • Future database backend additions only need to update one method
  • Clear separation between dialect-specific operations and common patterns

Recommendations

  1. Consider type safety improvements for the helper parameter
  2. Add defensive array checking for MySQL results
  3. Perfect candidate for follow-up: Apply this pattern to notifications.ts methods mentioned in the PR description

Summary

This is a well-executed refactoring that successfully:

  • ✅ Reduces code duplication and complexity
  • ✅ Improves maintainability and consistency
  • ✅ Maintains all existing functionality (tests pass)
  • ✅ Follows established patterns in the codebase
  • ✅ Provides clear documentation and intent

Recommendation: APPROVE - This PR represents quality refactoring that makes the codebase more maintainable without introducing risks.

Job run | Branch: refactor/get-affected-rows-helper

@Yeraze Yeraze merged commit e59f36d into main Mar 22, 2026
17 checks passed
@Yeraze Yeraze mentioned this pull request Mar 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant