Skip to content

Commit 547ab64

Browse files
author
Alex Ames
committed
Made changes to QueryParams more narrow.
Previously the changes to QueryParams so that they included optional fields had cascaded out to a number of other classes and functoins, but the bug being fixed didn't require the changes to extend to so many places. This narrows the scope of the changes to just the necessary classes and functions.
1 parent 5a1075b commit 547ab64

File tree

8 files changed

+117
-172
lines changed

8 files changed

+117
-172
lines changed

‎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(Variant(key), child_value));
68+
auto iter = index_.find(std::make_pair(key, child_value));
6969

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

‎database/src/desktop/query_params_comparator.cc

Lines changed: 18 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -37,14 +37,10 @@ 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<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
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);
4844

4945
static int VariantIsSentinel(const Variant& key, const Variant& value) {
5046
if (key == QueryParamsComparator::kMinKey && value == kMinVariant) {
@@ -56,43 +52,42 @@ static int VariantIsSentinel(const Variant& key, const Variant& value) {
5652
}
5753
}
5854

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());
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());
6560

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

7469
switch (query_params_->order_by) {
7570
case QueryParams::kOrderByPriority: {
76-
int result = ComparePriorities(*value_a, *value_b);
71+
int result = ComparePriorities(value_a, value_b);
7772
if (result == 0) {
78-
result = CompareKeys(*key_a, *key_b);
73+
result = CompareKeys(key_a, key_b);
7974
}
8075
return result;
8176
}
8277
case QueryParams::kOrderByChild: {
83-
int result = CompareChildren(*value_a, *value_b);
78+
int result = CompareChildren(value_a, value_b);
8479
if (result == 0) {
85-
result = CompareKeys(*key_a, *key_b);
80+
result = CompareKeys(key_a, key_b);
8681
}
8782
return result;
8883
}
8984
case QueryParams::kOrderByKey: {
90-
return CompareKeys(*key_a, *key_b);
85+
return CompareKeys(key_a, key_b);
9186
}
9287
case QueryParams::kOrderByValue: {
93-
int result = CompareValues(*value_a, *value_b);
88+
int result = CompareValues(value_a, value_b);
9489
if (result == 0) {
95-
result = CompareKeys(*key_a, *key_b);
90+
result = CompareKeys(key_a, key_b);
9691
}
9792
return result;
9893
}

‎database/src/desktop/query_params_comparator.h

Lines changed: 29 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -24,18 +24,6 @@ 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-
3927
// A Variant comparator, only meant for internal use.
4028
//
4129
// Explanation: Variants by default sort their elements into a map using a
@@ -74,27 +62,19 @@ inline const Variant* ToVariantPtr(const Variant* v) {
7462
// kOrderByKey rules.
7563
class QueryParamsComparator {
7664
public:
77-
QueryParamsComparator() : query_params_(nullptr) {
78-
}
65+
QueryParamsComparator() : query_params_(nullptr) {}
7966

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

8470
// Compare two database values given their key and value.
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-
}
71+
int Compare(const Variant& key_a, const Variant& value_a,
72+
const Variant& key_b, const Variant& value_b) const;
9073

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));
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);
9878
}
9979

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

11494
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-
12095
int CompareChildren(const Variant& value_a, const Variant& value_b) const;
12196

12297
const QueryParams* query_params_;
@@ -126,18 +101,28 @@ class QueryParamsComparator {
126101
// as the std::set's comparator argument.
127102
class QueryParamsLesser {
128103
public:
129-
QueryParamsLesser() : comparator_() {
130-
}
104+
QueryParamsLesser() : comparator_() {}
131105
explicit QueryParamsLesser(const QueryParams* query_params)
132-
: comparator_(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;
111+
}
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;
133121
}
134122

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;
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;
141126
}
142127

143128
private:

‎database/src/desktop/util_desktop.cc

Lines changed: 25 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -993,44 +993,32 @@ bool IsValidPriority(const Variant& variant) {
993993
return variant.is_null() || variant.is_numeric() || variant.is_string();
994994
}
995995

996-
std::pair<Optional<Variant>, Optional<Variant>> MakePost(
997-
const QueryParams& params,
998-
const Optional<std::string>& name,
999-
const Optional<Variant>& value) {
996+
std::pair<Variant, Variant> MakePost(const QueryParams& params,
997+
const std::string& name,
998+
const Variant& value) {
1000999
switch (params.order_by) {
10011000
case QueryParams::kOrderByPriority: {
1002-
return std::make_pair(
1003-
name.has_value() ? Optional<Variant>(*name) : Optional<Variant>(),
1004-
value.has_value() ? Optional<Variant>(std::map<Variant, Variant>{
1005-
std::make_pair(".priority", *value),
1006-
})
1007-
: Optional<Variant>());
1001+
return std::make_pair(name, std::map<Variant, Variant>{
1002+
std::make_pair(".priority", value),
1003+
});
10081004
}
10091005
case QueryParams::kOrderByChild: {
10101006
Variant variant;
1011-
SetVariantAtPath(&variant, Path(params.order_by_child),
1012-
value.has_value() ? *value : Variant::Null());
1013-
return std::make_pair(
1014-
name.has_value() ? Optional<Variant>(*name) : Optional<Variant>(),
1015-
Optional<Variant>(variant));
1007+
SetVariantAtPath(&variant, Path(params.order_by_child), value);
1008+
return std::make_pair(name, variant);
10161009
}
10171010
case QueryParams::kOrderByKey: {
1018-
FIREBASE_DEV_ASSERT(value.has_value() && value->is_string());
1011+
FIREBASE_DEV_ASSERT(value.is_string());
10191012
// We just use empty node, but it'll never be compared, since our
10201013
// comparator only looks at name.
1021-
return std::make_pair(value.has_value()
1022-
? Optional<Variant>(value->mutable_string())
1023-
: Optional<Variant>(),
1024-
Optional<Variant>(Variant::Null()));
1014+
return std::make_pair(value.string_value(), Variant::Null());
10251015
}
10261016
case QueryParams::kOrderByValue: {
1027-
return std::make_pair(
1028-
name.has_value() ? Optional<Variant>(*name) : Optional<Variant>(),
1029-
value);
1017+
return std::make_pair(name, value);
10301018
}
10311019
}
10321020
FIREBASE_DEV_ASSERT_MESSAGE(false, "Invalid QueryParams::OrderBy");
1033-
return std::pair<Optional<Variant>, Optional<Variant>>();
1021+
return std::pair<Variant, Variant>();
10341022
}
10351023

10361024
bool HasStart(const QueryParams& params) {
@@ -1041,25 +1029,28 @@ bool HasEnd(const QueryParams& params) {
10411029
return params.end_at_value.has_value() || params.equal_to_value.has_value();
10421030
}
10431031

1044-
const std::string& GetStartName(const QueryParams& params) {
1032+
std::string GetStartName(const QueryParams& params) {
1033+
FIREBASE_DEV_ASSERT_MESSAGE(
1034+
HasStart(params),
1035+
"Cannot get index start name if start has not been set");
10451036
if (params.start_at_child_key.has_value()) {
10461037
return *params.start_at_child_key;
10471038
} else if (params.equal_to_child_key.has_value()) {
10481039
return *params.equal_to_child_key;
10491040
} else {
1050-
static std::string s_min_key_str(QueryParamsComparator::kMinKey);
1051-
return s_min_key_str;
1041+
return QueryParamsComparator::kMinKey;
10521042
}
10531043
}
10541044

1055-
const std::string& GetEndName(const QueryParams& params) {
1045+
std::string GetEndName(const QueryParams& params) {
1046+
FIREBASE_DEV_ASSERT_MESSAGE(
1047+
HasEnd(params), "Cannot get index end name if end has not been set");
10561048
if (params.end_at_child_key.has_value()) {
10571049
return *params.end_at_child_key;
10581050
} else if (params.equal_to_child_key.has_value()) {
10591051
return *params.equal_to_child_key;
10601052
} else {
1061-
static std::string s_max_key_str(QueryParamsComparator::kMaxKey);
1062-
return s_max_key_str;
1053+
return QueryParamsComparator::kMaxKey;
10631054
}
10641055
}
10651056

@@ -1078,21 +1069,17 @@ const Variant& GetEndValue(const QueryParams& params) {
10781069
: *params.end_at_value;
10791070
}
10801071

1081-
std::pair<Optional<Variant>, Optional<Variant>> GetStartPost(
1082-
const QueryParams& params) {
1072+
std::pair<Variant, Variant> GetStartPost(const QueryParams& params) {
10831073
if (HasStart(params)) {
1084-
return MakePost(params, Optional<std::string>(GetStartName(params)),
1085-
Optional<Variant>(GetStartValue(params)));
1074+
return MakePost(params, GetStartName(params), GetStartValue(params));
10861075
} else {
10871076
return QueryParamsComparator::kMinNode;
10881077
}
10891078
}
10901079

1091-
std::pair<Optional<Variant>, Optional<Variant>> GetEndPost(
1092-
const QueryParams& params) {
1080+
std::pair<Variant, Variant> GetEndPost(const QueryParams& params) {
10931081
if (HasEnd(params)) {
1094-
return MakePost(params, Optional<std::string>(GetEndName(params)),
1095-
Optional<Variant>(GetEndValue(params)));
1082+
return MakePost(params, GetEndName(params), GetEndValue(params));
10961083
} else {
10971084
return QueryParamsComparator::kMaxNode;
10981085
}

‎database/src/desktop/util_desktop.h

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -279,10 +279,9 @@ const std::string& GetHashRepresentation(const Variant& data,
279279
// SDK
280280
const std::string& GetHash(const Variant& data, std::string* output);
281281

282-
std::pair<Optional<Variant>, Optional<Variant>> MakePost(
283-
const QueryParams& params,
284-
const Optional<std::string>& name,
285-
const Optional<Variant>& value);
282+
std::pair<Variant, Variant> MakePost(const QueryParams& params,
283+
const std::string& name,
284+
const Variant& value);
286285

287286
bool IsValidPriority(const Variant& variant);
288287

@@ -297,12 +296,12 @@ bool HasEnd(const QueryParams& params);
297296
// Get the lower bound of the key for the earliest element that can appear in
298297
// an IndexedVariant associated with these QueryParams. This will either be the
299298
// start_at_child_key or the equal_to_child_key if either is set.
300-
const std::string& GetStartName(const QueryParams& params);
299+
std::string GetStartName(const QueryParams& params);
301300

302301
// Get the upper bound of the key for the latest element that can appear in
303302
// an IndexedVariant associated with these QueryParams. This will either be the
304303
// end_at_child_key or the equal_to_child_key if either is set.
305-
const std::string& GetEndName(const QueryParams& params);
304+
std::string GetEndName(const QueryParams& params);
306305

307306
// Get the lower bound of the value for the earliest element that can appear in
308307
// an IndexedVariant associated with these QueryParams. This will either be the
@@ -316,13 +315,11 @@ const Variant& GetEndValue(const QueryParams& params);
316315

317316
// Get the earliest key/value pair that can appear in a given IndexedVariant,
318317
// based on the sorting order and range given in the QueryParams.
319-
std::pair<Optional<Variant>, Optional<Variant>> GetStartPost(
320-
const QueryParams& params);
318+
std::pair<Variant, Variant> GetStartPost(const QueryParams& params);
321319

322320
// Get the latest key/value pair that can appear in a given IndexedVariant,
323321
// based on the sorting order and range given in the QueryParams.
324-
std::pair<Optional<Variant>, Optional<Variant>> GetEndPost(
325-
const QueryParams& params);
322+
std::pair<Variant, Variant> GetEndPost(const QueryParams& params);
326323

327324
// Returns true if the QuerySpec does no filtering of child data, meaning that
328325
// the data loaded locally under this QuerySpec is a complete view of the data

‎database/src/desktop/view/limited_filter.cc

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -65,9 +65,8 @@ IndexedVariant LimitedFilter::UpdateChild(
6565
template <typename IteratorType>
6666
IndexedVariant UpdateFullVariantHelper(
6767
IndexedVariant filtered, int limit, IteratorType iter,
68-
IteratorType iter_end,
69-
const std::pair<Optional<Variant>, Optional<Variant>>& start_post,
70-
const std::pair<Optional<Variant>, Optional<Variant>>& end_post, int sign,
68+
IteratorType iter_end, const std::pair<Variant, Variant>& start_post,
69+
const std::pair<Variant, Variant>& end_post, int sign,
7170
const QueryParams& params) {
7271
int count = 0;
7372
bool found_start_post = false;

‎database/src/desktop/view/ranged_filter.h

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -55,21 +55,17 @@ class RangedFilter : public VariantFilter {
5555

5656
bool FiltersVariants() const override;
5757

58-
const std::pair<Optional<Variant>, Optional<Variant>>& start_post() const {
59-
return start_post_;
60-
}
58+
const std::pair<Variant, Variant>& start_post() const { return start_post_; }
6159

62-
const std::pair<Optional<Variant>, Optional<Variant>>& end_post() const {
63-
return end_post_;
64-
}
60+
const std::pair<Variant, Variant>& end_post() const { return end_post_; }
6561

6662
bool Matches(const std::pair<Variant, Variant>& node) const;
6763
bool Matches(const Variant& key, const Variant& value) const;
6864

6965
private:
7066
UniquePtr<VariantFilter> indexed_filter_;
71-
std::pair<Optional<Variant>, Optional<Variant>> start_post_;
72-
std::pair<Optional<Variant>, Optional<Variant>> end_post_;
67+
std::pair<Variant, Variant> start_post_;
68+
std::pair<Variant, Variant> end_post_;
7369
};
7470

7571
} // namespace internal

0 commit comments

Comments
 (0)