Address feedback

Author: cherylEnkidu

Firestore/Example/Tests/Integration/API/FIRIndexingTests.mm

@@ -74,7 +74,7 @@ - (void)testBadJsonDoesNotCrashClient {
                               completion:^(NSError* error) {
                                 XCTAssertNotNil(error);
                                 XCTAssertEqualObjects(error.domain, FIRFirestoreErrorDomain);
-                                XCTAssertEqual(error.code, FIRFirestoreErrorCodeDataLoss);
+                                XCTAssertEqual(error.code, FIRFirestoreErrorCodeInvalidArgument);
                               }];
 }
 
@@ -95,7 +95,7 @@ - (void)testBadIndexDoesNotCrashClient {
                               completion:^(NSError* error) {
                                 XCTAssertNotNil(error);
                                 XCTAssertEqualObjects(error.domain, FIRFirestoreErrorDomain);
-                                XCTAssertEqual(error.code, FIRFirestoreErrorCodeDataLoss);
+                                XCTAssertEqual(error.code, FIRFirestoreErrorCodeInvalidArgument);
                               }];
 }
 

Firestore/Source/Public/FirebaseFirestore/FIRFirestore.h

@@ -72,22 +72,20 @@ NS_SWIFT_NAME(Firestore)
  */
 @property(strong, nonatomic, readonly) FIRApp *app;
 
-// This method is in preview. API signature and functionality are subject to change.
 #pragma mark - Configure FieldIndexes
 
 /**
+ * This method is in preview. API signature and functionality are subject to change.
+ *
  * Configures indexing for local query execution. Any previous index configuration is overridden.
  *
  * The index entries themselves are created asynchronously. You can continue to use queries
  * that require indexing even if the indices are not yet available. Query execution will
  * automatically start using the index once the index entries have been written.
  *
- * Indexes are only supported with LevelDB persistence. Invoke `set_persistence_enabled(true)`
- * before setting an index configuration. If LevelDB is not enabled, any index configuration
- * will be rejected.
- *
  * The method accepts the JSON format exported by the Firebase CLI (`firebase
- * firestore:indexes`). If the JSON format is invalid, this method ignores the changes.
+ * firestore:indexes`). If the JSON format is invalid, the completion block will be
+ * invoked with a NSError.
  *
  * @param json The JSON format exported by the Firebase CLI.
  * @param completion A block to execute when setting is in a final state. The `error` parameter
@@ -98,6 +96,8 @@ NS_SWIFT_NAME(Firestore)
     NS_SWIFT_NAME(setIndexConfiguration(_:completion:));
 
 /**
+ * This method is in preview. API signature and functionality are subject to change.
+ *
  * Configures indexing for local query execution. Any previous index configuration is overridden.
  *
  * The index entries themselves are created asynchronously. You can continue to use queries

Firestore/core/src/api/firestore.cc

@@ -40,6 +40,8 @@
 #include "Firestore/core/src/util/exception.h"
 #include "Firestore/core/src/util/executor.h"
 #include "Firestore/core/src/util/hard_assert.h"
+#include "Firestore/core/src/util/json_reader.h"
+#include "Firestore/core/src/util/log.h"
 #include "Firestore/core/src/util/status.h"
 #include "Firestore/third_party/nlohmann_json/json.hpp"
 #include "absl/memory/memory.h"
@@ -251,32 +253,33 @@ DatabaseInfo Firestore::MakeDatabaseInfo() const {
                       settings_.ssl_enabled());
 }
 
-void Firestore::SetIndexConfiguration(std::string config,
-                                      util::StatusCallback callback) {
+void Firestore::SetIndexConfiguration(const std::string& config,
+                                      const util::StatusCallback& callback) {
   EnsureClientConfigured();
 
-  bundle::JsonReader reader;
+  util::JsonReader reader;
   if (!settings_.persistence_enabled()) {
-    callback(Status(Error::kErrorFailedPrecondition,
-                    "Cannot enable indexes when persistence is disabled."));
+    LOG_DEBUG("Cannot enable indexes when persistence is disabled.");
+    callback(util::Status::OK());
     return;
   }
 
   auto json_object =
       nlohmann::json::parse(config.begin(), config.end(),
                             /*callback=*/nullptr, /*allow_exceptions=*/false);
   if (json_object.is_discarded()) {
-    callback(Status(Error::kErrorDataLoss, "Invalid Json format."));
+    callback(Status(Error::kErrorInvalidArgument, "Invalid Json format."));
     return;
   }
 
   std::vector<FieldIndex> parsed_indexes;
