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

Flask DocumentedBlueprint: Add to spec all views defined in the blueprint. #27

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

JavierLuna
Copy link

Flask DocumentBlueprint

Is a blueprint which adds all views defined in it to the spec. You can ommit one view from being added to the spec setting documented=False in the Blueprint's route function (see usage below).

Usage

from flask import Flask
from flask.views import MethodView

app = Flask(__name__)

documented_blueprint = DocumentedBlueprint('gistapi', __name__)

@documented_blueprint.route('/gists/<gist_id>')
def gist_detail(gist_id):
    '''Gist detail view.
    ---
    x-extension: metadata
    get:
        responses:
            200:
                schema:
                    $ref: '#/definitions/Gist'
    '''
    return 'detail for gist {}'.format(gist_id)


@documented_blueprint.route('/repos/<repo_id>', documented=False)
def repo_detail(repo_id):
    '''This endpoint won't be documented
    ---
    x-extension: metadata
    get:
        responses:
            200:
                schema:
                    $ref: '#/definitions/Repo'
    '''
    return 'detail for repo {}'.format(repo_id)

app.register_blueprint(documented_blueprint)

print(spec.to_dict()['paths'])
# {'/gists/{gist_id}': {'get': {'responses': {200: {'schema': {'$ref': '#/definitions/Gist'}}}},
#                  'x-extension': 'metadata'}}

Has a test coverage of 100%.

@JavierLuna JavierLuna changed the title Flask DocumentBlueprint: Add to spec all views defined in the blueprint. Flask DocumentedBlueprint: Add to spec all views defined in the blueprint. Jan 12, 2019
.gitignore Outdated Show resolved Hide resolved
@sloria
Copy link
Member

sloria commented Jan 28, 2019

@tinproject @ergo Would you be up for reviewing this API, since you had some thoughts on #11?

@tinproject
Copy link

I hope I get more time on the weekend to take a proper look, this is just a quick read.

I'm not convinced of the extension by subclassing approach. I'm not sure what happen with MethodViews or other extensions of flask. Also if I remember well one problem is that one view function could have more than one route and should documented for every route, but I'm not very familiar with new openapi spec.

I'll try to try this PR on the weekend and write some code with it to clarify my thoughts.

@pcaro
Copy link

pcaro commented Feb 4, 2019

I like the idea implementation @JavierLuna but I found a little glitches:

  • It's not in the example or module documentation, but you need to pass the spec object to the DocumentedBlueprint constructor.
  • The route decorator appends a nested unnecessary function call (nested decorator)

@JavierLuna
Copy link
Author

I'll fix them as soon as I have a bit of free time.
Thanks @pcaro !

@tinproject
Copy link

I finally get some hours and take a look at this (it's been a year since I made something with apispec so I have to refresh my mind)

I've created a dumb project (at https://github.com/tinproject/apispec-webframeworks-test) and try the PR with different approach to add routes.

First had to said that the code of Javier is good, it only lacks support for Blueprint add_url_rule to be complete.

I saw two drawbacks:
1.- The subclassing approach force to inject the spec object, it could be a problem when having multiple separated blueprints, you will need some tricks to pass the spec object around and avoid circular references. Also add some toil to tests.
2.- Although it fulfills the issue #11 scope, it's more of a patch for this concrete case. It will be good to fix the FlaskPlugin one and for all, taking into account Flask/Werkzeug url routing, and also fix #14 with a complete solution. The good part of this PR is that is contained and could be easily deprecated and removed in the future if a proper solution is added finally to the project.

On the project referenced above I played around url routing in flask, trying to show the map/rules/endpoints/view_functions relationships if you want to take a peek.

self.documented_view_functions = defaultdict(list)
self.spec = spec

def route(self, rule, documented=True, **options):
Copy link
Author

Choose a reason for hiding this comment

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

I overrode this function to include the documented=True kwarg, as I like declaring them explicitly.

Do you like it this way or shall I remove this?

@JavierLuna
Copy link
Author

Thanks for checking it out and for the suggestions @tinproject !

I refactored a little the implementation and added some tests reflecting your project's cases.
add_url_rule is now supported 👍

Regarding your drawbacks:

1.- How would you implement it? The spec object is only needed when registering the application, maybe have it as spec=None in the __init__ method and then have some kind of attach(spec)?

2.- I looked around and it seems that it cannot be changed in this project and a change in the Apispec project will be needed. path_helper is expected by the APISpec object to return a single path, but in this case it is necessary to send multiple ones.
Shall I start working on it? What are your thoughts on the issue?

if documented:
self.documented_view_functions[rule].append(view_func)

def register(self, app, options, first_registration=False):
Copy link

Choose a reason for hiding this comment

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

For my usercase I prefer to pass the spec in the register (or add_url_rule method) instead of the constructor.

Copy link
Author

Choose a reason for hiding this comment

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

That could be a solution, yeah!

app.register_blueprint(doc_blueprint, spec=spec)

I'd still add the option of passing it through the __init__ as a kwarg and add an attach(spec) method, to add variety of uses.

What do you think @tinproject ?

@tinproject
Copy link

@JavierLuna The problem with the current implementation of FlaskPlugin is that it assume that one view_function match one to one to a path and that is not real.

The reality is that a view_function can be matched by multiple endpoints (but an endpoint can only match one view function), and an endpoint can be matched by multiple Rule objects, that are defined by a path (and associated methods and other data). And all rules lives inside a Werkzeug Map object at Flask's app.url_map.

I believe we can get the full spec representation of a path to return at FlaskPlugin.path_helper method.

For it we have to iterate over Flask app routing rules at app.url_map.iter_rules(), for every Werkzeurg Rule Object we have to convert the Rule.rule string to an spec compliant path and compare to the one passed to FlaskPlugin.path_helper.
If they match we get the associated endpoint, and hence the view_function. With introspection get the spec and add it to an internal representation of the path spec for the associated methods, and then continue with the next rule. If the path/method is already on the internal spec we just ignore it.

This rule parse can also process and add path parameters to the spec, but this if other story.

I'm seeing that right now the FlaskPlugin.path_helper method implementation does not match what apispec core APISpec.path() is expecting so FlaskPlugin.path_helper would have to change.

@jshwelz
Copy link

jshwelz commented May 27, 2020

any update on this?

@caffeinatedMike
Copy link

@sloria @pcaro Can you re-review this PR? I'm in need of a solution that allows creating individual specs, one per blueprint, that are version-specific. This seems like the perfect solution.

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

Successfully merging this pull request may close these issues.

6 participants