[2단계 - 웹 자동차 경주] 우르(김현우) 미션 제출합니다. #135

java-saeng · 2023-04-18T06:56:59Z

안녕하세요 빙봉 !!

이번 미션에서는 게임 내용을 조회하는 API 를 만들어보았습니다.
그래서 아래와 같이 mock data를 넣어두었습니다.

아래는 제가 미션을 진행하면서 궁금한 점들을 적어보았습니다.
적다보니 이렇게 길어질 줄 몰랐습니다,, 혹시 몰라서 노션 링크 첨부하겠습니다!

대부분 정답이 있는 것이 아니라서 많이 애매하시겠지만, 빙봉의 생각을 말씀해주시면 많은 것들을 배울 수 있을 것 같습니다!!

이번 리뷰도 잘 부탁드립니다!! 감사합니다 :)

DTO private 생성자

package racingcar.controller.dto;

import javax.validation.constraints.Positive;
import javax.validation.constraints.Size;

public class GameInfoRequest {

    @Size(min = 2)
    private String names;

    @Positive
    private int count;

    private GameInfoRequest() {
    }

    public String getNames() {
        return names;
    }

    public int getCount() {
        return count;
    }
}

이렇게 하면 RaceResultService에 대한 Unit Test를 작성하기 어렵게 됩니다,,

왜냐하면 GameInfoRequest가 없으면 RaceResultService#createRaceResult 단위 테스트가 아예 불가능하기 때문입니다.

리뷰 코멘트 에서는 자신 있게 private 생성자를 사용한 이유가 다른 개발자가 DTO인지 모르고 생성할 수 있는 가능성을 아예 닫아두기 위해서 사용했다고 했습니다.

결국 트레이드 오프를 생각하면

테스트 용이하지만 생성자를 public 으로 열어두기 vs 테스트 할 수 없지만 private 로 막아두기 중 선택할 수 있습니다.

전자를 먼저 살펴보겠습니다. 결국 테스트를 위해서 프로덕션 코드를 변경하는 셈입니다. 컨트롤러에서 데이터를 받을 때 Jackson이 리플렉션을 사용하기 때문에 굳이 생성자를 사용하지 않습니다. 하지만 Service Unit Test(통합 테스트도 마찬가지)를 진행하면서 해당 DTO를 만들지 못하기 때문에 테스트가 진행되지 않는다.

후자를 보면 private 로 막은 것 중 하나의 이유가 다른 개발자가 뭘 모르고 DTO를 생성하는 경우가 있다고 했다. 하지만 이 부분은 xxxRequest, xxxResponse 등 대부분 개발자 사이에서 컨벤션과 같이 DTO를 사용하는 경우가 많습니다. 아니면 프로젝트를 진행하면서 컨벤션으로 정하면,,?? 아무튼 xxxRequest는 DTO로 컨벤션을 정하고 사용하게 되면 public 으로 열어두어도 괜찮지 않을까 라는 생각이 듭니다.

레벨 1 때는 절대 테스트 때문에 프로덕션 코드를 변경하면 안된다고 생각했습니다.

하지만 그 때는 도메인에 관한 핵심, 비즈니스 로직이기 때문에 그렇지만 DTO 가 핵심 비즈니스 로직은 아니기 때문에 가능하지 않을까? 라는 조심스러운 의견을 내봅니다,,

MockMvc(feat @WebMvcTest, standaloneSetup)

정답은 없겠지만 제가 생각한 unit test란 프레임워크에 종속적이지 않고 테스트를 하는 것이라 생각합니다. 그래서 @WebMvcTest를 사용하게 되면 결국 스프링이 컴포넌트 스캔을 통해 @controller, @ControllerAdvice, Converter … 등과 같은 빈들을 등록하게 됩니다.

즉, 스프링 컨테이너가 띄어짐으로써 테스트가 진행되므로 제가 생각한 단위테스트와는 맞지 않다는 생각이 들어서 저는 standaloneSetup을 사용했습니다.

Annotation that can be used for a Spring MVC test that focuses only on Spring MVC components.
Using this annotation will disable full auto-configuration and instead apply only configuration relevant to MVC tests (i.e. @controller, @ControllerAdvice, @JsonComponent, Converter/GenericConverter, Filter, WebMvcConfigurer and HandlerMethodArgumentResolver beans but not @component, @service or @repository beans).

단위 테스트는 OO이다 라고 정확하게 말하기 어렵다고 알고 있습니다. TDD가 어려운 것처럼 느껴졌던게 “어떤게 단위 테스트인가?” 명확하게 기준이 없어서이기도 하고요.

그래서 제가 생각한 단위 테스트는 위와 같은데 빙봉의 고견을 듣고 싶습니다!

Long? long?

public String test(@PathVariable("id") Long id) {}

이렇게 데이터를 받을 때 Long이 아닌 long 도 가능하는데 long을 받는게 나을까 Long 을 받는게 나을까라는 생각을 했습니다.

모던 자바 인 액션에서 봤을 때 wrapper 클래스의 연산과 primitive 연산 속도가 약 4배 차이가 난다고 본 적이 있습니다. 그러면 이럴 때는 Long을 받는게 나을까 long을 받는게 나을까? null 인 경우에도 있어서 Long 을 받는게 나을까?

