feat: 소식지 서비스 구현 #337
feat: 소식지 서비스 구현 #337
Conversation
- 누락된 @transactional 추가
- 존재하지 않는 id 관련 테스트 삭제
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 (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
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: 8
🧹 Nitpick comments (17)
src/main/java/com/example/solidconnection/s3/domain/ImgType.java (1)
7-7: NEWS 상수 추가 👍
- 새로운
NEWS타입이 깔끔하게 추가되었습니다.- 다만, enum 값이 도메인별·사전식 정렬 규칙 없이 나열되어 있어 이후 충돌 시 가독성이 떨어질 수 있습니다.
COMMUNITY,GPA,LANGUAGE_TEST,NEWS,PROFILE순으로 정렬하는 방안을 검토해 보시면 어떨까요?src/main/java/com/example/solidconnection/news/dto/NewsCommandResponse.java (1)
5-13: DTO 필드 null-safety 및 확장성 고려
id가 영속화 직후에는 항상 존재하므로 원시 타입long으로도 문제는 없지만, 추후 비동기 생성(예: 이벤트 큐)처럼 아직 PK가 없는 상태를 표현해야 할 때를 대비해Long(wrapper)로 두면 널 안전성이 올라갑니다.- 멤버가 하나뿐이라도
"Response"네이밍에는 추가 정보가 붙을 가능성이 높으니, record 대신 클래스로 바꿔도 무게 차이가 거의 없다는 점도 참고해 주세요.-public record NewsCommandResponse( - long id -) { +public record NewsCommandResponse( + Long id +) {src/test/java/com/example/solidconnection/news/fixture/NewsFixture.java (1)
13-20: Builder 상태 초기화 누락 주의
NewsFixtureBuilder가 싱글톤@TestComponent로 주입되면, 필드 값이 이전 테스트의 상태를 물려받을 수 있습니다.- 각 테스트마다
new NewsFixtureBuilder()를 생성하거나,create()이후 내부 필드를null로 리셋해 재사용 안전성을 확보해 주세요.src/main/java/com/example/solidconnection/news/dto/NewsResponse.java (1)
5-10: 필드 명명 간결화 제안
newsItemsResponseList는 정보가 중복되어 길어집니다. 예:items,newsItems정도로 줄이면 직관적이고 JSON 프로퍼티도 간결해집니다.- 스프링
@JsonProperty로 외부 노출명만 별도로 지정할 수도 있으니 참고해 주세요.-public record NewsResponse( - List<NewsItemResponse> newsItemsResponseList -) { - public static NewsResponse from(List<NewsItemResponse> newsItemsResponseList) { - return new NewsResponse(newsItemsResponseList); +public record NewsResponse( + List<NewsItemResponse> items +) { + public static NewsResponse from(List<NewsItemResponse> items) { + return new NewsResponse(items);src/main/java/com/example/solidconnection/news/dto/NewsCreateRequest.java (2)
16-18: URL 형식 검증이 빠져 있습니다
@NotBlank및@Size만으로는 URL 유효성이 보장되지 않습니다.@URL(Hibernate Validator)이나@Pattern을 추가해 실제 URL 형식을 검증해 주세요.
20-27: DTO 내부에서 도메인 생성 책임을 분리할지 검토해 주세요
toEntity()가 도메인 객체 생성을 담당하면, 도메인 생성 규칙이 DTO에 스며들 수 있습니다. 서비스 레이어로 책임을 이동하거나, 팩토리 클래스를 별도로 두는 것도 고려해 보세요.src/main/java/com/example/solidconnection/news/dto/NewsFindResponse.java (1)
7-15: 필요시 Swagger/Json 직렬화 어노테이션을 추가해 주세요
외부 API 규격 문서화나 필드명 매핑이 필요하다면@Schema또는@JsonProperty를 추가하는 편이 좋습니다.src/test/java/com/example/solidconnection/news/service/NewsQueryServiceTest.java (1)
40-44: 정렬 검증이 느슨합니다
updatedAt가 모두 동일하면 현재 검증 로직은 통과합니다. 차이를 보장하려면 각 뉴스의updatedAt을 의도적으로 변경하거나, 정렬 결과를 id 또는 createdAt과 교차 검증해 주세요.src/main/java/com/example/solidconnection/news/domain/News.java (1)
44-58: 2. updateXXX 메서드 체이닝·반환값 고려
각 update 메서드가 void 이라 호출부가 중첩될 때 가독성이 떨어집니다.return this;형태로 체이닝을 지원하거나, 단일update메서드로 묶으면 서비스 코드가 한층 간결해집니다.src/main/java/com/example/solidconnection/news/service/NewsQueryService.java (2)
25-30: 1. 전체 목록 조회에 페이징 옵션 추가 고려
findAllByOrderByUpdatedAtDesc()로 모든 행을 한 번에 가져오면 데이터가 많을 때 OOM·응답지연이 발생할 수 있습니다.Pageable파라미터를 받아 Slice/Page 로 내려주는 방향을 권장드립니다.
32-37: 2. 단건 조회 시 존재 여부 검증 OK, 하지만 캐싱도 검토해볼 만
조회 비중이 잦은 콘텐츠라면@Cacheable등을 붙여 부하를 줄일 수 있습니다. 필요성만 체크해 주세요.src/main/java/com/example/solidconnection/news/service/NewsCommandService.java (1)
77-84: 4. 빈 제목 검증 로직에서 trim 후 length 체크 순서
title.trim().isEmpty()후 length 비교는 trim 결과가 아닌 원본 길이를 봅니다.trimTitle.length()로 측정하는 편이 자연스럽습니다.src/test/java/com/example/solidconnection/news/service/NewsCommandServiceTest.java (1)
148-151: 1. 예외 검증 시 ErrorCode 비교 권장
메시지 문자열은 i18n·문구 변경에 취약합니다.assertThatThrownBy(...).hasFieldOrPropertyWithValue("errorCode", NEWS_URL_INVALID)식으로 코드 자체를 검증하면 안정적입니다.src/main/java/com/example/solidconnection/news/controller/NewsController.java (4)
34-39: ① 목록 조회에 페이지네이션·정렬 옵션 추가 고려
한 줄 요약: 현재는 전체 리스트를 한 번에 반환합니다.
- 탐색 결과가 많아질 경우 응답 본문이 비대해집니다.
→ Pageable (Slice/Page) 과 정렬 파라미터를 받아 DB 부하·네트워크 트래픽을 줄이는 방안을 권장합니다.- API 스펙이 고정되기 전에 도입하면 클라이언트·서버가 모두 유연해집니다.
41-48: ②@AuthorizedUser매개변수 미사용
siteUser를 주입만 하고 로직에서 사용하지 않습니다.
- 미사용 파라미터는 읽는 사람에게 혼란을 줍니다.
→ 메서드 내부 또는 AOP 단에서 활용될 계획이 없다면 제거를 검토해주세요.- AOP 용도로 반드시 필요하다면 주석으로 용도를 명시해 두는 것이 가독성에 도움이 됩니다.
61-79: ④ PATCH 파라미터 전달 방식 재검토
한 줄 요약:@RequestParam개별 필드 vs DTO.
- 필드 수가 늘어나면 시그니처가 길어져 유지보수가 어려워집니다.
→NewsUpdateRequestDTO를@RequestPart또는 JSON Body로 받는 쪽이 깔끔합니다.- 현재 파일 파라미터는
@RequestParam으로 선언되어 있는데, 실제 multipart 전송 시에는@RequestPart가 명시적인 표현입니다.- 검증 로직이 서비스 레이어로 흩어지지 않도록, DTO에
@Valid제약을 걸어 컨트롤러 단계에서 1차 검증을 마치는 편이 좋습니다.
82-89: ⑤ 삭제 응답 코드 204 No Content 검토
현재 삭제 후 200 OK와 본문을 함께 돌려주고 있습니다.
- 삭제 결과가 단순 성공/실패라면 본문이 굳이 필요 없으므로 204 No Content가 더 간결합니다.
- 만약 삭제 후에도 CommandResponse가 꼭 필요하다면, 지금 구현을 유지하되 이유를 주석으로 남기면 명확합니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/main/java/com/example/solidconnection/common/exception/ErrorCode.java(3 hunks)src/main/java/com/example/solidconnection/news/controller/NewsController.java(1 hunks)src/main/java/com/example/solidconnection/news/domain/News.java(1 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/NewsFindResponse.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/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/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)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/test/java/com/example/solidconnection/news/fixture/NewsFixture.java (1)
src/test/java/com/example/solidconnection/news/fixture/NewsFixtureBuilder.java (1)
TestComponent(8-47)
src/test/java/com/example/solidconnection/news/fixture/NewsFixtureBuilder.java (1)
src/test/java/com/example/solidconnection/news/fixture/NewsFixture.java (1)
TestComponent(7-21)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
src/main/java/com/example/solidconnection/news/dto/NewsItemResponse.java (1)
7-14: 깔끔한 DTO 정의, 유지보수 관점에서도 무리 없음
간결한 record 사용과 필드 구성,from()팩토리 메서드까지 모두 일관성 있게 잘 작성되었습니다. 별다른 우려 사항 없습니다.src/main/java/com/example/solidconnection/news/service/NewsCommandService.java (1)
27-30: 1. DESCRIPTION 길이 상수 255 ↔ 메시지 500 불일치
상수와 UI 안내가 어긋나 혼란을 야기합니다. 어떤 길이가 진실인지 확정 후 둘 중 하나를 수정하세요.
src/main/java/com/example/solidconnection/news/repository/NewsRepository.java
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/news/fixture/NewsFixtureBuilder.java
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/common/exception/ErrorCode.java
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/common/exception/ErrorCode.java
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/news/service/NewsCommandService.java
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/news/service/NewsCommandService.java
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.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/com/example/solidconnection/news/service/NewsCommandService.java (1)
35-40: 🛠️ Refactor suggestion입력값 검증이 누락되었습니다
createNews메서드는updateNews와 달리 title, description, url에 대한 검증을 수행하지 않고 있어요. 동일한 제약 조건을 적용하지 않으면 데이터 일관성이 깨질 수 있습니다.다음과 같이 검증 로직을 추가하는 것을 권장합니다:
@Transactional public NewsCommandResponse createNews(NewsCreateRequest newsCreateRequest, MultipartFile imageFile) { + validateTitle(newsCreateRequest.title()); + validateDescription(newsCreateRequest.description()); + validateUrl(newsCreateRequest.url()); UploadedFileUrlResponse uploadedFile = s3Service.uploadFile(imageFile, ImgType.NEWS); News news = newsCreateRequest.toEntity(uploadedFile.fileUrl()); News savedNews = newsRepository.save(news); return NewsCommandResponse.from(savedNews); }
🧹 Nitpick comments (2)
src/main/java/com/example/solidconnection/news/service/NewsCommandService.java (2)
68-75: S3 삭제 실패 시 처리 방안을 고려해보세요현재 구조에서는 S3 이미지 삭제가 실패하면 전체 트랜잭션이 롤백되어 뉴스 삭제가 취소됩니다. 이는 S3 서비스 장애 시 뉴스를 삭제할 수 없게 만들 수 있어요.
몇 가지 개선 방안을 제안합니다:
- S3 삭제를 try-catch로 감싸고 로그만 남기기 - 이미지는 나중에 정리
- 비동기 이벤트 방식 - 뉴스 삭제 후 이벤트를 발행하여 별도로 처리
- 소프트 삭제 - 뉴스를 논리적으로만 삭제하고 배치로 정리
현재 요구사항에 따라 적절한 방식을 선택하시면 좋을 것 같습니다.
92-99: URL 검증 로직 개선을 고려해보세요현재 URL 검증은 프로토콜만 확인하고 있어요. 더 엄격한 검증을 위해 다음을 고려해보세요:
private void validateUrl(String url) { - if (!url.matches("^https?://.*")) { - throw new CustomException(NEWS_URL_INVALID); - } + try { + URL urlObj = new URL(url); + if (!urlObj.getProtocol().matches("https?")) { + throw new CustomException(NEWS_URL_INVALID); + } + } catch (MalformedURLException e) { + throw new CustomException(NEWS_URL_INVALID); + } if (url.length() > MAX_URL_LENGTH) { throw new CustomException(NEWS_URL_TOO_LONG); } }또는 더 정교한 정규식을 사용할 수도 있습니다:
"^https?://[\\w\\-._~:/?#\\[\\]@!$&'()*+,;=]+$"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/example/solidconnection/common/exception/ErrorCode.java(3 hunks)src/main/java/com/example/solidconnection/news/service/NewsCommandService.java(1 hunks)src/test/java/com/example/solidconnection/news/service/NewsCommandServiceTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/test/java/com/example/solidconnection/news/service/NewsCommandServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/example/solidconnection/common/exception/ErrorCode.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
src/main/java/com/example/solidconnection/news/service/NewsCommandService.java
Show resolved
Hide resolved
nayonsoso
left a comment
nayonsoso
left a comment
There was a problem hiding this comment.
정말 아름다운 코드입니다 🥹✨
고생하셨습니다!
0.
소식지 목록 조회는 일단 로그인 안해도 조회되게 했는데 어떻게 하면 될까요?
저도 소식지는 로그인 안해도 조회되는게 맞는 것 같아요!
1. PATCH 구현 방식
어차피 MultipartFile은 @RequestParam으로 받아야 할 것이고,..
그걸 제외한 나머지 (title, description, url)를 하나로 묶는건 어떤가요?
하나로 묶는게 좋겠다 생각한 이유는, 이들이 의미상 하나의 덩어리이기도 하고
String 파라미터가 연속으로 있으면 헷갈리기 쉽기 때문입니다.
(엑세스 토큰, 리프레시 토큰의 악몽이 생각나네요)
그리고 "몇 개의 파라미터 이상일 때 DTO를 쓸지"는 그때그때 많이 달라질 것 같아서 당장은 안 정하고 넘어가도 괜칞을 것 같습니다!
2. DTO 변수명 통일
크게 어색하게 느껴지는 부분은 없었어요 🙂
3. 로그인 유저 확인 방법
지금은 @Authorizeduser 를 파라미터로 받아야지만 검증할 수 있는 게 맞나요?
네 맞습니다
추가로 어드민인지 검증하는 AOP를 사용하려면 사실 파라미터에 SiteUser가 있어야합니다 ㅎㅎ..
어떤 맥락에서 규혁님이 이걸 말씀하셨는지 추측을 해보자면(?) 어드민 aop에는 siteUser가 파리미터로 필요하니 변경할 때 참고하라는 말씀이 맞나요? aop로 관심사를 분리해놨으니, 변경할 때도 쉽게 바꿀 수 있을 것 같아요😄
4. 존재하지 않는 id로 조회하는 경우
이 경우는 중복 테스트코드가 너무 많이 발생하여 제외하였습니다!
👍👍
5. 파라미터 관련 개행
전 회의 때 제가 파라미터 3개 초과 시 개행을 하자고 하긴 했는데...ㅎㅎ 좀 비효율적인 거 같다는 생각도 드네요 😅
ㅋㅋㅋ 이런 부분 말인가요?
NewsCommandResponse newsCommandResponse = newsCommandService.updateNews(
newsId,
title,
description,
url,
imageFile
);저는 크게 이상하게 느껴지지 않는 것 같아요~
[1번의 Patch를 Dto로 묶어서 받게 하기]를 반영하면 변경 사항이 많아질 것 같아서, 일단은 approve가 아니라 그냥 comment로 답니다!
| assertThat(savedNews.getTitle()).isEqualTo(expectedTitle); | ||
| assertThat(savedNews.getDescription()).isEqualTo(expectedDescription); | ||
| assertThat(savedNews.getThumbnailUrl()).isEqualTo(expectedNewImageUrl); | ||
| assertThat(savedNews.getUrl()).isEqualTo(expectedUrl); |
1. PATCH 구현 방식넵, 좋은 거 같습니다! 반영했는데 빈 값일 때와 같은 검증같은 걸 Service에서 검증하는 건 괜찮은가요? 3. 로그인 유저 확인 방법
이걸 어떻게 이해하셨을까요 ㅎㅎ... 맞습니다! 5. 파라미터 관련 개행맞습니다 ㅎㅎ.. 괜찮다니 다행이네요! |
whqtker
left a comment
whqtker
left a comment
There was a problem hiding this comment.
고생하셨습니다! 저에게 다소 어색했던 어노테이션이 있었는데, 이번 기회에 하나 배워가서 유익했습니다 ㅎㅎ 좋은 코드 감사드립니다.
하나 궁금한 점이 있는데, title, description은 NewsXXXRequest 에서 @NotBlank, @Size 어노테이션으로 파라미터 검증, NewsCommandService에서 validateXXX 로 중복 검증이 되는 것 같습니다.
"여러 번 검증해서 나쁠 건 없으니 그냥 냅두자" vs. "그래도 중복 코드이고 상수값 변경 등으로 유지보수 시 어려움이 있을 듯 하니 수정하자" 중 어느 의견을 선택하는 것이 좋을지 고민이 됩니다. 이 점에 대해 어떻게 생각하시나요?
3 participants
관련 이슈
작업 내용
소식지 관련 crud를 구현하였습니다!
커밋 순서를 따라가면 이해하기 편할 거 같아요!
특이 사항
소식지 목록 조회는 일단 로그인 안해도 조회되게 했는데 어떻게 하면 될까요?
리뷰 요구사항 (선택)
1. PATCH 구현 방식 고민
PATCH를 의미에 맞게 사용하려면 필요한 값만 보내야하니
이런식으로 구현하게되었는데 이게 적절한 방식인지 고민이 되네요 🥲 이렇게 하다보니 검증도 결국 서비스로직에서 할 수밖에 없게 되더라구요
그냥 DTO를 활용해서 body로 받아야할지 의견을 듣고싶습니다!
만약 이 방식으로 간다면 몇 개의 파라미터 이상?일 때 DTO를 쓸지도 고민이 됩니다!
2. DTO 변수명 통일
우선 기존의 구현되어있던 DTO명을 최대한 따라하려고했는데 이전에 계시던 분과 영서님과의 컨벤션도 조금씩 다른 거 같더라구요! 혹시 이상한 부분이 있다면 말씀해주시면 감사하겠습니다!
3. 로그인 유저 확인 방법
지금은 @Authorizeduser SiteUser siteUser 를 파라미터로 받아야지만 검증할 수 있는 게 맞나요? 우선은 그렇게 구현을 했습니다! 추가로 어드민인지 검증하는 AOP를 사용하려면 사실 파라미터에 SiteUser가 있어야합니다 ㅎㅎ..
4. 존재하지 않는 id로 조회하는 경우
이 경우는 중복 테스트코드가 너무 많이 발생하여 제외하였습니다!
5. 파라미터 관련 개행
전 회의 때 제가 파라미터 3개 초과 시 개행을 하자고 하긴 했는데...ㅎㅎ 좀 비효율적인 거 같다는 생각도 드네요 😅