Make Firestore use FirebaseDataEncoder and FirebaseDataDecoder (re-implementation of #8858)

[mortenbekditlevsen]

Continuing the discussion from PR #8858 here.

This PR makes Firestore use the FirebasesDataEncoder and FirebaseDataDecoder - bringing key coding strategies, etc. to the encoding, while attempting to be compatible with the existing encoder and decoder.

After restructuring of Firestore and FirebaseSharedSwift it was too hard for me to merge into the branch from #8858.

I still need to implement tests - since #8858 I have learned how to run tests by @peterfriese - thanks! :-) I have a feeling that the CodablePassthroughTypes isn't working as it is being passed type erased values. Needs to be done through is casting/checking instead.

[mortenbekditlevsen]

It's great to remove so many lines of code. thanks!

Do you need help resolving the PassthroughType related CI failures?

That would be great - I assume they are due to using a fixed version of FirebaseSharedSwift rather than the one in the same release? At least I had to add a reference to the local version of FirebaseSharedSwift in Firestore/Example/Podfile - but I don't know how all the other tests are built.

There are currently two failing tests in FirestoreEncoderTests, namely testDate and testDecodingDocumentIDWithConfictingFieldsThrows.

I think that the behavior of Date encoding and decoding ought to be discussed a bit more. I.e. I think that the default in the current Firebase encoder is to pass through dates directly - and while decoding there are a few special cases like Timestamps being able to be decoded directly to Date... I don't know if all these behaviors should be recreated.

With regards to throwing at conflicting document id fields, I think that I mentioned on the other PR that I think that this is a quite harsh behavior that makes it hard to evolve your models towards using document ids or away from them - if old data models with old ids make your code crash. So I'd recommend that this behavior was removed on purpose.

[paulb777]

@mortenbekditlevsen See #9493 for CI fixes

@peterfriese @ryanwilson @schmidt-sebastian Any thoughts about Morten's other questions?

[mortenbekditlevsen]

To clarify my uncertainty about Date encoding and decoding: I am not 100% certain about the current default encoding behavior for a Date. By reading the code alone, it appears that the current encoder leaves it as a Date and lets the firestore serialization deal with it. I just tried a small experiment, and it would appear to me that a Date serializes to a double_value field when written to firestore. Does this mean that Date currently doesn't round-trip?

If this is true, the behavior is asymmetric with the decoding, where at least it appears that a Timestamp in Firestore will default to decoding to a Date. This behavior is why I added the timestamp date decoding strategy with a fallback, so that Timestamp always decodes to Date, but secondarily any other strategy applies.

I would suggest that the default encoding strategy of a Date was .timestamp, so that the behavior is more symmetric.

There's of course a whole other discussion about why Timestamp exists at all. Isn't the platforms default currency Date precise enough? :-)

[paulb777]

Thanks @mortenbekditlevsen This Date discussion maps to these current CI test failures:

Failing tests: Firestore_IntegrationTests_iOS: FirestoreEncoderTests.testDate() FirestoreEncoderTests.testDecodingDocumentIDWithConfictingFieldsThrows()

[mortenbekditlevsen]

The first one, yes. But it appears that I am mistaken. Date actually does round trip on master. I will have to look closer into what's going on.

The second failure is related to my question about whether the presence of eg an 'id' property in data decoded with a DocumentID called 'id' should actually fail.

I think that it should not because it complicates evolution of your models. So I think that test case should just be deleted.

[mortenbekditlevsen]

Ok, I found out that my initial testing of the Date serialization in Firestore was wrong. A Date serialized to Firestore does indeed become a Timestamp when reading it back. I suggest that the default date encoding strategy then should just be .timestamp. This means that it becomes a Timestamp already upon encoding (before serialization), but the net result is the same. I'll go ahead and make that change and update the test to reflect that.

[schmidt-sebastian]

Sorry, I was out of office for a couple of days. Looks like you were able to find answers to your questions.

Firestore accepts Dates and Timestamps as input but by default only returns Timestamps. For customers that use Codable, we can deviate from the default when we know that the resulting type is meant to be a Date. This still seems to be the case if I understand testTimestampCanDecodeAsDate correctly. FWIW, Firestore TImestamps supports microsecond precision.

I don't have a strong opinion in regards to the DoumcentID validation. I agree that it makes it harder to migrate data models, so it seems reasonable to remove this validation.

[paulb777]

What is outstanding on this PR besides a CHANGELOG update? @mortenbekditlevsen @peterfriese @schmidt-sebastian @ryanwilson

[mortenbekditlevsen]

Hi Paul, There's one thing that I would like to change. I think that I 'over engineered' the .timestamp(fallback) date decoding strategy. The idea was, that since the existing Firestore Decoder always succeeds when decoding a Timestamp into a model containing a Date, then this always should be kept for backwards compatibility. So I made a timestamp(fallback) date decoding strategy that first sees if the encoded data is a Timestamp - and if it is, then it returns it's requested Date representation. And otherwise it falls back to the decoding strategy present in the argument...

But actually the always in the current decoder only needs to translate to 'by default'. Meaning that the default date decoding strategy for the Firestore decoder would just be a 'timestamp' decoder that looks like:

extension Firestore.Decoder.DateDecodingStrategy {
  /// Decode the `Date` from a Firestore `Timestamp`
  static var timestamp: Firestore.Decoder.DateDecodingStrategy {
    return .custom { decoder in
      let container = try decoder.singleValueContainer()
      let value = try container.decode(Timestamp.self)
      return value.dateValue()
    }
  }
}

This doesn't mean that the fallback behavior that I already implemented can't be of value, but it complicates the API a bit and it can of course be implemented by the user if one really wishes that behavior.

What do you think? Should I go ahead and do the simpler timestamp date decoding strategy or keep the version that's a bit more involved?

[paulb777]

@mortenbekditlevsen It sounds like you're saying that there is no difference in behavior between the two implementations. In that case, it sounds good to me to go ahead and update to the simpler implementation.

[mortenbekditlevsen]

@mortenbekditlevsen It sounds like you're saying that there is no difference in behavior between the two implementations. In that case, it sounds good to me to go ahead and update to the simpler implementation.

Well, not exactly, but the default behavior (and thus the compatibility with previous versions) is the same and I don't think there's a whole lot of value in the more complex behavior. I'll go ahead and commit the simpler version, and it can be compared to the more complex implementation through the git history if anyone is interested. :-)

[paulb777]

Thanks @mortenbekditlevsen! That looks much more maintainable.

Would you resolve the conflict with the master branch?

[mortenbekditlevsen]

Sure, I'll take a look at it later today.

[mortenbekditlevsen]

@paulb777 I think that should do it - but let's see what the CI says! :-)

[paulb777]

All green! Thanks @mortenbekditlevsen

[peterfriese]

Just a few remarks:

1. Can we rename the title of the PR to something more descriptive? I think it's OK to mention this is a re-implementation of an existing PR, but it'd be better to keep this short, e.g."Make Firestore use StructureEncoder and StructureDecoder (re-implementation of #8858)
2. Should be add a line to the changelog?
3. I wonder about the impact of this on the samples and docs. @mortenbekditlevsen can you provide your insight, so we can make sure this material is kept up to date in line with this change?

[mortenbekditlevsen]

@peterfriese: Certainly! By using FirebaseDataEncoder and FirebaseDataDecoder, we get some new expressivity while being compatible with the previous FirestoreEncoder and FirestoreDecoder.

The new expressivity is the same that one might already be familiar with when using FirebaseDataEncoder and FirebaseDataDecoder for FirebaseDatabaseSwift and FirebaseFunctionsSwift, and also the JSONEncoder and JSONDecoder from Foundation. Namely the ability to configure:

• keyEncodingStrategy / keyDecodingStrategy
• dateEncodingStrategy / dateDecodingStrategy
• dataEncodingStrategy / dataDecodingStrategy
• nonConformingFloatEncodingStrategy / nonConformingFloatDecodingStrategy

