Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add extension method / active pattern to simplify extraction of propagated errors on the client #261

Open
Zaid-Ajaj opened this issue Jul 9, 2021 · 5 comments

Comments

@Zaid-Ajaj
Copy link
Owner

Reported by @isaacabraham

Something along the lines of

// shared error type, propagated by the error handler on the backend
type GlobalError = 
    | DbError of string
    | InternalError of string

let inline tryParse<'t> (input: string) : 't option = 
    /// parse json here

let (|PropagatedError|_|) (error: Exception) : 't option = 
    // extract response text and try parse to 't
    tryParse error.Message

// usage example -> pattern match against the exception
try 
    failwith "whatever"
with
| PropagatedError (errorInfo: GlobalError) ->
    match errorInfo with 
    | DbError message -> ()
    | InternalError message -> ()

@kerams what do you think?

@kerams
Copy link
Collaborator

kerams commented Jul 9, 2021

Is this supposed to propagate application errors as text and recreate the error type on the client? I'm not a fan. Why wouldn't you model application errors in your domain contract?

type Api = {
    getUser: int -> Async<Result<User, GetUserError>>
    getUser': int -> Async<GetUserResult>>
}

In case there are errors that are relevant for all methods, you can define a general response type used in every method, as I do:

type Response<'a> =
    | Unauthenticated
    // ...
    | DbError of string
    | InternalError of string
    | Fine of 'a

type Api = {
    getUser: int -> Async<Response<Result<User, GetUserError>>>
    getUser': int -> Async<Response<GetUserResult>>
    // or if no specific error is needed
    getUser': int -> Async<Response<User>>

    // also uses Response
    getOrder: int -> Async<Response<Result<Order, GetOrderError>>>

On the client you then have to unwrap the Response and map it onto whatever model you need. Here the error path in Remoting is always just transport errors, everything else is encoded in the domain and easily separable (Fine is the only success case from the application's point of view, every other DU case is an error, even though Remoting considers it a success (200)).

I'll admit the client-side message rewrapping/remapping gets a little bit hairy and complicates Elmish commands, but I really like how explicit all these errors are.

@isaacabraham
Copy link
Contributor

That's an interesting way of doing it, I quite like that. However, as far as I'm aware there's no way to push an exception into a message in Fable Remoting - you create some ErrorHandler function and then choose to ignore or propagate.

I think it's good to have the escape hatch for exceptions - especially as not everyone wants to explicitly wrap all of their individual endpoints in some kind of try/catch -> Result decorator.

Unless this is going to be removed (and bear in mind this pattern above is essentially what is documented on the Fable Remoting site, just now slightly easier to do) then I don't see the harm in adding it, personally.

@kerams
Copy link
Collaborator

kerams commented Jul 9, 2021

as far as I'm aware there's no way to push an exception into a message in Fable Remoting

No, exceptions are not easily serializable, Do you really require the exception type though, wouldn't the message/call stack suffice?

not everyone wants to explicitly wrap all of their individual endpoints in some kind of try/catch -> Result decorator.

I'm using dozens and dozens of methods, and each is decorated with a higher order function that handles authentication, catches exceptions, etc. It does not bother me in the slightest, but fair enough, it's extra work.

let api (ctx: HttpContext) = {
    getItemDetails = requireAuthentication ctx (fun req _ -> CompositionRoot.getItemDetails req)
    getItemDetail = optionalAuthentication ctx (fun itemId user -> CompositionRoot.getItemDetail (maybeUserId user) itemId)
    getItemHistory = optionalAuthentication ctx (fun (deckId, itemId) user -> CompositionRoot.getItemHistory deckId itemId (maybeUserId user) (user |> Option.map (fun x -> x.TimeZone)))
    getDeckDetail = optionalAuthentication ctx (fun (deckId, includeSections) user -> CompositionRoot.getDeckDetail (maybeUserId user) deckId includeSections)
    getDeckSchema = requireAuthentication ctx (fun req _ -> CompositionRoot.getDeckSchema req)
    getItemPreviews = optionalAuthentication ctx (fun req user -> CompositionRoot.getItemPreviews (maybeUserId user) req)
    searchItems = optionalAuthentication ctx (fun req _ -> CompositionRoot.searchItems req)
    listDecks = optionalAuthentication ctx (fun req user -> CompositionRoot.listDecks (maybeUserId user) req)
}

As long as it's a non-breaking addition, I'm cool with that, though I still feel it's dirty and wouldn't encourage its use.

@Zaid-Ajaj
Copy link
Owner Author

Zaid-Ajaj commented Jul 9, 2021

@isaacabraham I agree with @kerams that the global error handler probably isn't the best way to model your errors and users could benefit more from the explicit error handling model. On the other hand, it requires more work to be explicit and for beginners it might not be as straightforward as we think it might be to "just" decorate functions with higher order ones.

Unless this is going to be removed (and bear in mind this pattern above is essentially what is documented on the Fable Remoting site, just now slightly easier to do) then I don't see the harm in adding it, personally.

It's not going to be removed because there is no other to take care of unhandled exceptions. I do think though that we should be clear about it in the documentation and clearly stating what it is for: an escape hatch for unhandled exceptions.

In any case, I still think that making this easier would be for the better and that more advanced users can decide for themselves how to handle their errors, either globally like this (when you don't care too much, just every now and then) or explicit error handling.

@isaacabraham
Copy link
Contributor

@Zaid-Ajaj right. Basically exceptions are a fact of life in .NET, and I think making it easy to deal with that second channel as an option is fine - and making it easy to do isn't a bad thing, IMHO.

@kerams It might not be the exception type - in the error handler, you might want to just return a string or DU or something. As you both say, being explicit about errors in the type system is no bad thing, but I'm not sure it's for everyone.

For example, one of our customers wants to have a generic handler on the front end to state that a fatal error has occurred on the backend, it's been logged etc. etc. - there's no "business" process to handle them. Not saying that that's the "right" or "wrong" way, but people (particularly in stateless web apps) do use exceptions as a way to fail fast for those kind of panic situations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants