[LNT] Add more information to error messages

[kpdev42]

Summary

Add some useful information to HTTP errors with codes 400 and 404 to make it easier to understand which argument is missing or incorrect.

[thopre]

lnt/server/ui/views.py

817–819

Is it still the case, I remember a patch that wanted to allow arbitrary strings

[kpdev42]

lnt/server/ui/views.py

817–819

AFAIK, yes, URL is expected to contain something like ?plot.0=1.2.3 . If you can point to a patch, which allows arbitrary string, it would be useful and I'll change this part

[slydiman]

lnt/server/ui/views.py

817–819

It is just a copy of the line 1716. The same is in the line 855.

[cmatthews]

LGTM. Thanks!

[thopre]

lnt/server/ui/views.py

817–819

I was thinking of https://reviews.llvm.org/D109577, I don't know if it's related to the value here.

[cmatthews]

lnt/server/ui/views.py

817–819

They are separate. This one is the data lookup used internally to resolve a particular data set; where as, the one int the review is the order format, which is used for x-axis and change-over-time calculations.

[thopre]

lnt/server/ui/views.py

817–819

LGTM too then

lnt/server/ui/views.py

def favicon_ico():
     return v4_redirect(url_for('.static', filename='favicon.ico'))
 
 
 @frontend.route('/select_db')
 def select_db():
     path = request.args.get('path')
     db = request.args.get('db')
     if path is None:
-        abort(400)
+        abort(400, "'path' argument is missing")
     if db not in current_app.old_config.databases:
-        abort(404)
+        abort(404, "'db' argument is missing or invalid")
 
     # Rewrite the path.
     new_path = "/db_%s" % db
     if not path.startswith("/db_"):
         new_path += path
     else:
         if '/' in path[1:]:
             new_path += "/" + path.split("/", 2)[2]
associated_runs = multidict.multidict(
                              .join(ts.Order)
                              .filter(ts.Run.machine_id == id)
                              .order_by(ts.Run.start_time.desc())))
     associated_runs = sorted(associated_runs.items())
 
     try:
         machine = session.query(ts.Machine).filter(ts.Machine.id == id).one()
     except NoResultFound:
-        abort(404)
+        abort(404, "Invalid machine id {}".format(id))
 
     if request.args.get('json'):
         json_obj = dict()
         json_obj['name'] = machine.name
         json_obj['id'] = machine.id
         json_obj['runs'] = []
         for order in associated_runs:
             rev = order[0].llvm_project_revision

 class V4RequestInfo(object):
     def __init__(self, run_id):
         session = request.session
         self.db = request.get_db()
         self.session = session
         self.ts = ts = request.get_testsuite()
         self.run = run = session.query(ts.Run).filter_by(id=run_id).first()
         if run is None:
-            abort(404)
+            abort(404, "Invalid run id {}".format(run_id))
 
         # Get the aggregation function to use.
         aggregation_fn_name = request.args.get('aggregation_fn')
         self.aggregation_fn = {'min': lnt.util.stats.safe_min,
                                'median': lnt.util.stats.median}.get(
             aggregation_fn_name, lnt.util.stats.safe_min)
 
         # Get the MW confidence level.
try:
         form.name.data = baseline.name
         form.description.data = baseline.comment
     except NoResultFound:
         pass
 
     # Get the order.
     order = session.query(ts.Order).filter(ts.Order.id == id).first()
     if order is None:
-        abort(404)
+        abort(404, "Invalid order id {}".format(id))
 
     previous_order = None
     if order.previous_order_id:
         previous_order = session.query(ts.Order) \
             .filter(ts.Order.id == order.previous_order_id).one()
     next_order = None
     if order.next_order_id:
         next_order = session.query(ts.Order) \

 
 @v4_route("/set_baseline/<int:id>")
 def v4_set_baseline(id):
     """Update the baseline stored in the user's session."""
     session = request.session
     ts = request.get_testsuite()
     base = session.query(ts.Baseline).get(id)
     if not base:
-        return abort(404)
+        return abort(404, "Invalid baseline id {}".format(id))
     flash("Baseline set to " + base.name, FLASH_SUCCESS)
     flask.session[baseline_key(ts.name)] = id
 
     return v4_redirect(get_redirect_target())
 
 
 @v4_route("/all_orders")
 def v4_all_orders():

 def v4_run_graph(id):
     # This is an old style endpoint that treated graphs as associated with
     # runs. Redirect to the new endpoint.
 
     session = request.session
     ts = request.get_testsuite()
     run = session.query(ts.Run).filter_by(id=id).first()
     if run is None:
-        abort(404)
+        abort(404, "Invalid run id {}".format(id))
 
     # Convert the old style test parameters encoding.
     args = {'highlight_run': id}
     plot_number = 0
     for name, value in request.args.items():
         # If this isn't a test specification, just forward it.
         if not name.startswith('test.'):
             args[name] = value
def v4_graph_for_sample(sample_id, field_name):
     :param field_name: the name of the field.
     :return: a redirect to the graph page for that sample and field.
 
     """
     session = request.session
     ts = request.get_testsuite()
     target_sample = session.query(ts.Sample).get(sample_id)
     if not target_sample:
-        abort(404, "Could not find sample id: {}".format(sample_id))
+        abort(404, "Could not find sample id {}".format(sample_id))
 
     # Get the field index we are interested in.
     field_index = None
     for idx, f in enumerate(ts.sample_fields):
         if f.name == field_name:
             field_index = idx
             break
     if field_index is None:
for name, value in request.args.items():
 
         # Ignore the extra part of the key, it is unused.
         try:
             machine_id_str, test_id_str, field_index_str = value.split('.')
             machine_id = int(machine_id_str)
             test_id = int(test_id_str)
             field_index = int(field_index_str)
         except ValueError:
-            return abort(400)
+            return abort(400, "Parameter {} was malformed. {} must be int.int.int" \
+                              .format(name, value))
 
         if not (0 <= field_index < len(ts.sample_fields)):
-            return abort(404)
+            return abort(404, "Invalid field index {}".format(field_index))
 
         try:
             machine = session.query(ts.Machine) \
                              .filter(ts.Machine.id == machine_id) \
                              .one()
+        except NoResultFound:
+            return abort(404, "Invalid machine id {}".format(machine_id))
+        try:
             test = session.query(ts.Test).filter(ts.Test.id == test_id).one()
+        except NoResultFound:
+            return abort(404, "Invalid test id {}".format(test_id))
+        try:
             field = ts.sample_fields[field_index]
         except NoResultFound:
-            return abort(404)
+            return abort(404, "Invalid field_index {}".format(field_index))
         graph_parameters.append(GraphParameter(machine, test, field, field_index))
 
     # Order the plots by machine name, test name and then field.
     graph_parameters.sort(key=lambda graph_parameter:
                           (graph_parameter.machine.name, graph_parameter.test.name,
                            graph_parameter.field.name, graph_parameter.field_index))
 
     # Extract requested mean trend.
     mean_parameter = None
     for name, value in request.args.items():
         # Mean to graph is passed as:
         #
         #  mean=<machine id>.<field index>
         if name != 'mean':
             continue
 
         machine_id_str, field_index_str = value.split('.')
         try:
             machine_id = int(machine_id_str)
             field_index = int(field_index_str)
         except ValueError:
-            return abort(400)
+            return abort(400, "Parameter {} was malformed. {} must be int.int" \
+                              .format(name, value))
 
         if not (0 <= field_index < len(ts.sample_fields)):
-            return abort(404)
+            return abort(404, "Invalid field index {}".format(field_index))
 
         try:
             machine = session.query(ts.Machine) \
                 .filter(ts.Machine.id == machine_id) \
                 .one()
         except NoResultFound:
-            return abort(404)
+            return abort(404, "Invalid machine id {}".format(machine_id))
         field = ts.sample_fields[field_index]
 
         mean_parameter = (machine, field)
 
     # Sanity check the arguments.
     if not graph_parameters and not mean_parameter:
         return render_template("error.html", message="Nothing to graph.")
 
     # Extract requested baselines, and their titles.
     baseline_parameters = []
     for name, value in request.args.items():
         # Baselines to graph are passed as:
         #
         #  baseline.title=<run id>
         if not name.startswith(str('baseline.')):
             continue
 
         baseline_title = name[len('baseline.'):]
 
         run_id_str = value
         try:
             run_id = int(run_id_str)
         except Exception:
-            return abort(400)
+            return abort(400, "Invalid baseline run id {}".format(run_id_str))
 
         try:
             run = session.query(ts.Run) \
                 .options(joinedload(ts.Run.machine)) \
                 .filter(ts.Run.id == run_id) \
                 .one()
         except Exception:
             err_msg = ("The run {} was not found in the database."
                        .format(run_id))
             return render_template("error.html",
                                    message=err_msg)
 
         baseline_parameters.append((run, baseline_title))
 
     # Create region of interest for run data region if we are performing a
     # comparison.
     revision_range = None
     highlight_run_id = request.args.get('highlight_run')
     if show_highlight and highlight_run_id and highlight_run_id.isdigit():
         highlight_run = session.query(ts.Run).filter_by(
             id=int(highlight_run_id)).first()
         if highlight_run is None:
-            abort(404)
+            abort(404, "Invalid highlight_run id {}".format(highlight_run_id))
 
         # Find the neighboring runs, by order.
         prev_runs = list(ts.get_previous_runs_on_machine(session,
                                                          highlight_run, N=1))
         if prev_runs:
             start_rev = prev_runs[0].order.llvm_project_revision
             end_rev = highlight_run.order.llvm_project_revision
             revision_range = {
for name, value in request.args.items():
 
         # Ignore the extra part of the key, it is unused.
         machine_id_str, test_id_str, field_index_str = value.split('.')
         try:
             machine_id = int(machine_id_str)
             test_id = int(test_id_str)
             field_index = int(field_index_str)
         except ValueError:
-            err_msg = "data {} was malformed. {} must be int.int.int"
+            err_msg = "Parameter {} was malformed. {} must be int.int.int"
             return abort(400, err_msg.format(name, value))
 
         if not (0 <= field_index < len(ts.sample_fields)):
-            return abort(404, "Invalid field index: {}".format(field_index))
+            return abort(404, "Invalid field index {}".format(field_index))
 
         try:
             machine = session.query(ts.Machine) \
                 .filter(ts.Machine.id == machine_id) \
                 .one()
         except NoResultFound:
-            return abort(404, "Invalid machine ID: {}".format(machine_id))
+            return abort(404, "Invalid machine id {}".format(machine_id))
         try:
             test = session.query(ts.Test).filter(ts.Test.id == test_id).one()
         except NoResultFound:
-            return abort(404, "Invalid test ID: {}".format(test_id))
+            return abort(404, "Invalid test id {}".format(test_id))
         try:
             field = ts.sample_fields[field_index]
         except NoResultFound:
-            return abort(404, "Invalid field_index: {}".format(field_index))
+            return abort(404, "Invalid field_index {}".format(field_index))
 
         valid_request = MatrixDataRequest(machine, test, field)
         data_parameters.append(valid_request)
 
     if not data_parameters:
-        abort(404, "Request requires some data arguments.")
+        abort(404, "Request requires some plot arguments.")
     # Feature: if all of the results are from the same machine, hide the name
     # to make the headers more compact.
     dedup = True
     for r in data_parameters:
         if r.machine.id != data_parameters[0].machine.id:
             dedup = False
     if dedup:
         machine_name_common = data_parameters[0].machine.name
for req in data_parameters:
 
         req.samples = defaultdict(list)
 
         for s in q.all():
             req.samples[s[1]].append(s[0])
             all_orders.add(s[1])
             order_to_id[s[1]] = s[2]
     if not all_orders:
-        abort(404, "No data found.")
+        abort(404, "No orders found.")
     # Now grab the baseline data.
     user_baseline = baseline()
     backup_baseline = next(iter(all_orders))
     if user_baseline:
         all_orders.add(user_baseline.order.llvm_project_revision)
         baseline_rev = user_baseline.order.llvm_project_revision
         baseline_name = user_baseline.name
     else:

tests/server/ui/test_matrix_page.py

def setUp(self):
         self.client = app.test_client()
 
     def test_config_errors(self):
         """Does passing bad arguments to matrix view error correctly.
         """
         client = self.client
         reply = check_code(client, '/v4/nts/matrix',
                            expected_code=HTTP_NOT_FOUND)
-        self.assertIn("Request requires some data arguments.",
+        self.assertIn("Request requires some plot arguments.",
                       reply.get_data(as_text=True))
 
         reply = check_code(client, '/v4/nts/matrix?plot.0=1.1.1',
                            expected_code=HTTP_NOT_FOUND)
-        self.assertIn("No data found.", reply.get_data(as_text=True))
+        self.assertIn("No orders found.", reply.get_data(as_text=True))
 
         reply = check_code(client, '/v4/nts/matrix?plot.0=a.2.0',
                            expected_code=HTTP_BAD_REQUEST)
         self.assertIn("malformed", reply.get_data(as_text=True))
 
         reply = check_code(client, '/v4/nts/matrix?plot.0=999.0.0',
                            expected_code=HTTP_NOT_FOUND)
         self.assertIn("Invalid machine", reply.get_data(as_text=True))