Query: correctly marshal errors to JSON and ignore if nil (#2848)

* correctly marshal errors to JSON and ignore if nil

Signed-off-by: Dan Potepa <[email protected]>

* LastError should be cleared if the newer update was ok

Signed-off-by: Dan Potepa <[email protected]>
Signed-off-by: Kemal Akkoyun <[email protected]>

pkg/query/storeset.go

@@ -527,15 +527,16 @@ func (s *StoreSet) updateStoreStatus(store *storeRef, err error) {
 		status = *prev
 	}
 
-	status.LastError = &stringError{originalErr: err}
-
 	if err == nil {
 		status.LastCheck = time.Now()
 		mint, maxt := store.TimeRange()
 		status.LabelSets = store.LabelSets()
 		status.StoreType = store.StoreType()
 		status.MinTime = mint
 		status.MaxTime = maxt
+		status.LastError = nil
+	} else {
+		status.LastError = &stringError{originalErr: err}
 	}
 
 	s.storeStatuses[store.addr] = &status

pkg/query/storeset_test.go

@@ -781,27 +781,80 @@ func TestStoreSet_Update_Rules(t *testing.T) {
 	}
 }
 
-type weirdError struct {
-	originalErr error
+type errThatMarshalsToEmptyDict struct {
+	msg string
 }
 
-// MarshalJSON marshals the error and returns weird results.
-func (e *weirdError) MarshalJSON() ([]byte, error) {
+// MarshalJSON marshals the error and returns and empty dict, not the error string.
+func (e *errThatMarshalsToEmptyDict) MarshalJSON() ([]byte, error) {
 	return json.Marshal(map[string]string{})
 }
 
 // Error returns the original, underlying string.
-func (e *weirdError) Error() string {
-	return e.originalErr.Error()
+func (e *errThatMarshalsToEmptyDict) Error() string {
+	return e.msg
 }
 
+// Test highlights that without wrapping the error, it is marshalled to empty dict {}, not its message.
 func TestStringError(t *testing.T) {
-	weirdError := &weirdError{originalErr: errors.New("test")}
-	properErr := &stringError{originalErr: weirdError}
+	dictErr := &errThatMarshalsToEmptyDict{msg: "Error message"}
+	stringErr := &stringError{originalErr: dictErr}
+
 	storestatusMock := map[string]error{}
-	storestatusMock["weird"] = weirdError
-	storestatusMock["proper"] = properErr
+	storestatusMock["dictErr"] = dictErr
+	storestatusMock["stringErr"] = stringErr
+
 	b, err := json.Marshal(storestatusMock)
+
+	testutil.Ok(t, err)
+	testutil.Equals(t, []byte(`{"dictErr":{},"stringErr":"Error message"}`), b, "expected to get proper results")
+}
+
+// Errors that usually marshal to empty dict should return the original error string.
+func TestUpdateStoreStateLastError(t *testing.T) {
+	tcs := []struct {
+		InputError      error
+		ExpectedLastErr string
+	}{
+		{errors.New("normal_err"), `"normal_err"`},
+		{nil, `null`},
+		{&errThatMarshalsToEmptyDict{"the error message"}, `"the error message"`},
+	}
+
+	for _, tc := range tcs {
+		mockStoreSet := &StoreSet{
+			storeStatuses: map[string]*StoreStatus{},
+		}
+		mockStoreRef := &storeRef{
+			addr: "testStore",
+		}
+
+		mockStoreSet.updateStoreStatus(mockStoreRef, tc.InputError)
+
+		b, err := json.Marshal(mockStoreSet.storeStatuses["testStore"].LastError)
+		testutil.Ok(t, err)
+		testutil.Equals(t, tc.ExpectedLastErr, string(b))
+	}
+}
+
+func TestUpdateStoreStateForgetsPreviousErrors(t *testing.T) {
+	mockStoreSet := &StoreSet{
+		storeStatuses: map[string]*StoreStatus{},
+	}
+	mockStoreRef := &storeRef{
+		addr: "testStore",
+	}
+
+	mockStoreSet.updateStoreStatus(mockStoreRef, errors.New("test err"))
+
+	b, err := json.Marshal(mockStoreSet.storeStatuses["testStore"].LastError)
+	testutil.Ok(t, err)
+	testutil.Equals(t, `"test err"`, string(b))
+
+	// updating status without and error should clear the previous one.
+	mockStoreSet.updateStoreStatus(mockStoreRef, nil)
+
+	b, err = json.Marshal(mockStoreSet.storeStatuses["testStore"].LastError)
 	testutil.Ok(t, err)
-	testutil.Equals(t, []byte(`{"proper":"test","weird":{}}`), b, "expected to get proper results")
+	testutil.Equals(t, `null`, string(b))
 }