연산속도도 중요하지만 primitive와 wrapper 의 가장 큰 차이를 보면 nullable 입니다.

id 값이 주어지지 않을 때, Long 이면 null, long 이면 0 이 들어오게 됩니다. 0과 null 은 매우 다르기 때문에 저는 연산속도 보다는 의미가 더 중요하다고 생각이 들어서 Long 을 사용할 것 같습니다.

빙봉은 Long, long 중에 어떤 것을 사용하시는 편이실까요?

mockito 어노테이션을 사용하지 않는 이유

프레임워크의 어노테이션에 종속적이지 않고 일반적인 자바 코드로 테스트를 이해하는 것이 더 편한 것 같아서 어노테이션을 사용하지 않고, 코드를 작성할 수 있으면 어노테이션을 잘 사용하지 않는 편입니다.

어노테이션이 부려주는 마법이 어떻게 동작하는게 알기 때문에 굳이 마법을 부리지 않고, 제가 코드로 작성한다는 뜻으로 이해해주셨으면 될 것 같습니다.

혹시 빙봉은 위의 제 의견을 들으시고 납득이 되시나요,,?

Service layer에서 mockito 를 사용한 이유

Service layer는 비즈니스 로직을 포함하고 있는 layer입니다.

그리고 저는 비즈니스 로직만을 테스트하고 싶어합니다.

하지만 Service에서는 repository 에 접근을 하게 되는데, repository 접근에 관한 로직은 배제하고 비즈니스 로직만을 테스트 하고 싶어서 repository를 mock으로 설정하여 Service layer의 순수한 java code 만을 테스트 할 수 있다고 생각합니다.

서비스에선 단위 테스트, 통합 테스트 ? 컨트롤러에서는 단위 테스트, 통합 테스트?

팀바팀 일 것 같다라는 생각이 듭니다.

하지만 제가 결정할 수 있다면 서비스에서는 통합 테스트를 진행하고, 컨트롤러에서 단위 테스트를 진행할 것 같습니다.

왜 서비스에서 통합 테스트를 진행하는가?

먼저 Service layer 관점에 대해서 생각해보자. Service는 도메인에 관련한 비즈니스 로직들을 처리하고, Service와 도메인이 1:1 로 맵핑되는 것이 아닙니다. 즉, 여러 도메인에 대한 정보들을 취합하고 presentation layer에게 보내는 역할을 합니다. 그러기 때문에 많은 의존성이 생길 수 있습니다. 그걸 하나하나 모킹하여 스터빙하는 것이 리소스(코드 리소스, 시간 등)가 많이 들 것 같다는 생각이 듭니다.

왜 컨트롤러에서 단위 테스트를 진행하는가?

먼저 Controller 에서는 어떤 테스트를 하고 싶은가를 먼저 생각해보자.

아래 코드에서 어떤 것을 테스트 할 것인가?

@PostMapping("/plays")
public RaceResultResponse registerRaceResult(@Validated @RequestBody final GameInfoRequest gameInfoRequest) {
    return raceResultService.createRaceResult(gameInfoRequest);
}

먼저, URL과 HTTP Method 가 제대로 맵핑되어있는지를 확인할 것이다.

content-type을 JSON 으로 보낼 수 있는지, 그리고 HTTP message body 에 GameInfoRequest를 보낼 수 있는지 확인한다.

그로 인해 HTTP 상태코드가 200이 나오고, 응답값으로 RaceResultResponse가 HTTP message body에 담길지 확인한다.

결국 위와 같은 검증을 할 때 굳이 스프링 컨테이너를 띄워야할까? 라는 생각이 듭니다.

실질적으로 RaceResultService 에 대해서만 의존을 가지기 때문에 모킹해주는 것이 Service layer보다 덜 하게 됩니다.

그래서 저는 서비스에서는 통합 테스트, 컨트롤러에서 단위 테스트를 하는 것이 괜찮지 않을까 라는 생각입니다.

빙봉은 어떻게 생각하시나요!?

service → controller 역참조?

// RaceResultService 

public RaceResultResponse createRaceResult(final GameInfoRequest gameInfoRequest) {

    final String names = gameInfoRequest.getNames();
    final int tryCount = gameInfoRequest.getCount();

    final RacingCars racingCars = moveCars(names, tryCount);

    final RaceResultRegisterRequest raceResultRegisterRequest =
            raceResultMapper.mapToRaceResult(tryCount, racingCars);

    final int savedId = raceResultDao.save(raceResultRegisterRequest);

    carService.registerCars(racingCars, savedId);

    final List<CarStatusResponse> carStatusResponses = raceResultMapper.mapToCarStatus(racingCars);

    return new RaceResultResponse(raceResultRegisterRequest.getWinners(), carStatusResponses);
}

현재 RaceResultService에서 GameInfoRequest DTO 클래스 의존성을 가지고 있습니다.

하지만 이렇게 되면 service → controller 역류 참조를 하고 있게 됩니다.

3 tier architecture에서 controller → service → repository 로 가게 되는데, 현재 service → controller 의존성을 가지게 됩니다.

이를 해결하는 방법은 controller에서 DTO를 받아서 Service에서 받을 때 데이터를 하나하나 받는 방법과 DTO 의 패키지를 service 내부로 이동하는 방법이 있습니다.

하지만 데이터를 하나하나 Service 메서드 파라미터로 전달한다면 변경될 때마다 코드들이 많이 변경되기 때문에 유지보수성이 많이 떨어질 것 같아서 DTO 패키지를 Service 내부로 움직이는 판단을 했습니다.

controller 에 대한 의존성이 없어졌기 때문에 역류 참조를 하지 않고 있습니다.

왜 역류 참조를 하면 안되는걸까?

3 tier architecture에서 서비스 계층에서 컨트롤러 계층으로의 역류 참조는 책임 분리가 제대로 되지 않아서 결합도 높은 시스템이 될 가능성이 높습니다.

제가 생각한 서비스는 비즈니스 로직 구현 및 데이터 처리를 담당하고 컨트롤러는 요청 데이터를 받고 서비스 레이어에 요청하여 응답값을 내보내는 역할을 합니다.

역류 참조를 하게 된다면 한 레이어를 변경할 때 다른 레이어를 변경해야 할 수 있으므로 수정하거나 업데이트하기 어려울 수 있습니다. 또한 컨트롤러 계층의 변경 사항이 서비스 계층의 동작에 영향을 미칠 수 있고 그 반대의 경우도 있으므로 유연성과 확장성이 떨어질 수 있습니다.

그래서 레이어 간의 역할과 책임이 명시적으로 있기 때문에 유연성 있는 시스템을 만들기 위해 두 레이어를 엄격히 분리하여 역류 참조를 피해야한다고 생각합니다.

맞을까요,,??

제가 최근에 토스 slash 를 보고 제 코드를 다시 보니 역류 참조를 하고 있어서 신경을 써보았습니다.

긴 글 읽어주셔서 감사합니다 :)

aegis1920

안녕하세요 우르! 빙봉입니다 🙂

2단계 잘 구현해주셨네요! (Mock 데이터 센스 좋았습니다 👍 )
질문에 대한 답변 및 새로운 피드백 코멘트 남겼으니 확인해주세요!

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

aegis1920 · 2023-04-19T15:13:20Z

src/main/java/racingcar/dao/raceresult/RaceResultDao.java

 import org.springframework.jdbc.core.namedparam.MapSqlParameterSource;
 import org.springframework.jdbc.core.namedparam.SqlParameterSource;
 import org.springframework.jdbc.core.simple.SimpleJdbcInsert;
 import org.springframework.stereotype.Repository;
 import racingcar.dao.raceresult.dto.RaceResultRegisterRequest;
+import racingcar.domain.Car;


~~DAO에 Domain이 있는 것보단 아예 없애서 DB와 접근만 하는 클래스로 만들어보는 건 어떨까요?~~ 제네릭을 사용해서 DAO 인터페이스로 빼는 건 어떨까요? (링크)

이번 애플리케이션에서는 dao의 기능이 그렇게 공통적이지 않더라구요,, 또한, 저장에 있어서도 RaceResult는 단건 저장, Car는 batch 저장이구요.
그래서 제네릭을 사용한 인터페이스를 만들기에는 조금 애매하다고 생각이 들었습니다!!

CarDao

save(List)

findAll

RaceResult

save(RaceResultEntity)

RaceResultDao의 save에서도 파라미터를 List<RaceResultEntity>로 받게 만들면 단건 저장도 가능하지 않을까요?

src/main/java/racingcar/service/RaceResultService.java

src/main/java/racingcar/controller/RacingCarWebController.java

src/main/java/racingcar/service/RaceResultService.java

src/main/java/racingcar/controller/RacingCarWebController.java

src/main/resources/sql/data.sql

src/test/java/racingcar/service/RaceResultServiceIntegrationTest.java

src/test/java/racingcar/service/CarServiceIntegrationTest.java

src/main/java/racingcar/service/RaceResultService.java

- SqlParameterSourceUtils 를 사용하면 배열을 한번 더 감싸기 때문에 executeBatch 에서 value 를 찾을 수 없었음

- exception message custom

- table 스펙 변경에 따라 Dao 수정 - Dao 수정에 따른 Serivce & test 수정

java-saeng · 2023-04-22T09:16:46Z

안녕하세요 빙봉 !!

코멘트 중에 DAO 인터페이스 추가는 조금 어려움이 있더라구요,, 자세한 내용은 코멘트 달았습니다.
이번 리뷰도 잘 부탁드립니다!!

exception 처리

Service의 createRaceResult()를 실행하던 도중 DB연결이 끊겨서 안될 수도 있을텐데 try catch가 존재하지 않아서 말씀드렸습니다. 커넥션이 끊겨도 그에 대해 커넥션 에러를 리턴하진 않아서요. 보통 예외를 감싸서 리턴하고 ExceptionHandler에서 캐치 후 공통 예외 응답으로 처리하게 됩니다.

제가 알고 있기로는 Spring에서 DB 벤더 사에 상관없이 database access 예외를 추상화시켜주는 것으로 알고 있습니다.

