From 7f89f12c4a80f95282c0c5b4807b20dbd688de43 Mon Sep 17 00:00:00 2001 From: John Bodley <4567245+john-bodley@users.noreply.github.com> Date: Wed, 29 Apr 2020 12:16:47 -0700 Subject: [PATCH] [debug] Debugging caching issue (#9665) Co-authored-by: John Bodley --- superset/jinja_context.py | 23 ++++--- superset/views/core.py | 4 ++ superset/views/utils.py | 21 ++++--- tests/jinja_context_tests.py | 119 +++++++++++++++++++---------------- tests/macro_tests.py | 77 ----------------------- tests/utils_tests.py | 85 ++++++++++++++++++++++++- 6 files changed, 174 insertions(+), 155 deletions(-) delete mode 100644 tests/macro_tests.py diff --git a/superset/jinja_context.py b/superset/jinja_context.py index 5438f0f3b5..28eb6e2ed9 100644 --- a/superset/jinja_context.py +++ b/superset/jinja_context.py @@ -16,7 +16,6 @@ # under the License. """Defines the templating context for SQL Lab""" import inspect -import json import re from typing import Any, List, Optional, Tuple @@ -49,7 +48,9 @@ def filter_values(column: str, default: Optional[str] = None) -> List[str]: :return: returns a list of filter values """ - form_data = json.loads(request.form.get("form_data", "{}")) + from superset.views.utils import get_form_data + + form_data, _ = get_form_data() convert_legacy_filters_into_adhoc(form_data) merge_extra_filters(form_data) @@ -168,18 +169,16 @@ class ExtraCache: :returns: The URL parameters """ + from superset.views.utils import get_form_data + if request.args.get(param): return request.args.get(param, default) - # Supporting POST as well as get - form_data = request.form.get("form_data") - if isinstance(form_data, str): - form_data = json.loads(form_data) - url_params = form_data.get("url_params") or {} - result = url_params.get(param, default) - if add_to_cache_keys: - self.cache_key_wrapper(result) - return result - return default + form_data, _ = get_form_data() + url_params = form_data.get("url_params") or {} + result = url_params.get(param, default) + if add_to_cache_keys: + self.cache_key_wrapper(result) + return result class BaseTemplateProcessor: # pylint: disable=too-few-public-methods diff --git a/superset/views/core.py b/superset/views/core.py index e6c9ac9769..9688b89b9d 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -1707,13 +1707,17 @@ class Superset(BaseSupersetView): form_data["extra_filters"] = get_dashboard_extra_filters( slc.id, dashboard_id ) + obj = get_viz( datasource_type=slc.datasource.type, datasource_id=slc.datasource.id, form_data=form_data, force=True, ) + + g.form_data = form_data payload = obj.get_payload() + delattr(g, "form_data") error = payload["error"] status = payload["status"] except Exception as ex: diff --git a/superset/views/utils.py b/superset/views/utils.py index edb2987ab4..5ed7e48383 100644 --- a/superset/views/utils.py +++ b/superset/views/utils.py @@ -20,7 +20,7 @@ from typing import Any, Dict, List, Optional, Tuple from urllib import parse import simplejson as json -from flask import request +from flask import g, request import superset.models.core as models from superset import app, db, is_feature_enabled @@ -99,23 +99,28 @@ def get_viz( return viz_obj -def get_form_data(slice_id=None, use_slice_data=False): +def get_form_data( + slice_id: Optional[int] = None, use_slice_data: bool = False +) -> Tuple[Dict[str, Any], Optional[Slice]]: form_data = {} - post_data = request.form.get("form_data") + request_form_data = request.form.get("form_data") request_args_data = request.args.get("form_data") - # Supporting POST - if post_data: - form_data.update(json.loads(post_data)) - # request params can overwrite post body + if request_form_data: + form_data.update(json.loads(request_form_data)) + # request params can overwrite the body if request_args_data: form_data.update(json.loads(request_args_data)) + # Fallback to using the Flask globals (used for cache warmup) if defined. + if not form_data and hasattr(g, "form_data"): + form_data = getattr(g, "form_data") + url_id = request.args.get("r") if url_id: saved_url = db.session.query(models.Url).filter_by(id=url_id).first() if saved_url: url_str = parse.unquote_plus( - saved_url.url.split("?")[1][10:], encoding="utf-8", errors=None + saved_url.url.split("?")[1][10:], encoding="utf-8" ) url_form_data = json.loads(url_str) # allow form_date in request override saved url diff --git a/tests/jinja_context_tests.py b/tests/jinja_context_tests.py index 91cb5e4761..431d3b67f2 100644 --- a/tests/jinja_context_tests.py +++ b/tests/jinja_context_tests.py @@ -15,76 +15,85 @@ # specific language governing permissions and limitations # under the License. import json -from unittest import mock -from superset.jinja_context import filter_values +import tests.test_app +from superset import app +from superset.jinja_context import ExtraCache, filter_values from tests.base_tests import SupersetTestCase class Jinja2ContextTests(SupersetTestCase): def test_filter_values_default(self) -> None: - request = mock.MagicMock() - request.form = {} - - with mock.patch("superset.jinja_context.request", request): + with app.test_request_context(): self.assertEquals(filter_values("name", "foo"), ["foo"]) def test_filter_values_no_default(self) -> None: - request = mock.MagicMock() - request.form = {} - - with mock.patch("superset.jinja_context.request", request): + with app.test_request_context(): self.assertEquals(filter_values("name"), []) def test_filter_values_adhoc_filters(self) -> None: - request = mock.MagicMock() - - request.form = { - "form_data": json.dumps( - { - "adhoc_filters": [ - { - "clause": "WHERE", - "comparator": "foo", - "expressionType": "SIMPLE", - "operator": "in", - "subject": "name", - } - ], - } - ) - } - - with mock.patch("superset.jinja_context.request", request): + with app.test_request_context( + data={ + "form_data": json.dumps( + { + "adhoc_filters": [ + { + "clause": "WHERE", + "comparator": "foo", + "expressionType": "SIMPLE", + "operator": "in", + "subject": "name", + } + ], + } + ) + } + ): self.assertEquals(filter_values("name"), ["foo"]) - request.form = { - "form_data": json.dumps( - { - "adhoc_filters": [ - { - "clause": "WHERE", - "comparator": ["foo", "bar"], - "expressionType": "SIMPLE", - "operator": "in", - "subject": "name", - } - ], - } - ) - } - - with mock.patch("superset.jinja_context.request", request): + with app.test_request_context( + data={ + "form_data": json.dumps( + { + "adhoc_filters": [ + { + "clause": "WHERE", + "comparator": ["foo", "bar"], + "expressionType": "SIMPLE", + "operator": "in", + "subject": "name", + } + ], + } + ) + } + ): self.assertEquals(filter_values("name"), ["foo", "bar"]) def test_filter_values_extra_filters(self) -> None: - request = mock.MagicMock() - - request.form = { - "form_data": json.dumps( - {"extra_filters": [{"col": "name", "op": "in", "val": "foo"}]} - ) - } - - with mock.patch("superset.jinja_context.request", request): + with app.test_request_context( + data={ + "form_data": json.dumps( + {"extra_filters": [{"col": "name", "op": "in", "val": "foo"}]} + ) + } + ): self.assertEquals(filter_values("name"), ["foo"]) + + def test_url_param_default(self) -> None: + with app.test_request_context(): + self.assertEquals(ExtraCache().url_param("foo", "bar"), "bar") + + def test_url_param_no_default(self) -> None: + with app.test_request_context(): + self.assertEquals(ExtraCache().url_param("foo"), None) + + def test_url_param_query(self) -> None: + with app.test_request_context(query_string={"foo": "bar"}): + self.assertEquals(ExtraCache().url_param("foo"), "bar") + + def test_url_param_form_data(self) -> None: + with app.test_request_context( + query_string={"form_data": json.dumps({"url_params": {"foo": "bar"}})} + ): + self.assertEquals(ExtraCache().url_param("foo"), "bar") diff --git a/tests/macro_tests.py b/tests/macro_tests.py deleted file mode 100644 index 1b9b66ec30..0000000000 --- a/tests/macro_tests.py +++ /dev/null @@ -1,77 +0,0 @@ -# Licensed to the Apache Software Foundation (ASF) under one -# or more contributor license agreements. See the NOTICE file -# distributed with this work for additional information -# regarding copyright ownership. The ASF licenses this file -# to you under the Apache License, Version 2.0 (the -# "License"); you may not use this file except in compliance -# with the License. You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, -# software distributed under the License is distributed on an -# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -# KIND, either express or implied. See the License for the -# specific language governing permissions and limitations -# under the License. -from flask import json - -from superset import app, jinja_context -from tests.base_tests import SupersetTestCase - - -class MacroTestCase(SupersetTestCase): - def test_filter_values_macro(self): - form_data1 = { - "extra_filters": [{"col": "my_special_filter", "op": "in", "val": ["foo"]}], - "filters": [{"col": "my_special_filter2", "op": "in", "val": ["bar"]}], - } - - form_data2 = { - "extra_filters": [ - {"col": "my_special_filter", "op": "in", "val": ["foo", "bar"]} - ] - } - - form_data3 = { - "extra_filters": [ - {"col": "my_special_filter", "op": "in", "val": ["foo", "bar"]} - ], - "filters": [{"col": "my_special_filter", "op": "in", "val": ["savage"]}], - } - - form_data4 = { - "extra_filters": [{"col": "my_special_filter", "op": "in", "val": "foo"}], - "filters": [{"col": "my_special_filter", "op": "in", "val": "savage"}], - } - - data1 = {"form_data": json.dumps(form_data1)} - data2 = {"form_data": json.dumps(form_data2)} - data3 = {"form_data": json.dumps(form_data3)} - data4 = {"form_data": json.dumps(form_data4)} - - with app.test_request_context(data=data1): - filter_values = jinja_context.filter_values("my_special_filter") - self.assertEqual(filter_values, ["foo"]) - - filter_values = jinja_context.filter_values("my_special_filter2") - self.assertEqual(filter_values, ["bar"]) - - filter_values = jinja_context.filter_values("") - self.assertEqual(filter_values, []) - - with app.test_request_context(data=data2): - filter_values = jinja_context.filter_values("my_special_filter") - self.assertEqual(filter_values, ["foo", "bar"]) - - with app.test_request_context(data=data3): - filter_values = jinja_context.filter_values("my_special_filter") - self.assertEqual(filter_values, ["savage", "foo", "bar"]) - - with app.test_request_context(): - filter_values = jinja_context.filter_values("nonexistent_filter", "foo") - self.assertEqual(filter_values, ["foo"]) - - with app.test_request_context(data=data4): - filter_values = jinja_context.filter_values("my_special_filter") - self.assertEqual(filter_values, ["savage", "foo"]) diff --git a/tests/utils_tests.py b/tests/utils_tests.py index 70b77afeac..9f3d0f95fe 100644 --- a/tests/utils_tests.py +++ b/tests/utils_tests.py @@ -20,11 +20,12 @@ import uuid from datetime import date, datetime, time, timedelta from decimal import Decimal import hashlib +import json import os from unittest.mock import Mock, patch import numpy -from flask import Flask +from flask import Flask, g from flask_caching import Cache from sqlalchemy.exc import ArgumentError @@ -60,8 +61,11 @@ from superset.utils.core import ( zlib_compress, zlib_decompress, ) -from superset.views.utils import get_time_range_endpoints -from superset.views.utils import build_extra_filters +from superset.views.utils import ( + build_extra_filters, + get_form_data, + get_time_range_endpoints, +) from tests.base_tests import SupersetTestCase from .fixtures.certificates import ssl_certificate @@ -1249,3 +1253,78 @@ class UtilsTestCase(SupersetTestCase): get_email_address_list(",a@a; b@b c@c a-c@c; d@d, f@f"), ["a@a", "b@b", "c@c", "a-c@c", "d@d", "f@f"], ) + + def test_get_form_data_default(self) -> None: + with app.test_request_context(): + form_data, slc = get_form_data() + + self.assertEqual( + form_data, + {"time_range_endpoints": get_time_range_endpoints(form_data={}),}, + ) + + self.assertEqual(slc, None) + + def test_get_form_data_request_args(self) -> None: + with app.test_request_context( + query_string={"form_data": json.dumps({"foo": "bar"})} + ): + form_data, slc = get_form_data() + + self.assertEqual( + form_data, + { + "foo": "bar", + "time_range_endpoints": get_time_range_endpoints(form_data={}), + }, + ) + + self.assertEqual(slc, None) + + def test_get_form_data_request_form(self) -> None: + with app.test_request_context(data={"form_data": json.dumps({"foo": "bar"})}): + form_data, slc = get_form_data() + + self.assertEqual( + form_data, + { + "foo": "bar", + "time_range_endpoints": get_time_range_endpoints(form_data={}), + }, + ) + + self.assertEqual(slc, None) + + def test_get_form_data_request_args_and_form(self) -> None: + with app.test_request_context( + data={"form_data": json.dumps({"foo": "bar"})}, + query_string={"form_data": json.dumps({"baz": "bar"})}, + ): + form_data, slc = get_form_data() + + self.assertEqual( + form_data, + { + "baz": "bar", + "foo": "bar", + "time_range_endpoints": get_time_range_endpoints(form_data={}), + }, + ) + + self.assertEqual(slc, None) + + def test_get_form_data_globals(self) -> None: + with app.test_request_context(): + g.form_data = {"foo": "bar"} + form_data, slc = get_form_data() + delattr(g, "form_data") + + self.assertEqual( + form_data, + { + "foo": "bar", + "time_range_endpoints": get_time_range_endpoints(form_data={}), + }, + ) + + self.assertEqual(slc, None)