[CHORE] #1 project base and spring javaformat - #2
Conversation
ownue
left a comment
There was a problem hiding this comment.
초기 세팅 정말 깔끔하게 잘 된 것 같아요! 다만 논의하면 좋을 것 같은 점들이 몇 가지 있어 코멘트 남겼으니 확인 부탁드립니당 ㅎ.ㅎ
on1yoneprivate
left a comment
There was a problem hiding this comment.
수고하셨습니다!
궁금한 점이 있어 코멘트 달아두었어요. 확인 부탁드립니다~
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughJava 17/Spring Boot 3.2.2 기반 프로젝트 초기 세팅입니다. Gradle 빌드/Wrapper, 문서와 로컬 설정 템플릿, 공통 응답·예외 처리 구조, Spring Security 기본 구성을 추가했습니다. Changes초기 프로젝트 세팅 및 공통 응답 구조
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SecurityConfig
participant Controller
participant GlobalExceptionHandler
participant ApiResponse
Client->>SecurityConfig: 요청
SecurityConfig->>Controller: Swagger 경로 허용 또는 인증 요구
Controller->>Controller: GeneralException 또는 유효성 예외 발생
Controller->>GlobalExceptionHandler: 예외 전달
GlobalExceptionHandler->>ApiResponse: onFailure 호출
ApiResponse-->>Client: 실패 응답 반환
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/main/java/com.mr/global/apipayload/handler/GlobalExceptionHandler.java (1)
47-57: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win동일한
Collectors.toMap중복 키 이슈가 여기도 있습니다.Line 26-36의
handleMethodArgumentNotValidException과 동일한 원인입니다. 같은propertyPath에 여러 개의ConstraintViolation이 발생하면 (예: 파라미터에 다중 제약 애노테이션)IllegalStateException이 발생합니다. 위 코멘트와 같은 방식으로 병합 함수를 추가해 주세요.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com.mr/global/apipayload/handler/GlobalExceptionHandler.java` around lines 47 - 57, The ConstraintViolationException handler has the same duplicate-key problem as the other validation handler: handleConstraintViolationException currently uses Collectors.toMap with propertyPath as the key, so multiple violations for the same path can throw IllegalStateException. Update the toMap call in GlobalExceptionHandler to include a merge function that combines duplicate messages, matching the approach used in handleMethodArgumentNotValidException, so repeated propertyPath entries are safely aggregated.
🧹 Nitpick comments (4)
build.gradle (1)
61-68: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win로컬 설정 복사는 소스 디렉터리 밖으로 빼주세요.
지금처럼
src/main/resources를 직접 덮어쓰면 빌드가 소스 트리를 오염시키고, 로컬application.yml이 클래스패스 리소스에 섞인 상태로 산출물에 남을 수 있습니다.MR_config/local은 빌드 전용 출력으로 연결하고, 필요한 파일만 복사하는 쪽이 README의 로컬 전용 계약과 더 잘 맞습니다.♻️ 수정 예시
tasks.register('copyPrivateConfig', Copy) { - from './MR_config/local' - into 'src/main/resources' + from('./MR_config/local') { + include 'application.yml' + } + into layout.buildDirectory.dir('generated/private-config') } tasks.named('processResources') { dependsOn 'copyPrivateConfig' + from(layout.buildDirectory.dir('generated/private-config')) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build.gradle` around lines 61 - 68, The copyPrivateConfig task is writing local config directly into src/main/resources, which can pollute the source tree and package local application.yml into classpath resources. Update copyPrivateConfig in build.gradle to copy MR_config/local into a build-only output directory instead, and adjust processResources to consume only the needed resource files from that output. Keep the fix centered on copyPrivateConfig and the processResources dependency wiring.src/main/java/com.mr/global/apipayload/ApiResponse.java (1)
13-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
BaseCode를 직접 받는onFailure오버로드를 추가하면 어떨까요?현재
onFailure(String code, String message, T data)형태라, 호출부(예:GlobalExceptionHandler)마다status.getCode(),status.getMessage()를 매번 풀어써야 합니다.BaseCode를 파라미터로 받는 오버로드를 추가하면 호출부 코드가 간결해지고, 향후BaseCode구현체가 늘어나도 일관된 방식으로 실패 응답을 생성할 수 있습니다.♻️ 제안: BaseCode 오버로드 추가
// 실패 public static <T> ApiResponse<T> onFailure(String code, String message, T data) { return new ApiResponse<>(false, code, message, data); } + + public static <T> ApiResponse<T> onFailure(BaseCode code, T data) { + return new ApiResponse<>(false, code.getCode(), code.getMessage(), data); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com.mr/global/apipayload/ApiResponse.java` around lines 13 - 21, Add an overloaded onFailure method in ApiResponse that accepts a BaseCode directly, alongside the existing onFailure(String code, String message, T data). The new overload should read the code and message from the BaseCode instance and delegate to the same ApiResponse constructor, so callers like GlobalExceptionHandler can pass the status object instead of manually extracting getCode() and getMessage(). Keep onSuccess unchanged and preserve the current string-based overload for compatibility.src/main/java/com.mr/global/apipayload/handler/GlobalExceptionHandler.java (1)
59-65: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win전역 예외를 로깅 없이 삼키고 있습니다.
Exception을 잡아 일괄 500 응답을 만드는 것은 좋으나, 예외 스택트레이스를 로깅하지 않으면 운영 환경에서 실제 원인을 추적하기 매우 어려워집니다. 최소한log.error("Unhandled exception", e)정도는 남겨두는 것을 권장합니다. Spring 공식 문서의@RestControllerAdvice예외 처리 가이드도 참고해 보세요.📝 로깅 추가 제안
+import lombok.extern.slf4j.Slf4j; + +@Slf4j `@RestControllerAdvice` public class GlobalExceptionHandler { ... `@ExceptionHandler`(Exception.class) public ResponseEntity<ApiResponse<Object>> handleAllException(Exception e) { + log.error("처리되지 않은 예외 발생", e); var status = CommonStatus.INTERNAL_SERVER_ERROR;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com.mr/global/apipayload/handler/GlobalExceptionHandler.java` around lines 59 - 65, The global Exception handler in GlobalExceptionHandler.handleAllException is swallowing unhandled exceptions without logging them, so add error-level logging of the caught Exception (including the stack trace) before returning the 500 ApiResponse. Use the existing `@ExceptionHandler`(Exception.class) method and a logger in GlobalExceptionHandler to record something like an “Unhandled exception” message with the exception object, while keeping the current CommonStatus.INTERNAL_SERVER_ERROR response behavior unchanged.src/main/java/com.mr/global/config/SecurityConfig.java (1)
14-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win설정 체인은 깔끔한데, CSRF 부분만 한 번 꼬였습니다.
csrf(...)안에서 다시http.csrf(...)를 호출할 필요가 없고,try-catch도throws Exception때문에 의미가 없습니다.http.csrf(AbstractHttpConfigurer::disable)로 정리하면 됩니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com.mr/global/config/SecurityConfig.java` around lines 14 - 29, The CSRF setup in SecurityConfig.securityFilterChain is incorrectly nesting a second http.csrf call inside the csrf lambda and wrapping it in an unnecessary try-catch. Simplify the configuration by using the csrf customization callback directly on HttpSecurity and disable CSRF with AbstractHttpConfigurer::disable, then keep the authorizeHttpRequests setup unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.gitignore:
- Around line 41-43: The current global application* ignore rules are too broad
and can hide required configuration files. Narrow the .gitignore entries for
application*.properties and application*.yml so they only exclude local secret
or developer-specific config, using a more specific path or filename pattern,
and verify the updated rules do not match shared defaults like
src/main/resources/application.yml or application-dev.yml.
In `@gradlew.bat`:
- Around line 1-94: The gradle wrapper batch script is stored with LF-only line
endings, which can break label and goto handling in cmd.exe. Update gradlew.bat
to use CRLF line endings, or enforce CRLF for batch files via .gitattributes
with an appropriate rule for *.bat so Windows executes the wrapper reliably.
In `@src/main/java/com.mr/Application.java`:
- Line 7: Remove the DataSource auto-configuration exclusion from Application so
Spring Boot can initialize the configured datasource and JPA normally. Update
the Application class annotation by dropping DataSourceAutoConfiguration from
the `@SpringBootApplication` exclude list, and if this exclusion was only meant
for certain environments, move that behavior behind a dedicated profile or
environment-specific configuration instead. Use Application and
`@SpringBootApplication` as the main symbols to locate the change.
In `@src/main/java/com.mr/global/apipayload/handler/GlobalExceptionHandler.java`:
- Around line 26-36:
`GlobalExceptionHandler.handleMethodArgumentNotValidException`에서
`getFieldErrors()`를 `Collectors.toMap`으로 바로 모으는 방식은 같은 필드명이 여러 번 나오면 예외가 납니다. 중복
키를 안전하게 처리하도록 병합 함수를 추가하거나, 필드별로 에러를 합치는 방식으로 변경하세요.
`handleMethodArgumentNotValidException`, `getFieldErrors`, `Collectors.toMap`,
`ApiResponse.onFailure`를 기준으로 수정하면 됩니다.
---
Duplicate comments:
In `@src/main/java/com.mr/global/apipayload/handler/GlobalExceptionHandler.java`:
- Around line 47-57: The ConstraintViolationException handler has the same
duplicate-key problem as the other validation handler:
handleConstraintViolationException currently uses Collectors.toMap with
propertyPath as the key, so multiple violations for the same path can throw
IllegalStateException. Update the toMap call in GlobalExceptionHandler to
include a merge function that combines duplicate messages, matching the approach
used in handleMethodArgumentNotValidException, so repeated propertyPath entries
are safely aggregated.
---
Nitpick comments:
In `@build.gradle`:
- Around line 61-68: The copyPrivateConfig task is writing local config directly
into src/main/resources, which can pollute the source tree and package local
application.yml into classpath resources. Update copyPrivateConfig in
build.gradle to copy MR_config/local into a build-only output directory instead,
and adjust processResources to consume only the needed resource files from that
output. Keep the fix centered on copyPrivateConfig and the processResources
dependency wiring.
In `@src/main/java/com.mr/global/apipayload/ApiResponse.java`:
- Around line 13-21: Add an overloaded onFailure method in ApiResponse that
accepts a BaseCode directly, alongside the existing onFailure(String code,
String message, T data). The new overload should read the code and message from
the BaseCode instance and delegate to the same ApiResponse constructor, so
callers like GlobalExceptionHandler can pass the status object instead of
manually extracting getCode() and getMessage(). Keep onSuccess unchanged and
preserve the current string-based overload for compatibility.
In `@src/main/java/com.mr/global/apipayload/handler/GlobalExceptionHandler.java`:
- Around line 59-65: The global Exception handler in
GlobalExceptionHandler.handleAllException is swallowing unhandled exceptions
without logging them, so add error-level logging of the caught Exception
(including the stack trace) before returning the 500 ApiResponse. Use the
existing `@ExceptionHandler`(Exception.class) method and a logger in
GlobalExceptionHandler to record something like an “Unhandled exception” message
with the exception object, while keeping the current
CommonStatus.INTERNAL_SERVER_ERROR response behavior unchanged.
In `@src/main/java/com.mr/global/config/SecurityConfig.java`:
- Around line 14-29: The CSRF setup in SecurityConfig.securityFilterChain is
incorrectly nesting a second http.csrf call inside the csrf lambda and wrapping
it in an unnecessary try-catch. Simplify the configuration by using the csrf
customization callback directly on HttpSecurity and disable CSRF with
AbstractHttpConfigurer::disable, then keep the authorizeHttpRequests setup
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 45f7e2bd-b451-4e29-9516-baf59b63e923
⛔ Files ignored due to path filters (1)
gradle/wrapper/gradle-wrapper.jaris excluded by!**/*.jar
📒 Files selected for processing (23)
.coderabbit.yaml.github/PULL_REQUEST_TEMPLATE.md.gitignoreMR_config/local/application.example.ymlREADME.mdbuild.gradlegradle/wrapper/gradle-wrapper.propertiesgradlewgradlew.batsettings.gradlesrc/main/java/com.mr/Application.javasrc/main/java/com.mr/global/apipayload/ApiResponse.javasrc/main/java/com.mr/global/apipayload/code/BaseCode.javasrc/main/java/com.mr/global/apipayload/code/CommonStatus.javasrc/main/java/com.mr/global/apipayload/code/StatusReasonDTO.javasrc/main/java/com.mr/global/apipayload/domain/AuthErrorStatus.javasrc/main/java/com.mr/global/apipayload/exception/GeneralException.javasrc/main/java/com.mr/global/apipayload/handler/GlobalExceptionHandler.javasrc/main/java/com.mr/global/config/SecurityConfig.javasrc/main/java/com.mr/global/file/FileCategory.javasrc/main/java/com.mr/global/file/FileException.javasrc/main/java/com.mr/global/file/S3Config.javasrc/main/java/com.mr/global/file/S3Service.java
📍 개요
⛓️💥 관련 이슈
🛠️ 작업 내용
자바 및 스프링 버전 사양 정립: Java 17(Java Toolchain 강제 적용), Spring Boot 3.2.2
코드 포매터 : Spring JavaFormat 플러그인 설정
코드 래빗 AI 설정: 프로젝트 루트 경로에 .coderabbit.yaml 세팅
도메인형 아키텍처 : com.mr.global, com.mr.domain 구조
공통 응답 규격(apiPayload) 구조, 전역 GlobalExceptionHandler 설계
🔥 리뷰 요청 사항
✅ 체크리스트
📎 참고 사항
copyPrivateConfig : application.yml 설정이 유출되지 않도록 처리
Summary by CodeRabbit