MOSU-270 feat: 회원가입 시 전화번호로 중복 사용자 검증#275
Conversation
WalkthroughThe user existence validation logic in both sign-up and OAuth user persistence flows was refactored to use phone number instead of login ID. A new repository method was added for phone number existence checks, and domain-specific exception handling was introduced for invalid gender input. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SignUpAccountStepProcessor
participant UserJpaRepository
Client->>SignUpAccountStepProcessor: process(signUpRequest)
SignUpAccountStepProcessor->>UserJpaRepository: existsByPhoneNumber(phoneNumber)
alt User exists
SignUpAccountStepProcessor-->>Client: throw USER_ALREADY_EXISTS
else User does not exist
SignUpAccountStepProcessor->>UserJpaRepository: save(user)
SignUpAccountStepProcessor-->>Client: return success
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. 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. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @wlgns12370, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a significant change to the user registration and authentication flow. Previously, user uniqueness was determined by loginId. Now, the system will verify duplicate users based on their phoneNumber during the sign-up process and when processing OAuth user information. This enhances the robustness of user identification.
Highlights
- User Registration Logic Update: Modified the SignUpAccountStepProcessor to check for existing users using their phone number instead of their login ID, ensuring unique phone numbers for new registrations.
- OAuth User Persistence Adjustment: Updated the OAuthUserPersistenceProcessor to find existing users by their phone number rather than their email (login ID) when processing OAuth user information.
- Repository Method Addition: Introduced a new existsByPhoneNumber method in UserJpaRepository to support the new phone number-based existence checks.
- Error Handling Refinement: Improved error handling in the Gender entity by replacing IllegalArgumentException with a more specific CustomRuntimeException for invalid gender names.
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
The pull request updates the user registration and OAuth persistence logic to use the phone number for duplicate user verification, which is a good enhancement. However, by removing the existing checks for loginId uniqueness, the changes introduce a risk of unhandled database exceptions if a user tries to register with an existing loginId but a new phone number. I've provided suggestions to re-introduce these checks to prevent potential DataIntegrityViolationException errors and ensure data consistency. The other changes for exception handling and repository methods are well-aligned with the main goal.
| if (userRepository.existsByPhoneNumber(user.getPhoneNumber())) { | ||
| throw new CustomRuntimeException(ErrorCode.USER_ALREADY_EXISTS); | ||
| } |
There was a problem hiding this comment.
While adding the check for phoneNumber is correct, removing the original check for loginId uniqueness can cause problems. The loginId field in UserJpaEntity has a unique constraint. If a user attempts to sign up with an existing loginId but a new phone number, userRepository.save(user) will throw a DataIntegrityViolationException, leading to an unhandled server error.
To prevent this, you should check for the existence of both phoneNumber and loginId before saving the new user.
if (userRepository.existsByPhoneNumber(user.getPhoneNumber())) {
throw new CustomRuntimeException(ErrorCode.USER_ALREADY_EXISTS);
}
if (userRepository.existsByLoginId(user.getLoginId())) {
throw new CustomRuntimeException(ErrorCode.USER_ALREADY_EXISTS);
}| @Transactional | ||
| public UserJpaEntity process(final OAuthUserInfo info) { | ||
| return userRepository.findByLoginId(info.email()) | ||
| return userRepository.findByPhoneNumber(info.phoneNumber()) |
There was a problem hiding this comment.
Changing the user lookup from loginId to phoneNumber is a good move, but it introduces a potential issue. In the orElseGet block, a new user is created if no existing user is found by phone number. However, the loginId for this new user is set to info.email(), and loginId has a unique constraint.
If another user already exists with the same email (loginId) but a different phone number, the userRepository.save(newUser) call will fail with a DataIntegrityViolationException.
You should handle this scenario by checking if a user with the given email already exists before attempting to create a new one inside the orElseGet block.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/main/java/life/mosu/mosuserver/application/auth/processor/SignUpAccountStepProcessor.java (1)
20-25: Enforce DB uniqueness on phone numbers and handle race conditionsThe current existsByPhoneNumber check in SignUpAccountStepProcessor is subject to a TOCTOU race under concurrency. We need to:
- Validate and normalize the incoming phone number.
- Rely on a database‐enforced unique constraint (instead of a pre‐check).
- Catch and map the resulting constraint violation to our domain error.
Locations to address:
UserJpaEntity.java
Add a unique constraint on phone_number:@Column(name = "phone_number") -private String phoneNumber; +@Column(name = "phone_number", unique = true) +private String phoneNumber;DB migrations
Create or update a migration under src/main/resources/db to add a unique index/constraint on phone_number.SignUpAccountStepProcessor.java
Replace the pre‐check with input validation + save + exception handling:@Transactional @Override public UserJpaEntity process(UserJpaEntity user) { - if (userRepository.existsByPhoneNumber(user.getPhoneNumber())) { - throw new CustomRuntimeException(ErrorCode.USER_ALREADY_EXISTS); - } - return userRepository.save(user); + String phone = user.getPhoneNumber(); + if (phone == null || phone.isBlank()) { + throw new CustomRuntimeException(ErrorCode.INVALID_PHONE_NUMBER); + } + // Ensure phone is normalized (e.g. to E.164) before calling this method. + try { + return userRepository.save(user); + } catch (DataIntegrityViolationException ex) { + // Unique‐constraint violation on phone_number + throw new CustomRuntimeException(ErrorCode.USER_ALREADY_EXISTS); + } }Follow-up:
- Centralize phone‐number normalization (e.g. trim, remove non‐digits, format to E.164) before persistence.
- Verify that both the JPA annotation and your DB schema/migration align on enforcing uniqueness.
🧹 Nitpick comments (1)
src/main/java/life/mosu/mosuserver/domain/profile/entity/Gender.java (1)
4-6: Domain-specific exception for invalid gender: LGTMGood move to standardize on
CustomRuntimeException(ErrorCode.INVALID_GENDER).If this method is hot, consider a precomputed lookup map to avoid the stream per call:
private static final Map<String, Gender> BY_NAME = Arrays.stream(values()) .collect(Collectors.toUnmodifiableMap(Gender::getGenderName, Function.identity())); public static Gender fromName(String genderName) { final Gender g = BY_NAME.get(genderName); if (g == null) throw new CustomRuntimeException(ErrorCode.INVALID_GENDER); return g; }Optionally normalize input (trim) if user input can contain incidental whitespace.
Also applies to: 23-23
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/life/mosu/mosuserver/application/auth/processor/SignUpAccountStepProcessor.java(1 hunks)src/main/java/life/mosu/mosuserver/application/oauth/OAuthUserPersistenceProcessor.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/profile/entity/Gender.java(2 hunks)src/main/java/life/mosu/mosuserver/domain/user/repository/UserJpaRepository.java(1 hunks)
| @Transactional | ||
| public UserJpaEntity process(final OAuthUserInfo info) { | ||
| return userRepository.findByLoginId(info.email()) | ||
| return userRepository.findByPhoneNumber(info.phoneNumber()) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Guard null/blank phone; fallback to a safe identifier to prevent duplicates
OAuth providers may not return phone numbers for all users. Querying by a null/blank phone risks false negatives or unintended matches, and can create duplicates on save. At minimum, fallback to loginId (email) when phone is absent; ideally, prefer a stable provider identifier if available.
Apply this diff within the method to guard phone and fallback:
- public UserJpaEntity process(final OAuthUserInfo info) {
- return userRepository.findByPhoneNumber(info.phoneNumber())
+ public UserJpaEntity process(final OAuthUserInfo info) {
+ final String phone = info.phoneNumber();
+ final java.util.Optional<UserJpaEntity> existing =
+ (phone != null && !phone.isBlank())
+ ? userRepository.findByPhoneNumber(phone)
+ : userRepository.findByLoginId(info.email());
+ return existing
.map(existingUser -> {Additionally, verify whether loginId (email) is unique at DB level. If it is, save(newUser) can fail if another account already uses the same email with a different phone; consider catching DataIntegrityViolationException and mapping to an appropriate domain error.
To confirm assumptions about OAuthUserInfo and DB constraints:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate OAuthUserInfo and check phoneNumber contract"
fd -uu -t f 'OAuthUserInfo.java' | while read -r f; do
echo "==> $f"
rg -n -A3 -B3 'phoneNumber\(\)|String\s+phoneNumber' "$f" || true
rg -n '@NotNull|@NotBlank' "$f" || true
done
echo
echo "Check entity mapping/migrations for loginId uniqueness"
fd -uu -t f 'UserJpaEntity.java' | while read -r f; do
echo "==> $f"
rg -n -A2 -B3 'loginId' "$f" || true
rg -n '@Column\([^)]*unique\s*=\s*true' "$f" || true
rg -n '@Table\([^)]*uniqueConstraints' "$f" || true
done
rg -n -S '(unique|uniq).*login|create\s+unique\s+index.*login|uk_.*login|uk_.*login_id' || trueLength of output: 2533
Guard null/blank phone before lookup and fallback to loginId in process()
OAuthUserInfo.phoneNumber is nullable (no @NotNull/@notblank) and may be blank, so querying by it can return unexpected results or allow duplicate accounts. Meanwhile, loginId (email) is annotated with @Column(unique = true), so it’s safe to use as a fallback—but you must handle potential DataIntegrityViolationException when saving a new user.
Changes needed:
• In src/main/java/life/mosu/mosuserver/application/oauth/OAuthUserPersistenceProcessor.java, update process() to:
public UserJpaEntity process(final OAuthUserInfo info) {
- return userRepository.findByPhoneNumber(info.phoneNumber())
+ final String phone = info.phoneNumber();
+ final Optional<UserJpaEntity> existing =
+ (phone != null && !phone.isBlank())
+ ? userRepository.findByPhoneNumber(phone)
+ : userRepository.findByLoginId(info.email());
+ return existing
.map(existingUser -> {
// …• After invoking save(newUser), catch and translate DataIntegrityViolationException into a meaningful domain error (e.g., “email already in use”), since login_id is unique at the DB level (see UserJpaEntity.loginId).
Fix these spots to prevent silent duplicates and surface unique‐constraint failures.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return userRepository.findByPhoneNumber(info.phoneNumber()) | |
| public UserJpaEntity process(final OAuthUserInfo info) { | |
| + final String phone = info.phoneNumber(); | |
| + final Optional<UserJpaEntity> existing = | |
| + (phone != null && !phone.isBlank()) | |
| + ? userRepository.findByPhoneNumber(phone) | |
| + : userRepository.findByLoginId(info.email()); | |
| return existing | |
| .map(existingUser -> { | |
| // … |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/oauth/OAuthUserPersistenceProcessor.java
around line 21, the code currently queries userRepository.findByPhoneNumber
without checking if the phone number is null or blank, which can cause
unexpected results or duplicates. Update the process() method to first check if
phoneNumber is non-null and non-blank before querying by it; if it is null or
blank, fallback to querying by loginId (email). Additionally, after saving a new
user, catch DataIntegrityViolationException and translate it into a clear
domain-specific error indicating the email is already in use to handle the
unique constraint on loginId properly.
|
|
||
| boolean existsByPhoneNumber(String phoneNumber); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Back this with a DB-level unique index and normalization
existsByPhoneNumber is fine, but without a unique constraint/index on the phone_number column, concurrent signups can still insert duplicates after passing the exists check, and queries will be slower at scale. Normalize phone numbers (e.g., E.164, strip spaces/hyphens) before persistence and queries.
- Add a unique constraint on
phone_number(and ensure an index). - Ensure a single normalization path is applied before calling repository methods.
Run this to verify entity/migration uniqueness:
If missing, consider (outside this diff) annotating the entity:
// in UserJpaEntity.java
@Table(uniqueConstraints = @UniqueConstraint(name = "uk_user_phone_number", columnNames = "phone_number"))
public class UserJpaEntity {
@Column(name = "phone_number", nullable = false, unique = true)
private String phoneNumber;
}🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Check UserJpaEntity mapping for phoneNumber uniqueness"
fd -uu -t f 'UserJpaEntity.java' | while read -r f; do
echo "==> $f"
rg -n -A3 -B3 'phoneNumber' "$f" || true
rg -n '@Column\([^)]*unique\s*=\s*true' "$f" || true
rg -n '@Table\([^)]*uniqueConstraints' "$f" || true
done
echo
echo "Search for DB migrations adding a unique index/constraint on phone"
rg -n -S '(unique|uniq).*phone|create\s+unique\s+index.*phone|uk_.*phone|uniqueConstraints' || trueLength of output: 2755
Add DB-level unique constraint on phone_number and enforce normalization
The existsByPhoneNumber check is OK, but without a unique index/constraint on phone_number and consistent normalization, you can still get duplicates under concurrency and face full-table scans at scale.
Please address the following:
• In src/main/java/life/mosu/mosuserver/domain/user/entity/UserJpaEntity.java
- Add a table-level unique constraint on
phone_number - Mark the
phoneNumbercolumn asunique = trueandnullable = false
@Table(
- name = "user"
+ name = "user",
+ uniqueConstraints = @UniqueConstraint(
+ name = "uk_user_phone_number",
+ columnNames = "phone_number"
+ )
)
public class UserJpaEntity {
@@
- @Column(name = "phone_number")
+ @Column(name = "phone_number", nullable = false, unique = true)
private String phoneNumber;• In your DB migration (e.g. under src/main/resources/db/migration):
- Create a migration that adds a unique index on
phone_number
CREATE UNIQUE INDEX uk_user_phone_number
ON user (phone_number);• Normalization
- Ensure every phone number is normalized (e.g. stripped of spaces/hyphens and formatted to E.164) before any repository call—both reads (
existsByPhoneNumber) and writes.
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/domain/user/entity/UserJpaEntity.java
around line 16-17, add a table-level unique constraint on the phone_number
column, and mark the phoneNumber field as unique=true and nullable=false. Then,
create a new DB migration script under src/main/resources/db/migration that adds
a unique index on phone_number using the provided SQL statement. Finally,
implement normalization logic to format phone numbers to E.164 and strip
spaces/hyphens before any repository calls, including existsByPhoneNumber and
save operations, to ensure consistency and prevent duplicates.
1 participant
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit
Bug Fixes
New Features