fix review comments

superset/utils/decorators.py

@@ -15,15 +15,15 @@
 # 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
 
 from contextlib2 import contextmanager
 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,24 +95,27 @@ 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:
                 response = f(*args, **kwargs)
 
                 # 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

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,22 +1601,29 @@ 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/<dashboard_id_or_slug>/")
     def dashboard(  # pylint: disable=too-many-locals
         self, dashboard_id_or_slug: str
     ) -> 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(

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):

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)