From 13f56f710e961165c9d92f8b588bea2ee4d39c7a Mon Sep 17 00:00:00 2001 From: Joe Li Date: Wed, 6 May 2026 17:30:58 -0700 Subject: [PATCH] test(subdirectory): scaffold red/green tests for application root URL helpers Skeleton commit for the subdirectory deployment refactor. Adds the test framework and one example test per layer; the helpers themselves are stubbed so the suite is meaningfully red until the green commit lands. Frameworks - spec/helpers/withApplicationRoot.ts: fixture that rewrites #app data and resets the module cache so getBootstrapData() returns the requested application root inside the callback. Replaces the inline ritual that pathUtils.test.ts currently repeats per test. - spec/helpers/sourceTreeScanner.ts: line-by-line regex scanner over the source tree with allow-list support. Backs the static-invariant tests in Layer 2 with workspace-relative file:line locations on failure. Stubs - src/utils/navigationUtils.ts: openInNewTab, redirect, redirectReplace, getShareableUrl, AppLink. Each throws a "not implemented" error with a doc comment describing the channel rule it enforces. Existing navigateTo / navigateWithState are kept untouched and called out as legacy multi-mode helpers scheduled for replacement. - packages/superset-ui-core/src/connection/normalizeBackendUrls.ts: conservative URL field normaliser. Ships the curated NORMALIZED_URL_FIELDS set (initially empty pending per-endpoint audit) and a documented NORMALIZER_EXCLUSIONS list explaining why bug_report_url, thumbnail_url, user_login_url, etc. are deliberately not normalised. Layered tests (one example each; full suite expands per layer in subsequent commits on this PR) - Layer 1 unit: navigationUtils.test.ts exercises openInNewTab under empty / single / nested application roots, plus absolute-URL and mailto passthrough. Red until the helper is implemented. - Layer 2 invariant: navigationUtils.invariants.test.ts asserts that ensureAppRoot / makeUrl are not imported outside navigationUtils.ts. Allow-list seeded with the 19 current call sites so the test is GREEN on day one; migration commits delete entries from the list. - Layer 3 normaliser: normalizeBackendUrls.test.ts pairs a positive strip case with negative passthrough cases (non-allow-listed field, absolute URL, similar-but-different prefix segment, empty root). Red until the normaliser is implemented. - Layer 4 contract: SupersetClientAppRootContract.test.ts pins the channel-2 invariant (root applied exactly once, never doubled). Documents the double-prefix symptom in a regression assertion. - Layer 5 regression: SliceHeaderControls.subdirectory.test.tsx asserts Cmd-click "Edit chart" opens a prefixed URL when the app is deployed under a subdirectory. Red until index.tsx:266 is migrated to openInNewTab. Strategy: each subsequent commit on this PR fans out one layer to its full coverage and migrates the corresponding call sites, shrinking the Layer 2 allow-list in lockstep. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/connection/normalizeBackendUrls.ts | 133 +++++++++++ .../SupersetClientAppRootContract.test.ts | 87 +++++++ .../connection/normalizeBackendUrls.test.ts | 114 +++++++++ .../spec/helpers/sourceTreeScanner.ts | 216 ++++++++++++++++++ .../spec/helpers/withApplicationRoot.ts | 91 ++++++++ .../SliceHeaderControls.subdirectory.test.tsx | 162 +++++++++++++ .../utils/navigationUtils.invariants.test.ts | 91 ++++++++ .../src/utils/navigationUtils.test.ts | 115 ++++++++++ .../src/utils/navigationUtils.ts | 85 +++++++ 9 files changed, 1094 insertions(+) create mode 100644 superset-frontend/packages/superset-ui-core/src/connection/normalizeBackendUrls.ts create mode 100644 superset-frontend/packages/superset-ui-core/test/connection/SupersetClientAppRootContract.test.ts create mode 100644 superset-frontend/packages/superset-ui-core/test/connection/normalizeBackendUrls.test.ts create mode 100644 superset-frontend/spec/helpers/sourceTreeScanner.ts create mode 100644 superset-frontend/spec/helpers/withApplicationRoot.ts create mode 100644 superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.subdirectory.test.tsx create mode 100644 superset-frontend/src/utils/navigationUtils.invariants.test.ts create mode 100644 superset-frontend/src/utils/navigationUtils.test.ts diff --git a/superset-frontend/packages/superset-ui-core/src/connection/normalizeBackendUrls.ts b/superset-frontend/packages/superset-ui-core/src/connection/normalizeBackendUrls.ts new file mode 100644 index 00000000000..23499597800 --- /dev/null +++ b/superset-frontend/packages/superset-ui-core/src/connection/normalizeBackendUrls.ts @@ -0,0 +1,133 @@ +/** + * 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. + */ + +/** + * Normalises backend-supplied URL fields so the frontend speaks one shape + * (router-relative paths) regardless of whether Superset is deployed at the + * web root or under a subdirectory. + * + * The backend renders absolute paths that include the application root, e.g. + * `/superset/explore/?slice_id=1`. Channel-3 helpers (window.open, redirect, + * AppLink) and channel-2 (`SupersetClient`) re-apply the root themselves; + * leaving the prefix on a backend value would double it. So we strip the + * configured root on the way in and let the consumers re-add it. + * + * # Why this is conservative by design + * + * The normaliser **only touches fields whose name appears in + * `NORMALIZED_URL_FIELDS`**. It does not heuristically detect URLs by content + * — a `description` field containing `/looks/like/a/path` is left alone. + * Adding a new URL field to the backend therefore requires an explicit + * one-line change here. Drift requires intentional opt-in. + * + * Exact-segment prefix matching prevents false positives where a value + * happens to share a prefix with the application root (e.g. + * `/superset-public/...` is not stripped when the root is `/superset`). + * + * Absolute URLs (`https://...`, `mailto:`, protocol-relative `//cdn`) and + * already-router-relative paths are passed through unchanged. + */ + +const NOT_IMPLEMENTED = + 'normalizeBackendUrls is not implemented yet — landing in the green commit of the subdirectory-helpers PR.'; + +/** + * Field names whose values are router-relative URLs to this Superset + * deployment and therefore safe to normalise. + * + * Curated, not heuristic. Add a field here only after confirming: + * + * 1. The backend always sets it to a path within this Superset instance + * (never an external URL or a path with a different prefix). + * 2. Every consumer expects to feed the value to a channel-3 helper or + * `SupersetClient`, both of which re-apply the application root. + * + * Fields that have been *deliberately excluded* are listed in + * `NORMALIZER_EXCLUSIONS` below with the reason — keep that list in sync. + */ +export const NORMALIZED_URL_FIELDS = new Set([ + // Concrete entries are added in the green commit after the per-endpoint + // audit. The skeleton commit only ships the constant so static-invariant + // tests have a stable import target. +]); + +/** + * URL-shaped field names that we have deliberately *not* added to + * `NORMALIZED_URL_FIELDS`, with the reason. The negative tests in + * `normalizeBackendUrls.test.ts` assert that values for these names are + * passed through unchanged even when the value happens to begin with the + * configured application root. + * + * This list is informational — code does not read it. Its purpose is to + * preserve institutional knowledge so a future contributor doesn't add an + * exclusion to the allow-list by mistake. + */ +export const NORMALIZER_EXCLUSIONS: ReadonlyArray<{ + field: string; + reason: string; +}> = [ + { field: 'bug_report_url', reason: 'External (GitHub)' }, + { field: 'documentation_url', reason: 'External (docs site)' }, + { field: 'external_url', reason: 'External by name' }, + { field: 'bundle_url', reason: 'CDN / static asset host, not a Superset route' }, + { field: 'tracking_url', reason: 'External (analytics)' }, + { field: 'user_login_url', reason: 'OAuth / SSO endpoints, may be external' }, + { field: 'user_logout_url', reason: 'OAuth / SSO endpoints, may be external' }, + { field: 'user_info_url', reason: 'OAuth / SSO endpoints, may be external' }, + { + field: 'thumbnail_url', + reason: + 'Storage host varies (S3 / local) — needs per-endpoint audit before normalising', + }, + { + field: 'creator_url', + reason: 'User-profile destination varies by deployment', + }, +]; + +export interface NormalizeOptions { + /** + * Application root to strip. Pass an empty string to disable normalisation. + * Trailing slash is tolerated; the strip logic compares whole path segments. + */ + applicationRoot: string; +} + +/** + * Recursively normalise URL fields in a JSON-shaped value. + * + * Returns a new value when normalisation changed anything; otherwise returns + * the input by reference so consumers can compare with `===`. + */ +// eslint-disable-next-line @typescript-eslint/no-unused-vars -- stub +export function normalizeBackendUrls(value: T, options: NormalizeOptions): T { + throw new Error(NOT_IMPLEMENTED); +} + +/** + * Normalise a single URL string. Exposed for use cases that read a URL + * directly (e.g. bootstrap data) without going through the recursive walker. + */ +// eslint-disable-next-line @typescript-eslint/no-unused-vars -- stub +export function normalizeBackendUrlString( + value: string, + options: NormalizeOptions, +): string { + throw new Error(NOT_IMPLEMENTED); +} diff --git a/superset-frontend/packages/superset-ui-core/test/connection/SupersetClientAppRootContract.test.ts b/superset-frontend/packages/superset-ui-core/test/connection/SupersetClientAppRootContract.test.ts new file mode 100644 index 00000000000..348d2d04c91 --- /dev/null +++ b/superset-frontend/packages/superset-ui-core/test/connection/SupersetClientAppRootContract.test.ts @@ -0,0 +1,87 @@ +/** + * 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 { SupersetClientClass } from '@superset-ui/core'; + +// ============================================================================= +// Layer 4 example: SupersetClient × applicationRoot contract +// ============================================================================= +// +// Layer 4 pins down the contract between the channel-2 client and the +// application root. The channel rule is "callers pass router-relative paths; +// the client adds the prefix exactly once." This file proves that property in +// isolation so the rest of the codebase can rely on it. +// +// The full PR adds parallel tests for the React Router channel +// (`` × ``) and a composition test that +// drives `redirect()` and `` together. This file ships the +// SupersetClient half as the template. +// ============================================================================= + +describe('SupersetClient applies the application root exactly once', () => { + test('endpoint without leading slash is concatenated correctly under a non-empty appRoot', () => { + const client = new SupersetClientClass({ + protocol: 'https:', + host: 'config_host', + appRoot: '/superset', + }); + expect(client.getUrl({ endpoint: 'api/v1/chart' })).toBe( + 'https://config_host/superset/api/v1/chart', + ); + }); + + test('endpoint with leading slash is normalised to a single root segment', () => { + const client = new SupersetClientClass({ + protocol: 'https:', + host: 'config_host', + appRoot: '/superset', + }); + expect(client.getUrl({ endpoint: '/api/v1/chart' })).toBe( + 'https://config_host/superset/api/v1/chart', + ); + }); + + test('does not double-apply the application root when caller pre-prefixes', () => { + // This documents the bug class the helpers protect against. Pre-prefixing + // is forbidden by the channel rule, but we record the current behaviour + // here so anyone debugging a double-prefix issue lands on this assertion + // and reads the comment. + const client = new SupersetClientClass({ + protocol: 'https:', + host: 'config_host', + appRoot: '/superset', + }); + expect(client.getUrl({ endpoint: '/superset/api/v1/chart' })).toBe( + 'https://config_host/superset/superset/api/v1/chart', + ); + // ^ The duplicated `/superset` is exactly the symptom developers see when + // they wrap a SupersetClient endpoint in `ensureAppRoot`. The static + // invariant test in `navigationUtils.invariants.test.ts` catches that + // pattern before it reaches runtime. + }); + + test('empty application root produces no prefix segment', () => { + const client = new SupersetClientClass({ + protocol: 'https:', + host: 'config_host', + }); + expect(client.getUrl({ endpoint: '/api/v1/chart' })).toBe( + 'https://config_host/api/v1/chart', + ); + }); +}); diff --git a/superset-frontend/packages/superset-ui-core/test/connection/normalizeBackendUrls.test.ts b/superset-frontend/packages/superset-ui-core/test/connection/normalizeBackendUrls.test.ts new file mode 100644 index 00000000000..ee074153241 --- /dev/null +++ b/superset-frontend/packages/superset-ui-core/test/connection/normalizeBackendUrls.test.ts @@ -0,0 +1,114 @@ +/** + * 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 { + normalizeBackendUrls, + normalizeBackendUrlString, + NORMALIZED_URL_FIELDS, +} from '../../src/connection/normalizeBackendUrls'; + +// ============================================================================= +// Layer 3 example: backend URL normaliser +// ============================================================================= +// +// Layer 3 has two halves: positive tests (the normaliser strips the +// configured root from values in `NORMALIZED_URL_FIELDS`) and negative tests +// (it leaves everything else alone). The negative half carries most of the +// safety value — it's how we prove the normaliser doesn't over-reach. +// +// The full PR adds: +// • Per-field positive tests for every entry in NORMALIZED_URL_FIELDS +// • Per-field negative tests for every entry in NORMALIZER_EXCLUSIONS +// • Recursion through arrays and nested objects +// • Idempotence: `normalize(normalize(x))` equals `normalize(x)` +// • Per-call opt-out hook from SupersetClient +// +// This file ships one positive + one negative test as a template. +// ============================================================================= + +const PREFIX = '/superset'; + +describe('normalizeBackendUrls (Layer 3 — positive)', () => { + test('strips configured application root from a recognised URL field', () => { + // `explore_url` will be added to NORMALIZED_URL_FIELDS in the green commit. + // Until then this assertion exists to drive that decision. + const input = { id: 1, explore_url: '/superset/explore/?slice_id=1' }; + const output = normalizeBackendUrls(input, { applicationRoot: PREFIX }); + expect(output).toEqual({ id: 1, explore_url: '/explore/?slice_id=1' }); + }); +}); + +describe('normalizeBackendUrls (Layer 3 — negative passthrough)', () => { + test('leaves random non-allow-listed fields alone even when value looks path-shaped', () => { + // `description` is not — and must never be — a URL field. A user could + // legitimately type `/looks/like/a/path` into a description; stripping + // the prefix would silently mutate user content. + const input = { description: '/superset/just-text-from-a-user' }; + const output = normalizeBackendUrls(input, { applicationRoot: PREFIX }); + expect(output).toEqual(input); + }); + + test('leaves absolute URLs alone in recognised fields', () => { + const input = { explore_url: 'https://other.example.com/superset/foo' }; + const output = normalizeBackendUrls(input, { applicationRoot: PREFIX }); + expect(output).toEqual(input); + }); + + test('leaves protocol-relative URLs alone', () => { + const input = { explore_url: '//cdn.example.com/superset/foo' }; + const output = normalizeBackendUrls(input, { applicationRoot: PREFIX }); + expect(output).toEqual(input); + }); + + test('does not strip a similar-but-different prefix segment', () => { + // `/superset-public/...` shares the `/superset` text but is a different + // path segment. Conservative match: only `/superset` followed by `/` or + // end-of-string is treated as the application root. + const input = { explore_url: '/superset-public/explore/?slice_id=1' }; + const output = normalizeBackendUrls(input, { applicationRoot: PREFIX }); + expect(output).toEqual(input); + }); + + test('is a no-op when application root is empty', () => { + const input = { explore_url: '/explore/?slice_id=1' }; + const output = normalizeBackendUrls(input, { applicationRoot: '' }); + expect(output).toEqual(input); + }); +}); + +describe('normalizeBackendUrlString (Layer 3 — string-level entry point)', () => { + test('strips application root from a router-relative path', () => { + expect( + normalizeBackendUrlString('/superset/sqllab', { applicationRoot: PREFIX }), + ).toBe('/sqllab'); + }); + + test('passes absolute URLs through unchanged', () => { + expect( + normalizeBackendUrlString('https://external.example.com/foo', { + applicationRoot: PREFIX, + }), + ).toBe('https://external.example.com/foo'); + }); +}); + +describe('NORMALIZED_URL_FIELDS (allow-list shape)', () => { + test('is a Set so callers can rely on O(1) membership checks', () => { + expect(NORMALIZED_URL_FIELDS).toBeInstanceOf(Set); + }); +}); diff --git a/superset-frontend/spec/helpers/sourceTreeScanner.ts b/superset-frontend/spec/helpers/sourceTreeScanner.ts new file mode 100644 index 00000000000..a2f8a49a0b4 --- /dev/null +++ b/superset-frontend/spec/helpers/sourceTreeScanner.ts @@ -0,0 +1,216 @@ +/** + * 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 { readdirSync, readFileSync, statSync } from 'fs'; +import { join, relative, resolve, sep } from 'path'; + +/** + * Directories scanned by `scanSource` when `roots` is not supplied. + * Resolved relative to the `superset-frontend` workspace. + */ +const DEFAULT_ROOTS = ['src', 'packages/superset-ui-core/src']; + +/** + * Path segments that are always excluded. We compare against path components + * so any directory named `node_modules` (etc.) is skipped wherever it appears + * in the tree. + */ +const ALWAYS_SKIP_SEGMENTS = new Set([ + 'node_modules', + 'dist', + 'build', + 'coverage', + '__mocks__', + 'cypress-base', + 'playwright', +]); + +/** + * Filename suffixes that legitimately mention otherwise-banned helpers (tests + * import them, stories embed them) and should not be scanned for invariants. + */ +const ALWAYS_SKIP_SUFFIXES = ['.test.ts', '.test.tsx', '.stories.ts', '.stories.tsx']; + +/** Extensions considered source files. */ +const SOURCE_EXTENSIONS = ['.ts', '.tsx']; + +export interface ScanOptions { + /** + * Workspace-relative directory roots to scan. Defaults to the source tree. + * Each entry is walked recursively. + */ + roots?: string[]; + /** + * Additional path segments to skip in addition to {@link ALWAYS_SKIP_SEGMENTS}. + */ + ignoreSegments?: string[]; + /** Regex run against each line of each file. */ + pattern: RegExp; + /** + * File paths (relative to `superset-frontend`, forward slashes) that are + * exempt from this scan. Use sparingly; every entry should justify itself + * in a comment. + */ + allowlist?: string[]; +} + +export interface ScanHit { + /** Path relative to `superset-frontend`, with forward slashes. */ + file: string; + /** 1-based line number. */ + line: number; + /** The text of the matching line, trimmed. */ + text: string; + /** The substring captured by `pattern`. */ + match: string; +} + +/** + * Workspace root used as the base for relative paths returned by the scanner. + * `__dirname` resolves to `/spec/helpers`, so the parent's parent + * is the workspace regardless of where Jest is invoked from. + */ +const WORKSPACE_ROOT = resolve(__dirname, '..', '..'); + +function isSourceFile(name: string): boolean { + return ( + SOURCE_EXTENSIONS.some(ext => name.endsWith(ext)) && + !ALWAYS_SKIP_SUFFIXES.some(suffix => name.endsWith(suffix)) + ); +} + +function walk(directory: string, ignoreSegments: Set): string[] { + const found: string[] = []; + + let entries; + try { + entries = readdirSync(directory, { withFileTypes: true }); + } catch { + return found; + } + + for (const entry of entries) { + if (ignoreSegments.has(entry.name)) continue; + const absolute = join(directory, entry.name); + + if (entry.isDirectory()) { + found.push(...walk(absolute, ignoreSegments)); + } else if (entry.isFile() && isSourceFile(entry.name)) { + found.push(absolute); + } + } + + return found; +} + +function toForwardSlashes(path: string): string { + return sep === '/' ? path : path.split(sep).join('/'); +} + +/** + * Scan source files under `roots` for lines matching `pattern`. + * + * Each match is returned as a {@link ScanHit} with a workspace-relative path + * and 1-based line number. Files listed in `allowlist` are skipped entirely. + * + * Scanning is deliberately textual (line-by-line regex) rather than AST-based + * — these invariants flag forbidden *patterns*, not forbidden *expressions*. + * False positives on string literals or comments should be addressed by + * tightening the regex, not by parsing. + */ +export function scanSource(options: ScanOptions): ScanHit[] { + const { + roots = DEFAULT_ROOTS, + ignoreSegments = [], + pattern, + allowlist = [], + } = options; + + const ignoreSet = new Set([...ALWAYS_SKIP_SEGMENTS, ...ignoreSegments]); + const allowSet = new Set(allowlist); + const hits: ScanHit[] = []; + + const seen = new Set(); + for (const root of roots) { + const absoluteRoot = resolve(WORKSPACE_ROOT, root); + let stat; + try { + stat = statSync(absoluteRoot); + } catch { + continue; + } + if (!stat.isDirectory()) continue; + + for (const absoluteFile of walk(absoluteRoot, ignoreSet)) { + if (seen.has(absoluteFile)) continue; + seen.add(absoluteFile); + + const relativePath = toForwardSlashes( + relative(WORKSPACE_ROOT, absoluteFile), + ); + if (allowSet.has(relativePath)) continue; + + const contents = readFileSync(absoluteFile, 'utf8'); + const lines = contents.split('\n'); + + for (let index = 0; index < lines.length; index += 1) { + const lineText = lines[index]; + // Re-create the regex per line so the global flag's lastIndex doesn't + // bleed across iterations. + const lineRegex = new RegExp(pattern.source, pattern.flags); + const match = lineRegex.exec(lineText); + if (match) { + hits.push({ + file: relativePath, + line: index + 1, + text: lineText.trim(), + match: match[0], + }); + } + } + } + } + + return hits; +} + +/** + * Format a list of hits as a human-readable failure message. Used by + * invariant tests so the developer sees `file:line` for every violation. + */ +export function formatHits(hits: ScanHit[], header: string): string { + if (hits.length === 0) return header; + const lines = hits + .slice(0, 50) + .map(hit => ` ${hit.file}:${hit.line} — ${hit.text}`); + const overflow = + hits.length > 50 ? `\n ... and ${hits.length - 50} more` : ''; + return `${header}\n${lines.join('\n')}${overflow}`; +} + +/** + * Helper that fails a Jest test with a formatted message when `hits` is + * non-empty. Returns void so call sites read naturally: + * + * expectNoHits(scanSource({ pattern: /window\.open\(/ }), 'Found raw window.open'); + */ +export function expectNoHits(hits: ScanHit[], header: string): void { + if (hits.length > 0) { + throw new Error(formatHits(hits, header)); + } +} diff --git a/superset-frontend/spec/helpers/withApplicationRoot.ts b/superset-frontend/spec/helpers/withApplicationRoot.ts new file mode 100644 index 00000000000..f024aeb0800 --- /dev/null +++ b/superset-frontend/spec/helpers/withApplicationRoot.ts @@ -0,0 +1,91 @@ +/** + * 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. + */ + +/** + * Test fixture for subdirectory-deployment scenarios. + * + * Bootstrap data in Superset is read once per module load via + * `getBootstrapData()` and cached. Tests that exercise URL generation under a + * non-empty `application_root` therefore need to rewrite the `#app` element + * and force the relevant modules to re-import so the cache is rebuilt. + * + * `withApplicationRoot` centralises that ritual. Usage: + * + * import { withApplicationRoot } from 'spec/helpers/withApplicationRoot'; + * + * test('redirects to prefixed root under subdirectory deployment', async () => { + * await withApplicationRoot('/superset/', async () => { + * const { redirect } = await import('src/utils/navigationUtils'); + * redirect('/'); + * expect(window.location.href).toBe('/superset/'); + * }); + * }); + * + * The callback receives a freshly-reset module registry, so any imports inside + * it observe the configured root. After the callback finishes (or throws), the + * fixture restores the previous `
` markup and resets modules + * again, leaving the global state clean for the next test. + * + * Pass `''` (the default) to simulate a deployment with no application root. + */ +export async function withApplicationRoot( + applicationRoot: string, + callback: () => Promise | T, +): Promise { + const previousBody = document.body.innerHTML; + + try { + const bootstrapData = { + common: { application_root: applicationRoot }, + }; + document.body.innerHTML = `
`; + jest.resetModules(); + + // Touch getBootstrapData first so the cached value reflects the new DOM. + await import('src/utils/getBootstrapData'); + + return await callback(); + } finally { + document.body.innerHTML = previousBody; + jest.resetModules(); + } +} + +/** + * Convenience wrapper that runs the same assertion under multiple application + * roots. Use when the assertion text doesn't depend on the prefix. + * + * applicationRootScenarios([ + * { root: '', expected: '/sqllab' }, + * { root: '/superset/', expected: '/superset/sqllab' }, + * { root: '/a/b/c/', expected: '/a/b/c/sqllab' }, + * ], async ({ expected }) => { + * const { ensureAppRoot } = await import('src/utils/pathUtils'); + * expect(ensureAppRoot('/sqllab')).toBe(expected); + * }); + */ +export async function applicationRootScenarios( + scenarios: S[], + body: (scenario: S) => Promise | void, +): Promise { + for (const scenario of scenarios) { + // eslint-disable-next-line no-await-in-loop -- intentional: scenarios share document state. + await withApplicationRoot(scenario.root, () => body(scenario)); + } +} diff --git a/superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.subdirectory.test.tsx b/superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.subdirectory.test.tsx new file mode 100644 index 00000000000..090302f868a --- /dev/null +++ b/superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.subdirectory.test.tsx @@ -0,0 +1,162 @@ +/** + * 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 { render, screen, userEvent } from 'spec/helpers/testing-library'; +import { VizType } from '@superset-ui/core'; +import mockState from 'spec/fixtures/mockState'; +import SliceHeaderControls, { SliceHeaderControlsProps } from '.'; + +// ============================================================================= +// Layer 5 example: per-site regression test for SliceHeaderControls +// ============================================================================= +// +// Subdirectory-specific behaviour for SliceHeaderControls. The full PR adds +// parallel files for RedirectWarning, ResultSet, DatasourceEditor, +// SaveDatasetModal, ViewQuery, plus reinstates the regression tests from +// commits 86fe4fc8b2 (chart export) and 36a32e7b49 (SavedQueryList, +// dashboard fullscreen) which haven't merged to master yet. +// +// Why a separate file: the existing SliceHeaderControls.test.tsx is 676 lines +// of shared setup that does not mock `getBootstrapData`. Mocking it at the +// top of that file would force every existing test to consider application +// root behaviour. Putting subdirectory regressions in their own file keeps +// the mock surface explicit and discoverable by name. +// +// This test is RED today: SliceHeaderControls/index.tsx:266 calls +// `window.open(props.exploreUrl, '_blank')` without prefixing the root, so +// the assertion below fails. The migration commit replaces that call with +// `openInNewTab(props.exploreUrl)` (which prefixes internally) and the test +// goes green. +// ============================================================================= + +const APPLICATION_ROOT_MOCK = jest.fn(() => ''); + +jest.mock('src/utils/getBootstrapData', () => ({ + applicationRoot: () => APPLICATION_ROOT_MOCK(), +})); + +const SLICE_ID = 371; + +const buildProps = (): SliceHeaderControlsProps => + ({ + addDangerToast: jest.fn(), + addSuccessToast: jest.fn(), + exploreChart: jest.fn(), + exportCSV: jest.fn(), + exportFullCSV: jest.fn(), + exportXLSX: jest.fn(), + exportFullXLSX: jest.fn(), + exportPivotExcel: jest.fn(), + forceRefresh: jest.fn(), + handleToggleFullSize: jest.fn(), + toggleExpandSlice: jest.fn(), + logEvent: jest.fn(), + logExploreChart: jest.fn(), + slice: { + slice_id: SLICE_ID, + slice_url: '/explore/?form_data=%7B%22slice_id%22%3A%20371%7D', + slice_name: 'Subdirectory regression chart', + slice_description: '', + form_data: { + slice_id: SLICE_ID, + datasource: '58__table', + viz_type: VizType.Sunburst, + }, + viz_type: VizType.Sunburst, + datasource: '58__table', + description: '', + description_markeddown: '', + owners: [], + modified: '', + changed_on: 0, + }, + isCached: [false], + isExpanded: false, + cachedDttm: [''], + updatedDttm: 0, + supersetCanExplore: true, + supersetCanCSV: true, + componentId: 'CHART-subdir', + dashboardId: 26, + isFullSize: false, + chartStatus: 'rendered', + showControls: true, + supersetCanShare: true, + formData: { + slice_id: SLICE_ID, + datasource: '58__table', + viz_type: VizType.Sunburst, + }, + exploreUrl: '/explore/?dashboard_page_id=abc&slice_id=371', + defaultOpen: true, + }) as unknown as SliceHeaderControlsProps; + +const renderControls = (): void => { + render(, { + useRedux: true, + useRouter: true, + initialState: { + ...mockState, + user: { + ...mockState.user, + roles: { Admin: [['can_samples', 'Datasource']] }, + }, + }, + }); +}; + +describe('SliceHeaderControls — Cmd-click "Edit chart" under subdirectory deployment', () => { + let openSpy: jest.SpyInstance; + + beforeEach(() => { + APPLICATION_ROOT_MOCK.mockReturnValue(''); + openSpy = jest.spyOn(window, 'open').mockImplementation(() => null); + }); + + afterEach(() => { + openSpy.mockRestore(); + }); + + test('opens the unprefixed exploreUrl when application root is empty', async () => { + APPLICATION_ROOT_MOCK.mockReturnValue(''); + renderControls(); + + userEvent.click(screen.getByRole('button', { name: 'More Options' })); + const editChart = await screen.findByText('Edit chart'); + userEvent.click(editChart, { metaKey: true }); + + expect(openSpy).toHaveBeenCalledWith( + '/explore/?dashboard_page_id=abc&slice_id=371', + '_blank', + ); + }); + + test('opens the prefixed exploreUrl when deployed under a subdirectory', async () => { + APPLICATION_ROOT_MOCK.mockReturnValue('/superset'); + renderControls(); + + userEvent.click(screen.getByRole('button', { name: 'More Options' })); + const editChart = await screen.findByText('Edit chart'); + userEvent.click(editChart, { metaKey: true }); + + expect(openSpy).toHaveBeenCalledWith( + '/superset/explore/?dashboard_page_id=abc&slice_id=371', + '_blank', + ); + }); +}); diff --git a/superset-frontend/src/utils/navigationUtils.invariants.test.ts b/superset-frontend/src/utils/navigationUtils.invariants.test.ts new file mode 100644 index 00000000000..91a900ad2e3 --- /dev/null +++ b/superset-frontend/src/utils/navigationUtils.invariants.test.ts @@ -0,0 +1,91 @@ +/** + * 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 { expectNoHits, scanSource } from 'spec/helpers/sourceTreeScanner'; + +// ============================================================================= +// Layer 2 example: structural invariant +// ============================================================================= +// +// Layer 2 contains tests that read the source tree and assert structural +// properties — "no file outside `navigationUtils.ts` imports `ensureAppRoot`" +// is the canonical example. The full PR adds parallel scans for raw +// `window.open` / `window.location.href`, double-prefix patterns through +// `SupersetClient` and `history.push`, and ``. +// +// Each test seeds an allow-list of current violations so the suite is GREEN +// on day one. Migration commits then delete entries from the allow-list, +// driving the count to zero. New violations introduced after migration fail +// the suite immediately and surface a `file:line` location in the message. +// +// The list below is the INITIAL seed — every entry will be removed by a +// subsequent migration commit. Do not extend it without an inline comment +// explaining why the file is exempt. +// ============================================================================= + +const PATH_UTILS_IMPORT_ALLOWLIST: string[] = [ + // Migrated by future commits. Each line listed here is a call site that + // currently imports `ensureAppRoot` or `makeUrl` directly; the migration + // PRs replace those imports with calls to focused helpers in + // `src/utils/navigationUtils.ts` and remove the file from this list. + 'src/SqlLab/components/QueryTable/index.tsx', + 'src/SqlLab/components/ResultSet/index.tsx', + 'src/components/Chart/DrillDetail/DrillDetailPane.tsx', + 'src/components/Chart/chartAction.ts', + 'src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx', + 'src/components/FacePile/index.tsx', + 'src/components/StreamingExportModal/useStreamingExport.ts', + 'src/explore/components/controls/ViewQuery.tsx', + 'src/explore/exploreUtils/index.ts', + 'src/features/databases/DatabaseModal/index.tsx', + 'src/features/datasets/AddDataset/LeftPanel/index.tsx', + 'src/features/home/EmptyState.tsx', + 'src/features/home/Menu.tsx', + 'src/features/home/RightMenu.tsx', + 'src/features/home/SavedQueries.tsx', + 'src/middleware/loggerMiddleware.ts', + 'src/pages/SavedQueryList/index.tsx', + 'src/preamble.ts', + 'src/views/CRUD/hooks.ts', +]; + +test('no file outside navigationUtils.ts imports ensureAppRoot or makeUrl from pathUtils', () => { + const hits = scanSource({ + pattern: /\b(?:ensureAppRoot|makeUrl)\b/, + allowlist: [ + // The two modules that are *allowed* to know about path prefixing. + // `pathUtils.ts` defines the helpers; `navigationUtils.ts` is the only + // re-export sanctioned for the rest of the codebase to consume. + 'src/utils/pathUtils.ts', + 'src/utils/navigationUtils.ts', + // SupersetClient has its own `appRoot` configuration path — it does not + // import from `pathUtils`. Excluded so a future occurrence of the word + // `appRoot` in connection internals doesn't trip this scan. + 'packages/superset-ui-core/src/connection/SupersetClientClass.ts', + 'packages/superset-ui-core/src/connection/normalizeBackendUrls.ts', + ...PATH_UTILS_IMPORT_ALLOWLIST, + ], + }); + + expectNoHits( + hits, + 'Found imports of ensureAppRoot / makeUrl outside navigationUtils.ts. ' + + 'Use the focused helpers (openInNewTab, redirect, getShareableUrl, AppLink) ' + + 'instead, or add the file to PATH_UTILS_IMPORT_ALLOWLIST with justification.', + ); +}); diff --git a/superset-frontend/src/utils/navigationUtils.test.ts b/superset-frontend/src/utils/navigationUtils.test.ts new file mode 100644 index 00000000000..82c776108de --- /dev/null +++ b/superset-frontend/src/utils/navigationUtils.test.ts @@ -0,0 +1,115 @@ +/** + * 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 { withApplicationRoot } from 'spec/helpers/withApplicationRoot'; + +// ============================================================================= +// Layer 1 example: openInNewTab +// ============================================================================= +// +// Layer 1 covers per-helper unit behaviour. The full PR adds parallel suites +// for `redirect`, `redirectReplace`, `getShareableUrl`, and ``. This +// file ships a single helper as a template for the structure those follow: +// +// 1. Each helper is exercised under empty appRoot AND a non-empty appRoot. +// 2. Absolute URLs (https://, mailto:, etc.) pass through unchanged. +// 3. Already-prefixed input is idempotent (does not double-prefix). +// ============================================================================= + +describe('openInNewTab', () => { + let openSpy: jest.SpyInstance; + + beforeEach(() => { + openSpy = jest.spyOn(window, 'open').mockImplementation(() => null); + }); + + afterEach(() => { + openSpy.mockRestore(); + }); + + test('passes router-relative path through unchanged when application root is empty', async () => { + await withApplicationRoot('', async () => { + const { openInNewTab } = await import('src/utils/navigationUtils'); + openInNewTab('/sqllab?new=true'); + expect(openSpy).toHaveBeenCalledWith( + '/sqllab?new=true', + '_blank', + 'noopener noreferrer', + ); + }); + }); + + test('prefixes router-relative path with application root under subdirectory deployment', async () => { + await withApplicationRoot('/superset/', async () => { + const { openInNewTab } = await import('src/utils/navigationUtils'); + openInNewTab('/sqllab?new=true'); + expect(openSpy).toHaveBeenCalledWith( + '/superset/sqllab?new=true', + '_blank', + 'noopener noreferrer', + ); + }); + }); + + test('prefixes correctly for nested subdirectory roots', async () => { + await withApplicationRoot('/a/b/c/', async () => { + const { openInNewTab } = await import('src/utils/navigationUtils'); + openInNewTab('/dashboard/list'); + expect(openSpy).toHaveBeenCalledWith( + '/a/b/c/dashboard/list', + '_blank', + 'noopener noreferrer', + ); + }); + }); + + test('passes absolute URLs through unchanged regardless of application root', async () => { + await withApplicationRoot('/superset/', async () => { + const { openInNewTab } = await import('src/utils/navigationUtils'); + openInNewTab('https://external.example.com/docs'); + expect(openSpy).toHaveBeenCalledWith( + 'https://external.example.com/docs', + '_blank', + 'noopener noreferrer', + ); + }); + }); + + test('passes mailto: URLs through unchanged', async () => { + await withApplicationRoot('/superset/', async () => { + const { openInNewTab } = await import('src/utils/navigationUtils'); + openInNewTab('mailto:owner@example.com'); + expect(openSpy).toHaveBeenCalledWith( + 'mailto:owner@example.com', + '_blank', + 'noopener noreferrer', + ); + }); + }); + + test('uses noopener noreferrer for security on every call', async () => { + await withApplicationRoot('/superset/', async () => { + const { openInNewTab } = await import('src/utils/navigationUtils'); + openInNewTab('/sqllab'); + expect(openSpy).toHaveBeenCalledTimes(1); + const features = openSpy.mock.calls[0][2] as string; + expect(features).toContain('noopener'); + expect(features).toContain('noreferrer'); + }); + }); +}); diff --git a/superset-frontend/src/utils/navigationUtils.ts b/superset-frontend/src/utils/navigationUtils.ts index 11606aa7e3d..f9682d46e54 100644 --- a/superset-frontend/src/utils/navigationUtils.ts +++ b/superset-frontend/src/utils/navigationUtils.ts @@ -16,8 +16,93 @@ * specific language governing permissions and limitations * under the License. */ +import type { AnchorHTMLAttributes, ReactElement } from 'react'; import { ensureAppRoot } from './pathUtils'; +// ============================================================================= +// Channel-3 helpers (browser-direct sinks) +// ============================================================================= +// +// Every helper in this section takes a *router-relative* path (the same shape +// you'd pass to `` or `history.push`) and applies the application +// root internally before handing the URL to the browser. This keeps the rest +// of the codebase decision-free: callers always write `/sqllab`, never +// `${applicationRoot()}/sqllab`. +// +// Once migration is complete, `ensureAppRoot` and `makeUrl` are imported only +// from this module. A static-invariant test (see +// `navigationUtils.invariants.test.ts`) enforces that boundary. +// ============================================================================= + +const NOT_IMPLEMENTED = + 'navigationUtils helper not implemented yet — landing in the green commit of the subdirectory-helpers PR.'; + +/** + * Open a router-relative path in a new browser tab. + * + * The path is automatically prefixed with the application root so the new tab + * lands inside Superset on subdirectory deployments. + */ +// eslint-disable-next-line @typescript-eslint/no-unused-vars -- stub +export function openInNewTab(path: string): void { + throw new Error(NOT_IMPLEMENTED); +} + +/** + * Navigate the current window to a router-relative path via `window.location`. + * + * Unlike `history.push`, this triggers a full page load. Use it only when the + * destination is outside the React Router tree (e.g. a backend-rendered page) + * or when a hard reload is required. + */ +// eslint-disable-next-line @typescript-eslint/no-unused-vars -- stub +export function redirect(path: string): void { + throw new Error(NOT_IMPLEMENTED); +} + +/** + * Replace the current entry in `window.history` with a router-relative path. + * No new history entry is pushed. Use sparingly — most navigation should go + * through React Router's `history.replace`. + */ +// eslint-disable-next-line @typescript-eslint/no-unused-vars -- stub +export function redirectReplace(path: string): void { + throw new Error(NOT_IMPLEMENTED); +} + +/** + * Build a fully-qualified URL (`://`) from a + * router-relative path. Use for clipboard / share / email targets that need + * to round-trip through external systems back to this Superset deployment. + */ +// eslint-disable-next-line @typescript-eslint/no-unused-vars -- stub +export function getShareableUrl(path: string): string { + throw new Error(NOT_IMPLEMENTED); +} + +/** + * Anchor element that prefixes its `href` with the application root. + * + * Use this instead of `` whenever the href is computed at + * runtime. Static `` literals are fine — the static- + * invariant test only flags non-literal hrefs. + */ +// eslint-disable-next-line @typescript-eslint/no-unused-vars -- stub +export function AppLink( + props: AnchorHTMLAttributes & { href: string }, +): ReactElement { + throw new Error(NOT_IMPLEMENTED); +} + +// ============================================================================= +// Legacy multi-mode helpers +// ============================================================================= +// These predate the focused helpers above. They behave correctly but are +// scheduled for replacement so the channel-3 surface is entirely composed of +// single-purpose functions. Migration commits will rewrite call sites to use +// the focused helpers, then delete these. +// ============================================================================= + export const navigateTo = ( url: string, options?: { newWindow?: boolean; assign?: boolean },