fix:[NEXT-455] Vulnerability for multiple asset types for a plugin #2357
Conversation
WalkthroughThe pull request updates the AWS Java SDK S3 dependency version in the project's POM file and restructures the S3 data retrieval process in the VulnerabilityAssociationManager. The rework includes listing S3 objects with a new request method, filtering based on file suffixes, and delegating entity upload to a new method. Error handling has been streamlined and new constants have been introduced to identify file types. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Manager as VulnerabilityAssociationManager
participant S3 as AWS S3
participant ES as Elasticsearch
Client->>Manager: initiate uploadVulnerabilityInfo()
Manager->>S3: ListObjectsV2(DATA_PATH)
S3-->>Manager: Return list of objects
Manager->>Manager: Filter objects by suffixes (VUL_FILE_SUFFIX, DETECTION_FILE_SUFFIX)
Manager->>Manager: Process and transform data
Manager->>ES: Call uploadEntity(entities, indexName, loadDate)
ES-->>Manager: Upload confirmation
Manager->>Client: Return success response
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
jobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/entity/VulnerabilityAssociationManager.java (4)
6-6: Avoid wildcard imports for clarity.
Using wildcard imports (import com.amazonaws.services.s3.model.*;) can reduce code clarity and may lead to unintended name conflicts if multiple classes share the same name. Consider importing individual classes explicitly or using a smaller subset of imports to improve readability.
54-60: Consider handling large S3 listings in smaller batches.
While listing objects in a single request works for smaller amounts of data, large buckets may significantly impact memory usage and performance. Consider using pagination (ListObjectsV2Request#setMaxKeys) or iterating through partial results if the data could become large in production.
63-65: Clarify combined logical checks when filtering file names.
The conditionif (fileName.endsWith(VUL_FILE_SUFFIX) && entry.getKey().contains("vulnerabilities") || fileName.endsWith(DETECTION_FILE_SUFFIX) && entry.getKey().contains("-detections")) { ... }is correct, but it can be confusing due to mixed
&&and||. Consider adding explicit parentheses or refactoring to multipleifchecks for enhanced readability.
68-77: Log messages within catch block may obscure real errors.
At lines 73-75, any exception when reading entities is logged as “data is empty,” which can be misleading if there's a parsing or network issue. Logging the actual exception message or rethrowing a custom exception might improve troubleshooting.} catch (Exception e) { - LOGGER.info("{} data is empty", filePrefix); + LOGGER.error("Error reading data from {}: {}", filePrefix, e.getMessage());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
jobs/pacman-data-shipper/pom.xml(1 hunks)jobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/entity/VulnerabilityAssociationManager.java(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Shipper-Build
- GitHub Check: Analyze (java)
- GitHub Check: SonarCloud-Build
🔇 Additional comments (5)
jobs/pacman-data-shipper/pom.xml (1)
15-15: Updated AWS Java SDK S3 dependency version.The version has been updated from 1.12.261 to 1.12.263. Please ensure that this dependency update has been thoroughly tested for compatibility with your S3 data retrieval improvements in the VulnerabilityAssociationManager. Verifying that no deprecated APIs are called and that the vulnerability fix is effective would be beneficial.
jobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/entity/VulnerabilityAssociationManager.java (4)
33-34: Constants for file suffixes look good.
Introducing dedicated suffix constants (VUL_FILE_SUFFIXandDETECTION_FILE_SUFFIX) improves maintainability and reduces the likelihood of typos when handling different file types.
66-66: Appropriate use of try-with-resources.
Using a try-with-resources block for reading from the S3 object content ensures the stream is closed automatically, preventing resource leaks. This is a good practice.
93-93: No functional change on this line.
This line appears to be a blank or spacing adjustment. No further action needed.
95-108: Validate concurrency when modifying entities with parallel streams.
TheparallelStream()on line 100 applies modifications (removingclosedDateand adding_loaddate) to each map. While this is acceptable if each map is independent, ensure that the root data structure is safe for parallel updates. Otherwise, consider a regular stream or use thread-safe structures if concurrency issues appear.
|
fix:NEXT-455 Vulnerability for multiple asset types for a plugin
Fixes # (issue if any)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also
list any relevant details for your test configuration
Checklist:
Other Information:
List any documentation updates that are needed for the Wiki
Summary by CodeRabbit