[Team-26][BE : Shine] Issue-Tracker 1주차 2회차 PR #94

zbqmgldjfh · 2022-06-17T08:40:06Z

안녕하세요! Shine 입니다!

우선 저의 리뷰를 담당해주시게 되어 감사하다는 말씀 먼저 전하게 됩니다.

질문 2가지

1. DTO 디렉토리의 위치

DTO를 controller → service 로 넘길때도 사용하고, service → controller에 반환할때도 사용하고 있는 상황입니다.
다만 DTO 페키지를 Controller 안에두면 Service에서 Controller.Dto에 의존성이 생기니, 즉 컨트롤러쪽에 의존성이 생겨버리니 문제라 생각합니다.

우선은 business, presentation 과 같은 level에 DTO 페키지를 두었습니다만...
리뷰어 께서는 이럴 경우 어느 위치에 DTO 패키지를 두실것 같나요?

2. Test에서 요쳥 API url

url prefix로 다음과 같이 사용중 이였습니다.

server:
  servlet:
    context-path: /api

요청 같은 경우 /api/labels 에 요청하게 되는데, 문제는 test 코드 작성시 mockMvc에서 문제가 발생하였습니다.

이를 다음과 같이 수행하니 정상작동 하였습니다...

그냥 /label 로 변경시 정상 작동되는데.... 이걸 "/api/label"로 요청 되야 할텐데.... 해결방법이 없을까요?

- ForLoginUser 을 추가한 handler는 jwt토큰을 검사하고 email을 등록시킨다.

…oundExceptionHandler 구현

… 반환하도록 변경

wheejuni

수고 많으셨습니다. 👍
확실히 이 단계에선 코멘트 드릴게 많지 않군요.
사용자 인증 로직을 Filter 기반으로 바꾸는 구현을 시도해 보시면 좋겠는데, 시간 많이 남으면 하시고, 안 남으면 안 하셔도 됩니다. 그러나 마지막 프로젝트(맞죠?) 인 만큼 해보고 넘어가는 것도 의미가 많을 것 같습니다.

wheejuni · 2022-06-20T22:36:37Z

BE/src/main/java/codesquad/shine/issuetracker/auth/AuthController.java

-    public ResponseEntity<LoginResponse> login(@PathVariable String provider, @RequestParam String code) {
-        LoginResponse response = authService.login(provider, code);
-        return ResponseEntity.ok().body(response);
+    public ResponseEntity<LoginResponse> login(@PathVariable String provider, @RequestParam String code, HttpServletResponse response) {


그냥 제안 인데 Controller 대신 Filter 에 위치하면 더 좋은 구조가 되지 않을까 생각합니다.
컨트롤러에선 비즈니스 로직만 담아야 하고 로그인이 비즈니스 로직에 포함되는지는 확실하지 않은 것 같아서요.
물론 서비스의 규모가 커지고, 인증/인가만 별도의 도메인으로 빠져 나온다면 조금 얘기가 달라지기는 합니다.

넵!! 마지막 프로젝트 입니다!! ㅎㅎ!!

로그인 과정 자체를 우선 interceptor로 옮겨보도록 노력하겠습니다!!
다만 인터셉터를 유지하고 싶은점이... 그 이후에 구현해논 부분에서 호출되는 handler에 따라 로그인 판단을 구분하는 ForLoginUser 라는 에노테이션을 만들어서 사용중인데 해당 기능을 유지하고 싶어서요 ㅠ,ㅠ

@Target(ElementType.METHOD) @Retention(RetentionPolicy.RUNTIME) public @interface ForLoginUser { }

이를 interceptor에서 다음과 같이 사용

private boolean loginRequired(Object handler) { return (handler instanceof HandlerMethod) && ((HandlerMethod) handler).hasMethodAnnotation(ForLoginUser.class); }

시도중인데... 로그인 구현이 필터나 인터셉터로 가버리면... OAuth의 callback url은 어디로 지정을 해줘야 할까요?
컨트롤러가 없는 상황에서 생각하려니 어려운것 같습니다ㅠ,ㅠ

필터를 특정 URL Pattern에 매핑시킬 수 있는 방법이 있습니다.

특정 조건에만 필터가 실행되도록 판별하는 로직을 넣을 수 있고 이를 스프링이 래핑해 두었는데요, 조금만 찾아보세요!

wheejuni · 2022-06-20T22:37:39Z

BE/src/main/java/codesquad/shine/issuetracker/auth/AuthController.java

-        return ResponseEntity.ok().body(response);
+    public ResponseEntity<LoginResponse> login(@PathVariable String provider, @RequestParam String code, HttpServletResponse response) {
+        UserInfo userInfo = authService.login(provider, code);
+        response.addHeader("Authorization", "Bearer " + userInfo.getAccessToken());


클라이언트 측에서 서버로 인증 정보를 담기 위해 사용하는 헤더인 거 같은데 좀 어색하네요.

Controller에서 response에서 직접 헤더를 추가하기 보다는, Interceptor를 활용하여 추가하도록 수정하겠습니다!!
아? 아니면 Header에 담는것이 어색하다는 건 그냥 ResponseDto body에 담아서 보내라는 의미이실까요?

네 맞습니다. 바디에요 ㅎㅎ

wheejuni · 2022-06-20T22:38:52Z

BE/src/main/java/codesquad/shine/issuetracker/common/HealthController.java

+    @GetMapping("/health")
+    public String healthCheck() {
+        return "OK!";
+    }


actuator 학습해 보세요~

actuator를 통해 다음과 같이 확인해볼수 있게 되었습니다!

굳 좋습니다.

wheejuni · 2022-06-20T22:41:15Z

BE/src/main/java/codesquad/shine/issuetracker/milestone/business/MilestoneService.java

+@Service
+public class MilestoneService {
+
+    private MilestoneRepository milestoneRepository;


요즘은 그냥 이렇게도 자동주입이 되는가보군요;; 신기하네요;;

앗... 수정완료 하였습니다 ㅎㅎ

wheejuni · 2022-06-20T22:42:12Z

BE/src/main/java/codesquad/shine/issuetracker/milestone/business/dto/response/MilestoneDto.java

+    private Long openedIssues;
+    private Long closedIssues;
+
+    public MilestoneDto(Milestone milestone) {


영속성 컨텍스트가 열려있는 상태에서 호출해야 할 로직들이 좀 보여서, 사용에 유의해야 할 것 같긴 합니다.

음 MilestoneService 라는 서비스의 findAll()이라는 메서드 안에서 즉, transaction 범위 안에서 사용하니까 상관없는것 아닐까요??

MilestoneDto는 딱 Service안에서만 사용합니다!! Controller까지 넘어가지는 않아서요!
아직 미숙한 부분이 많아 이러한 부분을 스스로 느끼지 못하는 것 같습니다 ㅠ,ㅠ

코드로 그런 사용을 막을 수 없다면 그런 사용에도 열려있는 것이라 보는게 맞다고 생각은 합니다만.. 사실 저도 현업에서 늘 고민하는 부분이에요. 어떤 상황인지는 충분히 이해합니다 ㅎㅎ

zbqmgldjfh · 2022-06-21T02:48:22Z

혹시??? 로그인 과정이 interceptor 대신 filter로 구현 해보라 하신 이유가 학습의 목적인가요??
아니면 기술적 이점이 있기 때문인가요???

zbqmgldjfh · 2022-06-21T04:09:35Z

@wheejuni 아참 질문은 따로 다 DM 드리면 되나요?? 여기에 쓰시는건 안보시는것 같아서요...

wheejuni · 2022-06-21T04:11:02Z

@wheejuni 아참 질문은 따로 다 DM 드리면 되나요?? 여기에 쓰시는건 안보시는것 같아서요...

아 죄송합니다. 답변은 추가로 달아드리겠습니다.

wheejuni · 2022-06-21T15:25:22Z

답변 드립니다. @zbqmgldjfh

질문 속에 답이 있는 것 같은데요, controller → service 로 넘길때도 사용하고, service → controller에 반환할때도 사용해야만 하나요? 이 구조부터 한번 끊고 가는게 좋지 않을까 합니다.

service -> controller 시에는 DTO를 사용하지 않는다면요? 어떻게 되나요? 아주 쉽게 풀릴 것 같은 문제이기도 하네요.
일단 저라면 지금 상황 그 자체를 놓고보면 presentation 쪽에 DTO를 놓겠습니다. 그것이 DTO의 역할의 본질에 더 가까운 위치인 것 같아서요.

서블릿 매핑이 들어가는 설정을 주셨기때문에 테스트코드에선 극복이 불가할 것 같습니다(다른 workaround가 있을 수도 있는데, 제가 그냥 보기엔 그렇습니다). 꼭 해당 설정을 사용해야 할까요? 해당 설정의 역할을 정확히 알고 사용하셨나요? 모르겠습니다. 저걸 써야하는 경우는... 잘 못 본것 같네요.

zbqmgldjfh · 2022-06-21T22:26:28Z

우선 답변 감사드립니다!!

service -> controller시에 DTO를 안쓰신다는 의미가? Entity를 직접 반환한다는 말씀 이신가요?
음 반환하지 않고 사용하는 방법에 대하여 좀더 고민해보겠습니다 ㅎㅎ
/api prefix 설정은 그냥 초기에 front 팀과 협의 했던 부분이라 사용하고 있기는 한데... 사실 딱히 필요는 없는것 같습니다...

wheejuni · 2022-06-22T03:42:02Z

@zbqmgldjfh 맞습니다. 엔티티에서도 특정 정보만 한정해서 넘길 수 있는 방법도 있을 것 같은데 "composition" 이라는 키워드로 한번 검색해 보시면 어떨까 싶네요 ㅎㅎㅎ

zbqmgldjfh · 2022-06-22T04:04:35Z

이번에 많은 키워드를 알려주셔서 감사합니다 Brian!! 상속보단 구성이란 단어를 많이 들었었는데 고민해보도록 하겠습니다!!

…_crud Backend/feature/#90 milestone crud

zbqmgldjfh added 4 commits June 17, 2022 17:28

feat(ForLoginUser): ForLoginUser 에노테이션 구현

6690bc1

- ForLoginUser 을 추가한 handler는 jwt토큰을 검사하고 email을 등록시킨다.

chore(p6spy): Insert문의 인자를 확인하기 위해 p6spy 의존성 추가

a4156e6

feat(ApiDocumentUtils): RestDocs 의 깔끔한 출력을 위한 pretty 설정

34c5894

test(LabelControllerTest): 통합 테스트 작성

82d6a39

zbqmgldjfh added the review-BE Improvements or additions to documentation label Jun 17, 2022

zbqmgldjfh requested a review from wheejuni June 17, 2022 08:40

zbqmgldjfh self-assigned this Jun 17, 2022

zbqmgldjfh added 11 commits June 17, 2022 19:11

feat(GlobalExceptionHandler.tokenNotFoundExceptionHandler): TokenNotF…

18089d7

…oundExceptionHandler 구현

feat(LoginResponse): LoginResponse로는 유저의 정보만 반환하고, Token은 Header에 담아서…

5fe1d27

… 반환하도록 변경

feat(LabelControllerTest): Delete test 작성 완료

c42e5b0

feat(LabelControllerTest): Edit test 작성 완료

a89fd23

feat(JpaAuditingConfiguration): Auditing을 위한 Configuration 별도 설정

44c8cac

test(LabelControllerTest): LabelControllerTest를 slice 테스트로 변경

2c0b42c

feat(MilestoneController): Milestone API 구현

d38c942

test(MilestoneServiceTest): Milestone Service Test 구현

b9a0af0

feat(Issue.isOpen): Open 상태인지 확인하는 메서드 구현완료

a212643

feat(application-local): Default_batch_fetch_size 100으로 설정

2c32b5a

feat(HealthController): 서버 상태 확인용 api

e0e1a77

wheejuni requested changes Jun 20, 2022

View reviewed changes

fix(MilestoneService): 의존성 추가 수정

982e5ac

zbqmgldjfh merged commit fcfdc08 into codesquad-members-2022:team-26 Jun 21, 2022

nathan29849 added a commit that referenced this pull request Jun 28, 2022

Merge pull request #94 from nathan29849/backend/feature/#90-milestone…

b5219c0

…_crud Backend/feature/#90 milestone crud

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Team-26][BE : Shine] Issue-Tracker 1주차 2회차 PR #94

[Team-26][BE : Shine] Issue-Tracker 1주차 2회차 PR #94

zbqmgldjfh commented Jun 17, 2022

wheejuni left a comment

wheejuni Jun 20, 2022

zbqmgldjfh Jun 21, 2022 •

edited

zbqmgldjfh Jun 21, 2022 •

edited

wheejuni Jun 21, 2022

wheejuni Jun 20, 2022

zbqmgldjfh Jun 21, 2022 •

edited

wheejuni Jun 21, 2022

wheejuni Jun 20, 2022

zbqmgldjfh Jun 21, 2022

wheejuni Jun 21, 2022

wheejuni Jun 20, 2022

zbqmgldjfh Jun 21, 2022 •

edited

wheejuni Jun 20, 2022

zbqmgldjfh Jun 21, 2022 •

edited

wheejuni Jun 21, 2022

zbqmgldjfh commented Jun 21, 2022

zbqmgldjfh commented Jun 21, 2022 •

edited

wheejuni commented Jun 21, 2022

wheejuni commented Jun 21, 2022

zbqmgldjfh commented Jun 21, 2022 •

edited

wheejuni commented Jun 22, 2022

zbqmgldjfh commented Jun 22, 2022 •

edited

[Team-26][BE : Shine] Issue-Tracker 1주차 2회차 PR #94

[Team-26][BE : Shine] Issue-Tracker 1주차 2회차 PR #94

Conversation

zbqmgldjfh commented Jun 17, 2022

안녕하세요! Shine 입니다!

질문 2가지

1. DTO 디렉토리의 위치

2. Test에서 요쳥 API url

wheejuni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zbqmgldjfh Jun 21, 2022 • edited

Choose a reason for hiding this comment

zbqmgldjfh Jun 21, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zbqmgldjfh Jun 21, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zbqmgldjfh Jun 21, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zbqmgldjfh Jun 21, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zbqmgldjfh commented Jun 21, 2022

zbqmgldjfh commented Jun 21, 2022 • edited

wheejuni commented Jun 21, 2022

wheejuni commented Jun 21, 2022

zbqmgldjfh commented Jun 21, 2022 • edited

wheejuni commented Jun 22, 2022

zbqmgldjfh commented Jun 22, 2022 • edited

zbqmgldjfh Jun 21, 2022 •

edited

zbqmgldjfh Jun 21, 2022 •

edited

zbqmgldjfh Jun 21, 2022 •

edited

zbqmgldjfh Jun 21, 2022 •

edited

zbqmgldjfh Jun 21, 2022 •

edited

zbqmgldjfh commented Jun 21, 2022 •

edited

zbqmgldjfh commented Jun 21, 2022 •

edited

zbqmgldjfh commented Jun 22, 2022 •

edited