Add more information to federation errors #1560
Conversation
pcapriotti
force-pushed
the
pcapriotti/improve-fed-errors
branch
from
1353fb8 to
12c0c7f
Compare
pcapriotti
force-pushed
the
pcapriotti/improve-fed-errors
branch
from
eb63d5f to
12eddaf
Compare
pcapriotti
changed the title
| instance Exception Error | ||
|
|
||
| data ErrorData = FederationErrorData | ||
| { federrDomain :: !Domain, |
There was a problem hiding this comment.
Is this domain the originating domain or the remote domain? Possibly FederationErrorData could be extended in the future to contain both.
There was a problem hiding this comment.
It's the remote domain. I assumed the originating domain would not be needed because it would simply be the one where the instance is running. But yes, it can be added easily.
| RequestBodyLBS lbs -> pure $ LBS.toStrict lbs | ||
| RequestBodyBS bs -> pure bs | ||
| RequestBodySource _ -> failure FederationClientStreamingUnsupported | ||
| put (Just path) |
There was a problem hiding this comment.
Hm, does this make an actual put request? Shouldn't this only happen a few lines below in callRemote ? Or why is this line needed now and wasn't before?
There was a problem hiding this comment.
Good question, I should have explained what this is in a comment. It is not a PUT request, but simply saving the path in the state of the monad stack. The function put is this one.
The idea here is that Servant is going to call this function for each request, and call throwClientError when some error occurs while translating a response into a Servant response. Since the path we want to add to the error structure is contained in the request, we have no direct way to access it from our implementation of throwClientError below.
My solution was to change the monad to keep a Maybe ByteString in its state, set it when a request is received, and fetch it when we need to produce an error. As far as I understand, Servant guarantees that throwClientError is only called after runRequestAcceptStatus, so this should work fine. If for any reason this guarantee does not hold, we get an empty path, but nothing catastrophic.
There was a problem hiding this comment.
Ah, that makes more sense. Too many functions have the same name and without a qualified import it's not directly obvious. Thanks for the explanation and the comments!
| RequestBodyLBS lbs -> pure $ LBS.toStrict lbs | ||
| RequestBodyBS bs -> pure bs | ||
| RequestBodySource _ -> failure FederationClientStreamingUnsupported | ||
| put (Just path) |
There was a problem hiding this comment.
Ah, that makes more sense. Too many functions have the same name and without a qualified import it's not directly obvious. Thanks for the explanation and the comments!
This adds a
datafield toWai.Error, containing some more specific error data. For now, only federation-specific data is supported. The federation error-conversion functions add domain and path information to the error, which can be used to diagnose which particular federated request failed in case of federation errors.Most of the diff is about adapting every usage point of
Wai.Error. SinceWai.Errorgained a new field, and the constructor was used directly everywhere, I have replaced all uses of the constructor with a call to a smart constructormkErrorwhich sets the extra data toNothing.