-
Notifications
You must be signed in to change notification settings - Fork 203
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
[5기 이세희, 이중원] Spring Boot JPA 게시판 구현 미션 제출합니다. #274
base: 세희,중원mission
Are you sure you want to change the base?
[5기 이세희, 이중원] Spring Boot JPA 게시판 구현 미션 제출합니다. #274
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.
과제 수고하셨습니다~!👍🏻👍🏻
mysql/init.sql
Outdated
constraint post_user_fk FOREIGN key (user_id) references users(user_id) | ||
); | ||
|
||
insert into users (name, age, hobby, created_at, updated_at) values ('test', 20, 'coding', now(), now()); |
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.
데이터베이스 생성과 테스트 데이터 삽입 파일을 분리하는 것은 어떨까요? (ex. schema.sql, init.sql)
지금은 개발 초기 단계라 테스트 데이터가 있으면 좋지만, 나중에 필요 없을 수 있을 것 같아서요!
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.
과제 내용에 User를 create 하는 메서드가 없어서 추가하긴 했지만 data와 schema로 해서 나누는 게 좋아보이긴 해요 감사합니당
mysql/init.sql
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.
ddl auto 속성을 create
로 주지 않고, 직접 sql에 짠 부분이 좋은 것 같습니다!👍🏻
src/.DS_Store
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.
DS_Store는 맥북쓰면 자동 생성되는 파일이라 .gitignore에 추가 해도 좋을 것 같아요~
@PostMapping | ||
public Response<Long> save(@RequestBody @Valid PostDto postDto, BindingResult bindingResult) { | ||
bindChecking(bindingResult); | ||
return Response.success(postService.save(postDto)); |
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.
리소스 생성의 경우에는 200 OK 보다는 201 Created 응답을 보내면 좋을 것 같습니다!
|
||
private static void postDetailResponseValidate(Post post) { | ||
if (post.getId() == null) { | ||
throw new BaseException(ErrorMessage.POST_NOT_FOUND); |
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.
에러 메시지 상수로 관리해서 깔끔해 보입니다👍🏻
import java.util.ArrayList; | ||
import java.util.List; | ||
|
||
public class 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.
저도 인재님 의견에 동의합니다!
Validation 클래스가 util 클래스라면 명시적으로 final
키워드를 class 앞에 붙이고, private 생성자를 만들어 주면 좋을 것 같습니다!
@SpringBootTest | ||
@AutoConfigureRestDocs | ||
@AutoConfigureMockMvc | ||
@ActiveProfiles("test") |
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.
@IjjS 몰랐는데, 감사합니당!
@DisplayName("제목이 20자 이상인 경우 예외를 발생시킨다.") | ||
void saveFailWithLongTitle() throws Exception { | ||
//given | ||
PostDto postDto = new PostDto(1L, "test222222222222222222222222", "test Contents2"); |
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.
긴 제목을 만들 때, ParameterizedTest의 @MethodSource
를 사용하면 가독성이 올라갈 것 같습니다!
긴 문자열을 리턴하는 메서드를 만들어서 @MethodSource
로 지정해주면 해당 메서드의 리턴 값을 파라미터로 받아서 사용할 수 있습니다!
//given | ||
|
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.
이 경우에 given에서 Long invalidId = 0;
로 지정해줘서 when절에서 사용해도 될 것 같아요!
MockMvc mockMvc; | ||
|
||
|
||
@Nested |
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.
저도 중원님 덕분에 처음 알았는데 확실히 테스트 결과를 확인할 때도, 상황을 분리할 때에도 좋은 것 같더라구요!
Co-authored-by: Injae Song <[email protected]>
Co-authored-by: Injae Song <[email protected]>
Co-authored-by: Injae Song <[email protected]>
…java Co-authored-by: Injae Song <[email protected]>
Co-authored-by: Injae Song <[email protected]>
Co-authored-by: Injae Song <[email protected]>
유저별 postCount를 Redis에 저장할 클래스 생성 - 유저가 글을 작성하는 상황별 로직 추가 * LocalDateTime 직렬화를 위한 @JsonSerialize, @JsonDeserialize 도입
RedisTemplate을 이용한 PostCounter 저장 및 탐색 메서드 추가
- "6830:6379" | ||
|
||
volumes: | ||
data_volume: |
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.
scripts 같은 패키지에 Dockerfile과 함께 두면 더 좋을 것 같습니다.
@Transactional | ||
public Long save(PostCreateDto postDto) { | ||
User user = userRepository.findById(postDto.userId()).orElseThrow(() -> | ||
new BaseException(ErrorMessage.USER_NOT_FOUND) |
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.
BaseException으로 포괄하는 exception 사용보다 클래스명에서 의미를 내포하는 형태가 더 가독성이 좋다고 느껴집니다!
private String title; | ||
|
||
@Lob | ||
private String contents; |
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.
복수형으로 되어있으니까 collection일 것 같은 느낌이 들어서 content로 하면 어떨까 생각합니다.
|
||
@GetMapping | ||
public Response<Page<PostResponseDto>> readAllPost( | ||
@PageableDefault Pageable 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.
offset, pageSize가 음수가 되거나 존재하는 컨텐츠 개수보다 커지는 경우는 어떤 식으로 처리되나요??
@PatchMapping("/{postId}") | ||
public Response<Long> update( | ||
@PathVariable Long postId, | ||
@RequestBody @Valid PostUpdateDto postUpdateDto, |
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.
@Valid를 통해 validation을 했을 때, 부분적인 validation이 되는 건가요?? 그게 안된다면 PATCH를 이용하는 이유가 없어보입니다. 그리고 실 서비스들은 어떤 식으로 update를 하는 지 찾아보고 여러 방법들을 생각해보면 좋을 것 같습니다!
post.getContents()); | ||
} | ||
|
||
private static void postDetailResponseValidate(Post 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.
Post model 내부에도 validation을 위한 비슷한 코드가 있고 DTO에서도 비슷한 코드가 있네요. 만약 title의 최대 길이가 20에서 30으로 바뀌었다고 생각하면 변경 지점이 2군데가 되고 IDE를 이용한 오류 감지가 불가능하기에 실수가 발생할 수 있어보입니다. export 해서 중복된 코드를 줄여보는 것이 좋을 것 같습니다.
@NoArgsConstructor(access = AccessLevel.PROTECTED) | ||
@Entity | ||
@Table(name = "users") | ||
public class User extends BaseEntity { |
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을 넣어보면 좋을 것 같습니다.
@MethodSource("invalidTitle") | ||
@NullAndEmptySource | ||
@DisplayName("post 생성 시 제목이 null, 빈 공백, 20자 이상인 경우 post 객체 생성에 실패한다.") | ||
void saveEntityFailWithInvalidTitle(String input) { |
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.
여기에 20자 이상인 parameter도 들어가 있는 건가요?? 그리고 null, 공백, 20자 이상은 같은 예외 상황일지 한번 생각해봐도 좋을 것 같습니다!
@AutoConfigureMockMvc | ||
@ActiveProfiles("test") | ||
@Transactional | ||
class PostControllerTest { |
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.
controller unit test 외에도 e2e 테스트를 작성해봐도 좋을 것 같습니다. TestRestTemplate 같은 것들을 찾아보면 좋을 것 같아요.
driver-class-name: com.mysql.cj.jdbc.Driver | ||
url: jdbc:mysql://localhost:3308/board | ||
username: root | ||
password: root |
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.
env로 관리해보면 좋을 것 같습니다.
datasource:
driver-class-name: com.mysql.cj.jdbc.Driver
url: jdbc:mysql://localhost:3306/${MYSQL_DATABASE}
username: ${MYSQL_USER}
password: ${MYSQL_PASSWORD}
이런 느낌으로 쓰는데 MYSQL_USER 환경변수들을 가진 env 파일을 두고 거기서 읽어오는 방식으로 바꾸고 env 파일은 git에 올리지 않으면 보안적으로 더 좋을 것 같습니다.
📌 과제 설명
JPA를 이용한 게시판 구현 과제입니다!
👩💻 요구 사항과 구현 내용
🚀 실행 방법
docker-compose up -d
명령어를 사용한다.✅ PR 포인트 & 궁금한 점
PostTest.java
에서 user 객체가 null이 되는 케이스를 구현했는데 구현 로직이 의미가 있는지 잘 모르겠습니다! 테스트를 위한 로직을 짜는 것 같은 생각이 들었습니다.PostServiceTest.java
PostGetAll
의 각 테스트 케이스에서 Pageable로 저희가 페이지를 직접 설정해주었는데 어떻게 보이시는 지 궁금합니다!