MOSU-171 refactor: profile, 문의 답변 알림톡 기능 구현 및 cookie util 표준화#172
MOSU-171 refactor: profile, 문의 답변 알림톡 기능 구현 및 cookie util 표준화#172chominju02 merged 7 commits intodevelopfrom
Conversation
Co-authored-by: chominju02 <cmj021002@knu.ac.kr>
Co-authored-by: chominju02 <cmj021002@knu.ac.kr>
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces a transactional event-based notification system for profile and inquiry features, refactors token and cookie handling for authentication, and adds new utility and handler classes to centralize and standardize token management. Several new classes and interfaces are added for event publishing, token resolution, and cookie creation, while existing authentication and notification flows are updated for improved modularity and error handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AccessTokenFilter
participant TokenResolver
participant AuthTokenManager
participant ReissueHandler
Client->>AccessTokenFilter: HTTP request to /api/v1/auth/reissue
AccessTokenFilter->>TokenResolver: resolveTokens(request)
TokenResolver-->>AccessTokenFilter: TokenCookies
AccessTokenFilter->>AuthTokenManager: reissueToken(TokenCookies)
AuthTokenManager-->>AccessTokenFilter: Token
AccessTokenFilter->>ReissueHandler: onReissueSuccess(request, response, Token)
ReissueHandler-->>Client: Set new cookies, respond with success
sequenceDiagram
participant Service
participant EventTxService
participant EventFactory
participant TxEventPublisher
participant EventListener
participant NotifyEventPublisher
Service->>EventTxService: publishSuccessEvent(userId, [inquiryId])
EventTxService->>EventFactory: create(context)
EventFactory-->>EventTxService: TxEvent
EventTxService->>TxEventPublisher: publish(TxEvent)
TxEventPublisher-->>EventListener: (after commit) handle(TxEvent)
EventListener->>NotifyEventPublisher: send notification
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
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 (2)
✨ 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 introduces a more robust and maintainable architecture for handling authentication tokens and asynchronous notifications. It refactors token reissuance to use a dedicated handler and standardizes cookie management across different environments. Furthermore, it decouples notification events from core business logic by implementing a transactional event publishing mechanism, ensuring that notifications are reliably sent only upon successful transaction completion. These changes enhance the system's reliability, modularity, and error handling capabilities.
Highlights
- Transactional Event Publishing: Implemented a transactional event publishing mechanism to decouple notification logic from core business processes (inquiry answer creation and profile registration). This ensures that notifications are reliably sent only after the main transaction commits successfully, improving system robustness and modularity.
- Centralized Token Management & Reissuance: Refactored token reissuance logic by introducing a dedicated
ReissueHandlerinterface and aTokenResolvercomponent. This standardizes how access and refresh tokens are extracted from HTTP requests and how new tokens are set in responses, leading to a more consistent and maintainable authentication flow. - Enhanced Cookie Utility: Significantly updated the
CookieBuilderUtilto provide environment-specific cookie creation methods (local, develop, production). This enhancement offers greater flexibility and maintainability for managing cookies across different deployment environments. - Refined Error Handling: Updated the
ErrorCodeenum andAccessTokenFilterto provide more specific and clearer error messages for token-related issues, such as missing signup tokens or expired refresh tokens, improving diagnostic capabilities.
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. ↩
chominju02
changed the title
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-executed refactoring to decouple business logic from notification side-effects using Spring's transactional events. This is a great improvement for the application's robustness and maintainability. The token handling logic has also been substantially improved with better-structured utility classes, resolvers, and filters.
While the overall direction and implementation are excellent, I've identified a few areas for improvement:
- Exception Handling: There are a couple of
try-catchblocks that catch the genericException, which could lead to incorrect transaction behavior under certain conditions. - Token Reissue Logic: The logic for determining if a token can be reissued seems to have a flaw that could deny valid requests.
- Error Information Loss: The exception handling in the
AccessTokenFiltercan obscure specific error details, making it harder for clients to react appropriately.
Addressing these points will further enhance the quality and correctness of the new features.
| public boolean availableReissue() { | ||
| return getAccessToken().isPresent() && getRefreshToken().isPresent(); | ||
| } |
There was a problem hiding this comment.
The availableReissue() method requires both the access token and refresh token to be present. Typically, token reissue functionality should only depend on the presence of a valid refresh token. This implementation could prevent a client from reissuing tokens if the access token has been cleared or is missing, which is a valid scenario.
Additionally, the AccessTokenFilter throws ErrorCode.EXPIRED_REFRESH_TOKEN when this check fails, which is misleading if the root cause is a missing access token.
Consider updating the logic to only check for the refresh token's presence.
| public boolean availableReissue() { | |
| return getAccessToken().isPresent() && getRefreshToken().isPresent(); | |
| } | |
| public boolean availableReissue() { | |
| return getRefreshToken().isPresent(); | |
| } |
| } catch (CustomRuntimeException e) { | ||
| log.error("액세스 토큰 인증 실패: {}", e.getMessage()); | ||
| throw new CustomRuntimeException(ErrorCode.INVALID_TOKEN); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
This catch block for CustomRuntimeException conceals the specific reason for an authentication failure. For instance, if accessTokenProvider.getAuthentication() throws a CustomRuntimeException with ErrorCode.EXPIRED_TOKEN, this block will catch it and re-throw a generic ErrorCode.INVALID_TOKEN. This causes the client to lose valuable information about why the token was rejected (e.g., expired vs. malformed).
To provide more precise feedback to the client, it would be better to re-throw the original exception.
} catch (CustomRuntimeException e) {
log.error("액세스 토큰 인증 실패: {}", e.getMessage());
throw e;
}| } catch (Exception ex) { | ||
| log.error("문의 답변 등록 실패: {}", ex.getMessage(), ex); | ||
| throw ex; | ||
| } |
There was a problem hiding this comment.
Catching the generic Exception is risky here. If a checked exception were thrown inside the try block, Spring's default transactional behavior would lead to a commit of the transaction upon re-throwing the checked exception. This is likely not the intended behavior, as any failure should probably result in a rollback.
To ensure correctness, it's safer to catch RuntimeException or wrap the caught exception in a new RuntimeException.
| } catch (Exception ex) { | |
| log.error("문의 답변 등록 실패: {}", ex.getMessage(), ex); | |
| throw ex; | |
| } | |
| } catch (RuntimeException ex) { | |
| log.error("문의 답변 등록 실패: {}", ex.getMessage(), ex); | |
| throw ex; | |
| } |
| } catch (Exception ex) { | ||
| log.error("프로필 등록 실패: {}", ex.getMessage(), ex); | ||
| throw ex; | ||
| } |
There was a problem hiding this comment.
Similar to the InquiryAnswerService, this catch (Exception ex) block is too broad and could lead to incorrect transactional behavior. If a checked exception is thrown, the transaction might commit instead of rolling back.
To ensure the transaction correctly rolls back on any failure, please catch RuntimeException specifically.
| } catch (Exception ex) { | |
| log.error("프로필 등록 실패: {}", ex.getMessage(), ex); | |
| throw ex; | |
| } | |
| } catch (RuntimeException ex) { | |
| log.error("프로필 등록 실패: {}", ex.getMessage(), ex); | |
| throw 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: 11
🔭 Outside diff range comments (2)
src/main/java/life/mosu/mosuserver/presentation/auth/MasterController.java (1)
26-27: Remove duplicate constants.These constants are already defined in
CookieBuilderUtil(lines 12-13 in the relevant code snippet). Having duplicate constants creates maintenance overhead and potential inconsistencies.Remove the local constants and use the ones from
CookieBuilderUtil:- private final static String ACCESS_TOKEN_COOKIE_NAME = "accessToken"; - private final static String REFRESH_TOKEN_COOKIE_NAME = "refreshToken";Then update the method calls to use
CookieBuilderUtil.ACCESS_TOKEN_COOKIE_NAMEandCookieBuilderUtil.REFRESH_TOKEN_COOKIE_NAME.src/main/java/life/mosu/mosuserver/application/auth/provider/AuthTokenManager.java (1)
28-37: Add null safety and token validation checks.The method doesn't validate the
tokenCookiesparameter or ensure the refresh token is present and valid before processing.Apply this diff to add proper validation:
public Token reissueToken(final TokenCookies tokenCookies) { + if (tokenCookies == null) { + throw new IllegalArgumentException("TokenCookies cannot be null"); + } + + final String refreshToken = tokenCookies.refreshToken(); + if (refreshToken == null || refreshToken.trim().isEmpty()) { + throw new IllegalArgumentException("Refresh token is required for token reissue"); + } final Authentication authentication = refreshTokenProvider.getAuthentication( - tokenCookies.refreshToken()); + refreshToken); final PrincipalDetails principalDetails = (PrincipalDetails) authentication.getPrincipal(); final UserJpaEntity user = principalDetails.user(); refreshTokenProvider.invalidateToken(user.getId()); return generateAuthToken(user); }
🧹 Nitpick comments (25)
src/main/java/life/mosu/mosuserver/application/profile/ProfileEventTxService.java (3)
12-26: LGTM! Consider adding logging and error handling.The service is well-structured with proper Spring annotations and follows the transactional event pattern correctly. The method properly creates a success context and publishes the event.
Consider adding debug logging and error handling:
@Transactional public void publishSuccessEvent(Long userId) { + log.debug("Publishing profile success event for userId: {}", userId); + try { TxEvent<?> event = eventFactory.create(ProfileContext.ofSuccess(userId)); txEventPublisher.publish(event); + log.debug("Profile success event published for userId: {}", userId); + } catch (Exception ex) { + log.error("Failed to publish profile success event for userId: {}", userId, ex); + throw ex; + } }
21-24: Consider more specific return type for better type safety.The method uses
TxEvent<?>which loses type information. Since we know this creates aProfileTxEvent, consider using a more specific type.@Transactional public void publishSuccessEvent(Long userId) { - TxEvent<?> event = eventFactory.create(ProfileContext.ofSuccess(userId)); + ProfileTxEvent event = (ProfileTxEvent) eventFactory.create(ProfileContext.ofSuccess(userId)); txEventPublisher.publish(event); }Or better yet, update the factory interface to return more specific types if possible.
20-24: Consider adding logging and failure case handling.The method implementation is clean and follows the transactional event pattern correctly. However, consider:
- Adding debug logging since
@Slf4jis present but unused- Adding a method for publishing failure events for completeness
@Transactional public void publishSuccessEvent(Long userId) { + log.debug("Publishing profile success event for userId: {}", userId); TxEvent<?> event = eventFactory.create(ProfileContext.ofSuccess(userId)); txEventPublisher.publish(event); } + + @Transactional + public void publishFailureEvent(Long userId) { + log.debug("Publishing profile failure event for userId: {}", userId); + TxEvent<?> event = eventFactory.create(ProfileContext.ofFailure(userId)); + txEventPublisher.publish(event); + }src/main/java/life/mosu/mosuserver/global/resolver/TokenResolver.java (1)
11-33: Consider validating token values for empty/whitespace strings.The method returns tokens even if they are empty strings or contain only whitespace, which may not be valid tokens.
Consider adding validation:
public TokenCookies resolveTokens(final HttpServletRequest request) { String accessToken = null; String refreshToken = null; final Cookie[] cookies = request.getCookies(); if (cookies == null) { return new TokenCookies(null, null); } for (final Cookie cookie : cookies) { final String cookieName = cookie.getName(); final String cookieValue = cookie.getValue(); if (cookieName != null && TokenCookies.ACCESS_TOKEN_NAME.equals(cookieName)) { - accessToken = cookieValue; + accessToken = isValidToken(cookieValue) ? cookieValue : null; } else if (cookieName != null && TokenCookies.REFRESH_TOKEN_NAME.equals(cookieName)) { - refreshToken = cookieValue; + refreshToken = isValidToken(cookieValue) ? cookieValue : null; } if (accessToken != null && refreshToken != null) { break; } } return new TokenCookies(accessToken, refreshToken); } + +private boolean isValidToken(final String token) { + return token != null && !token.trim().isEmpty(); +}src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAnswerTxService.java (1)
22-23: Consider adding logging for event publishing operations.For debugging and monitoring purposes, it would be helpful to log when events are published, especially in a transactional context.
Add logging:
@Transactional public void publishSuccessEvent(Long userId, Long inquiryId) { + log.debug("Publishing inquiry answer success event for userId: {}, inquiryId: {}", userId, inquiryId); TxEvent<?> event = eventFactory.create(InquiryAnswerContext.ofSuccess(userId, inquiryId)); txEventPublisher.publish(event); + log.debug("Successfully published inquiry answer success event"); }src/main/java/life/mosu/mosuserver/global/cookie/TokenCookies.java (2)
21-23: Consider enhancing token validation in availableReissue().The method only checks for presence but doesn't validate that the tokens are non-empty strings, which could lead to false positives for empty or whitespace-only tokens.
Consider this enhancement:
public boolean availableReissue() { - return getAccessToken().isPresent() && getRefreshToken().isPresent(); + return isValidToken(accessToken) && isValidToken(refreshToken); +} + +private boolean isValidToken(String token) { + return token != null && !token.trim().isEmpty(); }
10-11: Consider adding JavaDoc for public constants.The cookie name constants would benefit from documentation explaining their usage and any constraints.
Add documentation:
+/** Cookie name for storing the access token */ public static final String ACCESS_TOKEN_NAME = "accessToken"; +/** Cookie name for storing the refresh token */ public static final String REFRESH_TOKEN_NAME = "refreshToken";src/main/java/life/mosu/mosuserver/application/profile/tx/ProfileTxEventListener.java (1)
29-33: Consider adding error handling for notification failures.The notification logic is clean, but consider wrapping the notification call in a try-catch block to handle potential failures gracefully, as exceptions here could cause the event listener to fail.
private void sendNotification(Long userId) { - NotificationEvent notificationEvent = NotificationEvent.create( - NotificationStatus.SIGN_UP_SUCCESS, userId); - notifier.notify(notificationEvent); + try { + NotificationEvent notificationEvent = NotificationEvent.create( + NotificationStatus.SIGN_UP_SUCCESS, userId); + notifier.notify(notificationEvent); + } catch (Exception e) { + log.error("Failed to send notification for userId={}", userId, e); + // Consider if you want to rethrow or handle gracefully + } }src/main/java/life/mosu/mosuserver/presentation/oauth/SignUpTokenService.java (4)
35-36: Improve log message clarity and consider sensitivity.The log message "signUp {}" is not descriptive. Consider a more informative message, and evaluate if logging expiration time is appropriate from a security perspective.
-log.info("signUp {}", signUpToken.expiration()); +log.info("Processing valid sign-up token with expiration: {}", signUpToken.expiration());Or if expiration details are sensitive:
-log.info("signUp {}", signUpToken.expiration()); +log.debug("Valid sign-up token processed successfully");
21-27: Update javadoc to match the implementation.The javadoc still mentions
AuthenticationExceptionbut the method now throwsCustomRuntimeException. This inconsistency should be fixed.- * @throws AuthenticationException 토큰이 유효하지 않을 경우 + * @throws CustomRuntimeException 토큰이 유효하지 않을 경우
35-35: Improve log message clarity.The log message
"signUp {}"could be more descriptive to provide better context for debugging.- log.info("signUp {}", signUpToken.expiration()); + log.info("SignUp token validated successfully, expiration: {}", signUpToken.expiration());
35-35: Improve logging message and consider log level.The current log message lacks context and may expose sensitive information unnecessarily.
-log.info("signUp {}", signUpToken.expiration()); +log.debug("Sign-up token validated successfully, expires at: {}", signUpToken.expiration());Consider using
debuglevel instead ofinfofor internal validation details, and provide a more descriptive message.src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAnswerService.java (2)
36-49: Consider separating event publishing from core operation error handling.The try-catch structure looks good, but if
eventTxService.publishSuccessEvent()throws an exception, it could cause the entire transaction to rollback. Consider moving event publishing outside the try-catch that handles core operations.try { InquiryAnswerJpaEntity answerEntity = inquiryAnswerJpaRepository.save( request.toEntity(postId)); answerAttachmentService.createAttachment(request.attachments(), answerEntity); inquiryEntity.updateStatusToComplete(); - eventTxService.publishSuccessEvent(userId, postId); } catch (Exception ex) { log.error("문의 답변 등록 실패: {}", ex.getMessage(), ex); throw ex; } + +// Publish event after core operations succeed +eventTxService.publishSuccessEvent(userId, postId);
36-48: Consider more specific exception handling.The try-catch block catches all exceptions (
Exception ex), which might be too broad. Consider catching more specific exceptions or document why catching all exceptions is appropriate here.If the intent is to log any failure during the inquiry answer creation process, the current approach is acceptable. However, if there are specific exceptions that should be handled differently, consider a more targeted approach:
- } catch (Exception ex) { - log.error("문의 답변 등록 실패: {}", ex.getMessage(), ex); - throw ex; - } + } catch (CustomRuntimeException ex) { + log.error("문의 답변 등록 실패 (비즈니스 오류): {}", ex.getMessage(), ex); + throw ex; + } catch (Exception ex) { + log.error("문의 답변 등록 실패 (시스템 오류): {}", ex.getMessage(), ex); + throw ex; + }src/main/java/life/mosu/mosuserver/application/inquiry/tx/InquiryAnswerTxEventListener.java (1)
31-36: Consider adding error handling for notification failures.Similar to
ProfileTxEventListener, consider adding error handling to prevent notification failures from causing the event listener to fail.private void sendNotification(Long userId, Long inquiryId) { - NotificationEvent notificationEvent = NotificationEvent.create( - NotificationStatus.INQUIRY_ANSWER_SUCCESS, userId, inquiryId); - notifier.notify(notificationEvent); + try { + NotificationEvent notificationEvent = NotificationEvent.create( + NotificationStatus.INQUIRY_ANSWER_SUCCESS, userId, inquiryId); + notifier.notify(notificationEvent); + } catch (Exception e) { + log.error("Failed to send notification for userId={}, inquiryId={}", userId, inquiryId, e); + // Consider if you want to rethrow or handle gracefully + } }src/main/java/life/mosu/mosuserver/global/handler/TokenReissueHandler.java (5)
47-48: Consider error handling for partial response scenarios.If JSON serialization fails after cookies are set, the client receives cookies but no JSON response. Consider if this partial state is acceptable or if error recovery is needed.
+try { objectMapper.writeValue(response.getWriter(), responseBody); +} catch (IOException e) { + log.error("Failed to write token reissue response", e); + // Consider if you need to clear cookies or handle partial state + throw e; +}
26-37: Consider environment-aware cookie creation.The implementation always uses
createLocalCookie, which may not be appropriate for all deployment environments (development, production). Consider making the cookie creation environment-aware.Based on the
CookieBuilderUtilclass, consider adding environment detection or configuration:+ @Value("${app.environment:local}") + private String environment; + @Override public void onReissueSuccess(final HttpServletRequest request, final HttpServletResponse response, final Token newToken) throws IOException { Stream.of( - CookieBuilderUtil.createLocalCookie( + createEnvironmentCookie( CookieBuilderUtil.ACCESS_TOKEN_COOKIE_NAME, newToken.accessToken(), newToken.accessTokenExpireTime() ), - CookieBuilderUtil.createLocalCookie( + createEnvironmentCookie( CookieBuilderUtil.REFRESH_TOKEN_COOKIE_NAME, newToken.refreshToken(), newToken.refreshTokenExpireTime() ) ).forEach(response::addCookie); + + private Cookie createEnvironmentCookie(String name, String value, Long maxAge) { + return switch (environment.toLowerCase()) { + case "development" -> CookieBuilderUtil.createDevelopmentCookie(name, value, maxAge); + case "production" -> CookieBuilderUtil.createProductionCookie(name, value, maxAge); + default -> CookieBuilderUtil.createLocalCookie(name, value, maxAge); + }; + }
39-47: Consider internationalization for response messages.The success message is hardcoded in Korean. For better internationalization support, consider using message properties or making the message configurable.
+ @Value("${message.reissue.success:reissue를 성공했습니다.}") + private String reissueSuccessMessage; + - responseBody.put("message", "reissue를 성공했습니다."); + responseBody.put("message", reissueSuccessMessage);
27-27: Consider environment-specific cookie creation.The handler currently uses
createLocalCookiewhich suggests it's specific to local development. Consider making this configurable based on the deployment environment.You might want to inject an environment-aware cookie creation strategy or use a factory method that selects the appropriate cookie creation method based on the active profile.
41-41: Consider internationalization for user messages.The hardcoded Korean message might need to be externalized for internationalization support if the application serves multiple locales.
Consider using message properties or a similar i18n mechanism:
responseBody.put("message", messageSource.getMessage("token.reissue.success", null, locale));src/main/java/life/mosu/mosuserver/application/profile/ProfileService.java (1)
36-49: Consider removing redundant try-catch block.The try-catch block only logs and rethrows the exception. Since
@Transactionalwill handle the rollback automatically, this wrapper adds minimal value. Consider either:
- Remove the try-catch entirely, or
- Add meaningful error handling/recovery logic
- try { - ProfileJpaEntity profile = request.toEntity(userId); - profileJpaRepository.save(profile); - - user.grantUserRole(); - syncUserInfoFromProfile(user, request); - - eventTxService.publishSuccessEvent(userId); - - } catch (Exception ex) { - log.error("프로필 등록 실패: {}", ex.getMessage(), ex); - throw ex; - } + ProfileJpaEntity profile = request.toEntity(userId); + profileJpaRepository.save(profile); + + user.grantUserRole(); + syncUserInfoFromProfile(user, request); + + eventTxService.publishSuccessEvent(userId);src/main/java/life/mosu/mosuserver/global/util/CookieBuilderUtil.java (1)
116-117: Track production domain configuration TODOs.Multiple TODOs indicate that production domain configuration needs to be finalized. Consider:
- Creating a configuration property for the production domain
- Ensuring consistency between ResponseCookie domain (
.mosuedu.com) and servlet Cookie domain (api.mosuedu.com)Would you like me to create an issue to track the production domain configuration or suggest a configuration-based approach?
Also applies to: 133-133
src/main/java/life/mosu/mosuserver/global/filter/AccessTokenFilter.java (1)
63-65: Consider removing token logging for skipped URLs.Logging whether a token exists for skipped URLs could potentially leak information about user authentication state in logs. Consider removing or reducing the log level for this information.
- log.info("스킵된 URL 요청: {} 토큰: {}", requestUri, accessToken != null); + log.debug("스킵된 URL 요청: {}", requestUri);src/main/java/life/mosu/mosuserver/application/inquiry/tx/InquiryAnswerContext.java (1)
3-7: Consider using primitive boolean for the success flag.The
isSuccessfield usesBoolean(boxed type) which allowsnullvalues. Since this is a success flag, consider usingboolean(primitive) to prevent null values and make the intent clearer.public record InquiryAnswerContext( Long userId, Long inquiryId, - Boolean isSuccess + boolean isSuccess ) {src/main/java/life/mosu/mosuserver/application/profile/tx/ProfileTxEventFactory.java (1)
10-13: Consider more specific return type for consistency.Similar to
ProfileEventTxService, this method returnsTxEvent<?>which loses type information. For better type safety and consistency, consider returningProfileTxEventspecifically.@Override -public TxEvent<?> create(ProfileContext context) { +public ProfileTxEvent create(ProfileContext context) { return new ProfileTxEvent(context.isSuccess(), context); }Note: This would require updating the interface definition if other implementations need the same specificity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
src/main/java/life/mosu/mosuserver/application/auth/provider/AuthTokenManager.java(2 hunks)src/main/java/life/mosu/mosuserver/application/auth/provider/RefreshTokenProvider.java(2 hunks)src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAnswerService.java(1 hunks)src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAnswerTxService.java(1 hunks)src/main/java/life/mosu/mosuserver/application/inquiry/tx/InquiryAnswerContext.java(1 hunks)src/main/java/life/mosu/mosuserver/application/inquiry/tx/InquiryAnswerTxEvent.java(1 hunks)src/main/java/life/mosu/mosuserver/application/inquiry/tx/InquiryAnswerTxEventFactory.java(1 hunks)src/main/java/life/mosu/mosuserver/application/inquiry/tx/InquiryAnswerTxEventListener.java(1 hunks)src/main/java/life/mosu/mosuserver/application/profile/ProfileEventTxService.java(1 hunks)src/main/java/life/mosu/mosuserver/application/profile/ProfileService.java(2 hunks)src/main/java/life/mosu/mosuserver/application/profile/tx/ProfileContext.java(1 hunks)src/main/java/life/mosu/mosuserver/application/profile/tx/ProfileTxEvent.java(1 hunks)src/main/java/life/mosu/mosuserver/application/profile/tx/ProfileTxEventFactory.java(1 hunks)src/main/java/life/mosu/mosuserver/application/profile/tx/ProfileTxEventListener.java(1 hunks)src/main/java/life/mosu/mosuserver/global/cookie/TokenCookies.java(1 hunks)src/main/java/life/mosu/mosuserver/global/exception/ErrorCode.java(2 hunks)src/main/java/life/mosu/mosuserver/global/filter/AccessTokenFilter.java(3 hunks)src/main/java/life/mosu/mosuserver/global/handler/AuthLogoutHandler.java(2 hunks)src/main/java/life/mosu/mosuserver/global/handler/OAuth2LoginSuccessHandler.java(1 hunks)src/main/java/life/mosu/mosuserver/global/handler/ReissueHandler.java(1 hunks)src/main/java/life/mosu/mosuserver/global/handler/TokenReissueHandler.java(1 hunks)src/main/java/life/mosu/mosuserver/global/resolver/TokenResolver.java(1 hunks)src/main/java/life/mosu/mosuserver/global/util/CookieBuilderUtil.java(2 hunks)src/main/java/life/mosu/mosuserver/infra/notify/dto/NotificationEvent.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/auth/AuthController.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/auth/MasterController.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/auth/SignUpController.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/oauth/SignUpTokenService.java(2 hunks)src/test/java/life/mosu/mosuserver/global/fixture/UserTestFixture.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (8)
src/main/java/life/mosu/mosuserver/presentation/auth/MasterController.java (1)
src/main/java/life/mosu/mosuserver/global/util/CookieBuilderUtil.java (1)
CookieBuilderUtil(13-171)
src/main/java/life/mosu/mosuserver/global/handler/AuthLogoutHandler.java (1)
src/main/java/life/mosu/mosuserver/global/util/CookieBuilderUtil.java (1)
CookieBuilderUtil(13-171)
src/main/java/life/mosu/mosuserver/global/handler/OAuth2LoginSuccessHandler.java (1)
src/main/java/life/mosu/mosuserver/global/util/CookieBuilderUtil.java (1)
CookieBuilderUtil(13-171)
src/main/java/life/mosu/mosuserver/presentation/auth/AuthController.java (1)
src/main/java/life/mosu/mosuserver/global/util/CookieBuilderUtil.java (1)
CookieBuilderUtil(13-171)
src/main/java/life/mosu/mosuserver/global/handler/TokenReissueHandler.java (1)
src/main/java/life/mosu/mosuserver/global/util/CookieBuilderUtil.java (1)
CookieBuilderUtil(13-171)
src/main/java/life/mosu/mosuserver/application/profile/ProfileEventTxService.java (2)
src/main/java/life/mosu/mosuserver/application/profile/ProfileService.java (1)
Slf4j(19-81)src/main/java/life/mosu/mosuserver/application/profile/tx/ProfileTxEventListener.java (1)
Slf4j(14-34)
src/main/java/life/mosu/mosuserver/presentation/auth/SignUpController.java (1)
src/main/java/life/mosu/mosuserver/global/util/CookieBuilderUtil.java (1)
CookieBuilderUtil(13-171)
src/main/java/life/mosu/mosuserver/global/resolver/TokenResolver.java (1)
src/main/java/life/mosu/mosuserver/global/handler/TokenReissueHandler.java (1)
Component(17-49)
🔇 Additional comments (62)
src/main/java/life/mosu/mosuserver/global/handler/ReissueHandler.java (1)
8-12: LGTM! Clean interface design.The interface follows good design principles with a clear single responsibility for handling token reissue success events. The method signature is appropriate with necessary parameters and proper exception handling.
src/main/java/life/mosu/mosuserver/presentation/auth/MasterController.java (1)
65-74: LGTM! Cookie method update aligns with refactoring.The change from
createCookietocreateLocalCookieStringis consistent with the broader cookie handling refactoring across the application. The method properly creates local environment cookies with appropriate security settings.src/main/java/life/mosu/mosuserver/global/handler/OAuth2LoginSuccessHandler.java (1)
49-59: LGTM! Consistent cookie handling refactoring.The update from
temporaryCookietocreateLocalResponseCookiealigns with the broader cookie handling refactoring. The method properly creates local environment cookies with appropriate security settings (Secure=false,SameSite=Lax).src/main/java/life/mosu/mosuserver/presentation/auth/AuthController.java (1)
48-57: LGTM! Proper use of centralized constants and updated cookie methods.The update to
createLocalCookieStringis consistent with the broader refactoring, and the code properly uses the centralized constants fromCookieBuilderUtilrather than defining local duplicates. This demonstrates good maintainability practices.src/main/java/life/mosu/mosuserver/global/handler/AuthLogoutHandler.java (1)
8-8: LGTM! Good refactoring to eliminate magic strings.Replacing hardcoded cookie names with constants from
CookieBuilderUtilimproves maintainability and consistency across the codebase. This aligns well with the broader token and cookie management refactoring effort.Also applies to: 22-25
src/main/java/life/mosu/mosuserver/application/inquiry/tx/InquiryAnswerTxEvent.java (1)
1-11: All dependencies verified and integration confirmed• TxEvent is defined in src/main/java/life/mosu/mosuserver/global/tx/TxEvent.java
• InquiryAnswerContext is declared as a record in src/main/java/life/mosu/mosuserver/application/inquiry/tx/InquiryAnswerContext.java
• InquiryAnswerTxEventFactory publishes the event and InquiryAnswerTxEventListener handles it after commitNo further changes needed—this event implementation fits the existing infrastructure.
src/main/java/life/mosu/mosuserver/application/profile/tx/ProfileContext.java (1)
1-12: LGTM! Well-designed context record.The record implementation is clean and follows good practices:
- Immutable data carrier with clear field names
- Convenient
ofSuccessfactory method for the common success case- Appropriate use of wrapper types for nullable fields
This fits well into the event-driven architecture pattern being implemented.
src/main/java/life/mosu/mosuserver/application/auth/provider/RefreshTokenProvider.java (2)
17-17: LGTM! Formatting improvements enhance readability.The field reordering and parameter formatting changes improve code organization and readability. The multi-line parameter formatting in both the constructor and
cacheTokenmethod makes the code more maintainable.Also applies to: 20-24, 46-50
39-39: ErrorCode.NOT_FOUND_REFRESH_TOKEN VerifiedConfirmed that
ErrorCode.NOT_FOUND_REFRESH_TOKENis defined inErrorCode.javaand the obsoleteNOT_EXIST_REFRESH_TOKENentry has been removed. No further action required.src/main/java/life/mosu/mosuserver/application/profile/tx/ProfileTxEvent.java (3)
5-11: LGTM! Clean event class implementation.The event class properly extends the generic
TxEvent<ProfileContext>and correctly delegates constructor parameters to the superclass. This follows good inheritance practices and provides type safety for profile-related transactional events.
5-11: LGTM! Clean event class implementation.The
ProfileTxEventclass correctly extends the genericTxEvent<ProfileContext>and properly delegates constructor parameters to the superclass. The implementation is minimal and focused, which is appropriate for an event class.
5-11: LGTM! Clean event class implementation.The
ProfileTxEventclass follows the correct pattern for extending the genericTxEventbase class. The constructor properly delegates to the superclass with the success flag and context, maintaining the event-driven architecture's consistency.src/main/java/life/mosu/mosuserver/application/inquiry/tx/InquiryAnswerContext.java (3)
3-13: LGTM! Well-designed context record with convenient factory method.The record properly encapsulates the inquiry answer context data, and the static factory method
ofSuccessprovides a clean way to create success instances. This follows good patterns for immutable data structures and factory methods.
9-11: LGTM! Clean factory method implementation.The
ofSuccessstatic factory method provides a convenient way to create success instances with clear intent. The implementation correctly setsisSuccesstotrue.
3-13: Excellent use of Java records with convenient factory method.The
InquiryAnswerContextrecord is well-designed with clear field names and types. The static factory methodofSuccessprovides a convenient way to create success instances, improving code readability and reducing boilerplate.src/main/java/life/mosu/mosuserver/application/profile/tx/ProfileTxEventFactory.java (3)
7-15: LGTM! Clean factory implementation.The factory correctly implements the
TxEventFactory<ProfileContext>interface and properly createsProfileTxEventinstances by extracting the success status from the context. The Spring@Componentannotation ensures proper dependency injection.
7-8: LGTM! Proper factory pattern implementation.The class correctly implements the
TxEventFactory<ProfileContext>interface and follows Spring component patterns with the@Componentannotation.
7-15: LGTM! Clean factory pattern implementation.The
ProfileTxEventFactorycorrectly implements theTxEventFactoryinterface and follows the factory pattern. Thecreatemethod properly constructsProfileTxEventinstances using the context's success flag and the context itself.src/main/java/life/mosu/mosuserver/global/resolver/TokenResolver.java (1)
20-30: Add null safety checks for cookie properties.The code doesn't handle potential null values from
cookie.getName()orcookie.getValue(), which could causeNullPointerException.Apply this diff to add null safety:
for (final Cookie cookie : cookies) { - if (TokenCookies.ACCESS_TOKEN_NAME.equals(cookie.getName())) { - accessToken = cookie.getValue(); - } else if (TokenCookies.REFRESH_TOKEN_NAME.equals(cookie.getName())) { - refreshToken = cookie.getValue(); + final String cookieName = cookie.getName(); + final String cookieValue = cookie.getValue(); + + if (cookieName != null && TokenCookies.ACCESS_TOKEN_NAME.equals(cookieName)) { + accessToken = cookieValue; + } else if (cookieName != null && TokenCookies.REFRESH_TOKEN_NAME.equals(cookieName)) { + refreshToken = cookieValue; }Likely an incorrect or invalid review comment.
src/main/java/life/mosu/mosuserver/application/auth/provider/AuthTokenManager.java (1)
5-5: LGTM on the import addition.The addition of
TokenCookiesimport aligns well with the method signature refactoring.src/main/java/life/mosu/mosuserver/global/cookie/TokenCookies.java (1)
5-8: LGTM on the record design.The record structure is clean and follows good practices for immutable data containers. The field names are descriptive and the overall design supports the token handling refactoring well.
src/main/java/life/mosu/mosuserver/application/profile/tx/ProfileTxEventListener.java (8)
1-12: LGTM! Clean import structure.The package declaration and imports are well-organized and all imported classes are utilized in the implementation.
14-18: LGTM! Correct transactional configuration for event listeners.The use of
Propagation.NOT_SUPPORTEDis appropriate for post-commit event listeners handling notifications, ensuring they don't participate in any ongoing transactions.
20-20: LGTM! Proper dependency injection.The field declaration follows Spring dependency injection best practices with final modifier and constructor injection.
22-27: LGTM! Well-structured event handler.The event handler correctly uses
AFTER_COMMITphase to ensure notifications are sent only after successful transaction commits. The debug logging provides good traceability.
14-20: LGTM! Well-structured event listener class.The class annotations and dependency injection are properly configured. The
NOT_SUPPORTEDpropagation is correct for transactional event listeners to ensure they don't participate in ongoing transactions.
22-27: LGTM! Proper transactional event handling.The use of
AFTER_COMMITphase ensures notifications are only sent after successful transaction commits. The logging and context extraction are well-implemented.
29-33: LGTM! Clean notification publishing logic.The notification event creation and publishing logic is straightforward and well-implemented. The method has a clear single responsibility.
29-33: LGTM!The notification creation and publishing logic is clean and follows the expected pattern. The use of the factory method and event publisher is appropriate.
src/main/java/life/mosu/mosuserver/presentation/oauth/SignUpTokenService.java (5)
10-10: LGTM! Good addition of logging capability.Adding
@Slf4jannotation enables logging for better observability and debugging.Also applies to: 13-13
27-27: LGTM! Explicit exception declaration improves API clarity.Declaring
CustomRuntimeExceptionin the method signature makes the expected exceptions explicit for callers.
27-33: Verify null handling in OneTimeTokenProvider implementationI wasn’t able to find any class in the repo that implements OneTimeTokenProvider, so it’s unclear how getSignUpToken(String token) behaves when passed null. Without a null guard here, calling
oneTimeTokenProvider.getSignUpToken(token)with a null token could lead to an unexpected NPE in validateSignUpToken.
• File: src/main/java/life/mosu/mosuserver/presentation/oauth/SignUpTokenService.java
Lines 27–33:validateSignUpTokenpublic void validateSignUpToken(final String token) throws CustomRuntimeException { SignUpToken signUpToken = oneTimeTokenProvider.getSignUpToken(token); if (signUpToken == null) { throw new CustomRuntimeException(ErrorCode.INVALID_SIGN_UP_TOKEN); } }Please confirm (or update) that the provider’s getSignUpToken method either:
- Explicitly checks for a null token and throws a controlled exception, or
- Returns null safely (to be handled by this method),
so that we avoid any unintended NPE.
27-27: Good improvement in exception handling clarity.Explicitly declaring
CustomRuntimeExceptionin the method signature improves the API contract and makes error handling more transparent to callers.
29-33: Ensuretokenis non-null beforegetSignUpTokenis calledBoth calls to
SignUpTokenService.validateSignUpToken(token)originate in AccessTokenFilter, which first extracts the Authorization header:
- If the header is missing or doesn’t start with
"Bearer ", the filter skips validation entirely.- Only when a header is present does it call the service, so
tokencan never benullat that point.No other callers of
validateSignUpTokenwere found.Since upstream null checks are already in place, you can safely remove the “verify null handling” reminder.
src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAnswerService.java (5)
27-28: LGTM! Proper dependency injection for event service.The addition of
InquiryAnswerTxServicefollows dependency injection best practices and supports the new transactional event pattern.
28-28: LGTM! Good refactoring to event-driven approach.The dependency change from direct
NotifyEventPublishertoInquiryAnswerTxServiceproperly separates concerns and moves to a transactional event-driven notification pattern.
43-43: LGTM! Proper event publishing placement.The event publishing is correctly placed after all business operations complete successfully, ensuring events are only published for successful transactions.
28-28: Good refactoring to event-driven architecture.The introduction of
InquiryAnswerTxServiceproperly decouples notification logic from the core business service, following good separation of concerns principles.
36-49: Excellent error handling and transaction management.The try-catch block ensures proper error logging while maintaining transaction rollback behavior by re-throwing exceptions. The event publishing after successful operations ensures notifications are only sent for successful transactions.
src/main/java/life/mosu/mosuserver/application/inquiry/tx/InquiryAnswerTxEventListener.java (6)
1-20: LGTM! Consistent structure with ProfileTxEventListener.The class structure, annotations, and transactional configuration follow the same pattern as
ProfileTxEventListener, ensuring consistency across the codebase.
22-29: LGTM! Well-structured event handler with comprehensive logging.The event handler correctly uses
AFTER_COMMITphase and includes bothuserIdandinquiryIdin the debug logging for better traceability.
14-20: LGTM! Consistent event listener pattern.The class structure follows the same well-established pattern as
ProfileTxEventListener, with proper annotations and dependency injection.
22-29: LGTM! Well-implemented event handler.The event handler properly uses
AFTER_COMMITphase and includes comprehensive logging with both user and inquiry IDs. The implementation is consistent with the established pattern.
31-35: LGTM! Clean notification logic.The notification method properly creates the event with both
userIdandinquiryIdparameters, which is appropriate for inquiry answer notifications.
31-35: LGTM!The notification creation properly includes both userId and inquiryId parameters, which is appropriate for inquiry answer notifications. The event publishing pattern is consistent and well-implemented.
src/main/java/life/mosu/mosuserver/global/handler/TokenReissueHandler.java (5)
1-21: LGTM! Clean class structure with appropriate visibility.The package-private visibility and dependency injection setup are well-designed for this handler component.
39-46: LGTM! Proper HTTP response setup.The JSON response construction correctly sets the status code, content type, and character encoding. The Korean message is appropriate for the service domain.
17-49: LGTM! Well-structured token reissue handler.The implementation is clean and follows good practices. The use of streams for cookie creation, proper HTTP response handling, and JSON serialization are all well-implemented. The suggestions above are for minor improvements but don't affect the core functionality.
26-37: Clean implementation of cookie handling.The use of streams to create and add cookies is elegant and readable. The proper use of
CookieBuilderUtil.createLocalCookiewith appropriate expiration times is correct.
39-47: Well-structured HTTP response handling.The JSON response creation and HTTP metadata setting follow good practices. The response structure is consistent and appropriate for API responses.
src/main/java/life/mosu/mosuserver/global/exception/ErrorCode.java (2)
27-34: LGTM! Well-organized token error codes.The reorganization of token-related error codes into a dedicated section improves code maintainability. The new error codes (
MISSING_SIGNUP_TOKEN,EXPIRED_REFRESH_TOKEN) align well with the enhanced authentication flow.
110-110: LGTM! Appropriate error code for notification validation.The
INVALID_NOTIFICATION_STATUSerror code properly supports the new notification event system.src/main/java/life/mosu/mosuserver/application/profile/ProfileService.java (1)
27-27: Good implementation of transactional event pattern.The replacement of direct notification with
ProfileEventTxServiceproperly decouples the notification concern and ensures notifications are only sent after successful transaction commit.Also applies to: 43-43
src/main/java/life/mosu/mosuserver/global/util/CookieBuilderUtil.java (1)
13-171: Excellent refactoring of cookie utilities.The environment-specific cookie creation methods provide clear separation of concerns and appropriate security settings for each deployment environment. The utility class pattern is properly implemented with good documentation.
src/main/java/life/mosu/mosuserver/global/filter/AccessTokenFilter.java (3)
107-109: Good extraction of URL checking logic.The
isSkippedUrlmethod properly encapsulates the URL checking logic, making it reusable and testable.
70-74: Well-implemented token reissue flow.The token reissue logic properly validates the presence of a refresh token and delegates to appropriate handlers, maintaining good separation of concerns.
Also applies to: 111-120
88-102: Improved error handling with specific error codes.The refactored token extraction and authentication setting now properly uses
TokenCookiesand provides clear, specific error messages throughCustomRuntimeException, improving debuggability.src/main/java/life/mosu/mosuserver/infra/notify/dto/NotificationEvent.java (3)
14-14: LGTM! Improved exception handling.Using
CustomRuntimeExceptionwith a specific error code (INVALID_NOTIFICATION_STATUS) is better than the genericIllegalArgumentExceptionfor consistent error handling across the application.
12-17: No change needed—two-arg factory is intentionally scoped to sign-up events
The single-argumentcreate(status, userId)overload is explicitly designed forSIGN_UP_SUCCESS(which has notargetId). All other statuses—includingINQUIRY_ANSWER_SUCCESS,REFUND_SUCCESS, etc.—use the three-argumentcreate(status, userId, targetId)overload without restriction. The validation is therefore not overly restrictive but aligns with the intended design.Likely an incorrect or invalid review comment.
3-4: LGTM! Proper exception handling.Good improvement to use
CustomRuntimeExceptionwith a specificErrorCodeinstead of genericIllegalArgumentException. This provides better error context and consistency with the application's error handling strategy.src/main/java/life/mosu/mosuserver/application/profile/ProfileEventTxService.java (2)
12-15: LGTM! Well-structured service with clear responsibilities.The service correctly follows Spring patterns with appropriate annotations (
@Service,@Transactional) and dependency injection. The separation of concerns is good - this service focuses solely on publishing transactional events.
12-18: LGTM! Proper service configuration.Excellent use of Spring annotations and dependency injection. The
@Transactionalannotation ensures the event publishing happens within the same transaction context, and the service follows clean architecture principles.
| @Transactional | ||
| public void publishSuccessEvent(Long userId, Long inquiryId) { | ||
| TxEvent<?> event = eventFactory.create(InquiryAnswerContext.ofSuccess(userId, inquiryId)); | ||
| txEventPublisher.publish(event); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add parameter validation for business logic integrity.
The method accepts userId and inquiryId without validation, which could lead to invalid events being published if null or invalid IDs are passed.
Apply this diff to add validation:
@Transactional
public void publishSuccessEvent(Long userId, Long inquiryId) {
+ if (userId == null || userId <= 0) {
+ throw new IllegalArgumentException("UserId must be a positive number");
+ }
+ if (inquiryId == null || inquiryId <= 0) {
+ throw new IllegalArgumentException("InquiryId must be a positive number");
+ }
+
TxEvent<?> event = eventFactory.create(InquiryAnswerContext.ofSuccess(userId, inquiryId));
txEventPublisher.publish(event);
}📝 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.
| @Transactional | |
| public void publishSuccessEvent(Long userId, Long inquiryId) { | |
| TxEvent<?> event = eventFactory.create(InquiryAnswerContext.ofSuccess(userId, inquiryId)); | |
| txEventPublisher.publish(event); | |
| } | |
| @Transactional | |
| public void publishSuccessEvent(Long userId, Long inquiryId) { | |
| if (userId == null || userId <= 0) { | |
| throw new IllegalArgumentException("UserId must be a positive number"); | |
| } | |
| if (inquiryId == null || inquiryId <= 0) { | |
| throw new IllegalArgumentException("InquiryId must be a positive number"); | |
| } | |
| TxEvent<?> event = eventFactory.create(InquiryAnswerContext.ofSuccess(userId, inquiryId)); | |
| txEventPublisher.publish(event); | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAnswerTxService.java
around lines 20 to 24, the method publishSuccessEvent accepts userId and
inquiryId without validation, risking invalid events. Add checks to ensure
neither userId nor inquiryId is null before proceeding. If either is null, throw
an IllegalArgumentException with a clear message to prevent publishing invalid
events and maintain business logic integrity.
| @Override | ||
| public TxEvent<?> create(InquiryAnswerContext context) { | ||
| return new InquiryAnswerTxEvent(context.isSuccess(), context); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null safety check and align generic types.
The method doesn't validate the input parameter and the return type should be more specific to match the generic interface contract.
Apply this diff:
@Override
-public TxEvent<?> create(InquiryAnswerContext context) {
+public TxEvent<InquiryAnswerContext> create(InquiryAnswerContext context) {
+ if (context == null) {
+ throw new IllegalArgumentException("InquiryAnswerContext cannot be null");
+ }
return new InquiryAnswerTxEvent(context.isSuccess(), context);
}📝 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.
| @Override | |
| public TxEvent<?> create(InquiryAnswerContext context) { | |
| return new InquiryAnswerTxEvent(context.isSuccess(), context); | |
| } | |
| @Override | |
| public TxEvent<InquiryAnswerContext> create(InquiryAnswerContext context) { | |
| if (context == null) { | |
| throw new IllegalArgumentException("InquiryAnswerContext cannot be null"); | |
| } | |
| return new InquiryAnswerTxEvent(context.isSuccess(), context); | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/inquiry/tx/InquiryAnswerTxEventFactory.java
lines 10 to 13, add a null check for the context parameter at the start of the
create method to prevent NullPointerException. Also, update the method's return
type to use the specific generic type matching the interface contract, replacing
TxEvent<?> with TxEvent<InquiryAnswerContext>. This ensures type safety and
proper adherence to the interface.
| @TransactionalEventListener(phase = TransactionPhase.AFTER_COMMIT) | ||
| public void afterCommitHandler(InquiryAnswerTxEvent event) { | ||
| InquiryAnswerContext ctx = event.getContext(); | ||
| log.debug("[AFTER_COMMIT] 문의 답변 등록 후 알림톡 발송 시작: userId={}, inquiryId={}", ctx.userId(), | ||
| ctx.inquiryId()); | ||
|
|
||
| sendNotification(ctx.userId(), ctx.inquiryId()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for notification failures.
Similar to other event listeners, this should handle potential exceptions from the notification process to prevent propagation that could affect application stability.
@TransactionalEventListener(phase = TransactionPhase.AFTER_COMMIT)
public void afterCommitHandler(InquiryAnswerTxEvent event) {
InquiryAnswerContext ctx = event.getContext();
log.debug("[AFTER_COMMIT] 문의 답변 등록 후 알림톡 발송 시작: userId={}, inquiryId={}", ctx.userId(),
ctx.inquiryId());
-
- sendNotification(ctx.userId(), ctx.inquiryId());
+
+ try {
+ sendNotification(ctx.userId(), ctx.inquiryId());
+ } catch (Exception ex) {
+ log.error("Failed to send inquiry answer notification for userId={}, inquiryId={}: {}",
+ ctx.userId(), ctx.inquiryId(), ex.getMessage(), ex);
+ }
}📝 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.
| @TransactionalEventListener(phase = TransactionPhase.AFTER_COMMIT) | |
| public void afterCommitHandler(InquiryAnswerTxEvent event) { | |
| InquiryAnswerContext ctx = event.getContext(); | |
| log.debug("[AFTER_COMMIT] 문의 답변 등록 후 알림톡 발송 시작: userId={}, inquiryId={}", ctx.userId(), | |
| ctx.inquiryId()); | |
| sendNotification(ctx.userId(), ctx.inquiryId()); | |
| } | |
| @TransactionalEventListener(phase = TransactionPhase.AFTER_COMMIT) | |
| public void afterCommitHandler(InquiryAnswerTxEvent event) { | |
| InquiryAnswerContext ctx = event.getContext(); | |
| log.debug("[AFTER_COMMIT] 문의 답변 등록 후 알림톡 발송 시작: userId={}, inquiryId={}", ctx.userId(), | |
| ctx.inquiryId()); | |
| try { | |
| sendNotification(ctx.userId(), ctx.inquiryId()); | |
| } catch (Exception ex) { | |
| log.error("Failed to send inquiry answer notification for userId={}, inquiryId={}: {}", | |
| ctx.userId(), ctx.inquiryId(), ex.getMessage(), ex); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/inquiry/tx/InquiryAnswerTxEventListener.java
around lines 22 to 29, the afterCommitHandler method lacks error handling for
failures during notification sending. Wrap the sendNotification call in a
try-catch block to catch any exceptions, log the error details appropriately,
and prevent exceptions from propagating and affecting application stability.
| @TransactionalEventListener(phase = TransactionPhase.AFTER_COMMIT) | ||
| public void afterCommitHandler(ProfileTxEvent event) { | ||
| ProfileContext ctx = event.getContext(); | ||
| log.debug("[AFTER_COMMIT] 프로필 등록 후 알림톡 발송 시작: userId={}", ctx.userId()); | ||
| sendNotification(ctx.userId()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for the notification process.
The event listener doesn't handle potential exceptions from the notification sending process. If NotificationEvent.create() or notifier.notify() fails, the exception will propagate and potentially affect the application flow.
@TransactionalEventListener(phase = TransactionPhase.AFTER_COMMIT)
public void afterCommitHandler(ProfileTxEvent event) {
ProfileContext ctx = event.getContext();
log.debug("[AFTER_COMMIT] 프로필 등록 후 알림톡 발송 시작: userId={}", ctx.userId());
- sendNotification(ctx.userId());
+ try {
+ sendNotification(ctx.userId());
+ } catch (Exception ex) {
+ log.error("Failed to send notification for userId={}: {}", ctx.userId(), ex.getMessage(), ex);
+ // Consider whether to rethrow or handle gracefully based on business requirements
+ }
}📝 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.
| @TransactionalEventListener(phase = TransactionPhase.AFTER_COMMIT) | |
| public void afterCommitHandler(ProfileTxEvent event) { | |
| ProfileContext ctx = event.getContext(); | |
| log.debug("[AFTER_COMMIT] 프로필 등록 후 알림톡 발송 시작: userId={}", ctx.userId()); | |
| sendNotification(ctx.userId()); | |
| } | |
| @TransactionalEventListener(phase = TransactionPhase.AFTER_COMMIT) | |
| public void afterCommitHandler(ProfileTxEvent event) { | |
| ProfileContext ctx = event.getContext(); | |
| log.debug("[AFTER_COMMIT] 프로필 등록 후 알림톡 발송 시작: userId={}", ctx.userId()); | |
| try { | |
| sendNotification(ctx.userId()); | |
| } catch (Exception ex) { | |
| log.error("Failed to send notification for userId={}: {}", ctx.userId(), ex.getMessage(), ex); | |
| // Consider whether to rethrow or handle gracefully based on business requirements | |
| } | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/profile/tx/ProfileTxEventListener.java
around lines 22 to 27, the afterCommitHandler method lacks error handling for
the notification sending process. Wrap the sendNotification call in a try-catch
block to catch any exceptions thrown by NotificationEvent.create() or
notifier.notify(), log the error appropriately, and prevent the exception from
propagating and affecting the application flow.
| Stream.of( | ||
| CookieBuilderUtil.createLocalCookie( | ||
| CookieBuilderUtil.ACCESS_TOKEN_COOKIE_NAME, | ||
| newToken.accessToken(), | ||
| newToken.accessTokenExpireTime() | ||
| ), | ||
| CookieBuilderUtil.createLocalCookie( | ||
| CookieBuilderUtil.REFRESH_TOKEN_COOKIE_NAME, | ||
| newToken.refreshToken(), | ||
| newToken.refreshTokenExpireTime() | ||
| ) | ||
| ).forEach(response::addCookie); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider environment-specific cookie creation.
The code currently uses createLocalCookie for all environments. Based on the CookieBuilderUtil class, consider using environment-specific cookie creation methods for production deployments.
Consider implementing environment-aware cookie creation:
Stream.of(
- CookieBuilderUtil.createLocalCookie(
+ CookieBuilderUtil.createEnvironmentSpecificCookie(
CookieBuilderUtil.ACCESS_TOKEN_COOKIE_NAME,
newToken.accessToken(),
newToken.accessTokenExpireTime()
),
- CookieBuilderUtil.createLocalCookie(
+ CookieBuilderUtil.createEnvironmentSpecificCookie(
CookieBuilderUtil.REFRESH_TOKEN_COOKIE_NAME,
newToken.refreshToken(),
newToken.refreshTokenExpireTime()
)
).forEach(response::addCookie);Or inject a profile-aware cookie factory to handle environment differences.
📝 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.
| Stream.of( | |
| CookieBuilderUtil.createLocalCookie( | |
| CookieBuilderUtil.ACCESS_TOKEN_COOKIE_NAME, | |
| newToken.accessToken(), | |
| newToken.accessTokenExpireTime() | |
| ), | |
| CookieBuilderUtil.createLocalCookie( | |
| CookieBuilderUtil.REFRESH_TOKEN_COOKIE_NAME, | |
| newToken.refreshToken(), | |
| newToken.refreshTokenExpireTime() | |
| ) | |
| ).forEach(response::addCookie); | |
| Stream.of( | |
| CookieBuilderUtil.createEnvironmentSpecificCookie( | |
| CookieBuilderUtil.ACCESS_TOKEN_COOKIE_NAME, | |
| newToken.accessToken(), | |
| newToken.accessTokenExpireTime() | |
| ), | |
| CookieBuilderUtil.createEnvironmentSpecificCookie( | |
| CookieBuilderUtil.REFRESH_TOKEN_COOKIE_NAME, | |
| newToken.refreshToken(), | |
| newToken.refreshTokenExpireTime() | |
| ) | |
| ).forEach(response::addCookie); |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/global/handler/TokenReissueHandler.java
around lines 26 to 37, the code uses createLocalCookie for all cookies
regardless of environment. Modify the code to detect the current environment
(e.g., development or production) and use the appropriate cookie creation method
from CookieBuilderUtil, such as createLocalCookie for local and
createProductionCookie for production. Alternatively, refactor to inject a
profile-aware cookie factory that abstracts this logic and returns
environment-specific cookies, then use that factory to create and add cookies to
the response.
| public static NotificationEvent create(NotificationStatus status, Long userId) { | ||
| if (!NotificationStatus.SIGN_UP_SUCCESS.equals(status)) { | ||
| throw new IllegalArgumentException("Unknown notification status: " + status); | ||
| throw new CustomRuntimeException(ErrorCode.INVALID_NOTIFICATION_STATUS); | ||
| } | ||
| return new NotificationEvent(status, null, null); | ||
| return new NotificationEvent(status, userId, null); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify method signature change doesn't break existing callers.
The changes improve error handling by using CustomRuntimeException with a specific error code instead of IllegalArgumentException. The updated constructor call with userId and null targetId is also appropriate.
However, the method signature change from create(NotificationStatus status) to create(NotificationStatus status, Long userId) is a breaking change.
🏁 Script executed:
#!/bin/bash
# Verify all calls to NotificationEvent.create method match the new signature
rg -A 3 -B 1 "NotificationEvent\.create\(" --type javaLength of output: 2856
Fix mismatched NotificationEvent.create calls
Three call sites still invoke the old three-argument form and will not compile against the new two-parameter factory:
• src/main/java/life/mosu/mosuserver/application/payment/tx/PaymentTxEventListener.java
NotificationEvent.create(NotificationStatus.APPLICATION_SUCCESS, userId, examSchoolId)
• src/main/java/life/mosu/mosuserver/application/refund/tx/RefundTxEventListener.java
NotificationEvent.create(NotificationStatus.REFUND_SUCCESS, userId, examApplicationId)
• src/main/java/life/mosu/mosuserver/application/inquiry/tx/InquiryAnswerTxEventListener.java
NotificationEvent.create(NotificationStatus.INQUIRY_ANSWER_SUCCESS, userId, inquiryId)
Each of these must be updated to match the new signature
public static NotificationEvent create(NotificationStatus status, Long userId)(e.g. strip out the third argument, or introduce a dedicated overload/handler for the targetId).
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/infra/notify/dto/NotificationEvent.java
around lines 12 to 17, the create method was changed to accept only two
parameters (NotificationStatus and userId). Update all call sites in
PaymentTxEventListener.java, RefundTxEventListener.java, and
InquiryAnswerTxEventListener.java to remove the third argument (examSchoolId,
examApplicationId, inquiryId) from NotificationEvent.create calls. If the third
argument is needed, consider adding a new overloaded create method or another
handler to accommodate it.
🛠️ Refactor suggestion
Consider making the factory method more generic or specific.
The create method only accepts SIGN_UP_SUCCESS status but has a generic signature accepting any NotificationStatus. This creates a mismatch between the method signature and implementation.
Consider either:
- Making the method more generic by removing the status restriction
- Renaming to
createSignUpSuccessEvent(Long userId)to reflect its specific purpose
- public static NotificationEvent create(NotificationStatus status, Long userId) {
- if (!NotificationStatus.SIGN_UP_SUCCESS.equals(status)) {
- throw new CustomRuntimeException(ErrorCode.INVALID_NOTIFICATION_STATUS);
- }
- return new NotificationEvent(status, userId, null);
- }
+ public static NotificationEvent createSignUpSuccessEvent(Long userId) {
+ return new NotificationEvent(NotificationStatus.SIGN_UP_SUCCESS, userId, null);
+ }📝 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 NotificationEvent create(NotificationStatus status, Long userId) { | |
| if (!NotificationStatus.SIGN_UP_SUCCESS.equals(status)) { | |
| throw new IllegalArgumentException("Unknown notification status: " + status); | |
| throw new CustomRuntimeException(ErrorCode.INVALID_NOTIFICATION_STATUS); | |
| } | |
| return new NotificationEvent(status, null, null); | |
| return new NotificationEvent(status, userId, null); | |
| } | |
| // before: public static NotificationEvent create(NotificationStatus status, Long userId) { ... } | |
| public static NotificationEvent createSignUpSuccessEvent(Long userId) { | |
| return new NotificationEvent(NotificationStatus.SIGN_UP_SUCCESS, userId, null); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/infra/notify/dto/NotificationEvent.java
lines 12 to 17, the create method signature accepts any NotificationStatus but
only allows SIGN_UP_SUCCESS, causing a mismatch. To fix this, either remove the
status parameter to make the method specific to SIGN_UP_SUCCESS and rename it to
createSignUpSuccessEvent(Long userId), or adjust the method to accept any
NotificationStatus and remove the restriction check inside.
| headers.add(HttpHeaders.SET_COOKIE, CookieBuilderUtil.createLocalCookieString( | ||
| CookieBuilderUtil.ACCESS_TOKEN_COOKIE_NAME, | ||
| token.accessToken(), | ||
| token.accessTokenExpireTime() | ||
| )); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Method name change improves clarity, but verify environment appropriateness.
The change from temporaryCookieString to createLocalCookieString makes the method name more descriptive and aligns with the cookie utility refactoring. However, ensure this local environment cookie configuration (Secure=false, SameSite=Lax) is appropriate for your deployment environment.
Also applies to: 50-54
🌐 Web query:
What are the security implications of using cookies with Secure=false and SameSite=Lax in production environments?
💡 Result:
Using cookies with Secure=false and SameSite=Lax in production environments has several security implications:
-
Secure=false:
- Cookies with
Secure=falseare transmitted over both HTTP and HTTPS connections. - This means the cookie can be sent over unencrypted HTTP connections, exposing it to interception by network attackers (e.g., via man-in-the-middle attacks or packet sniffing).
- Sensitive cookies like session tokens should always have Secure=true to ensure they are only sent over encrypted HTTPS connections, protecting them from being leaked.
- Cookies with
-
SameSite=Lax:
SameSite=Laxoffers some protection against Cross-Site Request Forgery (CSRF) by restricting cookie sending on some third-party requests, but it still allows cookies to be sent with top-level navigations and some GET requests initiated by third parties.- This setting is less restrictive than
SameSite=Strict, which blocks cookies in all cross-site requests. - While Lax is generally a good balance, in very security-sensitive contexts, it might not be sufficient to fully prevent CSRF attacks.
- It also means that if attackers trick a user to navigate to a malicious link (top-level navigation), the cookie may still be sent, potentially allowing certain attacks.
Summary:
- Having
Secure=falseexposes cookies to interception on non-secure connections and is a critical security risk. - Having
SameSite=Laxprovides partial CSRF protection but may not be sufficient for high-security applications. - Best practice is to set
Secure=truein production to ensure cookies are only sent over HTTPS, and to carefully consider the appropriate SameSite setting based on the application’s threat model (oftenSameSite=StrictorSameSite=Laxcombined with other CSRF protections).
Would you like recommendations on best cookie settings for specific security concerns?
This will help us see if there are production-safe methods available for secure cookies.
🏁 Script executed:
#!/bin/bash
# Locate the CookieBuilderUtil class
rg --context 3 "class CookieBuilderUtil" -n
# List all cookie string creation methods in CookieBuilderUtil
rg "create.*CookieString" -nLength of output: 2315
Use the production cookie builder in controllers for secure cookies
The utility provides createLocalCookieString, createDevelopCookieString, and createProductionCookieString. In production you must use createProductionCookieString (to enforce Secure=true and stricter SameSite settings) instead of the local variant.
Please update the following locations:
- src/main/java/life/mosu/mosuserver/presentation/auth/SignUpController.java (lines 45, 50)
- src/main/java/life/mosu/mosuserver/presentation/auth/AuthController.java (lines 48, 53)
- src/main/java/life/mosu/mosuserver/presentation/auth/MasterController.java (lines 65, 70)
For example, replace:
- headers.add(HttpHeaders.SET_COOKIE,
- CookieBuilderUtil.createLocalCookieString(...));
+ headers.add(HttpHeaders.SET_COOKIE,
+ CookieBuilderUtil.createProductionCookieString(...));This ensures cookies are only sent over HTTPS and gain the intended SameSite protection in production.
📝 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.
| headers.add(HttpHeaders.SET_COOKIE, CookieBuilderUtil.createLocalCookieString( | |
| CookieBuilderUtil.ACCESS_TOKEN_COOKIE_NAME, | |
| token.accessToken(), | |
| token.accessTokenExpireTime() | |
| )); | |
| headers.add(HttpHeaders.SET_COOKIE, CookieBuilderUtil.createProductionCookieString( | |
| CookieBuilderUtil.ACCESS_TOKEN_COOKIE_NAME, | |
| token.accessToken(), | |
| token.accessTokenExpireTime() | |
| )); |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/presentation/auth/SignUpController.java
around lines 45 to 49, replace calls to
CookieBuilderUtil.createLocalCookieString with
CookieBuilderUtil.createProductionCookieString to ensure cookies are set with
Secure=true and stricter SameSite attributes for production security. Make the
same replacement at line 50 as well. This change enforces that cookies are only
sent over HTTPS and have proper SameSite protection.
| public static UserJpaEntity mosu_user() { | ||
| return UserJpaEntity.builder() | ||
| .loginId(USER_ID) | ||
| .password(USER_PASSWORD) | ||
| .gender(Gender.FEMALE) | ||
| .name(USER_NAME) | ||
| .phoneNumber(USER_PHONE_NUMBER) | ||
| .birth(LocalDate.of(2005, 12, 1)) | ||
| .userRole(UserRole.ROLE_USER) | ||
| .agreedToTermsOfService(true) | ||
| .agreedToPrivacyPolicy(true) | ||
| .agreedToMarketing(true) | ||
| .provider(AuthProvider.MOSU) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to eliminate code duplication.
The mosu_user() and kakao_user() methods are nearly identical, differing only in the AuthProvider. This creates maintenance overhead and violates DRY principles.
Consider refactoring to a single parameterized method:
- public static UserJpaEntity mosu_user() {
- return UserJpaEntity.builder()
- .loginId(USER_ID)
- .password(USER_PASSWORD)
- .gender(Gender.FEMALE)
- .name(USER_NAME)
- .phoneNumber(USER_PHONE_NUMBER)
- .birth(LocalDate.of(2005, 12, 1))
- .userRole(UserRole.ROLE_USER)
- .agreedToTermsOfService(true)
- .agreedToPrivacyPolicy(true)
- .agreedToMarketing(true)
- .provider(AuthProvider.MOSU)
- .build();
- }
+ public static UserJpaEntity createUser(AuthProvider provider) {
+ return UserJpaEntity.builder()
+ .loginId(USER_ID)
+ .password(USER_PASSWORD)
+ .gender(Gender.FEMALE)
+ .name(USER_NAME)
+ .phoneNumber(USER_PHONE_NUMBER)
+ .birth(USER_BIRTH)
+ .userRole(UserRole.ROLE_USER)
+ .agreedToTermsOfService(true)
+ .agreedToPrivacyPolicy(true)
+ .agreedToMarketing(true)
+ .provider(provider)
+ .build();
+ }
+
+ public static UserJpaEntity mosuUser() {
+ return createUser(AuthProvider.MOSU);
+ }📝 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 UserJpaEntity mosu_user() { | |
| return UserJpaEntity.builder() | |
| .loginId(USER_ID) | |
| .password(USER_PASSWORD) | |
| .gender(Gender.FEMALE) | |
| .name(USER_NAME) | |
| .phoneNumber(USER_PHONE_NUMBER) | |
| .birth(LocalDate.of(2005, 12, 1)) | |
| .userRole(UserRole.ROLE_USER) | |
| .agreedToTermsOfService(true) | |
| .agreedToPrivacyPolicy(true) | |
| .agreedToMarketing(true) | |
| .provider(AuthProvider.MOSU) | |
| .build(); | |
| } | |
| public static UserJpaEntity createUser(AuthProvider provider) { | |
| return UserJpaEntity.builder() | |
| .loginId(USER_ID) | |
| .password(USER_PASSWORD) | |
| .gender(Gender.FEMALE) | |
| .name(USER_NAME) | |
| .phoneNumber(USER_PHONE_NUMBER) | |
| .birth(USER_BIRTH) | |
| .userRole(UserRole.ROLE_USER) | |
| .agreedToTermsOfService(true) | |
| .agreedToPrivacyPolicy(true) | |
| .agreedToMarketing(true) | |
| .provider(provider) | |
| .build(); | |
| } | |
| public static UserJpaEntity mosuUser() { | |
| return createUser(AuthProvider.MOSU); | |
| } |
🤖 Prompt for AI Agents
In src/test/java/life/mosu/mosuserver/global/fixture/UserTestFixture.java
between lines 17 and 31, the mosu_user() and kakao_user() methods are almost
identical except for the AuthProvider value. Refactor by creating a single
method that accepts AuthProvider as a parameter and returns the UserJpaEntity
with all other fields set the same. Replace the existing methods by calling this
new parameterized method with the appropriate AuthProvider to eliminate
duplication and improve maintainability.
| .gender(Gender.FEMALE) | ||
| .name(USER_NAME) | ||
| .phoneNumber(USER_PHONE_NUMBER) | ||
| .birth(LocalDate.of(2005, 12, 1)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use the defined constant consistently.
The birth date is hardcoded as LocalDate.of(2005, 12, 1) in both methods but USER_BIRTH constant is defined at line 15. This creates inconsistency and potential maintenance issues.
Use the constant instead:
- .birth(LocalDate.of(2005, 12, 1))
+ .birth(USER_BIRTH)Also applies to: 40-40
🤖 Prompt for AI Agents
In src/test/java/life/mosu/mosuserver/global/fixture/UserTestFixture.java at
lines 24 and 40, replace the hardcoded birth date LocalDate.of(2005, 12, 1) with
the already defined USER_BIRTH constant to ensure consistency and easier
maintenance.
| public static UserJpaEntity kakao_user() { | ||
| return UserJpaEntity.builder() | ||
| .loginId(USER_ID) | ||
| .password(USER_PASSWORD) | ||
| .gender(Gender.FEMALE) | ||
| .name(USER_NAME) | ||
| .phoneNumber(USER_PHONE_NUMBER) | ||
| .birth(LocalDate.of(2005, 12, 1)) | ||
| .userRole(UserRole.ROLE_USER) | ||
| .agreedToTermsOfService(true) | ||
| .agreedToPrivacyPolicy(true) | ||
| .agreedToMarketing(true) | ||
| .provider(AuthProvider.KAKAO) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Apply the same refactoring approach.
This method has the same duplication issue as mosu_user().
Replace with:
- public static UserJpaEntity kakao_user() {
- return UserJpaEntity.builder()
- .loginId(USER_ID)
- .password(USER_PASSWORD)
- .gender(Gender.FEMALE)
- .name(USER_NAME)
- .phoneNumber(USER_PHONE_NUMBER)
- .birth(LocalDate.of(2005, 12, 1))
- .userRole(UserRole.ROLE_USER)
- .agreedToTermsOfService(true)
- .agreedToPrivacyPolicy(true)
- .agreedToMarketing(true)
- .provider(AuthProvider.KAKAO)
- .build();
- }
+ public static UserJpaEntity kakaoUser() {
+ return createUser(AuthProvider.KAKAO);
+ }📝 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 UserJpaEntity kakao_user() { | |
| return UserJpaEntity.builder() | |
| .loginId(USER_ID) | |
| .password(USER_PASSWORD) | |
| .gender(Gender.FEMALE) | |
| .name(USER_NAME) | |
| .phoneNumber(USER_PHONE_NUMBER) | |
| .birth(LocalDate.of(2005, 12, 1)) | |
| .userRole(UserRole.ROLE_USER) | |
| .agreedToTermsOfService(true) | |
| .agreedToPrivacyPolicy(true) | |
| .agreedToMarketing(true) | |
| .provider(AuthProvider.KAKAO) | |
| .build(); | |
| } | |
| public static UserJpaEntity kakaoUser() { | |
| return createUser(AuthProvider.KAKAO); | |
| } |
🤖 Prompt for AI Agents
In src/test/java/life/mosu/mosuserver/global/fixture/UserTestFixture.java around
lines 33 to 47, the kakao_user() method duplicates code similar to mosu_user().
Refactor kakao_user() to reuse a common private builder method or a shared user
creation method that sets common fields, then customize only the differing
fields like provider and gender. This reduces duplication and improves
maintainability.
2 participants
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests