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

@@ -269,26 +269,26 @@ def post(self):
         return jsonify(model_to_dict(new)), 201
 
 
-@api_metric.route("/api/v1/metric/<metric_name>")
+@api_metric.route("/api/v1/metric/<metric_id>")
 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)

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