-
Notifications
You must be signed in to change notification settings - Fork 18
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
[김민선] sprint10 #31
base: express-김민선
Are you sure you want to change the base?
The head ref may contain hidden characters: "express-\uAE40\uBBFC\uC120-sprint10"
[김민선] sprint10 #31
Conversation
@alscksdlek 컴파일된건 왜올리셨나요? |
name String | ||
description String | ||
price Int | ||
tags String[] @default([]) |
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.
@alscksdlek 왜 배열로 했나요?
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.
tag들이 여러개가 있고 문자열이기 때문에 배열을 사용하면 tag들을 묶기 편할 것 같아서 선택했습니다!
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.
@alscksdlek db 성능과 연관지어서 생각해보시겠어요?
그리고 tag는 재사용이 될 수 있는 요소인데 배열로 묶어버리면 그건 string[] 처럼 작동해서 하드코딩으로 들어갈텐데 괜찮을까요?
.gitignore 파일에 dist 폴더 넣는 것을 깜빡하였습니다 삭제하였습니다! |
…2-Sprint-Mission-BE into express-김민선-sprint10
"main": "index.js", | ||
"scripts": { | ||
"dev": "tsx watch src/app.ts", | ||
"build": "tsc --outDir dist", |
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.
@alscksdlek 이건 tsconfig에서 지정할 수 있는데 여기다가 쓰신 이유가 있을까요?
await prisma.like.deleteMany(); | ||
await prisma.comment.deleteMany(); | ||
await prisma.article.deleteMany(); | ||
await prisma.product.deleteMany(); | ||
await prisma.user.deleteMany(); |
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.
@alscksdlek 이건 Promise.all 로 묶으면 안되나요?
await prisma.user.createMany({ | ||
data: USERS, | ||
skipDuplicates: true, | ||
}); | ||
await prisma.product.createMany({ | ||
data: PRODUCTS, | ||
skipDuplicates: true, | ||
}); | ||
await prisma.article.createMany({ | ||
data: ARTICLES, | ||
skipDuplicates: true, | ||
}); | ||
await prisma.comment.createMany({ | ||
data: COMMENTS, | ||
skipDuplicates: true, | ||
}); | ||
await prisma.like.createMany({ | ||
data: LIKES, | ||
skipDuplicates: 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.
@alscksdlek 이것도 promise.all 로 묶으면안되나요?
.catch(async (e) => { | ||
console.error(e); | ||
await prisma.$disconnect(); | ||
process.exit(1); |
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.
1 주는게 무슨 의미인지 아시나요?
import { ProductRepository } from "../repositories/productRepository"; | ||
import { PrismaClient } from "@prisma/client"; | ||
|
||
const prisma = new PrismaClient(); |
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.
@alscksdlek prismaClient를 여러곳에서 생성하면 connection이 여러개 맺어질텐데 확인해보셨나요?
singleton 구조 내부에서는 connection 은 하나로만 해도 충분할거같아요
실제로 connection이 너무 많으면 부하가 심하거나 예상치못한문제가 생길수도있어요
export class ProductController { | ||
constructor(private service: ProductService) {} | ||
|
||
getProductById = asyncErrorHandler( |
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.
@alscksdlek controller의 메소드는 그 자체의 내용만 필요해요
async error handler가 controller에 속한 내용인지, util or middleware 성격인지 생각해보세요
|
||
getProductById = asyncErrorHandler( | ||
async (req: Request, res: Response): Promise<any> => { | ||
const { id } = req.params; |
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.
@alscksdlek id 유효성검사해야돼요
id 가 없거나 비정상적이면 이후 로직은 전부 실패할텐데, 실패할 로직을 실행할 이유가 없으니까요
const { id } = req.params; | ||
const product = await this.service.getProductById(Number(id)); | ||
if (!product) { | ||
return res.status(404).json({ message: "Not Found" }); |
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.
@alscksdlek product의 유효성 검사를 한다면 service 에서 해야죠. controller는 http 요청만 처리하는곳이고 리소스에 대한 관리는 서비스에서 해야돼요
그리고 404를 반환하는게아니라 에러를 던져야돼요 그래야 전역에러핸들러에서 받으니까요
|
||
createUser = asyncErrorHandler( | ||
async (req: Request, res: Response): Promise<any> => { | ||
const user = await this.service.createUser(req.body); |
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.
@alscksdlek body를 통째로 보낸다면, body에 엉뚱한게 들어있어도 서비스단으로 들어가는건데 이거맞나요?
const user = await this.service.getUser(email, password); | ||
const accessToken = this.service.createToken(user.id); | ||
const refreshToken = this.service.createToken(user.id, "refresh"); | ||
await this.service.updateUser(user.id, { refreshToken }); |
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.
@alscksdlek 여러 서비스 메소드를 사용하지말고 loginUser api 요청에 대해 적절한 service 메소드를 하나만 쓰고, 그 메소드가 여러 private method 를 사용하도록 구성해보세요
import { AuthRequest } from "../types/requestType"; | ||
|
||
export const verifyAccessToken = expressjwt({ | ||
secret: process.env.JWT_SECRET as 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.
@alscksdlek 왜 이쪽에는 전부 타입단언을 사용하셨나요?
.get(productContainer.productController.getProductById) | ||
.patch( | ||
verifyAccessToken, | ||
verifyProductAuth(productContainer.productRepository), |
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.
@alscksdlek routes 는 컨트롤러의 상위 레이어인데 repository에 의존성을 가지면 안돼요
const error = new Error("product not found"); | ||
throw error; |
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.
바로 throw 할 . 수있는데 error 변수로 초기화한 이유가 있나요?
if (error.message === "User already exists") { | ||
return res.status(422).json({ | ||
message: error.message, | ||
email: req.body.email, | ||
}); | ||
} else if (error.message === "Unauthorized") { | ||
return res.status(401).json({ message: error.message }); | ||
} else if ( | ||
error instanceof Prisma.PrismaClientValidationError || | ||
error.name === "StructError" | ||
) { | ||
return res.status(400).json({ message: error.message }); | ||
} else if ( | ||
error instanceof Prisma.PrismaClientKnownRequestError && | ||
error.code === "P2025" | ||
) { | ||
return res.status(404).json({ message: error.message }); | ||
} else { | ||
return res.status(500).json({ message: error.message }); | ||
} |
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.
@alscksdlek 하드코딩 빼고 커스텀에러클래스 만들어서 사용해보세요
assert(req.body, CreateProduct); | ||
next(); | ||
} catch (error) { | ||
next(error); |
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.
@alscksdlek superstruct 에러를 에러 핸들러에서 조건걸어 처리해야하는 번거로움이 있다면 아예 글로벌 에러 핸들러로 보내기 전에 우리의 에러로 변환해서 내보내도 되지 않을까싶어요
createProduct = async (req: any): Promise<Product> => { | ||
return await this.prisma.product.create({ | ||
data: req, | ||
}); | ||
}; | ||
|
||
updateProduct = async (req: any, id: number): Promise<Product> => { | ||
return await this.prisma.product.update({ | ||
where: { id }, | ||
data: req, | ||
}); | ||
}; |
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.
@alscksdlek repository layer에서는 request 객체에 의존하면 안돼요
findByEmail = async (email: any): Promise<any> => { | ||
return await this.prisma.user.findUnique({ | ||
where: { email }, | ||
}); | ||
}; | ||
|
||
createUser = async (user: any): Promise<User> => { | ||
return await this.prisma.user.create({ | ||
data: user, | ||
}); | ||
}; |
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.
@alscksdlek 왜 any 죠?
userRouter.route("/").post(userContainer.userController.createUser); | ||
userRouter.route("/login").post(userContainer.userController.loginUser); | ||
userRouter | ||
.route("/token/refresh") | ||
.post( | ||
verifyRefreshToken, | ||
debugRefreshToken, | ||
userContainer.userController.refreshAccessToken | ||
); | ||
|
||
return userRouter; |
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.
@alscksdlek 이걸 controller가 아니라 route 클래스를 만들어서 작성한이유가있나요/
요구사항
기본
Github PR
상품 등록
상품 상세
좋아요 기능
에러 처리
라우트 중복 제거
인증
상품 기능 인가
게시글 기능 인가
댓글 기능 인가
심화
인증
OAuth를 활용한 인증
프로젝트 구조 변경
자유게시판 게시물 등록
주요 변경사항
스크린샷
멘토에게