Skip to content
This repository has been archived by the owner on Aug 19, 2023. It is now read-only.

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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cmcarthur
Copy link

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
Copy link
Owner

s-knibbs commented Jun 14, 2019

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants