MOSU-150 feat: Profile 수정 변경되지 않는 필드 삭제#153
Conversation
WalkthroughThis update introduces a role hierarchy to the security configuration, improves exception handling with localized messages and logging, and refines several DTOs and domain entities by removing or renaming fields. It also applies transactional behavior to a failure handler, updates enum constants, and aligns related tests and usages with these changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ProfileController
participant ProfileJpaEntity
Client->>ProfileController: update(userId, EditProfileRequest)
ProfileController->>ProfileJpaEntity: edit(EditProfileRequest)
Note right of ProfileJpaEntity: Only updates email, education, schoolInfo, grade
ProfileJpaEntity-->>ProfileController: void
ProfileController-->>Client: ApiResponseWrapper<Void>
sequenceDiagram
participant SecurityConfig
participant SpringSecurity
participant Application
SecurityConfig->>SpringSecurity: Define RoleHierarchy (ADMIN > USER > ROLE_PENDING)
SecurityConfig->>SpringSecurity: Provide MethodSecurityExpressionHandler
Application->>SpringSecurity: Method-level security checks
SpringSecurity-->>Application: Enforce role hierarchy
sequenceDiagram
participant Client
participant GlobalExceptionHandler
Client->>GlobalExceptionHandler: Malformed JSON request
GlobalExceptionHandler-->>Client: 409 Conflict, message in Korean
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @wlgns12370, 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!
This pull request primarily focuses on refining the user profile update mechanism by restricting editable fields and enhancing transactional integrity for specific failure handling. It also significantly improves the application's robustness by implementing a comprehensive global exception handling strategy with more informative error messages. Furthermore, it introduces Spring Security role hierarchies for more granular access control and includes minor refactorings for DTO clarity and enum consistency.
Highlights
- Profile Update Streamlining: The
ProfileJpaEntity.editmethod andEditProfileRequestDTO have been updated to remove fields (username, gender, birth, phone number) that are not intended to be modified through the profile edit functionality, focusing theeditoperation onemail,education, andschoolInfo. - Transactional Isolation for KMC Failure Handling: The
KmcTxFailureHandler'shandlemethod now explicitly usesPropagation.REQUIRES_NEW, ensuring that transaction failure handling logic executes in its own independent transaction, preventing rollback issues from the main transaction. - Spring Security Role Hierarchy Configuration: Spring Security has been configured to support role hierarchies, establishing a clear permission structure where
ROLE_ADMINimplicitly inheritsROLE_USERpermissions, andROLE_USERinheritsPENDINGpermissions, simplifying access control management. - Enhanced Global Exception Handling: The
GlobalExceptionHandlerhas been improved to provide more detailed and user-friendly error responses for various exceptions, includingMethodArgumentNotValidException,IllegalArgumentException,EntityNotFoundException,AuthenticationException,AccessDeniedException, and a new handler forHttpMessageNotReadableExceptionfor better JSON parsing error feedback. - DTO and Enum Refactoring: The
LoginResponseDTO'soauthUserfield was renamed touserInfofor better clarity, and theUserRoleenum valuePENDINGwas consistently renamed toROLE_PENDINGacross the codebase.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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 or fill out our survey to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request refactors the profile editing functionality by removing fields that should not be modified and enhances security by setting up a role hierarchy. It also adds a new transaction propagation setting for a failure handler.
The review identified a few issues:
- A critical configuration error in
SecurityConfigwhere an incorrect role name is used in the role hierarchy, which will prevent it from working as intended. - A few medium severity issues in
GlobalExceptionHandlerrelated to inconsistent error response structures and the use of a non-standard HTTP status code for a specific exception type.
Addressing these points will improve the correctness and consistency of the application.
| @Bean | ||
| public RoleHierarchy roleHierarchy() { | ||
| RoleHierarchyImpl hierarchy = new RoleHierarchyImpl(); | ||
| hierarchy.setHierarchy("ROLE_ADMIN > ROLE_USER \n ROLE_USER > PENDING"); |
There was a problem hiding this comment.
The role hierarchy definition uses PENDING, but the UserRole enum was updated in this pull request to use ROLE_PENDING. This inconsistency will cause the role hierarchy to not function as expected for users with the pending role, as Spring Security will not recognize the relationship.
To ensure the hierarchy works correctly, update the string to use the correct ROLE_PENDING name.
| hierarchy.setHierarchy("ROLE_ADMIN > ROLE_USER \n ROLE_USER > PENDING"); | |
| hierarchy.setHierarchy("ROLE_ADMIN > ROLE_USER \n ROLE_USER > ROLE_PENDING"); |
| response.put("errors", "잘못된 요청입니다."); | ||
| response.put("message", ex.getMessage()); |
There was a problem hiding this comment.
In this handler, the user-friendly message ("잘못된 요청입니다.") is assigned to the errors field, while the technical detail from the exception is in the message field. In other handlers (like handleEntityNotFoundException), this is reversed. The content of these fields should be consistent across all handlers.
To improve consistency, swap the values assigned here to align with the pattern used in other handlers.
| response.put("errors", "잘못된 요청입니다."); | |
| response.put("message", ex.getMessage()); | |
| response.put("message", ex.getMessage()); | |
| response.put("errors", "잘못된 요청입니다."); |
| @ExceptionHandler(HttpMessageNotReadableException.class) | ||
| public ResponseEntity<Map<String, Object>> handleHttpMessageNotReadableException( | ||
| HttpMessageNotReadableException ex) { | ||
| Map<String, Object> response = new LinkedHashMap<>(); | ||
| response.put("status", HttpStatus.CONFLICT.value()); | ||
| response.put("message", "필드명 또는 데이터 타입이 일치하지 않습니다."); | ||
| response.put("errors", ex.getMessage()); | ||
|
|
||
| return ResponseEntity.status(HttpStatus.CONFLICT).body(response); | ||
| } |
There was a problem hiding this comment.
The Javadoc at the top says @return 409 Bad Request, which is a contradiction. It should be either "409 Conflict" or "400 Bad Request".
HttpStatus.CONFLICT (409) is returned for an HttpMessageNotReadableException. This exception is typically thrown for client-side errors like malformed JSON. The more conventional and semantically appropriate HTTP status for such errors is HttpStatus.BAD_REQUEST (400). Aligning with this standard will make your API more predictable for developers.
Change the status code to BAD_REQUEST and correct the Javadoc.
/**
* @return 400 Bad Request
* @RequestBody JSON 파싱 실패 (필드명 불일치, 데이터 타입 불일치, JSON 형식 오류 등)
*/
@ExceptionHandler(HttpMessageNotReadableException.class)
public ResponseEntity<Map<String, Object>> handleHttpMessageNotReadableException(
HttpMessageNotReadableException ex) {
Map<String, Object> response = new LinkedHashMap<>();
response.put("status", HttpStatus.BAD_REQUEST.value());
response.put("message", "필드명 또는 데이터 타입이 일치하지 않습니다.");
response.put("errors", ex.getMessage());
return ResponseEntity.status(HttpStatus.BAD_REQUEST).body(response);
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main/java/life/mosu/mosuserver/global/exception/GlobalExceptionHandler.java (2)
6-6: Good addition of logging capability.Adding
@Slf4jenables proper logging for exception handling, which is a best practice for monitoring and debugging.Consider replacing the
System.out.printlnon line 124 with proper logging:- System.out.println("Exception: " + ex.getMessage()); + log.error("Unhandled exception occurred", ex);Also applies to: 16-16
85-85: Good localization with minor grammar correction needed.The Korean authentication message is excellent. However, the authorization message has a minor grammar issue.
Fix the Korean grammar in the authorization message:
- response.put("message", "인가를 실패 했습니다"); + response.put("message", "인가에 실패했습니다");Also applies to: 101-101
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/main/java/life/mosu/mosuserver/application/auth/kmc/tx/KmcTxFailureHandler.java(2 hunks)src/main/java/life/mosu/mosuserver/domain/profile/ProfileJpaEntity.java(0 hunks)src/main/java/life/mosu/mosuserver/domain/user/UserRole.java(1 hunks)src/main/java/life/mosu/mosuserver/global/config/SecurityConfig.java(2 hunks)src/main/java/life/mosu/mosuserver/global/exception/GlobalExceptionHandler.java(6 hunks)src/main/java/life/mosu/mosuserver/presentation/auth/dto/LoginResponse.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/auth/dto/SignUpAccountRequest.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/profile/ProfileController.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/profile/dto/EditProfileRequest.java(0 hunks)src/test/java/life/mosu/mosuserver/presentation/auth/dto/LoginResponseTest.java(4 hunks)
💤 Files with no reviewable changes (2)
- src/main/java/life/mosu/mosuserver/domain/profile/ProfileJpaEntity.java
- src/main/java/life/mosu/mosuserver/presentation/profile/dto/EditProfileRequest.java
🔇 Additional comments (10)
src/main/java/life/mosu/mosuserver/presentation/profile/ProfileController.java (1)
33-33: LGTM: Parameter modifier changes are harmlessRemoving the
finalmodifier from method parameters is a minor change that doesn't affect functionality. Method parameters are effectively final when not reassigned, so this change likely improves code consistency or addresses linting recommendations.Also applies to: 43-43, 53-53
src/main/java/life/mosu/mosuserver/domain/user/UserRole.java (1)
4-4: ROLE_PENDING Rename Verified – No Remaining Code ReferencesThe grep search returned no occurrences of
UserRole.PENDINGor"PENDING"in the Java source files, confirming that the rename toROLE_PENDINGhas been applied consistently in code.Please also verify that:
- Configuration files (e.g.,
application.yml/.properties) don’t reference the oldPENDINGrole- Test suites, SQL/data scripts, and any external integrations are updated to use
ROLE_PENDINGinstead ofPENDINGsrc/main/java/life/mosu/mosuserver/presentation/auth/dto/SignUpAccountRequest.java (1)
38-38: LGTM: Consistent with UserRole enum updateThe update to use
UserRole.ROLE_PENDINGis consistent with the enum constant rename and maintains the same functional behavior for new user registration.src/test/java/life/mosu/mosuserver/presentation/auth/dto/LoginResponseTest.java (1)
80-80: LGTM: Test updates align with DTO field renameThe test assertions have been correctly updated to use
userInfo()instead ofoauthUser(), maintaining the same test logic while reflecting the field rename in the LoginResponse DTO. Test coverage and validation remain intact.Also applies to: 102-102, 121-121, 135-135
src/main/java/life/mosu/mosuserver/application/auth/kmc/tx/KmcTxFailureHandler.java (1)
8-9: Excellent: Proper transaction isolation for failure handlingAdding
@Transactional(propagation = Propagation.REQUIRES_NEW)is the correct approach for failure handlers. This ensures:
- The failure handler executes in a completely new transaction, isolated from the original
- Any persistence operations in the handler won't be affected by the original transaction's rollback
- Proper separation of concerns between business logic and failure handling
This aligns perfectly with the PR objective of setting the propagation type to REQUIRES_NEW.
Also applies to: 18-18
src/main/java/life/mosu/mosuserver/presentation/auth/dto/LoginResponse.java (1)
10-10: Approve rename; ensure external API compatibility
- Renamed
oauthUser→userInfoinLoginResponseand updated all related factory methods.- Confirmed no internal references to the old property name remain (grep across code and tests).
- Tests now use
response.userInfo()and pass with Jackson’s@JsonInclude(Include.NON_NULL)intact.Please manually verify that any external API consumers (e.g., frontend apps or third-party clients) are updated to expect the JSON field
"userInfo"instead of"oauthUser".src/main/java/life/mosu/mosuserver/global/config/SecurityConfig.java (1)
68-74: Excellent implementation of method security with role hierarchy.The
MethodSecurityExpressionHandlerbean is properly configured to use the role hierarchy, enabling method-level security annotations to respect role inheritance.src/main/java/life/mosu/mosuserver/global/exception/GlobalExceptionHandler.java (3)
37-37: Improved user experience with localized validation message.The Korean message "유효성 검사에 실패했습니다." provides clear feedback to users about validation failures.
53-54: Consistent localized error messaging.The Korean message "잘못된 요청입니다." provides consistent user experience while preserving the technical exception message for debugging.
69-70: Consistent error response structure.The Korean message "요청한 리소스가 존재하지 않습니다." with technical details in the errors field maintains consistency with other exception handlers.
| @Bean | ||
| public RoleHierarchy roleHierarchy() { | ||
| RoleHierarchyImpl hierarchy = new RoleHierarchyImpl(); | ||
| hierarchy.setHierarchy("ROLE_ADMIN > ROLE_USER \n ROLE_USER > PENDING"); | ||
| return hierarchy; | ||
| } |
There was a problem hiding this comment.
Fix role hierarchy inconsistency.
The role hierarchy defines "PENDING" but according to the AI summary, this should be "ROLE_PENDING" to maintain consistency with the enum constant renaming mentioned in the PR.
- hierarchy.setHierarchy("ROLE_ADMIN > ROLE_USER \n ROLE_USER > PENDING");
+ hierarchy.setHierarchy("ROLE_ADMIN > ROLE_USER \n ROLE_USER > ROLE_PENDING");📝 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.
| @Bean | |
| public RoleHierarchy roleHierarchy() { | |
| RoleHierarchyImpl hierarchy = new RoleHierarchyImpl(); | |
| hierarchy.setHierarchy("ROLE_ADMIN > ROLE_USER \n ROLE_USER > PENDING"); | |
| return hierarchy; | |
| } | |
| @Bean | |
| public RoleHierarchy roleHierarchy() { | |
| RoleHierarchyImpl hierarchy = new RoleHierarchyImpl(); | |
| hierarchy.setHierarchy("ROLE_ADMIN > ROLE_USER \n ROLE_USER > ROLE_PENDING"); | |
| return hierarchy; | |
| } |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/global/config/SecurityConfig.java around
lines 61 to 66, the role hierarchy string uses "PENDING" instead of the
consistent enum naming "ROLE_PENDING". Update the hierarchy string to replace
"PENDING" with "ROLE_PENDING" to align with the enum constant naming convention.
| /** | ||
| * @return 409 Bad Request | ||
| * @RequestBody JSON 파싱 실패 (필드명 불일치, 데이터 타입 불일치, JSON 형식 오류 등) | ||
| */ | ||
| @ExceptionHandler(HttpMessageNotReadableException.class) | ||
| public ResponseEntity<Map<String, Object>> handleHttpMessageNotReadableException( | ||
| HttpMessageNotReadableException ex) { | ||
| Map<String, Object> response = new LinkedHashMap<>(); | ||
| response.put("status", HttpStatus.CONFLICT.value()); | ||
| response.put("message", "필드명 또는 데이터 타입이 일치하지 않습니다."); | ||
| response.put("errors", ex.getMessage()); | ||
|
|
||
| return ResponseEntity.status(HttpStatus.CONFLICT).body(response); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Useful addition but consider using HTTP 400 status.
The new handler for JSON parsing errors fills an important gap in error handling coverage. However, HTTP 409 Conflict is typically reserved for resource state conflicts rather than malformed request data.
Consider using HTTP 400 Bad Request for JSON parsing errors:
- response.put("status", HttpStatus.CONFLICT.value());
+ response.put("status", HttpStatus.BAD_REQUEST.value());
- return ResponseEntity.status(HttpStatus.CONFLICT).body(response);
+ return ResponseEntity.badRequest().body(response);HTTP 400 is more semantically appropriate for malformed request data, while HTTP 409 is better suited for business logic conflicts.
📝 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.
| /** | |
| * @return 409 Bad Request | |
| * @RequestBody JSON 파싱 실패 (필드명 불일치, 데이터 타입 불일치, JSON 형식 오류 등) | |
| */ | |
| @ExceptionHandler(HttpMessageNotReadableException.class) | |
| public ResponseEntity<Map<String, Object>> handleHttpMessageNotReadableException( | |
| HttpMessageNotReadableException ex) { | |
| Map<String, Object> response = new LinkedHashMap<>(); | |
| response.put("status", HttpStatus.CONFLICT.value()); | |
| response.put("message", "필드명 또는 데이터 타입이 일치하지 않습니다."); | |
| response.put("errors", ex.getMessage()); | |
| return ResponseEntity.status(HttpStatus.CONFLICT).body(response); | |
| } | |
| /** | |
| * @return 409 Bad Request | |
| * @RequestBody JSON 파싱 실패 (필드명 불일치, 데이터 타입 불일치, JSON 형식 오류 등) | |
| */ | |
| @ExceptionHandler(HttpMessageNotReadableException.class) | |
| public ResponseEntity<Map<String, Object>> handleHttpMessageNotReadableException( | |
| HttpMessageNotReadableException ex) { | |
| Map<String, Object> response = new LinkedHashMap<>(); | |
| - response.put("status", HttpStatus.CONFLICT.value()); | |
| + response.put("status", HttpStatus.BAD_REQUEST.value()); | |
| response.put("message", "필드명 또는 데이터 타입이 일치하지 않습니다."); | |
| response.put("errors", ex.getMessage()); | |
| - | |
| - return ResponseEntity.status(HttpStatus.CONFLICT).body(response); | |
| + return ResponseEntity.badRequest().body(response); | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/global/exception/GlobalExceptionHandler.java
around lines 107 to 120, the handler for HttpMessageNotReadableException
currently returns HTTP 409 Conflict, which is not appropriate for JSON parsing
errors. Change the HTTP status code from HttpStatus.CONFLICT (409) to
HttpStatus.BAD_REQUEST (400) in both the response map and the ResponseEntity
status to correctly indicate a malformed request error.
1 participant
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests