Surprising (Incorrect?) Behavior with Result Errors returned by an HTTP Handler

[StachuDotNet]

if it's in the error rail, Ok is unwrapped (which is good) but the handler returns 404 not found on Error and ignores my handler output

If it's not in the error rail, Ok is unwrapped but Error is not. Most surprisingly, the handler returns 200 even on Error

[StachuDotNet]

"Fixing" this would likely cause unintended consequences for many existing endpoints. So, I suspect we'll tackle this after the new 'base' http handlers are released, and we've had time to build middlewares on top of it that generally replicate existing handler behavior.

[pbiggar]

This came from a user report in https://discord.com/channels/1015413734153388094/1015413734644137987/1036898660157505566

[pbiggar]

If it's not in the error rail, Ok is unwrapped but Error is not. Most surprisingly, the handler returns 200 even on Error

I think the suggestion here is that Errors/Nothings should return 404/500 status codes. Now we return 200s.

if it's in the error rail, Ok is unwrapped (which is good) but the handler returns 404 not found on Error and ignores my handler output

Unclear the issue here, I've asked the user for more context.

[edman]

As the original reporter, what I wanted was to handle "happy path" vs "error path" situations in my handler code.

My intuition was to use results + error rail to focus on the "happy path" and let my handler exit nicely when it falls in the rail. But I found dark doesn't let you do that.

A minimal example is to write a handler GET /user/:email that returns a user dict or badRequest "no user" if there's no user with that email.

This implementation does not work:

When the Result::fromOption call is in the error rail the handler returns a 404. When it is not in the rail, the handler returns a 200 with { "Error": "no user" }.

I was also surprised to see that the result being in/out the error rail causes different behavior (I assumed error rail was syntactic sugar).

---

The workaround I came up with was to implement a resultUnwrap function:

Calling this on a result before returning from the handler makes it behave like I thought it would in the error rail.

The downside is I can't code the "happy path" like I wanted. I can't use the error rail in a handler (as that's immediately a 404), and need to transform results with map and andThen throughout the handler and return resultUnwrap on it.

[edman]

One more finding..

I moved the DB::query call in the sample above to a helper findUserByEmail function to see what happens when the nested call is in/out the error rail, and found yet another behavior.

If the nested call is in the rail, the handler now returns a 500. And afaict I can't handle it anymore, can't pull that error out the rail at call site.

If it's not in the rail (it returns an option) and the caller can decide what to do like before.

Maybe my confusion comes from misunderstanding error rails. Seems they're really a platform level exception-like mechanism that says "something very wrong happened, drop everything and hand control back to the dark platform".

[pbiggar]

OK, this is really helpful, thanks so much. This response ended up quite large, as this issue really helped flesh out the scope of the problem. As such, I've split the discussion into categories to help organize it

Initial problem

As the original reporter, what I wanted was to handle "happy path" vs "error path" situations in my handler code.

My intuition was to use results + error rail to focus on the "happy path" and let my handler exit nicely when it falls in the rail. But I found dark doesn't let you do that.

The errorrail was designed for quick exploration but I'm not surprised to see people use it as a happy-path handler. So it does seem we should do better here. I expect many people will prefer to just leave it in rather than going back to handle the errors more manually.

A minimal example is to write a handler GET /user/:email that returns a user dict or badRequest "no user" if there's no user with that email.

When the Result::fromOption call is in the error rail the handler returns a 404. When it is not in the rail, the handler returns a 200 with { "Error": "no user" }.

DB::queryOnewWithExactFields { email : email } Users
|> Result::fromOption (Http::badRequest "No user")

Ok, so what you wanted here was HttpResponse(200, { email : "some email" }) or in the error case HttpResponse(404, "no user").

I think this was broken because the Result::fromOption call was on the errorrail. Because it was on the errorrail, the result would be either { email : "some email" } which would be converted to HttpResponse(200, { email : "some email" }); or errorRail(HttpResponse(400, "no user")), which would be converted to HttpResponse(404, "Not found") (which is what happens to all errorrail things).

I think this is a clear example of how Result::fromOption should not be on the error rail by default.

ISSUE: Result::fromOption (and other Result/Option functions) should probably not automatically go on the error rail

Errors vs HttpResponses

But let's say that it wasn't on the error rail. Then the result of the handler would have been Ok { email : "some email" }), which gets converted into HttpResponse(200, Ok { email : "some email" }), which is rendered as {email : "some email" }. OK, so that seems fine/good.

