Skip to content
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] 클론코딩 3번째! #19

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions carrot/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ dependencies {
annotationProcessor 'org.mapstruct:mapstruct-processor:1.6.0.Beta1'
developmentOnly 'org.springframework.boot:spring-boot-devtools'
runtimeOnly 'org.postgresql:postgresql'
implementation("software.amazon.awssdk:bom:2.21.0")
implementation("software.amazon.awssdk:s3:2.21.0")
Comment on lines +36 to +37

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

p5: implementation 으로 추가해주신 awsSDK BOM 은 어떤 일을 하는 라이브러리일까요?

해당 라이브러리를 왜 추가하는지 알아보시면 좋을 것 같아요.
예를 들어 지금 작성하신 파일에서 어떤 종속성에는 반드시 버전이 명시되어있고, 어떤 라이브러리는 버전이 명시되지 않아요.

특히 스프링은 어떤 이유에서 우리가 버전을 "직접" 작성하지 않아도 스프링의 라이브러리를 사용할 수 있을까요? 뭔가 라이브러리 버전을 한번에 관리해주는 친구가 있다면 그 친구의 버전으로 일괄성있게 종속성을 관리해주는게 좋지 않을까요?

참고로 해당 부분 공식 문서에는 다음과 같이 되어있습니다.

...
dependencies {    
    implementation(platform("software.amazon.awssdk:bom:2.21.1"))
    implementation("software.amazon.awssdk:s3")
 ...
}
...

}

