-
Notifications
You must be signed in to change notification settings - Fork 2
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
백엔드 서버 Pre-Signed URL 반환하는 API 구현 / 프론트에서 해당 API 를 이용해서 이미지 upload 함수 구현 #106
Conversation
backend/src/app.module.ts
Outdated
@@ -22,6 +23,7 @@ import { UserModule } from './user/user.module'; | |||
PlaceModule, | |||
MapModule, | |||
CourseModule, | |||
ImageModule, |
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.
p3: Image
말고도 다른 파일을 업로드하여 이용가능할 수 있도록 확장한다면 일반 파일에도 쓸 수 있을 것 같은데 현재는 Image
에만 쓸 수 있는 것 처럼 보입니다! 확장 가능성을 생각해보는 건 어떨까요?
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.
오... 그 생각을 못했네요.. FileModule
같이 바꿔보겠습니다.
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.
파일 모듈에서 이미지에 한정적인 함수들도 있을 거고 다양하게 쓸 수 있는 함수들도 있을 테니깐 구조 설계에 대해 고민 해보면 좋을 것 같네요!
frontend/src/api/image/index.ts
Outdated
export const uploadImage = async ( | ||
file: File, | ||
dirName: string, | ||
extension: string, |
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.
p2: 이미지로 제한한다면 extension
를 type
분리해서 들어올 수 있는 것들만 처리해두면 좋을 것 같아요
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.
아하 넵! 좋습니다. 확장자에 따라 이미지인지 확인하는 로직 추가할게요.
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.
extension: string, | |
extension: ImageExtention, | |
type ImageExtention |
제가 생각해본건 이런 식?
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.
수고하셨습니다~
구현한 김에 유량제어 관련해서도 고민해봐 주세요~
그리고 CORS 해결 방법 찾으셨나요?
frontend/src/api/image/index.ts
Outdated
export const uploadImage = async ( | ||
file: File, | ||
dirName: string, | ||
extension: string, |
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.
p2. 파일에서 제공하는 파일명에서 확장자를 추출하면 더 편할 것 같아요.
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.
오 그러네요 좋은 리뷰감사합니다.
frontend/src/api/image/index.ts
Outdated
|
||
const generatePreSignedUrl = async (dirName: string, extension: string) => { | ||
const { data } = await axiosInstance.get<PreSignedURLResponse>( | ||
END_POINTS.PRE_SIGNED_URL(dirName, extension), |
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.
p1. 지금 사용중인건 pre signed post 라고 합니다.
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.
아하 넵!
|
||
@Get('/preSignedURL/:dirName/:extension') | ||
@UseGuards(JwtAuthGuard) | ||
async getPreSignedURL( |
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.
p1. 지금 사용중인건 pre signed post 라고 합니다.
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.
q. 사용자마다 api 호출 횟수 제한을 두려면 어떻게 해야 할까요?
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.
p2. pre-signed URL/POST
를 생성하는 과정은
자원을 미리 준비하는 작업이므로 POST가 더 적절하지 않을까요?
어떻게 생각하시나요?
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.
저는 이미지를 직접적으로 올리는 곳엔 POST
라고 생각하고 그냥 서버로부터 URL 을 가져온다고 생각해서 GET
으로 생각해서 구현하긴하였는데 좀 더 생각해봐야겠네요..
이게 URL 을 생성 하고 가져온다 또는 그냥 단순하게 서버로부터 URL을 가져온다 중에 저는 단순하게 서버로부터 URL 을 가져온다고 생각해서 GET 으로 설정하긴 했습니다.
그런데 민서님의 말이 맞는 거 같네요.. 생성하고 가져오는 부분에서 생각한다면..
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.
저는 이미지를 직접적으로 올리는 곳엔
POST
라고 생각하고 그냥 서버로부터 URL 을 가져온다고 생각해서GET
으로 생각해서 구현하긴하였는데 좀 더 생각해봐야겠네요.. 이게 URL 을 생성 하고 가져온다 또는 그냥 단순하게 서버로부터 URL을 가져온다 중에 저는 단순하게 서버로부터 URL 을 가져온다고 생각해서 GET 으로 설정하긴 했습니다. 그런데 민서님의 말이 맞는 거 같네요.. 생성하고 가져오는 부분에서 생각한다면..
->
저는 이미지를 직접적으로 올리는 곳엔POST
라고 생각하고,
단순히 서버로부터 URL 을 가져온다고 생각해서GET
으로 구현 하였는데 조금 더 생각해봐야겠네요..URL 을 생성 하고 가져온다 또는 그냥 단순하게 서버로부터 URL을 가져온다 중에
저는 전자에 가깝다고 생각해, GET 으로 결정했습니다.그런데 민서님의 말이 맞는 거 같네요.. 생성하고 가져오는 부분에서 생각한다면..
가독성을 조금만 신경써 주세요 ㅎㅎ;
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.
넵 ㅜㅜ,,
backend/src/image/image.service.ts
Outdated
|
||
async generatePreSignedURL(dirName: string, extension: string) { | ||
return fetch( | ||
'https://o7bfsxygtk.apigw.ntruss.com/ogil/v1/BEwksUqeJl?blocking=true&result=true', |
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.
p2. 변수로 추출하면 좋을것 같아요.
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.
아예 환경변수로 빼는게 좋을 것 같은데요?
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.
아... .env
에 추가하겠습니다!
import { Throttle } from '@nestjs/throttler'; | ||
|
||
@Controller('storage') | ||
export class StorageController { |
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.
storage! 괜찮네요 ㅎㅎ
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.
감사합니다!
@Throttle({ default: { limit: 10, ttl: 60000 } }) | ||
@Post('/preSignedPost') | ||
@UseGuards(JwtAuthGuard) | ||
async getPreSignedPostUrl( |
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.
p3. url을 주긴 하는데 또 url만 주는건 아니라..
애매한 것 같아요
그래도 포스트에 대한 설명이 담겼으니 일단 오케이!
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.
엥 다 지운줄 알았는데... 수정하고 다시 push 할게요
); | ||
}; | ||
|
||
export const uploadImage = async (file: File, dirName: string) => { |
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.
q. 여긴 다 function 키워드 대신 익명함수네요?
이유가 무엇인가요?
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.
api
의 다른 디렉터리에 있는 index.ts
모두 익명함수여서 일관성 유지 때문에 익명함수로 작성했습니다.
|
||
@IsString() | ||
@IsNotEmpty() | ||
extension: string; |
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.
p3. 여기도 검증 추가할 수 있겠네요.
프론트엔드에 작성하신 상수 재활용해서 타입으로 가능할까요?
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.
오 좋습니다..
58084c2
to
1a66fac
Compare
@@ -7,5 +8,6 @@ export class PreSignedPostRequest { | |||
|
|||
@IsString() | |||
@IsNotEmpty() | |||
@IsImageFile() |
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.
이거 이렇게도 가능해요!
const ALLOWED_FRUITS = ['apple', 'banana', 'cherry'] as const;
export class CreateFruitDto {
@IsIn(ALLOWED_FRUITS, { message: 'Invalid fruit type' })
fruit: string;
}
사실 이걸 생각하고 리뷰 남긴건데 똑같이 동작하니 괜찮습니다 ㅎㅎ 학습했다 치죠!
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.
아하... 그러게요 엄청 어려웠네요..
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 data; | ||
}) | ||
.catch((err) => { | ||
throw new Error(err); |
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.
p1: CustomException
으로 만들면 좋을 것 같아요
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.
넵 시도해볼게요!
3b221b3
to
2bcde7d
Compare
📄 Summary
Pre-Signed URL
반환하는 컨트롤러, 서비스 구현Pre-Signed URL
요청하는 함수 구현Pre-Signed URL
로 이미지 업로드하고 업로드된 URL 을 리턴하는 함수 구현map.service
의addPlace
메소드 응답 변경savedPlaceId
에서placeId
로 변경🙋🏻 More
File
,dirName
,extension
을 파라미터로 받아서Pre-Signed URL
을 요청하고 해당 URL 로 이미지 업로드 하고 이미지 업로드 된 URL 을 반환하는 메소드🕰️ Actual Time of Completion
6
close #88