feat: 지역 검색 기능을 추가한다 #836

sosow0212 · 2023-10-06T10:20:27Z

📄 Summary

서비스를 실 사용하니 지역 검색 기능이 필요로 했습니다. 따라서 기존 충전소 검색에서 지역 검색도 되게끔 변경했습니다.
처음 서버를 키면 지역 정보가 DB에 insert됩니다. 만약 이미 지역 정보를 가지고 있다면 지역 정보를 저장하지 않습니다.

충전소 검색시 %검색어% 방식으로 지역을 검색합니다.

🕰️ Actual Time of Completion

🙋🏻 More

close #833

drunkenhw

작성하신 코드 잘 봤습니다 전반적으로 잘 작성해주셔서 몇가지 코멘트만 남겼습니다

어제 얘기했을 때는 json으로 관리하여 요청하는 이유가 city에 대해 추가,수정,삭제 이런 것들을 관리하기 편해서라고 말씀해주셨는데 작성해주신 코드를 보면 이미 저장되어 있다면 작업을 수행하지 않기 때문에 굳이 json으로 관리하는 이유를 잘 모르겠습니다!
오히려 이렇게 된다면 json도 관리해줘야하고 db의 데이터도 관리해줘야 될 것 같아보여서요 혹시 제이의 다른 의견이 있으신가요?

drunkenhw · 2023-10-07T03:35:15Z

backend/src/main/java/com/carffeine/carffeine/station/domain/city/CityCustomRepositoryImpl.java

