mirror of
https://github.com/apache/superset.git
synced 2026-07-12 01:35:36 +00:00
Closes the 404 gap for legacy `/superset/<canonical>` bookmarks after `Superset.route_base = ""` collapsed the historical `/superset` prefix off every view. Adds an outermost WSGI shim that 308-redirects enumerated legacy paths to their canonical equivalents (or 410s POSTs against GET-only canonicals, where a 308 would have the client retry-POST into a 405). Layering invariant ------------------ The shim wraps `app.wsgi_app` in `create_app()` *after* `init_app()` returns, so it sits outside AppRootMiddleware / ProxyFix / ChunkedEncodingFix / ADDITIONAL_MIDDLEWARE and sees the raw inbound `PATH_INFO`. `Location` is built from the `app_root` captured at construction time + the canonical path — never from `environ["SCRIPT_NAME"]` (unset at this outer layer) and never from `X-Forwarded-*`. See module docstring + PLAN.md "WSGI layering invariant" for the full ordering rationale. AF-4 coupling closure --------------------- `SqlaTable.sql_url` previously used naive `?table_name=…` concatenation, which produced double-`?` URLs when `Database.sql_url` already carried a query string (Database.sql_url was reshaped in Slice 3 to `/sqllab/?dbid=<id>`). Switch to `urlsplit` / `parse_qsl` / `urlencode` with `quote_via=quote` so query strings compose correctly and names with special characters (`/`, `&`, `+`, …) are properly encoded. Closed-set discipline --------------------- `LEGACY_REDIRECT_MAP` is a 16-row closed table, each row verified against the canonical endpoint's `@expose` decorator at HEAD. `/superset/sql/<id>/` is intentionally absent — `Database.sql_url` changed shape (path → query string) so no 1:1 mapping exists; the hard re-bookmark break is documented in UPDATING.md. A snapshot regression test pins the keyset. Oracle results (all RED-against-HEAD, reverted) ----------------------------------------------- 5-a remove shim wrap-site in app.py → 17 GETs fail 5-b flip `path_info.startswith` → exact-only → tail-bearing rows red 5-c drop allowed-methods gate → POST-410 rows red 5-d emit 302 instead of 308 → status assertion red 5-e read SCRIPT_NAME instead of captured app_root → contamination guard red 5-f read X-Forwarded-Prefix → contamination guard red 5-g `_LEGACY_PREFIX = ""` → pass-through guard red 5-h drop query-string passthrough → QS preservation red 5-i swap exact/longest-prefix order in _match → /dashboard/p/ row red 5-j AF-4: revert sql_url to naive concat → double-`?` test red Round-3/4/6 review closures --------------------------- * round-3 [High] prefix-source closure: `app_root_prefix` captured once at construction; never re-read from environ. * round-4 [High] wrap-site closure: shim installed at the single sanctioned outermost wrap site in `create_app()`; pinned by `test_wrap_order_*`. * round-6 `quote(safe=…)` pin: `Location` is %-encoded with a safe set that preserves header-safe URL bytes while sanitising raw control bytes from `PATH_INFO`. * round-6 outermost-only-within-`create_app()` invariant: unconditional wrap (independent of `app_root != "/"`) — legacy bookmarks exist under root deployments too. * H2 closure: 410 (not 308) for POST-against-GET-only avoids the 308→retry → 405 trap. Tests ----- Test file lives at `tests/unit_tests/middleware/` (not `integration_tests/` per PLAN) because the local docker-light env can't service `/login/` POST. Coverage is preserved via `werkzeug.test.Client` driving the middleware around a sentinel inner WSGI app, plus `create_app()` + patched `init_app()` for wrap-order assertions, plus live-route shadow pins. * `tests/unit_tests/middleware/test_legacy_prefix_redirect.py` — 72 tests (path rewriter, query-string preservation, SCRIPT_NAME/X-Forwarded contamination guards, closed-set snapshot, wrap-order, live-route shadow pins, AF-4 query encoding). * `tests/unit_tests/initialization_test.py` — `_unwrap_to_app_root` helper threads the new outermost layer so existing `TestCreateAppRoot` cases keep passing. UPDATING.md ----------- Documents (a) legacy `/superset/*` 308 shim with EOL at 5.0.0, (b) hard re-bookmark break for `/superset/sql/<database_id>/`, (c) `SqlaTable.sql_url` query-string format change (AF-4 fix). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
278 lines
11 KiB
Python
278 lines
11 KiB
Python
# 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.
|
|
|
|
import os
|
|
from unittest.mock import MagicMock, patch
|
|
|
|
from sqlalchemy.exc import OperationalError
|
|
|
|
from superset.app import AppRootMiddleware, create_app, SupersetApp
|
|
from superset.initialization import SupersetAppInitializer
|
|
from superset.middleware.legacy_prefix_redirect import LegacyPrefixRedirectMiddleware
|
|
|
|
|
|
def _unwrap_to_app_root(app):
|
|
"""Walk the WSGI middleware chain past the outermost
|
|
`LegacyPrefixRedirectMiddleware` (always installed) and return the
|
|
next layer. Lets the existing AppRootMiddleware-shape assertions
|
|
survive the Slice-5 outer-wrap change."""
|
|
assert isinstance(app.wsgi_app, LegacyPrefixRedirectMiddleware)
|
|
return app.wsgi_app.wsgi_app
|
|
|
|
|
|
class TestSupersetApp:
|
|
@patch("superset.app.logger")
|
|
def test_sync_config_to_db_skips_when_no_tables(self, mock_logger):
|
|
"""Test that sync is skipped when database is not up-to-date."""
|
|
# Setup
|
|
app = SupersetApp(__name__)
|
|
app.config = {"SQLALCHEMY_DATABASE_URI": "postgresql://user:pass@host:5432/db"}
|
|
|
|
# Mock _is_database_up_to_date to return False
|
|
with patch.object(app, "_is_database_up_to_date", return_value=False):
|
|
# Execute
|
|
app.sync_config_to_db()
|
|
|
|
# Assert
|
|
mock_logger.info.assert_called_once_with(
|
|
"Pending database migrations: run 'superset db upgrade'"
|
|
)
|
|
|
|
@patch("superset.extensions.db")
|
|
@patch("superset.app.logger")
|
|
def test_sync_config_to_db_handles_operational_error(self, mock_logger, mock_db):
|
|
"""Test that OperationalError during migration check is handled gracefully."""
|
|
# Setup
|
|
app = SupersetApp(__name__)
|
|
app.config = {"SQLALCHEMY_DATABASE_URI": "postgresql://user:pass@host:5432/db"}
|
|
error_msg = "Cannot connect to database"
|
|
|
|
# Mock db.engine.connect to raise an OperationalError
|
|
mock_db.engine.connect.side_effect = OperationalError(error_msg, None, None)
|
|
|
|
# Execute
|
|
app.sync_config_to_db()
|
|
|
|
# Assert - _is_database_up_to_date should catch the error and return False
|
|
# which causes the info log about pending migrations
|
|
mock_logger.info.assert_called_once_with(
|
|
"Pending database migrations: run 'superset db upgrade'"
|
|
)
|
|
|
|
@patch("superset.extensions.feature_flag_manager")
|
|
@patch("superset.app.logger")
|
|
@patch("superset.commands.theme.seed.SeedSystemThemesCommand")
|
|
def test_sync_config_to_db_initializes_when_tables_exist(
|
|
self,
|
|
mock_seed_themes_command,
|
|
mock_logger,
|
|
mock_feature_flag_manager,
|
|
):
|
|
"""Test that features are initialized when database is up-to-date."""
|
|
# Setup
|
|
app = SupersetApp(__name__)
|
|
app.config = {"SQLALCHEMY_DATABASE_URI": "postgresql://user:pass@host:5432/db"}
|
|
mock_feature_flag_manager.is_feature_enabled.return_value = True
|
|
mock_seed_themes = MagicMock()
|
|
mock_seed_themes_command.return_value = mock_seed_themes
|
|
|
|
# Mock _is_database_up_to_date to return True
|
|
with (
|
|
patch.object(app, "_is_database_up_to_date", return_value=True),
|
|
patch(
|
|
"superset.tags.core.register_sqla_event_listeners"
|
|
) as mock_register_listeners,
|
|
):
|
|
# Execute
|
|
app.sync_config_to_db()
|
|
|
|
# Assert
|
|
mock_feature_flag_manager.is_feature_enabled.assert_called_with(
|
|
"TAGGING_SYSTEM"
|
|
)
|
|
mock_register_listeners.assert_called_once()
|
|
# Should seed themes
|
|
mock_seed_themes_command.assert_called_once()
|
|
mock_seed_themes.run.assert_called_once()
|
|
# Should log successful completion
|
|
mock_logger.info.assert_any_call("Syncing configuration to database...")
|
|
mock_logger.info.assert_any_call(
|
|
"Configuration sync to database completed successfully"
|
|
)
|
|
|
|
|
|
class TestSupersetAppInitializer:
|
|
@patch("superset.initialization.logger")
|
|
def test_init_app_in_ctx_calls_sync_config_to_db(self, mock_logger):
|
|
"""Test that initialization calls app.sync_config_to_db()."""
|
|
# Setup
|
|
mock_app = MagicMock()
|
|
mock_app.config = {
|
|
"SQLALCHEMY_DATABASE_URI": "postgresql://user:pass@host:5432/db",
|
|
"FLASK_APP_MUTATOR": None,
|
|
}
|
|
app_initializer = SupersetAppInitializer(mock_app)
|
|
|
|
# Execute init_app_in_ctx which calls sync_config_to_db
|
|
with (
|
|
patch.object(app_initializer, "configure_fab"),
|
|
patch.object(app_initializer, "configure_url_map_converters"),
|
|
patch.object(app_initializer, "configure_data_sources"),
|
|
patch.object(app_initializer, "configure_auth_provider"),
|
|
patch.object(app_initializer, "configure_async_queries"),
|
|
patch.object(app_initializer, "configure_ssh_manager"),
|
|
patch.object(app_initializer, "configure_stats_manager"),
|
|
patch.object(app_initializer, "init_views"),
|
|
):
|
|
app_initializer.init_app_in_ctx()
|
|
|
|
# Assert that sync_config_to_db was called on the app
|
|
mock_app.sync_config_to_db.assert_called_once()
|
|
|
|
def test_database_uri_lazy_property(self):
|
|
"""Test database_uri property uses lazy initialization with smart caching."""
|
|
# Setup
|
|
mock_app = MagicMock()
|
|
test_uri = "postgresql://user:pass@host:5432/testdb"
|
|
mock_app.config = {"SQLALCHEMY_DATABASE_URI": test_uri}
|
|
app_initializer = SupersetAppInitializer(mock_app)
|
|
|
|
# Ensure cache is None initially
|
|
assert app_initializer._db_uri_cache is None
|
|
|
|
# First access should set the cache (valid URI)
|
|
uri = app_initializer.database_uri
|
|
assert uri == test_uri
|
|
assert app_initializer._db_uri_cache is not None
|
|
assert app_initializer._db_uri_cache == test_uri
|
|
|
|
# Second access should use cache (not call config.get again)
|
|
# Change the config to verify cache is being used
|
|
mock_app.config["SQLALCHEMY_DATABASE_URI"] = "different_uri"
|
|
uri2 = app_initializer.database_uri
|
|
assert (
|
|
uri2 == test_uri
|
|
) # Should still return cached value (not "different_uri")
|
|
|
|
def test_database_uri_doesnt_cache_fallback_values(self):
|
|
"""Test that fallback values like 'nouser' are not cached."""
|
|
# Setup
|
|
mock_app = MagicMock()
|
|
|
|
# Initially return the fallback nouser URI
|
|
config_dict = {
|
|
"SQLALCHEMY_DATABASE_URI": "postgresql://nouser:nopassword@nohost:5432/nodb"
|
|
}
|
|
mock_app.config = config_dict
|
|
app_initializer = SupersetAppInitializer(mock_app)
|
|
|
|
# First access returns fallback but shouldn't cache it
|
|
uri1 = app_initializer.database_uri
|
|
assert uri1 == "postgresql://nouser:nopassword@nohost:5432/nodb"
|
|
assert app_initializer._db_uri_cache is None # Should NOT be cached
|
|
|
|
# Now config is properly loaded - update the same dict
|
|
config_dict["SQLALCHEMY_DATABASE_URI"] = (
|
|
"postgresql://realuser:realpass@realhost:5432/realdb"
|
|
)
|
|
|
|
# Second access should get the new value since fallback wasn't cached
|
|
uri2 = app_initializer.database_uri
|
|
assert uri2 == "postgresql://realuser:realpass@realhost:5432/realdb"
|
|
assert app_initializer._db_uri_cache is not None # Now it should be cached
|
|
assert (
|
|
app_initializer._db_uri_cache
|
|
== "postgresql://realuser:realpass@realhost:5432/realdb"
|
|
)
|
|
|
|
|
|
class TestCreateAppRoot:
|
|
"""Test app root resolution precedence in create_app."""
|
|
|
|
@patch("superset.initialization.SupersetAppInitializer.init_app")
|
|
def test_default_app_root_no_middleware(self, mock_init_app):
|
|
"""No param, no config, no env var: app_root is '/', no middleware."""
|
|
env = os.environ.copy()
|
|
env.pop("SUPERSET_APP_ROOT", None)
|
|
env.pop("SUPERSET_CONFIG", None)
|
|
with patch.dict(os.environ, env, clear=True):
|
|
app = create_app()
|
|
|
|
# The outermost `LegacyPrefixRedirectMiddleware` is now always
|
|
# installed (Slice 5). Under root deployment, the next layer
|
|
# should NOT be `AppRootMiddleware`.
|
|
inner = _unwrap_to_app_root(app)
|
|
assert not isinstance(inner, AppRootMiddleware)
|
|
|
|
@patch("superset.initialization.SupersetAppInitializer.init_app")
|
|
def test_application_root_config_activates_middleware(self, mock_init_app):
|
|
"""APPLICATION_ROOT in config activates AppRootMiddleware."""
|
|
env = os.environ.copy()
|
|
env.pop("SUPERSET_APP_ROOT", None)
|
|
env.pop("SUPERSET_CONFIG", None)
|
|
with (
|
|
patch.dict(os.environ, env, clear=True),
|
|
patch("superset.config.APPLICATION_ROOT", "/from-config", create=True),
|
|
):
|
|
app = create_app()
|
|
|
|
inner = _unwrap_to_app_root(app)
|
|
assert isinstance(inner, AppRootMiddleware)
|
|
assert inner.app_root == "/from-config"
|
|
|
|
@patch("superset.initialization.SupersetAppInitializer.init_app")
|
|
def test_env_var_activates_middleware(self, mock_init_app):
|
|
"""SUPERSET_APP_ROOT env var activates AppRootMiddleware."""
|
|
env = os.environ.copy()
|
|
env.pop("SUPERSET_CONFIG", None)
|
|
env["SUPERSET_APP_ROOT"] = "/from-env"
|
|
with patch.dict(os.environ, env, clear=True):
|
|
app = create_app()
|
|
|
|
inner = _unwrap_to_app_root(app)
|
|
assert isinstance(inner, AppRootMiddleware)
|
|
assert inner.app_root == "/from-env"
|
|
|
|
@patch("superset.initialization.SupersetAppInitializer.init_app")
|
|
def test_env_var_takes_precedence_over_config(self, mock_init_app):
|
|
"""SUPERSET_APP_ROOT env var wins over APPLICATION_ROOT config."""
|
|
env = os.environ.copy()
|
|
env.pop("SUPERSET_CONFIG", None)
|
|
env["SUPERSET_APP_ROOT"] = "/from-env"
|
|
with (
|
|
patch.dict(os.environ, env, clear=True),
|
|
patch("superset.config.APPLICATION_ROOT", "/from-config", create=True),
|
|
):
|
|
app = create_app()
|
|
|
|
inner = _unwrap_to_app_root(app)
|
|
assert isinstance(inner, AppRootMiddleware)
|
|
assert inner.app_root == "/from-env"
|
|
|
|
@patch("superset.initialization.SupersetAppInitializer.init_app")
|
|
def test_param_takes_precedence_over_env_var(self, mock_init_app):
|
|
"""superset_app_root param wins over SUPERSET_APP_ROOT env var."""
|
|
env = os.environ.copy()
|
|
env.pop("SUPERSET_CONFIG", None)
|
|
env["SUPERSET_APP_ROOT"] = "/from-env"
|
|
with patch.dict(os.environ, env, clear=True):
|
|
app = create_app(superset_app_root="/from-param")
|
|
|
|
inner = _unwrap_to_app_root(app)
|
|
assert isinstance(inner, AppRootMiddleware)
|
|
assert inner.app_root == "/from-param"
|