aboutsummaryrefslogtreecommitdiffhomepage
path: root/Firestore
diff options
context:
space:
mode:
authorGravatar rsgowman <rgowman@google.com>2018-05-22 14:45:03 -0400
committerGravatar GitHub <noreply@github.com>2018-05-22 14:45:03 -0400
commita34b0b1cf54d6af636e4263837a07baf47d41992 (patch)
tree328f1448348b2eabfcbcdab9fbc34e0d7a7db3ec /Firestore
parentc6d5b68b086b595f91e8a586e98b8a70b2b118aa (diff)
[De]serialize non-empty Document instances (#1284)
* [De]serialize non-empty Document instances Still TODO: - NoDocument - ErrorHandling
Diffstat (limited to 'Firestore')
-rw-r--r--Firestore/core/src/firebase/firestore/remote/serializer.cc56
-rw-r--r--Firestore/core/test/firebase/firestore/remote/serializer_test.cc67
2 files changed, 96 insertions, 27 deletions
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<ObjectValue::Map::value_type>(
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<Document> 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<Document> 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<ObjectValue::Map::value_type>(
+ 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<Document> Serializer::DecodeDocument(Reader* reader) const {
}
}
- return absl::make_unique<Document>(std::move(fields), DecodeKey(name),
- version,
- /*has_local_modifications=*/false);
+ return absl::make_unique<Document>(
+ 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<std::string, google::firestore::v1beta1::Value>&
+ 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<std::string, google::firestore::v1beta1::Value>& 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.