feat(python): Improve handling of null and optional parameters in Python SDK #1223
Conversation
WalkthroughThis diff introduces an explicit null-handling system for generated Python API clients. The changes replace hard runtime exceptions for missing required parameters with a conditional inclusion model that tracks nullable parameters through a new Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes This comprises heterogeneous, logic-dense changes across four interconnected template files. The modifications introduce a new null-handling infrastructure pattern that must be traced through parameter templating, client API calls, request construction, and value encoding. The client.py changes are particularly involved, spanning header and parameter processing refinements, multipart form-data handling updates, error path modifications, and explicit null preservation logic. While the pattern is coherent throughout, the density of control flow changes, conditional logic branches (for required vs. optional parameters), and interactions between components demands careful reasoning across all affected areas rather than simple pattern matching. Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ 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}
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 (1)
templates/python/package/client.py.twig (1)
96-109: Use safe content-type lookups; support GET query stringification.
Avoid KeyError and ensure correct branch detection. Optionally stringify query params for GET.Apply this diff:
- # Handle JSON content - if headers['content-type'].startswith('application/json'): + # Handle JSON content + if headers.get('content-type', '').startswith('application/json'): data = json.dumps(data, cls=ValueClassEncoder) - # Handle multipart form data - if headers['content-type'].startswith('multipart/form-data'): + # Handle multipart form data + if headers.get('content-type', '').startswith('multipart/form-data'): del headers['content-type'] stringify = True @@ - data = self.flatten(data, stringify=stringify) + data = self.flatten(data, stringify=stringify) + + # For GET requests, ensure query params are strings + if method == 'get': + stringify = True
🧹 Nitpick comments (5)
templates/python/package/encoders/value_class_encoder.py.twig (1)
11-18: Explicit‑null and Enum handling looks good.
Attribute check for is_null integrates with the client’s marker; generic Enum handling reduces boilerplate.You can drop per‑enum isinstance checks since the generic Enum branch already returns .value, unless some generated enums don’t subclass Enum.
templates/python/package/client.py.twig (4)
110-122: Add a sane request timeout to avoid indefinite hangs.
External calls without timeouts are risky for SDK consumers.Apply this diff:
response = requests.request( method=method, url=self._endpoint + path, params=self.flatten(params, stringify=stringify), data=data, files=files, headers=headers, verify=(not self._self_signed), - allow_redirects=False if response_type == 'location' else True + allow_redirects=False if response_type == 'location' else True, + timeout=60 )If you prefer configurability, expose a set_timeout() on Client and use it here.
126-141: Defensive response header access.
Use .get to avoid KeyError when Content-Type is absent.Apply this diff:
- content_type = response.headers['Content-Type'] + content_type = response.headers.get('Content-Type', '') @@ - if content_type.startswith('application/json'): + if content_type.startswith('application/json'): return response.json()
143-151: Defensive error path header access.
Same for exception branch.Apply this diff:
- if response is not None: - content_type = response.headers['Content-Type'] + if response is not None: + content_type = response.headers.get('Content-Type', '')
235-259: Differentiate explicit null vs None when stringifying.
Preserve intent: emit 'null' for explicit null markers and '' for implicit None when stringify=True (query/multipart).Apply this diff:
- if value is None or self._is_explicitly_null(value): # Handle null values - if stringify: - output[finalKey] = '' - else: - output[finalKey] = None + if value is None or self._is_explicitly_null(value): # Handle null values + if stringify: + # Preserve explicit null semantics in string form + output[finalKey] = 'null' if self._is_explicitly_null(value) else '' + else: + output[finalKey] = NoneThis pairs with params.twig behavior for required/nullable params.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
templates/python/base/params.twig(1 hunks)templates/python/base/requests/api.twig(1 hunks)templates/python/package/client.py.twig(6 hunks)templates/python/package/encoders/value_class_encoder.py.twig(1 hunks)
🔇 Additional comments (2)
templates/python/base/requests/api.twig (1)
8-8: LGTM: passing nullable_params matches the new Client.call signature.
Order is correct: method, path, headers, params, nullable_params, response_type.templates/python/base/params.twig (1)
28-36: No issues found. The implementation correctly aligns with Appwrite's null-handling expectations.The code already implements the correct behavior:
client.null()creates a special marker to distinguish explicit nulls from undefined values- During parameter filtering (line 78), explicit nulls are preserved
- In
flatten()withstringify=False(used for GET query params), explicit nulls becomeNone- The
requestslibrary omitsNonevalues from query parameters—they are not sent as the literal string "null" or "None", but simply omittedPer the web search results, Appwrite SDKs commonly strip keys with value null from query-string params, which is exactly what happens here. No coordination with flatten() or changes to stringify behavior are needed.
| # Required path parameters - convert None to explicit null | ||
| if {{ parameter.name | escapeKeyword | caseSnake }} is None: | ||
| path_value = 'null' | ||
| else: | ||
| path_value = str({{ parameter.name | escapeKeyword | caseSnake }}) | ||
| api_path = api_path.replace('{{ '{' }}{{ parameter.name | caseCamel }}{{ '}' }}', path_value) |
There was a problem hiding this comment.
🧩 Analysis chain
Confirm server semantics for required path None → 'null'.
Replacing a required path segment with literal "null" may not map to the intended resource. Some servers 404 or treat it as a string. Verify this matches the API contract.
🌐 Web query:
For Appwrite REST endpoints, should absent/None path parameters be represented as the literal "null" segment, or should requests fail early?
💡 Result:
Short answer: do not represent absent/None path parameters as a literal "null" path segment — requests should fail early (400-level) and the client should omit or validate the parameter before building the URL.
Why (brief):
- Appwrite endpoints expect real path segments; passing "null" as a segment produces routing/type errors in practice (users report TypeError/invalid-param failures). [1]
- URI/URL builders and server frameworks treat path segments as non-null strings (many implementations throw on null segments), so using a literal "null" is ambiguous and breaks routing/validation. [2]
What to do instead:
- Validate on the client: if a path parameter is optional, either omit that path from the URL or use a different API design (query param, distinct endpoint, or explicit 0/empty-id semantics).
- On the server, return a client error (400/422) when a required path parameter is missing or when the client sends an invalid literal like "null".
Sources:
- Appwrite community thread showing errors when null is used for parameters. [1]
- JAX‑RS / UriBuilder documentation showing path segments must be non-null (frameworks reject null segments). [2]
Fix path parameter validation—do not convert None to literal "null" segment.
Appwrite REST endpoints reject requests with a literal "null" path segment; this causes routing/type errors. Required path parameters should fail early with validation (400-level error) on the client side before building the URL, not be replaced with a string.
Update lines 13–18 to either validate that the parameter is not None and raise an exception, or design the endpoint to accept optional parameters via a different mechanism (query parameter or distinct endpoint).
Appwrite endpoints expect real path segments; passing "null" as a segment produces routing/type errors in practice, and URI/URL builders and server frameworks treat path segments as non-null strings.
🤖 Prompt for AI Agents
templates/python/base/params.twig around lines 13 to 18: currently None path
params are converted to the literal string "null", which breaks Appwrite
routing; instead validate required path parameters and raise a client-side error
if None. Change the logic to check if {{ parameter.name | escapeKeyword |
caseSnake }} is None and raise an exception (e.g., ValueError with a clear
message indicating the missing required path parameter) so the caller gets a 4xx
failure; otherwise convert to str and proceed to replace the path segment as
before.
| # Required header parameters - convert None to explicit null | ||
| if {{ parameter.name | escapeKeyword | caseSnake }} is None: | ||
| self.client.add_header('{{ parameter.name }}', self.client.null()) | ||
| else: | ||
| self.client.add_header('{{ parameter.name }}', {{ parameter.name | escapeKeyword | caseSnake }}) | ||
| {% else %} | ||
| api_params['{{ parameter.name }}'] = {{ parameter.name | escapeKeyword | caseSnake }} | ||
| # Optional header parameters - only include if not None | ||
| if {{ parameter.name | escapeKeyword | caseSnake }} is not None: | ||
| self.client.add_header('{{ parameter.name }}', {{ parameter.name | escapeKeyword | caseSnake }}) | ||
| {% endif %} |
There was a problem hiding this comment.
Don’t send explicit nulls in HTTP headers.
Headers cannot carry JSON null. Passing client.null() will become None and may break requests or send "None".
Apply this diff to omit headers when None:
-{% if parameter.required %}
- # Required header parameters - convert None to explicit null
- if {{ parameter.name | escapeKeyword | caseSnake }} is None:
- self.client.add_header('{{ parameter.name }}', self.client.null())
- else:
- self.client.add_header('{{ parameter.name }}', {{ parameter.name | escapeKeyword | caseSnake }})
+{% if parameter.required %}
+ # Required header parameters - omit when None (HTTP headers have no null representation)
+ if {{ parameter.name | escapeKeyword | caseSnake }} is not None:
+ self.client.add_header('{{ parameter.name }}', {{ parameter.name | escapeKeyword | caseSnake }})Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In templates/python/base/params.twig around lines 86 to 95, the current logic
sends explicit nulls for header parameters by calling
self.client.add_header(..., self.client.null()) when a required header value is
None, which is invalid; change the logic so no header is added when the
parameter is None for both required and optional cases (i.e., only call
self.client.add_header(...) when the parameter is not None), and remove the
branch that converts None to client.null(); if business rules require a missing
required header to be surfaced, replace the null-send with raising a clear
ValueError before attempting to add the header.
| {% for key, header in method.headers %} | ||
| '{{ key }}': '{{ header }}', | ||
| {% endfor %} |
There was a problem hiding this comment.
Normalize static header keys to lowercase to match client checks.
Client.call looks up 'content-type' (lowercase). If spec emits 'Content-Type', JSON/multipart branches won’t trigger.
Apply this diff:
-{% for key, header in method.headers %}
- '{{ key }}': '{{ header }}',
-{% endfor %}
+{% for key, header in method.headers %}
+ '{{ key | lower }}': '{{ header }}',
+{% endfor %}📝 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.
| {% for key, header in method.headers %} | |
| '{{ key }}': '{{ header }}', | |
| {% endfor %} | |
| {% for key, header in method.headers %} | |
| '{{ key | lower }}': '{{ header }}', | |
| {% endfor %} |
🤖 Prompt for AI Agents
In templates/python/base/requests/api.twig around lines 5 to 7, static header
keys are emitted with their original casing which can be 'Content-Type' and thus
won't match client.call's lowercase lookup; update the template to normalize
header names to lowercase when rendering (e.g., convert each key to lower-case
before output) so emitted headers use lowercase keys like 'content-type',
preserving header values as-is.
| @staticmethod | ||
| def _is_explicitly_null(value): | ||
| """Helper method to distinguish between None (undefined) and explicit null values""" | ||
| # Check if value is a special marker indicating an explicit null | ||
| return isinstance(value, type('NullValue', (), {'is_null': True})) | ||
|
|
||
| @staticmethod | ||
| def null(): | ||
| """Helper method to create an explicit null value to be sent to the API | ||
| Use this when you want to explicitly set a field to null, as opposed to | ||
| not sending the field at all (which happens when you use None)""" | ||
| return type('NullValue', (), {'is_null': True})() | ||
|
|
There was a problem hiding this comment.
Explicit‑null detection is broken (always False).
Using isinstance(value, type('NullValue', ...)) creates a new class each time; no instance matches. This breaks filtering and leads to object reprs leaking into headers/params.
Apply this diff to introduce a stable sentinel and fix detection:
- @staticmethod
- def _is_explicitly_null(value):
- """Helper method to distinguish between None (undefined) and explicit null values"""
- # Check if value is a special marker indicating an explicit null
- return isinstance(value, type('NullValue', (), {'is_null': True}))
-
- @staticmethod
- def null():
- """Helper method to create an explicit null value to be sent to the API
- Use this when you want to explicitly set a field to null, as opposed to
- not sending the field at all (which happens when you use None)"""
- return type('NullValue', (), {'is_null': True})()
+ # Explicit NULL sentinel
+ class _NullValue:
+ __slots__ = ()
+ is_null = True
+ _NULL = _NullValue()
+
+ @staticmethod
+ def _is_explicitly_null(value):
+ """Distinguish explicit null sentinel (or any object with is_null=True) from None."""
+ return (value is Client._NULL) or (getattr(value, 'is_null', False) is True)
+
+ @staticmethod
+ def null():
+ """Create an explicit null value to be sent to the API."""
+ return Client._NULL🤖 Prompt for AI Agents
In templates/python/package/client.py.twig around lines 28 to 40, the
explicit-null detection is broken because type('NullValue', ...) creates a new
dynamic class each call so isinstance never matches; replace that with a stable
module-level sentinel by defining a private sentinel class (e.g., _NullValue)
and a single instance (e.g., _NULL) and then change null() to return that
singleton and _is_explicitly_null(value) to detect it (use identity check `value
is _NULL` or isinstance against the stable class), ensuring all code uses the
same sentinel instance.
| def call(self, method, path='', headers=None, params=None, nullable_params=None, response_type='json'): | ||
| if headers is None: | ||
| headers = {} | ||
|
|
||
| if params is None: | ||
| params = {} | ||
|
|
||
| if nullable_params is None: | ||
| nullable_params = [] | ||
|
|
||
| # Process headers and params to handle explicit nulls while removing undefined (None) values | ||
| headers = {k: v for k, v in headers.items() if v is not None or self._is_explicitly_null(v)} | ||
| params = {k: v for k, v in params.items() if v is not None or self._is_explicitly_null(v) or k in nullable_params} | ||
|
|
||
| params = {k: v for k, v in params.items() if v is not None} # Remove None values from params dictionary | ||
| # Replace explicit null markers with None for JSON serialization | ||
| headers = {k: None if self._is_explicitly_null(v) else v for k, v in headers.items()} | ||
| params = {k: None if self._is_explicitly_null(v) or (v is None and k in nullable_params) else v for k, v in params.items()} | ||
|
|
||
| # Merge with global headers | ||
| headers = {**self._global_headers, **headers} | ||
|
|
There was a problem hiding this comment.
Normalize header keys and make content-type checks robust.
Per-call headers may have mixed casing; code later reads 'content-type'. Normalize keys and use .get to avoid KeyError.
Apply this diff:
def call(self, method, path='', headers=None, params=None, nullable_params=None, response_type='json'):
@@
if params is None:
params = {}
-
+ # Normalize header keys to lowercase once
+ headers = { (k.lower() if isinstance(k, str) else k): v for k, v in headers.items() }
if nullable_params is None:
nullable_params = []
- # Process headers and params to handle explicit nulls while removing undefined (None) values
- headers = {k: v for k, v in headers.items() if v is not None or self._is_explicitly_null(v)}
+ # Process headers and params to handle explicit nulls while removing undefined (None) values
+ headers = {k: v for k, v in headers.items() if v is not None or self._is_explicitly_null(v)}
params = {k: v for k, v in params.items() if v is not None or self._is_explicitly_null(v) or k in nullable_params}
- # Replace explicit null markers with None for JSON serialization
- headers = {k: None if self._is_explicitly_null(v) else v for k, v in headers.items()}
+ # Replace explicit null markers with None for JSON serialization (headers: better to drop None later if present)
+ headers = {k: None if self._is_explicitly_null(v) else v for k, v in headers.items()}
params = {k: None if self._is_explicitly_null(v) or (v is None and k in nullable_params) else v for k, v in params.items()}
# Merge with global headers
headers = {**self._global_headers, **headers}And update downstream content-type checks (see next comment).
🤖 Prompt for AI Agents
In templates/python/package/client.py.twig around lines 66 to 86, per-call
headers may use mixed casing and later code reads 'content-type' directly
causing KeyError or missed matches; normalize header keys to lower-case and
ensure lookups use .get. After merging global and per-call headers, rebuild the
headers dict with all keys lowercased (e.g., headers = {k.lower(): v for k, v in
headers.items()}) so downstream checks like headers.get('content-type') are
robust; also ensure any direct header access in this file uses
.get('content-type') instead of indexing.
2 participants
This PR improves how null and optional parameters are handled in the Python SDK generator.
Resolves: appwrite/sdk-for-python#111
Changes:
Added explicit null value support in Client class:
null()helper method to create explicit null values_is_explicitly_null()method to distinguish between None and explicit nullsEnhanced parameter handling in requests:
Improved JSON serialization:
Fixed template issues:
Example Usage:
Testing Script
Screenshots
Case 1: Change elements value to
['option1', 'option2', 'option3']Verify elements value on console
Case 2: Define the optional param
new_key=transaction_updatedVerify new key name on console
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes