fix: Use supported runtimes from env config #10759
Conversation
📝 WalkthroughWalkthroughThis pull request refactors how application function runtimes are configured and managed. It removes the dynamic Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–60 minutes This refactor involves interconnected changes across multiple configuration files with varying logic patterns. Key areas requiring extra attention:
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ 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 |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/config/frameworks.php (1)
203-203: Critical: Undefined function call will cause fatal error.Line 203 still calls
getVersions($templateRuntimes['NODE']['versions'], 'node'), but thegetVersionshelper function was removed in this refactor. This will cause a fatal error when the configuration is loaded.Apply this diff to fix the issue:
- 'runtimes' => getVersions($templateRuntimes['NODE']['versions'], 'node'), + 'runtimes' => $templateRuntimes['NODE'],
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/config/frameworks.php(14 hunks)app/config/template-runtimes.php(1 hunks)app/config/templates/function.php(41 hunks)app/init/configs.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (6)
app/init/configs.php (1)
7-8: LGTM: Early runtime configuration loading.Moving the runtime configuration loading to the top ensures that runtime data is available when other configuration files (like
template-runtimes.php,frameworks.php, andtemplates/function.php) need to reference it.app/config/template-runtimes.php (1)
5-12: LGTM: Dynamic runtime configuration.The refactoring from hard-coded runtime arrays to dynamically built mappings from
Config::getParam('runtimes')provides better flexibility. The structure—uppercase keys mapping to arrays of "key-version" strings—aligns with the consumption pattern inframeworks.phpandtemplates/function.php.app/config/frameworks.php (1)
17-17: LGTM: Consistent direct runtime references.All frameworks (except tanstack-start on Line 203) correctly use direct references to
$templateRuntimes['NODE']or$templateRuntimes['FLUTTER'], eliminating the need for the removedgetVersionshelper function.Also applies to: 43-43, 69-69, 94-94, 111-111, 136-136, 153-153, 178-178, 228-228, 253-253, 270-270, 286-286, 303-303, 319-319
app/config/templates/function.php (3)
9-21: LGTM: Refactored to use allow-list filtering.The function signature change from a deny-list to an allow-list approach is consistent with the PR objective. The filtering logic correctly uses
in_array($runtime, $allowList)to include only explicitly allowed runtimes.
37-2158: LGTM: Consistent allow-list propagation across all templates.All
getRuntimes()calls throughout the file have been consistently updated to pass the$allowListparameter. This ensures uniform runtime filtering across all function templates.
7-7: Confirm this is intentional—template runtimes behavior when env var is unset differs from other parts of the codebase.The review comment is correct. When
_APP_FUNCTIONS_RUNTIMESis empty or unset, the code in this file filters out all runtimes (sincein_array($runtime, [])always returns false), resulting in zero available runtimes for templates. This behavior is inconsistent with other parts of the codebase:
- Create.php (line 191): Uses
if (!empty($allowList))first, so validation is skipped when empty—allowing all runtimes- This file: Uses
array_filterwithin_arraydirectly—filters out all runtimes when emptyWhile the default in
app/config/variables.phpis'node-16.0,php-8.0,python-3.9,ruby-3.0', this inconsistency should be addressed to ensure predictable behavior and prevent silent failures in edge cases where the env var is unset.Recommend:
- Add
!empty($allowList)guard before applying the filter, or- Use the same ternary pattern as
runtimes-v2.phpfor consistency, or- Document this behavior explicitly
| use Utopia\System\System; | ||
|
|
||
| $templateRuntimes = Config::getParam('template-runtimes'); | ||
| $allowList = \array_filter(\explode(',', System::getEnv('_APP_FUNCTIONS_RUNTIMES', ''))); |
There was a problem hiding this comment.
Potential issue: Whitespace in environment variable will break filtering.
The explode(',', ...) call doesn't trim whitespace from the resulting array elements. If the _APP_FUNCTIONS_RUNTIMES environment variable contains spaces (e.g., "node-18.0, node-20.0"), the allow-list will include strings with leading spaces, causing the in_array() check on Line 19 to fail.
Apply this diff to trim whitespace:
-$allowList = \array_filter(\explode(',', System::getEnv('_APP_FUNCTIONS_RUNTIMES', '')));
+$allowList = \array_filter(\array_map('trim', \explode(',', System::getEnv('_APP_FUNCTIONS_RUNTIMES', ''))));📝 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.
| $allowList = \array_filter(\explode(',', System::getEnv('_APP_FUNCTIONS_RUNTIMES', ''))); | |
| $allowList = \array_filter(\array_map('trim', \explode(',', System::getEnv('_APP_FUNCTIONS_RUNTIMES', '')))); |
🤖 Prompt for AI Agents
In app/config/templates/function.php around line 7, the allow-list is built with
explode(',', ...) but elements are not trimmed so entries with surrounding
whitespace will fail in_array checks; after exploding, trim each element (e.g.,
map trim over the array) and then filter out empty strings so the resulting
$allowList contains clean runtime names without leading/trailing spaces.
✨ Benchmark results
⚡ Benchmark Comparison
|
Task: https://linear.app/appwrite/issue/SER-359/function-listtemplates-shows-a-lot-of-runtimes-many-of-which-arent