그래서 DataAccessResourceFailureException 를 처리해주었습니다. 추가로 DB 연결은 개발자가 확인해야하므로 협업할 때는 코드에 대해서 정하겠지만, 제가 그냥 5xx 중에 중복되지 않는 코드로 임의 설정했습니다.

빙봉이 try-catch 를 말씀하셨는데 ExceptionHandler 로 커버가 되지 않는 경우가 있는건가요?

controller 단위 테스트

사실 컨트롤러 테스트는 그렇게 중요한 테스트가 아니라고 생각들긴 해서... 단순히 요청과 응답을 테스트하는 거라면 굳이 작성하지 않았을 것 같습니다.

이번에 작성해보니 단위 테스트는 조금 많이 의미가 없어지는 듯한 느낌이에요.. 그냥 메서드 호출이 잘됐니? 라고 테스트하는 듯한,,? 왜냐하면 body 에 객체를 직렬화, 역직렬화를 모두 ObjectMapper 를 사용해서 제가 직접해주는데 의미를 못 느끼겠더라구요..

그래서 controller는 어노테이션이 대부분 해주는거라 스프링 컨테이너를 올려야 하는 듯합니다. DB exception 테스트 할 때 RestControllerAdvice가 제대로 동작하는지 모든 빈을 올리지 않는 @WebMvcTest를 했습니다.

aegis1920 · 2023-04-23T04:02:34Z

src/main/resources/sql/data.sql

+insert into CAR
+values (1, '우르', 9, 1, 1, current_timestamp);
+insert into CAR
+values (2, '빙봉', 10, 1, 0, current_timestamp);


직접 데이터를 넣으신거라 잘못 넣으셨을거라고 생각하는데요. bool type은 true가 1, false가 0입니다. 아래처럼 수정이 필요합니다.

Suggested change

insert into CAR

values (1, '우르', 9, 1, 1, current_timestamp);

insert into CAR

values (2, '빙봉', 10, 1, 0, current_timestamp);

insert into CAR

values (1, '우르', 9, 1, 0, current_timestamp);

insert into CAR

values (2, '빙봉', 10, 1, 1, current_timestamp);

기왕 바꾸는 거 우르가 우승하도록 하시죠ㅎㅎ

Suggested change

insert into CAR

values (1, '우르', 9, 1, 1, current_timestamp);

insert into CAR

values (2, '빙봉', 10, 1, 0, current_timestamp);

insert into CAR

values (1, '우르', 10, 1, 1, current_timestamp);

insert into CAR

values (2, '빙봉', 9, 1, 0, current_timestamp);

test에 있는 데이터는 수정됐는데 main 은 안되있네요,, 빙봉이 이기는 것으로 수정하겠습니다 !! ㅋㅋㅋ

이렇게 헷갈리는거면 아래의 빙봉 코멘트처럼 1, 0 보다는 y, n으로 구분해주는게 더 가독성이 좋아보입니다.

aegis1920 · 2023-04-23T04:12:46Z

src/main/resources/sql/schema.sql

+    name           VARCHAR(50) NOT NULL,
+    position       INT         NOT NULL,
+    race_result_id BIGINT      NOT NULL,
+    winner         BOOL        NOT NULL,


boolean type 성격의 데이터를 bool 타입으로 지정하면 0과 1로 표현하게 되는데요. DB마다 booltype을 다르게 인식하는 문제, 직관적이지 않은 문제로 char(1)로 설정한 후, 'Y', 'N' 방식으로 표현하기도 합니다. 성능적으로 차이가 있긴 하겠지만 이로 인한 성능 차이를 크게 보진 않습니다.

이는 취향 문제일 거 같긴한데요. 필드 또한 좀 더 직관적으로 winner가 아닌 winner_yn과 같이 좀 더 직관적으로 표현합니다.

yn으로 명시적으로 해준다면 다른 개발자가 봤을 때, 굳이 DB가서 데이터 타입을 보지 않아도 될 것 같아요. 말씀대로 yn으로 수정해주는게 좋아보입니다.

커밋 링크

aegis1920 · 2023-04-23T04:22:11Z

src/main/java/racingcar/service/mapper/CarMapper.java

+import java.util.List;
+import java.util.stream.Collectors;
+
+@Component


CarMapper를 @Component로 선언하신 이유가 있을까요?

CarMapper라는 클래스가 스프링 컨테이너에서 관리되도록 할 이유가 있을까요?

mapToCarEntitiesFrom() 메서드에 static을 선언해서 유틸 클래스의 성격을 갖도록 하는 건 어떨까요?

@aegis1920

빙봉 말씀대로 Mapper는 인스턴스 필드도 가지지 않기 때문에 싱글톤으로 관리할 필요도 없고, 빈으로 관리함으로써 스프링 컨테이너와 라이프 사이클을 같게 유지할 필요도 없을 것 같아요.

그래서 유틸리티 클래스로 만들고 났는데, 단위 테스트에서 stubbing(mockito)은 static method에서는 동작하지 않더라구요,, ㅋㅋ 그래서 빙봉 덕분에 추가로 MockedStatic 라는 것도 알게 됐습니다 ㅋㅋ

커밋 링크

