#3224 Fix extra connections populating MRU List (#4368)

* Modify where we add active connections so that we can use the saveTheConnection option to decide whether to add a connection to the MRU. This was necessary because the old location was called from onConnectionComplete which is sent by the SqlToolsService - and that doesn't get any UI related information like the options. The new location is still only called after the connection completes and will be added only if the connection succeeds.

Added new test and updated existing tests to handle new logic (plus a bit of async-refactoring).

* Fix couple spacing issues

* Add logic back in to short-circuit if we already have the connection in the active connections list.

* Fix spaces -> tabs

src/sql/parts/objectExplorer/viewlet/treeSelectionHandler.ts

@@ -81,7 +81,7 @@ export class TreeSelectionHandler {
 		let connectionProfile: ConnectionProfile = undefined;
 		let options: IConnectionCompletionOptions = {
 			params: undefined,
-			saveTheConnection: false,
+			saveTheConnection: true,
 			showConnectionDialogOnError: true,
 			showFirewallRuleOnError: true,
 			showDashboard: isDoubleClick // only show the dashboard if the action is double click

src/sql/platform/connection/common/connectionManagementService.ts

@@ -464,6 +464,12 @@ export class ConnectionManagementService extends Disposable implements IConnecti
 			}
 			this.createNewConnection(uri, connection).then(connectionResult => {
 				if (connectionResult && connectionResult.connected) {
+					// The connected succeeded so add it to our active connections now, optionally adding it to the MRU based on
+					// the options.saveTheConnection setting
+					let connectionMgmtInfo = this._connectionStatusManager.findConnection(uri);
+					let activeConnection = connectionMgmtInfo.connectionProfile;
+					this.tryAddActiveConnection(connectionMgmtInfo, activeConnection, options.saveTheConnection);
+
 					if (callbacks.onConnectSuccess) {
 						callbacks.onConnectSuccess(options.params);
 					}
@@ -860,9 +866,9 @@ export class ConnectionManagementService extends Disposable implements IConnecti
 	/**
 	 * Add a connection to the active connections list.
 	 */
-	private tryAddActiveConnection(connectionManagementInfo: ConnectionManagementInfo, newConnection: IConnectionProfile, isConnectionToDefaultDb: boolean): void {
+	private tryAddActiveConnection(connectionManagementInfo: ConnectionManagementInfo, newConnection: IConnectionProfile, addToMru: boolean): void {
 		if (newConnection) {
-			this._connectionStore.addActiveConnection(newConnection, isConnectionToDefaultDb)
+			this._connectionStore.addActiveConnection(newConnection, addToMru)
 				.then(() => {
 					connectionManagementInfo.connectHandler(true);
 				}, err => {
@@ -896,10 +902,6 @@ export class ConnectionManagementService extends Disposable implements IConnecti
 		let connection = this._connectionStatusManager.onConnectionComplete(info);
 
 		if (info.connectionId) {
-			let isConnectionToDefaultDb = false;
-			if (connection.connectionProfile && (!connection.connectionProfile.databaseName || connection.connectionProfile.databaseName.trim() === '')) {
-				isConnectionToDefaultDb = true;
-			}
 			if (info.connectionSummary && info.connectionSummary.databaseName) {
 				this._connectionStatusManager.updateDatabaseName(info);
 			}
@@ -907,8 +909,6 @@ export class ConnectionManagementService extends Disposable implements IConnecti
 			connection.extensionTimer.stop();
 
 			connection.connectHandler(true);
-			let activeConnection = connection.connectionProfile;
-			self.tryAddActiveConnection(connection, activeConnection, isConnectionToDefaultDb);
 			self.addTelemetryForConnection(connection);
 
 			if (self._connectionStatusManager.isDefaultTypeUri(info.ownerUri)) {

src/sql/platform/connection/common/connectionProfile.ts

@@ -181,6 +181,13 @@ export class ConnectionProfile extends ProviderConnectionInfo implements interfa
 		};
 	}
 
+	/**
+	 * Returns whether this profile is connected to the default database (it doesn't specify a database to connect to)
+	 */
+	public static isConnectionToDefaultDb(profile: azdata.IConnectionProfile): boolean {
+		return !profile.databaseName || profile.databaseName.trim() === '';
+	}
+
 	public static fromIConnectionProfile(capabilitiesService: ICapabilitiesService, profile: azdata.IConnectionProfile) {
 		if (profile) {
 			if (profile instanceof ConnectionProfile) {

src/sql/platform/connection/common/connectionStore.ts

@@ -277,20 +277,30 @@ export class ConnectionStore {
 	 * Password values are stored to a separate credential store if the "savePassword" option is true
 	 *
 	 * @param {IConnectionCredentials} conn the connection to add
+	 * @param {boolean} addToMru Whether to add this connection to the MRU
 	 * @returns {Promise<void>} a Promise that returns when the connection was saved
 	 */
-	public addActiveConnection(conn: IConnectionProfile, isConnectionToDefaultDb: boolean = false): Promise<void> {
+	public async addActiveConnection(conn: IConnectionProfile, addToMru: boolean): Promise<void> {
-		if (this.getActiveConnections().some(existingConn => existingConn.id === conn.id)) {
+		if (addToMru) {
-			return Promise.resolve(undefined);
+			await this.addConnectionToMru(conn);
-		} else {
-			return this.addConnectionToMemento(conn, Constants.activeConnections, undefined, conn.savePassword).then(() => {
-				let maxConnections = this.getMaxRecentConnectionsCount();
-				if (isConnectionToDefaultDb) {
-					conn.databaseName = '';
-				}
-				return this.addConnectionToMemento(conn, Constants.recentConnections, maxConnections);
-			});
 		}
+
+		// Only add connections we don't already know about
+		if (!this.getActiveConnections().some(existingConn => existingConn.id === conn.id)) {
+			await this.addConnectionToMemento(conn, Constants.activeConnections, undefined, conn.savePassword);
+		}
+	}
+
+	/**
+	 * Adds the specified connection to the MRU list
+	 * @param conn The connection to add
+	 */
+	private async addConnectionToMru(conn: IConnectionProfile): Promise<void> {
+		let maxConnections = this.getMaxRecentConnectionsCount();
+		if (ConnectionProfile.isConnectionToDefaultDb(conn)) {
+			conn.databaseName = '';
+		}
+		await this.addConnectionToMemento(conn, Constants.recentConnections, maxConnections);
 	}
 
 	public addConnectionToMemento(conn: IConnectionProfile, mementoKey: string, maxConnections?: number, savePassword?: boolean): Promise<void> {

src/sqltest/parts/connection/connectionManagementService.test.ts

@@ -99,7 +99,6 @@ suite('SQL ConnectionManagementService tests', () => {
 		connectionDialogService.setup(x => x.showDialog(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => TPromise.as(none));
 		connectionDialogService.setup(x => x.showDialog(TypeMoq.It.isAny(), TypeMoq.It.isAny(), undefined, TypeMoq.It.isAny())).returns(() => TPromise.as(none));
 
-		connectionStore.setup(x => x.addActiveConnection(TypeMoq.It.isAny())).returns(() => Promise.resolve());
 		connectionStore.setup(x => x.addActiveConnection(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve());
 		connectionStore.setup(x => x.saveProfile(TypeMoq.It.isAny())).returns(() => Promise.resolve(connectionProfile));
 		workbenchEditorService.setup(x => x.openEditor(undefined, TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => TPromise.as(undefined));

src/sqltest/parts/connection/connectionStore.test.ts

@@ -227,7 +227,7 @@ suite('SQL ConnectionStore tests', () => {
 		defaultNamedConnectionProfile = new ConnectionProfile(capabilitiesService, defaultNamedProfile);
 	});
 
-	test('addActiveConnection should limit recent connection saves to the MaxRecentConnections amount', (done) => {
+	test('addActiveConnection should limit recent connection saves to the MaxRecentConnections amount', async () => {
 		// Given 5 is the max # creds
 		let numCreds = 6;
 
@@ -239,32 +239,47 @@ suite('SQL ConnectionStore tests', () => {
 		// Expect all of them to be saved even if size is limited to 3
 		let connectionStore = new ConnectionStore(storageServiceMock.object, context.object, undefined, workspaceConfigurationServiceMock.object,
 			credentialStore.object, capabilitiesService, connectionConfig.object);
-		let promise = Promise.resolve<void>();
 		for (let i = 0; i < numCreds; i++) {
 			let cred = Object.assign({}, defaultNamedProfile, { serverName: defaultNamedProfile.serverName + i });
 			let connectionProfile = new ConnectionProfile(capabilitiesService, cred);
-			promise = promise.then(() => {
+			await connectionStore.addActiveConnection(connectionProfile, true);
-				return connectionStore.addActiveConnection(connectionProfile);
+
-			}).then(() => {
+			let current = connectionStore.getRecentlyUsedConnections();
-				let current = connectionStore.getRecentlyUsedConnections();
+			if (i >= maxRecent) {
-				if (i >= maxRecent) {
+				assert.equal(current.length, maxRecent, `expect only top ${maxRecent} creds to be saved to MRU`);
-					assert.equal(current.length, maxRecent, `expect only top ${maxRecent} creds to be saved`);
+			} else {
-				} else {
+				assert.equal(current.length, i + 1, `expect all credentials to be saved to MRU ${current.length}|${i + 1} `);
-					assert.equal(current.length, i + 1, `expect all credentials to be saved ${current.length}|${i + 1} `);
+			}
-				}
+			assert.equal(current[0].serverName, cred.serverName, 'Expect most recently saved item to be first in list');
-				assert.equal(current[0].serverName, cred.serverName, 'Expect most recently saved item to be first in list');
+			assert.ok(!current[0].password);
-				assert.ok(!current[0].password);
-			});
 		}
-		promise.then(() => {
+		credentialStore.verify(x => x.saveCredential(TypeMoq.It.isAny(), TypeMoq.It.isAny()), TypeMoq.Times.exactly(numCreds));
-			credentialStore.verify(x => x.saveCredential(TypeMoq.It.isAny(), TypeMoq.It.isAny()), TypeMoq.Times.exactly(numCreds));
+		let recentConnections = connectionStore.getActiveConnections();
-			let recentConnections = connectionStore.getActiveConnections();
+		assert.equal(numCreds, recentConnections.length, `expect number of active connection ${numCreds}|${recentConnections.length} `);
-			assert.equal(numCreds, recentConnections.length, `expect number of active connection ${numCreds}|${recentConnections.length} `);
+	});
-			done();
+
-		}, err => {
+	test('addActiveConnection with addToMru as false should not add any recent connections', async () => {
-			// Must call done here so test indicates it's finished if errors occur
+		// setup memento for MRU to return a list we have access to
-			done(err);
+		credentialStore.setup(x => x.saveCredential(TypeMoq.It.isAny(), TypeMoq.It.isAny()))
-		});
+			.returns(() => Promise.resolve(true));
+
+		const numCreds = 3;
+		let connectionStore = new ConnectionStore(storageServiceMock.object, context.object, undefined, workspaceConfigurationServiceMock.object,
+			credentialStore.object, capabilitiesService, connectionConfig.object);
+		connectionStore.clearActiveConnections();
+		connectionStore.clearRecentlyUsed();
+
+		for (let i = 0; i < 3; i++) {
+			let cred = Object.assign({}, defaultNamedProfile, { serverName: defaultNamedProfile.serverName + i });
+			let connectionProfile = new ConnectionProfile(capabilitiesService, cred);
+			await connectionStore.addActiveConnection(connectionProfile, false);
+
+			let recentConnections = connectionStore.getRecentlyUsedConnections();
+			let activeConnections = connectionStore.getActiveConnections();
+			assert.equal(recentConnections.length, 0, `expect no entries to be saved to MRU`);
+			assert.equal(activeConnections.length, i + 1, `expect all credentials to be saved to activeConnections ${activeConnections.length}|${i + 1} `);
+		}
+		credentialStore.verify(x => x.saveCredential(TypeMoq.It.isAny(), TypeMoq.It.isAny()), TypeMoq.Times.exactly(numCreds));
 	});
 
 	test('getRecentlyUsedConnections should return connection for given provider', () => {
@@ -275,7 +290,7 @@ suite('SQL ConnectionStore tests', () => {
 		assert.equal(connections.every(c => c.providerName === 'Provider2'), true);
 	});
 
-	test('addActiveConnection should add same connection exactly once', (done) => {
+	test('addActiveConnection should add same connection exactly once', async () => {
 		// setup memento for MRU to return a list we have access to
 		credentialStore.setup(x => x.saveCredential(TypeMoq.It.isAny(), TypeMoq.It.isAny()))
 			.returns(() => Promise.resolve(true));
@@ -286,25 +301,19 @@ suite('SQL ConnectionStore tests', () => {
 			credentialStore.object, capabilitiesService, connectionConfig.object);
 		connectionStore.clearActiveConnections();
 		connectionStore.clearRecentlyUsed();
-		let promise = Promise.resolve();
 		let cred = Object.assign({}, defaultNamedProfile, { serverName: defaultNamedProfile.serverName + 1 });
 		let connectionProfile = new ConnectionProfile(capabilitiesService, cred);
-		promise = promise.then(() => {
+		await connectionStore.addActiveConnection(defaultNamedConnectionProfile, true);
-			return connectionStore.addActiveConnection(defaultNamedConnectionProfile);
+		await connectionStore.addActiveConnection(connectionProfile, true);
-		}).then(() => {
+		await connectionStore.addActiveConnection(connectionProfile, true);
-			return connectionStore.addActiveConnection(connectionProfile);
+
-		}).then(() => {
+		let recentConnections = connectionStore.getRecentlyUsedConnections();
-			return connectionStore.addActiveConnection(connectionProfile);
+		assert.equal(recentConnections.length, 2, 'expect 2 unique credentials to have been added');
-		}).then(() => {
+		assert.equal(recentConnections[0].serverName, cred.serverName, 'Expect most recently saved item to be first in list');
-			let current = connectionStore.getRecentlyUsedConnections();
+		assert.ok(!recentConnections[0].password);
-			assert.equal(current.length, 2, 'expect 2 unique credentials to have been added');
-			assert.equal(current[0].serverName, cred.serverName, 'Expect most recently saved item to be first in list');
-			assert.ok(!current[0].password);
-		}).then(() => done(), err => done(err));
 	});
 
-	test('addActiveConnection should save password to credential store', (done) => {
+	test('addActiveConnection should save password to credential store', async () => {
-
 		// Setup credential store to capture credentials sent to it
 		let capturedCreds: any;
 		credentialStore.setup(x => x.saveCredential(TypeMoq.It.isAny(), TypeMoq.It.isAny()))
@@ -316,11 +325,11 @@ suite('SQL ConnectionStore tests', () => {
 			})
 			.returns(() => Promise.resolve(true));
 
-		// Given we save 1 connection with password and multiple other connections without
 		let connectionStore = new ConnectionStore(storageServiceMock.object, context.object, undefined, workspaceConfigurationServiceMock.object,
 			credentialStore.object, capabilitiesService, connectionConfig.object);
 		connectionStore.clearActiveConnections();
 		connectionStore.clearRecentlyUsed();
+
 		let integratedCred = Object.assign({}, defaultNamedProfile, {
 			serverName: defaultNamedProfile.serverName + 'Integrated',
 			authenticationType: 'Integrated',
@@ -334,39 +343,38 @@ suite('SQL ConnectionStore tests', () => {
 		let connectionProfile = new ConnectionProfile(capabilitiesService, defaultNamedProfile);
 
 		let expectedCredCount = 0;
-		let promise = Promise.resolve();
+		expectedCredCount++;
-		promise = promise.then(() => {
+		// Connection with stored password
-			expectedCredCount++;
+		await connectionStore.addActiveConnection(connectionProfile, true);
-			return connectionStore.addActiveConnection(connectionProfile);
+		let recentConnections = connectionStore.getActiveConnections();
-		}).then(() => {
+
-			let current = connectionStore.getRecentlyUsedConnections();
+		// Then verify that saveCredential was called and correctly stored the password
-			// Then verify that since its password based we save the password
+		credentialStore.verify(x => x.saveCredential(TypeMoq.It.isAny(), TypeMoq.It.isAny()), TypeMoq.Times.once());
-			credentialStore.verify(x => x.saveCredential(TypeMoq.It.isAny(), TypeMoq.It.isAny()), TypeMoq.Times.once());
+		assert.strictEqual(capturedCreds.password, defaultNamedProfile.password);
-			assert.strictEqual(capturedCreds.password, defaultNamedProfile.password);
+		let credId: string = capturedCreds.credentialId;
-			let credId: string = capturedCreds.credentialId;
+		assert.ok(credId.includes(ConnectionStore.CRED_PROFILE_USER), 'Expect credential to be marked as an Profile cred');
-			assert.ok(credId.includes(ConnectionStore.CRED_PROFILE_USER), 'Expect credential to be marked as an Profile cred');
+		assert.ok(!recentConnections[0].password);
-			assert.ok(!current[0].password);
+
-		}).then(() => {
+		// Integrated auth
-			// When add integrated auth connection
+		expectedCredCount++;
-			expectedCredCount++;
+		let integratedCredConnectionProfile = new ConnectionProfile(capabilitiesService, integratedCred);
-			let integratedCredConnectionProfile = new ConnectionProfile(capabilitiesService, integratedCred);
+		await connectionStore.addActiveConnection(integratedCredConnectionProfile, true);
-			return connectionStore.addActiveConnection(integratedCredConnectionProfile);
+
-		}).then(() => {
+		recentConnections = connectionStore.getActiveConnections();
-			let current = connectionStore.getRecentlyUsedConnections();
+		// We shouldn't see an increase in the calls to saveCredential, but the MRU should be increased
-			// then expect no to have credential store called, but MRU count upped to 2
+		credentialStore.verify(x => x.saveCredential(TypeMoq.It.isAny(), TypeMoq.It.isAny()), TypeMoq.Times.once());
-			credentialStore.verify(x => x.saveCredential(TypeMoq.It.isAny(), TypeMoq.It.isAny()), TypeMoq.Times.once());
+		assert.equal(recentConnections.length, expectedCredCount, `expect ${expectedCredCount} unique credentials to have been added`);
-			assert.equal(current.length, expectedCredCount, `expect ${expectedCredCount} unique credentials to have been added`);
+
-		}).then(() => {
+		// Connection with blank (no) password
-			// When add connection without password
+		expectedCredCount++;
-			expectedCredCount++;
+		let noPwdCredConnectionProfile = new ConnectionProfile(capabilitiesService, noPwdCred);
-			let noPwdCredConnectionProfile = new ConnectionProfile(capabilitiesService, noPwdCred);
+		await connectionStore.addActiveConnection(noPwdCredConnectionProfile, true);
-			return connectionStore.addActiveConnection(noPwdCredConnectionProfile);
+
-		}).then(() => {
+		recentConnections = connectionStore.getActiveConnections();
-			let current = connectionStore.getRecentlyUsedConnections();
+		// We shouldn't see an increase in the calls to saveCredential, but the MRU should be increased
-			// then expect no to have credential store called, but MRU count upped to 3
+		credentialStore.verify(x => x.saveCredential(TypeMoq.It.isAny(), TypeMoq.It.isAny()), TypeMoq.Times.once());
-			credentialStore.verify(x => x.saveCredential(TypeMoq.It.isAny(), TypeMoq.It.isAny()), TypeMoq.Times.once());
+		assert.equal(recentConnections.length, expectedCredCount, `expect ${expectedCredCount} unique credentials to have been added`);
-			assert.equal(current.length, expectedCredCount, `expect ${expectedCredCount} unique credentials to have been added`);
+
-		}).then(() => done(), err => done(err));
 	});
 
 	test('can clear connections list', (done) => {