From bbd0972dde8cc3e531c9c215444368064d5bff7c Mon Sep 17 00:00:00 2001 From: Kevin Cunnane Date: Thu, 25 May 2017 18:26:52 -0700 Subject: [PATCH] Ensure connection open for OE queries and fix connection disposal (#359) * Ensure connection open for OE queries and fix connection disposal - Dispose connection in Metadata service, to ensure we cleanly dispose and don't rely on garbage colleciton - Fixed issue where if the connection was closed, expanding databases in the Server would fail. This is because SMO doesn't always reopen the connection, certainly not for Server level queries. The solution is to always check if open and reopen. - Added unit tests for this, which required mocking the relevant IsOpen / OpenConnection methods. Refactored SMO wrapper calls into a dedicated class file to handle this --- .../Metadata/MetadataService.cs | 6 +- .../ObjectExplorer/SmoModel/ServerNode.cs | 28 ++----- .../ObjectExplorer/SmoModel/SmoQuerier.cs | 5 +- .../SmoModel/SmoQueryContext.cs | 75 +++++++++++++++-- .../ObjectExplorer/SmoModel/SmoWrapper.cs | 55 ++++++++++++ .../ObjectExplorer/NodeTests.cs | 84 +++++++++++++++++-- 6 files changed, 218 insertions(+), 35 deletions(-) create mode 100644 src/Microsoft.SqlTools.ServiceLayer/ObjectExplorer/SmoModel/SmoWrapper.cs diff --git a/src/Microsoft.SqlTools.ServiceLayer/Metadata/MetadataService.cs b/src/Microsoft.SqlTools.ServiceLayer/Metadata/MetadataService.cs index 99e52cfb..e5ec100f 100644 --- a/src/Microsoft.SqlTools.ServiceLayer/Metadata/MetadataService.cs +++ b/src/Microsoft.SqlTools.ServiceLayer/Metadata/MetadataService.cs @@ -74,8 +74,10 @@ namespace Microsoft.SqlTools.ServiceLayer.Metadata var metadata = new List(); if (connInfo != null) { - SqlConnection sqlConn = OpenMetadataConnection(connInfo); - ReadMetadata(sqlConn, metadata); + using (SqlConnection sqlConn = OpenMetadataConnection(connInfo)) + { + ReadMetadata(sqlConn, metadata); + } } await requestContext.SendResult(new MetadataQueryResult diff --git a/src/Microsoft.SqlTools.ServiceLayer/ObjectExplorer/SmoModel/ServerNode.cs b/src/Microsoft.SqlTools.ServiceLayer/ObjectExplorer/SmoModel/ServerNode.cs index 208930ea..eb06453d 100644 --- a/src/Microsoft.SqlTools.ServiceLayer/ObjectExplorer/SmoModel/ServerNode.cs +++ b/src/Microsoft.SqlTools.ServiceLayer/ObjectExplorer/SmoModel/ServerNode.cs @@ -30,7 +30,7 @@ namespace Microsoft.SqlTools.ServiceLayer.ObjectExplorer.SmoModel private string connectionUri; private Lazy context; private ConnectionService connectionService; - private SmoServerCreator serverCreator; + private SmoWrapper smoWrapper; private SqlServerType sqlServerType; public ServerNode(ConnectionCompleteParams connInfo, IMultiServiceProvider serviceProvider) @@ -56,19 +56,19 @@ namespace Microsoft.SqlTools.ServiceLayer.ObjectExplorer.SmoModel Label = GetConnectionLabel(); } - internal SmoServerCreator ServerCreator + internal SmoWrapper SmoWrapper { get { - if (serverCreator == null) + if (smoWrapper == null) { - ServerCreator = new SmoServerCreator(); + smoWrapper = new SmoWrapper(); } - return serverCreator; + return smoWrapper; } set { - this.serverCreator = value; + this.smoWrapper = value; } } @@ -161,8 +161,8 @@ namespace Microsoft.SqlTools.ServiceLayer.ObjectExplorer.SmoModel try { - Server server = ServerCreator.Create(connection); - return new SmoQueryContext(server, serviceProvider) + Server server = SmoWrapper.CreateServer(connection); + return new SmoQueryContext(server, serviceProvider, SmoWrapper) { Parent = server, SqlServerType = this.sqlServerType @@ -187,16 +187,4 @@ namespace Microsoft.SqlTools.ServiceLayer.ObjectExplorer.SmoModel return context.Value; } } - - /// - /// Internal for testing purposes only - /// - internal class SmoServerCreator - { - public virtual Server Create(SqlConnection connection) - { - ServerConnection serverConn = new ServerConnection(connection); - return new Server(serverConn); - } - } } diff --git a/src/Microsoft.SqlTools.ServiceLayer/ObjectExplorer/SmoModel/SmoQuerier.cs b/src/Microsoft.SqlTools.ServiceLayer/ObjectExplorer/SmoModel/SmoQuerier.cs index d4da5699..85c619e5 100644 --- a/src/Microsoft.SqlTools.ServiceLayer/ObjectExplorer/SmoModel/SmoQuerier.cs +++ b/src/Microsoft.SqlTools.ServiceLayer/ObjectExplorer/SmoModel/SmoQuerier.cs @@ -92,7 +92,10 @@ namespace Microsoft.SqlTools.ServiceLayer.ObjectExplorer.SmoModel Enumerator en = new Enumerator(); Request request = new Request(new Urn(urn)); ServerConnection serverConnection = new ServerConnection(context.Server.ConnectionContext.SqlConnectionObject); - + if (!serverConnection.IsOpen) + { + serverConnection.Connect(); + } EnumResult result = en.Process(serverConnection, request); urns = GetUrns(result); diff --git a/src/Microsoft.SqlTools.ServiceLayer/ObjectExplorer/SmoModel/SmoQueryContext.cs b/src/Microsoft.SqlTools.ServiceLayer/ObjectExplorer/SmoModel/SmoQueryContext.cs index d6ebdfec..3da36c6c 100644 --- a/src/Microsoft.SqlTools.ServiceLayer/ObjectExplorer/SmoModel/SmoQueryContext.cs +++ b/src/Microsoft.SqlTools.ServiceLayer/ObjectExplorer/SmoModel/SmoQueryContext.cs @@ -7,9 +7,11 @@ using System; using System.Collections; using System.Collections.Generic; using System.Globalization; +using Microsoft.SqlServer.Management.Common; using Microsoft.SqlServer.Management.Smo; using Microsoft.SqlTools.Extensibility; using Microsoft.SqlTools.ServiceLayer.ObjectExplorer.Nodes; +using Microsoft.SqlTools.Utility; namespace Microsoft.SqlTools.ServiceLayer.ObjectExplorer.SmoModel { @@ -18,14 +20,25 @@ namespace Microsoft.SqlTools.ServiceLayer.ObjectExplorer.SmoModel /// public class SmoQueryContext { + private Server server; + private Database database; + private SmoObjectBase parent; + private SmoWrapper smoWrapper; + /// /// Creates a context object with a server to use as the basis for any queries /// /// public SmoQueryContext(Server server, IMultiServiceProvider serviceProvider) + : this(server, serviceProvider, null) { - Server = server; + } + + internal SmoQueryContext(Server server, IMultiServiceProvider serviceProvider, SmoWrapper serverManager) + { + this.server = server; ServiceProvider = serviceProvider; + this.smoWrapper = serverManager ?? new SmoWrapper(); } /// @@ -36,17 +49,40 @@ namespace Microsoft.SqlTools.ServiceLayer.ObjectExplorer.SmoModel /// /// The server SMO will query against /// - public Server Server { get; private set; } + public Server Server { + get + { + return GetObjectWithOpenedConnection(server); + } + } /// /// Optional Database context object to query against /// - public Database Database { get; set; } + public Database Database { + get + { + return GetObjectWithOpenedConnection(database); + } + set + { + database = value; + } + } /// /// Parent of a give node to use for queries /// - public SmoObjectBase Parent { get; set; } + public SmoObjectBase Parent { + get + { + return GetObjectWithOpenedConnection(parent); + } + set + { + parent = value; + } + } /// /// A query loader that can be used to find objects @@ -89,6 +125,7 @@ namespace Microsoft.SqlTools.ServiceLayer.ObjectExplorer.SmoModel return service; } + /// /// Copies the context for use by another node /// @@ -96,13 +133,39 @@ namespace Microsoft.SqlTools.ServiceLayer.ObjectExplorer.SmoModel /// new with all fields except the same public SmoQueryContext CopyWithParent(SmoObjectBase parent) { - SmoQueryContext context = new SmoQueryContext(this.Server, this.ServiceProvider) + SmoQueryContext context = new SmoQueryContext(this.Server, this.ServiceProvider, this.smoWrapper) { - Database = this.Database, + database = this.Database, Parent = parent, SqlServerType = this.SqlServerType }; return context; } + + private T GetObjectWithOpenedConnection(T smoObj) + where T : SmoObjectBase + { + if (smoObj != null) + { + EnsureConnectionOpen(smoObj); + } + return smoObj; + } + + /// + /// Ensures the server objects connection context is open. This is used by all child objects, and + /// the only way to easily access is via the server object. This should be called during access of + /// any of the object properties + /// + private void EnsureConnectionOpen(SmoObjectBase smoObj) + { + if (!smoWrapper.IsConnectionOpen(smoObj)) + { + // We have a closed server connection. Reopen this + // Note: not currently catching connection exceptions. Expect this to bubble + // up to calling methods and be logged there as this would be happening there in any case + smoWrapper.OpenConnection(smoObj); + } + } } } diff --git a/src/Microsoft.SqlTools.ServiceLayer/ObjectExplorer/SmoModel/SmoWrapper.cs b/src/Microsoft.SqlTools.ServiceLayer/ObjectExplorer/SmoModel/SmoWrapper.cs new file mode 100644 index 00000000..33e8ebb6 --- /dev/null +++ b/src/Microsoft.SqlTools.ServiceLayer/ObjectExplorer/SmoModel/SmoWrapper.cs @@ -0,0 +1,55 @@ +// +// Copyright (c) Microsoft. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. +// + +using System; +using System.Data.Common; +using System.Data.SqlClient; +using System.Globalization; +using System.Linq; +using Microsoft.SqlServer.Management.Common; +using Microsoft.SqlServer.Management.Smo; +using Microsoft.SqlTools.Extensibility; +using Microsoft.SqlTools.ServiceLayer.Connection; +using Microsoft.SqlTools.ServiceLayer.Connection.Contracts; +using Microsoft.SqlTools.ServiceLayer.Connection.ReliableConnection; +using Microsoft.SqlTools.ServiceLayer.ObjectExplorer.Nodes; +using Microsoft.SqlTools.ServiceLayer.Utility; +using Microsoft.SqlTools.Utility; + +namespace Microsoft.SqlTools.ServiceLayer.ObjectExplorer.SmoModel +{ + /// + /// Internal for testing purposes only. This class provides wrapper functionality + /// over SMO objects in order to facilitate unit testing + /// + internal class SmoWrapper + { + public virtual Server CreateServer(SqlConnection connection) + { + ServerConnection serverConn = new ServerConnection(connection); + return new Server(serverConn); + } + + public virtual bool IsConnectionOpen(SmoObjectBase smoObj) + { + SqlSmoObject sqlObj = smoObj as SqlSmoObject; + return sqlObj != null + && sqlObj.ExecutionManager != null + && sqlObj.ExecutionManager.ConnectionContext != null + && sqlObj.ExecutionManager.ConnectionContext.IsOpen; + } + + public virtual void OpenConnection(SmoObjectBase smoObj) + { + SqlSmoObject sqlObj = smoObj as SqlSmoObject; + if (sqlObj != null + && sqlObj.ExecutionManager != null + && sqlObj.ExecutionManager.ConnectionContext != null) + { + sqlObj.ExecutionManager.ConnectionContext.Connect(); + } + } + } +} \ No newline at end of file diff --git a/test/Microsoft.SqlTools.ServiceLayer.UnitTests/ObjectExplorer/NodeTests.cs b/test/Microsoft.SqlTools.ServiceLayer.UnitTests/ObjectExplorer/NodeTests.cs index d971cd01..5f384a3b 100644 --- a/test/Microsoft.SqlTools.ServiceLayer.UnitTests/ObjectExplorer/NodeTests.cs +++ b/test/Microsoft.SqlTools.ServiceLayer.UnitTests/ObjectExplorer/NodeTests.cs @@ -276,6 +276,74 @@ namespace Microsoft.SqlTools.ServiceLayer.UnitTests.ObjectExplorer string.Format(CultureInfo.CurrentCulture, SR.TreeNodeError, expectedMsg), node.ErrorStateMessage); } + + [Fact] + public void QueryContextShouldNotCallOpenOnAlreadyOpenConnection() + { + // given a server connection that will state its connection is open + SetupAndRegisterTestConnectionService(); + Server smoServer = new Server(new ServerConnection(new SqlConnection(fakeConnectionString))); + Mock wrapper = SetupSmoWrapperForIsOpenTest(smoServer, isOpen: true); + + SmoQueryContext context = new SmoQueryContext(smoServer, ServiceProvider, wrapper.Object); + + // when I access the Server property + Server actualServer = context.Server; + + // Then I do not expect to have open called + Assert.NotNull(actualServer); + wrapper.Verify(c => c.OpenConnection(It.IsAny()), Times.Never); + } + + private Mock SetupSmoWrapperForIsOpenTest(Server smoServer, bool isOpen) + { + Mock wrapper = new Mock(); + int count = 0; + wrapper.Setup(c => c.CreateServer(It.IsAny())) + .Returns(() => smoServer); + wrapper.Setup(c => c.IsConnectionOpen(It.IsAny())) + .Returns(() => isOpen); + wrapper.Setup(c => c.OpenConnection(It.IsAny())) + .Callback(() => count++) + .Verifiable(); + return wrapper; + } + + [Fact] + public void QueryContextShouldReopenClosedConnectionWhenGettingServer() + { + // given a server connection that will state its connection is closed + SetupAndRegisterTestConnectionService(); + Server smoServer = new Server(new ServerConnection(new SqlConnection(fakeConnectionString))); + Mock wrapper = SetupSmoWrapperForIsOpenTest(smoServer, isOpen: false); + + SmoQueryContext context = new SmoQueryContext(smoServer, ServiceProvider, wrapper.Object); + + // when I access the Server property + Server actualServer = context.Server; + + // Then I expect to have open called + Assert.NotNull(actualServer); + wrapper.Verify(c => c.OpenConnection(It.IsAny()), Times.Once); + } + + [Fact] + public void QueryContextShouldReopenClosedConnectionWhenGettingParent() + { + // given a server connection that will state its connection is closed + SetupAndRegisterTestConnectionService(); + Server smoServer = new Server(new ServerConnection(new SqlConnection(fakeConnectionString))); + Mock wrapper = SetupSmoWrapperForIsOpenTest(smoServer, isOpen: false); + + SmoQueryContext context = new SmoQueryContext(smoServer, ServiceProvider, wrapper.Object); + context.Parent = smoServer; + // when I access the Parent property + SmoObjectBase actualParent = context.Parent; + + // Then I expect to have open called + Assert.NotNull(actualParent); + wrapper.Verify(c => c.OpenConnection(It.IsAny()), Times.Once); + } private ConnectionService SetupAndRegisterTestConnectionService() { @@ -291,27 +359,31 @@ namespace Microsoft.SqlTools.ServiceLayer.UnitTests.ObjectExplorer private ServerNode SetupServerNodeWithServer(Server smoServer) { - Mock creator = new Mock(); - creator.Setup(c => c.Create(It.IsAny())) + Mock creator = new Mock(); + creator.Setup(c => c.CreateServer(It.IsAny())) .Returns(() => smoServer); + creator.Setup(c => c.IsConnectionOpen(It.IsAny())) + .Returns(() => true); ServerNode node = SetupServerNodeWithCreator(creator.Object); return node; } private ServerNode SetupServerNodeWithExceptionCreator(Exception ex) { - Mock creator = new Mock(); - creator.Setup(c => c.Create(It.IsAny())) + Mock creator = new Mock(); + creator.Setup(c => c.CreateServer(It.IsAny())) .Throws(ex); + creator.Setup(c => c.IsConnectionOpen(It.IsAny())) + .Returns(() => false); ServerNode node = SetupServerNodeWithCreator(creator.Object); return node; } - private ServerNode SetupServerNodeWithCreator(SmoServerCreator creator) + private ServerNode SetupServerNodeWithCreator(SmoWrapper creator) { ServerNode node = new ServerNode(defaultConnParams, ServiceProvider); - node.ServerCreator = creator; + node.SmoWrapper = creator; return node; }