diff --git a/src/Microsoft.SqlTools.LanguageService/Scripting/PeekDefinitionFileNames.cs b/src/Microsoft.SqlTools.LanguageService/Scripting/PeekDefinitionFileNames.cs new file mode 100644 index 0000000000..b016824ffa --- /dev/null +++ b/src/Microsoft.SqlTools.LanguageService/Scripting/PeekDefinitionFileNames.cs @@ -0,0 +1,150 @@ +// +// 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(); + + /// + /// 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); + + /// + /// 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 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) + { + char[] invalidCharacters = Path.GetInvalidFileNameChars(); + StringBuilder sanitized = new StringBuilder(baseName.Length); + foreach (char character in baseName) + { + sanitized.Append(Array.IndexOf(invalidCharacters, character) >= 0 ? '_' : character); + } + + 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; + } + + /// + /// 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..828cc043cf 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,29 @@ 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; + // 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 + 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 +355,63 @@ 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, used for identity even + /// when the request did not qualify the object. + internal static string CreateFileName( + Sql3PartIdentifier identifier, + string serverName, + string resolvedDatabaseName) { - string baseFileName; - - if (identifier.DatabaseName != null) + List nameParts = new List(); + // 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)) { - baseFileName = $"{identifier.DatabaseName}.{identifier.SchemaName}.{identifier.ObjectName}.sql"; + nameParts.Add(identifier.DatabaseName); } - else if (identifier.SchemaName != null) + if (!string.IsNullOrEmpty(identifier.SchemaName)) { - baseFileName = $"{identifier.SchemaName}.{identifier.ObjectName}.sql"; + nameParts.Add(identifier.SchemaName); } - else + nameParts.Add(identifier.ObjectName); + string baseFileName = string.Join(".", nameParts); + + string identity = PeekDefinitionFileNames.CreateIdentity( + serverName, + resolvedDatabaseName, + 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) { - baseFileName = $"{identifier.ObjectName}.sql"; - } + if (File.Exists(path) + && string.Equals(File.ReadAllText(path), script, StringComparison.Ordinal)) + { + return; + } - return $"{Path.GetFileNameWithoutExtension(baseFileName)}_{Guid.NewGuid():N}{Path.GetExtension(baseFileName)}"; + 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..11b0b75ee3 --- /dev/null +++ b/test/Microsoft.SqlTools.ServiceLayer.UnitTests/LanguageServer/PeekDefinitionFileNameTests.cs @@ -0,0 +1,343 @@ +// +// 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 UnqualifiedRequestsInDifferentDatabasesGetSeparateShortNames() + { + Sql3PartIdentifier unqualified = Identifier(null, "dbo", "myTable"); + + string resolvedToOne = Scripter.CreateFileName(unqualified, ServerA, "dbOne"); + string resolvedToTwo = Scripter.CreateFileName(unqualified, ServerA, "dbTwo"); + + Assert.AreEqual("dbo.myTable.sql", resolvedToOne); + Assert.AreEqual("dbo.myTable_2.sql", resolvedToTwo); + Assert.AreEqual( + resolvedToOne, + Scripter.CreateFileName(unqualified, ServerA, "dbOne")); + } + + [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 RequestWithADatabaseButWithoutASchemaOmitsTheSchemaSegment() + { + string name = Scripter.CreateFileName( + Identifier("master", null, "myTable"), + ServerA, + "master"); + + Assert.AreEqual("master.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); + } + + [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() + { + // "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); } ///