firebase-ios-sdk icon indicating copy to clipboard operation
firebase-ios-sdk copied to clipboard

Multiple FieldValue.delete() in same setData call causes listener to be called with wrong keys the first time

Open mobmad opened this issue 3 years ago • 2 comments

Description

Please see source code below to understand the context and screenshot from demo app reproducing the issue.

Actual result:

When adding two (or more) FieldValue.delete() fields where only one would have an effect, the listener is called with invalid/missing keys the first time. Illustrated with the demo app:

After pressing set Data with born date and then Fail: setData w/multiple FieldValue.delete(), addSnapshotListener is called twice: Simulator Screen Shot - iPhone 13 Pro - 2022-08-30 at 19 05 06

  • diff 2 in screenshot above: The first time it is called with born and first as keys, resulting in being unable to instantiate the User model (from demo app below) at this point, since last is missing for some reason. I would have expected first and last being set, since it is born that is being removed, not last.

  • diff 3 in screenshot above: The listener is called a second time, almost immediately. With first and last, as I would expect the first time.

Expected result:

addSnapshotListener is called once with existing keys (first and last in this example), or if that's impossible, at least with the existing keys so mapping doesn't fail if required fields are missing.

One might argue that it isn't a big issue since it "auto-correct" itself by calling the listener a second time with correct data, but there is inconsistent behaviour here and IMO it doesn't make sense to call the listener with incomplete data that may trigger weird bugs that are hard to debug.

Additional info:

  • The bug only seem to occur when 2 or more FieldValue.delete() are in place, and only if at least one of them ends up modifying the underlying data.
  • The bug does not occur if multiple FieldValue.delete() does not end up modifying the underlying data. So pressing Fail: setData w/multiple fields multiple times in the demo app only triggers the bug if "born" is set.
  • It seems that it is the last key (sorted alphabetically) is the one that is removed. In this case that is last, but if you had a key zzz that would be the one that is removed.

Reproducing the issue

import SwiftUI
import FirebaseCore
import FirebaseFirestore


@main
struct MappingBugApp: App {
    init() {
        FirebaseApp.configure()
    }
    var body: some Scene {
        WindowGroup {
            ContentView()
        }
    }
}

ContentView.swift

import SwiftUI
import FirebaseCore
import FirebaseFirestore

struct DebugEntry: Identifiable {
    var id: String
    var keys: String
    var user: User?
}

struct User {
    var first: String
    var last: String
    var alwaysDeleted: String?
    var born: Int?
}

struct ContentView: View {
    var db = Firestore.firestore()
    var documentId = "B50009BC-0200-4E78-A922-D01F76FA3040"
    
    @State private var debugEntryCounter = 0
    @State private var debugEntries: [DebugEntry] = []
    
    private func setupListener() {
        db.collection("users")
        .addSnapshotListener { querySnapshot, error in
                guard let querySnapshot = querySnapshot else {
                    print("Error \(String(describing: error))")
                    return
                }
                
                querySnapshot.documentChanges.forEach { diff in
                    let data = diff.document.data()
                    
                    var mappedUser: User?
                    if let first = data["first"] as? String,
                       let last = data["last"] as? String {
                        
                        let born = data["born"] as? Int
                        let alwaysDeleted = data["alwaysDeleted"] as? String
                        
                        mappedUser = User(first: first, last: last, alwaysDeleted: alwaysDeleted, born: born)
                    }
                    
                    self.debugEntryCounter += 1
                    let debugEntry = DebugEntry(id: String(debugEntryCounter),
                                                keys: data.keys.sorted().joined(separator: ", "),
                                                user: mappedUser)
                    self.debugEntries.insert(debugEntry, at: 0)
                }
            }
    }

    func setData(_ data: [String: Any]) {
        let userRef = db.collection("users").document(documentId)
        userRef.setData(data, merge: true)
    }
    
    var body: some View {
        VStack {
            Button(action: {
                setData([
                    "first": "Ada",
                    "last": "Lovelace",
                    "born": 1815,
                ])
            }) {
                Text("setData with born date")
            }
            
            // Triggers the bug if "born" is set (multiple FieldValue.delete() and one is affecting underlying data while the other one is not)
            Button(action: {
                setData([
                    "first": "Ada",
                    "last": "Lovelace",
                    "alwaysDeleted": FieldValue.delete(),
                    "born": FieldValue.delete(),
                ])
            }) {
                Text("Fail: setData w/multiple FieldValue.delete()")
            }
            
            // This always works as intended (only one FieldValue.delete())
            Button(action: {
                setData([
                    "first": "Ada",
                    "last": "Lovelace",
                    "born": FieldValue.delete(),
                ])
            }) {
                Text("Works: setData w/single FieldValue.delete()")
            }
            
            List {
                ForEach(self.debugEntries) { entries in
                    VStack(alignment: .leading) {
                        if let user = entries.user {
                            Text("\(user.first) \(user.last) \(user.born != nil ? String(user.born!) : "")")
                        } else {
                            Text("Failed to map user!").foregroundColor(.red)
                        }
                        Text("keys: \(entries.keys)").foregroundColor(.secondary)
                        Text("diff #\(entries.id)").font(.footnote).foregroundColor(.secondary)
                    }
                }
            }
        }.onAppear {
            setupListener()
        }
    }
}

Firebase SDK Version

9.5.0

Xcode Version

13.4.1

Installation Method

Swift Package Manager

Firebase Product(s)

Firestore

Targeted Platforms

iOS

Relevant Log Output

No response

If using Swift Package Manager, the project's Package.resolved

Expand Package.resolved snippet

Replace this line with the contents of your Package.resolved.

If using CocoaPods, the project's Podfile.lock

Expand Podfile.lock snippet

Replace this line with the contents of your Podfile.lock!

mobmad avatar Aug 30 '22 18:08 mobmad

I couldn't figure out how to label this issue, so I've labeled it for a human to triage. Hang tight.

google-oss-bot avatar Aug 30 '22 18:08 google-oss-bot

@mobmad, thanks for bringing this to our attention. We will look into it and keep you updated.

MarkDuckworth avatar Aug 31 '22 17:08 MarkDuckworth