Make Firestore use StructureEncoder and StructureDecoder

[mortenbekditlevsen]

Note: this builds on upon the PR #8854 , but uses StructureEncoder and StructureDecoder for Firestore as well.

Benefits:

• Reuse code.
• Users can now use key coding strategies and other features that they know from JSONEncoder and JSONDecoder.

[mortenbekditlevsen]

Note: I thought a bit about this, and there is currently an error in the implementation.
Currently I made a PassthroughType protocol that types can opt-in to. But the point is that it should only be passed through when encoding to and decoding from Firestore. For other codable situations the types should not be passed through.
This means that it would probably be better to define a list of passthrough types as an entry in the userInfo dict, so that it can be used at the correct places and not elsewhere.

[schmidt-sebastian]

@mortenbekditlevsen Thanks for putting this together. I will spend the next couple of days looking at this and should be able to figure out whether this works for us by the end of next week.

[mortenbekditlevsen]

Hi @schmidt-sebastian ,
Thank you for looking at this
As I mentioned, the pass through types really only ought to be preserved during encoding when explicitly opting in to do so (in combination with using Firestore). I've added a commit to the PR to express this.
During decoding it's ok to have the check because the structures that will be passed in to this (in combination with RTDB and Functions) will not be containing the passthrough types.

[mortenbekditlevsen]

The latest version uses a combination of a boolean flag and protocol conformance to the encoder to signify pass through types.
Thinking about this it may be cleaner to just pass a function to the encoder/decoder that exactly matches the isFirestorePassthroughType from the existing implementation.

This, however, is not simple to do due to the generic argument of the isFirestorePassthroughType. Basically it's hard to express this generic parameter on a reassignable closure parameter. So I guess some type erasure would be needed to implement this.

Let me know if you think that the current implementation is ok or if we need something better.

[schmidt-sebastian]

Can you quickly explain when this would be used in Firestore since we do not use JSON-formatted dates?

[mortenbekditlevsen]

Sure - no other reason than: someone might have a different preference then encoding to / decoding from the Timestamp type.

If I wish to represent my date as the number of seconds since epoch or as a string specifying week-year and week of the date: "2021-44" or as ISO 8601 or anything else, then I don't think that the API should force me to use Timestamp.

If you disagree then this can of course just be removed.

[schmidt-sebastian]

If I understand this correctly, this allows user to use String-based dates in their Codable objects, but we will store them as Timestamps? If so, then this makes sense.

[mortenbekditlevsen]

As this is in the direction of decoding it's more like:
If you have a Date in you encodable model and a Timestamp in you Firestore document, then the decoding will map the Timestamp to the Date. This step is to be API compatible with functionality that is already a part of the existing Firestore.Decoder. But it also lets you express a date decoding strategy to use when the Firestore document is not a Timestamp.
Perhaps you might always store iso8601 encoded date strings in your Firestore document - or you might be storing seconds or miliseconds since epoch. For whatever reasons - historical, compatibility with another framework, etc., etc.
So this just allows the exact same configurability that users may be used to from JSONDecoder while still being compatible with the existing internals of Firestore.Decoder that would always automatically decode a Timestamp to a Date if that was requested by the user.

This strategy should however be internal instead of public since it's used internally and is just there to ensure backwards compatibility with the existing Firestore.Decoder.

[schmidt-sebastian]

Is there a reason why this would not be turned by default (or always)? The only issue I see is that some of the Firestore types might be treated as Passthrough types when used in RTDB and Functions. This however is already an error condition.

[mortenbekditlevsen]

Yes, exactly so that you could use the same models with Firestore, RTDB and callable functions:

struct Model: Codable {
   var coord: GeoPoint
}

Would encode as a native GeoPoint in Firestore, but use the perfectly valid Codable implementation of GeoPoint for RTDB and functions.

[schmidt-sebastian]

Some quick notes. I need to do more testing to see if this supports all existing use cases.

[schmidt-sebastian]

Do you think there is a different way we could handle this? This does not seem very discoverable.

[mortenbekditlevsen]

My hopes were that the end user does not have any need for discovering this, as it would be an implementation detail of the Firestore Codable additions.

Don't you think that it's realistic that the Codable overlays for swift provided by the SDK were sufficient, so that end users would never directly decode structures from Firestore 'manually'?

