Support for summarized and scheduled notifications
Description
Currently, Dependency-Track Alerts send a single notification for every single event happening that an alert is subscribed to (e.g. NEW_VULNERABILITY). This can lead to a lot of emails for users and creates the risk of overlooking important notifications.
To improve the user experience of the Alerts, scheduled notifications are implemented in Dependency-Track to send summarized notifications of new events between the last and current scheduled notification. The schedule is defined as cron expression.
Scheduled notification management is available via API and Frontend under Administration -> Notifications -> Scheduled Alerts, which is introduced in the Frontend Pull Request [reference will be added after creation].
The PR includes default templates for console and email publishing. The needed UI-Changes are available in the Frontend-PR at DependencyTrack/frontend#903.
Addressed Issue
#322
Additional Details
- DEFAULT_SCHEDULED_CRON_EXPRESSION as environment variable for the default cron expression of new rules
- Error: "The type javax.validation.Payload cannot be resolved. It is indirectly referenced from required type com.github.packageurl.validator.PackageURL" was introduced in my local development environment after syncing the main project and the transition of some javax packages to jakarta
- was fixed by manually adding javax.validation-api to pom.xml as depencency
- While evaluating and debugging the consideration of child projects in the notification process, it has turned out that projects delivered by QueryManager.getAllProjects() and some other methods didn't consistently include the correct child references. To workaround this behavior, the notification task "SendScheduledNotificationTask" uses a project-by-project-retrieval approach in "evaluateAffectedProjects(qm, rule)". This worked out empirically to deliver consistent child references.
- DefaultPublisher-Retrieval was changed from by-class to a combination of name + class (or by-enum on a higher layer) to support multiple default publishers with same publisher class. This change is needed to use the existing publisher classes without defining new ones for scheduled default publishers.
- There might be an open bug currently in the
toJson-Process for PolicyViolation, where the policy is not fully included in the PolicyCondition and thus thePolicy.getUUID().toString()call fails with NullRefException. Often it worked for me just fine, sometimes not. Sometimes fixed with creating a new policy, sometimes with reboot of the application. With some additional random exceptions with the H2 database on ORM-Queries, i don't know for sure, if my code is the reason for this behavior or the instability of H2 with the ORM-mapper in my development environment.
Getting violations for scheduled mail:
PolicyQueryManager, L. 340, getPolicyViolations(Project, boolean, ZonedDateTime)
JSONify the data:
NotificationUtil, L. 550ff., toJson(PolicyViolation) -> toJson(pv.getPolicyCondition) -> toJson(pc.getPolicy) -> policy.getUUID().toString() -> [EXCEPTION]
Please test the expected behavior in a more robust environment (docker, other infrastructures, etc.) to validate
Screenshots
Checklist
- [x] 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~
- [x] 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]~(https://github.com/DependencyTrack/dependency-track/tree/master/src/main/java/org/dependencytrack/upgrade)
- [x] This PR introduces new or alters existing behavior, and I have updated the documentation accordingly
@nscuro After i finally found some time this week to restart the PR process, i cherry-picked all necessary commits from my previous branch, corrected all sign-offs and so on. From the coding side it seems to be fine for now, although additional testing from different perspectives should be done.
Please note in particular the missing javax.validation reference in "Additional details", that seems to be ignored in the current master and seems to be a personal problem of my workspace or an unseen error until now from the transition to jakarta. There may be some more topics to discuss, but the main implementation should be completed by now.
The branch for the PR is based on the same commit my other branches started, but merged with a master newer than this feature branch was created on. Because of this and the recent cherry-picks, the commit history is a bit mixed up with other commits, but that may be gone when the feature branch gets updated with the missing commits from the current master branch.
Please provide me with additional information for the errors of the Static Code Analysis as i cannot see any useful information in the details link. If there is anything else i need to do or you need/want to know, please get in touch with me. I will assist to the best of my abilities.
This looks very interesting. But so does the commit history ;-) Could you rebase it onto current master to make the code changes in the PR "reviewable"?
@valentijnscholten Yeah, i know :D In my git graph, all looked fine, after the PR i had this 😅 I'll try to rebase it and see, if it's possible without making it worse. After the first PR-attempt went terribly wrong, i'm a bit scared tbh 😆 But i will give it a try and see where i land. Maybe it helps already in the mean time to update the feature branch with the commits it is behind to resolve some "false-included" commits in this PR?
Because of this and the recent cherry-picks, the commit history is a bit mixed up with other commits, but that may be gone when the feature branch gets updated with the missing commits from the current master branch.
Let me push the latest changes from master to that branch.
Please provide me with additional information for the errors of the Static Code Analysis as i cannot see any useful information in the details link.
Unfortunately Codacy requires us to manually enable each "base" branch for analysis. Unless that is done, analysis will fail immediately. I have now enabled the feature branch, so the check should work for the next push.
Updated the branches for dependency-track and frontend with the latest master. You may need to close and reopen the PR since GitHub doesn't update PRs automatically when the base branch gets updated.
@valentijnscholten Should be "fixed" by now, thanks to @nscuro :) The update did thankfully what i hoped for.
The current CI test exceptions in BomUploadProcessingTaskTest.java seems to be caused by other stuff, that this PR does not affect, but if i can provide any support, please let me know.
Coverage summary from Codacy
See diff coverage on Codacy
| Coverage variation | Diff coverage |
|---|---|
| :x: -1.44% (target: -1.00%) | :x: 37.63% (target: 70.00%) |
Coverage variation details
| Coverable lines | Covered lines | Coverage | |
|---|---|---|---|
| Common ancestor commit (a4d5a7b328bd93a7dc32f56efb00d0f60b70b480) | 22074 | 16811 | 76.16% |
| Head commit (7ea910c97a36338f0a6eec38b0510971a17fb437) | 22993 (+919) | 17180 (+369) | 74.72% (-1.44%) |
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>
Diff coverage details
| Coverable lines | Covered lines | Diff coverage | |
|---|---|---|---|
| Pull request (#3925) | 885 | 333 | 37.63% |
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%
See your quality gate settings Change summary preferences
:rocket: Don’t miss a bit, follow what’s new on Codacy.
Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more
The current CI test exceptions in BomUploadProcessingTaskTest.java seems to be caused by other stuff, that this PR does not affect, but if i can provide any support, please let me know.
You got lucky and ran into a flaky test 😅 Re-ran the workflow and it completed successfully this time.
Thanks a lot @nscuro for the initial feedback! I'm currently in the process of writing additional test cases to meet the Code Coverage Quota. I may consider to add some of your suggested changes after that as well or reply to them later this week to discuss them. I think by the end of the week, we know if it's a good time to merge it :) I'll try to get the most things done until then.
Short update from my side: i added some tests and applied your suggested changes for records. Therefore, i created a factory to create these notification subjects with the logic which was previously located in the specific classes. I want to test it a bit more and implement some additional suggested changes before i push it, but i will try to finish my local changes in the next days.
@nscuro Hey there :) Could you trigger a test execution? As i can't really test most of the net-related test cases i want to know if i have to fix sth prior to releasing the PRs from draft state. Thanks in advance!
Is this by any chance another flaky test? :D
Is this by any chance another flaky test? :D
Looks like a hiccup in fetching dependencies. I re-started the workflow.
I will release the PR for now from the draft state to allow a broader review. I fixed some minor test errors. Sadly, i can't really get a grip on the other major errors:
- the 500 error codes seem like a server error and may not be related to the code itself...but that's only a guessing
- the publisher tests were created like the others, but i can't really interpret the test error message
I can look into it with assistance, but if a developer with more experience in java testing and especially this project might take a look, i would really appreciate it. Maybe it's a hiccup from the test env, as e.g. the invalid cron test (ScheduledNotificationRuleResourceTest.java, L. 182ff.) returns the expected string in my browser, but as it seems not in the github tests.
@nscuro can you give feedback if you need further changes and/or by when you will have time to look at this?
@rkg-mm I'll try to complete a review this week.
@nscuro could you please update the feature branch to the latest master? our branch is updated to the master branch which results in a lot of changes now :D Same for the frontend please
@rkg-mm Done. The GitHub diff of both PRs won't update though until you push a new commit.
The previously failing unit tests for this PR should now all be fixed. @nscuro could you please re-approve the pending GitHub workflows for the latest commit?
Coverage summary from Codacy
See diff coverage on Codacy
| Coverage variation | Diff coverage |
|---|---|
| :white_check_mark: -0.65% (target: -1.00%) | :x: 62.93% (target: 70.00%) |
Coverage variation details
| Coverable lines | Covered lines | Coverage | |
|---|---|---|---|
| Common ancestor commit (dd9bb4da37949a7a56eccc300d0b2a40c83b4152) | 22519 | 17829 | 79.17% |
| Head commit (27b85df000deb28c41f4e119ed174a26fb348a08) | 23404 (+885) | 18377 (+548) | 78.52% (-0.65%) |
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>
Diff coverage details
| Coverable lines | Covered lines | Diff coverage | |
|---|---|---|---|
| Pull request (#3925) | 920 | 579 | 62.93% |
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%
See your quality gate settings Change summary preferences
Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more
Oops, totally forgot that the workflows still require a maintainer approval for this PR 😅.
I think the remaining Codacy code coverage issues should be resolved by the new commits. @nscuro could you please approve the workflows for us again?
@rkg-mm @rbt-mm @MM-msr First let me say sorry for dragging this for so long. All your work is truly appreciated and I hate that this has been open for so long.
I have updated the target branch with the latest master, and also raised a PR to bringt your branch up to date: https://github.com/MM-msr/dependency-track/pull/2
There are a few changes I'd like to see here and with respect to everyone's time and resources I believe it's best if I simply propose the changes in a PR into your branch, which you can then review whether it still meets your needs.
Things I'd like to change:
- Reduce the duplication in code and database tables that is introduced by treating
ScheduledNotificationRules andNotificationRules as separate types (see also my previous comment here). - Generation of notification subjects must work for large(r) portfolios. At the moment we're loading all findings since the last submission at once, we do repetitive queries to load projects for every single finding, and so on. I just spent a lot of time to nuke such patterns from legacy code and really don't want to introduce them again in another place.
- I'm trying to see if we can avoid introducing a separate scheduling mechanism that bypasses the existing task/eventing mechanism. Of course while still allowing users to change Cron expressions at runtime.
Separately, I was wondering whether we should even allow users to configure scheduled notifications without limiting them to projects. At the moment, limiting to projects is optional. If users were to create such notifications for their entire portfolio, I'm seriously struggling to see how all involved systems could possibly handle the size of the notification content. It feels like a footgun to me. WDYT?
Thank you very much, we will check the PR tomorrow asap.
Separately, I was wondering whether we should even allow users to configure scheduled notifications without limiting them to projects. At the moment, limiting to projects is optional. If users were to create such notifications for their entire portfolio, I'm seriously struggling to see how all involved systems could possibly handle the size of the notification content. It feels like a footgun to me. WDYT?
That's true, this might cause some trouble,especially as you first configure everything else and THEN limit to projects, could cause some timeframe in which you accidentally trigger this without scope. Enforce limiting probably makes sense.
we will check the PR tomorrow asap.
Thanks. To be clear, this initial PR is solely to bring the code up-to-date. I will raise follow-up PRs after that so the changes are easier to see.
Enforce limiting probably makes sense.
Cool. Will look into adding that.
Thanks. To be clear, this initial PR is solely to bring the code up-to-date. I will raise follow-up PRs after that so the changes are easier to see.
sorry, misunderstood - I merged this one directly.
Coverage summary from Codacy
See diff coverage on Codacy
| Coverage variation | Diff coverage |
|---|---|
| :white_check_mark: -0.64% (target: -1.00%) | :x: 62.84% (target: 70.00%) |
Coverage variation details
| Coverable lines | Covered lines | Coverage | |
|---|---|---|---|
| Common ancestor commit (370fb14b5b86352286af0f5b087c0883e6ab41e9) | 23239 | 18662 | 80.30% |
| Head commit (e51d2f16125f76c49c9a594506aa73056b4c47c7) | 24124 (+885) | 19218 (+556) | 79.66% (-0.64%) |
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>
Diff coverage details
| Coverable lines | Covered lines | Diff coverage | |
|---|---|---|---|
| Pull request (#3925) | 923 | 580 | 62.84% |
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%
See your quality gate settings Change summary preferences
Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more
Working on my changes here: https://github.com/MM-msr/dependency-track/pull/3
Admittedly I struggled with finding the time to complete this but it's finally done, you can find the final result, including a high-level description of how this PR evolved here: #4783.
Superseded by #4783