feat: 서버에서 충전소 정보들을 필터링 하는 기능을 만든다 #218

sosow0212 · 2023-07-27T05:57:45Z

📄 Summary

충전기 필터링 기능 구현, 인덱스 설정, 몇 가지 객체 추가를 했습니다.

🕰️ Actual Time of Completion

🙋🏻 More

현재 좌표에 해당하는 전체 충전소를 조회하는 기능에 필터링 기능이 추가 되었습니다.
필터는 [충전소 회사 이름, 충전 타입, 충전 용량] 으로 세 가지입니다.

JPQL로 2^3 = 8개의 쿼리를 만들거나 Querydsl을 사용하지 않았고, 범위 내에 전체 조회를 한 후 Stream으로 자바 코드로 필터링 해주었습니다.

이유는 조회시에 적용한 필터를 기준으로 sql을 날리게 되는데, 이렇게 된다면 인덱스가 많이 필요합니다. 필터가 n개라면 n개의 인덱스가 늘어나야겠죠? 안 그렇다면 적용한 필터마다 조회 속도가 다릅니다.

어차피 범위 내에 조회는 현재도 느린 속도로 조회되지 않아서 latitude, longitude에 인덱스를 걸어서 이 두 가지만으로도 조회 및 필터링 기능을 구현할 수 있었습니다.

잦은 업데이트가 일어나는 서비스 특성상 인덱스 설정을 할 때 고민을 많이 했습니다.
최대한 자주 바뀌지 않는 부분에 인덱스를 걸어주었습니다. 그 결과 23만 건 데이터 기준으로 선릉역 주변 조회시 평균 0.84초 -> 평균 0.66초로 약 30%정도 조회 속도가 빨라졌습니다. (상세 정보도 마찬가지입니다.)

그리고 좌표는 Coordinate라는 객체를 만들어서 관리할 수 있도록 변경했고, Stations라는 객체를 만들어서 필터링을 수행할 수 있도록 만들었습니다.

인덱스 부분이나 다른 개선할 점 있으면 말해주세요~

close #176

drunkenhw

수고하셨습니다 컨벤션 관련해서 몇가지 고쳐주시면 감사하겠습니다.
그리고 모르는 부분에 대해 질문 남겼습니다~

drunkenhw · 2023-07-27T06:29:37Z

backend/src/main/java/com/carffeine/carffeine/station/domain/station/Station.java

@@ -23,7 +24,7 @@
 @NoArgsConstructor(access = AccessLevel.PROTECTED)
 @Entity
 @EqualsAndHashCode(of = "stationId")
-@Table(name = "charge_station")
+@Table(name = "charge_station", indexes = @Index(name = "idx_station_coordination", columnList = "latitude, longitude, stationId"))


station Id는 primary key 인데 해당 설정을 안하면 index를 만들지 않나요?

넵 처음에 제거하고 DB 인덱스 사용 확인해보니 lat, long만 있어서 추가했습니다!
혹시 제가 틀렸다면 말씀해주세요~

drunkenhw · 2023-07-27T06:30:55Z

backend/src/main/java/com/carffeine/carffeine/station/domain/charger/Charger.java

@@ -30,7 +31,8 @@
 @NoArgsConstructor(access = AccessLevel.PROTECTED)
 @IdClass(ChargerId.class)
 @Entity
-@Table(name = "charger")
+@Table(name = "charger", indexes = @Index(name = "idx_charger_default", columnList = "station_id, charger_id"))


charger station_id, charger_id가 복합 키로 primary key로 알고 있는데 해당 설정을 하지 않으면 indexing 하는 방법이 달라지나요?

맞습니다! 말씀하신대로 복합키 pk일 땐 안 달아도 됩니다. 제거하겠습니다~

drunkenhw · 2023-07-27T06:31:47Z

backend/src/main/java/com/carffeine/carffeine/station/domain/charger/ChargerStatus.java

