fix: revert eTag cache feature for dashboard (#11203)

* revert #11137

* revert #10963

superset/config.py

@@ -297,7 +297,6 @@ DEFAULT_FEATURE_FLAGS: Dict[str, bool] = {
     # Experimental feature introducing a client (browser) cache
     "CLIENT_CACHE": False,
     "ENABLE_EXPLORE_JSON_CSRF_PROTECTION": False,
-    "ENABLE_DASHBOARD_ETAG_HEADER": False,
     "ENABLE_TEMPLATE_PROCESSING": False,
     "KV_STORE": False,
     "PRESTO_EXPAND_DATA": False,

superset/utils/decorators.py

@@ -17,7 +17,7 @@
 import logging
 from datetime import datetime, timedelta
 from functools import wraps
-from typing import Any, Callable, Iterator, Optional
+from typing import Any, Callable, Iterator
 
 from contextlib2 import contextmanager
 from flask import request
@@ -46,13 +46,7 @@ def stats_timing(stats_key: str, stats_logger: BaseStatsLogger) -> Iterator[floa
         stats_logger.timing(stats_key, now_as_float() - start_ts)
 
 
-def etag_cache(
+def etag_cache(max_age: int, check_perms: Callable[..., Any]) -> Callable[..., Any]:
-    max_age: int,
-    check_perms: Callable[..., Any],
-    get_last_modified: Optional[Callable[..., Any]] = None,
-    skip: Optional[Callable[..., Any]] = None,
-    must_revalidate: Optional[bool] = False,
-) -> Callable[..., Any]:
     """
     A decorator for caching views and handling etag conditional requests.
 
@@ -75,12 +69,10 @@ def etag_cache(
             # 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" or (skip and skip(*args, **kwargs)):
+            if request.method == "POST":
                 return f(*args, **kwargs)
 
             response = None
-            last_modified = get_last_modified and get_last_modified(*args, **kwargs)
-
             if cache:
                 try:
                     # build the cache key from the function arguments and any
@@ -97,37 +89,17 @@ def etag_cache(
                         raise
                     logger.exception("Exception possibly due to cache backend.")
 
-                # if cache is stale?
+            # if no response was cached, compute it using the wrapped function
-                if (
-                    response
-                    and last_modified
-                    and response.last_modified
-                    and response.last_modified < last_modified
-                ):
-                    response = None
-
             if response is None:
-                # if no response was cached, compute it using the wrapped function
                 response = f(*args, **kwargs)
 
-                # set expiration headers:
+                # add headers for caching: Last Modified, Expires and ETag
-                #   Last-Modified, Expires, Cache-Control, ETag
+                response.cache_control.public = True
-                response.last_modified = last_modified or datetime.utcnow()
+                response.last_modified = datetime.utcnow()
                 expiration = max_age if max_age != 0 else FAR_FUTURE
                 response.expires = response.last_modified + timedelta(
                     seconds=expiration
                 )
-
-                # when needed, instruct the browser to always revalidate cache
-                if must_revalidate:
-                    # `Cache-Control: no-cache` asks the browser to always store
-                    # the cache, but also must validate it with the server.
-                    response.cache_control.no_cache = True
-                else:
-                    # `Cache-Control: Public` asks the browser to always store
-                    # the cache.
-                    response.cache_control.public = True
-
                 response.add_etag()
 
                 # if we have a cache, store the response from the request

superset/views/core.py

@@ -26,17 +26,7 @@ from urllib import parse
 import backoff
 import pandas as pd
 import simplejson as json
-from flask import (
+from flask import abort, flash, g, Markup, redirect, render_template, request, Response
-    abort,
-    flash,
-    g,
-    make_response,
-    Markup,
-    redirect,
-    render_template,
-    request,
-    Response,
-)
 from flask_appbuilder import expose
 from flask_appbuilder.models.sqla.interface import SQLAInterface
 from flask_appbuilder.security.decorators import has_access, has_access_api
@@ -124,12 +114,9 @@ from superset.views.utils import (
     _deserialize_results_payload,
     apply_display_max_row_limit,
     bootstrap_user_data,
-    check_dashboard_perms,
     check_datasource_perms,
     check_slice_perms,
     get_cta_schema_name,
-    get_dashboard,
-    get_dashboard_changedon_dt,
     get_dashboard_extra_filters,
     get_datasource_info,
     get_form_data,
@@ -171,13 +158,6 @@ class Superset(BaseSupersetView):  # pylint: disable=too-many-public-methods
 
     logger = logging.getLogger(__name__)
 
-    def __repr__(self) -> str:
-        """Determinate string representation of the view instance for etag_cache."""
-        return "Superset.views.core.Superset@v{}{}".format(
-            self.appbuilder.app.config["VERSION_STRING"],
-            self.appbuilder.app.config["VERSION_SHA"],
-        )
-
     @has_access_api
     @expose("/datasources/")
     def datasources(self) -> FlaskResponse:
@@ -1605,33 +1585,49 @@ class Superset(BaseSupersetView):  # pylint: disable=too-many-public-methods
         return json_success(json.dumps({"published": dash.published}))
 
     @has_access
-    @etag_cache(
-        0,
-        check_perms=check_dashboard_perms,
-        get_last_modified=get_dashboard_changedon_dt,
-        skip=lambda _self, dashboard_id_or_slug: not is_feature_enabled(
-            "ENABLE_DASHBOARD_ETAG_HEADER"
-        ),
-        must_revalidate=True,
-    )
     @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)
+        session = db.session()
+        qry = session.query(Dashboard)
+        if dashboard_id_or_slug.isdigit():
+            qry = qry.filter_by(id=int(dashboard_id_or_slug))
+        else:
+            qry = qry.filter_by(slug=dashboard_id_or_slug)
 
-        slices_by_datasources = defaultdict(list)
+        dash = qry.one_or_none()
+        if not dash:
+            abort(404)
+
+        datasources = defaultdict(list)
         for slc in dash.slices:
             datasource = slc.datasource
             if datasource:
-                slices_by_datasources[datasource].append(slc)
+                datasources[datasource].append(slc)
+
+        if config["ENABLE_ACCESS_REQUEST"]:
+            for datasource in datasources:
+                if datasource and not security_manager.can_access_datasource(
+                    datasource
+                ):
+                    flash(
+                        __(
+                            security_manager.get_datasource_access_error_msg(datasource)
+                        ),
+                        "danger",
+                    )
+                    return redirect(
+                        "superset/request_access/?" f"dashboard_id={dash.id}&"
+                    )
+
         # 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 slices_by_datasources.items()
+            for datasource, slices in datasources.items()
         }
 
         dash_edit_perm = check_ownership(
@@ -1665,7 +1661,7 @@ class Superset(BaseSupersetView):  # pylint: disable=too-many-public-methods
         if is_feature_enabled("REMOVE_SLICE_LEVEL_LABEL_COLORS"):
             # dashboard metadata has dashboard-level label_colors,
             # so remove slice-level label_colors from its form_data
-            for slc in dashboard_data.get("slices") or []:
+            for slc in dashboard_data.get("slices"):
                 form_data = slc.get("form_data")
                 form_data.pop("label_colors", None)
 
@@ -1699,17 +1695,15 @@ class Superset(BaseSupersetView):  # pylint: disable=too-many-public-methods
                 json.dumps(bootstrap_data, default=utils.pessimistic_json_iso_dttm_ser)
             )
 
-        return make_response(
+        return self.render_template(
-            self.render_template(
+            "superset/dashboard.html",
-                "superset/dashboard.html",
+            entry="dashboard",
-                entry="dashboard",
+            standalone_mode=standalone_mode,
-                standalone_mode=standalone_mode,
+            title=dash.dashboard_title,
-                title=dash.dashboard_title,
+            custom_css=dashboard_data.get("css"),
-                custom_css=dashboard_data.get("css"),
+            bootstrap_data=json.dumps(
-                bootstrap_data=json.dumps(
+                bootstrap_data, default=utils.pessimistic_json_iso_dttm_ser
-                    bootstrap_data, default=utils.pessimistic_json_iso_dttm_ser
+            ),
-                ),
-            )
         )
 
     @api

superset/views/utils.py

@@ -16,28 +16,21 @@
 # under the License.
 import logging
 from collections import defaultdict
-from datetime import date, datetime
+from datetime import date
 from typing import Any, Callable, DefaultDict, Dict, List, Optional, Set, Tuple, Union
 from urllib import parse
 
 import msgpack
 import pyarrow as pa
 import simplejson as json
-from flask import abort, flash, g, redirect, request
+from flask import g, request
 from flask_appbuilder.security.sqla import models as ab_models
 from flask_appbuilder.security.sqla.models import User
 from flask_babel import gettext as __
 from sqlalchemy.orm.exc import NoResultFound
 
 import superset.models.core as models
-from superset import (
+from superset import app, dataframe, db, is_feature_enabled, result_set
-    app,
-    dataframe,
-    db,
-    is_feature_enabled,
-    result_set,
-    security_manager,
-)
 from superset.connectors.connector_registry import ConnectorRegistry
 from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
 from superset.exceptions import SupersetException, SupersetSecurityException
@@ -307,36 +300,6 @@ def get_time_range_endpoints(
 CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
 
 
-def get_dashboard(dashboard_id_or_slug: str,) -> Dashboard:
-    session = db.session()
-    qry = session.query(Dashboard)
-    if dashboard_id_or_slug.isdigit():
-        qry = qry.filter_by(id=int(dashboard_id_or_slug))
-    else:
-        qry = qry.filter_by(slug=dashboard_id_or_slug)
-    dashboard = qry.one_or_none()
-
-    if not dashboard:
-        abort(404)
-
-    return dashboard
-
-
-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.
-
-    This function takes `self` since it must have the same signature as the
-    the decorated method.
-    """
-    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])
-    # 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(
     slice_id: int, dashboard_id: int
 ) -> List[Dict[str, Any]]:
