Review Request 40: basic RESTful API

[Martin Krizek]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-tflink.rhcloud.com/r/40/
-----------------------------------------------------------

Review request for blockerbugs.

Bugs: 392
    https://fedorahosted.org/fedora-qa/ticket/392

Repository: blockerbugs

Description
-------

Add endpoints:
 - list bugs
 - list updates
 - list spins
 - create spin

Diffs
-----

testing/test_validators.py PRE-CREATION 
  testing/test_api.py PRE-CREATION 
  blockerbugs/models/spin.py 99891448e78c7168d540b479ffd7ef00ce1eec1d 
  blockerbugs/controllers/api/validators.py PRE-CREATION 
  blockerbugs/controllers/api/iso8601.py PRE-CREATION 
  blockerbugs/controllers/api/errors.py PRE-CREATION 
  blockerbugs/controllers/api/api.py PRE-CREATION 
  blockerbugs/controllers/api/__init__.py PRE-CREATION 
  blockerbugs/__init__.py c0f298462670e2ca00ba63fd8e722d2babbe5760

Diff: http://reviewboard-tflink.rhcloud.com/r/40/diff/

Testing
-------

Wrote test suites.
I've tested on my develop instance.

Thanks,

Ilgiz Islamgulov

[Martin Krizek]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-tflink.rhcloud.com/r/40/#review56
-----------------------------------------------------------

There are GPL headers missing from most of the new source files - please add them

blockerbugs/controllers/api/api.py
http://reviewboard-tflink.rhcloud.com/r/40/#comment80

Why did you name the blueprint api_bp when none of the other blueprints use a '_bp' suffix? with the variable API_VERSION that can be set here, wouldn't it be better to call it api_v0 instead or just api until such a time when we have multiple API versions?

blockerbugs/controllers/api/errors.py
http://reviewboard-tflink.rhcloud.com/r/40/#comment78

api/errors is missing a GPL header

blockerbugs/controllers/api/iso8601.py
http://reviewboard-tflink.rhcloud.com/r/40/#comment81

why are you duplicating this code instead of pulling it in as a dependency? Isn't it the same code as https://pypi.python.org/pypi/iso8601 and https://admin.fedoraproject.org/pkgdb/acls/name/python-iso8601

blockerbugs/models/spin.py
http://reviewboard-tflink.rhcloud.com/r/40/#comment82

Is there a reason to make this change other than your sense of style? As I recall, most other model constructors do it the pre-change way

testing/test_validators.py
http://reviewboard-tflink.rhcloud.com/r/40/#comment85

this bothers me a little - is 12.3 really validated as an int?

testing/test_validators.py
http://reviewboard-tflink.rhcloud.com/r/40/#comment84

this should be a separate test case, not just put into one

testing/test_validators.py
http://reviewboard-tflink.rhcloud.com/r/40/#comment86

empty comment

testing/test_validators.py
http://reviewboard-tflink.rhcloud.com/r/40/#comment87

wouldn't this be better as test_int_list?

A couple of general concerns I have about the code:
 - your tests are, in general, way too compact. There are way too many assertions and checks per test that make it much more difficult to diagnose errors if/when they happen. I understand that adhering blindly to a 'one assertion per test' rule is silly but I think you've taken it too far in the other direction. Additional testing/poking shouldn't be required to figure out what is failing in a test (ie, whether the optional or required elements of the dict validator are failing)

- I'd like to see more explicit handling of the API version. I like having the version and I think it should stay, I just don't like hardcoding the 'v0' in all the tests without indicating that the test class is only for 'v0' - the file names shouldn't be generic if the api version they work with is specific.

Overall, I think it looks good. All the stuff I'm pointing out is minor stuff

- Tim Flink

