Add Pydantic models and uv support to the Python SDK#1375
Open
ChiragAgg5k wants to merge 13 commits intomasterfrom
Open
Add Pydantic models and uv support to the Python SDK#1375ChiragAgg5k wants to merge 13 commits intomasterfrom
ChiragAgg5k wants to merge 13 commits intomasterfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Pydantic-based model scaffolding, value normalization, response parsing (including Union support), expanded Python type helpers and request-model examples, and packaging templates (pyproject, requirements) including pydantic dependency — updates codegen templates and language helpers accordingly. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Service
participant HTTP_API as HTTP_API
participant Parser as _parse_response
participant Model as PydanticModel
Client->>Service: call method(params)
Service->>Service: _normalize_value(params)
Service->>HTTP_API: perform HTTP request
HTTP_API-->>Service: response (dict/JSON)
Service->>Parser: _parse_response(response, model or union_models)
alt union_models provided
Parser->>Model: try model.model_validate(response)
alt success
Model-->>Parser: validated instance
else
Parser->>Model: try next model
end
else single model provided
Parser->>Model: model.model_validate(response)
Model-->>Parser: validated instance
else no model
Parser-->>Service: return raw response
end
Parser-->>Service: typed result
Service-->>Client: return typed result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
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 (1)
templates/python/package/services/service.py.twig (1)
69-85:⚠️ Potential issue | 🟠 MajorKeep the return annotation aligned with the filtered response-model set.
If
method.responseModelscontains more than one entry but only one concrete model survives filtering, Line 85 falls back toDict[str, Any].templates/python/base/requests/api.twigLine 17 andtemplates/python/base/requests/file.twigLine 31 still parse and return that model, so the generated public type is wrong on a path this PR explicitly adds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/python/package/services/service.py.twig` around lines 69 - 85, The return type logic falls back to Dict[str, Any] when method.responseModels had multiple entries but filtering left exactly one concrete model; update the conditional that builds the return annotation (the block that inspects validResponseModels, method.responseModel and method.type for {{ method.name | caseSnake }}) so that: if validResponseModels|length > 1 produce Union[...] as before, else if validResponseModels|length == 1 emit that single model (apply the same service-name-to-Model rename logic used elsewhere), else fall back to the existing method.responseModel check, and only then to Dict[str, Any]; reference the validResponseModels variable and the return annotation generation around the def ... ) -> ... line to apply the fix.
🧹 Nitpick comments (1)
templates/python/requirements.txt.twig (1)
1-2: Consider consistent version pinning strategy.The
requestspackage is pinned to an exact version (2.31.0) whilepydanticuses a version range (>=2,<3). This inconsistency is minor but could be intentional. If this file is meant for reproducible development environments, consider pinning both; if it's for general compatibility, consider using ranges for both.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/python/requirements.txt.twig` around lines 1 - 2, The requirements file mixes exact pinning for requests and a range for pydantic; choose a consistent strategy and update both entries accordingly: either pin both (e.g., change pydantic to an exact version matching your tested env) or loosen both to ranges (e.g., change requests to a compatible range like requests>=2.31,<3); update the lines referencing "requests" and "pydantic" in templates/python/requirements.txt.twig to reflect the chosen consistent versioning approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/SDK/Language/Python.php`:
- Around line 490-535: The fallback path in getModelPropertyType currently
forces 'nullable' => false which drops schema nullability; update the array
passed to getTypeName in getModelPropertyType so you still force 'required' =>
true but preserve the original nullable flag (e.g. 'nullable' =>
$property['nullable'] ?? false) instead of overriding it to false, so
required-but-nullable fields remain nullable when types are generated.
In `@templates/python/package/services/service.py.twig`:
- Around line 20-24: The parameter type annotations in the template must match
the aliased import when a model name collides with the service name: update the
parameter annotation generation in
templates/python/package/services/service.py.twig (the use of {{ parameter |
getPropertyType(method) | raw }}) to apply the same collision check used for
return types (compare (parameter.model | caseUcfirst) to (service.name |
caseUcfirst)) and render the aliased name (e.g., {{ parameter.model |
caseUcfirst }}Model) when they match; alternatively, adjust
getPropertyType()/getTypeName() in the Python codegen so it returns the aliased
PascalCaseModel name for parameters whose model equals service.name to ensure
annotations reference an imported symbol.
---
Outside diff comments:
In `@templates/python/package/services/service.py.twig`:
- Around line 69-85: The return type logic falls back to Dict[str, Any] when
method.responseModels had multiple entries but filtering left exactly one
concrete model; update the conditional that builds the return annotation (the
block that inspects validResponseModels, method.responseModel and method.type
for {{ method.name | caseSnake }}) so that: if validResponseModels|length > 1
produce Union[...] as before, else if validResponseModels|length == 1 emit that
single model (apply the same service-name-to-Model rename logic used elsewhere),
else fall back to the existing method.responseModel check, and only then to
Dict[str, Any]; reference the validResponseModels variable and the return
annotation generation around the def ... ) -> ... line to apply the fix.
---
Nitpick comments:
In `@templates/python/requirements.txt.twig`:
- Around line 1-2: The requirements file mixes exact pinning for requests and a
range for pydantic; choose a consistent strategy and update both entries
accordingly: either pin both (e.g., change pydantic to an exact version matching
your tested env) or loosen both to ranges (e.g., change requests to a compatible
range like requests>=2.31,<3); update the lines referencing "requests" and
"pydantic" in templates/python/requirements.txt.twig to reflect the chosen
consistent versioning approach.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bfcaf903-eb37-4546-8a2d-c30c04bc525b
📒 Files selected for processing (15)
src/SDK/Language/Python.phptemplates/python/README.md.twigtemplates/python/base/params.twigtemplates/python/base/requests/api.twigtemplates/python/base/requests/file.twigtemplates/python/package/encoders/value_class_encoder.py.twigtemplates/python/package/models/__init__.py.twigtemplates/python/package/models/base_model.py.twigtemplates/python/package/models/model.py.twigtemplates/python/package/models/request_model.py.twigtemplates/python/package/service.py.twigtemplates/python/package/services/service.py.twigtemplates/python/pyproject.toml.twigtemplates/python/requirements.txt.twigtemplates/python/setup.py.twig
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/SDK/Language/Python.php`:
- Around line 490-535: The getModelPropertyType function returns bare types for
special-case branches (sub_schemas, sub_schema, enum arrays, enums, and object)
and doesn't respect nullable, causing Pydantic validation failures; define a
small helper closure (e.g., $wrapNullable as suggested) inside
getModelPropertyType that checks ($property['nullable'] ?? false) and returns
'Optional[TYPE]' when needed, then apply $wrapNullable to the return values for
the sub_schemas branch (unionType), sub_schema branch (modelType), the enum
array branch (enumType wrapped in List[...] then wrapped), the enum branch
(enumType or List[...] wrapped), and the object branch ('Dict[str, Any]') so
every path preserves nullable semantics before returning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f95faf7b-5101-4cb2-a915-b2590124c606
📒 Files selected for processing (3)
src/SDK/Language/Python.phptemplates/python/package/services/service.py.twigtemplates/python/requirements.txt.twig
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/languages/python/tests.py (1)
89-97: Add one typed request-model call here.These cases still only exercise raw
dict/list[dict]payloads, so they won't catch regressions in the newAppwriteModelnormalization/serialization path. Please add at least one request that constructs the generated request model directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/languages/python/tests.py` around lines 89 - 97, Add a test that constructs and passes a generated request-model instance (instead of a raw dict) to the client to exercise AppwriteModel normalization/serialization; specifically, import the generated model class for the player request (e.g., the Player or CreatePlayerRequest model) and call general.create_player(...) with an instance of that model (mirroring the existing dict payload used by general.create_player/general.create_players) so the AppwriteModel code path is exercised during tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@templates/python/docs/example.md.twig`:
- Around line 15-18: The docs example imports request models without guarding
against name collisions with the service class; update
templates/python/docs/example.md.twig so when computing modelName (the existing
variable) you check against the service class name (the symbol used to import
the service on line 3) and, if equal, import the model as an alias like {{
modelName | caseUcfirst }}Model (update the import line that currently does
"from {{ spec.title | caseSnake }}.models import {{ modelName | caseUcfirst }}"
to use the aliased identifier when collision detected and add the aliased name
to added). Also modify the requestModelExample filter in
src/SDK/Language/Python.php to accept the service name as an extra parameter
and, when the service and model names collide, emit constructor calls using the
aliased model identifier (e.g., FooModel) so example code references the correct
symbol.
In `@templates/python/package/models/request_model.py.twig`:
- Around line 50-52: The current to_dict() implementation in AppwriteModel calls
model_dump(by_alias=True, mode='json') (see base_model.py.twig and
templates/python/package/models/request_model.py.twig) which causes unset
optional fields to be serialized as null; update to call
model_dump(by_alias=True, mode='json', exclude_unset=True) so that omitted
fields are excluded while still preserving explicitly-set nulls for nullable
properties — modify the AppwriteModel.to_dict() (or the function that invokes
model_dump) to add exclude_unset=True and run existing tests to ensure nullable
fields remain serializable when explicitly set.
In `@templates/python/package/services/service.py.twig`:
- Around line 18-27: The import block in
templates/python/package/services/service.py.twig only checks parameter.model so
array-of-model parameters (parameter.array.model) aren't imported causing
NameError; update the import logic to derive a single modelName (e.g.
parameter.model ?? parameter.array.model ?? null) and use that for the existing
added-check and import generation (adjust the existing conditional that
references parameter.model to reference modelName instead) so List[Model] types
emitted by getServicePropertyType() have their model imported.
---
Nitpick comments:
In `@tests/languages/python/tests.py`:
- Around line 89-97: Add a test that constructs and passes a generated
request-model instance (instead of a raw dict) to the client to exercise
AppwriteModel normalization/serialization; specifically, import the generated
model class for the player request (e.g., the Player or CreatePlayerRequest
model) and call general.create_player(...) with an instance of that model
(mirroring the existing dict payload used by
general.create_player/general.create_players) so the AppwriteModel code path is
exercised during tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5b64c9a8-a7db-4d03-9b3c-a6d44b522b27
📒 Files selected for processing (6)
src/SDK/Language/Python.phptemplates/python/docs/example.md.twigtemplates/python/package/models/model.py.twigtemplates/python/package/models/request_model.py.twigtemplates/python/package/services/service.py.twigtests/languages/python/tests.py
🚧 Files skipped from review as they are similar to previous changes (1)
- templates/python/package/models/model.py.twig
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@templates/python/docs/example.md.twig`:
- Around line 62-63: The generated Python return type uses Union but the
template never imports it; update the template to conditionally add "from typing
import Union" when multiple response models are present by duplicating the
filter logic used to compute validResponseModels (i.e. check "{% if
method.responseModels is defined and method.responseModels | filter((m) => m and
m != 'any') | length > 1 %}" before the imports) or alternatively move the
existing validResponseModels computation ahead of the import block so you can
use "{% if validResponseModels | length > 1 %}" to emit the import; modify the
import block accordingly so result: Union[...] has a matching import.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 33838c89-a576-43c6-827a-4bef8b1cc1ef
📒 Files selected for processing (1)
templates/python/docs/example.md.twig
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
templates/python/package/services/service.py.twig (1)
18-28:⚠️ Potential issue | 🟠 MajorUse a non-empty fallback when picking
modelName.Line 18 stops at
parameter.modeleven when it is an empty string, but Line 103 still resolves array-of-model annotations fromarray.modelviagetServicePropertyType(). In that shape, the generated signature becomesList[Foo]without importingFoo, so the service module still breaks on import. The same selector is repeated intemplates/python/docs/example.md.twig:49-58andsrc/SDK/Language/Python.php:489-503, so it is worth fixing all three together.Suggested fix
-{% set modelName = parameter.model ?? parameter.array.model ?? null %} +{% set modelName = parameter.model is not empty ? parameter.model : (parameter.array.model is not empty ? parameter.array.model : null) %} {% if modelName %} {% if modelName not in added %} {% if (modelName | caseUcfirst) == (service.name | caseUcfirst) %} from ..models.{{ modelName | caseSnake }} import {{ modelName | caseUcfirst }} as {{ modelName | caseUcfirst }}Model;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/python/package/services/service.py.twig` around lines 18 - 28, The code selects modelName using parameter.model even when it's an empty string, causing imports to be skipped for array models; change the selection to prefer a non-empty value (e.g., set modelName to parameter.model if parameter.model is defined and not empty, otherwise to parameter.array.model when present) so the import logic that checks added and generates the from ..models import runs correctly; apply the same non-empty fallback fix in the other two affected locations (the docs example template and the PHP logic around getServicePropertyType()) and ensure the conditional uses a truthy check rather than just existence for variable modelName and for the merge into added.
🧹 Nitpick comments (1)
templates/python/package/models/base_model.py.twig (1)
23-27: Keepto_json()aligned withto_dict().
templates/python/package/service.py.twig:16-32andtemplates/python/package/encoders/value_class_encoder.py.twig:7-17both treatto_dict()as the canonical serializer and omit unset fields.to_json()currently keeps them, so the two public helpers can emit different payloads for the same model instance.Suggested fix
def to_dict(self) -> Dict[str, Any]: return self.model_dump(by_alias=True, mode='json', exclude_unset=True) def to_json(self) -> str: - return self.model_dump_json(by_alias=True) + return self.model_dump_json(by_alias=True, exclude_unset=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/python/package/models/base_model.py.twig` around lines 23 - 27, The to_json() method must match to_dict()'s behaviour: update to_json() (currently calling model_dump_json) to pass the same arguments used by to_dict() — by_alias=True, mode='json', and exclude_unset=True — so both public serializers (to_dict and to_json) produce identical payloads for the same model instance; locate the to_json and to_dict methods in base_model.py.twig and ensure model_dump_json is called with those exact kwargs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/SDK/Language/Python.php`:
- Around line 648-671: getResponseType currently returns raw model names and can
produce types that collide with the service class; update it to use the
collision-aware naming helper getDocsModelTypeName instead of getModelName when
resolving response model names (both inside the array_map for responseModels and
the single responseModel branch), and ensure the union created via getUnionType
uses those collision-aware names so the generated Returns doc in service.py.twig
references valid types.
---
Duplicate comments:
In `@templates/python/package/services/service.py.twig`:
- Around line 18-28: The code selects modelName using parameter.model even when
it's an empty string, causing imports to be skipped for array models; change the
selection to prefer a non-empty value (e.g., set modelName to parameter.model if
parameter.model is defined and not empty, otherwise to parameter.array.model
when present) so the import logic that checks added and generates the from
..models import runs correctly; apply the same non-empty fallback fix in the
other two affected locations (the docs example template and the PHP logic around
getServicePropertyType()) and ensure the conditional uses a truthy check rather
than just existence for variable modelName and for the merge into added.
---
Nitpick comments:
In `@templates/python/package/models/base_model.py.twig`:
- Around line 23-27: The to_json() method must match to_dict()'s behaviour:
update to_json() (currently calling model_dump_json) to pass the same arguments
used by to_dict() — by_alias=True, mode='json', and exclude_unset=True — so both
public serializers (to_dict and to_json) produce identical payloads for the same
model instance; locate the to_json and to_dict methods in base_model.py.twig and
ensure model_dump_json is called with those exact kwargs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: af7faeb0-918b-46d2-bc88-a3eb123454e9
📒 Files selected for processing (5)
src/SDK/Language/Python.phptemplates/python/docs/example.md.twigtemplates/python/package/models/base_model.py.twigtemplates/python/package/services/service.py.twigtests/languages/python/tests.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/languages/python/tests.py
Add generic type support for models with additionalProperties (e.g., Row):
- Add hasGenericType() method and Twig filters to detect generic models
- Update model.py.twig to generate Generic[T] classes with:
- PrivateAttr _data for storing typed user data
- data property with type T for IDE autocomplete
- with_data() class method for typed deserialization
- to_dict() override for proper serialization
- Update service.py.twig to accept model_type parameter
- Update api.twig to use with_data() for generic response models
- Support nested generic types (RowList -> Row[T])
Usage:
row = tables.get_row(..., model_type=Post)
print(row.data.title) # Fully typed access
rows = tables.list_rows(..., model_type=Post)
for row in rows.rows:
print(row.data.title) # Also typed!
ChiragAgg5k
force-pushed
the
feat/python-pydantic-models
branch
from
edbc435 to
46fb9f7
Compare
loks0n
reviewed
Mar 10, 2026
ChiragAgg5k
force-pushed
the
feat/python-pydantic-models
branch
from
46fb9f7 to
604d389
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
uvvia generatedpyproject.tomlVerification
php example.php pythoncomposer lint-twigpython3 -m compileall examples/python/appwriteLocale.get()returns typedLocaleLocale.list_countries()returns typedCountryListHealth.get()returns typedHealthStatusTablesDB.get_row(..., model_type=Post)returns Row[Post] with typeddatafieldTablesDB.list_rows(..., model_type=Post)returns RowList[Post] with typed rowsGeneric Models Usage
Define your custom Pydantic model:
Fetch with type-safe access:
Notes
Project.list_variables()) returned401 missing scopes (["projects.read"])with the provided API key, which indicates a key-permission issue rather than an SDK parsing/runtime issue.Files Changed
src/SDK/Language/Python.php: Added hasGenericType() method and Twig filterstemplates/python/package/models/model.py.twig: Added Generic[T] support, data property, with_data() methodtemplates/python/package/services/service.py.twig: Added model_type parametertemplates/python/base/requests/api.twig: Updated to use with_data() for generic responsesSummary by CodeRabbit
New Features
Updates
Tests
Documentation