Firestore: Allow passing POJOs as field values throughout API reference from Firebase #6843
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6843 +/- ##
============================================
+ Coverage 45.72% 55.6% +9.87%
+ Complexity 26528 10548 -15980
============================================
Files 2615 774 -1841
Lines 282766 88523 -194243
Branches 32745 8080 -24665
============================================
- Hits 129299 49223 -80076
+ Misses 143350 37082 -106268
+ Partials 10117 2218 -7899
Continue to review full report at Codecov.
|
| * automatically. | ||
| * | ||
| * @param fields A Map containing the data for the new document. | ||
| * @param fields A Map or a POJO containing the data for the new document. |
There was a problem hiding this comment.
nit: "a map or a POJO" and no period at end of comment since it's not a complete sentence
| */ | ||
| @Nonnull | ||
| public ApiFuture<DocumentReference> add(@Nonnull final Map<String, Object> fields) { | ||
| public ApiFuture<DocumentReference> add(@Nonnull final Object fields) { |
There was a problem hiding this comment.
This looks like a breaking API change. Is that true? If so, this would require a major version bump.
There was a problem hiding this comment.
I don't see why this would be breaking - do you have an example?
BenWhitehead
left a comment
There was a problem hiding this comment.
I ran a clirr check locally[1] and received the following errors:
[ERROR] 7002: com.google.cloud.firestore.CollectionReference: Method 'public com.google.api.core.ApiFuture add(java.util.Map)' has been removed
[ERROR] 7002: com.google.cloud.firestore.DocumentReference: Method 'public com.google.api.core.ApiFuture create(java.util.Map)' has been removed
[ERROR] 7002: com.google.cloud.firestore.DocumentReference: Method 'public com.google.api.core.ApiFuture set(java.util.Map)' has been removed
[ERROR] 7002: com.google.cloud.firestore.DocumentReference: Method 'public com.google.api.core.ApiFuture set(java.util.Map, com.google.cloud.firestore.SetOptions)' has been removed
[ERROR] 7005: com.google.cloud.firestore.DocumentReference: Parameter 1 of 'public com.google.api.core.ApiFuture update(java.util.Map)' has changed its type to java.lang.Object
[ERROR] 7005: com.google.cloud.firestore.DocumentReference: Parameter 1 of 'public com.google.api.core.ApiFuture update(java.util.Map, com.google.cloud.firestore.Precondition)' has changed its type to java.lang.Object
[ERROR] 7002: com.google.cloud.firestore.UpdateBuilder: Method 'public com.google.cloud.firestore.UpdateBuilder create(com.google.cloud.firestore.DocumentReference, java.util.Map)' has been removed
[ERROR] 7002: com.google.cloud.firestore.UpdateBuilder: Method 'public com.google.cloud.firestore.UpdateBuilder set(com.google.cloud.firestore.DocumentReference, java.util.Map)' has been removed
[ERROR] 7002: com.google.cloud.firestore.UpdateBuilder: Method 'public com.google.cloud.firestore.UpdateBuilder set(com.google.cloud.firestore.DocumentReference, java.util.Map, com.google.cloud.firestore.SetOptions)' has been removed
[ERROR] 7005: com.google.cloud.firestore.UpdateBuilder: Parameter 2 of 'public com.google.cloud.firestore.UpdateBuilder update(com.google.cloud.firestore.DocumentReference, java.util.Map)' has changed its type to java.lang.Object
[ERROR] 7005: com.google.cloud.firestore.UpdateBuilder: Parameter 2 of 'public com.google.cloud.firestore.UpdateBuilder update(com.google.cloud.firestore.DocumentReference, java.util.Map, com.google.cloud.firestore.Precondition)' has changed its type to java.lang.Object
I think we need to keep all of the existing surface methods, and treat the new methods as overloaded. When method dispatch happens the most specific method should be invoked.
[1] mvn -Dmaven.test.skip.exec=true clean install && mvn --projects :google-cloud-firestore --also-make clirr:check
schmidt-sebastian
left a comment
There was a problem hiding this comment.
Thank you! We need to clean up the update handling, but otherwise this is great.
| */ | ||
| @Nonnull | ||
| public ApiFuture<WriteResult> update(@Nonnull Map<String, Object> fields) { | ||
| public ApiFuture<WriteResult> update(@Nonnull Object fields) { |
There was a problem hiding this comment.
Updates still needs to take in a Map of <String,Object> and should not accept top-level POJOs (nested POJOs are ok). The reason for this is that update() treats the keys of the fields map as field paths and not as string literals. If you passed a POJO with a property name of "foo.bar", update() would actually updated the nested field "bar" in map "foo".
| * | ||
| * @param documentReference The DocumentReference to overwrite. | ||
| * @param fields A map of the field paths and values for the document. | ||
| * @param fields The (Map or POJO) that will be used to populate the document contents. |
There was a problem hiding this comment.
Please drop the parenthesis.
| public T update( | ||
| @Nonnull DocumentReference documentReference, @Nonnull Map<String, Object> fields) { | ||
| public T update(@Nonnull DocumentReference documentReference, @Nonnull Object fields) { | ||
| Object data = CustomClassMapper.convertToPlainJavaTypes(fields); |
There was a problem hiding this comment.
As stated above, the top-level fields value should be a Map, but individual values can be custom POJOs.
This seems like a problem with the tooling. We rolled a similar change out on Android without a major version bump: https://github.com/firebase/firebase-android-sdk/pull/76/files |
Fixes #6711
Reference