microsoft / component-detection

Scans your project to determine what components you use
MIT License
432 stars 90 forks source link

Get ancestor for displaying dependency tree in relationships #927

Closed tarun06 closed 9 months ago

tarun06 commented 11 months ago

This pr is required by https://github.com/microsoft/sbom-tool/pull/457 which display Hierarchy of package dependency in relatrionship section of SBOM

codecov[bot] commented 11 months ago

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (715078c) 75.3% compared to head (0433c76) 75.3%. Report is 2 commits behind head on main.

Files Patch % Lines
...GraphTranslation/DefaultGraphTranslationService.cs 80.0% 2 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #927 +/- ## ===================================== Coverage 75.3% 75.3% ===================================== Files 236 236 Lines 10325 10342 +17 Branches 1022 1025 +3 ===================================== + Hits 7775 7789 +14 - Misses 2267 2269 +2 - Partials 283 284 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

cobya commented 10 months ago

@tarun06 What does this look like in an example ScanManifest output? I'm worried about this change's impact on downstream dependencies

tarun06 commented 10 months ago

ScanManifest.json ScanManifest_WithChange.json

yes, @cobya it does change topLevelReferrers.. Sharing scanmanifest for reference

cobya commented 10 months ago

@tarun06 would you be able to come to Community Meeting 01-03-2024 to discuss? I'd like to get some more context on the change

tarun06 commented 10 months ago

@tarun06 would you be able to come to Community Meeting 01-03-2024 to discuss? I'd like to get some more context on the change

Sure.. Thank you

tarun06 commented 10 months ago

@tarun06 would you be able to come to Community Meeting 01-03-2024 to discuss? I'd like to get some more context on the change

Sure.. Thank you

@cobya i joned the community meeting now.. I am 1 hour late thinking the meeting is at 11:30 IST, Appologies for that.. Let me know if i can answer to queries here. :)

cobya commented 10 months ago

Sure, a couple of things then so we can do this async:

tarun06 commented 10 months ago

Sure, a couple of things then so we can do this async:

  • How is SBOM tool planning on using the ancestors information? Parse the graph by looking at each ancestor to see which is the root for each level?

  • Should we keep this as the existing field or add it as a new one? I'm preferential to a new field because it can be useful to have both sets of information (i.e. give me the full graph vs give me only the component which brings this one)

  • If you are planning on having TopLevelReferrers become the list of ancestors, we should rename it since it's not really accurate anymore

  • How is SBOM tool planning on using the ancestors information? Parse the graph by looking at each ancestor to see which is the root for each level?

The SBOM tool creates a record of the dependency hierarchy for each package, assigning a unique identifier to each. Developers can easily identify how a transitive dependency is included in their application. Also for fixing security vulnerabilities, teams can identify which package they need to upgrade to resolve the vulnerability on a transitive dependency.

example -> SPDXRef-RootPackage -> Microsoft.EntityFrameworkCore -> Microsoft.Extensions.Caching.Memory ->Microsoft.Extensions.Caching.Abstractions -> Microsoft.Extensions.Primitives -> System.Runtime.CompilerServices.Unsafe Please check the SBOM PR for complete spdx file generated using this aproach https://github.com/microsoft/sbom-tool/pull/457

  • Should we keep this as the existing field or add it as a new one? I'm preferential to a new field because it can be useful to have both sets of information (i.e. give me the full graph vs give me only the component which brings this one)

We can create a new field as well.. I opted for "TopLevelReferrers" as I believed it closely resembled its intended meaning. please suggest any name if you have it in mind. Thanks

tarun06 commented 10 months ago

Sure, a couple of things then so we can do this async:

  • How is SBOM tool planning on using the ancestors information? Parse the graph by looking at each ancestor to see which is the root for each level?

  • Should we keep this as the existing field or add it as a new one? I'm preferential to a new field because it can be useful to have both sets of information (i.e. give me the full graph vs give me only the component which brings this one)

  • If you are planning on having TopLevelReferrers become the list of ancestors, we should rename it since it's not really accurate anymore

  • How is SBOM tool planning on using the ancestors information? Parse the graph by looking at each ancestor to see which is the root for each level?

The SBOM tool creates a record of the dependency hierarchy for each package, assigning a unique identifier to each. Developers can easily identify how a transitive dependency is included in their application. Also for fixing security vulnerabilities, teams can identify which package they need to upgrade to resolve the vulnerability on a transitive dependency.

example -> SPDXRef-RootPackage -> Microsoft.EntityFrameworkCore -> Microsoft.Extensions.Caching.Memory ->Microsoft.Extensions.Caching.Abstractions -> Microsoft.Extensions.Primitives -> System.Runtime.CompilerServices.Unsafe Please check the SBOM PR for complete spdx file generated using this aproach microsoft/sbom-tool#457

  • Should we keep this as the existing field or add it as a new one? I'm preferential to a new field because it can be useful to have both sets of information (i.e. give me the full graph vs give me only the component which brings this one)

We can create a new field as well.. I opted for "TopLevelReferrers" as I believed it closely resembled its intended meaning. please suggest any name if you have it in mind. Thanks

taging @cobya

cobya commented 10 months ago

@tarun06 I think we should keep TopLevelReferrers as the list of explicit root only components (the top only), and we can make the new field something like AncestralReferrers (I'm not sold on that, if you have something better feel free) that way anyone who depends on TopLevelReferrers being only root components is unaffected as well.

tarun06 commented 9 months ago

@cobya i have used AncestralReferrers instead of TopLevelReferrers .. Please review.