GetAllPrintableProperties will return a map of all printable properties

[uded]

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]

Test case DecorateAndAddSame shows that a single error can hold several printable values for the same property key. It seems to be an issue that this new method doesn't actually return them the same way.

[PeterIvanov]

I am worried about a public API that returns actual value of properties, not its string formatted view, like Format() does.

[uded]

I thought about it and it all depends on the perspective of how this method should be used. If it's just to show the data - maybe. But it would greatly complicate the possible implementation with no obvious reason, especially introducing a risk of the complex casting. As this method would be, IMO, used only internally by the developer to either create some sort of representation of the error (like JSON) or to log the values of properties, I do not see the point of casting them to string. Then he or she can take care of the casting as needed, possibly knowing more of the type to expect as well.

On top of that interface{} is quite gently covered by MarshalJSON and similar methods.

[PeterIvanov]

I'm not sure I fully understood all of your arguments. Presently, I disagree on this point. If the goal of this method is just to expose the same thing that is being added to error message in a more structured way, I fail to see how a string representation is not enough.
On the other hand, returning a raw value of (otherwise possibly inaccessible) property may open the door for all sorts of illicit uses, and I'm not sure it is even worth the effort of carefully considering them all. In such a general purpose library as this, it is best to keep the API as clean as possible, and I would like to avoid adding new ways to access inner data without a very good reason to do so.

[PeterIvanov]

Failure trace is bad for such cases. It is preferable to have a test func and to call it with different arguments.

[uded]

It was quickly made test, but it does cover most of the problem. Please, do pitch in if you have something specific in mind. For now, I wanted to have a PoC with simple coverage just to check, if I am not doing something extremely stupid. And I am used to this as IDE will quickly create for me. VIctim of a habit and shortcuts...

BTW, as I am maybe new to this API - I haven't noticed many scenarios to test here. Just multi-level cause errors if someone is into that and some combinations of properties. You have more experience, so propose which direction you already took. But here a limited number of scenarios is needed.

[uded]

Hmm... I analyzed the code further. Currently, only one level of properties will be returned - the top one. If one would like to return properties of the underlying cause(s), one will have to call GetAllPrintableProperties for each cause. Now, I am uncertain whether this is good or bad. For me, probably, good. But I am neither alfa nor omega, don't wanna judge for all. The supplement method I listed in this PR as an example to create a marshalable struct handles that rather well. Hence my comment about prefixing properties, but I am uncertain if that is a good approach here...

[g7r]

The stack trace in case of failure would be much more helpful if test was written in another way:

func TestFoo(t *testing.T) { case := func(name string, prop Property, value interface{}) { t.Run(tt.name, func(t *testing.T) { errData := Decorate(testType.New(name), "ouch").WithProperty(prop, value) ... } case("foo", RegisterProperty("foo"), nil) case("bar", RegisterProperty("bar"), nil) ... }

[PeterIvanov]

Yes, this suggestion is a minor one, bit it goes to making test failure to be a little more readable.

[PeterIvanov]

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

[uded]

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...

[g7r]

If the final goal is to marshal the errors, I think we should consider another approach. We can introduce some structure that contains all the public fields that are required to marshal an error. And we need a function that renders an error into that struct. That will clarify the intent and will ease introduction of "unmarshal" operation in future.

[g7r]

The problem here is that maps in Go don't preserve insertion order. Therefore the strings in strs will appear in a kind of randomized order. I don't see any particular problem with that now though but there is no reason to break ordering here.

[uded]

I agree that it might be a problem, but on the other hand I copied that from the existing implementation. It was there, just took it.
I do not see a problem with making it ordered, but currently, it's iterating over properties map, which seems to be a linked list. If that is the case, except duplicated names (again, limited use case currently, as noted in other comments, which for me is still an open question to address), the order should be mostly the same on insertion. Now, the iteration of maps is randomized, but I believe all Go devs know that...

[PeterIvanov]

Still, there is indeed a regression here.
Presently, properties are being printed in reverse order of how they were put into error (skipping overwritten earlier values).
After currently proposed change, they will be printed in random order. This needlessly adds confusion.

This must be fixed. For example, a private method may be introduced to return a resulting slice of a key-value pairs, and then messageFromProperties and the new method would use this result a but differently.

[g7r]

The stack trace in case of failure would be much more helpful if test was written in another way:

func TestFoo(t *testing.T) { case := func(name string, prop Property, value interface{}) { t.Run(tt.name, func(t *testing.T) { errData := Decorate(testType.New(name), "ouch").WithProperty(prop, value) ... } case("foo", RegisterProperty("foo"), nil) case("bar", RegisterProperty("bar"), nil) ... }

[uded]

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]

@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]

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]

So, I revisited the behaviour of properties:

• property is hidden on wrap, preserved on decorate
• properties of a single error are held in a linked list, but if a value is overwritten, the obsolete one is visible neither is output, nor in code accessors
• if a decorated/wrapped error has the same property key as the cause, both values are printed in the message

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]

Still, there is indeed a regression here.
Presently, properties are being printed in reverse order of how they were put into error (skipping overwritten earlier values).
After currently proposed change, they will be printed in random order. This needlessly adds confusion.

This must be fixed. For example, a private method may be introduced to return a resulting slice of a key-value pairs, and then messageFromProperties and the new method would use this result a but differently.

[PeterIvanov]

I'm not sure I fully understood all of your arguments. Presently, I disagree on this point. If the goal of this method is just to expose the same thing that is being added to error message in a more structured way, I fail to see how a string representation is not enough.
On the other hand, returning a raw value of (otherwise possibly inaccessible) property may open the door for all sorts of illicit uses, and I'm not sure it is even worth the effort of carefully considering them all. In such a general purpose library as this, it is best to keep the API as clean as possible, and I would like to avoid adding new ways to access inner data without a very good reason to do so.

[PeterIvanov]

Yes, this suggestion is a minor one, bit it goes to making test failure to be a little more readable.

[PeterIvanov]

I'm closing this PR due to inactivity.