feat(errors): enhance error handling by improving JSON marshaling and logging structure, adding tests for nested errors and failing marshaler scenarios

Author: m-mizutani

error.go

@@ -5,6 +5,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"sort"
 
 	"log/slog"
 )
@@ -93,15 +94,16 @@ func Tags(err error) []string {
 
 // HasTag returns true if the error has the tag.
 func HasTag(err error, tag tag) bool {
-	if e := Unwrap(err); e != nil {
-		return e.HasTag(tag)
-	}
-
-	// Check for Errors type using AsErrors
+	// Check for Errors type first using AsErrors
 	if errs := AsErrors(err); errs != nil {
 		return errs.HasTag(tag)
 	}
 
+	// Check for Error type using Unwrap
+	if e := Unwrap(err); e != nil {
+		return e.HasTag(tag)
+	}
+
 	return false
 }
 
@@ -219,17 +221,34 @@ func (x *Error) Format(s fmt.State, verb rune) {
 			c.st.Format(s, verb)
 			_, _ = io.WriteString(s, "\n")
 
-			if len(x.values) > 0 {
+			// Use merged values from entire error chain
+			mergedValues := x.Values()
+			if len(mergedValues) > 0 {
 				_, _ = io.WriteString(s, "\nValues:\n")
-				for k, v := range x.values {
-					_, _ = io.WriteString(s, fmt.Sprintf("  %s: %v\n", k, v))
+				// Sort keys for predictable output
+				keys := make([]string, 0, len(mergedValues))
+				for k := range mergedValues {
+					keys = append(keys, k)
+				}
+				sort.Strings(keys)
+				for _, k := range keys {
+					_, _ = io.WriteString(s, fmt.Sprintf("  %s: %v\n", k, mergedValues[k]))
 				}
 				_, _ = io.WriteString(s, "\n")
 			}
-			if len(x.typedValues) > 0 {
+
+			// Use merged typed values from entire error chain
+			mergedTypedValues := x.TypedValues()
+			if len(mergedTypedValues) > 0 {
 				_, _ = io.WriteString(s, "\nTyped Values:\n")
-				for k, v := range x.typedValues {
-					_, _ = io.WriteString(s, fmt.Sprintf("  %s: %v\n", k, v))
+				// Sort keys for predictable output
+				keys := make([]string, 0, len(mergedTypedValues))
+				for k := range mergedTypedValues {
+					keys = append(keys, k)
+				}
+				sort.Strings(keys)
+				for _, k := range keys {
+					_, _ = io.WriteString(s, fmt.Sprintf("  %s: %v\n", k, mergedTypedValues[k]))
 				}
 				_, _ = io.WriteString(s, "\n")
 			}
@@ -285,26 +304,12 @@ func (x *Error) Wrap(cause error, options ...Option) *Error {
 
 // Values returns map of key and value that is set by With. All wrapped goerr.Error key and values will be merged. Key and values of wrapped error is overwritten by upper goerr.Error.
 func (x *Error) Values() map[string]any {
-	values := x.mergedValues()
-
-	// Add string-based values
-	for key, value := range x.values {
-		values[key] = value
-	}
-
-	return values
+	return x.mergedValues()
 }
 
 // TypedValues returns map of key and value that is set by TypedValue. All wrapped goerr.Error typed key and values will be merged. Key and values of wrapped error is overwritten by upper goerr.Error.
 func (x *Error) TypedValues() map[string]any {
-	typedValues := x.mergedTypedValues()
-
-	// Add typed values
-	for key, value := range x.typedValues {
-		typedValues[key] = value
-	}
-
-	return typedValues
+	return x.mergedTypedValues()
 }
 
 func (x *Error) mergedValues() values {

error_test.go

@@ -20,7 +20,7 @@ func TestNew(t *testing.T) {
 	// Test with options
 	tag := goerr.NewTag("test")
 	err = goerr.New("test error", goerr.Tag(tag), goerr.Value("key", "value"))
-	
+
 	if !err.HasTag(tag) {
 		t.Error("Error should have the tag")
 	}
@@ -46,7 +46,7 @@ func TestWrap(t *testing.T) {
 
 func TestUnwrap(t *testing.T) {
 	err := goerr.New("test error")
-	
+
 	// Test unwrapping goerr.Error
 	unwrapped := goerr.Unwrap(err)
 	if unwrapped == nil {
@@ -62,7 +62,7 @@ func TestUnwrap(t *testing.T) {
 }
 
 func TestErrorValues(t *testing.T) {
-	err := goerr.New("test error", 
+	err := goerr.New("test error",
 		goerr.Value("key1", "value1"),
 		goerr.Value("key2", 42))
 
@@ -85,7 +85,7 @@ func TestErrorValues(t *testing.T) {
 func TestErrorTags(t *testing.T) {
 	tag1 := goerr.NewTag("tag1")
 	tag2 := goerr.NewTag("tag2")
-	
+
 	err := goerr.New("test error", goerr.Tag(tag1), goerr.Tag(tag2))
 
 	tags := goerr.Tags(err)
@@ -109,7 +109,7 @@ func TestErrorTags(t *testing.T) {
 
 func TestErrorMarshalJSON(t *testing.T) {
 	tag := goerr.NewTag("test")
-	err := goerr.New("test error", 
+	err := goerr.New("test error",
 		goerr.Tag(tag),
 		goerr.Value("key", "value"))
 
@@ -140,7 +140,7 @@ func TestErrorMarshalJSON(t *testing.T) {
 
 func TestErrorLogValue(t *testing.T) {
 	err := goerr.New("test error", goerr.Value("key", "value"))
-	
+
 	logValue := err.LogValue()
 	if logValue.Kind() != slog.KindGroup {
 		t.Error("LogValue should return a group")
@@ -181,9 +181,108 @@ func TestErrorFormat(t *testing.T) {
 	}
 }
 
+func TestErrorFormatWrapped(t *testing.T) {
+	// Create a chain of wrapped errors with different values
+	baseErr := goerr.New("base error", 
+		goerr.Value("base_key", "base_value"),
+		goerr.Value("shared_key", "base_shared")) // This should be overwritten
+		
+	middleErr := goerr.Wrap(baseErr, "middle error", 
+		goerr.Value("middle_key", "middle_value"),
+		goerr.Value("shared_key", "middle_shared")) // This overwrites base_shared
+		
+	topErr := goerr.Wrap(middleErr, "top error", 
+		goerr.Value("top_key", "top_value"),
+		goerr.Value("shared_key", "top_shared")) // This overwrites middle_shared
+
+	// Test %+v format with wrapped errors
+	detailedResult := fmt.Sprintf("%+v", topErr)
+	
+	// Should contain the complete error message chain
+	if !strings.Contains(detailedResult, "top error: middle error: base error") {
+		t.Error("Detailed format should contain complete error message chain")
+	}
+	
+	// Should contain Values section
+	if !strings.Contains(detailedResult, "Values:") {
+		t.Error("Detailed format should contain Values section")
+	}
+	
+	// Should contain values from base error
+	if !strings.Contains(detailedResult, "base_key: base_value") {
+		t.Error("Detailed format should contain values from base error")
+	}
+	
+	// Should contain values from middle error
+	if !strings.Contains(detailedResult, "middle_key: middle_value") {
+		t.Error("Detailed format should contain values from middle error")
+	}
+	
+	// Should contain values from top error
+	if !strings.Contains(detailedResult, "top_key: top_value") {
+		t.Error("Detailed format should contain values from top error")
+	}
+	
+	// Should show the final value for overwritten key (top-level wins)
+	if !strings.Contains(detailedResult, "shared_key: top_shared") {
+		t.Error("Detailed format should show final value for overwritten key")
+	}
+	
+	// Should NOT contain overwritten values
+	if strings.Contains(detailedResult, "base_shared") {
+		t.Error("Detailed format should not contain overwritten base value")
+	}
+	if strings.Contains(detailedResult, "middle_shared") {
+		t.Error("Detailed format should not contain overwritten middle value")
+	}
+	
+	// Verify keys are sorted alphabetically
+	valuesSection := detailedResult[strings.Index(detailedResult, "Values:"):]
+	baseKeyPos := strings.Index(valuesSection, "base_key:")
+	middleKeyPos := strings.Index(valuesSection, "middle_key:")
+	sharedKeyPos := strings.Index(valuesSection, "shared_key:")
+	topKeyPos := strings.Index(valuesSection, "top_key:")
+	
+	// All positions should be found
+	if baseKeyPos == -1 || middleKeyPos == -1 || sharedKeyPos == -1 || topKeyPos == -1 {
+		t.Error("All keys should be present in values section")
+	}
+	
+	// Keys should appear in alphabetical order
+	if !(baseKeyPos < middleKeyPos && middleKeyPos < sharedKeyPos && sharedKeyPos < topKeyPos) {
+		t.Error("Keys should be sorted alphabetically in values section")
+	}
+}
+
+func TestErrorFormatTypedValues(t *testing.T) {
+	// Test that TypedValues from wrapped errors are also included
+	// Note: We need to use the typed values functionality here
+	// Since TypedValues is not directly exposed via options, we test the formatting
+	// by creating errors that would have typed values through internal mechanisms
+	
+	baseErr := goerr.New("base error", goerr.Value("regular_key", "regular_value"))
+	wrappedErr := goerr.Wrap(baseErr, "wrapped error", goerr.Value("wrapped_key", "wrapped_value"))
+	
+	// Test %+v format includes all values
+	detailedResult := fmt.Sprintf("%+v", wrappedErr)
+	
+	// Should contain both regular values
+	if !strings.Contains(detailedResult, "regular_key: regular_value") {
+		t.Error("Should contain base error values")
+	}
+	if !strings.Contains(detailedResult, "wrapped_key: wrapped_value") {
+		t.Error("Should contain wrapped error values")
+	}
+	
+	// Should contain Values section (since we have regular values)
+	if !strings.Contains(detailedResult, "Values:") {
+		t.Error("Should contain Values section")
+	}
+}
+
 func TestErrorStackTrace(t *testing.T) {
 	err := goerr.New("test error")
-	
+
 	stacks := err.Stacks()
 	if len(stacks) == 0 {
 		t.Error("Error should have stack trace")
@@ -226,7 +325,7 @@ func TestErrorIs(t *testing.T) {
 }
 
 func TestErrorCopy(t *testing.T) {
-	original := goerr.New("original", 
+	original := goerr.New("original",
 		goerr.ID("test-id"),
 		goerr.Value("key", "value"))
 
@@ -267,7 +366,7 @@ func TestErrorUnstack(t *testing.T) {
 	// Test UnstackN(1) should have same effect as Unstack
 	err2 := goerr.New("test error 2")
 	unstackedN1 := err2.UnstackN(1)
-	
+
 	if unstackedN1 != err2 {
 		t.Error("UnstackN should return the same instance")
 	}
@@ -300,4 +399,4 @@ func TestPrintable(t *testing.T) {
 	if len(printable.StackTrace) == 0 {
 		t.Error("Printable should have stack trace")
 	}
-}
+}

errors.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"io"
 	"log/slog"
+	"strconv"
 	"strings"
 )
 
@@ -198,10 +199,12 @@ func (x *Errors) MarshalJSON() ([]byte, error) {
 		if goErr := Unwrap(err); goErr != nil {
 			result.Errors[i] = goErr.Printable()
 		} else if marshaler, ok := err.(json.Marshaler); ok {
-			data, _ := marshaler.MarshalJSON()
-			var obj any
-			_ = json.Unmarshal(data, &obj)
-			result.Errors[i] = obj
+			data, marshalErr := marshaler.MarshalJSON()
+			if marshalErr != nil {
+				result.Errors[i] = err.Error()
+			} else {
+				result.Errors[i] = json.RawMessage(data)
+			}
 		} else {
 			result.Errors[i] = err.Error()
 		}
@@ -221,12 +224,13 @@ func (x *Errors) LogValue() slog.Value {
 	}
 
 	// Add individual errors
-	errorAttrs := make([]any, len(x.errs))
+	errorAttrs := make([]any, 0, len(x.errs)*2)
 	for i, err := range x.errs {
+		key := strconv.Itoa(i)
 		if lv, ok := err.(slog.LogValuer); ok {
-			errorAttrs[i] = lv.LogValue()
+			errorAttrs = append(errorAttrs, key, lv.LogValue())
 		} else {
-			errorAttrs[i] = err.Error()
+			errorAttrs = append(errorAttrs, key, err.Error())
 		}
 	}
 	attrs = append(attrs, slog.Group("errors", errorAttrs...))