-
Notifications
You must be signed in to change notification settings - Fork 0
Refactor: Clean up Docker, remove Elasticsearch, and fix search functionality. #219
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
base: developer
Are you sure you want to change the base?
Conversation
dnwls16071
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다.
수정할 부분이 많아보이는데 한 번 검토해주세요.
| @Configuration | ||
| public class ModelMapperConfig { | ||
| @Bean | ||
| public ModelMapper modelMapper() { | ||
| ModelMapper modelMapper = new ModelMapper(); | ||
| modelMapper.getConfiguration() | ||
| .setFieldAccessLevel(org.modelmapper.config.Configuration.AccessLevel.PRIVATE) | ||
| .setFieldMatchingEnabled(true); | ||
|
|
||
| return modelMapper; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ModelMapper의 필요성이 느껴진 이유가 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
convter는 전에 말씀주신대로 컨버터를 작성하신분만 사용가능하기 떄문에 재사용이 어렵다고 생각되었습니다.
또한, 하나씩 모든 객체를 매핑하기 위해서 작성하는것 보다 DTO에서 entity로 변환할 수 있는 ModelMapper를 사용하는것이 생산성을 높일 수 있다고 생각했습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
entity -> (service layer) dto -> (presentation layer) dto 과정에서 ModelMapper가 굳이 필요하진 않을 것 같다고는 생각합니다.
그 이유는 저희 프로젝트에서 복잡한 매핑을 맺는 부분은 없어보이고 ModelMapper 라이브러리를 사용할만한 곳이 보이지 않았기 때문인데 매핑 관계가 복잡한 테이블 예시가 있으시다면 사용해도 될 것 같습니다만 그게 아니라면 DTO 간에 변환으로만으로도 충분히 커버가 가능할 것 같은데 좋은 고민이었습니다. 같이 고민을 좀 해보시죠.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
우선 지금 사용하는곳이 없어서 제외했습니다.
추후 필요하게되면 다시 추가 하겠습니다!
| @RequestParam(required = false) Long cursor, | ||
| @RequestParam(defaultValue = "20") Long size, | ||
| @AuthenticationPrincipal UserDetails userDetails) { | ||
| public ResponseEntity<FilterResponse<HomeListResponse>> getDomesticFilter(@RequestBody ExhibitFilterRequest dto, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GET을 제외한 HTTP Method에서 반환 타입을 모두 낼 필요는 없을 것 같은데 이건 프론트엔드 측과 합의된 사항일까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아직 합의 된게 없습니다.
같은 코드가 너무 많아서 브렌치 새로 작성한 후 수정 진행하겠습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GET을 제외한 나머지 경우에는 상태코드만 명확히 전달해줘도 코드가 충분히 감소할 것 같아요.
이건 정답이 있는 것은 아니긴한데 빠른 시일 내로 결정지었으면 하네요.
| @AuthenticationPrincipal UserDetails userDetails) { | ||
| Long userId = getUserId(userDetails); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UserDetails에서 userId를 꺼내 쓰는 방식이 아무래도 공통적으로 사용되는 부분으로 보여지는데 재사용성을 위해서라면 별도의 어노테이션으로 달아서 현재 인증 객체를 참조하는 것은 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
커스텀 어노테이션으로 만들었습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
userId만 있다면 지금처럼 쓰셔도 될 것 같고 다른 Claims의 일부를 파싱해서 써야 한다면 @LoginUser 등 다른 네이밍도 검토할 수 있을 것 같아요.
| common = {CommonError._BAD_REQUEST, CommonError._UNAUTHORIZED}, | ||
| home = {HomeError._HOME_INVALID_DATE_RANGE, HomeError._HOME_UNRECOGNIZED_REGION, HomeError._HOME_EXHIBIT_NOT_FOUND} | ||
| ) | ||
| @PostMapping("/filter") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RESTful API의 규약상 "행위"보다는 "리소스"에 초점을 맞추는 것이 좋은데 행위에 초점을 맞춘 것으로 보여요.
다른 URL Path를 고려해보는 것은 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SearchController 외 Search 관련 된것을 제외 후 exhibit 하위로 이전하였습니다.
| @AuthenticationPrincipal UserDetails userDetails) { | ||
| Long userId = getUserId(userDetails); | ||
| FilterResponse exhibits = homeService.getFilterExhibit(dto, size, cursor,userId); | ||
| FilterResponse<HomeListResponse> exhibits = homeService.getFilterExhibit(dto, size, cursor,userId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
서비스 레이어에서 DTO로 리턴해주는 부분에서 그대로 프레젠테이션 레이어로 가게 되는데 이렇게 되면 유지보수 시 영향이 있을 것 같아요.
레이어 간 분리를 해보는 것은 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네! 분리하겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
entity -> (service layer dto -> (presentation layer) dto 이런 흐름으로 분리하는게 좋을 것 같아요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
service 레이어에 사용될 dto를 result로 생성을 해서 작업을 진행했습니다 말씀 주신 방향성이 맞는지 확인부탁드려도될까요 ?
| log.info("Exhibit indexing successfully : id={} ",savedExhibit.getExhibitId()); | ||
| } catch (Exception e) { | ||
| log.error("Exhibit indexing failed", e); | ||
| System.out.println("create Exhibit error : " + e.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
로그 대신에 println을 쓰신 이유가 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리팩토링 시 코드 작성 후 로그로 변경예정이었습니다.
우선 비워뒀습니다!
|
|
||
| //필터 전체 조회 | ||
| public FilterResponse getFilterExhibit(ExhibitFilterRequest dto, Long size, Long cursorId, Long userId) { | ||
| public FilterResponse<HomeListResponse> getFilterExhibit(ExhibitFilterRequest dto, Long size, Long cursorId, Long userId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전시 검색과 필터링 자체가 하나의 엔드포인트 내에서 처리될 수 있을 것 같은데 한 번 고민해보시고 수정해주세요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네 하나로 통일했습니다.
| } | ||
| } catch (Exception e) { | ||
| log.error("redis cache filed : {}", e.getMessage(), e); | ||
| System.out.println( "init Redis : " + e.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
로그 대신에 println을 쓰시는 이유
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리팩토링 시 로그 변경예정이었습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네, log level이 미치는 영향에 대해서 한 번 고민해주시고 변경해주세요.
|
|
||
| private static final String POPULAR_KEYWORDS_KEY = "search:popular_keywords"; | ||
|
|
||
| @PostConstruct |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
클래스 네임으로 봐서는... 이력을 조회하는 것으로 보이는데 여기서 레디스 init을 하는 부분에 있어서 단일 책임 원칙에 위배되는 것으로 보여요. 수정할 수 있는 부분으로 보여지는데 한 번 고민해봐주세요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네, 레디스 리팩토링 시 변경 진행해도될까요 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
정해진 기한 내에 가능하다면 상관없을 것 같아요.
…h with exhibit condition search.
#216