rusackas commented on code in PR #39461:
URL: https://github.com/apache/superset/pull/39461#discussion_r3481829898
##########
superset-frontend/src/explore/components/controls/DatasourceControl/index.tsx:
##########
@@ -237,413 +216,388 @@ const preventRouterLinkWhileMetaClicked = (evt:
React.MouseEvent) => {
}
};
-class DatasourceControl extends PureComponent<
- DatasourceControlProps,
- DatasourceControlState
-> {
- static defaultProps = defaultProps;
-
- constructor(props: DatasourceControlProps) {
- super(props);
- this.state = {
- showEditDatasourceModal: false,
- showChangeDatasourceModal: false,
- showSaveDatasetModal: false,
- };
- }
-
- onDatasourceSave = (datasource: Datasource) => {
- // Cast to ExtendedDatasource for the component's internal use
- this.props.actions.changeDatasource(datasource as ExtendedDatasource);
- // Cast datasource for getTemporalColumns which expects Dataset |
QueryResponse
- const { temporalColumns, defaultTemporalColumn } = getTemporalColumns(
- datasource as Parameters<typeof getTemporalColumns>[0],
- );
- const { columns } = datasource;
- // the current granularity_sqla might not be a temporal column anymore
- const timeCol = this.props.form_data?.granularity_sqla;
- const isGranularitySqlaTemporal = columns.find(
- ({ column_name }) => column_name === timeCol,
- )?.is_dttm;
- // the current main_dttm_col might not be a temporal column anymore
- const isDefaultTemporal = columns.find(
- ({ column_name }) => column_name === defaultTemporalColumn,
- )?.is_dttm;
-
- // if the current granularity_sqla is empty or it is not a temporal column
anymore
- // let's update the control value
- if (datasource.type === 'table' && !isGranularitySqlaTemporal) {
- const temporalColumn = isDefaultTemporal
- ? defaultTemporalColumn
- : temporalColumns?.[0];
- this.props.actions.setControlValue(
- 'granularity_sqla',
- temporalColumn || null,
+export default function DatasourceControl({
+ actions,
+ onChange = () => {},
+ value = null,
+ datasource,
+ form_data,
+ isEditable = true,
+ onDatasourceSave = null,
+ user,
+}: DatasourceControlProps) {
+ const theme = useTheme();
+
+ const [showEditDatasourceModal, setShowEditDatasourceModal] =
useState(false);
+ const [showChangeDatasourceModal, setShowChangeDatasourceModal] =
+ useState(false);
+ const [showSaveDatasetModal, setShowSaveDatasetModal] = useState(false);
+
+ const handleDatasourceSave = useCallback(
+ (savedDatasource: Datasource) => {
+ // Cast to ExtendedDatasource for the component's internal use
+ actions.changeDatasource(savedDatasource as ExtendedDatasource);
+ // Cast datasource for getTemporalColumns which expects Dataset |
QueryResponse
+ const { temporalColumns, defaultTemporalColumn } = getTemporalColumns(
+ savedDatasource as Parameters<typeof getTemporalColumns>[0],
);
- }
+ const { columns } = savedDatasource;
+ // the granularity_sqla might not be a temporal column anymore
+ const timeCol = form_data?.granularity_sqla;
+ const isGranularitySqlaTemporal = columns.find(
+ ({ column_name }) => column_name === timeCol,
+ )?.is_dttm;
Review Comment:
This `columns.find(...)` mirrors master verbatim - the class version did the
same destructure without a guard, so it predates the conversion rather than
being introduced here. Out of scope for the mechanical port.
##########
superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.tsx:
##########
@@ -90,170 +90,201 @@ const TabTitle = styled.span`
text-transform: none;
`;
-const AddTabIconWrapper = styled.span`
- display: inline-flex;
- vertical-align: middle;
-`;
-
// Get the user's OS
const userOS = detectOS();
type TabbedSqlEditorsProps = ReturnType<typeof mergeProps>;
-class TabbedSqlEditors extends PureComponent<TabbedSqlEditorsProps> {
- constructor(props: TabbedSqlEditorsProps) {
- super(props);
- this.removeQueryEditor = this.removeQueryEditor.bind(this);
- this.handleSelect = this.handleSelect.bind(this);
- this.handleEdit = this.handleEdit.bind(this);
- }
+function TabbedSqlEditors({
+ actions,
+ queryEditors = DEFAULT_PROPS.queryEditors,
+ queries,
+ tabHistory,
+ displayLimit,
+ offline = DEFAULT_PROPS.offline,
+ defaultQueryLimit,
+ maxRow,
+ saveQueryWarning = DEFAULT_PROPS.saveQueryWarning,
+ scheduleQueryWarning = DEFAULT_PROPS.scheduleQueryWarning,
+}: TabbedSqlEditorsProps) {
+ const activeQueryEditor = useMemo(() => {
+ if (tabHistory.length === 0) {
+ return queryEditors[0];
+ }
+ const qeid = tabHistory[tabHistory.length - 1];
+ return queryEditors.find(qe => qe.id === qeid) || null;
+ }, [tabHistory, queryEditors]);
+
+ // Track the last persisted resultsKey we fetched, so the effect retries when
+ // the active query editor resolves after mount (or its latest query changes)
+ // but dedupes when the same resultsKey has already been fetched.
+ const fetchedResultsKeyRef = useRef<string | null>(null);
- componentDidMount() {
- const qe = this.activeQueryEditor();
- const latestQuery = this.props.queries[qe?.latestQueryId || ''];
+ // Fetch query results for the active editor's latest query when its
+ // persisted resultsKey changes (equivalent to componentDidMount, but
resilient
+ // to async hydration of activeQueryEditor).
+ useEffect(() => {
+ const latestQuery = queries[activeQueryEditor?.latestQueryId || ''];
+ const resultsKey = latestQuery?.resultsKey;
if (
isFeatureEnabled(FeatureFlag.SqllabBackendPersistence) &&
- latestQuery?.resultsKey
+ resultsKey &&
+ fetchedResultsKeyRef.current !== resultsKey
) {
+ fetchedResultsKeyRef.current = resultsKey;
// when results are not stored in localStorage they need to be
// fetched from the results backend (if configured)
- this.props.actions.fetchQueryResults(
- latestQuery,
- this.props.displayLimit,
- );
+ actions.fetchQueryResults(latestQuery, displayLimit);
}
- }
+ }, [queries, activeQueryEditor, actions, displayLimit]);
- activeQueryEditor() {
- if (this.props.tabHistory.length === 0) {
- return this.props.queryEditors[0];
- }
- const qeid = this.props.tabHistory[this.props.tabHistory.length - 1];
- return this.props.queryEditors.find(qe => qe.id === qeid) || null;
- }
+ const newQueryEditor = useCallback(() => {
+ actions.addNewQueryEditor();
+ }, [actions]);
- newQueryEditor() {
- this.props.actions.addNewQueryEditor();
- }
+ const removeQueryEditor = useCallback(
+ (qe: QueryEditor) => {
+ actions.removeQueryEditor(qe);
+ },
+ [actions],
+ );
- handleSelect(key: string) {
- const qeid = this.props.tabHistory[this.props.tabHistory.length - 1];
- if (key !== qeid) {
- const queryEditor = this.props.queryEditors.find(qe => qe.id === key);
- if (!queryEditor) {
- return;
+ const handleSelect = useCallback(
+ (key: string) => {
+ const qeid = tabHistory[tabHistory.length - 1];
+ if (key !== qeid) {
+ const queryEditor = queryEditors.find(qe => qe.id === key);
+ if (!queryEditor) {
+ return;
+ }
+ actions.setActiveQueryEditor(queryEditor);
}
- this.props.actions.setActiveQueryEditor(queryEditor);
- }
- }
+ },
+ [tabHistory, queryEditors, actions],
+ );
- handleEdit(key: string, action: string) {
- if (action === 'remove') {
- const qe = this.props.queryEditors.find(qe => qe.id === key);
- if (qe) {
- this.removeQueryEditor(qe);
+ const handleEdit = useCallback(
+ (key: string, action: string) => {
+ if (action === 'remove') {
+ const qe = queryEditors.find(qe => qe.id === key);
+ if (qe) {
+ removeQueryEditor(qe);
+ }
}
- }
- if (action === 'add') {
- Logger.markTimeOrigin();
- this.newQueryEditor();
- }
- }
-
- removeQueryEditor(qe: QueryEditor) {
- this.props.actions.removeQueryEditor(qe);
- }
+ if (action === 'add') {
+ Logger.markTimeOrigin();
+ newQueryEditor();
+ }
+ },
+ [queryEditors, removeQueryEditor, newQueryEditor],
+ );
- onTabClicked = () => {
+ const onTabClicked = useCallback(() => {
Logger.markTimeOrigin();
- const noQueryEditors = this.props.queryEditors?.length === 0;
+ const noQueryEditors = queryEditors?.length === 0;
if (noQueryEditors) {
- this.newQueryEditor();
+ newQueryEditor();
}
+ }, [queryEditors, newQueryEditor]);
Review Comment:
The empty-state `onTabClicked` path matches master - the class
`onTabClicked` also called `newQueryEditor()` with no `offline` check. Not
something this conversion changed.
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:
##########
@@ -883,602 +869,701 @@ const mapStateToProps = (state: RootState) => ({
const connector = connect(mapStateToProps, mapDispatchToProps);
type PropsFromRedux = ConnectedProps<typeof connector>;
-type DatasourceEditorProps = DatasourceEditorOwnProps &
- PropsFromRedux & {
- theme?: SupersetTheme;
- };
-
-class DatasourceEditor extends PureComponent<
- DatasourceEditorProps,
- DatasourceEditorState
-> {
- private isComponentMounted: boolean;
+type DatasourceEditorProps = DatasourceEditorOwnProps & PropsFromRedux;
+
+function DatasourceEditor({
+ datasource: propsDatasource,
+ onChange = () => {},
+ addSuccessToast,
+ addDangerToast,
+ setIsEditing = () => {},
+ database,
+ runQuery,
+ resetQuery,
+ formatQuery: formatQueryAction,
+}: DatasourceEditorProps) {
+ const theme = useTheme();
+ const isComponentMounted = useRef(false);
+ const isInitialMount = useRef(true);
+ const prevPropsDatasourceRef = useRef(propsDatasource);
+ const isSyncingColumnsFromProps = useRef(false);
+ const abortControllers = useRef<AbortControllers>({
+ formatQuery: null,
+ formatSql: null,
+ syncMetadata: null,
+ fetchUsageData: null,
+ });
+
+ // Initialize datasource state with transformed owners and metrics
+ const [datasource, setDatasource] = useState<DatasourceObject>(() => ({
+ ...propsDatasource,
+ owners: propsDatasource.owners.map(owner => {
+ const ownerName = owner.label || `${owner.first_name}
${owner.last_name}`;
+ return {
+ value: owner.value || owner.id,
+ label: OwnerSelectLabel({
+ name: typeof ownerName === 'string' ? ownerName : '',
+ email: owner.email,
+ }),
+ [OWNER_TEXT_LABEL_PROP]: typeof ownerName === 'string' ? ownerName :
'',
+ [OWNER_EMAIL_PROP]: owner.email ?? '',
+ };
+ }),
+ metrics: propsDatasource.metrics?.map(metric => {
+ const {
+ certified_by: certifiedByMetric,
+ certification_details: certificationDetails,
+ } = metric;
+ const {
+ certification: {
+ details = undefined,
+ certified_by: certifiedBy = undefined,
+ } = {},
+ warning_markdown: warningMarkdown,
+ } = JSON.parse(metric.extra || '{}') || {};
+ return {
+ ...metric,
+ certification_details: certificationDetails || details,
+ warning_markdown: warningMarkdown || '',
+ certified_by: certifiedBy || certifiedByMetric,
+ };
+ }),
+ }));
- private abortControllers: AbortControllers;
+ const [errors, setErrors] = useState<string[]>([]);
+ const [isSqla] = useState(
+ propsDatasource.datasource_type === 'table' ||
+ propsDatasource.type === 'table',
+ );
+ const [isEditMode, setIsEditMode] = useState(false);
+ const [databaseColumns, setDatabaseColumns] = useState<Column[]>(
+ propsDatasource.columns.filter(col => !col.expression),
+ );
+ const [calculatedColumns, setCalculatedColumns] = useState<Column[]>(
+ propsDatasource.columns.filter(col => !!col.expression),
+ );
+ const [folders, setFolders] = useState<DatasourceFolder[]>(
+ propsDatasource.folders || [],
+ );
+ const [folderCount, setFolderCount] = useState(() => {
+ const savedFolders = propsDatasource.folders || [];
+ const savedCount = countAllFolders(savedFolders);
+ const hasDefaultsSaved = savedFolders.some(f => isDefaultFolder(f.uuid));
+ return savedCount + (hasDefaultsSaved ? 0 : DEFAULT_FOLDERS_COUNT);
+ });
+ const [metadataLoading, setMetadataLoading] = useState(false);
+ const [activeTabKey, setActiveTabKey] = useState(TABS_KEYS.SOURCE);
+ const [datasourceType, setDatasourceType] = useState(
+ propsDatasource.sql
+ ? DATASOURCE_TYPES.virtual.key
+ : DATASOURCE_TYPES.physical.key,
+ );
+ const [usageCharts, setUsageCharts] = useState<ChartUsageData[]>([]);
+ const [usageChartsCount, setUsageChartsCount] = useState(0);
+ const [metricSearchTerm, setMetricSearchTerm] = useState('');
+ const [columnSearchTerm, setColumnSearchTerm] = useState('');
+ const [calculatedColumnSearchTerm, setCalculatedColumnSearchTerm] =
+ useState('');
+
+ const findDuplicates = useCallback(
+ <T,>(arr: T[], accessor: (obj: T) => string): string[] => {
+ const seen: Record<string, null> = {};
+ const dups: string[] = [];
+ arr.forEach((obj: T) => {
+ const item = accessor(obj);
+ if (item in seen) {
+ dups.push(item);
+ } else {
+ seen[item] = null;
+ }
+ });
+ return dups;
+ },
+ [],
+ );
- static defaultProps = {
- onChange: () => {},
- setIsEditing: () => {},
- };
+ const validate = useCallback(
+ (callback: (validationErrors: string[]) => void) => {
+ let validationErrors: string[] = [];
+ let dups: string[];
- constructor(props: DatasourceEditorProps) {
- super(props);
- this.state = {
- datasource: {
- ...props.datasource,
- owners: props.datasource.owners.map(owner => {
- const ownerName =
- owner.label || `${owner.first_name} ${owner.last_name}`;
- return {
- value: owner.value || owner.id,
- label: OwnerSelectLabel({
- name: typeof ownerName === 'string' ? ownerName : '',
- email: owner.email,
- }),
- [OWNER_TEXT_LABEL_PROP]:
- typeof ownerName === 'string' ? ownerName : '',
- [OWNER_EMAIL_PROP]: owner.email ?? '',
- };
- }),
- metrics: props.datasource.metrics?.map(metric => {
- const {
- certified_by: certifiedByMetric,
- certification_details: certificationDetails,
- } = metric;
- const {
- certification: {
- details = undefined,
- certified_by: certifiedBy = undefined,
- } = {},
- warning_markdown: warningMarkdown,
- } = JSON.parse(metric.extra || '{}') || {};
- return {
- ...metric,
- certification_details: certificationDetails || details,
- warning_markdown: warningMarkdown || '',
- certified_by: certifiedBy || certifiedByMetric,
- };
- }),
- },
- errors: [],
- isSqla:
- props.datasource.datasource_type === 'table' ||
- props.datasource.type === 'table',
- isEditMode: false,
- databaseColumns: props.datasource.columns.filter(col => !col.expression),
- calculatedColumns: props.datasource.columns.filter(
- col => !!col.expression,
- ),
- folders: props.datasource.folders || [],
- 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
- ? DATASOURCE_TYPES.virtual.key
- : DATASOURCE_TYPES.physical.key,
- usageCharts: [],
- usageChartsCount: 0,
- metricSearchTerm: '',
- columnSearchTerm: '',
- calculatedColumnSearchTerm: '',
- };
+ // Looking for duplicate column_name
+ dups = findDuplicates(datasource.columns, obj => obj.column_name);
+ validationErrors = validationErrors.concat(
+ dups.map(name => t('Column name [%s] is duplicated', name)),
+ );
- this.isComponentMounted = false;
- this.abortControllers = {
- formatQuery: null,
- formatSql: null,
- syncMetadata: null,
- fetchUsageData: null,
- };
+ // Looking for duplicate metric_name
+ dups = findDuplicates(datasource.metrics ?? [], obj => obj.metric_name);
+ validationErrors = validationErrors.concat(
+ dups.map(name => t('Metric name [%s] is duplicated', name)),
+ );
- this.onChange = this.onChange.bind(this);
- this.onChangeEditMode = this.onChangeEditMode.bind(this);
- this.onDatasourcePropChange = this.onDatasourcePropChange.bind(this);
- this.onDatasourceChange = this.onDatasourceChange.bind(this);
- this.tableChangeAndSyncMetadata =
- this.tableChangeAndSyncMetadata.bind(this);
- this.syncMetadata = this.syncMetadata.bind(this);
- this.setColumns = this.setColumns.bind(this);
- this.validateAndChange = this.validateAndChange.bind(this);
- this.handleTabSelect = this.handleTabSelect.bind(this);
- this.formatSql = this.formatSql.bind(this);
- this.fetchUsageData = this.fetchUsageData.bind(this);
- this.handleFoldersChange = this.handleFoldersChange.bind(this);
- }
+ // Making sure calculatedColumns have an expression defined
+ const noFilterCalcCols = calculatedColumns.filter(
+ col => !col.expression && !col.json,
+ );
+ validationErrors = validationErrors.concat(
+ noFilterCalcCols.map(col =>
+ t('Calculated column [%s] requires an expression', col.column_name),
+ ),
+ );
- onChange() {
- // Emptying SQL if "Physical" radio button is selected
- // Currently the logic to know whether the source is
- // physical or virtual is based on whether SQL is empty or not.
- const { datasourceType, datasource } = this.state;
- const sql =
- datasourceType === DATASOURCE_TYPES.physical.key ? '' : datasource.sql;
-
- const columns = [
- ...this.state.databaseColumns,
- ...this.state.calculatedColumns,
- ];
-
- // Remove deleted column/metric references from folders
- const validUuids = new Set<string>();
- for (const col of columns) {
- if (col.uuid) validUuids.add(col.uuid);
- }
- for (const metric of datasource.metrics ?? []) {
- if (metric.uuid) validUuids.add(metric.uuid);
- }
- const folders = filterFoldersByValidUuids(this.state.folders, validUuids);
+ // validate currency code (skip 'AUTO' - it's a placeholder for
auto-detection)
+ try {
+ datasource.metrics?.forEach(
+ metric =>
+ metric.currency?.symbol &&
+ metric.currency.symbol !== 'AUTO' &&
+ new Intl.NumberFormat('en-US', {
+ style: 'currency',
+ currency: metric.currency.symbol,
+ }),
+ );
+ } catch {
+ validationErrors = validationErrors.concat([
+ t('Invalid currency code in saved metrics'),
+ ]);
+ }
- const newDatasource = {
- ...this.state.datasource,
- sql,
- columns,
- folders,
- };
+ // Validate folders
+ if (folders?.length > 0) {
+ const folderValidation = validateFolders(folders);
+ validationErrors = validationErrors.concat(folderValidation.errors);
+ }
- this.props.onChange?.(newDatasource, this.state.errors);
- }
+ setErrors(validationErrors);
+ callback(validationErrors);
+ },
+ [datasource, calculatedColumns, folders, findDuplicates],
+ );
- onChangeEditMode() {
- this.props.setIsEditing?.(!this.state.isEditMode);
- this.setState(prevState => ({ isEditMode: !prevState.isEditMode }));
- }
+ const onChangeInternal = useCallback(
+ (validationErrors: string[] = errors) => {
+ // Emptying SQL if "Physical" radio button is selected
+ const sql =
+ datasourceType === DATASOURCE_TYPES.physical.key ? '' : datasource.sql;
- onDatasourceChange(
- datasource: DatasourceObject,
- callback: () => void = this.validateAndChange,
- ) {
- this.setState({ datasource }, callback);
- }
+ const columns = [...databaseColumns, ...calculatedColumns];
- onDatasourcePropChange(attr: string, value: unknown) {
- if (value === undefined) return; // if value is undefined do not update
state
- const datasource = { ...this.state.datasource, [attr]: value };
- this.setState(
- prevState => ({
- datasource: { ...prevState.datasource, [attr]: value },
- }),
- () =>
- attr === 'table_name'
- ? this.onDatasourceChange(datasource,
this.tableChangeAndSyncMetadata)
- : this.onDatasourceChange(datasource, this.validateAndChange),
- );
- }
+ // Remove deleted column/metric references from folders
+ const validUuids = new Set<string>();
+ for (const col of columns) {
+ if (col.uuid) validUuids.add(col.uuid);
+ }
+ for (const metric of datasource.metrics ?? []) {
+ if (metric.uuid) validUuids.add(metric.uuid);
+ }
+ const filteredFolders = filterFoldersByValidUuids(folders, validUuids);
- onDatasourceTypeChange(datasourceType: string) {
- // Call onChange after setting datasourceType to ensure
- // SQL is cleared when switching to a physical dataset
- this.setState({ datasourceType }, this.onChange);
- }
+ const newDatasource = {
+ ...datasource,
+ sql,
+ columns,
+ folders: filteredFolders,
+ };
- handleFoldersChange(folders: DatasourceFolder[]) {
- const folderCount = countAllFolders(folders);
- this.setState({ folders, folderCount }, () => {
- this.onDatasourceChange({
- ...this.state.datasource,
- folders,
- });
- });
- }
+ onChange(newDatasource, validationErrors);
+ },
+ [
+ datasource,
+ datasourceType,
+ databaseColumns,
+ calculatedColumns,
+ folders,
+ errors,
+ onChange,
+ ],
+ );
- setColumns(
- obj: { databaseColumns?: Column[] } | { calculatedColumns?: Column[] },
- ) {
- // update calculatedColumns or databaseColumns
- this.setState(
- obj as Pick<
- DatasourceEditorState,
- 'databaseColumns' | 'calculatedColumns'
- >,
- this.validateAndChange,
- );
- }
+ const validateAndChange = useCallback(() => {
+ validate(onChangeInternal);
+ }, [validate, onChangeInternal]);
- validateAndChange() {
- this.validate(this.onChange);
- }
+ const onDatasourceChange = useCallback((newDatasource: DatasourceObject) => {
+ setDatasource(newDatasource);
+ }, []);
- async onQueryRun() {
- const databaseId = this.state.datasource.database?.id;
- const { sql } = this.state.datasource;
- if (!databaseId || !sql) {
- return;
- }
- this.props.runQuery({
- client_id: this.props.database?.clientId,
- database_id: databaseId,
- runAsync: false,
- catalog: this.state.datasource.catalog,
- schema: this.state.datasource.schema,
- sql,
- tmp_table_name: '',
- select_as_cta: false,
- ctas_method: 'TABLE',
- queryLimit: 25,
- expand_data: true,
+ const onDatasourcePropChange = useCallback((attr: string, value: unknown) =>
{
+ if (value === undefined) return;
+ setDatasource(prev => {
+ const newDatasource = { ...prev, [attr]: value };
+ return newDatasource;
});
Review Comment:
`if (value === undefined) return;` is carried over unchanged from the class
version on master. Whatever the catalog/schema-clear implications, they predate
this port.
--
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]
