From fd0b3dabb22f30f67c830d04e7b6389bf5789b02 Mon Sep 17 00:00:00 2001
From: Bowei Xu <boweixu@uber.com>
Date: Mon, 13 Aug 2018 11:12:34 -0700
Subject: [PATCH] Fix getErrorDetails panic (#542)

---
 internal/error_test.go          | 82 +++++++++++++++++++++++++++++++++
 internal/internal_utils.go      |  4 +-
 internal/internal_utils_test.go | 38 +++++++++++++++
 3 files changed, 122 insertions(+), 2 deletions(-)

diff --git a/internal/error_test.go b/internal/error_test.go
index c94b07bbf..f34289d99 100644
--- a/internal/error_test.go
+++ b/internal/error_test.go
@@ -40,10 +40,17 @@ type testStruct struct {
 	Age  int
 }
 
+type testStruct2 struct {
+	Name      string
+	Age       int
+	Favorites *[]string
+}
+
 var (
 	testErrorDetails1 = "my details"
 	testErrorDetails2 = 123
 	testErrorDetails3 = testStruct{"a string", 321}
+	testErrorDetails4 = testStruct2{"a string", 321, &[]string{"eat", "code"}}
 )
 
 func Test_GenericError(t *testing.T) {
@@ -195,6 +202,81 @@ func Test_CustomError(t *testing.T) {
 	require.Equal(t, testErrorDetails3, b3)
 }
 
+func Test_CustomError_Pointer(t *testing.T) {
+	a1 := testStruct2{}
+	err1 := NewCustomError(customErrReasonA, testErrorDetails4)
+	require.True(t, err1.HasDetails())
+	err := err1.Details(&a1)
+	require.NoError(t, err)
+	require.Equal(t, testErrorDetails4, a1)
+
+	a2 := &testStruct2{}
+	err2 := NewCustomError(customErrReasonA, &testErrorDetails4) // // pointer in details
+	require.True(t, err2.HasDetails())
+	err = err2.Details(&a2)
+	require.NoError(t, err)
+	require.Equal(t, &testErrorDetails4, a2)
+
+	// test EncodedValues as Details
+	errorActivityFn := func() error {
+		return err1
+	}
+	RegisterActivity(errorActivityFn)
+	s := &WorkflowTestSuite{}
+	env := s.NewTestActivityEnvironment()
+	_, err = env.ExecuteActivity(errorActivityFn)
+	require.Error(t, err)
+	err3, ok := err.(*CustomError)
+	require.True(t, ok)
+	require.True(t, err3.HasDetails())
+	b1 := testStruct2{}
+	require.NoError(t, err3.Details(&b1))
+	require.Equal(t, testErrorDetails4, b1)
+
+	errorActivityFn2 := func() error {
+		return err2 // pointer in details
+	}
+	RegisterActivity(errorActivityFn2)
+	_, err = env.ExecuteActivity(errorActivityFn2)
+	require.Error(t, err)
+	err4, ok := err.(*CustomError)
+	require.True(t, ok)
+	require.True(t, err4.HasDetails())
+	b2 := &testStruct2{}
+	require.NoError(t, err4.Details(&b2))
+	require.Equal(t, &testErrorDetails4, b2)
+
+	// test workflow error
+	errorWorkflowFn := func(ctx Context) error {
+		return err1
+	}
+	RegisterWorkflow(errorWorkflowFn)
+	wfEnv := s.NewTestWorkflowEnvironment()
+	wfEnv.ExecuteWorkflow(errorWorkflowFn)
+	err = wfEnv.GetWorkflowError()
+	require.Error(t, err)
+	err5, ok := err.(*CustomError)
+	require.True(t, ok)
+	require.True(t, err5.HasDetails())
+	err5.Details(&b1)
+	require.NoError(t, err5.Details(&b1))
+	require.Equal(t, testErrorDetails4, b1)
+
+	errorWorkflowFn2 := func(ctx Context) error {
+		return err2 // pointer in details
+	}
+	RegisterWorkflow(errorWorkflowFn2)
+	wfEnv.ExecuteWorkflow(errorWorkflowFn2)
+	err = wfEnv.GetWorkflowError()
+	require.Error(t, err)
+	err6, ok := err.(*CustomError)
+	require.True(t, ok)
+	require.True(t, err6.HasDetails())
+	err6.Details(&b2)
+	require.NoError(t, err6.Details(&b2))
+	require.Equal(t, &testErrorDetails4, b2)
+}
+
 func Test_CanceledError(t *testing.T) {
 	// test ErrorDetailValues as Details
 	var a1 string
diff --git a/internal/internal_utils.go b/internal/internal_utils.go
index e1fbd343f..049e28a48 100644
--- a/internal/internal_utils.go
+++ b/internal/internal_utils.go
@@ -152,7 +152,7 @@ func getErrorDetails(err error, dataConverter encoded.DataConverter) (string, []
 		switch details := err.details.(type) {
 		case ErrorDetailsValues:
 			data, err0 = encodeArgs(dataConverter, details)
-		case EncodedValues:
+		case *EncodedValues:
 			data = details.values
 		default:
 			panic("unknown error type")
@@ -167,7 +167,7 @@ func getErrorDetails(err error, dataConverter encoded.DataConverter) (string, []
 		switch details := err.details.(type) {
 		case ErrorDetailsValues:
 			data, err0 = encodeArgs(dataConverter, details)
-		case EncodedValues:
+		case *EncodedValues:
 			data = details.values
 		default:
 			panic("unknown error type")
diff --git a/internal/internal_utils_test.go b/internal/internal_utils_test.go
index ca4591fa0..8f3eb65a5 100644
--- a/internal/internal_utils_test.go
+++ b/internal/internal_utils_test.go
@@ -67,3 +67,41 @@ func TestNewValue(t *testing.T) {
 	NewValue(data).Get(&res)
 	require.Equal(t, res, heartbeatDetail)
 }
+
+func TestGetErrorDetails_CustomError(t *testing.T) {
+	dc := newDefaultDataConverter()
+	details, err := dc.ToData("error details")
+	require.NoError(t, err)
+
+	val := newEncodedValues(details, dc).(*EncodedValues)
+	customErr1 := NewCustomError(customErrReasonA, val)
+	reason, data := getErrorDetails(customErr1, dc)
+	require.Equal(t, customErrReasonA, reason)
+	require.Equal(t, val.values, data)
+
+	customErr2 := NewCustomError(customErrReasonA, testErrorDetails1)
+	val2, err := encodeArgs(dc, []interface{}{testErrorDetails1})
+	require.NoError(t, err)
+	reason, data = getErrorDetails(customErr2, dc)
+	require.Equal(t, customErrReasonA, reason)
+	require.Equal(t, val2, data)
+}
+
+func TestGetErrorDetails_CancelError(t *testing.T) {
+	dc := newDefaultDataConverter()
+	details, err := dc.ToData("error details")
+	require.NoError(t, err)
+
+	val := newEncodedValues(details, dc).(*EncodedValues)
+	canceledErr1 := NewCanceledError(val)
+	reason, data := getErrorDetails(canceledErr1, dc)
+	require.Equal(t, errReasonCanceled, reason)
+	require.Equal(t, val.values, data)
+
+	canceledErr2 := NewCanceledError(testErrorDetails1)
+	val2, err := encodeArgs(dc, []interface{}{testErrorDetails1})
+	require.NoError(t, err)
+	reason, data = getErrorDetails(canceledErr2, dc)
+	require.Equal(t, errReasonCanceled, reason)
+	require.Equal(t, val2, data)
+}