[WIP] FirebaseFunctionsSwift

[mortenbekditlevsen]

A possible implementation of #8528
Builds on top of a previous PR #8853 , so that should be merged before this one.

Still needs testing and verification that it actually works.

[paulb777]

Now rebased on codable-refactor branch which includes the merged #8853

[mortenbekditlevsen]

We could also go for something a little more opinionated.
In my own code I like to create type safety around things that are stringly typed as follows:

public protocol CallableType {
    associatedtype RequestType: Encodable
    associatedtype ResponseType: Decodable
    static var name: String { get }
}

public struct Callable<C: CallableType> {
    private let httpsCallable: HTTPSCallable
    internal init(_ httpsCallable: HTTPSCallable) {
        self.httpsCallable = httpsCallable
    }
    public func call(_ data: C.RequestType,
              encoder: StructureEncoder = StructureEncoder(),
              decoder: StructureDecoder = StructureDecoder(),
              completion: @escaping (Result<C.ResponseType, Error>)
                -> Void) throws {
        try httpsCallable.call(data,
                               resultAs: C.ResponseType.self,
                               encoder: encoder,
                               decoder: decoder,
                               completion: completion)
    }

#if compiler(>=5.5) && canImport(_Concurrency)
  @available(iOS 15, tvOS 15, macOS 12, watchOS 8, *)
    public func call(_ data: C.RequestType,
                   encoder: StructureEncoder = StructureEncoder(),
                   decoder: StructureDecoder =
                     StructureDecoder()) async throws -> C.ResponseType {
        try await httpsCallable.call(data, resultAs: C.ResponseType.self)
  }
#endif

}

extension Functions {
    public func httpsCallable<C: CallableType>(_: C.Type) -> Callable<C> {
        Callable<C>(self.httpsCallable(C.name))
    }
}

The use would be:

struct CalculatePiRequest: Encodable {
    var numberOfDecimals: Int
}

struct CalculatePiResponse: Decodable {
    var pi: String
    var executionTime: TimeInterval
}

struct CalculatePi: CallableType {
    typealias RequestType = CalculatePiRequest
    typealias ResponseType = CalculatePiResponse
    static var name: String = "calculatePi"
}

func usercode() async throws {
    let callable = Functions.functions().httpsCallable(CalculatePi.self)
    let piResponse = try await callable.call(a)
}

This means that the callable above will only accept CalculatePiRequest as input and return a CalculatePiResponse.

If this is a style you like, then that could either be added to the current PR - or the other call implementation could even be removed in order to encourage using this more type safe API.

Let me know what you think. And also about the naming (CallableType, Callable, RequestType, ResponseType).

[schmidt-sebastian]

I am new to the party here and just started looking at this.

I wonder if you have considered adding this API to Functions instead? This would then look as such:

let callable = functions.httpsCallable("HelloWorld", encoder: Foo, decoder: Bar);
callable.call(data);

This follows the pattern that Firestore uses on the Web and prescribes the schema for all functions call. You can see how we specify a schema for a Firestore collection here: https://firebase.google.com/docs/reference/js/firestore_.collectionreference.md#collectionreferencewithconverter

[mortenbekditlevsen]

Hi @schmidt-sebastian ,
Thank you for the suggestion - I think that API strikes a really nice balance between ease of use preventing unintended types at the call site.

I'll go ahead and update the PR and close the related discussion thread.

Thanks again, Sebastian!

[schmidt-sebastian]

I am not sure I fully understand the last update. I was hoping we could get rid of the "encoder" and "decoder" from cal() and just pass it to HTTPSCallable, which would then use the same encoder/decoder for every function call. Let me know if I am missing something here.

[mortenbekditlevsen]

Sorry, I misunderstood your suggestion then. The reference to the Firestore js api where you send a transform along made me think that you meant the encodable and decodable types (since they basically correspond to transforms between a serialized representation and an entity).
I can of course bring in the encoder and decoder at this point in the api too.

[schmidt-sebastian]

The main idea behind this is that I think the encoder and decoder are more tied to the Function rather than the Function invocation - and they should be the same for all invocations.

[peterfriese]

I am in favour of configuring the encoder / decoder on httpsCallable.

Morten - you mentioned "modifying the state of the encoder/decoder" - when would a user do this?
Also, I guess thread-safety is a concern for any code that uses an encoder/decoder, so telling the users that if they plan to re-configure the encoder/decoder instance, they need to do so in a thread-safe way. As Sebastian mentioned, most users should be fine with the defaults (at least we should try to make sure they can), so anyone using a custom encoder/decoder should hopefully know what they're getting themselves into :-)

