-
Notifications
You must be signed in to change notification settings - Fork 2
[REFACTOR] recent-summary 연도 fallback 및 미존재 엔드포인트 404 처리 #428
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
Changes from 2 commits
db8e1c6
ecb6bba
032ee43
ee50618
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ | |
| import org.springframework.web.bind.MethodArgumentNotValidException; | ||
| import org.springframework.web.bind.annotation.ExceptionHandler; | ||
| import org.springframework.web.bind.annotation.RestControllerAdvice; | ||
| import org.springframework.web.servlet.NoHandlerFoundException; | ||
|
|
||
| @Slf4j | ||
| @RestControllerAdvice | ||
|
|
@@ -38,4 +39,10 @@ protected ResponseEntity<ErrorResponse> handleMethodArgumentsNotValidException(M | |
| return ResponseEntity.badRequest() | ||
| .body(ErrorResponse.of(e.getBindingResult())); | ||
| } | ||
|
|
||
| @ExceptionHandler(NoHandlerFoundException.class) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 요거 테스트 추가 해주시면 좋을 것 같아요!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 넵 확인했습니다! 수정이랑 추가해서 머지해둘게요 |
||
| protected ResponseEntity<ErrorResponse> handleNotFoundEndpointException(NoHandlerFoundException e) { | ||
| return ResponseEntity.status(HttpStatus.NOT_FOUND) | ||
| .body(ErrorResponse.of("요청한 엔드포인트를 찾을 수 없습니다.")); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -70,7 +70,8 @@ public LeagueRecentSummaryResponse findRecentSummary(Integer year, Integer recor | |
| int safeTopScorerLimit = Math.max(topScorerLimit, 0); | ||
|
|
||
| LocalDateTime now = LocalDateTime.now(); | ||
| LocalDateTime yearStart = LocalDateTime.of(year, 1, 1, 0, 0); | ||
| int targetYear = getTargetYear(year, now); | ||
| LocalDateTime yearStart = LocalDateTime.of(targetYear, 1, 1, 0, 0); | ||
| LocalDateTime yearEnd = yearStart.plusYears(1); | ||
|
|
||
| List<LeagueRecentSummaryResponse.LeagueRecord> records = safeRecordLimit == 0 | ||
|
|
@@ -81,7 +82,7 @@ public LeagueRecentSummaryResponse findRecentSummary(Integer year, Integer recor | |
|
|
||
| List<PlayerGoalCountWithRank> topScorerResults = safeTopScorerLimit == 0 | ||
| ? Collections.emptyList() | ||
| : leagueTopScorerRepository.findTopPlayersByYearWithTotalGoals(year, PageRequest.of(0, safeTopScorerLimit)); | ||
| : leagueTopScorerRepository.findTopPlayersByYearWithTotalGoals(targetYear, PageRequest.of(0, safeTopScorerLimit)); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
|
|
||
| Map<Long, String> unitByPlayerId = getUnitByPlayerId(topScorerResults.stream() | ||
| .map(PlayerGoalCountWithRank::playerId) | ||
|
|
@@ -101,6 +102,15 @@ public LeagueRecentSummaryResponse findRecentSummary(Integer year, Integer recor | |
| return new LeagueRecentSummaryResponse(records, topScorers); | ||
| } | ||
|
|
||
| private int getTargetYear(Integer year, LocalDateTime now) { | ||
| if (year != null) { | ||
| return year; | ||
| } | ||
| return leagueQueryRepository.findRecentFinishedLeagueYears(now, PageRequest.of(0, 1)).stream() | ||
| .findFirst() | ||
| .orElse(now.getYear()); | ||
| } | ||
|
|
||
| private Map<Long, String> getUnitByPlayerId(List<Long> playerIds) { | ||
| if (playerIds.isEmpty()) { | ||
| return Collections.emptyMap(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| package com.sports.server.common.advice; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
| import static org.junit.jupiter.api.Assertions.assertAll; | ||
|
|
||
| import com.sports.server.common.dto.ErrorResponse; | ||
| import com.sports.server.support.AcceptanceTest; | ||
| import io.restassured.RestAssured; | ||
| import io.restassured.response.ExtractableResponse; | ||
| import io.restassured.response.Response; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.springframework.http.HttpStatus; | ||
| import org.springframework.http.MediaType; | ||
| import org.springframework.test.context.TestPropertySource; | ||
|
|
||
| @TestPropertySource(properties = { | ||
| "spring.mvc.throw-exception-if-no-handler-found=true", | ||
| "spring.web.resources.add-mappings=false" | ||
| }) | ||
|
Comment on lines
+16
to
+19
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이건 왜 필요한가요? 저희 기존 패턴이랑 안맞는 것 같아요!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 404 자체가 아니라 NoHandlerFoundException 핸들러가 실제로 호출되는지 검증하려고 넣었습니다!
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 아 MVC 레이어에서 바로 걸러져서 그렇군요 이해했습니다 👍👍 머지 고고 |
||
| class ControllerExceptionAdviceAcceptanceTest extends AcceptanceTest { | ||
|
|
||
| @Test | ||
| void 존재하지_않는_엔드포인트_요청시_NoHandlerFoundException_핸들러로_404를_반환한다() { | ||
| // when | ||
| ExtractableResponse<Response> response = RestAssured.given().log().all() | ||
| .contentType(MediaType.APPLICATION_JSON_VALUE) | ||
| .get("/not-exists") | ||
| .then().log().all() | ||
| .extract(); | ||
|
|
||
| // then | ||
| ErrorResponse actual = toResponse(response, ErrorResponse.class); | ||
| assertAll( | ||
| () -> assertThat(response.statusCode()).isEqualTo(HttpStatus.NOT_FOUND.value()), | ||
| () -> assertThat(actual.getMessage()).isEqualTo("요청한 엔드포인트를 찾을 수 없습니다.") | ||
| ); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
커밋 두개로 분리해주셨으면 더 좋았을 것 같아요! 👍👍