Unify query based dropdown population (#3337)

* stop testing `collect_query_parameters`, it's an implementation detail

* add tests for `missing_query_params`

* rename SQLQuery -> ParameterizedSqlQuery

* rename sql_query.py to parameterized_query.py

* split to parameterized queries and parameterized SQL queries, where
parameterized queries only do templating and parameterized SQL queries
add tree validation on top of it

* move missing parameter detection to ParameterizedQuery

* get rid of some old code

* fix tests

* set syntax to `custom`

* revert the max-age-related refactoring

* 👋 tree validations 😢

* BaseQueryRunner is no longer a factory for ParameterizedQuery, for now

* add an endpoint for running a query by its id and (optional) parameters
without having to provide the query text

* adds parameter schema to ParameterizedQuery

* adds parameter schema validation (currently for strings)

* validate number parameters

* validate date parameters

* validate parameters on POST /api/queries/<id>/results

* validate enum parameters

* validate date range parameters

* validate query-based dropdowns by preprocessing them at the handler
level and converting them to a populated enum

* change _is_date_range to be a tad more succinct

* a single assignment with a `map` is sufficiently explanatory

* Update redash/utils/parameterized_query.py

Co-Authored-By: rauchy <[email protected]>

* Update redash/utils/parameterized_query.py

Co-Authored-By: rauchy <[email protected]>

* Update redash/utils/parameterized_query.py

Co-Authored-By: rauchy <[email protected]>

* Update redash/utils/parameterized_query.py

Co-Authored-By: rauchy <[email protected]>

* Update redash/handlers/query_results.py

Co-Authored-By: rauchy <[email protected]>

* Update redash/utils/parameterized_query.py

Co-Authored-By: rauchy <[email protected]>

* build error message inside the error

* support all types of numbers as number parameters

* check for permissions when populating query-based dropdowns

* check for access to query before running it

* check for empty rows when populating query-based enums

* don't bother loading query results if user doesn't have access

* 💥 on unexpected parameter types

* parameter schema default is a list, not a dictionary

* fix a totally unrelated typo

* remove redundant null guards

* introduce /dropdown.json endpoint with dummy data

* wire frontend to /dropdown.json

* always return name/value combos from /dropdown.json

* load actual data into /dropdown.json

* pluck correct values for `name` and `value`

* reuse dropdwon plucking logic in QueryResultResource

* simplify _get_dropdown_values

* when doing parameter validation, we only care about the value and not
the display name

* rename dropdown to dropdownOptions

* move dropdown_values to utils/parameterized_query.py

* stop converting queries to enums and encapsulate the work inside
ParameterizedQuery (almost - /dropdown.json would still access the
dropdown_values method)

* re-order arguments by importance

* test query parameter validation

* tests for dropdown_values logic

* remove `.json` suffix to the dropdown endpoint

* allow `BaseResource` to handle JSON stuff

* move _pluck_name_and_value outside its containing method

* case-insensitive lookup when plucking name and value

* separate concerns and simplify test isolation for `dropdown_values`

* pick the default column according to the order specified in the query
result columns attribute

* use `current_org` instead of passing `org`

* test that user has access to the query when calling the /dropdown
endpoint

client/app/components/QueryBasedParameterInput.jsx

@@ -7,43 +7,6 @@ import { Query } from '@/services/query';
 
 const { Option } = Select;
 
-function optionsFromQueryResult(queryResult) {
-  const columns = queryResult.data.columns;
-  const numColumns = columns.length;
-  let options = [];
-  // If there are multiple columns, check if there is a column
-  // named 'name' and column named 'value'. If name column is present
-  // in results, use name from name column. Similar for value column.
-  // Default: Use first string column for name and value.
-  if (numColumns > 0) {
-    let nameColumn = null;
-    let valueColumn = null;
-    columns.forEach((column) => {
-      const columnName = column.name.toLowerCase();
-      if (columnName === 'name') {
-        nameColumn = column.name;
-      }
-      if (columnName === 'value') {
-        valueColumn = column.name;
-      }
-      // Assign first string column as name and value column.
-      if (nameColumn === null) {
-        nameColumn = column.name;
-      }
-      if (valueColumn === null) {
-        valueColumn = column.name;
-      }
-    });
-    if (nameColumn !== null && valueColumn !== null) {
-      options = queryResult.data.rows.map(row => ({
-        name: row[nameColumn],
-        value: row[valueColumn],
-      }));
-    }
-  }
-  return options;
-}
-
 export class QueryBasedParameterInput extends React.Component {
   static propTypes = {
     value: PropTypes.any, // eslint-disable-line react/forbid-prop-types
@@ -81,9 +44,8 @@ export class QueryBasedParameterInput extends React.Component {
   _loadOptions(queryId) {
     if (queryId && (queryId !== this.state.queryId)) {
       this.setState({ loading: true });
-      Query.resultById({ id: queryId }, (result) => {
+      Query.dropdownOptions({ id: queryId }, (options) => {
         if (this.props.queryId === queryId) {
-          const options = optionsFromQueryResult(result.query_result);
           this.setState({ options, loading: false });
 
           const found = find(options, option => option.value === this.props.value) !== undefined;

client/app/services/query.js

@@ -366,6 +366,11 @@ function QueryResource(
         isArray: false,
         url: 'api/queries/:id/results.json',
       },
+      dropdownOptions: {
+        method: 'get',
+        isArray: true,
+        url: 'api/queries/:id/dropdown',
+      },
       favorites: {
         method: 'get',
         isArray: false,

redash/handlers/api.py

@@ -10,7 +10,7 @@
 from redash.handlers.data_sources import DataSourceTypeListResource, DataSourceListResource, DataSourceSchemaResource, DataSourceResource, DataSourcePauseResource, DataSourceTestResource
 from redash.handlers.events import EventsResource
 from redash.handlers.queries import QueryArchiveResource, QueryForkResource, QueryRefreshResource, QueryListResource, QueryRecentResource, QuerySearchResource, QueryResource, MyQueriesResource
-from redash.handlers.query_results import QueryResultListResource, QueryResultResource, JobResource
+from redash.handlers.query_results import QueryResultListResource, QueryResultDropdownResource, QueryResultResource, JobResource
 from redash.handlers.users import UserResource, UserListResource, UserInviteResource, UserResetPasswordResource, UserDisableResource, UserRegenerateApiKeyResource
 from redash.handlers.visualizations import VisualizationListResource
 from redash.handlers.visualizations import VisualizationResource
@@ -69,10 +69,10 @@ def json_representation(data, code, headers=None):
 
 api.add_org_resource(EventsResource, '/api/events', endpoint='events')
 
-api.add_org_resource(QueryFavoriteListResource, '/api/queries/favorites', endpoint='query_fovorites')
-api.add_org_resource(QueryFavoriteResource, '/api/queries/<query_id>/favorite', endpoint='query_fovorite')
-api.add_org_resource(DashboardFavoriteListResource, '/api/dashboards/favorites', endpoint='dashboard_fovorites')
-api.add_org_resource(DashboardFavoriteResource, '/api/dashboards/<object_id>/favorite', endpoint='dashboard_fovorite')
+api.add_org_resource(QueryFavoriteListResource, '/api/queries/favorites', endpoint='query_favorites')
+api.add_org_resource(QueryFavoriteResource, '/api/queries/<query_id>/favorite', endpoint='query_favorite')
+api.add_org_resource(DashboardFavoriteListResource, '/api/dashboards/favorites', endpoint='dashboard_favorites')
+api.add_org_resource(DashboardFavoriteResource, '/api/dashboards/<object_id>/favorite', endpoint='dashboard_favorite')
 
 api.add_org_resource(QueryTagsResource, '/api/queries/tags', endpoint='query_tags')
 api.add_org_resource(DashboardTagsResource, '/api/dashboards/tags', endpoint='dashboard_tags')
@@ -90,6 +90,7 @@ def json_representation(data, code, headers=None):
 api.add_org_resource(CheckPermissionResource, '/api/<object_type>/<object_id>/acl/<access_type>', endpoint='check_permissions')
 
 api.add_org_resource(QueryResultListResource, '/api/query_results', endpoint='query_results')
+api.add_org_resource(QueryResultDropdownResource, '/api/queries/<query_id>/dropdown', endpoint='query_result_dropdown')
 api.add_org_resource(QueryResultResource,
                      '/api/query_results/<query_result_id>.<filetype>',
                      '/api/query_results/<query_result_id>',

redash/handlers/query_results.py

@@ -10,8 +10,8 @@
                                 require_permission, view_only)
 from redash.tasks import QueryTask
 from redash.tasks.queries import enqueue_query
-from redash.utils import (collect_parameters_from_request, gen_query_hash, json_dumps, json_loads, utcnow)
-from redash.utils.parameterized_query import ParameterizedQuery
+from redash.utils import (collect_parameters_from_request, gen_query_hash, json_dumps, utcnow)
+from redash.utils.parameterized_query import ParameterizedQuery, dropdown_values
 
 
 def error_response(message):
@@ -131,6 +131,11 @@ def post(self):
 ONE_YEAR = 60 * 60 * 24 * 365.25
 
 
+class QueryResultDropdownResource(BaseResource):
+    def get(self, query_id):
+        return dropdown_values(query_id)
+
+
 class QueryResultResource(BaseResource):
     @staticmethod
     def add_cors_headers(headers):
@@ -188,8 +193,7 @@ def post(self, query_id):
         max_age = int(params.get('max_age', 0))
 
         query = get_object_or_404(models.Query.get_by_id_and_org, query_id, self.current_org)
-        parameter_schema = map(self._convert_queries_to_enums,
-                               query.options.get("parameters", []))
+        parameter_schema = query.options.get("parameters", [])
 
         if not has_access(query.data_source.groups, self.current_user, not_view_only):
             return {'job': {'status': 4, 'error': 'You do not have permission to run queries with this data source.'}}, 403

redash/utils/parameterized_query.py

@@ -1,9 +1,38 @@
 import pystache
+from functools import partial
+from flask_login import current_user
+from redash.authentication.org_resolving import current_org
 from numbers import Number
-from redash.utils import mustache_render
+from redash import models
+from redash.utils import mustache_render, json_loads
+from redash.permissions import require_access, view_only
 from funcy import distinct
 from dateutil.parser import parse
 
+
+def _pluck_name_and_value(default_column, row):
+    row = {k.lower(): v for k, v in row.items()}
+    name_column = "name" if "name" in row.keys() else default_column
+    value_column = "value" if "value" in row.keys() else default_column
+
+    return {"name": row[name_column], "value": row[value_column]}
+
+
+def _load_result(query_id):
+    query = models.Query.get_by_id_and_org(query_id, current_org)
+    require_access(query.data_source.groups, current_user, view_only)
+    query_result = models.QueryResult.get_by_id_and_org(query.latest_query_data_id, current_org)
+
+    return json_loads(query_result.data)
+
+
+def dropdown_values(query_id):
+    data = _load_result(query_id)
+    first_column = data["columns"][0]["name"]
+    pluck = partial(_pluck_name_and_value, first_column)
+    return map(pluck, data["rows"])
+
+
 def _collect_key_names(nodes):
     keys = []
     for node in nodes._parse_tree:
@@ -76,6 +105,7 @@ def _valid(self, name, value):
             "text": lambda value: isinstance(value, basestring),
             "number": lambda value: isinstance(value, Number),
             "enum": lambda value: value in definition["enumOptions"],
+            "query": lambda value: value in [v["value"] for v in dropdown_values(definition["queryId"])],
             "date": _is_date,
             "datetime-local": _is_date,
             "datetime-with-seconds": _is_date,

tests/handlers/test_query_results.py

@@ -170,6 +170,16 @@ def test_signed_in_user_and_different_query_result(self):
         self.assertEquals(rv.status_code, 403)
 
 
+class TestQueryResultDropdownResource(BaseTestCase):
+    def test_checks_for_access_to_the_query(self):
+        ds2 = self.factory.create_data_source(group=self.factory.org.admin_group, view_only=False)
+        query = self.factory.create_query(data_source=ds2)
+
+        rv = self.make_request('get', '/api/queries/{}/dropdown'.format(query.id))
+
+        self.assertEquals(rv.status_code, 403)
+
+
 class TestQueryResultExcelResponse(BaseTestCase):
     def test_renders_excel_file(self):
         query = self.factory.create_query()

tests/utils/test_parameterized_query.py

@@ -1,7 +1,9 @@
 from unittest import TestCase
+from mock import patch
+from collections import namedtuple
 import pytest
 
-from redash.utils.parameterized_query import ParameterizedQuery, InvalidParameterError
+from redash.utils.parameterized_query import ParameterizedQuery, InvalidParameterError, dropdown_values
 
 
 class TestParameterizedQuery(TestCase):
@@ -110,6 +112,31 @@ def test_validates_enum_parameters(self):
 
         self.assertEquals("foo baz", query.text)
 
+    @patch('redash.utils.parameterized_query.dropdown_values')
+    def test_raises_on_invalid_query_parameters(self, _):
+        schema = [{"name": "bar", "type": "query", "queryId": 1}]
+        query = ParameterizedQuery("foo", schema)
+
+        with pytest.raises(InvalidParameterError):
+            query.apply({"bar": 7})
+
+    @patch('redash.utils.parameterized_query.dropdown_values', return_value=[{"value": "baz"}])
+    def test_raises_on_unlisted_query_value_parameters(self, _):
+        schema = [{"name": "bar", "type": "query", "queryId": 1}]
+        query = ParameterizedQuery("foo", schema)
+
+        with pytest.raises(InvalidParameterError):
+            query.apply({"bar": "shlomo"})
+
+    @patch('redash.utils.parameterized_query.dropdown_values', return_value=[{"value": "baz"}])
+    def test_validates_query_parameters(self, _):
+        schema = [{"name": "bar", "type": "query", "queryId": 1}]
+        query = ParameterizedQuery("foo {{bar}}", schema)
+
+        query.apply({"bar": "baz"})
+
+        self.assertEquals("foo baz", query.text)
+
     def test_raises_on_invalid_date_range_parameters(self):
         schema = [{"name": "bar", "type": "date-range"}]
         query = ParameterizedQuery("foo", schema)
@@ -131,3 +158,17 @@ def test_raises_on_unexpected_param_types(self):
 
         with pytest.raises(InvalidParameterError):
             query.apply({"bar": "baz"})
+
+    @patch('redash.utils.parameterized_query._load_result', return_value={
+        "columns": [{"name": "id"}, {"name": "Name"}, {"name": "Value"}],
+        "rows": [{"id": 5, "Name": "John", "Value": "John Doe"}]})
+    def test_dropdown_values_prefers_name_and_value_columns(self, _):
+        values = dropdown_values(1)
+        self.assertEquals(values, [{"name": "John", "value": "John Doe"}])
+
+    @patch('redash.utils.parameterized_query._load_result', return_value={
+        "columns": [{"name": "id"}, {"name": "fish"}, {"name": "poultry"}],
+        "rows": [{"fish": "Clown", "id": 5, "poultry": "Hen"}]})
+    def test_dropdown_values_compromises_for_first_column(self, _):
+        values = dropdown_values(1)
+        self.assertEquals(values, [{"name": 5, "value": 5}])