Skip to content

Conversation

@mirlee0304
Copy link

No description provided.

Copy link
Member

@davin111 davin111 left a comment

Choose a reason for hiding this comment

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

cc. @PFCJeong

@mirlee0304 테스트 하나가 실패하고 있습니다. 확인 부탁드립니다.

Copy link
Member

Choose a reason for hiding this comment

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

.DS_Store 와 이런 불필요한 빌드 결과물들은 git 에 안 올리는 것이 좋습니다. .gitignore 을 사용하세요.



interface PlaylistGroupRepository : JpaRepository<PlaylistGroupEntity, Long> {
@Query("SELECT pg FROM playlist_groups pg LEFT JOIN FETCH pg.playlists p WHERE pg.open = TRUE AND SIZE(p) > 0")
Copy link
Member

Choose a reason for hiding this comment

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

JPQL 로 SIZE(p) > 0를 하면 실제 쿼리가 복잡해질 수도 있어서, 일단 가져와서 서버 코드로 playlists 가 isNotEmpty()인 것만 filter 하는 것도 좋을 것 같습니다.

val image: String,
@ManyToOne // default FetchType.EAGER
@JoinColumn(name = "artist_id")
@JoinColumn(name = "artist_id") //여러앨범1아티스트
Copy link
Member

Choose a reason for hiding this comment

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

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

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

val id: Long = 0L,
val title: String,
val duration: Int,
@ManyToOne(fetch = FetchType.LAZY)
Copy link
Member

Choose a reason for hiding this comment

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

LAZY 로 바꿔둔 것 좋습니다.

@@ -0,0 +1,23 @@
//망한mapper
Copy link
Member

Choose a reason for hiding this comment

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

ㅋㅋㅋㅋㅋ 불필요한 코드는 지우면 좋습니다.


override fun search(keyword: String): List<Song> {
TODO()
return songRepository.searchSongTWithFetchJoin("%$keyword%").map(SongEntity::toSong)
Copy link
Member

Choose a reason for hiding this comment

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

여기서 %를 써서 넘기기보단, 그냥 repository @Query 내부에 %를 쓰는 게 좋을 것 같네요. 쿼리와 관련한 것을 service 계층에서 직접 다루고 있는 것이 아쉬운 부분입니다.

@Query("SELECT s FROM songs s LEFT JOIN FETCH s.album al LEFT JOIN FETCH s.artists sa LEFT JOIN FETCH sa.artist a WHERE s.id IN :songs")
fun searchSongIWithFetchJoin(songs: List<Long>): List<SongEntity>

@Query("SELECT s FROM songs s LEFT JOIN FETCH s.album al LEFT JOIN FETCH s.artists sa LEFT JOIN FETCH sa.artist a WHERE s.title LIKE :keyword ORDER BY LENGTH(s.title)")
Copy link
Member

Choose a reason for hiding this comment

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

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

Comment on lines +23 to +26
//val playlistGroupEntities = playlistGroupRepository.findAllWithFetchJoin().filter { it.playlists.isNotEmpty() }

//return playlistGroupEntities.map { entity ->
// PlaylistMapper.toPlayListGroup(entity)
Copy link
Member

Choose a reason for hiding this comment

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

불필요한 코드는 지우기!

TODO()
val playlist = playlistRepository.findByIdWithSongs(id) ?: throw PlaylistNotFoundException()
val songs = songRepository.searchSongIWithFetchJoin(playlist.playlistSongs.map { it.song.id })
return Playlist(playlist, songs)
Copy link
Member

Choose a reason for hiding this comment

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

이 메서드와 관련해, 테스트 플레이리스트 조회, 쿼리 횟수는 2회로 제한가 실패하네요. N+1 문제가 발생하고 있는 것 같습니다.

Comment on lines +33 to +36
val isLiked = when (user) {
null -> false
else -> playlistLikesService.exists(id, user.id)
}
Copy link
Member

Choose a reason for hiding this comment

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

요기는 굳이 when 을 써서 얻는 이점이 있을까? 싶기도 하네요. 그냥 if 문으로 써도 어쩌면 더 간결할지도?

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