Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions PR_DESCRIPTION_1020.md
Original file line number Diff line number Diff line change
@@ -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.
119 changes: 88 additions & 31 deletions src/achievements/achievements.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,14 @@
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';

Check failure on line 29 in src/achievements/achievements.service.ts

View workflow job for this annotation

GitHub Actions / validate

'@nestjs/common' import is duplicated

const ACHIEVEMENTS_CACHE_KEY = 'achievements_definitions';

@Injectable()
export class AchievementsService {
Expand All @@ -35,6 +43,7 @@
private userAchievementRepository: Repository<UserAchievement>,
@InjectRepository(AchievementStatistics)
private statisticsRepository: Repository<AchievementStatistics>,
@Inject(CACHE_MANAGER) private cacheManager: Cache,
) {}

// =====================================================
Expand All @@ -54,26 +63,37 @@
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<AchievementResponseDto[]> {
const query = this.achievementRepository.createQueryBuilder('achievement');
async getAllAchievements(
includeHidden: boolean = false,
query?: PaginationQueryDto,
): Promise<OffsetPaginatedResponse<AchievementResponseDto>> {
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);
}

/**
Expand All @@ -94,13 +114,22 @@
/**
* Get achievements by type
*/
async getAchievementsByType(type: AchievementType): Promise<AchievementResponseDto[]> {
const achievements = await this.achievementRepository.find({
async getAchievementsByType(
type: AchievementType,
query?: PaginationQueryDto,
): Promise<OffsetPaginatedResponse<AchievementResponseDto>> {
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);
}

/**
Expand All @@ -122,6 +151,7 @@
const saved = await this.achievementRepository.save(achievement);

this.logger.log(`Achievement updated: ${achievementId}`);
await this.cacheManager.del(ACHIEVEMENTS_CACHE_KEY);
return this.toAchievementResponseDto(saved);
}

Expand All @@ -131,6 +161,7 @@
async deactivateAchievement(achievementId: string): Promise<void> {
await this.achievementRepository.update({ id: achievementId }, { isActive: false });

await this.cacheManager.del(ACHIEVEMENTS_CACHE_KEY);
this.logger.log(`Achievement deactivated: ${achievementId}`);
}

Expand Down Expand Up @@ -253,14 +284,23 @@
/**
* Get all progress records for a user
*/
async getUserAllProgress(userId: string): Promise<AchievementProgressDto[]> {
const progresses = await this.progressRepository.find({
async getUserAllProgress(
userId: string,
query?: PaginationQueryDto,
): Promise<OffsetPaginatedResponse<AchievementProgressDto>> {
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);
}

/**
Expand Down Expand Up @@ -359,14 +399,23 @@
/**
* Get all unlocked achievements for a user
*/
async getUserAchievements(userId: string): Promise<UserAchievementDto[]> {
const achievements = await this.userAchievementRepository.find({
async getUserAchievements(
userId: string,
query?: PaginationQueryDto,
): Promise<OffsetPaginatedResponse<UserAchievementDto>> {
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);
}

/**
Expand Down Expand Up @@ -469,34 +518,42 @@
* Get user achievement overview
*/
async getUserAchievementOverview(userId: string): Promise<AchievementOverviewDto> {
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<number>('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();

Check failure on line 536 in src/achievements/achievements.service.ts

View workflow job for this annotation

GitHub Actions / validate

Delete `······`
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

Check failure on line 550 in src/achievements/achievements.service.ts

View workflow job for this annotation

GitHub Actions / validate

Replace `⏎········?·Math.round((unlockedCount·/·totalAchievements)·*·100)⏎·······` with `·?·Math.round((unlockedCount·/·totalAchievements)·*·100)`
? Math.round((unlockedCount / totalAchievements) * 100)
: 0;

return {
totalAchievements: allAchievements.length,
unlockedAchievements: userAchievements.length,
totalAchievements,
unlockedAchievements: unlockedCount,
progressPercentage,
totalPointsEarned: totalPoints,
totalExperienceEarned: totalExperience,
Expand Down
Binary file added tmp_achievements.ts
Binary file not shown.
Loading