Conversation
📝 WalkthroughWalkthroughThe pull request removes the monolithic health controller Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
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: 8
🧹 Nitpick comments (6)
src/Appwrite/Platform/Modules/Health/Http/Health/Certificate/Get.php (1)
67-67: Optional: Suppress false positive for required but unused parameters.The static analysis tool flags
$errnoand$errstras unused. While they're required by thestream_socket_clientfunction signature, you could optionally suppress the warning with a comment if the static analysis noise is bothersome:// @phpmd-suppress-next-line UnusedLocalVariable - Required by stream_socket_client signature $sslSocket = stream_socket_client('ssl://' . $domain . ':443', $errno, $errstr, 30, STREAM_CLIENT_CONNECT, $sslContext);src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Failed/Get.php (2)
108-121: Add a default case to the match expression for defensive coding.The
matchexpression relies on theWhiteListvalidator to guarantee a valid name. If the validator is bypassed or changed, anUnhandledMatchErrorwould be thrown. Adding a default case provides a safer fallback with a clear error message.🔎 Proposed fix
$queue = match ($name) { System::getEnv('_APP_DATABASE_QUEUE_NAME', Event::DATABASE_QUEUE_NAME) => $queueForDatabase, System::getEnv('_APP_DELETE_QUEUE_NAME', Event::DELETE_QUEUE_NAME) => $queueForDeletes, System::getEnv('_APP_AUDITS_QUEUE_NAME', Event::AUDITS_QUEUE_NAME) => $queueForAudits, System::getEnv('_APP_MAILS_QUEUE_NAME', Event::MAILS_QUEUE_NAME) => $queueForMails, System::getEnv('_APP_FUNCTIONS_QUEUE_NAME', Event::FUNCTIONS_QUEUE_NAME) => $queueForFunctions, System::getEnv('_APP_STATS_RESOURCES_QUEUE_NAME', Event::STATS_RESOURCES_QUEUE_NAME) => $queueForStatsResources, System::getEnv('_APP_STATS_USAGE_QUEUE_NAME', Event::STATS_USAGE_QUEUE_NAME) => $queueForStatsUsage, System::getEnv('_APP_WEBHOOK_QUEUE_NAME', Event::WEBHOOK_QUEUE_NAME) => $queueForWebhooks, System::getEnv('_APP_CERTIFICATES_QUEUE_NAME', Event::CERTIFICATES_QUEUE_NAME) => $queueForCertificates, System::getEnv('_APP_BUILDS_QUEUE_NAME', Event::BUILDS_QUEUE_NAME) => $queueForBuilds, System::getEnv('_APP_MESSAGING_QUEUE_NAME', Event::MESSAGING_QUEUE_NAME) => $queueForMessaging, System::getEnv('_APP_MIGRATIONS_QUEUE_NAME', Event::MIGRATIONS_QUEUE_NAME) => $queueForMigrations, + default => throw new \Appwrite\Extend\Exception(\Appwrite\Extend\Exception::GENERAL_ARGUMENT_INVALID, "Unknown queue: {$name}"), };
58-71: Consider extracting queue name mappings to reduce duplication.The
System::getEnvcalls are duplicated in both theWhiteListvalidator (lines 58-71) and thematchexpression (lines 108-121). Extracting these to a shared constant or method would reduce maintenance burden when adding new queues.src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Messaging/Get.php (1)
15-59: Consider reducing duplication across queue health endpoints.This endpoint implementation is nearly identical to other queue health endpoints (Audits, Mails, Webhooks, etc.), differing only in the queue type and naming. While the current approach is functional and consistent, you could reduce duplication using a factory pattern or template method pattern to generate these endpoints.
src/Appwrite/Platform/Modules/Health/Http/Health/Stats/Get.php (1)
36-58: Add error handling for cache and storage operations.The action method lacks defensive error handling. If the cache is not registered or storage operations fail, this will result in an unhandled exception.
🔎 Proposed fix with error handling
public function action(Response $response, Registry $register, Device $deviceForFiles): void { - $cache = $register->get('cache'); - - $cacheStats = $cache->info(); + try { + $cache = $register->get('cache'); + $cacheStats = $cache->info(); + } catch (\Throwable) { + $cacheStats = []; + } $response->json([ 'storage' => [ 'used' => Storage::human($deviceForFiles->getDirectorySize($deviceForFiles->getRoot() . '/')), 'partitionTotal' => Storage::human($deviceForFiles->getPartitionTotalSpace()), 'partitionFree' => Storage::human($deviceForFiles->getPartitionFreeSpace()), ], 'cache' => [ 'uptime' => $cacheStats['uptime_in_seconds'] ?? 0, 'clients' => $cacheStats['connected_clients'] ?? 0, 'hits' => $cacheStats['keyspace_hits'] ?? 0, 'misses' => $cacheStats['keyspace_misses'] ?? 0, 'memory_used' => $cacheStats['used_memory'] ?? 0, 'memory_used_human' => $cacheStats['used_memory_human'] ?? 0, 'memory_used_peak' => $cacheStats['used_memory_peak'] ?? 0, 'memory_used_peak_human' => $cacheStats['used_memory_peak_human'] ?? 0, ], ]); }src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Builds/Get.php (1)
15-59: LGTM! Implementation follows the established pattern correctly.The endpoint implementation is clean and consistent:
- Proper dependency injection with matching types (Build service)
- Correct use of shared assertQueueThreshold validation
- Appropriate authentication controls (ADMIN, KEY)
The substantial code duplication across all queue health endpoints (Builds, Migrations, Logs, Audits, etc.) could potentially be reduced via a factory pattern or parameterized handler, but this modular approach provides clear separation and is consistent with the PR's refactoring objectives.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
app/config/services.phpapp/controllers/api/health.phpsrc/Appwrite/Platform/Appwrite.phpsrc/Appwrite/Platform/Modules/Health/Http/Health/AntiVirus/Get.phpsrc/Appwrite/Platform/Modules/Health/Http/Health/Cache/Get.phpsrc/Appwrite/Platform/Modules/Health/Http/Health/Certificate/Get.phpsrc/Appwrite/Platform/Modules/Health/Http/Health/DB/Get.phpsrc/Appwrite/Platform/Modules/Health/Http/Health/Get.phpsrc/Appwrite/Platform/Modules/Health/Http/Health/PubSub/Get.phpsrc/Appwrite/Platform/Modules/Health/Http/Health/Queue/Audits/Get.phpsrc/Appwrite/Platform/Modules/Health/Http/Health/Queue/Base.phpsrc/Appwrite/Platform/Modules/Health/Http/Health/Queue/Builds/Get.phpsrc/Appwrite/Platform/Modules/Health/Http/Health/Queue/Certificates/Get.phpsrc/Appwrite/Platform/Modules/Health/Http/Health/Queue/Databases/Get.phpsrc/Appwrite/Platform/Modules/Health/Http/Health/Queue/Deletes/Get.phpsrc/Appwrite/Platform/Modules/Health/Http/Health/Queue/Failed/Get.phpsrc/Appwrite/Platform/Modules/Health/Http/Health/Queue/Functions/Get.phpsrc/Appwrite/Platform/Modules/Health/Http/Health/Queue/Logs/Get.phpsrc/Appwrite/Platform/Modules/Health/Http/Health/Queue/Mails/Get.phpsrc/Appwrite/Platform/Modules/Health/Http/Health/Queue/Messaging/Get.phpsrc/Appwrite/Platform/Modules/Health/Http/Health/Queue/Migrations/Get.phpsrc/Appwrite/Platform/Modules/Health/Http/Health/Queue/StatsResources/Get.phpsrc/Appwrite/Platform/Modules/Health/Http/Health/Queue/StatsUsage/Get.phpsrc/Appwrite/Platform/Modules/Health/Http/Health/Queue/Webhooks/Get.phpsrc/Appwrite/Platform/Modules/Health/Http/Health/Stats/Get.phpsrc/Appwrite/Platform/Modules/Health/Http/Health/Storage/Get.phpsrc/Appwrite/Platform/Modules/Health/Http/Health/Storage/Local/Get.phpsrc/Appwrite/Platform/Modules/Health/Http/Health/Time/Get.phpsrc/Appwrite/Platform/Modules/Health/Http/Health/Version/Get.phpsrc/Appwrite/Platform/Modules/Health/Module.phpsrc/Appwrite/Platform/Modules/Health/Services/Http.phptests/e2e/Services/Health/HealthCustomServerTest.php
💤 Files with no reviewable changes (1)
- app/controllers/api/health.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-10T04:12:13.037Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Indexes/Delete.php:85-86
Timestamp: 2025-05-10T04:12:13.037Z
Learning: The class `Appwrite\Platform\Modules\Databases\Http\Databases\Collections\Indexes\Action` defines a method `getGrantParentNotFoundException()` which is used to handle cases where a parent resource (like a database collection) is not found.
Applied to files:
src/Appwrite/Platform/Modules/Health/Http/Health/DB/Get.php
🧬 Code graph analysis (24)
src/Appwrite/Platform/Appwrite.php (3)
src/Appwrite/Utopia/Response/Model/Platform.php (1)
Platform(8-100)src/Appwrite/Platform/Modules/Health/Module.php (1)
Module(8-14)src/Appwrite/Platform/Modules/Account/Module.php (1)
Module(8-14)
tests/e2e/Services/Health/HealthCustomServerTest.php (2)
tests/e2e/Client.php (1)
Client(8-342)src/Appwrite/Utopia/Request.php (1)
getHeaders(166-188)
src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Deletes/Get.php (2)
src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Base.php (2)
Base(9-20)assertQueueThreshold(13-19)src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Migrations/Get.php (2)
Get(15-60)action(50-59)
src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Webhooks/Get.php (2)
src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Base.php (2)
Base(9-20)assertQueueThreshold(13-19)src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Messaging/Get.php (3)
Get(15-60)getName(17-20)action(50-59)
src/Appwrite/Platform/Modules/Health/Http/Health/Stats/Get.php (1)
src/Appwrite/Platform/Modules/Health/Http/Health/Get.php (4)
Get(14-57)getName(18-21)__construct(23-47)action(49-56)
src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Audits/Get.php (2)
src/Appwrite/Event/Audit.php (1)
Audit(8-162)src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Base.php (1)
assertQueueThreshold(13-19)
src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Builds/Get.php (3)
src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Base.php (2)
Base(9-20)assertQueueThreshold(13-19)src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Logs/Get.php (4)
Get(15-60)getName(17-20)__construct(22-48)action(50-59)src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Migrations/Get.php (3)
Get(15-60)getName(17-20)action(50-59)
src/Appwrite/Platform/Modules/Health/Http/Health/Queue/StatsResources/Get.php (1)
src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Base.php (2)
Base(9-20)assertQueueThreshold(13-19)
src/Appwrite/Platform/Modules/Health/Services/Http.php (10)
src/Appwrite/Platform/Appwrite.php (2)
Appwrite(18-34)__construct(20-33)src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Audits/Get.php (3)
Get(15-60)__construct(22-48)getName(17-20)src/Appwrite/Platform/Modules/Health/Http/Health/AntiVirus/Get.php (3)
Get(17-78)__construct(26-50)getName(21-24)src/Appwrite/Platform/Modules/Health/Http/Health/Cache/Get.php (3)
Get(18-94)__construct(27-52)getName(22-25)src/Appwrite/Platform/Modules/Health/Http/Health/DB/Get.php (3)
Get(18-95)__construct(27-52)getName(22-25)src/Appwrite/Platform/Modules/Health/Http/Health/Get.php (3)
Get(14-57)__construct(23-47)getName(18-21)src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Certificates/Get.php (3)
Get(15-60)__construct(22-48)getName(17-20)src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Databases/Get.php (3)
Get(16-61)__construct(23-50)getName(18-21)src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Logs/Get.php (3)
Get(15-60)__construct(22-48)getName(17-20)src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Messaging/Get.php (2)
Get(15-60)getName(17-20)
src/Appwrite/Platform/Modules/Health/Http/Health/Cache/Get.php (1)
src/Appwrite/Platform/Modules/Health/Http/Health/Get.php (4)
Get(14-57)getName(18-21)__construct(23-47)action(49-56)
src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Databases/Get.php (2)
src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Base.php (2)
Base(9-20)assertQueueThreshold(13-19)src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Migrations/Get.php (3)
Get(15-60)getName(17-20)action(50-59)
src/Appwrite/Platform/Modules/Health/Http/Health/Storage/Local/Get.php (3)
src/Appwrite/Extend/Exception.php (1)
Exception(7-472)src/Appwrite/Platform/Modules/Health/Http/Health/Certificate/Get.php (4)
Get(20-92)getName(24-27)__construct(29-54)action(56-91)src/Appwrite/Platform/Modules/Health/Http/Health/Get.php (4)
Get(14-57)getName(18-21)__construct(23-47)action(49-56)
src/Appwrite/Platform/Modules/Health/Http/Health/DB/Get.php (2)
src/Appwrite/Platform/Modules/Health/Http/Health/Get.php (4)
Get(14-57)getName(18-21)__construct(23-47)action(49-56)src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Databases/Get.php (4)
Get(16-61)getName(18-21)__construct(23-50)action(52-60)
src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Messaging/Get.php (2)
src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Base.php (2)
Base(9-20)assertQueueThreshold(13-19)src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Mails/Get.php (2)
Get(15-60)action(50-59)
src/Appwrite/Platform/Modules/Health/Http/Health/Certificate/Get.php (3)
src/Appwrite/Extend/Exception.php (1)
Exception(7-472)src/Appwrite/Utopia/Response.php (2)
Response(159-983)dynamic(703-736)src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Certificates/Get.php (3)
Get(15-60)getName(17-20)action(50-59)
src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Migrations/Get.php (1)
src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Base.php (2)
Base(9-20)assertQueueThreshold(13-19)
src/Appwrite/Platform/Modules/Health/Http/Health/Get.php (8)
src/Appwrite/SDK/Method.php (1)
Method(9-327)src/Appwrite/Utopia/Response.php (2)
Response(159-983)dynamic(703-736)src/Appwrite/Platform/Modules/Health/Http/Health/AntiVirus/Get.php (3)
Get(17-78)getName(21-24)action(52-77)src/Appwrite/Platform/Modules/Health/Http/Health/Cache/Get.php (3)
Get(18-94)getName(22-25)action(54-93)src/Appwrite/Platform/Modules/Health/Http/Health/PubSub/Get.php (3)
Get(18-94)getName(22-25)action(54-93)src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Certificates/Get.php (3)
Get(15-60)getName(17-20)action(50-59)src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Databases/Get.php (3)
Get(16-61)getName(18-21)action(52-60)src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Logs/Get.php (3)
Get(15-60)getName(17-20)action(50-59)
src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Mails/Get.php (2)
src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Base.php (2)
Base(9-20)assertQueueThreshold(13-19)src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Migrations/Get.php (3)
Get(15-60)getName(17-20)action(50-59)
src/Appwrite/Platform/Modules/Health/Http/Health/Queue/StatsUsage/Get.php (2)
src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Base.php (2)
Base(9-20)assertQueueThreshold(13-19)src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Messaging/Get.php (3)
Get(15-60)getName(17-20)action(50-59)
src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Failed/Get.php (3)
src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Base.php (2)
Base(9-20)assertQueueThreshold(13-19)src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Logs/Get.php (4)
Get(15-60)getName(17-20)__construct(22-48)action(50-59)src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Migrations/Get.php (3)
Get(15-60)getName(17-20)action(50-59)
src/Appwrite/Platform/Modules/Health/Http/Health/Time/Get.php (5)
src/Appwrite/Platform/Modules/Health/Http/Health/AntiVirus/Get.php (4)
Get(17-78)getName(21-24)__construct(26-50)action(52-77)src/Appwrite/Platform/Modules/Health/Http/Health/DB/Get.php (4)
Get(18-95)getName(22-25)__construct(27-52)action(54-94)src/Appwrite/Platform/Modules/Health/Http/Health/Get.php (4)
Get(14-57)getName(18-21)__construct(23-47)action(49-56)src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Logs/Get.php (4)
Get(15-60)getName(17-20)__construct(22-48)action(50-59)src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Messaging/Get.php (3)
Get(15-60)getName(17-20)action(50-59)
src/Appwrite/Platform/Modules/Health/Module.php (2)
src/Appwrite/Platform/Modules/Health/Services/Http.php (2)
Http(32-66)__construct(34-65)src/Appwrite/Platform/Modules/Health/Http/Health/Get.php (1)
__construct(23-47)
src/Appwrite/Platform/Modules/Health/Http/Health/Storage/Get.php (2)
src/Appwrite/SDK/Method.php (1)
Method(9-327)src/Appwrite/Platform/Modules/Health/Http/Health/Get.php (4)
Get(14-57)getName(18-21)__construct(23-47)action(49-56)
src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Certificates/Get.php (3)
src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Base.php (2)
Base(9-20)assertQueueThreshold(13-19)src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Logs/Get.php (4)
Get(15-60)getName(17-20)__construct(22-48)action(50-59)src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Migrations/Get.php (3)
Get(15-60)getName(17-20)action(50-59)
🪛 PHPMD (2.15.0)
src/Appwrite/Platform/Modules/Health/Http/Health/Certificate/Get.php
67-67: Avoid unused local variables such as '$errno'. (undefined)
(UnusedLocalVariable)
67-67: Avoid unused local variables such as '$errstr'. (undefined)
(UnusedLocalVariable)
⏰ 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 (29)
src/Appwrite/Platform/Modules/Health/Http/Health/Time/Get.php (1)
15-48: LGTM!The class structure and SDK metadata configuration are consistent with other health endpoints in the module (e.g.,
Health/Get.php,AntiVirus/Get.php).src/Appwrite/Platform/Modules/Health/Http/Health/Certificate/Get.php (3)
1-27: LGTM!The namespace, imports, and class structure follow the established pattern for health module endpoints.
29-54: LGTM!The endpoint configuration properly validates the domain parameter and follows the SDK metadata conventions established in the health module.
56-60: LGTM!The URL-to-host normalization correctly handles both URL and domain inputs.
app/config/services.php (1)
107-107: LGTM!The configuration change aligns with the established pattern used by other module-based services (e.g.,
databases,functions,console).src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Base.php (1)
9-19: LGTM!The abstract base class cleanly encapsulates the queue threshold validation logic, promoting code reuse across all queue health endpoints and providing clear, informative error messages.
src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Deletes/Get.php (1)
15-59: LGTM!The implementation correctly follows the established pattern for queue health endpoints, properly leveraging the shared
Baseclass for threshold validation.src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Functions/Get.php (1)
15-59: LGTM!The implementation correctly follows the established queue health endpoint pattern, with proper threshold validation and response handling.
src/Appwrite/Platform/Modules/Health/Http/Health/DB/Get.php (1)
27-52: LGTM!The endpoint configuration follows the established pattern with appropriate SDK metadata, groups, labels, and dependency injection.
src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Mails/Get.php (1)
15-59: LGTM!The implementation correctly follows the established queue health endpoint pattern, properly extending
Base, using the sharedassertQueueThresholdmethod, and returning the standard health queue response model.src/Appwrite/Platform/Modules/Health/Http/Health/Queue/StatsResources/Get.php (1)
15-59: LGTM!The implementation follows the established queue health endpoint pattern consistently.
src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Webhooks/Get.php (1)
15-59: LGTM!The implementation follows the established queue health endpoint pattern consistently.
src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Certificates/Get.php (1)
15-59: LGTM!The implementation follows the established queue health endpoint pattern consistently.
src/Appwrite/Platform/Modules/Health/Http/Health/Get.php (1)
14-56: LGTM!The basic HTTP health endpoint correctly provides a simple liveness check. The hardcoded
ping: 0is appropriate since this endpoint confirms HTTP layer responsiveness rather than measuring actual latency.src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Failed/Get.php (1)
89-127: LGTM on the action logic.The action correctly resolves the queue by name, retrieves the failed job count with
getSize(failed: true), and properly invokesassertQueueThresholdwith thefailedflag set totruefor appropriate error messaging.src/Appwrite/Platform/Appwrite.php (1)
10-10: LGTM! Health module properly integrated.The Health module import and registration follows the established pattern used by other platform modules (Account, Databases, Functions, etc.). The module is correctly positioned in the initialization sequence.
Also applies to: 27-27
tests/e2e/Services/Health/HealthCustomServerTest.php (1)
373-397: LGTM! Test coverage follows established patterns.The
testAuditsSuccess()test properly validates both success (200 with size < 100) and failure (503 when threshold = 0) scenarios for the new audits queue health endpoint. The implementation is consistent with other queue health tests in this file.src/Appwrite/Platform/Modules/Health/Module.php (1)
8-13: LGTM! Clean module implementation.The Health module follows the standard pattern for platform modules, properly extending
Platform\Moduleand registering its HTTP service. The implementation is consistent with other modules likeAccount\Module.src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Audits/Get.php (2)
22-48: LGTM! New audits queue health endpoint properly implemented.The new
/v1/health/queue/auditsendpoint is well-structured with:
- Proper HTTP route configuration and SDK metadata
- Consistent scope and authentication requirements
- Appropriate response model (MODEL_HEALTH_QUEUE)
- Test coverage in
HealthCustomServerTest.phpThe implementation follows the established pattern for queue health endpoints and integrates cleanly with the Health module architecture.
44-44: Consider validating threshold as positive integer.Same observation as in
Messaging/Get.php: theInteger(true)validator allows negative threshold values. For consistency and logical correctness across all queue health endpoints, consider using a validator that enforces non-negative values.This applies to all queue health endpoints. Would you like me to verify the validator usage pattern across the entire Health module?
src/Appwrite/Platform/Modules/Health/Services/Http.php (1)
32-65: LGTM!The service wiring is well-organized, grouping related health endpoints logically (general health checks, queue health checks, and stats). The pattern of using
ClassName::getName()for action registration ensures consistency between action names and their implementations.src/Appwrite/Platform/Modules/Health/Http/Health/AntiVirus/Get.php (1)
52-77: LGTM!The action method correctly handles all states: disabled (via env var), pass (successful ping), and fail (failed ping). The error suppression with
@is acceptable here since the try-catch provides proper exception handling for network failures.src/Appwrite/Platform/Modules/Health/Http/Health/Queue/StatsUsage/Get.php (1)
15-59: LGTM!The implementation follows the established pattern for queue health endpoints, properly extending
Basefor threshold assertion and using the standard response model.src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Databases/Get.php (1)
16-60: LGTM!The implementation correctly extends the base queue pattern with an additional
nameparameter for selecting specific database queues. ThesetQueue($name)->getSize()chain properly configures the queue before retrieving its size.src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Logs/Get.php (1)
44-58: Verify: Logs endpoint uses Audits queue.This endpoint is named
getQueueLogsand serves/v1/health/queue/logs, but it injects and queriesqueueForAudits(the same queue used by the Audits endpoint). This may be intentional if logs are processed through the audit queue, or it could be a copy-paste error. Please verify the intended queue assignment by checking: (1) whether a separatequeueForLogsshould exist, (2) the architecture documentation for how different queue types are used, and (3) similar patterns in other Health endpoints.src/Appwrite/Platform/Modules/Health/Http/Health/Version/Get.php (1)
19-34: Missing SDK metadata label.Other health endpoints include an
sdklabel withMethodmetadata for SDK generation. This endpoint lacks that label, which may cause inconsistencies in generated SDK clients.#!/bin/bash # Check if other public health endpoints have SDK metadata rg -A 20 "label\('scope', 'public'\)" --type=php src/Appwrite/Platform/Modules/Health/src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Migrations/Get.php (1)
15-59: LGTM! Clean and consistent implementation.The migrations queue health endpoint is correctly implemented with:
- Proper service injection matching the Migration queue type
- Correct threshold validation using the inherited assertQueueThreshold method
- Consistent endpoint configuration and SDK metadata
src/Appwrite/Platform/Modules/Health/Http/Health/Storage/Local/Get.php (2)
25-49: LGTM!The constructor setup follows the established pattern for health endpoints. The SDK metadata, authentication requirements, and response model configuration are correctly defined.
55-72: LGTM!The storage volume iteration and read/write permission checks are correctly implemented. The exception messages clearly identify which volume failed and the type of failure.
| $response->dynamic(new Document([ | ||
| 'status' => 'pass', | ||
| 'ping' => \round((\microtime(true) - $checkStart) / 1000), | ||
| ]), Response::MODEL_HEALTH_STATUS); |
There was a problem hiding this comment.
Two issues: incorrect ping calculation and missing name field.
-
Ping calculation bug: Same issue as other endpoints—should multiply by 1000, not divide, to convert seconds to milliseconds.
-
Missing
namefield: TheMODEL_HEALTH_STATUSresponse inHealth/Get.phpincludes anamefield (e.g.,'name' => 'http'). For consistency, consider adding'name' => 'storage'here.
🔎 Proposed fix
$response->dynamic(new Document([
+ 'name' => 'storage',
'status' => 'pass',
- 'ping' => \round((\microtime(true) - $checkStart) / 1000),
+ 'ping' => \round((\microtime(true) - $checkStart) * 1000),
]), Response::MODEL_HEALTH_STATUS);📝 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.
| $response->dynamic(new Document([ | |
| 'status' => 'pass', | |
| 'ping' => \round((\microtime(true) - $checkStart) / 1000), | |
| ]), Response::MODEL_HEALTH_STATUS); | |
| $response->dynamic(new Document([ | |
| 'name' => 'storage', | |
| 'status' => 'pass', | |
| 'ping' => \round((\microtime(true) - $checkStart) * 1000), | |
| ]), Response::MODEL_HEALTH_STATUS); |
🤖 Prompt for AI Agents
In src/Appwrite/Platform/Modules/Health/Http/Health/Storage/Get.php around lines
77-80, the health response computes ping incorrectly and omits the required name
field: change the ping calculation to multiply the elapsed seconds by 1000
(e.g., round((microtime(true) - $checkStart) * 1000)) to produce milliseconds,
and add a 'name' => 'storage' entry in the Document array so the response
matches MODEL_HEALTH_STATUS.
✨ Benchmark results
⚡ Benchmark Comparison
|
…et.php Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Appwrite/Platform/Modules/Health/Http/Health/Certificate/Get.php (1)
93-94: Consider defensive access for timestamp fields.For consistency with the other certificate fields, you could add null coalescing to
validFrom_time_tandvalidTo_time_t. While these are typically always present in valid certificates, defensive coding would make the response construction uniform.🔎 Optional improvement
- 'validFrom' => $certificatePayload['validFrom_time_t'], - 'validTo' => $certificatePayload['validTo_time_t'], + 'validFrom' => $certificatePayload['validFrom_time_t'] ?? 0, + 'validTo' => $certificatePayload['validTo_time_t'] ?? 0,
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/Platform/Modules/Health/Http/Health/Certificate/Get.php
🧰 Additional context used
🪛 GitHub Actions: Linter
src/Appwrite/Platform/Modules/Health/Http/Health/Certificate/Get.php
[error] 1-1: PSR-12 lint error detected by Pint. 1 error across 949 files.
⏰ 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). (18)
- GitHub Check: E2E Service Test (Migrations)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (VCS)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (Tokens)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Teams)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: Unit Test
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: E2E General Test
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: scan
🔇 Additional comments (1)
src/Appwrite/Platform/Modules/Health/Http/Health/Certificate/Get.php (1)
56-96: Excellent work addressing all critical issues from the previous review!All the previously flagged concerns have been properly resolved:
- ✅ Socket resource leak fixed (line 76)
- ✅ Error handling added for certificate parsing (lines 78-80)
- ✅ Safe array access with null coalescing operators (lines 90-92, 95)
- ✅ Naming corrected from
subjectSNtosubjectCN(line 91)The implementation is now safe and production-ready.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/Appwrite/Platform/Modules/Health/Http/Health/Storage/Get.php (1)
77-80: Missingnamefield in health response (previously flagged).The response Document is missing the
namefield. According to the previous review, theMODEL_HEALTH_STATUSresponse in other health endpoints (e.g.,Health/Get.php) includes anamefield for consistency. This endpoint should include'name' => 'storage'to match the expected model structure.Note: The ping calculation has been correctly fixed to multiply by 1000.
🔎 Proposed fix
$response->dynamic(new Document([ + 'name' => 'storage', 'status' => 'pass', 'ping' => \round((\microtime(true) - $checkStart) * 1000), ]), Response::MODEL_HEALTH_STATUS);
🧹 Nitpick comments (1)
src/Appwrite/Platform/Modules/Health/Http/Health/Certificate/Get.php (1)
82-94: Consider adding defensive null coalescing for timestamp fields.While the certificate timestamp fields (
validFrom_time_tandvalidTo_time_t) are typically always present in successfully parsed certificates, the code uses defensive null coalescing (??) for other fields likename,subjectCN,issuerOrganisation, andsignatureTypeSN. For consistency with this defensive pattern, consider adding null coalescing for the timestamp fields as well.🔎 Suggested enhancement for consistency
- $sslExpiration = $certificatePayload['validTo_time_t']; + $sslExpiration = $certificatePayload['validTo_time_t'] ?? 0; $status = $sslExpiration < time() ? 'fail' : 'pass'; if ($status === 'fail') { throw new Exception(Exception::HEALTH_CERTIFICATE_EXPIRED); } $response->dynamic(new Document([ 'name' => $certificatePayload['name'] ?? '', 'subjectCN' => $certificatePayload['subject']['CN'] ?? '', 'issuerOrganisation' => $certificatePayload['issuer']['O'] ?? '', - 'validFrom' => $certificatePayload['validFrom_time_t'], - 'validTo' => $certificatePayload['validTo_time_t'], + 'validFrom' => $certificatePayload['validFrom_time_t'] ?? 0, + 'validTo' => $certificatePayload['validTo_time_t'] ?? 0, 'signatureTypeSN' => $certificatePayload['signatureTypeSN'] ?? '', ]), Response::MODEL_HEALTH_CERTIFICATE);Note: Using
0as the default would make the expiration check at line 83 correctly identify the certificate as expired/failed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Appwrite/Platform/Modules/Health/Http/Health/Cache/Get.phpsrc/Appwrite/Platform/Modules/Health/Http/Health/Certificate/Get.phpsrc/Appwrite/Platform/Modules/Health/Http/Health/DB/Get.phpsrc/Appwrite/Platform/Modules/Health/Http/Health/PubSub/Get.phpsrc/Appwrite/Platform/Modules/Health/Http/Health/Storage/Get.phpsrc/Appwrite/Platform/Modules/Health/Http/Health/Storage/Local/Get.php
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Appwrite/Platform/Modules/Health/Http/Health/DB/Get.php
- src/Appwrite/Platform/Modules/Health/Http/Health/Storage/Local/Get.php
- src/Appwrite/Platform/Modules/Health/Http/Health/Cache/Get.php
🧰 Additional context used
🧬 Code graph analysis (2)
src/Appwrite/Platform/Modules/Health/Http/Health/PubSub/Get.php (4)
src/Appwrite/Platform/Modules/Health/Http/Health/Cache/Get.php (4)
Get(18-94)getName(22-25)__construct(27-52)action(54-93)src/Appwrite/Platform/Modules/Health/Http/Health/DB/Get.php (4)
Get(18-95)getName(22-25)__construct(27-52)action(54-94)src/Appwrite/Platform/Modules/Health/Http/Health/Storage/Get.php (4)
Get(16-82)getName(20-23)__construct(25-53)action(55-81)src/Appwrite/Platform/Modules/Health/Http/Health/Get.php (4)
Get(14-57)getName(18-21)__construct(23-47)action(49-56)
src/Appwrite/Platform/Modules/Health/Http/Health/Certificate/Get.php (5)
src/Appwrite/Extend/Exception.php (1)
Exception(7-472)src/Appwrite/SDK/Method.php (1)
Method(9-327)src/Appwrite/Platform/Modules/Health/Http/Health/Get.php (4)
Get(14-57)getName(18-21)__construct(23-47)action(49-56)src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Migrations/Get.php (3)
Get(15-60)getName(17-20)action(50-59)src/Appwrite/GraphQL/Types/Mapper.php (1)
model(176-243)
🪛 PHPMD (2.15.0)
src/Appwrite/Platform/Modules/Health/Http/Health/Certificate/Get.php
67-67: Avoid unused local variables such as '$errno'. (undefined)
(UnusedLocalVariable)
67-67: Avoid unused local variables such as '$errstr'. (undefined)
(UnusedLocalVariable)
⏰ 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). (18)
- GitHub Check: E2E Service Test (VCS)
- GitHub Check: E2E Service Test (Migrations)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Tokens)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E General Test
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: Unit Test
- GitHub Check: scan
🔇 Additional comments (4)
src/Appwrite/Platform/Modules/Health/Http/Health/PubSub/Get.php (1)
54-93: Action method follows established patterns correctly.The implementation properly iterates configured Pub/Sub pools, measures ping latency, aggregates failures, and returns a health status list. The ping calculation (line 74) correctly multiplies by 1000 to convert seconds to milliseconds. Error handling is consistent with the Cache and DB health endpoints.
src/Appwrite/Platform/Modules/Health/Http/Health/Certificate/Get.php (3)
1-54: Well-structured endpoint configuration.The imports, class structure, and endpoint configuration follow the health module patterns correctly. The domain parameter validation using
MultiplewithAnyOfandPublicDomainvalidators provides robust input validation.
67-67: Static analysis false positive: $errno and $errstr are required parameters.The PHPMD warning about unused
$errnoand$errstrvariables is a false positive. These are required output parameters forstream_socket_client()even when not used afterward. The current implementation is correct.
72-80: Previous critical issues properly addressed.The resource leak and missing error handling flagged in prior reviews have been correctly fixed:
- Socket is now closed immediately after certificate extraction (line 76)
- Parse failure is validated and throws an appropriate exception (lines 78-80)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
tests/e2e/Services/Health/MigrationsQueueTest.php (1)
7-16: LGTM! Test follows established pattern.The test correctly validates the migrations queue health endpoint.
tests/e2e/Services/Health/LogsQueueTest.php (1)
7-16: LGTM! Test follows established pattern.The test correctly validates the logs queue health endpoint.
tests/e2e/Services/Health/AuditsQueueTest.php (1)
7-16: LGTM! Test follows established pattern.The test correctly validates the audits queue health endpoint with both success and threshold-failure scenarios.
🧹 Nitpick comments (11)
tests/e2e/Services/Health/PubSubTest.php (1)
7-15: Consider adding a non-empty check before accessing the first element.The test accesses
$response['body']['statuses'][0]without first verifying that the array is non-empty. If no Pub/Sub pools are configured, this will throw an index error rather than a clear assertion failure.🔎 Proposed improvement
public function testPubSubSuccess(): void { $response = $this->callGet('/health/pubsub'); $this->assertEquals(200, $response['headers']['status-code']); $this->assertIsArray($response['body']['statuses']); + $this->assertNotEmpty($response['body']['statuses']); $this->assertIsInt($response['body']['statuses'][0]['ping']); $this->assertLessThan(100, $response['body']['statuses'][0]['ping']); $this->assertEquals('pass', $response['body']['statuses'][0]['status']); }tests/e2e/Services/Health/StorageTest.php (1)
7-14: Consider extracting magic number and adding failure case test.The hard-coded threshold
100at line 13 is repeated across multiple health tests. Consider extracting it to a constant in HealthBase for maintainability. Additionally, similar toStatsUsageQueueTestandFunctionsQueueTest, consider adding a test case withthreshold=0to verify the endpoint returns HTTP 503 under failure conditions.💡 Example refactor
public function testStorage(): void { $response = $this->callGet('/health/storage'); $this->assertEquals(200, $response['headers']['status-code']); $this->assertEquals('pass', $response['body']['status']); $this->assertIsInt($response['body']['ping']); $this->assertLessThan(100, $response['body']['ping']); + + $failure = $this->callGet('/health/storage', ['threshold' => '0']); + $this->assertEquals(503, $failure['headers']['status-code']); }tests/e2e/Services/Health/AntiVirusTest.php (1)
7-14: Consider asserting version is non-empty.Line 13 verifies
versionis a string but doesn't assert it's non-empty, unlike thestatusfield check at line 11. For consistency and robustness, consider adding$this->assertNotEmpty($response['body']['version']);to ensure the version field contains meaningful data.tests/e2e/Services/Health/StatsUsageQueueTest.php (1)
7-16: Good test coverage with threshold failure case.The test appropriately validates both success and failure scenarios. However, the hard-coded threshold
100at line 12 is repeated across multiple health tests. Consider extracting this to a constant in HealthBase for better maintainability.tests/e2e/Services/Health/FunctionsQueueTest.php (1)
7-16: Good test coverage with threshold failure case.The test appropriately validates both success and failure scenarios. However, the hard-coded threshold
100at line 12 is repeated across multiple health tests. Consider extracting this to a constant in HealthBase for better maintainability.tests/e2e/Services/Health/DeletesQueueTest.php (1)
7-16: LGTM! Test correctly validates queue health behavior.The test properly checks both the normal path (200 with size constraints) and threshold-triggered failure (503). The pattern is clear and effective.
Note: This test shares an identical structure with AuditsQueueTest, LogsQueueTest, and MigrationsQueueTest. If similar queue health tests continue to be added, consider extracting a shared test method or using a data provider to reduce duplication.
Example: Parameterized test approach
/** * @dataProvider queueProvider */ public function testQueueHealth(string $queueName): void { $response = $this->callGet("/health/queue/{$queueName}"); $this->assertEquals(200, $response['headers']['status-code']); $this->assertIsInt($response['body']['size']); $this->assertLessThan(100, $response['body']['size']); $failure = $this->callGet("/health/queue/{$queueName}", ['threshold' => '0']); $this->assertEquals(503, $failure['headers']['status-code']); } public function queueProvider(): array { return [ ['deletes'], ['audits'], ['logs'], ['migrations'], ]; }tests/e2e/Services/Health/MessagingQueueTest.php (1)
5-16: Consider reducing duplication across queue tests.This test class is nearly identical to
CertificatesQueueTestandStatsResourcesQueueTest. While explicit repetition in test code can aid clarity, consider extracting a shared helper method or data provider if the pattern continues to expand.Example: parameterized test approach
/** * @dataProvider queueEndpointProvider */ public function testQueueHealth(string $endpoint): void { $response = $this->callGet($endpoint); $this->assertEquals(200, $response['headers']['status-code']); $this->assertIsInt($response['body']['size']); $this->assertLessThan(100, $response['body']['size']); $failure = $this->callGet($endpoint, ['threshold' => '0']); $this->assertEquals(503, $failure['headers']['status-code']); } public function queueEndpointProvider(): array { return [ ['/health/queue/messaging'], ['/health/queue/certificates'], ['/health/queue/stats-resources'], // add more as needed ]; }tests/e2e/Services/Health/TimeTest.php (1)
13-14: Optional:assertNotEmptyon integers may not be necessary.Lines 13-14 use
assertNotEmptyon integer fields. SinceremoteTimeandlocalTimeare already validated as integers (lines 11-12),assertNotEmptywill only fail if they're0, which might be a valid edge case (Unix epoch). Consider whether a zero value is truly invalid, or if the type check alone is sufficient.tests/e2e/Services/Health/CertificateTest.php (1)
16-16: Use a reserved domain for better test reliability.The domain
doesnotexist.comis not a reserved test domain and could potentially be registered in the future, breaking this test. Consider using a reserved domain likeinvalid.example.comortest.invalid(per RFC 6761) which are guaranteed never to resolve.🔎 Suggested change
- $this->assertCertificateFailure('doesnotexist.com', 404); + $this->assertCertificateFailure('test.invalid', 404);tests/e2e/Services/GraphQL/HealthTest.php (2)
54-55: Strengthen test validation for the new response structure.The assertions correctly validate that
statusesis an array andtotalis positive. Consider adding:
- Validation of individual status object structure (ping and status fields)
- Verification that
totalmatches the count of items instatuses🔎 Enhanced validation
$this->assertIsArray($dbHealth['statuses']); $this->assertGreaterThan(0, $dbHealth['total']); +$this->assertCount($dbHealth['total'], $dbHealth['statuses']); +foreach ($dbHealth['statuses'] as $status) { + $this->assertArrayHasKey('ping', $status); + $this->assertArrayHasKey('status', $status); +}
77-78: Strengthen test validation for the new response structure.The assertions correctly validate that
statusesis an array andtotalis positive. Consider adding:
- Validation of individual status object structure (ping and status fields)
- Verification that
totalmatches the count of items instatuses🔎 Enhanced validation
$this->assertIsArray($cacheHealth['statuses']); $this->assertGreaterThan(0, $cacheHealth['total']); +$this->assertCount($cacheHealth['total'], $cacheHealth['statuses']); +foreach ($cacheHealth['statuses'] as $status) { + $this->assertArrayHasKey('ping', $status); + $this->assertArrayHasKey('status', $status); +}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
src/Appwrite/Platform/Modules/Health/Http/Health/Cache/Get.phpsrc/Appwrite/Platform/Modules/Health/Http/Health/DB/Get.phpsrc/Appwrite/Platform/Modules/Health/Http/Health/PubSub/Get.phpsrc/Appwrite/Platform/Modules/Health/Http/Health/Storage/Get.phpsrc/Appwrite/Platform/Modules/Health/Http/Health/Time/Get.phptests/e2e/Services/GraphQL/Base.phptests/e2e/Services/GraphQL/HealthTest.phptests/e2e/Services/Health/AntiVirusTest.phptests/e2e/Services/Health/AuditsQueueTest.phptests/e2e/Services/Health/BuildsQueueTest.phptests/e2e/Services/Health/CacheTest.phptests/e2e/Services/Health/CertificateTest.phptests/e2e/Services/Health/CertificatesQueueTest.phptests/e2e/Services/Health/DBTest.phptests/e2e/Services/Health/DatabasesQueueTest.phptests/e2e/Services/Health/DeletesQueueTest.phptests/e2e/Services/Health/FunctionsQueueTest.phptests/e2e/Services/Health/HTTPTest.phptests/e2e/Services/Health/HealthBase.phptests/e2e/Services/Health/HealthCustomServerTest.phptests/e2e/Services/Health/LogsQueueTest.phptests/e2e/Services/Health/MailsQueueTest.phptests/e2e/Services/Health/MessagingQueueTest.phptests/e2e/Services/Health/MigrationsQueueTest.phptests/e2e/Services/Health/PubSubTest.phptests/e2e/Services/Health/StatsResourcesQueueTest.phptests/e2e/Services/Health/StatsUsageQueueTest.phptests/e2e/Services/Health/StorageLocalTest.phptests/e2e/Services/Health/StorageTest.phptests/e2e/Services/Health/TimeTest.phptests/e2e/Services/Health/WebhooksQueueTest.php
💤 Files with no reviewable changes (1)
- tests/e2e/Services/Health/HealthCustomServerTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Appwrite/Platform/Modules/Health/Http/Health/Time/Get.php
- src/Appwrite/Platform/Modules/Health/Http/Health/Storage/Get.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-10T04:12:13.037Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Indexes/Delete.php:85-86
Timestamp: 2025-05-10T04:12:13.037Z
Learning: The class `Appwrite\Platform\Modules\Databases\Http\Databases\Collections\Indexes\Action` defines a method `getGrantParentNotFoundException()` which is used to handle cases where a parent resource (like a database collection) is not found.
Applied to files:
src/Appwrite/Platform/Modules/Health/Http/Health/DB/Get.php
🧬 Code graph analysis (19)
tests/e2e/Services/Health/PubSubTest.php (1)
tests/e2e/Services/Health/HealthBase.php (2)
HealthBase(10-27)callGet(20-26)
tests/e2e/Services/Health/CertificatesQueueTest.php (1)
tests/e2e/Services/Health/HealthBase.php (2)
HealthBase(10-27)callGet(20-26)
tests/e2e/Services/Health/StorageLocalTest.php (1)
tests/e2e/Services/Health/HealthBase.php (2)
HealthBase(10-27)callGet(20-26)
tests/e2e/Services/Health/AuditsQueueTest.php (1)
tests/e2e/Services/Health/HealthBase.php (2)
HealthBase(10-27)callGet(20-26)
tests/e2e/Services/Health/CacheTest.php (1)
tests/e2e/Services/Health/HealthBase.php (2)
HealthBase(10-27)callGet(20-26)
tests/e2e/Services/Health/HealthBase.php (2)
tests/e2e/Client.php (1)
Client(8-342)tests/e2e/Scopes/Scope.php (1)
Scope(12-256)
tests/e2e/Services/Health/WebhooksQueueTest.php (1)
tests/e2e/Services/Health/HealthBase.php (2)
HealthBase(10-27)callGet(20-26)
tests/e2e/Services/Health/BuildsQueueTest.php (1)
tests/e2e/Services/Health/HealthBase.php (2)
HealthBase(10-27)callGet(20-26)
tests/e2e/Services/Health/DBTest.php (1)
tests/e2e/Services/Health/HealthBase.php (2)
HealthBase(10-27)callGet(20-26)
tests/e2e/Services/Health/DeletesQueueTest.php (1)
tests/e2e/Services/Health/HealthBase.php (2)
HealthBase(10-27)callGet(20-26)
tests/e2e/Services/Health/CertificateTest.php (1)
tests/e2e/Services/Health/HealthBase.php (2)
HealthBase(10-27)callGet(20-26)
tests/e2e/Services/Health/FunctionsQueueTest.php (1)
tests/e2e/Services/Health/HealthBase.php (2)
HealthBase(10-27)callGet(20-26)
tests/e2e/Services/Health/MailsQueueTest.php (1)
tests/e2e/Services/Health/HealthBase.php (2)
HealthBase(10-27)callGet(20-26)
tests/e2e/Services/Health/LogsQueueTest.php (1)
tests/e2e/Services/Health/HealthBase.php (2)
HealthBase(10-27)callGet(20-26)
tests/e2e/Services/Health/StorageTest.php (1)
tests/e2e/Services/Health/HealthBase.php (2)
HealthBase(10-27)callGet(20-26)
src/Appwrite/Platform/Modules/Health/Http/Health/Cache/Get.php (1)
src/Appwrite/Platform/Modules/Health/Http/Health/Get.php (4)
Get(14-57)getName(18-21)__construct(23-47)action(49-56)
tests/e2e/Services/Health/StatsResourcesQueueTest.php (1)
tests/e2e/Services/Health/HealthBase.php (2)
HealthBase(10-27)callGet(20-26)
tests/e2e/Services/Health/MessagingQueueTest.php (1)
tests/e2e/Services/Health/HealthBase.php (2)
HealthBase(10-27)callGet(20-26)
tests/e2e/Services/Health/StatsUsageQueueTest.php (1)
tests/e2e/Services/Health/HealthBase.php (2)
HealthBase(10-27)callGet(20-26)
⏰ 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). (18)
- GitHub Check: E2E Service Test (Teams)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Tokens)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: Unit Test
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: E2E General Test
- GitHub Check: Benchmark
- GitHub Check: scan
🔇 Additional comments (17)
tests/e2e/Services/Health/HTTPTest.php (1)
5-14: LGTM!Clean and concise test that validates the basic health endpoint. The assertions appropriately cover the expected response structure (status code, status value, ping type and threshold).
src/Appwrite/Platform/Modules/Health/Http/Health/PubSub/Get.php (1)
18-93: LGTM!The implementation correctly addresses the previous review concerns:
- Ping calculation now multiplies by 1000 (line 74) to convert seconds to milliseconds.
- SDK response model uses
MODEL_HEALTH_STATUS_LIST(line 44) matching the actual response structure.The error handling with try/catch and failure aggregation is well-structured.
src/Appwrite/Platform/Modules/Health/Http/Health/Cache/Get.php (1)
18-93: LGTM!The implementation correctly mirrors the PubSub endpoint pattern with the fixed ping calculation (line 74) and proper SDK response model declaration. The error handling and response structure are consistent with other health endpoints.
tests/e2e/Services/Health/HealthBase.php (1)
10-26: LGTM!Good refactoring to an abstract base class. The
callGet()helper provides a clean, DRY approach for health endpoint tests, properly merging authentication headers fromgetHeaders()with the required content-type and project headers.tests/e2e/Services/Health/MailsQueueTest.php (1)
5-16: LGTM!Solid test coverage with both the success path (normal queue check) and the failure path (threshold=0 forcing a 503). This validates the threshold-based health check mechanism effectively.
tests/e2e/Services/Health/BuildsQueueTest.php (1)
5-16: LGTM!Follows the established pattern for queue health tests with appropriate success and threshold-based failure assertions.
tests/e2e/Services/Health/StorageLocalTest.php (1)
7-14: LGTM! Clean health check test.The test correctly validates the storage health endpoint response structure and reasonable ping threshold. The 100ms threshold aligns with the PR's ping calculation fix (now properly multiplied by 1000 to return milliseconds).
tests/e2e/Services/Health/MessagingQueueTest.php (1)
7-16: LGTM! Test coverage is appropriate.The test correctly validates both the success path (200 status, integer size, size < 100) and the threshold-triggered failure path (503 when threshold=0). This aligns with the pattern used across other queue health tests in this PR.
tests/e2e/Services/Health/CertificatesQueueTest.php (1)
7-16: LGTM! Test validates endpoint correctly.The test appropriately verifies both success and threshold failure scenarios for the certificates queue health endpoint.
tests/e2e/Services/Health/TimeTest.php (1)
7-16: LGTM! Time endpoint test validates expected fields.The test appropriately verifies the time health endpoint returns the required fields (remoteTime, localTime as integers) and that the time difference is within an acceptable range.
tests/e2e/Services/Health/StatsResourcesQueueTest.php (1)
7-16: LGTM! Test validates stats-resources queue health endpoint.The test correctly verifies both the success case and the threshold-triggered failure scenario, consistent with other queue health tests in this PR.
tests/e2e/Services/Health/WebhooksQueueTest.php (1)
7-16: LGTM!The test follows the established pattern for health queue endpoints, with appropriate assertions for both success (200) and threshold-based failure (503) scenarios.
tests/e2e/Services/Health/DatabasesQueueTest.php (1)
7-16: LGTM!The test correctly validates the databases queue health endpoint with appropriate assertions. The hardcoded database name
database_db_mainappears to be a standard test fixture based on the broader health module pattern.src/Appwrite/Platform/Modules/Health/Http/Health/DB/Get.php (1)
69-76: Ping calculation correctly fixed.The ping time is now correctly calculated by multiplying the time difference by 1000 to convert seconds to milliseconds. This addresses the issue from the previous review.
tests/e2e/Services/Health/CertificateTest.php (1)
9-10: Consider the reliability implications of external domain dependencies.The test depends on external domains (www.google.com, appwrite.io) being accessible and maintaining valid certificates. While these are stable domains, any downtime or certificate changes could cause test failures unrelated to code changes.
For critical CI pipelines, consider whether these external dependencies are acceptable or if mock/stub approaches might be more reliable.
tests/e2e/Services/GraphQL/Base.php (2)
2439-2444: GraphQL Cache health endpoint structure is correct and properly aligned with API implementation.The
healthGetCachequery correctly returns a nested structure withstatusesarray andtotalcount. This design intentionally represents multi-pool health status checks, matching the DB health endpoint pattern. Since the GraphQL schema is auto-generated from Response models, the structure is automatically kept in sync with the REST API.No action required: the test is validating correct behavior. If this endpoint was recently added or modified, ensure API documentation reflects the structure and any SDKs consuming this endpoint support the nested response format.
2429-2434: GraphQL schema structure change verified for DB and Cache health endpoints.The DB and Cache health endpoint responses have been updated to use
MODEL_HEALTH_STATUS_LISTwith a nested structure{ statuses: [...], total: count }instead of flat fields. This is confirmed in the API implementation (src/Appwrite/Platform/Modules/Health/Http/Health/DB/Get.php and Cache/Get.php) and reflected in the updated GraphQL test queries. The test suite (HealthTest.php) validates the new structure correctly.However, the following items require external verification beyond the codebase:
- SDK client updates across all language SDKs (maintained in separate repositories)
- API documentation updates for the new response structure
- Consumer notifications about this breaking change
| } catch (\Throwable) { | ||
| $failures[] = $database; | ||
| } |
There was a problem hiding this comment.
Consider narrowing the exception scope and adding observability.
Catching \Throwable silently swallows all errors (including fatal errors) without logging, making production debugging difficult. Consider:
- Catching more specific exceptions (e.g., connection/timeout exceptions)
- Logging the exception details before adding to failures
- Preserving exception context in the failure message
🔎 Suggested improvements
- } catch (\Throwable) {
+ } catch (\Throwable $e) {
+ // Log exception details for debugging
+ \error_log("DB health check failed for {$database}: " . $e->getMessage());
$failures[] = $database;
}🤖 Prompt for AI Agents
In src/Appwrite/Platform/Modules/Health/Http/Health/DB/Get.php around lines
80-82, replace the broad catch (\Throwable) that silently appends $database to
$failures with targeted exception handling: catch the specific DB/connection
exceptions (e.g., \PDOException or the DB client’s connection/timeout
exceptions), call the app logger to log an error including the database
identifier and the exception (message and optionally trace), and append a
failure entry that preserves context (for example include database name and
$e->getMessage() or an array with ['database'=> $database, 'error'=>
$e->getMessage()]). Optionally add a final catch (\Throwable) only to log
unexpected fatal errors before rethrowing or converting them to a safe failure
entry.
| public function testCacheSuccess(): void | ||
| { | ||
| $response = $this->callGet('/health/cache'); | ||
| $this->assertEquals(200, $response['headers']['status-code']); | ||
| $this->assertIsArray($response['body']['statuses']); | ||
| $this->assertIsInt($response['body']['statuses'][0]['ping']); | ||
| $this->assertLessThan(100, $response['body']['statuses'][0]['ping']); | ||
| $this->assertEquals('pass', $response['body']['statuses'][0]['status']); | ||
| } |
There was a problem hiding this comment.
Add bounds check before accessing array index.
Lines 12-14 access $response['body']['statuses'][0] without first verifying the array is non-empty. If the statuses array is empty, this will cause an undefined index error. Add an assertion like $this->assertNotEmpty($response['body']['statuses']); after line 11.
🔎 Proposed fix
public function testCacheSuccess(): void
{
$response = $this->callGet('/health/cache');
$this->assertEquals(200, $response['headers']['status-code']);
$this->assertIsArray($response['body']['statuses']);
+ $this->assertNotEmpty($response['body']['statuses']);
$this->assertIsInt($response['body']['statuses'][0]['ping']);
$this->assertLessThan(100, $response['body']['statuses'][0]['ping']);
$this->assertEquals('pass', $response['body']['statuses'][0]['status']);
}📝 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.
| public function testCacheSuccess(): void | |
| { | |
| $response = $this->callGet('/health/cache'); | |
| $this->assertEquals(200, $response['headers']['status-code']); | |
| $this->assertIsArray($response['body']['statuses']); | |
| $this->assertIsInt($response['body']['statuses'][0]['ping']); | |
| $this->assertLessThan(100, $response['body']['statuses'][0]['ping']); | |
| $this->assertEquals('pass', $response['body']['statuses'][0]['status']); | |
| } | |
| public function testCacheSuccess(): void | |
| { | |
| $response = $this->callGet('/health/cache'); | |
| $this->assertEquals(200, $response['headers']['status-code']); | |
| $this->assertIsArray($response['body']['statuses']); | |
| $this->assertNotEmpty($response['body']['statuses']); | |
| $this->assertIsInt($response['body']['statuses'][0]['ping']); | |
| $this->assertLessThan(100, $response['body']['statuses'][0]['ping']); | |
| $this->assertEquals('pass', $response['body']['statuses'][0]['status']); | |
| } |
🤖 Prompt for AI Agents
In tests/e2e/Services/Health/CacheTest.php around lines 7 to 15, the test
indexes $response['body']['statuses'][0] without ensuring the 'statuses' array
is non-empty; add an assertion after the status-code check (i.e., after line 11)
such as asserting the statuses array is not empty (e.g.,
$this->assertNotEmpty($response['body']['statuses']);) so subsequent accesses to
[0] are safe, then keep the existing ping/type checks as-is.
| $this->assertIsArray($response['body']['statuses']); | ||
| $this->assertIsInt($response['body']['statuses'][0]['ping']); | ||
| $this->assertLessThan(100, $response['body']['statuses'][0]['ping']); | ||
| $this->assertEquals('pass', $response['body']['statuses'][0]['status']); |
There was a problem hiding this comment.
Add bounds check before accessing array index.
Line 11 verifies statuses is an array, but lines 12-14 access statuses[0] without checking the array is non-empty. If the statuses array happens to be empty, the test will fail with an undefined offset error rather than a meaningful assertion failure.
Suggested fix
$response = $this->callGet('/health/db');
$this->assertEquals(200, $response['headers']['status-code']);
$this->assertIsArray($response['body']['statuses']);
+$this->assertNotEmpty($response['body']['statuses']);
$this->assertIsInt($response['body']['statuses'][0]['ping']);
$this->assertLessThan(100, $response['body']['statuses'][0]['ping']);
$this->assertEquals('pass', $response['body']['statuses'][0]['status']);🤖 Prompt for AI Agents
In tests/e2e/Services/Health/DBTest.php around lines 11 to 14, the test asserts
that 'statuses' is an array but then indexes statuses[0] without confirming it's
non-empty; add a bounds check before accessing index 0 (e.g.,
assertNotEmpty($response['body']['statuses']) or assertGreaterThan(0,
count($response['body']['statuses']))), then proceed with the existing
assertions on statuses[0] so the test fails with a clear assertion message
instead of an undefined offset error.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/Appwrite/Platform/Modules/Health/Http/Health/Certificate/Get.php:
- Around line 72-74: The call to openssl_x509_parse on $peerCertificate can
return false and must be validated; after $certificatePayload =
openssl_x509_parse($peerCertificate) check whether $certificatePayload === false
and handle that error path (e.g., return an appropriate HTTP error/response or
throw an exception) instead of proceeding to read fields, and include a clear
log or error message referencing $peerCertificate and $certificatePayload so
callers know the certificate parsing failed.
- Around line 84-91: The code in the $response->dynamic(...) call constructs a
Document using unvalidated keys from $certificatePayload (e.g.,
$certificatePayload['subject']['CN'], ['issuer']['O'], ['validFrom_time_t'],
['validTo_time_t'], ['signatureTypeSN']) which can cause undefined index errors;
update the logic in Get.php (the block that calls $response->dynamic with
Document and Response::MODEL_HEALTH_CERTIFICATE) to validate the presence and
structure of these fields before building the Document: either check nested keys
exist (subject and subject['CN'], issuer and issuer['O'], etc.) and use safe
access (null coalescing/defaults) or throw a clear exception when required
certificate fields are missing, ensuring the response only contains validated
values.
- Around line 62-70: The code opens an SSL socket via stream_socket_client into
$sslSocket but never closes it, causing a resource leak; after successfully
creating and using $sslSocket (the variable from stream_socket_client) ensure
you call fclose($sslSocket) before returning/ending the request, and eliminate
the unused $errno and $errstr by either removing them from the
stream_socket_client call or including their values in the thrown Exception
(e.g., attach to Exception::HEALTH_INVALID_HOST) so they are actually used.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/Platform/Modules/Health/Http/Health/Certificate/Get.php
🧰 Additional context used
🧬 Code graph analysis (1)
src/Appwrite/Platform/Modules/Health/Http/Health/Certificate/Get.php (2)
src/Appwrite/Extend/Exception.php (1)
Exception(7-472)src/Appwrite/Platform/Modules/Health/Http/Health/Get.php (2)
Get(14-57)action(49-56)
🪛 PHPMD (2.15.0)
src/Appwrite/Platform/Modules/Health/Http/Health/Certificate/Get.php
67-67: Avoid unused local variables such as '$errno'. (undefined)
(UnusedLocalVariable)
67-67: Avoid unused local variables such as '$errstr'. (undefined)
(UnusedLocalVariable)
⏰ 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). (20)
- GitHub Check: Benchmark
- GitHub Check: E2E Service Test (Migrations)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Tokens)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (Abuse enabled)
- GitHub Check: E2E General Test
- GitHub Check: Unit Test
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: scan
🔇 Additional comments (3)
src/Appwrite/Platform/Modules/Health/Http/Health/Certificate/Get.php (3)
1-22: LGTM!The namespace, imports, and class declaration follow the standard pattern for health endpoints in the new modular architecture.
29-54: LGTM!The endpoint configuration follows the standard pattern. The validator chain on line 51 appropriately ensures the domain parameter is both a valid URL/domain format and a public domain.
58-60: LGTM!The URL-to-host normalization correctly handles cases where a full URL is provided instead of just a domain.
src/Appwrite/Platform/Modules/Health/Http/Health/Certificate/Get.php
Outdated
Show resolved
Hide resolved
src/Appwrite/Platform/Modules/Health/Http/Health/Certificate/Get.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Appwrite/Platform/Modules/Health/Http/Health/Certificate/Get.php (1)
96-101: Simplify redundant status variable.The
$statusvariable is assigned but only used for a single conditional check. You can simplify this logic by directly checking the expiration condition.♻️ Proposed simplification
- $sslExpiration = $validTo; - $status = $sslExpiration < time() ? 'fail' : 'pass'; - - if ($status === 'fail') { + if ($validTo < time()) { throw new Exception(Exception::HEALTH_CERTIFICATE_EXPIRED); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/Platform/Modules/Health/Http/Health/Certificate/Get.php
⏰ 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). (20)
- GitHub Check: E2E Service Test (Migrations)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Abuse enabled)
- GitHub Check: Unit Test
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: E2E General Test
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (2)
src/Appwrite/Platform/Modules/Health/Http/Health/Certificate/Get.php (2)
70-70: No resource cleanup issues found.The code correctly handles socket resource cleanup. When
stream_socket_client()fails,$sslSocketisfalse, and the exception is thrown before entering thetry-catch-finallyblock—no cleanup is needed since no resource was created. When the connection succeeds, the resource is properly closed in thefinallyblock at line 117, regardless of any exceptions thrown within thetryblock. This pattern safely prevents resource leaks.
62-68: Clarify the intended behavior for certificate verification in this health check endpoint.The SSL context relies on PHP's default certificate verification settings (
verify_peer=truesince PHP 5.6.0), which will fail for domains with self-signed or untrusted CA certificates. Since this endpoint is designed to retrieve certificate metadata ("Get the SSL certificate for a domain"), it needs to work with any domain regardless of certificate trust status.If this endpoint should successfully retrieve certificates from any domain, add
'verify_peer' => falseto the SSL context. If it should only work with properly trusted certificates, document this limitation clearly or configure a CA bundle for the PHP environment.
| ], | ||
| contentType: ContentType::JSON | ||
| )) | ||
| ->param('domain', null, new Multiple([new AnyOf([new URL(), new Domain()]), new PublicDomain()]), Multiple::TYPE_STRING, 'Domain name') |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find PublicDomain validator class definition
echo "=== Searching for PublicDomain validator class ==="
find . -type f -name "*.php" | xargs grep -l "class PublicDomain" | head -5
# Also try ripgrep for more flexible search
echo -e "\n=== Ripgrep search for PublicDomain ==="
rg -t php "class PublicDomain" -A 30
# Check for test files related to PublicDomain
echo -e "\n=== Test files for PublicDomain ==="
find . -type f -name "*Test*.php" -o -name "*test*.php" | xargs grep -l "PublicDomain" 2>/dev/null | head -5Repository: appwrite/appwrite
Length of output: 178
🌐 Web query:
Appwrite PublicDomain validator SSRF protection RFC1918
💡 Result:
Short answer: Appwrite had a known SSRF (CVE‑2023‑27159) in the avatars/favicon handler that affected releases up to v1.2.1; Appwrite fixed it in later releases by adding stricter input validation and blocking requests to local/private addresses (RFC1918/loopback/etc.). You should upgrade any vulnerable self‑hosted instances and/or follow Appwrite’s security advisories. [1][2][3]
Sources:
- CVE / vulnerability writeups for the SSRF (describes /v1/avatars/favicon issue and affected versions up to 1.2.1). [1][2]
- Appwrite GitHub / project pages (official repo and docs where fixes/patches and upgrade guidance are published). [3]
If you want, I can:
- look up the exact Appwrite release that fixed CVE‑2023‑27159 and link the GitHub commit/PR that added RFC1918/private‑IP checks, or
- check your deployed Appwrite version for known vulnerabilities and give upgrade steps.
🌐 Web query:
Appwrite "PublicDomain" validator implementation source code
💡 Result:
Do you mean the Appwrite "PublicDomain" validator in the main appwrite repo on GitHub? If so, which language/implementation (TypeScript, PHP, or another SDK) or do you want the exact file path / link to the source?
🌐 Web query:
site:github.com appwrite PublicDomain validator PHP
💡 Result:
I searched GitHub for "site:github.com appwrite PublicDomain validator PHP" and found no repository/file in the Appwrite org that matches "PublicDomain" or a "PublicDomain validator" in PHP. Relevant Appwrite repos I found (for context) include the main appwrite repo and the PHP SDK/generator repos: appwrite/appwrite [1], appwrite/sdk-generator [2], and appwrite/functions-starter (which includes PHP function templates) [3].
Sources:
[1] appwrite/appwrite — GitHub. (github.com)
[2] appwrite/sdk-generator — GitHub. (github.com)
[3] appwrite/functions-starter — GitHub. (github.com)
Would you like me to broaden the search (e.g., across all GitHub or other orgs), or search for a specific filename or validator type?
Citations:
- 1: https://github.com/appwrite/appwrite?utm_source=openai
- 2: https://github.com/appwrite/sdk-generator?utm_source=openai
- 3: https://github.com/appwrite/functions-starter?utm_source=openai
🌐 Web query:
Appwrite utopia-php validators PublicDomain source
💡 Result:
Do you want the source code link for the "PublicDomain" validator in the utopia-php validators package, or an explanation of what it does?
🌐 Web query:
utopia-php validators PublicDomain implementation
💡 Result:
Do you want the PublicDomain validator's source code, an explanation of how it works, or how to implement one yourself?
🌐 Web query:
site:github.com utopia-php PublicDomain validator
💡 Result:
I searched GitHub for "utopia-php PublicDomain validator". I didn’t find a repository named "PublicDomain validator" under the utopia-php org — the closest match is the utopia-php/domains package (a PHP domain parsing library that uses the public suffix dataset). (github.com)
Other Utopia PHP repositories (audit, framework, vcs, etc.) exist but none appear titled "PublicDomain validator." (github.com)
If you want, I can:
- search GitHub more broadly (not limited to utopia-php),
- look for forks or issues mentioning "PublicDomain" or "PublicDomain validator", or
- open the utopia-php/domains repo and show relevant files that perform public-suffix validation. Which would you prefer?
Citations:
- 1: https://github.com/utopia-php/domains?utm_source=openai
- 2: https://github.com/utopia-php/audit?utm_source=openai
🌐 Web query:
utopia-php/framework validators domain public SSRF
💡 Result:
Short answer: Utopia's codebase separates domain parsing (utopia-php/domains) from validators (utopia-php/validators). The domains library uses the Public Suffix List to parse and classify hostnames (registerable vs. private/test TLDs) — it does not perform network calls. That means Utopia's domain validators can help detect non-public or “local” hostnames, but they are not a full protection against SSRF on their own. (github.com)
Key points and recommendations (concise)
- What the libraries do: utopia-php/domains parses and classifies hostnames (TLD, suffix, registerable name, isTest/isPrivate/isICANN). It’s useful for rejecting “localhost”, “169.254.x.x”-style or private-suffix names at validation time. (github.com)
- What they don’t do: they don’t resolve DNS, make HTTP requests, or detect IP-encoded/bizarre URL encodings that can bypass server-side filters. DNS resolution, redirects, HTTP client behavior and URL-parsing quirks are where SSRF bypasses usually occur. Use dedicated SSRF mitigations for network requests. (github.com)
- Practical hardening:
- Extract host with parse_url(PHP_URL_HOST) then run Utopia\Domains\Domain to check isTest()/isPrivate()/getRegisterable(). Block if matches disallowed classes. (github.com)
- Never trust a validator alone to permit a downstream HTTP fetch. After validation, canonicalize the host (punycode, lowercase), then resolve and reject private/internal IPs (127., 10., 172.16-31., 169.254., ::1, fc00::/7). Treat DNS rebinding and redirects as threats. (github.com)
- Use an HTTP client that can block private networks (or enforce egress rules) and filter on resolved IP before contacting. Up-to-date clients/frameworks now offer “no private network” options—use them or implement server-side IP allowlist/denylist. (content.govdelivery.com)
- Defend against URL-encoding and parser quirks (embedded credentials, @, port tricks, octal/hex/decimal IP encodings). Test common SSRF bypasses and use robust URL parsing + canonicalization. (github.com)
If you want, I can:
- show a short PHP example that uses parse_url + Utopia Domains + IP resolution check, or
- scan a specific Utopia validator/parameter code path you care about and point out SSRF gaps. Which would you prefer?
Citations:
- 1: https://github.com/utopia-php/domains?utm_source=openai
- 2: https://github.com/utopia-php/domains?utm_source=openai
- 3: https://github.com/cujanovic/SSRF-Testing?utm_source=openai
- 4: https://github.com/utopia-php/domains?utm_source=openai
- 5: https://github.com/cujanovic/SSRF-Testing?utm_source=openai
- 6: https://content.govdelivery.com/accounts/USDHSCISA/bulletins/3c1736c?utm_source=openai
- 7: https://github.com/cujanovic/SSRF-Testing?utm_source=openai
🌐 Web query:
"PublicDomain" validator appwrite health certificate endpoint
💡 Result:
You can fetch a domain's TLS/SSL certificate from Appwrite using the Health Certificate endpoint:
- HTTP: GET /v1/health/certificate?domain=
- SDK example: health.getCertificate(domain = '').
The endpoint returns the certificate info for the given domain (use your project's X-Appwrite-Project and API key or appropriate SDK client config). See Appwrite Health API and TLS docs for details. [1][2]
I couldn't find any reference to a "PublicDomain" validator in the Appwrite docs or API references; Appwrite expects a public-facing domain (Let’s Encrypt/ACME) for automatic certificates. [2][1]
Sources:
[1] Appwrite Health API — Certificate endpoint.
[2] Appwrite TLS / TLS certificates & self-hosting documentation.
🌐 Web query:
Appwrite domain validation SSRF prevention private IP RFC1918
💡 Result:
Short answer
- Appwrite historically had an SSRF (CVE-2023-27159) affecting endpoints like /v1/avatars/favicon; upgrade to a patched release. [1][5]
What to do for domain / SSRF / private-IP (RFC1918) protection
- Upgrade Appwrite to the latest patched release (fixes known SSRF). [1][5]
- Use Appwrite Cloud / Enterprise WAF or add a WAF in front of self‑hosted instances to filter malicious requests. [3]
- Harden self‑hosted settings: enable recommended production security env vars, restrict console access (IP/email whitelists) and force HTTPS. [4][2]
- If your app accepts user-provided URLs/domains, validate like this:
- Resolve the hostname to IP(s) server‑side (do not rely on client DNS).
- Reject any resolved address in private ranges per RFC1918 (IPv4: 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16), loopback (127.0.0.0/8), link‑local (169.254.0.0/16), and equivalent IPv6 ranges (e.g., ::1, fc00::/7, fe80::/10). Perform CIDR checks.
- Also block other internal ranges (e.g., 100.64.0.0/10, 192.0.0.0/24 special blocks) and uncommon cases (e.g., DNS rebinding, large TTLs).
- Enforce timeouts, maximum redirects, and content‑type/size checks on outbound requests.
- Optionally route all user-requested outbound requests through a hardened proxy service that enforces the above rules (preferred for complex setups).
References
- CVE / SSRF advisory for Appwrite (upgrade recommended). [1]
- Appwrite security & self‑hosting production hardening docs (console whitelists, env vars, HTTPS). [2][4]
- Appwrite WAF / network protection (Enterprise). [3]
- Appwrite GitHub (project/releases source). [5]
If you want, I can:
- give a short code snippet (Node/Python) to resolve+CIDR‑check a hostname before requesting it, or
- check the exact Appwrite release that fixed CVE-2023-27159 for your current version (tell me your version).
SSRF vulnerability: PublicDomain validator alone provides insufficient protection.
The endpoint accepts user-supplied domains and initiates direct SSL connections via stream_socket_client without proper SSRF safeguards. The PublicDomain validator only performs domain classification using the Public Suffix List; it does not:
- Resolve DNS to validate the actual target IP
- Reject private IP ranges (RFC1918: 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16)
- Block loopback (127.0.0.0/8) or link-local (169.254.0.0/16) addresses
- Protect against numeric IP inputs (e.g.,
127.0.0.1,192.168.1.1) - Defend against DNS rebinding, decimal/octal/hex IP encoding, or URL parsing bypasses
To properly prevent SSRF, the code must:
- Perform server-side DNS resolution of the supplied domain
- Validate all resolved IPs against private/reserved ranges before connecting
- Canonicalize hostnames and reject direct IP inputs or validate them strictly
- Implement defenses against DNS rebinding (e.g., TTL limits, re-resolution checks) and URL encoding tricks
This mirrors the pattern of CVE-2023-27159 (Appwrite SSRF in /v1/avatars/favicon). Patch immediately before this endpoint reaches production.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @CONTRIBUTING.md:
- Line 261: The comment "Supporting libraries (each lib has one role, common
libs are released as" is incomplete—finish the sentence to clarify how common
libraries are published; e.g., update the line to read something like
"Supporting libraries (each lib has one role; common libs are released as
separate packages/shared libraries and published to the monorepo registry or
npm)". Ensure the wording makes the publication/distribution method explicit and
keeps the parenthetical structure used in the original line.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CONTRIBUTING.md
⏰ 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). (1)
- GitHub Check: scan
CONTRIBUTING.md
Outdated
| │ ├── sdk-console | ||
| │ ├── sdk-project | ||
| │ └── sdk-web | ||
| ├── src # Supporting libraries (each lib has one role, common libs are released as |
There was a problem hiding this comment.
Complete the comment.
The comment on this line appears incomplete: "Supporting libraries (each lib has one role, common libs are released as" ends without finishing the thought.
📝 Suggested completion
Consider completing the sentence, for example:
-├── src # Supporting libraries (each lib has one role, common libs are released as
+├── src # Supporting libraries (each lib has one role, common libs are released as standalone packages)Or if the intent was different, adjust accordingly.
📝 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.
| ├── src # Supporting libraries (each lib has one role, common libs are released as | |
| ├── src # Supporting libraries (each lib has one role, common libs are released as standalone packages) |
🤖 Prompt for AI Agents
In @CONTRIBUTING.md at line 261, The comment "Supporting libraries (each lib has
one role, common libs are released as" is incomplete—finish the sentence to
clarify how common libraries are published; e.g., update the line to read
something like "Supporting libraries (each lib has one role; common libs are
released as separate packages/shared libraries and published to the monorepo
registry or npm)". Ensure the wording makes the publication/distribution method
explicit and keeps the parenthetical structure used in the original line.
b6532b7 to
4905f7c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Failed/Get.php (2)
59-73: Consider extracting queue names to reduce duplication.The same
System::getEnv()calls are duplicated in thematchexpression (lines 112-126). If a queue is added or removed, both locations must be updated, risking inconsistencies.♻️ Suggested approach
Extract the queue name resolution to a static method or constant that can be reused:
private static function getQueueMap(): array { return [ System::getEnv('_APP_DATABASE_QUEUE_NAME', Event::DATABASE_QUEUE_NAME) => 'queueForDatabase', System::getEnv('_APP_DELETE_QUEUE_NAME', Event::DELETE_QUEUE_NAME) => 'queueForDeletes', // ... other queues ]; }Then use
array_keys(self::getQueueMap())for the WhiteList and the map itself for queue resolution.
112-126: Consider adding a default case for defensive programming.While the
WhiteListvalidator should prevent invalid names from reaching thismatch, adding a default case provides an additional safety net and clearer error messaging if the validation is ever bypassed or the two lists diverge.♻️ Suggested fix
System::getEnv('_APP_MESSAGING_QUEUE_NAME', Event::MESSAGING_QUEUE_NAME) => $queueForMessaging, System::getEnv('_APP_MIGRATIONS_QUEUE_NAME', Event::MIGRATIONS_QUEUE_NAME) => $queueForMigrations, + default => throw new \Appwrite\Extend\Exception(\Appwrite\Extend\Exception::GENERAL_ARGUMENT_INVALID, "Unknown queue name: {$name}"), };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/config/services.phpsrc/Appwrite/Platform/Appwrite.phpsrc/Appwrite/Platform/Modules/Health/Http/Health/Queue/Failed/Get.php
🧰 Additional context used
🧬 Code graph analysis (2)
src/Appwrite/Platform/Appwrite.php (1)
src/Appwrite/Platform/Modules/Health/Module.php (1)
Module(8-14)
src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Failed/Get.php (4)
src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Base.php (2)
Base(9-20)assertQueueThreshold(13-19)src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Builds/Get.php (4)
Get(15-60)getName(17-20)__construct(22-48)action(50-59)src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Databases/Get.php (4)
Get(16-61)getName(18-21)__construct(23-50)action(52-60)src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Logs/Get.php (3)
Get(15-60)getName(17-20)action(50-59)
⏰ 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: Linter
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (5)
src/Appwrite/Platform/Appwrite.php (1)
11-11: LGTM!The Health module import and registration follow the established pattern used by other modules in the platform. The integration is clean and consistent with existing code.
Also applies to: 29-29
app/config/services.php (1)
107-107: LGTM!The controller configuration change follows the established pattern for module-based services in the codebase. This correctly indicates that health routes are now handled by the Health module rather than the legacy
api/health.phpcontroller.src/Appwrite/Platform/Modules/Health/Http/Health/Queue/Failed/Get.php (3)
1-35: LGTM!Class structure and imports are consistent with the pattern established by other queue health endpoints.
92-110: LGTM!The action signature and threshold handling are consistent with other queue health endpoints in the codebase.
127-132: LGTM!The failed queue size retrieval, threshold assertion, and response handling are correct and consistent with the Base class contract and other queue health endpoints.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Appwrite/Platform/Modules/Health/Http/Health/DB/Get.php`:
- Around line 89-93: The code throws on any DB failure but the comment says to
only fail when all DBs fail; change the condition that triggers the Exception to
check for no successful pings (e.g. replace if (!empty($failures)) with a check
like if (empty($successes)) or if (count($failures) === count($databases)) so
the endpoint only throws when every database failed; if instead the current
behavior is intended, update the comment to reflect "fail on any DB failure"
rather than "only throw if ALL databases failed."
♻️ Duplicate comments (1)
src/Appwrite/Platform/Modules/Health/Http/Health/DB/Get.php (1)
83-85: Silent exception handling still present.The broad
catch (\Throwable)still silently swallows all errors without logging, making production debugging difficult.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/Platform/Modules/Health/Http/Health/DB/Get.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-10T04:12:13.037Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Indexes/Delete.php:85-86
Timestamp: 2025-05-10T04:12:13.037Z
Learning: The class `Appwrite\Platform\Modules\Databases\Http\Databases\Collections\Indexes\Action` defines a method `getGrantParentNotFoundException()` which is used to handle cases where a parent resource (like a database collection) is not found.
Applied to files:
src/Appwrite/Platform/Modules/Health/Http/Health/DB/Get.php
⏰ 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 (2)
src/Appwrite/Platform/Modules/Health/Http/Health/DB/Get.php (2)
28-54: LGTM!The constructor properly configures the HTTP endpoint with appropriate SDK metadata, auth types, and response model. The injection of
poolsandauthorizationdependencies follows the established pattern.
95-99: LGTM!The response correctly returns the health status list with the statuses array and computed total count, matching the
MODEL_HEALTH_STATUS_LISTdeclared in the SDK configuration.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // Only throw error if ALL databases failed (no successful pings) | ||
| // This allows partial failures in environments where not all DBs are ready | ||
| if (!empty($failures)) { | ||
| throw new Exception(Exception::GENERAL_SERVER_ERROR, 'DB failure on: ' . implode(", ", $failures)); | ||
| } |
There was a problem hiding this comment.
Logic contradicts the comment - throws on ANY failure, not ALL.
The comment states the endpoint should "only throw error if ALL databases failed" to "allow partial failures," but the condition !empty($failures) throws whenever ANY database fails. This means a single unhealthy DB will fail the entire health check even if others succeed.
If the intent matches the comment, the condition should check for no successful pings:
🔧 Proposed fix to match documented intent
// Only throw error if ALL databases failed (no successful pings)
// This allows partial failures in environments where not all DBs are ready
- if (!empty($failures)) {
- throw new Exception(Exception::GENERAL_SERVER_ERROR, 'DB failure on: ' . implode(", ", $failures));
+ if (empty($output)) {
+ throw new Exception(Exception::GENERAL_SERVER_ERROR, 'All DB checks failed on: ' . implode(", ", $failures));
}Alternatively, if the current behavior (fail on any failure) is intentional, update the comment to reflect that.
🤖 Prompt for AI Agents
In `@src/Appwrite/Platform/Modules/Health/Http/Health/DB/Get.php` around lines 89
- 93, The code throws on any DB failure but the comment says to only fail when
all DBs fail; change the condition that triggers the Exception to check for no
successful pings (e.g. replace if (!empty($failures)) with a check like if
(empty($successes)) or if (count($failures) === count($databases)) so the
endpoint only throws when every database failed; if instead the current behavior
is intended, update the comment to reflect "fail on any DB failure" rather than
"only throw if ALL databases failed."
What does this PR do?
'ping' => \round((\microtime(true) - $checkStart) / 1000)now'ping' => \round((\microtime(true) - $checkStart) * 1000)Test 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