Conversation
|
👋 Hi! It looks like you modified some files in the
If none of the above scenarios apply, feel free to ignore this comment 🙂 |
There was a problem hiding this comment.
Pull request overview
This PR modifies the Maven detector implementation in Microsoft.ComponentDetection.Detectors with the apparent goals of tightening endpoint handling (to avoid leaking sensitive URL parts), adjusting cleanup behavior for generated bcde.mvndeps files, and updating detector identity/metadata.
Changes:
- Renames/re-identifies the Maven fallback detector class and updates its
Id/Version. - Adds endpoint URL normalization before logging/telemetry collection.
- Deletes generated Maven deps files during
OnFileFoundAsyncwhenCleanupCreatedFilesis enabled, and standardizes several log messages.
| /// 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 |
There was a problem hiding this comment.
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.
| public class MvnCliComponentDetector : FileComponentDetector | |
| public class MavenWithFallbackDetector : FileComponentDetector |
| 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; |
There was a problem hiding this comment.
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.
| /// 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 |
There was a problem hiding this comment.
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).
| public class MvnCliComponentDetector : FileComponentDetector | |
| public class MvnCliComponentDetector : FileComponentDetector, IExperimentalDetector |
| // 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); | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
| // 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); |
| public override IEnumerable<ComponentType> SupportedComponentTypes => [ComponentType.Maven]; | ||
|
|
||
| public override int Version => 2; | ||
| public override int Version => 5; |
There was a problem hiding this comment.
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.
| public override int Version => 5; | |
| public override int Version => 2; |
No description provided.