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

Add support for object serialization in Firestore #627

Closed
ramunasjurgilas opened this issue Jan 8, 2018 · 43 comments
Closed

Add support for object serialization in Firestore #627

ramunasjurgilas opened this issue Jan 8, 2018 · 43 comments

Comments

@ramunasjurgilas
Copy link

@ramunasjurgilas ramunasjurgilas commented Jan 8, 2018

Currently getDocuments function do not supports Codable/Decodable protocol ( It is Swift 4 biggest advantage). Currently I need to write wrapper which uses Codable protocols. Below is example (Example: 2) how I did it. Without using Codable/Decodable I would need to pars responses in old way using dictionary manner as shown in example: 1. It is overkill do not use Codable protocol.
My question is what stop Firebase engineers add Codable advantages to SDK?

// Example 1
query.getDocuments(completion: { (snapshot, error) in
            // .... 
            snapshot.documents.forEach({ (document) in
                let name = document.data()["name"]
                let street = document.data()["street"]
                // And so on ...
                let model = Person(name: name, street: street)
                self.models.append(model)
            })
        })
// Example 2
class FirestoreFetcher<T> where T: Decodable {

    private var models = [T]()
    private var modelsSnapshot = [DocumentSnapshot]()
    private var query: Query?
    
    func requestUsing(_ query: Query) {
        query.getDocuments(completion: { (snapshot, error) in
            guard let snapshot = snapshot else {
                print("Error fetching snapshot results: \(error!)")
                return
            }
            snapshot.documents.forEach({ (document) in
                let result = document.data().convert(T.self)
                if let model = result.object {
                    self.models.append(model)
                }
                else {
                    print("Error on converting to generics: \(document.data())")
                }
            })
            self.modelsSnapshot += snapshot.documents
        })
    }
}
@ramunasjurgilas
Copy link
Author

@ramunasjurgilas ramunasjurgilas commented Jan 8, 2018

@firebase-oss-bot I did modified my post to get more clear.

@wilhuff
Copy link
Contributor

@wilhuff wilhuff commented Jan 8, 2018

Hi! Thanks for writing in.

Due to the way Firestore is released (as a part of the Firebase iOS SDK, built by Google) the Firestore client library is still an Objective-C project. We include annotation macros (e.g. NS_SWIFT_NAME) for Swift compatibility but don't yet have a way to release an extension library that would implement Swift-only types like Encodable or Decodable.

There should be nothing that stops you from declaring your own extensions though, something like

extension DocumentSnapshot: Decodable {
  init(from: Decoder) {
    // ...
  }
}

However before you do that, what benefit are you hoping to get from using Codable objects? We are looking into being able to build swift extensions but it's not going to happen anytime in the short term. If there's a really good use case for this it could help us prioritize this work.

@wilhuff wilhuff closed this as completed Jan 8, 2018
@ramunasjurgilas
Copy link
Author

@ramunasjurgilas ramunasjurgilas commented Jan 10, 2018

Hello. Thanks for response.

Unfortunately currently 'Decodable' cannot be automatically synthesized. DocumentSnaphot can not inherit from Decodable. After doing it I am getting this error:

error: PG.playground:7:1: error: implementation of 'Decodable' cannot be automatically synthesized in an extension yet
extension DocumentSnapshot: Decodable 

@wilhuff
Copy link
Contributor

@wilhuff wilhuff commented Jan 10, 2018

Right you'd need to implement init(from: Decoder) for yourself rather than relying on automatic synthesis.

The first problem you'll encounter though is that DocumentSnapshots contain types that are tied to the Firestore instance that created them (e.g. DocumentReference). The snapshot itself can navigate back to a document reference too. This means snapshots don't really exist disconnected from the instance.

So outside of some really compelling use case this doesn't seem like a feature we'd add. Why are you trying to make snapshots Codable?

@ramunasjurgilas
Copy link
Author

@ramunasjurgilas ramunasjurgilas commented Jan 16, 2018

Thanks for your answer. Here is two reasons to use Codable:

  1. I would not need to write code for parsing dictionary. All stuff is done by Codable protocol.
  2. Adapt modern implementation.

@datureezy
Copy link

@datureezy datureezy commented Jan 25, 2018

I have to agree with @ramunasjurgilas about adapting to modern implementation.

Decodable/encodable was all the rage at WWDC 2017. Sure, we can live without it, but the whole thing with Swift is that it's supposed to be beautiful to a certain extent. And manually decoding JSON just adds a bunch of bloat.

