Skip to content

Commit a623268

Browse files
ericstjericstj
committed
Address feedback
1 parent c16984c commit a623268

File tree

3 files changed

+120
-62
lines changed

3 files changed

+120
-62
lines changed

‎src/Microsoft.ComponentDetection.Contracts/TypedComponent/DotNetComponent.cs

+15-34
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,6 @@
11
namespace Microsoft.ComponentDetection.Contracts.TypedComponent;
22

3-
using System.Text;
4-
5-
#nullable enable
6-
7-
using PackageUrl;
3+
using System;
84

95
public class DotNetComponent : TypedComponent
106
{
@@ -13,52 +9,37 @@ private DotNetComponent()
139
/* Reserved for deserialization */
1410
}
1511

16-
public DotNetComponent(string? sdkVersion, string? targetFramework = null, string? projectType = null)
12+
public DotNetComponent(string sdkVersion, string targetFramework = null, string projectType = null)
1713
{
18-
this.SdkVersion = sdkVersion;
19-
this.TargetFramework = targetFramework;
20-
this.ProjectType = projectType; // application, library, or null
14+
if (string.IsNullOrWhiteSpace(sdkVersion) && string.IsNullOrWhiteSpace(targetFramework))
15+
{
16+
throw new ArgumentNullException(nameof(sdkVersion), $"Either {nameof(sdkVersion)} or {nameof(targetFramework)} of component type {nameof(DotNetComponent)} must be specified.");
17+
}
18+
19+
this.SdkVersion = string.IsNullOrWhiteSpace(sdkVersion) ? "unknown" : sdkVersion;
20+
this.TargetFramework = string.IsNullOrWhiteSpace(targetFramework) ? "unknown" : targetFramework;
21+
this.ProjectType = string.IsNullOrWhiteSpace(projectType) ? "unknown" : projectType;
2122
}
2223

2324
/// <summary>
2425
/// SDK Version detected, could be null if no global.json exists and no dotnet is on the path.
2526
/// </summary>
26-
public string? SdkVersion { get; set; }
27+
public string SdkVersion { get; set; }
2728

2829
/// <summary>
2930
/// Target framework for this instance. Null in the case of global.json.
3031
/// </summary>
31-
public string? TargetFramework { get; set; }
32+
public string TargetFramework { get; set; }
3233

3334
/// <summary>
3435
/// Project type: application, library. Null in the case of global.json or if no project output could be discovered.
3536
/// </summary>
36-
public string? ProjectType { get; set; }
37+
public string ProjectType { get; set; }
3738

3839
public override ComponentType Type => ComponentType.DotNet;
3940

4041
/// <summary>
41-
/// Provides an id like `dotnet {SdkVersion} - {TargetFramework} - {ProjectType}` where targetFramework and projectType are only present if not null.
42+
/// Provides an id like `{SdkVersion} - {TargetFramework} - {ProjectType} - dotnet` where unspecified values are represented as 'unknown'.
4243
/// </summary>
43-
public override string Id
44-
{
45-
get
46-
{
47-
var builder = new StringBuilder($"dotnet {this.SdkVersion ?? "unknown"}");
48-
if (this.TargetFramework is not null)
49-
{
50-
builder.Append($" - {this.TargetFramework}");
51-
52-
if (this.ProjectType is not null)
53-
{
54-
builder.Append($" - {this.ProjectType}");
55-
}
56-
}
57-
58-
return builder.ToString();
59-
}
60-
}
61-
62-
// TODO - do we need to add a type to prul https://github.com/package-url/purl-spec/blob/main/PURL-TYPES.rst
63-
public override PackageURL PackageUrl => new PackageURL("generic", null, "dotnet-sdk", this.SdkVersion ?? "unknown", null, null);
44+
public override string Id => $"{this.SdkVersion} {this.TargetFramework} {this.ProjectType} - {this.Type}";
6445
}

‎src/Microsoft.ComponentDetection.Detectors/dotnet/DotNetComponentDetector.cs

+45-10
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ namespace Microsoft.ComponentDetection.Detectors.DotNet;
22

