Skip to content

Conversation

@clxxrlove
Copy link
Member

@clxxrlove clxxrlove commented Nov 8, 2025

🔗 관련 이슈

  • Close #

📘 작업 유형

  • ✨ Feature (기능 추가)
  • 🐞 Bugfix (버그 수정)
  • 🔧 Refactor (코드 리팩토링)
  • ⚙️ Chore (환경 설정)
  • 📝 Docs (문서 작성 및 수정)
  • ✅ Test (기능 테스트)
  • 🎨 style (코드 스타일 수정)

📙 작업 내역

  • deviceId 생성 로직 구현
    • 실제 deviceId는 제공이 불가능, Keychain에 UUID를 저장하여 앱 재설치 후에도 계속 사용할 수 있게 함

🧪 테스트 내역

  • 브라우저/기기에서 동작 확인
  • 엣지 케이스 테스트 완료
  • 기존 기능 영향 없음

💬 추가 설명 or 리뷰 포인트 (선택)

  • ..

Summary by CodeRabbit

새로운 기능

  • 앱 실행 시 장치 ID를 자동으로 생성하고 안전하게 저장하여 기기를 고유하게 식별합니다.
  • 푸시 알림 토큰 등록 시 장치 ID를 포함하여 알림 전달 추적을 더욱 정확하게 개선합니다.

@coderabbitai
Copy link

coderabbitai bot commented Nov 8, 2025

개요

기기 ID 관리 시스템을 구현하여 Keychain 저장소에 기기 식별자를 생성하고 유지하며, FCM 토큰 업로드 시 기기 ID를 포함하도록 변경하고 의존성 주입을 통해 통합했습니다.

변경 사항

코호트 / 파일 변경 요약
프로토콜 및 인터페이스 정의
src/Projects/BKData/Sources/Interface/Storage/DeviceIDProvider.swift, src/Projects/BKData/Sources/Interface/Storage/DeviceIDStore.swift
DeviceIDProvider 프로토콜(deviceID 속성, clearCache() 메서드)과 DeviceIDStore 프로토콜(getOrCreate(), clear() 퍼블리셔 메서드) 신규 추가
Keychain 저장소 구현
src/Projects/BKStorage/Sources/DeviceStorage/KeychainDeviceIDProvider.swift, src/Projects/BKStorage/Sources/DeviceStorage/KeychainDeviceIDStore.swift
KeychainDeviceIDProvider(캐싱 지원), KeychainDeviceIDStore(UUID 생성 및 저장) 클래스 신규 추가
FCM 토큰 API 통합
src/Projects/BKData/Sources/API/UserAPI.swift, src/Projects/BKData/Sources/DTO/Request/UpsertFCMTokenRequestDTO.swift
upsertFCMToken 케이스에 deviceId 매개변수 추가 및 요청 DTO에 deviceId 필드 추가
저장소 및 저장소 구성
src/Projects/BKStorage/Sources/Constant/StorageKeys.swift, src/Projects/BKStorage/Sources/StorageAssembly.swift
deviceIDKey 상수 신규 추가 및 DeviceIDProvider, DeviceIDStore DI 등록 신규 추가
저장소 계층 통합
src/Projects/BKData/Sources/Repository/DefaultNotificationRepository.swift, src/Projects/BKData/Sources/DataAssembly.swift
DefaultNotificationRepository에 DeviceIDProvider 의존성 주입 및 upsertFCMToken에서 기기 ID 사용 로직 구현
앱 시작 시 초기화
src/Projects/Booket/Sources/AppDelegate.swift
앱 시작 시 KeychainDeviceIDStore.shared.getOrCreate() 호출하여 기기 ID 생성 및 저장

시퀀스 다이어그램

