Feat/#127 ManiaDB API 를 사용하여 등록되지 않은 노래 검색 기능 구현 #153

Cyma-s · 2023-07-29T06:50:49Z

📝작업 내용

ManiaDB API 를 사용하여 등록되지 않은 노래를 검색하는 기능을 구현했습니다.

💬리뷰 참고사항

WebClient 를 사용하여 외부 API 와 통신하는 로직을 구현했습니다.

ManiaDBController, ManiaDBService 위치를 song 하위에 패키지 없이 두었는데, 위치가 괜찮은지 궁금합니다.

WebClient를 사용하는 것

RestTemplate, Feign 같은 다른 대체 라이브러리 대신 WebClient를 사용하게 된 이유에 대한 글은 따로 정리한 후 올리겠습니다.

현재 코드 상에서 ManiaDB로 요청을 보내는 메서드는 non-blocking으로 동작하지 않고, blocking으로 동작하고 있습니다.

테스트 주의 사항

테스트 중, 통합 테스트가 있는데 그 부분은 ManiaDB 상태에 따라서 될 때도 있고 안 될 때도 있습니다.. 양해 부탁드립니다.

주로 한글로 검색어를 적었을 때 예외가 발생하며, 여러 번 요청을 보내는 경우 ManiaDB가 뻗을 때도 있습니다. 유의하세요.

#️⃣연관된 이슈

closes #127

- webclient 사용

github-actions · 2023-07-29T06:52:21Z

Unit Test Results

  42 files   42 suites 5s ⏱️
156 tests 156 ✔️ 0 💤 0 ❌
157 runs 157 ✔️ 0 💤 0 ❌

Results for commit ef58544.

♻️ This comment has been updated with latest results.

splitCoding

베로 너무 수고많았으~~ 코드 보는 데도 고생한게 느껴지더라~👍

splitCoding · 2023-07-29T09:56:23Z

backend/src/main/java/shook/shook/song/application/ManiaDBSearchService.java

