From 1acd4c232c84341c1d5c4c94af968e1f413edaf2 Mon Sep 17 00:00:00 2001 From: Douglas Thor Date: Wed, 27 Mar 2019 14:48:39 -0700 Subject: [PATCH] Change routes.MetricById to follow the API philosophy. + Only accept metric_id, not `name` + Don't return data on PUT/PATCH/DELETE + Updated related tests. --- src/trendlines/routes.py | 58 ++++++++------------- tests/test_routes.py | 107 ++++++++++++++++++--------------------- 2 files changed, 69 insertions(+), 96 deletions(-) diff --git a/src/trendlines/routes.py b/src/trendlines/routes.py index 8d43bbc..4a9a071 100644 --- a/src/trendlines/routes.py +++ b/src/trendlines/routes.py @@ -269,26 +269,26 @@ def post(self): return jsonify(model_to_dict(new)), 201 -@api_metric.route("/api/v1/metric/") +@api_metric.route("/api/v1/metric/") class MetricById(MethodView): @api_metric.response(MetricSchema) - def get(self, metric_name): + def get(self, metric_id): """ Return metric information as JSON """ - logger.debug("API: get metric '%s'" % metric_name) + logger.debug("API: get metric '%s'" % metric_id) try: - raw_data = db.Metric.get(db.Metric.name == metric_name) + raw_data = db.Metric.get(db.Metric.metric_id == metric_id) except DoesNotExist: - return ErrorResponse.metric_not_found(metric_name) + return ErrorResponse.metric_not_found(metric_id) data = model_to_dict(raw_data) return jsonify(data) - @api_metric.response(MetricSchema, code=201) - def put(self, metric_name): + @api_metric.response(MetricSchema, code=204) + def put(self, metric_id): """ Replace a metric with new values. @@ -308,9 +308,8 @@ def put(self, metric_name): Returns ------- - 200 : - Success. Returned JSON data has two keys: ``old_value`` and - ``new_value``, each containing a full :class:`orm.Metric` object. + 204 : + Success. 400 : Malformed JSON data (such as when ``name`` is missing) 404 : @@ -329,10 +328,10 @@ def put(self, metric_name): # First see if our item actually exists try: - metric = db.Metric.get(db.Metric.name == metric_name) + metric = db.Metric.get(db.Metric.metric_id == metric_id) old = model_to_dict(metric) except DoesNotExist: - return ErrorResponse.metric_not_found(metric_name) + return ErrorResponse.metric_not_found(metric_id) # Parse our json. try: @@ -351,17 +350,14 @@ def put(self, metric_name): try: metric.save() - new = model_to_dict(metric) except IntegrityError: # Failed the unique constraint on Metric.name return ErrorResponse.unique_metric_name_required(old['name'], name) - rv = {"old_value": old, "new_value": new} + return 204 - return jsonify(rv), 200 - - @api_metric.response(MetricSchema) - def patch(self, metric_name): + @api_metric.response(code=204) + def patch(self, metric_id): """ Update the values for a given metric. @@ -400,10 +396,10 @@ def patch(self, metric_name): # First see if our item actually exists try: - metric = db.Metric.get(db.Metric.name == metric_name) + metric = db.Metric.get(db.Metric.metric_id == metric_id) old = model_to_dict(metric) except DoesNotExist: - return ErrorResponse.metric_not_found(metric_name) + return ErrorResponse.metric_not_found(metric_id) metric = update_model_from_dict(metric, data) @@ -413,26 +409,14 @@ def patch(self, metric_name): # Failed the unique constraint on Metric.name return ErrorResponse.unique_metric_name_required(old['name'], metric.name) - new = model_to_dict(metric) - - # Only return the values that were requested to be changed (even if they - # did not change). - # This seems like a silly way to do it. Is there a better way? - rv = {'old_value': {}, 'new_value': {}} - for item in data.keys(): - rv['old_value'][item] = old[item] - rv['new_value'][item] = new[item] - - return jsonify(rv), 200 + return 204 @api_metric.response(code=204) - def delete(self, metric_name): - logger.debug("'api: DELETE '%s'" % metric_name) + def delete(self, metric_id): + logger.debug("'api: DELETE '%s'" % metric_id) try: - found = db.Metric.get(db.Metric.name == metric_name) + found = db.Metric.get(db.Metric.metric_id == metric_id) found.delete_instance() except DoesNotExist: - return ErrorResponse.metric_not_found(metric_name) - else: - return "", 204 + return ErrorResponse.metric_not_found(metric_id) diff --git a/tests/test_routes.py b/tests/test_routes.py index 8378747..2726a29 100644 --- a/tests/test_routes.py +++ b/tests/test_routes.py @@ -100,7 +100,7 @@ def test_api_delete_datapoint_not_found(client, populated_db, caplog): def test_api_get_metric_as_json(client, populated_db, caplog): - rv = client.get("/api/v1/metric/foo") + rv = client.get("/api/v1/metric/2") assert rv.status_code == 200 assert rv.is_json d = rv.get_json() @@ -110,18 +110,18 @@ def test_api_get_metric_as_json(client, populated_db, caplog): def test_api_get_metric_as_json_not_found(client, populated_db, caplog): - metric = "querty" - rv = client.get("/api/v1/metric/{}".format(metric)) + metric_id = 99 + rv = client.get("/api/v1/metric/{}".format(metric_id)) assert rv.status_code == 404 assert rv.is_json d = rv.get_json() - assert metric in d['detail'] + assert str(metric_id) in d['detail'] assert d['title'] == "NOT_FOUND" assert "API error" in caplog.text def test_api_delete_metric(client, populated_db): - metric = "foo.bar" + metric = 3 rv = client.delete("/api/v1/metric/{}".format(metric)) assert rv.status_code == 204 @@ -131,12 +131,12 @@ def test_api_delete_metric(client, populated_db): def test_api_delete_metric_not_found(client, populated_db, caplog): - metric = "missing" - rv = client.delete("/api/v1/metric/{}".format(metric)) + metric_id = 99 + rv = client.delete("/api/v1/metric/{}".format(metric_id)) assert rv.status_code == 404 assert rv.is_json d = rv.get_json() - assert metric in d['detail'] + assert str(metric_id) in d['detail'] assert d['title'] == "NOT_FOUND" assert "API error" in caplog.text @@ -180,36 +180,33 @@ def test_api_post_metric_missing_key(client, populated_db): def test_api_put_metric(client, populated_db): - name = "foo.bar" + metric_id = 3 data = { - "name": name, + "name": "foo.bar", "units": "lines", "upper_limit": 100, "lower_limit": 0, } - rv = client.put("/api/v1/metric/{}".format(name), json=data) - assert rv.status_code == 200 - assert rv.is_json - d = rv.get_json() - assert d['old_value']['units'] is None - assert d['new_value']['units'] == "lines" + rv = client.put("/api/v1/metric/{}".format(metric_id), json=data) + assert rv.status_code == 204 + assert rv.data == b"" def test_api_put_metric_sets_other_values_to_none(client, populated_db): - name = "with_everything" + metric_id = 6 # "with_everything" - original = client.get("/api/v1/metric/{}".format(name)) + original = client.get("/api/v1/metric/{}".format(metric_id)) assert original.status_code == 200 original = original.get_json() assert original['units'] == 'percent' assert original['upper_limit'] == 100.0 assert original['lower_limit'] == 20.0 - data = {"name": name} - rv = client.put("/api/v1/metric/{}".format(name), json=data) - assert rv.status_code == 200 + data = {"name": "with_everything"} + rv = client.put("/api/v1/metric/{}".format(metric_id), json=data) + assert rv.status_code == 204 - new = client.get("/api/v1/metric/{}".format(name)) + new = client.get("/api/v1/metric/{}".format(metric_id)) assert new.status_code == 200 new = new.get_json() @@ -219,26 +216,27 @@ def test_api_put_metric_sets_other_values_to_none(client, populated_db): def test_api_put_metric_not_found(client, populated_db): - name = "missing" + metric_id = 99 data = { - "name": name, + "name": "missing", "units": "lines", "upper_limit": 100, "lower_limit": 0, } - rv = client.put("/api/v1/metric/{}".format(name), json=data) + rv = client.put("/api/v1/metric/{}".format(metric_id), json=data) assert rv.status_code == 404 assert rv.is_json d = rv.get_json() - assert name in d['detail'] + assert str(metric_id) in d['detail'] assert "does not exist" in d['detail'] def test_api_put_metric_duplicate_name(client, populated_db): - new_name = "foo.bar" + metric_id = 5 old_name = "old_data" + new_name = "foo.bar" data = {"name": new_name} - rv = client.put("/api/v1/metric/{}".format(old_name), json=data) + rv = client.put("/api/v1/metric/{}".format(metric_id), json=data) assert rv.status_code == 409 assert rv.is_json d = rv.get_json() @@ -249,7 +247,7 @@ def test_api_put_metric_duplicate_name(client, populated_db): def test_api_put_metric_missing_name(client, populated_db): data = {"units": "goats"} - rv = client.put("/api/v1/metric/foo", json=data) + rv = client.put("/api/v1/metric/3", json=data) assert rv.status_code == 400 assert rv.is_json d = rv.get_json() @@ -258,44 +256,34 @@ def test_api_put_metric_missing_name(client, populated_db): def test_api_put_metric_idempotence(client, populated_db): # TODO: I don't think this is the right way to test idempotence... - name = "foo.bar" + metric_id = 3 data = { - "name": name, + "name": "foo.bar", "units": "lines", "upper_limit": 100, "lower_limit": 0, } - rv = client.put("/api/v1/metric/{}".format(name), json=data) - assert rv.status_code == 200 - assert rv.is_json - d = rv.get_json() - # First verify that things changed. - assert d['old_value']['units'] != d['new_value'] + rv = client.put("/api/v1/metric/{}".format(metric_id), json=data) + assert rv.status_code == 204 - rv = client.put("/api/v1/metric/{}".format(name), json=data) - assert rv.status_code == 200 - assert rv.is_json - d = rv.get_json() - # Then verify that they *didn't* change. - assert d['old_value'] == d['new_value'] + after_1 = client.get("/api/v1/metric/{}".format(metric_id)) + + rv = client.put("/api/v1/metric/{}".format(metric_id), json=data) + assert rv.status_code == 204 + + after_2 = client.get("/api/v1/metric/{}".format(metric_id)) + assert after_1.get_json() == after_2.get_json() def test_api_patch_metric(client, populated_db): data = {"units": "pears"} - rv = client.patch("/api/v1/metric/foo", json=data) - assert rv.status_code == 200 - assert rv.is_json - d = rv.get_json() - assert d['old_value']['units'] is None - assert d['new_value']['units'] == "pears" - assert 'name' not in d['old_value'].keys() - assert 'name' not in d['new_value'].keys() - assert 'lower_limit' not in d['old_value'].keys() - assert 'lower_limit' not in d['new_value'].keys() + rv = client.patch("/api/v1/metric/2", json=data) + assert rv.status_code == 204 + assert rv.data == b"" def test_api_patch_metric_doesnt_change_other_values(client, populated_db): - name = "with_everything" + name = 6 # "with_everything" original = client.get("/api/v1/metric/{}".format(name)) assert original.status_code == 200 original = original.get_json() @@ -305,7 +293,7 @@ def test_api_patch_metric_doesnt_change_other_values(client, populated_db): data = {"lower_limit": 0.2} rv = client.patch("/api/v1/metric/{}".format(name), json=data) - assert rv.status_code == 200 + assert rv.status_code == 204 new = client.get("/api/v1/metric/{}".format(name)) assert new.status_code == 200 @@ -317,21 +305,22 @@ def test_api_patch_metric_doesnt_change_other_values(client, populated_db): def test_api_patch_metric_not_found(client, populated_db): - name = "missing" + metric_id = 99 data = {"units": "pears"} - rv = client.patch("/api/v1/metric/{}".format(name), json=data) + rv = client.patch("/api/v1/metric/{}".format(metric_id), json=data) assert rv.status_code == 404 assert rv.is_json d = rv.get_json() - assert name in d['detail'] + assert str(metric_id) in d['detail'] assert "does not exist" in d['detail'] def test_api_patch_metric_duplicate_name(client, populated_db): new_name = "foo.bar" + metric_id = 5 old_name = "old_data" data = {"name": new_name} - rv = client.patch("/api/v1/metric/{}".format(old_name), json=data) + rv = client.patch("/api/v1/metric/{}".format(metric_id), json=data) assert rv.status_code == 409 assert rv.is_json d = rv.get_json()