Step3. 지하철 구간 관리 #498
Open
Step3. 지하철 구간 관리 #498
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Dto(ErrorResponse)에 Getter 필수 Failure in @ExceptionHandler nextstep.subway.error.GlobalExceptionHandler#handleLineNotFoundException(LineNotFoundException)
- @OnetoOne 어노테이션 확인 - 기존에 DTO(LineRequest)에서 엔티티로 변경해주는 로직 Service단으로 이동(DTO에서 DB 접속 비상식적) - findStationByStationId 메소드가 StationService가 아닌 LineService에 있는 이유는 순환참조 방지 위해서
- Line 테이블 입력 시 Section Id 널 이슈 해결
- getLine(downStation.id, 1) 컴파일 에러 발생으로 get을 getString으로 변경 - 제대로 구간 추가되는지 임시로 콘솔 로그로 확인하였고 toString 메소드 추가(LineService.addSection)
- addSectionToNonPresentLine 메소드를 예로 들면, Given 조건인 추가할 구간에 해당하는 역을 생성하지 않아도 구간 추가가 가능하다는 것이 로직의 구멍 - 즉, 도메인은 Line > Section > Station의 단방향 의존을 갖도록 의도하였는데, 실질적으로 Station이 없어도 Line이 생성되는 이슈 존재
- LineAcceptance.deleteSection 메소드 then절 변경(반환값 생성) - 구간 추가/삭제 시 구간이 아닌 라인(LineResponse)을 리턴하는 게 맞는지 고민 = 구간 등록/삭제 시 SectionIsNotLastSequenceOfLine 예외 함께 사용함으로 분리 필요
- 테스트 추가
dbeod2
requested changes
Jul 29, 2022
| private Long downStationId; | ||
|
|
||
| @OneToMany | ||
| private List<Section> sections = new ArrayList<>(); |
There was a problem hiding this comment.
Section 컬렉션에 대해서 일급컬렉션으로 리팩토링해보면 어떨까요? 컬렉션에 대한 비지니스 로직이 일급컬렉션으로 모여서 응집도를 높일 수 있어요. 아래글을 참고해보세요.
| Station upStation = findStationByStationId(lineRequest.getUpStationId()); | ||
| Station downStation = findStationByStationId(lineRequest.getDownStationId()); | ||
| Section section = new Section(upStation, downStation, lineRequest.getDistance()); | ||
| sectionRepository.save(section); |
There was a problem hiding this comment.
코드상 line entity에서 section.add를 통해 구간을 저장하고 있는데요.
sectionRepository.save를 사용해주셨군요 🤔
질문 주신 답변에 영속성 전이 관련되서 링크 드렸는데요. 확인해보시면 좋을 것 같아요! 🙏
| throw new SectionAlreadyHasStationException(downStationId); | ||
| }); | ||
|
|
||
| // System.out.println("+++++++++"); |
Comment on lines
+94
to
+105
| List<Section> sections = line.getSections(); | ||
| if (sections.get(sections.size() - 1).getDownStation().getId() != sectionRequest.getUpStationId()) { | ||
| throw new SectionIsNotLastSequenceOfLine(); | ||
| } | ||
| Long downStationId = sectionRequest.getDownStationId(); | ||
| line.getSections().stream() | ||
| .filter(section -> section.getDownStation().getId() == downStationId || section.getUpStation().getId() == downStationId) | ||
| .findFirst() | ||
| .ifPresent(section -> { | ||
| throw new SectionAlreadyHasStationException(downStationId); | ||
| }); | ||
|
|
There was a problem hiding this comment.
예외처리 너무 잘해주셨네요!
한가지 의견드려보자면 예외처리도 비즈니스 로직이라면 비즈니스 로직일 것 같은데요.entity에서 담당하는건 어떨까요?
실제 비지니스 로직이 entity에 있다면 변경이 있는 경우에도 영향도가 적어질 수 있어요.
| } | ||
|
|
||
| @DeleteMapping("/lines/{id}/sections") | ||
| public ResponseEntity<LineResponse> deleteSection(@PathVariable Long id, @RequestParam Long stationId) { |
There was a problem hiding this comment.
구간 삭제 api보면return값이� 없습니다. :-)
확인 부탁드립니다.
| import org.springframework.web.bind.annotation.ExceptionHandler; | ||
| import org.springframework.web.bind.annotation.RestControllerAdvice; | ||
|
|
||
| @RestControllerAdvice |
|
|
||
| public class LengthOfLineIsOneException extends RuntimeException { | ||
| private final ErrorCode errorCode = ErrorCode.LENGTH_OF_LINE_IS_ONE_ERR; | ||
| private String errorMessage = "%s번 노선의 구간은 유일하므로 삭제할 수 없습니다."; |
|
|
||
| import org.springframework.data.jpa.repository.JpaRepository; | ||
|
|
||
| public interface SectionRepository extends JpaRepository<Section, Long> { |
|
A1. 넵 양방향 연관관계를 사용해야 한다는 말이었습니다 ~ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
안녕하세요 유라님
많이 늦었습니다.
말씀드린대로 테스트도 테스트거니와, JPA가 쉽게 이해가 가지 않아서요.
이번에 커밋 메세지도 좀 두서없는데 양해부탁드립니다..
제 생각에 제대로 구현한 게 아닌데, 어쨌든 어제 메세지 나눈 것과 고민한 부분에 대해 적어보겠습니다.
저는 다음과 같이 도메인간 단방향 연관관계를 맺고자 하였습니다.
Line > Section > Station
처음엔 Line이 Station에 대한 직접 의존을 하고 있었으나, 구간 등록/삭제 기능을 구현하며 Line은 List
또한 h2-console로 확인해보니 의도치 않은 LINE-SECTION 테이블이 생성된 것이 확인되어 이를 지워주기 위해 양방향 연관관계로 Section에 Line 필드를 추가하였는데, 아예 실행시 에러가 나더라구요...
Section - Station
OneToOne이 아닌 ManyToOne이 맞을 거 같다고 조언해주셨는데요. 테스트 코드는 어떤 방식으로 해도 성공하고, h2-console로 확인해보아도 테이블 구조는 같더라구요. 차이를 확인해보려면 데이터를 직접 넣어보면서 확인해야 될까요? 테스트에서 h2-console을 확인해보려고 했는데 방법을 잘 못 찾겠는데 혹시 어떤 식으로 테스트 해야할지 궁급합니다.
도메인(엔티티)에 로직을 작성해야 한다고 배웠는데요. 현재 구간 등록/삭제의 경우 Line 엔티티가 아닌 LineService에 로직이 존재합니다. addSection 및 deleteSection 메소드처럼 서비스 레이어에 로직이 작성되어 있는데요. Repository를 타야해서 불가피한 측면도 있으나, 만약 이런 경우가 아니라면 해당 로직들을 Entity 내부로 옮겨주는 게 맞겠죠?
듣기로 @transactional 내부에 작성해야 영속성 컨텍스트에 등록?이 된다고 이해했는데요. 그럼 도메인 로직들은 Entity가 아닌 @transactional이 붙어있는 서비스 레이어에 작성하는 게 맞는 건가 헷갈립니다.
그런데 의문은 이를테면 구간 삭제의 경우(LineService.deleteSection)에 현재는 라인을 조회하고, 해당하는 라인의 연관되어 있는 마지막 구간을 삭제하는 식으로 되어 있는데요. 이 경우에 1차 캐시 뭐 이런 걸 생각하지 않고 작성한거라 DB에 제대로 반영이 되고 있는 건지 모르겠습니다.
글이 긴데 읽어주셔서 감사합니다.
좋은 오후 보내세요!