This is an automated email from the ASF dual-hosted git repository.
elizabeth pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 87110ebce4 fix: dashboard performance (#28609)
87110ebce4 is described below
commit 87110ebce45a6b75891e30c876dd50e4e8c5dbab
Author: Daniel Vaz Gaspar <danielvazgas...@gmail.com>
AuthorDate: Tue May 28 21:09:05 2024 +0100
fix: dashboard performance (#28609)
Co-authored-by: Elizabeth Thompson <eschu...@gmail.com>
Co-authored-by: Joe Li <j...@preset.io>
---
pyproject.toml | 2 +-
requirements/base.txt | 7 +-
requirements/development.txt | 11 +-
.../src/features/dashboards/DashboardCard.test.tsx | 158 +++++++++++++++++++++
.../src/features/dashboards/DashboardCard.tsx | 39 ++++-
.../src/pages/DashboardList/DashboardList.test.jsx | 2 +-
.../src/pages/DashboardList/index.tsx | 26 ++++
superset-frontend/src/views/CRUD/hooks.test.tsx | 105 ++++++++++++++
superset-frontend/src/views/CRUD/hooks.ts | 2 +
9 files changed, 338 insertions(+), 14 deletions(-)
diff --git a/pyproject.toml b/pyproject.toml
index af6a15dbac..2e20fae77b 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -45,7 +45,7 @@ dependencies = [
"cryptography>=42.0.4, <43.0.0",
"deprecation>=2.1.0, <2.2.0",
"flask>=2.2.5, <3.0.0",
- "flask-appbuilder>=4.4.1, <5.0.0",
+ "flask-appbuilder>=4.5.0, <5.0.0",
"flask-caching>=2.1.0, <3",
"flask-compress>=1.13, <2.0",
"flask-talisman>=1.0.0, <2.0",
diff --git a/requirements/base.txt b/requirements/base.txt
index 6a68e50524..1b19d3a920 100644
--- a/requirements/base.txt
+++ b/requirements/base.txt
@@ -15,8 +15,6 @@ apispec[yaml]==6.3.0
# via flask-appbuilder
apsw==3.46.0.0
# via shillelagh
-async-timeout==4.0.3
- # via redis
attrs==23.2.0
# via
# cattrs
@@ -93,8 +91,6 @@ dnspython==2.6.1
# via email-validator
email-validator==2.1.1
# via flask-appbuilder
-exceptiongroup==1.2.1
- # via cattrs
flask==2.3.3
# via
# apache-superset
@@ -109,7 +105,7 @@ flask==2.3.3
# flask-session
# flask-sqlalchemy
# flask-wtf
-flask-appbuilder==4.4.1
+flask-appbuilder==4.5.0
# via apache-superset
flask-babel==2.0.0
# via flask-appbuilder
@@ -360,7 +356,6 @@ typing-extensions==4.12.0
# via
# alembic
# apache-superset
- # cattrs
# flask-limiter
# limits
# shillelagh
diff --git a/requirements/development.txt b/requirements/development.txt
index 5fdc102ea0..5b99fd81b6 100644
--- a/requirements/development.txt
+++ b/requirements/development.txt
@@ -10,6 +10,8 @@
# via
# -r requirements/base.in
# -r requirements/development.in
+appnope==0.1.4
+ # via ipython
astroid==3.1.0
# via pylint
boto3==1.34.112
@@ -175,7 +177,9 @@ protobuf==4.23.0
psycopg2-binary==2.9.6
# via apache-superset
pure-sasl==0.6.2
- # via thrift-sasl
+ # via
+ # pyhive
+ # thrift-sasl
pydata-google-auth==1.7.0
# via pandas-gbq
pydruid==0.6.9
@@ -184,7 +188,7 @@ pyee==11.0.1
# via playwright
pyfakefs==5.3.5
# via apache-superset
-pyhive[presto]==0.7.0
+pyhive[hive_pure_sasl]==0.7.0
# via apache-superset
pyinstrument==4.4.0
# via apache-superset
@@ -228,10 +232,9 @@ tableschema==1.20.10
thrift==0.16.0
# via
# apache-superset
+ # pyhive
# thrift-sasl
thrift-sasl==0.4.3
- # via apache-superset
-tomli==2.0.1
# via
# build
# coverage
diff --git a/superset-frontend/src/features/dashboards/DashboardCard.test.tsx
b/superset-frontend/src/features/dashboards/DashboardCard.test.tsx
new file mode 100644
index 0000000000..f1080c0e1f
--- /dev/null
+++ b/superset-frontend/src/features/dashboards/DashboardCard.test.tsx
@@ -0,0 +1,158 @@
+/**
+ * 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 { MemoryRouter } from 'react-router-dom';
+import { FeatureFlag, SupersetClient } from '@superset-ui/core';
+import * as uiCore from '@superset-ui/core';
+
+import { render, screen, waitFor } from 'spec/helpers/testing-library';
+
+import DashboardCard from './DashboardCard';
+
+const mockDashboard = {
+ id: 1,
+ dashboard_title: 'Sample Dashboard',
+ certified_by: 'John Doe',
+ certification_details: 'Certified on 2022-01-01',
+ published: true,
+ url: '/dashboard/1',
+ thumbnail_url: '/thumbnails/1.png',
+ changed_on_delta_humanized: '2 days ago',
+ owners: [
+ { id: 1, name: 'Alice', first_name: 'Alice', last_name: 'Doe' },
+ { id: 2, name: 'Bob', first_name: 'Bob', last_name: 'Smith' },
+ ],
+ changed_by_name: 'John Doe',
+ changed_by: 'john....@example.com',
+};
+
+const mockHasPerm = jest.fn().mockReturnValue(true);
+const mockOpenDashboardEditModal = jest.fn();
+const mockSaveFavoriteStatus = jest.fn();
+const mockHandleBulkDashboardExport = jest.fn();
+const mockOnDelete = jest.fn();
+
+let isFeatureEnabledMock: jest.MockInstance<boolean, [feature: FeatureFlag]>;
+
+beforeAll(() => {
+ isFeatureEnabledMock = jest
+ .spyOn(uiCore, 'isFeatureEnabled')
+ .mockImplementation(() => true);
+});
+
+afterAll(() => {
+ // @ts-ignore
+ isFeatureEnabledMock.mockClear();
+});
+
+beforeEach(() => {
+ render(
+ <MemoryRouter>
+ <DashboardCard
+ dashboard={mockDashboard}
+ hasPerm={mockHasPerm}
+ bulkSelectEnabled={false}
+ loading={false}
+ openDashboardEditModal={mockOpenDashboardEditModal}
+ saveFavoriteStatus={mockSaveFavoriteStatus}
+ favoriteStatus={false}
+ handleBulkDashboardExport={mockHandleBulkDashboardExport}
+ onDelete={mockOnDelete}
+ />
+ </MemoryRouter>,
+ );
+});
+
+it('Renders the dashboard title', () => {
+ const titleElement = screen.getByText('Sample Dashboard');
+ expect(titleElement).toBeInTheDocument();
+});
+
+it('Renders the certification details', () => {
+ const certificationDetailsElement = screen.getByLabelText(/certified/i);
+ expect(certificationDetailsElement).toBeInTheDocument();
+});
+
+it('Renders the published status', () => {
+ const publishedElement = screen.getByText(/published/i);
+ expect(publishedElement).toBeInTheDocument();
+});
+
+it('Renders the modified date', () => {
+ const modifiedDateElement = screen.getByText('Modified 2 days ago');
+ expect(modifiedDateElement).toBeInTheDocument();
+});
+
+it('should fetch thumbnail when dashboard has no thumbnail URL and feature
flag is enabled', async () => {
+ const mockGet = jest.spyOn(SupersetClient, 'get').mockResolvedValue({
+ response: new Response(
+ JSON.stringify({ thumbnail_url: '/new-thumbnail.png' }),
+ ),
+ json: () => Promise.resolve({ thumbnail_url: '/new-thumbnail.png' }),
+ });
+ const { rerender } = render(
+ <DashboardCard
+ dashboard={{
+ id: 1,
+ thumbnail_url: '',
+ changed_by_name: '',
+ changed_by: '',
+ dashboard_title: '',
+ published: false,
+ url: '',
+ owners: [],
+ }}
+ hasPerm={() => true}
+ bulkSelectEnabled={false}
+ loading={false}
+ saveFavoriteStatus={() => {}}
+ favoriteStatus={false}
+ handleBulkDashboardExport={() => {}}
+ onDelete={() => {}}
+ />,
+ );
+ await waitFor(() => {
+ expect(mockGet).toHaveBeenCalledWith({
+ endpoint: '/api/v1/dashboard/1',
+ });
+ });
+ rerender(
+ <DashboardCard
+ dashboard={{
+ id: 1,
+ thumbnail_url: '/new-thumbnail.png',
+ changed_by_name: '',
+ changed_by: '',
+ dashboard_title: '',
+ published: false,
+ url: '',
+ owners: [],
+ }}
+ hasPerm={() => true}
+ bulkSelectEnabled={false}
+ loading={false}
+ saveFavoriteStatus={() => {}}
+ favoriteStatus={false}
+ handleBulkDashboardExport={() => {}}
+ onDelete={() => {}}
+ />,
+ );
+ mockGet.mockRestore();
+});
diff --git a/superset-frontend/src/features/dashboards/DashboardCard.tsx
b/superset-frontend/src/features/dashboards/DashboardCard.tsx
index e977c62f52..a494a92d3c 100644
--- a/superset-frontend/src/features/dashboards/DashboardCard.tsx
+++ b/superset-frontend/src/features/dashboards/DashboardCard.tsx
@@ -16,9 +16,15 @@
* specific language governing permissions and limitations
* under the License.
*/
-import React from 'react';
+import React, { useEffect, useState } from 'react';
import { Link, useHistory } from 'react-router-dom';
-import { isFeatureEnabled, FeatureFlag, t, useTheme } from '@superset-ui/core';
+import {
+ isFeatureEnabled,
+ FeatureFlag,
+ t,
+ useTheme,
+ SupersetClient,
+} from '@superset-ui/core';
import { CardStyles } from 'src/views/CRUD/utils';
import { AntdDropdown } from 'src/components';
import { Menu } from 'src/components/Menu';
@@ -62,6 +68,35 @@ function DashboardCard({
const canExport = hasPerm('can_export');
const theme = useTheme();
+
+ const [thumbnailUrl, setThumbnailUrl] = useState<string | null>(null);
+ const [fetchingThumbnail, setFetchingThumbnail] = useState<boolean>(false);
+
+ useEffect(() => {
+ // fetch thumbnail only if it's not already fetched
+ if (
+ !fetchingThumbnail &&
+ dashboard.id &&
+ (thumbnailUrl === undefined || thumbnailUrl === null) &&
+ isFeatureEnabled(FeatureFlag.Thumbnails)
+ ) {
+ // fetch thumbnail
+ if (dashboard.thumbnail_url) {
+ // set to empty string if null so that we don't
+ // keep fetching the thumbnail
+ setThumbnailUrl(dashboard.thumbnail_url || '');
+ return;
+ }
+ setFetchingThumbnail(true);
+ SupersetClient.get({
+ endpoint: `/api/v1/dashboard/${dashboard.id}`,
+ }).then(({ json = {} }) => {
+ setThumbnailUrl(json.thumbnail_url || '');
+ setFetchingThumbnail(false);
+ });
+ }
+ }, [dashboard, thumbnailUrl]);
+
const menu = (
<Menu>
{canEdit && openDashboardEditModal && (
diff --git a/superset-frontend/src/pages/DashboardList/DashboardList.test.jsx
b/superset-frontend/src/pages/DashboardList/DashboardList.test.jsx
index fe307ee791..8ba60ca285 100644
--- a/superset-frontend/src/pages/DashboardList/DashboardList.test.jsx
+++ b/superset-frontend/src/pages/DashboardList/DashboardList.test.jsx
@@ -162,7 +162,7 @@ describe('DashboardList', () => {
const callsD = fetchMock.calls(/dashboard\/\?q/);
expect(callsD).toHaveLength(1);
expect(callsD[0][0]).toMatchInlineSnapshot(
-
`"http://localhost/api/v1/dashboard/?q=(order_column:changed_on_delta_humanized,order_direction:desc,page:0,page_size:25)"`,
+
`"http://localhost/api/v1/dashboard/?q=(order_column:changed_on_delta_humanized,order_direction:desc,page:0,page_size:25,select_columns:!(id,dashboard_title,published,url,slug,changed_by,changed_on_delta_humanized,owners.id,owners.first_name,owners.last_name,owners,tags.id,tags.name,tags.type,status,certified_by,certification_details,changed_on))"`,
);
});
diff --git a/superset-frontend/src/pages/DashboardList/index.tsx
b/superset-frontend/src/pages/DashboardList/index.tsx
index 2e5a5a2cd8..2c80e094f3 100644
--- a/superset-frontend/src/pages/DashboardList/index.tsx
+++ b/superset-frontend/src/pages/DashboardList/index.tsx
@@ -111,6 +111,27 @@ const Actions = styled.div`
color: ${({ theme }) => theme.colors.grayscale.base};
`;
+const DASHBOARD_COLUMNS_TO_FETCH = [
+ 'id',
+ 'dashboard_title',
+ 'published',
+ 'url',
+ 'slug',
+ 'changed_by',
+ 'changed_on_delta_humanized',
+ 'owners.id',
+ 'owners.first_name',
+ 'owners.last_name',
+ 'owners',
+ 'tags.id',
+ 'tags.name',
+ 'tags.type',
+ 'status',
+ 'certified_by',
+ 'certification_details',
+ 'changed_on',
+];
+
function DashboardList(props: DashboardListProps) {
const { addDangerToast, addSuccessToast, user } = props;
@@ -135,6 +156,11 @@ function DashboardList(props: DashboardListProps) {
'dashboard',
t('dashboard'),
addDangerToast,
+ undefined,
+ undefined,
+ undefined,
+ undefined,
+ DASHBOARD_COLUMNS_TO_FETCH,
);
const dashboardIds = useMemo(() => dashboards.map(d => d.id), [dashboards]);
const [saveFavoriteStatus, favoriteStatus] = useFavoriteStatus(
diff --git a/superset-frontend/src/views/CRUD/hooks.test.tsx
b/superset-frontend/src/views/CRUD/hooks.test.tsx
new file mode 100644
index 0000000000..94b03fd72a
--- /dev/null
+++ b/superset-frontend/src/views/CRUD/hooks.test.tsx
@@ -0,0 +1,105 @@
+/**
+ * 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 { renderHook } from '@testing-library/react-hooks';
+import { JsonResponse, SupersetClient } from '@superset-ui/core';
+
+import { useListViewResource } from './hooks';
+
+describe('useListViewResource', () => {
+ afterEach(() => {
+ jest.restoreAllMocks();
+ });
+
+ it('should fetch data with correct query parameters', async () => {
+ const pageIndex = 0; // Declare and initialize the pageIndex variable
+ const pageSize = 10; // Declare and initialize the pageSize variable
+ const baseFilters = [{ id: 'status', operator: 'equals', value: 'active'
}];
+
+ const fetchSpy = jest.spyOn(SupersetClient, 'get').mockResolvedValue({
+ json: {
+ result: {
+ dashboard_title: 'New Title',
+ slug: '/new',
+ json_metadata: '{"something":"foo"}',
+ owners: [{ id: 1, first_name: 'Al', last_name: 'Pacino' }],
+ roles: [],
+ },
+ },
+ } as unknown as JsonResponse);
+
+ const { result } = renderHook(() =>
+ useListViewResource('example', 'Example', jest.fn()),
+ );
+ result.current.fetchData({
+ pageIndex,
+ pageSize,
+ sortBy: [{ id: 'foo' }], // Change the type of sortBy from string to
SortColumn[]
+ filters: baseFilters,
+ });
+
+ expect(fetchSpy).toHaveBeenNthCalledWith(2, {
+ endpoint:
+
'/api/v1/example/?q=(filters:!((col:status,opr:equals,value:active)),order_column:foo,order_direction:asc,page:0,page_size:10)',
+ });
+ });
+
+ it('should pass the selectColumns to the fetch call', async () => {
+ const pageIndex = 0; // Declare and initialize the pageIndex variable
+ const pageSize = 10; // Declare and initialize the pageSize variable
+ const baseFilters = [{ id: 'status', operator: 'equals', value: 'active'
}];
+ const selectColumns = ['id', 'name'];
+
+ const fetchSpy = jest.spyOn(SupersetClient, 'get').mockResolvedValue({
+ json: {
+ result: {
+ dashboard_title: 'New Title',
+ slug: '/new',
+ json_metadata: '{"something":"foo"}',
+ owners: [{ id: 1, first_name: 'Al', last_name: 'Pacino' }],
+ roles: [],
+ },
+ },
+ } as unknown as JsonResponse);
+
+ const { result } = renderHook(() =>
+ useListViewResource(
+ 'example',
+ 'Example',
+ jest.fn(),
+ undefined,
+ undefined,
+ undefined,
+ undefined,
+ selectColumns,
+ ),
+ );
+
+ result.current.fetchData({
+ pageIndex,
+ pageSize,
+ sortBy: [{ id: 'foo' }], // Change the type of sortBy from string to
SortColumn[]
+ filters: baseFilters,
+ });
+
+ expect(fetchSpy).toHaveBeenNthCalledWith(2, {
+ endpoint:
+
'/api/v1/example/?q=(filters:!((col:status,opr:equals,value:active)),order_column:foo,order_direction:asc,page:0,page_size:10,select_columns:!(id,name))',
+ });
+ });
+});
diff --git a/superset-frontend/src/views/CRUD/hooks.ts
b/superset-frontend/src/views/CRUD/hooks.ts
index 129734fc8b..6fa167d934 100644
--- a/superset-frontend/src/views/CRUD/hooks.ts
+++ b/superset-frontend/src/views/CRUD/hooks.ts
@@ -77,6 +77,7 @@ export function useListViewResource<D extends object = any>(
defaultCollectionValue: D[] = [],
baseFilters?: FilterValue[], // must be memoized
initialLoadingState = true,
+ selectColumns?: string[],
) {
const [state, setState] = useState<ListViewResourceState<D>>({
count: 0,
@@ -162,6 +163,7 @@ export function useListViewResource<D extends object = any>(
page: pageIndex,
page_size: pageSize,
...(filterExps.length ? { filters: filterExps } : {}),
+ ...(selectColumns?.length ? { select_columns: selectColumns } : {}),
});
return SupersetClient.get({
