refactor: Introduce api resource hooks, fetch owners for chart errors (#13218)
* refactor: introduce api resource hooks, fix error owners
* comment
* vestigial comment
* not a function
* clarification
* vestigial comment
* filter the api for a leaner response
* reorganize code, add resource hook catalog
* better names
* implement state management in redux
* Revert "implement state management in redux"
This reverts commit d64d50a99f70e85e010fc6160d93fe74a6fe01eb.
* add tests for hooks
diff --git a/superset-frontend/src/chart/Chart.jsx b/superset-frontend/src/chart/Chart.jsx
index 520378a..d13a6e5 100644
--- a/superset-frontend/src/chart/Chart.jsx
+++ b/superset-frontend/src/chart/Chart.jsx
@@ -25,9 +25,9 @@
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 @@
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 @@
}
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..f6c44ad
--- /dev/null
+++ b/superset-frontend/src/chart/ChartErrorMessage.tsx
@@ -0,0 +1,47 @@
+/**
+ * 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 { useChartOwnerNames } from 'src/common/hooks/apiResources';
+import ErrorMessageWithStackTrace from 'src/components/ErrorMessage/ErrorMessageWithStackTrace';
+import { SupersetError } from 'src/components/ErrorMessage/types';
+
+interface Props {
+ chartId: string;
+ error?: SupersetError;
+}
+
+/**
+ * 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 { result: 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/apiResources.test.ts b/superset-frontend/src/common/hooks/apiResources/apiResources.test.ts
new file mode 100644
index 0000000..6a5478e
--- /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<any>('@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 0000000..99504c7
--- /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<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 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<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" field from a fetched API v1 endpoint
+const extractInnerResult = <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> {
+ 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 0000000..8b5f109
--- /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<Chart>(`/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 0000000..8befc73
--- /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 b028977..8229994 100644
--- a/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx
+++ b/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx
@@ -364,7 +364,6 @@
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 12fb784..e84b5c4 100644
--- a/superset-frontend/src/explore/components/ExploreChartPanel.jsx
+++ b/superset-frontend/src/explore/components/ExploreChartPanel.jsx
@@ -199,7 +199,6 @@
errorMessage={props.errorMessage}
formData={props.form_data}
onQuery={props.onQuery}
- owners={props?.slice?.owners}
queriesResponse={chart.queriesResponse}
refreshOverlayVisible={props.refreshOverlayVisible}
setControlValue={props.actions.setControlValue}
