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] 커뮤니티 게시글 작성 API #347

Merged
merged 9 commits into from
Apr 8, 2024
Merged

[FEAT] 커뮤니티 게시글 작성 API #347

merged 9 commits into from
Apr 8, 2024

Conversation

jokj624
Copy link
Member

@jokj624 jokj624 commented Mar 31, 2024

작업 내용

  • 커뮤니티 게시글 작성 API 구현
    • 카테고리 id 에 일치하는 카테고리 없을 시 400 BAD REQUEST
    • community_post 생성 후 community_category_post 연결
  • express-validator 사용해 req validator 구현
    • 기존에 너무 컨트롤러 내부에서 if 문에 의존해 validation 하고 있어서 express-validator 를 사용해 middleware 단에서 검증하는 부분 추가해봤습니다. 관련해서 많은 의견 부탁드려요

@jokj624 jokj624 added 🚀 enhancement New feature or request 🥱 jobchae labels Mar 31, 2024
@jokj624 jokj624 self-assigned this Mar 31, 2024
Comment on lines 3 to 12
const createCommunityPostValidator = [
body('communityCategoryIds')
.isArray()
.notEmpty()
.withMessage('Invalid communityCategoryIds field'),
body('title').isString().notEmpty().withMessage('Invalid title field'),
body('body').isString().notEmpty().withMessage('Invalid body field'),
body('contentUrl').isString().notEmpty().withMessage('Invalid contentUrl field'),
body('contentTitle').isString().notEmpty().withMessage('Invalid contentTitle field'),
];
Copy link
Member Author

Choose a reason for hiding this comment

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

이 validator 를 어느 폴더에 넣어야 알맞을까 고민하다 다 애매해서 constants 로 넣었는데 조언 부탁드립니다.

Copy link
Member

Choose a reason for hiding this comment

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

다 애매하긴 하네요..
middleware 단에서 검증하는 부분이니까 middlewares/validator로 폴더 위치를 지정해주는건 어떨까요?

Copy link
Member

Choose a reason for hiding this comment

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

저도 middlewares 하위가 좋은 것 같습니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

오늘 수정하겠습니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

수정했습니다.

Comment on lines +55 to +58
SELECT element
FROM unnest($1::int[]) AS element
LEFT JOIN community_category ON community_category.id = element
WHERE community_category.id IS NULL;
Copy link
Member Author

Choose a reason for hiding this comment

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

  1. 주어진 배열(카테고리 id 배열)을 unnest 메서드 사용해 행집합으로 변환하고
element
1
2
3
  1. community_category 테이블과 join
  2. join 결과 테이블에서 community_category id가 없는 경우의 element 조회 == 존재하지 않는 커뮤니티 카테고리

Copy link
Member

Choose a reason for hiding this comment

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

SELECT unnest($1::int[])
EXCEPT SELECT id FROM community_category;

저는 차집합을 구할 때 주로 EXCEPT를 사용하는 편인데요, LEFT JOIN을 사용하신 이유가 따로 있는지 궁금합니다 -!

Copy link
Member

Choose a reason for hiding this comment

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

방금 찾아보니 EXCEPT는 중복을 제거하기 때문에 정렬하는 과정이 추가돼서 성능 저하가 발생할 수도 있다고 하네요..!

Copy link
Member

Choose a reason for hiding this comment

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

배열에 있는 id가 있는지 없는지 판별하는 로직인 것 같은데, WHERE IN 조건은 어떨까요? 문법 정확하지는 않을텐데 대략 이런식으로

SELECT element
FROM community_category cc
WHERE cc.id IN $1::INT[]

Copy link
Member Author

Choose a reason for hiding this comment

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

혹시 WHERE IN 조건이 해당 쿼리에 비해 성능이 좋을까요? 특별히 바꿔야하는 이유가 있을지 궁금합니다.

Copy link
Member

Choose a reason for hiding this comment

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

정확히 성능 비교는 해보지 않았는데, 우리 카테고리 수가 많지도 않고, 가독성 면에서는 더 직관적인 것 같아서 제안해봤습니다

Copy link
Member Author

Choose a reason for hiding this comment

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

음 제가 SQL이 좀 떨어져서 ㅠ 잘 이해한건지 모르겠는데 대충 아래같은 쿼리를 말하신거죠?

select cc.id 
from community_category cc
where cc.id in (1, 10, 11); // 1, 10, 11 은 배열 원소 예시

IN, NOT IN 쿼리로 조회하면 지금은 배열 -> 카테고리 테이블에 존재하지 않는 값 조회에서 반대 (카테고리 테이블에서 배열에 존재하거나/존재하지 않는 값 조회) 가 돼서 의미가 더 어려워지는 것 같은데
위에 유빈이가 언급한 차집합 연산자(EXCEPT) 나 JOIN을 사용하는게 의미상 더 좋아보입니다.

참고로 MySQL에서는 EXCEPT 연산자가 없어서 보통 JOIN 으로 차집합 연산을 구현해서 Postgres 에 따로 차집합 연산이 존재하는 걸 이제 알았네요 @yubinquitous

Copy link
Member Author

Choose a reason for hiding this comment

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

