JSON formatting for an error

[uded]

It would be, IMHO, reasonable to make a custom MarshalJSON() function to handle marshaling given Error with all printable properties and, potentially, stack trace to JSON. Maybe not all will need this, but a lot of folks are using some Web framework, and that might come in handy...

As I am happy to make the implementation, I have two open questions which need an answer to address a variety of needs, not the one I see at this point:

1. As I assume the format can be something like:

   { 
       "msg": "error was here",
       "properties": {
           "propertyString": "value",
           "propertyInt": 123
       }
   }

   I am not sure if the stacked errors should appear as a loop under some property like cause and so on? Just thinking here. One more single cause property with a message might be also sufficient, but I am not certain which would be more useful here...
2. As I am new to this package, am I missing something that should be handled by MarshalJSON on top of that? Just asking as I am really not sure if there is something I am missing otherwise...

A really quick and simple implementation can be as follows:

func (e *Error) MarshalJSON() ([]byte, error) {
	return json.Marshal(&struct {
		Message    string                 `json:"message"`
		Properties map[string]interface{} `json:"properties,omitempty"`
	}{
		Message: e.message,
		Properties: e.mapFromPrintableProperties(),
	})
}

func (e *Error) mapFromPrintableProperties() map[string]interface{} {
	uniq := make(map[string]interface{}, e.printablePropertyCount)
	for m := e.properties; m != nil; m = m.next {
		if !m.p.printable {
			continue
		}
		if _, ok := uniq[m.p.label]; ok {
			continue
		}
		uniq[m.p.label] = m.value
	}
	return uniq
}

Happy to make a PR if that is acceptable for a larger audience

[PeterIvanov]

To start the discussion properly, we should examine at least a couple of real life use cases for this feature. Those could help us answer some important questions:

• should we marshal 'cause' and in what way?
• should we marshal 'type'?
• must the 'message' be the same as in '%v' formatting result or not?

All of this depends heavily on exactly how this is supposed to be used - or abused, for that matter.
The second thing to think about is: should such marshalling be just another form of '%+v' formatting or a proper serialized error?

And this brings us to the third thing, the elephant in the room: should there be an unmarshalling for error? I intuitively resist the idea, but if we start marshalling it one way, we will get to doing it both ways - sooner or later. And if so, then marshalling must be designed so that proper unmarshalling can be implemented later without breaking the backwards compatibility.

[uded]

• Ad 'cause'
  Yes, let's marshal the root cause, but that comes with some tricky questions as I started to examine our use cases. First of all, not always and not all of them one might want to show to end-users. fThis will be tricky as either there should be printable causes and non-printable causes. One MarshalJSON method might not be enough here...
  On the other thand one can do their town handlers to create a properly structured JSON, based on their logic. After errorx.Cast() one can easily get a non-formatted message, but - as far as I checked - no access to root-cause and property map is public. Hence it will limit what one can do, and hence... well, look below.
• Ad 'type' question:
  In my opinion, no. Again, the same problem as above - it is difficult to make one's own struct since access to errorx.Error properties are private. But I would prefer to make my own...
• Ad message the same as %v
  No, IMO JSON will be used for the web APIs, hence it should be simplified dramatically. But, as above, would prefer my own...

The biggest problem I can see is a lack of an easy access to errorx.Error properties, this way or another that would enable anyone to make their own, custom Error structs. I see this as a much better option compared to built-in MarshalJSON, since requirements for what and how should be propagated back to the end-user might be different. Hence, maybe we should change a discussion to something like what to make more public in the API, rather than to continue how to marshal a universal, more or less, JSON from an Error...

If we would make a method like the one I listed above, func mapFromPrintableProperties() map[string]interface{}``, publicly available then one problem is of the table - one can easily get all printable properties, so that plus Message()method should cover 80% of cases. Furthermore, a simple change tofunc mapFromProperties(includePrintable bool) map[string]interface{}can cover additional 2% of user cases. As I can public access toTypeandCause` is already there, so the only missing part seems to be a map of properties that can be easily fetched. From that point on everybody can be a cook and make their own JSON error structs with pretty much all of the information they might need...

[PeterIvanov]

OK, so, to make long story short, your proposal currently boils down to a public API like this:

func (e *Error) GetAllProperties() map[Property]interface{} { ... }

I'm not entirely sure this is harmless. First, you could access some 'private' properties of an error that are currently out of your reach, since property key is private within some other package. Second, the presence of such API could send a wrong message about how errors should be inspected. Then again, we already have this concern about Cause() method.

[uded]

That's why my first proposal would be to include only printable properties, as those will be visible anyway. I am somewhat reluctant and concerned about the other version of the API method in a form of:

func (e *Error) GetAllProperties(printableOnly bool) map[Property]interface {}
func (e *Error) GetAllProperties(printableOnly bool) map[string]interface {}

The default for printableOnly parameter should be true.

Here, also, I am reluctant to return a full errorx.Property and rather would opt to return only the label. The reasoning here is that for showing, logging, etc. I do not need a full property and there is no point to give me back the information if it is printable. But if we would opt to both printable and non-printable API then it will make sense.

My personal preference (and, already, a test implementation) is to use a method GetAllPrintableProperties and my whole team found it sufficient for all related purposes, i.e. logging and JSON marshaling. I would advise to follow that path if anything...

func (e *Error) GetAllPrintableProperties() map[string]interface {}

[edit: added the change mentioned above to my fork, take a look if you want]

[PeterIvanov]

Thank you.
Still, most of the argument about such API is also true for printable properties. Being 'printable' is just an optional convenience feature, a way to avoid putting the same data both in property (for inspection in a handler code) and in message (for logging and debug). If someone adds and property and then makes it printable, it is currently a simple choice of making it show in a debug log - not of making it somehow more easily accessible for inspection in an error handler's code.
We could, of course, resort to signature like this:

func (e *Error) GetAllPrintableProperties() map[Property]string

This, at least, is consistent with error formatting: only the string view of printable properties is exposed. I'm not entirely happy with this 'raw string' approach, too, though.
As for why a map key should be of type Property - well, distinct properties may easily have the same label. In case of a wrapped error, it gets even trickier, as errors in wrap chain may have different values for the same property keys - and this added method should work consistently with error formatting in this case, too.

[uded]

OK, you raised an interesting point about properties. But I see a surprising result of this - no matter whether this method will return errorx.Property or simple string, this collision is unavoidable and already existing! In any case here, we have a more flexible option as any developer can decide what should be returned from his/her code. If I understand the code correctly current formatting method is going around this problem using a unique map to hold existing names (as a string) to "cover" such a collision. And it's doing things for you, while I am – in the end – proposing a simple API method that will just make it public for you. What you're going to do with it is up to you.

Hence the code like:

func (e *Error) GetAllPrintableProperties() map[string]interface{} {
	uniq := make(map[string]interface{}, e.printablePropertyCount)
	for m := e.properties; m != nil; m = m.next {
		if !m.p.printable {
			continue
		}
		if _, ok := uniq[m.p.label]; ok {
			continue
		}
		uniq[m.p.label] = m.value
	}
	return uniq
}

The body of this method is pretty much a cut&paste from the method errorx.Error.messageFromProperties - I am not reinventing the wheel here, just using what I've found in the code. So if we should discuss, then that there is a completely new discussion ahead of us as this problem already existed and still exists in the code.

If we would go into a general discussion about properties, logging, console printing, etc. - that is a different story. I generally agree with the decisions already made in the library, and how the properties are handled is solid but not perfect. If you would like to improve this further the only way around for me would be to prefix all properties with the full namespace of the error. But that is yet another ticket and, I believe, another discussion. For not it's not a problem if you look at the example struct generation code I have attached in the PR. Again, just an example, but it does address most of the problems for me at this point. Obviously, it doesn't mean we cannot improve upon it, but this overall approach is quite good enough to be used at this time...