Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Create errors package #548

Merged
merged 2 commits into from
Aug 26, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 90 additions & 0 deletions errors/defraError.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
// Copyright 2022 Democratized Data Foundation
//
// Use of this software is governed by the Business Source License
// included in the file licenses/BSL.txt.
//
// As of the Change Date specified in that file, in accordance with
// the Business Source License, use of this software will be governed
// by the Apache License, Version 2.0, included in the file
// licenses/APL.txt.

package errors

import (
"errors"
"fmt"
"io"
"strings"
)

const StackKey string = "Stack"

var (
_ error = (*defraError)(nil)
_ fmt.Formatter = (*defraError)(nil)
)

type defraError struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: why not implement func (e *defraError) As(target any) bool as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What behaviour are you looking to gain by doing so?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well essentially to not change too much the existing codebase by using our errors pkg. in my understanding, when we do importing our errors pkg instead of the errors one - everywhere we use errors.Is breaks. we can import both libs but might as well just import one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

everywhere we use errors.Is breaks

This is incorrect as far as I can see, errors.Is should not break, and is even used by the tests for this package - assert.ErrorIs uses/calls errors.Is

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to be a bit more clear: using errors.Is() would only break when import our library as errors because don't implement the function, so that means when we do use our errors in the rest of the codebase we will have to import both stdlib errors and our errors pkg

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you mean because of the package name clash? Is easy to solve by aliasing the import. I'm really not looking to replace the errors.Is function in this PR (and am doubtful as to whether we ever should)

message string
inner error
stacktrace string
kvs []KV
}

func (e *defraError) Error() string {
builder := strings.Builder{}
builder.WriteString(e.message)
if len(e.kvs) > 0 {
builder.WriteString(".")
}

for i, kv := range e.kvs {
builder.WriteString(" ")
builder.WriteString(kv.key)
builder.WriteString(": ")
builder.WriteString(fmt.Sprint(kv.value))
if i < len(e.kvs)-1 {
builder.WriteString(",")
}
}

return builder.String()
}

func (e *defraError) Is(other error) bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion(unsure): additionally implement a func Is(err, target error) bool to facilitate porting from stdlib to use defra errors pkg.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that would potentially allow to avoid import both defra errors and stdlib errors

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am doubtful as to whether that would be worth the time/effort, and I consider it out of scope for this PR. Maybe something to consider in the future though

switch otherTyped := other.(type) { //nolint:errorlint
case *defraError:
return e.message == otherTyped.message
default:
otherString := other.Error()
return e.message == otherString || e.Error() == otherString || errors.Is(e.inner, other)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: because this is extra and different behavior compared above the stdlib - i.e. the stdlib isn't comparing error strings like this - documenting the Is(...) func would be beneficial

Copy link
Contributor Author

@AndrewSisley AndrewSisley Aug 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the standard lib comparing if not the error string? Will look into

  • maybe doc?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stdlib does essentially compare strings when dealing with errors created by errors.New - leaving as is

}
}

func (e *defraError) Unwrap() error {
return e.inner
}

// Format writes the error into the given state.
//
// Currently the following runes are supported: `v[+]` (+ also writes out the stacktrace), `s`, `q`.
func (e *defraError) Format(f fmt.State, verb rune) {
errorString := e.Error()
switch verb {
case 'v':
_, _ = io.WriteString(f, errorString)

if f.Flag('+') {
if len(errorString) > 0 && errorString[len(errorString)-1] != '.' {
_, _ = io.WriteString(f, ".")
}

_, _ = io.WriteString(f, " "+StackKey+": ")
_, _ = io.WriteString(f, e.stacktrace)
}
case 's':
_, _ = io.WriteString(f, errorString)
case 'q':
_, _ = fmt.Fprintf(f, "%q", errorString)
}
}
79 changes: 79 additions & 0 deletions errors/errors.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
// Copyright 2022 Democratized Data Foundation
//
// Use of this software is governed by the Business Source License
// included in the file licenses/BSL.txt.
//
// As of the Change Date specified in that file, in accordance with
// the Business Source License, use of this software will be governed
// by the Apache License, Version 2.0, included in the file
// licenses/APL.txt.

