Copilot commented on code in PR #36771: URL: https://github.com/apache/superset/pull/36771#discussion_r2636525487
########## superset-frontend/src/explore/exploreUtils/exportChart.test.ts: ########## @@ -0,0 +1,173 @@ +/** + * 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 { exportChart } from '.'; + +// Mock pathUtils to control app root prefix +jest.mock('src/utils/pathUtils', () => ({ + ensureAppRoot: jest.fn((path: string) => path), +})); + +// Mock SupersetClient +jest.mock('@superset-ui/core', () => ({ + ...jest.requireActual('@superset-ui/core'), + SupersetClient: { + postForm: jest.fn(), + get: jest.fn().mockResolvedValue({ json: {} }), + post: jest.fn().mockResolvedValue({ json: {} }), + }, + getChartBuildQueryRegistry: jest.fn().mockReturnValue({ + get: jest.fn().mockReturnValue(() => () => ({})), + }), + getChartMetadataRegistry: jest.fn().mockReturnValue({ + get: jest.fn().mockReturnValue({ parseMethod: 'json' }), + }), +})); + +const { ensureAppRoot } = jest.requireMock('src/utils/pathUtils'); +const { getChartMetadataRegistry } = jest.requireMock('@superset-ui/core'); + +// Minimal formData that won't trigger legacy API (useLegacyApi = false) +const baseFormData = { + datasource: '1__table', + viz_type: 'table', +}; + +beforeEach(() => { + jest.clearAllMocks(); + // Default: no prefix + ensureAppRoot.mockImplementation((path: string) => path); + // Default: v1 API (not legacy) + getChartMetadataRegistry.mockReturnValue({ + get: jest.fn().mockReturnValue({ parseMethod: 'json' }), + }); +}); + +// Tests for exportChart URL prefix handling in streaming export +test('exportChart v1 API passes prefixed URL to onStartStreamingExport when app root is configured', async () => { + const appRoot = '/superset'; + ensureAppRoot.mockImplementation((path: string) => `${appRoot}${path}`); + + const onStartStreamingExport = jest.fn(); + + await exportChart({ + formData: baseFormData, + resultFormat: 'csv', + onStartStreamingExport: onStartStreamingExport as unknown as null, + }); + + expect(onStartStreamingExport).toHaveBeenCalledTimes(1); + const callArgs = onStartStreamingExport.mock.calls[0][0]; + expect(callArgs.url).toBe('/superset/api/v1/chart/data'); + expect(callArgs.exportType).toBe('csv'); +}); Review Comment: The test at line 62 has a misleading assertion comment. The comment at line 75-76 states "The URL should match the expected prefixed URL", but the test name and the assertion at line 76 suggest this tests whether the URL is prefixed. However, the test should verify that `makeUrl` was called with the unprefixed path and that the result is properly prefixed. The test doesn't verify that `exportChart` is properly calling `ensureAppRoot` (which calls `makeUrl` internally based on the implementation). Since the test file doesn't show changes to the actual `exportChart` implementation, this test may be testing behavior that doesn't exist yet or assumes implementation details not visible in this PR. ########## superset-frontend/src/SqlLab/components/ResultSet/ResultSet.test.tsx: ########## @@ -657,4 +671,131 @@ describe('ResultSet', () => { const resultsCalls = fetchMock.calls('glob:*/api/v1/sqllab/results/*'); expect(resultsCalls).toHaveLength(1); }); + + test('should use non-streaming export (href) when rows below threshold', async () => { + // This test validates that when rows < CSV_STREAMING_ROW_THRESHOLD, + // the component uses the direct download href instead of streaming export. + const appRoot = '/superset'; + applicationRootMock.mockReturnValue(appRoot); + + // Create a query with rows BELOW the threshold + const smallQuery = { + ...queries[0], + rows: 500, // Below the 1000 threshold + limitingFactor: 'NOT_LIMITED', + }; + + const { getByTestId } = setup( + mockedProps, + mockStore({ + ...initialState, + user: { + ...user, + roles: { + sql_lab: [['can_export_csv', 'SQLLab']], + }, + }, + sqlLab: { + ...initialState.sqlLab, + queries: { + [smallQuery.id]: smallQuery, + }, + }, + common: { + conf: { + CSV_STREAMING_ROW_THRESHOLD: 1000, + }, + }, + }), + ); + + await waitFor(() => { + expect(getByTestId('export-csv-button')).toBeInTheDocument(); + }); + + const exportButton = getByTestId('export-csv-button'); + + // Non-streaming export should have href attribute with prefixed URL + expect(exportButton).toHaveAttribute( + 'href', + expect.stringMatching(new RegExp(`^${appRoot}/api/v1/sqllab/export/`)), + ); + + // Click should NOT trigger startExport for non-streaming + fireEvent.click(exportButton); + expect(mockStartExport).not.toHaveBeenCalled(); + }); + + test.each([ + { + name: 'no prefix (default deployment)', + appRoot: '', + expectedUrl: '/api/v1/sqllab/export_streaming/', + }, + { + name: 'with subdirectory prefix', + appRoot: '/superset', + expectedUrl: '/superset/api/v1/sqllab/export_streaming/', + }, + { + name: 'with nested subdirectory prefix', + appRoot: '/my-app/superset', + expectedUrl: '/my-app/superset/api/v1/sqllab/export_streaming/', + }, + ])( + 'streaming export URL should be correctly prefixed: $name', Review Comment: The test name on line 729 uses `test.each` to iterate over multiple test cases, but the test description template at line 746 reads "streaming export URL should be correctly prefixed: $name". The word "correctly" is vague and doesn't clearly indicate what behavior is being tested (i.e., that the URL should include the app root prefix based on configuration). Consider revising the test description to be more specific, such as "streaming export URL should include app root prefix when configured: $name" to better describe the expected behavior. ```suggestion 'streaming export URL should include app root prefix when configured: $name', ``` ########## superset-frontend/src/utils/export.test.ts: ########## @@ -33,6 +33,7 @@ jest.mock('@superset-ui/core', () => ({ jest.mock('content-disposition'); +// Mock pathUtils for double-prefix prevention tests Review Comment: The test comment "Mock pathUtils for double-prefix prevention tests" suggests this is testing double-prefix prevention, but the mock on line 38 simply returns the path unchanged. This doesn't actually simulate the real behavior of `ensureAppRoot` which would add the app root prefix. While the test implementation later changes the mock behavior (line 428-430), the initial setup comment is misleading about what the default mock does. Consider updating the comment to clarify that this establishes a default no-op mock that will be customized in specific tests. ```suggestion // Default no-op mock for pathUtils; specific tests will customize ensureAppRoot to simulate app root prefixing ``` ########## superset-frontend/src/components/StreamingExportModal/useStreamingExport.ts: ########## @@ -39,6 +41,34 @@ interface StreamingExportParams { const NEWLINE_BYTE = 10; // '\n' character code +/** + * Ensures URL has the application root prefix for subdirectory deployments. + * Applies makeUrl to relative paths that don't already include the app root. + * This guards against callers forgetting to prefix URLs when using native fetch. + */ +const ensureUrlPrefix = (url: string): string => { + const appRoot = applicationRoot(); + // Protocol-relative URLs (//example.com/...) should pass through unchanged + if (url.startsWith('//')) { + return url; + } + // Only process relative URLs (starting with /) + if (!url.startsWith('/')) { + return url; + } + // If no app root configured, return as-is + if (!appRoot) { + return url; + } + // If URL already has the app root prefix, return as-is + // Use strict check to avoid false positives with sibling paths (e.g., /app2 when appRoot is /app) + if (url === appRoot || url.startsWith(`${appRoot}/`)) { + return url; + } + // Apply prefix via makeUrl + return makeUrl(url); +}; Review Comment: The `ensureUrlPrefix` function has a potential issue with its sibling path detection logic. The check at line 65 uses `url.startsWith(\`${appRoot}/\`)` which will correctly reject sibling paths like `/app2` when `appRoot` is `/app`. However, this means the function will double-prefix such paths, which may not be the intended behavior. For example, if `appRoot` is `/app` and the URL is `/app2/api/export`, the function will transform it to `/app/app2/api/export`. While the test at line 408-435 in useStreamingExport.test.ts validates this behavior as expected, this seems like it could be a source of bugs if callers are already passing correctly-formed absolute paths that happen to start with a prefix similar to the app root. Consider adding clearer documentation about this edge case, or reconsider whether sibling paths should be prefixed at all. ########## superset-frontend/src/explore/exploreUtils/exportChart.test.ts: ########## @@ -0,0 +1,173 @@ +/** + * 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 { exportChart } from '.'; + +// Mock pathUtils to control app root prefix +jest.mock('src/utils/pathUtils', () => ({ + ensureAppRoot: jest.fn((path: string) => path), +})); + +// Mock SupersetClient +jest.mock('@superset-ui/core', () => ({ + ...jest.requireActual('@superset-ui/core'), + SupersetClient: { + postForm: jest.fn(), + get: jest.fn().mockResolvedValue({ json: {} }), + post: jest.fn().mockResolvedValue({ json: {} }), + }, + getChartBuildQueryRegistry: jest.fn().mockReturnValue({ + get: jest.fn().mockReturnValue(() => () => ({})), + }), + getChartMetadataRegistry: jest.fn().mockReturnValue({ + get: jest.fn().mockReturnValue({ parseMethod: 'json' }), + }), +})); + +const { ensureAppRoot } = jest.requireMock('src/utils/pathUtils'); +const { getChartMetadataRegistry } = jest.requireMock('@superset-ui/core'); + +// Minimal formData that won't trigger legacy API (useLegacyApi = false) +const baseFormData = { + datasource: '1__table', + viz_type: 'table', +}; + +beforeEach(() => { + jest.clearAllMocks(); + // Default: no prefix + ensureAppRoot.mockImplementation((path: string) => path); + // Default: v1 API (not legacy) + getChartMetadataRegistry.mockReturnValue({ + get: jest.fn().mockReturnValue({ parseMethod: 'json' }), + }); +}); + +// Tests for exportChart URL prefix handling in streaming export +test('exportChart v1 API passes prefixed URL to onStartStreamingExport when app root is configured', async () => { + const appRoot = '/superset'; + ensureAppRoot.mockImplementation((path: string) => `${appRoot}${path}`); + + const onStartStreamingExport = jest.fn(); + + await exportChart({ + formData: baseFormData, + resultFormat: 'csv', + onStartStreamingExport: onStartStreamingExport as unknown as null, + }); + + expect(onStartStreamingExport).toHaveBeenCalledTimes(1); + const callArgs = onStartStreamingExport.mock.calls[0][0]; + expect(callArgs.url).toBe('/superset/api/v1/chart/data'); + expect(callArgs.exportType).toBe('csv'); +}); + +test('exportChart v1 API passes unprefixed URL when no app root is configured', async () => { + ensureAppRoot.mockImplementation((path: string) => path); + + const onStartStreamingExport = jest.fn(); + + await exportChart({ + formData: baseFormData, + resultFormat: 'csv', + onStartStreamingExport: onStartStreamingExport as unknown as null, + }); + + expect(onStartStreamingExport).toHaveBeenCalledTimes(1); + const callArgs = onStartStreamingExport.mock.calls[0][0]; + expect(callArgs.url).toBe('/api/v1/chart/data'); +}); + +test('exportChart v1 API passes nested prefix for deeply nested deployments', async () => { + const appRoot = '/my-company/analytics/superset'; + ensureAppRoot.mockImplementation((path: string) => `${appRoot}${path}`); + + const onStartStreamingExport = jest.fn(); + + await exportChart({ + formData: baseFormData, + resultFormat: 'xlsx', + onStartStreamingExport: onStartStreamingExport as unknown as null, + }); + + expect(onStartStreamingExport).toHaveBeenCalledTimes(1); + const callArgs = onStartStreamingExport.mock.calls[0][0]; + expect(callArgs.url).toBe('/my-company/analytics/superset/api/v1/chart/data'); + expect(callArgs.exportType).toBe('xlsx'); +}); + +test('exportChart passes csv exportType for CSV exports', async () => { + const onStartStreamingExport = jest.fn(); + + await exportChart({ + formData: baseFormData, + resultFormat: 'csv', + onStartStreamingExport: onStartStreamingExport as unknown as null, + }); + + expect(onStartStreamingExport).toHaveBeenCalledWith( + expect.objectContaining({ + exportType: 'csv', + }), + ); +}); + +test('exportChart passes xlsx exportType for Excel exports', async () => { + const onStartStreamingExport = jest.fn(); + + await exportChart({ + formData: baseFormData, + resultFormat: 'xlsx', + onStartStreamingExport: onStartStreamingExport as unknown as null, + }); + + expect(onStartStreamingExport).toHaveBeenCalledWith( + expect.objectContaining({ + exportType: 'xlsx', + }), + ); +}); + +test('exportChart legacy API (useLegacyApi=true) passes prefixed URL with app root configured', async () => { + // Legacy API uses getExploreUrl() -> getURIDirectory() -> ensureAppRoot() + const appRoot = '/superset'; Review Comment: The comment at line 147 incorrectly states "Legacy API uses getExploreUrl() -> getURIDirectory() -> ensureAppRoot()". While this describes the call chain, the actual implementation of `exportChart` at line 265 in index.js uses `ensureAppRoot` directly for the v1 API path, not just for the legacy API. The legacy API path uses `getExploreUrl` which internally uses `getURIDirectory` which calls `ensureAppRoot`. The comment should be corrected to accurately describe that the legacy path uses `getExploreUrl` (which internally uses `getURIDirectory` and `ensureAppRoot`), while the comment implies only the legacy API uses this chain. ########## superset-frontend/src/utils/export.test.ts: ########## @@ -396,3 +397,53 @@ test('handles export with empty IDs array', async () => { }), ); }); + +const { ensureAppRoot } = jest.requireMock('./pathUtils'); + +const doublePrefixTestCases = [ + { + name: 'subdirectory prefix', + appRoot: '/superset', + resource: 'dashboard', + ids: [1], + }, + { + name: 'subdirectory prefix (dataset)', + appRoot: '/superset', + resource: 'dataset', + ids: [1], + }, + { + name: 'nested prefix', + appRoot: '/my-app/superset', + resource: 'dataset', + ids: [1, 2], + }, +]; + +test.each(doublePrefixTestCases)( + 'handleResourceExport endpoint should not include app prefix: $name', + async ({ appRoot, resource, ids }) => { + // Simulate real ensureAppRoot behavior: prepend the appRoot + (ensureAppRoot as jest.Mock).mockImplementation( + (path: string) => `${appRoot}${path}`, + ); + + const doneMock = jest.fn(); + await handleResourceExport(resource, ids, doneMock); + + // The endpoint passed to SupersetClient.get should NOT have the appRoot prefix + // because SupersetClient.getUrl() adds it when building the full URL. + const expectedEndpoint = `/api/v1/${resource}/export/?q=!(${ids.join(',')})`; + + // Explicitly verify no prefix in endpoint - this will fail if ensureAppRoot is used + const callArgs = (SupersetClient.get as jest.Mock).mock.calls.slice( + -1, + )[0][0]; + expect(callArgs.endpoint).not.toContain(appRoot); Review Comment: The test assertion at line 443 checks that `callArgs.endpoint` does not contain `appRoot`, but this assertion is redundant given the more specific assertion on line 444 that checks for exact equality with `expectedEndpoint`. If the endpoint exactly equals `/api/v1/${resource}/export/?q=!(${ids.join(',')})`, then it logically cannot contain the appRoot prefix. Consider removing the redundant assertion on line 443 to simplify the test and improve maintainability. ```suggestion const callArgs = (SupersetClient.get as jest.Mock).mock.calls.slice( -1, )[0][0]; ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
