[Master] 5주차 과제 Review PR #36
Conversation
oauth, security와 관련된 dependency를 추가하였습니다. 관련 이슈: #2
UserEntity와 Repository를 추가 하였습니다. 관련 이슈: #2
oauth 로그인을 위해 csrf를 차단 (rest api이기 때문에 jwt로 검증을 함), formlogin을 차단(사용하지 않음), httpBasic을 차단(사용하지 않음) 하였습니다. session에 stateless 속성을 부여하였습니다. 관련 이슈: #2
서브 모듈을 추가하여 민감한 정보를 관리합니다. 관련 이슈:#2
서브 모듈을 최신화 하여 api 키를 업데이트하였습니다 관련 이슈: #2
.gitignore 파일에 application.properties를 넣지안았는데도 추적되지 않음. 그 문제를 해결 관련 이슈: #2
authentication을 구현하였습니다. kakao만 한다는 가정하에 kakao를 직접 넣었습니다. 관련 이슈: #2
더미 데이터여서 그냥 만들었었는데, 피드백에 따라 변경하기로 결정
패키지 명은 모두 소문자로 한다는 컨벤션에 맞추어 소문자로 변경
…tep3/Team7_BE into feat/#5-videoAlias
해당 기능 최초 구현, 구현 방식이나 데이터의 추가가 개선되어야 할 것 같다
- PlaceService 인터페이스 제거, getCategories()에서 List 반환 - Place id의 nullable=false 제거 - CategoryListDTO -> CategoryListResponse로 이름 변경해서 직관화함 관련 이슈: #7
사용 지점과 최대한 가깝게 변수 선언 시점을 변경
메서드 체이닝을 통해 필요 없는 변수를 삭제
PlaceTime VO와 Menu VO를 Place 클래스에 추가후, 테스트 코드까지 수정하였습니다. 관련 이슈: #7
id를 제외한 필드를 사용하여 생성자를 만들고, 이를 통해 테스트를 진행하기 위하여 @nonnull과 @requiredargsconstructor을 사용하여 리펙토링
추후에 통합 테스트 필요 ( + POSTMAN을 이용한 API 테스트도 필요 )
/login 에 authenicated가 적용되어 있어 무한 리다이렉트 되던 문제를 해결하였습니다. 관련 이슈:#2
oauth 로그인 시 유저 정보를 db에 저장 관련 이슈: #2
패키지를 application-domain-persistence-presentaion 구조로 설정하였습니다. List<Category> 대신, CategoryInfo를 구현해서 dto로 적용시켰습니다. 관련 이슈: #7
Place 클래스에서 주소와 위도, 경도를 각각 Address와 Coordinate VO로 추출했습니다. 관련 이슈: #7
추후에 통합 테스트 필요 ( + POSTMAN을 이용한 API 테스트도 필요 )
record 컨벤션, dto 컨벤션 반영 및 패키지 구조 변경
…videoAlias # Conflicts: # src/test/java/team7/inplace/VideoTest/domain/VideoTest.java # src/test/java/team7/inplace/VideoTest/repository/VideoRepositoryTest.java # src/test/java/team7/inplace/VideoTest/service/VideoServiceTest.java
submodule이 내 private repository라서 사용하지 않기로 정함 고로 삭제 관련 이슈: #2
…tep3/Team7_BE into feat/#2-authorization
Feat/#7 place - 카테고리, 인플루언서 필터링을 적용시켰어요!
…rization [Feat] #2 authorization을 구현했어요.
…swaggerAndConvention [Refactor] #29 Swagger 코드 스타일과 패키지 네이밍 컨벤션을 맞춰 보아요
…tep3/Team7_BE into feat/#9-summarize-youtube
[Feat] #8 CI/CD 관련 수정했어요
…uencer [Feat] #20 로그인 없을 때 인플루언서 반환을 구현했어요
…rize-youtube [Feat]#9 유튜브에서 장소 정보를 크롤링해요
[Weekly] 2주차 과제 PR
[�Develop] 3주차 과제 PR
There was a problem hiding this comment.
안녕하세요~ 2번째 PR 잘 제출해주셨네요!
커밋 수를 보니 열심히 작업하신 것 같습니다. ㅎㅎ 그만큼 리뷰드리고 싶은 내용이 많네요. 한 번 확인해보시고, 다음 주 PR에 반영하는 것 고려해보세요!
질문 1.
좋은 고민이네요 👍
- Facade 패턴은 여러 서비스의 로직을 조합해서, 단순한 인터페이스를 제공하는 역할을 합니다. 그러므로 파사드 클래스는 비즈니스 로직이 있는 Service Layer에 위치하는 것이 맞습니다.
- CrawlingService는 단순히 Crawling만을 한다는 의미가 있으므로, CrawlingService에서 파사드를 호출하는 것은 부적절해 보이네요~ 별도의 파사드 클래스에서 트랜잭션을 처리하고, crawlingService와 파사드 클래스를 이용하는 별도의 서비스를 또 만들면 해결될거라고 생각이 되네요.
질문 2.
- 해당 쿼리를 수행하는데 필요한 필드를 눈으로 확인해보고, 해당 필드들에 복합 인덱스를 적용하시면 됩니다. 인덱스를 걸어서 성능이 개선되었는가? 를 직접 확인해보고 싶으시다면, 테스트 코드를 작성해서 직접 확인하실 수도 있습니다.
질문 3.
- 현재처럼 계층간의 dto를 잘 관리하는 컨셉을 잡으셨다면 이렇게 하는게 맞고, 최선이라고 생각이 듭니다.
- Influencer(Entity) -> InfluencerInfo(서비스 DTO) -> InfluencerResponse(컨트롤러 DTO) -> InfluencerListResponse(최종 DTO) 이렇게 되고 있네요~
- 저희같은 경우는, 멘토링에서 설명드렸던 것 처럼 컨트롤러 dto와 서비스 dto를 분리하지는 않기 때문에, Influencer(Entity) -> InfluencerDto(서비스 레이어에서 왔다갔다 하는 DTO) -> InfluencerResponse(Controller로 나가는 응답) 이렇게 될 것 같네요.
2번째 PR 모두 고생 많으셨고, 리뷰드린 내용 한번씩 확인해보세요~ 남은 일감, 일정 확인 잘 해보시고 다음주도 화이팅입니다 🔥
특별히 큰 문제는 없어, 바로 Approve 하겠습니다.
- 환경 변수도 그렇고 서브모듈 활용, CI/CD 야무지게 해주셨더라구요, 정말 고생하셨습니다.
추가적으로, intellij의 코드 Reformat 기능과, optimize import 기능도 한번 찾아보고 이용해보세요~!!
| this.apiKey = apiKey; | ||
| } | ||
|
|
||
| public List<RawVideoInfo> getVideos(String playListId, String finalVideoUUID) { |
There was a problem hiding this comment.
YoutubeClient는 단순히 api 호출을 수행하고, 응답 dto를 그대로 다른 서비스에게 전달하고, 다른 서비스에서 응답을 가공해서 사용하면 어떨까요?
getVideos() 메서드처럼 api호출한 결과를 내부에서 가공해버리면, 만약에 해당 api 호출 결과를 다른 서비스에서도 사용하려면, 동일한 url이라도 다른 메서드를 만들어서 응답을 만들어주어야할 것 같아요~
다른 API 활용도 마찬가지이니, 한 번 고려해보세요!
| @@ -0,0 +1,8 @@ | |||
| package team7.inplace.crawling.client.dto; | |||
|
|
|||
| public record RawVideoInfo( | |||
There was a problem hiding this comment.
Info라는 suffix보다는 Dto라는 suffix가 조금 더 dto를 명시적으로 나타낼 수 있는 것 같아요.
Info는 사실 그 어떠한 클래스 뒤에붙어도 이상하지는 않은 명칭이라 지양하는 편이에요.
이미 이렇게 컨벤션을 정하셨다면, 아하 그렇구나 하고 넘어가셔도 상관은 없습니다!
| ChannelType(String code) { | ||
| this.code = code; | ||
| } | ||
|
|
||
| public String getCode() { | ||
| return code; | ||
| } |
There was a problem hiding this comment.
@Getter, @RequiredArgsConstructor 를 사용하면 코드가 간결해지겠네요~
물론 getCode() 처럼 명시적으로 getter를 선언해주어도 나름대로의 장점이 있습니다. (다른 필드가 존재할 경우)
다만 현재는 code라는 필드 하나밖에 없어서, lombok을 활용하셔도 좋겠다는 의미에요!
| public String getChannelTypeCode() { | ||
| return channelType.getCode(); | ||
| } |
There was a problem hiding this comment.
channelType은 반드시 null이 아니어야겠군요~
channelType이 nullable이라면 NPE가 발생할 위험이 있네요
| List<InfluencerInfo> influencersDtoList = influencerService.getAllInfluencers(); | ||
| List<InfluencerResponse> influencers = influencersDtoList.stream() | ||
| .map(InfluencerResponse::from) | ||
| .toList(); | ||
| InfluencerListResponse response = new InfluencerListResponse(influencers); |
There was a problem hiding this comment.
요런 코드를 작성하실 때, 변수 선언을 통한 가독성 향상과, 한줄 요약을 통한 가독성 향상의 관점에서 고민하며 작성하시면 좋습니다! (정답은 없어요)
| public ResponseEntity<List<VideoResponse>> readVideos( | ||
| HttpServletRequest request, | ||
| @RequestParam(name = "influencer", required = false) List<String> influencers, | ||
| @ModelAttribute VideoSearchParams searchParams, | ||
| @RequestParam(defaultValue = "0", required = false) int page, | ||
| @RequestParam(defaultValue = "10", required = false) int size | ||
| ) { | ||
| // Authorization 헤더 추출 | ||
| String authHeader = request.getHeader("Authorization"); | ||
| // 토큰 존재 여부 검사 | ||
| if(authHeader != null && authHeader.startsWith("Bearer ")) { | ||
| // 토큰 유효성 검사 | ||
|
|
||
| // 토큰이 있는 경우 | ||
| return readByInfluencer(influencers); | ||
| } | ||
|
|
||
| // 토큰이 없는 경우 | ||
| return readBySurround(searchParams, page, size); |
There was a problem hiding this comment.
인증/인가 로직을 어떻게 개선할 수 있을까요?
다른 클래스에서 공통으로 처리하는 방식이 있을 수 있고,
예전에 배웠던 ArgumentResolver를 활용해도 될 것 같고,, 방법은 다양하니 고민해보세요!
There was a problem hiding this comment.
아 이 부분은 제가 임시로 만든 부분이고, 실제 로직은 Security 부분 구현한 팀원이 쿠키에서 Authorization 관련 헤더를 추출해서 User 객체를 만들어주는 부분까지 구현해서 이 부분 적용할 예정입니다!
| private ResponseEntity<List<VideoResponse>> readByInfluencer(List<String> influencers){ | ||
| List<VideoInfo> videoInfos = videoService.getByVideosInfluencer(influencers); | ||
| List<VideoResponse> videoResponses = videoInfos.stream().map(VideoResponse::from).toList(); | ||
| return new ResponseEntity<>(videoResponses, HttpStatus.OK); | ||
| } | ||
|
|
||
| private ResponseEntity<List<VideoResponse>> readBySurround(VideoSearchParams searchParams, int page, int size) { | ||
| Pageable pageable = PageRequest.of(page, size); | ||
| List<VideoInfo> videoInfos = videoService.getVideosBySurround(searchParams, pageable); | ||
| List<VideoResponse> videoResponses = videoInfos.stream().map(VideoResponse::from).toList(); | ||
| return new ResponseEntity<>(videoResponses, HttpStatus.OK); | ||
| } |
There was a problem hiding this comment.
별도의 Service 에서 조건 검사 --> 분기 처리를 하면, Controller에서는 단순히 요청을 Service에 넘겨주기만 하면 되겠네요!
There was a problem hiding this comment.
Controller를 보다 단순하게 만들어보라는 말씀이시죠? 알겠습니다.
| import java.util.List; | ||
|
|
||
|
|
||
| public interface VideoControllerApiSpec { |
There was a problem hiding this comment.
별도의 인터페이스에서 swagger 명세 처리를 하셨군요~
이런 방법도 가능한 방법이지만, 저희는 코드 가독성을 해치지 않는다면 Controller에 그냥 붙이기도 해요. (operation의 summary, description은 오히려 주석 역할을 대신해주는 느낌이라 개인적으로는 가독성이 좋아지는 것 같더라구요)
어떤 방법을 사용하는 것은 선택입니다!
| jpa: | ||
| hibernate: | ||
| ddl-auto: create-drop |
| @ActiveProfiles("test") | ||
| class YoutubeClientTest { | ||
| @Autowired | ||
| public YoutubeClient youtubeClient; |
There was a problem hiding this comment.
외부 api를 테스트하기 위한 방법에 대해 알아보세요~ (중요한 내용입니다)
- 만약 api를 사용하는 데 횟수 제한이 걸려있다면?
- 드물겠지만, api가 뻗었다면?
- 원하는 대로 응답을 쉽게 제어하여 테스트하고 싶다면?
https://www.youtube.com/watch?v=bJfbPWEMj_c
한 번 참고해보세요~
✨ 작업 내용
✨ 질문사항
DB 저장 로직에 대한 질문 드립니다.현재 crawling/application에 있는 서비스 참고 부탁드립니다.
먼저 매 스케쥴 단위마다 채널 하나당 0...n개의 비디오가 새롭게 추가될 수 있습니다.
n개가 추가될 경우, n개의 place정보도 동일하게 추가되어야합니다.
크롤링 자체를 transaction으로 묶기에는, 외부 API호출이 많이 있고 DB 커넥션을 많이 잡고 있어 Video와 Place정보 저장시에만 Transaction을 묶으려고하고 있습니다.
그래서 결과적으로 한 채널 단위로 video와 place정보를 저장하는 트랜젝션을 실행하려고 합니다.
일단 Facade패턴을 적용해 VideoService와, PlaceService를 갖는 파사드를 생각중인데 이 파사드 클래스는 어디에 위치하는게 좋을지 모르겠습니다. 또, CrawlingService 또한 ServiceLayer인데 다른 서비스레이어의 파사드 클래스를 호출하는게 맞나 의구심이 듭니다.
이 결론적으로 이 트랜젝션을 어디에서 처리해야할 지 모르겠습니다.
이에 대해서 조언 부탁드리겠습니다.
PlaceRepository에서 거리계산하는것을 sql문으로 짜서 db에서 계산하도록했습니다. 지금은 문제가 없지만 나중에 place가 많아지면 db 성능저하가 우려되어 인덱싱같은걸 어떻게 적용할지 고민중입니다. 이에대해 조언해주시면 감사하겠습니다
InfluencerRepository에서 반환 형식이 {"influencers": [{
},{},..]} 이런 식이라 dto가 3개가 됐는데 더 좋은 방법 있는지 궁금합니다! InfluencerResponse를 생략하자니 dto 분리의 의미가 없는 거 같아 현재는 3개로 두었습니다.