[3기-C 김선호] 앨런팀 게시판 추가 미션(1) 제출합니다. #198

haero77 · 2022-12-30T13:34:59Z

@hongbin-dev @seung-hun-h @Tnfls99

안녕하세요 앨런님 승훈님 수린님!
팀 1차 미션 (쿠키를 사용한 사용자 인증) 을 구현했습니다.
리뷰 잘부탁드리겠습니다~!

📌 과제 설명

앨런팀 추가 미션(1) 을 구현하였습니다.

쿠키를 이용하여 로그인을 구현합니다.
사용자 정보 조회 시 권한을 확인합니다.

📃localhost:8080 에서 API 명세를 확인하실 수 있습니다.

👩‍💻 요구 사항과 구현 내용

회원가입기능
- 사용자에게 이메일과 패스워드를 입력받아, 회원가입을 할 수 있다.
로그인 기능
- 가입한 정보로 로그인을 할 수 있다.
내 정보 조회기능
- 가입한 사용자는 자신의 정보를 조회할 수 있다.
- 이 경우 로그인이 되어있어야하고, 본인만 정보를 조회할 수 있어야한다.

✅ 피드백 반영사항

코멘트로 남기겠습니다!

✅ PR 포인트 & 궁금한 점

코멘트로 남기겠습니다!

haero77 · 2022-12-30T13:49:40Z

src/main/java/devcourse/board/domain/post/PostService.java

