Conversation
Test Results201 tests 201 ✅ 26s ⏱️ Results for commit c3684b0. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed71ea056b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| @PostMapping("/api/admin/notices") | ||
| public ResponseEntity<CreateNoticeResponse> createNotice( | ||
| HttpServletRequest request, | ||
| @Valid @RequestBody CreateNoticeRequest createNoticeRequest |
There was a problem hiding this comment.
Map bean validation failures to 4xx responses
Adding @Valid here makes invalid payloads throw MethodArgumentNotValidException, but GlobalExceptionHandler currently has no dedicated handler and its catch-all Exception path returns SERVER_ERROR (500). That means a bad notice create/update request is reported as a server fault instead of a client input error, which will break client-side error handling and monitoring by inflating 5xx rates for ordinary validation mistakes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
ㅇㅈ ExceptionHandler 추가하겠습니다
| @Size(max = 250) | ||
| String title, | ||
|
|
||
| @Size(max = 1000) | ||
| String content, |
There was a problem hiding this comment.
Reject blank notice fields on partial updates
The update DTO only enforces @Size, so values like "" or whitespace-only strings pass validation and are then persisted by Notice.update(...). This allows existing notices to end up with empty title/content even though creation explicitly requires non-blank fields, creating inconsistent data rules and potentially unreadable notices in admin/user views.
Useful? React with 👍 / 👎.
|
@haeyoon1 카테고리에 리뷰 추가했는데 한번 확인 부탁드립니다~ |
2Jin1031
left a comment
There was a problem hiding this comment.
고생하셨습니다~ 수민님!!
컨벤션 같은 코멘트만 몇개 달았어요~!!
| public boolean isDeleted() { | ||
| return isDeleted; | ||
| } | ||
|
|
There was a problem hiding this comment.
-
이번 변경 사항은 아니지만, 컨벤션으로 정립되면 좋을 것 같아서 남겨봅니다. 기존에 직접 작성된 getter 메서드를 어노테이션(
@Getter)으로 대체하는 건 어떨까요? 단순히 필드 값을 반환하는 getter는 별도로 관리할 필요가 없다고 생각해서요! -
soft delete 처리에
@SQLDelete어노테이션을 활용하는 방법도 있더라고요. JPA가DELETE쿼리를 가로채서 지정한UPDATE로 바꿔주는 방식이라, 개발자가delete()메서드 호출을 누락할 위험 없이 일괄 관리할 수 있어서 편리할 것 같습니다!
There was a problem hiding this comment.
이번 변경 사항은 아니지만, 컨벤션으로 정립되면 좋을 것 같아서 남겨봅니다. 기존에 직접 작성된 getter 메서드를 어노테이션(
@Getter)으로 대체하는 건 어떨까요? 단순히 필드 값을 반환하는 getter는 별도로 관리할 필요가 없다고 생각해서요
크롤러 쪽은 @Getter가 적용이 되어있습니다. 요쪽에서 컨벤션을 안지킨 것으로 볼 수 있겠네요. 반영해두었습니다~
soft delete 처리에 @SQLDelete 어노테이션을 활용하는 방법도 있더라고요. JPA가 DELETE 쿼리를 가로채서 지정한 UPDATE로 바꿔주는 방식이라, 개발자가 delete() 메서드 호출을 누락할 위험 없이 일괄 관리할 수 있어서 편리할 것 같습니다!
넘 좋은데요~? 요거는 근데 전체적으로 영향을 주는 친구이다보니 따로 pr로 올리면 좋을 것 같아서요, 이거 머지된 후에 바로 작업해서 올려도 될까요?
|
|
||
| @Query(""" | ||
| select n from Notice n | ||
| where n.isDeleted = false |
There was a problem hiding this comment.
이것도 @SQLRestriction("deleted_at IS NULL") 이걸 도메인 위의 어노테이션으로 붙이면서 까묵지 않고 처리할 수 있게 될 것 같아 제안드려봅니당
There was a problem hiding this comment.
이러면 조건을 항상 추가해줄 수 있군요?! 엔티티 전체적으로 반영하면 좋을 것 같아서 @SQLDelete과 함께 pr로 따로 묶어서 올리는것 어떨까요?
| private String content; | ||
|
|
||
| @Enumerated(EnumType.STRING) | ||
| private OperationType operationType; |
There was a problem hiding this comment.
@Column(nullable = false) 이것도 함께 추가하는 건 어떻게 생각하시나요??
enum으로 관리되고 있어서 null이 비지니스상 필요하진 않을 것 같아서요!
|
|
||
| @Transactional | ||
| public CreateNoticeResponse createNewNotice(CreateNoticeRequest createNoticeRequest) { | ||
| Notice notice = noticeRepository.save(Notice.of( |
There was a problem hiding this comment.
DTO 내부에서 도메인으로 변환하는 로직을 캡슐화한다면, 같은 로직이 layer 층에서 반복되더라도 변경 지점이 줄 것 같아 제안해봅니다~!
There was a problem hiding this comment.
해당 부분 이전에도 제안해주셨던 것으로 기억하는데요, DTO 내부에서 도메인을 생성하는게 과연 적절한지 쫌 의문입니다.
요청 DTO가 엔티티를 직접 생성하는 것은 계층이 흐려지는 것이라고 느껴집니다. DTO가 엔티티를 알고 있는 것이 변경 지점이 더 늘어나는 설계 아닌가요?!
엔티티 생성 시 요구사항 추가로 인해 로직이 추가된다거나,,의 경우에 DTO도 그 책임을 알게 되는 구조 아닌가요?
DTO마다 엔티티 생성 로직을 넣어두면 오히려 더 관리 리소스가 증가하는 것 아닌가 싶습니다. 저는 책임이 분산되는 것으로 느껴져서요.
결론은 DTO가 엔티티를 알게 되면 계층을 흐려지게 만드는 것 같은데, 진님의 제안에 대한 근거를 더 듣고싶습니다~
There was a problem hiding this comment.
제가 좀 끄적여봤는데, 상당히 길이가 길어서 읽기 리소스가 보통이 아니더라구요
AI 에게 정리 좀 해달라고 했습니다 ㅎ.ㅎ 슬쩍 한번 확인해주세여
DTO ↔ 도메인 변환 책임을 어디에 둘 것인가
배경
AdminNoticeService는 현재 Notice.of(title, content, operationType)처럼 원시 타입만 받아서 엔티티를 조립하고 있습니다. 도메인이 DTO를 import 하지 않는다는 점에서 의존성 관계는 보이지 않고 있습니다. 다만 한 가지 짚고 싶은 부분이 있어 코멘트로 남깁니다.
무엇이 신경 쓰이는가
이 코드에서 가장 자주 바뀌는 지점은 DTO의 필드, 그 다음이 엔티티의 필드라고 봅니다. 그런데 지금 구조에서는 그 변경이 다음과 같이 번집니다.
DTO에 필드가 하나 추가되면
Notice.of(...)의 시그니처가 바뀌고Notice.update(...)의 시그니처가 바뀌고AdminNoticeService의 호출부가 같이 수정됩니다.
Service는 "저장한다 / 갱신한다"만 알면 충분한데, 엔티티의 조립 시그니처를 매번 따라다녀야 하는 셈입니다. 캡슐화 관점에서도 도메인 필드들이 Service 본문에 그대로 나열되어 노출됩니다.
제안
DTO가 자기 자신을 도메인으로 변환하는 책임을 가지면 좋겠습니다.
public record CreateNoticeRequest(...) {
public Notice toEntity() {
return Notice.of(title, content, operationType);
}
}
public record UpdateNoticeRequest(...) {
public void applyTo(Notice notice) {
notice.update(title, content, operationType);
}
}public CreateNoticeResponse createNewNotice(CreateNoticeRequest request) {
Notice notice = noticeRepository.save(request.toEntity());
return CreateNoticeResponse.from(notice);
}
public UpdateNoticeResponse updateNotice(Long id, UpdateNoticeRequest request) {
Notice notice = noticeRepository.findActiveById(id)
.orElseThrow(() -> new AllcllException(AllcllErrorCode.NOTICE_NOT_FOUND, id));
request.applyTo(notice);
noticeRepository.flush();
return UpdateNoticeResponse.from(notice);
}이렇게 했을 때의 이점
- 변경의 영향 반경이 좁아진다. DTO 필드가 추가되어도 수정 지점은 DTO와 엔티티 두 곳으로 닫히고, Service는 diff에 등장하지 않습니다.
- Service가 조립 시그니처를 따라다니지 않는다. Service는 도메인에 어떤 필드가 있는지 알 필요가 없고, "저장 / 갱신"이라는 자기 책임만 표현하게 됩니다.
- 엔티티 안에서는 도메인 정책 코드가 더 잘 보인다.
of처럼 변환 로직 없이 필드를 그대로 받기만 하는 정적 팩토리가 계속 늘어나면 정작 중요한 정책 코드가 묻힙니다. 단순 매핑은 DTO 쪽에서 처리하는 편이 자연스럽습니다. - 도메인의 의존 방향은 그대로 단방향으로 유지된다.
Notice.from(dto)처럼 도메인이 DTO를 import 하게 만드는 방식과 달리, 의존은 여전히dto → domain한 방향입니다.
우려에 대한 답
엔티티 생성 시 요구사항 추가로 인해 로직이 추가된다면, DTO도 그 책임을 알게 되는 구조 아닌가?
지금은 Notice.of가 생성자만 호출해 그대로 반환하는 단순 매핑이라 이 우려는 발생하지 않습니다. 만약 이후에 도메인 정책(검증, 기본값 결정, 다른 객체 조회 등)이 필요해진다면 그 로직은 여전히 Notice 안의 정적 팩토리나 도메인 서비스로 들어가야 하고, DTO의 toEntity()는 그 정책 진입점을 호출만 하면 됩니다. 즉 단순 필드 매핑은 DTO가 책임지고, 도메인 정책은 도메인이 책임진다는 분리는 그대로 유지됩니다.
요약
- 가장 자주 바뀌는 건 DTO/엔티티 필드인데, 지금 구조에서는 그 변경이 Service까지 따라온다.
- DTO에
toEntity()/applyTo()를 두면 변경 지점이 DTO·엔티티 두 곳에 닫힌다. - 도메인은 여전히 DTO를 모르고, Service는 조립 절차를 모른다 — 각자의 책임만 표현하게 된다.
| import kr.allcll.backend.domain.operationPeriod.OperationType; | ||
|
|
||
| public record UpdateNoticeRequest( | ||
| @Size(max = 250) |
There was a problem hiding this comment.
CreateNoticeRequest에서는 @NotBlank로 DTO 측에서 한번 검증을 하고 들어온 것 같은데 update에서는 제외하신 이유가 있으실까요?? Notice.update 에서 null 검증을 하는 것보다 UpdateNoticeRequest에도 @NotBlank를 붙이면 DTO 단에서 검증이 끝나고, 도메인의 Notice.update에서는 null 체크를 뺄 수 있어 더 깔끔해질 것 같아서용
There was a problem hiding this comment.
PATCH 요청이라 수정하지 않은 필드는 보내지 않을 것으로 보고 있어서, 해당 값들은 null로 들어올 수 있다고 판단했습니다.
그래서 UpdateNoticeRequest에 @NotBlank를 적용하지 않았습니다~ 명세상 잘못된 요청이 아니기 때문에 Request에서 막지는 않고, update 시 유효한 값만 Notice에서 추가해주는 것이 자연스럽다고 생각했습니다.
어떻게 생각하시나요? 다른 방법이 좋아보이시나요?
There was a problem hiding this comment.
아,, 맞네요
update 시 미 업데이트인 null 데이터 ,, 이부분저도 고민했던 부분이었던 것 같습니다
나중에 더 엔티티가 커지면 관리 지점이 늘어나긴 ㅎㅏ겟지만 가능성이 크지 않은 것 같아서 지금 방향성이 좋은 것 같아요!
감사합니다!!
There was a problem hiding this comment.
진님께서 꼼꼼하게 리뷰해주셔서, 리뷰 할 부분이 많이 없네요! 진굿
간단한 코멘트 남겼습니다~!
수고많으셨어요 👍🏻
공지사항에 대해 어떤 공지사항인지 알려고 operationType을 씁니다.
따라서 전체 공지를 위해 모든 기능을 의미하는 ALL 필드를 추가했습니다. 어떠신가요? 의견 남겨주세요.
여기에서 고민한 부분이 다음과 같습니다.Notice가 사용하는 카테고리 구분이 기존 OperationType과 관심사가 다른가?
전체를 의미하는 OperationType을 추가하는게 맞나 고민했으나, Notice가 사용하는 카테고리도 원래 OperationType과 의미가 같다고 느껴졌습니다. 원래도 어드민쪽에서 사용했었잖아요?..,.
실제로 기능별 날짜 관리를 위해서 기능별로 카테고리를 나눈것이 OperationType이고, Notice도 기능별로 구분하려는거자나요. 그래서 공지사항도 해당 OperationType과 관심사가 같은거 아닌가 했습니다.
따라서 OperationType을 Notice가 동일하게 쓰는거 괜찮으신지 궁금하구용, ALL을 추가하는거 어떤지와, ALL 네이밍 어떤지 궁금해요~!
OperationType을 공지 카테고리에도 재사용하는 방향 굿이라고 생각합니다!
공지사항도 결국 기능 단위로 구분하려는 목적이니까, 기존 enum과 관심사가 완전히 다르지는 않다고 느꼈어용
오히려 별도 enum을 새로 만들기보다 기존 enum을 확장해서 사용하는 쪽이 더 단순하고, 이후 관리하기에도 편할 것 같아요.
공지사항 카테고리만 별도로 확장될 가능성보다는, 서비스 카테고리가 확장될 때 공지사항에서도 함께 쓰게 될 가능성이 더 높아 보여서요!
그리고 ALL 추가도 전체 공지를 표현하는 값으로 자연스럽다고 생각합니다~!
| entityManager.clear(); | ||
|
|
||
| // then | ||
| Notice deletedNotice = entityManager.find(Notice.class, notice.getId()); |
There was a problem hiding this comment.
이 테스트는 EntityManager 없이도 NoticeRepository만으로 충분히 검증 가능해 보입니다!!
findById()로 soft delete 상태를 확인하고, findActiveById()로 활성 조회에서 제외되는지만 봐도 의도한 정책은 충분히 검증될 것 같아서요!
어떻게 생각하시나용? 딱히 중요한 내용은 아니라 간단히 의견만 남겨주샤도 좋슴니다
There was a problem hiding this comment.
ㅋㅋㅋㅋㅋㅋㅋ코덱스를 너무 믿었나봅니다... 요 부분 꽤 별로네요
엔티티메니저 날려버렸습니다
| String title, | ||
| String content, | ||
| OperationType operationType, | ||
| LocalDateTime createdAt |
There was a problem hiding this comment.
이 부분은 공지 작성 시각보다는,
수정 이후의 상태를 보여주는 의미에서 공지 수정 시각 같은 정보를 내려주는 쪽이 더 자연스럽지 않을까 생각했습니당!
There was a problem hiding this comment.
흠...그러네요 BaseEntity에 updatedAt을 추가했는데 어떠신가요?
AdminNoticeService의 updateNotice메서드에서도 응답 반환 전 flush 호출로 중간 반영 시켜주어, response에 정확한 updatedAt이 담기도록 했습니다
(-> 추후 추가 작업사항: CrawlerBaseEntity에도 똑같이 만들어주기)
2Jin1031
left a comment
There was a problem hiding this comment.
코드 변경하신 것까지 모두 확인했습니다~~ 고생하셨습니다!!
aprrove 할게요!
goldm0ng
left a comment
There was a problem hiding this comment.
리뷰 반영 사항 및 추가 작업까지 모두 확인했습니다~!
어프룹입니다 고생하셨어요 :) 👍🏻
작업 내용
공지사항 관리 CRUD 기능을 구현했습니다.
수정기능에 대해
작성한 명세를 보니 PATCH 더라고요. 그래서 수정안한거는 null로 온다 생각하고 채워져 온 부분(title||content||operationType) 업데이트 하는 방향으로 구현했습니다.
OperationType에 대해
공지사항에 대해 어떤 공지사항인지 알려고 operationType을 씁니다.
따라서 전체 공지를 위해 모든 기능을 의미하는
ALL필드를 추가했습니다. 어떠신가요? 의견 남겨주세요.여기에서 고민한 부분이 다음과 같습니다.
ALL을 추가하는거 어떤지와, ALL 네이밍 어떤지 궁금해요~!@Valid추가요청 json이 잘못되어있으면 컨트롤러 진입시에 짤 해버릴라고 의존성 추가(
implementation 'org.springframework.boot:spring-boot-starter-validation')하고@Valid썼습니다. 괜찮은지 검토 부탁드려요.테스트에 대해
요거 Codex로 짜봤습니다.
단위테스트와 객체 협력 테스트 짜달라고해서 추가한 테스트입니다. 어때보이세요??
+)포스트맨 검증은 당연히 했구 1차 검토는 했습니다. 그냥 Codex가 테스트 어떻게 짜나 적용해봄
고민 지점과 리뷰 포인트
OperationType관련 코멘트