Feat: execution.deploymentId response model #10357
Conversation
📝 WalkthroughWalkthroughAdds a new public string field Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
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
🧹 Nitpick comments (4)
src/Appwrite/Utopia/Response/Model/Execution.php (2)
41-47: Revise outdated TODO comment to match new behaviorThe added tests for Sites now assert and filter by deploymentId in listLogs, which contradicts this TODO. Please update or remove the comment to prevent confusion about Sites support for deploymentId.
Proposed tweak:
- // TODO: Sites listLogs will not have this, and will need siteId instead + // Note: Sites listLogs expose deploymentId as well. If siteId is needed in responses later, + // consider adding it as an optional/read-only field for parity instead of removing deploymentId.
48-53: Generalize description for broader applicability (functions and sites)Since this model is reused for Sites SSR logs too, make the description resource‑agnostic to reduce ambiguity in API docs.
- ->addRule('deploymentId', [ - 'type' => self::TYPE_STRING, - 'description' => 'Function\'s deployment ID used to create the execution.', - 'default' => '', - 'example' => '5e5ea5c16897e', - ]) + ->addRule('deploymentId', [ + 'type' => self::TYPE_STRING, + 'description' => 'Deployment ID used to create the execution.', + 'default' => '', + 'example' => '5e5ea5c16897e', + ])tests/e2e/Services/Sites/SitesCustomServerTest.php (2)
1890-1890: Prefer exact match over substring for deploymentIddeploymentId should be an exact ID. Using contains could let subtle mismatches slip through.
- $this->assertStringContainsString($deploymentId, $logs['body']['executions'][0]['deploymentId']); + $this->assertEquals($deploymentId, $logs['body']['executions'][0]['deploymentId']);
1927-1927: Apply strict equality for deploymentId in logs-action as wellMirror the earlier suggestion for consistency and stronger verification.
- $this->assertStringContainsString($deploymentId, $logs['body']['executions'][0]['deploymentId']); + $this->assertEquals($deploymentId, $logs['body']['executions'][0]['deploymentId']);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/Appwrite/Utopia/Response/Model/Execution.php(1 hunks)tests/e2e/Services/Functions/FunctionsCustomServerTest.php(2 hunks)tests/e2e/Services/Sites/SitesCustomServerTest.php(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/e2e/Services/Sites/SitesCustomServerTest.php (1)
tests/e2e/Services/Sites/SitesBase.php (1)
listLogs(227-237)
tests/e2e/Services/Functions/FunctionsCustomServerTest.php (1)
tests/e2e/Services/Functions/FunctionsBase.php (1)
listExecutions(200-208)
src/Appwrite/Utopia/Response/Model/Execution.php (1)
src/Appwrite/Utopia/Response/Model.php (1)
addRule(87-99)
🪛 GitHub Actions: Linter
tests/e2e/Services/Sites/SitesCustomServerTest.php
[error] 1-1: PSR-12 lint rule violation: no_whitespace_in_blank_lines. 672 files scanned; 1 style issue found in this file.
⏰ 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 (4)
tests/e2e/Services/Functions/FunctionsCustomServerTest.php (2)
948-971: Additions look good: asserting and filtering by deploymentId in listExecutions
- Verifying the returned deploymentId and adding equality/non-match filter checks strengthens coverage for the new field. Nice.
1060-1062: GetExecution: good addition to validate deploymentIdAsserting deploymentId here complements listExecutions coverage and catches mismatches per execution.
tests/e2e/Services/Sites/SitesCustomServerTest.php (2)
1900-1908: Filtering by deploymentId: looks solidThe positive filter path validates both total and item count; good alignment with the new response field.
1909-1916: Negative filter path: good guard against false positivesAsserting both total and collection size at zero is thorough.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/e2e/Services/Sites/SitesCustomServerTest.php (1)
1908-1908: Fix PSR-12: trailing whitespace on blank line (breaks linter)There is whitespace on this otherwise empty line. This triggers
no_whitespace_in_blank_lines. Please remove it. Note: the same applies to Line 1899 as flagged earlier.- +
🧹 Nitpick comments (3)
tests/e2e/Services/Sites/SitesCustomServerTest.php (3)
1890-1890: Use strict equality for deploymentId instead of substring checkdeploymentId is an exact identifier; using contains could mask mismatches (e.g., substring collisions).
- $this->assertStringContainsString($deploymentId, $logs['body']['executions'][0]['deploymentId']); + $this->assertSame($deploymentId, $logs['body']['executions'][0]['deploymentId']);
1900-1907: Also assert the filtered result’s deploymentId equals the filterYou already validate total/count. Add an explicit equality check to ensure the filter actually matched the expected execution.
$logs = $this->listLogs($siteId, [ Query::orderDesc('$createdAt')->toString(), Query::limit(1)->toString(), Query::equal('deploymentId', [$deploymentId])->toString() ]); $this->assertEquals(200, $logs['headers']['status-code']); $this->assertEquals(1, $logs['body']['total']); $this->assertCount(1, $logs['body']['executions']); + $this->assertSame($deploymentId, $logs['body']['executions'][0]['deploymentId']);
1927-1927: Use strict equality for deploymentId in action logs tooSame rationale as earlier; prefer exact match over substring.
- $this->assertStringContainsString($deploymentId, $logs['body']['executions'][0]['deploymentId']); + $this->assertSame($deploymentId, $logs['body']['executions'][0]['deploymentId']);Optional: mirror the positive/negative filter checks you added for inline logs to action logs for symmetry and future regressions. Example snippet to add right after this block:
$logs = $this->listLogs($siteId, [ Query::orderDesc('$createdAt')->toString(), Query::limit(1)->toString(), Query::equal('deploymentId', [$deploymentId])->toString(), ]); $this->assertEquals(200, $logs['headers']['status-code']); $this->assertEquals(1, $logs['body']['total']); $this->assertCount(1, $logs['body']['executions']); $this->assertSame($deploymentId, $logs['body']['executions'][0]['deploymentId']); $logs = $this->listLogs($siteId, [ Query::orderDesc('$createdAt')->toString(), Query::limit(1)->toString(), Query::equal('deploymentId', ['some-random-id'])->toString(), ]); $this->assertEquals(200, $logs['headers']['status-code']); $this->assertEquals(0, $logs['body']['total']); $this->assertCount(0, $logs['body']['executions']);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/e2e/Services/Sites/SitesCustomServerTest.php(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/e2e/Services/Sites/SitesCustomServerTest.php (1)
tests/e2e/Services/Sites/SitesBase.php (1)
listLogs(227-237)
⏰ 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 (Messaging)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Tokens)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (VCS)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Databases)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E General Test
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: Unit Test
- GitHub Check: Benchmark
🔇 Additional comments (1)
tests/e2e/Services/Sites/SitesCustomServerTest.php (1)
1909-1917: Negative filter case LGTMGood coverage for non-matching
deploymentIdfilter: asserting 0 total and empty executions.
✨ Benchmark results
⚡ Benchmark Comparison
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
tests/e2e/Services/Sites/SitesCustomServerTest.php (7)
1890-1890: Use strict equality for deploymentIdIDs should match exactly; using contains is too lenient and may pass on partial matches.
- $this->assertStringContainsString($deploymentId, $logs['body']['executions'][0]['deploymentId']); + $this->assertEquals($deploymentId, $logs['body']['executions'][0]['deploymentId']);
1900-1907: Great filter test; add an explicit equality checkSince you filter by deploymentId and limit(1), also assert the returned execution’s deploymentId equals the filter value. Consider wrapping this block in an eventually helper if flakiness is observed in CI.
$this->assertEquals(200, $logs['headers']['status-code']); $this->assertGreaterThanOrEqual(1, $logs['body']['total']); $this->assertCount(1, $logs['body']['executions']); + $this->assertEquals($deploymentId, $logs['body']['executions'][0]['deploymentId']);
1909-1917: Prefer a valid-looking ID for the negative filterUsing a generated ID avoids accidental coupling to ID format validation changes. Functionally the test remains the same.
- Query::equal('deploymentId', ['some-random-id'])->toString() + Query::equal('deploymentId', [ID::unique()])->toString()
1927-1927: Use strict equality for deploymentId in logs-actionMirror the inline-logs equality check for consistency and precision.
- $this->assertStringContainsString($deploymentId, $logs['body']['executions'][0]['deploymentId']); + $this->assertEquals($deploymentId, $logs['body']['executions'][0]['deploymentId']);
1965-1968: Remove duplicate assertionsThe same assertions for logs and errors are executed twice in a row; keep one each to reduce noise.
$this->assertEmpty($logs['body']['executions'][0]['logs']); - $this->assertEmpty($logs['body']['executions'][0]['logs']); $this->assertEmpty($logs['body']['executions'][0]['errors']); - $this->assertEmpty($logs['body']['executions'][0]['errors']);
1885-1898: Optional: Guard against empty executions before indexingWhile prior calls should have created executions, add a quick guard for clearer failures if something flakes.
- $this->assertEquals(200, $logs['headers']['status-code']); + $this->assertEquals(200, $logs['headers']['status-code']); + $this->assertGreaterThan(0, $logs['body']['total']); + $this->assertArrayHasKey(0, $logs['body']['executions']);Also applies to: 1922-1936
1953-1989: Optional: Assert deploymentId is still present when logging is disabledSince execution-level metadata should remain even when content is suppressed, asserting deploymentId here strengthens coverage for the new field under the “logging => false” path.
$this->assertEquals("GET", $logs['body']['executions'][0]['requestMethod']); $this->assertEquals("/logs-inline", $logs['body']['executions'][0]['requestPath']); + $this->assertNotEmpty($logs['body']['executions'][0]['deploymentId']);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/e2e/Services/Sites/SitesCustomServerTest.php(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/e2e/Services/Sites/SitesCustomServerTest.php (1)
tests/e2e/Services/Sites/SitesBase.php (1)
listLogs(227-237)
⏰ 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 (VCS)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Teams)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Databases)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: E2E General Test
- GitHub Check: Unit Test
- GitHub Check: Benchmark
- GitHub Check: scan
🔇 Additional comments (1)
tests/e2e/Services/Sites/SitesCustomServerTest.php (1)
1885-1917: Sanity-check Sites logs fordeploymentIdsupportConfirmed the following server-side implementations:
- Execution response model includes
deploymentId(src/Appwrite/Utopia/Response/Model/Execution.php)- Query filtering on
deploymentIdis allowed for logs (src/Appwrite/Utopia/Database/Validator/Queries/Logs.php)- Sites logs route (
XList.php) defines the/v1/sites/:siteId/logsendpointNext step:
- Verify that the
actionmethod inXList.phpserializes each execution using the Execution model (ensuringdeploymentIdis present in the response).Points to check:
- In
src/Appwrite/Platform/Modules/Sites/Http/Logs/XList.php, that theactionmethod iterates over fetched executions and calls(or its equivalent) so that$response->dynamic($document, Response::MODEL_EXECUTION);deploymentIdis included in each execution entry.Please review the
actionimplementation inXList.phpto ensure it usesResponse::MODEL_EXECUTIONfor each execution.
If it does, the E2E tests asserting presence and filtering ofdeploymentIdwill be fully supported.
If not, update the serialization inXList.phpaccordingly.
What does this PR do?
Test Plan
New tests
Related PRs and Issues
x
Checklist