Repository: scionproto/scion (go)

Issue:
serrors isn't treating by value or by reference errors consistently
serrors.New returns a pointer. all the wrapper constructors return values.
Is() expects a value. It will fail to recognize a pointer as a basicError. However, if given a value, it will panic for any error that happens to be a wrapper around basicErrors, as it will try to compare these objects which are not comparable.

WithCtx() expects only a value and will fail to downcast a basicError ptr, resulting in always wrapping instead of merging ctx.
A number of test cases have been crafted to expect that un-planned behaviour. For example private/mgmtapi/cppki/api/api_test.go expect a wrapped error while serror() was designed to produce the same error in merged form. (i.e. fixing serrors) will break those tests.

Either we fix serrors and the tests or, may be simpler, we remove that accidentally unused merging mechanism from serrors.

In either case we should decide if we want serrrors to handle values, or references, or both and implement whatever we chose consistently. I suspect we really want references since that's what we always use (otherwise Is() would be panicking).


Pull request description:
serrors: delete error context merging and code cleanup
There were a number of intertwined issues difficult to characterize in isolation.
This proposes something that has a more consistent behavior and interface, with shims to ease transition.

Most issues stemmed from the withCtx() function which (in rare cases) created hybrid objects that required complicated support code and, in some cases could cause Is() to panic.

Removed the feature, replaced WithCtx() with a shim that doesn't create hybrids. Removed all the complicated code that resulted. The code is now more obvious but not shorter. There are now two subclasses of errors:
* basicError: dedicated to errors of the form string+cause - Constructed by Wrap()
* joinedError: dedicated to errors of the form error+cause - Constructed by Join()
This allows for Is() and Unwrap() to be different and do the right thing in all cases.

Introduced new constructors with obvious semantics to replace those already marked deprecated.

I still plan on editing the calls to deprecated constructors (and common.ErrMsg) out of everywhere, but in a subsequent PR.

Fixes #4486


Patch:
diff --git a/pkg/private/common/errors.go b/pkg/private/common/errors.go
index 7743cbe12..f932d0f24 100644
--- a/pkg/private/common/errors.go
+++ b/pkg/private/common/errors.go
@@ -15,8 +15,8 @@
 
 package common
 
-// ErrMsg should be used for error string constants. The constant can then be
-// used for Is checking in the calling code.
+// Deprecated: ErrMsg is not useful. It mimics errors.New() but fails to return a singleton. Just
+// use errors.New() if you need cheap sentinel errors.
 type ErrMsg string
 
 func (e ErrMsg) Error() string {
diff --git a/pkg/private/serrors/errors.go b/pkg/private/serrors/errors.go
index 012eb24f4..c913183b6 100644
--- a/pkg/private/serrors/errors.go
+++ b/pkg/private/serrors/errors.go
@@ -34,43 +34,24 @@ import (
 	"go.uber.org/zap/zapcore"
 )
 
-type errOrMsg struct {
-	str string
-	err error
-}
-
-func (m errOrMsg) Error() string {
-	if m.err != nil {
-		return m.err.Error()
-	}
-	return m.str
-}
-
-func (m errOrMsg) addToEncoder(enc zapcore.ObjectEncoder) error {
-	if m.err != nil {
-		if marshaler, ok := m.err.(zapcore.ObjectMarshaler); ok {
-			return enc.AddObject("msg", marshaler)
-		}
-		enc.AddString("msg", m.err.Error())
-		return nil
-	}
-	enc.AddString("msg", m.str)
-	return nil
+// ctxPair is one item of context info.
+type ctxPair struct {
+	Key   string
+	Value interface{}
 }
 
-type basicError struct {
-	msg    errOrMsg
-	fields map[string]interface{}
-	cause  error
-	stack  *stack
+// errorInfo is a base class for two implementations of error: basicError and joinedError.
+type errorInfo struct {
+	ctx   *[]ctxPair
+	cause error
+	stack *stack
 }
 
-func (e basicError) Error() string {
+func (e errorInfo) error() string {
 	var buf bytes.Buffer
-	buf.WriteString(e.msg.Error())
-	if len(e.fields) != 0 {
+	if len(*e.ctx) != 0 {
 		fmt.Fprint(&buf, " ")
-		encodeContext(&buf, e.ctxPairs())
+		encodeContext(&buf, *e.ctx)
 	}
 	if e.cause != nil {
 		fmt.Fprintf(&buf, ": %s", e.cause)
@@ -80,10 +61,7 @@ func (e basicError) Error() string {
 
 // MarshalLogObject implements zapcore.ObjectMarshaler to have a nicer log
 // representation.
-func (e basicError) MarshalLogObject(enc zapcore.ObjectEncoder) error {
-	if err := e.msg.addToEncoder(enc); err != nil {
-		return err
-	}
+func (e errorInfo) marshalLogObject(enc zapcore.ObjectEncoder) error {
 	if e.cause != nil {
 		if m, ok := e.cause.(zapcore.ObjectMarshaler); ok {
 			if err := enc.AddObject("cause", m); err != nil {
@@ -98,54 +76,20 @@ func (e basicError) MarshalLogObject(enc zapcore.ObjectEncoder) error {
 			return err
 		}
 	}
-	for k, v := range e.fields {
-		zap.Any(k, v).AddTo(enc)
+	for _, pair := range *e.ctx {
+		zap.Any(pair.Key, pair.Value).AddTo(enc)
 	}
 	return nil
 }
 
-func (e basicError) Is(err error) bool {
-	switch other := err.(type) {
-	case basicError:
-		return e.msg == other.msg
-	default:
-		if e.msg.err != nil {
-			return e.msg.err == err
-		}
-		return false
-	}
-}
-
-func (e basicError) As(as interface{}) bool {
-	if e.msg.err != nil {
-		return errors.As(e.msg.err, as)
-	}
-	return false
-}
-
-func (e basicError) Unwrap() error {
-	return e.cause
-}
-
 // StackTrace returns the attached stack trace if there is any.
-func (e basicError) StackTrace() StackTrace {
+func (e errorInfo) StackTrace() StackTrace {
 	if e.stack == nil {
 		return nil
 	}
 	return e.stack.StackTrace()
 }
 
-func (e basicError) ctxPairs() []ctxPair {
-	fields := make([]ctxPair, 0, len(e.fields))
-	for k, v := range e.fields {
-		fields = append(fields, ctxPair{Key: k, Value: v})
-	}
-	sort.Slice(fields, func(i, j int) bool {
-		return fields[i].Key < fields[j].Key
-	})
-	return fields
-}
-
 // IsTimeout returns whether err is or is caused by a timeout error.
 func IsTimeout(err error) bool {
 	var t interface{ Timeout() bool }
@@ -158,75 +102,196 @@ func IsTemporary(err error) bool {
 	return errors.As(err, &t) && t.Temporary()
 }
 
-// WithCtx returns an error that is the same as the given error but contains the
-// additional context. The additional context is printed in the Error method.
-// The returned error implements Is and Is(err) returns true.
-// Deprecated: use WrapStr or New instead.
-func WithCtx(err error, errCtx ...interface{}) error {
-	if top, ok := err.(basicError); ok {
-		return basicError{
-			msg:    top.msg,
-			fields: combineFields(top.fields, errCtxToFields(errCtx)),
-			cause:  top.cause,
-			stack:  top.stack,
-		}
+func mkErrorInfo(cause error, addStack bool, errCtx ...interface{}) errorInfo {
+	np := len(errCtx) / 2
+	ctx := make([]ctxPair, np)
+	for i := 0; i < np; i++ {
+		k := errCtx[2*i]
+		v := errCtx[2*i+1]
+		ctx[i] = ctxPair{Key: fmt.Sprint(k), Value: v}
+	}
+	sort.Slice(ctx, func(a, b int) bool {
+		return ctx[a].Key < ctx[b].Key
+	})
+
+	r := errorInfo{
+		cause: cause,
+		ctx:   &ctx,
+	}
+	if !addStack {
+		return r
+	}
+
+	var (
+		t1 basicError
+		t2 *basicError
+		t3 joinedError
+		t4 *joinedError
+	)
+
+	// We attach a stacktrace if there is no basic error cause already. Note that if the innermost
+	// basicError was without a stack trace, then there'll never be one. That's to avoid looking
+	// for it in every level or every constructor. TB revisisted if necessary.
+	// TODO(jiceatscion): should we define a "stackertracer" interface?
+	if r.cause == nil || !(errors.As(cause, &t1) || errors.As(cause, &t2) ||
+		errors.As(cause, &t3) || errors.As(cause, &t4)) {
+
+		r.stack = callers()
 	}
+	return r
+}
+
+// basicError is an implementation of error that encapsulates various pieces of information besides
+// a message. The msg field is strictly a string.
+type basicError struct {
+	errorInfo
+	msg string
+}
+
+func (e basicError) Error() string {
+	var buf bytes.Buffer
+	buf.WriteString(e.msg)
+	buf.WriteString(e.errorInfo.error())
+	return buf.String()
+}
+
+func (e basicError) Unwrap() error {
+	return e.cause
+}
 
+// MarshalLogObject implements zapcore.ObjectMarshaler to have a nicer log
+// representation.
+func (e basicError) MarshalLogObject(enc zapcore.ObjectEncoder) error {
+	enc.AddString("msg", e.msg)
+	return e.errorInfo.marshalLogObject(enc)
+}
+
+// WrapStr returns an error that associates the given error, with the given cause (an underlying
+// error) unless nil, and the given context.
+//
+// A stack dump is added unless cause is a basicError (in which case it is assumed to contain a
+// stack dump).
+//
+// The returned error supports Is. Is(cause)returns true.
+//
+// WrapStr will soon be renamed Wrap, once the name Wrap becomes available. Until then it is ok to
+// use wrapStr().
+func WrapStr(msg string, cause error, errCtx ...interface{}) error {
 	return basicError{
-		msg:    errOrMsg{err: err},
-		fields: errCtxToFields(errCtx),
+		errorInfo: mkErrorInfo(cause, true, errCtx...),
+		msg:       msg,
 	}
 }
 
-// Wrap wraps the cause with the msg error and adds context to the resulting
-// error. The returned error implements Is and Is(msg) and Is(cause) returns
-// true.
-// Deprecated: use WrapStr instead.
-func Wrap(msg, cause error, errCtx ...interface{}) error {
+// WrapStrNoStack returns an error that associates the given error, with the given cause
+// (an underlying error) unless nil, and the given context. A stack dump is not added.
+// if cause is a basicError that contains a stack dump, that stack dump is preserved. The returned
+// error supports Is. Is(cause) returns true.
+func WrapStrNoStack(msg string, cause error, errCtx ...interface{}) error {
 	return basicError{
-		msg:    errOrMsg{err: msg},
-		cause:  cause,
-		fields: errCtxToFields(errCtx),
+		errorInfo: mkErrorInfo(cause, false, errCtx...),
+		msg:       msg,
 	}
 }
 
-// WrapStr wraps the cause with an error that has msg in the error message and
-// adds the additional context. The returned error implements Is and Is(cause)
-// returns true.
-func WrapStr(msg string, cause error, errCtx ...interface{}) error {
-	var (
-		existingVal basicError
-		existingPtr *basicError
-		st          *stack
-	)
+// New creates a new basicError with the given message and context, plus a stack dump.
+// It returns a pointer as the underlying type of the error interface object.
+// Avoid using this in performance-critical code: it is the most expensive variant. If used to
+// construct other errors, such as with Join, the embedded stack trace and context serve no
+// purpose. Therefore, to make sentinel errors, errors.New() should be preferred.
+func New(msg string, errCtx ...interface{}) error {
+	return &basicError{
+		errorInfo: mkErrorInfo(nil, true, errCtx...),
+		msg:       msg,
+	}
+}
+
+// joinedError is an implementation of error that aggregates various pieces of information, around
+// an existing error, the base error (for example a unique sentinel error). The base error isn't
+// assumed to be of any particular implementation.
+type joinedError struct {
+	errorInfo
+	error error
+}
+
+func (e joinedError) Error() string {
+	var buf bytes.Buffer
+	buf.WriteString(e.error.Error())
+	buf.WriteString(e.errorInfo.error())
+	return buf.String()
+}
+
+func (e joinedError) Unwrap() []error {
+	return []error{e.error, e.cause}
+}
+
+// MarshalLogObject implements zapcore.ObjectMarshaler to have a nicer log
+// representation. The base error is not dissected. It is treated as a most generic error.
+func (e joinedError) MarshalLogObject(enc zapcore.ObjectEncoder) error {
+	enc.AddString("msg", e.error.Error())
+	return e.errorInfo.marshalLogObject(enc)
+}
 
-	// We attach a stacktrace if there is no basic error already.
-	if !errors.As(cause, &existingVal) && !errors.As(cause, &existingPtr) {
-		st = callers()
+// Join returns an error that associates the given error, with the given cause (an underlying error)
+// unless nil, and the given context.
+//
+// A stack dump is added unless cause is a basicError (in which case it is assume to contain a stack
+// dump).
+//
+// The returned error supports Is. If cause isn't nil, Is(cause) returns true. Is(error) returns
+// true.
+func Join(err, cause error, errCtx ...interface{}) error {
+	if err == nil && cause == nil {
+		// Pointless. Will not. Also, maintaining backward compatibility with
+		// a previous Join function.
+		return nil
 	}
-	return basicError{
-		msg:    errOrMsg{str: msg},
-		cause:  cause,
-		fields: errCtxToFields(errCtx),
-		stack:  st,
+	return joinedError{
+		errorInfo: mkErrorInfo(cause, true, errCtx...),
+		error:     err,
 	}
 }
 
-// New creates a new error with the given message and context.
-func New(msg string, errCtx ...interface{}) error {
-	if len(errCtx) == 0 {
-		return &basicError{
-			msg:   errOrMsg{str: msg},
-			stack: callers(),
-		}
+// JoinNoStack returns an error that associates the given error, with the given cause
+// (an underlying error) unless nil, and the given context.
+//
+// A stack dump is not added. If cause is a basicError and contain a stack dump. That stack dump is
+// preserved.
+//
+// The returned error supports Is. If cause isn't nil, Is(cause) returns true. Is(error) returns
+// true.
+func JoinNoStack(err, cause error, errCtx ...interface{}) error {
+	if err == nil && cause == nil {
+		// Pointless. Will not.
+		return nil
 	}
-	return &basicError{
-		msg:    errOrMsg{str: msg},
-		fields: errCtxToFields(errCtx),
-		stack:  callers(),
+	return joinedError{
+		errorInfo: mkErrorInfo(cause, false, errCtx...),
+		error:     err,
 	}
 }
 
+// Deprecated: WithCtx should never have existed. Depending on intent, use:
+//   - Join(err, nil, errCtx): If err has no context to preserve.
+//   - WrapStr("some error with context added", err, errctx): if err is an error with error
+//     information that needs to be preserved.
+//
+// This shim does the latter for you for the time being.
+//
+// WithCtx used to attempt the merger of the given error into the newly created one with
+// semantically incorrect results. That feature is gone and the results differ only slightly in the
+// formated string output. WithCtx didn't try to add a stack, so this shim doesn't either.
+func WithCtx(err error, errCtx ...interface{}) error {
+	return WrapStrNoStack("error", err, errCtx...)
+}
+
+// Deprecated: Wrap has been renamed Join. Join and the historical Wrap do differ very slightly:
+// any stack dump that might have be attached to err is ignored when logging. Like before, no stack
+// dump is added to the returned error.
+func Wrap(err, cause error, errCtx ...interface{}) error {
+	return JoinNoStack(err, cause, errCtx...)
+}
+
 // List is a slice of errors.
 type List []error
 
@@ -262,55 +327,6 @@ func (e List) MarshalLogArray(ae zapcore.ArrayEncoder) error {
 	return nil
 }
 
-// Join returns an error that wraps the given errors in a List error.
-// Any nil error values are discarded.
-// Join returns nil if errs contains no non-nil values.
-func Join(errs ...error) error {
-	n := 0
-	for _, err := range errs {
-		if err != nil {
-			n++
-		}
-	}
-	if n == 0 {
-		return nil
-	}
-	l := make(List, 0, n)
-	for _, err := range errs {
-		if err != nil {
-			l = append(l, err)
-		}
-	}
-	return l
-}
-
-func errCtxToFields(errCtx []interface{}) map[string]interface{} {
-	if len(errCtx) == 0 {
-		return nil
-	}
-	fields := make(map[string]interface{}, len(errCtx)/2)
-	for i := 0; i < len(errCtx)-1; i += 2 {
-		fields[fmt.Sprint(errCtx[i])] = errCtx[i+1]
-	}
-	return fields
-}
-
-func combineFields(a, b map[string]interface{}) map[string]interface{} {
-	fields := make(map[string]interface{}, len(a)+len(b))
-	for k, v := range a {
-		fields[k] = v
-	}
-	for k, v := range b {
-		fields[k] = v
-	}
-	return fields
-}
-
-type ctxPair struct {
-	Key   string
-	Value interface{}
-}
-
 func encodeContext(buf io.Writer, pairs []ctxPair) {
 	fmt.Fprint(buf, "{")
 	for i, p := range pairs {
diff --git a/pkg/private/serrors/stack.go b/pkg/private/serrors/stack.go
index b8a4f843a..7cb19bcd3 100644
--- a/pkg/private/serrors/stack.go
+++ b/pkg/private/serrors/stack.go
@@ -188,9 +188,10 @@ func (s *stack) StackTrace() StackTrace {
 }
 
 func callers() *stack {
+	// We skip 4 boilerplate frames: Ctor()->mkErrorInfo()->callers()->Callers()
 	const depth = 32
 	var pcs [depth]uintptr
-	n := runtime.Callers(3, pcs[:])
+	n := runtime.Callers(4, pcs[:])
 	var st stack = pcs[0 : n-1] // skip bottom runtime.goexit at bottom of each stack
 	return &st
 }
diff --git a/private/mgmtapi/segments/api/api.go b/private/mgmtapi/segments/api/api.go
index 2bb470001..7a1b48f79 100644
--- a/private/mgmtapi/segments/api/api.go
+++ b/private/mgmtapi/segments/api/api.go
@@ -50,14 +50,14 @@ func (s *Server) GetSegments(w http.ResponseWriter, r *http.Request, params GetS
 		if ia, err := addr.ParseIA(*params.StartIsdAs); err == nil {
 			q.StartsAt = []addr.IA{ia}
 		} else {
-			errs = append(errs, serrors.WithCtx(err, "parameter", "start_isd_as"))
+			errs = append(errs, serrors.WrapStr("invalid start ISD_AS", err))
 		}
 	}
 	if params.EndIsdAs != nil {
 		if ia, err := addr.ParseIA(*params.EndIsdAs); err == nil {
 			q.EndsAt = []addr.IA{ia}
 		} else {
-			errs = append(errs, serrors.WithCtx(err, "parameter", "end_isd_as"))
+			errs = append(errs, serrors.WrapStr("invalid end ISD_AS", err))
 		}
 	}
 	if err := errs.ToError(); err != nil {


Test patch:
diff --git a/pkg/private/serrors/errors_test.go b/pkg/private/serrors/errors_test.go
index 803cd8c42..9f0443fc0 100644
--- a/pkg/private/serrors/errors_test.go
+++ b/pkg/private/serrors/errors_test.go
@@ -219,6 +219,9 @@ func TestEncoding(t *testing.T) {
 			goldenFileBase: "testdata/wrapped-error",
 		},
 		"error with context": {
+			// WithCtx is deprecated. The shim does it the new way: sets err as the cause of a new
+			// error. The string output isn't exactly the same. Which almost nothing ever notices...
+			// except this test.
 			err:            serrors.WithCtx(serrors.New("simple err"), "someCtx", "someValue"),
 			goldenFileBase: "testdata/error-with-context",
 		},
@@ -281,35 +284,9 @@ func TestList(t *testing.T) {
 }
 
 func TestJoinNil(t *testing.T) {
-	assert.Nil(t, serrors.Join())
-	assert.Nil(t, serrors.Join(nil))
 	assert.Nil(t, serrors.Join(nil, nil))
 }
 
-func TestJoin(t *testing.T) {
-	err1 := serrors.New("err1")
-	err2 := serrors.New("err2")
-	for _, test := range []struct {
-		errs []error
-		want serrors.List
-	}{{
-		errs: []error{err1},
-		want: serrors.List{err1},
-	}, {
-		errs: []error{err1},
-		want: serrors.List{err1},
-	}, {
-		errs: []error{err1, err2},
-		want: serrors.List{err1, err2},
-	}, {
-		errs: []error{err1, nil, err2},
-		want: serrors.List{err1, err2},
-	}} {
-		got := serrors.Join(test.errs...)
-		assert.Equal(t, got, test.want)
-	}
-}
-
 func TestAtMostOneStacktrace(t *testing.T) {
 	err := errors.New("core")
 	for i := range [20]int{} {
@@ -353,13 +330,15 @@ func ExampleNew() {
 }
 
 func ExampleWithCtx() {
+	// It is not possible to augment the context of a basicError.
+	// WithCtx turns that into an error with a cause.
 	// ErrBadL4 is an error defined at package scope.
 	var ErrBadL4 = serrors.New("Unsupported L4 protocol")
 	addedCtx := serrors.WithCtx(ErrBadL4, "type", "SCTP")
 
 	fmt.Println(addedCtx)
 	// Output:
-	// Unsupported L4 protocol {type=SCTP}
+	// error {type=SCTP}: Unsupported L4 protocol
 }
 
 func ExampleWrapStr() {
@@ -411,3 +390,15 @@ func sanitizeLog(log []byte) []byte {
 	}
 	return log
 }
+
+func TestUncomparable(t *testing.T) {
+	t.Run("Is", func(t *testing.T) {
+		// We make two wrappers of uncomparable error objects. We could also create custom error
+		// types for the same result, but this is closer to our use cases.
+		errObject := serrors.WrapStr("simple err", nil, "dummy", "context")
+		wrapperA := serrors.Join(errObject, nil, "dummy", "context")
+		wrapperB := serrors.Join(errObject, nil, "dummy", "context")
+		assert.NotErrorIs(t, wrapperA, wrapperB)
+		// no panic
+	})
+}
diff --git a/pkg/private/serrors/testdata/error-list.log b/pkg/private/serrors/testdata/error-list.log
index 83775ffe9..040ccf97f 100644
--- a/pkg/private/serrors/testdata/error-list.log
+++ b/pkg/private/serrors/testdata/error-list.log
@@ -1,1 +1,1 @@
-{"err":[{"ctx1":"val1","msg":"test err","stacktrace":["pkg/private/serrors/go_default_test_test.TestEncoding pkg/private/serrors/errors_test.go:227","testing.tRunner gosdk"]},{"msg":"test err2","stacktrace":["pkg/private/serrors/go_default_test_test.TestEncoding pkg/private/serrors/errors_test.go:228","testing.tRunner gosdk"]}],"level":"info","msg":"Failed to do thing"}
\ No newline at end of file
+{"err":[{"ctx1":"val1","msg":"test err","stacktrace":["pkg/private/serrors/go_default_test_test.TestEncoding pkg/private/serrors/errors_test.go:230","testing.tRunner gosdk"]},{"msg":"test err2","stacktrace":["pkg/private/serrors/go_default_test_test.TestEncoding pkg/private/serrors/errors_test.go:231","testing.tRunner gosdk"]}],"level":"info","msg":"Failed to do thing"}
\ No newline at end of file
diff --git a/pkg/private/serrors/testdata/error-with-context.err b/pkg/private/serrors/testdata/error-with-context.err
index fd33f3ae5..6ff4bd2db 100644
--- a/pkg/private/serrors/testdata/error-with-context.err
+++ b/pkg/private/serrors/testdata/error-with-context.err
@@ -1,1 +1,1 @@
-simple err {someCtx=someValue}
\ No newline at end of file
+error {someCtx=someValue}: simple err
\ No newline at end of file
diff --git a/pkg/private/serrors/testdata/error-with-context.log b/pkg/private/serrors/testdata/error-with-context.log
index d341a8930..7548ec61c 100644
--- a/pkg/private/serrors/testdata/error-with-context.log
+++ b/pkg/private/serrors/testdata/error-with-context.log
@@ -1,1 +1,1 @@
-{"err":{"msg":{"msg":"simple err","stacktrace":["pkg/private/serrors/go_default_test_test.TestEncoding pkg/private/serrors/errors_test.go:222","testing.tRunner gosdk"]},"someCtx":"someValue"},"level":"info","msg":"Failed to do thing"}
\ No newline at end of file
+{"err":{"cause":{"msg":"simple err","stacktrace":["pkg/private/serrors/go_default_test_test.TestEncoding pkg/private/serrors/errors_test.go:225","testing.tRunner gosdk"]},"msg":"error","someCtx":"someValue"},"level":"info","msg":"Failed to do thing"}
\ No newline at end of file
diff --git a/pkg/private/serrors/testdata/goroutine.log b/pkg/private/serrors/testdata/goroutine.log
index b7d254084..c9212c8f0 100644
--- a/pkg/private/serrors/testdata/goroutine.log
+++ b/pkg/private/serrors/testdata/goroutine.log
@@ -1,1 +1,1 @@
-{"err":{"msg":"msg","stacktrace":["pkg/private/serrors/go_default_test_test.TestEncoding.func2.1 pkg/private/serrors/errors_test.go:236"]},"level":"info","msg":"Failed to do thing"}
\ No newline at end of file
+{"err":{"msg":"msg","stacktrace":["pkg/private/serrors/go_default_test_test.TestEncoding.func2.1 pkg/private/serrors/errors_test.go:239"]},"level":"info","msg":"Failed to do thing"}
\ No newline at end of file
diff --git a/pkg/private/serrors/testdata/wrapped-error.log b/pkg/private/serrors/testdata/wrapped-error.log
index d9f2212c1..93a7d47c9 100644
--- a/pkg/private/serrors/testdata/wrapped-error.log
+++ b/pkg/private/serrors/testdata/wrapped-error.log
@@ -1,1 +1,1 @@
-{"err":{"cause":{"msg":"msg cause","stacktrace":["pkg/private/serrors/go_default_test_test.TestEncoding pkg/private/serrors/errors_test.go:215","testing.tRunner gosdk"]},"k0":"v0","k1":1,"msg":{"msg":"msg error","stacktrace":["pkg/private/serrors/go_default_test_test.TestEncoding pkg/private/serrors/errors_test.go:214","testing.tRunner gosdk"]}},"level":"info","msg":"Failed to do thing"}
\ No newline at end of file
+{"err":{"cause":{"msg":"msg cause","stacktrace":["pkg/private/serrors/go_default_test_test.TestEncoding pkg/private/serrors/errors_test.go:215","testing.tRunner gosdk"]},"k0":"v0","k1":1,"msg":"msg error"},"level":"info","msg":"Failed to do thing"}
\ No newline at end of file
diff --git a/private/mgmtapi/cppki/api/testdata/certificates-malformed.json b/private/mgmtapi/cppki/api/testdata/certificates-malformed.json
index 4af2e0a64..1bdf82a99 100644
--- a/private/mgmtapi/cppki/api/testdata/certificates-malformed.json
+++ b/private/mgmtapi/cppki/api/testdata/certificates-malformed.json
@@ -1,5 +1,5 @@
 {
-    "detail": "[ invalid ISD-AS {value=garbage} {parameter=isd_as} ]",
+    "detail": "[ error {parameter=isd_as}: invalid ISD-AS {value=garbage} ]",
     "status": 400,
     "title": "malformed query parameters",
     "type": "/problems/bad-request"
diff --git a/private/mgmtapi/segments/api/testdata/segments-malformed-query.json b/private/mgmtapi/segments/api/testdata/segments-malformed-query.json
index f4c280a5e..facebfce9 100644
--- a/private/mgmtapi/segments/api/testdata/segments-malformed-query.json
+++ b/private/mgmtapi/segments/api/testdata/segments-malformed-query.json
@@ -1,5 +1,5 @@
 {
-    "detail": "[ parsing AS part {index=0; parameter=start_isd_as; value=ff001:0:110}: strconv.ParseUint: parsing \"ff001\": value out of range; parsing AS part {index=0; parameter=end_isd_as; value=ff000:0:112}: strconv.ParseUint: parsing \"ff000\": value out of range ]",
+    "detail": "[ invalid start ISD_AS: parsing AS part {index=0; value=ff001:0:110}: strconv.ParseUint: parsing \"ff001\": value out of range; invalid end ISD_AS: parsing AS part {index=0; value=ff000:0:112}: strconv.ParseUint: parsing \"ff000\": value out of range ]",
     "status": 400,
     "title": "malformed query parameters",
     "type": "/problems/bad-request"