diff --git a/src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/PackageSourceTelemetry.cs b/src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/PackageSourceTelemetry.cs index bdc107c6b6e..2c5c672ce21 100644 --- a/src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/PackageSourceTelemetry.cs +++ b/src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/PackageSourceTelemetry.cs @@ -8,6 +8,7 @@ using System.Collections.Generic; using System.Globalization; using System.Linq; + using System.Threading; using System.Threading.Tasks; using NuGet.Common; @@ -203,6 +204,19 @@ internal static void AddNupkgCopiedData(ProtocolDiagnosticNupkgCopiedEvent ncEve { data.NupkgCount++; data.NupkgSize += ncEvent.FileSize; + data.IdContainsNonAsciiCharacter = data.IdContainsNonAsciiCharacter || (ncEvent.PackageId != null && HasNonASCIICharacters(ncEvent.PackageId)); + } + + bool HasNonASCIICharacters(string packageId) + { + foreach (char c in packageId.AsSpan()) + { + if (!((c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z') || (c >= '0' && c <= '9') || c == '.' || c == '-')) + { + return true; + } + } + return false; } } @@ -261,6 +275,7 @@ internal static async Task ToTelemetryAsync(Data data, SourceRep telemetry[PropertyNames.Duration.Total] = data.Resources.Values.Sum(r => r.duration.TotalMilliseconds); telemetry[PropertyNames.Nupkgs.Copied] = data.NupkgCount; telemetry[PropertyNames.Nupkgs.Bytes] = data.NupkgSize; + telemetry[PropertyNames.Nupkgs.IdContainsNonAsciiCharacter] = data.IdContainsNonAsciiCharacter; AddResourceProperties(telemetry, data.Resources); if (data.Http.Requests > 0) @@ -426,6 +441,7 @@ internal class Data internal HttpData Http { get; } internal int NupkgCount { get; set; } internal long NupkgSize { get; set; } + internal bool IdContainsNonAsciiCharacter { get; set; } internal Data() { @@ -475,6 +491,7 @@ internal static class Nupkgs { internal const string Copied = "nupkgs.copied"; internal const string Bytes = "nupkgs.bytes"; + internal const string IdContainsNonAsciiCharacter = "nupkgs.idcontainsnonasciicharacter"; } internal static class Resources diff --git a/src/NuGet.Core/NuGet.Commands/RestoreCommand/RestoreCommand.cs b/src/NuGet.Core/NuGet.Commands/RestoreCommand/RestoreCommand.cs index 13df4d863f0..dae776327a1 100644 --- a/src/NuGet.Core/NuGet.Commands/RestoreCommand/RestoreCommand.cs +++ b/src/NuGet.Core/NuGet.Commands/RestoreCommand/RestoreCommand.cs @@ -54,6 +54,7 @@ private readonly Dictionary } telemetry.TelemetryEvent[NewPackagesInstalledCount] = graphs.Where(g => !g.InConflict).SelectMany(g => g.Install).Distinct().Count(); + telemetry.TelemetryEvent[AnyPackageIdContainsNonASCIICharacters] = graphs.Where(g => !g.InConflict).SelectMany(g => g.Flattened).Any(i => HasNonASCIICharacters(i.Key.Name)); telemetry.TelemetryEvent[RestoreSuccess] = success; } @@ -753,6 +755,23 @@ private async Task packagesLockFile, packagesLockFilePath, cacheFile); + + bool HasNonASCIICharacters(string packageId) + { + if (string.IsNullOrWhiteSpace(packageId)) + { + return false; + } + + foreach (char c in packageId.AsSpan()) + { + if (!((c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z') || (c >= '0' && c <= '9') || c == '.' || c == '-')) + { + return true; + } + } + return false; + } } /// Run NuGetAudit on the project's resolved restore graphs, and log messages and telemetry with the results. diff --git a/src/NuGet.Core/NuGet.Protocol/Events/ProtocolDiagnosticNupkgCopiedEvent.cs b/src/NuGet.Core/NuGet.Protocol/Events/ProtocolDiagnosticNupkgCopiedEvent.cs index d42ea8a49d5..570ae205aa7 100644 --- a/src/NuGet.Core/NuGet.Protocol/Events/ProtocolDiagnosticNupkgCopiedEvent.cs +++ b/src/NuGet.Core/NuGet.Protocol/Events/ProtocolDiagnosticNupkgCopiedEvent.cs @@ -10,12 +10,26 @@ public sealed class ProtocolDiagnosticNupkgCopiedEvent public string Source { get; } public long FileSize { get; } + /// + /// Gets the package ID of the copied nupkg, or if not available. + /// + public string PackageId { get; } + public ProtocolDiagnosticNupkgCopiedEvent( string source, long fileSize) + : this(source, fileSize, packageId: null) + { + } + + public ProtocolDiagnosticNupkgCopiedEvent( + string source, + long fileSize, + string packageId) { Source = source; FileSize = fileSize; + PackageId = packageId; } } } diff --git a/src/NuGet.Core/NuGet.Protocol/LocalPackageArchiveDownloader.cs b/src/NuGet.Core/NuGet.Protocol/LocalPackageArchiveDownloader.cs index 0ca015f1efe..ca249de6c6c 100644 --- a/src/NuGet.Core/NuGet.Protocol/LocalPackageArchiveDownloader.cs +++ b/src/NuGet.Core/NuGet.Protocol/LocalPackageArchiveDownloader.cs @@ -189,7 +189,7 @@ public async Task CopyNupkgFileToAsync(string destinationFilePath, Cancell await source.CopyToAsync(destination, bufferSize, cancellationToken); - ProtocolDiagnostics.RaiseEvent(new ProtocolDiagnosticNupkgCopiedEvent(Source, destination.Length)); + ProtocolDiagnostics.RaiseEvent(new ProtocolDiagnosticNupkgCopiedEvent(Source, destination.Length, _packageIdentity.Id)); return true; } diff --git a/src/NuGet.Core/NuGet.Protocol/LocalRepositories/LocalV2FindPackageByIdResource.cs b/src/NuGet.Core/NuGet.Protocol/LocalRepositories/LocalV2FindPackageByIdResource.cs index 33b1af863c1..e2cb9bc145a 100644 --- a/src/NuGet.Core/NuGet.Protocol/LocalRepositories/LocalV2FindPackageByIdResource.cs +++ b/src/NuGet.Core/NuGet.Protocol/LocalRepositories/LocalV2FindPackageByIdResource.cs @@ -176,7 +176,7 @@ public override async Task CopyNupkgToStreamAsync( using (var fileStream = File.OpenRead(info.Path)) { await fileStream.CopyToAsync(destination, cancellationToken); - ProtocolDiagnostics.RaiseEvent(new ProtocolDiagnosticNupkgCopiedEvent(_source, destination.Length)); + ProtocolDiagnostics.RaiseEvent(new ProtocolDiagnosticNupkgCopiedEvent(_source, destination.Length, id)); return true; } } diff --git a/src/NuGet.Core/NuGet.Protocol/LocalRepositories/LocalV3FindPackageByIdResource.cs b/src/NuGet.Core/NuGet.Protocol/LocalRepositories/LocalV3FindPackageByIdResource.cs index 7fbf0e3f90b..d477af8a3a3 100644 --- a/src/NuGet.Core/NuGet.Protocol/LocalRepositories/LocalV3FindPackageByIdResource.cs +++ b/src/NuGet.Core/NuGet.Protocol/LocalRepositories/LocalV3FindPackageByIdResource.cs @@ -210,7 +210,7 @@ public override async Task CopyNupkgToStreamAsync( using (var fileStream = File.OpenRead(packagePath)) { await fileStream.CopyToAsync(destination, cancellationToken); - ProtocolDiagnostics.RaiseEvent(new ProtocolDiagnosticNupkgCopiedEvent(_source, destination.Length)); + ProtocolDiagnostics.RaiseEvent(new ProtocolDiagnosticNupkgCopiedEvent(_source, destination.Length, id)); return true; } } diff --git a/src/NuGet.Core/NuGet.Protocol/PublicAPI/net472/PublicAPI.Unshipped.txt b/src/NuGet.Core/NuGet.Protocol/PublicAPI/net472/PublicAPI.Unshipped.txt index 7dc5c58110b..938c0d98a7d 100644 --- a/src/NuGet.Core/NuGet.Protocol/PublicAPI/net472/PublicAPI.Unshipped.txt +++ b/src/NuGet.Core/NuGet.Protocol/PublicAPI/net472/PublicAPI.Unshipped.txt @@ -1 +1,3 @@ #nullable enable +~NuGet.Protocol.Events.ProtocolDiagnosticNupkgCopiedEvent.PackageId.get -> string +~NuGet.Protocol.Events.ProtocolDiagnosticNupkgCopiedEvent.ProtocolDiagnosticNupkgCopiedEvent(string source, long fileSize, string packageId) -> void diff --git a/src/NuGet.Core/NuGet.Protocol/PublicAPI/net8.0/PublicAPI.Unshipped.txt b/src/NuGet.Core/NuGet.Protocol/PublicAPI/net8.0/PublicAPI.Unshipped.txt index 7dc5c58110b..938c0d98a7d 100644 --- a/src/NuGet.Core/NuGet.Protocol/PublicAPI/net8.0/PublicAPI.Unshipped.txt +++ b/src/NuGet.Core/NuGet.Protocol/PublicAPI/net8.0/PublicAPI.Unshipped.txt @@ -1 +1,3 @@ #nullable enable +~NuGet.Protocol.Events.ProtocolDiagnosticNupkgCopiedEvent.PackageId.get -> string +~NuGet.Protocol.Events.ProtocolDiagnosticNupkgCopiedEvent.ProtocolDiagnosticNupkgCopiedEvent(string source, long fileSize, string packageId) -> void diff --git a/src/NuGet.Core/NuGet.Protocol/Utility/FindPackagesByIdNupkgDownloader.cs b/src/NuGet.Core/NuGet.Protocol/Utility/FindPackagesByIdNupkgDownloader.cs index f87e02f7fbd..f80def2671a 100644 --- a/src/NuGet.Core/NuGet.Protocol/Utility/FindPackagesByIdNupkgDownloader.cs +++ b/src/NuGet.Core/NuGet.Protocol/Utility/FindPackagesByIdNupkgDownloader.cs @@ -137,7 +137,7 @@ public async Task CopyNupkgToStreamAsync( try { await stream.CopyToAsync(destination, token); - ProtocolDiagnostics.RaiseEvent(new ProtocolDiagnosticNupkgCopiedEvent(_httpSource.PackageSource, destination.Length)); + ProtocolDiagnostics.RaiseEvent(new ProtocolDiagnosticNupkgCopiedEvent(_httpSource.PackageSource, destination.Length, identity.Id)); } catch when (!token.IsCancellationRequested) { diff --git a/test/NuGet.Clients.Tests/NuGet.VisualStudio.Common.Test/Telemetry/PackageSourceTelemetryTests.cs b/test/NuGet.Clients.Tests/NuGet.VisualStudio.Common.Test/Telemetry/PackageSourceTelemetryTests.cs index 2a0ffc920dc..7360bfe4ee9 100644 --- a/test/NuGet.Clients.Tests/NuGet.VisualStudio.Common.Test/Telemetry/PackageSourceTelemetryTests.cs +++ b/test/NuGet.Clients.Tests/NuGet.VisualStudio.Common.Test/Telemetry/PackageSourceTelemetryTests.cs @@ -137,6 +137,79 @@ public void AddNupkgCopiedData_MultipleEvents_AccumulatesCorrectly() Assert.Equal(sizes.Sum(), result.NupkgSize); } + [Theory] + [InlineData("Newtonsoft.Json")] + [InlineData("NuGet.Protocol")] + [InlineData("My-Package.1")] + [InlineData("ALLCAPS")] + [InlineData("alllower")] + [InlineData("123Numeric")] + [InlineData("a")] + public void AddNupkgCopiedData_StandardPackageId_IdContainsNonAsciiCharacterIsFalse(string packageId) + { + // Arrange + var data = CreateDataDictionary(SampleSource); + var nce = new ProtocolDiagnosticNupkgCopiedEvent(SampleSource, fileSize: 1000, packageId); + + // Act + PackageSourceTelemetry.AddNupkgCopiedData(nce, data); + + // Assert + var result = Assert.Single(data).Value; + Assert.False(result.IdContainsNonAsciiCharacter); + } + + [Theory] + [InlineData("My_Package")] + [InlineData("Package@1.0")] + [InlineData("Ünïcödé")] + [InlineData("Package Name")] + [InlineData("package+extra")] + public void AddNupkgCopiedData_NonstandardPackageId_IdContainsNonAsciiCharacterIsTrue(string packageId) + { + // Arrange + var data = CreateDataDictionary(SampleSource); + var nce = new ProtocolDiagnosticNupkgCopiedEvent(SampleSource, fileSize: 1000, packageId); + + // Act + PackageSourceTelemetry.AddNupkgCopiedData(nce, data); + + // Assert + var result = Assert.Single(data).Value; + Assert.True(result.IdContainsNonAsciiCharacter); + } + + [Fact] + public void AddNupkgCopiedData_MultiplePackagesOneNonstandard_IdContainsNonAsciiCharacterIsTrue() + { + // Arrange + var data = CreateDataDictionary(SampleSource); + + // Act + PackageSourceTelemetry.AddNupkgCopiedData(new ProtocolDiagnosticNupkgCopiedEvent(SampleSource, fileSize: 1000, "Standard.Package"), data); + PackageSourceTelemetry.AddNupkgCopiedData(new ProtocolDiagnosticNupkgCopiedEvent(SampleSource, fileSize: 1000, "Nonstandard_Package"), data); + PackageSourceTelemetry.AddNupkgCopiedData(new ProtocolDiagnosticNupkgCopiedEvent(SampleSource, fileSize: 1000, "Another.Standard"), data); + + // Assert + var result = Assert.Single(data).Value; + Assert.True(result.IdContainsNonAsciiCharacter); + } + + [Fact] + public void AddNupkgCopiedData_NullPackageId_IdContainsNonAsciiCharacterIsFalse() + { + // Arrange + var data = CreateDataDictionary(SampleSource); + var nce = new ProtocolDiagnosticNupkgCopiedEvent(SampleSource, fileSize: 1000); + + // Act + PackageSourceTelemetry.AddNupkgCopiedData(nce, data); + + // Assert + var result = Assert.Single(data).Value; + Assert.False(result.IdContainsNonAsciiCharacter); + } + [Fact] public async Task AddData_IsThreadSafe() { @@ -308,6 +381,7 @@ public async Task ToTelemetry_WithData_CreatesTelemetryProperties(string package Assert.Equal(data.NupkgCount, result[PackageSourceTelemetry.PropertyNames.Nupkgs.Copied]); Assert.Equal(data.NupkgSize, result[PackageSourceTelemetry.PropertyNames.Nupkgs.Bytes]); + Assert.Equal(data.IdContainsNonAsciiCharacter, result[PackageSourceTelemetry.PropertyNames.Nupkgs.IdContainsNonAsciiCharacter]); Assert.Equal(data.Resources.Sum(r => r.Value.count), result[PackageSourceTelemetry.PropertyNames.Resources.Calls]); foreach (var resource in data.Resources) diff --git a/test/NuGet.Core.Tests/NuGet.Commands.Test/RestoreCommandTests/RestoreCommandTests.cs b/test/NuGet.Core.Tests/NuGet.Commands.Test/RestoreCommandTests/RestoreCommandTests.cs index d3d7c627af7..29688ab1b25 100644 --- a/test/NuGet.Core.Tests/NuGet.Commands.Test/RestoreCommandTests/RestoreCommandTests.cs +++ b/test/NuGet.Core.Tests/NuGet.Commands.Test/RestoreCommandTests/RestoreCommandTests.cs @@ -2957,6 +2957,7 @@ await SimpleTestPackageUtility.CreateFolderFeedV3Async( ["NoOpDuration"] = value => value.Should().NotBeNull(), ["TotalUniquePackagesCount"] = value => value.Should().Be(1), ["NewPackagesInstalledCount"] = value => value.Should().Be(1), + ["AnyPackageIdContainsNonASCIICharacters"] = value => value.Should().Be(false), ["EvaluateLockFileDuration"] = value => value.Should().NotBeNull(), ["CreateRestoreTargetGraphDuration"] = value => value.Should().NotBeNull(), ["GenerateRestoreGraphDuration"] = value => value.Should().NotBeNull(), @@ -3235,7 +3236,7 @@ await SimpleTestPackageUtility.CreateFolderFeedV3Async( var projectInformationEvent = telemetryEvents.Single(e => e.Name.Equals("ProjectRestoreInformation")); - projectInformationEvent.Count.Should().Be(49); + projectInformationEvent.Count.Should().Be(50); projectInformationEvent["RestoreSuccess"].Should().Be(true); projectInformationEvent["NoOpResult"].Should().Be(false); projectInformationEvent["TotalUniquePackagesCount"].Should().Be(2); @@ -3644,6 +3645,139 @@ private static PackageSpec CreatePackageSpec(List tf return packageSpec; } + [Fact] + public async Task ExecuteAsync_WithASCIIPackageId_AnyPackageIdContainsNonASCIICharactersIsFalse() + { + // Arrange + using var pathContext = new SimpleTestPathContext(); + var projectName = "TestProject"; + var projectPath = Path.Combine(pathContext.SolutionRoot, projectName); + PackageSpec packageSpec = ProjectTestHelpers.GetPackageSpec(projectName, pathContext.SolutionRoot, "net472", "My.Package1"); + + await SimpleTestPackageUtility.CreateFolderFeedV3Async( + pathContext.PackageSource, + PackageSaveMode.Defaultv3, + new SimpleTestPackageContext("My.Package1", "1.0.0")); + var logger = new TestLogger(); + + var request = new TestRestoreRequest(packageSpec, new PackageSource[] { new PackageSource(pathContext.PackageSource) }, pathContext.UserPackagesFolder, logger) + { + LockFilePath = Path.Combine(projectPath, "project.assets.json"), + ProjectStyle = ProjectStyle.PackageReference, + }; + + // Set-up telemetry service - Important to set-up the service *after* the package source creation call as that emits telemetry! + var telemetryEvents = new ConcurrentQueue(); + var _telemetryService = new Mock(MockBehavior.Loose); + _telemetryService + .Setup(x => x.EmitTelemetryEvent(It.IsAny())) + .Callback(x => telemetryEvents.Enqueue(x)); + + TelemetryActivity.NuGetTelemetryService = _telemetryService.Object; + + // Act + var restoreCommand = new RestoreCommand(request); + RestoreResult result = await restoreCommand.ExecuteAsync(); + + // Assert + result.Success.Should().BeTrue(because: logger.ShowMessages()); + var projectInformationEvent = telemetryEvents.Single(e => e.Name.Equals("ProjectRestoreInformation")); + projectInformationEvent["AnyPackageIdContainsNonASCIICharacters"].Should().Be(false); + } + + [Theory] + [InlineData("My_Package")] // underscore + [InlineData("Pac\u212Bage")] // Kelvin sign K (U+212A) + [InlineData("Package\u03B1")] // Greek lowercase alpha (U+03B1) + [InlineData("Package\u00E9")] // Latin small letter e with acute (U+00E9) + [InlineData("\u0410.Package")] // Cyrillic capital A (U+0410) + public async Task ExecuteAsync_WithNonASCIIPackageId_AnyPackageIdContainsNonASCIICharactersIsTrue(string packageId) + { + // Arrange + using var pathContext = new SimpleTestPathContext(); + var projectName = "TestProject"; + var projectPath = Path.Combine(pathContext.SolutionRoot, projectName); + PackageSpec packageSpec = ProjectTestHelpers.GetPackageSpec(projectName, pathContext.SolutionRoot, "net472", packageId); + + await SimpleTestPackageUtility.CreateFolderFeedV3Async( + pathContext.PackageSource, + PackageSaveMode.Defaultv3, + new SimpleTestPackageContext(packageId, "1.0.0")); + var logger = new TestLogger(); + + var request = new TestRestoreRequest(packageSpec, new PackageSource[] { new PackageSource(pathContext.PackageSource) }, pathContext.UserPackagesFolder, logger) + { + LockFilePath = Path.Combine(projectPath, "project.assets.json"), + ProjectStyle = ProjectStyle.PackageReference, + }; + + // Set-up telemetry service - Important to set-up the service *after* the package source creation call as that emits telemetry! + var telemetryEvents = new ConcurrentQueue(); + var _telemetryService = new Mock(MockBehavior.Loose); + _telemetryService + .Setup(x => x.EmitTelemetryEvent(It.IsAny())) + .Callback(x => telemetryEvents.Enqueue(x)); + + TelemetryActivity.NuGetTelemetryService = _telemetryService.Object; + + // Act + var restoreCommand = new RestoreCommand(request); + RestoreResult result = await restoreCommand.ExecuteAsync(); + + // Assert + result.Success.Should().BeTrue(because: logger.ShowMessages()); + var projectInformationEvent = telemetryEvents.Single(e => e.Name.Equals("ProjectRestoreInformation")); + projectInformationEvent["AnyPackageIdContainsNonASCIICharacters"].Should().Be(true); + } + + [Theory] + [InlineData("My_Package")] // underscore + [InlineData("Pac\u212Bage")] // Kelvin sign K (U+212A) + [InlineData("Package\u03B1")] // Greek lowercase alpha (U+03B1) + public async Task ExecuteAsync_WithMixedPackageIds_AnyPackageIdContainsNonASCIICharactersIsTrue(string packageId) + { + // Arrange + using var pathContext = new SimpleTestPathContext(); + var projectName = "TestProject"; + var projectPath = Path.Combine(pathContext.SolutionRoot, projectName); + var asciiOnlyPkg = new SimpleTestPackageContext("Some.Package", "1.0.0"); + var nonASCIIPkg = new SimpleTestPackageContext(packageId, "1.0.0"); + asciiOnlyPkg.Dependencies.Add(nonASCIIPkg); + + await SimpleTestPackageUtility.CreateFolderFeedV3Async( + pathContext.PackageSource, + PackageSaveMode.Defaultv3, + asciiOnlyPkg, + nonASCIIPkg); + + PackageSpec packageSpec = ProjectTestHelpers.GetPackageSpec(projectName, pathContext.SolutionRoot, "net472", "Some.Package"); + var logger = new TestLogger(); + + var request = new TestRestoreRequest(packageSpec, new PackageSource[] { new PackageSource(pathContext.PackageSource) }, pathContext.UserPackagesFolder, logger) + { + LockFilePath = Path.Combine(projectPath, "project.assets.json"), + ProjectStyle = ProjectStyle.PackageReference, + }; + + // Set-up telemetry service - Important to set-up the service *after* the package source creation call as that emits telemetry! + var telemetryEvents = new ConcurrentQueue(); + var _telemetryService = new Mock(MockBehavior.Loose); + _telemetryService + .Setup(x => x.EmitTelemetryEvent(It.IsAny())) + .Callback(x => telemetryEvents.Enqueue(x)); + + TelemetryActivity.NuGetTelemetryService = _telemetryService.Object; + + // Act + var restoreCommand = new RestoreCommand(request); + RestoreResult result = await restoreCommand.ExecuteAsync(); + + // Assert + result.Success.Should().BeTrue(because: logger.ShowMessages()); + var projectInformationEvent = telemetryEvents.Single(e => e.Name.Equals("ProjectRestoreInformation")); + projectInformationEvent["AnyPackageIdContainsNonASCIICharacters"].Should().Be(true); + } + private Task> DoWalkAsync(RemoteDependencyWalker walker, string name, NuGetFramework framework) { var range = new LibraryRange