Skip to content

Commit 8fb5350

Browse files
schmidt-sebastianschmidt-sebastian
authored
Include QueryTarget proto in error messages when serialization fails (#6818)
1 parent e6ada91 commit 8fb5350

File tree

11 files changed

+155
-76
lines changed

11 files changed

+155
-76
lines changed

‎Firestore/CHANGELOG.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
1-
# Unreleased
1+
# Unreleased (v7.0.1)
2+
- [changed] Added the original query data to error messages for Queries that
3+
cannot be deserizialized.
24
- [fixed] Remove explicit MobileCoreServices library linkage from podspec
35
(#6850).
46
- [fixed] Removed excess validation of null and NaN values in query filters.

‎Firestore/core/src/local/leveldb_target_cache.cc

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -391,7 +391,8 @@ TargetData LevelDbTargetCache::DecodeTarget(absl::string_view encoded) {
391391
auto message = Message<firestore_client_Target>::TryParse(&reader);
392392
auto result = serializer_->DecodeTargetData(&reader, *message);
393393
if (!reader.ok()) {
394-
HARD_FAIL("Target proto failed to parse: %s", reader.status().ToString());
394+
HARD_FAIL("Target proto failed to parse: %s, message: %s",
395+
reader.status().ToString(), message.ToString());
395396
}
396397

397398
return result;

‎Firestore/core/src/model/field_path.cc

Lines changed: 37 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@
2121

2222
#include "Firestore/core/src/util/exception.h"
2323
#include "Firestore/core/src/util/hard_assert.h"
24+
#include "Firestore/core/src/util/status.h"
25+
#include "Firestore/core/src/util/statusor.h"
2426
#include "absl/strings/str_join.h"
2527
#include "absl/strings/str_replace.h"
2628
#include "absl/strings/str_split.h"
@@ -30,6 +32,9 @@ namespace firestore {
3032
namespace model {
3133
namespace {
3234

35+
using util::Status;
36+
using util::StatusOr;
37+
using util::StringFormat;
3338
using util::ThrowInvalidArgument;
3439

3540
/**
@@ -109,23 +114,30 @@ FieldPath FieldPath::FromDotSeparatedStringView(absl::string_view path) {
109114
return FieldPath(std::move(segments));
110115
}
111116

112-
FieldPath FieldPath::FromServerFormat(const std::string& path) {
117+
StatusOr<FieldPath> FieldPath::FromServerFormat(const std::string& path) {
113118
return FromServerFormatView(path);
114119
}
115120

116-
FieldPath FieldPath::FromServerFormatView(absl::string_view path) {
121+
StatusOr<FieldPath> FieldPath::FromServerFormatView(absl::string_view path) {
117122
SegmentsT segments;
118123
std::string segment;
119124
segment.reserve(path.size());
120125

126+
Status status;
127+
121128
const auto finish_segment = [&segments, &segment, &path] {
122-
HARD_ASSERT(!segment.empty(),
123-
"Invalid field path (%s). Paths must not be empty, begin with "
124-
"'.', end with '.', or contain '..'",
125-
path);
129+
if (segment.empty()) {
130+
return Status{
131+
Error::kErrorInvalidArgument,
132+
StringFormat(
133+
"Invalid field path (%s). Paths must not be empty, begin with "
134+
"'.', end with '.', or contain '..'",
135+
path)};
136+
}
126137
// Move operation will clear segment, but capacity will remain the same
127138
// (not, strictly speaking, required by the standard, but true in practice).
128139
segments.push_back(std::move(segment));
140+
return Status::OK();
129141
};
130142

131143
// Inside backticks, dots are treated literally.
@@ -143,7 +155,7 @@ FieldPath FieldPath::FromServerFormatView(absl::string_view path) {
143155
switch (c) {
144156
case '.':
145157
if (!inside_backticks) {
146-
finish_segment();
158+
status = finish_segment();
147159
} else {
148160
segment += c;
149161
}
@@ -154,21 +166,33 @@ FieldPath FieldPath::FromServerFormatView(absl::string_view path) {
154166
break;
155167

156168
case '\\':
157-
HARD_ASSERT(i + 1 != path.size(),
158-
"Trailing escape characters not allowed in %s", path);
159-
++i;
160-
segment += path[i];
169+
if (i + 1 == path.size()) {
170+
status =
171+
Status{Error::kErrorInvalidArgument,
172+
StringFormat(
173+
"Trailing escape characters not allowed in %s", path)};
174+
} else {
175+
++i;
176+
segment += path[i];
177+
}
161178
break;
162179

163180
default:
164181
segment += c;
165182
break;
166183
}
167184
++i;
185+
186+
if (!status.ok()) return status;
168187
}
169-
finish_segment();
170188

171-
HARD_ASSERT(!inside_backticks, "Unterminated ` in path %s", path);
189+
status = finish_segment();
190+
if (!status.ok()) return status;
191+
192+
if (inside_backticks) {
193+
return Status{Error::kErrorInvalidArgument,
194+
StringFormat("Unterminated ` in path %s", path)};
195+
}
172196

173197
return FieldPath{std::move(segments)};
174198
}

‎Firestore/core/src/model/field_path.h

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
#include <utility>
2323

2424
#include "Firestore/core/src/model/base_path.h"
25+
#include "Firestore/core/src/util/status_fwd.h"
2526
#include "absl/strings/string_view.h"
2627

2728
namespace firebase {
@@ -89,12 +90,12 @@ class FieldPath : public impl::BasePath<FieldPath>,
8990
* where path segments are separated by a dot "." and optionally encoded using
9091
* backticks.
9192
*/
92-
static FieldPath FromServerFormat(const std::string& path);
93+
static util::StatusOr<FieldPath> FromServerFormat(const std::string& path);
9394

9495
private:
9596
// TODO(b/146372592): Make this public once we can use Abseil across
9697
// iOS/public C++ library boundaries.
97-
static FieldPath FromServerFormatView(absl::string_view path);
98+
static util::StatusOr<FieldPath> FromServerFormatView(absl::string_view path);
9899

99100
public:
100101
/** Returns a field path that represents an empty path. */

‎Firestore/core/src/remote/serializer.cc

Lines changed: 27 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,8 @@
5151
#include "Firestore/core/src/nanopb/writer.h"
5252
#include "Firestore/core/src/timestamp_internal.h"
5353
#include "Firestore/core/src/util/hard_assert.h"
54+
#include "Firestore/core/src/util/status.h"
55+
#include "Firestore/core/src/util/statusor.h"
5456
#include "Firestore/core/src/util/string_format.h"
5557
#include "absl/algorithm/container.h"
5658

@@ -106,6 +108,7 @@ using nanopb::SafeReadBoolean;
106108
using nanopb::Writer;
107109
using remote::WatchChange;
108110
using util::Status;
111+
using util::StatusOr;
109112
using util::StringFormat;
110113

111114
pb_bytes_array_t* Serializer::EncodeString(const std::string& str) {
@@ -158,6 +161,10 @@ Filter InvalidFilter() {
158161
return FieldFilter::Create({}, {}, {});
159162
}
160163

164+
FieldPath InvalidFieldPath() {
165+
return FieldPath::EmptyPath();
166+
}
167+
161168
} // namespace
162169

163170
Serializer::Serializer(DatabaseId database_id)
@@ -659,7 +666,7 @@ Mutation Serializer::DecodeMutation(
659666
ObjectValue value = DecodeFields(reader, mutation.update.fields_count,
660667
mutation.update.fields);
661668
if (mutation.has_update_mask) {
662-
FieldMask mask = DecodeFieldMask(mutation.update_mask);
669+
FieldMask mask = DecodeFieldMask(reader, mutation.update_mask);
663670
return PatchMutation(std::move(key), std::move(value), std::move(mask),
664671
std::move(precondition));
665672
} else {
@@ -775,10 +782,10 @@ google_firestore_v1_DocumentMask Serializer::EncodeFieldMask(
775782

776783
/* static */
777784
FieldMask Serializer::DecodeFieldMask(
778-
const google_firestore_v1_DocumentMask& mask) {
785+
nanopb::Reader* reader, const google_firestore_v1_DocumentMask& mask) {
779786
std::set<FieldPath> fields;
780787
for (size_t i = 0; i < mask.field_paths_count; i++) {
781-
fields.insert(DecodeFieldPath(mask.field_paths[i]));
788+
fields.insert(DecodeFieldPath(reader, mask.field_paths[i]));
782789
}
783790
return FieldMask(std::move(fields));
784791
}
@@ -828,21 +835,22 @@ Serializer::EncodeFieldTransform(const FieldTransform& field_transform) const {
828835
FieldTransform Serializer::DecodeFieldTransform(
829836
nanopb::Reader* reader,
830837
const google_firestore_v1_DocumentTransform_FieldTransform& proto) const {
838+
FieldPath field = DecodeFieldPath(reader, proto.field_path);
839+
831840
switch (proto.which_transform_type) {
832841
case google_firestore_v1_DocumentTransform_FieldTransform_set_to_server_value_tag: { // NOLINT
833842
HARD_ASSERT(
834843
proto.set_to_server_value ==
835844
google_firestore_v1_DocumentTransform_FieldTransform_ServerValue_REQUEST_TIME, // NOLINT
836845
"Unknown transform setToServerValue: %s", proto.set_to_server_value);
837846

838-
return FieldTransform(DecodeFieldPath(proto.field_path),
839-
ServerTimestampTransform());
847+
return FieldTransform(std::move(field), ServerTimestampTransform());
840848
}
841849

842850
case google_firestore_v1_DocumentTransform_FieldTransform_append_missing_elements_tag: { // NOLINT
843851
std::vector<FieldValue> elements =
844852
DecodeArray(reader, proto.append_missing_elements);
845-
return FieldTransform(DecodeFieldPath(proto.field_path),
853+
return FieldTransform(std::move(field),
846854
ArrayTransform(TransformOperation::Type::ArrayUnion,
847855
std::move(elements)));
848856
}
@@ -851,14 +859,14 @@ FieldTransform Serializer::DecodeFieldTransform(
851859
std::vector<FieldValue> elements =
852860
DecodeArray(reader, proto.remove_all_from_array);
853861
return FieldTransform(
854-
DecodeFieldPath(proto.field_path),
862+
std::move(field),
855863
ArrayTransform(TransformOperation::Type::ArrayRemove,
856864
std::move(elements)));
857865
}
858866

859867
case google_firestore_v1_DocumentTransform_FieldTransform_increment_tag: {
860868
FieldValue operand = DecodeFieldValue(reader, proto.increment);
861-
return FieldTransform(DecodeFieldPath(proto.field_path),
869+
return FieldTransform(std::move(field),
862870
NumericIncrementTransform(std::move(operand)));
863871
}
864872
}
@@ -1145,8 +1153,7 @@ google_firestore_v1_StructuredQuery_Filter Serializer::EncodeSingularFilter(
11451153
Filter Serializer::DecodeFieldFilter(
11461154
nanopb::Reader* reader,
11471155
const google_firestore_v1_StructuredQuery_FieldFilter& field_filter) const {
1148-
FieldPath field_path =
1149-
FieldPath::FromServerFormat(DecodeString(field_filter.field.field_path));
1156+
FieldPath field_path = DecodeFieldPath(reader, field_filter.field.field_path);
11501157
Filter::Operator op = DecodeFieldFilterOperator(reader, field_filter.op);
11511158
FieldValue value = DecodeFieldValue(reader, field_filter.value);
11521159

@@ -1161,8 +1168,7 @@ Filter Serializer::DecodeUnaryFilter(
11611168
"Unexpected UnaryFilter.which_operand_type: %s",
11621169
unary.which_operand_type);
11631170

1164-
auto field =
1165-
FieldPath::FromServerFormat(DecodeString(unary.field.field_path));
1171+
FieldPath field = DecodeFieldPath(reader, unary.field.field_path);
11661172

11671173
switch (unary.op) {
11681174
case google_firestore_v1_StructuredQuery_UnaryFilter_Operator_IS_NULL:
@@ -1344,8 +1350,7 @@ OrderByList Serializer::DecodeOrderBys(
13441350
OrderBy Serializer::DecodeOrderBy(
13451351
nanopb::Reader* reader,
13461352
const google_firestore_v1_StructuredQuery_Order& order_by) const {
1347-
auto field_path =
1348-
FieldPath::FromServerFormat(DecodeString(order_by.field.field_path));
1353+
auto field_path = DecodeFieldPath(reader, order_by.field.field_path);
13491354

13501355
Direction direction;
13511356
switch (order_by.direction) {
@@ -1403,9 +1408,15 @@ pb_bytes_array_t* Serializer::EncodeFieldPath(const FieldPath& field_path) {
14031408
}
14041409

14051410
/* static */
1406-
FieldPath Serializer::DecodeFieldPath(const pb_bytes_array_t* field_path) {
1411+
FieldPath Serializer::DecodeFieldPath(nanopb::Reader* reader,
1412+
const pb_bytes_array_t* field_path) {
14071413
absl::string_view str = MakeStringView(field_path);
1408-
return FieldPath::FromServerFormatView(str);
1414+
StatusOr<FieldPath> decoded_path = FieldPath::FromServerFormatView(str);
1415+
if (!decoded_path.ok()) {
1416+
reader->set_status(decoded_path.status());
1417+
return InvalidFieldPath();
1418+
}
1419+
return decoded_path.ConsumeValueOrDie();
14091420
}
14101421

14111422
google_protobuf_Timestamp Serializer::EncodeVersion(

‎Firestore/core/src/remote/serializer.h

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -172,7 +172,7 @@ class Serializer {
172172
static google_firestore_v1_DocumentMask EncodeFieldMask(
173173
const model::FieldMask& mask);
174174
static model::FieldMask DecodeFieldMask(
175-
const google_firestore_v1_DocumentMask& mask);
175+
nanopb::Reader* reader, const google_firestore_v1_DocumentMask& mask);
176176

177177
google_firestore_v1_DocumentTransform_FieldTransform EncodeFieldTransform(
178178
const model::FieldTransform& field_transform) const;
@@ -189,7 +189,8 @@ class Serializer {
189189
EncodeListenRequestLabels(const local::TargetData& target_data) const;
190190

191191
static pb_bytes_array_t* EncodeFieldPath(const model::FieldPath& field_path);
192-
static model::FieldPath DecodeFieldPath(const pb_bytes_array_t* field_path);
192+
static model::FieldPath DecodeFieldPath(nanopb::Reader* reader,
193+
const pb_bytes_array_t* field_path);
193194

194195
static google_protobuf_Timestamp EncodeVersion(
195196
const model::SnapshotVersion& version);

‎Firestore/core/test/unit/local/local_serializer_test.cc

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -352,6 +352,34 @@ TEST_F(LocalSerializerTest, EncodesTargetData) {
352352
ExpectRoundTrip(target_data, expected);
353353
}
354354

355+
TEST_F(LocalSerializerTest, HandlesInvalidTargetData) {
356+
TargetId target_id = 42;
357+
std::string invalid_field_path = "`";
358+
359+
::firestore::client::Target invalid_target;
360+
invalid_target.set_target_id(target_id);
361+
v1::Target::QueryTarget* query_proto = invalid_target.mutable_query();
362+
363+
// Add expected collection.
364+
query_proto->set_parent("projects/p/databases/d/documents");
365+
v1::StructuredQuery::CollectionSelector from;
366+
from.set_collection_id("room");
367+
*query_proto->mutable_structured_query()->add_from() = std::move(from);
368+
369+
// Add invalid order_by.
370+
v1::StructuredQuery::Order order;
371+
order.mutable_field()->set_field_path(invalid_field_path);
372+
order.set_direction(v1::StructuredQuery::ASCENDING);
373+
*query_proto->mutable_structured_query()->add_order_by() = std::move(order);
374+
375+
ByteString bytes = ProtobufSerialize(invalid_target);
376+
StringReader reader(bytes);
377+
378+
auto message = Message<firestore_client_Target>::TryParse(&reader);
379+
serializer.DecodeTargetData(&reader, *message);
380+
EXPECT_NOT_OK(reader.status());
381+
}
382+
355383
TEST_F(LocalSerializerTest, EncodesTargetDataWithDocumentQuery) {
356384
core::Query query = Query("room/1");
357385
TargetId target_id = 42;

‎Firestore/core/test/unit/model/field_mask_test.cc

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -26,13 +26,14 @@ namespace firestore {
2626
namespace model {
2727

2828
TEST(FieldMask, ConstructorAndEqual) {
29-
FieldMask mask_a{FieldPath::FromServerFormat("foo"),
30-
FieldPath::FromServerFormat("bar")};
31-
std::set<FieldPath> field_path_set{FieldPath::FromServerFormat("foo"),
32-
FieldPath::FromServerFormat("bar")};
29+
FieldMask mask_a{FieldPath::FromDotSeparatedString("foo"),
30+
FieldPath::FromDotSeparatedString("bar")};
31+
std::set<FieldPath> field_path_set{FieldPath::FromDotSeparatedString("foo"),
32+
FieldPath::FromDotSeparatedString("bar")};
3333
FieldMask mask_b{field_path_set};
34-
FieldMask mask_c{std::set<FieldPath>{FieldPath::FromServerFormat("foo"),
35-
FieldPath::FromServerFormat("bar")}};
34+
FieldMask mask_c{
35+
std::set<FieldPath>{FieldPath::FromDotSeparatedString("foo"),
36+
FieldPath::FromDotSeparatedString("bar")}};
3637
FieldMask mask_d{field_path_set.begin(), field_path_set.end()};
3738

3839
EXPECT_EQ(mask_a, mask_b);
@@ -41,16 +42,16 @@ TEST(FieldMask, ConstructorAndEqual) {
4142
}
4243

4344
TEST(FieldMask, Getter) {
44-
FieldMask mask{FieldPath::FromServerFormat("foo"),
45-
FieldPath::FromServerFormat("bar")};
46-
EXPECT_EQ(std::set<FieldPath>({FieldPath::FromServerFormat("foo"),
47-
FieldPath::FromServerFormat("bar")}),
45+
FieldMask mask{FieldPath::FromDotSeparatedString("foo"),
46+
FieldPath::FromDotSeparatedString("bar")};
47+
EXPECT_EQ(std::set<FieldPath>({FieldPath::FromDotSeparatedString("foo"),
48+
FieldPath::FromDotSeparatedString("bar")}),
4849
std::set<FieldPath>(mask.begin(), mask.end()));
4950
}
5051

5152
TEST(FieldMask, ToString) {
52-
FieldMask mask{FieldPath::FromServerFormat("foo"),
53-
FieldPath::FromServerFormat("bar")};
53+
FieldMask mask{FieldPath::FromDotSeparatedString("foo"),
54+
FieldPath::FromDotSeparatedString("bar")};
5455
EXPECT_EQ("{ bar foo }", mask.ToString());
5556
}
5657

0 commit comments

Comments
 (0)