-  const auto& json_indexes = reader.RequiredArray("indexes", json_object);
+  const std::vector<json> default_vector;
+  const auto& json_indexes =
+      reader.OptionalArray("indexes", json_object, default_vector);
   for (const auto& json_index : json_indexes) {
     const std::string& collection_group =
         reader.RequiredString("collectionGroup", json_index);
     std::vector<Segment> segments;
-    std::vector<json> default_vector;
     const auto& json_fields =
         reader.OptionalArray("fields", json_index, default_vector);
     for (const auto& json_field : json_fields) {
@@ -297,16 +300,17 @@ void Firestore::SetIndexConfiguration(std::string config,
             Segment(std::move(field_path), Segment::Kind::kDescending));
       }
     }
+
+    if (reader.status() != util::Status::OK()) {
+      callback(reader.status());
+      return;
+    }
+
     parsed_indexes.emplace_back(
         FieldIndex(FieldIndex::UnknownId(), collection_group,
                    std::move(segments), FieldIndex::InitialState()));
   }
 
-  if (reader.status() != util::Status::OK()) {
-    callback(reader.status());
-    return;
-  }
-
   client_->ConfigureFieldIndexes(std::move(parsed_indexes));
 
   callback(util::Status::OK());

Firestore/core/src/api/firestore.h

