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
[1기-P] 김다희 - JPA 게시판 구현하기 #30
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.
테트리스 장인 다희님 코드 깔끔하네요 ㅎ
옥상에서 늦게까지 공부도 열심히 하시고
질문도 많이 하는 모습 멋집니다!!
따봉~ 👍
src/main/java/com/prgrms/board/error/ValidExceptionHandler.java
Outdated
Show resolved
Hide resolved
@Transactional | ||
@SpringBootTest | ||
class PostRepositoryTest { |
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.
@SpringBootTest 대신에 @DataJpaTest를 쓰는걸 추천드립니다~
SpringBootTest를 쓰면 JPA와 관련 없는 모든 빈들까지 다 등록이 되지만,
DataJpaTest는 JPA 관련 빈들만 등록합니다
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.
이 부분은 적용해보았더니 오류가 .. 납니다!...
@sangmin7648 @eunseo2 리뷰 부탁 드립니다! |
this.content = content; | ||
} | ||
|
||
public void changeInfo(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.
changeInfo라는 이름이 저는 조금 모호하게 느껴지는게 어떤가요?🤔
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.
changePostInfo로 적용해보았는데.. 어떠신가요..?!
public record PostCreateRequest( | ||
@Positive Long userId, | ||
@NotBlank(message = "제목이 비어있습니다.") @Length(max = 250) 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.
public record PostCreateRequest( | |
@Positive Long userId, | |
@NotBlank(message = "제목이 비어있습니다.") @Length(max = 250) String title, | |
String content) { | |
} | |
public record PostCreateRequest( | |
@Positive | |
Long userId, | |
@NotBlank(message = "제목이 비어있습니다.") @Length(max = 250) | |
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.
바꾼 포메팅도 잘 돌아가네요!! ㅋㅋㅋㅋ
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.
전 개인적으로 필드 어노테이션은 new line 이 좋은 것 같습니다.
import org.springframework.web.bind.annotation.RestControllerAdvice; | ||
|
||
@RestControllerAdvice | ||
public class ValidExceptionHandler { |
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 class ValidExceptionHandler { | |
public class InvalidExceptionHandler { |
마찬가지로 또는 NotValidExceptionHandler가 어떤가요?
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.
이 부분도 InValidExceptionHandler로 적용하였습니다~
@Lob | ||
private 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.
@ Lob을 쓰면 nullable = false를 할 수 없는 단점이 있더라구여!🙄
찾아본 결과 이렇게도 바꿀 수 있었습니다!
@Column(name = "content", nullable = false, columnDefinition = "TEXT")
private String content;
@neilsonT @sangmin7648 @eunseo2 리뷰주신 부분 반영하였습니다! 꼼꼼한 코드리뷰 감사합니다! |
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.
지나가다 들렸읍니다. 정말 잘하셨네요
스윽 보고 갑니다~
@Transactional | ||
@Service | ||
public class PostService { |
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.
다희님 하이~ 지나가다 들렸는데요
클래스 전체에 @Transactional(readOnly = true)
로 범위를 좁혀서 걸어놓고,
조회하는 메소드에는 어노테이션을 생략하구 테이블 변경시키는 어노테이션에만 @Transactional
달아줘도 될것 같아여
지금 어노테이션이 중복되서 쫌 들어가있는것 같아서용
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.
오 ! 이 부분은 조금 더 공부해보고 재원님이 말씀해주신 방향으로 반영하도록 하겠습니다 ! 감사합니다~!
@Builder | ||
private User(Long id, String name, int age, String hobby){ | ||
this.id = id; | ||
this.name = name; | ||
this.age = age; | ||
this.hobby = hobby; | ||
} | ||
|
||
public void changeUserInfo(String name, int age, String hobby){ | ||
this.name = name; | ||
this.age = age; | ||
this.hobby = 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.
다희님 언제부터 이렇게 객체지향적 사람으로 성장하셨나요
저도 객체지향맨이 되기 위해 분발하겠습니다
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.
ㅋㅋㅋㅋ칭찬 감사합니다! 저도 재원님의 열쩡에 항상 동기부여 받고 갑니당
@PutMapping("/{id}") | ||
public ResponseEntity<IdResponse> modifyPost(final @PathVariable Long id, final @Valid @RequestBody PostModifyRequest postModifyRequest){ |
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.
요거 PATCH 메소드가 더 Restful 할것같아요~~
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.
아! 저는 많은 부분을 수정할때는 put이 더 어울린다고 생각합니다! 주로 1개의 필드를 수정할 때 패치를 사용하는게 적절하다고 생각합니다! PUT 과 PATCH 에 대해서 재원님은 어떻게 생각하시나요?
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.
PUT은 모든 컬럼을 업데이트 시켜줄때 사용한다고 해여
https://programmer93.tistory.com/39 이 블로그 참고해보세용
@hanjo8813 리뷰 감사합니다!🌱 |
import org.springframework.web.bind.annotation.*; | ||
|
||
@RestController | ||
public class PostFilterController { |
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.
검색용 API만 모아두실 의도로 만드신것같은데.
이렇게 별도로 나누신 이유는 뭔가요??
public class BaseEntity { | ||
|
||
@CreatedDate | ||
private LocalDateTime createdAt; |
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 void changePostInfo(String title, String content){ | ||
this.title = title; | ||
this.content = 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.
this.updatedAt
필드도 함께 업데이트 해주는건 어떨까요? 단 이때는 파라미터로 입력 받지 않고 블럭 내에서 날짜를 대입 해주면 좋을 것 같습니다.
import lombok.Getter; | ||
import lombok.NoArgsConstructor; | ||
|
||
import javax.persistence.*; |
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.
Static 하지 않다면 와일드 카드보다는 single import 를 할 수 있도록 ide 설정을 해보세요 :)
import lombok.Getter; | ||
|
||
@Getter | ||
public class IdResponse { |
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.
id를 위해 DTO 가 필요할까요?
public record PostCreateRequest( | ||
@Positive Long userId, | ||
@NotBlank(message = "제목이 비어있습니다.") @Length(max = 250) 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.
전 개인적으로 필드 어노테이션은 new line 이 좋은 것 같습니다.
return postRepository.findById(id).orElseThrow(() -> new NotFoundException("해당 게시글을 찾을 수 없습니다.")); | ||
} | ||
|
||
private User getUser(final Long 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.
해당 메서드는 UserService 에서 작성하는 것이 좋을 것 같습니다.
} | ||
|
||
private User getUser(final Long id) { | ||
return userRepository.findById(id).orElseThrow(() -> new NotFoundException("해당 사용자를 찾을 수 없습니다.")); |
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.
message 에 id 값을 다시 넣어 주셔도 좋을 것 같습니다.
내부데이터가 아닌 사용자의 입력 값은 다시 반환 해도 상관없다고 생각합니다.
에러 메시지도 코드가 아닌 propertise 로 관리해봐도 좋을 것 같네요.
@Transactional | ||
public IdResponse modifyUser(final Long id, final UserCreateRequest userCreateRequest){ | ||
User user = getUser(id); | ||
user.changeUserInfo(userCreateRequest.getName(), userCreateRequest.getAge(), userCreateRequest.getHobby()); |
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.
UserCreateRequest
네이밍이 길어 지다 보니 인자 3개만 넣어도 길이가 길어지고 가독성이 높은가? 라는 의문도 있습니다.
이럴땐 예외적으로 변수명을 줄여도 좋을 것 같습니다.
|
||
datasource: | ||
driver-class-name: com.mysql.cj.jdbc.Driver | ||
url: ${DATABASE_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.
👍
|
||
@Override | ||
public boolean isValid(String field, ConstraintValidatorContext cxt){ | ||
return field != null && field.matches("^[가-힣a-zA-Z]*$"); |
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.
"^[가-힣a-zA-Z]*$"
는 static final String 으로 따로 빼도 좋을 것 같습니다.
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.
지나가다가 리뷰 남겨봅니다.
전체적으로 좋은 코드를 짜기 위해 엄청 고민하고 노력하셨다는게 보이는 코드였습니다!
저도 이렇게 열정적으로 해야하는데 ㅜㅜ 반성하고 갑니다.
} | ||
|
||
@PostMapping | ||
public ResponseEntity<IdResponse> createPost(@Valid @RequestBody PostCreateRequest postCreateRequest){ |
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.
요기만 final이 빠져있습니다! ㅎㅎ
@NotBlank(message = "제목이 비어있습니다.") | ||
private String title; | ||
|
||
private 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.
Entity에는 길이 제한 조건이 걸려있는데 , DTO에는 이에 대한 Validation Check 없군요.
DTO에도 이에 대한 Check를 해주면 좋을 것 같습니다! ㅎㅎ
|
||
@Transactional(readOnly = true) | ||
public Page<PostFindResponse> findAllPostByUserId(Pageable pageable, final Long userId){ | ||
User user = getUser(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.
취향차이이긴 합니다만 "getUser()" 라는 네이밍이 뭔가 일반적인 Getter랑 네이밍 방식이 똑같다보니
개발하다가 햇갈릴 수도 있을 것 같습니다.
"get**"말고 "find**" 같은 네이밍은 어떨까요?
@Getter | ||
public class PostFindResponse { | ||
|
||
private Long id; | ||
|
||
private String title; | ||
|
||
private String content; | ||
|
||
private LocalDateTime createdAt; | ||
|
||
private LocalDateTime updatedAt; | ||
|
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.
작성자에 대한 정보도 같이 내려줘보는건 어떨까요?
하나의 Entity만 내려주기 보다는 연관된 2가지 Entity의 정보를 같이 내려줘보시면
거기서도 공부가 되시는 부분이 있을 것 같습니다! ( 페치조인이라던가, 배치사이즈 라던가.. ㅎㅎ )
import org.springframework.web.bind.annotation.*; | ||
|
||
@RestController | ||
public class PostFilterController { |
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.
검색용 API만 모아두실 의도로 만드신것같은데.
이렇게 별도로 나누신 이유는 뭔가요??
public record InvalidErrorResponse(String field, String message) { | ||
|
||
@Builder | ||
public InvalidErrorResponse { | ||
} | ||
|
||
} |
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.
GlobalErrorResponse과는 다르게 InvalidErrorResponse는 record를 사용하셨는데,
그냥 class가 아닌 record를 사용하신 이유는 뭔가요?
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.
InvalidErrorResponsed와 GlobalErrorResponse 두개로 나눠서 사용하고 계시는군요.
혹시 이렇게 두가지로 나눠서 사용하시는 이유가 있나요?
저같은 경우 ErrorResponse를 하나로 사용하고 있다보니.. ㅎㅎ
제 생각에는 현재 발생한 예외가 뭔지 구분할 에러코드도 같이 내려준다면 더 좋지 않을까 합니다.
만약 클라이언트 개발자가 에러객체를 받았을 때,
나눠주신 두가지 에러객체를 어떻게 구분해서 그에 맞는 로직을 처리할지 생각해본다면
지금 생성해주신 구조로는 받은 에러객체를 개발자가 직접 까서 "field"라는 필드가 있는지 확인하거나
받은 message가 뭔지 직접 확인한 후에야 어떤 에러가 발생한건지 구분이 가능할꺼라 생각됩니다.
그리고 예외를 이렇게 코드로 구분한다면 에러객체 또한 하나로 통일해서 사용할 수 있을꺼라 생각합니다.
에러객체가 하나로 통일된다면 에러객체의 포맷이 하나로 정해진다는 말이고
클라이언트 개발자는 모든 통신에서 동일한 에러포맷을 보장받으니
에러처리 로직 또한 하나의 템플릿으로 만들어서 사용 할 수 있을꺼라 생각합니다.
📌 과제 설명
💻 개발 환경
👩💻 요구 사항과 구현 내용
📍 엔티티 구성
📍 API 구현
User API
Post API
ERD
Directory