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

[스프링 - 협업 미션] 소롱(최솔지) 미션 제출합니다. #23

Merged
merged 70 commits into from
Jun 8, 2021

Conversation

soulgchoi
Copy link

안녕하세요 재연링~ 또 뵙네요!🙋‍♀️
프론트엔드 크루들, 백엔드 팀원들과 함께한 재밌는 미션이었어요!
이번에 새로 배우면서 적용한 것들이 많아 보람찬 미션이기도 했습니다.

REST Docs를 사용해 API 문서화 진행했습니다. subwayboy.n-e.kr 여기에서 확인하실 수 있어요! 성공-실패 모두 작성한 케이스가 아직 몇 개 없는데요, 협업을 위한 작업이니까 좀 더 상세한 편이 좋을까요? 모든 케이스에 대해 성공-실패를 작성하는 편이 좋은지 궁금합니다.

요청값 검증할 때 annotation, validator를 커스텀해서 사용해보았습니다. 중복되는 어노테이션이 계속 많아지더라구요.
코드를 줄이는 방법의 하나로 custom exception도 상속 구조로 작성했습니다.

요금 계산할 때 데코레이터 패턴을 사용했는데요, 아직 정확한 이해가 덜 된 것 같아 꼭 피드백 받고 싶습니다!

인터셉터도 이번에 처음 사용했어요. 기능이 추가되면 여러 개의 인터셉터를 만들어 볼 수 있을 것 같아요.
같은 URI를 사용하는 API에 대해 어떻게 메서드를 구분하면 좋을지 궁금했는데 그냥 인터셉터를 만들면 되더라구요....

프론트엔드 요청사항 중에 역과 거리 정보를 함께 달라는 내용이 있었는데요, 이에 따라 StationWithDistanceResponse에서 @JsonInclude를 사용했어요. 종점일 경우 distance가 없기 때문에 0이 되고, NON_DEFAULT라 출력되지 않습니다. 원하는대로 동작하기는 하지만 맥락상 좋은 방법은 아닌 것 같아요. 그럴 경우는 없겠지만, 정말 distance가 0일 때는 값이 있어야 하는거니까요. 종점을 나타내기 위한 다른 방법은 없을까요?😭

아무쪼록 잘 부탁드립니다. 감사합니다!

- AuthenticationPrincipalConfig 를 test에서 사용하지 않게 변경
- 문서화 설정 추가
- 테스트 데이터를 위한 TestDataLoader 생성
- LineController에서 delete 메서드를 noContent 반환하도록 변경
- 노선 수정을 위해 LineUpdateRequest 추가
- 테이블이 비어있을 때만 데이터 삽입하도록 변경
- schema.sql 에서 drop 쿼리 제거
- SectionResponse 생성
- Section을 정렬하는 로직 추가
- LineResponse가 SectionResponse를 갖도록 수정
- 관련된 테스트 수정
- API 문서 수정
- 중복된 이메일이 있을 때 false 반환
- findByEmail 실패했을 때 예외 처리
- API 문서 수정
- 테스트 작성 및 수정
- API 문서 개선
Copy link

@jaeyeonling jaeyeonling left a comment

Choose a reason for hiding this comment

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

안녕하세요 소롱! 다시 뵈어 반갑습니다 😄
요구사항에 맞게 잘 구현해주셨어요 💯
코멘트와 답변 남겨두었으니 확인 부탁드릴게요~

@@ -2,6 +2,23 @@ plugins {
id 'org.springframework.boot' version '2.4.3'
id 'io.spring.dependency-management' version '1.0.11.RELEASE'
id 'java'
id 'org.asciidoctor.convert' version '1.5.9.2'

Choose a reason for hiding this comment

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

asciidoctor 👍

return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).build();
}

@ExceptionHandler(NotFoundException.class)

Choose a reason for hiding this comment

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

지금은 Exception이 추가될 때 마다 해당 코드들을 수정해야 하는 구조인데 의도된 내용이 맞을까요?
의도된 내용이 아니라면

