Request model support #1264
Request model support #1264
Conversation
WalkthroughThis pull request introduces comprehensive support for request models across the SDK generator platform. Changes include: (1) PHP version updates in composer.json files (8.2→8.3, 8.2→8.4); (2) addition of Player model and validator classes in the mock server with new API endpoints for model testing; (3) type safety enhancements across core Spec classes with new abstract method return types and a new getRequestModels() method; (4) parallel updates to all language generators (Android, Apple, Dart, DotNet, Go, JS, Kotlin, Node, PHP, Python, Ruby, Swift, Web, etc.) to generate request model files and resolve request model types; (5) new Twig templates for generating language-specific request model classes; (6) test infrastructure expansion with Base constants for model responses and updates to all test suites; (7) specification updates adding player definition and new mock endpoints. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Areas requiring extra attention
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (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). (20)
🔇 Additional comments (7)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/SDK/Language/Ruby.php (1)
269-269: Pre-existing typo:'nill'should be'nil'.This is a pre-existing issue not introduced by this PR, but worth noting: Ruby's null value is
nil, notnill.- $output .= 'nill'; + $output .= 'nil';src/Spec/Spec.php (1)
166-188: Stale docblock parameter.Line 168 references
@param array $parameterbut the method signature has$typeas the third parameter, not$parameter./** * Set Attribute * * Method for setting a specific field attribute * * @param string $key * @param mixed $value - * @param array $parameter + * @param string $type * @return mixed */ public function setAttribute(string $key, mixed $value, $type = self::SET_TYPE_ASSIGN): mixed
🧹 Nitpick comments (17)
templates/php/src/Services/Service.php.twig (1)
9-23: Model imports for service methods look good; consider separating enum/model dedupeThe new
parameter.modelhandling correctly addsModels\...imports only once per model and mirrors the existing enum pattern, so generated services will have the right model classes in scope.Since
addedis shared between enums and models, if you ever have an enum and a model with the same base name, one of theusestatements could be skipped. If that’s a realistic case, consider tracking them separately (e.g.,addedEnumsvsaddedModels) to avoid accidental collisions.src/SDK/Language/CLI.php (1)
347-366: Improved CLI type resolution for model-backed paramsThe new branches in
getTypeName()forarray.modelandmodelnicely ensure model-backed parameters resolve to a concrete PascalCase type (with[]for arrays) instead of falling through to generic mappings—this should make generated CLI types much more accurate.Minor nit: the docblock still refers to the second parameter as
$nestedTypes, but the signature uses$spec. If you touch this again, aligning those names would avoid confusion for readers and tooling.src/SDK/Language/ReactNative.php (1)
131-150: React Native model types now resolved explicitlyThe added checks for
array.modelandmodelingetTypeName()correctly map model-backed parameters toModels.<PascalCase>(and[]for arrays), which should give much stronger typings for request models and other modeled inputs without disturbing the existingresponseModel-based object handling.Similar to CLI, the docblock still names the second argument
$nestedTypeswhile the method uses$method; if you revisit this, updating the annotation would keep docs and implementation in sync.templates/php/src/Models/RequestModel.php.twig (2)
74-74: Simplify the complex ternary logic for better readability.Line 74 contains deeply nested ternary operators handling sub_schema, arrays, and enums in a single expression. This makes the template difficult to read and maintain.
Consider breaking this into a Twig macro or helper function that handles the conversion logic more explicitly:
'{{ property.name }}' => {{ property | convertToArrayValue }},Or split the logic across multiple lines using Twig's set/if blocks before the array definition.
84-86: Add error handling for JSON encoding.The
json_encode()call can returnfalseon failure (e.g., for non-UTF-8 strings or circular references), but the method signature promises astringreturn type.Consider adding error handling:
public function toJson(): string { - return json_encode($this->toArray()); + $json = json_encode($this->toArray()); + if ($json === false) { + throw new \RuntimeException('Failed to encode model to JSON: ' . json_last_error_msg()); + } + return $json; }tests/languages/ruby/tests.rb (1)
93-96: Optional: Add trailing comma for consistency.RuboCop suggests adding a trailing comma after the last array item for consistency with Ruby style conventions.
Apply this diff:
response = general.create_players(players: [ {id: 'player1', name: 'John Doe', score: 100}, - {id: 'player2', name: 'Jane Doe', score: 200} + {id: 'player2', name: 'Jane Doe', score: 200}, ])src/SDK/Language/Swift.php (2)
310-314: Request-model file mapping for Swift looks consistent; double-check path conventionThe new
requestModelscope entry mirrors the existingdefinitionscope (same...{{ spec.title | caseUcfirst}}Models/...andswift/Sources/Models/*.twigusage), so it should integrate cleanly into generation.One thing to verify: other Swift files (e.g.,
Error.swift,UploadProgress.swift) use/Sources/{{ spec.title | caseUcfirst}}/Models/...with a slash beforeModels, while both thedefinitionand newrequestModelentries use{{ spec.title | caseUcfirst}}Modelswithout the slash. If the intention isSources/AppwriteModels/..., this is fine; if the desired layout isSources/Appwrite/Models/..., you may want to adjust both entries together for consistency.
335-341: Model and array-of-model type resolution ingetTypeNamelooks correct; consider minor robustness tweakThe new branches correctly map:
array.model→[<SpecTitle>Models.<ModelName>]model→<SpecTitle>Models.<ModelName>or[<SpecTitle>Models.<ModelName>]whentype === TYPE_ARRAY.To make this a bit more defensive in case
$parameter['array']or$parameter['type']is missing for some inputs, you could mirror the laterTYPE_ARRAYbranch and use null-coalescing/local temporaries, e.g.:$array = $parameter['array'] ?? null; if (!empty($array['model'])) { return '[' . ($spec['title'] ?? '') . 'Models.' . $this->toPascalCase($array['model']) . ']'; } $model = $parameter['model'] ?? null; if (!empty($model)) { $modelType = ($spec['title'] ?? '') . 'Models.' . $this->toPascalCase($model); return (($parameter['type'] ?? null) === self::TYPE_ARRAY) ? '[' . $modelType . ']' : $modelType; }This keeps behavior the same for well-formed parameters while reducing the chance of notices if a caller passes an unexpected shape.
src/SDK/SDK.php (1)
707-717: LGTM! Request model generation implemented correctly.The new
requestModelcase follows the established pattern used fordefinitiongeneration. The implementation properly iterates through request models, respects exclusion rules, and renders templates.Consider whether the
setExclude()method (line 765) andexclude()method (line 788) should be extended to support request model exclusions in the future. The current implementation only handles services, methods, and definitions, which may be sufficient for now but could be expanded if selective request model exclusion becomes necessary.mock-server/src/Utopia/Model/Player.php (1)
16-23: Consider defensive handling for missing array keys.The
fromArraymethod directly accesses array keys without checking for their existence. If a malformed payload is passed, this will throw anUndefined array keyerror rather than a more descriptive exception.public static function fromArray(array $value): static { + if (!isset($value['id'], $value['name'], $value['score'])) { + throw new \InvalidArgumentException('Missing required keys: id, name, score'); + } return new static( id: $value['id'], name: $value['name'], score: $value['score'], ); }templates/swift/Sources/Models/RequestModel.swift.twig (1)
36-43: Consider extracting complex mapping logic for readability.The inline logic on line 39 handles multiple cases (sub_schema arrays, sub_schema objects, enums, plain values) in a single dense line. While functional, this could be challenging to maintain.
Consider breaking out the value mapping into a Twig macro or splitting across multiple lines for clarity:
{%~ for property in requestModel.properties %} {% set propName = property.name | escapeSwiftKeyword | removeDollarSign %} {% set optionalChain = property.required ? '' : '?' %} {% if property.sub_schema %} {% if property.type == 'array' %} "{{ property.name }}": {{ propName }}{{ optionalChain }}.map { $0.toMap() }, {% else %} "{{ property.name }}": {{ propName }}{{ optionalChain }}.toMap(), {% endif %} {% elseif property.enum %} "{{ property.name }}": {{ propName }}{{ optionalChain }}.rawValue, {% else %} "{{ property.name }}": {{ propName }}, {% endif %} {%~ endfor %}templates/android/library/src/main/java/io/package/models/RequestModel.kt.twig (1)
1-30: Template duplication withtemplates/kotlin/.../RequestModel.kt.twig.This template is identical to
templates/kotlin/src/main/kotlin/io/appwrite/models/RequestModel.kt.twig. While this may be intentional to maintain separate Android and Kotlin SDK templates, consider whether a shared template could reduce maintenance burden if the logic remains identical.The template itself is correctly structured for generating Kotlin request model classes.
templates/python/package/models/request_model.py.twig (1)
33-36: Consider movingjsonimport to file level.The
jsonmodule import insideto_json()works but is unconventional. Moving it to the file-level imports would be more Pythonic and avoid repeated import overhead if called multiple times.-from typing import Optional, List, Dict, Any +from typing import Optional, List, Dict, Any +import json ... def to_json(self) -> str: """Convert the model to a JSON string.""" - import json return json.dumps(self.to_map())templates/ruby/lib/container/models/request_model.rb.twig (1)
29-40: Inconsistent use ofremoveDollarSignfilter.On line 32,
removeDollarSignis applied to properties withsub_schemabut not to regular properties in the else branch. If the property name can contain a$character, this inconsistency could cause issues.Consider applying the filter consistently:
- "{{ property.name }}": {% if property.sub_schema %}{% if property.type == 'array' %}@{{ property.name | caseSnake | escapeKeyword | removeDollarSign }}.map { |it| it.to_map }{% else %}@{{property.name | caseSnake | escapeKeyword | removeDollarSign }}.to_map{% endif %}{% else %}@{{property.name | caseSnake | escapeKeyword }}{% endif %}{% if not loop.last %},{% endif %} + "{{ property.name }}": {% if property.sub_schema %}{% if property.type == 'array' %}@{{ property.name | caseSnake | escapeKeyword | removeDollarSign }}.map { |it| it.to_map }{% else %}@{{property.name | caseSnake | escapeKeyword | removeDollarSign }}.to_map{% endif %}{% else %}@{{property.name | caseSnake | escapeKeyword | removeDollarSign }}{% endif %}{% if not loop.last %},{% endif %}mock-server/src/Utopia/Response.php (2)
32-36: Unused constructor parameter.The
$responseparameter is declared but never used in the constructor body. If this is intentional (to satisfy the parent signature without using it), consider adding a comment or suppression annotation to clarify intent.public function __construct(SwooleResponse $response) { + // Intentionally not calling parent constructor or using $response }
257-260: Consider using strict null comparison.Using
!=for null comparison works but!==is preferred for clarity and to avoid potential type coercion issues.public static function hasFilter(): bool { - return self::$filter != null; + return self::$filter !== null; }src/Spec/Swagger2.php (1)
516-559: Significant code duplication withgetDefinitions().
getRequestModels()shares ~90% of its logic withgetDefinitions(). Consider extracting the common property enrichment logic into a private helper method to reduce duplication and ensure consistent behavior.Also, the same
descriptiontype issue exists on line 527:$model = [ 'name' => $key, 'properties' => $schema['properties'] ?? [], - 'description' => $schema['description'] ?? [], + 'description' => $schema['description'] ?? '', 'required' => $schema['required'] ?? [], 'additionalProperties' => $schema['additionalProperties'] ?? [] ];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
mock-server/composer.lockis excluded by!**/*.lock
📒 Files selected for processing (89)
composer.json(1 hunks)example.php(1 hunks)mock-server/app/http.php(3 hunks)mock-server/composer.json(1 hunks)mock-server/src/Utopia/Model.php(2 hunks)mock-server/src/Utopia/Model/Player.php(1 hunks)mock-server/src/Utopia/Response.php(6 hunks)mock-server/src/Utopia/Validator/Player.php(1 hunks)src/SDK/Language/Android.php(1 hunks)src/SDK/Language/Apple.php(1 hunks)src/SDK/Language/CLI.php(1 hunks)src/SDK/Language/Dart.php(2 hunks)src/SDK/Language/Deno.php(1 hunks)src/SDK/Language/DotNet.php(2 hunks)src/SDK/Language/Flutter.php(1 hunks)src/SDK/Language/Go.php(2 hunks)src/SDK/Language/JS.php(1 hunks)src/SDK/Language/Kotlin.php(2 hunks)src/SDK/Language/Node.php(2 hunks)src/SDK/Language/PHP.php(2 hunks)src/SDK/Language/Python.php(2 hunks)src/SDK/Language/ReactNative.php(1 hunks)src/SDK/Language/Ruby.php(2 hunks)src/SDK/Language/Swift.php(2 hunks)src/SDK/Language/Web.php(1 hunks)src/SDK/SDK.php(2 hunks)src/Spec/Spec.php(5 hunks)src/Spec/Swagger2.php(8 hunks)templates/android/library/src/main/java/io/package/models/RequestModel.kt.twig(1 hunks)templates/dart/lib/models.dart.twig(1 hunks)templates/dart/lib/src/models/request_model.dart.twig(1 hunks)templates/dotnet/Package/Models/RequestModel.cs.twig(1 hunks)templates/go/models/request_model.go.twig(1 hunks)templates/kotlin/src/main/kotlin/io/appwrite/models/RequestModel.kt.twig(1 hunks)templates/node/src/models/request_model.ts.twig(1 hunks)templates/php/src/Models/RequestModel.php.twig(1 hunks)templates/php/src/Services/Service.php.twig(1 hunks)templates/python/package/models/request_model.py.twig(1 hunks)templates/ruby/lib/container/models/request_model.rb.twig(1 hunks)templates/swift/Sources/Models/RequestModel.swift.twig(1 hunks)tests/Android14Java11Test.php(1 hunks)tests/Android14Java17Test.php(1 hunks)tests/Android14Java8Test.php(1 hunks)tests/Android5Java17Test.php(1 hunks)tests/AppleSwift56Test.php(1 hunks)tests/Base.php(2 hunks)tests/DartBetaTest.php(1 hunks)tests/DartStableTest.php(1 hunks)tests/Deno1193Test.php(1 hunks)tests/Deno1303Test.php(1 hunks)tests/DotNet60Test.php(1 hunks)tests/DotNet80Test.php(1 hunks)tests/DotNet90Test.php(1 hunks)tests/FlutterBetaTest.php(1 hunks)tests/FlutterStableTest.php(1 hunks)tests/KotlinJava11Test.php(1 hunks)tests/KotlinJava17Test.php(1 hunks)tests/KotlinJava8Test.php(1 hunks)tests/Node16Test.php(1 hunks)tests/Node18Test.php(1 hunks)tests/Node20Test.php(1 hunks)tests/PHP80Test.php(1 hunks)tests/PHP83Test.php(1 hunks)tests/Python310Test.php(1 hunks)tests/Python311Test.php(1 hunks)tests/Python312Test.php(1 hunks)tests/Python313Test.php(1 hunks)tests/Python39Test.php(1 hunks)tests/Ruby27Test.php(1 hunks)tests/Ruby30Test.php(1 hunks)tests/Ruby31Test.php(1 hunks)tests/Swift56Test.php(1 hunks)tests/WebChromiumTest.php(1 hunks)tests/WebNodeTest.php(1 hunks)tests/languages/android/Tests.kt(2 hunks)tests/languages/apple/Tests.swift(1 hunks)tests/languages/dart/tests.dart(1 hunks)tests/languages/deno/tests.ts(1 hunks)tests/languages/dotnet/Tests.cs(1 hunks)tests/languages/flutter/tests.dart(1 hunks)tests/languages/kotlin/Tests.kt(2 hunks)tests/languages/node/test.js(1 hunks)tests/languages/php/test.php(2 hunks)tests/languages/python/tests.py(1 hunks)tests/languages/ruby/tests.rb(1 hunks)tests/languages/swift/Tests.swift(1 hunks)tests/languages/web/index.html(1 hunks)tests/languages/web/node.js(1 hunks)tests/resources/spec.json(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (56)
tests/languages/web/node.js (1)
tests/languages/node/test.js (1)
general(28-28)
src/SDK/Language/Dart.php (1)
src/SDK/Language.php (1)
toPascalCase(116-119)
tests/Android5Java17Test.php (1)
tests/Base.php (1)
Base(17-335)
tests/FlutterStableTest.php (1)
tests/Base.php (1)
Base(17-335)
tests/Python311Test.php (1)
tests/Base.php (1)
Base(17-335)
mock-server/app/http.php (2)
mock-server/src/Utopia/Model/Player.php (1)
Player(7-24)mock-server/src/Utopia/Validator/Player.php (1)
Player(8-29)
tests/Deno1303Test.php (1)
tests/Base.php (1)
Base(17-335)
tests/languages/swift/Tests.swift (4)
tests/languages/node/test.js (1)
general(28-28)tests/languages/web/node.js (1)
general(11-11)mock-server/src/Utopia/Model/Player.php (1)
Player(7-24)mock-server/src/Utopia/Validator/Player.php (1)
Player(8-29)
tests/DartBetaTest.php (1)
tests/Base.php (1)
Base(17-335)
tests/KotlinJava11Test.php (1)
tests/Base.php (1)
Base(17-335)
mock-server/src/Utopia/Validator/Player.php (4)
mock-server/src/Utopia/Model.php (2)
Model(7-166)getType(67-67)mock-server/src/Utopia/Model/Player.php (1)
Player(7-24)src/Spec/Spec.php (1)
getDescription(53-53)src/Spec/Swagger2.php (1)
getDescription(20-23)
tests/WebChromiumTest.php (1)
tests/Base.php (1)
Base(17-335)
tests/Ruby27Test.php (1)
tests/Base.php (1)
Base(17-335)
tests/Python313Test.php (1)
tests/Base.php (1)
Base(17-335)
mock-server/src/Utopia/Model/Player.php (2)
mock-server/src/Utopia/Model.php (1)
Model(7-166)mock-server/src/Utopia/Validator/Player.php (1)
Player(8-29)
tests/Node16Test.php (1)
tests/Base.php (1)
Base(17-335)
tests/languages/apple/Tests.swift (4)
tests/languages/node/test.js (1)
general(28-28)tests/languages/web/node.js (1)
general(11-11)mock-server/src/Utopia/Model/Player.php (1)
Player(7-24)mock-server/src/Utopia/Validator/Player.php (1)
Player(8-29)
src/SDK/Language/Web.php (1)
src/SDK/Language.php (1)
toPascalCase(116-119)
tests/KotlinJava8Test.php (1)
tests/Base.php (1)
Base(17-335)
tests/languages/node/test.js (1)
tests/languages/web/node.js (2)
response(4-4)general(11-11)
src/SDK/Language/DotNet.php (1)
src/SDK/Language.php (1)
toPascalCase(116-119)
tests/PHP80Test.php (1)
tests/Base.php (1)
Base(17-335)
src/SDK/Language/ReactNative.php (1)
src/SDK/Language.php (1)
toPascalCase(116-119)
tests/languages/deno/tests.ts (2)
tests/languages/node/test.js (2)
response(18-18)general(28-28)tests/languages/web/node.js (2)
response(4-4)general(11-11)
tests/Python310Test.php (1)
tests/Base.php (1)
Base(17-335)
tests/Android14Java11Test.php (1)
tests/Base.php (1)
Base(17-335)
tests/Python312Test.php (1)
tests/Base.php (1)
Base(17-335)
src/SDK/Language/Python.php (1)
src/SDK/Language.php (1)
toPascalCase(116-119)
tests/WebNodeTest.php (1)
tests/Base.php (1)
Base(17-335)
tests/Swift56Test.php (1)
tests/Base.php (1)
Base(17-335)
tests/Android14Java8Test.php (1)
tests/Base.php (1)
Base(17-335)
src/SDK/Language/CLI.php (1)
src/SDK/Language.php (1)
toPascalCase(116-119)
tests/Android14Java17Test.php (1)
tests/Base.php (1)
Base(17-335)
tests/Ruby30Test.php (1)
tests/Base.php (1)
Base(17-335)
tests/PHP83Test.php (1)
tests/Base.php (1)
Base(17-335)
src/SDK/Language/Deno.php (1)
src/SDK/Language.php (1)
toPascalCase(116-119)
tests/AppleSwift56Test.php (1)
tests/Base.php (1)
Base(17-335)
tests/DotNet60Test.php (1)
tests/Base.php (1)
Base(17-335)
src/SDK/Language/Kotlin.php (1)
src/SDK/Language.php (1)
toPascalCase(116-119)
tests/DartStableTest.php (1)
tests/Base.php (1)
Base(17-335)
src/SDK/Language/Node.php (1)
src/SDK/Language.php (1)
toPascalCase(116-119)
src/SDK/Language/JS.php (1)
src/SDK/Language.php (1)
toPascalCase(116-119)
tests/languages/dotnet/Tests.cs (4)
tests/languages/node/test.js (1)
general(28-28)tests/languages/web/node.js (1)
general(11-11)mock-server/src/Utopia/Model/Player.php (1)
Player(7-24)mock-server/src/Utopia/Validator/Player.php (1)
Player(8-29)
src/SDK/Language/PHP.php (1)
src/SDK/Language.php (1)
toPascalCase(116-119)
src/SDK/Language/Go.php (1)
src/SDK/Language.php (1)
toPascalCase(116-119)
tests/languages/kotlin/Tests.kt (3)
mock-server/src/Utopia/Model/Player.php (1)
Player(7-24)mock-server/src/Utopia/Validator/Player.php (1)
Player(8-29)tests/languages/android/Tests.kt (1)
writeToFile(315-318)
src/SDK/SDK.php (2)
src/Spec/Spec.php (1)
getRequestModels(133-133)src/Spec/Swagger2.php (1)
getRequestModels(516-559)
tests/Ruby31Test.php (1)
tests/Base.php (1)
Base(17-335)
tests/FlutterBetaTest.php (1)
tests/Base.php (1)
Base(17-335)
tests/languages/php/test.php (2)
mock-server/src/Utopia/Model/Player.php (1)
Player(7-24)mock-server/src/Utopia/Validator/Player.php (1)
Player(8-29)
tests/DotNet80Test.php (1)
tests/Base.php (1)
Base(17-335)
tests/KotlinJava17Test.php (1)
tests/Base.php (1)
Base(17-335)
src/SDK/Language/Swift.php (1)
src/SDK/Language.php (1)
toPascalCase(116-119)
tests/DotNet90Test.php (1)
tests/Base.php (1)
Base(17-335)
mock-server/src/Utopia/Response.php (1)
mock-server/src/Utopia/Model.php (2)
filter(50-53)Model(7-166)
src/Spec/Spec.php (1)
src/Spec/Swagger2.php (19)
getTitle(12-15)getDescription(20-23)getNamespace(28-31)getVersion(36-39)getEndpoint(44-49)getEndpointDocs(54-59)getLicenseName(64-67)getLicenseURL(72-75)getContactName(80-83)getContactURL(88-91)getContactEmail(96-99)getServices(104-136)getMethods(300-339)getTargetNamespace(432-435)getGlobalHeaders(440-457)getDefinitions(459-509)getRequestModels(516-559)getRequestEnums(564-587)getResponseEnums(592-629)
🪛 GitHub Actions: Twig Linting
templates/python/package/models/request_model.py.twig
[error] 6-6: H014 6:3 Found extra blank lines. endfor %}. djlint lint reported a formatting issue.
🪛 RuboCop (1.81.7)
tests/languages/ruby/tests.rb
[convention] 95-95: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
⏰ 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: build (8.3, Python313)
- GitHub Check: build (8.3, Swift56)
- GitHub Check: build (8.3, Python39)
- GitHub Check: build (8.3, Ruby30)
- GitHub Check: build (8.3, KotlinJava17)
- GitHub Check: build (8.3, WebNode)
- GitHub Check: build (8.3, PHP83)
- GitHub Check: build (8.3, DartBeta)
- GitHub Check: build (8.3, KotlinJava8)
- GitHub Check: build (8.3, FlutterStable)
- GitHub Check: build (8.3, KotlinJava11)
- GitHub Check: build (8.3, FlutterBeta)
- GitHub Check: dotnet (server)
- GitHub Check: kotlin (server)
- GitHub Check: swift (server)
- GitHub Check: flutter (client)
- GitHub Check: android (client)
- GitHub Check: apple (client)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
templates/node/src/models/request_model.ts.twig (1)
28-28: Consider refactoring for readability.Line 28 contains nested conditionals spanning ~400 characters, making it difficult to parse and maintain. While the logic is correct, splitting the conditions or using intermediate variables would improve clarity.
Example refactoring with intermediate template variables:
{%~ for property in requestModel.properties %} {%~ set propName = property.name %} {%~ set propKey = property.name | escapeKeyword %} {%~ if property.sub_schema %} {%~ if property.type == 'array' %} '{{ propName }}': model.{{ propKey }}{% if not property.required %}?{% endif %}.map(item => {{ property.sub_schema | caseCamel }}ToMap(item)), {%~ else %} '{{ propName }}': model.{{ propKey }} ? {{ property.sub_schema | caseCamel }}ToMap(model.{{ propKey }}) : undefined, {%~ endif %} {%~ elseif property.enum %} '{{ propName }}': model.{{ propKey }}, {%~ else %} '{{ propName }}': model.{{ propKey }}, {%~ endif %} {%~ endfor %}src/SDK/Language/DotNet.php (1)
524-541: Consider consolidating with the existingsub_schemafunction.The
getPropertyTypemethod largely duplicates the logic in the existingsub_schemaTwigFunction (lines 550-571). Both handle sub_schema resolution, enum conversion, and fallback togetTypeName. The main difference is thatsub_schemaadds nullability markers (?) for non-required properties.Consider extracting the shared type-resolution logic into a common helper method that both can call, then handle nullability as a separate concern:
+ protected function resolvePropertyTypeName(array $property, array $spec = []): string + { + if (isset($property['sub_schema']) && !empty($property['sub_schema'])) { + $type = $this->toPascalCase($property['sub_schema']); + if ($property['type'] === 'array') { + return 'List<' . $type . '>'; + } + return $type; + } + + if (isset($property['enum']) && !empty($property['enum'])) { + $enumName = $property['enumName'] ?? $property['name']; + return 'Appwrite.Enums.' . $this->toPascalCase($enumName); + } + + return $this->getTypeName($property, $spec); + } + protected function getPropertyType(array $property, array $spec = []): string { - if (isset($property['sub_schema']) && !empty($property['sub_schema'])) { - $type = $this->toPascalCase($property['sub_schema']); - - if ($property['type'] === 'array') { - return 'List<' . $type . '>'; - } - return $type; - } - - if (isset($property['enum']) && !empty($property['enum'])) { - $enumName = $property['enumName'] ?? $property['name']; - return 'Appwrite.Enums.' . $this->toPascalCase($enumName); - } - - return $this->getTypeName($property, $spec); + return $this->resolvePropertyTypeName($property, $spec); }Then update the
sub_schemafunction similarly and add nullability there.src/Spec/Swagger2.php (1)
511-559: Consider extracting shared property enrichment logic.The new
getRequestModels()method correctly implements request model extraction, but the property enrichment loop (lines 532-554) duplicates the logic fromgetDefinitions()(lines 478-504).Consider extracting the shared property processing into a private helper method:
private function enrichModelProperties(array &$model): void { if (!isset($model['properties'])) { return; } foreach ($model['properties'] as $name => $def) { $model['properties'][$name]['name'] = $name; $model['properties'][$name]['description'] = $def['description'] ?? ''; $model['properties'][$name]['example'] = $def['x-example'] ?? null; $model['properties'][$name]['required'] = in_array($name, $model['required']); if (isset($def['items']['$ref'])) { $model['properties'][$name]['sub_schema'] = str_replace('#/definitions/', '', $def['items']['$ref']); } if (isset($def['items']['x-anyOf'])) { $model['properties'][$name]['sub_schemas'] = \array_map( fn($schema) => str_replace('#/definitions/', '', $schema['$ref']), $def['items']['x-anyOf'] ); } if (isset($def['items']['x-oneOf'])) { $model['properties'][$name]['sub_schemas'] = \array_map( fn($schema) => str_replace('#/definitions/', '', $schema['$ref']), $def['items']['x-oneOf'] ); } if (isset($def['enum'])) { $model['properties'][$name]['enum'] = $def['enum']; $model['properties'][$name]['enumName'] = $def['x-enum-name'] ?? ucfirst($model['name']) . ucfirst($name); $model['properties'][$name]['enumKeys'] = $def['x-enum-keys'] ?? []; } } }Then both methods could call
$this->enrichModelProperties($model);instead of duplicating the loop.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
mock-server/app/http.php(2 hunks)src/SDK/Language/DotNet.php(3 hunks)src/Spec/Spec.php(5 hunks)src/Spec/Swagger2.php(8 hunks)templates/node/src/models/request_model.ts.twig(1 hunks)templates/php/src/Client.php.twig(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/Spec/Spec.php (1)
src/Spec/Swagger2.php (19)
getTitle(12-15)getDescription(20-23)getNamespace(28-31)getVersion(36-39)getEndpoint(44-49)getEndpointDocs(54-59)getLicenseName(64-67)getLicenseURL(72-75)getContactName(80-83)getContactURL(88-91)getContactEmail(96-99)getServices(104-136)getMethods(300-339)getTargetNamespace(432-435)getGlobalHeaders(440-457)getDefinitions(459-509)getRequestModels(516-559)getRequestEnums(564-587)getResponseEnums(592-629)
src/Spec/Swagger2.php (1)
src/Spec/Spec.php (17)
getTitle(48-48)getAttribute(144-159)getDescription(53-53)getNamespace(58-58)getVersion(63-63)getEndpoint(68-68)getEndpointDocs(73-73)getLicenseName(78-78)getLicenseURL(83-83)getContactName(88-88)getContactURL(93-93)getContactEmail(98-98)getServices(103-103)getMethods(109-109)getGlobalHeaders(121-121)getDefinitions(126-126)getRequestModels(133-133)
mock-server/app/http.php (4)
mock-server/src/Utopia/Model.php (1)
Model(7-166)mock-server/src/Utopia/Model/Player.php (1)
Player(7-24)mock-server/src/Utopia/Validator/Player.php (1)
Player(8-29)mock-server/src/Utopia/Response.php (1)
Response(12-273)
⏰ 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). (2)
- GitHub Check: swift (server)
- GitHub Check: flutter (client)
🔇 Additional comments (15)
mock-server/app/http.php (3)
29-30: LGTM! Clean import structure.The imports are properly organized with appropriate aliasing to prevent naming conflicts between the Player model and Player validator.
525-539: LGTM! Well-structured request model endpoint.The endpoint follows the established mock endpoint pattern and correctly integrates the new request model functionality with proper validation and typing.
541-556: LGTM! Nice use of type documentation.The endpoint correctly handles array request models with proper validation. The PHP docblock on line 555 providing
Player[]type information is a good practice for IDE support and developer clarity.templates/node/src/models/request_model.ts.twig (1)
10-20: LGTM!The interface definition correctly handles optional properties, property documentation, and type resolution. The use of
escapeKeywordprevents conflicts with TypeScript reserved words.src/SDK/Language/DotNet.php (4)
195-201: LGTM! Model type resolution is well-structured.The logic correctly handles both array and single model parameters with proper namespace prefixes. The precedence (checking
array['model']beforemodel) ensures the correct type is resolved.
474-478: LGTM! Request model generation entry follows established patterns.The new file generation entry is consistent with the existing definition-scope entry and correctly configured for generating request model files.
511-513: LGTM! Filter implementation is clean and consistent.The
propertyTypefilter follows the established pattern for Twig filters in this class.
526-533: Namespace handling for sub_schema types is intentional and correct.The distinction between sub_schema types (without namespace prefix) and regular model parameters (with
Appwrite.Models.prefix) is by design. sub_schema types are nested models defined within the same spec and generated in the sameAppwrite.Modelsnamespace as their parent models, so they don't require the namespace prefix. This differs from method parameters handled bygetTypeName(), which may reference external models and thus require full qualification.src/Spec/Spec.php (4)
21-27: Good practice: using fully qualified global function names.The use of leading backslashes (e.g.,
\filter_var,\file_get_contents,\stream_context_create) explicitly references global namespace functions, which is a best practice in namespaced code. This prevents potential conflicts and improves clarity.
48-126: Excellent: adding return type hints to abstract methods.The addition of explicit return type hints (
: stringand: array) to all abstract methods significantly improves type safety and enforces contracts for implementing classes. This aligns with modern PHP best practices.
128-133: LGTM! New abstract method for request models.The new
getRequestModels(): arrayabstract method directly supports the PR's request model feature. The method signature is clear, well-documented, and follows the same pattern asgetDefinitions().
144-144: LGTM! Proper use ofmixedtype for dynamic attribute methods.The
mixedreturn type ongetAttribute(line 144) and the typed parameters onsetAttribute(line 171) accurately reflect these methods' dynamic nature. Since attributes can be of any type,mixedis the correct type hint.Note: This also requires PHP 8.0+ (same verification as Client.php.twig applies).
Also applies to: 171-171
src/Spec/Swagger2.php (3)
12-440: Excellent: implementing return type hints for all methods.All method signatures now include explicit return type hints (
: string,: array) that match the abstract declarations inSpec.php. This strengthens type safety across the implementation.
198-198: LGTM! Adding model reference tracking to body parameters.Lines 262 and 265 add
modelfields to capture request model references from body parameters. This enables the template layer to generate properly typed parameters for request models.Also applies to: 262-265
459-509: LGTM! Clean separation of regular definitions from request models.The refactored
getDefinitions()now properly skipsx-request-modeldefinitions (lines 467-469) and enriches properties with metadata needed for template generation. The property processing loop correctly uses null coalescing operators for safe defaults.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/SDK/Language/Go.php(2 hunks)templates/dart/base/utils.twig(1 hunks)templates/flutter/base/utils.twig(1 hunks)templates/go/services/service.go.twig(1 hunks)templates/python/package/services/service.py.twig(1 hunks)templates/web/src/models.ts.twig(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/SDK/Language/Go.php (1)
src/SDK/Language.php (1)
toPascalCase(116-119)
⏰ 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: build (8.3, WebChromium)
- GitHub Check: build (8.3, AppleSwift56)
- GitHub Check: build (8.3, Swift56)
- GitHub Check: build (8.3, PHP80)
- GitHub Check: build (8.3, Node18)
- GitHub Check: build (8.3, Python310)
- GitHub Check: build (8.3, FlutterStable)
- GitHub Check: build (8.3, DotNet60)
- GitHub Check: build (8.3, Node20)
- GitHub Check: build (8.3, Android5Java17)
- GitHub Check: build (8.3, Go118)
- GitHub Check: build (8.3, Android14Java17)
- GitHub Check: build (8.3, DartStable)
- GitHub Check: build (8.3, FlutterBeta)
- GitHub Check: build (8.3, KotlinJava11)
- GitHub Check: build (8.3, CLINode18)
- GitHub Check: build (8.3, KotlinJava8)
- GitHub Check: swift (server)
- GitHub Check: apple (client)
- GitHub Check: android (client)
🔇 Additional comments (7)
templates/python/package/services/service.py.twig (1)
18-23: LGTM! Model import logic follows established patterns.The conditional import for
parameter.modelcorrectly mirrors the existing patterns for enums andInputFile. The duplicate prevention using theaddedarray ensures models are imported only once, and the use ofcaseSnakeandcaseUcfirstfilters maintains consistency with the codebase conventions.templates/go/services/service.go.twig (1)
11-13: LGTM! Clean integration with existing import logic.The conditional check for
parameter.modelcorrectly follows the same pattern used forInputFiledetection. SettingrequireModelsPkg = trueappropriately triggers the models package import, maintaining consistency with the template's existing approach.src/SDK/Language/Go.php (2)
151-155: LGTM!The new
requestModelfile generation entry follows the established pattern from the existingdefinitionscope and uses consistent naming conventions (caseSnakefilter for the destination filename).
169-175: LGTM!The type resolution logic correctly handles model-based types:
array.modelreturns a Go slice type[]models.Xmodelwith array type returns slice, otherwise returns the model type directlyThe check order (before
itemshandling at line 176) ensures model-based resolution takes precedence, which aligns with the AI summary's intent.templates/web/src/models.ts.twig (1)
34-47: LGTM! Clean implementation following existing patterns.The request model generation follows the same structure as the existing definitions block. The optional property handling, schema resolution, and documentation are all correctly implemented.
templates/flutter/base/utils.twig (1)
4-6: LGTM! Consistent with Dart template.The changes mirror those in
templates/dart/base/utils.twig, maintaining consistent handling of model and array.model types across both Dart and Flutter SDK generation. The null-safety logic is correct for Flutter's Dart implementation.templates/dart/base/utils.twig (1)
4-6: LGTM. Null-safety handling for model and array.model types is correct:
- Line 4 correctly uses
!assertion after the null-check guard for non-nullable optional parameters- Line 6 properly applies
?operator only when parameters are not required- The
.map((p) => p.toMap()).toList()pattern for arrays and.toMap()for single models generates valid Dart codeBoth
request_model.dart.twigandmodel.dart.twigconsistently implementtoMap()methods, and the abstractModelclass enforces this contract across all generated model classes.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
templates/python/package/models/request_model.py.twig (1)
1-7: Fix pipeline failure: extra blank lines after import loop.The template still has the formatting issue (H014) flagged in the previous review. When no enum properties are present, extra blank lines appear between imports and the class definition.
🧹 Nitpick comments (2)
templates/python/package/models/request_model.py.twig (1)
32-35: Move json import to module level.Importing
jsoninside the method is unconventional. Module-level imports improve clarity and follow Python conventions.Apply this diff to move the import to the top:
from typing import Optional, List, Dict, Any +import json {%~ for property in requestModel.properties %} {%~ if property.enum %} from ..enums.{{ property.enumName | caseSnake }} import {{ property.enumName | caseUcfirst }} {%~ endif %} {%~ endfor %} class {{ requestModel.name | caseUcfirst }}:Then remove it from the method:
def to_json(self) -> str: """Convert the model to a JSON string.""" - import json return json.dumps(self.to_map())templates/ruby/lib/container/models/request_model.rb.twig (1)
55-57: Consider string interpolation for better Ruby style.The
.to_scall addresses the TypeError risk from the previous review. However, string interpolation is more idiomatic than concatenation with+.Apply this diff for more idiomatic Ruby:
unless valid_{{ property.name | caseSnake }}.include?({{ property.name | caseSnake | escapeKeyword }}) - raise ArgumentError, "Invalid " + {{ property.name | caseSnake | escapeKeyword }}.to_s + ". Must be one of: " + valid_{{ property.name | caseSnake }}.join(', ') + raise ArgumentError, "Invalid #{{{ property.name | caseSnake | escapeKeyword }}}. Must be one of: #{valid_{{ property.name | caseSnake }}.join(', ')}" end
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
templates/node/src/models/request_model.ts.twig(1 hunks)templates/php/src/Client.php.twig(2 hunks)templates/python/package/models/request_model.py.twig(1 hunks)templates/ruby/lib/container/models/request_model.rb.twig(1 hunks)templates/web/src/models.ts.twig(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- templates/php/src/Client.php.twig
- templates/node/src/models/request_model.ts.twig
⏰ 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: build (8.3, PHP80)
- GitHub Check: build (8.3, Ruby27)
- GitHub Check: build (8.3, WebChromium)
- GitHub Check: build (8.3, Ruby30)
- GitHub Check: build (8.3, WebNode)
- GitHub Check: build (8.3, AppleSwift56)
- GitHub Check: build (8.3, Python311)
- GitHub Check: build (8.3, Android5Java17)
- GitHub Check: build (8.3, KotlinJava17)
- GitHub Check: build (8.3, Node16)
- GitHub Check: build (8.3, KotlinJava8)
- GitHub Check: build (8.3, Go118)
- GitHub Check: build (8.3, Go112)
- GitHub Check: build (8.3, DartStable)
- GitHub Check: build (8.3, FlutterStable)
- GitHub Check: build (8.3, FlutterBeta)
- GitHub Check: build (8.3, Android14Java17)
- GitHub Check: swift (server)
- GitHub Check: android (client)
- GitHub Check: apple (client)
🔇 Additional comments (5)
templates/web/src/models.ts.twig (1)
34-54: LGTM! Past review comment has been fully addressed.The requestModels block now correctly includes both generics support (line 39) and additionalProperties support (lines 47-53), matching the pattern used in the definitions block (lines 13-33). The implementation maintains consistency across both sections, ensuring request models have the same capabilities as response models.
templates/python/package/models/request_model.py.twig (1)
8-30: LGTM!The class structure, initialization, and serialization logic are well-implemented with proper type hints, nil checks for nested objects, and correct handling of enums and arrays.
templates/ruby/lib/container/models/request_model.rb.twig (3)
1-1: LGTM!The frozen string literal comment now has the correct syntax with a space after
#.
3-27: LGTM!The module structure, attr_readers, and initialization logic are well-implemented. The conditional enum validation correctly handles both required and optional properties.
38-63: LGTM!The
to_jsondelegation, conditional private section, and enum validation methods are correctly implemented with proper error messages.
2 participants
What does this PR do?
(Provide a description of what this PR does.)
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.)
Related PRs and Issues
(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)
Have you read the Contributing Guidelines on issues?
(Write your answer here.)
Summary by CodeRabbit
Release Notes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.