Pauldorsch/fix invalid version bug (#1232)

* catch exceptions thrown from manual dependency scanning

* handle argument exceptions thrown, skipping those packages

* whitespace

* pr feedback

src/Microsoft.ComponentDetection.Detectors/pip/Contracts/PipDependencySpecification.cs

@@ -5,6 +5,7 @@ using System.Collections.Generic;
 using System.Diagnostics;
 using System.Linq;
 using System.Text.RegularExpressions;
+using Microsoft.Extensions.Logging;
 
 /// <summary>
 /// Represents a package and a list of dependency specifications that the package must be.
@@ -59,6 +60,37 @@ public class PipDependencySpecification
     /// <param name="packageString">The <see cref="string"/> to parse.</param>
     /// <param name="requiresDist">The package format.</param>
     public PipDependencySpecification(string packageString, bool requiresDist = false)
+        => this.Initialize(packageString, requiresDist);
+
+    /// <summary>
+    /// Constructs a dependency specification from a string in one of two formats (Requires-Dist: a (==1.3)) OR a==1.3.
+    /// </summary>
+    /// <param name="packageString">The <see cref="string"/> to parse.</param>
+    /// <param name="requiresDist">The package format.</param>
+    /// <param name="logger">The logger used for events realting to the pip dependency specification.</param>
+    public PipDependencySpecification(ILogger logger, string packageString, bool requiresDist = false)
+    {
+        this.Logger = logger;
+        this.Initialize(packageString, requiresDist);
+    }
+
+    /// <summary>
+    /// Gets or sets the package <see cref="Name"/> (ex: pyyaml).
+    /// </summary>
+    public string Name { get; set; }
+
+    /// <summary>
+    /// Gets or sets the set of dependency specifications that constrain the overall dependency request (ex: ==1.0, >=2.0).
+    /// </summary>
+    public IList<string> DependencySpecifiers { get; set; } = new List<string>();
+
+    public IList<string> ConditionalDependencySpecifiers { get; set; } = new List<string>();
+
+    private string DebuggerDisplay => $"{this.Name} ({string.Join(';', this.DependencySpecifiers)})";
+
+    private ILogger Logger { get; set; }
+
+    private void Initialize(string packageString, bool requiresDist)
     {
         if (requiresDist)
         {
@@ -116,20 +148,6 @@ public class PipDependencySpecification
             .ToList();
     }
 
-    /// <summary>
-    /// Gets or sets the package <see cref="Name"/> (ex: pyyaml).
-    /// </summary>
-    public string Name { get; set; }
-
-    /// <summary>
-    /// Gets or sets the set of dependency specifications that constrain the overall dependency request (ex: ==1.0, >=2.0).
-    /// </summary>
-    public IList<string> DependencySpecifiers { get; set; } = new List<string>();
-
-    public IList<string> ConditionalDependencySpecifiers { get; set; } = new List<string>();
-
-    private string DebuggerDisplay => $"{this.Name} ({string.Join(';', this.DependencySpecifiers)})";
-
     /// <summary>
     /// Whether or not the package is safe to resolve based on the packagesToIgnore.
     /// </summary>
@@ -142,7 +160,7 @@ public class PipDependencySpecification
     /// <summary>
     /// Whether or not the package is safe to resolve based on the packagesToIgnore.
     /// </summary>
-    /// <returns> True if the package is unsafe, otherwise false. </returns>
+    /// <returns> True if the package meets all conditions.</returns>
     public bool PackageConditionsMet(Dictionary<string, string> pythonEnvironmentVariables)
     {
         var conditionalRegex = new Regex(@"(and|or)?\s*(\S+)\s*(<=|>=|<|>|===|==|!=|~=)\s*['""]?([^'""]+)['""]?", RegexOptions.Compiled);
@@ -166,8 +184,19 @@ public class PipDependencySpecification
                 if (pythonVersion.Valid)
                 {
                     var conditionalSpec = $"{conditionalOperator}{conditionalValue}";
+                    try
+                    {
                         conditionMet = PythonVersionUtilities.VersionValidForSpec(pythonEnvironmentVariables[conditionalVar], new List<string> { conditionalSpec });
                     }
+                    catch (ArgumentException ae)
+                    {
+                        conditionMet = false;
+                        if (this.Logger != null)
+                        {
+                            this.Logger.LogDebug("Could not create pip dependency: {ErrorMessage}", ae.Message);
+                        }
+                    }
+                }
                 else
                 {
                     conditionMet = pythonEnvironmentVariables[conditionalVar] == conditionalValue;
@@ -214,15 +243,27 @@ public class PipDependencySpecification
             .Where(x => !string.IsNullOrEmpty(x))
             .ToList();
 
+        try
+        {
             var topVersion = versions
                 .Where(x => PythonVersionUtilities.VersionValidForSpec(x, this.DependencySpecifiers))
                 .Select(x => (Version: x, PythonVersion: PythonVersion.Create(x)))
                 .Where(x => x.PythonVersion.Valid)
                 .OrderByDescending(x => x.PythonVersion)
-            .FirstOrDefault();
+                .FirstOrDefault((null, null));
 
             return topVersion.Version;
         }
+        catch (ArgumentException ae)
+        {
+            if (this.Logger != null)
+            {
+                this.Logger.LogDebug("Could not create pip dependency: {ErrorMessage}", ae.Message);
+            }
+
+            return null;
+        }
+    }
 
     /// <summary>
     /// Common method that can be used to determine whether this package is a valid parent

src/Microsoft.ComponentDetection.Detectors/pip/PipComponentDetector.cs

@@ -75,7 +75,8 @@ public class PipComponentDetector : FileComponentDetector, IDefaultOffComponentD
             var initialPackages = await this.pythonCommandService.ParseFileAsync(file.Location, pythonExePath);
             var listedPackage = SharedPipUtilities.ParsedPackagesToPipDependencies(
                     initialPackages,
-                    this.pythonResolver.GetPythonEnvironmentVariables())
+                    this.pythonResolver.GetPythonEnvironmentVariables(),
+                    this.Logger)
                 .ToList();
 
             var roots = await this.pythonResolver.ResolveRootsAsync(singleFileComponentRecorder, listedPackage);

src/Microsoft.ComponentDetection.Detectors/pip/PipReportComponentDetector.cs

@@ -294,9 +294,19 @@ public class PipReportComponentDetector : FileComponentDetector
             // if pipreport fails, try to at least list the dependencies that are found in the source files
             if (this.GetPipReportOverrideBehavior() != PipReportOverrideBehavior.SourceCodeScan && !this.PipReportSkipFallbackOnFailure())
             {
-                this.Logger.LogInformation("PipReport: Trying to Manually compile dependency list for '{File}' without reaching out to a remote feed.", file.Location);
+                try
+                {
+                    this.Logger.LogInformation(
+                        "PipReport: Trying to manually compile package list for '{File}' without reaching out to a remote feed. " +
+                        "This will NOT create a dependency graph, so should be avoided unless absolutely necessary.",
+                        file.Location);
                     await this.RegisterExplicitComponentsInFileAsync(singleFileComponentRecorder, file.Location, pythonExePath);
                 }
+                catch (Exception ex)
+                {
+                    this.Logger.LogWarning(ex, "PipReport: Failed to manually compile package list for '{File}'.", file.Location);
+                }
+            }
         }
         finally
         {
@@ -366,7 +376,7 @@ public class PipReportComponentDetector : FileComponentDetector
                 // cffi (>=1.12)
                 // futures; python_version <= \"2.7\"
                 // sphinx (!=1.8.0,!=3.1.0,!=3.1.1,>=1.6.5) ; extra == 'docs'
-                var dependencySpec = new PipDependencySpecification($"Requires-Dist: {dependency}", requiresDist: true);
+                var dependencySpec = new PipDependencySpecification(this.Logger, $"Requires-Dist: {dependency}", requiresDist: true);
                 if (!dependencySpec.IsValidParentPackage(pythonEnvVars))
                 {
                     continue;
@@ -459,7 +469,8 @@ public class PipReportComponentDetector : FileComponentDetector
 
         var listedPackage = SharedPipUtilities.ParsedPackagesToPipDependencies(
                 initialPackages,
-                this.pythonResolver.GetPythonEnvironmentVariables())
+                this.pythonResolver.GetPythonEnvironmentVariables(),
+                this.Logger)
             .ToList();
 
         listedPackage.Select(x => (x.Name, Version: x.GetHighestExplicitPackageVersion()))
@@ -486,7 +497,8 @@ public class PipReportComponentDetector : FileComponentDetector
             var initialPackages = await this.pythonCommandService.ParseFileAsync(filePath, pythonExePath);
             var listedPackage = SharedPipUtilities.ParsedPackagesToPipDependencies(
                     initialPackages,
-                    this.pythonResolver.GetPythonEnvironmentVariables())
+                    this.pythonResolver.GetPythonEnvironmentVariables(),
+                    this.Logger)
                 .Select(x => x.Name)
                 .ToImmutableSortedSet();
 

src/Microsoft.ComponentDetection.Detectors/pip/SharedPipUtilities.cs

@@ -3,6 +3,7 @@ namespace Microsoft.ComponentDetection.Detectors.Pip;
 using System.Collections.Generic;
 using System.Linq;
 using Microsoft.ComponentDetection.Contracts.TypedComponent;
+using Microsoft.Extensions.Logging;
 
 public static class SharedPipUtilities
 {
@@ -12,14 +13,16 @@ public static class SharedPipUtilities
     /// </summary>
     /// <param name="parsedPackages">List of packages and git components.</param>
     /// <param name="pythonEnvironmentVariables">Python environment specifiers.</param>
+    /// <param name="logger">Logger for pip dependency specification.</param>
     /// <returns>Enumerable containing the converted, sanitized Pip dependency specs.</returns>
     public static IEnumerable<PipDependencySpecification> ParsedPackagesToPipDependencies(
         IList<(string PackageString, GitComponent Component)> parsedPackages,
-        Dictionary<string, string> pythonEnvironmentVariables) =>
+        Dictionary<string, string> pythonEnvironmentVariables,
+        ILogger logger) =>
             parsedPackages.Where(tuple => tuple.PackageString != null)
                 .Select(tuple => tuple.PackageString)
                 .Where(x => !string.IsNullOrWhiteSpace(x))
-                .Select(x => new PipDependencySpecification(x))
+                .Select(x => new PipDependencySpecification(logger, x, false))
                 .Where(x => !x.PackageIsUnsafe())
                 .Where(x => x.PackageConditionsMet(pythonEnvironmentVariables));
 }

