[김예인] 스프린트 미션 6#83
Hidden character warning
Conversation
devbini
left a comment
devbini
left a comment
There was a problem hiding this comment.
🎉 고생 많으셨습니다!
이번 PR은 기존 프론트엔드 구현 위에 Express API부터 Prisma ORM, PostgreSQL까지 범위가 꽤 넓었는데... 모델을 나누고 각각 연결한 흐름까지, 처음 백엔드 관계형 모델링을 적용하는 단계치고 굉장히 잘 하신 것 같아요.
특히 onDelete: Cascade라던가 HTTP 요청 샘플까지 준비한 점은 다음 작업자가 프로젝트를 이해하는 데 큰 도움이 됩니다. 유지보수적으로도 좋고, 테스트할 때도 꽤 도움이 될 것 같아요.
그래도 실제 서비스 코드 관점에서는 API의 존재뿐 아니라, 잘못된 입력을 어떻게 막을지라거나, 페이지네이션이 실제 데이터가 늘어났을 때 어떻게 안정적으로 동작할지 조금 더 확인이 필요한 부분이 있기는 했어요. 그럼에도 이 부분은 조금만 정리해도 완성도가 많이 올라갈 것 같습니다.
또 기억에 남는 게, 로딩/에러 상태를 FE에서 분리해서 다루려는 부분도 좋았습니다. 특히 finally에서 로딩 상태를 끄는 흐름은 좋은 습관으로 보여요. 별도 훅을 만들어 검증하는 구조도 좋았고, 어느 정도 캡슐화에 대해서도 이해하고 응용할 줄 아는 것 같습니다.
앞으로도 기대가 되는 코드입니다.
너무나 고생 많으셨고, 파이팅입니다! 👍
| tags, | ||
| }) | ||
|
|
||
| navigate(`/items/${product._id}`) |
There was a problem hiding this comment.
🚩 Check Point
상품 등록 후 이동하는 페이지의 매개변수를 product._id로 쓰고 있네요?
제가 알기론 Prisma 데이터 응답 식별자가 _id가 아니라 id를 쓸 텐데
등록이 성공해도 이동하는 nav 주소가 undefined가 되는 불상사가 있을 수도 있을 것 같아요.
실제로 구동 시 문제가 없다면 괜찮겠지만
혹시 모르니 리뷰 남겨둘게요.
| app.post('/products', async (req, res) => { | ||
| const product = await prisma.product.create({ | ||
| data: req.body, | ||
| }) | ||
|
|
||
| res.status(201).send(product) | ||
| }) |
There was a problem hiding this comment.
☕ Thinking
상품 등록에서 res body를 그대로 보내고 있네용
문법적으로 저장이 될 수는 있겠지만 (필드가 잘 되어있으니...)
실제로 API를 만든다고 했을 때는 각 데이터들이 요구사항 등에 맞는지 검증하는 단계를 추가하는 게 좋을 것 같아요.
FE쪽에서 넘어오는 데이터는 사용자가 마음만 먹으면 우회하거나 위조할 수 있고, 비정상적인 루트 (postman 등)에서도 실행할 수 있어서... 서버에서도 한번 더 체크 해 주는 습관이 있어도 좋지 않을까 싶습니다 🤗
| const offset = Number(req.query.offset) || 0 | ||
| const limit = Number(req.query.limit) || 10 |
There was a problem hiding this comment.
☕ Thinking
이것도 문제는 아니지만,
숫자를 바로 DB로 던지고 있는 것 같아요. 페이지네이션 관련 데이터를..
정상적인 상황에서는 문제가 없지만, 실제 환경에서는 어떤 문제가 나올지 몰라요.
예를 들어- limit 데이터가 음수로 들어간다거나, 비정상적으로 엄청 큰 데이터가 들어간다던가...
Math 라이브러리를 활용하면 아마 상/하 제한을 걸 수 있을텐데, 서버 리소스 보호를 생각하는 방법이 하나 더 있지 않을까 싶어 남겨둡니다ㅎㅎ
|
|
||
| const comments = await prisma.productComment.findMany({ | ||
| where: { productId }, | ||
| orderBy: { createdAt: 'desc' }, |
There was a problem hiding this comment.
☕ Thinking
음, 음.. 이건 제 개인적인 경험에 빗대어 말씀드리는 거긴 합니다만
"동일한 시간대"의 데이터를 읽어오는 경우 꼬이는 경우가 종종 있어요.
거의 있을 수는 없겠지만, 그래도 정렬 기준을 하나만 잡기 보다는 유니크한 컬럼을 추가하여 두 개를 써 보는 것도 좋은 방법이지 않을까 싶어요!
예를 들면.. id 같은 컬럼이요!
| res.sendStatus(404); | ||
| } else { | ||
| console.error(err); | ||
| res.status(500).send({ message: err.message }); |
There was a problem hiding this comment.
🚩 Check Point
음.. 이건 조금 위험해요.
요구사항에 500에 대한 응답 에러처리가 있어서 이렇게 해도 되는건지는 잘 모르겠지만..
500의 경우에는 DB 연결 내용이나 구현에 있어 치명적일 수 있는 서버 오류메시지가 사용자에게 그대로 전달되게 됩니다.
가능하면 message를 하드하게 별도로 만들거나, 에러핸들러를 따로 만들어서 관리하는 게 좋을 것 같아요.
블라인드 SQL 인젝션 등, 다양한 보안적 위험요소가 존재한답니다.
자세한 오류 로그는 서버 로그에만 남기는 게 좋아요! 🥹
| const handleAddTag = (e) => { | ||
| if (e.key !== 'Enter') return | ||
|
|
||
| e.preventDefault() | ||
|
|
||
| const newTag = tagInput.trim() | ||
| if (!newTag) return | ||
| if (tags.includes(newTag)) return | ||
|
|
||
| setTags([...tags, newTag]) | ||
| setTagInput('') | ||
| } |
There was a problem hiding this comment.
☕ Thinking
보니까- 실제 summit 폼 에서는 태그 저장 규칙이 있는 것 같아요. (5자 이내)
그런데, 실제 tag를 추가할 때는 그런 제약이 없는 것 같아 보입니다.
지금 구조라면 아주 긴 태그를 추가한 뒤- 등록 버튼을 눌렀을 때 에러가 나오는 느낌이라
UX적으로 조금 혼동이 있을 수 있어 보여요.
태그를 추가하는 시점에서 길이를 제한하는 등 제약사항을 넣어주면 좋지 않을까요?
| </div> | ||
|
|
||
| <a href="/login" className={styles.loginButton}> | ||
| <NavLink to="/login" className={styles.loginButton}> |
There was a problem hiding this comment.
🚩 Check Point
React App Route에서 /login이 등록되지 않아 있네요.
2 participants
기본 요구사항
중고마켓
공통
자유게시판
댓글