diff --git a/Firestore/core/src/core/bound.cc b/Firestore/core/src/core/bound.cc index 07829b8c724..05be9c21e60 100644 --- a/Firestore/core/src/core/bound.cc +++ b/Firestore/core/src/core/bound.cc @@ -21,6 +21,9 @@ #include "Firestore/core/src/core/order_by.h" #include "Firestore/core/src/immutable/append_only_list.h" #include "Firestore/core/src/model/document.h" +#include "Firestore/core/src/model/document_key.h" +#include "Firestore/core/src/model/value_util.h" +#include "Firestore/core/src/nanopb/nanopb_util.h" #include "Firestore/core/src/util/hashing.h" #include "Firestore/core/src/util/to_string.h" @@ -28,36 +31,41 @@ namespace firebase { namespace firestore { namespace core { +using model::CanonicalId; +using model::Compare; +using model::DocumentKey; using model::FieldPath; -using model::FieldValue; +using model::GetTypeOrder; +using model::TypeOrder; using util::ComparisonResult; bool Bound::SortsBeforeDocument(const OrderByList& order_by, const model::Document& document) const { - HARD_ASSERT(position_.size() <= order_by.size(), + HARD_ASSERT(position_->values_count <= order_by.size(), "Bound has more components than the provided order by."); ComparisonResult result = ComparisonResult::Same; - for (size_t idx = 0; idx < position_.size(); ++idx) { - const FieldValue& field_value = position_[idx]; + for (size_t idx = 0; idx < position_->values_count; ++idx) { + const google_firestore_v1_Value& field_value = position_->values[idx]; const OrderBy& ordering_component = order_by[idx]; ComparisonResult comparison; if (ordering_component.field() == FieldPath::KeyFieldPath()) { HARD_ASSERT( - field_value.type() == FieldValue::Type::Reference, + GetTypeOrder(field_value) == TypeOrder ::kReference, "Bound has a non-key value where the key path is being used %s", field_value.ToString()); - const auto& ref = field_value.reference_value(); - comparison = ref.key().CompareTo(document.key()); + auto key = DocumentKey::FromName( + nanopb::MakeString(field_value.reference_value)); + comparison = key.CompareTo(document->key()); } else { - absl::optional doc_value = - document.field(ordering_component.field()); + absl::optional doc_value = + document->field(ordering_component.field()); HARD_ASSERT( doc_value.has_value(), "Field should exist since document matched the orderBy already."); - comparison = field_value.CompareTo(*doc_value); + comparison = Compare(field_value, *doc_value); } comparison = ordering_component.direction().ApplyTo(comparison); @@ -73,15 +81,15 @@ bool Bound::SortsBeforeDocument(const OrderByList& order_by, std::string Bound::CanonicalId() const { std::string result = before_ ? "b:" : "a:"; - for (const FieldValue& component : position_) { - result.append(component.ToString()); + for (pb_size_t i = 0; i < position_->values_count; ++i) { + result.append(CanonicalId(position_->values[i])); } return result; } std::string Bound::ToString() const { return util::StringFormat("Bound(position=%s, before=%s)", - util::ToString(position_), util::ToString(before_)); + CanonicalId(*position_), util::ToString(before_)); } std::ostream& operator<<(std::ostream& os, const Bound& bound) { @@ -93,7 +101,7 @@ bool operator==(const Bound& lhs, const Bound& rhs) { } size_t Bound::Hash() const { - return util::Hash(position_, before_); + return util::Hash(model::CanonicalId(*position_), before_); } } // namespace core diff --git a/Firestore/core/src/core/bound.h b/Firestore/core/src/core/bound.h index bd9645b12c7..1d4330892cb 100644 --- a/Firestore/core/src/core/bound.h +++ b/Firestore/core/src/core/bound.h @@ -18,13 +18,14 @@ #define FIRESTORE_CORE_SRC_CORE_BOUND_H_ #include +#include #include #include -#include +#include "Firestore/Protos/nanopb/google/firestore/v1/document.nanopb.h" #include "Firestore/core/src/core/core_fwd.h" -#include "Firestore/core/src/model/field_value.h" #include "Firestore/core/src/model/model_fwd.h" +#include "Firestore/core/src/nanopb/message.h" namespace firebase { namespace firestore { @@ -53,15 +54,15 @@ class Bound { * @param is_before Whether this bound is just before or just after the * position. */ - Bound(std::vector position, bool is_before) - : position_(std::move(position)), before_(is_before) { + Bound(google_firestore_v1_ArrayValue position, bool is_before) + : position_{position}, before_(is_before) { } /** * The index position of this bound represented as an array of field values. */ - const std::vector& position() const { - return position_; + const google_firestore_v1_ArrayValue& position() const { + return *position_; } /** Whether this bound is just before or just after the provided position */ @@ -83,7 +84,7 @@ class Bound { size_t Hash() const; private: - std::vector position_; + nanopb::SharedMessage position_; bool before_; }; diff --git a/Firestore/core/src/core/order_by.cc b/Firestore/core/src/core/order_by.cc index fccfae6ac8d..cb0e4d6c316 100644 --- a/Firestore/core/src/core/order_by.cc +++ b/Firestore/core/src/core/order_by.cc @@ -19,7 +19,7 @@ #include #include "Firestore/core/src/model/document.h" -#include "Firestore/core/src/model/field_value.h" +#include "Firestore/core/src/model/value_util.h" #include "Firestore/core/src/util/string_format.h" #include "absl/strings/str_cat.h" @@ -29,20 +29,19 @@ namespace core { using model::Document; using model::FieldPath; -using model::FieldValue; using util::ComparisonResult; ComparisonResult OrderBy::Compare(const Document& lhs, const Document& rhs) const { ComparisonResult result; if (field_ == FieldPath::KeyFieldPath()) { - result = lhs.key().CompareTo(rhs.key()); + result = lhs->key().CompareTo(rhs->key()); } else { - absl::optional value1 = lhs.field(field_); - absl::optional value2 = rhs.field(field_); + absl::optional value1 = lhs->field(field_); + absl::optional value2 = rhs->field(field_); HARD_ASSERT(value1.has_value() && value2.has_value(), "Trying to compare documents on fields that don't exist."); - result = value1->CompareTo(*value2); + result = model::Compare(*value1, *value2); } return direction_.ApplyTo(result); diff --git a/Firestore/core/src/core/query.cc b/Firestore/core/src/core/query.cc index 38e7a007236..35179d401a8 100644 --- a/Firestore/core/src/core/query.cc +++ b/Firestore/core/src/core/query.cc @@ -221,16 +221,16 @@ Query Query::AsCollectionQueryAtPath(ResourcePath path) const { // MARK: - Matching bool Query::Matches(const Document& doc) const { - return MatchesPathAndCollectionGroup(doc) && MatchesOrderBy(doc) && - MatchesFilters(doc) && MatchesBounds(doc); + return doc->is_found_document() && MatchesPathAndCollectionGroup(doc) && + MatchesOrderBy(doc) && MatchesFilters(doc) && MatchesBounds(doc); } bool Query::MatchesPathAndCollectionGroup(const Document& doc) const { - const ResourcePath& doc_path = doc.key().path(); + const ResourcePath& doc_path = doc->key().path(); if (collection_group_) { // NOTE: path_ is currently always empty since we don't expose Collection // Group queries rooted at a document path yet. - return doc.key().HasCollectionId(*collection_group_) && + return doc->key().HasCollectionId(*collection_group_) && path_.IsPrefixOf(doc_path); } else if (DocumentKey::IsDocumentKey(path_)) { // Exact match for document queries. @@ -253,7 +253,7 @@ bool Query::MatchesOrderBy(const Document& doc) const { const FieldPath& field_path = order_by.field(); // order by key always matches if (field_path != FieldPath::KeyFieldPath() && - doc.field(field_path) == absl::nullopt) { + doc->field(field_path) == absl::nullopt) { return false; } } diff --git a/Firestore/core/test/unit/core/query_test.cc b/Firestore/core/test/unit/core/query_test.cc index cdaffa54524..40d6295080c 100644 --- a/Firestore/core/test/unit/core/query_test.cc +++ b/Firestore/core/test/unit/core/query_test.cc @@ -20,10 +20,9 @@ #include "Firestore/core/src/core/bound.h" #include "Firestore/core/src/core/field_filter.h" -#include "Firestore/core/src/model/document.h" #include "Firestore/core/src/model/document_set.h" #include "Firestore/core/src/model/field_path.h" -#include "Firestore/core/src/model/field_value.h" +#include "Firestore/core/src/model/mutable_document.h" #include "Firestore/core/src/model/resource_path.h" #include "Firestore/core/test/unit/testutil/testutil.h" #include "gmock/gmock.h" @@ -34,10 +33,9 @@ namespace firestore { namespace core { using firebase::firestore::util::ComparisonResult; -using model::Document; using model::DocumentComparator; using model::FieldPath; -using model::FieldValue; +using model::MutableDocument; using model::ResourcePath; using testing::AssertionResult; @@ -465,12 +463,13 @@ TEST(QueryTest, FiltersBasedOnObjectValue) { * Checks that an ordered array of elements yields the correct pair-wise * comparison result for the supplied comparator. */ -testing::AssertionResult CorrectComparisons(const std::vector& vector, - const DocumentComparator& comp) { +testing::AssertionResult CorrectComparisons( + const std::vector& vector, + const DocumentComparator& comp) { for (size_t i = 0; i < vector.size(); i++) { for (size_t j = 0; j < vector.size(); j++) { - const Document& i_doc = vector[i]; - const Document& j_doc = vector[j]; + const MutableDocument& i_doc = vector[i]; + const MutableDocument& j_doc = vector[j]; ComparisonResult expected = util::Compare(i, j); ComparisonResult actual = comp.Compare(i_doc, j_doc); if (actual != expected) { @@ -487,7 +486,7 @@ TEST(QueryTest, SortsDocumentsInTheCorrectOrder) { auto query = testutil::Query("collection").AddingOrderBy(OrderBy("sort")); // clang-format off - std::vector docs = { + std::vector docs = { Doc("collection/1", 0, Map("sort", nullptr)), Doc("collection/1", 0, Map("sort", false)), Doc("collection/1", 0, Map("sort", true)), @@ -514,7 +513,7 @@ TEST(QueryTest, SortsDocumentsUsingMultipleFields) { .AddingOrderBy(OrderBy("sort2")); // clang-format off - std::vector docs = { + std::vector docs = { Doc("collection/1", 0, Map("sort1", 1, "sort2", 1)), Doc("collection/1", 0, Map("sort1", 1, "sort2", 2)), Doc("collection/2", 0, Map("sort1", 1, "sort2", 2)), // by key @@ -537,7 +536,7 @@ TEST(QueryTest, SortsDocumentsWithDescendingToo) { .AddingOrderBy(OrderBy("sort2", "desc")); // clang-format off - std::vector docs = { + std::vector docs = { Doc("collection/1", 0, Map("sort1", 2, "sort2", 3)), Doc("collection/3", 0, Map("sort1", 2, "sort2", 2)), Doc("collection/2", 0, Map("sort1", 2, "sort2", 2)), // by key @@ -770,7 +769,7 @@ TEST(QueryTest, CanonicalIDs) { filters = testutil::Query("coll").AddingFilter( Filter("a", "not-in", Array(1, 2, 3))); EXPECT_THAT(filters, - HasCanonicalId("coll|f:anot-in[1, 2, 3]|ob:aasc__name__asc")); + HasCanonicalId("coll|f:anot-in[1,2,3]|ob:aasc__name__asc")); auto order_bys = testutil::Query("coll").AddingOrderBy(OrderBy("up", "asc")); EXPECT_THAT(order_bys, HasCanonicalId("coll|f:|ob:upasc__name__asc")); @@ -783,12 +782,13 @@ TEST(QueryTest, CanonicalIDs) { auto limit = testutil::Query("coll").WithLimitToFirst(25); EXPECT_THAT(limit, HasCanonicalId("coll|f:|ob:__name__asc|l:25|lt:f")); - auto bounds = - testutil::Query("airports") - .AddingOrderBy(OrderBy("name", "asc")) - .AddingOrderBy(OrderBy("score", "desc")) - .StartingAt(Bound({Value("OAK"), Value(1000)}, /* is_before= */ true)) - .EndingAt(Bound({Value("SFO"), Value(2000)}, /* is_before= */ false)); + auto bounds = testutil::Query("airports") + .AddingOrderBy(OrderBy("name", "asc")) + .AddingOrderBy(OrderBy("score", "desc")) + .StartingAt(Bound({Array("OAK", 1000)}, + /* is_before= */ true)) + .EndingAt(Bound({Array("SFO", 2000)}, + /* is_before= */ false)); EXPECT_THAT(bounds, HasCanonicalId("airports|f:|ob:nameascscoredesc__name__" "desc|lb:b:OAK1000|ub:a:SFO2000")); } @@ -809,10 +809,10 @@ TEST(QueryTest, MatchesAllDocuments) { query = base_query.WithLimitToFirst(1); EXPECT_FALSE(query.MatchesAllDocuments()); - query = base_query.StartingAt(Bound({Value("SFO")}, true)); + query = base_query.StartingAt(Bound({Array("SFO")}, true)); EXPECT_FALSE(query.MatchesAllDocuments()); - query = base_query.StartingAt(Bound({Value("OAK")}, true)); + query = base_query.StartingAt(Bound({Array("OAK")}, true)); EXPECT_FALSE(query.MatchesAllDocuments()); } diff --git a/Firestore/core/test/unit/model/mutation_test.cc b/Firestore/core/test/unit/model/mutation_test.cc index 2ff54941090..c0bcbcfafb2 100644 --- a/Firestore/core/test/unit/model/mutation_test.cc +++ b/Firestore/core/test/unit/model/mutation_test.cc @@ -344,8 +344,7 @@ TEST(MutationTest, TransformBaseDoc(base_data, transforms, expected); } -TEST(MutationTest, - AppliesLocalArrayUnionTransformWithExistingElementsInOrder) { +TEST(MutationTest, AppliesLocalArrayUnionTransformWithExistingElementsInOrder) { // New elements should be appended in order. auto base_data = Map("array", Array(1, 3)); TransformPairs transforms = {{"array", ArrayUnion(1, 2, 3, 4, 5)}};