-
Notifications
You must be signed in to change notification settings - Fork 2
MOSU-96 feat: 학교 조회시 현재 신청자 수 추가 #101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
dc6f64f
e3a7138
13e207d
b116d7a
8c67c1c
1de704b
4a95f37
08a618f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,8 @@ | ||
| package life.mosu.mosuserver.application.school; | ||
|
|
||
|
|
||
| import java.util.List; | ||
| import life.mosu.mosuserver.domain.school.SchoolJpaRepository; | ||
| import life.mosu.mosuserver.presentation.school.dto.AvailableSchoolResponse; | ||
| import life.mosu.mosuserver.presentation.school.dto.SchoolRegistrationRequest; | ||
| import life.mosu.mosuserver.presentation.school.dto.SchoolResponse; | ||
| import lombok.RequiredArgsConstructor; | ||
|
|
@@ -15,12 +15,21 @@ | |
| public class SchoolService { | ||
|
|
||
| private final SchoolJpaRepository schoolJpaRepository; | ||
| private final SchoolQuotaCacheManager schoolQuotaCacheManager; | ||
|
|
||
| @Transactional(readOnly = true, propagation = Propagation.SUPPORTS) | ||
| public List<SchoolResponse> getSchools() { | ||
| public List<AvailableSchoolResponse> getAvailableSchools() { | ||
|
|
||
| return schoolJpaRepository.findAll() | ||
| .stream() | ||
| .map(SchoolResponse::from) | ||
| .map(school -> { | ||
| SchoolResponse schoolResponse = SchoolResponse.from(school); | ||
|
|
||
| Long currentQuantity = schoolQuotaCacheManager.getSchoolApplicationCounts( | ||
| schoolResponse.schoolName()); | ||
|
|
||
| return AvailableSchoolResponse.from(schoolResponse, currentQuantity); | ||
| }) | ||
|
Comment on lines
+25
to
+32
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add error handling and consider performance optimization. The implementation has several potential issues:
Consider this improved implementation: return schoolJpaRepository.findAll()
.stream()
- .map(school -> {
- SchoolResponse schoolResponse = SchoolResponse.from(school);
-
- Long currentQuantity = schoolQuotaCacheManager.getSchoolApplicationCounts(
- schoolResponse.schoolName());
-
- return AvailableSchoolResponse.from(schoolResponse, currentQuantity);
- })
+ .map(school -> {
+ SchoolResponse schoolResponse = SchoolResponse.from(school);
+
+ try {
+ Long currentQuantity = schoolQuotaCacheManager.getSchoolApplicationCounts(
+ schoolResponse.schoolName());
+ return AvailableSchoolResponse.from(schoolResponse,
+ currentQuantity != null ? currentQuantity : 0L);
+ } catch (Exception e) {
+ // Log error and return with default count
+ log.warn("Failed to get application count for school: {}",
+ schoolResponse.schoolName(), e);
+ return AvailableSchoolResponse.from(schoolResponse, 0L);
+ }
+ })
.toList();For better performance, consider batch operations if the cache manager supports it: // Get all school names first
List<String> schoolNames = schoolJpaRepository.findAll()
.stream()
.map(School::getName)
.toList();
// Batch get application counts
Map<String, Long> applicationCounts = schoolQuotaCacheManager.getBatchApplicationCounts(schoolNames);
// Then map with cached results
return schoolJpaRepository.findAll()
.stream()
.map(school -> {
SchoolResponse schoolResponse = SchoolResponse.from(school);
Long currentQuantity = applicationCounts.getOrDefault(schoolResponse.schoolName(), 0L);
return AvailableSchoolResponse.from(schoolResponse, currentQuantity);
})
.toList();🤖 Prompt for AI Agents |
||
| .toList(); | ||
|
Comment on lines
23
to
33
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This implementation has a performance issue known as the N+1 problem. It iterates through all schools and makes a separate call to Redis for each school inside the To optimize this, you should fetch all school application counts in a single batch request. Here's a suggested approach:
Example implementation: public List<AvailableSchoolResponse> getAvailableSchools() {
List<SchoolJpaEntity> schools = schoolJpaRepository.findAll();
List<String> schoolNames = schools.stream()
.map(SchoolJpaEntity::getSchoolName)
.toList();
// This method would need to be implemented in SchoolQuotaCacheManager
// to perform a batch fetch from Redis.
Map<String, Long> applicationCounts = schoolQuotaCacheManager.getSchoolApplicationCountsInBatch(schoolNames);
return schools.stream()
.map(school -> {
SchoolResponse schoolResponse = SchoolResponse.from(school);
Long currentQuantity = applicationCounts.getOrDefault(school.getSchoolName(), 0L);
return AvailableSchoolResponse.from(schoolResponse, currentQuantity);
})
.toList();
}This change will significantly improve the performance of this endpoint. |
||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,8 @@ public interface SchoolJpaRepository extends JpaRepository<SchoolJpaEntity, Long | |
| SELECT new life.mosu.mosuserver.domain.school.SchoolApplicationProjection(s.id, s.schoolName, COUNT(a)) | ||
| FROM SchoolJpaEntity s | ||
| LEFT JOIN ApplicationSchoolJpaEntity a ON a.schoolId = s.id | ||
| LEFT JOIN PaymentJpaEntity p ON p.applicationSchoolId = a.id | ||
| WHERE p.paymentStatus = 'DONE' | ||
|
Comment on lines
+15
to
+16
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix query logic to include schools with zero paid applications. The current WHERE clause will exclude schools that have no applications with successful payments from the result set entirely. This is likely incorrect behavior - schools with 0 paid applications should still appear in the results with a count of 0. Move the payment status condition to the JOIN clause to preserve schools with zero paid applications: - LEFT JOIN PaymentJpaEntity p ON p.applicationSchoolId = a.id
- WHERE p.paymentStatus = 'DONE'
+ LEFT JOIN PaymentJpaEntity p ON p.applicationSchoolId = a.id AND p.paymentStatus = 'DONE'This ensures all schools are included in the results, with accurate counts of only successfully paid applications. 🤖 Prompt for AI Agents |
||
| GROUP BY s.id, s.schoolName | ||
| """) | ||
| List<SchoolApplicationProjection> countBySchoolNameGroupBy(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,35 @@ | ||||||
| package life.mosu.mosuserver.presentation.school.dto; | ||||||
|
|
||||||
| import java.time.LocalDate; | ||||||
| import java.time.LocalDateTime; | ||||||
|
|
||||||
| public record AvailableSchoolResponse( | ||||||
| Long id, | ||||||
| String schoolName, | ||||||
| String area, | ||||||
| String lunch, | ||||||
| Integer lunchPrice, | ||||||
| LocalDateTime deadlineTime, | ||||||
| LocalDate examDate, | ||||||
| Long currentQuantity, | ||||||
| Long capacity | ||||||
| ) { | ||||||
|
|
||||||
| public static AvailableSchoolResponse from( | ||||||
| SchoolResponse school, | ||||||
| Long currentQuantity | ||||||
| ) { | ||||||
| return new AvailableSchoolResponse( | ||||||
| school.id(), | ||||||
| school.schoolName(), | ||||||
| school.area(), | ||||||
| school.lunch(), | ||||||
| school.lunchPrice(), | ||||||
| school.deadlineTime(), | ||||||
| school.examDate(), | ||||||
| currentQuantity, | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The It's better to treat a missing count as zero. You can ensure
Suggested change
|
||||||
| school.capacity() | ||||||
| ); | ||||||
| } | ||||||
|
|
||||||
| } | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Verify cache manager implementation for performance and reliability.
The new dependency
schoolQuotaCacheManageris properly injected, but ensure that:🏁 Script executed:
Length of output: 127
🏁 Script executed:
Length of output: 3583
Implement batching, error handling, and cache invalidation in SchoolQuotaCacheManager
The current implementation in
src/main/java/life/mosu/mosuserver/application/school/SchoolQuotaCacheManager.java
uses per‐key loops and has no error handling or eviction logic. To improve performance and reliability, please:
setcalls incacheSchoolMaxCapacities()andcacheSchoolCurrentApplicationCounts()with Redis pipelining oropsForValue().multiSet()for bulk writes.set,get,increment,decrement) in try/catch blocks and fall back toschoolRepositoryreads if Redis is unavailable.redisTemplate.expire(...)orinvalidateSchoolCache(String schoolName)) to ensure real-time accuracy.🤖 Prompt for AI Agents