Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[2단계 - 웹 자동차 경주] 준팍(박준현) 미션 제출합니다. #173

Merged
merged 43 commits into from
Apr 24, 2023

Conversation

junpakPark
Copy link
Member

@junpakPark junpakPark commented Apr 19, 2023

안녕하세요 빙봉!

1단계 이후 스프링 공부하느라 미션 진행이 약간 늦어졌네요 ㅠㅠ
일단 테스트 코드 작성없이 2단계 요구 사항 완성이 되어 먼저 PR 보냅니다.

피드백 주시면 반영하여 테스트 코드와 함께 다시 리뷰요청 보내겠습니다.

질문 사항

  • Entity를 만들어보려다가 사용하지 않는 칼럼까지 모두 호출해야하는 것 같아서 삭제했습니다.
    Entity는 언제 무슨 용도로 사용되나요?
    Entity는 항상 테이블과 일대일로 대응해야하나요?
    현재 불필요한 값들까지 불러와야할 이유를 못느껴서
    필요한 칼럼만 가져오고 있는데 이렇게 하면 생길 수 있는 문제가 있나요?
  • 현재 2개의 테이블에서 join을 하지 않고 각각 값을 가져오고 있습니다.
    join을 사용하게 될 경우 성능상에 이점이 있나요?

Copy link

@aegis1920 aegis1920 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 준팍! 빙봉입니다 🙂

2단계 잘 구현해주셨네요! 콘솔과 웹 사이 중복 제거 👍
질문에 대한 답변 및 새로운 피드백 코멘트 남겼으니 확인해주세요!

궁금한 점이 있다면 코멘트나 편하게 DM으로 말씀주셔요~

README.md Outdated
Copy link

@aegis1920 aegis1920 Apr 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Entity는 항상 테이블과 일대일로 대응해야하나요?

  • 준팍이 생각하시는 Entity는 무엇인가요?
  • 만약 Entity–relationship model에서 DB의 table 개념으로 생각하신다면 일대일 대응이 되어야할 것 같습니다.

현재 불필요한 값들까지 불러와야할 이유를 못느껴서 필요한 칼럼만 가져오고 있는데 이렇게 하면 생길 수 있는 문제가 있나요?

  • 현재 요구사항에서는 따로 문제가 될 건 없겠네요. 다만 말씀해주시는 게 단순히 DB 컬럼 조회를 위한 DTO 개념인 건지, 아니면 Entity의 개념인 건지부터 정확히 파악해야될 것 같습니다.

현재 2개의 테이블에서 join을 하지 않고 각각 값을 가져오고 있습니다. join을 사용하게 될 경우 성능상에 이점이 있나요?

좋은 질문이네요! 👍

  • 2가지 경우가 있을 것 같은데요.
    • service에서 각각 조회를 해서 join
    • DB 쿼리에서 join
  • 상황에 따라 다르며 각각 트레이드 오프가 있습니다. 현재 상황에서는 단순한 조회라 DB 쿼리에서 조인하는 게 보기도 편하고 성능상 이점이 있겠네요. 트레이드 오프 관련해서는 직접 고민해보시고 생각을 말씀해주세요!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Entity와 DTO, VO에 대한 개념들이 많이 헷갈렸습니다.
이번 리팩토링은 Entity를 DB의 table 개념으로 생각하고 진행하였습니다.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DB 쿼리에서 join하도록 리팩토링하면서 2가지 고민이 들었습니다.

  • 받아온 값들을 다시 ResponseDto에 매핑하는 과정이 까다로워졌습니다.
  • Join한 결과를 어떤 Entity에 담을 것인가?

1번 고민에 대해서는 최대한 메서드 분리를 하는 식으로 해결하려 하였으나,
한계가 느껴졌습니다.
2번 고민에 대해서는 join한 결과와 일대일 대응되는 joinEntity를 만드는 식으로 접근해보았습니다. 이게 적절한 접근이 맞을까요?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

