Tunnel underlying NSErrors through C++ Status #2808
Conversation
| FirestoreErrorCode code; | ||
| std::string msg; | ||
| std::unique_ptr<PlatformError> wrapped; |
There was a problem hiding this comment.
Nit: wrapped is a little generic. It also suggests that it's a wrapped state, which actually isn't the case. Maybe something like underlying_error, platform_error, platform_specific_error, original_error, cause, etc.?
There was a problem hiding this comment.
It's definitely not a cause, it's an alternate representation that can be used to recover the original shape of the error.
Changed to platform_error and added a comment.
| FirestoreErrorCode code; | ||
| std::string msg; | ||
| std::unique_ptr<PlatformError> wrapped; |
There was a problem hiding this comment.
You haven't adjusted either Status::op==, or Status.ToString().
I think leaving Status::op== alone is correct. (This would imply that every Status with a PlatformError would be considered different, even if they looked ~ the same.) Alternatively, you could compare the underlying PlatformErrors.
I'm less sure about ToString(). Would it be sensible to add a ToString() method to PlatformError and use that as part of the Status.ToString() implementation?
There was a problem hiding this comment.
So... this is another thing that I struggled with but I think this is correct.
In the Status domain, the only attributes that matter are the code and the error message. That is to say, if there are any parts of the platform error that contribute to those values they need to be put in there because Status as a construct doesn't have a notion of error chaining.
However, we also want to make it possible to losslessly convert NSError* to Status and back, and the wrapped platform error is carried along to make that possible.
So I think of the platform error as alternate representation that we happen to carry along, but it does not meaningfully contribute to the identity of the Status object.
Note that CausedBy implements these semantics: the error message of the outer error is adjusted to contain the cause's message so that the cause is visible in the Status domain.
| Status& Status::WithPlatformError(std::unique_ptr<PlatformError> error) { | ||
| if (!ok()) { | ||
| state_->wrapped = std::move(error); | ||
| } |
There was a problem hiding this comment.
Does this make sense?
Status s = Status::OK();
s.WithPlatformError(a_notnull_platform_error);Or in other words, should we assert that error is nullptr if the status is ok?
There was a problem hiding this comment.
I've gone back and forth on this. I put it this way because it's not exactly an error to create a platform error in the success case, but retaining that error just happens to be unrepresentable so we can discard it.
On the other hand, it's wasteful to create a platform error in this case so maybe it's worth enforcing that platform errors aren't created.
I'll make the change and add tests.
| return nil; | ||
| } | ||
|
|
||
| return static_cast<UnderlyingNSError*>(wrapped.get())->error(); |
There was a problem hiding this comment.
You haven't verified that wrapped is actually of type UnderlyingNSError. You could:
a) Use RTTI. (Except we can't)
b) Add a type() method to PlatformError.
c) Change the wrapped parameter to be a unique_ptr. Which mostly works if you refactor Status::CausedBy as suggested, though it leaves Status::ToNSError with the same problem.
d) Not worry about it. Clearly, we're dealing with NSErrors here. (As other platforms/error types are added, I believe this will continue to be the case too; I don't think we'll ever have a state with more than one subtype of PlatformError active at a time.)
No action required.
There was a problem hiding this comment.
Yes. The current implementation relies on the fact that on any given platform there will only be one subtype of PlatformError active.
The subtype relationship only exists because it's cumbersome to deal with the platform error in the generic parts of Status without knowing how to manipulate it. For example, the copy constructor for Status::State becomes platform specific without it.
An alternative I had considered would be to make Status::State itself platform specific, but that's major surgery to Status and I wanted to avoid that. This change seems more targeted if suboptimal otherwise.
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
* 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
If we're going to convert all our internal error plumbing to
util::Statuswe can't lose diagnostic information for iOS users.Fixes b/130349574.