sequenceDiagram
    participant AppDelegate as 앱 시작
    participant KeychainStore as KeychainDeviceIDStore
    participant Keychain as Keychain 저장소
    participant FCMUpsert as FCM 토큰 업로드
    participant DeviceProvider as DeviceIDProvider
    
    AppDelegate->>KeychainStore: getOrCreate()
    KeychainStore->>Keychain: 기기 ID 조회
    alt 기기 ID 존재
        Keychain-->>KeychainStore: 기기 ID 반환
    else 기기 ID 미존재
        KeychainStore->>KeychainStore: UUID 생성
        KeychainStore->>Keychain: UUID 저장
        Keychain-->>KeychainStore: 저장 완료
    end
    KeychainStore-->>AppDelegate: 기기 ID 발행
    
    AppDelegate->>FCMUpsert: 토큰 업로드 요청
    FCMUpsert->>DeviceProvider: deviceID 조회
    DeviceProvider->>Keychain: 캐시된 기기 ID 로드
    Keychain-->>DeviceProvider: 기기 ID 반환
    DeviceProvider-->>FCMUpsert: 기기 ID 반환
    FCMUpsert->>FCMUpsert: fcmToken + deviceId로 요청 생성
    FCMUpsert-->>AppDelegate: 업로드 완료
Loading

예상 코드 리뷰 노력

🎯 3 (중간) | ⏱️ ~25분

  • 별도 검토 필요 영역:
    • DefaultNotificationRepository.swift의 기기 ID 존재 여부 검증 로직 및 에러 처리 방식 확인
    • KeychainDeviceIDStore.swift의 UUID 생성 및 저장 실패 시 TokenError 매핑 검증
    • DataAssembly.swift의 DI 등록이 정확하게 Keychain 저장소에 연결되었는지 확인
    • AppDelegate.swift의 앱 시작 시점에서 비동기 작업 처리 및 구독 생명주기 관리 확인

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title accurately describes the main objective of the changeset: implementing FCM-related server specification updates by adding device ID creation and persistence logic across multiple files.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch BOOK-432-feature/#258

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (3)
src/Projects/Booket/Sources/AppDelegate.swift (1)

37-50: DI 패턴 일관성 고려

KeychainDeviceIDStore.shared를 직접 사용하고 있습니다. 프로젝트의 다른 부분에서는 @Autowired를 통한 의존성 주입 패턴을 사용하고 있는데, 여기서만 싱글톤을 직접 참조하고 있어 일관성이 떨어집니다.

StorageAssembly에서 DeviceIDStore가 이미 등록되어 있으므로, DI를 통해 주입받는 것을 고려해보세요.

참고: Keychain 데이터는 앱 삭제 시 기본적으로 함께 삭제됩니다. 재설치 후에도 유지하려면 kSecAttrAccessible 설정이 필요합니다.

src/Projects/BKData/Sources/Repository/DefaultNotificationRepository.swift (1)

22-26: 로깅 방식 통일 권장

Line 23에서 Log.error를 사용하고 있는데, 같은 파일 Line 32에서는 debugError(logger: AppLogger.network)를 사용하고 있습니다. 프로젝트의 다른 부분(예: AppDelegate)에서도 AppLogger를 사용하고 있으니, 일관성을 위해 AppLogger.storage를 사용하는 것이 좋겠습니다.

또한 DomainError.unknown은 너무 일반적인 에러입니다. 가능하다면 더 구체적인 에러 타입을 고려해보세요.

다음과 같이 수정할 수 있습니다:

-        guard let deviceID = deviceIDProvider.deviceID else {
-            Log.error("Device ID not available", logger: AppLogger.storage)
-            return Fail(error: DomainError.unknown)
-                .eraseToAnyPublisher()
-        }
+        guard let deviceID = deviceIDProvider.deviceID else {
+            AppLogger.error("Device ID not available", logger: AppLogger.storage)
+            return Fail(error: DomainError.unknown)
+                .eraseToAnyPublisher()
+        }
src/Projects/BKStorage/Sources/DeviceStorage/KeychainDeviceIDStore.swift (1)

20-20: TokenError 사용이 의미적으로 부자연스러움

Device ID 관련 작업에 TokenError를 사용하는 것은 의미적으로 맞지 않습니다. Device ID는 토큰과는 다른 개념입니다.

가능하다면 DeviceIDError 같은 별도의 에러 타입을 정의하거나, 더 일반적인 StorageError를 사용하는 것을 고려해보세요. 다만 프로토콜 시그니처가 이미 정의되어 있다면, 향후 리팩토링 시 고려사항으로 남겨두시면 됩니다.

Also applies to: 40-40

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e46cf91 and 46da8e9.

