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

SPDX expression support improvements #2965

Merged
merged 8 commits into from
Aug 22, 2023

Conversation

nscuro
Copy link
Member

@nscuro nscuro commented Aug 19, 2023

Description

Building upon #2400, this PR adds missing functionality for setting license expressions via REST API, and adds expression validation for both REST API and BOM ingestion.

Invalid expressions submitted via frontend or REST API will result in an API error, whereas invalid expressions encountered during BOM ingestion will be logged and skipped.

Addressed Issue

N/A

Additional Details

N/A

Checklist

  • I have read and understand the contributing guidelines
  • This PR fixes a defect, and I have provided tests to verify that the fix is effective
  • This PR implements an enhancement, and I have provided tests to verify that it works as intended
  • This PR introduces changes to the database model, and I have added corresponding update logic
  • This PR introduces new or alters existing behavior, and I have updated the documentation accordingly

@nscuro nscuro added the enhancement New feature or request label Aug 19, 2023
@nscuro nscuro added this to the 4.9 milestone Aug 19, 2023
nscuro added 4 commits August 20, 2023 19:21
With the addition of new test cases, sometimes tests executed later get additional events / notifications that were not sent during their execution, but potentially by the test executed before them.

Signed-off-by: nscuro <[email protected]>
Are unfinished asynchronous tasks using the database a problem? 🤔

Signed-off-by: nscuro <[email protected]>
@hborchardt
Copy link
Contributor

Thanks for following up on this, and making improvements where I was lacking knowledge about the framework's capabilities or intricacies 🚀

@melba-lopez
Copy link
Contributor

Thanks for following up on this, and making improvements where I was lacking knowledge about the framework's capabilities or intricacies 🚀

Thank you @hborchardt for contributing what you have been able to! Sometimes we don't get enough time or have the knowledge (im sure you can reference my PR thats been open for eons now 😸 )

@@ -136,7 +130,7 @@ public void informTest() throws Exception {
final byte[] bomBytes = Files.readAllBytes(Paths.get(getClass().getClassLoader().getResource("bom-1.xml").toURI()));

new BomUploadProcessingTask().inform(new BomUploadEvent(project.getUuid(), bomBytes));
assertConditionWithTimeout(() -> NOTIFICATIONS.size() >= 5, Duration.ofSeconds(5));
assertConditionWithTimeout(() -> NOTIFICATIONS.size() >= 6, Duration.ofSeconds(5));
Copy link
Contributor

Choose a reason for hiding this comment

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

i see this is being changed from notification size >= 5 ... to 6. Is this a 1 to 1 mapping with duration of seconds? @nscuro

Copy link
Member Author

Choose a reason for hiding this comment

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

It's an inconsistency in assertions that I noticed while debugging another issue with this test.

This statement is waiting for 6 (previously 5) notifications to be sent, and will wait up to 5 seconds for this to happen.
It's more or less a means of ensuring all asynchronous processing has completed before doing further assertions.

The actual notification content is being asserted later in the test:

assertThat(NOTIFICATIONS).satisfiesExactly(
n -> assertThat(n.getGroup()).isEqualTo(NotificationGroup.PROJECT_CREATED.name()),
n -> assertThat(n.getGroup()).isEqualTo(NotificationGroup.BOM_CONSUMED.name()),
n -> assertThat(n.getGroup()).isEqualTo(NotificationGroup.BOM_PROCESSED.name()),
n -> {
assertThat(n.getGroup()).isEqualTo(NotificationGroup.NEW_VULNERABILITY.name());
NewVulnerabilityIdentified nvi = (NewVulnerabilityIdentified) n.getSubject();
assertThat(nvi.getVulnerabilityAnalysisLevel().equals(VulnerabilityAnalysisLevel.BOM_UPLOAD_ANALYSIS));
},
n -> {
assertThat(n.getGroup()).isEqualTo(NotificationGroup.NEW_VULNERABILITY.name());
NewVulnerabilityIdentified nvi = (NewVulnerabilityIdentified) n.getSubject();
assertThat(nvi.getVulnerabilityAnalysisLevel().toString().equals(VulnerabilityAnalysisLevel.BOM_UPLOAD_ANALYSIS));
},
n -> assertThat(n.getGroup()).isEqualTo(NotificationGroup.NEW_VULNERABLE_DEPENDENCY.name())
);

I noticed that the above assertion is checking for 6 notifications, not just 5... :D


final byte[] bomBytes = """
{
"bomFormat": "CycloneDX",
Copy link
Contributor

Choose a reason for hiding this comment

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

if this is a test to upload a bom that is of SPDX, should we include appropriate spdx nomenclature outside of just the license? I suspect if someone uploads an SPDX bom, they would want that information to stay when downloading it too.

Copy link
Member Author

Choose a reason for hiding this comment

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

SPDX BOMs are not supported at all right now; Uploads of such BOMs will be rejected. We have #1746 open, but doesn't look like we're going to address that anytime soon.

For this change, we just want to ensure that SPDX license expressions within a CDX BOM are correctly ingested.

Copy link
Contributor

Choose a reason for hiding this comment

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

🤦‍♀️ for various reasons we discussed... i swear it was! Thanks for bringing me back to reality 😺

Copy link
Contributor

@melba-lopez melba-lopez left a comment

Choose a reason for hiding this comment

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

@nscuro 👨‍💻 guru! awesome changes in such a short amount of time. I had 2 comments that could be easy for you to answer/address. Hopefully you can take a look before merging!

@nscuro nscuro merged commit e66a28c into DependencyTrack:master Aug 22, 2023
@nscuro nscuro deleted the spdx-expression-improvements branch August 22, 2023 19:17
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants