Skip to content

Update Query code to use Protobuf #8070

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 14, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 22 additions & 14 deletions Firestore/core/src/core/bound.cc
Original file line number Diff line number Diff line change
Expand Up @@ -21,43 +21,51 @@
#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"

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<FieldValue> doc_value =
document.field(ordering_component.field());
absl::optional<google_firestore_v1_Value> 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);
Expand All @@ -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) {
Expand All @@ -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
Expand Down
15 changes: 8 additions & 7 deletions Firestore/core/src/core/bound.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,14 @@
#define FIRESTORE_CORE_SRC_CORE_BOUND_H_

#include <iosfwd>
#include <memory>
#include <string>
#include <utility>
#include <vector>

#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 {
Expand Down Expand Up @@ -53,15 +54,15 @@ class Bound {
* @param is_before Whether this bound is just before or just after the
* position.
*/
Bound(std::vector<model::FieldValue> 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<model::FieldValue>& 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 */
Expand All @@ -83,7 +84,7 @@ class Bound {
size_t Hash() const;

private:
std::vector<model::FieldValue> position_;
nanopb::SharedMessage<google_firestore_v1_ArrayValue> position_;
bool before_;
};

Expand Down
11 changes: 5 additions & 6 deletions Firestore/core/src/core/order_by.cc
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
#include <ostream>

#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"

Expand All @@ -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<FieldValue> value1 = lhs.field(field_);
absl::optional<FieldValue> value2 = rhs.field(field_);
absl::optional<google_firestore_v1_Value> value1 = lhs->field(field_);
absl::optional<google_firestore_v1_Value> 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);
Expand Down
10 changes: 5 additions & 5 deletions Firestore/core/src/core/query.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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;
}
}
Expand Down
40 changes: 20 additions & 20 deletions Firestore/core/test/unit/core/query_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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;
Expand Down Expand Up @@ -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<Document>& vector,
const DocumentComparator& comp) {
testing::AssertionResult CorrectComparisons(
const std::vector<MutableDocument>& 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) {
Expand All @@ -487,7 +486,7 @@ TEST(QueryTest, SortsDocumentsInTheCorrectOrder) {
auto query = testutil::Query("collection").AddingOrderBy(OrderBy("sort"));

// clang-format off
std::vector<Document> docs = {
std::vector<MutableDocument> docs = {
Doc("collection/1", 0, Map("sort", nullptr)),
Doc("collection/1", 0, Map("sort", false)),
Doc("collection/1", 0, Map("sort", true)),
Expand All @@ -514,7 +513,7 @@ TEST(QueryTest, SortsDocumentsUsingMultipleFields) {
.AddingOrderBy(OrderBy("sort2"));

// clang-format off
std::vector<Document> docs = {
std::vector<MutableDocument> 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
Expand All @@ -537,7 +536,7 @@ TEST(QueryTest, SortsDocumentsWithDescendingToo) {
.AddingOrderBy(OrderBy("sort2", "desc"));

// clang-format off
std::vector<Document> docs = {
std::vector<MutableDocument> 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
Expand Down Expand Up @@ -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"));
Expand All @@ -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"));
}
Expand All @@ -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());
}

Expand Down
3 changes: 1 addition & 2 deletions Firestore/core/test/unit/model/mutation_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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)}};
Expand Down