refactor: 한글 닉네임 및 대소문자 무시 정책 적용 - #301
Merged
Merged
Conversation
10 tasks
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThe PR adds shared nickname normalization and validation, stores unique NFC-normalized comparison keys, and updates member and authentication lookups to ignore case. It also adds migrations and tests for Unicode normalization, validation, duplicates, lookup, and login behavior. ChangesNickname identity
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Client
participant MemberCommandService
participant MemberRepository
participant AuthUser
Client->>MemberCommandService: submit nickname update
MemberCommandService->>MemberRepository: check and flush nickNameKey
MemberCommandService->>AuthUser: synchronize normalized nickname
sequenceDiagram
participant Client
participant CustomUserDetailsService
participant AuthRepository
Client->>CustomUserDetailsService: submit login identifier
CustomUserDetailsService->>AuthRepository: findByEmailIgnoreCase
CustomUserDetailsService->>AuthRepository: findByNickNameKey
AuthRepository-->>CustomUserDetailsService: return matching AuthUser
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/checkmo/member/internal/service/command/MemberCommandService.java`:
- Around line 150-155: Remove the DataIntegrityViolationException catch from
flushNicknameChange() and let memberRepository.flush() propagate the failure
through the transactional service method. Add or update an outer exception
handler to map only the nickname uniqueness constraint to
NICKNAME_ALREADY_EXISTS, while preserving other integrity violations unchanged.
In `@src/main/resources/db/migration/V20260808_1__add_member_nickname_key.sql`:
- Around line 4-9: Update the migration backfill in
src/main/resources/db/migration/V20260808_1__add_member_nickname_key.sql lines
4-9 and
src/main/resources/db/migration/V20260808_2__add_auth_user_nickname_key.sql
lines 4-9 to derive nick_name_key using the same NFC normalization followed by
lowercase behavior as NicknamePolicy.comparisonKey, then deterministically
resolve any canonical-key collisions before adding each unique constraint.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ac4349a0-9c6f-40e4-8017-e0540ea78d29
📒 Files selected for processing (20)
src/main/java/checkmo/authentication/internal/entity/AuthUser.javasrc/main/java/checkmo/authentication/internal/repository/AuthRepository.javasrc/main/java/checkmo/authentication/internal/security/auth/CustomUserDetailsService.javasrc/main/java/checkmo/common/validation/NicknamePolicy.javasrc/main/java/checkmo/common/validation/NicknameValidator.javasrc/main/java/checkmo/common/validation/ValidNickname.javasrc/main/java/checkmo/member/internal/entity/Member.javasrc/main/java/checkmo/member/internal/repository/MemberRepository.javasrc/main/java/checkmo/member/internal/service/command/MemberBlockCommandService.javasrc/main/java/checkmo/member/internal/service/command/MemberCommandService.javasrc/main/java/checkmo/member/internal/service/command/MemberFollowCommandService.javasrc/main/java/checkmo/member/internal/service/query/MemberQueryService.javasrc/main/java/checkmo/member/web/controller/MemberController.javasrc/main/java/checkmo/member/web/dto/MemberRequestDTO.javasrc/main/resources/db/migration/V20260808_1__add_member_nickname_key.sqlsrc/main/resources/db/migration/V20260808_2__add_auth_user_nickname_key.sqlsrc/test/java/checkmo/authentication/AuthApiTest.javasrc/test/java/checkmo/common/validation/NicknamePolicyTest.javasrc/test/java/checkmo/member/MemberApiTest.javasrc/test/java/checkmo/member/PublicMemberApiTest.java
Comment on lines
+150
to
+155
| private void flushNicknameChange() { | ||
| try { | ||
| memberRepository.flush(); | ||
| } catch (DataIntegrityViolationException exception) { | ||
| throw new MemberException(MemberErrorStatus.NICKNAME_ALREADY_EXISTS); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect transaction boundaries and existing persistence-exception mapping.
rg -n -C 6 '`@Transactional`|DataIntegrityViolationException|`@ControllerAdvice`|`@RestControllerAdvice`|`@ExceptionHandler`' \
src/main/javaRepository: checkmo2025/BE
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Narrow inspection to the member command service and persistence exception handling.
wc -l src/main/java/checkmo/member/internal/service/command/MemberCommandService.java
sed -n '1,230p' src/main/java/checkmo/member/internal/service/command/MemberCommandService.java | nl -ba
sed -n '1,120p' src/main/java/checkmo/common/apiPayload/exception/ExceptionAdvice.java | nl -ba
sed -n '1,80p' src/main/java/checkmo/common/apiPayload/exception/handler/GeneralExceptionHandler.java | nl -baRepository: checkmo2025/BE
Length of output: 276
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Narrow inspection to the member command service and persistence exception handling.
echo "== MemberCommandService line count =="
wc -l src/main/java/checkmo/member/internal/service/command/MemberCommandService.java
echo "== MemberCommandService relevant sections =="
awk '{printf "%6d %s\n", NR, $0}' src/main/java/checkmo/member/internal/service/command/MemberCommandService.java | sed -n '1,230p'
echo "== ExceptionAdvice relevant sections =="
awk '{printf "%6d %s\n", NR, $0}' src/main/java/checkmo/common/apiPayload/exception/ExceptionAdvice.java | sed -n '1,120p'
echo "== GeneralExceptionHandler relevant sections =="
awk '{printf "%6d %s\n", NR, $0}' src/main/java/checkmo/common/apiPayload/exception/handler/GeneralExceptionHandler.java | sed -n '1,80p'Repository: checkmo2025/BE
Length of output: 19566
Do not catch DataIntegrityViolationException inside the service transaction.
flushNicknameChange() runs inside the class-level @Transactional service method, so a uniqueness failure during memberRepository.flush() can make the transaction rollback-only or leave the commit failing with UnexpectedRollbackException. Also, this catches every integrity failure as NICKNAME_ALREADY_EXISTS. Let the exception leave the transaction and map only the nickname-key constraint in an outer exception handler.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/java/checkmo/member/internal/service/command/MemberCommandService.java`
around lines 150 - 155, Remove the DataIntegrityViolationException catch from
flushNicknameChange() and let memberRepository.flush() propagate the failure
through the transactional service method. Add or update an outer exception
handler to map only the nickname uniqueness constraint to
NICKNAME_ALREADY_EXISTS, while preserving other integrity violations unchanged.
Comment on lines
+4
to
+9
| UPDATE member | ||
| SET nick_name_key = LOWER(nick_name) | ||
| WHERE nick_name IS NOT NULL; | ||
|
|
||
| ALTER TABLE member | ||
| ADD CONSTRAINT UK_member_nickname_key UNIQUE (nick_name_key); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Backfill keys with the runtime NFC normalization policy.
LOWER(nick_name) does not apply NFC normalization. Existing decomposed nicknames get a different key from the NFC value generated for new writes. This can allow equivalent nicknames to coexist and can make normalized lookup fail.
src/main/resources/db/migration/V20260808_1__add_member_nickname_key.sql#L4-L9: generatenick_name_keywith the same NFC-plus-lowercase logic asNicknamePolicy.comparisonKey, then resolve canonical-key collisions before adding the constraint.src/main/resources/db/migration/V20260808_2__add_auth_user_nickname_key.sql#L4-L9: generatenick_name_keywith the same NFC-plus-lowercase logic asNicknamePolicy.comparisonKey, then resolve canonical-key collisions before adding the constraint.
🧰 Tools
🪛 Squawk (2.61.0)
[warning] 9-9: By default new constraints require a table scan and block writes to the table while that scan occurs. Use NOT VALID with a later VALIDATE CONSTRAINT call.
(constraint-missing-not-valid)
[warning] 9-9: Adding a UNIQUE constraint requires an ACCESS EXCLUSIVE lock which blocks reads and writes to the table while the index is built. Create an index CONCURRENTLY and create the constraint using the index.
(disallowed-unique-constraint)
📍 Affects 2 files
src/main/resources/db/migration/V20260808_1__add_member_nickname_key.sql#L4-L9(this comment)src/main/resources/db/migration/V20260808_2__add_auth_user_nickname_key.sql#L4-L9
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/resources/db/migration/V20260808_1__add_member_nickname_key.sql`
around lines 4 - 9, Update the migration backfill in
src/main/resources/db/migration/V20260808_1__add_member_nickname_key.sql lines
4-9 and
src/main/resources/db/migration/V20260808_2__add_auth_user_nickname_key.sql
lines 4-9 to derive nick_name_key using the same NFC normalization followed by
lowercase behavior as NicknamePolicy.comparisonKey, then deterministically
resolve any canonical-key collisions before adding each unique constraint.
공통에는 인증·회원이 함께 쓰는 닉네임 정책만 두고, Jakarta 제약조건과 Validator는 회원 모듈 내부로 이동한다. 정책 테스트와 Validator 테스트도 패키지 책임에 맞게 분리한다. Tested: ./gradlew compileJava; 닉네임 정책·Validator 테스트; AuthApiTest·MemberApiTest·PublicMemberApiTest; CheckmoApplicationTests Not-tested: 전체 테스트는 기존 Apple JWT 만료 3건과 Sentry workflow 1건 실패
There was a problem hiding this comment.
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/checkmo/member/internal/entity/Member.java (1)
64-66: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winStore
nullfor empty optional nicknames.
NicknamePolicy.validate("", false)accepts an empty nickname, andNicknamePolicy.isSameIdentitytreats an empty key as no identity. These paths currently persistNicknamePolicy.comparisonKey("")as""in the uniquenickNameKeycolumn. A second optional empty nickname, or multiple legacy empty rows during backfill, can violate the unique constraint.Store
nullfor an empty normalized nickname in bothupdateNicknameandsynchronizeNicknameIdentity. Apply the same rule to the parallelAuthUserpersistence path and add a persistence regression test.Proposed fix
public void updateNickname(String nickName) { - this.nickName = NicknamePolicy.normalize(nickName); - this.nickNameKey = NicknamePolicy.comparisonKey(nickName); + String normalizedNickname = NicknamePolicy.normalize(nickName); + this.nickName = normalizedNickname; + this.nickNameKey = normalizedNickname == null || normalizedNickname.isEmpty() + ? null + : NicknamePolicy.comparisonKey(normalizedNickname); }Also applies to: 97-106, 112-119
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/checkmo/member/internal/entity/Member.java` around lines 64 - 66, Normalize empty optional nicknames to null rather than persisting an empty key: update both Member.updateNickname and Member.synchronizeNicknameIdentity, and apply the equivalent change in the parallel AuthUser persistence path. Preserve non-empty comparison keys and add a persistence regression test covering multiple optional empty nicknames and legacy backfill rows.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/main/java/checkmo/member/internal/entity/Member.java`:
- Around line 64-66: Normalize empty optional nicknames to null rather than
persisting an empty key: update both Member.updateNickname and
Member.synchronizeNicknameIdentity, and apply the equivalent change in the
parallel AuthUser persistence path. Preserve non-empty comparison keys and add a
persistence regression test covering multiple optional empty nicknames and
legacy backfill rows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 95072d62-76b6-496c-9f03-c9c69822d8bc
📒 Files selected for processing (14)
src/main/java/checkmo/authentication/internal/entity/AuthUser.javasrc/main/java/checkmo/authentication/internal/security/auth/CustomUserDetailsService.javasrc/main/java/checkmo/common/nickname/NicknamePolicy.javasrc/main/java/checkmo/member/internal/entity/Member.javasrc/main/java/checkmo/member/internal/service/command/MemberBlockCommandService.javasrc/main/java/checkmo/member/internal/service/command/MemberCommandService.javasrc/main/java/checkmo/member/internal/service/command/MemberFollowCommandService.javasrc/main/java/checkmo/member/internal/service/query/MemberQueryService.javasrc/main/java/checkmo/member/internal/validation/nickname/NicknameValidator.javasrc/main/java/checkmo/member/internal/validation/nickname/ValidNickname.javasrc/main/java/checkmo/member/web/controller/MemberController.javasrc/main/java/checkmo/member/web/dto/MemberRequestDTO.javasrc/test/java/checkmo/common/nickname/NicknamePolicyTest.javasrc/test/java/checkmo/member/internal/validation/nickname/NicknameValidatorTest.java
🚧 Files skipped from review as they are similar to previous changes (8)
- src/main/java/checkmo/member/web/controller/MemberController.java
- src/main/java/checkmo/member/internal/service/command/MemberBlockCommandService.java
- src/main/java/checkmo/member/web/dto/MemberRequestDTO.java
- src/main/java/checkmo/authentication/internal/entity/AuthUser.java
- src/main/java/checkmo/authentication/internal/security/auth/CustomUserDetailsService.java
- src/main/java/checkmo/member/internal/service/command/MemberCommandService.java
- src/main/java/checkmo/member/internal/service/command/MemberFollowCommandService.java
- src/main/java/checkmo/member/internal/service/query/MemberQueryService.java
닉네임 저장 시 NFC 정규화와 검증을 적용하고 비교 키 및 DB 유일 제약의 동작을 일치시킨다. 기존 데이터 충돌을 마이그레이션 전에 감지하며 닉네임 제약 위반만 MEMBER_416으로 변환한다.\n\nConstraint: 표시 대소문자는 보존하고 식별 비교만 대소문자를 무시\nConfidence: high\nScope-risk: moderate\nTested: 관련 닉네임·회원·인증·모듈 테스트 99개 통과\nNot-tested: MySQL 실DB Flyway 실행; 전체 테스트의 기존 Apple JWT 3건 및 Sentry 1건 실패
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🚀 변경사항
~, 이모지 및 기타 문자 금지Member,AuthUser에 대소문자 무시 비교용nicknameKey추가BookMo에서bookMo로 변경하는 경우 표시값 변경 허용🔗 관련 이슈
✅ 체크리스트
📝 특이사항
nick_name에는 화면에 표시할 원본 닉네임을 저장합니다.nick_name_key에는 NFC 정규화 후 소문자로 변환한 비교값을 저장합니다.LOWER(nick_name)을 적용합니다.Summary by CodeRabbit