Repository navigation
Remove optional generated class attributes set to None from serialized payload #205
Description
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.
Activity
[-]Remove optional generated class attributes set to `None` from serialized payload[/-][+]Remove optional generated class attributes set to None from serialized payload[/+]bowenwr commentedon Oct 2, 2020
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:
This would allow the user to explicitly set
NullValueto show intent when we did want to sendNonein the body.dbanty commentedon Oct 4, 2020
I like the general idea, but I'd go in the opposite direction.
Nonesemantically already meansnull, andOptional(as unfortunate as the naming is...) means nullable. So I think we add our own semantics for unset with the sentinel, and have ourto_dictskip anything that's unset.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 commentedon Oct 5, 2020
I think dataclasses captures this concept with
MISSINGand attrs usesNOTHING(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: trueand when the server treats presence and a null value differently.Most json REST APIs that I've worked with don't distinguish between
requiredandnullableon 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 tonull) 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
Optionalis unfortunate indeed. I also think it would be unfortunate to introduce a newUNSETtype 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, notrequired: false.dbanty commentedon Oct 5, 2020
My first thought was to use the attrs
NOTHINGbut I believe if any field is instantiated with that value attrs will throw an exception, as I think it's used for things declared withattr.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 commentedon Oct 30, 2020
Accidentally closed this when merging out version to the fork. Leaving open here to evaluate if we want to port that design.
emann commentedon Nov 2, 2020
Did some testing using
attr.NOTHINGas 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 ofUNSETis probably the best way forwards.emann commentedon Nov 2, 2020
@bowenwr I took some inspiration from your implementation of
UNSETand made #230 which also addresses #228, take a look and let me know what you think!