Skip to content

fix: Avoid dataset drill request if no perm #34665

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Vitor-Avila merged 1 commit into from
Aug 25, 2025
Merged

fix: Avoid dataset drill request if no perm #34665

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ const ChartContextMenu = (
formData.datasource,
dashboardId,
formData,
!canDrillToDetail && !canDrillBy,
);

const isLoadingDataset = datasetResource.status === ResourceStatus.Loading;
Expand Down
58 changes: 58 additions & 0 deletions superset-frontend/src/components/Chart/ChartContextMenu/useContextMenu.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,15 @@ import { renderHook } from '@testing-library/react-hooks';
import mockState from 'spec/fixtures/mockState';
import { sliceId } from 'spec/fixtures/mockChartQueries';
import { noOp } from 'src/utils/common';
import { cachedSupersetGet } from 'src/utils/cachedSupersetGet';
import { useContextMenu } from './useContextMenu';
import { ContextMenuItem } from './ChartContextMenu';

jest.mock('src/utils/cachedSupersetGet');

const mockCachedSupersetGet = cachedSupersetGet as jest.MockedFunction<
typeof cachedSupersetGet
>;
const CONTEXT_MENU_TEST_ID = 'chart-context-menu';

// @ts-ignore
Expand Down Expand Up @@ -73,6 +79,19 @@ const setup = ({
return result;
};

beforeEach(() => {
mockCachedSupersetGet.mockClear();
mockCachedSupersetGet.mockResolvedValue({
response: {} as Response,
json: {
result: {
columns: [],
metrics: [],
},
},
});
});

test('Context menu renders', () => {
const result = setup();
expect(screen.queryByTestId(CONTEXT_MENU_TEST_ID)).not.toBeInTheDocument();
Expand Down Expand Up @@ -271,3 +290,42 @@ test('Context menu does not show "Drill to detail" with `can_drill`, `can_explor
result.current.onContextMenu(0, 0, {});
expect(screen.queryByText('Drill to detail')).not.toBeInTheDocument();
});

test('Dataset drill info API call is made when user has drill permissions', async () => {
const result = setup({
roles: {
Admin: [
['can_explore', 'Superset'],
['can_samples', 'Datasource'],
['can_write', 'ExploreFormDataRestApi'],
['can_get_drill_info', 'Dataset'],
],
},
});

result.current.onContextMenu(0, 0, {});

await new Promise(resolve => setTimeout(resolve, 0));

expect(mockCachedSupersetGet).toHaveBeenCalledWith({
endpoint: expect.stringContaining(
'/api/v1/dataset/1/drill_info/?q=(dashboard_id:',
),
});
});

test('Dataset drill info API call is not made when user lacks drill permissions', async () => {
const result = setup({
roles: {
Admin: [['invalid_permission', 'Dashboard']],
},
});

result.current.onContextMenu(0, 0, {});

await new Promise(resolve => setTimeout(resolve, 0));

expect(mockCachedSupersetGet).not.toHaveBeenCalled();
expect(screen.queryByText('Drill by')).not.toBeInTheDocument();
expect(screen.queryByText('Drill to detail')).not.toBeInTheDocument();
});
10 changes: 5 additions & 5 deletions superset-frontend/src/components/Chart/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,11 @@ export type Dataset = {
first_name: string;
last_name: string;
};
changed_on_humanized: string;
created_on_humanized: string;
description: string;
table_name: string;
owners: {
changed_on_humanized?: string;
created_on_humanized?: string;
description?: string;
table_name?: string;
owners?: {
first_name: string;
last_name: string;
}[];
Expand Down
53 changes: 53 additions & 0 deletions superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,14 @@
import { render, screen, userEvent } from 'spec/helpers/testing-library';
import { FeatureFlag, VizType } from '@superset-ui/core';
import mockState from 'spec/fixtures/mockState';
import { cachedSupersetGet } from 'src/utils/cachedSupersetGet';
import SliceHeaderControls, { SliceHeaderControlsProps } from '.';

jest.mock('src/utils/cachedSupersetGet');

const mockCachedSupersetGet = cachedSupersetGet as jest.MockedFunction<
typeof cachedSupersetGet
>;
const SLICE_ID = 371;

const createProps = (viz_type = VizType.Sunburst) =>
Expand Down Expand Up @@ -116,6 +122,19 @@ const openMenu = () => {
userEvent.click(screen.getByRole('button', { name: 'More Options' }));
};

beforeEach(() => {
mockCachedSupersetGet.mockClear();
mockCachedSupersetGet.mockResolvedValue({
response: {} as Response,
json: {
result: {
columns: [],
metrics: [],
},
},
});
});

test('Should render', () => {
renderWrapper();
openMenu();
Expand Down Expand Up @@ -526,3 +545,37 @@ test('Should not show the "Edit chart" button', () => {
openMenu();
expect(screen.queryByText('Edit chart')).not.toBeInTheDocument();
});

test('Dataset drill info API call is made when user has drill permissions', async () => {
(global as any).featureFlags = {
[FeatureFlag.DrillToDetail]: true,
};
renderWrapper(undefined, {
Admin: [
['can_samples', 'Datasource'],
['can_explore', 'Superset'],
['can_get_drill_info', 'Dataset'],
],
});

await new Promise(resolve => setTimeout(resolve, 0));

expect(mockCachedSupersetGet).toHaveBeenCalledWith({
endpoint: expect.stringContaining(
'/api/v1/dataset/58/drill_info/?q=(dashboard_id:26)',
),
});
});

test('Dataset drill info API call is not made when user lacks drill permissions', async () => {
(global as any).featureFlags = {
[FeatureFlag.DrillToDetail]: true,
};
renderWrapper(undefined, {
Admin: [['invalid_permission', 'Dashboard']],
});

await new Promise(resolve => setTimeout(resolve, 0));

expect(mockCachedSupersetGet).not.toHaveBeenCalled();
});
1 change: 1 addition & 0 deletions superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@ const SliceHeaderControls = (
props.slice.datasource,
props.dashboardId,
props.formData,
!canDrillToDetail,
Copy link
Member

@msyavuz msyavuz Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only improvement i could think of at first is to move useDatasetDrillInfo inside the actual components but not sure if it's an improvement at all. I think having a skip argument is fine.

Copy link
Member

@michael-s-molina michael-s-molina Aug 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be handled by callers of this hook. The hook's functionality is to make a fetch request and return the drill info. You shouldn't call the hook if the required permissions are not there.

const datasetResource = canDrillToDetail ? useDatasetDrillInfo(...) : undefined;

Then you can handle the undefined datasetResource.

Copy link
Contributor Author

@Vitor-Avila Vitor-Avila Aug 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michael-s-molina I agree your suggestion looks a lot better, and also that the caller should decide if the request is needed or not. However, that would be a conditional hook, which then raises on pre-commit.

I believe the ideal/long-term solution would be to refactor this whole flow, but for now hopefully this skip param is fine.

);

const datasetWithVerboseMap =
Expand Down
8 changes: 4 additions & 4 deletions superset-frontend/src/features/datasets/metadataBar/useDatasetMetadataBar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,23 +60,23 @@ export const useDatasetMetadataBar = ({
? `${changed_by.first_name} ${changed_by.last_name}`
: notAvailable;
const formattedOwners =
owners?.length > 0
owners && owners.length > 0
? owners.map(owner => `${owner.first_name} ${owner.last_name}`)
: [notAvailable];
items.push({
type: MetadataType.Table,
title: table_name,
title: table_name || notAvailable,
});
items.push({
type: MetadataType.LastModified,
value: changed_on_humanized,
value: changed_on_humanized || notAvailable,
modifiedBy,
});
items.push({
type: MetadataType.Owner,
createdBy,
owners: formattedOwners,
createdOn: created_on_humanized,
createdOn: created_on_humanized || notAvailable,
});
if (description) {
items.push({
Expand Down
12 changes: 11 additions & 1 deletion superset-frontend/src/hooks/apiResources/datasets.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ export const useDatasetDrillInfo = (
datasetId: string | number,
dashboardId: number,
formData?: QueryFormData,
skip: boolean = false,
): Resource<Dataset> => {
const [resource, setResource] = useState<Resource<Dataset>>({
status: ResourceStatus.Loading,
Expand All @@ -71,6 +72,15 @@ export const useDatasetDrillInfo = (
});

useEffect(() => {
if (skip) {
// short circuit if `skip` is `true`
setResource({
status: ResourceStatus.Complete,
result: {} as Dataset,

This comment was marked as resolved.

Sign in to view

error: null,
});
return;
}
const fetchDataset = async () => {
try {
const numericDatasetId = getDatasetId(datasetId);
Expand Down Expand Up @@ -115,7 +125,7 @@ export const useDatasetDrillInfo = (
};

fetchDataset();
}, [datasetId, dashboardId, formData]);
}, [datasetId, dashboardId, formData, skip]);

return resource;
};
Loading