Still love Firebase tho.

@ghost
Copy link

@ghost ghost commented Feb 3, 2018

I agree, I am learning how to code, and it is infuriating knowing that the new releases (codable, MusicKit) are the future but I am still having to first learn the old ways since there is not adequate documentation and also lag in implementation in situations like these.

@datureezy
Copy link

@datureezy datureezy commented Feb 3, 2018

@wilhuff
Copy link
Contributor

@wilhuff wilhuff commented Feb 5, 2018

I think I misunderstood the original request here. It's not that you want DocumentSnapshots themselves to be Codable/Decodable (since that would only give you the ability to serialize them to and from bytes which isn't useful).

What this is about is that you want an Encoder and Decoder for Snapshots that would map between your custom objects and snapshots.

We've had something like this on our radar for a bit. Essentially we want to provide an equivalent to the Android DocumentSnapshot.toObject.

@wilhuff wilhuff reopened this Feb 5, 2018
@firebase firebase deleted a comment from google-oss-bot Feb 5, 2018
@morganchen12 morganchen12 changed the title Support for Codable in Firestore getDocuments needs to be added Add support for object serialization in Firestore Feb 9, 2018
@alickbass
Copy link

@alickbass alickbass commented Feb 14, 2018

Hi guys! Maybe it's a bit off-topic, but I have written a small library to decode the Firestore's data() to Model using Codable. I would love to hear your feedback.

@wilhuff
Copy link
Contributor

@wilhuff wilhuff commented Feb 14, 2018

That's not off topic at all, and very cool! Would you be willing to work with us to incorporate something similar directly into the Firestore API?

@ramunasjurgilas
Copy link
Author

@ramunasjurgilas ramunasjurgilas commented Feb 15, 2018

@alickbass Myself started to write something like this. Thanks for sharing. It will save a lot of time. I could look into your library.

@alickbass
Copy link

@alickbass alickbass commented Feb 15, 2018

@wilhuff Sure! I would love to contribute! Maybe we could start a separate thread and discuss the details for the design and ideas on how to incorporate it into Firestore API?

@alickbass
Copy link

@alickbass alickbass commented Feb 16, 2018

@wilhuff where would be an appropriate place to start the discussion? Is it here in github issues or in Firebase talk google group?

@morganchen12
Copy link
Contributor

@morganchen12 morganchen12 commented Feb 16, 2018

Here is fine.

@alickbass
Copy link

@alickbass alickbass commented Feb 17, 2018

Okay guys, here are several things I would like to discuss:

  1. Do we want to add the support Codable only for Firestore? or the the Database as well? In my library I support both, because the only difference is how to handle Date, Data and other types like DocumentReference, GeoPoint, FieldValue. For the Date and Data in the Database I use the same strategy as the JSON Decoder/Encoder from Foundation.
  2. As for the DocumentReference, GeoPoint and FieldValue, I use a protocols FirestoreDecodable and FirestoreEncodable that are used to indicate the types are encoded by Firestore. I wanted to keep my library free from dependencies so I used these protocols, however, as we will have access directly to these types, we can hardcode them in the Decoder/Encoder. However, all these types need to conform to Codable protocol. In the case of GeoPoint, we could encode it as a dictionary with latitude and longitude if you try to use it with some other Decoders, like JSONDecoder or PlistDecoder. DocumentReference and FieldValue could by default just throw an Error if you use them with some other Decoder. I have already implemented it in my library and you can see the implementation here
  3. Do we want to make Decoder and Encoder to the public? Or should it just be internal?
  4. I based my Encoders and Decoders on the Foundation's JSONDecoder and PropertyListEncoder. However, the test coverage is not perfect there... Now in my library it is 57,28% according to Xcode's code coverage. I would probably need some help in writing more extensive tests for the Encoders.
  5. There are some interesting things that we can do with FieldValue. Some discussions started here. We could include something similar right away or we could leave to developers, but in my opinion, people would benefit a lot from such thing

Once we clarify all these points, I could draft a PR with the initial proposal 🙃

@wilhuff
Copy link
Contributor

@wilhuff wilhuff commented Feb 17, 2018

Broadly my goal for this would be to add to the surface API of Firestore such that something like the following becomes possible:

class Model : Codable {
}

// read
let snap: DocumentSnapshot = //
let model = snap.data(as:Model.self)

// write
let ref: DocumentReference = //
ref.setData(from:model) { error? in /**/ }

