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

[Spring 지하철 노선도 - 1,2단계] 매트(최기현) 미션 제출합니다. #218

Merged
merged 59 commits into from
May 10, 2022

Conversation

hyeonic
Copy link

@hyeonic hyeonic commented May 4, 2022

안녕하세요 앨런! 매트입니다 😃

🚀 1단계 - 지하철역/노선 관리 기능, 🚀 2단계 - 프레임워크 적용 미션 잘 부탁드립니다.

아래는 미션을 진행하며 느낀 내용과 질문들을 정리하였습니다.


1. 일일 회고

미션을 진행하며 하루를 마무리할 때 페어와 함께 회고를 진행하였습니다. 서로에 대한 고충이나 미션을 진행하며 어려운 부분을 서로 공유하며 높은 신뢰감을 쌓아갈 수 있었습니다. 아래는 실제 나눈 내용을 REAMDE에 정리하였습니다.

2. 테스트 격리?

E2E 테스트를 진행할 때 @SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT) 애노테이션 사용 시 트랜잭션이 롤백되지 않는 것을 관련 포스팅을 본 뒤 알게 되었습니다.

결국 전체 테스트를 진행할 때 DB에 데이터가 삭제되지 않고 쌓여 다른 테스트에 간섭하는 것을 확인하였습니다. 이것을 해결하기 위해 schema.sql에 해당 명령어를 추가하여 테이블 유무를 확인하고 있으면 삭제하는 방식으로 해결하였습니다.

drop table if exists STATION;
drop table if exists LINE;

앨런이 생각했을 때 이것은 적절한 테스트 격리라고 생각하시나요? 의견이 궁금합니다!

3. org.springframework.dao에서 제공하는 예외

jdbcTemplate을 활용하여 데이터 조회에 실패하거나 맞지 않는 쿼리를 요청한 경우 dao에서 제공하는 예외를 던지는 것을 확인하였습니다. 해당 예외들을 @RestControllerAdvice를 통해 예외 처리에 대한 로직을 분리하였습니다. 하지만 단순히 exception.getMessage()를 통해 클라이언트에게 반환해줄 경우 서버에서 발생한 예외 메시지를 그대로 노출할 우려가 있기 때문에 아래와 같이 명시적으로 문자열을 작성하였습니다.

@RestControllerAdvice
public class ExceptionAdviser {

    @ExceptionHandler(DuplicateKeyException.class)
    public ResponseEntity<ErrorResponse> duplicateKeyExceptionHandler() {
        return ResponseEntity.badRequest().body(new ErrorResponse("중복된 이름입니다."));
    }

    @ExceptionHandler(EmptyResultDataAccessException.class)
    public ResponseEntity<ErrorResponse> EmptyResultDataAccessExceptionHandler() {
        return ResponseEntity.badRequest().body(new ErrorResponse("해당 값이 존재하지 않습니다."));
    }
    ...
}

dao에서 제공하는 예외를 그대로 catch하는 부분이 약간은 어색하게 느껴졌습니다. DB에서 unique 제약 조건을 걸어두었기 때문에 도메인 내부에서는 특별한 검증은 진행하지 않고 있습니다. 또한 클라이언트를 위한 예외 메시지를 명시하는 부분은 ExceptionAdviser에서 진행하고 있는데 앨런이 생각했을 때 적절한 처리라고 생각하시나요??

4. service 계층의 부재

현재 로직에서는 service 계층에 대한 필요성이 느껴지지 않아 페어와의 상의 후 생성하지 않도록 결정하였습니다. 만약 여러 dao를 활용하여 일련의 트랜잭션 과정이 필요할 경우 service 계층을 도입하려 하는데 적절한 배경인지 궁금합니다!

이 밖에도 제가 놓친 부분이나 좋지 않은 습관 등 피드백 주시면 적극 반영 하겠습니다! 감사합니다 🙏

hyewoncc and others added 27 commits May 3, 2022 15:25
@hongbin-dev
Copy link

안녕하세요 매트~

테스트 격리

매트가 생각한대로 저도 테스트를 할 때, 일반적으로 데이터를 다 지우고 테스트하는편입니다.

하지만 위 스키마가 운영코드에 반영된다면 매번 테이블이 삭제하고, row가 다 날라갈 것 같아요 🥲

