[feature] add alibaba cloud oss support #8
Conversation
|
|
WalkthroughThis change introduces support for Alibaba Cloud OSS (Object Storage Service) as an additional object storage provider in the system. The backend adds OSS-specific configuration handling, a new implementation for object storage operations using the Alibaba OSS SDK, and updates the object store DTO to include OSS configuration. The frontend is updated to allow users to select OSS, enter the relevant credentials, and view localized UI strings for OSS in English, Simplified Chinese, and Traditional Chinese. The build configuration is updated to include the Alibaba OSS SDK dependency. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Backend
participant OSS_SDK
User->>Frontend: Selects OSS as object store type, enters credentials
Frontend->>Backend: Submits OSS configuration
Backend->>Backend: Parses and validates OSS config
Backend->>OSS_SDK: Initializes OSS client with credentials
Backend->>Backend: Registers OssObjectStoreServiceImpl
User->>Frontend: Uploads/downloads file
Frontend->>Backend: Sends file operation request
Backend->>OssObjectStoreServiceImpl: Handles upload/download/list
OssObjectStoreServiceImpl->>OSS_SDK: Performs OSS operation
OSS_SDK-->>OssObjectStoreServiceImpl: Returns result
OssObjectStoreServiceImpl-->>Backend: Returns result
Backend-->>Frontend: Responds with operation outcome
Poem
✨ Finishing Touches
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:
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 full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
manager/src/main/java/org/apache/hertzbeat/manager/service/impl/ObjectStoreConfigServiceImpl.java (1)
87-89: Consider using 'else if' instead of a separate 'if' condition.The current implementation uses a separate
ifstatement after the previous condition. While this works for enum values, usingelse ifwould be more explicit and prevent potential issues if the code evolves.- if (config.getType() == ObjectStoreDTO.Type.OSS) { + else if (config.getType() == ObjectStoreDTO.Type.OSS) { initOss(config); }web-app/src/app/routes/setting/settings/object-store/object-store.component.html (1)
38-137: UI labels are not updated for OSS.While the placeholders are conditionally changed based on the selected storage type, the form labels still reference OBS-specific labels even when OSS is selected. Consider updating the form labels to be conditional as well.
For example, update line 80-81 from:
<nz-form-label [nzSpan]="6" nzFor="obs.bucketName" nzRequired="true">{{ 'settings.object-store.obs.bucketName' | i18n }}</nz-form-label>To:
<nz-form-label [nzSpan]="6" nzFor="obs.bucketName" nzRequired="true">{{ config.type == ObjectStoreType.OBS ? ('settings.object-store.obs.bucketName' | i18n) : ('settings.object-store.oss.bucketName' | i18n) }}</nz-form-label>Apply similar changes to other form labels as well.
📜 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 context used
🧬 Code Graph Analysis (2)
web-app/src/app/routes/setting/settings/object-store/object-store.component.ts (1)
web-app/src/app/pojo/ObjectStore.ts (1)
OssConfig(50-56)
manager/src/main/java/org/apache/hertzbeat/manager/pojo/dto/ObjectStoreDTO.java (1)
web-app/src/app/pojo/ObjectStore.ts (1)
OssConfig(50-56)
🔇 Additional comments (24)
manager/pom.xml (2)
40-40: Good addition of Alibaba OSS version property.The version property for Alibaba OSS is correctly defined and follows the established project pattern for dependency management.
202-206:❓ Verification inconclusive
LGTM on OSS dependency addition.
The Alibaba Cloud OSS SDK dependency is correctly added with proper groupId, artifactId, and version reference. This provides the necessary library to implement OSS object storage service.
However, I notice other cloud dependencies (like Huawei) have exclusions for conflicting dependencies. Might be worth checking if any exclusions are needed here too.
🏁 Script executed:
#!/bin/bash # Check if there are any potential dependency conflicts # Look for any bgmprovider exclusions (like the ones for Huawei SDK) echo "Checking for potential dependency conflicts with Alibaba OSS SDK..." mvn dependency:tree -Dincludes=com.aliyun.oss:aliyun-sdk-oss,org.openeuler:bgmprovider # Also check if the latest version is significantly newer echo -e "\nLatest Alibaba OSS SDK version:" curl -s https://search.maven.org/solrsearch/select?q=g:com.aliyun.oss+AND+a:aliyun-sdk-oss&wt=json | jq -r '.response.docs[0].latestVersion'Length of output: 1290
#!/bin/bash # Locate the defined alibaba.oss.version property to compare against the latest (3.18.1) rg -Hn "<alibaba.oss.version>" manager/pom.xml
LGTM on Alibaba Cloud OSS SDK addition; verify if exclusions are needed
The Alibaba Cloud OSS SDK dependency is correctly declared in
manager/pom.xml(lines 202–206). To avoid potential transitive conflicts—similar to the exclusions applied for the Huawei SDK—please review the OSS SDK’s dependencies and add any necessary<exclusion>entries.
- File:
manager/pom.xml(lines 202–206)
<dependency> com.aliyun.oss:aliyun-sdk-oss:${alibaba.oss.version} </dependency>web-app/src/app/routes/setting/settings/object-store/object-store.component.ts (2)
25-25: Correctly imported the OSS config class.The OssConfig class is properly imported with other object store related classes.
115-117: Well implemented OSS config initialization.The implementation follows the existing pattern for handling object store type changes by initializing an appropriate configuration object when OSS is selected. This ensures consistency with how other storage types like OBS are handled.
web-app/src/assets/i18n/zh-CN.json (2)
555-555: Proper localization for Alibaba OSS type label.The Chinese translation for "Alibaba Cloud OSS" is correctly added as "阿里云OSS".
566-575: Complete localization for OSS configuration fields.All necessary OSS configuration field labels and placeholders are properly translated to Simplified Chinese, including AccessKey, SecretKey, Bucket, EndPoint, and save path. The translations provide useful guidance to users by clearly explaining what information is needed for each field.
web-app/src/assets/i18n/zh-TW.json (2)
552-552: Proper Traditional Chinese localization for Alibaba OSS type label.The Traditional Chinese translation for "Alibaba Cloud OSS" is correctly added as "阿裏雲OSS".
563-572: Complete Traditional Chinese localization for OSS configuration.All necessary OSS configuration field labels and placeholders are properly translated to Traditional Chinese, including AccessKey, SecretKey, Bucket, EndPoint, and save path. The translations maintain consistency with the Simplified Chinese version while using appropriate Traditional Chinese characters.
web-app/src/app/pojo/ObjectStore.ts (2)
35-39: Addition of Alibaba Cloud OSS to the ObjectStoreType looks good.The implementation correctly adds OSS as a new option in the ObjectStoreType enum with appropriate documentation linking to the Alibaba Cloud OSS console.
50-56: OssConfig class implementation is consistent with ObsConfig.The new OssConfig class follows the same structure as the existing ObsConfig class, maintaining consistency across different storage providers. Both classes share the same property structure and default values.
manager/src/main/java/org/apache/hertzbeat/manager/pojo/dto/ObjectStoreDTO.java (2)
57-62: Addition of OSS enum value is consistent with the frontend implementation.The OSS enum value is properly documented with a link to the Alibaba Cloud OSS console, matching the frontend implementation.
81-95: OssConfig class implementation is consistent with ObsConfig.The new static OssConfig class follows the same structure as the existing ObsConfig class, providing a clean and consistent approach for handling different storage providers.
manager/src/main/java/org/apache/hertzbeat/manager/service/impl/ObjectStoreConfigServiceImpl.java (2)
20-20: Import for OSSClientBuilder has been added correctly.The required import for Alibaba Cloud OSS SDK has been added to support OSS client creation.
94-107:✅ Verification successful
Implementation of initOss follows the same pattern as initObs.
The
initOssmethod correctly follows the same pattern asinitObs:
- Converting config to OssConfig
- Validating required parameters
- Creating an OSS client
- Registering a new OSS service implementation
One thing to verify is whether the
OssObjectStoreServiceImplclass is properly implemented, as it's referenced here but not included in the files for review.
🏁 Script executed:
#!/bin/bash # Check if OssObjectStoreServiceImpl is implemented rg -A 5 "class OssObjectStoreServiceImpl"Length of output: 791
🏁 Script executed:
#!/bin/bash # Display the first 200 lines of OssObjectStoreServiceImpl to review method implementations sed -n '1,200p' manager/src/main/java/org/apache/hertzbeat/manager/service/impl/OssObjectStoreServiceImpl.java # Display the first 200 lines of the ObjectStoreService interface to compare required methods sed -n '1,200p' manager/src/main/java/org/apache/hertzbeat/manager/service/ObjectStoreService.javaLength of output: 5021
OssObjectStoreServiceImpl correctly implements ObjectStoreService
The class
OssObjectStoreServiceImplis present and provides all required methods (upload,download,list, andtype) as defined byObjectStoreService. No further changes are needed.web-app/src/app/routes/setting/settings/object-store/object-store.component.html (3)
35-35: OSS option has been added to the dropdown.The Alibaba Cloud OSS option has been correctly added to the object store type selector dropdown.
38-38: Conditional rendering has been updated to include OSS type.The form items have been correctly updated to display for both OBS and OSS types using the
*ngIfdirective.Also applies to: 58-58, 78-78, 98-98, 118-118
46-49:✅ Verification successful
Placeholder text conditionally changes based on selected store type.
The implementation correctly switches between OBS and OSS placeholders based on the selected storage type. Make sure that all the referenced translation keys exist in the localization files.
Also applies to: 65-69, 85-89, 105-109, 125-129
🏁 Script executed:
#!/bin/bash # Check if the OSS-related translation keys exist in localization files rg "settings.object-store.oss" web-app/src/assets/i18n/Length of output: 3169
🏁 Script executed:
#!/bin/bash # Check if the OBS-related translation keys exist in localization files rg "settings.object-store.obs" web-app/src/assets/i18n/Length of output: 3165
Approve conditional placeholder implementation
I confirm that the template correctly switches between OBS and OSS placeholders based on the selected store type, and all referenced i18n keys exist in zh‑CN, zh‑TW, and en‑US files. No further action is needed.
• Affected file:
web-app/src/app/routes/setting/settings/object-store/object-store.component.html
– Lines 46–49, 65–69, 85–89, 105–109, 125–129web-app/src/assets/i18n/en-US.json (2)
554-554: LGTM! Good job adding support for Alibaba Cloud OSS.The entry for Alibaba Cloud OSS in the file server provider dropdown menu is properly added and follows the same format as the existing Huawei Cloud OBS option.
565-574: LGTM! Configuration entries are complete and consistent.The localization entries for OSS configuration fields (AccessKey, SecretKey, BucketName, EndPoint, and SavePath) are properly implemented with clear labels and helpful placeholder text. They follow the same pattern as the existing Huawei Cloud OBS entries, maintaining consistency in the UI.
manager/src/main/java/org/apache/hertzbeat/manager/service/impl/OssObjectStoreServiceImpl.java (5)
32-36: LGTM! Implementation class properly defined.The class declaration appropriately implements the ObjectStoreService interface and uses @slf4j for logging.
43-51: LGTM! Constructor properly handles path normalization.The constructor correctly initializes the required fields and normalizes the root path by removing a leading dash if present.
60-70: LGTM! Good error handling in download method.The download method properly handles exceptions by catching them, logging a warning, and returning null.
81-84: LGTM! Type method correctly returns OSS enum value.The type method properly returns the OSS enum value from ObjectStoreDTO.Type.
87-89: LGTM! Helper method properly constructs object key.The getObjectKey helper method correctly concatenates the root path and file path with a dash separator.
| @Override | ||
| public boolean upload(String filePath, InputStream is) { | ||
| var objectKey = getObjectKey(filePath); | ||
| var response = ossClient.putObject(bucketName, objectKey, is); | ||
| return Objects.equals(response.getResponse().getStatusCode(), 200); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to prevent uncaught exceptions.
The upload method doesn't have any exception handling, which could cause unexpected crashes if the OSS client throws an exception during upload.
@Override
public boolean upload(String filePath, InputStream is) {
- var objectKey = getObjectKey(filePath);
- var response = ossClient.putObject(bucketName, objectKey, is);
- return Objects.equals(response.getResponse().getStatusCode(), 200);
+ try {
+ var objectKey = getObjectKey(filePath);
+ var response = ossClient.putObject(bucketName, objectKey, is);
+ return response.getResponse() != null &&
+ Objects.equals(response.getResponse().getStatusCode(), 200);
+ } catch (Exception ex) {
+ log.error("Failed to upload file to OSS: {}", filePath, ex);
+ return false;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| public boolean upload(String filePath, InputStream is) { | |
| var objectKey = getObjectKey(filePath); | |
| var response = ossClient.putObject(bucketName, objectKey, is); | |
| return Objects.equals(response.getResponse().getStatusCode(), 200); | |
| } | |
| @Override | |
| public boolean upload(String filePath, InputStream is) { | |
| try { | |
| var objectKey = getObjectKey(filePath); | |
| var response = ossClient.putObject(bucketName, objectKey, is); | |
| return response.getResponse() != null && | |
| Objects.equals(response.getResponse().getStatusCode(), 200); | |
| } catch (Exception ex) { | |
| log.error("Failed to upload file to OSS: {}", filePath, ex); | |
| return false; | |
| } | |
| } |
| @Override | ||
| public List<FileDTO> list(String dir) { | ||
| var request = new ListObjectsRequest(bucketName); | ||
| request.setPrefix(getObjectKey(dir)); | ||
| return ossClient.listObjects(request).getObjectSummaries().stream() | ||
| .map(OSSObjectSummary::getKey).toList().stream() | ||
| .map(k -> new FileDTO(k, ossClient.getObject(bucketName, k).getObjectContent())).toList(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and performance in list method.
The list method has two issues:
- No exception handling, which could lead to unexpected crashes
- Each object is fetched individually with a separate API call, which can be inefficient for directories with many files
@Override
public List<FileDTO> list(String dir) {
+ try {
var request = new ListObjectsRequest(bucketName);
request.setPrefix(getObjectKey(dir));
- return ossClient.listObjects(request).getObjectSummaries().stream()
- .map(OSSObjectSummary::getKey).toList().stream()
- .map(k -> new FileDTO(k, ossClient.getObject(bucketName, k).getObjectContent())).toList();
+ var summaries = ossClient.listObjects(request).getObjectSummaries();
+ return summaries.stream()
+ .map(summary -> {
+ try {
+ String key = summary.getKey();
+ var object = ossClient.getObject(bucketName, key);
+ return new FileDTO(key, object.getObjectContent());
+ } catch (Exception e) {
+ log.warn("Failed to get object content for key: {}", summary.getKey(), e);
+ return null;
+ }
+ })
+ .filter(Objects::nonNull)
+ .toList();
+ } catch (Exception ex) {
+ log.error("Failed to list files from OSS directory: {}", dir, ex);
+ return List.of();
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| public List<FileDTO> list(String dir) { | |
| var request = new ListObjectsRequest(bucketName); | |
| request.setPrefix(getObjectKey(dir)); | |
| return ossClient.listObjects(request).getObjectSummaries().stream() | |
| .map(OSSObjectSummary::getKey).toList().stream() | |
| .map(k -> new FileDTO(k, ossClient.getObject(bucketName, k).getObjectContent())).toList(); | |
| } | |
| @Override | |
| public List<FileDTO> list(String dir) { | |
| try { | |
| var request = new ListObjectsRequest(bucketName); | |
| request.setPrefix(getObjectKey(dir)); | |
| var summaries = ossClient.listObjects(request).getObjectSummaries(); | |
| return summaries.stream() | |
| .map(summary -> { | |
| try { | |
| String key = summary.getKey(); | |
| var object = ossClient.getObject(bucketName, key); | |
| return new FileDTO(key, object.getObjectContent()); | |
| } catch (Exception e) { | |
| log.warn("Failed to get object content for key: {}", summary.getKey(), e); | |
| return null; | |
| } | |
| }) | |
| .filter(Objects::nonNull) | |
| .toList(); | |
| } catch (Exception ex) { | |
| log.error("Failed to list files from OSS directory: {}", dir, ex); | |
| return List.of(); | |
| } | |
| } |
|
/codehelper review |
![devd-pr-reviewer[bot]](https://avatars.githubusercontent.com/u/196302493?s=60&v=4){kind=small}
| this.rootPath = rootPath; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Suggestion: Consider adding a close() method to properly shut down the OSS client when it's no longer needed. The Alibaba OSS client resources should be released when done to prevent resource leaks.
| var objectKey = getObjectKey(filePath); | ||
| var response = ossClient.putObject(bucketName, objectKey, is); | ||
| return Objects.equals(response.getResponse().getStatusCode(), 200); | ||
| } |
There was a problem hiding this comment.
Issue: The response check may not be reliable. The OSS client's putObject method can return a response with null response field in some cases. Consider checking for exceptions instead of status code.
| var request = new ListObjectsRequest(bucketName); | ||
| request.setPrefix(getObjectKey(dir)); | ||
| return ossClient.listObjects(request).getObjectSummaries().stream() | ||
| .map(OSSObjectSummary::getKey).toList().stream() |
There was a problem hiding this comment.
Performance concern: This implementation loads all objects in memory at once. For directories with many files, this could lead to memory issues. Consider implementing pagination or streaming the results.
| request.setPrefix(getObjectKey(dir)); | ||
| return ossClient.listObjects(request).getObjectSummaries().stream() | ||
| .map(OSSObjectSummary::getKey).toList().stream() | ||
| .map(k -> new FileDTO(k, ossClient.getObject(bucketName, k).getObjectContent())).toList(); |
There was a problem hiding this comment.
Potential resource leak: The OSS objects retrieved in the list method aren't being closed. Each getObject() call opens a stream that should be properly closed to avoid resource leaks.
| Assert.hasText(ossConfig.getEndpoint(), "cannot find oss endpoint"); | ||
| Assert.hasText(ossConfig.getBucketName(), "cannot find oss bucket name"); | ||
|
|
||
| var ossClient = new OSSClientBuilder().build(ossConfig.getEndpoint(), ossConfig.getAccessKey(), ossConfig.getSecretKey()); |
There was a problem hiding this comment.
Missing validation: Unlike the initOss method, there's no validation for the OSS client initialization. Consider adding error handling to check if the OSS client was successfully created.
| @@ -83,10 +84,28 @@ public void handler(ObjectStoreDTO<T> config) { | |||
| initObs(config); | |||
| // case other object store service | |||
| } | |||
| if (config.getType() == ObjectStoreDTO.Type.OSS) { | |||
There was a problem hiding this comment.
Code structure: Consider using a switch statement instead of multiple if conditions for handling different object store types, which would be more maintainable as more store types are added.
| /** | ||
| * <a href="https://oss.console.aliyun.com/services/tools">Alibaba Cloud OSS</a> | ||
| */ | ||
| OSS = 'OSS' |
There was a problem hiding this comment.
Suggestion: Consider adding a trailing comma after the OSS enum value for consistency with other enum values and to make future additions cleaner in git diffs.
| @@ -66,14 +75,18 @@ | |||
| /> | |||
There was a problem hiding this comment.
DRY principle: There's significant duplication in the HTML template for handling OBS and OSS configurations. Consider refactoring to use a common template or component for both cloud storage types to improve maintainability.
What's changed?
Checklist
Add or update API
Summary by CodeRabbit
New Features
Localization
UI Enhancements