entity가 table 개념이라면 일대일 대응이 되어야하니 joinEntity를 따로 작성하기보단 DTO 개념이 더 어울릴 것 같네요. DTO는 Data Transfer Object로 Controller든 Service든 DAO든 어디에든 있을 수 있으니 DAO에 있어도 괜찮다고 생각합니다.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DTO의 사용 범위에 대하여
이 글을 읽고, DTO는 서비스와 컨트롤러 사이에서 생성되어야 한다고 생각했습니다.
영속성 계층에서도 DTO를 생성해도 되나요?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DTO는 단순히 계층간 데이터 이동을 위한 객체이고,
도메인을 DTO로 변환하거나, 그 반대가 발생해야하는 경우에 한해
서비스에 둘 것인지, 컨트롤러에 둘 것인지에 대한 논쟁이 있다고 이해하면 되나요?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

추가적으로, DAO에서 반환되는 값이 무엇인지 결정하는 기준을 다음과 같이 생각해보았는데 맞을까요?

  • Entity : 반환되는 데이터가 데이터베이스 테이블 구조와 정확히 일치하여, 추가 처리 또는 변환이 필요하지 않은 경우
  • DTO : 반환되는 데이터가 Entity에 있는 데이터 중 일부이거나 변환이 필요한 경우.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 글을 읽고, DTO는 서비스와 컨트롤러 사이에서 생성되어야 한다고 생각했습니다.
영속성 계층에서도 DTO를 생성해도 되나요?

저는 DTO를 좀 넓은 범위에서 생각하는데요. (원하는 필드를 조합하는 느낌) 이런 범위에서는 JPA의 interface-base projections(링크)도 DTO와 비슷하다고 말할 수 있지 않을까? -> 필요하다면 영속성 계층에서도 만들 수 있지 않을까?라는 생각이 있습니다. 만약 안된다면 어떤 이유에서 안될 지 생각했을 때 그 이유가 생각이 안 떠오릅니다.

DTO는 단순히 계층간 데이터 이동을 위한 객체이고,
도메인을 DTO로 변환하거나, 그 반대가 발생해야하는 경우에 한해
서비스에 둘 것인지, 컨트롤러에 둘 것인지에 대한 논쟁이 있다고 이해하면 되나요?

보내주신 링크에서는 그렇습니다. 이 부분은 크루들과 함께 이야기해보면 좋을 것 같아요! DTO를 어떻게 받아들이냐에 따라 다르고 정답이 없는 문제라서요ㅎㅎ

추가적으로, DAO에서 반환되는 값이 무엇인지 결정하는 기준을 다음과 같이 생각해보았는데 맞을까요?

  • Entity : 네 맞습니다. 테이블 구조와 정확히 일치해야하며 Entity간 구별되는 ID를 가지고 있어야 합니다.
  • DTO : 네 맞습니다. 데이터 중 일부도.. 맞긴한데요. 말 그대로 데이터 전송 객체라고 생각하시면 될 것 같습니다. (그래서 저는 이 DTO라는 객체가 Client -> Controller -> Service -> Repository 로 가는 모든 부분에서 광범위하게 사용된다고 생각해요. 사람마다 의견이 다를 수 있습니다ㅎㅎ)

src/main/java/racingcar/ConsoleRacingCarApplication.java Outdated Show resolved Hide resolved
src/main/java/racingcar/controller/WebCarController.java Outdated Show resolved Hide resolved
src/main/java/racingcar/service/CarService.java Outdated Show resolved Hide resolved
src/main/java/racingcar/domain/RacingGame.java Outdated Show resolved Hide resolved
src/main/java/racingcar/exception/BadRequestException.java Outdated Show resolved Hide resolved
src/main/java/racingcar/service/CarService.java Outdated Show resolved Hide resolved

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

서비스, 도메인 테스트는 꼭 추가 부탁드려요!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

테스트 추가 완료하였습니다.
controller와 Service, DAO를 테스트하는데
적절한 도구를 사용하지 못한 것 같습니다 ㅠㅠ

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분은 코멘트 남길게요!

@junpakPark
Copy link
Member Author

junpakPark commented Apr 22, 2023

그 외

  • console application과 web application의 중복 코드 제거하라는 요구사항에 대해
    여러가지 방법이 있는 것 같습니다. 저 같은 경우는 Console과 Web에 대해 형태가 다를 뿐 둘 다 클라이언트라고 생각해서, DB를 동일하게 사용하는 방법으로 접근했는데요.
    해당 접근이 적절한 접근이 맞았을까요?
  • DataSourceConfig 클래스가 dao 패키지 안에 있어도 괜찮은가요?
  • H2 데이터베이스가 UNSIGNED 키워드를 지원하지 않아, 테스트 시 오류가 발생하여 테이블에서 제외했습니다.
  • 추가적으로, RacingRandomNumberGenerator에 대해
    @configuration으로 Bean 등록 해주었습니다가 @component로 변경해주었습니다.