@@ -23,7 +24,7 @@
 @NoArgsConstructor(access = AccessLevel.PROTECTED)
 @IdClass(ChargerId.class)
 @Entity
-@Table(name = "charger_status")
+@Table(name = "charger_status", indexes = @Index(name = "idx_status_default", columnList = "charger_id"))


이 charger status은 어떤 이유로 해당 칼럼을 indexing 한 거죠?

이것도 마찬가지로 잘못 작성된 인덱스 같습니다

drunkenhw · 2023-07-27T06:33:43Z

backend/src/main/java/com/carffeine/carffeine/station/domain/station/StationRepository.java

+            "LEFT JOIN FETCH c.chargerStatus " +
+            "WHERE s.latitude.value >= :minLatitude AND s.latitude.value <= :maxLatitude " +
+            "AND s.longitude.value >= :minLongitude AND s.longitude.value <= :maxLongitude")
+    List<Station> findAllByLatitudeBetweenAndLongitudeBetweenWithFetch(@Param("minLatitude") BigDecimal minLatitude,


해당 메서드는 findAllFetchByLatitude~~~~~~~ 가 어떠신가요? 뭔가 with fetch라는 것이 어색해보여서요

drunkenhw · 2023-07-27T06:34:07Z

backend/src/main/java/com/carffeine/carffeine/station/domain/station/Coordinate.java

+        Longitude minLongitude = originLongitude.calculateMinLongitudeByDelta(longitudeDelta);
+        Longitude maxLongitude = originLongitude.calculateMaxLongitudeByDelta(longitudeDelta);
+
+        return new Coordinate(minLatitude, maxLatitude, minLongitude, maxLongitude);


drunkenhw · 2023-07-27T06:36:50Z

backend/src/main/java/com/carffeine/carffeine/station/domain/station/Stations.java

+
+    private final List<Station> stations;
+
+    private Stations(final List<Station> stations) {


final 제거 부탁드립니다

drunkenhw · 2023-07-27T06:37:53Z

backend/src/main/java/com/carffeine/carffeine/station/service/station/StationService.java

+        Stations stations = Stations.of(stationsByCoordinate);
+        stations.filterByCompanyNames(companyNames);
+        stations.filterByChargerTypes(chargerTypes);
+        stations.filterByCapacities(capacities);

-        return stationRepository.findAllByLatitudeBetweenAndLongitudeBetween(minLatitude, maxLatitude, minLongitude, maxLongitude);
+        return stations.getStationsExclusiveEmptyChargers();


해당 메서드를 그냥 stations에서 다 해서 반환하는 것은 어떤가요?

drunkenhw · 2023-07-27T06:38:27Z

backend/src/test/java/com/carffeine/carffeine/station/domain/station/StationsTest.java

+        stations.filterByCompanyNames(List.of("볼튼"));
+
+        // then
+        assertAll(


저희 assertAll 말고 assertSoftly를 사용하기로 했던 것 같아요..ㅜ

drunkenhw · 2023-07-27T06:39:10Z

...test/java/com/carffeine/carffeine/station/service/station/StationServiceIntegrationTest.java

+        List<Station> result = stationService.findByCoordinate(coordinateRequest, List.of("볼튼"), new ArrayList<>(), new ArrayList<>());
+
+        // then
+        assertAll(


assertSoftly..

drunkenhw · 2023-07-27T06:39:41Z

...test/java/com/carffeine/carffeine/station/service/station/StationServiceIntegrationTest.java

+
+@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class)
+@SuppressWarnings("NonAsciiCharacters")
+class StationServiceIntegrationTest extends IntegrationTest {


service 클래스를 integration test 하신 이유가 있나요?

Service에서는 주로 비즈니스 로직이 들어갑니다.
비즈니스 로직은 중요하다고 생각해서 이 부분은 가능하면 테스트 격리를 통해서 느리더라도 항상 통합 테스트하고 있습니다.
(물론 문자 메시지 같이 돈이 드는 로직이라면 테스트 더블을 사용하긴 합니다~)

현재 FakeRepository가 있으나 완전 동일한 환경 (쿼리가 실제로 안나가지만 fake에서는 나간 것처럼 보일 수 있는 것처럼)은 아니어서 통합 테스트로 만들었습니다.

제 생각에는 fake repository로 테스트해도 충분하다고 생각하는데요
이유는 이미 repository 테스트를 통해서 해당 쿼리가 원하는 대로 나가는지에 대한 검증은 충분히 할 수 있다고 생각합니다.
그래서 service 클래스에서는 jpa 혹은 jdbc 아니면 다른 database를 사용하던 repository의 구현에 의존하지 않고 fake repository를 통해 비즈니스 로직을 검증하는 것이 더 좋다고 생각합니다.
그리고 부가적으로 따라오는 속도가 빠른 장점도 있을 것 같습니다.
repository에 구현에 따라 service 클래스 로직의 테스트가 실패한다거나 성공한다는 것이 조금 더 이상하다고 생각합니다.
마지막으로는 저희가 인수테스트를 하기도 하니 통합테스트가 굳이 중복되지 않아도 될 것 같다는 생각입니다!

음 제 테스트 기준에는 서비스 레이어에 중요한 로직이 담긴 부분이기 때문에 통합 테스트로 정확한 검증을 해야된다고 생각했습니다.

만약 서비스 레이어가 외부 API에 의존하는 경우에 갑자기 어느날 외부 API가 바뀌었다면, Mocking이나 FakeRepository 방식으로는 성공하는 테스트가 될 순 있지만, 실제로는 사용자들의 서비스에 문제가 생기겠죠.
그렇다면 깨지는 테스트가 되는 건데 개발자 입장에서는 어떤 부분에서 문제가 생긴지 찾기 힘들 것 같습니다.

또한 저는 테스트도 유지보수의 대상이라고 생각하는데, FakeRepository를 하나하나 구현해주는 부분도 로직이 바뀐다면 유지보수의 대상이 되고 복잡한 쿼리일 경우 구현에 있어서는 오히려 많은 시간이 소모될 수 있다고 생각했습니다.

하지만 얘기를 들어보니 충분히 납득이 됩니다.
꼭 서비스는 통합, 테스트더블 이런 식으로 흑백논리로 나누기보다는 아까도 대화했듯이 필요한 부분에서 상황에 맞게 선택하는 것이 좋을 것 같습니다.

통합 테스트는 정확할 수 있으나, 느리다는 단점이 있고
제시해주신 테스트 방법은 깨지는 테스트가 될 순 있지만 빠르다는 장점이 있습니다.

그래서 현재 구조에서는 해당 서비스가 외부 API에 의존 하지도 않고 단순 내부 코드로 간단한 조회와 필터링하는 것이 끝이라 말씀해주신 방법이 납득이 됐습니다.

기준이 확장된 것 같아서 좋네요.
맛있는 리뷰 감사합니다🙇🏻‍♂️

drunkenhw

리뷰 한번 더 달았습니다~
확인 부탁드립니다.

drunkenhw · 2023-07-27T12:48:35Z

...test/java/com/carffeine/carffeine/station/service/station/StationServiceIntegrationTest.java

+
+@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class)
+@SuppressWarnings("NonAsciiCharacters")
+class StationServiceIntegrationTest extends IntegrationTest {


제 생각에는 fake repository로 테스트해도 충분하다고 생각하는데요
이유는 이미 repository 테스트를 통해서 해당 쿼리가 원하는 대로 나가는지에 대한 검증은 충분히 할 수 있다고 생각합니다.
그래서 service 클래스에서는 jpa 혹은 jdbc 아니면 다른 database를 사용하던 repository의 구현에 의존하지 않고 fake repository를 통해 비즈니스 로직을 검증하는 것이 더 좋다고 생각합니다.
그리고 부가적으로 따라오는 속도가 빠른 장점도 있을 것 같습니다.
repository에 구현에 따라 service 클래스 로직의 테스트가 실패한다거나 성공한다는 것이 조금 더 이상하다고 생각합니다.
마지막으로는 저희가 인수테스트를 하기도 하니 통합테스트가 굳이 중복되지 않아도 될 것 같다는 생각입니다!

drunkenhw · 2023-07-27T12:49:24Z

...test/java/com/carffeine/carffeine/station/service/station/StationServiceIntegrationTest.java

+        SoftAssertions.assertSoftly(softly -> {
+            softly.assertThat(result.size()).isEqualTo(1);
+            softly.assertThat(result.get(0).getStationId()).isEqualTo(station.getStationId());
+        });


해당 메서드의 static import 부탁드려요

drunkenhw · 2023-07-27T12:50:24Z

backend/src/main/java/com/carffeine/carffeine/station/domain/station/Stations.java

+    public static Stations of(List<Station> stations) {
+        return new Stations(stations);
+    }


from으로 변경 부탁드립니다

drunkenhw · 2023-07-27T12:54:03Z

backend/src/main/java/com/carffeine/carffeine/station/domain/station/Stations.java

+    public static Stations createFilteredOf(List<Station> stations,
+                                            List<String> companyNames,
+                                            List<ChargerType> chargerTypes,
+                                            List<BigDecimal> capacities) {
+        List<Station> stationsCopy = new ArrayList<>(stations);
+
+        filterByCompanyNames(stationsCopy, companyNames);
+        filterByChargerTypes(stationsCopy, chargerTypes);
+        filterByCapacities(stationsCopy, capacities);
+
+        return new Stations(stationsCopy);
+    }
+
+    private static void filterByCompanyNames(List<Station> stations, List<String> companyNames) {
+        if (!companyNames.isEmpty()) {
+            stations.removeIf(station -> !companyNames.contains(station.getCompanyName()));
+        }
+    }


정적 팩토리 메서드 말고
예를 들어

public List<Station> filteredStations(List<String> companyNames, List<ChargerType> chargerTypes, List<BigDecimal> capacities) { List<Station> stations = new ArrayList<>(this.stations); filterByCompanyNames(stations, companyNames); filterByChargerTypes(stations, chargerTypes); filterByCapacities(stations, capacities); stations.removeIf(station -> station.getChargers().isEmpty()); return stations; }

이런 방식은 어떤가요?

이 방법이 더 좋아보이네요~

drunkenhw · 2023-07-27T12:57:03Z

backend/src/main/java/com/carffeine/carffeine/station/domain/station/Stations.java

+    private static void filterByCapacities(List<Station> stations, List<BigDecimal> capacities) {
+        if (!capacities.isEmpty()) {
+            stations.removeIf(station -> station
+                    .getChargers()
+                    .stream()
+                    .noneMatch(charger -> capacities.contains(charger.getCapacity()))
+            );
+        }
+    }


capacities는 BigDecimal의 list 인데, contains를 사용하면 내부적으로 해당 객체의 equals()메서드를 호출하는 것으로 알고 있는데요. BigDecimal의 equals의 구현은
new BigDecimal("10.0")과 new BigDecimal("10.00")은 false로 반환하는데요. 그럼 contains를 사용하는 것이 위험할 수도 있지 않을까라는 생각입니다.

음 그렇네요 현재 구조로는 '1'과 '1.0'을 다르게 인식하네요
말씀하신대로 compareTo로 수정하겠습니다!

drunkenhw · 2023-07-27T13:26:13Z

backend/src/test/java/com/carffeine/carffeine/station/fixture/charger/ChargerFixture.java

+                    .stationId("MZ101011")
+                    .chargerId("01")
+                    .chargerCondition(ChargerCondition.CHARGING_IN_PROGRESS)
+                    .latestUpdateTime(LocalDateTime.now())


fixture에 LocalDateTime.now()를 하면 나중에 에러가 생길 요지가 있더라고요. 시간을 지정해주는 것이 어떤가요?

drunkenhw · 2023-07-27T13:26:21Z

backend/src/test/java/com/carffeine/carffeine/station/fixture/charger/ChargerFixture.java

+                    .stationId("MZ101011")
+                    .chargerId("02")
+                    .chargerCondition(ChargerCondition.CHARGING_IN_PROGRESS)
+                    .latestUpdateTime(LocalDateTime.now())


drunkenhw · 2023-07-27T13:28:00Z

backend/src/main/java/com/carffeine/carffeine/station/domain/station/Coordinate.java

+    private final Longitude minLongitude;
+    private final Longitude maxLongitude;
+
+    public static Coordinate from(BigDecimal latitude,


여러 매개변수를 받아 생성하는 메서드의 네이밍은 of 라네요..ㅋㅋ

drunkenhw

굿 멋져요 고생하셨습니다

drunkenhw · 2023-07-28T03:31:53Z

backend/src/main/java/com/carffeine/carffeine/station/domain/station/Stations.java

+                    .getChargers()
+                    .stream()
+                    .noneMatch(charger -> capacities.stream()
+                            .anyMatch(capacity -> capacity.compareTo(charger.getCapacity()) == NONE_CAPACITY))


Suggested change

.anyMatch(capacity -> capacity.compareTo(charger.getCapacity()) == NONE_CAPACITY))

.anyMatch(capacity -> isSame(capacity, charger.getCapacity()))

라고 private 메서드를 정의하면 깔끔할 것 같아요 그리고 compareTo 를 해서 0이면 같은 값이라는 뜻이니 상수로 정의 하지 않는 것도 괜찮아보여요

예리하시군요

be-student

고생 많으셨어요 👍

be-student · 2023-07-28T05:19:47Z

backend/src/main/java/com/carffeine/carffeine/station/domain/station/Stations.java

+    public List<Station> getFilteredStations(List<String> companyNames,
+                                             List<ChargerType> chargerTypes,
+                                             List<BigDecimal> capacities) {
+        filterByCompanyNames(stations, companyNames);
+        filterByChargerTypes(stations, chargerTypes);
+        filterByCapacities(stations, capacities);
+        return Collections.unmodifiableList(stations);
+    }


메서드 순서가 filterBy 가 더 나중에 나오면 좋을 것 같아요

be-student · 2023-07-28T05:20:22Z

backend/src/main/java/com/carffeine/carffeine/station/domain/station/Stations.java

+            stations.removeIf(station -> station
+                    .getChargers()
+                    .stream()
+                    .noneMatch(charger -> chargerTypes.contains(charger.getType()))
+            );


removeIf 는 부수효과가 있는 것으로 알고 있는데,
조회, 명령을 분리해보면 어떨까요?

객체의 원본을 건들면 안되기에 값을 복사한 후 필터를 진행하고 반환하도록 변경했습니다.

be-student · 2023-07-28T05:20:32Z

backend/src/main/java/com/carffeine/carffeine/station/domain/station/Stations.java

+        }
+    }
+
+    private static void filterByCapacities(List<Station> stations, List<BigDecimal> capacities) {


불필요한 static 이 있어요

be-student · 2023-07-28T05:22:33Z

backend/src/main/java/com/carffeine/carffeine/station/domain/charger/Charger.java

@@ -30,7 +29,6 @@
 @NoArgsConstructor(access = AccessLevel.PROTECTED)
 @IdClass(ChargerId.class)
 @Entity
-@Table(name = "charger")


테이블을 지운 이유가 있나요?

아이고 인덱스 지울 때 커맨드누르고 제거해서 해버려서 한 줄이 다 날라갔었네요
복구 해놓을게요~

be-student

확인했습니다 고생 많으셨어요! 👍

kiarakim

큼직한 기능 구현하느라 수고하셨어요!👍

kiarakim · 2023-07-28T14:34:53Z

backend/src/main/java/com/carffeine/carffeine/station/controller/station/StationController.java

        StationsSimpleResponse chargerStationsSimpleResponse = StationsSimpleResponse.from(stations);
        return ResponseEntity.ok(chargerStationsSimpleResponse);
    }

+


요기 줄 추가는 뭔가여

sosow0212 added 6 commits July 26, 2023 17:28

feat: 필터링 기능 추가

c01a025

test: 필터링 기능 테스트 및 기존 코드 리팩토링 진행

d14f67f

feat: List<Station>을 관리하는 객체 생성

a2a897e

test: List<Station>을 관리하는 객체 테스트

52c91a0

refactor: getValue()에서 다른 부분과 겹치지 않기 위해서 따로 getter 재정의

01c2a40

feat: 인덱스 적용 및 효율적인 연산을 위해서 fetch join 쿼리에서 BETWEEN 연산자 제거 및 범위 쿼리 사용

8458f8a

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

sosow0212 self-assigned this Jul 27, 2023

sosow0212 requested review from be-student, drunkenhw and kiarakim as code owners July 27, 2023 05:57

sosow0212 temporarily deployed to test July 27, 2023 05:57 — with GitHub Actions Inactive

drunkenhw requested changes Jul 27, 2023

View reviewed changes

sosow0212 added 2 commits July 27, 2023 17:26

refactor: 불필요한 인덱스 제거

8eb59a5

refactor: 코드 컨벤션 정리 및 일부 리팩토링 진행

fcb5eb8

sosow0212 temporarily deployed to test July 27, 2023 08:50 — with GitHub Actions Inactive

drunkenhw requested changes Jul 27, 2023

View reviewed changes

sosow0212 added 5 commits July 28, 2023 11:39

refactor: 에러 방지를 위해 fixture의 날짜를 수동으로 설정

e361251

refactor: 네이밍 및 메서드 구조 변경

ab5c7f0

refactor: 네이밍 및 메서드 구조 변경

28f034c

refactor: 테스트 통합화

942f799

refactor: 데이터를 가져올 때 INNER JOIN으로 가져와서 null인 값들은 제외하도록 변경

b12b0b9

sosow0212 temporarily deployed to test July 28, 2023 03:27 — with GitHub Actions Inactive

drunkenhw approved these changes Jul 28, 2023

View reviewed changes

refactor: 메서드 분리

f7d74e3

sosow0212 temporarily deployed to test July 28, 2023 05:20 — with GitHub Actions Inactive

be-student requested changes Jul 28, 2023

View reviewed changes

refactor: 값을 복사하고 반환하도록 변경

3ad4a6e

sosow0212 temporarily deployed to test July 28, 2023 06:55 — with GitHub Actions Inactive

be-student approved these changes Jul 28, 2023

View reviewed changes

kiarakim approved these changes Jul 31, 2023

View reviewed changes

kiarakim merged commit 31fbd19 into develop Jul 31, 2023
1 check passed

kiarakim deleted the feat/176 branch July 31, 2023 07:54


		private final List<Station> stations;

		private Stations(final List<Station> stations) {

	.anyMatch(capacity -> capacity.compareTo(charger.getCapacity()) == NONE_CAPACITY))
	.anyMatch(capacity -> isSame(capacity, charger.getCapacity()))

feat: 서버에서 충전소 정보들을 필터링 하는 기능을 만든다 #218

feat: 서버에서 충전소 정보들을 필터링 하는 기능을 만든다 #218

Conversation

sosow0212 commented Jul 27, 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

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

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 Jul 27, 2023 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

be-student 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

be-student left a 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 commented Jul 27, 2023 •

edited by github-actions bot

drunkenhw Jul 27, 2023 •

edited