marshmallow-code / apispec-webframeworks

Web framework plugins for apispec (formally in apispec.ext).
MIT License
32 stars 23 forks source link

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

Open JavierLuna opened 5 years ago

JavierLuna commented 5 years ago

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%.

sloria commented 5 years ago

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

tinproject commented 5 years ago

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 commented 5 years ago

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

JavierLuna commented 5 years ago

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

tinproject commented 5 years ago

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.

JavierLuna commented 5 years ago

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 :+1:

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?

tinproject commented 5 years ago

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

The reality is that a _viewfunction 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 _viewfunction. 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 commented 4 years ago

any update on this?

caffeinatedMike commented 2 years ago

@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.