MOSU-264 refactor: Dirty Checking을 위한 Transaction 분리 #265
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. WalkthroughThe changes consolidate Lombok and Testcontainers dependencies in the Gradle build file, introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant OAuthUserService
participant OAuthUserPersistenceProcessor
participant UserJpaRepository
participant UserJpaEntity
OAuthUserService->>OAuthUserPersistenceProcessor: process(userInfo)
OAuthUserPersistenceProcessor->>UserJpaRepository: findByLoginId(email)
alt User exists
OAuthUserPersistenceProcessor->>UserJpaEntity: updateOAuthUser(...)
OAuthUserPersistenceProcessor-->>OAuthUserService: return updated UserJpaEntity
else User does not exist
OAuthUserPersistenceProcessor->>UserJpaRepository: save(new UserJpaEntity)
OAuthUserPersistenceProcessor-->>OAuthUserService: return new UserJpaEntity
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. Possibly related PRs
Suggested labels
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous 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!
I've refactored the OAuth user handling logic to separate the transaction management for user persistence. The core change involves extracting the user creation and update logic into a dedicated OAuthUserPersistenceProcessor component. This ensures that the database operations for OAuth users are managed within their own explicit transaction boundaries, which is crucial for 'Dirty Checking' to function correctly and predictably. The OAuthUserService now orchestrates the OAuth flow by delegating persistence concerns to this new processor, leading to a cleaner separation of concerns. Additionally, I've updated the user entity to properly handle marketing agreement status and added a specific test case for parsing these terms.
Highlights
- Transaction Separation for OAuth User Persistence: I've introduced a new component,
OAuthUserPersistenceProcessor, to encapsulate the logic for persisting OAuth user data. This component now handles whether to update an existing user or create a new one, and itsprocessmethod is explicitly marked with@Transactional. - Refactoring of OAuthUserService: The
OAuthUserServicehas been refactored to delegate all user persistence operations to the newOAuthUserPersistenceProcessor. This removes the directUserJpaRepositorydependency and theupdateOrWritemethod fromOAuthUserService, making it more focused on OAuth flow orchestration rather than data persistence details. - User Entity Update for Marketing Agreement: The
UserJpaEntity'supdateOAuthUsermethod has been enhanced to accept and persist theagreedToMarketingstatus, ensuring that marketing agreement preferences are correctly updated during OAuth user information synchronization. - New Test for Marketing Agreement Parsing: I've added a new test class,
OAuthUserServiceTest, specifically to validate the parsing of marketing agreement terms from Kakao's service response. This test ensures robustness in handling various scenarios for user consent. - Build Dependency Cleanup: Minor adjustments were made to
build.gradleto reorganize Lombok dependencies and remove redundant Testcontainers entries, contributing to a cleaner and more efficient build configuration.
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 successfully refactors the OAuth user persistence logic by moving it into a separate OAuthUserPersistenceProcessor with its own transaction boundary. This is a good improvement for separation of concerns.
My review includes feedback on improving code quality and correctness:
- Adhering to Java naming conventions for fields.
- Improving consistency of default values and removing redundant code in the new persistence processor.
- Enhancing the new unit test to be more robust and correctly test the service behavior.
Overall, the changes are good, and with a few adjustments, the code quality can be further improved.
| public class OAuthUserService extends DefaultOAuth2UserService { | ||
|
|
||
| private final UserJpaRepository userRepository; | ||
| private final OAuthUserPersistenceProcessor OAuthUserPersistenceProcessor; |
There was a problem hiding this comment.
The field name OAuthUserPersistenceProcessor does not follow Java naming conventions, which state that field names should start with a lowercase letter (camelCase). Please rename it to oAuthUserPersistenceProcessor for consistency and readability. You will also need to update its usage on line 60.
| private final OAuthUserPersistenceProcessor OAuthUserPersistenceProcessor; | |
| private final OAuthUserPersistenceProcessor oAuthUserPersistenceProcessor; |
| final UserJpaEntity newUser = UserJpaEntity.builder() | ||
| .loginId(info.email() != null ? info.email() : "NA") | ||
| .gender(info.gender() != null ? info.gender() : Gender.PENDING) | ||
| .name(info.name() != null ? info.name() : "NA") | ||
| .birth(info.birthDay() != null ? info.birthDay() | ||
| : LocalDate.EPOCH) | ||
| .phoneNumber(info.phoneNumber() != null ? info.phoneNumber() | ||
| : "010-0000-0000") | ||
| .userRole(UserRole.ROLE_PENDING) | ||
| .provider(AuthProvider.KAKAO) | ||
| .agreedToTermsOfService(true) | ||
| .agreedToPrivacyPolicy(true) | ||
| .agreedToMarketing(info.marketingAgreed()) | ||
| .build(); |
There was a problem hiding this comment.
There are a few issues in this block for creating a new user:
-
Inconsistent Default Birth Date: The default birth date here is
LocalDate.EPOCH, which is different from the one used for updates (LocalDate.of(1900, 1, 1)on line 29). This inconsistency can lead to different data for users depending on whether they were created or updated on first login, and should be fixed. -
Redundant Null Checks: The
OAuthUserInfofactory guarantees thatgender,name, andbirthDayare not null. The null checks for these fields are redundant and should be removed. -
Magic Values: Strings like
"NA"and"010-0000-0000"should be extracted to named constants for better readability and maintainability.
final UserJpaEntity newUser = UserJpaEntity.builder()
.loginId(info.email() != null ? info.email() : "NA") // Consider using a constant
.gender(info.gender())
.name(info.name())
.birth(info.birthDay())
.phoneNumber(info.phoneNumber() != null ? info.phoneNumber()
: "010-0000-0000") // Consider using a constant
.userRole(UserRole.ROLE_PENDING)
.provider(AuthProvider.KAKAO)
.agreedToTermsOfService(true)
.agreedToPrivacyPolicy(true)
.agreedToMarketing(info.marketingAgreed())
.build();| .map(existingUser -> { | ||
| existingUser.updateOAuthUser( | ||
| info.gender(), | ||
| info.name(), | ||
| info.phoneNumber(), | ||
| info.birthDay() != null ? info.birthDay() : LocalDate.of(1900, 1, 1), | ||
| info.marketingAgreed()); | ||
| return existingUser; | ||
| }) |
There was a problem hiding this comment.
The .map() block can be made more concise by using Optional.peek() (available since Java 9). Also, the OAuthUserInfo factory guarantees that birthDay is not null, so the null check is redundant and can be removed.
.peek(existingUser -> existingUser.updateOAuthUser(
info.gender(),
info.name(),
info.phoneNumber(),
info.birthDay(),
info.marketingAgreed()))| @Test | ||
| @DisplayName("Service Terms 응답에 마케팅 동의(terms_03)가 없는 경우 false로 파싱한다") | ||
| void 마케팅_동의_여부를_파싱한다() { | ||
| Map<String, Object> term1 = Map.of( | ||
| "tag", "terms_02", | ||
| "required", true, | ||
| "agreed", true | ||
| ); | ||
| Map<String, Object> term2 = Map.of( | ||
| "tag", "terms_01", | ||
| "required", true, | ||
| "agreed", true | ||
| ); | ||
|
|
||
| Map<String, Object> serviceTermsAttributes = new HashMap<>(); | ||
| serviceTermsAttributes.put("id", 4342056184L); | ||
| serviceTermsAttributes.put("service_terms", List.of(term1, term2)); | ||
|
|
||
| boolean agreedToMarketing = false; | ||
| if (serviceTermsAttributes.get("service_terms") instanceof List<?> termsList) { | ||
| agreedToMarketing = termsList.stream() | ||
| .filter(term -> term instanceof Map) | ||
| .map(term -> (Map<String, Object>) term) | ||
| .filter(termMap -> "terms_03".equals(termMap.get("tag"))) | ||
| .findFirst() | ||
| .map(termMap -> (Boolean) termMap.get("agreed")) | ||
| .orElse(false); | ||
| } | ||
|
|
||
| Assertions.assertFalse(agreedToMarketing); | ||
| } |
There was a problem hiding this comment.
This test method copies logic from OAuthUserService instead of testing the service's behavior directly. This makes the test brittle; if the implementation in the service changes, this test won't fail but will become irrelevant.
A better approach for a unit test is to mock dependencies of OAuthUserService, call the loadUser method, and assert the outcome. This would provide a more robust and meaningful test of the service's functionality.
![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 (1)
src/main/java/life/mosu/mosuserver/application/oauth/OAuthUserService.java (1)
37-47: Harden marketing-consent parsing and avoid raw casts; extract to a methodCurrent parsing relies on raw Map casts and assumes
agreedis Boolean, riskingClassCastException. Also, the "terms_03" tag is hard-coded inline.
- Replace block with a dedicated parser method and a named constant.
- Use
Boolean.TRUE.equals(...)for safe boolean extraction.- boolean agreedToMarketing = false; - if (serviceTermsAttributes.get("service_terms") instanceof List<?> termsList) { - agreedToMarketing = termsList.stream() - .filter(term -> term instanceof Map) - .map(term -> (Map<String, Object>) term) - .filter(termMap -> - "terms_03".equals(termMap.get("tag"))) - .findFirst() - .map(termMap -> (Boolean) termMap.get("agreed")) - .orElse(false); - } + boolean agreedToMarketing = parseMarketingAgreed(serviceTermsAttributes);Add the following helper (outside this range):
private static final String KAKAO_MARKETING_TAG = "terms_03"; @SuppressWarnings("unchecked") private boolean parseMarketingAgreed(Map<String, Object> attrs) { Object list = attrs.get("service_terms"); if (!(list instanceof List<?> termsList)) return false; for (Object term : termsList) { if (!(term instanceof Map<?, ?> termMap)) continue; Object tag = termMap.get("tag"); if (!KAKAO_MARKETING_TAG.equals(tag)) continue; return Boolean.TRUE.equals(termMap.get("agreed")); } return false; }
🧹 Nitpick comments (8)
src/main/java/life/mosu/mosuserver/domain/user/entity/UserJpaEntity.java (1)
92-104: Consider keeping update semantics consistent with creation pathCreation uses a different default for birth (LocalDate.EPOCH) than update (1900-01-01). This asymmetry can lead to confusing data states. Prefer a single constant or null when unknown.
build.gradle (1)
36-42: Avoid redundanttestImplementationfor LombokFor Lombok,
compileOnly/annotationProcessor(main) andtestCompileOnly/testAnnotationProcessor(test) are sufficient.testImplementation 'org.projectlombok:lombok'is unnecessary and can be removed.// Lombok compileOnly 'org.projectlombok:lombok' annotationProcessor 'org.projectlombok:lombok' testCompileOnly 'org.projectlombok:lombok' - testImplementation 'org.projectlombok:lombok' testAnnotationProcessor 'org.projectlombok:lombok'src/main/java/life/mosu/mosuserver/application/oauth/OAuthUserService.java (3)
23-26: Field name violates Java naming conventions; rename to lowerCamelCaseUse lowerCamelCase for fields to avoid confusion with the type name.
- private final OAuthUserPersistenceProcessor OAuthUserPersistenceProcessor; + private final OAuthUserPersistenceProcessor oauthUserPersistenceProcessor;And update usages accordingly (see Line 60).
49-49: Sanitize log message and avoid logging sensitive payloads elsewhere
- Tweak message text for readability.
- Ensure no PII is logged. Consider removing the full
response.toString()log ingetServiceTerms.- log.info("동의 여부{}", agreedToMarketing); + log.info("Kakao marketing consent: {}", agreedToMarketing);
60-61: Update field usage after renamingAfter renaming the field, update the invocation.
- final UserJpaEntity oAuthUser = OAuthUserPersistenceProcessor.process(userInfo); + final UserJpaEntity oAuthUser = oauthUserPersistenceProcessor.process(userInfo);src/main/java/life/mosu/mosuserver/application/oauth/OAuthUserPersistenceProcessor.java (3)
21-22: Verify transaction boundary matches “transaction separation for Dirty Checking” intent
@Transactionaluses default propagation (REQUIRED), joining any outer transaction. If the goal is to isolate commit/flush for dirty checking independently, considerpropagation = REQUIRES_NEWand document why.Would you like this changed to
@Transactional(propagation = Propagation.REQUIRES_NEW)with tests around flush/commit semantics?
40-42: Avoid hard-coded placeholder phone numbers; use null or a domain-appropriate defaultPersisting a fake phone like
010-0000-0000can leak into user-facing flows. Prefer null (if the column allows) or an explicit “unknown” state.- .phoneNumber(info.phoneNumber() != null ? info.phoneNumber() - : "010-0000-0000") + .phoneNumber(info.phoneNumber()) // allow null when unknown
44-47: Legal/compliance: defaulting TOS/Privacy to true may be riskyYou set
agreedToTermsOfService(true)andagreedToPrivacyPolicy(true)without deriving from service terms. Confirm that upstream consent actually covers these and is auditable.If Kakao returns required consent tags (e.g.,
terms_01,terms_02), we can parse them similar to marketing and set booleans based on the response. I can add that parsing to the service and wire it throughOAuthUserInfo.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
build.gradle(1 hunks)src/main/java/life/mosu/mosuserver/application/oauth/OAuthUserPersistenceProcessor.java(1 hunks)src/main/java/life/mosu/mosuserver/application/oauth/OAuthUserService.java(3 hunks)src/main/java/life/mosu/mosuserver/domain/user/entity/UserJpaEntity.java(1 hunks)src/test/java/life/mosu/mosuserver/application/oauth/OAuthUserServiceTest.java(1 hunks)
🔇 Additional comments (1)
src/main/java/life/mosu/mosuserver/domain/user/entity/UserJpaEntity.java (1)
92-104: Method signature change looks correct; ensure call sites updated and DB defaults alignAdding
agreedToMarketingand assigning it is straightforward. Verify:
- All callers updated to pass the new flag.
- Migration/state: existing rows have a sensible default for
agreed_to_marketing(DB default or handled at read time).
| info.birthDay() != null ? info.birthDay() : LocalDate.of(1900, 1, 1), | ||
| info.marketingAgreed()); | ||
| return existingUser; | ||
| }) | ||
| .orElseGet(() -> { | ||
| final UserJpaEntity newUser = UserJpaEntity.builder() | ||
| .loginId(info.email() != null ? info.email() : "NA") | ||
| .gender(info.gender() != null ? info.gender() : Gender.PENDING) | ||
| .name(info.name() != null ? info.name() : "NA") | ||
| .birth(info.birthDay() != null ? info.birthDay() | ||
| : LocalDate.EPOCH) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent default birth date between update (1900-01-01) and create (EPOCH)
Update path defaults to 1900-01-01; create path to 1970-01-01. Pick a single sentinel or allow null. Consider a constant to avoid drift.
- info.birthDay() != null ? info.birthDay() : LocalDate.of(1900, 1, 1),
+ info.birthDay() != null ? info.birthDay() : DEFAULT_BIRTH,- .birth(info.birthDay() != null ? info.birthDay()
- : LocalDate.EPOCH)
+ .birth(info.birthDay() != null ? info.birthDay() : DEFAULT_BIRTH)Additionally, add to the class:
private static final LocalDate DEFAULT_BIRTH = LocalDate.of(1900, 1, 1);🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/oauth/OAuthUserPersistenceProcessor.java
between lines 29 and 39, the default birth date used when updating a user is
LocalDate.of(1900, 1, 1), but when creating a new user it is LocalDate.EPOCH
(1970-01-01), causing inconsistency. Define a private static final LocalDate
constant DEFAULT_BIRTH set to LocalDate.of(1900, 1, 1) at the class level, then
replace both default birth date usages with this constant to ensure consistency
and avoid drift.
| .loginId(info.email() != null ? info.email() : "NA") | ||
| .gender(info.gender() != null ? info.gender() : Gender.PENDING) |
There was a problem hiding this comment.
Critical: fallback loginId "NA" will violate the unique constraint
login_id is unique. Using "NA" for multiple users without email will cause constraint violations on the second insert. Generate a unique fallback (e.g., UUID or provider-based id).
- .loginId(info.email() != null ? info.email() : "NA")
+ .loginId(info.email() != null ? info.email() : java.util.UUID.randomUUID().toString())If a deterministic provider user-id exists in info, prefer using it (e.g., kakao:{providerUserId}) to keep traceability.
📝 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.
| .loginId(info.email() != null ? info.email() : "NA") | |
| .gender(info.gender() != null ? info.gender() : Gender.PENDING) | |
| // ... | |
| .loginId(info.email() != null | |
| ? info.email() | |
| : java.util.UUID.randomUUID().toString()) | |
| .gender(info.gender() != null | |
| ? info.gender() | |
| : Gender.PENDING) | |
| // ... |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/oauth/OAuthUserPersistenceProcessor.java
around lines 35 to 36, the fallback value "NA" for loginId violates the unique
constraint because multiple users without email will share it. Replace the
fallback with a unique identifier by checking if a deterministic provider
user-id exists in info and use a format like "provider:{providerUserId}" for
traceability; if not available, generate a UUID to ensure uniqueness for each
user.
src/test/java/life/mosu/mosuserver/application/oauth/OAuthUserServiceTest.java
Outdated
Show resolved
Hide resolved
| boolean agreedToMarketing = false; | ||
| if (serviceTermsAttributes.get("service_terms") instanceof List<?> termsList) { | ||
| agreedToMarketing = termsList.stream() | ||
| .filter(term -> term instanceof Map) | ||
| .map(term -> (Map<String, Object>) term) | ||
| .filter(termMap -> "terms_03".equals(termMap.get("tag"))) | ||
| .findFirst() | ||
| .map(termMap -> (Boolean) termMap.get("agreed")) | ||
| .orElse(false); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid duplicating production parsing logic in tests; extract and test a method instead
This test re-implements the parsing logic instead of exercising production code. Extract the parsing into a method (e.g., parseMarketingAgreed) in the service and test that method with positive/negative cases.
I can generate parameterized tests covering presence/absence of terms_03, non-boolean agreed, and empty lists once the helper method is introduced.
🤖 Prompt for AI Agents
In
src/test/java/life/mosu/mosuserver/application/oauth/OAuthUserServiceTest.java
around lines 33 to 42, the test duplicates the parsing logic for extracting the
marketing agreement from service terms instead of using the production code. To
fix this, move the parsing logic into a dedicated method like
parseMarketingAgreed in the OAuthUserService class, then update the test to call
this method directly. After that, create parameterized tests for this method
covering cases such as presence or absence of "terms_03", non-boolean "agreed"
values, and empty lists to properly validate the parsing behavior.
1 participant
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit