Skip to content

Commit

Permalink
Rewrite firestore account storage to new model
Browse files Browse the repository at this point in the history
  • Loading branch information
Supereg committed Jul 25, 2024
1 parent e907b70 commit c2c2908
Show file tree
Hide file tree
Showing 7 changed files with 274 additions and 178 deletions.
256 changes: 130 additions & 126 deletions Sources/SpeziFirebaseAccount/FirebaseAccountService.swift

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ enum FirebaseAccountError: LocalizedError { // TODO: make public?


init(authErrorCode: AuthErrorCode) {
FirebaseAccountService.logger.debug("Received authError with code \(authErrorCode)") // TODO: what?
// TODO: FirebaseAccountService.logger.debug("Received authError with code \(authErrorCode)") // TODO: what?

switch authErrorCode.code {
case .invalidEmail, .invalidRecipientEmail:
Expand Down
6 changes: 3 additions & 3 deletions Sources/SpeziFirebaseAccount/Models/FirebaseContext.swift
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
// SPDX-License-Identifier: MIT
//

import FirebaseAuth
@preconcurrency import FirebaseAuth
import OSLog
import Spezi
import SpeziAccount
Expand Down Expand Up @@ -71,7 +71,7 @@ actor FirebaseContext: Module, DefaultInitializable {

// a overload that just returns void
func dispatchFirebaseAuthAction(
action: () async throws -> Void
action: @Sendable () async throws -> Void
) async throws {
try await self.dispatchFirebaseAuthAction {
try await action()
Expand All @@ -90,7 +90,7 @@ actor FirebaseContext: Module, DefaultInitializable {
/// we can forward additional information back to SpeziAccount.
@_disfavoredOverload
func dispatchFirebaseAuthAction(
action: () async throws -> AuthDataResult?
action: @Sendable () async throws -> AuthDataResult?
) async throws {
defer {
cleanupQueuedChanges()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,40 @@
// SPDX-License-Identifier: MIT
//

import AuthenticationServices

enum ReauthenticationOperationResult {
case cancelled
case success

struct ReauthenticationOperation {
enum Result {
case success
case cancelled
}

let result: Result
/// The OAuth Credential if re-authentication was made through Single-Sign-On Provider.
let credential: ASAuthorizationAppleIDCredential?

private init(result: Result, credential: ASAuthorizationAppleIDCredential? = nil) {
self.result = result
self.credential = credential
}
}


extension ReauthenticationOperation {
static var cancelled: ReauthenticationOperation {
.init(result: .cancelled)
}

static var success: ReauthenticationOperation {
.init(result: .success)
}


static func success(with credential: ASAuthorizationAppleIDCredential) -> ReauthenticationOperation {
.init(result: .success, credential: credential)
}
}


extension ReauthenticationOperation: Hashable {}
4 changes: 4 additions & 0 deletions Sources/SpeziFirebaseAccount/Views/FirebaseLoginView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ struct FirebaseLoginView: View {


var body: some View {
// TODO: configure preferred visual way:
// => signup view + Already have an account?
// => login view + Don't have an account yet?
// TODO: emebded login view styles?
UserIdPasswordEmbeddedView { credential in
try await service.login(userId: credential.userId, password: credential.password)
} signup: { details in
Expand Down
141 changes: 98 additions & 43 deletions Sources/SpeziFirebaseAccountStorage/FirestoreAccountStorage.swift
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
// SPDX-License-Identifier: MIT
//

import FirebaseFirestore
@preconcurrency import FirebaseFirestore
import Spezi
import SpeziAccount
import SpeziFirestore
Expand Down Expand Up @@ -69,74 +69,103 @@ import SpeziFirestore
/// }
/// }
/// ```
public actor FirestoreAccountStorage: Module, AccountStorageConstraint {
public actor FirestoreAccountStorage: AccountStorageProvider { // TODO: completely restructure docs!

Check failure on line 72 in Sources/SpeziFirebaseAccountStorage/FirestoreAccountStorage.swift

View workflow job for this annotation

GitHub Actions / SwiftLint / SwiftLint / SwiftLint

Todo Violation: TODOs should be resolved (completely restructure docs!) (todo)
@Dependency private var firestore: SpeziFirestore.Firestore // ensure firestore is configured
@Dependency private var externalStorage: ExternalAccountStorage

private let collection: () -> CollectionReference
private let collection: @Sendable () -> CollectionReference


private var listenerRegistrations: [String: ListenerRegistration] = [:]
private var localCache: [String: AccountDetails] = [:]

public init(storeIn collection: @Sendable @autoclosure @escaping () -> CollectionReference) {
self.collection = collection
}


private func userDocument(for accountId: String) -> DocumentReference {
private nonisolated func userDocument(for accountId: String) -> DocumentReference {
collection().document(accountId)
}

public func create(_ identifier: AdditionalRecordId, _ details: SignupDetails) async throws {
let result = details.acceptAll(FirestoreEncodeVisitor())
private func snapshotListener(for accountId: String, with keys: [any AccountKey.Type]) {
if let existingListener = listenerRegistrations[accountId] {
existingListener.remove()
}
let document = userDocument(for: accountId)

do {
switch result {
case let .success(data):
try await userDocument(for: identifier.accountId)
.setData(data, merge: true)
case let .failure(error):
throw error
listenerRegistrations[accountId] = document.addSnapshotListener { [weak self] snapshot, error in
guard let self else {
return
}

guard let snapshot else {
// TODO: error happened, how to best notify about error?

Check failure on line 103 in Sources/SpeziFirebaseAccountStorage/FirestoreAccountStorage.swift

View workflow job for this annotation

GitHub Actions / SwiftLint / SwiftLint / SwiftLint

Todo Violation: TODOs should be resolved (error happened, how to best no...) (todo)
return
}

Task {
await self.processUpdatedSnapshot(for: accountId, with: keys, snapshot)
}
}
}

private func processUpdatedSnapshot(for accountId: String, with keys: [any AccountKey.Type], _ snapshot: DocumentSnapshot) {
do {
let details = try buildAccountDetails(from: snapshot, keys: keys)
localCache[accountId] = details

externalStorage.notifyAboutUpdatedDetails(for: accountId, details)
} catch {
throw FirestoreError(error)
// TODO: log or do something with that info!

Check failure on line 120 in Sources/SpeziFirebaseAccountStorage/FirestoreAccountStorage.swift

View workflow job for this annotation

GitHub Actions / SwiftLint / SwiftLint / SwiftLint

Todo Violation: TODOs should be resolved (log or do something with that ...) (todo)
// TODO: does it make sense to notify the account service about the error?

Check failure on line 121 in Sources/SpeziFirebaseAccountStorage/FirestoreAccountStorage.swift

View workflow job for this annotation

GitHub Actions / SwiftLint / SwiftLint / SwiftLint

Todo Violation: TODOs should be resolved (does it make sense to notify t...) (todo)
}
}

public func load(_ identifier: AdditionalRecordId, _ keys: [any AccountKey.Type]) async throws -> PartialAccountDetails {
let builder = PartialAccountDetails.Builder()
private nonisolated func buildAccountDetails(from snapshot: DocumentSnapshot, keys: [any AccountKey.Type]) throws -> AccountDetails {
guard let data = snapshot.data() else {
return AccountDetails()
}

let document = userDocument(for: identifier.accountId)
return try .build { details in
for key in keys {
guard let value = data[key.identifier] else {
continue
}

do {
let data = try await document
.getDocument()
.data()

if let data {
for key in keys {
guard let value = data[key.identifier] else {
continue
}

let visitor = FirestoreDecodeVisitor(value: value, builder: builder, in: document)
key.accept(visitor)
if case let .failure(error) = visitor.final() {
throw error
}
let visitor = FirestoreDecodeVisitor(value: value, builder: details, in: snapshot.reference)
key.accept(visitor)
if case let .failure(error) = visitor.final() {
throw FirestoreError(error)
}
}
} catch {
throw FirestoreError(error)
}
}

public func create(_ accountId: String, _ details: AccountDetails) async throws {
// we just treat it as modifications
let modifications = try AccountModifications(modifiedDetails: details)
try await modify(accountId, modifications)
}

public func load(_ accountId: String, _ keys: [any AccountKey.Type]) async throws -> AccountDetails? { // TODO: transport keys as set?

Check failure on line 151 in Sources/SpeziFirebaseAccountStorage/FirestoreAccountStorage.swift

View workflow job for this annotation

GitHub Actions / SwiftLint / SwiftLint / SwiftLint

Todo Violation: TODOs should be resolved (transport keys as set?) (todo)
let cached = localCache[accountId]

if listenerRegistrations[accountId] != nil { // check that there is a snapshot listener in place
snapshotListener(for: accountId, with: keys)
}


return builder.build()
return cached // TODO: also try to load from disk if in-memory cache doesn't work!

Check failure on line 159 in Sources/SpeziFirebaseAccountStorage/FirestoreAccountStorage.swift

View workflow job for this annotation

GitHub Actions / SwiftLint / SwiftLint / SwiftLint

Todo Violation: TODOs should be resolved (also try to load from disk if ...) (todo)
}

public func modify(_ identifier: AdditionalRecordId, _ modifications: AccountModifications) async throws {
public func modify(_ accountId: String, _ modifications: AccountModifications) async throws {
let result = modifications.modifiedDetails.acceptAll(FirestoreEncodeVisitor())

do {
switch result {
case let .success(data):
try await userDocument(for: identifier.accountId)
try await userDocument(for: accountId)
.setData(data, merge: true)

Check warning on line 169 in Sources/SpeziFirebaseAccountStorage/FirestoreAccountStorage.swift

View workflow job for this annotation

GitHub Actions / Build and Test Swift Package / Test using xcodebuild or run fastlane

passing argument of non-sendable type 'FirestoreEncodeVisitor.Data' (aka 'Dictionary<String, Any>') outside of actor-isolated context may introduce data races
case let .failure(error):
throw error
Expand All @@ -146,20 +175,46 @@ public actor FirestoreAccountStorage: Module, AccountStorageConstraint {
result[key.identifier] = FieldValue.delete()
}

try await userDocument(for: identifier.accountId)
try await userDocument(for: accountId)
.updateData(removedFields)

Check warning on line 179 in Sources/SpeziFirebaseAccountStorage/FirestoreAccountStorage.swift

View workflow job for this annotation

GitHub Actions / Build and Test Swift Package / Test using xcodebuild or run fastlane

passing argument of non-sendable type '[String : Any]' outside of actor-isolated context may introduce data races
} catch {
throw FirestoreError(error)
}

// make sure our cache is consistent
let details: AccountDetails = .build { details in
if let cached = localCache[accountId] {
details.add(contentsOf: cached)
}
details.add(contentsOf: modifications.modifiedDetails, merge: true)
details.removeAll(modifications.removedAccountKeys)
}
localCache[accountId] = details


// TODO: check if the snapshot listener is in place with the same set of keys (add remove)!

Check failure on line 195 in Sources/SpeziFirebaseAccountStorage/FirestoreAccountStorage.swift

View workflow job for this annotation

GitHub Actions / SwiftLint / SwiftLint / SwiftLint

Todo Violation: TODOs should be resolved (check if the snapshot listener...) (todo)
if listenerRegistrations[accountId] != nil {
// TODO: if we have sets, its easier!

Check failure on line 197 in Sources/SpeziFirebaseAccountStorage/FirestoreAccountStorage.swift

View workflow job for this annotation

GitHub Actions / SwiftLint / SwiftLint / SwiftLint

Todo Violation: TODOs should be resolved (if we have sets, its easier!) (todo)
// TODO: actually keep track of all account keys, this will fail!

Check failure on line 198 in Sources/SpeziFirebaseAccountStorage/FirestoreAccountStorage.swift

View workflow job for this annotation

GitHub Actions / SwiftLint / SwiftLint / SwiftLint

Todo Violation: TODOs should be resolved (actually keep track of all acc...) (todo)
snapshotListener(for: accountId, with: modifications.modifiedDetails.keys)
}
}

public func clear(_ identifier: AdditionalRecordId) async {
// nothing we can do ...
public func disassociate(_ accountId: String) {
guard let registration = listenerRegistrations.removeValue(forKey: accountId) else {
return
}
registration.remove()

localCache.removeValue(forKey: accountId)
// TODO: remove values form disk! don't keep personal data after logout
}

public func delete(_ identifier: AdditionalRecordId) async throws {
public func delete(_ accountId: String) async throws {
disassociate(accountId)

do {
try await userDocument(for: identifier.accountId)
try await userDocument(for: accountId)
.delete()
} catch {
throw FirestoreError(error)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,14 @@ import SpeziAccount


class FirestoreDecodeVisitor: AccountKeyVisitor {
private let builder: PartialAccountDetails.Builder
private let builder: SimpleBuilder<AccountDetails>
private let value: Any
private let reference: DocumentReference

private var error: Error?


init(value: Any, builder: PartialAccountDetails.Builder, in reference: DocumentReference) {
init(value: Any, builder: SimpleBuilder<AccountDetails>, in reference: DocumentReference) {
self.value = value
self.builder = builder
self.reference = reference
Expand All @@ -29,6 +29,7 @@ class FirestoreDecodeVisitor: AccountKeyVisitor {
let decoder = Firestore.Decoder()

do {
// TODO: do we really need to pass the doc reference?

Check failure on line 32 in Sources/SpeziFirebaseAccountStorage/Visitor/FirestoreDecodeVisitor.swift

View workflow job for this annotation

GitHub Actions / SwiftLint / SwiftLint / SwiftLint

Todo Violation: TODOs should be resolved (do we really need to pass the ...) (todo)
try builder.set(key, value: decoder.decode(Key.Value.self, from: value, in: reference))
} catch {
self.error = error
Expand Down

0 comments on commit c2c2908

Please sign in to comment.