-
Notifications
You must be signed in to change notification settings - Fork 0
6차 세미나 클론 코딩 과제 (#14) #16
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
base: main
Are you sure you want to change the base?
Changes from all commits
c56cd6b
0493cab
4809b8a
e85325a
bd9d74c
c048c8c
4109028
f04420a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ | |
| import org.sopt.carrotMarket.service.ItemService; | ||
| import org.sopt.carrotMarket.service.dto.GetAllItemsInfoResponseDTO; | ||
| import org.sopt.carrotMarket.service.dto.RegisterItemDTO; | ||
| import org.sopt.carrotMarket.service.dto.RegisterItemResponseDTO; | ||
| import org.springframework.http.ResponseEntity; | ||
| import org.springframework.web.bind.annotation.*; | ||
|
|
||
|
|
@@ -21,9 +22,10 @@ public class ItemController { | |
|
|
||
| @PostMapping("/item") | ||
| public ResponseEntity<BaseResponse<?>> registerItem(@RequestHeader final Long memberId, | ||
| @RequestBody final RegisterItemDTO registerItemDTO) { | ||
| itemService.registerItem(memberId, registerItemDTO); | ||
| return ApiResponseUtil.success(SuccessMessage.ITEM_REGISTER_SUCCESS); | ||
| @ModelAttribute final RegisterItemDTO registerItemDTO) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. comment: 과제를 살펴보니 모든 분들이 ModelAttibute 를 사용하셨는데 사용하신 이유가 뭘까요? 만약 그 이유가 다른 어떤 수단들 보다 좋다고 생각해서 사용하시는 것인지, 아니면 단순하게 따라하신 것인지 궁금합니다. 만약 전자와 같이 설득할 수 있는 이유가 없이 단순하게 사용만 하신 것이라면 왜 이것을 사용해야만 했는지 그 이유를 찾아보시는 것 역시 좋을 것 같아요. 과제는 "클론"코딩이기 때문에 따라했다 역시 사용할 줄 안다는 측면에서는 배움이 될 수 있어요.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 세미나에서 사용해서 그냥 사용했는데, 지금 바로 공부해보겠습니다! |
||
| RegisterItemResponseDTO registerItemResponseDTO = itemService.registerItem(memberId, registerItemDTO); | ||
|
|
||
| return ApiResponseUtil.success(SuccessMessage.ITEM_REGISTER_SUCCESS, registerItemResponseDTO); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. p1: ApiResponseUtil 를 별도로 작성하신 이유가 있을까요? 현재 ApiResponseUtil 에 작성해주신 내용 모두 ResponseEntity 객체 내부에 static method 로 보다 풍부하게 지원해주는 함수가 있습니다! 확인해보시면 좋을 것 같아요!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 제 현재 생각으로는 success와 failure를 따로 커스텀해서 보낼 수 있어서 위와 같이 했습니다! |
||
| } | ||
|
|
||
| //memberID에 해당되는 모든 물건 GET | ||
|
|
@@ -41,5 +43,11 @@ public ResponseEntity<BaseResponse<?>> getAllItemsByLocation(@RequestParam(value | |
| return ApiResponseUtil.success(SuccessMessage.GET_ITEMS_SUCCESS_BY_LOCATION, response); | ||
| } | ||
|
|
||
| @DeleteMapping("/item") | ||
| public ResponseEntity<BaseResponse<?>> deleteItem(@RequestHeader final Long itemId) { | ||
| itemService.deleteItem(itemId); | ||
| return ApiResponseUtil.success(SuccessMessage.ITEM_DELETE_SUCCESS); | ||
| } | ||
|
|
||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,8 @@ public class Item extends BaseTimeEntity { | |
| @ManyToOne(fetch = FetchType.LAZY) | ||
| private Member member; | ||
|
|
||
| private String imageUrl; | ||
|
|
||
| private String title; | ||
|
|
||
| private int price; | ||
|
|
@@ -41,8 +43,9 @@ public class Item extends BaseTimeEntity { | |
|
|
||
|
|
||
| @Builder //빌더패턴 | ||
| private Item(Member member, String title, int price, boolean isReceived, String detailInfo, Location hopeTradeSpot) { | ||
| private Item(Member member, String imageUrl, String title, int price, boolean isReceived, String detailInfo, Location hopeTradeSpot) { | ||
| this.member = member; | ||
| this.imageUrl = imageUrl; | ||
| this.title = title; | ||
| this.price = price; | ||
| this.isReceived = isReceived; | ||
|
|
@@ -51,9 +54,10 @@ private Item(Member member, String title, int price, boolean isReceived, String | |
| } | ||
|
|
||
| //정적팩토리메서드(빌더패턴이용) | ||
| public static Item register(Member member, String title, int price, boolean isReceived, String detailInfo, Location hopeTradeSpot) { | ||
| public static Item register(Member member, String imageUrl, String title, int price, boolean isReceived, String detailInfo, Location hopeTradeSpot) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
| return Item.builder() | ||
| .member(member) | ||
| .imageUrl(imageUrl) | ||
| .title(title) | ||
| .price(price) | ||
| .isReceived(isReceived) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| package org.sopt.carrotMarket.external; | ||
|
|
||
| import org.springframework.beans.factory.annotation.Value; | ||
| import org.springframework.context.annotation.Bean; | ||
| import org.springframework.context.annotation.Configuration; | ||
| import software.amazon.awssdk.auth.credentials.SystemPropertyCredentialsProvider; | ||
| import software.amazon.awssdk.regions.Region; | ||
| import software.amazon.awssdk.services.s3.S3Client; | ||
|
|
||
| @Configuration | ||
| public class AwsConfig { | ||
|
|
||
| private static final String AWS_ACCESS_KEY_ID = "aws.accessKeyId"; | ||
| private static final String AWS_SECRET_ACCESS_KEY = "aws.secretAccessKey"; | ||
|
|
||
| private final String accessKey; | ||
| private final String secretKey; | ||
| private final String regionString; | ||
|
|
||
| public AwsConfig(@Value("${aws-property.access-key}") final String accessKey, | ||
| @Value("${aws-property.secret-key}") final String secretKey, | ||
| @Value("${aws-property.aws-region}") final String regionString) { | ||
| this.accessKey = accessKey; | ||
| this.secretKey = secretKey; | ||
| this.regionString = regionString; | ||
| } | ||
|
|
||
|
|
||
| @Bean | ||
| public SystemPropertyCredentialsProvider systemPropertyCredentialsProvider() { | ||
| System.setProperty(AWS_ACCESS_KEY_ID, accessKey); | ||
| System.setProperty(AWS_SECRET_ACCESS_KEY, secretKey); | ||
| return SystemPropertyCredentialsProvider.create(); | ||
| } | ||
|
|
||
| @Bean | ||
| public Region getRegion() { | ||
| return Region.of(regionString); | ||
| } | ||
|
|
||
| @Bean | ||
| public S3Client getS3Client() { | ||
| return S3Client.builder() | ||
| .region(getRegion()) | ||
| .credentialsProvider(systemPropertyCredentialsProvider()) | ||
| .build(); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| package org.sopt.carrotMarket.external; | ||
|
|
||
| import org.springframework.beans.factory.annotation.Value; | ||
| import org.springframework.stereotype.Component; | ||
| import org.springframework.web.multipart.MultipartFile; | ||
| import software.amazon.awssdk.core.sync.RequestBody; | ||
| import software.amazon.awssdk.services.s3.S3Client; | ||
| import software.amazon.awssdk.services.s3.model.DeleteObjectRequest; | ||
| import software.amazon.awssdk.services.s3.model.PutObjectRequest; | ||
|
|
||
| import java.io.IOException; | ||
| import java.util.Arrays; | ||
| import java.util.List; | ||
| import java.util.UUID; | ||
|
|
||
| @Component | ||
| public class S3Service { | ||
|
|
||
| private final String bucketName; | ||
| private final AwsConfig awsConfig; | ||
| private static final List<String> IMAGE_EXTENSIONS = Arrays.asList("image/jpeg", "image/png", "image/jpg", "image/webp"); | ||
|
|
||
|
|
||
| public S3Service(@Value("${aws-property.s3-bucket-name}") final String bucketName, AwsConfig awsConfig) { | ||
| this.bucketName = bucketName; | ||
| this.awsConfig = awsConfig; | ||
| } | ||
|
|
||
|
|
||
| public String uploadImage(String directoryPath, MultipartFile image) throws IOException { | ||
| final String key = directoryPath + generateImageFileName(); | ||
| final S3Client s3Client = awsConfig.getS3Client(); | ||
|
|
||
| validateExtension(image); | ||
| validateFileSize(image); | ||
|
|
||
| PutObjectRequest request = PutObjectRequest.builder() | ||
| .bucket(bucketName) | ||
| .key(key) | ||
| .contentType(image.getContentType()) | ||
| .contentDisposition("inline") | ||
| .build(); | ||
|
|
||
| RequestBody requestBody = RequestBody.fromBytes(image.getBytes()); | ||
| s3Client.putObject(request, requestBody); | ||
| return key; | ||
| } | ||
|
|
||
| public void deleteImage(String key) throws IOException { | ||
| final S3Client s3Client = awsConfig.getS3Client(); | ||
|
|
||
| s3Client.deleteObject((DeleteObjectRequest.Builder builder) -> | ||
| builder.bucket(bucketName) | ||
| .key(key) | ||
| .build() | ||
| ); | ||
| } | ||
|
|
||
|
|
||
| private String generateImageFileName() { | ||
| return UUID.randomUUID() + ".jpg"; | ||
| } | ||
|
Comment on lines
+60
to
+62
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. p0: uploadImage 에서 validate 를 통해 현재 image 의 확장자를 "image/jpeg", "image/png", "image/jpg", "image/webp"로 받고있는것과 달리 이미지 파일에 .jpg 로 확장자로 모두 붙이는 이유가 있을까요? 반드시 jpg 만 들어오는 서비스라면 문제가 되지 않겠지만 그렇지 않는경우 문제가 발생할 수 있어보입니다.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 오 .jpg를 지워겠네요!! 그렇지 않는 경우를 생각하지 못했던 것 같습니다 |
||
|
|
||
|
|
||
| private void validateExtension(MultipartFile image) { | ||
| String contentType = image.getContentType(); | ||
| if (!IMAGE_EXTENSIONS.contains(contentType)) { | ||
| throw new RuntimeException("이미지 확장자는 jpg, png, webp만 가능합니다."); | ||
| } | ||
| } | ||
|
|
||
| private static final Long MAX_FILE_SIZE = 5 * 1024 * 1024L; | ||
|
|
||
| private void validateFileSize(MultipartFile image) { | ||
| if (image.getSize() > MAX_FILE_SIZE) { | ||
| throw new RuntimeException("이미지 사이즈는 5MB를 넘을 수 없습니다."); | ||
| } | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,14 +7,17 @@ | |
| import org.sopt.carrotMarket.domain.Item; | ||
| import org.sopt.carrotMarket.domain.Member; | ||
| import org.sopt.carrotMarket.exception.NotFoundException; | ||
| import org.sopt.carrotMarket.external.S3Service; | ||
| import org.sopt.carrotMarket.repository.ItemLikesRepository; | ||
| import org.sopt.carrotMarket.repository.ItemRepository; | ||
| import org.sopt.carrotMarket.repository.MemberRepository; | ||
| import org.sopt.carrotMarket.service.dto.GetAllItemsInfoResponseDTO; | ||
| import org.sopt.carrotMarket.service.dto.RegisterItemDTO; | ||
| import org.sopt.carrotMarket.service.dto.RegisterItemResponseDTO; | ||
| import org.springframework.stereotype.Service; | ||
| import org.springframework.transaction.annotation.Transactional; | ||
|
|
||
| import java.io.IOException; | ||
| import java.util.List; | ||
|
|
||
| @Service | ||
|
|
@@ -27,22 +30,31 @@ public class ItemService { | |
| private final ItemLikesRepository itemLikesRepository; | ||
| private final ItemLikesService itemLikesService; | ||
|
|
||
| private final S3Service s3Service; | ||
| private static final String BLOG_S3_UPLOAD_FOLER = "blog/"; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. p5: BLOG_S3_UPLOAD_FOLER에 해당하는 폴더 정보를 ItemService 가 알고 처리하기보다 폴더나 파일 처리 모든 부분의 책임을 S3Service 에 위임하는것은 어떨까요? 만약 ItemService 가 아닌 다른 곳에서도 해당 주소에 이미지나 파일을 올려야한다면 해당 Service에도 같은 변수를 작성해야할 수 있어요. |
||
|
|
||
| @Transactional | ||
| public void registerItem(final Long memberId, final RegisterItemDTO registerItemDTO) { | ||
| public RegisterItemResponseDTO registerItem(final Long memberId, final RegisterItemDTO registerItemDTO) { | ||
|
|
||
| Member member = findMemberById(memberId); | ||
|
|
||
| Location.checkIsLocationEnumHasString((registerItemDTO.hopeTradeSpot())); | ||
|
|
||
| try { | ||
| Item item = Item.register( | ||
| member, | ||
| s3Service.uploadImage(BLOG_S3_UPLOAD_FOLER, registerItemDTO.image()), | ||
| registerItemDTO.title(), | ||
| registerItemDTO.price(), | ||
| registerItemDTO.isReceived(), | ||
| registerItemDTO.detailInfo(), | ||
| Location.valueOf(registerItemDTO.hopeTradeSpot())); | ||
|
|
||
| itemRepository.save(item); | ||
| return RegisterItemResponseDTO.of(item.getId()); | ||
|
|
||
| } catch (RuntimeException | IOException e) { | ||
| throw new RuntimeException(e.getMessage()); | ||
| } | ||
|
Comment on lines
+55
to
+57
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. p5: 발생하는 Exception 을 모두 하나로 처리하는 이유가 있나요? 현재 RuntimeException 과 IOException이 모두 사실상 같은 RuntimeException 으로 처리되게 되는것 같아요. 예외라는 것은 무엇일까요? 에러와는 어떤 차이가 있을까요? 자바는 왜 Exception 을 여러가지로 나누고 상세하게 나누었을지 고민하면 우리가 예외를 처리할 때 역시 어떻게 해야하는지 보일것 같아요. 이런 이유를 알고 더 나아간다면 내 서비스 도메인에 맞는 로직을 처리하기 위한 Exception 을 Custom 하는 것 역시도 생각해볼 수 있을 거고 추후 도입하실지 모르겠지만 ControllerAdvice 등 보다 유연하게 에러를 처리하여 클라이언트에게 표시하는 방법도 고민해보면 좋을 것 같아요! 만약 위 이야기에서 자신만의 답을 찾으셨다면 또 다른 부분에서 고민해보실 수 있는 자료도 첨부해봅니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 추가적으로 현재 GlobalExceptionHandler 라는 CotrollerAdvice 를 사용하고 계신데 이와 같이 처리한다면 추후 실제로 의도하지 않은 예외가 발생하는 것에 처리를 할 수 없고, 예외 상황 추적에서도 문제가 발생할 수 있습니다 |
||
| } | ||
|
|
||
| public List<GetAllItemsInfoResponseDTO> getAllItemsByMemberId(final Long memberId) { | ||
|
|
@@ -83,4 +95,22 @@ public Member findMemberById(final Long memberId) { | |
| return memberRepository.findById(memberId).orElseThrow( | ||
| () -> new NotFoundException(ErrorMessage.MEMBER_NOT_FOUND)); | ||
| } | ||
|
|
||
| @Transactional | ||
| public void deleteItem(final Long itemId) { | ||
|
|
||
| Item item = findItemById(itemId); | ||
| try { | ||
| s3Service.deleteImage(item.getImageUrl()); | ||
| } catch (IOException e) { | ||
| throw new RuntimeException("이미지 삭제 오류 발생", e); | ||
| } | ||
| itemRepository.deleteById(itemId); | ||
| } | ||
|
|
||
| public Item findItemById(final Long itemId) { | ||
| return itemRepository.findById(itemId).orElseThrow( | ||
| () -> new NotFoundException(ErrorMessage.ITEM_NOT_FOUND) | ||
| ); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ | |
| public record GetAllItemsInfoResponseDTO( | ||
| Long itemId, | ||
| String title, | ||
| String imageUrl, | ||
| int price, | ||
| boolean isReceived, | ||
| String detailInfo, | ||
|
|
@@ -24,14 +25,15 @@ public record GetAllItemsInfoResponseDTO( | |
| //지역별로 받아올 때는, 내가 좋아요를 누른지가 필요하지 않아서 builder를 이용했다. | ||
|
|
||
| @Builder //빌더패턴 | ||
| public GetAllItemsInfoResponseDTO(Long itemId, String title, int price, | ||
| public GetAllItemsInfoResponseDTO(Long itemId, String title, String imageUrl,int price, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. p5: 오히려 entity 보다 지금같은 경우에서 정적 팩토리 메서드를 사용하는것이 더 좋다고 생각합니다. 해당 DTO 는 왜 필요할까요? 우리는 왜 Entity 자체를 Response 로 사용하지 않을까요? |
||
| boolean isReceived, String detailInfo, | ||
| boolean isLikedByMember, int likesCount, | ||
| String hopeTradeSpot, int textedCount, | ||
| String createdTime | ||
| ) { | ||
| this.itemId = itemId; | ||
| this.title = title; | ||
| this.imageUrl = imageUrl; | ||
| this.price = price; | ||
| this.isReceived = isReceived; | ||
| this.detailInfo = detailInfo; | ||
|
|
@@ -46,6 +48,7 @@ public static GetAllItemsInfoResponseDTO of(Item item, boolean isLiked, String c | |
| return GetAllItemsInfoResponseDTO.builder() | ||
| .itemId(item.getId()) | ||
| .title(item.getTitle()) | ||
| .imageUrl(item.getImageUrl()) | ||
| .price(item.getPrice()) | ||
| .isReceived(item.isReceived()) | ||
| .detailInfo(item.getDetailInfo()) | ||
|
|
||
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.
p5: 큰 문제는 없지만 혹시 따로 value 만 받아서 전달하시는 이유가 있을까요?
결국 해당 함수에서 그대로 가져와서 사용할 뿐이라면 HttpStatus.NOT_FOUND 여도 동일하게 작동하는데 별도로 value 만 받아오는 의도가 있으셨나요?