Added back fix for duplicate edits (#23003)

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<boolean> {
+		let profiles = deepClone(this.configurationService.inspect<IConnectionProfileStore[]>(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<IConnectionProfile> {
+	public async addConnection(profile: IConnectionProfile, matcher: ProfileMatcher = ConnectionProfile.matchesProfile): Promise<IConnectionProfile> {
 		if (profile.saveProfile) {
-			return this.addGroupFromProfile(profile).then(groupId => {
-				let profiles = deepClone(this.configurationService.inspect<IConnectionProfileStore[]>(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<IConnectionProfileStore[]>(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<string> {
+	public async addGroupFromProfile(profile: IConnectionProfile): Promise<string> {
 		if (profile.groupId && profile.groupId !== Utils.defaultGroupId) {
-			return Promise.resolve(profile.groupId);
+			return profile.groupId;
 		} else {
 			let groups = deepClone(this.configurationService.inspect<IConnectionProfileGroup[]>(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!;
 		}
 	}
 

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<IConnectionProfile> {
+	private async saveProfileToConfig(profile: IConnectionProfile, matcher?: ProfileMatcher): Promise<IConnectionProfile> {
 		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<boolean> {
+		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

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 '=';
 	}
 }

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');
+	});
 });

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);

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 + '<default>');
+		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<IConnectionResult> {
 		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);

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;