+    }
+
+    @Override
+    public boolean isExistAlready() {


is exists 로 메서드명을 바꾸는 건 어떤가요? already는 결국 과거형이라 뭔가 어색한 느낌인것 같아요

drunkenhw · 2023-10-07T03:36:03Z

backend/src/main/java/com/carffeine/carffeine/station/domain/city/CityCustomRepositoryImpl.java

+    @Override
+    public boolean isExistAlready() {
+        String sql = "SELECT EXISTS (SELECT 1 FROM city)";
+        Integer result = namedParameterJdbcTemplate.queryForObject(sql, Map.of(), Integer.class);


아무 변수를 바인딩할 필요가 없다면 namedparameter jdbc말고 그냥 jdbctemplate을 사용하는 것이 가독성이 좋을 것 같아요!

drunkenhw · 2023-10-07T03:37:53Z

...java/com/carffeine/carffeine/station/infrastructure/repository/city/CityQueryRepository.java

+                .where(city.name.contains(query))
+                .limit(LIMIT_SIZE_OF_CITY)


orderby 하지 않아도 매번 같은 결과가 보장되나요? 정렬을 하지 않으면 왠지 같은 단어를 검색해도 결과가 다를 수도 있을 것 같아서요

먼저 찾는 순서대로 나와서 결과는 같습니다! 그래도 OrderBy 해주는 것이 보는 사람 입장에서는 편할 것 같네요~ 수정하겠습니다

drunkenhw · 2023-10-07T03:40:07Z

backend/src/main/java/com/carffeine/carffeine/station/service/city/CityRequestService.java

+@RequiredArgsConstructor
+@Slf4j
+@Service
+public class CityRequestService {


해당 객체에 많은 책임이 있는 것 같은데 분리하는 것은 어떤가요?
예를 들어 요청을 보내는 클래스, 그걸 변환을 해주는 부분, 저장을 하는 부분

분리는 하는게 저도 좋다고 생각합니다!

다만, 위에 리뷰와 더불어서 어떻게 할지 한 번 생각을 들어보고 진행하고 싶어서 이번에는 코멘트만 남겨봅니다!

JSON으로 데이터를 받는 것에 대하여

이 부분은 처음에 cityCustomRepository.isExist()를 통해서 데이터가 없다면 city 데이터를 로드하여 저장합니다.
다만 만약 새로운 데이터가 추가된다면 저장하지 못하는 문제가 발생할 수 있다고 생각합니다.

--> 이를 개선하기 위해서 해당 if문을 지워도 될 것 같다고 생각합니다. 현재 batch insert 부분에서 INSERT IGNORE~ 쿼리로 작동하기 때문입니다

현재 City 데이터가 약 2만6천개 정도 되고 있습니다.
이를 저도 페이징 처리해서 1000개씩 저장하면 좋다고 생각하지만, 이 API에서 페이징할 수가 없습니다 ㅠㅠ 이를 어떻게 나눠서 저장하면 좋을까요?
물론 처음에만 저장하면 그 후부터는 중복되는 건 스킵하기 때문에 큰 문제는 없을 것 같지만 첫 저장에서 오래 걸릴까봐 걱정이 되긴하네요 ㅎㅎ 의견 주시면 좋을 것 같습니다.

지금 로직에서 insert ignore로 저장하게 된다면 똑같은 데이터가 또 저장될 것 같습니다. insert ignore은 id가 같은 부분에 대해서 insert 시 에러를 무시하도록 하는 함수인데, 현재는 insert 시에 id를 auto increment로 증가시켜주고 있기 때문에 id가 무조건 중복되지 않아 그냥 insert가 될 것 같아요 , 그리고 json에 데이터가 추가된다면 insert로 추가하는 부분은 쉬울 것 같은데, 만약 json에서 해당 주소명이 변경되거나, 위도 경도가 변경된다면 대응하기가 힘들 것 같은데 이 부분에 대해서는 어떻게 생각하시나요?

페이징 처리를 하지 않더라도 트랜잭션은 분리하면 좋을 것 같아요!

RequestService의 트랜잭션 분리 및 현재 다른 작업처럼 첫 실행에서 실행되고 나머지는 off로 실행되지 않도록 변경

가끔 추가되는 지역 데이터 및 수정되는 정보는 관리자 페이지로 작업하도록 변경

슬랙으로 대화 나눈 것처럼 API 호출하는 작업은 비싼 작업이니 관리자 페이지에서 관리하도록 변경하겠습니다.
자주 바뀌는 데이터가 아니다보니 관리자 페이지로 진행해도 될 것 같습니다!
좋은 리뷰 감사합니다 !

drunkenhw · 2023-10-07T03:42:12Z

backend/src/main/java/com/carffeine/carffeine/station/service/city/CityRequestService.java

+    @Transactional
+    public void requestCity() {
+        while (true) {
+            try {
+                if (cityCustomRepository.isExistAlready()) {
+                    log.info("지역 정보가 이미 존재합니다");
+                    return;
+                }
+                saveCities();
+                log.info("지역 정보 저장 완료");
+                return;
+            } catch (Exception e) {
+                log.error("지역 정보 저장 실패");
+            }
+        }
+    }


해당 메서드에 transactional이 붙어있으면 요청을 보내서 응답을 받아오는 시간 + 객체를 매핑하는 시간 + 저장하는 시간 때문에 트랜잭션을 오래 잡고 있을 것 같은데 문제 없을까요..?

drunkenhw · 2023-10-07T03:44:53Z

.../src/test/java/com/carffeine/carffeine/station/domain/city/CityCustomRepositoryImplTest.java

+        Assertions.assertDoesNotThrow(() ->
+                cityCustomRepository.saveAll(cities)
+        );


does not throw로 저장이 되었다고 검증을 할 수 있을까요? 차라리 아래의 테스트로 퉁치는 것이 어떨까요

그러네요 밑에는 어차피 저장을 해야하니 퉁칠 수 있겠네요

사실 두 가지를 검증하니 좋은 테스트는 아니라고 생각하지만 테스트를 위한 불필요한 메서드보다는 나은 것 같아요 감사합니다

drunkenhw · 2023-10-07T03:46:12Z

.../com/carffeine/carffeine/station/infrastructure/repository/city/CityQueryRepositoryTest.java

+            softly.assertThat(result.get(0)).usingRecursiveComparison()
+                    .isEqualTo(new CitySearchResponse("서울특별시", Latitude.from("37.5666103").getValue(), Longitude.from("126.9783882").getValue()));
+            softly.assertThat(result.get(1)).usingRecursiveComparison()
+                    .isEqualTo(new CitySearchResponse("서울특별시 송파구 잠실동", Latitude.from("37.5666103").getValue(), Longitude.from("126.9783882").getValue()));
+            softly.assertThat(result.get(2)).usingRecursiveComparison()
+                    .isEqualTo(new CitySearchResponse("서울특별시 송파구 신천동", Latitude.from("37.5666103").getValue(), Longitude.from("126.9783882").getValue()));


using recursive comparison을 사용하면 굳이 list.get을 하지 않고도 테스트 할 수 있어요 그렇게 수정하는 것이 가독성이 좋을 것 같아요

kiarakim

수고하셨어요 제이! 테스트 관련해서 의견 하나 남겼습니다~

kiarakim · 2023-10-07T05:17:29Z

backend/src/main/java/com/carffeine/carffeine/station/domain/city/CityCustomRepository.java

+public interface CityCustomRepository {
+


왜 Custom이라는 키워드가 들어갔는지 궁금해요!

기존에 주기적인 업데이트 하는 부분도 Custom이라고 이미 네이밍 되어 있어서 통일하려고 맞췄습니다!

kiarakim · 2023-10-07T05:25:08Z

.../com/carffeine/carffeine/station/infrastructure/repository/city/CityQueryRepositoryTest.java

+    @Test
+    void 지역을_최대_3개_불러온다() {
+        // given
+        cityCustomRepository.saveAll(List.of(
+                서울특별시_정보,
+                서울특별시_송파구_잠실동_정보,
+                서울특별시_송파구_신천동_정보
+        ));
+
+        // when
+        List<CitySearchResponse> result = cityQueryRepository.findSearchResult("서울");


저희 검색조건이 %지역%과 최대3개라면
-서울 송파 잠실
-서울 송파 신천
-경기 송파라솔 어쩌구
-경기 송파란색 저쩌구
저장해서 쿼리를 "송파"로 하고 송파라는 단어가 들어있는 것들 중 3개를 잘 갖고오는지 확인하는 테스트도 있었으면 좋겠어요.

그러네요~ 수정했습니다!

drunkenhw

제이 작성해주신 코드 잘 봤습니다
트랜잭션 분리하신 것도 확인했습니다
마지막으로 city패키지가 station에 있는게 어색한데 어떻게 생각하시나요?

drunkenhw · 2023-10-09T06:20:43Z

backend/src/main/java/com/carffeine/carffeine/station/service/city/CityRequestService.java

+    private final CityCustomRepository cityCustomRepository;
+
+    public void requestCity() {
+        while (true) {


while true로 메서드를 호출하면 문제가 없을까요?
제 생각에는 네트워크 환경이 안좋다던가 혹은 주소가 잘못되거나 이럴 때도 계속 호출하게 될 것 같은데요

그럴 수 있을 것 같아요
커넥션이나 네트워크나 여러 문제가 생기는 경우 호출을 계속할 가능성이 있네요
MAX_TRY_COUNT를 두어서 실행에 제한을 두도록 변경하겠습니다

drunkenhw · 2023-10-09T06:22:53Z

backend/src/main/java/com/carffeine/carffeine/admin/controller/AdminCityController.java

+    private final AdminCityService adminCityService;
+
+    @GetMapping
+    public ResponseEntity<CustomPage<CityResponse>> findAllCities(@AuthMember Long memberId, Pageable pageable) {


custom page 좋아요

drunkenhw · 2023-10-09T06:24:19Z

backend/src/main/java/com/carffeine/carffeine/station/domain/city/CityCustomRepositoryImpl.java

+
+@RequiredArgsConstructor
+@Component
+public class CityCustomRepositoryImpl implements CityCustomRepository {


jdbc template을 사용하는 클래스가 domain에 있는게 어색해요

맞네요 라이브러리이니 infrastructure로 이동시켰습니다.

drunkenhw · 2023-10-09T06:25:58Z

.../main/java/com/carffeine/carffeine/station/infrastructure/api/RestTemplateCityRequester.java

+@RequiredArgsConstructor
+@Slf4j
+@Component
+public class RestTemplateCityRequester {


해당 클래스도 인터페이스를 통해 의존성을 관리하는 것이 어떠신가요? 만약 rest template을 사용하다 web client라는 클래스를 사용하게 된다면 클래스 이름도 바뀌어야할 거고 내부 구현도 바뀌어야하는데 이걸 사용하는 서비스 클래스에서도 변경에 의한 영향이 있을 것 같아서요

정말 갓스터네요 👍

drunkenhw

수고하셨습니다

sosow0212 added 6 commits October 6, 2023 16:33

refactor: 컨벤션에 맞게 final 키워드 제거

025c75f

feat: 도시 정보 도메인 생성

f83e1e0

feat: 도시 정보 repository 추가

bdd286f

feat: 서버 시작시 도시 정보를 저장하는 기능 추가. 단, 이미 저장 됐다면 저장하지 않음

3b52b35

refactor: 지역도 검색되도록 변경

c68ea30

refactor: 인덱스 제거

3ce4272

sosow0212 added 🌱 기능추가 새로운 기능 요청입니다 BE 백엔드 관련 이슈입니다 labels Oct 6, 2023

sosow0212 self-assigned this Oct 6, 2023

sosow0212 requested review from drunkenhw and kiarakim as code owners October 6, 2023 10:20

sosow0212 temporarily deployed to test October 6, 2023 10:20 — with GitHub Actions Inactive

sosow0212 changed the title ~~feat: 지역 검색 기능을 추가~~ feat: 지역 검색 기능을 추가한다 Oct 6, 2023

drunkenhw requested changes Oct 7, 2023

View reviewed changes

kiarakim approved these changes Oct 7, 2023

View reviewed changes

sosow0212 added 3 commits October 8, 2023 15:29

refactor: 메서드명 변경

a73bc4f

refactor: orderBy 추가

d206487

refactor: 테스트 변경

4d5c177

sosow0212 temporarily deployed to test October 8, 2023 07:18 — with GitHub Actions Inactive

sosow0212 added 3 commits October 9, 2023 15:04

feat: City 관리자 페이지 CRUD & Test 생성

f41a96a

feat: 요청 보내는 기능 분리

f008ab8

refactor: 요청 보내는 기능 분리

0038c77

sosow0212 had a problem deploying to test October 9, 2023 06:11 — with GitHub Actions Failure

drunkenhw requested changes Oct 9, 2023

View reviewed changes

refactor: 패키지 이동 및 api 실행 제어

c675ec0

sosow0212 temporarily deployed to test October 9, 2023 06:45 — with GitHub Actions Inactive

sosow0212 added 2 commits October 10, 2023 11:48

refactor: request_url 변경

43cd123

refactor: 패키지 구조 변경

64bc863

sosow0212 temporarily deployed to test October 10, 2023 03:02 — with GitHub Actions Inactive

drunkenhw approved these changes Oct 10, 2023

View reviewed changes

sosow0212 merged commit 0fc76bf into develop Oct 10, 2023
1 check passed

sosow0212 deleted the feat/833 branch October 10, 2023 04:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: 지역 검색 기능을 추가한다 #836

feat: 지역 검색 기능을 추가한다 #836

sosow0212 commented Oct 6, 2023 •

edited by github-actions bot

drunkenhw left a comment

drunkenhw Oct 7, 2023

drunkenhw Oct 7, 2023

drunkenhw Oct 7, 2023

sosow0212 Oct 8, 2023

drunkenhw Oct 7, 2023

sosow0212 Oct 8, 2023

drunkenhw Oct 8, 2023

sosow0212 Oct 8, 2023

drunkenhw Oct 7, 2023

drunkenhw Oct 7, 2023

sosow0212 Oct 8, 2023

drunkenhw Oct 7, 2023

kiarakim left a comment

kiarakim Oct 7, 2023

sosow0212 Oct 8, 2023 •

edited

kiarakim Oct 7, 2023

sosow0212 Oct 8, 2023

drunkenhw left a comment

drunkenhw Oct 9, 2023

sosow0212 Oct 9, 2023

drunkenhw Oct 9, 2023

drunkenhw Oct 9, 2023

sosow0212 Oct 9, 2023

drunkenhw Oct 9, 2023

sosow0212 Oct 9, 2023

drunkenhw left a comment

feat: 지역 검색 기능을 추가한다 #836

feat: 지역 검색 기능을 추가한다 #836

Conversation

sosow0212 commented Oct 6, 2023 • edited by github-actions bot

📄 Summary

🕰️ Actual Time of Completion

🙋🏻 More

drunkenhw 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

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

kiarakim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sosow0212 Oct 8, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drunkenhw 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

Choose a reason for hiding this comment

drunkenhw left a comment

Choose a reason for hiding this comment

sosow0212 commented Oct 6, 2023 •

edited by github-actions bot

sosow0212 Oct 8, 2023 •

edited