@sql 어노테이션으로 매 테스트실행시 truncate 시키거나, 여러 방법들이 있을 것 같다.

추가로 현재는 베이스코드에 @DirtiesContext(classMode = DirtiesContext.ClassMode.*BEFORE_EACH_TEST_METHOD*) 를 통해서 새로운 context로 실행하고 있는데요. 아마 drop table을 지워도 잘 동작할 것 같아요.

해당 drop table 코드는삭제해도 좋을 것 같아요.

org.springframework.dao에서 제공하는 예외

사실 질문이 잘 이해가 안갔습니다 🥲 어떤 곳에서 문제를 느꼈는지 잘 이해가 안갔는데요.

한가지 질문드리자면, 현재 데이터베이스를 통해서만 무결성을 유지하고 있는데요. 애플리케이션에서 중복을 확인하고 삽입하는 것과 어떤 차이가 있을까요?
장단점을 알아보면 좋을 것 같아요.

service 계층의 부재

개인적으로 서비스가 있었으면 �좋겠습니다. 제가 생각해본 단점인데요.

  • 컨트롤러가 무거워진다.
  • 트랜잭션이 보장되지 않는다.
  • 유스케이스기반으로 테스트하기가 어렵다 -> 유스케이스기반 테스트를하려면 mvc를 띄우고 테스트해야함

물론 아직 해당되지 않을 수 있지만, 기본적으로 나누는게 좋을 것 같다고 생각해요. 업무를 하다보면 매번 베이스코드에 유지보수되고 변경이 일어나는데요. 변경이 일어나고 나서, 다시 새로운 레이어를 도입하기는 비용이 클 것 같아서요.

매트의 의견도 궁금하군요~

Copy link

@hongbin-dev hongbin-dev left a comment

Choose a reason for hiding this comment

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

안녕하세요 매트!
1,2 단계 미션 잘 구현해주셨군요. 특히 회고작성하신게 인상적이었습니다 ✨
매트 질문의 답변 남겼고, 추가 코멘트와 질문 남겼습니다~ 확인해주세요

Comment on lines +9 to +10
private Line() {
}

Choose a reason for hiding this comment

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

private 👍

Comment on lines 1 to 2
drop table if exists STATION;
drop table if exists LINE;

Choose a reason for hiding this comment

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

이 부분은 코멘트처럼 지우는게 좋을 것 같습니다~

Copy link
Author

@hyeonic hyeonic May 8, 2022

Choose a reason for hiding this comment

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

언급하신 것 처럼 제거 후 test/resourcestruncate.sql을 추가하여 각 테스트를 진행할 때마다 해당 쿼리가 실행될 수 있도록 개선하였습니다!

truncate table STATION;
truncate table LINE;
@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT)
@Sql("/truncate.sql")
public class AcceptanceTest {

    @LocalServerPort
    int port;

    @BeforeEach
    public void setUp() {
        RestAssured.port = port;
    }
}

Comment on lines +19 to +20
@DisplayName("노선 관련 기능")
public class LineAcceptanceTest extends AcceptanceTest {

Choose a reason for hiding this comment

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

실패에 대한 케이스도 생각해볼 수 있을 것 같아요.
식별자의 row가 존재하지않는데 update, delete하는 행위도 있을 것 같아요.

Copy link
Author

@hyeonic hyeonic May 8, 2022

Choose a reason for hiding this comment

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

식별자의 row가 존재하지 않을 때 어떤 식으로 예외를 처리할지 많은 고민을 진행했습니다. 추가적인 스터디를 통해 HTTP와 관련된 학습을 진행하던 중멱등에 대한 키워드를 확인할 수 있었습니다.

제가 학습한 멱등의 개념은 아래와 같습니다.

멱등

한 번 호출하든 두번 호출하든 매번 같은 결과를 반환한다. 이러한 멱등은 자동 복구 메커니즘에 활용된다. 서버가 정상 응답을 주지 못했을 때 클라이언트가 같은 요청을 다시 보내도 괜찮은지에 대한 판단 근거로 활용된다. 대표적으로 GET, PUT, DELETE가 이에 해당한다.

앞서 언급하신 것 처럼 update와 delete라는 행위는 핸들러 메서드에서 @PutMapping@DeleteMapping을 통해 진행됩니다. 해당 행위들은 예외를 던지기 보다 매번 같은 결과를 반환할 것을 보장해야 한다고 판단하였습니다.

아래는 service 계층에서 작성된 updatedeleteById입니다.

@Transactional(readOnly = true)
@Service
public class LineService {
    ...
    @Transactional
    public LineResponse update(Long id, LineRequest lineRequest) {
        lineDao.findById(id)
                .ifPresentOrElse(
                        line -> lineDao.update(id, new Line(lineRequest.getName(), lineRequest.getColor())),
                        () -> lineDao.saveWithId(id, new Line(lineRequest.getName(), lineRequest.getColor()))
                );

        return new LineResponse(getLine(id));
    }

