Conversation
* refactor: null 검증 예외명 통일 * refactor: null 검증 Objects.isNull, nonNull로 변경 * refactor: if 문 간 공백 제거 * test: TestActor Builder 제거 * test: Fixture 안 쓰는 곳 수정 * refactor: 서비스, 컨트롤러 코드 메서드 순서 수정 * refactor: Request to 메서드 제거 * refactor: 테스트명 실제 메서드명 불일치 부분 수정 * test: 커버리지 누락 보완 * test: FestivalImageTest 누락된 도메인 검증 추가 * test: DisplayNameGeneration 추가 * refactor: 미사용 import 제거 * docs: 컨벤션 엡데이트 * refactor: PlaceAnnouncement 검증 테스트 추가 * fix: Device 없을 경우도 알림 삭제 * refactor: 미사용 import 제거 * test: 예외명 컨벤션 수정 * fix: 잘못된 메서드명 수정 * test: 중복 display 어노테이션 제거 * feat: 비밀번호 검증 추가 * test: 토픽 검증 추가 * test: 불필요한 검증 제거 * test: 불필요한 null 검증 제거 * refactor: 불필요한 공백 제거 * feat: 동시성 제어 필요한 로직 인메모리락 추가 * test: 테스트명 수정 * test: 불필요 테스트 삭제 * test: assertSoftly 추가 * test: 비어 있는 것 검증 추가 * test: 불필요한 테스트 삭제 * test: 누락된 테스트 검증 추가 * test: 불필요한 테스트 삭제 * refactor: null 검증 == 비교로 변경 * test: LockAspectTest 추가 * test: 테스트 소속 이상한 부분 수정 * feat: 아이디 한글 검증 추가 * refactor: 미사용 import 제거 * refactor: 의도대로 동작하는 Objects.isNull로 변경 * feat: 예외 발생 락 해제 테스트 추가
📝 Walkthrough코드 리뷰 분석Walkthrough이번 PR은 코드 스타일 표준화와 동시성 제어를 중심으로 광범위한 리팩토링을 진행합니다. 주요 변경은 null 체크 방식을 Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 분석 근거:
Possibly related PRs
주요 고려사항 및 개선 제안1. 광범위한 스타일 통일의 위험성문제점:
대안: // 옵션 1: Java 15+ Optional의 isEmpty() 활용 (함수형 프로그래밍 지향)
if (actor == null) { ... } // 현재 선택
// 개선:
Optional.ofNullable(actor).ifPresentOrElse(
act -> { /* 처리 */ },
() -> throw new InternalServerException(...)
);
// 장점: 함수형 패러다임 일관성, null 처리 명시적
// 단점: 단순 null 체크에는 과도할 수 있음
// 옵션 2: `@Nullable/`@NonNull 어노테이션 도입
`@Nullable` Actor actor // IDE 경고 활성화
if (actor == null) { ... }
// 장점: IDE 지원, 정적 분석 강화
// 단점: 추가 어노테이션 의존성권장사항: 적어도 중요한 보안/권한 경로(ActorArgumentResolver, 권한 검증)에는 어노테이션을 추가하여 null 처리의 중요성을 강조하는 것이 좋습니다. 2. PlaceAccess 로그인 플로우 변경의 복잡성문제점: // 기존: 장소 기반 전체 접근 조회
findByPlaceIdAndExpiresAtAfter(placeId, now)
// 신규: 키 기반 개별 접근 검증
existsExpiredOrDeletedByAccessKey(accessKey)
findByAccessKeyAndExpiresAtAfter(accessKey, now)이 변경의 세 가지 문제:
개선 제안: // 옵션 1: 단일 쿼리로 통합 (권장)
`@Query`("""
SELECT pa FROM PlaceAccess pa
WHERE pa.accessKey = :accessKey
AND pa.expiresAt > :now
AND pa.isDeleted = false
""")
Optional<PlaceAccess> findValidByAccessKey(String accessKey, LocalDateTime now);
// 사용처:
PlaceAccess pa = findValidByAccessKey(accessKey, now)
.orElseThrow(() -> new BadRequestException("..."));
validatePasswordMatch(password, pa);
return generateLoginResponse(pa);
// 장점: 원자적(atomic) 조회, 단일 책임, 명확한 에러 처리
// 단점: 쿼리 복잡도 증가
// 옵션 2: 명시적 검증 래퍼 메서드
private PlaceAccess getValidAccessKey(String key, String password) {
return findByAccessKey(key)
.filter(pa -> !pa.isExpired())
.filter(pa -> !pa.isDeleted())
.filter(pa -> passwordMatches(password, pa))
.orElseThrow(() -> new BadRequestException("..."));
}
// 장점: 체이닝 패턴으로 가독성 우수
// 단점: 다중 필터로 인한 성능 영향 가능현재 코드는 에러 메시지 일관성도 개선이 필요합니다 ("만료된 플레이스 접근 링크" vs "비밀번호 불일치" 메시지 통일). 3. 한글 Username 검증의 정규식 범위문제점: Pattern KOREAN_USERNAME_PATTERN = Pattern.compile(".*[ㄱ-ㅎㅏ-ㅣ가-힣].*");이 정규식은:
개선 제안: // 옵션 1: Unicode 블록 기반 (더 정확)
private static final Pattern KOREAN_USERNAME_PATTERN =
Pattern.compile(".*[\\p{Script=Hangul}].*");
// 테스트:
// 통과: "user123", "abc", "userABC"
// 실패: "사용자", "користувач", "用户"
// 옵션 2: 캐싱된 검증 (성능)
private static final Set<String> DISALLOWED_SCRIPTS = Set.of(
"Hangul", "Han", "Hiragana", "Katakana", "Cyrillic"
);
private boolean containsDisallowedCharacters(String username) {
return username.chars().anyMatch(ch -> {
UnicodeScript script = UnicodeScript.of(ch);
return DISALLOWED_SCRIPTS.contains(script.name());
});
}
// 장점: 더 정확하고, 다국어 확장 가능
// 단점: 정규식보다 약간 느림 (대량 검증 시에만 영향)권장사항: 정규식을 4. @Lockable 어노테이션의 SpEL 키 생성 일관성 부족문제점: // EventDateService
`@Lockable`(spelKey = "'EventDateService'.concat(T(String).valueOf(`#festivalId`))", ...)
// LineupService
`@Lockable`(spelKey = "'LineupService'.concat(T(String).valueOf(`#festivalId`))", ...)
// PlaceAccessService
`@Lockable`(spelKey = "'PlaceAccessService'.concat(T(String).valueOf(`#placeId`))", ...)
// AnnouncementService (String.valueOf 사용)
String.valueOf(`#festivalId`)이 불일관성은:
개선 제안: // 옵션 1: 공통 유틸리티 메서드 생성 (권장)
public class LockKeyGenerator {
public static String generateKey(String serviceName, String resourceId) {
return serviceName + ":" + resourceId;
}
}
// 사용처:
`@Lockable`(
spelKey = "T(com.daedan.festabook.global.lock.LockKeyGenerator)" +
".generateKey(`#serviceName`, `#resourceId`)",
useMethodScopeLock = false
)
// 장점: 중앙화, 일관성 보장, 테스트 용이
// 단점: 추가 클래스 필요
// 옵션 2: `@Lockable` 메타 어노테이션 확장
`@Target`(ElementType.METHOD)
`@Retention`(RetentionPolicy.RUNTIME)
`@Lockable`(useMethodScopeLock = false)
public `@interface` ResourceLock {
String value(); // "serviceName:resourceId" 형식
}
// 사용:
`@ResourceLock`("EventDateService:`#festivalId`")
public EventDateResponse createEventDate(...) { }
// 장점: 어노테이션 간결화, IDE 자동완성
// 단점: 커스텀 어노테이션 처리 필요5. 테스트의 과도한 확장문제점:
예시: // Staff 관련 테스트만 해도:
- StaffServiceTest: 443 라인 변경
- StaffControllerTest: 122 라인 변경
- StaffConcurrencyTest: 92 라인 추가
- StaffTest: 27 라인 추가
- StaffAuthorityTest: 2 라인 변경이는 다음 문제를 야기합니다:
권장사항: // 옵션 1: 테스트 카테고리 분리 (권장)
`@Tag`("unit") // 단위 테스트
class StaffServiceUnitTest { }
`@Tag`("concurrency") // 동시성 테스트 (별도 실행)
class StaffConcurrencyTest { }
// Maven/Gradle에서 selective 실행
// mvn test -Dgroups="!concurrency" (단위테스트만)
// mvn test -Dgroups="concurrency" (동시성테스트만)
// 옵션 2: Parameterized Test로 중복 제거
`@ParameterizedTest`
`@CsvSource`({
"valid_username, false",
"123abc, false",
"한글username, true"
})
void validateUsername(String username, boolean shouldFail) {
// 하나의 테스트로 여러 케이스 커버
}
// 장점: 명확한 책임 분리, 실행 시간 제어
// 단점: CI/CD 파이프라인 복잡도 증가종합 평가이 PR은 기술 부채 정산과 기능 확장의 혼합입니다. 스타일 통일, 보안 강화(한글 검증), 동시성 제어는 긍정적이나, 다음 지점에서 신중한 재검토가 필요합니다:
다음 스텝:
✨ Finishing Touches
🧪 Generate unit tests (beta)
|
|
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! 이 PR은 개발 환경의 변경 사항을 프로덕션 환경으로 병합하는 릴리스 PR입니다. 코드베이스의 전반적인 품질을 향상시키기 위한 다양한 리팩토링 작업과 테스트 개선이 포함되어 있습니다. 특히, null 검증 방식의 일관성을 확보하고, 메서드 정렬 규칙을 명확히 하여 코드 가독성을 높였습니다. 또한, 동시성 제어 로직을 도입하고 사용자 인증 관련 검증을 강화하여 시스템 안정성과 보안을 개선했습니다. 여러 테스트 케이스를 추가하고 기존 테스트를 정리하여 코드의 신뢰도를 높이는 데 중점을 두었습니다. 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/main/java/com/daedan/festabook/timetag/service/TimeTagService.java (1)
53-56:⚠️ Potential issue | 🟠 Major
exists후 삭제 흐름은 동시성에서 원자성이 깨질 수 있습니다.Line 55-56, Line 59-63 조합은 전형적인 Check-Then-Act 패턴이라, 동시 요청 시 “검증 시점엔 미사용 / 삭제 시점엔 사용 중” 상태가 생길 수 있습니다.
- 옵션 A(성능/단순성):
timeTagId기준 인메모리 락(@Lockable)으로deleteTimeTag를 직렬화
- 장점: 구현 단순, 현재 운영 형태에서 효과적
- 단점: 멀티 인스턴스로 확장 시 한계
- 옵션 B(정합성 최우선): DB 레벨 제약/원자 쿼리(조건부 삭제)로 최종 방어
- 장점: 인스턴스 수와 무관한 정합성 보장
- 단점: 쿼리/스키마 복잡도 증가
Based on learnings: "festabook/backend 프로젝트는 단일 인스턴스(single JVM) 환경으로 운영된다. 따라서 인메모리 락(Lockable)으로 Check-Then-Act 동시성 문제를 충분히 제어할 수 있다."
Also applies to: 59-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/daedan/festabook/timetag/service/TimeTagService.java` around lines 53 - 56, The delete flow uses a Check-Then-Act pattern (getTimeTagById -> validateTimeTagNotInUse -> timeTag.deleteBy) which can break atomicity under concurrency; wrap the deleteTimeTag method with the in-memory lock mechanism used in this codebase (annotate or invoke the `@Lockable` keyed by timeTagId) so the validation and deletion run serialized per timeTagId, e.g., apply `@Lockable` on deleteTimeTag or acquire the same lock inside deleteTimeTag before calling getTimeTagById, validateTimeTagNotInUse, and timeTag.deleteBy to prevent race conditions in single-JVM deployments.src/main/java/com/daedan/festabook/global/argumentresolver/OrganizationIdArgumentResolver.java (1)
40-46:⚠️ Potential issue | 🟡 Minor
parseOrganizationId메서드는private으로 변경하세요.현재
parseOrganizationId가public으로 선언되어 있지만, 이 클래스 내부(line 31)에서만 호출되며 외부 어디에서도 사용되지 않습니다. 이는 불필요한 공개 인터페이스를 노출하는 것으로, 이 메서드는HandlerMethodArgumentResolver의 구현 세부사항일 뿐 외부에서 직접 호출할 필요가 없습니다.
validateOrganizationId가 이미private static으로 작성된 것처럼,parseOrganizationId도private으로 변경하여 클래스의 진정한 공개 계약(public interface)을 명확하게 정의하는 것이 좋습니다. 이렇게 하면 나중에 이 메서드를 리팩토링할 때 더 자유롭고, 의도하지 않은 외부 의존성이 생길 위험도 줄일 수 있습니다.🤖 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/argumentresolver/OrganizationIdArgumentResolver.java` around lines 40 - 46, The method parseOrganizationId in OrganizationIdArgumentResolver is declared public but is only used internally; change its visibility to private to match validateOrganizationId (which is private static) and to avoid exposing implementation details of this HandlerMethodArgumentResolver implementation—locate the parseOrganizationId method and change its modifier from public to private, leaving its body and exception behavior unchanged.src/main/java/com/daedan/festabook/place/service/PlaceService.java (1)
61-68:⚠️ Potential issue | 🟠 Major클론 대상 검증이 축제 범위를 보장하지 않아 부분 복제가 조용히 발생할 수 있습니다.
Line 189는 “ID 존재 여부”만 확인하고, Line 65-68에서 축제 ID로 다시 필터링합니다. 이 조합이면 다른 축제 소속 ID가 섞여도 에러 없이 일부만 복제됩니다(정합성 저하).
옵션 A(성능 우선): 요청 ID를distinct후countByIdInAndFestivalId로 수량 일치 검증.
옵션 B(운영/디버깅 우선): 조회 결과와 요청 ID 차집합을 계산해 어떤 ID가 문제인지 명시적으로 예외 반환.수정 예시 (옵션 A)
@@ `@Transactional` public PlacesCloneResponse clonePlaces(Actor actor, Long festivalId, PlacesCloneRequest request) { validateClonePlacesSize(request.originalPlaceIds().size()); - validateExistsPlace(request.originalPlaceIds()); + List<Long> originalPlaceIds = request.originalPlaceIds().stream() + .distinct() + .toList(); + validateExistsPlaceInFestival(originalPlaceIds, festivalId); List<Place> originalPlaces = placeJpaRepository.findAllByIdInAndFestivalId( - request.originalPlaceIds(), + originalPlaceIds, festivalId ); @@ - private void validateExistsPlace(List<Long> originalPlaceIds) { - if (placeJpaRepository.countByIdIn(originalPlaceIds) != originalPlaceIds.size()) { + private void validateExistsPlaceInFestival(List<Long> originalPlaceIds, Long festivalId) { + long count = placeJpaRepository.countByIdInAndFestivalId(originalPlaceIds, festivalId); + if (count != originalPlaceIds.size()) { throw new NotFoundException(Place.class); } }Also applies to: 188-192
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/daedan/festabook/place/service/PlaceService.java` around lines 61 - 68, The clonePlaces flow currently validates only existence (validateExistsPlace) and then re-filters by festival when loading via placeJpaRepository.findAllByIdInAndFestivalId, allowing IDs from other festivals to be silently skipped; fix by ensuring the requested originalPlaceIds all belong to the given festival: either (A) dedupe request.originalPlaceIds(), call placeJpaRepository.countByIdInAndFestivalId(dedupedIds, festivalId) and compare counts, throwing an exception if mismatch, or (B) load originalPlaces with placeJpaRepository.findAllByIdInAndFestivalId(...) and compute the set difference between the requested IDs and originalPlaces.stream().map(Place::getId).collect(...); if any missing IDs exist, throw an exception listing which IDs are invalid so partial clones cannot occur. Ensure this replaces or augments the current validateExistsPlace check used in clonePlaces.src/main/java/com/daedan/festabook/event/service/EventDateService.java (1)
58-67:⚠️ Potential issue | 🟠 Majorupdate 경로에서
festivalId신뢰로 락/중복검증 범위가 어긋날 수 있습니다.Line 64~67에서 실제 수정 대상은
eventDateId로 조회한 엔티티인데, 중복 검증은 외부 입력festivalId를 사용합니다. 두 값이 불일치하면 잘못된 축제를 기준으로 검증/락 스코프가 잡힐 수 있습니다.
옵션 A(권장): 조회된eventDate의 소속 축제를 기준으로 검증하고, 불일치 시 예외 처리.
옵션 B: 업데이트 API에서festivalId를 제거하고eventDateId기반으로만 처리.🔧 보강 예시
public EventDateUpdateResponse updateEventDate( Actor actor, Long festivalId, Long eventDateId, EventDateUpdateRequest request ) { EventDate eventDate = getEventDateById(eventDateId); + Long ownerFestivalId = eventDate.getFestival().getId(); + if (!ownerFestivalId.equals(festivalId)) { + throw new NotFoundException(EventDate.class); + } if (!eventDate.getDate().isEqual(request.date())) { - validateDuplicatedEventDate(festivalId, request.date()); + validateDuplicatedEventDate(ownerFestivalId, request.date()); } eventDate.updateDateBy(actor, request.date()); return EventDateUpdateResponse.from(eventDate); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/daedan/festabook/event/service/EventDateService.java` around lines 58 - 67, The updateEventDate method currently uses the external festivalId input for duplicate/lock checks which can diverge from the retrieved EventDate's actual festival; change the logic to derive the festival id from the loaded EventDate (use getEventDateById(eventDateId) -> eventDate.getFestival().getId()) when calling validateDuplicatedEventDate and when establishing any locks, and if the provided festivalId does not match eventDate.getFestival().getId() throw a clear validation exception; ensure all references to festivalId inside updateEventDate are replaced with the authoritative id from the EventDate entity (or remove the incoming festivalId per Option B).src/main/java/com/daedan/festabook/organizer/service/OrganizerService.java (1)
46-59:⚠️ Potential issue | 🔴 Critical락 키가 조직 단위 불변식을 완전히 보호하지 못합니다.
현재 락 키가
username기준이라, 같은 조직에 서로 다른username으로 동시 요청이 들어오면validateOrganizationHasNoOrganizer()와 저장 사이에서 경쟁 조건이 생길 수 있습니다.
- 옵션 A: 락 키를
organizationId기준으로 변경
- 장점: “조직당 관리자 1명” 불변식을 직접 보호합니다.
- 단점: 같은 조직 내 생성 요청 직렬화로 처리량이 약간 줄 수 있습니다.
- 옵션 B: DB unique 제약(
organization_id)을 최종 안전장치로 추가 + 예외를 Conflict로 매핑
- 장점: 애플리케이션 락 누락에도 데이터 무결성을 보장합니다.
- 단점: 마이그레이션/예외 변환 코드가 추가됩니다.
🔧 제안 패치 (옵션 A)
`@Lockable`( - spelKey = "'OrganizerService'.concat(T(String).valueOf(`#request.username`()))", + spelKey = "'OrganizerService:organization:'.concat(T(String).valueOf(`#request.organizationId`()))", useMethodScopeLock = false ) `@Transactional` public OrganizerResponse createOrganizer(Actor actor, OrganizerRequest request) {Based on learnings: festabook/backend 프로젝트는 단일 인스턴스(single JVM) 환경으로 운영되며 인메모리 락으로 Check-Then-Act 동시성 문제를 제어한다.
🤖 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/service/OrganizerService.java` around lines 46 - 59, The lock key on the Lockable annotation in createOrganizer should use the organization identifier instead of username to prevent check-then-act races across different usernames in the same organization: update the spelKey expression on the Lockable annotation to derive the key from request.organizationId() (so the lock covers the organization), keep the transactional boundary and calls to validateOrganizationHasNoOrganizer and getOrganizationByOrganizationId unchanged, and ensure tests exercising concurrent createOrganizer calls for the same organization pass; alternatively, if you prefer DB-enforced safety instead, add a unique constraint on organization_id for organizers and map the resulting constraint-violation exception to a conflict response in the createOrganizer flow.src/test/java/com/daedan/festabook/place/controller/PlaceAccessControllerTest.java (1)
152-225:⚠️ Potential issue | 🟠 Major컨트롤러 통합 테스트 범위를 서비스 규칙 검증과 분리해 주세요.
Line 152-225의 만료/삭제 링크 실패 시나리오는 HTTP 라우팅/권한보다 비즈니스 규칙 검증 성격이 강해서, 컨트롤러 테스트가 과도하게 두꺼워지고 서비스 테스트와 중복될 리스크가 있습니다.
- 옵션 A: 컨트롤러 테스트는 성공 + 권한 중심으로 유지
- 장점: API 계약 검증에 집중, 유지보수 단순
- 단점: 에러 비즈니스 검증은 다른 계층으로 이동 필요
- 옵션 B: 만료/삭제 규칙은
PlaceAccessServiceTest로 이동
- 장점: 도메인 규칙 변경 시 영향 범위가 명확
- 단점: 컨트롤러 단에서의 세부 메시지 검증은 축소됨
As per coding guidelines: "Controller 통합 테스트 범위: Controller 테스트(통합 테스트)는 성공 케이스와 권한 테스트만 작성한다. 비즈니스 로직 검증은 Service/Domain 단위 테스트에서 수행한다."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/daedan/festabook/place/controller/PlaceAccessControllerTest.java` around lines 152 - 225, These two controller tests (PlaceAccessControllerTest methods 실패_만료된_플레이스_접근_링크 and 실패_삭제된_플레이스_접근_링크) are testing business rules (expiration/deletion) and should be removed from the controller integration test and moved to a service-level test (PlaceAccessServiceTest) that asserts the same error conditions and messages; alternatively, if you want to keep controller coverage, reduce the controller tests to only assert routing/authorization and the HTTP status (e.g., success and auth failure) and delete the business-rule setup/assertions from PlaceAccessControllerTest so that PlaceAccessServiceTest owns the detailed rule validations.
🧹 Nitpick comments (21)
src/test/java/com/daedan/festabook/global/logging/dto/LogDtoTest.java (3)
17-20: 각 테스트에// given구간도 명시해 주시면 컨벤션 일관성이 더 좋아집니다.지금도 테스트는 읽히지만, 팀 규칙상
given/when/then3구간을 고정하면 이후 셋업 추가 시 구조가 흔들리지 않아 유지보수가 편해집니다.As per coding guidelines "모든 테스트에 // given, // when, // then 주석을 작성하여 구간을 구분한다."
Also applies to: 44-47, 61-64, 78-81
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/daedan/festabook/global/logging/dto/LogDtoTest.java` around lines 17 - 20, Add explicit "// given" comments to each test to follow the given/when/then convention: in LogDtoTest.java (test methods like 성공() and the other test cases referenced) add a "// given" section before any setup code (e.g., constructing inputs or stubbing), keep the existing "// when" before ApiLog.from(...) or action calls, and ensure a "// then" before assertions so each test clearly has // given, // when, // then markers for consistency.
14-16: @nested 클래스명은 테스트 대상 메서드명과 동일하게 맞추는 편이 좋겠습니다.현재 구조는 의도는 명확하지만, 팀 규칙 기준으로는 메서드명 일치성이 약합니다.
옵션 A) DTO별 테스트 클래스를 분리하고 각 파일에서@Nested class from사용: 규칙 부합도가 높고 탐색이 쉽습니다(파일 수 증가 단점).
옵션 B) 한 파일 유지 시 DTO별 상위 중첩 클래스를 두고 그 아래from중첩 클래스로 구성: 파일 수는 유지하면서 규칙도 맞출 수 있습니다(중첩 깊이 증가 단점).As per coding guidelines "테스트하려는 메서드마다
@Nested클래스를 작성하며@Nested클래스명은 테스트하려는 메서드명과 동일하게 작성한다."Also applies to: 41-43, 58-60, 75-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/daedan/festabook/global/logging/dto/LogDtoTest.java` around lines 14 - 16, Rename the `@Nested` classes so their names exactly match the method under test ("from") — e.g., change class ApiLogFrom to class from (or restructure into a DTO-specific outer `@Nested` class and an inner `@Nested` class named from) for each DTO test in LogDtoTest (replace the other similarly named nested classes in this file the same way); ensure each `@Nested` targets the method "from" to comply with the guideline and maintain test discoverability.
84-84: 2단계 체이닝은 개행하면 코드베이스 스타일과 더 잘 맞습니다.기능 영향은 없고 가독성/일관성 개선 제안입니다.
제안 diff
- assertThat(result).isEqualTo(new MethodEventLog(LogType.METHOD_EVENT, "Controller", "getAll")); + assertThat(result) + .isEqualTo(new MethodEventLog(LogType.METHOD_EVENT, "Controller", "getAll"));As per coding guidelines "메서드 체이닝이 2단계 이상이면 반드시 개행한다."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/daedan/festabook/global/logging/dto/LogDtoTest.java` at line 84, 테스트의 메서드 체이닝이 두 단계 이상이므로 한 줄로 작성된 assert 구문을 개행해 읽기 좋게 만드세요: LogDtoTest의 assertThat(result).isEqualTo(new MethodEventLog(...))를 assertThat(result) 다음 줄로 개행하고 인덴트된 .isEqualTo(new MethodEventLog(LogType.METHOD_EVENT, "Controller", "getAll")) 형태로 바꿔 일관된 체이닝 스타일을 적용하세요.src/main/java/com/daedan/festabook/global/security/filter/JwtAuthenticationFilter.java (1)
53-61: 설계 관점 참고:setAuthentication()의 방어적 null 체크현재
userDetails != null체크는accountDetailsExtractor.extract()가 null을 반환할 경우를 대비한 방어적 코드입니다. 그러나 팀의 JWT 검증 컨벤션에 따르면, 토큰이 유효하나 필수 claim이 누락된 경우AuthenticationException을 던지도록 되어 있습니다.고려할 수 있는 두 가지 방향:
옵션 설명 장점 단점 A) 현행 유지 방어적 null 체크 유지 예상치 못한 null 반환에도 NPE 방지 extractor의 버그가 묻힐 수 있음 B) null 체크 제거 + 계약 명확화 extract()가 non-null 반환 보장의도가 명확하고 버그 조기 발견 extractor 구현이 계약을 위반하면 NPE 발생 현재 PR 범위에서 변경이 필요한 것은 아니며, 향후 리팩토링 시
accountDetailsExtractor.extract()의 반환 계약을@NonNull로 명시하고 이 방어 코드를 제거하는 방향을 검토해 볼 수 있습니다. (Based on learnings: JWT claim 누락 시 AuthenticationException을 던지도록 되어 있으므로, extractor가 null을 반환하는 경우는 서버 측 버그로 간주됩니다.)🤖 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/filter/JwtAuthenticationFilter.java` around lines 53 - 61, The current defensive null check in setAuthentication(AccountDetails userDetails) masks possible contract violations from accountDetailsExtractor.extract(); remove the if (userDetails != null) guard so a null return will surface (causing an NPE) and instead ensure accountDetailsExtractor.extract() declares/guarantees a non-null return (e.g., annotate with `@NonNull`) and throws AuthenticationException when required claims are missing; update any extractor implementations to throw AuthenticationException on invalid/missing claims rather than returning null so setAuthentication and UsernamePasswordAuthenticationToken always receive a valid AccountDetails.src/test/java/com/daedan/festabook/global/logging/RequestBodyCachingFilterTest.java (1)
38-49: 래핑 “타입” 외에 “원본 요청 연결성”도 함께 검증해두면 더 안전합니다.현재도 충분히 유의미하지만, 향후 구현이 바뀔 때 의도와 다른 객체를 전달하는 회귀를 더 잘 잡으려면 한 단계만 더 확인하는 것이 좋습니다.
- 옵션 A(현재 유지): 단순하고 테스트가 가볍습니다.
- 옵션 B(권장): wrapper 내부의 원본 request가 동일한지까지 검증해 안정성이 높아집니다.
제안 diff (옵션 B)
// then assertThat(filterChain.getRequest()).isInstanceOf(ContentCachingRequestWrapper.class); + ContentCachingRequestWrapper result = (ContentCachingRequestWrapper) filterChain.getRequest(); + assertThat(result.getRequest()).isSameAs(request);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/daedan/festabook/global/logging/RequestBodyCachingFilterTest.java` around lines 38 - 49, Update the test 성공_wrapper가_아니면_새로_감싸서_전달 to not only assert the request type but also verify the wrapper delegates to the original request: cast filterChain.getRequest() to ContentCachingRequestWrapper and assert that wrapper.getRequest() (or equivalent accessor) is the same instance as the original MockHttpServletRequest `request`; this ensures both type and original-request connectivity are validated.src/test/java/com/daedan/festabook/question/service/QuestionServiceTest.java (1)
73-74: 기대값을expected로 분리하면 테스트 의도가 더 명확해집니다.현재도 동작은 정확합니다. 다만 리터럴 직접 비교보다 기대값을 변수로 분리하면 규칙 일관성과 유지보수성이 좋아집니다.
✏️ 제안 변경
// then - assertThat(result.sequence()).isEqualTo(1); + int expected = 1; + assertThat(result.sequence()).isEqualTo(expected);As per coding guidelines "테스트 실행 결과 값 변수명: result. 기대값 변수명: expected."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/daedan/festabook/question/service/QuestionServiceTest.java` around lines 73 - 74, Extract the literal assertion value into an expected variable to follow the testing naming convention: declare an int expected = 1 (or appropriate type) and then use assertThat(result.sequence()).isEqualTo(expected); keep the actual variable named result (from result.sequence()) and the expected variable named expected to improve clarity and maintain consistency with other tests.src/test/java/com/daedan/festabook/place/service/PlaceServiceTest.java (1)
335-361: 테스트가 Mockito 기본 반환값에 의존해 의도 고정이 약합니다.현재는
placeImageJpaRepository,placeAnnouncementJpaRepository를 stub 하지 않아도(기본 empty 반환) 통과할 수 있습니다.
의도에 따라 아래 중 하나로 고정해두는 쪽을 권장드립니다.
- 옵션 A(성능/분기 보장): 기타 플레이스에서 조회를 아예 생략한다면
shouldHaveNoInteractions()로 단락 실행을 검증- 옵션 B(가독성/단순성): 조회는 수행하되 빈 결과를 기대한다면 두 repository를 명시적으로
given(...).willReturn(List.of())stub예시 (옵션 A)
// when PlaceResponse result = placeService.getPlaceByPlaceId(placeId); // then assertSoftly(s -> { s.assertThat(result.placeId()).isEqualTo(placeId); s.assertThat(result.placeImages().responses()).isEmpty(); s.assertThat(result.placeAnnouncements().responses()).isEmpty(); s.assertThat(result.timeTags().responses()).hasSize(1); s.assertThat(result.timeTags().responses().getFirst().name()).isEqualTo(timeTag.getName()); }); + then(placeImageJpaRepository).shouldHaveNoInteractions(); + then(placeAnnouncementJpaRepository).shouldHaveNoInteractions();As per coding guidelines, "테스트는 검증하려는 의도만 드러내야 한다. given에는 해당 테스트에서 검증하는 값만 노출" 규칙을 더 강하게 만족시키기 위한 제안입니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/daedan/festabook/place/service/PlaceServiceTest.java` around lines 335 - 361, The test 성공_기타_플레이스는_이미지와_공지_빈_응답() relies on Mockito's default empty returns for placeImageJpaRepository and placeAnnouncementJpaRepository which weakens intent; either explicitly stub those repos to return empty lists with given(placeImageJpaRepository.findAllByPlaceId(placeId)).willReturn(List.of()) and given(placeAnnouncementJpaRepository.findAllByPlaceId(placeId)).willReturn(List.of()) so the expectation is explicit, or assert no calls were made with verifyNoInteractions/placeImageJpaRepository and placeAnnouncementJpaRepository (or verify(placeImageJpaRepository, never()).findAllByPlaceId(...)) to document that getPlaceByPlaceId(placeId) must not query them.src/test/java/com/daedan/festabook/global/security/handler/CustomAccessDeniedHandlerTest.java (1)
26-27: 테스트 대상 주입을@InjectMocks로 통일해 주세요.현재 방식은 각 테스트에서 핸들러를 반복 생성해서 중복이 생기고, 추후 생성자 의존성이 늘어날 때 테스트별 초기화 누락 위험이 있습니다.
- 옵션 A(권장):
@InjectMocks로 일관 주입 → 중복 감소, 설정 일관성 확보- 옵션 B: 현 방식 유지 → 생성 과정이 명시적이라는 장점은 있지만, 팀 테스트 컨벤션과 어긋나고 유지보수 비용이 커집니다.
변경 예시
import org.mockito.Mock; +import org.mockito.InjectMocks; import org.mockito.junit.jupiter.MockitoExtension; `@ExtendWith`(MockitoExtension.class) `@DisplayNameGeneration`(DisplayNameGenerator.ReplaceUnderscores.class) class CustomAccessDeniedHandlerTest { `@Mock` private ObjectMapper objectMapper; + + `@InjectMocks` + private CustomAccessDeniedHandler handler; `@Nested` class handle { `@Test` void 성공_forbidden_json_응답() throws IOException { // given - CustomAccessDeniedHandler handler = new CustomAccessDeniedHandler(objectMapper); MockHttpServletRequest request = new MockHttpServletRequest("GET", "/test/uri"); MockHttpServletResponse response = new MockHttpServletResponse(); AccessDeniedException exception = new AccessDeniedException("denied"); @@ `@Test` void 예외_objectMapper_직렬화_실패() throws IOException { // given - CustomAccessDeniedHandler handler = new CustomAccessDeniedHandler(objectMapper); MockHttpServletRequest request = new MockHttpServletRequest("POST", "/test/uri"); MockHttpServletResponse response = new MockHttpServletResponse(); AccessDeniedException exception = new AccessDeniedException("denied");As per coding guidelines "**/*Test.java:
@Mock으로모킹하고@InjectMocks로테스트 대상에 주입한다."Also applies to: 35-36, 56-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/daedan/festabook/global/security/handler/CustomAccessDeniedHandlerTest.java` around lines 26 - 27, The tests currently create the handler per test and only mock ObjectMapper with `@Mock`; change to use `@InjectMocks` to inject mocks into the test subject: add an `@InjectMocks` field for CustomAccessDeniedHandler in CustomAccessDeniedHandlerTest and remove repeated in-test instantiations (references: CustomAccessDeniedHandler, objectMapper), ensure Mockito initialization is present (e.g., MockitoExtension or MockitoAnnotations.openMocks) so the `@InjectMocks` injection works, and remove duplicate manual constructions at the other indicated spots.src/main/java/com/daedan/festabook/staff/domain/Staff.java (1)
170-172:matches()대신find()를 사용하고 패턴을 단순화하면 의도가 더 명확해집니다.현재 패턴
.*[ㄱ-ㅎㅏ-ㅣ가-힣].*은 의도(한글 문자 존재 여부 검사)를 직접적으로 표현하기보다 전체 문자열 매칭에 맞춰져 있어,.*라는 불필요한 와일드카드가 추가되었습니다.find()를 사용하면 패턴이 더 간결해지고 검사 로직이 직관적입니다.
- 옵션 A(권장): 패턴을
[ㄱ-ㅎㅏ-ㅣ가-힣]으로 단순화하고find()사용
- 장점: 의도 명확, 패턴 가독성 향상, 불필요한 앞뒤
.*제거- 단점: 없음에 가까움
- 옵션 B: 현재 그대로 유지
- 장점: 현재 동작하고 있음
- 단점: 과도한 패턴 표현으로 인한 가독성 저하
제안 diff
- private static final Pattern KOREAN_USERNAME_PATTERN = Pattern.compile(".*[ㄱ-ㅎㅏ-ㅣ가-힣].*"); + private static final Pattern KOREAN_USERNAME_PATTERN = Pattern.compile("[ㄱ-ㅎㅏ-ㅣ가-힣]"); ... - if (KOREAN_USERNAME_PATTERN.matcher(username).matches()) { + if (KOREAN_USERNAME_PATTERN.matcher(username).find()) { throw new BadRequestException("아이디는 한글을 포함할 수 없습니다."); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/daedan/festabook/staff/domain/Staff.java` around lines 170 - 172, The current KOREAN_USERNAME_PATTERN usage in Staff.java should be simplified: replace the pattern that uses leading/trailing ".*" with a concise character class "[ㄱ-ㅎㅏ-ㅣ가-힣]" and use KOREAN_USERNAME_PATTERN.find() against username instead of matches(); update the conditional that throws BadRequestException("아이디는 한글을 포함할 수 없습니다.") to check KOREAN_USERNAME_PATTERN.find(username) so the intent (presence of any Korean character) is clearer and more readable.src/test/java/com/daedan/festabook/place/service/PlaceImageServiceTest.java (1)
171-185: 빈 요청 테스트는 “저장소 미호출”까지 검증하면 더 견고합니다.현재 테스트는 빈 요청에서도
findAllById스텁을 두고 있어, 서비스가 불필요한 조회를 해도 통과할 수 있습니다.
- 옵션 A(성능 우선): 빈 요청이면 즉시 반환하고 저장소 미호출을 검증
- 옵션 B(현 구조 유지): 저장소 조회를 의도한 설계라면 테스트명/의도를 명확히 표현
♻️ 제안 diff (옵션 A)
import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anySet; import static org.mockito.BDDMockito.given; +import static org.mockito.BDDMockito.then; +import static org.mockito.Mockito.never; @@ void 성공_빈_요청이면_빈_응답() { // given Actor adminActor = ActorFixture.admin(); List<PlaceImageSequenceUpdateRequest> requests = List.of(); - - given(placeImageJpaRepository.findAllById(Set.of())) - .willReturn(new ArrayList<>()); @@ // then assertThat(result.responses()).isEmpty(); + then(placeImageJpaRepository).should(never()).findAllById(anySet()); }As per coding guidelines "Service 테스트 — BDDMockito 패턴: ... then().should(never()) 호출되지 않음 검증."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/daedan/festabook/place/service/PlaceImageServiceTest.java` around lines 171 - 185, The test 성공_빈_요청이면_빈_응답 should assert the repository is not called for an empty request: change the test to treat an empty requests list as a fast-path and verify placeImageJpaRepository.findAllById is never invoked after calling placeImageService.updatePlaceImagesSequence(adminActor, requests) by using BDDMockito then(placeImageJpaRepository).should(never()).findAllById(anySet()) (or the equivalent never() verification); if the design intentionally calls the repo, instead update the test name/intent to reflect that behavior.src/main/java/com/daedan/festabook/question/domain/Question.java (1)
142-149:validateQuestion에서도StringUtils.hasText()사용을 고려해 보세요.
validateAnswer메서드(Line 152)에서는StringUtils.hasText(answer)를 사용하여 null, 빈 문자열, 공백만 있는 문자열을 한 번에 검증하고 있습니다. 반면validateQuestion메서드에서는 여전히question == null || question.trim().isEmpty()패턴을 사용하고 있어 일관성이 떨어집니다.두 가지 선택지가 있습니다:
- 옵션 A:
validateQuestion도StringUtils.hasText()로 변경하여 일관성 확보- 옵션 B: 현재 상태 유지 (동작은 동일하므로 기능적 문제는 없음)
동작 결과는 같지만, 같은 클래스 내에서 동일한 검증 패턴을 사용하면 코드 가독성과 유지보수성이 향상됩니다.
♻️ 일관성 개선을 위한 제안
private void validateQuestion(String question) { - if (question == null || question.trim().isEmpty()) { + if (!StringUtils.hasText(question)) { throw new BadRequestException("질문은 비어 있을 수 없습니다."); } if (question.length() > MAX_QUESTION_LENGTH) { throw new BadRequestException(String.format("질문은 %d자를 초과할 수 없습니다.", MAX_QUESTION_LENGTH)); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/daedan/festabook/question/domain/Question.java` around lines 142 - 149, The validateQuestion method currently uses question == null || question.trim().isEmpty(), which is inconsistent with validateAnswer's use of StringUtils.hasText(answer); update validateQuestion to use StringUtils.hasText(question) for the null/empty/whitespace check (throw the same BadRequestException when it returns false) so both validation methods use the same utility; locate the method validateQuestion in class Question and replace the existing null/trim check with a hasText-based check while keeping the MAX_QUESTION_LENGTH length validation unchanged.src/test/java/com/daedan/festabook/organization/controller/OrganizationControllerTest.java (1)
92-114: ROLE_PLACE_ACCESS 거부 시나리오를 별도 고정 테스트로 두는 편이 안전합니다.Line 93에서 성공 대상 Role만 검증하고 있어, 보안 설정이 완화되어도 회귀를 바로 잡기 어렵습니다.
옵션 A:ROLE_PLACE_ACCESS단건 403 테스트를 추가(가독성 좋고 의도 명확).
옵션 B: Role/ExpectedStatus를 매핑한 파라미터 테스트로 통합(중복 감소, 다만 케이스 가독성은 다소 낮아짐).As per coding guidelines
**/*ControllerTest.java: Controller 테스트(통합 테스트)는 성공 케이스와 권한 테스트만 작성한다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/daedan/festabook/organization/controller/OrganizationControllerTest.java` around lines 92 - 114, The parameterized test method 성공 in OrganizationControllerTest currently excludes ROLE_PLACE_ACCESS but doesn't assert the denied-role behavior; add a separate fixed test (e.g., 권한_없음_PLACE_ACCESS_403) that builds the same request headers using createAuthorizationHeader(organization.getId(), RoleType.ROLE_PLACE_ACCESS) and createOrganizationIdHeader(organization.getId()), performs GET "/organizations", and asserts HttpStatus.FORBIDDEN (403) to ensure ROLE_PLACE_ACCESS denial is explicitly verified; alternatively, if you prefer a single parameterized test, convert 성공 to supply pairs of (RoleType, expectedStatus) and assert the response status accordingly, but prefer the explicit single-case test for clarity.src/test/java/com/daedan/festabook/organization/service/OrganizationServiceTest.java (1)
108-117: 실행 결과 변수명을result로 통일해 주세요.Line 108의
response는 팀 테스트 컨벤션과 달라 검색/리뷰 일관성이 떨어집니다. 기능 이슈는 아니므로 다음 정리 때 맞추면 충분합니다.As per coding guidelines
**/*Test.java: 테스트 실행 결과 값 변수명: result. 기대값 변수명: expected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/daedan/festabook/organization/service/OrganizationServiceTest.java` around lines 108 - 117, Rename the local test result variable from response to result in the OrganizationServiceTest method that calls organizationService.getOrganizationByOrganizationId (replace the OrganizationResponse response declaration and every usage inside the assertSoftly block with OrganizationResponse result), keeping the same value and assertions; ensure the variable name follows the test convention (result for actual, expected for expected) and update any imports or references within the method accordingly.src/main/java/com/daedan/festabook/place/service/PlaceAccessService.java (1)
69-73: 로그인 시 accessKey를 2번 조회해 일관성과 성능에서 손해가 있습니다.Line 71
73, 109117 구조는exists+find이중 조회라 DB 왕복이 늘고, 만료 경계 시점에는 두 조회 결과가 달라 예외 의미가 흔들릴 수 있습니다.
옵션 A(권장): 1회 조회 후 상태(삭제/만료/비밀번호)를 분기 처리해 예외를 일관화.
옵션 B: 현재 구조를 유지하되, 경계 시점 정책(어떤 예외를 우선할지)을 명시하고 테스트로 고정.Also applies to: 109-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/daedan/festabook/place/service/PlaceAccessService.java` around lines 69 - 73, The code performs two DB lookups for the same accessKey (validatePlaceAccessIsNotExpiredOrDeleted(...) then getActivePlaceAccessByAccessKey(...)), causing extra roundtrips and inconsistent behavior at expiry boundaries; fix by consolidating to a single retrieval in loginPlaceAccess (e.g., fetch an Optional/nullable PlaceAccess once via the repository lookup used by getActivePlaceAccessByAccessKey), then inspect that single PlaceAccess instance to decide whether to throw the deleted/expired/password-mismatch exceptions (replace validatePlaceAccessIsNotExpiredOrDeleted(...) and the second find), ensuring all decision branches (deleted, expired, wrong password) are derived from that one result so behavior and tests remain deterministic.code-style.md (1)
1376-1381: 동일 규칙이 중복 정의되어 문서 드리프트 위험이 있습니다.Line 1376
1381(12.22)와 Line 15821588(13.8)이 사실상 동일 규칙이고, 체크리스트도 Line 1771~1772와 Line 1787에서 같은 항목이 반복됩니다.
옵션 A(권장): 규칙 본문은 한 곳(예: 12.22)만 유지하고 다른 섹션은 참조 링크로 대체.
옵션 B: 중복 유지 시 “원본 섹션”을 명시해 동기화 기준을 고정.📝 정리 예시
-### 13.8 테스트에서 도메인/값객체 생성 방식 -- 적용 범위: `**/*Test.java` (모든 테스트) -- 테스트 본문에서는 도메인(Entity, VO)을 `new`로 직접 생성하지 않는다. -- 도메인/값객체는 반드시 Fixture를 통해 생성한다. -- 값객체(`Coordinate` 등)도 동일하게 `CoordinateFixture.create(...)` 형태로 사용한다. +### 13.8 테스트에서 도메인/값객체 생성 방식 +- 본 규칙은 12.22를 단일 원본(Source of Truth)으로 따른다. +- 부록 체크리스트도 12.22 기준 단일 항목으로 유지한다.Also applies to: 1582-1588, 1771-1772, 1787-1787
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code-style.md` around lines 1376 - 1381, The rule "테스트 도메인/값객체 생성 방식" is duplicated across sections 12.22 and 13.8 and repeated in checklist entries around lines 1771 and 1787; consolidate by keeping a single canonical definition (e.g., retain 12.22) and replace the duplicate section (13.8) with a cross-reference to 12.22, and either remove or replace the repeated checklist items with a reference to the canonical checklist entry; if you choose to keep duplicates, explicitly mark one as "원본(참조 기준)" and add a "see: 12.22" note in the other locations so the synchronization source is clear.src/test/java/com/daedan/festabook/organizer/controller/OrganizerControllerTest.java (1)
117-117:@Nested클래스명은 테스트 대상 메서드명과 맞추면 탐색성이 더 좋아집니다.
loginOrganization은 테스트 본문 의도(organizer 로그인)와 살짝 어긋나 보여, 실제 메서드명 기준으로 맞추면 추적이 쉬워집니다.As per coding guidelines:
테스트하려는 메서드마다@nested클래스를 작성하며@nested클래스명은 테스트하려는 메서드명과 동일하게 작성한다.🤖 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/controller/OrganizerControllerTest.java` at line 117, The `@Nested` class name loginOrganization does not match the tested method name; rename the nested test class in OrganizerControllerTest from loginOrganization to the actual method name under test (e.g., loginOrganizer or the exact controller method name) so it follows the guideline that each `@Nested` class name equals the method being tested and improves discoverability; update any references/imports and test annotations accordingly.src/test/java/com/daedan/festabook/place/concurrency/PlaceAnnouncementConcurrencyTest.java (1)
74-81: 동시성 테스트가 helper 기본 요청 수에 암묵 의존하고 있습니다.지금 구조는 helper 기본 설정이 바뀌면 테스트 이름/의도(“3개 이상 동시 요청”)와 실제 검증이 쉽게 어긋날 수 있습니다.
- 옵션 A: 요청 수를 명시적으로 설정하고(예: 5 이상) 결과를 검증
- 장점: 테스트 의도가 고정됩니다.
- 단점: helper 오버로드 사용이 필요할 수 있습니다.
- 옵션 B:
ConcurrencyTestResult를 받아 실제requestCount를 함께 검증
- 장점: helper 설정 변경에 덜 취약합니다.
- 단점: assertion이 1~2개 늘어납니다.
🔍 제안 패치 (옵션 B)
+import com.daedan.festabook.support.ConcurrencyTestResult; ... - concurrencyTestHelper.execute(httpRequest); + ConcurrencyTestResult concurrencyResult = concurrencyTestHelper.execute(httpRequest); // then - Integer result = placeAnnouncementJpaRepository.countByPlaceId(place.getId()); - assertThat(result).isEqualTo(expectedPlaceAnnouncementCount); + assertThat(concurrencyResult.getRequestCount()).isGreaterThanOrEqualTo(3); + Integer result = placeAnnouncementJpaRepository.countByPlaceId(place.getId()); + assertThat(result).isEqualTo(expectedPlaceAnnouncementCount);Based on learnings: 동시성 테스트는 concurrencyTestHelper를 활용한다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/daedan/festabook/place/concurrency/PlaceAnnouncementConcurrencyTest.java` around lines 74 - 81, The test currently relies on concurrencyTestHelper's implicit default request count; update PlaceAnnouncementConcurrencyTest to capture the ConcurrencyTestResult returned by concurrencyTestHelper.execute(httpRequest) (use the overload that returns ConcurrencyTestResult or change execute to return it), then assert the actual requestCount from that result meets the test's intent (e.g., >= 3) before asserting the DB state via placeAnnouncementJpaRepository.countByPlaceId(place.getId()); reference concurrencyTestHelper.execute, ConcurrencyTestResult.getRequestCount (or equivalent), and placeAnnouncementJpaRepository.countByPlaceId to locate and change the assertions.src/test/java/com/daedan/festabook/staff/controller/StaffControllerTest.java (1)
515-545: 컨트롤러 통합 테스트 범위를 조금만 줄이면 유지보수가 더 좋아집니다.현재 구간은 “비밀번호 실제 변경 여부”와 “비밀번호 불일치 비즈니스 예외”까지 컨트롤러 테스트가 직접 검증하고 있어, 계층 책임이 섞여 보입니다.
- 옵션 A: 해당 검증을
StaffServiceTest로 이동
- 장점: 테스트 실패 원인(라우팅/권한 vs 비즈니스)이 분리되어 디버깅이 빨라집니다.
- 단점: HTTP 레벨에서의 상세 행위 검증은 줄어듭니다.
- 옵션 B: 컨트롤러 테스트에는 성공/권한만 유지하고, 비밀번호 규칙은 서비스 단위 테스트로 집중
- 장점: 팀 컨벤션과 가장 일관적입니다.
- 단점: 테스트 파일이 분산됩니다.
As per coding guidelines:
**/*ControllerTest.java: Controller 테스트(통합 테스트)는 성공 케이스와 권한 테스트만 작성한다. 비즈니스 로직 검증은 Service/Domain 단위 테스트에서 수행한다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/daedan/festabook/staff/controller/StaffControllerTest.java` around lines 515 - 545, Controller test currently asserts business logic (password actually changed and password mismatch handling) inside StaffControllerTest; move those business-logic assertions into unit tests on the service layer (StaffServiceTest) and keep the controller integration test focused only on routing/auth and status codes. Specifically: remove assertions that inspect persistence or use passwordEncoder.matches from StaffControllerTest (e.g., the block using staffJpaRepository.findById(...) and passwordEncoder.matches(...) and the detailed “비밀번호_불일치” logic) and instead add service-level tests that call StaffService#updatePassword (or the service method used by the controller) to verify that the password is updated in the repository and that wrong current passwords throw the expected exception; in the controller tests (StaffControllerTest) only assert HTTP status codes for success and forbidden/unauthorized and keep the request setup (StaffPasswordUpdateRequest fixture) and authorization header scaffolding to validate routing/auth behavior.src/main/java/com/daedan/festabook/staff/service/StaffService.java (1)
133-142:updateStaffAuthorities검증 경계(TODO)를 이번 릴리즈에서 확정해두는 게 안전합니다.현재는 null 체크 중심이라 “어디서 권한/소속 정합성을 최종 보장하는지”가 코드만으로 명확히 드러나지 않습니다.
- 옵션 A: Service에서
staffId - organizationId - festivalIds소속 일치 선검증
- 장점: API 경계에서 실패 원인이 명확합니다.
- 단점: 서비스 로직이 다소 비대해질 수 있습니다.
- 옵션 B:
StaffAuthority.createBy/deleteBy도메인 메서드에서 불변식 강제 + 서비스 TODO 제거
- 장점: 권한/불변식 책임이 도메인에 모여 누락 위험이 줄어듭니다.
- 단점: 도메인 테스트 보강이 필요합니다.
As per coding guidelines:
권한 검증 누락 방지를 위해 CRUD 권한 검증을 도메인 내부로 강제한다. Service는 orchestration만 담당하고 권한 판단은 엔티티가 수행한다.Also applies to: 277-286
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/daedan/festabook/staff/service/StaffService.java` around lines 133 - 142, 현재 TODO는 권한·소속 정합성 검증 경계가 불명확하므로, 코딩 가이드에 따라 검증 책임을 도메인으로 이동해 확정하세요: 제거하려는 사전 검사(validateStaffAuthorityUpdateRequest)를 서비스(updateStaffAuthorities)에서 없애고 대신 StaffAuthority.createBy(...) 및 StaffAuthority.deleteBy(...) 내부에 staffId↔organizationId↔festivalIds 불변식(소속 일치, null/존재 검사 등)을 강제로 구현하여 예외를 던지도록 하며 서비스는 해당 도메인 메서드를 호출해 예외를 핸들링(or rethrow)하는 오케스트레이션만 담당하게 만드세요; 동일한 방식으로 다른 케이스(참조된 277-286 범위)도 도메인 검증으로 이관해 TODO를 제거하세요.src/test/java/com/daedan/festabook/place/service/PlaceAccessServiceTest.java (1)
243-244: 토큰 생성 인자 검증을 더 구체화하면 회귀 탐지가 강화됩니다.Line 243-244에서
createPlaceAccessToken(any(), eq(placeAccess.getRoles()))로 첫 번째 인자를 느슨하게 잡아, 토큰 주체/클레임 연결이 잘못되어도 테스트가 통과할 수 있습니다.
- 옵션 A:
eq(expectedValue)로 엄격 검증
- 장점: 회귀 탐지력이 가장 강함
- 단점: 구현 변경 시 테스트 수정 빈도 증가
- 옵션 B:
ArgumentCaptor로 캡처 후 핵심 필드만 검증
- 장점: 의도 검증 + 구현 변화 내성 균형
- 단점: 코드가 약간 길어짐
Also applies to: 247-254
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/daedan/festabook/place/service/PlaceAccessServiceTest.java` around lines 243 - 244, The mock for jwtProvider.createPlaceAccessToken in PlaceAccessServiceTest is using any() for the first argument which weakens regression detection; replace the loose matcher by either (A) using eq(expectedSubjectOrClaims) to strictly verify the exact subject/claims passed into createPlaceAccessToken (use the expected value derived from your test fixture) or (B) use an ArgumentCaptor for the first parameter, capture the token payload passed to jwtProvider.createPlaceAccessToken, and assert the critical fields (e.g., subject, place id, roles) match expectations; apply the same stricter verification to the other occurrences mentioned around lines 247-254.src/test/java/com/daedan/festabook/global/lock/LockAspectTest.java (1)
145-165: @nested 클래스명을 테스트 대상 메서드명과 맞추는 편이 탐색성이 좋습니다.현재
parameterMetadata는 범주형 이름이라, 어떤 메서드를 검증하는지 한 번 더 읽어야 합니다. 메서드명 기준으로 맞추면 유지보수 시 추적이 빨라집니다.🔧 제안 diff
- class parameterMetadata { + class createPlaceAccess {As per coding guidelines "테스트하려는 메서드마다
@Nested클래스를 작성하며 Nested 클래스명은 테스트하려는 메서드명과 동일하게 작성한다."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/daedan/festabook/global/lock/LockAspectTest.java` around lines 145 - 165, The `@Nested` class name parameterMetadata is too generic; rename it to match the tested method name to improve discoverability — change the nested class currently named parameterMetadata to createPlaceAccess (or CreatePlaceAccess) so it clearly indicates it contains tests for PlaceAccessService.createPlaceAccess; update the nested class declaration that wraps the test method 성공_PlaceAccessService_메서드_파라미터명이_보존된다 to the new name and adjust any references/imports accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 64a5963a-ef05-4ad4-a0dc-0d2dc7985f55
📒 Files selected for processing (138)
code-style.mdsrc/main/java/com/daedan/festabook/announcement/domain/Announcement.javasrc/main/java/com/daedan/festabook/announcement/service/AnnouncementService.javasrc/main/java/com/daedan/festabook/event/domain/Event.javasrc/main/java/com/daedan/festabook/event/domain/EventDate.javasrc/main/java/com/daedan/festabook/event/service/EventDateService.javasrc/main/java/com/daedan/festabook/festival/domain/Coordinate.javasrc/main/java/com/daedan/festabook/festival/domain/Festival.javasrc/main/java/com/daedan/festabook/festival/domain/FestivalImage.javasrc/main/java/com/daedan/festabook/festival/domain/FestivalNotification.javasrc/main/java/com/daedan/festabook/festival/dto/FestivalCoordinateResponse.javasrc/main/java/com/daedan/festabook/festival/service/FestivalNotificationService.javasrc/main/java/com/daedan/festabook/festival/service/TestFestivalNotificationService.javasrc/main/java/com/daedan/festabook/global/argumentresolver/ActorArgumentResolver.javasrc/main/java/com/daedan/festabook/global/argumentresolver/OrganizationIdArgumentResolver.javasrc/main/java/com/daedan/festabook/global/lock/infrastructure/InMemoryLockStorage.javasrc/main/java/com/daedan/festabook/global/security/authorization/AccountDetailsExtractor.javasrc/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetails.javasrc/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetailsExtractStrategy.javasrc/main/java/com/daedan/festabook/global/security/authorization/PlaceAccessDetails.javasrc/main/java/com/daedan/festabook/global/security/authorization/PlaceAccessDetailsExtractStrategy.javasrc/main/java/com/daedan/festabook/global/security/authorization/StaffDetails.javasrc/main/java/com/daedan/festabook/global/security/authorization/StaffDetailsExtractStrategy.javasrc/main/java/com/daedan/festabook/global/security/filter/JwtAuthenticationFilter.javasrc/main/java/com/daedan/festabook/lineup/domain/Lineup.javasrc/main/java/com/daedan/festabook/lineup/service/LineupService.javasrc/main/java/com/daedan/festabook/lostitem/domain/LostItem.javasrc/main/java/com/daedan/festabook/lostitem/service/LostItemService.javasrc/main/java/com/daedan/festabook/notification/dto/NotificationSendRequest.javasrc/main/java/com/daedan/festabook/organization/domain/Organization.javasrc/main/java/com/daedan/festabook/organizer/controller/OrganizerController.javasrc/main/java/com/daedan/festabook/organizer/domain/Organizer.javasrc/main/java/com/daedan/festabook/organizer/service/OrganizerService.javasrc/main/java/com/daedan/festabook/place/controller/PlaceAccessController.javasrc/main/java/com/daedan/festabook/place/controller/PlaceImageController.javasrc/main/java/com/daedan/festabook/place/domain/Place.javasrc/main/java/com/daedan/festabook/place/domain/PlaceAccess.javasrc/main/java/com/daedan/festabook/place/domain/PlaceAnnouncement.javasrc/main/java/com/daedan/festabook/place/domain/PlaceImage.javasrc/main/java/com/daedan/festabook/place/dto/PlaceCoordinateRequest.javasrc/main/java/com/daedan/festabook/place/service/PlaceAccessService.javasrc/main/java/com/daedan/festabook/place/service/PlaceAnnouncementService.javasrc/main/java/com/daedan/festabook/place/service/PlaceGeographyService.javasrc/main/java/com/daedan/festabook/place/service/PlaceImageService.javasrc/main/java/com/daedan/festabook/place/service/PlaceService.javasrc/main/java/com/daedan/festabook/question/domain/Question.javasrc/main/java/com/daedan/festabook/question/service/QuestionService.javasrc/main/java/com/daedan/festabook/staff/controller/StaffController.javasrc/main/java/com/daedan/festabook/staff/domain/Staff.javasrc/main/java/com/daedan/festabook/staff/domain/StaffAuthority.javasrc/main/java/com/daedan/festabook/staff/service/StaffService.javasrc/main/java/com/daedan/festabook/storage/infrastructure/StoreFile.javasrc/main/java/com/daedan/festabook/storage/service/ImageStoreService.javasrc/main/java/com/daedan/festabook/timetag/domain/PlaceTimeTag.javasrc/main/java/com/daedan/festabook/timetag/domain/TimeTag.javasrc/main/java/com/daedan/festabook/timetag/service/TimeTagService.javasrc/test/java/com/daedan/festabook/announcement/controller/AnnouncementControllerTest.javasrc/test/java/com/daedan/festabook/announcement/domain/AnnouncementTest.javasrc/test/java/com/daedan/festabook/announcement/infrastructure/AnnouncementJpaRepositoryTest.javasrc/test/java/com/daedan/festabook/announcement/service/AnnouncementServiceTest.javasrc/test/java/com/daedan/festabook/event/concurrency/EventDateConcurrencyTest.javasrc/test/java/com/daedan/festabook/event/controller/EventControllerTest.javasrc/test/java/com/daedan/festabook/event/domain/EventDateTest.javasrc/test/java/com/daedan/festabook/event/domain/EventTest.javasrc/test/java/com/daedan/festabook/festival/controller/AndroidFestivalNotificationSubscriptionControllerTest.javasrc/test/java/com/daedan/festabook/festival/controller/FestivalControllerTest.javasrc/test/java/com/daedan/festabook/festival/controller/FestivalImageControllerTest.javasrc/test/java/com/daedan/festabook/festival/controller/IosFestivalNotificationSubscriptionControllerTest.javasrc/test/java/com/daedan/festabook/festival/controller/TestFestivalNotificationSubscriptionControllerTest.javasrc/test/java/com/daedan/festabook/festival/domain/CoordinateTest.javasrc/test/java/com/daedan/festabook/festival/domain/FestivalFixture.javasrc/test/java/com/daedan/festabook/festival/domain/FestivalImageFixture.javasrc/test/java/com/daedan/festabook/festival/domain/FestivalImageTest.javasrc/test/java/com/daedan/festabook/festival/domain/FestivalNotificationTest.javasrc/test/java/com/daedan/festabook/festival/domain/FestivalTest.javasrc/test/java/com/daedan/festabook/festival/dto/FestivalCreateRequestFixture.javasrc/test/java/com/daedan/festabook/festival/dto/FestivalGeographyPolygonHoleBoundaryUpdateRequestFixture.javasrc/test/java/com/daedan/festabook/festival/service/FestivalNotificationServiceTest.javasrc/test/java/com/daedan/festabook/festival/service/FestivalServiceTest.javasrc/test/java/com/daedan/festabook/festival/service/TestFestivalNotificationServiceTest.javasrc/test/java/com/daedan/festabook/global/argumentresolver/ActorArgumentResolverUnitTest.javasrc/test/java/com/daedan/festabook/global/argumentresolver/FestivalIdArgumentResolverUnitTest.javasrc/test/java/com/daedan/festabook/global/exception/InvalidFileExceptionTest.javasrc/test/java/com/daedan/festabook/global/exception/LockExceptionTest.javasrc/test/java/com/daedan/festabook/global/infrastructure/ShuffleManagerTest.javasrc/test/java/com/daedan/festabook/global/lock/LockAspectTest.javasrc/test/java/com/daedan/festabook/global/lock/infrastructure/InMemoryLockStorageTest.javasrc/test/java/com/daedan/festabook/global/logging/RequestBodyCachingFilterTest.javasrc/test/java/com/daedan/festabook/global/logging/dto/LogDtoTest.javasrc/test/java/com/daedan/festabook/global/security/authorization/ActorFixture.javasrc/test/java/com/daedan/festabook/global/security/authorization/AdminDetailsTest.javasrc/test/java/com/daedan/festabook/global/security/authorization/OrganizerDetailsTest.javasrc/test/java/com/daedan/festabook/global/security/authorization/PlaceAccessDetailsTest.javasrc/test/java/com/daedan/festabook/global/security/authorization/StaffDetailsTest.javasrc/test/java/com/daedan/festabook/global/security/authorization/TestActor.javasrc/test/java/com/daedan/festabook/global/security/handler/CustomAccessDeniedHandlerTest.javasrc/test/java/com/daedan/festabook/global/security/util/JwtProviderTest.javasrc/test/java/com/daedan/festabook/lineup/concurrency/LineupConcurrencyTest.javasrc/test/java/com/daedan/festabook/lineup/controller/LineupControllerTest.javasrc/test/java/com/daedan/festabook/lineup/domain/LineupTest.javasrc/test/java/com/daedan/festabook/lostitem/controller/LostItemControllerTest.javasrc/test/java/com/daedan/festabook/lostitem/domain/LostItemTest.javasrc/test/java/com/daedan/festabook/notification/dto/NotificationSendRequestTest.javasrc/test/java/com/daedan/festabook/notification/infrastructure/FcmNotificationManagerTest.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/concurrency/OrganizerConcurrencyTest.javasrc/test/java/com/daedan/festabook/organizer/controller/OrganizerControllerTest.javasrc/test/java/com/daedan/festabook/organizer/domain/OrganizerTest.javasrc/test/java/com/daedan/festabook/organizer/service/OrganizerServiceTest.javasrc/test/java/com/daedan/festabook/place/concurrency/PlaceAccessConcurrencyTest.javasrc/test/java/com/daedan/festabook/place/concurrency/PlaceAnnouncementConcurrencyTest.javasrc/test/java/com/daedan/festabook/place/controller/PlaceAccessControllerTest.javasrc/test/java/com/daedan/festabook/place/controller/PlaceControllerTest.javasrc/test/java/com/daedan/festabook/place/domain/PlaceAccessTest.javasrc/test/java/com/daedan/festabook/place/domain/PlaceAnnouncementTest.javasrc/test/java/com/daedan/festabook/place/domain/PlaceImageTest.javasrc/test/java/com/daedan/festabook/place/domain/PlaceTest.javasrc/test/java/com/daedan/festabook/place/infrastructure/PlaceAccessJpaRepositoryTest.javasrc/test/java/com/daedan/festabook/place/infrastructure/PlaceImageJpaRepositoryTest.javasrc/test/java/com/daedan/festabook/place/service/PlaceAccessServiceTest.javasrc/test/java/com/daedan/festabook/place/service/PlaceImageServiceTest.javasrc/test/java/com/daedan/festabook/place/service/PlaceServiceTest.javasrc/test/java/com/daedan/festabook/question/controller/QuestionControllerTest.javasrc/test/java/com/daedan/festabook/question/domain/QuestionTest.javasrc/test/java/com/daedan/festabook/question/service/QuestionServiceTest.javasrc/test/java/com/daedan/festabook/staff/concurrency/StaffConcurrencyTest.javasrc/test/java/com/daedan/festabook/staff/controller/StaffControllerTest.javasrc/test/java/com/daedan/festabook/staff/domain/StaffAuthorityTest.javasrc/test/java/com/daedan/festabook/staff/domain/StaffTest.javasrc/test/java/com/daedan/festabook/staff/service/StaffServiceTest.javasrc/test/java/com/daedan/festabook/storage/controller/ImageStoreControllerTest.javasrc/test/java/com/daedan/festabook/storage/infrastructure/MockStorageManagerTest.javasrc/test/java/com/daedan/festabook/storage/infrastructure/StoreFileTest.javasrc/test/java/com/daedan/festabook/storage/service/ImageStoreServiceTest.javasrc/test/java/com/daedan/festabook/timetag/controller/TimeTagControllerTest.javasrc/test/java/com/daedan/festabook/timetag/domain/PlaceTimeTagTest.javasrc/test/java/com/daedan/festabook/timetag/domain/TimeTagTest.java
💤 Files with no reviewable changes (9)
- src/main/java/com/daedan/festabook/place/service/PlaceImageService.java
- src/test/java/com/daedan/festabook/timetag/controller/TimeTagControllerTest.java
- src/test/java/com/daedan/festabook/question/controller/QuestionControllerTest.java
- src/test/java/com/daedan/festabook/lineup/controller/LineupControllerTest.java
- src/test/java/com/daedan/festabook/festival/controller/FestivalImageControllerTest.java
- src/test/java/com/daedan/festabook/lostitem/controller/LostItemControllerTest.java
- src/main/java/com/daedan/festabook/place/dto/PlaceCoordinateRequest.java
- src/main/java/com/daedan/festabook/festival/dto/FestivalCoordinateResponse.java
- src/test/java/com/daedan/festabook/announcement/controller/AnnouncementControllerTest.java
Comment on lines
+125
to
+129
| private void validatePinnedLimit(Long festivalId) { | ||
| Long pinnedCount = announcementJpaRepository.countByFestivalIdAndIsPinnedTrue(festivalId); | ||
| if (pinnedCount >= MAX_PINNED_ANNOUNCEMENTS) { | ||
| throw new BadRequestException(String.format("공지글은 최대 %d개까지 고정할 수 있습니다.", MAX_PINNED_ANNOUNCEMENTS)); | ||
| } |
There was a problem hiding this comment.
고정 공지 제한 검증이 요청 festivalId 신뢰에 의존해 우회될 수 있습니다.
현재 제한 검증은 전달받은 festivalId 기준으로만 수행됩니다. announcementId가 속한 실제 축제와 요청 festivalId가 다를 때, 다른 축제의 카운트를 검사한 뒤 원래 공지를 고정할 수 있어 데이터 무결성이 깨질 수 있습니다.
- 옵션 A (안전성 우선): 실제 공지의 축제 ID(
announcement.getFestival().getId())로만 제한 검증하고, 요청 ID와 불일치하면 즉시 예외 처리- 장점: 우회 차단, 규칙 일관성 확보
- 단점: 검증 코드 소폭 증가
- 옵션 B (인터페이스 단순화):
updateAnnouncementPin에서festivalId인자를 제거하고announcementId로 축제를 유도- 장점: 호출 실수/오용 가능성 제거
- 단점: API 시그니처 변경 영향 존재
🔧 제안 수정안 (옵션 A)
@@
public AnnouncementPinUpdateResponse updateAnnouncementPin(
Actor actor,
Long festivalId,
Long announcementId,
AnnouncementPinUpdateRequest request
) {
Announcement announcement = getAnnouncementById(announcementId);
+ Long targetFestivalId = announcement.getFestival().getId();
+ if (!targetFestivalId.equals(festivalId)) {
+ throw new BadRequestException("요청 축제 정보가 공지의 축제와 일치하지 않습니다.");
+ }
if (announcement.isUnpinned() && request.pinned()) {
- validatePinnedLimit(festivalId);
+ validatePinnedLimit(targetFestivalId);
}
announcement.updatePinnedBy(actor, request.pinned());
return AnnouncementPinUpdateResponse.from(announcement);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/daedan/festabook/announcement/service/AnnouncementService.java`
around lines 125 - 129, The current validatePinnedLimit(Long festivalId) trusts
the passed festivalId and can be bypassed; change it to verify against the
announcement's actual festival id: in the flow that pins an announcement (e.g.,
updateAnnouncementPin), load the Announcement entity, get its festival id via
announcement.getFestival().getId(), compare it to the request festivalId and
throw BadRequestException on mismatch, then call
announcementJpaRepository.countByFestivalIdAndIsPinnedTrue(actualFestivalId) and
enforce MAX_PINNED_ANNOUNCEMENTS using that actualFestivalId (i.e., update
validatePinnedLimit to accept/validate the announcement's festival id or inline
the check in updateAnnouncementPin so countByFestivalIdAndIsPinnedTrue and
MAX_PINNED_ANNOUNCEMENTS operate on announcement.getFestival().getId()).
Comment on lines
+29
to
30
| if (subject == null || organizationId == null || organizerId == null) { | ||
| throw new ForbiddenException("토큰 필드에 문제가 있습니다."); |
There was a problem hiding this comment.
❓ Verification inconclusive
Script executed:
# 1) OrganizerDetailsExtractStrategy 전체 구현 확인
cat -n src/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetailsExtractStrategy.javaRepository: festabook/backend
Repository: festabook/backend
Exit code: 0
stdout:
1 package com.daedan.festabook.global.security.authorization;
2
3 import com.daedan.festabook.global.exception.ForbiddenException;
4 import com.daedan.festabook.global.security.role.RoleType;
5 import com.daedan.festabook.global.security.util.JwtProvider;
6 import io.jsonwebtoken.Claims;
7 import java.util.Set;
8 import lombok.RequiredArgsConstructor;
9 import org.springframework.stereotype.Component;
10
11 `@Component`
12 `@RequiredArgsConstructor`
13 public class OrganizerDetailsExtractStrategy implements AccountDetailsExtractStrategy {
14
15 private final JwtProvider jwtProvider;
16
17 `@Override`
18 public RoleType getSupportedRole() {
19 return RoleType.ROLE_ORGANIZER;
20 }
21
22 `@Override`
23 public AccountDetails extract(Claims claims, Set<RoleType> roleTypes) {
24 String subject = claims.getSubject();
25 Long organizationId = jwtProvider.extractOrganizationId(claims);
26 Long organizerId = jwtProvider.extractOrganizerId(claims);
27 Set<Long> festivalIds = jwtProvider.extractFestivalIds(claims);
28
29 if (subject == null || organizationId == null || organizerId == null) {
30 throw new ForbiddenException("토큰 필드에 문제가 있습니다.");
31 }
32
33 return new OrganizerDetails(
34 subject,
35 organizationId,
36 organizerId,
37 festivalIds,
38 roleTypes
39 );
40 }
41 }
Script executed:
# 2) 다른 추출 전략 구현체들 확인 (일관성 검증)
find . -name "*ExtractStrategy.java" -type f | head -20Repository: festabook/backend
Repository: festabook/backend
Exit code: 0
stdout:
./src/main/java/com/daedan/festabook/global/security/authorization/AccountDetailsExtractStrategy.java
./src/main/java/com/daedan/festabook/global/security/authorization/AdminDetailsExtractStrategy.java
./src/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetailsExtractStrategy.java
./src/main/java/com/daedan/festabook/global/security/authorization/PlaceAccessDetailsExtractStrategy.java
./src/main/java/com/daedan/festabook/global/security/authorization/StaffDetailsExtractStrategy.java
Script executed:
# 3) 전역 예외 핸들러 및 보안 엔트리포인트 확인
rg -A5 '@ExceptionHandler.*ForbiddenException|@RestControllerAdvice|@ControllerAdvice' --type=java -B2Repository: festabook/backend
Repository: festabook/backend
Exit code: 0
stdout:
src/main/java/com/daedan/festabook/global/exception/GlobalExceptionHandler.java-
src/main/java/com/daedan/festabook/global/exception/GlobalExceptionHandler.java-@Slf4j
src/main/java/com/daedan/festabook/global/exception/GlobalExceptionHandler.java:`@RestControllerAdvice`
src/main/java/com/daedan/festabook/global/exception/GlobalExceptionHandler.java-public class GlobalExceptionHandler {
src/main/java/com/daedan/festabook/global/exception/GlobalExceptionHandler.java-
src/main/java/com/daedan/festabook/global/exception/GlobalExceptionHandler.java- private static final String INTERNAL_ERROR_MESSAGE = "서버에 오류가 발생하였습니다. 관리자에게 문의해주세요.";
src/main/java/com/daedan/festabook/global/exception/GlobalExceptionHandler.java-
src/main/java/com/daedan/festabook/global/exception/GlobalExceptionHandler.java- `@ExceptionHandler`(DatabaseException.class)
Script executed:
# 4) JWT claim 누락 관련 로깅 흔적 확인
rg -n 'subject|organizerId|organizationId' src/main/java/com/daedan/festabook/global/security --type=java -B2 -A2 | head -100Repository: festabook/backend
Repository: festabook/backend
Exit code: 0
stdout:
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java-20-public class JwtProvider {
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java-21-
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java:22: private static final String CLAIM_ORGANIZATION_ID = "organizationId";
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java:23: private static final String CLAIM_ORGANIZER_ID = "organizerId";
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java-24- private static final String CLAIM_STAFF_ID = "staffId";
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java-25- private static final String CLAIM_PLACE_ID = "placeId";
--
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java-42- public String createOrganizerToken(
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java-43- String username,
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java:44: Long organizationId,
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java:45: Long organizerId,
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java-46- Set<RoleType> roles,
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java-47- Set<Long> festivalIds
--
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java-52-
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java-53- return Jwts.builder()
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java:54: .subject(username)
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java:55: .claim(CLAIM_ORGANIZATION_ID, organizationId)
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java-56- .claim(CLAIM_ROLES_TYPE, roles)
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java-57- .claim(CLAIM_FESTIVAL_IDS, normalizedFestivalIds)
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java:58: .claim(CLAIM_ORGANIZER_ID, organizerId)
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java-59- .issuedAt(now)
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java-60- .expiration(expiry)
--
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java-65- public String createStaffToken(
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java-66- String username,
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java:67: Long organizationId,
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java-68- Long staffId,
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java-69- Set<RoleType> roles,
--
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java-75-
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java-76- return Jwts.builder()
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java:77: .subject(username)
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java:78: .claim(CLAIM_ORGANIZATION_ID, organizationId)
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java-79- .claim(CLAIM_ROLES_TYPE, roles)
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java-80- .claim(CLAIM_FESTIVAL_IDS, normalizedFestivalIds)
--
src/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetailsExtractStrategy.java-22- `@Override`
src/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetailsExtractStrategy.java-23- public AccountDetails extract(Claims claims, Set<RoleType> roleTypes) {
src/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetailsExtractStrategy.java:24: String subject = claims.getSubject();
src/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetailsExtractStrategy.java:25: Long organizationId = jwtProvider.extractOrganizationId(claims);
src/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetailsExtractStrategy.java:26: Long organizerId = jwtProvider.extractOrganizerId(claims);
src/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetailsExtractStrategy.java-27- Set<Long> festivalIds = jwtProvider.extractFestivalIds(claims);
src/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetailsExtractStrategy.java-28-
src/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetailsExtractStrategy.java:29: if (subject == null || organizationId == null || organizerId == null) {
src/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetailsExtractStrategy.java-30- throw new ForbiddenException("토큰 필드에 문제가 있습니다.");
src/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetailsExtractStrategy.java-31- }
src/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetailsExtractStrategy.java-32-
src/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetailsExtractStrategy.java-33- return new OrganizerDetails(
src/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetailsExtractStrategy.java:34: subject,
src/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetailsExtractStrategy.java:35: organizationId,
src/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetailsExtractStrategy.java:36: organizerId,
src/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetailsExtractStrategy.java-37- festivalIds,
src/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetailsExtractStrategy.java-38- roleTypes
--
src/main/java/com/daedan/festabook/global/security/authorization/StaffDetails.java-8-
src/main/java/com/daedan/festabook/global/security/authorization/StaffDetails.java-9- private final String username;
src/main/java/com/daedan/festabook/global/security/authorization/StaffDetails.java:10: private final Long organizationId;
src/main/java/com/daedan/festabook/global/security/authorization/StaffDetails.java-11- private final Long staffId;
src/main/java/com/daedan/festabook/global/security/authorization/StaffDetails.java-12- private final Set<Long> festivalIds;
--
src/main/java/com/daedan/festabook/global/security/authorization/StaffDetails.java-14- public StaffDetails(
src/main/java/com/daedan/festabook/global/security/authorization/StaffDetails.java-15- String username,
src/main/java/com/daedan/festabook/global/security/authorization/StaffDetails.java:16: Long organizationId,
src/main/java/com/daedan/festabook/global/security/authorization/StaffDetails.java-17- Long staffId,
src/main/java/com/daedan/festabook/global/security/authorization/StaffDetails.java-18- Set<Long> festivalIds,
--
src/main/java/com/daedan/festabook/global/security/authorization/StaffDetails.java-21- super(roles);
src/main/java/com/daedan/festabook/global/security/authorization/StaffDetails.java-22- this.username = username;
src/main/java/com/daedan/festabook/global/security/authorization/StaffDetails.java:23: this.organizationId = organizationId;
src/main/java/com/daedan/festabook/global/security/authorization/StaffDetails.java-24- this.staffId = staffId;
src/main/java/com/daedan/festabook/global/security/authorization/StaffDetails.java-25- this.festivalIds = Objects.requireNonNullElse(festivalIds, Set.of());
--
src/main/java/com/daedan/festabook/global/security/authorization/StaffDetails.java-32-
src/main/java/com/daedan/festabook/global/security/authorization/StaffDetails.java-33- `@Override`
src/main/java/com/daedan/festabook/global/security/authorization/StaffDetails.java:34: public boolean hasOrganizationId(Long organizationId) {
src/main/java/com/daedan/festabook/global/security/authorization/StaffDetails.java:35: return organizationId != null && Objects.equals(this.organizationId, organizationId);
src/main/java/com/daedan/festabook/global/security/authorization/StaffDetails.java-36- }
src/main/java/com/daedan/festabook/global/security/authorization/StaffDetails.java-37-
--
src/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetails.java-8-
src/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetails.java-9- private final String username;
src/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetails.java:10: private final Long organizationId;
src/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetails.java:11: private final Long organizerId;
src/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetails.java-12- private final Set<Long> festivalIds;
src/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetails.java-13-
src/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetails.java-14- public OrganizerDetails(
src/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetails.java-15- String username,
src/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetails.java:16: Long organizationId,
src/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetails.java:17: Long organizerId,
src/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetails.java-18- Set<Long> festivalIds,
src/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetails.java-19- Set<RoleType> roles
--
src/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetails.java-21- super(roles);
src/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetails.java-22- this.username = username;
src/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetails.java:23: this.organizationId = organizationId;
src/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetails.java:24: this.organizerId = organizerId;
src/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetails.java-25- this.festivalIds = Objects.requireNonNullElse(festivalIds, Set.of());
src/main/java/com/daedan/festabook/global/security/authorization/OrganizerDetails.java-26- }
--
Script executed:
# 5) 토큰 생성 부분에서 claim 설정 확인
rg -n 'claim|subject|organizerId|organizationId' src/main/java/com/daedan/festabook --type=java | grep -E '(claim|withClaim|claim\(|setClaim)' | head -20Repository: festabook/backend
Repository: festabook/backend
Exit code: 0
stdout:
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java:55: .claim(CLAIM_ORGANIZATION_ID, organizationId)
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java:56: .claim(CLAIM_ROLES_TYPE, roles)
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java:57: .claim(CLAIM_FESTIVAL_IDS, normalizedFestivalIds)
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java:58: .claim(CLAIM_ORGANIZER_ID, organizerId)
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java:78: .claim(CLAIM_ORGANIZATION_ID, organizationId)
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java:79: .claim(CLAIM_ROLES_TYPE, roles)
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java:80: .claim(CLAIM_FESTIVAL_IDS, normalizedFestivalIds)
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java:81: .claim(CLAIM_STAFF_ID, staffId)
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java:93: .claim(CLAIM_PLACE_ID, placeId)
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java:94: .claim(CLAIM_ROLES_TYPE, roles)
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java:105: public Set<RoleType> extractRoles(Claims claims) {
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java:107: claims.get(CLAIM_ROLES_TYPE, List.class),
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java:118: public Long extractOrganizationId(Claims claims) {
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java:119: return claims.get(CLAIM_ORGANIZATION_ID, Long.class);
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java:122: public Set<Long> extractFestivalIds(Claims claims) {
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java:123: List<?> festivalIds = claims.get(CLAIM_FESTIVAL_IDS, List.class);
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java:133: public Long extractOrganizerId(Claims claims) {
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java:134: return claims.get(CLAIM_ORGANIZER_ID, Long.class);
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java:137: public Long extractStaffId(Claims claims) {
src/main/java/com/daedan/festabook/global/security/util/JwtProvider.java:138: return claims.get(CLAIM_STAFF_ID, Long.class);
Script executed:
# 1) GlobalExceptionHandler 전체 확인
cat -n src/main/java/com/daedan/festabook/global/exception/GlobalExceptionHandler.java | head -100Repository: festabook/backend
Repository: festabook/backend
Exit code: 0
stdout:
1 package com.daedan.festabook.global.exception;
2
3 import static net.logstash.logback.argument.StructuredArguments.kv;
4
5 import com.daedan.festabook.global.logging.dto.ExceptionLog;
6 import java.io.IOException;
7 import java.io.PrintWriter;
8 import java.io.StringWriter;
9 import lombok.extern.slf4j.Slf4j;
10 import org.springframework.dao.DataAccessException;
11 import org.springframework.dao.DataAccessResourceFailureException;
12 import org.springframework.dao.DataIntegrityViolationException;
13 import org.springframework.data.jpa.repository.query.BadJpqlGrammarException;
14 import org.springframework.http.HttpStatus;
15 import org.springframework.http.ResponseEntity;
16 import org.springframework.jdbc.BadSqlGrammarException;
17 import org.springframework.security.access.AccessDeniedException;
18 import org.springframework.security.authentication.AuthenticationCredentialsNotFoundException;
19 import org.springframework.web.bind.annotation.ExceptionHandler;
20 import org.springframework.web.bind.annotation.RestControllerAdvice;
21
22 `@Slf4j`
23 `@RestControllerAdvice`
24 public class GlobalExceptionHandler {
25
26 private static final String INTERNAL_ERROR_MESSAGE = "서버에 오류가 발생하였습니다. 관리자에게 문의해주세요.";
27
28 `@ExceptionHandler`(DatabaseException.class)
29 public ResponseEntity<ExceptionResponse> handleDatabaseException(DatabaseException databaseException) {
30 try {
31 return ResponseEntity
32 .status(databaseException.getStatus())
33 .body(databaseException.toResponse());
34 } finally {
35 ExceptionLog exceptionLog = ExceptionLog.from(
36 databaseException.getStatus().value(),
37 databaseException.getMessage(),
38 databaseException.getClass().getSimpleName(),
39 databaseException.getOriginalExceptionMessage()
40 );
41
42 logDatabaseExceptionAsErrorIf5xx(databaseException, exceptionLog);
43 }
44 }
45
46 `@ExceptionHandler`(BusinessException.class)
47 public ResponseEntity<ExceptionResponse> handleBusinessException(BusinessException businessException) {
48 try {
49 return ResponseEntity
50 .status(businessException.getStatus())
51 .body(businessException.toResponse());
52 } finally {
53 ExceptionLog exceptionLog = ExceptionLog.from(
54 businessException.getStatus().value(),
55 businessException.getMessage(),
56 businessException.getClass().getSimpleName(),
57 ""
58 );
59 log.info("", kv("event", exceptionLog));
60 }
61 }
62
63 `@ExceptionHandler`(AuthenticationCredentialsNotFoundException.class)
64 public ResponseEntity<ExceptionResponse> handleAuthenticationCredentialsNotFoundException(
65 AuthenticationCredentialsNotFoundException exception
66 ) {
67 try {
68 return ResponseEntity
69 .status(HttpStatus.UNAUTHORIZED.value())
70 .body(new ExceptionResponse("인증되지 않은 사용자입니다."));
71 } finally {
72 ExceptionLog exceptionLog = ExceptionLog.from(
73 HttpStatus.UNAUTHORIZED.value(),
74 exception.getMessage(),
75 exception.getClass().getSimpleName(),
76 ""
77 );
78 log.info("", kv("event", exceptionLog));
79 }
80 }
81
82 `@ExceptionHandler`(AccessDeniedException.class)
83 public ResponseEntity<ExceptionResponse> handleAccessDeniedException(AccessDeniedException exception) {
84 try {
85 return ResponseEntity
86 .status(HttpStatus.FORBIDDEN.value())
87 .body(new ExceptionResponse("접근 권한이 없습니다."));
88 } finally {
89 ExceptionLog exceptionLog = ExceptionLog.from(
90 HttpStatus.FORBIDDEN.value(),
91 exception.getMessage(),
92 exception.getClass().getSimpleName(),
93 ""
94 );
95 log.info("", kv("event", exceptionLog));
96 }
97 }
98
99 `@ExceptionHandler`({
100 DataIntegrityViolationException.class,
Script executed:
# 2) PlaceAccessDetailsExtractStrategy 확인 (PR 93 관련)
cat -n src/main/java/com/daedan/festabook/global/security/authorization/PlaceAccessDetailsExtractStrategy.javaRepository: festabook/backend
Repository: festabook/backend
Exit code: 0
stdout:
1 package com.daedan.festabook.global.security.authorization;
2
3 import com.daedan.festabook.global.exception.ForbiddenException;
4 import com.daedan.festabook.global.security.role.RoleType;
5 import com.daedan.festabook.global.security.util.JwtProvider;
6 import io.jsonwebtoken.Claims;
7 import java.util.Set;
8 import lombok.RequiredArgsConstructor;
9 import org.springframework.stereotype.Component;
10
11 `@Component`
12 `@RequiredArgsConstructor`
13 public class PlaceAccessDetailsExtractStrategy implements AccountDetailsExtractStrategy {
14
15 private final JwtProvider jwtProvider;
16
17 `@Override`
18 public RoleType getSupportedRole() {
19 return RoleType.ROLE_PLACE_ACCESS;
20 }
21
22 `@Override`
23 public AccountDetails extract(Claims claims, Set<RoleType> roleTypes) {
24 Long placeId = jwtProvider.extractPlaceId(claims);
25
26 if (placeId == null) {
27 throw new ForbiddenException("토큰 필드에 문제가 있습니다.");
28 }
29
30 return new PlaceAccessDetails(placeId, roleTypes);
31 }
32 }
Script executed:
# 3) StaffDetailsExtractStrategy 확인
cat -n src/main/java/com/daedan/festabook/global/security/authorization/StaffDetailsExtractStrategy.javaRepository: festabook/backend
Repository: festabook/backend
Exit code: 0
stdout:
1 package com.daedan.festabook.global.security.authorization;
2
3 import com.daedan.festabook.global.exception.ForbiddenException;
4 import com.daedan.festabook.global.security.role.RoleType;
5 import com.daedan.festabook.global.security.util.JwtProvider;
6 import io.jsonwebtoken.Claims;
7 import java.util.Set;
8 import lombok.RequiredArgsConstructor;
9 import org.springframework.stereotype.Component;
10
11 `@Component`
12 `@RequiredArgsConstructor`
13 public class StaffDetailsExtractStrategy implements AccountDetailsExtractStrategy {
14
15 private final JwtProvider jwtProvider;
16
17 `@Override`
18 public RoleType getSupportedRole() {
19 return RoleType.ROLE_STAFF;
20 }
21
22 `@Override`
23 public AccountDetails extract(Claims claims, Set<RoleType> roleTypes) {
24 String subject = claims.getSubject();
25 Long organizationId = jwtProvider.extractOrganizationId(claims);
26 Long staffId = jwtProvider.extractStaffId(claims);
27 Set<Long> festivalIds = jwtProvider.extractFestivalIds(claims);
28
29 if (subject == null || organizationId == null || staffId == null) {
30 throw new ForbiddenException("토큰 필드에 문제가 있습니다.");
31 }
32
33 return new StaffDetails(
34 subject,
35 organizationId,
36 staffId,
37 festivalIds,
38 roleTypes
39 );
40 }
41 }
Script executed:
# 4) AdminDetailsExtractStrategy 확인
cat -n src/main/java/com/daedan/festabook/global/security/authorization/AdminDetailsExtractStrategy.javaRepository: festabook/backend
Repository: festabook/backend
Exit code: 0
stdout:
1 package com.daedan.festabook.global.security.authorization;
2
3 import com.daedan.festabook.global.security.role.RoleType;
4 import io.jsonwebtoken.Claims;
5 import java.util.Set;
6 import org.springframework.stereotype.Component;
7
8 `@Component`
9 public class AdminDetailsExtractStrategy implements AccountDetailsExtractStrategy {
10
11 `@Override`
12 public RoleType getSupportedRole() {
13 return RoleType.ROLE_ADMIN;
14 }
15
16 `@Override`
17 public AccountDetails extract(Claims claims, Set<RoleType> roleTypes) {
18 return new AdminDetails(roleTypes);
19 }
20 }
Script executed:
# 5) 로깅 및 모니터링 현황 확인
rg -n 'logger|log\.|Logger|monitoring|metric|alert' src/main/java/com/daedan/festabook/global/security --type=java | head -30Repository: festabook/backend
Repository: festabook/backend
Exit code: 0
stdout:
src/main/java/com/daedan/festabook/global/security/handler/CustomAuthenticationEntryPoint.java:47: log.info("", kv("event", securityLog));
src/main/java/com/daedan/festabook/global/security/handler/CustomAccessDeniedHandler.java:47: log.info("", kv("event", securityLog));
src/main/java/com/daedan/festabook/global/security/filter/JwtAuthenticationFilter.java:46: log.warn("", kv("event", securityLog));
Script executed:
# ForbiddenException 정의 및 상속 확인
cat -n src/main/java/com/daedan/festabook/global/exception/ForbiddenException.javaRepository: festabook/backend
Repository: festabook/backend
Exit code: 0
stdout:
1 package com.daedan.festabook.global.exception;
2
3 import lombok.Getter;
4 import org.springframework.http.HttpStatus;
5
6 `@Getter`
7 public class ForbiddenException extends BusinessException {
8
9 private static final String message = "접근 권한이 없습니다.";
10
11 public ForbiddenException() {
12 super(message, HttpStatus.FORBIDDEN);
13 }
14
15 public ForbiddenException(String message) {
16 super(message, HttpStatus.FORBIDDEN);
17 }
18 }
Script executed:
# BusinessException 정의 확인
cat -n src/main/java/com/daedan/festabook/global/exception/BusinessException.javaRepository: festabook/backend
Repository: festabook/backend
Exit code: 0
stdout:
1 package com.daedan.festabook.global.exception;
2
3 import lombok.Getter;
4 import org.springframework.http.HttpStatus;
5
6 `@Getter`
7 public class BusinessException extends RuntimeException {
8
9 private final HttpStatus status;
10
11 public BusinessException(String message, HttpStatus status) {
12 super(message);
13 this.status = status;
14 }
15
16 public ExceptionResponse toResponse() {
17 return new ExceptionResponse(getMessage());
18 }
19 }
필수 JWT claim 누락 시 운영 가시성이 부족합니다.
현재 방식은 외부적으로 접근 차단(403)은 되지만, subject/organizationId/organizerId 누락이 토큰 발급 버그인지 판단할 신호(ERROR 로그/모니터링)가 남지 않습니다. 토큰 생성 시 이 필드들이 필수이므로, null 값은 서버 쪽 문제의 조기 신호입니다.
옵션 A (권장): ForbiddenException은 유지하고 claim 누락 시 ERROR 로그 + 메트릭(알람) 추가
- 장점: 기존 응답 정책(403)은 유지하면서 장애 탐지력 강화, 팀 컨벤션 일관성 유지
- 단점: 예외 처리 코드가 조금 증가
옵션 B: 모든 extract strategy에 공통 검증 로직 추출 (중복 제거)
- 장점: 유지보수성 향상, null 체크 로직 한 곳에서 관리
- 단점: 추가 추상화 필요
현재 PlaceAccessDetailsExtractStrategy, StaffDetailsExtractStrategy에서도 동일한 패턴이므로, 한 곳에서 개선하면 나머지도 함께 해결할 수 있습니다.
🤖 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/authorization/OrganizerDetailsExtractStrategy.java`
around lines 29 - 30, OrganizerDetailsExtractStrategy currently throws
ForbiddenException when subject/organizationId/organizerId are null but provides
no ERROR log or metric; update OrganizerDetailsExtractStrategy to log an ERROR
(including which claim(s) are missing and the token context) and emit a
monitoring metric/trace before throwing ForbiddenException to preserve the 403
response while surfacing server-side failures; additionally refactor the
repeated null-check logic used in PlaceAccessDetailsExtractStrategy and
StaffDetailsExtractStrategy into a shared validator/helper (e.g., a new
JwtClaimValidator utility) that returns details of missing claims so all three
strategies can log, emit metrics, and then throw ForbiddenException
consistently.
Comment on lines
+26
to
27
| if (placeId == null) { | ||
| throw new ForbiddenException("토큰 필드에 문제가 있습니다."); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1) ForbiddenException과 다른 인증 관련 예외 정의 확인
echo "=== 1. Exception classes and their status codes ==="
rg -A 3 "class.*Exception" src/main/java/com/daedan/festabook/global/exception --type java
# 2) PlaceAccessDetailsExtractStrategy 전체 파일 확인
echo "=== 2. PlaceAccessDetailsExtractStrategy full content ==="
cat -n src/main/java/com/daedan/festabook/global/security/authorization/PlaceAccessDetailsExtractStrategy.java
# 3) JWT claim 추출 관련 전략들 확인
echo "=== 3. Other JWT extraction strategies ==="
fd ".*DetailsExtractStrategy\|.*AccessAccountDetailsStrategy" src/main/java --type f
# 4) 예외 핸들러 확인 (401/403 매핑)
echo "=== 4. Exception handlers and their mappings ==="
rg -B 2 -A 5 "ExceptionHandler|ControllerAdvice|ResponseStatus" src/main/java/com/daedan/festabook/global --type java | head -100Repository: festabook/backend
Length of output: 15630
🏁 Script executed:
#!/bin/bash
# 1) BusinessException과 ForbiddenException 전체 코드 확인
echo "=== 1. BusinessException definition ==="
cat -n src/main/java/com/daedan/festabook/global/exception/BusinessException.java
echo -e "\n=== 2. ForbiddenException definition ==="
cat -n src/main/java/com/daedan/festabook/global/exception/ForbiddenException.java
# 2) UnauthorizedException 전체 코드 확인
echo -e "\n=== 3. UnauthorizedException definition ==="
cat -n src/main/java/com/daedan/festabook/global/exception/UnauthorizedException.java
# 3) JWT 관련 다른 추출 전략 검색
echo -e "\n=== 4. All details extraction strategies ==="
find src/main/java -name "*DetailsExtractStrategy*" -o -name "*AccountDetailsStrategy*" | xargs cat -n
# 4) GlobalExceptionHandler의 BusinessException 핸들러 상세 확인
echo -e "\n=== 5. GlobalExceptionHandler BusinessException handler ==="
sed -n '/handleBusinessException/,/^ `@ExceptionHandler/p`' src/main/java/com/daedan/festabook/global/exception/GlobalExceptionHandler.java | head -20Repository: festabook/backend
Length of output: 8351
JWT claim 누락은 403보다 401(인증 실패)로 분리해야 합니다. 이는 세 개의 추출 전략 모두에 적용됩니다.
현재 PlaceAccessDetailsExtractStrategy, OrganizerDetailsExtractStrategy, StaffDetailsExtractStrategy가 일관되게 JWT claim 누락을 ForbiddenException(403)으로 처리하고 있습니다. 하지만 서명 검증은 통과했지만 필수 claim이 누락된 경우는 서버의 토큰 생성 버그를 의미하므로, 인가 실패(403)와는 의미가 다릅니다.
-
옵션 A(권장): 401로 매핑되는
UnauthorizedException(UnauthorizedType.???)사용 + ERROR 로그/알람 추가- 장점: 인증 경계가 명확하고, 클라이언트가 재인증을 정확히 수행, 운영 탐지 향상
- 단점: 3개 전략 모두 수정 필요, UnauthorizedType 새 타입 추가 가능
-
옵션 B: 403 유지 + 내부 모니터링 알람만 추가
- 장점: 변경 폭이 작음
- 단점: 클라이언트 재인증 플로우 정확도 저하, 인증/인가 경계 모호
🤖 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/authorization/PlaceAccessDetailsExtractStrategy.java`
around lines 26 - 27, The three extract strategies currently throw
ForbiddenException when required JWT claims are missing; change this to throw an
UnauthorizedException (mapped to HTTP 401) to reflect authentication failure and
add an ERROR-level log/monitoring alarm when a claim is absent; update
PlaceAccessDetailsExtractStrategy (replace throw new ForbiddenException(...)
when placeId==null), OrganizerDetailsExtractStrategy, and
StaffDetailsExtractStrategy similarly, and if needed add a new UnauthorizedType
enum value to classify “missing_jwt_claim” so the UnauthorizedException can
carry that type for metrics/alerts.
Comment on lines
+29
to
30
| if (subject == null || organizationId == null || staffId == null) { | ||
| throw new ForbiddenException("토큰 필드에 문제가 있습니다."); |
There was a problem hiding this comment.
JWT 필수 클레임 누락 시 ForbiddenException만 던지고 종료되어 원인 추적 신호가 없습니다.
Line 29~30은 인증 실패 처리 자체는 맞지만, 이 케이스는 사용자 오입력보다 토큰 발급/서버 결함 신호일 가능성이 커서 내부 ERROR 로그(및 알림 연계)가 필요합니다.
- 옵션 A (변경 범위 최소): 이 전략 내부에서 누락 클레임명을
ERROR로 로깅 후 현재처럼 403 반환- 장점: 빠르게 반영 가능, 운영 가시성 즉시 확보
- 단점: 다른 ExtractStrategy에도 유사 코드가 중복될 수 있음
- 옵션 B (일관성 우선):
JwtProvider또는 공통 인증 계층에서 필수 클레임 검증/로깅을 중앙화하고 여기서는 결과만 사용- 장점: 정책 일관성/중복 제거
- 단점: 리팩토링 범위가 커짐
예시 패치 (옵션 A)
+import lombok.extern.slf4j.Slf4j;
...
`@Component`
`@RequiredArgsConstructor`
+@Slf4j
public class StaffDetailsExtractStrategy implements AccountDetailsExtractStrategy {
...
- if (subject == null || organizationId == null || staffId == null) {
- throw new ForbiddenException("토큰 필드에 문제가 있습니다.");
+ if (subject == null || organizationId == null || staffId == null) {
+ log.error(
+ "Invalid staff JWT claims. subjectPresent={}, organizationIdPresent={}, staffIdPresent={}",
+ subject != null,
+ organizationId != null,
+ staffId != null
+ );
+ throw new ForbiddenException("접근 권한이 없습니다.");
}Based on learnings: JWT 필수 클레임 누락은 인증 실패로 처리하되 내부적으로 ERROR 로그/모니터링을 남겨야 하며, 이 프로젝트의 인증 실패 외부 응답은 HttpStatus.FORBIDDEN + "접근 권한이 없습니다." 컨벤션을 따릅니다.
🤖 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/authorization/StaffDetailsExtractStrategy.java`
around lines 29 - 30, The current null-check in StaffDetailsExtractStrategy that
throws new ForbiddenException("토큰 필드에 문제가 있습니다.") should also emit an ERROR log
with the missing claim names before throwing to aid ops; update the check in
StaffDetailsExtractStrategy (where subject, organizationId, staffId are
validated) to determine which claims are null, call the class logger at ERROR
with a clear message including the missing claim identifiers (e.g., "Missing JWT
claims: subject, organizationId"), then throw the existing ForbiddenException so
the external response remains HttpStatus.FORBIDDEN/"접근 권한이 없습니다."; keep the
change local (option A) to minimize scope.
Comment on lines
+143
to
147
| @PreAuthorize("hasAnyRole('STAFF', 'ADMIN')") | ||
| @PatchMapping("/{staffId}/password") | ||
| @ResponseStatus(HttpStatus.OK) | ||
| @Operation(summary = "🟨 축제 관리자 권한 변경") | ||
| @Operation(summary = "🟪 축제 관리자 비밀번호 변경") | ||
| @ApiResponses(value = { |
There was a problem hiding this comment.
권한 이모지와 실제 접근 Role 범위를 맞춰 주세요.
Line 143-147에서 @Operation(summary = "🟪 ...")는 STAFF/ORGANIZER/ADMIN 범위를 암시하는데, 실제 @PreAuthorize는 STAFF/ADMIN만 허용하고 있어 API 문서 해석이 혼동될 수 있습니다.
- 옵션 A: 의도가 “STAFF 이상”이면
ORGANIZER를hasAnyRole에 포함- 장점: 문서/정책 일치, 호출자 혼란 감소
- 단점: 실제 권한 범위가 넓어짐
- 옵션 B: 의도가 STAFF/ADMIN 한정이면 summary 권한 표기를 그에 맞게 조정
- 장점: 정책 유지, 문서 정확도 개선
- 단점: 팀 이모지 체계와의 추가 합의 필요
As per coding guidelines: "Operation summary 권한 접두사 이모지: 🟪 STAFF/ORGANIZER/ADMIN"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/daedan/festabook/staff/controller/StaffController.java`
around lines 143 - 147, The `@Operation` summary emoji in StaffController (method
with `@PatchMapping`("/{staffId}/password")) implies STAFF/ORGANIZER/ADMIN but the
`@PreAuthorize` on that handler currently allows only STAFF and ADMIN; fix by
making them consistent: either add ORGANIZER to the
`@PreAuthorize`(hasAnyRole(...)) expression to grant ORGANIZER the same access, or
change the `@Operation`(summary=...) emoji/text to reflect only STAFF/ADMIN;
update the annotation(s) on the method (the `@PreAuthorize` or the `@Operation`
summary) accordingly so documentation and enforcement match.
Comment on lines
+210
to
+227
| @Test | ||
| void 예외_iOS_알림_전송_실패도_도메인_예외로_변환() throws FirebaseMessagingException { | ||
| // given | ||
| Long festivalId = 1L; | ||
| NotificationSendRequest request = NotificationSendRequest.builder() | ||
| .title("title") | ||
| .body("body") | ||
| .putData("announcementId", "1") | ||
| .build(); | ||
| given(firebaseMessaging.send(any())) | ||
| .willReturn("android-ok") | ||
| .willThrow(FirebaseMessagingException.class); | ||
|
|
||
| // when & then | ||
| assertThatThrownBy(() -> fcmNotificationManager.sendToFestivalTopic(festivalId, request)) | ||
| .isInstanceOf(InternalServerException.class) | ||
| .hasMessage("FCM 메시지 전송을 실패했습니다."); | ||
| } |
There was a problem hiding this comment.
iOS 실패 테스트가 ‘iOS 토픽 실패’ 자체를 직접 증명하지 못합니다.
지금은 “두 번째 send 호출이 실패한다”를 검증하므로, 호출 순서가 바뀌어도 테스트가 통과할 수 있습니다.
- 옵션 A(정확성 우선):
Answer에서Message.topic을 보고 iOS 토픽일 때만 예외 발생 → 테스트 의도와 1:1 매핑 - 옵션 B(단순성 우선): 현재 순차 stubbing 유지 + 테스트명을 “두 번째 전송 실패”로 변경 → 구현 의존적이지만 코드 단순
옵션 A 예시 diff
- given(firebaseMessaging.send(any()))
- .willReturn("android-ok")
- .willThrow(FirebaseMessagingException.class);
+ given(firebaseMessaging.send(any()))
+ .willAnswer(invocation -> {
+ Message message = invocation.getArgument(0);
+ String topic = (String) ReflectionTestUtils.getField(message, "topic");
+ if ((TOPIC_PREFIX + festivalId + "-ios").equals(topic)) {
+ throw new FirebaseMessagingException("send-failed", "ios send failed");
+ }
+ return "ok";
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/com/daedan/festabook/notification/infrastructure/FcmNotificationManagerTest.java`
around lines 210 - 227, The test currently stubs firebaseMessaging.send
sequentially which doesn't guarantee the exception is tied to the iOS topic;
change the stub to inspect the Message argument and throw only when the message
topic matches the iOS topic so the test proves iOS-specific failure: replace the
chained willReturn/willThrow on firebaseMessaging.send(...) with a willAnswer
that reads the invocation argument (Message msg), checks msg.getTopic() (the
topic name used for iOS in sendToFestivalTopic / how NotificationSendRequest is
mapped) and throws FirebaseMessagingException for that topic while returning a
success id for other topics; keep the assertion on
fcmNotificationManager.sendToFestivalTopic(...) and adjust the thrown exception
instance/message if needed.
Comment on lines
+56
to
+76
| try (ExecutorService executorService = Executors.newFixedThreadPool(threadCount)) { | ||
| CountDownLatch startLatch = new CountDownLatch(1); | ||
| CountDownLatch endLatch = new CountDownLatch(threadCount); | ||
|
|
||
| for (int i = 0; i < threadCount; i++) { | ||
| executorService.submit(() -> { | ||
| try { | ||
| startLatch.await(); | ||
| staffService.createStaff(actor, organization.getId(), request); | ||
| successCount.incrementAndGet(); | ||
| } catch (ConflictException e) { | ||
| conflictCount.incrementAndGet(); | ||
| } catch (Exception ignore) { | ||
| } finally { | ||
| endLatch.countDown(); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| startLatch.countDown(); | ||
| endLatch.await(); |
There was a problem hiding this comment.
동시성 테스트는 공용 헬퍼 사용으로 일관성을 맞추는 편이 안전합니다.
Line 56~76의 수동 스레드 제어는 동작은 가능하지만, 프로젝트 표준(concurrencyTestHelper 활용)과 달라 향후 유지보수 시 테스트 패턴이 분산됩니다.
옵션 A(권장): concurrencyTestHelper로 실행/집계 로직 통일.
옵션 B: 지금 구조를 유지하려면 헬퍼에 동일 패턴을 흡수해 중복 구현을 제거.
As per coding guidelines: 동시성 테스트 클래스는 XXXConcurrencyTest로 명명한다. AcceptanceTestSupport를 상속받으며 concurrencyTestHelper를 활용한다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/com/daedan/festabook/staff/concurrency/StaffConcurrencyTest.java`
around lines 56 - 76, The concurrency test in StaffConcurrencyTest uses manual
ExecutorService/CountDownLatch logic (executorService, startLatch, endLatch)
instead of the project-standard concurrencyTestHelper; refactor the test to call
the shared concurrencyTestHelper to run concurrent invocations of
staffService.createStaff(actor, organization.getId(), request) and aggregate
success/conflict counts, or move the current latch/submit pattern into
concurrencyTestHelper and invoke that helper from StaffConcurrencyTest so the
test class uses the standard helper and matches naming/Inheritance conventions
(ensure class still extends AcceptanceTestSupport).
Comment on lines
+68
to
+69
| } catch (Exception ignore) { | ||
| } finally { |
There was a problem hiding this comment.
예외/인터럽트 무시는 동시성 테스트 신뢰도를 크게 떨어뜨립니다.
Line 6869, 7778에서 예외를 무시하면 실패 원인이 사라져 “실패해야 할 상황이 통과”로 보일 수 있습니다.
옵션 A(권장): 예상 외 예외를 수집해 마지막에 assertThat(unexpectedErrors).isEmpty()로 실패 처리.
옵션 B: 최소한 인터럽트는 Thread.currentThread().interrupt()로 복구하고 AssertionError로 즉시 실패 처리.
🔧 최소 보강 예시 (인터럽트 처리)
- } catch (InterruptedException ignore) {
- }
+ } catch (InterruptedException exception) {
+ Thread.currentThread().interrupt();
+ throw new AssertionError("동시성 테스트가 인터럽트되었습니다.", exception);
+ }Also applies to: 77-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/com/daedan/festabook/staff/concurrency/StaffConcurrencyTest.java`
around lines 68 - 69, In StaffConcurrencyTest replace empty exception catches
with logic that records unexpected exceptions and properly handles interrupts:
create a thread-safe List<Throwable> unexpectedErrors (or use
ConcurrentLinkedQueue) and in each catch(Exception e) add e to unexpectedErrors;
for InterruptedException specifically call Thread.currentThread().interrupt()
and add the exception, and for other exceptions add them and/or rethrow an
AssertionError to fail fast; after the concurrent work completes
assertThat(unexpectedErrors).isEmpty() to surface failures instead of silently
swallowing them.
Comment on lines
+87
to
108
| @Test | ||
| void 예외_존재하지_않는_조직() { | ||
| // given | ||
| Actor adminActor = ActorFixture.admin(); | ||
|
|
||
| Long organizationId = 10L; | ||
| String username = "staff"; | ||
| String password = "1234"; | ||
|
|
||
| given(organizationJpaRepository.findById(organizationId)) | ||
| .willReturn(Optional.empty()); | ||
|
|
||
| StaffRequest request = StaffRequestFixture.create(username, password); | ||
|
|
||
| // when & then | ||
| assertThatThrownBy(() -> staffService.createStaff(adminActor, organizationId, request)) | ||
| .isInstanceOf(NotFoundException.class) | ||
| .hasMessage("존재하지 않는 Organization 입니다."); | ||
| } | ||
|
|
||
| @Test | ||
| void 성공() { |
There was a problem hiding this comment.
createStaff Nested의 첫 테스트를 성공()으로 맞추는 편이 좋겠습니다.
Line 87에서 예외 케이스가 먼저 와서, 팀 공통 읽기 순서(정상 흐름 → 예외 흐름)와 어긋납니다. 기능 영향은 없지만 리뷰/탐색 비용이 커집니다.
- 옵션 A: 현재 테스트 순서만 재배치
- 장점: 변경량 최소, 컨벤션 즉시 정렬
- 단점: 구조적 의미 개선은 제한적
- 옵션 B:
성공/입력검증을 하위 Nested로 분리- 장점: 의도 구분이 더 선명
- 단점: 클래스 깊이가 1단계 늘어남
As per coding guidelines: "도메인 권한/입력 검증 테스트: createBy, updateXxxBy, deleteBy Nested 클래스의 첫 번째 테스트는 반드시 '성공()'으로 작성."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/java/com/daedan/festabook/staff/service/StaffServiceTest.java`
around lines 87 - 108, Reorder the tests inside the createStaff Nested in
StaffServiceTest so the happy-path test method 성공() is the first test: locate
the nested block containing the tests 예외_존재하지_않는_조직() and 성공() and move the 성공()
test before 예외_존재하지_않는_조직(); alternatively, if you prefer clearer separation,
split happy-path and validation/error cases into separate inner Nested classes
(e.g., Success and Validation/Exception) so createStaff's first test is always
성공() per the guideline.
Comment on lines
+59
to
+64
| StorageUploadResponse response = mockStorageManager.uploadFile(request); | ||
|
|
||
| // then | ||
| assertSoftly(s -> { | ||
| s.assertThat(response.accessRelativePath()).isEqualTo(storagePath); | ||
| s.assertThat(response.storagePath()).isEqualTo(storagePath); |
There was a problem hiding this comment.
테스트 결과/식별자 네이밍을 가이드에 맞추면 일관성이 더 좋아집니다.
Line 59의 response는 테스트 결과 변수 규칙(result)과 다르고, Line 62의 s는 축약 식별자라 팀 규칙과 어긋납니다.
선택지는 아래처럼 보시면 됩니다.
- 옵션 A:
response만result로 변경 (변경 범위 최소, 빠른 정합성 확보) - 옵션 B:
response+ 람다 파라미터(s)까지 함께 정리 (가독성/팀 컨벤션 일관성 극대화)
제안 diff (옵션 B)
- StorageUploadResponse response = mockStorageManager.uploadFile(request);
+ StorageUploadResponse result = mockStorageManager.uploadFile(request);
// then
- assertSoftly(s -> {
- s.assertThat(response.accessRelativePath()).isEqualTo(storagePath);
- s.assertThat(response.storagePath()).isEqualTo(storagePath);
+ assertSoftly(assertions -> {
+ assertions.assertThat(result.accessRelativePath()).isEqualTo(storagePath);
+ assertions.assertThat(result.storagePath()).isEqualTo(storagePath);
});As per coding guidelines, "테스트 실행 결과 값 변수명: result." 및 "**/*.{java,ts,tsx,js}: 모든 식별자에서 줄임말을 사용하지 않는다."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/com/daedan/festabook/storage/infrastructure/MockStorageManagerTest.java`
around lines 59 - 64, Rename the test result variable response to result and
replace the abbreviated lambda parameter s with a descriptive name (e.g.,
softAssertions or assertions) in the MockStorageManagerTest.uploadFile assertion
block so it follows the test naming rule and avoids abbreviations; specifically
update the variable returned by mockStorageManager.uploadFile(request)
(currently StorageUploadResponse response) to StorageUploadResponse result and
change the assertSoftly lambda parameter (currently s) to a full identifier,
then update all uses (result.accessRelativePath(), result.storagePath(), and the
lambda body) accordingly.
![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
이번 릴리스를 위한 대규모 병합 PR이네요. 코드 스타일 가이드에 맞춰 전반적인 코드를 개선하고, 동시성 제어를 위한 분산락을 도입하는 등 시스템 안정성을 크게 향상시키는 중요한 변경사항들이 많이 포함되어 있습니다. 특히, 새로운 코드 컨벤션을 code-style.md에 명확히 정의하고 이를 코드베이스 전반에 일관되게 적용하려는 노력이 돋보입니다. 전체적으로 훌륭한 작업이라고 생각합니다. 한 가지 잠재적인 NullPointerException 발생 가능성에 대한 리뷰 의견을 남겼으니 확인 부탁드립니다.
Note: Security Review did not run due to the size of the PR.
changuii
approved these changes
Mar 6, 2026
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
2 participants
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.
refactor: null 검증 예외명 통일
refactor: null 검증 Objects.isNull, nonNull로 변경
refactor: if 문 간 공백 제거
test: TestActor Builder 제거
test: Fixture 안 쓰는 곳 수정
refactor: 서비스, 컨트롤러 코드 메서드 순서 수정
refactor: Request to 메서드 제거
refactor: 테스트명 실제 메서드명 불일치 부분 수정
test: 커버리지 누락 보완
test: FestivalImageTest 누락된 도메인 검증 추가
test: DisplayNameGeneration 추가
refactor: 미사용 import 제거
docs: 컨벤션 엡데이트
refactor: PlaceAnnouncement 검증 테스트 추가
fix: Device 없을 경우도 알림 삭제
refactor: 미사용 import 제거
test: 예외명 컨벤션 수정
fix: 잘못된 메서드명 수정
test: 중복 display 어노테이션 제거
feat: 비밀번호 검증 추가
test: 토픽 검증 추가
test: 불필요한 검증 제거
test: 불필요한 null 검증 제거
refactor: 불필요한 공백 제거
feat: 동시성 제어 필요한 로직 인메모리락 추가
test: 테스트명 수정
test: 불필요 테스트 삭제
test: assertSoftly 추가
test: 비어 있는 것 검증 추가
test: 불필요한 테스트 삭제
test: 누락된 테스트 검증 추가
test: 불필요한 테스트 삭제
refactor: null 검증 == 비교로 변경
test: LockAspectTest 추가
test: 테스트 소속 이상한 부분 수정
feat: 아이디 한글 검증 추가
refactor: 미사용 import 제거
refactor: 의도대로 동작하는 Objects.isNull로 변경
feat: 예외 발생 락 해제 테스트 추가
#️⃣ 이슈 번호
🛠️ 작업 내용
🙇🏻 중점 리뷰 요청
📸 이미지 첨부 (Optional)
Summary by CodeRabbit
릴리스 노트
새로운 기능
개선 사항
테스트