-    public MultiplePostResponse findWithPaging(int startPosition, int maxResultCount) {
-        List<PostResponse> postResponses = postRepository.findWithPaging(startPosition, maxResultCount)
+    public MultipleSimplePostResponse findWithPaging(int startPosition, int maxResultCount) {
+        List<SimplePostResponse> simplePostResponses = postRepository.findWithPaging(startPosition, maxResultCount)
                .stream()
-                .map(PostResponse::new)
+                .map(SimplePostResponse::new)
                .toList();


♻️ 피드백 반영

단건 조회와 N건 조회에 같은 DTO(PostResponse)를 사용했을 때 생길 수 있는 문제 를 해결해봤습니다.

게시글 단건 조회는 상세정보를 보기 위함이고, N건 조회는 어떤 게시글들이 있나 조회하는 작업입니다.
따라서 N건 조회시, 게시글 데이터 각각의 상세정보를 모두 내려준다면 클라이언트에서 사용하지 않는 불필요한 정보까지 내려줄 가능성이 있습니다.

전: 게시글 단건 조회, N건 조회 시 같은 응답 DTO를 사용했습니다.

후: 게시글 N건 조회 시에는 게시글의 식별자와 제목만 필드로 갖는 DTO(SimplePostResponse)를 응답 객체로 사용하도록 변경하였습니다.

제가 의도한 방향입니다 👍

haero77 · 2022-12-30T14:09:54Z

src/test/java/devcourse/board/api/controller/PostApiControllerTest.java

-    private void createDummyPosts(int size) {
-        for (int i = 1; i <= size; i++) {
-            createDummyPost("member" + i, "title" + i, "content" + i);
-        }
-    }


♻️ 피드백 반영

테스트 코드의 복잡한 분기문 또는 for문의 가독성 저해

별도의 메서드 호출없이, 테스트 메서드만 보고 모두 이해하도록 작성해보기

게시글 페이징 조회 시, '다수의 게시글에 대한 정보가 필요하니까 반복문을 사용해서 미리 게시글 데이터를 생성해두어야 하는 거 아닌가?' 라고 생각했었습니다. 그러나 응답 JSON 포맷에서 리스트 형태의 데이터가 반환됨을 알 수 있으므로, 테스트 코드에서도 1개의 게시글 데이터만 미리 생성하도록 변경하였습니다.

❓ 질문

테스트에 필요한 더미 데이터를 필드로 선언하고, @BeforeEach 에서 매번 인스턴스를 생성하여 사용하는 것에 대해서는 어떻게 생각하시나요? 필드로 dummyMember 처럼 선언하고, 테스트 메서드에서 service.save(dummyMember) 바로 사용하는 것 역시 가독성을 저해할까요?

매번 같은 데이터를 필요로 한다면 @BeforeEach에서 생성하고 저장까지 해줘도 괜찮다고 생각합니다

haero77 · 2022-12-30T14:31:30Z

src/main/java/devcourse/board/domain/post/PostService.java

-    public PostResponse updatePost(Long postId, PostUpdateRequest updateRequest) {
-        Post post = findPostEntity(postId);
+    public PostResponse updatePost(Long loggedInMemberId, Long postId, PostUpdateRequest updateRequest) {
+        Post post = findById(postId);
+
+        if (!post.matchMember(loggedInMemberId)) {
+            log.warn(
+                    "An unauthorized Member attempted to update post. memberId={}, postId={}",
+                    loggedInMemberId,
+                    postId
+            );
+            throw new IllegalStateException(MessageFormat.format(
+                    "Member has no authority updated post. memberId={0}", loggedInMemberId
+            ));
+        }


♻️ 피드백 반영

게시글 작성자가 아닌 사용자의 게시글 수정을 막아보기를 적용했습니다.

전: 게시글의 작성자를 확인하는 로직이 없어 게시글 식별자와 수정할 제목, 내용만 있으면 데이터가 수정되는 문제가 있었습니다.

후: 로그인한 사용자가 게시글 작성자인지 확인하는 로직을 추가하여 해당 문제가 발생하지 않도록 변경하였습니다.

haero77 · 2022-12-30T15:11:29Z

src/main/java/devcourse/board/api/util/AuthenticationUtil.java

+    public static Long getLoggedInMemberId(HttpServletRequest request) {
+        assertAnyMemberLoggedIn(request);
+        return Long.valueOf(
+                getCookieValueByCookieName(
+                        request.getCookies(),
+                        CookieName.MEMBER_ID.getCookieName()
+                )
+        );


🧑‍💻 코드 설명

로그인한 사용자를 확인하는 메서드입니다.
LoginApiController 에서 로그인 시 쿠키 이름 memberId 로 쿠키를 등록하므로,
HttpServletRequest 에 이름 memberId 로 등록된 쿠키가 있는지 확인함으로써 로그인한 상태를 확인하도록 하였습니다.

유틸 클래스로 쿠키에 관한 로직을 분리한 점 좋은 것 같습니다 👍
그런데 스프링에서 쿠키와 관련된 유틸 메서드를 제공하고 있어서, 기존에 잘 작성된 메서드를 사용하면 좋을 것 같습니다.
WebUtils.getCookie()

seung-hun-h

선호님 리뷰 남겼습니다. 확인해주세요

seung-hun-h · 2022-12-31T11:23:50Z

src/main/java/devcourse/board/api/controller/MemberApiController.java

+        this.memberService = memberService;
+    }
+
+    @GetMapping


로그인한 사용자의 정보를 가져오려고 의도한 API인 것 같습니다만, URI만 보면 다건이 조회될 것 같습니다. URI를 변경할 필요가 있는 것 같아요

seung-hun-h · 2022-12-31T11:29:03Z

src/main/java/devcourse/board/api/util/AuthenticationUtil.java

+    private static void assertAnyMemberLoggedIn(HttpServletRequest request) {
+        if (!anyMemberLoggedIn(request)) {
+            throw new IllegalStateException("No member logged in.");
+        }
+    }
+
+    private static boolean anyMemberLoggedIn(HttpServletRequest request) {
+        return request.getCookies() != null &&
+                hasCookieName(request.getCookies(), CookieName.MEMBER_ID.getCookieName());
+    }
+
+    private static boolean hasCookieName(Cookie[] cookies, String cookieName) {
+        return Arrays.stream(cookies)
+                .anyMatch(cookie -> cookie.getName().equals(cookieName));
+    }


이 로직이 필요한지 의문이 드네요. getCookieValueByCookieName에서 해당하는 쿠키가 없는 것으로 해결 될 것 같아서요.

request.getCookies() 결과가 null 일때와 null이 아닐 때를 분기할 필요성을 느껴 위처럼 작성했었습니다만,

쿠키가 존재하는지를 확인하는 것과, 해당하는 쿠키가 없는지 확인하는 것으로 해당 유틸 클래스를 좀 더 단순화해보겠습니다!

seung-hun-h · 2022-12-31T11:31:00Z

src/main/java/devcourse/board/api/util/AuthenticationUtil.java

+    public static Long getLoggedInMemberId(HttpServletRequest request) {
+        assertAnyMemberLoggedIn(request);
+        return Long.valueOf(
+                getCookieValueByCookieName(
+                        request.getCookies(),
+                        CookieName.MEMBER_ID.getCookieName()
+                )
+        );


유틸 클래스로 쿠키에 관한 로직을 분리한 점 좋은 것 같습니다 👍
그런데 스프링에서 쿠키와 관련된 유틸 메서드를 제공하고 있어서, 기존에 잘 작성된 메서드를 사용하면 좋을 것 같습니다.
WebUtils.getCookie()

seung-hun-h · 2022-12-31T11:31:42Z

src/main/java/devcourse/board/domain/login/LoginService.java

+        Optional<Member> optionalMember = memberRepository.findByEmail(loginRequest.email());
+
+        if (optionalMember.isEmpty() || !optionalMember.get().matchPassword(loginRequest.password())) {
+            throw new IllegalArgumentException("존재하지 않는 회원이거나 비밀번호가 일치하지 않습니다.");


예외 메시지 잘 작성하셨네요👍

member.login(loginRequest.password());

처럼 객체에 메시지를 요청하는 방식은 어떻게 생각하시나요?

파라미터로 받은 password와, 회원 객체 자신의 password가 같은지 비교하는 matchPassword() 보다
요구사항에 의미상 더 가까운 login을 이용해볼 수 있겠군요!

현재 해당 로직은 리포지토리 레벨에서 email과 password가 짝을 이루는지 쿼리를 날림으로써 대체 했는데,
앞으로 비슷한 경우가 있을 때 조언해주신 방향으로 코드를 작성해보겠습니다. 인사이트 감사합니다🥳

seung-hun-h · 2022-12-31T11:33:39Z

src/main/java/devcourse/board/domain/member/MemberService.java

+        return memberRepository.findAll()
+                .stream()
+                .anyMatch(member -> member.matchEmail(email));
+    }


모든 멤버를 먼저 조회하고 이메일을 비교하면 멤버를 가져오는 비용이 클 것 같네요.
memberRepository.existByEmail 처럼 해당 이메일을 가진 멤버가 존재하는지만 확인하면 비용을 줄일 수 있을 것 같습니다

seung-hun-h · 2022-12-31T11:44:46Z

src/test/java/devcourse/board/domain/post/PostServiceTest.java

-        }
+        // when & then
+        assertThrows(IllegalStateException.class, () -> {
+            postService.updatePost(unAuthorizedMemberId, authorId, updateRequest);


authorId가 아니라 post의 Id값을 인자로 넘겨줘야 할 것 같네요.

seung-hun-h · 2022-12-31T11:47:20Z

src/test/java/devcourse/board/api/controller/PostApiControllerTest.java

-    private void createDummyPosts(int size) {
-        for (int i = 1; i <= size; i++) {
-            createDummyPost("member" + i, "title" + i, "content" + i);
-        }
-    }


매번 같은 데이터를 필요로 한다면 @BeforeEach에서 생성하고 저장까지 해줘도 괜찮다고 생각합니다

hongbin-dev

선호님 수고하셨습니다! 코드 잘 작성해주셨네요 👍👍

hongbin-dev · 2023-01-01T11:34:46Z

src/main/java/devcourse/board/api/controller/LoginApiController.java

+        Long memberId = loginService.login(loginRequest);
+        response.addCookie(new Cookie(CookieName.MEMBER_ID.getCookieName(), String.valueOf(memberId)));


쿠키를 컨트롤러레이어에서 다뤄주신 점 좋네요 👍

hongbin-dev · 2023-01-01T11:35:00Z

src/main/java/devcourse/board/api/controller/LoginApiController.java

+@RequestMapping("/login")
+public class LoginApiController {
+


로그아웃 API도 있으면 좋겠네요

hongbin-dev · 2023-01-01T11:35:57Z

src/main/java/devcourse/board/api/controller/PostApiController.java

    }

    @GetMapping
-    public ResponseEntity<MultiplePostResponse> getPosts(
+    public ResponseEntity<MultipleSimplePostResponse> getPosts(


Suggested change

public ResponseEntity<MultipleSimplePostResponse> getPosts(

public ResponseEntity<PostResponses> getPosts(

복수로 표현하면 짧고 명료하게 표현할 수 있지않을까요?

나중에 코드를 작성할 때,

PostResponses postResponses = ...
List<PostResponse> postResponses = ...

처럼 로컬변수명이 겹칠 수도 있을 것 같아 일부러 클래스명을 복수형으로 작성하지 않았습니다.

지금 생각해보니 위 두 코드가 하나의 메서드에서 작성될 일은 거의 없을 것 같네요. 불필요하게 나중 상황을 고려했던 것 같아요.🥲
감사합니다!

hongbin-dev · 2023-01-01T11:37:32Z

src/main/java/devcourse/board/api/model/CookieName.java

+public enum CookieName {
+
+    MEMBER_ID("memberId");


enum이 아니라 클래스로 관리하는게 좋을 거 같습니다.

객체의 행위에 집중해야합니다. 상수의 모음으로 enum을 생각하셨다면 절차지향적인 프로그래밍이 나올 것 같아요

🤔 생각

우선, 제가 쿠키이름을 enum으로 관리한 것이 왜 객체지향적이지 않을까라고 생각해봤습니다.

public enum CookieName처럼 선언하게 되면 각 열거상수가 하나의 CookieName 인스턴스가 됩니다.

해당 인스턴스는 필드로 String 타입의 cookieName 만 가지고 있을 뿐, 아무런 행위를 하지 않습니다. 일종의 값 객체처럼 사용됩니다.

즉, 저는 상수의 모음으로써 인스턴스의 모음인 enum을 사용했습니다.

❓ 질문

enum이 아니라 클래스로 관리하는게 좋을 거 같습니다. 라고 조언해주신 것이 'enum 은 인스턴스의 모음이므로 상수의 모음으로는 적절치 않다' 라는 뜻이셨을까요?

상수의 모음으로 enum을 생각하셨다면 절차지향적인 프로그래밍이 나올 것 같아요 라고 하신 부분은 이해가 잘 되질 않습니다.
상수의 모음으로 enum을 사용하게 됨으로써, 객체에게 메시지를 보냄으로써 협력이 이루어지는 것이 아닌 '직접 값을 접근해서 사용하게 됨으로 절차지향적이게 된다' 라는 의미셨는지 궁금합니다!

우선 enum은 주로 타입이나, 상태를 다룰 때 많이사용하는데요. 현재는 로그인행위에 대한 키 값을 enum으로 관리할 필요가 있을까? 생각이 들었습니다.

Long.valueOf( getCookieValueByCookieName( request.getCookies(), CookieName.MEMBER_ID.getCookieName() ) );

이 부분이 CookieName.MEMBER_ID.getCookieName() 절차적인 코드인데요. 절차지향적인 코드는 주로 getter, setter가 있다면 의심해볼 수 있을 것 같아요.

객체지향적으로 해볼려면 아래처럼 해볼 수 있지않았을까요?

new LoginCookie(request.getCookies()) .parseLoginId()

hongbin-dev · 2023-01-01T11:38:53Z

src/main/java/devcourse/board/domain/login/LoginService.java

+    }
+
+    public Long login(LoginRequest loginRequest) {
+        Optional<Member> optionalMember = memberRepository.findByEmail(loginRequest.email());


Repository레벨에서 password까지 필터링해서 찾는건 어떨까요?

hongbin-dev · 2023-01-01T11:39:58Z

src/main/java/devcourse/board/domain/member/MemberService.java

+            );
+        }
+
+        save(joinRequest);


요긴 메서드로 분리할 필요는 없어보이네요~

❓ 질문

save() 가 join()에서만 단독적으로 사용하는 메서드여서 별도로 분리하지 않아도 되겠다는 의미셨을까요!

save() 메서드가

memberRepository.save( Member.create( joinRequest.email(), joinRequest.password(), joinRequest.name(), joinRequest.age(), joinRequest.hobby() ) );

단순 요 작업만 하고 있는데, 굳이 메서드로 나눌 필욘 없다는 의견이었습니다~

hongbin-dev · 2023-01-01T11:40:19Z

src/main/java/devcourse/board/domain/member/MemberService.java

+        memberRepository.save(
+                Member.create(
+                        joinRequest.email(),
+                        joinRequest.password(),
+                        joinRequest.name(),
+                        joinRequest.age(),
+                        joinRequest.hobby()
+                )
+        );


Member.create의 경우는 값으로 빼는게 가독성이 더 좋을 것 같아요.

hongbin-dev · 2023-01-01T11:41:16Z

src/main/java/devcourse/board/domain/post/PostService.java

-    public MultiplePostResponse findWithPaging(int startPosition, int maxResultCount) {
-        List<PostResponse> postResponses = postRepository.findWithPaging(startPosition, maxResultCount)
+    public MultipleSimplePostResponse findWithPaging(int startPosition, int maxResultCount) {
+        List<SimplePostResponse> simplePostResponses = postRepository.findWithPaging(startPosition, maxResultCount)
                .stream()
-                .map(PostResponse::new)
+                .map(SimplePostResponse::new)
                .toList();


제가 의도한 방향입니다 👍

Tnfls99 · 2023-01-02T07:02:03Z

src/test/java/devcourse/board/api/controller/LoginApiControllerTest.java

    @Autowired
    private ObjectMapper objectMapper;

    @Test
-    @DisplayName("회원 생성")
-    void createMember() throws Exception {
+    @DisplayName("로그인")


이것이 로그인이 성공하는 것에 대한 테스트인지 실패하는 것에 대한 테스트인지 명시해주는 것이 좋을 것 같아보이는데 어떻게 생각하시나요?

API 호출 테스트 코드의 경우 막연히 실패 케이스에 대해서 문서화를 해두어야지라고 생각하고 있었습니다.
나중에라도 API 호출 실패케이스 테스트를 작성하게 될텐데, 말씀해주신 것처럼 @DisplayName으로 로그인 - 성공, 로그인 - 실패 로 표기해볼 것 같습니다.

성공 케이스와 실패 케이스로 구분 지어 명시해주는 것은 생각해보지 못했던 내용이네요. 인사이트 감사합니다🥳

Tnfls99 · 2023-01-02T07:04:32Z

src/main/java/devcourse/board/domain/member/model/Member.java

+        member.createdAt = LocalDateTime.now();
+
+        return member;
+    }


Member.create 라고 하면 멤버 객체를 생성한다는 가독성이 매우 좋네요! 또 배워갑니다 👍

Tnfls99

선호님! 수고하셨습니다~
이번에 제가 정말 배워가는 것이 많아요 👍
고민을 많이 하셨던만큼 좋은 코드가 많이 나온 것 같습니다!!
몇가지 소소한 코멘트를 달아봤으니 한번 확인 부탁드려요 ☺️

ps. 리뷰하다가 잘못 만져서 코멘트가 조금 끊겨보일 수 있습니다.. ㅎㅎ 양해부탁드려요! 🙏

Tnfls99 · 2023-01-02T07:15:10Z

src/main/java/devcourse/board/domain/post/PostService.java

+            throw new IllegalStateException(MessageFormat.format(
+                    "Member has no authority updated post. memberId={0}", loggedInMemberId
+            ));
+        }

        post.updateContents(updateRequest.title(), updateRequest.content());


updateContents는 content를 업데이트 한다는 느낌이 드는데 updatePost로 바꿔보시는 건 어떠신가요?
위에서 createPost를 사용하시는 것을 보고 통일성 있게 사용하는 것이 좋지 않을까 해서요 ☺️

메서드 네이밍에 통일성을 부여하지 못했는데, 수린님 덕에 이번 기회에 확실히 통일성 있게 변경해보았습니다.

우선 Post.createPost 의 경우 Post.create 으로,
updatePost의 경우 게시글 엔티티의 어떤 필드를 수정하는지 명확히 나타내기 위해 updateTitleAndContent으로 변경했습니다.

메서드 네이밍 뿐만 아니라 메서드 호출 방법 등 전체 관점에서 통일성 있게 코드를 작성하여 더욱 견고한 애플리케이션을 만들어야겠다고 생각드네요. 인사이트 감사합니다🥳

…게시글 수정 메서드명 변경

updatePost 호출 시 postId 파라미터에 authorId를 인자로 주었던 오류 수정

MultipleSimplePostResponse -> SimplePostResponses

haero77 · 2023-01-03T09:03:47Z

src/main/java/devcourse/board/api/authentication/AuthenticationUtil.java

        return Long.valueOf(
                getCookieValueByCookieName(
                        request.getCookies(),
-                        CookieName.MEMBER_ID.getCookieName()
+                        CookieConst.MEMBER_ID
                )
        );


♻️ 상수의 모음으로 enum 이 아니라 클래스를 사용해보기를 적용했습니다.

enum 에 선언되는 열거 상수는 사실 하나하나가 열거 타입의 인스턴스이기 때문에,
문자열 상수 등 단순한 상수의 모음으로는 enum 보다 클래스를 사용하는 쪽이 나은 것으로 이해했습니다.

그렇다면 'OrderStatus 등 상태를 나타낼 때는 왜 enum 을 사용할까?' 역시 생각해봤는데,
이 때는 주문의 상태를 상수로써 사용하는 것이 아닌 상태 하나하나를 의미 있는 객체로 사용하기 위함이 아닐까 라고 이해했습니다.

전: 단순한 상수의 모음으로 enum을 사용합니다.

후: 상수를 사용하기 위해 클래스를 인스턴스화까지 할 필요는 없다고 생각하여, 인터페이스의 필드로 선언하였습니다.

인터페이스 상수는 지양해야 합니다. 인터페이스는 타입을 정의하는데 사용해야합니다. 이펙티브자바에도 나오니 한 번 읽어 보세요

https://camel-context.tistory.com/58

인터페이스 상수가 안티패턴임을 알게 되었고, 아이템 22. 인터페이스는 타입을 정의하는 용도로만 사용하자. 을 통해
지금과 같은 단순 상수 모음으로는 인스턴스화 할 수없는 유틸리티 클래스가 적합함을 알게 되었습니다. 인사이트 감사합니다!🥳

haero77 · 2023-01-03T09:10:06Z

src/main/java/devcourse/board/api/authentication/AuthenticationUtil.java

+    public static Long getLoggedInMemberId(HttpServletRequest request) {
+        Cookie memberIdCookie = WebUtils.getCookie(request, CookieConst.MEMBER_ID);
+
+        if (memberIdCookie == null) {
+            throw new IllegalStateException("No member logged in.");
+        }
+
+        return Long.valueOf(memberIdCookie.getValue());
+    }


♻️ 기존에 잘 작성된 쿠키 관련 유틸 메서드 WebUtils.getCookie()를 활용해보기를 적용해봤습니다.

인자로 넘긴 name 에 해당하는 쿠키가 없다면 WebUtils.getCookie()이 null 을 리턴하므로, memberId로 등록된 쿠키가 없다면 로그인한 사용자가 없는 것으로 판단하였습니다.

haero77 · 2023-01-03T09:20:00Z

src/main/java/devcourse/board/api/controller/MemberApiController.java

-    @GetMapping
+    @GetMapping("/{memberId}")
    public ResponseEntity<MemberResponse> getMember(
-            HttpServletRequest request
+            HttpServletRequest request,
+            @PathVariable Long memberId
    ) {
        Long loggedInMemberId = getLoggedInMemberId(request);
+
+        if (!loggedInMemberId.equals(memberId)) {


♻️ 불명확한 사용자 정보 조회 URI 수정해보기를 적용해봤습니다.

또한, 요구사항 내 정보 조회 기능 - 로그인이 되어있어야하고, 본인만 정보를 조회할 수 있어야한다.을 다시 구현했습니다.

전:

사용자 브라우저에 쿠키가 있다면 그것으로 본인 정보를 조회할 수 있는 권한 처리가 완료된 것으로 생각했습니다.

따라서, 로그인한 회원이 다른 사용자의 정보에 접근하는 경우를 생각하지 못했습니다.

후:

위 문제를 해결하기 위해 로그인한 회원과 조회하고자 하는 회원의 식별자가 같은지 확인하는 로직을 추가하였습니다.

haero77 · 2023-01-03T09:27:07Z

src/main/java/devcourse/board/domain/login/LoginService.java

+        Optional<Member> optionalMember = memberRepository.findByEmail(loginRequest.email());
+
+        if (optionalMember.isEmpty() || !optionalMember.get().matchPassword(loginRequest.password())) {
+            throw new IllegalArgumentException("존재하지 않는 회원이거나 비밀번호가 일치하지 않습니다.");


파라미터로 받은 password와, 회원 객체 자신의 password가 같은지 비교하는 matchPassword() 보다
요구사항에 의미상 더 가까운 login을 이용해볼 수 있겠군요!

현재 해당 로직은 리포지토리 레벨에서 email과 password가 짝을 이루는지 쿼리를 날림으로써 대체 했는데,
앞으로 비슷한 경우가 있을 때 조언해주신 방향으로 코드를 작성해보겠습니다. 인사이트 감사합니다🥳

haero77

@seung-hun-h @hongbin-dev @Tnfls99

피드백을 적용하고, 적용 사항을 코멘트로 남겼습니다. 확인해주시면 감사하겠습니다!

ps. 제 실수로 중간에 코멘트가 끊겼습니다..🥲 이 점 양해해주시면 감사하겠습니다🙇‍♂️

haero77 · 2023-01-03T09:36:25Z

src/main/java/devcourse/board/domain/member/MemberRepository.java

+    public boolean existsByEmail(String email) {
+        return findByEmail(email).isPresent();
+    }


♻️ 이메일 중복 검증 로직의 비용 줄여보기를 적용했습니다.

전: DB에 중복된 이메일이 있는지 검사하기 위해, memberRepository.findAll()의 리턴 결과인 List<Member>을 스트림을 이용. 요소 중 원하는 이메일이 있는지 확인함으로써 매번 모든 회원을 조회하고 이를 스트림으로 처리하는 비용이 들어갔습니다.

후: 기존에 작성한 Optional<Member> 를 리턴하는 findByEmail() 을 이용함으로써 DB에 쿼리를 날리는 비용외에 다른 비용이 발생하지 않도록 변경하였습니다.

앗 이 부분은 EXISTS 쿼리를 써보라는 말씀이셨군요... 😅
다시 리팩토링 해보겠습니다!

haero77 · 2023-01-03T09:41:48Z

src/main/java/devcourse/board/api/controller/LoginApiController.java

+    @PostMapping("/logout")
+    public ResponseEntity<Void> logout(
+            HttpServletResponse response
+    ) {
+        Cookie cookie = new Cookie(CookieConst.MEMBER_ID, null);
+        cookie.setMaxAge(0);
+        response.addCookie(cookie);


♻️ 로그아웃 API를 추가했습니다.

❓ 질문

로그아웃의 경우 사용자가 로그인 상태인지 별도로 확인하지 않아도 괜찮을까요?

클라이언트 기준으로 로그아웃된 사용자가 다시 로그아웃 요청을 보낼 일은 없을 것 같아, 사용자가 로그인한 상태인지를 확인하는 로직은 추가하지 않았습니다.

논리적으로는 로그인 상태인지 판단하는 것이 맞는 것 같아요. 요청하는 사용자의 상태 경우가

로그인한 상태

로그인 하지 않은 상태

로그인 후 로그아웃한 상태

정도 일 것 같은데, 검사하지 않는다고 해서 큰 문제는 없을 것 같긴하네요.

haero77 · 2023-01-03T10:02:33Z

src/main/java/devcourse/board/domain/post/model/SimplePostResponses.java

+public record SimplePostResponses(
+        List<SimplePostResponse> simplePostResponses


♻️ 복수형 클래스명 사용을 적용했습니다.

❓질문

(해당 질문은 슬랙에도 업로드 해두었습니다)

복수의 게시글 응답 DTO 를 설계할 때,

public record PostResponses<T>( List<T> postResponses; ) {}

처럼 어떤 형태의 단일 게시글 응답 DTO(DetailPostResponse, SimplePostResponse, ...)든 제네릭 타입으로 받을 수 있게 설계하였습니다.

그런데 위처럼 설계 시, API 메서드에서 리턴타입으로 ResponseEntity<PostResponses<SimplePostResponse>> 처럼 코드를 작성하게되어 가독성이 떨어지는 문제가 있었습니다. 따라서 현재처럼 작성하게 되었는데요,

Q. 제네릭을 활용하면서, ResponseEntity<PostResponses<SimplePostResponse>> 같은 코드를 작성하지 않으려면 어떤 것을 더 공부하면 좋을까요? 제가 어떤 것을 놓치고 있는지 궁금합니다.

Q. 필요한 응답 DTO에 따라 아래처럼 복수 응답 DTO를 매번 생성해주는 편은 지양하는 편이 좋을까요? DTO가 지나치게 많아지는 것은 아닌지 염려됩니다.

public record SimplePostResponses( List<SimplePostResponse> simplePostResponses; ) {} public record DetailPostResponses( List<DetailPostResponse> detailPostResponses; ) {}

지금처럼 DTO를 만들어주는 것외에 다른 방법은 떠오르지 않네요.
DTO가 아무래도 많아지겠지만, 지나치게 많아질 지는 잘 모르겠어요. 많아지면서 발생하는 문제점이 어떤것이라고 생각하시나요?

haero77 · 2023-01-03T10:14:23Z

src/main/java/devcourse/board/domain/member/MemberRepository.java

+    public Optional<Member> findByEmailAndPassword(String email, String password) {
+        try {
+            return Optional.of(
+                    em.createQuery("select m from Member m " +
+                                    "where m.email=:email and m.password=:password", Member.class)


♻️ Repository레벨에서 password까지 필터링해서 찾아보기 를 적용했습니다.

로그인 시 아이디와 비밀번호를 조회하는 등의 비즈니스 로직을 서비스 레벨에서 담당하고,
리포지토리 레벨에서는 정말 단순한 CRUD 책임만 갖는 것으로 알고 있었는데,
상황에 따라 DB에서 쿼리문을 사용하여 충분히 처리할 수 있는 로직은 리포지토리 레벨에서 담당해도 괜찮겠다고 생각이 드네요 🤔

hongbin-dev

수고하셨습니다 선호님!
다음 단계 진행하시죠!

hongbin-dev · 2023-01-04T15:20:02Z

src/main/java/devcourse/board/domain/member/MemberService.java

+            );
+        }
+
+        save(joinRequest);


save() 메서드가

memberRepository.save( Member.create( joinRequest.email(), joinRequest.password(), joinRequest.name(), joinRequest.age(), joinRequest.hobby() ) );

단순 요 작업만 하고 있는데, 굳이 메서드로 나눌 필욘 없다는 의견이었습니다~

Seonho Kim added 14 commits December 29, 2022 17:22

Refactor: 회원 엔티티에 이메일, 비밀번호 필드 추가

49abea8

Feat: 회원가입 시 중복 이메일 검증 로직 추가

1acb5d8

Feat: 전역 예외처리 객체 구현

d769082

Feat: 로그인 기능 구현(쿠키)

3e9b71b

Feat: 이메일을 이용한 회원 조회 기능 추가

9f87b10

Feat: 회원 단건 조회 API 구현

d2ccfa7

Refactor: 각 레이어에 맞게 메서드명 변경

c112ef2

Feat: IllegalStateException에 대한 ExceptionHandler 추가

6e09946

Feat: 사용자 인가를 관리하는 AuthenticationUtil 객체 구현

3dc3c30

Refactor: 게시글 수정 시 게시글 작성자만 수정 가능하도록 변경

b0fb779

Chore: 미사용 파일 삭제

459e9d6

Refactor: API Controller 클래스 패키지 변경

03e33e2

Refactor: 게시글 단건 조회와 N건 조회시 서로 다른 응답 DTO를 사용하도록 변경

3a6f249

Docs: API 명세 최신화

daaf36c

haero77 changed the title ~~[3기-C 김선호] 앨런팀 게시판 추가 미션(1) 제출합니다 .~~ [3기-C 김선호] 앨런팀 게시판 추가 미션(1) 제출합니다. Dec 30, 2022

haero77 commented Dec 30, 2022

View reviewed changes

haero77 requested review from hongbin-dev, seung-hun-h and Tnfls99 December 30, 2022 15:17

haero77 self-assigned this Dec 30, 2022

haero77 added Team-Allen 🔥 매운맛 labels Dec 30, 2022

Refactor: 회원 엔티티 이메일, 비밀번호, 이름 NOT NULL 제약 조건 추가

98b32c5

seung-hun-h reviewed Dec 31, 2022

View reviewed changes

hongbin-dev reviewed Jan 1, 2023

View reviewed changes

Tnfls99 reviewed Jan 2, 2023

View reviewed changes

Seonho Kim added 3 commits January 2, 2023 19:22

Refactor: 쿠키이름을 enum이 아니라 CookieConst 인터페이스에서 관리하도록 변경

c5d1b36

Refactor: AuthenticationUtil 의 로그인한 회원 식별 로직 단순화 & 해당 로직 테스트 코드 추가

c2082c2

Chore: 예외 관련 클래스 패키지 변경

31a178f

Seonho Kim added 13 commits January 3, 2023 15:51

Refactor: 회원 본인만 정보 조회 가능하도록 권한 확인 로직 추가 & ForbiddenException 추가

cfead50

Refactor: 로그인 회원 식별 로직에서 WebUtils를 이용하도록 변경

d970957

Refactor: 중복 이메일 검증을 Repository 레벨에서 하도록 변경

b6064be

Refactor: 로그인 시 Repository 레벨에서 password까지 필터링해서 회원을 찾도록 변경

d5da773

Test: 중복되는 데이터를 테스트 클래스의 필드로 관리하도록 변경

8fa2f21

Refactor: 네이밍 일관성을 위한 게시글 생성 메서드명 변경 & 게시글의 어느 필드를 수정하는지 명확히 알 수 있도록 …

2626fcd

…게시글 수정 메서드명 변경

Test: 게시글 수정 테스트의 논리적 오류 수정

d18c4b6

updatePost 호출 시 postId 파라미터에 authorId를 인자로 주었던 오류 수정

Feat: 쿠키를 이용한 로그아웃 기능 구현

45a4c3b

Refactor: 회원 엔티티의 not null 필드로 구성된 정적 팩터리 메서드 추가

829604d

Refactor: DTO 클래스명 변경

74d1c46

MultipleSimplePostResponse -> SimplePostResponses

Refactor: 회원 생성시 DTO의 toEntity를 사용하도록 변경

6119640

Chore: 로그 메시지 오탈자 수정, 미사용 메서드 삭제

9829280

Docs: REST Docs 최신화, 로그아웃 API 추가

1f8cef9

haero77 commented Jan 3, 2023

View reviewed changes

hongbin-dev approved these changes Jan 4, 2023

View reviewed changes

Seonho Kim added 2 commits January 5, 2023 00:53

Refactor: 쿠키이름 상수를 인증 유틸 클래스에서 관리하도록 변경

dc95eda

Refactor: web-domain 형태의 패키지 구조로 변경, API URI를 '/api/v1' 으로 변경

2ed5658

haero77 merged commit 825725a into prgrms-be-devcourse:preferKim Jan 4, 2023

		Long memberId = loginService.login(loginRequest);
		response.addCookie(new Cookie(CookieName.MEMBER_ID.getCookieName(), String.valueOf(memberId)));

	public ResponseEntity<MultipleSimplePostResponse> getPosts(
	public ResponseEntity<PostResponses> getPosts(

		public record SimplePostResponses(
		List<SimplePostResponse> simplePostResponses

[3기-C 김선호] 앨런팀 게시판 추가 미션(1) 제출합니다. #198

[3기-C 김선호] 앨런팀 게시판 추가 미션(1) 제출합니다. #198

Conversation

haero77 commented Dec 30, 2022 • edited

📌 과제 설명

👩‍💻 요구 사항과 구현 내용

✅ 피드백 반영사항

✅ PR 포인트 & 궁금한 점

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

🧑‍💻 코드 설명

Choose a reason for hiding this comment

seung-hun-h left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haero77 Jan 2, 2023 • 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

Choose a reason for hiding this comment

hongbin-dev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hongbin-dev Jan 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haero77 Jan 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haero77 Jan 2, 2023 • 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

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

Tnfls99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haero77 Jan 3, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haero77 Jan 3, 2023 • edited

Choose a reason for hiding this comment

haero77 Jan 3, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haero77 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haero77 Jan 3, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

❓ 질문

Choose a reason for hiding this comment

haero77 Jan 3, 2023 • edited

Choose a reason for hiding this comment

❓질문

haero77 commented Dec 30, 2022 •

edited

haero77 Jan 2, 2023 •

edited

hongbin-dev Jan 1, 2023 •

edited

haero77 Jan 2, 2023 •

edited

haero77 Jan 2, 2023 •

edited

haero77 Jan 3, 2023 •

edited

haero77 Jan 3, 2023 •

edited

haero77 Jan 3, 2023 •

edited

haero77 left a comment •

edited

haero77 Jan 3, 2023 •

edited

haero77 Jan 3, 2023 •

edited