Shorten commit url and branch url #9919
Conversation
|
""" WalkthroughThe logic for generating domain names for VCS branch and commit previews in site deployments was updated. Branch names are truncated to 16 characters with a hash suffix if longer, and a hash of resource and project IDs is appended. Commit hashes are truncated to 16 characters. Domain formats and rule ID calculations were revised accordingly. Changes
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ 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:
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
Documentation and Community
|
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/controllers/api/vcs.php (1)
296-302: LGTM! Implementation correctly follows the PR requirements.The URL shortening logic is properly implemented according to the specifications:
- Truncates branch names to 16 characters
- Hashes excess characters and appends 7-character prefix
- Creates separate 7-character hash from resource/project IDs
- Constructs the final domain correctly
Consider adding validation to ensure branch names are domain-safe:
$branchPrefix = substr($providerBranch, 0, 16); +// Sanitize branch name for domain compatibility +$branchPrefix = preg_replace('/[^a-z0-9-]/', '-', strtolower($branchPrefix)); if (strlen($providerBranch) > 16) { $remainingChars = substr($providerBranch, 16); $branchPrefix .= substr(hash('sha256', $remainingChars), 0, 7); }Also consider documenting the 7-character hash length choice, as it provides ~268M combinations which should be sufficient but could theoretically lead to collisions in very large deployments.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
app/controllers/api/vcs.php(1 hunks)src/Appwrite/Platform/Modules/Functions/Workers/Builds.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (2)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (2)
1137-1139: The hash concatenation logic is correct.The conditional logic properly handles branch names longer than 16 characters by hashing the remainder and appending a 7-character suffix. This maintains deterministic behavior while keeping domain lengths manageable.
1135-1141:✅ Verification successful
Verify consistency with parallel changes in the codebase.
The AI summary mentions a parallel update in
app/controllers/api/vcs.phpfor VCS branch preview domain generation. Ensure both implementations use identical logic to prevent inconsistencies.
🏁 Script executed:
#!/bin/bash # Description: Search for domain generation logic in VCS-related files to ensure consistency # Expected: Find similar domain generation patterns that should match this implementation # Search for domain generation patterns in VCS files ast-grep --pattern 'branch-$_.$_' # Search for branch prefix generation patterns rg -A 10 -B 5 "branchPrefix|branch.*substr|substr.*branch" # Search for resourceProjectHash patterns rg -A 5 -B 5 "resourceProjectHash|resource.*project.*hash"Length of output: 7070
Branch preview domain logic is consistent across both modules
I’ve confirmed that both
src/Appwrite/Platform/Modules/Functions/Workers/Builds.phpandapp/controllers/api/vcs.phpuse identical substring and SHA-256 hashing steps forbranchPrefix,resourceProjectHash, and the final"branch-{$branchPrefix}{$resourceProjectHash}.{$sitesDomain}"domain format. No further changes are needed.
| $branchPrefix = substr($branchName, 0, 16); | ||
| if (strlen($branchName) > 16) { | ||
| $remainingChars = substr($branchName, 16); | ||
| $branchPrefix .= substr(hash('sha256', $remainingChars), 0, 7); | ||
| } | ||
| $resourceProjectHash = substr(hash('sha256', $resource->getId() . $project->getId()), 0, 7); | ||
| $domain = "branch-{$branchPrefix}{$resourceProjectHash}.{$sitesDomain}"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider domain name validation and character sanitization.
The new domain generation logic correctly implements the shortening mechanism, but there are several considerations:
- Domain character validation: Branch names may contain characters not valid in domain names (underscores, special characters, spaces, etc.)
- Collision risk: 7-character hashes have a small but non-zero collision probability
- Edge cases: No handling for empty branch names or very short names
Consider adding validation and sanitization:
$sitesDomain = System::getEnv('_APP_DOMAIN_SITES', '');
+// Sanitize branch name for domain usage
+$sanitizedBranchName = preg_replace('/[^a-z0-9-]/', '-', strtolower($branchName));
+$sanitizedBranchName = trim($sanitizedBranchName, '-');
+
+if (empty($sanitizedBranchName)) {
+ throw new \Exception('Invalid branch name for domain generation');
+}
+
-$branchPrefix = substr($branchName, 0, 16);
+$branchPrefix = substr($sanitizedBranchName, 0, 16);
if (strlen($branchName) > 16) {
$remainingChars = substr($branchName, 16);
$branchPrefix .= substr(hash('sha256', $remainingChars), 0, 7);
}📝 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.
| $branchPrefix = substr($branchName, 0, 16); | |
| if (strlen($branchName) > 16) { | |
| $remainingChars = substr($branchName, 16); | |
| $branchPrefix .= substr(hash('sha256', $remainingChars), 0, 7); | |
| } | |
| $resourceProjectHash = substr(hash('sha256', $resource->getId() . $project->getId()), 0, 7); | |
| $domain = "branch-{$branchPrefix}{$resourceProjectHash}.{$sitesDomain}"; | |
| // Fetch the sites domain | |
| $sitesDomain = System::getEnv('_APP_DOMAIN_SITES', ''); | |
| // Sanitize branch name for domain usage | |
| $sanitizedBranchName = preg_replace('/[^a-z0-9-]/', '-', strtolower($branchName)); | |
| $sanitizedBranchName = trim($sanitizedBranchName, '-'); | |
| if (empty($sanitizedBranchName)) { | |
| throw new \Exception('Invalid branch name for domain generation'); | |
| } | |
| // Build the branch prefix (truncate and hash remainder if too long) | |
| $branchPrefix = substr($sanitizedBranchName, 0, 16); | |
| if (strlen($branchName) > 16) { | |
| $remainingChars = substr($branchName, 16); | |
| $branchPrefix .= substr(hash('sha256', $remainingChars), 0, 7); | |
| } | |
| $resourceProjectHash = substr(hash('sha256', $resource->getId() . $project->getId()), 0, 7); | |
| $domain = "branch-{$branchPrefix}{$resourceProjectHash}.{$sitesDomain}"; |
🤖 Prompt for AI Agents
In src/Appwrite/Platform/Modules/Functions/Workers/Builds.php around lines 1135
to 1141, the domain name generation uses branch names directly without
sanitizing invalid domain characters or handling empty/short branch names. To
fix this, sanitize the branchName by removing or replacing characters not
allowed in domain names (e.g., spaces, underscores, special characters), add
validation to handle empty or very short branch names by providing a default or
fallback value, and consider increasing hash length or adding a collision check
to reduce collision risk. Implement these checks before constructing the final
domain string.
✨ Benchmark results
⚡ Benchmark Comparison
|
Co-authored-by: Matej Bačo <matejbacocom@gmail.com>
Co-authored-by: Matej Bačo <matejbacocom@gmail.com>
What does this PR do?
Shorten commit url by truncating commit hash to 16 chars
Shorten the branch url by following this rule:
resourceIdandprojectIdand take first 7 charsTest Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)
Related PRs and Issues
Checklist
Summary by CodeRabbit