33
#nullable enable
44
using System;
5+
using System.Collections.Concurrent;
56
using System.Collections.Generic;
67
using System.IO;
78
using System.Linq;
@@ -23,7 +24,7 @@ public class DotNetComponentDetector : FileComponentDetector, IExperimentalDetec
2324
private readonly IFileUtilityService fileUtilityService;
2425
private readonly IPathUtilityService pathUtilityService;
2526
private readonly LockFileFormat lockFileFormat = new();
26-
private readonly Dictionary<string, string?> sdkVersionCache = [];
27+
private readonly ConcurrentDictionary<string, string?> sdkVersionCache = [];
2728
private string? sourceDirectory;
2829
private string? sourceFileRootDirectory;
2930

@@ -60,7 +61,15 @@ public DotNetComponentDetector(
6061
var workingDirectory = new DirectoryInfo(workingDirectoryPath);
6162

6263
var process = await this.commandLineInvocationService.ExecuteCommandAsync("dotnet", ["dotnet.exe"], workingDirectory, cancellationToken, "--version").ConfigureAwait(false);
63-
return process.ExitCode == 0 ? process.StdOut.Trim() : null;
64+
65+
if (process.ExitCode != 0)
66+
{
67+
// debug only - it could be that dotnet is not actually on the path and specified directly by the build scripts.
68+
this.Logger.LogDebug("Failed to invoke 'dotnet --version'. Return: {Return} StdErr: {StdErr} StdOut: {StdOut}.", process.ExitCode, process.StdErr, process.StdOut);
69+
return null;
70+
}
71+
72+
return process.StdOut.Trim();
6473
}
6574

6675
public override Task<IndividualDetectorScanResult> ExecuteDetectorAsync(ScanRequest request, CancellationToken cancellationToken = default)
@@ -76,11 +85,24 @@ protected override async Task OnFileFoundAsync(ProcessRequest processRequest, ID
7685
var lockFile = this.lockFileFormat.Read(processRequest.ComponentStream.Stream, processRequest.ComponentStream.Location);
7786

7887
var projectPath = lockFile.PackageSpec.RestoreMetadata.ProjectPath;
88+
89+
if (!this.fileUtilityService.Exists(projectPath))
90+
{
91+
// Could be the assets file was not actually from this build
92+
this.Logger.LogWarning("Project path {ProjectPath} specified by {ProjectAssetsPath} does not exist.", projectPath, processRequest.ComponentStream.Location);
93+
}
94+
7995
var projectDirectory = this.pathUtilityService.GetParentDirectory(projectPath);
8096
var sdkVersion = await this.GetSdkVersionAsync(projectDirectory, cancellationToken);
8197

8298
var projectName = lockFile.PackageSpec.RestoreMetadata.ProjectName;
8399
var projectOutputPath = lockFile.PackageSpec.RestoreMetadata.OutputPath;
100+
101+
if (!this.directoryUtilityService.Exists(projectOutputPath))
102+
{
103+
this.Logger.LogWarning("Project output path {ProjectOutputPath} specified by {ProjectAssetsPath} does not exist.", projectOutputPath, processRequest.ComponentStream.Location);
104+
}
105+
84106
var targetType = this.GetProjectType(projectOutputPath, projectName, cancellationToken);
85107

86108
var componentReporter = this.ComponentRecorder.CreateSingleFileComponentRecorder(projectPath);
@@ -126,8 +148,9 @@ private bool IsApplication(string assemblyPath)
126148
// despite the name `IsExe` this is actually based of the CoffHeader Characteristics
127149
return peReader.PEHeaders.IsExe;
128150
}
129-
catch (Exception)
151+
catch (Exception e)
130152
{
153+
this.Logger.LogWarning("Failed to open output assembly {AssemblyPath} error {Message}.", assemblyPath, e.Message);
131154
return false;
132155
}
133156
}
@@ -140,6 +163,12 @@ private bool IsApplication(string assemblyPath)
140163
/// <returns>Sdk version found, or null if no version can be detected.</returns>
141164
private async Task<string?> GetSdkVersionAsync(string projectDirectory, CancellationToken cancellationToken)
142165
{
166+
if (string.IsNullOrWhiteSpace(projectDirectory))
167+
{
168+
// not expected
169+
return null;
170+
}
171+
143172
// normalize since we need to use as a key
144173
projectDirectory = this.pathUtilityService.NormalizePath(projectDirectory);
145174
if (this.sdkVersionCache.TryGetValue(projectDirectory, out var sdkVersion))
@@ -152,17 +181,23 @@ private bool IsApplication(string assemblyPath)
152181

153182
if (this.fileUtilityService.Exists(globalJsonPath))
154183
{
155-
var globalJson = await JsonDocument.ParseAsync(this.fileUtilityService.MakeFileStream(globalJsonPath), cancellationToken: cancellationToken);
156-
if (globalJson.RootElement.TryGetProperty("sdk", out var sdk))
184+
sdkVersion = await this.RunDotNetVersionAsync(projectDirectory, cancellationToken);
185+
186+
if (string.IsNullOrWhiteSpace(sdkVersion))
157187
{
158-
if (sdk.TryGetProperty("version", out var version))
188+
var globalJson = await JsonDocument.ParseAsync(this.fileUtilityService.MakeFileStream(globalJsonPath), cancellationToken: cancellationToken);
189+
if (globalJson.RootElement.TryGetProperty("sdk", out var sdk))
159190
{
160-
sdkVersion = version.GetString();
161-
var globalJsonComponent = new DetectedComponent(new DotNetComponent(sdkVersion));
162-
var recorder = this.ComponentRecorder.CreateSingleFileComponentRecorder(globalJsonPath);
163-
recorder.RegisterUsage(globalJsonComponent, isExplicitReferencedDependency: true);
191+
if (sdk.TryGetProperty("version", out var version))
192+
{
193+
sdkVersion = version.GetString();
194+
}
164195
}
165196
}
197+
198+
var globalJsonComponent = new DetectedComponent(new DotNetComponent(sdkVersion));
199+
var recorder = this.ComponentRecorder.CreateSingleFileComponentRecorder(globalJsonPath);
200+
recorder.RegisterUsage(globalJsonComponent, isExplicitReferencedDependency: true);
166201
}
167202
else if (projectDirectory.Equals(this.sourceDirectory, StringComparison.OrdinalIgnoreCase) ||
168203
projectDirectory.Equals(this.sourceFileRootDirectory, StringComparison.OrdinalIgnoreCase) ||

‎test/Microsoft.ComponentDetection.Detectors.Tests/DotNetComponentDetectorTests.cs

+60-18
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
namespace Microsoft.ComponentDetection.Detectors.Tests;
22

3+
using System;
34
using System.Collections.Generic;
45
using System.IO;
56
using System.Linq;
@@ -43,6 +44,8 @@ public class DotNetComponentDetectorTests : BaseDetectorTest<DotNetComponentDete
4344
// uses GetParentDirectory, NormalizePath
4445
private readonly Mock<IPathUtilityService> mockPathUtilityService = new();
4546

47+
private Func<string, DirectoryInfo, CommandLineExecutionResult> commandLineCallback;
48+
4649
/// <summary>
4750
/// Initializes a new instance of the <see cref="DotNetComponentDetectorTests"/> class.
4851
/// </summary>
@@ -64,7 +67,8 @@ public DotNetComponentDetectorTests()
6467
this.mockPathUtilityService.Setup(x => x.NormalizePath(It.IsAny<string>())).Returns((string p) => p); // don't do normalization
6568
this.mockPathUtilityService.Setup(x => x.GetParentDirectory(It.IsAny<string>())).Returns((string p) => Path.GetDirectoryName(p));
6669

67-
this.mockCommandLineInvocationService.Setup(x => x.ExecuteCommandAsync(It.IsAny<string>(), It.IsAny<IEnumerable<string>>(), It.IsAny<DirectoryInfo>(), It.IsAny<CancellationToken>(), It.IsAny<string[]>())).Returns(Task.FromResult(this.commandLineExecutionResult));
70+
this.mockCommandLineInvocationService.Setup(x => x.ExecuteCommandAsync(It.IsAny<string>(), It.IsAny<IEnumerable<string>>(), It.IsAny<DirectoryInfo>(), It.IsAny<CancellationToken>(), It.IsAny<string[]>()))
71+
.Returns((string c, IEnumerable<string> ac, DirectoryInfo d, CancellationToken ct, string[] args) => Task.FromResult(this.CommandResult(c, d)));
6872
}
6973

7074
private bool FileExists(string path)
@@ -147,11 +151,20 @@ private void AddDirectory(string path, string subDirectory = null)
147151

148152
private void SetCommandResult(int exitCode, string stdOut = null, string stdErr = null)
149153
{
154+
this.commandLineCallback = null;
150155
this.commandLineExecutionResult.ExitCode = exitCode;
151156
this.commandLineExecutionResult.StdOut = stdOut;
152157
this.commandLineExecutionResult.StdErr = stdErr;
153158
}
154159

160+
private void SetCommandResult(Func<string, DirectoryInfo, CommandLineExecutionResult> callback)
161+
{
162+
this.commandLineCallback = callback;
163+
}
164+
165+
private CommandLineExecutionResult CommandResult(string command, DirectoryInfo directory) =>
166+
(this.commandLineCallback != null) ? this.commandLineCallback(command, directory) : this.commandLineExecutionResult;
167+
155168
[TestCleanup]
156169
public void ClearMocks()
157170
{
@@ -214,6 +227,31 @@ public async Task TestDotNetDetectorGlobalJson_ReturnsSDKVersion()
214227
var globalJson = GlobalJson("42.0.800");
215228
this.AddFile(projectPath, null);
216229
this.AddFile(Path.Combine(RootDir, "path", "global.json"), globalJson);
230+
this.SetCommandResult(-1);
231+
232+
var (scanResult, componentRecorder) = await this.DetectorTestUtility
233+
.WithFile("project.assets.json", projectAssets)
234+
.ExecuteDetectorAsync();
235+
236+
scanResult.ResultCode.Should().Be(ProcessingResultCode.Success);
237+
238+
var detectedComponents = componentRecorder.GetDetectedComponents();
239+
detectedComponents.Should().HaveCount(2);
240+
241+
var discoveredComponents = detectedComponents.ToArray();
242+
discoveredComponents.Where(component => component.Component.Id == "42.0.800 unknown unknown - DotNet").Should().ContainSingle();
243+
discoveredComponents.Where(component => component.Component.Id == "42.0.800 net8.0 unknown - DotNet").Should().ContainSingle();
244+
}
245+
246+
[TestMethod]
247+
public async Task TestDotNetDetectorGlobalJsonRollForward_ReturnsSDKVersion()
248+
{
249+
var projectPath = Path.Combine(RootDir, "path", "to", "project");
250+
var projectAssets = ProjectAssets("projectName", "does-not-exist", projectPath, "net8.0");
251+
var globalJson = GlobalJson("8.0.100");
252+
this.AddFile(projectPath, null);
253+
this.AddFile(Path.Combine(RootDir, "path", "global.json"), globalJson);
254+
this.SetCommandResult(0, "8.0.808");
217255

218256
var (scanResult, componentRecorder) = await this.DetectorTestUtility
219257
.WithFile("project.assets.json", projectAssets)
@@ -225,8 +263,8 @@ public async Task TestDotNetDetectorGlobalJson_ReturnsSDKVersion()
225263
detectedComponents.Should().HaveCount(2);
226264

227265
var discoveredComponents = detectedComponents.ToArray();
228-
discoveredComponents.Where(component => component.Component.Id == "dotnet 42.0.800").Should().ContainSingle();
229-
discoveredComponents.Where(component => component.Component.Id == "dotnet 42.0.800 - net8.0").Should().ContainSingle();
266+
discoveredComponents.Where(component => component.Component.Id == "8.0.808 unknown unknown - DotNet").Should().ContainSingle();
267+
discoveredComponents.Where(component => component.Component.Id == "8.0.808 net8.0 unknown - DotNet").Should().ContainSingle();
230268
}
231269

232270
[TestMethod]
@@ -247,7 +285,7 @@ public async Task TestDotNetDetectorNoGlobalJson_ReturnsDotNetVersion()
247285
detectedComponents.Should().ContainSingle();
248286

249287
var discoveredComponents = detectedComponents.ToArray();
250-
discoveredComponents.Where(component => component.Component.Id == "dotnet 86.75.309 - net8.0").Should().ContainSingle();
288+
discoveredComponents.Where(component => component.Component.Id == "86.75.309 net8.0 unknown - DotNet").Should().ContainSingle();
251289
}
252290

