[Review] 2022/06/21 2차 PR (Issue Tracker)

이번 리뷰는 Brain 께서 해주셨다! 리뷰해주셔서 감사합니다 !!

[Team-26][BE : Shine] Issue-Tracker 1주차 2회차 PR by zbqmgldjfh · Pull Request #94 · codesquad-members-2022/issue-tracker

 

1. 코드리뷰

1-1) JWT token 응답은 body로!

원래 나는 사용자가 로그인 하면 응답으로 JWT token을 Header에 담아서 보내고 있었다.

리뷰어의 말처럼 client가 서버에 요청할때 마다 Header에 담아서 보내줘야하는 token이다.

 

다만 나는 이전까지들의 경험에 의하면, 이를 헤더에 담아서 보내주면 브라우져가 저장해 두었다가, 이후 서버측에 전달해주던 방식을 생각하였다.

 

하지만 API 방식에서는 이렇게 동작하는것이 어색하다 하셨다.

따라서 Response Body 에 token을 담아서 보내기로 변경하였다.

@GetMapping("/{provider}/callback")
public ResponseEntity<LoginResponse> login(@PathVariable String provider, @RequestParam String code, HttpServletResponse response) {
    UserInfo userInfo = authService.login(provider, code);
    return ResponseEntity.ok().body(LoginResponse.of(userInfo));
}

UserInfo안에 아예 token까지 저장해 두었다.

 

1-2) Spring Actuator

이 리뷰는 사실 지난번 Dion에게도 받았었던 리뷰이다.

다만 지난번에는 협업 방식으로 진행하다보니 더욱 시간이 없었어서... 적용하지 못했는데,

이번 프로젝트는 혼자서 전부 구현중이라 빠르게 적용해 보았다.

 

결과는 다음과 같이 확인해볼 수 있었다.

여러 엔드포인트를 지원하는데, 필요시 직접 엔드포인트를 만들어 확인해볼수도 있다고 한다.

다만 이 기능들은 보안상 위험한 부분이 많다고 한다. 잘 사용하려면 Spring Security와의 콜라보가 필요한것 같다.

 

1-3) 영속성 컨텍스트의 범위

위와 관련된 내용으로 OSIV에 대하여 공부해본적이 있다.

 

우선 해당 DTO는 다음과 같다.

@Getter
@NoArgsConstructor
public class MilestoneDto {
    private Long id;
    private String title;
    private String description;
    private LocalDateTime createdDateTime;
    private LocalDate dueDate;
    private Long openedIssues;
    private Long closedIssues;

    public MilestoneDto(Milestone milestone) {
        this.id = milestone.getId();
        this.title = milestone.getTitle();
        this.description = milestone.getDescription();
        this.createdDateTime = milestone.getCreatedDateTime();
        this.dueDate = milestone.getDueDate();
        this.openedIssues = milestone.countOpenedIssues();
        this.closedIssues = milestone.countClosedIssues();
    }
}

MileStone entity를 인자로 전달받은 후, 해당 entity의 값들을 getter로 가져오게 된다.

 

여기서 opendIssues 와 closedIssue가 문제인것 같다. 해당 코드들은 다음과 같다.

@Getter
@Entity
@EqualsAndHashCode(of = "id")
public class Milestone extends BaseTimeEntity {

    @Id
    @GeneratedValue(strategy = GenerationType.IDENTITY)
    @Column(name = "milestone_id")
    private Long id;

    private String title;
    private String description;
    private LocalDate dueDate;
    private boolean isOpen;

    @Builder.Default
    @OneToMany(mappedBy = "milestone")
    private List<Issue> issues = new ArrayList<>();

    // 생략...

    public Long countOpenedIssues() {
        return issues.stream()
                .filter(Issue::isOpen)
                .count();
    }

    public Long countClosedIssues() {
        return issues.stream()
                .filter(Issue::isClosed)
                .count();
    }
}

 milestone에 List로 저장되어 있는 issues에 접근하여 count를 하게된다.

여기서 issues를 Lazy Loading이 걸려 있다. 따라서 View까지 넘어가서 초기화가 되면 안된다. Service 안에서 끝나야 한다.

 

