Changes to fix code review comments and bug with handling empty batches

src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Batch.cs

@@ -38,15 +38,31 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution
         /// </summary>
         public bool HasExecuted { get; set; }
 
+        /// <summary>
+        /// Internal representation of the messages so we can modify internally
+        /// </summary>
+        private List<string> resultMessages;
+
         /// <summary>
         /// Messages that have come back from the server
         /// </summary>
-        public List<string> ResultMessages { get; set; }
+        public IEnumerable<string> ResultMessages
+        {
+            get { return resultMessages; }
+        }
+
+        /// <summary>
+        /// Internal representation of the result sets so we can modify internally
+        /// </summary>
+        private List<ResultSet> resultSets;
 
         /// <summary>
         /// The result sets of the batch execution
         /// </summary>
-        public List<ResultSet> ResultSets { get; set; }
+        public IEnumerable<ResultSet> ResultSets
+        {
+            get { return resultSets; }
+        }
 
         /// <summary>
         /// Property for generating a set result set summaries from the result sets
@@ -67,7 +83,7 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution
         /// <summary>
         /// The 0-indexed line number that this batch started on
         /// </summary>
-        private int StartLine { get; set; }
+        internal int StartLine { get; set; }
 
         #endregion
 
@@ -81,10 +97,10 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution
 
             // Initialize the internal state
             BatchText = batchText;
-            StartLine = startLine - 1;
+            StartLine = startLine - 1;  // -1 to make sure that the line number of the batch is 0-indexed, since SqlParser gives 1-indexed line numbers
             HasExecuted = false;
-            ResultSets = new List<ResultSet>();
-            ResultMessages = new List<string>();
+            resultSets = new List<ResultSet>();
+            resultMessages = new List<string>();
         }
 
         /// <summary>
@@ -125,7 +141,7 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution
                             if (!reader.HasRows && reader.FieldCount == 0)
                             {
                                 // Create a message with the number of affected rows -- IF the query affects rows
-                                ResultMessages.Add(reader.RecordsAffected >= 0
+                                resultMessages.Add(reader.RecordsAffected >= 0
                                     ? string.Format(RowsAffectedFormat, reader.RecordsAffected)
                                     : "Command(s) completed successfully.");
                                 continue;
@@ -145,10 +161,10 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution
                             }
 
                             // Add the result set to the results of the query
-                            ResultSets.Add(resultSet);
+                            resultSets.Add(resultSet);
 
                             // Add a message for the number of rows the query returned
-                            ResultMessages.Add(string.Format(RowsAffectedFormat, resultSet.Rows.Count));
+                            resultMessages.Add(string.Format(RowsAffectedFormat, resultSet.Rows.Count));
                         } while (await reader.NextResultAsync(cancellationToken));
                     }
                 }
@@ -187,14 +203,14 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution
         public ResultSetSubset GetSubset(int resultSetIndex, int startRow, int rowCount)
         {
             // Sanity check to make sure we have valid numbers
-            if (resultSetIndex < 0 || resultSetIndex >= ResultSets.Count)
+            if (resultSetIndex < 0 || resultSetIndex >= resultSets.Count)
             {
                 throw new ArgumentOutOfRangeException(nameof(resultSetIndex), "Result set index cannot be less than 0" +
                                                                              "or greater than the number of result sets");
             }
 
             // Retrieve the result set
-            return ResultSets[resultSetIndex].GetSubset(startRow, rowCount);
+            return resultSets[resultSetIndex].GetSubset(startRow, rowCount);
         }
 
         #region Private Helpers
@@ -208,7 +224,7 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution
         /// <param name="args">Arguments from the event</param>
         private void StoreDbMessage(object sender, SqlInfoMessageEventArgs args)
         {
-            ResultMessages.Add(args.Message);
+            resultMessages.Add(args.Message);
         }
 
         /// <summary>
@@ -232,13 +248,13 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution
                         string message = String.Format("Msg {0}, Level {1}, State {2}, Line {3}{4}{5}",
                             sqlError.Number, sqlError.Class, sqlError.State, lineNumber,
                             Environment.NewLine, sqlError.Message);
