Clone feature/202405 oss support #9
Conversation
WalkthroughSupport for Alibaba Cloud OSS as an object storage provider has been added across both backend and frontend. This includes new configuration classes, service implementations, UI options, and localization strings for OSS, enabling users to configure and interact with Alibaba Cloud OSS alongside existing providers. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WebApp
participant Backend
participant OSS
User->>WebApp: Selects OSS as object store type and enters config
WebApp->>Backend: Sends OSS config for initialization
Backend->>OSS: Initializes OSSClient with provided credentials
Backend-->>WebApp: Confirms OSS setup
User->>WebApp: Uploads file
WebApp->>Backend: Sends file upload request
Backend->>OSS: Uploads file to OSS bucket
OSS-->>Backend: Returns upload status
Backend-->>WebApp: Returns upload result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @arvi18, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces comprehensive support for Alibaba Cloud Object Storage Service (OSS) as an alternative file storage provider within the application. It integrates the necessary backend services and client libraries, extends the application's configuration capabilities to manage OSS settings, and updates the frontend user interface to allow seamless setup and management of OSS for storing application data, complementing the existing local file and Huawei Cloud OBS options.
Highlights
- Alibaba Cloud OSS Integration (Backend): The pull request adds the
aliyun-sdk-ossdependency tomanager/pom.xmland introduces a new service implementation,OssObjectStoreServiceImpl.java, to handle file operations (upload, download, list) with Alibaba Cloud OSS. This includes the necessary client initialization and configuration handling withinObjectStoreConfigServiceImpl.java. - Object Storage Configuration Extension: The
ObjectStoreDTO.javafile is updated to includeOSSas a new object storageTypeand defines a newOssConfigclass to encapsulate Alibaba Cloud OSS-specific configuration parameters such asaccessKey,secretKey,bucketName,endpoint, andsavePath. - Frontend UI and Localization Updates: The web application's frontend (
web-app/src/app/pojo/ObjectStore.ts,web-app/src/app/routes/setting/settings/object-store/object-store.component.html,web-app/src/app/routes/setting/settings/object-store/object-store.component.ts) has been updated to allow users to select and configure Alibaba Cloud OSS. This includes adding theOSStype to the UI dropdown, dynamically displaying configuration fields, and providing internationalization support for English, Simplified Chinese, and Traditional Chinese in thei18nJSON files.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
🤖 Refacto.AI: PR Summary & Reviewer Guide🎯 1. The Goal (Why was this PR raised?)This PR implements support for Alibaba Cloud Object Storage Service (OSS) as a file storage provider in HertzBeat, extending the existing object storage capabilities which already include local file storage and Huawei Cloud OBS. 🗺️ 2. The Changes (What was implemented?)
🤔 3. Key Areas for Human ReviewArea of Concern: OSS Object Store Implementation
Area of Concern: OSS Service Initialization
Area of Concern: UI Form Validation
Area of Concern: Internationalization
|
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
There was a problem hiding this comment.
Code Review
This pull request adds support for Alibaba Cloud OSS as an object storage provider. The changes include backend logic for handling OSS configuration, a new service implementation for OSS operations, and frontend components to configure it.
The implementation is solid, but I have a few suggestions to improve maintainability and efficiency:
- In
OssObjectStoreServiceImpl, thelistmethod can be made more efficient and readable by simplifying the stream pipeline. - The
downloadmethod should catch more specific exceptions than the genericException. - On the frontend, the i18n keys for form labels should be dynamic to make the component more robust for future changes.
manager/src/main/java/org/apache/hertzbeat/manager/service/impl/OssObjectStoreServiceImpl.java
Show resolved
Hide resolved
manager/src/main/java/org/apache/hertzbeat/manager/service/impl/OssObjectStoreServiceImpl.java
Show resolved
Hide resolved
web-app/src/app/routes/setting/settings/object-store/object-store.component.html
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
manager/pom.xml (1)
202-206: LGTM! Consider checking for potential exclusions.The dependency declaration follows the correct pattern. However, you may want to verify if the OSS SDK introduces any transitive dependencies that should be excluded, similar to how the Huawei Cloud dependencies exclude
bgmprovider.manager/src/main/java/org/apache/hertzbeat/manager/service/impl/ObjectStoreConfigServiceImpl.java (1)
94-107: OSS initialization follows established patterns with room for improvement.The initOss method correctly mirrors the initObs implementation with proper validation and bean lifecycle management. However, consider adding exception handling around the OSS client creation.
Consider wrapping the OSS client creation in a try-catch block for better error handling:
- var ossClient = new OSSClientBuilder().build(ossConfig.getEndpoint(), ossConfig.getAccessKey(), ossConfig.getSecretKey()); + try { + var ossClient = new OSSClientBuilder().build(ossConfig.getEndpoint(), ossConfig.getAccessKey(), ossConfig.getSecretKey()); + } catch (Exception e) { + log.error("Failed to initialize OSS client: {}", e.getMessage()); + throw new IllegalStateException("OSS client initialization failed", e); + }manager/src/main/java/org/apache/hertzbeat/manager/service/impl/OssObjectStoreServiceImpl.java (2)
61-70: Consider improving error handling in download method.While the basic exception handling logs a warning, returning null for any exception may hide important error details from callers. Consider throwing a more specific exception or providing error details.
- } catch (Exception ex) { - log.warn("download file from oss error {}", objectKey); - return null; - } + } catch (Exception ex) { + log.error("Failed to download file from OSS: {}", objectKey, ex); + throw new RuntimeException("Download failed for object: " + objectKey, ex); + }
73-79: List method may have performance issues with large datasets.The current implementation fetches the actual content stream for each file during listing, which could be expensive for directories with many files. Consider whether the FileDTO really needs the InputStream for listing operations.
Consider separating concerns - provide a lightweight list operation that doesn't fetch content:
- return ossClient.listObjects(request).getObjectSummaries().stream() - .map(OSSObjectSummary::getKey).toList().stream() - .map(k -> new FileDTO(k, ossClient.getObject(bucketName, k).getObjectContent())).toList(); + return ossClient.listObjects(request).getObjectSummaries().stream() + .map(summary -> new FileDTO(summary.getKey(), null)) // Consider if InputStream is needed for listing + .toList();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
manager/pom.xml(2 hunks)manager/src/main/java/org/apache/hertzbeat/manager/pojo/dto/ObjectStoreDTO.java(2 hunks)manager/src/main/java/org/apache/hertzbeat/manager/service/impl/ObjectStoreConfigServiceImpl.java(2 hunks)manager/src/main/java/org/apache/hertzbeat/manager/service/impl/OssObjectStoreServiceImpl.java(1 hunks)web-app/src/app/pojo/ObjectStore.ts(2 hunks)web-app/src/app/routes/setting/settings/object-store/object-store.component.html(5 hunks)web-app/src/app/routes/setting/settings/object-store/object-store.component.ts(2 hunks)web-app/src/assets/i18n/en-US.json(2 hunks)web-app/src/assets/i18n/zh-CN.json(2 hunks)web-app/src/assets/i18n/zh-TW.json(2 hunks)
🔇 Additional comments (19)
web-app/src/app/routes/setting/settings/object-store/object-store.component.ts (2)
25-25: LGTM!The import addition for
OssConfigis correctly placed and follows the established pattern.
115-117: LGTM!The new case for
ObjectStoreType.OSScorrectly follows the established pattern and properly initializes the configuration with a newOssConfiginstance.web-app/src/assets/i18n/en-US.json (2)
554-554: LGTM!The OSS type label follows the established pattern and provides clear, consistent naming.
565-574: LGTM!The OSS configuration field localization strings are comprehensive and follow the established pattern. The labels and placeholders provide clear guidance to users.
web-app/src/assets/i18n/zh-CN.json (2)
555-555: LGTM!The Chinese OSS type label correctly uses "阿里云OSS" which is the proper Chinese name for Alibaba Cloud OSS.
566-575: LGTM!The Chinese OSS configuration field localization strings are well-translated and follow the established pattern. The use of "阿里云" terminology is correct and consistent.
web-app/src/app/routes/setting/settings/object-store/object-store.component.html (3)
35-35: LGTM!The OSS option addition to the dropdown follows the established pattern and correctly uses the enum value and localization.
38-38: Efficient approach to form field reuse.Extending the condition to show the same form fields for both OBS and OSS is a smart design choice that avoids code duplication while supporting both cloud providers with similar configuration parameters.
45-49: Well-implemented dynamic placeholders.The ternary operator approach for dynamic placeholders provides users with provider-specific guidance while maintaining a clean, unified form structure. This pattern is consistently applied across all configuration fields.
web-app/src/assets/i18n/zh-TW.json (1)
552-572: Localization entries for OSS are well-structured and complete.The Traditional Chinese translations for Alibaba Cloud OSS configuration are comprehensive and follow the established pattern. All necessary configuration fields (accessKey, secretKey, bucketName, endpoint, savePath) are properly localized with appropriate labels and placeholders.
web-app/src/app/pojo/ObjectStore.ts (2)
35-39: Clean enum extension for OSS support.The addition of the OSS enum value is properly structured with clear documentation linking to the Alibaba Cloud OSS console. The implementation follows TypeScript best practices.
50-56: OssConfig class mirrors ObsConfig structure correctly.The new OssConfig class maintains consistency with the existing ObsConfig implementation. All required fields are properly typed, and the default savePath value matches the backend configuration.
manager/src/main/java/org/apache/hertzbeat/manager/pojo/dto/ObjectStoreDTO.java (2)
57-62: OSS enum constant is properly documented.The new OSS enum value includes helpful documentation with a link to the Alibaba Cloud OSS console, maintaining consistency with the existing OBS documentation pattern.
81-95: OssConfig class follows established patterns.The new OssConfig inner class maintains perfect consistency with the existing ObsConfig structure. All required fields are present with appropriate types, and the default savePath value matches other implementations in the codebase.
manager/src/main/java/org/apache/hertzbeat/manager/service/impl/ObjectStoreConfigServiceImpl.java (2)
20-20: Import addition is appropriate.The OSSClientBuilder import is necessary for the new OSS initialization functionality.
87-89: Handler method updated correctly.The conditional logic for OSS type is properly integrated alongside the existing OBS handling, maintaining consistency in the configuration flow.
manager/src/main/java/org/apache/hertzbeat/manager/service/impl/OssObjectStoreServiceImpl.java (3)
43-51: Constructor properly handles root path normalization.The constructor correctly normalizes the root path by removing leading dashes, ensuring consistent object key construction.
54-58: Upload method implementation is solid.The upload method correctly constructs the object key and validates success by checking the HTTP status code.
87-89: Object key construction is clean and consistent.The getObjectKey method properly constructs keys using the established pattern with SignConstants.
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Solid Implementation - Well-Structured OSS Support Integration!
\n\nThis PR adds Alibaba Cloud OSS (Object Storage Service) support to HertzBeat's object storage capabilities, complementing the existing local file storage and Huawei Cloud OBS implementations. The implementation follows the established patterns in the codebase, properly extending the object store service with a new provider. The code is well-structured, includes appropriate error handling, and maintains consistency with existing implementations. The UI changes properly integrate the new storage option into the settings interface with appropriate internationalization support.\n\n |
![refacto-test[bot]](https://avatars.githubusercontent.com/u/196302493?s=60&v=4){kind=small}
manager/src/main/java/org/apache/hertzbeat/manager/service/impl/OssObjectStoreServiceImpl.java
Show resolved
Hide resolved
manager/src/main/java/org/apache/hertzbeat/manager/service/impl/OssObjectStoreServiceImpl.java
Show resolved
Hide resolved
manager/src/main/java/org/apache/hertzbeat/manager/service/impl/OssObjectStoreServiceImpl.java
Show resolved
Hide resolved
Solid Implementation - Well-Structured OSS Support Integration!
|
manager/src/main/java/org/apache/hertzbeat/manager/service/impl/OssObjectStoreServiceImpl.java
Show resolved
Hide resolved
Solid Implementation - Well-Structured OSS Support Integration!
|
manager/src/main/java/org/apache/hertzbeat/manager/service/impl/OssObjectStoreServiceImpl.java
Show resolved
Hide resolved
Solid Implementation - Well-Structured OSS Support Integration!
|
| public FileDTO download(String filePath) { | ||
| var objectKey = getObjectKey(filePath); | ||
| try { | ||
| var ossObject = ossClient.getObject(bucketName, objectKey); |
There was a problem hiding this comment.
Potential Resource Leak in OssObjectStoreServiceImpl
The download method retrieves an OSSObject but doesn't close it when an exception occurs. According to Alibaba Cloud OSS SDK documentation, OSSObject implements Closeable and should be properly closed to avoid resource leaks, especially for the input stream returned by getObjectContent(). In the current implementation, if an exception occurs after obtaining the OSSObject but before returning, the resources may not be properly released.
- public FileDTO download(String filePath) {
-
var objectKey = getObjectKey(filePath); -
try { -
var ossObject = ossClient.getObject(bucketName, objectKey); -
// Create a new FileDTO with the content stream -
// The caller is responsible for closing the stream after use -
return new FileDTO(filePath, ossObject.getObjectContent()); -
} catch (Exception ex) { -
log.warn("download file from oss error {}", objectKey); -
return null; -
} - }
| public FileDTO download(String filePath) { | ||
| var objectKey = getObjectKey(filePath); | ||
| try { | ||
| var ossObject = ossClient.getObject(bucketName, objectKey); |
There was a problem hiding this comment.
Potential Resource Leak in OssObjectStoreServiceImpl
The download method retrieves an OSSObject but doesn't close it when an exception occurs. According to Alibaba Cloud OSS SDK documentation, OSSObject implements Closeable and should be properly closed to avoid resource leaks, especially for the input stream returned by getObjectContent(). In the current implementation, if an exception occurs after obtaining the OSSObject but before returning, the resources may not be properly released.
| var ossObject = ossClient.getObject(bucketName, objectKey); | |
| + public FileDTO download(String filePath) { | |
| + var objectKey = getObjectKey(filePath); | |
| + try { | |
| + var ossObject = ossClient.getObject(bucketName, objectKey); | |
| + // Create a new FileDTO with the content stream | |
| + // The caller is responsible for closing the stream after use | |
| + return new FileDTO(filePath, ossObject.getObjectContent()); | |
| + } catch (Exception ex) { | |
| + log.warn("download file from oss error {}", objectKey); | |
| + return null; | |
| + } | |
| + } |
| var ossObject = ossClient.getObject(bucketName, objectKey); | ||
| return new FileDTO(filePath, ossObject.getObjectContent()); | ||
| } catch (Exception ex) { | ||
| log.warn("download file from oss error {}", objectKey); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential Resource Leak in OssObjectStoreServiceImpl
The download method retrieves an OSSObject but doesn't close it when an exception occurs. According to Alibaba Cloud OSS SDK documentation, OSSObject implements Closeable and should be properly closed to avoid resource leaks, especially for the input stream returned by getObjectContent(). In the current implementation, if an exception occurs after obtaining the OSSObject but before returning, the resources may not be properly released.
| var ossObject = ossClient.getObject(bucketName, objectKey); | |
| return new FileDTO(filePath, ossObject.getObjectContent()); | |
| } catch (Exception ex) { | |
| log.warn("download file from oss error {}", objectKey); | |
| return null; | |
| } | |
| } | |
| + public FileDTO download(String filePath) { | |
| + var objectKey = getObjectKey(filePath); | |
| + try { | |
| + var ossObject = ossClient.getObject(bucketName, objectKey); | |
| + // Create a new FileDTO with the content stream | |
| + // The caller is responsible for closing the stream after use | |
| + return new FileDTO(filePath, ossObject.getObjectContent()); | |
| + } catch (Exception ex) { | |
| + log.warn("download file from oss error {}", objectKey); | |
| + return null; | |
| + } | |
| + } |
Solid Implementation - Well-Structured OSS Support Integration!
|
| var ossObject = ossClient.getObject(bucketName, objectKey); | ||
| return new FileDTO(filePath, ossObject.getObjectContent()); | ||
| } catch (Exception ex) { | ||
| log.warn("download file from oss error {}", objectKey); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential Resource Leak in OssObjectStoreServiceImpl
The download method retrieves an OSSObject but doesn't close it when an exception occurs. According to Alibaba Cloud OSS SDK documentation, OSSObject implements Closeable and should be properly closed to avoid resource leaks, especially for the input stream returned by getObjectContent(). In the current implementation, if an exception occurs after obtaining the OSSObject but before returning, the resources may not be properly released.
| var ossObject = ossClient.getObject(bucketName, objectKey); | |
| return new FileDTO(filePath, ossObject.getObjectContent()); | |
| } catch (Exception ex) { | |
| log.warn("download file from oss error {}", objectKey); | |
| return null; | |
| } | |
| } | |
| + public FileDTO download(String filePath) { | |
| + var objectKey = getObjectKey(filePath); | |
| + try { | |
| + var ossObject = ossClient.getObject(bucketName, objectKey); | |
| + // Create a new FileDTO with the content stream | |
| + // The caller is responsible for closing the stream after use | |
| + return new FileDTO(filePath, ossObject.getObjectContent()); | |
| + } catch (Exception ex) { | |
| + log.warn("download file from oss error {}", objectKey); | |
| + return null; | |
| + } | |
| + } |
Solid Foundation - Critical Connection Validation Improvements Needed!
|
What's changed?
Checklist
Add or update API
Summary by CodeRabbit
New Features
Localization
Chores