chore: improve cyclonedx-mvn bom file check to include files with cus… #524

benmss · 2023-10-19T03:30:01Z

Allows Macaron to discover cyclonedx-mvn SBOM files that have been created with a non-default name.
Does not provide support for custom directories or cases where multiple SBOM files end up in the expected target directory, as these would require POM parsing to properly detect.
Closes #518

src/macaron/dependency_analyzer/cyclonedx_mvn.py

tromai · 2023-10-19T23:52:56Z

Is it possible to have a unit test for the scenario of custom file name for the top path SBOM file?

tests/dependency_analyzer/cyclonedx/test_cyclonedx.py

tromai

This PR looks good to me

behnazh-w · 2023-10-31T05:30:14Z

Since the apache/maven repo that we analyze in the integration tests also customizes the name of the BOM file, it would make sense to change the pinned commit to the latest version and test the change in this PR as part of the integration tests.

…tom names Signed-off-by: Ben Selwyn-Smith <[email protected]>

…t for custom named maven sboms Signed-off-by: Ben Selwyn-Smith <[email protected]>

Signed-off-by: Ben Selwyn-Smith <[email protected]>

…est case Signed-off-by: Ben Selwyn-Smith <[email protected]>

Signed-off-by: Ben Selwyn-Smith <[email protected]>

tromai · 2023-11-07T02:11:27Z

src/macaron/dependency_analyzer/cyclonedx_mvn.py


        # Collect all the dependency files recursively.
        child_paths = [
            Path(path)
-            for path in glob.glob(os.path.join(dir_path, "**", "target", self.file_name), recursive=True)
+            for path in glob.glob(
+                os.path.join(dir_path, "**", "target", "*.json" if top_path_altered else self.file_name), recursive=True


What would happen if there is multiple *.json files in the "children" taget directories, given that there exist a custom-named SBOM file in the top path?

Signed-off-by: Ben Selwyn-Smith <[email protected]>

behnazh-w

Because the BOM file can have a custom name, we also need to adjust the error message here and avoid printing the default file name to avoid confusion:

raise CycloneDXParserError(f"Unable to locate any BOM files at: {str(file_path.parent)}.")

src/macaron/dependency_analyzer/cyclonedx_mvn.py

Signed-off-by: Ben Selwyn-Smith <[email protected]>

tromai · 2023-11-24T01:21:33Z

src/macaron/dependency_analyzer/cyclonedx_mvn.py

            if Path(path) != top_path
        ]

+        # Ensure recursively found SBOMs are at most one per directory.
+        child_paths_set = set()


This set contains the parent directory names of all children SBOM files. I think we could re-name it to something like child_sbom_dir_names

I left some comments for the latest changes so I dismissed this approval.

Signed-off-by: Ben Selwyn-Smith <[email protected]>

…in projects and subprojects, and clarify in comments Signed-off-by: Ben Selwyn-Smith <[email protected]>

tromai · 2023-11-24T05:38:32Z

src/macaron/dependency_analyzer/cyclonedx_mvn.py

+        directory. The name of the file is not considered because projects can be configured to produce a custom named
+        SBOM, which cannot be overridden if included at the parent POM level.
+        The presence of multiple JSON files within a target directory differs too greatly from the expectations of the
+        plugin's output. It is for this reason that an error is thrown in such cases.


Suggested change

plugin's output. It is for this reason that an error is thrown in such cases.

plugin's output. It is for this reason that these cases are treated as error and this method will return an empty dictionary.

I think it's better to be explicitly on how the error are handled. We don't throw any error in this method, but only return an empty dictionary. Please feel free to change the comment accordingly, my suggestion here is only for references only.

behnazh-w · 2023-11-24T05:46:25Z

src/macaron/dependency_analyzer/cyclonedx_mvn.py

-                    return {}
-                top_path = Path(possible_paths[0])
-                top_path_altered = True
+        possible_paths = glob.glob(os.path.join(dir_path, "target", "*.json"))


I prefer the previous implementation which first checks for the expected file name. If that file cannot be found, we can look for other JSON files. This can avoid missing the BOM files when a project uses expected names but also produces other JSON files.

Signed-off-by: Ben Selwyn-Smith <[email protected]>

#524) Signed-off-by: Ben Selwyn-Smith <[email protected]>

benmss requested review from behnazh-w and tromai as code owners October 19, 2023 03:30

oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Oct 19, 2023

tromai reviewed Oct 19, 2023

View reviewed changes

src/macaron/dependency_analyzer/cyclonedx_mvn.py Outdated Show resolved Hide resolved

benmss linked an issue Oct 22, 2023 that may be closed by this pull request

Support custom named cyclonedx maven bom output #518

Closed

tromai reviewed Oct 23, 2023

View reviewed changes

tests/dependency_analyzer/cyclonedx/test_cyclonedx.py Outdated Show resolved Hide resolved

tromai reviewed Oct 23, 2023

View reviewed changes

tests/dependency_analyzer/cyclonedx/test_cyclonedx.py Outdated Show resolved Hide resolved

tromai previously approved these changes Oct 23, 2023

View reviewed changes

benmss added 8 commits November 7, 2023 09:07

chore: improve cyclonedx-mvn bom file check to include files with cus…

3660da7

…tom names Signed-off-by: Ben Selwyn-Smith <[email protected]>

chore: added error output for multiple sbom discovery; added unit tes…

c1ad105

…t for custom named maven sboms Signed-off-by: Ben Selwyn-Smith <[email protected]>

chore: refine newly added test

1195667

Signed-off-by: Ben Selwyn-Smith <[email protected]>

chore: pruned cyclonedxmaven constructor input string parameters in t…

bcda1ec

…est case Signed-off-by: Ben Selwyn-Smith <[email protected]>

chore: update integration tests to use latest maven commit

8349a89

Signed-off-by: Ben Selwyn-Smith <[email protected]>

chore: make e2e comparisons of remote paths ignore '.git'

6b92419

Signed-off-by: Ben Selwyn-Smith <[email protected]>

chore: update maven commit in docker tests

b575dd9

Signed-off-by: Ben Selwyn-Smith <[email protected]>

chore: revert .git handling in e2e comparison

f738040

Signed-off-by: Ben Selwyn-Smith <[email protected]>

benmss force-pushed the 518-support-named-cyclonedx-output branch from 04614ec to f738040 Compare November 6, 2023 23:15

chore: remove .git from expected results

0190856

Signed-off-by: Ben Selwyn-Smith <[email protected]>

tromai reviewed Nov 7, 2023

View reviewed changes

benmss added 2 commits November 8, 2023 14:07

chore: allow only one custom named sbom file per child directory

eec1a66

Signed-off-by: Ben Selwyn-Smith <[email protected]>

chore: update tests for named sboms

512b5aa

Signed-off-by: Ben Selwyn-Smith <[email protected]>

behnazh-w requested changes Nov 23, 2023

View reviewed changes

src/macaron/dependency_analyzer/cyclonedx_mvn.py Outdated Show resolved Hide resolved

chore: addressed PR feedback

2a1e8b8

Signed-off-by: Ben Selwyn-Smith <[email protected]>

behnazh-w approved these changes Nov 23, 2023

View reviewed changes

behnazh-w requested a review from tromai November 23, 2023 22:54

tromai reviewed Nov 24, 2023

View reviewed changes

chore: update variable name to be more descriptive

819304f

Signed-off-by: Ben Selwyn-Smith <[email protected]>

behnazh-w approved these changes Nov 24, 2023

View reviewed changes

chore: separate acceptance of named vs default cyclonedx-maven sboms …

755ef04

…in projects and subprojects, and clarify in comments Signed-off-by: Ben Selwyn-Smith <[email protected]>

tromai reviewed Nov 24, 2023

View reviewed changes

behnazh-w self-requested a review November 24, 2023 05:42

behnazh-w reviewed Nov 24, 2023

View reviewed changes

chore: revert, so named cyclonedx-maven sbom files are still preferred

8e957d6

Signed-off-by: Ben Selwyn-Smith <[email protected]>

tromai approved these changes Nov 24, 2023

View reviewed changes

benmss merged commit 2971708 into staging Nov 24, 2023
9 checks passed

nathanwn deleted the 518-support-named-cyclonedx-output branch December 14, 2023 02:23

art1f1c3R pushed a commit that referenced this pull request Nov 29, 2024

chore: improve cyclonedx-mvn bom file check to include files with cus… (

2a2b0af

#524) Signed-off-by: Ben Selwyn-Smith <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: improve cyclonedx-mvn bom file check to include files with cus… #524

chore: improve cyclonedx-mvn bom file check to include files with cus… #524

benmss commented Oct 19, 2023 •

edited

Loading

tromai commented Oct 19, 2023 •

edited

Loading

tromai left a comment •

edited

Loading

behnazh-w commented Oct 31, 2023 •

edited

Loading

tromai Nov 7, 2023 •

edited

Loading

behnazh-w left a comment

tromai Nov 24, 2023 •

edited

Loading

tromai Nov 24, 2023 •

edited

Loading

behnazh-w Nov 24, 2023 •

edited

Loading

	plugin's output. It is for this reason that an error is thrown in such cases.
	plugin's output. It is for this reason that these cases are treated as error and this method will return an empty dictionary.

chore: improve cyclonedx-mvn bom file check to include files with cus… #524

chore: improve cyclonedx-mvn bom file check to include files with cus… #524

Conversation

benmss commented Oct 19, 2023 • edited Loading

tromai commented Oct 19, 2023 • edited Loading

tromai left a comment • edited Loading

Choose a reason for hiding this comment

behnazh-w commented Oct 31, 2023 • edited Loading

tromai Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

behnazh-w left a comment

Choose a reason for hiding this comment

tromai Nov 24, 2023 • edited Loading

Choose a reason for hiding this comment

tromai Nov 24, 2023 • edited Loading

Choose a reason for hiding this comment

behnazh-w Nov 24, 2023 • edited Loading

Choose a reason for hiding this comment

benmss commented Oct 19, 2023 •

edited

Loading

tromai commented Oct 19, 2023 •

edited

Loading

tromai left a comment •

edited

Loading

behnazh-w commented Oct 31, 2023 •

edited

Loading

tromai Nov 7, 2023 •

edited

Loading

tromai Nov 24, 2023 •

edited

Loading

tromai Nov 24, 2023 •

edited

Loading

behnazh-w Nov 24, 2023 •

edited

Loading