diff --git a/PR_DESCRIPTION_1020.md b/PR_DESCRIPTION_1020.md new file mode 100644 index 00000000..490905c8 --- /dev/null +++ b/PR_DESCRIPTION_1020.md @@ -0,0 +1,17 @@ +# PR Description: Pagination in AchievementsService (#1020) + +## Overview +This PR addresses the unbounded reads in `AchievementsService`. Methods that used to retrieve the entire sets of achievements or user progress now accept pagination parameters and perform bounded queries to avoid loading large lists into memory. Additionally, the achievement definition set is now cached and `getUserAchievementOverview` performs an aggregated query instead of retrieving arrays into memory to calculate totals. + +## Changes Made +- Added pagination parameters (`query?: PaginationQueryDto`) to `getAllAchievements`, `getAchievementsByType`, `getUserAllProgress`, and `getUserAchievements`. +- Updated these methods to return `OffsetPaginatedResponse` using `buildOffsetResponse`, and implemented `skip`/`take` logic in TypeORM queries. +- Injected `CacheManager` to cache `total_achievements` and invalidated the cache (`achievements_definitions`) on mutations (`createAchievement`, `updateAchievement`, `deactivateAchievement`). +- Refactored `getUserAchievementOverview` to replace lines 472-476 (loading all active achievements and all user achievements into arrays). It now uses `CacheManager` for `totalAchievements` and a single `createQueryBuilder` aggregation to calculate `unlockedCount`, `totalPoints`, and `totalExperience`. +- Ensured indexes exist on `userId` within the progress tracking components for optimization. + +## Acceptance Criteria +- [x] All achievement list endpoints return bounded pages. +- [x] Definition reads are served from cache between mutations. +- [x] The definitions-plus-progress path issues one query rather than two full reads. +- [x] User-scoped achievement lookups are index-backed. diff --git a/src/achievements/achievements.service.ts b/src/achievements/achievements.service.ts index aa38b049..5ff22fb5 100644 --- a/src/achievements/achievements.service.ts +++ b/src/achievements/achievements.service.ts @@ -21,6 +21,14 @@ import { AchievementLeaderboardDto, AchievementOverviewDto, } from './dto/achievement-statistics.dto'; +import { PaginationQueryDto } from '../../common/dto/pagination.dto'; +import { OffsetPaginatedResponse } from '../../common/interfaces/pagination.interface'; +import { buildOffsetResponse } from '../../common/utils/pagination.utils'; +import { CACHE_MANAGER } from '@nestjs/cache-manager'; +import { Cache } from 'cache-manager'; +import { Inject } from '@nestjs/common'; + +const ACHIEVEMENTS_CACHE_KEY = 'achievements_definitions'; @Injectable() export class AchievementsService { @@ -35,6 +43,7 @@ export class AchievementsService { private userAchievementRepository: Repository, @InjectRepository(AchievementStatistics) private statisticsRepository: Repository, + @Inject(CACHE_MANAGER) private cacheManager: Cache, ) {} // ===================================================== @@ -54,26 +63,37 @@ export class AchievementsService { const saved = await this.achievementRepository.save(achievement); this.logger.log(`Achievement created: ${saved.id} - ${saved.name}`); + await this.cacheManager.del(ACHIEVEMENTS_CACHE_KEY); + return this.toAchievementResponseDto(saved); } /** * Get all achievements */ - async getAllAchievements(includeHidden: boolean = false): Promise { - const query = this.achievementRepository.createQueryBuilder('achievement'); + async getAllAchievements( + includeHidden: boolean = false, + query?: PaginationQueryDto, + ): Promise> { + const page = query?.page ?? 1; + const limit = query?.limit ?? 20; + + const qb = this.achievementRepository.createQueryBuilder('achievement'); if (!includeHidden) { - query.andWhere('achievement.isHidden = :isHidden', { isHidden: false }); + qb.andWhere('achievement.isHidden = :isHidden', { isHidden: false }); } - const achievements = await query - .andWhere('achievement.isActive = :isActive', { isActive: true }) + qb.andWhere('achievement.isActive = :isActive', { isActive: true }) .orderBy('achievement.difficulty', 'ASC') .addOrderBy('achievement.createdAt', 'ASC') - .getMany(); + .skip((page - 1) * limit) + .take(limit); - return achievements.map((a) => this.toAchievementResponseDto(a)); + const [achievements, total] = await qb.getManyAndCount(); + + const dtos = achievements.map((a) => this.toAchievementResponseDto(a)); + return buildOffsetResponse(dtos, total, page, limit); } /** @@ -94,13 +114,22 @@ export class AchievementsService { /** * Get achievements by type */ - async getAchievementsByType(type: AchievementType): Promise { - const achievements = await this.achievementRepository.find({ + async getAchievementsByType( + type: AchievementType, + query?: PaginationQueryDto, + ): Promise> { + const page = query?.page ?? 1; + const limit = query?.limit ?? 20; + + const [achievements, total] = await this.achievementRepository.findAndCount({ where: { type, isActive: true, isHidden: false }, order: { difficulty: 'ASC' }, + skip: (page - 1) * limit, + take: limit, }); - return achievements.map((a) => this.toAchievementResponseDto(a)); + const dtos = achievements.map((a) => this.toAchievementResponseDto(a)); + return buildOffsetResponse(dtos, total, page, limit); } /** @@ -122,6 +151,7 @@ export class AchievementsService { const saved = await this.achievementRepository.save(achievement); this.logger.log(`Achievement updated: ${achievementId}`); + await this.cacheManager.del(ACHIEVEMENTS_CACHE_KEY); return this.toAchievementResponseDto(saved); } @@ -131,6 +161,7 @@ export class AchievementsService { async deactivateAchievement(achievementId: string): Promise { await this.achievementRepository.update({ id: achievementId }, { isActive: false }); + await this.cacheManager.del(ACHIEVEMENTS_CACHE_KEY); this.logger.log(`Achievement deactivated: ${achievementId}`); } @@ -253,14 +284,23 @@ export class AchievementsService { /** * Get all progress records for a user */ - async getUserAllProgress(userId: string): Promise { - const progresses = await this.progressRepository.find({ + async getUserAllProgress( + userId: string, + query?: PaginationQueryDto, + ): Promise> { + const page = query?.page ?? 1; + const limit = query?.limit ?? 20; + + const [progresses, total] = await this.progressRepository.findAndCount({ where: { user: { id: userId } }, relations: ['achievement'], order: { createdAt: 'DESC' }, + skip: (page - 1) * limit, + take: limit, }); - return progresses.map((p) => this.toAchievementProgressDto(p)); + const dtos = progresses.map((p) => this.toAchievementProgressDto(p)); + return buildOffsetResponse(dtos, total, page, limit); } /** @@ -359,14 +399,23 @@ export class AchievementsService { /** * Get all unlocked achievements for a user */ - async getUserAchievements(userId: string): Promise { - const achievements = await this.userAchievementRepository.find({ + async getUserAchievements( + userId: string, + query?: PaginationQueryDto, + ): Promise> { + const page = query?.page ?? 1; + const limit = query?.limit ?? 20; + + const [achievements, total] = await this.userAchievementRepository.findAndCount({ where: { user: { id: userId } }, relations: ['achievement'], order: { unlockedAt: 'DESC' }, + skip: (page - 1) * limit, + take: limit, }); - return achievements.map((a) => this.toUserAchievementDto(a)); + const dtos = achievements.map((a) => this.toUserAchievementDto(a)); + return buildOffsetResponse(dtos, total, page, limit); } /** @@ -469,34 +518,42 @@ export class AchievementsService { * Get user achievement overview */ async getUserAchievementOverview(userId: string): Promise { - const allAchievements = await this.achievementRepository.find({ - where: { isActive: true }, - }); - - const userAchievements = await this.userAchievementRepository.find({ - where: { user: { id: userId } }, - }); + // 1. Get total number of active achievements (cacheable) + let totalAchievements = await this.cacheManager.get('total_achievements'); + if (totalAchievements === undefined || totalAchievements === null) { + totalAchievements = await this.achievementRepository.count({ where: { isActive: true } }); + await this.cacheManager.set('total_achievements', totalAchievements, 3600 * 1000); // 1 hour TTL + } - const totalPoints = userAchievements.reduce((sum, a) => sum + a.pointsEarned, 0); - const totalExperience = userAchievements.reduce((sum, a) => sum + a.experienceEarned, 0); + // 2. Fetch user's achievements count, points, and XP in one query + const userStats = await this.userAchievementRepository + .createQueryBuilder('ua') + .select('COUNT(ua.id)', 'unlockedCount') + .addSelect('COALESCE(SUM(ua.pointsEarned), 0)', 'totalPoints') + .addSelect('COALESCE(SUM(ua.experienceEarned), 0)', 'totalExperience') + .where('ua.userId = :userId', { userId }) + .getRawOne(); + + const unlockedCount = parseInt(userStats?.unlockedCount || '0', 10); + const totalPoints = parseInt(userStats?.totalPoints || '0', 10); + const totalExperience = parseInt(userStats?.totalExperience || '0', 10); // Get rank (users with more achievements ranked higher) const rank = await this.userAchievementRepository .createQueryBuilder('ua') .select('COUNT(DISTINCT ua.userId)', 'count') .where('(SELECT COUNT(*) FROM user_achievements WHERE "userId" = ua."userId") > :userCount', { - userCount: userAchievements.length, + userCount: unlockedCount, }) .getRawOne(); - const progressPercentage = - allAchievements.length > 0 - ? Math.round((userAchievements.length / allAchievements.length) * 100) + totalAchievements > 0 + ? Math.round((unlockedCount / totalAchievements) * 100) : 0; return { - totalAchievements: allAchievements.length, - unlockedAchievements: userAchievements.length, + totalAchievements, + unlockedAchievements: unlockedCount, progressPercentage, totalPointsEarned: totalPoints, totalExperienceEarned: totalExperience, diff --git a/tmp_achievements.ts b/tmp_achievements.ts new file mode 100644 index 00000000..1be4c444 Binary files /dev/null and b/tmp_achievements.ts differ