Remove optional generated class attributes set to None from serialized payload

[bowenwr]

Is your feature request related to a problem? Please describe.

Given a generated class like:

class MyModel:
  required_property: str
  optional_property: Optional[Dict[Any, Any]]

When communicating with an API, the requests are getting rejected for certain optional properties in which we're sending None / null which causes 400 Bad Request like:

Invalid input at "optional_property": None is not of type 'object'

The idea being that the API is expecting if a field is present, it has a non-null value. Not sending optional_property solves the problem.

Describe the solution you'd like

I'd like to have an option to only serialize fields which have a value unless they are marked required, where it's assumed None/null would have semantic value. I'm suggesting a configuration option as I realize this behavior may break other cases where folks may be relying on always sending None now.

The result would be that the generated to_dict() method in the classes only adds the key/value if the key is required or is not None.

Describe alternatives you've considered

Currently we are extending the generated classes and overriding the to_dict() method to perform the desired behavior.

[bowenwr]

Speaking about this with @dtkav I think a solution for places where None/null is semantically meaningful might be to use a sentinel type like:

optional_property: Optional[Union[Dict[Any, Any], NullValue]]

This would allow the user to explicitly set NullValue to show intent when we did want to send None in the body.

[dbanty]

I like the general idea, but I'd go in the opposite direction. None semantically already means null, and Optional (as unfortunate as the naming is...) means nullable. So I think we add our own semantics for unset with the sentinel, and have our to_dict skip anything that's unset.

class MyModel:
  normal: str
  nullable: Optional[str]
  optional: Union[str, Unset] = UNSET
  optional_nullable: Union[str, Unset, None] = UNSET
  optional_with_default: Union[str, Unset] = "default"

Not completely sure on how to do a type annotation for unset- if UNSET is an object I believe Unset needs to be the type of it, so we might have to have a dummy type?

[dtkav]

I think dataclasses captures this concept with MISSING and attrs uses NOTHING
(see: https://github.com/python-attrs/attrs/blob/master/src/attr/_make.py#L67 )

Our use case for explicit null is pretty rare. It comes up for fields that are required: false, nullable: true and when the server treats presence and a null value differently.

Most json REST APIs that I've worked with don't distinguish between required and nullable on reads (i.e. they always return all fields, or if they didn't it would not be semantically meaningful), though some do on writes (explicitly patching a value to null) which is the scenario we're running into.
( Related: https://opensource.zalando.com/restful-api-guidelines/#123 )

Either way, it seems wise here to follow the patterns used by dataclasses/attrs and follow their interpretation of None / NOTHING/MISSING.

I agree the naming of Optional is unfortunate indeed. I also think it would be unfortunate to introduce a new UNSET type that is specific to openapi-python-client, preferring to reuse the attrs NOTHING type if possible.

Here's a related stackoverflow answer from hynek (author of attrs) that also confirms that Optional means nullable: true, not required: false.

[dbanty]

My first thought was to use the attrs NOTHING but I believe if any field is instantiated with that value attrs will throw an exception, as I think it's used for things declared with attr.ib() without a default value. I haven't actually tested it, so it may be possible to reuse it. If it is, I will do so.

[bowenwr]

Accidentally closed this when merging out version to the fork. Leaving open here to evaluate if we want to port that design.

[emann]

Did some testing using attr.NOTHING as a default value and @dbanty is correct, the way attrs constructs the class interprets doing so as "this field has no default value" and thus complains on instantiation that the init is missing an arg for the field. I think @bowenwr's implementation of UNSET is probably the best way forwards.

[emann]

@bowenwr I took some inspiration from your implementation of UNSET and made #230 which also addresses #228, take a look and let me know what you think!