fix: 필터링 API 파싱 오류 수정 #604 #605

linirini · 2025-01-19T02:34:45Z

⭐️ Issue Number

🚩 Summary

필터링 API 에서 필터링 조건 없을 때 NPE에 대한 고려를 놓쳤습니다.
컨트롤러 단위 테스트와 도메인 단위 테스트로는 잡지 못했어요! dto 내부에 해당 로직이 있기 때문에 dto 단위 테스트가 필요하다고 생각해서 추가했습니다.
추가로, 같은 조건값이 들어왔을 때에는 한 번만 카운트 하도록 로직 추가했습니다.
Dev 서버의 workflow가 2개가 있어서 하나는 비활성화했습니다~

🛠️ Technical Concerns

🙂 To Reviewer

📋 To Do

github-actions · 2025-01-19T02:36:42Z

Test Results

29 files 29 suites 5s ⏱️
192 tests 192 ✅ 0 💤 0 ❌
199 runs 199 ✅ 0 💤 0 ❌

Results for commit 283d21f.

♻️ This comment has been updated with latest results.

github-actions · 2025-01-19T02:36:43Z

🌻Test Coverage Report

Overall Project	78.8%	🍏
Files changed	100%	🍏

File	Coverage
LoginRequest.java	100%	🍏
MemoryFilter.java	100%	🍏
MemoryReadRequest.java	85.71%	🍏

Junyoung-WON

원인을 빠르게 찾아내고 수정해주셔서 감사합니다!! 리니 쵝오~ 🙌

Junyoung-WON · 2025-01-19T03:20:07Z

backend/src/main/java/com/staccato/memory/service/dto/request/MemoryReadRequest.java

+        if (Objects.nonNull(filters)) {
+            return Arrays.stream(filters.split(DELIMITER)).toList();
+        }
+        return List.of();


filters가 nonNull인지 확인하여 분기 처리를 해주었군요! 👍
메서드 분리도 깔끔합니다!

Junyoung-WON · 2025-01-19T03:22:52Z

backend/src/test/java/com/staccato/memory/service/dto/request/MemoryReadRequestTest.java

DTO 클래스의 테스트를 만들어서, filters와 sort가 주어지지 않은 상황에 대한 테스트를 추가해주셨군요! 👍

BurningFalls

브랜치 이름에 인덱스가 604가 아니라 546으로 되어 있던데, 다른 기준을 사용하신 건가요?
이번과 같이 주요 작업에 부작업(워크플로우 비활성화)을 섞어서 하나의 PR로 처리하다보면, 나중에 부작업을 어디서 했는지 제목으로 찾기가 불가능하더라구요. 실제로 몇번 느꼈습니다. 그렇다고 각각 다른 PR로 분리하자니 별로 중요한 작업이 아니라면 귀찮게 느껴지기도 합니다. 앞으로 어느 방향으로 진행하면 좋을까요?

BurningFalls · 2025-01-19T03:13:05Z

.github/workflows/backend-ci-cd-dev.yml

-  push:
-    paths: [ 'backend/**', '.github/**' ]
-    branches: [ "develop-be", "develop" ]
+  workflow_dispatch:


사용자가 필요할 때 수동으로 실행할 수 있도록 설정하는 기능이네요! 👍

BurningFalls · 2025-01-19T03:21:07Z

backend/src/main/java/com/staccato/memory/service/MemoryFilter.java

