MOSU-213 refactor: Util 생성자 파괴 및 필터 로그 제거#214
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis set of changes refactors several utility classes to use Lombok's Changes
Sequence Diagram(s)Not generated: changes are primarily structural refactoring and logging removal without new or modified control flow. Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
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. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
✨ 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. 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 refactoring utility classes and cleaning up unnecessary logging throughout the codebase. I've leveraged Lombok's @UtilityClass annotation to ensure that utility classes cannot be instantiated, promoting better design practices. Additionally, I've removed verbose log statements to reduce noise and improve log readability, and introduced a new constants class for better maintainability of API paths. A new issue template for performance analysis was also added.
Highlights
- Utility Class Refactoring: Applied @UtilityClass to several utility classes (CookieBuilderUtil, EncodeUtil, IpUtil, KeyGeneratorUtil, PhoneNumberUtil) to enforce static method usage and prevent accidental instantiation.
- Logging Cleanup: Removed numerous log.info and log.debug statements from TokenFilter, KMC authentication controllers, mappers, and services to streamline logging output.
- Centralized Constants: Introduced AuthConstants to consolidate and manage API path strings, improving code clarity and reducing magic strings in TokenFilter.
- New Performance Issue Template: Added a new markdown template for reporting performance tests and improvements, standardizing the process for future optimizations.
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 effectively refactors several utility classes to use Lombok's @UtilityClass, which simplifies the code by removing boilerplate for private constructors. The removal of numerous debug and info logs cleans up the application's log output, and replacing hardcoded URL paths with constants from the new AuthConstants class significantly improves maintainability. My main concern is the removal of an error log that contained crucial context for debugging. I've added a comment suggesting how to retain this important information.
| } catch (Exception ex) { | ||
| log.error("Failed to create KMC verification request for certNum: {}", certNum, ex); | ||
| eventTxService.publishFailureEvent(certNum); | ||
| throw new RuntimeException("KMC 인증 요청 생성에 실패했습니다.", ex); |
There was a problem hiding this comment.
While the goal of this pull request is to remove unnecessary logs, the previous error log provided valuable context (certNum) for debugging failures. Removing it could make troubleshooting production issues more difficult. To preserve this context, consider including certNum in the exception message as suggested.
| throw new RuntimeException("KMC 인증 요청 생성에 실패했습니다.", ex); | |
| throw new RuntimeException("KMC 인증 요청 생성에 실패했습니다. certNum: " + certNum, ex); |
![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
🔭 Outside diff range comments (2)
src/main/java/life/mosu/mosuserver/global/filter/TokenFilter.java (2)
30-32: Remove duplicated header constants & follow naming conventions
tokenHeaderandBEARER_TYPEduplicate values now provided byAuthConstants, andtokenHeaderviolates the upper-snake‐case convention forstatic finalfields.Proposed inline fix:
- private final static String tokenHeader = "Authorization"; - private static final String BEARER_TYPE = "Bearer"; + // Centralised constants – avoids divergence with AuthConstants + private static final String TOKEN_HEADER = AuthConstants.AUTH_HEADER; + private static final String BEARER_TYPE = AuthConstants.BEARER_TYPE;Follow-up adjustments are required in
resolveBearerTokenFromHeader(see next comment).
120-124: LeverageAuthConstantsand drop ad-hoc trimmingOnce the previous duplication is removed, we can simplify the extraction logic and avoid a redundant
trim():- final String header = request.getHeader(tokenHeader); - if (header != null && header.startsWith(BEARER_TYPE)) { - return Optional.of(header.substring(BEARER_TYPE.length()).trim()); + final String header = request.getHeader(AuthConstants.AUTH_HEADER); + if (header != null && header.startsWith(AuthConstants.BEARER_TYPE)) { + /* BEARER_TYPE already contains the trailing space, so the substring + starts exactly at the token payload. */ + return Optional.of(header.substring(AuthConstants.BEARER_TYPE.length())); }This keeps the token-parsing contract in one place, removes magic constants, and avoids unnecessary string allocation from
trim().
🧹 Nitpick comments (7)
.github/ISSUE_TEMPLATE/성능 분석.md (2)
6-6: Avoid empty string forassigneesAn empty string is interpreted as a single blank assignee and is ignored by the UI.
Either remove the field or provide an explicit list:-assignees: "" +# assignees: +# - user1 +# - user2
1-1: Consider removing the space in the filenameSpaces in paths are URL-encoded (
%20) and can be inconvenient when linking or scripting.
Renaming the file to.github/ISSUE_TEMPLATE/성능-분석.md(or using an English slug) improves portability.src/main/java/life/mosu/mosuserver/global/util/EncodeUtil.java (1)
6-10: Redundantstaticmodifiers after introducing@UtilityClass
@UtilityClassalready promotes every member tostatic; keeping the explicitstatickeywords is harmless but adds noise. Dropping them would make the class more concise and emphasise that the annotation is the sole mechanism enforcing utility semantics.src/main/java/life/mosu/mosuserver/global/util/PhoneNumberUtil.java (1)
8-17: Consider more robust phone-number validationWhile outside this PR’s main scope, the current check only enforces a hard-coded length of 11 digits.
If formats such as 10-digit numbers, country codes (+82…) or non-numeric characters ever appear, the method will throw.
A simple regex (e.g.^\\d{10,11}$) or a library like libphonenumber would make the utility safer and reusable.src/main/java/life/mosu/mosuserver/global/util/CookieBuilderUtil.java (1)
14-15: Optional tidy-up: drop explicitstatic/publicmodifiers that Lombok now adds for youWith
@UtilityClassin place Lombok already:• makes the class
finaland adds a private constructor
• marks every field and methodstaticKeeping the extra modifiers is harmless but slightly noisy. If you prefer the minimal form you can remove them, e.g.:
- public static final String ACCESS_TOKEN_COOKIE_NAME = "accessToken"; - public static final String REFRESH_TOKEN_COOKIE_NAME = "refreshToken"; + String ACCESS_TOKEN_COOKIE_NAME = "accessToken"; + String REFRESH_TOKEN_COOKIE_NAME = "refreshToken";Purely stylistic—feel free to ignore if your team favours the explicit style.
src/main/java/life/mosu/mosuserver/global/util/IpUtil.java (1)
37-40: Return only the address for localhost to avoid leaking host names.
InetAddress.getLocalHost().getHostName() + "/" + getHostAddress()returns both the host name and the address.
Exposing the internal host name can unintentionally reveal infrastructure details when this value is logged or propagated to clients.Consider returning only the address:
- InetAddress inetAddress = InetAddress.getLocalHost(); - ip = inetAddress.getHostName() + "/" + inetAddress.getHostAddress(); + ip = InetAddress.getLocalHost().getHostAddress();[security]
src/main/java/life/mosu/mosuserver/global/filter/TokenFilter.java (1)
74-76: Use the central attribute key constant
"kmcToken"is now defined inAuthConstants.ATTR_KMC_TOKEN.
Hard-coding the same literal here defeats the purpose of the constants class and risks typos in future edits.- request.setAttribute("kmcToken", passwordToken); + request.setAttribute(AuthConstants.ATTR_KMC_TOKEN, passwordToken);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.github/ISSUE_TEMPLATE/성능 분석.md(1 hunks)src/main/java/life/mosu/mosuserver/global/filter/AuthConstants.java(1 hunks)src/main/java/life/mosu/mosuserver/global/filter/TokenFilter.java(2 hunks)src/main/java/life/mosu/mosuserver/global/resolver/UserIdArgumentResolver.java(0 hunks)src/main/java/life/mosu/mosuserver/global/util/CookieBuilderUtil.java(2 hunks)src/main/java/life/mosu/mosuserver/global/util/EncodeUtil.java(2 hunks)src/main/java/life/mosu/mosuserver/global/util/IpUtil.java(1 hunks)src/main/java/life/mosu/mosuserver/global/util/KeyGeneratorUtil.java(1 hunks)src/main/java/life/mosu/mosuserver/global/util/PhoneNumberUtil.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/kmc/KmcAuthController.java(0 hunks)src/main/java/life/mosu/mosuserver/infra/kmc/KmcDataMapper.java(0 hunks)src/main/java/life/mosu/mosuserver/infra/kmc/KmcService.java(1 hunks)
💤 Files with no reviewable changes (3)
- src/main/java/life/mosu/mosuserver/global/resolver/UserIdArgumentResolver.java
- src/main/java/life/mosu/mosuserver/infra/kmc/KmcDataMapper.java
- src/main/java/life/mosu/mosuserver/infra/kmc/KmcAuthController.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/life/mosu/mosuserver/global/util/KeyGeneratorUtil.java (4)
src/main/java/life/mosu/mosuserver/global/util/PhoneNumberUtil.java (1)
UtilityClass(5-18)src/main/java/life/mosu/mosuserver/global/util/EncodeUtil.java (1)
UtilityClass(9-63)src/main/java/life/mosu/mosuserver/global/util/CookieBuilderUtil.java (1)
UtilityClass(14-169)src/main/java/life/mosu/mosuserver/global/util/IpUtil.java (1)
UtilityClass(8-48)
🔇 Additional comments (5)
src/main/java/life/mosu/mosuserver/global/util/KeyGeneratorUtil.java (1)
4-7: Consistent use of@UtilityClass– looks goodAdopting Lombok’s
@UtilityClasshere aligns this util with the rest of the codebase and removes the need for explicit constructor suppression. No issues spotted.src/main/java/life/mosu/mosuserver/global/util/EncodeUtil.java (1)
34-36: Javadoc formatting change is clearThe consolidated description reads well and retains all necessary information. No further action required.
src/main/java/life/mosu/mosuserver/global/util/PhoneNumberUtil.java (1)
3-5: No DI Usages Found – Change Is SafeI searched the entire codebase for any
@Autowired PhoneNumberUtilor manual instantiations and found none. The only references to PhoneNumberUtil are static method calls (e.g., in MyUserService.java). Switching from a Spring bean to Lombok’s@UtilityClasswon’t break anything since there are no injection sites to update.No follow-up changes are needed here.
Likely an incorrect or invalid review comment.
src/main/java/life/mosu/mosuserver/global/util/CookieBuilderUtil.java (1)
4-5: Import of@UtilityClasslooks goodThe added import cleanly supports the Lombok refactor; no further action needed.
src/main/java/life/mosu/mosuserver/global/util/IpUtil.java (1)
6-9: Confirm Lombok configuration for@UtilityClass.
@UtilityClassimplicitly makes the classfinal, adds a private constructor, and converts all members tostatic.
Nothing is functionally wrong here, but ensure that:
- Lombok annotation processing is enabled for all relevant build-profiles (IDE & CI).
- The project’s Lombok version matches the compiler/IDE plug-ins to avoid “cannot find symbol” errors.
The explicit
statickeyword ongetClientIpis redundant but harmless.
If you prefer leaner byte-code you can safely drop it.
.github/ISSUE_TEMPLATE/성능 분석.md
Outdated
There was a problem hiding this comment.
labels must be a YAML array, not a comma-separated string
GitHub treats the current value as one label that literally contains commas.
Define each label as a separate list item to ensure they are correctly applied.
-labels: "성능개선, 부하테스트, 최적화"
+labels:
+ - 성능개선
+ - 부하테스트
+ - 최적화📝 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.
| labels: "성능개선, 부하테스트, 최적화" | |
| labels: | |
| - 성능개선 | |
| - 부하테스트 | |
| - 최적화 |
🤖 Prompt for AI Agents
In .github/ISSUE_TEMPLATE/성능 분석.md at line 5, the labels field is currently a
comma-separated string, which GitHub interprets as a single label with commas.
Change the labels field to a YAML array by listing each label on its own line
prefixed with a dash and a space, ensuring each label is a separate item in the
list.
| public static final String AUTH_HEADER = "Authorization"; | ||
| public static final String BEARER_TYPE = "Bearer "; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unify BEARER_TYPE definition & trim policy across the code-base
BEARER_TYPE here carries a trailing space ("Bearer "), whereas TokenFilter keeps its own copy without the space and compensates with .trim().
Keeping two slightly different values is a maintainability trap and increases the risk of subtle bugs during refactors.
Diff-idea – reference the single source of truth and make the space policy explicit:
- public static final String BEARER_TYPE = "Bearer ";
+ /** Prefix used in the `Authorization` header, space intentionally included. */
+ public static final String BEARER_TYPE = "Bearer ";
+
+ /* If callers need the raw token type without the trailing space, expose it once. */
+ public static final String BEARER_TYPE_RAW = BEARER_TYPE.trim();Then drop the hard-coded copies in TokenFilter (see separate comment).
This removes duplication and clarifies intent.
📝 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.
| public static final String AUTH_HEADER = "Authorization"; | |
| public static final String BEARER_TYPE = "Bearer "; | |
| public static final String AUTH_HEADER = "Authorization"; | |
| /** Prefix used in the `Authorization` header, space intentionally included. */ | |
| public static final String BEARER_TYPE = "Bearer "; | |
| /* If callers need the raw token type without the trailing space, expose it once. */ | |
| public static final String BEARER_TYPE_RAW = BEARER_TYPE.trim(); |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/global/filter/AuthConstants.java around
lines 8 to 10, the BEARER_TYPE constant includes a trailing space while
TokenFilter uses a version without the space and trims it. To unify this, define
BEARER_TYPE without the trailing space and explicitly handle spacing or trimming
where needed. Then update TokenFilter and other usages to reference this single
constant and remove any hard-coded copies or redundant trimming, ensuring
consistent and maintainable usage across the codebase.
| if (isInvalidIp(ip)) { | ||
| ip = request.getHeader("Proxy-Client-IP"); | ||
| } | ||
| if (isInvalidIp(ip)) { | ||
| ip = request.getHeader("WL-Proxy-Client-IP"); | ||
| } | ||
| if (isInvalidIp(ip)) { | ||
| ip = request.getHeader("HTTP_CLIENT_IP"); | ||
| } | ||
| if (isInvalidIp(ip)) { | ||
| ip = request.getHeader("HTTP_X_FORWARDED_FOR"); | ||
| } | ||
| if (isInvalidIp(ip)) { | ||
| ip = request.getHeader("X-Real-IP"); | ||
| } | ||
| if (isInvalidIp(ip)) { | ||
| ip = request.getRemoteAddr(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace the repetitive header checks with a loop for maintainability.
The six consecutive if (isInvalidIp(ip)) { ip = request.getHeader(...); } blocks duplicate logic and make future header additions error-prone.
A compact loop keeps the intent clear, cuts duplication, and reduces the risk of missing a null/blank check.
- if (isInvalidIp(ip)) {
- ip = request.getHeader("Proxy-Client-IP");
- }
- if (isInvalidIp(ip)) {
- ip = request.getHeader("WL-Proxy-Client-IP");
- }
- if (isInvalidIp(ip)) {
- ip = request.getHeader("HTTP_CLIENT_IP");
- }
- if (isInvalidIp(ip)) {
- ip = request.getHeader("HTTP_X_FORWARDED_FOR");
- }
- if (isInvalidIp(ip)) {
- ip = request.getHeader("X-Real-IP");
- }
- if (isInvalidIp(ip)) {
- ip = request.getRemoteAddr();
- }
+ if (isInvalidIp(ip)) {
+ for (String header : java.util.List.of(
+ "Proxy-Client-IP",
+ "WL-Proxy-Client-IP",
+ "HTTP_CLIENT_IP",
+ "HTTP_X_FORWARDED_FOR",
+ "X-Real-IP")) {
+ ip = request.getHeader(header);
+ if (!isInvalidIp(ip)) break;
+ }
+ }
+ if (isInvalidIp(ip)) {
+ ip = request.getRemoteAddr();
+ }Benefits:
• One change-point for the header list
• Easier to extend or reorder
• Eliminates six duplicated conditions
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/global/util/IpUtil.java between lines 18
and 35, replace the repeated if-blocks checking each header individually with a
single loop iterating over an array or list of header names. For each header in
the list, check if the current IP is invalid and if so, assign ip from that
header. This reduces code duplication, centralizes the header list for easier
maintenance, and keeps the null/blank checks consistent.
|
|
||
| try { | ||
| // 암호화 작업 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider the impact of removing error logging in critical authentication flows.
While removing the logging statement aligns with the PR objectives, this change eliminates valuable debugging information in the exception handling of the KMC authentication process. Consider the following concerns:
- Debugging difficulty: Without logging the exception details, troubleshooting production issues becomes significantly harder
- Monitoring impact: Systems that rely on log analysis for alerting may miss critical failures
- Authentication criticality: Identity verification failures should ideally be logged for security and audit purposes
If log reduction is necessary, consider a more targeted approach:
} catch (Exception ex) {
+ // Log only essential error information for debugging
+ log.error("KMC 인증 요청 생성 실패 - certNum: {}", certNum, ex);
eventTxService.publishFailureEvent(certNum);
throw new RuntimeException("KMC 인증 요청 생성에 실패했습니다.", ex);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/infra/kmc/KmcService.java at line 35, the
removal of error logging in the exception handling of the KMC authentication
process reduces critical debugging and monitoring information. To fix this,
reintroduce logging of the exception details in the catch block while
considering log level adjustments or conditional logging to reduce noise but
still capture essential error information for troubleshooting and security
auditing.
1 participant
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit
Chores
Documentation