[mortenbekditlevsen]

Hi @peterfriese ,
Likely they would never do that - although userInfo could be used together with an encodable or decodable entity for some custom purpose.

And you're right that this basically the same as other usages of encoders and decoders - but there is one difference:
The httpCallable is specifically intended for longer durations where you create you callable and invoke it many times, where with all other usages I have seen myself, it is customary to create and configure the encoder or decoder for a single use - as in the Codable overlay for Firestore.

It's likely not an issue, but as I mentioned somewhere else in this thread it represents a pattern of usage of the encoder and decoder that I haven't met before.

[mortenbekditlevsen]

But using something besides the defaults may not be very uncommon. For instance through a preference for snake cased keys in JS, but not in Swift...

[mortenbekditlevsen]

And apologies for using 'you' to refer to you all working on the Firebase Apple sdk collectively. I know that you may of course have different opinions. 😊

I you wish to try out the two flavors of generics discussed above, toggle between head and head minus two commits. 😊

[mortenbekditlevsen]

Also note that I am in the process of adding tests so you can see the call site after @paulb777 made it possible to use the required 'fakes' from Swift. I think I'll have the tests ready tomorrow.

[mortenbekditlevsen]

Let me know if you like the requestType and responseType argument names, or if you can think of better names.

[paulb777]

Now that we have Swift test infrastructure for FirebaseFunctions after #8957, #8959 and #8960, we should be able to add integration tests. The integration tests could also potentially be used to demonstrate the Functions Codable API for the API proposal.

I rebased the codable-refactor branch with #8853 on the latest master and pushed it in codable-refactor2. This PR could rebase to codable-refactor2 and change its merge target to that branch and then add the tests to https://github.com/firebase/firebase-ios-sdk/tree/master/FirebaseFunctions/Tests/SwiftIntegration

[schmidt-sebastian]

Let me know if you like the requestType and responseType argument names, or if you can think of better names.

I am trying to understand why we need those arguments. Are they provided to let the compiler infer the types correctly?

[mortenbekditlevsen]

Exactly - so that's the way to say: this callable always takes requestType as it's input and returns a responseType. As opposed to supplying the types at the call site with the risk of using the same callable with different types from different call sites.
To me, the request and response types correspond to the transformation closures of the Firestore js APIs you referred to.

[schmidt-sebastian]

It looks like the requestType/responseType is decoupled from the encoder though. How do we enforce that a custom encoder returns the right types? Could we do something like:

func httpsCallable<Request: Encodable,
    Response: Decodable>(_ name: String,
                         encoder: StructureEncoder<Request.Type>,
                         decoder: StructureEncoder<Response.Type>)
    -> Callable<Request, Response> {... }

or:

