From d149840374c9e020ccb206271f4a14da8a371f45 Mon Sep 17 00:00:00 2001
From: "Mahatthana (Kelvin) Nomsawadi" <me@bboykelvin.dev>
Date: Tue, 21 Nov 2023 12:14:06 +0700
Subject: [PATCH] Fix embedding crashes when one of the cards fails to load
(#35851)
* Fix static/public embed crashes when query returns error
* Add E2E test to reproduce the issue
* Sort the key more consistently
---
.../embedding/embedding-dashboard.cy.spec.js | 81 +++++++++++++++++++
frontend/src/metabase-types/api/dataset.ts | 18 +++--
.../src/metabase-types/api/mocks/dataset.ts | 42 ++++++----
frontend/src/metabase/dashboard/utils.ts | 7 +-
.../PublicQuestion.unit.spec.tsx | 4 +-
.../test/__support__/server-mocks/public.ts | 4 +-
6 files changed, 131 insertions(+), 25 deletions(-)
diff --git a/e2e/test/scenarios/embedding/embedding-dashboard.cy.spec.js b/e2e/test/scenarios/embedding/embedding-dashboard.cy.spec.js
index 63b9b0faf77..8621c6bb83e 100644
--- a/e2e/test/scenarios/embedding/embedding-dashboard.cy.spec.js
+++ b/e2e/test/scenarios/embedding/embedding-dashboard.cy.spec.js
@@ -6,6 +6,7 @@ import {
filterWidget,
visitIframe,
getDashboardCard,
+ addOrUpdateDashboardCard,
} from "e2e/support/helpers";
import { SAMPLE_DATABASE } from "e2e/support/cypress_sample_database";
@@ -249,6 +250,86 @@ describe("scenarios > embedding > dashboard parameters", () => {
.should("be.visible");
});
});
+
+ it("should render error without crashing when embed query returns error (metabase#34954)", () => {
+ const categoryTemplateTag = {
+ type: "text",
+ name: "category",
+ id: "377a4a4a-179e-4d86-8263-f3b3887df15f",
+ "display-name": "Category",
+ };
+ const questionDetails = {
+ native: {
+ query: "Select * from products Where category = {{category}}",
+ "template-tags": {
+ category: categoryTemplateTag,
+ },
+ },
+ };
+
+ const dashboardParameter = {
+ name: "Category",
+ slug: "category",
+ id: "9cd1ee78",
+ type: "string/=",
+ sectionId: "string",
+ values_query_type: "none",
+ };
+ const dashboardDetails = {
+ name: 'dashboard with "category" parameter',
+ parameters: [dashboardParameter],
+ };
+
+ cy.createNativeQuestionAndDashboard({
+ questionDetails,
+ dashboardDetails,
+ }).then(({ body: { card_id, dashboard_id } }) => {
+ cy.wrap(dashboard_id).as("dashboardId2");
+
+ addOrUpdateDashboardCard({
+ card_id,
+ dashboard_id,
+ card: {
+ parameter_mappings: [
+ {
+ parameter_id: dashboardParameter.id,
+ card_id,
+ target: ["variable", ["template-tag", categoryTemplateTag.name]],
+ },
+ ],
+ visualization_settings: {
+ "card.hide_empty": true,
+ },
+ },
+ });
+
+ cy.request("PUT", `/api/dashboard/${dashboard_id}`, {
+ embedding_params: {
+ category: "enabled",
+ },
+ enable_embedding: true,
+ });
+
+ const payload = {
+ resource: { dashboard: dashboard_id },
+ params: {},
+ };
+
+ visitEmbeddedPage(payload);
+ });
+
+ cy.log("The whole page would have crashed before the fix at this point");
+ getDashboardCard()
+ .findByText("There was a problem displaying this chart.")
+ .should("be.visible");
+
+ cy.log("Add a filter to complete the query");
+ filterWidget().findByPlaceholderText("Category").type("Widget{enter}");
+
+ getDashboardCard()
+ .findByText("Practical Bronze Computer")
+ .should("be.visible");
+ });
});
describe("scenarios > embedding > dashboard parameters with defaults", () => {
diff --git a/frontend/src/metabase-types/api/dataset.ts b/frontend/src/metabase-types/api/dataset.ts
index 2d02e046f1f..d613d1d8e1f 100644
--- a/frontend/src/metabase-types/api/dataset.ts
+++ b/frontend/src/metabase-types/api/dataset.ts
@@ -72,7 +72,7 @@ export interface Dataset {
status?: string;
}
-export interface PublicDatasetData {
+export interface EmbedDatasetData {
rows: RowValues[];
cols: DatasetColumn[];
rows_truncated: number;
@@ -82,10 +82,18 @@ export interface PublicDatasetData {
results_timezone?: string;
}
-export interface PublicDataset {
- data: PublicDatasetData;
- json_query?: JsonQuery;
- status?: string;
+export type EmbedDataset = SuccessEmbedDataset | ErrorEmbedDataset;
+
+interface SuccessEmbedDataset {
+ data: EmbedDatasetData;
+ json_query: JsonQuery;
+ status: string;
+}
+
+export interface ErrorEmbedDataset {
+ error_type: string;
+ error: string;
+ status: string;
}
export interface NativeQueryForm {
diff --git a/frontend/src/metabase-types/api/mocks/dataset.ts b/frontend/src/metabase-types/api/mocks/dataset.ts
index aa9db7ea082..3a6c7099f88 100644
--- a/frontend/src/metabase-types/api/mocks/dataset.ts
+++ b/frontend/src/metabase-types/api/mocks/dataset.ts
@@ -2,8 +2,9 @@ import type {
Dataset,
DatasetColumn,
DatasetData,
- PublicDataset,
- PublicDatasetData,
+ EmbedDataset,
+ EmbedDatasetData,
+ ErrorEmbedDataset,
ResultsMetadata,
TemplateTag,
} from "metabase-types/api/dataset";
@@ -57,7 +58,7 @@ export const createMockDataset = ({
...opts,
});
-export const createMockPublicDatasetData = ({
+export const createMockEmbedDatasetData = ({
cols = [
createMockColumn({
display_name: "NAME",
@@ -66,7 +67,7 @@ export const createMockPublicDatasetData = ({
}),
],
...opts
-}: Partial<PublicDatasetData>): PublicDatasetData => ({
+}: Partial<EmbedDatasetData>): EmbedDatasetData => ({
rows: [],
cols,
rows_truncated: 0,
@@ -74,16 +75,29 @@ export const createMockPublicDatasetData = ({
...opts,
});
-export const createMockPublicDataset = ({
- data = {},
- ...opts
-}: MockDatasetOpts = {}): PublicDataset => ({
- data: createMockPublicDatasetData(data),
- database_id: 1,
- row_count: 0,
- running_time: 1000,
- ...opts,
-});
+export type MockEmbedDatasetOpts = Omit<Partial<EmbedDataset>, "data"> & {
+ data?: Partial<EmbedDatasetData>;
+};
+
+export const createMockEmbedDataset = (
+ opts: MockEmbedDatasetOpts,
+): EmbedDataset => {
+ if ("data" in opts) {
+ const { data = {}, ...rest } = opts;
+ return {
+ data: createMockEmbedDatasetData(data),
+ json_query: {
+ database: 1,
+ type: "native",
+ native: { query: "SELECT 1" },
+ },
+ status: "success",
+ ...rest,
+ };
+ }
+
+ return opts as ErrorEmbedDataset;
+};
export const createMockTemplateTag = (
opts?: Partial<TemplateTag>,
diff --git a/frontend/src/metabase/dashboard/utils.ts b/frontend/src/metabase/dashboard/utils.ts
index 1219757d2c4..13b66f58c87 100644
--- a/frontend/src/metabase/dashboard/utils.ts
+++ b/frontend/src/metabase/dashboard/utils.ts
@@ -19,6 +19,7 @@ import type {
Parameter,
StructuredDatasetQuery,
ActionDashboardCard,
+ EmbedDataset,
} from "metabase-types/api";
import type { SelectedTabId } from "metabase-types/store";
import Question from "metabase-lib/Question";
@@ -225,14 +226,16 @@ const isDashcardDataLoaded = (
return data != null && Object.values(data).every(result => result != null);
};
-const hasRows = (dashcardData: Record<CardId, Dataset>) => {
+const hasRows = (dashcardData: Record<CardId, Dataset | EmbedDataset>) => {
const queryResults = dashcardData
? Object.values(dashcardData).filter(Boolean)
: [];
return (
queryResults.length > 0 &&
- queryResults.every(queryResult => queryResult.data.rows.length > 0)
+ queryResults.every(
+ queryResult => "data" in queryResult && queryResult.data.rows.length > 0,
+ )
);
};
diff --git a/frontend/src/metabase/public/containers/PublicQuestion/PublicQuestion.unit.spec.tsx b/frontend/src/metabase/public/containers/PublicQuestion/PublicQuestion.unit.spec.tsx
index 91d1bd45221..1dd66aef2a8 100644
--- a/frontend/src/metabase/public/containers/PublicQuestion/PublicQuestion.unit.spec.tsx
+++ b/frontend/src/metabase/public/containers/PublicQuestion/PublicQuestion.unit.spec.tsx
@@ -9,7 +9,7 @@ import registerVisualizations from "metabase/visualizations/register";
import { createMockState } from "metabase-types/store/mocks";
import {
createMockPublicCard,
- createMockPublicDataset,
+ createMockEmbedDataset,
} from "metabase-types/api/mocks";
import { PublicQuestion } from "./PublicQuestion";
@@ -27,7 +27,7 @@ async function setup() {
);
setupPublicCardQueryEndpoints(
FAKE_UUID,
- createMockPublicDataset({
+ createMockEmbedDataset({
data: { rows: [["John W."]] },
}),
);
diff --git a/frontend/test/__support__/server-mocks/public.ts b/frontend/test/__support__/server-mocks/public.ts
index a0af23b86f9..79a1645a303 100644
--- a/frontend/test/__support__/server-mocks/public.ts
+++ b/frontend/test/__support__/server-mocks/public.ts
@@ -1,6 +1,6 @@
import fetchMock from "fetch-mock";
-import type { PublicCard, PublicDataset } from "metabase-types/api";
+import type { PublicCard, EmbedDataset } from "metabase-types/api";
export function setupPublicQuestionEndpoints(
uuid: string,
@@ -11,7 +11,7 @@ export function setupPublicQuestionEndpoints(
export function setupPublicCardQueryEndpoints(
uuid: string,
- publicDataset: PublicDataset,
+ publicDataset: EmbedDataset,
) {
fetchMock.get(`path:/api/public/card/${uuid}/query`, publicDataset);
}
--
GitLab