-
Couldn't load subscription status.
- Fork 189
feat(python): Improve handling of null and optional parameters in Python SDK #1223
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
aeca32f
a1811b8
5c53515
81e4817
a530b32
1dbb3cc
704040d
16d56fb
49b41ea
6043a41
6bfe072
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,31 +1,97 @@ | ||
| api_params = {} | ||
| {% if method.parameters.all | length %} | ||
| {% for parameter in method.parameters.all %} | ||
| {% if parameter.required and not parameter.nullable %} | ||
| if {{ parameter.name | escapeKeyword | caseSnake }} is None: | ||
| raise {{spec.title | caseUcfirst}}Exception('Missing required parameter: "{{ parameter.name | escapeKeyword | caseSnake }}"') | ||
| nullable_params = [] | ||
|
|
||
| {% if method.parameters.all | length %} | ||
| {% for parameter in method.parameters.all %} | ||
| {% if parameter.nullable %} | ||
| nullable_params.append('{{ parameter.name }}') | ||
| {% endif %} | ||
| {% endfor %} | ||
|
|
||
| {% for parameter in method.parameters.path %} | ||
| api_path = api_path.replace('{{ '{' }}{{ parameter.name | caseCamel }}{{ '}' }}', {{ parameter.name | escapeKeyword | caseSnake }}) | ||
| {% if parameter.required %} | ||
| # 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) | ||
| {% else %} | ||
| # Optional path parameters - only include if not None | ||
| if {{ parameter.name | escapeKeyword | caseSnake }} is not None: | ||
| api_path = api_path.replace('{{ '{' }}{{ parameter.name | caseCamel }}{{ '}' }}', str({{ parameter.name | escapeKeyword | caseSnake }})) | ||
| {% endif %} | ||
| {% endfor %} | ||
|
|
||
| {% for parameter in method.parameters.query %} | ||
| api_params['{{ parameter.name }}'] = {{ parameter.name | escapeKeyword | caseSnake }} | ||
| {% if parameter.required %} | ||
| # Required query parameters - convert None to explicit null | ||
| if {{ parameter.name | escapeKeyword | caseSnake }} is None: | ||
| api_params['{{ parameter.name }}'] = self.client.null() | ||
| else: | ||
| api_params['{{ parameter.name }}'] = {{ parameter.name | escapeKeyword | caseSnake }} | ||
| {% else %} | ||
| # Optional query parameters - only include if not None | ||
| if {{ parameter.name | escapeKeyword | caseSnake }} is not None: | ||
| api_params['{{ parameter.name }}'] = {{ parameter.name | escapeKeyword | caseSnake }} | ||
| {% endif %} | ||
| {% endfor %} | ||
|
|
||
| {% for parameter in method.parameters.body %} | ||
| {% if parameter.required %} | ||
| # Required body parameters - convert None to explicit null | ||
| if {{ parameter.name | escapeKeyword | caseSnake }} is None: | ||
| api_params['{{ parameter.name }}'] = self.client.null() | ||
| else: | ||
| {% if method.consumes[0] == "multipart/form-data" and ( parameter.type != "string" and parameter.type != "array" ) %} | ||
| api_params['{{ parameter.name }}'] = str({{ parameter.name | escapeKeyword | caseSnake }}).lower() if type({{ parameter.name | escapeKeyword | caseSnake }}) is bool else {{ parameter.name | escapeKeyword | caseSnake }} | ||
| api_params['{{ parameter.name }}'] = str({{ parameter.name | escapeKeyword | caseSnake }}).lower() if isinstance({{ parameter.name | escapeKeyword | caseSnake }}, bool) else {{ parameter.name | escapeKeyword | caseSnake }} | ||
| {% else %} | ||
| api_params['{{ parameter.name }}'] = {{ parameter.name | escapeKeyword | caseSnake }} | ||
| api_params['{{ parameter.name }}'] = {{ parameter.name | escapeKeyword | caseSnake }} | ||
| {% endif %} | ||
| {% else %} | ||
| # Optional body parameters - only include if not None | ||
| if {{ parameter.name | escapeKeyword | caseSnake }} is not None: | ||
| {% if method.consumes[0] == "multipart/form-data" and ( parameter.type != "string" and parameter.type != "array" ) %} | ||
| api_params['{{ parameter.name }}'] = str({{ parameter.name | escapeKeyword | caseSnake }}).lower() if isinstance({{ parameter.name | escapeKeyword | caseSnake }}, bool) else {{ parameter.name | escapeKeyword | caseSnake }} | ||
| {% else %} | ||
| api_params['{{ parameter.name }}'] = {{ parameter.name | escapeKeyword | caseSnake }} | ||
| {% endif %} | ||
| {% endif %} | ||
| {% endfor %} | ||
|
|
||
| {% for parameter in method.parameters.formData %} | ||
| {% if parameter.required %} | ||
| # Required form data parameters - convert None to explicit null | ||
| if {{ parameter.name | escapeKeyword | caseSnake }} is None: | ||
| api_params['{{ parameter.name }}'] = self.client.null() | ||
| else: | ||
| {% if method.consumes[0] == "multipart/form-data" and ( parameter.type != "string" and parameter.type != "array" ) %} | ||
| api_params['{{ parameter.name }}'] = str({{ parameter.name | escapeKeyword | caseSnake }}).lower() if isinstance({{ parameter.name | escapeKeyword | caseSnake }}, bool) else {{ parameter.name | escapeKeyword | caseSnake }} | ||
| {% else %} | ||
| api_params['{{ parameter.name }}'] = {{ parameter.name | escapeKeyword | caseSnake }} | ||
| {% endif %} | ||
| {% else %} | ||
| # Optional form data parameters - only include if not None | ||
| if {{ parameter.name | escapeKeyword | caseSnake }} is not None: | ||
| {% if method.consumes[0] == "multipart/form-data" and ( parameter.type != "string" and parameter.type != "array" ) %} | ||
| api_params['{{ parameter.name }}'] = str({{ parameter.name | escapeKeyword | caseSnake }}).lower() if type({{ parameter.name | escapeKeyword | caseSnake }}) is bool else {{ parameter.name | escapeKeyword | caseSnake }} | ||
| api_params['{{ parameter.name }}'] = str({{ parameter.name | escapeKeyword | caseSnake }}).lower() if isinstance({{ parameter.name | escapeKeyword | caseSnake }}, bool) else {{ parameter.name | escapeKeyword | caseSnake }} | ||
| {% else %} | ||
| api_params['{{ parameter.name }}'] = {{ parameter.name | escapeKeyword | caseSnake }} | ||
| {% endif %} | ||
| {% endif %} | ||
| {% endfor %} | ||
|
|
||
| {% for parameter in method.parameters.header %} | ||
| {% 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 }}) | ||
| {% 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 %} | ||
|
Comment on lines
+86
to
95
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don’t send explicit nulls in HTTP headers. 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 }})
🤖 Prompt for AI Agents |
||
| {% endfor %} | ||
| {% endif %} | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,4 +5,4 @@ | |||||||||||||
| {% for key, header in method.headers %} | ||||||||||||||
| '{{ key }}': '{{ header }}', | ||||||||||||||
| {% endfor %} | ||||||||||||||
|
Comment on lines
5
to
7
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Normalize static header keys to lowercase to match client checks. Apply this diff: -{% for key, header in method.headers %}
- '{{ key }}': '{{ header }}',
-{% endfor %}
+{% for key, header in method.headers %}
+ '{{ key | lower }}': '{{ header }}',
+{% endfor %}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||
| }, api_params{% if method.type == 'webAuth' %}, response_type='location'{% endif %}) | ||||||||||||||
| }, api_params, nullable_params{% if method.type == 'webAuth' %}, response_type='location'{% endif %}) | ||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,19 @@ class Client: | |
| {% endfor %} | ||
| } | ||
|
|
||
| @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})() | ||
|
|
||
|
Comment on lines
+28
to
+40
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Explicit‑null detection is broken (always False). 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 |
||
| def set_self_signed(self, status=True): | ||
| self._self_signed = status | ||
| return self | ||
|
|
@@ -50,28 +63,41 @@ class Client: | |
| return self | ||
| {% endfor %} | ||
|
|
||
| def call(self, method, path='', headers=None, params=None, response_type='json'): | ||
| 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} | ||
|
|
||
|
Comment on lines
+66
to
86
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Normalize header keys and make content-type checks robust. 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 |
||
| data = {} | ||
| files = {} | ||
| stringify = False | ||
|
|
||
| headers = {**self._global_headers, **headers} | ||
|
|
||
| # Move params to data for non-GET requests | ||
| if method != 'get': | ||
| data = params | ||
| params = {} | ||
|
|
||
| # Handle JSON content | ||
| if headers['content-type'].startswith('application/json'): | ||
| data = json.dumps(data, cls=ValueClassEncoder) | ||
|
|
||
| # Handle multipart form data | ||
| if headers['content-type'].startswith('multipart/form-data'): | ||
| del headers['content-type'] | ||
| stringify = True | ||
|
|
@@ -81,9 +107,10 @@ class Client: | |
| del data[key] | ||
| data = self.flatten(data, stringify=stringify) | ||
|
|
||
| # Make the HTTP request | ||
| response = None | ||
| try: | ||
| response = requests.request( # call method dynamically https://stackoverflow.com/a/4246075/2299554 | ||
| response = requests.request( | ||
| method=method, | ||
| url=self._endpoint + path, | ||
| params=self.flatten(params, stringify=stringify), | ||
|
|
@@ -96,22 +123,25 @@ class Client: | |
|
|
||
| response.raise_for_status() | ||
|
|
||
| # Handle warnings | ||
| warnings = response.headers.get('x-{{ spec.title | lower }}-warning') | ||
| if warnings: | ||
| for warning in warnings.split(';'): | ||
| print(f'Warning: {warning}', file=sys.stderr) | ||
|
|
||
| content_type = response.headers['Content-Type'] | ||
|
|
||
| # Handle different response types | ||
| if response_type == 'location': | ||
| return response.headers.get('Location') | ||
|
|
||
| if content_type.startswith('application/json'): | ||
| return response.json() | ||
|
|
||
| return response._content | ||
|
|
||
| except Exception as e: | ||
| if response != None: | ||
| if response is not None: | ||
| content_type = response.headers['Content-Type'] | ||
| if content_type.startswith('application/json'): | ||
| raise {{spec.title | caseUcfirst}}Exception(response.json()['message'], response.status_code, response.json().get('type'), response.text) | ||
|
|
@@ -203,6 +233,7 @@ class Client: | |
| return result | ||
|
|
||
| def flatten(self, data, prefix='', stringify=False): | ||
| """Flatten a nested dictionary/list into a flat dictionary with dot notation.""" | ||
| output = {} | ||
| i = 0 | ||
|
|
||
|
|
@@ -212,7 +243,12 @@ class Client: | |
| finalKey = prefix + '[' + str(i) +']' if isinstance(data, list) else finalKey | ||
| i += 1 | ||
|
|
||
| if isinstance(value, list) or isinstance(value, dict): | ||
| if value is None or self._is_explicitly_null(value): # Handle null values | ||
| if stringify: | ||
| output[finalKey] = '' | ||
| else: | ||
| output[finalKey] = None | ||
| elif isinstance(value, (list, dict)): | ||
| output = {**output, **self.flatten(value, finalKey, stringify)} | ||
| else: | ||
| if stringify: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 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:
💡 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):
What to do instead:
Sources:
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