9조 BE 코드리뷰 3회차 #52
9조 BE 코드리뷰 3회차 #52
Conversation
수정된 ERD에 맞게 ArtistInfo 수정
수정된 ERD에 맞게 BusinesArtist
수정된 ERD에 맞게 StudentArtist 변경
수정된 ERD에 맞게 User 변경
여러 사진 파일 업로드 기능
Weekly - 정기 병합
develop - 정기 병합
작가 페이징 목록 조회를 제외한 작가 조회,등록 기능 추가
작가관련 Dto 추가 작성
ArtistInfo 엔티티 필드 수정
간단한 테스트를 위한 Controller 수정
작가 서비스 로직 추가
작가 서비스 로직 추가
유저 관련 Dto 추가
ArtistType에 getter 추가
기존에 yaml 파일에 정의된 도커 컨테이너 mysql 과 연결안되던 문제를 해결함
master -> Master
Swagger Ui를 security에 추가
refactor:[#24]- swagger UI 추가
|
|
||
| @Configuration | ||
| public class S3Config { | ||
| @Value("${cloud.aws.credentials.access-key}") |
There was a problem hiding this comment.
Value로 받아도 좋은데 따로 Property 객체를 만들어서 바인딩 받아도 좋을것 같아요. 👍
| import com.helpmeCookies.global.exception.user.ResourceNotFoundException; | ||
|
|
||
| @ControllerAdvice | ||
| public class GlobalExceptionHandler { |
There was a problem hiding this comment.
Rest API라면 RestControllerAdvice가 조금더 적합한 애너테이션이 될수있을것 같아요~
There was a problem hiding this comment.
넵!!! 수정하겠습니다 감사합니다 : )
| private long refreshTokenExpireTime; | ||
| private String secret = "4099a46b-39db-4860-a61b-2ae76ea24c43"; | ||
| private long accessTokenExpireTime = 1800000; // 30 minutes; | ||
| private long refreshTokenExpireTime = 259200000; // 3 days; |
There was a problem hiding this comment.
- 애플리케이션 설정으로 받다가, 다시 바꾸신이유가 있을까요?
- 바뀌지 않는값이라면 final, 상수라면 static 붙여도 좋을것 같아요! 또한 불변상수라면 변수명을 대문자로 사용하시면 좋을것 같습니다.
There was a problem hiding this comment.
깃허브 액션을 사용하는 과정에서 jwt 설정을 못받아오는 경우가 생겨서, 임시로 값을 넣은 것으로 알고있습니다!! 추후 바인딩 할 수 있게 바꾸어두겠습니다.
| return fileList; | ||
| } | ||
|
|
||
| public String createFileName(String fileName) { |
| private String bucket; | ||
|
|
||
| //다중파일 업로드후 url 반환 | ||
| public List<FileUploadResponse> uploadMultiImages(List<MultipartFile> multipartFiles) { |
There was a problem hiding this comment.
조금 리팩토링해서 파일 하나를 업로드하는 uploadMultiImage() 메서드를 만들고 이걸 여러번 호출하는식으로 리팩토링하면 해당 Util을 사용할때 조금더 간편할수있지 않을까요?
public FileUploadResponse uploadMultiImage() {
}
public List<FileUploadResponse> uploadMultiImages(List<MultipartFile> multipartFiles) {
// uploadMultiImage() 여러번 호출
}
| return ResponseEntity.ok().build(); | ||
| } | ||
|
|
||
| @PostMapping("/{productId}/images") | ||
| public ResponseEntity<ProductImageResponse> uploadImages(@PathVariable("productId") Long productId, List<MultipartFile> files) throws IOException { | ||
| List<FileUploadResponse> responses = productImageService.uploadMultiFiles(productId,files); |
There was a problem hiding this comment.
이미지 업로드와 동시에 DB에 같이 저장을 하는식으로 동작하네요~
이렇게 작성해주셔도 너무 좋지만 약간의 걱정이 되는 부분이 있다면
- 이미지가 많아서 업로드가 느릴경우 문제가 될 수 있음 (DB 커넥션을 계속 물고있어서 서버에서 트랜잭션 타임아웃 발생)
- 이미지 업로드가 하나라도 실패한다면 처음부터 다시 업로드를 시작해야한다는점
위 2가지인데요, 사실 이미지 용량이 엄청 크거나, 몇십개씩 한번에 업로드하는게 아니라면 크게 문제가 되진 않을것 같아요.
다만 조금 동작을 바꿔서 이렇게 해볼수도 있을것 같은데요~
- 클라이언트에서 서버로 이미지 업로드 요청 (이때 이미지를 업로드만함, DB에 저장 X, productId도 받을 필요없음)
- 서버에서 클라이언트로 이미지 URL 응답
- 클라이언트에서 이미지들에 대해 업로드 완료이후, 얻은 URL로 다시 productId와 imageUrl들을 함께 저장 요청
- 서버에서는 해당 productId, imageUrl을 DB에 저장
이같은 플로우도 괜찮을것 같아요~
| ) { | ||
| // UserInfoDto를 통해서 유저 정보를 수정한다. | ||
| userService.updateUserInfo(userInfoReq.toDto(), jwtUser.getId()); | ||
| return "ok"; |
There was a problem hiding this comment.
되도록이면 모든 API에 대해서 JSON Content-Type 응답을 나가도록 프론트엔드에 값을 주시면 프론트엔드분들도 개발하기 수월할것 같아요~
| throw new IllegalArgumentException("이미 등록된 아티스트입니다."); | ||
| } | ||
|
|
||
| // ArtistInfo 생성 |
There was a problem hiding this comment.
저의 경우 현재 고민인 부분은 ArtistService 에서 클라이언트가 준 request 를 통해 ArtistInfo와 StudentArtist를 생성하는 부분에 있습니다. 이때 StudentArtist에서 ArtistInfo를 생성해서 담는 로직을 가지고 있는 것이 더 좋아보이는데, 어떻게 생각하시는지 궁금합니다!! 특히 이런 코드를 구현할 때 도메인의 생성자에서 예외처리를 하려고 하니 찝찝함이 많이 느껴지는데, 맞는 방법일까요?
-
요 질문에 해당하는 부분이 이쪽인것 같은데요~ ArtistInfo를 Service에서 생성하는 방법이나 StudentArtist에서 내부에서 생성하는 로직을 담고있는거나, 2가지 방식 모두 크게 다르진 않을것 같습니다. 두가지 방식 모두 선호하는방식으로 사용하고 어느한쪽이 나쁘다 좋다 이렇게 나누기는 어려운것 같아요.
-
생성자에서 예외처리를 하는건 찝찝하다고 생각안하셔도 괜찮을것 같아요. 실제로 생성자에서 많이 검증 및 데이터가 이상할 경우 예외가 발생하고, 값이 이상한데 도메인 객체가 만들어지는게 더 좋지 않은 상황일것 같습니다.
There was a problem hiding this comment.
팀원들과 토의 한 다음 선호하는 한 가지 방법을 선택하겠습니다. 좋은 리뷰 감사합니다 : )
|
|
||
| @Test | ||
| @DisplayName("상품 이미지 S3 서버에 업로드") | ||
| void uploadMultiFiles() throws IOException { |
There was a problem hiding this comment.
이미지 업로드를 S3를 이용해서 구현 중입니다. 이때 실제 S3가 아닌 로컬에서 테스트를 하고 싶은데 현재는 MockBean을 이용하고 있습니다. 근데 시간도 오래 걸리고 통합테스트보다는 util에 대한 기능을 테스트하고 싶은데 더 좋은 방법이 있을까요?
제가 요 질문을 정확하게 이해하지 못해서요 ㅠ S3를 호출하지 않고 MockBean처리하였지만, 해당 테스트가 느리다는 말씀이실까요..? S3를 호출하지않고 util에 대한 기능을 정확히 검증하기는 쉽지 않을것 같습니다. 해당 테스트는 정확하게 어떤걸 테스트하기 위한걸까요? 현재는 테스트하는 의미가 크게는 없어보이긴하는데, 혹시 부가적인설명을 해주시면 조금더 답변을 잘드릴수있을것 같습니다.
| public void addCorsMappings(CorsRegistry registry) { | ||
| registry.addMapping("/**") |
There was a problem hiding this comment.
브라우저에서 Origin이 달라서 CORS문제가 발생하나보네요.
헤더를 추가하기보다는, FE와 BE의 Origin을 맞추는 방식도 고민해보면 좋을것 같아요.
앞단의 웹 서버(ex nginx)에서 FE 관련 인덱스 파일도 내리고, BE관련 애플리케이션 서버로 리버스 프록시를 역할을 하면 Origin을 한번 맞춰볼수 있지 않을까요?
PR
✨ 작업 내용
지난 주차 리뷰에 말씀해주신 부분은 이번주 weekly 정기 병합에 추가하겠습니다!!
저의 경우 현재 고민인 부분은 ArtistService 에서 클라이언트가 준 request 를 통해 ArtistInfo와 StudentArtist를 생성하는 부분에 있습니다.
이때 StudentArtist에서 ArtistInfo를 생성해서 담는 로직을 가지고 있는 것이 더 좋아보이는데, 어떻게 생각하시는지 궁금합니다!!
특히 이런 코드를 구현할 때 도메인의 생성자에서 예외처리를 하려고 하니 찝찝함이 많이 느껴지는데, 맞는 방법일까요?
이미지 업로드를 S3를 이용해서 구현 중입니다. 이때 실제 S3가 아닌 로컬에서 테스트를 하고 싶은데 현재는 MockBean을 이용하고 있습니다. 근데 시간도 오래 걸리고 통합테스트보다는 util에 대한 기능을 테스트하고 싶은데 더 좋은 방법이 있을까요?
늦은 리뷰 받아주셔서 감사합니다 : )