From 9fdeec612868e3e68a3541b1172c9748d2026765 Mon Sep 17 00:00:00 2001 From: Charles Gagnon Date: Tue, 12 Mar 2019 14:05:50 -0700 Subject: [PATCH] #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 --- .../viewlet/treeSelectionHandler.ts | 2 +- .../common/connectionManagementService.ts | 16 +- .../connection/common/connectionProfile.ts | 7 + .../connection/common/connectionStore.ts | 32 ++-- .../connectionManagementService.test.ts | 1 - .../parts/connection/connectionStore.test.ts | 154 +++++++++--------- 6 files changed, 118 insertions(+), 94 deletions(-) diff --git a/src/sql/parts/objectExplorer/viewlet/treeSelectionHandler.ts b/src/sql/parts/objectExplorer/viewlet/treeSelectionHandler.ts index 9304913e96..786f8684fd 100644 --- a/src/sql/parts/objectExplorer/viewlet/treeSelectionHandler.ts +++ b/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 diff --git a/src/sql/platform/connection/common/connectionManagementService.ts b/src/sql/platform/connection/common/connectionManagementService.ts index 7492fab72e..22ab12db69 100644 --- a/src/sql/platform/connection/common/connectionManagementService.ts +++ b/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)) { diff --git a/src/sql/platform/connection/common/connectionProfile.ts b/src/sql/platform/connection/common/connectionProfile.ts index b4b305e9df..f9b6bfa277 100644 --- a/src/sql/platform/connection/common/connectionProfile.ts +++ b/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) { diff --git a/src/sql/platform/connection/common/connectionStore.ts b/src/sql/platform/connection/common/connectionStore.ts index 3b34ea19b7..8fae8094af 100644 --- a/src/sql/platform/connection/common/connectionStore.ts +++ b/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} a Promise that returns when the connection was saved */ - public addActiveConnection(conn: IConnectionProfile, isConnectionToDefaultDb: boolean = false): Promise { - if (this.getActiveConnections().some(existingConn => existingConn.id === conn.id)) { - return Promise.resolve(undefined); - } 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); - }); + public async addActiveConnection(conn: IConnectionProfile, addToMru: boolean): Promise { + if (addToMru) { + await this.addConnectionToMru(conn); } + + // 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 { + 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 { diff --git a/src/sqltest/parts/connection/connectionManagementService.test.ts b/src/sqltest/parts/connection/connectionManagementService.test.ts index d6582f68a8..bd2b8a0fe1 100644 --- a/src/sqltest/parts/connection/connectionManagementService.test.ts +++ b/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)); diff --git a/src/sqltest/parts/connection/connectionStore.test.ts b/src/sqltest/parts/connection/connectionStore.test.ts index 4f1b590886..1e5920b02b 100644 --- a/src/sqltest/parts/connection/connectionStore.test.ts +++ b/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(); 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(() => { - return connectionStore.addActiveConnection(connectionProfile); - }).then(() => { - let current = connectionStore.getRecentlyUsedConnections(); - if (i >= maxRecent) { - assert.equal(current.length, maxRecent, `expect only top ${maxRecent} creds to be saved`); - } else { - 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.ok(!current[0].password); - }); + await connectionStore.addActiveConnection(connectionProfile, true); + + let current = connectionStore.getRecentlyUsedConnections(); + if (i >= maxRecent) { + assert.equal(current.length, maxRecent, `expect only top ${maxRecent} creds to be saved to MRU`); + } else { + assert.equal(current.length, i + 1, `expect all credentials to be saved to MRU ${current.length}|${i + 1} `); + } + assert.equal(current[0].serverName, cred.serverName, 'Expect most recently saved item to be first in list'); + assert.ok(!current[0].password); } - promise.then(() => { - credentialStore.verify(x => x.saveCredential(TypeMoq.It.isAny(), TypeMoq.It.isAny()), TypeMoq.Times.exactly(numCreds)); - let recentConnections = connectionStore.getActiveConnections(); - assert.equal(numCreds, recentConnections.length, `expect number of active connection ${numCreds}|${recentConnections.length} `); - done(); - }, err => { - // Must call done here so test indicates it's finished if errors occur - done(err); - }); + credentialStore.verify(x => x.saveCredential(TypeMoq.It.isAny(), TypeMoq.It.isAny()), TypeMoq.Times.exactly(numCreds)); + let recentConnections = connectionStore.getActiveConnections(); + assert.equal(numCreds, recentConnections.length, `expect number of active connection ${numCreds}|${recentConnections.length} `); + }); + + test('addActiveConnection with addToMru as false should not add any recent connections', 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)); + + 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(() => { - return connectionStore.addActiveConnection(defaultNamedConnectionProfile); - }).then(() => { - return connectionStore.addActiveConnection(connectionProfile); - }).then(() => { - return connectionStore.addActiveConnection(connectionProfile); - }).then(() => { - let current = connectionStore.getRecentlyUsedConnections(); - 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)); + await connectionStore.addActiveConnection(defaultNamedConnectionProfile, true); + await connectionStore.addActiveConnection(connectionProfile, true); + await connectionStore.addActiveConnection(connectionProfile, true); + + let recentConnections = connectionStore.getRecentlyUsedConnections(); + assert.equal(recentConnections.length, 2, 'expect 2 unique credentials to have been added'); + assert.equal(recentConnections[0].serverName, cred.serverName, 'Expect most recently saved item to be first in list'); + assert.ok(!recentConnections[0].password); }); - 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(); - promise = promise.then(() => { - expectedCredCount++; - return connectionStore.addActiveConnection(connectionProfile); - }).then(() => { - let current = connectionStore.getRecentlyUsedConnections(); - // 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()); - assert.strictEqual(capturedCreds.password, defaultNamedProfile.password); - let credId: string = capturedCreds.credentialId; - assert.ok(credId.includes(ConnectionStore.CRED_PROFILE_USER), 'Expect credential to be marked as an Profile cred'); - assert.ok(!current[0].password); - }).then(() => { - // When add integrated auth connection - expectedCredCount++; - let integratedCredConnectionProfile = new ConnectionProfile(capabilitiesService, integratedCred); - return connectionStore.addActiveConnection(integratedCredConnectionProfile); - }).then(() => { - let current = connectionStore.getRecentlyUsedConnections(); - // 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()); - assert.equal(current.length, expectedCredCount, `expect ${expectedCredCount} unique credentials to have been added`); - }).then(() => { - // When add connection without password - expectedCredCount++; - let noPwdCredConnectionProfile = new ConnectionProfile(capabilitiesService, noPwdCred); - return connectionStore.addActiveConnection(noPwdCredConnectionProfile); - }).then(() => { - let current = connectionStore.getRecentlyUsedConnections(); - // 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()); - assert.equal(current.length, expectedCredCount, `expect ${expectedCredCount} unique credentials to have been added`); - }).then(() => done(), err => done(err)); + expectedCredCount++; + // Connection with stored password + await connectionStore.addActiveConnection(connectionProfile, true); + let recentConnections = connectionStore.getActiveConnections(); + + // Then verify that saveCredential was called and correctly stored the password + credentialStore.verify(x => x.saveCredential(TypeMoq.It.isAny(), TypeMoq.It.isAny()), TypeMoq.Times.once()); + assert.strictEqual(capturedCreds.password, defaultNamedProfile.password); + let credId: string = capturedCreds.credentialId; + assert.ok(credId.includes(ConnectionStore.CRED_PROFILE_USER), 'Expect credential to be marked as an Profile cred'); + assert.ok(!recentConnections[0].password); + + // Integrated auth + expectedCredCount++; + let integratedCredConnectionProfile = new ConnectionProfile(capabilitiesService, integratedCred); + await connectionStore.addActiveConnection(integratedCredConnectionProfile, true); + + recentConnections = connectionStore.getActiveConnections(); + // We shouldn't see an increase in the calls to saveCredential, but the MRU should be increased + 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`); + + // Connection with blank (no) password + expectedCredCount++; + let noPwdCredConnectionProfile = new ConnectionProfile(capabilitiesService, noPwdCred); + await connectionStore.addActiveConnection(noPwdCredConnectionProfile, true); + + recentConnections = connectionStore.getActiveConnections(); + // We shouldn't see an increase in the calls to saveCredential, but the MRU should be increased + 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`); + }); test('can clear connections list', (done) => {