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

Simplify the From/ToJSON instances for Envelope #5

Open
lexi-lambda opened this Issue May 2, 2017 · 8 comments

Comments

2 participants
@lexi-lambda
Contributor

lexi-lambda commented May 2, 2017

Currently, Envelope wraps errors with { "err": ... } and successes with { "data": ... }. These cannot be customized by the user, and they prevent some structures I would like to be able to return. I think the ToJSON instance should simply defer to the underlying ToJSON instances without doing any wrapping to avoid imposing requirements on the user’s responses.

I also question whether or not the FromJSON instance is even a good idea at all. I’m not sure when I would want to construct an Envelope from JSON input, and as the documentation mentions, it’s problematic if the instances are ambiguous. My preference would simply be to eliminate the FromJSON instance entirely.

@lexi-lambda

This comment has been minimized.

Contributor

lexi-lambda commented May 2, 2017

Err, actually, I realized the FromJSON instance is probably for use with servant-client. In that case, the FromJSON instance is obviously necessary, but I think the “right” thing to do is probably disambiguate between the error and success cases using HTTP status code, which is related to #2 and haskell-servant/servant#732.

lexi-lambda added a commit to lexi-lambda/servant-checked-exceptions that referenced this issue May 3, 2017

lexi-lambda added a commit to lexi-lambda/servant-checked-exceptions that referenced this issue May 3, 2017

lexi-lambda added a commit to lexi-lambda/servant-checked-exceptions that referenced this issue May 3, 2017

@cdepillabout

This comment has been minimized.

Owner

cdepillabout commented May 4, 2017

@lexi-lambda

Currently, Envelope wraps errors with { "err": ... } and successes with { "data": ... }. These cannot be customized by the user, and they prevent some structures I would like to be able to return. I think the ToJSON instance should simply defer to the underlying ToJSON instances without doing any wrapping to avoid imposing requirements on the user’s responses.

I'm really on the fence about this. Personally, I like that the ToJSON instances are unambiguous with the encoding of error vs success.

However, I totally understand that some users don't like the all responses wrapped with either
{ "err": ... } or { "data": ... }.

It's somewhat hacky, but how about a solution like this: Make the ToJSON and FromJSON instances for Envelope orphan instances. Right now, the ToJSON and FromJSON instances are defined in Servant.Checked.Exceptions.Internal.Envelope, right along with Envelope. So if you import Envelope from Servant.Checked.Exceptions.Internal.Envelope, the ToJSON and FromJSON instances will be in scope.

Instead, the ToJSON and FromJSON instances could be defined in a module like Servant.Checked.Exceptions.Internal.Envelope.JSON and not imported in Servant.Checked.Exceptions.Internal.Envelope. However, they would be imported in Servant.Checked.Exceptions.

So you could import just the Envelope and Throws type with code like this:

import Servant.Checked.Exceptions.Internal.Envelope (Envelope)
import Servant.Checked.Exceptions.Internal.Servant (Throws)

If you did this, you could write your own ToJSON and FromJSON instances without having to resort to a newtype around Envelope.

@cdepillabout

This comment has been minimized.

Owner

cdepillabout commented May 4, 2017

Another solution would be to split servant-checked-exceptions into two packages. One package would be identical to the current package, just without the ToJSON and FromJSON instances defined. The other package would define the ToJSON and FromJSON instances, and re-export everything from the first package.

@cdepillabout

This comment has been minimized.

Owner

cdepillabout commented May 4, 2017

As far as the FromJSON instance goes, I think you're right that a better solution would be to differentiate between error and success with the HTTP status code.

However, one small question I have is the following: Does it ever make sense to send back an error with a HTTP 200 response? I personally can't think of a time it would make sense, but I wouldn't be surprised if someone somewhere has to work with an API like this.

@lexi-lambda

This comment has been minimized.

Contributor

lexi-lambda commented May 4, 2017

I’m not fond of any of the solutions involving orphans, especially since it could be extremely confusing if any other libraries want to depend on servant-checked-exceptions. I do totally understand the desire to make the FromJSON instances unambiguous by default, though. I’m not really using servant-client to consume my APIs, so it matters a lot less to me, but it’d be really nice to have a default that avoids a serious potential pitfall.

Ultimately, this is a minor issue for what I’m doing so far, but if I were to use this for a production application in a professional setting, there are reasons I would really need complete control over the JSON bodies in my responses. With the current API, it’s impossible to get rid of the wrappers, while simplified instances would at least allow users to opt-in. It’s still not a very good solution, though.

Perhaps an alternative approach would be to allow Throwing to be parameterized on the type of Envelope it wraps its values in, expecting any type with kind [*] -> * -> *. Then, you could export Throws as an alias for Throwing Envelope, but you could also provide a variant that uses a different type with other instances. This would also allow power users to customize the behavior of the wrapper if they really wanted to.

@lexi-lambda

This comment has been minimized.

Contributor

lexi-lambda commented May 4, 2017

However, one small question I have is the following: Does it ever make sense to send back an error with a HTTP 200 response? I personally can't think of a time it would make sense, but I wouldn't be surprised if someone somewhere has to work with an API like this.

You’re right that this is totally feasible for certain use-cases. Personally, I’m not sure it’s worth supporting this if it’s a ton of additional work (since this use case is pretty pathological), but this is all theoretical right now anyway, since servant-checked-exceptions doesn’t have any mechanism in place to tweak status codes.

@cdepillabout

This comment has been minimized.

Owner

cdepillabout commented May 4, 2017

Perhaps an alternative approach would be to allow Throwing to be parameterized on the type of Envelope it wraps its values in, expecting any type with kind [*] -> * -> *.

This is a good idea. It would allow a user to use a different type if they really wanted.

If you (or someone else) wanted to implement this, I would accept the PR.

If I were to use this for a production application in a professional setting, there are reasons I would really need complete control over the JSON bodies in my responses.

I agree with this. It's somewhat a problem with Servant as it is, but I don't think servant-checked-exceptions should exacerbate the problem.

@cdepillabout

This comment has been minimized.

Owner

cdepillabout commented Sep 25, 2017

Another idea could be to parameterize both Throwing and Envelope with a phantom type, and provide a default that works the same way it is currently working.

data DataAndErr deriving Typeable

data Throwing' phantom

type Throwing = Throwing DataAndErr

data Envelope' phantom es a = ...

type Envelope es a = Envelope DataAndErr es a

instance ToJSON (Envelope' DataAndErr es a) where ...

The majority of the servant-checked-exceptions library could stay as-is. All functions currently dealing with an Envelope could be changed to take an Envelope' instead.

The end user should be able to create their own type (similar to DataAndErr) that they can use to write their own ToJSON instance for Envelope'.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment