[BE] "/api/v1/festivals"의 "filter" 쿼리 파리미터의 기본 값을 지정한다.

seokjin8678 commented 7 months ago

✨ 세부 내용

다음과 같이 /api/v1/festivals 요청에서 FestivalV1QueryDslRepository의 85번째 라인에서 NPE가 발생합니다.

[2024-02-07 11:22:10:67524785] [http-nio-8081-exec-6] ERROR [com.festago.common.handler.GlobalExceptionHandler.logError:147] - 
[🔴ERROR] - (GET /api/v1/festivals)
(id: null, role: ANONYMOUS) java.lang.NullPointerException: Cannot invoke "com.festago.festival.repository.FestivalFilter.ordinal()" because "filter" is null
  at com.festago.festival.repository.FestivalV1QueryDslRepository.cursorBasedWhere(FestivalV1QueryDslRepository.java:85)

원인은 switch의 인자로 들어가는 filter가 null이기 때문에 발생하는 문제입니다.

private BooleanExpression cursorBasedWhere(...) {
    BooleanExpression filterResult = switch (filter) {
        ...
    }
    ...
}

API 스펙에서 filter 쿼리 파리미터는 기본 값으로 PROGRESS가 되어야 하지만, 기본 값이 설정되어 있지 않기 때문에 위와 같은 문제가 발생합니다.

/api/v1/festivals API는 다음과 같이 FestivalV1Controller에 구현되어 있습니다.

@RequestMapping("/api/v1/festivals")
public class FestivalV1Controller {
    ...
    @GetMapping
    public ResponseEntity<Slice<FestivalV1Response>> findFestivals(
        @PageableDefault(size = 10) Pageable pageable,
        FestivalV1QueryRequest request
    ) {
        ...
    }
}

여기서 filter 쿼리 파라미터는 FestivalV1QueryRequest의 필드에 포함되어 있습니다.

public record FestivalV1QueryRequest(
    SchoolRegion location,
    FestivalFilter filter,
    Long lastFestivalId,
    LocalDate lastStartDate
) {
    ...
}

FestivalV1QueryRequest는 record이기 때문에 불변하여, 중간에 값을 바꿔치기 할 수 없습니다.

따라서 기본 값은 요청이 들어오고 난 뒤 설정해야 합니다.

제가 생각하기에 해결할 3가지 방법이 있는데, 다음과 같습니다.

FestivalV1QueryRequest getter 재정의
FestivalV1QueryRequest 사용하지 않고, 컨트롤러 메서드 파라미터에 @RequestParam 사용
FestivalV1QueryService에 filter가 null이면 기본 값을 설정하는 로직 추가

1. `FestivalV1QueryRequest` getter 재정의

record 클래스의 필드를 접근할 때 무조건 getter를 사용해야 하므로, 해당 getter를 재정의하면 해결할 수 있습니다.

public record FestivalV1QueryRequest(
    ...
) {

    @Override
    public FestivalFilter filter() {
        return filter == null ? FestivalFilter.PROGRESS : filter;
    }
}

2. `FestivalV1QueryRequest` 사용하지 않고, 컨트롤러 핸들러 메서드 파라미터에 `@RequestParam` 사용

다음과 같이 핸들러 메서드에서 FestivalV1QueryRequest 타입의 파라미터를 그대로 받고 있습니다.

@RequestMapping("/api/v1/festivals")
public class FestivalV1Controller {
    ...
    @GetMapping
    public ResponseEntity<Slice<FestivalV1Response>> findFestivals(
        @PageableDefault(size = 10) Pageable pageable,
        FestivalV1QueryRequest request
    ) {
        ...
    }
}

여기서 @RequestParam을 사용한다면 다음과 같이 해결할 수 있습니다.

@RequestMapping("/api/v1/festivals")
public class FestivalV1Controller {
    ...
    @GetMapping
    public ResponseEntity<Slice<FestivalV1Response>> findFestivals(
        @PageableDefault(size = 10) Pageable pageable,
        @RequestParam SchoolRegion location,
        @RequestParam(defaultValue = "PROGRESS") FestivalFilter filter,
        @RequestParam Long lastFestivalId,
        @RequestParam LocalDate lastStartDate
    ) {
        ...
    }
}