@@ -547,26 +510,6 @@ def check_slice_perms(_self: Any, slice_id: int) -> None:
         viz_obj.raise_for_access()
 
 
-def check_dashboard_perms(_self: Any, dashboard_id_or_slug: str) -> None:
-    """
-    Check if user can access a cached response from explore_json.
-
-    This function takes `self` since it must have the same signature as the
-    the decorated method.
-    """
-
-    dash = get_dashboard(dashboard_id_or_slug)
-    datasources = list(dash.datasources)
-    if app.config["ENABLE_ACCESS_REQUEST"]:
-        for datasource in datasources:
-            if datasource and not security_manager.can_access_datasource(datasource):
-                flash(
-                    __(security_manager.get_datasource_access_error_msg(datasource)),
-                    "danger",
-                )
-                redirect("superset/request_access/?" f"dashboard_id={dash.id}&")
-
-
 def _deserialize_results_payload(
     payload: Union[bytes, str], query: Query, use_msgpack: Optional[bool] = False
 ) -> Dict[str, Any]:

tests/utils_tests.py

@@ -67,7 +67,6 @@ from superset.utils.core import (
 from superset.utils import schema
 from superset.views.utils import (
     build_extra_filters,
-    get_dashboard_changedon_dt,
     get_form_data,
     get_time_range_endpoints,
 )
@@ -1135,14 +1134,3 @@ class TestUtils(SupersetTestCase):
         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)