Continue Firestore Codable 3 #2229

paulb777 · 2019-01-03T16:59:45Z

Continue Firestore codable after squashing #838 and #2178

Companion QuickStart PR is firebase/quickstart-ios#609

Final merge to master plan is to create another PR with two commits - one with #838 and one with everything after, verify they match this branch, then do a merge (unsquashed) of those two commits to master.

jonathansolorzn · 2019-01-13T00:09:02Z

Any updates on this @ryanwilson @wilhuff ?

paulb777 · 2019-01-14T04:01:17Z

@felixsolorzano Next steps:

Remove throws from APIs in the extensions
Make sure the new extension APIs don't mess up the non-codable original versions of the APIs
Fix the error handling to occur on the correct queue

I have some other priorities continuing into this week and hope to get back to this PR in the later part of the week.

In the meantime, feel free to work on any of the above by sending a PR to this pb-codable3 branch.

ryanwilson

Reviewing tests now.

Firestore/Example/Firestore.xcodeproj/project.pbxproj

Firestore/Example/Tests/API/FIRQueryTests.mm

Firestore/Swift/Source/Codable/DocumentReference+Codable.swift

Firestore/Swift/Source/Codable/FieldValue+Codable.swift

Firestore/Swift/Source/Codable/FirestoreDecoder.swift

Firestore/Swift/Source/Codable/FirestoreEncoder.swift

ryanwilson · 2019-01-23T16:26:21Z

Firestore/Swift/Source/Codable/FirestoreEncoder.swift

+    do {
+      setData(try Firestore.Encoder().encode(value))
+    } catch let error {
+      fatalError("TODO \(error)")


"Unable to encode data with Firestore encoder: \(error)"?

Here and a few places below where we also throw.

Doesn't throwing mess up the signature and usability? Is there a better option than fatalError?

Crossed my terms up a bit here, I meant "throwing the fatal error" - should have said "call fatalError" or something 😄

Firestore/Swift/Source/Codable/FirestoreEncoder.swift

Firestore/Swift/Tests/Codable/CodableDocumentTests.swift

ryanwilson · 2019-01-23T18:46:19Z

Firestore/Swift/Tests/Codable/CodableDocumentTests.swift

+    }
+    let dict = ["x": 42]
+    let model = Model(x: 42, opt: nil)
+    XCTAssertEqual(model.x, roundTrip(input: model, expected: dict).x)


Can we split the round trip out and also test model.opt?

Isn't it being tested on the last line?

This would be less easy to miss if Model were Equatable.

assertRoundTrip(model: Model(x: 42, opt: nil), encoded: ["x": 42]) assertRoundTrip(model: Model(x: 42, opt: 7), encoded: ["x": 42, "opt": 7])

Also, more cases that don't round trip are worth testing:

assertDecodes(["x": 42, "opt": nil], to: Model(x: 42, opt: nil))

Also cases that are invalid are worth testing too:

assertDecodingFails(Model.self, from: ["x": 42, "opt": true])

Firestore/Swift/Tests/Codable/CodableDocumentTests.swift

Firestore/Swift/Source/Codable/third_party/FirestoreEncoder.swift

Firestore/Swift/Tests/Codable/CodableDocumentTests.swift

ryanwilson

This LGTM , please wait for @wilhuff's review.

paulb777 · 2019-01-24T16:31:30Z

Thanks @ryanwilson , the PR still needs to ensure the original, overridden APIs work correctly and merge master.

wilhuff

First pass.

Firestore/Swift/Source/Codable/FieldValue+Codable.swift

Firestore/Swift/Source/Codable/Timestamp+Codable.swift

Firestore/Swift/Source/Codable/CodableErrors.swift

wilhuff · 2019-01-24T18:09:06Z

Firestore/Swift/Source/Codable/third_party/FirestoreEncoder.swift

+import Foundation
+import FirebaseFirestore
+
+extension CollectionReference {


These extensions are adding API surface sugar and don't have anything to do with the FirestoreEncoder.

Given the separate provenance of this stuff and the complexity of the encoder proper, we should try to keep the sugar separate from the FirestoreEncoder

wilhuff · 2019-01-24T18:28:24Z

Firestore/Swift/Source/Codable/third_party/FirestoreEncoder.swift

+  }
+}
+
+extension DocumentReference {


Should these things be in DocumentReference+Codable.swift? Similarly should the other extensions here go in Foo+Codable for all Foos? (Note that I think this would be on the DocumentReference extension rather than the CodableDocumentReference extension.

One point of concern is that these methods aren't strictly required for Codable--rather they're Firestore-specific extensions to the API to make encoding simple. Then again, these are intimately related to our Codable support so it seems reasonable to just have one file per type we're extending.

wilhuff · 2019-01-24T18:29:21Z

Firestore/Swift/Source/Codable/third_party/FirestoreEncoder.swift

+extension CollectionReference {
+  public func addDocument<T: Encodable>(_ item: T) -> DocumentReference {
+    do {
+      return addDocument(data: try Firestore.Encoder().encode(item))


This Firestore.Encoder().encode() incantation is cumbersome.

Secondarily, nesting the Encoder and Decoder within Firestore seems inconsistent with other similar types. For example, JSONEncoder and JSONDecoder aren't both nested in some common outer type. Within Firestore, our "settings" object is FirestoreSettings not Firestore.Settings. I think this argues for marking the encoder type FirestoreEncoder.

However, do we even need to expose the FirestoreEncoder type? Could we just have a static method hanging off Firestore, and call it like Firestore.encode()? JSONEncoder doesn't implement some common supertype so it doesn't seem like you could write generic code that uses different encoder types.

Changed to public static Firestore.encode and Firestore.decode

wilhuff · 2019-01-24T18:50:55Z

Firestore/Swift/Source/Codable/third_party/FirestoreEncoder.swift

+      let encoded = try Firestore.Encoder().encode(item)
+      return addDocument(data: encoded, completion: completion)
+    } catch let error {
+      Firestore.firestore().settings.dispatchQueue.sync {


The dispatch queue associated with the default settings may not be the same as the dispatch queue for this instance of Firestore. This should be self.firestore.settings.dispatchQueue to refer through the settings for this instance.

N/A because of switch to fatalError

wilhuff · 2019-01-24T19:46:09Z

Firestore/Swift/Source/Codable/third_party/FirestoreEncoder.swift

+
+extension CollectionReference {
+  public func addDocument<T: Encodable>(_ item: T) -> DocumentReference {
+    do {


I think we could greatly reduce the amount of boilerplate in here if we had some common code like this:

internal func encodeOrDie<T: Encodable>(_ value: T) -> [String : Any] { do { return try Firestore.encode(value) } catch let error { fatalError("Unable to encode data with Firestore encoder: \(error)") } }

Then each type that exposes these kinds of methods could be implemented like this:

extension CollectionReference { public func addDocument<T: Encodable>(_ item: T) -> DocumentReference { let encoded = encodeOrDie(item) return addDocument(data: encoded) } }

Done in EncodeOrDie.swift

wilhuff · 2019-01-24T20:06:49Z

Firestore/Swift/Source/Codable/third_party/FirestoreEncoder.swift

+      Firestore.firestore().settings.dispatchQueue.sync {
+        completion!(error)
+      }
+      return document() // Is there something better to return after the error?


This particular wart exposes a major problem. As I'm messing around with this I think the inconsistent failure behavior between completion-accepting and non-completion-accepting variants of these methods is wrong. It makes it difficult to describe what our expectations are on conforming types, and makes it difficult to concisely describe the API guarantee we're making.

I'd like us to say simply that encoding errors on any of these encodable-accepting types are fatalErrors, and then we don't need to synthesize fake instances to return like this.

Users that want to handle the error programmatically can use the encode function (or encoder type) directly.

Changed to always fatalError

wilhuff · 2019-01-24T20:08:00Z

Firestore/Swift/Source/Codable/third_party/FirestoreEncoder.swift

+  }
+}
+
+fileprivate class _FirestoreEncoder: Encoder {


I can't see what you're changing in here. I'd like us to do a separate PR for moving the FirestoreEncoder into third_party so that it's more obvious we're changing.

This is a new file to master and mostly unchanged from the original PR. Perhaps, best to look at commit by commit history?

…ls, 2019

paulb777 · 2019-01-28T21:52:56Z

Firestore/Swift/Source/Codable/third_party/FirestoreEncoder.swift

+  }
+}
+
+fileprivate class _FirestoreEncoder: Encoder {


This is a new file to master and mostly unchanged from the original PR. Perhaps, best to look at commit by commit history?

paulb777 · 2019-01-28T21:53:52Z

Firestore/Swift/Tests/API/BasicCompileTests.swift

@@ -198,7 +198,7 @@ func writeDocuments(at docRef: DocumentReference, database db: Firestore) {
 }

 func addDocument(to collectionRef: CollectionReference) {
-  collectionRef.addDocument(data: ["foo": 42])
+  _ = collectionRef.addDocument(data: ["foo": 42])


Prevents warning if collectionRef is a Swift API.

cabarique · 2019-01-28T22:19:34Z

@paulb777 thanks for the work done. Any updates on this issue?

paulb777 · 2019-01-28T22:27:35Z

@cabarique The last batch of changes is running CI on travis as we speak.

The next step will be to continue the review with @wilhuff, @ryanwilson, and anyone else with input.

Does the current implementation satisfy your requirements?

cabarique · 2019-01-28T22:35:14Z

@cabarique The last batch of changes is running CI on travis as we speak.

The next step will be to continue the review with @wilhuff, @ryanwilson, and anyone else with input.

Does the current implementation satisfy your requirements?

@paulb777 It does. Looking forward its completion.

wilhuff · 2019-01-29T19:48:48Z

Firestore/Example/Firestore.xcodeproj/project.pbxproj

+				HEADER_SEARCH_PATHS = (
+					"$(inherited)",
+					"\"${PODS_ROOT}/../../..\"",
+					"\"${PODS_ROOT}/../../../Firestore/third_party/abseil-cpp\"",


This should not be required and is likely harmful. We don't want any C++ when compiling Swift.

Same for the Release configuration below.

Here's what happens without the abseil include building the swift tests:

In file included from /Users/paulbeusterien/gh/firebase-ios-sdk/Firestore/Example/Tests/API/FSTAPIHelpers.mm:25: In file included from /Users/paulbeusterien/gh/firebase-ios-sdk/Firestore/Example/Tests/Util/FSTHelpers.h:22: In file included from /Users/paulbeusterien/gh/firebase-ios-sdk/Firestore/Source/Model/FSTDocument.h:19: In file included from /Users/paulbeusterien/gh/firebase-ios-sdk/Firestore/core/src/firebase/firestore/model/document_key.h:26: In file included from /Users/paulbeusterien/gh/firebase-ios-sdk/Firestore/Source/Model/FSTDocumentKey.h:22: In file included from /Users/paulbeusterien/gh/firebase-ios-sdk/Firestore/core/src/firebase/firestore/model/resource_path.h:24: In file included from /Users/paulbeusterien/gh/firebase-ios-sdk/Firestore/core/src/firebase/firestore/model/base_path.h:27: In file included from /Users/paulbeusterien/gh/firebase-ios-sdk/Firestore/core/src/firebase/firestore/util/hard_assert.h:23: In file included from /Users/paulbeusterien/gh/firebase-ios-sdk/Firestore/core/src/firebase/firestore/util/string_format.h:24: /Users/paulbeusterien/gh/firebase-ios-sdk/Firestore/core/src/firebase/firestore/util/string_apple.h:29:10: fatal error: 'absl/strings/string_view.h' file not found #include "absl/strings/string_view.h" ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 error generated.

wilhuff · 2019-01-29T19:52:44Z

Firestore/Swift/Source/Codable/CodableErrors.swift

+ * limitations under the License.
+ */
+
+enum FirestoreDecodingError: Error {


These need comments if they're going to be public.

We can ignore this for now though.

wilhuff · 2019-01-29T20:01:44Z

Firestore/Swift/Source/Codable/CodablePassThroughTypes.swift

+import Foundation
+import FirebaseFirestore
+
+func isCodablePassThroughType<T: Any>(_ value: T) -> Bool {


A few thoughts:

This could be internal, no?

T could likely be constrained to Codable.

The name is slightly off, see below

The issue with the name is that these aren't pass-through for Codable generally so much as they need special handling in the Firestore encoder and decoder. I'd suggest either calling this just isPassThroughType or isFirestorePassthroughType to make it clearer that we're doing this for some Firestore-specific reason.

If it's Codable, then both the Encodable and Decodable calls will fail to match because of not conforming to the other. Making changes 1 and 3.

/Users/paulbeusterien/gh/firebase-ios-sdk/Firestore/Swift/Source/Codable/third_party/FirestoreEncoder.swift:345:42: error: argument type 'T' does not conform to expected type 'Decodable' /Users/paulbeusterien/gh/firebase-ios-sdk/Firestore/Swift/Source/Codable/third_party/FirestoreDecoder.swift:1008:43: error: argument type 'T' does not conform to expected type 'Encodable'

Firestore/Swift/Source/Codable/CollectionReference+Codable.swift

wilhuff · 2019-01-29T20:41:13Z

Firestore/Swift/Source/Codable/DocumentReference+Codable.swift

+}
+
+extension DocumentReference: CodableDocumentReference {
+  public func setData<T: Encodable>(encoderInput: T) {


Similar to above, from encodable: T. Unlike CollectionReference.addDocument the underlying setData does not have a label, but I think it's OK.

let model: Codable = ... collection.setData(from: model)

vs

let values: [String : Any] = ... collection.setData(values)

wilhuff · 2019-01-29T21:25:18Z

Firestore/Swift/Tests/Codable/CodableDocumentTests.swift

+    struct Model: Codable {}
+    let model = Model()
+    let dict = [String: Any]() as NSDictionary
+    let encoded = try! Firestore.encode(model) as NSDictionary


With the above changes all these tests would now have the same form:

struct Model: Codable, Equatable { // ... } let model = Model(...) let dict = [...] // without as NSDictionary assertRoundTrip(model: model, encoded: dict)

wilhuff · 2019-01-29T21:39:40Z

Firestore/Swift/Tests/Codable/CodableDocumentTests.swift

+    }
+    let dict = ["x": 42]
+    let model = Model(x: 42, opt: nil)
+    XCTAssertEqual(model.x, roundTrip(input: model, expected: dict).x)


This would be less easy to miss if Model were Equatable.

assertRoundTrip(model: Model(x: 42, opt: nil), encoded: ["x": 42]) assertRoundTrip(model: Model(x: 42, opt: 7), encoded: ["x": 42, "opt": 7])

Also, more cases that don't round trip are worth testing:

assertDecodes(["x": 42, "opt": nil], to: Model(x: 42, opt: nil))

Also cases that are invalid are worth testing too:

assertDecodingFails(Model.self, from: ["x": 42, "opt": true])

wilhuff · 2019-01-29T22:07:12Z

Firestore/Swift/Tests/Codable/CodableDocumentTests.swift

+  }
+
+  func testEnum() {
+    enum MyEnum: Codable, Equatable {


As implemented here it appears this enumeration may be suitable for encoding both as a top-level type and when nested. Pull this out and assert both round-trips work?

Are enums ever directly Codable (i.e. without writing this boilerplate)? If so we should have a test for that too.

Not that I could find.

wilhuff · 2019-01-29T22:09:00Z

Firestore/Swift/Tests/Codable/CodableDocumentTests.swift

+      let x: Int
+    }
+    let dict = ["x": "abc"] // Wrong type;
+    XCTAssertThrowsError(try Firestore.decode(Model.self, from: dict))


For readability, consider extract this to something like assertFailsDecoding(_:from:)

wilhuff · 2019-01-29T22:18:03Z

Firestore/Swift/Tests/Codable/CodableDocumentTests.swift

+    }
+    class FirestoreDummy {
+      var visited = 0
+      func setObject<T: Codable>(_ object: T, fieldValues: [PartialKeyPath<T>: FieldValue] = [:]) {


I can't understand what this test is doing at all. It needs some comments describing what's going on here.

For example, is setObject some hook that's part of Codable generally? Are we simulating something an end user might write? Or are we using this to assert the behavior of the encoder? It seems like if the latter we can just assert that a completed object serializes one way or another.

In any case, it appears that fieldValues is completely ignored here which doesn't seem right.

Also, asserting the visited count doesn't seem useful. It seems as if it's asserting that the test itself called setObject twice?

Deleted the test.

wilhuff · 2019-01-29T23:02:28Z

Firestore/Swift/Tests/Codable/CodableDocumentTests.swift

+    let model = Model(x: nil)
+    let dict = ["x": nil] as [String: Int?]
+    let encodedDict = try! Firestore.encode(model)
+    XCTAssertNil(encodedDict["x"])


So I just checked the Android SDK and I think this assertion is ambiguous. It doesn't distinguish between an "x" that's present and has a value of nil or an "x" that's absent. We really care that it's the former not the latter.

On Android, given a class like this:

private static class StringBean { private String value; public String getValue() { return value; } }

This test holds true:

@Test public void primitiveSerializeString() { StringBean bean = new StringBean(); assertJson("{'value': null}", serialize(bean)); }

This is slightly different from Swift, because nullability is implicit and default for Object-types in Java. Nevertheless it's important to preserve that the key exists with a null value in the output because null has an explicit representation in Firestore and you can't query for missing fields.

This means that we have to be very careful:

XCTAssertNotEqualObjects([:], ["x": nil]); assertRoundTrip(model: model, encoded: dict) assertDecodes([:], to: model)

Handling nil properly looks challenging and is not currently supported by the JSON encoder. See https://stackoverflow.com/questions/47266862/encode-nil-value-as-null-with-jsonencoder and https://bugs.swift.org/browse/SR-9232.

Presumably we don't handle this now in Objective C because NSDictionary's don't allow nil values.

Any suggestions other than deferring nil support?

ghost · 2019-04-14T09:34:35Z

@wilhuff Any chance this will be seeing the light of day soon? It would be a greatly appreciated feature! Thanks!

paulb777 · 2019-06-17T17:22:41Z

Closing in favor of #3198

Squash #2178 to continue Firestore Codable (#2228)

1743eb2

googlebot added the cla: yes label Jan 3, 2019

paulb777 mentioned this pull request Jan 3, 2019

Continue Firestore Codable #2178

Closed

paulb777 requested review from wilhuff and ryanwilson January 3, 2019 17:11

Fix bad merge

e8fbb2a

paulb777 added the api: firestore label Jan 4, 2019

Remove throws from decodable APIs

35365ac

ryanwilson reviewed Jan 23, 2019

View reviewed changes

Address review feedback

1533b28

ryanwilson reviewed Jan 23, 2019

View reviewed changes

Firestore/Swift/Source/Codable/third_party/FirestoreEncoder.swift Outdated Show resolved Hide resolved

Address test review feedback

280bbf6

ryanwilson reviewed Jan 24, 2019

View reviewed changes

Firestore/Swift/Tests/Codable/CodableDocumentTests.swift Outdated Show resolved Hide resolved

Firestore/Swift/Tests/Codable/CodableDocumentTests.swift Show resolved Hide resolved

Remove force unwrap

ee349c2

ryanwilson approved these changes Jan 24, 2019

View reviewed changes

wilhuff reviewed Jan 24, 2019

View reviewed changes

wilhuff suggested changes Jan 24, 2019

View reviewed changes

paulb777 added 8 commits January 24, 2019 16:47

Codable Timestamp

a858b9a

Extension to separate files, Timestamp pass-through, match param labe…

d79806b

…ls, 2019

Firestore.encode() and Firestore.decode()

162e775

Add Function for checking Firestore pass-through types

0de7c25

encodeOrDie

c980da0

Fix unused build warning

ef120f9

Stop overriding in extensions

cca9503

merge master

064b0c3

paulb777 commented Jan 28, 2019

View reviewed changes

paulb777 added 2 commits January 28, 2019 14:03

One more 2019

7bd089d

style

be05ca6

from argument label

7dd421e

wilhuff reviewed Jan 29, 2019

View reviewed changes

paulb777 added 8 commits February 2, 2019 09:12

Address code review feedback (not including tests)

b98e874

wip

4be21cd

style

f1a94b8

checkpoint

75d2bbb

style

5558597

Test and comments about nil encoding challenge

a3eaadc

Back to encode/decode instance methods and remove some extensions

7c6ee0f

style

90673ea

This was referenced Mar 26, 2019

Swift Codable and Extension DocumentSnapshot #2635

Closed

Swift Codable and Extension DocumentSnapshot #2636

Closed

serjooo mentioned this pull request May 30, 2019

How to map to FIRTimestamp alickbass/CodableFirebase#36

Open

paulb777 closed this Jun 17, 2019

paulb777 deleted the pb-codable3 branch October 2, 2019 19:19

firebase locked and limited conversation to collaborators Oct 23, 2019

Continue Firestore Codable 3 #2229

Continue Firestore Codable 3 #2229

Conversation

paulb777 commented Jan 3, 2019 • edited Loading

jonathansolorzn commented Jan 13, 2019

paulb777 commented Jan 14, 2019

ryanwilson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanwilson left a comment

Choose a reason for hiding this comment

paulb777 commented Jan 24, 2019

wilhuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cabarique commented Jan 28, 2019

paulb777 commented Jan 28, 2019

cabarique commented Jan 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Apr 14, 2019

paulb777 commented Jun 17, 2019

paulb777 commented Jan 3, 2019 •

edited

Loading