@bean

  • 빈 생성 시, 더 많은 요소를 설정할 수 있음.
  • 싱글톤이 아님

@component

  • 자동 컴포넌트 스캔 가능

  • 싱글톤

저의 사용법을 보았을 때, Component로 지정하는 것이
더 적절하다고 생각했기 때문입니다.

Copy link

@aegis1920 aegis1920 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 준팍! 빙봉입니다 🙂

요구사항 만족하도록 잘 작성해주셨습니다! 👍 👍
다만 테스트가 깨지는 부분이 꽤 있어서 이 부분 다시 확인 좀 부탁드릴게요!

궁금하신 점이 있다면 언제든 DM 주세요~

import racingcar.dto.GamePlayResponseDto;
import racingcar.service.CarService;

@ExtendWith(SpringExtension.class)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 어노테이션을 사용하신 이유가 있을까요?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SpringBootTest에 이미 해당 어노테이션이 적용되어있었군요 ㅠㅠ

@ExtendWith(SpringExtension.class)를 사용할 경우
의존성 주입, 트랜잭션 관리 등 Spring 기능을 사용 가능하다고 알고 있습니다!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test 패키지에 대해서 테스트 한 번 돌려봐주세요! 깨지는 테스트가 꽤 있습니다!!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

스프링 관련 테스트가 다 🎉 터져버렸네요...🥲

Copy link
Member Author

@junpakPark junpakPark Apr 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 문제점 : 테이블이 생성된 상태에서 테이블 재생성 시도
    • 테이블 SQL에 IF NOT EXISTS 구문 추가하여 해결하였습니다.
  2. 문제점 : 테스트 간 테이블 공유로 인한 테스트 결과 예측 불가
    • DAO test들은 @JdbcTest 내부의 @Transactional 덕분에 

      테스트 완료 후 롤백이 되어 문제가 발생하지 않았습니다.
      
- WebControllerTest의 결과가 DB에 남아있어서 통과되지 않았습니다.
      
- WebControllerTest의 경우 @Transactional이 적용되지 않았습니다.

      
- @BeforeEach@AfterEach로 테이블의 내용을 삭제해주었습니다.
      

- 이 과정에서 외래키 사용의 문제점에 대해 몸으로 체험할 수 있었습니다.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 그 문제에 대한 이유까지 잘 설명해주셨네요 👍 👍
  • 테스트 완료 후 롤백에 대한 문제를 해결하는데는 다양한 방법이 있습니다. 그 방법들에 대해서 크루들과 함께 이야기해보는 걸 추천드려요!
  • 특히 @Transactional이 테스트에 붙어있으면 프로덕션 코드에 @Transactional이 존재하지 않더라도 의도치 않게 테스트가 통과할 수 있으니 이 점 유의해주세요!

src/test/java/racingcar/service/CarServiceTest.java Outdated Show resolved Hide resolved

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분은 코멘트 남길게요!

import racingcar.strategy.RacingNumberGenerator;
import racingcar.strategy.RacingRandomNumberGenerator;

