Hmm, I guess I should have merged this request into PR #4

Instead of doing this you can just customize the 404 error handler (i.e. @api.errorhandler(404)).
http://flask.pocoo.org/docs/0.12/patterns/errorpages/

You can optionally add the error name so that it'd look like:

{'status_code': 404, 'error': 'Not Found', 'message': 'The requested resource was not found'}

Yeah, but it does not allow me to use custom error messages.

I think this is unnecessary as Flask will automatically fill the status filed with '404 NOT FOUND'.

You can with something like this in your init.py:

@api.errorhandler(404)
def not_found_error(e):
    ### Run your code to return the error here (e.g. JSON)

Hmm yeah maybe I being dense but I am not seeing the purpose of the second commit -- I guess you are wanting to improve the error response bodies somehow, but what exact difference does it make?

Redefining our own version of all those exception types seems tedious and I hope we could avoid having to do that.

But how could I have custom error messages? For example, If I want to raise 404 with an error message like 'Product version balbla is not found' .

Hmm yeah maybe I being dense but I am not seeing the purpose of the second commit -- I >guess you are wanting to improve the error response bodies somehow, but what exact >difference does it make?
Redefining our own version of all those exception types seems tedious and I hope we could >avoid having to do that.
Aha, you are right, I was being silly to redefine all those exceptions. My intention of doing this is to allow me to use custom error messages.

I have uploaded a new patch for adding an API to fetch a list of waivers. I will add more tests tomorrow as I want to get some feedback first.

Not sure why Resultsdb is not using the flask-sqlalchemy built-in pagination. If you look at its code, it's quite messy as they have implemented their own pagination which I think is quite tedious.

There is one thing I am not quite sure. The pagination in resultsdb is using page=0 as the first page[1], however the pagination function in flask-sqlalchemy is using page=1 as default [2]. I guess we should encourage them to use the built-in pagination.

[1] https://taskotron.fedoraproject.org/resultsdb_api/api/v2.0/results?page=1&limit=1
[2] http://flask-sqlalchemy.pocoo.org/2.1/api/

This is totally fine, but I personally like having more data like we do in the Module Build Service:

        'page': p_query.page,
        'per_page': p_query.per_page,
        'total': p_query.total,
        'pages': p_query.pages,
        'first': url_for(request.endpoint, page=1, per_page=p_query.per_page, _external=True),
        'last': url_for(request.endpoint, page=p_query.pages, per_page=p_query.per_page, _external=True)

Additionally, we store it in a meta dictionary so that the returned result looks like:

{
  "items": [
    {
      "id": 1,
      "state": 3
    },
    {
      "id": 2,
      "state": 3
    },
    {
      "id": 3,
      "state": 3
    },
    {
      "id": 4,
      "state": 4
    },
    {
      "id": 5,
      "state": 4
    },
    {
      "id": 6,
      "state": 4
    },
    {
      "id": 7,
      "state": 4
    },
    {
      "id": 8,
      "state": 4
    },
    {
      "id": 9,
      "state": 4
    },
    {
      "id": 10,
      "state": 1
    }
  ],
  "meta": {
    "first": "https://127.0.0.1:5000/module-build-service/1/module-builds/?per_page=10&page=1",
    "last": "https://127.0.0.1:5000/module-build-service/1/module-builds/?per_page=10&page=3",
    "next": "https://127.0.0.1:5000/module-build-service/1/module-builds/?per_page=10&page=2",
    "page": 1,
    "pages": 3,
    "per_page": 10,
    "total": 30
  }
}

It might be easier to use:

flask.request.args

Once you've modified the dict to the way you want, you can just pass it in to your url_for statement.

For more info:
http://flask.pocoo.org/docs/0.10/api/#flask.Request.args

Can you explain what this does please?

@mprahl but we don't want every 404 to say "Waiver not found", the message will depend which flask handler they were hitting.

I assume that @api.errorhandler(404) decorator is installing an error handler for every single 404 error produced by the app.

@dcallagh good point. In previous projects I've just returned something generic like the "Item not found". This is fine as it is.

@mjia I just had a few comments/optional suggestions. +1 from me.

By default waiverdb will exclude obsolete waivers by defaul(that is waivers where the same user has posted an updated waiver for the same result). If you include_obsolete=True, it will return the complete waiver history.

Well, that's something I am uncertain. Do we have a standard JSON API response format we can follow ? If the MBS is the one we should follow, I guess we should encourage the rest of our apps to follow this.

@ralph @mikeb what do you guys think?

Thanks, that is indeed much easier, :-)

There is the JSON API spec, which includes some rules for pagination:
http://jsonapi.org/format/#fetching-pagination

@dcallagh that's a good link.

@mjia for the first and last, you can do:

first = url_for(request.endpoint, page=1, per_page=paginated_query.per_page, _external=True),
last = url_for(request.endpoint, page=paginated_query.pages, per_page=paginated_query.per_page, _external=True)

@mprahl Thanks, I will introduce those two in the patch.

This is in Fedora, but not EPEL7. Is that a problem?

See https://apps.fedoraproject.org/packages/python-factory-boy

It also is only for the test suite, not for the app itself. Can you move it down a line so that is more clear?

@mprahl and @dcallagh are both on PTO now, so we're going to have to get some other people on to finish out this review. :)

:+1: from me on the general work here, although I'd like to see the factory_boy + EPEL7 issue resolved if EPEL7 is a deployment target.

I guess it shouldn't be hard to build it into EPEL-7. For internal, I have tried to build it into sed-rhel-7 and it seems like only python3 and python3-setuptools are missing, so I think it should not be hard to build it either.

+0.5 (Because it is first time I'm checking the waiverdb code). I did not find anything wrong there. I was not checking the sanity of the API deeply, but the code looks fine to me.

+1

I think you should consider if factory_boy is worth the effort to package though. It might just be easier to test without it instead of maintaining it on EPEL7.

I think you should consider if factory_boy is worth the effort to package though. It might just be easier to test without it instead of maintaining it on EPEL7.

Yeah, I will write a fixture for creating waivers then.

:+1: