Revert "feat: Use unversioned frontend asset paths (#27091)"

This reverts commit 8b2ff25.

.github/workflows/acceptance.yml

@@ -64,11 +64,15 @@ jobs:
           name: jest-html
           path: .artifacts/visual-snapshots/jest
 
+      - name: Get CSS filename
+        id: css-manifest
+        run: echo "::set-output name=sentrycss::$(jq -r '.["sentry.css"]' src/sentry/static/sentry/dist/manifest.json)"
+
       - name: Create Images from HTML
         uses: getsentry/action-html-to-image@main
         with:
           base-path: .artifacts/visual-snapshots/jest
-          css-path: src/sentry/static/sentry/dist/entrypoints/sentry.css
+          css-path: src/sentry/static/sentry/dist/${{ steps.css-manifest.outputs.sentrycss }}
 
       - name: Save snapshots
         if: always()

conftest.py

@@ -1,6 +1,10 @@
 import os
 import sys
 
+dist_path = os.path.abspath(
+    os.path.join(os.path.dirname(__file__), "src", "sentry", "static", "sentry", "dist")
+)
+manifest_path = os.path.join(dist_path, "manifest.json")
 pytest_plugins = ["sentry.utils.pytest"]
 
 sys.path.insert(0, os.path.join(os.path.dirname(__file__), "src"))
@@ -12,3 +16,24 @@ def pytest_configure(config):
     # XXX(dcramer): Kombu throws a warning due to transaction.commit_manually
     # being used
     warnings.filterwarnings("error", "", Warning, r"^(?!(|kombu|raven|sentry))")
+
+    # Create an empty webpack manifest file - otherwise tests will crash if it does not exist
+    os.makedirs(dist_path, exist_ok=True)
+
+    # Only create manifest if it doesn't exist
+    # (e.g. acceptance tests will have an actual manifest from webpack)
+    if os.path.exists(manifest_path):
+        return
+
+    with open(manifest_path, "w+") as fp:
+        fp.write("{}")
+
+
+def pytest_unconfigure():
+    if not os.path.exists(manifest_path):
+        return
+
+    # Clean up manifest file if contents are empty
+    with open(manifest_path) as f:
+        if f.read() == "{}":
+            os.remove(manifest_path)

package.json

@@ -135,6 +135,7 @@
     "u2f-api": "1.0.10",
     "webpack": "5.43.0",
     "webpack-cli": "4.7.2",
+    "webpack-manifest-plugin": "^3.1.1",
     "webpack-remove-empty-scripts": "^0.7.1",
     "wink-jaro-distance": "^2.0.0",
     "zxcvbn": "^4.4.2"

requirements-base.txt

@@ -9,6 +9,7 @@ croniter==0.3.37
 dataclasses==0.8; python_version <= '3.6'
 datadog==0.29.3
 django-crispy-forms==1.7.2
+django-manifest-loader==1.0.0
 django-picklefield==1.0.0
 Django==1.11.29
 djangorestframework==3.11.2

src/sentry/conf/server.py

@@ -327,6 +327,7 @@ def env(key, default="", type=None):
     "django.contrib.sites",
     "crispy_forms",
     "rest_framework",
+    "manifest_loader",
     "sentry",
     "sentry.analytics",
     "sentry.incidents.apps.Config",
@@ -366,13 +367,16 @@ def env(key, default="", type=None):
 STATIC_URL = "/_static/{version}/"
 # webpack assets live at a different URL that is unversioned
 # as we configure webpack to include file content based hash in the filename
-STATIC_UNVERSIONED_URL = "/_static/dist/"
+STATIC_MANIFEST_URL = "/_static/dist/"
 
-# The webpack output directory
+# The webpack output directory, used by django-manifest-loader
 STATICFILES_DIRS = [
     os.path.join(STATIC_ROOT, "sentry", "dist"),
 ]
 
+# django-manifest-loader settings
+MANIFEST_LOADER = {"cache": True}
+
 # various middleware will use this to identify resources which should not access
 # cookies
 ANONYMOUS_STATIC_PREFIXES = (

src/sentry/middleware/user.py

@@ -7,7 +7,6 @@
 
 class UserActiveMiddleware(MiddlewareMixin):
     disallowed_paths = (
-        "sentry.web.frontend.generic.unversioned_static_media",
         "sentry.web.frontend.generic.static_media",
         "sentry.web.frontend.organization_avatar",
         "sentry.web.frontend.project_avatar",

src/sentry/templates/sentry/bases/react.html

@@ -1 +1,2 @@
 {% extends "sentry/base-react.html" %}
+{% load sentry_assets %}

src/sentry/templates/sentry/bases/react_pipeline.html

@@ -14,6 +14,6 @@
 {% endblock %}
 
 {% block scripts_main_entrypoint %}
-    {% unversioned_asset_url "sentry" "entrypoints/pipeline.js" as asset_url %}
+    {% manifest_asset_url "sentry" "pipeline.js" as asset_url %}
     {% script src=asset_url data-entry="true" %}{% endscript %}
 {% endblock %}

src/sentry/templates/sentry/layout.html

@@ -25,7 +25,7 @@
 
   <link rel="mask-icon" sizes="any" href="{% absolute_asset_url "sentry" "images/logos/logo-sentry.svg" %}" color="#FB4226">
 
-  <link href="{% unversioned_asset_url "sentry" "entrypoints/sentry.css" %}" rel="stylesheet"/>
+  <link href="{% manifest_asset_url "sentry" "sentry.css" %}" rel="stylesheet"/>
 
   {% block css %}{% endblock %}
 
@@ -54,7 +54,7 @@
 
   {% block scripts %}
   {% block scripts_main_entrypoint %}
-    {% unversioned_asset_url "sentry" "entrypoints/app.js" as asset_url %}
+    {% manifest_asset_url "sentry" "app.js" as asset_url %}
     {% script src=asset_url data-entry="true" %}{% endscript %}
   {% endblock %}
 

src/sentry/templatetags/sentry_assets.py

@@ -5,13 +5,13 @@
 from django.template.base import token_kwargs
 
 from sentry import options
-from sentry.utils.assets import get_asset_url, get_unversioned_asset_url
+from sentry.utils.assets import get_asset_url, get_manifest_url
 from sentry.utils.http import absolute_uri
 
 register = template.Library()
 
 register.simple_tag(get_asset_url, name="asset_url")
-register.simple_tag(get_unversioned_asset_url, name="unversioned_asset_url")
+register.simple_tag(get_manifest_url, name="manifest_asset_url")
 
 
 @register.simple_tag

src/sentry/testutils/cases.py

@@ -106,6 +106,36 @@ def setup_dummy_auth_provider(self):
     def tasks(self):
         return TaskRunner()
 
+    @classmethod
+    @contextmanager
+    def static_asset_manifest(cls, manifest_data):
+        dist_path = "src/sentry/static/sentry/dist"
+        manifest_path = f"{dist_path}/manifest.json"
+
+        with open(manifest_path, "w") as manifest_fp:
+            json.dump(manifest_data, manifest_fp)
+
+        files = []
+        for file_path in manifest_data.values():
+            full_path = f"{dist_path}/{file_path}"
+            # make directories in case they don't exist
+            # (e.g. dist path should exist, but subdirs won't)
+            os.makedirs(os.path.dirname(full_path), exist_ok=True)
+            open(full_path, "a").close()
+            files.append(full_path)
+
+        try:
+            yield {"manifest": manifest_data, "files": files}
+        finally:
+            with open(manifest_path, "w") as manifest_fp:
+                # Instead of unlinking, preserve an empty manifest file so that other tests that
+                # may or may not load static assets, do not fail
+                manifest_fp.write("{}")
+
+            # Remove any files created from the test manifest
+            for filepath in files:
+                os.unlink(filepath)
+
     @classmethod
     @contextmanager
     def capture_on_commit_callbacks(cls, using=DEFAULT_DB_ALIAS, execute=False):

src/sentry/utils/assets.py

@@ -1,26 +1,37 @@
 from django.conf import settings
+from manifest_loader.utils import _get_manifest, _load_from_manifest
 
 
-def get_unversioned_asset_url(module, key):
+def get_manifest_obj():
     """
-    Returns an asset URL that is unversioned. These assets should have a
-    `Cache-Control: max-age=0, must-revalidate` so that clients must validate with the origin
-    server before using their locally cached asset.
+    Returns the webpack asset manifest as a dict of <file key, hashed file name>
+
+    The `manifest_loader` library caches this (if `cache` settings is set)
+    """
+    return _get_manifest()
+
+
+def get_manifest_url(module, key):
+    """
+    Returns an asset URL that is produced by webpack. Uses webpack's manifest to map
+    `key` to the asset produced by webpack. Required if using file contents based hashing for filenames.
 
     Example:
-      {% unversioned_asset_url 'sentry' 'sentry.css' %}
-      =>  "/_static/dist/sentry/sentry.css"
+      {% manifest_asset_url 'sentry' 'sentry.css' %}
+      =>  "/_static/dist/sentry/sentry.filehash123.css"
     """
+    manifest_obj = get_manifest_obj()
+    manifest_value = _load_from_manifest(manifest_obj, key=key)
 
-    return "{}/{}/{}".format(settings.STATIC_UNVERSIONED_URL.rstrip("/"), module, key.lstrip("/"))
+    return "{}/{}/{}".format(settings.STATIC_MANIFEST_URL.rstrip("/"), module, manifest_value)
 
 
 def get_asset_url(module, path):
     """
     Returns a versioned asset URL (located within Sentry's static files).
 
     Example:
-    {% asset_url 'sentry' 'images/sentry.png' %}
-    =>  "/_static/74d127b78dc7daf2c51f/sentry/sentry.png"
+      {% asset_url 'sentry' 'images/sentry.png' %}
+      =>  "/_static/74d127b78dc7daf2c51f/sentry/sentry.png"
     """
     return "{}/{}/{}".format(settings.STATIC_URL.rstrip("/"), module, path.lstrip("/"))

src/sentry/web/client_config.py

@@ -12,7 +12,7 @@
 from sentry.auth.superuser import is_active_superuser
 from sentry.models import ProjectKey
 from sentry.utils import auth
-from sentry.utils.assets import get_unversioned_asset_url
+from sentry.utils.assets import get_manifest_url
 from sentry.utils.email import is_smtp_enabled
 from sentry.utils.support import get_support_mail
 
@@ -146,7 +146,7 @@ def get_client_config(request=None):
         "urlPrefix": options.get("system.url-prefix"),
         "version": version_info,
         "features": enabled_features,
-        "distPrefix": get_unversioned_asset_url("sentry", ""),
+        "distPrefix": get_manifest_url("sentry", ""),
         "needsUpgrade": needs_upgrade,
         "dsn": public_dsn,
         "dsn_requests": _get_dsn_requests(),

src/sentry/web/frontend/generic.py

@@ -8,18 +8,6 @@
 from django.views import static
 
 FOREVER_CACHE = "max-age=315360000"
-
-# See
-# https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control#requiring_revalidation
-# This means that clients *CAN* cache the resource, but they must revalidate
-# before using it This means we will have a small HTTP request overhead to
-# verify that the local resource is not outdated
-#
-# Note that the above docs state that "no-cache" is the same as "max-age=0,
-# must-revalidate", but some CDNs will not treat them as the same
-NO_CACHE = "max-age=0, must-revalidate"
-
-# no-store means that the response should not be stored in *ANY* cache
 NEVER_CACHE = "max-age=0, no-cache, no-store, must-revalidate"
 
 
@@ -45,21 +33,19 @@ def resolve(path):
     return os.path.split(absolute_path)
 
 
-def unversioned_static_media(request, **kwargs):
-    """
-    Serve static files that should not have any versioned paths/filenames.
-    These assets will have cache headers to say that it can be cached by a
-    client, but it *must* be validated against the origin server before the
-    cached asset can be used.
+def static_media_with_manifest(request, **kwargs):
     """
+    Serve static files that are generated with webpack.
 
+    Only these assets should have a long TTL as its filename has a hash based on file contents
+    """
     path = kwargs.get("path", "")
 
     kwargs["path"] = f"dist/{path}"
     response = static_media(request, **kwargs)
 
     if not settings.DEBUG:
-        response["Cache-Control"] = NO_CACHE
+        response["Cache-Control"] = FOREVER_CACHE
 
     return response
 

src/sentry/web/urls.py

@@ -96,8 +96,8 @@
     # a filecontent-based hash in its filenames so that it can be cached long term
     url(
         r"^_static/dist/(?P<module>[^/]+)/(?P<path>.*)$",
-        generic.unversioned_static_media,
-        name="sentry-unversioned-media",
+        generic.static_media_with_manifest,
+        name="sentry-webpack-media",
     ),
     # The static version is either a 10 digit timestamp, a sha1, or md5 hash
     url(

tests/sentry/web/frontend/generic/test_static_media.py

@@ -3,8 +3,8 @@
 from django.test.utils import override_settings
 
 from sentry.testutils import TestCase
-from sentry.utils.assets import get_unversioned_asset_url
-from sentry.web.frontend.generic import FOREVER_CACHE, NEVER_CACHE, NO_CACHE
+from sentry.utils.assets import get_manifest_url
+from sentry.web.frontend.generic import FOREVER_CACHE, NEVER_CACHE
 
 
 class StaticMediaTest(TestCase):
@@ -44,40 +44,36 @@ def test_versioned(self):
             assert response["Access-Control-Allow-Origin"] == "*"
 
     @override_settings(DEBUG=False)
-    def test_unversioned(self):
+    def test_from_manifest(self):
         """
-        static assets that do not have versioned filenames/paths
+        manifest here refers to the webpack manifest for frontend assets
         """
 
-        # non-existant dist file
-        response = self.client.get("/_static/dist/sentry/invalid.js")
-        assert response.status_code == 404, response
+        app_manifest = {
+            "app.js": "app.f00f00.js",
+        }
 
-        dist_path = os.path.join("src", "sentry", "static", "sentry", "dist")
-        os.makedirs(dist_path, exist_ok=True)
+        with self.static_asset_manifest(app_manifest):
+            # `get_manifest_url()` should return the mapped filename
+            url = get_manifest_url("sentry", "app.js")
 
-        try:
-            with open(os.path.join(dist_path, "test.js"), "a"):
-                url = get_unversioned_asset_url("sentry", "test.js")
+            response = self.client.get(url)
+            assert response.status_code == 200, response
+            assert response["Cache-Control"] == FOREVER_CACHE
+            assert response["Vary"] == "Accept-Encoding"
+            assert response["Access-Control-Allow-Origin"] == "*"
+            assert "Content-Encoding" not in response
 
-                response = self.client.get(url)
-                assert response.status_code == 200, response
-                assert response["Cache-Control"] == NO_CACHE
-                assert response["Vary"] == "Accept-Encoding"
-                assert response["Access-Control-Allow-Origin"] == "*"
-                assert "Content-Encoding" not in response
+            # non-existant dist file
+            response = self.client.get("/_static/dist/sentry/invalid.js")
+            assert response.status_code == 404, response
 
             with override_settings(DEBUG=True):
                 response = self.client.get(url)
                 assert response.status_code == 200, response
                 assert response["Cache-Control"] == NEVER_CACHE
                 assert response["Vary"] == "Accept-Encoding"
                 assert response["Access-Control-Allow-Origin"] == "*"
-        finally:
-            try:
-                os.unlink(os.path.join(dist_path, "test.js"))
-            except Exception:
-                pass
 
     @override_settings(DEBUG=False)
     def test_no_cors(self):