refactor use env variables for queue and class names#11010
refactor use env variables for queue and class names#11010ChiragAgg5k merged 2 commits into1.8.xfrom
Conversation
|
Warning Rate limit exceeded@ChiragAgg5k has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 14 minutes and 22 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThis pull request migrates queue name and worker class name configuration from hardcoded constants in the Event class to environment-driven lookups. New global constants are introduced in Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Appwrite/Platform/Tasks/ScheduleFunctions.php (1)
44-44: Address the TODO:$lastEnqueueUpdateis never updated, breaking enqueue timing logic.The property
$lastEnqueueUpdateis read on line 44 to calculate$enqueueDiff, but it's never updated (line 116 is commented out). Since the property remainsnullforever,$enqueueDiffis always0, and the timeframe calculation on line 45 always uses the fullENQUEUE_TIMERvalue (60 seconds).The TODO comment indicates this was intentionally disabled because updating it breaks scheduling, but this leaves the enqueue timing logic in a broken state. The mention of "wasn't passed by reference" suggests a deeper architectural issue.
Please either:
- Fix the underlying architectural issue so
$lastEnqueueUpdatecan be properly updated, or- Remove the unused property and simplify the logic if dynamic timing is not needed
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/Platform/Tasks/ScheduleFunctions.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). (4)
- GitHub Check: scan-pr / osv-scan
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (1)
src/Appwrite/Platform/Tasks/ScheduleFunctions.php (1)
22-22: Remove unnecessary protected visibility or add subclass access justification.No subclasses of ScheduleFunctions were found in the codebase. The
$lastEnqueueUpdateproperty is only used within this class and never accessed from any subclass. If this visibility change is not needed for subclass access, consider reverting it toprivate.Additionally, note that the property update on line 116 remains commented out with a TODO stating it "breaks scheduling", leaving
$lastEnqueueUpdatepermanently null and making the enqueue diff calculation on line 44 always zero.Likely an incorrect or invalid review comment.
✨ Benchmark results
⚡ Benchmark Comparison
|
ChiragAgg5k
force-pushed
the
update-schedule-functions
branch
from
64b69a4 to
9ab9f6d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Appwrite/Platform/Tasks/ScheduleFunctions.php (1)
119-120: Address the TODO comment for lastEnqueueUpdate bug.The TODO indicates a known bug where updating
$lastEnqueueUpdatebreaks scheduling. While this PR doesn't address this issue, the technical debt should be tracked and investigated separately to ensure proper timing behavior.Would you like me to open a separate issue to track this bug? Understanding and resolving this timing issue would improve the reliability of the scheduling mechanism.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/Platform/Tasks/ScheduleFunctions.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-22T06:23:32.026Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 10348
File: app/controllers/shared/api.php:546-546
Timestamp: 2025-08-22T06:23:32.026Z
Learning: In the Utopia Queue system, the Pool class implements both Publisher and Consumer interfaces, so BrokerPool (Pool) can be passed directly to constructors expecting Publisher parameters like Func::__construct(protected Publisher $publisher).
Applied to files:
src/Appwrite/Platform/Tasks/ScheduleFunctions.php
🧬 Code graph analysis (1)
src/Appwrite/Platform/Tasks/ScheduleFunctions.php (1)
src/Appwrite/Event/Func.php (1)
Func(9-229)
⏰ 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). (4)
- GitHub Check: scan-pr / osv-scan
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Check if utopia-php/database changed
- GitHub Check: scan
🔇 Additional comments (3)
src/Appwrite/Platform/Tasks/ScheduleFunctions.php (3)
39-42: Excellent use of factory method pattern.The
getQueueForFunctions()method centralizes queue instantiation, making the code more maintainable and testable. The protected visibility enables subclasses to customize queue creation behavior if needed.
103-103: Inconsistency found: ScheduleExecutions.php should also use the factory method.The factory method usage in line 103 is correct. However, the codebase shows an inconsistency:
src/Appwrite/Platform/Tasks/ScheduleExecutions.php:39directly instantiatesFuncwith the same pattern instead of using a factory method like ScheduleFunctions.php does. For consistency across similar task classes, ScheduleExecutions.php should be refactored to use a corresponding factory method.
22-22: Remove the incorrect reasoning about $lastEnqueueUpdate visibility.The factory method pattern (
getQueueForFunctions()) is valid for extensibility, but the visibility change of$lastEnqueueUpdatefrom private to protected is not justified by this pattern. The property is only used internally at line 49 withinenqueueResources(), and there are no subclasses in the codebase that would benefit from accessing it. The visibility change appears speculative without evidence of actual subclass usage.
ChiragAgg5k
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/controllers/api/health.php (1)
948-959: Consider caching environment lookups to avoid duplication.The
System::getEnv()calls are duplicated in both theWhiteListvalidator (lines 948-959) and thematchexpression (lines 996-1007). While this works correctly, it introduces maintenance overhead and potential for drift if one is updated but not the other.🔎 Proposed refactor to reduce duplication
Consider extracting queue name resolution to local variables or a helper, for example at the start of the action closure:
$queueNames = [ 'database' => System::getEnv('_APP_DATABASE_QUEUE_NAME', APP_DATABASE_QUEUE_NAME), 'delete' => System::getEnv('_APP_DELETE_QUEUE_NAME', APP_DELETE_QUEUE_NAME), // ... other queues ]; $queue = match ($name) { $queueNames['database'] => $queueForDatabase, $queueNames['delete'] => $queueForDeletes, // ... };Also applies to: 996-1007
src/Appwrite/Event/Event.php (1)
9-9: Unused import.The
Systemclass is imported but doesn't appear to be used in this file. TheSystem::getEnv()calls are made in the subclasses, not in the baseEventclass.🔎 Proposed fix
-use Utopia\System\System;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
app/controllers/api/health.phpapp/init/constants.phpsrc/Appwrite/Event/Audit.phpsrc/Appwrite/Event/Build.phpsrc/Appwrite/Event/Certificate.phpsrc/Appwrite/Event/Database.phpsrc/Appwrite/Event/Delete.phpsrc/Appwrite/Event/Event.phpsrc/Appwrite/Event/Func.phpsrc/Appwrite/Event/Mail.phpsrc/Appwrite/Event/Messaging.phpsrc/Appwrite/Event/Migration.phpsrc/Appwrite/Event/StatsResources.phpsrc/Appwrite/Event/StatsUsage.phpsrc/Appwrite/Event/Webhook.php
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-22T06:23:32.026Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 10348
File: app/controllers/shared/api.php:546-546
Timestamp: 2025-08-22T06:23:32.026Z
Learning: In the Utopia Queue system, the Pool class implements both Publisher and Consumer interfaces, so BrokerPool (Pool) can be passed directly to constructors expecting Publisher parameters like Func::__construct(protected Publisher $publisher).
Applied to files:
src/Appwrite/Event/StatsResources.phpsrc/Appwrite/Event/Func.phpsrc/Appwrite/Event/Webhook.phpsrc/Appwrite/Event/Messaging.phpsrc/Appwrite/Event/Build.phpsrc/Appwrite/Event/Delete.phpsrc/Appwrite/Event/Mail.phpsrc/Appwrite/Event/StatsUsage.phpsrc/Appwrite/Event/Migration.phpsrc/Appwrite/Event/Certificate.phpsrc/Appwrite/Event/Event.php
📚 Learning: 2025-08-22T06:23:32.026Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 10348
File: app/controllers/shared/api.php:546-546
Timestamp: 2025-08-22T06:23:32.026Z
Learning: The Utopia\Queue\Broker\Pool class (aliased as BrokerPool) implements both Publisher and Consumer interfaces, making it directly compatible with constructors expecting Publisher parameters like Func::__construct(protected Publisher $publisher). No wrapping is needed.
Applied to files:
src/Appwrite/Event/StatsResources.phpsrc/Appwrite/Event/Func.phpsrc/Appwrite/Event/Webhook.phpsrc/Appwrite/Event/Messaging.phpsrc/Appwrite/Event/Build.phpsrc/Appwrite/Event/Delete.phpsrc/Appwrite/Event/StatsUsage.phpsrc/Appwrite/Event/Migration.phpsrc/Appwrite/Event/Certificate.php
📚 Learning: 2025-11-12T04:07:12.576Z
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10800
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Delete.php:153-154
Timestamp: 2025-11-12T04:07:12.576Z
Learning: In Appwrite's event queue system, `queueForRealtime`, `queueForFunctions`, and `queueForWebhooks` are copied from `queueForEvents`. Therefore, when resetting event queues in transaction staging paths, only `$queueForEvents->reset()` needs to be called in most cases. The other queues only require explicit reset when they are triggered in a loop.
Applied to files:
src/Appwrite/Event/Func.phpapp/controllers/api/health.php
🧬 Code graph analysis (12)
src/Appwrite/Event/Audit.php (1)
src/Appwrite/Event/Event.php (2)
setQueue(62-67)setClass(247-252)
src/Appwrite/Event/StatsResources.php (1)
src/Appwrite/Event/Event.php (2)
setQueue(62-67)setClass(247-252)
src/Appwrite/Event/Func.php (1)
src/Appwrite/Event/Event.php (2)
setQueue(62-67)setClass(247-252)
src/Appwrite/Event/Webhook.php (1)
src/Appwrite/Event/Event.php (2)
setQueue(62-67)setClass(247-252)
src/Appwrite/Event/Messaging.php (1)
src/Appwrite/Event/Event.php (2)
setQueue(62-67)setClass(247-252)
src/Appwrite/Event/Build.php (1)
src/Appwrite/Event/Event.php (2)
setQueue(62-67)setClass(247-252)
src/Appwrite/Event/Delete.php (1)
src/Appwrite/Event/Event.php (2)
setQueue(62-67)setClass(247-252)
src/Appwrite/Event/Mail.php (1)
src/Appwrite/Event/Event.php (2)
setQueue(62-67)setClass(247-252)
src/Appwrite/Event/StatsUsage.php (1)
src/Appwrite/Event/Event.php (2)
setQueue(62-67)setClass(247-252)
src/Appwrite/Event/Database.php (1)
src/Appwrite/Event/Event.php (1)
setClass(247-252)
src/Appwrite/Event/Migration.php (1)
src/Appwrite/Event/Event.php (2)
setQueue(62-67)setClass(247-252)
src/Appwrite/Event/Certificate.php (1)
src/Appwrite/Event/Event.php (2)
setQueue(62-67)setClass(247-252)
🔇 Additional comments (14)
app/controllers/api/health.php (1)
1009-1015: LGTM!The failed jobs handling logic is clear and the variable naming (
$failed) accurately reflects the data being tracked.app/init/constants.php (1)
378-404: LGTM!The new constants are well-organized, consistently named, and follow the established patterns. All 12 services have matching queue and class name constants.
src/Appwrite/Event/Mail.php (1)
23-30: LGTM!The environment-driven configuration is correctly implemented with proper fallback to constants, ensuring backward compatibility.
src/Appwrite/Event/Messaging.php (1)
18-25: LGTM!The environment-driven configuration follows the consistent pattern established across all event classes.
src/Appwrite/Event/Webhook.php (1)
10-17: LGTM!Consistent implementation of environment-driven configuration with proper fallbacks.
src/Appwrite/Event/Audit.php (1)
18-25: LGTM!The environment-driven configuration is correctly implemented, maintaining consistency with other event classes.
src/Appwrite/Event/Database.php (1)
24-29: LGTM!The Database event correctly only sets the class name via environment variable, as the queue name is dynamically determined from the project's database DSN in
setProject()(lines 165-179). This is the appropriate pattern for this event type.src/Appwrite/Event/Func.php (1)
8-8: LGTM!The environment-driven configuration is implemented consistently with the Delete event class.
Also applies to: 29-30
src/Appwrite/Event/Build.php (1)
8-8: LGTM!The changes follow the same environment-driven configuration pattern as other event classes.
Also applies to: 22-23
src/Appwrite/Event/Certificate.php (1)
7-7: LGTM!Environment-based configuration is correctly implemented with appropriate fallbacks.
Also applies to: 20-21
src/Appwrite/Event/StatsUsage.php (1)
7-7: LGTM!The configuration pattern is consistent with other event classes in this PR.
Also applies to: 22-23
src/Appwrite/Event/StatsResources.php (1)
6-6: LGTM!The changes are consistent and follow the established pattern for environment-driven configuration.
Also applies to: 17-18
src/Appwrite/Event/Migration.php (1)
8-8: LGTM!The migration event follows the same environment-driven pattern, completing the consistent refactor across all event classes.
Also applies to: 20-21
src/Appwrite/Event/Delete.php (1)
7-7: Environment-driven configuration with proper constant fallbacks.The
Systemimport and configuration in the constructor correctly implement environment-based settings with fallbacks. BothAPP_DELETE_QUEUE_NAMEandAPP_DELETE_CLASS_NAMEconstants are defined inapp/init/constants.php, and the pattern is consistent with other event classes in the codebase.
ChiragAgg5k
force-pushed
the
update-schedule-functions
branch
2 times, most recently
from
24f1ca2 to
43d48f1
Compare
ChiragAgg5k
force-pushed
the
update-schedule-functions
branch
from
43d48f1 to
bbeca28
Compare
3 participants
No description provided.