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

Discussion: Rip out Request Parser and marshalling system and replace them with Marshmallow #335

Open
svenstaro opened this issue Nov 15, 2014 · 47 comments

Comments

@svenstaro
Copy link
Contributor

@svenstaro svenstaro commented Nov 15, 2014

Hi, since I discovered flask-restful I've been wary of its marshalling support and the under-featured Request Parser. In short, I want to replace those with Marshmallow which specializes on doing that exact thing and it does it very well. This might not be for 1.0.0, however. I don't want to break everyone's applications.

Why?

I want to do this for two reason mainly. Firstly, most issues in this project are either with the Request Parser (#275 #258 #279 #326 #322 #321 #317 #305 #301) or the marshalling support (#291 #309 #332 #324 #318 #300). Secondly, since we already have tools in the Python world to do all that for us, I'm not sure we should reinvent the wheel badly and put it into this library. It would serve everybody to use proper solutions for this and to outsource the effort required for maintaining those.

Now, obviously we might go one step further and just propose to have entirely generic solutions in place of this with documentation on how to integrate them. I'm skeptical of that as it might lead to overengineering. I think by using Marshmallow (which actually already handles most of the current issues that we have with marshalling and validation) we can get rid of a lot of code as well.

I know this is a radical change so I want to discuss it with you guys first.

@rabbbit
Copy link

@rabbbit rabbbit commented Nov 17, 2014

+1

We've just made a decision to drop flask-restful marshalling in favor of Marshmallow, so, yeah, I think it's a very good idea.

@skimbrel
Copy link
Contributor

@skimbrel skimbrel commented Nov 17, 2014

Marshmallow looks awesome. What does the proposed integration look like?

@svenstaro
Copy link
Contributor Author

@svenstaro svenstaro commented Nov 17, 2014

Frankly, I want to couple it pretty losely only with no real wrapper. In fact, I probably only put it as optional and treat it more like a documentation thing than an implementation thing. So more along the lines of "oh and if you want to get data from your data base serialized, use this and do it like this". Opinions?

@skimbrel
Copy link
Contributor

@skimbrel skimbrel commented Nov 17, 2014

Sounds good. Maybe mark marshal and reqparse as deprecated in the 1.0 release, update the docs to suggest Marshmallow instead, and then remove the old stuff in a future release?

@joshfriend
Copy link
Member

@joshfriend joshfriend commented Nov 17, 2014

I've looked at Marshmallow before and just spent some time playing with it since I planned on migrating to it myself. What I like so far is that it has a lot more serializer formats (especially datetimes). The error checking looks pretty cool too. I havent tried using it for deseriaization yet, but it looks fantastic because you can use the same schema definition to dump/load.

One thing I do commonly is embed pagination info like so:

user_fields = {
    'id': fields.Integer,
    'name': fields.String,
    'email': fields.String,
}

user_collection_fields = {
    'items': fields.List(fields.Nested(user_fields)),
    'total': fields.Integer,
    'page': fields.Integer,
}

How do I replicate something like this in Marshmallow? The examples show the list of objects being serialized separately, then the extra info is built around it.

I think if the fields module is going to be removed, at least release one more minor version so that all the recent fixes are available for people who would still be using the marshalling module (I think this is what you were suggesting anyways).

The feature I liked from the reqparse module was that it can aggregate input from lots of locations, though when I think about it, I've never really used that feature and can't think of a scenario where I would want to pull info from both the JSON and query string at the same time.

TL;DR I could totally get behind Marshmallow.

@joshfriend
Copy link
Member

@joshfriend joshfriend commented Nov 17, 2014

Kinda related: the paging module does not appear anywhere in the docs and I'm not sure what it does. If that also went away, tests wouldn't require pycrypto to be installed.

Also, deprecating the utils.cors module in favor of Flask-CORS would fix things like #229 and #276. The crossdomain decorator from utils.cors is basically just this anyways.

Including a page in the docs about suggested integrations with marshmallow and flask-cors would be a better idea IMO.

@nickretallack
Copy link

@nickretallack nickretallack commented Nov 18, 2014

I like reqparse. It's the only part of flask-restful that I am using. Sorry for artificially inflating the issue count for you guys :P.

If we switched to marshmallow, how would you handle the location parameter? Manually, I guess?

@joshfriend
Copy link
Member

@joshfriend joshfriend commented Nov 18, 2014

@nickretallack The location parameter of the Argument was supposed to let you extract info from multiple places in the request. Unfortunately, the feature is broken (see #261), so its probably safe to assume that it's not used.

Take a look at the Marshmallow deserialization example here. You would pass request.json, request.args, etc. to the load() method of the Schema.

@dtheodor
Copy link
Contributor

@dtheodor dtheodor commented Nov 18, 2014

If you want to replace reqparse, you should take an educated decision and do it right. There are a lot of libraries out there: Python Schematics (which from a very brief look seems identical to marshmellow but more mature?), there's Voluptuous for rich python dict validation, but the thing that I miss the most is integration of JSON-Schema validation so that my JSON validation can get out of the boundaries of Python and become inter-operable with other systems.

JSON-schema is the only technology (still draft) that can get adopted by things like MongoDB, Javascript (https://github.com/garycourt/JSV), Postgres JSON (https://json-schema-toolkit.readthedocs.org/en/latest/postgresql.html), as well as data model specifications in general (http://snowplowanalytics.com/blog/2014/05/15/introducing-self-describing-jsons/). Maintenance of data validation then becomes minimal if I can reuse a single JSON-schema file instead of having to manually write and maintain my constraints in marshmellow, in SQL and in whatever other system.

@svenstaro
Copy link
Contributor Author

@svenstaro svenstaro commented Nov 18, 2014

Exactly the point, @dtheodor. That's why I want to make it part of docs only without real integration. After all, the api doesn't care what exactly you are outputting/inputting as long as it looks like json right?

I want to make a series of docs for maybe 3 validation/schema libs. I know json-schema and you'd be welcome to write an example with that.

My reasoning is that there's absolutely no reason to take away the user's choice here. The current solution is the worst of all (half baked validation and marshalling). What do you think?

@nickretallack
Copy link

@nickretallack nickretallack commented Nov 18, 2014

Awesome, some discussion on good (de)serialization/validation libraries.

I would use jsonschema if it supported type coercion from text inputs like formdata. Unfortunately, it won't coerce the "type" parameter. It has a "format" parameter you can use to check a custom type coercion, but then it just drops that value on the floor instead of keeping it. Disappointing. It seems like it'd be pretty easy to fix up for our purposes, though the maintainers are not on board with this.

@svenstaro svenstaro mentioned this issue Nov 20, 2014
3 of 4 tasks complete
@Ch00k
Copy link
Contributor

@Ch00k Ch00k commented Nov 26, 2014

There is also a flask-marshmallow project: https://github.com/sloria/flask-marshmallow
Would it make sense to integrate with it instead of plain marshmallow?

@svenstaro
Copy link
Contributor Author

@svenstaro svenstaro commented Nov 26, 2014

Yeah that was the idea. We need flask-marshmallow for the url stuff.

@Ch00k
Copy link
Contributor

@Ch00k Ch00k commented Nov 27, 2014

I came across a webargs project (inspired by Flask-RESTful reqparse), which is specifically used to handle input data. It looks very easy to use. And they also recommend using marshmallow for output data formatting. Perhaps another library to document support for.

@ghost
Copy link

@ghost ghost commented Dec 3, 2014

Flask-Marshmallow does have the URL generating support, but it's pretty much just a small wrapper to hook it up with url_for and endpoints. One good thing about Flask-Marshmallow URL creation vs. Flask-RESTful's is that you can specify which attributes to replace which variables in the endpoint name with a dictionary argument - so whichever way this happens, that's a good feature to have.

Is it possible to access request.values directly from the post(self) method (for example)?

Or, where does reqparse get the request? Flask-Marshmallow can't be used to parse requests right now (afaik) because request.values returns an empty dictionary (actually CombinedMultiDict) within the methods.

@joshfriend
Copy link
Member

@joshfriend joshfriend commented Dec 3, 2014

Reqparse gets the request content from the flask.request object. You can import this yourself and use it in your app if you like.

@rozenm
Copy link

@rozenm rozenm commented Dec 14, 2014

+1

@ghost
Copy link

@ghost ghost commented Dec 18, 2014

@joshfriend
I'm not able to access request.form from the post() method within an API class even with sent data. More specifically, request.form exists but I get an empty ImmutableMultiDict. Is there another way to access request?

@linkyndy
Copy link

@linkyndy linkyndy commented Dec 30, 2014

👍

@sloria
Copy link

@sloria sloria commented Nov 13, 2015

I encourage people to take a look at flask-apispec, which has a similar API to Flask-RESTful's marshal_with. It supports input/output with webargs and marshmallow and adds automatic swagger documentation.

I am considering merging flask-apispec with flask-marshmallow if there is interest. I welcome your feedback on marshmallow-code/flask-marshmallow#26.

@imaia
Copy link

@imaia imaia commented Feb 5, 2016

+1 for marshmallow
I agree that having flask-restful focused on restful API problems, would make it even more useful and would avoid functionality "clash" with other popular APIs.

@jeffwidman
Copy link
Contributor

@jeffwidman jeffwidman commented Feb 29, 2016

👍 for using marshmallow/flask-marshmallow rather than custom-baked marshalling/serializing.

I've been really happy with flask-marshmallow on some of my projects.

@svenstaro at this point has the decision been made and just waiting on implementation?

@henadzit
Copy link

@henadzit henadzit commented Mar 1, 2016

I would suggest to refer marshmallow and webargs at this and this pages as more flexible alternatives for now.

If you're a newcomer and you start building your project with flask-restful, you soon realize that there're lots of limitations of regparse and marshal. But at the moment when you realize that, some code and some hacks have been written and they need to be redone. Talking from my personal experience...

I like marshmallow so far. Actually, it doesn't require any supporting code to use it along with flask-restful except returning error code explicitly. Personally, I don't like flask-marshmallow because it makes you to import the ma object (ma = Marshmallow(app)) in all resource modules.

@joshfriend
Copy link
Member

@joshfriend joshfriend commented Mar 1, 2016

@henadzit what are those links? Isn't that just the Flask-RESTful docs mirrored to a different RTD page?

@henadzit
Copy link

@henadzit henadzit commented Mar 2, 2016

@joshfriend oh, yes, they're. I meant to put links to the original documentation.

@lafrech
Copy link

@lafrech lafrech commented May 12, 2016

@ColeKettler:

I think it's worth noting that flask-classy looks pretty dead. No commits / response for almost a year and a half.

Indeed: apiguy/flask-classy#80.

There's an active fork called flask-classful.

jonafato added a commit to PyGotham/pygotham that referenced this issue Jul 4, 2016
Our use of Flask-Restful's marshalling functionality is hackish. There
exists significant support for moving Flask-Restful's internal
marshalling to Marshmallow [1], so we should switch to that.
Flask-Marshmallow is used for convenience functions (e.g. `jsonify`).

This changeset is strictly a port from Flask-Restful's builtin
marshalling mechanism to Marshmallow. TODOs and FIXMEs still exist and
should be addressed in a separate change.

Flask-Marshmallow relies on changes to `flask.jsonify` made in Flask
0.11, so our version must be upgraded in order to use it. 0.11.1 is the
latest stable release of Flask.

[1] flask-restful/flask-restful#335
@bradwood
Copy link

@bradwood bradwood commented May 9, 2017

Hey guys, did this conversation get anywhere?

Has reqparse been finally ripped out?

For someone about to to commit to flask-restful, what is the current best practice recommendation for handling complex JSON payloads?

@svenstaro
Copy link
Contributor Author

@svenstaro svenstaro commented Jul 10, 2017

The problem with actually doing this is that it would make this project effectively a wrapper around flask's routing system. At this point it might just be best to declare the whole project deprecated but maintained and refer people to projects and guides that easily replace all of this with more modern tools. Thoughts?

@taion
Copy link

@taion taion commented Jul 10, 2017

I would say that a lot of what Flask-RESTful does is better handled by composing more atomic libraries – essentially just using Marshmallow plus Flask MethodViews gets you a lot of the way. Content negotiation is something unique to Flask-RESTful, though – but many APIs don't need that.

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

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.