Refactor Specs task for better code organization#10950
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThe PR adds a constructor and five helper methods to src/Appwrite/Platform/Tasks/Specs.php (getPlatforms, getAuthCounts, getKeys, getSDKPlatformsForRouteSecurity, plus _construct) and refactors Specs::action() to use them. OpenAPI3 and Swagger2 formatters were changed to call Specs::getSDKPlatformsForRouteSecurity() instead of using inline auth→platform mapping and to conditionally add edit links. app/config/sdks.php renames three SDK platform constants to the APP_SDK* namespace. app/config/services.php adds a new 'platforms' array to many service entries. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (14)
📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-08-29T15:29:23.250ZApplied to files:
📚 Learning: 2025-08-29T15:29:23.250ZApplied to files:
🧬 Code graph analysis (2)src/Appwrite/SDK/Specification/Format/Swagger2.php (1)
src/Appwrite/SDK/Specification/Format/OpenAPI3.php (3)
⏰ 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)
🔇 Additional comments (2)
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 |
commit: |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
de017b5 to
8b00e95
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the Specs task class to improve maintainability by extracting inline logic into dedicated methods. The constructor is moved to the top of the class, and platform definitions, authentication counts, security keys, and SDK platform determination logic are extracted into separate methods with PHPDoc annotations.
Key Changes:
- Constructor relocated to top of class following common PHP conventions
- Inline arrays for platforms, auth counts, and keys extracted into dedicated getter methods
- SDK platform security logic extracted into
getSDKPlatformsForRouteSecurity()method
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/Appwrite/Platform/Tasks/Specs.php (3)
64-68: PHPDoc description remains too brief.The PHPDoc still only says "Platforms" without describing what the method does or what the returned mapping represents.
93-97: PHPDoc doesn't document the nested array structure.The return type
array{client: array, server: array, console: array}doesn't describe the nested configuration object structure withtype,name,description, andinfields.
206-224: Missing PHPDoc documentation.This method lacks PHPDoc describing its purpose, parameter types, and return type.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/Platform/Tasks/Specs.php(4 hunks)
⏰ 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). (14)
- GitHub Check: Benchmark
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Migrations)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Databases/TablesDB)
🔇 Additional comments (2)
src/Appwrite/Platform/Tasks/Specs.php (2)
31-38: LGTM!The constructor is properly implemented, correctly configuring the task with description, parameters, and callback using Utopia Platform conventions.
226-241: Clean refactoring of action() method.The method correctly initializes resources and delegates to the new helper methods. The use of null coalescing at line 330 (
$authCounts[$platform] ?? 0) provides safe handling for any missing platform keys.
✨ Benchmark results
⚡ Benchmark Comparison
|
6d674be to
cc3b7dc
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
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/Specs.php (1)
273-279: Remove redundant platform addition causing duplicates.The
getSDKPlatformsForRouteSecurity()method already handles empty$routeSecurityby adding default platforms (lines 224-227). The check at lines 276-279 duplicates this logic, resulting in duplicate platform entries when$routeSecurityis empty.Trace: When
$routeSecurityis[]:
- Line 274 calls
getSDKPlatformsForRouteSecurity([])→ returns[APP_PLATFORM_SERVER, APP_PLATFORM_CLIENT]- Lines 276-279 check
if (empty($routeSecurity))→true, appends again- Result:
[APP_PLATFORM_SERVER, APP_PLATFORM_CLIENT, APP_PLATFORM_SERVER, APP_PLATFORM_CLIENT]Apply this diff to remove the redundant check:
$routeSecurity = $sdk->getAuth(); $sdkPlatforms = $this->getSDKPlatformsForRouteSecurity($routeSecurity); - if (empty($routeSecurity)) { - $sdkPlatforms[] = APP_PLATFORM_SERVER; - $sdkPlatforms[] = APP_PLATFORM_CLIENT; - } - if (!$route->getLabel('docs', true)) { continue; }
♻️ Duplicate comments (1)
src/Appwrite/Platform/Tasks/Specs.php (1)
206-206: Add PHPDoc documentation for this public method.The method is missing documentation describing its purpose, parameter types, and return value. This is important for a public API method.
Apply this diff:
+ /** + * Returns an array of platform identifiers corresponding to the given route security types. + * + * Maps authentication types (SESSION, JWT, KEY, ADMIN) to their respective SDK platforms. + * Defaults to SERVER and CLIENT platforms when no security types are provided. + * + * @param array $routeSecurity Array of AuthType enum values representing route security + * @return array Array of platform constant strings + */ public function getSDKPlatformsForRouteSecurity(array $routeSecurity): array
🧹 Nitpick comments (2)
src/Appwrite/Platform/Tasks/Specs.php (2)
64-68: Improve the PHPDoc description.The current description "Platforms" is too brief. Consider a more descriptive comment that explains the method's purpose.
Apply this diff:
/** - * Platforms + * Returns platform identifiers mapped to their constants. + * Used to determine the platforms for which API specifications should be generated. * * @return array{client: string, server: string, console: string} */
93-97: Consider enhancing the PHPDoc with structure details.The current PHPDoc "Keys for each platform" doesn't document the nested array structure. Consider adding details about the configuration object fields (type, name, description, in) to improve maintainability.
Example improvement:
/** * Keys for each platform. + * + * Returns an associative array where each platform key maps to an array + * of authentication/header configuration objects with fields: + * - type: string ('apiKey') + * - name: string (header name) + * - description: string (description of the key) + * - in: string ('header') * * @return array{client: array, server: array, console: array} */
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Appwrite/Platform/Tasks/Specs.php(4 hunks)src/Appwrite/SDK/Specification/Format/OpenAPI3.php(3 hunks)src/Appwrite/SDK/Specification/Format/Swagger2.php(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-05T15:30:59.312Z
Learnt from: ArnabChatterjee20k
Repo: appwrite/appwrite PR: 10443
File: src/Appwrite/SDK/Specification/Format/OpenAPI3.php:450-458
Timestamp: 2025-09-05T15:30:59.312Z
Learning: In OpenAPI 3.0.x, using oneOf: [{'type': 'array'}] for array items is the correct approach for spatial data validation, as it ensures each element is specifically an array type. Using new \stdClass() or empty objects {} for items can cause compatibility issues with OpenAPI 3.0.x specification.
Applied to files:
src/Appwrite/SDK/Specification/Format/OpenAPI3.php
🧬 Code graph analysis (2)
src/Appwrite/SDK/Specification/Format/OpenAPI3.php (1)
src/Appwrite/Platform/Tasks/Specs.php (2)
Specs(29-385)getSDKPlatformsForRouteSecurity(206-230)
src/Appwrite/SDK/Specification/Format/Swagger2.php (1)
src/Appwrite/Platform/Tasks/Specs.php (2)
Specs(29-385)getSDKPlatformsForRouteSecurity(206-230)
🪛 GitHub Actions: Linter
src/Appwrite/SDK/Specification/Format/OpenAPI3.php
[error] 1-1: Pint: PSR-12 ordered_imports violation. File has imports not sorted lexicographically.
src/Appwrite/SDK/Specification/Format/Swagger2.php
[error] 1-1: Pint: PSR-12 ordered_imports violation. File has imports not sorted lexicographically.
⏰ 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 (Locale)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: Unit Test
- GitHub Check: E2E General Test
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: Benchmark
- GitHub Check: scan
🔇 Additional comments (4)
src/Appwrite/Platform/Tasks/Specs.php (4)
31-38: LGTM!The constructor properly initializes the action with appropriate parameters, validators, and callback registration following Utopia conventions.
78-91: LGTM!The method correctly returns authentication counts for each platform, and the documentation is consistent with the implementation.
207-230: LGTM!The method correctly maps authentication types to platform identifiers with appropriate fallback behavior for empty security arrays.
245-247: LGTM!Good refactoring using the new helper methods to initialize platforms, authentication counts, and keys. This improves code organization and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 1
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/Specs.php (1)
276-279: Remove redundant default platform logic.The
getSDKPlatformsForRouteSecurity()method already returns default platforms when the input array is empty (lines 224-227). Adding the same defaults again here results in duplicate platforms in the$sdkPlatformsarray.Remove this redundant block:
$routeSecurity = $sdk->getAuth(); $sdkPlatforms = $this->getSDKPlatformsForRouteSecurity($routeSecurity); - if (empty($routeSecurity)) { - $sdkPlatforms[] = APP_PLATFORM_SERVER; - $sdkPlatforms[] = APP_PLATFORM_CLIENT; - } - if (!$route->getLabel('docs', true)) {
♻️ Duplicate comments (5)
src/Appwrite/SDK/Specification/Format/OpenAPI3.php (3)
5-5: Import order violation already flagged.This issue was identified in previous reviews.
123-124: Repeated instantiation in loop already flagged.This performance issue was identified and marked as addressed in previous reviews, but the code still shows the instance being created inside the loop.
170-171: Repeated instantiation in nested loop already flagged.This performance issue in the nested loop was identified in previous reviews.
src/Appwrite/Platform/Tasks/Specs.php (2)
64-76: Brief PHPDoc already flagged.The PHPDoc comment for this method was identified as too brief in previous reviews.
93-204: Brief PHPDoc already flagged.The PHPDoc for this method was identified as needing more detail in previous reviews.
🧹 Nitpick comments (1)
src/Appwrite/Platform/Tasks/Specs.php (1)
206-230: Add PHPDoc for public method.This public method lacks documentation describing its purpose, parameters, and return value. Consider adding a docblock.
Example:
/** * Maps route security types to SDK platform identifiers. * * @param array $routeSecurity Array of AuthType enum values * @return array Array of platform constants (APP_PLATFORM_*) */ public function getSDKPlatformsForRouteSecurity(array $routeSecurity): array
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Appwrite/Platform/Tasks/Specs.php(4 hunks)src/Appwrite/SDK/Specification/Format/OpenAPI3.php(3 hunks)src/Appwrite/SDK/Specification/Format/Swagger2.php(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Appwrite/SDK/Specification/Format/Swagger2.php
🧰 Additional context used
🧬 Code graph analysis (1)
src/Appwrite/SDK/Specification/Format/OpenAPI3.php (1)
src/Appwrite/Platform/Tasks/Specs.php (2)
Specs(29-385)getSDKPlatformsForRouteSecurity(206-230)
⏰ 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: moderate
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Teams)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Tokens)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (Migrations)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E General Test
🔇 Additional comments (2)
src/Appwrite/Platform/Tasks/Specs.php (2)
31-38: LGTM!The constructor is properly implemented with appropriate parameter validation and callback setup.
78-91: LGTM!The
managerinconsistency from previous reviews has been resolved. The documentation and implementation now match correctly.
cc3b7dc to
77ee2f6
Compare
- Extract platform, auth counts, and keys into separate methods - Add getPlatforms(), getAuthCounts(), and getKeys() methods - Extract SDK platform logic into getSDKPlatformsForRouteSecurity() - Add PHPDoc comments with proper type hints - Move constructor to top of class for better readability
77ee2f6 to
5daacb7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (12)
app/config/specs/open-api3-1.8.x-client.jsonis excluded by!app/config/specs/**app/config/specs/open-api3-1.8.x-console.jsonis excluded by!app/config/specs/**app/config/specs/open-api3-1.8.x-server.jsonis excluded by!app/config/specs/**app/config/specs/open-api3-latest-client.jsonis excluded by!app/config/specs/**app/config/specs/open-api3-latest-console.jsonis excluded by!app/config/specs/**app/config/specs/open-api3-latest-server.jsonis excluded by!app/config/specs/**app/config/specs/swagger2-1.8.x-client.jsonis excluded by!app/config/specs/**app/config/specs/swagger2-1.8.x-console.jsonis excluded by!app/config/specs/**app/config/specs/swagger2-1.8.x-server.jsonis excluded by!app/config/specs/**app/config/specs/swagger2-latest-client.jsonis excluded by!app/config/specs/**app/config/specs/swagger2-latest-console.jsonis excluded by!app/config/specs/**app/config/specs/swagger2-latest-server.jsonis excluded by!app/config/specs/**
📒 Files selected for processing (2)
app/config/sdks.php(24 hunks)app/config/services.php(22 hunks)
⏰ 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 (Teams)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Tokens)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Migrations)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: Unit Test
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: E2E General Test
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: Setup & Build SDK
- GitHub Check: scan
🔇 Additional comments (1)
app/config/sdks.php (1)
4-5: Constants properly defined and all references updated.The constants have been consistently renamed from
APP_PLATFORM_*toAPP_SDK_PLATFORM_*throughout the file and codebase. Verification confirms:
- Constants are properly defined as class constants in
app/init/constants.php(lines 92-94)- No remaining references to old constant names exist across the codebase
- New constants are consistently used in
app/config/sdks.php,src/Appwrite/Platform/Tasks/Specs.php,src/Appwrite/Platform/Tasks/SDKs.php, andapp/init/resources.php
Summary
Refactors the Specs task class to improve code organization and maintainability by extracting inline arrays and logic into dedicated methods.
Changes
getPlatforms()methodgetAuthCounts()methodgetKeys()methodgetSDKPlatformsForRouteSecurity()methodBenefits
Test plan