feat: add Alibaba Cloud SMS adapter (#6307)#109
Conversation
WalkthroughThis pull request introduces a new Alibaba Cloud SMS adapter. The implementation includes a new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![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 (3)
src/Utopia/Messaging/Adapter/SMS/AlibabaCloud.php (3)
56-56: Consider making RegionId configurable.The
RegionIdis hardcoded to'cn-hangzhou'. Alibaba Cloud SMS is available in multiple regions. Consider accepting this as a constructor parameter with a default value for flexibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Utopia/Messaging/Adapter/SMS/AlibabaCloud.php` at line 56, The RegionId is hardcoded; update the AlibabaCloud class to accept a configurable region by adding a constructor parameter (e.g., $region = 'cn-hangzhou') that assigns to a private property (e.g., $this->region) and replace the hardcoded 'cn-hangzhou' in the request payload with that property; ensure any existing instantiations of AlibabaCloud continue to work by keeping the default value and update references to the class constructor if tests or other code create the object with a custom region.
62-62: Hardcoded template parameter key limits flexibility.The
TemplateParamis hardcoded to use a'code'key, which assumes all SMS templates use this exact parameter name. Alibaba Cloud SMS templates can have arbitrary parameter names defined by the user. This also creates a mismatch with the test, which passes JSON as content that gets wrapped again.Consider allowing the message content to be the full
TemplateParamJSON directly:♻️ Proposed fix
- 'TemplateParam' => \json_encode(['code' => $message->getContent()]), + 'TemplateParam' => $message->getContent(),This would require the caller to pass properly formatted JSON for
TemplateParam, giving flexibility for any template structure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Utopia/Messaging/Adapter/SMS/AlibabaCloud.php` at line 62, The TemplateParam is currently hardcoded to ['code' => $message->getContent()] in AlibabaCloud.php which prevents arbitrary template keys and double-wraps JSON; change the call that sets 'TemplateParam' to accept the message content as the full TemplateParam JSON/object directly by using $message->getContent() (ensuring it is passed through as JSON if it's an array/object or left as-is if already a JSON string) so callers supply the exact template parameter structure expected by Alibaba Cloud; update any tests to pass a full JSON TemplateParam string or array to message->getContent() accordingly.
59-59: Useuniqidwith more entropy for better uniqueness.
uniqid()without themore_entropyparameter may generate duplicate nonces in high-concurrency scenarios, which could cause signature collisions and API request failures.♻️ Proposed fix
- 'SignatureNonce' => \uniqid(), + 'SignatureNonce' => \uniqid('', true),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Utopia/Messaging/Adapter/SMS/AlibabaCloud.php` at line 59, Replace the current use of \uniqid() for the 'SignatureNonce' value with a higher-entropy call to avoid collisions under concurrency; specifically, in AlibabaCloud.php where the request payload/headers are assembled (the entry that sets 'SignatureNonce' => \uniqid()), change it to use \uniqid('', true) so the nonce includes more entropy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Utopia/Messaging/Adapter/SMS/AlibabaCloud.php`:
- Around line 106-113: The percentEncode function uses preg_replace('/%7E/i',
'~', $res) which can return null on error but percentEncode is typed to return
string; update percentEncode to handle a null result from preg_replace (in
function percentEncode) by checking the return value and falling back to the
original $res or casting to string before returning so the method always returns
a string, ensuring any preg_replace failure is safely handled.
In `@tests/Messaging/Adapter/SMS/AlibabaCloudTest.php`:
- Around line 23-26: The test is passing a JSON string as SMS content which
causes double-encoding when the adapter wraps it again; update the test in
AlibabaCloudTest so SMS is constructed with content as an associative array
(e.g., ['code' => '123456']) or otherwise match the adapter’s expected
TemplateParam shape instead of passing json_encode(...), and adjust any
assertions that inspect the resulting TemplateParam payload to expect the
single-layer JSON (or raw array) produced by the adapter; update references to
the SMS constructor call (symbol: SMS) in the test to pass the direct array
content.
---
Nitpick comments:
In `@src/Utopia/Messaging/Adapter/SMS/AlibabaCloud.php`:
- Line 56: The RegionId is hardcoded; update the AlibabaCloud class to accept a
configurable region by adding a constructor parameter (e.g., $region =
'cn-hangzhou') that assigns to a private property (e.g., $this->region) and
replace the hardcoded 'cn-hangzhou' in the request payload with that property;
ensure any existing instantiations of AlibabaCloud continue to work by keeping
the default value and update references to the class constructor if tests or
other code create the object with a custom region.
- Line 62: The TemplateParam is currently hardcoded to ['code' =>
$message->getContent()] in AlibabaCloud.php which prevents arbitrary template
keys and double-wraps JSON; change the call that sets 'TemplateParam' to accept
the message content as the full TemplateParam JSON/object directly by using
$message->getContent() (ensuring it is passed through as JSON if it's an
array/object or left as-is if already a JSON string) so callers supply the exact
template parameter structure expected by Alibaba Cloud; update any tests to pass
a full JSON TemplateParam string or array to message->getContent() accordingly.
- Line 59: Replace the current use of \uniqid() for the 'SignatureNonce' value
with a higher-entropy call to avoid collisions under concurrency; specifically,
in AlibabaCloud.php where the request payload/headers are assembled (the entry
that sets 'SignatureNonce' => \uniqid()), change it to use \uniqid('', true) so
the nonce includes more entropy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1202d194-634e-41a4-b399-8d5ff5644271
📒 Files selected for processing (2)
src/Utopia/Messaging/Adapter/SMS/AlibabaCloud.phptests/Messaging/Adapter/SMS/AlibabaCloudTest.php
| private function percentEncode(string $str): string | ||
| { | ||
| $res = \urlencode($str); | ||
| $res = \str_replace(['+', '*'], ['%20', '%2A'], $res); | ||
| $res = \preg_replace('/%7E/i', '~', $res); | ||
|
|
||
| return $res; | ||
| } |
There was a problem hiding this comment.
preg_replace can return null on error.
If preg_replace encounters an error, it returns null, but the function is typed to return string. This could cause a type error.
🛡️ Proposed fix
private function percentEncode(string $str): string
{
$res = \urlencode($str);
$res = \str_replace(['+', '*'], ['%20', '%2A'], $res);
- $res = \preg_replace('/%7E/i', '~', $res);
+ $res = \preg_replace('/%7E/i', '~', $res) ?? $res;
return $res;
}📝 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.
| private function percentEncode(string $str): string | |
| { | |
| $res = \urlencode($str); | |
| $res = \str_replace(['+', '*'], ['%20', '%2A'], $res); | |
| $res = \preg_replace('/%7E/i', '~', $res); | |
| return $res; | |
| } | |
| private function percentEncode(string $str): string | |
| { | |
| $res = \urlencode($str); | |
| $res = \str_replace(['+', '*'], ['%20', '%2A'], $res); | |
| $res = \preg_replace('/%7E/i', '~', $res) ?? $res; | |
| return $res; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Utopia/Messaging/Adapter/SMS/AlibabaCloud.php` around lines 106 - 113,
The percentEncode function uses preg_replace('/%7E/i', '~', $res) which can
return null on error but percentEncode is typed to return string; update
percentEncode to handle a null result from preg_replace (in function
percentEncode) by checking the return value and falling back to the original
$res or casting to string before returning so the method always returns a
string, ensuring any preg_replace failure is safely handled.
| $message = new SMS( | ||
| to: [\getenv('ALIBABA_CLOUD_TO')], | ||
| content: \json_encode(['code' => '123456']), | ||
| ); |
There was a problem hiding this comment.
Message content format may cause double JSON encoding.
The test passes \json_encode(['code' => '123456']) as content, but the adapter wraps this in another JSON object with the 'code' key (line 62 of the adapter). This results in double encoding: {"code":"{\"code\":\"123456\"}"}.
If the adapter fix to pass content directly as TemplateParam is applied, update this test accordingly:
🐛 Proposed fix (if adapter is updated to use content directly)
$message = new SMS(
to: [\getenv('ALIBABA_CLOUD_TO')],
- content: \json_encode(['code' => '123456']),
+ content: '{"code":"123456"}',
);📝 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.
| $message = new SMS( | |
| to: [\getenv('ALIBABA_CLOUD_TO')], | |
| content: \json_encode(['code' => '123456']), | |
| ); | |
| $message = new SMS( | |
| to: [\getenv('ALIBABA_CLOUD_TO')], | |
| content: '{"code":"123456"}', | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Messaging/Adapter/SMS/AlibabaCloudTest.php` around lines 23 - 26, The
test is passing a JSON string as SMS content which causes double-encoding when
the adapter wraps it again; update the test in AlibabaCloudTest so SMS is
constructed with content as an associative array (e.g., ['code' => '123456']) or
otherwise match the adapter’s expected TemplateParam shape instead of passing
json_encode(...), and adjust any assertions that inspect the resulting
TemplateParam payload to expect the single-layer JSON (or raw array) produced by
the adapter; update references to the SMS constructor call (symbol: SMS) in the
test to pass the direct array content.
deepshekhardas
force-pushed
the
feat/6307-alibaba-cloud-adapter
branch
from
076e6de to
3dc0b48
Compare
|
Rebased onto latest main. Could you please review? 🙏 |
Greptile SummaryThis PR adds a new Alibaba Cloud SMS adapter following the existing pattern of other SMS adapters in the project, plus a corresponding integration test.
Confidence Score: 2/5The adapter has two functional defects that affect its core happy path and should be fixed before merging. The TemplateParam key is hardcoded to 'code', silently breaking all templates that use other variable names, and the test double-encodes the message content producing an incorrect API payload on every call. src/Utopia/Messaging/Adapter/SMS/AlibabaCloud.php and tests/Messaging/Adapter/SMS/AlibabaCloudTest.php both need changes before this is ready to merge. Important Files Changed
Reviews (1): Last reviewed commit: "feat: add Alibaba Cloud SMS adapter (#63..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| 'SignatureNonce' => \uniqid(), | ||
| 'SignatureVersion' => '1.0', | ||
| 'TemplateCode' => $this->templateCode, | ||
| 'TemplateParam' => \json_encode(['code' => $message->getContent()]), |
There was a problem hiding this comment.
'code'
The adapter always wraps the message content under the key 'code', meaning it only works with Alibaba Cloud SMS templates that define a ${code} placeholder variable. Any template using a different variable name (e.g., ${name}, ${product}, ${otp}) will receive an incorrect or empty substitution and the API will return an error. The TemplateParam JSON should either be passed through as the raw content (letting callers supply the full JSON structure) or the parameter key should be user-configurable via the constructor.
| $message = new SMS( | ||
| to: [\getenv('ALIBABA_CLOUD_TO')], | ||
| content: \json_encode(['code' => '123456']), | ||
| ); |
There was a problem hiding this comment.
TemplateParam
The test sets content to json_encode(['code' => '123456']) = '{"code":"123456"}'. Inside process(), that string is then wrapped again with json_encode(['code' => $message->getContent()]), producing TemplateParam = {"code":"{\"code\":\"123456\"}"}. The Alibaba Cloud API receives code = {"code":"123456"} (a JSON string) rather than 123456, which will either fail validation or inject the wrong value into the SMS template. The test content should be the plain OTP value — '123456' — not a pre-encoded JSON object.
| 'RegionId' => 'cn-hangzhou', | ||
| 'SignName' => $this->signName, | ||
| 'SignatureMethod' => 'HMAC-SHA1', | ||
| 'SignatureNonce' => \uniqid(), |
There was a problem hiding this comment.
uniqid() may produce collisions under concurrent load — it is based on the current microsecond timestamp. Under high request volumes, two simultaneous calls could generate identical nonces, which the Alibaba Cloud API may reject as replay attempts. Prefer bin2hex(random_bytes(16)) to generate a cryptographically random nonce.
| 'SignatureNonce' => \uniqid(), | |
| 'SignatureNonce' => \bin2hex(\random_bytes(16)), |
| 'Action' => 'SendSms', | ||
| 'Format' => 'JSON', | ||
| 'PhoneNumbers' => $to, | ||
| 'RegionId' => 'cn-hangzhou', |
There was a problem hiding this comment.
RegionId is hardcoded to 'cn-hangzhou'
The Alibaba Cloud SMS API supports multiple regions (e.g., ap-southeast-1 for Southeast Asia, eu-central-1 for Europe). Hardcoding cn-hangzhou means all traffic is routed through mainland China, which may incur higher latency and potential regulatory concerns for international deployments. Consider accepting regionId as an optional constructor parameter so callers can choose the appropriate datacenter.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Adds Alibaba Cloud SMS adapter.
Changes
Testing
Closes #6307