diff --git a/pkg/services/alerting/conditions/query_test.go b/pkg/services/alerting/conditions/query_test.go index 4c2b1689277..3e4835ffa82 100644 --- a/pkg/services/alerting/conditions/query_test.go +++ b/pkg/services/alerting/conditions/query_test.go @@ -23,7 +23,8 @@ func TestQueryCondition(t *testing.T) { ctx.evaluator = `{"type": "gt", "params": [100]}` Convey("Can read query condition from json model", func() { - ctx.exec() + _, err := ctx.exec() + So(err, ShouldBeNil) So(ctx.condition.Query.From, ShouldEqual, "5m") So(ctx.condition.Query.To, ShouldEqual, "now") diff --git a/pkg/services/alerting/engine.go b/pkg/services/alerting/engine.go index c57558d03d0..7a52d31a40f 100644 --- a/pkg/services/alerting/engine.go +++ b/pkg/services/alerting/engine.go @@ -15,6 +15,7 @@ import ( "github.com/grafana/grafana/pkg/services/rendering" "github.com/grafana/grafana/pkg/setting" "golang.org/x/sync/errgroup" + "golang.org/x/xerrors" ) // AlertEngine is the background process that @@ -210,7 +211,16 @@ func (e *AlertEngine) processJob(attemptID int, attemptChan chan int, cancelChan // dont reuse the evalContext and get its own context. evalContext.Ctx = resultHandleCtx evalContext.Rule.State = evalContext.GetNewState() - e.resultHandler.handle(evalContext) + if err := e.resultHandler.handle(evalContext); err != nil { + if xerrors.Is(err, context.Canceled) { + e.log.Debug("Result handler returned context.Canceled") + } else if xerrors.Is(err, context.DeadlineExceeded) { + e.log.Debug("Result handler returned context.DeadlineExceeded") + } else { + e.log.Error("Failed to handle result", "err", err) + } + } + span.Finish() e.log.Debug("Job Execution completed", "timeMs", evalContext.GetDurationMs(), "alertId", evalContext.Rule.ID, "name", evalContext.Rule.Name, "firing", evalContext.Firing, "attemptID", attemptID) close(attemptChan) diff --git a/pkg/services/alerting/engine_integration_test.go b/pkg/services/alerting/engine_integration_test.go index de38138b031..cb4051780d4 100644 --- a/pkg/services/alerting/engine_integration_test.go +++ b/pkg/services/alerting/engine_integration_test.go @@ -18,7 +18,8 @@ import ( func TestEngineTimeouts(t *testing.T) { Convey("Alerting engine timeout tests", t, func() { engine := &AlertEngine{} - engine.Init() + err := engine.Init() + So(err, ShouldBeNil) setting.AlertingNotificationTimeout = 30 * time.Second setting.AlertingMaxAttempts = 3 engine.resultHandler = &FakeResultHandler{} @@ -36,7 +37,8 @@ func TestEngineTimeouts(t *testing.T) { engine.evalHandler = evalHandler engine.resultHandler = resultHandler - engine.processJobWithRetry(context.TODO(), job) + err := engine.processJobWithRetry(context.TODO(), job) + So(err, ShouldBeNil) So(evalHandler.EvalSucceed, ShouldEqual, true) So(resultHandler.ResultHandleSucceed, ShouldEqual, true) diff --git a/pkg/services/alerting/engine_test.go b/pkg/services/alerting/engine_test.go index b444cb18927..fe866ee2f62 100644 --- a/pkg/services/alerting/engine_test.go +++ b/pkg/services/alerting/engine_test.go @@ -40,7 +40,8 @@ func (handler *FakeResultHandler) handle(evalContext *EvalContext) error { func TestEngineProcessJob(t *testing.T) { Convey("Alerting engine job processing", t, func() { engine := &AlertEngine{} - engine.Init() + err := engine.Init() + So(err, ShouldBeNil) setting.AlertingEvaluationTimeout = 30 * time.Second setting.AlertingNotificationTimeout = 30 * time.Second setting.AlertingMaxAttempts = 3 @@ -99,7 +100,8 @@ func TestEngineProcessJob(t *testing.T) { evalHandler := NewFakeEvalHandler(0) engine.evalHandler = evalHandler - engine.processJobWithRetry(context.TODO(), job) + err := engine.processJobWithRetry(context.TODO(), job) + So(err, ShouldBeNil) So(evalHandler.CallNb, ShouldEqual, expectedAttempts) }) @@ -108,7 +110,8 @@ func TestEngineProcessJob(t *testing.T) { evalHandler := NewFakeEvalHandler(1) engine.evalHandler = evalHandler - engine.processJobWithRetry(context.TODO(), job) + err := engine.processJobWithRetry(context.TODO(), job) + So(err, ShouldBeNil) So(evalHandler.CallNb, ShouldEqual, expectedAttempts) }) @@ -117,7 +120,8 @@ func TestEngineProcessJob(t *testing.T) { evalHandler := NewFakeEvalHandler(expectedAttempts) engine.evalHandler = evalHandler - engine.processJobWithRetry(context.TODO(), job) + err := engine.processJobWithRetry(context.TODO(), job) + So(err, ShouldBeNil) So(evalHandler.CallNb, ShouldEqual, expectedAttempts) }) }) diff --git a/pkg/services/alerting/notifiers/telegram.go b/pkg/services/alerting/notifiers/telegram.go index be354bc2733..74f1afbeb75 100644 --- a/pkg/services/alerting/notifiers/telegram.go +++ b/pkg/services/alerting/notifiers/telegram.go @@ -89,19 +89,20 @@ func NewTelegramNotifier(model *models.AlertNotification) (alerting.Notifier, er }, nil } -func (tn *TelegramNotifier) buildMessage(evalContext *alerting.EvalContext, sendImageInline bool) *models.SendWebhookSync { +func (tn *TelegramNotifier) buildMessage(evalContext *alerting.EvalContext, sendImageInline bool) (*models.SendWebhookSync, error) { if sendImageInline { cmd, err := tn.buildMessageInlineImage(evalContext) if err == nil { - return cmd + return cmd, nil } + tn.log.Error("Could not generate Telegram message with inline image.", "err", err) } return tn.buildMessageLinkedImage(evalContext) } -func (tn *TelegramNotifier) buildMessageLinkedImage(evalContext *alerting.EvalContext) *models.SendWebhookSync { +func (tn *TelegramNotifier) buildMessageLinkedImage(evalContext *alerting.EvalContext) (*models.SendWebhookSync, error) { message := fmt.Sprintf("%s\nState: %s\nMessage: %s\n", evalContext.GetNotificationTitle(), evalContext.Rule.Name, evalContext.Rule.Message) ruleURL, err := evalContext.GetRuleURL() @@ -118,11 +119,17 @@ func (tn *TelegramNotifier) buildMessageLinkedImage(evalContext *alerting.EvalCo message = message + fmt.Sprintf("\nMetrics:%s", metrics) } - cmd := tn.generateTelegramCmd(message, "text", "sendMessage", func(w *multipart.Writer) { - fw, _ := w.CreateFormField("parse_mode") - fw.Write([]byte("html")) + return tn.generateTelegramCmd(message, "text", "sendMessage", func(w *multipart.Writer) { + fw, err := w.CreateFormField("parse_mode") + if err != nil { + tn.log.Error("Failed to create form file", "err", err) + return + } + + if _, err := fw.Write([]byte("html")); err != nil { + tn.log.Error("Failed to write to form field", "err", err) + } }) - return cmd } func (tn *TelegramNotifier) buildMessageInlineImage(evalContext *alerting.EvalContext) (*models.SendWebhookSync, error) { @@ -149,22 +156,38 @@ func (tn *TelegramNotifier) buildMessageInlineImage(evalContext *alerting.EvalCo metrics := generateMetricsMessage(evalContext) message := generateImageCaption(evalContext, ruleURL, metrics) - cmd := tn.generateTelegramCmd(message, "caption", "sendPhoto", func(w *multipart.Writer) { - fw, _ := w.CreateFormFile("photo", evalContext.ImageOnDiskPath) - io.Copy(fw, imageFile) + return tn.generateTelegramCmd(message, "caption", "sendPhoto", func(w *multipart.Writer) { + fw, err := w.CreateFormFile("photo", evalContext.ImageOnDiskPath) + if err != nil { + tn.log.Error("Failed to create form file", "err", err) + return + } + + if _, err := io.Copy(fw, imageFile); err != nil { + tn.log.Error("Failed to write to form file", "err", err) + } }) - return cmd, nil } -func (tn *TelegramNotifier) generateTelegramCmd(message string, messageField string, apiAction string, extraConf func(writer *multipart.Writer)) *models.SendWebhookSync { +func (tn *TelegramNotifier) generateTelegramCmd(message string, messageField string, apiAction string, extraConf func(writer *multipart.Writer)) (*models.SendWebhookSync, error) { var body bytes.Buffer w := multipart.NewWriter(&body) - fw, _ := w.CreateFormField("chat_id") - fw.Write([]byte(tn.ChatID)) + fw, err := w.CreateFormField("chat_id") + if err != nil { + return nil, err + } + if _, err := fw.Write([]byte(tn.ChatID)); err != nil { + return nil, err + } - fw, _ = w.CreateFormField(messageField) - fw.Write([]byte(message)) + fw, err = w.CreateFormField(messageField) + if err != nil { + return nil, err + } + if _, err := fw.Write([]byte(message)); err != nil { + return nil, err + } extraConf(w) @@ -181,7 +204,7 @@ func (tn *TelegramNotifier) generateTelegramCmd(message string, messageField str "Content-Type": w.FormDataContentType(), }, } - return cmd + return cmd, nil } func generateMetricsMessage(evalContext *alerting.EvalContext) string { @@ -232,10 +255,14 @@ func appendIfPossible(message string, extra string, sizeLimit int) string { // Notify send an alert notification to Telegram. func (tn *TelegramNotifier) Notify(evalContext *alerting.EvalContext) error { var cmd *models.SendWebhookSync + var err error if evalContext.ImagePublicURL == "" && tn.UploadImage { - cmd = tn.buildMessage(evalContext, true) + cmd, err = tn.buildMessage(evalContext, true) } else { - cmd = tn.buildMessage(evalContext, false) + cmd, err = tn.buildMessage(evalContext, false) + } + if err != nil { + return err } if err := bus.DispatchCtx(evalContext.Ctx, cmd); err != nil { diff --git a/pkg/services/alerting/result_handler.go b/pkg/services/alerting/result_handler.go index 62c2d9d7e58..4a5beb36065 100644 --- a/pkg/services/alerting/result_handler.go +++ b/pkg/services/alerting/result_handler.go @@ -1,6 +1,7 @@ package alerting import ( + "context" "time" "github.com/grafana/grafana/pkg/bus" @@ -8,6 +9,7 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/infra/metrics" "github.com/grafana/grafana/pkg/models" + "golang.org/x/xerrors" "github.com/grafana/grafana/pkg/services/annotations" "github.com/grafana/grafana/pkg/services/rendering" @@ -98,6 +100,15 @@ func (handler *defaultResultHandler) handle(evalContext *EvalContext) error { } } - handler.notifier.SendIfNeeded(evalContext) + if err := handler.notifier.SendIfNeeded(evalContext); err != nil { + if xerrors.Is(err, context.Canceled) { + handler.log.Debug("handler.notifier.SendIfNeeded returned context.Canceled") + } else if xerrors.Is(err, context.DeadlineExceeded) { + handler.log.Debug("handler.notifier.SendIfNeeded returned context.DeadlineExceeded") + } else { + handler.log.Error("handler.notifier.SendIfNeeded failed", "err", err) + } + } + return nil } diff --git a/pkg/services/auth/auth_token.go b/pkg/services/auth/auth_token.go index 5ab255e280f..da6ab6628ff 100644 --- a/pkg/services/auth/auth_token.go +++ b/pkg/services/auth/auth_token.go @@ -201,7 +201,10 @@ func (s *UserAuthTokenService) TryRotateToken(ctx context.Context, token *models return false, nil } - model := userAuthTokenFromUserToken(token) + model, err := userAuthTokenFromUserToken(token) + if err != nil { + return false, err + } now := getTime() @@ -262,7 +265,9 @@ func (s *UserAuthTokenService) TryRotateToken(ctx context.Context, token *models s.log.Debug("auth token rotated", "affected", affected, "auth_token_id", model.Id, "userId", model.UserId) if affected > 0 { model.UnhashedToken = newToken - model.toUserToken(token) + if err := model.toUserToken(token); err != nil { + return false, err + } return true, nil } @@ -274,10 +279,12 @@ func (s *UserAuthTokenService) RevokeToken(ctx context.Context, token *models.Us return models.ErrUserTokenNotFound } - model := userAuthTokenFromUserToken(token) + model, err := userAuthTokenFromUserToken(token) + if err != nil { + return err + } var rowsAffected int64 - var err error err = s.SQLStore.WithDbSession(ctx, func(dbSession *sqlstore.DBSession) error { rowsAffected, err = dbSession.Delete(model) return err @@ -347,7 +354,6 @@ func (s *UserAuthTokenService) BatchRevokeAllUserTokens(ctx context.Context, use } func (s *UserAuthTokenService) GetUserToken(ctx context.Context, userId, userTokenId int64) (*models.UserToken, error) { - var result models.UserToken err := s.SQLStore.WithDbSession(ctx, func(dbSession *sqlstore.DBSession) error { var token userAuthToken @@ -360,8 +366,7 @@ func (s *UserAuthTokenService) GetUserToken(ctx context.Context, userId, userTok return models.ErrUserTokenNotFound } - token.toUserToken(&result) - return nil + return token.toUserToken(&result) }) return &result, err @@ -384,7 +389,9 @@ func (s *UserAuthTokenService) GetUserTokens(ctx context.Context, userId int64) for _, token := range tokens { var userToken models.UserToken - token.toUserToken(&userToken) + if err := token.toUserToken(&userToken); err != nil { + return err + } result = append(result, &userToken) } diff --git a/pkg/services/auth/auth_token_test.go b/pkg/services/auth/auth_token_test.go index bf12d914e97..c898a68a302 100644 --- a/pkg/services/auth/auth_token_test.go +++ b/pkg/services/auth/auth_token_test.go @@ -124,7 +124,8 @@ func TestUserAuthToken(t *testing.T) { for i := 0; i < 3; i++ { userId := userID + int64(i+1) userIds = append(userIds, userId) - userAuthTokenService.CreateToken(context.Background(), userId, "192.168.10.11:1234", "some user agent") + _, err := userAuthTokenService.CreateToken(context.Background(), userId, "192.168.10.11:1234", "some user agent") + So(err, ShouldBeNil) } err := userAuthTokenService.BatchRevokeAllUserTokens(context.Background(), userIds) @@ -441,7 +442,8 @@ func TestUserAuthToken(t *testing.T) { utMap := utJSON.MustMap() var uat userAuthToken - uat.fromUserToken(&ut) + err = uat.fromUserToken(&ut) + So(err, ShouldBeNil) uatBytes, err := json.Marshal(uat) So(err, ShouldBeNil) uatJSON, err := simplejson.NewJson(uatBytes) diff --git a/pkg/services/auth/model.go b/pkg/services/auth/model.go index 36652e70436..2948361a70c 100644 --- a/pkg/services/auth/model.go +++ b/pkg/services/auth/model.go @@ -21,10 +21,10 @@ type userAuthToken struct { UnhashedToken string `xorm:"-"` } -func userAuthTokenFromUserToken(ut *models.UserToken) *userAuthToken { +func userAuthTokenFromUserToken(ut *models.UserToken) (*userAuthToken, error) { var uat userAuthToken - uat.fromUserToken(ut) - return &uat + err := uat.fromUserToken(ut) + return &uat, err } func (uat *userAuthToken) fromUserToken(ut *models.UserToken) error { diff --git a/pkg/services/auth/token_cleanup.go b/pkg/services/auth/token_cleanup.go index 671d3d7f5b7..683a9ee47b6 100644 --- a/pkg/services/auth/token_cleanup.go +++ b/pkg/services/auth/token_cleanup.go @@ -13,9 +13,10 @@ func (srv *UserAuthTokenService) Run(ctx context.Context) error { maxLifetime := time.Duration(srv.Cfg.LoginMaxLifetimeDays) * 24 * time.Hour err := srv.ServerLockService.LockAndExecute(ctx, "cleanup expired auth tokens", time.Hour*12, func() { - srv.deleteExpiredTokens(ctx, maxInactiveLifetime, maxLifetime) + if _, err := srv.deleteExpiredTokens(ctx, maxInactiveLifetime, maxLifetime); err != nil { + srv.log.Error("An error occurred while deleting expired tokens", "err", err) + } }) - if err != nil { srv.log.Error("failed to lock and execute cleanup of expired auth token", "error", err) } @@ -24,9 +25,10 @@ func (srv *UserAuthTokenService) Run(ctx context.Context) error { select { case <-ticker.C: err := srv.ServerLockService.LockAndExecute(ctx, "cleanup expired auth tokens", time.Hour*12, func() { - srv.deleteExpiredTokens(ctx, maxInactiveLifetime, maxLifetime) + if _, err := srv.deleteExpiredTokens(ctx, maxInactiveLifetime, maxLifetime); err != nil { + srv.log.Error("An error occurred while deleting expired tokens", "err", err) + } }) - if err != nil { srv.log.Error("failed to lock and execute cleanup of expired auth token", "error", err) } diff --git a/pkg/services/cleanup/cleanup.go b/pkg/services/cleanup/cleanup.go index 6e07a76f800..6eb3bd74d3e 100644 --- a/pkg/services/cleanup/cleanup.go +++ b/pkg/services/cleanup/cleanup.go @@ -40,10 +40,13 @@ func (srv *CleanUpService) Run(ctx context.Context) error { srv.cleanUpTmpFiles() srv.deleteExpiredSnapshots() srv.deleteExpiredDashboardVersions() - srv.ServerLockService.LockAndExecute(ctx, "delete old login attempts", time.Minute*10, func() { - srv.deleteOldLoginAttempts() - }) - + err := srv.ServerLockService.LockAndExecute(ctx, "delete old login attempts", + time.Minute*10, func() { + srv.deleteOldLoginAttempts() + }) + if err != nil { + srv.log.Error("failed to lock and execute cleanup of old login attempts", "error", err) + } case <-ctx.Done(): return ctx.Err() } diff --git a/pkg/services/notifications/codes.go b/pkg/services/notifications/codes.go index 4225c89af1a..ae66aa1d1bc 100644 --- a/pkg/services/notifications/codes.go +++ b/pkg/services/notifications/codes.go @@ -16,7 +16,7 @@ const timeLimitCodeLength = 12 + 6 + 40 // create a time limit code // code format: 12 length date time string + 6 minutes string + 40 sha1 encoded string -func createTimeLimitCode(data string, minutes int, startInf interface{}) string { +func createTimeLimitCode(data string, minutes int, startInf interface{}) (string, error) { format := "200601021504" var start, end time.Time @@ -38,17 +38,20 @@ func createTimeLimitCode(data string, minutes int, startInf interface{}) string // create sha1 encode string sh := sha1.New() - sh.Write([]byte(data + setting.SecretKey + startStr + endStr + com.ToStr(minutes))) + if _, err := sh.Write([]byte(data + setting.SecretKey + startStr + endStr + + com.ToStr(minutes))); err != nil { + return "", err + } encoded := hex.EncodeToString(sh.Sum(nil)) code := fmt.Sprintf("%s%06d%s", startStr, minutes, encoded) - return code + return code, nil } // verify time limit code -func validateUserEmailCode(user *m.User, code string) bool { +func validateUserEmailCode(user *m.User, code string) (bool, error) { if len(code) <= 18 { - return false + return false, nil } minutes := setting.EmailCodeValidMinutes @@ -63,18 +66,21 @@ func validateUserEmailCode(user *m.User, code string) bool { // right active code data := com.ToStr(user.Id) + user.Email + user.Login + user.Password + user.Rands - retCode := createTimeLimitCode(data, minutes, start) + retCode, err := createTimeLimitCode(data, minutes, start) + if err != nil { + return false, err + } fmt.Printf("code : %s\ncode2: %s", retCode, code) if retCode == code && minutes > 0 { // check time is expired or not before, _ := time.ParseInLocation("200601021504", start, time.Local) now := time.Now() if before.Add(time.Minute*time.Duration(minutes)).Unix() > now.Unix() { - return true + return true, nil } } - return false + return false, nil } func getLoginForEmailCode(code string) string { @@ -88,12 +94,15 @@ func getLoginForEmailCode(code string) string { return string(b) } -func createUserEmailCode(u *m.User, startInf interface{}) string { +func createUserEmailCode(u *m.User, startInf interface{}) (string, error) { minutes := setting.EmailCodeValidMinutes data := com.ToStr(u.Id) + u.Email + u.Login + u.Password + u.Rands - code := createTimeLimitCode(data, minutes, startInf) + code, err := createTimeLimitCode(data, minutes, startInf) + if err != nil { + return "", err + } // add tail hex username code += hex.EncodeToString([]byte(u.Login)) - return code + return code, nil } diff --git a/pkg/services/notifications/codes_test.go b/pkg/services/notifications/codes_test.go index be1fc91153b..645be78cb79 100644 --- a/pkg/services/notifications/codes_test.go +++ b/pkg/services/notifications/codes_test.go @@ -14,7 +14,8 @@ func TestEmailCodes(t *testing.T) { setting.EmailCodeValidMinutes = 120 user := &m.User{Id: 10, Email: "t@a.com", Login: "asd", Password: "1", Rands: "2"} - code := createUserEmailCode(user, nil) + code, err := createUserEmailCode(user, nil) + So(err, ShouldBeNil) Convey("getLoginForCode should return login", func() { login := getLoginForEmailCode(code) @@ -22,12 +23,16 @@ func TestEmailCodes(t *testing.T) { }) Convey("Can verify valid code", func() { - So(validateUserEmailCode(user, code), ShouldBeTrue) + isValid, err := validateUserEmailCode(user, code) + So(err, ShouldBeNil) + So(isValid, ShouldBeTrue) }) Convey("Cannot verify in-valid code", func() { code = "ASD" - So(validateUserEmailCode(user, code), ShouldBeFalse) + isValid, err := validateUserEmailCode(user, code) + So(err, ShouldBeNil) + So(isValid, ShouldBeFalse) }) }) diff --git a/pkg/services/notifications/notifications.go b/pkg/services/notifications/notifications.go index 3168fdcd5e9..49e1461f2bd 100644 --- a/pkg/services/notifications/notifications.go +++ b/pkg/services/notifications/notifications.go @@ -147,11 +147,15 @@ func (ns *NotificationService) sendEmailCommandHandler(cmd *m.SendEmailCommand) } func (ns *NotificationService) sendResetPasswordEmail(cmd *m.SendResetPasswordEmailCommand) error { + code, err := createUserEmailCode(cmd.User, nil) + if err != nil { + return err + } return ns.sendEmailCommandHandler(&m.SendEmailCommand{ To: []string{cmd.User.Email}, Template: tmplResetPassword, Data: map[string]interface{}{ - "Code": createUserEmailCode(cmd.User, nil), + "Code": code, "Name": cmd.User.NameOrFallback(), }, }) @@ -168,7 +172,11 @@ func (ns *NotificationService) validateResetPasswordCode(query *m.ValidateResetP return err } - if !validateUserEmailCode(userQuery.Result, query.Code) { + validEmailCode, err := validateUserEmailCode(userQuery.Result, query.Code) + if err != nil { + return err + } + if !validEmailCode { return m.ErrInvalidEmailCode } diff --git a/pkg/services/notifications/send_email_integration_test.go b/pkg/services/notifications/send_email_integration_test.go index 4459d039142..42a4d80904a 100644 --- a/pkg/services/notifications/send_email_integration_test.go +++ b/pkg/services/notifications/send_email_integration_test.go @@ -61,7 +61,8 @@ func TestEmailIntegrationTest(t *testing.T) { sentMsg := <-ns.mailQueue So(sentMsg.From, ShouldEqual, "Grafana Admin ") So(sentMsg.To[0], ShouldEqual, "asdf@asdf.com") - ioutil.WriteFile("../../../tmp/test_email.html", []byte(sentMsg.Body), 0777) + err = ioutil.WriteFile("../../../tmp/test_email.html", []byte(sentMsg.Body), 0777) + So(err, ShouldBeNil) }) }) } diff --git a/pkg/services/notifications/webhook.go b/pkg/services/notifications/webhook.go index 3e2d097847f..7cc74409278 100644 --- a/pkg/services/notifications/webhook.go +++ b/pkg/services/notifications/webhook.go @@ -77,7 +77,9 @@ func (ns *NotificationService) sendWebRequestSync(ctx context.Context, webhook * if resp.StatusCode/100 == 2 { // flushing the body enables the transport to reuse the same connection - io.Copy(ioutil.Discard, resp.Body) + if _, err := io.Copy(ioutil.Discard, resp.Body); err != nil { + ns.log.Error("Failed to copy resp.Body to ioutil.Discard", "err", err) + } return nil } diff --git a/pkg/services/sqlstore/alert.go b/pkg/services/sqlstore/alert.go index 47258677658..8714d89e8d8 100644 --- a/pkg/services/sqlstore/alert.go +++ b/pkg/services/sqlstore/alert.go @@ -151,10 +151,16 @@ func HandleAlertsQuery(query *m.GetAlertsQuery) error { func deleteAlertDefinition(dashboardId int64, sess *DBSession) error { alerts := make([]*m.Alert, 0) - sess.Where("dashboard_id = ?", dashboardId).Find(&alerts) + if err := sess.Where("dashboard_id = ?", dashboardId).Find(&alerts); err != nil { + return err + } for _, alert := range alerts { - deleteAlertByIdInternal(alert.Id, "Dashboard deleted", sess) + if err := deleteAlertByIdInternal(alert.Id, "Dashboard deleted", sess); err != nil { + // If we return an error, the current transaction gets rolled back, so no use + // trying to delete more + return err + } } return nil @@ -251,7 +257,11 @@ func deleteMissingAlerts(alerts []*m.Alert, cmd *m.SaveAlertsCommand, sess *DBSe } if missing { - deleteAlertByIdInternal(missingAlert.Id, "Removed from dashboard", sess) + if err := deleteAlertByIdInternal(missingAlert.Id, "Removed from dashboard", sess); err != nil { + // No use trying to delete more, since we're in a transaction and it will be + // rolled back on error. + return err + } } } diff --git a/pkg/services/sqlstore/alert_notification.go b/pkg/services/sqlstore/alert_notification.go index f054f2b94d0..d570b4e0166 100644 --- a/pkg/services/sqlstore/alert_notification.go +++ b/pkg/services/sqlstore/alert_notification.go @@ -392,10 +392,11 @@ func SetAlertNotificationStateToCompleteCommand(ctx context.Context, cmd *m.SetA return inTransactionCtx(ctx, func(sess *DBSession) error { version := cmd.Version var current m.AlertNotificationState - sess.ID(cmd.Id).Get(¤t) + if _, err := sess.ID(cmd.Id).Get(¤t); err != nil { + return err + } newVersion := cmd.Version + 1 - sql := `UPDATE alert_notification_state SET state = ?, version = ?, @@ -404,7 +405,6 @@ func SetAlertNotificationStateToCompleteCommand(ctx context.Context, cmd *m.SetA id = ?` _, err := sess.Exec(sql, m.AlertNotificationStateCompleted, newVersion, timeNow().Unix(), cmd.Id) - if err != nil { return err } diff --git a/pkg/services/sqlstore/alert_test.go b/pkg/services/sqlstore/alert_test.go index 3ea03caa874..66459a11c7a 100644 --- a/pkg/services/sqlstore/alert_test.go +++ b/pkg/services/sqlstore/alert_test.go @@ -73,7 +73,8 @@ func TestAlertingDataAccess(t *testing.T) { stateDateBeforePause := alert.NewStateDate Convey("can pause all alerts", func() { - pauseAllAlerts(true) + err := pauseAllAlerts(true) + So(err, ShouldBeNil) Convey("cannot updated paused alert", func() { cmd := &m.SetAlertStateCommand{ @@ -98,7 +99,8 @@ func TestAlertingDataAccess(t *testing.T) { }) Convey("unpausing alerts should update their NewStateDate again", func() { - pauseAllAlerts(false) + err := pauseAllAlerts(false) + So(err, ShouldBeNil) alert, _ = getAlertById(1) stateDateAfterUnpause := alert.NewStateDate So(stateDateBeforePause, ShouldHappenBefore, stateDateAfterUnpause) @@ -241,13 +243,13 @@ func TestAlertingDataAccess(t *testing.T) { UserId: 1, } - SaveAlerts(&cmd) + err = SaveAlerts(&cmd) + So(err, ShouldBeNil) err = DeleteDashboard(&m.DeleteDashboardCommand{ OrgId: 1, Id: testDash.Id, }) - So(err, ShouldBeNil) Convey("Alerts should be removed", func() { @@ -275,10 +277,12 @@ func TestPausingAlerts(t *testing.T) { stateDateBeforePause := alert.NewStateDate stateDateAfterPause := stateDateBeforePause Convey("when paused", func() { - pauseAlert(testDash.OrgId, 1, true) + _, err := pauseAlert(testDash.OrgId, 1, true) + So(err, ShouldBeNil) Convey("the NewStateDate should be updated", func() { - alert, _ := getAlertById(1) + alert, err := getAlertById(1) + So(err, ShouldBeNil) stateDateAfterPause = alert.NewStateDate So(stateDateBeforePause, ShouldHappenBefore, stateDateAfterPause) @@ -286,10 +290,12 @@ func TestPausingAlerts(t *testing.T) { }) Convey("when unpaused", func() { - pauseAlert(testDash.OrgId, 1, false) + _, err := pauseAlert(testDash.OrgId, 1, false) + So(err, ShouldBeNil) Convey("the NewStateDate should be updated again", func() { - alert, _ := getAlertById(1) + alert, err := getAlertById(1) + So(err, ShouldBeNil) stateDateAfterUnpause := alert.NewStateDate So(stateDateAfterPause, ShouldHappenBefore, stateDateAfterUnpause) diff --git a/pkg/services/sqlstore/dashboard_folder_test.go b/pkg/services/sqlstore/dashboard_folder_test.go index 33b6c566f54..9ee801286e3 100644 --- a/pkg/services/sqlstore/dashboard_folder_test.go +++ b/pkg/services/sqlstore/dashboard_folder_test.go @@ -38,7 +38,13 @@ func TestDashboardFolderDataAccess(t *testing.T) { Convey("and acl is set for dashboard folder", func() { var otherUser int64 = 999 - testHelperUpdateDashboardAcl(folder.Id, models.DashboardAcl{DashboardId: folder.Id, OrgId: 1, UserId: otherUser, Permission: models.PERMISSION_EDIT}) + err := testHelperUpdateDashboardAcl(folder.Id, models.DashboardAcl{ + DashboardId: folder.Id, + OrgId: 1, + UserId: otherUser, + Permission: models.PERMISSION_EDIT, + }) + So(err, ShouldBeNil) Convey("should not return folder", func() { query := &search.FindPersistedDashboardsQuery{ @@ -46,14 +52,17 @@ func TestDashboardFolderDataAccess(t *testing.T) { OrgId: 1, DashboardIds: []int64{folder.Id, dashInRoot.Id}, } err := SearchDashboards(query) - So(err, ShouldBeNil) + So(len(query.Result), ShouldEqual, 1) So(query.Result[0].Id, ShouldEqual, dashInRoot.Id) }) Convey("when the user is given permission", func() { - testHelperUpdateDashboardAcl(folder.Id, models.DashboardAcl{DashboardId: folder.Id, OrgId: 1, UserId: currentUser.Id, Permission: models.PERMISSION_EDIT}) + err := testHelperUpdateDashboardAcl(folder.Id, models.DashboardAcl{ + DashboardId: folder.Id, OrgId: 1, UserId: currentUser.Id, Permission: models.PERMISSION_EDIT, + }) + So(err, ShouldBeNil) Convey("should be able to access folder", func() { query := &search.FindPersistedDashboardsQuery{ @@ -91,11 +100,17 @@ func TestDashboardFolderDataAccess(t *testing.T) { Convey("and acl is set for dashboard child and folder has all permissions removed", func() { var otherUser int64 = 999 - testHelperUpdateDashboardAcl(folder.Id) - testHelperUpdateDashboardAcl(childDash.Id, models.DashboardAcl{DashboardId: folder.Id, OrgId: 1, UserId: otherUser, Permission: models.PERMISSION_EDIT}) + err := testHelperUpdateDashboardAcl(folder.Id) + So(err, ShouldBeNil) + err = testHelperUpdateDashboardAcl(childDash.Id, models.DashboardAcl{ + DashboardId: folder.Id, OrgId: 1, UserId: otherUser, Permission: models.PERMISSION_EDIT, + }) + So(err, ShouldBeNil) Convey("should not return folder or child", func() { - query := &search.FindPersistedDashboardsQuery{SignedInUser: &models.SignedInUser{UserId: currentUser.Id, OrgId: 1, OrgRole: models.ROLE_VIEWER}, OrgId: 1, DashboardIds: []int64{folder.Id, childDash.Id, dashInRoot.Id}} + query := &search.FindPersistedDashboardsQuery{ + SignedInUser: &models.SignedInUser{UserId: currentUser.Id, OrgId: 1, OrgRole: models.ROLE_VIEWER}, OrgId: 1, DashboardIds: []int64{folder.Id, childDash.Id, dashInRoot.Id}, + } err := SearchDashboards(query) So(err, ShouldBeNil) So(len(query.Result), ShouldEqual, 1) @@ -103,7 +118,8 @@ func TestDashboardFolderDataAccess(t *testing.T) { }) Convey("when the user is given permission to child", func() { - testHelperUpdateDashboardAcl(childDash.Id, models.DashboardAcl{DashboardId: childDash.Id, OrgId: 1, UserId: currentUser.Id, Permission: models.PERMISSION_EDIT}) + err := testHelperUpdateDashboardAcl(childDash.Id, models.DashboardAcl{DashboardId: childDash.Id, OrgId: 1, UserId: currentUser.Id, Permission: models.PERMISSION_EDIT}) + So(err, ShouldBeNil) Convey("should be able to search for child dashboard but not folder", func() { query := &search.FindPersistedDashboardsQuery{SignedInUser: &models.SignedInUser{UserId: currentUser.Id, OrgId: 1, OrgRole: models.ROLE_VIEWER}, OrgId: 1, DashboardIds: []int64{folder.Id, childDash.Id, dashInRoot.Id}} @@ -162,7 +178,10 @@ func TestDashboardFolderDataAccess(t *testing.T) { Convey("and acl is set for one dashboard folder", func() { var otherUser int64 = 999 - testHelperUpdateDashboardAcl(folder1.Id, models.DashboardAcl{DashboardId: folder1.Id, OrgId: 1, UserId: otherUser, Permission: models.PERMISSION_EDIT}) + err := testHelperUpdateDashboardAcl(folder1.Id, models.DashboardAcl{ + DashboardId: folder1.Id, OrgId: 1, UserId: otherUser, Permission: models.PERMISSION_EDIT, + }) + So(err, ShouldBeNil) Convey("and a dashboard is moved from folder without acl to the folder with an acl", func() { moveDashboard(1, childDash2.Data, folder1.Id) @@ -199,7 +218,11 @@ func TestDashboardFolderDataAccess(t *testing.T) { }) Convey("and a dashboard with an acl is moved to the folder without an acl", func() { - testHelperUpdateDashboardAcl(childDash1.Id, models.DashboardAcl{DashboardId: childDash1.Id, OrgId: 1, UserId: otherUser, Permission: models.PERMISSION_EDIT}) + err := testHelperUpdateDashboardAcl(childDash1.Id, models.DashboardAcl{ + DashboardId: childDash1.Id, OrgId: 1, UserId: otherUser, Permission: models.PERMISSION_EDIT, + }) + So(err, ShouldBeNil) + moveDashboard(1, childDash1.Data, folder2.Id) Convey("should return folder without acl but not the dashboard with acl", func() { @@ -318,9 +341,12 @@ func TestDashboardFolderDataAccess(t *testing.T) { }) Convey("Should have write access to one dashboard folder if default role changed to view for one folder", func() { - testHelperUpdateDashboardAcl(folder1.Id, models.DashboardAcl{DashboardId: folder1.Id, OrgId: 1, UserId: editorUser.Id, Permission: models.PERMISSION_VIEW}) + err := testHelperUpdateDashboardAcl(folder1.Id, models.DashboardAcl{ + DashboardId: folder1.Id, OrgId: 1, UserId: editorUser.Id, Permission: models.PERMISSION_VIEW, + }) + So(err, ShouldBeNil) - err := SearchDashboards(&query) + err = SearchDashboards(&query) So(err, ShouldBeNil) So(len(query.Result), ShouldEqual, 1) @@ -379,9 +405,12 @@ func TestDashboardFolderDataAccess(t *testing.T) { }) Convey("Should be able to get one dashboard folder if default role changed to edit for one folder", func() { - testHelperUpdateDashboardAcl(folder1.Id, models.DashboardAcl{DashboardId: folder1.Id, OrgId: 1, UserId: viewerUser.Id, Permission: models.PERMISSION_EDIT}) + err := testHelperUpdateDashboardAcl(folder1.Id, models.DashboardAcl{ + DashboardId: folder1.Id, OrgId: 1, UserId: viewerUser.Id, Permission: models.PERMISSION_EDIT, + }) + So(err, ShouldBeNil) - err := SearchDashboards(&query) + err = SearchDashboards(&query) So(err, ShouldBeNil) So(len(query.Result), ShouldEqual, 1) @@ -407,7 +436,10 @@ func TestDashboardFolderDataAccess(t *testing.T) { }) Convey("and admin permission is given for user with org role viewer in one dashboard folder", func() { - testHelperUpdateDashboardAcl(folder1.Id, models.DashboardAcl{DashboardId: folder1.Id, OrgId: 1, UserId: viewerUser.Id, Permission: models.PERMISSION_ADMIN}) + err := testHelperUpdateDashboardAcl(folder1.Id, models.DashboardAcl{ + DashboardId: folder1.Id, OrgId: 1, UserId: viewerUser.Id, Permission: models.PERMISSION_ADMIN, + }) + So(err, ShouldBeNil) Convey("should have edit permission in folders", func() { query := &models.HasEditPermissionInFoldersQuery{ @@ -420,7 +452,10 @@ func TestDashboardFolderDataAccess(t *testing.T) { }) Convey("and edit permission is given for user with org role viewer in one dashboard folder", func() { - testHelperUpdateDashboardAcl(folder1.Id, models.DashboardAcl{DashboardId: folder1.Id, OrgId: 1, UserId: viewerUser.Id, Permission: models.PERMISSION_EDIT}) + err := testHelperUpdateDashboardAcl(folder1.Id, models.DashboardAcl{ + DashboardId: folder1.Id, OrgId: 1, UserId: viewerUser.Id, Permission: models.PERMISSION_EDIT, + }) + So(err, ShouldBeNil) Convey("should have edit permission in folders", func() { query := &models.HasEditPermissionInFoldersQuery{ diff --git a/pkg/services/sqlstore/dashboard_snapshot_test.go b/pkg/services/sqlstore/dashboard_snapshot_test.go index 09b3fb7607f..1589919c8e8 100644 --- a/pkg/services/sqlstore/dashboard_snapshot_test.go +++ b/pkg/services/sqlstore/dashboard_snapshot_test.go @@ -138,7 +138,8 @@ func TestDeleteExpiredSnapshots(t *testing.T) { OrgId: 1, SignedInUser: &m.SignedInUser{OrgRole: m.ROLE_ADMIN}, } - SearchDashboardSnapshots(&query) + err = SearchDashboardSnapshots(&query) + So(err, ShouldBeNil) So(len(query.Result), ShouldEqual, 1) So(query.Result[0].Key, ShouldEqual, notExpiredsnapshot.Key) diff --git a/pkg/services/sqlstore/dashboard_test.go b/pkg/services/sqlstore/dashboard_test.go index f822d223617..e2c88f70c1b 100644 --- a/pkg/services/sqlstore/dashboard_test.go +++ b/pkg/services/sqlstore/dashboard_test.go @@ -345,15 +345,17 @@ func TestDashboardDataAccess(t *testing.T) { Convey("Given two dashboards, one is starred dashboard by user 10, other starred by user 1", func() { starredDash := insertTestDashboard("starred dash", 1, 0, false) - StarDashboard(&m.StarDashboardCommand{ + err := StarDashboard(&m.StarDashboardCommand{ DashboardId: starredDash.Id, UserId: 10, }) + So(err, ShouldBeNil) - StarDashboard(&m.StarDashboardCommand{ + err = StarDashboard(&m.StarDashboardCommand{ DashboardId: savedDash.Id, UserId: 1, }) + So(err, ShouldBeNil) Convey("Should be able to search for starred dashboards", func() { query := search.FindPersistedDashboardsQuery{ @@ -439,7 +441,8 @@ func createUser(name string, role string, isAdmin bool) m.User { So(err, ShouldBeNil) q1 := m.GetUserOrgListQuery{UserId: currentUserCmd.Result.Id} - GetUserOrgList(&q1) + err = GetUserOrgList(&q1) + So(err, ShouldBeNil) So(q1.Result[0].Role, ShouldEqual, role) return currentUserCmd.Result diff --git a/pkg/services/sqlstore/dashboard_version_test.go b/pkg/services/sqlstore/dashboard_version_test.go index a6403755d05..21f2782d5fc 100644 --- a/pkg/services/sqlstore/dashboard_version_test.go +++ b/pkg/services/sqlstore/dashboard_version_test.go @@ -122,7 +122,8 @@ func TestDeleteExpiredVersions(t *testing.T) { So(err, ShouldBeNil) query := m.GetDashboardVersionsQuery{DashboardId: savedDash.Id, OrgId: 1} - GetDashboardVersions(&query) + err = GetDashboardVersions(&query) + So(err, ShouldBeNil) So(len(query.Result), ShouldEqual, versionsToKeep) // Ensure latest versions were kept @@ -137,7 +138,8 @@ func TestDeleteExpiredVersions(t *testing.T) { So(err, ShouldBeNil) query := m.GetDashboardVersionsQuery{DashboardId: savedDash.Id, OrgId: 1, Limit: versionsToWrite} - GetDashboardVersions(&query) + err = GetDashboardVersions(&query) + So(err, ShouldBeNil) So(len(query.Result), ShouldEqual, versionsToWrite) }) @@ -154,7 +156,8 @@ func TestDeleteExpiredVersions(t *testing.T) { So(err, ShouldBeNil) query := m.GetDashboardVersionsQuery{DashboardId: savedDash.Id, OrgId: 1, Limit: versionsToWriteBigNumber} - GetDashboardVersions(&query) + err = GetDashboardVersions(&query) + So(err, ShouldBeNil) // Ensure we have at least versionsToKeep versions So(len(query.Result), ShouldBeGreaterThanOrEqualTo, versionsToKeep) diff --git a/pkg/services/sqlstore/datasource_test.go b/pkg/services/sqlstore/datasource_test.go index 32555c958d7..97b1348f460 100644 --- a/pkg/services/sqlstore/datasource_test.go +++ b/pkg/services/sqlstore/datasource_test.go @@ -145,7 +145,9 @@ func TestDataAccess(t *testing.T) { err := DeleteDataSourceById(&models.DeleteDataSourceByIdCommand{Id: ds.Id, OrgId: ds.OrgId}) So(err, ShouldBeNil) - GetDataSources(&query) + err = GetDataSources(&query) + So(err, ShouldBeNil) + So(len(query.Result), ShouldEqual, 0) }) @@ -153,7 +155,9 @@ func TestDataAccess(t *testing.T) { err := DeleteDataSourceByName(&models.DeleteDataSourceByNameCommand{Name: ds.Name, OrgId: ds.OrgId}) So(err, ShouldBeNil) - GetDataSources(&query) + err = GetDataSources(&query) + So(err, ShouldBeNil) + So(len(query.Result), ShouldEqual, 0) }) @@ -161,7 +165,9 @@ func TestDataAccess(t *testing.T) { err := DeleteDataSourceById(&models.DeleteDataSourceByIdCommand{Id: ds.Id, OrgId: 123123}) So(err, ShouldBeNil) - GetDataSources(&query) + err = GetDataSources(&query) + So(err, ShouldBeNil) + So(len(query.Result), ShouldEqual, 1) }) }) diff --git a/pkg/services/sqlstore/migrations/annotation_mig.go b/pkg/services/sqlstore/migrations/annotation_mig.go index 70f512ddc4b..959d91e9cbb 100644 --- a/pkg/services/sqlstore/migrations/annotation_mig.go +++ b/pkg/services/sqlstore/migrations/annotation_mig.go @@ -155,6 +155,6 @@ func (m *AddMakeRegionSingleRowMigration) Exec(sess *xorm.Session, mg *Migrator) } } - sess.Exec("DELETE FROM annotation WHERE region_id > 0 AND id <> region_id") - return nil + _, err = sess.Exec("DELETE FROM annotation WHERE region_id > 0 AND id <> region_id") + return err } diff --git a/pkg/services/sqlstore/migrations/migrations_test.go b/pkg/services/sqlstore/migrations/migrations_test.go index ec4cb5fbce1..62ecbaad11d 100644 --- a/pkg/services/sqlstore/migrations/migrations_test.go +++ b/pkg/services/sqlstore/migrations/migrations_test.go @@ -25,7 +25,8 @@ func TestMigrations(t *testing.T) { x, err := xorm.NewEngine(testDB.DriverName, testDB.ConnStr) So(err, ShouldBeNil) - NewDialect(x).CleanDB() + err = NewDialect(x).CleanDB() + So(err, ShouldBeNil) _, err = x.SQL(sql).Get(&r) So(err, ShouldNotBeNil) diff --git a/pkg/services/sqlstore/migrator/migrator.go b/pkg/services/sqlstore/migrator/migrator.go index 68820c7feb6..612a333a722 100644 --- a/pkg/services/sqlstore/migrator/migrator.go +++ b/pkg/services/sqlstore/migrator/migrator.go @@ -6,6 +6,7 @@ import ( _ "github.com/go-sql-driver/mysql" "github.com/go-xorm/xorm" "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/util/errutil" _ "github.com/lib/pq" _ "github.com/mattn/go-sqlite3" ) @@ -99,12 +100,14 @@ func (mg *Migrator) Start() error { if err != nil { mg.Logger.Error("Exec failed", "error", err, "sql", sql) record.Error = err.Error() - sess.Insert(&record) + if _, err := sess.Insert(&record); err != nil { + return err + } return err } record.Success = true - sess.Insert(&record) - return nil + _, err = sess.Insert(&record) + return err }) if err != nil { @@ -158,21 +161,22 @@ func (mg *Migrator) exec(m Migration, sess *xorm.Session) error { type dbTransactionFunc func(sess *xorm.Session) error func (mg *Migrator) inTransaction(callback dbTransactionFunc) error { - var err error - sess := mg.x.NewSession() defer sess.Close() - if err = sess.Begin(); err != nil { + if err := sess.Begin(); err != nil { return err } - err = callback(sess) + if err := callback(sess); err != nil { + if rollErr := sess.Rollback(); err != rollErr { + return errutil.Wrapf(err, "Failed to roll back transaction due to error: %s", rollErr) + } - if err != nil { - sess.Rollback() return err - } else if err = sess.Commit(); err != nil { + } + + if err := sess.Commit(); err != nil { return err } diff --git a/pkg/services/sqlstore/org_test.go b/pkg/services/sqlstore/org_test.go index 577357469a6..85d806ce574 100644 --- a/pkg/services/sqlstore/org_test.go +++ b/pkg/services/sqlstore/org_test.go @@ -51,8 +51,10 @@ func TestAccountDataAccess(t *testing.T) { q1 := m.GetUserOrgListQuery{UserId: ac1cmd.Result.Id} q2 := m.GetUserOrgListQuery{UserId: ac2cmd.Result.Id} - GetUserOrgList(&q1) - GetUserOrgList(&q2) + err = GetUserOrgList(&q1) + So(err, ShouldBeNil) + err = GetUserOrgList(&q2) + So(err, ShouldBeNil) So(q1.Result[0].OrgId, ShouldEqual, q2.Result[0].OrgId) So(q1.Result[0].Role, ShouldEqual, "Viewer") diff --git a/pkg/services/sqlstore/preferences_test.go b/pkg/services/sqlstore/preferences_test.go index f9a839bf5a7..43e1e96d930 100644 --- a/pkg/services/sqlstore/preferences_test.go +++ b/pkg/services/sqlstore/preferences_test.go @@ -23,67 +23,91 @@ func TestPreferencesDataAccess(t *testing.T) { }) Convey("GetPreferencesWithDefaults with saved org and user home dashboard should return user home dashboard", func() { - SavePreferences(&models.SavePreferencesCommand{OrgId: 1, HomeDashboardId: 1}) - SavePreferences(&models.SavePreferencesCommand{OrgId: 1, UserId: 1, HomeDashboardId: 4}) + err := SavePreferences(&models.SavePreferencesCommand{OrgId: 1, HomeDashboardId: 1}) + So(err, ShouldBeNil) + err = SavePreferences(&models.SavePreferencesCommand{OrgId: 1, UserId: 1, HomeDashboardId: 4}) + So(err, ShouldBeNil) query := &models.GetPreferencesWithDefaultsQuery{User: &models.SignedInUser{OrgId: 1, UserId: 1}} - err := GetPreferencesWithDefaults(query) + err = GetPreferencesWithDefaults(query) So(err, ShouldBeNil) So(query.Result.HomeDashboardId, ShouldEqual, 4) }) Convey("GetPreferencesWithDefaults with saved org and other user home dashboard should return org home dashboard", func() { - SavePreferences(&models.SavePreferencesCommand{OrgId: 1, HomeDashboardId: 1}) - SavePreferences(&models.SavePreferencesCommand{OrgId: 1, UserId: 1, HomeDashboardId: 4}) + err := SavePreferences(&models.SavePreferencesCommand{OrgId: 1, HomeDashboardId: 1}) + So(err, ShouldBeNil) + err = SavePreferences(&models.SavePreferencesCommand{OrgId: 1, UserId: 1, HomeDashboardId: 4}) + So(err, ShouldBeNil) query := &models.GetPreferencesWithDefaultsQuery{User: &models.SignedInUser{OrgId: 1, UserId: 2}} - err := GetPreferencesWithDefaults(query) + err = GetPreferencesWithDefaults(query) So(err, ShouldBeNil) So(query.Result.HomeDashboardId, ShouldEqual, 1) }) Convey("GetPreferencesWithDefaults with saved org and teams home dashboard should return last team home dashboard", func() { - SavePreferences(&models.SavePreferencesCommand{OrgId: 1, HomeDashboardId: 1}) - SavePreferences(&models.SavePreferencesCommand{OrgId: 1, TeamId: 2, HomeDashboardId: 2}) - SavePreferences(&models.SavePreferencesCommand{OrgId: 1, TeamId: 3, HomeDashboardId: 3}) + err := SavePreferences(&models.SavePreferencesCommand{OrgId: 1, HomeDashboardId: 1}) + So(err, ShouldBeNil) + err = SavePreferences(&models.SavePreferencesCommand{OrgId: 1, TeamId: 2, HomeDashboardId: 2}) + So(err, ShouldBeNil) + err = SavePreferences(&models.SavePreferencesCommand{OrgId: 1, TeamId: 3, HomeDashboardId: 3}) + So(err, ShouldBeNil) - query := &models.GetPreferencesWithDefaultsQuery{User: &models.SignedInUser{OrgId: 1, Teams: []int64{2, 3}}} - err := GetPreferencesWithDefaults(query) + query := &models.GetPreferencesWithDefaultsQuery{ + User: &models.SignedInUser{OrgId: 1, Teams: []int64{2, 3}}, + } + err = GetPreferencesWithDefaults(query) So(err, ShouldBeNil) So(query.Result.HomeDashboardId, ShouldEqual, 3) }) Convey("GetPreferencesWithDefaults with saved org and other teams home dashboard should return org home dashboard", func() { - SavePreferences(&models.SavePreferencesCommand{OrgId: 1, HomeDashboardId: 1}) - SavePreferences(&models.SavePreferencesCommand{OrgId: 1, TeamId: 2, HomeDashboardId: 2}) - SavePreferences(&models.SavePreferencesCommand{OrgId: 1, TeamId: 3, HomeDashboardId: 3}) + err := SavePreferences(&models.SavePreferencesCommand{OrgId: 1, HomeDashboardId: 1}) + So(err, ShouldBeNil) + err = SavePreferences(&models.SavePreferencesCommand{OrgId: 1, TeamId: 2, HomeDashboardId: 2}) + So(err, ShouldBeNil) + err = SavePreferences(&models.SavePreferencesCommand{OrgId: 1, TeamId: 3, HomeDashboardId: 3}) + So(err, ShouldBeNil) query := &models.GetPreferencesWithDefaultsQuery{User: &models.SignedInUser{OrgId: 1}} - err := GetPreferencesWithDefaults(query) + err = GetPreferencesWithDefaults(query) So(err, ShouldBeNil) So(query.Result.HomeDashboardId, ShouldEqual, 1) }) Convey("GetPreferencesWithDefaults with saved org, teams and user home dashboard should return user home dashboard", func() { - SavePreferences(&models.SavePreferencesCommand{OrgId: 1, HomeDashboardId: 1}) - SavePreferences(&models.SavePreferencesCommand{OrgId: 1, TeamId: 2, HomeDashboardId: 2}) - SavePreferences(&models.SavePreferencesCommand{OrgId: 1, TeamId: 3, HomeDashboardId: 3}) - SavePreferences(&models.SavePreferencesCommand{OrgId: 1, UserId: 1, HomeDashboardId: 4}) + err := SavePreferences(&models.SavePreferencesCommand{OrgId: 1, HomeDashboardId: 1}) + So(err, ShouldBeNil) + err = SavePreferences(&models.SavePreferencesCommand{OrgId: 1, TeamId: 2, HomeDashboardId: 2}) + So(err, ShouldBeNil) + err = SavePreferences(&models.SavePreferencesCommand{OrgId: 1, TeamId: 3, HomeDashboardId: 3}) + So(err, ShouldBeNil) + err = SavePreferences(&models.SavePreferencesCommand{OrgId: 1, UserId: 1, HomeDashboardId: 4}) + So(err, ShouldBeNil) - query := &models.GetPreferencesWithDefaultsQuery{User: &models.SignedInUser{OrgId: 1, UserId: 1, Teams: []int64{2, 3}}} - err := GetPreferencesWithDefaults(query) + query := &models.GetPreferencesWithDefaultsQuery{ + User: &models.SignedInUser{OrgId: 1, UserId: 1, Teams: []int64{2, 3}}, + } + err = GetPreferencesWithDefaults(query) So(err, ShouldBeNil) So(query.Result.HomeDashboardId, ShouldEqual, 4) }) Convey("GetPreferencesWithDefaults with saved org, other teams and user home dashboard should return org home dashboard", func() { - SavePreferences(&models.SavePreferencesCommand{OrgId: 1, HomeDashboardId: 1}) - SavePreferences(&models.SavePreferencesCommand{OrgId: 1, TeamId: 2, HomeDashboardId: 2}) - SavePreferences(&models.SavePreferencesCommand{OrgId: 1, TeamId: 3, HomeDashboardId: 3}) - SavePreferences(&models.SavePreferencesCommand{OrgId: 1, UserId: 1, HomeDashboardId: 4}) + err := SavePreferences(&models.SavePreferencesCommand{OrgId: 1, HomeDashboardId: 1}) + So(err, ShouldBeNil) + err = SavePreferences(&models.SavePreferencesCommand{OrgId: 1, TeamId: 2, HomeDashboardId: 2}) + So(err, ShouldBeNil) + err = SavePreferences(&models.SavePreferencesCommand{OrgId: 1, TeamId: 3, HomeDashboardId: 3}) + So(err, ShouldBeNil) + err = SavePreferences(&models.SavePreferencesCommand{OrgId: 1, UserId: 1, HomeDashboardId: 4}) + So(err, ShouldBeNil) - query := &models.GetPreferencesWithDefaultsQuery{User: &models.SignedInUser{OrgId: 1, UserId: 2}} - err := GetPreferencesWithDefaults(query) + query := &models.GetPreferencesWithDefaultsQuery{ + User: &models.SignedInUser{OrgId: 1, UserId: 2}, + } + err = GetPreferencesWithDefaults(query) So(err, ShouldBeNil) So(query.Result.HomeDashboardId, ShouldEqual, 1) }) diff --git a/pkg/services/sqlstore/session.go b/pkg/services/sqlstore/session.go index a705af39da3..07f1d4524c4 100644 --- a/pkg/services/sqlstore/session.go +++ b/pkg/services/sqlstore/session.go @@ -68,13 +68,18 @@ func withDbSession(ctx context.Context, callback dbTransactionFunc) error { func (sess *DBSession) InsertId(bean interface{}) (int64, error) { table := sess.DB().Mapper.Obj2Table(getTypeName(bean)) - dialect.PreInsertId(table, sess.Session) - + if err := dialect.PreInsertId(table, sess.Session); err != nil { + return 0, err + } id, err := sess.Session.InsertOne(bean) + if err != nil { + return 0, err + } + if err := dialect.PostInsertId(table, sess.Session); err != nil { + return 0, err + } - dialect.PostInsertId(table, sess.Session) - - return id, err + return id, nil } func getTypeName(bean interface{}) (res string) { diff --git a/pkg/services/sqlstore/sql_test_data.go b/pkg/services/sqlstore/sql_test_data.go index 37740dc8373..4014cd01980 100644 --- a/pkg/services/sqlstore/sql_test_data.go +++ b/pkg/services/sqlstore/sql_test_data.go @@ -57,10 +57,12 @@ func InsertSqlTestData(cmd *m.InsertSqlTestDataCommand) error { rows, _ := res.RowsAffected() sqlog.Info("SQL TestData: Truncate done", "rows", rows) - sqlRandomWalk("server1", "frontend", 100, 1.123, sess) - sqlRandomWalk("server2", "frontend", 100, 1.123, sess) - sqlRandomWalk("server3", "frontend", 100, 1.123, sess) - - return err + if err := sqlRandomWalk("server1", "frontend", 100, 1.123, sess); err != nil { + return err + } + if err := sqlRandomWalk("server2", "frontend", 100, 1.123, sess); err != nil { + return err + } + return sqlRandomWalk("server3", "frontend", 100, 1.123, sess) }) } diff --git a/pkg/services/sqlstore/sqlstore.go b/pkg/services/sqlstore/sqlstore.go index 19d4da27793..2099f5d10bc 100644 --- a/pkg/services/sqlstore/sqlstore.go +++ b/pkg/services/sqlstore/sqlstore.go @@ -180,7 +180,10 @@ func (ss *SqlStore) buildConnectionString() (string, error) { if err != nil { return "", err } - mysql.RegisterTLSConfig("custom", tlsCert) + if err := mysql.RegisterTLSConfig("custom", tlsCert); err != nil { + return "", err + } + cnnstr += "&tls=custom" } @@ -205,7 +208,10 @@ func (ss *SqlStore) buildConnectionString() (string, error) { if !filepath.IsAbs(ss.dbCfg.Path) { ss.dbCfg.Path = filepath.Join(ss.Cfg.DataPath, ss.dbCfg.Path) } - os.MkdirAll(path.Dir(ss.dbCfg.Path), os.ModePerm) + if err := os.MkdirAll(path.Dir(ss.dbCfg.Path), os.ModePerm); err != nil { + return "", err + } + cnnstr = fmt.Sprintf("file:%s?cache=%s&mode=rwc", ss.dbCfg.Path, ss.dbCfg.CacheMode) cnnstr += ss.buildExtraConnectionString('&') default: @@ -312,16 +318,27 @@ func InitTestDB(t ITestDB) *SqlStore { // set test db config sqlstore.Cfg = setting.NewCfg() - sec, _ := sqlstore.Cfg.Raw.NewSection("database") - sec.NewKey("type", dbType) + sec, err := sqlstore.Cfg.Raw.NewSection("database") + if err != nil { + t.Fatalf("Failed to create section: %s", err) + } + if _, err := sec.NewKey("type", dbType); err != nil { + t.Fatalf("Failed to create key: %s", err) + } switch dbType { case "mysql": - sec.NewKey("connection_string", sqlutil.TestDB_Mysql.ConnStr) + if _, err := sec.NewKey("connection_string", sqlutil.TestDB_Mysql.ConnStr); err != nil { + t.Fatalf("Failed to create key: %s", err) + } case "postgres": - sec.NewKey("connection_string", sqlutil.TestDB_Postgres.ConnStr) + if _, err := sec.NewKey("connection_string", sqlutil.TestDB_Postgres.ConnStr); err != nil { + t.Fatalf("Failed to create key: %s", err) + } default: - sec.NewKey("connection_string", sqlutil.TestDB_Sqlite3.ConnStr) + if _, err := sec.NewKey("connection_string", sqlutil.TestDB_Sqlite3.ConnStr); err != nil { + t.Fatalf("Failed to create key: %s", err) + } } // need to get engine to clean db before we init diff --git a/pkg/services/sqlstore/sqlstore_test.go b/pkg/services/sqlstore/sqlstore_test.go index 9fe86f92f7c..99896ca4e9d 100644 --- a/pkg/services/sqlstore/sqlstore_test.go +++ b/pkg/services/sqlstore/sqlstore_test.go @@ -90,12 +90,18 @@ func TestSqlConnectionString(t *testing.T) { func makeSqlStoreTestConfig(dbType string, host string) *setting.Cfg { cfg := setting.NewCfg() - sec, _ := cfg.Raw.NewSection("database") - sec.NewKey("type", dbType) - sec.NewKey("host", host) - sec.NewKey("user", "user") - sec.NewKey("name", "test_db") - sec.NewKey("password", "pass") + sec, err := cfg.Raw.NewSection("database") + So(err, ShouldBeNil) + _, err = sec.NewKey("type", dbType) + So(err, ShouldBeNil) + _, err = sec.NewKey("host", host) + So(err, ShouldBeNil) + _, err = sec.NewKey("user", "user") + So(err, ShouldBeNil) + _, err = sec.NewKey("name", "test_db") + So(err, ShouldBeNil) + _, err = sec.NewKey("password", "pass") + So(err, ShouldBeNil) return cfg } diff --git a/pkg/services/sqlstore/team_test.go b/pkg/services/sqlstore/team_test.go index e6bf281af43..51c8a651c9e 100644 --- a/pkg/services/sqlstore/team_test.go +++ b/pkg/services/sqlstore/team_test.go @@ -216,20 +216,22 @@ func TestTeamCommandsAndQueries(t *testing.T) { }) Convey("A user should be able to remove an admin if there are other admins", func() { - AddTeamMember(&models.AddTeamMemberCommand{OrgId: testOrgId, TeamId: group1.Result.Id, UserId: userIds[1], Permission: models.PERMISSION_ADMIN}) + err = AddTeamMember(&models.AddTeamMemberCommand{OrgId: testOrgId, TeamId: group1.Result.Id, UserId: userIds[1], Permission: models.PERMISSION_ADMIN}) + So(err, ShouldBeNil) err = RemoveTeamMember(&models.RemoveTeamMemberCommand{OrgId: testOrgId, TeamId: group1.Result.Id, UserId: userIds[0], ProtectLastAdmin: true}) - So(err, ShouldEqual, nil) + So(err, ShouldBeNil) }) Convey("A user should not be able to remove the admin permission for the last admin", func() { err = UpdateTeamMember(&models.UpdateTeamMemberCommand{OrgId: testOrgId, TeamId: group1.Result.Id, UserId: userIds[0], Permission: 0, ProtectLastAdmin: true}) - So(err, ShouldEqual, models.ErrLastTeamAdmin) + So(err, ShouldBeError, models.ErrLastTeamAdmin) }) Convey("A user should be able to remove the admin permission if there are other admins", func() { - AddTeamMember(&models.AddTeamMemberCommand{OrgId: testOrgId, TeamId: group1.Result.Id, UserId: userIds[1], Permission: models.PERMISSION_ADMIN}) + err = AddTeamMember(&models.AddTeamMemberCommand{OrgId: testOrgId, TeamId: group1.Result.Id, UserId: userIds[1], Permission: models.PERMISSION_ADMIN}) + So(err, ShouldBeNil) err = UpdateTeamMember(&models.UpdateTeamMemberCommand{OrgId: testOrgId, TeamId: group1.Result.Id, UserId: userIds[0], Permission: 0, ProtectLastAdmin: true}) - So(err, ShouldEqual, nil) + So(err, ShouldBeNil) }) }) diff --git a/pkg/services/sqlstore/transactions.go b/pkg/services/sqlstore/transactions.go index 9b744fd3288..27928f52a4a 100644 --- a/pkg/services/sqlstore/transactions.go +++ b/pkg/services/sqlstore/transactions.go @@ -7,6 +7,7 @@ import ( "github.com/go-xorm/xorm" "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/util/errutil" sqlite3 "github.com/mattn/go-sqlite3" ) @@ -41,19 +42,24 @@ func inTransactionWithRetryCtx(ctx context.Context, engine *xorm.Engine, callbac err = callback(sess) // special handling of database locked errors for sqlite, then we can retry 5 times - if sqlError, ok := err.(sqlite3.Error); ok && retry < 5 { - if sqlError.Code == sqlite3.ErrLocked || sqlError.Code == sqlite3.ErrBusy { - sess.Rollback() - time.Sleep(time.Millisecond * time.Duration(10)) - sqlog.Info("Database locked, sleeping then retrying", "error", err, "retry", retry) - return inTransactionWithRetry(callback, retry+1) + if sqlError, ok := err.(sqlite3.Error); ok && retry < 5 && sqlError.Code == + sqlite3.ErrLocked || sqlError.Code == sqlite3.ErrBusy { + if rollErr := sess.Rollback(); rollErr != nil { + return errutil.Wrapf(err, "Rolling back transaction due to error failed: %s", rollErr) } + + time.Sleep(time.Millisecond * time.Duration(10)) + sqlog.Info("Database locked, sleeping then retrying", "error", err, "retry", retry) + return inTransactionWithRetry(callback, retry+1) } if err != nil { - sess.Rollback() + if rollErr := sess.Rollback(); rollErr != nil { + return errutil.Wrapf(err, "Rolling back transaction due to error failed: %s", rollErr) + } return err - } else if err = sess.Commit(); err != nil { + } + if err := sess.Commit(); err != nil { return err } diff --git a/pkg/services/sqlstore/user.go b/pkg/services/sqlstore/user.go index 04b56a32644..f9ce0cdfa7c 100644 --- a/pkg/services/sqlstore/user.go +++ b/pkg/services/sqlstore/user.go @@ -284,7 +284,9 @@ func UpdateUserLastSeenAt(cmd *models.UpdateUserLastSeenAtCommand) error { func SetUsingOrg(cmd *models.SetUsingOrgCommand) error { getOrgsForUserCmd := &models.GetUserOrgListQuery{UserId: cmd.UserId} - GetUserOrgList(getOrgsForUserCmd) + if err := GetUserOrgList(getOrgsForUserCmd); err != nil { + return err + } valid := false for _, other := range getOrgsForUserCmd.Result { @@ -292,7 +294,6 @@ func SetUsingOrg(cmd *models.SetUsingOrgCommand) error { valid = true } } - if !valid { return fmt.Errorf("user does not belong to org") } @@ -507,7 +508,9 @@ func SearchUsers(query *models.SearchUsersQuery) error { func DisableUser(cmd *models.DisableUserCommand) error { user := models.User{} sess := x.Table("user") - sess.ID(cmd.UserId).Get(&user) + if _, err := sess.ID(cmd.UserId).Get(&user); err != nil { + return err + } user.IsDisabled = cmd.IsDisabled sess.UseBool("is_disabled") @@ -573,7 +576,9 @@ func deleteUserInTransaction(sess *DBSession, cmd *models.DeleteUserCommand) err func UpdateUserPermissions(cmd *models.UpdateUserPermissionsCommand) error { return inTransaction(func(sess *DBSession) error { user := models.User{} - sess.ID(cmd.UserId).Get(&user) + if _, err := sess.ID(cmd.UserId).Get(&user); err != nil { + return err + } user.IsAdmin = cmd.IsGrafanaAdmin sess.UseBool("is_admin") diff --git a/pkg/services/sqlstore/user_test.go b/pkg/services/sqlstore/user_test.go index ece68ed8915..a10bb0c30c1 100644 --- a/pkg/services/sqlstore/user_test.go +++ b/pkg/services/sqlstore/user_test.go @@ -227,13 +227,21 @@ func TestUserDataAccess(t *testing.T) { }) Convey("when a user is an org member and has been assigned permissions", func() { - err := AddOrgUser(&models.AddOrgUserCommand{LoginOrEmail: users[1].Login, Role: models.ROLE_VIEWER, OrgId: users[0].OrgId, UserId: users[1].Id}) + err := AddOrgUser(&models.AddOrgUserCommand{ + LoginOrEmail: users[1].Login, Role: models.ROLE_VIEWER, + OrgId: users[0].OrgId, UserId: users[1].Id, + }) So(err, ShouldBeNil) - testHelperUpdateDashboardAcl(1, models.DashboardAcl{DashboardId: 1, OrgId: users[0].OrgId, UserId: users[1].Id, Permission: models.PERMISSION_EDIT}) + err = testHelperUpdateDashboardAcl(1, models.DashboardAcl{ + DashboardId: 1, OrgId: users[0].OrgId, UserId: users[1].Id, + Permission: models.PERMISSION_EDIT, + }) So(err, ShouldBeNil) - err = SavePreferences(&models.SavePreferencesCommand{UserId: users[1].Id, OrgId: users[0].OrgId, HomeDashboardId: 1, Theme: "dark"}) + err = SavePreferences(&models.SavePreferencesCommand{ + UserId: users[1].Id, OrgId: users[0].OrgId, HomeDashboardId: 1, Theme: "dark", + }) So(err, ShouldBeNil) Convey("when the user is deleted", func() {