Skip to content
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

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

Merged
merged 8 commits into from
Jan 19, 2025
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions .github/workflows/backend-ci-cd-dev.yml
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
name: Backend CI/CD dev

on:
push:
paths: [ 'backend/**', '.github/**' ]
branches: [ "develop-be", "develop" ]
workflow_dispatch:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


jobs:
ci:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
name: Backend CI/CD multi prod
name: Backend CI/CD multi dev

on:
push:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

.toList();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import java.util.Arrays;
import java.util.List;
import java.util.Objects;
import com.staccato.memory.service.MemoryFilter;
import com.staccato.memory.service.MemorySort;
import io.swagger.v3.oas.annotations.media.Schema;
Expand All @@ -16,12 +17,19 @@ public record MemoryReadRequest(
private static final String DELIMITER = ",";

public List<MemoryFilter> getFilters() {
List<String> filters = Arrays.stream(this.filters.split(DELIMITER))
List<String> filters = parseFilters().stream()
.map(String::trim)
.toList();
return MemoryFilter.findAllByName(filters);
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


public MemorySort getSort() {
return MemorySort.findByName(sort);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ void findByNameWithValidFilter() {
);
}

@DisplayName("필터링명이 주어졌을 때 유효한 MemoryFilter 목록 민 반환한다.")
@DisplayName("필터링명이 주어졌을 때 유효한 MemoryFilter 목록만 반환한다.")
@Test
void findAllByNameIfOnlyValid() {
// when
Expand All @@ -49,6 +49,19 @@ void findAllByNameIfOnlyValid() {
);
}

@DisplayName("필터링명이 중복으로 주어졌을 때 중복 없이 MemoryFilter 목록을 반환한다.")
@Test
void findAllByNameDistinct() {
// when
List<MemoryFilter> result = MemoryFilter.findAllByName(List.of("TERM", "term"));

// then
assertAll(
() -> assertThat(result).hasSize(1),
() -> assertThat(result.get(0)).isEqualTo(MemoryFilter.TERM)
);
}

@DisplayName("유효하지 않거나 null인 정렬명이 주어졌을 때 빈 값을 반환한다.")
@Test
void findByNameWithInvalid() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.CsvSource;
import org.junit.jupiter.params.provider.NullSource;
import org.junit.jupiter.params.provider.ValueSource;
import org.springframework.beans.factory.annotation.Autowired;
Expand All @@ -26,13 +27,21 @@ public class MemorySortTest extends ServiceSliceTest {
private MemoryRepository memoryRepository;

@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);
}
Comment on lines 29 to 45
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
    );
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


@DisplayName("유효하지 않거나 null인 정렬명이 주어졌을 때 기본값인 UPDATED를 반환한다.")
Expand Down
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
package com.staccato.memory.service.dto.request;

import java.util.List;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.NullAndEmptySource;
import com.staccato.memory.service.MemoryFilter;
import com.staccato.memory.service.MemorySort;

import static org.assertj.core.api.Assertions.assertThat;

class MemoryReadRequestTest {
@DisplayName("필터가 주어졌을 때 올바른 MemoryFilter 리스트를 반환한다")
@Test
void getFiltersWithValidFilters() {
// given
MemoryReadRequest request = new MemoryReadRequest("TERM, term", "NEWEST");

// when
List<MemoryFilter> filters = request.getFilters();

// then
assertThat(filters).hasSize(1).containsOnly(MemoryFilter.TERM);
}

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

@BurningFalls BurningFalls Jan 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@ParameterizedTest
@NullAndEmptySource
void getFiltersWithNullOrEmptyFilters(String filters) {
// given
MemoryReadRequest request = new MemoryReadRequest(filters, "NEWEST");

// when
List<MemoryFilter> result = request.getFilters();

// then
assertThat(result).isEmpty();
}

@DisplayName("정렬이 주어지지 않았을 때 기본 정렬 기준을 반환한다")
@ParameterizedTest
@NullAndEmptySource
void getSortWithNullOrEmptyFilters(String sort) {
// given
MemoryReadRequest request = new MemoryReadRequest(null, sort);

// when
MemorySort result = request.getSort();

// then
assertThat(result).isEqualTo(MemorySort.UPDATED);
}
}
Loading