미션과 같이 작은 애플리케이션에서는 빈이 많지 않아서 등록해도 상관없을텐데, 이 생각을 확장해보면 "스프링 빈이 많으면 좋지 않을까?" 라는 생각을 해봤어요.

단적인 예로 빈이 많으면 그만큼 스프링 컨테이너가 하는 일이 많아져서 빌드가 길어진다.
나중에 모를 순환참조가 생길 수 있다.

그런데 생성자 주입으로 인해 컴파일 타임 때 걸러지지 않을까?

순환참조 생기면 스프링이 예쁘게 로그도 띄워줘서 그렇게 큰 문제가 아니라 생각

정도입니다.

제가 아직 볼륨이 큰 애플리케이션을 만져보지 못해서 잘 알지 못하지만 혹시 빈이 많아져서 어려움을 겪으셨던 적이 있으신가요??

미션과 같이 작은 애플리케이션에서는 빈이 많지 않아서 등록해도 상관없을텐데, 이 생각을 확장해보면 "스프링 빈이 많으면 좋지 않을까?" 라는 생각을 해봤어요.

일단.. 빌드가 길어진다는 것(빈 초기화 시간이 늘어난다는 것)만으로도 큰 단점이라고 생각합니다. 빈 초기화 시간이 늘어나면 프로덕션 뿐만 아니라 통합테스트 시에도 그 시간이 늘어나게 될테고요.

빈이 늘어난다는 거 자체가 스프링 컨테이너가 관리할 일이 더 많아진다는 것이고 또 의존성이 복잡해질텐데 그러면 유지보수에 어려움을 겪을 일 또한 많아질 수 있다는 걸 의미하지 않을까요?

제가 아직 볼륨이 큰 애플리케이션을 만져보지 못해서 잘 알지 못하지만 혹시 빈이 많아져서 어려움을 겪으셨던 적이 있으신가요??

빈이 많아져서 어려움을 겪었다기보단.. 빈은 적으면 적을수록 좋은 거라고 생각들어요. 그리고 빈이 많아지는 것과 순환 참조가 큰 관계가 있는지는 잘 모르겠어요. 빈이 많아지더라도 단방향으로 잘 참조되어있다면 순환 참조가 없지 않을까요?

아 갑자기 생각났는데 예전에 순환참조 관련해서 이런 일이 있었어요. 기존 레거시 코드가 setter 주입으로 되어있어서 모두 생성자 주입으로 바꿨더니 그때 순환참조로 다 터져버린 적이 있었습니다ㅎㅎ 이때 빈이 많지 않고 적었더라면 원인이 되는 빈을 빠르게 파악하고 개선할 수 있었을텐데 이런 생각은 드네요!

src/test/java/racingcar/service/RaceResultServiceIntegrationTest.java

aegis1920 · 2023-04-23T05:08:19Z

src/test/java/racingcar/service/RaceResultServiceIntegrationTest.java

+
+@SpringBootTest
+@DisplayName("RaceResultService Integration Test")
+class RaceResultServiceIntegrationTest {


혹시 테스트를 test 패키지 대상으로 실행시켜보셨을까요? 테스트는 실행순서를 보장하지 않아서 아래의 경우 test_createRaceResult() -> test_searchRaceResult() 순서로 테스트할 경우 실패하게 됩니다.

test_createRaceResult() 테스트가 먼저 실행되어 test_searchRaceResult() 에서 사이즈 차이로 실패합니다.

@TestMethodOrder(MethodOrderer.OrderAnnotation.class)와 @Order를 통해 직접 순서를 다르게 해서 테스트해볼 수도 있습니다.

테스트는 실행 순서와 상관없이 독립적으로 테스트가 통과되도록 보장되어야 합니다.

@BeforeAll, @BeforeEach 등을 잘 활용해보세요!

@aegis1920

넵 전체 테스트를 돌렸었는데 우연히 search부터 돌았던 것 같습니다.

저도 테스트 실행 순서와 상관없이 독립적으로 테스트가 통과되야한다고 생각합니다.

해당 테스트에서는 @Transactional 을 붙여줌으로써 해결할 수 있습니다.

하지만 예전에 테스트 컨테이너 + 랜덤포트를 사용하게 된다면 트랜잭션이 보장되지 않아서 테스트의 멱등성을 보장하는데 어려움이 있었습니다. 그래서 @Sql 을 사용하여 테스트 메서드가 실행되기 전에 sql 스크립트 파일을 재실행해주어서 해결할 수 있었어요.

커밋 링크

멱등성은 해결할 수 있지만 이 방법은 애플리케이션 볼륨이 커질수록, 즉, 테이블이 많아지고 그에 대한 데이터가 많아질수록 시간이 엄청 오래걸린다는 단점이 있습니다.

빙봉이 말씀해주신 @BeforeAll, @BeforeEach 또한 매 테스트 메서드가 끝날 때마다 초기화를 해주는 것으로 비슷하다고 생각해요.

혹시 다른 방법이 있는건지? 아니면 이게 최선의 방법인지 궁금합니다.

제가 생각한 방법은 현재 schema.sql 에

DROP TABLE IF EXISTS RACE_RESULT; DROP TABLE IF EXISTS CAR;

이 존재하는데, truncate 를 써서 drop table을 하지 않음으로써 schema.sql 을 재실행은 막을 수 있을 것 같아요. 그러면 create table 연산은 막을 수 있어서

@Sql(value = "/sql/schema.sql", executionPhase = Sql.ExecutionPhase.BEFORE_TEST_METHOD)

이 부분이 생략될 수 있어서 더 빠를 수 있을 것 같습니다.

빙봉은 혹시 테스트 결과의 멱등성을 유지하기 위해서 어떤 방법을 쓰셨는지 궁금합니다.

말씀대로 @Transactional을 써서 테스트가 끝나면 rollback하도록 해줄 수 있습니다. 저는 개인적으로 테스트에는 @Transactional을 붙이지 않는 편인데요. 프로덕션과 트랜잭션 범위가 달라질 수 있기 때문입니다. 테스트에서 트랜잭션이 이미 보장되어있는 상황이므로 실제로 기대하는 테스트와 달라질 수 있습니다. (e.g. 프로덕션에는 @Transactional이 없는데 테스트에는 @Transactional이 붙여서 성공하는 상황이라던지..)

테스트의 멱등성을 보장하는 방법에는 여러 방법이 있습니다. Service에서 함수 호출을 통해 지우는 방법, @Dirtiescontext를 붙이는 방법, SQL 실행 등등.. 각각 트레이드오프가 있을텐데 우르가 작성해주신 SQL 실행 방법이 직관적이기도 하고 딱히 단점도 없어서 SQL을 사용할 것 같네요.

aegis1920 · 2023-04-23T05:23:49Z

src/main/java/racingcar/global/exception/ExceptionHandlerController.java

+                             .body(DefaultExceptionResponse.from(exception));
+    }
+
+    @ExceptionHandler(DataAccessResourceFailureException.class)


빙봉이 try-catch 를 말씀하셨는데 ExceptionHandler 로 커버가 되지 않는 경우가 있는건가요?

좋은 질문입니다 👍 👍

createRaceResul() 메서드 중간에 (충분히 발생할 수 있는) throw new NullPointerException(); 를 넣으면 어떻게 될까요? ExceptionHandler에서 catch하고 있지 않기 때문에 500에러가 터지겠죠.

NullPointerException 에러로 인한 500에러 표시는 개발자가 원하던 상황일까요?

이를 방지하려면 ExceptionHandler에서 모든 에러에 대해 catch를 해야할까요?

(상황에 따라 다르겠지만) 서비스 로직은 중요하기 때문에 가급적 에러가 발생할 수 있는 부분을 try catch로 감싸고 custom exception으로 throw 한 후, exceptionhandler에서 custom exception을 기반으로 처리를 합니다. 이에 따라 return하는 메시지도 달라지고 에러 코드도 달라집니다.

더불어 에러가 발생했을 때 @Transaction의 propagation 및 상위 하위 트랜잭션에 따라서 롤백 처리 방법도 달라지기 때문에 이 부분에 대해서 많은 고민이 필요합니다.

추후 미션 진행하면서 이 고민들과 함께 코드를 작성해보시길 추천드려요!

ExceptionHandler에서 catch하고 있지 않기 때문에 500에러가 터지겠죠.

NullPointerException 에러로 인한 500에러 표시는 개발자가 원하던 상황일까요?

이를 방지하려면 ExceptionHandler에서 모든 에러에 대해 catch를 해야할까요?

(상황에 따라 다르겠지만) 서비스 로직은 중요하기 때문에 가급적 에러가 발생할 수 있는 부분을 try catch로 감싸고 custom exception으로 throw 한 후, exceptionhandler에서 custom exception을 기반으로 처리를 합니다. 이에 따라 return하는 메시지도 달라지고 에러 코드도 달라집니다.

즉, 개발자가 예상했던 상황이 아닐 때의 exception에 대한 처리를 해줘야하는데, ExceptionHandler 에서 모든 exception을 처리할 수 없기 때문에 try-catch를 사용한다고 생각하면 될까요?

말씀하신대로 모든 에러에 대해 catch 하는게 거의 불가능하다고 생각합니다.

그러면 custom exception throw를 한다해도, 그 custom exception 이 가지는 의미가 너무 포괄적이지 않을까? 라는 생각입니다. 너무 세세하게 나누게 되면 또 너무 지엽적이지 않나 라는 생각도 들구요.

Spring이 제공해주는 exception 추상화처럼 하나의 큰 exception 추상 클래스를 여러 exception 클래스로 나눠서 하는 방법이 있을 것 같기도 합니다.

그런데 exception 처리가 중요한 것은 알겠지만, 현업에서도 이렇게 exception 에 대한 처리를 세세하게 해주는 편인가요,,?

더불어 에러가 발생했을 때 @Transaction의 propagation 및 상위 하위 트랜잭션에 따라서 롤백 처리 방법도 달라지기 때문에 이 부분에 대해서 많은 고민이 필요합니다.

오,, 이 부분은 들어보기만 했습니다.

A → B 실행했을 때, 실패했을 경우 A를 모두 다 롤백할지, B만 롤백할지요.

대부분 그런데 데이터 정합성 때문에 A를 전부 다 롤백시키지 않을까라는 생각을 해봅니다,,