public class ConsoleRacingCarApplication {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

console application과 web application의 중복 코드 제거하라는 요구사항에 대해
여러가지 방법이 있는 것 같습니다. 저 같은 경우는 Console과 Web에 대해 형태가 다를 뿐 둘 다 클라이언트라고 생각해서, DB를 동일하게 사용하는 방법으로 접근했는데요
해당 접근이 적절한 접근이 맞았을까요?

네 맞습니다! 정확히는 DB와 Service 레이어입니다! 잘해주셨어요!

src/main/java/racingcar/dao/DataSourceConfig.java Outdated Show resolved Hide resolved
spring.datasource.driver-class-name=org.h2.Driver

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

H2 데이터베이스가 UNSIGNED 키워드를 지원하지 않아, 테스트 시 오류가 발생하여 테이블에서 제외했습니다.

음.. 오류가 발생한 커밋을 알려주실 수 있을까요? mysql 모드라면 unsinged 키워드도 지원하는 걸로 알고있어서요.

Copy link
Member Author

@junpakPark junpakPark Apr 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caused by: org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'dataSourceScriptDatabaseInitializer' defined in class path resource [org/springframework/boot/autoconfigure/sql/init/DataSourceInitializationConfiguration.class]: Invocation of init method failed; nested exception is org.springframework.jdbc.datasource.init.ScriptStatementFailedException: Failed to execute SQL script statement #1 of URL [file:/Users/junpak/Documents/wooteco/jwp-racingcar/build/resources/main/data.sql]: CREATE TABLE IF NOT EXISTS GAME ( id BIGINT UNSIGNED NOT NULL AUTO_INCREMENT, winners VARCHAR(50) NOT NULL, trial_count INT NOT NULL default 0, created_at DATETIME NOT NULL default current_timestamp, PRIMARY KEY (id) ); nested exception is org.h2.jdbc.JdbcSQLSyntaxErrorException: Syntax error in SQL statement "CREATE TABLE IF NOT EXISTS GAME ( id BIGINT [*]UNSIGNED NOT NULL AUTO_INCREMENT, winners VARCHAR(50) NOT NULL, trial_count INT NOT NULL default 0, created_at DATETIME NOT NULL default current_timestamp, PRIMARY KEY (id) )"; expected "ARRAY, INVISIBLE, VISIBLE, NOT NULL, NULL, AS, DEFAULT, GENERATED, ON UPDATE, NOT NULL, NULL, AUTO_INCREMENT, DEFAULT ON NULL, NULL_TO_DEFAULT, SEQUENCE, SELECTIVITY, COMMENT, CONSTRAINT, COMMENT, PRIMARY KEY, UNIQUE, NOT NULL, NULL, CHECK, REFERENCES, AUTO_INCREMENT, ,, )"; SQL statement:
CREATE TABLE IF NOT EXISTS GAME ( id BIGINT UNSIGNED NOT NULL AUTO_INCREMENT, winners VARCHAR(50) NOT NULL, trial_count INT NOT NULL default 0, created_at DATETIME NOT NULL default current_timestamp, PRIMARY KEY (id) ) [42001-214]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

마지막에서 두번째 커밋에 테이블에 UNSIGNED 붙인 케이스를 push 했습니다.
UNSIGNED 설정 시 나타나는 오류메시지는 위와 같습니다.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

커밋으로 잘 분리해주셨군요! 덕분에 분석하기 좋았습니다 👍

@JdbcTest안에 있는 @AutoConfigureTestDatabasereplace()가 있는데요. default가 Replace.ANY로 되어있습니다.

Replace the DataSource bean whether it was auto-configured or manually defined.

자동으로 구성하든 수동으로 구성하든 무시하고 데이터베이스 소스를 교체한다는 뜻입니다. 그래서 H2 데이터소스를 쓰고 있기 때문에 EmbeddedDatabaseConnection.H2가 들어갔고 여기에는 MODE=MYSQL이 존재하지 않습니다. (jdbc:h2:mem:%s;DB_CLOSE_DELAY=-1;DB_CLOSE_ON_EXIT=FALSE)

@JdbcTest를 사용하면서 기존 데이터베이스 소스를 사용하고 싶다면 @AutoConfigureTestDatabase(replace = AutoConfigureTestDatabase.Replace.NONE)를 함께 사용해보세요. 그러면 기존 데이터소스를 들고와서 될겁니다.

e.g.

@JdbcTest
@AutoConfigureTestDatabase(replace = AutoConfigureTestDatabase.Replace.NONE)
class PlayerDaoTest {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이런 부분은 어떻게 분석하셨는지 여쭤봐도 될까요?
혹시 디버거를 쓰셨다면 어느 포인트에 브레이크 포인트를 찍으셨는지도 궁금하고,
그 브레이크 포인트를 찾은 방법도 궁금합니다!!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵!

  1. H2 데이터베이스가 UNSIGNED 키워드를 지원한다는 걸 검색하다 봄
  2. 만약 해당 오류로 터져야했다면 테스트가 아닌 기본 스프링부트를 실행할 때도 터졌어야했는데 안 터졌네?
  3. 테스트 어노테이션에서 뭔가 있을까?
  4. 자동으로 설정해주는 게 있지 않을까?
  5. 오 replace라는 게 있네?
  6. 기본 조건이 뭐지?

이런 식으로 찾았습니다ㅎㅎ 브레이크 포인트보단 로그에 mode가 없는 채로 url이 찍혀서 나오더라구요 🙂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

감사합니다 빙봉!ㅎㅎ

import org.springframework.stereotype.Component;

@Component
public class RacingRandomNumberGenerator implements RacingNumberGenerator {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

추가적으로, RacingRandomNumberGenerator에 대해
@configuration으로 Bean 등록 해주었습니다가 @component로 변경해주었습니다.

  • RacingRandomNumberGenerator과 랜덤한 숫자를 생성해주는 클래스가 스프링에 관리되어야 할 이유가 있을까요?
  • 현재 @Component를 붙였음에도 불구하고 의존성 주입을 사용하지 않고 있어요! 확인 부탁드립니다!
  • @Configuration안에도 @Component가 들어있는데요. 이와 별개로 @Configuration@Component의 차이를 알아두면 좋을 것 같아요!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

테스트의 용이성을 위하여 RacingRandomNumberGenerator를 최대한 밖에서 주입해주고 있습니다.

랜덤값은 통제가 어렵기 때문에 TestNumberGenerator를 이용해서 원하는 값을 생성하도록 했습니다.

하지만 이 RacingRandomNumberGenerator를 어디까지 빼야할지 감이 오지 않아서, CarService의 필드로 선언하고,
@Component를 부착하여 주입해주고 있었습니다.

그 덕분에 CarService 까지는 TestNumberGenerator를 이용해서 테스트가 가능했습니다.

그러나 WebCarController에서 TestRestTemplate을 이용하자,
제가 테스트를 위해 생성한 CarService에서 처리하는 것이 아닌
다른 CarService에서 요청이 처리되고 있었습니다.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

제가 궁금한 것은 다음 두가지 입니다.

  1. 테스트 용이성을 위해 RacingRandomNumberGenerator를 어디까지 빼야하는 것일까?
  2. 도메인 내부에서 관련 테스트가 끝났으니, 이 결과를 신뢰를 하고 그 이상의 계층 테스트에서는 다른 방식으로 테스트 성공 여부를 점검하는 것이 맞는 것일까?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 윽.. Web에서 new RacingRandomNumberGenerator()을 하는 줄 알고 제가 착각했습니다 😭
  • 일단, 2번째 질문에서

도메인 내부에서 관련 테스트가 끝났으니, 이 결과를 신뢰를 하고 그 이상의 계층 테스트에서는 다른 방식으로 테스트 성공 여부를 점검하는 것이 맞는 것일까?

네 저는 그렇게 생각해요. 그래서 도메인 내부 테스트가 매우 중요하다고 인지하고 있고요. 만약 신뢰하지 못한다면 그 자체로 제대로 된 테스트가 아니지 않을까요?

테스트 용이성을 위해 RacingRandomNumberGenerator를 어디까지 빼야하는 것일까?

RacingRandomNumberGenerator의 역할을 생각해보면 어디까지 빼야할 지 이해가 쉬울 것 같은데요. 개인적으로 레이싱을 위한 랜덤 숫자 생성 역할은 도메인 레이어가 가장 적절할 것 같습니다. 조금 밖으로 뺀다 치면 서비스레이어까지 뺄 수도 있을 것 같고요.

그리고 레이싱을 위한 랜덤 숫자 생성 역할이라고 한다면 스프링 컨테이너에서 관리되기 보단 도메인의 유틸 클래스에 가장 어울린다고 생각이 들긴 해요. (domain.utils 이런 식으로 패키지에 안에 만들 것 같습니다)

Copy link

@aegis1920 aegis1920 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 준팍! 빙봉입니다 🙂

주말에 미션하시느라 고생 많으셨습니다ㅎㅎ 피드백도 이유와 함께 반영해주셔서 더욱 좋았습니다. 궁금증한 점도 잘 정리된 채로 코멘트 달아주셔서 보기 편했어요 👍

지금 프로젝트 구조가 딱 스프링의 기본 형태같은데요. application.propretieslogging.level.root=debug를 추가해서 스프링이 시작할 때 로그를 분석해 보시면 스프링 공부에 도움이 많이 되실 것 같습니다. (중간에 스프링 디버깅 관련 질문이 있어 도움이 되실까 싶어 남겨요ㅎㅎ)

고생하셨고 웹 자동차 경주는 여기서 머지하겠습니다! 수고하셨어요! 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants