rantav / flask-restful-swagger

A Swagger spec extractor for flask-restful
MIT License
664 stars 215 forks source link

Intuit request parameters from reqparse? #18

Open DeaconDesperado opened 10 years ago

DeaconDesperado commented 10 years ago

First off, great work! :smile:

An idea: Flask-restful has a module called reqparse that is intended to be used as the main way to consume request parameters from within an endpoint. I am wondering if it could be useful to introspect on an endpoint's reqparse.RequestParser instance to generate the params for the swagger docs? I think it could be really cool to have one interface for generating this as a high-level option, where the @swagger.operation decorator could still be very useful for more granular specs.

What do you think? I could see some challenges implementing this (mainly where to scope the RequestParser instance). I'd be willing to have a crack at it if you think it's a compelling case.

rantav commented 10 years ago

Hi @DeaconDesperado thanks for the suggestion! reqparse does look very intuitive and useful. I never used it before... Anyway, it sounds like a good idea but I wonder how would you model that? Could you demonstrate how the decorators would look like (an example)? cheers.

DeaconDesperado commented 10 years ago

I'll brew together an example and post here a little later :smile:

chrisfranklin commented 10 years ago

+1 this seems like the way to go, did you get anywhere with it Deacon? If not do you have any pointers for things to ensure / avoid? Any help is appreciated. Cheers =)

DeaconDesperado commented 10 years ago

@chrisfranklin Sorry for the late reply on this.

I did have a crack at it, but nothing too substantial came of it because halfway through implementation I had to seriously rethink my intended design. From what I can tell, Swagger relies pretty heavily on the notion of a "response class", which is basically a complete data object. The original angle I was taking this from was just to handle the request parsing part, but generating to this class footprint is a little more complicated.

I'll have a look back at it and reply again here. If I've perhaps misunderstood the internals of swagger, I encourage others to chime in. I think it would be really cool to have the option to sit the doc generation atop a convention already existing in flask-restful. :smile:

chrisfranklin commented 10 years ago

Thanks for the response, all useful information, work aren't going to sign off on this until I'm sure it won't just be a time drain but I'll have a look at it over the weekend see if I can chime in at all.

You are absolutely right though it would be really cool to have it use reqparse. I've used the Django Restframework Swagger integration before and it worked extremely well. It might be worth a look for seeing how they got round the issue? https://github.com/marcgibbons/django-rest-swagger

simstre commented 10 years ago

+1 for reqparse

seriousben commented 10 years ago

@DeaconDesperado any updates on this?

hamx0r commented 10 years ago

+1 on this too. The way I seeing it being used intropectively is to use the @swagger.operation decorator, but omit the parameters arg. The decorator would then look at the method being decorated for any RequestParser() objects. If it finds one, it them looks at all the arguments (created usingRequestParser.add_argument()). From this, it will know whether a parameter is required along with its name, type and description.