추후 미션 진행하면서 이 고민들과 함께 코드를 작성해보시길 추천드려요!

제가 exception 에 대한 처리를 크게 고민해본적이 없어서 혹시 어떤 방식으로 코드를 작성하시는 것을 말씀하시는지 여쭤봐도 될까요,,?

예를 들어서 이 부분에서는 NPE 가 발생할 수 있어서 Optional 로 감싼다음에 exception 을 처리할 때, 뭐 다른 것을 반환해준다든지, 그냥 무시한다든지, custom exception 을 던져줄지 이런 식의 고민을 말씀하시는 걸까요?

뭔가 exception 처리에 대해서 깊이 생각해보지 않아서 best practice 볼만한 자료가 있을까요,,?

즉, 개발자가 예상했던 상황이 아닐 때의 exception에 대한 처리를 해줘야하는데, ExceptionHandler 에서 모든 exception을 처리할 수 없기 때문에 try-catch를 사용한다고 생각하면 될까요?

개발자가 예상했던 상황이 아닐 때의 exception에 대한 처리를 해줘야하는데
이 부분은 맞는데요. ExceptionHandler에서 전체 예외를 잡기 위해서 Exception.class에 대한 ExceptionHandler도 필요하지 않을까?라는 의미에서 작성했습니다! 장바구니 미션에서 한 번 구현해보세요!ㅎㅎ

제가 exception 에 대한 처리를 크게 고민해본적이 없어서 혹시 어떤 방식으로 코드를 작성하시는 것을 말씀하시는지 여쭤봐도 될까요,,? 예를 들어서 이 부분에서는 NPE 가 발생할 수 있어서 Optional 로 감싼다음에 exception 을 처리할 때, 뭐 다른 것을 반환해준다든지, 그냥 무시한다든지, custom exception 을 던져줄지 이런 식의 고민을 말씀하시는 걸까요? 뭔가 exception 처리에 대해서 깊이 생각해보지 않아서 best practice 볼만한 자료가 있을까요,,?

네 말씀하신 그대로입니다! 어떤 경우에는 무시하는 경우도 있고, Custom Exception을 던지는 경우도 있고 처음부터 감싸서 다른 걸 반환하는 경우도 있습니다. 다만 이 부분은 도메인마다 천차만별입니다.

음.. 예를 들어, 우르 결제 시스템, 외부 제휴사 할인 시스템이 있고 중첩 트랜잭션을 사용했다고 가정했을 때

우르 결제 시스템에서 결제 진행

외부 제휴사 할인 시스템 승인 요청 성공

외부 제휴사 할인 시스템 이력 생성

우르 결제 시스템에서 결제 진행 중 실패 (네트워크 오류 등..)

이때 네트워크 오류로 인해 결제는 실패했지만 외부 제휴사 할인 시스템에서 생성한 이력은 남겨둬야겠죠. 외부 제휴사 이력은 남겨둬야 추후 확인이 가능하니까요. 이때 바라는 건 할인 시스템 이력 생성은 성공했으나 결제는 실패한 상황일 거예요.

실무는 훨씬 복잡하지만 이와 비슷한 상황에서 어떻게 트랜잭션을 관리하고 예외에 어떻게 대해서 처리해야될 지를 이야기한 것이었습니다ㅎㅎ 이제 장바구니 미션하시고 계실테니 천천히 고민해보세요!

aegis1920 · 2023-04-23T05:26:16Z

src/main/java/racingcar/dao/raceresult/RaceResultDao.java

 import org.springframework.jdbc.core.namedparam.MapSqlParameterSource;
 import org.springframework.jdbc.core.namedparam.SqlParameterSource;
 import org.springframework.jdbc.core.simple.SimpleJdbcInsert;
 import org.springframework.stereotype.Repository;
 import racingcar.dao.raceresult.dto.RaceResultRegisterRequest;
+import racingcar.domain.Car;


RaceResultDao의 save에서도 파라미터를 List<RaceResultEntity>로 받게 만들면 단건 저장도 가능하지 않을까요?

aegis1920

안녕하세요 우르! 빙봉입니다 🙂

주말까지 정말 고생 많으셨어요!
1, 2단계에 거쳐서 고민할 거리가 많은 피드백을 드렸는데요. 그럼에도 불구하고 왜 그렇게 적용했는지 우르의 생각을 적어주셔서 좋았습니다 👍

요구사항은 충분히 만족하셔서 웹 자동차 경주는 여기서 머지하겠습니다! 수고하셨어요!

java-saeng added 13 commits April 13, 2023 11:54

refactor : schema 스펙 변경

cd77c2d

feat : bean validation 추가

72fab15

refactor : column 명 변경

c40b2a1

feat : 게임 이력 조회 기능 구현

585644b

feat : mock data 추가 및 data 초기화 path 변경

2f1278b

refactor : RacingCarController, RaceResultService 자동차 움직임 method extract

450e8d7

feat : RaceResultService Unit Test 작성

a7577cd

feat : RacingCarWebController Unit Test 작성

add4aad

feat : CarService Unit Test

c7fd7e9

style : code 제출 전 reformatting

b59f890

refactor : dto 를 Controller -> Service 패키지로 변경