func httpsCallable<Request: Encodable,
    Response: Decodable>(_ name: String,
                         requestType: Request.Type,
                         responseType: Response.Type)
    -> Callable<Request, Response> {...

 public func call(_ data: Request,
                   encoder: StructureEncoder<Request.Type> = StructureEncoder(),
                   decoder: StructureDecoder<Response.Type> = StructureDecoder(),
                    ...
``

[mortenbekditlevsen]

The encoder and decoder are indeed decoupled from the Encodable and Decodable types. That's a big part of the design of the Codable system.
You'll notice that the StructureEncoder and StructureDecoder are not generic types. They do have generic functions, however, and the types of these generic functions are precisely resolved by the two supplied types.

[mortenbekditlevsen]

Excellent, let me know if you need me to do something in relation to this PRs.

[paulb777]

No immediate need. I can manage them after the PR merges.

However, if you do additional changes in this branch, it would be good to pull in #8983 for CI.

[paulb777]

#8981 is now merged and this PR should be similarly updated.

[mortenbekditlevsen]

I have a question regarding the following API:

  public func call(_ data: Request,
                   completion: @escaping (Result<Response, Error>)
                     -> Void) throws {
...
}

The throws is there as encoding can fail. But error handling is also performed inside of the callback (when unwrapping the Result).
Should I remove the throws here and call the completion handler with the error from encoding if that fails?
That would remove the double point of error handling...

[schmidt-sebastian]

The throws is there as encoding can fail. But error handling is also performed inside of the callback (when unwrapping the Result).
Should I remove the throws here and call the completion handler with the error from encoding if that fails?
That would remove the double point of error handling...

We generally try to throw for API input validations and raise error callbacks for network errors. In this case, though, I think we should remove the throws declaration as we can use the callback for errors during encoding and decoding. I think it would be confusing to only use the callback for network errors and decoding errors and treat encoding errors separately.

[schmidt-sebastian]

I just requested an approval for this API change. Currently, this is scheduled for Dec 6 (sorry for the long waiting time). The API in the approval is as follows:

public extension Functions {
  func httpsCallable<Request: Encodable,
    Response: Decodable>(_ name: String,
                        encoder: StructureEncoder = StructureEncoder(),
                        decoder: StructureDecoder = StructureDecoder())
    -> Callable<Request, Response> { }
}

public struct Callable<Request: Encodable, Response: Decodable> {
  public func call(_ data: Request,
                   completion: @escaping (Result<Response, Error>)
                     -> Void) {}
  public func call(_ data: Request) async throws -> Response { }

  public func callAsFunction(_ data: Request,
                   completion: @escaping (Result<Response, Error>)
                     -> Void) {}
  public func callAsFunction(_ data: Request) async throws -> Response { }
}

We can update the proposal until the 6th.

[mortenbekditlevsen]

I just requested an approval for this API change. Currently, this is scheduled for Dec 6 (sorry for the long waiting time). The API in the approval is as follows:

public extension Functions {
  func httpsCallable<Request: Encodable,
    Response: Decodable>(_ name: String,
                        encoder: StructureEncoder = StructureEncoder(),
                        decoder: StructureDecoder = StructureDecoder())
    -> Callable<Request, Response> { }
}

public struct Callable<Request: Encodable, Response: Decodable> {
  public func call(_ data: Request,
                   completion: @escaping (Result<Response, Error>)
                     -> Void) {}
  public func call(_ data: Request) async throws -> Response { }

  public func callAsFunction(_ data: Request,
                   completion: @escaping (Result<Response, Error>)
                     -> Void) {}
  public func callAsFunction(_ data: Request) async throws -> Response { }
}

We can update the proposal until the 6th.

Hi @schmidt-sebastian ,
No worries about the timing, I've been using similar overlays since forever, so contributing with these APIs is mainly just for fun. :-)

I agree with your suggestion to bake in both the Request and Response types as well as the configuration for encoding and decoding when defining the Callable - perfect.
Also great with the addition of callAsFunction. This is one of the rare cases where this niche Swift feature actually makes sense. :-)

So the usage point for defining the callable using the above API would be require the type of the Callable to be spelled out:

let callable: Callable<MyRequest, MyResponse> = functions.httpsCallable("calculatePi")

Do you like that better than:

let callable = functions.httpsCallable("calculatePi", requestAs: MyRequest.self, responseAs: MyResponse.self)

I guess there are pros and cons, but I would love to hear your reasoning about them.

For myself, I can think of the following pros and cons:
Pros:

• The call site is shorter with the inferred case

Cons:

• The end user needs to look up the type returned by the function in order to know what type (Callable) to use. If you just write let callable = functions.httpsCallable("myCallable"), then you automatically get the previous style, untyped API. So basically it's hard to know that you can define the callable to be of type Callable<A, B> and suddenly get this new behaviour.
• Even if it had a function name that did not clash with the existing API, the compiler just writes Generic parameter Request could not be inferred and similar for Response, you still need to look up the function signature to know that those generic parameters belong to a type called Callable.

I think the discoverability of the new API is better when the types are given explicitly as arguments.

With those things said, good documentation and examples can get you a long way. :-)

Looking forward to hearing the conclusions of the review.

[schmidt-sebastian]

I talked to @peterfriese about this in the API proposal and we both prefer the shorter version, but I agree that it comes at the cost of discoverability. Let me explicitly highlight this as we go through the approval process.

[mortenbekditlevsen]

I can recommend just trying out both versions in Xcode.
Another idea could be to name the function differently - like perhaps callable(...) rather than overloading httpsCallable. Don't know if it's a good idea, but it's something to consider, perhaps.

[ryanwilson]

Thanks @mortenbekditlevsen for the PR!

Catching up on this thread now, but wanted to check in on one thing related to the custom encoder/decoder. I can't remember the reason for needing a custom encoder/decoder vs using the built in JSON(Encoder|Decoder). As far as I can tell at a quick glance of the diff, the two differences are removing the outputFormatting property, and removing usage of JSONSerialization. My main curiosity is if we can eliminate the need for the custom encoder/decoder, stick with the JSON encoder and remove the duplicated code and extra lib. Any insight to what I'm missing would be very helpful 😄

[mortenbekditlevsen]

