Skip to content

Commit 41f2a4a

Browse files
rwestMSFTrwestMSFT
committed
Address feedback
1 parent d4c3a1e commit 41f2a4a

2 files changed

Lines changed: 76 additions & 37 deletions

File tree

‎SqlScriptDom/ScriptDom/SqlServer/ScriptGenerator/SqlScriptGeneratorVisitor.AlterTableAddTableElementStatement.cs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,8 @@ public override void ExplicitVisit(AlterTableAddTableElementStatement node)
6161
ExplicitVisit((SystemTimePeriodDefinition)node.Definition.SystemTimePeriod);
6262
}
6363

64+
// Separate preceding elements from inline indexes with a comma
65+
// and newline to avoid token concatenation (e.g. 'NOT NULLINDEX').
6466
if ((node.Definition.ColumnDefinitions.Count > 0
6567
|| node.Definition.TableConstraints.Count > 0
6668
|| node.Definition.SystemTimePeriod != null)

‎Test/SqlDom/ScriptGeneratorTests.cs‎

Lines changed: 74 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
//------------------------------------------------------------------------------
66

77
using System;
8+
using System.Collections.Generic;
89
using System.IO;
910
using Microsoft.SqlServer.TransactSql.ScriptDom;
1011
using Microsoft.VisualStudio.TestTools.UnitTesting;
@@ -348,14 +349,19 @@ public void TestAlterTableAddColumnThenIndex_EmitsSeparatorBeforeIndex()
348349
// ('NOT NULL') and the inline INDEX, producing 'NOT NULLINDEX
349350
// ix_Customer' which fails to reparse.
350351
var sql =
351-
"ALTER TABLE Sales.SalesOrderDetail_inmem \n" +
352-
" ADD CustomerID int NOT NULL DEFAULT -1 WITH VALUES, \n" +
353-
" ShipMethodID int NOT NULL DEFAULT -1 WITH VALUES, \n" +
354-
" INDEX ix_Customer (CustomerID); \n" +
355-
"GO \n";
352+
"ALTER TABLE Sales.SalesOrderDetail_inmem " + Environment.NewLine +
353+
" ADD CustomerID int NOT NULL DEFAULT -1 WITH VALUES, " + Environment.NewLine +
354+
" ShipMethodID int NOT NULL DEFAULT -1 WITH VALUES, " + Environment.NewLine +
355+
" INDEX ix_Customer (CustomerID); " + Environment.NewLine +
356+
"GO " + Environment.NewLine;
356357

357358
var parser = new TSql170Parser(true);
358-
var fragment = parser.Parse(new StringReader(sql), out var parseErrors);
359+
TSqlFragment fragment;
360+
IList<ParseError> parseErrors;
361+
using (var reader = new StringReader(sql))
362+
{
363+
fragment = parser.Parse(reader, out parseErrors);
364+
}
359365
Assert.AreEqual(0, parseErrors.Count, "Input must parse.");
360366

361367
var generator = new Sql170ScriptGenerator(new SqlScriptGeneratorOptions
@@ -370,7 +376,11 @@ public void TestAlterTableAddColumnThenIndex_EmitsSeparatorBeforeIndex()
370376
"INDEX keyword must be present and separated. Actual:\n" + generated);
371377

372378
var reparser = new TSql170Parser(true);
373-
reparser.Parse(new StringReader(generated), out var reparseErrors);
379+
IList<ParseError> reparseErrors;
380+
using (var reader = new StringReader(generated))
381+
{
382+
reparser.Parse(reader, out reparseErrors);
383+
}
374384
Assert.AreEqual(0, reparseErrors.Count,
375385
"Generated SQL must reparse. Actual:\n" + generated);
376386
}
@@ -385,12 +395,17 @@ public void TestAlterTableAddIndexOnly_EmitsNoLeadingSeparator()
385395
// column or constraint, the generator must NOT emit a leading
386396
// comma before the INDEX (which would produce invalid syntax).
387397
var sql =
388-
"ALTER TABLE Sales.SalesOrderDetail_inmem \n" +
389-
" ADD INDEX ix_ModifiedDate (ModifiedDate); \n" +
390-
"GO \n";
398+
"ALTER TABLE Sales.SalesOrderDetail_inmem " + Environment.NewLine +
399+
" ADD INDEX ix_ModifiedDate (ModifiedDate); " + Environment.NewLine +
400+
"GO " + Environment.NewLine;
391401

392402
var parser = new TSql170Parser(true);
393-
var fragment = parser.Parse(new StringReader(sql), out var parseErrors);
403+
TSqlFragment fragment;
404+
IList<ParseError> parseErrors;
405+
using (var reader = new StringReader(sql))
406+
{
407+
fragment = parser.Parse(reader, out parseErrors);
408+
}
394409
Assert.AreEqual(0, parseErrors.Count, "Input must parse.");
395410

396411
var generator = new Sql170ScriptGenerator(new SqlScriptGeneratorOptions
@@ -405,7 +420,11 @@ public void TestAlterTableAddIndexOnly_EmitsNoLeadingSeparator()
405420
"INDEX clause must follow ADD directly. Actual:\n" + generated);
406421

407422
var reparser = new TSql170Parser(true);
408-
reparser.Parse(new StringReader(generated), out var reparseErrors);
423+
IList<ParseError> reparseErrors;
424+
using (var reader = new StringReader(generated))
425+
{
426+
reparser.Parse(reader, out reparseErrors);
427+
}
409428
Assert.AreEqual(0, reparseErrors.Count,
410429
"Generated SQL must reparse. Actual:\n" + generated);
411430
}
@@ -420,27 +439,32 @@ public void TestWindowDefinition_RefWindowFollowedByPartitionByEmitsSpace()
420439
// because the visitor didn't separate the inherited window-name
421440
// reference from the PARTITION keyword.
422441
var sql =
423-
"ALTER DATABASE AdventureWorks2025\n" +
424-
"SET COMPATIBILITY_LEVEL = 160;\n" +
425-
"GO\n" +
426-
"\n" +
427-
"USE AdventureWorks2025;\n" +
428-
"GO\n" +
429-
"\n" +
430-
"SELECT SalesOrderID AS OrderNumber,\n" +
431-
" ProductID,\n" +
432-
" OrderQty AS Qty,\n" +
433-
" SUM(OrderQty) OVER win2 AS Total,\n" +
434-
" AVG(OrderQty) OVER win1 AS Avg\n" +
435-
"FROM Sales.SalesOrderDetail\n" +
436-
"WHERE SalesOrderID IN (43659, 43664)\n" +
437-
" AND ProductID LIKE '71%'\n" +
438-
"WINDOW win1 AS (win3),\n" +
439-
" win2 AS (ORDER BY SalesOrderID, ProductID),\n" +
440-
" win3 AS (win2 PARTITION BY SalesOrderID);\n";
442+
"ALTER DATABASE AdventureWorks2025" + Environment.NewLine +
443+
"SET COMPATIBILITY_LEVEL = 160;" + Environment.NewLine +
444+
"GO" + Environment.NewLine +
445+
Environment.NewLine +
446+
"USE AdventureWorks2025;" + Environment.NewLine +
447+
"GO" + Environment.NewLine +
448+
Environment.NewLine +
449+
"SELECT SalesOrderID AS OrderNumber," + Environment.NewLine +
450+
" ProductID," + Environment.NewLine +
451+
" OrderQty AS Qty," + Environment.NewLine +
452+
" SUM(OrderQty) OVER win2 AS Total," + Environment.NewLine +
453+
" AVG(OrderQty) OVER win1 AS Avg" + Environment.NewLine +
454+
"FROM Sales.SalesOrderDetail" + Environment.NewLine +
455+
"WHERE SalesOrderID IN (43659, 43664)" + Environment.NewLine +
456+
" AND ProductID LIKE '71%'" + Environment.NewLine +
457+
"WINDOW win1 AS (win3)," + Environment.NewLine +
458+
" win2 AS (ORDER BY SalesOrderID, ProductID)," + Environment.NewLine +
459+
" win3 AS (win2 PARTITION BY SalesOrderID);" + Environment.NewLine;
441460

442461
var parser = new TSql170Parser(true);
443-
var fragment = parser.Parse(new StringReader(sql), out var parseErrors);
462+
TSqlFragment fragment;
463+
IList<ParseError> parseErrors;
464+
using (var reader = new StringReader(sql))
465+
{
466+
fragment = parser.Parse(reader, out parseErrors);
467+
}
444468
Assert.AreEqual(0, parseErrors.Count, "Input must parse.");
445469

446470
var generator = new Sql170ScriptGenerator(new SqlScriptGeneratorOptions
@@ -455,7 +479,11 @@ public void TestWindowDefinition_RefWindowFollowedByPartitionByEmitsSpace()
455479
"Generated window must read 'win2 PARTITION'. Actual:\n" + generated);
456480

457481
var reparser = new TSql170Parser(true);
458-
reparser.Parse(new StringReader(generated), out var reparseErrors);
482+
IList<ParseError> reparseErrors;
483+
using (var reader = new StringReader(generated))
484+
{
485+
reparser.Parse(reader, out reparseErrors);
486+
}
459487
Assert.AreEqual(0, reparseErrors.Count,
460488
"Generated SQL must reparse. Actual:\n" + generated);
461489
}
@@ -470,13 +498,18 @@ public void TestWindowDefinition_RefWindowFollowedByOrderByEmitsSpace()
470498
// 'refnameORDER'. This shape isn't in the docs but the same code
471499
// path can produce it; included as belt-and-suspenders coverage.
472500
var sql =
473-
"SELECT SalesOrderID, SUM(OrderQty) OVER win2 AS Total\n" +
474-
"FROM Sales.SalesOrderDetail\n" +
475-
"WINDOW win1 AS (PARTITION BY ProductID),\n" +
501+
"SELECT SalesOrderID, SUM(OrderQty) OVER win2 AS Total" + Environment.NewLine +
502+
"FROM Sales.SalesOrderDetail" + Environment.NewLine +
503+
"WINDOW win1 AS (PARTITION BY ProductID)," + Environment.NewLine +
476504
" win2 AS (win1 ORDER BY SalesOrderID);";
477505

478506
var parser = new TSql170Parser(true);
479-
var fragment = parser.Parse(new StringReader(sql), out var parseErrors);
507+
TSqlFragment fragment;
508+
IList<ParseError> parseErrors;
509+
using (var reader = new StringReader(sql))
510+
{
511+
fragment = parser.Parse(reader, out parseErrors);
512+
}
480513
Assert.AreEqual(0, parseErrors.Count, "Input must parse.");
481514

482515
var generator = new Sql170ScriptGenerator(new SqlScriptGeneratorOptions
@@ -491,7 +524,11 @@ public void TestWindowDefinition_RefWindowFollowedByOrderByEmitsSpace()
491524
"Generated window must read 'win1 ORDER'. Actual:\n" + generated);
492525

493526
var reparser = new TSql170Parser(true);
494-
reparser.Parse(new StringReader(generated), out var reparseErrors);
527+
IList<ParseError> reparseErrors;
528+
using (var reader = new StringReader(generated))
529+
{
530+
reparser.Parse(reader, out reparseErrors);
531+
}
495532
Assert.AreEqual(0, reparseErrors.Count,
496533
"Generated SQL must reparse. Actual:\n" + generated);
497534
}

0 commit comments

Comments
 (0)