Merge pull request #324 from firebase/g3merge10

Firestore: Expand validation_test to cover other platforms

Author: alexames

firestore/src/android/field_path_portable.cc

@@ -90,6 +90,22 @@ std::vector<std::string> SplitOnDots(const std::string& input) {
   return result;
 }
 
+void ValidateSegments(const std::vector<std::string>& segments) {
+  if (segments.empty()) {
+    SimpleThrowInvalidArgument(
+        "Invalid field path. Provided names must not be empty.");
+  }
+
+  for (size_t i = 0; i < segments.size(); ++i) {
+    if (segments[i].empty()) {
+      std::ostringstream message;
+      message << "Invalid field name at index " << i
+              << ". Field names must not be empty.";
+      SimpleThrowInvalidArgument(message.str());
+    }
+  }
+}
+
 }  // anonymous namespace
 
 std::string FieldPathPortable::CanonicalString() const {
@@ -118,6 +134,12 @@ bool FieldPathPortable::IsKeyFieldPath() const {
   return size() == 1 && segments_[0] == FieldPathPortable::kDocumentKeyPath;
 }
 
+FieldPathPortable FieldPathPortable::FromSegments(
+    std::vector<std::string> segments) {
+  ValidateSegments(segments);
+  return FieldPathPortable(Move(segments));
+}
+
 FieldPathPortable FieldPathPortable::FromDotSeparatedString(
     const std::string& path) {
   if (path.find_first_of("~*/[]") != std::string::npos) {

firestore/src/android/field_path_portable.h

@@ -65,8 +65,16 @@ class FieldPathPortable {
     return segments_ >= rhs.segments_;
   }
 
-  // Creates and returns a new path from a dot-separated field-path string,
-  // where path segments are separated by a dot ".".
+  /**
+   * Creates and returns a new path from an explicitly pre-split list of
+   * segments.
+   */
+  static FieldPathPortable FromSegments(std::vector<std::string> segments);
+
+  /**
+   * Creates and returns a new path from a dot-separated field-path string,
+   * where path segments are separated by a dot ".".
+   */
   static FieldPathPortable FromDotSeparatedString(const std::string& path);
 
   static FieldPathPortable KeyFieldPath();

firestore/src/android/firestore_android.cc

@@ -61,6 +61,8 @@ namespace {
 
 using jni::Constructor;
 using jni::Env;
+using jni::Global;
+using jni::HashMap;
 using jni::Loader;
 using jni::Local;
 using jni::Long;
@@ -170,15 +172,15 @@ class JavaFirestoreMap {
  private:
   // Ensures that the backing map is initialized.
   // Prerequisite: `mutex_` must be locked before calling this method.
-  jni::HashMap& GetMapLocked(Env& env) {
+  HashMap& GetMapLocked(Env& env) {
     if (!firestores_) {
-      firestores_ = jni::HashMap::Create(env);
+      firestores_ = HashMap::Create(env);
     }
     return firestores_;
   }
 
   Mutex mutex_;
-  jni::Global<jni::HashMap> firestores_;
+  Global<HashMap> firestores_;
 };
 
 // init_mutex protects all the globals below.
@@ -496,34 +498,34 @@ void FirestoreInternal::ClearListeners() {
   listener_registrations_.clear();
 }
 
-jni::Env FirestoreInternal::GetEnv() {
-  jni::Env env;
+Env FirestoreInternal::GetEnv() {
+  Env env;
   env.SetUnhandledExceptionHandler(GlobalUnhandledExceptionHandler, nullptr);
   return env;
 }
 
 CollectionReference FirestoreInternal::NewCollectionReference(
-    jni::Env& env, const jni::Object& reference) const {
+    Env& env, const jni::Object& reference) const {
   return MakePublic<CollectionReference>(env, mutable_this(), reference);
 }
 
 DocumentReference FirestoreInternal::NewDocumentReference(
-    jni::Env& env, const jni::Object& reference) const {
+    Env& env, const jni::Object& reference) const {
   return MakePublic<DocumentReference>(env, mutable_this(), reference);
 }
 
 DocumentSnapshot FirestoreInternal::NewDocumentSnapshot(
-    jni::Env& env, const jni::Object& snapshot) const {
+    Env& env, const jni::Object& snapshot) const {
   return MakePublic<DocumentSnapshot>(env, mutable_this(), snapshot);
 }
 
-Query FirestoreInternal::NewQuery(jni::Env& env,
+Query FirestoreInternal::NewQuery(Env& env,
                                   const jni::Object& query) const {
   return MakePublic<Query>(env, mutable_this(), query);
 }
 
 QuerySnapshot FirestoreInternal::NewQuerySnapshot(
-    jni::Env& env, const jni::Object& snapshot) const {
+    Env& env, const jni::Object& snapshot) const {
   return MakePublic<QuerySnapshot>(env, mutable_this(), snapshot);
 }
 

firestore/src/android/transaction_android.cc

@@ -1,5 +1,6 @@
 #include "firestore/src/android/transaction_android.h"
 
+#include <stdexcept>
 #include <utility>
 
 #include "app/meta/move.h"
@@ -10,9 +11,11 @@
 #include "firestore/src/android/field_value_android.h"
 #include "firestore/src/android/set_options_android.h"
 #include "firestore/src/android/util_android.h"
+#include "firestore/src/common/macros.h"
 #include "firestore/src/jni/env.h"
 #include "firestore/src/jni/hash_map.h"
 #include "firestore/src/jni/loader.h"
+#include "Firestore/core/src/util/firestore_exceptions.h"
 
 namespace firebase {
 namespace firestore {
@@ -121,9 +124,10 @@ DocumentSnapshot TransactionInternal::Get(const DocumentReference& document,
     }
 
     if (!ExceptionInternal::IsFirestoreException(env, exception)) {
-      // We would only preserve exception if it is not
-      // FirebaseFirestoreException. The user should decide whether to raise the
-      // error or let the transaction succeed.
+      // Only preserve the exception if it is not `FirebaseFirestoreException`.
+      // For Firestore exceptions, the user decides whether to raise the error
+      // or let the transaction succeed through the error code/message the
+      // `TransactionFunction` returns.
       PreserveException(env, Move(exception));
     }
     return DocumentSnapshot{};
@@ -141,9 +145,18 @@ DocumentSnapshot TransactionInternal::Get(const DocumentReference& document,
 }
 
 Env TransactionInternal::GetEnv() {
+#if FIRESTORE_HAVE_EXCEPTIONS
+  // If exceptions are enabled, report Java exceptions by translating them to
+  // their C++ equivalents in the usual way. These will throw out to the
+  // user-supplied TransactionFunction and ultimately out to
+  // `TransactionFunctionNativeApply`, below.
+  return FirestoreInternal::GetEnv();
+
+#else
   Env env;
   env.SetUnhandledExceptionHandler(ExceptionHandler, this);
   return env;
+#endif
 }
 
 void TransactionInternal::ExceptionHandler(Env& env,
@@ -195,7 +208,32 @@ jobject TransactionInternal::TransactionFunctionNativeApply(
       new TransactionInternal(firestore, Object(java_transaction)));
 
   std::string message;
-  Error code = transaction_function->Apply(transaction, message);
+  Error code;
+
+#if FIRESTORE_HAVE_EXCEPTIONS
+  try {
+#endif
+
+    code = transaction_function->Apply(transaction, message);
+
+#if FIRESTORE_HAVE_EXCEPTIONS
+  } catch (const FirestoreException& e) {
+    message = e.what();
+    code = e.code();
+  } catch (const std::invalid_argument& e) {
+    message = e.what();
+    code = Error::kErrorInvalidArgument;
+  } catch (const std::logic_error& e) {
+    message = e.what();
+    code = Error::kErrorFailedPrecondition;
+  } catch (const std::exception& e) {
+    message = std::string("Unknown exception: ") + e.what();
+    code = Error::kErrorUnknown;
+  } catch (...) {
+    message = "Unknown exception";
+    code = Error::kErrorUnknown;
+  }
+#endif  // FIRESTORE_HAVE_EXCEPTIONS
 
   // Verify that `internal_` is not null before using it. It could be set to
   // `nullptr` if the `FirestoreInternal` is destroyed during the invocation of

firestore/src/common/field_path.cc

@@ -24,6 +24,8 @@
 #include <unordered_set>
 #endif  // defined(_STLPORT_VERSION)
 
+#include "app/meta/move.h"
+
 #if defined(__ANDROID__) || defined(FIRESTORE_STUB_BUILD)
 #include "firestore/src/android/field_path_portable.h"
 #else
@@ -38,11 +40,11 @@ FieldPath::FieldPath() {}
 
 #if !defined(_STLPORT_VERSION)
 FieldPath::FieldPath(std::initializer_list<std::string> field_names)
-    : internal_(new FieldPathInternal{field_names}) {}
+    : internal_(InternalFromSegments(std::vector<std::string>(field_names))) {}
 #endif  // !defined(_STLPORT_VERSION)
 
 FieldPath::FieldPath(const std::vector<std::string>& field_names)
-    : internal_(new FieldPathInternal{std::vector<std::string>{field_names}}) {}
+    : internal_(InternalFromSegments(field_names)) {}
 
 FieldPath::FieldPath(const FieldPath& path)
     : internal_(new FieldPathInternal{*path.internal_}) {}
@@ -78,6 +80,12 @@ FieldPath FieldPath::DocumentId() {
   return FieldPath{new FieldPathInternal{FieldPathInternal::KeyFieldPath()}};
 }
 
+FieldPath::FieldPathInternal* FieldPath::InternalFromSegments(
+    std::vector<std::string> field_names) {
+  return new FieldPathInternal(
+      FieldPathInternal::FromSegments(Move(field_names)));
+}
+
 /* static */
 FieldPath FieldPath::FromDotSeparatedString(const std::string& path) {
   return FieldPath{

firestore/src/include/firebase/firestore/field_path.h

@@ -170,6 +170,9 @@ class FieldPath final {
 
   explicit FieldPath(FieldPathInternal* internal);
 
+  static FieldPathInternal* InternalFromSegments(
+      std::vector<std::string> field_names);
+
   static FieldPath FromDotSeparatedString(const std::string& path);
 
   FieldPathInternal* internal_ = nullptr;

firestore/src/ios/user_data_converter_ios.cc

@@ -36,6 +36,13 @@ using nanopb::ByteString;
 
 using Type = FieldValue::Type;
 
+FIRESTORE_ATTRIBUTE_NORETURN
+void ThrowInvalidData(const ParseContext& context, const std::string& message) {
+  std::string full_message =
+      "Invalid data. " + message + context.FieldDescription();
+  SimpleThrowInvalidArgument(full_message);
+}
+
 void ParseDelete(ParseContext&& context) {
   if (context.data_source() == UserDataSource::MergeSet) {
     // No transform to add for a delete, but we need to add it to our field mask
@@ -49,32 +56,16 @@ void ParseDelete(ParseContext&& context) {
         !context.path()->empty(),
         "FieldValue.Delete() at the top level should have already been "
         "handled.");
-    // TODO(b/147444199): use string formatting.
-    // ThrowInvalidArgument(
-    //     "FieldValue::Delete() can only appear at the top level of your "
-    //     "update data%s",
-    //     context.FieldDescription());
-    auto message =
-        std::string(
-            "FieldValue::Delete() can only appear at the top level of your "
-            "update data") +
-        context.FieldDescription();
-    SimpleThrowInvalidArgument(message);
+    ThrowInvalidData(context,
+                     "FieldValue::Delete() can only appear at the top level of "
+                     "your update data");
   }
 
   // We shouldn't encounter delete sentinels for queries or non-merge `Set`
   // calls.
-  // TODO(b/147444199): use string formatting.
-  // ThrowInvalidArgument(
-  //     "FieldValue::Delete() can only be used with Update() and Set() with "
-  //     "merge == true%s",
-  //     context.FieldDescription());
-  auto message =
-      std::string(
-          "FieldValue::Delete() can only be used with Update() and Set() with "
-          "merge == true") +
-      context.FieldDescription();
-  SimpleThrowInvalidArgument(message);
+  ThrowInvalidData(context,
+                   "FieldValue::Delete() can only be used with Update() and "
+                   "Set() with merge == true");
 }
 
 void ParseServerTimestamp(ParseContext&& context) {
@@ -143,7 +134,7 @@ FieldMask CreateFieldMask(const ParseAccumulator& accumulator,
       //     path.CanonicalString());
       auto message =
           std::string("Field '") + path.CanonicalString() +
-          "' is specified in your field mask but missing from your input data.";
+          "' is specified in your field mask but not in your input data.";
       SimpleThrowInvalidArgument(message);
     }
 
@@ -276,7 +267,7 @@ model::FieldValue::Array UserDataConverter::ParseArray(
   // disable this validation.
   if (context.array_element() &&
       context.data_source() != core::UserDataSource::ArrayArgument) {
-    SimpleThrowInvalidArgument("Nested arrays are not supported");
+    ThrowInvalidData(context, "Nested arrays are not supported");
   }
 
   model::FieldValue::Array result;
@@ -323,21 +314,21 @@ void UserDataConverter::ParseSentinel(const FieldValue& value,
   // Sentinels are only supported with writes, and not within arrays.
   if (!context.write()) {
     // TODO(b/147444199): use string formatting.
-    // ThrowInvalidArgument("%s can only be used with Update() and Set()%s",
-    //                      Describe(value.type()), context.FieldDescription());
-    auto message = Describe(value.type()) +
-                   " can only be used with Update() and Set()" +
-                   context.FieldDescription();
-    SimpleThrowInvalidArgument(message);
+    // ThrowInvalidData(
+    //     context, "%s can only be used with Update() and Set()%s",
+    //     Describe(value.type()), context.FieldDescription());
+    auto message =
+        Describe(value.type()) + " can only be used with Update() and Set()";
+    ThrowInvalidData(context, message);
   }
 
   if (!context.path()) {
     // TODO(b/147444199): use string formatting.
-    // ThrowInvalidArgument("%s is not currently supported inside arrays",
-    //                      Describe(value.type()));
+    // ThrowInvalidData(context, "%s is not currently supported inside arrays",
+    //                  Describe(value.type()));
     auto message =
         Describe(value.type()) + " is not currently supported inside arrays";
-    SimpleThrowInvalidArgument(message);
+    ThrowInvalidData(context, message);
   }
 
   switch (value.type()) {
@@ -406,7 +397,8 @@ model::FieldValue UserDataConverter::ParseScalar(const FieldValue& value,
           GetInternal(reference.firestore())->database_id();
       if (other != *database_id_) {
         // TODO(b/147444199): use string formatting.
-        // ThrowInvalidArgument(
+        // ThrowInvalidData(
+        //     context,
         //     "DocumentReference is for database %s/%s but should be for "
         //     "database %s/%s%s",
         //     other.project_id(), other.database_id(),
@@ -415,10 +407,9 @@ model::FieldValue UserDataConverter::ParseScalar(const FieldValue& value,
         auto actual_db = other.project_id() + "/" + other.database_id();
         auto expected_db =
             database_id_->project_id() + "/" + database_id_->database_id();
-        auto message = std::string("DocumentReference is for database ") +
-                       actual_db + " but should be for database " +
-                       expected_db + context.FieldDescription();
-        SimpleThrowInvalidArgument(message);
+        auto message = std::string("Document reference is for database ") +
+                       actual_db + " but should be for database " + expected_db;
+        ThrowInvalidData(context, message);
       }
 
       const model::DocumentKey& key = GetInternal(&reference)->key();