Parameter feedback - #1 Server errors

client/app/services/query.js

@@ -2,7 +2,7 @@ import moment from 'moment';
 import debug from 'debug';
 import Mustache from 'mustache';
 import {
-  zipObject, isEmpty, map, filter, includes, union,
+  zipObject, isEmpty, map, includes, union,
   uniq, has, identity, extend, each, some,
 } from 'lodash';
 
@@ -107,10 +107,6 @@ class Parameters {
     return param;
   }
 
-  getMissing() {
-    return map(filter(this.get(), p => p.isEmpty), i => i.title);
-  }
-
   isRequired() {
     return !isEmpty(this.get());
   }
@@ -313,23 +309,6 @@ function QueryResource(
 
   QueryService.prototype.prepareQueryResultExecution = function prepareQueryResultExecution(execute, maxAge) {
     const parameters = this.getParameters();
-    const missingParams = parameters.getMissing();
-
-    if (missingParams.length > 0) {
-      let paramsWord = 'parameter';
-      let valuesWord = 'value';
-      if (missingParams.length > 1) {
-        paramsWord = 'parameters';
-        valuesWord = 'values';
-      }
-
-      return new QueryResult({
-        job: {
-          error: `missing ${valuesWord} for ${missingParams.join(', ')} ${paramsWord}.`,
-          status: 4,
-        },
-      });
-    }
 
     if (parameters.isRequired()) {
       // Need to clear latest results, to make sure we don't use results for different params.

redash/handlers/query_results.py

@@ -1,4 +1,3 @@
-import logging
 import time
 
 from flask import make_response, request
@@ -16,15 +15,15 @@
 from redash.serializers import serialize_query_result, serialize_query_result_to_csv, serialize_query_result_to_xlsx
 
 
-def error_response(message, http_status=400):
-    return {'job': {'status': 4, 'error': message}}, http_status
+def error_response(message, data=None, http_status=400):
+    return {'job': {'status': 4, 'error': message, 'error_data': data}}, http_status
 
 
 error_messages = {
-    'unsafe_when_shared': error_response('This query contains potentially unsafe parameters and cannot be executed on a shared dashboard or an embedded visualization.', 403),
-    'unsafe_on_view_only': error_response('This query contains potentially unsafe parameters and cannot be executed with read-only access to this data source.', 403),
-    'no_permission': error_response('You do not have permission to run queries with this data source.', 403),
-    'select_data_source': error_response('Please select data source to run this query.', 401)
+    'unsafe_when_shared': error_response('This query contains potentially unsafe parameters and cannot be executed on a shared dashboard or an embedded visualization.', None, 403),
+    'unsafe_on_view_only': error_response('This query contains potentially unsafe parameters and cannot be executed with read-only access to this data source.', None, 403),
+    'no_permission': error_response('You do not have permission to run queries with this data source.', None, 403),
+    'select_data_source': error_response('Please select data source to run this query.', None, 401)
 }
 
 
@@ -39,11 +38,15 @@ def run_query(query, parameters, data_source, query_id, max_age=0):
 
     try:
         query.apply(parameters)
-    except (InvalidParameterError, QueryDetachedFromDataSourceError) as e:
+    except QueryDetachedFromDataSourceError as e:
         abort(400, message=e.message)
+    except InvalidParameterError as e:
+        return error_response(e.message, {'parameters': e.parameter_errors})
 
-    if query.missing_params:
-        return error_response('Missing parameter value for: {}'.format(", ".join(query.missing_params)))
+    missing_params_error = query.missing_params_error
+    if missing_params_error:
+        message, parameter_errors = missing_params_error
+        return error_response(message, {'parameters': parameter_errors})
 
     if max_age == 0:
         query_result = None

redash/models/parameterized_query.py

@@ -3,7 +3,7 @@
 from numbers import Number
 from redash.utils import mustache_render, json_loads
 from redash.permissions import require_access, view_only
-from funcy import distinct
+from funcy import distinct, lpluck, compact
 from dateutil.parser import parse
 
 from six import string_types, text_type
@@ -107,6 +107,15 @@ def _is_date_range(obj):
         return False
 
 
+def _is_date_range_type(type):
+    return type in ["date-range", "datetime-range", "datetime-range-with-seconds"]
+
+
+def _is_tag_in_template(name, template):
+    tags = _collect_query_parameters(template)
+    return name in tags
+
+
 def _is_value_within_options(value, dropdown_options, allow_list=False):
     if isinstance(value, list):
         return allow_list and set(map(text_type, value)).issubset(set(dropdown_options))
@@ -122,23 +131,32 @@ def __init__(self, template, schema=None, org=None):
         self.parameters = {}
 
     def apply(self, parameters):
-        invalid_parameter_names = [key for (key, value) in parameters.items() if not self._valid(key, value)]
-        if invalid_parameter_names:
-            raise InvalidParameterError(invalid_parameter_names)
+        # filter out params not defined in schema
+        if self.schema:
+            names_with_definition = lpluck("name", self.schema)
+            parameters = {k: v for (k, v) in parameters.items() if k in names_with_definition}
+
+        invalid_parameters = compact({k: self._invalid_message(k, v) for (k, v) in parameters.items()})
+        if invalid_parameters:
+            raise InvalidParameterError(invalid_parameters)
         else:
             self.parameters.update(parameters)
             self.query = mustache_render(self.template, join_parameter_list_values(parameters, self.schema))
 
         return self
 
-    def _valid(self, name, value):
+    def _invalid_message(self, name, value):
+        if value is None:
+            return 'Required parameter'
+
+        # skip if no schema
         if not self.schema:
-            return True
+            return None
 
         definition = next((definition for definition in self.schema if definition["name"] == name), None)
 
         if not definition:
-            return False
+            return 'Parameter no longer exists in query.'
 
         enum_options = definition.get('enumOptions')
         query_id = definition.get('queryId')
@@ -147,7 +165,7 @@ def _valid(self, name, value):
         if isinstance(enum_options, string_types):
             enum_options = enum_options.split('\n')
 
-        validators = {
+        value_validators = {
             "text": lambda value: isinstance(value, string_types),
             "number": _is_number,
             "enum": lambda value: _is_value_within_options(value,
@@ -164,9 +182,32 @@ def _valid(self, name, value):
             "datetime-range-with-seconds": _is_date_range,
         }
 
-        validate = validators.get(definition["type"], lambda x: False)
+        validate_value = value_validators.get(definition["type"], lambda x: False)
+
+        if not validate_value(value):
+            return 'Invalid value'
+
+        tag_error_msg = self._validate_tag(name, definition["type"])
+        if tag_error_msg is not None:
+            return tag_error_msg
+
+        return None
+
+    def _validate_tag(self, name, type):
+        error_msg = '{{{{ {0} }}}} not found in query'
+        if _is_date_range_type(type):
+            start_tag = '{}.start'.format(name)
+            if not _is_tag_in_template(start_tag, self.template):
+                return error_msg.format(start_tag)
 
-        return validate(value)
+            end_tag = '{}.end'.format(name)
+            if not _is_tag_in_template(end_tag, self.template):
+                return error_msg.format(end_tag)
+
+        elif not _is_tag_in_template(name, self.template):
+            return error_msg.format(name)
+
+        return None
 
     @property
     def is_safe(self):
@@ -175,23 +216,46 @@ def is_safe(self):
 
     @property
     def missing_params(self):
-        query_parameters = set(_collect_query_parameters(self.template))
+        query_parameters = _collect_query_parameters(self.template)
         return set(query_parameters) - set(_parameter_names(self.parameters))
 
+    @property
+    def missing_params_error(self):
+        missing_params = self.missing_params
+        if not missing_params:
+            return None
+
+        parameter_names = ', '.join('"{}"'.format(name) for name in missing_params)
+        if len(missing_params) > 1:
+            message = 'Parameters {} are missing.'.format(parameter_names)
+        else:
+            message = 'Parameter {} is missing.'.format(parameter_names)
+
+        parameter_errors = {name: 'Missing parameter' for name in missing_params}
+        return message, parameter_errors
+
     @property
     def text(self):
         return self.query
 
 
 class InvalidParameterError(Exception):
-    def __init__(self, parameters):
-        parameter_names = ", ".join(parameters)
-        message = "The following parameter values are incompatible with their definitions: {}".format(parameter_names)
-        super(InvalidParameterError, self).__init__(message)
+    def __init__(self, parameter_errors):
+        parameter_names = ', '.join('"{}"'.format(name) for name in parameter_errors.keys())
+        if len(parameter_errors) > 1:
+            message = 'Parameters {} are invalid.'.format(parameter_names)
+        else:
+            message = 'Parameter {} is invalid.'.format(parameter_names)
+
+        self.message = message
+        self.parameter_errors = parameter_errors
+
+        super().__init__(message, parameter_errors)
 
 
 class QueryDetachedFromDataSourceError(Exception):
     def __init__(self, query_id):
         self.query_id = query_id
-        super(QueryDetachedFromDataSourceError, self).__init__(
-            "This query is detached from any data source. Please select a different query.")
+        self.message = "This query is detached from any data source. Please select a different query."
+
+        super().__init__(self.message)

tests/models/test_parameterized_query.py

@@ -45,12 +45,94 @@ def test_handles_objects(self):
         })
         self.assertEqual(set([]), query.missing_params)
 
-    def test_raises_on_parameters_not_in_schema(self):
+    def test_single_invalid_parameter_exception(self):
+        query = ParameterizedQuery("foo")
+        with pytest.raises(InvalidParameterError) as excinfo:
+            query.apply({"bar": None})
+
+        message, parameter_errors = excinfo.value.args
+        self.assertEquals(message, 'Parameter "bar" is invalid.')
+        self.assertEquals(len(parameter_errors), 1)
+
+    def test_multiple_invalid_parameter_exception(self):
+        query = ParameterizedQuery("foo")
+        with pytest.raises(InvalidParameterError) as excinfo:
+            query.apply({"bar": None, "baz": None})
+
+        message, parameter_errors = excinfo.value.args
+        self.assertEquals(message, 'Parameters "bar", "baz" are invalid.')
+        self.assertEquals(len(parameter_errors), 2)
+
+    def test_invalid_parameter_error_messages(self):
+        schema = [
+            {"name": "bar", "type": "text"},
+            {"name": "baz", "type": "text"},
+            {"name": "foo", "type": "text"},
+            {"name": "spam", "type": "date-range"},
+            {"name": "ham", "type": "date-range"},
+            {"name": "eggs", "type": "number"},
+        ]
+        parameters = {
+            "bar": None,
+            "baz": 7,
+            "foo": "text",
+            "spam": {"start": "2000-01-01 12:00:00", "end": "2000-12-31 12:00:00"},
+            "ham": {"start": "2000-01-01 12:00:00", "end": "2000-12-31 12:00:00"},
+            "eggs": 42,
+        }
+        query = ParameterizedQuery("foo {{ spam }} {{ ham.start}} {{ eggs.start }}", schema)
+        with pytest.raises(InvalidParameterError) as excinfo:
+            query.apply(parameters)
+
+        _, parameter_errors = excinfo.value.args
+        self.assertEquals(parameter_errors, {
+            "bar": "Required parameter",
+            "baz": "Invalid value",
+            "foo": "{{ foo }} not found in query",
+            "spam": "{{ spam.start }} not found in query",
+            "ham": "{{ ham.end }} not found in query",
+            "eggs": "{{ eggs }} not found in query",
+        })
+
+    def test_single_missing_parameter_error(self):
+        query = ParameterizedQuery("foo {{ bar }}")
+
+        message, parameter_errors = query.missing_params_error
+        self.assertEquals(message, 'Parameter "bar" is missing.')
+        self.assertEquals(len(parameter_errors), 1)
+
+    def test_multiple_missing_parameter_error(self):
+        query = ParameterizedQuery("foo {{ bar }} {{ baz }}")
+
+        message, parameter_errors = query.missing_params_error
+        self.assertEquals(message, 'Parameters "bar", "baz" are missing.')
+        self.assertEquals(len(parameter_errors), 2)
+
+    def test_missing_parameter_error_message(self):
+        query = ParameterizedQuery("foo {{ bar }}")
+
+        _, parameter_errors = query.missing_params_error
+        self.assertEquals(parameter_errors, { "bar": "Missing parameter" })
+
+    def test_ignores_parameters_not_in_schema(self):
+        schema = [{"name": "bar", "type": "text"}]
+        query = ParameterizedQuery("foo {{ bar }}", schema)
+
+        with pytest.raises(InvalidParameterError) as excinfo:
+            query.apply({"qux": 7, "bar": 7})
+
+        _, parameter_errors = excinfo.value.args
+        self.assertTrue('bar' in parameter_errors)
+        self.assertFalse('qux' in parameter_errors)
+
+    def test_passes_on_parameters_not_in_schema(self):
         schema = [{"name": "bar", "type": "text"}]
         query = ParameterizedQuery("foo", schema)
 
-        with pytest.raises(InvalidParameterError):
-            query.apply({"qux": 7})
+        try:
+            query.apply({"qux": None})
+        except InvalidParameterError:
+            pytest.fail("Unexpected InvalidParameterError")
 
     def test_raises_on_invalid_text_parameters(self):
         schema = [{"name": "bar", "type": "text"}]