[2단계 - 웹 자동차 경주] 제이미(임정수) 미션 제출합니다. #183
Conversation
ResponseEntity -> 객체
테이블이 이미 존재한다면 Drop 하도록 함
2단계 미션에 대한 기능 목록 추가
dao -> repository dto 패키지 domain과 분리
| @PostMapping("plays") | ||
| public GameResultDto createGame(@RequestBody GameInforamtionDto gameInforamtionDto) { | ||
| return racingCarService.play(gameInforamtionDto, numberGenerator); | ||
| } |
There was a problem hiding this comment.
ResponseEntity가 아닌 객체를 바로 반환하도록 함
리뷰 답변 참고
| DROP TABLE IF EXISTS RACING_CARS; | ||
| DROP TABLE IF EXISTS RESULTS; |
There was a problem hiding this comment.
테이블이 존재한다면 DROP하도록 함
리뷰 답변 참고
| String carNames = inputView.inputCarNames(); | ||
| return racingCarService.getCars(carNames); |
There was a problem hiding this comment.
콘솔과 웹 게임의 중복 코드를 제거해 주기 위해 ConsoleController에서 사용하는 메서드들을 public으로 바꿔주었습니다.
이와 같은 방법이 맞는 것인지 아직 감이 오지 않는 상태입니다.
카프카의 의견이 궁금합니다...!
There was a problem hiding this comment.
사실 보통의 상황이라면 이렇게 콘솔과 웹을 동시에 구현하지는 않을듯해요. 그러나 미션 요구사항이므로...특수한 상황임을 감안하면 이해가 되는 부분입니다.
일반적인 구조라면, Controller는 요청에 대한 라우팅만 수행하는 것이 맞기는 합니다. 위의 runGame 메소드같은 경우도, 요청이 들어올때 Service에서 수행해줘야 하지요. 일단 RacingCarWebController에서 사양에 맞게 잘 구현되어 있으므로, 이 부분에 대해서는 감안하겠습니다.
There was a problem hiding this comment.
RacingCarConsoleController 클래스의 runGame()에 존재하는 로직은 모두 View를 위한 로직인데 그래도 Service에 있어야 하는 것일까요?
There was a problem hiding this comment.
View를 위한 로직이라는 말이 참 애매한데... 현재 보면 컨트롤러가 View 레이어와 강한 의존을 가지고 있다고 느껴져요.
만약 완벽하게 리팩토링을 하고 싶다면, outputview와 inputview를 json DTO를 통해 통신하도록 완벽히 구현해서 WebController와 동일한 서비스를 사용하도록 해볼 수 있어 보이긴 합니다. 다만 이미 WebController 부분의 구현에서 해당 스펙을 구현했으니, 이번 미션에서 그 정도까지의 작업을 할 필요는 없다고 판단했습니다.
MVC 구조에서 서비스가 하는 역할에 대해 잘 이해하고 있다면, 크게 문제가 되어 보이지는 않습니다.
| public GameResultDto createGame(@RequestBody GameInforamtionDto gameInforamtionDto) { | ||
| return racingCarService.play(gameInforamtionDto, numberGenerator); | ||
| } |
There was a problem hiding this comment.
Controller와 Service의 책임을 어떻게 부여해야 할지 감이 오지 않습니다.
현재는 중복코드를 제거해 주기 위해 모든 비즈니스 로직을 서비스로 넘긴 상태인데 괜찮은 구조일까요?
레벨1에서는 콘솔을 view로 사용하였고 Service 레이어의 필요성을 느끼지 못했기에 현재 Service에 있는 로직은 Controller에 있었습니다.
이후 체스 미션에서 DB를 사용하게 되면서 Service는 Controller와 dao를 연결해 주는 중간 다리 역할로 사용하게 되었었습니다.
그런데 이를 모두 Service에 옮기게 되니 와닿지 않는 상태입니다...!
There was a problem hiding this comment.
현재 구조를 잘 짜주었다고 생각됩니다.
제가 생각하는 Controller의 역할은, 요청을 받아 적절한 서비스에 전달 후 서비스에서 결과값을 전달받아 반환하는 라우팅의 역할입니다. 그에 비해 Service의 역할은, Dao/repository의 연산 순서와 트랜잭션(이 부분은 이후 미션에서 학습할 듯 합니다) 을 지켜주면서 DB와의 통신을 수행해 주고, 추가로 필요한 연산(엔티티/도메인 클래스에 대한 조작)을 수행해 주는 것이지요.
이러한 시각에서 보았을 때 저는 현재 구조에 큰 문제는 없다고 생각합니다. 😄
There was a problem hiding this comment.
자세한 답변 감사합니다!
서비스에서 도메인에 접근해도 괜찮은 것인가에 대해 고민이었는데 덕분에 구조에 대해 조금은 이해가 되었습니다!
| public List<RacingCarDto> findBy(long resultId) { | ||
| String sql = "select name, position from racing_cars where result_id = ?"; |
There was a problem hiding this comment.
현재 제 쿼리문과 Dto를 살펴보면 table 데이터의 한 행을 가져오는 것이 아니라 행에서 필요한 정보만 가져오고 있습니다.
해당 방법과 한 행을 모두 가져오는 것 중 어떤 것이 더 적절하다고 생각하시나요?
이를 고민하며 추가적으로 든 의문은 DTO와 Entity의 차이입니다.
Entity는 Table과 1:1 매핑이 되는 Dto와 같은 객체로 알고 있는데, Dto와의 역할은 아예 다른 것일까요?
만약 원하는 정보만이 아니라 한 행을 가져오게 된다면 Dto보단 Entity가 더 적절할까요?
There was a problem hiding this comment.
제가 생각하는 바를 정리해보면 아래와 같습니다.
- Dao는 입력(삽입) 및 출력(조회) 시 Entity 값을 주는 것이 좋습니다.
- dao 혹은 repository가 하나의 entity에 대한 CRUD 연산을 수행한다고 개발자들이 인식하기 때문입니다. 예를 들어 하나의 dao에서 두세개의 엔티티에 대한 연산을 수행하거나, RacingCarDao가 Player entity를 인자로 받는다고 생각하면 이상하지 않나요?
- 만약 여러 엔티티를 조회해와서 연산해야 한다면, 서비스에서 적절히 dao를 사용해 조회한 값들을 바탕으로 연산하면 됩니다.
- 특정한 값만 필요하다면, 상황에 따라서는 그에 맞는 메소드를 만들 수 있습니다. 예를 들어 jpa 기반의 repository들은,
String findNameById(Long id)처럼, 특정 필드만 조회하는 메소드를 지원하고 있습니다. 다만 여러 필드를 가져온다면, 저는 차라리 엔티티째로 받아 적절히 연산하는 것이 가독성이나 확장성 면에서 더 낫다고 생각합니다. 쿼리의 효율도 큰 차이가 없을듯하고요.
- 그리고 저는 DTO가 dao 연산에 들어가는 것에는 반대하고 있습니다.
- DTO는 컨트롤러 및 서비스가 다른 레이어와 통신 시 사용하는 클래스입니다. 그런데 이것이 dao와의 연산에서 사용되면, dao가 컨트롤러 등에 종속될 수 있습니다.
- 서비스 레이어는 엔티티 클래스를 직접 다루는 레이어이기 때문에, 엔티티로 dao와 통신하는 것이 책임상의 문제가 되지는 않습니다.
- 물론 이런 경우가 있을 수 있지요. id가 auto_increment여서 해당 필드를 비워서 보내야 하는 경우엔 어떻게 하면 좋을까요? 저는 그러한 경우에는, 엔티티에 빌더 패턴을 적용하고 id는 null로 설정해서 보내줍니다. 이것이 좋은 방법이 아닌 것처럼 보일수도 있으나, 해당 연산을 수행하기 위해 별도의 클래스를 도입하거나 Map으로 인자를 전달하는 것보다는 가독성 면에서 낫다고 생각하기 때문입니다.
제 생각을 정리하다 보니 다소 두서없게 글이 나왔네요.
혹시 이해가 잘 되지 않는 부분이 있거나, 납득이 되지 않는 부분이 있다면 편하게 코멘트 달아주세요.
제이미가 어떻게 생각하는지 의견을 들어보고, 함께 고민하며 더 좋은 방향을 결정해보고 싶습니다. 👍
There was a problem hiding this comment.
자세한 답변 감사합니다!
일단 DTO를 사용하면 안 되는 이유에 대해서 카프카의 설명 덕분에 이해하게 되었습니다!
또한, 저 역시 확장성을 생각했을 때는 Entity로 가져오는 것이 더 낫다는 이야기에 공감합니다.
그래서 우선 Entity를 반환하는 방법으로 수정해 보았습니다. (로직 바로가기)
처음에는 사용하지 않는 데이터도 있으니, dto를 사용해야 하지 않을까? 라고, 생각했습니다.
그런데 확장성을 생각하다 보니 Entity가 더 적절한지 고민하게 되며 질문하게 되었습니다.
그런데 select를 통해 *을 사용해야 하는 것을 지양해야 한다는 글을 보았습니다. (select 시 * 사용 지양 블로그 글)
저 역시 처음에 부분 값만 가져오려고 한 이유는 불필요한 데이터를 저장할 필요까지 있는가에서 시작되었기 때문에 이에 대한 카프카의 의견이 궁금합니다..
혹은 제가 카프카의 의견을 잘못 이해한 것인지도 궁금합니다.
추가적으로 만약 전체가 아니라 특정 데이터들만 가져오게 된다면 해당 객체는 무슨 객체에 속하는 것일까요?
DTO도 Entity도 아닌 것 같아 여쭤봅니다…!
There was a problem hiding this comment.
또한, id를 null로 설정하고 보내라는 이야기가 있는 이 부분에 대해 제대로 이해하지 못했습니다.
Select를 통해 값을 가져오는 경우는 null로 설정할 필요가 없는데, 카프카가 말씀하신 부분은 dao에 값을 넘길 때를 이야기인 걸까요?
만약 그렇다면 현재처럼 객체, 특정 값을 넘기는 것 보다 db에 insert 시에도 Entity를 사용해 Service에서 dao로 넘겨야 한다는 것을 말씀해 주신 걸까요?
There was a problem hiding this comment.
피드백에 대해 학습해서 빠르게 수정해주신 부분 좋습니다 💯
repository(dao)를 어떻게 구성하면 좋을지에 대해 고민을 해볼 수 있었네요.
그리고 이제 코멘트 남겨주신 부분에 대해서도 제 생각을 정리해 보겠습니다.
(다만 저는 주로 jpa+querydsl 스펙을 사용하므로, jdbc 구현에 대해 좋은 코멘트를 드리지 못할 수도 있습니다.)
There was a problem hiding this comment.
적어주신대로 select * 를 지양하는 것은 좋은 습관이라고 생각합니다.
실제로 jpa 등에서 자동으로 생성되는 쿼리를 보면, *을 쓰지 않고 각각의 필드를 매핑해 주는 방식이거든요.
다만 저는 그렇다고 해도, 엔티티에 필요한 필드 전체를 쿼리에 명시해주면 되지 않나, 라고 생각하고 있습니다. �
- 만약 테이블에 5개의 컬럼이 있는데, 엔티티에는 3개 필드만이 존재하고, 쿼리는 그 3개 컬럼을 조회한다 -> 이건 이상하다고 보는 거고요
- 테이블에 5개 컬럼이 있고, 엔티티에는 5개 필드가 존재한다. 쿼리는 5개 컬럼을 조회한다 -> 이건 자연스럽다고 생각하고 있어요.
- 테이블에 5개 컬럼이 있고, 쿼리를 통해 그 중 2개 컬럼을 Map에 담아 반환하려고 한다 -> 요거까지도 자연스럽지 않나 라는 생각이 듭니다.
다만 db의 값 중 필요한 값 일부만을 가져오는 것이 현재 미션에서 필요할지에 대해서도 의문이 들기는 하지만요.
그리고 특정 데이터들만 가져오게 된다면, 아무래도 entity보다는 ResultSet을 VO 클래스에 매핑하게 되지 않을까 생각이 됩니다. 저번 리뷰에서 제가 그러한 경우에 사용하는 클래스 역시 엔티티라고 지칭했는데,그보다는 VO라는 표현이 더 맞지 않을까 싶었어요.
| public List<Long> findAllId() { | ||
| String sql = "select id from results"; | ||
| return jdbcTemplate.queryForList(sql, Long.class); | ||
| } | ||
|
|
||
| public String findWinnerBy(long id) { | ||
| String sql = "select winners from results where id = ?"; | ||
| return jdbcTemplate.queryForObject(sql, String.class, id); | ||
| } |
There was a problem hiding this comment.
현재 id를 모두 가져와 results 테이블에서는 각 아이디의 우승자를 가져오고 racing_cars 테이블에서는 각 아이디를 result_id로 갖고 있는 행들에 대한 List를 가져오게 되어 있습니다.
그렇게 가져온 정보들은 Service에서 List<>로 다시 만들어 Controller에 넘겨주게 됩니다.
그런데 쿼리문 중 join이 있는 것을 알고 있는데 이를 사용하는 것이 더 효율적인지 궁금합니다.
또한, join을 사용해 우승자, 참여 레이싱카 리스트를 한 번에 가져오게 된다면 어떤 RacingCarDao와 ResultsDao 중 어떤 클래스에 있는 것이 적절한지 궁금합니다.
There was a problem hiding this comment.
만약 dao에 join을 꼭 적용해야 한다면, 저라면 join으로 얻게 되는 값과 매핑되는 entity 클래스를 만들어주고 그 엔티티에 대한 연산만 수행하도록 dao의 역할을 제한할 듯 하네요.
다만 저는 현재 미션 레벨에서 join을 직접 사용하는 것을 권하지는 않습니다.
이후 jpa, querydsl 등의 기술스택을 익힐 경우 더 편리하게 구현할 수 있기도 하고, 현재 레벨에서 join을 포함한 쿼리를 작성하면서 생기는 문제점들도 있다고 생각하기 때문입니다. (최적화가 어렵고, 쿼리의 지속적인 관리가 잘 될지도 조심스러우며, 이후의 변화에 동적으로 대응하기 어려운 점 등...)
There was a problem hiding this comment.
다만 이렇게 더 좋은 방법에 대해 고민해본 부분은 매우 좋습니다 💯
질문해준 내용에 대해 해답을 드리기보다 권하지 않는다는 말로 끝내게 되어서 아쉽지만요,
그래도 이 부분은 좀 더 학습을 진행해보면서 차근차근 접근하는 쪽이 더 효율적일 것이라고 생각이 되었습니다.
There was a problem hiding this comment.
join 사용에 대한 의견 감사합니다!
join으로 얻게 되는 값은 존재하는 테이블이 아닌데 해당 데이터도 Entity라고 호칭하나요?
join을 통해 둘을 합친 테이블이기 때문에 Entity라고 보는 것일까요?
그렇다면 select 등을 통해 만들어진 정보도 결국 테이블이라고 책에서 봤던 것으로 기억하는데 이도 Entity라고 볼 수 있을까요?
join과 관련된 추가적인 질문으로 현재 foreign key를 사용하고 있습니다.
이에 대해 join을 사용하지 않기에 foreign key로 만들지 않았다는 이야기를 몇 번 들었는데 정말 의미가 없는 것일까요?
일단 제가 foreign key를 사용한 이유는 racing_cars에 있는 result_id 데이터는 꼭 results 테이블에 있는 아이디어야 하기 때문입니다.
이를 보장해 주기 위해 사용했다고 생각했는데 이에 대한 카프카의 의견이 궁금합니다!
There was a problem hiding this comment.
join으로 얻게 되는 값도 위의 리뷰에 기재한 것처럼, Entity라기보다는 ResultSet을 VO 클래스에 매핑하는 식이 되지 않을까 싶네요.
제가 이 부분에 대해서는 용어를 부적절하게 사용했다는 생각이 듭니다. 환기해주셔서 감사해요 👍
- https://www.ibm.com/docs/en/imdm/12.0?topic=concepts-key-entity-attribute-entity-type 요 문서를 보면 entity의 정의가 좀 더 명확해지네요.
- 다만 그래도 뭔가 고민이 되는 지점들이 있는데, 혹시 관련하여 좋은 레퍼런스가 있으면 코멘트로 남기겠습니다. 저도 함께 학습하게 되네요 😄
그리고 FK를 사용하는 것은 join과는 별개로 좋다고 저는 생각해요!
FK에 대한 제약조건이 많기 때문에, 부적절한 데이터가 발생하는 것(예: 잘못된 매핑, 실제로 없는 데이터와의 연관관계 생성 등)을 막을 수 있기 때문입니다.
그래서 저도 제이미의 의견에 적극 공감합니다. 이번 미션을 통해 외래키 관련하여 생각을 잘 정리해서 적어주셨는데, 정말 잘해주셨어요 💯 💯
| @AfterEach | ||
| void reset() { | ||
| jdbcTemplate.execute("SET REFERENTIAL_INTEGRITY FALSE"); | ||
| jdbcTemplate.update("TRUNCATE TABLE results"); | ||
| jdbcTemplate.execute("SET REFERENTIAL_INTEGRITY TRUE"); | ||
| } |
There was a problem hiding this comment.
테이블의 데이터가 각 테스트를 수행할 때마다 초기화되어야 하므로 테스트가 끝날 때마다 Truncate를 통해 계속해 초기화해 주었습니다.
해당 쿼리문은 테이블의 아이디도 초기화해 주는 것으로 알고 있기에 사용했으며, FOREIGN KEY가 문제가 되는 것 같아 해당 관계를 끊어주는 쿼리문도 넣게 되었습니다.
그런데 왜인지 해당 쿼리문이 제대로 수행되지 않는 상태입니다.
해당 쿼리문에서 오류는 더 이상 나지 않지만, Truncate을 통해 테이블을 초기화하는 명령도 제대로 수행되지 않고 있습니다.
혹시 어떤 문제인지 힌트를 얻을 수 있을까요?
(RacingCarDao에서 진행한 Truncate)
There was a problem hiding this comment.
지금 전체를 돌릴 경우 테스트들이 실패하고 있는데, 이건 auto_increment되는 id의 설정 문제로 보이네요.
관련된 글을 첨부합니다: https://tecoble.techcourse.co.kr/post/2020-09-15-test-isolation/
저는 사실 이런 문제때문에 테스트할때 id 외의 필드를 참조하도록 하기도 하는데요,
작성해주신 테스트코드를 보니 id에 의존할 수밖에 없는 부분들도 보여서, 위의 가이드를 참조해서 해결해보는 방향을 권합니다.
There was a problem hiding this comment.
제 생각에 truncate가 먹지 않은 것은 아래 문서에 나온 것과 동일한 상황으로 보이는데요,
https://stackoverflow.com/questions/2900016/how-to-truncate-a-table-with-spring-jdbctemplate
아마 jdbcTemplate이 제공하는 기능 범위 바깥이기 때문에 생긴 문제로 보여요.
일단 위에서 첨부한 링크를 바탕으로, @Sql 어노테이션을 사용해서 (테스트 패키지 내 리소스 패키지에 넣어둔) 적절한 truncate sql을 실행시키는 방향으로 이 문제를 풀어봅시다.
There was a problem hiding this comment.
첨부 해주신 링크대로 @Sql 어노테이션을 통해 수행해 보았지만, 동일한 문제가 발생했습니다.
그리고 여러 방법으로 수행해 본 결과 아래 두 가지 방법으로만 제대로 수행되는 것을 확인했습니다.
// 방법 1 - TRUNCATE
jdbcTemplate.execute("SET REFERENTIAL_INTEGRITY FALSE");
jdbcTemplate.update("TRUNCATE TABLE results");
jdbcTemplate.execute("ALTER TABLE results ALTER COLUMN id RESTART WITH 1");
jdbcTemplate.execute("SET REFERENTIAL_INTEGRITY TRUE");
// 방법 2 - DELETE
jdbcTemplate.update("DELETE FROM results");
jdbcTemplate.execute("ALTER TABLE results ALTER COLUMN id RESTART WITH 1");즉, 어떤 방식으로 수행하든지 ALERT 문을 통해 id를 1로 초기화해 주어야 했습니다.
이는 REFERENTIAL_INTEGRITY를 FALSE로 설정함으로써 발생하는 문제라고 생각이 드는데 그렇다면 For ign Key가 존재하는 경우 TRUNCATE보다는 DELETE를 사용하는 것이 더 안전하다고 생각해도 괜찮은 것일까요?
There was a problem hiding this comment.
제가 알아본 내용이 확실한게 아니었군요! 아무래도 외래키의 문제까지 겹쳐서 그랬구나 싶습니다.
그러나 이 부분에 대해 어떤 정답이 있지는 않다고 생각해요. 사실 현재 환경이 h2를 쓰는 환경이라는 특수한 상황이라 그렇지,
DB 환경이 어떻게 설계되어 있는지, 어떠한 방식으로 테스트를 구현하는지에 따라 좋은 방향이 달라질 수 있거든요.
그러니 이 부분에서는 제 경험을 간단히 서술해 보겠습니다.
저의 경우 repository 테스트시에 아래와 같이 수행했던 것으로 생각됩니다.
DB와 연결해서 테스트 (저는 testContainer라는 기술을 사용하고 있는데, 이부분은 아직 학습할 필요는 없습니다)
- db에 데이터를 넣은 뒤 id로 조회 테스트: db에 데이터를 넣는 update 메소드가 저장된 엔티티를 반환하게 jpa에서 지원해줘서, 해당 id값을 테스트할때 사용했음.
- 그 외의 경우, auto_increment되는 id보다는 다른 필드를 사용해 테스트를 수행할 수 있도록 함 (jdbctest와 같은 db 테스트용 어노테이션을 붙이면 테스트마다 데이터가 롤백되니, 간단히 findAll을 해서 조회되는 값을 의도한 값과 대조해봐도 문제 없었음)
- 테스트를 할 때 테스트용 mysql DB 이미지를 도커에 띄우도록 자동화해둠. 운영환경에서는 별도의 mysql을 사용했으므로, 서버 재구동시 영향이 가지 않도록 data.sql은 사용하지 않음
h2로 테스트 (예전에 토이 프로젝트로 진행)
- 테스트 리소스에 별도의 data.sql을 두고 테스트
- jdbctest와 동일한 효과를 가지는 jpa 테스트 어노테이션을 통해 롤백 지원
- 위와 마찬가지로 id값에 의존하지 않도록 테스트를 구현하여, 크게 문제는 없었음.
There was a problem hiding this comment.
결론적으로 정리해보면, 테스트 코드를 조금 수정해보면 자연스럽게 해결되지 않을까 하는 생각도 있습니다.
이번 미션에서는 경험을 통해 학습 방향을 잡았으니, 다음 미션에서 DB 테스트를 작성할 때 좀 더 집중해서 파고들어 보기를 권합니다.
- 현재 레벨에서 data에 대한 테스트를 엄격하게 작성하는건 어려움이 있는 일이라 코멘트를 달면서도 조심스러웠는데, 제이미가 잘 수행해 주셨다고 생각합니다.
- https://www.baeldung.com/spring-jdbctemplate-testing 요 문서도 가볍게 참고해보면 좋을듯하네요.
| @AfterEach | ||
| void reset() { | ||
| jdbcTemplate.execute("SET REFERENTIAL_INTEGRITY FALSE"); | ||
| jdbcTemplate.update("TRUNCATE TABLE racing_cars"); | ||
| jdbcTemplate.update("TRUNCATE TABLE results"); | ||
| jdbcTemplate.execute("SET REFERENTIAL_INTEGRITY TRUE"); | ||
| } |
There was a problem hiding this comment.
RacingCarDao에서 진행한 Truncate
리뷰 답변 참고
|
안녕하세요 제이미, 리뷰어 카프카입니다. 리뷰 요청 확인하였습니다. 저번 리뷰도 잠시 확인해 보았는데, 코멘트로 정리를 정말 잘해주셔서 놀랐습니다 💯 리뷰는 내일 오전 9시 ~ 10시 사이에 진행 예정입니다. 작업하시느라 고생 많으셨습니다 👍 이렇게 열심히 미션에 임해주셔서 리뷰어로써 감사합니다 😄 |
| String carNames = inputView.inputCarNames(); | ||
| return racingCarService.getCars(carNames); |
There was a problem hiding this comment.
사실 보통의 상황이라면 이렇게 콘솔과 웹을 동시에 구현하지는 않을듯해요. 그러나 미션 요구사항이므로...특수한 상황임을 감안하면 이해가 되는 부분입니다.
일반적인 구조라면, Controller는 요청에 대한 라우팅만 수행하는 것이 맞기는 합니다. 위의 runGame 메소드같은 경우도, 요청이 들어올때 Service에서 수행해줘야 하지요. 일단 RacingCarWebController에서 사양에 맞게 잘 구현되어 있으므로, 이 부분에 대해서는 감안하겠습니다.
| public GameResultDto createGame(@RequestBody GameInforamtionDto gameInforamtionDto) { | ||
| return racingCarService.play(gameInforamtionDto, numberGenerator); | ||
| } |
There was a problem hiding this comment.
현재 구조를 잘 짜주었다고 생각됩니다.
제가 생각하는 Controller의 역할은, 요청을 받아 적절한 서비스에 전달 후 서비스에서 결과값을 전달받아 반환하는 라우팅의 역할입니다. 그에 비해 Service의 역할은, Dao/repository의 연산 순서와 트랜잭션(이 부분은 이후 미션에서 학습할 듯 합니다) 을 지켜주면서 DB와의 통신을 수행해 주고, 추가로 필요한 연산(엔티티/도메인 클래스에 대한 조작)을 수행해 주는 것이지요.
이러한 시각에서 보았을 때 저는 현재 구조에 큰 문제는 없다고 생각합니다. 😄
| public List<RacingCarDto> findBy(long resultId) { | ||
| String sql = "select name, position from racing_cars where result_id = ?"; |
There was a problem hiding this comment.
제가 생각하는 바를 정리해보면 아래와 같습니다.
- Dao는 입력(삽입) 및 출력(조회) 시 Entity 값을 주는 것이 좋습니다.
- dao 혹은 repository가 하나의 entity에 대한 CRUD 연산을 수행한다고 개발자들이 인식하기 때문입니다. 예를 들어 하나의 dao에서 두세개의 엔티티에 대한 연산을 수행하거나, RacingCarDao가 Player entity를 인자로 받는다고 생각하면 이상하지 않나요?
- 만약 여러 엔티티를 조회해와서 연산해야 한다면, 서비스에서 적절히 dao를 사용해 조회한 값들을 바탕으로 연산하면 됩니다.
- 특정한 값만 필요하다면, 상황에 따라서는 그에 맞는 메소드를 만들 수 있습니다. 예를 들어 jpa 기반의 repository들은,
String findNameById(Long id)처럼, 특정 필드만 조회하는 메소드를 지원하고 있습니다. 다만 여러 필드를 가져온다면, 저는 차라리 엔티티째로 받아 적절히 연산하는 것이 가독성이나 확장성 면에서 더 낫다고 생각합니다. 쿼리의 효율도 큰 차이가 없을듯하고요.
- 그리고 저는 DTO가 dao 연산에 들어가는 것에는 반대하고 있습니다.
- DTO는 컨트롤러 및 서비스가 다른 레이어와 통신 시 사용하는 클래스입니다. 그런데 이것이 dao와의 연산에서 사용되면, dao가 컨트롤러 등에 종속될 수 있습니다.
- 서비스 레이어는 엔티티 클래스를 직접 다루는 레이어이기 때문에, 엔티티로 dao와 통신하는 것이 책임상의 문제가 되지는 않습니다.
- 물론 이런 경우가 있을 수 있지요. id가 auto_increment여서 해당 필드를 비워서 보내야 하는 경우엔 어떻게 하면 좋을까요? 저는 그러한 경우에는, 엔티티에 빌더 패턴을 적용하고 id는 null로 설정해서 보내줍니다. 이것이 좋은 방법이 아닌 것처럼 보일수도 있으나, 해당 연산을 수행하기 위해 별도의 클래스를 도입하거나 Map으로 인자를 전달하는 것보다는 가독성 면에서 낫다고 생각하기 때문입니다.
제 생각을 정리하다 보니 다소 두서없게 글이 나왔네요.
혹시 이해가 잘 되지 않는 부분이 있거나, 납득이 되지 않는 부분이 있다면 편하게 코멘트 달아주세요.
제이미가 어떻게 생각하는지 의견을 들어보고, 함께 고민하며 더 좋은 방향을 결정해보고 싶습니다. 👍
| public List<Long> findAllId() { | ||
| String sql = "select id from results"; | ||
| return jdbcTemplate.queryForList(sql, Long.class); | ||
| } | ||
|
|
||
| public String findWinnerBy(long id) { | ||
| String sql = "select winners from results where id = ?"; | ||
| return jdbcTemplate.queryForObject(sql, String.class, id); | ||
| } |
There was a problem hiding this comment.
만약 dao에 join을 꼭 적용해야 한다면, 저라면 join으로 얻게 되는 값과 매핑되는 entity 클래스를 만들어주고 그 엔티티에 대한 연산만 수행하도록 dao의 역할을 제한할 듯 하네요.
다만 저는 현재 미션 레벨에서 join을 직접 사용하는 것을 권하지는 않습니다.
이후 jpa, querydsl 등의 기술스택을 익힐 경우 더 편리하게 구현할 수 있기도 하고, 현재 레벨에서 join을 포함한 쿼리를 작성하면서 생기는 문제점들도 있다고 생각하기 때문입니다. (최적화가 어렵고, 쿼리의 지속적인 관리가 잘 될지도 조심스러우며, 이후의 변화에 동적으로 대응하기 어려운 점 등...)
| public List<Long> findAllId() { | ||
| String sql = "select id from results"; | ||
| return jdbcTemplate.queryForList(sql, Long.class); | ||
| } | ||
|
|
||
| public String findWinnerBy(long id) { | ||
| String sql = "select winners from results where id = ?"; | ||
| return jdbcTemplate.queryForObject(sql, String.class, id); | ||
| } |
There was a problem hiding this comment.
다만 이렇게 더 좋은 방법에 대해 고민해본 부분은 매우 좋습니다 💯
질문해준 내용에 대해 해답을 드리기보다 권하지 않는다는 말로 끝내게 되어서 아쉽지만요,
그래도 이 부분은 좀 더 학습을 진행해보면서 차근차근 접근하는 쪽이 더 효율적일 것이라고 생각이 되었습니다.
| @@ -39,6 +29,15 @@ void setUp() { | |||
| RestAssured.port = port; | |||
| } | |||
|
|
|||
| @TestConfiguration | |||
There was a problem hiding this comment.
TestConfiguration 활용해주신 부분 좋습니다 💯 관련해서 학습을 잘 진행해 주셨군요!
| @AfterEach | ||
| void reset() { | ||
| jdbcTemplate.execute("SET REFERENTIAL_INTEGRITY FALSE"); | ||
| jdbcTemplate.update("TRUNCATE TABLE results"); | ||
| jdbcTemplate.execute("SET REFERENTIAL_INTEGRITY TRUE"); | ||
| } |
There was a problem hiding this comment.
지금 전체를 돌릴 경우 테스트들이 실패하고 있는데, 이건 auto_increment되는 id의 설정 문제로 보이네요.
관련된 글을 첨부합니다: https://tecoble.techcourse.co.kr/post/2020-09-15-test-isolation/
저는 사실 이런 문제때문에 테스트할때 id 외의 필드를 참조하도록 하기도 하는데요,
작성해주신 테스트코드를 보니 id에 의존할 수밖에 없는 부분들도 보여서, 위의 가이드를 참조해서 해결해보는 방향을 권합니다.
| @AfterEach | ||
| void reset() { | ||
| jdbcTemplate.execute("SET REFERENTIAL_INTEGRITY FALSE"); | ||
| jdbcTemplate.update("TRUNCATE TABLE results"); | ||
| jdbcTemplate.execute("SET REFERENTIAL_INTEGRITY TRUE"); | ||
| } |
There was a problem hiding this comment.
제 생각에 truncate가 먹지 않은 것은 아래 문서에 나온 것과 동일한 상황으로 보이는데요,
https://stackoverflow.com/questions/2900016/how-to-truncate-a-table-with-spring-jdbctemplate
아마 jdbcTemplate이 제공하는 기능 범위 바깥이기 때문에 생긴 문제로 보여요.
일단 위에서 첨부한 링크를 바탕으로, @Sql 어노테이션을 사용해서 (테스트 패키지 내 리소스 패키지에 넣어둔) 적절한 truncate sql을 실행시키는 방향으로 이 문제를 풀어봅시다.
| @ExceptionHandler | ||
| public void handle(Exception exception) { | ||
| System.out.println(exception.getMessage()); | ||
| } |
There was a problem hiding this comment.
1단계 피드백주신 내용에 따라 ExceptionHandler를 추가해보았습니다.
그런데 html 코드를 살펴보니 다음과 같이 예외가 발생하면 무조건 Error()를 던지게 되어 있습니다.
.then(response => {
if (response.ok) {
return response.json();
} else {
throw new Error("Network response was not ok.");
}
})이 경우 Controller에서 예외를 반환해 주어야 하는 걸까요?
혹은 현재처럼 예외를 던지지 않고 콘솔에 예외 메시지만 출력해 주는 것이 좋을까요?
There was a problem hiding this comment.
제 생각을 정리해 보겠습니다.
- 현재 html 내의 js 코드를 보면, response의 상태값이 ok(200)가 아니면 에러를 발생시키고 있습니다.
- 즉 예기치 못한 상황에서 생긴 4xx 에러(예: 컨트롤러의 path를 잘못 수정해 404 에러 발생)나 5xx 에러 (핸들링되지 못한 예외 발생 등) 에 대해 에러를 발생시킨다는 의미입니다.
- 그러나 ExceptionHandler로 예외를 캐치했을 경우, 우리가 원하는 것은 그 예외를 다시 발생시키는 것이 아니라, 예외의 내용을 전달하는 것이겠지요? 현재는 해당 내용을 콘솔 로그로 찍어주고 있고요. (처음 해보는 것인데, 이 정도의 성취도 충분히 잘한 것이라고 생각합니다 💯 )
- 그렇다면 이제 우리가 해야 할 것은, "예외가 발생했고 handler가 예외를 핸들링했다면, 적절한 상태로 적절한 DTO에 에러 메시지를 보내주면 되지 않을까?" 라고 생각합니다.
There was a problem hiding this comment.
저라면, 아래처럼 구현할 것으로 생각됩니다. 한번 생각한 대로 정리해 볼게요.
- field로 errorMessage를 포함하는 ErrorDTO 클래스를 만들어 준다.
- 예외를 handling할 경우, ResponseDTO를 반환해 준다.
- 예외가 발생했고 handling이 되면, 400 bad request 상태를 ResponseEntity에 설정해서 반환한다.
- 현재 프론트엔드 코드를 일부 수정하여, Error 발생시 response.errorMessage를 인자로 삼도록 한다.
혹시 이해가 가지 않는 부분이 있다면 코멘트 부탁드립니다. 👍
| public List<Result> findAll() { | ||
| String sql = "select * from results"; | ||
| return jdbcTemplate.query(sql,actorRowMapper); | ||
| } |
There was a problem hiding this comment.
Dao에서 Entity를 반환하도록 수정
리뷰 답변 참고
|
안녕하세요 카프카! 그런데 아직 완벽히 해결되지 않아 추가적인 질문에 대해 코멘트로 달아두었습니다. |
|
안녕하세요 제이미! 리뷰어 카프카입니다. 리뷰는 제가 일정이 있어서 내일 오전 9시~10시 사이에 가능할 것으로 생각됩니다. |
There was a problem hiding this comment.
안녕하세요 제이미, 리뷰어 카프카입니다.
미션 마무리까지 쭉 따라오시느라 고생 많으셨습니다 👍 👍
새로운 내용을 학습하느라 정신없으셨을텐데, 오히려 좋은 논의를 잘 이끌어주셔서 즐겁게 리뷰를 진행할 수 있었습니다.
코드상으로 지적할 부분은 없었고, 이전에 논의하던 내용에 제 생각을 정리해 두었습니다.
제이미도 편하게 의견 마저 적어주시고, 혹시 제가 확인이 늦으면 dm 주셔도 괜찮습니다.
미션 마무리까지 고생하셨습니다! 이번 미션은 approve 하겠습니다 💯
| String carNames = inputView.inputCarNames(); | ||
| return racingCarService.getCars(carNames); |
There was a problem hiding this comment.
View를 위한 로직이라는 말이 참 애매한데... 현재 보면 컨트롤러가 View 레이어와 강한 의존을 가지고 있다고 느껴져요.
만약 완벽하게 리팩토링을 하고 싶다면, outputview와 inputview를 json DTO를 통해 통신하도록 완벽히 구현해서 WebController와 동일한 서비스를 사용하도록 해볼 수 있어 보이긴 합니다. 다만 이미 WebController 부분의 구현에서 해당 스펙을 구현했으니, 이번 미션에서 그 정도까지의 작업을 할 필요는 없다고 판단했습니다.
MVC 구조에서 서비스가 하는 역할에 대해 잘 이해하고 있다면, 크게 문제가 되어 보이지는 않습니다.
| @ExceptionHandler | ||
| public void handle(Exception exception) { | ||
| System.out.println(exception.getMessage()); | ||
| } |
There was a problem hiding this comment.
제 생각을 정리해 보겠습니다.
- 현재 html 내의 js 코드를 보면, response의 상태값이 ok(200)가 아니면 에러를 발생시키고 있습니다.
- 즉 예기치 못한 상황에서 생긴 4xx 에러(예: 컨트롤러의 path를 잘못 수정해 404 에러 발생)나 5xx 에러 (핸들링되지 못한 예외 발생 등) 에 대해 에러를 발생시킨다는 의미입니다.
- 그러나 ExceptionHandler로 예외를 캐치했을 경우, 우리가 원하는 것은 그 예외를 다시 발생시키는 것이 아니라, 예외의 내용을 전달하는 것이겠지요? 현재는 해당 내용을 콘솔 로그로 찍어주고 있고요. (처음 해보는 것인데, 이 정도의 성취도 충분히 잘한 것이라고 생각합니다 💯 )
- 그렇다면 이제 우리가 해야 할 것은, "예외가 발생했고 handler가 예외를 핸들링했다면, 적절한 상태로 적절한 DTO에 에러 메시지를 보내주면 되지 않을까?" 라고 생각합니다.
| @ExceptionHandler | ||
| public void handle(Exception exception) { | ||
| System.out.println(exception.getMessage()); | ||
| } |
There was a problem hiding this comment.
저라면, 아래처럼 구현할 것으로 생각됩니다. 한번 생각한 대로 정리해 볼게요.
- field로 errorMessage를 포함하는 ErrorDTO 클래스를 만들어 준다.
- 예외를 handling할 경우, ResponseDTO를 반환해 준다.
- 예외가 발생했고 handling이 되면, 400 bad request 상태를 ResponseEntity에 설정해서 반환한다.
- 현재 프론트엔드 코드를 일부 수정하여, Error 발생시 response.errorMessage를 인자로 삼도록 한다.
혹시 이해가 가지 않는 부분이 있다면 코멘트 부탁드립니다. 👍
| public List<RacingCarDto> findBy(long resultId) { | ||
| String sql = "select name, position from racing_cars where result_id = ?"; |
There was a problem hiding this comment.
피드백에 대해 학습해서 빠르게 수정해주신 부분 좋습니다 💯
repository(dao)를 어떻게 구성하면 좋을지에 대해 고민을 해볼 수 있었네요.
그리고 이제 코멘트 남겨주신 부분에 대해서도 제 생각을 정리해 보겠습니다.
(다만 저는 주로 jpa+querydsl 스펙을 사용하므로, jdbc 구현에 대해 좋은 코멘트를 드리지 못할 수도 있습니다.)
| public List<RacingCarDto> findBy(long resultId) { | ||
| String sql = "select name, position from racing_cars where result_id = ?"; |
There was a problem hiding this comment.
적어주신대로 select * 를 지양하는 것은 좋은 습관이라고 생각합니다.
실제로 jpa 등에서 자동으로 생성되는 쿼리를 보면, *을 쓰지 않고 각각의 필드를 매핑해 주는 방식이거든요.
다만 저는 그렇다고 해도, 엔티티에 필요한 필드 전체를 쿼리에 명시해주면 되지 않나, 라고 생각하고 있습니다. �
- 만약 테이블에 5개의 컬럼이 있는데, 엔티티에는 3개 필드만이 존재하고, 쿼리는 그 3개 컬럼을 조회한다 -> 이건 이상하다고 보는 거고요
- 테이블에 5개 컬럼이 있고, 엔티티에는 5개 필드가 존재한다. 쿼리는 5개 컬럼을 조회한다 -> 이건 자연스럽다고 생각하고 있어요.
- 테이블에 5개 컬럼이 있고, 쿼리를 통해 그 중 2개 컬럼을 Map에 담아 반환하려고 한다 -> 요거까지도 자연스럽지 않나 라는 생각이 듭니다.
다만 db의 값 중 필요한 값 일부만을 가져오는 것이 현재 미션에서 필요할지에 대해서도 의문이 들기는 하지만요.
그리고 특정 데이터들만 가져오게 된다면, 아무래도 entity보다는 ResultSet을 VO 클래스에 매핑하게 되지 않을까 생각이 됩니다. 저번 리뷰에서 제가 그러한 경우에 사용하는 클래스 역시 엔티티라고 지칭했는데,그보다는 VO라는 표현이 더 맞지 않을까 싶었어요.
| public List<Long> findAllId() { | ||
| String sql = "select id from results"; | ||
| return jdbcTemplate.queryForList(sql, Long.class); | ||
| } | ||
|
|
||
| public String findWinnerBy(long id) { | ||
| String sql = "select winners from results where id = ?"; | ||
| return jdbcTemplate.queryForObject(sql, String.class, id); | ||
| } |
There was a problem hiding this comment.
join으로 얻게 되는 값도 위의 리뷰에 기재한 것처럼, Entity라기보다는 ResultSet을 VO 클래스에 매핑하는 식이 되지 않을까 싶네요.
제가 이 부분에 대해서는 용어를 부적절하게 사용했다는 생각이 듭니다. 환기해주셔서 감사해요 👍
- https://www.ibm.com/docs/en/imdm/12.0?topic=concepts-key-entity-attribute-entity-type 요 문서를 보면 entity의 정의가 좀 더 명확해지네요.
- 다만 그래도 뭔가 고민이 되는 지점들이 있는데, 혹시 관련하여 좋은 레퍼런스가 있으면 코멘트로 남기겠습니다. 저도 함께 학습하게 되네요 😄
그리고 FK를 사용하는 것은 join과는 별개로 좋다고 저는 생각해요!
FK에 대한 제약조건이 많기 때문에, 부적절한 데이터가 발생하는 것(예: 잘못된 매핑, 실제로 없는 데이터와의 연관관계 생성 등)을 막을 수 있기 때문입니다.
그래서 저도 제이미의 의견에 적극 공감합니다. 이번 미션을 통해 외래키 관련하여 생각을 잘 정리해서 적어주셨는데, 정말 잘해주셨어요 💯 💯
| @AfterEach | ||
| void reset() { | ||
| jdbcTemplate.execute("SET REFERENTIAL_INTEGRITY FALSE"); | ||
| jdbcTemplate.update("TRUNCATE TABLE results"); | ||
| jdbcTemplate.execute("SET REFERENTIAL_INTEGRITY TRUE"); | ||
| } |
There was a problem hiding this comment.
제가 알아본 내용이 확실한게 아니었군요! 아무래도 외래키의 문제까지 겹쳐서 그랬구나 싶습니다.
그러나 이 부분에 대해 어떤 정답이 있지는 않다고 생각해요. 사실 현재 환경이 h2를 쓰는 환경이라는 특수한 상황이라 그렇지,
DB 환경이 어떻게 설계되어 있는지, 어떠한 방식으로 테스트를 구현하는지에 따라 좋은 방향이 달라질 수 있거든요.
그러니 이 부분에서는 제 경험을 간단히 서술해 보겠습니다.
저의 경우 repository 테스트시에 아래와 같이 수행했던 것으로 생각됩니다.
DB와 연결해서 테스트 (저는 testContainer라는 기술을 사용하고 있는데, 이부분은 아직 학습할 필요는 없습니다)
- db에 데이터를 넣은 뒤 id로 조회 테스트: db에 데이터를 넣는 update 메소드가 저장된 엔티티를 반환하게 jpa에서 지원해줘서, 해당 id값을 테스트할때 사용했음.
- 그 외의 경우, auto_increment되는 id보다는 다른 필드를 사용해 테스트를 수행할 수 있도록 함 (jdbctest와 같은 db 테스트용 어노테이션을 붙이면 테스트마다 데이터가 롤백되니, 간단히 findAll을 해서 조회되는 값을 의도한 값과 대조해봐도 문제 없었음)
- 테스트를 할 때 테스트용 mysql DB 이미지를 도커에 띄우도록 자동화해둠. 운영환경에서는 별도의 mysql을 사용했으므로, 서버 재구동시 영향이 가지 않도록 data.sql은 사용하지 않음
h2로 테스트 (예전에 토이 프로젝트로 진행)
- 테스트 리소스에 별도의 data.sql을 두고 테스트
- jdbctest와 동일한 효과를 가지는 jpa 테스트 어노테이션을 통해 롤백 지원
- 위와 마찬가지로 id값에 의존하지 않도록 테스트를 구현하여, 크게 문제는 없었음.
| @AfterEach | ||
| void reset() { | ||
| jdbcTemplate.execute("SET REFERENTIAL_INTEGRITY FALSE"); | ||
| jdbcTemplate.update("TRUNCATE TABLE results"); | ||
| jdbcTemplate.execute("SET REFERENTIAL_INTEGRITY TRUE"); | ||
| } |
There was a problem hiding this comment.
결론적으로 정리해보면, 테스트 코드를 조금 수정해보면 자연스럽게 해결되지 않을까 하는 생각도 있습니다.
이번 미션에서는 경험을 통해 학습 방향을 잡았으니, 다음 미션에서 DB 테스트를 작성할 때 좀 더 집중해서 파고들어 보기를 권합니다.
- 현재 레벨에서 data에 대한 테스트를 엄격하게 작성하는건 어려움이 있는 일이라 코멘트를 달면서도 조심스러웠는데, 제이미가 잘 수행해 주셨다고 생각합니다.
- https://www.baeldung.com/spring-jdbctemplate-testing 요 문서도 가볍게 참고해보면 좋을듯하네요.
|
그리고 미션 종료하면서, 다른 크루들에게도 코멘트로 남겼던 추가 학습 자료를 전달드립니다. (제가 개인적으로 준비해봤어요) 첫째로, 이제 RestController를 다루게 되었으니 이에 필요한 툴을 학습해 봅시다. 둘째로, 보다 개선된 방법으로 Controller를 테스트하는 방법을 학습해 봅시다. 현재는 컨트롤러에 대해서는 테스트를 별도로 수행하고 있지 않은 것으로 알고 있습니다. 이후 단위 테스트에 대해 학습하게 된다면 컨트롤러를 더 간편하게 테스트할 수 있을텐데요. 혹시 이 부분에 대해 관심이 있다면 참고할 수 있도록, 관련된 공식 문서를 첨부합니다. 이 부분은 지금 이해가 안되더라도 너무 부담가지시지 마시고, 가볍게 미리 봐 둔다면 이후 미션에 큰 도움이 될 것으로 생각됩니다. |
안녕하세요. 카프카!
1단계 미션에 이어 2단계 미션도 PR 리뷰 요청합니다.
고민과 잘 안되는 부분들이 많아 생각보다 리뷰 요청이 늦어졌습니다 😥
의문이 든 사항에 대해선 코드에 코멘트를 작성해 두었습니다.
또한, 아래와 같은 요구사항이 있었는데 이를 제가 잘 이해한 것인지 궁금합니다..!
이와 같은 점들에 관해서도 확인해 주시기를 바랍니다!
그리고 1단계 미션에서 던져주셨던 질문들은 해당 코멘트에 답글로 달아두었습니다.
해당 부분도 확인해 주시면 감사하겠습니다! (1단계 미션 바로가기)