Skip to content

Commit 33e5d73

Browse files
alexamesalexames
authored
Merge pull request #391 from firebase/bugfix/optional-query-param-fields
Made parameters in QueryParams optional.
2 parents b8b1f19 + 785f2ab commit 33e5d73

File tree

6 files changed

+125
-80
lines changed

6 files changed

+125
-80
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/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/util_desktop.cc

Lines changed: 40 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1022,30 +1022,33 @@ std::pair<Variant, Variant> MakePost(const QueryParams& params,
10221022
}
10231023

10241024
bool HasStart(const QueryParams& params) {
1025-
return !params.start_at_value.is_null() || !params.equal_to_value.is_null() ||
1026-
GetStartName(params) != QueryParamsComparator::kMinKey;
1025+
return params.start_at_value.has_value() || params.equal_to_value.has_value();
10271026
}
10281027

10291028
bool HasEnd(const QueryParams& params) {
1030-
return !params.end_at_value.is_null() || !params.equal_to_value.is_null() ||
1031-
GetEndName(params) != QueryParamsComparator::kMaxKey;
1029+
return params.end_at_value.has_value() || params.equal_to_value.has_value();
10321030
}
10331031

10341032
std::string GetStartName(const QueryParams& params) {
1035-
if (!params.start_at_child_key.empty()) {
1036-
return params.start_at_child_key;
1037-
} else if (!params.equal_to_child_key.empty()) {
1038-
return params.equal_to_child_key;
1033+
FIREBASE_DEV_ASSERT_MESSAGE(
1034+
HasStart(params),
1035+
"Cannot get index start name if start has not been set");
1036+
if (params.start_at_child_key.has_value()) {
1037+
return *params.start_at_child_key;
1038+
} else if (params.equal_to_child_key.has_value()) {
1039+
return *params.equal_to_child_key;
10391040
} else {
10401041
return QueryParamsComparator::kMinKey;
10411042
}
10421043
}
10431044

10441045
std::string GetEndName(const QueryParams& params) {
1045-
if (!params.end_at_child_key.empty()) {
1046-
return params.end_at_child_key;
1047-
} else if (!params.equal_to_child_key.empty()) {
1048-
return params.equal_to_child_key;
1046+
FIREBASE_DEV_ASSERT_MESSAGE(
1047+
HasEnd(params), "Cannot get index end name if end has not been set");
1048+
if (params.end_at_child_key.has_value()) {
1049+
return *params.end_at_child_key;
1050+
} else if (params.equal_to_child_key.has_value()) {
1051+
return *params.equal_to_child_key;
10491052
} else {
10501053
return QueryParamsComparator::kMaxKey;
10511054
}
@@ -1055,15 +1058,15 @@ const Variant& GetStartValue(const QueryParams& params) {
10551058
FIREBASE_DEV_ASSERT_MESSAGE(
10561059
HasStart(params),
10571060
"Cannot get index start value if start has not been set");
1058-
return params.equal_to_value.is_null() ? params.start_at_value
1059-
: params.equal_to_value;
1061+
return params.equal_to_value.has_value() ? *params.equal_to_value
1062+
: *params.start_at_value;
10601063
}
10611064

10621065
const Variant& GetEndValue(const QueryParams& params) {
10631066
FIREBASE_DEV_ASSERT_MESSAGE(
10641067
HasEnd(params), "Cannot get index end value if end has not been set");
1065-
return params.equal_to_value.is_null() ? params.end_at_value
1066-
: params.equal_to_value;
1068+
return params.equal_to_value.has_value() ? *params.equal_to_value
1069+
: *params.end_at_value;
10671070
}
10681071

10691072
std::pair<Variant, Variant> GetStartPost(const QueryParams& params) {
@@ -1087,10 +1090,13 @@ bool QuerySpecLoadsAllData(const QuerySpec& query_spec) {
10871090
}
10881091

10891092
bool QueryParamsLoadsAllData(const QueryParams& params) {
1090-
return params.start_at_value.is_null() && params.start_at_child_key.empty() &&
1091-
params.end_at_value.is_null() && params.end_at_child_key.empty() &&
1092-
params.equal_to_value.is_null() && params.equal_to_child_key.empty() &&
1093-
params.limit_first == 0 && params.limit_last == 0;
1093+
return !params.start_at_value.has_value() &&
1094+
!params.start_at_child_key.has_value() &&
1095+
!params.end_at_value.has_value() &&
1096+
!params.end_at_child_key.has_value() &&
1097+
!params.equal_to_value.has_value() &&
1098+
!params.equal_to_child_key.has_value() && params.limit_first == 0 &&
1099+
params.limit_last == 0;
10941100
}
10951101

10961102
bool QuerySpecIsDefault(const QuerySpec& query_spec) {
@@ -1128,31 +1134,31 @@ std::string WireProtocolPathToString(const Path& path) {
11281134
Variant GetWireProtocolParams(const QueryParams& query_params) {
11291135
Variant result = Variant::EmptyMap();
11301136

1131-
if (!query_params.start_at_value.is_null()) {
1132-
result.map()[kQueryParamsIndexStartValue] = query_params.start_at_value;
1133-
if (!query_params.start_at_child_key.empty()) {
1137+
if (query_params.start_at_value.has_value()) {
1138+
result.map()[kQueryParamsIndexStartValue] = *query_params.start_at_value;
1139+
if (query_params.start_at_child_key.has_value()) {
11341140
result.map()[kQueryParamsIndexStartName] =
1135-
query_params.start_at_child_key;
1141+
*query_params.start_at_child_key;
11361142
}
11371143
}
11381144

1139-
if (!query_params.end_at_value.is_null()) {
1140-
result.map()[kQueryParamsIndexEndValue] = query_params.end_at_value;
1141-
if (!query_params.end_at_child_key.empty()) {
1142-
result.map()[kQueryParamsIndexEndName] = query_params.end_at_child_key;
1145+
if (query_params.end_at_value.has_value()) {
1146+
result.map()[kQueryParamsIndexEndValue] = *query_params.end_at_value;
1147+
if (query_params.end_at_child_key.has_value()) {
1148+
result.map()[kQueryParamsIndexEndName] = *query_params.end_at_child_key;
11431149
}
11441150
}
11451151

11461152
// QueryParams in Android implementation does not really have "equal_to"
11471153
// property. Instead, it is converted into "start_at" and "end_at" with the
11481154
// same value.
1149-
if (!query_params.equal_to_value.is_null()) {
1150-
result.map()[kQueryParamsIndexStartValue] = query_params.equal_to_value;
1151-
result.map()[kQueryParamsIndexEndValue] = query_params.equal_to_value;
1152-
if (!query_params.equal_to_child_key.empty()) {
1155+
if (query_params.equal_to_value.has_value()) {
1156+
result.map()[kQueryParamsIndexStartValue] = *query_params.equal_to_value;
1157+
result.map()[kQueryParamsIndexEndValue] = *query_params.equal_to_value;
1158+
if (query_params.equal_to_child_key.has_value()) {
11531159
result.map()[kQueryParamsIndexStartName] =
1154-
query_params.equal_to_child_key;
1155-
result.map()[kQueryParamsIndexEndName] = query_params.equal_to_child_key;
1160+
*query_params.equal_to_child_key;
1161+
result.map()[kQueryParamsIndexEndName] = *query_params.equal_to_child_key;
11561162
}
11571163
}
11581164

‎database/tests/desktop/core/indexed_variant_test.cc

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -82,23 +82,23 @@ std::string QueryParamsToString(const QueryParams& params) {
8282
break;
8383
}
8484

85-
if (!params.equal_to_value.is_null()) {
86-
ss << ", equal_to_value=" << util::VariantToJson(params.equal_to_value);
85+
if (params.equal_to_value.has_value()) {
86+
ss << ", equal_to_value=" << util::VariantToJson(*params.equal_to_value);
8787
}
88-
if (!params.equal_to_child_key.empty()) {
89-
ss << ", equal_to_child_key=" << params.equal_to_child_key;
88+
if (params.equal_to_child_key.has_value()) {
89+
ss << ", equal_to_child_key=" << *params.equal_to_child_key;
9090
}
91-
if (!params.start_at_value.is_null()) {
92-
ss << ", start_at_value=" << util::VariantToJson(params.start_at_value);
91+
if (params.start_at_value.has_value()) {
92+
ss << ", start_at_value=" << util::VariantToJson(*params.start_at_value);
9393
}
94-
if (!params.start_at_child_key.empty()) {
95-
ss << ", start_at_child_key=" << params.start_at_child_key;
94+
if (params.start_at_child_key.has_value()) {
95+
ss << ", start_at_child_key=" << *params.start_at_child_key;
9696
}
97-
if (!params.end_at_value.is_null()) {
98-
ss << ", end_at_value=" << util::VariantToJson(params.end_at_value);
97+
if (params.end_at_value.has_value()) {
98+
ss << ", end_at_value=" << util::VariantToJson(*params.end_at_value);
9999
}
100-
if (!params.end_at_child_key.empty()) {
101-
ss << ", end_at_child_key=" << params.end_at_child_key;
100+
if (params.end_at_child_key.has_value()) {
101+
ss << ", end_at_child_key=" << *params.end_at_child_key;
102102
}
103103
if (params.limit_first != 0) {
104104
ss << ", limit_first=" << params.limit_first;

‎database/tests/desktop/core/sync_point_spec_test.cc

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -177,7 +177,7 @@ UniquePtr<QueryInternal> ParseQuery(
177177
}
178178
if (query_params->startAt()) {
179179
const test_data::Bound* bound = query_params->startAt();
180-
const char* name = bound->name() ? bound->name()->c_str() : "";
180+
const char* name = bound->name() ? bound->name()->c_str() : nullptr;
181181
Variant index =
182182
bound->index()
183183
? util::FlexbufferToVariant(bound->index_flexbuffer_root())
@@ -186,7 +186,7 @@ UniquePtr<QueryInternal> ParseQuery(
186186
}
187187
if (query_params->endAt()) {
188188
const test_data::Bound* bound = query_params->endAt();
189-
const char* name = bound->name() ? bound->name()->c_str() : "";
189+
const char* name = bound->name() ? bound->name()->c_str() : nullptr;
190190
Variant index =
191191
bound->index()
192192
? util::FlexbufferToVariant(bound->index_flexbuffer_root())
@@ -195,7 +195,7 @@ UniquePtr<QueryInternal> ParseQuery(
195195
}
196196
if (query_params->equalTo()) {
197197
const test_data::Bound* bound = query_params->equalTo();
198-
const char* name = bound->name() ? bound->name()->c_str() : "";
198+
const char* name = bound->name() ? bound->name()->c_str() : nullptr;
199199
Variant index =
200200
bound->index()
201201
? util::FlexbufferToVariant(bound->index_flexbuffer_root())
@@ -684,7 +684,6 @@ TEST_F(SyncTreeTest, UserDeepSetPullsInCorrectValues) {
684684
}
685685

686686
TEST_F(SyncTreeTest, QueriesWithEqualToNullWork) {
687-
GTEST_SKIP(); // Fails expectations.
688687
RunOne("Queries with equalTo(null) work");
689688
}
690689

0 commit comments

Comments
 (0)