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

[4기 고범준 & 박은지] Springboot-board-JPA 1차 PR입니다. #238

Open
wants to merge 47 commits into
base: 범준,은지-mission
Choose a base branch
from

Conversation

1o18z
Copy link

@1o18z 1o18z commented Aug 2, 2023

📌 과제 설명

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

1. 엔티티를 구성한다.

  • 회원(User)
  • id (PK) (auto increment)
  • name
  • age
  • hobby
  • created_at
  • created_by
  • 게시글(Post)
  • id (PK) (auto increment)
  • title
  • content
  • created_at
  • created_by
  • 회원과 게시글에 대한 연관관계를 설정한다.
  • 회원과 게시글은 1:N 관계이다.
  • 게시글 Repository를 구현한다. (PostRepository)

2. API를 구현한다.

  • 게시글 조회
  • 페이징 조회 (GET "/posts")
  • 단건 조회 (GET "/posts/{id}")
  • 게시글 작성 (POST "/posts")
  • 게시글 수정 (POST "/posts/{id}")

3. REST-DOCS를 이용해서 문서화한다.

  • REST Docs 적용

4. 추가 구현 사항

  • 회원
  • 회원 가입
  • 회원 로그인
  • 회원 로그아웃

  • UserController 테스트 코드는 작성하는 중입니다 🙂
  • 서비스와 레포지토리 테스트는 추후에 추가하겠습니다.

@1o18z 1o18z changed the base branch from main to 범준,은지-mission August 4, 2023 01:09
Copy link

@seung-hun-h seung-hun-h left a comment

Choose a reason for hiding this comment

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

리뷰 남겼습니다~
컨트롤러 테스트 외 도메인 로직에 대한 테스트도 필요해 보이네요

build.gradle Outdated
// test
testImplementation 'org.springframework.boot:spring-boot-starter-test'
testImplementation 'org.springframework.restdocs:spring-restdocs-mockmvc'
testImplementation 'com.epages:restdocs-api-spec-mockmvc:' + restdocsApiSpecVersion

Choose a reason for hiding this comment

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

Suggested change
testImplementation 'com.epages:restdocs-api-spec-mockmvc:' + restdocsApiSpecVersion
testImplementation "com.epages:restdocs-api-spec-mockmvc:${restdocsApiSpecVersion}"

이렇게도 작성할 수 있을 것 같아요

import static org.springframework.http.HttpStatus.*;

@RestControllerAdvice
public class ControllerAdvice {

Choose a reason for hiding this comment

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

클래스 이름을 좀 더 명확하게 작성해주어야 할 것 같아요

Copy link
Author

@1o18z 1o18z Aug 6, 2023

Choose a reason for hiding this comment

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

수정하였습니다 ! :)
dabc8c4

return postService.create(request, userId);
}

@GetMapping()

Choose a reason for hiding this comment

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

Suggested change
@GetMapping()
@GetMapping

Copy link
Author

Choose a reason for hiding this comment

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

수정하였습니다 ! :)
5ddedbc

Comment on lines 45 to 52
@PatchMapping("/{id}")
@ResponseStatus(NO_CONTENT)
public void update(@PathVariable("id") Long postId,
@RequestBody PostUpdateRequest request,
@SessionAttribute(name = "userId", required = false) Long userId) {
validateUserId(userId);
postService.update(postId, request, userId);
}

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.

물론, 다른 상황에서는 응답 본문에 수정된 리소스를 포함시킬 수도 있으나, 이 경우에는 클라이언트가 필요로 하지 않는다고 판단하여 반환값을 전달하지 않기로 했습니다 !
그래서 반환값을 주지 않고 nocontent 응답코드로 요청은 성공적으로 처리 했으나 반환값은 없다는걸 전달해줬습니다 !😃

postService.delete(postId, userId);
}

private void validateUserId(Long userId) {

Choose a reason for hiding this comment

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

인터셉터나 AOP를 활용해서 중복 코드를 줄 일 수 있겠는데요?

Copy link
Author

Choose a reason for hiding this comment

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

수정하였습니다 ! :)
0f360e1

@@ -0,0 +1,4 @@
package dev.jpaboard.post.dto;

public record PostUpdateRequest(String title, String content) {

Choose a reason for hiding this comment

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

validation은 필요 없나요?

Copy link
Author

Choose a reason for hiding this comment

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

추가하였습니다 ! :)

Comment on lines 28 to 31
public Long login(UserLoginRequest request) {
User user = findByEmailAndPassword(request.email(), request.password());
return user.getId();
}

Choose a reason for hiding this comment

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

UserService의 책임이 꽤 넓은 것 같아요. 인증, 인가 관련 로직은 클래스를 분리하는 것도 좋다고 생각합니다.
UserNotFoundException이 발생하면 ControllerAdvice에서 NotFound 응답을 주는데 Unauthorized 응답을 주어야 하는게 아닐까요?

Copy link
Author

Choose a reason for hiding this comment

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

수정하였습니다 ! :)
6fe5820

private Long id;

@Column(length = 25, nullable = false)
@Email(regexp = EMAIL_REGEX)

Choose a reason for hiding this comment

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

