diff --git a/docker-compose.yml b/docker-compose.yml index 1c0d1592d179..ae07bfa76b25 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -34,6 +34,7 @@ x-superset-volumes: &superset-volumes - superset_home:/app/superset_home - ./tests:/app/tests - superset_data:/app/data + - ./local_extensions:/app/local_extensions x-common-build: &common-build context: . target: ${SUPERSET_BUILD_TARGET:-dev} # can use `dev` (default) or `lean` diff --git a/docker/docker-bootstrap.sh b/docker/docker-bootstrap.sh index 8eec76cbcd03..b909b3b50929 100755 --- a/docker/docker-bootstrap.sh +++ b/docker/docker-bootstrap.sh @@ -98,7 +98,9 @@ case "${1}" in echo " 🔒 Werkzeug debugger disabled (set SUPERSET_DEBUG_ENABLED=true to enable)" fi - flask run -p $PORT --reload $DEBUGGER_FLAG --host=0.0.0.0 --exclude-patterns "*/node_modules/*:*/.venv/*:*/build/*:*/__pycache__/*:*/superset-frontend/*" + flask run -p $PORT --reload $DEBUGGER_FLAG --host=0.0.0.0 \ + --extra-files "/app/superset/extensions/.reload_trigger" \ + --exclude-patterns "*/node_modules/*:*/.venv/*:*/build/*:*/__pycache__/*:*/superset-frontend/*:*/superset/__init__.py" ;; app-gunicorn) echo "Starting web app..." diff --git a/superset/extensions/api.py b/superset/extensions/api.py index b1b5734979ee..8ed3129d34c6 100644 --- a/superset/extensions/api.py +++ b/superset/extensions/api.py @@ -169,7 +169,7 @@ def get(self, publisher: str, name: str, **kwargs: Any) -> Response: @protect() @safe - @expose("///", methods=("GET",)) + @expose("///", methods=("GET",)) def content(self, publisher: str, name: str, file: str) -> Response: """Get a frontend chunk of an extension. --- diff --git a/superset/extensions/cache_middleware.py b/superset/extensions/cache_middleware.py index e8a134052a00..c688bbe8d9fb 100644 --- a/superset/extensions/cache_middleware.py +++ b/superset/extensions/cache_middleware.py @@ -23,9 +23,11 @@ if TYPE_CHECKING: from _typeshed.wsgi import StartResponse, WSGIApplication, WSGIEnvironment -# Matches only the static asset endpoint: /api/v1/extensions/// +# Matches only the static asset endpoint: +# /api/v1/extensions///, where the file portion may +# contain nested segments (worker / WASM / chunk subfolders). # Does not match the list (/), get (//), or info (/_info) endpoints. -_ASSET_PATH_RE = re.compile(r"^/api/v1/extensions/[^/]+/[^/]+/[^/]+$") +_ASSET_PATH_RE: re.Pattern[str] = re.compile(r"^/api/v1/extensions/[^/]+/[^/]+/.+$") class ExtensionCacheMiddleware: diff --git a/superset/extensions/local_extensions_watcher.py b/superset/extensions/local_extensions_watcher.py index 6233f91fe5cf..a2908f941ac8 100644 --- a/superset/extensions/local_extensions_watcher.py +++ b/superset/extensions/local_extensions_watcher.py @@ -29,37 +29,207 @@ logger = logging.getLogger(__name__) +# Sentinel file Flask watches via --extra-files. Touching it on a real change +# triggers a server reload without depending on cwd or the location of any +# Python source file. +RELOAD_TRIGGER: Path = Path(__file__).resolve().parent / ".reload_trigger" + # Guard to prevent multiple initializations _watcher_initialized = False _watcher_lock = threading.Lock() -def _get_file_handler_class() -> Any: +def _get_file_handler_class() -> Any: # noqa: C901 """Get the file handler class, importing watchdog only when needed.""" try: - from watchdog.events import FileSystemEventHandler + import hashlib + + from watchdog.events import ( + FileCreatedEvent, + FileDeletedEvent, + FileModifiedEvent, + FileMovedEvent, + FileSystemEventHandler, + ) class LocalExtensionFileHandler(FileSystemEventHandler): - """Custom file system event handler for LOCAL_EXTENSIONS directories.""" + """Custom file system event handler for LOCAL_EXTENSIONS directories. + + Only reacts to genuine content changes (create / modify / move) in the + dist directory, verified by comparing a SHA-256 of the file's content. + This avoids the Docker VirtioFS / osxfs problem where reading a file + generates inotify events that watchdog surfaces as modifications. + """ + + def __init__(self) -> None: + super().__init__() + # sha256 of last-seen content, keyed by absolute path. Populated + # from existing files in watched `dist` dirs at startup (see + # `prime_baseline`) so that startup-noise inotify events from + # Docker VirtioFS reads don't get treated as the first real edit. + self._file_hashes: dict[str, str] = {} + self._lock: threading.Lock = threading.Lock() + # Trailing debounce: schedule a single reload after a quiet + # window so simultaneous webpack writes coalesce into one + # restart that fires *after* the build settles. + self._debounce_seconds: float = 1.0 + self._pending_timer: threading.Timer | None = None + # Monotonically increasing token identifying the most recently + # scheduled timer. Guards the timer-already-fired race where + # `Timer.cancel()` can't stop a callback that has begun running. + self._reload_generation: int = 0 + + # ── helpers ────────────────────────────────────────────────────── + + @staticmethod + def _sha256(path: str) -> str | None: + try: + with open(path, "rb") as fh: + return hashlib.sha256(fh.read()).hexdigest() + except OSError: + return None + + def prime_baseline(self, watch_dirs: set[str]) -> None: + """Pre-populate content hashes for existing files in watched + `dist` directories. Called once at watcher startup so a + developer's first real edit registers as a content change + rather than as the file's 'first observation'.""" + for root_dir in watch_dirs: + root = Path(root_dir) + for path in root.rglob("*"): + if not path.is_file(): + continue + if "dist" not in path.parts: + continue + digest = self._sha256(str(path)) + if digest is not None: + self._file_hashes[str(path)] = digest + + def _content_changed(self, path: str) -> bool: + """Return True when the file's content differs from last seen. + + With `prime_baseline` called at startup, the baseline reflects + what was on disk when the watcher started. A first observation + that differs (or doesn't exist in baseline) is treated as a + genuine change. + """ + digest = self._sha256(path) + if digest is None: + return False + old_digest = self._file_hashes.get(path) + self._file_hashes[path] = digest + # New file (not in baseline) is a real change; otherwise compare. + return old_digest != digest + + def _trigger_reload(self, source_path: str, generation: int) -> None: + """Touch the reload-trigger sentinel; Flask's --extra-files + watcher reloads on its mtime change.""" + # A newer event may have superseded this timer after it began + # running (`cancel()` can't stop an in-flight callback), so only + # the most recently scheduled generation is allowed to fire. The + # check and the sentinel touch happen inside the same critical + # section so a `_schedule_reload` call racing in from another + # thread can't bump the generation between the check and the + # write and let this stale callback still fire. + with self._lock: + if generation != self._reload_generation: + return + logger.info( + "File change settled in LOCAL_EXTENSIONS: %s", source_path + ) + logger.info("Triggering restart by touching %s", RELOAD_TRIGGER) + try: + os.utime(RELOAD_TRIGGER, (time.time(), time.time())) + except OSError as e: + logger.warning( + "Failed to touch reload trigger %s: %s", RELOAD_TRIGGER, e + ) + + def _schedule_reload(self, source_path: str) -> None: + """Trailing-debounce: cancel any pending reload and schedule a + new one for `_debounce_seconds` from now. Each new event resets + the timer, so the reload fires only after a quiet window.""" + with self._lock: + if self._pending_timer is not None: + self._pending_timer.cancel() + self._reload_generation += 1 + timer = threading.Timer( + self._debounce_seconds, + self._trigger_reload, + args=(source_path, self._reload_generation), + ) + timer.daemon = True + self._pending_timer = timer + timer.start() + + def _handle_moved(self, event: Any) -> None: + """Moves into/out of `dist` are explicit signals — trigger + regardless of content match (the source may already be gone + or the destination may not have a meaningful hash yet). + Atomic-build workflows rename tmp -> dist (dest in dist), + while removing an artifact renames dist -> elsewhere (src in + dist); either side touching `dist` is a real signal.""" + dest = getattr(event, "dest_path", None) + src = getattr(event, "src_path", None) + # The file no longer lives at the source path; evict its + # hash entry so the index only tracks paths that exist. + if isinstance(src, str): + self._file_hashes.pop(src, None) + dist_side = next( + ( + p + for p in (dest, src) + if isinstance(p, str) and "dist" in Path(p).parts + ), + None, + ) + if dist_side is not None: + self._schedule_reload(dist_side) + + # ── event handler ───────────────────────────────────────────────── def on_any_event(self, event: Any) -> None: - """Handle any file system event in the watched directories.""" + """Handle file system events in the watched directories.""" if event.is_directory: return - # Only trigger on changes to files in `dist` directory - src = getattr(event, "src_path", None) - if not isinstance(src, str) or "dist" not in Path(src).parts: + # Deletions don't trigger a reload (webpack clean steps delete + # old chunks right before writing new ones, which trigger via + # the subsequent create/modify), but the stale hash entry must + # be evicted so `_file_hashes` doesn't grow without bound as + # hashed chunk filenames churn across rebuilds. + if isinstance(event, FileDeletedEvent): + src = getattr(event, "src_path", None) + if isinstance(src, str): + self._file_hashes.pop(src, None) return - logger.info( - "File change detected in LOCAL_EXTENSIONS: %s", event.src_path - ) + # Only react to true write events; skip access / close / open etc. + if not isinstance( + event, (FileCreatedEvent, FileModifiedEvent, FileMovedEvent) + ): + return - # Touch superset/__init__.py to trigger Flask's file watcher - superset_init = Path("superset/__init__.py") - logger.info("Triggering restart by touching %s", superset_init) - os.utime(superset_init, (time.time(), time.time())) + if isinstance(event, FileMovedEvent): + self._handle_moved(event) + return + + # For Create/Modify events watchdog only sets src_path. + target = getattr(event, "src_path", None) + if not isinstance(target, str): + return + + # Only care about paths inside a `dist` directory. + if "dist" not in Path(target).parts: + return + + # For Create/Modify, verify the content actually changed to + # ignore spurious inotify events generated by Docker bind-mount + # reads. + if not self._content_changed(target): + return + + self._schedule_reload(target) return LocalExtensionFileHandler except ImportError: @@ -130,11 +300,23 @@ def setup_local_extensions_watcher(app: Flask) -> None: # noqa: C901 if not watch_dirs: return + # Ensure the sentinel exists so os.utime() and Flask's --extra-files watcher + # both have a real path to operate on. + try: + RELOAD_TRIGGER.touch(exist_ok=True) + except OSError as e: + logger.warning("Could not create reload trigger %s: %s", RELOAD_TRIGGER, e) + return + try: from watchdog.observers import Observer # Set up and start the file watcher event_handler = handler_class() + # Pre-populate baseline hashes from existing dist files so the + # developer's first real edit isn't silently dropped as a "first + # observation". + event_handler.prime_baseline(watch_dirs) observer = Observer() for watch_dir in watch_dirs: diff --git a/superset/extensions/utils.py b/superset/extensions/utils.py index fc5b6250bb6c..82f7b475d7fa 100644 --- a/superset/extensions/utils.py +++ b/superset/extensions/utils.py @@ -35,12 +35,17 @@ logger = logging.getLogger(__name__) -FRONTEND_REGEX = re.compile(r"^frontend/dist/([^/]+)$") +# Accept nested paths inside frontend/dist so extensions can serve +# worker / WASM / chunk subfolders. Reject any entry whose path contains "..", +# conservatively excluding parent traversal segments so a crafted entry name +# cannot escape the bundle directory (defense in depth; check_is_safe_zip runs +# first). +FRONTEND_REGEX: re.Pattern[str] = re.compile(r"^frontend/dist/(?!.*\.\.)(.+)$") # Reject any entry whose path contains "..", conservatively excluding parent # traversal segments along with the (in practice nonexistent) case of a module # path embedding consecutive dots, so a crafted entry name cannot produce a # traversal-style module path (defense in depth; check_is_safe_zip runs first). -BACKEND_REGEX = re.compile(r"^backend/src/(?!.*\.\.)(.+)$") +BACKEND_REGEX: re.Pattern[str] = re.compile(r"^backend/src/(?!.*\.\.)(.+)$") class InMemoryLoader(importlib.abc.Loader): diff --git a/tests/unit_tests/extensions/test_cache_middleware.py b/tests/unit_tests/extensions/test_cache_middleware.py index 22f8b5068200..e9398032d68c 100644 --- a/tests/unit_tests/extensions/test_cache_middleware.py +++ b/tests/unit_tests/extensions/test_cache_middleware.py @@ -64,6 +64,21 @@ def test_asset_path_is_intercepted() -> None: assert "Cookie" not in vary +def test_nested_asset_path_is_intercepted() -> None: + headers: ResponseHeaders = call_middleware( + "/api/v1/extensions/acme/my-ext/workers/nested/chunk.wasm", + [("Vary", "Accept-Encoding, Cookie")], + ) + vary: str = dict(headers).get("Vary", "") + assert "Cookie" not in vary + + +def test_get_endpoint_with_trailing_slash_is_not_intercepted() -> None: + upstream: ResponseHeaders = [("Vary", "Accept-Encoding, Cookie")] + headers = call_middleware("/api/v1/extensions/acme/my-ext/", upstream) + assert headers == upstream + + def test_list_endpoint_is_not_intercepted() -> None: upstream = [("Vary", "Accept-Encoding, Cookie")] headers = call_middleware("/api/v1/extensions/", upstream) diff --git a/tests/unit_tests/extensions/test_local_extensions_watcher.py b/tests/unit_tests/extensions/test_local_extensions_watcher.py new file mode 100644 index 000000000000..85f2a2bdaacd --- /dev/null +++ b/tests/unit_tests/extensions/test_local_extensions_watcher.py @@ -0,0 +1,59 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from typing import Any + +from watchdog.events import FileDeletedEvent, FileMovedEvent + +from superset.extensions.local_extensions_watcher import _get_file_handler_class + + +def _noop_schedule_reload(_path: str) -> None: + """Stand in for a real debounce timer in unit tests.""" + + +def make_handler() -> Any: + handler_class = _get_file_handler_class() + handler = handler_class() + # Avoid spinning up real debounce timers in unit tests. + handler._schedule_reload = _noop_schedule_reload + return handler + + +def test_delete_evicts_hash_entry() -> None: + handler = make_handler() + handler._file_hashes["/ext/dist/old-chunk.abc123.js"] = "digest" + + handler.on_any_event(FileDeletedEvent("/ext/dist/old-chunk.abc123.js")) + + assert "/ext/dist/old-chunk.abc123.js" not in handler._file_hashes + + +def test_delete_of_untracked_path_is_a_noop() -> None: + handler = make_handler() + + handler.on_any_event(FileDeletedEvent("/ext/dist/never-seen.js")) + + assert handler._file_hashes == {} + + +def test_move_out_of_dist_evicts_source_hash_entry() -> None: + handler = make_handler() + handler._file_hashes["/ext/dist/chunk.js"] = "digest" + + handler.on_any_event(FileMovedEvent("/ext/dist/chunk.js", "/ext/tmp/chunk.js")) + + assert "/ext/dist/chunk.js" not in handler._file_hashes