Merge pull request grafana#9594 from bergquist/datasources_optimistic…

…_concurrency datasources: change to optimistic concurrency
miv · Oct 25, 2017 · c91a1e9 · c91a1e9
2 parents aa3fc9f + d68bfaa
commit c91a1e9
Show file tree

Hide file tree

Showing 8 changed files with 200 additions and 73 deletions.
diff --git a/pkg/api/datasources.go b/pkg/api/datasources.go
@@ -119,7 +119,13 @@ func AddDataSource(c *middleware.Context, cmd m.AddDataSourceCommand) {
 		return
 	}
 
-	c.JSON(200, util.DynMap{"message": "Datasource added", "id": cmd.Result.Id, "name": cmd.Result.Name})
+	ds := convertModelToDtos(cmd.Result)
+	c.JSON(200, util.DynMap{
+		"message":    "Datasource added",
+		"id":         cmd.Result.Id,
+		"name":       cmd.Result.Name,
+		"datasource": ds,
+	})
 }
 
 func UpdateDataSource(c *middleware.Context, cmd m.UpdateDataSourceCommand) Response {
@@ -133,10 +139,19 @@ func UpdateDataSource(c *middleware.Context, cmd m.UpdateDataSourceCommand) Resp
 
 	err = bus.Dispatch(&cmd)
 	if err != nil {
-		return ApiError(500, "Failed to update datasource", err)
+		if err == m.ErrDataSourceUpdatingOldVersion {
+			return ApiError(500, "Failed to update datasource. Reload new version and try again", err)
+		} else {
+			return ApiError(500, "Failed to update datasource", err)
+		}
 	}
-
-	return Json(200, util.DynMap{"message": "Datasource updated", "id": cmd.Id, "name": cmd.Name})
+	ds := convertModelToDtos(cmd.Result)
+	return Json(200, util.DynMap{
+		"message":    "Datasource updated",
+		"id":         cmd.Id,
+		"name":       cmd.Name,
+		"datasource": ds,
+	})
 }
 
 func fillWithSecureJsonData(cmd *m.UpdateDataSourceCommand) error {
@@ -158,8 +173,6 @@ func fillWithSecureJsonData(cmd *m.UpdateDataSourceCommand) error {
 		}
 	}
 
-	// set version from db
-	cmd.Version = ds.Version
 	return nil
 }
 
