Skip to content
Permalink
Browse files Browse the repository at this point in the history
Do not autorun ad-hoc native queries (#25675) (#25732)
  • Loading branch information
ranquild committed Oct 3, 2022
1 parent 9bcc704 commit b7c6bb9
Show file tree
Hide file tree
Showing 7 changed files with 121 additions and 34 deletions.
Expand Up @@ -306,7 +306,7 @@ async function handleQBInit(
});

if (uiControls.queryBuilderMode !== "notebook") {
if (question.canRun()) {
if (question.canRun() && (question.isSaved() || question.isStructured())) {
// Timeout to allow Parameters widget to set parameterValues
setTimeout(
() => dispatch(runQuestionQuery({ shouldUpdateUrl: false })),
Expand Down
@@ -1,5 +1,4 @@
import { LocationDescriptorObject } from "history";
import _ from "underscore";
import xhrMock from "xhr-mock";

import * as CardLib from "metabase/lib/card";
Expand Down Expand Up @@ -248,12 +247,6 @@ describe("QB Actions > initializeQB", () => {
);
});

it("runs question query in view mode", async () => {
const runQuestionQuerySpy = jest.spyOn(querying, "runQuestionQuery");
await setup({ question });
expect(runQuestionQuerySpy).toHaveBeenCalledTimes(1);
});

it("does not run non-runnable question queries", async () => {
const runQuestionQuerySpy = jest.spyOn(querying, "runQuestionQuery");
jest.spyOn(Question.prototype, "canRun").mockReturnValue(false);
Expand Down Expand Up @@ -405,6 +398,12 @@ describe("QB Actions > initializeQB", () => {
),
);
});

it("runs question query in view mode", async () => {
const runQuestionQuerySpy = jest.spyOn(querying, "runQuestionQuery");
await setup({ question });
expect(runQuestionQuerySpy).toHaveBeenCalledTimes(1);
});
});
});
});
Expand Down Expand Up @@ -506,6 +505,26 @@ describe("QB Actions > initializeQB", () => {
});
});

describe("unsaved structured questions", () => {
const { question } = TEST_CASE.SAVED_STRUCTURED_QUESTION;

it("runs question query in view mode", async () => {
const runQuestionQuerySpy = jest.spyOn(querying, "runQuestionQuery");
await setup({ question });
expect(runQuestionQuerySpy).toHaveBeenCalledTimes(1);
});
});

describe("unsaved native questions", () => {
const { question } = TEST_CASE.UNSAVED_NATIVE_QUESTION;

it("doesn't run an ad-hoc native question in view mode automatically", async () => {
const runQuestionQuerySpy = jest.spyOn(querying, "runQuestionQuery");
await setup({ question });
expect(runQuestionQuerySpy).not.toHaveBeenCalled();
});
});

