[Fix/#268] 기수 정렬을 alias로 해결할 수 없는 문제 수정 - #269
Merged
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 0 minutes and 19 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughUser 조회에 UserOrderBy 기반 생성 연도 정렬 경로가 추가되어, 생성 연도 기준으로 정렬된 사용자 ID 페이징을 반환하는 JPA 메서드 2개가 추가되고 이를 이용해 정렬된 Page를 구성하도록 검색 로직이 변경됨. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Service as GetUserProfileService
participant RepoAPI as UserRepository (port)
participant Retriever as UserRetriever
participant JPA as UserJpaRepository
participant DB
Client->>Service: 요청(filters, pageable, orderBy)
Service->>RepoAPI: findAllByGenerationAndPartAndNameAndTeam(..., pageable, orderBy)
RepoAPI->>Retriever: findAllByGenerationAndPartAndNameAndTeam(..., pageable, orderBy)
alt orderBy.isGenerationOrder()
Retriever->>JPA: findUserIdsOrderByMatchedGenerationDesc/Asc(filters, pageable)
JPA->>DB: 실행 (JOIN userActivityHistory, GROUP BY u.id, ORDER BY matched generation)
DB-->>JPA: Page<Long> (ordered userIds)
Retriever->>JPA: findAllByIdIn(orderedIds)
JPA->>DB: 조회 UserEntity by ids
DB-->>JPA: List<UserEntity>
Retriever-->>RepoAPI: Page<User> (constructed preserving id order)
else
Retriever->>JPA: 기존 UserEntity 기반 페이징 쿼리(filters, pageable)
JPA->>DB: 실행
DB-->>JPA: Page<UserEntity>
Retriever-->>RepoAPI: Page<User> (mapped from entities)
end
RepoAPI-->>Service: Page<User>
Service-->>Client: 결과 반환
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/sopt/makers/authentication/adapter/out/persistence/repository/user/UserJpaRepository.java`:
- Around line 59-99: The current queries in
findAllWithActivityHistoriesOrderByGenerationDesc and
findAllWithActivityHistoriesOrderByGenerationAsc use JOIN FETCH
u.userActivityHistoryList (alias ah) for ordering but apply filters against a
different alias a, so ordering can come from unrelated histories and paging is
unstable; change to a two-step approach: (1) add new repository methods (or
modify these) to first select only UserEntity ids using a grouped query on
UserActivityHistoryEntity (alias a) applying the
generation/part/team/isAdmin/name filters and ordering by MAX(a.generation) for
DESC (MIN(a.generation) for ASC) with Pageable to get the page of ids, and (2)
reuse the existing findAllWithActivityHistoriesByIdIn to fetch full UserEntity
objects and their userActivityHistoryList for the returned ids in a second
query; ensure method names referenced are
findAllWithActivityHistoriesOrderByGenerationDesc/findAllWithActivityHistoriesOrderByGenerationAsc
and findAllWithActivityHistoriesByIdIn so callers can be updated to perform the
two-step fetch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6c8c874e-d361-4bc4-b667-29cda8bcdc07
📒 Files selected for processing (5)
src/main/java/sopt/makers/authentication/adapter/out/persistence/repository/user/UserJpaRepository.javasrc/main/java/sopt/makers/authentication/adapter/out/persistence/repository/user/UserRepositoryImpl.javasrc/main/java/sopt/makers/authentication/adapter/out/persistence/repository/user/UserRetriever.javasrc/main/java/sopt/makers/authentication/application/port/out/user/UserRepository.javasrc/main/java/sopt/makers/authentication/application/service/user/GetUserProfileService.java
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/sopt/makers/authentication/adapter/out/persistence/repository/user/UserRetriever.java`:
- Around line 104-115: The current code silently drops IDs missing from the
second query (userJpaRepository.findAllWithActivityHistoriesByIdIn) which can
make PageImpl(content, pageable, userIdPage.getTotalElements()) inconsistent;
after building userById from that repository call, detect if any orderedIds are
missing (compare orderedIds against userById.keySet()) and fail fast by throwing
an IllegalStateException (or custom exception) that includes the missing ID list
and context (e.g., mentioning orderedIds, userById, and
userIdPage.getTotalElements()); only convert to domain (UserEntity::toDomain)
and construct PageImpl if no IDs are missing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 90a835bd-0f59-46b0-ba89-60a12124b021
📒 Files selected for processing (2)
src/main/java/sopt/makers/authentication/adapter/out/persistence/repository/user/UserJpaRepository.javasrc/main/java/sopt/makers/authentication/adapter/out/persistence/repository/user/UserRetriever.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/sopt/makers/authentication/adapter/out/persistence/repository/user/UserJpaRepository.java
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/sopt/makers/authentication/adapter/out/persistence/repository/user/UserRetriever.java (1)
98-100: 빈 페이지 처리 방식 개선을 권장합니다.
userIdPage.map(id -> null)코드는 동작하지만,null을 반환하는 람다는 의미적으로 혼란스럽습니다. 더 명시적인 방식으로 빈 페이지를 생성하는 것이 가독성에 좋습니다.♻️ 제안 수정안
List<Long> orderedIds = userIdPage.getContent(); if (orderedIds.isEmpty()) { - return userIdPage.map(id -> null); + return new PageImpl<>(List.of(), pageable, userIdPage.getTotalElements()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/sopt/makers/authentication/adapter/out/persistence/repository/user/UserRetriever.java` around lines 98 - 100, In UserRetriever, replace the confusing userIdPage.map(id -> null) when orderedIds.isEmpty() with an explicit empty Page return; for example return Page.empty(userIdPage.getPageable()) (or, if your Spring version lacks that helper, return new PageImpl<>(Collections.emptyList(), userIdPage.getPageable(), 0)) so the intent is clear and you avoid mapping to nulls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/main/java/sopt/makers/authentication/adapter/out/persistence/repository/user/UserRetriever.java`:
- Around line 98-100: In UserRetriever, replace the confusing userIdPage.map(id
-> null) when orderedIds.isEmpty() with an explicit empty Page return; for
example return Page.empty(userIdPage.getPageable()) (or, if your Spring version
lacks that helper, return new PageImpl<>(Collections.emptyList(),
userIdPage.getPageable(), 0)) so the intent is clear and you avoid mapping to
nulls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 400275a7-58a0-4faa-b5f7-ff147fc38c4d
📒 Files selected for processing (1)
src/main/java/sopt/makers/authentication/adapter/out/persistence/repository/user/UserRetriever.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related Issue 🚀
Work Description ✏️
PR Point 📸