Data sources: Refactor logic for naming new data sources (#78479)

* move the name finding logic for new data sources from frontend to backend

* cleanup and fix test

* linting

* change the way the number after the ds type is incremented - keep incrementing it without adding more hyphens

* enterprise spec updates (unrelated to the PR)

pkg/services/datasources/models.go

@@ -140,7 +140,7 @@ func (e ErrDatasourceSecretsPluginUserFriendly) Error() string {
 
 // Also acts as api DTO
 type AddDataSourceCommand struct {
-	Name            string            `json:"name" binding:"Required"`
+	Name            string            `json:"name"`
 	Type            string            `json:"type" binding:"Required"`
 	Access          DsAccess          `json:"access" binding:"Required"`
 	URL             string            `json:"url"`

pkg/services/datasources/service/datasource.go

@@ -188,12 +188,25 @@ 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
+	dataSources, err := s.SQLStore.GetDataSources(ctx, &datasources.GetDataSourcesQuery{OrgID: cmd.OrgID})
+	if err != nil {
+		return nil, err
 	}
 
+	// Set the first created data source as default
+	if len(dataSources) == 0 {
+		cmd.IsDefault = true
+	}
+
+	if cmd.Name == "" {
+		cmd.Name = getAvailableName(cmd.Type, dataSources)
+	}
+
+	if err := validateFields(cmd.Name, cmd.URL); err != nil {
+		return nil, err
+	}
+
+	var dataSource *datasources.DataSource
 	return dataSource, s.db.InTransaction(ctx, func(ctx context.Context) error {
 		var err error
 
@@ -236,6 +249,24 @@ func (s *Service) AddDataSource(ctx context.Context, cmd *datasources.AddDataSou
 	})
 }
 
+// getAvailableName finds the first available name for a datasource of the given type.
+func getAvailableName(dsType string, dataSources []*datasources.DataSource) string {
+	dsNames := make(map[string]bool)
+	for _, ds := range dataSources {
+		dsNames[strings.ToLower(ds.Name)] = true
+	}
+
+	name := dsType
+	currentDigit := 0
+
+	for dsNames[strings.ToLower(name)] {
+		currentDigit++
+		name = fmt.Sprintf("%s-%d", dsType, currentDigit)
+	}
+
+	return name
+}
+
 func (s *Service) DeleteDataSource(ctx context.Context, cmd *datasources.DeleteDataSourceCommand) error {
 	return s.db.InTransaction(ctx, func(ctx context.Context) error {
 		cmd.UpdateSecretFn = func() error {

pkg/services/datasources/service/datasource_test.go

@@ -79,6 +79,85 @@ func TestService_AddDataSource(t *testing.T) {
 	})
 }
 
+func TestService_getAvailableName(t *testing.T) {
+	type testCase struct {
+		desc       string
+		dsType     string
+		existingDs []*datasources.DataSource
+		expected   string
+	}
+
+	testCases := []testCase{
+		{
+			desc:     "should return type as the name if no DS are passed in",
+			dsType:   "prometheus",
+			expected: "prometheus",
+		},
+		{
+			desc:   "should return type as the name if no DS with that name exists",
+			dsType: "prometheus",
+			existingDs: []*datasources.DataSource{
+				{Name: "graphite"},
+				{Name: "loki"},
+			},
+			expected: "prometheus",
+		},
+		{
+			desc:   "should return type-1 as the name if one data source with that name exists",
+			dsType: "prometheus",
+			existingDs: []*datasources.DataSource{
+				{Name: "graphite"},
+				{Name: "prometheus"},
+			},
+			expected: "prometheus-1",
+		},
+		{
+			desc:   "should correctly increment the number suffix of the name",
+			dsType: "prometheus",
+			existingDs: []*datasources.DataSource{
+				{Name: "prometheus"},
+				{Name: "prometheus-1"},
+				{Name: "prometheus-3"},
+			},
+			expected: "prometheus-2",
+		},
+		{
+			desc:   "should correctly increment the number suffix for multidigit numbers",
+			dsType: "prometheus",
+			existingDs: []*datasources.DataSource{
+				{Name: "prometheus"},
+				{Name: "prometheus-1"},
+				{Name: "prometheus-2"},
+				{Name: "prometheus-3"},
+				{Name: "prometheus-4"},
+				{Name: "prometheus-5"},
+				{Name: "prometheus-6"},
+				{Name: "prometheus-7"},
+				{Name: "prometheus-8"},
+				{Name: "prometheus-9"},
+				{Name: "prometheus-10"},
+			},
+			expected: "prometheus-11",
+		},
+		{
+			desc:   "name comparison should be case insensitive",
+			dsType: "prometheus",
+			existingDs: []*datasources.DataSource{
+				{Name: "Prometheus"},
+				{Name: "PROMETHEUS"},
+			},
+			expected: "prometheus-1",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			name := getAvailableName(tc.dsType, tc.existingDs)
+			assert.Equal(t, tc.expected, name)
+		})
+	}
+}
+
 func TestService_UpdateDataSource(t *testing.T) {
 	cfg := &setting.Cfg{}
 

public/app/features/datasources/state/actions.test.ts

@@ -26,7 +26,6 @@ import {
   testDataSourceSucceeded,
   testDataSourceFailed,
   dataSourceLoaded,
-  dataSourcesLoaded,
 } from './reducers';
 
 jest.mock('../api');
@@ -383,18 +382,12 @@ describe('addDataSource', () => {
       type: PluginType.datasource,
       name: 'test DS',
     };
-    const state = {
-      dataSources: {
-        dataSources: [],
-      },
-    };
     const dataSourceMock = { datasource: { uid: 'azure23' }, meta };
     (api.createDataSource as jest.Mock).mockResolvedValueOnce(dataSourceMock);
     (api.getDataSources as jest.Mock).mockResolvedValueOnce([]);
 
-    const dispatchedActions = await thunkTester(state).givenThunk(addDataSource).whenThunkIsDispatched(meta);
+    await thunkTester({}).givenThunk(addDataSource).whenThunkIsDispatched(meta);
 
-    expect(dispatchedActions).toEqual([dataSourcesLoaded([])]);
     expect(trackDataSourceCreated).toHaveBeenCalledWith({
       plugin_id: 'azure-monitor',
       plugin_version: '1.2.3',

public/app/features/datasources/state/actions.ts

@@ -27,7 +27,6 @@ import { AccessControlAction, DataSourcePluginCategory, ThunkDispatch, ThunkResu
 import * as api from '../api';
 import { DATASOURCES_ROUTES } from '../constants';
 import { trackDataSourceCreated, trackDataSourceTested } from '../tracking';
-import { findNewName, nameExits } from '../utils';
 
 import { buildCategories } from './buildCategories';
 import { buildNavModel } from './navModel';
@@ -232,27 +231,12 @@ export function addDataSource(
   plugin: DataSourcePluginMeta,
   editRoute = DATASOURCES_ROUTES.Edit
 ): ThunkResult<Promise<void>> {
-  return async (dispatch, getStore) => {
-    // update the list of datasources first.
-    // We later use this list to check whether the name of the datasource
-    // being created is unuque or not and assign a new name to it if needed.
-    const response = await api.getDataSources();
-    dispatch(dataSourcesLoaded(response));
-
-    const dataSources = getStore().dataSources.dataSources;
-    const isFirstDataSource = dataSources.length === 0;
+  return async () => {
     const newInstance = {
-      name: plugin.name,
       type: plugin.id,
       access: 'proxy',
-      isDefault: isFirstDataSource,
     };
 
-    // TODO: typo in name
-    if (nameExits(dataSources, newInstance.name)) {
-      newInstance.name = findNewName(dataSources, newInstance.name);
-    }
-
     const result = await api.createDataSource(newInstance);
     const editLink = editRoute.replace(/:uid/gi, result.datasource.uid);
 

public/app/features/datasources/utils.test.ts

@@ -1,41 +0,0 @@
-import { getMockPlugin, getMockPlugins } from '@grafana/data/test/__mocks__/pluginMocks';
-
-import { nameExits, findNewName } from './utils';
-
-describe('Datasources / Utils', () => {
-  describe('nameExists()', () => {
-    const plugins = getMockPlugins(5);
-
-    it('should return TRUE if an existing plugin already has the same name', () => {
-      expect(nameExits(plugins, plugins[1].name)).toEqual(true);
-    });
-
-    it('should return FALSE if no plugin has the same name yet', () => {
-      expect(nameExits(plugins, 'unknown-plugin'));
-    });
-  });
-
-  describe('findNewName()', () => {
-    it('should return with a new name in case an existing plugin already has the same name', () => {
-      const plugins = getMockPlugins(5);
-      const name = 'pretty cool plugin-1';
-
-      expect(findNewName(plugins, name)).toEqual('pretty cool plugin-6');
-    });
-
-    it('should handle names without suffixes when name already exists', () => {
-      const name = 'prometheus';
-      const plugin = getMockPlugin({ name });
-
-      expect(findNewName([plugin], name)).toEqual('prometheus-1');
-    });
-
-    it('should handle names that end with a "-" when name does not exist yet', () => {
-      const plugin = getMockPlugin();
-      const plugins = [plugin];
-      const name = 'pretty cool plugin-';
-
-      expect(findNewName(plugins, name)).toEqual('pretty cool plugin-');
-    });
-  });
-});

public/app/features/datasources/utils.ts

@@ -1,53 +1,5 @@
 import { DataSourceJsonData, DataSourceSettings, urlUtil, locationUtil } from '@grafana/data';
 
-interface ItemWithName {
-  name: string;
-}
-
-export function nameExits(dataSources: ItemWithName[], name: string) {
-  return (
-    dataSources.filter((dataSource) => {
-      return dataSource.name.toLowerCase() === name.toLowerCase();
-    }).length > 0
-  );
-}
-
-export function findNewName(dataSources: ItemWithName[], name: string) {
-  // Need to loop through current data sources to make sure
-  // the name doesn't exist
-  while (nameExits(dataSources, name)) {
-    // If there's a duplicate name that doesn't end with '-x'
-    // we can add -1 to the name and be done.
-    if (!nameHasSuffix(name)) {
-      name = `${name}-1`;
-    } else {
-      // if there's a duplicate name that ends with '-x'
-      // we can try to increment the last digit until the name is unique
-
-      // remove the 'x' part and replace it with the new number
-      name = `${getNewName(name)}${incrementLastDigit(getLastDigit(name))}`;
-    }
-  }
-
-  return name;
-}
-
-function nameHasSuffix(name: string) {
-  return name.endsWith('-', name.length - 1);
-}
-
-function getLastDigit(name: string) {
-  return parseInt(name.slice(-1), 10);
-}
-
-function incrementLastDigit(digit: number) {
-  return isNaN(digit) ? 1 : digit + 1;
-}
-
-function getNewName(name: string) {
-  return name.slice(0, name.length - 1);
-}
-
 export const constructDataSourceExploreUrl = (dataSource: DataSourceSettings<DataSourceJsonData, {}>) => {
   const exploreState = JSON.stringify({ datasource: dataSource.name, context: 'explore' });
   const exploreUrl = urlUtil.renderUrl(locationUtil.assureBaseUrl('/explore'), { left: exploreState });