    @Transactional
    public void deleteById(Long id) {
        lineDao.deleteById(id);
    }
    ...
}

그것을 사용하는 LineController입니다.

@RestController
@RequestMapping("/lines")
public class LineController {
    ...
    @PutMapping("/{id}")
    public ResponseEntity<LineResponse> updateLine(@PathVariable Long id, @RequestBody LineRequest lineRequest) {
        lineService.update(id, lineRequest);
        return ResponseEntity.ok().build();
    }

    @DeleteMapping("/{id}")
    public ResponseEntity<Void> deleteLine(@PathVariable Long id) {
        lineService.deleteById(id);
        return ResponseEntity.noContent().build();
    }
}

특히 PUT의 경우 특정 식별자의 아이디가 존재하지 않는 경우 리소스를 생성할것을 권장하기 때문에 예외를 던지기보다 Id를 포함하여 생성할 수 있도록 작성하였습니다. DELETE의 경우도 마찬가지입니다. 아이디가 존재하지 않는 리소스를 삭제해도 외부에서는 204 상태코드로 같은 결과를 반환할 수 있도록 작성하였습니다!

한 가지 고민인 것은 이러한 멱등을 보장하기 위한 로직 들을 어떤 계층에 위치하는 것이 적절한가 입니다. 단순히 비즈니스 로직이라고 판단하여 service 계층에 위치 하였지만 HTTP 메서드를 통한 일종의 규약과 같다고 느껴져 controller에 작성해야 할지 고민이 됩니다.

앨런이 생각했을 때 이것이 멱등에 대해 적절히 이해하고 적용했다고 판단하시나요? 의견이 궁금합니다!

Choose a reason for hiding this comment

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

매트! 멱등성의 대한 이야기 잘 들었습니다. 멱등성을 확인해보니 관점이 잘못파악된 거 같습니다.
HTTP에서 말하는 멱등성은 응답의 멱등성이 아닌, '리소스'의 대한 멱등성을 뜻합니다.

식별자를 찾지못하면 404를 반환하는게 일반적인 것 같습니다.

그럼 delete에서 리소스관점에서 멱등성을 어기는 예는 '마지막 노선삭제 API' 일 것 같아요. 매번 실행해도 다른 결과를 반환하고 있어서요.

https://developer.mozilla.org/ko/docs/Glossary/Idempotent

Choose a reason for hiding this comment

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

만약 그런 비즈니스룰이 있다면, 매트말대로 컨트롤러에서 처리할 것 같습니다!
Service에서 항상 성공된 결과를 반환하는 코드는 웹을 의존하고 있는게 아닐까싶습니다

Copy link
Author

Choose a reason for hiding this comment

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

말씀 하신 것 처럼 응답에 집중하여 멱등성을 이해하였습니다. 🥲 남겨주신 링크를 통해 리소스에 대한 멱등성을 이해하였습니다!

아래는 개선한 updatedeleteById입니다.

@Transactional(readOnly = true)
@Service
public class LineService {
    ...
    @Transactional
    public LineResponse update(Long id, LineRequest lineRequest) {
        getLine(id);

        try {
            lineDao.update(id, new Line(lineRequest.getName(), lineRequest.getColor()));
        } catch (DuplicateKeyException e) {
            throw new IllegalArgumentException(lineRequest.getName() + "은 이미 존재하는 노선 이름입니다.");
        }

        return new LineResponse(getLine(id));
    }

    @Transactional
    public void deleteById(Long id) {
        getLine(id);
        lineDao.deleteById(id);
    }

