Skip to content

Commit a232d50

Browse files
tom-andersentom-andersen
committed
Fixes from code review
1 parent fe5b6e8 commit a232d50

File tree

10 files changed

+162
-52
lines changed

10 files changed

+162
-52
lines changed

‎firestore/integration_test_internal/src/filter_test.cc

Lines changed: 92 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -441,29 +441,107 @@ TEST_F(FilterTest, CompositeComparison) {
441441
}
442442

443443
TEST_F(FilterTest, QueryWhereComposite) {
444-
MapFieldValue doc_aa = {{"x", FieldValue::String("a")},
445-
{"y", FieldValue::String("a")}};
446-
MapFieldValue doc_ab = {{"x", FieldValue::String("a")},
447-
{"y", FieldValue::String("b")}};
448-
MapFieldValue doc_ba = {{"x", FieldValue::String("b")},
449-
{"y", FieldValue::String("a")}};
450-
MapFieldValue doc_bb = {{"x", FieldValue::String("b")},
451-
{"y", FieldValue::String("b")}};
452-
CollectionReference collection = Collection(
453-
{{"aa", doc_aa}, {"ab", doc_ab}, {"ba", doc_ba}, {"bb", doc_bb}});
444+
MapFieldValue doc_aaa = {{"x", FieldValue::String("a")},
445+
{"y", FieldValue::String("a")},
446+
{"z", FieldValue::String("a")}};
447+
MapFieldValue doc_aab = {{"x", FieldValue::String("a")},
448+
{"y", FieldValue::String("a")},
449+
{"z", FieldValue::String("b")}};
450+
MapFieldValue doc_aba = {{"x", FieldValue::String("a")},
451+
{"y", FieldValue::String("b")},
452+
{"z", FieldValue::String("a")}};
453+
MapFieldValue doc_abb = {{"x", FieldValue::String("a")},
454+
{"y", FieldValue::String("b")},
455+
{"z", FieldValue::String("b")}};
456+
MapFieldValue doc_baa = {{"x", FieldValue::String("b")},
457+
{"y", FieldValue::String("a")},
458+
{"z", FieldValue::String("a")}};
459+
MapFieldValue doc_bab = {{"x", FieldValue::String("b")},
460+
{"y", FieldValue::String("a")},
461+
{"z", FieldValue::String("b")}};
462+
MapFieldValue doc_bba = {{"x", FieldValue::String("b")},
463+
{"y", FieldValue::String("b")},
464+
{"z", FieldValue::String("a")}};
465+
MapFieldValue doc_bbb = {{"x", FieldValue::String("b")},
466+
{"y", FieldValue::String("b")},
467+
{"z", FieldValue::String("b")}};
468+
CollectionReference collection = Collection({{"aaa", doc_aaa},
469+
{"aab", doc_aab},
470+
{"aba", doc_aba},
471+
{"abb", doc_abb},
472+
{"baa", doc_baa},
473+
{"bab", doc_bab},
474+
{"bba", doc_bba},
475+
{"bbb", doc_bbb}});
454476

455477
Filter filter_xa = Filter::EqualTo("x", FieldValue::String("a"));
478+
Filter filter_ya = Filter::EqualTo("y", FieldValue::String("a"));
456479
Filter filter_yb = Filter::EqualTo("y", FieldValue::String("b"));
480+
Filter filter_za = Filter::EqualTo("z", FieldValue::String("a"));
457481

482+
// And(x=a)
458483
QuerySnapshot snapshot1 =
459-
ReadDocuments(collection.Where(Filter::And(filter_xa, filter_yb)));
460-
EXPECT_EQ(std::vector<MapFieldValue>({doc_ab}),
484+
ReadDocuments(collection.Where(Filter::And(filter_xa)));
485+
EXPECT_EQ(std::vector<MapFieldValue>({doc_aaa, doc_aab, doc_aba, doc_abb}),
461486
QuerySnapshotToValues(snapshot1));
462487

488+
// And(x=a, y=b)
463489
QuerySnapshot snapshot2 =
464-
ReadDocuments(collection.Where(Filter::Or(filter_xa, filter_yb)));
465-
EXPECT_EQ(std::vector<MapFieldValue>({doc_aa, doc_ab, doc_bb}),
490+
ReadDocuments(collection.Where(Filter::And(filter_xa, filter_yb)));
491+
EXPECT_EQ(std::vector<MapFieldValue>({doc_aba, doc_abb}),
466492
QuerySnapshotToValues(snapshot2));
493+
494+
// And(Or(And(x=a)),Or(And(Or()))
495+
QuerySnapshot snapshot3 = ReadDocuments(
496+
collection.Where(Filter::And(Filter::Or(Filter::And(filter_xa)),
497+
Filter::Or(Filter::And(Filter::Or())))));
498+
EXPECT_EQ(std::vector<MapFieldValue>({doc_aaa, doc_aab, doc_aba, doc_abb}),
499+
QuerySnapshotToValues(snapshot3));
500+
501+
// Or(x=a)
502+
QuerySnapshot snapshot4 =
503+
ReadDocuments(collection.Where(Filter::Or(filter_xa)));
504+
EXPECT_EQ(std::vector<MapFieldValue>({doc_aaa, doc_aab, doc_aba, doc_abb}),
505+
QuerySnapshotToValues(snapshot4));
506+
507+
// Or(x=a, y=b)
508+
QuerySnapshot snapshot5 =
509+
ReadDocuments(collection.Where(Filter::Or(filter_xa, filter_yb)));
510+
EXPECT_EQ(std::vector<MapFieldValue>(
511+
{doc_aaa, doc_aab, doc_aba, doc_abb, doc_bba, doc_bbb}),
512+
QuerySnapshotToValues(snapshot5));
513+
514+
// Or(And(Or(x=a)),And(Or(And()))
515+
QuerySnapshot snapshot6 = ReadDocuments(
516+
collection.Where(Filter::Or(Filter::And(Filter::Or(filter_xa)),
517+
Filter::And(Filter::Or(Filter::And())))));
518+
EXPECT_EQ(std::vector<MapFieldValue>({doc_aaa, doc_aab, doc_aba, doc_abb}),
519+
QuerySnapshotToValues(snapshot6));
520+
521+
// And(x=b, Or(y=a, And(y=b, z=a)))
522+
QuerySnapshot snapshot7 = ReadDocuments(collection.Where(Filter::And(
523+
filter_xa, Filter::Or(filter_ya, Filter::And(filter_yb, filter_za)))));
524+
EXPECT_EQ(std::vector<MapFieldValue>({doc_aaa, doc_aab, doc_aba}),
525+
QuerySnapshotToValues(snapshot7));
526+
}
527+
528+
TEST_F(FilterTest, QueryEmptyWhereComposite) {
529+
MapFieldValue doc = {{"foo", FieldValue::String("bar")}};
530+
CollectionReference collection = Collection({{"x", doc}});
531+
532+
QuerySnapshot s1 = ReadDocuments(collection.Where(Filter::And()));
533+
EXPECT_EQ(std::vector<MapFieldValue>({doc}), QuerySnapshotToValues(s1));
534+
535+
QuerySnapshot s2 =
536+
ReadDocuments(collection.Where(Filter::And(Filter::Or(), Filter::Or())));
537+
EXPECT_EQ(std::vector<MapFieldValue>({doc}), QuerySnapshotToValues(s2));
538+
539+
QuerySnapshot s3 = ReadDocuments(collection.Where(Filter::Or()));
540+
EXPECT_EQ(std::vector<MapFieldValue>({doc}), QuerySnapshotToValues(s3));
541+
542+
QuerySnapshot s4 =
543+
ReadDocuments(collection.Where(Filter::Or(Filter::And(), Filter::And())));
544+
EXPECT_EQ(std::vector<MapFieldValue>({doc}), QuerySnapshotToValues(s4));
467545
}
468546

469547
} // namespace

‎firestore/src/android/query_android.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,7 @@ void QueryInternal::Initialize(jni::Loader& loader) {
146146
kGreaterThan, kGreaterThanOrEqualTo, kArrayContains, kArrayContainsAny,
147147
kIn, kNotIn, kOrderBy, kLimit, kLimitToLast, kStartAtSnapshot, kStartAt,
148148
kStartAfterSnapshot, kStartAfter, kEndBeforeSnapshot, kEndBefore,
149-
kEndAtSnapshot, kEndAt, kGet, kAddSnapshotListener, kHashCode);
149+
kEndAtSnapshot, kEndAt, kGet, kAddSnapshotListener, kHashCode, kWhere);
150150
}
151151

