s-knibbs / dataclasses-jsonschema

JSON schema generation from dataclasses
MIT License
167 stars 38 forks source link

Change behavior of typing.Optional (create optional, nullable field) #56

Open cmcarthur opened 5 years ago

cmcarthur commented 5 years ago

Hi @s-knibbs ,

Thanks for building and maintaining this package. I spent some time evaluating it today and noticed a surprising interaction between typing.Optional and how nullable fields are treated in JSON schema. So, I put together a small proof of concept of what a different approach might look like for discussion.

The thing that specifically surprised me is that according to the typing.Optional documentation, Optional is simply an alias for Union[..., None]. With defaults, this lets you null a field to indicate a value is not present, or apply a default value. With that in mind, I would expect the following code to work:

from dataclasses import dataclass
from dataclasses_jsonschema import JsonSchemaMixin, SchemaType
from typing import Optional, Union
from pprint import pprint

@dataclass
class DatabaseModel(JsonSchemaMixin):
    """Description"""
    id: Optional[int] = None

print(DatabaseModel.from_dict({}).to_dict()) # works great

print(DatabaseModel.from_dict({'id': None}).to_dict()) # fails

The above code fails with:

dataclasses_jsonschema.ValidationError: None is not of type 'integer'

Failed validating 'type' in schema['properties']['id']:
    {'type': 'integer'}

On instance['id']:
    None

Which is a somewhat surprising way to fail, given that the default value for id is None!


Digging into internals here, it seems like a better way to represent Optional[int] is similarly to how you represent Union[str, int], except with None/null as a first class type. This branch changes the schema generator to use oneOf: [{type: integer}, {type: null}] in addition to marking the field as not required. I think this better approximates how I'd expect the JSON schema to look. (although it would be a lot better if there were a way to separately define the field as required vs. nullable, I just don't think typing has a good syntax for that.)

One downside is that this branch in its current state breaks SWAGGER_V2 schema generation. I could modify this to conditionally revert to the old way of generating Optional schemas for SWAGGER_V2. Then this change would only apply to openapi 3 / swagger 3 / json schema.

Let me know what you think & thanks for your time.

s-knibbs commented 5 years ago

I need to improve the documentation and publish the api docs, but the default behaviour of to_dict is to filter out any None values (the omit_none parameter), so in your example above the following will work correctly:

d = DatabaseModel().to_dict()
DatabaseModel.from_dict(d)

It should be possible to support your use case but I don't think the default behaviour should be to generate nullable fields given that this isn't supported in swagger 2.0. I've proposed a solution in #57.