Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upCodable firestore #838
Conversation
wilhuff
and others
added some commits
Feb 20, 2018
This comment has been minimized.
This comment has been minimized.
googlebot
commented
Feb 22, 2018
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). Once you've signed, please reply here (e.g.
|
googlebot
added
the
cla: no
label
Feb 22, 2018
This comment has been minimized.
This comment has been minimized.
I signed it! |
This comment has been minimized.
This comment has been minimized.
googlebot
commented
Feb 22, 2018
CLAs look good, thanks! |
googlebot
added
cla: yes
and removed
cla: no
labels
Feb 22, 2018
paulb777
added
the
Firestore
label
Feb 23, 2018
wilhuff
requested changes
Feb 24, 2018
I've taken a pass through the decoder and note that there is a ton of effectively duplicate code. It's very hard to tell what's unique about each overload in here. I've made some suggestions for how to cut down on this. |
@@ -0,0 +1,38 @@ | |||
// |
This comment has been minimized.
This comment has been minimized.
wilhuff
Feb 24, 2018
Member
All files need a copyright notice to be in this repo. You can copy from the top of any existing file and adjust the date or use this:
/*
* Copyright 2018 Google
*
* 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.
*/
This comment has been minimized.
This comment has been minimized.
import FirebaseFirestore | ||
|
||
enum FirestoreDecodingError: Error { | ||
case decodingIsNotSupported |
This comment has been minimized.
This comment has been minimized.
wilhuff
Feb 24, 2018
Member
Google's coding style requires two space indents. Rather than fix this by hand please wait for PR #847 to land and run that here to fix this.
import FirebaseFirestore | ||
|
||
/** | ||
* A protocol describing the encodable properties of a FirebaseFirestore. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@@ -0,0 +1,38 @@ | |||
// |
This comment has been minimized.
This comment has been minimized.
wilhuff
Feb 24, 2018
Member
I just re-read our internal style guide and realized that these files should really be Class+Protocol.swift. In this case DocumentReference+Codable.swift
. Sorry to lead you astray with the other PR :-(.
This comment has been minimized.
This comment has been minimized.
import FirebaseFirestore | ||
|
||
enum FirestoreDecodingError: Error { |
This comment has been minimized.
This comment has been minimized.
wilhuff
Feb 24, 2018
Member
These don't have much to do with DocumentReference. Perhaps put these in an Errors.swift
?
This comment has been minimized.
This comment has been minimized.
throw DecodingError.valueNotFound(type, DecodingError.Context(codingPath: decoder.codingPath + [_FirestoreKey(index: currentIndex)], debugDescription: "Unkeyed container is at end.")) | ||
} | ||
|
||
decoder.codingPath.append(_FirestoreKey(index: currentIndex)) |
This comment has been minimized.
This comment has been minimized.
wilhuff
Feb 24, 2018
Member
If this (and the defer) were at the top of function then we wouldn't have to append to to the codingPath to compose the error message for the !isAtEnd case.
defer { decoder.codingPath.removeLast() } | ||
|
||
guard let decoded = try decoder.unbox(container[currentIndex], as: Bool.self) else { | ||
throw DecodingError.valueNotFound(type, DecodingError.Context(codingPath: decoder.codingPath + [_FirestoreKey(index: currentIndex)], debugDescription: "Expected \(type) but found null instead.")) |
This comment has been minimized.
This comment has been minimized.
wilhuff
Feb 24, 2018
Member
Also decoder.codingPath
already has _FirestoreKey(index: currentIndex)
appended to the end of it; this will duplicate the index in the coding path.
guard !self.isAtEnd else { | ||
throw DecodingError.valueNotFound(KeyedDecodingContainer<NestedKey>.self, | ||
DecodingError.Context(codingPath: self.codingPath, | ||
debugDescription: "Cannot get nested keyed container -- unkeyed container is at end.")) |
This comment has been minimized.
This comment has been minimized.
wilhuff
Feb 24, 2018
Member
Despite the different error message, this could probably also use expectNotAtEnd
. I'm not sure there's any special value in giving a slightly better error message for cases that seem extremely unlikely.
This comment has been minimized.
This comment has been minimized.
} | ||
|
||
let value = self.container[self.currentIndex] | ||
guard !(value is NSNull) else { |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
alickbass
Feb 27, 2018
Author
We cannot really use requireValue
here as it expects the Optional
and we are checking for the NSNull
This comment has been minimized.
This comment has been minimized.
debugDescription: "Cannot get keyed decoding container -- found null value instead.")) | ||
} | ||
|
||
guard let dictionary = value as? [String : Any] else { |
This comment has been minimized.
This comment has been minimized.
wilhuff
Feb 24, 2018
Member
This through the bit that constructs the KeyedDecodingContainer
seems as if it can be a method we factor out on the top-level decoder.
public func container<Key>(keyedBy type: Key.Type) throws -> KeyedDecodingContainer<Key> {
return try container(of: storage.topContainer, keyedBy: type)
}
public func container<Key>(of value: Any, keyedBy type: Key.Type) throws -> KeyedDecodingContainer<Key> {
guard !(value is NSNull) else {
throw DecodingError.valueNotFound(KeyedDecodingContainer<Key>.self,
DecodingError.Context(codingPath: codingPath,
debugDescription: "Cannot get keyed decoding container -- found null value instead."))
}
guard let dictionary = value as? [String: Any] else {
let context = DecodingError.Context(codingPath: codingPath, debugDescription: "Not a dictionary")
throw DecodingError.typeMismatch([String: Any].self, context)
}
let container = _FirestoreKeyedDecodingContainer<Key>(referencing: self, wrapping: dictionary)
return KeyedDecodingContainer(container)
}
alickbass
added some commits
Feb 26, 2018
alickbass
added some commits
Feb 27, 2018
This comment has been minimized.
This comment has been minimized.
googlebot
commented
Feb 27, 2018
So there's good news and bad news. Note to project maintainer: This is a terminal state, meaning the |
googlebot
added
cla: no
and removed
cla: yes
labels
Feb 27, 2018
This comment has been minimized.
This comment has been minimized.
@wilhuff I merged master into this branch... And now I am not sure what should I do :) Should I create another PR against master? |
This comment has been minimized.
This comment has been minimized.
Just keep going. If you're really concerned about the clarity of the PR
stack, feel free to rebase your changes and force push to your repository.
We're going to squash merge your change into canonical history so it
doesn't matter too much if your changes are all that clear at the commit
level.
…On Tue, Feb 27, 2018 at 7:44 AM Oleksii Dykan ***@***.***> wrote:
@wilhuff <https://github.com/wilhuff> I merged master into this branch...
And now I am not sure what should I do :) Should I create another PR
against master?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#838 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AJxjkPzXejXuGEt0sJQM_mbBBiYa9jahks5tZCLagaJpZM4SPic9>
.
|
wilhuff
changed the base branch from
wilhuff/codable
to
master
Mar 1, 2018
This comment has been minimized.
This comment has been minimized.
The base PR from which this started has since been merged to master. I've retargeted this PR against master and now the diff is clean again. |
wilhuff
added
cla: yes
and removed
cla: no
labels
Mar 3, 2018
wilhuff
reviewed
Mar 3, 2018
Decoder changes are great. I have minor naming feedback (see specific callouts) and a larger question about how to handle values beyond the range of the intended storage type. JSONDecoder seems to want to fail, but in other context Firestore just does what a native cast would do. |
@@ -49,6 +49,16 @@ | |||
ReferencedContainer = "container:Firestore.xcodeproj"> | |||
</BuildableReference> | |||
</TestableReference> | |||
<TestableReference |
This comment has been minimized.
This comment has been minimized.
wilhuff
Mar 3, 2018
Member
The base state from which you branched was broken. I've since fixed this, but unfortunately these diffs remain. For all the .xcscheme
files, make sure your upstream branch is up-to-date and then git checkout the master version of these files and commit them. After pushing you shouldn't see any additions to project files.
@@ -0,0 +1,16 @@ | |||
// |
This comment has been minimized.
This comment has been minimized.
wilhuff
Mar 3, 2018
Member
Copyright notice.
I realize this isn't super important to you, but this is the only way that Google allows me to continue to work in the open with you, so it's very important to me :-).
} | ||
|
||
@available(swift 4.0.0) | ||
fileprivate class _FirestoreDecoder : Decoder { |
This comment has been minimized.
This comment has been minimized.
wilhuff
Mar 3, 2018
Member
Is Firestore.DocumentDecoder
a possibility? I'm still not clear on exactly what the nested struct within Firestore
is for. To me this seems like it should be the public interface on the extension of DocumentSnapshot
.
That is to say DocumentDecoder
becomes internal
, and we create an extension of DocumentSnapshot
that uses it (as above) to implement the public API for decoding to an object of a Decodable
type.
} | ||
|
||
@available(swift 4.0.0) | ||
fileprivate struct _FirestoreDecodingStorage { |
This comment has been minimized.
This comment has been minimized.
wilhuff
Mar 3, 2018
Member
Could this be named DocumentDecodingStorage
?
My basic concern is that "Firestore decoding" is too general. For example, we could, in the future decode whole query snapshots, in which case a "Firestore decoder" is insufficient.
} | ||
|
||
@available(swift 4.0.0) | ||
fileprivate struct _FirestoreKeyedDecodingContainer<K : CodingKey> : KeyedDecodingContainerProtocol { |
This comment has been minimized.
This comment has been minimized.
} | ||
|
||
public func decode(_ type: Bool.Type, forKey key: Key) throws -> Bool { | ||
let entry = try require(key: key) |
This comment has been minimized.
This comment has been minimized.
} | ||
|
||
@available(swift 4.0.0) | ||
fileprivate struct _FirestoreUnkeyedDecodingContainer : UnkeyedDecodingContainer { |
This comment has been minimized.
This comment has been minimized.
return false | ||
} | ||
|
||
/* FIXME: If swift-corelibs-foundation doesn't change to use NSNumber, this code path will need to be included and tested: |
This comment has been minimized.
This comment has been minimized.
wilhuff
Mar 3, 2018
Member
How are these FIXMEs to be addressed? I assume it comes from the implementation to which you referred.
Do you think there a way to add a check to make sure that we're not messing this up? I'm more concerned with correctness than speed so an extra else case here (after the two cases that are realistically possible) doesn't bother me.
|
||
let int8 = number.int8Value | ||
guard NSNumber(value: int8) == number else { | ||
throw DecodingError.dataCorrupted(DecodingError.Context(codingPath: self.codingPath, debugDescription: "Parsed JSON number <\(number)> does not fit in \(type).")) |
This comment has been minimized.
This comment has been minimized.
// * If it was a Float, you will get back the precise value | ||
// * If it was a Double or Decimal, you will get back the nearest approximation if it will fit | ||
let double = number.doubleValue | ||
guard abs(double) <= Double(Float.greatestFiniteMagnitude) else { |
This comment has been minimized.
This comment has been minimized.
wilhuff
Mar 3, 2018
Member
- @mikelehen What's the right way to respond to this based on our Android implementation?
It seems like CustomClassMapper just relies on "cast" operations (Number#floatValue
): non-failing, saturating casts that would have e.g. positive values beyond ~3.4 x 10^38 saturate to Infinity when assigned to a model value containing a Float instead of a double. Isn't a similar response appropriate here?
wilhuff
reviewed
Mar 3, 2018
In general the encoder stuff is far less egregious and doesn't seem like it needs much work (aside from naming). |
} | ||
|
||
@available(swift 4.0.0) | ||
fileprivate class _FirestoreEncoder : Encoder { |
This comment has been minimized.
This comment has been minimized.
wilhuff
Mar 3, 2018
Member
The names in here should parallel the resolution on the decoder (i.e. Document*)
This comment has been minimized.
This comment has been minimized.
phatmann
commented
Apr 13, 2018
Any progress on this? Would be great to have this part of the SDK. |
This comment has been minimized.
This comment has been minimized.
siyao1030
commented
May 7, 2018
•
Thanks for the hard work @alickbass @wilhuff! |
This comment has been minimized.
This comment has been minimized.
jorgej-ramos
commented
Jul 3, 2018
A great work @alickbass @wilhuff. Thank you both. |
This comment has been minimized.
This comment has been minimized.
swftvsn
commented
Jul 25, 2018
We really need this for our iOS project, so please please keep up the good work @alickbass @wilhuff! |
wilhuff
referenced this pull request
Jul 30, 2018
Merged
Add build infrastructure for Codable support in Firestore #815
phatmann
referenced this pull request
Oct 6, 2018
Open
Support keyDecodingStrategy property for snake case field names #34
This comment has been minimized.
This comment has been minimized.
Nailer
commented
Nov 5, 2018
Hi guys! What's the progress on this beautiful and highly anticipated feature? |
This comment has been minimized.
This comment has been minimized.
fenixsolorzano
commented
Nov 8, 2018
Any update on this? |
This comment has been minimized.
This comment has been minimized.
Closing and continuing in #2178 |
alickbass commentedFeb 22, 2018
•
edited
Hi guys! So this is a WIP PR to solve #627. It is based on #815.
In general, code is almost 1-to-1 port of JSONEncoder from Foundation and PlistEncoder with some minor changes to fit it to Firestore. Most of the code is just boilerplate and the most interesting part is in
in
FirestoreDecoder
andin
FirestoreEncoder
.I also created internal types
Firestore.Encoder
andFirestore.Decoder
to able to test without needingDocumentReference
andDocumentSnapshot
. Everything is internal now without any public methods, so that we can already start testing and deciding if it's the right approach without the pain of approving the public API//сс @wilhuff @paulb777