Open
Conversation
📝 WalkthroughWalkthroughThis PR establishes common infrastructure by introducing standardized API response types (ApiResponse, ResultType), comprehensive error handling (ErrorCode, BusinessException), a reusable BaseEntity with JPA auditing, and rewrites GlobalExceptionHandler for unified error responses. The Track domain is refactored to use this new framework with soft-delete support. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/bcsd/internal/common/ApiResponse.java`:
- Around line 8-18: The canonical constructor for ApiResponse must validate that
result is not null; update the ApiResponse canonical constructor (the public
ApiResponse(...) compact constructor) to first check if result == null and throw
an IllegalArgumentException (or Objects.requireNonNull) with a clear message,
then keep the existing checks for ResultType.SUCCESS/ERROR against error and
data; reference the ResultType enum and the constructor parameters result, data,
and error when adding the null guard so new ApiResponse<>(null, null, null) is
rejected.
In `@src/main/java/com/bcsd/internal/common/GlobalExceptionHandler.java`:
- Around line 26-37: Split the long
handleValidation(MethodArgumentNotValidException) into a short orchestrator and
small private helpers: keep handleValidation to call three new helpers —
extractFieldErrors(MethodArgumentNotValidException) to return List<FieldError>
(perform the stream map there), buildErrorMessage(List<FieldError>) to construct
and return the ErrorMessage using ErrorCode.INVALID_INPUT_VALUE, and
buildErrorResponse(ErrorMessage) to assemble and return the
ResponseEntity<ApiResponse<Void>> with the proper HTTP status; make all helpers
private and keep each helper under 10 lines so handleValidation only delegates
to them.
- Around line 46-51: The log(ErrorCode errorCode, Exception e) method currently
uses a switch on errorCode.getLogLevel(); replace that switch with early-return
if branches: first check if errorCode.getLogLevel() == ERROR then call
log.error("BusinessException: {}", e.getMessage(), e) and return; next check for
WARN then call log.warn("BusinessException: {}", e.getMessage()) and return;
otherwise call log.info("BusinessException: {}", e.getMessage()). Keep the same
logging calls and message text (log.error, log.warn, log.info) and ensure the
method returns immediately after each matched branch.
In `@src/main/java/com/bcsd/internal/track/controller/TrackController.java`:
- Around line 43-46: The delete method in TrackController currently returns
ApiResponse.success() with HttpStatus.NO_CONTENT which violates 204 semantics;
change the response to return a 200 OK with the unified wrapper instead—update
TrackController.delete to use ResponseEntity.ok(ApiResponse.success()) (or
ResponseEntity.status(HttpStatus.OK).body(ApiResponse.success())), keeping the
call to trackService.delete(id) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d0be88dc-b561-4bf1-be29-742920676320
📒 Files selected for processing (24)
src/main/java/com/bcsd/internal/BcsdInternalApiApplication.javasrc/main/java/com/bcsd/internal/common/ApiResponse.javasrc/main/java/com/bcsd/internal/common/BaseEntity.javasrc/main/java/com/bcsd/internal/common/BusinessException.javasrc/main/java/com/bcsd/internal/common/ErrorCode.javasrc/main/java/com/bcsd/internal/common/ErrorMessage.javasrc/main/java/com/bcsd/internal/common/ErrorResponse.javasrc/main/java/com/bcsd/internal/common/FieldError.javasrc/main/java/com/bcsd/internal/common/GlobalExceptionHandler.javasrc/main/java/com/bcsd/internal/common/ResultType.javasrc/main/java/com/bcsd/internal/track/Track.javasrc/main/java/com/bcsd/internal/track/TrackApi.javasrc/main/java/com/bcsd/internal/track/TrackController.javasrc/main/java/com/bcsd/internal/track/TrackNotFoundException.javasrc/main/java/com/bcsd/internal/track/TrackRepository.javasrc/main/java/com/bcsd/internal/track/TrackService.javasrc/main/java/com/bcsd/internal/track/controller/TrackApi.javasrc/main/java/com/bcsd/internal/track/controller/TrackController.javasrc/main/java/com/bcsd/internal/track/dto/TrackRequest.javasrc/main/java/com/bcsd/internal/track/exception/TrackNotFoundException.javasrc/main/java/com/bcsd/internal/track/model/Track.javasrc/main/java/com/bcsd/internal/track/service/TrackService.javasrc/main/resources/db/migration/V2__add_audit_columns_to_track.sqlsrc/test/java/com/bcsd/internal/track/service/TrackServiceTest.java
💤 Files with no reviewable changes (7)
- src/main/java/com/bcsd/internal/track/TrackController.java
- src/main/java/com/bcsd/internal/track/TrackService.java
- src/main/java/com/bcsd/internal/track/TrackNotFoundException.java
- src/main/java/com/bcsd/internal/track/TrackRepository.java
- src/main/java/com/bcsd/internal/track/TrackApi.java
- src/main/java/com/bcsd/internal/track/Track.java
- src/main/java/com/bcsd/internal/common/ErrorResponse.java
Comment on lines
+8
to
+18
| public ApiResponse { | ||
| if (result == ResultType.SUCCESS && error != null) { | ||
| throw new IllegalArgumentException("SUCCESS response must not have error"); | ||
| } | ||
| if (result == ResultType.ERROR && data != null) { | ||
| throw new IllegalArgumentException("ERROR response must not have data"); | ||
| } | ||
| if (result == ResultType.ERROR && error == null) { | ||
| throw new IllegalArgumentException("ERROR response must have error"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Guard against null for result in the canonical constructor.
new ApiResponse<>(null, null, null) currently passes and creates an invalid response shape.
💡 Proposed fix
public ApiResponse {
+ if (result == null) {
+ throw new IllegalArgumentException("result must not be null");
+ }
if (result == ResultType.SUCCESS && error != null) {
throw new IllegalArgumentException("SUCCESS response must not have error");
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/bcsd/internal/common/ApiResponse.java` around lines 8 - 18,
The canonical constructor for ApiResponse must validate that result is not null;
update the ApiResponse canonical constructor (the public ApiResponse(...)
compact constructor) to first check if result == null and throw an
IllegalArgumentException (or Objects.requireNonNull) with a clear message, then
keep the existing checks for ResultType.SUCCESS/ERROR against error and data;
reference the ResultType enum and the constructor parameters result, data, and
error when adding the null guard so new ApiResponse<>(null, null, null) is
rejected.
Comment on lines
+26
to
+37
| public ResponseEntity<ApiResponse<Void>> handleValidation(MethodArgumentNotValidException e) { | ||
| List<FieldError> fieldErrors = e.getBindingResult().getFieldErrors().stream() | ||
| .map(fe -> new FieldError(fe.getField(), fe.getDefaultMessage())) | ||
| .toList(); | ||
| ErrorMessage errorMessage = ErrorMessage.of( | ||
| ErrorCode.INVALID_INPUT_VALUE.name(), | ||
| ErrorCode.INVALID_INPUT_VALUE.getMessage(), | ||
| fieldErrors | ||
| ); | ||
| return ResponseEntity.status(ErrorCode.INVALID_INPUT_VALUE.getHttpStatus()) | ||
| .body(ApiResponse.error(errorMessage)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if file exists and extract the method at lines 26-37
if [ -f "src/main/java/com/bcsd/internal/common/GlobalExceptionHandler.java" ]; then
echo "=== Lines 26-37 with line numbers ==="
sed -n '26,37p' src/main/java/com/bcsd/internal/common/GlobalExceptionHandler.java | cat -n
echo ""
echo "=== Line count for lines 26-37 ==="
sed -n '26,37p' src/main/java/com/bcsd/internal/common/GlobalExceptionHandler.java | wc -l
echo ""
echo "=== Context: lines 20-40 ==="
sed -n '20,40p' src/main/java/com/bcsd/internal/common/GlobalExceptionHandler.java | cat -n
else
echo "File not found"
fiRepository: BCSDLab/BCSD_INTERNAL_API_V2
Length of output: 2052
Extract helper methods to comply with the 10-line method-length rule.
The handleValidation() method spans 12 lines and performs multiple tasks (field error extraction, error message creation, response assembly). Refactor into separate helper methods to keep each under 10 lines and satisfy the single-responsibility principle outlined in coding guidelines.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/bcsd/internal/common/GlobalExceptionHandler.java` around
lines 26 - 37, Split the long handleValidation(MethodArgumentNotValidException)
into a short orchestrator and small private helpers: keep handleValidation to
call three new helpers — extractFieldErrors(MethodArgumentNotValidException) to
return List<FieldError> (perform the stream map there),
buildErrorMessage(List<FieldError>) to construct and return the ErrorMessage
using ErrorCode.INVALID_INPUT_VALUE, and buildErrorResponse(ErrorMessage) to
assemble and return the ResponseEntity<ApiResponse<Void>> with the proper HTTP
status; make all helpers private and keep each helper under 10 lines so
handleValidation only delegates to them.
Comment on lines
+46
to
+51
| private void log(ErrorCode errorCode, Exception e) { | ||
| switch (errorCode.getLogLevel()) { | ||
| case ERROR -> log.error("BusinessException: {}", e.getMessage(), e); | ||
| case WARN -> log.warn("BusinessException: {}", e.getMessage()); | ||
| default -> log.info("BusinessException: {}", e.getMessage()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "GlobalExceptionHandler.java" -type fRepository: BCSDLab/BCSD_INTERNAL_API_V2
Length of output: 140
🏁 Script executed:
cat -n ./src/main/java/com/bcsd/internal/common/GlobalExceptionHandler.java | sed -n '40,60p'Repository: BCSDLab/BCSD_INTERNAL_API_V2
Length of output: 857
🏁 Script executed:
find . -name "ErrorCode.java" -type fRepository: BCSDLab/BCSD_INTERNAL_API_V2
Length of output: 127
🏁 Script executed:
cat -n ./src/main/java/com/bcsd/internal/common/ErrorCode.javaRepository: BCSDLab/BCSD_INTERNAL_API_V2
Length of output: 1179
Replace switch/case in log(...) with early-return if branches.
Lines 46-51 violate the Java guideline: "Do not use else or switch/case statements. Use early return (if + return) instead."
Proposed refactor
- private void log(ErrorCode errorCode, Exception e) {
- switch (errorCode.getLogLevel()) {
- case ERROR -> log.error("BusinessException: {}", e.getMessage(), e);
- case WARN -> log.warn("BusinessException: {}", e.getMessage());
- default -> log.info("BusinessException: {}", e.getMessage());
- }
- }
+ private void log(ErrorCode errorCode, Exception exception) {
+ if (errorCode.getLogLevel() == org.slf4j.event.Level.ERROR) {
+ log.error("BusinessException: {}", exception.getMessage(), exception);
+ return;
+ }
+ if (errorCode.getLogLevel() == org.slf4j.event.Level.WARN) {
+ log.warn("BusinessException: {}", exception.getMessage());
+ return;
+ }
+ log.info("BusinessException: {}", exception.getMessage());
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/bcsd/internal/common/GlobalExceptionHandler.java` around
lines 46 - 51, The log(ErrorCode errorCode, Exception e) method currently uses a
switch on errorCode.getLogLevel(); replace that switch with early-return if
branches: first check if errorCode.getLogLevel() == ERROR then call
log.error("BusinessException: {}", e.getMessage(), e) and return; next check for
WARN then call log.warn("BusinessException: {}", e.getMessage()) and return;
otherwise call log.info("BusinessException: {}", e.getMessage()). Keep the same
logging calls and message text (log.error, log.warn, log.info) and ensure the
method returns immediately after each matched branch.
Comment on lines
+43
to
46
| public ResponseEntity<ApiResponse<Void>> delete(@PathVariable Long id) { | ||
| trackService.delete(id); | ||
| return ResponseEntity.noContent().build(); | ||
| return ResponseEntity.status(HttpStatus.NO_CONTENT).body(ApiResponse.success()); | ||
| } |
There was a problem hiding this comment.
Do not return a body with HTTP 204.
Line 45 sends ApiResponse.success() with NO_CONTENT; 204 responses should have no body. Prefer 200 OK with wrapper to keep the unified response contract.
💡 Proposed fix
- return ResponseEntity.status(HttpStatus.NO_CONTENT).body(ApiResponse.success());
+ return ResponseEntity.ok(ApiResponse.success());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public ResponseEntity<ApiResponse<Void>> delete(@PathVariable Long id) { | |
| trackService.delete(id); | |
| return ResponseEntity.noContent().build(); | |
| return ResponseEntity.status(HttpStatus.NO_CONTENT).body(ApiResponse.success()); | |
| } | |
| public ResponseEntity<ApiResponse<Void>> delete(`@PathVariable` Long id) { | |
| trackService.delete(id); | |
| return ResponseEntity.ok(ApiResponse.success()); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/bcsd/internal/track/controller/TrackController.java` around
lines 43 - 46, The delete method in TrackController currently returns
ApiResponse.success() with HttpStatus.NO_CONTENT which violates 204 semantics;
change the response to return a 200 OK with the unified wrapper instead—update
TrackController.delete to use ResponseEntity.ok(ApiResponse.success()) (or
ResponseEntity.status(HttpStatus.OK).body(ApiResponse.success())), keeping the
call to trackService.delete(id) unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
관련 이슈
Closes #5
작업 내용
ApiResponse<T>) 도입ErrorCode) 및BusinessException기반 예외 처리 구조 확립BaseEntity로 생성/수정 시각 및 softDelete 패턴 공통화변경 사항 상세
신규 파일 (common 패키지)
ResultType.java— SUCCESS/ERROR enumFieldError.java— 필드 에러 recordErrorMessage.java— 에러 응답 본문 record (code, message, fieldErrors)ApiResponse.java— 통합 응답 래퍼 record (@JsonInclude(NON_NULL))ErrorCode.java— HttpStatus, message, logLevel을 갖는 에러 코드 enumBusinessException.java— ErrorCode 기반 도메인 예외 베이스 클래스BaseEntity.java— id, createdAt, updatedAt, deletedAt + softDelete/restore/isDeleted수정 파일
BcsdInternalApiApplication.java—@EnableJpaAuditing추가GlobalExceptionHandler.java— BusinessException / MethodArgumentNotValidException / Exception 핸들러로 재작성ErrorResponse.java— 삭제 (ApiResponse + ErrorMessage로 대체)Track.java— BaseEntity 상속, @SQLDelete 제거TrackNotFoundException.java— BusinessException 상속, TRACK_NOT_FOUND 에러코드 사용TrackService.java— delete 메서드를 softDelete 방식으로 변경TrackController.java/TrackApi.java— 반환 타입을ApiResponse<T>로 변경TrackRequest.java— Validation 메시지 구체화V2__add_audit_columns_to_track.sql— track 테이블에 created_at, updated_at 컬럼 추가참고 사항
FieldError는org.springframework.validation.FieldError와 이름이 충돌하므로GlobalExceptionHandler에서 FQCN으로 구분ApiResponse의 compact constructor에서 SUCCESS+error, ERROR+data, ERROR+error없음 조합을 모두 금지@SQLDelete대신BaseEntity.softDelete()메서드를 직접 호출하는 방식으로 변경Summary by CodeRabbit