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: 회원 프로필 설정 #146 #147

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

shinyj0
Copy link
Member

@shinyj0 shinyj0 commented Nov 17, 2024

개요

s3 버킷을 이용하여 프로필 설정

작업사항

#146

@shinyj0 shinyj0 linked an issue Nov 17, 2024 that may be closed by this pull request
1 task
Copy link
Member

@yooooonshine yooooonshine left a comment

Choose a reason for hiding this comment

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

구현은 잘 된 거 같습니다! 고생하셨어요.
코드 리팩토링 하시면 테스트 해보시고 테스트 결과도 pr에 함께 첨부해주시면 좋겠습니다!

@RequiredArgsConstructor
public class AwsS3Service {
private final AmazonS3 amazonS3;
@Value("${cloud.aws.s3.bucket}")
Copy link
Member

Choose a reason for hiding this comment

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

이 부분 Value 애노테이션으로 가져오지 말고 Properties를 통해 가져오면 좋을 거 같습니다!


@Configuration
public class S3Config {
@Value("${cloud.aws.credentials.access-key}")
Copy link
Member

Choose a reason for hiding this comment

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

이 부분도 Properties를 이용하면 좋을 거 같습니다

@@ -24,13 +30,30 @@ public class MemberService {
private final PasswordEncoderUtil passwordEncoderUtil;
private final RefreshTokenService refreshTokenService;
private final LogoutService logoutService;
private final AwsS3Service awsS3Service;

@Value("${default.profile.image.url}") // application.properties 또는 application.yml에서 기본 이미지 URL 설정
Copy link
Member

Choose a reason for hiding this comment

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

마찬가지입니다!

@@ -114,5 +120,69 @@ public ResponseEntity<DataResponse<Void>> loginMember(
// 이 메소드는 실제로 실행되지 않습니다. 문서용도로만 사용됩니다.
return ResponseEntity.ok(DataResponse.ok());
}

@PostMapping(value = "/profile-image", consumes = MediaType.MULTIPART_FORM_DATA_VALUE)
Copy link
Member

Choose a reason for hiding this comment

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

직접 Image를 받아서 처리하시는군요
보통 image를 백엔드에서 받는 건 부담이 커서, presigned url을 통해 프론트엔드 단에서 s3에 저장하는데
직접 이미지를 받으신 이유가 있으실까요?

} catch (Exception e) {
throw CustomException.from(MEMBER_FILE_UPLOAD_ERROR);
}
Member member = Member.from(request.getEmail(), pw, request.getName(), Role.USER, profileImageUrl);
Copy link
Member

Choose a reason for hiding this comment

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

이미지를 저장하는 로직하고 회원가입하는 로직은 분리되는 게 바람직해보이는데 어떻게 생각하시나요?

  1. 이미지 저장 실패 시 회원가입 정보는 유효한 경우
  2. 이미지 저장은 성공했지만 회원가입은 실패한 경우

위 케이스에 대한 고려가 안 된 거 같습니다.

제 생각에는 이미지 저장보다 회원가입이 우선이라 생각되어
일단 회원가입 한 후에 이미지 저장이 진행되어야 한다고 생각합니다.

또한 외부 api를 통해 저장을 하는 경우는 성능에 지장이 갈수 있습니다. 테스트 결과가 어떻게 됐을지는 모르겠지만
평균 api 응답시간이 0.5초 이내인 점을 고려했을 때 이 api는 넘을 거 같아요
s3 저장로직은 비동기 처리를 하는 것도 좋아보여요

정리하면

  1. 회원가입과 이미지 저장은 분리한다.
  2. 이미지 저장은 비동기화를 통해 성능을 개선한다.

amazonS3.deleteObject(bucket, fileName);
System.out.println(bucket);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

오 이미지 저장은 처음해본다고 들었는데 잘 구현하셨군요

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

✨ s3이용하여 프로필 이미지 설정
2 participants