Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Log when Restore gets an HTTP 404 for a version when the package is listed on the feed #5845

Merged
merged 4 commits into from
Jun 10, 2024
Merged

Log when Restore gets an HTTP 404 for a version when the package is listed on the feed #5845

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
60a8df0
log an error when a package is not found in the feed.
kartheekp-ms Jun 5, 2024
57b7a85
updated existing test
kartheekp-ms Jun 6, 2024
15e1ea7
updated test name
kartheekp-ms Jun 6, 2024
f8a6d84
test name
donnie-msft Jun 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/NuGet.Core/NuGet.DependencyResolver.Core/ResolverUtility.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ public static async Task<GraphItem<RemoteResolveResult>> FindLibraryEntryAsync(
Strings.Error_PackageNotFoundWhenExpected,
match.Provider.Source,
ex.PackageIdentity.ToString());
context.Logger.LogError(message);
Copy link
Contributor

@kartheekp-ms kartheekp-ms Jun 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks to @nkolev92 for providing pointers to the code in NuGet/Home#13460 (comment). The detailed information really helped to understand the root cause of the bug.

There was one statement in that linked comment that I thought I could explain here:

Not sure why InvalidCacheProtocolException in particular gets ignored.

From what I understand, looking at the code, InvalidCacheProtocolException is ignored here because the caller ResolverUtility.FindLibraryEntryAsync has logic to try downloading the package twice. The first time when PackageNotFoundProtocolException is thrown, the code in the first catch block at the caller invalidates the cache and does a retry. That's why the exception is not logged at the callee but instead logged at the caller.

Hence, in this PR, the exception is logged to the logger at the caller in the second catch block that handles the PackageNotFoundProtocolException.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is missing a log code.
It should've probably been NU1301 like the other ones, or potentially a new one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about it more, this scenario is distinct enough, where I think it could use a separate code.


throw new FatalProtocolException(message, ex);
}
Expand Down
11 changes: 9 additions & 2 deletions test/NuGet.Core.Tests/NuGet.DependencyResolver.Core.Tests/FindPackageTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
using System.Globalization;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
Expand Down Expand Up @@ -165,7 +165,7 @@ public async Task FindLibraryEntryAsync_LogsOnlyPackages(LibraryDependencyTarget
}

[Fact]
public async Task FindPackage_VerifyMissingListedPackageThrowsNotFound()
public async Task FindPackage_VerifyMissingListedPackage_ThrowsNotFoundAndLogsErrorAsync()
{
// Arrange
var range = new LibraryRange("x", VersionRange.Parse("1.0.0-beta"), LibraryDependencyTarget.Package);
Expand All @@ -176,6 +176,7 @@ public async Task FindPackage_VerifyMissingListedPackageThrowsNotFound()
var token = CancellationToken.None;
var edge = new GraphEdge<RemoteResolveResult>(null, null, null);
var actualIdentity = new LibraryIdentity("x", NuGetVersion.Parse("1.0.0-beta"), LibraryType.Package);
var packageIdentity = new PackageIdentity(actualIdentity.Name, actualIdentity.Version);
var dependencies = new[] { new LibraryDependency() { LibraryRange = new LibraryRange("y", VersionRange.All, LibraryDependencyTarget.Package) } };
var dependencyInfo = LibraryDependencyInfo.Create(actualIdentity, framework, dependencies);

Expand All @@ -198,6 +199,12 @@ public async Task FindPackage_VerifyMissingListedPackageThrowsNotFound()

// Assert
Assert.Equal(2, hitCount);
Assert.Equal(1, testLogger.Errors);
string errorMessage = string.Format(CultureInfo.CurrentCulture,
Strings.Error_PackageNotFoundWhenExpected,
remoteProvider.Object.Source,
packageIdentity.ToString());
Assert.Equal(errorMessage, testLogger.ErrorMessages.Single());
}

[Fact]
Expand Down