    private Line getLine(Long id) {
        return lineDao.findById(id)
                .orElseThrow(() -> new NotFoundException(id + " 의 노선은 존재하지 않습니다."));
    }
}

getLine을 통해 해당 식별자의 Line이 없는 경우 커스텀 예외인 NotFoundException을 통해 예외를 던지도록 작성하였습니다.

던져진 예외는 ExceptionAdviser를 통해 상태 코드 404를 반환하도록 작성하였습니다!

@RestControllerAdvice
public class ExceptionAdviser {
    ...
    @ExceptionHandler(NotFoundException.class)
    public ResponseEntity<ErrorResponse> notFoundExceptionHandler(Exception e) {
        return ResponseEntity.notFound().build();
    }
    ...
}

Comment on lines +19 to +20
@DisplayName("노선 관련 기능")
public class LineAcceptanceTest extends AcceptanceTest {

Choose a reason for hiding this comment

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

또 행위가 많아지다보니 코드가 약간 복잡함을 느꼈는데요.
각 테스트를 메서드로 분리하거나, junit의 다이나믹 테스트를 고려해볼 수 있을 것 같아요.

https://tecoble.techcourse.co.kr/post/2020-07-31-dynamic-test/

Copy link
Author

Choose a reason for hiding this comment

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

다이나믹 테스트에 대한 키워드 확인하였습니다! 다만 아직 적용하기에 배경 지식이 부족하여 추가적인 학습 진행 후 반영하려 합니다!

Copy link
Author

@hyeonic hyeonic May 8, 2022

Choose a reason for hiding this comment

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

추가적인 학습 이후 인수테스트를 다이나믹 테스트로 작성하였습니다!

두 테스트의 장단점을 비교하기 위해 테스트를 분리하여 작성하였습니다. 다이나믹 테스트를 사용하며 느낀 점입니다. 테스트 케이스가 동적으로 생성되기 때문에 연속성 있는 테스트를 작성할 수 있었습니다. 하지만 이러한 연속성 테스트로 인해 다소 읽기 어려운 코드를 야기한 것 같다는 생각이 들었습니다. 이것은 아직 다이나믹 테스트 작성에 익숙하지 않아 더 좋은 방법을 고민해보려 합니다!

Comment on lines 12 to 21
public Line(Long id, String name, String color) {
this.id = id;
this.name = name;
this.color = color;
}

public Line(String name, String color) {
this.name = name;
this.color = color;
}

Choose a reason for hiding this comment

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

주생성자와 부성생서자를 나눠, this로 모두 주생성자를 바라보도록 작성해보면 좋을 것 같아요.

Copy link
Author

@hyeonic hyeonic May 8, 2022

Choose a reason for hiding this comment

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

언급하신 것 처럼 주생성자와 부생성자를 나눠 모두 주생성자를 바라보도록 수정하였습니다.

public class Line {

    private Long id;
    private String name;
    private String color;

    private Line() {
    }

    public Line(String name, String color) { // 주생성자
        this.name = name;
        this.color = color;
    }

