Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Require explicit typing for DocumentSnapshot decoding. DocumentReference decoding. #9101

Conversation

mortenbekditlevsen
Copy link
Contributor

@mortenbekditlevsen mortenbekditlevsen commented Dec 15, 2021

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

Copy link
Contributor

@peterfriese peterfriese left a comment

Choose a reason for hiding this comment

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

  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
Copy link
Contributor Author

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)"
      }
    }
  }

Copy link
Member

@paulb777 paulb777 left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

mortenbekditlevsen commented Dec 22, 2021

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
Copy link
Contributor Author

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

Copy link
Contributor

@peterfriese peterfriese left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

mortenbekditlevsen commented Jan 14, 2022

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
Copy link
Member

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

@ryanwilson
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

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

@mortenbekditlevsen
Copy link
Contributor Author

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
Copy link
Contributor

peterfriese commented Jan 28, 2022

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 added a commit to peterfriese/Swift-Firestore-Guide that referenced this pull request Feb 4, 2022
Copy link
Contributor

@peterfriese peterfriese left a comment

Choose a reason for hiding this comment

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

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 added a commit to peterfriese/Swift-Firestore-Guide that referenced this pull request Feb 18, 2022
Signed-off-by: Peter Friese <peter@peterfriese.de>
@peterfriese peterfriese changed the title [WIP] Require explicit typing for DocumentSnapshot decoding. DocumentReference decoding Require explicit typing for DocumentSnapshot decoding. DocumentReference decoding. Feb 18, 2022
@peterfriese peterfriese self-requested a review February 18, 2022 08:57
Copy link
Contributor

@peterfriese peterfriese left a comment

Choose a reason for hiding this comment

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

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

@mortenbekditlevsen
Copy link
Contributor Author

@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
Copy link
Contributor

peterfriese commented Feb 18, 2022

@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
Copy link
Contributor Author

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 paulb777 added this to the 8.13.0 - M112 milestone Feb 18, 2022
Copy link
Member

@paulb777 paulb777 left a comment

Choose a reason for hiding this comment

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

Great work! Thanks @mortenbekditlevsen and @peterfriese

@schmidt-sebastian schmidt-sebastian merged commit a1dde8e into firebase:master Feb 23, 2022
morganchen12 added a commit to firebase/quickstart-ios that referenced this pull request Mar 3, 2022
peterfriese added a commit to peterfriese/Swift-Firestore-Guide that referenced this pull request Mar 7, 2022
Signed-off-by: Peter Friese <peter@peterfriese.de>
@firebase firebase locked and limited conversation to collaborators Mar 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants