Conversation
…ckend into feat/#38 # Conflicts: # src/main/java/com/whereyouad/WhereYouAd/domains/organization/domain/service/OrgService.java # src/main/java/com/whereyouad/WhereYouAd/domains/organization/domain/service/OrgServiceImpl.java # src/main/java/com/whereyouad/WhereYouAd/domains/organization/exception/code/OrgErrorCode.java # src/main/java/com/whereyouad/WhereYouAd/domains/organization/presentation/OrgController.java # src/main/java/com/whereyouad/WhereYouAd/domains/organization/presentation/docs/OrgControllerDocs.java
Walkthrough조직 멤버의 권한을 변경하는 기능을 추가합니다. PATCH 엔드포인트를 통해 요청 후 ADMIN 권한 검증, 권한 업데이트, 결과 반환의 흐름으로 구성됩니다. Changes
Sequence DiagramsequenceDiagram
actor Client
participant Controller as OrgController
participant Service as OrgServiceImpl
participant Repo as OrgMemberRepository
participant Entity as OrgMember
Client->>Controller: PATCH /members/{orgId}/{memberId}<br/>UpdateRole{orgRole}
Controller->>Service: updateOrgMembersRole(userId, orgId, memberId, dto)
Service->>Repo: findByOrgIdAndId(orgId, ?)
Repo-->>Service: Organization
Service->>Repo: findByUserIdAndOrgId(userId, orgId)
Repo-->>Service: OrgMember (requester)
Note over Service: ✓ requester.role == ADMIN?
Service->>Repo: findByOrgIdAndId(orgId, memberId)
Repo-->>Service: OrgMember (target)
Note over Service: ✓ target.role != ADMIN?
Service->>Entity: updateRole(newOrgRole)
Entity->>Entity: this.role = newOrgRole
Service->>Repo: save(OrgMember)
Repo-->>Service: OrgMember (updated)
Service->>Service: toOrgMemberDTO(updated)
Service-->>Controller: OrgMemberDTO
Controller-->>Client: DataResponse{200, OrgMemberDTO}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 전체 9개 파일의 변경이 이루어졌으며, 권한 검증 로직(ADMIN 확인, ADMIN→MEMBER 변경 방지)이 포함되어 있습니다. 각 계층(Controller → Service → Entity)이 명확한 책임을 가지고 있어 흐름 파악은 쉽지만, 보안 관련 검증 로직을 꼼꼼히 확인해야 합니다. 특히 Possibly related PRs
Suggested reviewers
📋 리뷰 팁잘 짠 부분들:
꼼꼼히 확인할 부분들:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/main/java/com/whereyouad/WhereYouAd/domains/organization/persistence/repository/OrgMemberRepository.java (1)
43-48: JPQL enum 비교에 문자열 리터럴 대신 파라미터 바인딩을 사용하는 것을 권장합니다.현재
om.user.status = 'ACTIVE'처럼 enum 값을 문자열 리터럴로 직접 비교하고 있습니다.UserStatus.ACTIVEenum의 이름이 변경되면 컴파일 에러 없이 런타임에서 쿼리가 잘못 동작할 수 있습니다.파라미터 방식으로 변경하면 타입 안전성이 확보됩니다:
♻️ 파라미터 바인딩으로 변경 제안
- `@Query`("SELECT om FROM OrgMember om " + - "WHERE om.user.id = :userId " + - "AND om.organization.id = :orgId " + - "AND om.user.status = 'ACTIVE'") - Optional<OrgMember> findByUserIdAndOrgId(`@Param`("userId") Long userId, `@Param`("orgId") Long orgId); + `@Query`("SELECT om FROM OrgMember om " + + "WHERE om.user.id = :userId " + + "AND om.organization.id = :orgId " + + "AND om.user.status = :userStatus") + Optional<OrgMember> findByUserIdAndOrgId(`@Param`("userId") Long userId, + `@Param`("orgId") Long orgId, + `@Param`("userStatus") UserStatus userStatus);호출부에서는
findByUserIdAndOrgId(userId, orgId, UserStatus.ACTIVE)형태로 사용합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/whereyouad/WhereYouAd/domains/organization/persistence/repository/OrgMemberRepository.java` around lines 43 - 48, Change the JPQL in OrgMemberRepository so it does not compare the enum via the string literal 'ACTIVE'; update the query used by findByUserIdAndOrgId (or add an overloaded method) to accept a UserStatus parameter and bind it (e.g., "om.user.status = :status") and change the method signature to include a UserStatus status parameter (call site will pass UserStatus.ACTIVE). Ensure you reference the UserStatus enum and parameter name (e.g., status) in the `@Param` annotation so om.user.status uses a typed parameter instead of a hard-coded string.src/main/java/com/whereyouad/WhereYouAd/domains/organization/domain/service/OrgServiceImpl.java (1)
210-221:findByUserIdAndOrgId결과에JOIN FETCH없이toOrgMemberDTO호출 시 추가 쿼리가 발생합니다.
findByUserIdAndOrgIdJPQL은om.user.status조건으로 인해 user 테이블과 암묵적 JOIN을 하지만,JOIN FETCH가 없어서 user 엔티티를 실제로 로딩하지 않습니다.이후 221번 라인에서
OrgConverter.toOrgMemberDTO(orgMember)호출 시orgMember.getUser().getName()등으로 user에 접근하면 Hibernate가 별도SELECT * FROM user WHERE id = ?를 추가로 발생시킵니다.이 메서드 전체로 보면 총 4번의 쿼리(조직 조회 1 + 요청자 조회 1 + 대상 멤버 조회 1 + user 지연 로딩 1)가 발생합니다.
대상 조회 쿼리에
JOIN FETCH를 추가하거나, 이 용도에 특화된 별도 쿼리 메서드를 추가하는 것을 권장합니다:♻️ JOIN FETCH 추가 예시 (OrgMemberRepository)
// 권한 변경 응답용: user 정보도 함께 페치 `@Query`("SELECT om FROM OrgMember om " + "JOIN FETCH om.user u " + "WHERE om.user.id = :userId " + "AND om.organization.id = :orgId " + "AND u.status = :userStatus") Optional<OrgMember> findByUserIdAndOrgIdWithUser(`@Param`("userId") Long userId, `@Param`("orgId") Long orgId, `@Param`("userStatus") UserStatus userStatus);As per coding guidelines, "JPA 사용 시 N+1 문제나 불필요한 쿼리가 발생하지 않는지" 확인이 필요합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/whereyouad/WhereYouAd/domains/organization/domain/service/OrgServiceImpl.java` around lines 210 - 221, The current retrieval uses orgMemberRepository.findByUserIdAndOrgId which does not fetch the associated User, causing a lazy-load when OrgConverter.toOrgMemberDTO(orgMember) accesses orgMember.getUser(); change the service to use a repository method that JOIN FETCHes the user (e.g., add and call findByUserIdAndOrgIdWithUser on OrgMemberRepository that joins FETCH om.user and filters by user status) or create a dedicated query returning the OrgMember with user eagerly loaded, then use that result for updateRole and OrgConverter.toOrgMemberDTO to avoid the extra select.
🤖 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/com/whereyouad/WhereYouAd/domains/organization/domain/service/OrgServiceImpl.java`:
- Around line 197-199: The method updateOrgMembersRole in OrgServiceImpl fetches
Organization organization but never checks its status, allowing role changes on
soft-deleted orgs; add a check after retrieving organization: if
organization.getStatus() == OrgStatus.DELETED then throw new
OrgHandler(OrgErrorCode.ORG_SOFT_DELETED) (same pattern as
getOrganizationDetail) so updates are blocked for soft-deleted organizations.
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/organization/presentation/docs/OrgControllerDocs.java`:
- Line 144: Update the ApiResponse for the ADMIN→MEMBER change in
OrgControllerDocs.java: replace the incorrect responseCode "401" with "400" and
adjust the description accordingly (the annotation on the ApiResponse for the
business-rule rejection in OrgControllerDocs should reflect Bad Request); locate
the ApiResponse entry in the OrgControllerDocs class that currently reads
responseCode = "401" and change it to responseCode = "400" so it matches the
ORG_400_4 error and actual response payload.
- Line 143: The `@ApiResponse` description for responseCode "200" in
OrgControllerDocs is incorrectly copied from getOrgMembersCount; update the
description for the 권한 변경 API to reflect that it returns the changed member info
(OrgMemberDTO). Locate the `@ApiResponse`(...) annotation in OrgControllerDocs for
the permission-change endpoint and replace the text "성공 (totalCount: 전체 멤버 수)"
with a concise message such as "성공 (변경된 멤버 정보 반환: OrgMemberDTO)" or equivalent
that mentions OrgMemberDTO.
- Line 152: The UpdateRole request parameter is missing `@Valid` so Bean
Validation on OrgRequest.UpdateRole.orgRole (annotated `@NotNull`) won't run;
update the controller method signature that accepts OrgRequest.UpdateRole (the
parameter currently declared as "@RequestBody OrgRequest.UpdateRole dto") to use
"@RequestBody `@Valid` OrgRequest.UpdateRole dto" so Spring triggers validation
(mirror the existing approach used in createOrganization/modifyOrganization).
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/organization/presentation/OrgController.java`:
- Around line 129-138: The updateOrgMembersRole controller is missing request
validation so OrgRequest.UpdateRole's `@NotNull` on orgRole isn't enforced; add
`@Valid` to the controller parameter (change the signature of updateOrgMembersRole
to accept `@RequestBody` `@Valid` OrgRequest.UpdateRole dto) so Spring performs
validation before calling orgService.updateOrgMembersRole and avoids null being
passed into orgMember.updateRole/OrgConverter.toOrgMemberDTO causing NPEs;
ensure any BindingResult or global exception handler already handles
MethodArgumentNotValidException appropriately.
---
Nitpick comments:
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/organization/domain/service/OrgServiceImpl.java`:
- Around line 210-221: The current retrieval uses
orgMemberRepository.findByUserIdAndOrgId which does not fetch the associated
User, causing a lazy-load when OrgConverter.toOrgMemberDTO(orgMember) accesses
orgMember.getUser(); change the service to use a repository method that JOIN
FETCHes the user (e.g., add and call findByUserIdAndOrgIdWithUser on
OrgMemberRepository that joins FETCH om.user and filters by user status) or
create a dedicated query returning the OrgMember with user eagerly loaded, then
use that result for updateRole and OrgConverter.toOrgMemberDTO to avoid the
extra select.
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/organization/persistence/repository/OrgMemberRepository.java`:
- Around line 43-48: Change the JPQL in OrgMemberRepository so it does not
compare the enum via the string literal 'ACTIVE'; update the query used by
findByUserIdAndOrgId (or add an overloaded method) to accept a UserStatus
parameter and bind it (e.g., "om.user.status = :status") and change the method
signature to include a UserStatus status parameter (call site will pass
UserStatus.ACTIVE). Ensure you reference the UserStatus enum and parameter name
(e.g., status) in the `@Param` annotation so om.user.status uses a typed parameter
instead of a hard-coded string.
| // 1. 조직 존재 여부 확인 | ||
| Organization organization = orgRepository.findById(orgId) | ||
| .orElseThrow(() -> new OrgHandler(OrgErrorCode.ORG_NOT_FOUND)); |
There was a problem hiding this comment.
소프트 삭제된 조직에 대한 권한 변경이 가능합니다 — OrgStatus.DELETED 체크가 누락되었습니다.
Organization organization 변수를 할당했지만 이후 한 번도 사용하지 않습니다. 기존 getOrganizationDetail 메서드를 보면:
if (organization.getStatus() == OrgStatus.DELETED) {
throw new OrgHandler(OrgErrorCode.ORG_SOFT_DELETED);
}이 체크가 존재합니다. 현재 updateOrgMembersRole에서는 이 체크가 없어서 소프트 삭제된 조직(status = DELETED)에서도 멤버 권한이 변경 가능합니다.
🐛 소프트 삭제 체크 추가 제안
Organization organization = orgRepository.findById(orgId)
.orElseThrow(() -> new OrgHandler(OrgErrorCode.ORG_NOT_FOUND));
+
+// Soft Delete 된 조직이면 예외처리
+if (organization.getStatus() == OrgStatus.DELETED) {
+ throw new OrgHandler(OrgErrorCode.ORG_SOFT_DELETED);
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/organization/domain/service/OrgServiceImpl.java`
around lines 197 - 199, The method updateOrgMembersRole in OrgServiceImpl
fetches Organization organization but never checks its status, allowing role
changes on soft-deleted orgs; add a check after retrieving organization: if
organization.getStatus() == OrgStatus.DELETED then throw new
OrgHandler(OrgErrorCode.ORG_SOFT_DELETED) (same pattern as
getOrganizationDetail) so updates are blocked for soft-deleted organizations.
| description = "맴버 권한 변경을 요청한 유저의 권한이 ADMIN인 경우 실행이 가능합니다. memberId에 해당하는 맴버의 권한을 변경시킵니다." | ||
| ) | ||
| @ApiResponses({ | ||
| @ApiResponse(responseCode = "200", description = "성공 (totalCount: 전체 멤버 수)"), |
There was a problem hiding this comment.
200 응답 설명이 잘못된 값으로 복붙되었습니다.
"성공 (totalCount: 전체 멤버 수)" 는 getOrgMembersCount API의 설명을 그대로 가져온 것입니다. 권한 변경 API의 성공 응답에는 변경된 멤버 정보(OrgMemberDTO)가 반환되므로 설명을 수정해야 합니다.
📝 수정 제안
-@ApiResponse(responseCode = "200", description = "성공 (totalCount: 전체 멤버 수)"),
+@ApiResponse(responseCode = "200", description = "성공 (변경된 멤버 정보 반환)"),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @ApiResponse(responseCode = "200", description = "성공 (totalCount: 전체 멤버 수)"), | |
| `@ApiResponse`(responseCode = "200", description = "성공 (변경된 멤버 정보 반환)"), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/organization/presentation/docs/OrgControllerDocs.java`
at line 143, The `@ApiResponse` description for responseCode "200" in
OrgControllerDocs is incorrectly copied from getOrgMembersCount; update the
description for the 권한 변경 API to reflect that it returns the changed member info
(OrgMemberDTO). Locate the `@ApiResponse`(...) annotation in OrgControllerDocs for
the permission-change endpoint and replace the text "성공 (totalCount: 전체 멤버 수)"
with a concise message such as "성공 (변경된 멤버 정보 반환: OrgMemberDTO)" or equivalent
that mentions OrgMemberDTO.
| ) | ||
| @ApiResponses({ | ||
| @ApiResponse(responseCode = "200", description = "성공 (totalCount: 전체 멤버 수)"), | ||
| @ApiResponse(responseCode = "401", description = "잘못된 요청을 보낸 경우(ADMIN의 권한 변경)"), |
There was a problem hiding this comment.
잘못된 HTTP 상태 코드: 401이 아니라 400이 맞습니다.
HTTP 401 Unauthorized는 인증(Authentication) 실패를 의미합니다. ADMIN→MEMBER 변경을 거부하는 것은 비즈니스 로직 규칙 위반이므로 400 Bad Request가 올바릅니다. PR 스크린샷에서도 실제 에러 응답이 "status": "Bad Request" (ORG_400_4)임을 확인할 수 있습니다.
예시:
401: 토큰이 없거나 만료된 경우 (인증 실패)400: ADMIN을 MEMBER로 변경하려는 잘못된 요청 (비즈니스 규칙 위반)
📝 수정 제안
-@ApiResponse(responseCode = "401", description = "잘못된 요청을 보낸 경우(ADMIN의 권한 변경)"),
+@ApiResponse(responseCode = "400", description = "잘못된 요청을 보낸 경우(ADMIN의 권한 변경)"),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @ApiResponse(responseCode = "401", description = "잘못된 요청을 보낸 경우(ADMIN의 권한 변경)"), | |
| `@ApiResponse`(responseCode = "400", description = "잘못된 요청을 보낸 경우(ADMIN의 권한 변경)"), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/organization/presentation/docs/OrgControllerDocs.java`
at line 144, Update the ApiResponse for the ADMIN→MEMBER change in
OrgControllerDocs.java: replace the incorrect responseCode "401" with "400" and
adjust the description accordingly (the annotation on the ApiResponse for the
business-rule rejection in OrgControllerDocs should reflect Bad Request); locate
the ApiResponse entry in the OrgControllerDocs class that currently reads
responseCode = "401" and change it to responseCode = "400" so it matches the
ORG_400_4 error and actual response payload.
| @AuthenticationPrincipal(expression = "userId") Long userId, | ||
| @PathVariable Long orgId, | ||
| @PathVariable Long memberId, | ||
| @RequestBody OrgRequest.UpdateRole dto |
There was a problem hiding this comment.
@RequestBody에 @Valid가 누락되었습니다 — @NotNull 검증이 동작하지 않습니다.
AI 요약에 따르면 OrgRequest.UpdateRole의 orgRole 필드에 @NotNull이 선언되어 있습니다. 그런데 @Valid가 없으면 Spring은 Bean Validation을 실행하지 않아서, orgRole이 null인 요청이 그대로 서비스 레이어까지 내려갑니다. 이는 NPE나 예상치 못한 DB 오류로 이어질 수 있습니다.
다른 메서드들(예: Line 24 createOrganization, Line 62 modifyOrganization)은 모두 @RequestBody @Valid``를 사용하고 있습니다.
🛡️ 수정 제안
-@RequestBody OrgRequest.UpdateRole dto
+@RequestBody `@Valid` OrgRequest.UpdateRole dto🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/organization/presentation/docs/OrgControllerDocs.java`
at line 152, The UpdateRole request parameter is missing `@Valid` so Bean
Validation on OrgRequest.UpdateRole.orgRole (annotated `@NotNull`) won't run;
update the controller method signature that accepts OrgRequest.UpdateRole (the
parameter currently declared as "@RequestBody OrgRequest.UpdateRole dto") to use
"@RequestBody `@Valid` OrgRequest.UpdateRole dto" so Spring triggers validation
(mirror the existing approach used in createOrganization/modifyOrganization).
| @PatchMapping("/members/{orgId}/{memberId}") | ||
| public ResponseEntity<DataResponse<OrgResponse.OrgMemberDTO>> updateOrgMembersRole( | ||
| @AuthenticationPrincipal(expression = "userId") Long userId, | ||
| @PathVariable Long orgId, | ||
| @PathVariable Long memberId, | ||
| @RequestBody OrgRequest.UpdateRole dto | ||
| ) { | ||
| OrgResponse.OrgMemberDTO response = orgService.updateOrgMembersRole(userId, orgId, memberId, dto); | ||
| return ResponseEntity.ok(DataResponse.from(response)); | ||
| } |
There was a problem hiding this comment.
🚨 @Valid 누락 — null orgRole이 입력되면 런타임 NPE가 발생합니다!
@RequestBody OrgRequest.UpdateRole dto에 @Valid가 없습니다. 다른 엔드포인트들을 보면:
@PostMapping("/create")→@RequestBody@ValidOrgRequest.Create request✓@PatchMapping("/{orgId}")→@RequestBody@ValidOrgRequest.Update request✓
현재 엔드포인트는 @Valid가 없어서 OrgRequest.UpdateRole의 @NotNull(message = "역할은 필수입니다.") 검증이 전혀 실행되지 않습니다.
클라이언트가 {"orgRole": null} 또는 {} 를 전송하면 다음 흐름이 발생합니다:
dto.orgRole()→nullorgMember.updateRole(null)→this.role = null(메모리)OrgConverter.toOrgMemberDTO(orgMember)→orgMember.getRole().name()💥 NullPointerException
🐛 수정 방법
- `@RequestBody` OrgRequest.UpdateRole dto
+ `@RequestBody` `@Valid` OrgRequest.UpdateRole dto🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/whereyouad/WhereYouAd/domains/organization/presentation/OrgController.java`
around lines 129 - 138, The updateOrgMembersRole controller is missing request
validation so OrgRequest.UpdateRole's `@NotNull` on orgRole isn't enforced; add
`@Valid` to the controller parameter (change the signature of updateOrgMembersRole
to accept `@RequestBody` `@Valid` OrgRequest.UpdateRole dto) so Spring performs
validation before calling orgService.updateOrgMembersRole and avoids null being
passed into orgMember.updateRole/OrgConverter.toOrgMemberDTO causing NPEs;
ensure any BindingResult or global exception handler already handles
MethodArgumentNotValidException appropriately.
| @AuthenticationPrincipal(expression = "userId") Long userId, | ||
| @PathVariable Long orgId, | ||
| @PathVariable Long memberId, | ||
| @RequestBody OrgRequest.UpdateRole dto |
There was a problem hiding this comment.
P3: Controller 메서드에서 OrgRequest.UpdateRole 필드에 Valid 어노테이션 달아서 NotNull 검사 적용되게 해주면 좋을 것 같아요!
ojy0903
left a comment
There was a problem hiding this comment.
P4: 고생하셨어요! 제가 보기엔 ADMIN 인 사용자를 MEMBER 로 바꾸지 못하면 문제가 생길 수도 있을 것 같은데, 아예 OrgRole 에 CREATOR 같이 최고 권한 역할을 만드는게 좋을까요? 아니면 지금처럼 ADMIN 은 MEMBER 로 바꾸지 못하도록 하되 화면 UI 에서 사용자에게 경고를 주는 방식으로 하는 게 맞을까요? 지민님 의견도 들어봐야 될 듯 합니다...
저도 뭔가 계속 새로운 권한을 만들면 끝이 없을 것 같아서 고민 중이었습니다..! 최고 권한 역할도 좋습니다 뭔가 ADMIN도 강등할 수 있는 방법이 필요할 것 같긴 합니다! |
There was a problem hiding this comment.
P3: 고생하셨습니다! 제 생각에도 ADMIN이 MEMBER 권한으로 변경될 수 없다면 문제가 있을 것 같습니다.. 해당 조직 내 ADMIN이 두 명 이상 존재한다면 ADMIN -> MEMBER를 가능하게 하는 건 어떨까요?
만약 추가로 더 구현한다면, 조직 내 ADMIN이 오직 한 명이면 관리자를 다른 이로 임명하게끔 구현해도 될 듯 합니다! (+ ADMIN 권한인 유저 삭제(추방)하는 것도 비슷하게 고민해볼 수 있을 것 같습니다..)
📌 관련 이슈
🚀 개요
📄 작업 내용
📸 스크린샷 / 테스트 결과 (선택)
✅ 체크리스트
🔍 리뷰 포인트 (Review Points)
Summary by CodeRabbit
새로운 기능