    public Line(Long id, String name, String color) { // 부생성자
        this(name, color);
        this.id = id;
    }
    ...
}

한가지 의문인 것은 두 가지 생성자 중 어떤 것을 주생성자라고 판단하는 것 입니다. Line이라는 객체가 온전한 상태로 생성되기 위해서는 현재 id, name, color가 필요하다고 생각합니다. 이러한 근거를 바탕으로 Line(Long id, String name, String color)를 주생성자로 작성할 경우 Line(String name, String color)this(null, name, color)를 활용하게 되며 null 사용을 불가피하게 만들었습니다.

결국 고민 끝에 Line(String name, String color)를 주 생성자로 작성하게 되었습니다. 물론 Line(String name, String color)으로 Line을 생성하게 되면 여전히 id는 null이지만 코드 상에서 명시적인 작성을 피할 수 있게 되었습니다.

생성자는 해당 객체가 온전히 생성되는 것을 보장해야 한다고 생각합니다. 하지만 제가 작성한 코드는 null을 포함할 수 있기 때문에 온전한 객체를 보장할수도 하지 못할 수도 있다고 생각됩니다. 이러한 접근 방법이 잘못된 것인가요? 혹은 앨런이 말씀하신 주생성자와 부생성자의 판단 기준은 무엇인가요?

Choose a reason for hiding this comment

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

일반적으로 파라미터가 많은 쪽(혹은 필드와 일치하는 파라미터가 많을 때)을 주생성자로 사용하고 있긴합니다.

결국 고민 끝에 Line(String name, String color)를 주 생성자로 작성하게 되었습니다. 물론 Line(String name, String color)으로 Line을 생성하게 되면 여전히 id는 null이지만 코드 상에서 명시적인 작성을 피할 수 있게 되었습니다.

말씀해주신 것 처럼 큰 차이는 없을 것 같아요 🥲

왜 이런 문제가 생겼을까? 라고 생각해봤을때
현재 도메인코드가 id가 없으면 영속되지 않는 객체, id가 있으면 영속된 객체라는 암묵적인(?) 룰이 있는 것 같네요.
아무래도 id생성을 데이터베이스에 맡겨서, 데이터베이스의 의존적인 코드라서 생기는 문제라고 생각해요.

null을 포함할 수 있기 때문에 온전한 객체를 보장할수도 하지 못할 수도 있다고 생각됩니다

이 부분을 공감합니다! 어떻게하면 이 문제를 해결할 수 있을까요?

Copy link
Author

@hyeonic hyeonic May 9, 2022

Choose a reason for hiding this comment

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

고민해본 방법으로는 현재 컨트롤러에서 사용하고 있는 요청 전용 dto인 LineRequest를 활용하는 방법입니다. 하지만 이것은 controller에서 사용하고 있는 dto가 영속성 계층인 dao까지 관여하게 되며 dao가 controller에서 사용하는 요청을 위한 dto의 의존성을 가지기 때문에 좋지 않은 구조라고 생각합니다.

그렇다면 dao에서만 사용하는 dto를 추가적으로 생성하는 것인데, 결국 Entity와 Dto의 경계가 모호해지며 관리해야할 포인트들이 늘어난다고 생각합니다.

이 문제를 해결하기 위한 명쾌한 방법이 잘 떠오르지 않네요 ㅠㅠ 결국 명시적인 null을 허용하는 것도 트레이드 오프라고 생각해야 할까요??

Copy link

Choose a reason for hiding this comment

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

말씀해주신 것처럼 영속된 객체/영속되지 않는객체를 물리적으로 두개로 나누는것도 방법일 것 같아요.

개인적인 생각은 '애플리케이션에서 ID를 생성하고 저장하면 되지않을까?' 라고 생각해봤는데요. 애초에 문제였던 불안정객체가 생성될 일이 없다고 생각했어요. 하지만 병렬적(실제로 병렬은 아니지만)으로 돌아가는 시스템에서 유니크한 ID를 만들기가 어렵고, 성능상 이슈가 발생할 수 있네요 🥲

�매트의견대로 트레이드오프인 것 같습니다. 데이터베이스의 ID 생성기를 이용하면 객체자체를 두개를 나눌필요도 없고, 데이터베이스에서 손쉽게 유니크한 ID를 만들어주니까요

Comment on lines +14 to +15
private LineResponse() {
}

Choose a reason for hiding this comment

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

LineResponse는 기본 생성자가 필요한가요?

Copy link
Author

@hyeonic hyeonic May 8, 2022

Choose a reason for hiding this comment

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

운영 코드 내부에서 사용하는 것은 아니지만 인수 테스트 내부에서 jsonPath()를 활용할 때 사용하는 것을 확인했습니다.

@DisplayName("노선 관련 기능")
public class LineAcceptanceTest extends AcceptanceTest {
    ...
    @DisplayName("단일 노선을 조회한다.")
    @Test
    void getLine() {
        //given
        Map<String, String> param = new HashMap<>();
        param.put("name", "1호선");
        param.put("color", "bg-blue-600");

        ExtractableResponse<Response> createResponse = RestAssured.given().log().all()
                .body(param)
                .contentType(MediaType.APPLICATION_JSON_VALUE)
                .when()
                .post("/lines")
                .then().log().all()
                .extract();

        Long id = createResponse.body().jsonPath().getLong("id");
        String name = createResponse.body().jsonPath().getString("name");
        String color = createResponse.body().jsonPath().getString("color");

        ExtractableResponse<Response> response = RestAssured.given().log().all()
                .when()
                .get("/lines/" + id)
                .then().log().all()
                .extract();

        assertThat(response.statusCode()).isEqualTo(HttpStatus.OK.value());
        assertThat(id).isEqualTo(response.body().jsonPath().getLong("id"));
        assertThat(name).isEqualTo(response.body().jsonPath().getString("name"));
        assertThat(color).isEqualTo(response.body().jsonPath().getString("color"));
    }
    ...
}

누락 시킬 경우 아래와 같은 예외 메시지를 확인할 수 있기 때문에 추가하였습니다!

com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Cannot construct instance of `wooteco.subway.dto.LineResponse` (no Creators, like default constructor, exist): cannot deserialize from Object value (no delegate- or property-based Creator)
 at [Source: (String)"{"id":13,"name":"2\ud638\uc120","color":"bg-green-600","stations":[]}"; line: 1, column: 2]

다만 public으로 열어둘 경우 외부에서 의도하지 않게 사용될 우려가 있기 때문에 private으로 설정하였습니다!

Choose a reason for hiding this comment

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

테스트에서 ObjectMapper를 사용하고 있었군요 😅
private으로 변경해주신 거 좋습니다 👍

Comment on lines +17 to +51
## 일일 회고

### 2022-05-03

#### 느낀점

- `써머`: 이렇게 계획이나 룰을 정하고 시작한 것은 처음인데 굉장히 마음도 편안하고 앞으로 일정에 대한 부담감이 덜하다. 다음에 페어 만나면 꼭 나도 해야겠다.
- `매트`: 이전에 써머를 만난 기억이 있어서 반가웠다. 덕분에 아이스 브레이킹 까지의 과정이 빨랐다. 또한 미션에 대한 난이도가 상대적으로 쉬운 것 같다는 느낌을 받았다.

#### 페어에게 좋았던 점

- `써머`: 코드 컨벤션에서 일치하는 부분이 많아서 좋았다. 의견 교환에 적극적인 것 같아서 좋았다.
- `매트`: 초반에 상당 부분이 일치하는 것이 많아서 맞춰가는 시간이 적게 들었고 하고자하는 의도를 전달했을 때 빠르게 캐치하여 반영할 수 있어서 좋았다.

#### 아쉬웠던 점

- `써머`: 아쉬운게 없어서 아쉽다. 최고의 날이었다.
- `매트`: 큰 의견 차이없어 끝낼 수 있어서 좋았다. 아쉬움은 없었다.

### 2022-05-04

#### 느낀점

- `써머`: 즐거웠고 스프링 다루는게 어색한데 매트에게 많이 배웠다.
- `매트`: J로써 예상한 일정대로 착착 진행되어 매우 기분이 좋았다.

#### 페어에게 좋았던 점

- `써머`: 성격이 굉장히 좋고 소프트 스킬이 뛰어나서 같이 코드를 짜는데 마음이 편했다.
- `매트`: 의견에 대해 적절히 수용하는 태도와 정적인 분위기로 인해 차분한 환경에서 페어를 진행할 수 있어서 좋았다.

#### 아쉬웠던 점

- `써머`: 좋은 페어를 만났는데 미션이 생각보다 가볍고 쉬워서 아쉬웠다. 만약 더 복잡한 미션을 진행했다면 더 편하게 많은 의견을 나눌 수 있었을 것 같다.
- `매트`: 페어에는 큰 문제가 없었지만 아주 사소한 이슈로 나를 괴롭게 했다. 아직도 명확한 해답이나 방법을 찾지 못했기 때문에 살짝 아쉽다.

Choose a reason for hiding this comment

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

매일 회고 인상적이네요 😮

Comment on lines +18 to +19
private final NamedParameterJdbcTemplate jdbcTemplate;
private final SimpleJdbcInsert insertActor;

Choose a reason for hiding this comment

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

NamedParameterJdbcTemplate 좋네요 👍

@hyeonic
Copy link
Author

hyeonic commented May 8, 2022

안녕하세요 앨런! 매트입니다 😃

앞서 말씀해주신 것 처럼 service 계층을 새롭게 작성하였습니다. 덕분에 컨트롤러는 요청 및 응답에 집중할 수 있도록 개선되었고 service의 메서드들은 @Transactonal을 통해 일련의 과정들이 트랜잭션을 보장할 수 있도록 개선하였습니다.

org.springframework.dao에서 제공하는 예외는 말씀하신 것 처럼 데이터의 무결성을 DB 제약 조건을 통해서만 검증하며 DB에서 발생한 예외를 그대로 catch해도 되는가에 대한 질문이었습니다. 이것은 service 계층의 도입을 통해 비즈니스 로직에서 데이터의 무결성을 추가적으로 검증할 수 있도록 작성하였습니다.

제가 생각한 애플리케이션에서 중복을 확인하고 삽입하는 것의 가장 큰 장점은 DB에 의존하지 않고 비즈니스 로직에서 판단하기 때문에 내부에서 적절한 예외 메시지를 명시적으로 작성하여 확인할 수 있다는 것 입니다. 단점으로는 검증을 위해 추가적인 쿼리가 발생하기 때문에 단순히 DB의 제약 조건을 활용할 때보다 더 많은 작업을 활용한다고 생각 됩니다. 또한 검증을 위해 비즈니스 로직의 코드가 늘어나 관리해야할 포인트들이 늘어난 다고 생각합니다.

마지막으로 다이나믹 테스트에 대한 키워드를 확인하였습니다. 하지만 아직 적용하기에 배경지식이 부족하다고 판단하여 추가적인 학습을 진행한 뒤 최대한 빠르게 적용하겠습니다.

남겨주신 리뷰 아래에 제 의견과 개선 사항 적어두었습니다. 이번 리뷰도 잘 부탁드립니다. 감사합니다!

Copy link

@hongbin-dev hongbin-dev left a comment

Choose a reason for hiding this comment

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

안녕하세요 매트!
피드백 반영 잘 해주셨네요 💯💯
답변과 코멘트 남겼으니 확인해주세요!!

Comment on lines +14 to +15
private LineResponse() {
}

Choose a reason for hiding this comment

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

테스트에서 ObjectMapper를 사용하고 있었군요 😅
private으로 변경해주신 거 좋습니다 👍

@@ -32,6 +32,12 @@ public Line save(Line line) {
return findById(id);
}

@Override
public boolean existsByName(String name) {
String sql = "select count(*) from LINE where name = :name";

Choose a reason for hiding this comment

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

미션과 관련없을 수 있지만, count쿼리는 모든 row를 탐색해야해서 비효율적일 수 있습니다.
exists 쿼리를 이용해볼 수 있을 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

언급해주신 것 처럼 JPA exists 쿼리 성능 개선을 참고하여 아래와 같이 개선하였습니다!

@Repository
public class JdbcLineDao implements LineDao {
    ...
    @Override
    public boolean existsByName(String name) {
        String sql = "select exists (select 1 from LINE where name = :name)";
        return jdbcTemplate.queryForObject(sql, Map.of("name", name), Boolean.class);
    }
    ...
}

Comment on lines +25 to +27
@DisplayName("지하철역을 관리한다.")
@TestFactory
Stream<DynamicTest> dynamicTestFromStream() {

Choose a reason for hiding this comment

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

다이나믹 테스트도 적용해보셨군요👍

Comment on lines 29 to 45
dynamicTest("지하철역을 생성한다.", () -> {
String name = "강남역";

ExtractableResponse<Response> response = generateStation(name);

assertAll(
() -> assertThat(response.statusCode()).isEqualTo(HttpStatus.CREATED.value()),
() -> assertThat(response.header("Location")).isNotBlank()
);
}),
dynamicTest("기존에 존재하는 지하철역 이름으로 지하철역을 생성한다.", () -> {
String name = "강남역";

ExtractableResponse<Response> response = generateStation(name);

assertThat(response.statusCode()).isEqualTo(HttpStatus.BAD_REQUEST.value());
}),

Choose a reason for hiding this comment

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

각각의 테스트별로 개행을 해주면, 가독성을 챙길 수 있을 것 같아요

Copy link
Author

@hyeonic hyeonic May 9, 2022

Choose a reason for hiding this comment

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

개행을 추가 하였고 공통적으로 사용하는 초기 데이터들을 미리 생성하여 위에서 부터 일련의 과정 처럼 테스트가 읽힐 수 있도록 수정하였습니다! 확실히 기존 보다 명확하게 테스트가 진행되는 흐름을 확인할 수 있다고 판단합니다!

또한 앞서 멱등에서 언급 하신 것 처럼 동일한 식별자로 delete를 여러 번 진행할 경우를 검증할 때 다이나믹 테스트의 장점을 확인할 수 있었습니다.

@DisplayName("노선 관련 기능")
public class LineAcceptanceDynamicTest extends AcceptanceTest {
    ...
    @DisplayName("노선을 관리한다.")
    @TestFactory
    Stream<DynamicTest> dynamicTestStream() {
        ExtractableResponse<Response> createdResponse1 = generateLine("1호선", "bg-blue-600");
        ExtractableResponse<Response> createdResponse2 = generateLine("2호선", "bg-green-600");

        return Stream.of(
                ...
                dynamicTest("노선을 삭제한다.", () -> {
                    Long id = createdResponse1.body().jsonPath().getLong("id");

                    ExtractableResponse<Response> response = RestAssured.given().log().all()
                            .when()
                            .delete("/lines/" + id)
                            .then().log().all()
                            .extract();

                    assertThat(response.statusCode()).isEqualTo(HttpStatus.NO_CONTENT.value());
                }),

                dynamicTest("존재하지 않는 노선의 id를 삭제할 경우 잘못된 요청이므로 404를 반환한다.", () -> {
                    Long id = createdResponse1.body().jsonPath().getLong("id");

                    ExtractableResponse<Response> response = RestAssured.given().log().all()
                            .when()
                            .delete("/lines/" + id)
                            .then().log().all()
                            .extract();

                    assertThat(response.statusCode()).isEqualTo(HttpStatus.NOT_FOUND.value());
                })
        );
    }

연속적인 테스트를 통해 기존에 삭제한 id로 동일하게 삭제를 진행할 경우 404 상태 코드를 반환하는 부분입니다. 정적인 테스트와 다르게 추가적인 데이터를 설정하지 않고 자연스러운 흐름으로 검증을 진행할 수 있었습니다.

@hyeonic
Copy link
Author

hyeonic commented May 9, 2022

안녕하세요 앨런! 매트 입니다 😀

남겨주신 코멘트 아래 제 생각과 개선 사항 작성해두었습니다! 또한 멱등성에 대해 남겨주신 링크를 통해 다시 한번 학습할 수 있는 계기가 되었습니다.

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

Comment on lines +51 to +55
try {
lineDao.update(id, new Line(lineRequest.getName(), lineRequest.getColor()));
} catch (DuplicateKeyException e) {
throw new IllegalArgumentException(lineRequest.getName() + "은 이미 존재하는 노선 이름입니다.");
}

Choose a reason for hiding this comment

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

이쪽은 애플리케이션에서 검사하지 않으시는 이유가 있을까요?

Suggested change
try {
lineDao.update(id, new Line(lineRequest.getName(), lineRequest.getColor()));
} catch (DuplicateKeyException e) {
throw new IllegalArgumentException(lineRequest.getName() + "은 이미 존재하는 노선 이름입니다.");
}
validateName(lineRequest)
lineDao.update(id, new Line(lineRequest.getName(), lineRequest.getColor()));

저장과 비즈니스흐름이 다르네요 👀

Comment on lines 12 to 21
public Line(Long id, String name, String color) {
this.id = id;
this.name = name;
this.color = color;
}

public Line(String name, String color) {
this.name = name;
this.color = color;
}
Copy link

Choose a reason for hiding this comment

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

말씀해주신 것처럼 영속된 객체/영속되지 않는객체를 물리적으로 두개로 나누는것도 방법일 것 같아요.

개인적인 생각은 '애플리케이션에서 ID를 생성하고 저장하면 되지않을까?' 라고 생각해봤는데요. 애초에 문제였던 불안정객체가 생성될 일이 없다고 생각했어요. 하지만 병렬적(실제로 병렬은 아니지만)으로 돌아가는 시스템에서 유니크한 ID를 만들기가 어렵고, 성능상 이슈가 발생할 수 있네요 🥲

�매트의견대로 트레이드오프인 것 같습니다. 데이터베이스의 ID 생성기를 이용하면 객체자체를 두개를 나눌필요도 없고, 데이터베이스에서 손쉽게 유니크한 ID를 만들어주니까요

Copy link

@hongbin-dev hongbin-dev left a comment

Choose a reason for hiding this comment

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

안녕하세요 매트~
이전 피드백 잘 적용해주셨군요 👍
여기서 머지하겠습니다! 미션, 코멘트관련 궁금하신점은 편하게 DM 주세요!
다음 단계에서 만나요 👋

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.

None yet

3 participants