Skip to content
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.

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

feat: add a new detector MvnPomCliComponentDetector #544

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

AbhinavAbhinav11
Copy link

This detector just finds all the .pom files and parses the dependencies (which are present similar to maven structure like ) in those files. It does a simple text parsing and does not create the dependency graph.

This detector just finds all the .pom files and parses the dependencies (which are present similar to maven structure like<dependency> </dependency>) in those files.
It does a simple text parsing and does not create the dependency graph.
@AbhinavAbhinav11 AbhinavAbhinav11 requested a review from a team as a code owner April 28, 2023 20:38
using Microsoft.ComponentDetection.Contracts.TypedComponent;
using Microsoft.Extensions.Logging;

public class MvnPomCliComponentDetector : FileComponentDetector
Copy link
Member

Choose a reason for hiding this comment

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

Could you also implement IDefaultOffComponentDetector? We don't want a new detector to be enabled by default (only explicitly opted-in) before we've had the chance to fully verify it correctly works.

Copy link
Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

Choose a reason for hiding this comment

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

MavenPomComponentDetector is marked IDefaultOffComponenDetector, but MvnPomCliComponentDetector is not

@melotic
Copy link
Member

melotic commented Apr 28, 2023

Thanks for the contribution! I left some feedback on your PR. We also need unit tests to verify this new detector. You can view an example here

AbhinavAbhinav11 and others added 2 commits May 15, 2023 19:26
…es (Just the deps in the pom files will be read as components) along with one implementation (MavenFileParserService) that is being used in MavenPomCliComponentDetector.

Renamed MavenPomCliComponentDetector to MavenPomComponentDetector
Added test case (MavenPomComponentDetectorTest.cs) for the new detector class MavenPomComponentDetector.
@JamieMagee
Copy link
Member

@AbhinavAbhinav11 what the need for this detector have over the MvnCliComponentDetector? The only thing I can see is the ability to get a list of direct dependencies without invoking the Maven CLI.

}
}

private void RegisterComponent(XmlNode node, XmlNamespaceManager nsmgr, ISingleFileComponentRecorder singleFileComponentRecorder)
Copy link
Member

Choose a reason for hiding this comment

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

This method looks the same as RegisterComponent(XmlNode node, XmlNamespaceManager nsmgr, ISingleFileComponentRecorder singleFileComponentRecorder) in MavenFileParserService. We should refactor this and DRY.

using Microsoft.ComponentDetection.Contracts.TypedComponent;
using Microsoft.Extensions.Logging;

public class MvnPomCliComponentDetector : FileComponentDetector
Copy link
Member

Choose a reason for hiding this comment

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

MavenPomComponentDetector is marked IDefaultOffComponenDetector, but MvnPomCliComponentDetector is not

@@ -97,6 +97,8 @@ public static IServiceCollection AddComponentDetection(this IServiceCollection s
services.AddSingleton<IMavenCommandService, MavenCommandService>();
services.AddSingleton<IMavenStyleDependencyGraphParserService, MavenStyleDependencyGraphParserService>();
services.AddSingleton<IComponentDetector, MvnCliComponentDetector>();
services.AddSingleton<IMavenFileParserService, MavenFileParserService>();
services.AddSingleton<IComponentDetector, MavenPomComponentDetector>();
Copy link
Member

Choose a reason for hiding this comment

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

Missing MvnPomCliComponentDetector?

@JamieMagee JamieMagee changed the title Added a new detector MvnPomCliComponentDetector. feat: add a new detector MvnPomCliComponentDetector Jun 22, 2023
@melotic melotic linked an issue Jun 26, 2023 that may be closed by this pull request
@cobya cobya added type:feature Feature (new functionality) detector:maven The Maven detector version:minor labels Jul 10, 2023
@cobya cobya added the status:waiting-on-response Waiting on a response/more information from the user label Dec 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
detector:maven The Maven detector status:waiting-on-response Waiting on a response/more information from the user type:feature Feature (new functionality) version:minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Support for ".pom" file parsing in Maven ecosystem
4 participants