@@ -28,6 +28,7 @@ public static List<MemoryFilter> findAllByName(List<String> filters) {
                .map(name -> MemoryFilter.findByName(name.trim()))
                .filter(Optional::isPresent)
                .map(Optional::get)
+                .distinct()


똑같은 필터가 두 개 들어올 수도 있기 때문에, 불필요한 연산 방지를 위해서 중복 제거 작업이 꼭 필요하겠네요! 👍

BurningFalls · 2025-01-19T03:32:19Z

backend/src/main/java/com/staccato/memory/service/dto/request/MemoryReadRequest.java

+    private List<String> parseFilters() {
+        if (Objects.nonNull(filters)) {
+            return Arrays.stream(filters.split(DELIMITER)).toList();
+        }
+        return List.of();
+    }


private List<String> parseFilters() { if (Objects.isNull(filters)) { return List.of(); } return Arrays.stream(filters.split(DELIMITER)).toList(); }

리니는 둘 중 어느 코드를 더 선호하는 편이신가요?

저는 로직상의 흐름이 이러이러한 예외가 발생하면 이렇게 하고... 이러이러한 예외가 발생하면 이렇게 하고... 그래서 최종적으로 어떤 것을 리턴한다 이렇게 읽히는게 편해서 이 방법을 선호하는 편입니다.

그래서 이 메서드가 결국 뭘 하고 싶은건데? 를 알고 싶다면 메서드 끝에 뭘 리턴하는지만 보고도 알 수 있다는 점에서도 장점을 지닌다고 생각합니다.

저도 되도록 예외 상황에 대해서는 빠른 예외처리를 선호하기 때문에 폭포가 말한 방식으로 처리 후 본질에 집중하는 것을 선호합니다. 😅
여기에서는 예외마다 처리 방식이 달라지지 않고, 저번 pr에서 말씀드렸듯 입력된 값이 잘못되었을 때 예외 처리 보다는 무시하는 방향으로 로직을 고려했기 때문에 딱히 상관 없다고 생각했어요! (성공 시에만 이렇게 하고, 나머지는 이걸 반환할거야라고 생각하면서 코드를 짰습니다 ㅋㅋ)
일관적인 것이 좋을 것 같아서 리팩터링 했습니다~

BurningFalls · 2025-01-19T03:41:41Z

backend/src/test/java/com/staccato/memory/service/MemorySortTest.java

    @DisplayName("정렬명이 주어졌을 때 대소문자 구분 없이 MemorySort을 반환한다.")
-    @Test
-    void findByNameWithValidSort() {
+    @ParameterizedTest
+    @CsvSource({
+            "UPDATED, UPDATED",
+            "NEWEST, NEWEST",
+            "OLDEST, OLDEST",
+            "updated, UPDATED",
+            "newest, NEWEST",
+            "oldest, OLDEST"
+    })
+    void findByNameWithValidSort(String name, MemorySort sort) {
        // when
-        MemorySort result = MemorySort.findByName("newest");
+        MemorySort result = MemorySort.findByName(name);

        // then
-        assertThat(result).isEqualTo(MemorySort.NEWEST);
+        assertThat(result).isEqualTo(sort);
    }


CsvSource 대신 EnumSource 사용하면 괜찮지 않을까? 생각이 들어서 직접 아래와 같이 구현해봤는데, 오히려 CsvSource 쪽이 훨씬 직관적이어서 좋네요.

@DisplayName("정렬명이 주어졌을 때 대소문자 구분 없이 MemorySort을 반환한다.") @ParameterizedTest @EnumSource(MemorySort.class) void findByNameWithValidSort(MemorySort sort) { // when MemorySort resultUpper = MemorySort.findByName(sort.name()); MemorySort resultLower = MemorySort.findByName(sort.name().toLowerCase()); // then assertAll( () -> assertThat(resultUpper).isEqualTo(sort), () -> assertThat(resultLower).isEqualTo(sort) ); }

추가로, Enum으로 테스트를 진행했을 때 검증 결과 값으로 검증을 하는 것이 어색하게 다가온 것도 있었어요 ㅎㅎ
물론 지금은 대소문자 구분하지 않는 것을 검증하는 것이 주 목적이긴 하지만요 ㅎㅎ

BurningFalls · 2025-01-19T03:49:50Z

backend/src/test/java/com/staccato/memory/service/dto/request/MemoryReadRequestTest.java

+        assertThat(filters).hasSize(1).containsOnly(MemoryFilter.TERM);
+    }
+
+    @DisplayName("필터가 주어지지 않았을 때 빈 리스트를 반환한다")


이렇게 고치면 의미가 확실해져서 좋을 것 같습니다!

Suggested change

@DisplayName("필터가 주어지지 않았을 때 빈 리스트를 반환한다")

@DisplayName("필터가 주어지지 않았을 때 빈 MemoryFilter 리스트를 반환한다")

linirini · 2025-01-19T09:25:29Z

브랜치 이름에 인덱스가 604가 아니라 546으로 되어 있던데, 다른 기준을 사용하신 건가요?

바로 직전에 작업한 546 이슈의 연장이라고 생각해서 이슈를 공유했습니다. (fix 이슈가 있는 걸 후에 알아서 pr에서는 604로 연동해놓았습니다.)

이번과 같이 주요 작업에 부작업(워크플로우 비활성화)을 섞어서 하나의 PR로 처리하다보면, 나중에 부작업을 어디서 했는지 제목으로 찾기가 불가능하더라구요. 실제로 몇번 느꼈습니다. 그렇다고 각각 다른 PR로 분리하자니 별로 중요한 작업이 아니라면 귀찮게 느껴지기도 합니다. 앞으로 어느 방향으로 진행하면 좋을까요?

이후에 이전 작업 기록을 추적해야할 작업이라면 분리하는 것 같아요. 하지만, 이번 작업은 워크 플로우 설계를 바꾼 것이 아닌 단순히 비활성화라서 이후에 어디에서 작업 했는지가 중요할 작업은 아니라고 생각했어요.

BurningFalls

수고하셨습니다, 리니! 더 고려해야할 점은 없는 것 같아요. approve 하겠습니다.

BurningFalls · 2025-01-19T09:32:13Z

backend/src/main/java/com/staccato/auth/service/dto/request/LoginRequest.java

@@ -17,7 +17,7 @@ public record LoginRequest(
        String nickname
 ) {
    public LoginRequest {
-        if (!Objects.isNull(nickname)) {
+        if (Objects.nonNull(nickname)) {


저도 커밋달고 나서 Objects.isNull을 또 어디에서 사용한 곳이 없을까 찾다가, 이 부분에서 약간 어색함을 느꼈었습니다 ㅋㅋㅋ 👍 👍 👍

사람 생각하는거 다 똑같구만요~ ㅋㅋㅋㅋㅋㅋㅋㅋ

BurningFalls · 2025-01-19T09:32:50Z

backend/src/test/java/com/staccato/memory/service/dto/request/MemoryReadRequestTest.java

@@ -24,7 +24,7 @@ void getFiltersWithValidFilters() {
        assertThat(filters).hasSize(1).containsOnly(MemoryFilter.TERM);
    }

-    @DisplayName("필터가 주어지지 않았을 때 빈 목록을 반환한다")
+    @DisplayName("필터가 주어지지 않았을 때 빈 필터 목록을 반환한다")


'필터 목록'이라고 하지 않으면, 정렬도 생각할 수 있기 때문에 확실히 좋은 것 같아요!

넵! 한글이 좋아서 한그롤...바꿧습니다 ㅋㅋ

Junyoung-WON

빠르게 잘 수정해주셨군요!! 고생하셨어요~ 😃

linirini added 4 commits January 19, 2025 11:15

feat: 중복 없이 MemoryFilter 가져오도록 수정

1c35abe

test: 대소문자 검증 테스트케이스 추가

c5173f8

fix: 요청 쿼리 스트링 파싱 사 NPE 문제 해결

84488b0

chore: dev 서버 중복 워크 플로우 비활성화

7c5d4c8

linirini added backend We are backend>< 🥵 hotfix 버그 및 오류 긴급 수정 (모두 모두 붙어라) labels Jan 19, 2025

linirini added this to the sprint-9 milestone Jan 19, 2025

linirini requested review from BurningFalls and Ho-Tea January 19, 2025 02:34

linirini self-assigned this Jan 19, 2025

linirini changed the title ~~fix: 필터링 API 파싱 오류 수정~~ fix: 필터링 API 파싱 오류 수정 #604 Jan 19, 2025

This was linked to issues Jan 19, 2025

fix: 추억 필터 및 정렬 query 조건 추가 #604

Closed

feat: 추억 목록 정렬, 필터링 API 구현 #546

Closed

Junyoung-WON reviewed Jan 19, 2025

View reviewed changes

BurningFalls requested changes Jan 19, 2025

View reviewed changes

linirini added 2 commits January 19, 2025 18:20

refactor: ! 연산 제거

c3e99e6

refactor: 조건문 변경

173a58c

linirini added 2 commits January 19, 2025 18:26

test: displayName 변경

0e9491d

test: displayName 변경

283d21f

linirini requested a review from BurningFalls January 19, 2025 09:27

BurningFalls approved these changes Jan 19, 2025

View reviewed changes

Junyoung-WON approved these changes Jan 19, 2025

View reviewed changes

linirini merged commit 5776e7f into develop Jan 19, 2025
2 checks passed

linirini deleted the hotfix/#546-fix-sort-and-filter branch January 19, 2025 10:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: 필터링 API 파싱 오류 수정 #604 #605

fix: 필터링 API 파싱 오류 수정 #604 #605

linirini commented Jan 19, 2025 •

edited

Loading

github-actions bot commented Jan 19, 2025 •

edited

Loading

github-actions bot commented Jan 19, 2025 •

edited

Loading

Junyoung-WON left a comment

Junyoung-WON Jan 19, 2025

Junyoung-WON Jan 19, 2025

BurningFalls left a comment •

edited by linirini

Loading

BurningFalls Jan 19, 2025

BurningFalls Jan 19, 2025

BurningFalls Jan 19, 2025

linirini Jan 19, 2025

BurningFalls Jan 19, 2025

linirini Jan 19, 2025

BurningFalls Jan 19, 2025 •

edited

Loading

linirini commented Jan 19, 2025

BurningFalls left a comment

BurningFalls Jan 19, 2025

linirini Jan 19, 2025

BurningFalls Jan 19, 2025

linirini Jan 19, 2025

Junyoung-WON left a comment

	@DisplayName("필터가 주어지지 않았을 때 빈 리스트를 반환한다")
	@DisplayName("필터가 주어지지 않았을 때 빈 MemoryFilter 리스트를 반환한다")

fix: 필터링 API 파싱 오류 수정 #604 #605

fix: 필터링 API 파싱 오류 수정 #604 #605

Conversation

linirini commented Jan 19, 2025 • edited Loading

⭐️ Issue Number

🚩 Summary

🛠️ Technical Concerns

🙂 To Reviewer

📋 To Do

github-actions bot commented Jan 19, 2025 • edited Loading

Test Results

github-actions bot commented Jan 19, 2025 • edited Loading

🌻Test Coverage Report

Junyoung-WON left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BurningFalls left a comment • edited by linirini Loading

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

BurningFalls Jan 19, 2025 • edited Loading

Choose a reason for hiding this comment

linirini commented Jan 19, 2025

BurningFalls 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

Junyoung-WON left a comment

Choose a reason for hiding this comment

linirini commented Jan 19, 2025 •

edited

Loading

github-actions bot commented Jan 19, 2025 •

edited

Loading

github-actions bot commented Jan 19, 2025 •

edited

Loading

BurningFalls left a comment •

edited by linirini

Loading

BurningFalls Jan 19, 2025 •

edited

Loading