From a4f8d099542e14d7960899263f2e658dc1d3692f Mon Sep 17 00:00:00 2001 From: AleksandrMatsko Date: Tue, 26 Nov 2024 20:13:05 +0700 Subject: [PATCH 01/11] refactor: status codes and errors on creating, updating checking remote triggers --- api/handler/triggers.go | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/api/handler/triggers.go b/api/handler/triggers.go index 8ee2787a0..48ee6891b 100644 --- a/api/handler/triggers.go +++ b/api/handler/triggers.go @@ -192,13 +192,15 @@ func getTriggerFromRequest(request *http.Request) (*dto.Trigger, *api.ErrorRespo return nil, api.ErrorInvalidRequest(fmt.Errorf("invalid expression: %s", err.Error())) case api.ErrInvalidRequestContent: return nil, api.ErrorInvalidRequest(err) - case remote.ErrRemoteTriggerResponse: + case remote.ErrRemoteUnavailable: response := api.ErrorRemoteServerUnavailable(err) middleware.GetLoggerEntry(request).Error(). String("status", response.StatusText). Error(err). Msg("Remote server unavailable") return nil, response + case remote.ErrRemoteTriggerResponse: + return nil, api.ErrorInvalidRequest(fmt.Errorf("error from graphite remote: %w", err)) case *json.UnmarshalTypeError: return nil, api.ErrorInvalidRequest(fmt.Errorf("invalid payload: %s", err.Error())) case *prometheus.Error: @@ -236,6 +238,7 @@ func getMetricTTLByTrigger(request *http.Request, trigger *dto.Trigger) (time.Du // @success 200 {object} dto.TriggerCheckResponse "Validation is done, see response body for validation result" // @failure 400 {object} api.ErrorInvalidRequestExample "Bad request from client" // @failure 500 {object} api.ErrorInternalServerExample "Internal server error" +// @failure 503 {object} api.ErrorRemoteServerUnavailableExample "Remote server unavailable" // @router /trigger/check [put] func triggerCheck(writer http.ResponseWriter, request *http.Request) { trigger := &dto.Trigger{} @@ -246,10 +249,22 @@ func triggerCheck(writer http.ResponseWriter, request *http.Request) { case expression.ErrInvalidExpression, local.ErrParseExpr, local.ErrEvalExpr, local.ErrUnknownFunction: // TODO: move ErrInvalidExpression to separate case - // These errors are skipped because if there are error from local source then it will be caught in + // Errors above are skipped because if there are error from local source then it will be caught in // dto.TargetVerification and will be explained in detail. + case remote.ErrRemoteUnavailable: + errRsp := api.ErrorRemoteServerUnavailable(err) + middleware.GetLoggerEntry(request).Error(). + String("status", errRsp.StatusText). + Error(err). + Msg("Remote server unavailable") + render.Render(writer, request, errRsp) //nolint + return + case remote.ErrRemoteTriggerResponse: + render.Render(writer, request, api.ErrorInvalidRequest(fmt.Errorf("error from graphite remote: %w", err))) + return case *prometheus.Error: render.Render(writer, request, errorResponseOnPrometheusError(typedErr)) //nolint + return default: render.Render(writer, request, api.ErrorInvalidRequest(err)) //nolint return From 95a9798d9e5fbeae17e2c4ecc6578bc54498c5ce Mon Sep 17 00:00:00 2001 From: AleksandrMatsko Date: Wed, 27 Nov 2024 11:05:47 +0700 Subject: [PATCH 02/11] test: for getTriggerFromRequest --- api/handler/triggers_test.go | 61 +++++++++++++++++++++++++----------- 1 file changed, 43 insertions(+), 18 deletions(-) diff --git a/api/handler/triggers_test.go b/api/handler/triggers_test.go index 275121c15..f8650f486 100644 --- a/api/handler/triggers_test.go +++ b/api/handler/triggers_test.go @@ -5,6 +5,8 @@ import ( "context" "encoding/json" "fmt" + logging "github.com/moira-alert/moira/logging/zerolog_adapter" + "github.com/moira-alert/moira/metric_source/remote" "io" "net/http" "net/http/httptest" @@ -12,9 +14,6 @@ import ( "testing" "time" - logging "github.com/moira-alert/moira/logging/zerolog_adapter" - "github.com/moira-alert/moira/metric_source/remote" - prometheus "github.com/prometheus/client_golang/api/prometheus/v1" "github.com/moira-alert/moira" @@ -196,27 +195,53 @@ func TestGetTriggerFromRequest(t *testing.T) { } Convey("for graphite remote", func() { - triggerDTO.TriggerSource = moira.GraphiteRemote - body, _ := json.Marshal(triggerDTO) + Convey("when ErrRemoteTriggerResponse returned", func() { + triggerDTO.TriggerSource = moira.GraphiteRemote + body, _ := json.Marshal(triggerDTO) - request := httptest.NewRequest(http.MethodPut, "/trigger", bytes.NewReader(body)) - request.Header.Add("content-type", "application/json") - request = request.WithContext(middleware.SetContextValueForTest(request.Context(), "metricSourceProvider", allSourceProvider)) - request = request.WithContext(middleware.SetContextValueForTest(request.Context(), "limits", api.GetTestLimitsConfig())) + request := httptest.NewRequest(http.MethodPut, "/trigger", bytes.NewReader(body)) + request.Header.Add("content-type", "application/json") + request = request.WithContext(middleware.SetContextValueForTest(request.Context(), "metricSourceProvider", allSourceProvider)) + request = request.WithContext(middleware.SetContextValueForTest(request.Context(), "limits", api.GetTestLimitsConfig())) - testLogger, _ := logging.GetLogger("Test") + testLogger, _ := logging.GetLogger("Test") - request = middleware.WithLogEntry(request, middleware.NewLogEntry(testLogger, request)) + request = middleware.WithLogEntry(request, middleware.NewLogEntry(testLogger, request)) - var returnedErr error = remote.ErrRemoteTriggerResponse{ - InternalError: fmt.Errorf(""), - } + var returnedErr error = remote.ErrRemoteTriggerResponse{ + InternalError: fmt.Errorf(""), + } + + graphiteRemoteSrc.EXPECT().Fetch(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(nil, returnedErr) - graphiteRemoteSrc.EXPECT().Fetch(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). - Return(nil, returnedErr) + _, errRsp := getTriggerFromRequest(request) + So(errRsp, ShouldResemble, api.ErrorInvalidRequest(fmt.Errorf("error from graphite remote: %w", returnedErr))) + }) + + Convey("when ErrRemoteUnavailable", func() { + triggerDTO.TriggerSource = moira.GraphiteRemote + body, _ := json.Marshal(triggerDTO) + + request := httptest.NewRequest(http.MethodPut, "/trigger", bytes.NewReader(body)) + request.Header.Add("content-type", "application/json") + request = request.WithContext(middleware.SetContextValueForTest(request.Context(), "metricSourceProvider", allSourceProvider)) + request = request.WithContext(middleware.SetContextValueForTest(request.Context(), "limits", api.GetTestLimitsConfig())) - _, errRsp := getTriggerFromRequest(request) - So(errRsp, ShouldResemble, api.ErrorRemoteServerUnavailable(returnedErr)) + testLogger, _ := logging.GetLogger("Test") + + request = middleware.WithLogEntry(request, middleware.NewLogEntry(testLogger, request)) + + var returnedErr error = remote.ErrRemoteUnavailable{ + InternalError: fmt.Errorf(""), + } + + graphiteRemoteSrc.EXPECT().Fetch(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(nil, returnedErr) + + _, errRsp := getTriggerFromRequest(request) + So(errRsp, ShouldResemble, api.ErrorRemoteServerUnavailable(returnedErr)) + }) }) Convey("for prometheus remote", func() { From 6cb7741ff4264ac40a387765fe04accb7ad61c6a Mon Sep 17 00:00:00 2001 From: AleksandrMatsko Date: Wed, 27 Nov 2024 11:42:38 +0700 Subject: [PATCH 03/11] test: refactor setting values in ctx --- api/handler/triggers_test.go | 38 +++++++++++++++++++----------------- 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/api/handler/triggers_test.go b/api/handler/triggers_test.go index f8650f486..ae13b5461 100644 --- a/api/handler/triggers_test.go +++ b/api/handler/triggers_test.go @@ -65,6 +65,17 @@ func TestGetTriggerFromRequest(t *testing.T) { fetchResult.EXPECT().GetPatterns().Return(make([]string, 0), nil).AnyTimes() fetchResult.EXPECT().GetMetricsData().Return([]metricSource.MetricData{*metricSource.MakeMetricData("", []float64{}, 0, 0)}).AnyTimes() + setValuesToRequestCtx := func( + ctx context.Context, + metricSourceProvider *metricSource.SourceProvider, + limits api.LimitsConfig, + ) context.Context { + ctx = middleware.SetContextValueForTest(ctx, "metricSourceProvider", metricSourceProvider) + ctx = middleware.SetContextValueForTest(ctx, "limits", limits) + + return ctx + } + Convey("Given a correct payload", t, func() { triggerWarnValue := 0.0 triggerErrorValue := 1.0 @@ -105,8 +116,7 @@ func TestGetTriggerFromRequest(t *testing.T) { request := httptest.NewRequest(http.MethodPut, "/trigger", bytes.NewReader(body)) request.Header.Add("content-type", "application/json") - request = request.WithContext(middleware.SetContextValueForTest(request.Context(), "metricSourceProvider", sourceProvider)) - request = request.WithContext(middleware.SetContextValueForTest(request.Context(), "limits", api.GetTestLimitsConfig())) + request = request.WithContext(setValuesToRequestCtx(request.Context(), sourceProvider, api.GetTestLimitsConfig())) triggerDTO.Schedule.Days = moira.GetFilledScheduleDataDays(false) triggerDTO.Schedule.Days[0].Enabled = true @@ -147,8 +157,7 @@ func TestGetTriggerFromRequest(t *testing.T) { request := httptest.NewRequest(http.MethodPut, "/trigger", strings.NewReader(body)) request.Header.Add("content-type", "application/json") - request = request.WithContext(middleware.SetContextValueForTest(request.Context(), "metricSourceProvider", sourceProvider)) - request = request.WithContext(middleware.SetContextValueForTest(request.Context(), "limits", api.GetTestLimitsConfig())) + request = request.WithContext(setValuesToRequestCtx(request.Context(), sourceProvider, api.GetTestLimitsConfig())) Convey("Parser should return en error", func() { _, err := getTriggerFromRequest(request) @@ -195,14 +204,13 @@ func TestGetTriggerFromRequest(t *testing.T) { } Convey("for graphite remote", func() { - Convey("when ErrRemoteTriggerResponse returned", func() { - triggerDTO.TriggerSource = moira.GraphiteRemote - body, _ := json.Marshal(triggerDTO) + triggerDTO.TriggerSource = moira.GraphiteRemote + body, _ := json.Marshal(triggerDTO) + Convey("when ErrRemoteTriggerResponse returned", func() { request := httptest.NewRequest(http.MethodPut, "/trigger", bytes.NewReader(body)) request.Header.Add("content-type", "application/json") - request = request.WithContext(middleware.SetContextValueForTest(request.Context(), "metricSourceProvider", allSourceProvider)) - request = request.WithContext(middleware.SetContextValueForTest(request.Context(), "limits", api.GetTestLimitsConfig())) + request = request.WithContext(setValuesToRequestCtx(request.Context(), allSourceProvider, api.GetTestLimitsConfig())) testLogger, _ := logging.GetLogger("Test") @@ -220,13 +228,9 @@ func TestGetTriggerFromRequest(t *testing.T) { }) Convey("when ErrRemoteUnavailable", func() { - triggerDTO.TriggerSource = moira.GraphiteRemote - body, _ := json.Marshal(triggerDTO) - request := httptest.NewRequest(http.MethodPut, "/trigger", bytes.NewReader(body)) request.Header.Add("content-type", "application/json") - request = request.WithContext(middleware.SetContextValueForTest(request.Context(), "metricSourceProvider", allSourceProvider)) - request = request.WithContext(middleware.SetContextValueForTest(request.Context(), "limits", api.GetTestLimitsConfig())) + request = request.WithContext(setValuesToRequestCtx(request.Context(), allSourceProvider, api.GetTestLimitsConfig())) testLogger, _ := logging.GetLogger("Test") @@ -251,8 +255,7 @@ func TestGetTriggerFromRequest(t *testing.T) { Convey("with error type = bad_data got bad request", func() { request := httptest.NewRequest(http.MethodPut, "/trigger", bytes.NewReader(body)) request.Header.Add("content-type", "application/json") - request = request.WithContext(middleware.SetContextValueForTest(request.Context(), "metricSourceProvider", allSourceProvider)) - request = request.WithContext(middleware.SetContextValueForTest(request.Context(), "limits", api.GetTestLimitsConfig())) + request = request.WithContext(setValuesToRequestCtx(request.Context(), allSourceProvider, api.GetTestLimitsConfig())) var returnedErr error = &prometheus.Error{ Type: prometheus.ErrBadData, @@ -278,8 +281,7 @@ func TestGetTriggerFromRequest(t *testing.T) { for _, errType := range otherTypes { request := httptest.NewRequest(http.MethodPut, "/trigger", bytes.NewReader(body)) request.Header.Add("content-type", "application/json") - request = request.WithContext(middleware.SetContextValueForTest(request.Context(), "metricSourceProvider", allSourceProvider)) - request = request.WithContext(middleware.SetContextValueForTest(request.Context(), "limits", api.GetTestLimitsConfig())) + request = request.WithContext(setValuesToRequestCtx(request.Context(), allSourceProvider, api.GetTestLimitsConfig())) var returnedErr error = &prometheus.Error{ Type: errType, From 1f3db9732053098056db431efece2df2879626f3 Mon Sep 17 00:00:00 2001 From: AleksandrMatsko Date: Wed, 27 Nov 2024 11:45:01 +0700 Subject: [PATCH 04/11] style: use linter --- api/handler/triggers_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/api/handler/triggers_test.go b/api/handler/triggers_test.go index ae13b5461..4534c35d6 100644 --- a/api/handler/triggers_test.go +++ b/api/handler/triggers_test.go @@ -5,8 +5,6 @@ import ( "context" "encoding/json" "fmt" - logging "github.com/moira-alert/moira/logging/zerolog_adapter" - "github.com/moira-alert/moira/metric_source/remote" "io" "net/http" "net/http/httptest" @@ -14,6 +12,9 @@ import ( "testing" "time" + logging "github.com/moira-alert/moira/logging/zerolog_adapter" + "github.com/moira-alert/moira/metric_source/remote" + prometheus "github.com/prometheus/client_golang/api/prometheus/v1" "github.com/moira-alert/moira" From 0129e494581b45c720ac209847263ec16e76cbe4 Mon Sep 17 00:00:00 2001 From: AleksandrMatsko Date: Wed, 27 Nov 2024 11:46:00 +0700 Subject: [PATCH 05/11] style: use swag fmt --- api/handler/triggers.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/api/handler/triggers.go b/api/handler/triggers.go index 48ee6891b..5649e9c6d 100644 --- a/api/handler/triggers.go +++ b/api/handler/triggers.go @@ -234,10 +234,10 @@ func getMetricTTLByTrigger(request *http.Request, trigger *dto.Trigger) (time.Du // @tags trigger // @accept json // @produce json -// @param trigger body dto.Trigger true "Trigger data" -// @success 200 {object} dto.TriggerCheckResponse "Validation is done, see response body for validation result" -// @failure 400 {object} api.ErrorInvalidRequestExample "Bad request from client" -// @failure 500 {object} api.ErrorInternalServerExample "Internal server error" +// @param trigger body dto.Trigger true "Trigger data" +// @success 200 {object} dto.TriggerCheckResponse "Validation is done, see response body for validation result" +// @failure 400 {object} api.ErrorInvalidRequestExample "Bad request from client" +// @failure 500 {object} api.ErrorInternalServerExample "Internal server error" // @failure 503 {object} api.ErrorRemoteServerUnavailableExample "Remote server unavailable" // @router /trigger/check [put] func triggerCheck(writer http.ResponseWriter, request *http.Request) { From 5cfed1f889e8d4655729f53b071190eb678d3166 Mon Sep 17 00:00:00 2001 From: AleksandrMatsko Date: Wed, 27 Nov 2024 11:47:18 +0700 Subject: [PATCH 06/11] style: use linter --- api/handler/triggers.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/handler/triggers.go b/api/handler/triggers.go index 5649e9c6d..539594898 100644 --- a/api/handler/triggers.go +++ b/api/handler/triggers.go @@ -260,7 +260,7 @@ func triggerCheck(writer http.ResponseWriter, request *http.Request) { render.Render(writer, request, errRsp) //nolint return case remote.ErrRemoteTriggerResponse: - render.Render(writer, request, api.ErrorInvalidRequest(fmt.Errorf("error from graphite remote: %w", err))) + render.Render(writer, request, api.ErrorInvalidRequest(fmt.Errorf("error from graphite remote: %w", err))) //nolint return case *prometheus.Error: render.Render(writer, request, errorResponseOnPrometheusError(typedErr)) //nolint From 455dfd708f8f0c3a349085106a02e3eef2ac6e17 Mon Sep 17 00:00:00 2001 From: AleksandrMatsko Date: Wed, 27 Nov 2024 14:44:55 +0700 Subject: [PATCH 07/11] test: move shared variable --- api/handler/triggers_test.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/api/handler/triggers_test.go b/api/handler/triggers_test.go index 4534c35d6..ea558c2bf 100644 --- a/api/handler/triggers_test.go +++ b/api/handler/triggers_test.go @@ -207,14 +207,13 @@ func TestGetTriggerFromRequest(t *testing.T) { Convey("for graphite remote", func() { triggerDTO.TriggerSource = moira.GraphiteRemote body, _ := json.Marshal(triggerDTO) + testLogger, _ := logging.GetLogger("Test") Convey("when ErrRemoteTriggerResponse returned", func() { request := httptest.NewRequest(http.MethodPut, "/trigger", bytes.NewReader(body)) request.Header.Add("content-type", "application/json") request = request.WithContext(setValuesToRequestCtx(request.Context(), allSourceProvider, api.GetTestLimitsConfig())) - testLogger, _ := logging.GetLogger("Test") - request = middleware.WithLogEntry(request, middleware.NewLogEntry(testLogger, request)) var returnedErr error = remote.ErrRemoteTriggerResponse{ @@ -233,8 +232,6 @@ func TestGetTriggerFromRequest(t *testing.T) { request.Header.Add("content-type", "application/json") request = request.WithContext(setValuesToRequestCtx(request.Context(), allSourceProvider, api.GetTestLimitsConfig())) - testLogger, _ := logging.GetLogger("Test") - request = middleware.WithLogEntry(request, middleware.NewLogEntry(testLogger, request)) var returnedErr error = remote.ErrRemoteUnavailable{ From 7e6648f2f424cb745168149591d21fdedeb4db33 Mon Sep 17 00:00:00 2001 From: Aleksandr Matsko <90016901+AleksandrMatsko@users.noreply.github.com> Date: Fri, 29 Nov 2024 13:02:09 +0700 Subject: [PATCH 08/11] docs: change comment Co-authored-by: Tetrergeru --- api/handler/triggers.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/handler/triggers.go b/api/handler/triggers.go index 539594898..d13e19bfb 100644 --- a/api/handler/triggers.go +++ b/api/handler/triggers.go @@ -249,7 +249,7 @@ func triggerCheck(writer http.ResponseWriter, request *http.Request) { case expression.ErrInvalidExpression, local.ErrParseExpr, local.ErrEvalExpr, local.ErrUnknownFunction: // TODO: move ErrInvalidExpression to separate case - // Errors above are skipped because if there are error from local source then it will be caught in + // Errors above are skipped because if there is an error from local source then it will be caught in // dto.TargetVerification and will be explained in detail. case remote.ErrRemoteUnavailable: errRsp := api.ErrorRemoteServerUnavailable(err) From 0c45ef57f29ac3c357187f93fdd9f853ef0d43a9 Mon Sep 17 00:00:00 2001 From: AleksandrMatsko Date: Fri, 29 Nov 2024 14:40:37 +0700 Subject: [PATCH 09/11] fix: custom type in swagger for DayName --- datatypes.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/datatypes.go b/datatypes.go index a9f317563..938c10068 100644 --- a/datatypes.go +++ b/datatypes.go @@ -255,7 +255,7 @@ type ScheduleData struct { // ScheduleDataDay represents week day of schedule. type ScheduleDataDay struct { Enabled bool `json:"enabled" example:"true"` - Name DayName `json:"name,omitempty" example:"Mon" validate:"oneof=Mon Tue Wed Thu Fri Sat Sun"` + Name DayName `json:"name,omitempty" example:"Mon" swaggertype:"string" validate:"oneof=Mon Tue Wed Thu Fri Sat Sun"` } // DayName represents the day name used in ScheduleDataDay. From 1301e9b5ab9075cd3a903ccc84eadf1c8b23211c Mon Sep 17 00:00:00 2001 From: AleksandrMatsko Date: Tue, 3 Dec 2024 10:21:23 +0700 Subject: [PATCH 10/11] fix: swagger spec for DELETE /api/notification/all --- api/handler/notification.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/handler/notification.go b/api/handler/notification.go index 996a92352..85fd04c33 100644 --- a/api/handler/notification.go +++ b/api/handler/notification.go @@ -110,7 +110,7 @@ func deleteNotification(writer http.ResponseWriter, request *http.Request) { // @success 200 {object} dto.NotificationsList "Notification have been deleted" // @failure 403 {object} api.ErrorForbiddenExample "Forbidden" // @failure 500 {object} api.ErrorInternalServerExample "Internal server error" -// @router /notification [delete] +// @router /notification/all [delete] func deleteAllNotifications(writer http.ResponseWriter, request *http.Request) { if errorResponse := controller.DeleteAllNotifications(database); errorResponse != nil { render.Render(writer, request, errorResponse) //nolint From fb664178f49ef429fdc231f7a36105133cc115c8 Mon Sep 17 00:00:00 2001 From: AleksandrMatsko Date: Thu, 5 Dec 2024 19:46:18 +0700 Subject: [PATCH 11/11] refactor: swagger spec --- api/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/config.go b/api/config.go index 2db57ff71..8ef93fa2b 100644 --- a/api/config.go +++ b/api/config.go @@ -23,7 +23,7 @@ type FeatureFlags struct { IsPlottingAvailable bool `json:"isPlottingAvailable" example:"true"` IsSubscriptionToAllTagsAvailable bool `json:"isSubscriptionToAllTagsAvailable" example:"false"` IsReadonlyEnabled bool `json:"isReadonlyEnabled" example:"false"` - CelebrationMode CelebrationMode `json:"celebrationMode" example:"new_year"` + CelebrationMode CelebrationMode `json:"celebrationMode" swaggertype:"string" example:"new_year"` } // CelebrationMode is type for celebrate Moira.