package errors

import (
"bytes"
"runtime"

goErrors "github.com/go-errors/errors"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: We can probably handle what this does ourselves and get rid of the dependency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did that originally, but I had to plagiarize 50 or so lines, was a surprising amount of effort involved in getting a callstack in Go lol

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

general thought: seems like good practice to note why a dependency is introduced in a PR, in the PR text or comment.

)

// todo: make this configurable:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: in-code todos like this seems to be a natural occurence, and I'm fine with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We agreed to only allow todos if linked to a ticket - isn't too bad I think :)

// https://github.com/sourcenetwork/defradb/issues/733
const MaxStackDepth int = 50

type KV struct {
key string
value interface{}
}

func NewKV(key string, value interface{}) KV {
return KV{
key: key,
value: value,
}
}

// New creates a new Defra error, suffixing any key-value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: can be on one line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving as is - doesn't matter

// pairs provided.
//
// A stacktrace will be yielded if formatting with a `+`, e.g `fmt.Sprintf("%+v", err)`.
func New(message string, keyvals ...KV) error {
return newError(message, keyvals...)
}

// Wrap creates a new error of the given message that contains
// the given inner error, suffixing any key-value pairs provided.
func Wrap(message string, inner error, keyvals ...KV) error {
err := newError(message, keyvals...)
err.inner = inner
return err
}

func newError(message string, keyvals ...KV) *defraError {
shahzadlone marked this conversation as resolved.
Show resolved Hide resolved
return withStackTrace(message, keyvals...)
}

func withStackTrace(message string, keyvals ...KV) *defraError {
stackBuffer := make([]uintptr, MaxStackDepth)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: which "error generation library" is it from? It still is unclear where the 4 is coming from.

Copy link
Contributor Author

@AndrewSisley AndrewSisley Aug 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This library. I'll adjust the comment.

  • Improve wording

// Skip the first X frames as they are part of this library (and dependencies) and are
// best hidden.
length := runtime.Callers(4, stackBuffer[:])
stack := stackBuffer[:length]
stackText := toString(stack)

return &defraError{
message: message,
stacktrace: stackText,
kvs: keyvals,
}
}

func toString(stack []uintptr) string {
buf := bytes.Buffer{}

for _, pc := range stack {
frame := goErrors.NewStackFrame(pc)
buf.WriteString(frame.String())
}
return buf.String()
}
168 changes: 168 additions & 0 deletions errors/errors_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
// Copyright 2022 Democratized Data Foundation
//
// Use of this software is governed by the Business Source License
// included in the file licenses/BSL.txt.
//
// As of the Change Date specified in that file, in accordance with
// the Business Source License, use of this software will be governed
// by the Apache License, Version 2.0, included in the file
// licenses/APL.txt.

package errors

import (
"errors"
"fmt"
"testing"

"github.com/stretchr/testify/assert"
)

func TestErrorIs(t *testing.T) {
const errorMessage string = "gndjdhs"

err := New(errorMessage)

assert.ErrorIs(t, err, errors.New(errorMessage))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from stdlib:

Each call to New returns a distinct error value even if the text is identical.

https://pkg.go.dev/errors

}

func TestErrorIsDefraError(t *testing.T) {
const errorMessage string = "gndjdhs"

err := New(errorMessage)

assert.ErrorIs(t, err, New(errorMessage))
}

func TestErrorWrap(t *testing.T) {
const errorMessage1 string = "gndjdhs"
const errorMessage2 string = "nhdfbgshna"

err1 := New(errorMessage1)
err2 := Wrap(errorMessage2, err1)

assert.ErrorIs(t, err2, errors.New(errorMessage1))
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Add a test using a chain of at least 2 defra errors

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TestErrorWrap tests this

func TestErrorUnwrap(t *testing.T) {
const errorMessage1 string = "gndjdhs"
const errorMessage2 string = "nhdfbgshna"

err1 := New(errorMessage1)
err2 := Wrap(errorMessage2, err1)

unwrapped := errors.Unwrap(err2)

assert.ErrorIs(t, unwrapped, errors.New(errorMessage1))
}

func TestErrorAs(t *testing.T) {
const errorMessage1 string = "gndjdhs"
const errorMessage2 string = "nhdfbgshna"

err1 := New(errorMessage1)
err2 := fmt.Errorf("%s: %w", errorMessage2, err1)

target := &defraError{}
isErr1 := errors.As(err2, &target)

assert.True(t, isErr1)
assert.ErrorIs(t, target, errors.New(errorMessage1))
}

func TestErrorFmts(t *testing.T) {
const errorMessage string = "gndjdhs"

err := New(errorMessage)
result := fmt.Sprintf("%s", err)

assert.Equal(t, errorMessage, result)
}

func TestErrorFmtq(t *testing.T) {
const errorMessage string = "gndjdhs"

err := New(errorMessage)
result := fmt.Sprintf("%q", err)

assert.Equal(t, "\""+errorMessage+"\"", result)
}

func TestErrorFmtvWithoutStacktrace(t *testing.T) {
const errorMessage string = "gndjdhs"

err := New(errorMessage)
result := fmt.Sprintf("%v", err)

assert.Equal(t, errorMessage, result)
}

func TestErrorFmtsWithKvp(t *testing.T) {
const errorMessage string = "gndjdhs"

err := New(errorMessage, NewKV("Kv1", 1))
result := fmt.Sprintf("%s", err)

assert.Equal(t, errorMessage+". Kv1: 1", result)
}

func TestErrorFmtsWithManyKvps(t *testing.T) {
const errorMessage string = "gndjdhs"

err := New(errorMessage, NewKV("Kv1", 1), NewKV("Kv2", "2"))
result := fmt.Sprintf("%s", err)

assert.Equal(t, errorMessage+". Kv1: 1, Kv2: 2", result)
}

func TestErrorFmtvWithStacktrace(t *testing.T) {
const errorMessage string = "gndjdhs"

err := New(errorMessage)
result := fmt.Sprintf("%+v", err)

/*
The Go test flag `-race` messes with the stacktrace causing this function's frame to be ommited from
the stacktrace, as our CI runs with the `-race` flag, these assertions need to be disabled.

// Assert that the first line starts with the error message and contains this [test] function's stacktrace-line -
// including file, line number, and function reference. An exact string match should not be used as the stacktrace
// is machine dependent.
assert.Regexp(t, fmt.Sprintf("^%s\\. Stack: .*\\/defradb\\/errors\\/errors_test\\.go:[0-9]+ \\([a-zA-Z0-9]*\\)", errorMessage), result)
// Assert that the error contains this function's name, and a print-out of the generating line.
assert.Regexp(t, "TestErrorFmtvWithStacktrace: err := Error\\(errorMessage\\)", result)
*/
Comment on lines +124 to +134
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: Why not use go's build tag to turn race off for these assertions or if that's not possible then this file? I think it was something like this:

// +build !race

But never used it before, so not a 100% if it works properly, just an idea.

Copy link
Contributor Author

@AndrewSisley AndrewSisley Aug 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I searched for a way of doing this and couldn't find one, will try, as it is nice if these assertions run otherwise

  • Try comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I quickly looked into this and it seems highly unlikely that -race affects compilation/would not be accessible via build tags. Build tags also can only be applied to the entire file, which would mean breaking up these tests into two files (plus the original shared), which on it's own would probably not be worth it. Leaving as it, but thanks for letting me such a thing exists in go, even if it seems somewhat error prone lol:

To distinguish build constraints from package documentation, a build constraint should be followed by a blank line.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If all it takes is splitting these tests into two files and in return it would let us test these assertions, IMO is still worth it. The final decision is up to your discretion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be 3 files - shared, race, and non-race. But I think we'd need to create our own build flags for the CI to use and -race would still fail if you don't set the custom flags. Leaving as is

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case agree with omitting.


// As noted above, we cannot assert that this function's stack frame is included in the trace,
// however we should still assert that the error message is present.
assert.Regexp(t, fmt.Sprintf("^%s\\. Stack: ", errorMessage), result)

// Assert that the next line of the stacktrace is also present.
assert.Regexp(t, ".*\\/testing/testing.go:[0-9]+ \\([a-zA-Z0-9]*\\)", result)
}

func TestErrorFmtvWithStacktraceAndKvps(t *testing.T) {
const errorMessage string = "gndjdhs"

err := New(errorMessage, NewKV("Kv1", 1), NewKV("Kv2", "2"))
result := fmt.Sprintf("%+v", err)

/*
The Go test flag `-race` messes with the stacktrace causing this function's frame to be ommited from
the stacktrace, as our CI runs with the `-race` flag, these assertions need to be disabled.

// Assert that the first line starts with the error message and contains this [test] function's stacktrace-line -
// including file, line number, and function reference. An exact string match should not be used as the stacktrace
// is machine dependent.
assert.Regexp(t, fmt.Sprintf("^%s\\. Kv1: 1, Kv2: 2\\. Stack: .*\\/defradb\\/errors\\/errors_test\\.go:[0-9]+ \\([a-zA-Z0-9]*\\)", errorMessage), result)
// Assert that the error contains this function's name, and a print-out of the generating line.
assert.Regexp(t, "TestErrorFmtvWithStacktraceAndKvps: err := Error\\(errorMessage\\)", result)
*/

// As noted above, we cannot assert that this function's stack frame is included in the trace,
// however we should still assert that the error message is present.
assert.Regexp(t, fmt.Sprintf("^%s\\. Kv1: 1, Kv2: 2\\. Stack: ", errorMessage), result)

// Assert that the next line of the stacktrace is also present.
assert.Regexp(t, ".*\\/testing/testing.go:[0-9]+ \\([a-zA-Z0-9]*\\)", result)
}
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ require (
github.com/fxamacker/cbor/v2 v2.4.0
github.com/go-chi/chi/v5 v5.0.7
github.com/go-chi/cors v1.2.1
github.com/go-errors/errors v1.0.1
github.com/gogo/protobuf v1.3.2
github.com/graphql-go/graphql v0.8.0
github.com/hsanjuan/ipfs-lite v1.4.1
Expand Down
3 changes: 3 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,10 @@ github.com/go-chi/chi/v5 v5.0.7/go.mod h1:DslCQbL2OYiznFReuXYUmQ2hGd1aDpCnlMNITL
github.com/go-chi/cors v1.2.1 h1:xEC8UT3Rlp2QuWNEr4Fs/c2EAGVKBwy/1vHx3bppil4=
github.com/go-chi/cors v1.2.1/go.mod h1:sSbTewc+6wYHBBCW7ytsFSn836hqM7JxpglAy2Vzc58=
github.com/go-delve/delve v1.5.0/go.mod h1:c6b3a1Gry6x8a4LGCe/CWzrocrfaHvkUxCj3k4bvSUQ=
github.com/go-errors/errors v1.0.1 h1:LUHzmkK3GUKUrL/1gfBUxAHzcev3apQlezX/+O7ma6w=
github.com/go-errors/errors v1.0.1/go.mod h1:f4zRHt4oKfwPJE5k8C9vpYG+aDHdBFUsgrm6/TyX73Q=
github.com/go-errors/errors v1.4.2 h1:J6MZopCL4uSllY1OfXM374weqZFFItUbrImctkmUxIA=
github.com/go-errors/errors v1.4.2/go.mod h1:sIVyrIiJhuEF+Pj9Ebtd6P/rEYROXFi3BopGUQ5a5Og=
github.com/go-gl/glfw v0.0.0-20190409004039-e6da0acd62b1/go.mod h1:vR7hzQXu2zJy9AVAgeJqvqgH9Q5CA+iKCZ2gyEVpxRU=
github.com/go-gl/glfw/v3.3/glfw v0.0.0-20191125211704-12ad95a8df72/go.mod h1:tQ2UAYgL5IevRw8kRxooKSPJfGvJ9fJQFa0TUsXzTg8=
github.com/go-gl/glfw/v3.3/glfw v0.0.0-20200222043503-6f7a984d4dc4/go.mod h1:tQ2UAYgL5IevRw8kRxooKSPJfGvJ9fJQFa0TUsXzTg8=
Expand Down