errors: Improve tests, unify interfaces, wrapping

errors: Improve tests, unify interfaces, wrapping

The errors package is refactored so that for all
errors defined there are 3 functions with consistent
names:
 * Is<type>(error) bool
 Returns true if the error satifies <type> (i.e.
 it was created with one of the two constructors
 below for <type>.
 * New<type>(error, string) error
 Creates an error of <type> wrapping the given error
 and adding the prefix to the message.
 * <type>f(string, ...interface{}) error
 Creates an error of <type> with a message, formatted
 like fmt.Sprintf.

Dropped "Error" from type and function names (as they
already are in the "errors" package).
Moved utils.ErrorContextf in here, renaming it to
Maskf. A new helper called Contextf is added, which works
similarly to Maskf, but preserves the original error when
wrapping (if it's one of the known types), so that the
Is<type>() satifiers will work on the wrapped error as
well.

Added automatic exhaustive testing for each error
type in a way that makes it easier to add new types.

Few drive-by fixes made while fixing the rest of the
code base to use the new function names.

I realize we should use errgo for such things, but
we can easily change the errors package to use errgo
internally later.

https://code.launchpad.net/~dimitern/juju-core/396-improve-errors/+merge/215652

(do not edit description out of merge proposal)

[dimitern, 2014-04-14 12:51:36]

Please take a look.

[rog, 2014-04-14 13:21:37]

Big +1 to the renaming improvements. 

I haven't looked at anything apart from errors.go, but I have a suggestion,
included below.

It seems a bit of a pity that Contextf is the only way to wrap an error without
losing the diagnosis, but we can move towards errgo later.

https://codereview.appspot.com/87560043/diff/1/errors/errors.go
File errors/errors.go (right):

https://codereview.appspot.com/87560043/diff/1/errors/errors.go#newcode13
errors/errors.go:13: Err error
These fields don't need to be exported AFAICS (and
I'm not entirely convinced by the usefulness of keeping
the underlying error here, given that there's no
way of extracting it, but we can leave it alone for
the time being.).

https://codereview.appspot.com/87560043/diff/1/errors/errors.go#newcode39
errors/errors.go:39: var allErrors = []struct {
Suffix and ArgsConstructor don't appear to be used outside tests.

How about something a bit simpler:

// newer is implemented by error types
// that can add a context message while
// preserving their type.
type newer interface {
    new(msg string) error
}

then on each type, you implement newError.
for example:

func (e *notFound) new(msg string) error {
    return NewNotFound(e, msg)
}


then Contextf can be:

func Contextf(err *error, format string, args ...interface{}) {
    if *err == nil {
        return
    }
    msg := fmt.Sprintf(format, args...)
    errNewer, ok := (*err).(newer)
    if ok {
        return errNewer.new(msg)
    }
    return fmt.Errorf("%s: %v", msg, *err)
}

then each error type is independent
and the tests are the only place we need to have a
table that ties them all together.

https://codereview.appspot.com/87560043/diff/1/errors/errors.go#newcode57
errors/errors.go:57: if *err != nil {
if *err == nil {
    return
}

[dimitern, 2014-04-14 13:44:26]

Please take a look.

https://codereview.appspot.com/87560043/diff/1/errors/errors.go
File errors/errors.go (right):

https://codereview.appspot.com/87560043/diff/1/errors/errors.go#newcode13
errors/errors.go:13: Err error
On 2014/04/14 13:21:38, rog wrote:
> These fields don't need to be exported AFAICS (and
> I'm not entirely convinced by the usefulness of keeping
> the underlying error here, given that there's no
> way of extracting it, but we can leave it alone for
> the time being.).

Unexported the fields.

https://codereview.appspot.com/87560043/diff/1/errors/errors.go#newcode39
errors/errors.go:39: var allErrors = []struct {
On 2014/04/14 13:21:38, rog wrote:
> Suffix and ArgsConstructor don't appear to be used outside tests.
> 
> How about something a bit simpler:
> 
> // newer is implemented by error types
> // that can add a context message while
> // preserving their type.
> type newer interface {
>     new(msg string) error
> }
> 
> then on each type, you implement newError.
> for example:
> 
> func (e *notFound) new(msg string) error {
>     return NewNotFound(e, msg)
> }
> 
> 
> then Contextf can be:
> 
> func Contextf(err *error, format string, args ...interface{}) {
>     if *err == nil {
>         return
>     }
>     msg := fmt.Sprintf(format, args...)
>     errNewer, ok := (*err).(newer)
>     if ok {
>         return errNewer.new(msg)
>     }
>     return fmt.Errorf("%s: %v", msg, *err)
> }
> 
> then each error type is independent
> and the tests are the only place we need to have a
> table that ties them all together.

Done.

https://codereview.appspot.com/87560043/diff/1/errors/errors.go#newcode57
errors/errors.go:57: if *err != nil {
On 2014/04/14 13:21:38, rog wrote:
> if *err == nil {
>     return
> }

Done.

[rog, 2014-04-14 18:19:27]

On further reflection, I'm not convinced that this change is benign. Currently
anywhere that uses ErrorContextf is hiding the underlying error cause. This is
actually a good thing in general, although we do want the capability to let the
cause through when specifically desired.

I would like to keep the old version of ErrorContextf around and continue to use
it everywhere that it is currently used. I am OK if we add a different version
(or possibly add an argument to ErrorContextf) that does pass through the
errors, but I would be happier if we moved towards the errgo style of explicitly
declaring when we want to pass an error type back.

Perhaps something like this?

// Contextf prefixes any error stored in err with text formatted
// according to the format specifier. If err does not contain an
// error, Contextf does nothing. If satifies is non-nil and returns
// true when called with the error, then the error type is preserved.
// It is an error if satisfies returns true for an error that
// was not created by this package.
func Contextf(err *error, satisfies func(error) bool, f string, a
...interface{})

https://codereview.appspot.com/87560043/diff/2/errors/errors.go
File errors/errors.go (right):

https://codereview.appspot.com/87560043/diff/2/errors/errors.go#newcode60
errors/errors.go:60: *wrapper
I think this should be a value type inside the struct and each error type should
itself be a pointer.

It's conventional in Go for errors to be passed around as pointer types (and it
means that if one of these errors is printed with %#v, it'll print something
more useful)

https://codereview.appspot.com/87560043/diff/2/errors/errors.go#newcode69
errors/errors.go:69: return notFound{wrap(nil, format, " not found", args...)}
return &notFound{wrap(...)}

(you'll need to change the return type of the wrap function too)

https://codereview.appspot.com/87560043/diff/2/errors/errors_test.go
File errors/errors_test.go (right):

https://codereview.appspot.com/87560043/diff/2/errors/errors_test.go#newcode48
errors/errors_test.go:48: func (s *errorSatisfier) String() string {
Why not define this on the type used to construct the allErrors slice?

e.g.

type errorInfo struct {
      satisfier       func(error) bool
      argsConstructor func(string, ...interface{}) error
      wrapConstructor func(error, string) error
      suffix          string
}

func (t *errorInfo) satisfierName() string {
    value := reflect.ValueOf(s.f)
    f := runtime.FuncForPC(value.Pointer())
    return f.Name()
}

then the test code can be simpler.

https://codereview.appspot.com/87560043/diff/2/errors/errors_test.go#newcode172
errors/errors_test.go:172: for _, errorInfo := range allErrors {
I can't understand the logic here any more.

I think the fact that you have both tests with and without a valid satisfier,
and that you're testing contextf too, is complicating matters considerably.

Also, the only reason errorSatisfier was there to begin with was so that we
could know which error type we're on so that we could do the != test.

If we move the nil satifier tests and the contextf tests to their own test
functions, then I think the logic can remain almost as simple as it was before.

You won't need to use errorInfo in creating the test cases.

https://codereview.appspot.com/87560043/diff/2/errors/errors_test.go#newcode183
errors/errors_test.go:183: if !t.satisfier(t.err) {
Hasn't this already been checked just above?

https://codereview.appspot.com/87560043/diff/2/state/apiserver/usermanager/us...
File state/apiserver/usermanager/usermanager.go (right):

https://codereview.appspot.com/87560043/diff/2/state/apiserver/usermanager/us...
state/apiserver/usermanager/usermanager.go:71: err =
errors.NewAlreadyExists(err, "Failed to create user")
This doesn't seem right. How do we know that the error is because the user
already exists?

[fwereade, 2014-04-17 09:55:35]

On 2014/04/14 18:19:27, rog wrote:
> On further reflection, I'm not convinced that this change is benign. Currently
> anywhere that uses ErrorContextf is hiding the underlying error cause. This is
> actually a good thing in general, although we do want the capability to let
the
> cause through when specifically desired.

I would like to gently remind you that we -- the, uh, royal we -- want to pass
error types by default, and to explicitly mask specific errors at judiciously
chosen boundaries; my best judgment is that this approach is more likely to
remain robust in the face of likely future refactoring. 

> I would like to keep the old version of ErrorContextf around and continue to
use
> it everywhere that it is currently used. I am OK if we add a different version
> (or possibly add an argument to ErrorContextf) that does pass through the
> errors, but I would be happier if we moved towards the errgo style of
explicitly
> declaring when we want to pass an error type back.

I *do* care a lot about stability, though, and you're right here: this CL
represents is a moderately significant change in behaviour, in an area that can
have subtle and surprising impac, and we should preserve the original
ErrorContextf behaviour. But that's not because it's *good* behaviour; in fact,
IMO, ErrorContextf's masking behaviour bears much responsibility for our poor
error reporting (we need types, so we don't use ErrorContextf, so we end up
without broader context for our errors). As we get opportunities to better
annotate our errors *without* necessarily discarding types, we should absolutely
take them.

You have made your case re mask-by-default, and not convinced me of its
rightness; this may change in the fullness of time as we encounter problems with
my preferred approach, but please don't make these statements as though they're
agreed-upon policy when they are in fact the opposite.

[rog, 2014-04-17 12:19:19]

On 17 April 2014 10:55,  <fwereade@gmail.com> wrote:
> On 2014/04/14 18:19:27, rog wrote:
>>
>> On further reflection, I'm not convinced that this change is benign.
>
> Currently
>>
>> anywhere that uses ErrorContextf is hiding the underlying error cause.
>
> This is
>>
>> actually a good thing in general, although we do want the capability
>
> to let the
>>
>> cause through when specifically desired.
>
>
> I would like to gently remind you that we -- the, uh, royal we -- want
> to pass error types by default, and to explicitly mask specific errors
> at judiciously chosen boundaries; my best judgment is that this approach
> is more likely to remain robust in the face of likely future
> refactoring.

Refactoring is my major concern too. If an error is masked when
it's returned, it cannot affect behaviour further up the call stack.
That means that we are free to refactor things to change the
possible error type without worrying about unexpectedly changing
behaviour.

That's why I think that *when writing new code* we should mask
errors unless we explicitly know that the error type is part of
the function's contract. That keeps us honest and means we
can't sneakily depend on some error that's returned by
some function 5 levels deep that actually only happens to be
a specific implementation detail and may be changed at any
point. We already have code like that - it's really hard to understand
how it works, and even harder to refactor.

For existing code, it's not so easy to know when a returned
error type is intentional or not, so I'm happy to preserve
the types by default, hopefully with the eventual aim of
reducing "action-at-a-distance" error returning to a minimum.

IMO the problem with ErrorContextf as it always was, is that
it didn't provide *any* way to preserve an error type, not that
it masked most error types.

[dimitern, 2014-04-17 13:12:58]

Please take a look.

https://codereview.appspot.com/87560043/diff/2/errors/errors.go
File errors/errors.go (right):

https://codereview.appspot.com/87560043/diff/2/errors/errors.go#newcode60
errors/errors.go:60: *wrapper
On 2014/04/14 18:19:28, rog wrote:
> I think this should be a value type inside the struct and each error type
should
> itself be a pointer.
> 
> It's conventional in Go for errors to be passed around as pointer types (and
it
> means that if one of these errors is printed with %#v, it'll print something
> more useful)

Done.

https://codereview.appspot.com/87560043/diff/2/errors/errors.go#newcode69
errors/errors.go:69: return notFound{wrap(nil, format, " not found", args...)}
On 2014/04/14 18:19:28, rog wrote:
> return &notFound{wrap(...)}
> 
> (you'll need to change the return type of the wrap function too)

Done.

https://codereview.appspot.com/87560043/diff/2/errors/errors_test.go
File errors/errors_test.go (right):

https://codereview.appspot.com/87560043/diff/2/errors/errors_test.go#newcode48
errors/errors_test.go:48: func (s *errorSatisfier) String() string {
On 2014/04/14 18:19:28, rog wrote:
> Why not define this on the type used to construct the allErrors slice?
> 
> e.g.
> 
> type errorInfo struct {
>       satisfier       func(error) bool
>       argsConstructor func(string, ...interface{}) error
>       wrapConstructor func(error, string) error
>       suffix          string
> }
> 
> func (t *errorInfo) satisfierName() string {
>     value := reflect.ValueOf(s.f)
>     f := runtime.FuncForPC(value.Pointer())
>     return f.Name()
> }
> 
> then the test code can be simpler.

Good idea, done.

https://codereview.appspot.com/87560043/diff/2/errors/errors_test.go#newcode172
errors/errors_test.go:172: for _, errorInfo := range allErrors {
On 2014/04/14 18:19:28, rog wrote:
> I can't understand the logic here any more.
> 
> I think the fact that you have both tests with and without a valid satisfier,
> and that you're testing contextf too, is complicating matters considerably.
> 
> Also, the only reason errorSatisfier was there to begin with was so that we
> could know which error type we're on so that we could do the != test.
> 
> If we move the nil satifier tests and the contextf tests to their own test
> functions, then I think the logic can remain almost as simple as it was
before.
> 
> You won't need to use errorInfo in creating the test cases.

Moved Maskf, Contextf and the rest of the error tests in separate test cases. It
should be much more straightforward to read and understand now.

https://codereview.appspot.com/87560043/diff/2/errors/errors_test.go#newcode183
errors/errors_test.go:183: if !t.satisfier(t.err) {
On 2014/04/14 18:19:28, rog wrote:
> Hasn't this already been checked just above?

This is now changed.

https://codereview.appspot.com/87560043/diff/2/state/apiserver/usermanager/us...
File state/apiserver/usermanager/usermanager.go (right):

https://codereview.appspot.com/87560043/diff/2/state/apiserver/usermanager/us...
state/apiserver/usermanager/usermanager.go:71: err =
errors.NewAlreadyExists(err, "Failed to create user")
On 2014/04/14 18:19:28, rog wrote:
> This doesn't seem right. How do we know that the error is because the user
> already exists?

Reverted the logic back, just reformatted.

[rog, 2014-04-17 14:44:40]

LGTM - tests are much improved, thanks.

https://codereview.appspot.com/87560043/diff/20001/errors/errors_test.go
File errors/errors_test.go (right):

https://codereview.appspot.com/87560043/diff/20001/errors/errors_test.go#newc...
errors/errors_test.go:94: c.Check(message, gc.Equals, "<nil>")
neither of the two checks above are testing anything apart from what's in the
test.

it seems a bit odd that what we check is depending on the
err value, not our expected value.

i'd expect to see something more like this:

func checkErrorMatches(c *gc.C, err error, message string) {
    if message == "" {
        c.Assert(err, gc.Equals, nil)
    } else {
        c.Assert(err, gc.ErrorMatches, message)
    }
}

https://codereview.appspot.com/87560043/diff/20001/errors/errors_test.go#newc...
errors/errors_test.go:131: mustNotSatisfy(c, t.err, t.errInfo)
isn't this checked below?

[dimitern, 2014-04-17 15:04:33]

Please take a look.

https://codereview.appspot.com/87560043/diff/20001/errors/errors_test.go
File errors/errors_test.go (right):

https://codereview.appspot.com/87560043/diff/20001/errors/errors_test.go#newc...
errors/errors_test.go:94: c.Check(message, gc.Equals, "<nil>")
On 2014/04/17 14:44:40, rog wrote:
> neither of the two checks above are testing anything apart from what's in the
> test.
> 
> it seems a bit odd that what we check is depending on the
> err value, not our expected value.
> 
> i'd expect to see something more like this:
> 
> func checkErrorMatches(c *gc.C, err error, message string) {
>     if message == "" {
>         c.Assert(err, gc.Equals, nil)
>     } else {
>         c.Assert(err, gc.ErrorMatches, message)
>     }
> }

Done similarly: kept "<nil>" as a special message value to distinguish between
"empty message, but err != nil" and "empty message, err == nil". Also improved
and further simplified all 3 test cases.

https://codereview.appspot.com/87560043/diff/20001/errors/errors_test.go#newc...
errors/errors_test.go:131: mustNotSatisfy(c, t.err, t.errInfo)
On 2014/04/17 14:44:40, rog wrote:
> isn't this checked below?

It is actually, dropped.