Skip to content
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.

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

Permit auto generation of definition schema names #167

Merged
merged 12 commits into from Dec 9, 2017

Conversation

@lebouquetin
Copy link

commented Nov 8, 2017

The case we faced is the following one:

  • we have a self referencing schema, lets call it NodeSchema.
  • this schema is included in another schema - NodeList

When we try to define NodeList in APIspec, the NodeSchema is not yet reference, so its structure is inlined. As it is self referencing, the inline results in an infinite recursion, so it crashes

The global solution is to add definitions of our Schema in the right order. As we use APIspec in hapic (a generic auto-documented REST API framework), we can't assume that schema will be defined in the right order for APIspec.

There are two ways to get schemas defined in the right order:

  1. first solution is to preprocess all the schema we have to document in order to add thems to apispec in the right order. It's not very elegant (but it works)
  2. second solution is to define the rule to define name of a schema in the auto-generated documentation. This is quite easy to integrate in APIspec - this is the current PR.

How it works:

  • if you do nothing special, APIspec keep on working as it already do.
  • if you define a callable schema_name_resolver_callable at construction time of APISpec, then it will be used for auto-referencing required schemas.

We'd be happy this PR to be accepted fast, let us know if you are ok and/or if we should change some stuff for the PR to be accepted (naming, etc).

Damien


# Auto reference schema if available
if schema_cls not in plug.get('refs', {}):
if spec.schema_name_resolver_callable:

This comment has been minimized.

Copy link
@yoichi

yoichi Nov 9, 2017

Collaborator

spec can be None here (travis-ci test fails)

This comment has been minimized.

Copy link
@lebouquetin

lebouquetin Nov 9, 2017

Author

@buxx could you upgrade our code for fix ?

This comment has been minimized.

Copy link
@buxx
@@ -91,15 +91,25 @@ class APISpec(object):
See https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#infoObject
:param \*\*dict options: Optional top-level keys
See https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#swagger-object
:param callable schema_name_resolver_callable: callable who generate the schema definition name

This comment has been minimized.

Copy link
@yoichi

yoichi Nov 9, 2017

Collaborator

What is the input and output of this callable? Could you add a test code to illustrate its usage?

This comment has been minimized.

Copy link
@lebouquetin

lebouquetin Nov 9, 2017

Author

@yoichi good question and query. @buxx you upgrade the documentation and add a test code to illustrate?

This comment has been minimized.

Copy link
@buxx

buxx Nov 9, 2017

Contributor

@lebouquetin I do that.

This comment has been minimized.

Copy link
@buxx

buxx Nov 10, 2017

Contributor

done

@yoichi

yoichi approved these changes Nov 10, 2017

@yoichi

This comment has been minimized.

Copy link
Collaborator

commented Nov 10, 2017

Thanks for clarifying the spec. I think this feature is useful.

  • It would be nice if we have a test code for a new feature in order to avoid future regression.
  • "schema_name_resolver_callable" is a bit long. Does it make sense if simply "schema_name_resolver"?
  • How about skipping spec.definition() call if "not definition_name"?
@buxx

This comment has been minimized.

Copy link
Contributor

commented Nov 10, 2017

@yoichi For your differents points: no problem, i will do these changes next week :-)

@buxx

This comment has been minimized.

Copy link
Contributor

commented Nov 13, 2017

@yoichi It's seems we have a problem with python2.7: when json.dumps (used in tests) we got an RuntimeError: dictionary changed size during iteration. I guess it's because we add definitions when reading them (i don't know why it is working in python 3.x).
I don't found a smart solution. Maybe try to execute the code who parse definitions before ? Or disable this feature for python < 3.x ?

@buxx

This comment has been minimized.

Copy link
Contributor

commented Nov 17, 2017

@yoichi I found another way to implement this feature: auto-reference schemas when add a definition. Solve the problem of dynamic evolution of definition dict. Tests are green and all nested schemas are auto referenced correctly.

@buxx

This comment has been minimized.

Copy link
Contributor

commented Dec 7, 2017

Hello @yoichi @sloria we have no news since last month about this pull request. any problem with it ?

@buxx buxx force-pushed the algoo:dev branch from c68d7eb to f1e1a1c Dec 7, 2017

@sloria

This comment has been minimized.

Copy link
Member

commented Dec 7, 2017

@buxx Thanks for your patience and for getting this up to date. I'll try to take a look at this this weekend.

@sloria sloria merged commit 61939ff into marshmallow-code:dev Dec 9, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sloria

This comment has been minimized.

Copy link
Member

commented Dec 9, 2017

Thanks for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.