기본으로 제공하는 Email 정규표현식과 직접 작성하신 정규표현식에는 어떤 차이가 있나요?

Copy link
Author

Choose a reason for hiding this comment

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

직접 작성하면 추가적인 유효성 검사를 할 수 있고 더 엄격하게 검증할 수 있습니다 !
@Email은 모든 문자열을 통과 시켜주고 있어서 직접 작성해 주었습니다 !

private String hobby;

@Builder
private User(@Valid String email, @Valid String password, String name, int age, String hobby) {

Choose a reason for hiding this comment

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

이렇게하면 validation이 동작하나요?

Copy link
Author

Choose a reason for hiding this comment

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

제대로 동작하지 않는 것을 확인하여, 수정하였습니다 ! :)
d885391

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.

제거하였습니다 ! :)
8eb1637

Comment on lines 60 to 64
Post post = findPostById(postId);
User user = findUserById(userId);

post.checkAuthorize(user);
postRepository.deleteById(postId);
Copy link
Member

Choose a reason for hiding this comment

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

아니면 userId만 검증을 할 수도 있습니다! 연관 객체의 @id 붙은 필드는 프록시 객체가 가지고 있어서 별도로 조회 없이도 검증이 가능해요

src/main/java/dev/jpaboard/post/dto/PostsResponse.java Outdated Show resolved Hide resolved
src/main/java/dev/jpaboard/user/api/UserController.java Outdated Show resolved Hide resolved
src/main/java/dev/jpaboard/user/api/UserController.java Outdated Show resolved Hide resolved
Copy link

@seung-hun-h seung-hun-h left a comment

Choose a reason for hiding this comment

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

리뷰 남겼습니다.
애플리케이션 실행이 안되네요. 더이상 사용하지 않는 코드는 제거해주세요

@ExceptionHandler(NotFoundException.class)
@ResponseStatus(NOT_FOUND)
public ErrorResponse handleNotFoundException(NotFoundException e) {
return new ErrorResponse(e.getMessage());

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.

'게시글을 찾을 수 없습니다'와 '유저를 찾을 수 없습니다'라는 메시지도 그대로 넘겨주면 안되는 걸까요 ??

Choose a reason for hiding this comment

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

예외 메시지는 일반적으로 디버깅을 위해 사용해요. 그래서 어떤 값이 들어왔고 어떠한 이유로 실패했다는 내용이 들어가게 될 거에요.
시스템 내부 구현에 해당하는 정보를 그대로 클라이언트에 전달해도 좋을까요?

Comment on lines 25 to 33
@GetMapping("/me")
public UserInfoResponse findUser(@SessionAttribute(name = "userId") Long userId) {
return userService.findUser(userId);
}

@GetMapping("/me")
public UserResponse findUser(@SessionAttribute(name = "userId") Long userId) {
return userService.findUser(userId);
}

Choose a reason for hiding this comment

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

url이 중복되는 것 같네요

Copy link
Author

Choose a reason for hiding this comment

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

인텔리제이로 원격으로 수정한 부분이 올라가지 않은 것을 이제야 확인했습니다 ! 🥲
반영되지 않은 커밋들 한 번에 올렸습니다 !
0be4727


private static final Pattern EMAIL_PATTERN = Pattern.compile("^[0-9a-zA-Z]([-_.]?[0-9a-zA-Z])*@[0-9a-zA-Z]([-_.]?[0-9a-zA-Z])*.[a-zA-Z]{2,3}$");

private String email;

Choose a reason for hiding this comment

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

Suggested change
private String email;
private final String email;

Copy link
Author

@1o18z 1o18z Aug 7, 2023

Choose a reason for hiding this comment

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

패스워드랑 이메일은 VO이므로 불변이여야함이 맞지만 @Embeddable을 사용해서 final을 사용할 수 없습니다. 그래서 final을 붙이지 않고 변경할 수 있는 메서드를 생성하지 않는 방식으로 했습니다 !


private static final Pattern PASSWORD_REGEX = Pattern.compile("^(?=.*[a-zA-Z])(?=.*\\d)(?=.*\\W).{8,20}$");

private String password;

Choose a reason for hiding this comment

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

Suggested change
private String password;
private final String password;

@Embeddable
@Getter
@NoArgsConstructor(access = AccessLevel.PROTECTED)
public class Email {

Choose a reason for hiding this comment

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

equals, hashCode는 오버라이딩 하지 않아도 될까요?

Comment on lines 21 to 33
void test() {
User user = User.builder()
.email("qkrdmswl1018@naner.com")
.password("Qwqer123?")
.name("박은지")
.age(23)
.hobby("산책")
.build();
userRepository.save(user);

User findUser = userRepository.findByEmailAndPassword(new Email("qkrdmswl1018@naner.com"), new Password("Qwqer123?")).get();
System.out.println(String.format("Email: %s, Password: %s", findUser.getEmail(), findUser.getPassword()));
}

Choose a reason for hiding this comment

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

어떤 테스트인가요?

Copy link
Member

@dojinyou dojinyou left a comment

Choose a reason for hiding this comment

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

고생하셨습니다

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants