From 68c25f506e505e8cffb36814eec29ec7185731ee Mon Sep 17 00:00:00 2001 From: Benjamin Russell Date: Wed, 10 Aug 2016 16:40:36 -0700 Subject: [PATCH] Adding changes as requested for code review --- .../Contracts/QueryDisposeRequest.cs | 1 - .../QueryExecuteCompleteNotification.cs | 12 ++- .../Contracts/QueryExecuteRequest.cs | 1 - .../Contracts/QueryExecuteSubsetRequest.cs | 8 +- .../Contracts/ResultSetSubset.cs | 19 +++- .../Contracts/ResultSetSummary.cs | 10 +- .../QueryExecution/Query.cs | 96 +++++++++++++++++-- .../QueryExecution/QueryExecutionService.cs | 79 ++++++++++++--- .../QueryExecution/ResultSet.cs | 6 +- .../QueryExecution/ExecuteTests.cs | 8 +- 10 files changed, 203 insertions(+), 37 deletions(-) diff --git a/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Contracts/QueryDisposeRequest.cs b/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Contracts/QueryDisposeRequest.cs index 51e1b5dd..70e6631c 100644 --- a/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Contracts/QueryDisposeRequest.cs +++ b/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Contracts/QueryDisposeRequest.cs @@ -3,7 +3,6 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. // -using System; using Microsoft.SqlTools.ServiceLayer.Hosting.Protocol.Contracts; namespace Microsoft.SqlTools.ServiceLayer.QueryExecution.Contracts diff --git a/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Contracts/QueryExecuteCompleteNotification.cs b/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Contracts/QueryExecuteCompleteNotification.cs index b5c69941..f81edb62 100644 --- a/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Contracts/QueryExecuteCompleteNotification.cs +++ b/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Contracts/QueryExecuteCompleteNotification.cs @@ -1,7 +1,15 @@ -using Microsoft.SqlTools.ServiceLayer.Hosting.Protocol.Contracts; +// +// Copyright (c) Microsoft. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. +// + +using Microsoft.SqlTools.ServiceLayer.Hosting.Protocol.Contracts; namespace Microsoft.SqlTools.ServiceLayer.QueryExecution.Contracts { + /// + /// Parameters to be sent back with a query execution complete event + /// public class QueryExecuteCompleteParams { /// @@ -17,7 +25,7 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution.Contracts /// /// Whether or not the query was successful. True indicates errors, false indicates success /// - public bool Error { get; set; } + public bool HasError { get; set; } /// /// Summaries of the result sets that were returned with the query diff --git a/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Contracts/QueryExecuteRequest.cs b/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Contracts/QueryExecuteRequest.cs index 59453fb9..cac98c1a 100644 --- a/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Contracts/QueryExecuteRequest.cs +++ b/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Contracts/QueryExecuteRequest.cs @@ -3,7 +3,6 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. // -using System; using Microsoft.SqlTools.ServiceLayer.Hosting.Protocol.Contracts; namespace Microsoft.SqlTools.ServiceLayer.QueryExecution.Contracts diff --git a/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Contracts/QueryExecuteSubsetRequest.cs b/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Contracts/QueryExecuteSubsetRequest.cs index 8a7b3587..cdf434bb 100644 --- a/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Contracts/QueryExecuteSubsetRequest.cs +++ b/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Contracts/QueryExecuteSubsetRequest.cs @@ -3,7 +3,6 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. // -using System; using Microsoft.SqlTools.ServiceLayer.Hosting.Protocol.Contracts; namespace Microsoft.SqlTools.ServiceLayer.QueryExecution.Contracts @@ -42,7 +41,14 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution.Contracts /// public class QueryExecuteSubsetResult { + /// + /// Subset request error messages. Optional, can be set to null to indicate no errors + /// public string Message { get; set; } + + /// + /// The requested subset of results. Optional, can be set to null to indicate an error + /// public ResultSetSubset ResultSubset { get; set; } } diff --git a/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Contracts/ResultSetSubset.cs b/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Contracts/ResultSetSubset.cs index a9256581..8e2b49a9 100644 --- a/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Contracts/ResultSetSubset.cs +++ b/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Contracts/ResultSetSubset.cs @@ -1,13 +1,24 @@ -using System; -using System.Collections.Generic; -using System.Linq; -using System.Threading.Tasks; +// +// Copyright (c) Microsoft. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. +// namespace Microsoft.SqlTools.ServiceLayer.QueryExecution.Contracts { + /// + /// Class used to represent a subset of results from a query for transmission across JSON RPC + /// public class ResultSetSubset { + /// + /// The number of rows returned from result set, useful for determining if less rows were + /// returned than requested. + /// public int RowCount { get; set; } + + /// + /// 2D array of the cell values requested from result set + /// public object[][] Rows { get; set; } } } diff --git a/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Contracts/ResultSetSummary.cs b/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Contracts/ResultSetSummary.cs index 416aafb8..5f8de12a 100644 --- a/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Contracts/ResultSetSummary.cs +++ b/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Contracts/ResultSetSummary.cs @@ -1,7 +1,15 @@ -using System.Data.Common; +// +// Copyright (c) Microsoft. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. +// + +using System.Data.Common; namespace Microsoft.SqlTools.ServiceLayer.QueryExecution.Contracts { + /// + /// Represents a summary of information about a result without returning any cells of the results + /// public class ResultSetSummary { /// diff --git a/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Query.cs b/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Query.cs index eeaa8903..434188a5 100644 --- a/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Query.cs +++ b/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Query.cs @@ -1,4 +1,9 @@ -using System; +// +// 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.Collections.Generic; using System.Data; using System.Data.Common; @@ -10,18 +15,39 @@ using Microsoft.SqlTools.ServiceLayer.QueryExecution.Contracts; namespace Microsoft.SqlTools.ServiceLayer.QueryExecution { - public class Query //: IDisposable + /// + /// Internal representation of an active query + /// + public class Query : IDisposable { #region Properties - public string QueryText { get; set; } - - public ConnectionInfo EditorConnection { get; set; } - + /// + /// Cancellation token source, used for cancelling async db actions + /// private readonly CancellationTokenSource cancellationSource; + /// + /// The connection info associated with the file editor owner URI, used to create a new + /// connection upon execution of the query + /// + public ConnectionInfo EditorConnection { get; set; } + + public bool HasExecuted { get; set; } + + /// + /// The text of the query to execute + /// + public string QueryText { get; set; } + + /// + /// The result sets of the query execution + /// public List ResultSets { get; set; } + /// + /// Property for generating a set result set summaries from the result sets + /// public ResultSetSummary[] ResultSummary { get @@ -35,10 +61,13 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution } } - public bool HasExecuted { get; set; } - #endregion + /// + /// Constructor for a query + /// + /// The text of the query to execute + /// The information of the connection to use to execute the query public Query(string queryText, ConnectionInfo connection) { // Sanity check for input @@ -59,6 +88,9 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution cancellationSource = new CancellationTokenSource(); } + /// + /// Executes this query asynchronously and collects all result sets + /// public async Task Execute() { // Sanity check to make sure we haven't already run this query @@ -67,11 +99,13 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution throw new InvalidOperationException("Query has already executed."); } + DbConnection conn = null; + // Create a connection from the connection details try { string connectionString = ConnectionService.BuildConnectionString(EditorConnection.ConnectionDetails); - using (DbConnection conn = EditorConnection.Factory.CreateSqlConnection(connectionString)) + using (EditorConnection.Factory.CreateSqlConnection(connectionString)) { await conn.OpenAsync(cancellationSource.Token); @@ -112,6 +146,11 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution } } } + catch (Exception) + { + // Dispose of the connection + conn?.Dispose(); + } finally { // Mark that we have executed @@ -119,6 +158,13 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution } } + /// + /// Retrieves a subset of the result sets + /// + /// The index for selecting the result set + /// The starting row of the results + /// How many rows to retrieve + /// A subset of results public ResultSetSubset GetSubset(int resultSetIndex, int startRow, int rowCount) { // Sanity check that the results are available @@ -152,5 +198,37 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution RowCount = rows.Length }; } + + #region IDisposable Implementation + + private bool disposed; + + public void Dispose() + { + Dispose(true); + GC.SuppressFinalize(this); + } + + protected virtual void Dispose(bool disposing) + { + if (disposed) + { + return; + } + + if (disposing) + { + cancellationSource.Dispose(); + } + + disposed = true; + } + + ~Query() + { + Dispose(false); + } + + #endregion } } diff --git a/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/QueryExecutionService.cs b/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/QueryExecutionService.cs index b98b5ac8..6480e4ba 100644 --- a/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/QueryExecutionService.cs +++ b/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/QueryExecutionService.cs @@ -14,7 +14,10 @@ using Microsoft.SqlTools.ServiceLayer.QueryExecution.Contracts; namespace Microsoft.SqlTools.ServiceLayer.QueryExecution { - public sealed class QueryExecutionService + /// + /// Service for executing queries + /// + public sealed class QueryExecutionService : IDisposable { #region Singleton Instance Implementation @@ -39,24 +42,32 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution #region Properties - private readonly Lazy> queries = - new Lazy>(() => new ConcurrentDictionary()); - + /// + /// The collection of active queries + /// internal ConcurrentDictionary ActiveQueries { get { return queries.Value; } } + /// + /// Instance of the connection service, used to get the connection info for a given owner URI + /// private ConnectionService ConnectionService { get; set; } + /// + /// Internal storage of active queries, lazily constructed as a threadsafe dictionary + /// + private readonly Lazy> queries = + new Lazy>(() => new ConcurrentDictionary()); + #endregion - #region Public Methods - /// - /// + /// Initializes the service with the service host, registers request handlers and shutdown + /// event handler. /// - /// + /// The service host instance to register with public void InitializeService(ServiceHost serviceHost) { // Register handlers for requests @@ -64,11 +75,14 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution serviceHost.SetRequestHandler(QueryExecuteSubsetRequest.Type, HandleResultSubsetRequest); serviceHost.SetRequestHandler(QueryDisposeRequest.Type, HandleDisposeRequest); - // Register handlers for events + // Register handler for shutdown event + serviceHost.RegisterShutdownTask((shutdownParams, requestContext) => + { + Dispose(); + return Task.FromResult(0); + }); } - #endregion - #region Request Handlers public async Task HandleExecuteRequest(QueryExecuteParams executeParams, @@ -167,6 +181,8 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution #endregion + #region Private Helpers + private async Task CreateAndActivateNewQuery(QueryExecuteParams executeParams, RequestContext requestContext) { try @@ -232,7 +248,7 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution await Task.WhenAll(executeTask); QueryExecuteCompleteParams eventParams = new QueryExecuteCompleteParams { - Error = false, + HasError = false, Messages = new string[] { }, // TODO: Figure out how to get messages back from the server OwnerUri = executeParams.OwnerUri, ResultSetSummaries = query.ResultSummary @@ -244,7 +260,7 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution // Dump the message to a complete event QueryExecuteCompleteParams errorEvent = new QueryExecuteCompleteParams { - Error = true, + HasError = true, Messages = new[] {dbe.Message}, OwnerUri = executeParams.OwnerUri, ResultSetSummaries = query.ResultSummary @@ -252,5 +268,42 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution await requestContext.SendEvent(QueryExecuteCompleteEvent.Type, errorEvent); } } + + #endregion + + #region IDisposable Implementation + + private bool disposed; + + public void Dispose() + { + Dispose(true); + GC.SuppressFinalize(this); + } + + private void Dispose(bool disposing) + { + if (disposed) + { + return; + } + + if (disposing) + { + foreach (var query in ActiveQueries) + { + query.Value.Dispose(); + } + } + + disposed = true; + } + + ~QueryExecutionService() + { + Dispose(false); + } + + #endregion } } diff --git a/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/ResultSet.cs b/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/ResultSet.cs index ee7d7852..fed08ea3 100644 --- a/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/ResultSet.cs +++ b/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/ResultSet.cs @@ -1,4 +1,8 @@ -using System; +// +// Copyright (c) Microsoft. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. +// + using System.Collections.Generic; using System.Data.Common; diff --git a/test/Microsoft.SqlTools.ServiceLayer.Test/QueryExecution/ExecuteTests.cs b/test/Microsoft.SqlTools.ServiceLayer.Test/QueryExecution/ExecuteTests.cs index ffc0a870..cddf1831 100644 --- a/test/Microsoft.SqlTools.ServiceLayer.Test/QueryExecution/ExecuteTests.cs +++ b/test/Microsoft.SqlTools.ServiceLayer.Test/QueryExecution/ExecuteTests.cs @@ -215,7 +215,7 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.QueryExecution Assert.Null(result.Messages); Assert.Empty(completeParams.Messages); Assert.Empty(completeParams.ResultSetSummaries); - Assert.False(completeParams.Error); + Assert.False(completeParams.HasError); Assert.Equal(1, queryService.ActiveQueries.Count); } @@ -241,7 +241,7 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.QueryExecution Assert.Null(result.Messages); Assert.Empty(completeParams.Messages); Assert.NotEmpty(completeParams.ResultSetSummaries); - Assert.False(completeParams.Error); + Assert.False(completeParams.HasError); Assert.Equal(1, queryService.ActiveQueries.Count); } @@ -321,7 +321,7 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.QueryExecution // ... There should only be one active query VerifyQueryExecuteCallCount(secondRequestContext, Times.Once(), Times.Once(), Times.Never()); Assert.Null(result.Messages); - Assert.False(complete.Error); + Assert.False(complete.HasError); Assert.Equal(1, queryService.ActiveQueries.Count); } @@ -368,7 +368,7 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.QueryExecution // ... A completion event should have been sent with error VerifyQueryExecuteCallCount(requestContext, Times.Once(), Times.Once(), Times.Never()); Assert.Null(result.Messages); - Assert.True(complete.Error); + Assert.True(complete.HasError); Assert.NotEmpty(complete.Messages); }