diff --git a/superset-frontend/src/chart/Chart.jsx b/superset-frontend/src/chart/Chart.jsx index 520378a391..d13a6e59a1 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 ( - = ({ + chartId, + error, + ...props +}) => { + const { result: owners } = useChartOwnerNames(chartId); + + // don't mutate props + const ownedError = error && { + ...error, + extra: { ...error.extra, owners }, + }; + + return ; +}; diff --git a/superset-frontend/src/common/hooks/apiResources/apiResources.test.ts b/superset-frontend/src/common/hooks/apiResources/apiResources.test.ts new file mode 100644 index 0000000000..6a5478e3bb --- /dev/null +++ b/superset-frontend/src/common/hooks/apiResources/apiResources.test.ts @@ -0,0 +1,165 @@ +/** + * 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 { act, renderHook } from '@testing-library/react-hooks'; +import { + ResourceStatus, + useApiResourceFullBody, + useApiV1Resource, + useTransformedResource, +} from './apiResources'; + +const fakeApiResult = { + id: 1, + name: 'fake api result', +}; + +const nameToAllCaps = (thing: any) => ({ + ...thing, + name: thing.name.toUpperCase(), +}); + +jest.mock('@superset-ui/core', () => ({ + ...jest.requireActual('@superset-ui/core'), + makeApi: jest + .fn() + .mockReturnValue(jest.fn().mockResolvedValue(fakeApiResult)), +})); + +describe('apiResource hooks', () => { + beforeAll(() => { + jest.useFakeTimers(); + }); + + afterAll(() => { + jest.useRealTimers(); + }); + + describe('useApiResourceFullBody', () => { + it('returns a loading state at the start', async () => { + const { result } = renderHook(() => + useApiResourceFullBody('/test/endpoint'), + ); + expect(result.current).toEqual({ + status: ResourceStatus.LOADING, + result: null, + error: null, + }); + await act(async () => { + jest.runAllTimers(); + }); + }); + + it('resolves to the value from the api', async () => { + const { result } = renderHook(() => + useApiResourceFullBody('/test/endpoint'), + ); + await act(async () => { + jest.runAllTimers(); + }); + expect(result.current).toEqual({ + status: ResourceStatus.COMPLETE, + result: fakeApiResult, + error: null, + }); + }); + + it('handles api errors', async () => { + const fakeError = new Error('fake api error'); + (makeApi as any).mockReturnValue(jest.fn().mockRejectedValue(fakeError)); + const { result } = renderHook(() => + useApiResourceFullBody('/test/endpoint'), + ); + await act(async () => { + jest.runAllTimers(); + }); + expect(result.current).toEqual({ + status: ResourceStatus.ERROR, + result: null, + error: fakeError, + }); + }); + }); + + describe('useTransformedResource', () => { + it('applies a transformation to the resource', () => { + const { result } = renderHook(() => + useTransformedResource( + { + status: ResourceStatus.COMPLETE, + result: fakeApiResult, + error: null, + }, + nameToAllCaps, + ), + ); + expect(result.current).toEqual({ + status: ResourceStatus.COMPLETE, + result: { + id: 1, + name: 'FAKE API RESULT', + }, + error: null, + }); + }); + + it('works while loading', () => { + const nameToAllCaps = (thing: any) => ({ + ...thing, + name: thing.name.toUpperCase(), + }); + const { result } = renderHook(() => + useTransformedResource( + { + status: ResourceStatus.LOADING, + result: null, + error: null, + }, + nameToAllCaps, + ), + ); + expect(result.current).toEqual({ + status: ResourceStatus.LOADING, + result: null, + error: null, + }); + }); + }); + + describe('useApiV1Endpoint', () => { + it('resolves to the value from the api', async () => { + (makeApi as any).mockReturnValue( + jest.fn().mockResolvedValue({ + meta: 'data', + count: 1, + result: fakeApiResult, + }), + ); + const { result } = renderHook(() => useApiV1Resource('/test/endpoint')); + await act(async () => { + jest.runAllTimers(); + }); + expect(result.current).toEqual({ + status: ResourceStatus.COMPLETE, + result: fakeApiResult, + error: null, + }); + }); + }); +}); diff --git a/superset-frontend/src/common/hooks/apiResources/apiResources.ts b/superset-frontend/src/common/hooks/apiResources/apiResources.ts new file mode 100644 index 0000000000..99504c7f31 --- /dev/null +++ b/superset-frontend/src/common/hooks/apiResources/apiResources.ts @@ -0,0 +1,181 @@ +/** + * 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 = LoadingState | CompleteState | 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 = { +// status: ResourceStatus; +// result: null | T; +// error: null | Error; +// } + +type LoadingState = { + status: ResourceStatus.LOADING; + result: null; + error: null; +}; + +type CompleteState = { + 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( + endpoint: string, +): Resource { + const [resource, setResource] = useState>(initialState); + const cancelRef = useRef<() => void>(() => {}); + + useEffect(() => { + // If refresh is implemented, this will need to change. + // The previous values should stay during refresh. + setResource(initialState); + + // when this effect runs, the endpoint 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, + }); + + 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 useTransformedResource( + resource: Resource, + transformFn: (result: IN) => OUT, +): Resource { + 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" field from a fetched API v1 endpoint +const extractInnerResult = (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(endpoint: string): Resource { + return useTransformedResource( + useApiResourceFullBody<{ result: RESULT }>(endpoint), + extractInnerResult, + ); +} diff --git a/superset-frontend/src/common/hooks/apiResources/charts.ts b/superset-frontend/src/common/hooks/apiResources/charts.ts new file mode 100644 index 0000000000..8b5f109183 --- /dev/null +++ b/superset-frontend/src/common/hooks/apiResources/charts.ts @@ -0,0 +1,39 @@ +/** + * 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 rison from 'rison'; +import Chart from 'src/types/Chart'; +import { useApiV1Resource, useTransformedResource } from './apiResources'; + +function extractOwnerNames({ owners }: Chart) { + if (!owners) return null; + return owners.map(owner => `${owner.first_name} ${owner.last_name}`); +} + +const ownerNamesQuery = rison.encode({ + columns: ['owners.first_name', 'owners.last_name'], + keys: ['none'], +}); + +export function useChartOwnerNames(chartId: string) { + return useTransformedResource( + useApiV1Resource(`/api/v1/chart/${chartId}?q=${ownerNamesQuery}`), + extractOwnerNames, + ); +} diff --git a/superset-frontend/src/common/hooks/apiResources/index.ts b/superset-frontend/src/common/hooks/apiResources/index.ts new file mode 100644 index 0000000000..8befc73735 --- /dev/null +++ b/superset-frontend/src/common/hooks/apiResources/index.ts @@ -0,0 +1,29 @@ +/** + * 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. + */ + +export { + useApiResourceFullBody, + useApiV1Resource, + useTransformedResource, +} from './apiResources'; + +// A central catalog of API Resource hooks. +// Add new API hooks here, organized under +// different files for different resource types. +export { useChartOwnerNames } from './charts'; diff --git a/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx b/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx index b02897782b..822999446e 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx @@ -364,7 +364,6 @@ export default class Chart extends React.Component { timeout={timeout} triggerQuery={chart.triggerQuery} vizType={slice.viz_type} - owners={slice.owners} /> diff --git a/superset-frontend/src/explore/components/ExploreChartPanel.jsx b/superset-frontend/src/explore/components/ExploreChartPanel.jsx index 12fb784173..e84b5c4301 100644 --- a/superset-frontend/src/explore/components/ExploreChartPanel.jsx +++ b/superset-frontend/src/explore/components/ExploreChartPanel.jsx @@ -199,7 +199,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}