From c3f5e50b9f489123d20acb52ad972c49c4b9b760 Mon Sep 17 00:00:00 2001
From: Vaibhav Kamra <vkamra@alcion.ai>
Date: Fri, 17 Feb 2023 17:43:59 -0800
Subject: [PATCH] Fix how ErrrorData is marshaled (#2565)

## Description

The error type does not get marshaled which can lead to errors when we try to un-marshal a manifest
that had errors stored in it.

Repro here: https://go.dev/play/p/tgj8oq5CGFd

For now - this disables JSON marshaling and also fixes the error un-marshaling. I have added a regression test
to verify both behaviors.

As a follow-up, I believe we can implement a custom marshaler.

## Does this PR need a docs update or release note?

- [ ] :white_check_mark: Yes, it's included
- [ ] :clock1: Yes, but in a later PR
- [x] :no_entry: No

## Type of change

<!--- Please check the type of change your PR introduces: --->
- [ ] :sunflower: Feature
- [x] :bug: Bugfix
- [ ] :world_map: Documentation
- [ ] :robot: Test
- [ ] :computer: CI/Deployment
- [ ] :broom: Tech Debt/Cleanup

## Test Plan

<!-- How will this be tested prior to merging.-->
- [ ] :muscle: Manual
- [x] :zap: Unit test
- [ ] :green_heart: E2E
---
 src/pkg/fault/fault.go      |  4 ++--
 src/pkg/fault/fault_test.go | 47 +++++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/src/pkg/fault/fault.go b/src/pkg/fault/fault.go
index ea26757ee..71de706a7 100644
--- a/src/pkg/fault/fault.go
+++ b/src/pkg/fault/fault.go
@@ -32,8 +32,8 @@ type Errors struct {
 // ErrorsData provides the errors data alone, without sync
 // controls, allowing the data to be persisted.
 type ErrorsData struct {
-	Err      error   `json:"err"`
-	Errs     []error `json:"errs"`
+	Err      error   `json:"-"`
+	Errs     []error `json:"-"`
 	FailFast bool    `json:"failFast"`
 }
 
diff --git a/src/pkg/fault/fault_test.go b/src/pkg/fault/fault_test.go
index 8fb2981d7..4a995581e 100644
--- a/src/pkg/fault/fault_test.go
+++ b/src/pkg/fault/fault_test.go
@@ -1,7 +1,9 @@
 package fault_test
 
 import (
+	"encoding/json"
 	"errors"
+	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -204,3 +206,48 @@ func (suite *FaultErrorsUnitSuite) TestData() {
 	assert.ElementsMatch(t, n.Errs(), d.Errs)
 	assert.True(t, d.FailFast)
 }
+
+func (suite *FaultErrorsUnitSuite) TestMarshalUnmarshal() {
+	t := suite.T()
+
+	// not fail-fast
+	n := fault.New(false)
+	require.NotNil(t, n)
+
+	n.Add(errors.New("1"))
+	n.Add(errors.New("2"))
+
+	data := n.Data()
+
+	jsonStr, err := json.Marshal(data)
+	require.NoError(t, err)
+
+	um := fault.ErrorsData{}
+
+	err = json.Unmarshal(jsonStr, &um)
+	require.NoError(t, err)
+}
+
+type legacyErrorsData struct {
+	Err      error   `json:"err"`
+	Errs     []error `json:"errs"`
+	FailFast bool    `json:"failFast"`
+}
+
+func (suite *FaultErrorsUnitSuite) TestUnmarshalLegacy() {
+	t := suite.T()
+
+	oldData := &legacyErrorsData{
+		Errs: []error{fmt.Errorf("foo error"), fmt.Errorf("foo error"), fmt.Errorf("foo error")},
+	}
+
+	jsonStr, err := json.Marshal(oldData)
+	require.NoError(t, err)
+
+	t.Logf("jsonStr is %s\n", jsonStr)
+
+	um := fault.ErrorsData{}
+
+	err = json.Unmarshal(jsonStr, &um)
+	require.NoError(t, err)
+}