logger.Info(zap.Any("",*url.URL)) should not panic if *url.URL is nil #495
Comments
yutongp
changed the title
|
It's possible (and allowed) for a I don't think there's any easy way to detect this without using reflection or unsafe either. See https://groups.google.com/forum/#!topic/golang-nuts/wnH302gBa4I/discussion |
|
I noticed some interesting behaviour when playing with the From https://golang.org/pkg/fmt/
We should consider doing something similar in zap, where we do something like: defer func() {
if err := recover(); err != nil {
// use reflect to check if the value is `nil` and add error saying value is nil
// else add a log saying we panicked trying to call the method with the recovered error
}
}()
|
|
I think all use cases of In my opinion, it's performant and intuitive solution. |
This commit ensures any panic raised while encoding zap.Stringer is catched and reported as an error of the form kError for a field k. The reported panic is prefixed with "PANIC=" to distinguish these errors from other encoding errors. Closes uber-go#495 Closes uber-go#528
This commit ensures any panic raised while encoding zap.Stringer is caught and reported as an error as field kError for a field k. The reported panic is prefixed with "PANIC=" to distinguish these errors from other encoding errors. Closes uber-go#495
code:
result:
the problem is zap found out url has a
String()function and directly used it. Unfortunately, net/url doesn't perform nil ptr check inString()function.I don't think zap user should perform the nil ptr check, it is very normal to log a nil value.
I think ideally the
String()implementor should do the nil ptr check, but that is hardly enforced for every struct implementedString()So maybe zap can do the nil ptr check. The easy way is print "<nil>" if input object is nil. The harder way is call
String()first(String()may handle nil case already) and trap the panic if there is one.The text was updated successfully, but these errors were encountered: