Require explicit typing for DocumentSnapshot decoding. DocumentReference decoding.

[mortenbekditlevsen]

A while back, the Codable API for RTDB was accepted.
That API differs from the existing Codable implementation for Firestore in one important aspect:

Decoding a DocumentSnapshot in Firestore always returns an Optional result even though you may have prior knowledge (you are iterating over documents in a collection, which means they must exist) or you have prior explicit expectations (for instance: I consider it to be an error if I cannot find a Configuration entity in this specific Firestore document).

The similar RTDB API allows the user to be more explicit about their expectations. If they know or have a hard expectation about the presence of an entity, they can use:

let configuration = try snapshot.data(as: Configuration.self) 

while if they only know that a specific reference only might contain an entity, they can be explicit about that expectation as well:

let optionalConfiguration = try snapshot.data(as: Configuration?.self)

I suggest to allow the users of Firestore the same amount of flexibility and explicit control, decreasing the amount of required 'unwrapping' when dealing with Codable entities in Firestore.

I also added a getDocument(as: ...) API in order to allow for simpler usage of the API.

To take a real world example, here's the guide from @peterfriese to simplify Firestore usage using Codable:
https://github.com/peterfriese/Swift-Firestore-Guide/blob/main/FirestoreCodableSamples/Screens/MappingSimpleTypesScreen.swift

  private func fetchBook(documentId: String) {
    let docRef = db.collection("books").document(documentId)
    
    docRef.getDocument { document, error in
      if let error = error as NSError? {
        self.errorMessage = "Error getting document: \(error.localizedDescription)"
      }
      else {
        let result = Result { try document?.data(as: Book.self) }
        switch result {
        case .success(let book):
          if let book = book {
            // A Book value was successfully initialized from the DocumentSnapshot.
            self.book = book
            self.errorMessage = nil
          }
          else {
            // A nil value was successfully initialized from the DocumentSnapshot,
            // or the DocumentSnapshot was nil.
            self.errorMessage = "Document doesn't exist."
          }
        case .failure(let error):
          // A Book value could not be initialized from the DocumentSnapshot.
          switch error {
          case DecodingError.typeMismatch(_, let context):
            self.errorMessage = "\(error.localizedDescription): \(context.debugDescription)"
          case DecodingError.valueNotFound(_, let context):
            self.errorMessage = "\(error.localizedDescription): \(context.debugDescription)"
          case DecodingError.keyNotFound(_, let context):
            self.errorMessage = "\(error.localizedDescription): \(context.debugDescription)"
          case DecodingError.dataCorrupted(let key):
            self.errorMessage = "\(error.localizedDescription): \(key)"
          default:
            self.errorMessage = "Error decoding document: \(error.localizedDescription)"
          }
        }
      }
    }
  }

with this PR this can be simplified a bit further to:

private func fetchBook(documentId: String) {
    let docRef = db.collection("books").document(documentId)
    
    docRef.getDocument(as: Book.self) { result in
      do {
        // A Book value was successfully initialized from the DocumentSnapshot.
        self.book = try result.get()
        self.errorMessage = nil
      } catch {
        switch error {
        case DecodingError.typeMismatch(_, let context):
            self.errorMessage = "\(error.localizedDescription): \(context.debugDescription)"
        case DecodingError.valueNotFound(_, let context):
            self.errorMessage = "\(error.localizedDescription): \(context.debugDescription)"
        case DecodingError.keyNotFound(_, let context):
            self.errorMessage = "\(error.localizedDescription): \(context.debugDescription)"
        case DecodingError.dataCorrupted(let key):
            self.errorMessage = "\(error.localizedDescription): \(key)"
        default:
          self.errorMessage = "Error decoding document: \(error.localizedDescription)"
        }
      }
    }
  }

If you wish a dedicated FirestoreDecodingError for the situation where the snapshot can't be decoded and the reason is that the data doesn't exist, then it is fairly easy to accommodate that too by changing the decoding of the snapshot a bit:

  public func data<T: Decodable>(as type: T.Type,
                                 with serverTimestampBehavior: ServerTimestampBehavior = .none,
                                 decoder: Firestore.Decoder? = nil) throws -> T {
    let d = decoder ?? Firestore.Decoder()
    let data = data(with: serverTimestampBehavior)
    do {
      return try d.decode(T.self, from: data ?? NSNull(), in: reference)
    } catch {
      if case DecodingError.valueNotFound = error, data == nil {
        // If the data is nil, and we're expecting a value, then map the error
        // into our own domain.
        throw FirestoreDecodingError.missingValue
      } else {
        throw error
      }
    }
  }

Let me know what you think: @ryanwilson, @paulb777, @schmidt-sebastian

[peterfriese]

1. Docs needs to be cleaned up,
2. I had a question around NSNull()
3. The tests should be updated
4. Adding a sample would be nice

[mortenbekditlevsen]

And here's the async use case version of the code:

  private func fetchBook(documentId: String) async {
    let docRef = db.collection("books").document(documentId)
    do {
      self.book = try await docRef.getDocument(as: Book.self)
      self.errorMessage = nil
    } catch {
      switch error {
      case DecodingError.typeMismatch(_, let context):
        self.errorMessage = "\(error.localizedDescription): \(context.debugDescription)"
      case DecodingError.valueNotFound(_, let context):
        self.errorMessage = "\(error.localizedDescription): \(context.debugDescription)"
      case DecodingError.keyNotFound(_, let context):
        self.errorMessage = "\(error.localizedDescription): \(context.debugDescription)"
      case DecodingError.dataCorrupted(let key):
        self.errorMessage = "\(error.localizedDescription): \(key)"
      default:
        self.errorMessage = "Error decoding document: \(error.localizedDescription)"
      }
    }
  }

[paulb777]

Looks like a nice usability improvement. Thanks @mortenbekditlevsen! What is the impact on migrating code using the existing library?

If others agree, let's try to run this through API review in January.

[mortenbekditlevsen]

If you have code that decodes a snapshot into an entity, the result will no longer be an Optional, so an attempt at unwrapping will fail.

There are two fixes: Inserting a question mark on the decodable type will give you the old behavior: snapshot.data(as: Book?.self)

The other fix - for instance in case the snapshot comes from a collection, meaning that you know the data exists, is to then skip the extra unwrapping. The compiler will likely hint at this since the value is no longer optional.

I should probably sketch out both scenarios in an example to provide a bit of guidance on the change.

[mortenbekditlevsen]

I believe that all requested changes have been addressed. Would you mind a second review, @peterfriese ?

[peterfriese]

I ❤️ the improved usability of this change. However, making T non-optional is a breaking change for all Firestore users. Can we try implementing this in a non-breaking way?

Other than that, a couple of small issues in the docs.

[mortenbekditlevsen]

Hi @peterfriese ,
Thank you for the review! I'll get to fixing the comments ASAP. :-)

Regarding the breaking change:
A way to do it without breaking would be to supply a new API and keep the old with a deprecation warning.
A downside is that we have to come up with a new name for this API. :-)

Second best for users already using the API would be to provide a good guide with an update path.
This is of course only feasible if the extension is still considered to be in beta.
Regarding a guide:
Adding a question mark to your types - i.e. changing .data(as: T.self) -> .data(as: T?.self) would give the behavior of today, but many call sites could be examined to see whether the return type should actually be optional.

What do you think? New API and deprecation of existing - or rely on beta testers to be open to such a change?

[paulb777]

@ryanwilson Any suggestions about the best way to deprecate the current and naming of the new?

[ryanwilson]

What do you think? New API and deprecation of existing - or rely on beta testers to be open to such a change?

@ryanwilson Any suggestions about the best way to deprecate the current and naming of the new?

Great questions, and thanks as always @mortenbekditlevsen for working through this with us.

Since this is beta and we're already at the ideal naming, I'm in favour of making the breaking change and add instructions / an explanation in the CHANGELOG (or a link to instructions).

cc @schmidt-sebastian do you have a preference one way or the other?

[schmidt-sebastian]

We can ship this as a breaking change. There is no reason to have @beta APIs if we don't take advantage of them. Let's just make sure that we always move in the right direction.

[paulb777]

I updated the SwiftFormat we're using in #9239, so please rebase and update. Sorry about the inconvenience.

[mortenbekditlevsen]

I rebased and reacted to the 'changes requested' by requesting a re-review.
Let me know what else I can do to move this forward.

[peterfriese]

Thanks for the changes, Morten!

Here are the next steps:

• Put together an internal API proposal (in progress, internal link: go/firebase-codable-explicit-typing)
• Update the code in my Codable sample project
• Once this is approved, merge the PR
• Update the docs
• Update this blog post (and sample code)
• Reach out the community and make them aware of this breaking change
• Potentially update a bunch of StackOverflow answers I (and others) wrote

I will update this comment as I make progress.

[peterfriese]

Investigated what happens if we force unwrap the error, and if returning nil/nil (a.k.a. "this should never happen") works as expected (although - not sure if something that should never happen can have an expected outcome 😏 )

Some small nits in the docs.

API review was positive!

[peterfriese]

The integration tests in Firestore/Swift/Tests/Integration/CodableIntegrationTests.swift need to be updated as well.

[mortenbekditlevsen]

@peterfriese I updated the code, but in blind as I haven't been able to figure out how to run the codable integration tests.
I ran ./scripts/setup_spm_tests.sh as per the documentation and I got a lot of new test schemes, but I can't find any that suggest they are running integration tests.
If I somehow missed something in my commit, I'd of course love to know how to build and run the tests - and please excuse my ignorance if I am missing something completely obvious. :-)

[peterfriese]

@peterfriese I updated the code, but in blind as I haven't been able to figure out how to run the codable integration tests. I ran ./scripts/setup_spm_tests.sh as per the documentation and I got a lot of new test schemes, but I can't find any that suggest they are running integration tests. If I somehow missed something in my commit, I'd of course love to know how to build and run the tests - and please excuse my ignorance if I am missing something completely obvious. :-)

I had to dig around a bit myself. To run the Firestore tests, you need to

1. go to Firestore/Example
2. Run pod install
3. xed . to open the workspace
4. Chose Firestore_IntegrationTests_iOS in the launch config drop down
5. Find CodableIntegrationTests.swift
6. Launch the Emulator using scripts/run_firestore_emulator.sh
7. Run the tests

[mortenbekditlevsen]

Thanks @peterfriese
After battling a bit with arm64 based openjdk, cocoapods, ruby and versioning of dependencies I got the pods to install. :-)

FYI I got a linker error that the Firestore_Example_iOS couldn't be found, so that appears to be an implicit dependency.

After building the app target I can now compile, link and run the tests successfully. So the previous commit should be good to go. :-)

[paulb777]

Great work! Thanks @mortenbekditlevsen and @peterfriese