From 756458f031e335c29f56778fecbd39e969aa9015 Mon Sep 17 00:00:00 2001 From: Joe Li Date: Thu, 14 May 2026 09:51:25 -0700 Subject: [PATCH] fix(subdirectory): create-chart link, permalink doubling, and dead Superset.* routes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 follow-up to the TDD scaffold. Two user-visible bugs in QA on the local /superset deployment: - Empty dashboard-tab "create a new chart" link opened a 404 tab under /superset: raw with target="_blank" bypasses React Router's basename, so the new tab resolves the absolute path against the document origin without the application root. - "Copy permalink" produced /superset/superset/dashboard/p// on the clipboard. The same backend mechanism made the 18 routes on the `Superset` view class unreachable at request time under subdirectory deployment (404 for /superset/welcome/, /superset/dashboard//, /superset/explore/, etc.). Frontend: - Tab.tsx — wrap the create-chart href with `ensureAppRoot()` from navigationUtils so the application root is applied exactly once and the new tab lands at the right path. - New L2 invariant `RAW_HREF_ABSOLUTE_PATH_PATTERN` flags raw absolute-from-root anchors (`href="/..."`, `href={`/...`}`, etc.) — the bug class that bypasses both helpers and React Router basename. Seeded with 7 remaining violator files paired with a `toEqual([])` stale-allow-list check so each migration commit shrinks the list in lockstep. - Tab.subdirectory.test.tsx — empty / /superset / nested-root regression pins via a jest.mock pattern on applicationRoot(). Backend (breaking change documented in UPDATING.md): - Override `Superset.route_base = ""` so Flask-AppBuilder's auto-derived `/superset` prefix no longer compounds with the SCRIPT_NAME / basename that AppRootMiddleware sets. url_for now emits single-prefix URLs and the routes are reachable under both root and subdirectory deployments. - Pin the new shape with four unit tests covering Superset.dashboard_permalink (relative + external) and Superset.welcome, with and without SCRIPT_NAME. Co-Authored-By: Claude Opus 4.7 (1M context) --- UPDATING.md | 2 + .../Tab/Tab.subdirectory.test.tsx | 146 ++++++++++++++++++ .../components/gridComponents/Tab/Tab.tsx | 5 +- .../utils/navigationUtils.invariants.test.ts | 56 +++++++ superset/views/core.py | 10 ++ tests/unit_tests/test_subdirectory_url_for.py | 111 +++++++++++++ 6 files changed, 329 insertions(+), 1 deletion(-) create mode 100644 superset-frontend/src/dashboard/components/gridComponents/Tab/Tab.subdirectory.test.tsx create mode 100644 tests/unit_tests/test_subdirectory_url_for.py diff --git a/UPDATING.md b/UPDATING.md index 9558c297421..ecd3f70100f 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -26,6 +26,8 @@ assists people when migrating to a new version. - [39925](https://github.com/apache/superset/pull/39925): URL prefixing for `SUPERSET_APP_ROOT` subdirectory deployments is now handled automatically by helpers in `src/utils/navigationUtils` (`openInNewTab`, `redirect`, `getShareableUrl`, ``). Direct imports of `ensureAppRoot` / `makeUrl` from `src/utils/pathUtils` are forbidden outside `navigationUtils.ts` (enforced by a static-invariant test); contributors writing new code should use the focused helpers instead. No runtime behaviour change for existing callers — all 19 prior call sites have been migrated and four pre-existing double-prefix and missing-prefix bugs are fixed as part of the migration. +- **Breaking — `Superset` view class route prefix removed.** The `Superset` view in `superset/views/core.py` now declares `route_base = ""`, overriding Flask-AppBuilder's auto-derived `/superset` prefix. Routes that previously lived at `/superset/welcome/`, `/superset/dashboard//`, `/superset/dashboard/p//`, `/superset/explore/`, etc. now respond at `/welcome/`, `/dashboard//`, `/dashboard/p//`, `/explore/`, etc. Under subdirectory deployment (`SUPERSET_APP_ROOT=/superset`) the URLs are unchanged from end-user perspective — `AppRootMiddleware` re-applies the prefix via `SCRIPT_NAME`. Under root deployments, any external integration or bookmark that hard-codes `/superset//` paths must be updated to drop the prefix. This fixes the doubled `/superset/superset/...` URLs that `url_for` emitted for these endpoints under subdirectory deployment and the related 404s on the routes themselves. + ### Granular Export Controls A new feature flag `GRANULAR_EXPORT_CONTROLS` introduces three fine-grained permissions that replace the legacy `can_csv` permission: diff --git a/superset-frontend/src/dashboard/components/gridComponents/Tab/Tab.subdirectory.test.tsx b/superset-frontend/src/dashboard/components/gridComponents/Tab/Tab.subdirectory.test.tsx new file mode 100644 index 00000000000..c26c807dd5e --- /dev/null +++ b/superset-frontend/src/dashboard/components/gridComponents/Tab/Tab.subdirectory.test.tsx @@ -0,0 +1,146 @@ +/** + * 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 type { FC } from 'react'; +import { render, screen } from 'spec/helpers/testing-library'; + +// Tab.tsx is statically imported below; the mock pattern intercepts +// applicationRoot() rather than relying on withApplicationRoot (which is for +// dynamic-import unit tests). + +const mockApplicationRoot = jest.fn(() => ''); + +jest.mock('src/utils/getBootstrapData', () => { + const actual = jest.requireActual< + typeof import('src/utils/getBootstrapData') + >('src/utils/getBootstrapData'); + return { + __esModule: true, + default: actual.default, + applicationRoot: () => mockApplicationRoot(), + staticAssetsPrefix: actual.staticAssetsPrefix, + }; +}); + +jest.mock('src/dashboard/util/getChartIdsFromComponent', () => + jest.fn(() => []), +); +jest.mock('src/dashboard/containers/DashboardComponent', () => + jest.fn(() =>
), +); +jest.mock('@superset-ui/core/components/EditableTitle', () => ({ + __esModule: true, + EditableTitle: jest.fn(() =>
), +})); +jest.mock('src/dashboard/components/dnd/DragDroppable', () => ({ + ...jest.requireActual('src/dashboard/components/dnd/DragDroppable'), + Droppable: jest.fn(props => ( +
{props.children ? props.children({}) : null}
+ )), +})); + +// eslint-disable-next-line import/first +import ActualTab from './Tab'; + +const Tab = ActualTab as unknown as FC>; + +const DASHBOARD_ID = 23; + +const buildProps = () => ({ + id: 'TAB-empty-', + parentId: 'TABS-empty-', + depth: 2, + index: 0, + renderType: 'RENDER_TAB_CONTENT', + availableColumnCount: 12, + columnWidth: 120, + isFocused: false, + component: { + children: [], + id: 'TAB-empty-', + meta: { text: 'Empty Tab' }, + parents: ['ROOT_ID', 'GRID_ID', 'TABS-empty-'], + type: 'TAB', + }, + parentComponent: { + children: ['TAB-empty-'], + id: 'TABS-empty-', + meta: {}, + parents: ['ROOT_ID', 'GRID_ID'], + type: 'TABS', + }, + editMode: true, + embeddedMode: false, + undoLength: 0, + redoLength: 0, + filters: {}, + directPathToChild: [], + directPathLastUpdated: 0, + dashboardId: DASHBOARD_ID, + focusedFilterScope: null, + isComponentVisible: true, + onDropOnTab: jest.fn(), + handleComponentDrop: jest.fn(), + updateComponents: jest.fn(), + setDirectPathToChild: jest.fn(), + onResizeStart: jest.fn(), + onResize: jest.fn(), + onResizeStop: jest.fn(), +}); + +const renderEmptyEditModeTab = () => + render(, { + useRedux: true, + useDnd: true, + initialState: { + dashboardInfo: { dash_edit_perm: true }, + }, + }); + +beforeEach(() => { + mockApplicationRoot.mockReturnValue(''); +}); + +test('Tab — empty edit-mode "create a new chart" link is unprefixed when application root is empty', () => { + mockApplicationRoot.mockReturnValue(''); + renderEmptyEditModeTab(); + + expect( + screen.getByRole('link', { name: 'create a new chart' }), + ).toHaveAttribute('href', `/chart/add?dashboard_id=${DASHBOARD_ID}`); +}); + +test('Tab — empty edit-mode "create a new chart" link carries the application root under subdirectory deployment', () => { + mockApplicationRoot.mockReturnValue('/superset'); + renderEmptyEditModeTab(); + + // Single prefix — not /superset/superset/ — verifying ensureAppRoot's + // dedupe boundary holds against the path's leading slash. + expect( + screen.getByRole('link', { name: 'create a new chart' }), + ).toHaveAttribute('href', `/superset/chart/add?dashboard_id=${DASHBOARD_ID}`); +}); + +test('Tab — empty edit-mode "create a new chart" link prefixes correctly for nested subdirectory roots', () => { + mockApplicationRoot.mockReturnValue('/a/b/c'); + renderEmptyEditModeTab(); + + expect( + screen.getByRole('link', { name: 'create a new chart' }), + ).toHaveAttribute('href', `/a/b/c/chart/add?dashboard_id=${DASHBOARD_ID}`); +}); diff --git a/superset-frontend/src/dashboard/components/gridComponents/Tab/Tab.tsx b/superset-frontend/src/dashboard/components/gridComponents/Tab/Tab.tsx index f2486a80dc8..9bdbf9221c2 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Tab/Tab.tsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Tab/Tab.tsx @@ -36,6 +36,7 @@ import getChartIdsFromComponent from 'src/dashboard/util/getChartIdsFromComponen import DashboardComponent from 'src/dashboard/containers/DashboardComponent'; import AnchorLink from 'src/dashboard/components/AnchorLink'; import { Typography } from '@superset-ui/core/components/Typography'; +import { ensureAppRoot } from 'src/utils/navigationUtils'; import { useIsAutoRefreshing, useIsRefreshInFlight, @@ -333,7 +334,9 @@ const Tab = (props: TabProps): ReactElement => { {t('You can')}{' '} diff --git a/superset-frontend/src/utils/navigationUtils.invariants.test.ts b/superset-frontend/src/utils/navigationUtils.invariants.test.ts index d4946aef4d4..ae41e4f818f 100644 --- a/superset-frontend/src/utils/navigationUtils.invariants.test.ts +++ b/superset-frontend/src/utils/navigationUtils.invariants.test.ts @@ -36,3 +36,59 @@ test('no file outside navigationUtils.ts imports from pathUtils', () => { 'getShareableUrl, AppLink) for most call sites.', ); }); + +// Raw absolute-from-root anchor hrefs (`href="/foo"`, `href={`/foo`}`, etc.) +// bypass React Router's basename when rendered with `target="_blank"` and +// produce 404s under subdirectory deployment. Migrate to either +// `` or wrap with `ensureAppRoot()` so the application +// root is applied exactly once. +// +// The negative lookbehind `(? { + const hits = scanSource({ + pattern: RAW_HREF_ABSOLUTE_PATH_PATTERN, + allowlist: RAW_HREF_ABSOLUTE_PATH_ALLOWLIST, + }); + + expectNoHits( + hits, + 'Found raw absolute-path href anchors. With target="_blank" these ' + + "bypass React Router's basename and 404 under /superset deployment. " + + 'Migrate to from src/utils/navigationUtils, or ' + + 'wrap the value with ensureAppRoot().', + ); +}); + +test('RAW_HREF_ABSOLUTE_PATH_ALLOWLIST has no stale entries', () => { + // Every allow-listed file must still contain at least one raw-href + // violation. When a migration commit removes the last violation from a + // file, that file must be dropped from the allow-list in the same commit + // — otherwise the scan silently stops protecting the rest of the tree + // from regressions in that file. + const hitFiles = new Set( + scanSource({ pattern: RAW_HREF_ABSOLUTE_PATH_PATTERN }).map( + hit => hit.file, + ), + ); + + const stale = RAW_HREF_ABSOLUTE_PATH_ALLOWLIST.filter( + file => !hitFiles.has(file), + ); + + expect(stale).toEqual([]); +}); diff --git a/superset/views/core.py b/superset/views/core.py index c4c16cb65fd..de357d9e119 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -134,6 +134,16 @@ SqlResults = dict[str, Any] class Superset(BaseSupersetView): """The base views for Superset!""" + # Flask-AppBuilder's BaseView auto-derives `route_base` from the class + # name, which would mount every @expose route under `/superset/...`. That + # collides catastrophically with subdirectory deployments: AppRootMiddle- + # ware strips the configured root from PATH_INFO and sets SCRIPT_NAME, so + # url_for emits `/superset/superset/...` (doubled), and the in-rule + # `/superset` prefix no longer matches the post-strip PATH_INFO — making + # the routes themselves unreachable. Mount at the root so the application + # root applies exactly once via SCRIPT_NAME / basename. + route_base = "" + logger = logging.getLogger(__name__) @has_access diff --git a/tests/unit_tests/test_subdirectory_url_for.py b/tests/unit_tests/test_subdirectory_url_for.py new file mode 100644 index 00000000000..3fb1350ba7d --- /dev/null +++ b/tests/unit_tests/test_subdirectory_url_for.py @@ -0,0 +1,111 @@ +# 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. +"""url_for emission for Superset.* endpoints under subdirectory deployments. + +Flask-AppBuilder's BaseView auto-derives `route_base` from the class name, +which used to mount every `@expose` route on `Superset` under `/superset/...`. +Combined with `AppRootMiddleware` stripping `/superset` from `PATH_INFO` and +setting `SCRIPT_NAME=/superset`, werkzeug's `MapAdapter.build` produced +doubled URLs (`/superset/superset/...`) on every `url_for(..., _external=True)` +call into that namespace — and the routes themselves became unreachable at +request time because the in-rule `/superset` prefix no longer matched the +post-strip PATH_INFO. + +`Superset.route_base = ""` mounts the routes at the root so the appRoot +applies exactly once (via SCRIPT_NAME / basename). These tests pin both +branches: no SCRIPT_NAME and SCRIPT_NAME=`/superset`. +""" + +from flask import url_for + + +def test_dashboard_permalink_url_has_no_route_prefix_without_script_name( + app_context: None, +) -> None: + """Under root deployment, the permalink route lives at /dashboard/p//. + + The auto-derived `/superset` prefix on the `Superset` view class is gone; + the route is now mounted at the root path so url_for returns a single, + prefix-free URL. + """ + from flask import current_app + + with current_app.test_request_context("/"): + url = url_for("Superset.dashboard_permalink", key="abc123") + assert url == "/dashboard/p/abc123/" + + +def test_dashboard_permalink_url_carries_single_script_name_prefix( + app_context: None, +) -> None: + """Under subdirectory deployment, url_for emits exactly one prefix. + + AppRootMiddleware sets SCRIPT_NAME=/superset on every inbound request + once APPLICATION_ROOT is configured. url_for prepends SCRIPT_NAME to the + rule, so the emitted URL is `/superset/dashboard/p//` — a single + prefix, not the previous `/superset/superset/dashboard/p//`. + """ + from flask import current_app + + with current_app.test_request_context( + "/", + environ_overrides={"SCRIPT_NAME": "/superset"}, + ): + url = url_for("Superset.dashboard_permalink", key="abc123") + assert url == "/superset/dashboard/p/abc123/" + + +def test_welcome_url_carries_single_script_name_prefix( + app_context: None, +) -> None: + """Spot-check a second route to confirm the fix is not endpoint-specific. + + The `brand.path` regression in the QA findings traced to + `url_for("Superset.welcome", _external=True)` returning the doubled + `/superset/superset/welcome/`. Pinning a single-prefix expectation for + welcome guards against a regression that reintroduces the auto-derived + route_base on the Superset class. + """ + from flask import current_app + + with current_app.test_request_context( + "/", + environ_overrides={"SCRIPT_NAME": "/superset"}, + ): + url = url_for("Superset.welcome") + assert url == "/superset/welcome/" + + +def test_dashboard_permalink_external_url_is_single_prefixed( + app_context: None, +) -> None: + """`url_for(..., _external=True)` is the shape the permalink API serves. + + Pin the external (scheme://host included) variant explicitly — that is + the value that ends up on the user's clipboard via + `superset/dashboards/permalink/api.py` and must carry one application + root segment, not two. + """ + from flask import current_app + + with current_app.test_request_context( + "/", + environ_overrides={"SCRIPT_NAME": "/superset"}, + ): + url = url_for("Superset.dashboard_permalink", key="abc123", _external=True) + assert url.endswith("/superset/dashboard/p/abc123/") + assert "/superset/superset/" not in url