성능 이슈가 크지 않다면

select unnest($1::int[])
except 
select id FROM community_category;

요게 더 직관적이긴 하네욥 except 써보는 것도 좋을 것 같아요

Copy link
Member

Choose a reason for hiding this comment

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

좋습니다!

Comment on lines 6 to 21
const validate = (req, res, next) => {
const errors = validationResult(req);
if (errors.isEmpty()) {
return next();
}

const validatorErrorMessage = errors.array()[1] ? errors.array()[1].msg : errors.array()[1].msg;

const detailError = {
statusCode: statusCode.BAD_REQUEST,
responseMessage: responseMessage.NULL_VALUE,
validatorErrorMessage,
};

errorHandler(detailError, req, res, next);
};
Copy link
Member Author

Choose a reason for hiding this comment

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

validate 결과에 error 가 있을 경우 errorHandler 로 400 에러 객체를 보내는 미들웨어

Copy link
Member

Choose a reason for hiding this comment

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

errorHandler 를 명시적으로 호출하면, Sentry 미들웨어가 스킵될 수도 있을 것 같은데, 혹시 한 번 확인해볼 수 있을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

이거 next(detailError) 이렇게 보내도 sentry 에서 안잡혀서 일단 머지하고 제가 좀 더 테스트해볼게요

@@ -116,6 +118,7 @@ router.get(
router.post(
'/posts',
checkUser,
[...communityValidator.createCommunityPostValidator, validate],
Copy link
Member Author

Choose a reason for hiding this comment

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

라우터 미들웨어 단에서 validatior와 전역 validate 함수 같이 추가해주면 됩니다.

Copy link
Member

Choose a reason for hiding this comment

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

같은 validation 역할을 하는 미들웨어들이라 배열로 묶고 spread 한 걸까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

createCommunityPostValidator 자체가 express-validator 에서 제공하는 ValidationChain[] 타입이어서 그냥 넣으면 이중 배열이라 spread 했습니다.
validator와 validate 는 단일 구조의 미들웨어로 묶으려고 배열 사용했습니다.

Copy link
Member

@yubinquitous yubinquitous left a comment

Choose a reason for hiding this comment

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

고생하셨슴당 덕분에 깔끔하게 request validation을 할 수 있게 됐네요 👍 !

Copy link
Member

@HYOSITIVE HYOSITIVE left a comment

Choose a reason for hiding this comment

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

의견 간단하게 몇 개 남겼습니다. 전체적으로 좋아보입니다! 고생하셨어요~!

);
};

const verifyExistCategories = async (client, communityCategoryIds) => {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: verifyCategoriesExist는 어떠신가요?

return next();
}

const validatorErrorMessage = errors.array()[1] ? errors.array()[1].msg : errors.array()[1].msg;
Copy link
Member

Choose a reason for hiding this comment

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

ternary 앞 뒤 조건이 모두 errors.array()[1].msg로 동일한 것 같은데 의도된 부분일까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

아 오타입니다. custom message 가 1번 위치라 없으면 기본 메시지 출력하려고 [0] 을 넣어야하는데 오타 발생

Comment on lines 6 to 21
const validate = (req, res, next) => {
const errors = validationResult(req);
if (errors.isEmpty()) {
return next();
}

const validatorErrorMessage = errors.array()[1] ? errors.array()[1].msg : errors.array()[1].msg;

const detailError = {
statusCode: statusCode.BAD_REQUEST,
responseMessage: responseMessage.NULL_VALUE,
validatorErrorMessage,
};

errorHandler(detailError, req, res, next);
};
Copy link
Member

Choose a reason for hiding this comment

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

errorHandler 를 명시적으로 호출하면, Sentry 미들웨어가 스킵될 수도 있을 것 같은데, 혹시 한 번 확인해볼 수 있을까요?

Comment on lines +55 to +58
SELECT element
FROM unnest($1::int[]) AS element
LEFT JOIN community_category ON community_category.id = element
WHERE community_category.id IS NULL;
Copy link
Member

Choose a reason for hiding this comment

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

배열에 있는 id가 있는지 없는지 판별하는 로직인 것 같은데, WHERE IN 조건은 어떨까요? 문법 정확하지는 않을텐데 대략 이런식으로

SELECT element
FROM community_category cc
WHERE cc.id IN $1::INT[]

@@ -116,6 +118,7 @@ router.get(
router.post(
'/posts',
checkUser,
[...communityValidator.createCommunityPostValidator, validate],
Copy link
Member

Choose a reason for hiding this comment

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

같은 validation 역할을 하는 미들웨어들이라 배열로 묶고 spread 한 걸까요?

const dbConnection = await db.connect(req);
req.dbConnection = dbConnection;

const notExistCategoryIds = await communityDB.verifyExistCategories(
Copy link
Member

Choose a reason for hiding this comment

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

Nit: notExistingCategoryIds는 어떨까요?

@jokj624 jokj624 merged commit 7d23202 into develop Apr 8, 2024
@jokj624 jokj624 deleted the feature/344 branch April 8, 2024 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚀 enhancement New feature or request 🥱 jobchae
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEAT] 커뮤니티 글 작성 API
3 participants