Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Reuse Dashboard redux data in Explore #20668

Merged
merged 1 commit into from
Jul 12, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,6 @@
* under the License.
*/

export default function isDefined(x: unknown) {
export default function isDefined<T>(x: T): x is NonNullable<T> {
return x !== null && x !== undefined;
}
6 changes: 5 additions & 1 deletion superset-frontend/src/dashboard/actions/hydrate.js
Original file line number Diff line number Diff line change
Expand Up @@ -150,9 +150,13 @@ export const hydrateDashboard =
datasource: slice.form_data.datasource,
description: slice.description,
description_markeddown: slice.description_markeddown,
owners: slice.owners,
owners: slice.owners.map(owner => owner.id),
modified: slice.modified,
changed_on: new Date(slice.changed_on).getTime(),
is_managed_externally: slice.is_managed_externally,
query_context: slice.query_context,
certified_by: slice.certified_by,
certification_details: slice.certification_details,
};

sliceIds.add(key);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ class Chart extends React.Component {
) {
window.open(url, '_blank', 'noreferrer');
} else {
this.props.history.push(url);
this.props.history.push(url, { dashboardId: this.props.dashboardId });
}
} catch (error) {
logging.error(error);
Expand Down
61 changes: 57 additions & 4 deletions superset-frontend/src/explore/ExplorePage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,24 @@
* specific language governing permissions and limitations
* under the License.
*/
import React, { useEffect, useState } from 'react';
import { useDispatch } from 'react-redux';
import { makeApi, t } from '@superset-ui/core';
import React, { useCallback, useEffect, useState } from 'react';
import { useDispatch, useSelector } from 'react-redux';
import { isDefined, makeApi, SupersetClient, t } from '@superset-ui/core';
import { useLocation } from 'react-router-dom';
import Loading from 'src/components/Loading';
import pick from 'lodash/pick';
import { Dataset } from '@superset-ui/chart-controls';
import { getParsedExploreURLParams } from './exploreUtils/getParsedExploreURLParams';
import { hydrateExplore } from './actions/hydrateExplore';
import ExploreViewContainer from './components/ExploreViewContainer';
import { ExploreResponsePayload } from './types';
import { fallbackExploreInitialData } from './fixtures';
import { addDangerToast } from '../components/MessageToasts/actions';
import { isNullish } from '../utils/common';
import { getUrlParam } from '../utils/urlUtils';
import { URL_PARAMS } from '../constants';
import { RootState } from '../dashboard/types';
import { Slice } from '../types/Chart';

const loadErrorMessage = t('Failed to load chart data.');

Expand All @@ -38,12 +45,58 @@ const fetchExploreData = () => {
})(exploreUrlParams);
};

const useExploreInitialData = (
shouldUseDashboardData: boolean,
sliceId: string | null,
) => {
const slice = useSelector<RootState, Slice | null>(({ sliceEntities }) =>
isDefined(sliceId) ? sliceEntities?.slices?.[sliceId] : null,
);
const formData = slice?.form_data;
const { id: datasourceId, type: datasourceType } = useSelector<
RootState,
{ id: number | undefined; type: string | undefined }
>(({ datasources }) =>
formData?.datasource
? pick(datasources[formData.datasource], ['id', 'type'])
: { id: undefined, type: undefined },
);
return useCallback(() => {
if (
!shouldUseDashboardData ||
!isDefined(slice) ||
!isDefined(formData) ||
!isDefined(datasourceId) ||
!isDefined(datasourceType)
) {
return fetchExploreData();
}
return SupersetClient.get({
endpoint: `/datasource/get/${datasourceType}/${datasourceId}/`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a new endpoint for the dataset GET method is /api/v1/dataset/<datasource PK(a.k.a datasource id without type>. I added it at here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually used GET /v1/dataset endpoint at first, but then changed it to GET /datasource/get/. As far as I know, Explore now supports (or will soon support?) other sources than datasets, such as SQL queries. Tagging @hughhhh for transparency.
Also, the /v1/explore/ endpoint uses DatasourceDAO to get datasource and the response from /v1/dataset endpoint is missing some fields compared to datasource from /v1/explore, which caused some components in Explore to crash

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose we stick to the legacy /datasource/get endpoint for now and create a new /v1/datasource API in the future and refactor

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kgabryje Thanks for the explanation, we will optimize it in the future.

}).then(({ json }) => ({
result: {
slice,
form_data: formData,
dataset: json as Dataset,
message: '',
},
}));
/* eslint-disable react-hooks/exhaustive-deps */
}, []);
};

const ExplorePage = () => {
const [isLoaded, setIsLoaded] = useState(false);
const dispatch = useDispatch();
const location = useLocation<{ dashboardId?: number }>();

const getExploreInitialData = useExploreInitialData(
isDefined(location.state?.dashboardId),
getUrlParam(URL_PARAMS.sliceId),
);

useEffect(() => {
fetchExploreData()
getExploreInitialData()
.then(({ result }) => {
if (isNullish(result.dataset?.id) && isNullish(result.dataset?.uid)) {
dispatch(hydrateExplore(fallbackExploreInitialData));
Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/src/explore/actions/hydrateExplore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ export const hydrateExplore =
initialFormData.dashboardId = dashboardId;
}
const initialDatasource =
datasources?.[initialFormData.datasource] ?? dataset;
dataset ?? datasources?.[initialFormData.datasource];

const initialExploreState = {
form_data: initialFormData,
Expand Down
10 changes: 10 additions & 0 deletions superset/charts/schemas.py
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@
owners_name_description = "Name of an owner of the chart."
certified_by_description = "Person or group that has certified this chart"
certification_details_description = "Details of the certification"
is_managed_externally_description = "If the chart is managed outside externally"

#
# OpenAPI method specification overrides
Expand Down Expand Up @@ -148,6 +149,10 @@
}


class UserSchema(Schema):
id = fields.Int()


class ChartEntityResponseSchema(Schema):
"""
Schema for a chart object
Expand All @@ -165,6 +170,11 @@ class ChartEntityResponseSchema(Schema):
slice_url = fields.String(description=slice_url_description)
certified_by = fields.String(description=certified_by_description)
certification_details = fields.String(description=certification_details_description)
is_managed_externally = fields.Boolean(
description=is_managed_externally_description
)
owners = fields.List(fields.Nested(UserSchema))
query_context = fields.String(description=query_context_description)


class ChartPostSchema(Schema):
Expand Down