From d4c3a1e1eb5879826e721934d7b165e3fd634e67 Mon Sep 17 00:00:00 2001 From: Randolph West MSFT <97149825+rwestMSFT@users.noreply.github.com> Date: Wed, 27 May 2026 13:34:35 -0600 Subject: [PATCH] Fix generator whitespace bugs: ALTER TABLE ADD INDEX and WINDOW name reference Two unrelated pre-existing generator bugs surfaced by round-tripping the sql-docs corpus: 1. AlterTableAddTableElementStatement omitted the separator between column-definitions/constraints and table indexes, producing 'NOT NULLINDEX ix_...' which fails to reparse. 2. WindowDefinition omitted whitespace between the inherited window-name reference (RefWindowName) and a following PARTITION BY or ORDER BY, producing 'win2PARTITION ...' which fails to reparse. Both fixes are local to their visitors and follow the same separator patterns already present in sibling code paths. Adds three focused regression tests under a new 'Generator Whitespace Regression Tests' region. --- ...itor.AlterTableAddTableElementStatement.cs | 10 ++ ...ScriptGeneratorVisitor.WindowDefinition.cs | 7 + Test/SqlDom/ScriptGeneratorTests.cs | 163 ++++++++++++++++++ 3 files changed, 180 insertions(+) diff --git a/SqlScriptDom/ScriptDom/SqlServer/ScriptGenerator/SqlScriptGeneratorVisitor.AlterTableAddTableElementStatement.cs b/SqlScriptDom/ScriptDom/SqlServer/ScriptGenerator/SqlScriptGeneratorVisitor.AlterTableAddTableElementStatement.cs index a1d21e0..60a830b 100644 --- a/SqlScriptDom/ScriptDom/SqlServer/ScriptGenerator/SqlScriptGeneratorVisitor.AlterTableAddTableElementStatement.cs +++ b/SqlScriptDom/ScriptDom/SqlServer/ScriptGenerator/SqlScriptGeneratorVisitor.AlterTableAddTableElementStatement.cs @@ -61,6 +61,16 @@ public override void ExplicitVisit(AlterTableAddTableElementStatement node) ExplicitVisit((SystemTimePeriodDefinition)node.Definition.SystemTimePeriod); } + if ((node.Definition.ColumnDefinitions.Count > 0 + || node.Definition.TableConstraints.Count > 0 + || node.Definition.SystemTimePeriod != null) + && node.Definition.Indexes != null + && node.Definition.Indexes.Count > 0) + { + GenerateSymbolAndSpace(TSqlTokenType.Comma); + NewLine(); + } + if (node.Definition.Indexes != null && node.Definition.Indexes.Count > 0) { GenerateCommaSeparatedList(node.Definition.Indexes, true); diff --git a/SqlScriptDom/ScriptDom/SqlServer/ScriptGenerator/SqlScriptGeneratorVisitor.WindowDefinition.cs b/SqlScriptDom/ScriptDom/SqlServer/ScriptGenerator/SqlScriptGeneratorVisitor.WindowDefinition.cs index 94934f2..dc40eb8 100644 --- a/SqlScriptDom/ScriptDom/SqlServer/ScriptGenerator/SqlScriptGeneratorVisitor.WindowDefinition.cs +++ b/SqlScriptDom/ScriptDom/SqlServer/ScriptGenerator/SqlScriptGeneratorVisitor.WindowDefinition.cs @@ -18,6 +18,13 @@ public override void ExplicitVisit(WindowDefinition node) GenerateFragmentIfNotNull(node.RefWindowName); bool partitionByClauseExists = node.Partitions.Count > 0; + + // 'win2PARTITION' / 'win2ORDER' would otherwise tokenize as one identifier. + if (node.RefWindowName != null && (partitionByClauseExists || node.OrderByClause != null)) + { + GenerateSpace(); + } + if (partitionByClauseExists) { GenerateIdentifier(CodeGenerationSupporter.Partition); diff --git a/Test/SqlDom/ScriptGeneratorTests.cs b/Test/SqlDom/ScriptGeneratorTests.cs index 34eebbc..2f0bb22 100644 --- a/Test/SqlDom/ScriptGeneratorTests.cs +++ b/Test/SqlDom/ScriptGeneratorTests.cs @@ -335,6 +335,169 @@ void ParseAndAssertEquality(string sqlText, SqlScriptGeneratorOptions generatorO Assert.AreEqual(sqlText, generatedSqlText); } + #region Generator Whitespace Regression Tests + + [TestMethod] + [Priority(0)] + [SqlStudioTestCategory(Category.UnitTest)] + public void TestAlterTableAddColumnThenIndex_EmitsSeparatorBeforeIndex() + { + // Verbatim sample from docs/relational-databases/in-memory-oltp/ + // altering-memory-optimized-tables.md (line 119). Generator was + // omitting the separator between the trailing column constraint + // ('NOT NULL') and the inline INDEX, producing 'NOT NULLINDEX + // ix_Customer' which fails to reparse. + var sql = + "ALTER TABLE Sales.SalesOrderDetail_inmem \n" + + " ADD CustomerID int NOT NULL DEFAULT -1 WITH VALUES, \n" + + " ShipMethodID int NOT NULL DEFAULT -1 WITH VALUES, \n" + + " INDEX ix_Customer (CustomerID); \n" + + "GO \n"; + + var parser = new TSql170Parser(true); + var fragment = parser.Parse(new StringReader(sql), out var parseErrors); + Assert.AreEqual(0, parseErrors.Count, "Input must parse."); + + var generator = new Sql170ScriptGenerator(new SqlScriptGeneratorOptions + { + IncludeSemicolons = true, + }); + generator.GenerateScript(fragment, out var generated); + + Assert.IsFalse(generated.Contains("NULLINDEX"), + "Column constraint 'NULL' must not run into the 'INDEX' keyword. Actual:\n" + generated); + Assert.IsTrue(generated.Contains("INDEX ix_Customer"), + "INDEX keyword must be present and separated. Actual:\n" + generated); + + var reparser = new TSql170Parser(true); + reparser.Parse(new StringReader(generated), out var reparseErrors); + Assert.AreEqual(0, reparseErrors.Count, + "Generated SQL must reparse. Actual:\n" + generated); + } + + [TestMethod] + [Priority(0)] + [SqlStudioTestCategory(Category.UnitTest)] + public void TestAlterTableAddIndexOnly_EmitsNoLeadingSeparator() + { + // Verbatim sample from docs/relational-databases/in-memory-oltp/ + // altering-memory-optimized-tables.md (line 111). With no preceding + // column or constraint, the generator must NOT emit a leading + // comma before the INDEX (which would produce invalid syntax). + var sql = + "ALTER TABLE Sales.SalesOrderDetail_inmem \n" + + " ADD INDEX ix_ModifiedDate (ModifiedDate); \n" + + "GO \n"; + + var parser = new TSql170Parser(true); + var fragment = parser.Parse(new StringReader(sql), out var parseErrors); + Assert.AreEqual(0, parseErrors.Count, "Input must parse."); + + var generator = new Sql170ScriptGenerator(new SqlScriptGeneratorOptions + { + IncludeSemicolons = true, + }); + generator.GenerateScript(fragment, out var generated); + + Assert.IsFalse(generated.Contains("ADD ,") || generated.Contains("ADD\n,"), + "ADD must not be followed by a stray separator. Actual:\n" + generated); + Assert.IsTrue(generated.Contains("ADD INDEX ix_ModifiedDate"), + "INDEX clause must follow ADD directly. Actual:\n" + generated); + + var reparser = new TSql170Parser(true); + reparser.Parse(new StringReader(generated), out var reparseErrors); + Assert.AreEqual(0, reparseErrors.Count, + "Generated SQL must reparse. Actual:\n" + generated); + } + + [TestMethod] + [Priority(0)] + [SqlStudioTestCategory(Category.UnitTest)] + public void TestWindowDefinition_RefWindowFollowedByPartitionByEmitsSpace() + { + // Verbatim sample from docs/t-sql/queries/select-window-transact-sql.md + // (line 312). Generator was emitting 'win2PARTITION' (no space) + // because the visitor didn't separate the inherited window-name + // reference from the PARTITION keyword. + var sql = + "ALTER DATABASE AdventureWorks2025\n" + + "SET COMPATIBILITY_LEVEL = 160;\n" + + "GO\n" + + "\n" + + "USE AdventureWorks2025;\n" + + "GO\n" + + "\n" + + "SELECT SalesOrderID AS OrderNumber,\n" + + " ProductID,\n" + + " OrderQty AS Qty,\n" + + " SUM(OrderQty) OVER win2 AS Total,\n" + + " AVG(OrderQty) OVER win1 AS Avg\n" + + "FROM Sales.SalesOrderDetail\n" + + "WHERE SalesOrderID IN (43659, 43664)\n" + + " AND ProductID LIKE '71%'\n" + + "WINDOW win1 AS (win3),\n" + + " win2 AS (ORDER BY SalesOrderID, ProductID),\n" + + " win3 AS (win2 PARTITION BY SalesOrderID);\n"; + + var parser = new TSql170Parser(true); + var fragment = parser.Parse(new StringReader(sql), out var parseErrors); + Assert.AreEqual(0, parseErrors.Count, "Input must parse."); + + var generator = new Sql170ScriptGenerator(new SqlScriptGeneratorOptions + { + IncludeSemicolons = true, + }); + generator.GenerateScript(fragment, out var generated); + + Assert.IsFalse(generated.Contains("win2PARTITION"), + "Window-name reference must not run into PARTITION keyword. Actual:\n" + generated); + Assert.IsTrue(generated.Contains("win2 PARTITION"), + "Generated window must read 'win2 PARTITION'. Actual:\n" + generated); + + var reparser = new TSql170Parser(true); + reparser.Parse(new StringReader(generated), out var reparseErrors); + Assert.AreEqual(0, reparseErrors.Count, + "Generated SQL must reparse. Actual:\n" + generated); + } + + [TestMethod] + [Priority(0)] + [SqlStudioTestCategory(Category.UnitTest)] + public void TestWindowDefinition_RefWindowFollowedByOrderByEmitsSpace() + { + // Same bug class as above, exercised through the ORDER BY branch. + // 'WINDOW name AS (refname ORDER BY ...)' must not emit + // 'refnameORDER'. This shape isn't in the docs but the same code + // path can produce it; included as belt-and-suspenders coverage. + var sql = + "SELECT SalesOrderID, SUM(OrderQty) OVER win2 AS Total\n" + + "FROM Sales.SalesOrderDetail\n" + + "WINDOW win1 AS (PARTITION BY ProductID),\n" + + " win2 AS (win1 ORDER BY SalesOrderID);"; + + var parser = new TSql170Parser(true); + var fragment = parser.Parse(new StringReader(sql), out var parseErrors); + Assert.AreEqual(0, parseErrors.Count, "Input must parse."); + + var generator = new Sql170ScriptGenerator(new SqlScriptGeneratorOptions + { + IncludeSemicolons = true, + }); + generator.GenerateScript(fragment, out var generated); + + Assert.IsFalse(generated.Contains("win1ORDER"), + "Window-name reference must not run into ORDER keyword. Actual:\n" + generated); + Assert.IsTrue(generated.Contains("win1 ORDER"), + "Generated window must read 'win1 ORDER'. Actual:\n" + generated); + + var reparser = new TSql170Parser(true); + reparser.Parse(new StringReader(generated), out var reparseErrors); + Assert.AreEqual(0, reparseErrors.Count, + "Generated SQL must reparse. Actual:\n" + generated); + } + + #endregion + #region Comment Preservation Tests [TestMethod]