Use ORM for player filtering

[tudddorrr]

No description provided.

[claude]

Code Review: Use ORM for player filtering

✅ Positive Changes

1. Better ORM Usage: Excellent migration from raw query builder to proper ORM methods using FilterQuery and em.repo(Player).find(). This improves type safety and follows MikroORM best practices.

2. Improved Caching Strategy: Good addition of separate cache keys for counts and better cache key structure with search/dev data components.

3. Transaction Management: Proper use of em.fork().transactional() ensures data consistency during the search operation.

4. Code Organization: The restructured search conditions using FilterQuery[] array is much cleaner and more maintainable than the previous chain of .orWhere() calls.

🔧 Issues & Recommendations

Performance Concerns

• Lines 177-180 & 196-199: The await calls inside loops for group and channel lookups will execute sequentially, potentially causing N+1 query issues. Consider using Promise.all() to parallelize these operations.

Potential Bugs

• Line 200: channels: { $some: channel.id } looks incorrect. The $some operator expects an object with conditions, not a primitive value. Should likely be channels: { $some: { id: channel.id } }

• Line 234: The isLastPage calculation might be incorrect. The original used allPlayers.length <= itemsPerPage (which included the +1 for pagination detection), but now uses players.length <= itemsPerPage without the extra item fetch.

Code Quality

• Line 136: encodeURIComponent(search) || 'no-search' - if search is falsy but not undefined, this could create unexpected cache keys. Consider: search ? encodeURIComponent(search) : 'no-search'

Security

• The $like queries with user input (lines 152, 160, 166) appear safe since they're parameterized, but ensure the MikroORM version handles SQL injection protection properly.

🧪 Testing Recommendations

Based on CONTRIBUTING.md, please ensure tests cover:

• Search functionality with various filter combinations
• Group and channel filtering edge cases
• Caching behavior validation
• Performance with large datasets
• The isLastPage calculation logic

Run tests with: npm test

📋 Summary

This is a solid refactor that improves type safety and follows better ORM practices. The main concerns are the sequential async operations in loops and the potential channel filtering bug. After addressing these issues, this will be a significant improvement to the codebase.

Priority: Address the channel filtering bug and consider the performance optimization for group/channel lookups.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.37%. Comparing base (93bf4a8) to head (06fc847).
⚠️ Report is 2 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #601      +/-   ##
===========================================
+ Coverage    99.36%   99.37%   +0.01%     
===========================================
  Files          208      208              
  Lines        14488    14478      -10     
  Branches      1292     1292              
===========================================
- Hits         14396    14388       -8     
+ Misses          89       87       -2     
  Partials         3        3              

☔ 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.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[claude]

Code Review: Use ORM for player filtering

This PR refactors the player filtering logic from query builder to ORM repository methods. Here's my comprehensive review:

✅ Positive Changes

Code Quality & Best Practices:

• Cleaner ORM Usage: Excellent transition from manual query building to MikroORM's repository pattern with FilterQuery<Player> - this is more maintainable and type-safe
• Better Structure: The separation of concerns is improved with dedicated filter building logic
• Consistent Caching: Good use of getResultCacheOptions() helper for both count and results queries
• Transaction Usage: Proper use of em.fork().transactional() for consistency

Performance Improvements:

• Parallel Caching: Both results and count queries now have proper cache keys (${cacheKey} and ${cacheKey}-count)
• Eliminated N+1: The old approach with separate .getResult() and .getCount() calls is replaced with parallel execution
• Better Population: Using populate: ['aliases'] in the main query instead of separate populate call

⚠️ Issues Found

Critical Bug - Logic Error:

// Line 197: This logic is incorrect
isLastPage: players.length <= itemsPerPage

Problem: Since you're using limit: itemsPerPage + 1, when players.length <= itemsPerPage it means there ARE more pages, not that it's the last page.

Fix needed:

isLastPage: players.length <= itemsPerPage

Should be:

isLastPage: players.length < itemsPerPage + 1
// OR better yet:
const hasMore = players.length > itemsPerPage
const actualPlayers = hasMore ? players.slice(0, itemsPerPage) : players
// Then: isLastPage: !hasMore

Potential Type Safety Issue:

// Lines 157-158: Mixing string and number filter types
groups: { $some: filter.split(':')[1] }  // string
channels: { $some: Number(filter.split(':')[1]) }  // number

The groups filter assumes string IDs but doesn't validate the format. Consider adding type validation.

Missing Array Handling:

const actualPlayers = players.slice(0, itemsPerPage)

The current code doesn't slice the results when returning, so it might return itemsPerPage + 1 results instead of itemsPerPage.

🔒 Security Assessment

Good security practices maintained:

