CAMEL-24373: Add camel-alibaba MVP (common, OSS, MNS) — phase 1 - #25433
CAMEL-24373: Add camel-alibaba MVP (common, OSS, MNS) — phase 1#25433atiaomar1978-hub wants to merge 12 commits into
Conversation
Bugbot / Grok Review — CAMEL-24373 Phase 1 MVPVerdict: Approve (fixes applied before upstream PR) Findings addressed
Tests (33 passing)./mvnw -pl components/camel-alibaba/camel-alibaba-common,components/camel-alibaba/camel-alibaba-oss,components/camel-alibaba/camel-alibaba-mns testFollow-ups (non-blocking)
AI-generated review on behalf of atiaomar1978-hub |
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thank you for this contribution, @atiaomar1978-hub — great to see Alibaba Cloud support coming to Camel! The component structure is well organized and follows Camel patterns nicely. I have a few items that need attention before this can be merged.
Blocking
- Unresolved merge conflict in
parent/pom.xml— the branch needs to be rebased on currentmainto resolve the conflict markers.
Significant
camel-alibaba-commondepends onalibabacloud-oss-v2— the common module pulls in the OSS SDK, which meanscamel-alibaba-mnstransitively gets the entire OSS SDK even though it doesn't use it.AlibabaClientBuilderUtil(which creates OSS clients) should live incamel-alibaba-oss; the common module should only contain truly shared code.OSSEndpoint.initClient()doesn't cache the created client — the method creates a new client but never assigns it tothis.ossClient, sogetOssClient()always returns null and clients may be leaked.AlibabaClientRegistryis unused — defined in common but never referenced by OSS or MNS.
Moderate
- OSS headers
Content-LengthandContent-Typecollide with standard HTTP headers — Camel convention uses theCamelXxxprefix for component headers (the other headers in the same class follow this correctly). OSSComponent.createEndpoint()missing@Override— the MNS component has it, this one doesn't.
Minor
- OSS producer serializes all SDK responses to JSON strings via Gson, which is unusual — Camel components typically set structured response metadata in headers/properties.
- Inconsistent switch style: MNS uses enhanced switch (
->), OSS uses traditional (case: break).
Questions
- Alibaba MNS is deprecated by Alibaba Cloud in favor of RocketMQ. Should the documentation note this?
OSSClientappears closeable but neither endpoint nor producer/consumer callsclose()on shutdown.
Note: This review covers project rules and conventions. It does not replace specialized tools such as CodeRabbit, Sourcery, or SonarCloud for deeper static analysis.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
| <avro-ipc-jetty-version>1.12.1</avro-ipc-jetty-version> | ||
| <avro-ipc-netty-version>1.12.1</avro-ipc-netty-version> | ||
| <awaitility-version>4.3.0</awaitility-version> | ||
| <<<<<<< HEAD |
There was a problem hiding this comment.
Blocker: Unresolved merge conflict markers. The branch needs to be rebased on current main.
<<<<<<< HEAD
<aws-java-sdk2-version>2.50.3</aws-java-sdk2-version>
=======
...
>>>>>>> 23fa869e949
After rebasing, the resolved version should keep the new alibabacloud-oss-version and aliyun-sdk-mns-version properties and use the latest aws-java-sdk2-version from main.
There was a problem hiding this comment.
Fixed — the merge conflict in parent/pom.xml was resolved and the branch has been rebased on current main (see commits 08dd1a2, 9d890ace, and latest eb936edc).
AI-generated reply on behalf of atiaomar1978-hub
| <artifactId>camel-support</artifactId> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>com.aliyun</groupId> |
There was a problem hiding this comment.
The alibabacloud-oss-v2 SDK dependency is OSS-specific but lives in the common module. This means camel-alibaba-mns (which depends on common) transitively pulls in the entire OSS SDK.
Consider moving AlibabaClientBuilderUtil (the only class that uses this SDK) into camel-alibaba-oss and removing this dependency from common. The common module should only contain ServiceKeys, AlibabaClientRegistry, and other truly shared code.
There was a problem hiding this comment.
Fixed — the OSS SDK dependency was removed from camel-alibaba-common. Client creation now lives in OSSUtils.createClient() inside camel-alibaba-oss; the common module only contains shared ServiceKeys.
AI-generated reply on behalf of atiaomar1978-hub
|
|
||
| String auth = getServiceKeys() != null ? getServiceKeys().getAccessKey() : getAccessKey(); | ||
| String secret = getServiceKeys() != null ? getServiceKeys().getSecretKey() : getSecretKey(); | ||
|
|
There was a problem hiding this comment.
The created client is returned but never assigned to this.ossClient, so getOssClient() always returns null after initialization and a new client is created on every call.
| this.ossClient = AlibabaClientBuilderUtil.createOssClient(auth, secret, region, endpoint); | |
| return ossClient; |
There was a problem hiding this comment.
Fixed — initClient() now assigns the created client to this.ossClient and returns the cached instance on subsequent calls. Non-autowired clients are closed in doStop().
AI-generated reply on behalf of atiaomar1978-hub
| @Component("alibaba-oss") | ||
| public class OSSComponent extends HealthCheckComponent { | ||
|
|
||
| protected Endpoint createEndpoint(String uri, String remaining, Map<String, Object> parameters) throws Exception { |
There was a problem hiding this comment.
Missing @Override annotation (the MNS component has it).
| protected Endpoint createEndpoint(String uri, String remaining, Map<String, Object> parameters) throws Exception { | |
| @Override | |
| protected Endpoint createEndpoint(String uri, String remaining, Map<String, Object> parameters) throws Exception { |
There was a problem hiding this comment.
Fixed — @Override has been added to OSSComponent.createEndpoint().
AI-generated reply on behalf of atiaomar1978-hub
3a8d899 to
1ddf05a
Compare
Review feedback addressed (@davsclaus)Thanks for the thorough review! The following items have been fixed in commits
Tests (33 passing)./mvnw -pl components/camel-alibaba/camel-alibaba-common,components/camel-alibaba/camel-alibaba-oss,components/camel-alibaba/camel-alibaba-mns testFollow-ups (non-blocking, unchanged)
Bugbot / Grok ReviewVerdict: Approve — prior blocking/significant review items addressed. Remaining non-blocking observations:
AI-generated comment on behalf of atiaomar1978-hub |
Bugbot / Grok follow-up (commit
|
| Severity | Issue | Fix |
|---|---|---|
| Significant | OSS credential resolution ignored endpoint AK/SK when empty ServiceKeys bean present |
Match MNS/Huawei pattern: prefer endpoint params, then non-empty ServiceKeys |
| Significant | MNS client never closed on shutdown | Added doStop() + autowiredMnsClient flag (mirrors OSS) |
| Moderate | listObjects ignored maxKeys and scanned entire bucket |
Cap collected results at maxKeys |
Bugbot verdict: Approve for phase 1 MVP after these commits.
Grok verdict: Request changes resolved — remaining items are non-blocking (Gson JSON bodies, OSS doc property vs header wording, optional integration tests).
All 33 unit tests still passing locally.
AI-generated comment on behalf of atiaomar1978-hub
|
okay so main is now ready for 4.23.0-SNAPSHOT and this PR needs to be rebased on top and update its versions |
ab358f2 to
9d890ac
Compare
Rebased on main for 4.23.0-SNAPSHOT (@davsclaus)Addressed the latest review comment:
TestsAll alibaba module unit tests passing locally: ./mvnw -pl components/camel-alibaba/camel-alibaba-common,components/camel-alibaba/camel-alibaba-oss,components/camel-alibaba/camel-alibaba-mns -am testCommit: Ready for re-review once CI is green. AI-generated comment on behalf of atiaomar1978-hub |
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
github-actions
Bot
added
components
core
catalog
docs
tooling
tooling-maven
core-build-and-dependencies
labels
9d890ac to
1554797
Compare
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thank you for the contribution, @atiaomar1978-hub — introducing Alibaba Cloud components is a great addition to the Camel ecosystem. The overall structure follows Camel conventions well (parent module layout, MojoHelper registration, lifecycle management, documentation), and the test coverage is solid.
I found several issues that need to be addressed before this can be merged. The most critical is a version mismatch in the BOM that will break downstream consumers.
Summary of findings
| # | Severity | Finding |
|---|---|---|
| 1 | Blocking | BOM hardcodes 4.22.0-SNAPSHOT instead of 4.23.0-SNAPSHOT |
| 2 | Important | OSS accessKey/secretKey missing secret = true — credentials won't be masked |
| 3 | Important | OSSProducer reads runtime overrides from exchange properties instead of headers |
| 4 | Important | OSSProducer serializes all results to JSON strings via Gson |
| 5 | Moderate | Inconsistent credential annotation style between OSS and MNS |
| 6 | Moderate | operation as @UriPath with label = "producer" but consumer also supported |
| 7 | Minor | Duplicate receipt-handle constants between MNSProperties and MNSHeaders |
See inline comments for details.
This review focuses on project conventions and code correctness. It does not replace specialized review tools such as CodeRabbit, Sourcery, or SonarCloud.
This review was generated by an AI agent (Claude Code on behalf of davsclaus) and may contain inaccuracies. Please verify all suggestions before applying.
| <dependency> | ||
| <groupId>org.apache.camel</groupId> | ||
| <artifactId>camel-alibaba-common</artifactId> | ||
| <version>4.22.0-SNAPSHOT</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.apache.camel</groupId> | ||
| <artifactId>camel-alibaba-mns</artifactId> | ||
| <version>4.22.0-SNAPSHOT</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.apache.camel</groupId> | ||
| <artifactId>camel-alibaba-oss</artifactId> | ||
| <version>4.22.0-SNAPSHOT</version> | ||
| </dependency> |
There was a problem hiding this comment.
Blocking: The three new BOM entries hardcode 4.22.0-SNAPSHOT instead of 4.23.0-SNAPSHOT. The parent/pom.xml and all module pom.xml files correctly use 4.23.0-SNAPSHOT. This version mismatch will break dependency management for any project importing the Camel BOM.
Note: the Camel BOM uses hardcoded versions (not ${project.version}), so these must match the current project version.
| <dependency> | |
| <groupId>org.apache.camel</groupId> | |
| <artifactId>camel-alibaba-common</artifactId> | |
| <version>4.22.0-SNAPSHOT</version> | |
| </dependency> | |
| <dependency> | |
| <groupId>org.apache.camel</groupId> | |
| <artifactId>camel-alibaba-mns</artifactId> | |
| <version>4.22.0-SNAPSHOT</version> | |
| </dependency> | |
| <dependency> | |
| <groupId>org.apache.camel</groupId> | |
| <artifactId>camel-alibaba-oss</artifactId> | |
| <version>4.22.0-SNAPSHOT</version> | |
| </dependency> | |
| <dependency> | |
| <groupId>org.apache.camel</groupId> | |
| <artifactId>camel-alibaba-common</artifactId> | |
| <version>4.23.0-SNAPSHOT</version> | |
| </dependency> | |
| <dependency> | |
| <groupId>org.apache.camel</groupId> | |
| <artifactId>camel-alibaba-mns</artifactId> | |
| <version>4.23.0-SNAPSHOT</version> | |
| </dependency> | |
| <dependency> | |
| <groupId>org.apache.camel</groupId> | |
| <artifactId>camel-alibaba-oss</artifactId> | |
| <version>4.23.0-SNAPSHOT</version> | |
| </dependency> |
There was a problem hiding this comment.
Fixed in eb936edc — all three alibaba BOM entries now use 4.23.0-SNAPSHOT, matching parent/pom.xml and the module POMs.
AI-generated reply on behalf of atiaomar1978-hub
| @UriParam(description = "Access key for the cloud user", displayName = "API access key (AK)", | ||
| security = "secret", label = "security") | ||
| @Metadata(required = true) | ||
| private String accessKey; |
There was a problem hiding this comment.
Important: Missing secret = true on @UriParam. The security = "secret" attribute controls the security policy framework (e.g., camel.main.profile = prod), but it does not mask the value in logs, JMX, and management APIs. For that, secret = true is required.
Per CLAUDE.md: "Mark sensitive parameters with secret = true on @UriParam or @Metadata (passwords, tokens, API keys)".
The MNS module correctly uses secret = true — this module should do the same. Apply the same fix to the secretKey and serviceKeys fields below.
| @UriParam(description = "Access key for the cloud user", displayName = "API access key (AK)", | |
| security = "secret", label = "security") | |
| @Metadata(required = true) | |
| private String accessKey; | |
| @UriParam(description = "Access key for the cloud user", displayName = "API access key (AK)", | |
| secret = true, security = "secret", label = "security") | |
| @Metadata(required = true) | |
| private String accessKey; |
There was a problem hiding this comment.
Fixed in eb936edc — accessKey, secretKey, and serviceKeys on OSSEndpoint now use secret = true, security = "secret", and label = "security" so credentials are masked in logs/JMX as well as covered by the security policy framework.
AI-generated reply on behalf of atiaomar1978-hub
| if (ossClient == null) { | ||
| this.ossClient = endpoint.initClient(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Important — exchange properties vs headers: The updateClientConfigs() method reads runtime overrides from exchange.getProperty(OSSProperties.*), but the Camel convention is to use message headers (exchange.getIn().getHeader(...)) for dynamic parameter overrides. Exchange properties are for internal exchange metadata.
The docs table in alibaba-oss-component.adoc has the column header "Header" but the code reads from properties — users following the documentation will call setHeader("CamelAlibabaOssOperation", ...) and the producer will silently ignore it.
Recommendation: change OSSProperties to be used as header constants (like OSSHeaders already is for the consumer), and read from exchange.getIn().getHeader(...) instead of exchange.getProperty(...).
There was a problem hiding this comment.
Fixed in eb936edc — runtime overrides are resolved in OSSUtils.createClientConfigurations(), which checks message headers first, then exchange properties as fallback, then endpoint/URI defaults. Tests and docs updated to use headers.
AI-generated reply on behalf of atiaomar1978-hub
| super.doInit(); | ||
| this.gson = new Gson(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Important — JSON serialization via Gson: All producer operations serialize results to JSON strings using gson.toJson(...). This is non-standard in Camel:
- Most components return the SDK result object or typed
Map<String, Object>as the body, letting Camel's type converter system handle serialization. - Users who want to work with the structured data must parse JSON back, defeating the purpose of an integration framework.
- This introduces a hard runtime dependency on Gson when Jackson is already ubiquitous in the Camel ecosystem.
Consider returning the Map<String, Object> directly (or the SDK result objects). Users can convert to JSON themselves if needed via Camel's data format system.
There was a problem hiding this comment.
Fixed in eb936edc — Gson was removed. Producer operations now return structured Map<String, Object> or List<Map<String, Object>> bodies. Tests assert on map entries instead of JSON strings.
AI-generated reply on behalf of atiaomar1978-hub
|
|
||
| @UriParam(description = "Configuration object for cloud service authentication", displayName = "Service Keys", | ||
| security = "secret") | ||
| private ServiceKeys serviceKeys; |
There was a problem hiding this comment.
Moderate — inconsistent annotation style: MNS correctly uses secret = true here, but is missing label = "security" for catalog grouping (which OSS has). For consistency across the two modules, both endpoints should use secret = true (for masking) AND label = "security" (for catalog grouping).
| @UriParam(description = "Configuration object for cloud service authentication", displayName = "Service Keys", | |
| security = "secret") | |
| private ServiceKeys serviceKeys; | |
| @UriParam(description = "Access key for the cloud user", displayName = "Access Key", | |
| secret = true, label = "security") | |
| private String accessKey; |
Apply the same to the secretKey and serviceKeys fields.
There was a problem hiding this comment.
Fixed in eb936edc — MNS credentials now match OSS: accessKey, secretKey, and serviceKeys all use secret = true, security = "secret", and label = "security".
AI-generated reply on behalf of atiaomar1978-hub
| public static final String RECEIPT_HANDLE = "CamelAlibabaMnsReceiptHandle"; | ||
|
|
||
| public static final String MESSAGE_ID = "CamelAlibabaMnsMessageId"; | ||
| public static final String REQUEST_ID = "CamelAlibabaMnsRequestId"; |
There was a problem hiding this comment.
Minor: MNSProperties.RECEIPT_HANDLE and MNSHeaders.RECEIPT_HANDLE both resolve to "CamelAlibabaMnsReceiptHandle". In MNSUtils.resolveReceiptHandle(), the method checks 3 different sources (property by MNSProperties.RECEIPT_HANDLE, header by MNSProperties.RECEIPT_HANDLE, header by MNSHeaders.RECEIPT_HANDLE) — but the last two are identical. Consider removing the duplicate constant or consolidating the lookup.
There was a problem hiding this comment.
Fixed in eb936edc — removed RECEIPT_HANDLE, MESSAGE_ID, and MESSAGE_BODY_MD5 from MNSProperties. MNSUtils.resolveReceiptHandle() and MNSProducer now use MNSHeaders constants exclusively.
AI-generated reply on behalf of atiaomar1978-hub
| @UriEndpoint(firstVersion = "4.23.0", scheme = "alibaba-oss", title = "Alibaba Object Storage Service (OSS)", | ||
| syntax = "alibaba-oss:operation", | ||
| category = { Category.CLOUD }, headersClass = OSSHeaders.class) |
There was a problem hiding this comment.
Moderate — URI design for consumer use: The syntax alibaba-oss:operation requires an operation as the URI path, but this component supports both producer and consumer. When used as a consumer (from("alibaba-oss:listObjects?bucketName=...")), the user must provide an operation name that is meaningless for consumers.
Consider using bucketName as the path parameter (similar to camel-aws2-s3) and making operation a query parameter, e.g.:
syntax = "alibaba-oss:bucketName"
This would make the consumer URI more natural: from("alibaba-oss:my-bucket?deleteAfterRead=true").
There was a problem hiding this comment.
Fixed in eb936edc — URI syntax is now alibaba-oss:bucketName?operation=putObject&.... The bucket name is the URI path; operation is a query parameter. Docs, tests, and catalog metadata updated.
AI-generated reply on behalf of atiaomar1978-hub
Implement Alibaba Cloud OSS component using alibabacloud-oss-v2 SDK: - Producer operations: listBuckets, listObjects, putObject, getObject, deleteObject, copyObject, headObject - Consumer polls listObjectsV2 with optional deleteAfterRead - Shared AlibabaClientBuilderUtil in camel-alibaba-common - Unit tests with AssertJ and Mockito mock OSSClient Co-authored-by: Omar Atie <atiaomar1978-hub@users.noreply.github.com>
Implement Alibaba Cloud Message Service (MNS) component with queue and topic support. Includes sendMessage, receiveMessage, deleteMessage, and publishMessage operations, ScheduledBatchPollingConsumer for queue polling, HealthCheckComponent integration, unit tests with Mockito/AssertJ, and component documentation. Co-authored-by: Omar Atie <atiaomar1978-hub@users.noreply.github.com>
…stry Add BOM/coverage entries, SDK version properties, fix MojoHelper to list component modules only, and add AlibabaClientRegistry with tests. Co-authored-by: Omar Atie <atiaomar1978-hub@users.noreply.github.com>
Co-authored-by: Omar Atie <atiaomar1978-hub@users.noreply.github.com>
- Default MNS queue producer to sendMessage; add test - Validate OSS bucket name for all putObject body types - Advance OSS consumer continuation token only after successful batch - Remove non-existent Spring Boot starter from MNS docs - Regenerate catalog entries for alibaba-oss and alibaba-mns Co-authored-by: Omar Atie <atiaomar1978-hub@users.noreply.github.com>
- Resolve parent/pom.xml merge conflict and keep aws-java-sdk2 2.50.3 - Move OSS client builder from common to OSSUtils in camel-alibaba-oss - Remove unused AlibabaClientRegistry and OSS SDK dependency from common - Cache OSS client on endpoint and close on shutdown when not autowired - Rename OSS content headers to CamelAlibabaOss* convention - Add @OverRide on OSSComponent.createEndpoint() - Document MNS deprecation guidance in component docs Co-authored-by: Cursor Agent <noreply@cursor.com>
Co-authored-by: Cursor Agent <noreply@cursor.com>
…nt lifecycle - Prefer endpoint accessKey/secretKey over empty ServiceKeys bean (OSS) - Cap listObjects results at maxKeys instead of scanning entire bucket - Close MNS client on endpoint stop when not autowired Co-authored-by: Cursor Agent <noreply@cursor.com>
Co-authored-by: Cursor Agent <noreply@cursor.com>
- Rebased on upstream main (4.23.0-SNAPSHOT) - Updated firstVersion and docs to 4.23 - Resolved parent/pom.xml conflicts; alibaba SDK versions in alpha order - Regenerated catalog and component metadata Co-authored-by: Cursor Agent <noreply@cursor.com>
1554797 to
3adb638
Compare
- Fix BOM alibaba module versions to 4.23.0-SNAPSHOT - Redesign OSS URI: bucketName in path, operation as query param - Mark OSS/MNS credentials with secret=true and security label - OSS producer resolves runtime overrides from headers first - Return structured Map/List bodies instead of Gson JSON strings - Remove duplicate MNS receipt-handle/message constants from MNSProperties - Update tests, docs, and catalog metadata Co-authored-by: Cursor Agent <noreply@cursor.com>
Review feedback addressed (@davsclaus)Thanks for the follow-up review. The latest commit (
Docs, tests, component metadata, and catalog JSON updated accordingly. TestsAll alibaba module unit tests passing locally: ./mvnw -pl components/camel-alibaba/camel-alibaba-common,components/camel-alibaba/camel-alibaba-oss,components/camel-alibaba/camel-alibaba-mns -am test -Dtest='org.apache.camel.component.alibaba.**' -Dsurefire.failIfNoSpecifiedTests=falseReady for re-review once CI is green. AI-generated comment on behalf of atiaomar1978-hub |
…ents Commit generated files produced by the regen build so CI "Fail if there are uncommitted changes" passes. Co-authored-by: Cursor Agent <noreply@cursor.com>
|
@davsclaus please re-trigger the CI . thanks |
|
triggered |
3 participants
Summary
Phase 1 (MVP) of CAMEL-24373: introduces the new
components/camel-alibaba/parent module with shared common code and initial Alibaba Cloud components for Object Storage (OSS) and Message Service (MNS).Target version: Camel 4.23.0 (rebased on
main@4.23.0-SNAPSHOT)Modules added
camel-alibaba-commonServiceKeysonly (no SDK dependencies)camel-alibaba-ossalibaba-oss:alibabacloud-oss-v2SDKcamel-alibaba-mnsalibaba-mns:aliyun-sdk-mnsSDKReview feedback addressed
main(4.23.0-SNAPSHOT) per @davsclausOSSUtils.createClient(); removed unused registry from commonOSSEndpoint/MNSEndpointclient caching and shutdown lifecycleCamelAlibabaOss*conventionTest plan
./mvnw -pl components/camel-alibaba/camel-alibaba-common,components/camel-alibaba/camel-alibaba-oss,components/camel-alibaba/camel-alibaba-mns -am testCloses CAMEL-24373 (phase 1 MVP)
AI-generated PR description on behalf of atiaomar1978-hub