Conversation
📝 WalkthroughWalkthroughThe PR moves the external school client dependency from the API module into the domain module, introduces a cached RankSchoolManager in the domain to resolve school names, adds a SchoolRankPageResult DTO, updates SchoolRankQueryService to return page results with school names, and simplifies the API controller/response to consume the domain-provided page results. Changes
Sequence DiagramsequenceDiagram
participant Client
participant RankReadController
participant SchoolRankQueryService
participant RankSchoolManager
participant Cache
participant SchoolClient
Client->>RankReadController: GET /rankings/schools
RankReadController->>SchoolRankQueryService: querySchoolRanks(params)
SchoolRankQueryService->>SchoolRankQueryService: DB query -> PageResult<SchoolRankQueryResult>
loop per result
SchoolRankQueryService->>RankSchoolManager: getSchoolNameByCode(schoolCode)
RankSchoolManager->>Cache: lookup key "schoolCode::<code>"
alt cache hit
Cache-->>RankSchoolManager: cached schoolName
else cache miss
RankSchoolManager->>SchoolClient: getSchool(request)
SchoolClient-->>RankSchoolManager: SchoolInfo
RankSchoolManager->>Cache: store schoolName
end
RankSchoolManager-->>SchoolRankQueryService: schoolName
SchoolRankQueryService->>SchoolRankQueryService: build SchoolRankPageResult
end
SchoolRankQueryService-->>RankReadController: PageResult<SchoolRankPageResult>
RankReadController-->>Client: SchoolRankPageResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @huhdy32, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 학교 랭크 조회 시 학교 이름 조회 과정의 성능을 최적화하기 위해 캐싱 메커니즘을 도입합니다. 기존에는 API 계층에서 직접 외부 클라이언트를 통해 학교 이름을 조회했으나, 이제 도메인 계층에 캐싱 기능을 갖춘 매니저를 두어 학교 이름 조회 로직을 중앙화하고 반복적인 외부 호출을 줄였습니다. 이를 통해 전반적인 응답 속도 향상과 시스템 부하 감소를 기대할 수 있습니다. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
...mathrank-rank-domain/src/main/java/kr/co/mathrank/domain/rank/service/RankSchoolManager.java
Show resolved
Hide resolved
...ank-rank-domain/src/main/java/kr/co/mathrank/domain/rank/service/SchoolRankQueryService.java
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
domain/mathrank-rank-domain/src/main/java/kr/co/mathrank/domain/rank/service/SchoolRankQueryService.java (1)
27-53: Add@Min(1)and@Min(1)/@Max(...)validation constraints to prevent negative pageNumber and pageSize.The service is marked
@Validated, but the method parameters lack constraint annotations.PageRequest.of(pageNumber - 1, pageSize)will throwIllegalArgumentExceptionifpageNumberis 0 or negative;@NotNullalone does not prevent this. Although the controller provides safe defaults, the service method is publicly callable and should enforce its own input bounds.Proposed fix
import jakarta.validation.constraints.NotNull; +import jakarta.validation.constraints.Min; +import jakarta.validation.constraints.Max; public PageResult<SchoolRankPageResult> querySchoolRanks( - @NotNull final Integer pageSize, - @NotNull final Integer pageNumber + @NotNull @Min(1) @Max(100) final Integer pageSize, + @NotNull @Min(1) final Integer pageNumber ) {Additionally, note that
.map(queryResult -> rankSchoolManager.getSchoolNameByCode(...))will trigger per-item external lookups on cold caches; the per-schoolCode caching inRankSchoolManagermitigates this within a page (repeated schoolCodes hit cache), but on first load this can still cause multiple API calls. Consider batch-fetching or pre-warming the cache if the external school service becomes a bottleneck.
🧹 Nitpick comments (3)
domain/mathrank-rank-domain/src/main/java/kr/co/mathrank/domain/rank/RankDomainConfiguration.java (1)
17-37: Cache spec addition looks consistent; double-check naming/TTL expectations.
Cache name andRequiredCacheSpecpattern match the existing ones; TTL 10 minutes is reasonable if school names rarely change. Consider whether the constant name should follow the*_CACHE_NAMEconvention used above for consistency (optional).domain/mathrank-rank-domain/src/main/java/kr/co/mathrank/domain/rank/dto/SchoolRankPageResult.java (1)
3-18: DTO is fine; clarify nullability expectations.
Ifscore/rank/memberCountandschoolNameare always present, consider primitives (long) and/or bean validation / documentation to prevent accidental null propagation.app/api/mathrank-rank-read-api/src/main/java/kr/co/mathrank/rank/read/SchoolRankPageResponse.java (1)
3-16: Cleaner response mapping; decide fallback behavior for missing schoolName.
IfschoolNamecan be null/blank (e.g., lookup failure), consider a consistent API fallback (empty string vs code vs “unknown”).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/api/mathrank-rank-read-api/build.gradleapp/api/mathrank-rank-read-api/src/main/java/kr/co/mathrank/rank/read/RankReadController.javaapp/api/mathrank-rank-read-api/src/main/java/kr/co/mathrank/rank/read/SchoolRankPageResponse.javadomain/mathrank-rank-domain/build.gradledomain/mathrank-rank-domain/src/main/java/kr/co/mathrank/domain/rank/RankDomainConfiguration.javadomain/mathrank-rank-domain/src/main/java/kr/co/mathrank/domain/rank/dto/SchoolRankPageResult.javadomain/mathrank-rank-domain/src/main/java/kr/co/mathrank/domain/rank/service/RankSchoolManager.javadomain/mathrank-rank-domain/src/main/java/kr/co/mathrank/domain/rank/service/SchoolRankQueryService.java
💤 Files with no reviewable changes (1)
- app/api/mathrank-rank-read-api/build.gradle
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (3)
app/api/mathrank-rank-read-api/src/main/java/kr/co/mathrank/rank/read/RankReadController.java (1)
51-53: Good API-layer simplification.
Controller now just maps domain results to response DTOs; avoids external client coupling in the API module.domain/mathrank-rank-domain/build.gradle (1)
13-13: No actionable concerns. This dependency follows the established pattern in the codebase where domain modules depend on client libraries (including external ones) to integrate with external systems. There are no circular dependencies, andmathrank-schoolis a lightweight HTTP client library—not a web/API layer module. The usage inRankSchoolManageris a legitimate domain service calling an external school API.Likely an incorrect or invalid review comment.
domain/mathrank-rank-domain/src/main/java/kr/co/mathrank/domain/rank/service/RankSchoolManager.java (1)
20-28: The caching logic is already correct; focus on input validation.The current code with
unless = "#result == null"already prevents caching whengetSchool()returns empty. SinceSchoolInfo.none().SCHUL_NM()returns null (all fields in the record are null), the fallback result is not cached. Replace@Validwith@NotBlankon theschoolCodeparameter for more meaningful validation.
1 participant
Summary by CodeRabbit
Performance Improvements
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.