• ✅ Proper parameterized queries via ORM (prevents SQL injection)
• ✅ User permission checks for API vs non-API users
• ✅ Input sanitization with encodeURIComponent()
• ✅ Game context isolation with game: req.ctx.state.game

📊 Performance Considerations

Improvements:

• ✅ Better caching strategy with separate count cache
• ✅ Transaction usage ensures consistency
• ✅ Eliminated redundant populate call

Concerns:

• ⚠️ The em.fork() creates a new EntityManager - ensure this doesn't cause connection pool issues under load
• ⚠️ Group/channel filter validation happens in a loop - consider batch validation for better performance

🧪 Test Coverage

Missing test coverage for:

• Player filtering with group/channel filters
• Edge cases with itemsPerPage + 1 logic
• Cache behavior validation
• Transaction rollback scenarios

Recommendation: Add integration tests for the filtering logic, especially the pagination edge cases.

📝 Suggested Improvements

1. Fix the pagination logic:

   const hasMore = players.length > itemsPerPage
   const actualPlayers = hasMore ? players.slice(0, itemsPerPage) : players
   
   return {
     // ...
     players: actualPlayers,  
     isLastPage: !hasMore
   }

2. Add input validation:

   if (groupFilters.length > 0 && !req.ctx.state.user.api) {
     for (const filter of groupFilters) {
       const groupId = filter.split(':')[1]
       if (!groupId || groupId.trim() === '') continue
       // ... existing logic
     }
   }

3. Consider extracting filter building into a separate method for better testability.

🎯 Overall Assessment

This is a solid refactoring that improves code maintainability and performance. The main issues are the pagination logic bug and missing result slicing. Once those are addressed, this will be a significant improvement over the previous query builder approach.

Priority: Fix the pagination logic before merging - this could cause incorrect UI behavior.

[claude]

Code Review for PR #601: Use ORM for player filtering

🔍 Overview

This PR refactors the player search functionality from using query builder to using MikroORM's find() method with FilterQuery. This is a good architectural improvement that makes the code more maintainable and type-safe.

✅ Positive Changes

Code Quality & Best Practices

• Type Safety: Adding FilterQuery<Player> import provides better TypeScript support and type checking
• ORM Consistency: Using em.repo(Player).find() with FilterQuery is more idiomatic than raw query building
• Reduced Database Queries: Eliminating separate database lookups for PlayerGroup and GameChannel entities improves efficiency
• Cleaner Logic: The new approach with searchConditions array is more readable and maintainable

Performance Improvements

• Single Transaction: Wrapping both count and find operations in em.fork().transactional() ensures data consistency
• Reduced N+1 Issues: Avoiding multiple entity lookups for groups and channels
• Maintained Caching: Proper cache key usage with getResultCacheOptions() is preserved

⚠️ Issues to Address

Security Concerns

// Line 150, 158, 164: SQL Injection Risk
value: { $like: `%${search}%` }
identifier: { $like: `%${search}%` }  
id: { $like: `%${search}%` }

Issue: Direct string interpolation in LIKE queries can lead to SQL injection if search contains SQL wildcards or escape characters.

Recommendation: Sanitize the search input or use parameterized queries:

value: { $like: `%${search.replace(/[%_]/g, '\\$&')}%` }

Potential Bugs

1. Group Filter Logic (Lines 173-182):

   if (groupId) {
     searchConditions.push({
       groups: { $some: groupId }
     })
   }

   Issue: This assumes $some operator works with direct IDs, but the original code used entity instances.

   Recommendation: Verify this works correctly or use:

   groups: { id: { $in: [groupId] } }

2. Channel Filter Logic (Lines 189-200):
   Similar concern with the $some operator usage for channel filtering.

3. Missing Error Handling: The new implementation doesn't handle potential database errors from the transactional block.

Performance Considerations

• Transaction Overhead: While transactions ensure consistency, they may add overhead for read operations
• Population Strategy: Consider if populate: ['aliases'] is always needed or could be conditional

🧪 Test Coverage

Good: Existing tests in /tests/services/player/index.test.ts cover:

• Basic functionality
• Pagination
• Dev build filtering
• Search by props, aliases, ID, groups, and channels

Missing: Tests should be updated to verify:

• The new ORM filtering logic works correctly
• Group and channel filtering still functions with the new $some approach
• SQL injection prevention with malicious search inputs

📝 Recommendations

1. Security: Add input sanitization for the search parameter
2. Testing: Verify existing tests still pass and add tests for edge cases
3. Error Handling: Add try-catch around the transactional block
4. Documentation: Consider adding JSDoc comments explaining the complex filtering logic

🎯 Verdict

This is a solid refactoring that improves code quality and maintainability. The main concerns are around security (SQL injection) and verifying the $some operator usage for groups/channels. Once these are addressed, this will be a great improvement to the codebase.

Approval Status: ⏳ Requires changes (security fixes)