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

🐛 fix: music save problem solve #62

Merged
merged 2 commits into from
May 26, 2023
Merged

🐛 fix: music save problem solve #62

merged 2 commits into from
May 26, 2023

Conversation

seonghun-dev
Copy link
Collaborator

@seonghun-dev seonghun-dev commented May 26, 2023

resolve : #61

  • Music Save problem Solve
  • 다른 도메인간 JPA 연관관계 영속성 전이 방지, 강결합 방지
  • Item, ItemLocation 통합 - 개념적인 분리요소 X

@@ -11,7 +11,7 @@
@Getter
@NoArgsConstructor
@Validated
public class LocationRequestDto {
public class ItemLocationRequestDto {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Item 도메인 하위에 종속되므로 ItemRequest로 이동하였습니다

@@ -33,20 +32,24 @@ public class Item extends BaseTimeEntity {
@JoinColumn(name = "user_id")
private User user;

@ManyToOne(fetch = LAZY, cascade = CascadeType.ALL)
@ManyToOne(fetch = LAZY)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

서로 다른 도메인간 강결합 해제

Copy link
Collaborator

@yunyoung1819 yunyoung1819 May 26, 2023

Choose a reason for hiding this comment

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

한번의 아이템 드랍 시 itemRepository.save(), musicRepository.save() 등 여러번 save를 호출하는 것을 피하고 item 저장 시 연관 객체도 한꺼번에 저장하려고 했던 부분인데 강결합으로 묶이는 단점도 있는 것 같습니다.

@JoinColumn(name = "song_id")
private Song song;

@ManyToOne(fetch = LAZY, cascade = CascadeType.ALL)

@ManyToOne(fetch = LAZY)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

서로 다른 도메인간 강결합 해제


var item = Item.builder()
Item item = Item.builder()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Item과 ItemLocation 객체간의 협력관계가 크게 존재하지 않아서
다른 객체로 분류할 필요가 없을 것 같아서 통합했습니다

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good! 좋은 것 같습니다.

private String albumImage;

private String albumThumbnail;

@Builder
public AlbumCover(Album album, String albumImage, String albumThumbnail) {
this.album = album;
public AlbumCover(String albumImage, String albumThumbnail) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

AlbumCover에서 Album 조회 할 일이 없어서 제거
OneToOne 매핑이라 필요시 추가

String albumName = musicRequestDto.getAlbumName();
String albumImage = musicRequestDto.getAlbumImage();

Optional<Artist> artist = findArtist(artistName);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아티스트가 없다면, 아티스트, 앨범, 음악 모두 저장 후 반환
아티스트-앨범이 없다면, 앨범, 음악 모두 저장 후 반환
앨범-음악이 없다면, 음악 저장 후 반환

Copy link
Collaborator

@yunyoung1819 yunyoung1819 May 26, 2023

Choose a reason for hiding this comment

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

데이터 저장 시 중복을 확인하기 위한 DB 조회 로직이 많아서 음악 데이터는 계속 쌓일텐데 성능 면에서는 느려질 수도 있을 것 같습니다.
중복 저장이 되지 않는다는 점에서 DB에서 차지하는 공간은 절약할 수 있을 것 같습니다. 우선 이렇게 작업하고 추후 개선점을 다같이 고민해보면 좋을 것 같습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

음악쪽은 초기를 제외하면 대부분 조회로직일 것 같아서 아예 NoSQL로 전환해보는 것도 좋을 것 같습니다

Copy link
Collaborator

Choose a reason for hiding this comment

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

넵 NoSQL로 전환하는 것이 좋다고 생각합니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

중간발표이후 바로 논의해보는게 좋을 것 같습니다

Copy link
Member

Choose a reason for hiding this comment

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

저도 음악 관련 데이터는 RDB보다 NoSQL에 저장되는 게 더 적합할 것 같아요

.genres(new ArrayList<>())
.build()
);
public Optional<Song> findSong(String songTitle, Album album) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

각 함수가 하나의 역할만 하도록 변경

public void updateAlbum(Album album, AlbumCover albumCover) {
album.updateAlbumCover(albumCover);
@Transactional
public Album createAlbum(Artist artist, String albumName, String albumImage) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

앨범과 앨범 커버는 부모 자식 관계라 함께 처리

@seonghun-dev seonghun-dev self-assigned this May 26, 2023
@seonghun-dev seonghun-dev added the ✨feature New feature or request label May 26, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #62 (b1d25aa) into main (5177a7d) will increase coverage by 0.72%.
The diff coverage is 0.00%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main      #62      +/-   ##
============================================
+ Coverage     28.91%   29.64%   +0.72%     
  Complexity       52       52              
============================================
  Files            32       31       -1     
  Lines           287      280       -7     
  Branches          2        4       +2     
============================================
  Hits             83       83              
+ Misses          202      195       -7     
  Partials          2        2              
Impacted Files Coverage Δ
...depromeet/streetdrop/domains/item/entity/Item.java 0.00% <0.00%> (ø)
...t/streetdrop/domains/item/entity/ItemLocation.java 0.00% <0.00%> (ø)
...t/streetdrop/domains/item/service/ItemService.java 19.35% <0.00%> (-4.65%) ⬇️
...eetdrop/domains/music/album/entity/AlbumCover.java 0.00% <0.00%> (ø)
...streetdrop/domains/music/artist/entity/Artist.java 0.00% <0.00%> (ø)
...streetdrop/domains/music/service/MusicService.java 0.00% <0.00%> (ø)
...eet/streetdrop/domains/music/song/entity/Song.java 0.00% <0.00%> (ø)

@seonghun-dev seonghun-dev merged commit 1d5ae58 into main May 26, 2023
@seonghun-dev seonghun-dev deleted the refactor/dropitems branch May 26, 2023 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drop Item Refactoring
4 participants