Migrate FieldValue to Protobuf #8124
Conversation
* Run migration to rewrite canonical IDs * Be more thorough. * Feedback. * Fix lint error
Should be compared to Android. These classes now use Document as the new immutable document type
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| case TypeOrder::kString: | ||
| return nanopb::MakeStringView(lhs.string_value) == | ||
| nanopb::MakeStringView(rhs.string_value); | ||
|
|
There was a problem hiding this comment.
Incremental code coverage report
Tests for New code lines are not detected in FirebaseFirestore-iOS, please add tests on highlighted lines.
|
|
||
| case TypeOrder::kBlob: | ||
| return CompareBlobs(lhs, rhs) == ComparisonResult::Same; | ||
|
|
There was a problem hiding this comment.
Incremental code coverage report
Tests for New code lines are not detected in FirebaseFirestore-iOS, please add tests on highlighted lines.
| case TypeOrder::kReference: | ||
| return nanopb::MakeStringView(lhs.reference_value) == | ||
| nanopb::MakeStringView(rhs.reference_value); | ||
|
|
There was a problem hiding this comment.
Incremental code coverage report
Tests for New code lines are not detected in FirebaseFirestore-iOS, please add tests on highlighted lines.
| case TypeOrder::kGeoPoint: | ||
| return lhs.geo_point_value.latitude == rhs.geo_point_value.latitude && | ||
| lhs.geo_point_value.longitude == rhs.geo_point_value.longitude; | ||
|
|
There was a problem hiding this comment.
Incremental code coverage report
Tests for New code lines are not detected in FirebaseFirestore-iOS, please add tests on highlighted lines.
|
|
||
| case TypeOrder::kArray: | ||
| return ArrayEquals(lhs.array_value, rhs.array_value); | ||
|
|
There was a problem hiding this comment.
Incremental code coverage report
Tests for New code lines are not detected in FirebaseFirestore-iOS, please add tests on highlighted lines.
|
|
||
| default: | ||
| HARD_FAIL("Invalid type value: %s", left_type); |
There was a problem hiding this comment.
Incremental code coverage report
Tests for New code lines are not detected in FirebaseFirestore-iOS, please add tests on highlighted lines.
| switch (value.which_value_type) { | ||
| case google_firestore_v1_Value_null_value_tag: | ||
| return "null"; | ||
|
|
There was a problem hiding this comment.
Incremental code coverage report
Tests for New code lines are not detected in FirebaseFirestore-iOS, please add tests on highlighted lines.
|
|
||
| case google_firestore_v1_Value_boolean_value_tag: | ||
| return value.boolean_value ? "true" : "false"; | ||
|
|
There was a problem hiding this comment.
Incremental code coverage report
Tests for New code lines are not detected in FirebaseFirestore-iOS, please add tests on highlighted lines.
|
|
||
| case google_firestore_v1_Value_integer_value_tag: | ||
| return std::to_string(value.integer_value); | ||
|
|
There was a problem hiding this comment.
Incremental code coverage report
Tests for New code lines are not detected in FirebaseFirestore-iOS, please add tests on highlighted lines.
|
|
||
| case google_firestore_v1_Value_double_value_tag: | ||
| return absl::StrFormat("%.1f", value.double_value); | ||
|
|
There was a problem hiding this comment.
Incremental code coverage report
Tests for New code lines are not detected in FirebaseFirestore-iOS, please add tests on highlighted lines.
|
|
||
| case google_firestore_v1_Value_timestamp_value_tag: | ||
| return CanonifyTimestamp(value); | ||
|
|
There was a problem hiding this comment.
Incremental code coverage report
Tests for New code lines are not detected in FirebaseFirestore-iOS, please add tests on highlighted lines.
|
|
||
| case google_firestore_v1_Value_string_value_tag: | ||
| return nanopb::MakeString(value.string_value); | ||
|
|
There was a problem hiding this comment.
Incremental code coverage report
Tests for New code lines are not detected in FirebaseFirestore-iOS, please add tests on highlighted lines.
|
|
||
| case google_firestore_v1_Value_bytes_value_tag: | ||
| return CanonifyBlob(value); | ||
|
|
There was a problem hiding this comment.
Incremental code coverage report
Tests for New code lines are not detected in FirebaseFirestore-iOS, please add tests on highlighted lines.
|
|
||
| case google_firestore_v1_Value_reference_value_tag: | ||
| return CanonifyReference(value); | ||
|
|
There was a problem hiding this comment.
Incremental code coverage report
Tests for New code lines are not detected in FirebaseFirestore-iOS, please add tests on highlighted lines.
|
|
||
| case google_firestore_v1_Value_geo_point_value_tag: | ||
| return CanonifyGeoPoint(value); | ||
|
|
There was a problem hiding this comment.
Incremental code coverage report
Tests for New code lines are not detected in FirebaseFirestore-iOS, please add tests on highlighted lines.
|
|
||
| case google_firestore_v1_Value_array_value_tag: | ||
| return CanonifyArray(value.array_value); | ||
|
|
There was a problem hiding this comment.
Incremental code coverage report
Tests for New code lines are not detected in FirebaseFirestore-iOS, please add tests on highlighted lines.
| } | ||
|
|
||
| default: | ||
| HARD_FAIL("Invalid type value: %s", value.which_value_type); |
There was a problem hiding this comment.
Incremental code coverage report
Tests for New code lines are not detected in FirebaseFirestore-iOS, please add tests on highlighted lines.
| std::string CanonicalId(const google_firestore_v1_ArrayValue& value) { | ||
| return CanonifyArray(value); | ||
| } |
There was a problem hiding this comment.
Incremental code coverage report
Tests for New code lines are not detected in FirebaseFirestore-iOS, please add tests on highlighted lines.
| source.string_value->size) | ||
| : nullptr; | ||
| break; | ||
|
|
There was a problem hiding this comment.
Incremental code coverage report
Tests for New code lines are not detected in FirebaseFirestore-iOS, please add tests on highlighted lines.
| target->reference_value = nanopb::MakeBytesArray( | ||
| source.reference_value->bytes, source.reference_value->size); | ||
| break; | ||
|
|
There was a problem hiding this comment.
Incremental code coverage report
Tests for New code lines are not detected in FirebaseFirestore-iOS, please add tests on highlighted lines.
| source.bytes_value->size) | ||
| : nullptr; | ||
| break; | ||
|
|
There was a problem hiding this comment.
Incremental code coverage report
Tests for New code lines are not detected in FirebaseFirestore-iOS, please add tests on highlighted lines.
| *DeepClone(source.array_value.values[i]).release(); | ||
| } | ||
| break; | ||
|
|
There was a problem hiding this comment.
Incremental code coverage report
Tests for New code lines are not detected in FirebaseFirestore-iOS, please add tests on highlighted lines.
| MaybeDocument VerifyMutation::Rep::ApplyToRemoteDocument( | ||
| const absl::optional<MaybeDocument>&, const MutationResult&) const { | ||
| void VerifyMutation::Rep::ApplyToRemoteDocument(MutableDocument&, | ||
| const MutationResult&) const { |
There was a problem hiding this comment.
Incremental code coverage report
Tests for New code lines are not detected in FirebaseFirestore-iOS, please add tests on highlighted lines.
| absl::optional<MaybeDocument> VerifyMutation::Rep::ApplyToLocalView( | ||
| const absl::optional<MaybeDocument>&, const Timestamp&) const { | ||
| void VerifyMutation::Rep::ApplyToLocalView(MutableDocument&, | ||
| const Timestamp&) const { |
There was a problem hiding this comment.
Incremental code coverage report
Tests for New code lines are not detected in FirebaseFirestore-iOS, please add tests on highlighted lines.
| return FieldFilter::Create({}, {}, | ||
| MakeSharedMessage(google_firestore_v1_Value{})); |
There was a problem hiding this comment.
Incremental code coverage report
Tests for New code lines are not detected in FirebaseFirestore-iOS, please add tests on highlighted lines.
| // TODO(rsgowman): implement. | ||
| // In particular, since this statement isn't hit, it implies a missing | ||
| // test for UnknownDocument. However, we'll defer that until after | ||
| // nanopb-master is merged to master. | ||
| abort(); | ||
| } else { | ||
| FAIL() << "We somehow created an invalid model object"; |
There was a problem hiding this comment.
Incremental code coverage report
Tests for New code lines are not detected in FirebaseFirestore-iOS, please add tests on highlighted lines.
| Message<google_firestore_v1_MapValue> value) { | ||
| Message<google_firestore_v1_Value> result; | ||
| result->which_value_type = google_firestore_v1_Value_map_value_tag; | ||
| result->map_value = *value.release(); | ||
| return result; | ||
| } |
There was a problem hiding this comment.
Incremental code coverage report
Tests for New code lines are not detected in FirebaseFirestore-iOS, please add tests on highlighted lines.
| Message<google_firestore_v1_Value> Value(const model::ObjectValue& value) { | ||
| return DeepClone(value.Get()); |
There was a problem hiding this comment.
Incremental code coverage report
Tests for New code lines are not detected in FirebaseFirestore-iOS, please add tests on highlighted lines.
| nanopb::MakeStringView(value.string_value) != kDeleteSentinel) { | ||
| object_value.Set(field_path, DeepClone(value)); | ||
| } else if (nanopb::MakeStringView(value.string_value) == kDeleteSentinel) { | ||
| object_value.Delete(field_path); |
There was a problem hiding this comment.
Incremental code coverage report
Tests for New code lines are not detected in FirebaseFirestore-iOS, please add tests on highlighted lines.
This PR removes the FieldValue classes and updates the client to directly use Protobuf messages internally. It aligns the iOS client with the Web and Android client.
CPU time benchmark On iPhone 8:
Full results (internal): go/fieldvalue-benchmark
XCode's Leak Detector does not detect any leaks with this PR when used in FriendlyEats.