Conversation
…록 변경 - 도메인 규칙이 아니라 정보를 조회해서 반환하는 것 - 따라서 DomainService가 아닌 repository에서 응답하도록 변경
📝 WalkthroughWalkthrough이 PR은 관리자(Admin) 엔티티 및 도메인 로직, 애플리케이션 서비스, DTO, 리포지토리 구현체를 추가합니다. 어드민 컨트롤러를 신규 생성하고, 기존 Organization과 Organizer 기능을 재구조화하며, 보안 설정(JWT, 로깅 마스킹, 화이트리스트)을 업데이트합니다. 광범위한 유닛 및 통합 테스트도 함께 추가됩니다. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 근거: Admin 엔티티의 검증 로직(username 한글 문자 제외, 길이 제한, 비밀번호 검증)과 접근 제어(createBy 메서드)가 일정한 복잡도를 갖고 있으며, AdminController(151줄)는 8개의 엔드포인트를 노출합니다. 다만 DTO와 리포지토리 구현체는 반복적인 패턴이므로 검토 부담을 낮춥니다. 기존 클래스 수정(AdminDetails 생성자 변경, JwtProvider.createAdminToken 추가, OrganizationService 메서드 제거)이 여러 곳에서 발생하고, 광범위한 테스트(AdminControllerTest 472줄)가 포함되어 중간 수준의 검토 노력이 필요합니다. Possibly related PRs
🚥 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. ✨ 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 |
Summary of ChangesHello, 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! 이 풀 리퀘스트는 MVP 설계에 따라 Admin 백오피스 기능을 도입하여 시스템 관리 역량을 강화합니다. Admin 계정 관리, 조직 생성 및 조회, 오거나이저 생성 및 사용자명 업데이트와 같은 핵심 관리 기능이 추가되었습니다. 또한, 새로운 도메인에 DDD 아키텍처 스타일을 적용하기 위한 기반을 마련하고 관련 코드 컨벤션을 문서화하여 향후 확장성과 유지보수성을 높였습니다. Highlights
Changelog
Activity
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
|
|
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8155e62c70
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+32
to
+33
| String encodedPassword = passwordEncoder.encode(request.password()); | ||
| Admin admin = Admin.createBy(actor, request.username(), encodedPassword); |
There was a problem hiding this comment.
createAdmin hashes request.password() before any raw-input validation, so "" is transformed into a non-empty bcrypt string and then passes Admin.createBy's password checks (which now run on the hash, not the original input). This allows creating admin accounts with an empty password, and null passwords will surface as encoder runtime errors (500) instead of a controlled 4xx validation response.
Useful? React with 👍 / 👎.
Comment on lines
+30
to
+31
| public void validatePasswordByte(String rawPassword) { | ||
| if (rawPassword.getBytes(StandardCharsets.UTF_8).length > MAX_PASSWORD_BYTE_LENGTH) { |
There was a problem hiding this comment.
validatePasswordByte only checks byte length and immediately dereferences rawPassword, so null inputs throw a runtime exception and empty-string passwords pass validation; after that, createOrganizer hashes the empty password and Organizer.createBy accepts the hash as non-empty. On /admin/organizers, this regresses password policy enforcement and can return 500 for null payloads instead of a client error.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This PR introduces a new DDD architecture based on MVP design and adds admin backoffice functionality. While the core authentication and authorization logic appears sound and the PR generally adheres to the new DDD rules with thorough test coverage, a significant security vulnerability has been identified: sensitive information, specifically plain-text passwords, are not properly masked in logging filters for several new endpoints. This could lead to credential leakage in application logs. It is recommended to add the missing paths to the masking configuration and ensure the masking logic is correctly applied in all logging filters. Additionally, please review concerns regarding architectural scalability (e.g., pagination), separation of controller responsibilities, and the password validation logic.
Comment on lines
+51
to
+52
| new MaskingPath("/places/accesses/login", "POST"), | ||
| new MaskingPath("/admin/login", "POST") |
There was a problem hiding this comment.
The LoggingFilter is missing masking for the POST /admin and POST /admin/organizers endpoints. These endpoints receive request bodies containing plain-text passwords (AdminRequest and AdminOrganizerCreateRequest). Since they are not included in BODY_MASKING_PATH, the passwords will be logged in plain text in production and development environments, which is a security risk.
new MaskingPath("/places/accesses/login", "POST"),
new MaskingPath("/admin/login", "POST"),
new MaskingPath("/admin", "POST"),
new MaskingPath("/admin/organizers", "POST")
Comment on lines
+30
to
+36
| public void validatePasswordByte(String rawPassword) { | ||
| if (rawPassword.getBytes(StandardCharsets.UTF_8).length > MAX_PASSWORD_BYTE_LENGTH) { | ||
| throw new BadRequestException( | ||
| String.format("비밀번호는 %d바이트를 초과할 수 없습니다.", MAX_PASSWORD_BYTE_LENGTH) | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
validatePasswordByte 메서드에서 원시 비밀번호(raw password)의 바이트 길이를 60으로 제한하고 있습니다. BCrypt 해시 결과가 60자인 것은 맞지만, 이는 해싱된 결과의 길이지 원본 비밀번호의 길이 제한이 아닙니다. 이 검증으로 인해 60바이트를 초과하는 (그러나 보안상 더 강력할 수 있는) 정상적인 비밀번호를 가진 사용자가 가입하지 못하는 문제가 발생할 수 있습니다. Organizer 엔티티의 password 필드는 이미 VARCHAR(100)으로 충분한 길이를 가지고 있으므로, 원시 비밀번호에 대한 바이트 길이 검증은 제거하는 것이 맞아 보입니다.
Comment on lines
+41
to
+42
| new MaskingPath("/staff/password", "PATCH"), | ||
| new MaskingPath("/admin/login", "POST") |
There was a problem hiding this comment.
The LocalLoggingFilter defines a BODY_MASKING_PATH set but fails to apply any masking logic before logging the requestBody in the doFilterInternal method. This results in all request bodies, including those containing sensitive information like passwords, being logged in plain text in environments where this filter is active (e.g., local development). Additionally, the new sensitive endpoints /admin and /admin/organizers are missing from the masking set.
new MaskingPath("/staff/password", "PATCH"),
new MaskingPath("/admin/login", "POST"),
new MaskingPath("/admin", "POST"),
new MaskingPath("/admin/organizers", "POST")
Comment on lines
+48
to
+52
| public AdminDuplicateResponse checkUsernameDuplicate(String username) { | ||
| return AdminDuplicateResponse.from( | ||
| adminRepository.existsActiveByUsername(username) | ||
| ); | ||
| } |
There was a problem hiding this comment.
checkUsernameDuplicate 메서드는 읽기 전용 작업이지만 @Transactional(readOnly = true) 어노테이션이 누락되었습니다. code-style.md의 7.5.1 규칙에 따라 읽기 전용 메서드에는 해당 어노테이션을 붙여주는 것이 좋습니다. 이렇게 하면 JPA에게 불필요한 쓰기 관련 작업을 생략하도록 힌트를 주어 성능을 최적화할 수 있고, 코드의 의도를 더 명확하게 표현할 수 있습니다.
@Transactional(readOnly = true)
public AdminDuplicateResponse checkUsernameDuplicate(String username) {
return AdminDuplicateResponse.from(
adminRepository.existsActiveByUsername(username)
);
}References
- 읽기 전용 메서드에는
@Transactional(readOnly = true)또는 어노테이션을 생략한다. (7.5.1 @transactional 사용) (link)
| ); | ||
|
|
||
| default boolean existsActiveByUsername(String username) { | ||
| return existsActiveByUsername(username, LocalDateTime.of(9999, 1, 1, 0, 0)) > 0; |
There was a problem hiding this comment.
sentinel 날짜로 사용된 LocalDateTime.of(9999, 1, 1, 0, 0) 값이 하드코딩되어 있습니다. 이 값은 네이티브 쿼리에서도 논리적으로 중요한 역할을 하는 "매직 넘버"에 해당합니다. 가독성과 유지보수성을 높이기 위해 인터페이스 상단에 private static final LocalDateTime SENTINEL_DATE_TIME = LocalDateTime.of(9999, 1, 1, 0, 0); 와 같이 상수로 추출하는 것을 권장합니다.
Suggested change
| return existsActiveByUsername(username, LocalDateTime.of(9999, 1, 1, 0, 0)) > 0; | |
| return existsActiveByUsername(username, SENTINEL_DATE_TIME) > 0; |
| @RequestMapping("/admin") | ||
| @RequiredArgsConstructor | ||
| @Tag(name = "어드민", description = "어드민 백오피스 API") | ||
| public class AdminController { |
There was a problem hiding this comment.
AdminController가 Admin 계정 관리 외에 Organization과 Organizer 생성 및 관리 책임을 함께 가지고 있습니다. 이는 단일 책임 원칙(SRP)에 위배될 수 있으며, 향후 어드민 기능이 확장될수록 클래스가 비대해져 유지보수가 어려워질 수 있습니다.
[관점 A: 현재 구조 유지] MVP 단계이고 어드민 기능이 많지 않다면, 하나의 컨트롤러에서 관리하는 것이 파일 수를 줄여 더 간단할 수 있습니다.
[관점 B: 리소스 기반 분리] 장기적인 관점에서 AdminOrganizationController, AdminOrganizerController 와 같이 리소스별로 컨트롤러를 분리하면 각 컨트롤러의 책임이 명확해지고 확장성이 좋아집니다.
결론: 현재는 기능이 많지 않아 괜찮아 보이지만, 팀의 설계 방향에 따라 리팩토링을 고려해보시는 것을 추천합니다. 이는 festabook 스타일 가이드의 3.2 SRP & Cohesion, 3.4 Feedback Style: Trade-offs 원칙에 따른 제안입니다.
Comment on lines
+23
to
+29
| public OrganizationResponses getAllOrganizations() { | ||
| return new OrganizationResponses( | ||
| organizationRepository.findAll().stream() | ||
| .map(OrganizationResponse::from) | ||
| .toList() | ||
| ); | ||
| } |
There was a problem hiding this comment.
getAllOrganizations 메서드가 findAll()을 통해 모든 Organization을 조회하고 있습니다. 현재는 Organization 수가 적어 문제가 되지 않을 수 있지만, 데이터가 수백, 수천 개로 늘어날 경우 애플리케이션에 심각한 성능 저하 및 메모리 부족(OOM)을 유발할 수 있습니다. 이는 festabook 스타일 가이드의 2.1 Memory Management, 2.2 Broaden Horizons 원칙에서 경고하는 패턴입니다.
[제안] Pageable을 사용하여 페이지네이션을 구현하는 것을 강력히 권장합니다.
// 예시
import org.springframework.data.domain.Page;
import org.springframework.data.domain.Pageable;
public Page<OrganizationResponse> getAllOrganizations(Pageable pageable) {
return organizationRepository.findAll(pageable)
.map(OrganizationResponse::from);
}
Comment on lines
+43
to
+50
| @Transactional(readOnly = true) | ||
| public AdminOrganizerResponses getAllOrganizers() { | ||
| return new AdminOrganizerResponses( | ||
| organizerRepository.findAll().stream() | ||
| .map(AdminOrganizerResponse::from) | ||
| .toList() | ||
| ); | ||
| } |
There was a problem hiding this comment.
getAllOrganizers 메서드가 findAll()을 통해 모든 Organizer를 조회하고 있습니다. Organization과 마찬가지로, Organizer 데이터가 많아질 경우 성능 문제 및 메모리 부족(OOM)이 발생할 수 있습니다. 이는 festabook 스타일 가이드의 2.1 Memory Management, 2.2 Broaden Horizons 원칙에서 경고하는 패턴입니다.
[제안] Pageable을 사용하여 페이지네이션을 구현하는 것을 권장합니다.
// 예시
import org.springframework.data.domain.Page;
import org.springframework.data.domain.Pageable;
public Page<AdminOrganizerResponse> getAllOrganizers(Pageable pageable) {
return organizerRepository.findAll(pageable)
.map(AdminOrganizerResponse::from);
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (15)
src/main/java/com/daedan/festabook/admin/dto/AdminDuplicateResponse.java (1)
3-10: Swagger 문서화 어노테이션 추가를 권장드립니다.코딩 가이드라인에 따르면 Controller와 DTO 클래스에 Swagger 어노테이션을 작성해야 합니다. 동일 PR 내 다른 DTO들(예:
AdminOrganizerUsernameUpdateRequest)에는@Schema어노테이션이 적용되어 있어 일관성을 위해 추가하면 좋겠습니다.📝 Swagger 어노테이션 추가 제안
package com.daedan.festabook.admin.dto; +import io.swagger.v3.oas.annotations.media.Schema; + +@Schema(description = "어드민 아이디 중복 확인 응답") public record AdminDuplicateResponse( + `@Schema`(description = "중복 여부", example = "false") boolean duplicated ) { public static AdminDuplicateResponse from(boolean duplicated) { return new AdminDuplicateResponse(duplicated); } }As per coding guidelines: "Write Swagger annotations only on Controller classes and DTO classes."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/daedan/festabook/admin/dto/AdminDuplicateResponse.java` around lines 3 - 10, Add Swagger `@Schema` annotations to the AdminDuplicateResponse record to match other DTOs: annotate the record (AdminDuplicateResponse) and its component/field (duplicated) with descriptive `@Schema` metadata (e.g., description and example) and import io.swagger.v3.oas.annotations.media.Schema; ensure the static factory method from(boolean) remains unchanged and follow the same annotation style used in AdminOrganizerUsernameUpdateRequest for consistency.src/main/java/com/daedan/festabook/organizer/dto/AdminOrganizerUsernameUpdateRequest.java (1)
5-9: 입력값 검증 어노테이션 추가를 고려해보세요.
username필드에@NotBlank또는@NotNull등의 Bean Validation 어노테이션이 없어서 빈 문자열이나 null 값이 서비스 레이어까지 전달될 수 있습니다.트레이드 오프:
- 옵션 A (권장): DTO에
@NotBlank추가 → 컨트롤러 레벨에서 조기 검증, 명확한 400 Bad Request 응답- 옵션 B: 서비스/도메인 레이어에서 검증 → 검증 로직 집중화, 단 에러 응답 형식이 달라질 수 있음
MVP 단계에서는 컨트롤러 레벨 검증이 빠른 피드백을 제공하므로 옵션 A를 권장드립니다.
✅ 검증 어노테이션 추가 제안
package com.daedan.festabook.organizer.dto; import io.swagger.v3.oas.annotations.media.Schema; +import jakarta.validation.constraints.NotBlank; public record AdminOrganizerUsernameUpdateRequest( + `@NotBlank`(message = "아이디는 필수입니다") `@Schema`(description = "변경할 아이디", example = "newUsername") String username ) { }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/daedan/festabook/organizer/dto/AdminOrganizerUsernameUpdateRequest.java` around lines 5 - 9, The DTO AdminOrganizerUsernameUpdateRequest currently exposes the username record component without validation; add a Bean Validation annotation (e.g., `@NotBlank`) to the username component to prevent null/empty values from reaching the service layer, and import javax.validation.constraints.NotBlank (or jakarta.validation.constraints.NotBlank depending on project setup); keep the existing `@Schema` and ensure controllers have `@Valid` on the request parameter so validation is enforced.src/main/java/com/daedan/festabook/organizer/dto/AdminOrganizerCreateRequest.java (1)
5-18: 입력 검증 어노테이션 추가를 고려해 보세요현재 DTO에 Bean Validation 어노테이션(
@NotBlank,@NotNull등)이 없습니다. 도메인 레이어에서 검증을 수행하고 있는 것으로 보이지만, 컨트롤러 레벨에서 빠르게 실패(fail-fast)하는 것이 일반적으로 권장됩니다.장점 비교:
- 도메인에서만 검증 (현재): 검증 로직이 한 곳에 집중되어 관리가 단순합니다. 단, 잘못된 요청이 서비스/도메인 레이어까지 도달합니다.
- DTO + 도메인 이중 검증: 잘못된 요청을 빨리 거부하여 불필요한 처리를 줄이고, API 문서에 필수 필드가 명확히 표시됩니다. 단, 검증 로직이 분산될 수 있습니다.
MVP 단계에서는 현재 방식도 충분하지만, 추후 API 문서화와 클라이언트 피드백 개선을 위해 기본적인
@NotBlank정도는 추가하는 것을 고려해 보시기 바랍니다.♻️ 검증 어노테이션 추가 예시
package com.daedan.festabook.organizer.dto; import io.swagger.v3.oas.annotations.media.Schema; +import jakarta.validation.constraints.NotBlank; +import jakarta.validation.constraints.NotNull; public record AdminOrganizerCreateRequest( `@Schema`(description = "조직 관리자 아이디", example = "admin") + `@NotBlank` String username, `@Schema`(description = "조직 관리자 비밀번호", example = "1234") + `@NotBlank` String password, `@Schema`(description = "조직 관리자 이름", example = "홍길동") + `@NotBlank` String name, `@Schema`(description = "조직 ID", example = "1") + `@NotNull` Long organizationId ) { }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/daedan/festabook/organizer/dto/AdminOrganizerCreateRequest.java` around lines 5 - 18, Add Bean Validation annotations to the AdminOrganizerCreateRequest record so controller-level validation fails fast: annotate username, password, and name with `@NotBlank` and organizationId with `@NotNull` (keep existing `@Schema`), and add the corresponding imports (javax.validation.constraints.NotBlank, javax.validation.constraints.NotNull) so Spring/Bean Validation can validate requests before reaching the domain layer.src/main/java/com/daedan/festabook/organizer/domain/Organizer.java (1)
137-145: 코드 중복에 대한 선택적 리팩터링 고려
validateUpdatableUsernameBy와validateCreatableBy(127-135라인)의 구현이 동일합니다. 현재로서는 두 가지 접근 방식이 있습니다:
- 옵션 A (현재 유지): 추후 username 수정 권한이 생성 권한과 분리될 가능성이 있다면 현재처럼 별도 메서드로 유지하는 것이 확장성 측면에서 유리합니다.
- 옵션 B (통합): 권한 정책이 동일하게 유지될 것이 확실하다면,
validateAdminOnly(Actor actor)같은 공통 메서드로 추출하여 중복을 제거할 수 있습니다.현 시점에서는 MVP 단계이므로 옵션 A를 유지하되, 권한 정책이 안정화되면 리팩터링을 고려해 보시는 것이 좋겠습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/daedan/festabook/organizer/domain/Organizer.java` around lines 137 - 145, The two identical methods validateUpdatableUsernameBy(Actor actor) and validateCreatableBy(Actor actor) duplicate the same admin-only check; extract the shared logic into a single helper method (e.g., validateAdminOnly(Actor actor)) that throws InternalServerException when actor is null and ForbiddenException when actor is non-admin, then have both validateUpdatableUsernameBy and validateCreatableBy delegate to validateAdminOnly(Actor) to remove duplication while keeping method names for future divergence.src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java (1)
88-99: roles 파라미터에 대한 null 안전성 검토기존
createOrganizerToken과createStaffToken메서드에서는festivalIds에 대해Objects.requireNonNullElse(festivalIds, Set.of())를 사용하여 null 안전성을 보장하고 있습니다.그러나 새로 추가된
createAdminToken에서는roles파라미터에 대한 null 처리가 없습니다. 만약roles가 null로 전달되면 JWT claim에 null이 저장되어 토큰 파싱 또는 권한 검증 시 예상치 못한 동작이 발생할 수 있습니다.선택지:
- 옵션 A (방어적 코딩):
Objects.requireNonNullElse(roles, Set.of())로 기본값 처리 - 호출자의 실수를 방어하지만, 문제를 숨길 수 있음- 옵션 B (명시적 계약):
Objects.requireNonNull(roles, "roles must not be null")로 빠른 실패 - 호출자에게 명확한 계약을 강제현재 Admin의 역할이 필수적이라면 옵션 B가 더 적합할 수 있습니다.
📝 옵션 B 제안 (명시적 계약)
public String createAdminToken(String username, Set<RoleType> roles) { + Objects.requireNonNull(roles, "roles must not be null"); Date now = new Date(); Date expiry = new Date(now.getTime() + validityInMilliseconds);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java` around lines 88 - 99, The createAdminToken method lacks null-safety for its roles parameter; add a null-check at the start of createAdminToken (e.g., use Objects.requireNonNull(roles, "roles must not be null") if you want fast-fail contract or Objects.requireNonNullElse(roles, Set.of()) for a defensive default) so that the CLAIM_ROLES_TYPE claim never receives a null value when building the JWT.src/main/java/com/daedan/festabook/admin/dto/AdminResponse.java (1)
5-8: Swagger 문서화를 위한@Schema어노테이션 누락코딩 가이드라인에 따르면 DTO 클래스에는 Swagger 어노테이션을 작성해야 합니다.
AdminLoginRequest에서는@Schema어노테이션이 적절히 적용되어 있는 반면, 이 Response DTO에서는 누락되어 있습니다.API 문서의 일관성과 클라이언트 개발자의 이해를 돕기 위해, Response DTO에도 필드에 대한 설명을 추가하는 것을 권장합니다.
📝 제안하는 수정
package com.daedan.festabook.admin.dto; import com.daedan.festabook.admin.domain.Admin; +import io.swagger.v3.oas.annotations.media.Schema; public record AdminResponse( + `@Schema`(description = "Admin 아이디", example = "admin01") String username ) {As per coding guidelines: "Write Swagger annotations only on Controller classes and DTO classes."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/daedan/festabook/admin/dto/AdminResponse.java` around lines 5 - 8, The AdminResponse record is missing Swagger annotations; add an `@Schema` annotation to the record component `username` (e.g., `@Schema`(description = "관리자 사용자명" or appropriate description) on the `username` component in `AdminResponse`) and import io.swagger.v3.oas.annotations.media.Schema so the response DTO is documented consistently like `AdminLoginRequest`.src/main/java/com/daedan/festabook/admin/dto/AdminLoginResponse.java (1)
3-6: Swagger 문서화를 위한@Schema어노테이션 추가 권장
AdminLoginRequest와의 일관성을 위해 Response DTO에도@Schema어노테이션을 추가하는 것이 좋습니다. 특히accessToken필드는 클라이언트가 이해하기 쉽도록 설명이 필요합니다.📝 제안하는 수정
package com.daedan.festabook.admin.dto; +import io.swagger.v3.oas.annotations.media.Schema; + public record AdminLoginResponse( + `@Schema`(description = "JWT 액세스 토큰", example = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9...") String accessToken ) {As per coding guidelines: "Write Swagger annotations only on Controller classes and DTO classes."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/daedan/festabook/admin/dto/AdminLoginResponse.java` around lines 3 - 6, Add Swagger `@Schema` annotations to the AdminLoginResponse record to match AdminLoginRequest: annotate the accessToken component with `@Schema`(description = "JWT access token returned after successful admin login", example = "eyJhbGciOi...") and (optionally) annotate the record itself with a brief `@Schema`(description = "Admin login response DTO"); import io.swagger.v3.oas.annotations.media.Schema and apply the annotation to the accessToken symbol in the AdminLoginResponse record declaration.src/test/java/com/daedan/festabook/organizer/domain/OrganizerTest.java (1)
288-304:updateUsernameBy도 권한 성공 케이스를 한 번 분리해 두면 스위트가 더 읽기 쉬워집니다.여기
성공()은 상태 변경 검증 역할을 잘 하고 있어서, 권한 매트릭스 쪽은성공_ADMIN_접근_권한_있음()을doesNotThrowAnyException()만 확인하는 테스트로 따로 두는 편이 인접한createBy/updateNameBy/updatePasswordBy와 구조가 맞습니다. 지금처럼 이 케이스가 빠져 있으면 권한 규칙 회귀를 비교할 때 이 스위트만 한 칸 비어 보입니다.Based on learnings, "Domain behavior method tests (createBy, updateXxxBy, deleteBy): first test must be named '성공()' validating all result fields. Same nested class success-permission tests use doesNotThrowAnyException() only, skip value re-validation."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/daedan/festabook/organizer/domain/OrganizerTest.java` around lines 288 - 304, Add a separate permission-only test in the updateUsernameBy nested class that mirrors the pattern used for createBy/updateNameBy/updatePasswordBy: create a test named like 성공_ADMIN_접근_권한_있음() which uses ActorFixture.admin() and calls organizer.updateUsernameBy(actor, username) wrapped in an assertion that it does not throw (e.g., assertDoesNotThrow / doesNotThrowAnyException), leaving the existing 성공() to continue validating state changes; this keeps permission checks isolated from result-field assertions.src/main/java/com/daedan/festabook/organizer/domain/OrganizerRepository.java (1)
8-8:findAll()계약은 MVP를 넘어서면 바로 확장성 병목이 될 수 있습니다.백오피스의 “전체 조회” 요구가 있더라도 repository를
List전체 적재로 고정해 두면, 데이터가 커질수록 응답 지연과 메모리 사용량이 같이 커집니다.
옵션 A는 지금부터 페이지네이션 계약으로 바꿔 확장 안전성을 확보하는 방식이고, 옵션 B는 MVP 범위에서 전체 조회를 유지하되 최대 건수/정렬 기준을 명시하고 운영 상한을 두는 방식입니다.As per coding guidelines, "
Stream.collect()로 대량의 데이터를 힙에 로드하거나,static컬렉션에 데이터가 무한히 쌓이는 패턴(OOM 위험)을 찾아내세요."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/daedan/festabook/organizer/domain/OrganizerRepository.java` at line 8, The current OrganizerRepository.findAll() returning List<Organizer> is an OOM/scalability risk; change the repository contract to a pageable/streaming-friendly API (e.g., replace findAll() with a paginated method like findAll(Pageable) or a streaming/iterator-based method such as streamAllOrganizers()) or, if you must keep an eager API for MVP, enforce an explicit max limit and sort parameters (e.g., findAllWithLimit(int limit, Sort sort)) and document/validate the limit to prevent unbounded loads; update all callers of OrganizerRepository.findAll() to use the new method and add input validation and tests to ensure limits/pagination are enforced.src/test/java/com/daedan/festabook/organizer/dto/AdminOrganizerCreateRequestFixture.java (1)
19-24: 전체 파라미터 팩토리는createCustom()으로 맞추는 편이 좋겠습니다.지금도 동작은 하지만, 이 저장소의 Fixture 규칙에서는 “모든 필드를 받는 메서드”를
createCustom()으로 통일하고 있습니다. 여기만create(...)로 남겨두면 다른 Fixture와 검색 규칙이 어긋나서 테스트 코드의 예측 가능성이 조금씩 떨어집니다.Based on learnings, "Fixture create() methods use overloading for parameter variations. All-parameters method named createCustom()."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/daedan/festabook/organizer/dto/AdminOrganizerCreateRequestFixture.java` around lines 19 - 24, The all-parameters factory method currently named create(...) in AdminOrganizerCreateRequestFixture should be renamed to createCustom(...) to match the repository's Fixture convention; update the method signature from create(String username, String password, String name, Long organizationId) to createCustom(...) and update any internal overloaded create(...) methods to delegate to the new createCustom(...) as needed, then search and replace usages in tests to call AdminOrganizerCreateRequestFixture.createCustom(...) so callers and other fixtures remain consistent.src/test/java/com/daedan/festabook/organizer/domain/OrganizerDomainServiceTest.java (1)
86-111: 테스트 내 상수 중복에 대한 작은 제안입니다.
MAX_PASSWORD_BYTE_LENGTH = 60이 프로덕션 코드와 테스트 코드에 각각 정의되어 있습니다. 프로덕션 값이 변경되면 테스트가 여전히 통과하지만 실제 동작과 불일치할 수 있습니다.선택지:
- 옵션 A (현재 유지): 테스트의 독립성을 우선시하여 명시적 값을 사용합니다.
- 옵션 B (상수 공유): 프로덕션 상수를
package-private으로 노출하거나, 별도 상수 클래스를 만들어 공유합니다.MVP 단계에서는 현재 방식으로 충분하며, 추후 해당 상수가 자주 변경될 것 같다면 옵션 B를 고려하세요.
한편,
"가".repeat(21)(63 bytes > 60)로 경계값 테스트를 작성한 점은 좋습니다! 👍🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/daedan/festabook/organizer/domain/OrganizerDomainServiceTest.java` around lines 86 - 111, Test defines MAX_PASSWORD_BYTE_LENGTH separately from production which can drift; update the test to reference the single source of truth instead of duplicating the constant: change OrganizerDomainServiceTest.validatePasswordByte to use the production constant (expose the production constant used by OrganizerDomainService.validatePasswordByte as package-private or move it into a shared Constants class) and update the assertion message accordingly so the test uses the same MAX_PASSWORD_BYTE_LENGTH value as production (or, if you intentionally want test independence, add a comment explaining why the literal 60 is chosen).src/main/java/com/daedan/festabook/organizer/application/OrganizerApplicationService.java (1)
43-50:findAll()은 데이터 증가 시 메모리 이슈의 잠재적 원인이 될 수 있습니다.현재 MVP 단계에서는 문제가 없겠지만, Organizer 수가 증가하면
findAll().stream().toList()가 모든 데이터를 힙에 로드하여 OOM(Out of Memory) 위험이 있습니다.향후 고려사항:
- 옵션 A (페이징 도입):
Pageable을 받아Page<Organizer>를 반환하도록 변경- 옵션 B (Streaming):
@QueryHints와 함께Stream<Organizer>를 사용하여 청크 단위 처리MVP 이후 데이터가 증가할 것으로 예상된다면, 미리 페이징 API로 설계하는 것이 좋습니다. 지금은 운영 데이터 규모에 따라 판단하시면 됩니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/daedan/festabook/organizer/application/OrganizerApplicationService.java` around lines 43 - 50, The current getAllOrganizers method uses organizerRepository.findAll() which can OOM as data grows; change getAllOrganizers in OrganizerApplicationService to accept a Pageable (e.g., getAllOrganizers(Pageable pageable)) and call organizerRepository.findAll(pageable).map(AdminOrganizerResponse::from) to build an AdminOrganizerResponses (or a Page<AdminOrganizerResponse> if you prefer); update AdminOrganizerResponses construction or return type accordingly and adjust callers; alternatively, if you prefer streaming, replace findAll() with a repository method returning Stream<Organizer> and process in fixed-size chunks with `@Transactional` and `@QueryHints`, but prefer the Pageable approach for an immediate safe fix.src/main/java/com/daedan/festabook/admin/infrastructure/AdminJpaRepository.java (1)
14-30: Soft Delete 검사 쿼리의 복잡도와 유지보수성에 대한 고민이 필요합니다.현재
sentinel값을 활용한coalesce패턴은 정확히 동작하지만, 몇 가지 고려할 점이 있습니다:
- 가독성:
coalesce(deleted_at, :sentinel) >= :sentinel로직은 처음 보는 개발자가 이해하기 어려울 수 있습니다.- 매직 값:
9999-01-01이 default 메서드에 하드코딩되어 있어, 의도가 명확히 드러나지 않습니다.- Native Query 의존성: DB 벤더에 따라 동작이 달라질 수 있습니다.
선택지를 제안드립니다:
- 옵션 A (현재 유지): 성능 최적화가 최우선이라면 현재 방식을 유지하되, 상수와 주석으로 의도를 명확히 하세요.
- 옵션 B (JPQL 활용): 유지보수성이 중요하다면 JPQL로 변경을 고려해보세요:
`@Query`("SELECT COUNT(a) > 0 FROM Admin a WHERE a.username = :username AND a.deletedAt IS NULL") boolean existsActiveByUsername(`@Param`("username") String username);현재 방식이 특정 DB 최적화(예: 인덱스 활용)를 위한 것이라면 옵션 A가 적절하고, 그렇지 않다면 옵션 B가 더 직관적입니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/daedan/festabook/admin/infrastructure/AdminJpaRepository.java` around lines 14 - 30, The current native-query soft-delete check in AdminJpaRepository uses a coalesce+sentinel pattern with a hardcoded LocalDateTime (9999-01-01) which hurts readability and maintainability; either (A) keep the native approach but extract the sentinel into a named constant (e.g., SENTINEL) and add a clarifying comment next to existsActiveByUsername(String username, LocalDateTime sentinel) and the default method to explain intent and DB rationale, or (B) replace the native query with a JPQL method: remove the native `@Query` and default sentinel logic and add a JPQL method signature boolean existsActiveByUsername(`@Param`("username") String username) using "a.deletedAt IS NULL" to make the intent explicit (choose B unless you require native-index optimization). Ensure changes reference the existsActiveByUsername methods and the deleted_at / deletedAt field consistently.src/main/java/com/daedan/festabook/admin/presentation/AdminController.java (1)
91-91:@Operationsummary 문구를 레포 컨벤션에 맞춰 한 번 정리해두면 문서 일관성이 좋아집니다.
Organization 전체 조회,Organizer 전체 조회는 collection 패턴에서 parent domain을 포함하는 쪽이 맞고,아이디 변경은 CRUD 용어를수정으로 통일하는 편이 Swagger 검색성과 문서 일관성에 유리합니다. 지금 단계에서 패턴을 맞춰두면 백오피스 API가 더 늘어나도 문서 유지보수가 훨씬 편해집니다.Based on learnings, "Operation summary patterns: Full collection = '[parent domain]의 [child domain] 전체 조회'. CRUD = '[domain] 생성/수정/삭제'. Child non-festival = '[parent] [child] 생성/수정/삭제'. Festival direct children = omit festival name. Forbidden: '특정', '~에 대한', '~들', '모든'."
Also applies to: 116-116, 127-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/daedan/festabook/admin/presentation/AdminController.java` at line 91, Update the `@Operation` summary texts in AdminController (the annotation on the methods currently using "Organization 전체 조회", "Organizer 전체 조회", and "아이디 변경") to follow the repo's summary patterns: use the collection pattern "[parent domain]의 [child domain] 전체 조회" for collection endpoints (replace "Organization 전체 조회" and "Organizer 전체 조회" accordingly), and use CRUD verbs "생성/수정/삭제" instead of "아이디 변경" (change it to "아이디 수정" or "[domain] 수정" as appropriate); also apply the same pattern adjustments to the other annotations referenced in the comment (the summaries at lines noted around 116 and 127) so all `@Operation` summaries in AdminController follow the documented conventions.src/test/java/com/daedan/festabook/admin/domain/AdminTest.java (1)
102-102: 테스트 이름을 시나리오 중심 컨벤션으로 맞춰두면 이후 탐색이 편합니다.
예외_Null_Actor_서버_예외_발생,예외_아이디_null_or_blank,예외_비밀번호_null_or_blank는 현재 레포 규칙의 “한글 명명 + 입력 검증은예외_{field}_{condition}+ 예외 타입/결과 미노출” 패턴과 조금 어긋납니다. 지금 맞춰두면 IDE 검색, 정렬, 실패 리포트에서 테스트 의도가 더 바로 읽힙니다.Based on learnings, "Domain permission test method naming: 성공_{ROLE}{condition}접근_권한_있음 or 예외{ROLE}{condition}접근_권한_없음. Input validation uses 예외{field}{condition} pattern. Never expose exception type in method name." and "Test method names in Korean. Start with '성공...' for success, '예외_...' or '실패_...' for failure."
Also applies to: 221-221, 270-270
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/daedan/festabook/admin/domain/AdminTest.java` at line 102, Rename the tests to follow the repository's Korean naming conventions: for domain permission tests use 성공_{ROLE}_{condition}_접근_권한_있음 or 예외_{ROLE}_{condition}_접근_권한_없음, and for input validation use 예외_{field}_{condition} without exposing exception types. Concretely, change method names like 예외_Null_Actor_서버_예외_발생 to match the permission pattern (e.g., 예외_{ROLE}_null_Actor_접근_권한_없음) and rename 예외_아이디_null_or_blank and 예외_비밀번호_null_or_blank to use the input-validation pattern (e.g., 예외_아이디_null_or_blank, 예외_비밀번호_null_or_blank) if not already; ensure no exception class/type names remain in method names and update any related test descriptions or assertions to match the new names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code-style.md`:
- Around line 1652-1699: Appendix A's checklist must be updated to cover the new
DDD rules introduced in Section 17: add items verifying package and naming
conventions (presentation/ vs controller/, application/ vs service/), class
names (XXXApplicationService, XXXDomainService) and their locations
(application/, domain/), coexistence rule for existing service/ when adding
application/, and test conventions (ApplicationService tests mock DomainService;
DomainService tests mock Repository); locate the Appendix A "### 구조" checklist
and append these specific checks so automated reviewers can detect DDD
violations.
- Around line 1665-1672: The fenced code block that begins with the directory
listing ("도메인/" followed by presentation/, application/, domain/,
infrastructure/, dto/) is missing a language identifier; update that opening
fence from ``` to ```text (or ```plaintext) so the block becomes
language-tagged, which satisfies markdownlint MD040 and improves accessibility
and rendering consistency.
In
`@src/main/java/com/daedan/festabook/admin/application/AdminApplicationService.java`:
- Around line 31-34: 지금 AdminApplicationService의 validateUsernameDuplicated →
Admin.createBy → adminRepository.save 흐름은 check-then-act 레이스에 취약하므로 저장 단계에서 DB의
unique 제약 위반(duplicate key) 예외를 잡아 ConflictException으로 변환하도록 처리하세요; 구체적으로는
AdminService의 adminRepository.save(admin) 호출을 try-catch로 감싸고 DB/ORM이 던지는
duplicate-key 관련 예외를 잡아 ConflictException으로 변환하여 409를 반환하거나(옵션 A/B 모두에 적용), 기존
선조회(validateUsernameDuplicated)를 유지한다면 저장 실패 시의 fallback 매핑만 추가하세요. 또한 장기적으로는
동시성 경합이 빈번한 리소스(예: username)에는 애플리케이션 레벨에서 AtomicBoolean/AtomicReference, 또는
ConcurrentHashMap을 이용한 간단한 잠금/플래그 패턴으로 경쟁을 완화하는 방안을 고려하세요; 관련 심볼:
validateUsernameDuplicated, passwordEncoder.encode, Admin.createBy,
adminRepository.save, ConflictException.
- Around line 32-33: The code currently encodes the password before domain
validation and does an unsafely-separated duplicate check/save, so fix by (1)
validating the raw password before encoding: either add `@NotBlank/`@Size to
AdminRequest and validate raw password prior to calling
passwordEncoder.encode(...) or change Admin.createBy(...) to accept the raw
password and perform validatePassword(...) inside the domain before encoding;
and (2) handle races on username by catching the persistence unique-constraint
error around repository.save(...) (or wrap repository.save in a try/catch), map
it to a domain DuplicateAdminException and then to a BadRequestException with a
user-friendly "username already exists" message; ensure references touched
include AdminRequest, passwordEncoder.encode, Admin.createBy, validatePassword,
validateUsernameDuplicated, repository.save, DuplicateAdminException and
BadRequestException.
In `@src/main/java/com/daedan/festabook/admin/domain/Admin.java`:
- Around line 21-40: Admin currently uses class-level `@Getter` which exposes
getPassword(); remove that exposure by preventing a generated getter for the
password field and expose only a controlled check method: annotate the password
field with `@Getter`(AccessLevel.NONE) (keeping the class-level `@Getter`) to stop
getPassword() from being generated, and add an explicit method like boolean
verifyPassword(String rawPassword) or boolean matchesPassword(String candidate)
on Admin that performs the allowed comparison/validation; alternatively, if you
prefer stricter control, remove the class-level `@Getter` and add explicit getters
only for id and username.
- Around line 52-69: Move the permission check before constructing the Admin
object: call validateCreatableBy(actor) at the start of Admin.createBy (so it
throws InternalServerException if actor is null or ForbiddenException if not
admin) and only then instantiate new Admin(username, password) to avoid exposing
validation errors before permission failure; alternatively split into a small
static guard method (e.g., ensureCreatableBy(actor)) invoked first and keep
Admin(username, password) construction after it, and add a regression test
covering organizer/staff + invalid username to lock the behavior.
In `@src/main/java/com/daedan/festabook/admin/presentation/AdminController.java`:
- Around line 89-121: The controller methods getAllOrganizations() and
getAllOrganizers() currently return full result sets (OrganizationResponses,
AdminOrganizerResponses) and must be changed to support paginated access: add
optional request parameters (e.g., page, size, sort) to the GetMapping endpoints
and update their signatures to accept a Pageable-like input or explicit
page/size/sort params, then call
organizationApplicationService.getAllOrganizations(...) and
organizerApplicationService.getAllOrganizers(...) with those pagination
arguments (or a wrapper DTO) so the services return page-aware responses
(Page/PagedResponse) or enforce a configurable max limit; ensure response DTO
types are updated accordingly or wrap current DTOs in a paged envelope to avoid
loading all records into memory.
In `@src/main/java/com/daedan/festabook/global/logging/LoggingFilter.java`:
- Around line 51-52: Logging currently omits masking for the admin creation
endpoint so AdminRequest.password is logged in plaintext; update
BODY_MASKING_PATH in LoggingFilter by adding a new MaskingPath entry for
"/admin" with method "POST" (i.e. add MaskingPath("/admin", "POST") alongside
the existing entries) to immediately prevent password exposure, and consider a
follow-up refactor to a key-based masking policy (e.g., mask any "password"
field) to avoid similar omissions in future.
In
`@src/main/java/com/daedan/festabook/organization/infrastructure/OrganizationRepositoryImpl.java`:
- Around line 20-22: The current OrganizationRepositoryImpl.findAll() returns
the entire dataset via organizationJpaRepository.findAll(), which risks OOM and
unpredictable ordering; change the repository contract to support pagination and
sorting by replacing findAll() with a paged method (e.g., Page<Organization>
findAll(Pageable pageable)) and implement it in OrganizationRepositoryImpl by
delegating to organizationJpaRepository.findAll(pageable); if a full export is
still required keep a separate explicitly named method (e.g.,
exportAllOrganizations()) for admin/export use-cases and do not expose it as the
default list API.
In
`@src/main/java/com/daedan/festabook/organizer/domain/OrganizerDomainService.java`:
- Around line 30-36: The validatePasswordByte method (validatePasswordByte)
currently calls rawPassword.getBytes(...) directly which can throw NPE if
rawPassword is null; add a defensive null check at the start of
validatePasswordByte and throw a BadRequestException with a clear message (e.g.,
"비밀번호를 입력해주세요" or similar) when rawPassword is null, then proceed to measure
bytes against MAX_PASSWORD_BYTE_LENGTH as before; also verify/comment if
MAX_PASSWORD_BYTE_LENGTH = 60 is intentional (BCrypt produces 60-char hashes)
and keep that business rule in mind.
In `@src/main/resources/db/migration/V15.0__create_admin.sql`:
- Around line 6-7: The schema has two soft-delete indicators (deleted and
deleted_at) that can diverge and confuse uniqueness/active-account checks; fix
by choosing one canonical soft-delete column or enforcing DB-level consistency:
either remove the deleted BIT column and update any UNIQUE/index logic to use
deleted_at as the single active/soft-delete indicator (and migrate/remove usages
of deleted), OR keep both but add a NOT NULL/generated/constraint or trigger to
ensure deleted = (deleted_at IS NOT NULL) on all writes and make the
UNIQUE/index on username reference both columns consistently (so uniqueness and
"active" checks use the same condition). Update migrations and any UNIQUE/index
definitions that currently reference deleted_at alone to match the chosen
approach.
In
`@src/test/java/com/daedan/festabook/admin/presentation/AdminControllerTest.java`:
- Around line 109-131: 테스트 Fixture가 평문 비밀번호로 Admin을 생성해 실제 생성 로직(createAdmin)과
다른 처리를 하고 있으므로 AdminFixture.createCustom을 수정해 저장 전에 PasswordEncoder로 비밀번호를
인코딩하도록 바꿔주세요; 권장 방법은 BaseEntityTestHelper의 비밀번호 인코딩 유틸을 호출해 Admin 객체를 만들 때
encoded password를 설정하는 것이고(대안: 테스트에서 PasswordEncoder를 주입해 미리 encode한 값을 넘겨주는
방식), 관련된 로그인 검증이 실제 흐름과 동일하게 동작하도록 AdminFixture.createCustom과 테스트에서 사용하는 비밀번호가
동일한 인코딩 규칙을 따르도록 일치시켜 주세요.
---
Nitpick comments:
In `@src/main/java/com/daedan/festabook/admin/dto/AdminDuplicateResponse.java`:
- Around line 3-10: Add Swagger `@Schema` annotations to the
AdminDuplicateResponse record to match other DTOs: annotate the record
(AdminDuplicateResponse) and its component/field (duplicated) with descriptive
`@Schema` metadata (e.g., description and example) and import
io.swagger.v3.oas.annotations.media.Schema; ensure the static factory method
from(boolean) remains unchanged and follow the same annotation style used in
AdminOrganizerUsernameUpdateRequest for consistency.
In `@src/main/java/com/daedan/festabook/admin/dto/AdminLoginResponse.java`:
- Around line 3-6: Add Swagger `@Schema` annotations to the AdminLoginResponse
record to match AdminLoginRequest: annotate the accessToken component with
`@Schema`(description = "JWT access token returned after successful admin login",
example = "eyJhbGciOi...") and (optionally) annotate the record itself with a
brief `@Schema`(description = "Admin login response DTO"); import
io.swagger.v3.oas.annotations.media.Schema and apply the annotation to the
accessToken symbol in the AdminLoginResponse record declaration.
In `@src/main/java/com/daedan/festabook/admin/dto/AdminResponse.java`:
- Around line 5-8: The AdminResponse record is missing Swagger annotations; add
an `@Schema` annotation to the record component `username` (e.g.,
`@Schema`(description = "관리자 사용자명" or appropriate description) on the `username`
component in `AdminResponse`) and import
io.swagger.v3.oas.annotations.media.Schema so the response DTO is documented
consistently like `AdminLoginRequest`.
In
`@src/main/java/com/daedan/festabook/admin/infrastructure/AdminJpaRepository.java`:
- Around line 14-30: The current native-query soft-delete check in
AdminJpaRepository uses a coalesce+sentinel pattern with a hardcoded
LocalDateTime (9999-01-01) which hurts readability and maintainability; either
(A) keep the native approach but extract the sentinel into a named constant
(e.g., SENTINEL) and add a clarifying comment next to
existsActiveByUsername(String username, LocalDateTime sentinel) and the default
method to explain intent and DB rationale, or (B) replace the native query with
a JPQL method: remove the native `@Query` and default sentinel logic and add a
JPQL method signature boolean existsActiveByUsername(`@Param`("username") String
username) using "a.deletedAt IS NULL" to make the intent explicit (choose B
unless you require native-index optimization). Ensure changes reference the
existsActiveByUsername methods and the deleted_at / deletedAt field
consistently.
In `@src/main/java/com/daedan/festabook/admin/presentation/AdminController.java`:
- Line 91: Update the `@Operation` summary texts in AdminController (the
annotation on the methods currently using "Organization 전체 조회", "Organizer 전체
조회", and "아이디 변경") to follow the repo's summary patterns: use the collection
pattern "[parent domain]의 [child domain] 전체 조회" for collection endpoints
(replace "Organization 전체 조회" and "Organizer 전체 조회" accordingly), and use CRUD
verbs "생성/수정/삭제" instead of "아이디 변경" (change it to "아이디 수정" or "[domain] 수정" as
appropriate); also apply the same pattern adjustments to the other annotations
referenced in the comment (the summaries at lines noted around 116 and 127) so
all `@Operation` summaries in AdminController follow the documented conventions.
In `@src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java`:
- Around line 88-99: The createAdminToken method lacks null-safety for its roles
parameter; add a null-check at the start of createAdminToken (e.g., use
Objects.requireNonNull(roles, "roles must not be null") if you want fast-fail
contract or Objects.requireNonNullElse(roles, Set.of()) for a defensive default)
so that the CLAIM_ROLES_TYPE claim never receives a null value when building the
JWT.
In
`@src/main/java/com/daedan/festabook/organizer/application/OrganizerApplicationService.java`:
- Around line 43-50: The current getAllOrganizers method uses
organizerRepository.findAll() which can OOM as data grows; change
getAllOrganizers in OrganizerApplicationService to accept a Pageable (e.g.,
getAllOrganizers(Pageable pageable)) and call
organizerRepository.findAll(pageable).map(AdminOrganizerResponse::from) to build
an AdminOrganizerResponses (or a Page<AdminOrganizerResponse> if you prefer);
update AdminOrganizerResponses construction or return type accordingly and
adjust callers; alternatively, if you prefer streaming, replace findAll() with a
repository method returning Stream<Organizer> and process in fixed-size chunks
with `@Transactional` and `@QueryHints`, but prefer the Pageable approach for an
immediate safe fix.
In `@src/main/java/com/daedan/festabook/organizer/domain/Organizer.java`:
- Around line 137-145: The two identical methods
validateUpdatableUsernameBy(Actor actor) and validateCreatableBy(Actor actor)
duplicate the same admin-only check; extract the shared logic into a single
helper method (e.g., validateAdminOnly(Actor actor)) that throws
InternalServerException when actor is null and ForbiddenException when actor is
non-admin, then have both validateUpdatableUsernameBy and validateCreatableBy
delegate to validateAdminOnly(Actor) to remove duplication while keeping method
names for future divergence.
In
`@src/main/java/com/daedan/festabook/organizer/domain/OrganizerRepository.java`:
- Line 8: The current OrganizerRepository.findAll() returning List<Organizer> is
an OOM/scalability risk; change the repository contract to a
pageable/streaming-friendly API (e.g., replace findAll() with a paginated method
like findAll(Pageable) or a streaming/iterator-based method such as
streamAllOrganizers()) or, if you must keep an eager API for MVP, enforce an
explicit max limit and sort parameters (e.g., findAllWithLimit(int limit, Sort
sort)) and document/validate the limit to prevent unbounded loads; update all
callers of OrganizerRepository.findAll() to use the new method and add input
validation and tests to ensure limits/pagination are enforced.
In
`@src/main/java/com/daedan/festabook/organizer/dto/AdminOrganizerCreateRequest.java`:
- Around line 5-18: Add Bean Validation annotations to the
AdminOrganizerCreateRequest record so controller-level validation fails fast:
annotate username, password, and name with `@NotBlank` and organizationId with
`@NotNull` (keep existing `@Schema`), and add the corresponding imports
(javax.validation.constraints.NotBlank, javax.validation.constraints.NotNull) so
Spring/Bean Validation can validate requests before reaching the domain layer.
In
`@src/main/java/com/daedan/festabook/organizer/dto/AdminOrganizerUsernameUpdateRequest.java`:
- Around line 5-9: The DTO AdminOrganizerUsernameUpdateRequest currently exposes
the username record component without validation; add a Bean Validation
annotation (e.g., `@NotBlank`) to the username component to prevent null/empty
values from reaching the service layer, and import
javax.validation.constraints.NotBlank (or
jakarta.validation.constraints.NotBlank depending on project setup); keep the
existing `@Schema` and ensure controllers have `@Valid` on the request parameter so
validation is enforced.
In `@src/test/java/com/daedan/festabook/admin/domain/AdminTest.java`:
- Line 102: Rename the tests to follow the repository's Korean naming
conventions: for domain permission tests use 성공_{ROLE}_{condition}_접근_권한_있음 or
예외_{ROLE}_{condition}_접근_권한_없음, and for input validation use
예외_{field}_{condition} without exposing exception types. Concretely, change
method names like 예외_Null_Actor_서버_예외_발생 to match the permission pattern (e.g.,
예외_{ROLE}_null_Actor_접근_권한_없음) and rename 예외_아이디_null_or_blank and
예외_비밀번호_null_or_blank to use the input-validation pattern (e.g.,
예외_아이디_null_or_blank, 예외_비밀번호_null_or_blank) if not already; ensure no exception
class/type names remain in method names and update any related test descriptions
or assertions to match the new names.
In
`@src/test/java/com/daedan/festabook/organizer/domain/OrganizerDomainServiceTest.java`:
- Around line 86-111: Test defines MAX_PASSWORD_BYTE_LENGTH separately from
production which can drift; update the test to reference the single source of
truth instead of duplicating the constant: change
OrganizerDomainServiceTest.validatePasswordByte to use the production constant
(expose the production constant used by
OrganizerDomainService.validatePasswordByte as package-private or move it into a
shared Constants class) and update the assertion message accordingly so the test
uses the same MAX_PASSWORD_BYTE_LENGTH value as production (or, if you
intentionally want test independence, add a comment explaining why the literal
60 is chosen).
In `@src/test/java/com/daedan/festabook/organizer/domain/OrganizerTest.java`:
- Around line 288-304: Add a separate permission-only test in the
updateUsernameBy nested class that mirrors the pattern used for
createBy/updateNameBy/updatePasswordBy: create a test named like
성공_ADMIN_접근_권한_있음() which uses ActorFixture.admin() and calls
organizer.updateUsernameBy(actor, username) wrapped in an assertion that it does
not throw (e.g., assertDoesNotThrow / doesNotThrowAnyException), leaving the
existing 성공() to continue validating state changes; this keeps permission checks
isolated from result-field assertions.
In
`@src/test/java/com/daedan/festabook/organizer/dto/AdminOrganizerCreateRequestFixture.java`:
- Around line 19-24: The all-parameters factory method currently named
create(...) in AdminOrganizerCreateRequestFixture should be renamed to
createCustom(...) to match the repository's Fixture convention; update the
method signature from create(String username, String password, String name, Long
organizationId) to createCustom(...) and update any internal overloaded
create(...) methods to delegate to the new createCustom(...) as needed, then
search and replace usages in tests to call
AdminOrganizerCreateRequestFixture.createCustom(...) so callers and other
fixtures remain consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3f71577c-860f-4078-b98e-a4afbf3c217b
📒 Files selected for processing (57)
code-style.mdsrc/main/java/com/daedan/festabook/admin/application/AdminApplicationService.javasrc/main/java/com/daedan/festabook/admin/domain/Admin.javasrc/main/java/com/daedan/festabook/admin/domain/AdminDomainService.javasrc/main/java/com/daedan/festabook/admin/domain/AdminRepository.javasrc/main/java/com/daedan/festabook/admin/dto/AdminDuplicateResponse.javasrc/main/java/com/daedan/festabook/admin/dto/AdminLoginRequest.javasrc/main/java/com/daedan/festabook/admin/dto/AdminLoginResponse.javasrc/main/java/com/daedan/festabook/admin/dto/AdminRequest.javasrc/main/java/com/daedan/festabook/admin/dto/AdminResponse.javasrc/main/java/com/daedan/festabook/admin/infrastructure/AdminJpaRepository.javasrc/main/java/com/daedan/festabook/admin/infrastructure/AdminRepositoryImpl.javasrc/main/java/com/daedan/festabook/admin/presentation/AdminController.javasrc/main/java/com/daedan/festabook/global/logging/LocalLoggingFilter.javasrc/main/java/com/daedan/festabook/global/logging/LoggingFilter.javasrc/main/java/com/daedan/festabook/global/security/authorization/AdminDetails.javasrc/main/java/com/daedan/festabook/global/security/authorization/AdminDetailsExtractStrategy.javasrc/main/java/com/daedan/festabook/global/security/config/SecurityConfig.javasrc/main/java/com/daedan/festabook/global/security/util/JwtProvider.javasrc/main/java/com/daedan/festabook/organization/application/OrganizationApplicationService.javasrc/main/java/com/daedan/festabook/organization/controller/OrganizationController.javasrc/main/java/com/daedan/festabook/organization/domain/OrganizationRepository.javasrc/main/java/com/daedan/festabook/organization/dto/OrganizationResponses.javasrc/main/java/com/daedan/festabook/organization/infrastructure/OrganizationRepositoryImpl.javasrc/main/java/com/daedan/festabook/organization/service/OrganizationService.javasrc/main/java/com/daedan/festabook/organizer/application/OrganizerApplicationService.javasrc/main/java/com/daedan/festabook/organizer/domain/Organizer.javasrc/main/java/com/daedan/festabook/organizer/domain/OrganizerDomainService.javasrc/main/java/com/daedan/festabook/organizer/domain/OrganizerRepository.javasrc/main/java/com/daedan/festabook/organizer/dto/AdminOrganizerCreateRequest.javasrc/main/java/com/daedan/festabook/organizer/dto/AdminOrganizerCreateResponse.javasrc/main/java/com/daedan/festabook/organizer/dto/AdminOrganizerResponse.javasrc/main/java/com/daedan/festabook/organizer/dto/AdminOrganizerResponses.javasrc/main/java/com/daedan/festabook/organizer/dto/AdminOrganizerUsernameUpdateRequest.javasrc/main/java/com/daedan/festabook/organizer/dto/AdminOrganizerUsernameUpdateResponse.javasrc/main/java/com/daedan/festabook/organizer/infrastructure/OrganizerRepositoryImpl.javasrc/main/resources/db/migration/V15.0__create_admin.sqlsrc/test/java/com/daedan/festabook/admin/application/AdminApplicationServiceTest.javasrc/test/java/com/daedan/festabook/admin/domain/AdminDomainServiceTest.javasrc/test/java/com/daedan/festabook/admin/domain/AdminFixture.javasrc/test/java/com/daedan/festabook/admin/domain/AdminTest.javasrc/test/java/com/daedan/festabook/admin/dto/AdminLoginRequestFixture.javasrc/test/java/com/daedan/festabook/admin/dto/AdminRequestFixture.javasrc/test/java/com/daedan/festabook/admin/presentation/AdminControllerTest.javasrc/test/java/com/daedan/festabook/global/security/authorization/AccountDetailsFixture.javasrc/test/java/com/daedan/festabook/global/security/authorization/AccountDetailsTest.javasrc/test/java/com/daedan/festabook/global/security/authorization/AdminDetailsTest.javasrc/test/java/com/daedan/festabook/global/security/util/JwtProviderTest.javasrc/test/java/com/daedan/festabook/organization/application/OrganizationApplicationServiceTest.javasrc/test/java/com/daedan/festabook/organization/controller/OrganizationControllerTest.javasrc/test/java/com/daedan/festabook/organization/service/OrganizationServiceTest.javasrc/test/java/com/daedan/festabook/organizer/application/OrganizerApplicationServiceTest.javasrc/test/java/com/daedan/festabook/organizer/domain/OrganizerDomainServiceTest.javasrc/test/java/com/daedan/festabook/organizer/domain/OrganizerFixture.javasrc/test/java/com/daedan/festabook/organizer/domain/OrganizerTest.javasrc/test/java/com/daedan/festabook/organizer/dto/AdminOrganizerCreateRequestFixture.javasrc/test/java/com/daedan/festabook/organizer/dto/AdminOrganizerUsernameUpdateRequestFixture.java
💤 Files with no reviewable changes (4)
- src/main/java/com/daedan/festabook/organization/controller/OrganizationController.java
- src/main/java/com/daedan/festabook/organization/service/OrganizationService.java
- src/test/java/com/daedan/festabook/organization/service/OrganizationServiceTest.java
- src/test/java/com/daedan/festabook/organization/controller/OrganizationControllerTest.java
Comment on lines
+1652
to
+1699
| ## 17. DDD 전환 규칙 | ||
|
|
||
| > 기존 코드는 Service 중심 레이어 구조로 작성되어 있다. | ||
| > 신규 도메인(Admin 등)부터 DDD 스타일로 작성하며, 기존 도메인은 점진적으로 마이그레이션한다. | ||
|
|
||
| ### 17.1 DDD 전환 범위 | ||
|
|
||
| - **기존 코드**: 현행 컨벤션(Service 레이어 중심)을 그대로 유지한다. 기존 코드를 DDD로 강제 변환하지 않는다. | ||
| - **신규 도메인**: DDD 패키지 구조와 레이어 규칙을 적용한다. | ||
| - 기존 도메인에 DDD 레이어를 추가하는 경우, 기존 `service/` 패키지는 유지하고 `application/` 패키지를 별도로 추가한다. | ||
|
|
||
| ### 17.2 DDD 패키지 구조 | ||
|
|
||
| ``` | ||
| 도메인/ | ||
| presentation/ ← Controller (기존 controller/ 대신 사용) | ||
| application/ ← Application Service (유스케이스 오케스트레이션) | ||
| domain/ ← Entity, Domain Service, Repository 인터페이스 | ||
| infrastructure/ ← JPA Repository 구현 | ||
| dto/ ← Request, Response DTO | ||
| ``` | ||
|
|
||
| ### 17.3 레이어 역할 | ||
|
|
||
| | 레이어 | 클래스 | 역할 | | ||
| |--------|--------|------| | ||
| | `presentation` | `XXXController` | HTTP 요청/응답 처리, Application Service 위임 | | ||
| | `application` | `XXXApplicationService` | 유스케이스 오케스트레이션, 트랜잭션 관리, DTO ↔ 도메인 변환 | | ||
| | `domain` | `XXXDomainService` | 단일 Entity에 넣기 힘든 도메인 규칙 (cross-aggregate 검증 등) | | ||
| | `domain` | `XXX` (Entity) | Aggregate Root, 도메인 상태와 행위 | | ||
| | `infrastructure` | `XXXJpaRepository` | 데이터 접근 | | ||
|
|
||
| ### 17.4 기존 컨벤션과 DDD 컨벤션 차이 | ||
|
|
||
| | 항목 | 기존 (Service 중심) | DDD (신규) | | ||
| |------|---------------------|------------| | ||
| | Controller 패키지 | `controller/` | `presentation/` | | ||
| | 비즈니스 로직 위치 | `service/XXXService` | `application/XXXApplicationService` + `domain/XXXDomainService` | | ||
| | 메서드 호출 흐름 | Controller → Service | Controller → ApplicationService → DomainService / Entity | | ||
|
|
||
| ### 17.5 테스트 규칙 | ||
|
|
||
| - DDD 레이어의 테스트도 기존 테스트 컨벤션(12장)을 동일하게 따른다. | ||
| - `ApplicationService` 테스트: 단위 테스트 (Mockito + BDDMockito), `DomainService`를 모킹한다. | ||
| - `DomainService` 테스트: 단위 테스트 (Mockito + BDDMockito), Repository를 모킹한다. | ||
| - Controller 테스트: 기존과 동일하게 `AcceptanceTestSupport` 상속 통합 테스트. | ||
|
|
||
| --- |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
부록 A 체크리스트 업데이트 누락
섹션 17 "DDD 전환 규칙"에서 새로운 패턴과 규칙을 도입했지만, 문서 자체의 메타 지침(35번 라인)을 따르지 않았습니다:
새 규칙이 추가되면 부록 체크리스트에도 반드시 해당 항목을 추가한다.
도입된 새 패턴들:
- 새 패키지 구조:
presentation/,application/(기존controller/,service/와 다름) - 새 클래스 네이밍:
XXXApplicationService,XXXDomainService - 레이어별 책임 분리 규칙
부록 A에 추가가 필요한 체크리스트 항목 예시:
- DDD 도메인에서 Controller가
presentation/패키지에 위치하는가? - DDD 도메인에서 비즈니스 로직이
XXXApplicationService로 명명되었는가? XXXApplicationService가 도메인 로직을DomainService나 Entity에 위임하는가?- 신규 도메인이 DDD 구조를 따라야 하는데 기존
service/패키지를 사용하고 있지는 않은가?
체크리스트를 업데이트하지 않으면 AI 코드 리뷰 시 DDD 규칙 위반을 일관되게 검출할 수 없습니다.
📋 부록 A 업데이트 제안
부록 A의 "### 구조" 섹션(약 1742번 라인 근처)에 다음과 같은 DDD 관련 체크리스트를 추가하세요:
### DDD 전환 (신규 도메인)
- [ ] DDD 적용 대상 신규 도메인에서 Controller가 `presentation/` 패키지에 위치하는가?
- [ ] DDD 적용 대상 신규 도메인에서 비즈니스 로직 클래스가 `XXXApplicationService`로 명명되었는가?
- [ ] `XXXApplicationService`가 `application/` 패키지에 위치하는가?
- [ ] `XXXDomainService`가 `domain/` 패키지에 위치하는가?
- [ ] 기존 도메인에 DDD 레이어를 추가할 때 기존 `service/` 패키지와 신규 `application/` 패키지가 공존하는가?
- [ ] ApplicationService 테스트에서 DomainService를 모킹하고 있는가?
- [ ] DomainService 테스트에서 Repository를 모킹하고 있는가?🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 1665-1665: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code-style.md` around lines 1652 - 1699, Appendix A's checklist must be
updated to cover the new DDD rules introduced in Section 17: add items verifying
package and naming conventions (presentation/ vs controller/, application/ vs
service/), class names (XXXApplicationService, XXXDomainService) and their
locations (application/, domain/), coexistence rule for existing service/ when
adding application/, and test conventions (ApplicationService tests mock
DomainService; DomainService tests mock Repository); locate the Appendix A "###
구조" checklist and append these specific checks so automated reviewers can detect
DDD violations.
Comment on lines
+1665
to
+1672
| ``` | ||
| 도메인/ | ||
| presentation/ ← Controller (기존 controller/ 대신 사용) | ||
| application/ ← Application Service (유스케이스 오케스트레이션) | ||
| domain/ ← Entity, Domain Service, Repository 인터페이스 | ||
| infrastructure/ ← JPA Repository 구현 | ||
| dto/ ← Request, Response DTO | ||
| ``` |
There was a problem hiding this comment.
코드 블록에 언어 식별자 추가 필요
라인 1665의 펜스 코드 블록에 언어 식별자가 누락되었습니다. 이는 다음과 같은 문제를 초래합니다:
- 접근성 저하: 스크린 리더 사용자에게 컨텍스트 정보가 제공되지 않습니다
- 렌더링 불일치: 일부 마크다운 파서에서 올바르게 처리되지 않을 수 있습니다
- 린트 규칙 위반: markdownlint MD040 규칙을 위반합니다
디렉토리 구조를 나타내는 블록이므로 text 또는 plaintext 식별자를 추가하는 것을 권장합니다.
🔧 제안하는 수정
-```
+```text
도메인/
presentation/ ← Controller (기존 controller/ 대신 사용)
application/ ← Application Service (유스케이스 오케스트레이션)🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 1665-1665: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code-style.md` around lines 1665 - 1672, The fenced code block that begins
with the directory listing ("도메인/" followed by presentation/, application/,
domain/, infrastructure/, dto/) is missing a language identifier; update that
opening fence from ``` to ```text (or ```plaintext) so the block becomes
language-tagged, which satisfies markdownlint MD040 and improves accessibility
and rendering consistency.
Comment on lines
+31
to
+34
| adminDomainService.validateUsernameDuplicated(request.username()); | ||
| String encodedPassword = passwordEncoder.encode(request.password()); | ||
| Admin admin = Admin.createBy(actor, request.username(), encodedPassword); | ||
| return AdminResponse.from(adminRepository.save(admin)); |
There was a problem hiding this comment.
선조회 후 저장 패턴이라 동시 요청에서는 중복 체크가 뚫립니다.
validateUsernameDuplicated()는 사용자 경험에는 좋지만, 같은 username 생성 요청이 동시에 들어오면 둘 다 통과한 뒤 하나가 DB unique 제약에서 깨지는 전형적인 check-then-act 레이스가 됩니다. 지금 계층에서 그 예외를 ConflictException으로 다시 매핑하지 않으면 409 대신 500으로 보일 가능성이 큽니다.
옵션 A는 DB unique를 최종 진실로 보고 저장 시 duplicate key를 ConflictException으로 변환하는 방식이고, 옵션 B는 현재 선조회를 유지하되 저장 단계의 fallback 매핑만 추가하는 방식입니다.
As per coding guidelines, "공유 자원에 대한 'Check-Then-Act' 레이스 컨디션을 찾아내고, Atomic 변수나 ConcurrentHashMap 활용을 제안하세요."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/daedan/festabook/admin/application/AdminApplicationService.java`
around lines 31 - 34, 지금 AdminApplicationService의 validateUsernameDuplicated →
Admin.createBy → adminRepository.save 흐름은 check-then-act 레이스에 취약하므로 저장 단계에서 DB의
unique 제약 위반(duplicate key) 예외를 잡아 ConflictException으로 변환하도록 처리하세요; 구체적으로는
AdminService의 adminRepository.save(admin) 호출을 try-catch로 감싸고 DB/ORM이 던지는
duplicate-key 관련 예외를 잡아 ConflictException으로 변환하여 409를 반환하거나(옵션 A/B 모두에 적용), 기존
선조회(validateUsernameDuplicated)를 유지한다면 저장 실패 시의 fallback 매핑만 추가하세요. 또한 장기적으로는
동시성 경합이 빈번한 리소스(예: username)에는 애플리케이션 레벨에서 AtomicBoolean/AtomicReference, 또는
ConcurrentHashMap을 이용한 간단한 잠금/플래그 패턴으로 경쟁을 완화하는 방안을 고려하세요; 관련 심볼:
validateUsernameDuplicated, passwordEncoder.encode, Admin.createBy,
adminRepository.save, ConflictException.
Comment on lines
+32
to
+33
| String encodedPassword = passwordEncoder.encode(request.password()); | ||
| Admin admin = Admin.createBy(actor, request.username(), encodedPassword); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Admin.java =="
admin_file="$(fd 'Admin\.java$' src/main/java | head -n 1)"
if [ -n "${admin_file:-}" ]; then
sed -n '1,260p' "$admin_file" | rg -n -C3 'class Admin|createBy|validatePassword|password'
fi
echo
echo "== AdminRequest.java =="
request_file="$(fd 'AdminRequest\.java$' src/main/java | head -n 1)"
if [ -n "${request_file:-}" ]; then
sed -n '1,220p' "$request_file" | rg -n -C3 'record AdminRequest|password|NotBlank|Size|Pattern'
fi
echo
echo "== Admin tests covering raw password validation =="
rg -n -C2 '예외_비밀번호|password.*초과|password.*blank|password.*null' src/test/java/com/daedan/festabook/adminRepository: festabook/backend
Length of output: 5305
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== AdminApplicationService.createAdmin 메서드 전체 ==
admin_service="$(fd 'AdminApplicationService\.java$' src/main/java | head -n 1)"
if [ -n "${admin_service:-}" ]; then
cat -n "${admin_service}" | sed -n '20,50p'
fi
echo
echo "== checkUsernameDuplicate 메서드 ==
if [ -n "${admin_service:-}" ]; then
rg -n -C5 'checkUsernameDuplicate' "${admin_service}"
fiRepository: festabook/backend
Length of output: 112
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== AdminApplicationService.createAdmin 메서드 전체 =="
admin_service="$(fd 'AdminApplicationService\.java$' src/main/java | head -n 1)"
if [ -n "${admin_service}" ]; then
cat -n "${admin_service}" | sed -n '20,50p'
fi
echo
echo "== checkUsernameDuplicate 메서드 =="
if [ -n "${admin_service}" ]; then
rg -n -C5 'checkUsernameDuplicate' "${admin_service}"
fiRepository: festabook/backend
Length of output: 2039
raw password 검증이 우회될 수 있으며, 동시 요청에서 username 중복 제약이 깨질 수 있습니다.
코드 흐름을 보면 두 가지 설계 문제가 있습니다.
1) 비밀번호 인코딩 시점
AdminRequest에는 @NotBlank나 @Size 같은 제약이 없어서 raw password가 HTTP layer에서 검증되지 않습니다. 그런데 line 32에서 인코딩한 후 line 33의 Admin.createBy(..., encodedPassword)로 전달하면, Admin 생성자 내 validatePassword()는 실제로 encoded hash를 검증하게 됩니다. 원래 의도는 사용자가 입력한 raw password의 빈 문자열이나 길이 제약을 검증하려는 것 같은데, 이렇게 되면 validation 대상이 뒤바뀌어 규칙이 명확하지 않습니다.
-
옵션 A:
AdminRequest에@NotBlank@SiZe(max=100)추가 후, 인코딩 전에 raw password를 명시적으로 검증하고 나서 encode하기- 장점: HTTP layer와 domain layer의 책임 분명
- 단점: 양쪽에서 같은 규칙 관리 필요 (중복)
-
옵션 B:
Admin.createBy()를(Actor, String username, String rawPassword)형태로 변경해서, 도메인 내부에서 raw password를 검증한 뒤 encode하기- 장점: 도메인이 password 규칙의 유일한 source of truth
- 단점: 도메인 계층이 encoding 책임까지 가져야 함 (관심사 분리 관점에서 고민 필요)
2) username 중복 체크와 저장 사이의 race condition
line 31의 validateUsernameDuplicated()와 line 34의 save() 사이에 시간 간격이 있어서, 동시 요청 2개가 모두 중복 체크를 통과한 후 save를 시도하면 유니크 제약에 걸립니다. migration에서 unique constraint를 추가했으므로 exception이 발생하는데, 현재 코드는 이를 처리하지 않아 클라이언트가 500 에러를 받게 됩니다.
- 개선안: duplicate check 예외를 catch해서
DuplicateAdminException같은 도메인 예외로 매핑하고, 이를BadRequestException으로 변환해 400 응답하기- 또는 DB의
UNIQUE제약이 최후의 보루라고 가정하되, 이 경우 명시적인 예외 처리로 "이미 존재하는 username입니다" 메시지 제공
- 또는 DB의
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/daedan/festabook/admin/application/AdminApplicationService.java`
around lines 32 - 33, The code currently encodes the password before domain
validation and does an unsafely-separated duplicate check/save, so fix by (1)
validating the raw password before encoding: either add `@NotBlank/`@Size to
AdminRequest and validate raw password prior to calling
passwordEncoder.encode(...) or change Admin.createBy(...) to accept the raw
password and perform validatePassword(...) inside the domain before encoding;
and (2) handle races on username by catching the persistence unique-constraint
error around repository.save(...) (or wrap repository.save in a try/catch), map
it to a domain DuplicateAdminException and then to a BadRequestException with a
user-friendly "username already exists" message; ensure references touched
include AdminRequest, passwordEncoder.encode, Admin.createBy, validatePassword,
validateUsernameDuplicated, repository.save, DuplicateAdminException and
BadRequestException.
Comment on lines
+21
to
+40
| @Entity | ||
| @Getter | ||
| @SQLRestriction("deleted = false") | ||
| @SQLDelete(sql = "UPDATE admin SET deleted = true, deleted_at = CURRENT_TIMESTAMP WHERE id = ?") | ||
| @NoArgsConstructor(access = AccessLevel.PROTECTED) | ||
| public class Admin extends BaseEntity { | ||
|
|
||
| private static final int MAX_USERNAME_LENGTH = 50; | ||
| private static final int MAX_PASSWORD_LENGTH = 100; | ||
| private static final Pattern KOREAN_USERNAME_PATTERN = Pattern.compile(".*[ㄱ-ㅎㅏ-ㅣ가-힣].*"); | ||
|
|
||
| @Id | ||
| @GeneratedValue(strategy = GenerationType.IDENTITY) | ||
| private Long id; | ||
|
|
||
| @Column(nullable = false, length = MAX_USERNAME_LENGTH) | ||
| private String username; | ||
|
|
||
| @Column(nullable = false, length = MAX_PASSWORD_LENGTH) | ||
| private String password; |
There was a problem hiding this comment.
password까지 공개 getter로 열어두는 건 보안상 아쉽습니다.
현재 @Getter가 getPassword()까지 생성해서, 이후 DTO 매핑·로깅·디버깅 과정에서 민감값이 실수로 흘러나갈 여지가 생깁니다. 편의성은 있지만, 보안 필드는 “값 노출”보다 “필요한 동작만 노출”하는 쪽이 안전합니다.
- 옵션 A: 클래스 레벨
@Getter는 유지하되password만@Getter(AccessLevel.NONE)로 막고, 필요한 비교/검증 동작만 메서드로 노출 - 옵션 B: 엔티티 전반의 접근 범위를 더 엄격히 가져가고 싶다면 explicit getter 방식으로 축소
🔒 제안 diff
`@Column`(nullable = false, length = MAX_USERNAME_LENGTH)
private String username;
+ `@Getter`(AccessLevel.NONE)
`@Column`(nullable = false, length = MAX_PASSWORD_LENGTH)
private String password;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/daedan/festabook/admin/domain/Admin.java` around lines 21 -
40, Admin currently uses class-level `@Getter` which exposes getPassword(); remove
that exposure by preventing a generated getter for the password field and expose
only a controlled check method: annotate the password field with
`@Getter`(AccessLevel.NONE) (keeping the class-level `@Getter`) to stop
getPassword() from being generated, and add an explicit method like boolean
verifyPassword(String rawPassword) or boolean matchesPassword(String candidate)
on Admin that performs the allowed comparison/validation; alternatively, if you
prefer stricter control, remove the class-level `@Getter` and add explicit getters
only for id and username.
Comment on lines
+51
to
+52
| new MaskingPath("/places/accesses/login", "POST"), | ||
| new MaskingPath("/admin/login", "POST") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== AdminController request mappings =="
fd 'AdminController\.java$' src/main/java | xargs -r sed -n '1,240p'
echo
echo "== Admin request DTO fields =="
rg -n -C2 'record|class|password|username' src/main/java/com/daedan/festabook/admin -g '*.java'
echo
echo "== LoggingFilter masking paths =="
sed -n '43,53p' src/main/java/com/daedan/festabook/global/logging/LoggingFilter.javaRepository: festabook/backend
Length of output: 29148
/admin 생성 엔드포인트의 비밀번호가 로그에 노출됩니다.
현재 마스킹 목록에 /admin/login (POST)은 추가되었으나, 관리자 계정 생성 엔드포인트인 /admin (POST)이 누락되어 있습니다. AdminRequest는 username과 password를 모두 포함하고 있기 때문에 최초 관리자 계정 발급 시 평문 비밀번호가 그대로 애플리케이션 로그에 기록됩니다.
organizers와 staff의 경우 생성(/organizers, /staff)과 로그인(/organizers/login, /staff/login) 경로를 모두 마스킹하고 있어서, admin도 동일한 수준의 보안 정책을 적용해야 합니다.
- 옵션 A:
/admin(POST)을 즉시BODY_MASKING_PATH에 추가합니다. 적용은 빠르지만, 향후 credential endpoint가 늘어날 때마다 누락할 위험이 있습니다. - 옵션 B: path 나열 방식 대신
password같은 민감 필드 키 기준의 공통 마스킹 정책으로 리팩토링합니다. 초기 작업은 조금 크지만 장기적으로 더 안전하고 유지보수하기 좋습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/daedan/festabook/global/logging/LoggingFilter.java` around
lines 51 - 52, Logging currently omits masking for the admin creation endpoint
so AdminRequest.password is logged in plaintext; update BODY_MASKING_PATH in
LoggingFilter by adding a new MaskingPath entry for "/admin" with method "POST"
(i.e. add MaskingPath("/admin", "POST") alongside the existing entries) to
immediately prevent password exposure, and consider a follow-up refactor to a
key-based masking policy (e.g., mask any "password" field) to avoid similar
omissions in future.
Comment on lines
+20
to
+22
| @Override | ||
| public List<Organization> findAll() { | ||
| return organizationJpaRepository.findAll(); |
There was a problem hiding this comment.
백오피스 목록 계약을 findAll()로 고정하면 데이터가 늘수록 운영 리스크가 커집니다.
지금 방식은 조직을 전부 메모리로 올린 뒤 다음 계층에서 다시 DTO 리스트로 변환하게 됩니다. MVP 단계에서는 구현이 단순하다는 장점이 있지만, 데이터가 커지면 응답 시간과 힙 사용량이 함께 증가하고, 정렬 기준도 암묵적이라 화면 순서가 흔들릴 수 있습니다.
- 옵션 A: 일반 목록 조회라면
Pageable + Sort를 리포지토리 계약으로 올려 필요한 개수만 읽도록 바꾸는 쪽이 성능에 유리합니다. - 옵션 B: 정말 전체 스캔이 필요한 관리 작업이라면, 별도 export/use-case로 분리하고 기본 목록 API는 페이지네이션으로 제한하는 쪽이 유지보수에 유리합니다.
As per coding guidelines, Stream.collect()로 대량의 데이터를 힙에 로드하거나 전체 조회를 무심코 노출하는 패턴은 시스템 안정성 관점에서 우선 점검해야 합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/daedan/festabook/organization/infrastructure/OrganizationRepositoryImpl.java`
around lines 20 - 22, The current OrganizationRepositoryImpl.findAll() returns
the entire dataset via organizationJpaRepository.findAll(), which risks OOM and
unpredictable ordering; change the repository contract to support pagination and
sorting by replacing findAll() with a paged method (e.g., Page<Organization>
findAll(Pageable pageable)) and implement it in OrganizationRepositoryImpl by
delegating to organizationJpaRepository.findAll(pageable); if a full export is
still required keep a separate explicitly named method (e.g.,
exportAllOrganizations()) for admin/export use-cases and do not expose it as the
default list API.
Comment on lines
+30
to
+36
| public void validatePasswordByte(String rawPassword) { | ||
| if (rawPassword.getBytes(StandardCharsets.UTF_8).length > MAX_PASSWORD_BYTE_LENGTH) { | ||
| throw new BadRequestException( | ||
| String.format("비밀번호는 %d바이트를 초과할 수 없습니다.", MAX_PASSWORD_BYTE_LENGTH) | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
rawPassword가 null일 경우 NPE 발생 가능성이 있습니다.
현재 rawPassword.getBytes()를 직접 호출하고 있어, null이 전달되면 NullPointerException이 발생합니다. 상위 레이어(DTO 검증 등)에서 null 체크가 보장된다면 괜찮지만, 방어적 코딩 관점에서 검토가 필요합니다.
선택지:
- 옵션 A (현재 유지): DTO의
@NotBlank/@NotNull검증에 의존하여 null이 들어올 수 없다고 가정합니다. - 옵션 B (방어적 코딩): 도메인 서비스 레벨에서도 방어 로직을 추가합니다:
🛡️ 방어적 코딩 예시
public void validatePasswordByte(String rawPassword) {
+ if (rawPassword == null) {
+ throw new BadRequestException("비밀번호는 비어 있을 수 없습니다.");
+ }
if (rawPassword.getBytes(StandardCharsets.UTF_8).length > MAX_PASSWORD_BYTE_LENGTH) {또한 MAX_PASSWORD_BYTE_LENGTH = 60의 의도가 궁금합니다. BCrypt는 입력 길이와 관계없이 항상 60자의 해시를 생성하므로, 이 제한이 비즈니스 요구사항인지 확인해주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/daedan/festabook/organizer/domain/OrganizerDomainService.java`
around lines 30 - 36, The validatePasswordByte method (validatePasswordByte)
currently calls rawPassword.getBytes(...) directly which can throw NPE if
rawPassword is null; add a defensive null check at the start of
validatePasswordByte and throw a BadRequestException with a clear message (e.g.,
"비밀번호를 입력해주세요" or similar) when rawPassword is null, then proceed to measure
bytes against MAX_PASSWORD_BYTE_LENGTH as before; also verify/comment if
MAX_PASSWORD_BYTE_LENGTH = 60 is intentional (BCrypt produces 60-char hashes)
and keep that business rule in mind.
Comment on lines
+6
to
+7
| deleted BIT(1) NOT NULL COMMENT '삭제 여부 (0: 미삭제, 1: 삭제). 삭제된 계정도 운영 로그 추적을 위해 보존', | ||
| deleted_at DATETIME(6) NULL COMMENT '삭제 일시. 미삭제 시 NULL', |
There was a problem hiding this comment.
deleted와 deleted_at 중 어느 쪽이 “활성 계정”의 기준인지 스키마상으로 갈릴 수 있습니다.
유니크 제약은 deleted_at만 보고 있는데, 테이블에는 deleted 플래그도 따로 있습니다. 이후 soft delete 구현이 둘 중 하나만 갱신해도 username 재사용 가능 여부와 활성 계정 판정이 서로 어긋날 수 있어서, 인증/중복 검사 쪽에서 꽤 까다로운 데이터 불일치로 이어질 수 있습니다.
옵션 A는 활성 판단 기준을 한 컬럼으로만 통일하는 방식이고, 옵션 B는 두 컬럼을 유지하되 DB 레벨 제약으로 항상 같은 상태 전이를 강제하는 방식입니다.
Also applies to: 16-20
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/db/migration/V15.0__create_admin.sql` around lines 6 - 7,
The schema has two soft-delete indicators (deleted and deleted_at) that can
diverge and confuse uniqueness/active-account checks; fix by choosing one
canonical soft-delete column or enforcing DB-level consistency: either remove
the deleted BIT column and update any UNIQUE/index logic to use deleted_at as
the single active/soft-delete indicator (and migrate/remove usages of deleted),
OR keep both but add a NOT NULL/generated/constraint or trigger to ensure
deleted = (deleted_at IS NOT NULL) on all writes and make the UNIQUE/index on
username reference both columns consistently (so uniqueness and "active" checks
use the same condition). Update migrations and any UNIQUE/index definitions that
currently reference deleted_at alone to match the chosen approach.
Comment on lines
+109
to
+131
| @Test | ||
| void 성공() { | ||
| // given | ||
| String username = "admin_" + UUID.randomUUID(); | ||
| String password = "password123"; | ||
|
|
||
| Admin admin = AdminFixture.createCustom(username, password); | ||
| adminJpaRepository.save(admin); | ||
|
|
||
| int expectedFieldSize = 1; | ||
|
|
||
| // when & then | ||
| RestAssured | ||
| .given() | ||
| .contentType(ContentType.JSON) | ||
| .body(AdminLoginRequestFixture.create(username, password)) | ||
| .when() | ||
| .post("/admin/login") | ||
| .then() | ||
| .statusCode(HttpStatus.CREATED.value()) | ||
| .body("size()", equalTo(expectedFieldSize)) | ||
| .body("accessToken", notNullValue()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# AdminFixture.createCustom 메서드가 비밀번호를 어떻게 처리하는지 확인
ast-grep --pattern $'class AdminFixture {
$$$
createCustom($_, $_) {
$$$
}
$$$
}'
# 또는 파일 내용 직접 확인
fd -t f 'AdminFixture.java' --exec cat {}Repository: festabook/backend
Length of output: 948
🏁 Script executed:
#!/bin/bash
# 1. Admin 엔티티 클래스 찾기
fd -t f 'Admin.java' src/main --exec cat {} \;
# 2. AdminController의 login 메서드 찾기
fd -t f 'AdminController.java' src/main --exec cat {} \;Repository: festabook/backend
Length of output: 9533
🏁 Script executed:
#!/bin/bash
# AdminApplicationService 찾기
fd -t f 'AdminApplicationService.java' src/main --exec cat {} \;Repository: festabook/backend
Length of output: 2333
🏁 Script executed:
#!/bin/bash
# AdminDomainService의 validatePasswordMatch 메서드 찾기
fd -t f 'AdminDomainService.java' src/main --exec cat {} \;Repository: festabook/backend
Length of output: 967
테스트 Fixture의 비밀번호 인코딩 방식이 실제 Admin 생성 로직과 불일치합니다.
현재 테스트는 통과하지만, 이것은 잘못된 일치 때문입니다:
createAdmin():passwordEncoder.encode()로 비밀번호를 인코딩한 후 저장 ✓AdminFixture.createCustom(): 비밀번호를 인코딩하지 않고 평문으로 저장 ✗- 테스트는 평문 비밀번호로 로그인을 요청하고, Fixture도 평문으로 저장되어 있어서
matches(평문, 평문)으로 우연히 통과합니다.
문제: Fixture가 실제 Admin 생성과 동일한 방식(인코딩)으로 비밀번호를 처리해야 합니다. 현재 구조는 PasswordEncoder 검증 로직에 대한 신뢰도를 낮춥니다.
개선 방향: Fixture에서 비밀번호를 인코딩하는 방법을 고려해주세요:
- 옵션 A (권장): Fixture가
BaseEntityTestHelper를 활용해 인코딩된 비밀번호를 직접 설정하도록 개선 (복잡도 최소, 환경 일치도 최대) - 옵션 B: 테스트에서
PasswordEncoder를 주입받아 비밀번호를 미리 인코딩한 후 사용 (의존성 증가)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/com/daedan/festabook/admin/presentation/AdminControllerTest.java`
around lines 109 - 131, 테스트 Fixture가 평문 비밀번호로 Admin을 생성해 실제 생성 로직(createAdmin)과
다른 처리를 하고 있으므로 AdminFixture.createCustom을 수정해 저장 전에 PasswordEncoder로 비밀번호를
인코딩하도록 바꿔주세요; 권장 방법은 BaseEntityTestHelper의 비밀번호 인코딩 유틸을 호출해 Admin 객체를 만들 때
encoded password를 설정하는 것이고(대안: 테스트에서 PasswordEncoder를 주입해 미리 encode한 값을 넘겨주는
방식), 관련된 로그인 검증이 실제 흐름과 동일하게 동작하도록 AdminFixture.createCustom과 테스트에서 사용하는 비밀번호가
동일한 인코딩 규칙을 따르도록 일치시켜 주세요.
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.
#️⃣ 이슈 번호
#116
🛠️ 작업 내용
Summary by CodeRabbit
릴리스 노트
New Features
Documentation