Conversation
…at doesn't use notification APIs
| * @throws IntegrationException | ||
| */ | ||
| public Date getLatestUserNotificationDate(UserView userView) throws IntegrationException { | ||
| logger.warn("Fetching latest notification date for user via a low‑performance API."); |
There was a problem hiding this comment.
If we are warning this method is low performance which API is more performant? Is the more performant API used and this method is a fallback? By warning that this API is in use if we don't use the more performant API and this message appears in the logs a customer may question why are we using an API with low performance and why aren't we using a more performant API. So we will need to be prepared for those questions if we are now informing on the performance of this API.
There was a problem hiding this comment.
That is a very good point you raise, and maybe the removal of this method entirely is a better move. Since this is a shared library, I kept the changes limited to what affects Detect only. If I can confirm that Alert also does not use this method, we can safely get rid of it. Below are answers to your questions that will hopefully clear things up:
- If we are warning this method is low performance which API is more performant? Is the more performant API used and this method is a fallback? This method is actually obsolete from Detect's perspective, but kept since this is a shared library and I couldn't be sure it wouldn't affect Alert team. Going forward, Detect no longer queries a user's most recent notification (see Detect PR linked in description). The true motivation behind this log statement was to make QA testing easier and spot any missed uses of this API from Detect, if any.
- Logs customers may see: Since Detect no longer makes this call, this log statement will never show up. But since you pointed it out, I'm thinking leaving a log statement just to make sure it never shows up is an odd way to do things. I am confident QA will not see this in the logs for any of our scans, across Hub versions, etc. Trace logs can of course confirm this also, I just wanted to make it a little easier to spot by QA. Somewhat related: we do query notifications endpoint (via some other method) in only one remaining edge case due to backwards compatibility needs and for that users see something like this in the Detect logs:
DEBUG [main] --- Using legacy binary scan upload method. This can be slow, consider upgrading to a newer version of Black Duck SCA to enable multipart uploading if possible.
INFO [main] --- Preparing to upload binary scan file: /Users/shanty/blackduck/example-source/binaries/ifm.tar
DEBUG [main] --- Will use old notifications based waiting for the following code locations: [ifm.tar/shanty-testing/testing binary]
...
DEBUG [main] --- Will use old notifications based waiting for the following code locations: [ifm.tar/shanty-testing/testing binary]
DEBUG [main] --- Notifications after Tue Apr 14 04:47:10 MDT 2026 will be considered.
Sorry for the wall of text, hope that answers your questions. Do you suggest the complete removal of this method? (affects upstream methods and mostly just test cases within this lib)
There was a problem hiding this comment.
Thank you. Your points are valid here I just wanted to avoid confusion for our customers and support. If you remove the method that would be a breaking change, so the version needs to change to the next major version. I have looked and Alert does not make this call so it will not be affected either.
There was a problem hiding this comment.
Removed the method getLatestuserNotification() and replaced any uses with the current date instead (this is what Detect does now as well). The reason for fetching latest notification timestamp was just to create a time window for checking new notifications, so the current timestamp works for our use cases just as well and all tests pass.
+Bumped major version.
@psantos1113 @DanaMaxfield Would you mind taking a look when you get a chance at the last 2 commits? Thank you!
…he current date instead.
See related Detect PR: blackducksoftware/detect#1708
Summary of changes:
This is considered a breaking change and will bump blackbuck-common version to 68.0.0