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

[BE] feat: 축제 조회 필터링 구현(#602) #603

Merged
merged 14 commits into from
Nov 13, 2023
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ public enum ErrorCode {
DELETE_CONSTRAINT_STAGE("티켓이 등록된 공연은 삭제할 수 없습니다."),
DELETE_CONSTRAINT_SCHOOL("학생 또는 축제에 등록된 학교는 삭제할 수 없습니다."),
DUPLICATE_SCHOOL("이미 존재하는 학교 정보입니다."),
INVALID_FESTIVAL_FILTER("유효하지 않은 필터 값입니다."),
Copy link
Collaborator

Choose a reason for hiding this comment

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

축제 라는 단어가 들어가도 좋겠네요.
ex) 올바르지 않은 축제의 필터 값 입니다.



// 401
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import com.festago.festival.dto.FestivalResponse;
import com.festago.festival.dto.FestivalUpdateRequest;
import com.festago.festival.dto.FestivalsResponse;
import com.festago.festival.repository.FestivalFilter;
import com.festago.festival.repository.FestivalRepository;
import com.festago.school.domain.School;
import com.festago.school.repository.SchoolRepository;
Expand All @@ -19,8 +20,8 @@
import java.time.Clock;
import java.time.LocalDate;
import java.util.List;
import org.springframework.dao.DataIntegrityViolationException;
import lombok.RequiredArgsConstructor;
import org.springframework.dao.DataIntegrityViolationException;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;

Expand Down Expand Up @@ -49,8 +50,8 @@ private void validate(Festival festival) {
}

@Transactional(readOnly = true)
public FestivalsResponse findAll() {
List<Festival> festivals = festivalRepository.findAll();
public FestivalsResponse findFestivals(FestivalFilter festivalFilter) {
List<Festival> festivals = festivalRepository.findAll(festivalFilter.getSpecification());
return FestivalsResponse.from(festivals);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package com.festago.festival.repository;

import static com.festago.festival.repository.FestivalSpecification.afterEndDate;
import static com.festago.festival.repository.FestivalSpecification.afterStartDate;
import static com.festago.festival.repository.FestivalSpecification.all;
import static com.festago.festival.repository.FestivalSpecification.beforeEndDate;
import static com.festago.festival.repository.FestivalSpecification.beforeStartDate;

import com.festago.common.exception.BadRequestException;
import com.festago.common.exception.ErrorCode;
import com.festago.festival.domain.Festival;
import java.time.LocalDate;
import java.util.Arrays;
import java.util.function.Function;
import org.springframework.data.jpa.domain.Specification;

public enum FestivalFilter {
ALL(currentTime -> all()),
PROGRESS(currentTime -> afterStartDate(currentTime)
.and(beforeEndDate(currentTime))),
Copy link
Collaborator

Choose a reason for hiding this comment

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

진행중인 축제의 스펙이 다음과 같이 되어있네요.

public static Specification<Festival> afterStartDate(LocalDate currentTime) {
    return (root, query, criteriaBuilder) -> criteriaBuilder.lessThan(root.get(START_DATE), currentTime);
}

public static Specification<Festival> beforeEndDate(LocalDate currentTime) {
    return (root, query, criteriaBuilder) -> criteriaBuilder.greaterThan(root.get(END_DATE), currentTime);
}

이때 lessThan, greaterThan 이라면 당일에만 진행하는 축제에 대해 당일에 조회하면 조회가 되지 않을 것 같네요.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Test
void 현재_진행_축제_넘기기_오늘시작_내일끝() {
    // given
                                                              ...
    festivalRepository.save(new Festival("name", now.minusDays(0), now.plusDays(1), school));

    // when
    int actual = festivalRepository.findAll(FestivalFilter.PROGRESS.getSpecification()).size();

    // then
    assertThat(actual).isEqualTo(1);
}

@Test
void 현재_진행_축제_넘기기_어제시작_오늘끝() {
    // given
                                                              ...
    festivalRepository.save(new Festival("name", now.minusDays(1), now.plusDays(0), school));

    // when
    int actual = festivalRepository.findAll(FestivalFilter.PROGRESS.getSpecification()).size();

    // then
    assertThat(actual).isEqualTo(1);
}

글렌이 말씀해준것 처럼 이 테스트 코드들이 통과하기 위해선 eqaulsTo 조건이 들어가야겠네요 !!
오늘이 11월 11일이라면, 11월 11일에 시작하는 축제 및 11월 11일에 끝나는 축제가 조회되어야 할테니까요.

Copy link
Member Author

Choose a reason for hiding this comment

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

헉 수정했습니다

PLANNED(currentTime -> beforeStartDate(currentTime)),
END(currentTime -> afterEndDate(currentTime));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Enum에서 스펙을 조합하여 조건을 맞추기 보단, FestivalSpecification 객체가 명시적으로 스펙을 던져주는 것은 어떤가요?

Suggested change
ALL(currentTime -> all()),
PROGRESS(currentTime -> afterStartDate(currentTime)
.and(beforeEndDate(currentTime))),
PLANNED(currentTime -> beforeStartDate(currentTime)),
END(currentTime -> afterEndDate(currentTime));
ALL(FestivalSpecification::all),
PROGRESS(FestivalSpecification::progress),
PLANNED(FestivalSpecification::beforeStartDate),
END(FestivalSpecification::afterEndDate);
@NoArgsConstructor(access = AccessLevel.PRIVATE)
public class FestivalSpecification {

    private static final String START_DATE = "startDate";
    private static final String END_DATE = "endDate";

    public static Specification<Festival> all(LocalDate currentTime) { // 메서드 참조를 위해 파라미터 추가
        return (root, query, criteriaBuilder) -> criteriaBuilder.conjunction();
    }

    public static Specification<Festival> progress(LocalDate currentTime) {
        return (root, query, criteriaBuilder) -> criteriaBuilder.and(
            criteriaBuilder.lessThan(root.get(START_DATE), currentTime), // lessThanOrEqualTo 고려할 것
            criteriaBuilder.greaterThan(root.get(END_DATE), currentTime) // greaterThanOrEqualTo 고려할 것
        );
    }

    public static Specification<Festival> beforeStartDate(LocalDate currentTime) {
        return (root, query, criteriaBuilder) -> criteriaBuilder.greaterThan(root.get(START_DATE), currentTime);
    }

    public static Specification<Festival> afterEndDate(LocalDate currentTime) {
        return (root, query, criteriaBuilder) -> criteriaBuilder.lessThan(root.get(END_DATE), currentTime);
    }
}

Copy link
Collaborator

@seokjin8678 seokjin8678 Nov 9, 2023

Choose a reason for hiding this comment

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

beforeStartDate, afterEndDate는 알잘딱하게 해주세요.

Copy link
Collaborator

Choose a reason for hiding this comment

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

저도 글렌이 말씀하신 부분에 동의합니다. Specification method가 Progress 같은 경우 외부에서 and 조건으로 조합해주는 형식이 아니라 명확하게 완성된 조건을 뱉어주는게 더 이해하기 쉽고, 테스트하기 좋은 코드라고 여겨져요.

Copy link
Member Author

Choose a reason for hiding this comment

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

반영했습니다!


private final Function<LocalDate, Specification<Festival>> filter;

FestivalFilter(Function<LocalDate, Specification<Festival>> filter) {
this.filter = filter;
}
Comment on lines +17 to +19
Copy link
Member

Choose a reason for hiding this comment

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

이녀석은 왜 접근제어자가 없나요?!

Copy link
Member Author

Choose a reason for hiding this comment

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

Enum 생성자 접근제어자에 뭘 썼는지 기억이 안나요 이걸 따로 명시해줬나요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Enum은 접근제어자가 필요하지 않습니다..!

Copy link
Member

Choose a reason for hiding this comment

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

아이고 이놈 enum이었네

Copy link
Member Author

Choose a reason for hiding this comment

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

라임 디졌다 ;;


public static FestivalFilter from(String filterName) {
return Arrays.stream(FestivalFilter.values())
.filter(festivalFilter -> festivalFilter.name().equalsIgnoreCase(filterName))
.findAny()
.orElseThrow(() -> new BadRequestException(ErrorCode.INVALID_FESTIVAL_FILTER));
}
Copy link
Collaborator

@seokjin8678 seokjin8678 Nov 9, 2023

Choose a reason for hiding this comment

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

크게 고려해야 할 상황은 아니지만...
스트림을 매번 돌리기 보단 스위치를 사용하는게 좋아 보이네요.
물론 새로운 Filter가 추가되면 해당 코드를 변경해야 합니다.

Suggested change
public static FestivalFilter from(String filterName) {
return Arrays.stream(FestivalFilter.values())
.filter(festivalFilter -> festivalFilter.name().equalsIgnoreCase(filterName))
.findAny()
.orElseThrow(() -> new BadRequestException(ErrorCode.INVALID_FESTIVAL_FILTER));
}
public static FestivalFilter from(String filterName) {
return switch (filterName) { // filterName.toUpperCase() 고려
case "ALL" -> ALL;
case "PROGRESS" -> PROGRESS;
case "PLANNED" -> PLANNED;
case "END" -> END;
default -> throw new BadRequestException(ErrorCode.INVALID_FESTIVAL_FILTER);
};
}

Copy link
Member Author

Choose a reason for hiding this comment

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

저도 연산마다 Stream 이 생성되는 것이 비효율적이라 생각했지만
Java 코드 레벨에서의 비효율이 실질적으로 미치는 영향이 크지 않다 생각했습니다.
하지만 But 한번 적용해보는 것도 좋아보여 반영했습니다!


public Specification<Festival> getSpecification() {
return filter.apply(LocalDate.now());
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Clock을 매개변수로 받는게 좋아 보이네요.

}
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@

import com.festago.festival.domain.Festival;
import org.springframework.data.jpa.repository.JpaRepository;
import org.springframework.data.jpa.repository.JpaSpecificationExecutor;

public interface FestivalRepository extends JpaRepository<Festival, Long> {
public interface FestivalRepository extends JpaRepository<Festival, Long>, JpaSpecificationExecutor<Festival> {

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package com.festago.festival.repository;

import com.festago.festival.domain.Festival;
import java.time.LocalDate;
import lombok.AccessLevel;
import lombok.NoArgsConstructor;
import org.springframework.data.jpa.domain.Specification;

@NoArgsConstructor(access = AccessLevel.PRIVATE)
public class FestivalSpecification {

private static final String START_DATE = "startDate";
private static final String END_DATE = "endDate";

public static Specification<Festival> all() {
return (root, query, criteriaBuilder) -> criteriaBuilder.conjunction();
}

public static Specification<Festival> afterStartDate(LocalDate currentTime) {
return (root, query, criteriaBuilder) -> criteriaBuilder.lessThan(root.get(START_DATE), currentTime);
}

public static Specification<Festival> beforeStartDate(LocalDate currentTime) {
return (root, query, criteriaBuilder) -> criteriaBuilder.greaterThan(root.get(START_DATE), currentTime);
}

public static Specification<Festival> afterEndDate(LocalDate currentTime) {
return (root, query, criteriaBuilder) -> criteriaBuilder.lessThan(root.get(END_DATE), currentTime);
}

public static Specification<Festival> beforeEndDate(LocalDate currentTime) {
return (root, query, criteriaBuilder) -> criteriaBuilder.greaterThan(root.get(END_DATE), currentTime);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

위의 커멘트에도 남기긴 했지만...
다음과 같이 메서드가 하는 일을 명확하게 하는것은 어떤가요

Suggested change
public static Specification<Festival> all() {
return (root, query, criteriaBuilder) -> criteriaBuilder.conjunction();
}
public static Specification<Festival> afterStartDate(LocalDate currentTime) {
return (root, query, criteriaBuilder) -> criteriaBuilder.lessThan(root.get(START_DATE), currentTime);
}
public static Specification<Festival> beforeStartDate(LocalDate currentTime) {
return (root, query, criteriaBuilder) -> criteriaBuilder.greaterThan(root.get(START_DATE), currentTime);
}
public static Specification<Festival> afterEndDate(LocalDate currentTime) {
return (root, query, criteriaBuilder) -> criteriaBuilder.lessThan(root.get(END_DATE), currentTime);
}
public static Specification<Festival> beforeEndDate(LocalDate currentTime) {
return (root, query, criteriaBuilder) -> criteriaBuilder.greaterThan(root.get(END_DATE), currentTime);
}
public static Specification<Festival> all(LocalDate currentTime) { // 메서드 참조를 위해 파라미터 추가
return (root, query, criteriaBuilder) -> criteriaBuilder.conjunction();
}
public static Specification<Festival> progress(LocalDate currentTime) {
return (root, query, criteriaBuilder) -> criteriaBuilder.and(
criteriaBuilder.lessThan(root.get(START_DATE), currentTime), // lessThanOrEqualTo 고려할 것
criteriaBuilder.greaterThan(root.get(END_DATE), currentTime) // greaterThanOrEqualTo 고려할 것
);
}
public static Specification<Festival> beforeStartDate(LocalDate currentTime) {
return (root, query, criteriaBuilder) -> criteriaBuilder.greaterThan(root.get(START_DATE), currentTime);
}
public static Specification<Festival> afterEndDate(LocalDate currentTime) {
return (root, query, criteriaBuilder) -> criteriaBuilder.lessThan(root.get(END_DATE), currentTime);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,15 @@
import com.festago.festival.application.FestivalService;
import com.festago.festival.dto.FestivalDetailResponse;
import com.festago.festival.dto.FestivalsResponse;
import com.festago.festival.repository.FestivalFilter;
import io.swagger.v3.oas.annotations.Operation;
import io.swagger.v3.oas.annotations.tags.Tag;
import lombok.RequiredArgsConstructor;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.PathVariable;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.bind.annotation.RestController;

@RestController
Expand All @@ -21,9 +23,10 @@ public class FestivalController {
private final FestivalService festivalService;

@GetMapping
@Operation(description = "모든 축제들을 조회한다.", summary = "축제 목록 조회")
public ResponseEntity<FestivalsResponse> findAll() {
FestivalsResponse response = festivalService.findAll();
@Operation(description = "축제를 조건별로 조회한다. ALL: 전체, PROGRESS: 진행 중, PLANNED: 진행 예정, END: 종료", summary = "축제 목록 조회")
public ResponseEntity<FestivalsResponse> findFestivals(
@RequestParam(defaultValue = "ALL") String festivalFilter) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

카톡에서 이야기 했던대로 전체가 없어진다면 default는 PROGRESS로 처리하는게 맞죠?_?

Copy link
Member Author

Choose a reason for hiding this comment

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

네네 맞습니다!

FestivalsResponse response = festivalService.findFestivals(FestivalFilter.from(festivalFilter));
Copy link
Member

Choose a reason for hiding this comment

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

default ALL 👍

return ResponseEntity.ok()
.body(response);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import com.festago.festival.dto.FestivalDetailStageResponse;
import com.festago.festival.dto.FestivalResponse;
import com.festago.festival.dto.FestivalsResponse;
import com.festago.festival.repository.FestivalFilter;
import com.festago.festival.repository.FestivalRepository;
import com.festago.school.domain.School;
import com.festago.school.repository.SchoolRepository;
Expand All @@ -40,6 +41,7 @@
import org.mockito.Mock;
import org.mockito.Spy;
import org.mockito.junit.jupiter.MockitoExtension;
import org.springframework.data.jpa.domain.Specification;

@ExtendWith(MockitoExtension.class)
@DisplayNameGeneration(ReplaceUnderscores.class)
Expand All @@ -66,10 +68,10 @@ class FestivalServiceTest {
// given
Festival festival1 = FestivalFixture.festival().id(1L).build();
Festival festival2 = FestivalFixture.festival().id(2L).build();
given(festivalRepository.findAll()).willReturn(List.of(festival1, festival2));
given(festivalRepository.findAll(any(Specification.class))).willReturn(List.of(festival1, festival2));

// when
FestivalsResponse response = festivalService.findAll();
FestivalsResponse response = festivalService.findFestivals(FestivalFilter.ALL);

// then
List<Long> festivalIds = response.festivals().stream().map(FestivalResponse::id).toList();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package com.festago.festival.domain;

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

import com.festago.common.exception.BadRequestException;
import com.festago.festival.repository.FestivalFilter;
import org.junit.jupiter.api.DisplayNameGeneration;
import org.junit.jupiter.api.DisplayNameGenerator.ReplaceUnderscores;
import org.junit.jupiter.api.Test;

@DisplayNameGeneration(ReplaceUnderscores.class)
@SuppressWarnings("NonAsciiCharacters")
class FestivalFilterTest {

@Test
void 유요하지_않은_name_이면_예외() {
// given && when && then
assertThatThrownBy(() -> FestivalFilter.from("unvalid"))
.isInstanceOf(BadRequestException.class);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
package com.festago.festival.repository;

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

import com.festago.festival.domain.Festival;
import com.festago.school.domain.School;
import com.festago.school.repository.SchoolRepository;
import java.time.LocalDate;
import java.util.List;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.DisplayNameGeneration;
import org.junit.jupiter.api.DisplayNameGenerator.ReplaceUnderscores;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.autoconfigure.orm.jpa.DataJpaTest;

@DisplayNameGeneration(ReplaceUnderscores.class)
@SuppressWarnings("NonAsciiCharacters")
@DataJpaTest
class FestivalRepositoryTest {

private static final String CURRENT_FESTIVAL = "현재 축제";
private static final String PAST_FESTIVAL = "과거 축제";
private static final String FUTURE_FESTIVAL = "미래 축제";

@Autowired
FestivalRepository festivalRepository;

@Autowired
SchoolRepository schoolRepository;

@BeforeEach
public void setting() {
School school = schoolRepository.save(new School("domain", "name"));
LocalDate now = LocalDate.now();
festivalRepository.save(new Festival(PAST_FESTIVAL, now.minusDays(10L), now.minusDays(8L), school));
festivalRepository.save(new Festival(CURRENT_FESTIVAL, now.minusDays(1L), now.plusDays(1L), school));
festivalRepository.save(new Festival(FUTURE_FESTIVAL, now.plusDays(1L), now.plusDays(3L), school));
Copy link
Collaborator

Choose a reason for hiding this comment

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

시간에 대한 테스트는 "2023-11-10" 과 같이 고정된 값을 사용하는 것은 어떤가요?

}

@Test
void 모든_축제_반환() {
// given
FestivalFilter filter = FestivalFilter.ALL;

// when
List<Festival> actual = festivalRepository.findAll(filter.getSpecification());

// then
assertThat(actual).hasSize(3);
}

@Test
void 진행_예정_축제_반환() {
// given
FestivalFilter filter = FestivalFilter.PLANNED;

// when
List<Festival> actual = festivalRepository.findAll(filter.getSpecification());

// then
assertSoftly(softAssertions -> {
assertThat(actual).hasSize(1);
assertThat(actual.get(0)).matches(festival -> festival.getName().equals(FUTURE_FESTIVAL));
});
}

@Test
void 진행_중_축제_반환() {
// given
FestivalFilter filter = FestivalFilter.PROGRESS;

// when
List<Festival> actual = festivalRepository.findAll(filter.getSpecification());

// then
assertSoftly(softAssertions -> {
assertThat(actual).hasSize(1);
assertThat(actual.get(0)).matches(festival -> festival.getName().equals(CURRENT_FESTIVAL));
});
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

당일에 해당하는 축제에 대한 테스트 코드도 필요할 것 같네요.

Copy link
Member Author

Choose a reason for hiding this comment

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

반영했습니다!


@Test
void 종료_축제_반환() {
// given
FestivalFilter filter = FestivalFilter.END;

// when
List<Festival> actual = festivalRepository.findAll(filter.getSpecification());

// then
assertSoftly(softAssertions -> {
assertThat(actual).hasSize(1);
assertThat(actual.get(0)).matches(festival -> festival.getName().equals(PAST_FESTIVAL));
});
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.festago.presentation;

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyLong;
import static org.mockito.BDDMockito.given;
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get;
Expand All @@ -12,6 +13,7 @@
import com.festago.festival.dto.FestivalDetailResponse;
import com.festago.festival.dto.FestivalResponse;
import com.festago.festival.dto.FestivalsResponse;
import com.festago.festival.repository.FestivalFilter;
import com.festago.support.CustomWebMvcTest;
import java.nio.charset.StandardCharsets;
import java.time.LocalDate;
Expand Down Expand Up @@ -47,7 +49,7 @@ class FestivalControllerTest {
FestivalResponse festivalResponse2 = new FestivalResponse(2L, 2L, "우테대학교", LocalDate.now().minusDays(3),
LocalDate.now(), "https://image2.png");
FestivalsResponse expected = new FestivalsResponse(List.of(festivalResponse1, festivalResponse2));
given(festivalService.findAll())
given(festivalService.findFestivals(any(FestivalFilter.class)))
.willReturn(expected);

// when & then
Expand Down
Loading