Chore: Return correct error for name taken and validation error on add/update datasource (#70465)

pkg/api/datasources.go

@@ -390,14 +390,14 @@ func (hs *HTTPServer) AddDataSource(c *contextmodel.ReqContext) response.Respons
 	dataSource, err := hs.DataSourcesService.AddDataSource(c.Req.Context(), &cmd)
 	if err != nil {
 		if errors.Is(err, datasources.ErrDataSourceNameExists) || errors.Is(err, datasources.ErrDataSourceUidExists) {
-			return response.Error(409, err.Error(), err)
+			return response.Error(http.StatusConflict, err.Error(), err)
 		}
 
 		if errors.As(err, &secretsPluginError) {
-			return response.Error(500, "Failed to add datasource: "+err.Error(), err)
+			return response.Error(http.StatusInternalServerError, "Failed to add datasource: "+err.Error(), err)
 		}
 
-		return response.Error(500, "Failed to add datasource", err)
+		return response.ErrOrFallback(http.StatusInternalServerError, "Failed to add datasource", err)
 	}
 
 	// Clear permission cache for the user who's created the data source, so that new permissions are fetched for their next call
@@ -512,14 +512,19 @@ func (hs *HTTPServer) updateDataSourceByID(c *contextmodel.ReqContext, ds *datas
 
 	_, err := hs.DataSourcesService.UpdateDataSource(c.Req.Context(), &cmd)
 	if err != nil {
+		if errors.Is(err, datasources.ErrDataSourceNameExists) {
+			return response.Error(http.StatusConflict, "Failed to update datasource: "+err.Error(), err)
+		}
+
 		if errors.Is(err, datasources.ErrDataSourceUpdatingOldVersion) {
-			return response.Error(409, "Datasource has already been updated by someone else. Please reload and try again", err)
+			return response.Error(http.StatusConflict, "Datasource has already been updated by someone else. Please reload and try again", err)
 		}
 
 		if errors.As(err, &secretsPluginError) {
-			return response.Error(500, "Failed to update datasource: "+err.Error(), err)
+			return response.Error(http.StatusInternalServerError, "Failed to update datasource: "+err.Error(), err)
 		}
-		return response.Error(500, "Failed to update datasource", err)
+
+		return response.ErrOrFallback(http.StatusInternalServerError, "Failed to update datasource", err)
 	}
 
 	query := datasources.GetDataSourceQuery{

pkg/api/datasources_test.go

@@ -23,6 +23,7 @@ import (
 	"github.com/grafana/grafana/pkg/services/datasources"
 	"github.com/grafana/grafana/pkg/services/datasources/permissions"
 	"github.com/grafana/grafana/pkg/setting"
+	"github.com/grafana/grafana/pkg/web"
 	"github.com/grafana/grafana/pkg/web/webtest"
 )
 
@@ -248,6 +249,38 @@ func TestUpdateDataSource_URLWithoutProtocol(t *testing.T) {
 	assert.Equal(t, 200, sc.resp.Code)
 }
 
+// Updating data source name where data source with same name exists.
+func TestUpdateDataSourceByID_DataSourceNameExists(t *testing.T) {
+	hs := &HTTPServer{
+		DataSourcesService: &dataSourcesServiceMock{
+			expectedDatasource: &datasources.DataSource{},
+			mockUpdateDataSource: func(ctx context.Context, cmd *datasources.UpdateDataSourceCommand) (*datasources.DataSource, error) {
+				return nil, datasources.ErrDataSourceNameExists
+			},
+		},
+		Cfg:                  setting.NewCfg(),
+		AccessControl:        acimpl.ProvideAccessControl(setting.NewCfg()),
+		accesscontrolService: actest.FakeService{},
+		Live:                 newTestLive(t, nil),
+	}
+
+	sc := setupScenarioContext(t, "/api/datasources/1")
+
+	sc.m.Put(sc.url, routing.Wrap(func(c *contextmodel.ReqContext) response.Response {
+		c.Req = web.SetURLParams(c.Req, map[string]string{":id": "1"})
+		c.Req.Body = mockRequestBody(datasources.UpdateDataSourceCommand{
+			Access: "direct",
+			Type:   "test",
+			Name:   "test",
+		})
+		return hs.UpdateDataSourceByID(c)
+	}))
+
+	sc.fakeReqWithParams("PUT", sc.url, map[string]string{}).exec()
+
+	require.Equal(t, http.StatusConflict, sc.resp.Code)
+}
+
 func TestAPI_datasources_AccessControl(t *testing.T) {
 	type testCase struct {
 		desc         string
@@ -359,6 +392,8 @@ type dataSourcesServiceMock struct {
 	expectedDatasources []*datasources.DataSource
 	expectedDatasource  *datasources.DataSource
 	expectedError       error
+
+	mockUpdateDataSource func(ctx context.Context, cmd *datasources.UpdateDataSourceCommand) (*datasources.DataSource, error)
 }
 
 func (m *dataSourcesServiceMock) GetDataSource(ctx context.Context, query *datasources.GetDataSourceQuery) (*datasources.DataSource, error) {
@@ -386,6 +421,10 @@ func (m *dataSourcesServiceMock) AddDataSource(ctx context.Context, cmd *datasou
 }
 
 func (m *dataSourcesServiceMock) UpdateDataSource(ctx context.Context, cmd *datasources.UpdateDataSourceCommand) (*datasources.DataSource, error) {
+	if m.mockUpdateDataSource != nil {
+		return m.mockUpdateDataSource(ctx, cmd)
+	}
+
 	return m.expectedDatasource, m.expectedError
 }
 

pkg/services/datasources/errors.go

@@ -1,6 +1,10 @@
 package datasources
 
-import "errors"
+import (
+	"errors"
+
+	"github.com/grafana/grafana/pkg/util/errutil"
+)
 
 var (
 	ErrDataSourceNotFound                = errors.New("data source not found")
@@ -11,4 +15,6 @@ var (
 	ErrDataSourceFailedGenerateUniqueUid = errors.New("failed to generate unique datasource ID")
 	ErrDataSourceIdentifierNotSet        = errors.New("unique identifier and org id are needed to be able to get or delete a datasource")
 	ErrDatasourceIsReadOnly              = errors.New("data source is readonly, can only be updated from configuration")
+	ErrDataSourceNameInvalid             = errutil.NewBase(errutil.StatusValidationFailed, "datasource.nameInvalid", errutil.WithPublicMessage("Invalid datasource name."))
+	ErrDataSourceURLInvalid              = errutil.NewBase(errutil.StatusValidationFailed, "datasource.urlInvalid", errutil.WithPublicMessage("Invalid datasource url."))
 )

pkg/services/datasources/service/datasource.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"crypto/tls"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"net/http"
 	"strconv"
@@ -26,6 +27,11 @@ import (
 	"github.com/grafana/grafana/pkg/setting"
 )
 
+const (
+	maxDatasourceNameLen = 190
+	maxDatasourceUrlLen  = 255
+)
+
 type Service struct {
 	SQLStore           Store
 	SecretsStore       kvstore.SecretsKVStore
@@ -172,6 +178,10 @@ func (s *Service) GetDataSourcesByType(ctx context.Context, query *datasources.G
 func (s *Service) AddDataSource(ctx context.Context, cmd *datasources.AddDataSourceCommand) (*datasources.DataSource, error) {
 	var dataSource *datasources.DataSource
 
+	if err := validateFields(cmd.Name, cmd.URL); err != nil {
+		return dataSource, err
+	}
+
 	return dataSource, s.db.InTransaction(ctx, func(ctx context.Context) error {
 		var err error
 
@@ -231,6 +241,11 @@ func (s *Service) DeleteDataSource(ctx context.Context, cmd *datasources.DeleteD
 
 func (s *Service) UpdateDataSource(ctx context.Context, cmd *datasources.UpdateDataSourceCommand) (*datasources.DataSource, error) {
 	var dataSource *datasources.DataSource
+
+	if err := validateFields(cmd.Name, cmd.URL); err != nil {
+		return dataSource, err
+	}
+
 	return dataSource, s.db.InTransaction(ctx, func(ctx context.Context) error {
 		var err error
 
@@ -243,6 +258,21 @@ func (s *Service) UpdateDataSource(ctx context.Context, cmd *datasources.UpdateD
 			return err
 		}
 
+		if cmd.Name != "" && cmd.Name != dataSource.Name {
+			query := &datasources.GetDataSourceQuery{
+				Name:  cmd.Name,
+				OrgID: cmd.OrgID,
+			}
+			exist, err := s.SQLStore.GetDataSource(ctx, query)
+			if exist != nil {
+				return datasources.ErrDataSourceNameExists
+			}
+
+			if err != nil && !errors.Is(err, datasources.ErrDataSourceNotFound) {
+				return err
+			}
+		}
+
 		err = s.fillWithSecureJSONData(ctx, cmd, dataSource)
 		if err != nil {
 			return err
@@ -631,6 +661,18 @@ func (s *Service) fillWithSecureJSONData(ctx context.Context, cmd *datasources.U
 	return nil
 }
 
+func validateFields(name, url string) error {
+	if len(name) > maxDatasourceNameLen {
+		return datasources.ErrDataSourceNameInvalid.Errorf("max length is %d", maxDatasourceNameLen)
+	}
+
+	if len(url) > maxDatasourceUrlLen {
+		return datasources.ErrDataSourceURLInvalid.Errorf("max length is %d", maxDatasourceUrlLen)
+	}
+
+	return nil
+}
+
 func readQuotaConfig(cfg *setting.Cfg) (*quota.Map, error) {
 	limits := &quota.Map{}
 

pkg/services/datasources/service/datasource_test.go

@@ -9,8 +9,10 @@ import (
 	"testing"
 	"time"
 
+	"github.com/google/uuid"
 	sdkhttpclient "github.com/grafana/grafana-plugin-sdk-go/backend/httpclient"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
 
 	"github.com/grafana/grafana/pkg/components/simplejson"
@@ -18,6 +20,7 @@ import (
 	"github.com/grafana/grafana/pkg/infra/httpclient"
 	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/services/accesscontrol"
+	"github.com/grafana/grafana/pkg/services/accesscontrol/actest"
 	acmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock"
 	"github.com/grafana/grafana/pkg/services/datasources"
 	"github.com/grafana/grafana/pkg/services/featuremgmt"
@@ -45,6 +48,147 @@ func (d *dataSourceMockRetriever) GetDataSource(ctx context.Context, query *data
 	return nil, datasources.ErrDataSourceNotFound
 }
 
+func TestService_AddDataSource(t *testing.T) {
+	cfg := &setting.Cfg{}
+
+	t.Run("should return validation error if command validation failed", func(t *testing.T) {
+		sqlStore := db.InitTestDB(t)
+		secretsService := secretsmng.SetupTestService(t, fakes.NewFakeSecretsStore())
+		secretsStore := secretskvs.NewSQLSecretsKVStore(sqlStore, secretsService, log.New("test.logger"))
+		quotaService := quotatest.New(false, nil)
+		mockPermission := acmock.NewMockedPermissionsService()
+		dsService, err := ProvideService(sqlStore, secretsService, secretsStore, cfg, featuremgmt.WithFeatures(), actest.FakeAccessControl{}, mockPermission, quotaService)
+		require.NoError(t, err)
+
+		cmd := &datasources.AddDataSourceCommand{
+			OrgID: 1,
+			Name:  string(make([]byte, 256)),
+		}
+
+		_, err = dsService.AddDataSource(context.Background(), cmd)
+		require.EqualError(t, err, "[datasource.nameInvalid] max length is 190")
+
+		cmd = &datasources.AddDataSourceCommand{
+			OrgID: 1,
+			URL:   string(make([]byte, 256)),
+		}
+
+		_, err = dsService.AddDataSource(context.Background(), cmd)
+		require.EqualError(t, err, "[datasource.urlInvalid] max length is 255")
+	})
+}
+
+func TestService_UpdateDataSource(t *testing.T) {
+	cfg := &setting.Cfg{}
+
+	t.Run("should return not found error if datasource not found", func(t *testing.T) {
+		sqlStore := db.InitTestDB(t)
+		secretsService := secretsmng.SetupTestService(t, fakes.NewFakeSecretsStore())
+		secretsStore := secretskvs.NewSQLSecretsKVStore(sqlStore, secretsService, log.New("test.logger"))
+		quotaService := quotatest.New(false, nil)
+		mockPermission := acmock.NewMockedPermissionsService()
+		dsService, err := ProvideService(sqlStore, secretsService, secretsStore, cfg, featuremgmt.WithFeatures(), actest.FakeAccessControl{}, mockPermission, quotaService)
+		require.NoError(t, err)
+
+		cmd := &datasources.UpdateDataSourceCommand{
+			UID:   uuid.New().String(),
+			ID:    1,
+			OrgID: 1,
+		}
+
+		_, err = dsService.UpdateDataSource(context.Background(), cmd)
+		require.ErrorIs(t, err, datasources.ErrDataSourceNotFound)
+	})
+
+	t.Run("should return validation error if command validation failed", func(t *testing.T) {
+		sqlStore := db.InitTestDB(t)
+		secretsService := secretsmng.SetupTestService(t, fakes.NewFakeSecretsStore())
+		secretsStore := secretskvs.NewSQLSecretsKVStore(sqlStore, secretsService, log.New("test.logger"))
+		quotaService := quotatest.New(false, nil)
+		mockPermission := acmock.NewMockedPermissionsService()
+		dsService, err := ProvideService(sqlStore, secretsService, secretsStore, cfg, featuremgmt.WithFeatures(), actest.FakeAccessControl{}, mockPermission, quotaService)
+		require.NoError(t, err)
+
+		cmd := &datasources.UpdateDataSourceCommand{
+			ID:    1,
+			OrgID: 1,
+			Name:  string(make([]byte, 256)),
+		}
+
+		_, err = dsService.UpdateDataSource(context.Background(), cmd)
+		require.EqualError(t, err, "[datasource.nameInvalid] max length is 190")
+
+		cmd = &datasources.UpdateDataSourceCommand{
+			ID:    1,
+			OrgID: 1,
+			URL:   string(make([]byte, 256)),
+		}
+
+		_, err = dsService.UpdateDataSource(context.Background(), cmd)
+		require.EqualError(t, err, "[datasource.urlInvalid] max length is 255")
+	})
+
+	t.Run("should return no error if updated datasource", func(t *testing.T) {
+		sqlStore := db.InitTestDB(t)
+		secretsService := secretsmng.SetupTestService(t, fakes.NewFakeSecretsStore())
+		secretsStore := secretskvs.NewSQLSecretsKVStore(sqlStore, secretsService, log.New("test.logger"))
+		quotaService := quotatest.New(false, nil)
+		mockPermission := acmock.NewMockedPermissionsService()
+		dsService, err := ProvideService(sqlStore, secretsService, secretsStore, cfg, featuremgmt.WithFeatures(), actest.FakeAccessControl{}, mockPermission, quotaService)
+		require.NoError(t, err)
+
+		mockPermission.On("SetPermissions", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]accesscontrol.ResourcePermission{}, nil)
+
+		ds, err := dsService.AddDataSource(context.Background(), &datasources.AddDataSourceCommand{
+			OrgID: 1,
+			Name:  "test-datasource",
+		})
+		require.NoError(t, err)
+
+		cmd := &datasources.UpdateDataSourceCommand{
+			ID:    ds.ID,
+			OrgID: ds.OrgID,
+			Name:  "test-datasource-updated",
+		}
+
+		_, err = dsService.UpdateDataSource(context.Background(), cmd)
+		require.NoError(t, err)
+	})
+
+	t.Run("should return error if datasource with same name exist", func(t *testing.T) {
+		sqlStore := db.InitTestDB(t)
+		secretsService := secretsmng.SetupTestService(t, fakes.NewFakeSecretsStore())
+		secretsStore := secretskvs.NewSQLSecretsKVStore(sqlStore, secretsService, log.New("test.logger"))
+		quotaService := quotatest.New(false, nil)
+		mockPermission := acmock.NewMockedPermissionsService()
+		dsService, err := ProvideService(sqlStore, secretsService, secretsStore, cfg, featuremgmt.WithFeatures(), actest.FakeAccessControl{}, mockPermission, quotaService)
+		require.NoError(t, err)
+
+		mockPermission.On("SetPermissions", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]accesscontrol.ResourcePermission{}, nil)
+
+		dsToUpdate, err := dsService.AddDataSource(context.Background(), &datasources.AddDataSourceCommand{
+			OrgID: 1,
+			Name:  "test-datasource",
+		})
+		require.NoError(t, err)
+
+		existingDs, err := dsService.AddDataSource(context.Background(), &datasources.AddDataSourceCommand{
+			OrgID: 1,
+			Name:  "name already taken",
+		})
+		require.NoError(t, err)
+
+		cmd := &datasources.UpdateDataSourceCommand{
+			ID:    dsToUpdate.ID,
+			OrgID: dsToUpdate.OrgID,
+			Name:  existingDs.Name,
+		}
+
+		_, err = dsService.UpdateDataSource(context.Background(), cmd)
+		require.ErrorIs(t, err, datasources.ErrDataSourceNameExists)
+	})
+}
+
 func TestService_NameScopeResolver(t *testing.T) {
 	retriever := &dataSourceMockRetriever{[]*datasources.DataSource{
 		{Name: "test-datasource", UID: "1"},