b4c6fc6

feat : Car 에 created_at column 추가

af5f873

feat : Service Integration Test 추가

141f18b

aegis1920 requested changes Apr 19, 2023

View reviewed changes

java-saeng added 16 commits April 20, 2023 13:05

refactor : 경기 결과 저장 시 자동차 batch insert

2c9f6e2

refactor : 경기 결과 생성일 경우 Http 상태코드 Created로 변경

781f2c1

feat : 경기 결과 생성 시 자동차 저장까지의 로직을 논리적으로 묶기 위해 트랜잭션 사용

f501bcd

fix : batch insert 제대로 수행안되어 해결

4d50307

- SqlParameterSourceUtils 를 사용하면 배열을 한번 더 감싸기 때문에 executeBatch 에서 value 를 찾을 수 없었음

feat : exception handler 기능 추가

ca6898f

- exception message custom

test : input, output 에 의존되지 않게 수정

1f39b72

feat : CarDao 에서 Entity 사용

a84ae91

feat : RaceResultDao 에서 Entity 사용

74d46a9

feat : RaceResultDao 조회에서 도메인이 아닌 Entity 사용

69faec4

refactor : 패키지 변경

f8ae605

refactor : CarDao SimpleJdbcInsert 를 인스턴스 필드로 변경

b1ab8d0

feat : CarEntity로 변경시켜주는 CarMapper 추가

e310a1f

refactor : 모든 인스턴스 필드를 담고 있는 생성자 접근 제어자를 public -> private 변경

994ac58

feat : domain 들의 움직임과 결과를 명령하는 RacingGame 추가

12317c4

feat : RacingGame 추가에 따른 CarService 변경

000d97b

feat : java 에서 제공하는 기본 exception 에 대한 handling method 추가

87f96b8

java-saeng added 7 commits April 21, 2023 21:58

refactor : Random 대신 ThreadLocalRandom 사용

4f7b056

feat : Car가 우승 여부를 가지고 있게 테이블 변경

95aebb3

feat : Car 에 winner column 추가 후 코드 변경

0f99a89

- table 스펙 변경에 따라 Dao 수정 - Dao 수정에 따른 Serivce & test 수정

feat : DB 관련 exception handler 추가

78dd809

style : 코드 제출 전 reformatting

775e9a1

refactor : 사용하지 않는 메서드 삭제

f4f9060

feat : 데이터베이스 문제 생길 시 exception test

d4c33f8

java-saeng force-pushed the java-saeng branch from 24e1c87 to d4c33f8 Compare April 22, 2023 09:01

aegis1920 reviewed Apr 23, 2023

View reviewed changes

aegis1920 approved these changes Apr 23, 2023

View reviewed changes

aegis1920 merged commit f602eea into woowacourse:java-saeng Apr 23, 2023

aegis1920 mentioned this pull request Jun 6, 2023

[2단계 - 주문 기능 구현] 디노(신종화) 미션 제출합니다. woowacourse/jwp-shopping-order#53

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2단계 - 웹 자동차 경주] 우르(김현우) 미션 제출합니다. #135

[2단계 - 웹 자동차 경주] 우르(김현우) 미션 제출합니다. #135

java-saeng commented Apr 18, 2023 •

edited

Loading

aegis1920 left a comment

aegis1920 Apr 19, 2023 •

edited

Loading

java-saeng Apr 22, 2023

aegis1920 Apr 23, 2023

java-saeng commented Apr 22, 2023

aegis1920 Apr 23, 2023

java-saeng Apr 24, 2023

aegis1920 Apr 23, 2023

java-saeng Apr 24, 2023

aegis1920 Apr 23, 2023

java-saeng Apr 24, 2023

aegis1920 Apr 30, 2023

aegis1920 Apr 23, 2023

java-saeng Apr 24, 2023

aegis1920 Apr 30, 2023

aegis1920 Apr 23, 2023

java-saeng Apr 24, 2023

aegis1920 Apr 30, 2023

aegis1920 Apr 23, 2023

aegis1920 left a comment

[2단계 - 웹 자동차 경주] 우르(김현우) 미션 제출합니다. #135

[2단계 - 웹 자동차 경주] 우르(김현우) 미션 제출합니다. #135

Conversation

java-saeng commented Apr 18, 2023 • edited Loading

DTO private 생성자

MockMvc(feat @WebMvcTest, standaloneSetup)

Long? long?

mockito 어노테이션을 사용하지 않는 이유

Service layer에서 mockito 를 사용한 이유

서비스에선 단위 테스트, 통합 테스트 ? 컨트롤러에서는 단위 테스트, 통합 테스트?

왜 서비스에서 통합 테스트를 진행하는가?

왜 컨트롤러에서 단위 테스트를 진행하는가?

service → controller 역참조?

왜 역류 참조를 하면 안되는걸까?

aegis1920 left a comment

Choose a reason for hiding this comment

aegis1920 Apr 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

java-saeng commented Apr 22, 2023

exception 처리

controller 단위 테스트

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aegis1920 left a comment

Choose a reason for hiding this comment

java-saeng commented Apr 18, 2023 •

edited

Loading

aegis1920 Apr 19, 2023 •

edited

Loading