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.

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

report specific error when project cannot be restored #10720

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
using System.Text.Json;

using NuGetUpdater.Core.Updater;

using Xunit;
Expand Down Expand Up @@ -137,10 +135,7 @@ protected static async Task TestUpdateForProject(
// run update
var worker = new UpdaterWorker(new Logger(verbose: true));
var projectPath = placeFilesInSrc ? $"src/{projectFilePath}" : projectFilePath;
var updateResultFile = Path.Combine(temporaryDirectory, "update-result.json");
await worker.RunAsync(temporaryDirectory, projectPath, dependencyName, oldVersion, newVersion, isTransitive, updateResultFile);
var actualResultContents = await File.ReadAllTextAsync(updateResultFile);
var actualResult = JsonSerializer.Deserialize<UpdateOperationResult>(actualResultContents, UpdaterWorker.SerializerOptions);
var actualResult = await worker.RunWithErrorHandlingAsync(temporaryDirectory, projectPath, dependencyName, oldVersion, newVersion, isTransitive);
if (expectedResult is { })
{
ValidateUpdateOperationResult(expectedResult, actualResult!);
Expand All @@ -159,7 +154,7 @@ protected static async Task TestUpdateForProject(
protected static void ValidateUpdateOperationResult(UpdateOperationResult expectedResult, UpdateOperationResult actualResult)
{
Assert.Equal(expectedResult.ErrorType, actualResult.ErrorType);
Assert.Equal(expectedResult.ErrorDetails, actualResult.ErrorDetails);
Assert.Equivalent(expectedResult.ErrorDetails, actualResult.ErrorDetails);
}

protected static Task TestNoChangeforSolution(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2190,6 +2190,84 @@ await TestUpdateForProject("Some.Package", "1.0.0", "1.1.0",
);
}

[Fact]
public async Task MissingDependencyErrorIsReported()
{
// trying to update Some.Package from 1.0.1 to 1.0.2, but another package isn't available; update fails
await TestUpdateForProject("Some.Package", "1.0.1", "1.0.2",
packages:
[
MockNuGetPackage.CreateSimplePackage("Some.Package", "1.0.1", "net45"),
MockNuGetPackage.CreateSimplePackage("Some.Package", "1.0.2", "net45"),

// the package `Unrelated.Package/1.0.0` is missing and will cause the update to fail
],
// existing
projectContents: """
<Project ToolsVersion="15.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<Import Project="$(MSBuildExtensionsPath)\$(MSBuildToolsVersion)\Microsoft.Common.props" Condition="Exists('$(MSBuildExtensionsPath)\$(MSBuildToolsVersion)\Microsoft.Common.props')" />
<PropertyGroup>
<TargetFrameworkVersion>v4.5</TargetFrameworkVersion>
</PropertyGroup>
<ItemGroup>
<None Include="packages.config" />
</ItemGroup>
<ItemGroup>
<Reference Include="Some.Package, Version=1.0.0.0, Culture=neutral, PublicKeyToken=30ad4fe6b2a6aeed">
<HintPath>packages\Some.Package.1.0.1\lib\net45\Some.Package.dll</HintPath>
<Private>True</Private>
</Reference>
<Reference Include="Unrelated.Package, Version=1.0.0.0, Culture=neutral, PublicKeyToken=30ad4fe6b2a6aeed">
<HintPath>packages\Unrelated.Package.1.0.0\lib\net45\Unrelated.Package.dll</HintPath>
<Private>True</Private>
</Reference>
</ItemGroup>
<Import Project="$(MSBuildToolsPath)\Microsoft.CSharp.targets" />
</Project>
""",
packagesConfigContents: """
<packages>
<package id="Some.Package" version="1.0.1" targetFramework="net45" />
<package id="Unrelated.Package" version="1.0.0" targetFramework="net45" />
</packages>
""",
// expected
expectedProjectContents: """
<Project ToolsVersion="15.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<Import Project="$(MSBuildExtensionsPath)\$(MSBuildToolsVersion)\Microsoft.Common.props" Condition="Exists('$(MSBuildExtensionsPath)\$(MSBuildToolsVersion)\Microsoft.Common.props')" />
<PropertyGroup>
<TargetFrameworkVersion>v4.5</TargetFrameworkVersion>
</PropertyGroup>
<ItemGroup>
<None Include="packages.config" />
</ItemGroup>
<ItemGroup>
<Reference Include="Some.Package, Version=1.0.0.0, Culture=neutral, PublicKeyToken=30ad4fe6b2a6aeed">
<HintPath>packages\Some.Package.1.0.1\lib\net45\Some.Package.dll</HintPath>
<Private>True</Private>
</Reference>
<Reference Include="Unrelated.Package, Version=1.0.0.0, Culture=neutral, PublicKeyToken=30ad4fe6b2a6aeed">
<HintPath>packages\Unrelated.Package.1.0.0\lib\net45\Unrelated.Package.dll</HintPath>
<Private>True</Private>
</Reference>
</ItemGroup>
<Import Project="$(MSBuildToolsPath)\Microsoft.CSharp.targets" />
</Project>
""",
expectedPackagesConfigContents: """
<packages>
<package id="Some.Package" version="1.0.1" targetFramework="net45" />
<package id="Unrelated.Package" version="1.0.0" targetFramework="net45" />
</packages>
""",
expectedResult: new()
{
ErrorType = ErrorType.UpdateNotPossible,
ErrorDetails = new[] { "Unrelated.Package.1.0.0" },
}
);
}

protected static Task TestUpdateForProject(
string dependencyName,
string oldVersion,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,5 @@ public enum ErrorType
None,
AuthenticationFailure,
MissingFile,
UpdateNotPossible,
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,5 @@ public record NativeResult
{
// TODO: nullable not required, `ErrorType.None` is the default anyway
public ErrorType? ErrorType { get; init; }
public string? ErrorDetails { get; init; }
public object? ErrorDetails { get; init; }
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,5 @@ public abstract record JobErrorBase
[JsonPropertyName("error-type")]
public abstract string Type { get; }
[JsonPropertyName("error-details")]
public required string Details { get; init; }
public required object Details { get; init; }
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
namespace NuGetUpdater.Core.Run.ApiModel;

public record UpdateNotPossible : JobErrorBase
{
public override string Type => "update_not_possible";
}
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,13 @@ private async Task<RunResult> RunWithErrorHandlingAsync(Job job, DirectoryInfo r
Details = ex.FilePath,
};
}
catch (UpdateNotPossibleException ex)
{
error = new UpdateNotPossible()
{
Details = ex.Dependencies,
};
}
catch (Exception ex)
{
error = new UnknownError()
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
namespace NuGetUpdater.Core;

internal class UpdateNotPossibleException : Exception
{
public string[] Dependencies { get; }

public UpdateNotPossibleException(string[] dependencies)
{
Dependencies = dependencies;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ private static void RunNugetUpdate(List<string> updateArgs, List<string> restore

if (exitCodeAgain != 0)
{
MSBuildHelper.ThrowOnMissingPackages(restoreOutput);
throw new Exception($"Unable to restore.\nOutput:\n${restoreOutput}\n");
}

Expand All @@ -147,6 +148,7 @@ private static void RunNugetUpdate(List<string> updateArgs, List<string> restore

MSBuildHelper.ThrowOnUnauthenticatedFeed(fullOutput);
MSBuildHelper.ThrowOnMissingFile(fullOutput);
MSBuildHelper.ThrowOnMissingPackages(fullOutput);
throw new Exception(fullOutput);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,16 @@ public UpdaterWorker(Logger logger)
}

public async Task RunAsync(string repoRootPath, string workspacePath, string dependencyName, string previousDependencyVersion, string newDependencyVersion, bool isTransitive, string? resultOutputPath = null)
{
var result = await RunWithErrorHandlingAsync(repoRootPath, workspacePath, dependencyName, previousDependencyVersion, newDependencyVersion, isTransitive);
if (resultOutputPath is { })
{
await WriteResultFile(result, resultOutputPath, _logger);
}
}

// this is a convenient method for tests
internal async Task<UpdateOperationResult> RunWithErrorHandlingAsync(string repoRootPath, string workspacePath, string dependencyName, string previousDependencyVersion, string newDependencyVersion, bool isTransitive)
{
UpdateOperationResult result = new(); // assumed to be ok until proven otherwise
try
Expand Down Expand Up @@ -52,11 +62,16 @@ public async Task RunAsync(string repoRootPath, string workspacePath, string dep
ErrorDetails = ex.FilePath,
};
}

if (resultOutputPath is { })
catch (UpdateNotPossibleException ex)
{
await WriteResultFile(result, resultOutputPath, _logger);
result = new()
{
ErrorType = ErrorType.UpdateNotPossible,
ErrorDetails = ex.Dependencies,
};
}

return result;
}

public async Task<UpdateOperationResult> RunAsync(string repoRootPath, string workspacePath, string dependencyName, string previousDependencyVersion, string newDependencyVersion, bool isTransitive)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -833,6 +833,17 @@ internal static void ThrowOnMissingFile(string output)
}
}

internal static void ThrowOnMissingPackages(string output)
{
var missingPackagesPattern = new Regex(@"Package '(?<PackageName>[^'].*)' is not found on source");
var matchCollection = missingPackagesPattern.Matches(output);
var missingPackages = matchCollection.Select(m => m.Groups["PackageName"].Value).Distinct().ToArray();
if (missingPackages.Length > 0)
{
throw new UpdateNotPossibleException(missingPackages);
}
}

internal static bool TryGetGlobalJsonPath(string repoRootPath, string workspacePath, [NotNullWhen(returnValue: true)] out string? globalJsonPath)
{
globalJsonPath = PathHelper.GetFileInDirectoryOrParent(workspacePath, repoRootPath, "global.json", caseSensitive: false);
Expand Down
8 changes: 5 additions & 3 deletions nuget/lib/dependabot/nuget/native_helpers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -263,14 +263,16 @@ def self.run_nuget_updater_tool(repo_root:, proj_path:, dependency:, is_transiti
sig { params(json: T::Hash[String, T.untyped]).void }
def self.ensure_no_errors(json)
error_type = T.let(json.fetch("ErrorType", nil), T.nilable(String))
error_details = T.let(json.fetch("ErrorDetails", nil), T.nilable(String))
error_details = json.fetch("ErrorDetails", nil)
case error_type
when "None", nil
# no issue
when "AuthenticationFailure"
raise PrivateSourceAuthenticationFailure, error_details
raise PrivateSourceAuthenticationFailure, T.let(error_details, T.nilable(String))
when "MissingFile"
raise DependencyFileNotFound, error_details
raise DependencyFileNotFound, T.let(error_details, T.nilable(String))
when "UpdateNotPossible"
raise UpdateNotPossible, T.let(error_details, T::Array[String])
else
raise "Unexpected error type from native tool: #{error_type}: #{error_details}"
end
Expand Down
65 changes: 65 additions & 0 deletions nuget/spec/dependabot/nuget/native_helpers_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -192,4 +192,69 @@
end
end
end

describe "#ensure_no_errors" do
subject(:error_message) do
begin
described_class.ensure_no_errors(JSON.parse(json))

# defaults to no error
return nil
rescue StandardError => e
return e.to_s
end
end

context "when nothing is reported" do
let(:json) { "{}" } # an empty object

it { is_expected.to be_nil }
end

context "when the error is expclicitly none" do
let(:json) do
{
ErrorType: "None"
}.to_json
end

it { is_expected.to be_nil }
end

context "when an authentication failure is encountered" do
let(:json) do
{
ErrorType: "AuthenticationFailure",
ErrorDetails: "(some-source)"
}.to_json
end

it { is_expected.to include(": (some-source)") }
end

context "when a file is missing" do
let(:json) do
{
ErrorType: "MissingFile",
ErrorDetails: "some.file"
}.to_json
end

it { is_expected.to include("some.file not found") }
end

context "when an update is not possible" do
let(:json) do
{
ErrorType: "UpdateNotPossible",
ErrorDetails: [
"dependency 1",
"dependency 2"
]
}.to_json
end

it { is_expected.to include("dependency 1, dependency 2") }
end
end
end
Loading