From 16d28a2645808cca34870392c9e972b15627ae9e Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Fri, 7 Dec 2018 11:10:48 -0500 Subject: [PATCH 1/3] Port DocumentState and UnknownDocument. Part of heldwriteacks. Serialization work for this is largely deferred until after nanopb-master is merged with master. --- .../firestore/local/local_serializer.cc | 12 +++- .../firebase/firestore/model/CMakeLists.txt | 2 + .../src/firebase/firestore/model/document.cc | 6 +- .../src/firebase/firestore/model/document.h | 29 +++++++- .../firebase/firestore/model/maybe_document.h | 7 ++ .../src/firebase/firestore/model/mutation.cc | 10 +-- .../firebase/firestore/model/no_document.cc | 7 +- .../firebase/firestore/model/no_document.h | 11 +++- .../firestore/model/unknown_document.cc | 32 +++++++++ .../firestore/model/unknown_document.h | 43 ++++++++++++ .../firebase/firestore/remote/serializer.cc | 8 ++- .../firebase/firestore/model/CMakeLists.txt | 1 - .../firebase/firestore/model/document_test.cc | 47 ++++++++----- .../firestore/model/maybe_document_test.cc | 66 ------------------- .../firebase/firestore/model/mutation_test.cc | 12 ++-- .../firestore/model/no_document_test.cc | 14 ++-- .../firestore/remote/serializer_test.cc | 9 ++- .../firebase/firestore/testutil/testutil.h | 14 ++-- 18 files changed, 206 insertions(+), 124 deletions(-) create mode 100644 Firestore/core/src/firebase/firestore/model/unknown_document.cc create mode 100644 Firestore/core/src/firebase/firestore/model/unknown_document.h delete mode 100644 Firestore/core/test/firebase/firestore/model/maybe_document_test.cc diff --git a/Firestore/core/src/firebase/firestore/local/local_serializer.cc b/Firestore/core/src/firebase/firestore/local/local_serializer.cc index 97246d90d61..99154967460 100644 --- a/Firestore/core/src/firebase/firestore/local/local_serializer.cc +++ b/Firestore/core/src/firebase/firestore/local/local_serializer.cc @@ -64,8 +64,12 @@ void LocalSerializer::EncodeMaybeDocument( }); return; + case MaybeDocument::Type::UnknownDocument: + // TODO(rsgowman): Implement + abort(); + case MaybeDocument::Type::Unknown: - // TODO(rsgowman) + // TODO(rsgowman): Error handling abort(); } @@ -168,8 +172,12 @@ std::unique_ptr LocalSerializer::DecodeNoDocument( } if (!reader->status().ok()) return nullptr; + // TODO(rsgowman): Fix hardcoding of has_committed_mutations. + // Instead, we should grab this from the proto (see other ports). However, + // we'll defer until the nanopb-master gets merged to master. return absl::make_unique(rpc_serializer_.DecodeKey(name), - *std::move(version)); + *std::move(version), + /*has_committed_mutations=*/false); } void LocalSerializer::EncodeQueryData(Writer* writer, diff --git a/Firestore/core/src/firebase/firestore/model/CMakeLists.txt b/Firestore/core/src/firebase/firestore/model/CMakeLists.txt index 4a230eefe52..15fb6a09157 100644 --- a/Firestore/core/src/firebase/firestore/model/CMakeLists.txt +++ b/Firestore/core/src/firebase/firestore/model/CMakeLists.txt @@ -42,6 +42,8 @@ cc_library( snapshot_version.h transform_operations.h types.h + unknown_document.cc + unknown_document.h DEPENDS absl_optional absl_strings diff --git a/Firestore/core/src/firebase/firestore/model/document.cc b/Firestore/core/src/firebase/firestore/model/document.cc index 7adf684b241..06deb50576b 100644 --- a/Firestore/core/src/firebase/firestore/model/document.cc +++ b/Firestore/core/src/firebase/firestore/model/document.cc @@ -27,10 +27,10 @@ namespace model { Document::Document(FieldValue&& data, DocumentKey key, SnapshotVersion version, - bool has_local_mutations) + DocumentState document_state) : MaybeDocument(std::move(key), std::move(version)), data_(std::move(data)), - has_local_mutations_(has_local_mutations) { + document_state_(document_state) { set_type(Type::Document); HARD_ASSERT(FieldValue::Type::Object == data.type()); } @@ -41,7 +41,7 @@ bool Document::Equals(const MaybeDocument& other) const { } auto& other_doc = static_cast(other); return MaybeDocument::Equals(other) && - has_local_mutations_ == other_doc.has_local_mutations_ && + document_state_ == other_doc.document_state_ && data_ == other_doc.data_; } diff --git a/Firestore/core/src/firebase/firestore/model/document.h b/Firestore/core/src/firebase/firestore/model/document.h index 1b7cc1388f3..2ccbe3807c1 100644 --- a/Firestore/core/src/firebase/firestore/model/document.h +++ b/Firestore/core/src/firebase/firestore/model/document.h @@ -26,6 +26,21 @@ namespace firebase { namespace firestore { namespace model { +enum class DocumentState { + /** + * Local mutations applied via the mutation queue. Document is potentially + * inconsistent. + */ + kLocalMutations, + /** + * Mutations applied based on a write acknowledgment. Document is potentially + * inconsistent. + */ + kCommittedMutations, + /** No mutations applied. Document was sent to us by Watch. */ + kSynced, +}; + /** * Represents a document in Firestore with a key, version, data and whether the * data has local mutations applied to it. @@ -38,7 +53,7 @@ class Document : public MaybeDocument { Document(FieldValue&& data, DocumentKey key, SnapshotVersion version, - bool has_local_mutations); + DocumentState document_state); const FieldValue& data() const { return data_; @@ -49,7 +64,15 @@ class Document : public MaybeDocument { } bool has_local_mutations() const { - return has_local_mutations_; + return document_state_ == DocumentState::kLocalMutations; + } + + bool has_committed_mutations() const { + return document_state_ == DocumentState::kCommittedMutations; + } + + bool HasPendingWrites() const override { + return has_local_mutations() || has_committed_mutations(); } protected: @@ -57,7 +80,7 @@ class Document : public MaybeDocument { private: FieldValue data_; // This is of type Object. - bool has_local_mutations_; + DocumentState document_state_; }; /** Compares against another Document. */ diff --git a/Firestore/core/src/firebase/firestore/model/maybe_document.h b/Firestore/core/src/firebase/firestore/model/maybe_document.h index c2ffb86a0de..2db605baad7 100644 --- a/Firestore/core/src/firebase/firestore/model/maybe_document.h +++ b/Firestore/core/src/firebase/firestore/model/maybe_document.h @@ -41,6 +41,7 @@ class MaybeDocument { Unknown, Document, NoDocument, + UnknownDocument, }; MaybeDocument(DocumentKey key, SnapshotVersion version); @@ -66,6 +67,12 @@ class MaybeDocument { return version_; } + /** + * Whether this document has a local mutation applied that has not yet been + * acknowledged by Watch. + */ + virtual bool HasPendingWrites() const = 0; + protected: // Only allow subclass to set their types. void set_type(Type type) { diff --git a/Firestore/core/src/firebase/firestore/model/mutation.cc b/Firestore/core/src/firebase/firestore/model/mutation.cc index a2577221ce7..17bbe0fc421 100644 --- a/Firestore/core/src/firebase/firestore/model/mutation.cc +++ b/Firestore/core/src/firebase/firestore/model/mutation.cc @@ -65,7 +65,7 @@ std::shared_ptr SetMutation::ApplyToLocalView( SnapshotVersion version = GetPostMutationVersion(maybe_doc.get()); return absl::make_unique(FieldValue(value_), key(), version, - /*has_local_mutations=*/true); + DocumentState::kLocalMutations); } PatchMutation::PatchMutation(DocumentKey&& key, @@ -84,17 +84,13 @@ std::shared_ptr PatchMutation::ApplyToLocalView( VerifyKeyMatches(maybe_doc.get()); if (!precondition().IsValidFor(maybe_doc.get())) { - if (maybe_doc) { - return absl::make_unique(maybe_doc->key(), - maybe_doc->version()); - } - return nullptr; + return maybe_doc; } SnapshotVersion version = GetPostMutationVersion(maybe_doc.get()); FieldValue new_data = PatchDocument(maybe_doc.get()); return absl::make_unique(std::move(new_data), key(), version, - /*has_local_mutations=*/true); + DocumentState::kLocalMutations); } FieldValue PatchMutation::PatchDocument(const MaybeDocument* maybe_doc) const { diff --git a/Firestore/core/src/firebase/firestore/model/no_document.cc b/Firestore/core/src/firebase/firestore/model/no_document.cc index 98cb428843b..5f348af4ea3 100644 --- a/Firestore/core/src/firebase/firestore/model/no_document.cc +++ b/Firestore/core/src/firebase/firestore/model/no_document.cc @@ -22,8 +22,11 @@ namespace firebase { namespace firestore { namespace model { -NoDocument::NoDocument(DocumentKey key, SnapshotVersion version) - : MaybeDocument(std::move(key), std::move(version)) { +NoDocument::NoDocument(DocumentKey key, + SnapshotVersion version, + bool has_committed_mutations) + : MaybeDocument(std::move(key), std::move(version)), + has_committed_mutations_(has_committed_mutations) { set_type(Type::NoDocument); } diff --git a/Firestore/core/src/firebase/firestore/model/no_document.h b/Firestore/core/src/firebase/firestore/model/no_document.h index 7cfd47c66cc..da2cac0f4e6 100644 --- a/Firestore/core/src/firebase/firestore/model/no_document.h +++ b/Firestore/core/src/firebase/firestore/model/no_document.h @@ -26,7 +26,16 @@ namespace model { /** Represents that no documents exists for the key at the given version. */ class NoDocument : public MaybeDocument { public: - NoDocument(DocumentKey key, SnapshotVersion version); + NoDocument(DocumentKey key, + SnapshotVersion version, + bool has_committed_mutations); + + bool HasPendingWrites() const override { + return has_committed_mutations_; + } + + private: + bool has_committed_mutations_; }; } // namespace model diff --git a/Firestore/core/src/firebase/firestore/model/unknown_document.cc b/Firestore/core/src/firebase/firestore/model/unknown_document.cc new file mode 100644 index 00000000000..ea42c577c6f --- /dev/null +++ b/Firestore/core/src/firebase/firestore/model/unknown_document.cc @@ -0,0 +1,32 @@ +/* + * Copyright 2018 Google + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "Firestore/core/src/firebase/firestore/model/unknown_document.h" + +#include + +namespace firebase { +namespace firestore { +namespace model { + +UnknownDocument::UnknownDocument(DocumentKey key, SnapshotVersion version) + : MaybeDocument(std::move(key), std::move(version)) { + set_type(Type::UnknownDocument); +} + +} // namespace model +} // namespace firestore +} // namespace firebase diff --git a/Firestore/core/src/firebase/firestore/model/unknown_document.h b/Firestore/core/src/firebase/firestore/model/unknown_document.h new file mode 100644 index 00000000000..fb031c74320 --- /dev/null +++ b/Firestore/core/src/firebase/firestore/model/unknown_document.h @@ -0,0 +1,43 @@ +/* + * Copyright 2018 Google + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#ifndef FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_MODEL_UNKNOWN_DOCUMENT_H_ +#define FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_MODEL_UNKNOWN_DOCUMENT_H_ + +#include "Firestore/core/src/firebase/firestore/model/maybe_document.h" + +namespace firebase { +namespace firestore { +namespace model { + +/** + * A class representing an existing document whose data is unknown (e.g. a + * document that was updated without a known base document). + */ +class UnknownDocument : public MaybeDocument { + public: + UnknownDocument(DocumentKey key, SnapshotVersion version); + + bool HasPendingWrites() const override { + return true; + } +}; + +} // namespace model +} // namespace firestore +} // namespace firebase + +#endif // FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_MODEL_UNKNOWN_DOCUMENT_H_ diff --git a/Firestore/core/src/firebase/firestore/remote/serializer.cc b/Firestore/core/src/firebase/firestore/remote/serializer.cc index f588133c126..4fd71c10737 100644 --- a/Firestore/core/src/firebase/firestore/remote/serializer.cc +++ b/Firestore/core/src/firebase/firestore/remote/serializer.cc @@ -48,6 +48,7 @@ using firebase::firestore::core::Query; using firebase::firestore::model::DatabaseId; using firebase::firestore::model::Document; using firebase::firestore::model::DocumentKey; +using firebase::firestore::model::DocumentState; using firebase::firestore::model::FieldValue; using firebase::firestore::model::MaybeDocument; using firebase::firestore::model::NoDocument; @@ -497,8 +498,9 @@ std::unique_ptr Serializer::DecodeBatchGetDocumentsResponse( } else if (found != nullptr) { return found; } else if (!missing.empty()) { - return absl::make_unique( - DecodeKey(missing), SnapshotVersion{*std::move(read_time)}); + return absl::make_unique(DecodeKey(missing), + SnapshotVersion{*std::move(read_time)}, + /*has_committed_mutations=*/false); } else { reader->Fail( "Invalid BatchGetDocumentsReponse message: " @@ -551,7 +553,7 @@ std::unique_ptr Serializer::DecodeDocument(Reader* reader) const { if (!reader->status().ok()) return nullptr; return absl::make_unique(FieldValue::FromMap(fields_internal), DecodeKey(name), *std::move(version), - /*has_local_modifications=*/false); + DocumentState::kSynced); } void Serializer::EncodeQueryTarget(Writer* writer, diff --git a/Firestore/core/test/firebase/firestore/model/CMakeLists.txt b/Firestore/core/test/firebase/firestore/model/CMakeLists.txt index 9d872d49a01..e90c0c73d35 100644 --- a/Firestore/core/test/firebase/firestore/model/CMakeLists.txt +++ b/Firestore/core/test/firebase/firestore/model/CMakeLists.txt @@ -21,7 +21,6 @@ cc_test( field_mask_test.cc field_path_test.cc field_value_test.cc - maybe_document_test.cc mutation_test.cc no_document_test.cc precondition_test.cc diff --git a/Firestore/core/test/firebase/firestore/model/document_test.cc b/Firestore/core/test/firebase/firestore/model/document_test.cc index 5955b854312..fe9d54f3b57 100644 --- a/Firestore/core/test/firebase/firestore/model/document_test.cc +++ b/Firestore/core/test/firebase/firestore/model/document_test.cc @@ -16,6 +16,8 @@ #include "Firestore/core/src/firebase/firestore/model/document.h" +#include "Firestore/core/src/firebase/firestore/model/unknown_document.h" + #include "absl/strings/string_view.h" #include "gtest/gtest.h" @@ -28,18 +30,18 @@ namespace { inline Document MakeDocument(const absl::string_view data, const absl::string_view path, const Timestamp& timestamp, - bool has_local_mutations) { + DocumentState document_state) { return Document( FieldValue::FromMap({{"field", FieldValue::FromString(data.data())}}), DocumentKey::FromPathString(path.data()), SnapshotVersion(timestamp), - has_local_mutations); + document_state); } } // anonymous namespace TEST(Document, Getter) { - const Document& doc = - MakeDocument("foo", "i/am/a/path", Timestamp(123, 456), true); + const Document& doc = MakeDocument("foo", "i/am/a/path", Timestamp(123, 456), + DocumentState::kLocalMutations); EXPECT_EQ(MaybeDocument::Type::Document, doc.type()); EXPECT_EQ(FieldValue::FromMap({{"field", FieldValue::FromString("foo")}}), doc.data()); @@ -49,25 +51,34 @@ TEST(Document, Getter) { } TEST(Document, Comparison) { - EXPECT_EQ(MakeDocument("foo", "i/am/a/path", Timestamp(123, 456), true), - MakeDocument("foo", "i/am/a/path", Timestamp(123, 456), true)); - EXPECT_NE(MakeDocument("foo", "i/am/a/path", Timestamp(123, 456), true), - MakeDocument("bar", "i/am/a/path", Timestamp(123, 456), true)); - EXPECT_NE( - MakeDocument("foo", "i/am/a/path", Timestamp(123, 456), true), - MakeDocument("foo", "i/am/another/path", Timestamp(123, 456), true)); - EXPECT_NE(MakeDocument("foo", "i/am/a/path", Timestamp(123, 456), true), - MakeDocument("foo", "i/am/a/path", Timestamp(456, 123), true)); - EXPECT_NE(MakeDocument("foo", "i/am/a/path", Timestamp(123, 456), true), - MakeDocument("foo", "i/am/a/path", Timestamp(123, 456), false)); + EXPECT_EQ(MakeDocument("foo", "i/am/a/path", Timestamp(123, 456), + DocumentState::kLocalMutations), + MakeDocument("foo", "i/am/a/path", Timestamp(123, 456), + DocumentState::kLocalMutations)); + EXPECT_NE(MakeDocument("foo", "i/am/a/path", Timestamp(123, 456), + DocumentState::kLocalMutations), + MakeDocument("bar", "i/am/a/path", Timestamp(123, 456), + DocumentState::kLocalMutations)); + EXPECT_NE(MakeDocument("foo", "i/am/a/path", Timestamp(123, 456), + DocumentState::kLocalMutations), + MakeDocument("foo", "i/am/another/path", Timestamp(123, 456), + DocumentState::kLocalMutations)); + EXPECT_NE(MakeDocument("foo", "i/am/a/path", Timestamp(123, 456), + DocumentState::kLocalMutations), + MakeDocument("foo", "i/am/a/path", Timestamp(456, 123), + DocumentState::kLocalMutations)); + EXPECT_NE(MakeDocument("foo", "i/am/a/path", Timestamp(123, 456), + DocumentState::kLocalMutations), + MakeDocument("foo", "i/am/a/path", Timestamp(123, 456), + DocumentState::kSynced)); // Document and MaybeDocument will not equal. In particular, Document and // NoDocument will not equal, which I won't test here. EXPECT_NE(Document(FieldValue::FromMap({}), DocumentKey::FromPathString("same/path"), - SnapshotVersion(Timestamp()), false), - MaybeDocument(DocumentKey::FromPathString("same/path"), - SnapshotVersion(Timestamp()))); + SnapshotVersion(Timestamp()), DocumentState::kSynced), + UnknownDocument(DocumentKey::FromPathString("same/path"), + SnapshotVersion(Timestamp()))); } } // namespace model diff --git a/Firestore/core/test/firebase/firestore/model/maybe_document_test.cc b/Firestore/core/test/firebase/firestore/model/maybe_document_test.cc deleted file mode 100644 index 70ae319624f..00000000000 --- a/Firestore/core/test/firebase/firestore/model/maybe_document_test.cc +++ /dev/null @@ -1,66 +0,0 @@ -/* - * Copyright 2018 Google - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -#include "Firestore/core/src/firebase/firestore/model/maybe_document.h" - -#include "absl/strings/string_view.h" -#include "gtest/gtest.h" - -namespace firebase { -namespace firestore { -namespace model { - -namespace { - -inline MaybeDocument MakeMaybeDocument(const absl::string_view path, - const Timestamp& timestamp) { - return MaybeDocument(DocumentKey::FromPathString(path.data()), - SnapshotVersion(timestamp)); -} - -inline bool operator<(const MaybeDocument& lhs, const MaybeDocument& rhs) { - static const DocumentKeyComparator less; - return less(lhs, rhs); -} - -} // anonymous namespace - -TEST(MaybeDocument, Getter) { - const MaybeDocument& doc = - MakeMaybeDocument("i/am/a/path", Timestamp(123, 456)); - EXPECT_EQ(MaybeDocument::Type::Unknown, doc.type()); - EXPECT_EQ(DocumentKey::FromPathString("i/am/a/path"), doc.key()); - EXPECT_EQ(SnapshotVersion(Timestamp(123, 456)), doc.version()); -} - -TEST(MaybeDocument, Comparison) { - EXPECT_TRUE(MakeMaybeDocument("root/123", Timestamp(456, 123)) < - MakeMaybeDocument("root/456", Timestamp(123, 456))); - // MaybeDocument comparison is purely key-based. - EXPECT_FALSE(MakeMaybeDocument("root/123", Timestamp(111, 111)) < - MakeMaybeDocument("root/123", Timestamp(222, 222))); - - EXPECT_EQ(MakeMaybeDocument("root/123", Timestamp(456, 123)), - MakeMaybeDocument("root/123", Timestamp(456, 123))); - EXPECT_NE(MakeMaybeDocument("root/123", Timestamp(456, 123)), - MakeMaybeDocument("root/456", Timestamp(456, 123))); - EXPECT_NE(MakeMaybeDocument("root/123", Timestamp(456, 123)), - MakeMaybeDocument("root/123", Timestamp(123, 456))); -} - -} // namespace model -} // namespace firestore -} // namespace firebase diff --git a/Firestore/core/test/firebase/firestore/model/mutation_test.cc b/Firestore/core/test/firebase/firestore/model/mutation_test.cc index fd0e0b8ccad..cfd2a7fa62e 100644 --- a/Firestore/core/test/firebase/firestore/model/mutation_test.cc +++ b/Firestore/core/test/firebase/firestore/model/mutation_test.cc @@ -47,7 +47,7 @@ TEST(Mutation, AppliesSetsToDocuments) { ASSERT_EQ(set_doc->type(), MaybeDocument::Type::Document); EXPECT_EQ(*set_doc.get(), Doc("collection/key", 0, {{"bar", FieldValue::FromString("bar-value")}}, - /*has_local_mutations=*/true)); + DocumentState::kLocalMutations)); } TEST(Mutation, AppliesPatchToDocuments) { @@ -68,7 +68,7 @@ TEST(Mutation, AppliesPatchToDocuments) { {{"foo", FieldValue::FromMap( {{"bar", FieldValue::FromString("new-bar-value")}})}, {"baz", FieldValue::FromString("baz-value")}}, - /*has_local_mutations=*/true)); + DocumentState::kLocalMutations)); } TEST(Mutation, AppliesPatchWithMergeToDocuments) { @@ -85,7 +85,7 @@ TEST(Mutation, AppliesPatchWithMergeToDocuments) { Doc("collection/key", 0, {{"foo", FieldValue::FromMap( {{"bar", FieldValue::FromString("new-bar-value")}})}}, - /*has_local_mutations=*/true)); + DocumentState::kLocalMutations)); } TEST(Mutation, AppliesPatchToNullDocWithMergeToDocuments) { @@ -102,7 +102,7 @@ TEST(Mutation, AppliesPatchToNullDocWithMergeToDocuments) { Doc("collection/key", 0, {{"foo", FieldValue::FromMap( {{"bar", FieldValue::FromString("new-bar-value")}})}}, - /*has_local_mutations=*/true)); + DocumentState::kLocalMutations)); } TEST(Mutation, DeletesValuesFromTheFieldMask) { @@ -122,7 +122,7 @@ TEST(Mutation, DeletesValuesFromTheFieldMask) { Doc("collection/key", 0, {{"foo", FieldValue::FromMap( {{"baz", FieldValue::FromString("baz-value")}})}}, - /*has_local_mutations=*/true)); + DocumentState::kLocalMutations)); } TEST(Mutation, PatchesPrimitiveValue) { @@ -143,7 +143,7 @@ TEST(Mutation, PatchesPrimitiveValue) { {{"foo", FieldValue::FromMap( {{"bar", FieldValue::FromString("new-bar-value")}})}, {"baz", FieldValue::FromString("baz-value")}}, - /*has_local_mutations=*/true)); + DocumentState::kLocalMutations)); } } // namespace model diff --git a/Firestore/core/test/firebase/firestore/model/no_document_test.cc b/Firestore/core/test/firebase/firestore/model/no_document_test.cc index 825820f068c..974ddbf3a31 100644 --- a/Firestore/core/test/firebase/firestore/model/no_document_test.cc +++ b/Firestore/core/test/firebase/firestore/model/no_document_test.cc @@ -16,6 +16,8 @@ #include "Firestore/core/src/firebase/firestore/model/no_document.h" +#include "Firestore/core/src/firebase/firestore/model/unknown_document.h" + #include "absl/strings/string_view.h" #include "gtest/gtest.h" @@ -28,7 +30,8 @@ namespace { inline NoDocument MakeNoDocument(const absl::string_view path, const Timestamp& timestamp) { return NoDocument(DocumentKey::FromPathString(path.data()), - SnapshotVersion(timestamp)); + SnapshotVersion(timestamp), + /*has_committed_mutations=*/false); } } // anonymous namespace @@ -39,11 +42,12 @@ TEST(NoDocument, Getter) { EXPECT_EQ(DocumentKey::FromPathString("i/am/a/path"), doc.key()); EXPECT_EQ(SnapshotVersion(Timestamp(123, 456)), doc.version()); - // NoDocument and MaybeDocument will not equal. + // NoDocument and UnknownDocument will not equal. EXPECT_NE(NoDocument(DocumentKey::FromPathString("same/path"), - SnapshotVersion(Timestamp())), - MaybeDocument(DocumentKey::FromPathString("same/path"), - SnapshotVersion(Timestamp()))); + SnapshotVersion(Timestamp()), + /*has_committed_mutations=*/false), + UnknownDocument(DocumentKey::FromPathString("same/path"), + SnapshotVersion(Timestamp()))); } } // namespace model diff --git a/Firestore/core/test/firebase/firestore/remote/serializer_test.cc b/Firestore/core/test/firebase/firestore/remote/serializer_test.cc index 483bcb7cf3d..4250388911f 100644 --- a/Firestore/core/test/firebase/firestore/remote/serializer_test.cc +++ b/Firestore/core/test/firebase/firestore/remote/serializer_test.cc @@ -357,6 +357,12 @@ class SerializerTest : public ::testing::Test { case MaybeDocument::Type::NoDocument: EXPECT_FALSE(value.has_value()); break; + case MaybeDocument::Type::UnknownDocument: + // 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(); case MaybeDocument::Type::Unknown: FAIL() << "We somehow created an invalid model object"; } @@ -888,7 +894,8 @@ TEST_F(SerializerTest, // Ensure the decoded model is as expected. NoDocument expected_model = - NoDocument(Key("one/two"), SnapshotVersion::None()); + NoDocument(Key("one/two"), SnapshotVersion::None(), + /*has_committed_mutations=*/false); EXPECT_EQ(expected_model, *actual_model); } diff --git a/Firestore/core/test/firebase/firestore/testutil/testutil.h b/Firestore/core/test/firebase/firestore/testutil/testutil.h index 4f35472f768..ff2da5a6e1d 100644 --- a/Firestore/core/test/firebase/firestore/testutil/testutil.h +++ b/Firestore/core/test/firebase/firestore/testutil/testutil.h @@ -76,16 +76,18 @@ inline model::SnapshotVersion Version(int64_t version) { return model::SnapshotVersion{Timestamp::FromTimePoint(timepoint)}; } -inline model::Document Doc(absl::string_view key, - int64_t version = 0, - const model::ObjectValue::Map& data = {}, - bool has_local_mutations = false) { +inline model::Document Doc( + absl::string_view key, + int64_t version = 0, + const model::ObjectValue::Map& data = {}, + model::DocumentState document_state = model::DocumentState::kSynced) { return model::Document{model::FieldValue::FromMap(data), Key(key), - Version(version), has_local_mutations}; + Version(version), document_state}; } inline model::NoDocument DeletedDoc(absl::string_view key, int64_t version) { - return model::NoDocument{Key(key), Version(version)}; + return model::NoDocument{Key(key), Version(version), + /*has_committed_mutations=*/false}; } inline core::RelationFilter::Operator OperatorFromString(absl::string_view s) { From faf0390259d642623cbf0271506b706fe0c492fc Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Fri, 7 Dec 2018 12:22:56 -0500 Subject: [PATCH 2/3] sync_project.rb --- .../Firestore.xcodeproj/project.pbxproj | 28 ++++++++----------- 1 file changed, 11 insertions(+), 17 deletions(-) diff --git a/Firestore/Example/Firestore.xcodeproj/project.pbxproj b/Firestore/Example/Firestore.xcodeproj/project.pbxproj index edbdbad2c49..72a0bd9f734 100644 --- a/Firestore/Example/Firestore.xcodeproj/project.pbxproj +++ b/Firestore/Example/Firestore.xcodeproj/project.pbxproj @@ -187,7 +187,6 @@ AB380D04201BC6E400D97691 /* ordered_code_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = AB380D03201BC6E400D97691 /* ordered_code_test.cc */; }; AB38D93020236E21000A432D /* database_info_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = AB38D92E20235D22000A432D /* database_info_test.cc */; }; AB6B908420322E4D00CC290A /* document_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = AB6B908320322E4D00CC290A /* document_test.cc */; }; - AB6B908620322E6D00CC290A /* maybe_document_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = AB6B908520322E6D00CC290A /* maybe_document_test.cc */; }; AB6B908820322E8800CC290A /* no_document_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = AB6B908720322E8800CC290A /* no_document_test.cc */; }; AB7BAB342012B519001E0872 /* geo_point_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = AB7BAB332012B519001E0872 /* geo_point_test.cc */; }; ABA495BB202B7E80008A7851 /* snapshot_version_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = ABA495B9202B7E79008A7851 /* snapshot_version_test.cc */; }; @@ -203,7 +202,6 @@ B65D34A9203C995B0076A5E1 /* FIRTimestampTest.m in Sources */ = {isa = PBXBuildFile; fileRef = B65D34A7203C99090076A5E1 /* FIRTimestampTest.m */; }; B66D8996213609EE0086DA0C /* stream_test.mm in Sources */ = {isa = PBXBuildFile; fileRef = B66D8995213609EE0086DA0C /* stream_test.mm */; }; B67BF449216EB43000CA9097 /* create_noop_connectivity_monitor.cc in Sources */ = {isa = PBXBuildFile; fileRef = B67BF448216EB43000CA9097 /* create_noop_connectivity_monitor.cc */; }; - B67BF44A216EB43000CA9097 /* create_noop_connectivity_monitor.cc in Sources */ = {isa = PBXBuildFile; fileRef = B67BF448216EB43000CA9097 /* create_noop_connectivity_monitor.cc */; }; B686F2AF2023DDEE0028D6BE /* field_path_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = B686F2AD2023DDB20028D6BE /* field_path_test.cc */; }; B686F2B22025000D0028D6BE /* resource_path_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = B686F2B02024FFD70028D6BE /* resource_path_test.cc */; }; B6BBE43121262CF400C6A53E /* grpc_stream_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = B6BBE42F21262CF400C6A53E /* grpc_stream_test.cc */; }; @@ -505,7 +503,6 @@ AB38D9342023966E000A432D /* credentials_provider_test.cc */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = credentials_provider_test.cc; sourceTree = ""; }; AB38D93620239689000A432D /* empty_credentials_provider_test.cc */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = empty_credentials_provider_test.cc; sourceTree = ""; }; AB6B908320322E4D00CC290A /* document_test.cc */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; path = document_test.cc; sourceTree = ""; }; - AB6B908520322E6D00CC290A /* maybe_document_test.cc */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; path = maybe_document_test.cc; sourceTree = ""; }; AB6B908720322E8800CC290A /* no_document_test.cc */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; path = no_document_test.cc; sourceTree = ""; }; AB71064B201FA60300344F18 /* database_id_test.cc */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = database_id_test.cc; sourceTree = ""; }; AB7BAB332012B519001E0872 /* geo_point_test.cc */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = geo_point_test.cc; sourceTree = ""; }; @@ -665,12 +662,12 @@ 546854A720A3681B004BDBD5 /* remote */ = { isa = PBXGroup; children = ( - B6D964942163E63900EB9CFB /* grpc_unary_call_test.cc */, - B6D9649021544D4F00EB9CFB /* grpc_connection_test.cc */, - B6D964922154AB8F00EB9CFB /* grpc_streaming_reader_test.cc */, 546854A820A36867004BDBD5 /* datastore_test.mm */, B6D1B68420E2AB1A00B35856 /* exponential_backoff_test.cc */, + B6D9649021544D4F00EB9CFB /* grpc_connection_test.cc */, B6BBE42F21262CF400C6A53E /* grpc_stream_test.cc */, + B6D964922154AB8F00EB9CFB /* grpc_streaming_reader_test.cc */, + B6D964942163E63900EB9CFB /* grpc_unary_call_test.cc */, 61F72C5520BC48FD001A68CB /* serializer_test.cc */, B66D8995213609EE0086DA0C /* stream_test.mm */, ); @@ -680,10 +677,6 @@ 54740A561FC913EB00713A1A /* util */ = { isa = PBXGroup; children = ( - B60894F62170207100EBC644 /* fake_credentials_provider.cc */, - B60894F52170207100EBC644 /* fake_credentials_provider.h */, - B67BF448216EB43000CA9097 /* create_noop_connectivity_monitor.cc */, - B67BF447216EB42F00CA9097 /* create_noop_connectivity_monitor.h */, B6FB4680208EA0BE00554BA2 /* async_queue_libdispatch_test.mm */, B6FB4681208EA0BE00554BA2 /* async_queue_std_test.cc */, B6FB467B208E9A8200554BA2 /* async_queue_test.cc */, @@ -692,10 +685,14 @@ 54740A521FC913E500713A1A /* autoid_test.cc */, AB380D01201BC69F00D97691 /* bits_test.cc */, 548DB928200D59F600E00ABC /* comparison_test.cc */, + B67BF448216EB43000CA9097 /* create_noop_connectivity_monitor.cc */, + B67BF447216EB42F00CA9097 /* create_noop_connectivity_monitor.h */, B6FB4689208F9B9100554BA2 /* executor_libdispatch_test.mm */, B6FB4687208F9B9100554BA2 /* executor_std_test.cc */, B6FB4688208F9B9100554BA2 /* executor_test.cc */, B6FB468A208F9B9100554BA2 /* executor_test.h */, + B60894F62170207100EBC644 /* fake_credentials_provider.cc */, + B60894F52170207100EBC644 /* fake_credentials_provider.h */, F51859B394D01C0C507282F1 /* filesystem_test.cc */, B1A7E1959AF8141FA7E6B888 /* grpc_stream_tester.cc */, ED4B3E3EA0EBF3ED19A07060 /* grpc_stream_tester.h */, @@ -1044,7 +1041,6 @@ 549CCA5320A36E1F00BCEB75 /* field_mask_test.cc */, B686F2AD2023DDB20028D6BE /* field_path_test.cc */, AB356EF6200EA5EB0089B766 /* field_value_test.cc */, - AB6B908520322E6D00CC290A /* maybe_document_test.cc */, C8522DE226C467C54E6788D8 /* mutation_test.cc */, AB6B908720322E8800CC290A /* no_document_test.cc */, 549CCA5520A36E1F00BCEB75 /* precondition_test.cc */, @@ -1870,7 +1866,6 @@ 5467FB01203E5717009C9584 /* FIRFirestoreTests.mm in Sources */, 5492E052202154AB00B64F25 /* FIRGeoPointTests.mm in Sources */, 5492E059202154AB00B64F25 /* FIRQuerySnapshotTests.mm in Sources */, - B60894F72170207200EBC644 /* fake_credentials_provider.cc in Sources */, 5492E051202154AA00B64F25 /* FIRQueryTests.mm in Sources */, 5492E057202154AB00B64F25 /* FIRSnapshotMetadataTests.mm in Sources */, B65D34A9203C995B0076A5E1 /* FIRTimestampTest.m in Sources */, @@ -1905,7 +1900,6 @@ 5492E0A42021552D00B64F25 /* FSTMemoryQueryCacheTests.mm in Sources */, 5492E0A52021552D00B64F25 /* FSTMemoryRemoteDocumentCacheTests.mm in Sources */, 5492E03420213FFC00B64F25 /* FSTMemorySpecTests.mm in Sources */, - B6D964952163E63900EB9CFB /* grpc_unary_call_test.cc in Sources */, 5492E03220213FFC00B64F25 /* FSTMockDatastore.mm in Sources */, 5492E0AC2021552D00B64F25 /* FSTMutationQueueTests.mm in Sources */, 5492E0BE2021555100B64F25 /* FSTMutationTests.mm in Sources */, @@ -1935,6 +1929,7 @@ AB380D02201BC69F00D97691 /* bits_test.cc in Sources */, 618BBEA920B89AAC00B5BCE7 /* common.pb.cc in Sources */, 548DB929200D59F600E00ABC /* comparison_test.cc in Sources */, + B67BF449216EB43000CA9097 /* create_noop_connectivity_monitor.cc in Sources */, ABC1D7DC2023A04B00BA84F0 /* credentials_provider_test.cc in Sources */, ABE6637A201FA81900ED349A /* database_id_test.cc in Sources */, AB38D93020236E21000A432D /* database_info_test.cc in Sources */, @@ -1947,6 +1942,7 @@ B6FB468F208F9BAE00554BA2 /* executor_std_test.cc in Sources */, B6FB4690208F9BB300554BA2 /* executor_test.cc in Sources */, B6D1B68520E2AB1B00B35856 /* exponential_backoff_test.cc in Sources */, + B60894F72170207200EBC644 /* fake_credentials_provider.cc in Sources */, 549CCA5720A36E1F00BCEB75 /* field_mask_test.cc in Sources */, B686F2AF2023DDEE0028D6BE /* field_path_test.cc in Sources */, 54A0352620A3AED0003E0143 /* field_transform_test.mm in Sources */, @@ -1955,9 +1951,11 @@ ABC1D7E42024AFDE00BA84F0 /* firebase_credentials_provider_test.mm in Sources */, 618BBEAA20B89AAC00B5BCE7 /* firestore.pb.cc in Sources */, AB7BAB342012B519001E0872 /* geo_point_test.cc in Sources */, + B6D9649121544D4F00EB9CFB /* grpc_connection_test.cc in Sources */, B6BBE43121262CF400C6A53E /* grpc_stream_test.cc in Sources */, 333FCB7BB0C9986B5DF28FC8 /* grpc_stream_tester.cc in Sources */, B6D964932154AB8F00EB9CFB /* grpc_streaming_reader_test.cc in Sources */, + B6D964952163E63900EB9CFB /* grpc_unary_call_test.cc in Sources */, 73FE5066020EF9B2892C86BF /* hard_assert_test.cc in Sources */, 54511E8E209805F8005BD28F /* hashing_test.cc in Sources */, 618BBEB020B89AAC00B5BCE7 /* http.pb.cc in Sources */, @@ -1968,7 +1966,6 @@ 020AFD89BB40E5175838BB76 /* local_serializer_test.cc in Sources */, 54C2294F1FECABAE007D065B /* log_test.cc in Sources */, 618BBEA720B89AAC00B5BCE7 /* maybe_document.pb.cc in Sources */, - AB6B908620322E6D00CC290A /* maybe_document_test.cc in Sources */, 618BBEA820B89AAC00B5BCE7 /* mutation.pb.cc in Sources */, 32F022CB75AEE48CDDAF2982 /* mutation_test.cc in Sources */, 84DBE646DCB49305879D3500 /* nanopb_string_test.cc in Sources */, @@ -2001,9 +1998,7 @@ ABC1D7E12023A40C00BA84F0 /* token_test.cc in Sources */, 54A0352720A3AED0003E0143 /* transform_operations_test.mm in Sources */, 549CCA5120A36DBC00BCEB75 /* tree_sorted_map_test.cc in Sources */, - B6D9649121544D4F00EB9CFB /* grpc_connection_test.cc in Sources */, C80B10E79CDD7EF7843C321E /* type_traits_apple_test.mm in Sources */, - B67BF449216EB43000CA9097 /* create_noop_connectivity_monitor.cc in Sources */, ABC1D7DE2023A05300BA84F0 /* user_test.cc in Sources */, 618BBEAD20B89AAC00B5BCE7 /* write.pb.cc in Sources */, ); @@ -2041,7 +2036,6 @@ 5492E080202154EC00B64F25 /* FSTSmokeTests.mm in Sources */, 5492E07F202154EC00B64F25 /* FSTTransactionTests.mm in Sources */, 5492E0442021457E00B64F25 /* XCTestCase+Await.mm in Sources */, - B67BF44A216EB43000CA9097 /* create_noop_connectivity_monitor.cc in Sources */, EBFC611B1BF195D0EC710AF4 /* app_testing.mm in Sources */, CA989C0E6020C372A62B7062 /* testutil.cc in Sources */, ); From 8cef18c7626995efc0add4a32812c5de9914af58 Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Wed, 19 Dec 2018 11:15:21 -0500 Subject: [PATCH 3/3] Review feedback --- Firestore/core/src/firebase/firestore/model/document.h | 10 ++++++---- .../core/src/firebase/firestore/model/maybe_document.h | 9 ++++++++- .../test/firebase/firestore/model/document_test.cc | 2 +- 3 files changed, 15 insertions(+), 6 deletions(-) diff --git a/Firestore/core/src/firebase/firestore/model/document.h b/Firestore/core/src/firebase/firestore/model/document.h index 2ccbe3807c1..9afa327f64b 100644 --- a/Firestore/core/src/firebase/firestore/model/document.h +++ b/Firestore/core/src/firebase/firestore/model/document.h @@ -32,11 +32,13 @@ enum class DocumentState { * inconsistent. */ kLocalMutations, + /** * Mutations applied based on a write acknowledgment. Document is potentially * inconsistent. */ kCommittedMutations, + /** No mutations applied. Document was sent to us by Watch. */ kSynced, }; @@ -63,16 +65,16 @@ class Document : public MaybeDocument { return data_.Get(path); } - bool has_local_mutations() const { + bool HasLocalMutations() const { return document_state_ == DocumentState::kLocalMutations; } - bool has_committed_mutations() const { + bool HasCommittedMutations() const { return document_state_ == DocumentState::kCommittedMutations; } bool HasPendingWrites() const override { - return has_local_mutations() || has_committed_mutations(); + return HasLocalMutations() || HasCommittedMutations(); } protected: @@ -86,7 +88,7 @@ class Document : public MaybeDocument { /** Compares against another Document. */ inline bool operator==(const Document& lhs, const Document& rhs) { return lhs.version() == rhs.version() && lhs.key() == rhs.key() && - lhs.has_local_mutations() == rhs.has_local_mutations() && + lhs.HasLocalMutations() == rhs.HasLocalMutations() && lhs.data() == rhs.data(); } diff --git a/Firestore/core/src/firebase/firestore/model/maybe_document.h b/Firestore/core/src/firebase/firestore/model/maybe_document.h index 2db605baad7..1039de19e44 100644 --- a/Firestore/core/src/firebase/firestore/model/maybe_document.h +++ b/Firestore/core/src/firebase/firestore/model/maybe_document.h @@ -35,10 +35,17 @@ class MaybeDocument { public: /** * All the different kinds of documents, including MaybeDocument and its - * subclasses. This is used to provide RTTI for documents. + * subclasses. This is used to provide RTTI for documents. See the docstrings + * of the subclasses for details. */ enum class Type { + // An unknown subclass of MaybeDocument. This should never happen. + // + // TODO(rsgowman): Since it's no longer possible to directly create + // MaybeDocument's, we can likely remove this value entirely. But + // investigate impact on the serializers first. Unknown, + Document, NoDocument, UnknownDocument, diff --git a/Firestore/core/test/firebase/firestore/model/document_test.cc b/Firestore/core/test/firebase/firestore/model/document_test.cc index fe9d54f3b57..ba62782889b 100644 --- a/Firestore/core/test/firebase/firestore/model/document_test.cc +++ b/Firestore/core/test/firebase/firestore/model/document_test.cc @@ -47,7 +47,7 @@ TEST(Document, Getter) { doc.data()); EXPECT_EQ(DocumentKey::FromPathString("i/am/a/path"), doc.key()); EXPECT_EQ(SnapshotVersion(Timestamp(123, 456)), doc.version()); - EXPECT_TRUE(doc.has_local_mutations()); + EXPECT_TRUE(doc.HasLocalMutations()); } TEST(Document, Comparison) {