public abstract class CustomException extends RuntimeException {
    HttpStatus status();
    Object body();
}

@ExceptionHandler(ServiceException.class)
public ResponseEntity<Object> customException(final CustomException e) {
        return ResponseEntity.status(e.status())
                .body(e.body());
}

와 같은 추상화를 통해 중복되는 작업을 없애볼 수 있을 것 같습니다 :)

public boolean preHandle(HttpServletRequest request, HttpServletResponse response,
Object handler) {

if (request.getMethod().equals(HttpMethod.GET.name())) {

Choose a reason for hiding this comment

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

Http Method에 따라 인증 여부를 나누는 것이 맞는지 고민해보면 좋을 것 같은데요
Get Method일 때 무시하는 것은 지금 프로젝트 상황에선 맞는 상황이지만, 추후 Get Method에 인증이 필요한 기능이 추가 됐을 때 문제가 될 수 있는 부분 같은데 소롱은 어떻게 생각하시나요?

Copy link
Author

Choose a reason for hiding this comment

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

저도 이 부분에서 충분히 문제가 생길 수 있다고 느꼈습니다! 그렇게 되면 여러 개의 Interceptor를 만들고 메서드 별로 인증 여부를 나누게 될 것 같은데 이런 방향이 맞나요?

Choose a reason for hiding this comment

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

Q. 저도 이 부분에서 충분히 문제가 생길 수 있다고 느꼈습니다! 그렇게 되면 여러 개의 Interceptor를 만들고 메서드 별로 인증 여부를 나누게 될 것 같은데 이런 방향이 맞나요?

A. 구현에 대한 부분만 보고 해당 코멘트를 남겼는데, Interceptor로 인증을 하는 지금 프로젝트에서 해당 코멘트를 적용하기엔 까다로운 부분들이 있네요 ㅎㅎ

스프링 시큐리티를 예로 들면 메서드 단위로 인증에 대한 처리를 하는데요!
메서드 단위로 처리하기 위해선 AOP를 활용해야하는데, 아직 AOP에 대해 학습하지 않은 것으로 알고있으니 지금 상황에서 최소한의 비용으로 적용시킬 수 있는 방법을 생각해본다면..!

  1. 인증이 필요한 Path와 Method 목록들을 만든다.
  2. AuthenticationPrincipalConfig에서 1에 작성한 Path 기준으로 Interceptor를 매칭해준다.
  3. Interceptor에서 1에 작성한 Path와 Method에 해당하는지 확인한다.

이정도가 가장 적은 비용으로 해결할 수 있는 방법이지 않을까..! 생각이 드는데 번거로운 부분이 있을 수 있기 때문에 해당 내용은 넘어가주셔도 괜찮습니다 😄

Choose a reason for hiding this comment

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

Q. 어떤 기준으로 목록을 만들고 매칭하면 좋을까요?
/members 요청같은 경우 POST 요청일 때 true여야 하고, 나중에 전체 멤버를 조회하는 API가 생긴다면 GET 요청에 대해 인증이 필요할 것 같아요. 그리고 지금은 쿼리 스트링으로 이메일이 들어올 때 중복체크 용으로 GET 메서드로 요청이 오고 있구요.
이렇게 한 끗 차이로 달라지는 것들에 대응하기가 힘들었습니다.
앞으로 비슷한 상황이 있다면 어떤 식으로 처리하게 될까요? 스프링 시큐리티를 사용하면 이런 부분이 해소되는지도 궁금합니당😅

A.

class SecurePath {
  private final String path;
  private final HttpMethod httpMethod;
}

class SecurityZone {
  private final Set<SecurePath> targets;
}

securityZone.isTarget(new SecurePath(request.getRequestURI(), request.getMethod()));

대충 위와 같은 개념으로 생각해보시면 좋을 것 같습니다 ㅎㅎ

스프링 시큐리티를 사용할 경우

@Secured({ "ROLE_MEMBER" })
void something() {
}

위와 같이 메서드에 애노테이션을 걸어 해결할 수 있어요!

@Service
@Transactional

Choose a reason for hiding this comment

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

저는 개인적으로 클래스 레벨에 Transactional을 사용하는 것을 선호하지 않습니다
트랜잭션은 굉장히 비용이 큰 작업이고, 불필요한 트랜잭션으로 성능이 낮아지는 문제도 있지만 더 큰 문제는 의도되지 않은 트랜잭션으로 인해 데드락이 발생할 수 있다 라는 것이 생각입니다
실제 현업에서 대규모 트래픽을 처리하기 위해 데드락이 발생하지 않도록 설계하는 것이 굉장히 중요한데요
개발하는 과정에서도 이 기능에 데드락이 발생할 요소가 있을까? 트랜잭션을 꼭 잡아야 가능한 기능일까? 구조를 변경하여 락이 필요 없는 구조로 바꿀 수 없을까? 에 대해 끊임없이 고민하게 되는데, 이러한 고민을 하는 시점이 저는 메서드에 Transactional 처리를 하는 시점에 가장 많이 하는 것 같습니다

또한 클래스마다 트랜잭션과 격리레벨이 정해지는 것이 아닌 기능(메서드)단위로 처리되기 때문에 클래스에 적용하여 공통으로 처리할 필요가 있을까? 라는 의문이 들었습니다 ㅎㅎ

해당 내용에 대해 소롱은 어떻게 생각하시나요?

Copy link
Author

Choose a reason for hiding this comment

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

사실 트랜잭션을 잘 모르고 사용했어요. 이번 기회에 정확히 공부하고 리팩토링 해보겠습니다!!ㅎㅎ

@Repository
public class LineDao {

private static RowMapper<Line> ROW_MAPPER() {

Choose a reason for hiding this comment

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

Suggested change
private static RowMapper<Line> ROW_MAPPER() {
private static RowMapper<Line> rowMpper() {

메서드는 카멜케이스로 작성하는 것이 관례입니다 :)

import org.springframework.stereotype.Component;

@Component
public class NameValidator implements ConstraintValidator<SubwayName, String> {

Choose a reason for hiding this comment

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

CustomValidator 👍


@Override
public boolean isValid(String name, ConstraintValidatorContext context) {
return name.matches("^[가-힣0-9]+$") && StringUtils.isNotBlank(name);

Choose a reason for hiding this comment

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

Pattern 객체를 반복해서 생성하는 것은 굉장히 비용이 큰 작업입니다.
재사용하면 어떠한 장점이 있을지 고민해보아요.

충분한 고민 후 이펙티브 자바 아이템 6. 불필요한 객체 생성을 피하라 를 참고해주세요!

Copy link
Author

Choose a reason for hiding this comment

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

입력이 들어올 때마다 Pattern을 생성하지 않아서 성능이 개선되겠네요!
성능 부분 말고도 상수로 정의하니까 명확하게 보이고, 변경이 있을 때 편하겠다는 생각을 했습니다.

@AutoConfigureRestDocs
class LineControllerTest {

@MockBean

Choose a reason for hiding this comment

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

Mock의 트레이드 오프와 관련하여 좋은 글이 있어 공유드립니다~

woowacourse/jwp-refactoring#2 (comment)

해당 내용을 읽어보았을 때 해당 Mock이 필요한지에 대해 고민해보고 의견을 나눠보면 좋을 것 같아요 :)

Copy link
Author

Choose a reason for hiding this comment

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

저도 이 부분의 mock 테스트가 로직을 검증 하는데는 의미가 없다는 생각을 많이 했습니다. 문서화를 위해 수행한 테스트인데, andExpect 부분이 훨씬 적어도 목표한 바는 달성할 수 있을 것 같습니다. 서비스 레이어에 비즈니스 로직이 많지 않기 때문에, sections 도메인 같은 부분의 테스트를 늘리는 편이 좋은 테스트가 될 것 같다는 생각이 드네요!

import java.util.function.Function;
import java.util.function.Predicate;

public class AgeFareCalculator implements FareCalculator {

Choose a reason for hiding this comment

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

Q. 요금 계산할 때 데코레이터 패턴을 사용했는데요, 아직 정확한 이해가 덜 된 것 같아 꼭 피드백 받고 싶습니다!

A. 데코레이터 패턴은 동적으로 책임을 추가할 수 있는 디자인패턴 중 하나인데요!
지금과 같이 거리와 나이에 따른 요금체계가 있는 상황에서 데코레이터 패턴을 사용하였을 때 얻을 수 있는 이점이 큰지에 대해 고민해보면 좋을 것 같습니다!

데코레이터 패턴의 대표적인 예시인 Input(Output)Stream을 보면 굉장히 많은 I/O에 대응하기 위해 여러 데코레이터를 래핑하여 활용할 수 있는 것을 볼 수 있습니다
즉, 기본 기능에 여러가지 상황에 맞게 추가 기능을 데코레이팅 하는 상황이라면 유용하게 쓰일 수 있지만, 지금과 같이 모든 사용자가 나이/거리에 따른 규칙을 가지는 상황에서는 장점보다 단점이 클 수 있기 때문에 주의하는 것이 좋을 것 같다! 라는 것이 제 생각입니다 ㅎㅎ

이해가 덜 됐다! 라고 생각하신 이유를 추측해보자면, 일반적인 데코레이터 패턴에선 순서와 선택에 따라 조합할 수 있도록 Abstract Decorator와 Concrete Decorator가 존재하는데 이 부분에 대해 공부해보면 좋을 것 같아요!!

물론 설계엔 정답이 없고, 설계하는 사람의 의도가 담기는 것이 더욱 중요하기 때문에 소롱이 어떠한 기준으로 설계하였는지 생각을 공유해주시면 의미있는 대화를 할 수 있을 것 같습니다 :)

Copy link
Author

Choose a reason for hiding this comment

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

요금 계산 규칙이 늘어날 수 있다고 상정하고 데코레이터 패턴을 사용하려고 했던건데, 막상 사용할 때는 별 조건이 없다보니 장점이 와닿지 않았던 것 같아요. 요금 계산 규칙이 늘어난다면 더 금방 이해했을텐데, 말씀해 주신 것처럼 모든 사용자가 동일한 규칙을 가지는 상황에서 조합할 수 있는 데코레이터가 적으니 오히려 복잡도가 상승했다는 느낌이 드네요. 일단 책을 보면서 공부하고 있는데, 데코레이터 조합 예시들을 보니까 쬐~~끔은 감을 잡을 수 있을것 같기도 합니다🤔


private long id;
private String name;
@JsonInclude(Include.NON_DEFAULT)

Choose a reason for hiding this comment

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

Q. 프론트엔드 요청사항 중에 역과 거리 정보를 함께 달라는 내용이 있었는데요, 이에 따라 StationWithDistanceResponse에서 @JsonInclude를 사용했어요. 종점일 경우 distance가 없기 때문에 0이 되고, NON_DEFAULT라 출력되지 않습니다. 원하는대로 동작하기는 하지만 맥락상 좋은 방법은 아닌 것 같아요. 그럴 경우는 없겠지만, 정말 distance가 0일 때는 값이 있어야 하는거니까요. 종점을 나타내기 위한 다른 방법은 없을까요?😭

A. 상황에 따라 다르지만 지금의 경우 int -> Integer로 변경 후 Null일 경우 포함하지 않는 방식으로 해결이 가능할 것 같습니다 :)

Copy link
Author

Choose a reason for hiding this comment

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

기능상 달라지지는 않지만 종점일 때 거리를 null로 가지고 Include.NON_NULL로 제외하는 방식이 더 맥락에 맞는것 같아 변경했습니다!

- null 일때 표시하지 않도록 변경
- 상황에 맞는 예외 만들어서 처리
- HttpStatus를 exception에서 처리
- ExceptionHandler 간략화
@soulgchoi
Copy link
Author

soulgchoi commented Jun 4, 2021

안녕하세요, 재연링!
생각할 거리를 많이 던져주셔서 큰 도움이 되었습니다. 아직 완벽하게 공부해서 이해하고 있지는 않은 것 같아 피드백 반영할 수 있는 부분만 우선 수행했습니다.
코멘트에 남긴 것과 별개로, method에 따라 인증 여부를 나누는 것 같은 경우에는 API 별로 Interceptor를 분리하면 되겠다는 생각을 했는데 당장 기능에 영향이 있지는 않다고 생각되어 일단 그대로 두었습니다!
또, Exception 처리할 때 Http Status도 가지도록 변경을 했는데요. 지금은 웹 프로젝트이므로 중복 코드도 줄고 예외 처리가 증가했을 때도 별도로 수정할 필요 없다는 점에서 좋았습니다. 다만 이런 방법이 웹에 의존적이라 레벨 1때 처럼 콘솔, 웹 환경을 모두 고려해야 하는 상황이라면 맞지 않다는 생각을 하게 되었습니다. 이 부분에 대해서도 의견 주시면 감사하겠습니다!
미션을 수행하면서 @transactional이 어렵더라구요. 블로그 글을 보는 데는 한계가 있고, 스프링 공식 문서가 더 어렵게 느껴져서.. 혹시 관련해서 좋은 글이나 도서를 추천해주실 수 있을까요?😂

이번에도 잘 부탁드립니다. 감사합니다👍

Copy link

@jaeyeonling jaeyeonling left a comment

Choose a reason for hiding this comment

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

코멘트를 잘 반영해주셨어요 👍
질문의 답변 남겨두었으니 확인 부탁드릴게요 :)

public boolean preHandle(HttpServletRequest request, HttpServletResponse response,
Object handler) {

if (request.getMethod().equals(HttpMethod.GET.name())) {

Choose a reason for hiding this comment

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

Q. 저도 이 부분에서 충분히 문제가 생길 수 있다고 느꼈습니다! 그렇게 되면 여러 개의 Interceptor를 만들고 메서드 별로 인증 여부를 나누게 될 것 같은데 이런 방향이 맞나요?

A. 구현에 대한 부분만 보고 해당 코멘트를 남겼는데, Interceptor로 인증을 하는 지금 프로젝트에서 해당 코멘트를 적용하기엔 까다로운 부분들이 있네요 ㅎㅎ

스프링 시큐리티를 예로 들면 메서드 단위로 인증에 대한 처리를 하는데요!
메서드 단위로 처리하기 위해선 AOP를 활용해야하는데, 아직 AOP에 대해 학습하지 않은 것으로 알고있으니 지금 상황에서 최소한의 비용으로 적용시킬 수 있는 방법을 생각해본다면..!

  1. 인증이 필요한 Path와 Method 목록들을 만든다.
  2. AuthenticationPrincipalConfig에서 1에 작성한 Path 기준으로 Interceptor를 매칭해준다.
  3. Interceptor에서 1에 작성한 Path와 Method에 해당하는지 확인한다.

이정도가 가장 적은 비용으로 해결할 수 있는 방법이지 않을까..! 생각이 드는데 번거로운 부분이 있을 수 있기 때문에 해당 내용은 넘어가주셔도 괜찮습니다 😄

@@ -54,7 +54,7 @@ public void insertSections(Line line) {
simpleJdbcInsert.executeBatch(batchValues.toArray(new Map[sections.size()]));
}

public int countSectionByStationId(Long id) {
public Integer countSectionByStationId(Long id) {

Choose a reason for hiding this comment

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

Integer로 응답해주신 이유가 있을까요!?

Copy link
Author

@soulgchoi soulgchoi Jun 6, 2021

Choose a reason for hiding this comment

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

언박싱이 NPE를 발생시킬 수 있다고 경고문이 나와서 수정했던 것인데, 생각해보니 count는 null이 나오지 않겠네요..🤔 수정했습니다!

Choose a reason for hiding this comment

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

return jdbcTemplate.queryForObject(query, int.class, id, id);

IDE의 경고를 제거하는 코드를 만들기 위해 위와 같이 작성해볼 수도 있겠네요 ㅎㅎ

@@ -21,6 +20,7 @@ public MemberService(MemberDao memberDao) {
this.memberDao = memberDao;
}

@Transactional

Choose a reason for hiding this comment

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

해당 위치의 @transactional은 어떠한 의미로 남겨주셨는지 알 수 있을까요!?

Copy link
Author

Choose a reason for hiding this comment

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

제가 트랜잭셔널에 대해 짧게 이해한 바는 아래와 같습니다.

  • @transactional 어노테이션은 오토커밋을 멈추고 에러가 나면 롤백, 성공하면 커밋한다.
  • 클래스 단위에 붙일 경우 데드락 발생 가능, 전파 문제(둘 다 공부중입니다..)로 메서드 단위에 붙이는 게 좋다.
  • 조회 쿼리는 커밋을 안하니까 CUD 메서드는 @transactilnal로 사용하는 편이 좋다.

이런 결론으로 사용하게 되었습니다!

Choose a reason for hiding this comment

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

@transactional 어노테이션은 오토커밋을 멈추고 에러가 나면 롤백, 성공하면 커밋한다.
-> @transactional이 있는 경우 해당 메서드 내부에서 하나의 커넥션을 사용하고(전파옵션에 따라 조금씩 다르긴 합니다 ㅎㅎ), @transactional이 없는 경우 매 쿼리마다 커넥션을 생성하여 요청한다고 생각하면 이해가 편할 것 같습니다!

클래스 단위에 붙일 경우 데드락 발생 가능, 전파 문제(둘 다 공부중입니다..)로 메서드 단위에 붙이는 게 좋다.
-> 이미 아시겠지만 부연 설명을 드리자면, 클래스에 붙이는 경우와 메서드의 붙이는 것의 동작은 똑같습니다 :)

트랜잭션은 여러 기능을 묶어주는 역할을 하는데, 해당 위치에 작성하였을 때 어떠한 이점이 있을까요!?

Comment on lines 7 to 8
HttpStatus httpStatus;
Object body;

Choose a reason for hiding this comment

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

접근제어자 작성을 놓치셨네요 @_@

Copy link
Author

Choose a reason for hiding this comment

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

수정완료입니다!😭


import org.springframework.http.HttpStatus;

public class SubwayException extends RuntimeException {

Choose a reason for hiding this comment

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

Suggested change
public class SubwayException extends RuntimeException {
public abstract class SubwayException extends RuntimeException {

SubwayException은 상속으로만 사용하고 있는데, 계층을 위해서만 사용한다면 abstract를 통해 의도를 전달할 수 있습니다 :)
추가적으로 저는 모든 클래스를 abstract 또는 final로만 작성하는데 그 이유에 대해 고민해보면 좋을 것 같아요 ㅎㅎ

Copy link
Author

Choose a reason for hiding this comment

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

오! 이 주제에 대해 다른 크루들과 얘기했던 적이 있었는데요, 단순히 final이 불변 객체의 이점을 가져가는 것 외에도 협업 관점에서 장점이 있다고 결론지었어요. abstract는 상속 가능한 클래스에만, 그 외에는 의도하지 않은 상황이 발생할 수 있으니 다른 모든 클래스는 final을 붙이면 좋겠다는 의견이었어요. 그렇다면 final/아무것도 사용하지 않은 클래스로도 의도를 전달할 수 있지 않냐는 말도 있었는데, 타인이 봤을 때 조금이라도 더 명확한 편이 좋지 않겠냐는 게 제 생각이었습니당!

Copy link
Author

Choose a reason for hiding this comment

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

그렇다면 제 코드는 exception 말고 모두 final 클래스가 되겠군요..🤔

import wooteco.subway.exception.SubwayException;

@ControllerAdvice
public class ExceptionAdvice {

Choose a reason for hiding this comment

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

Q. Exception 처리할 때 Http Status도 가지도록 변경을 했는데요. 지금은 웹 프로젝트이므로 중복 코드도 줄고 예외 처리가 증가했을 때도 별도로 수정할 필요 없다는 점에서 좋았습니다. 다만 이런 방법이 웹에 의존적이라 레벨 1때 처럼 콘솔, 웹 환경을 모두 고려해야 하는 상황이라면 맞지 않다는 생각을 하게 되었습니다. 이 부분에 대해서도 의견 주시면 감사하겠습니다!
A. 말씀하신 것 처럼 지금의 구조에서 예외처리는 웹의 환경에 의존하는 것으로 트레이드 오프의 영역이라고 보면 될 것 같습니다!
저희가 @component, @service 등 애노테이션을 통해 스프링 프레임워크에 의존하는 대신 여러 이점을 얻을 수 있는 것으로 생각해보면 좋을 것 같아요 ㅎㅎ

웹 이외 환경에서 의존성이 생기는게 거부감이 든다면...! Handler를 분리해줘야겠네요 T.T

Copy link
Author

Choose a reason for hiding this comment

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

저도 지금 상황에서는 이점이 많아서 편리하게 사용하게 되었습니다. 의문이 좀 생겼던거라서.. handler를 별도로 수정하지 않으니 너무 편했어요😂

@@ -26,12 +30,24 @@ public LineService(LineDao lineDao, SectionDao sectionDao, StationService statio
this.stationService = stationService;
}

@Transactional

Choose a reason for hiding this comment

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

Q. 미션을 수행하면서 @transactional이 어렵더라구요. 블로그 글을 보는 데는 한계가 있고, 스프링 공식 문서가 더 어렵게 느껴져서.. 혹시 관련해서 좋은 글이나 도서를 추천해주실 수 있을까요?
A. 사실 저도 아직까지 DB와 관련된 부분은 굉장히 어렵습니다 ㅎㅎ..
@transactional 자체가 DB의 트랜잭션과 관련된 내용이기 때문에 DB 자체를 공부해보시면 좋을 것 같아요!

데드락과 관련된 내용은 직접 EntityManager로 여러 트랜잭션을 만들어 예상 시나리오를 만들어보시면 더욱 도움이 될 것 같습니다 ㅎㅎ

@soulgchoi
Copy link
Author

안녕하세요, 재연링!
피드백 주신 부분들 반영해서 우선 리뷰요청 드립니다.

Interceptor 관련한 부분을 하려고 해봤는데요, path와 method별로 나누어 처리하려니 분기가 굉장히 많더라구요.
uri pattern을 더 세세하게 추가하고 나누려고 했더니 if문이 지나치게 많아져서 되돌렸습니다.

인증이 필요한 Path와 Method 목록들을 만든다.
AuthenticationPrincipalConfig에서 1에 작성한 Path 기준으로 Interceptor를 매칭해준다.
Interceptor에서 1에 작성한 Path와 Method에 해당하는지 확인한다.

어떤 기준으로 목록을 만들고 매칭하면 좋을까요?
/members 요청같은 경우 POST 요청일 때 true여야 하고, 나중에 전체 멤버를 조회하는 API가 생긴다면 GET 요청에 대해 인증이 필요할 것 같아요. 그리고 지금은 쿼리 스트링으로 이메일이 들어올 때 중복체크 용으로 GET 메서드로 요청이 오고 있구요.
이렇게 한 끗 차이로 달라지는 것들에 대응하기가 힘들었습니다.
앞으로 비슷한 상황이 있다면 어떤 식으로 처리하게 될까요? 스프링 시큐리티를 사용하면 이런 부분이 해소되는지도 궁금합니당😅

Copy link

@jaeyeonling jaeyeonling left a comment

Choose a reason for hiding this comment

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

코멘트를 잘 반영해주셨어요 👍
질문의 답변만 확인해주시면 머지할게요 :)

@@ -54,7 +54,7 @@ public void insertSections(Line line) {
simpleJdbcInsert.executeBatch(batchValues.toArray(new Map[sections.size()]));
}

public int countSectionByStationId(Long id) {
public Integer countSectionByStationId(Long id) {

Choose a reason for hiding this comment

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

return jdbcTemplate.queryForObject(query, int.class, id, id);

IDE의 경고를 제거하는 코드를 만들기 위해 위와 같이 작성해볼 수도 있겠네요 ㅎㅎ

@@ -21,6 +20,7 @@ public MemberService(MemberDao memberDao) {
this.memberDao = memberDao;
}

@Transactional

Choose a reason for hiding this comment

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

@transactional 어노테이션은 오토커밋을 멈추고 에러가 나면 롤백, 성공하면 커밋한다.
-> @transactional이 있는 경우 해당 메서드 내부에서 하나의 커넥션을 사용하고(전파옵션에 따라 조금씩 다르긴 합니다 ㅎㅎ), @transactional이 없는 경우 매 쿼리마다 커넥션을 생성하여 요청한다고 생각하면 이해가 편할 것 같습니다!

클래스 단위에 붙일 경우 데드락 발생 가능, 전파 문제(둘 다 공부중입니다..)로 메서드 단위에 붙이는 게 좋다.
-> 이미 아시겠지만 부연 설명을 드리자면, 클래스에 붙이는 경우와 메서드의 붙이는 것의 동작은 똑같습니다 :)

트랜잭션은 여러 기능을 묶어주는 역할을 하는데, 해당 위치에 작성하였을 때 어떠한 이점이 있을까요!?

public boolean preHandle(HttpServletRequest request, HttpServletResponse response,
Object handler) {

if (request.getMethod().equals(HttpMethod.GET.name())) {

Choose a reason for hiding this comment

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

Q. 어떤 기준으로 목록을 만들고 매칭하면 좋을까요?
/members 요청같은 경우 POST 요청일 때 true여야 하고, 나중에 전체 멤버를 조회하는 API가 생긴다면 GET 요청에 대해 인증이 필요할 것 같아요. 그리고 지금은 쿼리 스트링으로 이메일이 들어올 때 중복체크 용으로 GET 메서드로 요청이 오고 있구요.
이렇게 한 끗 차이로 달라지는 것들에 대응하기가 힘들었습니다.
앞으로 비슷한 상황이 있다면 어떤 식으로 처리하게 될까요? 스프링 시큐리티를 사용하면 이런 부분이 해소되는지도 궁금합니당😅

A.

class SecurePath {
  private final String path;
  private final HttpMethod httpMethod;
}

class SecurityZone {
  private final Set<SecurePath> targets;
}

securityZone.isTarget(new SecurePath(request.getRequestURI(), request.getMethod()));

대충 위와 같은 개념으로 생각해보시면 좋을 것 같습니다 ㅎㅎ

스프링 시큐리티를 사용할 경우

@Secured({ "ROLE_MEMBER" })
void something() {
}

위와 같이 메서드에 애노테이션을 걸어 해결할 수 있어요!

Copy link

@jaeyeonling jaeyeonling left a comment

Choose a reason for hiding this comment

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

수 많은 코멘트 반영하시느라 고생 많으셨습니다 💯
해당 요청은 머지하도록 할게요!
즐거운 방학 보내시고 남은 과정도 화이팅입니다 😄

@jaeyeonling jaeyeonling merged commit 34ca316 into woowacourse:soulgchoi Jun 8, 2021
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

2 participants