From a67bafd247edc3db0628dad64ec9e67641f3a33b Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Fri, 9 Sep 2022 11:06:31 +0300 Subject: [PATCH 1/4] fix(cache): respect default cache timeout on v1 chart data requests --- superset/charts/schemas.py | 8 ++- superset/common/query_context_processor.py | 6 ++ tests/common/query_context_generator.py | 4 ++ .../charts/data/api_tests.py | 60 +++++++++++++++++++ .../fixtures/query_context.py | 6 ++ 5 files changed, 83 insertions(+), 1 deletion(-) diff --git a/superset/charts/schemas.py b/superset/charts/schemas.py index c06768d127b55..eca2fe6101e53 100644 --- a/superset/charts/schemas.py +++ b/superset/charts/schemas.py @@ -1194,6 +1194,12 @@ class ChartDataQueryContextSchema(Schema): query_context_factory: Optional[QueryContextFactory] = None datasource = fields.Nested(ChartDataDatasourceSchema) queries = fields.List(fields.Nested(ChartDataQueryObjectSchema)) + custom_cache_timeout = fields.Integer( + description="Override the default cache timeout", + required=False, + allow_none=True, + ) + force = fields.Boolean( description="Should the queries be forced to load from the source. " "Default: `false`", @@ -1255,7 +1261,7 @@ class ChartDataResponseResult(Schema): ) cache_timeout = fields.Integer( description="Cache timeout in following order: custom timeout, datasource " - "timeout, default config timeout.", + "timeout, default config timeout, cache config default timeout.", required=True, allow_none=True, ) diff --git a/superset/common/query_context_processor.py b/superset/common/query_context_processor.py index b253caa6b980d..ae3202981b906 100644 --- a/superset/common/query_context_processor.py +++ b/superset/common/query_context_processor.py @@ -434,6 +434,12 @@ def get_cache_timeout(self) -> int: cache_timeout_rv = self._query_context.get_cache_timeout() if cache_timeout_rv: return cache_timeout_rv + if ( + data_cache_timeout := config["DATA_CACHE_CONFIG"].get( + "CACHE_DEFAULT_TIMEOUT" + ) + ) is not None: + return data_cache_timeout return config["CACHE_DEFAULT_TIMEOUT"] def cache_key(self, **extra: Any) -> str: diff --git a/tests/common/query_context_generator.py b/tests/common/query_context_generator.py index a22646983bdda..bd13feff067fd 100644 --- a/tests/common/query_context_generator.py +++ b/tests/common/query_context_generator.py @@ -248,6 +248,8 @@ class QueryContextGenerator: def generate( self, query_name: str, + force: bool = False, + custom_cache_timeout: Optional[int] = None, add_postprocessing_operations: bool = False, add_time_offsets: bool = False, table_id=1, @@ -259,6 +261,8 @@ def generate( table = self.get_table(table_name, table_id, table_type) return { "datasource": {"id": table.id, "type": table.type}, + "force": force, + "custom_cache_timeout": custom_cache_timeout, "queries": [ get_query_object( query_name, diff --git a/tests/integration_tests/charts/data/api_tests.py b/tests/integration_tests/charts/data/api_tests.py index 67b9826d26f39..6aab4d3df75a9 100644 --- a/tests/integration_tests/charts/data/api_tests.py +++ b/tests/integration_tests/charts/data/api_tests.py @@ -210,6 +210,66 @@ def test_with_row_limit_as_samples__rowcount_as_row_limit(self): self.assert_row_count(rv, expected_row_count) assert "GROUP BY" not in rv.json["result"][0]["query"] + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + @mock.patch( + "superset.common.query_context_processor.config", + { + **app.config, + "CACHE_DEFAULT_TIMEOUT": 1234, + "DATA_CACHE_CONFIG": { + **app.config["DATA_CACHE_CONFIG"], + "CACHE_DEFAULT_TIMEOUT": None, + }, + }, + ) + def test_cache_default_timeout(self): + query_context = get_query_context("birth_names", force=True) + rv = self.post_assert_metric( + CHART_DATA_URI, + query_context, + "data", + ) + data = json.loads(rv.data.decode("utf-8")) + assert data["result"][0]["cache_timeout"] == 1234 + + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + @mock.patch( + "superset.common.query_context_processor.config", + { + **app.config, + "CACHE_DEFAULT_TIMEOUT": 100000, + "DATA_CACHE_CONFIG": { + **app.config["DATA_CACHE_CONFIG"], + "CACHE_DEFAULT_TIMEOUT": 3456, + }, + }, + ) + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + def test_data_cache_default_timeout(self): + query_context = get_query_context("birth_names", force=True) + rv = self.post_assert_metric( + CHART_DATA_URI, + query_context, + "data", + ) + data = json.loads(rv.data.decode("utf-8")) + assert data["result"][0]["cache_timeout"] == 3456 + + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + def test_custom_cache_timeout(self): + query_context = get_query_context( + "birth_names", + force=True, + custom_cache_timeout=5678, + ) + rv = self.post_assert_metric( + CHART_DATA_URI, + query_context, + "data", + ) + data = json.loads(rv.data.decode("utf-8")) + assert data["result"][0]["cache_timeout"] == 5678 + def test_with_incorrect_result_type__400(self): self.query_context_payload["result_type"] = "qwerty" rv = self.post_assert_metric(CHART_DATA_URI, self.query_context_payload, "data") diff --git a/tests/integration_tests/fixtures/query_context.py b/tests/integration_tests/fixtures/query_context.py index 00a3036e01c25..2c6a8e04f421c 100644 --- a/tests/integration_tests/fixtures/query_context.py +++ b/tests/integration_tests/fixtures/query_context.py @@ -27,6 +27,8 @@ def get_table(self, name, id_, type_): def get_query_context( query_name: str, + force: bool = False, + custom_cache_timeout: Optional[int] = None, add_postprocessing_operations: bool = False, add_time_offsets: bool = False, form_data: Optional[Dict[str, Any]] = None, @@ -37,6 +39,8 @@ def get_query_context( generated by the "Boy Name Cloud" chart in the examples. :param query_name: name of an example query, which is always in the format of `datasource_name[:test_case_name]`, where `:test_case_name` is optional. + :param force: force cache refresh + :param custom_cache_timeout: Custom cache timeout :param add_postprocessing_operations: Add post-processing operations to QueryObject :param add_time_offsets: Add time offsets to QueryObject(advanced analytics) :param form_data: chart metadata @@ -44,6 +48,8 @@ def get_query_context( """ return QueryContextGeneratorInteg().generate( query_name=query_name, + force=force, + custom_cache_timeout=custom_cache_timeout, add_postprocessing_operations=add_postprocessing_operations, add_time_offsets=add_time_offsets, form_data=form_data, From 551d7772bf0010ad3338ce50bcadb3c5f8e4e8e7 Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Tue, 13 Sep 2022 09:53:27 +0300 Subject: [PATCH 2/4] disable unit tests in integration test flow --- .github/workflows/superset-python-unittest.yml | 2 +- scripts/python_tests.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/superset-python-unittest.yml b/.github/workflows/superset-python-unittest.yml index 64db4d3e649af..1ff07375d4727 100644 --- a/.github/workflows/superset-python-unittest.yml +++ b/.github/workflows/superset-python-unittest.yml @@ -37,7 +37,7 @@ jobs: python-version: ${{ matrix.python-version }} cache: 'pip' cache-dependency-path: 'requirements/testing.txt' -# TODO: separated requiermentes.txt file just for unit tests +# TODO: separated requirements.txt file just for unit tests - name: Install dependencies if: steps.check.outcome == 'failure' uses: ./.github/actions/cached-dependencies diff --git a/scripts/python_tests.sh b/scripts/python_tests.sh index 0554f8ca65ad9..6491a3f6f9d46 100755 --- a/scripts/python_tests.sh +++ b/scripts/python_tests.sh @@ -32,4 +32,4 @@ superset init echo "Running tests" -pytest --durations-min=2 --maxfail=1 --cov-report= --cov=superset "$@" +pytest --durations-min=2 --maxfail=1 --cov-report= --cov=superset ./tests/integration_tests "$@" From e03c4185ff49fc41629a1f4a35bd4a8f6764f301 Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Tue, 13 Sep 2022 12:44:42 +0300 Subject: [PATCH 3/4] fix tests --- superset/charts/schemas.py | 2 +- tests/common/query_context_generator.py | 4 - .../charts/data/api_tests.py | 126 +++++++++--------- .../fixtures/query_context.py | 6 - 4 files changed, 66 insertions(+), 72 deletions(-) diff --git a/superset/charts/schemas.py b/superset/charts/schemas.py index eca2fe6101e53..ffd64e8bb023a 100644 --- a/superset/charts/schemas.py +++ b/superset/charts/schemas.py @@ -1261,7 +1261,7 @@ class ChartDataResponseResult(Schema): ) cache_timeout = fields.Integer( description="Cache timeout in following order: custom timeout, datasource " - "timeout, default config timeout, cache config default timeout.", + "timeout, cache default timeout, config default cache timeout.", required=True, allow_none=True, ) diff --git a/tests/common/query_context_generator.py b/tests/common/query_context_generator.py index bd13feff067fd..a22646983bdda 100644 --- a/tests/common/query_context_generator.py +++ b/tests/common/query_context_generator.py @@ -248,8 +248,6 @@ class QueryContextGenerator: def generate( self, query_name: str, - force: bool = False, - custom_cache_timeout: Optional[int] = None, add_postprocessing_operations: bool = False, add_time_offsets: bool = False, table_id=1, @@ -261,8 +259,6 @@ def generate( table = self.get_table(table_name, table_id, table_type) return { "datasource": {"id": table.id, "type": table.type}, - "force": force, - "custom_cache_timeout": custom_cache_timeout, "queries": [ get_query_object( query_name, diff --git a/tests/integration_tests/charts/data/api_tests.py b/tests/integration_tests/charts/data/api_tests.py index 6aab4d3df75a9..4a81d67d53386 100644 --- a/tests/integration_tests/charts/data/api_tests.py +++ b/tests/integration_tests/charts/data/api_tests.py @@ -21,7 +21,7 @@ import copy from datetime import datetime from io import BytesIO -from typing import Optional +from typing import Any, Dict, Optional from unittest import mock from zipfile import ZipFile @@ -210,66 +210,6 @@ def test_with_row_limit_as_samples__rowcount_as_row_limit(self): self.assert_row_count(rv, expected_row_count) assert "GROUP BY" not in rv.json["result"][0]["query"] - @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") - @mock.patch( - "superset.common.query_context_processor.config", - { - **app.config, - "CACHE_DEFAULT_TIMEOUT": 1234, - "DATA_CACHE_CONFIG": { - **app.config["DATA_CACHE_CONFIG"], - "CACHE_DEFAULT_TIMEOUT": None, - }, - }, - ) - def test_cache_default_timeout(self): - query_context = get_query_context("birth_names", force=True) - rv = self.post_assert_metric( - CHART_DATA_URI, - query_context, - "data", - ) - data = json.loads(rv.data.decode("utf-8")) - assert data["result"][0]["cache_timeout"] == 1234 - - @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") - @mock.patch( - "superset.common.query_context_processor.config", - { - **app.config, - "CACHE_DEFAULT_TIMEOUT": 100000, - "DATA_CACHE_CONFIG": { - **app.config["DATA_CACHE_CONFIG"], - "CACHE_DEFAULT_TIMEOUT": 3456, - }, - }, - ) - @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") - def test_data_cache_default_timeout(self): - query_context = get_query_context("birth_names", force=True) - rv = self.post_assert_metric( - CHART_DATA_URI, - query_context, - "data", - ) - data = json.loads(rv.data.decode("utf-8")) - assert data["result"][0]["cache_timeout"] == 3456 - - @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") - def test_custom_cache_timeout(self): - query_context = get_query_context( - "birth_names", - force=True, - custom_cache_timeout=5678, - ) - rv = self.post_assert_metric( - CHART_DATA_URI, - query_context, - "data", - ) - data = json.loads(rv.data.decode("utf-8")) - assert data["result"][0]["cache_timeout"] == 5678 - def test_with_incorrect_result_type__400(self): self.query_context_payload["result_type"] = "qwerty" rv = self.post_assert_metric(CHART_DATA_URI, self.query_context_payload, "data") @@ -975,3 +915,67 @@ def test_chart_data_with_adhoc_column(self): unique_genders = {row["male_or_female"] for row in data} assert unique_genders == {"male", "female"} assert result["applied_filters"] == [{"column": "male_or_female"}] + + +@pytest.fixture() +def physical_query_context(physical_dataset) -> Dict[str, Any]: + return { + "datasource": { + "type": physical_dataset.type, + "id": physical_dataset.id, + }, + "queries": [ + { + "columns": ["col1"], + "metrics": ["count"], + "orderby": [["col1", True]], + } + ], + "result_type": ChartDataResultType.FULL, + "force": True, + } + + +@mock.patch( + "superset.common.query_context_processor.config", + { + **app.config, + "CACHE_DEFAULT_TIMEOUT": 1234, + "DATA_CACHE_CONFIG": { + **app.config["DATA_CACHE_CONFIG"], + "CACHE_DEFAULT_TIMEOUT": None, + }, + }, +) +def test_cache_default_timeout(test_client, login_as_admin, physical_query_context): + rv = test_client.post(CHART_DATA_URI, json=physical_query_context) + data = json.loads(rv.data.decode("utf-8")) + assert data["result"][0]["cache_timeout"] == 1234 + + +def test_custom_cache_timeout(test_client, login_as_admin, physical_query_context): + physical_query_context["custom_cache_timeout"] = 5678 + rv = test_client.post(CHART_DATA_URI, json=physical_query_context) + data = json.loads(rv.data.decode("utf-8")) + assert data["result"][0]["cache_timeout"] == 5678 + + +@mock.patch( + "superset.common.query_context_processor.config", + { + **app.config, + "CACHE_DEFAULT_TIMEOUT": 100000, + "DATA_CACHE_CONFIG": { + **app.config["DATA_CACHE_CONFIG"], + "CACHE_DEFAULT_TIMEOUT": 3456, + }, + }, +) +def test_data_cache_default_timeout( + test_client, + login_as_admin, + physical_query_context, +): + rv = test_client.post(CHART_DATA_URI, json=physical_query_context) + data = json.loads(rv.data.decode("utf-8")) + assert data["result"][0]["cache_timeout"] == 3456 diff --git a/tests/integration_tests/fixtures/query_context.py b/tests/integration_tests/fixtures/query_context.py index 2c6a8e04f421c..00a3036e01c25 100644 --- a/tests/integration_tests/fixtures/query_context.py +++ b/tests/integration_tests/fixtures/query_context.py @@ -27,8 +27,6 @@ def get_table(self, name, id_, type_): def get_query_context( query_name: str, - force: bool = False, - custom_cache_timeout: Optional[int] = None, add_postprocessing_operations: bool = False, add_time_offsets: bool = False, form_data: Optional[Dict[str, Any]] = None, @@ -39,8 +37,6 @@ def get_query_context( generated by the "Boy Name Cloud" chart in the examples. :param query_name: name of an example query, which is always in the format of `datasource_name[:test_case_name]`, where `:test_case_name` is optional. - :param force: force cache refresh - :param custom_cache_timeout: Custom cache timeout :param add_postprocessing_operations: Add post-processing operations to QueryObject :param add_time_offsets: Add time offsets to QueryObject(advanced analytics) :param form_data: chart metadata @@ -48,8 +44,6 @@ def get_query_context( """ return QueryContextGeneratorInteg().generate( query_name=query_name, - force=force, - custom_cache_timeout=custom_cache_timeout, add_postprocessing_operations=add_postprocessing_operations, add_time_offsets=add_time_offsets, form_data=form_data, From 9511f9c6a084929423425a8b21aa895384673825 Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Tue, 13 Sep 2022 14:57:51 +0300 Subject: [PATCH 4/4] simplify --- tests/integration_tests/charts/data/api_tests.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/tests/integration_tests/charts/data/api_tests.py b/tests/integration_tests/charts/data/api_tests.py index 4a81d67d53386..73d33cd793b76 100644 --- a/tests/integration_tests/charts/data/api_tests.py +++ b/tests/integration_tests/charts/data/api_tests.py @@ -949,15 +949,13 @@ def physical_query_context(physical_dataset) -> Dict[str, Any]: ) def test_cache_default_timeout(test_client, login_as_admin, physical_query_context): rv = test_client.post(CHART_DATA_URI, json=physical_query_context) - data = json.loads(rv.data.decode("utf-8")) - assert data["result"][0]["cache_timeout"] == 1234 + assert rv.json["result"][0]["cache_timeout"] == 1234 def test_custom_cache_timeout(test_client, login_as_admin, physical_query_context): physical_query_context["custom_cache_timeout"] = 5678 rv = test_client.post(CHART_DATA_URI, json=physical_query_context) - data = json.loads(rv.data.decode("utf-8")) - assert data["result"][0]["cache_timeout"] == 5678 + assert rv.json["result"][0]["cache_timeout"] == 5678 @mock.patch( @@ -977,5 +975,4 @@ def test_data_cache_default_timeout( physical_query_context, ): rv = test_client.post(CHART_DATA_URI, json=physical_query_context) - data = json.loads(rv.data.decode("utf-8")) - assert data["result"][0]["cache_timeout"] == 3456 + assert rv.json["result"][0]["cache_timeout"] == 3456