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: 방명록 읽기/쓰기 기능 구현 #21

Merged
merged 23 commits into from
Jun 10, 2024
Merged

Conversation

euijinkk
Copy link
Contributor

@euijinkk euijinkk commented Jun 6, 2024

작업내용

  • 방명록 읽기 쓰기 기능 구현합니다. (UD 는 다음 시간에...)

고민거리

서버 고민들 많음

  • dto 와 db schema 잘 분리해야함. 지금 겹쳐서 쓰고 있어서 이상함
  • transaction 이 d1 에서 지원을 안함. 아래 댓글에서 상세하게 써볼게요
  • service, repository 의 차이점? 다른 service 에서는 repository 를 함부로 가져다 쓰기 보다는 service 를 가져다 쓰는 구조가 좋겠죠?

스크린샷

image image

Comment on lines +13 to +36
async getAllCommentsByGuestbookId(guestbookId: number) {
return await this.db
.selectFrom("Comments")
.where("Comments.guestbookId", "=", guestbookId)
.innerJoin("Users", "Users.id", "Comments.authorId")
.select([
"Comments.id",
"Comments.guestbookId",
"Comments.content",
"Comments.parentId",
"Comments.createdAt",
"Comments.updatedAt",
sql<User>`
json_object(
'id', Users.id,
'githubUserId', Users.githubUserId,
'thumbnailUrl', Users.thumbnailUrl,
'name', Users.name,
'githubUserName', Users.githubUserName,
'bio', Users.bio,
'githubUrl', Users.githubUrl,
'createdAt', Users.createdAt,
'updatedAt', Users.updatedAt
)`.as("author"),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

요거 DB에서 긁으려니까, 좀 이상하네요. 타입 지원이 안돼서 많이 위험한듯 합니다.
JS 코드로 합치자니 그것도 마음에 들진 않아서 고민입니다

Copy link
Contributor

Choose a reason for hiding this comment

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

JS단에서 처리하는것도 저는 좋은 방법이라고 생각해요! 결국 Repository 의 역할은 데이터 소스에서 데이터를 가져와 가공하는 것이기 때문에, DB에 JSON 이라는 형태로 저장된 포맷을 파싱하는 JS 로직이 들어가도 자연스러운 것 같아요.
여기에 꼭 kysely 의 메서드만 들어갈 필요는 없을 것 같아요!

Copy link
Contributor Author

@euijinkk euijinkk Jun 9, 2024

Choose a reason for hiding this comment

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

여기에 꼭 kysely 의 메서드만 들어갈 필요는 없을 것 같아요!

와우 그렇네요. 감사합니다 반영해볼게요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이거 수정하려고 했는데, 어차피 두개 테이블 조인한 결과를 selectAll 로 추출하기 어렵더라고요. id, createdAt, updatedAt 이 중복되서, alias 지어줘야함.
그렇게 치면 어차피 아래 같은 코드가 등장해야하고, 그러면 사실 kysely에서 처리하는 것과 다를 바가 없더라고요.
그래서 이 경우엔 kysely 로직으로만 가도 되겠다고 생각했어요

하지만 말씀하신 의견은 너무 좋은거같아서, 다른 부분 있으면 적용해볼게요!

async getAllCommentsByGuestbookId(guestbookId: number) {
  const comments = await this.db
    .selectFrom("Comments")
    .where("Comments.guestbookId", "=", guestbookId)
    .innerJoin("Users", "Users.id", "Comments.authorId")
    .select([
      "Comments.id as commentId",
      "Comments.createdAt as commentCreatedAt",
      "Comments.updatedAt as commentUpdatedAt",
      "Comments.content",
      "Comments.guestbookId",
      "Comments.authorId",
      "Users.id as userId",
      "Users.createdAt as userCreatedAt",
      "Users.updatedAt as userUpdatedAt",
    ])
    .execute();

  return comments.map(
    ({
      id,
      content,
      parentId,
      createdAt,
      updatedAt,
      authorId,
      ...author
    }) => ({
      id,
      content,
      parentId,
      createdAt,
      updatedAt,
      author: { ...author, id: authorId },
    })
  );
}

Comment on lines +66 to +75
async createUserAndMinihomeAndGuestbook(
props: Omit<User, "id" | "createdAt" | "updatedAt">
) {
const createdUser = await this.getUserOrCreateUser(props);
const createdMinihome = await this.minihomeService.getMinihomeOrCreate(
createdUser.id
);
await this.guestbookService.getGuestbookOrCreate(createdMinihome.id);
return createdUser;
}
Copy link
Contributor Author

@euijinkk euijinkk Jun 6, 2024

Choose a reason for hiding this comment

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

d1 에서 transaction 이 안되어서 이런 로직이 탄생했어요.
D1: our quest to simplify databases

대신에 batch를 활용한다고 하는데, batch 는 동기적으로 응답을 활용할 수 없고, 한번에 여러개 쏘는 구조예요.

Query D1 · Cloudflare D1 docs

kysely - d1 에서 transaction 사용이 큰 주제인듯 하네요.
aidenwallis/kysely-d1#2

Copy link
Contributor

Choose a reason for hiding this comment

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

D1의 단점이라 슬픕니다.. ㅠ

Comment on lines +42 to +62
const userService = new UserService({
env,
userRepository: new UserRepository({
db,
}),
minihomeService: new MinihomeService({
env,
minihomeRepository: new MinihomeRepository({ db }),
}),
guestbookService: new GuestbookService({
env,
gestbookRepository: new GuestbookRepository({ db }),
}),
});
const authService = new AuthService({ env });
const commentService = new CommentService({
env,
commentRepository: new CommentRepository({ db }),
userService,
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

손좀 봐야겠어요 ㅎㅎ

Copy link
Contributor

Choose a reason for hiding this comment

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

어떤걸요!?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

중복 코드가 너무 많아지고, 각 클래스가 싱글톤 유지가 안될수도 있는 등 좀 아쉽다고 생각했어요!

nestjs 의 패턴이 왜 좋은지 조금씩 배우는 중입니다 ㅎㅎ
일단 더 해보고 개선 방향성 생각해볼게욥!

@euijinkk euijinkk marked this pull request as ready for review June 6, 2024 09:03
@euijinkk euijinkk requested review from Tekiter and joohaem June 6, 2024 09:03
Comment on lines 41 to 43
} catch (e) {
throw new HTTPException(404, { message: "Guestbook not found" });
}
Copy link
Contributor

@Tekiter Tekiter Jun 9, 2024

Choose a reason for hiding this comment

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

현재 try 문 안의 에러의 원인은 아주 다양할 수 있어서, 모든 예외를 일괄적으로 404 처리하면 문제가 발생했을 때 원인 파악이 매우 힘들어질 수 있을 것 같아요.
(DB 연결 오류? DB 가져오기 오류? 파싱 오류? 그 외 타입오류?),

이 문제를 해결할 수 있는 방식은 대표적으로 두 가지가 있을 것 같은데요!

  1. 아래쪽 레이어에서 Guestbook 이 없음을 감지하고 NotFoundError 같은 특별한 에러를 던지는 방법
    a. 장점: 밑에서 던진 에러를 한번에 일괄적으로 처리 가능
    b. 단점: 어떤 에러가 던져질 수 있는지 컨트롤러단에서 예측하기 힘듦
try {
  // blah blah

  const { comments, guestbookId } =
    await commentService.getAllGuestbookCommentsByGithubUserName(
    githubUserName
  );

  return ctx.json({
    success: true,
    data: { comments, guestbookId },
  });
} catch (e) {
  if (e instanceof NotFoundError) {
    throw new HTTPException(404, { message: "Guestbook not found" });
  }
  // 그 외 에러 500 처리
}
  1. 아래쪽 레이어에서 Guestbook 이 없음을 감지하고 에러를 표시해 리턴하기
    a. 장점: 가능한 에러들을 놓치지 않고 다 받아낼 수 있음
    b. 단점: 에러들이 return 을 통해 전파되기 때문에 일종의 드릴링 현상이 일어날 수 있음
type BlahResult = { result: 'success', comments: unknown[], guestbookId: string } | { result: 'notFound', comments: null, guetsbookId: null }
try {
  // blah blah

  const { result, comments, guestbookId } =
    await commentService.getAllGuestbookCommentsByGithubUserName(
    githubUserName
  );

  if (result === 'notFound') {
    return // 400 에러처리
  }

  return ctx.json({
    success: true,
    data: { comments, guestbookId },
  });
} catch (e) {
  // 에러 500 처리
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

좋은 의견 감사합니다. 2번이 타입 안정성 측면에서 더 좋은거 같아서, 일단 2번으로 해보겠습니다!

feat: notFound, 500 에러 처리 구분하기

Comment on lines +6 to +13
export function addTimeStamp<T>(obj: T): T & TimestampedObject {
const now = new Date().toISOString();
return {
...obj,
createdAt: now,
updatedAt: now,
};
}
Copy link
Contributor

Choose a reason for hiding this comment

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

DB 에는 타임스탬프 INTEGER 값으로 저장하고, 클라와 통신할 때는 ISO 8601 Date String 형태로 다루는게 효율적일 것 같은데 어떠신가요?
(Integer 값을 사용하면 SQL에서 날짜순으로 정렬해서 가져오거나, 특정 시점 전/후 조건이 가능하다는 장점이 있어요)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

오... 굉장히 좋은 생각입니다요.

(Integer 값을 사용하면 SQL에서 날짜순으로 정렬해서 가져오거나, 특정 시점 전/후 조건이 가능하다는 장점이 있어요)

요걸 생각못했네요 ㅎㅎ. 별도 PR 에서 반영해볼게요!

@@ -0,0 +1,44 @@
DROP TABLE IF EXISTS Users;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Tekiter
파일 합쳤습니다~~
테이블 생성도 쉽고 훨씬 좋네요!

@euijinkk euijinkk added this pull request to the merge queue Jun 10, 2024
Merged via the queue into main with commit 12c96fd Jun 10, 2024
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.

2 participants