📒 Files selected for processing (11)
  • src/Projects/BKData/Sources/API/UserAPI.swift (2 hunks)
  • src/Projects/BKData/Sources/DTO/Request/UpsertFCMTokenRequestDTO.swift (1 hunks)
  • src/Projects/BKData/Sources/DataAssembly.swift (1 hunks)
  • src/Projects/BKData/Sources/Interface/Storage/DeviceIDProvider.swift (1 hunks)
  • src/Projects/BKData/Sources/Interface/Storage/DeviceIDStore.swift (1 hunks)
  • src/Projects/BKData/Sources/Repository/DefaultNotificationRepository.swift (1 hunks)
  • src/Projects/BKStorage/Sources/Constant/StorageKeys.swift (1 hunks)
  • src/Projects/BKStorage/Sources/DeviceStorage/KeychainDeviceIDProvider.swift (1 hunks)
  • src/Projects/BKStorage/Sources/DeviceStorage/KeychainDeviceIDStore.swift (1 hunks)
  • src/Projects/BKStorage/Sources/StorageAssembly.swift (1 hunks)
  • src/Projects/Booket/Sources/AppDelegate.swift (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: doyeonk429
Repo: YAPP-Github/Reed-iOS PR: 163
File: src/Projects/BKData/Sources/DTO/Request/UserBookRegisterRequestDTO.swift:7-9
Timestamp: 2025-08-08T16:13:31.712Z
Learning: Reed-iOS에서 서버 스펙 변경으로 DTO 필드명이 바뀔 때, doyeonk429는 코드 전반의 파라미터/시그니처 리네이밍까지 확장하지 않고, 실제 요청 직렬화 키가 맞는지(예: isbn13)만 보장하면 충분하다는 범위 최소화 방식을 선호한다. (PR: YAPP-Github/Reed-iOS#163, 파일: BKData/Sources/DTO/Request/UserBookRegisterRequestDTO.swift)
Learnt from: doyeonk429
Repo: YAPP-Github/Reed-iOS PR: 163
File: src/Projects/BKData/Sources/DTO/Response/UserBookResponseDTO.swift:0-0
Timestamp: 2025-08-08T16:14:35.659Z
Learning: Reed-iOS: 서버/DTO 응답 키는 isbn13로 정규화되었음(사용자 doyeonk429 확인). Domain은 현재 BookInfo가 isbn을 사용 중이라 혼재 상태이며, 향후 isbn13으로 통일 리팩터 필요 가능성이 있음.
📚 Learning: 2025-08-08T16:13:31.712Z
Learnt from: doyeonk429
Repo: YAPP-Github/Reed-iOS PR: 163
File: src/Projects/BKData/Sources/DTO/Request/UserBookRegisterRequestDTO.swift:7-9
Timestamp: 2025-08-08T16:13:31.712Z
Learning: Reed-iOS에서 서버 스펙 변경으로 DTO 필드명이 바뀔 때, doyeonk429는 코드 전반의 파라미터/시그니처 리네이밍까지 확장하지 않고, 실제 요청 직렬화 키가 맞는지(예: isbn13)만 보장하면 충분하다는 범위 최소화 방식을 선호한다. (PR: YAPP-Github/Reed-iOS#163, 파일: BKData/Sources/DTO/Request/UserBookRegisterRequestDTO.swift)

Applied to files:

  • src/Projects/BKData/Sources/API/UserAPI.swift
🧬 Code graph analysis (7)
src/Projects/Booket/Sources/AppDelegate.swift (2)
src/Projects/BKStorage/Sources/DeviceStorage/KeychainDeviceIDStore.swift (1)
  • getOrCreate (20-38)
src/Projects/BKCore/Sources/Logger/AppLogger.swift (1)
  • error (44-53)
src/Projects/BKData/Sources/Repository/DefaultNotificationRepository.swift (1)
src/Projects/BKCore/Sources/Logger/AppLogger.swift (1)
  • error (44-53)
src/Projects/BKStorage/Sources/StorageAssembly.swift (1)
src/Projects/BKCore/Sources/DiContainer/DIContainer.swift (1)
  • register (25-46)
src/Projects/BKData/Sources/Interface/Storage/DeviceIDProvider.swift (1)
src/Projects/BKStorage/Sources/DeviceStorage/KeychainDeviceIDProvider.swift (1)
  • clearCache (29-31)
src/Projects/BKStorage/Sources/DeviceStorage/KeychainDeviceIDProvider.swift (1)
src/Projects/BKCore/Sources/Logger/AppLogger.swift (1)
  • error (44-53)
src/Projects/BKData/Sources/API/UserAPI.swift (1)
src/Projects/BKData/Sources/Repository/DefaultNotificationRepository.swift (1)
  • upsertFCMToken (19-36)
src/Projects/BKData/Sources/Interface/Storage/DeviceIDStore.swift (1)
src/Projects/BKStorage/Sources/DeviceStorage/KeychainDeviceIDStore.swift (2)
  • getOrCreate (20-38)
  • clear (40-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build
🔇 Additional comments (8)
src/Projects/BKStorage/Sources/Constant/StorageKeys.swift (1)

8-8: LGTM!

기존 키 상수들과 일관된 네이밍 패턴을 따르고 있습니다.

src/Projects/BKData/Sources/DTO/Request/UpsertFCMTokenRequestDTO.swift (1)

5-8: LGTM!

서버 스펙 변경사항을 올바르게 반영하고 있으며, DTO 구조가 적절합니다.

src/Projects/BKData/Sources/DataAssembly.swift (1)

133-142: LGTM!

DeviceIDProvider를 DI를 통해 주입하는 방식이 기존 패턴과 일관되게 잘 구현되어 있습니다.

src/Projects/BKData/Sources/Interface/Storage/DeviceIDProvider.swift (1)

3-6: LGTM!

프로토콜 설계가 명확하고, 기존 Provider 패턴과 일관성 있게 구현되어 있습니다.

src/Projects/BKData/Sources/Interface/Storage/DeviceIDStore.swift (1)

6-9: LGTM!

기존 TokenStore 패턴과 일관된 인터페이스 설계입니다. Combine을 활용한 비동기 처리 방식도 적절합니다.

src/Projects/BKData/Sources/Repository/DefaultNotificationRepository.swift (1)

11-17: LGTM!

DeviceIDProvider 의존성 주입이 올바르게 구현되어 있습니다.

src/Projects/BKStorage/Sources/StorageAssembly.swift (1)

63-80: LGTM!

DeviceIDProviderDeviceIDStore의 등록이 기존 TokenProvider/TokenStorePushTokenProvider/PushTokenStore 패턴과 일관되게 구현되어 있습니다. Provider는 싱글톤으로, Store는 기본 스코프로 등록하는 방식이 적절합니다.

src/Projects/BKData/Sources/API/UserAPI.swift (1)

8-8: LGTM! FCM 서버 스펙 변경사항이 올바르게 반영됨

upsertFCMToken API에 deviceId 파라미터가 깔끔하게 추가되었고, DTO 생성 로직도 일관되게 업데이트되었습니다. DefaultNotificationRepository에서 deviceID가 없을 때 적절히 가드 처리하고 있어 안전합니다.

Based on learnings: 서버 스펙 변경 시 최소 범위로 변경하는 팀의 선호도에 부합합니다.

Also applies to: 54-55

Comment on lines +7 to +27
public final class KeychainDeviceIDProvider: DeviceIDProvider {
private let storage: KeyValueStorage
private var cachedDeviceID: String?

public init(storage: KeyValueStorage) {
self.storage = storage
}

public var deviceID: String? {
if let cachedDeviceID {
return cachedDeviceID
}
do {
let id: String = try storage.load(for: StorageKeys.deviceIDKey)
self.cachedDeviceID = id
return id
} catch {
Log.error("Failed to load deviceID: \(error)", logger: AppLogger.storage)
return nil
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

스레드 안전성 문제: 캐시 접근이 동기화되지 않음

cachedDeviceID는 여러 스레드에서 동시에 접근될 수 있지만 동기화 메커니즘이 없습니다. 특히 deviceID 프로퍼티에서 캐시를 읽고(line 16-18), 저장소에서 로드한 후 캐시에 쓰는(line 21) 작업이 원자적이지 않아 레이스 컨디션이 발생할 수 있습니다.

다음 중 하나의 방법으로 해결하세요:

방법 1: DispatchQueue를 사용한 동기화

 public final class KeychainDeviceIDProvider: DeviceIDProvider {
     private let storage: KeyValueStorage
     private var cachedDeviceID: String?
+    private let queue = DispatchQueue(label: "com.booket.deviceIDProvider")
 
     public init(storage: KeyValueStorage) {
         self.storage = storage
     }
     
     public var deviceID: String? {
-        if let cachedDeviceID {
-            return cachedDeviceID
-        }
-        do {
-            let id: String = try storage.load(for: StorageKeys.deviceIDKey)
-            self.cachedDeviceID = id
-            return id
-        } catch {
-            Log.error("Failed to load deviceID: \(error)", logger: AppLogger.storage)
-            return nil
+        return queue.sync {
+            if let cachedDeviceID {
+                return cachedDeviceID
+            }
+            do {
+                let id: String = try storage.load(for: StorageKeys.deviceIDKey)
+                self.cachedDeviceID = id
+                return id
+            } catch {
+                Log.error("Failed to load deviceID: \(error)", logger: AppLogger.storage)
+                return nil
+            }
         }
     }
     
     public func clearCache() {
-        cachedDeviceID = nil
+        queue.sync {
+            cachedDeviceID = nil
+        }
     }

방법 2: actor 사용 (iOS 13+ 지원 시)

actor를 사용하면 자동으로 스레드 안전성이 보장되지만, deviceID가 동기 프로퍼티에서 비동기로 변경되어야 합니다.

Committable suggestion skipped: line range outside the PR's diff.

}

/// 디바이스 ID를 가져오거나, 없으면 새로 생성하여 저장
/// 키체인에 저장되므로 앱 삭제 후 재설치해도 유지됨
Copy link

@coderabbitai coderabbitai bot Nov 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

부정확한 주석: 키체인의 앱 재설치 시 동작

주석에서 "앱 삭제 후 재설치해도 유지됨"이라고 명시되어 있으나, 기본 키체인 항목은 앱 삭제 시 함께 삭제됩니다. 키체인 데이터를 유지하려면 별도의 접근 제어 설정(예: kSecAttrAccessGroup)과 앱 그룹 구성이 필요합니다.

주석을 수정하여 실제 동작을 정확히 반영하세요:

-    /// 디바이스 ID를 가져오거나, 없으면 새로 생성하여 저장
-    /// 키체인에 저장되므로 앱 삭제 후 재설치해도 유지됨
+    /// 디바이스 ID를 가져오거나, 없으면 새로 생성하여 저장
+    /// 키체인에 저장되므로 앱 업데이트나 재시작 시에도 유지됨 (앱 삭제 시에는 제거됨)

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In src/Projects/BKStorage/Sources/DeviceStorage/KeychainDeviceIDStore.swift
around line 19, the comment stating "앱 삭제 후 재설치해도 유지됨" is inaccurate; update the
comment to explain that by default keychain items may be removed when an app is
uninstalled and that retaining keychain data across uninstall/reinstall requires
explicit configuration (e.g., using kSecAttrAccessGroup and configuring an App
Group/Keychain Access Group or other secure backup strategies). Replace the
current statement with a concise, accurate note about default behavior and the
steps/requirements needed to preserve keychain entries across app
reinstallations.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kSecAttrAccessGroup는 다른 이야기를 하고 있는거같은데. 이건 공유를 위해서 쓰는 키고.
실제로 키체인은 앱 재설치 이후에도 남아있는걸로 알고 있는데 ? 다시 확인해볼 필요가 있을 듯.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🌐 Web query:

iOS keychain persistence after app uninstall reinstall

💡 Result:

Short answer: By default, Keychain items generally survive an app uninstall and are available after reinstall — because keychain storage is kept by the OS, separate from the app sandbox. They are only removed if the device/keychain is wiped or if the items are explicitly deleted (or the keychain is reset). [1][2][3][4]

Important caveats:

  • The app must use the same keychain access group / code‑signing team/entitlements; otherwise the new install won’t be able to read the old items. [2][3]
  • iCloud Keychain sync or backups can affect which items persist/are restored. [3]
  • If you need “fresh” installs to start clean, detect first-launch (NSUserDefaults cleared on uninstall) and explicitly remove keychain items on first run, or revoke/remove server tokens before uninstall. [2][4]

Sources: Apple Keychain docs and community/engineering reports. [1][2][3][4]


@clxxrlove 말씀하신 것이 맞습니다. 죄송합니다. 제가 잘못된 정보를 드렸네요.

확인 결과, iOS 키체인 아이템은 기본적으로 앱 삭제 후 재설치해도 유지됩니다. 키체인 스토리지는 앱 샌드박스가 아닌 OS 레벨에서 별도로 관리되기 때문입니다.

말씀하신 대로 kSecAttrAccessGroup은 앱 간 키체인 공유를 위한 것이고, 재설치 후 지속성과는 별개의 개념입니다.

따라서 현재 코드의 주석 **"키체인에 저장되므로 앱 삭제 후 재설치해도 유지됨"**은 정확합니다. 제 리뷰 코멘트가 부정확했습니다.

참고사항:

  • 재설치 후 동일한 코드 서명/엔타이틀먼트를 사용해야 기존 키체인 아이템에 접근 가능합니다
  • 디바이스를 초기화하거나 명시적으로 삭제하지 않는 한 키체인 데이터는 유지됩니다

지적해주셔서 감사합니다! 🙏


🧠 Learnings used
Learnt from: doyeonk429
Repo: YAPP-Github/Reed-iOS PR: 68
File: src/Projects/BKDesign/PreviewApp/Sources/View/BKButtonTestViewController.swift:0-0
Timestamp: 2025-07-10T08:21:44.204Z
Learning: doyeonk429는 BKDesign PreviewApp의 테스트 파일에서 주석 처리된 코드를 의도적으로 남겨두어 쉽게 테스트 구성을 전환할 수 있도록 구현한다. 이는 개발 편의성을 위한 것으로, 테스트 파일에서는 일반적인 "주석 처리된 코드 제거" 규칙을 적용하지 않는다.

Learnt from: doyeonk429
Repo: YAPP-Github/Reed-iOS PR: 163
File: src/Projects/BKData/Sources/DTO/Request/UserBookRegisterRequestDTO.swift:7-9
Timestamp: 2025-08-08T16:13:31.712Z
Learning: Reed-iOS에서 서버 스펙 변경으로 DTO 필드명이 바뀔 때, doyeonk429는 코드 전반의 파라미터/시그니처 리네이밍까지 확장하지 않고, 실제 요청 직렬화 키가 맞는지(예: isbn13)만 보장하면 충분하다는 범위 최소화 방식을 선호한다. (PR: YAPP-Github/Reed-iOS#163, 파일: BKData/Sources/DTO/Request/UserBookRegisterRequestDTO.swift)

Learnt from: doyeonk429
Repo: YAPP-Github/Reed-iOS PR: 163
File: src/Projects/BKData/Sources/DTO/Response/UserBookResponseDTO.swift:0-0
Timestamp: 2025-08-08T16:14:35.659Z
Learning: Reed-iOS: 서버/DTO 응답 키는 isbn13로 정규화되었음(사용자 doyeonk429 확인). Domain은 현재 BookInfo가 isbn을 사용 중이라 혼재 상태이며, 향후 isbn13으로 통일 리팩터 필요 가능성이 있음.

Comment on lines +20 to +38
public func getOrCreate() -> AnyPublisher<String, TokenError> {
do {
if let existingID: String = try? storage.load(for: StorageKeys.deviceIDKey) {
return Just(existingID)
.setFailureType(to: TokenError.self)
.eraseToAnyPublisher()
}

let newDeviceID = UUID().uuidString
try storage.save(newDeviceID, for: StorageKeys.deviceIDKey)

return Just(newDeviceID)
.setFailureType(to: TokenError.self)
.eraseToAnyPublisher()
} catch {
return Fail(error: TokenError.saveFailed(underlying: error))
.eraseToAnyPublisher()
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

레이스 컨디션: getOrCreate()의 check-then-act 패턴이 원자적이지 않음

getOrCreate() 메서드에서 저장소 확인(line 22)과 생성/저장(lines 28-29)이 별도의 작업으로 분리되어 있어, 여러 스레드나 여러 호출이 동시에 실행되면 중복된 UUID가 생성될 수 있습니다. 특히 앱 초기화 시 여러 컴포넌트에서 동시에 호출될 가능성이 있습니다.

저장소 레이어에 동기화 메커니즘을 추가하거나, 이 메서드 자체를 동기화 처리하세요:

+import Foundation
+
 public struct KeychainDeviceIDStore: DeviceIDStore {
     private let storage: KeyValueStorage
+    private static let lock = NSLock()
     
     /// AppDelegate나 다른 곳에서 DI 없이 사용할 수 있도록 shared instance 제공
     public static let shared = KeychainDeviceIDStore(storage: KeychainKeyValueStorage())

     public init(storage: KeyValueStorage) {
         self.storage = storage
     }
     
     /// 디바이스 ID를 가져오거나, 없으면 새로 생성하여 저장
     /// 키체인에 저장되므로 앱 삭제 후 재설치해도 유지됨
     public func getOrCreate() -> AnyPublisher<String, TokenError> {
+        Self.lock.lock()
+        defer { Self.lock.unlock() }
+        
         do {
             if let existingID: String = try? storage.load(for: StorageKeys.deviceIDKey) {
                 return Just(existingID)
                     .setFailureType(to: TokenError.self)
                     .eraseToAnyPublisher()
             }
             
             let newDeviceID = UUID().uuidString
             try storage.save(newDeviceID, for: StorageKeys.deviceIDKey)

             return Just(newDeviceID)
                 .setFailureType(to: TokenError.self)
                 .eraseToAnyPublisher()
         } catch {
             return Fail(error: TokenError.saveFailed(underlying: error))
                 .eraseToAnyPublisher()
         }
     }

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In src/Projects/BKStorage/Sources/DeviceStorage/KeychainDeviceIDStore.swift
around lines 20 to 38, the getOrCreate() method uses a non-atomic check-then-act
sequence which can produce duplicate UUIDs under concurrent calls; make the
operation atomic by serializing access or using a concurrency primitive: either
protect the entire get-or-create sequence with a dedicated serial DispatchQueue
or lock (or convert to a Swift actor) so only one caller can check and create at
a time, or alternatively attempt to write the new UUID and on write-failure
re-load the stored value and return it; update error handling to reflect the
chosen approach and ensure the returned publisher always contains the single
canonical device ID.

Comment on lines +28 to +30
let newDeviceID = UUID().uuidString
try storage.save(newDeviceID, for: StorageKeys.deviceIDKey)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

실제 디바이스 아이디 UIDevice.current.identifierForVendor!.uuidString 이런식으로 구할 수 있는걸로 알고 있긴 한데, 이거랑 UUID랑 크게 역할이 다르지 않을까요?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

애초에 FCM 토큰 외에 deviceId를 별도로 두는 이유는 FCM은 앱 재설치 등의 변수를 통해 변할 수 있기 떄문입니다. 이러면 deviceId를 딱히 쓰는 의미는 없다고 생각하구요. (생명 주기가 같으므로)
말씀해주신 IDFV는 실제 디바이스 아이디는 아니구요, Team ID 단위로 부여되는 Identifier인데, FCM이랑 마찬가지로 앱 재설치하는 경우 변합니다.
UUID가 이런걸 대체해주기보다는, 정확히는 키체인을 믿고 설계한겁니다. 키체인은 앱 재설치/기기 초기화 등 변수에서도 남아있을 수 있어요. 초기화의 경우에는 리스토어를 해줘야 돌아오긴 하지만요.

추가로, 실제 디바이스 ID에 해당하는 IMEI나 MAC 같은 정말 PK가 될 수 있는 값들은 정책상 접근 금지이므로 이런 꼼수를 통해 deviceId를 공유한거에요.

나름 조금 알아보고 했는데, 혹시라도 제 의견이 틀렸거나 다른 의견이 있으시면 말씀해주세요. 개선 방향이 있는지 더 알아볼게요.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아 네 저도 일단 IDFV 생각하고 말씀드린거긴 한데, 이게 앱 재설치 했을 때 값이 변할 수 있다는 사실을 제가 몰랐었네요! 지금 방식도 충분한데, 왜 안쓰셨는지 궁금해서 여쭤봤습니다! 👍

@clxxrlove clxxrlove merged commit 69dad06 into develop Nov 9, 2025
5 checks passed
@clxxrlove clxxrlove deleted the BOOK-432-feature/#258 branch November 9, 2025 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants