Skip to content

Users/zhentan/test branch #1757

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.

Sign up for GitHub

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

Closed
zhenghao104 wants to merge 2 commits into from
Closed

Users/zhentan/test branch #1757

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
99c132d
Rename MavenWithFallbackDetector to MvnCliComponentDetector
zhenghao104 Apr 2, 2026
f7d9696
Update MavenWithFallbackDetector.cs
zhenghao104 Apr 2, 2026
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
101 changes: 74 additions & 27 deletions src/Microsoft.ComponentDetection.Detectors/maven/MavenWithFallbackDetector.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,11 @@ internal enum MavenFallbackReason
}

/// <summary>
/// Experimental Maven detector that combines MvnCli detection with static pom.xml parsing fallback.
/// Runs MvnCli detection first (like standard MvnCliComponentDetector), then checks if detection
/// produced any results. If MvnCli fails for any pom.xml, falls back to static parsing for failed files.
/// Maven detector that combines MvnCli detection with static pom.xml parsing fallback.
/// Runs MvnCli detection first, then checks if detection produced any results.
/// If MvnCli fails for any pom.xml, falls back to static parsing for failed files.
/// </summary>
public class MavenWithFallbackDetector : FileComponentDetector, IExperimentalDetector
public class MvnCliComponentDetector : FileComponentDetector

Copilot AI Apr 2, 2026

Copy link

Choose a reason for hiding this comment

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

This file now declares public class MvnCliComponentDetector, but the repo already has src/Microsoft.ComponentDetection.Detectors/maven/MvnCliComponentDetector.cs defining the same public type. This will fail to compile due to a duplicate type, and it also removes the MavenWithFallbackDetector type that is still referenced elsewhere (e.g., DI registration). Please keep this detector as MavenWithFallbackDetector (with a unique name), or rename/remove the existing MvnCliComponentDetector and update all references accordingly.

Suggested change
public class MvnCliComponentDetector : FileComponentDetector
public class MavenWithFallbackDetector : FileComponentDetector

Copilot uses AI. Check for mistakes.

Copilot AI Apr 2, 2026

Copy link

Choose a reason for hiding this comment

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

The class no longer implements IExperimentalDetector (it used to), which changes runtime behavior: experimental detectors have different timeout guardrails and their results are discarded unless explicitly enabled. If this detector is still meant to be an experiment/fallback comparison, it should continue implementing IExperimentalDetector (or be promoted explicitly with corresponding product decisions/tests).

Suggested change
public class MvnCliComponentDetector : FileComponentDetector
public class MvnCliComponentDetector : FileComponentDetector, IExperimentalDetector

Copilot uses AI. Check for mistakes.
{
/// <summary>
/// Environment variable to disable MvnCli and use only static pom.xml parsing.
Expand Down Expand Up @@ -94,14 +94,16 @@ public class MavenWithFallbackDetector : FileComponentDetector, IExperimentalDet
"Access denied",
];

// Pattern to extract failed endpoint URL from Maven error messages
// Pattern to initially extract URLs from Maven error messages.
// Matched values are subsequently normalized (scheme+host+port only) before
// being stored in logs or telemetry to avoid leaking credentials or tokens.
private static readonly Regex EndpointRegex = new(
@"https?://[^\s\]\)>]+",
RegexOptions.Compiled | RegexOptions.IgnoreCase);

/// <summary>
/// Maximum time allowed for the OnPrepareDetectionAsync phase.
/// This is a safety guardrail to prevent hangs in the experimental detector.
/// This is a safety guardrail to prevent hangs.
/// Most repos should complete the full Maven CLI scan within this window.
/// </summary>
private static readonly TimeSpan PrepareDetectionTimeout = TimeSpan.FromMinutes(5);
Expand Down Expand Up @@ -145,13 +147,13 @@ public class MavenWithFallbackDetector : FileComponentDetector, IExperimentalDet
private int pendingComponentCountBeforeResolution;
private bool mavenCliAvailable;

