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.

Internal API review at go/firestore-data-encoding-strategies-api

[paulb777]

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

Do you need help resolving the PassthroughType related CI failures?

[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]

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?

[charlotteliang]

@mortenbekditlevsen I think we just forget to move this to the next release as we are releasing the previous release.

[paulb777]

@mortenbekditlevsen Sorry that we've been so slow at landing this PR. I'd really like to get it in before our code freeze deadline for Firebase 10.0.0 in the next week or so.

Reading through the comments, I see a few open questions, but I'm not sure what exactly still needs to be resolved to merge. Given that 10.0.0 is a breaking change release, we can allow some breakages if we document them and believe they're only used in corner cases.

Is it possible to update the Firestore CHANGELOG that summarizes the implications for the API's clients?

[mortenbekditlevsen]

I'll have a look at it tomorrow evening. There may not be any breaking changes after all. I'll have a look and suggest an addition to the change log.

[mortenbekditlevsen]

I added the following to the CHANGELOG - please let me know if you think it requires more details:

10.0.0

• [changed] Firestore.Encoder and Firestore.Decoder now wraps the shared FirebaseDataEncoder and FirebaseDataDecoder types which provides new customization options for encoding and decoding date to and from Firestore - similar to the options present on JSONEncoder and JSONDecoder from Foundation.
• [added] Firestore.Encoder.KeyEncodingStrategy
• [added] Firestore.Encoder.DateEncodingStrategy
• [added] Firestore.Encoder.DataEncodingStrategy
• [added] Firestore.Encoder.NonConformingFloatEncodingStrategy
• [added] Firestore.Decoder.KeyDecodingStrategy
• [added] Firestore.Decoder.DateDecodingStrategy
• [added] Firestore.Decoder.DataDecodingStrategy
• [added] Firestore.Decoder.NonConformingFloatDecodingStrategy

[mortenbekditlevsen]

With regards to breaking changes, I think that the way we ended up implementing it - by wrapping the shared encoder and decoder instead of just making it be a typealias to the shared encoder and decoder - ended up giving it a fully backwards compatible API surface. I'm not certain how to prove this, however. Isn't there some api checker tool that can verify this externally?

[paulb777]

Thanks @mortenbekditlevsen. Would it feasible to add tests for the new customization options?

[mortenbekditlevsen]

The various encoding and decoding strategies are tested thoroughly in FirebaseSharedSwift and the 'defaults' are tested through the existing tests. I could add some extra tests for the combination. This will verify that the wrapping of the decoder/encoder correctly passes on the strategies.
Is there a deadline for the release 10 work?

[paulb777]

Thanks @mortenbekditlevsen. Those tests would be useful, although not necessarily blocking for this PR to merge. We're planning to code freeze for Firebase 10 on September 27.

[paulb777]

The API review is still awaiting final approval, and I'm going to merge here to get into the Firebase 10 testing.

[andrewheard]

nit: "options for encoding and decoding date" date --> data or data types?

[andrewheard]

nit: should this be line wrapped?