[디디] 지하철 정보 관리 - 인수 테스트 미션 제출합니다. #5
Conversation
- createLine - getLines - getLine - updateLine - deleteLine
- Line Entity의 stations 필드 final 제거 - update 시 body에 Response 반환 - update 시 중복 검증 안하도록 수정 - delete 시 statusCode 204(no-content) 반환하도록 수정
- 불필요한 메서드 삭제 - 예외 표현 통일 - 생성자 수정
src/main/java/wooteco/subway/admin/controller/LineController.java
Outdated
Show resolved
Hide resolved
src/main/java/wooteco/subway/admin/controller/LineController.java
Outdated
Show resolved
Hide resolved
src/main/java/wooteco/subway/admin/controller/LineController.java
Outdated
Show resolved
Hide resolved
src/main/java/wooteco/subway/admin/controller/LineStationController.java
Outdated
Show resolved
Hide resolved
src/main/java/wooteco/subway/admin/controller/LineStationController.java
Outdated
Show resolved
Hide resolved
- 생성자 Autowired 제거 - request - entity 변환 로직 service로 이동 - lineService get 메소드 네이밍 동일하게 수정 - LineStationController 생성자 주입 방식으로 수정 - StationService 구현 - URI 카멜케이스에서 하이픈 방식으로 수정 - 구간 DELETE 요청 URI 수정 - @Valid 를 통한 요청 값 검증 - ExceptionHandling 방식 수정 - ResponseEntity Parameterized Type 지정
- 프론트에서 에러를 alert하도록 수정 - Transactional 사용 - delete와 addFirst 수행시 적절하게 preStation을 수정하도록 변경 - 변경된 로직의 버그 수정 - 에러 메세지 프론트로 전달 - lineStation URI를 하이픈 표기법으로 수정 - station List 가져오는 쿼리문 한번으로 호출 - StationRepository 의 findAllById 리턴 List로 하도록 수정
src/main/java/wooteco/subway/admin/controller/LineController.java
Outdated
Show resolved
Hide resolved
| import wooteco.subway.admin.exception.BusinessException; | ||
|
|
||
| @ControllerAdvice | ||
| public class GlobalDefaultExceptionHandler { |
| @@ -1,19 +1,31 @@ | |||
| create table if not exists STATION | |||
| ( | |||
| id bigint auto_increment not null, | |||
| name varchar(255) not null, | |||
| name varchar(255) unique not null, | |||
src/test/java/wooteco/subway/admin/acceptance/LineStationAcceptanceTest.java
Outdated
Show resolved
Hide resolved
src/test/java/wooteco/subway/admin/service/LineServiceTest.java
Outdated
Show resolved
Hide resolved
src/main/java/wooteco/subway/admin/controller/LineStationController.java
Outdated
Show resolved
Hide resolved
| LineStationResponse lineStationResponse = lineService.addLineStation(requestWithId); | ||
|
|
||
| return ResponseEntity.created( | ||
| URI.create("/line-stations/" + request.getLineId() + "/" + stationId)) |
There was a problem hiding this comment.
삭제는 /line-stations/lines/{lineId}/stations/{stationId} 인데 Location 설정은 다르게 하고 있네요!
There was a problem hiding this comment.
감사합니다! 주신 피드백 반영하면서 궁금증이 생겼는데요. 제가 create를 /lines/{id} 위치와 Body에 이전역 이름과 대상역 이름으로 Post요청을 통해 수행하도록 설계했습니다. 여기서 PathVariable과 RequestParam과 Body에 담는 정보에 대해 어떤 기준으로 분류를 해야할지 헷갈리네요.😅 강의 시간에 배운 내용으로는 '계층적 구조이고, 리소스를 식별하기 위해 PathVariable을 사용한다' 라는 것이였는데, 그렇다면 PK인 id는 PathVariable이 될 수 있고, Name은 중복이 가능하기 때문에 리소스를 식별할 수 없는 요청값이므로 Query로 받는게 맞나요? 아니면 Name 또한 이 서비스에서 관리하는 리소스이므로 Path로 사용하는게 좋을까요? (처음엔 질문드린것처럼 구현했다가 또 헷갈려서 결국 id만 path로 받고, 이전역 이름과 대상역 이름을 body에 담아 보내는 방식으로 구현해놓았습니다.)
There was a problem hiding this comment.
PathVraible 은 리소스 중에 특정 값을 식별해야할 때 사용합니다. 꼭 PK가 아니더라도 특정 리소스를 식별할 수 있는 값이라면 사용할 수 있습니다. 다만, 말씀하신대로 이름이 중복된다면 식별값으론 적절하지 않습니다.
그리고 query는 조회 결과를 필터링/정렬하는 용도로 만들어졌습니다. DB의 select문이라고 생각하시면 좋을 듯 해요!
예를 들어 특정 이름의 역만 조회할 수 있는 API를 제공한다면 아래와 같이 URL을 만들 수 있습니다.
/stations?names=서울,교대&sort=id
RequestBody는 POST/PUT과 같이 리소스 생성/변경 시 관련 정보를 전달하는데 사용합니다.
지금은 각 역할에 맞게 잘 구현하셨네요! 👍
- body가 없는 경우 ResponseEntity<Void> 반환하도록 수정 - line 상수 접근제어자 수정 - line addFirst 메서드 수정 - ifPresent 로 불필요한 분기문 제거 - Station 불필요한 생성자 제거 - NotEmpty 어노테이션 NotBlank로 수정 - LineResponse 팩토리 메서드 중복 제거 - LineResponse 의 List 타입 StationResponse로 수정 - findById 중복 제거 - assertJ 테스트 메서드를 이용해서 LineServiceTest 리팩토링 - 구간 삭제 요청 URI 수정 - 구간 추가 헤더 로케이션 URI 수정 - 구간 관리 컨트롤러 삭제 -> line 컨트롤러로 이동
fucct
force-pushed
the
feature/dd
branch
2 times, most recently
from
1f8da3e to
f55d5a3
Compare
| @ExceptionHandler({BusinessException.class, DataAccessException.class}) | ||
| protected ResponseEntity<ErrorResponse> handleBusinessException(final Exception e) { | ||
| final ErrorResponse response = new ErrorResponse(e.getMessage()); | ||
| return new ResponseEntity<>(response, HttpStatus.INTERNAL_SERVER_ERROR); |
There was a problem hiding this comment.
사용자가 요청한 리소스를 찾지 못했다고 500 서버에러가 발생하는게 적절한가에 대해 고민해보시면 좋을 것 같아요!
There was a problem hiding this comment.
오 이부분에 대해서도 질문을 드렸어야 했는데 정신이 없었네요..감사합니다!!😂 추가적으로 제가 고민했던 부분중 하나가 '500에러를 언제 발생시키는가' 였습니다. 서버 개발자 입장에서 500은 예측하지 못한 에러에 대해서만 발생시키는 걸까요? 그렇다면 서버상에서 발생하는 모든 예측가능한 예외에 대해서 처리를 해준 뒤 400대의 에러로 반환해야하는 걸까요?
There was a problem hiding this comment.
말씀하신대로 500에러는 서버 개발자가 예상하지 못한 상황에서 에러가 발생하는 경우에 사용합니다.
ex) 서버 로직 상 헛점으로 발생하는 오류, 일시적인 상황(타임아웃 등)에 따른 오류
저는 서버 개발자가 예측 가능한 부분에 대해선 클라이언트가 잘못된 요청을 한 경우일 테니, 명확하게 클라이언트 오류 코드(4xx)로 응답하는 게 적절하다고 생각합니다.
| } | ||
|
|
||
| public static LineResponse of(Line line) { | ||
| return new LineResponse(line.getId(), line.getName(), line.getStartTime(), line.getEndTime(), line.getIntervalTime(), line.getCreatedAt(), line.getUpdatedAt(), new HashSet<>()); | ||
| return LineResponse.of(line, new ArrayList<>()); |
There was a problem hiding this comment.
오 제가 미처 생각하지 못한 부분이네요! 감사합니다!
주신 피드백을 보고 문득 궁금해지는 점이 생겼는데 혹시 new ArrayList<>()와 같은 컬렉션 생성은 필드에서 직접 해주는게 더 좋을까요? 아니면 위 처럼 생성자에 넣는게 좋을까요? 예전에 학습했던 내용중에 필드에 컬렉션을 생성하는 편이 좋다는 글이 있었던 것 같아서요! 혹시 정해진 방법이 있을까요?
필드에서 생성한다면 한 곳에서 값을 생성할 수 있다는 장점이 있습니다. 다만, 생성자에서 필드값을 바로 바꿔버린다면 불필요하게 생성되는 경우가 발생할 수 있습니다.
반면에 생성자에서 초기화한다면 불필요한 값이 생성되지 않고, 필요한 시점에 값을 생성할 수 있다는 장점이 있습니다. 이 경우에는 생성자마다 초기화해줘야 하는 불편함이 있습니다.
저는 두 가지 모두 좋은 방법이라고 생각하기 때문에 일관된 방식으로만 사용한다면 어떠한 방법을 써도 상관없을 것 같습니다!
| LineStationResponse lineStationResponse = lineService.addLineStation(requestWithId); | ||
|
|
||
| return ResponseEntity.created( | ||
| URI.create("/line-stations/" + request.getLineId() + "/" + stationId)) |
There was a problem hiding this comment.
PathVraible 은 리소스 중에 특정 값을 식별해야할 때 사용합니다. 꼭 PK가 아니더라도 특정 리소스를 식별할 수 있는 값이라면 사용할 수 있습니다. 다만, 말씀하신대로 이름이 중복된다면 식별값으론 적절하지 않습니다.
그리고 query는 조회 결과를 필터링/정렬하는 용도로 만들어졌습니다. DB의 select문이라고 생각하시면 좋을 듯 해요!
예를 들어 특정 이름의 역만 조회할 수 있는 API를 제공한다면 아래와 같이 URL을 만들 수 있습니다.
/stations?names=서울,교대&sort=id
RequestBody는 POST/PUT과 같이 리소스 생성/변경 시 관련 정보를 전달하는데 사용합니다.
지금은 각 역할에 맞게 잘 구현하셨네요! 👍
- 비즈니스 로직 에러와 DB접근에러 시 http 상태코드 수정
| @ExceptionHandler({BusinessException.class, DataAccessException.class}) | ||
| protected ResponseEntity<ErrorResponse> handleBusinessException(final Exception e) { | ||
| final ErrorResponse response = new ErrorResponse(e.getMessage()); | ||
| return new ResponseEntity<>(response, HttpStatus.INTERNAL_SERVER_ERROR); |
There was a problem hiding this comment.
말씀하신대로 500에러는 서버 개발자가 예상하지 못한 상황에서 에러가 발생하는 경우에 사용합니다.
ex) 서버 로직 상 헛점으로 발생하는 오류, 일시적인 상황(타임아웃 등)에 따른 오류
저는 서버 개발자가 예측 가능한 부분에 대해선 클라이언트가 잘못된 요청을 한 경우일 테니, 명확하게 클라이언트 오류 코드(4xx)로 응답하는 게 적절하다고 생각합니다.
* feat: 노선 등록/목록/삭제 기능 구현 * feat: 노선 조회 기능 구현 * feat: 노선 수정 기능 구현 * feat: 중복된 노선/역 이름에 대한 예외처리 * fix: 중복된 이름에 대한 상태 코드 변경 * test: 테스트 메소드 실행 전에 Dao를 초기화하는 기능 추가 * test: 노선 기능에 대한 E2E 테스트 * refactor: jdbc 기반으로 전환 * refactor: jdbc 기반으로 전환 * refactor: jdbc 기반으로 전환 * refactor: 코드 구조 리팩토링 * Merge pull request #1 from woong7/step2 * feat: 구간 관련 로직 일부 추가 * Merge pull request #2 from woong7/step2 * fix: 테스트 오류 수정 * Merge pull request #3 from woong7/step2 * refactor: 코드 전체 리팩토링 * Merge pull request #4 from woong7/step2 * refactor: Lombok / Transaction / Bean Validation 적용 * refactor: 서비스 레이어 경량화 * refactor: 전체 리팩토링 * refactor: Sections 도메인 추가 * refactor: Lines / Stations 도메인 추가 * Merge pull request #5 from woong7/step2 * feat: 역 정렬 로직 추가 * fix: 재귀 오류 수정 * refactor: 정렬 로직 수정 * Merge pull request #6 from woong7/step2 * refactor: 피드백 반영 * Merge pull request #7 from woong7/step2 * test: Sections 테스트코드 추가 * refactor: Stations / Lines 일급 컬렉션 제거 * fix: 테스트 오류 수정 * Merge pull request #8 from woong7/step2
안녕하세요! 디디입니다!
이번에 미션에 프론트 관련된 기능목록이 많아서 힘들었네요 😂. 리팩토링에 시간을 많이 투자하지 못해 지저분한 코드를 제출하게 되서 아쉽습니다.. 미션 진행하면서 궁금했던점 정리해 보았습니다.
첫번째로, 예외에 대해서 exception Handler를 통해 에러페이지로 보내주는 경우와, 프론트로 에러를 보여주는 경우를 어떻게 나눠야 할 지 모르겠습니다. 구분 기준이 있다면 알려주시면 감사하겠습니다.
두번째로 프론트로 주고 받는 데이터는 최대한 필요한 내용만을 전달하는게 좋을지, 확장을 고려하여 가능한 정보를 최대한 제공해야 할 지 모르겠습니다.
세번째로 컨트롤러에서 CRUD에 대한 처리를 한 뒤 프론트로 보내줘야 하는 body에 정확히 어떤 데이터가 담겨야 하는지 잘 모르겠습니다. save를 하면 save한 대상을 담고, read를 하면 read한 대상을 담고, update 하면 update 한 대상을 담고, delete하면 delete한 대상을 담아주면 될까요?
마지막으로 atdd관련 질문입니다. controller의 인수테스트에서 crud에 대한 테스트를 하더라도, 따로 repository에 대한 테스트가 필요할까요?
리뷰 감사드립니다!