rusackas commented on code in PR #39461:
URL: https://github.com/apache/superset/pull/39461#discussion_r3411982515
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:
##########
@@ -883,602 +835,693 @@ 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 => ({
+ value: owner.value || owner.id,
+ label: owner.label || `${owner.first_name} ${owner.last_name}`,
+ })),
+ 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);
Review Comment:
`validate` reads `datasource.columns` on `master` too, this conversion kept
that as-is. If the duplicate check should run against the merged live state
instead, that is a pre-existing behavior worth its own PR rather than something
this lint pass introduced.
##########
superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterControl/index.tsx:
##########
@@ -243,177 +214,205 @@ class AdhocFilterControl extends Component<
});
}
}
- }
+ }, [datasource]);
- componentDidUpdate(prevProps: AdhocFilterControlProps): void {
- if (this.props.columns !== prevProps.columns) {
- this.setState({ options: optionsForSelect(this.props) });
- }
- if (this.props.value !== prevProps.value) {
- this.setState({
- values: (this.props.value || []).map(filter =>
+ useEffect(() => {
+ if (value !== undefined) {
+ setValues(
+ (value || []).map(filter =>
isDictionaryForAdhocFilter(filter) ? new AdhocFilter(filter) :
filter,
),
- });
+ );
}
- }
+ }, [value]);
- removeFilter(index: number): void {
- const valuesCopy = [...this.state.values];
- valuesCopy.splice(index, 1);
- this.setState(prevState => ({
- ...prevState,
- values: valuesCopy,
- }));
- this.props.onChange?.(valuesCopy);
- }
+ const getMetricExpression = useCallback(
+ (savedMetricName: string): string => {
+ const metric = savedMetrics?.find(
+ savedMetric => savedMetric.metric_name === savedMetricName,
+ );
+ return metric?.expression ?? '';
+ },
+ [savedMetrics],
+ );
- onRemoveFilter(index: number): void {
- const { canDelete } = this.props;
- const { values } = this.state;
- const result = canDelete?.(values[index], values);
- if (typeof result === 'string') {
- warning({ title: t('Warning'), content: result });
- return;
- }
- this.removeFilter(index);
- }
+ const mapOption = useCallback(
+ (option: FilterOption | AdhocFilter): AdhocFilter | null => {
+ // already a AdhocFilter, skip
+ if (option instanceof AdhocFilter) {
+ return option;
+ }
+ // via datasource saved metric
+ if (option.saved_metric_name) {
+ return new AdhocFilter({
+ expressionType: ExpressionTypes.Sql,
+ subject: getMetricExpression(option.saved_metric_name),
+ operator:
+ OPERATOR_ENUM_TO_OPERATOR_TYPE[Operators.GreaterThan].operation,
+ comparator: 0,
+ clause: Clauses.Having,
+ });
+ }
+ // has a custom label, meaning it's custom column
+ if (option.label) {
+ return new AdhocFilter({
+ expressionType: ExpressionTypes.Sql,
+ subject: new AdhocMetric(option).translateToSql(),
+ operator:
+ OPERATOR_ENUM_TO_OPERATOR_TYPE[Operators.GreaterThan].operation,
+ comparator: 0,
+ clause: Clauses.Having,
+ });
+ }
+ // add a new filter item
+ if (option.column_name) {
+ return new AdhocFilter({
+ expressionType: ExpressionTypes.Simple,
+ subject: option.column_name,
+ operator: OPERATOR_ENUM_TO_OPERATOR_TYPE[Operators.Equals].operation,
+ comparator: '',
+ clause: Clauses.Where,
+ isNew: true,
+ });
+ }
+ return null;
+ },
+ [getMetricExpression],
+ );
- onNewFilter(newFilter: FilterOption | AdhocFilter): void {
- const mappedOption = this.mapOption(newFilter);
- if (mappedOption) {
- this.setState(
- prevState => ({
- ...prevState,
- values: [...prevState.values, mappedOption],
- }),
- () => {
- this.props.onChange?.(this.state.values);
- },
- );
- }
- }
+ const removeFilter = useCallback(
+ (index: number) => {
+ const valuesCopy = [...values];
+ valuesCopy.splice(index, 1);
+ setValues(valuesCopy);
+ onChange?.(valuesCopy);
+ },
+ [values, onChange],
+ );
- onFilterEdit(changedFilter: AdhocFilter): void {
- this.props.onChange?.(
- this.state.values.map(value => {
- if (value.filterOptionName === changedFilter.filterOptionName) {
- return changedFilter;
- }
- return value;
- }),
- );
- }
+ const onRemoveFilter = useCallback(
+ (index: number) => {
+ const result = canDelete?.(values[index], values);
+ if (typeof result === 'string') {
+ warning({ title: t('Warning'), content: result });
+ return;
+ }
+ removeFilter(index);
Review Comment:
This is unchanged from `master`, where `onRemoveFilter` only blocks on a
string return (the warning) and removes otherwise. The existing contract treats
`false` as "no reason to warn", not "block", so the conversion is faithful here.
--
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]
