Merge pull request #22468 from michaelnebel/csharp/refactorfeedmanager

C#: Re-factor FeedManager to allow better unit testing.

Author: michaelnebel

csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DependabotProxy.cs

@@ -9,7 +9,7 @@
 
 namespace Semmle.Extraction.CSharp.DependencyFetching
 {
-    public class DependabotProxy : IDisposable
+    public class DependabotProxy : IDependabotProxy
     {
         /// <summary>
         /// Represents configurations for package registries.
@@ -21,24 +21,15 @@ public record class RegistryConfig(string Type, string URL);
         private readonly string host;
         private readonly string port;
 
-        /// <summary>
-        /// The full address of the Dependabot proxy, if available.
-        /// </summary>
-        internal string Address { get; }
-        /// <summary>
-        /// The URLs of package registries that are configured for the proxy.
-        /// </summary>
-        internal HashSet<string> RegistryURLs { get; }
-        /// <summary>
-        /// The path to the temporary file where the certificate is stored.
-        /// </summary>
-        internal string? CertificatePath { get; private set; }
-        /// <summary>
-        /// The certificate used for the Dependabot proxy.
-        /// </summary>
-        internal X509Certificate2? Certificate { get; private set; }
+        public string Address { get; }
+
+        public HashSet<string> RegistryURLs { get; }
+
+        public string? CertificatePath { get; private set; }
+
+        public X509Certificate2? Certificate { get; private set; }
 
-        internal static DependabotProxy? GetDependabotProxy(
+        internal static IDependabotProxy? GetDependabotProxy(
             ILogger logger, IDiagnosticsWriter diagnosticsWriter, TemporaryDirectory tempWorkingDirectory)
         {
             // Setting HTTP(S)_PROXY and SSL_CERT_FILE have no effect on Windows or macOS,

csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DependencyManager.cs

@@ -27,10 +27,10 @@ public sealed partial class DependencyManager : IDisposable, ICompilationInfoCon
         private readonly ILogger logger;
         private readonly IDiagnosticsWriter diagnosticsWriter;
         private readonly NugetPackageRestorer nugetPackageRestorer;
-        private readonly DependabotProxy? dependabotProxy;
+        private readonly IDependabotProxy? dependabotProxy;
         private readonly IDotNet dotnet;
         private readonly FileContent fileContent;
-        private readonly FileProvider fileProvider;
+        private readonly IFileProvider fileProvider;
 
         // Only used as a set, but ConcurrentDictionary is the only concurrent set in .NET.
         private readonly IDictionary<string, bool> usedReferences = new ConcurrentDictionary<string, bool>();

csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DotNet.cs

@@ -31,11 +31,11 @@ private DotNet(IDotNetCliInvoker dotnetCliInvoker, ILogger logger, bool runDotne
             }
         }
 
-        private DotNet(ILogger logger, string? dotNetPath, TemporaryDirectory tempWorkingDirectory, DependabotProxy? dependabotProxy) : this(new DotNetCliInvoker(logger, Path.Join(dotNetPath ?? string.Empty, "dotnet"), dependabotProxy), logger, dotNetPath is null, tempWorkingDirectory) { }
+        private DotNet(ILogger logger, string? dotNetPath, TemporaryDirectory tempWorkingDirectory, IDependabotProxy? dependabotProxy) : this(new DotNetCliInvoker(logger, Path.Join(dotNetPath ?? string.Empty, "dotnet"), dependabotProxy), logger, dotNetPath is null, tempWorkingDirectory) { }
 
         internal static IDotNet Make(IDotNetCliInvoker dotnetCliInvoker, ILogger logger, bool runDotnetInfo) => new DotNet(dotnetCliInvoker, logger, runDotnetInfo);
 
-        public static IDotNet Make(ILogger logger, string? dotNetPath, TemporaryDirectory tempWorkingDirectory, DependabotProxy? dependabotProxy) => new DotNet(logger, dotNetPath, tempWorkingDirectory, dependabotProxy);
+        public static IDotNet Make(ILogger logger, string? dotNetPath, TemporaryDirectory tempWorkingDirectory, IDependabotProxy? dependabotProxy) => new DotNet(logger, dotNetPath, tempWorkingDirectory, dependabotProxy);
 
         private static void HandleRetryExitCode143(string dotnet, int attempt, ILogger logger)
         {
@@ -90,7 +90,8 @@ private List<string> GetRestoreArgs(RestoreSettings restoreSettings)
                 args.Add("/p:EnableWindowsTargeting=true");
             }
 
-            args.AddRange(restoreSettings.NugetSources);
+            var nugetSources = restoreSettings.NugetSources.SelectMany<string, string>(source => ["-s", source]).ToList();
+            args.AddRange(nugetSources);
 
             return args;
         }

csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DotNetCliInvoker.cs

@@ -12,11 +12,11 @@ namespace Semmle.Extraction.CSharp.DependencyFetching
     internal sealed class DotNetCliInvoker : IDotNetCliInvoker
     {
         private readonly ILogger logger;
-        private readonly DependabotProxy? proxy;
+        private readonly IDependabotProxy? proxy;
 
         public string Exec { get; }
 
-        public DotNetCliInvoker(ILogger logger, string exec, DependabotProxy? dependabotProxy)
+        public DotNetCliInvoker(ILogger logger, string exec, IDependabotProxy? dependabotProxy)
         {
             this.logger = logger;
             this.proxy = dependabotProxy;

csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/FeedManager.cs

@@ -1,15 +1,8 @@
 using System;
 using System.Collections.Generic;
 using System.Collections.Immutable;
-using System.IO;
 using System.Linq;
-using System.Net;
-using System.Net.Http;
-using System.Security.Cryptography.X509Certificates;
-using System.Text;
 using System.Text.RegularExpressions;
-using System.Threading;
-using System.Threading.Tasks;
 using Semmle.Util;
 using Semmle.Util.Logging;
 
@@ -21,10 +14,10 @@ internal sealed partial class FeedManager : IDisposable
 
         private readonly ILogger logger;
         private readonly IDotNet dotnet;
-        private readonly FileProvider fileProvider;
-        private readonly DependabotProxy? dependabotProxy;
+        private readonly IFileProvider fileProvider;
         private readonly DependencyDirectory emptyPackageDirectory;
         private readonly ImmutableHashSet<string> privateRegistryFeeds;
+        private readonly IFeedManagerIO feedManagerIo;
 
         /// <summary>
         /// Gets whether there are private package registries configured for C#.
@@ -79,12 +72,12 @@ internal sealed partial class FeedManager : IDisposable
         /// </summary>
         public ImmutableHashSet<string> ReachableFallbackFeeds => lazyReachableFallbackFeeds.Value;
 
-        public FeedManager(ILogger logger, IDotNet dotnet, DependabotProxy? dependabotProxy, FileProvider fileProvider)
+        public FeedManager(ILogger logger, IDotNet dotnet, IDependabotProxy? dependabotProxy, IFileProvider fileProvider, IFeedManagerIO feedManagerIo)
         {
             this.logger = logger;
             this.dotnet = dotnet;
-            this.dependabotProxy = dependabotProxy;
             this.fileProvider = fileProvider;
+            this.feedManagerIo = feedManagerIo;
             privateRegistryFeeds = dependabotProxy?.RegistryURLs.ToImmutableHashSet() ?? [];
             HasPrivateRegistryFeeds = privateRegistryFeeds.Count > 0;
             emptyPackageDirectory = new DependencyDirectory("empty", "empty package", logger);
@@ -105,17 +98,9 @@ public FeedManager(ILogger logger, IDotNet dotnet, DependabotProxy? dependabotPr
             });
         }
 
-        private string? GetDirectoryName(string path)
+        public FeedManager(ILogger logger, IDotNet dotnet, IDependabotProxy? dependabotProxy, IFileProvider fileProvider)
+            : this(logger, dotnet, dependabotProxy, fileProvider, new FeedManagerIO(logger, dependabotProxy))
         {
-            try
-            {
-                return new FileInfo(path).Directory?.FullName;
-            }
-            catch (Exception exc)
-            {
-                logger.LogWarning($"Failed to get directory of '{path}': {exc}");
-            }
-            return null;
         }
 
         private IEnumerable<string> GetFeeds(Func<IList<string>> getNugetFeeds)
@@ -157,19 +142,16 @@ private IEnumerable<string> GetFeedsFromNugetConfig(string nugetConfigPath) =>
         /// If there are no feeds, a dummy source argument is added to override any default feeds that `restore` would use.
         /// </summary>
         /// <param name="feeds">The list of feeds to use for the restore command.</param>
-        /// <param name="sourceArgumentPrefix">The prefix to use for each source argument (e.g., "-s").</param>
         /// <returns>The list of NuGet sources arguments for the restore command.</returns>
-        public List<string> FeedsToRestoreArgument(IEnumerable<string> feeds, string sourceArgumentPrefix)
+        public List<string> RestoreFeeds(IEnumerable<string> feeds)
         {
             // If there are no feeds, we want to override any default feeds that `restore` would use by passing a dummy source argument.
             if (!feeds.Any())
             {
-                return [sourceArgumentPrefix, emptyPackageDirectory.DirInfo.FullName];
+                return [emptyPackageDirectory.DirInfo.FullName];
             }
 
-            // Add package sources. If any are present, they override all sources specified in
-            // the configuration file(s).
-            return feeds.SelectMany<string, string>(feed => [sourceArgumentPrefix, feed]).ToList();
+            return feeds.ToList();
         }
 
         private IEnumerable<string> FeedsToUseAux(HashSet<string> feedsToConsider)
@@ -196,30 +178,20 @@ private IEnumerable<string> FeedsToUseAux(HashSet<string> feedsToConsider)
         public IEnumerable<string> FeedsToUse(string path)
         {
             // Find the path specific feeds.
-            var folder = GetDirectoryName(path);
+            var folder = feedManagerIo.GetDirectoryName(path);
             var feedsToConsider = folder is not null ? GetFeedsFromFolder(folder).ToHashSet() : new HashSet<string>();
 
             return FeedsToUseAux(feedsToConsider);
         }
 
-        /// <summary>
-        /// Constructs the NuGet sources argument for the `dotnet restore` command based on the given feeds.
-        /// </summary>
-        /// <param name="feeds">The list of NuGet feeds to use for the restore command.</param>
-        /// <returns>A list representing the NuGet sources arguments for the `dotnet restore` command.</returns>
-        public List<string> FeedsToDotnetRestoreArgument(IEnumerable<string> feeds)
-        {
-            return FeedsToRestoreArgument(feeds, "-s");
-        }
-
         /// <summary>
         /// Constructs the list of NuGet sources to use for dotnet restore.
         /// (1) Use the feeds we get from `dotnet nuget list source`
         /// (2) Use private registries, if they are configured
         /// </summary>
         /// <param name="path">Path to project/solution</param>
         /// <returns>A list representing the NuGet sources arguments for the `dotnet restore` command.</returns>
-        public List<string> MakeDotnetRestoreSourcesArguments(string path)
+        public List<string> MakeRestoreFeeds(string path)
         {
             // Do not construct a set of explicit NuGet sources to use for restore.
             if (!CheckNugetFeedResponsiveness && !HasPrivateRegistryFeeds)
@@ -229,7 +201,7 @@ public List<string> MakeDotnetRestoreSourcesArguments(string path)
 
             var feedsToUse = FeedsToUse(path);
 
-            return FeedsToDotnetRestoreArgument(feedsToUse);
+            return RestoreFeeds(feedsToUse);
         }
 
         private (int initialTimeout, int tryCount) GetFeedRequestSettings(bool isFallback)
@@ -251,76 +223,6 @@ public List<string> MakeDotnetRestoreSourcesArguments(string path)
             return (timeoutMilliSeconds, tryCount);
         }
 
-        private static async Task<HttpResponseMessage> ExecuteGetRequest(string address, HttpClient httpClient, CancellationToken cancellationToken)
-        {
-            return await httpClient.GetAsync(address, HttpCompletionOption.ResponseHeadersRead, cancellationToken);
-        }
-
-        private bool IsFeedReachable(string feed, int timeoutMilliSeconds, int tryCount)
-        {
-            logger.LogInfo($"Checking if NuGet feed '{feed}' is reachable...");
-
-            // Configure the HttpClient to be aware of the Dependabot Proxy, if used.
-            HttpClientHandler httpClientHandler = new();
-            if (dependabotProxy != null)
-            {
-                httpClientHandler.Proxy = new WebProxy(dependabotProxy.Address);
-
-                if (dependabotProxy.Certificate != null)
-                {
-                    httpClientHandler.ServerCertificateCustomValidationCallback = (message, cert, chain, _) =>
-                    {
-                        if (chain is null || cert is null)
-                        {
-                            var msg = cert is null && chain is null
-                                ? "certificate and chain"
-                                : chain is null
-                                    ? "chain"
-                                    : "certificate";
-                            logger.LogWarning($"Dependabot proxy certificate validation failed due to missing {msg}");
-                            return false;
-                        }
-                        chain.ChainPolicy.TrustMode = X509ChainTrustMode.CustomRootTrust;
-                        chain.ChainPolicy.CustomTrustStore.Add(dependabotProxy.Certificate);
-                        return chain.Build(cert);
-                    };
-                }
-            }
-
-            using HttpClient client = new(httpClientHandler);
-
-            for (var i = 0; i < tryCount; i++)
-            {
-                using var cts = new CancellationTokenSource();
-                cts.CancelAfter(timeoutMilliSeconds);
-                try
-                {
-                    logger.LogInfo($"Attempt {i + 1}/{tryCount} to reach NuGet feed '{feed}'.");
-                    using var response = ExecuteGetRequest(feed, client, cts.Token).GetAwaiter().GetResult();
-                    response.EnsureSuccessStatusCode();
-                    logger.LogInfo($"Querying NuGet feed '{feed}' succeeded.");
-                    return true;
-                }
-                catch (Exception exc)
-                {
-                    if (exc is TaskCanceledException tce &&
-                        tce.CancellationToken == cts.Token &&
-                        cts.Token.IsCancellationRequested)
-                    {
-                        logger.LogInfo($"Didn't receive answer from NuGet feed '{feed}' in {timeoutMilliSeconds}ms.");
-                        timeoutMilliSeconds *= 2;
-                        continue;
-                    }
-
-                    logger.LogInfo($"Querying NuGet feed '{feed}' failed. The reason for the failure: {exc.Message}");
-                    return false;
-                }
-            }
-
-            logger.LogWarning($"Didn't receive answer from NuGet feed '{feed}'. Tried it {tryCount} times.");
-            return false;
-        }
-
         /// <summary>
         /// Retrieves a list of excluded NuGet feeds from the corresponding environment variable.
         /// </summary>
@@ -374,7 +276,7 @@ public bool IsDefaultFeedReachable()
             if (CheckNugetFeedResponsiveness)
             {
                 var (initialTimeout, tryCount) = GetFeedRequestSettings(isFallback: false);
-                return IsFeedReachable(PublicNugetOrgFeed, initialTimeout, tryCount);
+                return feedManagerIo.IsFeedReachable(PublicNugetOrgFeed, initialTimeout, tryCount);
             }
 
             return true;
@@ -393,7 +295,7 @@ private List<string> GetReachableNuGetFeeds(HashSet<string> feedsToCheck, bool i
 
             var (initialTimeout, tryCount) = GetFeedRequestSettings(isFallback);
             var reachableFeeds = feedsToCheck
-                .Where(feed => IsFeedReachable(feed, initialTimeout, tryCount))
+                .Where(feed => feedManagerIo.IsFeedReachable(feed, initialTimeout, tryCount))
                 .ToList();
 
             if (reachableFeeds.Count == 0)
@@ -477,7 +379,7 @@ private ImmutableHashSet<string> GetAllFeeds()
             if (nugetConfigs.Count > 0)
             {
                 var nugetConfigFeeds = nugetConfigs
-                    .Select(GetDirectoryName)
+                    .Select(feedManagerIo.GetDirectoryName)
                     .Where(folder => folder != null)
                     .SelectMany(folder => GetFeedsFromFolder(folder!))
                     .ToHashSet();