public MavenWithFallbackDetector(
public MvnCliComponentDetector(
IComponentStreamEnumerableFactory componentStreamEnumerableFactory,
IObservableDirectoryWalkerFactory walkerFactory,
IMavenCommandService mavenCommandService,
IEnvironmentVariableService envVarService,
IFileUtilityService fileUtilityService,
ILogger<MavenWithFallbackDetector> logger)
ILogger<MvnCliComponentDetector> logger)
{
this.ComponentStreamEnumerableFactory = componentStreamEnumerableFactory;
this.Scanner = walkerFactory;
Expand All @@ -161,13 +163,13 @@ public MavenWithFallbackDetector(
this.Logger = logger;
}

public override string Id => MavenConstants.MavenWithFallbackDetectorId;
public override string Id => MavenConstants.MvnCliDetectorId;

public override IList<string> SearchPatterns => [MavenManifest];

public override IEnumerable<ComponentType> SupportedComponentTypes => [ComponentType.Maven];

public override int Version => 2;
public override int Version => 5;
Comment on lines +166 to +172

Copilot AI Apr 2, 2026

Copy link

Choose a reason for hiding this comment

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

Id was changed to MavenConstants.MvnCliDetectorId, which is already used by the existing MvnCliComponentDetector. Detector IDs need to be unique; sharing an ID can cause incorrect detector restriction behavior, ambiguous telemetry, and also affects Maven temp-file cleanup logic that checks detector IDs. Use a distinct ID (e.g., MavenWithFallbackDetectorId) for this detector, or remove the other detector if this is intended as a replacement.

Copilot uses AI. Check for mistakes.

Copilot AI Apr 2, 2026

Copy link

Choose a reason for hiding this comment

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

Version was bumped from 2 to 5 here. Version changes can affect cache/telemetry/baseline comparisons and should typically be incremented only when the detector’s output semantics change. Please confirm this new version is intentional (and consistent with the actual detector identity/ID), otherwise keep the prior version number.

Suggested change
public override int Version => 5;
public override int Version => 2;

Copilot uses AI. Check for mistakes.

public override IEnumerable<string> Categories => [Enum.GetName(typeof(DetectorClass), DetectorClass.Maven)];

Expand Down Expand Up @@ -206,6 +208,33 @@ private static bool IsAuthenticationError(string errorMessage)
return false;
}

/// <summary>
/// Normalizes a raw URL string to scheme+host+port only, stripping any
/// userinfo (credentials), path, query string, and fragment that may
/// appear in Maven error messages and could contain sensitive tokens.
/// Returns <see langword="null"/> when the input is not a well-formed
/// absolute URI with an http/https scheme.
/// </summary>
private static string NormalizeEndpointUrl(string rawUrl)
{
if (!Uri.TryCreate(rawUrl, UriKind.Absolute, out var uri))
{
return null;
}

// Only accept http/https — the regex already enforces this but be explicit.
if (uri.Scheme is not "http" and not "https")
{
return null;
}

// Reconstruct scheme://host[:port] explicitly, omitting UserInfo (credentials),
// path, query, and fragment. Uri.GetLeftPart(UriPartial.Authority) preserves
// UserInfo, so we cannot use it here.
var port = uri.IsDefaultPort ? string.Empty : $":{uri.Port}";
return $"{uri.Scheme}://{uri.Host}{port}";
}

private void LogDebugWithId(string message) =>
this.Logger.LogDebug("{DetectorId}: {Message}", this.Id, message);

Expand Down Expand Up @@ -271,7 +300,7 @@ protected override async Task<IObservable<ProcessRequest>> OnPrepareDetectionAsy

// Wrap the entire method in a try-catch with timeout to protect against hangs.
// OnPrepareDetectionAsync doesn't have the same guardrails as OnFileFoundAsync,
// so we need to be extra careful in this experimental detector.
// so we need to be extra careful here.
try
{
using var timeoutCts = new CancellationTokenSource(PrepareDetectionTimeout);
Expand Down Expand Up @@ -384,8 +413,7 @@ private async Task<IObservable<ProcessRequest>> RunMavenCliDetectionAsync(
var cliSuccessCount = 0;
var cliFailureCount = 0;

// Process pom.xml files sequentially to match MvnCliComponentDetector behavior.
// Sequential execution avoids Maven local repository lock contention and
// Process pom.xml files sequentially to avoid Maven local repository lock contention and
// reduces memory pressure from concurrent Maven JVM processes.
var processPomFile = new ActionBlock<ProcessRequest>(
async processRequest =>
Expand All @@ -402,8 +430,6 @@ private async Task<IObservable<ProcessRequest>> RunMavenCliDetectionAsync(
var depsFilePath = Path.Combine(pomDir, depsFileName);

// Generate dependency file using Maven CLI.
// Note: If both MvnCliComponentDetector and this detector are enabled,
// they may run Maven CLI on the same pom.xml independently.
var result = await this.mavenCommandService.GenerateDependenciesFileAsync(
processRequest,
cancellationToken);
Expand All @@ -414,7 +440,6 @@ private async Task<IObservable<ProcessRequest>> RunMavenCliDetectionAsync(
// Use existence check to avoid redundant I/O (file will be read during directory scan)
if (this.fileUtilityService.Exists(depsFilePath))
{
// File reader registration is now handled in GenerateDependenciesFileAsync
Interlocked.Increment(ref cliSuccessCount);
}
else
Expand Down Expand Up @@ -488,7 +513,6 @@ await this.RemoveNestedPomXmls(processRequests, parentPomDictionary, cancellatio
.Select(componentStream =>
{
// Read and store content to avoid stream disposal issues
// Note: Cleanup coordination is handled in OnFileFoundAsync to avoid duplicate work
using var reader = new StreamReader(componentStream.Stream);
var content = reader.ReadToEnd();
return new ProcessRequest
Expand Down Expand Up @@ -539,6 +563,21 @@ protected override Task OnFileFoundAsync(
{
// Process MvnCli result
this.ProcessMvnCliResult(processRequest);

// Delete the deps file now that its content has been consumed (was read into MemoryStream during prepare phase)
if (this.CurrentScanRequest?.CleanupCreatedFiles == true)
{
var filePath = processRequest.ComponentStream.Location;
try
{
this.fileUtilityService.Delete(filePath);
this.Logger.LogDebug("Cleaned up Maven deps file {File}", filePath);
}
catch (Exception e)
{
this.Logger.LogDebug(e, "Failed to delete Maven deps file {File}", filePath);
}
}
Comment on lines 564 to +580

Copilot AI Apr 2, 2026

Copy link

Choose a reason for hiding this comment

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

Deleting bcde.mvndeps inside OnFileFoundAsync can race with other Maven detectors because detectors run concurrently (Task.WhenAll in the orchestrator). The other MvnCliComponentDetector enumerates these files during its prepare phase; if this detector deletes them early, the other detector can miss them and return incomplete results. Prefer leaving cleanup to the orchestrator-level CleanupMavenFiles that runs after all detectors finish, or coordinate deletion in a way that can’t affect other detectors.

Suggested change
// Process MvnCli result
this.ProcessMvnCliResult(processRequest);
// Delete the deps file now that its content has been consumed (was read into MemoryStream during prepare phase)
if (this.CurrentScanRequest?.CleanupCreatedFiles == true)
{
var filePath = processRequest.ComponentStream.Location;
try
{
this.fileUtilityService.Delete(filePath);
this.Logger.LogDebug("Cleaned up Maven deps file {File}", filePath);
}
catch (Exception e)
{
this.Logger.LogDebug(e, "Failed to delete Maven deps file {File}", filePath);
}
}
// Process MvnCli result.
// Do not delete the generated deps file here because Maven detectors run concurrently,
// and other detectors may still need to enumerate or consume it.
// Cleanup is handled after detector execution completes.
this.ProcessMvnCliResult(processRequest);

Copilot uses AI. Check for mistakes.
}
else
{
Expand Down Expand Up @@ -660,7 +699,7 @@ private void ProcessPomFileStatically(ProcessRequest processRequest)
this.collectedVariables.AddOrUpdate(key, variableValue, (_, _) => variableValue);
}

this.Logger.LogDebug("MavenWithFallback: Collected {Count} variables from {File}", localVariables.Count, Path.GetFileName(filePath));
this.Logger.LogDebug("{DetectorId}: Collected {Count} variables from {File}", this.Id, localVariables.Count, Path.GetFileName(filePath));
}

// First pass: collect dependencies (may have unresolved variables)
Expand Down Expand Up @@ -759,7 +798,7 @@ private void CollectVariablesFromDocument(XmlDocument document, XmlNamespaceMana
{
if (propertiesNodes.Count > 1)
{
this.Logger.LogDebug("MavenWithFallback: Found {Count} properties sections in {File}", propertiesNodes.Count, Path.GetFileName(filePath));
this.Logger.LogDebug("{DetectorId}: Found {Count} properties sections in {File}", this.Id, propertiesNodes.Count, Path.GetFileName(filePath));
}

foreach (XmlNode propertiesNode in propertiesNodes)
Expand Down Expand Up @@ -828,7 +867,8 @@ private void ParseMavenParentRelationship(XmlDocument document, XmlNamespaceMana
{
this.mavenParentChildRelationships[currentFilePath] = parentPath;
this.Logger.LogDebug(
"MavenWithFallback: Parsed parent relationship: {Child} → {Parent}",
"{DetectorId}: Parsed parent relationship: {Child} → {Parent}",
this.Id,
Path.GetFileName(currentFilePath),
Path.GetFileName(parentPath));
}
Expand All @@ -837,7 +877,8 @@ private void ParseMavenParentRelationship(XmlDocument document, XmlNamespaceMana
// Parent not found yet - queue for second pass resolution after all files are processed
this.unresolvedParentRelationships.Enqueue((currentFilePath, parentGroupId, parentArtifactId));
this.Logger.LogDebug(
"MavenWithFallback: Queued unresolved parent relationship for {Child} → {ParentArtifactId}",
"{DetectorId}: Queued unresolved parent relationship for {Child} → {ParentArtifactId}",
this.Id,
Path.GetFileName(currentFilePath),
parentArtifactId);
}
Expand Down Expand Up @@ -875,7 +916,8 @@ private string FindParentPomByArtifactId(string parentGroupId, string parentArti
if (this.processedMavenProjects.TryGetValue(coordinateKey, out var coordinateBasedPath))
{
this.Logger.LogDebug(
"MavenWithFallback: Found parent {ParentCoordinate} at {Path} for {Child}",
"{DetectorId}: Found parent {ParentCoordinate} at {Path} for {Child}",
this.Id,
coordinateKey,
Path.GetFileName(coordinateBasedPath),
Path.GetFileName(filePath));
Expand All @@ -896,7 +938,8 @@ private string FindParentPomByArtifactId(string parentGroupId, string parentArti
if (!visitedDirectories.Add(parentDir))
{
this.Logger.LogDebug(
"MavenWithFallback: Circular directory reference detected while searching for parent POM, breaking at {Directory}",
"{DetectorId}: Circular directory reference detected while searching for parent POM, breaking at {Directory}",
this.Id,
parentDir);
break;
}
Expand Down Expand Up @@ -955,7 +998,8 @@ private void TrackMavenProjectCoordinates(XmlDocument document, XmlNamespaceMana
}

this.Logger.LogDebug(
"MavenWithFallback: Tracked project {GroupId}:{ArtifactId} at {Path}",
"{DetectorId}: Tracked project {GroupId}:{ArtifactId} at {Path}",
this.Id,
groupId ?? "(inherited)",
artifactId,
Path.GetFileName(filePath));
Expand Down Expand Up @@ -1001,7 +1045,8 @@ private IEnumerable<string> ExtractFailedEndpoints(string errorMessage)
}

return EndpointRegex.Matches(errorMessage)
.Select(m => m.Value)
.Select(m => NormalizeEndpointUrl(m.Value))
.Where(u => u is not null)
.Distinct();
}

Expand Down Expand Up @@ -1055,15 +1100,17 @@ private void ResolveUnresolvedParentRelationships()
this.mavenParentChildRelationships[filePath] = parentPath;
resolvedCount++;
this.Logger.LogDebug(
"MavenWithFallback: Resolved deferred parent relationship: {Child} → {Parent}",
"{DetectorId}: Resolved deferred parent relationship: {Child} → {Parent}",
this.Id,
Path.GetFileName(filePath),
Path.GetFileName(parentPath));
}
else
{
unresolvedCount++;
this.Logger.LogDebug(
"MavenWithFallback: Could not resolve parent {ParentGroupId}:{ParentArtifactId} for {Child}",
"{DetectorId}: Could not resolve parent {ParentGroupId}:{ParentArtifactId} for {Child}",
this.Id,
parentGroupId ?? "(null)",
parentArtifactId,
Path.GetFileName(filePath));
Expand Down
Loading