253291
[TestMethod]
@@ -268,7 +306,7 @@ public async Task TestDotNetDetectorNoGlobalJsonNoDotnet_ReturnsUnknownVersion()
268306
detectedComponents.Should().ContainSingle();
269307

270308
var discoveredComponents = detectedComponents.ToArray();
271-
discoveredComponents.Where(component => component.Component.Id == "dotnet unknown - net8.0").Should().ContainSingle();
309+
discoveredComponents.Where(component => component.Component.Id == "unknown net8.0 unknown - DotNet").Should().ContainSingle();
272310
}
273311

274312
[TestMethod]
@@ -289,20 +327,24 @@ public async Task TestDotNetDetectorMultipleTargetFrameworks()
289327
detectedComponents.Should().HaveCount(3);
290328

291329
var discoveredComponents = detectedComponents.ToArray();
292-
discoveredComponents.Where(component => component.Component.Id == "dotnet 1.2.3 - net8.0").Should().ContainSingle();
293-
discoveredComponents.Where(component => component.Component.Id == "dotnet 1.2.3 - net6.0").Should().ContainSingle();
294-
discoveredComponents.Where(component => component.Component.Id == "dotnet 1.2.3 - netstandard2.0").Should().ContainSingle();
330+
discoveredComponents.Where(component => component.Component.Id == "1.2.3 net8.0 unknown - DotNet").Should().ContainSingle();
331+
discoveredComponents.Where(component => component.Component.Id == "1.2.3 net6.0 unknown - DotNet").Should().ContainSingle();
332+
discoveredComponents.Where(component => component.Component.Id == "1.2.3 netstandard2.0 unknown - DotNet").Should().ContainSingle();
295333
}
296334

297335
[TestMethod]
298336
public async Task TestDotNetDetectorMultipleProjectsWithDifferentOutputTypeAndSdkVersion()
299337
{
300-
// dotnet on the path with be version 1.2.3
301-
this.SetCommandResult(0, "1.2.3");
302-
303338
// dotnet from global.json will be 4.5.6
304339
var globalJson = GlobalJson("4.5.6");
305-
this.AddFile(Path.Combine(RootDir, "path", "global.json"), globalJson);
340+
var globalJsonDir = Path.Combine(RootDir, "path");
341+
this.AddFile(Path.Combine(globalJsonDir, "global.json"), globalJson);
342+
343+
this.SetCommandResult((c, d) => new CommandLineExecutionResult()
344+
{
345+
ExitCode = 0,
346+
StdOut = d.FullName == globalJsonDir ? "4.5.6" : "1.2.3",
347+
});
306348

307349
// set up a library project - under global.json
308350
var libraryProjectName = "library";
@@ -338,11 +380,11 @@ public async Task TestDotNetDetectorMultipleProjectsWithDifferentOutputTypeAndSd
338380
detectedComponents.Should().HaveCount(6);
339381

340382
var discoveredComponents = detectedComponents.ToArray();
341-
discoveredComponents.Where(component => component.Component.Id == "dotnet 4.5.6").Should().ContainSingle();
342-
discoveredComponents.Where(component => component.Component.Id == "dotnet 4.5.6 - net8.0 - library").Should().ContainSingle();
343-
discoveredComponents.Where(component => component.Component.Id == "dotnet 4.5.6 - net6.0 - library").Should().ContainSingle();
344-
discoveredComponents.Where(component => component.Component.Id == "dotnet 4.5.6 - netstandard2.0 - library").Should().ContainSingle();
345-
discoveredComponents.Where(component => component.Component.Id == "dotnet 1.2.3 - net8.0 - application").Should().ContainSingle();
346-
discoveredComponents.Where(component => component.Component.Id == "dotnet 1.2.3 - net48 - application").Should().ContainSingle();
383+
discoveredComponents.Where(component => component.Component.Id == "4.5.6 unknown unknown - DotNet").Should().ContainSingle();
384+
discoveredComponents.Where(component => component.Component.Id == "4.5.6 net8.0 library - DotNet").Should().ContainSingle();
385+
discoveredComponents.Where(component => component.Component.Id == "4.5.6 net6.0 library - DotNet").Should().ContainSingle();
386+
discoveredComponents.Where(component => component.Component.Id == "4.5.6 netstandard2.0 library - DotNet").Should().ContainSingle();
387+
discoveredComponents.Where(component => component.Component.Id == "1.2.3 net8.0 application - DotNet").Should().ContainSingle();
388+
discoveredComponents.Where(component => component.Component.Id == "1.2.3 net48 application - DotNet").Should().ContainSingle();
347389
}
348390
}

0 commit comments

Comments
 (0)