Introducing UserDataReader/UserDataWriter #1177
Conversation
There was a problem hiding this comment.
Mostly LGTM, though I'm not a huge fan of the mocking changes. I've included an idea that could simplify the writer implementation and possibly avoid the mocking changes, so start there before investing too much in addressing the other comments; they might be irrelevant.
| @@ -33,7 +33,7 @@ | |||
|
|
|||
| public static FieldValue wrap(Object value) { | |||
| DatabaseId databaseId = DatabaseId.forProject("project"); | |||
| UserDataConverter dataConverter = new UserDataConverter(databaseId); | |||
| UserDataReader dataConverter = new UserDataReader(databaseId); | |||
There was a problem hiding this comment.
Rename the local to dataReader?
There was a problem hiding this comment.
I thought I had caught all of these, but this PR went through a lot of iterations. Fixed now.
| * Install mocks for `FirebaseFirestore.getFirestoreSettings()` and | ||
| * `FirebaseFirestore.getUserDataWriter()`, which are used in DocumentSnapshot tests. | ||
| */ | ||
| public static void installDocumentSnapshotMocks(FirebaseFirestore firestore) { |
There was a problem hiding this comment.
This kind of centralization of mocking creation makes me uneasy. It seems like we're using the wrong tool for the job. If TestUtil.firestore() returned a real instance of Firestore these tests wouldn't have to mock anything to get what's otherwise default behavior of the instance.
Also, the function of this particular class is to expose as public methods things that are otherwise access protected in the public API. A way to allow mocking of package private methods would be just to contralize the Mockito.when call like this:
public static OngoingStubbing<UserDataReader> getUserDataWriter(FirebaseFirestore firestore) {
return Mockito.when(firestore.getUserDataWriter());
}There was a problem hiding this comment.
installDocumentSnapshotMocks() is not needed in our codebase to bypass access restrictions, but it was in the Kotlin tests since they live in a subpackage.
Either way, I reverted this change as I removed the getUserDataWriter() instance method on FirebaseFirestore.
| private Map<String, Object> convertObject( | ||
| ObjectValue objectValue, UserDataWriterOptions options) { | ||
| Object result = userDataWriter.convertValue(objectValue, options); | ||
| hardAssert(result instanceof Map, "Documens should be decoded into Maps"); |
| result.add(convertValue(v, options)); | ||
| } | ||
| return result; | ||
| private Map<String, Object> convertObject( |
There was a problem hiding this comment.
This seems like it could reasonably just be a part of the UserDataWriter API, making it more self-contained as to how to use it safely.
Indeed there's already a convertObject method there; why not just use that and avoid asserting/casting?
There was a problem hiding this comment.
The idea behind this was to only expose a single method in UserDataWriter. I don't feel strongly and made convertObject package private.
| private final FirebaseFirestore firestore; | ||
|
|
||
| /** Holds settings that define field value deserialization options. */ | ||
| static class UserDataWriterOptions { |
There was a problem hiding this comment.
I have no idea if this is a good idea, but what do you think of making these things properties of the UserDataWriter itself?
We already have to allocate a UserDataWriterOptions object so it's not like we're avoiding heap allocations with the current scheme. Meanwhile it would save us having to pass the options up and down every call.
This would allow you to remove the getUserDataWriter method on FirebaseFirestore and the associated mocking weirdness.
What do you think?
There was a problem hiding this comment.
The version of the PR that I showed you yesterday (that addressed both UserDataConversion and CanonicalIds) actually did just that. It was needed because I did not want to expose options in the canonical ID API. I scrapped this since I didn't want to create the extra object. You are correct though that I am still creating an extra object - and we will likely not be able to get rid of it since timestampsInSnapshots will have to stay around even when we drop in from Settings, as DocumentSnapshot.getDate() relies on it.
I reverted the parts of the last edit that removed the constructor params for serverTimestampBehavior and timestampsInSnapshotsEnabled.
This renames UserDataConverter to UserDataReader and refactors out the Java type conversion in DocumentSnapshot to UserDataWriter.
Note: This is against master.