On July 22, 2013, 12:04 p.m., Ilgiz Islamgulov wrote:
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard-tflink.rhcloud.com/r/40/
>
> (Updated July 22, 2013, 12:04 p.m.)
>
> Review request for blockerbugs.
>
> Bugs: 392
>     https://fedorahosted.org/fedora-qa/ticket/392
>
> Repository: blockerbugs
> Description
>
> Add endpoints:
> list bugs
> list updates
> list spins
> create spin
> Diffs
>
> testing/test_validators.py PRE-CREATION 
>   testing/test_api.py PRE-CREATION 
>   blockerbugs/models/spin.py 99891448e78c7168d540b479ffd7ef00ce1eec1d 
>   blockerbugs/controllers/api/validators.py PRE-CREATION 
>   blockerbugs/controllers/api/iso8601.py PRE-CREATION 
>   blockerbugs/controllers/api/errors.py PRE-CREATION 
>   blockerbugs/controllers/api/api.py PRE-CREATION 
>   blockerbugs/controllers/api/__init__.py PRE-CREATION 
>   blockerbugs/__init__.py c0f298462670e2ca00ba63fd8e722d2babbe5760
>
> Diff: http://reviewboard-tflink.rhcloud.com/r/40/diff/
> Testing
>
> Wrote test suites.
> I've tested on my develop instance.
>
> Thanks,
>
> Ilgiz Islamgulov

[Martin Krizek]

> On Aug. 2, 2013, 2:38 p.m., Tim Flink wrote:
> > blockerbugs/controllers/api/iso8601.py, line 48
> > http://reviewboard-tflink.rhcloud.com/r/40/diff/1/?file=533#file533line48
> > why are you duplicating this code instead of pulling it in as a dependency? Isn't it the same code as https://pypi.python.org/pypi/iso8601 and https://admin.fedoraproject.org/pkgdb/acls/name/python-iso8601

I thought that having include one file is better than having dependency. Will use this package.
> On Aug. 2, 2013, 2:38 p.m., Tim Flink wrote:
> > blockerbugs/controllers/api/api.py, line 19
> > http://reviewboard-tflink.rhcloud.com/r/40/diff/1/?file=531#file531line19
> > Why did you name the blueprint api_bp when none of the other blueprints use a '_bp' suffix? with the variable API_VERSION that can be set here, wouldn't it be better to call it api_v0 instead or just api until such a time when we have multiple API versions?

Just 'api' name conflict with module name. As for me better  to call it api_v0.
> On Aug. 2, 2013, 2:38 p.m., Tim Flink wrote:
> > blockerbugs/models/spin.py, line 68
> > http://reviewboard-tflink.rhcloud.com/r/40/diff/1/?file=535#file535line68
> > Is there a reason to make this change other than your sense of style? As I recall, most other model constructors do it the pre-change way

datetime.utcnow() is called once when it will load. I guess that date_requested should have time then model created.

In [2]: def foo(d=datetime.utcnow()):
    return d
   ...:

In [3]: foo()
Out[3]: datetime.datetime(2013, 8, 2, 18, 37, 20, 876235)

In [4]: %date -u
ERROR: Line magic function `%date` not found.
In [5]: !date -u
Fri Aug  2 18:37:43 UTC 2013

In [6]: foo()
Out[6]: datetime.datetime(2013, 8, 2, 18, 37, 20, 876235)
> On Aug. 2, 2013, 2:38 p.m., Tim Flink wrote:
> > testing/test_validators.py, line 20
> > http://reviewboard-tflink.rhcloud.com/r/40/diff/1/?file=537#file537line20
> > this bothers me a little - is 12.3 really validated as an int?

Looks like odd. Really float value should not be valid int.

On Aug. 2, 2013, 2:38 p.m., Ilgiz Islamgulov wrote:
> > A couple of general concerns I have about the code:
> >
> > your tests are, in general, way too compact. There are way too many assertions and checks per test that make it much more difficult to diagnose errors if/when they happen. I understand that adhering blindly to a 'one assertion per test' rule is silly but I think you've taken it too far in the other direction. Additional testing/poking shouldn't be required to figure out what is failing in a test (ie, whether the optional or required elements of the dict validator are failing)
> >
> > I'd like to see more explicit handling of the API version. I like having the version and I think it should stay, I just don't like hardcoding the 'v0' in all the tests without indicating that the test class is only for 'v0' - the file names shouldn't be generic if the api version they work with is specific.
> >
> > Overall, I think it looks good. All the stuff I'm pointing out is minor stuff

Thank you for your review.

I'm totally agree with your comments about tests. Also message with assertion should be useful.

Looks like there should be generic version of api tests and separated tests for specific api version. In my mind all api tests are generic now.

- Ilgiz

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-tflink.rhcloud.com/r/40/#review56
-----------------------------------------------------------

On July 22, 2013, 12:04 p.m., Ilgiz Islamgulov wrote:
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard-tflink.rhcloud.com/r/40/
>
> (Updated July 22, 2013, 12:04 p.m.)
>
> Review request for blockerbugs.
>
> Bugs: 392
>     https://fedorahosted.org/fedora-qa/ticket/392
>
> Repository: blockerbugs
> Description
>
> Add endpoints:
> list bugs
> list updates
> list spins
> create spin
> Diffs
>
> testing/test_validators.py PRE-CREATION 
>   testing/test_api.py PRE-CREATION 
>   blockerbugs/models/spin.py 99891448e78c7168d540b479ffd7ef00ce1eec1d 
>   blockerbugs/controllers/api/validators.py PRE-CREATION 
>   blockerbugs/controllers/api/iso8601.py PRE-CREATION 
>   blockerbugs/controllers/api/errors.py PRE-CREATION 
>   blockerbugs/controllers/api/api.py PRE-CREATION 
>   blockerbugs/controllers/api/__init__.py PRE-CREATION 
>   blockerbugs/__init__.py c0f298462670e2ca00ba63fd8e722d2babbe5760
>
> Diff: http://reviewboard-tflink.rhcloud.com/r/40/diff/
> Testing
>
> Wrote test suites.
> I've tested on my develop instance.
>
> Thanks,
>
> Ilgiz Islamgulov

[Martin Krizek]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-tflink.rhcloud.com/r/40/#review60
-----------------------------------------------------------

- Ilgiz Islamgulov

On July 22, 2013, 12:04 p.m., Ilgiz Islamgulov wrote:
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard-tflink.rhcloud.com/r/40/
>
> (Updated July 22, 2013, 12:04 p.m.)
>
> Review request for blockerbugs.
>
> Bugs: 392
>     https://fedorahosted.org/fedora-qa/ticket/392
>
> Repository: blockerbugs
> Description
>
> Add endpoints:
> list bugs
> list updates
> list spins
> create spin
> Diffs
>
> testing/test_validators.py PRE-CREATION 
>   testing/test_api.py PRE-CREATION 
>   blockerbugs/models/spin.py 99891448e78c7168d540b479ffd7ef00ce1eec1d 
>   blockerbugs/controllers/api/validators.py PRE-CREATION 
>   blockerbugs/controllers/api/iso8601.py PRE-CREATION 
>   blockerbugs/controllers/api/errors.py PRE-CREATION 
>   blockerbugs/controllers/api/api.py PRE-CREATION 
>   blockerbugs/controllers/api/__init__.py PRE-CREATION 
>   blockerbugs/__init__.py c0f298462670e2ca00ba63fd8e722d2babbe5760
>
> Diff: http://reviewboard-tflink.rhcloud.com/r/40/diff/
> Testing
>
> Wrote test suites.
> I've tested on my develop instance.
>
> Thanks,
>
> Ilgiz Islamgulov

[Martin Krizek]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-tflink.rhcloud.com/r/40/
-----------------------------------------------------------

(Updated Aug. 5, 2013, 10:15 a.m.)

Review request for blockerbugs.

Bugs: 392
    https://fedorahosted.org/fedora-qa/ticket/392

Repository: blockerbugs

Description
-------

Add endpoints:
 - list bugs
 - list updates
 - list spins
 - create spin

Diffs (updated)
-----

testing/test_validators.py PRE-CREATION 
  testing/test_api.py PRE-CREATION 
  requirements.txt 09e0318bc189512f5d324bda8879ad74c4763f95 
  blockerbugs/models/spin.py 99891448e78c7168d540b479ffd7ef00ce1eec1d 
  blockerbugs/controllers/api/validators.py PRE-CREATION 
  blockerbugs/controllers/api/errors.py PRE-CREATION 
  blockerbugs/controllers/api/api.py PRE-CREATION 
  blockerbugs/controllers/api/__init__.py PRE-CREATION 
  blockerbugs/__init__.py c4f2c34f5eac713253336d026aa9485259096cb0 
  blockerbugs.spec 726fa6920c67cfe36a2e544f97c9e0f16537f11e

Diff: http://reviewboard-tflink.rhcloud.com/r/40/diff/

Testing
-------

Wrote test suites.
I've tested on my develop instance.

Thanks,

Ilgiz Islamgulov

[Martin Krizek]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-tflink.rhcloud.com/r/40/#review62
-----------------------------------------------------------

blockerbugs.spec
http://reviewboard-tflink.rhcloud.com/r/40/#comment101

'Required' is not a valid keyword for spec files, it's 'Requires'. Also, if iso8601 is used during unit tests, you'll have to add a 'BuildRequires'. Have you built an RPM with this spec?

blockerbugs/controllers/api/api.py
http://reviewboard-tflink.rhcloud.com/r/40/#comment105

You're the author here, not me :)

There are several cases of this in this patch. I'm just marking one, though.

blockerbugs/controllers/api/api.py
http://reviewboard-tflink.rhcloud.com/r/40/#comment108

out of curiosity, how come you're using <rel_name> here instead of something like milestone?

blockerbugs/controllers/api/api.py
http://reviewboard-tflink.rhcloud.com/r/40/#comment106

One of the things that I'm wondering about is consistency with the existing pages and whether or not this is important.

Specifically, I'm questioning naming this route 'spin' when the gui uses 'spins' and whether this would count as an inconsistency for API discovery - thoughts?

I'm also wondering if it's useful enough to allow GET on milestone/<rel_num>/<rel_name>/spin/<id> to get one spin instead of all spins for a release/milestone

blockerbugs/controllers/api/api.py
http://reviewboard-tflink.rhcloud.com/r/40/#comment107

it seems to me that this POST might be simpler (ie, less validation) if the URI had an <id> at the end - do you think that would add or take away any utility of the API?

blockerbugs/controllers/api/api.py
http://reviewboard-tflink.rhcloud.com/r/40/#comment109

I think this should be calculated at request time instead of just parsing what the client sends in - the request time should be pretty much when the request is received, anyways

It's looking pretty good - I think you're planning to add a method for spin modification as we talked about on IRC which is one of my biggest concerns right now.

- Tim Flink

On Aug. 5, 2013, 10:15 a.m., Ilgiz Islamgulov wrote:
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard-tflink.rhcloud.com/r/40/
>
> (Updated Aug. 5, 2013, 10:15 a.m.)
>
> Review request for blockerbugs.
>
> Bugs: 392
>     https://fedorahosted.org/fedora-qa/ticket/392
>
> Repository: blockerbugs
> Description
>
> Add endpoints:
> list bugs
> list updates
> list spins
> create spin
> Diffs
>
> testing/test_validators.py PRE-CREATION 
>   testing/test_api.py PRE-CREATION 
>   requirements.txt 09e0318bc189512f5d324bda8879ad74c4763f95 
>   blockerbugs/models/spin.py 99891448e78c7168d540b479ffd7ef00ce1eec1d 
>   blockerbugs/controllers/api/validators.py PRE-CREATION 
>   blockerbugs/controllers/api/errors.py PRE-CREATION 
>   blockerbugs/controllers/api/api.py PRE-CREATION 
>   blockerbugs/controllers/api/__init__.py PRE-CREATION 
>   blockerbugs/__init__.py c4f2c34f5eac713253336d026aa9485259096cb0 
>   blockerbugs.spec 726fa6920c67cfe36a2e544f97c9e0f16537f11e
>
> Diff: http://reviewboard-tflink.rhcloud.com/r/40/diff/
> Testing
>
> Wrote test suites.
> I've tested on my develop instance.
>
> Thanks,
>
> Ilgiz Islamgulov

[Martin Krizek]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-tflink.rhcloud.com/r/40/
-----------------------------------------------------------

(Updated Aug. 8, 2013, 2:38 p.m.)

Review request for blockerbugs.

Changes
-------

merge develop branch

Bugs: 392
    https://fedorahosted.org/fedora-qa/ticket/392

Repository: blockerbugs

Description
-------

Add endpoints:
 - list bugs
 - list updates
 - list spins
 - create spin

Diffs (updated)
-----

testing/test_validators.py PRE-CREATION 
  testing/test_api.py PRE-CREATION 
  requirements.txt 09e0318bc189512f5d324bda8879ad74c4763f95 
  blockerbugs/models/spin.py 99891448e78c7168d540b479ffd7ef00ce1eec1d 
  blockerbugs/controllers/api/validators.py PRE-CREATION 
  blockerbugs/controllers/api/utils.py PRE-CREATION 
  blockerbugs/controllers/api/errors.py PRE-CREATION 
  blockerbugs/controllers/api/api.py PRE-CREATION 
  blockerbugs/controllers/api/__init__.py PRE-CREATION 
  blockerbugs/__init__.py bd9973579e80fc859f3e8d22c35753fbd024c5f0 
  blockerbugs.spec 726fa6920c67cfe36a2e544f97c9e0f16537f11e

Diff: http://reviewboard-tflink.rhcloud.com/r/40/diff/

Testing
-------

Wrote test suites.
I've tested on my develop instance.

Thanks,

Ilgiz Islamgulov

[Martin Krizek]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-tflink.rhcloud.com/r/40/#review68
-----------------------------------------------------------

Ship it!

Overall, what I'm seeing looks good.

Can you change the copyright dates to 2013?

I think that I'd like to see more discovery endpoints, like 'api/v0/' and 'api/v0/milestones/' etc. but that can be a later feature. I'd say merge this in and file a bug for the discovery endpoints. The copyright date doesn't need another review.

Good work, I'm glad to see this getting done!

- Tim Flink

On Aug. 8, 2013, 2:38 p.m., Ilgiz Islamgulov wrote:
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard-tflink.rhcloud.com/r/40/
>
> (Updated Aug. 8, 2013, 2:38 p.m.)
>
> Review request for blockerbugs.
>
> Bugs: 392
>     https://fedorahosted.org/fedora-qa/ticket/392
>
> Repository: blockerbugs
> Description
>
> Add endpoints:
> list bugs
> list updates
> list spins
> create spin
> Diffs
>
> testing/test_validators.py PRE-CREATION 
>   testing/test_api.py PRE-CREATION 
>   requirements.txt 09e0318bc189512f5d324bda8879ad74c4763f95 
>   blockerbugs/models/spin.py 99891448e78c7168d540b479ffd7ef00ce1eec1d 
>   blockerbugs/controllers/api/validators.py PRE-CREATION 
>   blockerbugs/controllers/api/utils.py PRE-CREATION 
>   blockerbugs/controllers/api/errors.py PRE-CREATION 
>   blockerbugs/controllers/api/api.py PRE-CREATION 
>   blockerbugs/controllers/api/__init__.py PRE-CREATION 
>   blockerbugs/__init__.py bd9973579e80fc859f3e8d22c35753fbd024c5f0 
>   blockerbugs.spec 726fa6920c67cfe36a2e544f97c9e0f16537f11e
>
> Diff: http://reviewboard-tflink.rhcloud.com/r/40/diff/
> Testing
>
> Wrote test suites.
> I've tested on my develop instance.
>
> Thanks,
>
> Ilgiz Islamgulov

[Martin Krizek]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-tflink.rhcloud.com/r/40/
-----------------------------------------------------------

(Updated Aug. 16, 2013, 1:38 p.m.)

Status
------

This change has been marked as submitted.

Review request for blockerbugs.

Bugs: 392
    https://fedorahosted.org/fedora-qa/ticket/392

Repository: blockerbugs

Description
-------

Add endpoints:
 - list bugs
 - list updates
 - list spins
 - create spin

Diffs
-----

testing/test_validators.py PRE-CREATION 
  testing/test_api.py PRE-CREATION 
  requirements.txt 09e0318bc189512f5d324bda8879ad74c4763f95 
  blockerbugs/models/spin.py 99891448e78c7168d540b479ffd7ef00ce1eec1d 
  blockerbugs/controllers/api/validators.py PRE-CREATION 
  blockerbugs/controllers/api/utils.py PRE-CREATION 
  blockerbugs/controllers/api/errors.py PRE-CREATION 
  blockerbugs/controllers/api/api.py PRE-CREATION 
  blockerbugs/controllers/api/__init__.py PRE-CREATION 
  blockerbugs/__init__.py bd9973579e80fc859f3e8d22c35753fbd024c5f0 
  blockerbugs.spec 726fa6920c67cfe36a2e544f97c9e0f16537f11e

Diff: http://reviewboard-tflink.rhcloud.com/r/40/diff/

Testing
-------

Wrote test suites.
I've tested on my develop instance.

Thanks,

Ilgiz Islamgulov