PR#8: HTTP APIs for managing waivers - waiverdb

waiverdb

#8 HTTP APIs for managing waivers

Merged 7 years ago by mjia. Opened 7 years ago by mjia.

mjia/waiverdb get_a_waiver into master

support jsonp

Matt Jia • 7 years ago

7a87d3a

HTTP API for fetching a list of waivers

Matt Jia • 7 years ago

ab49b31

HTTP API for fetching a single waiver

Matt Jia • 7 years ago

4a11878

README.md

file modified

+1 -1

		`@@ -21,7 +21,7 @@`

		`$ DEV=true python runapp.py`

		`- The server is now running at <http://localhost:5004> and aPI calls can be sent to`
		`+ The server is now running at <http://localhost:5004> and API calls can be sent to`
		<http://localhost:5004/api/v1.0>. All data is stored inside `/var/tmp/waiverdb_db.sqlite`.

		`## Adjusting configuration`

tests/test_api_v10.py

file modified

+125

		`@@ -11,6 +11,8 @@`

		`import pytest`
		`import json`
		`+ from .utils import create_waiver`
		`+ import datetime`

		`def test_create_waiver(client, session):`
		`data = {`
		`@@ -38,3 +40,126 @@`
		`res_data = json.loads(r.data)`
		`assert r.status_code == 400`
		`assert 'invalid literal for int()' in res_data['message']['result_id']`
		`+`
		`+ def test_get_waiver(client, session):`
		`+ # create a new waiver`
		`+ waiver = create_waiver(session, result_id=123, username='foo',`
		`+ product_version='foo-1', comment='bla bla bla')`
		`+ r = client.get('/api/v1.0/waivers/%s' % waiver.id)`
		`+ res_data = json.loads(r.data)`
		`+ assert r.status_code == 200`
		`+ assert res_data['username'] == waiver.username`
		`+ assert res_data['result_id'] == waiver.result_id`
		`+ assert res_data['product_version'] == waiver.product_version`
		`+ assert res_data['waived'] == True`
		`+ assert res_data['comment'] == waiver.comment`
		`+`
		`+ def test_404_for_nonexistent_waiver(client, session):`
		`+ r = client.get('/api/v1.0/waivers/foo')`
		`+ assert r.status_code == 404`
		`+`
		`+ def test_get_waivers(client, session):`
		`+ for i in range(0,10):`
		`+ create_waiver(session, result_id=i, username='foo %d' % i,`
		`+ product_version='foo-%d' % i, comment='bla bla bla')`
		`+ r = client.get('/api/v1.0/waivers/')`
		`+ res_data = json.loads(r.data)`
		`+ assert r.status_code == 200`
		`+ assert len(res_data['data']) == 10`
		`+`
		`+ def test_pagination_waivers(client, session):`
		`+ for i in range(0,10):`
		`+ create_waiver(session, result_id=i, username='foo %d' % i,`
		`+ product_version='foo-%d' % i, comment='bla bla bla')`
		`+ r = client.get('/api/v1.0/waivers/?page=2&limit=1')`
		`+ res_data = json.loads(r.data)`
		`+ assert r.status_code == 200`
		`+ assert len(res_data['data']) == 1`
		`+ assert '/waivers/?limit=1&page=1' in res_data['prev']`
		`+ assert '/waivers/?limit=1&page=3' in res_data['next']`
		`+ assert '/waivers/?limit=1&page=1' in res_data['first']`
		`+ assert '/waivers/?limit=1&page=10' in res_data['last']`
		`+`
		`+ def test_obsolete_waivers_are_excluded_by_default(client, session):`
		`+ old_waiver = create_waiver(session, result_id=123, username='foo',`
		`+ product_version='foo-1')`
		`+ new_waiver = create_waiver(session, result_id=123, username='foo',`
		`+ product_version='foo-1', waived=False)`
		`+ r = client.get('/api/v1.0/waivers/')`
		`+ res_data = json.loads(r.data)`
		`+ assert r.status_code == 200`
		`+ assert len(res_data['data']) == 1`
		`+ assert res_data['data'][0]['id'] == new_waiver.id`
		`+ assert res_data['data'][0]['waived'] == new_waiver.waived`
		`+`
		`+ def test_get_obsolete_waivers(client, session):`
		`+ old_waiver = create_waiver(session, result_id=123, username='foo',`
		`+ product_version='foo-1')`
		`+ new_waiver = create_waiver(session, result_id=123, username='foo',`
		`+ product_version='foo-1', waived=False)`
		`+ r = client.get('/api/v1.0/waivers/?include_obsolete=1')`
		`+ res_data = json.loads(r.data)`
		`+ assert r.status_code == 200`
		`+ assert len(res_data['data']) == 2`
		`+ assert res_data['data'][0]['id'] == new_waiver.id`
		`+ assert res_data['data'][1]['id'] == old_waiver.id`
		`+`
		`+ def test_filtering_waivers_by_result_id(client, session):`
		`+ create_waiver(session, result_id=123, username='foo-1', product_version='foo-1')`
		`+ create_waiver(session, result_id=234, username='foo-2', product_version='foo-1')`
		`+ r = client.get('/api/v1.0/waivers/?result_id=123')`
		`+ res_data = json.loads(r.data)`
		`+ assert r.status_code == 200`
		`+ assert len(res_data['data']) == 1`
		`+ assert res_data['data'][0]['result_id'] == 123`
		`+`
		`+ def test_filtering_waivers_by_product_version(client, session):`
		`+ create_waiver(session, result_id=123, username='foo-1', product_version='release-1')`
		`+ create_waiver(session, result_id=124, username='foo-1', product_version='release-2')`
		`+ r = client.get('/api/v1.0/waivers/?product_version=release-1')`
		`+ res_data = json.loads(r.data)`
		`+ assert r.status_code == 200`
		`+ assert len(res_data['data']) == 1`
		`+ assert res_data['data'][0]['product_version'] == 'release-1'`
		`+`
		`+ def test_filtering_waivers_by_username(client, session):`
		`+ create_waiver(session, result_id=123, username='foo', product_version='foo-1')`
		`+ create_waiver(session, result_id=124, username='bar', product_version='foo-2')`
		`+ r = client.get('/api/v1.0/waivers/?username=foo')`
		`+ res_data = json.loads(r.data)`
		`+ assert r.status_code == 200`
		`+ assert len(res_data['data']) == 1`
		`+ assert res_data['data'][0]['username'] == 'foo'`
		`+`
		`+ def test_filtering_waivers_by_since(client, session):`
		`+ before1 = (datetime.datetime.utcnow() - datetime.timedelta(seconds=100)).isoformat()`
		`+ before2 = (datetime.datetime.utcnow() - datetime.timedelta(seconds=99)).isoformat()`
		`+ after = (datetime.datetime.utcnow() + datetime.timedelta(seconds=100)).isoformat()`
		`+ create_waiver(session, result_id=123, username='foo', product_version='foo-1')`
		`+ r = client.get('/api/v1.0/waivers/?since=%s' % before1)`
		`+ res_data = json.loads(r.data)`
		`+ assert r.status_code == 200`
		`+ assert len(res_data['data']) == 1`
		`+ assert res_data['data'][0]['result_id'] == 123`
		`+`
		`+ r = client.get('/api/v1.0/waivers/?since=%s,%s' % (before1, after))`
		`+ res_data = json.loads(r.data)`
		`+ assert r.status_code == 200`
		`+ assert len(res_data['data']) == 1`
		`+ assert res_data['data'][0]['result_id'] == 123`
		`+`
		`+ r = client.get('/api/v1.0/waivers/?since=%s' % (after))`
		`+ res_data = json.loads(r.data)`
		`+ assert r.status_code == 200`
		`+ assert len(res_data['data']) == 0`
		`+`
		`+ r = client.get('/api/v1.0/waivers/?since=%s,%s' % (before1, before2))`
		`+ res_data = json.loads(r.data)`
		`+ assert r.status_code == 200`
		`+ assert len(res_data['data']) == 0`
		`+`
		`+ def test_jsonp(client, session):`
		`+ waiver = create_waiver(session, result_id=123, username='foo', product_version='foo-1')`
		`+ r = client.get('/api/v1.0/waivers/%s?callback=jsonpcallback' % waiver.id)`
		`+ assert r.mimetype == 'application/javascript'`
		`+ assert 'jsonpcallback' in r.data`

tests/utils.py

file added

+20

		`@@ -0,0 +1,20 @@`
		`+`
		`+ # This program is free software; you can redistribute it and/or modify`
		`+ # it under the terms of the GNU General Public License as published by`
		`+ # the Free Software Foundation; either version 2 of the License, or`
		`+ # (at your option) any later version.`
		`+ #`
		`+ # This program is distributed in the hope that it will be useful,`
		`+ # but WITHOUT ANY WARRANTY; without even the implied warranty of`
		`+ # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the`
		`+ # GNU General Public License for more details.`
		`+ #`
		`+`
		`+ from waiverdb.models import Waiver`
		`+`
		`+ def create_waiver(session, result_id, username, product_version, waived=True,`
		`+ comment=None):`
		`+ waiver = Waiver(result_id, username, product_version, waived, comment)`
		`+ session.add(waiver)`
		`+ session.flush()`
		`+ return waiver`

waiverdb/api_v1.py

file modified

+64 -17

		`@@ -11,13 +11,16 @@`
		`#`

		`from flask import Blueprint`
		`- from flask_restful import reqparse`
		`- from werkzeug.exceptions import HTTPException`
		`+ from flask_restful import Resource, Api, reqparse, marshal_with`
		`+ from werkzeug.exceptions import HTTPException, BadRequest, NotFound`
		`+ from sqlalchemy.sql.expression import func`

		`from waiverdb.models import db, Waiver`
		`- from waiverdb.utils import to_json`
		`+ from waiverdb.utils import reqparse_since, json_collection, jsonp`
		`+ from waiverdb.fields import waiver_fields`

		`- api = Blueprint('api_v1', __name__)`
		`+ api_v1 = (Blueprint('api_v1', __name__))`
		`+ api = Api(api_v1)`

		`# RP contains request parsers (reqparse.RequestParser).`
		`# Parsers are added in each 'resource section' for better readability`
		`@@ -29,20 +32,64 @@`
		`RP['create_waiver'].add_argument('product_version', type=str, required=True, location='json')`
		`RP['create_waiver'].add_argument('comment', type=str, default=None, location='json')`

		`- @api.route('/waivers/', methods=['POST'])`
		`- @to_json`
		`- def create_waiver():`
		`- try:`
		`+ RP['get_waivers'] = reqparse.RequestParser()`
		`+ RP['get_waivers'].add_argument('result_id', location='args')`
		`+ RP['get_waivers'].add_argument('product_version', location='args')`
		`+ RP['get_waivers'].add_argument('username', location='args')`
		`+ RP['get_waivers'].add_argument('include_obsolete', type=bool, default=False, location='args')`
		`+ # XXX This matches the since query parameter in resultsdb but I think it would`
		`+ # be good to use two parameters(since and until).`
		`+ RP['get_waivers'].add_argument('since', location='args')`
		`+ RP['get_waivers'].add_argument('page', default=1, type=int, location='args')`
		`+ RP['get_waivers'].add_argument('limit', default=10, type=int, location='args')`
		`+`
		`+ class WaiversResource(Resource):`
		`+ @jsonp`
		`+ def get(self):`
		`+ args = RP['get_waivers'].parse_args()`
		`+ query = Waiver.query.order_by(Waiver.timestamp.desc())`
		`+ if args['result_id']:`
		`+ query = query.filter(Waiver.result_id.in_(args['result_id'].split(',')))`
		`+ if args['product_version']:`
		`+ query = query.filter(Waiver.product_version == args['product_version'])`
		`+ if args['username']:`
		`+ query = query.filter(Waiver.username == args['username'])`
		`+ if args['since']:`
		`+ try:`
		`+ since_start, since_end = reqparse_since(args['since'])`
		`+ except:`
		`+ raise BadRequest("'since' parameter not in ISO8601 format")`
		`+ if since_start:`
		`+ query = query.filter(Waiver.timestamp >= since_start)`
		`+ if since_end:`
		`+ query = query.filter(Waiver.timestamp <= since_end)`
		`+ if not args['include_obsolete']:`
		`+ subquery = db.session.query(func.max(Waiver.id)).group_by(Waiver.result_id)`
		`+ query = query.filter(Waiver.id.in_(subquery))`
		`+ return json_collection(query, args['page'], args['limit'])`
		`+`
		`+ @jsonp`
		`+ @marshal_with(waiver_fields)`
		`+ def post(self):`
		`args = RP['create_waiver'].parse_args()`
		`- except HTTPException as error:`
		`- return error.data, error.code`
		`+ # hardcode the username for now`
		`+ username = 'mjia'`
		`+ waiver = Waiver(args['result_id'], username, args['product_version'], args['waived'],`
		`+ args['comment'])`
		`+ db.session.add(waiver)`
		`+ db.session.commit()`
		`+ return waiver, 201`

		`- # hardcode the username for now`
		`- username = 'mjia'`

		`- waiver = Waiver(args['result_id'], username, args['product_version'], args['waived'],`
		`- args['comment'])`
		`+ class WaiverResource(Resource):`
		`+ @jsonp`
		`+ @marshal_with(waiver_fields)`
		`+ def get(self, waiver_id):`
		`+ try:`
		`+ return Waiver.query.get_or_404(waiver_id)`
		`+ except:`
		`+ raise NotFound('Waiver not found')`

		`- db.session.add(waiver)`
		`- db.session.commit()`
		`- return waiver, 201`
		`+ # set up the Api resource routing here`
		`+ api.add_resource(WaiversResource, '/waivers/')`
		`+ api.add_resource(WaiverResource, '/waivers/<int:waiver_id>')`

