Conversation
…detail-search # Conflicts: # build.gradle
seongjunnoh
left a comment
seongjunnoh
left a comment
There was a problem hiding this comment.
고생하셨습니다! 사소한 리뷰하나 남겼는데 확인부탁드립니다!
| //책 상세검색 결과 조회 | ||
| @GetMapping("/books/{isbn}") | ||
| public BaseResponse<GetBookDetailSearchResponse> getBookDetailSearch(@PathVariable("isbn") | ||
| @Pattern(regexp = "\\d{13}") final String isbn, | ||
| @UserId final Long userId) { |
There was a problem hiding this comment.
오 GET api의 쿼리 파라미터에 대한 validation을 하는 코드는 처음보는데 좋은것 같습니다!
클래스 레벨에서의 @validated 선언 후 bean validation 하는 것도 좋습니다!
src/main/java/konkuk/thip/common/exception/handler/GlobalExceptionHandler.java
Outdated
Show resolved
Hide resolved
| private int getRecruitingReadCount(Book book) { | ||
| // 해당책으로 피드에 글 작성 | ||
| // 해당책에 대해 모임방 참여 | ||
| // 둘 중 하나라도 부합될 경우 카운트, 중복 카운트 불가 | ||
|
|
||
| // 이 책으로 만들어진 방에 참여한 사용자 ID 집합 | ||
| Set<Long> roomParticipantUserIds = userQueryPort.findUserIdsParticipatedInRoomsByBookId(book.getId()); | ||
| // 이 책으로 피드를 쓴 사용자 ID 집합 | ||
| Set<Long> feedAuthorUserIds = feedQueryPort.findUserIdsByBookId(book.getId()); | ||
|
|
||
| // 합집합(중복 제거) | ||
| Set<Long> combinedUsers = new HashSet<>(); | ||
| combinedUsers.addAll(roomParticipantUserIds); | ||
| combinedUsers.addAll(feedAuthorUserIds); | ||
| return combinedUsers.size(); | ||
|
|
||
| } |
| public class BookSearchService implements BookSearchUseCase { | ||
|
|
||
| private final SearchBookQueryPort searchBookQueryPort; | ||
| private final BookApiQueryPort bookApiQueryPort; | ||
| private final RoomQueryPort roomQueryPort; | ||
| private final UserQueryPort userQueryPort; | ||
| private final FeedQueryPort feedQueryPort; | ||
| private final SavedQueryPort savedQueryPort; | ||
| private final BookCommandPort bookCommandPort; | ||
| private final UserCommandPort userCommandPort; |
There was a problem hiding this comment.
이렇게 여러 도메인의 Port 에 대한 의존성이 많아지는 것이 싫다면, BookQueryPort 내부에 여러 도메인들(= 테이블들)을 querydsl 을 활용해 join 해서 큰 단위의 데이터들을 조회하는 식으로 구현하는 것도 하나의 방법이 될 수 있습니다!
-> api의 response에 해당하는 데이터 포맷을 service에서 맞추냐, 아니면 Port 하위의 영속성 adapter에서 맞추냐 의 취향 차이 + DB를 여러번 왔다갔다 vs 한번에 비싼 join 연산 의 차이? 인것 같습니다
지금 코드도 충분히 Good 입니다
| @SpringBootTest | ||
| @AutoConfigureMockMvc(addFilters = false) | ||
| @ActiveProfiles("test") | ||
| class BookDetailSearchControllerTest { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/main/java/konkuk/thip/common/exception/handler/GlobalExceptionHandler.java (2)
28-28: 이전 리뷰 제안이 훌륭하게 반영되었습니다
@RequiredArgsConstructor를 사용하여 의존성 주입을 깔끔하게 처리했습니다.
123-150: 전략 패턴 구현이 완벽합니다이전 리뷰에서 제안된 전략 패턴이 정확히 구현되어 있으며, 폴백 처리도 적절합니다. 다만 첫 번째 위반만 처리하는 부분에 대한 고려가 필요할 수 있습니다.
// 만약 모든 위반을 처리해야 한다면 다음과 같이 개선할 수 있습니다: Set<ConstraintViolation<?>> violations = e.getConstraintViolations(); List<String> errorMessages = violations.stream() .map(violation -> { // 전략 패턴 적용하여 각 위반에 대한 메시지 생성 }) .collect(Collectors.toList());하지만 현재 구현도 대부분의 케이스에서 충분합니다.
🧹 Nitpick comments (1)
src/main/java/konkuk/thip/common/exception/validation/SizeViolationStrategy.java (1)
12-12: 정적 임포트 일관성 개선 필요
API_INVALID_SIZE만 정적 임포트하고 있는데, line 27에서는ErrorCode.API_INVALID_SIZE로 사용하고 있어 일관성이 부족합니다.다음과 같이 수정하여 일관성을 맞추세요:
- ErrorCode.API_INVALID_SIZE, + API_INVALID_SIZE,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/main/java/konkuk/thip/book/adapter/out/persistence/BookCommandPersistenceAdapter.java(1 hunks)src/main/java/konkuk/thip/book/application/port/out/BookCommandPort.java(1 hunks)src/main/java/konkuk/thip/book/application/service/BookSearchService.java(3 hunks)src/main/java/konkuk/thip/common/exception/handler/GlobalExceptionHandler.java(3 hunks)src/main/java/konkuk/thip/common/exception/validation/ConstraintViolationResult.java(1 hunks)src/main/java/konkuk/thip/common/exception/validation/ConstraintViolationStrategy.java(1 hunks)src/main/java/konkuk/thip/common/exception/validation/PatternViolationStrategy.java(1 hunks)src/main/java/konkuk/thip/common/exception/validation/SizeViolationStrategy.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/konkuk/thip/common/exception/validation/ConstraintViolationResult.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/konkuk/thip/book/application/port/out/BookCommandPort.java
- src/main/java/konkuk/thip/book/adapter/out/persistence/BookCommandPersistenceAdapter.java
- src/main/java/konkuk/thip/book/application/service/BookSearchService.java
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/main/java/konkuk/thip/common/exception/validation/SizeViolationStrategy.java (1)
src/main/java/konkuk/thip/common/exception/validation/PatternViolationStrategy.java (1)
Component(13-30)
src/main/java/konkuk/thip/common/exception/validation/PatternViolationStrategy.java (1)
src/main/java/konkuk/thip/common/exception/validation/SizeViolationStrategy.java (1)
Component(15-32)
🔇 Additional comments (3)
src/main/java/konkuk/thip/common/exception/validation/ConstraintViolationStrategy.java (1)
7-10: 전략 패턴 인터페이스 설계가 우수합니다인터페이스가 단일 책임 원칙을 잘 따르고 있으며,
supports와handle메서드의 분리로 명확한 역할을 정의하고 있습니다. 이전 리뷰에서 제안된 전략 패턴이 깔끔하게 구현되었습니다.src/main/java/konkuk/thip/common/exception/validation/SizeViolationStrategy.java (1)
15-31: 전략 구현이 잘 되어 있습니다
@Size어노테이션 위반에 대한 처리 로직이 명확하고, 한국어 에러 메시지도 적절합니다. 전략 패턴의 구현이 올바르게 되어 있습니다.src/main/java/konkuk/thip/common/exception/validation/PatternViolationStrategy.java (1)
13-30: 일관성 있는 전략 구현입니다
SizeViolationStrategy와 동일한 패턴으로 구현되어 있어 코드 일관성이 우수합니다. 정적 임포트도 올바르게 사용되었습니다.
buzz0331
left a comment
buzz0331
left a comment
There was a problem hiding this comment.
복잡한 로직 구현하느라 수고 많으셨습니다~ 리뷰 확인 부탁드릴게욥
| public String searchBook(String keyword, int start) { | ||
| return callNaverApi(keyword, start, false); | ||
| } | ||
|
|
||
| public String detailSearchBook(String isbn) { | ||
| return callNaverApi(isbn, null, true); | ||
| } | ||
|
|
||
| private String callNaverApi(String keyword, Integer start, boolean isDetail) { | ||
| String query = keywordToEncoding(keyword); | ||
| String url = buildSearchApiUrl(query, start); | ||
| String url = isDetail ? buildDetailSearchApiUrl(query) : buildSearchApiUrl(query, start); | ||
|
|
||
| Map<String, String> requestHeaders = new HashMap<>(); | ||
| requestHeaders.put("X-Naver-Client-Id", clientId); | ||
| requestHeaders.put("X-Naver-Client-Secret", clientSecret); | ||
|
|
||
| return get(url,requestHeaders); | ||
| return get(url, requestHeaders); | ||
| } |
There was a problem hiding this comment.
p3: 현재 callNaverApi의 호출부 쪽에서 3번째 boolean 변수의 의미가 와닿지 않아서 리팩토링이 필요한 부분이라고 생각하는데 추후에 전략 패턴을 도입해봐도 좋을 것 같아요.
There was a problem hiding this comment.
음 책검색, 책 상세 검색에서 네이버 api를 사용하다보니 공통 메서드로 추출해서 3번째 boolean 변수는 상세조회 검색인지를 뜻하는 값입니다!
There was a problem hiding this comment.
callNaverApi() 내부에서는 boolean 파라미터가 어떤 역할을 하는지 이해되지만, searchBook()이나 detailSearchBook()처럼 호출하는 입장에서는 세 번째 인자가 어떤 의미인지 조금 직관적이지 않을 수 있다는 생각이 들었습니다!
현재 구조가 전체적으로 책 검색 api와 책 상세 검색 api가 유사한 부분과 미세하게 다른 흐름으로 나뉘는 것 같아서 전략 패턴을 고려해보는 것도 좋을 것 같아 리뷰 남겨보았습니다 ㅎ
src/main/java/konkuk/thip/book/adapter/out/api/NaverBookXmlParser.java
Outdated
Show resolved
Hide resolved
| private static Element getFirstChannel(String xml) throws Exception { | ||
| DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); | ||
| factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); | ||
| factory.setFeature("http://xml.org/sax/features/external-general-entities", false); | ||
| factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false); | ||
| factory.setExpandEntityReferences(false); | ||
| DocumentBuilder builder = factory.newDocumentBuilder(); | ||
| InputSource is = new InputSource(new StringReader(xml)); | ||
| Document doc = builder.parse(is); | ||
|
|
||
| NodeList channelNodes = doc.getElementsByTagName("channel"); | ||
| if (channelNodes.getLength() > 0) { | ||
| return (Element) channelNodes.item(0); | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
p3: 메서드가 좀 커서 다음과 같이 메서드로 좀 분리하는거 어떤가욤
private static Document parseXml(String xml) throws Exception {
DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
factory.setFeature("http://xml.org/sax/features/external-general-entities", false);
factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
factory.setExpandEntityReferences(false);
DocumentBuilder builder = factory.newDocumentBuilder();
return builder.parse(new InputSource(new StringReader(xml)));
}
private static Element getFirstChannel(String xml) throws Exception {
Document doc = parseXml(xml);
NodeList channelNodes = doc.getElementsByTagName("channel");
return (channelNodes.getLength() > 0) ? (Element) channelNodes.item(0) : null;
}There was a problem hiding this comment.
xml 보안관련 로직은 따로 함수로 추출해보도록 하겠습니당
| private int getRecruitingReadCount(Book book) { | ||
| // 해당책으로 피드에 글 작성 | ||
| // 해당책에 대해 모임방 참여 | ||
| // 둘 중 하나라도 부합될 경우 카운트, 중복 카운트 불가 | ||
|
|
||
| // 이 책으로 만들어진 방에 참여한 사용자 ID 집합 | ||
| Set<Long> roomParticipantUserIds = userQueryPort.findUserIdsParticipatedInRoomsByBookId(book.getId()); | ||
| // 이 책으로 피드를 쓴 사용자 ID 집합 | ||
| Set<Long> feedAuthorUserIds = feedQueryPort.findUserIdsByBookId(book.getId()); | ||
|
|
||
| // 합집합(중복 제거) | ||
| Set<Long> combinedUsers = new HashSet<>(); | ||
| combinedUsers.addAll(roomParticipantUserIds); | ||
| combinedUsers.addAll(feedAuthorUserIds); | ||
| return combinedUsers.size(); | ||
|
|
||
| } |
| @@ -41,7 +49,10 @@ public enum ErrorCode implements ResponseCode { | |||
| BOOK_NAVER_API_RESPONSE_ERROR(HttpStatus.BAD_REQUEST, 80005,"네이버 API 응답에 실패하였습니다."), | |||
There was a problem hiding this comment.
요거는 HttpStatus.INTERNAL_SERVER_ERROR가 맞아보이네욤
| @Override | ||
| public Set<Long> findUserIdsByBookId(Long bookId) { | ||
| QUserRoomJpaEntity userRoom = QUserRoomJpaEntity.userRoomJpaEntity; | ||
| QRoomJpaEntity room = QRoomJpaEntity.roomJpaEntity; | ||
|
|
||
| return new HashSet<>( | ||
| jpaQueryFactory | ||
| .select(userRoom.userJpaEntity.userId) | ||
| .distinct() | ||
| .from(userRoom) | ||
| .join(userRoom.roomJpaEntity, room) | ||
| .where(room.bookJpaEntity.bookId.eq(bookId)) | ||
| .fetch() | ||
| ); |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/konkuk/thip/book/adapter/out/api/NaverBookXmlParser.java (1)
51-88: parseBookDetail 메서드에서 null 반환 문제를 해결해주세요.메서드 끝에서
null을 반환하는 부분이 여전히 남아있어 이전 리뷰에서 지적된 문제가 해결되지 않았습니다. 호출하는 코드에서NullPointerException이 발생할 수 있습니다.다음과 같이 수정하세요:
} catch (Exception e) { throw new ExternalApiException(BOOK_NAVER_API_PARSING_ERROR); } - return null; + throw new ExternalApiException(BOOK_NAVER_API_PARSING_ERROR);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/konkuk/thip/book/adapter/out/api/NaverBookXmlParser.java(2 hunks)src/main/java/konkuk/thip/common/exception/ExternalApiException.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/konkuk/thip/book/adapter/out/api/NaverBookXmlParser.java (1)
src/main/java/konkuk/thip/common/exception/ExternalApiException.java (1)
ExternalApiException(5-17)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (3)
src/main/java/konkuk/thip/book/adapter/out/api/NaverBookXmlParser.java (3)
19-49: parseBookList 메서드 리팩토링이 잘 되었습니다.메서드명이 더 명확해졌고, 헬퍼 메서드 추출과 향상된 for-each 루프 사용으로 코드 가독성이 개선되었습니다. ExternalApiException 사용도 적절합니다.
90-98: XML 보안 설정이 잘 적용되었습니다.이전 리뷰에서 지적된 XXE 공격 방지를 위한 보안 설정이 모두 올바르게 적용되었습니다.
100-114: 헬퍼 메서드 추출로 코드 구조가 개선되었습니다.이전 리뷰에서 제안된 메서드 분리가 잘 적용되어 코드의 가독성과 재사용성이 향상되었습니다.
| private final ErrorCode errorCode; | ||
|
|
||
| public ExternalApiException(ErrorCode errorCode) { | ||
| super(errorCode.getMessage()); | ||
| this.errorCode = errorCode; | ||
| } | ||
| public ExternalApiException(ErrorCode errorCode, Exception e) { | ||
| super(errorCode.getMessage(), e); | ||
| this.errorCode = errorCode; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
ErrorCode에 대한 getter 메서드를 추가해주세요.
errorCode 필드가 private final로 선언되어 있지만 getter 메서드가 없어서 외부에서 에러 코드에 접근할 수 없습니다. 에러 처리나 로깅에서 에러 코드 정보가 필요할 수 있습니다.
다음과 같이 getter 메서드를 추가하세요:
public ExternalApiException(ErrorCode errorCode, Exception e) {
super(errorCode.getMessage(), e);
this.errorCode = errorCode;
}
+
+ public ErrorCode getErrorCode() {
+ return errorCode;
+ }
}🤖 Prompt for AI Agents
In src/main/java/konkuk/thip/common/exception/ExternalApiException.java around
lines 7 to 17, the private final field errorCode lacks a getter method,
preventing external access to the error code. Add a public getter method named
getErrorCode() that returns the errorCode field to enable error handling and
logging to access this information.
|
3 participants
#️⃣ 연관된 이슈
📝 작업 내용
@Size,@Pattern예외처리 하기위하여handleConstraintViolationException를 작성했습니다.NaverBookXmlParser,NaverApiUtil에 책 상세 조회가 추가되면서 기존에 있던 책 조회와의 중복 코드가 많아 공통 메서드는 추출해서 사용하는 식으로 리팩토링 했습니다.BookSearchUseCase로 작성했습니다.Controller -> BookSearchUseCase를 구현한 BookSearchService.searchDetailBooks -> 도메인별 Command/Query Port -> 각 포트를 구현한 Adapter -> Service에서 BookDetailSearchResult반환 -> Controller에서 Result를 Response dto로 변환후 반환bookDetailSearchUrl환경변수 처리하여 yml 노션에 업데이트했습니다.📸 스크린샷
아래의 테스트 데이터를 활용해 정상적으로 데이터가 반환되는 것을 확인했습니다.
💬 리뷰 요구사항
📌 PR 진행 시 이러한 점들을 참고해 주세요
Summary by CodeRabbit
신규 기능
버그 수정
개선 및 리팩터링
테스트