+            .acceptCharset(StandardCharsets.UTF_8)
+            .retrieve()
+            .onStatus(HttpStatusCode::is4xxClientError, (clientResponse) -> {
+                throw new UnregisteredSongException.ManiaDBClientException();


개인적으로 등록되지 않은 노래 예외라는 뜻의 UnregisteredSongException 의 inner class로 ManiaDBClientException 이 있는 것이 조금 부자연스러워 보이는 것 같아요~

Suggested change

throw new UnregisteredSongException.ManiaDBClientException();

throw new ExternalApiException.ManiaDBClientException();

좋은 피드백 감사합니다! 사실 이름 짓기가 너무 어려워서 ㅋㅋㅋ ExternalApiException 좋네요 👍

splitCoding · 2023-07-29T09:56:38Z

backend/src/main/java/shook/shook/song/application/ManiaDBSearchService.java

+                throw new UnregisteredSongException.ManiaDBClientException();
+            })
+            .onStatus(HttpStatusCode::is5xxServerError, (clientResponse) -> {
+                throw new UnregisteredSongException.ManiaDBServerException();


위에 코멘트와 동일합니다!!�

Suggested change

throw new UnregisteredSongException.ManiaDBServerException();

throw new ExternalApiException.ManiaDBServerException();

splitCoding · 2023-07-29T10:30:18Z

backend/src/main/java/shook/shook/song/application/dto/maniadb/UnregisteredSongResponses.java

+
+@Getter
+@XmlRootElement(name = "channel")
+public class UnregisteredSongResponses {


등록되지 않은 노래 목록 응답이라는 뜻 UnregisteredSongResponses 네이밍을 사용했네요~

개인적으로 아직 ManiaDB가 DB에 없는 노래만 검색하기 위해서 사용한다는 정책이 모두와 같이 결정된 사항이 아니기도 하고

ManiaDBSearchService.searchSongs() 메서드의 반환 객체 네이밍이 UnregisteredSongResponses 인게 저는 부자연스러운 것 같다고 느껴져요~

어차피 ManiaDB api에 종속적인 코드들에서 사용되는 DTO기에 개인적으로는 아래 네이밍이 어떨까 싶어요~

Suggested change

public class UnregisteredSongResponses {

public class SearchedSongsWithManidaDbApiResponses {

혹시 추후에 등록되지 않은 노래의 검색, 등록된 노래의 검색 을 나누었을 때 등록되지 않는 노래의 검색을 mainaDB api가 아닌 다른 방법으로 구현할 수도 있다는 확장성을 위해 택한 네이밍일까요??

넹 이해하신 내용이 맞습니다
등록되지 않은 노래의 검색 / 등록된 노래의 검색이 분리되어 있을 것이라고 가정하고 DTO 이름을 설정해서 UnregisteredSongResponse 라고 하긴 했어요!

그렇다면 ManiaDB에서 가져오는 결과라는 뜻을 좀 더 확실하게 담기 위해 SearchedSongFromManiaDBApiResponse 라는 네이밍은 어떤가요?
이 부분은 스플릿 코멘트 확인 후에 반영하겠습니다~ 😄

i like it👍

splitCoding · 2023-07-29T10:43:15Z

backend/src/main/java/shook/shook/song/ui/ManiaDBApiController.java

+import shook.shook.song.application.dto.UnregisteredSongSearchResponse;
+
+@RequiredArgsConstructor
+@RequestMapping("/songs/unregistered/search")


등록되지 않은 상황에서만으로 가정한 기능이였군요!

위에 코멘트에 대한 의문이 풀렸습니다~ 👍

splitCoding · 2023-07-29T10:55:42Z

backend/src/main/java/shook/shook/song/ui/ManiaDBApiController.java

+@RequiredArgsConstructor
+@RequestMapping("/songs/unregistered/search")
+@RestController
+public class ManiaDBApiController {


개인적으로 "/songs/unregistered/search" 에 대한 컨트롤러 이름이 ManiaDBApiController인 것이 조금 부자연스러운 것 같아요!

UnregisteredSongSearchController 인터페이스, UnregisteredSongSearchService 인터페이스 분리를 통하여 ManiaDB를 사용하는 구현체를 둘 수 있는 방법에 대해서 베로도 알고 있을 거에요.

아마 베로도 인터페이스로 모두 분리하기에는 굳이? 와 오버엔지니어링일 꺼라고 생각했을 것 같고 저도 그렇게 생각합니다!

Controller 이름만을 UnregisteredSongSearchController로 아예 바꾸고 내부에서 사용하는 Service만 ManiaDBSearchService 인 것은 어떨까요??

컨트롤러 이름이 ManiaDBApiController 지만 사실상 하는 일은 searchUnregisteredSong() 이고 컨트롤러의 네임과 역할이 맞지 않는 것 같다는 생각이 들어요~

구체적인 비즈니스 로직을 수행하는 Service 레이어의 타입에만 ManiaDB이라는 단어를 포함해도 충분하지 않을 까라는 생각을 해봅니다~

오 좋은 피드백 감사합니다 👍
확실히 구체적인 비즈니스 로직을 수행하는 Service 이름이 구체적인 건 좋지만 Controller 이름이 API 이름으로 되어 있는 건 좀 부자연스럽기도 하네요 🤔

splitCoding · 2023-07-29T11:15:21Z

backend/src/test/java/shook/shook/song/application/ManiaDBSearchServiceTest.java

코드만 봐도 고생함이 느껴지는 테스트 👍

splitCoding · 2023-07-29T11:16:18Z

backend/src/test/java/shook/shook/song/ui/ManiaDBApiControllerTest.java

Controller에 예외 케이스에 대한 테스트 코드들이 추가되야 할 것 같아요!!

통합 테스트는 제거할 예정입니다!
mocking 되지 않은 상태로 '실제' 요청을 보내는 코드다보니 테스트가 실패하는 경우도 있어서, 실제 요청을 보내는 테스트 메서드는 아예 제거하려고 합니다. (그렇다면 통합 테스트는 없어지겠죠 😂 추가하더라도 service를 모킹한 controller test 를 사용하지 않을까 싶습니다.)

splitCoding · 2023-07-29T11:18:31Z

backend/src/test/java/shook/shook/song/ui/ManiaDBApiControllerTest.java

+
+    @DisplayName("내부 DB에 등록되지 않은 노래를 검색할 때 200 상태코드, 노래의 이름과 가수, 앨범 커버를 담은 응답을 반환한다.")
+    @Test
+    void registerPart_unique() {


테스트 메서드 명이 registerPart_unique 인데 의도하신 건가요??

의도는 했는데 마찬가지로 메서드 이름이 짓기 어려웠어요 ㅋㅋㅋ 😂
위에 언급했듯이 통합 테스트는 사라질 예정입니다!

splitCoding · 2023-07-29T11:39:03Z

backend/src/main/java/shook/shook/song/application/ManiaDBSearchService.java

+        if (Objects.isNull(result)) {
+            throw new EmptyResultException();
+        }


제가 생각한 해당 분기에 들어오는 경우입니다.

getResultFromManiaDB 메서드를 실행한다.

webClient 통신에 성공한다.

응답코드가 4xx, 5xx 가 아니다.

응답 본문을 ManiaDBAPISearchResponse.class 로 역직렬화하는 과정에서 예외가 발생하지 않는다.

역직렬화된 maniaDBAPISearchResponse 가 null 이다.

언제 해당 상황이 발생하고 이 때 던지는 예외가 EmptyResultException 인 이유도 궁금해요~😊

💬 저도 스플릿이랑 비슷한 질문입니다 ㅎ.ㅎ

처음에 서비스 코드만 봤을 때는 EmptyResultException 이어서 "검색 결과가 없으면 예외가 발생하는건가~?" 라고 생각했는데, 테스트 코드를 보니 maniaDB에서 EMPTY_STRING ("") 이 반환될 때에 대한 예외 처리인 것 같더라구요!

Response 객체가 null이 되는 상황이 maniaDB에서 EMPTY_STRING을 반환할 때만인지, 아니면 다른 상황도 있는지 궁금해요!

그리구 빈 문자열만 해당한다면 언제 이런 상황이 생기는지도 궁금합니다

앗 그리구 UnregisteredSongException.EmptyResultException() 으로 통일해야 할 것 같아요~!

스플릿이 말한 플로우가 맞습니다.

아직 저 예외를 만나본 적은 없어요.
일단 getSongs() 나 getSingers() 같은 값들에 null 처리를 해준 이유는 응답에 DTO의 @XmlRootElement에 해당하는 XML 태그가 존재하지 않은 경우에 null이 들어가기 때문입니다.

final ManiaDBAPISearchResponse result = getResultFromManiaDB(searchUrl); 부분은 ManiaDB에서 응답 값을 ManiaDBAPISearchResponse 로 변경하고 있습니다.
이때, ManiaDB에서 응답하는 값에 rss 태그가 존재하지 않으면 result 값이 null 이 될 가능성이 있습니다.

예외를 경험한 것은 아니지만, 충분히 예측 가능한 예외라고 생각하여 null 체크를 해서 예외를 던지도록 했습니다. 좀 더 생각해보니 이 경우는 ManiaDBServerException 이라고 볼 수도 있겠네요 🤔

++ 상위 예외가 자동으로 import 됐나 보네요 ㅠㅠ 한 3번쯤 확인한 것 같은데 어째서!

splitCoding · 2023-07-29T11:43:46Z

backend/src/main/java/shook/shook/song/application/ManiaDBSearchService.java

+    private static final String SPECIAL_MARK_REGEX = "[^ㄱ-ㅎㅏ-ㅣ가-힣a-zA-Z0-9,. ]";
+    private final WebClient webClient;
+
+    public List<UnregisteredSongSearchResponse> searchSongs(final String searchWord) {


인자로 들어오는 searchWord가 비어있는 문자열일 때에 대한 검증이 필요할 것 같아요!! 😁

헉 꼼꼼한 예외 처리 👍
검색어가 null 일 때, 빈 문자열일 때 예외를 처리하는 로직을 추가했습니다.

seokhwan-an

전반적으로 코드를 작성한 것에서 노력이 느껴져서 최고였습니다. 👍 👍
몇가지 리뷰남겼습니다!!

seokhwan-an · 2023-07-29T15:33:53Z

backend/src/main/java/shook/shook/song/application/ManiaDBSearchService.java

+    private static final String MANIA_DB_API_URI = "/%s/?sr=song&display=%d&key=example&v=0.5";
+    private static final int SEARCH_SIZE = 100;
+    private static final String SPECIAL_MARK_REGEX = "[^ㄱ-ㅎㅏ-ㅣ가-힣a-zA-Z0-9,. ]";
+    private final WebClient webClient;


상수와 필드 사이에 줄 간격 두면 가독성이 더 좋아질 것 같아요!

seokhwan-an · 2023-07-29T15:38:48Z

backend/src/main/java/shook/shook/song/application/dto/UnregisteredSongSearchResponse.java

+    private static final String EMPTY_SINGER = "";
+    private static final String SINGER_DELIMITER = ", ";
+    private String title;
+    private String singer;
+    private String albumImageUrl;


여기도 위와 같은 리뷰입니다!

seokhwan-an · 2023-07-30T11:42:43Z

backend/src/main/java/shook/shook/song/application/dto/UnregisteredSongSearchResponse.java

+        final shook.shook.song.application.dto.maniadb.UnregisteredSongResponse unregisteredSongResponse) {
+        if (unregisteredSongResponse.getTrackArtists() == null
+            || unregisteredSongResponse.getTrackArtists().getArtists() == null) {


final 뒤의 UnregisteredSongResponse의 경로는 import문으로 뺄 수 있을 것 같습니다.

if문에서 가수 이름이 null인지 아닌지를 파악하는 것 같습니다. 먼저 unregisteredSongResponse의 SongtrackArtistsResponse(trackArtiests)객체가 null인지 파악하고 그 내부의 리스트가 null인지 파악하는데 혹시 데이터를 받아올 때 객체 내부의 값이null로 들어오는 경우가 있어서 나누어 놓으신건가요?

코드 작성하다가 놓친 것 같네용 수정했습니다!

넹 이해하신 내용이 맞습니다 😊 위 코멘트에 설명해놓았듯이 XML 태그가 없는 경우 필드에 null 값이 들어가게 됩니다. track_artist tag, artist tag 가 존재하지 않을 때는 모두 가수 이름이 없는 것으로 간주하고 EMPTY_STRING을 리턴하도록 했습니다 👍🏻

seokhwan-an · 2023-07-30T11:43:33Z

backend/src/main/java/shook/shook/song/application/dto/UnregisteredSongSearchResponse.java

+    }
+
+    private static String collectToString(
+        final shook.shook.song.application.dto.maniadb.UnregisteredSongResponse unregisteredSongResponse) {


여기도 import문으로 경로를 나타낼 수 있을 것 같습니다.

seokhwan-an · 2023-07-30T12:03:15Z

backend/src/main/java/shook/shook/song/application/dto/UnregisteredSongSearchResponse.java

+    public static UnregisteredSongSearchResponse from(
+        final shook.shook.song.application.dto.maniadb.UnregisteredSongResponse unregisteredSongResponse) {
+        if (unregisteredSongResponse.getTrackArtists() == null
+            || unregisteredSongResponse.getTrackArtists().getArtists() == null) {
+            return new UnregisteredSongSearchResponse(
+                unregisteredSongResponse.getTitle().trim(),
+                EMPTY_SINGER,
+                unregisteredSongResponse.getAlbum().getImage().trim()
+            );
+        }
+
+        final String singers = collectToString(unregisteredSongResponse);
+
+        return new UnregisteredSongSearchResponse(
+            unregisteredSongResponse.getTitle().trim(),
+            singers,
+            unregisteredSongResponse.getAlbum().getImage().trim()
+        );
+    }


현재 정적 팩토리 메소드를 이용하고 있는데 @AllArgsConstructor에 따로 접근제어를 하지 않았습니다. public으로 한 것이 의도한게 아니라면 private로 접근제어자를 추가하면 좋을 것 같습니다.

somsom13

정말 어마어마한 코드네요,,,, 특히 서비스 테스트코드 보고 감동받았습니다 😿 😿
이미 남겨져있는 코멘트들과 제가 궁금했던 내용들이 많이 겹쳐서 저는 코멘트가 많지 않습니당 ㅎ.ㅎ

고생하셨어요 베로쨩~~ 👍 🤗

somsom13 · 2023-07-30T12:29:32Z

backend/src/main/java/shook/shook/song/application/ManiaDBSearchService.java

+        if (Objects.isNull(result)) {
+            throw new EmptyResultException();
+        }


💬 저도 스플릿이랑 비슷한 질문입니다 ㅎ.ㅎ

처음에 서비스 코드만 봤을 때는 EmptyResultException 이어서 "검색 결과가 없으면 예외가 발생하는건가~?" 라고 생각했는데, 테스트 코드를 보니 maniaDB에서 EMPTY_STRING ("") 이 반환될 때에 대한 예외 처리인 것 같더라구요!

Response 객체가 null이 되는 상황이 maniaDB에서 EMPTY_STRING을 반환할 때만인지, 아니면 다른 상황도 있는지 궁금해요!

그리구 빈 문자열만 해당한다면 언제 이런 상황이 생기는지도 궁금합니다

somsom13 · 2023-07-30T12:32:08Z

backend/src/main/java/shook/shook/song/application/ManiaDBSearchService.java

+        if (Objects.isNull(result)) {
+            throw new EmptyResultException();
+        }


앗 그리구 UnregisteredSongException.EmptyResultException() 으로 통일해야 할 것 같아요~!

somsom13 · 2023-07-30T12:32:33Z

backend/src/main/java/shook/shook/song/application/ManiaDBSearchService.java

+            .onStatus(HttpStatusCode::is4xxClientError, (clientResponse) -> {
+                throw new UnregisteredSongException.ManiaDBClientException();


예외처리 기가막히고 신기하네요 👍

somsom13 · 2023-07-30T12:36:54Z

backend/src/main/java/shook/shook/song/application/dto/UnregisteredSongSearchResponse.java

+        if (unregisteredSongResponse.getTrackArtists() == null
+            || unregisteredSongResponse.getTrackArtists().getArtists() == null) {


💬 요 if문 내의 조건이 어떤걸 의미하는지 잘 나타낼 수 있게 따로 메서드로 분리하는건 어떻게 생각하시나요 ?.?
저는 처음 코드를 봤을 때 의미가 한 번에 이해되지 않았어서 남겨봅니다 ㅎ.ㅎ

좋은 피드백이네요!!! 😄
처음 코드를 보는 사람은 확실히 이해하기 힘든 부분인 것 같아요. 메서드 분리했습니다 ˙ᵕ˙

somsom13 · 2023-07-30T12:47:28Z

backend/src/main/java/shook/shook/song/ui/ManiaDBApiController.java

+
+    @GetMapping
+    public ResponseEntity<List<UnregisteredSongSearchResponse>> searchUnregisteredSong(
+        final @RequestParam("keyword") String searchWord


베로쨩~
맨 밑에 첨부한 이미지처럼
localhost:8080/songs/unregistered/search?keyword=

keyword에 빈 값을 넣어서 보냈을 때,
Connection refused: no further information 500 에러가 발생하더라구요!

여기에 대한 예외 처리와 테스트 코드도 있으면 좋을 것 같습니다 ㅎ.ㅎ

이 부분 빠졌었군요 😅 역시 코드 리뷰의 중요성...
빈 값 테스트 코드 추가했습니다!

- ManiaDB 관련 예외를 ExternalApiException 으로 분리

- EmptyResultException 클래스를 ExternalApiException 클래스 내부로 이동

- 외부 API 에 직접 의존하는 통합 테스트 삭제

feat: ManiaDB API 를 사용하여 등록되지 않은 노래 검색 기능 구현

d7f5560

- webclient 사용

Cyma-s added [ 🌙 BE ] 백엔드 크루들의 멋진 개발 이야기 하나둘셋 야! ✨ Feat 꼼꼼한 기능 구현 중요하죠 labels Jul 29, 2023

Cyma-s added this to the 노래 목록 및 상세 정보 조회 기능 구현 milestone Jul 29, 2023

Cyma-s self-assigned this Jul 29, 2023

Cyma-s requested review from splitCoding, somsom13 and seokhwan-an as code owners July 29, 2023 06:50

splitCoding reviewed Jul 29, 2023

View reviewed changes

seokhwan-an requested changes Jul 30, 2023

View reviewed changes

somsom13 requested changes Jul 30, 2023

View reviewed changes

Cyma-s added 12 commits July 31, 2023 21:52

refactor: 커스텀 예외 분리

ee3e38b

- ManiaDB 관련 예외를 ExternalApiException 으로 분리

chore: 컨트롤러 클래스 이름 변경

53443bd

config: prod group 설정 추가

6a7bf4e

refactor: UnregisteredSongException 삭제

89fd828

- EmptyResultException 클래스를 ExternalApiException 클래스 내부로 이동

feat: 검색어가 빈 문자열이나 null 인 경우 검증 로직 추가

2383468

style: 개행 추가

0577695

feat: 빈 값 예외 테스트 추가

237d01e

chore: 클래스 이름 변경

4d2ab0c

fix: 통합 테스트 삭제

e8eb415

- 외부 API 에 직접 의존하는 통합 테스트 삭제

chore: 클래스 이름 변경

2c5c53b

refactor: if 문 메서드 분리

7bd679a

Merge branch 'main' into feat/#127

ef58544

Cyma-s requested review from splitCoding, seokhwan-an and somsom13 August 9, 2023 06:27

	throw new UnregisteredSongException.ManiaDBClientException();
	throw new ExternalApiException.ManiaDBClientException();

	throw new UnregisteredSongException.ManiaDBServerException();
	throw new ExternalApiException.ManiaDBServerException();

	public class UnregisteredSongResponses {
	public class SearchedSongsWithManidaDbApiResponses {

		.onStatus(HttpStatusCode::is4xxClientError, (clientResponse) -> {
		throw new UnregisteredSongException.ManiaDBClientException();

		if (unregisteredSongResponse.getTrackArtists() == null
		\|\| unregisteredSongResponse.getTrackArtists().getArtists() == null) {

Feat/#127 ManiaDB API 를 사용하여 등록되지 않은 노래 검색 기능 구현 #153

Are you sure you want to change the base?

Feat/#127 ManiaDB API 를 사용하여 등록되지 않은 노래 검색 기능 구현 #153

Conversation

Cyma-s commented Jul 29, 2023

📝작업 내용

💬리뷰 참고사항

WebClient를 사용하는 것

테스트 주의 사항

#️⃣연관된 이슈

github-actions bot commented Jul 29, 2023 • edited Loading

Unit Test Results

splitCoding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cyma-s Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cyma-s Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seokhwan-an left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

somsom13 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 29, 2023 •

edited

Loading

Cyma-s Jul 31, 2023 •

edited

Loading

Cyma-s Jul 31, 2023 •

edited

Loading

somsom13 left a comment •

edited

Loading