황두현 Seminar1 과제 by DoohyunHwang97 · Pull Request #55 · wafflestudio/seminar-2023-springboot-assignment

DoohyunHwang97 · 2023-10-02T12:38:13Z

No description provided.

davin111

@DoohyunHwang97 테스트 하나가 실패하는데, 인지하고 계셨나요? 확인해보시고 답변 부탁드립니다~

davin111 · 2023-10-21T06:51:42Z

+    @ManyToOne
+    @JoinColumn(name = "group_id")
+    val playlistGroup: PlaylistGroupEntity


어떠한 경우의 연관관계에도 Lazy 를 지향하는 것이 좋습니다. ManyToOne 의 default 가 EAGER 라 하더라도 이것을 변경해두는 것이 좋아요. wafflestudio/seminar-2023#169 를 참고하시면 좋습니다.

이걸 Lazy 로 해두었다면, 불필요한 N+1 쿼리가 덜 날아가서 곳곳에서 JOIN FETCH 를 덜 써도 되었을 수도 있어요.

davin111 · 2023-10-21T06:52:24Z

+    @ManyToOne
+    @JoinColumn(name = "group_id")
+    val playlistGroup: PlaylistGroupEntity
+)


git 에서 파일의 마지막 newline 을 넣는 것이 좋은 습관입니다.

davin111 · 2023-10-21T06:53:31Z

+//    @OneToMany(mappedBy = "album")
+//    val songs: List<SongEntity>,


사용하지 않는 코드는 아예 지우는 것이 좋습니다.

davin111 · 2023-10-21T06:53:54Z

+@Repository
+interface AlbumRepository : JpaRepository<AlbumEntity, Long> {
+    @Query("SELECT a FROM albums a LEFT JOIN FETCH a.artist " +
+            "WHERE a.title LIKE CONCAT('%',:title,'%') ORDER BY length(a.title) ASC")


쿼리로 DB 에서 length() 로 order 하는 것보단, 일단 application (서버) 에 가져와서 코드로 sort 하는 게 성능상 더 나은 선택일 것 같습니다.

@davin111 성능상 더 나은 선택이라는 것이 궁금해서 혹시 이유도 설명해주실 수 있을까요? 저는 먼가 DB에서 최대한 덜 가져오는 것이 성능상 이점이 있을 것 같아서 저렇게 짜긴 했는데 인사이트가 궁금합니다(__)

ORDER BY 를 한다고 해서 DB 에서 가져오는 데이터 양이 줄어들진 않을 거고요. 정렬만 하는 거니까요. DB 에서 함수를 써서 연산을 가하면 index 를 일반적으로 사용할 수 없어서 느려집니다. 데이터 양이 많을수록 이는 치명적일 수 있고요.

davin111 · 2023-10-21T06:54:38Z

 import org.springframework.data.jpa.repository.Query
+import org.springframework.stereotype.Repository

+@Repository


참고로 JpaRepository interface 를 상속하는 경우, @Repository annotation 을 붙이지 않아도 됩니다.

davin111 · 2023-10-21T07:00:44Z

+    @Test
+    fun `로그인 상태로 플레이리스트 조회`() {


이 테스트가 실패하네요.

@davin111 저번 세미나 때 PlaylistIntegrationTest 테스트에 Transactional 어노테이션을 붙히지 않아서 테스트끼리 독립되지 않아 개별 테스트로는 통과하지만 전체 테스트로 돌리게 되면 실패하는 것으로 피드백받았었습니다! 일단 개별 테스트 통과해서 제출은 했었는데 세미니장님이 이번 과제에 한해서 개별테스트는 통과하는 경우에는 통과하는 것으로 인정해주신다고 해서 따로 수정은 안했습니다:)

davin111 · 2023-10-21T07:02:19Z

 interface UserRepository : JpaRepository<UserEntity, Long> {
    fun findByUsername(username: String): UserEntity?
+
+    override fun findById(id: Long): Optional<UserEntity>


이건 override 를 굳이 하셨을 필요가 없었을 거 같아요.

davin111 · 2023-10-21T07:04:40Z

+        if (playlistGroupList.isNotEmpty()){
+            return playlistGroupList
+        } else throw PlaylistNotFoundException()


playlistGroupList 가 비었다면 빈 list 응답을 그대로 내려도 될 거 같아요. 그런데 만약 exception 을 던지기로 결정하셨다면, service 동작의 전반부에서 먼저 처리를 하는 식으로 순서를 바꾸면 좋겠습니다. early return pattern 이라고 불렀던 거 같네요.

@davin111 빈 list 응답이라는 것이 playlistGroupList 가 비었다면 서버에서 클라이언트로 빈 리스트를 보내준다는 것인걸까요?

넵 그런 스펙과 구현도 충분히 가능하단 얘기였습니다.

감사합니다:)

davin111 · 2023-10-21T07:05:52Z

+        val playlistGroupList = playlistGroupRepository.findAllByOpen(true)
+            .filter { it.playlists.isNotEmpty() }
+            .map {
+                    playlistGroupEntity -> PlaylistGroup(
+                id = playlistGroupEntity.id,
+                title = playlistGroupEntity.title,
+                playlists = playlistGroupEntity.playlists.map {
+                        playlistEntity -> PlaylistBrief(playlistEntity.id, playlistEntity.title, playlistEntity.subtitle, playlistEntity.image)
+                })}


이 부분과 get() 메서드의 코드들이 더 간결할 수 있을 거 같은데, 세미나장의 구현을 참고해보셔도 좋을 듯 합니다. adbc14a#diff-fb8eefb0b18924930e6d751a08235b98aa23ada492f6ae4fe535a1ac8e54da12R21-R29

davin111 · 2023-10-21T07:08:28Z

+    @Query("SELECT p FROM playlists p " +
+            "LEFT JOIN FETCH p.playlistGroup " +
+            "LEFT JOIN FETCH p.playlistSongs ps " +
+            "LEFT JOIN FETCH ps.song s " +
+            "LEFT JOIN FETCH s.album a " +
+            "LEFT JOIN FETCH a.artist " +
+            "WHERE p.id = :id")
+    fun findByIDJoinFetch(id: Long) : PlaylistEntity?


막상 service 에서는 쿼리를 songRepository.findAllByIDListJoinFetch() 과 함께 나눠서 실행하는데, 여기서 불필요하게 너무 많이 join 해서 가져오는 듯한 느낌이 있네요. 이거 혹시 연관관계 FetchType 을 EAGER 로 해둬서 발생하는 N+1 때문이었다면, 다른 코멘트와 조합해서 생각해보시면 좋을 듯해요.

N+1 때문이었던 것 맞아요! LAZY로 고쳐서 한번 시도해봐야겠네요. 감사합니다(__)

DoohyunHwang97 added 5 commits September 30, 2023 12:48

controller & entities

3bc7d8c

entity initialize & song service test, album repository test

9ceb6cf

modified SongIntegrationTest

9959612

playlistService test done

8fafea1

playlistlike service&Integration test done

6f05ed3

davin111 reviewed Oct 21, 2023

View reviewed changes

PFCJeong approved these changes Oct 23, 2023

View reviewed changes

		// @OneToMany(mappedBy = "album")
		// val songs: List<SongEntity>,

		@Test
		fun `로그인 상태로 플레이리스트 조회`() {

Conversation

DoohyunHwang97 commented Oct 2, 2023

Uh oh!

davin111 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants