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

errors: errorString JSON marshaled #10748

Closed
manucorporat opened this issue May 7, 2015 · 15 comments
Closed

errors: errorString JSON marshaled #10748

manucorporat opened this issue May 7, 2015 · 15 comments
Milestone

Comments

@manucorporat
Copy link

@manucorporat manucorporat commented May 7, 2015

I would be very convenient to be able to JSON marshal an error created with errors.New() (ie. *errorString)
Right now it is just not possible.

I purpose this solution:
https://github.com/gin-gonic/go/commit/a79095c87e671d0ec2d62d96db957bfe8fb531c0

@manucorporat manucorporat changed the title errorString JSON marshaled errors: errorString JSON marshaled May 7, 2015
@dominikh
Copy link
Member

@dominikh dominikh commented May 7, 2015

C.f. #8778 and #5161

@manucorporat
Copy link
Author

@manucorporat manucorporat commented May 7, 2015

Nice! thanks!

@manucorporat
Copy link
Author

@manucorporat manucorporat commented May 7, 2015

#8778 and #5161

My two cents:

We should not use Error() to marshal an error. I may have a custom error like:

type MyError struct {
    Code int `json:"code"`
    Message string `json:"message"`
}

func (err *MyError) Error() string {
    return fmt.Sprintf("[%d] %s", err.Code, err.Message")
}

If I JSON marshal it, nobody would expect:

error: "[2] this is a error"

instead:

error: {"code": 2, "message": "this is an error"}

We have to fix errorString, the error interface is just that, a interface. No Magic.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 7, 2015

We don't want to let the low-level errors package import the encoding/json package. That will bring encoding/json, and all of its dependencies, into all sorts of programs that don't need it.

But it might work to implement a MarshalText method.

@ianlancetaylor ianlancetaylor added this to the Go1.5Maybe milestone May 7, 2015
@manucorporat
Copy link
Author

@manucorporat manucorporat commented May 7, 2015

@ianlancetaylor that makes sense! I implemented MarshalText() for you.
https://github.com/gin-gonic/go/commit/52d7945d572c3546a7e47ad56b887cf641cd86e1

it works perfectly fine. I also reviewed the source code of json package and looks safe.

@manucorporat
Copy link
Author

@manucorporat manucorporat commented May 7, 2015

Let me introduce an use case:

The Gin Framework provides some facilities for error management during a http request. Basically it provides a tool to collect any error or several ones. Each error is defined internally as:

type errorMsg struct {
    Error error       `json:"error"`
    Type  int         `json:"-"`
    Meta  interface{} `json:"meta"`
}

Then, by using a middleware you can read the list of errors, and do whatever you want: write the errors is a log, send to sentry, write in the http response...

Simple error handler middleware:

func errorHandler(c *gin.Context) {
    c.Next()

    if len(c.Errors) > 0 {
        c.JSON(-1, c.Errors) // this does not work as expected
    }
}

As you can see, if the canonical value of the Error field is an instance of *errorString it will not work as expected since the error message is not marshalled.

@minux
Copy link
Member

@minux minux commented May 7, 2015

@manucorporat
Copy link
Author

@manucorporat manucorporat commented May 7, 2015

Not all errors are from errors.New, what if the error is a custom error
type implemented in user code?

I know! what is the problem? I am not talking about error, I only want to fix errorString, the error created by errors.New().
https://github.com/gin-gonic/go/commit/52d7945d572c3546a7e47ad56b887cf641cd86e1

If the error is a custom error, for example:

type MyError struct {
    Code int `json:"code"`
    Message string `json:"message"`
}

func (err *MyError) Error() string {
    return fmt.Sprintf("[%d] %s", err.Code, err.Message")
}

json.Marshal(MyError{2, "message"}) // WORKS!
json.Marshal(errors.New("message")) // DOES NOT WORK! << should be fixed (only this case)

you don't really have to implement MashaslJSON() or MarshalText. The json package uses reflexion in order to generate the json string from the exported field (upper case).

@minux
Copy link
Member

@minux minux commented May 7, 2015

What if the error is in user code that doesn't implement MashaslJSON
or MarshalText and doesn't have exported fields? Is marshaling to "{}"
acceptable?

Fixing errors.errorString this way doesn't fix the generic problem.

http://play.golang.org/p/5d08Ec2rdQ

@manucorporat
Copy link
Author

@manucorporat manucorporat commented May 7, 2015

@minux ok, I see your point now.
how about this solution, modifying the json package:

  1. Try MarshalJSON(), continue if not implemented
  2. Try MarshalText(), continue if not implemented
  3. Try generate json with exported field, continue if empty
  4. If interface is error, use Error()

NOTE: probably this should be implemented for XML, Gob... as well

@adg
Copy link
Contributor

@adg adg commented May 8, 2015

I don't think we can do this. Users might already depend on their error types not being marshalled using the Error method, and such a change would break their code.

To address your specific case, why don't you make your JSON method recognise error values and handle them specially?

@robpike
Copy link
Contributor

@robpike robpike commented Jun 19, 2015

Design issues aside, your proposed solution introduces a circular dependency because encoding/json depends on the errors package. I do not know how you got it to work.

To be sure, I just tried it:

% go build
can't load package: import cycle not allowed
package errors
imports encoding/json
imports bytes
imports errors
%

You are better off handling this in your own code.

@robpike
Copy link
Contributor

@robpike robpike commented Jun 19, 2015

See also #3353

@rogpeppe
Copy link
Contributor

@rogpeppe rogpeppe commented Jun 19, 2015

I'm not convinced that having automatic marshaling of errors is a good thing.
If something returns an error, it could be any one of many different types,
not all of which may be designed to be marshaled. Moreover there's no way that
we can provide enough information in the resulting JSON (or YAML or ...)
that the original error can be reassembled.

Implementing MarshalText on errors.errorString is never going to be that useful.
If you're marshaling an error, you're almost certainly making a mistake.

For the record, our approach to error marshaling is to have a function
that is called to explicitly transform errors to marshalable values,
for example:

func(err error) (httpStatus int, errorBody interface{})

That function can inspect the error and decide on how it should
be represented as an HTTP response (in this case), falling back
to using the error string if it doesn't fit with any of the known types.

This is not something that can be done by the individual error types themselves,
as an error may be used in many different marshaling contexts.

@rsc
Copy link
Contributor

@rsc rsc commented Jul 15, 2015

I agree with Roger.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
9 participants
You can’t perform that action at this time.