From 775ebbcff4d585cb9e75608d41d711bb306e0e24 Mon Sep 17 00:00:00 2001 From: Donnie Goodson <49205731+donnie-msft@users.noreply.github.com> Date: Fri, 7 Jan 2022 18:50:30 -0800 Subject: [PATCH] Push with -SkipDuplicate now skips automatic symbol pushes when nupkg is duplicate (#4373) * Push now does N1,S1,N2,S2 so snupkg can be skipped when nupkg is duplicate * Plumb in getSymbolApiKey * Address feedback --- .../Resources/PackageUpdateResource.cs | 155 +++++++++++------- .../Utility/LocalFolderUtility.cs | 7 +- .../Commands/PushCommandTest.cs | 14 +- .../NuGetPushCommandTest.cs | 2 +- 4 files changed, 111 insertions(+), 67 deletions(-) diff --git a/src/NuGet.Core/NuGet.Protocol/Resources/PackageUpdateResource.cs b/src/NuGet.Core/NuGet.Protocol/Resources/PackageUpdateResource.cs index 40c3bb9ff5c..5761208dda9 100644 --- a/src/NuGet.Core/NuGet.Protocol/Resources/PackageUpdateResource.cs +++ b/src/NuGet.Core/NuGet.Protocol/Resources/PackageUpdateResource.cs @@ -3,6 +3,8 @@ using System; using System.Collections.Generic; +using System.Data; +using System.Diagnostics; using System.Globalization; using System.IO; using System.Linq; @@ -68,34 +70,31 @@ public async Task Push( // TODO: Figure out how to hook this up with the HTTP request _disableBuffering = disableBuffering; - using (var tokenSource = new CancellationTokenSource()) - { - var requestTimeout = TimeSpan.FromSeconds(timeoutInSecond); - tokenSource.CancelAfter(requestTimeout); - var apiKey = getApiKey(_source); + using var tokenSource = new CancellationTokenSource(); + var requestTimeout = TimeSpan.FromSeconds(timeoutInSecond); + tokenSource.CancelAfter(requestTimeout); + var apiKey = getApiKey(_source); - foreach (var packagePath in packagePaths) + foreach (string packagePath in packagePaths) + { + if (!packagePath.EndsWith(NuGetConstants.SnupkgExtension, StringComparison.OrdinalIgnoreCase)) + { + // Push nupkgs and possibly the corresponding snupkgs. + await PushPackagePath(packagePath, _source, symbolSource, apiKey, getSymbolApiKey, noServiceEndpoint, skipDuplicate, + symbolPackageUpdateResource, requestTimeout, log, tokenSource.Token); + } + else // Explicit snupkg push { - bool explicitSnupkgPush = true; - if (!packagePath.EndsWith(NuGetConstants.SnupkgExtension, StringComparison.OrdinalIgnoreCase)) - { - await PushPackage(packagePath, _source, apiKey, noServiceEndpoint, skipDuplicate, - requestTimeout, log, tokenSource.Token); - - //Since this was not a snupkg push (probably .nupkg), when we try pushing symbols later, don't error if there are no snupkg files found. - explicitSnupkgPush = false; - } - // symbolSource is only set when: // - The user specified it on the command line // - The endpoint for main package supports pushing snupkgs if (!string.IsNullOrEmpty(symbolSource)) { - var symbolApiKey = getSymbolApiKey(symbolSource); + string symbolApiKey = getSymbolApiKey(symbolSource); - await PushSymbols(packagePath, symbolSource, symbolApiKey, + await PushSymbolsPath(packagePath, symbolSource, symbolApiKey, noServiceEndpoint, skipDuplicate, symbolPackageUpdateResource, - requestTimeout, log, explicitSnupkgPush, tokenSource.Token); + requestTimeout, log, explicitSymbolsPush: true, tokenSource.Token); } } } @@ -178,8 +177,8 @@ public async Task Delete(string packageId, } } - private async Task PushSymbols(string packagePath, - string source, + private async Task PushSymbolsPath(string packagePath, + string symbolSource, string apiKey, bool noServiceEndpoint, bool skipDuplicate, @@ -189,12 +188,11 @@ private async Task PushSymbols(string packagePath, bool explicitSymbolsPush, CancellationToken token) { - - var isSymbolEndpointSnupkgCapable = symbolPackageUpdateResource != null; + bool isSymbolEndpointSnupkgCapable = symbolPackageUpdateResource != null; // Get the symbol package for this package - var symbolPackagePath = GetSymbolsPath(packagePath, isSymbolEndpointSnupkgCapable); + string symbolPackagePath = GetSymbolsPath(packagePath, isSymbolEndpointSnupkgCapable); - var symbolsToPush = LocalFolderUtility.ResolvePackageFromPath(symbolPackagePath, isSnupkg: isSymbolEndpointSnupkgCapable); + IEnumerable symbolsToPush = LocalFolderUtility.ResolvePackageFromPath(symbolPackagePath, isSnupkg: isSymbolEndpointSnupkgCapable); bool symbolsPathResolved = symbolsToPush != null && symbolsToPush.Any(); //No files were resolved. @@ -209,10 +207,10 @@ private async Task PushSymbols(string packagePath, } else { - var sourceUri = UriUtility.CreateSourceUri(source); + Uri symbolSourceUri = UriUtility.CreateSourceUri(symbolSource); // See if the api key exists - if (string.IsNullOrEmpty(apiKey) && !sourceUri.IsFile) + if (string.IsNullOrEmpty(apiKey) && !symbolSourceUri.IsFile) { log.LogWarning(string.Format(CultureInfo.CurrentCulture, Strings.Warning_SymbolServerNotConfigured, @@ -220,20 +218,31 @@ private async Task PushSymbols(string packagePath, Strings.DefaultSymbolServer)); } - await PushAll(source, apiKey, noServiceEndpoint, skipDuplicate, requestTimeout, log, packagesToPush: symbolsToPush, token); + foreach (string packageToPush in symbolsToPush) + { + await PushPackageCore(symbolSource, apiKey, packageToPush, noServiceEndpoint, skipDuplicate, requestTimeout, log, token); + } } } - private async Task PushPackage(string packagePath, + /// + /// Push nupkgs, and if successful, push any corresponding symbols. + /// + /// Thrown when any resolved file path does not exist. + private async Task PushPackagePath(string packagePath, string source, + string symbolSource, // empty to not push symbols string apiKey, + Func getSymbolApiKey, bool noServiceEndpoint, bool skipDuplicate, + SymbolPackageUpdateResourceV3 symbolPackageUpdateResource, TimeSpan requestTimeout, ILogger log, CancellationToken token) { - var nupkgsToPush = LocalFolderUtility.ResolvePackageFromPath(packagePath, isSnupkg: false); + IEnumerable nupkgsToPush = LocalFolderUtility.ResolvePackageFromPath(packagePath, isSnupkg: false); + bool alreadyWarnedSymbolServerNotConfigured = false; if (!(nupkgsToPush != null && nupkgsToPush.Any())) { @@ -242,27 +251,54 @@ private async Task PushPackage(string packagePath, packagePath)); } - var sourceUri = UriUtility.CreateSourceUri(source); + Uri packageSourceUri = UriUtility.CreateSourceUri(source); - if (string.IsNullOrEmpty(apiKey) && !sourceUri.IsFile) + if (string.IsNullOrEmpty(apiKey) && !packageSourceUri.IsFile) { log.LogWarning(string.Format(CultureInfo.CurrentCulture, Strings.NoApiKeyFound, GetSourceDisplayName(source))); } - await PushAll(source, apiKey, noServiceEndpoint, skipDuplicate, requestTimeout, log, packagesToPush: nupkgsToPush, token); - } - - private async Task PushAll(string source, string apiKey, bool noServiceEndpoint, bool skipDuplicate, TimeSpan requestTimeout, ILogger log, IEnumerable packagesToPush, CancellationToken token) - { - foreach (var packageToPush in packagesToPush) + foreach (string nupkgToPush in nupkgsToPush) { - await PushPackageCore(source, apiKey, packageToPush, noServiceEndpoint, skipDuplicate, requestTimeout, log, token); + bool packageWasPushed = await PushPackageCore(source, apiKey, nupkgToPush, noServiceEndpoint, skipDuplicate, requestTimeout, log, token); + + // Push corresponding symbols, if successful. + if (packageWasPushed && !string.IsNullOrEmpty(symbolSource)) + { + bool isSymbolEndpointSnupkgCapable = symbolPackageUpdateResource != null; + string symbolPackagePath = GetSymbolsPath(nupkgToPush, isSnupkg: isSymbolEndpointSnupkgCapable); + + // There may not be a snupkg with the same filename. Ignore it since this isn't an explicit snupkg push. + if (!File.Exists(symbolPackagePath)) + { + continue; + } + + if (!alreadyWarnedSymbolServerNotConfigured) + { + Uri symbolSourceUri = UriUtility.CreateSourceUri(symbolSource); + + // See if the api key exists + if (string.IsNullOrEmpty(apiKey) && !symbolSourceUri.IsFile) + { + log.LogWarning(string.Format(CultureInfo.CurrentCulture, + Strings.Warning_SymbolServerNotConfigured, + Path.GetFileName(symbolPackagePath), + Strings.DefaultSymbolServer)); + + alreadyWarnedSymbolServerNotConfigured = true; + } + } + + string symbolApiKey = getSymbolApiKey(symbolSource); + await PushPackageCore(symbolSource, symbolApiKey, symbolPackagePath, noServiceEndpoint, skipDuplicate, requestTimeout, log, token); + } } } - private async Task PushPackageCore(string source, + private async Task PushPackageCore(string source, string apiKey, string packageToPush, bool noServiceEndpoint, @@ -279,7 +315,7 @@ private async Task PushPackageCore(string source, Path.GetFileName(packageToPush), sourceName)); - bool showPushCommandPackagePushed = true; + bool wasPackagePushed = true; if (sourceUri.IsFile) { @@ -288,15 +324,16 @@ private async Task PushPackageCore(string source, else { var length = new FileInfo(packageToPush).Length; - showPushCommandPackagePushed = await PushPackageToServer(source, apiKey, packageToPush, length, noServiceEndpoint, skipDuplicate - , requestTimeout, log, token); - + wasPackagePushed = await PushPackageToServer(source, apiKey, packageToPush, length, noServiceEndpoint, skipDuplicate, + requestTimeout, log, token); } - if (showPushCommandPackagePushed) + if (wasPackagePushed) { log.LogInformation(Strings.PushCommandPackagePushed); } + + return wasPackagePushed; } private static string GetSourceDisplayName(string source) @@ -344,10 +381,10 @@ private async Task PushPackageToServer(string source, ILogger logger, CancellationToken token) { - var serviceEndpointUrl = GetServiceEndpointUrl(source, string.Empty, noServiceEndpoint); - var useTempApiKey = IsSourceNuGetSymbolServer(source); - var codeNotToThrow = ConvertSkipDuplicateParamToHttpStatusCode(skipDuplicate); - var showPushCommandPackagePushed = true; + Uri serviceEndpointUrl = GetServiceEndpointUrl(source, string.Empty, noServiceEndpoint); + bool useTempApiKey = IsSourceNuGetSymbolServer(source); + HttpStatusCode? codeNotToThrow = ConvertSkipDuplicateParamToHttpStatusCode(skipDuplicate); + bool showPushCommandPackagePushed = true; if (useTempApiKey) { @@ -355,7 +392,7 @@ private async Task PushPackageToServer(string source, using (var packageReader = new PackageArchiveReader(pathToPackage)) { - var packageIdentity = packageReader.GetIdentity(); + PackageIdentity packageIdentity = packageReader.GetIdentity(); var success = false; var retry = 0; @@ -366,7 +403,7 @@ private async Task PushPackageToServer(string source, retry++; success = true; // If user push to https://nuget.smbsrc.net/, use temp api key. - var tmpApiKey = await GetSecureApiKey(packageIdentity, apiKey, noServiceEndpoint, requestTimeout, logger, token); + string tmpApiKey = await GetSecureApiKey(packageIdentity, apiKey, noServiceEndpoint, requestTimeout, logger, token); await _httpSource.ProcessResponseAsync( new HttpSourceRequest(() => CreateRequest(serviceEndpointUrl, pathToPackage, tmpApiKey, logger)) @@ -376,9 +413,8 @@ await _httpSource.ProcessResponseAsync( }, response => { - var responseStatusCode = EnsureSuccessStatusCode(response, codeNotToThrow, logger); - - var logOccurred = DetectAndLogSkippedErrorOccurrence(responseStatusCode, source, pathToPackage, response.ReasonPhrase, logger); + HttpStatusCode? responseStatusCode = EnsureSuccessStatusCode(response, codeNotToThrow, logger); + bool logOccurred = DetectAndLogSkippedErrorOccurrence(responseStatusCode, source, pathToPackage, response.ReasonPhrase, logger); showPushCommandPackagePushed = !logOccurred; return Task.FromResult(0); @@ -419,8 +455,8 @@ await _httpSource.ProcessResponseAsync( }, response => { - var responseStatusCode = EnsureSuccessStatusCode(response, codeNotToThrow, logger); - var logOccurred = DetectAndLogSkippedErrorOccurrence(responseStatusCode, source, pathToPackage, response.ReasonPhrase, logger); + HttpStatusCode? responseStatusCode = EnsureSuccessStatusCode(response, codeNotToThrow, logger); + bool logOccurred = DetectAndLogSkippedErrorOccurrence(responseStatusCode, source, pathToPackage, response.ReasonPhrase, logger); showPushCommandPackagePushed = !logOccurred; return Task.FromResult(0); @@ -462,9 +498,13 @@ await _httpSource.ProcessResponseAsync( /// Gently log any specified Skipped status code without throwing. /// /// If provided, it indicates that this StatusCode occurred but was flagged as to be Skipped. + /// + /// + /// /// /// Indication of whether the log occurred. - private static bool DetectAndLogSkippedErrorOccurrence(HttpStatusCode? skippedErrorStatusCode, string source, string packageIdentity, string reasonMessage, ILogger logger) + private static bool DetectAndLogSkippedErrorOccurrence(HttpStatusCode? skippedErrorStatusCode, string source, string packageIdentity, + string reasonMessage, ILogger logger) { bool skippedErrorOccurred = false; @@ -476,7 +516,6 @@ private static bool DetectAndLogSkippedErrorOccurrence(HttpStatusCode? skippedEr switch (skippedErrorStatusCode.Value) { case HttpStatusCode.Conflict: - messageToLog = string.Format( CultureInfo.CurrentCulture, Strings.AddPackage_PackageAlreadyExists, diff --git a/src/NuGet.Core/NuGet.Protocol/Utility/LocalFolderUtility.cs b/src/NuGet.Core/NuGet.Protocol/Utility/LocalFolderUtility.cs index 030d4e2b14c..b31ad332cfd 100644 --- a/src/NuGet.Core/NuGet.Protocol/Utility/LocalFolderUtility.cs +++ b/src/NuGet.Core/NuGet.Protocol/Utility/LocalFolderUtility.cs @@ -956,11 +956,16 @@ public static IEnumerable GetPackagesV3(string root, string id /// A list of package paths that match the input path. public static IEnumerable ResolvePackageFromPath(string packagePath, bool isSnupkg = false) { - // Ensure packagePath ends with *.nupkg packagePath = EnsurePackageExtension(packagePath, isSnupkg); return PathResolver.PerformWildcardSearch(Directory.GetCurrentDirectory(), packagePath); } + /// + /// Ensure any wildcards in packagePath end with *.nupkg or *.snupkg. + /// + /// + /// + /// The absolute path, or the normalized wildcard path. private static string EnsurePackageExtension(string packagePath, bool isSnupkg) { #if NETCOREAPP diff --git a/test/NuGet.Clients.FuncTests/NuGet.CommandLine.FuncTest/Commands/PushCommandTest.cs b/test/NuGet.Clients.FuncTests/NuGet.CommandLine.FuncTest/Commands/PushCommandTest.cs index 9b4f7aab92c..6437304f040 100644 --- a/test/NuGet.Clients.FuncTests/NuGet.CommandLine.FuncTest/Commands/PushCommandTest.cs +++ b/test/NuGet.Clients.FuncTests/NuGet.CommandLine.FuncTest/Commands/PushCommandTest.cs @@ -478,7 +478,7 @@ public void PushCommand_Server_Nupkg_ByWildcard_SnupkgDoesNotExist_NoFileNotFoun string expectedFileNotFoundErrorMessage = string.Format(MESSAGE_FILE_DOES_NOT_EXIST, pushArgument); - Assert.True(result.Success, "Snupkg File did not exist but should not fail a nupkg push."); + Assert.True(result.Success, "Snupkg File did not exist but should not fail a nupkg push.\n\n" + result.AllOutput); Assert.Contains(MESSAGE_PACKAGE_PUSHED, result.Output); Assert.DoesNotContain(WITHOUT_FILENAME_MESSAGE_FILE_DOES_NOT_EXIST, result.Errors); Assert.DoesNotContain(NuGetConstants.SnupkgExtension, result.AllOutput); //Snupkgs should not be mentioned. @@ -592,7 +592,7 @@ public void PushCommand_Server_Nupkg_ByWildcard_FindsMatchingSnupkgs_Conflict() result = CommandRunner.Run( nuget, packageDirectory, - $"push {wildcardPush} -Source {sourceName} -Timeout 110", + $"push {wildcardPush} -Source {sourceName} -SymbolSource {sourceName} -Timeout 110", waitForExit: true, timeOutInMilliseconds: 120000); // 120 seconds } @@ -612,7 +612,7 @@ public void PushCommand_Server_Nupkg_ByWildcard_FindsMatchingSnupkgs_Conflict() } /// - /// When pushing *.Nupkg with SkipDuplicate, a 409 Conflict is ignored and the secondary symbols push proceeds. + /// When pushing *.Nupkg with SkipDuplicate, a 409 Conflict is ignored and the corresponding symbols push is skipped. /// [Fact] public void PushCommand_Server_Nupkg_ByWildcard_FindsMatchingSnupkgs_SkipDuplicate() @@ -666,15 +666,15 @@ public void PushCommand_Server_Nupkg_ByWildcard_FindsMatchingSnupkgs_SkipDuplica //Ignoring filename in File Not Found error since the error should not appear in any case. string genericFileNotFoundError = WITHOUT_FILENAME_MESSAGE_FILE_DOES_NOT_EXIST; - //Nupkg should be an ignored conflict, so its snupkg should push. - Assert.True(result.Success, "Expected to successfully push a snupkg with SkipDuplicate option when the nupkg is a duplicate."); + //Nupkg should be an ignored conflict, so its snupkg shouldn't push. + Assert.True(result.Success, "Expected to skip pushing a snupkg with SkipDuplicate option when the nupkg is a duplicate.\n\n" + result.AllOutput); Assert.DoesNotContain(MESSAGE_RESPONSE_NO_SUCCESS, result.Errors); //nupkg duplicate Assert.Contains(MESSAGE_EXISTING_PACKAGE, result.AllOutput); Assert.DoesNotContain(MESSAGE_PACKAGE_PUSHED, result.AllOutput); //nothing is pushed since nupkg/snupkgs are all skipped duplicates Assert.DoesNotContain(genericFileNotFoundError, result.Errors); - Assert.Contains(snupkgFileName, result.AllOutput); //first snupkg is attempted as push - Assert.Contains(snupkgFileName2, result.AllOutput); //second snupkg is attempted when first duplicate is skipped + Assert.DoesNotContain(snupkgFileName, result.AllOutput); //first snupkg is not attempted since nupkg was duplicate. + Assert.DoesNotContain(snupkgFileName2, result.AllOutput); //second snupkg is not attempted since nupkg was duplicate. } } diff --git a/test/NuGet.Clients.Tests/NuGet.CommandLine.Test/NuGetPushCommandTest.cs b/test/NuGet.Clients.Tests/NuGet.CommandLine.Test/NuGetPushCommandTest.cs index 3a7928d6145..ea90b7b8e5e 100644 --- a/test/NuGet.Clients.Tests/NuGet.CommandLine.Test/NuGetPushCommandTest.cs +++ b/test/NuGet.Clients.Tests/NuGet.CommandLine.Test/NuGetPushCommandTest.cs @@ -417,7 +417,7 @@ public void PushCommand_PushToServerWithSymbols() waitForExit: true); // Assert - Assert.Equal(0, result.Item1); + Assert.True(0 == result.ExitCode, result.AllOutput); Assert.Contains($"Pushing testPackage1.1.1.0.nupkg to '{pushUri}'", result.Item2); Assert.Contains($"Created {pushUri}", result.Item2); Assert.Contains($"Pushing testPackage1.1.1.0.symbols.nupkg to '{pushSymbolsUri}'", result.Item2);