3. `FestivalV1QueryService`에 `filter`가 null이면 기본 값을 설정하는 로직 추가

FestivalV1QueryService에서 FestivalV1QueryDslRepository에 값을 전달할 때 현재 시간이 필요하므로 FestivalSearchCondition을 새로 만들어 전달합니다.

이때 request.filter()를 호출하여 생성자에 값을 채우는데, 다음과 같이 null이면 기본 값을 반환하는 메서드를 사용하면 해결할 수 있습니다.

@Service
public class FestivalV1QueryService {

    private final FestivalV1QueryDslRepository festivalV1QueryDslRepository;
    private final Clock clock;

    public Slice<FestivalV1Response> findFestivals(Pageable pageable, FestivalV1QueryRequest request) {
        return festivalV1QueryDslRepository.findBy(new FestivalSearchCondition(
            getDefaultFilterIfNull(request.filter()),
            request.location(),
            request.lastStartDate(),
            request.lastFestivalId(),
            pageable,
            LocalDate.now(clock)
        ));
    }

    private FestivalFilter getDefaultFilterIfNull(FestivalFilter filter) {
        if (filter == null) {
            return FestivalFilter.PROGRESS;
        }
        return filter;
    }
}

저는 명시적으로 쿼리 파리미터를 표시하고, 기본 값의 유무를 확실하게 알 수 있는 2번 방법이 괜찮아 보이네요!

다른 분들은 어떤 방법이 좋아 보이시나요?

⏰ 예상 소요 시간

30분

carsago commented 7 months ago

저는 @RequestParam(defaultValue = "PROGRESS") FestivalFilter filter와 같이 컨트롤러에서는 명시적으로 볼 수 있는 것이 좋다고 느껴집니다.

어차피 controller 메소드는 테스트를 제외하곤 중복돼서 사용되지 않기 때문에 인자를 줄이는 것도 큰 의미 없고, 명시적인 가독성이 중요하다고 여겨져요. 경험적으로도 Controller를 볼 때 한 뎁스 더 들어가는게 불편하기도 했고요.

다만 고민점은 이 필터 인자들을 service, repository에 한개 한개씩 다 넘길 것이냐는 부담감이 있네요. service, repository 인자가 변경되니 관련된 테스트도 모두 변경될 거니까요.

그래서 저번에 글렌이 말씀해주신 controller의 dto와 service의 Dto를 특정 영역에선 나누는 것도 가능할 것 같아요. (전에는 사용하기엔 부담이다 느꼈는데, 이 영역에선 충분할 것 같아요.)

구체적으로 Controller에서

  @RequestParam SchoolRegion location,
  @RequestParam(defaultValue = "PROGRESS") FestivalFilter filter,
  @RequestParam Long lastFestivalId,
  @RequestParam LocalDate lastStartDate

4개의 인자를 받고

현재의 FestivalV1QueryRequest와 동일한 형태의 DTO를 생성자로 만들어서, Sevice 단으로 넘겨주는 방법이 가능할 것 같습니다. lastFestivalId와 lastStartDate도 nullable 하기 때문에 관련된 부분의 코드도 들어갈 수 있을 것 같고요.

그리고 추가적으로 현재 SchoolRegion도 null 이 가능하고 지정하지 않은 경우에는 if null or 기타로 처리하는데 default로 SchoolRegion.기타를 넣어주는 것 어떤가요?

그리고 기타라는 명칭도 명확하지 않은 것 같아서 더 명료한 네이밍도 가능할 것 같네요.

seokjin8678 commented 7 months ago

그리고 추가적으로 현재 SchoolRegion도 null 이 가능하고 지정하지 않은 경우에는 if null or 기타로 처리하는데 default로 SchoolRegion.기타를 넣어주는 것 어떤가요?

그리고 기타라는 명칭도 명확하지 않은 것 같아서 더 명료한 네이밍도 가능할 것 같네요.

SchoolRegion이 한글로 서울, 부산 같이 한글로 되어 있으니 모두 보단 SchoolRegion.ALL, SchoolRegion.ANY 같은 값은 어떤가요?

woowacourse-teams / 2023-festa-go