Tunnel underlying NSErrors through C++ Status (#2808)

* Tunnel underlying NSErrors through Status

* Make FIRFirestoreErrorDomain available to CMake builds

The prior state was actually semi-broken. The
firebase_credentials_provider_apple definition wasn't extern "C" so it
was defining the value in addition to whatever FIRFirestore was doing.

FIRFirestore's value isn't available to straight C++ though, so this
change pushes the definition down into error_apple.mm so that it can be
shared by the Objective-C API and MakeNSError.

Pushing MakeNSError up into the API layer isn't practical yet so this
change is a compromise that makes things work

Author: wilhuff

Firestore/Example/Firestore.xcodeproj/project.pbxproj

@@ -259,6 +259,9 @@
 		5492E0BF2021555100B64F25 /* FSTFieldValueTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E0B82021555100B64F25 /* FSTFieldValueTests.mm */; };
 		5492E0C72021557E00B64F25 /* FSTSerializerBetaTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E0C12021557E00B64F25 /* FSTSerializerBetaTests.mm */; };
 		5492E0C92021557E00B64F25 /* FSTRemoteEventTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E0C32021557E00B64F25 /* FSTRemoteEventTests.mm */; };
+		5493A424225F9990006DE7BA /* status_apple_test.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5493A423225F9990006DE7BA /* status_apple_test.mm */; };
+		5493A425225F9990006DE7BA /* status_apple_test.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5493A423225F9990006DE7BA /* status_apple_test.mm */; };
+		5493A426225F9990006DE7BA /* status_apple_test.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5493A423225F9990006DE7BA /* status_apple_test.mm */; };
 		5495EB032040E90200EBA509 /* CodableGeoPointTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 5495EB022040E90200EBA509 /* CodableGeoPointTests.swift */; };
 		54995F6F205B6E12004EFFA0 /* leveldb_key_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 54995F6E205B6E12004EFFA0 /* leveldb_key_test.cc */; };
 		549CCA5020A36DBC00BCEB75 /* sorted_set_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 549CCA4C20A36DBB00BCEB75 /* sorted_set_test.cc */; };
@@ -831,6 +834,7 @@
 		5492E0B82021555100B64F25 /* FSTFieldValueTests.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = FSTFieldValueTests.mm; sourceTree = "<group>"; };
 		5492E0C12021557E00B64F25 /* FSTSerializerBetaTests.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = FSTSerializerBetaTests.mm; sourceTree = "<group>"; };
 		5492E0C32021557E00B64F25 /* FSTRemoteEventTests.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = FSTRemoteEventTests.mm; sourceTree = "<group>"; };
+		5493A423225F9990006DE7BA /* status_apple_test.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = status_apple_test.mm; sourceTree = "<group>"; };
 		5495EB022040E90200EBA509 /* CodableGeoPointTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = CodableGeoPointTests.swift; sourceTree = "<group>"; };
 		54995F6E205B6E12004EFFA0 /* leveldb_key_test.cc */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = leveldb_key_test.cc; sourceTree = "<group>"; };
 		549CCA4C20A36DBB00BCEB75 /* sorted_set_test.cc */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = sorted_set_test.cc; sourceTree = "<group>"; };
@@ -1251,6 +1255,7 @@
 				AB380D03201BC6E400D97691 /* ordered_code_test.cc */,
 				403DBF6EFB541DFD01582AA3 /* path_test.cc */,
 				54740A531FC913E500713A1A /* secure_random_test.cc */,
+				5493A423225F9990006DE7BA /* status_apple_test.mm */,
 				54A0352C20A3B3D7003E0143 /* status_test.cc */,
 				54A0352B20A3B3D7003E0143 /* status_test_util.h */,
 				54A0352D20A3B3D7003E0143 /* statusor_test.cc */,
@@ -3051,6 +3056,7 @@
 				862B1AC9EDAB309BBF4FB18C /* sorted_map_test.cc in Sources */,
 				4A62B708A6532DD45414DA3A /* sorted_set_test.cc in Sources */,
 				C9F96C511F45851D38EC449C /* status.pb.cc in Sources */,
+				5493A425225F9990006DE7BA /* status_apple_test.mm in Sources */,
 				4DC660A62BC2B6369DA5C563 /* status_test.cc in Sources */,
 				74985DE2C7EF4150D7A455FD /* statusor_test.cc in Sources */,
 				C5C01A1FB216DA4BA8BF1A02 /* stream_test.mm in Sources */,
@@ -3218,6 +3224,7 @@
 				86E6FC2B7657C35B342E1436 /* sorted_map_test.cc in Sources */,
 				8413BD9958F6DD52C466D70F /* sorted_set_test.cc in Sources */,
 				0D2D25522A94AA8195907870 /* status.pb.cc in Sources */,
+				5493A426225F9990006DE7BA /* status_apple_test.mm in Sources */,
 				C0AD8DB5A84CAAEE36230899 /* status_test.cc in Sources */,
 				DC48407370E87F2233D7AB7E /* statusor_test.cc in Sources */,
 				215643858470A449D3A3E168 /* stream_test.mm in Sources */,
@@ -3460,6 +3467,7 @@
 				549CCA5220A36DBC00BCEB75 /* sorted_map_test.cc in Sources */,
 				549CCA5020A36DBC00BCEB75 /* sorted_set_test.cc in Sources */,
 				618BBEB120B89AAC00B5BCE7 /* status.pb.cc in Sources */,
+				5493A424225F9990006DE7BA /* status_apple_test.mm in Sources */,
 				54A0352F20A3B3D8003E0143 /* status_test.cc in Sources */,
 				54A0353020A3B3D8003E0143 /* statusor_test.cc in Sources */,
 				B66D8996213609EE0086DA0C /* stream_test.mm in Sources */,

Firestore/Source/API/FIRFirestore.mm

@@ -55,8 +55,6 @@
 
 NS_ASSUME_NONNULL_BEGIN
 
-extern "C" NSString *const FIRFirestoreErrorDomain = @"FIRFirestoreErrorDomain";
-
 #pragma mark - FIRFirestore
 
 @interface FIRFirestore ()

Firestore/core/src/firebase/firestore/auth/firebase_credentials_provider_apple.mm

@@ -22,13 +22,10 @@
 #import <FirebaseCore/FIRComponentContainer.h>
 #import <FirebaseCore/FIROptionsInternal.h>
 
+#include "Firestore/core/src/firebase/firestore/util/error_apple.h"
 #include "Firestore/core/src/firebase/firestore/util/hard_assert.h"
 #include "Firestore/core/src/firebase/firestore/util/string_apple.h"
 