The equivalent API that already exists on Android looks like this:

// read
DocumentSnapshot snap = // ...
Model model = snap.toObject(Model.class);  // inferred type T

// write
DocumentReference ref = // ...
ref.set(model);

The benefit of building this into the APIs is that the result of the encode/decode is not specified giving us the opportunity to optimize this later.

Note that the dictionary form we accept and emit is temporary. We convert to an internal form immediately so keeping the encoder internal allows us to eventually convert directly to the internal form.

However, I wouldn't recommend attempting that now because we're rewriting the internals of the client in C++ (to make it easier to directly support Unity and desktop use). The internal representation is currently FSTObjectValue but we're migrating to model::FieldValue.

So if you accept this as a reasonable direction I think this leads to some answers to your questions:

  1. While there's some initial benefit to sharing, if we move to direct conversion there won't be as much shared code. On the flip side we could put common code in a shared pod, since most of the differences are in the boxing/unboxing. This is a decision we can defer somewhat, starting with Firestore and then migrating common code out later.

  2. I think we can make types like GeoPoint conform to Codable directly. The default serialization behavior you get with other encoders is probably fine for that type. DocumentReference is trickier and failing in that case may be warranted just to leave us the option to do this better in the future.

  3. As noted above, I think there's a benefit to making these internal. Additionally because the API surface is smaller it will be far easier to get this through Firebase API approval (though this is my problem, not yours).

  4. This is an area where we can work together. The Android client's class mapper has pretty extensive tests that we could use as a pattern here. Starting there may go some ways toward harmonizing corner case behavior across the platforms. (The Android client is not (yet) open source but we can certainly share this.)

  5. FieldValue is interesting. In the Android port we handle these with annotations. A model that requests a server timestamp could look like this:

    public class Model {
      @ServerTimestamp
      public Date timestamp;
    }

    When encoding, if timestamp is null then the value is taken to be FieldValue.serverTimestamp().

    Unfortunately, Swift does not have an equivalent and I don't think CodingKeys can be bent to this purpose either. We could potentially take an approach similar to CodingKeys though, perhaps using a nested FieldValueKeys enumeration. I don't think we should end up with users writing their own encode(to:Encoder) and init(from:Decoder) to support these types. This is likely to be an area where some research is warranted. @ryanwilson WDYT?

Another area we have to figure out is exactly how to build this. Firebase still supports users using Xcode 8 and Objective-C-only clients so this probably needs to be in a non-default subspec of the FirebaseFirestore pod. The intersection of this and how we ship our production (pre-built) pods creates an interesting problem to solve but I think it's tractable. I'll get back to you on this part.

@alickbass
Copy link

@alickbass alickbass commented Feb 17, 2018

Thanks @wilhuff for such a quick reply! I think I have quite some information to start working on some initial proposal. I will take your desired code as a starting point and will submit a PR. I will start with using dictionary as before and once there will be more stable internal API, we could rewrite it to direct internal form.

As for the version of swift, I will add annotations to make it available only from swift version 4, so it shouldn't be a problem.

For now I will leave aside the FieldValue, and we can come back to the improved design later. I could also suggest couple of ideas here 😉

@wilhuff
Copy link
Contributor

@wilhuff wilhuff commented Feb 17, 2018

I'd suggest actually that the very last piece we want is the top-level API changes. These commit us to shipping the feature and will be blocked on API review. We can make a number of smaller commits to master before that point otherwise.

@alickbass
Copy link

@alickbass alickbass commented Feb 19, 2018

Hi @wilhuff and @morganchen12 ! So I have started working on it already, however, I immediately bumped into issues with adding a Swift file to the Firestore static library source code. It seems like from Xcode 9 static libraries can now have mixed Objective-C and Swift source code. However as I found here it requires having a bridging header. TBH, I can't create a working bridging header and I get errors like "Failed to import bridging header". I have already switched Defines module to YES and swift version to 4. Do you know what can be a problem?

@paulb777
Copy link
Member

@paulb777 paulb777 commented Feb 19, 2018

Hi @alickbass It's unfortunate that there is not yet proper CocoaPods support for mixed languages in static frameworks. Hopefully we'll get that added in the next few months.

In the meantime, the bridging header workaround should work. Do you understand what is different from the project described there? If you get stuck, please share a branch that demonstrates the problem and I'll take a look this week.

Alternatively, if you want to avoid dealing with an immature part of CocoaPods, you could do your implementation as a separate FirebaseFirestoreSwift CocoaPod that depends on FirebaseFirestore.

@MauriceArikoglu
Copy link

@MauriceArikoglu MauriceArikoglu commented Jun 21, 2019

@wilhuff Any news? ABI stability is there in the mean time.

@wilhuff
Copy link
Contributor

@wilhuff wilhuff commented Jun 24, 2019

There's been some progress on this (see #3198 and #3231). We're not yet ready for a release but this is coming along.

It's possible to kick the tires on this now if you're really enterprising, but we don't have instructions written up for how to do that yet.

As a side note, ABI stability going forward is good but it's going to be quite some time before we can require e.g. iOS 12.2 as the minimum supported version.

@MauriceArikoglu
Copy link

@MauriceArikoglu MauriceArikoglu commented Jul 10, 2019

@wilhuff Please ping me if there are open tickets that I could help you with.

Also on a side note: How feasible do you think would it be to maintain a 12.2 targeted Swift-only Framework? Like the one @alickbass started working on?

@wilhuff
Copy link
Contributor

@wilhuff wilhuff commented Jul 10, 2019

Nearly everything required to make this work has landed. We're still wrangling a few internal things and haven't yet tested on anything but iOS, but you can try it out. Try adding this to your Podfile:

pod 'FirebaseFirestoreSwift', :podspec => 'https://raw.githubusercontent.com/firebase/firebase-ios-sdk/master/FirebaseFirestoreSwift.podspec'

In any Swift source where you want to use this add:

import FirebaseFirestoreSwift

We have no docs yet, so the swift source is really all you'll have to go on.

Let us know how it goes!

@MauriceArikoglu
Copy link

@MauriceArikoglu MauriceArikoglu commented Jul 16, 2019

@wilhuff cool, I'll give it a try as soon as I get the chance. Will report

@moflo
Copy link

@moflo moflo commented Aug 5, 2019

@wilhuff thanks for this. I'm having trouble with the pod installation, the podspec doesn't seem to match the proper tag/branch. Can you please update?

warning: Could not find remote branch 0.1 to clone.

@wilhuff
Copy link
Contributor

@wilhuff wilhuff commented Aug 5, 2019

That's weird! I see that too.

Try adding this instead:

pod 'FirebaseFirestoreSwift', :git => 'https://github.com/firebase/firebase-ios-sdk.git', :branch => 'master'

@grennis
Copy link
Contributor

@grennis grennis commented Aug 14, 2019

@wilhuff Thanks, this looks to be working well. I did have an issue that a Date object in my model class cannot decode because of the error about expecting a Date but found FIRTimestamp instead. I know I can fix this (for now) by using setting areTimestampsInSnapshotsEnabled but this is deprecated and apparently going away soon. And, I don't want to add a Firebase reference to put a Firebase Timestamp data type in my model class.
Do you think it will be possible to update Decoder unbox to also check for a Timestamp and read the dateValue from that?
Thanks!

@mikelehen
Copy link
Contributor

@mikelehen mikelehen commented Aug 15, 2019

@grennis Thanks very much for giving this a spin and contributing that bugfix! Holler if you have any other feedback or issues.

@grennis
Copy link
Contributor

@grennis grennis commented Aug 16, 2019

@mikelehen Will do. So far everything else is working well for me. Thanks!

@tomirons
Copy link

@tomirons tomirons commented Aug 16, 2019

@wilhuff Is it possible to decode the reference ID? I see the DocumentReference but I'm not sure how to use it properly.

@wilhuff
Copy link
Contributor

@wilhuff wilhuff commented Aug 16, 2019

Before we talk about anything else: DocumentReference is for carrying references to other documents within some document. So for example, if you had a document representing a specific game it might contain a document reference for each player in the game. Your code could load the game and then use the embedded document references to load the players playing the game.

With regard to Codable though it doesn't sound like you're asking about this. It sounds like you're asking to be able to recover the ID of the document itself?

If so, we haven't implemented this yet because it's tricky. By default Codable expects there to be a correspondence between the input values--the Firestore document data--and the resulting Codable object. This is a case were we'll have to add something to carry that out of band.

Would a DocumentId class that works a little like ServerTimestamp work? (The difference being that it would be output-only.) The idea would be that you add a DocumentId member and the FirestoreDecoder would populate it on output?

@tomirons
Copy link

@tomirons tomirons commented Aug 16, 2019

Yeah, I'm looking for ID of the document after it's been created/retrieved.

I think your idea to solve that issue is great.v :)

@ramunasjurgilas
Copy link
Author

@ramunasjurgilas ramunasjurgilas commented Sep 30, 2019

@wilhuff Thanks for implementing FirebaseFirestoreSwift to support Codable protocol.

From my understanding here is missing addSnapshotListenerWithIncludeMetadataChanges function. This function could support Encodable protocol as well.

- (id<FIRListenerRegistration>)
addSnapshotListenerWithIncludeMetadataChanges:(BOOL)includeMetadataChanges
                                     listener:(FIRQuerySnapshotBlock)listener
    NS_SWIFT_NAME(addSnapshotListener(includeMetadataChanges:listener:));

@wilhuff
Copy link
Contributor

@wilhuff wilhuff commented Sep 30, 2019

The way this works is that you can ask the snapshots you receive from a listener to convert themselves to a codable object. So, for example, if you have a type MyCodableType, you get its value out like so:

let docRef: DocumentReference = // ...

docRef.addSnapshotListener { snapshot, error in
  // handle error
  let value = snapshot.data(as: MyCodableType.self)
}

@paulb777 paulb777 assigned wilhuff and unassigned paulb777 Oct 1, 2019
@ZaidPathan
Copy link

@ZaidPathan ZaidPathan commented Oct 6, 2019

@slk333
Copy link

@slk333 slk333 commented Oct 11, 2019

can't wait for this feature to become available in the main pod, codable support sounds very important. Thanks for the work!

@wilhuff
Copy link
Contributor

@wilhuff wilhuff commented Oct 22, 2019

FirebaseFirestoreSwift 0.2 is now available via CocoaPods! Add it to your Podfile alongside existing Firebase pods, like so:

pod 'Firebase/Core'
pod 'Firebase/Firestore'
pod 'FirebaseFirestoreSwift'

This is a separate module, so importing Firestore with Codable support works like this:

import Firebase
import FirebaseFirestoreSwift

If you're using Swift 5.1 (from Xcode 11) this includes several property wrappers to help make reading and writing documents easier:

  • Mark a String or DocumentReference with @DocumentID to have Firestore populate the document's location into your object when reading.
  • Mark a Timestamp or Date with @ServerTimestamp to have Firestore populate a server-generated timestamp when writing (equivalent to FieldValue.serverTimestamp()).
  • Mark an optional field with @ExplicitNull to have Firestore write null if the value is unset.

If you're upgrading from the pre-release FirebaseFirestoreSwift (version 0.1) a few things have changed:

  • The ExplicitNull class has been renamed Swift4ExplicitNull and marked deprecated. The @ExplicitNull property wrapper is its replacement.
  • The ServerTimestamp class has been renamed Swift4ServerTimestamp and marked deprecated. The @ServerTimestamp property wrapper is its replacement.
  • There is no equivalent of @DocumentID for Swift versions prior to 5.1.

Try it out and let us know how it goes!

@wilhuff wilhuff closed this as completed Oct 22, 2019
@jgoodrick
Copy link

@jgoodrick jgoodrick commented Nov 21, 2019

I'm not sure if this is the right place to bring this up, as I am still getting used to contributing to GitHub, but I notice that the data method now returns an optional, even for QueryDocumentSnapshots, which are guaranteed to have a data object. If the method already throws, why does it need to return an optional? I implemented a fix in my own project by creating an extension on QueryDocumentSnapshot itself, and adding this method:

public func data<T: Decodable>(decodedAs type: T.Type,
decoder: Firestore.Decoder? = nil) throws -> T {
let d = decoder ?? Firestore.Decoder()
return try d.decode(T.self, from: data(), in: reference)
}

However, I feel like it would be cleaner if it was able to use the same data(as: ...) signature as is provided for DocumentSnapshot, and I cannot do that because the compiler tells me that overriding methods in extensions is not supported. That said, not having to unwrap something that will never be nil seems worthwhile to me, but I honestly understand if I'm alone on that.

@wilhuff
Copy link
Contributor

@wilhuff wilhuff commented Nov 21, 2019

The reason we don’t provide that override of the return type is for exactly the reason you’re seeing. Extensions are limited in what they can do and this is one area where our approach of extending the Objective-C SDK leads to a suboptimal result.

Unfortunately there isn’t anything we can do to fix it short of adding another method.

@firebase firebase locked and limited conversation to collaborators Nov 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests