Skip to content

Add appropriate nullable flag depending on OpenAPI version #197

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Add appropriate nullable flag depending on OpenAPI version #197

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions apispec/ext/marshmallow/swagger.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,23 @@ def field2length(field):
return attributes


def field2nullable(field, spec):
"""Returns the dictionary with an appropriate nullable key depending on
the OpenAPI version. Returns an empty dict if the field is not nullable

:param Field field: A marshmallow field.
:param spec: apispec.core.APISpec instance
:rtype: dict
"""
if not field.allow_none:
return {}

if spec and spec.openapi_version.version[0] == 3:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spec.openapi_version.version[0]

This is accessed in two places, and more to come. Maybe we should add a shortcut for it.
For instance properties spec.major_version, spec.minor_version.

Nothing blocking, just a thought.

return {'nullable': True}

return {'x-nullable': True}


# Properties that may be defined in a field's metadata that will be added to the output
# of field2property
# https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#schemaObject
Expand Down Expand Up @@ -293,8 +310,7 @@ def field2property(field, spec=None, use_refs=True, dump=True, name=None):
if field.dump_only:
ret['readOnly'] = True

if field.allow_none:
ret['x-nullable'] = True
ret.update(field2nullable(field, spec))
Copy link
Member

@lafrech lafrech Apr 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit bothered by the fact that the spec attribute to the function is optional and defaults to None.

Same concern for get_ref_path, BTW. Even worse then because we don't check it is not None.

Maybe we should make spec mandatory. It breaks a lot of tests, but maybe not real-life use cases (otherwise, you'd get an AttributeError in the call to get_ref_path).

We should give this a thought and maybe refactor a bit. Check what spec is used for and decide what needs to be passed. From what I gathered, we only use OpenAPI version, the list of schema definitions (refs) and an optional schema_name_resolver callable). Maybe we don't need to carry the whole spec object all around.

Anyway, this issue is much wider than this specific PR, so I suppose we can merge this and open a separate issue for the spec attribute refactor.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making spec mandatory everywhere is probably the right first step. Practically the entry points to all of these functions (schema_definition_helper schema_path_helper and schema_operation_resolver) have spec as mandatory so it's only the tests that break.

I think there is definitely some opportunity for refactoring. We have quite a bit of complexity around multiple ways of handling nested schemas (using refs, inlining, waiting to see if a ref gets added later, immediately resolving a name). Simplifying that would probably help in other areas.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, totally.

I'm not sure the whole spec needs to be carried about, but I think the version should be everywhere, as it may affect anything, so it should be provided to all helpers.

Anyway, we can merge this PR and focus on the refactor later on.


ret.update(field2range(field))
ret.update(field2length(field))
Expand Down
12 changes: 12 additions & 0 deletions tests/test_swagger.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,18 @@ def test_field_with_allow_none(self):
res = swagger.field2property(field)
assert res['x-nullable'] is True

def test_field_with_allow_none_v3(self):
spec = APISpec(
title='Pets',
version='0.1',
plugins=['apispec.ext.marshmallow'],
openapi_version='3.0.0'
)
field = fields.Str(allow_none=True)
res = swagger.field2property(field, spec)
assert res['nullable'] is True


class TestMarshmallowSchemaToModelDefinition:

def test_invalid_schema(self):
Expand Down