물론 SpringBoot가 default로 OSIV 기능을 지원해 주니까 Controller 에서도 지연로딩을 초기화 할 수 있지만,

사실 Service안에서 초기화 를 끝내는 것이 정확하다고 생각한다.

 

다행이도 나의 DTO는 Service안에서만 딱 초기화를 끝내기 때문에 사용에 문제가 없을것 같다?

 

2. 팀원의 코드리뷰

지난 리뷰어인 Dion이 진짜 리뷰를 잘 해주시는것을 알기에 동료인 쿠킴의 PR을 좀 살펴보게 되었다!

 

2-1) Graceful Shutdown

graceful shutdown 이라는 키워드는 난생 처음 들어본 키워드였다!

 

server.shutdown=graceful

위와 같이 옵션으로 설정하여, tomcat이 종료 시그널 받을 때 이미 처리중인 request 모두 처리하고 application을 종료하는 기능이다!

 

또한 다음과 같이 타임아웃 기간 설정을 추가로 할수 있다고 한다 (역시 쿠킴을 조사를 잘하시는것 같다)

spring.lifecycle.timeout-per-shutdown-phase=00s

https://docs.spring.io/spring-boot/docs/current/reference/htmlsingle/#web.graceful-shutdown

Spring Boot Reference Documentation

 

2-2) Log의 저장방법

해당 리뷰는 나에게도 해당되는 리뷰인것 같다...

로그 파일이 계속 저장되다 너무 커져도 문제이다... 이를 어떻게 해결할 것 인가?

방법 1)

위 방법을 추가하여 매 배포 때 마다 , 배포 단위의 log를 저장

/home/ec2-user/Issues-tracker-deploy-{version}.log

단점은 배포 단위가 커진다면 말씀해주셨던 로그가 한 파일에 크게 쌓이게 될 것 이다.

그래도 개별 버전별로 로그를 관리할 수 있는 점은 장점인 것 같다.

 

방법 2)
logback 을 활용해 특정 경로에 특정 시간마다 로그를 저장!

logback으로 지정해주는게 좋을거같습니다.!

 

2-3) @AllArgsConstructor 의 잠재적인 문제점

AllArgsConstructor 는 자동생성 되는 장점이 있지만 자동 생성이 문제인 것 같습니다.

1. 멤버변수의 순서를 바꿀 때 문제가 될 수 있습니다.
   같은 타입의 멤버 변수 순서를 바꾼다면 생성자가 다시 만들어집니다. 이때 다른 값이 들어갈 수 있는 문제가 있습니다.
2. 멤버변수 추가, 삭제
   멤버변수의 추가, 삭제 시 기존 운영코드가 깨져 수정하는 것은 같지만, 생성자가 의도적으로 명시되어있는 코드와 자동생성되는 문제가 있어 보입니다.

이러한 문제에 더 나아가, 1번 같은 경우 같은 Type을 중복으로 사용하지 않기를 권장한다 하셨다.

 

2-4) 안전한 Builder 패턴

왜 Builder를 썼냐 물어보신다면.... 사실 필드가 많아지는 경우 생성자로 다 생성해주기는 불편하기때문에...

편하게 쓰려고 사용했다... 롬복이 이만큼 강력하다...

 

다만 이에 따른 문제점도많은데 이에 대하여 고민을 좀 해보자

일단 특정 인자를 까먹고 전달하지 않아도 객체가 생성이 되어버린다. null값에 방어가 안된다고나 할까?

이점이 가장 큰 단점인것 같다.

 

Dion 덕분에 추가적으로 2가지를 더 알게 되었다.

컴파일 오류가 발생하지 않는다는점, 필드가 많아지는 단점을 알아차리기 힘들다는점.

 

필드가 많아지는 것을 원래 생성자를 사용할때는 금방금방 인식이 되는데, 위와같이 롬복 + 빌더를 사용하면 인식하기 힘든 부분인것 같다.

너무 간편해서 필드 몇게더 늘어도 부담이 없어진달까? -> 이점 자체가 문제인 것 같다.

 

결론적으로 다음 글이 많이 도움이 된다!

https://cheese10yun.github.io/spring-builder-pattern/

Builder 기반으로 객체를 안전하게 생성하는 방법 - Yun Blog | 기술 블로그