Skip to content

New behavior around optional nested objects breaks deserialization when explicit null is returned #343

Closed
Closed
New behavior around optional nested objects breaks deserialization when explicit null is returned#343
Assignees
dbanty
Labels
🐞bugSomething isn't working
Milestone
0.9.0

Description

@joshzana
joshzana
opened on Mar 3, 2021

Describe the bug
We use FastAPI as an API server and generate a python client with open-api-client that is then used to run integration tests. Our team is using 0.7.3 but I was trying to update to 0.8.0. When doing so, we ran into the following exception running our test suite:

/usr/local/lib/python3.8/site-packages/falkon_sdk/api/observations/get_observations.py:73: in _parse_response
    response_200 = ObservationResult.from_dict(response.json())
/usr/local/lib/python3.8/site-packages/falkon_sdk/models/observation_result.py:104: in from_dict
    metric_change = ObservationFactWindowSummaryResource.from_dict(
/usr/local/lib/python3.8/site-packages/falkon_sdk/models/observation_fact_window_summary_resource.py:156: in from_dict
    weighted_change_unfiltered = WeightedChange.from_dict(
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

cls = <class 'falkon_sdk.models.weighted_change.WeightedChange'>
src_dict = None

    @classmethod
    def from_dict(cls: Type[T], src_dict: Dict[str, Any]) -> T:
>       d = src_dict.copy()
E       AttributeError: 'NoneType' object has no attribute 'copy'

I was able to narrow down the problem to handling of Optional[T] objects within response models. Here is a minimal repro:

To Reproduce
Steps to reproduce the behavior:

  1. Make a main.py file with the following contents:
from typing import Optional
from fastapi import FastAPI
from pydantic import BaseModel

app = FastAPI()

class ItemResource(BaseModel):
    id: int

class ResourceWithOptional(BaseModel):
    item: Optional[ItemResource]

@app.get("/", response_model=ResourceWithOptional)
def read_item():
    return ResourceWithOptional(item=None)
  1. Start it up in uvicorn
uvicorn main:app &
  1. Curling that endpoint returns this:
curl -s http://localhost:8000/
INFO:     127.0.0.1:32770 - "GET / HTTP/1.1" 200 OK
{"item":null}
  1. Generate a client for this:
openapi-python-client generate --url http://localhost:8000/openapi.json
  1. Look at the generated resource_with_optional.py in the from_dict method:
        item: Union[ItemResource, Unset] = UNSET
        _item = d.pop("item", UNSET)
        if not isinstance(_item, Unset):
            item = ItemResource.from_dict(_item)

Expected behavior
I expect that this allows for an explicit null to be returned, as was the case in openapi-python-client==0.7.3, where the generated code looks like this:

        item: Union[ItemResource, Unset] = UNSET
        _item = d.pop("item", UNSET)
        if _item is not None and not isinstance(_item, Unset):
            item = ItemResource.from_dict(cast(Dict[str, Any], _item))

OpenAPI Spec File

{"openapi":"3.0.2","info":{"title":"FastAPI","version":"0.1.0"},"paths":{"/":{"get":{"summary":"Read Item","operationId":"read_item__get","responses":{"200":{"description":"Successful Response","content":{"application/json":{"schema":{"$ref":"#/components/schemas/ResourceWithOptional"}}}}}}}},"components":{"schemas":{"ItemResource":{"title":"ItemResource","required":["id"],"type":"object","properties":{"id":{"title":"Id","type":"integer"}}},"ResourceWithOptional":{"title":"ResourceWithOptional","type":"object","properties":{"item":{"$ref":"#/components/schemas/ItemResource"}}}}}}

Desktop (please complete the following information):

  • OS: Ubuntu Linux 20.04
  • Python Version: 3.8.6
  • openapi-python-client version 0.8.0

Additional context
FastAPI has support for excluding nulls from responses with response_model_exclude_none but this means changing how our API works to suit the sdk generator, which I would prefer not to have to do.
The new behavior I was referring to seems to have come in with #334

Activity

  1. joshzana
    added
    🐞bugSomething isn't working
    on Mar 3, 2021

  2. dbanty commented on Mar 3, 2021

    @dbanty
    dbanty
    on Mar 3, 2021
    Collaborator

    Thanks for filing the bug report! I'll try to get a fix in for this by the end of the week 😊

  3. dbanty
    self-assigned this
    on Mar 3, 2021
  4. dbanty
    added this to the 0.8.1 milestone on Mar 3, 2021

  5. joshzana commented on Mar 3, 2021

    @joshzana
    joshzana
    on Mar 3, 2021
    Author

    Thanks @dbanty - is #332 by any chance a fix to this?

  6. dbanty commented on Mar 3, 2021

    @dbanty
    dbanty
    on Mar 3, 2021
    Collaborator

    It's certainly possible, that PR does a lot to clean up that code. If you get a chance to check out that branch and confirm, that would be great! I plan on also re-reviewing that PR later this week, but there's been a lot of back and forth on it so far so I don't know how long until it's ready to merge.

  7. dbanty commented on Mar 16, 2021

    @dbanty
    dbanty
    on Mar 16, 2021
    Collaborator

    Hey @joshzana now that #332 is done, I took a deeper look at this. Turns out this is actually working as intended. In 0.8.0 we tightened up our required and nullable handling, so the previous behavior which happened to be working for you in 0.7.3 was actually incorrect.

    In OpenAPI, there is a difference between something being not-required and something being nullable. If something is not required (as is the case with your example), it's possible that it will not be included at all. If something is nullable, it's possible for it to have an explicit null value. In this case, the schema looks like this:

    {
    "title": "ResourceWithOptional",
    "type": "object",
    "properties": {
        "item": { "$ref": "#/components/schemas/ItemResource" }
    }
}

    Where that item property is not required (if it were, there would be a "required": ["item"] in the schema) meaning it can be omitted. However, item is also not nullable (if it were there would be a "nullable": true in the declaration of item) meaning explicit null is not allowed and therefore not expected.

    I've run into this issue quite a bit using FastAPI in the back end and TypeScript in the front end. If something is not required it can be undefined whereas if something is nullable it can be null. With Pydantic, declaring a field as Optional[T] means it's not required (can be omitted), but does not set nullable. If you want to pass an explicit null you need to declare your field like this:

    from pydantic import Field

class ResourceWithOptional(BaseModel):
    item: Optional[ItemResource] = Field(None, nullable=True)

    If you are never going to omit the field, but rather always pass explicit null, then you can also include a required=True param in there to get rid of the Unset (in our generated clients) or undefined (in TS) option.

    That was a long-winded explanation, but I know this particular facet of OpenAPI can be very confusing, especially when working from Python where there is only 1 type of null. I'm going to close this issue, but let me know if you have any questions, I'm more than happy to help!

  8. dbanty
    closed this as completedon Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

🐞bugSomething isn't working

Type

No type

Projects

No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions