From 1be4daf41df44d1bf026bbad9a4c292637edfa31 Mon Sep 17 00:00:00 2001 From: Benjamin Russell Date: Thu, 11 Aug 2016 15:45:59 -0700 Subject: [PATCH 1/3] Adding enhanced support for messages from server * Adding error flag for Query class * Adding message capture for messages from server (using SqlConnection cast) * Adding better handling of SELECT queries with 0 results * Adding affected row count message * Adding SqlError unwrapping (using SqlException cast) * Removing DbException handling from QueryExecutionService and into Query class --- .../QueryExecution/Query.cs | 78 ++++++++++++++++++- .../QueryExecution/QueryExecutionService.cs | 33 +++----- 2 files changed, 83 insertions(+), 28 deletions(-) diff --git a/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Query.cs b/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Query.cs index eb093484..292b1e81 100644 --- a/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Query.cs +++ b/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Query.cs @@ -7,6 +7,7 @@ using System; using System.Collections.Generic; using System.Data; using System.Data.Common; +using System.Data.SqlClient; using System.Linq; using System.Threading; using System.Threading.Tasks; @@ -33,6 +34,11 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution /// public ConnectionInfo EditorConnection { get; set; } + public bool HasError { get; set; } + + /// + /// Whether or not the query has completed executed, regardless of success or failure + /// public bool HasExecuted { get; set; } /// @@ -40,6 +46,11 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution /// public string QueryText { get; set; } + /// + /// Messages that have come back from the server + /// + public List ResultMessages { get; set; } + /// /// The result sets of the query execution /// @@ -71,7 +82,7 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution public Query(string queryText, ConnectionInfo connection) { // Sanity check for input - if (String.IsNullOrWhiteSpace(queryText)) + if (String.IsNullOrEmpty(queryText)) { throw new ArgumentNullException(nameof(queryText), "Query text cannot be null"); } @@ -85,6 +96,7 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution EditorConnection = connection; HasExecuted = false; ResultSets = new List(); + ResultMessages = new List(); cancellationSource = new CancellationTokenSource(); } @@ -107,6 +119,14 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution string connectionString = ConnectionService.BuildConnectionString(EditorConnection.ConnectionDetails); using (conn = EditorConnection.Factory.CreateSqlConnection(connectionString)) { + // If we have the message listener, bind to it + // TODO: This doesn't allow testing via mocking + SqlConnection sqlConn = conn as SqlConnection; + if (sqlConn != null) + { + sqlConn.InfoMessage += StoreDbMessage; + } + await conn.OpenAsync(cancellationSource.Token); // Create a command that we'll use for executing the query @@ -120,8 +140,10 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution { do { - // TODO: This doesn't properly handle scenarios where the query is SELECT but does not have rows - if (!reader.HasRows) + // Create a message with the number of affected rows + ResultMessages.Add(String.Format("({0} row(s) affected)", reader.RecordsAffected)); + + if (!reader.HasRows && reader.FieldCount == 0) { continue; } @@ -146,9 +168,15 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution } } } + catch (DbException dbe) + { + HasError = true; + UnwrapDbException(dbe); + conn?.Dispose(); + } catch (Exception) { - // Dispose of the connection + HasError = true; conn?.Dispose(); throw; } @@ -200,6 +228,48 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution }; } + /// + /// Delegate handler for storing messages that are returned from the server + /// NOTE: Only messages that are below a certain severity will be returned via this + /// mechanism. Anything above that level will trigger an exception. + /// + /// Object that fired the event + /// Arguments from the event + private void StoreDbMessage(object sender, SqlInfoMessageEventArgs args) + { + ResultMessages.Add(args.Message); + } + + /// + /// Attempts to convert a to a that + /// contains much more info about Sql Server errors. The exception is then unwrapped and + /// messages are formatted and stored in . If the exception + /// cannot be converted to SqlException, the message is written to the messages list. + /// + /// The exception to unwrap + private void UnwrapDbException(DbException dbe) + { + SqlException se = dbe as SqlException; + if (se != null) + { + foreach (var error in se.Errors) + { + SqlError sqlError = error as SqlError; + if (sqlError != null) + { + string message = String.Format("Msg {0}, Level {1}, State {2}, Line {3}{4}{5}", + sqlError.Number, sqlError.Class, sqlError.State, sqlError.LineNumber, + Environment.NewLine, sqlError.Message); + ResultMessages.Add(message); + } + } + } + else + { + ResultMessages.Add(dbe.Message); + } + } + #region IDisposable Implementation private bool disposed; diff --git a/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/QueryExecutionService.cs b/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/QueryExecutionService.cs index 6480e4ba..0d5f2db7 100644 --- a/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/QueryExecutionService.cs +++ b/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/QueryExecutionService.cs @@ -242,31 +242,16 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution Messages = null }); - try + // Wait for query execution and then send back the results + await Task.WhenAll(executeTask); + QueryExecuteCompleteParams eventParams = new QueryExecuteCompleteParams { - // Wait for query execution and then send back the results - await Task.WhenAll(executeTask); - QueryExecuteCompleteParams eventParams = new QueryExecuteCompleteParams - { - HasError = false, - Messages = new string[] { }, // TODO: Figure out how to get messages back from the server - OwnerUri = executeParams.OwnerUri, - ResultSetSummaries = query.ResultSummary - }; - await requestContext.SendEvent(QueryExecuteCompleteEvent.Type, eventParams); - } - catch (DbException dbe) - { - // Dump the message to a complete event - QueryExecuteCompleteParams errorEvent = new QueryExecuteCompleteParams - { - HasError = true, - Messages = new[] {dbe.Message}, - OwnerUri = executeParams.OwnerUri, - ResultSetSummaries = query.ResultSummary - }; - await requestContext.SendEvent(QueryExecuteCompleteEvent.Type, errorEvent); - } + HasError = query.HasError, + Messages = query.ResultMessages.ToArray(), + OwnerUri = executeParams.OwnerUri, + ResultSetSummaries = query.ResultSummary + }; + await requestContext.SendEvent(QueryExecuteCompleteEvent.Type, eventParams); } #endregion From 9890e828bda8239bb67683466549d2b2dbd9c138 Mon Sep 17 00:00:00 2001 From: Benjamin Russell Date: Thu, 11 Aug 2016 16:39:33 -0700 Subject: [PATCH 2/3] Adding unit tests to the updated message mechanism --- .../QueryExecution/QueryExecutionService.cs | 1 - .../QueryExecution/Common.cs | 5 ++- .../QueryExecution/ExecuteTests.cs | 42 +++++++++++++------ 3 files changed, 34 insertions(+), 14 deletions(-) diff --git a/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/QueryExecutionService.cs b/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/QueryExecutionService.cs index 0d5f2db7..389be092 100644 --- a/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/QueryExecutionService.cs +++ b/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/QueryExecutionService.cs @@ -5,7 +5,6 @@ using System; using System.Collections.Concurrent; -using System.Data.Common; using System.Threading.Tasks; using Microsoft.SqlTools.ServiceLayer.Connection; using Microsoft.SqlTools.ServiceLayer.Hosting; diff --git a/test/Microsoft.SqlTools.ServiceLayer.Test/QueryExecution/Common.cs b/test/Microsoft.SqlTools.ServiceLayer.Test/QueryExecution/Common.cs index 9bc8053b..49b329a0 100644 --- a/test/Microsoft.SqlTools.ServiceLayer.Test/QueryExecution/Common.cs +++ b/test/Microsoft.SqlTools.ServiceLayer.Test/QueryExecution/Common.cs @@ -2,6 +2,7 @@ using System.Collections.Generic; using System.Data; using System.Data.Common; +using System.Data.SqlClient; using System.Threading.Tasks; using Microsoft.SqlTools.ServiceLayer.Connection; using Microsoft.SqlTools.ServiceLayer.Connection.Contracts; @@ -62,7 +63,9 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.QueryExecution // Setup the expected behavior if (throwOnRead) { - commandMockSetup.Throws(new Mock().Object); + var mockException = new Mock(); + mockException.SetupGet(dbe => dbe.Message).Returns("Message"); + commandMockSetup.Throws(mockException.Object); } else { diff --git a/test/Microsoft.SqlTools.ServiceLayer.Test/QueryExecution/ExecuteTests.cs b/test/Microsoft.SqlTools.ServiceLayer.Test/QueryExecution/ExecuteTests.cs index cddf1831..1cc56e53 100644 --- a/test/Microsoft.SqlTools.ServiceLayer.Test/QueryExecution/ExecuteTests.cs +++ b/test/Microsoft.SqlTools.ServiceLayer.Test/QueryExecution/ExecuteTests.cs @@ -36,8 +36,9 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.QueryExecution query.Execute().Wait(); // Then: - // ... It should have executed + // ... It should have executed without error Assert.True(query.HasExecuted, "The query should have been marked executed."); + Assert.False(query.HasError); // ... The results should be empty Assert.Empty(query.ResultSets); @@ -46,6 +47,9 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.QueryExecution // ... The results should not be null Assert.NotNull(query.ResultSets); Assert.NotNull(query.ResultSummary); + + // ... There should be a message for how many rows were affected + Assert.Equal(1, query.ResultMessages.Count); } [Fact] @@ -61,8 +65,9 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.QueryExecution query.Execute().Wait(); // Then: - // ... It should have executed + // ... It should have executed without error Assert.True(query.HasExecuted, "The query should have been marked executed."); + Assert.False(query.HasError); // ... There should be exactly one result set Assert.Equal(resultSets, query.ResultSets.Count); @@ -82,6 +87,9 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.QueryExecution // ... Inside the result summary, there should be 5 rows Assert.Equal(rows, query.ResultSummary[0].RowCount); + + // ... There should be a message for how many rows were affected + Assert.Equal(resultSets, query.ResultMessages.Count); } [Fact] @@ -98,8 +106,9 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.QueryExecution query.Execute().Wait(); // Then: - // ... It should have executed + // ... It should have executed without error Assert.True(query.HasExecuted, "The query should have been marked executed."); + Assert.False(query.HasError); // ... There should be exactly two result sets Assert.Equal(resultSets, query.ResultSets.Count); @@ -125,6 +134,9 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.QueryExecution // ... Inside each result summary, there should be 5 rows Assert.Equal(rows, rs.RowCount); } + + // ... There should be a message for how many rows were affected + Assert.Equal(resultSets, query.ResultMessages.Count); } [Fact] @@ -134,10 +146,15 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.QueryExecution // If I execute a query that is invalid Query query = new Query("Invalid query", ci); + query.Execute().Wait(); // Then: - // ... It should throw an exception - Exception e = Assert.Throws(() => query.Execute().Wait()); + // ... It should have executed with error + Assert.True(query.HasExecuted); + Assert.True(query.HasError); + + // ... There should be plenty of messages for the eror + Assert.NotEmpty(query.ResultMessages); } [Fact] @@ -150,8 +167,9 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.QueryExecution query.Execute().Wait(); // Then: - // ... It should have executed + // ... It should have executed without error Assert.True(query.HasExecuted, "The query should have been marked executed."); + Assert.False(query.HasError); // If I execute it again // Then: @@ -160,7 +178,8 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.QueryExecution Assert.Equal(1, ae.InnerExceptions.Count); Assert.IsType(ae.InnerExceptions[0]); - // ... The data should still be available + // ... The data should still be available without error + Assert.False(query.HasError); Assert.True(query.HasExecuted, "The query should still be marked executed."); Assert.NotEmpty(query.ResultSets); Assert.NotEmpty(query.ResultSummary); @@ -208,12 +227,12 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.QueryExecution // Then: // ... No Errors should have been sent - // ... A successful result should have been sent with no messages + // ... A successful result should have been sent with messages // ... A completion event should have been fired with empty results // ... There should be one active query VerifyQueryExecuteCallCount(requestContext, Times.Once(), Times.Once(), Times.Never()); Assert.Null(result.Messages); - Assert.Empty(completeParams.Messages); + Assert.NotEmpty(completeParams.Messages); Assert.Empty(completeParams.ResultSetSummaries); Assert.False(completeParams.HasError); Assert.Equal(1, queryService.ActiveQueries.Count); @@ -234,12 +253,12 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.QueryExecution // Then: // ... No errors should have been sent - // ... A successful result should have been sent with no messages + // ... A successful result should have been sent with messages // ... A completion event should have been fired with one result // ... There should be one active query VerifyQueryExecuteCallCount(requestContext, Times.Once(), Times.Once(), Times.Never()); Assert.Null(result.Messages); - Assert.Empty(completeParams.Messages); + Assert.NotEmpty(completeParams.Messages); Assert.NotEmpty(completeParams.ResultSetSummaries); Assert.False(completeParams.HasError); Assert.Equal(1, queryService.ActiveQueries.Count); @@ -327,7 +346,6 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.QueryExecution [Theory] [InlineData("")] - [InlineData(" ")] [InlineData(null)] public void QueryExecuteMissingQueryTest(string query) { From c596a0db7ac125b41303bc63a12419951bbb0695 Mon Sep 17 00:00:00 2001 From: Benjamin Russell Date: Thu, 11 Aug 2016 17:41:18 -0700 Subject: [PATCH 3/3] Small tweaks to query execution * Adding comments where missing * Adding "# rows affected" only if there was 0 or more --- .../QueryExecution/Query.cs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Query.cs b/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Query.cs index 292b1e81..887bbbaf 100644 --- a/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Query.cs +++ b/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Query.cs @@ -34,6 +34,9 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution /// public ConnectionInfo EditorConnection { get; set; } + /// + /// Whether or not the query has an error + /// public bool HasError { get; set; } /// @@ -120,7 +123,6 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution using (conn = EditorConnection.Factory.CreateSqlConnection(connectionString)) { // If we have the message listener, bind to it - // TODO: This doesn't allow testing via mocking SqlConnection sqlConn = conn as SqlConnection; if (sqlConn != null) { @@ -141,7 +143,10 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution do { // Create a message with the number of affected rows - ResultMessages.Add(String.Format("({0} row(s) affected)", reader.RecordsAffected)); + if (reader.RecordsAffected >= 0) + { + ResultMessages.Add(String.Format("({0} row(s) affected)", reader.RecordsAffected)); + } if (!reader.HasRows && reader.FieldCount == 0) {