What would be missing is the paramType and allowMultiple dictionary entries. The decorator could take either defaults (ie paramType to be used on all parameters) or else take a parameters list with partial dictionaries which will map (as applicable) to the discovered reqparse entries (ie parameters = [{'name': 'someId', 'allowMultiple': True, 'paramType': 'query'}] , and then later in decorated class, parser.add_argument('someId', required=True, type=int, help='This string goes into the description field of the parameter')

Even though it splits up the parameters fields into 2 sections of code (explicit and reqparse fields), it prevents duplicated data which may diverge.

hamx0r commented 10 years ago

I went ahead and implemented the above by adding these lines to swagger.py beginning at line 181 (see pull request https://github.com/rantav/flask-restful-swagger/pull/39):

        if 'parser' in method_impl.__dict__:
          arglist = method_impl.__dict__['parser'].__dict__['args']
          temp_op = []
          for argobj in arglist:
            arg = argobj.__dict__
            #push parameters from reqparse onto our swagger parameters list
            new_param = {'name': arg['name'], 'required': arg['required'], 'description': arg['help'], 'dataType': arg['type'].__name__}
            temp_op.append(new_param)
          op['parameters'] = merge_parameter_list(op['parameters'], temp_op)

I'll do a pull request or whatever (I dont use git much) so it can be rolled into the next rev. To be sure context is clear, here's what I added again but with a few lines before and after:

      if '__swagger_attr' in method_impl.__dict__:
        # This method was annotated with @swagger.operation
        decorators = method_impl.__dict__['__swagger_attr']
        for att_name, att_value in list(decorators.items()):
          if isinstance(att_value, (str, int, list)):
            if att_name == 'parameters':
              op['parameters'] = merge_parameter_list(op['parameters'], att_value)
            else:
              op[att_name] = att_value
          elif isinstance(att_value, object):
            op[att_name] = att_value.__name__  ### HERE IS WHERE NEW CODE STARTS
        if 'parser' in method_impl.__dict__:
          arglist = method_impl.__dict__['parser'].__dict__['args']
          temp_op = []
          for argobj in arglist:
            arg = argobj.__dict__
            #push parameters from reqparse onto our swagger parameters list
            new_param = {'name': arg['name'], 'required': arg['required'], 'description': arg['help'], 'dataType': arg['type'].__name__}
            temp_op.append(new_param)
          op['parameters'] = merge_parameter_list(op['parameters'], temp_op)
      operations.append(op) ### HERE IS WHERE ORIGINAL CODE RESUMES
    return operations

There's a catch though: one must be particular in where they define their ReqParse objects in their class: For this patch to be useful, define your "parser" (same object names/types as flask-restful docs) within your Resource-based class, but outside your methods. Lastly, you must then add a line after your method to pin the parser you defined to the method itself. This is because while functions can have attributes, methods cannot, unless the attribute is added to the method from within its class.

For example:

class TODOober(restful.Resource):
    """Our uber To-Do resource"""

    # This example of decorating get() doesn't use the code enhancement.  But if it did, you would want to define, say, 
    #get_parser = reqparse.RequestParser() here and add some arguments.  See post() example below.
    @swagger.operation(
        nickname='uberGet',
        parameters=[],
        responseClass=ToDoober.__name__ #this is some class defined elsewhere
    )
    def get(self):
        """This returns a list of all our To-Do items, so we didn't take an input parameters"""
        todos = {} # add your code to fill out todos
        return todos

    # Here we define our RequestParser within our TODOober class, but outside our post() method
    post_parser = reqparse.RequestParser()
    post_parser.add_argument('todo_id', required=True, type=int, help='ID of your favorite To-Do item')
    post_parser.add_argument('task', required=True, type=string, help='Task verbiage.')
    #to make this new to do list really awesome, let's add a priority field: with "choices"!
    post_parser.add_argument('priority', required=True, type=str, choices=('high', 'med', 'low'), help="What's it matter?")

    # Now decorate.  Note our parameters below can be sparser than usual - they just need to have the SAME name as used in our RequestParser
    # and contain the missing values required by swagger (paramType and allowMultiple)
    @swagger.operation(
        nickname='updateToDoober',
        parameters=[
            {'name': 'todo_id', 'paramType': 'query', 'allowMultiple': False},
            {'name': 'task', 'paramType': 'query', 'allowMultiple': False},
            {'name': 'priority', 'paramType': 'query', 'allowMultiple': False},
        ],
        responseClass=IfParams.__name__
    )
    def post(self):
        #bring in our parser defined above within the class.  You may be able to recycle your parser if, say, your get method also uses
        #the same parse args
        parser = self.post_parser

        args = parser.parse_args()
        resp = {}
        # put interesting code here to flesh out resp 
        return resp

    #Now pin our post_parser to our post method as an attribute for swagger to find
    post.parser = post_parser

What you should experience is that swagger adds data from your ReqParser into its parameters, thereby letting you have just one "master" in where your parameter definitions live. Otherwise, you'd have to update both your ReqParser and swagger.operation decorator whenever you made a change. Hope this helps!

dases commented 10 years ago

I would love to see this too, makes perfect sense. I think using the existing add_model might work well for this, e.g.

@swagger.model
class MyPostInput:
    parser = reqparse.RequestParser()
    # parser.add_argument(...)
    # ...

add another elif onto swagger.add_model

elif 'parser' in dir(model_class):
    for arg in model_class.parser.args:
        # do swaggery things. 
        # arg.name, arg.required, etc, etc
        # if arg.action == 'append':
            # allowMultiple is true
        # arg.location can be translated to paramType (with the exception of "cookies", which has no swagger analogue)

use it:

@swagger.operation(
    parameters=[{"name": "body", "paramType": "body", "dataType": MyPostInput.__name__}]
)
def post(self):
    # args = MyPostInput.parser.parse_args()
    # ...

Edit: no need to declare the parser outside of MyPostInput

dases commented 10 years ago

I ended up doing something different to the above that gives more flexibility with inputs of different paramType/location. I just added a "reqparser" arg on swagger.operation(). Scoping is a little smelly, but not too bad, imo. I'll not do a pull request as I see another way of doing this is being considered at #39. Nevertheless, I'll put a link here for your consideration as it's working well for me. https://github.com/dases/flask-restful-swagger/commit/2ba24a0d12a61a2e93e86492d278f45705f333bf

usage:

class UserResource(Resource):
    getUserParser = reqparse.RequestParser()
    getUserParser.add_argument('flags', type=bool, location='args')
    getUserParser.add_argument('contact', type=bool, location='args')
    @swagger.operation(
        notes='Get user details',
        nickname='getUser',
        reqparser=getUserParser
    )
    def get(self, username):
        args = self.getUserParser.parse_args()
        return get_user(username, **args), 200

    # if parser.arg.location has "json" (default), then a model is
    # created and used on the swagger output
    updateUserParser = reqparse.RequestParser()
    updateUserParser.add_argument('address', type=str, required=True,
                                  help="You must supply an address")
    @swagger.operation(
        notes='Update user details',
        nickname='updateUser',
        reqparser=updateUserParser,
        # reqparser args and parameters get merged
        parameters=[
            {"name": "some_other_param",
             "paramType": "query",
             "dataType": "string"
             }
        ]
    )
    def put(self, username):
        args = self.updateUserParser.parse_args()
        if args.some_other_param:
            do_something()
        return update_user(username, args.address), 204
hamx0r commented 10 years ago

Hi djm

I think the issue with the implementation below is that swagger.operations.parameters has more keys than a reqparse object. Even if we define a reqparse object and pass it to swagger.operations for it to build the parameters list, the list dictionaries will be missing: paramType and allowMultiples are not in reqparse objects (perhaps allowMultiple can be set based on if action=='append' on the reqparse object). Also, i haven't looked into how to access the arguments added to a reqparse object, so I'm not sure how to extract them to build the swagger parameters.

For this reason, i chose to have a dictionary which would drive creating a reqparse object rather than a reqparse object driving the creation of a swagger.operations.paramters list.

I'll aim to get you some code soon!

Jason

On Tue, Aug 26, 2014 at 3:18 PM, djm notifications@github.com wrote:

I ended up doing something different to the above that gives more flexibility with inputs of different paramType/location. I just added a "reqparser" arg on swagger.operation(). Scoping is a little smelly, but not too bad, imo. I'll not do a pull request as I see another way of doing this is being considered at #39 https://github.com/rantav/flask-restful-swagger/pull/39. Nevertheless, I'll put a link here for your consideration as it's working well for me. dases@2ba24a0 https://github.com/dases/flask-restful-swagger/commit/2ba24a0d12a61a2e93e86492d278f45705f333bf

usage:

class UserResource(Resource): getUserParser = reqparse.RequestParser() getUserParser.add_argument('flags', type=bool, location='args') getUserParser.add_argument('contact', type=bool, location='args') @swagger.operation( notes='Get user details', nickname='getUser', reqparser=getUserParser ) def get(self, username): args = self.getUserParser.parse_args() return get_user(username, **args), 200

# if parser.arg.location has "json" (default), then a model is
# created and used on the swagger output
updateUserParser = reqparse.RequestParser()
updateUserParser.add_argument('address', type=str, required=True,
                              help="You must supply an address")
@swagger.operation(
    notes='Update user details',
    nickname='updateUser',
    reqparser=updateUserParser,
    # reqparser args and parameters get merged
    parameters=[
        {"name": "some_other_param",
         "paramType": "query",
         "dataType": "string"
         }
    ]
)
def put(self, username):
    args = self.updateUserParser.parse_args()
    if args.some_other_param:
        do_something()
    return update_user(username, args.address), 204

Reply to this email directly or view it on GitHub https://github.com/rantav/flask-restful-swagger/issues/18#issuecomment-53501646 .

dases commented 10 years ago

Hi Jason,

See the commit I linked to above for how to iterate through reqparse args (line 326). Also, yes, some translation (quite a bit actually) of reqparse args is needed. e.g. action=='append' maps to allowMultiple (line 374).

A little bit off-topic, but: an annoyance that needs addressing: reqparse arg type="bool" is useless as a swagger "boolean" without some hackery. A query string sent by swagger-ui as a "boolean" such as: flags=false or flags=true are both True for python :(

cheers

hamx0r commented 10 years ago

Great, I will take a look at the commit!

How important is it to you that reqparse and swagger are tethered by default, but could be configured differently if the user wanted? For example, the help text a reqparse is often going to be the same as the description in swagger, but the user may want them to be different for whatever reason. Perhaps a user could still pass a parameters value in the swagger operations decorator to allow overwriting?

I think the reason the bools always shows true for python is because it gets converted to a string and then checked if it's true. Anything that is not 0 will show up as true for python. Maybe if it sent as a boolish string, it could be processed by forcing it to lowercase and comparing with the word "true".

Lastly, on the autogenerated API web page, i find that the GET requests work great with the "try it out" button, but not the POST requests. I don't even see the request make it to the server. I haven't looked into the code yet, but do POST requests from the Try It Out button work for you?

Jason

On Wednesday, August 27, 2014, djm notifications@github.com wrote:

Hi Jason,

See the commit I linked to above for how to iterate through reqparse args (line 326). Also, yes, some translation (quite a bit actually) of reqparse args is needed. e.g. action=='append' maps to allowMultiple (line 374).

A little bit off-topic, but: an annoyance that needs addressing: reqparse arg type="bool" is useless as a swagger "boolean" without some hackery. A query string sent by swagger-ui as a "boolean" such as: flags=false or flags=true are both True for python :(

cheers

Reply to this email directly or view it on GitHub https://github.com/rantav/flask-restful-swagger/issues/18#issuecomment-53633046 .

hamx0r commented 10 years ago

Hi There,

I looked at your code you linked to me https://github.com/dases/flask-restful-swagger/commit/2ba24a0d12a61a2e93e86492d278f45705f333bf- my apologies for the delay! It looks like a reqparse object does contain an exhaustive set of data to complete a swagger.operations.parameters list! I failed to see the connection between paramType (swagger) and location (reqparse). Looks great! The single reason I was lobbying for using a dictionary as the "master" dataset is because I though the set of data needed by parameters and reqparse were not completely overlapped. My aim was to make a superset dictionary containing all mutual values (help=description, required, etc) plus the values unique to either parameters or reqparse. As it turns out, you've shown me that there's a direct mapping of reqparse args to parameter dicts. I'd love to see your improvement in the next release!

Regarding your bool problem, i did some digging and i think row 369 needs to be changed to this: param = deduce_swagger_type(arg.type())

Unlike parsing resource_fields, it seems that reqparse doesn't really store the type in usual way. Check this out:

from flask.ext.restful import reqparse r = reqparse.RequestParser() r.add_argument('foo', type=bool) <flask_restful.reqparse.RequestParser object at 0xf03710> a = r.args[0] # this is our argumenet we just added, of type bool a.type <type 'bool'> isinstance(a.type, bool) False a.type() False isinstance(a.type(), bool) True

This may fix your bool problem, and likely other problems you might run into when detecting reqparse arg types.

Jason

On Wed, Aug 27, 2014 at 3:38 PM, Jason Haury jason.haury@gmail.com wrote:

Great, I will take a look at the commit!

How important is it to you that reqparse and swagger are tethered by default, but could be configured differently if the user wanted? For example, the help text a reqparse is often going to be the same as the description in swagger, but the user may want them to be different for whatever reason. Perhaps a user could still pass a parameters value in the swagger operations decorator to allow overwriting?

I think the reason the bools always shows true for python is because it gets converted to a string and then checked if it's true. Anything that is not 0 will show up as true for python. Maybe if it sent as a boolish string, it could be processed by forcing it to lowercase and comparing with the word "true".

Lastly, on the autogenerated API web page, i find that the GET requests work great with the "try it out" button, but not the POST requests. I don't even see the request make it to the server. I haven't looked into the code yet, but do POST requests from the Try It Out button work for you?

Jason

On Wednesday, August 27, 2014, djm notifications@github.com wrote:

Hi Jason,

See the commit I linked to above for how to iterate through reqparse args (line 326). Also, yes, some translation (quite a bit actually) of reqparse args is needed. e.g. action=='append' maps to allowMultiple (line 374).

A little bit off-topic, but: an annoyance that needs addressing: reqparse arg type="bool" is useless as a swagger "boolean" without some hackery. A query string sent by swagger-ui as a "boolean" such as: flags=false or flags=true are both True for python :(

cheers

Reply to this email directly or view it on GitHub https://github.com/rantav/flask-restful-swagger/issues/18#issuecomment-53633046 .

dases commented 10 years ago

Nah, the type is being recognised as a bool - that's not the problem. FYI, issubclass() is being called, not isinstance() - which is why I moved the test for bool up above the test for int (bool is a subclass of int, so 'integer' was being returned, short-circuiting the test for bool). The problem is not with flask-restful-swagger, but with the swagger-ui's coarse notion of boolean: the string 'false' is not a false value. Yeah I could test the string, but ugh - like I said, "hackery". :) I'd probably prefer using "string" and enum [0, 1] - values that I'd expect from the production frontend.

BTW, yes, post requests work fine for me (as do delete and put).

hamx0r commented 10 years ago

I've now begun using bool types and see more of what you mean. Is it related to the fact that when one uses an HTML form with a checkbox, the checkbox doesn't return True or False, but rather something or nothing, and any something will be seen as a string which evaluates to True (Python bool here), whereas nothing evaluates to False?

The reason POSTs weren't working was because I hadn't set the swagger.operataion 'paramType' correctly. It seems that graceful degradation is at play with the Swagger UI: if something is coded incorrectly, it doesn't result in an error to be tracked down, but rather doesn't operate as expected. is there a "debug mode" that other swagger implementations have? Maybe I could start writing one for flask-restful-swagger

BTW, I noticed the flask-restplus https://pypi.python.org/pypi/flask-restplusmodule out. At first I though, "maybe the developers could join forces!", but then I saw flask-restplus requires Python 2.7+. Just wanted you to know that flask-restful-swagger has extra value by working on Python 2.6 (the version shipping with the latest CentOS). While I prefer more up-to-date-OSs, the preference for stable/vintage OSs is strong, which keeps CentOS widly used (at my company) which also keeps your backwards compatibility very valuable! So, thanks!

Jason

On Thu, Aug 28, 2014 at 5:17 PM, djm notifications@github.com wrote:

Nah, the type is being recognised as a bool - that's not the problem. FYI, issubclass() is being called, not isinstance() - which is why I moved the test for bool up above the test for int (bool is a subclass of int, so 'integer' was being returned, short-circuiting the test for bool). The problem is not with flask-restful-swagger, but with the swagger-ui's coarse notion of boolean: the string 'false' is not a false value. Yeah I could test the string, but ugh - like I said, "hackery". :) I'd probably prefer using "string" and enum [0, 1] - values that I'd expect from the production frontend.

BTW, yes, post requests work fine for me (as do delete and put).

Reply to this email directly or view it on GitHub https://github.com/rantav/flask-restful-swagger/issues/18#issuecomment-53800588 .

rantav commented 10 years ago

I didn't know about flask-restplus, looks kind of new. I wonder if that dude know about flask-restful-swagger. I'd be happy to cooperate, does someone know the guys (his email is hidden on GH so I though I might take a more personal route before opening an issue to get his attention ;))

hamx0r commented 10 years ago

Hah, i don't know who he is. I do know that his package requires python 2.7+, and that's a deal breaker. If you two decide to merge, it would be great to be at least python 2.6 compatible (only for CentOS...which I dislike but have to use at work).

Is there anything I can help with for this reqparse? I see that the pull request i made (and then wrote code for) was based on 0.13. The newer code in 0.14 is much more verbose, so i'm currently still using my hacked 0.13 module for a work project. I'd love to see integration with reqparse and include your new fixes/features in 0.14. How can I best help?

Jason

On Thu, Sep 11, 2014 at 1:08 AM, Ran Tavory notifications@github.com wrote:

I didn't know about flask-restplus, looks kind of new. I wonder if that dude know about flask-restful-swagger. I'd be happy to cooperate, does someone know the guys (his email is hidden on GH so I though I might take a more personal route before opening an issue to get his attention ;))

Reply to this email directly or view it on GitHub https://github.com/rantav/flask-restful-swagger/issues/18#issuecomment-55220041 .

hwdavidward commented 9 years ago

What's the status on this? Is this going to pulled or not? I am very interested in this.