Skip to content

fix: enable consistent etag across workers and force no-cache for dashboards #11137

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
graceguo-supercat merged 5 commits into from
Oct 6, 2020
Merged

fix: enable consistent etag across workers and force no-cache for dashboards #11137

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5086d26
fix: enable consistent etag across workers
ktmud Oct 1, 2020
96dc5cf
Use CACHE_DEFAULT_TIMEOUT instead of 0
ktmud Oct 2, 2020
9e057b9
Change timeout to 0 and set expires header even for no-cache
ktmud Oct 2, 2020
97ce058
Reduce number of if branches to appease Pylint
ktmud Oct 2, 2020
f5d9a2a
Fix mypy error
ktmud Oct 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 21 additions & 13 deletions superset/utils/decorators.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ def etag_cache(
check_perms: Callable[..., Any],
get_last_modified: Optional[Callable[..., Any]] = None,
skip: Optional[Callable[..., Any]] = None,
must_revalidate: Optional[bool] = False,
Copy link
Member Author

@ktmud ktmud Oct 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could potentially always assume must_revalidate if get_last_modified is provided. Because validation only makes sense when there is a way to programmatically get a resource's the "real" last modified time.

) -> Callable[..., Any]:
"""
A decorator for caching views and handling etag conditional requests.
Expand Down Expand Up @@ -78,6 +79,8 @@ def wrapper(*args: Any, **kwargs: Any) -> ETagResponseMixin:
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
Expand All @@ -94,32 +97,37 @@ def wrapper(*args: Any, **kwargs: Any) -> ETagResponseMixin:
raise
logger.exception("Exception possibly due to cache backend.")

# if cache is stale?
if get_last_modified:
content_changed_time = get_last_modified(*args, **kwargs)
# if cache is stale?
if (
response
and last_modified
and response.last_modified
and response.last_modified.timestamp()
< content_changed_time.timestamp()
and response.last_modified < last_modified
):
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:
# if no response was cached, compute it using the wrapped function
response = f(*args, **kwargs)

# add headers for caching: Last Modified, Expires and ETag
response.cache_control.public = True
response.last_modified = content_changed_time
# set expiration headers:
# Last-Modified, Expires, Cache-Control, ETag
response.last_modified = last_modified or 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
Expand Down
8 changes: 8 additions & 0 deletions superset/views/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,13 @@ 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:
Expand Down Expand Up @@ -1605,6 +1612,7 @@ def publish( # pylint: disable=no-self-use
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
Expand Down