feat: add enum support to response models in ts #1198
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. WalkthroughThis change adds comprehensive enum support across SDK generators and templates. The Spec layer splits enums into requestEnums, responseEnums, and allEnums; Swagger2 collects both, and SDK passes them to templates. getSubSchema now accepts methodName and resolves enum type names. Language generators and templates (Web/React Native/Node/Dart/Kotlin/Swift/.NET/Ruby/Python) update imports, type resolution, and (de)serialization to handle enums. DotNet adds Twig functions for type and property naming. Tests/spec.json gains a required enum field status. Deno generation and CI entries are removed. Swift and Apple package templates switch to allEnums-based conditions. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)src/SDK/Language/Kotlin.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). (15)
🔇 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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR adds enum support to response models in TypeScript by modifying the code generation to properly handle enum properties when generating TypeScript type definitions.
- Extracts enum values from OpenAPI/Swagger definitions
- Generates TypeScript union types for enum properties
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Spec/Swagger2.php | Adds logic to extract enum values from property definitions |
| src/SDK/Language/Web.php | Implements TypeScript union type generation for enum properties |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/SDK/Language/Web.php(1 hunks)src/Spec/Swagger2.php(1 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). (12)
- GitHub Check: build (8.3, WebNode)
- GitHub Check: build (8.3, Ruby30)
- GitHub Check: build (8.3, WebChromium)
- GitHub Check: build (8.3, PHP80)
- GitHub Check: build (8.3, AppleSwift56)
- GitHub Check: build (8.3, Swift56)
- GitHub Check: build (8.3, KotlinJava11)
- GitHub Check: build (8.3, PHP83)
- GitHub Check: build (8.3, KotlinJava8)
- GitHub Check: build (8.3, FlutterBeta)
- GitHub Check: build (8.3, Android5Java17)
- GitHub Check: build (8.3, Android14Java17)
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
src/Spec/Swagger2.php (1)
509-532: Fix duplicate-check in getRequestEnumsUsing
in_array($enumName, $list)is ineffective since$listcontains arrays. Use key existence instead and default keys to[].Apply this diff:
- if (isset($parameter['enumValues']) && !\in_array($enumName, $list)) { + if (isset($parameter['enumValues']) && !isset($list[$enumName])) { $list[$enumName] = [ 'name' => $enumName, 'enum' => $parameter['enumValues'], - 'keys' => $parameter['enumKeys'], + 'keys' => $parameter['enumKeys'] ?? [], ]; }src/SDK/Language/Kotlin.php (1)
521-524: Read normalized enumName key in property type resolutionTemplates and spec now normalize to
enumName. Use it instead ofx-enum-name.Apply this diff:
- } elseif (isset($property['enum'])) { - $enumName = $property['x-enum-name'] ?? $property['name']; - $type = \ucfirst($enumName); + } elseif (isset($property['enum'])) { + $enumName = $property['enumName'] ?? $property['name']; + $type = \ucfirst($enumName);templates/android/library/src/main/java/io/package/models/Model.kt.twig (2)
5-10: Use normalized enumName for importsKeep templates aligned with spec (
enumName) to avoid missing imports.Apply this diff:
-{%~ if property.enum %} -{%~ set enumName = property['x-enum-name'] ?? property.name %} -import {{ sdk.namespace | caseDot }}.enums.{{ enumName | caseUcfirst }} +{%~ if property.enum %} +{%~ set enumName = property.enumName ?? property.name %} +import {{ sdk.namespace | caseDot }}.enums.{{ enumName | caseUcfirst }} {%~ endif %}
70-70: Use normalized enumName in from() constructorPrevents mismatches when
x-enum-nameisn’t present at generation time.Apply this diff:
-{% set enumName = property['x-enum-name'] ?? property.name %}{{ enumName | caseUcfirst }}.values().find { it.value == map["{{ property.name | escapeDollarSign }}"] as String }{% if property.required %}!!{% else %} ?: null{% endif %} +{% set enumName = property.enumName ?? property.name %}{{ enumName | caseUcfirst }}.values().find { it.value == map["{{ property.name | escapeDollarSign }}"] as String }{% if property.required %}!!{% else %} ?: null{% endif %}templates/kotlin/src/main/kotlin/io/appwrite/models/Model.kt.twig (2)
5-10: Use normalized enumName for importsAlign with spec normalization and Kotlin generator.
Apply this diff:
-{%~ if property.enum %} -{%~ set enumName = property['x-enum-name'] ?? property.name %} -import {{ sdk.namespace | caseDot }}.enums.{{ enumName | caseUcfirst }} +{%~ if property.enum %} +{%~ set enumName = property.enumName ?? property.name %} +import {{ sdk.namespace | caseDot }}.enums.{{ enumName | caseUcfirst }} {%~ endif %}
70-70: Use normalized enumName in from() constructorPrevents reliance on raw
x-keys in templates.Apply this diff:
-{% set enumName = property['x-enum-name'] ?? property.name %}{{ enumName | caseUcfirst }}.values().find { it.value == map["{{ property.name | escapeDollarSign }}"] as String }{% if property.required %}!!{% else %} ?: null{% endif %} +{% set enumName = property.enumName ?? property.name %}{{ enumName | caseUcfirst }}.values().find { it.value == map["{{ property.name | escapeDollarSign }}"] as String }{% if property.required %}!!{% else %} ?: null{% endif %}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/SDK/Language/Kotlin.php(1 hunks)src/SDK/Language/Web.php(3 hunks)src/Spec/Swagger2.php(3 hunks)templates/android/library/src/main/java/io/package/models/Model.kt.twig(3 hunks)templates/kotlin/src/main/kotlin/io/appwrite/models/Model.kt.twig(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/SDK/Language/Web.php (1)
src/SDK/Language.php (1)
toPascalCase(96-99)
src/Spec/Swagger2.php (1)
src/Spec/Spec.php (3)
getRequestEnums(185-185)getResponseEnums(192-192)getDefinitions(123-123)
⏰ 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, Swift56)
- GitHub Check: build (8.3, WebNode)
- GitHub Check: build (8.3, WebChromium)
- GitHub Check: build (8.3, AppleSwift56)
- GitHub Check: build (8.3, Ruby30)
- GitHub Check: build (8.3, Ruby31)
- GitHub Check: build (8.3, KotlinJava17)
- GitHub Check: build (8.3, Python313)
- GitHub Check: build (8.3, Android14Java17)
- GitHub Check: build (8.3, Python311)
- GitHub Check: build (8.3, PHP83)
- GitHub Check: build (8.3, Go112)
- GitHub Check: build (8.3, Python310)
- GitHub Check: build (8.3, FlutterBeta)
- GitHub Check: build (8.3, KotlinJava11)
- GitHub Check: build (8.3, FlutterStable)
- GitHub Check: build (8.3, DartStable)
- GitHub Check: build (8.3, Android5Java17)
🔇 Additional comments (3)
src/Spec/Swagger2.php (1)
492-499: Also propagate array item enums (items.enum) and carry enum metadataArray-of-enum properties aren’t handled. Mirror parameter metadata on
itemsfor parity.Apply this diff below the existing enum block:
if (isset($def['enum'])) { // enum property $sch['properties'][$name]['enum'] = $def['enum']; $sch['properties'][$name]['enumName'] = $def['x-enum-name'] ?? null; $sch['properties'][$name]['enumKeys'] = $def['x-enum-keys'] ?? []; // Back-compat aliases expected by some generators/templates if (!isset($sch['properties'][$name]['x-enum-name']) && isset($sch['properties'][$name]['enumName'])) { $sch['properties'][$name]['x-enum-name'] = $sch['properties'][$name]['enumName']; } if (!isset($sch['properties'][$name]['x-enum-keys']) && isset($sch['properties'][$name]['enumKeys'])) { $sch['properties'][$name]['x-enum-keys'] = $sch['properties'][$name]['enumKeys']; } } + // array of enum items + if ((($def['type'] ?? null) === 'array') && isset($def['items']['enum'])) { + $sch['properties'][$name]['items']['enum'] = $def['items']['enum']; + $sch['properties'][$name]['items']['enumName'] = $def['items']['x-enum-name'] ?? null; + $sch['properties'][$name]['items']['enumKeys'] = $def['items']['x-enum-keys'] ?? []; + // Back-compat aliases + if (!isset($sch['properties'][$name]['items']['x-enum-name']) && isset($sch['properties'][$name]['items']['enumName'])) { + $sch['properties'][$name]['items']['x-enum-name'] = $sch['properties'][$name]['items']['enumName']; + } + if (!isset($sch['properties'][$name]['items']['x-enum-keys']) && isset($sch['properties'][$name]['items']['enumKeys'])) { + $sch['properties'][$name]['items']['x-enum-keys'] = $sch['properties'][$name]['items']['enumKeys']; + } + }src/SDK/Language/Web.php (2)
359-361: Filter wiring looks goodRegistering getSubSchema with the new signature is correct.
319-347: Add array-of-enum support and prefer normalized enumNameVerified templates pass
definition.nameas the 3rd arg togetSubSchema— safe to use$methodNamefallback. Apply the patch below.public function getSubSchema(array $property, array $spec, string $methodName = ''): string { if (array_key_exists('sub_schema', $property)) { $ret = ''; $generics = []; $this->populateGenerics($property['sub_schema'], $spec, $generics); $generics = array_filter($generics, fn ($model) => $model != $this->toPascalCase($property['sub_schema'])); $ret .= $this->toPascalCase($property['sub_schema']); if (!empty($generics)) { $ret .= '<' . implode(', ', $generics) . '>'; } if ($property['type'] === 'array') { $ret .= '[]'; } return $ret; } + // Array of enum items → EnumName[] + if ((($property['type'] ?? null) === 'array') && isset($property['items']['enum']) && !empty($methodName)) { + $itemEnumName = $property['items']['enumName'] ?? ($property['items']['x-enum-name'] ?? null); + if (!empty($itemEnumName)) { + return $this->toPascalCase($itemEnumName) . '[]'; + } + $propName = $property['name'] ?? ''; + return $this->toPascalCase($methodName) . $this->toPascalCase($propName) . '[]'; + } - if (array_key_exists('enum', $property) && !empty($methodName)) { - if (isset($property['enumName'])) { - return $this->toPascalCase($property['enumName']); - } - - return $this->toPascalCase($methodName) . $this->toPascalCase($property['name']); - } + if (array_key_exists('enum', $property) && !empty($methodName)) { + if (isset($property['enumName'])) { + return $this->toPascalCase($property['enumName']); + } + $propName = $property['name'] ?? ''; + return $this->toPascalCase($methodName) . $this->toPascalCase($propName); + }
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/SDK/Language/Kotlin.php (2)
109-114: Use consistent casing for enum types; avoid hard-coded namespace.Prefer toPascalCase for enum class names. Also, the hard-coded 'io.appwrite.enums.' may break if sdk.namespace changes.
Apply:
- if (isset($parameter['enumName'])) { - return 'io.appwrite.enums.' . \ucfirst($parameter['enumName']); - } - if (!empty($parameter['enumValues'])) { - return 'io.appwrite.enums.' . \ucfirst($parameter['name']); - } + if (isset($parameter['enumName'])) { + return 'io.appwrite.enums.' . $this->toPascalCase($parameter['enumName']); + } + if (!empty($parameter['enumValues'])) { + return 'io.appwrite.enums.' . $this->toPascalCase($parameter['name']); + }If feasible, consider passing namespace into this path (like getReturnType) to avoid hard-coding. Please confirm sdk.namespace is always io.appwrite for Kotlin generation.
524-527: Enum property typing: guard empties and unify casing.Handle empty enums and use toPascalCase for consistency.
- } elseif (isset($property['enum'])) { - $enumName = $property['x-enum-name'] ?? $property['name']; - $type = \ucfirst($enumName); + } elseif (isset($property['enum']) && !empty($property['enum'])) { + $enumName = $property['x-enum-name'] ?? $property['name']; + $type = $this->toPascalCase($enumName);Follow-up: If arrays of enums are possible (items.enum), consider handling that here.
templates/kotlin/src/main/kotlin/io/appwrite/models/Model.kt.twig (2)
5-10: De-duplicate enum imports and keep casing consistent.Multiple properties referencing the same enum will produce duplicate imports. Also ensure consistent PascalCase.
-{%~ for property in definition.properties %} -{%~ if property.enum %} -{%~ set enumName = property['x-enum-name'] ?? property.name %} -import {{ sdk.namespace | caseDot }}.enums.{{ enumName | caseUcfirst }} -{%~ endif %} -{%~ endfor %} +{% set enumImports = [] %} +{% for property in definition.properties if property.enum %} + {% set enumName = property['x-enum-name'] ?? property.name %} + {% set enumImports = enumImports|merge([enumName]) %} +{% endfor %} +{% for enumName in enumImports|unique %} +import {{ sdk.namespace | caseDot }}.enums.{{ enumName | caseUcfirst }} +{% endfor %}If
uniqueis unavailable in your Twig setup, I can provide an alternative.
36-36: Potential crash:as Anyon nullable values.For non-required enum fields,
prop?.value as Anywill cast null to Any and fail at runtime. Consider conditionally adding the pair only when value != null, or building the map imperatively to skip nulls.Want me to propose a safe
buildMap { ... }pattern that avoids null casts without changing the signature?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/SDK/Language/Kotlin.php(3 hunks)templates/android/library/src/main/java/io/package/models/Model.kt.twig(3 hunks)templates/kotlin/src/main/kotlin/io/appwrite/models/Model.kt.twig(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- templates/android/library/src/main/java/io/package/models/Model.kt.twig
🧰 Additional context used
🧬 Code graph analysis (1)
src/SDK/Language/Kotlin.php (2)
src/SDK/Language.php (1)
toPascalCase(96-99)src/SDK/Language/Go.php (1)
hasGenericType(350-371)
⏰ 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: build (8.3, DartStable)
- GitHub Check: build (8.3, Python312)
- GitHub Check: build (8.3, Ruby31)
- GitHub Check: build (8.3, AppleSwift56)
- GitHub Check: build (8.3, Ruby27)
- GitHub Check: build (8.3, Node18)
- GitHub Check: build (8.3, Swift56)
- GitHub Check: build (8.3, KotlinJava8)
- GitHub Check: build (8.3, PHP83)
- GitHub Check: build (8.3, Go118)
- GitHub Check: build (8.3, FlutterBeta)
- GitHub Check: build (8.3, Node16)
- GitHub Check: build (8.3, FlutterStable)
- GitHub Check: build (8.3, KotlinJava11)
- GitHub Check: build (8.3, Android5Java17)
- GitHub Check: build (8.3, Android14Java17)
🔇 Additional comments (2)
src/SDK/Language/Kotlin.php (1)
472-475: New Twig filter registration looks good.Filter name and delegation align with the new method.
templates/kotlin/src/main/kotlin/io/appwrite/models/Model.kt.twig (1)
70-71: from(...) assignment via filter is solid.Delegating to propertyAssignment centralizes null-safety and enum handling.
After applying the Kotlin.php null-safety fixes, please regenerate a model with:
- required enum, optional enum, required/optional nested object and array, and verify compilation.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/SDK/Language/Kotlin.php(3 hunks)src/SDK/Language/Swift.php(1 hunks)src/Spec/Swagger2.php(3 hunks)templates/swift/Sources/Enums/Enum.swift.twig(1 hunks)templates/swift/Sources/Models/Model.swift.twig(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/SDK/Language/Kotlin.php
🧰 Additional context used
🧬 Code graph analysis (1)
src/Spec/Swagger2.php (1)
src/Spec/Spec.php (3)
getRequestEnums(185-185)getResponseEnums(192-192)getDefinitions(123-123)
⏰ 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, WebChromium)
- GitHub Check: build (8.3, Ruby30)
- GitHub Check: build (8.3, Ruby27)
- GitHub Check: build (8.3, Swift56)
- GitHub Check: build (8.3, Ruby31)
- GitHub Check: build (8.3, Python311)
- GitHub Check: build (8.3, WebNode)
- GitHub Check: build (8.3, Node20)
- GitHub Check: build (8.3, PHP80)
- GitHub Check: build (8.3, AppleSwift56)
- GitHub Check: build (8.3, Python313)
- GitHub Check: build (8.3, Python310)
- GitHub Check: build (8.3, KotlinJava11)
- GitHub Check: build (8.3, KotlinJava17)
- GitHub Check: build (8.3, FlutterBeta)
- GitHub Check: build (8.3, FlutterStable)
- GitHub Check: build (8.3, Android14Java17)
- GitHub Check: build (8.3, Android5Java17)
🔇 Additional comments (2)
src/Spec/Swagger2.php (1)
493-498: Array item enums are dropped from response schemasRight now we only copy scalar property enums. If a property is declared as an array whose
itemscontain anenum, that metadata never gets propagated into the definition, so the TS response models still render those arrays asstring[]instead of unions of the enum values. That breaks the “enum support” goal for any array-of-enum response field.Please mirror the request-side handling by carrying the
items.enum(and related metadata) into the schema:if (isset($def['enum'])) { // enum property $sch['properties'][$name]['enum'] = $def['enum']; $sch['properties'][$name]['enumName'] = $def['x-enum-name'] ?? ucfirst($key) . ucfirst($name);; $sch['properties'][$name]['enumKeys'] = $def['x-enum-keys'] ?? []; } + if (($def['type'] ?? null) === 'array' && isset($def['items']['enum'])) { + $sch['properties'][$name]['items'] = $sch['properties'][$name]['items'] ?? []; + $sch['properties'][$name]['items']['enum'] = $def['items']['enum']; + if (isset($def['items']['x-enum-name'])) { + $sch['properties'][$name]['items']['x-enum-name'] = $def['items']['x-enum-name']; + } + if (isset($def['items']['x-enum-keys'])) { + $sch['properties'][$name]['items']['x-enum-keys'] = $def['items']['x-enum-keys']; + } + }This keeps array enums intact so downstream generators can emit the correct typed unions.
templates/swift/Sources/Enums/Enum.swift.twig (1)
3-12: Great to see Codable conformance addedMaking the generated enums
Codableunblocks seamless encoding/decoding in Swift models. Thanks for taking care of this!
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/SDK/Language/Dart.php(1 hunks)templates/dart/lib/models.dart.twig(1 hunks)templates/dart/lib/src/enums/enum.dart.twig(1 hunks)templates/dart/lib/src/models/model.dart.twig(2 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). (15)
- GitHub Check: build (8.3, WebNode)
- GitHub Check: build (8.3, Swift56)
- GitHub Check: build (8.3, Ruby31)
- GitHub Check: build (8.3, WebChromium)
- GitHub Check: build (8.3, Python310)
- GitHub Check: build (8.3, Ruby30)
- GitHub Check: build (8.3, AppleSwift56)
- GitHub Check: build (8.3, Ruby27)
- GitHub Check: build (8.3, Node20)
- GitHub Check: build (8.3, Python39)
- GitHub Check: build (8.3, Python313)
- GitHub Check: build (8.3, Node16)
- GitHub Check: build (8.3, Go112)
- GitHub Check: build (8.3, Android5Java17)
- GitHub Check: build (8.3, Android14Java17)
ChiragAgg5k
force-pushed
the
feat-enum-support
branch
from
df76d9c to
66adb5a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
example.php(1 hunks)src/SDK/Language/Dart.php(1 hunks)templates/dart/lib/enums.dart.twig(1 hunks)templates/dart/lib/models.dart.twig(1 hunks)templates/dart/lib/src/models/model.dart.twig(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- templates/dart/lib/src/models/model.dart.twig
- templates/dart/lib/models.dart.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). (17)
- GitHub Check: build (8.3, Swift56)
- GitHub Check: build (8.3, PHP83)
- GitHub Check: build (8.3, Python310)
- GitHub Check: build (8.3, AppleSwift56)
- GitHub Check: build (8.3, DotNet90)
- GitHub Check: build (8.3, Python313)
- GitHub Check: build (8.3, WebChromium)
- GitHub Check: build (8.3, Node16)
- GitHub Check: build (8.3, KotlinJava11)
- GitHub Check: build (8.3, Android5Java17)
- GitHub Check: build (8.3, FlutterStable)
- GitHub Check: build (8.3, KotlinJava17)
- GitHub Check: build (8.3, CLINode16)
- GitHub Check: build (8.3, Ruby30)
- GitHub Check: build (8.3, KotlinJava8)
- GitHub Check: build (8.3, FlutterBeta)
- GitHub Check: build (8.3, Android14Java17)
🔇 Additional comments (1)
src/SDK/Language/Dart.php (1)
130-135: Fix enum type casing before returning it.Plain
ucfirston snake-cased enum identifiers still producesStatus_code, whileenum.dart.twigdeclares the enum asStatusCode, so the generated SDK will not compile.Apply this diff to normalize the name:
- return \ucfirst($parameter['enumName']); + return \ucfirst($this->toCamelCase($parameter['enumName'])); @@ - return \ucfirst($parameter['name']); + return \ucfirst($this->toCamelCase($parameter['name']));
There was a problem hiding this comment.
Actionable comments posted: 3
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/Swift.php (2)
302-309: Ensure fully-qualified, correctly-cased enum type namesCurrent output can render "Enums.Foo" when spec is missing, or wrong-cased module names, and enum identifiers aren’t pascal-cased. This will break imports against "{{ spec.title | caseUcfirst }}Enums".
Apply:
- if (isset($parameter['enumName'])) { - return ($spec['title'] ?? '') . 'Enums.' . \ucfirst($parameter['enumName']); - } - if (!empty($parameter['enumValues'])) { - return ($spec['title'] ?? '') . 'Enums.' . \ucfirst($parameter['name']); + if (isset($parameter['enumName'])) { + $module = isset($spec['title']) ? \ucfirst($spec['title']) . 'Enums.' : ''; + return $module . $this->toPascalCase($parameter['enumName']); + } + if (!empty($parameter['enumValues'])) { + $module = isset($spec['title']) ? \ucfirst($spec['title']) . 'Enums.' : ''; + return $module . $this->toPascalCase($parameter['name']); }
320-323: Propagate $spec and $isProperty in array item type resolutionWithout passing $spec and $isProperty, arrays of enums render as "Enums.X" (missing module) and arrays of objects degrade to "[Any]" instead of "[[String: AnyCodable]]".
- self::TYPE_ARRAY => (!empty(($parameter['array'] ?? [])['type']) && !\is_array($parameter['array']['type'])) - ? '[' . $this->getTypeName($parameter['array']) . ']' + self::TYPE_ARRAY => (!empty(($parameter['array'] ?? [])['type']) && !\is_array($parameter['array']['type'])) + ? '[' . $this->getTypeName($parameter['array'], $spec, $isProperty) . ']' : '[AnyCodable]',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/SDK/Language/Swift.php(1 hunks)templates/android/library/src/main/java/io/package/models/Model.kt.twig(3 hunks)templates/dart/lib/models.dart.twig(1 hunks)templates/dart/lib/src/models/model.dart.twig(2 hunks)templates/kotlin/src/main/kotlin/io/appwrite/models/Model.kt.twig(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- templates/android/library/src/main/java/io/package/models/Model.kt.twig
- templates/dart/lib/src/models/model.dart.twig
🧰 Additional context used
🧬 Code graph analysis (1)
src/SDK/Language/Swift.php (1)
src/SDK/Language.php (1)
getTypeName(44-44)
⏰ 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)
- GitHub Check: build (8.3, Ruby27)
- GitHub Check: build (8.3, Ruby30)
- GitHub Check: build (8.3, WebChromium)
- GitHub Check: build (8.3, Python312)
- GitHub Check: build (8.3, AppleSwift56)
- GitHub Check: build (8.3, Python39)
- GitHub Check: build (8.3, Python311)
- GitHub Check: build (8.3, Go118)
- GitHub Check: build (8.3, KotlinJava11)
- GitHub Check: build (8.3, PHP80)
- GitHub Check: build (8.3, PHP83)
- GitHub Check: build (8.3, KotlinJava17)
- GitHub Check: build (8.3, FlutterBeta)
- GitHub Check: build (8.3, Node18)
- GitHub Check: build (8.3, FlutterStable)
- GitHub Check: build (8.3, DotNet60)
- GitHub Check: build (8.3, DartStable)
- GitHub Check: build (8.3, Android5Java17)
- GitHub Check: build (8.3, Android14Java17)
🔇 Additional comments (1)
src/SDK/Language/Swift.php (1)
580-581: Approve fix; no remaining named-argumentisProperty:calls found
Confirmed zero occurrences ofgetTypeName(..., isProperty:across the codebase.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Spec/Swagger2.php (1)
519-525: Fix duplicate-check and default keys in getRequestEnums()Current in_array() on an array of arrays is ineffective. Use isset() and default keys to [].
- if (isset($parameter['enumValues']) && !\in_array($enumName, $list)) { + if (isset($parameter['enumValues']) && !isset($list[$enumName])) { $list[$enumName] = [ 'name' => $enumName, 'enum' => $parameter['enumValues'], - 'keys' => $parameter['enumKeys'], + 'keys' => $parameter['enumKeys'] ?? [], ]; }
🧹 Nitpick comments (1)
templates/kotlin/src/main/kotlin/io/appwrite/models/Model.kt.twig (1)
5-9: Import enum types for array-of-enum properties as wellEnsure enums used in List are imported.
-{%~ for property in definition.properties %} -{%~ if property.enum %} -import {{ sdk.namespace | caseDot }}.enums.{{ (property.enumName ?? property.name) | caseUcfirst }} -{%~ endif %} -{%~ endfor %} +{%~ for property in definition.properties %} + {%~ if property.enum %} +import {{ sdk.namespace | caseDot }}.enums.{{ (property.enumName ?? property.name) | caseUcfirst }} + {%~ elseif property.type == 'array' and property.items and property.items.enum %} +import {{ sdk.namespace | caseDot }}.enums.{{ ((property.items.enumName ?? (property.name ~ 'Item'))) | caseUcfirst }} + {%~ endif %} +{%~ endfor %}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Spec/Swagger2.php(3 hunks)templates/dotnet/Package/Models/Model.cs.twig(3 hunks)templates/kotlin/src/main/kotlin/io/appwrite/models/Model.kt.twig(3 hunks)templates/swift/Sources/Models/Model.swift.twig(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- templates/swift/Sources/Models/Model.swift.twig
🧰 Additional context used
🧬 Code graph analysis (1)
src/Spec/Swagger2.php (1)
src/Spec/Spec.php (3)
getRequestEnums(185-185)getResponseEnums(192-192)getDefinitions(123-123)
⏰ 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: build (8.3, AppleSwift56)
- GitHub Check: build (8.3, FlutterBeta)
- GitHub Check: build (8.3, Android5Java17)
- GitHub Check: build (8.3, Android14Java17)
🔇 Additional comments (2)
templates/kotlin/src/main/kotlin/io/appwrite/models/Model.kt.twig (1)
35-35: Serialize arrays-of-enums to list of .valueCurrent enum branch uses .value even when the property is a list.
- "{{ property.name | escapeDollarSign }}" to {% if property.sub_schema %}{% if property.type == 'array' %}{{property.name | escapeKeyword | removeDollarSign}}.map { it.toMap() }{% else %}{{property.name | escapeKeyword | removeDollarSign}}.toMap(){% endif %}{% elseif property.enum %}{{property.name | escapeKeyword | removeDollarSign}}{% if not property.required %}?{% endif %}.value{% else %}{{property.name | escapeKeyword | removeDollarSign}}{% endif %} as Any, + "{{ property.name | escapeDollarSign }}" to {% if property.sub_schema %}{% if property.type == 'array' %}{{property.name | escapeKeyword | removeDollarSign}}.map { it.toMap() }{% else %}{{property.name | escapeKeyword | removeDollarSign}}.toMap(){% endif %}{% elseif property.enum %}{% if property.type == 'array' %}{{property.name | escapeKeyword | removeDollarSign}}{% if not property.required %}?{% endif %}.map { it.value }{% else %}{{property.name | escapeKeyword | removeDollarSign}}{% if not property.required %}?{% endif %}.value{% endif %}{% else %}{{property.name | escapeKeyword | removeDollarSign}}{% endif %} as Any,src/Spec/Swagger2.php (1)
493-498: Support array-item enums and mirror enum metadata for downstream parityAlso align fallback naming with clearer pattern and populate back-compat x-enum-* keys.
Apply this diff:
- if (isset($def['enum'])) { - // enum property - $sch['properties'][$name]['enum'] = $def['enum']; - $sch['properties'][$name]['enumName'] = $def['x-enum-name'] ?? ucfirst($key) . ucfirst($name); - $sch['properties'][$name]['enumKeys'] = $def['x-enum-keys'] ?? []; - } + // enums on property or on array items + if (isset($def['enum'])) { + $sch['properties'][$name]['enum'] = $def['enum']; + $enumName = $def['x-enum-name'] ?? (ucfirst($key) . 'Property' . ucfirst($name)); + $enumKeys = $def['x-enum-keys'] ?? []; + $sch['properties'][$name]['enumName'] = $enumName; + $sch['properties'][$name]['enumKeys'] = $enumKeys; + // back-compat aliases expected by some generators/templates + $sch['properties'][$name]['x-enum-name'] = $enumName; + $sch['properties'][$name]['x-enum-keys'] = $enumKeys; + } + if ((($def['type'] ?? null) === 'array') && isset($def['items']['enum'])) { + $sch['properties'][$name]['items']['enum'] = $def['items']['enum']; + $itemEnumName = $def['items']['x-enum-name'] ?? (ucfirst($key) . 'Property' . ucfirst($name) . 'Item'); + $itemEnumKeys = $def['items']['x-enum-keys'] ?? []; + $sch['properties'][$name]['items']['enumName'] = $itemEnumName; + $sch['properties'][$name]['items']['enumKeys'] = $itemEnumKeys; + $sch['properties'][$name]['items']['x-enum-name'] = $itemEnumName; + $sch['properties'][$name]['items']['x-enum-keys'] = $itemEnumKeys; + }
| @@ -1,11 +1,12 @@ | |||
| {% macro sub_schema(property) %}{% if property.sub_schema %}{% if property.type == 'array' %}List<{{property.sub_schema | caseUcfirst | overrideIdentifier}}>{% else %}{{property.sub_schema | caseUcfirst | overrideIdentifier}}{% endif %}{% else %}{{property | typeName}}{% endif %}{% if not property.required %}?{% endif %}{% endmacro %} | |||
| {% macro sub_schema(property) %}{% if property.sub_schema %}{% if property.type == 'array' %}List<{{property.sub_schema | caseUcfirst | overrideIdentifier}}>{% else %}{{property.sub_schema | caseUcfirst | overrideIdentifier}}{% endif %}{% elseif property.enum %}{% set enumName = property['enumName'] ?? property.name %}{{ enumName | caseUcfirst }}{% else %}{{property | typeName}}{% endif %}{% if not property.required %}?{% endif %}{% endmacro %} | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Type arrays-of-enums as List in sub_schema()
Current branch emits EnumType even when the property is an array.
-{% macro sub_schema(property) %}{% if property.sub_schema %}{% if property.type == 'array' %}List<{{property.sub_schema | caseUcfirst | overrideIdentifier}}>{% else %}{{property.sub_schema | caseUcfirst | overrideIdentifier}}{% endif %}{% elseif property.enum %}{% set enumName = property['enumName'] ?? property.name %}{{ enumName | caseUcfirst }}{% else %}{{property | typeName}}{% endif %}{% if not property.required %}?{% endif %}{% endmacro %}
+{% macro sub_schema(property) -%}
+{%- if property.sub_schema -%}
+ {%- if property.type == 'array' -%}List<{{property.sub_schema | caseUcfirst | overrideIdentifier}}>{%- else -%}{{property.sub_schema | caseUcfirst | overrideIdentifier}}{%- endif -%}
+{%- elseif property.type == 'array' and property.items and property.items.enum -%}
+ {%- set enumName = property.items['enumName'] ?? (property.name ~ 'Item') -%}List<{{ enumName | caseUcfirst }}>
+{%- elseif property.enum -%}
+ {%- set enumName = property['enumName'] ?? property.name -%}{{ enumName | caseUcfirst }}
+{%- else -%}
+ {{property | typeName}}
+{%- endif -%}{% if not property.required %}?{% endif %}
+{%- endmacro %}📝 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.
| {% macro sub_schema(property) %}{% if property.sub_schema %}{% if property.type == 'array' %}List<{{property.sub_schema | caseUcfirst | overrideIdentifier}}>{% else %}{{property.sub_schema | caseUcfirst | overrideIdentifier}}{% endif %}{% elseif property.enum %}{% set enumName = property['enumName'] ?? property.name %}{{ enumName | caseUcfirst }}{% else %}{{property | typeName}}{% endif %}{% if not property.required %}?{% endif %}{% endmacro %} | |
| {% macro sub_schema(property) -%} | |
| {%- if property.sub_schema -%} | |
| {%- if property.type == 'array' -%} | |
| List<{{ property.sub_schema | caseUcfirst | overrideIdentifier }}> | |
| {%- else -%} | |
| {{ property.sub_schema | caseUcfirst | overrideIdentifier }} | |
| {%- endif -%} | |
| {%- elseif property.type == 'array' and property.items and property.items.enum -%} | |
| {%- set enumName = property.items['enumName'] ?? (property.name ~ 'Item') -%} | |
| List<{{ enumName | caseUcfirst }}> | |
| {%- elseif property.enum -%} | |
| {%- set enumName = property['enumName'] ?? property.name -%} | |
| {{ enumName | caseUcfirst }} | |
| {%- else -%} | |
| {{ property | typeName }} | |
| {%- endif -%} | |
| {% if not property.required %}?{% endif %} | |
| {%- endmacro %} |
🤖 Prompt for AI Agents
In templates/dotnet/Package/Models/Model.cs.twig around lines 1 to 1, the
sub_schema() macro treats array sub-schemas the same as object sub-schemas and
emits the raw sub_schema identifier even when that sub-schema is an enum,
causing arrays-of-enums to be typed incorrectly; modify the macro so that when
property.sub_schema exists and property.type == 'array' you inspect
property.sub_schema to see if it represents an enum (e.g.
property.sub_schema.enum or property.sub_schema.enumName), derive the enum type
name (fallback to sub_schema.name if needed), and emit List<EnumType> (applying
caseUcfirst and overrideIdentifier as before); otherwise keep the existing
behavior for object/complex sub-schemas and preserve the nullable suffix when
property.required is false.
There was a problem hiding this comment.
@ChiragAgg5k Comment is legit, let's check that "array of enums" is handled for all langs
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| /** | ||
| * Generate property assignment logic for model deserialization | ||
| * | ||
| * @param array $property | ||
| * @param array $spec | ||
| * @return string | ||
| */ | ||
| protected function getPropertyAssignment(array $property, array $spec): string |
There was a problem hiding this comment.
I really like this shift of the complex templating logic into PHP, can we do the same for the other languages changed so far? To keep consistency, and improve readability of their templates too
| @@ -1,11 +1,12 @@ | |||
| {% macro sub_schema(property) %}{% if property.sub_schema %}{% if property.type == 'array' %}List<{{property.sub_schema | caseUcfirst | overrideIdentifier}}>{% else %}{{property.sub_schema | caseUcfirst | overrideIdentifier}}{% endif %}{% else %}{{property | typeName}}{% endif %}{% if not property.required %}?{% endif %}{% endmacro %} | |||
| {% macro sub_schema(property) %}{% if property.sub_schema %}{% if property.type == 'array' %}List<{{property.sub_schema | caseUcfirst | overrideIdentifier}}>{% else %}{{property.sub_schema | caseUcfirst | overrideIdentifier}}{% endif %}{% elseif property.enum %}{% set enumName = property['enumName'] ?? property.name %}{{ enumName | caseUcfirst }}{% else %}{{property | typeName}}{% endif %}{% if not property.required %}?{% endif %}{% endmacro %} | |||
There was a problem hiding this comment.
@ChiragAgg5k Comment is legit, let's check that "array of enums" is handled for all langs
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
templates/swift/Sources/Models/Model.swift.twig (1)
87-87: Avoid leaking Optional values into [String: Any] in toMap().Emitting optionals with “as Any” can produce Optional(...) wrappers and break JSONSerialization. Prefer building a [String: Any?] then compactMapValues to drop nils.
Apply this diff to toMap():
- public func toMap() -> [String: Any] { - return [ + public func toMap() -> [String: Any] { + let map: [String: Any?] = [ {%~ for property in definition.properties %} - "{{ property.name }}": {% if property.sub_schema %}{% if property.type == 'array' %}{{property.name | escapeSwiftKeyword | removeDollarSign}}.map { $0.toMap() }{% else %}{{property.name | escapeSwiftKeyword | removeDollarSign}}.toMap(){% endif %}{% elseif property.enum %}{{property.name | escapeSwiftKeyword | removeDollarSign}}{% if not property.required %}?{% endif %}.rawValue{% else %}{{property.name | escapeSwiftKeyword | removeDollarSign}}{% endif %} as Any{% if not loop.last or (loop.last and definition.additionalProperties) %},{% endif %} + "{{ property.name }}": {% if property.sub_schema %}{% if property.type == 'array' %}{{property.name | escapeSwiftKeyword | removeDollarSign}}.map { $0.toMap() }{% else %}{{property.name | escapeSwiftKeyword | removeDollarSign}}.toMap(){% endif %}{% elseif property.enum %}{{property.name | escapeSwiftKeyword | removeDollarSign}}{% if not property.required %}?{% endif %}.rawValue{% else %}{{property.name | escapeSwiftKeyword | removeDollarSign}}{% endif %}{% if not loop.last or (loop.last and definition.additionalProperties) %},{% endif %} {%~ endfor %} {%~ if definition.additionalProperties %} "data": try! JSONEncoder().encode(data) {%~ endif %} - ] + ] + return map.compactMapValues { $0 } }templates/dart/test/src/models/model_test.dart.twig (1)
20-20: Use overrideIdentifier for enum type to match generated identifiers.Enum types can include reserved/invalid chars; apply the same normalization used when generating enums to avoid reference mismatches.
Apply this diff:
- {{ property.name | escapeKeyword }}: {% if property.type == 'array' %}[]{% elseif property.type == 'object' and (property.sub_schema == 'prefs' or property.sub_schema == 'preferences') %}Preferences(data: {}){% elseif property.type == 'object' and property.sub_schema %}{{_self.sub_schema(spec.definitions, property)}}{% elseif property.type == 'object' %}{}{% elseif property.enum %}{{ property.enumName | caseUcfirst }}.{{ (property.enumKeys[0] ?? property.enum[0]) | caseEnumKey }}{% elseif property.type == 'string' %}'{{property['x-example'] | escapeDollarSign}}'{% elseif property.type == 'boolean' %}true{% else %}{{property['x-example']}}{% endif %}, + {{ property.name | escapeKeyword }}: {% if property.type == 'array' %}[]{% elseif property.type == 'object' and (property.sub_schema == 'prefs' or property.sub_schema == 'preferences') %}Preferences(data: {}){% elseif property.type == 'object' and property.sub_schema %}{{_self.sub_schema(spec.definitions, property)}}{% elseif property.type == 'object' %}{}{% elseif property.enum %}{{ property.enumName | caseUcfirst | overrideIdentifier }}.{{ (property.enumKeys[0] ?? property.enum[0]) | caseEnumKey }}{% elseif property.type == 'string' %}'{{property['x-example'] | escapeDollarSign}}'{% elseif property.type == 'boolean' %}true{% else %}{{property['x-example']}}{% endif %},src/Spec/Swagger2.php (1)
509-532: Fix duplicate-check in getRequestEnums and harden keys default.Use isset on associative array to avoid wasted reassignments; default keys to [].
Apply this diff:
- $enumName = $parameter['enumName'] ?? $parameter['name']; - - if (isset($parameter['enumValues']) && !\in_array($enumName, $list)) { + $enumName = $parameter['enumName'] ?? $parameter['name']; + if (isset($parameter['enumValues']) && !isset($list[$enumName])) { $list[$enumName] = [ 'name' => $enumName, 'enum' => $parameter['enumValues'], - 'keys' => $parameter['enumKeys'], + 'keys' => $parameter['enumKeys'] ?? [], ]; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Spec/Swagger2.php(3 hunks)templates/dart/test/src/models/model_test.dart.twig(3 hunks)templates/swift/Sources/Models/Model.swift.twig(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Spec/Swagger2.php (1)
src/Spec/Spec.php (3)
getRequestEnums(185-185)getResponseEnums(192-192)getDefinitions(123-123)
⏰ 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). (9)
- GitHub Check: build (8.3, WebNode)
- GitHub Check: build (8.3, AppleSwift56)
- GitHub Check: build (8.3, Swift56)
- GitHub Check: build (8.3, WebChromium)
- GitHub Check: build (8.3, Ruby27)
- GitHub Check: build (8.3, Python311)
- GitHub Check: build (8.3, KotlinJava11)
- GitHub Check: build (8.3, FlutterStable)
- GitHub Check: build (8.3, Android5Java17)
🔇 Additional comments (7)
templates/swift/Sources/Models/Model.swift.twig (3)
3-5: Conditional enum import looks good.Nice: import only when enums exist, avoiding redundant imports per model.
Please confirm the Swift target defining the Enums is named "{{spec.title | caseUcfirst}}Enums" for all Swift package/Xcode build settings so this import resolves across platforms. If not, consider gating the import by platform or adjusting the module name.
77-77: Encoding enums via rawValue is correct.The encode/encodeIfPresent with .rawValue aligns with Codable expectations.
109-112: Still force‑unwrapping optional enums in from(map:); this will crash.Optional enum properties must not be force‑unwrapped. Use a safe mapping for optionals. This was flagged earlier and remains.
Apply this diff:
- {%- set enumName = property['enumName'] ?? property.name -%} - {{ enumName | caseUcfirst }}(rawValue: map["{{property.name }}"] as{% if isDocument %}?{% else %}{% if property.required %}!{% else %}?{% endif %}{% endif %} String{% if isDocument and property.required %} ?? ""{% endif %}){% if not property.required %}{% if not isDocument %}{% endif %}{% endif %}! + {%- set enumName = property['enumName'] ?? property.name -%} + {% if property.required %} + {{ enumName | caseUcfirst }}(rawValue: map["{{property.name }}"] as{% if isDocument %}?{% else %}!{% endif %} String{% if isDocument and property.required %} ?? ""{% endif %})! + {% else %} + (map["{{property.name }}"] as? String).flatMap { {{ enumName | caseUcfirst }}(rawValue: $0) } + {% endif %}Run to find any other remaining force‑unwraps of enum rawValue constructors:
#!/bin/bash # Find force-unwraps of enum(rawValue:) in Swift templates rg -nP --type=twig -C2 '\(rawValue:\s*[^)]*\)\s*!' templates/swifttemplates/dart/test/src/models/model_test.dart.twig (1)
6-8: LGTM: Conditional import for enums is correct.Imports enums.dart only when enum properties exist; aligns with usage in this test template.
src/Spec/Swagger2.php (3)
493-498: Unify enum metadata keys and improve fallback names; also carry item-enum metadata.Populate both enumName/x-enum-name and enumKeys/x-enum-keys for downstream templates; use clearer fallback “ModelPropertyPropertyName”; propagate enum metadata for array items.
Apply this diff:
- if (isset($def['enum'])) { - // enum property - $sch['properties'][$name]['enum'] = $def['enum']; - $sch['properties'][$name]['enumName'] = $def['x-enum-name'] ?? ucfirst($key) . ucfirst($name); - $sch['properties'][$name]['enumKeys'] = $def['x-enum-keys'] ?? []; - } + if (isset($def['enum'])) { + // enum property + $sch['properties'][$name]['enum'] = $def['enum']; + $sch['properties'][$name]['enumName'] = $def['x-enum-name'] ?? (ucfirst($key) . 'Property' . ucfirst($name)); + $sch['properties'][$name]['enumKeys'] = $def['x-enum-keys'] ?? []; + // back-compat aliases + if (!isset($sch['properties'][$name]['x-enum-name']) && isset($sch['properties'][$name]['enumName'])) { + $sch['properties'][$name]['x-enum-name'] = $sch['properties'][$name]['enumName']; + } + if (!isset($sch['properties'][$name]['x-enum-keys']) && isset($sch['properties'][$name]['enumKeys'])) { + $sch['properties'][$name]['x-enum-keys'] = $sch['properties'][$name]['enumKeys']; + } + } + // array of enum items + if ((($def['type'] ?? null) === 'array') && isset($def['items']['enum'])) { + $sch['properties'][$name]['items']['enum'] = $def['items']['enum']; + $sch['properties'][$name]['items']['enumName'] = $def['items']['x-enum-name'] ?? (ucfirst($key) . 'Property' . ucfirst($name) . 'Item'); + $sch['properties'][$name]['items']['enumKeys'] = $def['items']['x-enum-keys'] ?? []; + if (!isset($sch['properties'][$name]['items']['x-enum-name']) && isset($sch['properties'][$name]['items']['enumName'])) { + $sch['properties'][$name]['items']['x-enum-name'] = $sch['properties'][$name]['items']['enumName']; + } + if (!isset($sch['properties'][$name]['items']['x-enum-keys']) && isset($sch['properties'][$name]['items']['enumKeys'])) { + $sch['properties'][$name]['items']['x-enum-keys'] = $sch['properties'][$name]['items']['enumKeys']; + } + }Based on past review comments
537-572: Use normalized enumName/keys and proper duplicate-check; correct array-item handling and naming.Read enumName/enumKeys (not x- variants), add “Property” (and “Item” for arrays) to fallbacks, prefer items’ enumName for arrays.
Apply this diff:
- if (isset($property['enum'])) { - $enumName = $property['x-enum-name'] ?? ucfirst($modelName) . ucfirst($propertyName); - if (!isset($list[$enumName])) { - $list[$enumName] = [ - 'name' => $enumName, - 'enum' => $property['enum'], - 'keys' => $property['x-enum-keys'] ?? [], - ]; - } - } + if (isset($property['enum'])) { + $enumName = $property['enumName'] ?? (ucfirst($modelName) . 'Property' . ucfirst($propertyName)); + if (!isset($list[$enumName])) { + $list[$enumName] = [ + 'name' => $enumName, + 'enum' => $property['enum'], + 'keys' => $property['enumKeys'] ?? [], + ]; + } + } @@ - // array of enums - if ((($property['type'] ?? null) === 'array') && isset($property['items']['enum'])) { - $enumName = $property['x-enum-name'] ?? ucfirst($modelName) . ucfirst($propertyName); - if (!isset($list[$enumName])) { - $list[$enumName] = [ - 'name' => $enumName, - 'enum' => $property['items']['enum'], - 'keys' => $property['items']['x-enum-keys'] ?? [], - ]; - } - } + // array of enums + if ((($property['type'] ?? null) === 'array') && isset($property['items']['enum'])) { + $itemEnumName = $property['items']['enumName'] ?? (ucfirst($modelName) . 'Property' . ucfirst($propertyName) . 'Item'); + if (!isset($list[$itemEnumName])) { + $list[$itemEnumName] = [ + 'name' => $itemEnumName, + 'enum' => $property['items']['enum'], + 'keys' => $property['items']['enumKeys'] ?? [], + ]; + } + }Based on past review comments
509-532: AllgetEnums()call sites andx-enum-*references have been removed or updated. No remaining uses ofgetEnums()orx-enum-name/x-enum-keyswere found.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
templates/dotnet/Package/Models/Model.cs.twig (2)
62-67: Fix optional number/boolean deserialization and JsonElement handlingIndexing map["..."] for optional numbers/booleans throws when the key is absent. Also, Convert.ToX on JsonElement fails. Use TryGetValue and unwrap JsonElement.
- {%- if property.type == "integer" or property.type == "number" %} - {%- if not property.required -%}map["{{ property.name }}"] == null ? null :{% endif %}Convert.To{% if property.type == "integer" %}Int64{% else %}Double{% endif %}(map["{{ property.name }}"]) + {%- if property.type == "integer" or property.type == "number" %} + {%- if not property.required -%} + map.TryGetValue("{{ property.name }}", out var numRaw{{ loop.index }}) + ? (numRaw{{ loop.index }} == null + ? null + : (numRaw{{ loop.index }} is JsonElement jsonNum{{ loop.index }} + ? ({{ property.type == "integer" ? "long" : "double" }}) (jsonNum{{ loop.index }}.ValueKind == JsonValueKind.Number ? (jsonNum{{ loop.index }}.TryGetInt64(out var i{{ loop.index }}) ? i{{ loop.index }} : jsonNum{{ loop.index }}.GetDouble()) : Convert.To{% if property.type == "integer" %}Int64{% else %}Double{% endif %}(jsonNum{{ loop.index }}.ToString())) + : Convert.To{% if property.type == "integer" %}Int64{% else %}Double{% endif %}(numRaw{{ loop.index }}))) + : null + {%- else -%} + (map["{{ property.name }}"] is JsonElement jsonNum{{ loop.index }} + ? ({{ property.type == "integer" ? "long" : "double" }}) (jsonNum{{ loop.index }}.ValueKind == JsonValueKind.Number ? (jsonNum{{ loop.index }}.TryGetInt64(out var i{{ loop.index }}) ? i{{ loop.index }} : jsonNum{{ loop.index }}.GetDouble()) : Convert.To{% if property.type == "integer" %}Int64{% else %}Double{% endif %}(jsonNum{{ loop.index }}.ToString())) + : Convert.To{% if property.type == "integer" %}Int64{% else %}Double{% endif %}(map["{{ property.name }}"])) {%- else %} {%- if property.type == "boolean" -%} - ({{ property | typeName }}{% if not property.required %}?{% endif %})map["{{ property.name }}"] + {%- if not property.required -%} + map.TryGetValue("{{ property.name }}", out var boolRaw{{ loop.index }}) + ? (boolRaw{{ loop.index }} == null + ? null + : (boolRaw{{ loop.index }} is JsonElement jsonBool{{ loop.index }} + ? ({{ property | typeName }})jsonBool{{ loop.index }}.GetBoolean() + : ({{ property | typeName }})Convert.ToBoolean(boolRaw{{ loop.index }}))) + : null + {%- else -%} + (map["{{ property.name }}"] is JsonElement jsonBool{{ loop.index }} + ? ({{ property | typeName }})jsonBool{{ loop.index }}.GetBoolean() + : ({{ property | typeName }})Convert.ToBoolean(map["{{ property.name }}"])) + {%- endif -%}
59-61: Guard optional arrays (non sub-schema) and unwrap JsonElementOptional arrays should use TryGetValue; current code indexes map["..."] unconditionally.
- map["{{ property.name }}"] is JsonElement jsonArrayProp{{ loop.index }} ? jsonArrayProp{{ loop.index }}.Deserialize<{{ property | typeName }}>()! : ({{ property | typeName }})map["{{ property.name }}"] + {%- if not property.required -%} + map.TryGetValue("{{ property.name }}", out var arrRaw{{ loop.index }}) + ? (arrRaw{{ loop.index }} is JsonElement jsonArrayProp{{ loop.index }} + ? jsonArrayProp{{ loop.index }}.Deserialize<{{ property | typeName }}>()! + : ({{ property | typeName }})arrRaw{{ loop.index }}) + : null + {%- else -%} + map["{{ property.name }}"] is JsonElement jsonArrayProp{{ loop.index }} + ? jsonArrayProp{{ loop.index }}.Deserialize<{{ property | typeName }}>()! + : ({{ property | typeName }})map["{{ property.name }}"] + {%- endif -%}
🧹 Nitpick comments (2)
src/SDK/Language/DotNet.php (1)
457-466: Good filters; consider mirroring overrideIdentifier as a function tooFilters look fine. Given you now expose functions for templates, consider also exposing an override_identifier(name) TwigFunction for parity and to avoid reimplementing overrides in multiple places.
templates/dotnet/Package/Models/Model.cs.twig (1)
41-46: Optional sub-schema properties should be null-safeBoth object and array sub-schemas index map["..."] without TryGetValue. For optional properties, this throws when omitted. Mirror the string/enum pattern with TryGetValue and return null when absent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/SDK/Language/DotNet.php(2 hunks)templates/dotnet/Package/Models/Model.cs.twig(4 hunks)tests/resources/spec.json(1 hunks)
🧰 Additional context used
🪛 PHPMD (2.15.0)
src/SDK/Language/DotNet.php
498-498: Avoid unused parameters such as '$definition'. (undefined)
(UnusedFormalParameter)
⏰ 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)
- GitHub Check: build (8.3, WebChromium)
- GitHub Check: build (8.3, Swift56)
- GitHub Check: build (8.3, Python313)
- GitHub Check: build (8.3, AppleSwift56)
- GitHub Check: build (8.3, Python310)
- GitHub Check: build (8.3, KotlinJava8)
- GitHub Check: build (8.3, Python311)
- GitHub Check: build (8.3, Python312)
- GitHub Check: build (8.3, PHP83)
- GitHub Check: build (8.3, Node18)
- GitHub Check: build (8.3, KotlinJava17)
- GitHub Check: build (8.3, FlutterStable)
- GitHub Check: build (8.3, Node16)
- GitHub Check: build (8.3, FlutterBeta)
- GitHub Check: build (8.3, KotlinJava11)
- GitHub Check: build (8.3, DotNet90)
- GitHub Check: build (8.3, DartBeta)
- GitHub Check: build (8.3, Android5Java17)
- GitHub Check: build (8.3, Android14Java17)
🔇 Additional comments (5)
src/SDK/Language/DotNet.php (1)
7-7: LGTM: TwigFunction import is correctThe new TwigFunction import is needed for getFunctions() and looks good.
tests/resources/spec.json (1)
1965-1977: LGTM: response enum added with required fieldstatus enum definition (x-enum-name: MockStatus) and its inclusion in required are correct and will exercise the new response-enum path.
Please confirm the generator now emits an enum type named MockStatus (not Mockstatus) across all targets. If needed, run the SDK generation locally to validate the casing and imports.
Also applies to: 1980-1982
templates/dotnet/Package/Models/Model.cs.twig (3)
6-6: LGTM: import Enums namespaceusing {{ spec.title | caseUcfirst }}.Enums; aligns with enum usage without full qualification.
14-14: Property type resolution via sub_schema(property) is fineSwitching to the new function is consistent with DotNet::getFunctions().
47-57: Add arrays-of-enums deserialization in From(...)Arrays-of-enums are not handled; current code will leave them as List or throw at runtime when projecting. Insert an array-of-enum branch before the scalar enum branch.
{%- elseif property.enum %} + {%- elseif property.type == 'array' and property.items and property.items.enum %} + {%- set enumName = property.items['enumName'] ?? (property.name ~ 'Item') -%} + {%- if not property.required -%} + map.TryGetValue("{{ property.name }}", out var enumArrRaw{{ loop.index }}) + ? (enumArrRaw{{ loop.index }} is JsonElement jsonArr{{ loop.index }} + ? jsonArr{{ loop.index }}.Deserialize<List<object>>()! + : (IEnumerable<object>)enumArrRaw{{ loop.index }}) + .Select(v => v == null ? null : new {{ enumName | caseUcfirst }}(v.ToString()!)) + .ToList() + : null + {%- else -%} + ((map["{{ property.name }}"] is JsonElement jsonArr{{ loop.index }} + ? jsonArr{{ loop.index }}.Deserialize<List<object>>()! + : (IEnumerable<object>)map["{{ property.name }}"])) + .Select(v => new {{ enumName | caseUcfirst }}(v.ToString()!)) + .ToList() + {%- endif %} + {%- elseif property.enum %}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
templates/apple/Package.swift.twig (1)
17-39: Guard the Enums target in the products listWe now skip emitting the
{{spec.title | caseUcfirst}}Enumstarget wheneverspec.requestEnumsis empty (Lines 37-55), yet the.libraryproduct still references that target unconditionally (Lines 17-19). On specs without request enums this generates aPackage.swiftthat refers to a non-existent target, breakingswift build. Wrap the product entry (and its dependency slot) in the same conditional guarding the target definition."{{spec.title | caseUcfirst}}", - "{{spec.title | caseUcfirst}}Enums", + {%~ if spec.requestEnums is not empty %} + "{{spec.title | caseUcfirst}}Enums", + {%~ endif %} "{{spec.title | caseUcfirst}}Models",templates/swift/Package.swift.twig (1)
17-39: Guard the Enums target in the products listIdentical to the Apple template: the
.libraryproduct still lists{{spec.title | caseUcfirst}}Enumseven whenspec.requestEnumsis empty, while the target itself is now dropped under the same condition (Lines 37-55). Every spec lacking request enums will yield an invalid manifest. Please wrap the product entry with the conditional too."{{spec.title | caseUcfirst}}", - "{{spec.title | caseUcfirst}}Enums", + {%~ if spec.requestEnums is not empty %} + "{{spec.title | caseUcfirst}}Enums", + {%~ endif %} "{{spec.title | caseUcfirst}}Models",templates/deno/mod.ts.twig (1)
11-28: Surface response enums in Deno imports/exportsBoth the import block at Line 11 and the export block at Line 26 now read only
spec.requestEnums, so response enums never make it into the Deno SDK entrypoint. Build a shared, deduped union of request and response enums and iterate over that in both sections.-{% for enum in spec.requestEnums %} +{% set exportedEnums = (spec.requestEnums | merge(spec.responseEnums)) | unique(attribute='name') %} +{% for enum in exportedEnums %} import { {{enum.name | caseUcfirst}} } from "./src/enums/{{enum.name | caseKebab}}.ts"; {% endfor %} ... -{% for enum in spec.requestEnums %} +{% for enum in exportedEnums %} {{enum.name | caseUcfirst}}, {% endfor %}src/SDK/SDK.php (1)
728-737: Avoid duplicate enum renders when names collide across request/response.If an enum name exists in both lists, the second render will overwrite the first (or cause duplicate outputs in some targets). Deduplicate by name during render.
Apply:
- foreach ($this->spec->getRequestEnums() as $key => $enum) { - $params['enum'] = $enum; - - $this->render($template, $destination, $block, $params, $minify); - } - foreach ($this->spec->getResponseEnums() as $key => $enum) { - $params['enum'] = $enum; - - $this->render($template, $destination, $block, $params, $minify); - } + $rendered = []; + foreach ($this->spec->getRequestEnums() as $enum) { + if (isset($rendered[$enum['name']])) { continue; } + $params['enum'] = $enum; + $this->render($template, $destination, $block, $params, $minify); + $rendered[$enum['name']] = true; + } + foreach ($this->spec->getResponseEnums() as $enum) { + if (isset($rendered[$enum['name']])) { continue; } + $params['enum'] = $enum; + $this->render($template, $destination, $block, $params, $minify); + $rendered[$enum['name']] = true; + }
🧹 Nitpick comments (1)
templates/python/package/encoders/value_class_encoder.py.twig (1)
2-4: Confirm encoder scope — request-only vs. also handling response enums.If ValueClassEncoder is used exclusively for outgoing payloads, limiting to request enums is fine. If it may serialize response-model instances (containing response enums), consider iterating the union of request+response enums to ensure
.valueextraction works for both.Would you like a follow-up change to include response enums as well (using a merged loop) or to add a quick unit test ensuring the encoder isn’t applied to response models?
Also applies to: 8-12
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/SDK/SDK.php(2 hunks)templates/apple/Package.swift.twig(2 hunks)templates/dart/lib/enums.dart.twig(1 hunks)templates/dart/lib/models.dart.twig(1 hunks)templates/deno/mod.ts.twig(2 hunks)templates/dotnet/Package/Services/ServiceTemplate.cs.twig(1 hunks)templates/kotlin/src/main/kotlin/io/appwrite/services/ServiceTemplate.kt.twig(1 hunks)templates/node/src/index.ts.twig(1 hunks)templates/python/package/encoders/value_class_encoder.py.twig(1 hunks)templates/react-native/src/index.ts.twig(1 hunks)templates/ruby/lib/container.rb.twig(1 hunks)templates/swift/Package.swift.twig(2 hunks)templates/web/src/index.ts.twig(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/SDK/SDK.php (2)
src/Spec/Swagger2.php (2)
getRequestEnums(509-532)getResponseEnums(537-572)src/Spec/Spec.php (2)
getRequestEnums(185-185)getResponseEnums(192-192)
⏰ 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, PHP80)
- GitHub Check: build (8.3, Ruby27)
- GitHub Check: build (8.3, WebChromium)
- GitHub Check: build (8.3, Swift56)
- GitHub Check: build (8.3, AppleSwift56)
- GitHub Check: build (8.3, WebNode)
- GitHub Check: build (8.3, PHP83)
- GitHub Check: build (8.3, Python313)
- GitHub Check: build (8.3, Node16)
- GitHub Check: build (8.3, Node20)
- GitHub Check: build (8.3, KotlinJava11)
- GitHub Check: build (8.3, KotlinJava17)
- GitHub Check: build (8.3, KotlinJava8)
- GitHub Check: build (8.3, FlutterBeta)
- GitHub Check: build (8.3, FlutterStable)
- GitHub Check: build (8.3, DartStable)
- GitHub Check: build (8.3, Android14Java17)
- GitHub Check: build (8.3, Android5Java17)
🔇 Additional comments (4)
templates/dart/lib/enums.dart.twig (1)
4-8: Deduplicate request/response enum partsAs flagged earlier, emitting two separate loops means any enum shared between request and response paths will generate duplicate
partdirectives, and Dart fails compilation on duplicatepartentries. Merge both lists and dedupe by enum name before writing the directives.-{% for enum in spec.requestEnums %} -part 'src/enums/{{enum.name | caseSnake}}.dart'; -{% endfor %} -{% for enum in spec.responseEnums %} -part 'src/enums/{{enum.name | caseSnake}}.dart'; -{% endfor %} +{% set enums = (spec.requestEnums | merge(spec.responseEnums)) | unique(attribute='name') %} +{% for enum in enums %} +part 'src/enums/{{enum.name | caseSnake}}.dart'; +{% endfor %}templates/kotlin/src/main/kotlin/io/appwrite/services/ServiceTemplate.kt.twig (1)
7-9: LGTM: import guard switched to request enums is correct.Services only need request-side enums for parameter types; response enums belong to models. Change aligns with split.
templates/dotnet/Package/Services/ServiceTemplate.cs.twig (1)
10-12: LGTM: conditionally importing Enums on request enums is appropriate.Service methods reference enum types in parameters; response enums are model-only. Good alignment with the split.
templates/dart/lib/models.dart.twig (1)
4-6: LGTM: import guarded on either request or response enums.Resolves unused_import warnings when no enums exist.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
templates/dart/lib/enums.dart.twig (1)
4-6: Optional: sort parts for deterministic output.To stabilize diffs across runs, sort by enum name.
-{% for enum in spec.allEnums %} +{% for enum in (spec.allEnums|sort(attribute='name')) %} part 'src/enums/{{enum.name | caseSnake}}.dart'; {% endfor %}src/Spec/Swagger2.php (3)
493-498: Normalize and keep back-compat enum metadata on properties.Downstream now reads normalized enumName/enumKeys, but some paths still expect x-enum-name/x-enum-keys. Set both to avoid breakage and ease transitions.
if (isset($def['enum'])) { // enum property $sch['properties'][$name]['enum'] = $def['enum']; $sch['properties'][$name]['enumName'] = $def['x-enum-name'] ?? ucfirst($key) . ucfirst($name); - $sch['properties'][$name]['enumKeys'] = $def['x-enum-keys'] ?? []; + $sch['properties'][$name]['enumKeys'] = $def['x-enum-keys'] ?? []; + // Back-compat aliases + $sch['properties'][$name]['x-enum-name'] = $sch['properties'][$name]['enumName']; + $sch['properties'][$name]['x-enum-keys'] = $sch['properties'][$name]['enumKeys']; }
509-533: Tighten duplicate-check and default keys in getRequestEnums().Current in_array() checks values, not keys; duplicates aren’t actually filtered by the guard. Also default enumKeys to [].
- if (isset($parameter['enumValues']) && !\in_array($enumName, $list)) { + if (isset($parameter['enumValues']) && !isset($list[$enumName])) { $list[$enumName] = [ 'name' => $enumName, 'enum' => $parameter['enumValues'], - 'keys' => $parameter['enumKeys'], + 'keys' => $parameter['enumKeys'] ?? [], ]; }
579-590: Optional: return enums in a deterministic order.Sort by name before array_values to stabilize outputs across runs.
public function getAllEnums(): array { $list = []; foreach ($this->getRequestEnums() as $enum) { $list[$enum['name']] = $enum; } foreach ($this->getResponseEnums() as $enum) { $list[$enum['name']] = $enum; } - return \array_values($list); + ksort($list, SORT_NATURAL | SORT_FLAG_CASE); + return \array_values($list); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/SDK/SDK.php(2 hunks)src/Spec/Swagger2.php(3 hunks)templates/dart/lib/enums.dart.twig(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/SDK/SDK.php (2)
src/Spec/Swagger2.php (2)
getAllEnums(579-590)getResponseEnums(537-574)src/Spec/Spec.php (1)
getResponseEnums(192-192)
src/Spec/Swagger2.php (1)
src/Spec/Spec.php (3)
getRequestEnums(185-185)getResponseEnums(192-192)getDefinitions(123-123)
🔇 Additional comments (3)
templates/dart/lib/enums.dart.twig (1)
4-6: LGTM: switched to spec.allEnums resolves duplicate part risk.Using spec.allEnums dedupes by enum.name, so overlapping request/response enums won’t emit duplicate part directives. Based on learnings
src/SDK/SDK.php (1)
729-734: LGTM: enum scope renders from getAllEnums.Iterating getAllEnums here aligns enum file generation with the merged, de-duplicated list.
src/Spec/Swagger2.php (1)
537-574: Blocking: getResponseEnums reads x-enum- but getDefinitions writes enumName/enumKeys.*This loses provided enum names/keys and yields inconsistent names for arrays. Read normalized enumName/enumKeys and use items metadata for array-of-enum cases.
- if (isset($property['enum'])) { - $enumName = $property['x-enum-name'] ?? ucfirst($modelName) . ucfirst($propertyName); - - if (!isset($list[$enumName])) { - $list[$enumName] = [ - 'name' => $enumName, - 'enum' => $property['enum'], - 'keys' => $property['x-enum-keys'] ?? [], - ]; - } - } + if (isset($property['enum'])) { + $enumName = $property['enumName'] ?? (ucfirst($modelName) . ucfirst($propertyName)); + if (!isset($list[$enumName])) { + $list[$enumName] = [ + 'name' => $enumName, + 'enum' => $property['enum'], + 'keys' => $property['enumKeys'] ?? [], + ]; + } + } // array of enums - if ((($property['type'] ?? null) === 'array') && isset($property['items']['enum'])) { - $enumName = $property['x-enum-name'] ?? ucfirst($modelName) . ucfirst($propertyName); - - if (!isset($list[$enumName])) { - $list[$enumName] = [ - 'name' => $enumName, - 'enum' => $property['items']['enum'], - 'keys' => $property['items']['x-enum-keys'] ?? [], - ]; - } - } + if ((($property['type'] ?? null) === 'array') && isset($property['items']['enum'])) { + $enumName = $property['items']['enumName'] ?? (ucfirst($modelName) . ucfirst($propertyName) . 'Item'); + if (!isset($list[$enumName])) { + $list[$enumName] = [ + 'name' => $enumName, + 'enum' => $property['items']['enum'], + 'keys' => $property['items']['enumKeys'] ?? [], + ]; + } + }
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)
templates/dart/lib/src/models/model.dart.twig (1)
29-34: Null-safety: fromMap() dereferences missing optional sub-schema fields
- For array sub-schemas,
map['key'].map(...)throws if the key is missing/null.- For object sub-schemas, calling
.fromMapwith null will throw.Use the same null-tolerant approach as primitive arrays and guard optional objects:
- {%- if property.type == 'array' -%} - List<{{property.sub_schema | caseUcfirst | overrideIdentifier}}>.from(map['{{property.name | escapeDollarSign }}'].map((p) => {{property.sub_schema | caseUcfirst | overrideIdentifier}}.fromMap(p))) - {%- else -%} - {{property.sub_schema | caseUcfirst | overrideIdentifier}}.fromMap(map['{{property.name | escapeDollarSign }}']) - {%- endif -%} + {%- if property.type == 'array' -%} + List<{{property.sub_schema | caseUcfirst | overrideIdentifier}}>.from((map['{{property.name | escapeDollarSign }}'] ?? []).map((p) => {{property.sub_schema | caseUcfirst | overrideIdentifier}}.fromMap(p))) + {%- else -%} + {%- if property.required -%} + {{property.sub_schema | caseUcfirst | overrideIdentifier}}.fromMap(map['{{property.name | escapeDollarSign }}']) + {%- else -%} + map['{{property.name | escapeDollarSign }}'] != null ? {{property.sub_schema | caseUcfirst | overrideIdentifier}}.fromMap(map['{{property.name | escapeDollarSign }}']) : null + {%- endif -%} + {%- endif -%}
🧹 Nitpick comments (3)
templates/dart/lib/src/models/model.dart.twig (2)
35-41: Harden enum type name with overrideIdentifierUse
overrideIdentifierto ensure generated enum type identifiers are valid/safe in Dart.Apply:
- enums.{{ enumName | caseUcfirst }}.values.firstWhere((e) => e.value == map['{{property.name | escapeDollarSign }}']) + enums.{{ enumName | caseUcfirst | overrideIdentifier }}.values.firstWhere((e) => e.value == map['{{property.name | escapeDollarSign }}']) @@ - map['{{property.name | escapeDollarSign }}'] != null ? enums.{{ enumName | caseUcfirst }}.values.firstWhere((e) => e.value == map['{{property.name | escapeDollarSign }}']) : null + map['{{property.name | escapeDollarSign }}'] != null ? enums.{{ enumName | caseUcfirst | overrideIdentifier }}.values.firstWhere((e) => e.value == map['{{property.name | escapeDollarSign }}']) : null
35-41: Consider fallback for unknown enum values on optional enums
firstWherethrows if no match is found; for optional fields use theorElseparameter or wrap intry/catchto returnnullinstead of throwing at runtime.src/SDK/SDK.php (1)
729-733: Optional: sort enums by name for deterministic outputStable ordering improves reproducible builds across environments.
Apply:
- foreach ($this->spec->getAllEnums() as $key => $enum) { + $__enums = $this->spec->getAllEnums(); + usort($__enums, static function ($a, $b) { + return strcmp($a['name'], $b['name']); + }); + foreach ($__enums as $key => $enum) { $params['enum'] = $enum; $this->render($template, $destination, $block, $params, $minify); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/tests.yml(0 hunks)src/SDK/SDK.php(2 hunks)templates/dart/lib/src/models/model.dart.twig(2 hunks)templates/node/src/index.ts.twig(1 hunks)templates/python/package/encoders/value_class_encoder.py.twig(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/tests.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- templates/python/package/encoders/value_class_encoder.py.twig
🧰 Additional context used
🧬 Code graph analysis (1)
src/SDK/SDK.php (2)
src/Spec/Swagger2.php (3)
getRequestEnums(509-532)getResponseEnums(537-574)getAllEnums(579-590)src/Spec/Spec.php (2)
getRequestEnums(185-185)getResponseEnums(192-192)
🔇 Additional comments (3)
templates/node/src/index.ts.twig (2)
10-12: Good fix: export union of request+response enums via allEnumsSwitching to spec.allEnums resolves the earlier omission of response-only enums from the Node entrypoint exports.
10-12: No changes required: Node enum templates emit value enums, soexport { Foo }correctly re-exports both value and type.src/SDK/SDK.php (1)
636-638: Add back-compatenumsalias and migrate legacy templates
- In src/SDK/SDK.php, insert:
'services' => $this->getFilteredServices(),
'enums' => $this->spec->getAllEnums(), // back-compat alias for legacy templates 'requestEnums' => $this->spec->getRequestEnums(), 'responseEnums' => $this->spec->getResponseEnums(), 'allEnums' => $this->spec->getAllEnums(),- Update `templates/android/library/src/main/java/io/package/services/Service.kt.twig` (lines 9–11) to replace `spec.enums` with the new `allEnums` (or `enums`) context key. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| return { | ||
| {% for property in definition.properties %} | ||
| "{{ property.name | escapeDollarSign }}": {% if property.sub_schema %}{% if property.type == 'array' %}{{property.name | escapeKeyword}}.map((p) => p.toMap()).toList(){% else %}{{property.name | escapeKeyword}}.toMap(){% endif %}{% else %}{{property.name | escapeKeyword }}{% endif %}, | ||
| "{{ property.name | escapeDollarSign }}": {% if property.sub_schema %}{% if property.type == 'array' %}{{property.name | escapeKeyword}}.map((p) => p.toMap()).toList(){% else %}{{property.name | escapeKeyword}}.toMap(){% endif %}{% elseif property.enum %}{{property.name | escapeKeyword}}{% if not property.required %}?{% endif %}.value{% else %}{{property.name | escapeKeyword }}{% endif %}, |
There was a problem hiding this comment.
Null-safety: toMap() dereferences optional sub-schema fields
Optional object/array sub-schemas call .toMap() / .map() unguarded, causing NPEs when the field is null.
Apply:
- "{{ property.name | escapeDollarSign }}": {% if property.sub_schema %}{% if property.type == 'array' %}{{property.name | escapeKeyword}}.map((p) => p.toMap()).toList(){% else %}{{property.name | escapeKeyword}}.toMap(){% endif %}{% elseif property.enum %}{{property.name | escapeKeyword}}{% if not property.required %}?{% endif %}.value{% else %}{{property.name | escapeKeyword }}{% endif %},
+ "{{ property.name | escapeDollarSign }}": {% if property.sub_schema %}{% if property.type == 'array' %}{% if property.required %}{{property.name | escapeKeyword}}.map((p) => p.toMap()).toList(){% else %}{{property.name | escapeKeyword}}?.map((p) => p.toMap())?.toList(){% endif %}{% else %}{% if property.required %}{{property.name | escapeKeyword}}.toMap(){% else %}{{property.name | escapeKeyword}}?.toMap(){% endif %}{% endif %}{% elseif property.enum %}{{property.name | escapeKeyword}}{% if not property.required %}?{% endif %}.value{% else %}{{property.name | escapeKeyword }}{% endif %},📝 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.
| "{{ property.name | escapeDollarSign }}": {% if property.sub_schema %}{% if property.type == 'array' %}{{property.name | escapeKeyword}}.map((p) => p.toMap()).toList(){% else %}{{property.name | escapeKeyword}}.toMap(){% endif %}{% elseif property.enum %}{{property.name | escapeKeyword}}{% if not property.required %}?{% endif %}.value{% else %}{{property.name | escapeKeyword }}{% endif %}, | |
| "{{ property.name | escapeDollarSign }}": {% if property.sub_schema %}{% if property.type == 'array' %}{% if property.required %}{{property.name | escapeKeyword}}.map((p) => p.toMap()).toList(){% else %}{{property.name | escapeKeyword}}?.map((p) => p.toMap())?.toList(){% endif %}{% else %}{% if property.required %}{{property.name | escapeKeyword}}.toMap(){% else %}{{property.name | escapeKeyword}}?.toMap(){% endif %}{% endif %}{% elseif property.enum %}{{property.name | escapeKeyword}}{% if not property.required %}?{% endif %}.value{% else %}{{property.name | escapeKeyword }}{% endif %}, |
🤖 Prompt for AI Agents
In templates/dart/lib/src/models/model.dart.twig around line 65, optional
sub-schema fields are calling toMap()/map() unguarded causing null-dereference
errors; update the template to use null-aware operators for optional fields: for
array sub-schemas use {{property.name | escapeKeyword}}?.map((p) =>
p.toMap()).toList(), for object sub-schemas use {{property.name |
escapeKeyword}}?.toMap(), and for optional enums use {{property.name |
escapeKeyword}}?.value, while preserving existing behavior (no ? ) when the
property is required.
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/SDK/Language/Swift.php (1)
320-322: Keep passing the spec when resolving array element typesWhen a property is an array of enums, this branch now emits
[Enums.<EnumName>]because the recursivegetTypeNamecall no longer receives$spec, so the module prefix collapses to justEnums.. The generated Swift won’t compile since there is noEnumsmodule. Please forward$spec(and keep the previousisPropertydefault) when recursing so we continue emitting[{{ spec.title | caseUcfirst }}Enums.<EnumName>].- ? '[' . $this->getTypeName($parameter['array']) . ']' + ? '[' . $this->getTypeName($parameter['array'], $spec) . ']'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/SDK/Language/Swift.php(1 hunks)templates/ruby/lib/container/models/model.rb.twig(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/SDK/Language/Swift.php (3)
src/SDK/Language/Kotlin.php (1)
getTypeName(107-130)src/SDK/Language/GraphQL.php (1)
getTypeName(19-57)src/SDK/Language.php (1)
getTypeName(44-44)
⏰ 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: build (8.3, WebChromium)
- GitHub Check: build (8.3, AppleSwift56)
- GitHub Check: build (8.3, Swift56)
- GitHub Check: build (8.3, WebNode)
- GitHub Check: build (8.3, Python312)
- GitHub Check: build (8.3, KotlinJava11)
- GitHub Check: build (8.3, Python311)
- GitHub Check: build (8.3, Python313)
- GitHub Check: build (8.3, KotlinJava8)
- GitHub Check: build (8.3, Node18)
- GitHub Check: build (8.3, KotlinJava17)
- GitHub Check: build (8.3, Go112)
- GitHub Check: build (8.3, FlutterStable)
- GitHub Check: build (8.3, DartBeta)
- GitHub Check: build (8.3, Android5Java17)
- GitHub Check: build (8.3, Android14Java17)
ChiragAgg5k
force-pushed
the
feat-enum-support
branch
from
d8f32b4 to
be203d7
Compare
What does this PR do?
add enum support to response models
Test Plan
Related PRs and Issues
Have you read the Contributing Guidelines on issues?
yes.
Summary by CodeRabbit
New Features
Refactor
Chores
Tests