152152
Firestore* QueryInternal::firestore() {

‎firestore/src/android/query_android.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,12 @@ class QueryInternal : public Wrapper {
7171
*/
7272
virtual AggregateQuery Count() const;
7373

74+
/**
75+
* @brief Creates and returns a new Query with the additional filter.
76+
*
77+
* @param filter The new filter to apply to the existing query.
78+
* @return The created Query.
79+
*/
7480
Query Where(const Filter& filter) const;
7581

7682
/**

‎firestore/src/android/wrapper.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ Wrapper::Wrapper(Wrapper* rhs) : Wrapper() {
5656

5757
Wrapper::~Wrapper() = default;
5858

59-
jni::Env Wrapper::GetEnv() { return FirestoreInternal::GetEnv(); }
59+
jni::Env Wrapper::GetEnv() const { return firestore_->GetEnv(); }
6060

6161
Object Wrapper::ToJava(const FieldValue& value) {
6262
return FieldValueInternal::ToJava(value);

‎firestore/src/android/wrapper.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ class Wrapper {
5656
// Similar to a copy constructor, but can handle the case where `rhs` is null.
5757
explicit Wrapper(Wrapper* rhs);
5858

59-
static jni::Env GetEnv();
59+
jni::Env GetEnv() const;
6060

6161
FirestoreInternal* firestore_ = nullptr; // not owning
6262
jni::Global<jni::Object> obj_;

‎firestore/src/include/firebase/firestore/filter.h

Lines changed: 50 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -264,52 +264,74 @@ class Filter {
264264
static Filter NotIn(const FieldPath& field,
265265
const std::vector<FieldValue>& values);
266266

267-
/**
268-
* @brief Conjunction of a single filter will simply return original filter.
269-
*
270-
* @param[in] filter Single filter for conjunction.
271-
*
272-
* @return Same as filter passed in as parameter.
273-
*/
274-
static Filter And(const Filter filter) { return filter; }
275-
276267
/**
277268
* @brief Creates a new filter that is a conjunction of the given filters. A
278269
* conjunction filter includes a document if it satisfies all of the given
279270
* filters.
280271
*
272+
* If no filter is given, the composite filter is a no-op, and if only one
273+
* filter is given, the composite filter has the same behavior as the
274+
* underlying filter.
275+
*
281276
* @param[in] filters The filters to perform a conjunction for.
282277
*
283278
* @return The newly created filter.
284279
*/
285280
template <typename... Filters>
286281
static Filter And(const Filters&... filters) {
287-
return And(std::vector<Filter>({filters...}));
282+
return AndInternal(filters...);
288283
}
289284

290285
/**
291-
* @brief Disjunction of a single filter will simply return original filter.
286+
* @brief Creates a new filter that is a conjunction of the given filters. A
287+
* conjunction filter includes a document if it satisfies all of the given
288+
* filters.
292289
*
293-
* @param[in] filter Single filter for disjunction.
290+
* If no filter is given, the composite filter is a no-op, and if only one
291+
* filter is given, the composite filter has the same behavior as the
292+
* underlying filter.
294293
*
295-
* @return Same as filter passed in as parameter.
294+
* @param[in] filters The list that contains filters to perform a conjunction
295+
* for.
296+
*
297+
* @return The newly created filter.
296298
*/
297-
static Filter Or(const Filter filter) { return filter; }
299+
static Filter And(const std::vector<Filter>& filters);
298300

299301
/**
300302
* @brief Creates a new filter that is a disjunction of the given filters. A
301303
* disjunction filter includes a document if it satisfies <em>any</em> of the
302304
* given filters.
303305
*
306+
* If no filter is given, the composite filter is a no-op, and if only one
307+
* filter is given, the composite filter has the same behavior as the
308+
* underlying filter.
309+
*
304310
* @param[in] filters The filters to perform a disjunction for.
305311
*
306312
* @return The newly created filter.
307313
*/
308314
template <typename... Filters>
309315
static Filter Or(const Filters&... filters) {
310-
return Or(std::vector<Filter>({filters...}));
316+
return OrInternal(filters...);
311317
}
312318

319+
/**
320+
* @brief Creates a new filter that is a disjunction of the given filters. A
321+
* disjunction filter includes a document if it satisfies <em>any</em> of the
322+
* given filters.
323+
*
324+
* If no filter is given, the composite filter is a no-op, and if only one
325+
* filter is given, the composite filter has the same behavior as the
326+
* underlying filter.
327+
*
328+
* @param[in] filters The list that contains filters to perform a disjunction
329+
* for.
330+
*
331+
* @return The newly created filter.
332+
*/
333+
static Filter Or(const std::vector<Filter>& filters);
334+
313335
/**
314336
* @brief Copy constructor.
315337
*
@@ -355,8 +377,19 @@ class Filter {
355377
friend bool operator==(const Filter& lhs, const Filter& rhs);
356378
friend struct ConverterImpl;
357379

358-
static Filter And(const std::vector<Filter>& filters);
359-
static Filter Or(const std::vector<Filter>& filters);
380+
static inline Filter AndInternal(const Filter& filter) { return filter; }
381+
382+
template <typename... Filters>
383+
static inline Filter AndInternal(const Filters&... filters) {
384+
return And(std::vector<Filter>({filters...}));
385+
}
386+
387+
static inline Filter OrInternal(const Filter& filter) { return filter; }
388+
389+
template <typename... Filters>
390+
static inline Filter OrInternal(const Filters&... filters) {
391+
return Or(std::vector<Filter>({filters...}));
392+
}
360393

361394
bool IsEmpty() const;
362395

‎firestore/src/main/composite_filter_main.cc

Lines changed: 10 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323

2424
#include "Firestore/core/src/core/composite_filter.h"
2525
#include "absl/algorithm/container.h"
26+
#include "firestore/src/common/util.h"
2627
#include "firestore/src/main/composite_filter_main.h"
2728
#include "firestore/src/main/converter_main.h"
2829

@@ -46,7 +47,7 @@ bool CompositeFilterInternal::IsEmpty() const { return filters_.empty(); }
4647
core::Filter CompositeFilterInternal::ToCoreFilter(
4748
const api::Query& query,
4849
const firebase::firestore::UserDataConverter& user_data_converter) const {
49-
std::vector<core::Filter> core_filters;
50+
std::vector<core::Filter> core_filters{};
5051
for (auto& filter : filters_) {
5152
core_filters.push_back(filter->ToCoreFilter(query, user_data_converter));
5253
}
@@ -55,18 +56,14 @@ core::Filter CompositeFilterInternal::ToCoreFilter(
5556

5657
bool operator==(const CompositeFilterInternal& lhs,
5758
const CompositeFilterInternal& rhs) {
58-
if (lhs.op_ != rhs.op_) return false;
59-
const std::vector<std::shared_ptr<FilterInternal>>& lhs_filters =
60-
lhs.filters_;
61-
const std::vector<std::shared_ptr<FilterInternal>>& rhs_filters =
62-
rhs.filters_;
63-
size_t size = lhs_filters.size();
64-
if (size != rhs_filters.size()) return false;
65-
for (int i = 0; i < size; i++) {
66-
if (lhs_filters[i] != rhs_filters[i] && *lhs_filters[i] != *rhs_filters[i])
67-
return false;
68-
}
69-
return true;
59+
return lhs.op_ == rhs.op_ && lhs.filters_.size() == rhs.filters_.size() &&
60+
std::equal(lhs.filters_.begin(), lhs.filters_.end(),
61+
rhs.filters_.begin(), rhs.filters_.end(),
62+
[](const std::shared_ptr<FilterInternal>& lhs_filter,
63+
const std::shared_ptr<FilterInternal>& rhs_filter) {
64+
return EqualityCompare(lhs_filter.get(),
65+
rhs_filter.get());
66+
});
7067
}
7168

7269
} // namespace firestore

‎firestore/src/main/composite_filter_main.h

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,6 @@ class CompositeFilterInternal : public FilterInternal {
3535
CompositeFilterInternal(core::CompositeFilter::Operator op,
3636
std::vector<FilterInternal*>& filters);
3737

38-
~CompositeFilterInternal() override = default;
39-
4038
core::Filter ToCoreFilter(const api::Query& query,
4139
const firebase::firestore::UserDataConverter&
4240
user_data_converter) const override;

‎firestore/src/main/filter_main.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ Filter FilterInternal::UnaryFilter(const FieldPath& field_path,
9999

100100
Filter FilterInternal::CompositeFilter(core::CompositeFilter::Operator op,
101101
const std::vector<Filter>& filters) {
102-
std::vector<FilterInternal*> nonEmptyFilters;
102+
std::vector<FilterInternal*> nonEmptyFilters{};
103103
for (const Filter& filter : filters) {
104104
FilterInternal* filterInternal = GetInternal(&filter);
105105
if (!filterInternal->IsEmpty()) {

‎firestore/src/main/unary_filter_main.h

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,6 @@ class UnaryFilterInternal final : public FilterInternal {
3838
core::FieldFilter::Operator op,
3939
const std::vector<FieldValue>& values);
4040

41-
~UnaryFilterInternal() override = default;
42-
4341
core::Filter ToCoreFilter(const api::Query& query,
4442
const firebase::firestore::UserDataConverter&
4543
user_data_converter) const override;

0 commit comments

Comments
 (0)