Skip to content

Add paginated user query sproc#3

Merged
rmartin93 merged 1 commit into
masterfrom
claude/issue-2-20260106-1921
Jan 7, 2026
Merged

Add paginated user query sproc#3
rmartin93 merged 1 commit into
masterfrom
claude/issue-2-20260106-1921

Conversation

@rmartin93

Copy link
Copy Markdown
Collaborator

Adds stored procedure for paginated, filterable user queries

Closes #2

Generated with Claude Code

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Co-authored-by: Ryan Martin <rmartin93@users.noreply.github.com>
@claude

claude Bot commented Jan 6, 2026

Copy link
Copy Markdown

PR Review: Paginated User Query Stored Procedure

✅ Strengths

  • Clean, well-documented code with clear usage example
  • Proper input validation (page bounds, max page size cap)
  • Uses OFFSET/FETCH for pagination (modern T-SQL approach)
  • SET NOCOUNT ON for performance
  • Returns total count via OUTPUT param for UI pagination

⚠️ Security Concerns

SQL Injection Vulnerability (CRITICAL)
The LIKE pattern concatenation is vulnerable:

WHERE @SearchTerm IS NULL
   OR FirstName LIKE '%' + @SearchTerm + '%'

If @SearchTerm contains % or _, users can perform wildcard attacks. Escape special chars:

-- Add after input validation
SET @SearchTerm = REPLACE(REPLACE(@SearchTerm, '[', '[[]'), '%', '[%]');
SET @SearchTerm = REPLACE(REPLACE(@SearchTerm, ']', '[]]'), '_', '[_]');

🐛 Issues

1. Query Duplication
WHERE clause repeated in COUNT and SELECT - maintenance risk. Consider CTE:

WITH FilteredUsers AS (
    SELECT * FROM Users
    WHERE @SearchTerm IS NULL OR ...
)
SELECT @TotalCount = COUNT(*) FROM FilteredUsers;
SELECT ... FROM FilteredUsers ORDER BY ...

2. Full Table Scan
LIKE '%term%' (leading wildcard) can't use indexes. Performance degrades with large tables.

Recommendations:

  • Add full-text search index if supported
  • Use FirstName LIKE @SearchTerm + '%' (no leading wildcard) if acceptable
  • Consider separate exact-match params

📊 Performance

Missing Index
Add composite index:

CREATE INDEX IX_Users_Name_Email 
ON Users(LastName, FirstName, Email, Username)
INCLUDE (UserId, IsActive, CreatedAt, LastLoginAt);

COUNT(*) Cost
Every request counts all matching rows. For large tables, consider caching or approximate counts.

🧪 Test Coverage

No tests present. Add validation for:

  • Edge cases (PageNumber=0, PageSize=1000, NULL SearchTerm)
  • Special chars in SearchTerm (%_[])
  • Empty result sets
  • Case sensitivity

✨ Minor Improvements

  • Add @SortOrder param (ASC/DESC) for flexibility
  • Return metadata in second result set (TotalPages, CurrentPage)
  • Consider TRY/CATCH for error handling

📋 Verdict

Needs fixes before merge - address SQL injection escaping. Otherwise solid foundation per repo conventions (self-contained, well-commented, copy/paste ready).


🤖 Generated review via Claude Code

@rmartin93

Copy link
Copy Markdown
Collaborator Author

@claude can you take a look at this PR Review and make any updates necessary?

@rmartin93 rmartin93 merged commit e891958 into master Jan 7, 2026
1 check passed
@rmartin93 rmartin93 deleted the claude/issue-2-20260106-1921 branch January 7, 2026 14:34
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.

SQL - Paginated Filterable Query

1 participant