-
Notifications
You must be signed in to change notification settings - Fork 0
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
돌봄급구 조회 시, N+1 이슈를 해결하고 필터링 조건을 추가하였습니다. #65
Conversation
DROP VIEW IF EXISTS v_sos_posts; | ||
DROP VIEW IF EXISTS v_pets; | ||
DROP VIEW IF EXISTS v_media; | ||
DROP VIEW IF EXISTS v_conditions; |
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.
돌봄급구에 한정되어 사용되는 view인 것에 비해 이름이 좀 범용적인 것 같은데 sos_post에 사용되는 view임을 나타내도록 이름을 바꿔보면 어떨까요?
- v_sos_posts
- v_pets
- v_media
- v_conditions
e.g. v_sos_posts_pets, v_pets_for_sos_posts, ...
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.
좋습니다 👍🏻
internal/domain/sos_post/sos_post.go
Outdated
CreatedAt string `field:"created_at"` | ||
UpdatedAt string `field:"updated_at"` | ||
DeletedAt string `field:"deleted_at"` |
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.
전반적으로 entity의 datetime 타입을 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.
- 엔티티마다
CreatedAt, ...
의 타입이time.Time
과string
을 혼용하고 있어 변경하게 되었습니다. string
타입으로 변경한 이유는 데이터베이스에서 시간 데이터를 조회한다면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.
기존엔 string으로 사용하고 있었는데 type safe하지 않은 것 같아서(+ nil check) DB에서 받아올 땐 time.Time으로 받고, view로 나갈 때 string으로 변환하도록 제가 일괄적으로 다 바꿔두긴 했는데요.
이건 필요한 부분만 적용하는게 어떨까요? (e.g., 돌봄급구)
나중에 어드민 API 만들 때는 deletedAt이 노출될 수 있을 것 같은데 nil 여부에 따라 빈 문자열이 아닌 null로 나가고 싶어서요.
time.Time 타입일 때 추가 가공하기 편하기도 하구요.
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.
그렇다면 time.Time으로 받아서 후가공하는 식으로 수정하겠습니다 :)
@@ -278,6 +278,338 @@ func TestSosPostService(t *testing.T) { | |||
} | |||
} | |||
}) | |||
t.Run("전체 돌봄 급구 게시글의 정렬기준을 'deadline'으로 조회한다", func(t *testing.T) { |
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.
이 부분 코드가 상당히 긴 것 같은데 data setup 이나 assert하는 부분을 최소화하면 검증하고 싶은 부분의 가독성이 높아질 것 같습니다.
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.
#63 작업이 rebase 된 후에 이어서 작업하겠습니다 :)
4b9cfcc
to
7538eea
Compare
d525b4d
to
511a0ea
Compare
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.
👍
/posts/sos/{id}
의 버그를 해결하였습니다.