Skip to content

임혜진 seminar1 과제#43

Open
hjlim7831 wants to merge 10 commits into
wafflestudio:master-week1from
hjlim7831:master-week1
Open

임혜진 seminar1 과제#43
hjlim7831 wants to merge 10 commits into
wafflestudio:master-week1from
hjlim7831:master-week1

Conversation

@hjlim7831
Copy link
Copy Markdown

  1. /api/v1/playlists/{id} API를 구현할때, MultipleBagFetchException을 피하기 위해 playlist에 포함된 song들의 id들을 리스트로 뽑아 인자로 넘겨주고 WHERE ~ IN으로 query를 날렸는데 좋은 방법인지 모르겠습니다.

  2. 테스트를 작성하고 같은 클래스 내에 있는 모든 테스트를 실행시켰을 때 테스트 간에 서로 영향을 주던데, 원래 그런건가요?

@PFCJeong
Copy link
Copy Markdown
Member

PFCJeong commented Oct 1, 2023

안녕하세요.

지금 실패하는 테스트들 몇 개 있습니다. (2)에서 질문 주신 내용 때문인 것 같은데요.

  1. 테스트 간 서로 영향을 미치는게 맞습니다. (데이터가 초기화되지 않습니다!)
  2. 모든 테스트에 @Transactional어노테이션을 붙이고 다시 푸시 부탁드립니다. 관련해서는 과제리뷰때 더 자세히 말씀드리겠습니다. (Transactional 롤백 테스트로 구글링해보셔도 좋을 것 같습니다.)

@hjlim7831
Copy link
Copy Markdown
Author

@transactional을 추가했습니다!
그 외에도 entity의 fetchType을 건드려서 문제가 더 있었던 것 같습니다. 그 부분도 수정했습니다.

Copy link
Copy Markdown
Member

@PFCJeong PFCJeong left a comment

Choose a reason for hiding this comment

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

과제 잘해주셨네요 수고하셨습니다~

val playlistGroupEntities = playlistGroupRepository.findAllOpenWithJoinFetch()

return playlistGroupEntities
.map { PlaylistGroup(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

이쁘게 개행하면 좋을 것 같습니다.

it, it2로 명시하는 것보다는 의미가 보이는 이름을 붙여주는게 가독성이 좋아질 것 같아요

user = UserEntity(id = userId)
))
}
@Transactional
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Transactional 잘 붙여주셨는데요. 트랜잭션의 범위는 작으면 작을 수록 좋습니다.

deleteByPlaylistIdAndUserId()에 트랜잭션을 붙여주셨기 때문에 요기에는 없어도 될 거 같아요.


sealed class CacheException : RuntimeException()

class CacheMissException : CacheException() No newline at end of file
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

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