Cleanups #9511
Cleanups #9511
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe initialization process has been refactored to simplify and modularize the startup routine. The previously monolithic Changes
Sequence Diagram(s)sequenceDiagram
participant Init as init.php
participant Config as Config Loader
participant Const as Constants Loader
participant DB as Database Filters/Formats Loader
participant Locale as Locale Loader
participant Reg as Registers Loader
participant Res as Resources Loader
Init->>Config: require_once 'app/init/config.php'
Config-->>Init: Configuration loaded
Init->>Const: require_once 'app/init/constants.php'
Const-->>Init: Constants defined
Init->>DB: require_once 'app/init/database/filters.php'<br/>and 'app/init/database/formats.php'
DB-->>Init: DB filters and formats registered
Init->>Locale: require_once 'app/init/locale.php'
Locale-->>Init: Localization configured
Init->>Reg: require_once 'app/init/registers.php'
Reg-->>Init: Services registered
Init->>Res: require_once 'app/init/resources.php'
Res-->>Init: Resources registered
Poem
🪧 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 (
|
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: 2
🧹 Nitpick comments (24)
app/init/locale.php (2)
9-9: Consider enabling exceptions for debugging.By setting
Locale::$exceptionstofalse, the library might suppress valuable error information. Consider enabling exceptions, at least in non-production modes, to facilitate troubleshooting.
13-27: Add a safety fallback ifen.jsonis missing.While this logic rightly falls back to
en.jsonif localized files are unavailable, there is no handling ifen.jsonitself is missing or unreadable. Consider adding a final safety check or logging to handle that scenario cleanly.app/init/config.php (1)
5-36: Ensure config loading is robust.Multiple configuration files are loaded here without explicit error handling. If any file is absent or malformed, it may cause runtime issues. Consider verifying file existence or wrapping
Config::load()in try/catch blocks to log potential loading errors.app/init/database/filters.php (14)
10-22: Handle JSON decode errors in casting filter.The casting filter encodes/decodes to JSON. If decoding fails or returns null, it could cause subsequent type issues. Consider adding validations or default fallback values.
43-65: Double-check minimum/maximum handling.When removing and reassigning attributes like
minandmax, ensure that the data type is appropriate (integer, float, etc.) and that out-of-bound values are handled properly.
67-91: Watch out for performance overhead in sub-query (Attributes).Retrieving attributes in a loop can become expensive for large datasets. Consider indexing or limiting data to essential fields to keep performance in check.
108-120: Subquery performance consideration (Platforms).The query uses a fixed
Query::limit(APP_LIMIT_SUBQUERY). Confirm that this limit is suitable for all production scenarios. Overly large limits can degrade performance.
136-148: SubQueryWebhooks: validate usage under high load.Repeatedly fetching webhooks for many documents can be an intensive operation. Confirm that caching or pagination is in place to manage scale.
177-189: SubQueryChallenges: verify concurrency and usage.Challenges may be ephemeral. Confirm that pulling them in a standard listing process does not cause concurrency or performance issues with frequent updates.
205-217: SubQueryMemberships might be heavy for large user sets.Membership data can accumulate rapidly. Consider implementing pagination or lazy loading for high-volume usage.
234-258: Consider key rotation for encryption filter.Encryption uses a single environment key. Rotating keys periodically is a best practice. Provide a mechanism to decrypt older data with historical keys.
274-295: Enhance indexing for userSearch.Combining user ID, email, and phone into a single string is fine, but consider dedicated indexes or a more performant full-text search approach for large user bases.
297-309: Review subQueryTargets logic for partial updates.If partial user data changes independently, reevaluating target references might become inconsistent. Confirm that the logic handles concurrency or stale references.
311-331: SubQueryTopicTargets: check large subscriber sets.When a topic has many subscribers, collecting all target IDs could be large. Consider further paging or streaming results for performance.
333-350: providerSearch might benefit from robust search indexing.Relying on string concatenation can hinder performance. Evaluate using advanced indexing or search engine features for better scalability.
352-368: topicSearch could unify fields with indexing.Similar to other search filters, consider a more sophisticated search approach to handle partial matches, synonyms, or large text fields.
370-397: Validate message payload structure in messageSearch.We assume
subject,content, ortitleexist indata. If these fields are missing or malformed, it might cause warnings or incomplete searches.app/init/database/formats.php (1)
33-43: Ensure compatibility of-INFandINFusage.Some PHP environments may not define
INFor may handle negative infinity differently. Consider validating the environment or ensuring fallback values in caseINFis not recognized.app/init/registers.php (2)
43-95: Review deprecated vs. new DSN-based logging configuration.The fallback logic for older versions complicates maintainability. Consider a deprecation path or a clear removal plan for legacy configurations to streamline support for DSN-based setups.
271-334: Enforce error handling and resilience in service registrations.
- The database DSN might be invalid, leading to runtime errors.
- The geolocation file might be missing or outdated.
- The passwords dictionary could fail to load, returning false on
file_get_contents.Consider adding checks for these scenarios and providing fallback or error-handling strategies to enhance system stability.
app/init/resources.php (3)
63-99: Consolidate repetitive queue resource registrations for maintainability.You’re defining multiple queue-based event classes (Messaging, Mail, Build, etc.) in the same repetitive pattern. While this is functionally correct, consider reducing duplication by using a shared registration function or a loop-based approach to scale better as you add more queues.
695-711: Handle JSON file read errors more robustly.When reading files like
contributors.json,employees.json, orheroes.json, the code does not check for JSON parsing errors or invalid data types. Consider validating the returned data fromjson_decode()to prevent runtime issues.-$list = (file_exists($path)) ? json_decode(file_get_contents($path), true) : []; +$list = []; +if (file_exists($path)) { + $contents = file_get_contents($path); + if ($contents !== false) { + $decoded = json_decode($contents, true); + if (json_last_error() === JSON_ERROR_NONE) { + $list = (array) $decoded; + } + } +}
730-732: Clarify usage or remove placeholder plan resource.The
planresource always returns an empty array, providing no discernible functionality. If this is a placeholder for future development, clarify it with aTODOcomment or remove it if not needed.app/init/constants.php (1)
65-68: Consider adjusting database reconnect strategy.While retrying is a sensible approach, sleeping for 2 seconds up to 10 attempts might introduce a noticeable delay if the database remains unavailable. Consider defining a more dynamic backoff strategy or a maximum threshold to fail faster under critical workloads.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/init.php(1 hunks)app/init/config.php(1 hunks)app/init/constants.php(1 hunks)app/init/database/filters.php(1 hunks)app/init/database/formats.php(1 hunks)app/init/locale.php(1 hunks)app/init/registers.php(1 hunks)app/init/resources.php(1 hunks)composer.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (14)
composer.json (1)
99-102: Verify plugin permissions and security advisories.Allowing plugins automatically can introduce security risks if a malicious or compromised plugin is added. Verify these plugins’ trustworthiness and ensure they remain up-to-date.
app/init/database/filters.php (8)
24-41: ValidateformatOptionsfor enum filter.The filter removes or sets the
elementsattribute based onformatOptions. Confirm thatformatOptionsalways has valid JSON and gracefully handle unexpected or malformed data.
93-106: Confirm large index retrieval constraints.For the
subQueryIndexesfilter, ensure the limit set ingetLimitForIndexes()is tested not to omit critical indexes or cause performance bottlenecks in large-scale usage.
122-134: SubQueryKeys: verify security/permissions.Keys data can be sensitive. Ensure that this subquery is appropriate for the request context, preventing unauthorized key exposure.
150-161: Sessions subquery might expose sensitive data.Even though
Authorization::skip()is used here, confirm that referencing sessions in certain contexts won't unintentionally leak PII or compromise security.
163-175: SubQueryTokens: watch for exposure risk.Tokens can be highly sensitive. Ensure there's a valid reason for skipping authorization checks and returning them unfiltered from the database.
191-203: SubQueryAuthenticators: check security.Similar to other subqueries, skipping authorization can introduce vulnerabilities. Confirm that only privileged users can access these authenticator records.
219-232: SubQueryVariables: ensure type consistency.Variables tied to resource type
functionmight differ in structure. Double-check consistent key usage and data shape to prevent runtime errors.
260-272: SubQueryProjectVariables: confirm correct resourceType usage.The filter hardcodes
'resourceType', ['project']. Ensure no overlap with other resource types or missing logic for additional resource categories.app/init.php (1)
23-29:✅ Verification successful
Verify existence of newly required files.
These newly added
require_oncestatements can produce a fatal error if any file is missing or misnamed. Consider gracefully handling missing files or confirming their presence.
🏁 Script executed:
#!/bin/bash # Verify that all newly required files exist fd constants.php | xargs ls -l fd config.php | xargs ls -l fd filters.php | xargs ls -l fd formats.php | xargs ls -l fd locale.php | xargs ls -l fd registers.php | xargs ls -l fd resources.php | xargs ls -lLength of output: 913
All required files verified
The file verification confirms that all the newly required files exist at their expected locations:
app/init/constants.phpapp/init/config.phpapp/init/database/filters.phpapp/init/database/formats.phpapp/init/locale.php(note: while additional locale files exist in other directories, the one inapp/initis correctly required)app/init/registers.phpapp/init/resources.phpGiven these results, the risk of fatal errors due to missing or misnamed files is minimized. For robustness, consider implementing error handling if future refactoring could affect file paths.
app/init/registers.php (1)
97-269: Validate environment-based pooling parameters for edge cases.
workerCountcould exceed available connections, leading to exceptions.- A zero or negative value for
_APP_CONNECTIONS_MAXor_APP_POOL_CLIENTSmight cause division by zero.- Using persistent Redis connections (
pconnect) may warrant timeouts or reconnection logic.Consider adding additional checks and fallback logic to handle unexpected environment variable values more gracefully.
app/init/resources.php (1)
402-453: Verify potential concurrency impact of caching multiple Database instances.The
$databasesarray is kept in a closure and may be reused across subsequent calls. In certain request-serving environments, this might cause concurrency or state-sharing issues if multiple requests share a single runtime instance. Ensure each request operates in an isolated context, or implement synchronization where needed.Would you like a shell script to search for concurrency guards or references to
$databasesusage in other parts of the code?app/init/constants.php (2)
107-115: Expand mail type distinctions with constants or enumerations.You’ve done an excellent job enumerating mail types (verification, magicSession, etc.). If you plan to add new mail types or rename them, consider using a more formal Enum (when upgrading to PHP 8.1+) or a consistently documented collection for better type safety and maintainability.
61-63:✅ Verification successful
Confirm availability of the
Specification::S_05VCPU_512MBconstant.
APP_FUNCTION_SPECIFICATION_DEFAULTreferencesSpecification::S_05VCPU_512MB, which is not imported or showcased in the snippet. Verify that this constant is defined and available in the relevant namespace and that the corresponding file is properly loaded.Search for its declaration:
🏁 Script executed:
#!/bin/bash # Attempt to find the `S_05VCPU_512MB` reference in your codebase rg "S_05VCPU_512MB" -A 5 -B 5Length of output: 2341
Review Response: Constant Reference Verified
The constant
Specification::S_05VCPU_512MBis properly defined in thesrc/Appwrite/Functions/Specification.phpfile. Our search confirms that this constant exists and is correctly declared as a public constant in theSpecificationclass. Additionally, other parts of the code (e.g., inapp/config/runtimes/specifications.php) are correctly importing and referencing it. Assuming the autoloading setup handles namespacing appropriately, no further modifications are necessary inapp/init/constants.php.
✨ Benchmark results
⚡ Benchmark Comparison
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
app/init/constants.php (6)
7-8: Consider environment-specific configuration for email addressesThe default email addresses are hardcoded with placeholder values. For production environments, these values should be overridden by environment variables or configuration files to ensure proper setup across different environments.
-const APP_EMAIL_TEAM = 'team@localhost.test'; // Default email address -const APP_EMAIL_SECURITY = ''; // Default security email address +const APP_EMAIL_TEAM = getenv('APP_EMAIL_TEAM') ?: 'team@localhost.test'; // Default email address +const APP_EMAIL_SECURITY = getenv('APP_EMAIL_SECURITY') ?: ''; // Default security email address
18-20: Use standard PHP notation for file size constantsThe current implementation uses numeric values with comments to indicate the size in MB. Consider using more explicit calculations to make the byte values clear directly in the code.
-const APP_LIMIT_ANTIVIRUS = 20_000_000; //20MB -const APP_LIMIT_ENCRYPTION = 20_000_000; //20MB -const APP_LIMIT_COMPRESSION = 20_000_000; //20MB +const APP_LIMIT_ANTIVIRUS = 20 * 1024 * 1024; // 20MB +const APP_LIMIT_ENCRYPTION = 20 * 1024 * 1024; // 20MB +const APP_LIMIT_COMPRESSION = 20 * 1024 * 1024; // 20MB
50-50: Inconsistent buffer size calculationThe buffer size calculation uses a different format (multiplication with parentheses) than other size constants. Consider standardizing the format across all size-related constants.
-const APP_STORAGE_READ_BUFFER = 20 * (1000 * 1000); //20MB other names `APP_STORAGE_MEMORY_LIMIT`, `APP_STORAGE_MEMORY_BUFFER`, `APP_STORAGE_READ_LIMIT`, `APP_STORAGE_BUFFER_LIMIT` +const APP_STORAGE_READ_BUFFER = 20 * 1024 * 1024; //20MB other names `APP_STORAGE_MEMORY_LIMIT`, `APP_STORAGE_MEMORY_BUFFER`, `APP_STORAGE_READ_LIMIT`, `APP_STORAGE_BUFFER_LIMIT`
109-112: Duplicate comment headers for message typesThere are two sections with similar headers for "Message types" at lines 109 and 129. Consider consolidating these sections or using more specific headings to differentiate them.
// Message types const MESSAGE_SEND_TYPE_INTERNAL = 'internal'; const MESSAGE_SEND_TYPE_EXTERNAL = 'external'; ... -// Message types +// Message delivery types const MESSAGE_TYPE_EMAIL = 'email'; const MESSAGE_TYPE_SMS = 'sms'; const MESSAGE_TYPE_PUSH = 'push';Also applies to: 129-131
124-124: Standardize size constant formatThe MAX_OUTPUT_CHUNK_SIZE constant uses a different format for defining size than other similar constants. Consider standardizing to make the codebase more consistent.
-const MAX_OUTPUT_CHUNK_SIZE = 10 * 1024 * 1024; // 10MB +const MAX_OUTPUT_CHUNK_SIZE = 10 * 1024 * 1024; // 10MB
1-192: Consider further modularization of constantsThe file contains a large number of constants that could be further organized into separate files by domain (e.g., database constants, metrics constants, authentication constants). This would align with the PR's goal of making the codebase more maintainable.
Consider breaking this file into domain-specific constant files such as:
app/init/constants/app.php- Basic app constantsapp/init/constants/database.php- Database-related constantsapp/init/constants/metrics.php- All metrics constantsapp/init/constants/auth.php- Authentication constantsThis organization would make the constants easier to locate and maintain as the project grows.
🧰 Tools
🪛 Gitleaks (8.21.2)
58-58: Identified a Discord client ID, which may lead to unauthorized integrations and data exposure in Discord applications.
(discord-client-id)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/init/config.php(1 hunks)app/init/constants.php(1 hunks)app/init/locale.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/init/locale.php
- app/init/config.php
🧰 Additional context used
🪛 Gitleaks (8.21.2)
app/init/constants.php
58-58: Identified a Discord client ID, which may lead to unauthorized integrations and data exposure in Discord applications.
(discord-client-id)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Teams)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (VCS)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Databases)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: Benchmark
- GitHub Check: E2E General Test
- GitHub Check: Unit Test
- GitHub Check: scan
🔇 Additional comments (1)
app/init/constants.php (1)
58-58:❌ Incorrect review comment
Security concern: Discord channel ID exposure
The Discord channel ID is hardcoded and might be sensitive information that shouldn't be exposed in the source code. Consider moving this to a configuration file or environment variable.
🏁 Script executed:
#!/bin/bash # Description: Check if there are any other instances of discord channel IDs or similar tokens in the codebase echo "Checking for potential exposed tokens/IDs in the codebase..." rg -i --no-filename -e "discord.*[0-9]{18}" -e "[0-9]{18}.*discord" echo "Verifying if this particular Discord channel ID exists in other locations:" rg "564160730845151244" --no-filenameLength of output: 65820
Action: No changes required regarding hardcoded Discord channel ID
After verifying the repository:
- The search for any additional exposed Discord-related IDs returned only unrelated API documentation content, with no further instances of the specific channel ID.
- Discord channel IDs are public identifiers and do not grant special access—unlike tokens or secret keys—and therefore are not classified as sensitive information.
- While storing configuration values externally can improve overall maintainability, in this case moving the channel ID isn’t necessary from a security standpoint.
Likely an incorrect or invalid review comment.
🧰 Tools
🪛 Gitleaks (8.21.2)
58-58: Identified a Discord client ID, which may lead to unauthorized integrations and data exposure in Discord applications.
(discord-client-id)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/init/config.php (3)
5-36: Consider grouping related configurations and adding consistent comments.While the configuration loading is well-structured, consider organizing related configurations together (e.g., all locale-related configs in one section, storage configs in another). Also, some configuration loads have descriptive comments while others don't - adding consistent comments for all configuration files would improve code maintainability.
// Group related configurations with consistent comments + // Authentication and authorization Config::load('auth', __DIR__ . '/../config/auth.php'); // Authentication settings Config::load('oAuthProviders', __DIR__ . '/../config/oAuthProviders.php'); // OAuth provider configurations Config::load('roles', __DIR__ . '/../config/roles.php'); // User roles and scopes Config::load('scopes', __DIR__ . '/../config/scopes.php'); // User roles and scopes + // API and services Config::load('apis', __DIR__ . '/../config/apis.php'); // List of APIs Config::load('services', __DIR__ . '/../config/services.php'); // List of services Config::load('events', __DIR__ . '/../config/events.php'); // Application events // Additional groupings for other categories...
5-36: Add basic error handling for missing configuration files.The current implementation doesn't have error handling for missing configuration files. Consider adding basic checks to ensure critical configuration files exist before attempting to load them, especially for configurations that are essential for the application to function correctly.
// Example of adding basic error handling $configPath = __DIR__ . '/../config/apis.php'; if (!file_exists($configPath)) { throw new Exception('Critical configuration file not found: ' . $configPath); } Config::load('apis', $configPath); // List of APIs
1-4: Consider adding file documentation header.Adding a documentation header that explains the purpose of this file would enhance maintainability and provide context for developers who are new to the codebase.
<?php + /** + * Configuration Loader + * + * This file is responsible for loading all configuration files used throughout the application. + * It centralizes configuration loading as part of the application initialization process. + */ use Utopia\Config\Config;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/init/config.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (VCS)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Teams)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Databases)
- GitHub Check: Benchmark
- GitHub Check: Unit Test
- GitHub Check: E2E General Test
- GitHub Check: scan
🔇 Additional comments (1)
app/init/config.php (1)
5-36: Well-organized configuration loading structure.The modular approach to loading configuration files improves maintainability by centralizing all configuration loading in one place. This is a good architectural improvement that aligns with the PR objectives.
app/init/locale.php
Outdated
There was a problem hiding this comment.
all the files are plural. does it make sense to call this locales as well ?
composer.json
Outdated
| @@ -96,6 +96,10 @@ | |||
| "config": { | |||
| "platform": { | |||
| "php": "8.3" | |||
| }, | |||
| "allow-plugins": { | |||
There was a problem hiding this comment.
whats this needed for ?
There was a problem hiding this comment.
Composer is asking for it, was also in 1.6.x after sync
| @@ -1300,7 +1300,7 @@ public function testCreateIndexes(array $data): array | |||
| ]); | |||
|
|
|||
| $this->assertEquals(400, $unknown['headers']['status-code']); | |||
| $this->assertEquals('Unknown attribute: Unknown', $unknown['body']['message']); | |||
| $this->assertStringContainsString('Unknown attribute: Unknown', $unknown['body']['message']); | |||
There was a problem hiding this comment.
if we want to merge this, it would be better to have it fixed 😬
There was a problem hiding this comment.
It was already fixed in 1.6.x I just synced.
What does this PR do?
Restructured init for easier maintenance
Test Plan
Existing tests
Related PRs and Issues
Checklist
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chore