-                        ResultMessages.Add(message);
+                        resultMessages.Add(message);
                     }
                 }
             }
             else
             {
-                ResultMessages.Add(dbe.Message);
+                resultMessages.Add(dbe.Message);
             }
         }
 

src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Query.cs

@@ -60,6 +60,8 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution
         /// </summary>
         private ConnectionInfo EditorConnection { get; set; }
 
+        private bool HasExecuteBeenCalled { get; set; }
+
         /// <summary>
         /// Whether or not the query has completed executed, regardless of success or failure
         /// </summary>
@@ -68,9 +70,10 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution
         /// </remarks>
         public bool HasExecuted
         {
-            get { return Batches.All(b => b.HasExecuted); }
+            get { return Batches.Length == 0 ? HasExecuteBeenCalled : Batches.All(b => b.HasExecuted); }
             internal set
             {
+                HasExecuteBeenCalled = value;
                 foreach (var batch in Batches)
                 {
                     batch.HasExecuted = value;
@@ -94,7 +97,7 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution
         public Query(string queryText, ConnectionInfo connection, QueryExecutionSettings settings)
         {
             // Sanity check for input
-            if (String.IsNullOrEmpty(queryText))
+            if (string.IsNullOrEmpty(queryText))
             {
                 throw new ArgumentNullException(nameof(queryText), "Query text cannot be null");
             }
@@ -117,7 +120,9 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution
             {
                 BatchSeparator = settings.BatchSeparator
             });
-            Batches = parseResult.Script.Batches.Select(b => new Batch(b.Sql, b.StartLocation.LineNumber)).ToArray();
+            // NOTE: We only want to process batches that have statements (ie, ignore comments and empty lines)
+            Batches = parseResult.Script.Batches.Where(b => b.Statements.Count > 0)
+                .Select(b => new Batch(b.Sql, b.StartLocation.LineNumber)).ToArray();
         }
 
         /// <summary>
@@ -125,6 +130,15 @@ namespace Microsoft.SqlTools.ServiceLayer.QueryExecution
         /// </summary>
         public async Task Execute()
         {
+            // Mark that we've internally executed
+            HasExecuteBeenCalled = true;
+
+            // Don't actually execute if there aren't any batches to execute
+            if (Batches.Length == 0)
+            {
+                return;
+            }
+
             // Open up a connection for querying the database
             string connectionString = ConnectionService.BuildConnectionString(EditorConnection.ConnectionDetails);
             using (DbConnection conn = EditorConnection.Factory.CreateSqlConnection(connectionString))

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

@@ -26,6 +26,10 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.QueryExecution
     {
         public const string StandardQuery = "SELECT * FROM sys.objects";
 
+        public const string InvalidQuery = "SELECT *** FROM sys.objects";
+
+        public const string NoOpQuery = "-- No ops here, just us chickens.";
+
         public const string OwnerUri = "testFile";
 
         public const int StandardRows = 5;
@@ -55,7 +59,7 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.QueryExecution
 
         public static Batch GetBasicExecutedBatch()
         {
-            Batch batch = new Batch(StandardQuery);
+            Batch batch = new Batch(StandardQuery, 1);
             batch.Execute(CreateTestConnection(new[] {StandardTestData}, false), CancellationToken.None).Wait();
             return batch;
         }

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

@@ -28,7 +28,7 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.QueryExecution
         public void BatchCreationTest()
         {
             // If I create a new batch...
-            Batch batch = new Batch(Common.StandardQuery);
+            Batch batch = new Batch(Common.StandardQuery, 1);
 
             // Then: 
             // ... The text of the batch should be stored
@@ -42,13 +42,16 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.QueryExecution
             Assert.Empty(batch.ResultSets);
             Assert.Empty(batch.ResultSummaries);
             Assert.Empty(batch.ResultMessages);
+
+            // ... The start line of the batch should be 0
+            Assert.Equal(0, batch.StartLine);
         }
 
         [Fact]
         public void BatchExecuteNoResultSets()
         {
             // If I execute a query that should get no result sets
-            Batch batch = new Batch(Common.StandardQuery);
+            Batch batch = new Batch(Common.StandardQuery, 1);
             batch.Execute(GetConnection(Common.CreateTestConnectionInfo(null, false)), CancellationToken.None).Wait();
 
             // Then:
@@ -65,7 +68,7 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.QueryExecution
             Assert.NotNull(batch.ResultSummaries);
 
             // ... There should be a message for how many rows were affected
-            Assert.Equal(1, batch.ResultMessages.Count);
+            Assert.Equal(1, batch.ResultMessages.Count());
         }
 
         [Fact]
@@ -75,7 +78,7 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.QueryExecution
             ConnectionInfo ci = Common.CreateTestConnectionInfo(new[] { Common.StandardTestData }, false);
 
             // If I execute a query that should get one result set
-            Batch batch = new Batch(Common.StandardQuery);
+            Batch batch = new Batch(Common.StandardQuery, 1);
             batch.Execute(GetConnection(ci), CancellationToken.None).Wait();
 
             // Then:
@@ -84,20 +87,20 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.QueryExecution
             Assert.False(batch.HasError, "The batch should not have an error");
 
             // ... There should be exactly one result set
-            Assert.Equal(resultSets, batch.ResultSets.Count);
+            Assert.Equal(resultSets, batch.ResultSets.Count());
             Assert.Equal(resultSets, batch.ResultSummaries.Length);
 
             // ... Inside the result set should be with 5 rows
-            Assert.Equal(Common.StandardRows, batch.ResultSets[0].Rows.Count);
+            Assert.Equal(Common.StandardRows, batch.ResultSets.First().Rows.Count);
             Assert.Equal(Common.StandardRows, batch.ResultSummaries[0].RowCount);
 
             // ... Inside the result set should have 5 columns and 5 column definitions
-            Assert.Equal(Common.StandardColumns, batch.ResultSets[0].Rows[0].Length);
-            Assert.Equal(Common.StandardColumns, batch.ResultSets[0].Columns.Length);
+            Assert.Equal(Common.StandardColumns, batch.ResultSets.First().Rows[0].Length);
+            Assert.Equal(Common.StandardColumns, batch.ResultSets.First().Columns.Length);
             Assert.Equal(Common.StandardColumns, batch.ResultSummaries[0].ColumnInfo.Length);
 
             // ... There should be a message for how many rows were affected
-            Assert.Equal(resultSets, batch.ResultMessages.Count);
+            Assert.Equal(resultSets, batch.ResultMessages.Count());
         }
 
         [Fact]
@@ -108,7 +111,7 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.QueryExecution
             ConnectionInfo ci = Common.CreateTestConnectionInfo(dataset, false);
 
             // If I execute a query that should get two result sets
-            Batch batch = new Batch(Common.StandardQuery);
+            Batch batch = new Batch(Common.StandardQuery, 1);
             batch.Execute(GetConnection(ci), CancellationToken.None).Wait();
 
             // Then:
@@ -117,7 +120,7 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.QueryExecution
             Assert.False(batch.HasError, "The batch should not have an error");
 
             // ... There should be exactly two result sets
-            Assert.Equal(resultSets, batch.ResultSets.Count);
+            Assert.Equal(resultSets, batch.ResultSets.Count());
 
             foreach (ResultSet rs in batch.ResultSets)
             {
@@ -142,7 +145,7 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.QueryExecution
             }
 
             // ... There should be a message for how many rows were affected
-            Assert.Equal(resultSets, batch.ResultMessages.Count);
+            Assert.Equal(resultSets, batch.ResultMessages.Count());
         }
 
         [Fact]
@@ -151,7 +154,7 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.QueryExecution
             ConnectionInfo ci = Common.CreateTestConnectionInfo(null, true);
 
             // If I execute a batch that is invalid
-            Batch batch = new Batch(Common.StandardQuery);
+            Batch batch = new Batch(Common.StandardQuery, 1);
             batch.Execute(GetConnection(ci), CancellationToken.None).Wait();
 
             // Then:
@@ -173,7 +176,7 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.QueryExecution
             ConnectionInfo ci = Common.CreateTestConnectionInfo(new[] { Common.StandardTestData }, false);
 
             // If I execute a batch
-            Batch batch = new Batch(Common.StandardQuery);
+            Batch batch = new Batch(Common.StandardQuery, 1);
             batch.Execute(GetConnection(ci), CancellationToken.None).Wait();
 
             // Then:
@@ -203,7 +206,7 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.QueryExecution
             // ... I create a batch that has an empty query
             // Then:
             // ... It should throw an exception
-            Assert.Throws<ArgumentNullException>(() => new Batch(query));
+            Assert.Throws<ArgumentNullException>(() => new Batch(query, 1));
         }
 
         #endregion
@@ -269,6 +272,31 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.QueryExecution
             Assert.Equal(1, query.BatchSummaries.Length);
         }
 
+        [Fact]
+        public void QueryExecuteNoOpBatch()
+        {
+            // If:
+            // ... I create a query from a single batch that does nothing
+            ConnectionInfo ci = Common.CreateTestConnectionInfo(null, false);
+            Query query = new Query(Common.NoOpQuery, ci, new QueryExecutionSettings());
+
+            // Then:
+            // ... I should get no batches back
+            Assert.NotEmpty(query.QueryText);
+            Assert.Empty(query.Batches);
+            Assert.False(query.HasExecuted);
+            Assert.Throws<InvalidOperationException>(() => query.BatchSummaries);
+
+            // If:
+            // ... I Then execute the query
+            query.Execute().Wait();
+
+            // Then:
+            // ... The query should have completed successfully with no batch summaries returned
+            Assert.True(query.HasExecuted);
+            Assert.Empty(query.BatchSummaries);
+        }
+
         [Fact]
         public void QueryExecuteMultipleBatches()
         {
@@ -297,13 +325,40 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.QueryExecution
             Assert.Equal(2, query.BatchSummaries.Length);
         }
 
+        [Fact]
+        public void QueryExecuteMultipleBatchesWithNoOp()
+        {
+            // If:
+            // ... I create a query from a two batches (with separator)
+            ConnectionInfo ci = Common.CreateTestConnectionInfo(null, false);
+            string queryText = string.Format("{0}\r\nGO\r\n{1}", Common.StandardQuery, Common.NoOpQuery);
+            Query query = new Query(queryText, ci, new QueryExecutionSettings());
+
+            // Then:
+            // ... I should get back one batch to execute that hasn't been executed
+            Assert.NotEmpty(query.QueryText);
+            Assert.NotEmpty(query.Batches);
+            Assert.Equal(1, query.Batches.Length);
+            Assert.False(query.HasExecuted);
+            Assert.Throws<InvalidOperationException>(() => query.BatchSummaries);
+
+            // If:
+            // .. I then execute the query
+            query.Execute().Wait();
+
+            // ... The query should have completed successfully with one batch summary returned
+            Assert.True(query.HasExecuted);
+            Assert.NotEmpty(query.BatchSummaries);
+            Assert.Equal(1, query.BatchSummaries.Length);
+        }
+
         [Fact]
         public void QueryExecuteInvalidBatch()
         {
             // If:
             // ... I create a query from an invalid batch
             ConnectionInfo ci = Common.CreateTestConnectionInfo(null, true);
-            Query query = new Query("SELECT *** FROM sys.objects", ci, new QueryExecutionSettings());
+            Query query = new Query(Common.InvalidQuery, ci, new QueryExecutionSettings());
 
             // Then:
             // ... I should get back a query with one batch not executed

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

@@ -143,7 +143,7 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.QueryExecution
             var executeParams = new QueryExecuteParams { QueryText = "Doesn'tMatter", OwnerUri = Common.OwnerUri };
             var executeRequest = Common.GetQueryExecuteResultContextMock(null, null, null);
             queryService.HandleExecuteRequest(executeParams, executeRequest.Object).Wait();
-            //queryService.ActiveQueries[Common.OwnerUri].HasExecuted = false;
+            queryService.ActiveQueries[Common.OwnerUri].HasExecuted = false;
 
             // ... And I then ask for a valid set of results from it
             var subsetParams = new QueryExecuteSubsetParams { OwnerUri = Common.OwnerUri, RowsCount = 1, ResultSetIndex = 0, RowsStartIndex = 0 };