diff --git a/pkg/services/datasources/models.go b/pkg/services/datasources/models.go index 0006523b6fe..494ac2b8f28 100644 --- a/pkg/services/datasources/models.go +++ b/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"` diff --git a/pkg/services/datasources/service/datasource.go b/pkg/services/datasources/service/datasource.go index 82b51b9c678..0f7b98e5e63 100644 --- a/pkg/services/datasources/service/datasource.go +++ b/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 { diff --git a/pkg/services/datasources/service/datasource_test.go b/pkg/services/datasources/service/datasource_test.go index 3ebfbaeb0d4..86c6632ab46 100644 --- a/pkg/services/datasources/service/datasource_test.go +++ b/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{} diff --git a/public/app/features/datasources/state/actions.test.ts b/public/app/features/datasources/state/actions.test.ts index e67fc2ca756..7f6b93b9208 100644 --- a/public/app/features/datasources/state/actions.test.ts +++ b/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', diff --git a/public/app/features/datasources/state/actions.ts b/public/app/features/datasources/state/actions.ts index 294bb87b4f4..7f7585a323a 100644 --- a/public/app/features/datasources/state/actions.ts +++ b/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> { - 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); diff --git a/public/app/features/datasources/utils.test.ts b/public/app/features/datasources/utils.test.ts deleted file mode 100644 index bf3743b0994..00000000000 --- a/public/app/features/datasources/utils.test.ts +++ /dev/null @@ -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-'); - }); - }); -}); diff --git a/public/app/features/datasources/utils.ts b/public/app/features/datasources/utils.ts index 7da4a2d41fc..f03b91af8d7 100644 --- a/public/app/features/datasources/utils.ts +++ b/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) => { const exploreState = JSON.stringify({ datasource: dataSource.name, context: 'explore' }); const exploreUrl = urlUtil.renderUrl(locationUtil.assureBaseUrl('/explore'), { left: exploreState });