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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Intuit request parameters from reqparse? #18

Open
DeaconDesperado opened this issue Apr 2, 2014 · 20 comments
Open

Intuit request parameters from reqparse? #18

DeaconDesperado opened this issue Apr 2, 2014 · 20 comments

Comments

@DeaconDesperado
Copy link

First off, great work! 馃槃

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
Copy link
Owner

rantav commented Apr 2, 2014

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
Copy link
Author

I'll brew together an example and post here a little later 馃槃

@chrisfranklin
Copy link

+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
Copy link
Author

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

@chrisfranklin
Copy link

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
Copy link
Contributor

simstre commented Jun 19, 2014

+1 for reqparse

@seriousben
Copy link
Contributor

@DeaconDesperado any updates on this?

@hamx0r
Copy link

hamx0r commented Aug 13, 2014

+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
Copy link

hamx0r commented Aug 13, 2014

I went ahead and implemented the above by adding these lines to swagger.py beginning at line 181 (see pull request #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!

hamx0r added a commit to hamx0r/flask-restful-swagger that referenced this issue Aug 13, 2014
Per issue rantav#18, added code to look for a "parser" attribute on a method decorated by @swagger.operation.  If parser is found, then it assumes it to be a ReqParser object and extracts overlapping data between ReqParser and Swagger: name, required, description/help and dataType/type.

This requires ReqParser to be defined and added to method as an attribute per this issue comment:
rantav#18 (comment)
@dases
Copy link

dases commented Aug 18, 2014

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
Copy link

dases commented Aug 26, 2014

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. dases@2ba24a0

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
Copy link

hamx0r commented Aug 26, 2014

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
#39. Nevertheless,
I'll put a link here for your consideration as it's working well for me.
dases/flask-restful-swagger@2ba24a0
dases@2ba24a0

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
#18 (comment)
.

@dases
Copy link

dases commented Aug 27, 2014

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
Copy link

hamx0r commented Aug 27, 2014

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
#18 (comment)
.

@hamx0r
Copy link

hamx0r commented Aug 28, 2014

Hi There,

I looked at your code you linked to me
dases@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
#18 (comment)
.

@dases
Copy link

dases commented Aug 28, 2014

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
Copy link

hamx0r commented Sep 9, 2014

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
#18 (comment)
.

@rantav
Copy link
Owner

rantav commented Sep 11, 2014

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
Copy link

hamx0r commented Sep 16, 2014

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
#18 (comment)
.

@hwdavidward
Copy link

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

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

No branches or pull requests

8 participants