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 workspaceRelationship #7889

Merged

Conversation

DmitriyLewen
Copy link
Contributor

Description

Add workspaceRelationship.

workspaceRelationship will be used for cargo, npm, maven, etc. packages.
See #7802 for more details.

This PR also adds workspaceRelationship for maven modules.

Related issues

Checklist

  • I've read the guidelines for contributing to this repository.
  • I've followed the conventions in the PR title.
  • I've added tests that prove my fix is effective or that my feature works.
  • I've updated the documentation with the relevant information (if needed).
  • I've added usage information (if the PR introduces new options)
  • I've included a "before" and "after" example to the description (if the PR is a user interface change).

@DmitriyLewen DmitriyLewen self-assigned this Nov 7, 2024
@DmitriyLewen DmitriyLewen marked this pull request as ready for review November 8, 2024 05:55
@@ -418,23 +418,38 @@ func (*Encoder) vulnerability(vuln types.DetectedVulnerability) core.Vulnerabili
}

// belongToParent determines if a package should be directly included in the parent based on its relationship and dependencies.
func (*Encoder) belongToParent(pkg ftypes.Package, parents map[string]ftypes.Packages) bool {
// Case 1: Direct/Indirect: known , DependsOn: known
func (*Encoder) belongToParent(pkg ftypes.Package, pkgType ftypes.TargetType, parents map[string]ftypes.Packages) bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This code must be package-type agnostic. I'm thinking about implementation now.

Copy link
Contributor Author

@DmitriyLewen DmitriyLewen Nov 21, 2024

Choose a reason for hiding this comment

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

I looked at it again with clear thoughtsю
What if we will check parents types:

	// Case 1-1: direct packages
	case pkg.Relationship == ftypes.RelationshipDirect:
		pkgParents, ok := parents[pkg.ID]
                // direct package is not related with root package
		if !ok {
			return true
		}
		// Add a package to the parent relation if one of the parents is a root package
		if _, found := lo.Find(pkgParents, func(p ftypes.Package) bool {
			return p.Relationship == ftypes.RelationshipRoot
		}); found {
			return true
		}
		// Case where this package is a child of workspace(s) only
		// Case 1-4
		return false

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should construct the dependency relationship between root and direct packages on the analyzer/parser end. Then, we can check whether the package has a parent. If the package is direct and has any parent (root or workspace), belongToParent should return false.

I'm not sure if this approach works. Please let me try. This is the first PR relevant to my idea. #7973

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should construct the dependency relationship between root and direct packages on the analyzer/parser end.

That seems like a good idea too.

Let me know if i can help you.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you do the same thing as #7973 for go modules?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no problem.
Can I add the changes to your PR or should I create a new PR for go modules?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Better to create a new PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created #7977

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@knqyf263 I merged #7985 into this PR.
Also i added relationships for maven modules.
Seems to work correctly.

Take a look, please.

@DmitriyLewen DmitriyLewen force-pushed the feat/workspace-relationship branch from c0c4bf1 to 775ae7f Compare November 28, 2024 10:47
Copy link
Collaborator

@knqyf263 knqyf263 left a comment

Choose a reason for hiding this comment

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

Looks simple now! Do we have any leftovers? If not, you can merge this PR.

@DmitriyLewen DmitriyLewen added this pull request to the merge queue Nov 29, 2024
Merged via the queue into aquasecurity:main with commit d622ca2 Nov 29, 2024
17 checks passed
@DmitriyLewen DmitriyLewen deleted the feat/workspace-relationship branch November 29, 2024 05:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug(java): dependOn contains extra dependencies for pom.xml files with modules when using SBOM formats
2 participants