Skip to content
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

[BUG] Null fields dropped from HTTP request bodies #19067

Open
5 of 6 tasks
Ichimonji10 opened this issue Jul 3, 2024 · 0 comments · May be fixed by #19068
Open
5 of 6 tasks

[BUG] Null fields dropped from HTTP request bodies #19067

Ichimonji10 opened this issue Jul 3, 2024 · 0 comments · May be fixed by #19068

Comments

@Ichimonji10
Copy link
Contributor

Ichimonji10 commented Jul 3, 2024

Bug Report Checklist

  • Have you provided a full/minimal spec to reproduce the issue?
  • Have you validated the input using an OpenAPI validator (example)?
  • Have you tested with the latest master to confirm the issue still exists?
  • Have you searched for related issues/PRs?
  • What's the actual output vs expected output?
  • [Optional] Sponsorship to speed up the bug fix or feature request (example)
Description

Null fields are dropped from HTTP request bodies. This is especially problematic for HTTP PATCH and PUT requests, where null values are semantically important.

Imagine you have a movie logging service, used to log movies you watch. You log having watched Blade and The Matrix, but then decide to clear their ratings so you can mull over your ratings a bit more. Here's how you might accomplish that with HTTP PUT and PATCH:

method  path    target body                          actual body
PUT     /log/1  {"name": "Blade", "rating": 5}       {"name": "Blade", "rating": 5}
PUT     /log/1  {"name": "Blade", "rating": null}    {"name": "Blade"}
PUT     /log/2  {"name": "The Matrix", "rating": 3}  {"name": "The Matrix", "rating": 3}
PATCH   /log/2  {"rating": null}                     {}

The problem, as shown by the "actual body" column above, is that openapi-generator-cli v7.6.0 python bindings drop null fields from requests. Therefore, it's impossible to make HTTP PUT and PATCH requests which explicitly clear fields. The root cause is this line in the python API client:

obj_dict = obj.model_dump(by_alias=True, exclude_unset=True)

Per BaseModel.model_dump(), the exclude_none controls the following behavior:

Whether to exclude fields that have a value of None.

openapi-generator version

This issue is present in "python" bindings generated by openapi-generator-cli v7.6.0 and latest. Earlier versions of openapi-generator-cli shipped a very different python code generator.

OpenAPI declaration file content or url

https://git.sr.ht/~jaudet/impedimenta/tree/main/item/openapi/sample-project/openapi/spec.yml

Generation Details

To generate API bindings:

git clone https://git.sr.ht/~jaudet/impedimenta
cd impedimenta/openapi/sample-project
./openapi/gen-bindings.sh
Steps to reproduce

To reproduce the issue, execute the above, then:

python3 -m unittest sample_project.tests

Several unit tests demonstrating the behavior will fail.

Related issues/PRs

No.

Suggest a fix

There are several possible solutions. One might be to apply this diff:

diff --git a/modules/openapi-generator/src/main/resources/python/api_client.mustache b/modules/openapi-generator/src/main/resources/python/api_client.mustache
index c7d4a6d700f..61d91641dc5 100644
--- a/modules/openapi-generator/src/main/resources/python/api_client.mustache
+++ b/modules/openapi-generator/src/main/resources/python/api_client.mustache
@@ -371,19 +371,12 @@ class ApiClient:
             )
         elif isinstance(obj, (datetime.datetime, datetime.date)):
             return obj.isoformat()
-
         elif isinstance(obj, dict):
             obj_dict = obj
+        elif hasattr(obj, "model_dump") and callable(obj.model_dump):
+            obj_dict = obj.model_dump(by_alias=True, exclude_unset=True)
         else:
-            # Convert model obj to dict except
-            # attributes `openapi_types`, `attribute_map`
-            # and attributes which value is not None.
-            # Convert attribute name to json key in
-            # model definition for request.
-            if hasattr(obj, 'to_dict') and callable(getattr(obj, 'to_dict')):
-                obj_dict = obj.to_dict()
-            else:
-                obj_dict = obj.__dict__
+            raise TypeError(f"Can't serialize obj: {obj}")
 
         return {
             key: self.sanitize_for_serialization(val)

As an aside, there's quite a bit of serialization logic in the python API client and models. This seems odd, given that pydantic models include logic for serialization via model_dump() and similar.

Ichimonji10 added a commit to Ichimonji10/openapi-generator that referenced this issue Jul 3, 2024
Null request fields are dropped from HTTP request bodies. This is
especially problematic for HTTP PATCH and PUT requests, where null
values are semantically important.

Fix: OpenAPITools#19067
Ichimonji10 added a commit to Ichimonji10/openapi-generator that referenced this issue Jul 3, 2024
The python API bindings drop null request field from HTTP request
bodies. This is especially problematic for HTTP PUT and PATCH requests,
where null values are semantically important.

Change the behavior of the bindings:

```python
# before
model_dump(by_alias=True, exclude=excluded_fields, exclude_none=True)

# after
model_dump(by_alias=True, exclude_unset=True)
```

The effect of this change is to make the API send data to the remote
server that hews more closely to the original model. This is important
when making HTTP PUT and PATCH requests:

```python
# model describing a movie you've watched
class LogEntry(BaseModel):
    name: str
    rating: int | None = None

# create log entry
log_entry = LogEntry(name="Blade", rating=5)
api_client.put_log_entry(1, log_entry)

# clear rating; PUT {"name": "Blade", "rating": null}
log_entry = LogEntry(name="Blade", rating=None)
api_client.put_log_entry(1, log_entry)

# clear rating; PATCH {"rating": null}
log_entry = LogEntry.model_construct(rating=None)
api_client.patch_log_entry(1, log_entry)
```

Before this change, the API client would omit `"rating": null` from
request bodies.

Fix: OpenAPITools#19067
@Ichimonji10 Ichimonji10 linked a pull request Jul 3, 2024 that will close this issue
@Ichimonji10 Ichimonji10 changed the title [BUG] Description [BUG] Null fields dropped from HTTP request bodies Jul 3, 2024
Ichimonji10 added a commit to Ichimonji10/openapi-generator that referenced this issue Jul 8, 2024
The python API bindings drop null request field from HTTP request
bodies. This is especially problematic for HTTP PUT and PATCH requests,
where null values are semantically important.

Change the behavior of the bindings:

```python
# before
model_dump(by_alias=True, exclude=excluded_fields, exclude_none=True)

# after
model_dump(by_alias=True, exclude_unset=True)
```

The effect of this change is to make the API send data to the remote
server that hews more closely to the original model. This is important
when making HTTP PUT and PATCH requests:

```python
# model describing a movie you've watched
class LogEntry(BaseModel):
    name: str
    rating: int | None = None

# create log entry
log_entry = LogEntry(name="Blade", rating=5)
api_client.put_log_entry(1, log_entry)

# clear rating; PUT {"name": "Blade", "rating": null}
log_entry = LogEntry(name="Blade", rating=None)
api_client.put_log_entry(1, log_entry)

# clear rating; PATCH {"rating": null}
log_entry = LogEntry.model_construct(rating=None)
api_client.patch_log_entry(1, log_entry)
```

Before this change, the API client would omit `"rating": null` from
request bodies.

Fix: OpenAPITools#19067
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant