From 142a3aaf7cdd58cebbea2750f5fc37390afae90a Mon Sep 17 00:00:00 2001 From: Alex Ma Date: Thu, 11 May 2023 17:56:42 -0700 Subject: [PATCH] Added back fix for duplicate edits (#23003) --- .../connection/common/connectionConfig.ts | 108 +++++++++++++----- .../connection/common/connectionStore.ts | 17 ++- .../common/providerConnectionInfo.ts | 52 +++++---- .../test/common/connectionConfig.test.ts | 68 +++++++++++ .../common/providerConnectionInfo.test.ts | 10 -- .../browser/connectionManagementService.ts | 26 ++++- .../connectionManagementService.test.ts | 49 ++++++++ 7 files changed, 259 insertions(+), 71 deletions(-) diff --git a/src/sql/platform/connection/common/connectionConfig.ts b/src/sql/platform/connection/common/connectionConfig.ts index d1e370b1c9..a0dabc30f5 100644 --- a/src/sql/platform/connection/common/connectionConfig.ts +++ b/src/sql/platform/connection/common/connectionConfig.ts @@ -70,43 +70,88 @@ export class ConnectionConfig { }); } + /** + * Checks to make sure that the profile that is being edited is not identical to another profile. + */ + public async isDuplicateEdit(profile: IConnectionProfile, matcher: ProfileMatcher = ConnectionProfile.matchesProfile): Promise { + let profiles = deepClone(this.configurationService.inspect(CONNECTIONS_CONFIG_KEY).userValue as IConnectionProfileStore[]); + if (!profiles) { + profiles = []; + } + + let groupId = await this.addGroupFromProfile(profile); + + let connectionProfile = this.getConnectionProfileInstance(profile, groupId); + // Profile to be stored during an edit, used to check for duplicate profile edits. + let firstMatchProfile = undefined; + + profiles.find(value => { + const providerConnectionProfile = ConnectionProfile.createFromStoredProfile(value, this._capabilitiesService); + const match = matcher(providerConnectionProfile, connectionProfile); + // If we have a profile match, and the matcher is an edit, we must store this match. + if (match && (matcher.toString() !== ConnectionProfile.matchesProfile.toString())) { + firstMatchProfile = value; + } + return match; + }); + + // If a profile edit, we must now check to see it does not match the other profiles available. + if (firstMatchProfile) { + // Copy over profile list so that we can remove the actual profile we want to edit. + const index = profiles.indexOf(firstMatchProfile); + if (index > -1) { + profiles.splice(index, 1); + } + + // Use the regular profile matching here to find if edit is duplicate. + let matchesExistingProfile = profiles.find(value => { + const providerConnectionProfile = ConnectionProfile.createFromStoredProfile(value, this._capabilitiesService); + const match = ConnectionProfile.matchesProfile(providerConnectionProfile, connectionProfile); + return match; + }); + + return matchesExistingProfile !== undefined; + } + return false; + } + /** * Add a new connection to the connection config. */ - public addConnection(profile: IConnectionProfile, matcher: ProfileMatcher = ConnectionProfile.matchesProfile): Promise { + public async addConnection(profile: IConnectionProfile, matcher: ProfileMatcher = ConnectionProfile.matchesProfile): Promise { if (profile.saveProfile) { - return this.addGroupFromProfile(profile).then(groupId => { - let profiles = deepClone(this.configurationService.inspect(CONNECTIONS_CONFIG_KEY).userValue as IConnectionProfileStore[]); - if (!profiles) { - profiles = []; - } + let groupId = await this.addGroupFromProfile(profile) - let connectionProfile = this.getConnectionProfileInstance(profile, groupId); - let newProfile = ConnectionProfile.convertToProfileStore(this._capabilitiesService, connectionProfile); - // Remove the profile if already set - let sameProfileInList = profiles.find(value => { - const providerConnectionProfile = ConnectionProfile.createFromStoredProfile(value, this._capabilitiesService); - return matcher(providerConnectionProfile, connectionProfile); - }); - if (sameProfileInList) { - let profileIndex = profiles.findIndex(value => value === sameProfileInList); - profiles[profileIndex] = newProfile; - } else { - profiles.push(newProfile); - } + let profiles = deepClone(this.configurationService.inspect(CONNECTIONS_CONFIG_KEY).userValue as IConnectionProfileStore[]); + if (!profiles) { + profiles = []; + } - return this.configurationService.updateValue(CONNECTIONS_CONFIG_KEY, profiles, ConfigurationTarget.USER).then(() => { - profiles.forEach(p => { - if (p && isDisposable(p)) { - p.dispose(); - } - }); - return connectionProfile; - }); + let connectionProfile = this.getConnectionProfileInstance(profile, groupId); + let newProfile = ConnectionProfile.convertToProfileStore(this._capabilitiesService, connectionProfile); + + // Remove the profile if already set + let sameProfileInList = profiles.find(value => { + const providerConnectionProfile = ConnectionProfile.createFromStoredProfile(value, this._capabilitiesService); + return matcher(providerConnectionProfile, connectionProfile); }); + if (sameProfileInList) { + let profileIndex = profiles.findIndex(value => value === sameProfileInList); + profiles[profileIndex] = newProfile; + } else { + profiles.push(newProfile); + } + + await this.configurationService.updateValue(CONNECTIONS_CONFIG_KEY, profiles, ConfigurationTarget.USER); + profiles.forEach(p => { + if (p && isDisposable(p)) { + p.dispose(); + } + }); + return connectionProfile; } else { - return Promise.resolve(profile); + return profile; } } @@ -122,15 +167,16 @@ export class ConnectionConfig { /** *Returns group id */ - public addGroupFromProfile(profile: IConnectionProfile): Promise { + public async addGroupFromProfile(profile: IConnectionProfile): Promise { if (profile.groupId && profile.groupId !== Utils.defaultGroupId) { - return Promise.resolve(profile.groupId); + return profile.groupId; } else { let groups = deepClone(this.configurationService.inspect(GROUPS_CONFIG_KEY).userValue as IConnectionProfileGroup[]); let result = this.saveGroup(groups!, profile.groupFullName, undefined, undefined); groups = result.groups; - return this.configurationService.updateValue(GROUPS_CONFIG_KEY, groups, ConfigurationTarget.USER).then(() => result.newGroupId!); + await this.configurationService.updateValue(GROUPS_CONFIG_KEY, groups, ConfigurationTarget.USER); + return result.newGroupId!; } } diff --git a/src/sql/platform/connection/common/connectionStore.ts b/src/sql/platform/connection/common/connectionStore.ts index 637376c362..3548bf45ce 100644 --- a/src/sql/platform/connection/common/connectionStore.ts +++ b/src/sql/platform/connection/common/connectionStore.ts @@ -159,14 +159,27 @@ export class ConnectionStore { return this.connectionConfig.addGroup(profile); } - private saveProfileToConfig(profile: IConnectionProfile, matcher?: ProfileMatcher): Promise { + private async saveProfileToConfig(profile: IConnectionProfile, matcher?: ProfileMatcher): Promise { if (profile.saveProfile) { - return this.connectionConfig.addConnection(profile, matcher); + let result = await this.connectionConfig.addConnection(profile, matcher); + return result; } else { return Promise.resolve(profile); } } + /** + * Checks to see if a connection profile edit is not identical to an existing saved profile. + * + * @param profile the profile group that is being edited. + * @param matcher the profile matching function for the actual connection we want to edit. + * @returns a boolean value indicating if there's an identical profile to the edit. + */ + public async isDuplicateEdit(profile: IConnectionProfile, matcher?: ProfileMatcher): Promise { + let result = await this.connectionConfig.isDuplicateEdit(profile, matcher); + return result; + } + /** * Gets the list of recently used connections. These will not include the password - a separate call to * {addSavedPassword} is needed to fill that before connecting diff --git a/src/sql/platform/connection/common/providerConnectionInfo.ts b/src/sql/platform/connection/common/providerConnectionInfo.ts index c12aa85a83..2c6239295d 100644 --- a/src/sql/platform/connection/common/providerConnectionInfo.ts +++ b/src/sql/platform/connection/common/providerConnectionInfo.ts @@ -8,7 +8,7 @@ import { isString } from 'vs/base/common/types'; import * as azdata from 'azdata'; import * as Constants from 'sql/platform/connection/common/constants'; import { ICapabilitiesService, ConnectionProviderProperties } from 'sql/platform/capabilities/common/capabilitiesService'; -import { ConnectionOptionSpecialType, ServiceOptionType } from 'sql/platform/connection/common/interfaces'; +import { ConnectionOptionSpecialType } from 'sql/platform/connection/common/interfaces'; import { localize } from 'vs/nls'; type SettableProperty = 'serverName' | 'authenticationType' | 'databaseName' | 'password' | 'connectionName' | 'userName'; @@ -234,6 +234,28 @@ export class ProviderConnectionInfo implements azdata.ConnectionInfo { this.providerName + ProviderConnectionInfo.idSeparator + idValues.join(ProviderConnectionInfo.idSeparator); } + /** + * Returns a more readable version of the options key intended for display areas, replaces the regular separators with display separators + * @param optionsKey options key in the original format. + */ + public static getDisplayOptionsKey(optionsKey: string) { + let ids: string[] = optionsKey.split(ProviderConnectionInfo.idSeparator); + ids = ids.map(id => { + let result = ''; + let idParts = id.split(ProviderConnectionInfo.nameValueSeparator); + // Filter out group name for display purposes as well as empty values. + if (idParts[0] !== 'group' && idParts[1] !== '') { + result = idParts[0] + ProviderConnectionInfo.displayNameValueSeparator; + if (idParts.length >= 2) { + result += idParts.slice(1).join(ProviderConnectionInfo.nameValueSeparator); + } + } + return result; + }); + ids = ids.filter(id => id !== ''); + return ids.join(ProviderConnectionInfo.displayIdSeparator); + } + public static getProviderFromOptionsKey(optionsKey: string) { let providerId: string = ''; if (optionsKey) { @@ -291,29 +313,11 @@ export class ProviderConnectionInfo implements azdata.ConnectionInfo { return ':'; } - public get titleParts(): string[] { - let parts: string[] = []; - // Always put these three on top. TODO: maybe only for MSSQL? - parts.push(this.serverName); - parts.push(this.databaseName); - parts.push(this.authenticationTypeDisplayName); + public static get displayIdSeparator(): string { + return '; '; + } - if (this.serverCapabilities) { - this.serverCapabilities.connectionOptions.forEach(element => { - if (element.specialValueType !== ConnectionOptionSpecialType.serverName && - element.specialValueType !== ConnectionOptionSpecialType.databaseName && - element.specialValueType !== ConnectionOptionSpecialType.authType && - element.specialValueType !== ConnectionOptionSpecialType.password && - element.specialValueType !== ConnectionOptionSpecialType.connectionName && - element.isIdentity && element.valueType === ServiceOptionType.string) { - let value = this.getOptionValue(element.name); - if (value) { - parts.push(value); - } - } - }); - } - - return parts; + public static get displayNameValueSeparator(): string { + return '='; } } diff --git a/src/sql/platform/connection/test/common/connectionConfig.test.ts b/src/sql/platform/connection/test/common/connectionConfig.test.ts index 03ff595c19..0723802171 100644 --- a/src/sql/platform/connection/test/common/connectionConfig.test.ts +++ b/src/sql/platform/connection/test/common/connectionConfig.test.ts @@ -778,4 +778,72 @@ suite('ConnectionConfig', () => { assert.strictEqual(editGroups.length, testGroups.length); } }); + + test('isDuplicateEdit should return true if an edit profile matches an existing profile', async () => { + let originalProfile: IConnectionProfile = { + serverName: 'server3', + databaseName: 'database', + userName: 'user', + password: 'password', + authenticationType: '', + savePassword: true, + groupFullName: 'g3', + groupId: 'g3', + getOptionsKey: () => { return 'connectionId'; }, + matches: undefined!, + providerName: 'MSSQL', + options: {}, + saveProfile: true, + id: 'server3-2', + connectionName: undefined! + }; + let changedProfile: IConnectionProfile = { + serverName: 'server3', + databaseName: 'database', + userName: 'user', + password: 'password', + authenticationType: '', + savePassword: true, + groupFullName: 'test', + groupId: 'test', + getOptionsKey: () => { return 'connectionId'; }, + matches: undefined!, + providerName: 'MSSQL', + options: {}, + saveProfile: true, + id: 'server3-2', + connectionName: undefined! + }; + let existingProfile = ConnectionProfile.convertToProfileStore(capabilitiesService.object, { + serverName: 'server3', + databaseName: 'database', + userName: 'user', + password: 'password', + authenticationType: '', + savePassword: true, + groupFullName: 'test', + groupId: 'test', + getOptionsKey: () => { return 'connectionId'; }, + matches: undefined!, + providerName: 'MSSQL', + options: {}, + saveProfile: true, + id: 'server3', + connectionName: undefined! + }); + + let _testConnections = [...deepClone(testConnections), existingProfile, originalProfile]; + + let configurationService = new TestConfigurationService(); + configurationService.updateValue('datasource.connections', _testConnections, ConfigurationTarget.USER); + + let connectionProfile = new ConnectionProfile(capabilitiesService.object, changedProfile); + + let config = new ConnectionConfig(configurationService, capabilitiesService.object); + + let matcher = (a: IConnectionProfile, b: IConnectionProfile) => a.id === originalProfile.id; + let result = await config.isDuplicateEdit(connectionProfile, matcher); + + assert(result, 'Matcher did not find a match for identical edit'); + }); }); diff --git a/src/sql/platform/connection/test/common/providerConnectionInfo.test.ts b/src/sql/platform/connection/test/common/providerConnectionInfo.test.ts index 74a2308eee..f8b45f9704 100644 --- a/src/sql/platform/connection/test/common/providerConnectionInfo.test.ts +++ b/src/sql/platform/connection/test/common/providerConnectionInfo.test.ts @@ -258,16 +258,6 @@ suite('SQL ProviderConnectionInfo tests', () => { assert.notStrictEqual(conn.getOptionsKey(), conn2.getOptionsKey()); }); - test('titleParts should return server, database and auth type as first items', () => { - let conn = new ProviderConnectionInfo(capabilitiesService, connectionProfile); - let titleParts = conn.titleParts; - assert.strictEqual(titleParts.length, 4); - assert.strictEqual(titleParts[0], connectionProfile.serverName); - assert.strictEqual(titleParts[1], connectionProfile.databaseName); - assert.strictEqual(titleParts[2], connectionProfile.authenticationType); - assert.strictEqual(titleParts[3], connectionProfile.userName); - }); - test('getProviderFromOptionsKey should return the provider name from the options key successfully', () => { let optionsKey = `providerName:${mssqlProviderName}|authenticationType:|databaseName:database|serverName:new server|userName:user`; let actual = ProviderConnectionInfo.getProviderFromOptionsKey(optionsKey); diff --git a/src/sql/workbench/services/connection/browser/connectionManagementService.ts b/src/sql/workbench/services/connection/browser/connectionManagementService.ts index 11eec6dfb6..26dfc082af 100644 --- a/src/sql/workbench/services/connection/browser/connectionManagementService.ts +++ b/src/sql/workbench/services/connection/browser/connectionManagementService.ts @@ -533,12 +533,34 @@ export class ConnectionManagementService extends Disposable implements IConnecti return this.connectWithOptions(connection, uri, options, callbacks); } + + private duplicateEditErrorMessage(connection: interfaces.IConnectionProfile): void { + let groupNameBase = ConnectionProfile.displayIdSeparator + 'groupName' + ConnectionProfile.displayNameValueSeparator; + let connectionOptionsKey = ConnectionProfile.getDisplayOptionsKey(connection.getOptionsKey()); + // Must get connection group name here as it may not always be initialized. + let connectionGroupName = (connection.groupFullName !== undefined && connection.groupFullName !== '' && connection.groupFullName !== '/') ? + (groupNameBase + connection.groupFullName) : (groupNameBase + ''); + this._logService.error(`Profile edit for '${connection.id}' matches an existing profile with data: '${connectionOptionsKey}'`); + throw new Error(nls.localize('connection.duplicateEditErrorMessage', 'Cannot save profile, the selected connection matches an existing profile with the same server info in the same group: \n\n {0}{1}', connectionOptionsKey, connectionGroupName)); + } + private async connectWithOptions(connection: interfaces.IConnectionProfile, uri: string, options?: IConnectionCompletionOptions, callbacks?: IConnectionCallbacks): Promise { connection.options['groupId'] = connection.groupId; connection.options['databaseDisplayName'] = connection.databaseName; let isEdit = options?.params?.isEditConnection ?? false; + let matcher: interfaces.ProfileMatcher; + if (isEdit) { + matcher = (a: interfaces.IConnectionProfile, b: interfaces.IConnectionProfile) => a.id === options.params.oldProfileId; + + //Check to make sure the edits are not identical to another connection. + let result = await this._connectionStore.isDuplicateEdit(connection, matcher); + if (result) { + this.duplicateEditErrorMessage(connection); + } + } + if (!uri) { uri = Utils.generateUri(connection); } @@ -586,10 +608,6 @@ export class ConnectionManagementService extends Disposable implements IConnecti callbacks.onConnectSuccess(options.params, connectionResult.connectionProfile); } if (options.saveTheConnection || isEdit) { - let matcher: interfaces.ProfileMatcher; - if (isEdit) { - matcher = (a: interfaces.IConnectionProfile, b: interfaces.IConnectionProfile) => a.id === options.params.oldProfileId; - } await this.saveToSettings(uri, connection, matcher).then(value => { this._onAddConnectionProfile.fire(connection); diff --git a/src/sql/workbench/services/connection/test/browser/connectionManagementService.test.ts b/src/sql/workbench/services/connection/test/browser/connectionManagementService.test.ts index 4540c42f8a..7bd0372697 100644 --- a/src/sql/workbench/services/connection/test/browser/connectionManagementService.test.ts +++ b/src/sql/workbench/services/connection/test/browser/connectionManagementService.test.ts @@ -1013,6 +1013,11 @@ suite('SQL ConnectionManagementService tests', () => { showFirewallRuleOnError: true }; + connectionStore.setup(x => x.isDuplicateEdit(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => { + //In a real scenario this would be false as it would match the first instance and not find a duplicate. + return Promise.resolve(false); + }); + await connect(uri1, options, true, profile); let newProfile = Object.assign({}, connectionProfile); newProfile.connectionName = newname; @@ -1047,6 +1052,9 @@ suite('SQL ConnectionManagementService tests', () => { showFirewallRuleOnError: true }; + // In an actual edit situation, the profile options would be different for different URIs, as a placeholder, we check the test uris instead here. + connectionStore.setup(x => x.isDuplicateEdit(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve(uri1 === uri2)); + await connect(uri1, options, true, profile); options.params.isEditConnection = true; await connect(uri2, options, true, profile); @@ -1055,6 +1063,47 @@ suite('SQL ConnectionManagementService tests', () => { assert.strictEqual(uri1info.connectionProfile.id, uri2info.connectionProfile.id); }); + test('Edit Connection - Connecting with an already connected profile via edit should throw an error', async () => { + let uri1 = 'test_uri1'; + let profile = Object.assign({}, connectionProfile); + profile.id = '0451'; + let options: IConnectionCompletionOptions = { + params: { + connectionType: ConnectionType.editor, + input: { + onConnectSuccess: undefined, + onConnectReject: undefined, + onConnectStart: undefined, + onDisconnect: undefined, + onConnectCanceled: undefined, + uri: uri1 + }, + queryRange: undefined, + runQueryOnCompletion: RunQueryOnConnectionMode.none, + isEditConnection: true + }, + saveTheConnection: true, + showDashboard: false, + showConnectionDialogOnError: true, + showFirewallRuleOnError: true + }; + + let originalProfileKey = ''; + connectionStore.setup(x => x.isDuplicateEdit(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns((inputProfile, matcher) => { + let newProfile = ConnectionProfile.fromIConnectionProfile(new TestCapabilitiesService(), inputProfile); + let result = newProfile.getOptionsKey() === originalProfileKey; + return Promise.resolve(result) + }); + + await connect(uri1, options, true, profile); + let originalProfile = ConnectionProfile.fromIConnectionProfile(new TestCapabilitiesService(), connectionProfile); + originalProfileKey = originalProfile.getOptionsKey(); + let newProfile = Object.assign({}, connectionProfile); + options.params.isEditConnection = true; + await assert.rejects(async () => await connect(uri1, options, true, newProfile)); + }); + + test('failed firewall rule should open the firewall rule dialog', async () => { handleFirewallRuleResult.canHandleFirewallRule = true;