As a side note, I really think that the @DocumentID property wrapper (which the above aims to support) is an anti-pattern, as it ties your model to a very specific use case - but of course it must be supported here as it's already part of the API.

[schmidt-sebastian]

FWIW, I think the updated protocol-based implementation is more maintainable than what we had before.

[mortenbekditlevsen]

Thanks.
The only thing that bothers me is that conforming to a protocol states globally that you are a thing that needs to be left alone during structure encoding and decoding.
In actuality it's probably more likely that it's tied to the specific underlying framework (Firestore) - and thus not really a global opt-in. This is of course why I added the boolean flag to opt-in to this behavior in general.

I think it would be a cleaner solution to be able to have separate passthrough types for separate use cases. Thus going back to something like the previous function instead of conforming to a type.
As closures in Swift can't be generic (or at least you need to know the generic parameter when assigning the closure to a property), then it's not possible to pass a 'passthroughTypes()' function directly to the encoder/decoder, but I think you could create a protocol with a requirement of a generic function and then wrap the function in a struct conforming to the closure.

I haven't tried it, but something like:

protocol StructureCodingPassthroughTypeResolver {
   func isPassthroughType<T: Any>(_ t: T) -> Bool
}
struct FirestorePassthroughTypeResolver: StructureCodingPassthroughTypeResolver {
 ...
}

Then again: perhaps such a design is overkill since the only use case for the passthrough types is currently Firestore...

What do you think?

[mortenbekditlevsen]

The function requirement of the protocol could even be static, so that no actual instance of a resolver needs to be passed around, just the type...

[schmidt-sebastian]

Do you think we might be able to expand on the id of the StructureCodingPassthroughTypeResolver and make it handle both the @DcoumentId annotation as well as the Passthrough types? We could call the new TypeResolver for every type and can optionally implement custom encoding.

protocol TypeResolver {
  func encode<T : Encodable>(_ value: T, forKey key: Key) : Optional<Data>;
  func decode<T : Decodable>(_ type: T.Type, forKey key: Key) : Decode<T>;
}

struct FirestoreTypeResolver: TypeResolver {
  func encode<T : Encodable>(_ value: T, forKey key: Key) : Optional<Data> {
    // Handles DocumenID, GeoPoint, Timestamp, DocumentReference. Returns Optional.none for all other types
   }

  ...
}

The DocumentReference can then be passes to FirestoreTypeResolver, which means that we no longer need to add it to userInfo.

I am not sure how feasible this is without implementing it first, but you might some more expertise here.

[mortenbekditlevsen]

I've tried looking at it, and I am not certain that the two concepts can be bridged.

The passthrough type checking is being used in the box/unbox methods of the encoder/decoder and thus don't have access to any keys.

Keys are actually also ignored in the documentid decoding (since the point is that there is no data at the currently parsed level in the input, that corresponds to the id - it comes from somewhere else).

So the signatures should perhaps rather be encode<T>(_ value: T): Optional<T> and similar for decode - to allow passing types through. So basically the existing boolean function check converted to an optional of the input...

But for decoding DocumentID this type signature is not really a fit since there's no 'DocumentID' value to pass along to the decoding...

---

I did try implementing a passthrough type resolver - and I enhanced discoverability a little by introducing a public func decode<T: Decodable>(_ t: T.Type, from data: Any, in reference: DocumentReference)
overload on Firestore.Decoder (aka StructureDecoder).

I updated the PR branch with the new refactor. Let me know what you think.

[mortenbekditlevsen]

I just had a different thought:
Instead of Firestore.Decoder being a typealias for StructureDecoder (and ditto encoder), it could be a really thin wrapper, so that the correct configuration is always guaranteed.
What do you think about that idea?

[mortenbekditlevsen]

In the latest version of the PR I tried implementing the lightweight wrapper types around the encoder and decoder, and that does indeed give a much nicer API at the use point.

[schmidt-sebastian]

Can this be FirebaseEncoderSwift? Reason I ask is because we already have FirebaseCommon, so in some ways it would make sense to rename this to FirebaseCommonSwift... with the exception that we do not want to include this code for users that do not use RTDB, Functions or Firestore. So maybe the naming should indicate that this is Encoding specific rather than a shared Swift library.

[schmidt-sebastian]

I am also not able to build from your branch as this module cannot be found. Do you have a PR that includes a Podspec?

[mortenbekditlevsen]

Sure, I don't have any preference for the naming. Should I go ahead and rename?

With regards to building, I am not familiar with the CocoaPods setup, so initially I've just added support for SwiftPM. I think @paulb777 mentioned that the podspec could perhaps be added later by one more knowledgeable than me in this area. :-)

[schmidt-sebastian]

Yeah, I think we should rename. I will see if I can put together a Podspec.

[schmidt-sebastian]

I created a diff with the Podspec: https://gist.github.com/schmidt-sebastian/a3df8aac27067ef572351627664ba175

Let me know if you can apply this, otherwise I will fork your repo and create a PR.

[mortenbekditlevsen]

Thanks - it's now applied and pushed. :-)

[schmidt-sebastian]

I also need the Podspec for FirebaseFunctionsSwift: https://gist.github.com/schmidt-sebastian/8b726a52540c972a2e083d460a51f954

[schmidt-sebastian]

Some quick notes. I need to do more testing to see if this supports all existing use cases.

[mortenbekditlevsen]

I have a small question: In the existing Firestore encoding tests, I can see that a Date always roundtrips to Date after encode and decode.
Now that date encoding strategies are available, wouldn't it be nice to have Date encode to Timestamp using such a strategy?
This would be a new, opt-in feature.

[mortenbekditlevsen]

I have a small question: In the existing Firestore encoding tests, I can see that a Date always roundtrips to Date after encode and decode.
Now that date encoding strategies are available, wouldn't it be nice to have Date encode to Timestamp using such a strategy?
This would be a new, opt-in feature.

I took the liberty to implement this strategy in the latest version of this PR.

[schmidt-sebastian]

I have a small question: In the existing Firestore encoding tests, I can see that a Date always roundtrips to Date after encode and decode.
Now that date encoding strategies are available, wouldn't it be nice to have Date encode to Timestamp using such a strategy?
This would be a new, opt-in feature.

I took the liberty to implement this strategy in the latest version of this PR.

Are we not able to to determine what type we need to convert to based on what we find in the Codable object?

I will probably add more tests to this PR to make sure that we do not break anything.

[mortenbekditlevsen]

Yes, but if you models in general contains Date entities (like for instance if you don't want to tie your models to Firestore) - but you'd still prefer that those dates were represented as Timestamps when used with Firestore, then you can now just use the .timestamp date encoding strategy.

[schmidt-sebastian]

Can we drop resultAs? The compiler should be able to infer the argument from the Result type.

[mortenbekditlevsen]

So all of the functions-relate code is already part of PR #8854 - apologies for including the same code here too, but I developed it in three steps - first the refactor to a shared module, then the functions part and finally the firestore part.

But you're completely right - the compiler is indeed able to infer the type. However, there is precedence for including the type used for decoding - in both Apple's JSONDecoder and also in the existing Firestore swift overlays:

https://developer.apple.com/documentation/foundation/jsondecoder/2895189-decode

Meaning that the API lets you do:

let a = try decoder.decode(GroceryProduct.self, from: json)

rather than:

let a: GroceryProduct = try decoder.decode(from: json)

And similar in the Firestore overlays:
https://github.com/firebase/firebase-ios-sdk/blob/master/Firestore/Swift/Source/Codable/DocumentSnapshot%2BReadDecodable.swift

  public func data<T: Decodable>(as type: T.Type,
                                 with serverTimestampBehavior: ServerTimestampBehavior = .none,
                                 decoder: Firestore.Decoder? = nil) throws -> T? {
...
  }

which is used as:

let a = try snapshot.data(as: GroceryProduct.self)

So even in the very direct API there's precedence for including the type - and when using a callback that has even more value, because otherwise the end user would be forced to spell out the type inside of the callback closure, which makes them quite verbose.

Note also, that in the latest commit on PR #8854, this is where I moved the types into the function that returns the callable - meaning that once you have defined your callable, the types are directly represented by that callable, meaning that you neither have to spell out the request or response types at the call site, and you get compiler validation that you only use the callable with input data of the request type.

[paulb777]

Closing in favor of #9465