MOSU-373 refactor: profile 등록 동시 요청 처리 #379
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. WalkthroughAdds handling in ProfileService.registerProfile to catch DataIntegrityViolationException and throw CustomRuntimeException(ErrorCode.PROFILE_ALREADY_EXISTS, userId). Adds a concurrency integration test class that attempts simultaneous registrations, and updates build.gradle to exclude that test from test-source compilation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant PS as ProfileService
participant PR as ProfileRepository
participant DB as Database
C->>PS: registerProfile(userId, request)
PS->>PR: save(profile)
PR->>DB: INSERT profile (unique user_id)
alt Insert succeeds
DB-->>PR: OK
PR-->>PS: Profile
PS-->>C: 201 Created (profile)
else Unique constraint violation
DB-->>PR: Error: duplicate key
PR-->>PS: DataIntegrityViolationException
PS->>C: throws CustomRuntimeException(PROFILE_ALREADY_EXISTS)
note right of PS: Mapped for 4xx handling by global exception layer
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Poem
✨ Finishing touches🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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. Comment Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
|
![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!
이 PR은 프로필 등록 과정에서 발생할 수 있는 동시성 문제를 효율적으로 관리하기 위한 개선사항을 포함합니다. 데이터베이스의 고유 제약 조건을 활용하여 중복 등록 시 발생하는 오류를 명확하게 처리하고, 이를 검증하는 자동화된 테스트를 도입하여 시스템의 견고성을 높였습니다.
Highlights
- 프로필 등록 동시 요청 처리: 프로필 등록 시 동일한 사용자에 대한 동시 요청으로 인해 발생할 수 있는 데이터베이스의 고유 제약 조건 위반 오류를
DataIntegrityViolationException으로 감지하고, 이를PROFILE_ALREADY_EXISTS라는 커스텀 예외로 변환하여 처리하도록ProfileService에 로직을 추가했습니다. - 동시성 테스트 추가: 프로필 등록 기능의 동시성 문제를 검증하기 위해
ProfileServiceConcurrencyTest를 새로 추가했습니다. 이 테스트는 여러 스레드에서 동시에 프로필 등록을 시도할 때, 하나의 요청만 성공하고 나머지 요청은 예상대로CustomRuntimeException을 발생시키는지 확인합니다.
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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
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
프로필 등록 시 발생할 수 있는 동시성 문제를 데이터베이스의 Unique 제약 조건을 활용하여 해결하신 점이 인상적입니다. 이 방식은 별도의 Lock을 사용하지 않아 리소스 측면에서 효율적이며, 코드도 간결하게 유지할 수 있는 좋은 접근법입니다. 또한, 동시성 문제를 검증하기 위한 테스트 케이스를 추가하여 변경 사항의 안정성을 높여주신 점도 훌륭합니다. 전반적으로 좋은 변경이라고 생각합니다. 다만, 추가된 테스트 코드의 검증 로직을 조금 더 명확하게 개선할 수 있는 부분이 있어 리뷰 코멘트를 남겼습니다.
| assertThat(exceptions).hasSize(1); | ||
|
|
||
| Exception thrownException = exceptions.getFirst(); | ||
| assertThat(thrownException).isInstanceOf(CustomRuntimeException.class); |
There was a problem hiding this comment.
좋은 테스트입니다. 테스트의 정확성을 더 높이기 위해, 발생하는 예외가 CustomRuntimeException이라는 것만 검증하는 것보다 어떤 ErrorCode를 가지고 있는지까지 확인하는 것이 좋습니다. 이렇게 하면 의도치 않은 다른 CustomRuntimeException이 발생했을 때 테스트가 실패하여 문제를 더 빨리 발견할 수 있습니다.
assertThat(thrownException).isInstanceOfSatisfying(CustomRuntimeException.class, e ->
assertThat(e.getCode()).isEqualTo("PROFILE_ALREADY_EXISTS"));![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/life/mosu/mosuserver/application/profile/ProfileService.java (1)
39-53: Catch duplicate-key inside the method: flush right after save.Without an explicit flush, the unique-constraint violation may surface at transaction commit (outside this try/catch), bypassing your intended mapping to CustomRuntimeException. Force the insert to execute here.
- ProfileJpaEntity profile = request.toEntity(userId); - profileJpaRepository.save(profile); + ProfileJpaEntity profile = request.toEntity(userId); + profileJpaRepository.saveAndFlush(profile); // ensure constraint violation is thrown here
🧹 Nitpick comments (5)
src/main/java/life/mosu/mosuserver/application/profile/ProfileService.java (2)
48-50: Narrow the mapping to unique-constraint only (don’t collapse all DataIntegrityViolationException).DataIntegrityViolationException also covers NOT NULL, FK, length, etc. Map to PROFILE_ALREADY_EXISTS only when it’s a unique/duplicate-key error; otherwise rethrow.
- } catch (DataIntegrityViolationException ex) { - throw new CustomRuntimeException(ErrorCode.PROFILE_ALREADY_EXISTS, userId); + } catch (DataIntegrityViolationException ex) { + if (isUniqueConstraintViolation(ex)) { + throw new CustomRuntimeException(ErrorCode.PROFILE_ALREADY_EXISTS, userId); + } + throw ex;Add helper:
// Place inside ProfileService private boolean isUniqueConstraintViolation(Throwable t) { for (Throwable c = t; c != null; c = c.getCause()) { if (c instanceof org.hibernate.exception.ConstraintViolationException) { String sqlState = ((org.hibernate.exception.ConstraintViolationException) c).getSQLState(); if ("23505".equals(sqlState) || "23000".equals(sqlState)) return true; // Postgres/H2, MySQL/MariaDB } if (c instanceof java.sql.SQLIntegrityConstraintViolationException) return true; } return false; }
74-77: Method name misleads; it checks role, not profile existence.checkIfProfileExistsForUser gates on ROLE_PENDING rather than actual profile presence. Consider renaming for clarity (e.g., assertUserIsPendingForProfileRegistration) or checking repository if you want semantic alignment.
src/test/java/life/mosu/mosuserver/application/profile/ProfileServiceConcurrencyTest.java (3)
105-109: Assert the exact error code for stronger guarantees.Validate that the mapped exception is PROFILE_ALREADY_EXISTS, not just the type.
- Exception thrownException = exceptions.get(0); - assertThat(thrownException).isInstanceOf(CustomRuntimeException.class); + Exception thrownException = exceptions.get(0); + assertThat(thrownException).isInstanceOf(CustomRuntimeException.class); + assertThat(((CustomRuntimeException) thrownException).getErrorCode()) + .isEqualTo(life.mosu.mosuserver.global.exception.ErrorCode.PROFILE_ALREADY_EXISTS);
82-99: Make the start barrier explicit and time-bounded to reduce flakiness.CountDownLatch-as-barrier works but can hang if a task errors before await. Use a CyclicBarrier or add a timeout to await.
- latch.countDown(); - latch.await(); + latch.countDown(); + if (!latch.await(3, TimeUnit.SECONDS)) { + throw new IllegalStateException("Barrier timeout"); + }
72-109: Alternative: capture task outcomes via Future#get for deterministic completion.Submitting Callables and collecting results/exceptions via get() simplifies shutdown/await logic.
If you want, I can provide a Future-based variant that asserts exactly one CustomRuntimeException and one success deterministically.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/life/mosu/mosuserver/application/profile/ProfileService.java(2 hunks)src/test/java/life/mosu/mosuserver/application/profile/ProfileServiceConcurrencyTest.java(1 hunks)
🔇 Additional comments (2)
src/main/java/life/mosu/mosuserver/application/profile/ProfileService.java (2)
48-50: 4xx mapping confirmed –ErrorCode.PROFILE_ALREADY_EXISTScarriesHttpStatus.CONFLICTand is returned as a 409 viaGlobalExceptionHandler.handleCustomRuntimeException([src/main/java/life/mosu/mosuserver/global/exception/GlobalExceptionHandler.java#179-185]).
46-46: Event listener runs AFTER_COMMIT; no change needed
ProfileTxEventListener is annotated with @TransactionalEventListener(phase = TransactionPhase.AFTER_COMMIT), ensuring publishSuccessEvent handlers execute only after the transaction commits.
| Exception thrownException = exceptions.getFirst(); | ||
| assertThat(thrownException).isInstanceOf(CustomRuntimeException.class); | ||
| } |
There was a problem hiding this comment.
Avoid Java 21-only List.getFirst(); use get(0) for wider JDK compatibility.
This fails on JDK 17 (common LTS). Replace with get(0).
- Exception thrownException = exceptions.getFirst();
+ Exception thrownException = exceptions.get(0);📝 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.
| Exception thrownException = exceptions.getFirst(); | |
| assertThat(thrownException).isInstanceOf(CustomRuntimeException.class); | |
| } | |
| Exception thrownException = exceptions.get(0); | |
| assertThat(thrownException).isInstanceOf(CustomRuntimeException.class); | |
| } |
🤖 Prompt for AI Agents
In
src/test/java/life/mosu/mosuserver/application/profile/ProfileServiceConcurrencyTest.java
around lines 107 to 109, the test uses List.getFirst() which is Java 21-only;
replace the call with exceptions.get(0) to restore JDK 17 compatibility and keep
the same behavior, i.e., retrieve the first element from the list before
asserting its type.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
build.gradle (3)
75-78: Align Testcontainers versions to avoid classpath conflicts.You’re mixing
1.19.3with1.20.0; this is a common source of NoSuchMethod/Linkage errors at runtime.Update the core and JUnit Jupiter modules to match MySQL
1.20.0:- testImplementation 'org.testcontainers:testcontainers:1.19.3' - testImplementation 'org.testcontainers:junit-jupiter:1.19.3' - testImplementation 'org.testcontainers:mysql:1.20.0' + testImplementation 'org.testcontainers:testcontainers:1.20.0' + testImplementation 'org.testcontainers:junit-jupiter:1.20.0' + testImplementation 'org.testcontainers:mysql:1.20.0'
86-91: AWS SDK: inconsistent versions (BOM 2.20.0 vs core 2.30.20).Mixing BOM-managed and explicitly pinned versions breaks dependency alignment.
Unify on a single version via the BOM and drop the explicit
core:- implementation platform('software.amazon.awssdk:bom:2.20.0') + implementation platform('software.amazon.awssdk:bom:2.30.20') implementation 'software.amazon.awssdk:s3' implementation 'software.amazon.awssdk:sts' - implementation 'software.amazon.awssdk:core:2.30.20' implementation 'software.amazon.awssdk:ec2'
52-54: Replace javax JSTL with the Jakarta (GlassFish) implementationFile: build.gradle (lines 52–54) — change dependency:
- implementation 'javax.servlet:jstl:1.2' + implementation 'org.glassfish.web:jakarta.servlet.jsp.jstl:3.0.1'
| sourceSets { | ||
| test { | ||
| java { | ||
| exclude '**/ProfileServiceConcurrencyTest.java' | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t hide the new concurrency test; make it opt-in/opt-out instead of permanently excluded.
Excluding ProfileServiceConcurrencyTest.java from compilation defeats the PR’s objective and risks regressions. Prefer a property-gated exclude so CI can still run it when desired.
Apply this diff to make the exclusion conditional (run with -PincludeConcurrencyTests=true to include the test):
-sourceSets {
- test {
- java {
- exclude '**/ProfileServiceConcurrencyTest.java'
- }
- }
-}
+sourceSets {
+ test {
+ java {
+ if (!project.hasProperty('includeConcurrencyTests')) {
+ exclude '**/ProfileServiceConcurrencyTest.java'
+ }
+ }
+ }
+}If you want a cleaner separation, move it to an integrationTest source set and wire a dedicated task; I can provide a minimal snippet.
📝 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.
| sourceSets { | |
| test { | |
| java { | |
| exclude '**/ProfileServiceConcurrencyTest.java' | |
| } | |
| } | |
| } | |
| sourceSets { | |
| test { | |
| java { | |
| if (!project.hasProperty('includeConcurrencyTests')) { | |
| exclude '**/ProfileServiceConcurrencyTest.java' | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In build.gradle around lines 17–23 the ProfileServiceConcurrencyTest.java is
permanently excluded which hides the new concurrency test; change the exclusion
to be property-gated so the test is excluded by default but included when the
build is invoked with -PincludeConcurrencyTests=true. Implement a conditional
check on project.hasProperty('includeConcurrencyTests') (or
project.property('includeConcurrencyTests') == 'true') and only add the exclude
when that property is not present/false; alternatively, move the test into an
integrationTest sourceSet and wire an integrationTest task if you prefer
separation.
✨ 구현한 기능
📢 DB 제약 조건의 결정
프로필 생성 시의 동시성 문제를 해결하기 위해 여러 방법을 고려했습니다. 가장 먼저 떠올릴 수 있는 방법은 JPA의
@Lock을 활용한 비관적 락(Pessimistic Lock)입니다. 하지만 저는 데이터베이스의 Unique 제약 조건을 활용해 예외를 처리하는 방식을 최종적으로 선택했습니다.그 이유는 다음과 같습니다.
비관적 락은 '자원 소모가 큰' 접근 방식입니다.
비관적 락은 데이터 충돌이 발생할 것이라 가정하고, 트랜잭션이 시작될 때부터 특정 데이터에 락을 겁니다.
User테이블에 락을 건다면 프로필 생성과 무관한 다른 로직까지 막아버리는 과도한 제어가 될 수 있습니다.DB 제약 조건 활용하기
데이터베이스의 Unique 제약 조건을 활용하는 방식은 다음과 같은 장점을 가집니다.
try-catch로 예외 상황을 처리하기만 하면 되므로 매우 간결하고, 데이터베이스의 동작은 신뢰할 수 있습니다.결론적으로, 불필요한 비관적 락으로 인한 자원 낭비와 성능 저하를 피하고, 데이터베이스 제약조건을 활용하여 효율적이고 안정적으로 동시성 문제를 해결할 수 있다고 판단하였습니다.