waiverdb/app.py

file modified

+1 -1

		`@@ -13,7 +13,7 @@`
		`from flask import Flask`
		`from flask_sqlalchemy import SQLAlchemy`
		`from waiverdb.logger import init_logging`
		`- from waiverdb.api_v1 import api as api_v1`
		`+ from waiverdb.api_v1 import api_v1`
		`from waiverdb.models import db`

		`def load_default_config(app):`

waiverdb/config.py

file modified

		`@@ -19,6 +19,9 @@`
		`PRODUCTION = False`
		`SHOW_DB_URI = False`
		`SECRET_KEY = 'replace-me-with-something-random'`
		`+ # need to explicitly turn this off`
		`+ # https://github.com/flask-restful/flask-restful/issues/449`
		`+ ERROR_404_HELP = False`


		`class ProductionConfig(Config):`

waiverdb/fields.py

file added

+22

		`@@ -0,0 +1,22 @@`
		`+`
		`+ # This program is free software; you can redistribute it and/or modify`
		`+ # it under the terms of the GNU General Public License as published by`
		`+ # the Free Software Foundation; either version 2 of the License, or`
		`+ # (at your option) any later version.`
		`+ #`
		`+ # This program is distributed in the hope that it will be useful,`
		`+ # but WITHOUT ANY WARRANTY; without even the implied warranty of`
		`+ # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the`
		`+ # GNU General Public License for more details.`
		`+`
		`+ from flask_restful import fields`
		`+`
		`+ waiver_fields = {`
		`+ 'id': fields.Integer,`
		`+ 'result_id': fields.Integer,`
		`+ 'username': fields.String,`
		`+ 'product_version': fields.String,`
		`+ 'waived': fields.Boolean,`
		`+ 'comment': fields.String,`
		`+ 'timestamp': fields.DateTime(dt_format='iso8601'),`
		`+ }`

