From 18b31f856407a21eb099302872bd24e117557608 Mon Sep 17 00:00:00 2001 From: Grace Date: Fri, 25 Sep 2020 11:11:09 -0700 Subject: [PATCH] fix review comments --- superset/utils/decorators.py | 44 ++++++++++++++++-------------------- superset/views/core.py | 17 ++++++++++---- superset/views/utils.py | 18 ++++----------- tests/utils_tests.py | 12 ++++++++++ 4 files changed, 47 insertions(+), 44 deletions(-) diff --git a/superset/utils/decorators.py b/superset/utils/decorators.py index ec284fc7cac77..ae4c5726d8ee7 100644 --- a/superset/utils/decorators.py +++ b/superset/utils/decorators.py @@ -15,7 +15,7 @@ # specific language governing permissions and limitations # under the License. import logging -from datetime import datetime, timedelta, timezone +from datetime import datetime, timedelta from functools import wraps from typing import Any, Callable, Iterator, Optional @@ -23,7 +23,7 @@ from flask import request from werkzeug.wrappers.etag import ETagResponseMixin -from superset import app, cache, is_feature_enabled +from superset import app, cache from superset.stats_logger import BaseStatsLogger from superset.utils.dates import now_as_float @@ -34,10 +34,6 @@ logger = logging.getLogger(__name__) -def is_dashboard_request(kwargs: Any) -> bool: - return kwargs.get("dashboard_id_or_slug") is not None - - @contextmanager def stats_timing(stats_key: str, stats_logger: BaseStatsLogger) -> Iterator[float]: """Provide a transactional scope around a series of operations.""" @@ -53,7 +49,8 @@ def stats_timing(stats_key: str, stats_logger: BaseStatsLogger) -> Iterator[floa def etag_cache( max_age: int, check_perms: Callable[..., Any], - check_latest_changed_on: Optional[Callable[..., Any]] = None, + get_last_modified: Optional[Callable[..., Any]] = None, + skip: Optional[Callable[..., Any]] = None, ) -> Callable[..., Any]: """ A decorator for caching views and handling etag conditional requests. @@ -77,13 +74,7 @@ def wrapper(*args: Any, **kwargs: Any) -> ETagResponseMixin: # for POST requests we can't set cache headers, use the response # cache nor use conditional requests; this will still use the # dataframe cache in `superset/viz.py`, though. - if request.method == "POST": - return f(*args, **kwargs) - - # if it is dashboard request but feature is not eabled, - # do not use cache - is_dashboard = is_dashboard_request(kwargs) - if is_dashboard and not is_feature_enabled("ENABLE_DASHBOARD_ETAG_HEADER"): + if request.method == "POST" or (skip and skip(*args, **kwargs)): return f(*args, **kwargs) response = None @@ -104,14 +95,19 @@ def wrapper(*args: Any, **kwargs: Any) -> ETagResponseMixin: logger.exception("Exception possibly due to cache backend.") # if cache is stale? - if check_latest_changed_on: - latest_changed_on = check_latest_changed_on(*args, **kwargs) - if response and response.last_modified: - latest_record = response.last_modified.replace( - tzinfo=timezone.utc - ).astimezone(tz=None) - if latest_changed_on.timestamp() > latest_record.timestamp(): - response = None + if get_last_modified: + content_changed_time = get_last_modified(*args, **kwargs) + if ( + response + and response.last_modified + and response.last_modified.timestamp() + < content_changed_time.timestamp() + ): + response = None + else: + # if caller didn't provide content's last_modified time, assume + # its cache won't be stale. + content_changed_time = datetime.utcnow() # if no response was cached, compute it using the wrapped function if response is None: @@ -119,9 +115,7 @@ def wrapper(*args: Any, **kwargs: Any) -> ETagResponseMixin: # add headers for caching: Last Modified, Expires and ETag response.cache_control.public = True - response.last_modified = ( - latest_changed_on if is_dashboard else datetime.utcnow() - ) + response.last_modified = content_changed_time expiration = max_age if max_age != 0 else FAR_FUTURE response.expires = response.last_modified + timedelta( seconds=expiration diff --git a/superset/views/core.py b/superset/views/core.py index 3bb7ce30d731b..a82b59fa1e41f 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -17,6 +17,7 @@ # pylint: disable=comparison-with-callable import logging import re +from collections import defaultdict from contextlib import closing from datetime import datetime from typing import Any, cast, Dict, List, Optional, Union @@ -128,10 +129,9 @@ check_slice_perms, get_cta_schema_name, get_dashboard, + get_dashboard_changedon_dt, get_dashboard_extra_filters, - get_dashboard_latest_changed_on, get_datasource_info, - get_datasources_from_dashboard, get_form_data, get_viz, is_owner, @@ -1601,7 +1601,10 @@ def publish( # pylint: disable=no-self-use @etag_cache( 0, check_perms=check_dashboard_perms, - check_latest_changed_on=get_dashboard_latest_changed_on, + get_last_modified=get_dashboard_changedon_dt, + skip=lambda _self, dashboard_id_or_slug: not is_feature_enabled( + "ENABLE_DASHBOARD_ETAG_HEADER" + ), ) @expose("/dashboard//") def dashboard( # pylint: disable=too-many-locals @@ -1609,14 +1612,18 @@ def dashboard( # pylint: disable=too-many-locals ) -> FlaskResponse: """Server side rendering for a dashboard""" dash = get_dashboard(dashboard_id_or_slug) - datasources = get_datasources_from_dashboard(dash) + slices_by_datasources = defaultdict(list) + for slc in dash.slices: + datasource = slc.datasource + if datasource: + slices_by_datasources[datasource].append(slc) # Filter out unneeded fields from the datasource payload datasources_payload = { datasource.uid: datasource.data_for_slices(slices) if is_feature_enabled("REDUCE_DASHBOARD_BOOTSTRAP_PAYLOAD") else datasource.data - for datasource, slices in datasources.items() + for datasource, slices in slices_by_datasources.items() } dash_edit_perm = check_ownership( diff --git a/superset/views/utils.py b/superset/views/utils.py index f467494c0ae6f..dc164944c2b57 100644 --- a/superset/views/utils.py +++ b/superset/views/utils.py @@ -321,18 +321,7 @@ def get_dashboard(dashboard_id_or_slug: str,) -> Dashboard: return dashboard -def get_datasources_from_dashboard( - dashboard: Dashboard, -) -> DefaultDict[Any, List[Any]]: - datasources = defaultdict(list) - for slc in dashboard.slices: - datasource = slc.datasource - if datasource: - datasources[datasource].append(slc) - return datasources - - -def get_dashboard_latest_changed_on(_self: Any, dashboard_id_or_slug: str) -> datetime: +def get_dashboard_changedon_dt(_self: Any, dashboard_id_or_slug: str) -> datetime: """ Get latest changed datetime for a dashboard. The change could be dashboard metadata change, or any of its slice data change. @@ -343,7 +332,8 @@ def get_dashboard_latest_changed_on(_self: Any, dashboard_id_or_slug: str) -> da dash = get_dashboard(dashboard_id_or_slug) dash_changed_on = dash.changed_on slices_changed_on = max([s.changed_on for s in dash.slices]) - return max(dash_changed_on, slices_changed_on) + # drop microseconds in datetime to match with last_modified header + return max(dash_changed_on, slices_changed_on).replace(microsecond=0) def get_dashboard_extra_filters( @@ -547,7 +537,7 @@ def check_dashboard_perms(_self: Any, dashboard_id_or_slug: str) -> None: """ dash = get_dashboard(dashboard_id_or_slug) - datasources = get_datasources_from_dashboard(dash) + datasources = list(dash.datasources) if app.config["ENABLE_ACCESS_REQUEST"]: for datasource in datasources: if datasource and not security_manager.can_access_datasource(datasource): diff --git a/tests/utils_tests.py b/tests/utils_tests.py index 035a7864feb20..c63fcc119328d 100644 --- a/tests/utils_tests.py +++ b/tests/utils_tests.py @@ -67,6 +67,7 @@ from superset.utils import schema from superset.views.utils import ( build_extra_filters, + get_dashboard_changedon_dt, get_form_data, get_time_range_endpoints, ) @@ -1134,3 +1135,14 @@ def test_get_form_data_token(self): assert get_form_data_token({"token": "token_abcdefg1"}) == "token_abcdefg1" generated_token = get_form_data_token({}) assert re.match(r"^token_[a-z0-9]{8}$", generated_token) is not None + + def test_get_dashboard_changedon_dt(self) -> None: + slug = "world_health" + dashboard = db.session.query(Dashboard).filter_by(slug=slug).one() + dashboard_last_changedon = dashboard.changed_on + slices = dashboard.slices + slices_last_changedon = max([slc.changed_on for slc in slices]) + # drop microsecond in datetime + assert get_dashboard_changedon_dt(self, slug) == max( + dashboard_last_changedon, slices_last_changedon + ).replace(microsecond=0)