From 3109defcc0a34f2773bb7c8d6e310cce9b965fa5 Mon Sep 17 00:00:00 2001 From: "Aasim Khan (from Dev Box)" Date: Tue, 1 Sep 2026 10:58:49 -0700 Subject: [PATCH 1/3] Shorten Peek Definition tab names Peek Definition wrote each generated script to a file named ".._<32 hex GUID>.sql", and VS Code labels a tab with the file name, so a handful of open definitions filled the tab strip with unreadable titles. The GUID also meant every request produced a new file, so asking for the same definition twice opened a second tab for it. Name the file after the object instead. A given object keeps one file for the life of the process, so a repeated request refreshes the tab already open. Two different objects that want the same name, which happens when one qualified name resolves against two connections, are separated by a numeric suffix. The GUID was also doing real work: it was added to make these APIs parallel safe, since definition requests run concurrently and would otherwise write the same path at once. That is now handled directly, by taking a per-file lock around the write and by reading the lines from the generated script rather than back off disk. A write is skipped entirely when the file already holds the same script, so an editor with the definition open is left alone. Object identity is compared case sensitively, because a case sensitive collation can hold both "Foo" and "foo" and giving them one file would show the wrong definition for one of them. Characters that are legal in a quoted identifier but not in a file name are replaced, which the GUID never guarded against. --- .../Scripting/PeekDefinitionFileNames.cs | 125 +++++++ .../Scripting/ScripterCore.cs | 82 ++++- .../PeekDefinitionFileNameTests.cs | 321 ++++++++++++++++++ .../LanguageServer/PeekDefinitionTests.cs | 18 +- 4 files changed, 522 insertions(+), 24 deletions(-) create mode 100644 src/Microsoft.SqlTools.LanguageService/Scripting/PeekDefinitionFileNames.cs create mode 100644 test/Microsoft.SqlTools.ServiceLayer.UnitTests/LanguageServer/PeekDefinitionFileNameTests.cs diff --git a/src/Microsoft.SqlTools.LanguageService/Scripting/PeekDefinitionFileNames.cs b/src/Microsoft.SqlTools.LanguageService/Scripting/PeekDefinitionFileNames.cs new file mode 100644 index 0000000000..ca9df779a2 --- /dev/null +++ b/src/Microsoft.SqlTools.LanguageService/Scripting/PeekDefinitionFileNames.cs @@ -0,0 +1,125 @@ +// +// Copyright (c) Microsoft. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. +// + +#nullable disable + +using System; +using System.Collections.Generic; +using System.IO; +using System.Text; +using Microsoft.SqlTools.Utility; + +namespace Microsoft.SqlTools.LanguageService.Scripting +{ + /// + /// Hands out the file names used to hold Peek Definition scripts. An object keeps the same + /// file for the lifetime of the process, so asking for its definition again refreshes the + /// editor tab that is already open instead of adding another one. Two different objects that + /// want the same file name are told apart by a numeric suffix. + /// + internal static class PeekDefinitionFileNames + { + private const string Extension = ".sql"; + + private static readonly object SyncRoot = new object(); + + /// Object identity to the file name assigned to it. + private static readonly Dictionary NamesByIdentity = + new Dictionary(StringComparer.Ordinal); + + /// + /// Every name handed out so far, compared the way the file system compares them so that + /// two objects differing only by case still get separate files. + /// + private static readonly HashSet AssignedNames = + new HashSet(StringComparer.OrdinalIgnoreCase); + + /// + /// Returns the file name to use for an object, assigning one the first time it is seen. + /// + /// Identifies the object, from . + /// The preferred file name, without extension. + internal static string GetOrAssign(string identity, string baseName) + { + Validate.IsNotNull(nameof(identity), identity); + Validate.IsNotNullOrWhitespaceString(nameof(baseName), baseName); + + lock (SyncRoot) + { + if (NamesByIdentity.TryGetValue(identity, out string assigned)) + { + return assigned; + } + + string candidate = baseName + Extension; + for (int suffix = 2; !AssignedNames.Add(candidate); suffix++) + { + candidate = $"{baseName}_{suffix}{Extension}"; + } + + NamesByIdentity[identity] = candidate; + return candidate; + } + } + + /// + /// Builds the value that distinguishes one object from another. The parts are compared + /// case sensitively: a case sensitive collation can hold both "Foo" and "foo", and giving + /// them one file would show the wrong definition for one of them. + /// + internal static string CreateIdentity( + string serverName, + string databaseName, + string schemaName, + string objectName) + { + StringBuilder identity = new StringBuilder(); + AppendPart(identity, serverName); + AppendPart(identity, databaseName); + AppendPart(identity, schemaName); + AppendPart(identity, objectName); + return identity.ToString(); + } + + /// + /// Appends one part of an identity. The length prefix keeps the parts unambiguous without + /// relying on a separator character that a quoted identifier could itself contain. + /// + private static void AppendPart(StringBuilder builder, string part) + { + part ??= string.Empty; + builder.Append(part.Length).Append(':').Append(part); + } + + /// + /// Replaces the characters that are legal in a quoted SQL identifier but not in a file + /// name. Two names that differ only in those characters collapse onto the same base name + /// and are then separated by the numeric suffix, so no definition is ever lost. + /// + internal static string SanitizeBaseName(string baseName) + { + char[] invalidCharacters = Path.GetInvalidFileNameChars(); + StringBuilder sanitized = new StringBuilder(baseName.Length); + foreach (char character in baseName) + { + sanitized.Append(Array.IndexOf(invalidCharacters, character) >= 0 ? '_' : character); + } + + return sanitized.ToString(); + } + + /// + /// Forgets every assigned name. For tests only. + /// + internal static void Reset() + { + lock (SyncRoot) + { + NamesByIdentity.Clear(); + AssignedNames.Clear(); + } + } + } +} diff --git a/src/Microsoft.SqlTools.LanguageService/Scripting/ScripterCore.cs b/src/Microsoft.SqlTools.LanguageService/Scripting/ScripterCore.cs index 56ff554ecd..fceb63bf55 100644 --- a/src/Microsoft.SqlTools.LanguageService/Scripting/ScripterCore.cs +++ b/src/Microsoft.SqlTools.LanguageService/Scripting/ScripterCore.cs @@ -7,6 +7,7 @@ #pragma warning disable CS8632 using System; +using System.Collections.Concurrent; using System.Collections.Generic; using System.Data.Common; using System.IO; @@ -54,6 +55,14 @@ internal sealed partial class Scripter private Dictionary objectScriptMap = new Dictionary(); + /// + /// Guards each definition file so that requests running in parallel cannot interleave + /// their writes. Scripts are only ever written into the per-process folder owned by + /// , so no other process competes for these files. + /// + private static readonly ConcurrentDictionary FileLocks = + new ConcurrentDictionary(StringComparer.OrdinalIgnoreCase); + internal Scripter() { } /// @@ -294,20 +303,27 @@ internal Location[] GetSqlObjectDefinition( Sql3PartIdentifier identifier, string objectType) { - // script file destination - string fileName = CreateFileName(identifier); - - string tempFileName = Path.Combine(this.tempPath, fileName); - SmoScriptingOperation operation = InitScriptOperation(identifier, objectType); operation.Execute(); string script = operation.ScriptText; + // script file destination, resolved the same way the scripting operation resolves the + // database so that the file name always describes the definition it holds + string fileName = CreateFileName( + identifier, + this.serverConnection?.ServerInstance, + identifier.DatabaseName ?? this.Database?.Name); + + string tempFileName = Path.Combine(this.tempPath, fileName); + bool objectFound = false; int createStatementLineNumber = 0; - File.WriteAllText(tempFileName, script); - string[] lines = File.ReadAllLines(tempFileName); + WriteScriptFile(tempFileName, script); + + // Read the lines from the script we just generated rather than back off disk, so a + // request running in parallel for the same object cannot be seen mid write + string[] lines = script.Split(new[] { "\r\n", "\r", "\n" }, StringSplitOptions.None); int lineCount = 0; string createSyntax = null; if (objectScriptMap.ContainsKey(objectType.ToLower(System.Globalization.CultureInfo.InvariantCulture))) @@ -337,24 +353,64 @@ internal Location[] GetSqlObjectDefinition( } } - private static string CreateFileName(Sql3PartIdentifier identifier) + /// + /// Returns the file name that holds an object's definition. The name is stable for a given + /// object so that requesting its definition again reuses the editor tab that is already + /// open. Objects that would otherwise share a name are separated by a numeric suffix. + /// + /// The object being scripted. + /// The server the object was resolved against, used only to tell + /// apart objects that share a fully qualified name across connections. + /// The resolved database name, which may differ from the one on + /// when the request did not qualify it. + internal static string CreateFileName( + Sql3PartIdentifier identifier, + string serverName, + string databaseName) { string baseFileName; - if (identifier.DatabaseName != null) + if (!string.IsNullOrEmpty(databaseName)) { - baseFileName = $"{identifier.DatabaseName}.{identifier.SchemaName}.{identifier.ObjectName}.sql"; + baseFileName = $"{databaseName}.{identifier.SchemaName}.{identifier.ObjectName}"; } else if (identifier.SchemaName != null) { - baseFileName = $"{identifier.SchemaName}.{identifier.ObjectName}.sql"; + baseFileName = $"{identifier.SchemaName}.{identifier.ObjectName}"; } else { - baseFileName = $"{identifier.ObjectName}.sql"; + baseFileName = identifier.ObjectName; } - return $"{Path.GetFileNameWithoutExtension(baseFileName)}_{Guid.NewGuid():N}{Path.GetExtension(baseFileName)}"; + string identity = PeekDefinitionFileNames.CreateIdentity( + serverName, + databaseName, + identifier.SchemaName, + identifier.ObjectName); + + return PeekDefinitionFileNames.GetOrAssign( + identity, + PeekDefinitionFileNames.SanitizeBaseName(baseFileName)); + } + + /// + /// Writes a definition to its file. The file is left untouched when it already holds the + /// same script, so repeating a request does not disturb an editor that has it open. + /// + internal static void WriteScriptFile(string path, string script) + { + object fileLock = FileLocks.GetOrAdd(path, _ => new object()); + lock (fileLock) + { + if (File.Exists(path) + && string.Equals(File.ReadAllText(path), script, StringComparison.Ordinal)) + { + return; + } + + File.WriteAllText(path, script); + } } #region Helper Methods diff --git a/test/Microsoft.SqlTools.ServiceLayer.UnitTests/LanguageServer/PeekDefinitionFileNameTests.cs b/test/Microsoft.SqlTools.ServiceLayer.UnitTests/LanguageServer/PeekDefinitionFileNameTests.cs new file mode 100644 index 0000000000..11d8318c37 --- /dev/null +++ b/test/Microsoft.SqlTools.ServiceLayer.UnitTests/LanguageServer/PeekDefinitionFileNameTests.cs @@ -0,0 +1,321 @@ +// +// Copyright (c) Microsoft. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. +// + +#nullable disable + +using System; +using System.Collections.Concurrent; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Threading.Tasks; +using Microsoft.SqlTools.LanguageService.Scripting; +using NUnit.Framework; + +namespace Microsoft.SqlTools.ServiceLayer.UnitTests.LanguageServer +{ + /// + /// Tests the file names Peek Definition assigns to the scripts it generates. The names show up + /// as editor tab titles, so they need to stay short, stay stable for a given object, and still + /// keep two different objects apart. + /// + public class PeekDefinitionFileNameTests + { + private const string ServerA = "serverA"; + private const string ServerB = "serverB"; + + [SetUp] + [TearDown] + public void ForgetAssignedNames() + { + PeekDefinitionFileNames.Reset(); + } + + private static Sql3PartIdentifier Identifier( + string databaseName, + string schemaName, + string objectName) + { + return new Sql3PartIdentifier + { + DatabaseName = databaseName, + SchemaName = schemaName, + ObjectName = objectName + }; + } + + private static string NameFor( + string server, + string database, + string schema, + string objectName) + { + Sql3PartIdentifier identifier = Identifier(database, schema, objectName); + return Scripter.CreateFileName(identifier, server, database); + } + + [Test] + public void NameCarriesNoRandomSuffix() + { + // The 32 character GUID this replaced is what made tab titles unreadable + Assert.AreEqual("master.dbo.myTable.sql", NameFor(ServerA, "master", "dbo", "myTable")); + } + + [Test] + public void RepeatedRequestsForOneObjectShareOneFile() + { + string first = NameFor(ServerA, "master", "dbo", "myTable"); + string second = NameFor(ServerA, "master", "dbo", "myTable"); + string third = NameFor(ServerA, "master", "dbo", "myTable"); + + Assert.AreEqual(first, second); + Assert.AreEqual(first, third); + } + + [Test] + public void DifferentObjectsInOneDatabaseGetTheirOwnFiles() + { + string table = NameFor(ServerA, "master", "dbo", "myTable"); + string view = NameFor(ServerA, "master", "dbo", "myView"); + + Assert.AreEqual("master.dbo.myTable.sql", table); + Assert.AreEqual("master.dbo.myView.sql", view); + } + + [Test] + public void SameObjectNameInDifferentSchemasDoesNotCollide() + { + string dbo = NameFor(ServerA, "master", "dbo", "myTable"); + string sales = NameFor(ServerA, "master", "sales", "myTable"); + + Assert.AreEqual("master.dbo.myTable.sql", dbo); + Assert.AreEqual("master.sales.myTable.sql", sales); + } + + [Test] + public void SameObjectNameInDifferentDatabasesDoesNotCollide() + { + // The database is part of the name, so these never need a numeric suffix + string first = NameFor(ServerA, "dbOne", "dbo", "myTable"); + string second = NameFor(ServerA, "dbTwo", "dbo", "myTable"); + + Assert.AreEqual("dbOne.dbo.myTable.sql", first); + Assert.AreEqual("dbTwo.dbo.myTable.sql", second); + } + + [Test] + public void SameQualifiedNameOnAnotherServerFallsBackToANumberedFile() + { + // Nothing in the name distinguishes the servers, so the second one is numbered + string first = NameFor(ServerA, "master", "dbo", "myTable"); + string second = NameFor(ServerB, "master", "dbo", "myTable"); + + Assert.AreEqual("master.dbo.myTable.sql", first); + Assert.AreEqual("master.dbo.myTable_2.sql", second); + } + + [Test] + public void EachAdditionalServerTakesTheNextNumber() + { + string first = NameFor(ServerA, "master", "dbo", "myTable"); + string second = NameFor(ServerB, "master", "dbo", "myTable"); + string third = NameFor("serverC", "master", "dbo", "myTable"); + + Assert.AreEqual("master.dbo.myTable.sql", first); + Assert.AreEqual("master.dbo.myTable_2.sql", second); + Assert.AreEqual("master.dbo.myTable_3.sql", third); + + // and each of them stays put once assigned + Assert.AreEqual(second, NameFor(ServerB, "master", "dbo", "myTable")); + Assert.AreEqual(first, NameFor(ServerA, "master", "dbo", "myTable")); + } + + [Test] + public void UnqualifiedRequestUsesTheResolvedDatabase() + { + // The request did not name a database; the caller resolves it from the connection so + // that the file name matches the definition actually scripted + Sql3PartIdentifier unqualified = Identifier(null, "dbo", "myTable"); + + string resolvedToOne = Scripter.CreateFileName(unqualified, ServerA, "dbOne"); + string resolvedToTwo = Scripter.CreateFileName(unqualified, ServerA, "dbTwo"); + + Assert.AreEqual("dbOne.dbo.myTable.sql", resolvedToOne); + Assert.AreEqual("dbTwo.dbo.myTable.sql", resolvedToTwo); + } + + [Test] + public void RequestWithoutADatabaseOmitsTheDatabaseSegment() + { + string name = Scripter.CreateFileName(Identifier(null, "dbo", "myTable"), ServerA, null); + + Assert.AreEqual("dbo.myTable.sql", name); + } + + [Test] + public void RequestWithoutASchemaOmitsTheSchemaSegment() + { + string name = Scripter.CreateFileName(Identifier(null, null, "myTable"), ServerA, null); + + Assert.AreEqual("myTable.sql", name); + } + + [Test] + public void ObjectsDifferingOnlyByCaseGetSeparateFiles() + { + // A case sensitive collation can hold both, and sharing one file would show the wrong + // definition for one of them. The file system ignores case, hence the numbered name. + string upper = NameFor(ServerA, "master", "dbo", "MyTable"); + string lower = NameFor(ServerA, "master", "dbo", "mytable"); + + Assert.AreEqual("master.dbo.MyTable.sql", upper); + Assert.AreEqual("master.dbo.mytable_2.sql", lower); + } + + [Test] + public void CharactersThatAreIllegalInAFileNameAreReplaced() + { + string name = NameFor(ServerA, "master", "dbo", "odd/name:here"); + + Assert.AreEqual(-1, name.IndexOfAny(Path.GetInvalidFileNameChars())); + Assert.AreEqual("master.dbo.odd_name_here.sql", name); + } + + [Test] + public void ObjectsThatSanitizeToTheSameNameStayApart() + { + // "a/b" and "a:b" both sanitize to "a_b", so the second still gets its own file + string first = NameFor(ServerA, "master", "dbo", "a/b"); + string second = NameFor(ServerA, "master", "dbo", "a:b"); + + Assert.AreEqual("master.dbo.a_b.sql", first); + Assert.AreEqual("master.dbo.a_b_2.sql", second); + } + + [Test] + public void ConcurrentRequestsForOneObjectAgreeOnOneFile() + { + ConcurrentBag names = new ConcurrentBag(); + + Parallel.For(0, 200, _ => + names.Add(NameFor(ServerA, "master", "dbo", "myTable"))); + + Assert.AreEqual(1, names.Distinct().Count(), "every caller should see the same name"); + Assert.AreEqual("master.dbo.myTable.sql", names.First()); + } + + [Test] + public void ConcurrentRequestsForDistinctObjectsNeverShareAFile() + { + const int objectCount = 200; + ConcurrentBag names = new ConcurrentBag(); + + // Every object has a distinct name, so no numeric suffix should be needed at all + Parallel.For(0, objectCount, index => + names.Add(NameFor(ServerA, "master", "dbo", $"table{index}"))); + + Assert.AreEqual(objectCount, names.Distinct().Count()); + CollectionAssert.IsEmpty(names.Where(name => name.Contains("_2.sql"))); + } + + [Test] + public void ConcurrentRequestsThatAllWantOneNameAreNumberedWithoutDuplicates() + { + const int serverCount = 100; + ConcurrentBag names = new ConcurrentBag(); + + // Same qualified name on many servers: all of them want "master.dbo.myTable.sql" + Parallel.For(0, serverCount, index => + names.Add(NameFor($"server{index}", "master", "dbo", "myTable"))); + + Assert.AreEqual(serverCount, names.Distinct().Count(), "no two objects may share a file"); + CollectionAssert.Contains(names, "master.dbo.myTable.sql"); + } + } + + /// + /// Tests writing the generated script to its file. + /// + public class PeekDefinitionFileWriteTests + { + private string folder; + + [SetUp] + public void CreateFolder() + { + folder = Path.Combine(Path.GetTempPath(), $"peek_write_{Guid.NewGuid():N}"); + Directory.CreateDirectory(folder); + } + + [TearDown] + public void RemoveFolder() + { + try + { + Directory.Delete(folder, recursive: true); + } + catch (IOException) + { + } + } + + [Test] + public void WritesTheScript() + { + string path = Path.Combine(folder, "definition.sql"); + + Scripter.WriteScriptFile(path, "CREATE VIEW dbo.v AS SELECT 1"); + + Assert.AreEqual("CREATE VIEW dbo.v AS SELECT 1", File.ReadAllText(path)); + } + + [Test] + public void RewritingTheSameScriptLeavesTheFileAlone() + { + // Rewriting would make an editor that has the file open reload it, and would discard + // anything the user had typed into that buffer + string path = Path.Combine(folder, "definition.sql"); + const string script = "CREATE VIEW dbo.v AS SELECT 1"; + Scripter.WriteScriptFile(path, script); + + DateTime stamp = new DateTime(2020, 1, 1, 0, 0, 0, DateTimeKind.Utc); + File.SetLastWriteTimeUtc(path, stamp); + + Scripter.WriteScriptFile(path, script); + + Assert.AreEqual(stamp, File.GetLastWriteTimeUtc(path)); + } + + [Test] + public void AChangedDefinitionReplacesTheFileInPlace() + { + string path = Path.Combine(folder, "definition.sql"); + Scripter.WriteScriptFile(path, "CREATE VIEW dbo.v AS SELECT 1"); + + Scripter.WriteScriptFile(path, "CREATE VIEW dbo.v AS SELECT 2"); + + // Same path, so the editor tab is reused rather than a second one being opened + Assert.AreEqual("CREATE VIEW dbo.v AS SELECT 2", File.ReadAllText(path)); + Assert.AreEqual(1, Directory.GetFiles(folder).Length); + } + + [Test] + public void ConcurrentWritesLeaveOneCompleteScript() + { + string path = Path.Combine(folder, "definition.sql"); + string shortScript = "CREATE VIEW dbo.v AS SELECT 1"; + string longScript = "CREATE VIEW dbo.v AS SELECT " + new string('9', 200000); + List allowed = new List { shortScript, longScript }; + + // Hammer one file from both sides. Writes must not interleave, so whichever one lands + // last the file holds exactly one of the two scripts and never a mixture. + Parallel.For(0, 200, index => + Scripter.WriteScriptFile(path, index % 2 == 0 ? shortScript : longScript)); + + CollectionAssert.Contains(allowed, File.ReadAllText(path)); + Assert.AreEqual(1, Directory.GetFiles(folder).Length); + } + } +} diff --git a/test/Microsoft.SqlTools.ServiceLayer.UnitTests/LanguageServer/PeekDefinitionTests.cs b/test/Microsoft.SqlTools.ServiceLayer.UnitTests/LanguageServer/PeekDefinitionTests.cs index 6d2911cd50..c9be185c44 100644 --- a/test/Microsoft.SqlTools.ServiceLayer.UnitTests/LanguageServer/PeekDefinitionTests.cs +++ b/test/Microsoft.SqlTools.ServiceLayer.UnitTests/LanguageServer/PeekDefinitionTests.cs @@ -7,7 +7,6 @@ using System; using System.IO; -using System.Reflection; using System.Runtime.InteropServices; using System.Threading.Tasks; using Microsoft.SqlServer.Management.SqlParser.Intellisense; @@ -168,10 +167,9 @@ public async Task GetPeekDefinitionTempFolder_IsThreadSafe() } [Test] - public void CreateFileName_ReturnsUniqueNamePerRequest() + public void CreateFileName_ReturnsSameNameForTheSameObject() { - MethodInfo createFileNameMethod = typeof(Scripter).GetMethod("CreateFileName", BindingFlags.Static | BindingFlags.NonPublic); - Assert.NotNull(createFileNameMethod); + PeekDefinitionFileNames.Reset(); var identifier = new Sql3PartIdentifier { @@ -180,14 +178,12 @@ public void CreateFileName_ReturnsUniqueNamePerRequest() ObjectName = "testTable" }; - string firstFileName = (string)createFileNameMethod.Invoke(null, new object[] { identifier }); - string secondFileName = (string)createFileNameMethod.Invoke(null, new object[] { identifier }); + string firstFileName = Scripter.CreateFileName(identifier, "serverA", identifier.DatabaseName); + string secondFileName = Scripter.CreateFileName(identifier, "serverA", identifier.DatabaseName); - Assert.AreNotEqual(firstFileName, secondFileName); - StringAssert.StartsWith("master.dbo.testTable_", firstFileName); - StringAssert.StartsWith("master.dbo.testTable_", secondFileName); - StringAssert.EndsWith(".sql", firstFileName); - StringAssert.EndsWith(".sql", secondFileName); + // The same object keeps one file so that a repeated request reuses the open tab + Assert.AreEqual(firstFileName, secondFileName); + Assert.AreEqual("master.dbo.testTable.sql", firstFileName); } /// From 30efee5886a0c93122600ac1bb6a650e9d60a3d4 Mon Sep 17 00:00:00 2001 From: Aasim Khan Date: Tue, 1 Sep 2026 23:25:31 -0700 Subject: [PATCH 2/3] Enhance Peek Definition file naming to handle Windows reserved device names and improve script generation logic --- .../Scripting/PeekDefinitionFileNames.cs | 33 ++++++++++++++++--- .../Scripting/ScripterCore.cs | 19 +++++------ .../PeekDefinitionFileNameTests.cs | 21 ++++++++++++ 3 files changed, 59 insertions(+), 14 deletions(-) diff --git a/src/Microsoft.SqlTools.LanguageService/Scripting/PeekDefinitionFileNames.cs b/src/Microsoft.SqlTools.LanguageService/Scripting/PeekDefinitionFileNames.cs index ca9df779a2..b016824ffa 100644 --- a/src/Microsoft.SqlTools.LanguageService/Scripting/PeekDefinitionFileNames.cs +++ b/src/Microsoft.SqlTools.LanguageService/Scripting/PeekDefinitionFileNames.cs @@ -25,6 +25,18 @@ internal static class PeekDefinitionFileNames private static readonly object SyncRoot = new object(); + /// + /// Device names that Windows reserves even when they have a file extension. + /// See https://learn.microsoft.com/windows/win32/fileio/naming-a-file. + /// + private static readonly HashSet WindowsReservedDeviceNames = + new HashSet(StringComparer.OrdinalIgnoreCase) + { + "CON", "PRN", "AUX", "NUL", + "COM1", "COM2", "COM3", "COM4", "COM5", "COM6", "COM7", "COM8", "COM9", + "LPT1", "LPT2", "LPT3", "LPT4", "LPT5", "LPT6", "LPT7", "LPT8", "LPT9" + }; + /// Object identity to the file name assigned to it. private static readonly Dictionary NamesByIdentity = new Dictionary(StringComparer.Ordinal); @@ -94,9 +106,9 @@ private static void AppendPart(StringBuilder builder, string part) } /// - /// Replaces the characters that are legal in a quoted SQL identifier but not in a file - /// name. Two names that differ only in those characters collapse onto the same base name - /// and are then separated by the numeric suffix, so no definition is ever lost. + /// Replaces characters that are legal in a quoted SQL identifier but not in a file name, + /// and prefixes names reserved by Windows. Names that sanitize to the same base name are + /// separated by the numeric suffix, so no definition is ever lost. /// internal static string SanitizeBaseName(string baseName) { @@ -107,7 +119,20 @@ internal static string SanitizeBaseName(string baseName) sanitized.Append(Array.IndexOf(invalidCharacters, character) >= 0 ? '_' : character); } - return sanitized.ToString(); + string sanitizedBaseName = sanitized.ToString(); + int firstPeriod = sanitizedBaseName.IndexOf('.'); + string firstNamePart = firstPeriod >= 0 + ? sanitizedBaseName.Substring(0, firstPeriod) + : sanitizedBaseName; + + // Windows applies device-name rules to the portion before the first period, even when + // the complete file name has an extension (for example, CON.sql). + if (WindowsReservedDeviceNames.Contains(firstNamePart.TrimEnd(' ', '.'))) + { + sanitizedBaseName = "_" + sanitizedBaseName; + } + + return sanitizedBaseName; } /// diff --git a/src/Microsoft.SqlTools.LanguageService/Scripting/ScripterCore.cs b/src/Microsoft.SqlTools.LanguageService/Scripting/ScripterCore.cs index fceb63bf55..31de9d7626 100644 --- a/src/Microsoft.SqlTools.LanguageService/Scripting/ScripterCore.cs +++ b/src/Microsoft.SqlTools.LanguageService/Scripting/ScripterCore.cs @@ -305,7 +305,9 @@ internal Location[] GetSqlObjectDefinition( { SmoScriptingOperation operation = InitScriptOperation(identifier, objectType); operation.Execute(); - string script = operation.ScriptText; + // A missing object produces no script. Treat that the same as the empty file written + // by the previous implementation so the caller can return an empty location list. + string script = operation.ScriptText ?? string.Empty; // script file destination, resolved the same way the scripting operation resolves the // database so that the file name always describes the definition it holds @@ -368,20 +370,17 @@ internal static string CreateFileName( string serverName, string databaseName) { - string baseFileName; - + List nameParts = new List(); if (!string.IsNullOrEmpty(databaseName)) { - baseFileName = $"{databaseName}.{identifier.SchemaName}.{identifier.ObjectName}"; - } - else if (identifier.SchemaName != null) - { - baseFileName = $"{identifier.SchemaName}.{identifier.ObjectName}"; + nameParts.Add(databaseName); } - else + if (!string.IsNullOrEmpty(identifier.SchemaName)) { - baseFileName = identifier.ObjectName; + nameParts.Add(identifier.SchemaName); } + nameParts.Add(identifier.ObjectName); + string baseFileName = string.Join(".", nameParts); string identity = PeekDefinitionFileNames.CreateIdentity( serverName, diff --git a/test/Microsoft.SqlTools.ServiceLayer.UnitTests/LanguageServer/PeekDefinitionFileNameTests.cs b/test/Microsoft.SqlTools.ServiceLayer.UnitTests/LanguageServer/PeekDefinitionFileNameTests.cs index 11d8318c37..be365eb819 100644 --- a/test/Microsoft.SqlTools.ServiceLayer.UnitTests/LanguageServer/PeekDefinitionFileNameTests.cs +++ b/test/Microsoft.SqlTools.ServiceLayer.UnitTests/LanguageServer/PeekDefinitionFileNameTests.cs @@ -162,6 +162,17 @@ public void RequestWithoutASchemaOmitsTheSchemaSegment() Assert.AreEqual("myTable.sql", name); } + [Test] + public void RequestWithADatabaseButWithoutASchemaOmitsTheSchemaSegment() + { + string name = Scripter.CreateFileName( + Identifier("master", null, "myTable"), + ServerA, + "master"); + + Assert.AreEqual("master.myTable.sql", name); + } + [Test] public void ObjectsDifferingOnlyByCaseGetSeparateFiles() { @@ -183,6 +194,16 @@ public void CharactersThatAreIllegalInAFileNameAreReplaced() Assert.AreEqual("master.dbo.odd_name_here.sql", name); } + [TestCase("CON")] + [TestCase("COM1")] + [TestCase("LPT9")] + public void WindowsReservedDeviceNamesArePrefixed(string objectName) + { + string name = NameFor(ServerA, null, null, objectName); + + Assert.AreEqual($"_{objectName}.sql", name); + } + [Test] public void ObjectsThatSanitizeToTheSameNameStayApart() { From 728c4c3fe493f6de699972c27ba494954e34dc95 Mon Sep 17 00:00:00 2001 From: Aasim Khan Date: Tue, 1 Sep 2026 23:38:52 -0700 Subject: [PATCH 3/3] Preserve concise Peek Definition file names --- .../Scripting/ScripterCore.cs | 14 ++++++++------ .../LanguageServer/PeekDefinitionFileNameTests.cs | 11 ++++++----- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/src/Microsoft.SqlTools.LanguageService/Scripting/ScripterCore.cs b/src/Microsoft.SqlTools.LanguageService/Scripting/ScripterCore.cs index 31de9d7626..828cc043cf 100644 --- a/src/Microsoft.SqlTools.LanguageService/Scripting/ScripterCore.cs +++ b/src/Microsoft.SqlTools.LanguageService/Scripting/ScripterCore.cs @@ -363,17 +363,19 @@ internal Location[] GetSqlObjectDefinition( /// The object being scripted. /// The server the object was resolved against, used only to tell /// apart objects that share a fully qualified name across connections. - /// The resolved database name, which may differ from the one on - /// when the request did not qualify it. + /// The resolved database name, used for identity even + /// when the request did not qualify the object. internal static string CreateFileName( Sql3PartIdentifier identifier, string serverName, - string databaseName) + string resolvedDatabaseName) { List nameParts = new List(); - if (!string.IsNullOrEmpty(databaseName)) + // Preserve the previous display name: include the database only when the request + // explicitly qualified it. The resolved database is still part of the identity below. + if (!string.IsNullOrEmpty(identifier.DatabaseName)) { - nameParts.Add(databaseName); + nameParts.Add(identifier.DatabaseName); } if (!string.IsNullOrEmpty(identifier.SchemaName)) { @@ -384,7 +386,7 @@ internal static string CreateFileName( string identity = PeekDefinitionFileNames.CreateIdentity( serverName, - databaseName, + resolvedDatabaseName, identifier.SchemaName, identifier.ObjectName); diff --git a/test/Microsoft.SqlTools.ServiceLayer.UnitTests/LanguageServer/PeekDefinitionFileNameTests.cs b/test/Microsoft.SqlTools.ServiceLayer.UnitTests/LanguageServer/PeekDefinitionFileNameTests.cs index be365eb819..11b0b75ee3 100644 --- a/test/Microsoft.SqlTools.ServiceLayer.UnitTests/LanguageServer/PeekDefinitionFileNameTests.cs +++ b/test/Microsoft.SqlTools.ServiceLayer.UnitTests/LanguageServer/PeekDefinitionFileNameTests.cs @@ -133,17 +133,18 @@ public void EachAdditionalServerTakesTheNextNumber() } [Test] - public void UnqualifiedRequestUsesTheResolvedDatabase() + public void UnqualifiedRequestsInDifferentDatabasesGetSeparateShortNames() { - // The request did not name a database; the caller resolves it from the connection so - // that the file name matches the definition actually scripted Sql3PartIdentifier unqualified = Identifier(null, "dbo", "myTable"); string resolvedToOne = Scripter.CreateFileName(unqualified, ServerA, "dbOne"); string resolvedToTwo = Scripter.CreateFileName(unqualified, ServerA, "dbTwo"); - Assert.AreEqual("dbOne.dbo.myTable.sql", resolvedToOne); - Assert.AreEqual("dbTwo.dbo.myTable.sql", resolvedToTwo); + Assert.AreEqual("dbo.myTable.sql", resolvedToOne); + Assert.AreEqual("dbo.myTable_2.sql", resolvedToTwo); + Assert.AreEqual( + resolvedToOne, + Scripter.CreateFileName(unqualified, ServerA, "dbOne")); } [Test]