Personally, not having the key coding strategies for Firestore was the main reason that I could not the provided Swift overlays until now.

Internally, another improvement is to only have to maintain one pair of encoder/decoder across the various components.

[ryanwilson]

~I'll push a commit to revert the copyrights to save you some time cc @mortenbekditlevsen~

Just kidding, forgot you were working in a fork. Sorry about that.

[ryanwilson]

I'm going to try to make some changes based on the comments and push to a branch to get this in for Firebase 9, hopefully GitHub does the right thing and recognizes the same PR... will give it a go.

[paulb777]

Sorry, it looks like we're not going to have enough time to work through the API implications of this PR in time for our 9.0.0 release. We appreciate your efforts @mortenbekditlevsen and will follow up once we have 9.0.0 out.

[mortenbekditlevsen]

Hi @paulb777 , No worries, I'm not in a hurry. :-) But I do hope that the changes can still be used once the swift overlays are out of beta. Depending on how to wrap the default configuration of the encoder/decoder (wrap the type or provide a static property with configuration) there is actually a small breaking change for those who might call the decoder directly - although I can't imagine the situation where you would actually need to do this.

Just let me know how to proceed. :-)

[paulb777]

Thanks @mortenbekditlevsen. Would you more fully explain what the capabilities you are looking to use once the swift overlays go out of beta? My understanding was that this PR was mostly an internal refactor to reduce duplication of codable implementations across the Firebase products.

What exactly is the use case that the change will break for?

Why does the API surface need to be increased with the additional public methods in Firestore/Swift/Source/Codable/EncoderDecoder.swift? Following up the discussion there, does changing to a static eliminate that need?

[mortenbekditlevsen]

Hi @paulb777 , You're correct that it's mostly just a refactor. But I am really, really looking forward to keyEncodingStrategy and ditto for decoding.

The possible source breaking change I am mentioning is actually a decrease in api surface area per the discussion between me and @ryanwilson above.

The current Firestore.Decoder has a method:

public func decode<T: Decodable>(_: T.Type,
                                     from container: Any,
                                     in document: DocumentReference? = nil) throws -> T {

and I replicated that, but Ryan and I were considering removing this and moving this functionality a level 'up' so that the reference will be set in the places where the decoder is used instead.

I am not certain why the api is public today since it appears to me that it may as well just be internal...

Perhaps that's the solution: marking it internal as part of moving it out of beta.

[paulb777]

Yep, a separate PR that just does the breakages could be helpful. Then we could just do the breakages in 9.0.0 and then take our time with the adds and API reviews for 9.1.0

[paulb777]

After a few more conversations today, we'd like to try to land the new capabilities in 9.1.0 and deprecate, instead of remove, anything that needs to be deleted. See #9694. Then we'll clean up the deprecated stuff in 10.0.0.

[mortenbekditlevsen]

Sounds good. But deprecation means that in 9.10 the function still can't be removed as a side effect of the refactor. And this in turn means that the current (in this PR) Firestore.Decoder wrapper is still needed - rather than removing the wrapper as @ryanwilson suggested in the comments above.

And this is of course just fine - just wanted to point it out. 😊

[mortenbekditlevsen]

Well, I guess that the Firestore Swift overlay could still add an overload to the shared decoder as an extension, so there's perhaps no issue here...

[morganchen12]

In our discussion, we weighed the benefits of pushing the changes in 9.0 vs accumulating technical debt for the deprecation, and decided that we could achieve a smoother 9.0 release as well as provide a better dev exp by deprecating this method before removing it. Since the Firestore Codable implementation does not change often, we expect the tech debt incurred to be fairly manageable.

Thanks for the thorough breakdown in your comments Morten, it made understanding this change when Ryan handed it off much more manageable :)

[paulb777]

Since this won't make 9.2.0, moving milestone.

[mortenbekditlevsen]

Hi @chliangGoogle , I just noticed that the milestone for this PR was previously moved forward, but 16 days ago it was removed entirely. Does that mean that it's 'off the radar' for getting merged - or is the review just taking longer than expected?