Merged
Conversation
- GET /api/members로 변경 - 기존 엔드포인트 경로는 유지 (/api/members/me 등) - 문서화 경로 동기화
Contributor
WalkthroughThis PR adds a public API endpoint for listing users with pagination, including their certification counts and current points. It extends the adapter layer with a new query method for aggregating certification data, introduces new DTOs for user summary data, and implements service-level orchestration to fetch and merge user information from multiple data sources. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Controller as MemberController
participant Service as MemberQueryService
participant Repo as MemberQueryRepository
participant PointClient
participant ChallengeClient
Client->>Controller: GET /users?page=0&size=10
activate Controller
Controller->>Controller: getCurrentMemberIdOrNull()
Controller->>Service: getUsersSummary(page, size, currentMemberId)
activate Service
Service->>Repo: getUsersBasicInfo(condition)
activate Repo
Note over Repo: Query active members<br/>(NORMAL status, not deleted)<br/>with pagination
Repo-->>Service: PageTemplate<UserBasicInfo>
deactivate Repo
par Fetch Aggregated Data
Service->>PointClient: getPoints(memberIds)
Service->>ChallengeClient: getCertificationCountByMembers(memberIds)
end
PointClient-->>Service: Map<Long, BigDecimal>
ChallengeClient-->>Service: Map<Long, Long>
Note over Service: Map basic info + points<br/>+ cert counts → UserSummaryDto
Service-->>Controller: PageTemplate<UserSummaryDto>
deactivate Service
Controller-->>Client: ApiTemplate.ok(USER_LIST_RETRIEVED, page)
deactivate Controller
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
Unit Test Coverage Report
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (10)
src/main/java/com/example/green/domain/challenge/infra/adapter/ChallengeAdapter.java (1)
40-40: Keep data-access in repository/infra query layer, not the adapter.Injecting JPAQueryFactory into the adapter mixes concerns. Consider moving the QueryDSL logic into a repository/Query class and have the adapter delegate to it for cohesion and testability.
src/main/java/com/example/green/domain/member/repository/MemberQueryRepository.java (1)
20-25: Naming consistency: search vs get.Interface mixes “searchMemberBasicInfo(...)” and “getUsersBasicInfo(...)”. Consider aligning verbs (both “search” or both “get”) for a predictable API.
src/main/java/com/example/green/domain/member/infra/querydsl/MemberQueryRepositoryImpl.java (1)
75-101: LGTM; filters and paging are correct.Predicate, count, projection, and ordering look good. Optional: add a DB index supporting (status, deleted, id desc) to optimize count + page scans at scale.
src/main/java/com/example/green/domain/member/controller/message/MemberResponseMessage.java (1)
19-23: Avoid overlapping message semantics.Now both USER_LIST_RETRIEVED and MEMBER_LIST_RETRIEVED exist. Consider consolidating or clearly scoping usage to prevent confusion in controllers/docs.
src/main/java/com/example/green/infra/client/ChallengeClient.java (1)
21-26: Define behavior for empty inputs and null-safe returns.Specify and enforce:
- If
memberIdsis null/empty, return an empty map without remote calls.- Returned map must not contain null values; missing members imply
0.Add method-level Javadoc note and make implementation short‑circuit accordingly. Also consider accepting
Collection<Long>for flexibility.src/main/java/com/example/green/domain/member/controller/docs/MemberControllerDocs.java (2)
30-54: Align documented defaults and constraints with implementation.Docs say “page starts at 1 (default 1)”, but controller doesn’t default
page. Please either set controller default to 1 or update docs. Add bounds (e.g.,1 ≤ page,1 ≤ size ≤ 100).
95-101: Add documentation of status filter and ordering guarantees to API contract.The repository's
getUsersBasicInfomethod enforces two guarantees that should be documented in the controller API:
- Results are filtered to
MemberStatus.NORMALmembers only (active, non-deleted)- Results are ordered by ID in descending order
Update the
@ParameterObjector method documentation to explicitly state these filtering and ordering behaviors so clients understand the result contract.src/main/java/com/example/green/domain/member/service/MemberQueryService.java (1)
72-74: Defensive null handling for external maps.If clients return
null, subsequent lookups NPE. Default to empty maps.-Map<Long, BigDecimal> pointsMap = pointClient.getEarnedPointByMember(memberIds); -Map<Long, Long> certCountMap = challengeClient.getCertificationCountByMembers(memberIds); +Map<Long, BigDecimal> pointsMap = + java.util.Optional.ofNullable(pointClient.getEarnedPointByMember(memberIds)) + .orElseGet(java.util.Collections::emptyMap); +Map<Long, Long> certCountMap = + java.util.Optional.ofNullable(challengeClient.getCertificationCountByMembers(memberIds)) + .orElseGet(java.util.Collections::emptyMap);src/main/java/com/example/green/domain/member/dto/UserSummaryDto.java (1)
7-27: Stabilize boolean JSON name ‘isMe’.Some Jackson versions can drop the “is” prefix for booleans. To guarantee the field name matches docs (“isMe”), annotate it.
+import com.fasterxml.jackson.annotation.JsonProperty; ... - boolean isMe, + @JsonProperty("isMe") boolean isMe,src/main/java/com/example/green/domain/member/controller/MemberController.java (1)
197-204: Minor: tighten principal check.Optionally also verify
auth.isAuthenticated()to skip edge anonymous states in custom providers (defensive). Currentinstanceof PrincipalDetailscheck is generally sufficient.-Authentication auth = SecurityContextHolder.getContext().getAuthentication(); -if (auth != null && auth.getPrincipal() instanceof PrincipalDetails details) { +Authentication auth = SecurityContextHolder.getContext().getAuthentication(); +if (auth != null && auth.isAuthenticated() && auth.getPrincipal() instanceof PrincipalDetails details) { return details.getMemberId(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/main/java/com/example/green/domain/challenge/infra/adapter/ChallengeAdapter.java(4 hunks)src/main/java/com/example/green/domain/member/controller/MemberController.java(4 hunks)src/main/java/com/example/green/domain/member/controller/docs/MemberControllerDocs.java(2 hunks)src/main/java/com/example/green/domain/member/controller/message/MemberResponseMessage.java(1 hunks)src/main/java/com/example/green/domain/member/dto/UserSummaryDto.java(1 hunks)src/main/java/com/example/green/domain/member/infra/querydsl/MemberQueryRepositoryImpl.java(2 hunks)src/main/java/com/example/green/domain/member/repository/MemberQueryRepository.java(2 hunks)src/main/java/com/example/green/domain/member/repository/dto/UserBasicInfo.java(1 hunks)src/main/java/com/example/green/domain/member/service/MemberQueryService.java(3 hunks)src/main/java/com/example/green/infra/client/ChallengeClient.java(2 hunks)
🔇 Additional comments (1)
src/main/java/com/example/green/domain/member/repository/dto/UserBasicInfo.java (1)
3-7: LGTM.Minimal, clear projection for repository use.
Comment on lines
+68
to
+128
| @Override | ||
| public Map<Long, Long> getCertificationCountByMembers(List<Long> memberIds) { | ||
| if (memberIds == null || memberIds.isEmpty()) { | ||
| return Map.of(); | ||
| } | ||
|
|
||
| Map<Long, Long> resultMap = new HashMap<>(); | ||
|
|
||
| // PersonalChallengeParticipation | ||
| List<Tuple> personalCounts = queryFactory | ||
| .select(personalChallengeParticipation.memberId, personalChallengeParticipation.certCount.sum()) | ||
| .from(personalChallengeParticipation) | ||
| .where( | ||
| personalChallengeParticipation.memberId.in(memberIds), | ||
| personalChallengeParticipation.deleted.eq(false) | ||
| ) | ||
| .groupBy(personalChallengeParticipation.memberId) | ||
| .fetch(); | ||
|
|
||
| for (Tuple tuple : personalCounts) { | ||
| Long memberId = tuple.get(personalChallengeParticipation.memberId); | ||
| Integer sum = tuple.get(personalChallengeParticipation.certCount.sum()); | ||
| resultMap.put(memberId, sum != null ? sum.longValue() : 0L); | ||
| } | ||
|
|
||
| // TeamChallengeParticipation | ||
| List<Tuple> teamCounts = queryFactory | ||
| .select(teamChallengeParticipation.memberId, teamChallengeParticipation.certCount.sum()) | ||
| .from(teamChallengeParticipation) | ||
| .where( | ||
| teamChallengeParticipation.memberId.in(memberIds), | ||
| teamChallengeParticipation.deleted.eq(false) | ||
| ) | ||
| .groupBy(teamChallengeParticipation.memberId) | ||
| .fetch(); | ||
|
|
||
| for (Tuple tuple : teamCounts) { | ||
| Long memberId = tuple.get(teamChallengeParticipation.memberId); | ||
| Integer sum = tuple.get(teamChallengeParticipation.certCount.sum()); | ||
| resultMap.merge(memberId, sum != null ? sum.longValue() : 0L, Long::sum); | ||
| } | ||
|
|
||
| // Participation | ||
| List<Tuple> participationCounts = queryFactory | ||
| .select(participation.memberId, participation.certCount.sum()) | ||
| .from(participation) | ||
| .where( | ||
| participation.memberId.in(memberIds), | ||
| participation.deleted.eq(false) | ||
| ) | ||
| .groupBy(participation.memberId) | ||
| .fetch(); | ||
|
|
||
| for (Tuple tuple : participationCounts) { | ||
| Long memberId = tuple.get(participation.memberId); | ||
| Integer sum = tuple.get(participation.certCount.sum()); | ||
| resultMap.merge(memberId, sum != null ? sum.longValue() : 0L, Long::sum); | ||
| } | ||
|
|
||
| return resultMap; | ||
| } |
Contributor
There was a problem hiding this comment.
Fix SUM(Integer) → Long mismatch; also return zeros for missing members.
JPA SUM over integer columns typically returns Long. Reading it as Integer can throw a ClassCastException at runtime. Also, members without any rows are missing from the map; callers may expect 0.
Apply this to make sums Long at the query level (no nulls) and to fill absent members with 0:
@@
- public Map<Long, Long> getCertificationCountByMembers(List<Long> memberIds) {
+ public Map<Long, Long> getCertificationCountByMembers(List<Long> memberIds) {
if (memberIds == null || memberIds.isEmpty()) {
return Map.of();
}
- Map<Long, Long> resultMap = new HashMap<>();
+ Map<Long, Long> resultMap = new HashMap<>(memberIds.size());
// PersonalChallengeParticipation
- List<Tuple> personalCounts = queryFactory
- .select(personalChallengeParticipation.memberId, personalChallengeParticipation.certCount.sum())
+ var pSum = personalChallengeParticipation.certCount.sum().coalesce(0).longValue();
+ List<Tuple> personalCounts = queryFactory
+ .select(personalChallengeParticipation.memberId, pSum)
.from(personalChallengeParticipation)
.where(
personalChallengeParticipation.memberId.in(memberIds),
personalChallengeParticipation.deleted.eq(false)
)
.groupBy(personalChallengeParticipation.memberId)
.fetch();
for (Tuple tuple : personalCounts) {
- Long memberId = tuple.get(personalChallengeParticipation.memberId);
- Integer sum = tuple.get(personalChallengeParticipation.certCount.sum());
- resultMap.put(memberId, sum != null ? sum.longValue() : 0L);
+ Long memberId = tuple.get(personalChallengeParticipation.memberId);
+ Long sum = tuple.get(pSum);
+ resultMap.put(memberId, sum);
}
// TeamChallengeParticipation
- List<Tuple> teamCounts = queryFactory
- .select(teamChallengeParticipation.memberId, teamChallengeParticipation.certCount.sum())
+ var tSum = teamChallengeParticipation.certCount.sum().coalesce(0).longValue();
+ List<Tuple> teamCounts = queryFactory
+ .select(teamChallengeParticipation.memberId, tSum)
.from(teamChallengeParticipation)
.where(
teamChallengeParticipation.memberId.in(memberIds),
teamChallengeParticipation.deleted.eq(false)
)
.groupBy(teamChallengeParticipation.memberId)
.fetch();
for (Tuple tuple : teamCounts) {
Long memberId = tuple.get(teamChallengeParticipation.memberId);
- Integer sum = tuple.get(teamChallengeParticipation.certCount.sum());
- resultMap.merge(memberId, sum != null ? sum.longValue() : 0L, Long::sum);
+ Long sum = tuple.get(tSum);
+ resultMap.merge(memberId, sum, Long::sum);
}
// Participation
- List<Tuple> participationCounts = queryFactory
- .select(participation.memberId, participation.certCount.sum())
+ var partSum = participation.certCount.sum().coalesce(0).longValue();
+ List<Tuple> participationCounts = queryFactory
+ .select(participation.memberId, partSum)
.from(participation)
.where(
participation.memberId.in(memberIds),
participation.deleted.eq(false)
)
.groupBy(participation.memberId)
.fetch();
for (Tuple tuple : participationCounts) {
Long memberId = tuple.get(participation.memberId);
- Integer sum = tuple.get(participation.certCount.sum());
- resultMap.merge(memberId, sum != null ? sum.longValue() : 0L, Long::sum);
+ Long sum = tuple.get(partSum);
+ resultMap.merge(memberId, sum, Long::sum);
}
- return resultMap;
+ for (Long id : memberIds) {
+ resultMap.putIfAbsent(id, 0L);
+ }
+ return resultMap;
}And add the missing import:
+import com.querydsl.core.types.dsl.NumberExpression;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
src/main/java/com/example/green/domain/challenge/infra/adapter/ChallengeAdapter.java
around lines 68–128, change the SUM handling to use Longs and ensure every
memberId is present in the returned map with a 0L default: update each QueryDSL
sum selection to produce a Long (use coalesce/Expressions to default the sum to
0L at the query level or call tuple.get(..., Long.class) and treat null as 0L),
read the tuple sums as Long (not Integer) and merge/add them into resultMap as
Long values, then after processing all result sets iterate memberIds and
putIfAbsent(memberId, 0L) so missing members are present with zero; also add the
necessary QueryDSL import(s) you use for coalesce/Expressions.
Comment on lines
+50
to
+64
| @Override | ||
| @PublicApi(reason = "누구나 사용자 목록을 조회할 수 있습니다") | ||
| @GetMapping | ||
| public ApiTemplate<PageTemplate<UserSummaryDto>> getUsers( | ||
| @RequestParam(required = false) Integer page, | ||
| @RequestParam(required = false, defaultValue = "10") Integer size | ||
| ) { | ||
| Long currentMemberId = getCurrentMemberIdOrNull(); | ||
|
|
||
| PageTemplate<UserSummaryDto> response = memberQueryService.getUsersSummary(page, size, currentMemberId); | ||
|
|
||
| log.info("[USER_LIST] 사용자 목록 조회: page={}, size={}, currentMemberId={}", page, size, currentMemberId); | ||
|
|
||
| return ApiTemplate.ok(MemberResponseMessage.USER_LIST_RETRIEVED, response); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Set page default to 1 to match docs and avoid null propagation.
Aligns with docs and reduces downstream checks.
-@GetMapping
-public ApiTemplate<PageTemplate<UserSummaryDto>> getUsers(
- @RequestParam(required = false) Integer page,
- @RequestParam(required = false, defaultValue = "10") Integer size
-) {
+@GetMapping
+public ApiTemplate<PageTemplate<UserSummaryDto>> getUsers(
+ @RequestParam(required = false, defaultValue = "1") Integer page,
+ @RequestParam(required = false, defaultValue = "10") Integer size
+) {Optionally bound size (e.g., ≤ 100) and page (≥ 1) using validation or service-side clamping added above.
🤖 Prompt for AI Agents
In
src/main/java/com/example/green/domain/member/controller/MemberController.java
around lines 50 to 64, the request parameter page currently allows null which
contradicts the docs and forces downstream null handling; change the
@RequestParam for page to have defaultValue = "1" so page defaults to 1, and add
simple validation/clamping (either via @Min(1) and @Max on the controller
parameter or by clamping inside the method) to ensure page >= 1 and size is
bounded (e.g., size ≤ 100) before calling memberQueryService.getUsersSummary;
keep logging and response flow unchanged.
Comment on lines
+42
to
+55
| public PageTemplate<UserSummaryDto> getUsersSummary(Integer page, Integer size, Long currentMemberId) { | ||
| PageSearchCondition condition = new PageSearchCondition() { | ||
| @Override | ||
| public Integer page() { | ||
| return page; | ||
| } | ||
|
|
||
| @Override | ||
| public Integer size() { | ||
| return size; | ||
| } | ||
| }; | ||
|
|
||
| PageTemplate<UserBasicInfo> basicPage = memberQueryRepository.getUsersBasicInfo(condition); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Normalize page/size before building the condition.
page and size may be null/invalid; pass-through risks NPE downstream and inconsistent pagination.
Apply sane defaults and bounds at method start (e.g., page=1 when null/lt 1; clamp size to [1,100]).
public PageTemplate<UserSummaryDto> getUsersSummary(Integer page, Integer size, Long currentMemberId) {
- PageSearchCondition condition = new PageSearchCondition() {
+ // Normalize inputs
+ int safePage = (page == null || page < 1) ? 1 : page;
+ int safeSize = (size == null || size < 1) ? 10 : Math.min(size, 100);
+
+ PageSearchCondition condition = new PageSearchCondition() {
@Override
public Integer page() {
- return page;
+ return safePage;
}
@Override
public Integer size() {
- return size;
+ return safeSize;
}
};🤖 Prompt for AI Agents
In src/main/java/com/example/green/domain/member/service/MemberQueryService.java
around lines 42 to 55, the page and size parameters are used directly to build
PageSearchCondition which can cause NPEs or invalid pagination; normalize them
at the start of the method by assigning defaults and bounds (e.g., if page is
null or <1 set page = 1; if size is null set size = default like 20; clamp size
to a safe range such as 1..100), then use these validated integers to construct
the PageSearchCondition so downstream code receives non-null, bounded values.
Comment on lines
+75
to
+84
| List<UserSummaryDto> result = basicPage.content().stream() | ||
| .map(info -> UserSummaryDto.of( | ||
| info.memberId(), | ||
| info.nickname(), | ||
| info.profileImageUrl(), | ||
| currentMemberId, | ||
| certCountMap.getOrDefault(info.memberId(), 0L), | ||
| pointsMap.getOrDefault(info.memberId(), BigDecimal.ZERO) | ||
| )) | ||
| .toList(); |
Contributor
There was a problem hiding this comment.
Possible NPE from Long→long unboxing when map value is null.
Map.getOrDefault(k, 0L) returns null if the map contains k with null value. Autounboxing to long then throws NPE.
-List<UserSummaryDto> result = basicPage.content().stream()
- .map(info -> UserSummaryDto.of(
- info.memberId(),
- info.nickname(),
- info.profileImageUrl(),
- currentMemberId,
- certCountMap.getOrDefault(info.memberId(), 0L),
- pointsMap.getOrDefault(info.memberId(), BigDecimal.ZERO)
- ))
- .toList();
+List<UserSummaryDto> result = basicPage.content().stream()
+ .map(info -> {
+ Long cc = certCountMap.get(info.memberId());
+ long certCount = (cc == null) ? 0L : cc;
+ BigDecimal pts = pointsMap.get(info.memberId());
+ return UserSummaryDto.of(
+ info.memberId(),
+ info.nickname(),
+ info.profileImageUrl(),
+ currentMemberId,
+ certCount,
+ pts
+ );
+ })
+ .toList();Committable suggestion skipped: line range outside the PR's diff.
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.
#️⃣ 연관된 이슈
📝 작업 내용
📷 스크린샷
💬 리뷰 요구사항(선택)
📌 PR 진행 시 참고사항
P1: 꼭 반영해 주세요 (Request Changes) – 이슈나 취약점 관련P2: 반영을 고려해 주세요 (Comment) – 개선 의견P3: 단순 제안 (Chore)Summary by CodeRabbit
Release Notes