Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Aug 24, 2025

The IsNuGetPluginValid method was catching all exceptions from FileVersionInfo.GetVersionInfo and assuming the plugin was invalid, which could trigger unnecessary dotnet restore operations on systems where version checking consistently fails due to file format issues.

Problem

On some systems, FileVersionInfo.GetVersionInfo throws NotSupportedException when the file format isn't supported for version reading, even though the plugin DLL itself is perfectly valid. The previous implementation treated all exceptions the same way:

catch (Exception ex)
{
    logger?.LogDebug(ex, "Error checking version for NuGet plugin: {dllPath}", plugin.DllPath);
    return false; // Assume invalid if we can't check version
}

This caused unnecessary dotnet restore operations to run repeatedly on systems with consistent file format issues.

Solution

Now differentiates between exception types with more appropriate fallback behavior:

  • NotSupportedException: Logs a warning and assumes the plugin is valid (returns true)
  • Other exceptions: Logs an error and assumes the plugin is invalid (returns false)
catch (NotSupportedException ex)
{
    logger?.LogWarning(ex, "File format not supported when checking version for NuGet plugin: {dllPath}. Assuming valid.", plugin.DllPath);
    return true; // Assume valid if file format is not supported
}
catch (Exception ex)
{
    logger?.LogError(ex, "Unexpected error checking version for NuGet plugin: {dllPath}. Assuming invalid.", plugin.DllPath);
    return false; // Assume invalid for other exceptions
}

Testing

Added comprehensive tests to verify both exception handling paths:

  • IsNuGetPluginValid_WithNotSupportedException_ReturnsTrue
  • IsNuGetPluginValid_WithOtherException_ReturnsFalseAndLogsError

All existing tests continue to pass (103 total: 98 succeeded, 5 skipped, 0 failed).

Fixes #53.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@github-actions github-actions bot added the preview Triggers a release of preview version of Nuget packages label Aug 24, 2025
Copilot AI changed the title [WIP] The comment suggests assuming invalid when version checking fails, but this could cause unnecessary restores on systems where FileVersionInfo.GetVersionInfo consistently fails due to file format issues. Consider logging the specific exception type and potentially having different fallback behavior for different exception types. Improve exception handling in IsNuGetPluginValid to prevent unnecessary restores Aug 24, 2025
Copilot AI requested a review from matt-goldman August 24, 2025 22:13
@github-actions github-actions bot added enhancement New feature or request Blake.BuildTools labels Aug 25, 2025
Copy link
Owner

@matt-goldman matt-goldman left a comment

Choose a reason for hiding this comment

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

LGTM

@matt-goldman matt-goldman marked this pull request as ready for review August 25, 2025 08:38
Copilot AI review requested due to automatic review settings August 25, 2025 08:38
@matt-goldman matt-goldman merged commit e1c3f90 into main Aug 25, 2025
5 checks passed
@matt-goldman matt-goldman deleted the copilot/fix-53 branch August 25, 2025 08:38
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR improves exception handling in the IsNuGetPluginValid method to prevent unnecessary dotnet restore operations caused by file format exceptions. The change differentiates between NotSupportedException (assumes plugin is valid) and other exceptions (assumes plugin is invalid), reducing redundant restore operations on systems with consistent file format issues.

  • Adds specific handling for NotSupportedException to assume plugin validity
  • Changes generic exception handling to log errors instead of debug messages
  • Adds comprehensive test coverage for both exception handling paths

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/Blake.BuildTools/Utils/PluginLoader.cs Updates exception handling logic to differentiate between NotSupportedException and other exceptions
tests/Blake.BuildTools.Tests/Utils/PluginLoaderTests.cs Adds two new test methods and enhances TestLogger to capture warning messages

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Blake.BuildTools enhancement New feature or request preview Triggers a release of preview version of Nuget packages

Projects

None yet

2 participants