diff --git a/superset-frontend/src/components/Datasource/FoldersEditor/folderOperations.test.ts b/superset-frontend/src/components/Datasource/FoldersEditor/folderOperations.test.ts index 0b0c8f7dd6b..79531acdaf0 100644 --- a/superset-frontend/src/components/Datasource/FoldersEditor/folderOperations.test.ts +++ b/superset-frontend/src/components/Datasource/FoldersEditor/folderOperations.test.ts @@ -250,5 +250,111 @@ describe('folderUtils', () => { expect(result.length).toBeGreaterThan(2); expect(result.find(f => f.name === 'Custom Folder')).toBeDefined(); }); + + test('should preserve default folder positions when saved', () => { + const customFolder = createFolder('Custom'); + customFolder.children = [ + { + uuid: 'metric-1', + type: FoldersEditorItemType.Metric, + name: 'Test Metric 1', + }, + ]; + const savedFolders = [ + { + uuid: DEFAULT_METRICS_FOLDER_UUID, + type: FoldersEditorItemType.Folder as const, + name: 'Metrics', + children: [ + { + uuid: 'metric-2', + name: 'metric-2', + type: FoldersEditorItemType.Metric, + }, + ], + }, + customFolder, + { + uuid: DEFAULT_COLUMNS_FOLDER_UUID, + type: FoldersEditorItemType.Folder as const, + name: 'Columns', + children: [ + { + uuid: 'column-1', + name: 'column-1', + type: FoldersEditorItemType.Column, + }, + { + uuid: 'column-2', + name: 'column-2', + type: FoldersEditorItemType.Column, + }, + ], + }, + ]; + const result = ensureDefaultFolders( + savedFolders, + mockMetrics, + mockColumns, + ); + + expect(result).toHaveLength(3); + expect(result[0].uuid).toBe(DEFAULT_METRICS_FOLDER_UUID); + expect(result[1].uuid).toBe(customFolder.uuid); + expect(result[2].uuid).toBe(DEFAULT_COLUMNS_FOLDER_UUID); + }); + + test('should add unassigned items to existing default folders', () => { + const savedFolders = [ + { + uuid: DEFAULT_METRICS_FOLDER_UUID, + type: FoldersEditorItemType.Folder as const, + name: 'Metrics', + children: [ + { + uuid: 'metric-1', + name: 'metric-1', + type: FoldersEditorItemType.Metric, + }, + ], + }, + { + uuid: DEFAULT_COLUMNS_FOLDER_UUID, + type: FoldersEditorItemType.Folder as const, + name: 'Columns', + children: [ + { + uuid: 'column-1', + name: 'column-1', + type: FoldersEditorItemType.Column, + }, + ], + }, + ]; + const result = ensureDefaultFolders( + savedFolders, + mockMetrics, + mockColumns, + ); + + const metricsFolder = result.find( + f => f.uuid === DEFAULT_METRICS_FOLDER_UUID, + ); + const columnsFolder = result.find( + f => f.uuid === DEFAULT_COLUMNS_FOLDER_UUID, + ); + + // metric-2 was unassigned, should be appended + expect(metricsFolder?.children).toHaveLength(2); + expect(metricsFolder?.children?.some(c => c.uuid === 'metric-2')).toBe( + true, + ); + + // column-2 was unassigned, should be appended + expect(columnsFolder?.children).toHaveLength(2); + expect(columnsFolder?.children?.some(c => c.uuid === 'column-2')).toBe( + true, + ); + }); }); }); diff --git a/superset-frontend/src/components/Datasource/FoldersEditor/folderOperations.ts b/superset-frontend/src/components/Datasource/FoldersEditor/folderOperations.ts index e9f0b0d9119..cb810bd4a01 100644 --- a/superset-frontend/src/components/Datasource/FoldersEditor/folderOperations.ts +++ b/superset-frontend/src/components/Datasource/FoldersEditor/folderOperations.ts @@ -174,8 +174,6 @@ export const ensureDefaultFolders = ( f => f.uuid === DEFAULT_COLUMNS_FOLDER_UUID, ); - const result = [...enrichedFolders]; - // Build a Set of all assigned UUIDs in a single pass for O(1) lookups const assignedIds = new Set(); const collectAssignedIds = (folder: DatasourceFolder) => { @@ -189,33 +187,60 @@ export const ensureDefaultFolders = ( }; enrichedFolders.forEach(collectAssignedIds); - if (!hasMetricsFolder) { - const unassignedMetrics = metrics.filter(m => !assignedIds.has(m.uuid)); + const unassignedMetrics = metrics + .filter(m => !assignedIds.has(m.uuid)) + .map(m => ({ + type: FoldersEditorItemType.Metric as const, + uuid: m.uuid, + name: m.metric_name || '', + })); + const unassignedColumns = columns + .filter(c => !assignedIds.has(c.uuid)) + .map(c => ({ + type: FoldersEditorItemType.Column as const, + uuid: c.uuid, + name: c.column_name || '', + })); + // Add unassigned items to existing default folders (handles new items added after last save) + const result = enrichedFolders.map(folder => { + if ( + folder.uuid === DEFAULT_METRICS_FOLDER_UUID && + unassignedMetrics.length > 0 + ) { + return { + ...folder, + children: [...(folder.children || []), ...unassignedMetrics], + }; + } + if ( + folder.uuid === DEFAULT_COLUMNS_FOLDER_UUID && + unassignedColumns.length > 0 + ) { + return { + ...folder, + children: [...(folder.children || []), ...unassignedColumns], + }; + } + return folder; + }); + + // If default folders don't exist at all, add them at the end (backward compatibility) + if (!hasMetricsFolder) { result.push({ uuid: DEFAULT_METRICS_FOLDER_UUID, type: FoldersEditorItemType.Folder, name: t('Metrics'), - children: unassignedMetrics.map(m => ({ - type: FoldersEditorItemType.Metric, - uuid: m.uuid, - name: m.metric_name || '', - })), + children: unassignedMetrics, }); } if (!hasColumnsFolder) { - const unassignedColumns = columns.filter(c => !assignedIds.has(c.uuid)); - result.push({ uuid: DEFAULT_COLUMNS_FOLDER_UUID, type: FoldersEditorItemType.Folder, name: t('Columns'), - children: unassignedColumns.map(c => ({ - type: FoldersEditorItemType.Column, - uuid: c.uuid, - name: c.column_name || '', - })), + children: unassignedColumns, }); } diff --git a/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx b/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx index fe1a35cc72e..fd3b899f399 100644 --- a/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx +++ b/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx @@ -91,9 +91,8 @@ import Field from '../Field'; import { fetchSyncedColumns, updateColumns } from '../../utils'; import DatasetUsageTab from './components/DatasetUsageTab'; import { - DEFAULT_COLUMNS_FOLDER_UUID, DEFAULT_FOLDERS_COUNT, - DEFAULT_METRICS_FOLDER_UUID, + isDefaultFolder, } from '../../FoldersEditor/constants'; import { validateFolders } from '../../FoldersEditor/folderValidation'; import { countAllFolders } from '../../FoldersEditor/treeUtils'; @@ -942,8 +941,14 @@ class DatasourceEditor extends PureComponent< col => !!col.expression, ), folders: props.datasource.folders || [], - folderCount: - countAllFolders(props.datasource.folders || []) + DEFAULT_FOLDERS_COUNT, + folderCount: (() => { + const savedFolders = props.datasource.folders || []; + const savedCount = countAllFolders(savedFolders); + const hasDefaultsSaved = savedFolders.some(f => + isDefaultFolder(f.uuid), + ); + return savedCount + (hasDefaultsSaved ? 0 : DEFAULT_FOLDERS_COUNT); + })(), metadataLoading: false, activeTabKey: TABS_KEYS.SOURCE, datasourceType: props.datasource.sql @@ -1031,16 +1036,10 @@ class DatasourceEditor extends PureComponent< handleFoldersChange(folders: DatasourceFolder[]) { const folderCount = countAllFolders(folders); - const userMadeFolders = folders.filter( - f => - f.uuid !== DEFAULT_METRICS_FOLDER_UUID && - f.uuid !== DEFAULT_COLUMNS_FOLDER_UUID && - (f.children?.length ?? 0) > 0, - ); - this.setState({ folders: userMadeFolders, folderCount }, () => { + this.setState({ folders, folderCount }, () => { this.onDatasourceChange({ ...this.state.datasource, - folders: userMadeFolders, + folders, }); }); } @@ -2493,7 +2492,10 @@ class DatasourceEditor extends PureComponent< metrics={ sortedMetrics as unknown as import('@superset-ui/chart-controls').Metric[] } - columns={this.state.databaseColumns} + columns={[ + ...this.state.databaseColumns, + ...this.state.calculatedColumns, + ]} onChange={this.handleFoldersChange} /> ), diff --git a/superset-frontend/src/explore/components/DatasourcePanel/transformDatasourceFolders.test.ts b/superset-frontend/src/explore/components/DatasourcePanel/transformDatasourceFolders.test.ts index 33f922c69f6..c2bd343649b 100644 --- a/superset-frontend/src/explore/components/DatasourcePanel/transformDatasourceFolders.test.ts +++ b/superset-frontend/src/explore/components/DatasourcePanel/transformDatasourceFolders.test.ts @@ -21,6 +21,10 @@ import { Metric } from '@superset-ui/core'; import { transformDatasourceWithFolders } from './transformDatasourceFolders'; import { DatasourceFolder, DatasourcePanelColumn } from './types'; import { FoldersEditorItemType } from 'src/components/Datasource/types'; +import { + DEFAULT_METRICS_FOLDER_UUID, + DEFAULT_COLUMNS_FOLDER_UUID, +} from 'src/components/Datasource/FoldersEditor/constants'; const mockMetrics: Metric[] = [ { metric_name: 'metric1', uuid: 'metric1-uuid', expression: 'SUM(col1)' }, @@ -45,13 +49,13 @@ test('transforms data into default folders when no folder config is provided', ( expect(result).toHaveLength(2); - expect(result[0].id).toBe('metrics-default'); + expect(result[0].id).toBe(DEFAULT_METRICS_FOLDER_UUID); expect(result[0].name).toBe('Metrics'); expect(result[0].items).toHaveLength(3); expect(result[0].items[0].uuid).toBe('metric1-uuid'); expect(result[0].items[0].type).toBe(FoldersEditorItemType.Metric); - expect(result[1].id).toBe('columns-default'); + expect(result[1].id).toBe(DEFAULT_COLUMNS_FOLDER_UUID); expect(result[1].name).toBe('Columns'); expect(result[1].items).toHaveLength(3); expect(result[1].items[0].uuid).toBe('column1-uuid'); @@ -118,11 +122,11 @@ test('transforms data according to folder configuration', () => { expect(result[1].items).toHaveLength(1); expect(result[1].items[0].uuid).toBe('column1-uuid'); - expect(result[2].id).toBe('metrics-default'); + expect(result[2].id).toBe(DEFAULT_METRICS_FOLDER_UUID); expect(result[2].items).toHaveLength(1); expect(result[2].items[0].uuid).toBe('metric3-uuid'); - expect(result[3].id).toBe('columns-default'); + expect(result[3].id).toBe(DEFAULT_COLUMNS_FOLDER_UUID); expect(result[3].items).toHaveLength(2); }); diff --git a/superset-frontend/src/explore/components/DatasourcePanel/transformDatasourceFolders.ts b/superset-frontend/src/explore/components/DatasourcePanel/transformDatasourceFolders.ts index 414b7f85a62..eb94823069a 100644 --- a/superset-frontend/src/explore/components/DatasourcePanel/transformDatasourceFolders.ts +++ b/superset-frontend/src/explore/components/DatasourcePanel/transformDatasourceFolders.ts @@ -19,6 +19,10 @@ import { t } from '@apache-superset/core'; import { Metric } from '@superset-ui/core'; import { FoldersEditorItemType } from 'src/components/Datasource/types'; +import { + DEFAULT_METRICS_FOLDER_UUID, + DEFAULT_COLUMNS_FOLDER_UUID, +} from 'src/components/Datasource/FoldersEditor/constants'; import { ColumnItem, DatasourceFolder, @@ -98,10 +102,36 @@ const transformToFolderStructure = ( return folder; }; + const addUnassignedToFolder = ( + folders: Folder[], + items: (MetricItem | ColumnItem)[], + folderId: string, + folderName: string, + allItemsCount: number, + inFoldersCount: number, + ) => { + if (items.length === 0) return; + const existing = folders.find(f => f.id === folderId); + if (existing) { + existing.items.push(...items); + existing.totalItems += items.length; + existing.showingItems += items.length; + } else { + folders.push({ + id: folderId, + name: folderName, + isCollapsed: false, + items, + totalItems: allItemsCount - inFoldersCount, + showingItems: items.length, + }); + } + }; + if (!folderConfig) { return [ { - id: 'metrics-default', + id: DEFAULT_METRICS_FOLDER_UUID, name: t('Metrics'), isCollapsed: false, items: metricsToDisplay, @@ -109,7 +139,7 @@ const transformToFolderStructure = ( showingItems: metricsToDisplay.length, }, { - id: 'columns-default', + id: DEFAULT_COLUMNS_FOLDER_UUID, name: t('Columns'), isCollapsed: false, items: columnsToDisplay, @@ -128,27 +158,22 @@ const transformToFolderStructure = ( columnsMap.has(column.uuid), ); - if (unassignedMetrics.length > 0) { - folders.push({ - id: 'metrics-default', - name: t('Metrics'), - isCollapsed: false, - items: unassignedMetrics, - totalItems: allMetrics.length - metricsInFolders, - showingItems: unassignedMetrics.length, - }); - } - - if (unassignedColumns.length > 0) { - folders.push({ - id: 'columns-default', - name: t('Columns'), - isCollapsed: false, - items: unassignedColumns, - totalItems: allColumns.length - columnsInFolders, - showingItems: unassignedColumns.length, - }); - } + addUnassignedToFolder( + folders, + unassignedMetrics, + DEFAULT_METRICS_FOLDER_UUID, + t('Metrics'), + allMetrics.length, + metricsInFolders, + ); + addUnassignedToFolder( + folders, + unassignedColumns, + DEFAULT_COLUMNS_FOLDER_UUID, + t('Columns'), + allColumns.length, + columnsInFolders, + ); return folders; }; diff --git a/superset/commands/dataset/update.py b/superset/commands/dataset/update.py index be86cd876b4..3acab024b9e 100644 --- a/superset/commands/dataset/update.py +++ b/superset/commands/dataset/update.py @@ -54,6 +54,14 @@ from superset.utils.decorators import on_error, transaction logger = logging.getLogger(__name__) +# Default folder UUIDs matching the frontend constants. +# Stored as strings so comparisons work whether obj["uuid"] is str or UUID. +DEFAULT_METRICS_FOLDER_UUID = "255b537d-58c8-443d-9fc1-4e4dc75047e2" +DEFAULT_COLUMNS_FOLDER_UUID = "83a7ae8f-2e8a-4f2b-a8cb-ebaebef95b9b" +DEFAULT_FOLDER_UUIDS = frozenset( + {DEFAULT_METRICS_FOLDER_UUID, DEFAULT_COLUMNS_FOLDER_UUID} +) + class UpdateDatasetCommand(UpdateMixin, BaseCommand): def __init__( @@ -320,11 +328,19 @@ def validate_folders( # noqa: C901 raise ValidationError(f"Duplicate folder name: {name}") seen_fqns.add(fqn) - if name.lower() in {"metrics", "columns"}: + # Allow default folders (by UUID) to use reserved names + if ( + name.lower() in {"metrics", "columns"} + and str(uuid) not in DEFAULT_FOLDER_UUIDS + ): raise ValidationError(f"Folder cannot have name '{name}'") - # check if metric/column UUID exists - elif not name and uuid not in valid_uuids: + # check if metric/column UUID exists (skip default folders) + elif ( + not name + and uuid not in valid_uuids + and str(uuid) not in DEFAULT_FOLDER_UUIDS + ): raise ValidationError(f"Invalid UUID: {uuid}") # traverse children diff --git a/tests/unit_tests/commands/dataset/update_test.py b/tests/unit_tests/commands/dataset/update_test.py index 5eb26fea4a5..afc33741c51 100644 --- a/tests/unit_tests/commands/dataset/update_test.py +++ b/tests/unit_tests/commands/dataset/update_test.py @@ -29,7 +29,12 @@ from superset.commands.dataset.exceptions import ( DatasetNotFoundError, MultiCatalogDisabledValidationError, ) -from superset.commands.dataset.update import UpdateDatasetCommand, validate_folders +from superset.commands.dataset.update import ( + DEFAULT_COLUMNS_FOLDER_UUID, + DEFAULT_METRICS_FOLDER_UUID, + UpdateDatasetCommand, + validate_folders, +) from superset.commands.exceptions import OwnersNotFoundValidationError from superset.datasets.schemas import FolderSchema from superset.errors import ErrorLevel, SupersetError, SupersetErrorType @@ -564,6 +569,34 @@ def test_validate_folders_invalid_names(mocker: MockerFixture) -> None: assert str(excinfo.value) == "Folder cannot have name 'Columns'" +@with_feature_flags(DATASET_FOLDERS=True) +def test_validate_folders_allows_default_folders(mocker: MockerFixture) -> None: + """ + Test that default system folders (Metrics/Columns) are allowed when using + the well-known default folder UUIDs, so their position can be persisted. + """ + folders = cast( + list[FolderSchema], + [ + { + "uuid": DEFAULT_METRICS_FOLDER_UUID, + "type": "folder", + "name": "Metrics", + "children": [], + }, + { + "uuid": DEFAULT_COLUMNS_FOLDER_UUID, + "type": "folder", + "name": "Columns", + "children": [], + }, + ], + ) + + # Should not raise - default folders are allowed to use reserved names + validate_folders(folders=folders, valid_uuids=set()) + + @with_feature_flags(DATASET_FOLDERS=True) def test_validate_folders_invalid_uuid(mocker: MockerFixture) -> None: """