Add appropriate nullable flag depending on OpenAPI version#197
Conversation
| if not field.allow_none: | ||
| return {} | ||
|
|
||
| if spec and spec.openapi_version.version[0] == 3: |
There was a problem hiding this comment.
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.
|
|
||
| if field.allow_none: | ||
| ret['x-nullable'] = True | ||
| ret.update(field2nullable(field, spec)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
I just proposed a PR that does this and more: #199. |
|
|
3 participants
Addresses issue identified by @lafrech in #165