Include QueryTarget proto in error messages when serialization fails (#6818)

Author: schmidt-sebastian

Firestore/CHANGELOG.md

@@ -1,4 +1,6 @@
-# Unreleased
+# Unreleased (v7.0.1)
+- [changed] Added the original query data to error messages for Queries that
+  cannot be deserizialized.
 - [fixed] Remove explicit MobileCoreServices library linkage from podspec
   (#6850).
 - [fixed] Removed excess validation of null and NaN values in query filters.

Firestore/core/src/local/leveldb_target_cache.cc

@@ -391,7 +391,8 @@ TargetData LevelDbTargetCache::DecodeTarget(absl::string_view encoded) {
   auto message = Message<firestore_client_Target>::TryParse(&reader);
   auto result = serializer_->DecodeTargetData(&reader, *message);
   if (!reader.ok()) {
-    HARD_FAIL("Target proto failed to parse: %s", reader.status().ToString());
+    HARD_FAIL("Target proto failed to parse: %s, message: %s",
+              reader.status().ToString(), message.ToString());
   }
 
   return result;

Firestore/core/src/model/field_path.cc

@@ -21,6 +21,8 @@
 
 #include "Firestore/core/src/util/exception.h"
 #include "Firestore/core/src/util/hard_assert.h"
+#include "Firestore/core/src/util/status.h"
+#include "Firestore/core/src/util/statusor.h"
 #include "absl/strings/str_join.h"
 #include "absl/strings/str_replace.h"
 #include "absl/strings/str_split.h"
@@ -30,6 +32,9 @@ namespace firestore {
 namespace model {
 namespace {
 
+using util::Status;
+using util::StatusOr;
+using util::StringFormat;
 using util::ThrowInvalidArgument;
 
 /**
@@ -109,23 +114,30 @@ FieldPath FieldPath::FromDotSeparatedStringView(absl::string_view path) {
   return FieldPath(std::move(segments));
 }
 
-FieldPath FieldPath::FromServerFormat(const std::string& path) {
+StatusOr<FieldPath> FieldPath::FromServerFormat(const std::string& path) {
   return FromServerFormatView(path);
 }
 
-FieldPath FieldPath::FromServerFormatView(absl::string_view path) {
+StatusOr<FieldPath> FieldPath::FromServerFormatView(absl::string_view path) {
   SegmentsT segments;
   std::string segment;
   segment.reserve(path.size());
 
+  Status status;
+
   const auto finish_segment = [&segments, &segment, &path] {
-    HARD_ASSERT(!segment.empty(),
-                "Invalid field path (%s). Paths must not be empty, begin with "
-                "'.', end with '.', or contain '..'",
-                path);
+    if (segment.empty()) {
+      return Status{
+          Error::kErrorInvalidArgument,
+          StringFormat(
+              "Invalid field path (%s). Paths must not be empty, begin with "
+              "'.', end with '.', or contain '..'",
+              path)};
+    }
     // Move operation will clear segment, but capacity will remain the same
     // (not, strictly speaking, required by the standard, but true in practice).
     segments.push_back(std::move(segment));
+    return Status::OK();
   };
 
   // Inside backticks, dots are treated literally.
@@ -143,7 +155,7 @@ FieldPath FieldPath::FromServerFormatView(absl::string_view path) {
     switch (c) {
       case '.':
         if (!inside_backticks) {
-          finish_segment();
+          status = finish_segment();
         } else {
           segment += c;
         }
@@ -154,21 +166,33 @@ FieldPath FieldPath::FromServerFormatView(absl::string_view path) {
         break;
 
       case '\\':
-        HARD_ASSERT(i + 1 != path.size(),
-                    "Trailing escape characters not allowed in %s", path);
-        ++i;
-        segment += path[i];
+        if (i + 1 == path.size()) {
+          status =
+              Status{Error::kErrorInvalidArgument,
+                     StringFormat(
+                         "Trailing escape characters not allowed in %s", path)};
+        } else {
+          ++i;
+          segment += path[i];
+        }
         break;
 
       default:
         segment += c;
         break;
     }
     ++i;
+
+    if (!status.ok()) return status;
   }
-  finish_segment();
 
-  HARD_ASSERT(!inside_backticks, "Unterminated ` in path %s", path);
+  status = finish_segment();
+  if (!status.ok()) return status;
+
+  if (inside_backticks) {
+    return Status{Error::kErrorInvalidArgument,
+                  StringFormat("Unterminated ` in path %s", path)};
+  }
 
   return FieldPath{std::move(segments)};
 }

Firestore/core/src/model/field_path.h

@@ -22,6 +22,7 @@
 #include <utility>
 
 #include "Firestore/core/src/model/base_path.h"
+#include "Firestore/core/src/util/status_fwd.h"
 #include "absl/strings/string_view.h"
 
 namespace firebase {
@@ -89,12 +90,12 @@ class FieldPath : public impl::BasePath<FieldPath>,
    * where path segments are separated by a dot "." and optionally encoded using
    * backticks.
    */
-  static FieldPath FromServerFormat(const std::string& path);
+  static util::StatusOr<FieldPath> FromServerFormat(const std::string& path);
 
  private:
   // TODO(b/146372592): Make this public once we can use Abseil across
   // iOS/public C++ library boundaries.
-  static FieldPath FromServerFormatView(absl::string_view path);
+  static util::StatusOr<FieldPath> FromServerFormatView(absl::string_view path);
 
  public:
   /** Returns a field path that represents an empty path. */

Firestore/core/src/remote/serializer.cc

@@ -51,6 +51,8 @@
 #include "Firestore/core/src/nanopb/writer.h"
 #include "Firestore/core/src/timestamp_internal.h"
 #include "Firestore/core/src/util/hard_assert.h"
+#include "Firestore/core/src/util/status.h"
+#include "Firestore/core/src/util/statusor.h"
 #include "Firestore/core/src/util/string_format.h"
 #include "absl/algorithm/container.h"
 
@@ -106,6 +108,7 @@ using nanopb::SafeReadBoolean;
 using nanopb::Writer;
 using remote::WatchChange;
 using util::Status;
+using util::StatusOr;
 using util::StringFormat;
 
 pb_bytes_array_t* Serializer::EncodeString(const std::string& str) {
@@ -158,6 +161,10 @@ Filter InvalidFilter() {
   return FieldFilter::Create({}, {}, {});
 }
 
+FieldPath InvalidFieldPath() {
+  return FieldPath::EmptyPath();
+}
+
 }  // namespace
 
 Serializer::Serializer(DatabaseId database_id)
@@ -659,7 +666,7 @@ Mutation Serializer::DecodeMutation(
       ObjectValue value = DecodeFields(reader, mutation.update.fields_count,
                                        mutation.update.fields);
       if (mutation.has_update_mask) {
-        FieldMask mask = DecodeFieldMask(mutation.update_mask);
+        FieldMask mask = DecodeFieldMask(reader, mutation.update_mask);
         return PatchMutation(std::move(key), std::move(value), std::move(mask),
                              std::move(precondition));
       } else {
@@ -775,10 +782,10 @@ google_firestore_v1_DocumentMask Serializer::EncodeFieldMask(
 
 /* static */
 FieldMask Serializer::DecodeFieldMask(
-    const google_firestore_v1_DocumentMask& mask) {
+    nanopb::Reader* reader, const google_firestore_v1_DocumentMask& mask) {
   std::set<FieldPath> fields;
   for (size_t i = 0; i < mask.field_paths_count; i++) {
-    fields.insert(DecodeFieldPath(mask.field_paths[i]));
+    fields.insert(DecodeFieldPath(reader, mask.field_paths[i]));
   }
   return FieldMask(std::move(fields));
 }
@@ -828,21 +835,22 @@ Serializer::EncodeFieldTransform(const FieldTransform& field_transform) const {
 FieldTransform Serializer::DecodeFieldTransform(
     nanopb::Reader* reader,
     const google_firestore_v1_DocumentTransform_FieldTransform& proto) const {
+  FieldPath field = DecodeFieldPath(reader, proto.field_path);
+
   switch (proto.which_transform_type) {
     case google_firestore_v1_DocumentTransform_FieldTransform_set_to_server_value_tag: {  // NOLINT
       HARD_ASSERT(
           proto.set_to_server_value ==
               google_firestore_v1_DocumentTransform_FieldTransform_ServerValue_REQUEST_TIME,  // NOLINT
           "Unknown transform setToServerValue: %s", proto.set_to_server_value);
 
-      return FieldTransform(DecodeFieldPath(proto.field_path),
-                            ServerTimestampTransform());
+      return FieldTransform(std::move(field), ServerTimestampTransform());
     }
 
     case google_firestore_v1_DocumentTransform_FieldTransform_append_missing_elements_tag: {  // NOLINT
       std::vector<FieldValue> elements =
           DecodeArray(reader, proto.append_missing_elements);
-      return FieldTransform(DecodeFieldPath(proto.field_path),
+      return FieldTransform(std::move(field),
                             ArrayTransform(TransformOperation::Type::ArrayUnion,
                                            std::move(elements)));
     }
@@ -851,14 +859,14 @@ FieldTransform Serializer::DecodeFieldTransform(
       std::vector<FieldValue> elements =
           DecodeArray(reader, proto.remove_all_from_array);
       return FieldTransform(
-          DecodeFieldPath(proto.field_path),
+          std::move(field),
           ArrayTransform(TransformOperation::Type::ArrayRemove,
                          std::move(elements)));
     }
 
     case google_firestore_v1_DocumentTransform_FieldTransform_increment_tag: {
       FieldValue operand = DecodeFieldValue(reader, proto.increment);
-      return FieldTransform(DecodeFieldPath(proto.field_path),
+      return FieldTransform(std::move(field),
                             NumericIncrementTransform(std::move(operand)));
     }
   }
@@ -1145,8 +1153,7 @@ google_firestore_v1_StructuredQuery_Filter Serializer::EncodeSingularFilter(
 Filter Serializer::DecodeFieldFilter(
     nanopb::Reader* reader,
     const google_firestore_v1_StructuredQuery_FieldFilter& field_filter) const {
-  FieldPath field_path =
-      FieldPath::FromServerFormat(DecodeString(field_filter.field.field_path));
+  FieldPath field_path = DecodeFieldPath(reader, field_filter.field.field_path);
   Filter::Operator op = DecodeFieldFilterOperator(reader, field_filter.op);
   FieldValue value = DecodeFieldValue(reader, field_filter.value);
 
@@ -1161,8 +1168,7 @@ Filter Serializer::DecodeUnaryFilter(
               "Unexpected UnaryFilter.which_operand_type: %s",
               unary.which_operand_type);
 
-  auto field =
-      FieldPath::FromServerFormat(DecodeString(unary.field.field_path));
+  FieldPath field = DecodeFieldPath(reader, unary.field.field_path);
 
   switch (unary.op) {
     case google_firestore_v1_StructuredQuery_UnaryFilter_Operator_IS_NULL:
@@ -1344,8 +1350,7 @@ OrderByList Serializer::DecodeOrderBys(
 OrderBy Serializer::DecodeOrderBy(
     nanopb::Reader* reader,
     const google_firestore_v1_StructuredQuery_Order& order_by) const {
-  auto field_path =
-      FieldPath::FromServerFormat(DecodeString(order_by.field.field_path));
+  auto field_path = DecodeFieldPath(reader, order_by.field.field_path);
 
   Direction direction;
   switch (order_by.direction) {
@@ -1403,9 +1408,15 @@ pb_bytes_array_t* Serializer::EncodeFieldPath(const FieldPath& field_path) {
 }
 
 /* static */
-FieldPath Serializer::DecodeFieldPath(const pb_bytes_array_t* field_path) {
+FieldPath Serializer::DecodeFieldPath(nanopb::Reader* reader,
+                                      const pb_bytes_array_t* field_path) {
   absl::string_view str = MakeStringView(field_path);
-  return FieldPath::FromServerFormatView(str);
+  StatusOr<FieldPath> decoded_path = FieldPath::FromServerFormatView(str);
+  if (!decoded_path.ok()) {
+    reader->set_status(decoded_path.status());
+    return InvalidFieldPath();
+  }
+  return decoded_path.ConsumeValueOrDie();
 }
 
 google_protobuf_Timestamp Serializer::EncodeVersion(

Firestore/core/src/remote/serializer.h

@@ -172,7 +172,7 @@ class Serializer {
   static google_firestore_v1_DocumentMask EncodeFieldMask(
       const model::FieldMask& mask);
   static model::FieldMask DecodeFieldMask(
-      const google_firestore_v1_DocumentMask& mask);
+      nanopb::Reader* reader, const google_firestore_v1_DocumentMask& mask);
 
   google_firestore_v1_DocumentTransform_FieldTransform EncodeFieldTransform(
       const model::FieldTransform& field_transform) const;
@@ -189,7 +189,8 @@ class Serializer {
   EncodeListenRequestLabels(const local::TargetData& target_data) const;
 
   static pb_bytes_array_t* EncodeFieldPath(const model::FieldPath& field_path);
-  static model::FieldPath DecodeFieldPath(const pb_bytes_array_t* field_path);
+  static model::FieldPath DecodeFieldPath(nanopb::Reader* reader,
+                                          const pb_bytes_array_t* field_path);
 
   static google_protobuf_Timestamp EncodeVersion(
       const model::SnapshotVersion& version);

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

@@ -352,6 +352,34 @@ TEST_F(LocalSerializerTest, EncodesTargetData) {
   ExpectRoundTrip(target_data, expected);
 }
 
+TEST_F(LocalSerializerTest, HandlesInvalidTargetData) {
+  TargetId target_id = 42;
+  std::string invalid_field_path = "`";
+
+  ::firestore::client::Target invalid_target;
+  invalid_target.set_target_id(target_id);
+  v1::Target::QueryTarget* query_proto = invalid_target.mutable_query();
+
+  // Add expected collection.
+  query_proto->set_parent("projects/p/databases/d/documents");
+  v1::StructuredQuery::CollectionSelector from;
+  from.set_collection_id("room");
+  *query_proto->mutable_structured_query()->add_from() = std::move(from);
+
+  // Add invalid order_by.
+  v1::StructuredQuery::Order order;
+  order.mutable_field()->set_field_path(invalid_field_path);
+  order.set_direction(v1::StructuredQuery::ASCENDING);
+  *query_proto->mutable_structured_query()->add_order_by() = std::move(order);
+
+  ByteString bytes = ProtobufSerialize(invalid_target);
+  StringReader reader(bytes);
+
+  auto message = Message<firestore_client_Target>::TryParse(&reader);
+  serializer.DecodeTargetData(&reader, *message);
+  EXPECT_NOT_OK(reader.status());
+}
+
 TEST_F(LocalSerializerTest, EncodesTargetDataWithDocumentQuery) {
   core::Query query = Query("room/1");
   TargetId target_id = 42;

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

@@ -26,13 +26,14 @@ namespace firestore {
 namespace model {
 
 TEST(FieldMask, ConstructorAndEqual) {
-  FieldMask mask_a{FieldPath::FromServerFormat("foo"),
-                   FieldPath::FromServerFormat("bar")};
-  std::set<FieldPath> field_path_set{FieldPath::FromServerFormat("foo"),
-                                     FieldPath::FromServerFormat("bar")};
+  FieldMask mask_a{FieldPath::FromDotSeparatedString("foo"),
+                   FieldPath::FromDotSeparatedString("bar")};
+  std::set<FieldPath> field_path_set{FieldPath::FromDotSeparatedString("foo"),
+                                     FieldPath::FromDotSeparatedString("bar")};
   FieldMask mask_b{field_path_set};
-  FieldMask mask_c{std::set<FieldPath>{FieldPath::FromServerFormat("foo"),
-                                       FieldPath::FromServerFormat("bar")}};
+  FieldMask mask_c{
+      std::set<FieldPath>{FieldPath::FromDotSeparatedString("foo"),
+                          FieldPath::FromDotSeparatedString("bar")}};
   FieldMask mask_d{field_path_set.begin(), field_path_set.end()};
 
   EXPECT_EQ(mask_a, mask_b);
@@ -41,16 +42,16 @@ TEST(FieldMask, ConstructorAndEqual) {
 }
 
 TEST(FieldMask, Getter) {
-  FieldMask mask{FieldPath::FromServerFormat("foo"),
-                 FieldPath::FromServerFormat("bar")};
-  EXPECT_EQ(std::set<FieldPath>({FieldPath::FromServerFormat("foo"),
-                                 FieldPath::FromServerFormat("bar")}),
+  FieldMask mask{FieldPath::FromDotSeparatedString("foo"),
+                 FieldPath::FromDotSeparatedString("bar")};
+  EXPECT_EQ(std::set<FieldPath>({FieldPath::FromDotSeparatedString("foo"),
+                                 FieldPath::FromDotSeparatedString("bar")}),
             std::set<FieldPath>(mask.begin(), mask.end()));
 }
 
 TEST(FieldMask, ToString) {
-  FieldMask mask{FieldPath::FromServerFormat("foo"),
-                 FieldPath::FromServerFormat("bar")};
+  FieldMask mask{FieldPath::FromDotSeparatedString("foo"),
+                 FieldPath::FromDotSeparatedString("bar")};
   EXPECT_EQ("{ bar foo }", mask.ToString());
 }