Skip to content

Commit 5a1075b

Browse files
author
Alex Ames
committed
Made parameters in QueryParams optional.
QueryParams previous used null variants and empty strings to indicate that a field should not be used as a filter. This creates a problem when you want to filter by something that has a null value or an empty string. To fix this std::string and Variant have been replaced with Optional<std::string> and Optional<Variant> so that "filter on null" is distinct from "there is no filter".
1 parent a69445d commit 5a1075b

13 files changed

+292
-192
lines changed

‎database/src/common/query_spec.h

Lines changed: 45 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
#define FIREBASE_DATABASE_CLIENT_CPP_SRC_COMMON_QUERY_SPEC_H_
1717

1818
#include "app/src/include/firebase/variant.h"
19+
#include "app/src/optional.h"
1920
#include "app/src/path.h"
2021

2122
namespace firebase {
@@ -43,6 +44,18 @@ struct QueryParams {
4344
limit_first == other.limit_first && limit_last == other.limit_last;
4445
}
4546

47+
template <typename T>
48+
int OptionalCmp(const Optional<T>& a, const Optional<T>& b) const {
49+
if (a.has_value() && b.has_value()) {
50+
if (*a < *b) return -1;
51+
if (*a > *b) return 1;
52+
return 0;
53+
}
54+
if (!a.has_value() && b.has_value()) return -1;
55+
if (a.has_value() && !b.has_value()) return 1;
56+
return 0;
57+
}
58+
4659
// Less-than operator, required so we can place QuerySpec instances in an
4760
// std::map. This is an arbitrary operation, but must be consistent.
4861
bool operator<(const QueryParams& other) const {
@@ -52,18 +65,32 @@ struct QueryParams {
5265
if (order_by_child < other.order_by_child) return true;
5366
if (order_by_child > other.order_by_child) return false;
5467
}
55-
if (start_at_value < other.start_at_value) return true;
56-
if (start_at_value > other.start_at_value) return false;
57-
if (start_at_child_key < other.start_at_child_key) return true;
58-
if (start_at_child_key > other.start_at_child_key) return false;
59-
if (end_at_value < other.end_at_value) return true;
60-
if (end_at_value > other.end_at_value) return false;
61-
if (end_at_child_key < other.end_at_child_key) return true;
62-
if (end_at_child_key > other.end_at_child_key) return false;
63-
if (equal_to_value < other.equal_to_value) return true;
64-
if (equal_to_value > other.equal_to_value) return false;
65-
if (equal_to_child_key < other.equal_to_child_key) return true;
66-
if (equal_to_child_key > other.equal_to_child_key) return false;
68+
// clang-format off
69+
switch (OptionalCmp(start_at_value, other.start_at_value)) {
70+
case -1: return true;
71+
case 1: return false;
72+
}
73+
switch (OptionalCmp(start_at_child_key, other.start_at_child_key)) {
74+
case -1: return true;
75+
case 1: return false;
76+
}
77+
switch (OptionalCmp(end_at_value, other.end_at_value)) {
78+
case -1: return true;
79+
case 1: return false;
80+
}
81+
switch (OptionalCmp(end_at_child_key, other.end_at_child_key)) {
82+
case -1: return true;
83+
case 1: return false;
84+
}
85+
switch (OptionalCmp(equal_to_value, other.equal_to_value)) {
86+
case -1: return true;
87+
case 1: return false;
88+
}
89+
switch (OptionalCmp(equal_to_child_key, other.equal_to_child_key)) {
90+
case -1: return true;
91+
case 1: return false;
92+
}
93+
// clang-format on
6794
if (limit_first < other.limit_first) return true;
6895
if (limit_first > other.limit_first) return false;
6996
if (limit_last < other.limit_last) return true;
@@ -82,19 +109,19 @@ struct QueryParams {
82109
std::string order_by_child;
83110

84111
// Set by Query::StartAt(). Variant::Null() if unspecified.
85-
Variant start_at_value;
112+
Optional<Variant> start_at_value;
86113
// Set by Query::StartAt() with child specified. Blank if unspecified.
87-
std::string start_at_child_key;
114+
Optional<std::string> start_at_child_key;
88115

89116
// Set by Query::EndAt(). Variant::Null() if unspecified.
90-
Variant end_at_value;
117+
Optional<Variant> end_at_value;
91118
// Set by Query::EndAt() with child specified. Blank if unspecified.
92-
std::string end_at_child_key;
119+
Optional<std::string> end_at_child_key;
93120

94121
// Set by Query::EqualTo(). Variant::Null() if unspecified.
95-
Variant equal_to_value;
122+
Optional<Variant> equal_to_value;
96123
// Set by Query::EqualTo() with child specified. Blank if unspecified.
97-
std::string equal_to_child_key;
124+
Optional<std::string> equal_to_child_key;
98125

99126
// Set by Query::LimitToFirst(). 0 means no limit.
100127
size_t limit_first;

‎database/src/desktop/core/indexed_variant.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ IndexedVariant& IndexedVariant::operator=(const IndexedVariant& other) {
6565
const char* IndexedVariant::GetPredecessorChildName(
6666
const std::string& child_key, const Variant& child_value) const {
6767
Variant key = child_key.c_str();
68-
auto iter = index_.find(std::make_pair(key, child_value));
68+
auto iter = index_.find(std::make_pair(Variant(key), child_value));
6969

7070
if (iter == index_.end()) {
7171
return nullptr;

‎database/src/desktop/persistence/flatbuffer_conversions.cc

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -258,13 +258,25 @@ Offset<persistence::PersistedQueryParams> FlatbufferFromQueryParams(
258258
return CreatePersistedQueryParams(
259259
*builder, static_cast<persistence::OrderBy>(params.order_by),
260260
builder->CreateString(params.order_by_child),
261-
builder->CreateVector(VariantToFlexbuffer(params.start_at_value)),
262-
builder->CreateString(params.start_at_child_key),
263-
builder->CreateVector(VariantToFlexbuffer(params.end_at_value)),
264-
builder->CreateString(params.end_at_child_key),
265-
builder->CreateVector(VariantToFlexbuffer(params.equal_to_value)),
266-
builder->CreateString(params.equal_to_child_key), params.limit_first,
267-
params.limit_last);
261+
params.start_at_value.has_value()
262+
? builder->CreateVector(VariantToFlexbuffer(*params.start_at_value))
263+
: 0,
264+
params.start_at_child_key.has_value()
265+
? builder->CreateString(params.start_at_child_key->c_str())
266+
: 0,
267+
params.end_at_value.has_value()
268+
? builder->CreateVector(VariantToFlexbuffer(*params.end_at_value))
269+
: 0,
270+
params.end_at_child_key.has_value()
271+
? builder->CreateString(params.end_at_child_key->c_str())
272+
: 0,
273+
params.equal_to_value.has_value()
274+
? builder->CreateVector(VariantToFlexbuffer(*params.equal_to_value))
275+
: 0,
276+
params.equal_to_child_key.has_value()
277+
? builder->CreateString(params.equal_to_child_key->c_str())
278+
: 0,
279+
params.limit_first, params.limit_last);
268280
}
269281

270282
Offset<persistence::PersistedQuerySpec> FlatbufferFromQuerySpec(

‎database/src/desktop/query_desktop.cc

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -358,10 +358,10 @@ QueryInternal* QueryInternal::StartAt(const Variant& value,
358358
}
359359
return nullptr;
360360
}
361-
FIREBASE_ASSERT_RETURN(nullptr, child_key != nullptr);
362361
QuerySpec spec = query_spec_;
363362
spec.params.start_at_value = value;
364-
spec.params.start_at_child_key = child_key;
363+
spec.params.start_at_child_key =
364+
child_key ? Optional<std::string>(child_key) : Optional<std::string>();
365365
if (!ValidateQueryEndpoints(spec.params, logger)) {
366366
return nullptr;
367367
}
@@ -407,10 +407,10 @@ QueryInternal* QueryInternal::EndAt(const Variant& value,
407407
}
408408
return nullptr;
409409
}
410-
FIREBASE_ASSERT_RETURN(nullptr, child_key != nullptr);
411410
QuerySpec spec = query_spec_;
412411
spec.params.end_at_value = value;
413-
spec.params.end_at_child_key = child_key;
412+
spec.params.end_at_child_key =
413+
child_key ? Optional<std::string>(child_key) : Optional<std::string>();
414414
if (!ValidateQueryEndpoints(spec.params, logger)) {
415415
return nullptr;
416416
}
@@ -452,7 +452,8 @@ QueryInternal* QueryInternal::EqualTo(const Variant& value,
452452
}
453453
QuerySpec spec = query_spec_;
454454
spec.params.equal_to_value = value;
455-
spec.params.equal_to_child_key = child_key;
455+
spec.params.equal_to_child_key =
456+
child_key ? Optional<std::string>(child_key) : Optional<std::string>();
456457
if (!ValidateQueryEndpoints(spec.params, logger)) {
457458
return nullptr;
458459
}

‎database/src/desktop/query_params_comparator.cc

Lines changed: 23 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -37,10 +37,14 @@ static const Variant kMinVariant = Variant::FromStaticBlob( // NOLINT
3737
static const Variant kMaxVariant = Variant::FromStaticBlob( // NOLINT
3838
QueryParamsComparator::kMaxKey, sizeof(QueryParamsComparator::kMaxKey));
3939

40-
const std::pair<Variant, Variant> QueryParamsComparator::kMinNode = // NOLINT
41-
std::make_pair(QueryParamsComparator::kMinKey, kMinVariant);
42-
const std::pair<Variant, Variant> QueryParamsComparator::kMaxNode = // NOLINT
43-
std::make_pair(QueryParamsComparator::kMaxKey, kMaxVariant);
40+
const std::pair<Optional<Variant>, Optional<Variant>>
41+
QueryParamsComparator::kMinNode =
42+
std::make_pair(Optional<Variant>(QueryParamsComparator::kMinKey),
43+
Optional<Variant>(kMinVariant)); // NOLINT
44+
const std::pair<Optional<Variant>, Optional<Variant>>
45+
QueryParamsComparator::kMaxNode =
46+
std::make_pair(Optional<Variant>(QueryParamsComparator::kMaxKey),
47+
Optional<Variant>(kMaxVariant)); // NOLINT
4448

4549
static int VariantIsSentinel(const Variant& key, const Variant& value) {
4650
if (key == QueryParamsComparator::kMinKey && value == kMinVariant) {
@@ -52,42 +56,43 @@ static int VariantIsSentinel(const Variant& key, const Variant& value) {
5256
}
5357
}
5458

55-
int QueryParamsComparator::Compare(const Variant& key_a, const Variant& value_a,
56-
const Variant& key_b,
57-
const Variant& value_b) const {
58-
assert(key_a.is_string() || key_a.is_int64());
59-
assert(key_b.is_string() || key_b.is_int64());
59+
int QueryParamsComparator::CompareInternal(const Variant* key_a,
60+
const Variant* value_a,
61+
const Variant* key_b,
62+
const Variant* value_b) const {
63+
assert(key_a->is_string() || key_a->is_int64());
64+
assert(key_b->is_string() || key_b->is_int64());
6065

6166
// First check if either of our nodes is the special min or max sentinel
6267
// value. If that's the case, we can short circuit the rest of the comparison.
63-
int min_max_a = VariantIsSentinel(key_a, value_a);
64-
int min_max_b = VariantIsSentinel(key_b, value_b);
68+
int min_max_a = VariantIsSentinel(*key_a, *value_a);
69+
int min_max_b = VariantIsSentinel(*key_b, *value_b);
6570
if (min_max_a != min_max_b) {
6671
return min_max_a - min_max_b;
6772
}
6873

6974
switch (query_params_->order_by) {
7075
case QueryParams::kOrderByPriority: {
71-
int result = ComparePriorities(value_a, value_b);
76+
int result = ComparePriorities(*value_a, *value_b);
7277
if (result == 0) {
73-
result = CompareKeys(key_a, key_b);
78+
result = CompareKeys(*key_a, *key_b);
7479
}
7580
return result;
7681
}
7782
case QueryParams::kOrderByChild: {
78-
int result = CompareChildren(value_a, value_b);
83+
int result = CompareChildren(*value_a, *value_b);
7984
if (result == 0) {
80-
result = CompareKeys(key_a, key_b);
85+
result = CompareKeys(*key_a, *key_b);
8186
}
8287
return result;
8388
}
8489
case QueryParams::kOrderByKey: {
85-
return CompareKeys(key_a, key_b);
90+
return CompareKeys(*key_a, *key_b);
8691
}
8792
case QueryParams::kOrderByValue: {
88-
int result = CompareValues(value_a, value_b);
93+
int result = CompareValues(*value_a, *value_b);
8994
if (result == 0) {
90-
result = CompareKeys(key_a, key_b);
95+
result = CompareKeys(*key_a, *key_b);
9196
}
9297
return result;
9398
}

‎database/src/desktop/query_params_comparator.h

Lines changed: 44 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,18 @@ namespace firebase {
2424
namespace database {
2525
namespace internal {
2626

27+
inline const Variant* ToVariantPtr(const Optional<Variant>& v) {
28+
return v.has_value() ? &v.value() : nullptr;
29+
}
30+
31+
inline const Variant* ToVariantPtr(const Variant& v) {
32+
return &v;
33+
}
34+
35+
inline const Variant* ToVariantPtr(const Variant* v) {
36+
return v;
37+
}
38+
2739
// A Variant comparator, only meant for internal use.
2840
//
2941
// Explanation: Variants by default sort their elements into a map using a
@@ -62,19 +74,27 @@ namespace internal {
6274
// kOrderByKey rules.
6375
class QueryParamsComparator {
6476
public:
65-
QueryParamsComparator() : query_params_(nullptr) {}
77+
QueryParamsComparator() : query_params_(nullptr) {
78+
}
6679

6780
explicit QueryParamsComparator(const QueryParams* query_params)
68-
: query_params_(query_params) {}
81+
: query_params_(query_params) {
82+
}
6983

7084
// Compare two database values given their key and value.
71-
int Compare(const Variant& key_a, const Variant& value_a,
72-
const Variant& key_b, const Variant& value_b) const;
85+
template <typename V1, typename V2>
86+
int Compare(const std::pair<V1, V1>& a, const std::pair<V2, V2>& b) const {
87+
return CompareInternal(ToVariantPtr(a.first), ToVariantPtr(a.second),
88+
ToVariantPtr(b.first), ToVariantPtr(b.second));
89+
}
7390

74-
// Compare two database values given their key and value.
75-
int Compare(const std::pair<Variant, Variant>& a,
76-
const std::pair<Variant, Variant>& b) const {
77-
return Compare(a.first, a.second, b.first, b.second);
91+
template <typename KeyA, typename ValueA, typename KeyB, typename ValueB>
92+
int Compare(const KeyA& key_a,
93+
const ValueA& value_a,
94+
const KeyB& key_b,
95+
const ValueB& value_b) const {
96+
return CompareInternal(ToVariantPtr(key_a), ToVariantPtr(value_a),
97+
ToVariantPtr(key_b), ToVariantPtr(value_b));
7898
}
7999

80100
// Utility function to compare two variants as keys.
@@ -88,10 +108,15 @@ class QueryParamsComparator {
88108
// These values will always be sorted before or after all over values.
89109
static const char kMinKey[];
90110
static const char kMaxKey[];
91-
static const std::pair<Variant, Variant> kMinNode;
92-
static const std::pair<Variant, Variant> kMaxNode;
111+
static const std::pair<Optional<Variant>, Optional<Variant>> kMinNode;
112+
static const std::pair<Optional<Variant>, Optional<Variant>> kMaxNode;
93113

94114
private:
115+
int CompareInternal(const Variant* key_a,
116+
const Variant* value_a,
117+
const Variant* key_b,
118+
const Variant* value_b) const;
119+
95120
int CompareChildren(const Variant& value_a, const Variant& value_b) const;
96121

97122
const QueryParams* query_params_;
@@ -101,28 +126,18 @@ class QueryParamsComparator {
101126
// as the std::set's comparator argument.
102127
class QueryParamsLesser {
103128
public:
104-
QueryParamsLesser() : comparator_() {}
105-
explicit QueryParamsLesser(const QueryParams* query_params)
106-
: comparator_(query_params) {}
107-
108-
bool operator()(const std::pair<Variant, Variant>& a,
109-
const std::pair<Variant, Variant>& b) const {
110-
return comparator_.Compare(a.first, a.second, b.first, b.second) < 0;
129+
QueryParamsLesser() : comparator_() {
111130
}
112-
113-
bool operator()(const std::pair<const Variant, const Variant>& a,
114-
const std::pair<const Variant, const Variant>& b) const {
115-
return comparator_.Compare(a.first, a.second, b.first, b.second) < 0;
116-
}
117-
118-
bool operator()(const std::pair<Variant*, Variant*>& a,
119-
const std::pair<Variant*, Variant*>& b) const {
120-
return comparator_.Compare(*a.first, *a.second, *b.first, *b.second) < 0;
131+
explicit QueryParamsLesser(const QueryParams* query_params)
132+
: comparator_(query_params) {
121133
}
122134

123-
bool operator()(const std::pair<const Variant*, const Variant*>& a,
124-
const std::pair<const Variant*, const Variant*>& b) const {
125-
return comparator_.Compare(*a.first, *a.second, *b.first, *b.second) < 0;
135+
template <typename V1, typename V2>
136+
bool operator()(const std::pair<V1, V1>& a,
137+
const std::pair<V2, V2>& b) const {
138+
return comparator_.Compare(ToVariantPtr(a.first), ToVariantPtr(a.second),
139+
ToVariantPtr(b.first),
140+
ToVariantPtr(b.second)) < 0;
126141
}
127142

128143
private:

0 commit comments

Comments
 (0)