-
Notifications
You must be signed in to change notification settings - Fork 0
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
[feat] 족보 생성 삭제 API #23
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.
수고하셨습니당
import java.util.List; | ||
|
||
public record FavoritePostRequest( | ||
@NotEmpty(message = "제목이 비었습니다.") |
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.
@notempty 보다는 @notblank 어노테이션이 더 적절한 것 같습니다. 아래 내용을 참고해주세요.
https://wildeveloperetrain.tistory.com/68
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 final FavoriteRepository favoriteRepository; | ||
|
||
public Favorite findById(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.
변수명을 단순히 id라 하는 것보다는 어떤 객체의 id인지 표현해주는 것이 좋을 것 같은데(ex) userId) 어떻게 생각하시나요?
@Transactional | ||
public Long create(final FavoritePostCommand favoritePostCommand) { | ||
|
||
long userId = favoritePostCommand.memberId(); |
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.
memberId말고 테이블 명 그대로 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.
놓쳤습니다ㅜㅜ 반영했습니다!
.build(); | ||
} | ||
|
||
@Builder |
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 작성해주면 해당 내용 없이도 builder 사용하실 수 있어요!
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 붙이는 거 선호합니다.
|
||
@RestController | ||
@RequiredArgsConstructor | ||
@RequestMapping("/api/v1/favorites") |
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.
@RequestMapping에는 /api/v1만 남겨주세요!
376805d
to
7c8a47e
Compare
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.
고생하셨습니다
return HankkiResponse.success(CommonSuccessCode.CREATED); | ||
} | ||
|
||
@PostMapping("/favorites/delete") |
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.
다중 삭제 기능이란 것을 명확하게 나타내기 위해 /favorites/batch-delete 라고 하는 것은 어떨까요?
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.
반영하겠습니다 👍
List<String> details | ||
) { | ||
|
||
public static FavoritePostCommand of(final Long memberId, final FavoritePostRequest favoritePostRequest) { |
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.
memberId -> userId로 수정해주세요
@RequiredArgsConstructor | ||
public class FavoriteCommandService { | ||
|
||
private final FavoriteRepository favoriteRepository; |
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.
favoriteRespository.save를 favoriteUpdater 안으로 넣어주세요
@PostMapping("/favorites") | ||
public HankkiResponse<Void> createFavorite(@UserId final Long userId, @RequestBody @Valid final FavoritePostRequest request) { | ||
|
||
favoriteCommandService.create(FavoritePostCommand.of(userId, 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.
원래는 201을 내려주면 Location 헤더에 resource의 위치를 포함시켜야 해서
저희 응답을 처리하는 advice에 추후 201상태 코드 가진 친구들에게 location header를 추가해주는 코드를 추가할 예정입니당.
이를 대비해서 create이후에 생성된 리소스의 pk 사용하지 않더라도 일단 갖고 있을 수 있을까요?
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 return 타입을 Long으로 변경하고
return favoriteRepository.save(Favorite.create(findUser, title, details)).getId();
으로 변경하겠습니다.
.build(); | ||
} | ||
|
||
@Builder |
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 붙이는 거 선호합니다.
@NotBlank(message = "제목이 비었습니다.") | ||
@Size(max = 20, message = "제목 길이가 20 이상입니다.") | ||
String title, | ||
List<String> details |
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.
@SiZe 추가해서 최대 리스트 요소수 추가하는 것은 어떨까요?
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.
일단 @SiZe가 List size에도 적용되는지 Test 해보고 된다면 요구사항대로 1이상 2이하로 바꾸겠습니다 그리고 제목 길이는 18자 이하로 변경되어서 그것도 반영하겠습니다!
@Query("delete from Favorite f where f in :favorites") | ||
void deleteAll(@Param("favorites") List<Favorite> favorites); | ||
|
||
@Query("select f from Favorite f join fetch f.user where f.id in :favoriteId") |
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.
favorites.members의 각각의 멤버의 id를 갖고오기 위해 fetchjoin을 하고 계신 거 같은데 pk같은 경우는 지연로딩만으로도 조회를 할 수 있는 것으로 알고 있습니다. 한번 확인해주시겠어요?
그리고 fetch join쓰시는 경우에는 함수명에 명시적으로 적어주세요
|
||
public List<Favorite> findByIds(final List<Long> ids) { | ||
return favoriteRepository.findByIds(ids); | ||
} |
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.
사소하지만 복수의 값을 반환한다면 findAllbyIds로 명명하는 것은 어떨까요?
Long userId = command.userId(); | ||
favorites.forEach(favorite -> { | ||
if (!favorite.getUser().getId().equals(userId)) { | ||
throw new UnauthorizedException(UserErrorCode.USER_FORBIDDEN); | ||
} | ||
}); |
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 final FavoriteRepository favoriteRepository; | ||
public void deleteAll(List<Favorite> favorites) { | ||
|
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.
공백 맞춰주세요
Related Issue 📌
close #16
Description ✔️
To Reviewers
족보 생성 응답값
족보 생성 쿼리
(에러) 족보 title 길이 20 이상 or 빈 문자열
족보 삭제 쿼리실행 Test
나중에 반영해야할 수도 있는 반영사항.