fix: provider_id가 null인 컬럼에 대해서도 데이터 이관 처리#362
Conversation
📝 WalkthroughWalkthroughuser_oauth_account 테이블 마이그레이션에서 Changes
Sequence Diagram(s)(생성 조건 미충족 — 생략) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10분 Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/resources/db/migration/V43__add_user_oauth_account_table.sql (2)
30-35:⚠️ Potential issue | 🔴 Critical[LEVEL: Critical] NULL 비교 로직 오류로 중복 삽입 또는 제약조건 위반 가능
ua.provider_id = u.provider_id조건은provider_id가 NULL인 경우 정상 동작하지 않습니다. SQL에서NULL = NULL은 NULL을 반환하며 TRUE가 아니므로, NOT EXISTS 절이 기존 NULL provider_id 행을 감지하지 못합니다.결과적으로:
- 동일한
(provider, NULL)조합이 이미 존재해도 중복 삽입 시도- UNIQUE 제약조건 위반 또는 의도치 않은 중복 데이터 발생 가능
🐛 NULL-safe 비교를 위한 수정 제안
AND NOT EXISTS ( SELECT 1 FROM user_oauth_account ua WHERE ua.provider = u.provider - AND ua.provider_id = u.provider_id + AND (ua.provider_id = u.provider_id OR (ua.provider_id IS NULL AND u.provider_id IS NULL)) );또는 MySQL의 NULL-safe 비교 연산자 사용:
AND NOT EXISTS ( SELECT 1 FROM user_oauth_account ua WHERE ua.provider = u.provider - AND ua.provider_id = u.provider_id + AND ua.provider_id <=> u.provider_id );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/db/migration/V43__add_user_oauth_account_table.sql` around lines 30 - 35, The NOT EXISTS check currently uses "ua.provider_id = u.provider_id" which fails for NULLs; update the NULL comparison to a NULL-safe form so existing rows with provider_id IS NULL are detected—for Postgres replace "ua.provider_id = u.provider_id" with "ua.provider_id IS NOT DISTINCT FROM u.provider_id" (or for MySQL use the NULL-safe operator "ua.provider_id <=> u.provider_id"); keep the rest of the NOT EXISTS clause and aliases (user_oauth_account as ua and the source alias u) unchanged.
13-14:⚠️ Potential issue | 🟠 Major[LEVEL: Major] UNIQUE 제약조건에 nullable 컬럼 포함 확인
UNIQUE (provider, provider_id)제약조건에서provider_id컬럼이 NULL을 허용합니다(line 5:VARCHAR(255) NULL). MySQL에서는 UNIQUE 인덱스 내 NULL 값이 서로 다른 것으로 취급되어, 동일한provider에 여러 개의 NULLprovider_id행이 허용됩니다.현재 설계에서 이것이 의도된 동작인지 확인이 필요합니다. 만약 provider별로 NULL provider_id를 정확히 하나만 허용해야 한다면 다음 중 조치가 필요합니다:
- provider_id를 NOT NULL로 변경
- 제약조건 정의 또는 데이터 검증 로직 수정
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/db/migration/V43__add_user_oauth_account_table.sql` around lines 13 - 14, The UNIQUE constraint uq_user_oauth_account_provider_provider_id currently includes provider_id which is defined as nullable, causing MySQL to allow multiple rows with the same provider and NULL provider_id; to fix, decide intended behavior and either alter the column provider_id to NOT NULL or change the uniqueness strategy (e.g., remove provider_id from uq_user_oauth_account_provider_provider_id and add a different constraint or implement application-level validation) and update the migration V43__add_user_oauth_account_table.sql accordingly to reflect the chosen approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/resources/db/migration/V43__add_user_oauth_account_table.sql`:
- Line 5: The NOT EXISTS check in V43__add_user_oauth_account_table.sql
incorrectly compares ua.provider_id = u.provider_id which fails when
u.provider_id IS NULL, allowing duplicate rows; update the WHERE clause used in
the INSERT ... SELECT NOT EXISTS subquery (the condition referencing
ua.provider, ua.provider_id, u.provider and u.provider_id) to treat NULLs as
equal by replacing the simple equality with a compound check: (u.provider_id IS
NULL AND ua.provider_id IS NULL) OR ua.provider_id = u.provider_id so NULL
provider_id values are considered matching.
---
Outside diff comments:
In `@src/main/resources/db/migration/V43__add_user_oauth_account_table.sql`:
- Around line 30-35: The NOT EXISTS check currently uses "ua.provider_id =
u.provider_id" which fails for NULLs; update the NULL comparison to a NULL-safe
form so existing rows with provider_id IS NULL are detected—for Postgres replace
"ua.provider_id = u.provider_id" with "ua.provider_id IS NOT DISTINCT FROM
u.provider_id" (or for MySQL use the NULL-safe operator "ua.provider_id <=>
u.provider_id"); keep the rest of the NOT EXISTS clause and aliases
(user_oauth_account as ua and the source alias u) unchanged.
- Around line 13-14: The UNIQUE constraint
uq_user_oauth_account_provider_provider_id currently includes provider_id which
is defined as nullable, causing MySQL to allow multiple rows with the same
provider and NULL provider_id; to fix, decide intended behavior and either alter
the column provider_id to NOT NULL or change the uniqueness strategy (e.g.,
remove provider_id from uq_user_oauth_account_provider_provider_id and add a
different constraint or implement application-level validation) and update the
migration V43__add_user_oauth_account_table.sql accordingly to reflect the
chosen approach.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 46d53daf-4335-4a0f-994d-6cbcfc951ed1
📒 Files selected for processing (1)
src/main/resources/db/migration/V43__add_user_oauth_account_table.sql
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
src/main/resources/db/migration/**/*.sql
⚙️ CodeRabbit configuration file
src/main/resources/db/migration/**/*.sql: Flyway 마이그레이션 리뷰 규칙:
- 버전 파일명 규칙(V{number}__{description}.sql) 위반 여부를 우선 확인한다.
- 이미 배포된 마이그레이션 수정/재번호 부여 위험이 있으면 반드시 차단 코멘트를 남긴다.
- 파괴적 변경(drop, rename 등)은 롤백 가능성과 운영 영향 관점에서 검토한다.
Files:
src/main/resources/db/migration/V43__add_user_oauth_account_table.sql
**/*
⚙️ CodeRabbit configuration file
**/*: 공통 리뷰 톤 가이드:
- 모든 코멘트는 첫 줄에
[LEVEL: ...]태그를 포함한다.- 과장된 표현 없이 사실 기반으로 작성한다.
- 한 코멘트에는 하나의 이슈만 다룬다.
- 코드 예시가 필요하면 최소 수정 예시를 제시한다.
- 가독성/단순화/확장성 이슈를 발견하면 우선순위를 높여 코멘트한다.
Files:
src/main/resources/db/migration/V43__add_user_oauth_account_table.sql
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/resources/db/migration/V43__add_user_oauth_account_table.sql`:
- Around line 27-29: The WHERE clause in the migration now includes users with
provider IS NOT NULL but allows provider_id IS NULL; before running
V43__add_user_oauth_account_table.sql, verify the number of rows with deleted_at
IS NULL, provider IS NOT NULL and provider_id IS NULL in the users table to
ensure this change is intentional and results match expectations, and confirm
the NOT EXISTS clause that references provider and provider_id uses a NULL-safe
comparison to avoid mismatches when provider_id is NULL.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ffed152d-f92a-421f-8f08-80b6864e21dd
📒 Files selected for processing (1)
src/main/resources/db/migration/V43__add_user_oauth_account_table.sql
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
src/main/resources/db/migration/**/*.sql
⚙️ CodeRabbit configuration file
src/main/resources/db/migration/**/*.sql: Flyway 마이그레이션 리뷰 규칙:
- 버전 파일명 규칙(V{number}__{description}.sql) 위반 여부를 우선 확인한다.
- 이미 배포된 마이그레이션 수정/재번호 부여 위험이 있으면 반드시 차단 코멘트를 남긴다.
- 파괴적 변경(drop, rename 등)은 롤백 가능성과 운영 영향 관점에서 검토한다.
Files:
src/main/resources/db/migration/V43__add_user_oauth_account_table.sql
**/*
⚙️ CodeRabbit configuration file
**/*: 공통 리뷰 톤 가이드:
- 모든 코멘트는 첫 줄에
[LEVEL: ...]태그를 포함한다.- 과장된 표현 없이 사실 기반으로 작성한다.
- 한 코멘트에는 하나의 이슈만 다룬다.
- 코드 예시가 필요하면 최소 수정 예시를 제시한다.
- 가독성/단순화/확장성 이슈를 발견하면 우선순위를 높여 코멘트한다.
Files:
src/main/resources/db/migration/V43__add_user_oauth_account_table.sql
🔇 Additional comments (2)
src/main/resources/db/migration/V43__add_user_oauth_account_table.sql (2)
34-37: [LEVEL: Approve] NULL 비교 로직이 올바르게 수정됨SQL에서
NULL = NULL은 false를 반환하는 문제를 올바르게 처리했습니다. 수정된 패턴:(u.provider_id IS NULL AND ua.provider_id IS NULL) OR ua.provider_id = u.provider_id이 로직은 NULL provider_id를 가진 행들도 NOT EXISTS 검사에서 정확히 매칭되어 중복 삽입을 방지합니다.
5-5: [LEVEL: Resolve] NULL provider_id 설계는 적절하게 구현됨provider_id가 nullable인 설계는 의도적이며 안전하게 구현되어 있습니다.
서비스 레이어에서
StringUtils.hasText(providerId)가드(99, 108줄)로 NULL 값을 명시적으로 처리하며, provider_id가 없을 때는 email 기반 매칭으로 대체합니다(111-113줄). 핵심 lookup은findByUserIdAndProvider()를 사용(115줄)하므로 provider_id의 NULL 여부와 무관하게 동작합니다.
(user_id, provider)제약조건이 사용자당 provider별로 1개 계정만 허용하므로 데이터 중복도 방지됩니다. 마이그레이션의 NULL 안전 비교(34-37줄)도 올바릅니다.
| FROM users u | ||
| WHERE u.deleted_at IS NULL | ||
| AND u.provider IS NOT NULL |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
[LEVEL: Info] WHERE 조건 변경 확인
u.provider_id IS NOT NULL 필터가 제거되어 NULL provider_id를 가진 사용자도 마이그레이션 대상에 포함됩니다. 이는 의도된 변경으로 보이며, NOT EXISTS 절의 NULL-safe 비교와 함께 올바르게 작동합니다.
다만, 마이그레이션 실행 전 NULL provider_id를 가진 행의 수를 확인하여 예상 결과와 일치하는지 검증하는 것을 권장합니다:
-- 실행 전 확인용 쿼리
SELECT COUNT(*) FROM users WHERE deleted_at IS NULL AND provider IS NOT NULL AND provider_id IS NULL;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/db/migration/V43__add_user_oauth_account_table.sql` around
lines 27 - 29, The WHERE clause in the migration now includes users with
provider IS NOT NULL but allows provider_id IS NULL; before running
V43__add_user_oauth_account_table.sql, verify the number of rows with deleted_at
IS NULL, provider IS NOT NULL and provider_id IS NULL in the users table to
ensure this change is intentional and results match expectations, and confirm
the NOT EXISTS clause that references provider and provider_id uses a NULL-safe
comparison to avoid mismatches when provider_id is NULL.
1 participant
🔍 개요
🚀 주요 변경 내용
💬 참고 사항
✅ Checklist (완료 조건)