Fixing bug in GetOrOpenConnection (#234)

Fixes a bug where the GetOrOpenConnection method of the ConnectionService would throw if the connection didn't exist. It would yield a very unhelpful "key not found in collection" exception. The code has been updated to be much more helpful and fix the bug.

src/Microsoft.SqlTools.ServiceLayer/Connection/ConnectionService.cs

@@ -388,25 +388,24 @@ namespace Microsoft.SqlTools.ServiceLayer.Connection
         /// creates a new connection. This cannot be used to create a default connection or to create a 
         /// connection if a default connection does not exist.
         /// </summary>
+        /// <returns>A DB connection for the connection type requested</returns>
         public async Task<DbConnection> GetOrOpenConnection(string ownerUri, string connectionType)
         {
-            if (string.IsNullOrEmpty(ownerUri) || string.IsNullOrEmpty(connectionType))
+            Validate.IsNotNullOrEmptyString(nameof(ownerUri), ownerUri);
-            {
+            Validate.IsNotNullOrEmptyString(nameof(connectionType), connectionType);
-                return null;
-            }
 
             // Try to get the ConnectionInfo, if it exists
-            ConnectionInfo connectionInfo = ownerToConnectionMap[ownerUri];
+            ConnectionInfo connectionInfo;
-            if (connectionInfo == null)
+            if (!ownerToConnectionMap.TryGetValue(ownerUri, out connectionInfo))
             {
-                return null;
+                throw new ArgumentOutOfRangeException(SR.ConnectionServiceListDbErrorNotConnected(ownerUri));
             }
 
             // Make sure a default connection exists
             DbConnection defaultConnection;
             if (!connectionInfo.TryGetConnection(ConnectionType.Default, out defaultConnection))
             {
-                return null;
+                throw new InvalidOperationException(SR.ConnectionServiceDbErrorDefaultNotConnected(ownerUri));
             }
 
             // Try to get the DbConnection
@@ -416,7 +415,7 @@ namespace Microsoft.SqlTools.ServiceLayer.Connection
                 // If the DbConnection does not exist and is not the default connection, create one.
                 // We can't create the default (initial) connection here because we won't have a ConnectionDetails 
                 // if Connect() has not yet been called.
-                ConnectParams connectParams = new ConnectParams()
+                ConnectParams connectParams = new ConnectParams
                 {
                     OwnerUri = ownerUri,
                     Connection = connectionInfo.ConnectionDetails,

src/Microsoft.SqlTools.ServiceLayer/Localization/sr.cs

@@ -730,6 +730,11 @@ namespace Microsoft.SqlTools.ServiceLayer
             return Keys.GetString(Keys.ConnectionServiceListDbErrorNotConnected, uri);
         }
 
+        public static string ConnectionServiceDbErrorDefaultNotConnected(string uri)
+        {
+            return Keys.GetString(Keys.ConnectionServiceDbErrorDefaultNotConnected, uri);
+        }
+
         public static string ConnectionServiceConnStringInvalidAuthType(string authType)
         {
             return Keys.GetString(Keys.ConnectionServiceConnStringInvalidAuthType, authType);
@@ -802,6 +807,9 @@ namespace Microsoft.SqlTools.ServiceLayer
             public const string ConnectionServiceListDbErrorNotConnected = "ConnectionServiceListDbErrorNotConnected";
 
 
+            public const string ConnectionServiceDbErrorDefaultNotConnected = "ConnectionServiceDbErrorDefaultNotConnected";
+
+
             public const string ConnectionServiceConnStringInvalidAuthType = "ConnectionServiceConnStringInvalidAuthType";
 
 

src/Microsoft.SqlTools.ServiceLayer/Localization/sr.resx

@@ -128,6 +128,11 @@
   <data name="ConnectionServiceListDbErrorNotConnected" xml:space="preserve">
     <value>SpecifiedUri '{0}' does not have existing connection</value>
     <comment>.
+ Parameters: 0 - uri (string) </comment>
+  </data>
+  <data name="ConnectionServiceDbErrorDefaultNotConnected" xml:space="preserve">
+    <value>Specified URI '{0}' does not have a default connection</value>
+    <comment>.
  Parameters: 0 - uri (string) </comment>
   </data>
   <data name="ConnectionServiceConnStringInvalidAuthType" xml:space="preserve">

src/Microsoft.SqlTools.ServiceLayer/Localization/sr.strings

@@ -29,6 +29,8 @@ ConnectionServiceListDbErrorNullOwnerUri = OwnerUri cannot be null or empty
 
 ConnectionServiceListDbErrorNotConnected(string uri) = SpecifiedUri '{0}' does not have existing connection
 
+ConnectionServiceDbErrorDefaultNotConnected(string uri) = Specified URI '{0}' does not have a default connection
+
 ConnectionServiceConnStringInvalidAuthType(string authType) = Invalid value '{0}' for AuthenticationType.  Valid values are 'Integrated' and 'SqlLogin'.
 
 ConnectionServiceConnStringInvalidIntent(string intent) = Invalid value '{0}' for ApplicationIntent. Valid values are 'ReadWrite' and 'ReadOnly'.

test/Microsoft.SqlTools.ServiceLayer.Test/Connection/ConnectionServiceTests.cs

@@ -1139,5 +1139,53 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.Connection
             await service.Connect(connectParamsQuery);
             Assert.Equal(2, connectionInfo.CountConnections);
         }
+
+        [Theory]
+        [InlineData(null)]
+        [InlineData("")]
+        public async Task GetOrOpenNullOwnerUri(string ownerUri)
+        {
+            // If: I have a connection service and I ask for a connection with an invalid ownerUri
+            // Then: An exception should be thrown
+            var service = TestObjects.GetTestConnectionService();
+            await Assert.ThrowsAsync<ArgumentException>(
+                () => service.GetOrOpenConnection(ownerUri, ConnectionType.Default));
+        }
+
+        [Theory]
+        [InlineData(null)]
+        [InlineData("")]
+        public async Task GetOrOpenNullConnectionType(string connType)
+        {
+            // If: I have a connection service and I ask for a connection with an invalid connectionType
+            // Then: An exception should be thrown
+            var service = TestObjects.GetTestConnectionService();
+            await Assert.ThrowsAsync<ArgumentException>(
+                () => service.GetOrOpenConnection(TestObjects.ScriptUri, connType));
+        }
+
+        [Fact]
+        public async Task GetOrOpenNoConnection()
+        {
+            // If: I have a connection service and I ask for a connection for an unconnected uri
+            // Then: An exception should be thrown
+            var service = TestObjects.GetTestConnectionService();
+            await Assert.ThrowsAsync<ArgumentOutOfRangeException>(
+                () => service.GetOrOpenConnection(TestObjects.ScriptUri, ConnectionType.Query));
+        }
+
+        [Fact]
+        public async Task GetOrOpenNoDefaultConnection()
+        {
+            // Setup: Create a connection service with an empty connection info obj
+            var service = TestObjects.GetTestConnectionService();
+            var connInfo = new ConnectionInfo(null, null, null);
+            service.OwnerToConnectionMap[TestObjects.ScriptUri] = connInfo;
+
+            // If: I ask for a connection on a connection that doesn't have a default connection
+            // Then: An exception should be thrown
+            await Assert.ThrowsAsync<InvalidOperationException>(
+                () => service.GetOrOpenConnection(TestObjects.ScriptUri, ConnectionType.Query));
+        }
     }
 }

test/Microsoft.SqlTools.ServiceLayer.Test/QueryExecution/Common.cs

@@ -206,7 +206,11 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.QueryExecution
 
         public static ConnectionInfo CreateTestConnectionInfo(TestResultSet[] data, bool throwOnRead)
         {
-            return new ConnectionInfo(CreateMockFactory(data, throwOnRead), OwnerUri, StandardConnectionDetails);
+            // Create a connection info and add the default connection to it
+            ISqlConnectionFactory factory = CreateMockFactory(data, throwOnRead);
+            ConnectionInfo ci = new ConnectionInfo(factory, OwnerUri, StandardConnectionDetails);
+            ci.ConnectionTypeToConnectionMap[ConnectionType.Default] = factory.CreateSqlConnection(null);
+            return ci;
         }
 
         public static ConnectionInfo CreateConnectedConnectionInfo(TestResultSet[] data, bool throwOnRead, string type = ConnectionType.Default)