[PATCH 3/3] Create the API namespace

Bohuslav Kabrda bkabrda at redhat.com
Wed Jan 23 12:38:52 UTC 2013


----- Original Message -----
> On Wed, 2013-01-23 at 07:11 -0500, Bohuslav Kabrda wrote:
> > ----- Original Message -----
> > > This is the namespace that we will use to describe and expose the
> > > copr API.
> > > This commit introduce it, move some code from misc to the
> > > api_general
> > > and
> > > start with two API functions to list someone's copr and add a new
> > > one.
> > > 
> > > These two functions are documented in the /api/ (ie in the
> > > api.html
> > > template).
> > > ---
> > > 
> > > This is very simple for the moment and I a beginning of a CLI
> > > that
> > > can call this
> > > API and do something with it. That will be for a later commit.
> > > 
> > > One point though, I was not able to allow sending the data via
> > > post,
> > > I am afraid
> > > that this is due to the fact that I do not use forms. I might be
> > > better finally
> > > to just re-use the forms with csrf deactivated. Bonus, re-using
> > > the
> > > forms gives
> > > us input validation as well.
> > > 
> > > Thoughts on this?
> > > 
> > 
> > Could you be more elaborate on what the problem with post data was?
> > Reusing forms seems to be a good idea, although we have to be
> > careful to pass the data in the format that forms expect.
> > 
> > <snip>
> > > + at api_ns.route('/add/<name>/<chroots>/',
> > > +    defaults={'repos':"", 'initial_pkgs':""})
> > > + at api_ns.route('/add/<name>/<chroots>/<repos>/',
> > > +    defaults={'initial_pkgs':None})
> > > + at api_ns.route('/add/<name>/<chroots>/<repos>/<initial_pkgs>/')
> > > + at login_required
> > > +def api_add_copr(name, chroots, repos="", initial_pkgs=""):
> > </snip>
> > 
> > Hmm, I really wouldn't do this. Let's take the time to do it via
> > POST or GET params, not part of the url.
> 
> You have here the answer to the question above. We need to re-use the
> form or parse the request argument by hand (:-/) to be able to use
> POST
> and/or GET.
> 

Then I'm +1 for reusing the forms. Turning off csrf is really easy (use "form = MyForm(csrf_enabled=False)") [1] and then it's just a matter of passing the data correctly to the Form [2].

> > <snip>
> > > + at api_ns.route('/list/<username>/', methods=['GET'])
> > > +def api_list_copr(username):
> > </snip>
> > 
> > What I'd like to see here is copying the URLs from the web frontend
> > part. E.g. listing all coprs should be just '/api/coprs/', listing
> > coprs for a user '/coprs/owned/<username>/'. Also, I'd name the
> > action for creation "new", not "add", as it corresponds with
> > "copr_new" from coprs_ns, not "copr_add".
> 
> I am afraid we are going to generate a lot of code duplication.
> I wonder if we couldn't make some sort of global variable which would
> allow to distinguish website/cli.
> Something like:
> 
> flask.g.output = 'html'
> 
> # In the login method:
> if call relies on api_token:
>    flask.g.output = 'json'
> 
> # In the views:
> if output == 'json':
>   return convert_to_json...
> else:
>   return render_template()
> 
> No idea if that would work but it would clearly reduce code
> duplication
> (although it might also make keeping the API stable harder).
> 

I was thinking a lot about this and I'm still not 100 % sure. It might be worth considering passing something like "?cli=True" with the request and then the views would decide whether to use csrf and what format to return based on that. It would be optimal to do an @app.before_request processor, that would set something like flask.g.cli to True/False and that could be used in views then.
What I fear about this is, that the views will grow insanely long and complicated, as each will contain the "ifs and elses" to decide what to actually render/etc. I think that adapting current views would be acceptable for now, so we may try going down that path, although I'm not sure where it will take us.

I'll leave this decision up to you, I don't want to waste your work by mandating this or that. Each way has its pros and cons (although after reconsidering it, altering current views might make more sense for the time being).

Thanks for your contributions!

> Pierre

[1] http://packages.python.org/Flask-WTF/#configuring-flask-wtf
[2] http://wtforms.simplecodes.com/docs/1.0.2/forms.html#wtforms.form.Form

-- 
Regards,
Bohuslav "Slavek" Kabrda.


More information about the copr-devel mailing list