In the error case the result would be Error(HttpResponse(400, "no user")), which converts into HttpResponse(200, { "Error": "no user" })) - which honestly surprised me. The way this happened is that unless a handler's return value is on the errorrail or a http response, it gets converted to json and returned with a 200. So the Error gets the { "Error": ... } shape, and the inner HttpResponse is json-rendered as just its contents.

One issue is that that's not the shape Dark was expecting, which would be HttpResponse(400, Error "no user"). The code to generate that would be

match DB::queryOnewWithExactFields { email : email } Users
| Just v => v
// Just v => Http::success v // better as this would typecheck
| Nothing => Http::badRequest "No user"

You could also do:

DB::queryOnewWithExactFields { email : email } Users
|> Option::withDefault(Http::badRequest "No user") 

or if you presumably wanted it to typecheck:

DB::queryOnewWithExactFields { email : email } Users
|> Option::map(\v -> Http::success v)
|> Option::withDefault(Http::badRequest "No user")

In a sense this is what you can up with in unwrapResult. This doesn't typecheck either, so not ideal, but it makes sense as an attempt from where you were at that point.

Automatic unwrap

But I think what you were looking for was for Error/Ok and Just/Nothing to be meaningful as return values of their own. To think about that, Ok and Just would just unwrap, and Nothing would return a HttpResponse(404, "Not found"), Error is trickier - how do we know if this is a 400, 404, or 500, or something less used but valid like 407. Which I think is why you found it intuitive to put try to return Error(Http::badRequest "no user").

As a solution, some options:

• Perhaps we need Result::toHttpResponse result statusCode to convert these.
  • Also an Option::toHttpResponse option statusCode errorMessage
• just automatically unwrap Results with 500 and Options with 404
• these options are complementary, though some can be done now (functions) and some will need to go into a new HttpHandler implementation

Another solution might be to automatically unwrap the contents of Error if it had a HttpResponse in it. I'm not in favor of that, that's a big magic-y edge case compared to some alternatives.

ErrorRail as syntactic sugar

I was also surprised to see that the result being in/out the error rail causes different behavior (I assumed error rail was syntactic sugar).

Right, you expected the actual value in the handler to be used. That was actually the initial intent, as discussed in https://blog.darklang.com/real-problems-with-functional-languages. For function calls, the errorrail is automatically unwrapped, and for HTTP handlers they should be unwrapped as well. In a sense, it's supposed to act like early returns and instead it acts like a panic (though only in a handler, which is odd).

ISSUE: errorrail contents are ignored in http handlers, but shouldn't be

If we desugared it using the actual value, we'd presumably get:

match DB::queryOneWithExactFields { email : email } Users
| Nothing -> HttpResponse(404, "Not found")
| Just v -> HttpResponse(200, v)

Which I think just supports the idea that errorRail results should go into HTTP handler results.

Docs/suggestions

I think unfortunately, the biggest wins are not backwards compatible, so looking at other options, I think it would be really helpful if the editor showed what we actually expected the result to be at the bottom (for example Http 1.1/200 OK\r\n[...headers...]\r\n\r\nNot found, not `200 { "Not found" }).

Some docs around errorrails and Ok/Error/Just/Nothing that display contextually would also make a big difference here I think.

Summary of suggested changes

OK, so to summarize the changes I've seen that would make sense here (annotated BC if it's backward compatible, and NHH if it has to go into a new HTTP handler implementation):

• (BC) Result::fromOption (and other Result/Option functions) should probably not automatically go on the error rail
• (BC) Perhaps we need Result::toHttpResponse result statusCode to convert these.
  • (BC) Also an Option::toHttpResponse option statusCode errorMessage
• (NHH) just automatically unwrap Results with 500 or 400, and Options with 404
• (NHH) errorrail contents are ignored in http handlers, but shouldn't be (automatically turn Nothing into 404 and Error into 500 or 400)
• (BC) Display HTTP handler result as actual HTTP output
• (BC) When the result is a Result, Option, or ErrorRail, show contextual info explaining what will happen and what the user should do instead
• (BC) when the user has a value on the errorRail, provide a menu of refactorings including conversions to http:: values
• (BC) implement type checking in the editor

Thoughts and feedback and suggestions much appreciated!

[StachuDotNet]

This is all great. Referenced via another issue. Closing this for now as part of a re-org to our issues, but will be sure to not forget about this, and will bring back conversation in the future.