@@ -107,7 +107,8 @@ class Firestore : public std::enable_shared_from_this<Firestore> {
   void EnableNetwork(util::StatusCallback callback);
   void DisableNetwork(util::StatusCallback callback);
 
-  void SetIndexConfiguration(std::string config, util::StatusCallback callback);
+  void SetIndexConfiguration(const std::string& config,
+                             const util::StatusCallback& callback);
 
   std::shared_ptr<api::LoadBundleTask> LoadBundle(
       std::unique_ptr<util::ByteStream> bundle_data);

Firestore/core/src/bundle/bundle_reader.h

@@ -24,6 +24,7 @@
 #include "Firestore/core/src/bundle/bundle_metadata.h"
 #include "Firestore/core/src/bundle/bundle_serializer.h"
 #include "Firestore/core/src/util/byte_stream.h"
+#include "Firestore/core/src/util/json_reader.h"
 #include "absl/types/optional.h"
 
 namespace firebase {
@@ -111,7 +112,7 @@ class BundleReader {
   std::unique_ptr<BundleElement> DecodeBundleElementFromBuffer();
 
   BundleSerializer serializer_;
-  JsonReader json_reader_;
+  util::JsonReader json_reader_;
 
   // Input stream holding bundle data.
   std::unique_ptr<util::ByteStream> input_;

Firestore/core/src/bundle/bundle_serializer.cc

@@ -45,7 +45,6 @@
 namespace firebase {
 namespace firestore {
 namespace bundle {
-namespace {
 
 using absl::Time;
 using core::Bound;
@@ -73,21 +72,16 @@ using nanopb::Message;
 using nanopb::SetRepeatedField;
 using nanopb::SharedMessage;
 using nlohmann::json;
+using util::JsonReader;
 using util::NoDestructor;
 using util::StatusOr;
 using util::StringFormat;
 using Operator = FieldFilter::Operator;
 
 namespace {
+
 const NoDestructor<Bound> kDefaultBound{Bound::FromValue(
     MakeSharedMessage<google_firestore_v1_ArrayValue>({}), false)};
-}  // namespace
-
-template <typename T>
-const std::vector<T>& EmptyVector() {
-  static NoDestructor<std::vector<T>> empty;
-  return *empty;
-}
 
 Timestamp DecodeTimestamp(JsonReader& reader, const json& version) {
   StatusOr<Timestamp> decoded;
@@ -326,180 +320,6 @@ pb_bytes_array_t* DecodeBytesValue(JsonReader& reader,
 
 }  // namespace
 
-// Mark: JsonReader
-
-const std::string& JsonReader::RequiredString(const char* name,
-                                              const json& json_object) {
-  if (json_object.contains(name)) {
-    const json& child = json_object.at(name);
-    if (child.is_string()) {
-      return child.get_ref<const std::string&>();
-    }
-  }
-
-  Fail("'%s' is missing or is not a string", name);
-  return util::EmptyString();
-}
-
-const std::string& JsonReader::OptionalString(
-    const char* name,
-    const json& json_object,
-    const std::string& default_value) {
-  if (json_object.contains(name)) {
-    const json& child = json_object.at(name);
-    if (child.is_string()) {
-      return child.get_ref<const std::string&>();
-    }
-  }
-
-  return default_value;
-}
-
-const std::vector<json>& JsonReader::RequiredArray(const char* name,
-                                                   const json& json_object) {
-  if (json_object.contains(name)) {
-    const json& child = json_object.at(name);
-    if (child.is_array()) {
-      return child.get_ref<const std::vector<json>&>();
-    }
-  }
-
-  Fail("'%s' is missing or is not an array", name);
-  return EmptyVector<json>();
-}
-
-const std::vector<json>& JsonReader::OptionalArray(
-    const char* name,
-    const json& json_object,
-    const std::vector<json>& default_value) {
-  if (!json_object.contains(name)) {
-    return default_value;
-  }
-
-  const json& child = json_object.at(name);
-  if (child.is_array()) {
-    return child.get_ref<const std::vector<json>&>();
-  } else {
-    Fail("'%s' is not an array", name);
-    return EmptyVector<json>();
-  }
-}
-
-bool JsonReader::OptionalBool(const char* name,
-                              const json& json_object,
-                              bool default_value) {
-  return (json_object.contains(name) && json_object.at(name).is_boolean() &&
-          json_object.at(name).get<bool>()) ||
-         default_value;
-}
-
-const nlohmann::json& JsonReader::RequiredObject(const char* child_name,
-                                                 const json& json_object) {
-  if (!json_object.contains(child_name)) {
-    Fail("Missing child '%s'", child_name);
-    return json_object;
-  }
-  return json_object.at(child_name);
-}
-
-const nlohmann::json& JsonReader::OptionalObject(
-    const char* child_name,
-    const json& json_object,
-    const nlohmann::json& default_value) {
-  if (json_object.contains(child_name)) {
-    return json_object.at(child_name);
-  }
-  return default_value;
-}
-
-double JsonReader::RequiredDouble(const char* name, const json& json_object) {
-  if (json_object.contains(name)) {
-    double result = DecodeDouble(json_object.at(name));
-    if (ok()) {
-      return result;
-    }
-  }
-
-  Fail("'%s' is missing or is not a double", name);
-  return 0.0;
-}
-
-double JsonReader::OptionalDouble(const char* name,
-                                  const json& json_object,
-                                  double default_value) {
-  if (json_object.contains(name)) {
-    double result = DecodeDouble(json_object.at(name));
-    if (ok()) {
-      return result;
-    }
-  }
-
-  return default_value;
-}
-
-double JsonReader::DecodeDouble(const nlohmann::json& value) {
-  if (value.is_number()) {
-    return value.get<double>();
-  }
-
-  double result = 0;
-  if (value.is_string()) {
-    const auto& s = value.get_ref<const std::string&>();
-    auto ok = absl::SimpleAtod(s, &result);
-    if (!ok) {
-      Fail("Failed to parse into double: " + s);
-    }
-  }
-  return result;
-}
-
-template <typename IntType>
-IntType ParseInt(const json& value, JsonReader& reader) {
-  if (value.is_number_integer()) {
-    return value.get<IntType>();
-  }
-
-  IntType result = 0;
-  if (value.is_string()) {
-    const auto& s = value.get_ref<const std::string&>();
-    auto ok = absl::SimpleAtoi<IntType>(s, &result);
-    if (!ok) {
-      reader.Fail("Failed to parse into integer: " + s);
-      return 0;
-    }
-
-    return result;
-  }
-
-  reader.Fail("Only integer and string can be parsed into int type");
-  return 0;
-}
-
-template <typename IntType>
-IntType JsonReader::RequiredInt(const char* name, const json& json_object) {
-  if (!json_object.contains(name)) {
-    Fail("'%s' is missing or is not a double", name);
-    return 0;
-  }
-
-  const json& value = json_object.at(name);
-  return ParseInt<IntType>(value, *this);
-}
-
-template <typename IntType>
-IntType JsonReader::OptionalInt(const char* name,
-                                const json& json_object,
-                                IntType default_value) {
-  if (!json_object.contains(name)) {
-    return default_value;
-  }
-
-  const json& value = json_object.at(name);
-  return ParseInt<IntType>(value, *this);
-}
-
-// Mark: BundleSerializer
-
 BundleMetadata BundleSerializer::DecodeBundleMetadata(
     JsonReader& reader, const json& metadata) const {
   return BundleMetadata(