-
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기 김민희,박세영] Spring Boot JPA로 게시판 구현 미션 제출합니다. #261
base: 김민희/박세영
Are you sure you want to change the base?
Conversation
- PermissionDeniedException - PostNotFoundException
- Controller & Service
- Controller & Service
fieldWithPath("userResponses").type(JsonFieldType.OBJECT).description("유저 응답"), | ||
fieldWithPath("userResponses.content[]").type(JsonFieldType.ARRAY).description("유저 정보 배열"), | ||
fieldWithPath("userResponses.content[].id").type(JsonFieldType.NUMBER).description("유저 아이디"), | ||
fieldWithPath("userResponses.content[].name").type(JsonFieldType.STRING).description("유저 이름"), | ||
fieldWithPath("userResponses.content[].age").type(JsonFieldType.NUMBER).description("유저 나이"), | ||
fieldWithPath("userResponses.content[].hobby").type(JsonFieldType.STRING).description("유저 취미"), | ||
fieldWithPath("userResponses.content[].createdAt").type(JsonFieldType.STRING).description("유저 생성일"), | ||
fieldWithPath("userResponses.content[].updatedAt").type(JsonFieldType.STRING).description("유저 갱신일"), | ||
fieldWithPath("userResponses.pageable").type(JsonFieldType.OBJECT).description("pageable").ignored(), | ||
fieldWithPath("userResponses.last").type(JsonFieldType.BOOLEAN).description("last").ignored(), | ||
fieldWithPath("userResponses.totalElements").type(JsonFieldType.NUMBER).description("totalElements"), | ||
fieldWithPath("userResponses.totalPages").type(JsonFieldType.NUMBER).description("totalPages"), | ||
fieldWithPath("userResponses.size").type(JsonFieldType.NUMBER).description("size").ignored(), | ||
fieldWithPath("userResponses.number").type(JsonFieldType.NUMBER).description("number").ignored(), | ||
fieldWithPath("userResponses.sort.empty").type(JsonFieldType.BOOLEAN).description("sort.empty").ignored(), | ||
fieldWithPath("userResponses.sort.sorted").type(JsonFieldType.BOOLEAN).description("sort.sorted").ignored(), | ||
fieldWithPath("userResponses.sort.unsorted").type(JsonFieldType.BOOLEAN).description("sort.unsorted").ignored(), | ||
fieldWithPath("userResponses.first").type(JsonFieldType.BOOLEAN).description("first").ignored(), | ||
fieldWithPath("userResponses.numberOfElements").type(JsonFieldType.NUMBER).description("numberOfElements").ignored(), | ||
fieldWithPath("userResponses.empty").type(JsonFieldType.BOOLEAN).description("empty").ignored() |
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.
저희는 JPA pageable을 사용하기 때문에 asciidocs에서 모든 pageable 속성을 명시해줘야 했습니다.
필요한 정보만 노출하는 방법은 필요한 Page 속성을 가진 새로운 DTO를 도입하는 것입니다.
하지만, 저희는 Page 를 감싸고 있는 DTO를 정의해 사용하고 있기 때문에 모든 필드를 명시하는 방향으로 작성했습니다.
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/resources/application.yml
Outdated
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.
Jasypt 모듈을 사용해서 yaml 파일 보안했습니다.
@ParameterizedTest | ||
@DisplayName("유저를 전체 조회한다.") | ||
@MethodSource("userCreateRequest_Data") | ||
void getAllUsers_Pageable_ReturnResponses(List<UserCreateRequest> requests) throws Exception { |
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.
파라미터 requests가 코드 내에서 사용되고 있지 않은 것 같습니다!
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.
에러 코드를 정말 깔끔하게 관리하셨군요!!
심지어 사용하는 도메인별로 나누어 놓으셔서 더 보기 좋은 것 같습니다!!
.findById(postId) | ||
.orElseThrow(() -> new PostNotFoundException(NOT_FOUND_POST)); | ||
|
||
if (!Objects.equals(targetPost.getUser().getId(), 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.
이런 조건 특히 !가 들어간 조건문은 다른 개발자가 코드를 해석할때 가독성을 저하시킬 수 있을것 같아요...
isNotOwner 이런 느낌의 메서드명으로 분리하는건 어떻게 생각하시나요??ㅎㅎ
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.
boolean isNotOwner = !Objects.equals(targetPost.getUser().getId(), 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.
targetPost.isOwner(userId)
hibernate: | ||
type: | ||
descriptor: | ||
sql: trace |
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.
현재 테스트 코드내에서 test 프로파일 설정된 곳이 없어 이 yml파일이 필요 없을것 같습니다!!
this.content = content; | ||
} | ||
|
||
public void changePost(PostUpdateRequest post) { |
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.
엔티티가 서비스 레이어에 있는 dto에 의존하는것이 저는 좀 어색한 것 같아요ㅎㅎ
이렇게 되면 단방향을 지키기 위해 controller와 service레이어의 dto를 따로 나누었는데
정작 entity와 service간에 양방향이 생기고 있다고 저는 생각듭니다!!
민희님과 세영님은 어떻게 생각하시나요??
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.
사실 이정도면 그냥 title, content로 쪼개도 괜찮다고 생각합니다 ㅎㅎ 굳이 request 객체 넘기지 않고요.
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.
방향에 대한 고려로 크게 두 가지 해결법을 생각해봤습니다.
- DTO 사용말고 분해해서 파라미터로 넘기는 방법
- Entity가 있는 model 패키지 안에서 별도로 Converter와 DTO를 따로 생성해서 사용
저희는 컨벤션으로 5개 미만의 필드인 경우에는 따로 DTO를 만들지 않고 1번 방법처럼 분해해서 사용하는 것이 좋다고 생각합니다.
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.
코드가 전반적으로 깔끔하네요 ㅎㅎ 리뷰내용 많이 달아놓긴 했지만 계속 인지시켜줘야하는 부분이라서 request change 까지는 필요 없을 것 같습니다. 하지만 반드시 리뷰내용은 검토하고 지나갔으면 좋겠습니다. 수고하셨습니다!
this.controllerConverter = controllerConverter; | ||
} | ||
|
||
@PostMapping(value = "/{userId}", consumes = MediaType.APPLICATION_JSON_VALUE) |
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.
consumes 보다는 produces가 모아두기 좋겠네요. body가 없는데 굳이 consumes를 둘 필요가 없어서요.
.body(postService.savePost(userId, controllerConverter.toCreateRequest(createDto))); | ||
} | ||
|
||
@PatchMapping(value = "/{postId}/user/{userId}", consumes = MediaType.APPLICATION_JSON_VALUE) |
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.
path 로 쓰는것 같은데 여기는 흘리셨네요 ㅎㅎ
.status(HttpStatus.OK) | ||
.body(postService.findPostsByUserId(userId, pageable)); | ||
} | ||
|
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.
getAllPosts 와 getPostsByUserId 두 API는 통합할 수 있을 것 같은데요?
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
를 넣는다는 생각으로 통합할 수 있을 것 같아요 🐣
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.
requestParam을 사용하여 getAllPosts와 getPostsByUserId 두 API 통합하는 방식으로 수정했습니다!
.body(postService.findPostById(postId)); | ||
} | ||
|
||
@GetMapping(value = "/user/{userId}", produces = MediaType.APPLICATION_JSON_VALUE) |
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=xxx 로 request parameter 사용 권장.
불필요한 path 인것 같아요. 여기는 post 도메인이니
@NotBlank | ||
String title, | ||
|
||
@NotNull | ||
@Size(max = 255) | ||
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.
메시지를 명시해주는게 좋을 것 같습니다.
@Embeddable | ||
public class Name { | ||
|
||
@NotBlank |
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.
not blank도 생성자에서 해주는게 더 좋겠네요.
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.
참고로 생성자 파라미터에 갖다 붙이라는게 아니라 StringUtils.isNotBlank(name) 혹은 require(StringUtils.isNotBlank(name)) 이런 내용이 코드에 있어야 한다는 이야기입니다.
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.
StringUtils.isNotBlank가 딱 원하는 메소드라 생성자에 넣어두었습니다.
|
||
@Transactional | ||
public UserResponse saveUser(@Valid UserCreateRequest dto) { | ||
return converter.toResponse(repository.save(converter.toEntity(dto))); |
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.
취차..
log.warn("{}", errorResponse); | ||
log.warn("{}", e.getCause()); | ||
return ResponseEntity | ||
.status(HttpStatus.NON_AUTHORITATIVE_INFORMATION) |
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.
403으로 변경했습니다
|
||
@ParameterizedTest | ||
@DisplayName("존재하는 유저로 게시글을 생성하면 성공한다.") | ||
@MethodSource("providePostCreateRequest") |
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.
providePostCreateRequest 텍스트 오타가 발생하는 경우에 type safe하게 호출하긴 어려울 것 같아요.
ArgumentsSource 를 한번 알아봅세다.
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.
ArgumentSource를 이용하여 데이터를 주입받는 방식으로 변경했습니다!
(inner static 클래스 이용)
|
||
@SpringBootTest | ||
@Transactional | ||
class PostServiceTest { |
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.
전체 테스트 코드중에 service 테스트위주로 진행되었는데. web 계층에 선언해놓은 validation 코드는 어떻게 테스트가 되고 있는지도 돌아보면 좋을 것 같습니다.
.status(HttpStatus.OK) | ||
.body(postService.findPostsByUserId(userId, pageable)); | ||
} | ||
|
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
를 넣는다는 생각으로 통합할 수 있을 것 같아요 🐣
private Long id; | ||
|
||
@Column(nullable = false) | ||
private String title; |
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 연산자를 통해서 title이 빈 값이 들어온다면, 테스트 코드의 경우 null 상태의 title을 만들 수 있는데 어떻게 방어할 수 있을까요? 🐣
|
||
@Column(nullable = false) | ||
@Positive | ||
private int ageValue; |
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.
만약 사용자가 2000살이라고 잘못 입력하면 어떻게 될까요? 🐣
- final 키워드 추가 - PermissionDeniedHandler http 코드 수정
- 작성자 여부 확인하는 boolean 변수 추출 - 개행
📌 과제 설명
요구 사항
SpringDataJPA 를 설정한다.
엔티티를 구성한다
API를 구현한다.
Post
User
REST-DOCS를 이용해서 문서화한다.