[REFACTOR] recent-summary 연도 fallback 및 미존재 엔드포인트 404 처리#428
[REFACTOR] recent-summary 연도 fallback 및 미존재 엔드포인트 404 처리#428
Conversation
Summary of ChangesHello @TAEW00KIM, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This PR successfully implements fallback logic for the year parameter in the recent-summary endpoint, making it optional and selecting the most recent finished league year. It also adds a global exception handler for NoHandlerFoundException to provide a clean 404 response for non-existent endpoints. While the logic improvements are sound, a critical security concern has been identified: a potential unbounded resource allocation issue exists where the topScorerLimit parameter is used in a database query without an upper bound check, which could be exploited to cause a Denial of Service. Additionally, there are a few minor suggestions to improve code quality and maintainability.
| 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.
The topScorerLimit parameter is used to create a PageRequest without an upper bound check. An attacker can provide an extremely large value for this parameter (e.g., 2147483647), which may lead to excessive memory allocation or database performance issues, potentially causing a Denial of Service (DoS). It is recommended to validate the input and enforce a maximum allowable limit (e.g., 100).
| @ExceptionHandler(NoHandlerFoundException.class) | ||
| protected ResponseEntity<ErrorResponse> handleNotFoundEndpointException(NoHandlerFoundException e) { | ||
| return ResponseEntity.status(HttpStatus.NOT_FOUND) | ||
| .body(ErrorResponse.of("요청한 엔드포인트를 찾을 수 없습니다.")); |
There was a problem hiding this comment.
오류 메시지를 문자열 리터럴로 직접 사용하는 것보다 상수로 정의하여 관리하는 것이 좋습니다. 이렇게 하면 메시지의 일관성을 유지하고, 향후 수정이 필요할 때 더 쉽게 관리할 수 있습니다. 이 클래스 내에 private static final String 상수로 정의하거나, 별도의 ErrorMessage 클래스를 만들어 관리하는 것을 고려해 보세요.
| } | ||
| return leagueQueryRepository.findRecentFinishedLeagueYears(now, PageRequest.of(0, 1)).stream() | ||
| .findFirst() | ||
| .orElse(LocalDate.now().getYear()); |
There was a problem hiding this comment.
fallback 로직에서 LocalDate.now()를 새로 호출하는 대신, 파라미터로 전달받은 now를 사용하는 것이 일관성 있고 테스트 용이성 측면에서 더 좋습니다. now.getYear()를 사용하도록 수정하는 것을 제안합니다.
| .orElse(LocalDate.now().getYear()); | |
| .orElse(now.getYear()); |
| 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; |
There was a problem hiding this comment.
커밋 두개로 분리해주셨으면 더 좋았을 것 같아요! 👍👍
| } | ||
| return leagueQueryRepository.findRecentFinishedLeagueYears(now, PageRequest.of(0, 1)).stream() | ||
| .findFirst() | ||
| .orElse(LocalDate.now().getYear()); |
| .body(ErrorResponse.of(e.getBindingResult())); | ||
| } | ||
|
|
||
| @ExceptionHandler(NoHandlerFoundException.class) |
There was a problem hiding this comment.
요거 테스트 추가 해주시면 좋을 것 같아요! RestAssured 활용 해 보시면 될 것 같습니당
There was a problem hiding this comment.
넵 확인했습니다! 수정이랑 추가해서 머지해둘게요
| @TestPropertySource(properties = { | ||
| "spring.mvc.throw-exception-if-no-handler-found=true", | ||
| "spring.web.resources.add-mappings=false" | ||
| }) |
There was a problem hiding this comment.
이건 왜 필요한가요? 저희 기존 패턴이랑 안맞는 것 같아요!
There was a problem hiding this comment.
404 자체가 아니라 NoHandlerFoundException 핸들러가 실제로 호출되는지 검증하려고 넣었습니다!
기존에는 없는 엔드포인트 요청이 404 처리로만 끝날 수도 있어서 핸들러 테스트가 안되더라구요. 테스트에서만 예외 강제로 실행시키려고 넣었는데 빼야한다면 다시 빼서 커밋할게요
There was a problem hiding this comment.
아 MVC 레이어에서 바로 걸러져서 그렇군요 이해했습니다 👍👍 머지 고고
머지 하시고 성민님한테 전달 주시면 좋을 것 같아요!
Jin409
force-pushed
the
refactor/recent-summary-latest-year-404
branch
from
fd83279 to
ecb6bba
Compare
2 participants
이슈 배경
기존의 문제
해결 방식
확인해야 할 부분