noirbizarre / flask-restplus

Fully featured framework for fast, easy and documented API development with Flask
http://flask-restplus.readthedocs.org
Other
2.73k stars 505 forks source link

Migration to webargs #9

Open vovanbo opened 9 years ago

vovanbo commented 9 years ago

What you think about migration from reqparser to webargs? Flask-RESTful will be ready soon to rip out their reqparser and marshalling (see https://github.com/flask-restful/flask-restful/issues/335).

Does it make sense?

noirbizarre commented 9 years ago

Yes it makes sense.

I'll be waiting for upstream work to advance and be merged.

Maybe I should open a webargs branch to prepare the migration and stay close to the upstream work in progress.

noirbizarre commented 9 years ago

I will probably add support for Webargs soon !

croshchupkin commented 9 years ago

The referenced flask-restful ticket also proposes to use marshmallow, does it not? As far as I've read the docs, marshmallow has many more features than webargs (besides retrieving data from several locations on the request) and looks more promising. Or did I accidentally miss something?

noirbizarre commented 9 years ago

Yes, in fact I/we can do both.

I'm thinking about dropping flask-restful. It was a very good base to start, but now, it may be easier for me to only copy the code I'm depending on. The hook and monkey-patch approach has some limits and introduce some complexity.

Dropping flask-restful doesn't mean API breakage, because I will still have to support their request parser, but it means I can let people choose their request parser (Webargs, Marshmallow...)

I'm very open to discussion, feedback or any advice on this topic.

sloria commented 9 years ago

To clarify: webargs is used for declaratively defining request arguments (e.g. query params, JSON body) and marshmallow is used for deserialization and serialization. webargs knows how to pull values from a framework's request objects; marshmallow does not. While there is some overlap in their functionality (both can do validation), they are meant to be used together.

+1 to the direction @noirbizarre is proposing. I've done very early work on a pluggable API doc generator (https://github.com/marshmallow-code/smore/issues/1). Would be open to collaborating.

frol commented 8 years ago

I would like to add here that webargs moved on using marshmallow fields under the hood (https://github.com/sloria/webargs/issues/61).

I have also successfuly patched Flask-RESTplus to handle marshmallow schemas and webargs parameters (using flask-marshmallow, webargs, and apispec modules together). I may try to make a PR, but I'm not sure whether it should be done before Flask-RESTplus drops Flask-RESTful dependency.

Currently, my API looks like this:


@namespace.route('/')
@errors.DefaultHTTPError(code=422, description="Arguments are invalid")
@errors.AuthHTTPError()
class Users(Resource):
    """
    Manipulations with users.
    """

    @auth.login_required(scopes=['users:read'])
    @permissions.AdminRolePermission()
    @parameters.PaginationParameters()
    @schemas.BaseUserSchema(many=True)
    def get(self, args):
        """
        List of users.

        Returns a list of users starting from ``offset`` limited by ``limit``
        parameter.
        """
        return User.query.all()[args.offset: args.offset + args.limit]

Instead of

@namespace.route('/')
@api.doc(
    responses={
        401: ("Auth error", AUTH_HTTP_ERROR_MODEL),
        400: ("Arguments are invalid", INVALID_ARGS_HTTP_ERROR_MODEL),
    }
)
class Users(Resource):
    """
    Manipulations with users.
    """

    @auth.login_required(scopes=['users:read'])
    @permissions.AdminRolePermission()
    @api.doc(parser=pagination_arguments_parser)
    @api.marshal_with(BASE_USER_DEFINITION)
    def get(self):
        """
        List of users.

        Returns a list of users starting from ``offset`` limited by ``limit``
        parameter.
        """
        args = pagination_arguments_parser.parse()
        return User.query.all()[args.offset: args.offset + args.limit]

For me, the code became cleaner and also things like AUTH_HTTP_ERROR_MODEL and pagination_arguments_parser became classes, so it will be easier to maintain them.

noirbizarre commented 8 years ago

Hum, I'm really interested in. I suggest you wait for flask-restful removal. There might be an adaptter pattern to keep the legacy flask-restful request parser in the hood but allowing to provide different adapter for different frameworks, Marshmallow being one of them.

Until then don't hesitate to publish the code somewhere, it can be a good source of inspiration.

frol commented 8 years ago

@noirbizarre Here is my RESTful API example server using Flask-RESTplus with marshmallow, webargs, and JSON Editor and Password Flow patches to Swagger-UI: https://github.com/frol/flask-restplus-server-example

Notice that I have patched Flask-RESTplus right there (find flask_restplus_patched folder).

It still requires some polishing, so I will notify you once I am completely satisfied with its design. Actually, there is only one thing that is bothering me: I don't like that Schema (in flask_restplus_patched/model.py), Parameters (in flask_restplus_patched/parameters.py), and Permission (in app/users/permissions.py) handle api instance inside, it has to be done either by api instance call (like it is for parser - @api.doc(parser=parser_instance)), or something else (because I don't see a way api instance can handle Permission instance if it is not supported directly).

noirbizarre commented 8 years ago

Thansk @frol for the sample project.

I will take a closer look for the 0.9 release.

frol commented 8 years ago

@noirbizarre My example matured and it is close to the final state. Flask-RESTplus patches (in flask_restplus_patched folder) are not going to change, though. I just want to tune permission for the example, and I also want to add tests (I couldn't really write them before I finally decide on the interfaces, but tests must be in the example to make it complete).

frol commented 8 years ago

@noirbizarre I have upgraded my Flask-RESTplus-Server-Example to align it with Flask-RESTplus 0.9 release (though, #139 merge is required). Please, take a look into flask_restplus_patched folder, and particularly, into namespace.py and swagger.py. Overall, my patchset is quite small. Besides webargs (parameters.py) it adds support for marshmallow models (models.py), and defines DefaultHTTPErrorSchema which is automatically applied to api.response() decorated methods by default.

thclark commented 8 years ago

Watching this thread with great interest, @frol and @noirbizarre . Thanks so much for both of your efforts, and a definite +1 for a pull of the flask_restplus_patched patch set into flask_restplus.

frol commented 8 years ago

@thclark Thanks for your interest. Since the most refactoring is done (flask-restless got partially integrated inside flask-restplus), I will try to prepare several pull requests merging my "fork" into flask-restplus. I expect the following features will be added separately:

I have cut several corners while writing my patchset since for the sake of simplicity, I made an assumption that only Marshmallow schemas and Webargs will be used. Thus, it will require some amount of work to integrate my patches back to general implementation. You are welcome to make those pull-requests to Flask-RESTplus project porting them from my patchset or writing your own since I don't know when I will have enough time to work on this.

By the way, I am not sure where the line should be drawn for Flask-RESTplus, so in my example, I have implemented users, authentication, and authorization on an application level.

thclark commented 8 years ago

@frol Thank you. I'm just now learning python / flask after many years with MATLAB, FORTRAN and C so some of these issues are a bit outside my comfort zone at the moment. Nevertheless, I'll clone flask-restplus and see if I can implement any of those features reasonably elegantly and without breaking things. If I get comfortable with it I'll make a PR and make a note of it here.

I think you drew the line correctly re. users, authentication and authorisation - it doesn't really seem like the central goal of Flask-Restplus. Additionally (admittedly unusually) I'm implementing a customised unix-style permissioning system which could turn into a headache if I tried to shoehorn it into a pre-set framework. I think your example is a good enough guideline for how to do it quite compactly.

frol commented 8 years ago

@thclark I'm not sure what problem you want to solve with "unix-style permissioning system", but I encourage you to look at this great project: https://github.com/hustlzp/permission (while the documentation has enough examples, you can also take a look at my example server, where I implement roles and "mixin" permissions).

thclark commented 8 years ago

Thanks @frol, very helpful. I'll take a look.

autoferrit commented 7 years ago

Has the integration with webargs moved forward at all recently? I saw the demo from @frol that seems to work. I'm not sure if that base could be used as a PR or not. Thanks.

sohaibfarooqi commented 7 years ago

Hi! I would be interested to know if the above example by @frol will work will latest release of flask-restplus? Thanks!

frol commented 7 years ago

@sohaibfarooqi My example server works with the latest Flask-RESTplus just fine. I have been using it as a bootstrap for my production projects for almost a year now.

sohaibfarooqi commented 7 years ago

@frol flask_restplus_patched version? One with marshmallow

frol commented 7 years ago

@sohaibfarooqi I didn't understand your question. Rephrase it, please. The flask_restplus_patched doesn't have any versioning, but it works just fine with the latest and recent Flask-RESTplus releases.

sohaibfarooqi commented 7 years ago

This repo have different code in app/ and flask_restplus_patched/. So args parsing with webargs/marshmallow is done in app/?

frol commented 7 years ago

flask_restplus_patched/ contains a collection of wrappers for marshmallow, webargs, and apispec extending Flask-RESTplus helpers. app/ implements the demo server defining schemas, parameters, and resources.

sohaibfarooqi commented 7 years ago

Ohhh right.! Thanks @frol for this great sample. :)

sonovice commented 7 years ago

Any progress on this? Would still love to see the patches integrated into restplus. :)

atulatri commented 7 years ago

+1

frol commented 7 years ago

No progress from me, sorry. I am still extending my example server (rarely touching the Flask-RESTplus wrapper helpers, though), and I use it for a bigger project of my own, where it works well, so since everything is fine for me, I cannot allocate any more time to move my helpers to the upstream (as I have mentioned, it will require quite a bit of work to ensure that the existing schemas don't break).

I have several options for those who are really interested in this topic:

lafrech commented 7 years ago

Our project uses Marshmallow to define the models, so this integration is a key feature for us.

I checked flask-apispec a while ago. It didn't have all the features we required, and it didn't seem to show much [re|]activity. I wasn't comfortable with it because I found it hard understanding the code, so I couldn't modify it to fit my needs (doesn't mean it is that twisted, could be me being noobish on that one).

We ended up crafting an equivalent of our own: flask-rest-api. It is inspired by flask-restplus and flask-apispec and relies on apispec and webargs. It also deals with etag and pagination. There is no docs yet and the direct test coverage is poor, although it is tested through our project. Needless to say it is by no means stable, as we introduce breaking changes as we go. There are also a few issues in the generated docs.

AFAIK, flask-restplus is the most stable and featured alternative. I'd probably be using it if our project was not so Marshmallow-based.

Apparently, there's been a little activity on flask-apispec, lately...

atulatri commented 7 years ago

Same issue here. flask-apispec is not active. And notes explicitly states that project is not stable. @lafrech is there any documentation for flask-rest-api? is it stable and are you planning to support it for long time?

lafrech commented 7 years ago

@atulatri:

notes explicitly states that project is not stable.

I think what it means is that new releases might introduce breaking changes. It is probably reliable in its current state if it does what you need.

@atulatri:

is there any documentation for flask-rest-api? is it stable

@lafrech:

There is no docs yet and the direct test coverage is poor, although it is tested through our project. Needless to say it is by no means stable, as we introduce breaking changes as we go.

flask-rest-api is less mature. Don't use it if you want something that works out of the box. For now we don't "support" it, we're building it as we build our application, and we expect to be using it on several long term projects. Communicating about it might be a little early. My point was to provide feedback about the solutions I tested and how I ended up starting a new project. Publishing it makes it easier to discuss specific parts or backup discussions about the libs we use. And we appreciate feedback. Plus we get CI for free.

cizixs commented 7 years ago

Is there a detailed plan and due date for this feature?

ramarivera commented 7 years ago

@frol could you give us some "tips" on where would we have to start in order to integrate your helpers to flask restplus?

@noirbizarre are you still open to PRs? is the project still "live" ?

Im asking because im trying to choose flask extensions for an upcming project for uni, and so far restplus is winning. But i would also liketo be able to "give something baack", so this is ideal.

Thanks in advanced!!

frol commented 7 years ago

@ramarivera I still use my flask-restplus-server-example project as a bootstrap for new API servers I implement, and it works great for me... I wish I have time to port all the patches back to flask-restplus upstream.

ramarivera commented 7 years ago

@frol i'm sorry, when i said to integrate into flask restplus i meant into the upstream (my bad u.u)

Thanks

frol commented 7 years ago

@ramarivera Well, you may simply go and replace the methods in the upstream with the methods from my patchset (just keep in mind the super usage, which will essentially mean to merge both implementations in a single method). Such simple approach will result in that you will be able to use webargs and marshmallow right away, but the problem, which stops me from working on the PRs, is that other ways of defining parameters and models supported by flask-restplus will be broken. That is OK for my use since I don't want those alternative ways, but that will break backward compatibility of the project, which is bad.

jbrownsc commented 7 years ago

@frol Have you reached out to @noirbizarre -- Perhaps consider becoming an owner on the project?

frol commented 7 years ago

@jbrownsc I haven't tried to reach out @noirbizarre because I don't see any point in that. He is aware of my example with patches, but I assume, he is quite busy just as myself to work on this. Thank you for your support, but I don't want to become an owner/maintainer as I have too many things to maintain already. I must admit that getting my patches around from project to project is tedious (I have already built 4 production-ready projects based on the example), so I consider to finally work on making PRs to flask-restplus.

Also, @khorolets and I learned that there is still quite a bit of code duplication and boilerplate for a new module. @khorolets has been experimenting with "generics" approach based on my example, so we keep improving the example and the patchset.

joeyorlando commented 6 years ago

@frol any word on getting your example changes merged into the project? this is exactly what my team and I are looking for šŸ˜ , prevents us from having to duplicate defining Schemas and api.model responses.

frol commented 6 years ago

@joeyorlando Sorry for this lack of progress, but I have been super-busy, and to be honest, the idea of ensuring that I don't break others code doesn't make me exciting. I have brought some patches to the upstream, but the main patchset still remains as a set of patches. I hope I will be able to sort this out by the mid of January, but I cannot promise.

Also, I want to highlight that there is an interesting project emerged, called apistar, which exploits Python 3 typing annotations as validation and serialization information. Though, beware that its support for spec generation is weak (they support OpenAPI / Swagger, RAML, and CoreAPI), and its development is on pause. I still find my own implementation to be the most stable and robust to the "real-world" tasks, but I keep exploring what it is available out there.

joeyorlando commented 6 years ago

thanks @frol for referencing apistar, looks neat will definitely have to check it out! I may just stick with your patched version for now, more suits my needs. On this note though, I'm having to implement the Namespace.expect method to allow passing in marshmallow.Schema objects (as opposed to Namespace.model objects, any idea how this might look?

frol commented 6 years ago

@joeyorlando I am not sure if I understood you correctly, but have you looked at Namespace.parameters implementation in my API server example? https://github.com/frol/flask-restplus-server-example/blob/9bc528f0e9a15e28b245284bf695f113d36987f4/app/modules/teams/resources.py#L36

joeyorlando commented 6 years ago

@frol I see what you're referring to now šŸ‘ just what I needed. Sorry to be a pain but last question I had was referring to changing the structure/default HTTP status code when there are validation errors. By default it seems that you have it set to 422 HTTP code and the following JSON object:

{
  "messages": { // stuff inside here is from my Schema validators
    "email": [
      "Not a valid email address."
    ],
    "user_role": [
      "User role specified by UUID string does not exist"
    ]
  }
}

Ideally I'd like to change it to a 400 HTTP code and a different object structure but can't find where in your patched library this is happening šŸ¤” , figured it would be where <my_schema_class>.load() is being called, but don't see any references to .load(). Is this something happening internally in the webargs.flaskparser library?

frol commented 6 years ago

@joeyorlando Let's stop hijacking this thread. Please, move your questions to the issue tracker on my example, and let's deal with them there.

zedrdave commented 6 years ago

I am about to tackle a SQLAlchemy + Marshmallow + flask-restplus automated REST schema generation thingā€¦ Before I dive in, I was wondering what might be the status of a possible PR for this feature?

frol commented 6 years ago

@zedrdave I have mostly switched from REST to Thrift and from Python to Rust, so I have lost much of my initial interest in this topic. Thus, I don't believe I will ever bring my patches to the upstream, yet feel free to use them if you find any value in them (they seem to be stable and minimal enough).

tiangolo commented 6 years ago

Disclaimer: this might seem open source "self-promotion", but I think it might be useful for some of you:

After not very successfully trying to use Flask-rest, Flask-restplus, and Django-rest for APIs, and after seeing several comments and activity in issues in several projects (especially from @frol , so thanks for all your activity, it helped a lot) I'm very happily using a combination of:

It all combined creates a Swagger endpoint, so now my APIs have automatic interactive documentation. It can parse and validate even subtrees of args, report errors automatically, etc.

If you want to try the recipe, I created a project generator here: https://github.com/tiangolo/full-stack

lafrech commented 6 years ago

+1 for the marshmallow stack (webargs, apispec) which provides a sane base to build upon.

Self-promotion disclaimer here as well: As I said in a comment above, we had difficulties with flask-apispec, and we ended up building our own wrapper around the marshmallow libs: flask-rest-api. A year has passed and it is now much more mature, with full test coverage, clearer code, PyPI releases. It still lacks docs but there are examples in the tests. However, it is far from being as stable and featured as flask-restplus.

If you search the Internet, you'll find loads of "flask + whatever" REST libs, each with its own features and shortcomings. A part of them are meant to interact only with SQLAlchemy.

zedrdave commented 6 years ago

@tiangolo @lafrech: thanks for the suggestions! No worries about the self-promotion (I wasn't aware of these two projects, and it's nice to know about these alternatives)ā€¦ However, our current project is well under way with flask-restplus, which means we are more interested in a solution based on that. Especially since the patched version made by @frol seems to work fine.

Out of curiosity, how would you summarise the differences in features and useability between your libs and @frol's patched version of Flask-restplus?

@frol: no worries, I understand. From looking at your patched files, this is indeed a fairly small amount of code, especially if we set aside the Python 3/HTTP code part for now. I could potentially try my hand at a straightforward PR that just replaces/merges existing functions with the ones you wroteā€¦ Would you foresee any issue with that?

@noirbizarre: would you have any interest in such a PR? Would you rather look at it yourself?

frol commented 6 years ago

especially if we set aside the Python 3/HTTP code

@zedrdave That part is already in the upstream (#303), yet there is no release on PyPi since a year ago, so I cannot drop that from my patchset just yet.

I could potentially try my hand at a straightforward PR that just replaces/merges existing functions with the ones you wroteā€¦ Would you foresee any issue with that?

The problems may show up if users of the library (i.e. developers, not the end-users) mix up Marshmallow schemas / parameters with Flask-RESTplus built-in schemas / parameters on a single end-point. This compatibility issue and proper testing were the show-stoppers for me to work on the PR, as I didn't use the built-in schemas / parameters. Other than that, everything should go smoothly.

zedrdave commented 6 years ago

The problems may show up if users of the library (i.e. developers, not the end-users) mix up Marshmallow schemas / parameters with Flask-RESTplus built-in schemas / parameters on a single end-point.

Gotcha. Maybe for now, a simple and safe solution, would be a config setting that only lets devs use built-in schemas or Marshmallow schemas (but not mix them) and set to built-in by defaultā€¦