fixing #37 issue support of definitions for nested objects#38
fixing #37 issue support of definitions for nested objects#38xxmatyuk wants to merge 20 commits intocore-api:masterfrom
Conversation
xxmatyuk
commented
Jun 11, 2017
xxmatyuk
commented
- added definitions
- arrays handled properly
|
@tomchristie I think it's ready to be merged. |
openapi_codec/encode.py
Outdated
|
|
||
| def _get_definitions(document): | ||
|
|
||
| definitions = dict() |
openapi_codec/encode.py
Outdated
| } | ||
| else: | ||
| definition_data['properties'][f_name] = { | ||
| 'type': _get_field_type(f_schema), |
There was a problem hiding this comment.
type can also be an array here. Now items is missing and the swagger-ui (version 2) breaks.
There was a problem hiding this comment.
i suppose it needs something like
elif _get_field_type(f_schema) == 'array':
_item_name = '{}_item'.format(f_name)
defs[_item_name] = get_field_def_data(f_schema.items, defs)
definition_data['properties'][f_name] = {
'type': 'array',
'items': {'$ref': '#/definitions/{}'.format(_item_name)}
}
There was a problem hiding this comment.
The usage of f_name confuses me here. There can be multiple fields with the same name (in different paths/objects) but different array member type in a schema, so this approach will definitely fail in such case.
There was a problem hiding this comment.
yes. name clashes shoudl be a problem in all of this PR.
There was a problem hiding this comment.
Okay, let me figure that out. I thought it was useful and nobody even start it, but as soon I did it turns out everyone, but me knows how to do it better =)
There was a problem hiding this comment.
@xxmatyuk No offense =), I tried 2 other PRs for the same issue, and yours is still the best although still not quite ready.
There was a problem hiding this comment.
@tuffnatty no problems, that was looong hard day) I'm willing to fix it, so stay tuned. And yeah, thanks )
There was a problem hiding this comment.
@tuky @tuffnatty
guys, I'm making the changes as per you proposed to do, but could you please help me to find an example of the same name for definitions? So that I can write tests and perform changes real quick. Thanks.
There was a problem hiding this comment.
Say e.g. you have 4 serializers in 2 different files:
class AuthorSerializer(Serializer):
name = CharField()
class Song(Serializer):
author = AuthorSerializer()
co_authors = AuthorSerializer(many=True)class AuthorSerializer(Serializer):
birthday = DateField()
class Song(Serializer):
author = AuthorSerializer()The 2 different AuthorSerializer should get the same names when generated into CoreAPI format. Then we have a name clash, when reusing them as '#/definitions/author' or '#/definitions/co_authors_item' in OpenAPI.
There was a problem hiding this comment.
@tuky, thanks a lot! I wrote a test, reproduced the clash, working on fix.
openapi_codec/encode.py
Outdated
| } | ||
|
|
||
| if isinstance(field_item, coreschema.Object): | ||
| props = field_item.properties |
There was a problem hiding this comment.
since the recursion can also be an array of "simple" types, i would also suggest to add here:
elif isinstance(field_item, (coreschema.String, coreschema.Boolean, coreschema.Integer, coreschema.Number)):
return {'type': _get_field_type(field_item), 'description': _get_field_description(field_item)}
|
hey @xxmatyuk is this still inflight? It would be immensely helpful for a problem I'm running into - please let me know if I can help out at all! |
|
hey @joshowen! Now you gave me a strong reason to finish it =) I'll try to squeeze my duties and make it done, and for sure will let you know If any help's needed! |
|
Hey @xxmatyuk Please let us know if we can help out at all with this issue. |
|
test is failing due to some python3.X difference I'm looking into now (filter object is a problem). Though, works like a charm on python2.x. |
|
@tuffnatty @tuky @tovmeod now it's really ready for review, so feel free. |
| if _get_field_type(f_schema) is 'object': | ||
| def_data = _get_or_update_definitions( | ||
| _get_field_definition_data(f_schema, defs), | ||
| '{}_def_item'.format(f_schema.name), |
There was a problem hiding this comment.
I'm starting to play with this.
It works for a super simple API using nested definitions, but when I tried to apply it to our whole site I'm getting a 500 and the traceback is giving an AttributeError here saying the name property doesn't exist. I'll try to look into it later, but just a heads up. (I cloned your branch to nexleaf/python-openapi-codec and may contribute that way)
There was a problem hiding this comment.
Sorry, I'm going to be switching to drf-yasg as it already supports nested definitions.
There was a problem hiding this comment.
thanks for heads up. I'd rather close this one and switch to it either.
|
@xxmatyuk Did you find a solution for the field name definition conflict? |
7 participants