Sure, the JSONEncoder and decoder could be used just fine for both RTDB and Functions, but it comes at the cost of requiring deserializing the data to a structure in one direction or seralizing to data in the other direction. So it would do some extra work that is redundant, because it's just undoing some work that the JSONEncoder is performing.
The StructureEncoder is exactly as you mention the same as JSONEncoder without the final jsonserialization step.
And that explains the removal of the single configuration option that relates to json serialization.

Note that I think that Paul and I discussed an attempt many years back to make the official JSONEncoder support outputting both Data as today, but also String (json) and the raw array/dict structure. Unfortunately that never happened.

In the Firestore pr (#8858) there are a few extra features added to the StructureEncoder and decoder while maintaining compatibility with the other two use cases.

Optimally the very limited set of changes from the JSONEncoder in Foundation could be applied as a small patch to make it easy to be on par with any future changes to JSONEEncoder.

[ryanwilson]

@mortenbekditlevsen thanks for the details, that's super helpful! I see the small changes now and understand the issue (and saw your posts on the Swift forums). It would be nice to just use the JSONEncoder/Decoder but I think since we aren't making massive changes to it, the fork makes sense to avoid the extra CPU cycles wasted (and potential data loss for Firestore). Sorry again for the long delay in things but I'm still catching up on it and hope to be able to give it a full review within the next week!

[ryanwilson]

Hey @mortenbekditlevsen - wanted to give you an idea as to what I am looking into and maybe you have some thoughts here.

Thinking of the higher level API and the decoder for RTDB and Functions (haven't re-evaluated Firestore yet so these comments don't apply):

1. I'm wondering if the encoder/decoder APIs for each product need to be different (or necessary for all products). Example: do we need to provide a custom way to encode the date, or are the respective Firebase backends expecting a specific format and we should disallow custom encoding of the date? I'm going to look into these, cc @schmidt-sebastian for thoughts on RTDB (and maybe Firestore)

a) If they are necessary, should we have the StructureEncoder as an implementation detail and have a small custom encoder for each one, only surfacing the customizations that make sense? I don't see a huge benefit of using the same customized encoder for both Functions and RTDB, but could be wrong.

b) If they're not necessary, we could remove the parameter and just leave it as an implementation detail.

My hunch: we may want some sort of customization, but it could be for specialized use cases. If that's the case, maybe we could remove the parameter and evaluate re-introducing it once we hear some reasonable and valid use cases.

[mortenbekditlevsen]

Hi @ryanwilson ,
For the three services, functions, RTDB and Firestore, only Firestore is really opinionated about the format of Date in that it supplies its own Timestamp type.

Which encoding is better for the other two services? And do you even really want to enforce dates to always be encoded as the timestamp type in Firestore? Are Dates even encoded as Timestamps in the existing FirestoreEncoder? I think I saw that Timestamp can decode to Date directly, but I don't recall seeing any special measures during encoding, so I assume that Dates Codable implementation is used on encoding. Please correct me if I'm wrong. 😊

The use case is that these encodings are (at least in my eyes) purely a matter of preference, style and taste.

If your functions are written in typescript you may like milliseconds since epoch. If your functions are written in rust and you use 'serde' for serialization it appears that iso8601 strings are the default.

Similarly your preferences may differ for RTDB and Firestore based on commonality between client languages or frameworks or may be dictated by a dominating language or by precedence or by the whims of a platforms architect.

The custom case is important too. You could prefer to have your dates encoded as YYYY-MM-DD-HH-mm in local time zone in order to perform time based queries on prefixes of the strings. Not that you can't do similar queries with numerical encodings, but it's an easy way to get 'start of period (year, month, day, hour, minute)' without doing calendrical calculations.

I think that my best argument for keeping the strategies that are already proposed by JSONEncoder is: you don't need to squint very hard in order to see functions arguments, RTDB data and Firestore data as json. Swift already defines a great set of strategies for encoding Date to json in Foundation, and this concept is likely familiar to users of Swift. Since the three Firebase services can be thought of as mechanisms for communication, synchronization and storage of json, then it seems natural to give the same amount of expressiveness in how that json can be generated as present in JSONEncoder.

That said, I like the hiding away of StructureEncoder behind small wrappers rather than indirectly exposing it through the typealias. But I really hope that all wrappers can get the same flexibility.

That's a big part of my motivation for suggesting the use of the same encoder across the services: I am lacking flexibility with the current FirestoreEncoder. 😊

[paulb777]

Rebased to codable-refactor3(which is the shared Codable from #8853 rebased on current master)

[paulb777]

I'm going to merge to enable setting up a full review on the codable-refactor3 branch