From f8a708fc1f1730fe6e95c3d5d06a8d05e68a1158 Mon Sep 17 00:00:00 2001 From: Dan Potepa Date: Mon, 6 Jul 2020 22:40:07 +0100 Subject: [PATCH 1/2] correctly marshal errors to JSON and ignore if nil Signed-off-by: Dan Potepa --- pkg/query/storeset.go | 4 +-- pkg/query/storeset_test.go | 53 ++++++++++++++++++++++++++++++-------- 2 files changed, 44 insertions(+), 13 deletions(-) diff --git a/pkg/query/storeset.go b/pkg/query/storeset.go index 654cee494b..d3e4728c36 100644 --- a/pkg/query/storeset.go +++ b/pkg/query/storeset.go @@ -527,8 +527,6 @@ 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() @@ -536,6 +534,8 @@ func (s *StoreSet) updateStoreStatus(store *storeRef, err error) { status.StoreType = store.StoreType() status.MinTime = mint status.MaxTime = maxt + } else { + status.LastError = &stringError{originalErr: err} } s.storeStatuses[store.addr] = &status diff --git a/pkg/query/storeset_test.go b/pkg/query/storeset_test.go index 58813b2dce..d5e8ade9f0 100644 --- a/pkg/query/storeset_test.go +++ b/pkg/query/storeset_test.go @@ -781,27 +781,58 @@ 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(`{"proper":"test","weird":{}}`), b, "expected to get proper results") + 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)) + } } From 3170037a5a489e637c20f5ba3a5b9fb901bd1d82 Mon Sep 17 00:00:00 2001 From: Dan Potepa Date: Tue, 7 Jul 2020 14:23:13 +0100 Subject: [PATCH 2/2] LastError should be cleared if the newer update was ok Signed-off-by: Dan Potepa --- pkg/query/storeset.go | 1 + pkg/query/storeset_test.go | 22 ++++++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/pkg/query/storeset.go b/pkg/query/storeset.go index d3e4728c36..22abbda239 100644 --- a/pkg/query/storeset.go +++ b/pkg/query/storeset.go @@ -534,6 +534,7 @@ func (s *StoreSet) updateStoreStatus(store *storeRef, err error) { status.StoreType = store.StoreType() status.MinTime = mint status.MaxTime = maxt + status.LastError = nil } else { status.LastError = &stringError{originalErr: err} } diff --git a/pkg/query/storeset_test.go b/pkg/query/storeset_test.go index d5e8ade9f0..0ccc61f9db 100644 --- a/pkg/query/storeset_test.go +++ b/pkg/query/storeset_test.go @@ -836,3 +836,25 @@ func TestUpdateStoreStateLastError(t *testing.T) { 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, `null`, string(b)) +}