[FIX] 리그팀 수정 시 발생하는 오류 해결 #245
Conversation
src/main/java/com/sports/server/command/leagueteam/application/LeagueTeamService.java
Show resolved
Hide resolved
| @@ -97,7 +110,7 @@ void setUp() { | |||
| } | |||
|
|
|||
| @Test | |||
| void 유효하지_않은_이미지_url을_등록하려고_하는_경우_예외가_발생한다() { | |||
| void S3으로부터_받아오지_않은_이미지_url을_등록할_경우_예외가_발생한다() { | |||
There was a problem hiding this comment.
이건 수정이 아니라 등록에 대한 테스트니까 유효하지_않은_이미지_url을_등록하려고_하는_경우_예외가_발생한다 에 대한 테스트는 유지되어야 하지 않을까요? 왜 수정하신건지 궁금합니다!
There was a problem hiding this comment.
아
수정할 때
- url이 그대로면 그냥 패스
- 변경됐으면 다시 changeLogoImageUrlToBeSaved 메소드 거쳐서 url 변경
이라 등록이나 수정이나 둘 다 같은 '유효하지_않은_이미지_url을_등록하려고_하는_경우_예외가_발생한다'를 사용하려고 테스트명을 변경했었어요
url이 origin prefix를 포함하는지 검사 == S3서버에 이미지 등록된 후 받아온 url인지 검사
라고 생각해서 테스트명만 변경했던 건데
'S3으로부터_받아오지_않은_이미지_url을_등록할_경우_예외가_발생한다'가 아니라 '유효하지_않은_이미지_url을_등록하려고_하는_경우_예외가_발생한다'가 더 이름이 명확하려나용,,
처음에 S3에 접속되는지도 같이 테스트해야겠다고 생각했던 시점에서.. 유효하지 않은 url이라는 게 s3에서 받아온 url이 아니(origin을 포함하지 않는 url) 라는 건지, 아니면 접속이 되지 않는 url 이라는 건지 헷갈릴 수도 있겠다고 생각해서 변경했긴 했었어요!
There was a problem hiding this comment.
훔 좀 더 직관적으로 origin을_포함하지_않은_url을_등록하려고_하는_경우_예외가_발생한다 로 바꿀까여??
| @@ -208,4 +223,10 @@ void setUp() { | |||
| .forEach(id -> assertThat(leagueTeamPlayerFixtureRepository.findById(id)).isEmpty()); | |||
| } | |||
|
|
|||
| // @Test | |||
There was a problem hiding this comment.
doesFileExist가 잘 돌아가는지 로컬에서 테스트하는 용으로 남겨뒀던 건데 이미지 주소를 그대로 두면 안되겠네용. 삭제하겠습미다
| String imageUrl; | ||
| if (!request.logoImageUrl().equals(leagueTeam.getLogoImageUrl())) { | ||
| imageUrl = changeLogoImageUrlToBeSaved(request.logoImageUrl()); | ||
| } else { | ||
| imageUrl = leagueTeam.getLogoImageUrl(); | ||
| } | ||
| s3Service.doesFileExist(imageUrl); |
There was a problem hiding this comment.
LeagueTeam 엔티티에 있는 수정메서드에 위의 로직을 넣어보면 어떨까요?
LogoImage 변환은 하고, 해당 로고 이미지 url만 문자열로 객체에게 전달하고, 이미지 url이 같은지 다른지는 객체가 판단하게끔 두면 좋을거 같아요
TDA(Tell, Don't Ask)을 고려해보면 좋을거 같아요.
There was a problem hiding this comment.
오 엔티티 내의 수정메서드로 옮기는 거 좋은 것 같아요! 어차피 엔티티 수정될 때만 사용되는 로직이니까 옮겨두겠습니다
링크 걸어주신 TDA 잘 읽어보았습미다 🫡 근데 현재 로직의 경우
- 이미지 변경이 없는 경우 : DB에 있는 url이 들어옴(replaced)
- 이미지 변경이 있는 경우 : S3 서버로부터 받아온 raw url이 들어옴(origin)
라서 만약 이미지 url이 같은지 다른지를 체크를 하지 않고(=이미지 변경을 체크하지 않고) 이미지 변환을 무조건 거칠 경우
1의 경우 이미지 변환 시 에러가 던져져서.. 이번 경우는 TDA를 적용할 수 없고 체크를 해줘야 하는 경우라고 생각이 드는데 어떤 의견을 가지고 계신지 궁금합니다요
There was a problem hiding this comment.
리그팀 이미지 첫 등록/변경의 경우 프론트측에서 S3 서버에 이미지 올리면서 origin url을 post/put
-> 백에서는 해당 url을 받아서 replace url로 변경한 뒤 db에 저장
-> 그 이후로 리그팀 정보 조회할 때는 db에 있는 값이 리턴되기 때문에 db에 있는 url을 주고받는 걸로 알고 있습니당
There was a problem hiding this comment.
URL을 변경하지 않을거라면 꼭 기존 URL을 다시 돌려 받지 않아도 될거 같아요.
이미지를 변경 하지 않을거라면 기존 URL을 받아도 사실 사용안하는 값이니까요.
더더군다나 Bean Validation도 사용하고 있지 않으니 Null 이나 빈 문자열을 받아서 도메인 레벨에서 처리하도록 구현할 수 있을거 같아요.
| String imageUrl; | ||
| if (!request.logoImageUrl().equals(leagueTeam.getLogoImageUrl())) { | ||
| imageUrl = changeLogoImageUrlToBeSaved(request.logoImageUrl()); | ||
| } else { | ||
| imageUrl = leagueTeam.getLogoImageUrl(); | ||
| } | ||
| s3Service.doesFileExist(imageUrl); |
🌍 이슈 번호
📝 구현 내용
🍀 확인해야 할 부분