Conversation
📝 WalkthroughWalkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (1 passed, 1 warning, 1 inconclusive)❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)src/Appwrite/Event/Database.php (1)
⏰ 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). (19)
✨ Finishing touches
🧪 Generate unit tests
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: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/controllers/api/health.php(1 hunks)src/Appwrite/Event/Database.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/controllers/api/health.php (1)
src/Appwrite/Event/Event.php (2)
setQueue(95-100)getSize(648-652)
src/Appwrite/Event/Database.php (2)
src/Appwrite/Event/Event.php (2)
setProject(140-144)Event(10-653)src/Appwrite/Utopia/Response/Model/Document.php (1)
Document(8-112)
⏰ 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/Event/Database.php (1)
164-175: Preserve explicit queue when setProject() is calledsetProject() assigns $this->queue from the project's DSN and will overwrite a queue previously set via setQueue(); either (a) only assign $this->queue when empty/null, or (b) document that setProject takes precedence.
Location: src/Appwrite/Event/Database.php — setProject (lines 164–175).
Repo search for setQueue(...)->setProject(...) returned no matches; manual verification recommended.
app/controllers/api/health.php (1)
541-541: LGTM: queue selection by name before size checkChaining
setQueue($name)->getSize()correctly scopes the size to the requested database queue.
src/Appwrite/Event/Database.php
Outdated
| public function setProject(Document $project): Event | ||
| { | ||
| try { | ||
| $dsn = new DSN($this->getProject()->getAttribute('database')); | ||
| $dsn = new DSN($project->getAttribute('database')); | ||
| } catch (\InvalidArgumentException) { | ||
| // TODO: Temporary until all projects are using shared tables | ||
| $dsn = new DSN('mysql://' . $this->getProject()->getAttribute('database')); | ||
| $dsn = new DSN('mysql://' . $project->getAttribute('database')); | ||
| } | ||
|
|
||
| $this->queue = $dsn->getHost(); | ||
| return $this->queue; | ||
|
|
||
| return parent::setProject($project); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Preserve fluent API: return self, not Event
Other setters in this class return self; aligning setProject avoids breaking chaining/static analysis.
- public function setProject(Document $project): Event
+ public function setProject(Document $project): self
{
try {
$dsn = new DSN($project->getAttribute('database'));
} catch (\InvalidArgumentException) {
// TODO: Temporary until all projects are using shared tables
$dsn = new DSN('mysql://' . $project->getAttribute('database'));
}
- $this->queue = $dsn->getHost();
+ // Avoid clobbering a queue explicitly set earlier in a chain
+ if (empty($this->queue)) {
+ $this->queue = $dsn->getHost();
+ }
- return parent::setProject($project);
+ return parent::setProject($project);
}📝 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 setProject(Document $project): Event | |
| { | |
| try { | |
| $dsn = new DSN($this->getProject()->getAttribute('database')); | |
| $dsn = new DSN($project->getAttribute('database')); | |
| } catch (\InvalidArgumentException) { | |
| // TODO: Temporary until all projects are using shared tables | |
| $dsn = new DSN('mysql://' . $this->getProject()->getAttribute('database')); | |
| $dsn = new DSN('mysql://' . $project->getAttribute('database')); | |
| } | |
| $this->queue = $dsn->getHost(); | |
| return $this->queue; | |
| return parent::setProject($project); | |
| } | |
| public function setProject(Document $project): self | |
| { | |
| try { | |
| $dsn = new DSN($project->getAttribute('database')); | |
| } catch (\InvalidArgumentException) { | |
| // TODO: Temporary until all projects are using shared tables | |
| $dsn = new DSN('mysql://' . $project->getAttribute('database')); | |
| } | |
| // Avoid clobbering a queue explicitly set earlier in a chain | |
| if (empty($this->queue)) { | |
| $this->queue = $dsn->getHost(); | |
| } | |
| return parent::setProject($project); | |
| } |
🤖 Prompt for AI Agents
In src/Appwrite/Event/Database.php around lines 164-175, the method currently
declares a return type of Event and returns parent::setProject($project),
breaking the class' fluent API; change the signature to return self, call
parent::setProject($project) for side effects (without returning it), and then
return $this so callers can chain setters and static analysis sees the fluent
type.
✨ Benchmark results
⚡ Benchmark Comparison
|
0ee77e8 to
5c9dff5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/controllers/api/health.php (1)
535-541: Constrain the database queue name inputRight now any 256-char string passes. Add a lightweight prefix/charset check to avoid cross-queue introspection via arbitrary names.
Apply:
$threshold = \intval($threshold); + // Only allow database_* queues with a conservative charset. + if (!\preg_match('/^database_[A-Za-z0-9._-]+$/', $name)) { + throw new Exception(Exception::GENERAL_SERVER_ERROR, 'Invalid database queue name.'); + } - $size = $queueForDatabase->setQueue($name)->getSize(); + $size = (clone $queueForDatabase)->setQueue($name)->getSize();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/controllers/api/health.php(1 hunks)src/Appwrite/Event/Database.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Appwrite/Event/Database.php
🧰 Additional context used
🧬 Code graph analysis (1)
app/controllers/api/health.php (1)
src/Appwrite/Event/Event.php (2)
setQueue(95-100)getSize(648-652)
⏰ 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). (16)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: E2E General Test
- GitHub Check: Unit Test
- GitHub Check: Benchmark
- GitHub Check: scan
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Appwrite/Event/Database.php (1)
164-178: Fix return type to preserve fluent API and align with parent signatureReturn
self, callparent::setProject($project)for side effects, and thenreturn $this;. This keeps chaining and static analysis intact.- public function setProject(Document $project): Event + public function setProject(Document $project): self { - $database = $project->getAttribute('database'); - if (!empty($database) || $project->getId() === 'console') { - try { - $dsn = new DSN($database); - } catch (\InvalidArgumentException) { - // TODO: Temporary until all projects are using shared tables - $dsn = new DSN("mysql://$database"); - } - $this->queue = $dsn->getHost(); - } - - return parent::setProject($project); + // Update base state + parent::setProject($project); + + $database = (string) $project->getAttribute('database', ''); + $dsn = null; + + if ($database !== '') { + try { + $dsn = new DSN($database); + } catch (\InvalidArgumentException) { + try { + // TODO: Temporary until all projects are using shared tables + $dsn = new DSN('mysql://' . ltrim($database, '/')); + } catch (\InvalidArgumentException) { + // Leave $dsn null if still invalid + } + } + } + + if ($dsn) { + $host = (string) $dsn->getHost(); + if ($host !== '' && empty($this->queue)) { + $this->queue = $host; + } + } + + return $this; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/controllers/api/health.php(1 hunks)src/Appwrite/Event/Database.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/controllers/api/health.php
🧰 Additional context used
🧬 Code graph analysis (1)
src/Appwrite/Event/Database.php (2)
src/Appwrite/Event/Event.php (2)
setProject(140-144)Event(10-653)src/Appwrite/Utopia/Response/Model/Document.php (1)
Document(8-112)
🪛 GitHub Actions: Tests
src/Appwrite/Event/Database.php
[error] 169-169: Appwrite\Event\Database: DSN construction failed due to null DSN (environment misconfiguration).
| $database = $project->getAttribute('database'); | ||
| if (!empty($database) || $project->getId() === 'console') { | ||
| try { | ||
| $dsn = new DSN($database); | ||
| } catch (\InvalidArgumentException) { | ||
| // TODO: Temporary until all projects are using shared tables | ||
| $dsn = new DSN("mysql://$database"); | ||
| } | ||
| $this->queue = $dsn->getHost(); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Harden DSN handling to stop null/empty DSN crashes and avoid clobbering queue
Current logic attempts DSN construction even when $database is empty (due to || $project->getId() === 'console'), which can yield an invalid DSN or empty host and cause the reported pipeline failure. Guard against empty values, wrap the fallback in a second try/catch, and only set the queue when we have a non-empty host. Also, avoid overwriting a previously chosen queue.
To confirm no callers rely on the removed console special-case, search for code paths that set an empty/absent database attribute on the console project:
🏁 Script executed:
#!/bin/bash
# Inspect references to the console project and database attribute usage
rg -nC2 "getId\(\)\s*===\s*'console'|['\"]database['\"]" --type=phpLength of output: 174606
Harden DSN handling — prevent empty/console DSN from clobbering queue
Current code (src/Appwrite/Event/Database.php:166-175) will attempt to build a DSN when $database may be empty because of || $project->getId() === 'console', which can produce an empty host and overwrite $this->queue. Repo search shows many console-specific early-returns and DSN patterns (app/cli.php, app/worker.php, app/init/resources.php, app/init/registers.php), so this file is an outlier and must be hardened.
- Guard: don't construct a DSN when $database is empty; if the console project needs special handling, return/use the platform/console connection instead of passing empty values into DSN.
- Fallback: only attempt the
"mysql://$database"fallback when $database is non-empty; wrap the fallback in its own try/catch. - Assigning queue: set
$this->queue = $dsn->getHost()only when the host is non-empty and do not overwrite an existing non-empty $this->queue.
src/Appwrite/Event/Database.php:166-175 — apply fixes above.
🧰 Tools
🪛 GitHub Actions: Tests
[error] 169-169: Appwrite\Event\Database: DSN construction failed due to null DSN (environment misconfiguration).
🤖 Prompt for AI Agents
In src/Appwrite/Event/Database.php around lines 166-175, avoid building a DSN
when $database is empty or when this is the console project: first check that
$database is non-empty before attempting new DSN(), and if the project is
console handle it via the platform/console connection path instead of falling
through to DSN logic; only attempt the "mysql://$database" fallback when
$database is non-empty and wrap that fallback in its own try/catch; after
resolving a DSN only assign $this->queue = $dsn->getHost() if getHost() returns
a non-empty value and only overwrite $this->queue when it is currently empty to
prevent clobbering an existing queue value.
5c9dff5 to
06a9daf
Compare
06a9daf to
39a190f
Compare
No description provided.