src/Microsoft.ComponentDetection.Detectors/pip/SimplePipComponentDetector.cs

@@ -68,7 +68,7 @@ public class SimplePipComponentDetector : FileComponentDetector, IDefaultOffComp
             var listedPackage = initialPackages.Where(tuple => tuple.PackageString != null)
                 .Select(tuple => tuple.PackageString)
                 .Where(x => !string.IsNullOrWhiteSpace(x))
-                .Select(x => new PipDependencySpecification(x))
+                .Select(x => new PipDependencySpecification(x, false))
                 .Where(x => !x.PackageIsUnsafe())
                 .ToList();
 

test/Microsoft.ComponentDetection.Detectors.Tests/PipDependencySpecifierTests.cs

@@ -161,4 +161,43 @@ public class PipDependencySpecifierTests
 
         VerifyPipConditionalDependencyParsing(specs, pythonEnvironmentVariables, true);
     }
+
+    [TestMethod]
+    public void TestPipDependencyGetHighestExplicitPackageVersion_Valid()
+    {
+        var spec = new PipDependencySpecification
+        {
+            Name = "TestPackage",
+            DependencySpecifiers = new List<string> { ">=1.0", "<=3.0", "!=2.0", "!=4.0" },
+        };
+
+        var highestVersion = spec.GetHighestExplicitPackageVersion();
+        highestVersion.Should().Be("3.0");
+    }
+
+    [TestMethod]
+    public void TestPipDependencyGetHighestExplicitPackageVersion_SingleInvalidSpec()
+    {
+        var spec = new PipDependencySpecification
+        {
+            Name = "TestPackage",
+            DependencySpecifiers = new List<string> { ">=1.0", "info", "!=2.0", "!=4.0" },
+        };
+
+        var highestVersion = spec.GetHighestExplicitPackageVersion();
+        highestVersion.Should().BeNull();
+    }
+
+    [TestMethod]
+    public void TestPipDependencyGetHighestExplicitPackageVersion_AllInvalidSpec()
+    {
+        var spec = new PipDependencySpecification
+        {
+            Name = "TestPackage",
+            DependencySpecifiers = new List<string> { "info" },
+        };
+
+        var highestVersion = spec.GetHighestExplicitPackageVersion();
+        highestVersion.Should().BeNull();
+    }
 }

