----- 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>
> > +(a)api_ns.route('/add/<name>/<chroots>/',
> > + defaults={'repos':"",
'initial_pkgs':""})
> > +(a)api_ns.route('/add/<name>/<chroots>/<repos>/',
> > + defaults={'initial_pkgs':None})
> >
+@api_ns.route('/add/<name>/<chroots>/<repos>/<initial_pkgs>/')
> > +@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>
> > +(a)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.