From a34b0b1cf54d6af636e4263837a07baf47d41992 Mon Sep 17 00:00:00 2001 From: rsgowman Date: Tue, 22 May 2018 14:45:03 -0400 Subject: [De]serialize non-empty Document instances (#1284) * [De]serialize non-empty Document instances Still TODO: - NoDocument - ErrorHandling --- .../src/firebase/firestore/remote/serializer.cc | 56 ++++++++++++------ .../firebase/firestore/remote/serializer_test.cc | 67 +++++++++++++++++++--- 2 files changed, 96 insertions(+), 27 deletions(-) (limited to 'Firestore/core') diff --git a/Firestore/core/src/firebase/firestore/remote/serializer.cc b/Firestore/core/src/firebase/firestore/remote/serializer.cc index 26a06c6..3e7212c 100644 --- a/Firestore/core/src/firebase/firestore/remote/serializer.cc +++ b/Firestore/core/src/firebase/firestore/remote/serializer.cc @@ -307,6 +307,12 @@ ObjectValue::Map::value_type DecodeMapValueFieldsEntry(Reader* reader) { google_firestore_v1beta1_MapValue_FieldsEntry_value_tag); } +ObjectValue::Map::value_type DecodeDocumentFieldsEntry(Reader* reader) { + return DecodeFieldsEntry( + reader, google_firestore_v1beta1_Document_FieldsEntry_key_tag, + google_firestore_v1beta1_Document_FieldsEntry_value_tag); +} + void EncodeObjectMap(Writer* writer, const ObjectValue::Map& object_value_map, uint32_t map_tag, @@ -346,15 +352,19 @@ ObjectValue::Map DecodeMapValue(Reader* reader) { reader->ReadNestedMessage( DecodeMapValueFieldsEntry); - // Sanity check: ensure that this key doesn't already exist in the - // map. - // TODO(rsgowman): figure out error handling: We can do better than a - // failed assertion. if (!reader->status().ok()) return result; - FIREBASE_ASSERT(result.find(fv.first) == result.end()); + + // Assumption: If we parse two entries for the map that have the same key, + // then the latter should overwrite the former. This does not appear to be + // explicitly called out by the docs, but seems to be in the spirit of how + // things work. (i.e. non-repeated fields explicitly follow this behaviour.) + // In any case, well behaved proto emitters shouldn't create encodings like + // this, but well behaved parsers are expected to handle these cases. + // + // https://developers.google.com/protocol-buffers/docs/encoding#optional // Add this key,fieldvalue to the results map. - result.emplace(std::move(fv)); + result[fv.first] = fv.second; } return result; } @@ -467,11 +477,10 @@ void Serializer::EncodeDocument(Writer* writer, // Encode Document.fields (unless it's empty) if (!object_value.internal_value.empty()) { - // TODO(rsgowman): add support for documents with contents. (This - // implementation is wrong, but will be corrected in the next PR.) - writer->WriteTag( - {PB_WT_STRING, google_firestore_v1beta1_Document_fields_tag}); - EncodeMapValue(writer, object_value); + EncodeObjectMap(writer, object_value.internal_value, + google_firestore_v1beta1_Document_fields_tag, + google_firestore_v1beta1_Document_FieldsEntry_key_tag, + google_firestore_v1beta1_Document_FieldsEntry_value_tag); } // Skip Document.create_time and Document.update_time, since they're @@ -540,7 +549,7 @@ std::unique_ptr Serializer::DecodeDocument(Reader* reader) const { if (!reader->status().ok()) return nullptr; std::string name; - FieldValue fields = FieldValue::ObjectValueFromMap({}); + ObjectValue::Map fields_internal; SnapshotVersion version = SnapshotVersion::None(); while (reader->bytes_left()) { @@ -551,11 +560,20 @@ std::unique_ptr Serializer::DecodeDocument(Reader* reader) const { case google_firestore_v1beta1_Document_name_tag: name = reader->ReadString(); break; - case google_firestore_v1beta1_Document_fields_tag: - // TODO(rsgowman): Rather than overwriting, we should instead merge with - // the existing FieldValue (if any). - fields = DecodeFieldValueImpl(reader); + case google_firestore_v1beta1_Document_fields_tag: { + ObjectValue::Map::value_type fv = + reader->ReadNestedMessage( + DecodeDocumentFieldsEntry); + + if (!reader->status().ok()) return nullptr; + + // Assumption: For duplicates, the latter overrides the former, see + // comment on writing object map for details (DecodeMapValue). + + // Add fieldvalue to the results map. + fields_internal[fv.first] = fv.second; break; + } case google_firestore_v1beta1_Document_create_time_tag: // This field is ignored by the client sdk, but we still need to extract // it. @@ -576,9 +594,9 @@ std::unique_ptr Serializer::DecodeDocument(Reader* reader) const { } } - return absl::make_unique(std::move(fields), DecodeKey(name), - version, - /*has_local_modifications=*/false); + return absl::make_unique( + FieldValue::ObjectValueFromMap(fields_internal), DecodeKey(name), version, + /*has_local_modifications=*/false); } } // namespace remote diff --git a/Firestore/core/test/firebase/firestore/remote/serializer_test.cc b/Firestore/core/test/firebase/firestore/remote/serializer_test.cc index a147309..f15363d 100644 --- a/Firestore/core/test/firebase/firestore/remote/serializer_test.cc +++ b/Firestore/core/test/firebase/firestore/remote/serializer_test.cc @@ -219,6 +219,29 @@ class SerializerTest : public ::testing::Test { return proto; } + /** + * Creates entries in the proto that we don't care about. + * + * We ignore create time in our serializer. We never set it, and never read it + * (other than to throw it away). But the server could (and probably does) set + * it, so we need to be able to discard it properly. The ExpectRoundTrip deals + * with this asymmetry. + * + * This method adds these ignored fields to the proto. + */ + void TouchIgnoredBatchGetDocumentsResponseFields( + google::firestore::v1beta1::BatchGetDocumentsResponse* proto) { + // TODO(rsgowman): This method currently assumes that this is a 'found' + // document. We (probably) will need to adjust this to work with NoDocuments + // too. + google::firestore::v1beta1::Document* doc_proto = proto->mutable_found(); + + google::protobuf::Timestamp* create_time_proto = + doc_proto->mutable_create_time(); + create_time_proto->set_seconds(8765); + create_time_proto->set_nanos(4321); + } + private: void ExpectSerializationRoundTrip( const FieldValue& model, @@ -618,17 +641,45 @@ TEST_F(SerializerTest, EncodesEmptyDocument) { update_time_proto->set_seconds(1234); update_time_proto->set_nanos(5678); - // Note that we ignore create time in our serializer. We never set it, and - // never read it (other than to throw it away). But the server could (and - // probably does) set it, so we need to be able to discard it properly. The - // ExpectRoundTrip deals with this asymmetry. - google::protobuf::Timestamp* create_time_proto = - doc_proto->mutable_create_time(); - create_time_proto->set_seconds(8765); - create_time_proto->set_nanos(4321); + TouchIgnoredBatchGetDocumentsResponseFields(&proto); ExpectRoundTrip(key, empty_value, update_time, proto); } +TEST_F(SerializerTest, EncodesNonEmptyDocument) { + DocumentKey key = DocumentKey::FromPathString("path/to/the/doc"); + FieldValue fields = FieldValue::ObjectValueFromMap({ + {"foo", FieldValue::StringValue("bar")}, + {"two", FieldValue::IntegerValue(2)}, + {"nested", FieldValue::ObjectValueFromMap({ + {"fourty-two", FieldValue::IntegerValue(42)}, + })}, + }); + SnapshotVersion update_time = SnapshotVersion{{1234, 5678}}; + + google::firestore::v1beta1::Value inner_proto; + google::protobuf::Map& + inner_fields = *inner_proto.mutable_map_value()->mutable_fields(); + inner_fields["fourty-two"] = ValueProto(int64_t{42}); + + google::firestore::v1beta1::BatchGetDocumentsResponse proto; + google::firestore::v1beta1::Document* doc_proto = proto.mutable_found(); + doc_proto->set_name(serializer.EncodeKey(key)); + google::protobuf::Map& m = + *doc_proto->mutable_fields(); + m["foo"] = ValueProto("bar"); + m["two"] = ValueProto(int64_t{2}); + m["nested"] = inner_proto; + + google::protobuf::Timestamp* update_time_proto = + doc_proto->mutable_update_time(); + update_time_proto->set_seconds(1234); + update_time_proto->set_nanos(5678); + + TouchIgnoredBatchGetDocumentsResponseFields(&proto); + + ExpectRoundTrip(key, fields, update_time, proto); +} + // TODO(rsgowman): Test [en|de]coding multiple protos into the same output // vector. -- cgit v1.2.3