test/Microsoft.ComponentDetection.Detectors.Tests/PipReportComponentDetectorTests.cs

@@ -522,12 +522,72 @@ public class PipReportComponentDetectorTests : BaseDetectorTest<PipReportCompone
         gitComponent.RepositoryUrl.Should().Be("https://github.com/example/example");
         gitComponent.CommitHash.Should().Be("deadbee");
 
-        this.mockLogger.Verify(x => x.Log(
+        this.mockLogger.Verify(
+            x => x.Log(
                 LogLevel.Information,
                 It.IsAny<EventId>(),
                 It.Is<It.IsAnyType>((o, t) => o.ToString().StartsWith("PipReport: Found PipReportOverrideBehavior environment variable set to SourceCodeScan.")),
                 It.IsAny<Exception>(),
-            (Func<It.IsAnyType, Exception, string>)It.IsAny<object>()));
+                (Func<It.IsAnyType, Exception, string>)It.IsAny<object>()),
+            Times.Exactly(2));
+    }
+
+    [TestMethod]
+    public async Task TestPipReportDetector_FallbackAsync()
+    {
+        this.pythonCommandService.Setup(x => x.PythonExistsAsync(It.IsAny<string>())).ReturnsAsync(true);
+
+        var baseSetupPyDependencies = this.ToGitTuple(new List<string> { "a==1.0", "b>=2.0,!=2.1,<3.0.0", "c!=1.1", "y==invalidversion" });
+        var baseRequirementsTextDependencies = this.ToGitTuple(new List<string> { "d~=1.0", "e<=2.0", "f===1.1", "g<3.0", "h>=1.0,<=3.0,!=2.0,!=4.0", "z==anotherinvalidversion" });
+        baseRequirementsTextDependencies.Add((null, new GitComponent(new Uri("https://github.com/example/example"), "deadbee")));
+
+        this.pythonCommandService.Setup(x => x.ParseFileAsync(Path.Join(Path.GetTempPath(), "setup.py"), null)).ReturnsAsync(baseSetupPyDependencies);
+        this.pythonCommandService.Setup(x => x.ParseFileAsync(Path.Join(Path.GetTempPath(), "requirements.txt"), null)).ReturnsAsync(baseRequirementsTextDependencies);
+
+        this.pipCommandService.Setup(x => x.GenerateInstallationReportAsync(
+            It.Is<string>(s => s.Contains("requirements.txt", StringComparison.OrdinalIgnoreCase)),
+            It.IsAny<string>(),
+            It.IsAny<string>(),
+            It.IsAny<CancellationToken>()))
+            .ThrowsAsync(new InvalidOperationException("Want to fallback, so fail initial report generation"));
+
+        var (result, componentRecorder) = await this.DetectorTestUtility
+            .WithFile("setup.py", string.Empty)
+            .WithFile("requirements.txt", string.Empty)
+            .ExecuteDetectorAsync();
+
+        result.ResultCode.Should().Be(ProcessingResultCode.Success);
+
+        var detectedComponents = componentRecorder.GetDetectedComponents();
+        detectedComponents.Should().HaveCount(7);
+
+        var pipComponents = detectedComponents.Where(detectedComponent => detectedComponent.Component.Id.Contains("pip")).ToList();
+        ((PipComponent)pipComponents.Single(x => ((PipComponent)x.Component).Name == "a").Component).Version.Should().Be("1.0");
+        ((PipComponent)pipComponents.Single(x => ((PipComponent)x.Component).Name == "b").Component).Version.Should().Be("2.0");
+        ((PipComponent)pipComponents.Single(x => ((PipComponent)x.Component).Name == "d").Component).Version.Should().Be("1.0");
+        ((PipComponent)pipComponents.Single(x => ((PipComponent)x.Component).Name == "e").Component).Version.Should().Be("2.0");
+        ((PipComponent)pipComponents.Single(x => ((PipComponent)x.Component).Name == "f").Component).Version.Should().Be("1.1");
+        ((PipComponent)pipComponents.Single(x => ((PipComponent)x.Component).Name == "h").Component).Version.Should().Be("3.0");
+        pipComponents.Should().NotContain(x => ((PipComponent)x.Component).Name == "c");
+        pipComponents.Should().NotContain(x => ((PipComponent)x.Component).Name == "g");
+        pipComponents.Should().NotContain(x => ((PipComponent)x.Component).Name == "y");
+        pipComponents.Should().NotContain(x => ((PipComponent)x.Component).Name == "z");
+
+        this.mockLogger.Verify(
+            x => x.Log(
+                LogLevel.Debug,
+                It.IsAny<EventId>(),
+                It.Is<It.IsAnyType>((o, t) => o.ToString().StartsWith("Could not create pip dependency: invalidversion is not a valid python version")),
+                It.IsAny<Exception>(),
+                (Func<It.IsAnyType, Exception, string>)It.IsAny<object>()),
+            Times.Once);
+
+        var gitComponents = detectedComponents.Where(detectedComponent => detectedComponent.Component.Type == ComponentType.Git);
+        gitComponents.Should().ContainSingle();
+        var gitComponent = (GitComponent)gitComponents.Single().Component;
+
+        gitComponent.RepositoryUrl.Should().Be("https://github.com/example/example");
+        gitComponent.CommitHash.Should().Be("deadbee");
     }
 
     [TestMethod]

test/Microsoft.ComponentDetection.VerificationTests/resources/pip/fallback/requirements.txt

@@ -1,2 +1,3 @@
 fakepythonpackage==1.2.3
 zipp>=3.2.0,<4.0
+badpackage==info