Conversation
Greptile SummaryThis PR updates the Python SDK to version 18.0.0 targeting Appwrite API 1.9.1. It adds a new Several P1 issues flagged in prior review threads remain open: Confidence Score: 3/5Not safe to merge — multiple P1 nullable-field issues from prior threads remain unaddressed and will raise ValidationError in common real-world scenarios Four distinct P1 bugs identified in prior review rounds are still present: Key/DevKey expire nullability, Webhook secret nullability, Project billingLimits nullability, and unconditional null serialisation of the webhook secret param. Any of these will produce runtime ValidationError or send unexpected null values for a non-trivial subset of users. appwrite/models/key.py, appwrite/models/dev_key.py, appwrite/models/webhook.py, appwrite/models/project.py, appwrite/services/webhooks.py Important Files Changed
Reviews (4): Last reviewed commit: "chore: update Python SDK to 18.0.0" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| api_params['authUsername'] = self._normalize_value(auth_username) | ||
| if auth_password is not None: | ||
| api_params['authPassword'] = self._normalize_value(auth_password) | ||
| api_params['secret'] = self._normalize_value(secret) |
There was a problem hiding this comment.
secret unconditionally sent in request body
Unlike every other optional parameter in this function (enabled, tls, auth_username, auth_password), secret is always added to api_params without an if secret is not None guard. When secret defaults to None, this serialises as "secret": null in the JSON body. If the intent is to let the server auto-generate the key when the caller omits this argument, omitting the key entirely (consistent with the other optional params) is the safer approach.
| api_params['secret'] = self._normalize_value(secret) | |
| if secret is not None: | |
| api_params['secret'] = self._normalize_value(secret) |
| Whether or not to check the user password for similarity with their personal data. | ||
| authdisposableemails : bool | ||
| Whether or not to disallow disposable email addresses during signup and email updates. | ||
| authcanonicalemails : bool |
There was a problem hiding this comment.
Several docstring entries contain raw HTML entities (') instead of plain apostrophes. This makes help() output and generated API docs harder to read. The same pattern appears on line 240 (Used with plan's). The entities should be decoded during SDK generation or replaced here.
| authcanonicalemails : bool | |
| authpassworddictionary : bool | |
| Whether or not to check user's password against most commonly used passwords. |
| List of platforms. | ||
| """ | ||
| total: float = Field(..., alias='total') | ||
| platforms: List[Union[PlatformWeb, PlatformApple, PlatformAndroid, PlatformWindows, PlatformLinux]] = Field(..., alias='platforms') |
There was a problem hiding this comment.
Union[PlatformWeb, PlatformApple, PlatformAndroid, PlatformWindows, PlatformLinux] has no Pydantic discriminator. Pydantic v2 tries each type left-to-right until one validates, which works here because each subtype has a unique required field (hostname, bundleIdentifier, applicationId, etc.). However, it performs up to five validation attempts per item and fails silently if the server ever omits the type-specific field.
Since every platform already carries a type field backed by the PlatformType enum, adding a Literal constraint to each model and using Field(discriminator='type') would make parsing O(1), more explicit, and resilient to API schema drift. The same applies to the platforms field in appwrite/models/project.py.
|
|
||
| api_path = api_path.replace('{webhookId}', str(self._normalize_value(webhook_id))) | ||
|
|
||
| api_params['secret'] = self._normalize_value(secret) |
There was a problem hiding this comment.
secret assignment in update_secret
update_secret has the same pattern: api_params['secret'] = self._normalize_value(secret) always runs, sending null when the caller omits the argument. Applying the same if secret is not None guard as other optional params would make the behaviour consistent.
| api_params['secret'] = self._normalize_value(secret) | |
| if secret is not None: | |
| api_params['secret'] = self._normalize_value(secret) |
ChiragAgg5k
changed the title
ChiragAgg5k
changed the title
| createdat: str = Field(..., alias='$createdAt') | ||
| updatedat: str = Field(..., alias='$updatedAt') | ||
| name: str = Field(..., alias='name') | ||
| expire: str = Field(..., alias='expire') |
There was a problem hiding this comment.
expire must be Optional[str] — keys with unlimited expiration will fail to parse
create_key and update_key both unconditionally send expire: null when no expiry is specified, meaning any key created without an explicit expiry date will have expire: null in the server's response. With expire: str = Field(...) (required, non-nullable), Pydantic will raise a ValidationError when parsing the response for such a key, breaking create_key, get_key, and list_keys for all unlimited-expiry keys. The same applies to DevKey.expire.
| expire: str = Field(..., alias='expire') | |
| expire: Optional[str] = Field(default=None, alias='expire') |
| updatedat: str = Field(..., alias='$updatedAt') | ||
| name: str = Field(..., alias='name') | ||
| expire: str = Field(..., alias='expire') | ||
| secret: str = Field(..., alias='secret') |
There was a problem hiding this comment.
expire must be Optional[str] for the same reason as Key.expire
Dev keys can also be created without an expiration (expire: null). The field must be nullable or Project parsing (which embeds a List[DevKey]) will fail whenever any dev key has unlimited expiration.
| secret: str = Field(..., alias='secret') | |
| expire: Optional[str] = Field(default=None, alias='expire') |
ChiragAgg5k
changed the title
ChiragAgg5k
changed the title
| protocolstatusforgraphql: bool = Field(..., alias='protocolStatusForGraphql') | ||
| protocolstatusforwebsocket: bool = Field(..., alias='protocolStatusForWebsocket') | ||
| region: str = Field(..., alias='region') | ||
| billinglimits: BillingLimits = Field(..., alias='billingLimits') |
There was a problem hiding this comment.
billinglimits should be Optional[BillingLimits]
Projects on free-tier plans or without billing configured will likely return null for this field. With the current Field(...) (required, non-nullable) declaration, parsing any such project response will raise a ValidationError, breaking get_project-style calls for those users.
| billinglimits: BillingLimits = Field(..., alias='billingLimits') | |
| billinglimits: Optional[BillingLimits] = Field(default=None, alias='billingLimits') |
| tls: bool = Field(..., alias='tls') | ||
| authusername: str = Field(..., alias='authUsername') | ||
| authpassword: str = Field(..., alias='authPassword') | ||
| secret: str = Field(..., alias='secret') |
There was a problem hiding this comment.
secret field must be nullable — regular reads will raise ValidationError
The docstring for this field explicitly says "Only returned on creation and secret rotation." Any get() or list() call that doesn't include this field in the response will cause Pydantic to raise a ValidationError because it is declared as required (Field(...)). It should be Optional[str] with a None default, matching the same pattern used for other conditionally-returned fields in this SDK.
ChiragAgg5k
changed the title
1 participant
This PR contains updates to the Python SDK for version 18.0.0.