-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make Firestore use StructureEncoder and StructureDecoder #8858
Changes from 11 commits
6df778d
819c37b
cb6a165
743c11c
9f27bce
ec86d61
bfb1353
765f3a6
deec46c
33f4379
77acaae
728446c
7a78dd0
f3b7279
34718bb
aca0271
b10e1d2
41820f0
9c4b9a0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
/* | ||
* Copyright 2020 Google LLC | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
import Foundation | ||
import FirebaseFunctions | ||
import FirebaseSharedSwift | ||
|
||
extension HTTPSCallable { | ||
enum CallableError: Error { | ||
case internalError | ||
} | ||
|
||
public func call<T: Encodable, U: Decodable>(_ data: T, | ||
resultAs: U.Type, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we drop There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 https://developer.apple.com/documentation/foundation/jsondecoder/2895189-decode Meaning that the API lets you do:
rather than:
And similar in the Firestore overlays:
which is used as:
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. |
||
encoder: StructureEncoder = StructureEncoder(), | ||
decoder: StructureDecoder = StructureDecoder(), | ||
completion: @escaping (Result<U, Error>) | ||
-> Void) throws { | ||
let encoded = try encoder.encode(data) | ||
call(encoded) { result, error in | ||
do { | ||
if let result = result { | ||
let decoded = try decoder.decode(U.self, from: result.data) | ||
completion(.success(decoded)) | ||
} else if let error = error { | ||
completion(.failure(error)) | ||
} else { | ||
completion(.failure(CallableError.internalError)) | ||
} | ||
} catch { | ||
completion(.failure(error)) | ||
} | ||
} | ||
} | ||
|
||
#if compiler(>=5.5) && canImport(_Concurrency) | ||
@available(iOS 15, tvOS 15, macOS 12, watchOS 8, *) | ||
public func call<T: Encodable, U: Decodable>(_ data: T, | ||
resultAs: U.Type, | ||
encoder: StructureEncoder = StructureEncoder(), | ||
decoder: StructureDecoder = | ||
StructureDecoder()) async throws -> U { | ||
let encoded = try encoder.encode(data) | ||
let result = try await call(encoded) | ||
return try decoder.decode(U.self, from: result.data) | ||
} | ||
#endif | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,9 @@ | |
|
||
import Foundation | ||
|
||
public protocol StructureCodingPassthroughType: NSObject { } | ||
public protocol StructureCodingUncodedUnkeyed {} | ||
|
||
extension DecodingError { | ||
/// Returns a `.typeMismatch` error describing the expected type. | ||
/// | ||
|
@@ -233,6 +236,9 @@ public class StructureEncoder { | |
/// The strategy to use for encoding keys. Defaults to `.useDefaultKeys`. | ||
open var keyEncodingStrategy: KeyEncodingStrategy = .useDefaultKeys | ||
|
||
/// If `usePassthroughTypes` is set to `true`, then any value of a type conforming to StructureCodingPassthroughType will not be encoded, but left as is in the resulting structure | ||
open var usePassthroughTypes: Bool = false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, exactly so that you could use the same models with Firestore, RTDB and callable functions:
Would encode as a native |
||
|
||
/// Contextual user-provided information for use during encoding. | ||
open var userInfo: [CodingUserInfoKey : Any] = [:] | ||
|
||
|
@@ -242,6 +248,7 @@ public class StructureEncoder { | |
let dataEncodingStrategy: DataEncodingStrategy | ||
let nonConformingFloatEncodingStrategy: NonConformingFloatEncodingStrategy | ||
let keyEncodingStrategy: KeyEncodingStrategy | ||
let usePassthroughTypes: Bool | ||
let userInfo: [CodingUserInfoKey : Any] | ||
} | ||
|
||
|
@@ -251,6 +258,7 @@ public class StructureEncoder { | |
dataEncodingStrategy: dataEncodingStrategy, | ||
nonConformingFloatEncodingStrategy: nonConformingFloatEncodingStrategy, | ||
keyEncodingStrategy: keyEncodingStrategy, | ||
usePassthroughTypes: usePassthroughTypes, | ||
userInfo: userInfo) | ||
} | ||
|
||
|
@@ -501,6 +509,7 @@ fileprivate struct _JSONKeyedEncodingContainer<K : CodingKey> : KeyedEncodingCon | |
} | ||
|
||
public mutating func encode<T : Encodable>(_ value: T, forKey key: Key) throws { | ||
if T.self is StructureCodingUncodedUnkeyed.Type { return } | ||
self.encoder.codingPath.append(key) | ||
defer { self.encoder.codingPath.removeLast() } | ||
self.container[_converted(key).stringValue] = try self.encoder.box(value) | ||
|
@@ -928,6 +937,8 @@ extension __JSONEncoder { | |
return (value as! NSDecimalNumber) | ||
} else if value is _JSONStringDictionaryEncodableMarker { | ||
return try self.box(value as! [String : Encodable]) | ||
} else if let passthrough = value as? StructureCodingPassthroughType, self.options.usePassthroughTypes { | ||
return passthrough | ||
} | ||
|
||
// The value should request a container from the __JSONEncoder. | ||
|
@@ -1601,6 +1612,11 @@ fileprivate struct _JSONKeyedDecodingContainer<K : CodingKey> : KeyedDecodingCon | |
} | ||
|
||
public func decode<T : Decodable>(_ type: T.Type, forKey key: Key) throws -> T { | ||
if T.self is StructureCodingUncodedUnkeyed.Type { | ||
// Note: not pushing and popping key to codingPath since the key is | ||
// not part of the decoded structure. | ||
return try T.init(from: self.decoder) | ||
} | ||
guard let entry = self.container[key.stringValue] else { | ||
throw DecodingError.keyNotFound(key, DecodingError.Context(codingPath: self.decoder.codingPath, debugDescription: "No value associated with key \(_errorDescription(of: key)).")) | ||
} | ||
|
@@ -2505,6 +2521,9 @@ extension __JSONDecoder { | |
} else { | ||
self.storage.push(container: value) | ||
defer { self.storage.popContainer() } | ||
if let passthrough = value as? StructureCodingPassthroughType, Swift.type(of: value) == type { | ||
return passthrough | ||
} | ||
return try type.init(from: self) | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,11 +16,9 @@ | |
|
||
import Foundation | ||
import FirebaseFirestore | ||
import FirebaseSharedSwift | ||
|
||
internal func isFirestorePassthroughType<T: Any>(_ value: T) -> Bool { | ||
return | ||
T.self == GeoPoint.self || | ||
T.self == Timestamp.self || | ||
T.self == FieldValue.self || | ||
T.self == DocumentReference.self | ||
} | ||
extension GeoPoint: StructureCodingPassthroughType {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW, I think the updated protocol-based implementation is more maintainable than what we had before. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. 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. I haven't tried it, but something like:
Then again: perhaps such a design is overkill since the only use case for the passthrough types is currently Firestore... What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you think we might be able to expand on the id of the
The DocumentReference can then be passes to FirestoreTypeResolver, which means that we no longer need to add it to I am not sure how feasible this is without implementing it first, but you might some more expertise here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 But for decoding I did try implementing a passthrough type resolver - and I enhanced discoverability a little by introducing a I updated the PR branch with the new refactor. Let me know what you think. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just had a different thought: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
extension Timestamp: StructureCodingPassthroughType {} | ||
extension FieldValue: StructureCodingPassthroughType {} | ||
extension DocumentReference: StructureCodingPassthroughType {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,7 @@ | |
|
||
import Foundation | ||
import FirebaseFirestore | ||
import FirebaseSharedSwift | ||
|
||
extension DocumentSnapshot { | ||
/// Retrieves all fields in a document and converts them to an instance of | ||
|
@@ -37,8 +38,10 @@ extension DocumentSnapshot { | |
with serverTimestampBehavior: ServerTimestampBehavior = .none, | ||
decoder: Firestore.Decoder? = nil) throws -> T? { | ||
let d = decoder ?? Firestore.Decoder() | ||
d.userInfo[documentRefUserInfoKey] = reference | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you think there is a different way we could handle this? This does not seem very discoverable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My hopes were that the end user does not have any need for discovering this, as it would be an implementation detail of the Don't you think that it's realistic that the As a side note, I really think that the |
||
d.dateDecodingStrategy = .timestamp(fallback: d.dateDecodingStrategy) | ||
if let data = data(with: serverTimestampBehavior) { | ||
return try d.decode(T.self, from: data, in: reference) | ||
return try d.decode(T.self, from: data) | ||
} | ||
return nil | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
/* | ||
* Copyright 2021 Google LLC | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
import FirebaseFirestore | ||
import FirebaseSharedSwift | ||
import Foundation | ||
|
||
extension Firestore { | ||
public typealias Encoder = StructureEncoder | ||
public typealias Decoder = StructureDecoder | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,81 @@ | ||
/* | ||
* Copyright 2021 Google LLC | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
import Foundation | ||
import FirebaseFirestore | ||
import FirebaseSharedSwift | ||
|
||
@available(macOS 10.12, iOS 10.0, watchOS 3.0, tvOS 10.0, *) | ||
private var _iso8601Formatter: ISO8601DateFormatter = { | ||
let formatter = ISO8601DateFormatter() | ||
formatter.formatOptions = .withInternetDateTime | ||
return formatter | ||
}() | ||
|
||
extension StructureDecoder.DateDecodingStrategy { | ||
public static func timestamp(fallback: StructureDecoder | ||
.DateDecodingStrategy = .deferredToDate) -> StructureDecoder.DateDecodingStrategy { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you quickly explain when this would be used in Firestore since we do not use JSON-formatted dates? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure - no other reason than: someone might have a different preference then encoding to / decoding from the 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 If you disagree then this can of course just be removed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As this is in the direction of decoding it's more like: 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 |
||
return .custom { decoder in | ||
let container = try decoder.singleValueContainer() | ||
do { | ||
let value = try container.decode(Timestamp.self) | ||
return value.dateValue() | ||
} catch { | ||
switch fallback { | ||
case .deferredToDate: | ||
return try Date(from: decoder) | ||
|
||
case .secondsSince1970: | ||
let double = try container.decode(Double.self) | ||
return Date(timeIntervalSince1970: double) | ||
|
||
case .millisecondsSince1970: | ||
let double = try container.decode(Double.self) | ||
return Date(timeIntervalSince1970: double / 1000.0) | ||
|
||
case .iso8601: | ||
if #available(macOS 10.12, iOS 10.0, watchOS 3.0, tvOS 10.0, *) { | ||
let string = try container.decode(String.self) | ||
guard let date = _iso8601Formatter.date(from: string) else { | ||
throw DecodingError.dataCorrupted(DecodingError.Context( | ||
codingPath: decoder.codingPath, | ||
debugDescription: "Expected date string to be ISO8601-formatted." | ||
)) | ||
} | ||
|
||
return date | ||
} else { | ||
fatalError("ISO8601DateFormatter is unavailable on this platform.") | ||
} | ||
|
||
case let .formatted(formatter): | ||
let string = try container.decode(String.self) | ||
guard let date = formatter.date(from: string) else { | ||
throw DecodingError.dataCorrupted(DecodingError.Context( | ||
codingPath: decoder.codingPath, | ||
debugDescription: "Date string does not match format expected by formatter." | ||
)) | ||
} | ||
|
||
return date | ||
|
||
case let .custom(closure): | ||
return try closure(decoder) | ||
} | ||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think we should rename. I will see if I can put together a Podspec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks - it's now applied and pushed. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also need the Podspec for FirebaseFunctionsSwift: https://gist.github.com/schmidt-sebastian/8b726a52540c972a2e083d460a51f954