waiverdb/models/waivers.py

file modified

-11

		`@@ -32,14 +32,3 @@`
		`return '%s(result_id=%r, username=%r, product_version=%r, waived=%r)' % (`
		`self.__class__.__name__, self.result_id, self.username,`
		`self.product_version, self.waived)`
		`-`
		`- def __json__(self):`
		`- return {`
		`- 'id': self.id,`
		`- 'result_id': self.result_id,`
		`- 'username': self.username,`
		`- 'product_version': self.product_version,`
		`- 'waived': self.waived,`
		`- 'comment': self.comment,`
		`- 'timestamp': self.timestamp.isoformat(),`
		`- }`

waiverdb/utils.py

file modified

+64 -23

		`@@ -9,30 +9,71 @@`
		`# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the`
		`# GNU General Public License for more details.`

		`+ import datetime`
		`import functools`
		`- from flask import jsonify`
		`+ from flask import request, url_for, jsonify`
		`+ from flask_restful import marshal`
		`+ from waiverdb.fields import waiver_fields`
		`+ from werkzeug.exceptions import NotFound`

		`- # https://github.com/miguelgrinberg/api-pycon2015/blob/master/api/decorators.py#L8-L30`
		`- def to_json(f):`
		`- """This decorator generates a JSON response from a Python dictionary or`
		`- a SQLAlchemy model."""`
		`- @functools.wraps(f)`
		`- def wrapped(args, *kwargs):`
		`- rv = f(args, *kwargs)`
		`- status_or_headers = None`
		`- headers = None`
		`- if isinstance(rv, tuple):`
		`- rv, status_or_headers, headers = rv + (None,) * (3 - len(rv))`
		`- if isinstance(status_or_headers, (dict, list)):`
		`- headers, status_or_headers = status_or_headers, None`
		`- if not isinstance(rv, dict):`
		`- # assume it is a model, call its __json__() method`
		`- rv = rv.__json__()`
		`+ def reqparse_since(since):`
		`+ """`
		`+ This parses the since(i.e. 2017-02-13T23:37:58.193281, 2017-02-16T23:37:58.193281)`
		`+ query parameter and returns a tuple.`
		`+ """`
		`+ start = None`
		`+ end = None`
		`+ if ',' in since:`
		`+ start, end = since.split(',')`
		`+ else:`
		`+ start = since`
		`+ if start:`
		`+ start = datetime.datetime.strptime(start, "%Y-%m-%dT%H:%M:%S.%f")`
		`+ if end:`
		`+ end = datetime.datetime.strptime(end, "%Y-%m-%dT%H:%M:%S.%f")`
		`+ return start, end`
		`+`
		`+ def json_collection(query, page=1, limit=10):`
		`+ """`
		`+ Helper function for Flask request handlers which want to return`
		`+ a collection of resources as JSON.`
		`+ """`
		`+ try:`
		`+ p = query.paginate(page, limit)`
		`+ except NotFound:`
		`+ return {'data': [], 'prev': None, 'next': None, 'first': None, 'last':None}`
		`+ pages = {'data': marshal(p.items, waiver_fields)}`
		`+ query_pairs = request.args.copy()`
		`+ if query_pairs:`
		`+ # remove the page number`
		`+ query_pairs.pop('page', default=None)`
		`+ if p.has_prev:`
		`+ pages['prev'] = url_for(request.endpoint, page=p.prev_num, _external=True,`
		`+ **query_pairs)`
		`+ else:`
		`+ pages['prev'] = None`
		`+ if p.has_next:`
		`+ pages['next'] = url_for(request.endpoint, page=p.next_num, _external=True,`
		`+ **query_pairs)`
		`+ else:`
		`+ pages['next'] = None`
		`+ pages['first'] = url_for(request.endpoint, page=1, _external=True, **query_pairs)`
		`+ pages['last'] = url_for(request.endpoint, page=p.pages, _external=True, **query_pairs)`
		`+ return pages`

		`- rv = jsonify(rv)`
		`- if status_or_headers is not None:`
		`- rv.status_code = status_or_headers`
		`- if headers is not None:`
		`- rv.headers.extend(headers)`
		`- return rv`
		`+ def jsonp(func):`
		`+ """Wraps Jsonified output for JSONP requests."""`
		`+ @functools.wraps(func)`
		`+ def wrapped(args, *kwargs):`
		`+ callback = request.args.get('callback', False)`
		`+ if callback:`
		`+ resp = jsonify(func(args, *kwargs))`
		`+ resp.set_data('{}({})'.format(`
		`+ str(callback),`
		`+ resp.get_data()`
		`+ ))`
		`+ resp.mimetype = 'application/javascript'`
		`+ return resp`
		`+ else:`
		`+ return func(args, *kwargs)`
		`return wrapped`

no initial comment

mjia commented 7 years ago

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

rebased

7 years ago

mprahl commented on line 40 of waiverdb/api_v1.py 7 years ago

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/

mprahl commented on line 20 of waiverdb/errors.py 7 years ago

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'}

mjia commented on line 40 of waiverdb/api_v1.py 7 years ago

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

mjia commented on line 20 of waiverdb/errors.py 7 years ago

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

mprahl commented on line 40 of waiverdb/api_v1.py 7 years ago

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)

Edited 7 years ago by mprahl

dcallagh commented 7 years ago

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.

mjia commented on line 40 of waiverdb/api_v1.py 7 years ago

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' .

Edited 7 years ago by mjia

mjia commented 7 years ago

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.

rebased

7 years ago

rebased

7 years ago

1 new commit added

HTTP API for fetching a list of waivers

7 years ago

mjia commented 7 years ago

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.

Edited 7 years ago by mjia

mjia commented on line 56 of waiverdb/utils.py 7 years ago

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/

Edited 7 years ago by mjia

mprahl commented on line 56 of waiverdb/utils.py 7 years ago

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
  }
}

Edited 7 years ago by mprahl

mprahl commented on line 66 of waiverdb/utils.py 7 years ago

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

Edited 7 years ago by mprahl

mprahl commented on line 61 of waiverdb/api_v1.py 7 years ago

Can you explain what this does please?

dcallagh commented 7 years ago

@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.

mprahl commented 7 years ago

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

mprahl commented 7 years ago

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

mjia commented on line 61 of waiverdb/api_v1.py 7 years ago

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.

mjia commented on line 56 of waiverdb/utils.py 7 years ago

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.

mjia commented on line 56 of waiverdb/utils.py 7 years ago

@ralph @mikeb what do you guys think?

Edited 7 years ago by mjia

mjia commented on line 66 of waiverdb/utils.py 7 years ago

Thanks, that is indeed much easier, :-)

rebased

7 years ago

3 new commits added

support jsonp
HTTP API for fetching a list of waivers
HTTP API for fetching a single waiver

7 years ago

dcallagh commented 7 years ago

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

mprahl commented 7 years ago

@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)

mjia commented 7 years ago

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

3 new commits added

support jsonp
HTTP API for fetching a list of waivers
HTTP API for fetching a single waiver

7 years ago

ralph commented on line 5 of requirements.txt 7 years ago

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

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

ralph commented on line 5 of requirements.txt 7 years ago

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?

ralph commented 7 years ago

@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. :)

ralph commented 7 years ago

:+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.

mjia commented on line 5 of requirements.txt 7 years ago

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.

3 new commits added

support jsonp
HTTP API for fetching a list of waivers
HTTP API for fetching a single waiver

7 years ago

jkaluza commented 7 years ago

+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.

mprahl commented 7 years ago

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.

mjia commented 7 years ago

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.

rebased

7 years ago

rebased

7 years ago

ralph commented 7 years ago

:+1:

Pull-Request has been merged by mjia

7 years ago