@@ -228,6 +241,7 @@ func convertModelToDtos(ds *m.DataSource) dtos.DataSource {
 		IsDefault:         ds.IsDefault,
 		JsonData:          ds.JsonData,
 		SecureJsonFields:  map[string]bool{},
+		Version:           ds.Version,
 	}
 
 	for k, v := range ds.SecureJsonData {

diff --git a/pkg/api/dtos/datasource.go b/pkg/api/dtos/datasource.go
@@ -0,0 +1,59 @@
+package dtos
+
+import (
+	"strings"
+
+	"github.com/grafana/grafana/pkg/components/simplejson"
+	m "github.com/grafana/grafana/pkg/models"
+)
+
+type DataSource struct {
+	Id                int64            `json:"id"`
+	OrgId             int64            `json:"orgId"`
+	Name              string           `json:"name"`
+	Type              string           `json:"type"`
+	TypeLogoUrl       string           `json:"typeLogoUrl"`
+	Access            m.DsAccess       `json:"access"`
+	Url               string           `json:"url"`
+	Password          string           `json:"password"`
+	User              string           `json:"user"`
+	Database          string           `json:"database"`
+	BasicAuth         bool             `json:"basicAuth"`
+	BasicAuthUser     string           `json:"basicAuthUser"`
+	BasicAuthPassword string           `json:"basicAuthPassword"`
+	WithCredentials   bool             `json:"withCredentials"`
+	IsDefault         bool             `json:"isDefault"`
+	JsonData          *simplejson.Json `json:"jsonData,omitempty"`
+	SecureJsonFields  map[string]bool  `json:"secureJsonFields"`
+	Version           int              `json:"version"`
+}
+
+type DataSourceListItemDTO struct {
+	Id          int64            `json:"id"`
+	OrgId       int64            `json:"orgId"`
+	Name        string           `json:"name"`
+	Type        string           `json:"type"`
+	TypeLogoUrl string           `json:"typeLogoUrl"`
+	Access      m.DsAccess       `json:"access"`
+	Url         string           `json:"url"`
+	Password    string           `json:"password"`
+	User        string           `json:"user"`
+	Database    string           `json:"database"`
+	BasicAuth   bool             `json:"basicAuth"`
+	IsDefault   bool             `json:"isDefault"`
+	JsonData    *simplejson.Json `json:"jsonData,omitempty"`
+}
+
+type DataSourceList []DataSourceListItemDTO
+
+func (slice DataSourceList) Len() int {
+	return len(slice)
+}
+
+func (slice DataSourceList) Less(i, j int) bool {
+	return strings.ToLower(slice[i].Name) < strings.ToLower(slice[j].Name)
+}
+
+func (slice DataSourceList) Swap(i, j int) {
+	slice[i], slice[j] = slice[j], slice[i]
+}
diff --git a/pkg/api/dtos/models.go b/pkg/api/dtos/models.go
@@ -37,56 +37,6 @@ type CurrentUser struct {
 	HelpFlags1     m.HelpFlags1 `json:"helpFlags1"`
 }
 
-type DataSource struct {
-	Id                int64            `json:"id"`
-	OrgId             int64            `json:"orgId"`
-	Name              string           `json:"name"`
-	Type              string           `json:"type"`
-	TypeLogoUrl       string           `json:"typeLogoUrl"`
-	Access            m.DsAccess       `json:"access"`
-	Url               string           `json:"url"`
-	Password          string           `json:"password"`
-	User              string           `json:"user"`
-	Database          string           `json:"database"`
-	BasicAuth         bool             `json:"basicAuth"`
-	BasicAuthUser     string           `json:"basicAuthUser"`
-	BasicAuthPassword string           `json:"basicAuthPassword"`
-	WithCredentials   bool             `json:"withCredentials"`
-	IsDefault         bool             `json:"isDefault"`
-	JsonData          *simplejson.Json `json:"jsonData,omitempty"`
-	SecureJsonFields  map[string]bool  `json:"secureJsonFields"`
-}
-
-type DataSourceListItemDTO struct {
-	Id          int64            `json:"id"`
-	OrgId       int64            `json:"orgId"`
-	Name        string           `json:"name"`
-	Type        string           `json:"type"`
-	TypeLogoUrl string           `json:"typeLogoUrl"`
-	Access      m.DsAccess       `json:"access"`
-	Url         string           `json:"url"`
-	Password    string           `json:"password"`
-	User        string           `json:"user"`
-	Database    string           `json:"database"`
-	BasicAuth   bool             `json:"basicAuth"`
-	IsDefault   bool             `json:"isDefault"`
-	JsonData    *simplejson.Json `json:"jsonData,omitempty"`
-}
-
-type DataSourceList []DataSourceListItemDTO
-
-func (slice DataSourceList) Len() int {
-	return len(slice)
-}
-
-func (slice DataSourceList) Less(i, j int) bool {
-	return strings.ToLower(slice[i].Name) < strings.ToLower(slice[j].Name)
-}
-
-func (slice DataSourceList) Swap(i, j int) {
-	slice[i], slice[j] = slice[j], slice[i]
-}
-
 type MetricRequest struct {
 	From    string             `json:"from"`
 	To      string             `json:"to"`

diff --git a/pkg/models/datasource.go b/pkg/models/datasource.go
@@ -23,10 +23,10 @@ const (
 	DS_ACCESS_PROXY  = "proxy"
 )
 
-// Typed errors
 var (
-	ErrDataSourceNotFound   = errors.New("Data source not found")
-	ErrDataSourceNameExists = errors.New("Data source with same name already exists")
+	ErrDataSourceNotFound           = errors.New("Data source not found")
+	ErrDataSourceNameExists         = errors.New("Data source with same name already exists")
+	ErrDataSourceUpdatingOldVersion = errors.New("Trying to update old version of datasource")
 )
 
 type DsAccess string
@@ -131,10 +131,12 @@ type UpdateDataSourceCommand struct {
 	IsDefault         bool              `json:"isDefault"`
 	JsonData          *simplejson.Json  `json:"jsonData"`
 	SecureJsonData    map[string]string `json:"secureJsonData"`
+	Version           int               `json:"version"`
 
-	OrgId   int64 `json:"-"`
-	Id      int64 `json:"-"`
-	Version int   `json:"-"`
+	OrgId int64 `json:"-"`
+	Id    int64 `json:"-"`
+
+	Result *DataSource
 }
 
 type DeleteDataSourceByIdCommand struct {

diff --git a/pkg/services/sqlstore/datasource.go b/pkg/services/sqlstore/datasource.go
@@ -3,6 +3,8 @@ package sqlstore
 import (
 	"time"
 
+	"github.com/go-xorm/xorm"
+
 	"github.com/grafana/grafana/pkg/bus"
 	"github.com/grafana/grafana/pkg/components/securejsondata"
 	"github.com/grafana/grafana/pkg/metrics"
@@ -69,7 +71,6 @@ func DeleteDataSourceByName(cmd *m.DeleteDataSourceByNameCommand) error {
 }
 
 func AddDataSource(cmd *m.AddDataSourceCommand) error {
-
 	return inTransaction(func(sess *DBSession) error {
 		existing := m.DataSource{OrgId: cmd.OrgId, Name: cmd.Name}
 		has, _ := sess.Get(&existing)
@@ -96,6 +97,7 @@ func AddDataSource(cmd *m.AddDataSourceCommand) error {
 			SecureJsonData:    securejsondata.GetEncryptedJsonData(cmd.SecureJsonData),
 			Created:           time.Now(),
 			Updated:           time.Now(),
+			Version:           1,
 		}
 
 		if _, err := sess.Insert(ds); err != nil {
@@ -122,7 +124,6 @@ func updateIsDefaultFlag(ds *m.DataSource, sess *DBSession) error {
 }
 
 func UpdateDataSource(cmd *m.UpdateDataSourceCommand) error {
-
 	return inTransaction(func(sess *DBSession) error {
 		ds := &m.DataSource{
 			Id:                cmd.Id,
@@ -149,12 +150,29 @@ func UpdateDataSource(cmd *m.UpdateDataSourceCommand) error {
 		sess.UseBool("basic_auth")
 		sess.UseBool("with_credentials")
 
-		_, err := sess.Where("id=? and org_id=?", ds.Id, ds.OrgId).Update(ds)
+		var updateSession *xorm.Session
+		if cmd.Version != 0 {
+			// the reason we allow cmd.version > db.version is make it possible for people to force
+			// updates to datasources using the datasource.yaml file without knowing exactly what version
+			// a datasource have in the db.
+			updateSession = sess.Where("id=? and org_id=? and version < ?", ds.Id, ds.OrgId, ds.Version)
+
+		} else {
+			updateSession = sess.Where("id=? and org_id=?", ds.Id, ds.OrgId)
+		}
+
+		affected, err := updateSession.Update(ds)
 		if err != nil {
 			return err
 		}
 
+		if affected == 0 {
+			return m.ErrDataSourceUpdatingOldVersion
+		}
+
 		err = updateIsDefaultFlag(ds, sess)
+
+		cmd.Result = ds
 		return err
 	})
 }
diff --git a/pkg/services/sqlstore/datasource_test.go b/pkg/services/sqlstore/datasource_test.go
@@ -35,12 +35,9 @@ type Test struct {
 }
 
 func TestDataAccess(t *testing.T) {
-
 	Convey("Testing DB", t, func() {
 		InitTestDB(t)
-
 		Convey("Can add datasource", func() {
-
 			err := AddDataSource(&m.AddDataSourceCommand{
 				OrgId:    10,
 				Name:     "laban",
@@ -65,7 +62,6 @@ func TestDataAccess(t *testing.T) {
 		})
 
 		Convey("Given a datasource", func() {
-
 			err := AddDataSource(&m.AddDataSourceCommand{
 				OrgId:  10,
 				Name:   "nisse",
@@ -81,6 +77,89 @@ func TestDataAccess(t *testing.T) {
 
 			ds := query.Result[0]
 
+			Convey(" updated ", func() {
+				cmd := &m.UpdateDataSourceCommand{
+					Id:      ds.Id,
+					OrgId:   10,
+					Name:    "nisse",
+					Type:    m.DS_GRAPHITE,
+					Access:  m.DS_ACCESS_PROXY,
+					Url:     "http://test",
+					Version: ds.Version,
+				}
+
+				Convey("with same version as source", func() {
+					err := UpdateDataSource(cmd)
+					So(err, ShouldBeNil)
+				})
+
+				Convey("when someone else updated between read and update", func() {
+					query := m.GetDataSourcesQuery{OrgId: 10}
+					err = GetDataSources(&query)
+					So(err, ShouldBeNil)
+
+					ds := query.Result[0]
+					intendedUpdate := &m.UpdateDataSourceCommand{
+						Id:      ds.Id,
+						OrgId:   10,
+						Name:    "nisse",
+						Type:    m.DS_GRAPHITE,
+						Access:  m.DS_ACCESS_PROXY,
+						Url:     "http://test",
+						Version: ds.Version,
+					}
+
+					updateFromOtherUser := &m.UpdateDataSourceCommand{
+						Id:      ds.Id,
+						OrgId:   10,
+						Name:    "nisse",
+						Type:    m.DS_GRAPHITE,
+						Access:  m.DS_ACCESS_PROXY,
+						Url:     "http://test",
+						Version: ds.Version,
+					}
+
+					err := UpdateDataSource(updateFromOtherUser)
+					So(err, ShouldBeNil)
+
+					err = UpdateDataSource(intendedUpdate)
+					So(err, ShouldNotBeNil)
+				})
+
+				Convey("updating datasource without version", func() {
+					cmd := &m.UpdateDataSourceCommand{
+						Id:     ds.Id,
+						OrgId:  10,
+						Name:   "nisse",
+						Type:   m.DS_GRAPHITE,
+						Access: m.DS_ACCESS_PROXY,
+						Url:    "http://test",
+					}
+
+					Convey("should not raise errors", func() {
+						err := UpdateDataSource(cmd)
+						So(err, ShouldBeNil)
+					})
+				})
+
+				Convey("updating datasource without higher version", func() {
+					cmd := &m.UpdateDataSourceCommand{
+						Id:      ds.Id,
+						OrgId:   10,
+						Name:    "nisse",
+						Type:    m.DS_GRAPHITE,
+						Access:  m.DS_ACCESS_PROXY,
+						Url:     "http://test",
+						Version: 90000,
+					}
+
+					Convey("should not raise errors", func() {
+						err := UpdateDataSource(cmd)
+						So(err, ShouldBeNil)
+					})
+				})
+			})
+
 			Convey("Can delete datasource by id", func() {
 				err := DeleteDataSourceById(&m.DeleteDataSourceByIdCommand{Id: ds.Id, OrgId: ds.OrgId})
 				So(err, ShouldBeNil)
@@ -104,9 +183,6 @@ func TestDataAccess(t *testing.T) {
 				GetDataSources(&query)
 				So(len(query.Result), ShouldEqual, 1)
 			})
-
 		})
-
 	})
-
 }
diff --git a/pkg/services/sqlstore/migrations/datasource_mig.go b/pkg/services/sqlstore/migrations/datasource_mig.go
@@ -120,4 +120,10 @@ func addDataSourceMigration(mg *Migrator) {
 		{Name: "json_data", Type: DB_Text, Nullable: true},
 		{Name: "secure_json_data", Type: DB_Text, Nullable: true},
 	}))
+
+	const setVersionToOneWhereZero = `UPDATE data_source SET version = 1 WHERE version = 0`
+	mg.AddMigration("Update initial version to 1", new(RawSqlMigration).
+		Sqlite(setVersionToOneWhereZero).
+		Postgres(setVersionToOneWhereZero).
+		Mysql(setVersionToOneWhereZero))
 }