joomcode / errorx

A comprehensive error handling library for Go
MIT License
1.12k stars 29 forks source link

GetAllPrintableProperties will return a map of all printable properties #22

Closed uded closed 3 years ago

uded commented 5 years ago

As discussed in #21, we can simply move some of the code and create a method corresponding to current logic to expose all properties from the error for further processing of errors in situations one would to use them in REST APIs and such.

Simple code like the one below can cover creating a struct from errorx.Error:

type ErrorJson struct {
    Message    string                 `json:"message"`
    Properties map[string]interface{} `json:"properties,omitempty"`
    Cause      interface{}            `json:"cause,omitempty"`
}

func SimpleConvertToStruct(e error) *ErrorJson {
    if e != nil {
        if err := errorx.Cast(e); err != nil {
            result :=  &ErrorJson{
                Message:    err.Message(),
                Properties: err.GetAllPrintableProperties(),
            }
            if err.Cause() != nil {
                result.Cause = SimpleConvertToStruct(err.Cause())
            }
            return result
        } else {
            return &ErrorJson{
                Message: e.Error(),
            }
        }
    } else {
        return nil
    }
}

This can be used to create a JSON returned to the end customer as an error message. But the code above is just an example, not a production-ready code! Please, review and test before using it as I haven't at all!

PeterIvanov commented 5 years ago

I believe this topic requires some more thought. Comments above are just those first things that spring to mind.

uded commented 5 years ago

I do agree that it should be discussed, hence my very initial PR. I am not sure about the direction you want to take, but I do see the utility of this method. As the implementation is, as mentioned, initial I am happy to take it somewhere else. But where? For me, the one above – despite some simplifications – is fast, efficient and flexible. If we would like to complicate this further then two risks I see on the horizon:

  1. It will require much more significant maintenance with each future change
  2. It will bear a higher risk of a failure in the feature with a more complex scenario that even I cannot predict today

IMHO KISS it...

uded commented 5 years ago

Generally, I agree with @g7r, except until now I was about to rather avoid a common struct, leaving a decision on why and how to a developer. Introducing a common GetErrorStruct or similar method would be beneficial, but somewhat limiting. And then the question is whether to limit internal APIs for such a method or rather to keep them public? And if we introduce a method to get a struct, should we - again - introduce MarshalText and MarshalJSON? I am not sure quite frankly if that is the way to go. I like it, makes life generally easier, but is also limiting.

OK, we need a decision here I guess. I am happy to take it further and work on it, but I need to have a consensus. For now, IMHO, the best course of action is to make a public function to retrieve a full list of properties, ordered or not, for each level or for all levels combined. On top of that, I can introduce, as in the crude example above, a simple struct with a method to build and retrieve it. All agreed? @g7r and @PeterIvanov? Don't want to step on everyone's toes here, would prefer a more of a consensus here...

PeterIvanov commented 5 years ago

@uded your position if very reasonable, and thank you for your initiative. I am currently just a little overwhelmed by the amount of activity that takes place in a discussion :) Could you please pause here a little? I can promise to give it a proper though at the very beginning of next week, and then I'll be ready share my thoughts on the best course of actions.

uded commented 5 years ago

Sure, no problem on my end. Currently, we use a fork here and we're rather happy with the implementation as I listed - it's sufficient, covers all of our use cases.

I fully understand that the problem is rather heavy-weight, although the implementation is really straight forward. Since I am not a maintainer - I have asked and willing to wait and then help with a final one :-)

PeterIvanov commented 4 years ago

So, I revisited the behaviour of properties:

Therefore, if we are to expose properties directly, we have 2 options:

  1. Collect all properties from a wrap chain and break on first opaque wrap. This would not produce the same result as what we see in error message, but we would have no other choice: if func (e *Error) Property(key Property) does not return one from a wrapped error, why should this new function do so?
  2. Collect the properties from current error only - and let the caller manually unwrap the cause chain of an error to collect all properties. This promotes the use of a func (e *Error) Cause() method, which is bad, but overall seems much more practical than the first option.

If we choose to pursue the second alternative, then it is OK to have Property as a key, since it is consistent with other ways to observe property of a single error.

PeterIvanov commented 3 years ago

I'm closing this PR due to inactivity.