feat: 소식지 서비스 구현 #353
feat: 소식지 서비스 구현 #353
Conversation
- 기존 어드민 전용 어노테이션을 다중 역할 지원하도록 확장 - @RequireRoleAccess 어노테이션으로 여러 역할 조합 가능하게 변경
- 소식지 관련 fixture 추가
- default 이미지 URL 설정 어떻게 할지 논의 필요
- 어드민은 그냥 삭제 가능한지 논의 필요
- 추후 Slice 적용 필요
Gyuhyeok99
requested review from
lsy1307,
nayonsoso,
whqtker and
wibaek
as code owners
Walkthrough
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
src/main/java/com/example/solidconnection/news/repository/NewsRepository.java (1)
9-9: 2. 성능 최적화 고려사항`siteUserId`로 필터링하고 `updatedAt`으로 정렬하는 쿼리입니다. 데이터가 많아질 경우를 고려하여 `(site_user_id, updated_at)` 복합 인덱스 추가를 검토해보시기 바랍니다.src/main/java/com/example/solidconnection/news/service/NewsQueryService.java (1)
20-26: 3. 데이터 변환 로직 개선 제안Stream API를 사용한 데이터 변환이 깔끔합니다. 다만 메서드명을 `getNewsBySiteUserId`로 변경하는 것을 고려해보세요. `find`보다는 `get`이 서비스 레이어에서 더 일반적입니다.- public NewsResponse findNewsBySiteUserId(Long siteUserId) { + public NewsResponse getNewsBySiteUserId(Long siteUserId) {src/main/java/com/example/solidconnection/security/aspect/RoleAuthorizationAspect.java (2)
22-24: TODO 주석 처리 계획을 확인해주세요현재 SiteUser 객체를 직접 파라미터로 받는 방식에 대한 TODO가 있습니다.
- siteUserId로 변경 예정인지 확인이 필요합니다
- 변경 시점과 마이그레이션 계획을 검토해보세요이 변경과 관련된 마이그레이션 계획이나 일정이 있다면 이슈로 추적하는 것을 도와드릴까요?
33-36: null 체크 로직 개선을 고려해보세요현재 SiteUser 파라미터가 없으면 ACCESS_DENIED 예외를 던지는데, 이것이 의도된 동작인지 확인이 필요합니다:
- 인증되지 않은 사용자의 경우 UNAUTHORIZED가 더 적절할 수 있습니다
- 또는 메서드 시그니처 검증을 통해 개발 시점에 오류를 잡는 것도 고려해보세요if (siteUser == null) { - throw new CustomException(ACCESS_DENIED); + throw new CustomException(UNAUTHORIZED); // 또는 적절한 에러 코드 }src/main/java/com/example/solidconnection/news/service/NewsCommandService.java (1)
73-77: 소유권 검증 로직을 개선해보세요현재 primitive long 비교에서 잠재적 이슈가 있을 수 있습니다:
1.!=대신!Objects.equals()사용을 고려해보세요
2. null 체크 로직 추가를 검토해보세요private void validateOwnership(News news, Long siteUserId) { - if (news.getSiteUserId() != siteUserId) { + if (!Objects.equals(news.getSiteUserId(), siteUserId)) { throw new CustomException(INVALID_NEWS_ACCESS); } }src/test/java/com/example/solidconnection/security/aspect/RoleAuthorizationAspectTest.java (2)
1-173: 역할 기반 인가 테스트 구현이 잘 되어있습니다!전체적으로 역할별 접근 권한을 체계적으로 테스트하는 구조가 잘 짜여져 있습니다. 다음 개선사항들을 고려해보시면 좋겠습니다:
테스트 메서드명 일관성 개선
- 현재 한국어 메서드명이 혼재되어 있어 프로젝트 전체의 네이밍 컨벤션과 맞춰보시면 좋겠습니다
- 영어로 통일하거나 팀 컨벤션에 따라 결정하시길 권장합니다
픽스처 사용 패턴 통일
- 멘토 생성 시
siteUserFixture.멘토(1, "mentor")와 같이 파라미터를 전달하는 방식과- 관리자/일반사용자 생성 시 파라미터 없이 호출하는 방식이 다릅니다
- 일관된 패턴으로 통일하시면 테스트 가독성이 향상됩니다
테스트 커버리지 추가 고려사항
- null 사용자에 대한 테스트 케이스
- 역할이 없는 사용자에 대한 테스트 케이스
- 다중 역할을 가진 사용자에 대한 테스트 케이스
77-77: 멘토 픽스처 생성 방식의 일관성멘토 생성 시에만 파라미터(
1, "mentor")를 전달하고 있는데, 이는 관리자나 일반 사용자 생성과 다른 패턴입니다.// 현재 방식 SiteUser mentor = siteUserFixture.멘토(1, "mentor"); // 다른 픽스처들과 일관성을 위해 고려할 수 있는 방식 SiteUser mentor = siteUserFixture.멘토();이렇게 하면 테스트 코드의 일관성이 더욱 향상됩니다.
Also applies to: 88-88, 100-100
src/main/java/com/example/solidconnection/news/controller/NewsController.java (2)
35-35: TODO 코멘트 처리 방안Slice 적용에 대한 TODO가 있습니다. 페이징 처리는 성능상 중요한 부분이므로 우선순위를 정해서 처리하시길 권장합니다.
Slice 적용을 위한 코드 예시를 작성해드릴까요? 이슈를 생성하여 추적하는 것도 좋겠습니다.
77-77: deleteNewsById 메서드 파라미터 일관성다른 CUD 메서드들과 달리
deleteNewsById에서만SiteUser객체 전체를 서비스로 전달하고 있습니다.- NewsCommandResponse newsCommandResponse = newsCommandService.deleteNewsById(siteUser, newsId); + NewsCommandResponse newsCommandResponse = newsCommandService.deleteNewsById(siteUser.getId(), newsId);이렇게 하면 다른 메서드들과 일관성을 유지할 수 있습니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
src/main/java/com/example/solidconnection/application/controller/ApplicationController.java(2 hunks)src/main/java/com/example/solidconnection/common/exception/ErrorCode.java(2 hunks)src/main/java/com/example/solidconnection/news/controller/NewsController.java(1 hunks)src/main/java/com/example/solidconnection/news/domain/News.java(2 hunks)src/main/java/com/example/solidconnection/news/dto/NewsCommandResponse.java(1 hunks)src/main/java/com/example/solidconnection/news/dto/NewsCreateRequest.java(1 hunks)src/main/java/com/example/solidconnection/news/dto/NewsItemResponse.java(1 hunks)src/main/java/com/example/solidconnection/news/dto/NewsResponse.java(1 hunks)src/main/java/com/example/solidconnection/news/dto/NewsUpdateRequest.java(1 hunks)src/main/java/com/example/solidconnection/news/repository/NewsRepository.java(1 hunks)src/main/java/com/example/solidconnection/news/service/NewsCommandService.java(1 hunks)src/main/java/com/example/solidconnection/news/service/NewsQueryService.java(1 hunks)src/main/java/com/example/solidconnection/s3/domain/ImgType.java(1 hunks)src/main/java/com/example/solidconnection/security/annotation/RequireRoleAccess.java(1 hunks)src/main/java/com/example/solidconnection/security/aspect/RoleAuthorizationAspect.java(1 hunks)src/main/resources/db/migration/V20__add_site_user_id_fk_to_news.sql(1 hunks)src/test/java/com/example/solidconnection/news/fixture/NewsFixture.java(1 hunks)src/test/java/com/example/solidconnection/news/fixture/NewsFixtureBuilder.java(1 hunks)src/test/java/com/example/solidconnection/news/service/NewsCommandServiceTest.java(1 hunks)src/test/java/com/example/solidconnection/news/service/NewsQueryServiceTest.java(1 hunks)src/test/java/com/example/solidconnection/security/aspect/AdminAuthorizationAspectTest.java(0 hunks)src/test/java/com/example/solidconnection/security/aspect/RoleAuthorizationAspectTest.java(1 hunks)src/test/java/com/example/solidconnection/siteuser/fixture/SiteUserFixture.java(1 hunks)
💤 Files with no reviewable changes (1)
- src/test/java/com/example/solidconnection/security/aspect/AdminAuthorizationAspectTest.java
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/test/java/com/example/solidconnection/news/fixture/NewsFixtureBuilder.java (1)
src/test/java/com/example/solidconnection/news/fixture/NewsFixture.java (1)
TestComponent(7-32)
src/main/java/com/example/solidconnection/security/aspect/RoleAuthorizationAspect.java (2)
src/main/java/com/example/solidconnection/application/controller/ApplicationController.java (1)
RequiredArgsConstructor(23-65)src/test/java/com/example/solidconnection/security/aspect/RoleAuthorizationAspectTest.java (1)
Component(156-172)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (39)
src/main/java/com/example/solidconnection/s3/domain/ImgType.java (1)
7-7: 소식지 이미지 타입 추가가 깔끔하게 구현되었습니다!기존 이미지 타입들과 일관된 네이밍 패턴을 유지하면서 NEWS 타입을 추가한 것이 좋습니다. 소식지 기능의 이미지 처리를 위한 적절한 확장이네요.
src/test/java/com/example/solidconnection/siteuser/fixture/SiteUserFixture.java (1)
59-68: 멘토 사용자 픽스처 추가가 잘 구현되었습니다!1. **일관성 유지**: 기존 사용자 생성 메서드들과 동일한 패턴을 따르고 있어 좋습니다 2. **적절한 기본값**: mentor 접두사가 붙은 이메일과 mentor123 비밀번호로 멘토 역할에 맞는 기본값을 설정했네요 3. **테스트 지원**: 소식지 기능 테스트에서 멘토 역할 사용자를 쉽게 생성할 수 있도록 지원합니다src/main/resources/db/migration/V20__add_site_user_id_fk_to_news.sql (1)
1-4: 기존 뉴스 데이터 유무 확인 필요
- NOT NULL 제약조건 주의: site_user_id를 NOT NULL으로 추가하면 기존 뉴스 레코드가 있을 경우 마이그레이션이 실패할 수 있습니다.
- 외래키 제약조건 적절: site_user(id) 테이블과의 관계 설정은 올바르게 구현되었습니다.
- 단계적 마이그레이션 고려: 초기에는 NULL 허용으로 컬럼을 추가하고, 기존 데이터를 채운 뒤 NOT NULL 제약조건을 추가하는 방식을 검토하세요.
운영 환경에서 다음 쿼리를 실행해 기존 뉴스 데이터가 존재하는지 확인해주세요:
SELECT COUNT(*) FROM news;src/main/java/com/example/solidconnection/news/dto/NewsCommandResponse.java (1)
5-12: 깔끔한 Command 응답 DTO 구현입니다!1. **Record 활용**: Java 14+ record를 사용해 간결하고 불변한 DTO를 구현했네요 2. **명확한 목적**: Command 패턴의 응답으로 id만 반환하는 것이 적절합니다 3. **팩토리 메서드**: from() 메서드로 News 엔티티로부터 쉽게 변환할 수 있도록 구현했습니다src/main/java/com/example/solidconnection/application/controller/ApplicationController.java (2)
9-10: 1. 보안 어노테이션 리팩토링 승인기존 `RequireAdminAccess`에서 `RequireRoleAccess(roles = {Role.ADMIN})`로의 변경이 잘 적용되었습니다. 이는 향후 멘토나 다른 역할에 대한 접근 제어 확장성을 제공하는 좋은 개선사항입니다.
43-43: 일관된 @RequireRoleAccess 적용 확인 완료
- 일관된 어노테이션 적용 확인
- NewsController: @PostMapping, @PatchMapping, @DeleteMapping에 @RequireRoleAccess(roles = {Role.ADMIN, Role.MENTOR}) 적용
- ApplicationController: @GetMapping에 @RequireRoleAccess(roles = {Role.ADMIN}) 적용- 이전 @RequireAdminAccess 어노테이션 제거 확인
- 전체 검색 결과, @RequireAdminAccess 사용 잔여 없음이상으로 모든 컨트롤러에 역할 기반 접근 제어가 일관되게 적용된 것을 확인했습니다. 추가 수정은 필요하지 않습니다.
src/main/java/com/example/solidconnection/common/exception/ErrorCode.java (2)
45-45: 1. 뉴스 관련 에러 코드 추가 승인`NEWS_NOT_FOUND` 에러 코드가 적절하게 추가되었습니다. 404 상태 코드와 명확한 한국어 메시지가 잘 정의되어 있습니다.
100-101: 2. 접근 권한 에러 코드 적절히 배치`INVALID_NEWS_ACCESS` 에러 코드가 적절한 위치에 추가되었습니다. 기존 커뮤니티의 `INVALID_POST_ACCESS`와 동일한 패턴을 따라 일관성이 유지되었습니다.src/main/java/com/example/solidconnection/news/dto/NewsResponse.java (2)
5-7: 1. Record 클래스 구조 승인`NewsResponse` record 클래스가 깔끔하게 정의되었습니다. `List<NewsItemResponse>`를 감싸는 wrapper 역할을 잘 수행합니다.
8-10: 2. Factory Method 패턴 적용 좋음`from()` static factory method가 적절하게 구현되었습니다. 객체 생성의 의도를 명확하게 표현하는 좋은 패턴입니다.src/main/java/com/example/solidconnection/news/repository/NewsRepository.java (1)
8-10: 1. JPA Repository 구조 및 쿼리 메서드 승인`NewsRepository`가 적절하게 정의되었습니다. `findAllBySiteUserIdOrderByUpdatedAtDesc` 메서드명이 직관적이고 JPA naming convention을 잘 따릅니다.src/main/java/com/example/solidconnection/news/service/NewsQueryService.java (2)
13-17: 1. 서비스 클래스 구조 승인`@Service`, `@RequiredArgsConstructor` 어노테이션이 적절하게 적용되었고, NewsRepository 의존성 주입이 깔끔하게 처리되었습니다.
19-19: 2. 읽기 전용 트랜잭션 적용 좋음`@Transactional(readOnly = true)` 적용으로 성능 최적화가 잘 되어있습니다. 읽기 전용 쿼리에 적절한 설정입니다.src/test/java/com/example/solidconnection/news/fixture/NewsFixture.java (2)
13-21: 1. 소식지 테스트 픽스처 메서드 구현이 깔끔합니다기본 thumbnailUrl을 포함한 소식지 생성 메서드가 잘 구현되어 있습니다. 테스트에 필요한 기본값들이 적절하게 설정되어 있고, NewsFixtureBuilder와의 협력도 명확합니다.
23-31: 2. 오버로딩된 소식지 메서드가 유연성을 제공합니다thumbnailUrl을 커스터마이징할 수 있는 오버로딩 메서드가 테스트의 다양한 시나리오를 지원하네요. 메서드 시그니처도 명확하고 사용하기 편리해 보입니다.
src/test/java/com/example/solidconnection/news/service/NewsQueryServiceTest.java (2)
19-21: 1. 테스트 클래스 구조가 표준적이고 명확합니다
@TestContainerSpringBootTest어노테이션과 한국어@DisplayName이 일관되게 사용되어 있어 테스트의 목적이 명확하게 드러납니다.
33-50: 2. 멘토별 소식지 조회 테스트가 포괄적으로 구현되었습니다1. **테스트 데이터 설정**: 두 명의 멘토와 각각의 소식지를 생성하여 격리된 테스트 환경을 구성 2. **비즈니스 로직 검증**: 특정 멘토의 소식지만 조회되는지 확인 3. **정렬 순서 검증**: updatedAt 기준 내림차순 정렬이 제대로 동작하는지 검증특히
Comparator.reverseOrder()를 사용한 정렬 검증이 정확하고 명확합니다.src/main/java/com/example/solidconnection/news/dto/NewsItemResponse.java (2)
8-18: 1. NewsItemResponse 레코드 구조가 잘 설계되었습니다1. **필드 구성**: 소식지 조회에 필요한 모든 핵심 필드들이 포함되어 있음 2. **JSON 매핑**: `@JsonProperty("contentPreview")`로 description 필드의 naming inconsistency를 깔끔하게 해결 3. **타입 선택**: `ZonedDateTime`을 사용하여 시간대 정보까지 포함한 정확한 시간 표현PR 목표에서 언급된 Bruno API 스펙과의 호환성 문제를
@JsonProperty로 해결한 것이 인상적입니다.
19-28: 2. 팩토리 메서드 구현이 간결하고 명확합니다
from(News news)정적 팩토리 메서드가 도메인 엔티티에서 DTO로의 변환을 깔끔하게 처리하고 있습니다. 모든 필드 매핑이 직관적이고 누락된 부분이 없네요.src/main/java/com/example/solidconnection/news/dto/NewsCreateRequest.java (2)
9-23: 1. 유효성 검증 어노테이션이 철저하게 적용되었습니다1. **필드별 검증**: `@NotBlank`, `@Size`, `@URL` 어노테이션으로 입력값 검증이 포괄적으로 구현 2. **길이 제한**: PR 목표에서 언급된 제목 20자, 내용 30자 제한이 정확히 반영 3. **사용자 친화적 메시지**: 한국어로 된 명확한 오류 메시지 제공 4. **JSON 매핑 일관성**: `@JsonProperty("contentPreview")`로 response DTO와 일관된 API 스펙 유지URL 최대 길이를 500자로 설정한 것도 실용적인 선택입니다.
24-32: 2. toEntity 메서드가 도메인 변환을 깔끔하게 처리합니다
thumbnailUrl과siteUserId를 외부에서 주입받아 News 엔티티를 생성하는 구조가 책임 분리 원칙에 잘 맞습니다. 이미지 처리와 사용자 인증은 서비스 레이어에서 처리하고, DTO는 순수한 데이터 변환만 담당하는 설계가 적절합니다.src/test/java/com/example/solidconnection/news/fixture/NewsFixtureBuilder.java (2)
8-19: 1. NewsFixtureBuilder 클래스 구조가 표준적입니다
@TestComponent와@RequiredArgsConstructor를 사용한 의존성 주입이 적절하고, News 엔티티의 모든 필드를 포함한 private 필드 구성도 완전합니다. 새로 추가된siteUserId필드까지 포함되어 있어 도메인 모델 변경사항이 잘 반영되었습니다.
20-43: 2. 플루언트 인터페이스 구현이 완벽합니다1. **메서드 체이닝**: 모든 setter 메서드가 `this`를 반환하여 fluent interface 패턴 지원 2. **일관된 네이밍**: 각 필드별 setter 메서드명이 명확하고 일관됨 3. **타입 안전성**: long 타입의 siteUserId까지 포함하여 타입 안전성 보장빌더 패턴이 깔끔하게 구현되어 테스트 코드 작성이 편리해질 것 같습니다.
src/main/java/com/example/solidconnection/news/dto/NewsUpdateRequest.java (2)
8-13: ✅ 검증 어노테이션이 적절히 적용되었습니다
- 제목과 내용의 길이 제한이 명확하게 설정되어 있어 좋습니다
@JsonProperty("contentPreview")사용으로 Bruno API 명세와의 네이밍 불일치를 잘 해결했습니다- 한국어 메시지로 사용자 친화적인 에러 응답이 가능합니다
19-19: Boolean 래퍼 타입 사용이 적절합니다
resetToDefaultImage필드에 Boolean 래퍼 타입을 사용한 것이 좋은 선택입니다. 이렇게 하면:
-true: 기본 이미지로 리셋
-false: 리셋하지 않음
-null: 이미지 변경 없음의 3가지 상태를 명확히 구분할 수 있어 업데이트 로직이 더 직관적입니다.
src/main/java/com/example/solidconnection/security/aspect/RoleAuthorizationAspect.java (1)
37-44: 역할 검증 로직이 명확하고 효율적입니다
finalSiteUser변수 사용으로 lambda 내부에서 안전하게 접근 가능합니다Arrays.stream().anyMatch()사용으로 간결하고 읽기 쉬운 코드가 되었습니다- Role enum 비교로 타입 안전성이 보장됩니다
src/main/java/com/example/solidconnection/news/service/NewsCommandService.java (3)
79-85: 권한 검증 로직이 잘 구현되었습니다
- 소유자와 관리자 모두 삭제 권한을 가지도록 구현된 것이 합리적입니다
- boolean 변수로 조건을 명확히 분리하여 가독성이 좋습니다
- Role enum을 사용한 타입 안전한 비교가 적용되었습니다
99-109: 이미지 업데이트 로직이 체계적으로 구현되었습니다
- 기본 이미지 리셋과 새 이미지 업로드를 명확히 분리했습니다
- 기존 커스텀 이미지 삭제 로직이 적절히 처리되었습니다
- else-if 구조로 우선순위가 명확합니다
한 가지 개선점: 두 조건이 동시에 true인 경우의 동작을 명확히 정의하면 좋겠습니다.
111-115: 기본 이미지 보호 로직이 안전하게 구현되었습니다기본 이미지 URL과 다를 때만 S3 삭제를 호출하는 것이 올바른 접근입니다. 이렇게 하면:
- 기본 이미지가 실수로 삭제되는 것을 방지합니다
- S3 호출 최적화가 가능합니다src/main/java/com/example/solidconnection/news/domain/News.java (3)
38-49: 커스텀 생성자가 적절히 구현되었습니다
- ID를 제외한 모든 필드를 초기화하는 생성자로 엔티티 생성 로직이 명확합니다
- 필드 순서가 논리적으로 배치되어 있어 가독성이 좋습니다
- JPA 요구사항인 기본 생성자와 함께 잘 설계되었습니다
51-65: 업데이트 메서드들이 캡슐화 원칙에 따라 잘 구현되었습니다
- 각 필드별로 개별 업데이트 메서드를 제공하여 세밀한 제어가 가능합니다
- 메서드명이 명확하여 사용 의도를 쉽게 파악할 수 있습니다
- void 반환으로 사이드 이펙트가 명확히 드러납니다
이는 DDD의 도메인 객체 설계 원칙에 부합하는 좋은 접근입니다.
36-36: siteUserId 필드 타입 및 제약 명시 검토하기
- siteUserId 필드는 non-null 제약이 확실하다면 primitive
long으로 유지해도 괜찮습니다.- non-null 제약을 명확히 표현하기 위해
@Column(nullable = false)어노테이션을 추가해주세요.- 향후 siteUserId를 optional로 처리해야 한다면 wrapper 타입
Long으로 변경을 고려해보세요.src/test/java/com/example/solidconnection/news/service/NewsCommandServiceTest.java (4)
35-37: 테스트 클래스 구조가 체계적으로 설계되었습니다
@TestContainerSpringBootTest사용으로 실제 DB 환경에서 통합 테스트가 가능합니다@DisplayName으로 한국어 테스트 설명이 명확하게 작성되었습니다@Nested클래스로 기능별 테스트가 잘 그룹화되었습니다
90-315: 이미지 처리 관련 테스트 커버리지가 매우 우수합니다특히 이미지 관련 테스트가 포괄적으로 작성되었습니다:
1. 커스텀 이미지 테스트: 기존 이미지 삭제 후 새 이미지 업로드 검증
2. 기본 이미지 테스트: 기본 이미지는 삭제되지 않음을 확인
3. 리셋 기능 테스트:resetToDefaultImage플래그 동작 검증
4. S3 호출 검증:then(s3Service).should()로 정확한 메서드 호출 확인이런 세밀한 테스트로 이미지 처리 로직의 신뢰성이 크게 향상됩니다.
182-199: 권한 검증 테스트가 보안 관점에서 중요하게 다뤄졌습니다다른 사용자의 소식지 수정 시도에 대한 예외 검증이 제대로 구현되었습니다:
1. 다른 멘토 사용자로 테스트하여 현실적인 시나리오를 재현했습니다
2.CustomException과 정확한 에러 메시지 검증으로 보안이 강화되었습니다
3. 권한 체크 로직의 정확성을 확신할 수 있습니다
162-179: 공백 입력 처리 테스트로 견고성이 확보되었습니다공백 문자열 입력 시 업데이트되지 않는 로직을 검증하는 것이 좋은 접근입니다:
- 사용자가 실수로 공백을 입력하는 경우를 대비
-hasValue()메서드의 동작을 정확히 검증
- 데이터 무결성 보장이런 세심한 테스트가 프로덕션 환경에서의 안정성을 높입니다.
src/test/java/com/example/solidconnection/security/aspect/RoleAuthorizationAspectTest.java (1)
148-172: 테스트 설정 클래스들이 깔끔하게 구성되어있습니다TestConfiguration과 TestService 클래스가 명확하게 분리되어 있고, @RequireRoleAccess 어노테이션의 다양한 사용 사례를 잘 보여주고 있습니다. 특히 다중 역할 지원(
Role.ADMIN, Role.MENTOR)을 테스트하는 부분이 훌륭합니다.src/main/java/com/example/solidconnection/news/controller/NewsController.java (2)
1-80: 뉴스 컨트롤러 구현이 전반적으로 잘 되어있습니다!RESTful API 설계 원칙을 잘 따르고 있고, 역할 기반 접근 제어도 적절히 적용되어 있습니다. 다음 사항들을 검토해보시면 좋겠습니다:
보안 검토사항
- GET 엔드포인트(line 36-42)에 인가 처리가 없어 누구나 접근 가능합니다
- siteUserId를 통해 특정 사용자의 뉴스를 조회할 수 있어 데이터 노출 위험이 있습니다
파라미터 네이밍 일관성
site-user-id,news-id(kebab-case)와file(camelCase)가 혼재되어 있습니다- 프로젝트 전체의 네이밍 컨벤션에 맞춰 통일하시길 권장합니다
파일 업로드 검증
- MultipartFile에 대한 크기, 타입 검증이 컨트롤러 레벨에 없습니다
- 서비스 레벨에서 처리하고 계시는지 확인이 필요합니다
44-44: 역할 기반 접근 제어가 적절히 적용되었습니다ADMIN과 MENTOR 역할에만 뉴스 생성, 수정, 삭제 권한을 부여한 것이 비즈니스 요구사항에 적합해 보입니다. PR 목표에서 언급된 유연한 역할 기반 접근 제어가 잘 구현되어 있습니다.
Also applies to: 55-55, 71-71
src/main/java/com/example/solidconnection/security/annotation/RequireRoleAccess.java
Outdated
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/news/fixture/NewsFixtureBuilder.java
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/news/service/NewsCommandService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/news/controller/NewsController.java
Show resolved
Hide resolved
There was a problem hiding this comment.
1/ 기본 로고 이미지 경로를 어떻게 가져올지
S3에 업로드된 로고의 URL을 활용하는게 좋다 생각합니다.
코드 내부에서 보관 방법은 두가지가 떠오르는데요,
- static final로 상수로 보관
- application-varaible.yml 에 변수로 보관, ConfigurationProperties로 가져와서 사용
이 둘중에 후자가 더 괜찮아보입니다.
그 이유는, “비지니스 요구사항”이 아닌 값이기도 하고, 변경되었을 때 코드의 내용이 변하는게 아니라
환경변수를 관리하는 파일의 값이 변하는게 더 자연스럽다 생각합니다.
(+추가: 코더 레빗도 이걸 추천했네요🐰👍)
2/ 제목 및 내용 최대 글자 수 제한 관련
기획의 내용으로 쉽게 변경될 수 있는 것이니, schema 에서는 타이트하기 잡지 말고
어노테이션으로 검증하는 것만으로 충분하다 생각합니다!
3/ 소식지도 siteUserId로 받고 flyway에 fk추가만 하면 되는 것일까요?
👌👌
4/ 소식지 siteUserId 관련
피그마를 보다보니 소식지 전체조회가 아닌 해당 멘토의 소식지 조회만 있는 거 같은데
"홈 화면"에서는 전체 소식지 목록을 조회하고 있습니다. 😲
그리고 솔리드 커넥션 인스타그램 게시물 등을 홈 화면에서 보여줄 수 있으니, site-user-id 로 받게 하는게 맞는 것 같습니다!
5/ 권한 구분 관련
권한 구분을 유연하게 하기 위해 영서님이 security 관련 리팩토링을 완료하기 전에 임시로 RequireRoleAccess를 구현하였는데 괜찮은가요?
네 좋습니다!
6/ 소식지 수정 및 삭제 권한
관리자라면 그것과 상관없이 수정 및 삭제를 할 수 있어야할까요?
여러번 느끼는건데 규혁님은 쉽게 놓칠 수 있는 고려사항들을 잘 떠올리시는 것 같아요. 🥹✨
수정은 문제가 될 것 같지만, 삭제는 필요해보이네요!
이건 기획 / 프론트와도 같이 이야기해야할 것 같아요.
다음 회의에서 이야기할 수 있도록 디스코드에 남겨뒀어요~
7/ 썸네일 기본이미지로 변경하기
Boolean resetToDefaultImage라는 값을 추가하였는데 괜찮을까요?
프론트에서 “thumbnailmg”를 빈 값으로 보내면, 서버가 기본 이미지를 넣는게 더 간단한 방법이라 생각해요.
그리고 '기본 이미지 사용'이라는 상태를 나타내기 위해
- thumbnailmg가 빈 값
- resetToDefaultImage가 true
이렇게 하나의 의미에 대해 두개의 데이터가 존재하는 것은 불필요하다는 생각도 들고요!
src/main/java/com/example/solidconnection/security/annotation/RequireRoleAccess.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/security/aspect/RoleAuthorizationAspect.java
Outdated
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/security/aspect/RoleAuthorizationAspectTest.java
Show resolved
Hide resolved
| @Aspect | ||
| @Component | ||
| @RequiredArgsConstructor | ||
| public class AdminAuthorizationAspect { | ||
| public class RoleAuthorizationAspect { |
There was a problem hiding this comment.
캬.. 확장성 고려해서 이렇게 바꾸는 것 너무 좋아요🥹
src/main/java/com/example/solidconnection/news/controller/NewsController.java
Outdated
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/news/service/NewsCommandServiceTest.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/news/repository/NewsRepository.java
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/news/service/NewsQueryService.java
Outdated
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/news/service/NewsQueryServiceTest.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/news/dto/NewsItemResponse.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/test/java/com/example/solidconnection/news/service/NewsCommandServiceTest.java (1)
282-285: 삭제 테스트의 검증도 assertAll로 묶으면 좋겠습니다.일관성을 위해 다른 테스트처럼 assertAll을 사용하는 것이 좋을 것 같습니다:
// then -assertThat(response.id()).isEqualTo(originNews.getId()); -assertThat(newsRepository.findById(originNews.getId())).isEmpty(); -then(s3Service).should().deletePostImage(expectedImageUrl); +assertAll( + () -> assertThat(response.id()).isEqualTo(originNews.getId()), + () -> assertThat(newsRepository.findById(originNews.getId())).isEmpty() +); +then(s3Service).should().deletePostImage(expectedImageUrl);
🧹 Nitpick comments (7)
src/main/java/com/example/solidconnection/news/dto/NewsResponse.java (1)
15-24: 정적 팩토리 메서드에 null 안전성 검증을 추가해보세요.
from메서드의 구현이 명확하고 직관적입니다. 다만News엔티티의 필드들이 null일 가능성에 대한 방어 로직을 고려해보시면 좋겠습니다.다음과 같이 null 체크를 추가하는 것을 제안드립니다:
public static NewsResponse from(News news) { + if (news == null) { + throw new IllegalArgumentException("News cannot be null"); + } return new NewsResponse( news.getId(), news.getTitle(), news.getDescription(), news.getThumbnailUrl(), news.getUrl(), news.getUpdatedAt() ); }src/main/java/com/example/solidconnection/security/aspect/RoleAuthorizationAspect.java (2)
35-39: 🚀 역할 확인 로직의 성능 최적화를 고려해보세요현재
Arrays.asList().contains()방식은 매번 새로운 리스트 객체를 생성합니다. 더 효율적인 방법을 제안드립니다:- Role[] allowedRoles = requireRoleAccess.roles(); - boolean hasAccess = Arrays.asList(allowedRoles).contains(siteUser.getRole()); - if (!hasAccess) { + Role[] allowedRoles = requireRoleAccess.roles(); + boolean hasAccess = Arrays.stream(allowedRoles) + .anyMatch(role -> role == siteUser.getRole()); + if (!hasAccess) {또는 더 간단하게:
- Role[] allowedRoles = requireRoleAccess.roles(); - boolean hasAccess = Arrays.asList(allowedRoles).contains(siteUser.getRole()); + Role[] allowedRoles = requireRoleAccess.roles(); + Role userRole = siteUser.getRole(); + boolean hasAccess = false; + for (Role allowedRole : allowedRoles) { + if (allowedRole == userRole) { + hasAccess = true; + break; + } + }두 방법 모두 불필요한 객체 생성을 피할 수 있어 성능상 이점이 있습니다.
22-22: 📝 TODO 주석에 대한 고려사항주석에서 언급된
siteUserId로의 파라미터 변경은 좋은 아이디어입니다. 현재SiteUser객체 전체를 전달받는 방식보다는 ID만 전달받아 내부에서 조회하는 방식이 더 깔끔할 수 있습니다.향후 리팩토링 시 고려사항:
1. 보안 강화: ID 기반 조회로 더 안전한 사용자 검증
2. 의존성 감소: 컨트롤러에서 전체 객체를 전달할 필요 없음
3. 일관성: 다른 보안 체크와의 일관된 패턴 유지다만 현재 구조도 명확하고 직관적이므로, 전체적인 보안 리팩토링 계획과 함께 진행하시는 것이 좋을 것 같습니다.
src/test/java/com/example/solidconnection/news/service/NewsCommandServiceTest.java (2)
80-83: 소식지 생성 테스트에 추가 검증을 포함하면 좋겠습니다.현재는 ID만 검증하고 있는데, 다른 필드들도 함께 검증하면 더 완전한 테스트가 될 것 같습니다:
// then News savedNews = newsRepository.findById(response.id()).orElseThrow(); -assertThat(response.id()).isEqualTo(savedNews.getId()); +assertAll( + () -> assertThat(response.id()).isEqualTo(savedNews.getId()), + () -> assertThat(savedNews.getTitle()).isEqualTo(request.title()), + () -> assertThat(savedNews.getDescription()).isEqualTo(request.description()), + () -> assertThat(savedNews.getUrl()).isEqualTo(request.url()), + () -> assertThat(savedNews.getThumbnailUrl()).isEqualTo(expectedImageUrl), + () -> assertThat(savedNews.getSiteUserId()).isEqualTo(user.getId()) +);
65-287: 테스트 커버리지 확장을 고려해보세요.현재 테스트는 잘 작성되어 있지만, 다음과 같은 추가 시나리오도 고려해보면 좋겠습니다:
생성 실패 케이스
- S3 업로드 실패 시나리오
- 잘못된 입력값 검증
삭제 권한 검증
- 다른 사용자가 삭제 시도하는 케이스
- 관리자가 삭제하는 케이스
동시성 테스트
- 동시에 같은 뉴스를 수정하는 시나리오
이러한 테스트 케이스들을 추가로 작성해드릴까요?
src/main/java/com/example/solidconnection/news/service/NewsCommandService.java (2)
32-32: 메서드 파라미터 사이에 공백을 추가해주세요.가독성을 위해 파라미터 사이에 공백을 추가하는 것이 좋습니다:
-public NewsCommandResponse createNews(Long siteUserId,NewsCreateRequest newsCreateRequest, MultipartFile imageFile) { +public NewsCommandResponse createNews(Long siteUserId, NewsCreateRequest newsCreateRequest, MultipartFile imageFile) {
54-62: 관리자 권한 처리가 잘 구현되었습니다.PR에서 언급하신 "관리자가 모든 소식지를 수정/삭제할 수 있는지"에 대한 고민이 잘 반영되었네요. 현재 구현은:
- 소유자 또는 관리자만 삭제 가능
- 일반적인 커뮤니티 시스템과 달리 관리자에게 전체 권한 부여
운영 관점에서 로깅 추가를 고려해보세요:
// 관리자가 다른 사용자의 뉴스를 삭제하는 경우 로그 남기기 if (isAdmin && !isOwner) { log.info("Admin {} deleted news {} owned by user {}", currentUser.getId(), news.getId(), news.getSiteUserId()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/main/java/com/example/solidconnection/news/config/NewsProperties.java(1 hunks)src/main/java/com/example/solidconnection/news/controller/NewsController.java(1 hunks)src/main/java/com/example/solidconnection/news/domain/News.java(2 hunks)src/main/java/com/example/solidconnection/news/dto/NewsListResponse.java(1 hunks)src/main/java/com/example/solidconnection/news/dto/NewsResponse.java(1 hunks)src/main/java/com/example/solidconnection/news/dto/NewsUpdateRequest.java(1 hunks)src/main/java/com/example/solidconnection/news/repository/NewsRepository.java(1 hunks)src/main/java/com/example/solidconnection/news/service/NewsCommandService.java(1 hunks)src/main/java/com/example/solidconnection/news/service/NewsQueryService.java(1 hunks)src/main/java/com/example/solidconnection/security/annotation/RequireRoleAccess.java(1 hunks)src/main/java/com/example/solidconnection/security/aspect/RoleAuthorizationAspect.java(1 hunks)src/main/resources/secret(1 hunks)src/test/java/com/example/solidconnection/news/service/NewsCommandServiceTest.java(1 hunks)src/test/java/com/example/solidconnection/news/service/NewsQueryServiceTest.java(1 hunks)src/test/java/com/example/solidconnection/security/aspect/RoleAuthorizationAspectTest.java(1 hunks)src/test/resources/application.yml(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- src/test/resources/application.yml
- src/main/resources/secret
- src/main/java/com/example/solidconnection/news/dto/NewsListResponse.java
- src/main/java/com/example/solidconnection/news/config/NewsProperties.java
🚧 Files skipped from review as they are similar to previous changes (7)
- src/main/java/com/example/solidconnection/news/repository/NewsRepository.java
- src/test/java/com/example/solidconnection/news/service/NewsQueryServiceTest.java
- src/main/java/com/example/solidconnection/news/dto/NewsUpdateRequest.java
- src/main/java/com/example/solidconnection/news/service/NewsQueryService.java
- src/test/java/com/example/solidconnection/security/aspect/RoleAuthorizationAspectTest.java
- src/main/java/com/example/solidconnection/news/domain/News.java
- src/main/java/com/example/solidconnection/news/controller/NewsController.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/com/example/solidconnection/security/aspect/RoleAuthorizationAspect.java (2)
src/main/java/com/example/solidconnection/application/controller/ApplicationController.java (1)
RequiredArgsConstructor(23-65)src/test/java/com/example/solidconnection/security/aspect/RoleAuthorizationAspectTest.java (1)
Component(82-98)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (5)
src/main/java/com/example/solidconnection/news/dto/NewsResponse.java (1)
7-14: 뉴스 응답 DTO 구조가 잘 설계되어 있습니다!Record를 사용한 구현이 깔끔하고 적절합니다. 다음과 같은 장점들이 있습니다:
1. 불변성 보장으로 데이터 안전성 확보 2. 간결한 코드로 가독성 향상 3. 뉴스 도메인에 필요한 핵심 정보들을 모두 포함src/main/java/com/example/solidconnection/security/annotation/RequireRoleAccess.java (1)
12-14: 🎯 명시적 역할 지정으로 개선된 설계입니다!과거 리뷰 피드백을 잘 반영하여 기본값을 제거하고 명시적 역할 지정을 강제하도록 개선되었네요. 이제 어노테이션 사용 시 의도가 명확해지고 예측 가능한 동작을 보장할 수 있습니다.
다음과 같은 개선점들이 돋보입니다:
1. 명확한 의도: 기본값 없이 역할을 명시적으로 지정해야 함
2. 확장성: 여러 역할을 배열로 지원하여 유연한 권한 제어 가능
3. 예측가능성: 어노테이션 사용 시 어떤 역할이 필요한지 코드에서 바로 확인 가능src/main/java/com/example/solidconnection/security/aspect/RoleAuthorizationAspect.java (1)
20-24: ✨ 역할 기반 접근 제어로 훌륭하게 확장되었습니다!클래스명과 메서드명이 더욱 명확해지고, 여러 역할을 동적으로 처리할 수 있도록 개선되었네요. 확장성과 유연성이 크게 향상되었습니다.
주요 개선사항들:
1. 명확한 네이밍:RoleAuthorizationAspect로 기능을 더 잘 표현
2. 동적 역할 처리: 어노테이션에서 지정된 역할들을 런타임에 확인
3. 유연한 권한 제어: ADMIN과 MENTOR 등 다양한 역할 조합 지원src/test/java/com/example/solidconnection/news/service/NewsCommandServiceTest.java (1)
125-132: assertAll을 잘 활용하셨네요! 👍이전 리뷰 코멘트를 반영해서 여러 검증을 assertAll로 묶어주신 것이 좋습니다. 가독성도 높아지고 모든 검증이 실행되도록 보장됩니다.
src/main/java/com/example/solidconnection/news/service/NewsCommandService.java (1)
86-96: 썸네일 업데이트 로직이 잘 구현되었습니다! 👍다음 사항들이 특히 좋습니다:
- null 안전성:
Boolean.TRUE.equals()를 사용한 null 체크- 리소스 관리: 기존 커스텀 이미지 삭제 처리
- 명확한 조건 분기: 세 가지 케이스를 명확히 구분
PR에서 언급하신 Boolean vs boolean 고민에 대해서는 현재 구현(Boolean wrapper)이 적절합니다. null 상태로 "변경 없음"을 표현할 수 있어 더 유연합니다.
src/main/java/com/example/solidconnection/news/service/NewsCommandService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/news/service/NewsCommandService.java
Show resolved
Hide resolved
|
꼼꼼하게 리뷰해주셔서 감사합니다! 남은 의문점 하나만 더 여쭤보겠습니다!
이 방식으로 구현을 한다면 프로필을 수정하지 않는 것과 기본이미지로 바꾸려는 것을 어떻게 구분을 해야할까요?
이런 것일까요? 근데 이방식도 null과 empty를 잘 구분할 수 있을지 아직 시도를 안해봐서 모르겠네요 ㅎㅎ.. 저는 그래서 resetToDefaultImage이게 불필요한 데이터라고 생각을 안하고 있었는데 제가 잘못이해한 것인지 여쭤봐요! |
whqtker
left a comment
whqtker
left a comment
There was a problem hiding this comment.
고생하셨습니다! 이미 어느 정도 피드백에 반영된 것 같아, 의논해야 하는 부분만 코멘트 달겠습니다.
- 소식지 수정/삭제 관련: 저도 관리자는 삭제만, 멘토는 수정/삭제 모두 가능해야 한다고 생각합니다.
resetToDefaultImage관련: 저는 있는 쪽이 사용자의 의도를 더 잘 드러낸다고 생각합니다.
nayonsoso
left a comment
nayonsoso
left a comment
There was a problem hiding this comment.
추가 답 코멘트 & 리뷰 남깁니다 😊
빠르게 코드 구현해주시고 소통도 잘 해주셔서 정말x100 감사드립니다~ 🌟
이 방식으로 구현을 한다면 프로필을 수정하지 않는 것과 기본이미지로 바꾸려는 것을 어떻게 구분을 해야할까요?
아하.. 복잡해지긴 하겠네요 😓
설명을 듣고 저도 설득되었습니다!
src/main/java/com/example/solidconnection/news/repository/NewsRepository.java
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/news/service/NewsCommandServiceTest.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/news/service/NewsQueryService.java
Outdated
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/news/service/NewsCommandServiceTest.java
Show resolved
Hide resolved
nayonsoso
left a comment
nayonsoso
left a comment
There was a problem hiding this comment.
반영하신 것 확인했습니다 😁
고생 많으셨어요!!
3 participants
관련 이슈
작업 내용
소식지 관련 crud를 구현하였습니다!
구현하며 궁금한 점들은 밑에 요구사항이 추가해놓았습니다!(생각보다 많아서 죄송합니다 😅)
특이 사항
으앗.. 권한 구분 관련 로직은 따로 pr을 뺄걸 그랬네요... 다음부턴 이런 긴 pr 올리지 않겠습니다... ㅠㅠ
그래도 커밋순서대로 읽으면 그나마 읽기 편할 거 같습니다....
리뷰 요구사항 (선택)
1. 기본 로고 이미지 경로를 어떻게 가져올지
수경님이 썸네일을 등록 안한다면 기본 로고 이미지를 보여달라고 해주셨는데 이걸 어디서 가져오면 좋을까요?
2. 제목 및 내용 최대 글자 수 제한 관련
제목은 최대 20자, 내용은 최대 30자로 되어있는데 이걸 지금 flyway에 반영을 할까요 아니면 지금처럼 검증어노테이션으로만 막고 있을까요?
3. 소식지도 siteUserId로 받고 flyway에 fk추가만 하면 되는 것일까요?
4. 소식지 siteUserId 관련
저희가 처음 이야기할 때는 관리자도 소식지를 업로드할 수 있으니 mentorId말고 siteUserId로 받자라고 이야기를 했었습니다. 그런데 피그마를 보다보니 소식지 전체조회가 아닌 해당 멘토의 소식지 조회만 있는 거 같은데 그러면 이를 mentorId로 받는 것으로 수정해야할까요?
5. 권한 구분 관련
권한 구분을 유연하게 하기 위해 영서님이 security 관련 리팩토링을 완료하기 전에 임시로 RequireRoleAccess를 구현하였는데 괜찮은가요?
@RequireRoleAccess(roles = {Role.ADMIN, Role.MENTOR})이런 식으로 사용할 수 있게 일단 해놓았습니다.
6. 소식지 수정 및 삭제 권한
그동안 보통 해당 글을 올린 사람만 수정 및 삭제를 할 수 있게 커뮤니티 쪽이 구현되어있습니다. 혹시 관리자라면 그것과 상관없이 수정 및 삭제를 할 수 있어야할까요?
7. 썸네일 기본이미지로 변경하기
기본 이미지라는 요구사항이 추가되어서 수정 시 Boolean resetToDefaultImage라는 값을 추가하였는데 괜찮을까요? 추가로 이를 boolean으로 할지 Boolean으로 할지 고민이 되네요