describe("models", () => {
MODEL_TEST_CASES.forEach(testCase => {
const { question, questionType } = testCase;
Expand Down
@@ -1,4 +1,5 @@
import { SAMPLE_DB_ID, SAMPLE_DB_TABLES } from "__support__/e2e/cypress_data";
import { runNativeQuery } from "__support__/e2e/helpers/e2e-misc-helpers";

const {
STATIC_ORDERS_ID,
Expand Down Expand Up @@ -41,18 +42,21 @@ export function startNewQuestion() {
* @param {object} question
* @param {{callback: function, mode: (undefined|"notebook")}} config
*/
export function visitQuestionAdhoc(question, { callback, mode } = {}) {
export function visitQuestionAdhoc(
question,
{ callback, mode, autorun = true } = {},
) {
const questionMode = mode === "notebook" ? "/notebook" : "";

const [url, alias] = getInterceptDetails(question, mode);
const [url, alias] = getInterceptDetails(question, mode, autorun);

cy.intercept(url).as(alias);

cy.visit(`/question${questionMode}#` + adhocQuestionHash(question));

cy.wait("@" + alias).then(xhr => {
callback && callback(xhr);
});
runQueryIfNeeded(question, autorun);

cy.wait("@" + alias).then(xhr => callback && callback(xhr));
}

/**
Expand Down Expand Up @@ -98,18 +102,25 @@ export function openReviewsTable({ mode, limit, callback } = {}) {
return openTable({ table: STATIC_REVIEWS_ID, mode, limit, callback });
}

function getInterceptDetails(question, mode) {
function getInterceptDetails(question, mode, autorun) {
const {
display,
dataset_query: { type },
} = question;

// When visiting notebook mode directly, we don't render any results to the page.
// Therefore, there is no `dataset` to wait for.
// But we need to make sure the schema for our database is loaded before we can proceed.
if (mode === "notebook") {
return [`/api/database/${SAMPLE_DB_ID}/schema/PUBLIC`, "publicSchema"];
}

const {
display,
dataset_query: { type },
} = question;
// Ad-hoc native queries are not autorun by default.
// Therefore, there is no `dataset` to wait for.
// We need to make sure data for the native query builder has loaded before we can proceed.
if (type === "native" && !autorun) {
return ["/api/native-query-snippet", "snippets"];
}

// native queries should use the normal dataset endpoint even when set to pivot
const isPivotEndpoint = display === "pivot" && type === "query";
Expand All @@ -119,3 +130,13 @@ function getInterceptDetails(question, mode) {

return [url, alias];
}

function runQueryIfNeeded(question, autorun) {
const {
dataset_query: { type },
} = question;

if (type === "native" && autorun) {
runNativeQuery({ wait: false });
}
}
10 changes: 7 additions & 3 deletions frontend/test/__support__/e2e/helpers/e2e-misc-helpers.js
Expand Up @@ -40,11 +40,15 @@ export function openNativeEditor({
/**
* Executes native query and waits for the results to load.
* Makes sure that the question is not "dirty" after the query successfully ran.
* @param {string} [xhrAlias ="dataset"]
*/
export function runNativeQuery(xhrAlias = "dataset") {
export function runNativeQuery({ wait = true } = {}) {
cy.intercept("POST", "api/dataset").as("dataset");
cy.get(".NativeQueryEditor .Icon-play").click();
cy.wait("@" + xhrAlias);

if (wait) {
cy.wait("@dataset");
}

cy.icon("play").should("not.exist");
}

Expand Down
18 changes: 18 additions & 0 deletions frontend/test/metabase/scenarios/native/native.cy.spec.js
Expand Up @@ -262,4 +262,22 @@ describe("scenarios > question > native", () => {
.and("eq", `/question/${questionId}-test-question`);
});
});

it("should not autorun ad-hoc native queries by default", () => {
visitQuestionAdhoc(
{
display: "scalar",
dataset_query: {
type: "native",
native: {
query: "SELECT 1",
},
database: SAMPLE_DB_ID,
},
},
{ autorun: false },
);

cy.findByText("Here's where your results will appear").should("be.visible");
});
});
@@ -1,35 +1,51 @@
import { restore, visitQuestion } from "__support__/e2e/helpers";
import {
restore,
visitQuestion,
visitQuestionAdhoc,
} from "__support__/e2e/helpers";
import { SAMPLE_DB_ID } from "__support__/e2e/cypress_data";

describe("scenarios > permissions", () => {
beforeEach(restore);

const PATHS = [
"/dashboard/1",
"/question/1",
"/collection/1",
"/admin",
// this url is a native query pointing at the sample db
"/question#eyJkYXRhc2V0X3F1ZXJ5Ijp7InR5cGUiOiJuYXRpdmUiLCJuYXRpdmUiOnsicXVlcnkiOiJzZWxlY3QgMSIsInRlbXBsYXRlLXRhZ3MiOnt9fSwiZGF0YWJhc2UiOjF9LCJkaXNwbGF5IjoidGFibGUiLCJ2aXN1YWxpemF0aW9uX3NldHRpbmdzIjp7fX0=",
];
const PATHS = ["/dashboard/1", "/question/1", "/collection/1", "/admin"];

for (const path of PATHS) {
it(`should display the permissions screen on ${path}`, () => {
cy.signIn("none");
cy.visit(path);
cy.icon("key");
cy.contains("Sorry, you don’t have permission to see that.");
checkUnauthorized();
});
}

it("should not allow to run adhoc native questions without permissions", () => {
cy.signIn("none");

visitQuestionAdhoc(
{
display: "scalar",
dataset_query: {
type: "native",
native: {
query: "SELECT 1",
},
database: SAMPLE_DB_ID,
},
},
{ autorun: false },
);

cy.findAllByRole("button", { name: "refresh icon" }).should("be.disabled");
});

// There's no pulse in the fixture data, so we stub out the api call to
// replace the 404 with a 403.
it("should display the permissions screen for pulses", () => {
cy.signIn("none");
cy.server();
cy.route({ url: /\/api\/pulse\/1/, status: 403, response: {} });
cy.visit("/pulse/1");
cy.icon("key");
cy.contains("Sorry, you don’t have permission to see that.");
checkUnauthorized();
});

it("should let a user with no data permissions view questions", () => {
Expand All @@ -38,3 +54,10 @@ describe("scenarios > permissions", () => {
cy.contains("February 11, 2019, 9:40 PM"); // check that the data loads
});
});

const checkUnauthorized = () => {
cy.icon("key").should("be.visible");
cy.findByText("Sorry, you don’t have permission to see that.").should(
"be.visible",
);
};
Expand Up @@ -2,6 +2,7 @@ import {
restore,
withDatabase,
adhocQuestionHash,
runNativeQuery,
} from "__support__/e2e/helpers";

const PG_DB_ID = 2;
Expand All @@ -28,6 +29,7 @@ describe("issue 11727", () => {
cy.visit(`/question#` + adhocQuestionHash(questionDetails));
cy.wait("@getDatabases");

runNativeQuery({ wait: false });
cy.findByText("Doing science...").should("be.visible");
cy.get("body").type("{cmd}{enter}");
cy.findByText("Here's where your results will appear").should(
Expand Down

0 comments on commit b7c6bb9

Please sign in to comment.