-// NB: This is also defined in Firestore/Source/Public/FIRFirestoreErrors.h
-// NOLINTNEXTLINE: public constant
-NSString* const FIRFirestoreErrorDomain = @"FIRFirestoreErrorDomain";
-
 namespace firebase {
 namespace firestore {
 namespace auth {

Firestore/core/src/firebase/firestore/util/CMakeLists.txt

@@ -246,6 +246,8 @@ cc_library(
     comparison.h
     config.h
     delayed_constructor.h
+    error_apple.h
+    error_apple.mm
     hashing.h
     iterator_adaptors.h
     objc_compatibility.h

Firestore/core/src/firebase/firestore/util/error_apple.h

@@ -22,31 +22,43 @@
 
 #import <Foundation/Foundation.h>
 
-#import <FirebaseFirestore/FIRFirestoreErrors.h>  // for FIRFirestoreErrorDomain
-
 #include "Firestore/core/include/firebase/firestore/firestore_errors.h"
 #include "Firestore/core/src/firebase/firestore/util/status.h"
 #include "Firestore/core/src/firebase/firestore/util/string_apple.h"
 #include "absl/strings/string_view.h"
 
+// The Cloud Firestore error domain. Keep in sync with FIRFirestoreErrors.h.
+// Exposed here to make it possible to build in CMake without bringing in the
+// sources under Firestore/Source.
+FOUNDATION_EXPORT NSString* const FIRFirestoreErrorDomain
+    NS_SWIFT_NAME(FirestoreErrorDomain);
+
 namespace firebase {
 namespace firestore {
 namespace util {
 
 // Translates a set of error_code and error_msg to an NSError.
 inline NSError* MakeNSError(const int64_t error_code,
-                            const absl::string_view error_msg) {
+                            const absl::string_view error_msg,
+                            NSError* cause = nil) {
   if (error_code == FirestoreErrorCode::Ok) {
     return nil;
   }
-  return [NSError
-      errorWithDomain:FIRFirestoreErrorDomain
-                 code:static_cast<NSInteger>(error_code)
-             userInfo:@{NSLocalizedDescriptionKey : WrapNSString(error_msg)}];
+
+  NSMutableDictionary<NSString*, id>* user_info =
+      [NSMutableDictionary dictionary];
+  user_info[NSLocalizedDescriptionKey] = WrapNSString(error_msg);
+  if (cause) {
+    user_info[NSUnderlyingErrorKey] = cause;
+  }
+
+  return [NSError errorWithDomain:FIRFirestoreErrorDomain
+                             code:static_cast<NSInteger>(error_code)
+                         userInfo:user_info];
 }
 
 inline NSError* MakeNSError(const util::Status& status) {
-  return MakeNSError(status.code(), status.error_message());
+  return status.ToNSError();
 }
 
 inline Status MakeStatus(NSError* error) {

Firestore/core/src/firebase/firestore/util/error_apple.mm

@@ -0,0 +1,22 @@
+/*
+ * Copyright 2019 Google
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "Firestore/core/src/firebase/firestore/util/error_apple.h"
+
+// NB: This is also defined in Firestore/Source/Public/FIRFirestoreErrors.h
+// NOLINTNEXTLINE: public constant
+FOUNDATION_EXPORT NSString* const FIRFirestoreErrorDomain =
+    @"FIRFirestoreErrorDomain";

Firestore/core/src/firebase/firestore/util/status.cc

@@ -17,6 +17,7 @@
 #include "Firestore/core/src/firebase/firestore/util/status.h"
 
 #include <ostream>
+#include <utility>
 
 #include "Firestore/core/src/firebase/firestore/util/string_format.h"
 #include "absl/memory/memory.h"
@@ -49,6 +50,22 @@ Status& Status::CausedBy(const Status& cause) {
   }
 
   absl::StrAppend(&state_->msg, ": ", cause.error_message());
+
+  // If this Status has no accompanying PlatformError but the cause does, create
+  // an PlatformError for this Status ahead of time to preserve the causal chain
+  // that Status doesn't otherwise support.
+  if (state_->platform_error == nullptr &&
+      cause.state_->platform_error != nullptr) {
+    state_->platform_error =
+        cause.state_->platform_error->WrapWith(code(), error_message());
+  }
+
+  return *this;
+}
+
+Status& Status::WithPlatformError(std::unique_ptr<PlatformError> error) {
+  HARD_ASSERT(!ok(), "Platform errors should not be applied to Status::OK()");
+  state_->platform_error = std::move(error);
   return *this;
 }
 

Firestore/core/src/firebase/firestore/util/status.h

@@ -35,6 +35,8 @@ namespace firebase {
 namespace firestore {
 namespace util {
 
+class PlatformError;
+
 /// Denotes success or failure of a call.
 class ABSL_MUST_USE_RESULT Status {
  public:
@@ -63,6 +65,8 @@ class ABSL_MUST_USE_RESULT Status {
 
 #if defined(__OBJC__)
   static Status FromNSError(NSError* error);
+
+  NSError* ToNSError() const;
 #endif  // defined(__OBJC__)
 
   /// Returns true iff the status indicates success.
@@ -97,6 +101,8 @@ class ABSL_MUST_USE_RESULT Status {
   /// \return *this
   Status& CausedBy(const Status& cause);
 
+  Status& WithPlatformError(std::unique_ptr<PlatformError> error);
+
   /// \brief Return a string representation of this status suitable for
   /// printing. Returns the string `"OK"` for success.
   std::string ToString() const;
@@ -110,20 +116,51 @@ class ABSL_MUST_USE_RESULT Status {
  private:
   static const std::string& empty_string();
   struct State {
+    State() = default;
+    State(const State& other);
+
     FirestoreErrorCode code;
     std::string msg;
+
+    // An additional platform-specific error representation that was used to
+    // generate this Status. The PlatformError does not meaningfully contribute
+    // to the identity of this Status: it exists to allow tunneling e.g.
+    // NSError* to Status and back to NSError* losslessly.
+    std::unique_ptr<PlatformError> platform_error;
   };
-  // OK status has a `NULL` state_.  Otherwise, `state_` points to
+  // OK status has a `nullptr` state_.  Otherwise, `state_` points to
   // a `State` structure containing the error code and message(s)
   std::unique_ptr<State> state_;
 
   void SlowCopyFrom(const State* src);
 };
 
+class PlatformError {
+ public:
+  virtual ~PlatformError() {
+  }
+
+  virtual std::unique_ptr<PlatformError> Copy() = 0;
+
+  /**
+   * Creates a new PlatformError with the given code and message, whose cause is
+   * this PlatformError.
+   */
+  virtual std::unique_ptr<PlatformError> WrapWith(FirestoreErrorCode code,
+                                                  std::string message) = 0;
+};
+
 inline Status::Status(const Status& s)
     : state_((s.state_ == nullptr) ? nullptr : new State(*s.state_)) {
 }
 
+inline Status::State::State(const State& s)
+    : code(s.code),
+      msg(s.msg),
+      platform_error((s.platform_error == nullptr) ? nullptr
+                                                   : s.platform_error->Copy()) {
+}
+
 inline void Status::operator=(const Status& s) {
   // The following condition catches both aliasing (when this == &s),
   // and the common case where both s and *this are ok.

Firestore/core/src/firebase/firestore/util/status_apple.mm

@@ -18,30 +18,79 @@
 
 #if defined(__APPLE__)
 
+#include "Firestore/core/src/firebase/firestore/util/error_apple.h"
 #include "Firestore/core/src/firebase/firestore/util/string_format.h"
+#include "absl/memory/memory.h"
 
 namespace firebase {
 namespace firestore {
 namespace util {
 
+class UnderlyingNSError : public PlatformError {
+ public:
+  explicit UnderlyingNSError(NSError* error) : error_(error) {
+  }
+
+  static std::unique_ptr<UnderlyingNSError> Create(NSError* error) {
+    return absl::make_unique<UnderlyingNSError>(error);
+  }
+
+  static NSError* Recover(
+      const std::unique_ptr<PlatformError>& platform_error) {
+    if (platform_error == nullptr) {
+      return nil;
+    }
+
+    return static_cast<UnderlyingNSError*>(platform_error.get())->error();
+  }
+
+  std::unique_ptr<PlatformError> Copy() override {
+    return absl::make_unique<UnderlyingNSError>(error_);
+  }
+
+  std::unique_ptr<PlatformError> WrapWith(FirestoreErrorCode code,
+                                          std::string message) override {
+    NSError* chain = MakeNSError(code, message, error_);
+    return Create(chain);
+  }
+
+  NSError* error() const {
+    return error_;
+  }
+
+ private:
+  NSError* error_;
+};
+
 Status Status::FromNSError(NSError* error) {
   if (!error) {
     return Status::OK();
   }
 
-  NSError* original = error;
+  auto original = UnderlyingNSError::Create(error);
 
   while (error) {
     if ([error.domain isEqualToString:NSPOSIXErrorDomain]) {
       return FromErrno(static_cast<int>(error.code),
-                       MakeString(original.localizedDescription));
+                       MakeString(original->error().localizedDescription))
+          .WithPlatformError(std::move(original));
     }
 
     error = error.userInfo[NSUnderlyingErrorKey];
   }
 
   return Status{FirestoreErrorCode::Unknown,
-                StringFormat("Unknown error: %s", original)};
+                StringFormat("Unknown error: %s", original->error())}
+      .WithPlatformError(std::move(original));
+}
+
+NSError* Status::ToNSError() const {
+  if (ok()) return nil;
+
+  NSError* error = UnderlyingNSError::Recover(state_->platform_error);
+  if (error) return error;
+
+  return MakeNSError(code(), error_message());
 }
 
 }  // namespace util

Firestore/core/src/firebase/firestore/util/status_posix.cc

@@ -184,6 +184,10 @@ static FirestoreErrorCode CodeForErrno(int errno_code) {
 }
 
 Status Status::FromErrno(int errno_code, absl::string_view msg) {
+  if (errno_code == 0) {
+    return Status::OK();
+  }
+
   FirestoreErrorCode canonical_code = CodeForErrno(errno_code);
   return Status{canonical_code,
                 util::StringFormat("%s (errno %s: %s)", msg, errno_code,

Firestore/core/test/firebase/firestore/util/CMakeLists.txt

@@ -154,6 +154,7 @@ cc_test(
     hashing_test.cc
     iterator_adaptors_test.cc
     ordered_code_test.cc
+    status_apple_test.mm
     status_test.cc
     status_test_util.h
     statusor_test.cc