clientdetails: handle error interfaces in Details somewhat
#4137
Conversation
mvo5
force-pushed
the
test-clientdetails
branch
3 times, most recently
from
07ceb4c
to
bba6087
Compare
mvo5
force-pushed
the
test-clientdetails
branch
2 times, most recently
from
6fd78c2
to
c7f6fdf
Compare
We have no tests for the clienterror, start with a trivial one.
Florian pointed out that `clienterror.Error.Details` could contain arbitrarily deeply nested error values. Add code to detect this. Also deal with the (common?) case that `.Details` contains a list of errors.
mvo5
changed the title
Details somewhat
| case reflect.Struct: | ||
| for i := 0; i < v.NumField(); i++ { | ||
| if err := ensureNoErrs(v.Field(i)); err != nil { | ||
| return err |
There was a problem hiding this comment.
@mvo5 Do I get this right, that only the first will be returned (same for array and map)? Is this on purpose?
There was a problem hiding this comment.
Yes, only the first is detected right now. We can change that of course, I'm not sure if it's worth it (also I'm not sure this entire PR is the right approach, it's nice in the sense that it does detect the issue now but it still will not catch other cases where we pass something to Details interface{} that is in fact an interface that is not properly serializable to json
I still wonder if we a better appraoch would be:
a) merge #4145 to fix the immedidate issue
b) change Details inteface{} -> Details string or Details []string or Details json.RawMessage (the later would force the caller to serialize). (i.e. ensure passing an err becomes a compile time error instead of a runtime error).
There was a problem hiding this comment.
Not sure where the idea comes from that Details should be flexible but then it would be a) and b) with json.RawMessage
Starting with #4145 for now sound perfect 😏
There was a problem hiding this comment.
it's probably worth to salvage (some of) the test cases from this PR but otherwise I'm fine closing it (or closing it after (a) and (b) got done). [fwiw, (b) with json.RawMessage might become a bit annoying on the caller side, but I guess we need to try it to see how good/bad it will be :)
|
This PR is stale because it has been open 30 days with no activity. Remove "Stale" label or comment or this will be closed in 7 days. |
|
This PR was closed because it has been stalled for 30+7 days with no activity. |
The clienterror report has no tests right now, that is not ideal so this PR adds some.
While doing that it fixes an issue that @schuellerf discovered, i.e. that when an
errorinterface is passed intoclienterrors.Error.Detailsthe details get lost because the json.Marshaler will not know how to handler anerror interface.
So I think this is fine to merge and will improve the current state. However I think we should rework the
clienterrors.Error so that go types will help us detect issues at compile time. With this PR we will only
detect nested errorrs at runtime and error and it will not deal with nested interface of other types.
We should probably:
clienterrors.Errora real error type, it's kinda unusual that an error does not implement theerrorinterface.Details- AFAICT all the details will be "flattended" (via%v) after the de-serialization anyway, so we might just make details a "string". Of course testing this requires a bit of digging into the source and maybe writting some tests that validate that we indeed just use do the flattening on the other side.