Clean up error experience when downloading non-tools (#43045)

dotnet · Sep 16, 2024 · 7ffe7d4 · 7ffe7d4
2 parents dfaa9c5 + aed512a
commit 7ffe7d4
Show file tree

Hide file tree

Showing 20 changed files with 130 additions and 14 deletions.
diff --git a/src/Cli/dotnet/NugetPackageDownloader/INuGetPackageDownloader.cs b/src/Cli/dotnet/NugetPackageDownloader/INuGetPackageDownloader.cs
@@ -16,7 +16,8 @@ Task<string> DownloadPackageAsync(PackageId packageId,
             bool includePreview = false,
             bool? includeUnlisted = null,
             DirectoryPath? downloadFolder = null,
-            PackageSourceMapping packageSourceMapping = null);
+            PackageSourceMapping packageSourceMapping = null,
+            bool isTool = false);
 
         Task<string> GetPackageUrl(PackageId packageId,
             NuGetVersion packageVersion = null,

diff --git a/src/Cli/dotnet/NugetPackageDownloader/LocalizableStrings.resx b/src/Cli/dotnet/NugetPackageDownloader/LocalizableStrings.resx
@@ -126,6 +126,9 @@
   <data name="IsNotFoundInNuGetFeeds" xml:space="preserve">
     <value>{0} is not found in NuGet feeds {1}.</value>
   </data>
+  <data name="NotATool" xml:space="preserve">
+    <value>Package {0} is not a .NET tool.</value>
+  </data>
   <data name="DownloadVersionFailed" xml:space="preserve">
     <value>Downloading {0} version {1} failed.</value>
   </data>

diff --git a/src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs b/src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs
@@ -83,19 +83,45 @@ public async Task<string> DownloadPackageAsync(PackageId packageId,
             bool includePreview = false,
             bool? includeUnlisted = null,
             DirectoryPath? downloadFolder = null,
-            PackageSourceMapping packageSourceMapping = null)
+            PackageSourceMapping packageSourceMapping = null,
+            bool isTool = false)
         {
             CancellationToken cancellationToken = CancellationToken.None;
 
             (var source, var resolvedPackageVersion) = await GetPackageSourceAndVersion(packageId, packageVersion,
                 packageSourceLocation, includePreview, includeUnlisted ?? packageVersion is not null, packageSourceMapping).ConfigureAwait(false);
 
-            FindPackageByIdResource resource = null;
             SourceRepository repository = GetSourceRepository(source);
 
-            resource = await repository.GetResourceAsync<FindPackageByIdResource>(cancellationToken)
-                .ConfigureAwait(false);
+            // TODO: Fix this to use the PackageSearchResourceV3 once https://github.com/NuGet/NuGet.Client/pull/5991 is completed.
+            if (isTool && await repository.GetResourceAsync<ServiceIndexResourceV3>().ConfigureAwait(false) is ServiceIndexResourceV3 serviceIndex)
+            {
+                // See https://learn.microsoft.com/en-us/nuget/api/package-base-address-resource#download-package-manifest-nuspec
+                var uri = serviceIndex.GetServiceEntries("PackageBaseAddress/3.0.0")[0].Uri;
+                var queryUri = uri + $"{packageId}/{packageVersion}/{packageId}.nuspec";
+
+                using HttpClient client = new(new HttpClientHandler() { CheckCertificateRevocationList = true });
+                using HttpResponseMessage response = await client.GetAsync(queryUri).ConfigureAwait(false);
+
+                if (response.IsSuccessStatusCode)
+                {
+                    string nuspec = await response.Content.ReadAsStringAsync().ConfigureAwait(false);
+
+                    XDocument doc = XDocument.Parse(nuspec);
+
+                    if (!ToolPackageInstance.IsToolPackage(doc))
+                    {
+                        throw new GracefulException(string.Format(LocalizableStrings.NotATool, packageId));
+                    }
+                }
+                else
+                {
+                    throw new GracefulException(string.Format(LocalizableStrings.NotATool, packageId));
+                }
+            }
 
+            FindPackageByIdResource resource = await repository.GetResourceAsync<FindPackageByIdResource>(cancellationToken)
+                .ConfigureAwait(false);
             if (resource == null)
             {
                 throw new NuGetPackageNotFoundException(

diff --git a/src/Cli/dotnet/NugetPackageDownloader/xlf/LocalizableStrings.cs.xlf b/src/Cli/dotnet/NugetPackageDownloader/xlf/LocalizableStrings.cs.xlf
diff --git a/src/Cli/dotnet/NugetPackageDownloader/xlf/LocalizableStrings.de.xlf b/src/Cli/dotnet/NugetPackageDownloader/xlf/LocalizableStrings.de.xlf
diff --git a/src/Cli/dotnet/NugetPackageDownloader/xlf/LocalizableStrings.es.xlf b/src/Cli/dotnet/NugetPackageDownloader/xlf/LocalizableStrings.es.xlf
diff --git a/src/Cli/dotnet/NugetPackageDownloader/xlf/LocalizableStrings.fr.xlf b/src/Cli/dotnet/NugetPackageDownloader/xlf/LocalizableStrings.fr.xlf
diff --git a/src/Cli/dotnet/NugetPackageDownloader/xlf/LocalizableStrings.it.xlf b/src/Cli/dotnet/NugetPackageDownloader/xlf/LocalizableStrings.it.xlf
diff --git a/src/Cli/dotnet/NugetPackageDownloader/xlf/LocalizableStrings.ja.xlf b/src/Cli/dotnet/NugetPackageDownloader/xlf/LocalizableStrings.ja.xlf
diff --git a/src/Cli/dotnet/NugetPackageDownloader/xlf/LocalizableStrings.ko.xlf b/src/Cli/dotnet/NugetPackageDownloader/xlf/LocalizableStrings.ko.xlf
diff --git a/src/Cli/dotnet/NugetPackageDownloader/xlf/LocalizableStrings.pl.xlf b/src/Cli/dotnet/NugetPackageDownloader/xlf/LocalizableStrings.pl.xlf
diff --git a/src/Cli/dotnet/NugetPackageDownloader/xlf/LocalizableStrings.pt-BR.xlf b/src/Cli/dotnet/NugetPackageDownloader/xlf/LocalizableStrings.pt-BR.xlf
diff --git a/src/Cli/dotnet/NugetPackageDownloader/xlf/LocalizableStrings.ru.xlf b/src/Cli/dotnet/NugetPackageDownloader/xlf/LocalizableStrings.ru.xlf
diff --git a/src/Cli/dotnet/NugetPackageDownloader/xlf/LocalizableStrings.tr.xlf b/src/Cli/dotnet/NugetPackageDownloader/xlf/LocalizableStrings.tr.xlf
diff --git a/src/Cli/dotnet/NugetPackageDownloader/xlf/LocalizableStrings.zh-Hans.xlf b/src/Cli/dotnet/NugetPackageDownloader/xlf/LocalizableStrings.zh-Hans.xlf
diff --git a/src/Cli/dotnet/NugetPackageDownloader/xlf/LocalizableStrings.zh-Hant.xlf b/src/Cli/dotnet/NugetPackageDownloader/xlf/LocalizableStrings.zh-Hant.xlf
diff --git a/src/Cli/dotnet/ToolPackage/ToolPackageDownloader.cs b/src/Cli/dotnet/ToolPackage/ToolPackageDownloader.cs
@@ -134,13 +134,21 @@ public IToolPackage InstallPackage(PackageLocation packageLocation, PackageId pa
                     {
                         DownloadAndExtractPackage(packageId, nugetPackageDownloader, toolDownloadDir.Value, packageVersion, packageSourceLocation, includeUnlisted: givenSpecificVersion).GetAwaiter().GetResult();
                     }
-                    else if(isGlobalTool)
+                    else
                     {
-                        throw new ToolPackageException(
-                            string.Format(
-                                CommonLocalizableStrings.ToolPackageConflictPackageId,
-                                packageId,
-                                packageVersion.ToNormalizedString()));
+                        if (!ToolPackageInstance.IsToolPackage(package.Nuspec.Xml))
+                        {
+                            throw new GracefulException(string.Format(NuGetPackageDownloader.LocalizableStrings.NotATool, packageId));
+                        }
+
+                        if (isGlobalTool)
+                        {
+                            throw new ToolPackageException(
+                                string.Format(
+                                    CommonLocalizableStrings.ToolPackageConflictPackageId,
+                                    packageId,
+                                    packageVersion.ToNormalizedString()));
+                        }
                     }
 
                     CreateAssetFile(packageId, packageVersion, toolDownloadDir, assetFileDirectory, _runtimeJsonPath, targetFramework);
@@ -284,7 +292,7 @@ private static async Task<NuGetVersion> DownloadAndExtractPackage(
             bool includeUnlisted = false
             )
         {
-            var packagePath = await nugetPackageDownloader.DownloadPackageAsync(packageId, packageVersion, packageSourceLocation, includeUnlisted: includeUnlisted).ConfigureAwait(false);
+            var packagePath = await nugetPackageDownloader.DownloadPackageAsync(packageId, packageVersion, packageSourceLocation, includeUnlisted: includeUnlisted, isTool: true).ConfigureAwait(false);
 
             // look for package on disk and read the version
             NuGetVersion version;

diff --git a/src/Cli/dotnet/ToolPackage/ToolPackageInstance.cs b/src/Cli/dotnet/ToolPackage/ToolPackageInstance.cs
@@ -23,6 +23,17 @@ public static ToolPackageInstance CreateFromAssetFile(PackageId id, DirectoryPat
 
             return new ToolPackageInstance(id, version, packageDirectory, assetsJsonParentDirectory);
         }
+
+        /// <summary>
+        /// Validates that the nuspec XML represents a .NET tool package.
+        /// </summary>
+        /// <param name="nuspec">The nuspec XML to check.</param>
+        /// <returns><see langword="true"/> if the nuspec represents a .NET tool package; otherwise, <see langword="false"/>.</returns>
+        public static bool IsToolPackage(XDocument nuspec) =>
+            nuspec.Root.Descendants().Where(
+                e => e.Name.LocalName == "packageType" &&
+                e.Attributes().Where(a => a.Name.LocalName == "name" && a.Value == "DotnetTool").Any()).Any();
+
         private const string PackagedShimsDirectoryConvention = "shims";
 
         public IEnumerable<string> Warnings => _toolConfiguration.Value.Warnings;

diff --git a/test/dotnet-workload-install.Tests/FailingNuGetPackageInstaller.cs b/test/dotnet-workload-install.Tests/FailingNuGetPackageInstaller.cs
@@ -23,7 +23,8 @@ public Task<string> DownloadPackageAsync(PackageId packageId, NuGetVersion packa
             bool includePreview = false,
             bool? includeUnlisted = null,
             DirectoryPath? downloadFolder = null,
-            PackageSourceMapping packageSourceMapping = null)
+            PackageSourceMapping packageSourceMapping = null,
+            bool isTool = false)
         {
             var mockPackagePath = Path.Combine(MockPackageDir, $"{packageId}.{packageVersion}.nupkg");
             File.WriteAllText(mockPackagePath, string.Empty);

diff --git a/test/dotnet-workload-install.Tests/MockNuGetPackageInstaller.cs b/test/dotnet-workload-install.Tests/MockNuGetPackageInstaller.cs
@@ -38,7 +38,8 @@ public Task<string> DownloadPackageAsync(PackageId packageId,
             bool includePreview = false,
             bool? includeUnlisted = null,
             DirectoryPath? downloadFolder = null,
-            PackageSourceMapping packageSourceMapping = null)
+            PackageSourceMapping packageSourceMapping = null,
+            bool isTool = false)
         {
             DownloadCallParams.Add((packageId, packageVersion, downloadFolder, packageSourceLocation));