This is an automated email from the ASF dual-hosted git repository. suddjian pushed a commit to branch api-resource-hooks in repository https://gitbox.apache.org/repos/asf/superset.git
commit ea37f5ed5aabb0fadcff7fd9b9317746b9951396 Author: David Aaron Suddjian <[email protected]> AuthorDate: Thu Feb 18 01:31:37 2021 -0800 refactor: introduce api resource hooks, fix error owners --- superset-frontend/src/chart/Chart.jsx | 15 +- superset-frontend/src/chart/ChartErrorMessage.tsx | 63 +++++++ superset-frontend/src/common/hooks/apiResources.ts | 184 +++++++++++++++++++++ .../dashboard/components/gridComponents/Chart.jsx | 1 - .../src/explore/components/ExploreChartPanel.jsx | 1 - 5 files changed, 251 insertions(+), 13 deletions(-) diff --git a/superset-frontend/src/chart/Chart.jsx b/superset-frontend/src/chart/Chart.jsx index 3f1596c..8bda7a8 100644 --- a/superset-frontend/src/chart/Chart.jsx +++ b/superset-frontend/src/chart/Chart.jsx @@ -25,9 +25,9 @@ import { isFeatureEnabled, FeatureFlag } from 'src/featureFlags'; import { Logger, LOG_ACTIONS_RENDER_CHART } from '../logger/LogUtils'; import Loading from '../components/Loading'; import RefreshChartOverlay from '../components/RefreshChartOverlay'; -import ErrorMessageWithStackTrace from '../components/ErrorMessage/ErrorMessageWithStackTrace'; import ErrorBoundary from '../components/ErrorBoundary'; import ChartRenderer from './ChartRenderer'; +import { ChartErrorMessage } from './ChartErrorMessage'; const propTypes = { annotationData: PropTypes.object, @@ -49,9 +49,6 @@ const propTypes = { timeout: PropTypes.number, vizType: PropTypes.string.isRequired, triggerRender: PropTypes.bool, - owners: PropTypes.arrayOf( - PropTypes.oneOfType([PropTypes.string, PropTypes.number]), - ), // state chartAlert: PropTypes.string, chartStatus: PropTypes.string, @@ -152,17 +149,13 @@ class Chart extends React.PureComponent { } renderErrorMessage(queryResponse) { - const { chartAlert, chartStackTrace, dashboardId, owners } = this.props; + const { chartId, chartAlert, chartStackTrace, dashboardId } = this.props; const error = queryResponse?.errors?.[0]; - if (error) { - const extra = error.extra || {}; - extra.owners = owners; - error.extra = extra; - } const message = chartAlert || queryResponse?.message; return ( - <ErrorMessageWithStackTrace + <ChartErrorMessage + chartId={chartId} error={error} subtitle={message} copyText={message} diff --git a/superset-frontend/src/chart/ChartErrorMessage.tsx b/superset-frontend/src/chart/ChartErrorMessage.tsx new file mode 100644 index 0000000..123d60f --- /dev/null +++ b/superset-frontend/src/chart/ChartErrorMessage.tsx @@ -0,0 +1,63 @@ +/** + * 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 React from 'react'; +import { + useApiV1Resource, + useResourceTransform, +} from 'src/common/hooks/apiResources'; +import ErrorMessageWithStackTrace from 'src/components/ErrorMessage/ErrorMessageWithStackTrace'; +import { SupersetError } from 'src/components/ErrorMessage/types'; +import Chart from 'src/types/Chart'; + +interface Props { + chartId: string; + error?: SupersetError; +} + +function extractOwnerNames({ owners }: Chart) { + if (!owners) return null; + return owners.map(owner => `${owner.first_name} ${owner.last_name}`); +} + +function useChartOwnerNames(chartId: string) { + const { result } = useResourceTransform( + useApiV1Resource<Chart>(`/api/v1/chart/${chartId}`), + extractOwnerNames, + ); + return result; +} + +/** + * fetches the chart owners and adds them to the extra data of the error message + */ +export const ChartErrorMessage: React.FC<Props> = ({ + chartId, + error, + ...props +}) => { + const owners = useChartOwnerNames(chartId); + // don't mutate props + const ownedError = error && { + ...error, + extra: { ...error.extra, owners }, + }; + + return <ErrorMessageWithStackTrace {...props} error={ownedError} />; +}; diff --git a/superset-frontend/src/common/hooks/apiResources.ts b/superset-frontend/src/common/hooks/apiResources.ts new file mode 100644 index 0000000..5ae54a7 --- /dev/null +++ b/superset-frontend/src/common/hooks/apiResources.ts @@ -0,0 +1,184 @@ +/** + * 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 { makeApi } from '@superset-ui/core'; +import { useEffect, useMemo, useRef, useState } from 'react'; + +export enum ResourceStatus { + LOADING = 'loading', + COMPLETE = 'complete', + ERROR = 'error', +} + +/** + * An object containing the data fetched from the API, + * as well as loading and error info + */ +export type Resource<T> = LoadingState | CompleteState<T> | ErrorState; + +// Trying out something a little different: a separate type per status. +// This should let Typescript know whether a Resource has a result or error. +// It's possible that I'm expecting too much from Typescript here. +// If this ends up causing problems, we can change the type to: +// +// export type Resource<T> = { +// status: ResourceStatus; +// result: null | T; +// error: null | Error; +// } + +type LoadingState = { + status: ResourceStatus.LOADING; + result: null; + error: null; +}; + +type CompleteState<T> = { + status: ResourceStatus.COMPLETE; + result: T; + error: null; +}; + +type ErrorState = { + status: ResourceStatus.ERROR; + result: null; + error: Error; +}; + +const initialState: LoadingState = { + status: ResourceStatus.LOADING, + result: null, + error: null, +}; + +/** + * A general-purpose hook to fetch the response from an endpoint. + * Returns the full response body from the API, including metadata. + * + * Note: You likely want {useApiV1Resource} instead of this! + * + * TODO Store the state in redux or something, share state between hook instances. + * + * TODO Include a function in the returned resource object to refresh the data. + * + * A core design decision here is composition > configuration, + * and every hook should only have one job. + * Please address new needs with new hooks if possible, + * rather than adding config options to this hook. + * + * @param endpoint The url where the resource is located. + */ +export function useApiResourceFullBody<RESULT>( + endpoint: string, +): Resource<RESULT> { + const [resource, setResource] = useState<Resource<RESULT>>(initialState); + const cancelRef = useRef<() => void>(() => {}); + + useEffect(() => { + // If refresh is implemented, this will need to change. + // The state will need to keep + setResource(initialState); + + // when this effect runs, the endpoint function has changed. + // cancel any current calls so that state doesn't get messed up. + cancelRef.current(); + let cancelled = false; + cancelRef.current = () => { + cancelled = true; + }; + + const fetchResource = makeApi<{}, RESULT>({ + method: 'GET', + endpoint, + }); + + // pass {} to fetchResource, only because makeApi requires it. + // we should change makeApi, and then remove that param. + // TODO support request cancellation in makeApi + fetchResource({}) + .then(result => { + if (!cancelled) { + setResource({ + status: ResourceStatus.COMPLETE, + result, + error: null, + }); + } + }) + .catch(error => { + if (!cancelled) { + setResource({ + status: ResourceStatus.ERROR, + result: null, + error, + }); + } + }); + + // Cancel the request when the component un-mounts + return () => { + cancelled = true; + }; + }, [endpoint]); + + return resource; +} + +/** + * For when you want to transform the result of an api resource hook before using it. + * + * @param resource the Resource object returned from useApiV1Resource + * @param transformFn a callback that transforms the result object into the shape you want. + * Make sure to use a persistent function for this so it doesn't constantly recalculate! + */ +export function useResourceTransform<IN, OUT>( + resource: Resource<IN>, + transformFn: (result: IN) => OUT, +): Resource<OUT> { + return useMemo(() => { + if (resource.status !== ResourceStatus.COMPLETE) { + // While incomplete, there is no result - no need to transform. + return resource; + } + return { + ...resource, + result: transformFn(resource.result), + }; + }, [resource, transformFn]); +} + +// returns the result from fetching an api resource +const innerResult = <T>(responseBody: { result: T }) => responseBody.result; + +/** + * A general-purpose hook to fetch a Superset resource from a v1 API endpoint. + * Handles request lifecycle and async logic so you don't have to. + * + * This returns the data under the "result" field in the API response body. + * If you need the full response body, use {useFullApiResource} instead. + * + * @param endpoint The url where the resource is located. + */ +export function useApiV1Resource<RESULT>(endpoint: string): Resource<RESULT> { + // get the api "result" field within the resource result + return useResourceTransform( + useApiResourceFullBody<{ result: RESULT }>(endpoint), + innerResult, + ); +} diff --git a/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx b/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx index 7dd01e9..b3c7975 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx @@ -361,7 +361,6 @@ export default class Chart extends React.Component { timeout={timeout} triggerQuery={chart.triggerQuery} vizType={slice.viz_type} - owners={slice.owners} /> </div> </div> diff --git a/superset-frontend/src/explore/components/ExploreChartPanel.jsx b/superset-frontend/src/explore/components/ExploreChartPanel.jsx index 6962dfa..813b6a9 100644 --- a/superset-frontend/src/explore/components/ExploreChartPanel.jsx +++ b/superset-frontend/src/explore/components/ExploreChartPanel.jsx @@ -198,7 +198,6 @@ const ExploreChartPanel = props => { errorMessage={props.errorMessage} formData={props.form_data} onQuery={props.onQuery} - owners={props?.slice?.owners} queriesResponse={chart.queriesResponse} refreshOverlayVisible={props.refreshOverlayVisible} setControlValue={props.actions.setControlValue}
