-
Notifications
You must be signed in to change notification settings - Fork 204
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
base: 범준,은지-mission
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
클래스 이름을 좀 더 명확하게 작성해주어야 할 것 같아요
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GetMapping() | |
@GetMapping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수정하였습니다 ! :)
5ddedbc
@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); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수정 후 응답은 아무것도 안줘도 괜찮을까요?
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
인터셉터나 AOP를 활용해서 중복 코드를 줄 일 수 있겠는데요?
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validation은 필요 없나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
추가하였습니다 ! :)
public Long login(UserLoginRequest request) { | ||
User user = findByEmailAndPassword(request.email(), request.password()); | ||
return user.getId(); | ||
} |
There was a problem hiding this comment.
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 응답을 주어야 하는게 아닐까요?
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
기본으로 제공하는 Email 정규표현식과 직접 작성하신 정규표현식에는 어떤 차이가 있나요?
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이렇게하면 validation이 동작하나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제대로 동작하지 않는 것을 확인하여, 수정하였습니다 ! :)
d885391
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제거 해주세요~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제거하였습니다 ! :)
8eb1637
Post post = findPostById(postId); | ||
User user = findUserById(userId); | ||
|
||
post.checkAuthorize(user); | ||
postRepository.deleteById(postId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아니면 userId만 검증을 할 수도 있습니다! 연관 객체의 @id 붙은 필드는 프록시 객체가 가지고 있어서 별도로 조회 없이도 검증이 가능해요
There was a problem hiding this 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()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
예외 메시지를 그대로 클라이언트에 넘겨주면 안 될 것 같네요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'게시글을 찾을 수 없습니다'와 '유저를 찾을 수 없습니다'라는 메시지도 그대로 넘겨주면 안되는 걸까요 ??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
예외 메시지는 일반적으로 디버깅을 위해 사용해요. 그래서 어떤 값이 들어왔고 어떠한 이유로 실패했다는 내용이 들어가게 될 거에요.
시스템 내부 구현에 해당하는 정보를 그대로 클라이언트에 전달해도 좋을까요?
@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); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
url이 중복되는 것 같네요
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private String email; | |
private final String email; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private String password; | |
private final String password; |
@Embeddable | ||
@Getter | ||
@NoArgsConstructor(access = AccessLevel.PROTECTED) | ||
public class Email { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
equals, hashCode는 오버라이딩 하지 않아도 될까요?
void test() { | ||
User user = User.builder() | ||
.email("[email protected]") | ||
.password("Qwqer123?") | ||
.name("박은지") | ||
.age(23) | ||
.hobby("산책") | ||
.build(); | ||
userRepository.save(user); | ||
|
||
User findUser = userRepository.findByEmailAndPassword(new Email("[email protected]"), new Password("Qwqer123?")).get(); | ||
System.out.println(String.format("Email: %s, Password: %s", findUser.getEmail(), findUser.getPassword())); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
어떤 테스트인가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다
📌 과제 설명
👩💻 요구 사항과 구현 내용
1. 엔티티를 구성한다.
2. API를 구현한다.
3. REST-DOCS를 이용해서 문서화한다.
4. 추가 구현 사항
✅