tasks.named('test') {
Expand Down
48 changes: 48 additions & 0 deletions carrot/src/main/java/server/sopt/carrot/config/S3Config.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
package server.sopt.carrot.config;

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 S3Config {

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 S3Config(@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
Expand Up @@ -6,13 +6,11 @@
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.*;
import server.sopt.carrot.constant.Place;
import server.sopt.carrot.dto.product.ProductCreate;
import server.sopt.carrot.dto.product.ProductEdit;
import server.sopt.carrot.dto.product.ProductFindDto;
import server.sopt.carrot.dto.product.ProductGoodUpdateDto;
import server.sopt.carrot.dto.product.*;
import server.sopt.carrot.mapper.ProductMapper;
import server.sopt.carrot.service.ProductService;

import java.io.IOException;
import java.util.List;

@RestController
Expand All @@ -22,12 +20,20 @@ public class ProductController {
private final ProductService productService;
private final ProductMapper productMapper;
// 새로운 상품을 등록해줘!
@PostMapping("")
@PostMapping
public ProductCreate.Response createProduct(
@Valid @RequestBody ProductCreate.Request req

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

p0: RequestBody 를 사용하였기 때문에 전달 받은 데이터를 어떻게 처리 되는지 알아보면 좋을 것 같아요.

만약 이미지를 처리할 수 없다면 어떤 이유에서 이미지를 처리할 수 없는지, 혹은 있다면 어떻게 처리해야 되는지 알아보시면 좋을 것 같습니다.
물론 현재 변경사항이 없는 것을 미루어보아 단순하게 수정 사항 누락하신 것 같은 느낌도 들지만, 만약 바꿔야한다면 왜 그것을 사용해야하는지 예제는, 혹은 어떤 현업에서는 왜 그것을 사용했는지 아는 것 자체가 클론코딩을 통해 성장할 수 있는 가장 큰 지점이라고 생각하기 때문이에요

) {
) throws IOException {
return productService.createProductwithCustomerId(req);
}

@DeleteMapping("/{productId}")
public ResponseEntity<Void> deleteProduct(
@Valid @RequestHeader Long productId
) throws IOException {
Comment on lines +30 to +33

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

p1: path 는 사용되는데 값을 받지 않고 해더에서 productID 를 받는 이유가 있을까요?

RequestHeader 는 header 에서 값을 받아오기만 하면 된다면 불필요하게 URL 에서 어떤 값을 읽을 것이라고 표현해둘 필요가 있을까요?

추가적으로 Valid 어노테이션이 단일 값일때도 해당 어노테이션이 작동하나요? 어떤 상황에서 사용하는 친구일까요?

return productService.deleteProduct(productId);
}

// 특정 상품 조회
@PatchMapping("/{productId}")
public ProductFindDto productGoodUporDown(
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
package server.sopt.carrot.dto.product;

import jakarta.persistence.Column;
import jakarta.persistence.EnumType;
import jakarta.persistence.Enumerated;
import jakarta.validation.constraints.Min;
import jakarta.validation.constraints.NotNull;
import jakarta.validation.constraints.Size;
import lombok.*;
import org.springframework.web.multipart.MultipartFile;
import server.sopt.carrot.constant.CellingStatus;
import server.sopt.carrot.constant.Place;
import server.sopt.carrot.entity.Customer;
Expand All @@ -25,22 +25,23 @@ public static class Request {
private Integer price;

@NotNull
@Size(min=3,max=50,message= "Name is 3~50")
@Size(min = 3, max = 50, message = "Name is 3~50")
private String itemName;

@NotNull
private Long customerId;

// @NotNull
// private CellingStatus cellingStatus;

@NotNull
@Size(min=1,max=512,message= "Description is 3~50")
@Size(min = 1, max = 512, message = "Description is 3~50")
private String itemDescription;

@NotNull
@Enumerated(EnumType.STRING)
private Place place;

private MultipartFile image;

}
@Getter
@Builder
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
package server.sopt.carrot.dto.product;

26 changes: 26 additions & 0 deletions carrot/src/main/java/server/sopt/carrot/entity/Image.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package server.sopt.carrot.entity;

import jakarta.persistence.*;
import lombok.Builder;
import lombok.Getter;
import lombok.NoArgsConstructor;

@Entity
@Getter
@NoArgsConstructor
public class Image {
@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
private Long id;

private String imageUrl;

@OneToOne
Product product;

@Builder
public Image(String imageUrl, Product product) {
this.imageUrl = imageUrl;
this.product = product;
}
Comment on lines +21 to +25

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

p1: 현재 설계적으로 Image 는 생성될 때 반드시 Product 가 필요할 것 같습니다.
하지만 빌더 패턴의 사용으로 객체 생성에서 가독성이나 편리함은 챙길 수 있었지만, 오히려 필수 파라미터가 초기화 되는지를 보장하지 못하고 있다고 생각해요.

실제로 일부 값을 넣지 않고 build() 를 호출하더라도 개발자의 실수이므로..라고 생각할 수 있겠지만 이런 개발자의 실수 까지 고려하고 설계하여 적절하게 예외를 만들거나 강제하여 사용이 유용하게 만드는 것 역시 초기 코드를 설계하는 사람의 실력이라고 생각합니다.

어떻게 해야 필수적으로 받아야할 부분을 받을 수 있을까요?

+) 여담이지만 실제로 빌더 패턴이 안티패턴이냐 아니냐로 되게 주제가 많이 올라온 적이 있습니다. 이러한 부분도 함께 고민해보셔도 좋을 것 같아요! 제 생각이 궁금하시면 이 역시 저를 언급해서 질문해주세요ㅎㅎ

}
80 changes: 80 additions & 0 deletions carrot/src/main/java/server/sopt/carrot/external/S3Service.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
package server.sopt.carrot.external;

import org.springframework.beans.factory.annotation.Value;
import org.springframework.stereotype.Component;
import org.springframework.web.multipart.MultipartFile;
import server.sopt.carrot.config.S3Config;
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 S3Config 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, S3Config 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

Choose a reason for hiding this comment

The 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 만 들어오는 서비스라면 문제가 되지 않겠지만 그렇지 않는경우 문제가 발생할 수 있어보입니다.
단순하게 확장자를 바꾸어도 조회에 문제가 없는 경우 역시 존재하지만, 그렇지 않고 이미지가 보이지 않는 경우도 발생할 수 있을 것 같아요!



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를 넘을 수 없습니다.");
}
}

}
10 changes: 10 additions & 0 deletions carrot/src/main/java/server/sopt/carrot/repo/ImageRepository.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package server.sopt.carrot.repo;

import org.springframework.data.jpa.repository.JpaRepository;
import server.sopt.carrot.entity.Image;

import java.util.Optional;

public interface ImageRepository extends JpaRepository<Image, Long> {
Optional<Image> findByProduct_Id(Long proudctId);
}
Original file line number Diff line number Diff line change
@@ -1,19 +1,18 @@
package server.sopt.carrot.service;

import org.springframework.http.ResponseEntity;
import server.sopt.carrot.constant.Place;
import server.sopt.carrot.dto.product.ProductCreate;
import server.sopt.carrot.dto.product.ProductFindDto;
import server.sopt.carrot.dto.product.ProductEdit;
import server.sopt.carrot.dto.product.ProductGoodUpdateDto;
import server.sopt.carrot.dto.product.*;
import server.sopt.carrot.entity.Product;

import java.io.IOException;
import java.util.List;

public interface ProductService {

Product findProductById(Long id);
ProductFindDto getProductById(Long id);
ProductCreate.Response createProductwithCustomerId(ProductCreate.Request req);
ProductCreate.Response createProductwithCustomerId(ProductCreate.Request req) throws IOException;

List<ProductFindDto> getAllProductByCustomerId(Long customerId);

Expand All @@ -26,4 +25,6 @@ public interface ProductService {
List<ProductFindDto> getProductbyPlace(Place place);

ProductFindDto productGoodUporDown(Long productId, ProductGoodUpdateDto productGoodUpdateDto);

ResponseEntity<Void> deleteProduct(Long productId);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment: 현재 작성하신 부분이 아니지만, S3Service 는 별도의 Interface 를 구현하지 않고 ProductService 는 Interface 를 구현한 이유가 있을까요?

단순하게 필요하다 필요하지 않다를 넘어 어떤 의도를 가지고 만드신 것인지 궁금합니다.

또한 이러한 의도를 생각하면 저는 Impl 이라는 네이밍은 상당히 객체지향적이지 못하고, 확장성에 좋지 않은 영향을 주며 더 나아가 오히려 Service Interface 가 필요없다고 생각되는 이유가 될 수 있다고 생각합니다.

Interface 가 어떤 친구인지도 고민해보면 좋을 것 같아요!

Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,23 @@
import jakarta.transaction.Transactional;
import jakarta.validation.Valid;
import lombok.RequiredArgsConstructor;
import org.springframework.http.ResponseEntity;
import org.springframework.stereotype.Service;
import server.sopt.carrot.constant.CellingStatus;
import server.sopt.carrot.constant.Place;
import server.sopt.carrot.dto.product.ProductEdit;
import server.sopt.carrot.dto.product.ProductGoodUpdateDto;
import server.sopt.carrot.dto.product.*;
import server.sopt.carrot.entity.Customer;
import server.sopt.carrot.entity.Image;
import server.sopt.carrot.entity.Product;
import server.sopt.carrot.dto.product.ProductCreate;
import server.sopt.carrot.dto.product.ProductFindDto;
import server.sopt.carrot.error.ErrorMessage;
import server.sopt.carrot.exception.BusinessException;
import server.sopt.carrot.exception.NotFoundPlaceException;
import server.sopt.carrot.external.S3Service;
import server.sopt.carrot.mapper.ProductMapper;
import server.sopt.carrot.repo.ImageRepository;
import server.sopt.carrot.repo.ProductRepository;

import java.io.IOException;
import java.util.List;
import java.util.stream.Collectors;

Expand All @@ -29,7 +31,9 @@ public class ProductServiceImpl implements ProductService {
private final ProductRepository productRepository;
private final CustomerService customerService;
private final ProductMapper productMapper;

private final S3Service s3Service;
private final ImageRepository imageRepository;
private final String BLOG_S3_UPLOAD_FOLER = "/blog";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

p5: BLOG_S3_UPLOAD_FOLER에 해당하는 폴더 정보를 ItemService 가 알고 처리하기보다 폴더나 파일 처리 모든 부분의 책임을 S3Service 에 위임하는것은 어떨까요?

만약 ItemService 가 아닌 다른 곳에서도 해당 주소에 이미지나 파일을 올려야한다면 해당 Service에도 같은 변수를 작성해야할 수 있어요.
그런 경우를 대비한다면 올려야하는 경로를 관리하는 무언가가 있으면 더 객체지향적이지 않을까요?

@Override
public Product findProductById(Long id) {
return productRepository.findById(id)
Expand All @@ -43,8 +47,7 @@ public ProductFindDto getProductById(Long id) {
}
@Transactional
@Override
public ProductCreate.Response createProductwithCustomerId(ProductCreate.Request req)
{
public ProductCreate.Response createProductwithCustomerId(ProductCreate.Request req) throws IOException {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

p5: 해당 함수 안에서 Product 를 만들어 저장하는 일과 Image 를 저장하는일이 일어나고 있어요
추상화 단계에 따라서 각각의 일을 분리하여 가독성을 높여주는 것도 좋을 것같아요.

만약 상품에 보다 복잡하게 연관 관계가 발생하면(태그 등) 해당 함수는 매우 길어지고 가독성이 나빠질 수 있어요. 또한 분리한 함수를 어느정도 재사용 가능하게 추상화 단계에 따라 함수가 하나의 일을 하도록 구성하면 이후에 재사용성 역시 챙길 수 있을 것 같아요

Customer customer = customerService.getCustomerById(req.getCustomerId());
Product product = Product.builder()
.price(req.getPrice())
Expand All @@ -56,6 +59,11 @@ public ProductCreate.Response createProductwithCustomerId(ProductCreate.Request
.build();

productRepository.save(product);
String imageUrl = s3Service.uploadImage(BLOG_S3_UPLOAD_FOLER, req.getImage());
Image image = Image.builder()
.imageUrl(imageUrl)
.product(product).build();
imageRepository.save(image);
return ProductCreate.Response.fromEntity(product);
}

Expand Down Expand Up @@ -89,8 +97,6 @@ public ProductFindDto editProduct(
@Override
@Transactional
public ProductFindDto soldProduct(Product product) {
// Product product = productRepository.findById(productId)
// .orElseThrow(() -> new BusinessException(ErrorMessage.PRODUC)T_NOT_FOUND));
product.setCellingStatus(CellingStatus.SOLD);
return ProductFindDto.of(product);
}
Expand All @@ -112,4 +118,13 @@ public ProductFindDto productGoodUporDown(Long productId, ProductGoodUpdateDto p
product.setGood(product.getGood() + point);
return productMapper.toProductFindDto(product);
}

@Override
public ResponseEntity<Void> deleteProduct(Long productId) {
Image image= imageRepository.findByProduct_Id(productId).orElseThrow(
() -> new BusinessException(ErrorMessage.PRODUCT_NOT_FOUND)
);
imageRepository.delete(image);
return ResponseEntity.ok().build();
}
}