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

Format errors as JSON-API #46

Closed
wants to merge 2 commits into from
Closed

Format errors as JSON-API #46

wants to merge 2 commits into from

Conversation

nono
Copy link
Member

@nono nono commented Oct 13, 2016

@aenario @jinroh I'm working on JSON-API formatting. This PR is not yet ready to be merged, but you may want to look at it to see where I'm going

It's now ready!

case vfs.ErrIllegalFilename:
return InvalidParameter("folder-id", err)
case vfs.ErrInvalidHash:
return InvalidParameter("Content-MD5", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not really an InvalidParameter or StatusUnprocessableEntity 422 (the MD5 given has the good "format") but a StatusPreconditionFailed 412 (the MD5 calculated does not match the given one)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same remark for ErrContentLengthMismatch which can fall into the same category.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, I'm going to fix that!

@jinroh
Copy link
Contributor

jinroh commented Oct 13, 2016

@nono no api2go finally, or it's coming in ?

@nono
Copy link
Member Author

nono commented Oct 13, 2016

@jinroh errors in api2go was identified as a limitation. It's tied to using the routing stuff from api2go. I don't want to use the routing stuff of api2go, only the serialization/deserialization of objects. So, I have to code it myself ;-)

@nono nono changed the title [WIP] Format errors as JSON-API Format errors as JSON-API Oct 13, 2016
@aenario
Copy link
Contributor

aenario commented Oct 13, 2016

I am OK to merge this PR as is, but in the future think we should have a similar structure for error handling in both CRUD and VFS api.

In CRUD, I opted to use the gin error handling mechanism, with the combo HTTPStatus(), c.AbortWithError and then an error handling middleware which renders the couchdb.Error to JSON if it is one. This is not so clean :

  • HTTPStatus is only useful because, we need the status code to call AbortWithError. If there was another (*Context) function equivalent to c.Error(err) && c.Abort(), it could all be handled in the error handling middleware (like you did in jsonapi.AbortWithError())
  • gin wraps the errors we pass into gin.Error and maintains a slice of them, which is pretty useless, but it can be configured to log some errors, which might prove useful.

In VFS, you went with having a jsonapi.AbortWithError() which immediately do the render, without passing the error to gin, which is way cleaner and doesnt involve


In the future we could

A. switch to echo v3, where we simply return error and then have an error handling function wich does all the triaging/formating.
B. If we stay with gin, we might do something like :

  • a common interface, type StackError interface Error(), Title(), Reason(), JSON(), JSONAPI() StatusCode() implemented by both VFS errors, couchdb errors, other errors we will need in the future and a oneline constructor.
  • a function Abort(context, StackError), which call c.Error(), c.JSON(), c.Abort() and renders the errors with proper formating depending on c.NegotiateFormat.

my 2c

@jinroh
Copy link
Contributor

jinroh commented Oct 14, 2016

Rebased and merged 226e75a

@jinroh jinroh closed this Oct 14, 2016
@nono nono deleted the jsonapi branch November 23, 2016 15:20
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

Successfully merging this pull request may close these issues.

None yet

3 participants