Board Service 구현 및 단위 테스트 #19
Conversation
- Adapter(Port 구현체)를 아직 구현하지 않았으므로, 빌드 에러 - 관련 에러로 인한 테스트 실행 불가
- annotationProcessor 관련이 없어, Lombok이 컴파일 타임에 정상적으로 동작하지 않는 문제가 발생함. - 05ab61 커밋에서 추가함으로써, Service에 Port를 주입이 가능해지고, bulid와 test가 정상적으로 동작할 수 있게 됨.
wch-os
added
in: board
|
Port와 UseCase의 메소드명 통일에 관련해서 의논을 해봐야할 것 같습니다!! |
merge-simpson
requested review from
wowddok99,
sweetykr7,
silberbullet,
taewookimm,
github-insu and
shin-jingyu
모든 사람을 Reviewer로 추가저번 정기 회의에서 우리는 이렇게 정했습니다:
@wch-os 님, @jilpoom 님은 참고 부탁드립니다.
|
merge-simpson
changed the title
|
@github-insu 님, PR에서 코드리뷰의 응답으로 'request changes'를 남겨 주세요. 코드리뷰 방식에 대해서는 일단 해 보면서 개선책을 고민해 보면 좋을 것 같습니다.
필요하다면, 일부는 주관적인 생각입니다만 참고 부탁드리겠습니다. |
리뷰 감사합니다! 물론 Service에서도 구현할 수 있지만, 조회 후 상태 처리를 하는 것보다 |
아이디어 보충두 분의 주장이 다음처럼 상충하는 것으로 보입니다.
두 분 모두 좋은 아이디어와 관점 같습니다. 👍 필터할 Status를 서비스 레이어에서 제공하는 이유개발자가 구현하려고 하는 직접적인 '의도'에 상응하는 로직은 애플리케이션 레이어에 표현됩니다. 이것이 도메인 중심적으로 구성되는 애플리케이션 계층의 역할이기도 합니다. 따라서 데이터베이스로부터 무엇을 갖고 오고 싶은지 '결정하는 의도'는 애플리케이션 레이어에, 의도대로 데이터를 제공하는 역할은 데이터베이스 연결용 어댑터에 구현될 수 있습니다. 즉, 애플리케이션 레이어가 "ACTIVE", "SUSPENDED", "REMOVED" 상태인 데이터를 제공해 달라고 어댑터에 요청하면, 데이터베이스 연결용 어댑터는 그 상태에 맞추어 제공하고, 애플리케이션 레이어가 "ACTIVE", "SUSPENDED" 상태인 데이터 목록만 달라고 요청하면, 데이터베이스 연결용 어댑터는 그 상태에 맞추어 제공합니다. 어댑터의 제한적 역할어댑터는 PC에 빗대자면, 모니터, 키보드, 마우스 등입니다. 이처럼 외부(키보드, 마우스 등)가 무언가를 의도를 갖고 결정하지 않도록 구분하는 것이 우리가 헥사고날 아키텍처를 구현할 때 학습하게 되는 내용 중 하나입니다. 추가적인 피드백이나 아이디어추가적으로 논의나 설명이 필요한 부분은 자유롭게들 말씀해 주세요! 좋은 논의를 시작해 주셔서 감사합니다, @wch-os 님, @github-insu 님. 👍 |
|
@merge-simpson 님 소중한 코멘트 감사드립니다!🙇🏻♂️ 말씀주신 것처럼, 애플리케이션에 계층에서 Port-Adpater 계층에 ‘의도’를 구체적으로 전달하는 것이 바람직한 설계가 될 것 같습니다. '의도'를 구체적으로 전달하는 방법으로 설계 시, 애플리케이션 계층에서 BoardStatus(의도)를 전달하고, Port-Adpater 계층에서 해당 의도에 맞는 필터 쿼리로 변환하여 조회하는 흐름으로 이해됩니다. 이러한 흐름은 “각 BoardStatus에 따른 Board 리스트 조회” 비즈니스 로직으로 보이며, 일반적인 요구사항에 맞추어 다음의 비즈니스 로직도 추가적으로 작성할 수 있을 것 같아 보입니다.
혹시 잘못 이해하고 있는 부분이 있다면 말씀해 주시면 감사하겠습니다! |
|
@sweetykr7 @wowddok99 님 BoardQueryPort에서 findByStatusesList가 기존의 findAll 기능까지 포함할 수 있어, findAll을 사용하지 않는 방향으로 수정하였습니다. 이와 관련하여 반영해주시면 감사하겠습니다! |
There was a problem hiding this comment.
@wch-os 님, @jilpoom 님 확인 부탁드립니다!
❗ 게시판 목록 조회 시, BoardStatus는 List가 아닌 Set 자료구조가 타당하다고 생각합니다.
현재 BoardReadByStatusesUseCase.java에는 다음과 같이 정의되어 있습니다:
@Override
public Page<Board> findByStatuses(Pageable pageable, List<BoardStatus> statuses) {
return boardQueryPort.findByStatusesList(pageable, statuses);
}이 구현은 클라이언트로부터 아래와 같은 요청을 허용하는 셈입니다:
statuses = [ ACTIVE, ACTIVE, ACTIVE, SUSPENDED, SUSPENDED, PENDING ]결과적으로 DBMS에 IN 절 안에 중복된 목록이 전달되더라도, DBMS는 최적화를 통해 내부적으로 중복을 제거합니다. 결과만 보면 별다른 문제가 없어 보일 수 있습니다. 🤔
하지만 중복을 제거하는 것도 비용이 드는 과정입니다. 처음부터 중복 제거 된 목록을 준다면 불필요한 비용이 발생하지 않는 더 올바른 입출력이 가능해지지 않을까요?
만약 게시판 목록 조회를 담당하는 DBMS 혹은 NoSQL가 중복 제거 로직을 따로 요구하면 어떻게 될까요?
결국 서비스 로직 내에서 중복을 제거하는 불필요한 코딩이 발생할 것 입니다.
이 모든 걸 고려한다면 Set 사용은 더 나은 설계 선택이라고 생각합니다.
물론 클라이언트에서 중복된 목록이 와도 DTO 에서는 Set 자료구조 덕분에 중복 없는 BoardStatus가 만들어집니다. 또한 시간복잡도도 평균적으로 O(1)이기 때문에 성능에도 지장이 없어보입니다. 👍
merge-simpson
added
type: feature
25/01/15 라이브 코드 리뷰 결과@wch-os 님, @jilpoom 님, 작업하시느라 수고하셨습니다 : ) 특히 유스케이스, 포트, 서비스 레이어의 구조가 명확하고 일관성 있게 잘 설계되어 인상적이었습니다. 이러한 체계적인 설계 덕분에 저희 driven-query 쪽에서도 해당 구조를 참고하여 어댑터 구현 시 편하게 작업할 수 있었습니다!! 🥰 아래 내용은 정기회의에서 진행한 라이브 코드 리뷰 내용입니다. Hard Delete/Soft Delete를 애플리케이션 계층에서 제어Hard Delete vs Soft Delete 여부를 Application 계층에서 제어하는 것이 좋아 보입니다.
createdAt 파라미터의 안전성 문제
Read Model 다루기
ReadModel 관련 덧붙임
Parameter Type 변경 제안(Set vs. Collection)필터 조건인 Status 목록의 스펙에 대해서입니다.
Collection을 선택한다면 그 이유 애플리케이션 레이어는 다른 계층에 공유된 스펙이므로 추상화 수준이 높은 것이 유리할 때가 많습니다.
Set을 선택한다면 그 이유 애플리케이션 레이어에서 Set 타입의 기능을 사용한다면 파라미터를 미리 Set으로 제한할 수 있습니다.
Query Port에서 Pageable 파라미터를 마지막 순서로 입력
UseCase를 너무 나누지 않고 모아 두는 것에 대한 논의
|
…yStatusesUseCase`
…in `BoardReadByStatusesUseCase`
논의 사항 반영 중 문의모두들 깔끔한 리뷰 감사합니다! 라이브 코드 리뷰를 반영하던 도중 다음과 같은 의문점이 생겼습니다. UseCase와 Port의 메소드명 통일 관련
이에 대한 저의 의견은, 묵시적으로 메소드명(ex) |
Origin
Reply: 다음에 새로 논의@jilpoom 님, 좋은 논의 감사드립니다! 👍 |
There was a problem hiding this comment.
👍 Board 도메인 및 애플리케이션 계층의 까다로운 작업을 완수해 주셔서 감사드립니다!
@wch-os 님, @jilpoom 님!
애플리케이션 및 도메인 계층은 해당 도메인 서비스의 중심이 되며 호환성을 유지해야 하는 까다로운 계층입니다!
각 어댑터를 작업하는 여러 페어와 소통하며 적절한 요건을 충족하기 위해 많은 논의와 수정을 거쳤던 것 같습니다.
잘 완수해 주셔서 정말 감사드립니다. 👍
p.s. 첨부한 수정 사항 및 제안 목록은 추후 새로운 이슈로 작성하고, 이번에는 병합하기에 무리가 없어 보입니다! 감사합니다.
| 🚀 | 🏅 Approve! |
| BoardStatus status, | ||
| Instant createdAt, | ||
| Instant updatedAt, | ||
| Instant deletedAt |
There was a problem hiding this comment.
❗ deletedAt 필드는 제외하기로 하였습니다.
조회 모델에 추가하여도 무시됩니다.
따라서 오해가 없도록 삭제하는 것이 좋습니다.
There was a problem hiding this comment.
❗ status 필드는 목록 조회에서도 필요해 보입니다.
목록 조회에 사용되는 Summary는 status 필드를 포함하는 것이 좋아 보입니다.
| Board create(Board board); | ||
|
|
||
| Board update(Board board); | ||
|
|
||
| void delete(Long id); | ||
|
|
||
| void delete(Board id); |
There was a problem hiding this comment.
💬 아이디 타입 대신 도메인 모델 객체를 넘기는 이유가 설명되면 좋겠습니다.
기존 라이브 코드리뷰 코멘트를 참고하여 배경이 되는 다음 내용을 숙지하시고, 혹시나 이 외에 추가적인 이유로 Board 객체를 넘겨야 하는지 살펴 보면 좋겠습니다!
- Adapter에서는 비즈니스로직의 '의도'를 담지 않습니다. (Board 객체가 ~할 때만 삭제할 수 있는 등)
- Adapter에 삭제를 명령하면, 어댑터는 순수하게 삭제를 수행해야 합니다.
- Soft Delete는 Adapter에서 수행하지 않고,
delete메서드가 실제 하드 딜리트를 뜻해야 합니다. - Soft Delete용 메서드는 새로 추가되어야 합니다. (status 업데이트용 메서드)
관련 논의
같은 이슈의 이 코멘트에 담겨 있습니다.
|
|
||
| Optional<Board> findById(Long id); | ||
| Page<BoardReadSummaryModel> findByStatusesList(Pageable pageable, Set<BoardStatus> statuses); |
There was a problem hiding this comment.
💊 일반적인 파라미터 입력 순서에 맞게 statuses, pageable 순서가 어떨까 합니다.
Port는 반드시 Spring Data JPA의 스펙을 준수할 필요가 없지만, 일반적으로 사람들에게 익숙한 관례에 맞게 순서를 바꾸는 것이 좋다고 생각합니다.
그 외 메서드 선언 순서, read model 등의 적용은 잘되어 있어서 잘 신경써 주신 것 같습니다! 👍
There was a problem hiding this comment.
💊 클래스 이름을 BoardDetailReadModel 또는 BoardDetail로 제안드립니다.
클래스 이름에서 변하는 부분과 반복되는 부분의 배치를 다음 패턴으로 권하고 있습니다.
- {바뀌는 부분} + {바뀌지 않는 부분}
- ex) BoardDetail + ReadModel
- {바뀌는 부분 1} + {바뀌지 않는 부분} + {바뀌는 부분 2}
- ex) Board + ReadModel + Detail
지금 클래스 이름은 {바뀌는 부분} + {바뀌지 않는 부분} + {바뀌는 부분} + {바뀌지 않는 부분}으로, 클래스 이름을 인식할 때 다소간 혼란이 될 수도 있다고 생각합니다.
또는 Summary, Detail 등 자주 사용되는 조회 모델에서 -ReadModel 접두사 제거를 고려할 수 있습니다.
Summary와 Detail은 자주 사용되는 관례로 인식하여 이름에서 조회 모델임을 알아볼 수도 있다고 생각합니다.
따라서 Application 레이어에서는 매번 이름에 -ReadModel 접두사를 붙이지 않는 것을 고려할 수 있습니다.
단, 다른 도메인 모델이 -Summary, -Detail로 명명되는 케이스와 네이밍에서 중복될 여지를 고려해야 합니다.
There was a problem hiding this comment.
💊 클래스 이름을 BoardSummary 또는 BoardSummaryReadModel로 제안드립니다.
이유는 위 BoardDetail 명명 제안의 이유와 같습니다.
|
|
||
| board.softDelete(); | ||
|
|
||
| boardCommandPort.delete(board); |
There was a problem hiding this comment.
❗ Soft Delete로 변경하는 것이 좋습니다.
포트 및 어댑터의 delete 함수는 실제로 함수의 이름이 지시하는 대로, '삭제' 명령을 수행하기로 결정하였습니다. 즉, Repository Port 및 Adapter의 delete 함수는 Hard Delete를 지시합니다. 마치 JPA Repository의 delete 함수와 동일한 동작으로 이해하고 사용해야 합니다. 이름이 그렇게 시키고 있기 때문이며, 애플리케이션의 기획을 '어댑터에서 자의적으로 해석하여' 소프트 딜리트로 처리해서는 안 된다는 관점입니다.
'기획상' 또는 '서버의 정책상' 소프트 딜리트를 수행할지 여부는 애플리케이션 계층에서 결정하는 것이 좋다는 논의가 있었습니다.
라이브 코드리뷰 내용을 참고하세요!
src/test/kotlin/me/nettee/board/application/service/BoardCommandServiceTest.kt
Show resolved
Hide resolved
src/test/kotlin/me/nettee/board/application/service/BoardQueryServiceTest.kt
Show resolved
Hide resolved
merge-simpson
removed request for
wowddok99,
jilpoom,
sweetykr7,
taewookimm and
github-insu
merge-simpson
added
review: provided
Pull Request
Issues
Resolves #18
Description
How Has This Been Tested?