diff --git a/e2e/test/scenarios/permissions/sandboxes.cy.spec.js b/e2e/test/scenarios/permissions/sandboxes.cy.spec.js
index 20ee10d75622b4a89547546ae74ad53a4b5d4d43..d38103eb3663365b572096b11efebf4b93e1d873 100644
--- a/e2e/test/scenarios/permissions/sandboxes.cy.spec.js
+++ b/e2e/test/scenarios/permissions/sandboxes.cy.spec.js
@@ -585,122 +585,92 @@ describeEE("formatting > sandboxes", () => {
cy.findAllByText("McClure-Lockman");
});
- /**
- * This issue (metabase-enterprise#520) has a peculiar quirk:
- * - It works ONLY if SQL question is first run (`result_metadata` builds), and then the question is saved.
- * - In a real-world scenario it is quite possible for an admin to save that SQL question without running it first. This fails!
- * (more info: https://github.com/metabase/metabase-enterprise/issues/520#issuecomment-772528159)
- *
- * That's why this test has 2 versions that reflect both scenarios. We'll call them "normal" and "workaround".
- * Until the underlying issue is fixed, "normal" scenario will be skipped.
- *
- * Related issues: metabase#10474, metabase#14629
- *
- * Update 2024/11/07:
- * It's expected that this test fails - the issue is still there.
- * See https://github.com/metabase/metabase/issues/49671 for a proposed fix.
- */
- ["normal", "workaround"].forEach(test => {
- it(
- `${test.toUpperCase()} version:\n advanced sandboxing should not ignore data model features like object detail of FK (metabase-enterprise#520)`,
- { tags: "@quarantine" },
- () => {
- cy.intercept("POST", "/api/card/*/query").as("cardQuery");
- cy.intercept("PUT", "/api/card/*").as("questionUpdate");
-
- cy.createNativeQuestion({
- name: "EE_520_Q1",
- native: {
- query:
- "SELECT * FROM ORDERS WHERE USER_ID={{sandbox}} AND TOTAL > 10",
- "template-tags": {
- sandbox: {
- "display-name": "Sandbox",
- id: "1115dc4f-6b9d-812e-7f72-b87ab885c88a",
- name: "sandbox",
- type: "number",
- },
- },
+ it("Advanced sandboxing should not ignore data model features like object detail of FK (metabase-enterprise#520)", () => {
+ cy.intercept("POST", "/api/card/*/query").as("cardQuery");
+ cy.intercept("PUT", "/api/card/*").as("questionUpdate");
+
+ cy.createNativeQuestion({
+ name: "EE_520_Q1",
+ native: {
+ query:
+ "SELECT * FROM ORDERS WHERE USER_ID={{sandbox}} AND TOTAL > 10",
+ "template-tags": {
+ sandbox: {
+ "display-name": "Sandbox",
+ id: "1115dc4f-6b9d-812e-7f72-b87ab885c88a",
+ name: "sandbox",
+ type: "number",
},
- }).then(({ body: { id: CARD_ID } }) => {
- test === "workaround"
- ? runQuestion({ question: CARD_ID, sandboxValue: "1" })
- : null;
-
- cy.sandboxTable({
- table_id: ORDERS_ID,
- card_id: CARD_ID,
- attribute_remappings: {
- attr_uid: ["variable", ["template-tag", "sandbox"]],
- },
- });
- });
-
- cy.createNativeQuestion({
- name: "EE_520_Q2",
- native: {
- query:
- "SELECT * FROM PRODUCTS WHERE CATEGORY={{sandbox}} AND PRICE > 10",
- "template-tags": {
- sandbox: {
- "display-name": "Sandbox",
- id: "3d69ba99-7076-2252-30bd-0bb8810ba895",
- name: "sandbox",
- type: "text",
- },
- },
+ },
+ },
+ }).then(({ body: { id: CARD_ID } }) => {
+ runQuestion({ question: CARD_ID, sandboxValue: "1" });
+
+ cy.sandboxTable({
+ table_id: ORDERS_ID,
+ card_id: CARD_ID,
+ attribute_remappings: {
+ attr_uid: ["variable", ["template-tag", "sandbox"]],
+ },
+ });
+ });
+
+ cy.createNativeQuestion({
+ name: "EE_520_Q2",
+ native: {
+ query:
+ "SELECT * FROM PRODUCTS WHERE CATEGORY={{sandbox}} AND PRICE > 10",
+ "template-tags": {
+ sandbox: {
+ "display-name": "Sandbox",
+ id: "3d69ba99-7076-2252-30bd-0bb8810ba895",
+ name: "sandbox",
+ type: "text",
},
- }).then(({ body: { id: CARD_ID } }) => {
- test === "workaround"
- ? runQuestion({
- question: CARD_ID,
- sandboxValue: "Widget",
- })
- : null;
-
- cy.sandboxTable({
- table_id: PRODUCTS_ID,
- card_id: CARD_ID,
- attribute_remappings: {
- attr_cat: ["variable", ["template-tag", "sandbox"]],
- },
- });
- });
-
- cy.signOut();
- cy.signInAsSandboxedUser();
-
- openOrdersTable();
-
- cy.log("Reported failing on v1.36.x");
-
- cy.log(
- "It should show remapped Display Values instead of Product ID",
- );
- cy.get("[data-testid=cell-data]")
- .contains("Awesome Concrete Shoes")
- .click();
- // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
- cy.findByText(/View details/i).click();
-
- cy.log(
- "It should show object details instead of filtering by this Product ID",
- );
- // The name of this Vendor is visible in "details" only
- cy.findByTestId("object-detail");
- cy.findAllByText("McClure-Lockman");
-
- /**
- * Helper function related to this test only!
- */
- function runQuestion({ question, sandboxValue } = {}) {
- // Run the question
- cy.visit(`/question/${question}?sandbox=${sandboxValue}`);
- // Wait for results
- cy.wait("@cardQuery");
- }
+ },
},
+ }).then(({ body: { id: CARD_ID } }) => {
+ runQuestion({ question: CARD_ID, sandboxValue: "Widget" });
+
+ cy.sandboxTable({
+ table_id: PRODUCTS_ID,
+ card_id: CARD_ID,
+ attribute_remappings: {
+ attr_cat: ["variable", ["template-tag", "sandbox"]],
+ },
+ });
+ });
+
+ cy.signOut();
+ cy.signInAsSandboxedUser();
+
+ openOrdersTable();
+
+ cy.log("Reported failing on v1.36.x");
+
+ cy.log("It should show remapped Display Values instead of Product ID");
+ cy.get("[data-testid=cell-data]")
+ .contains("Awesome Concrete Shoes")
+ .click();
+ // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
+ cy.findByText(/View details/i).click();
+
+ cy.log(
+ "It should show object details instead of filtering by this Product ID",
);
+ // The name of this Vendor is visible in "details" only
+ cy.findByTestId("object-detail");
+ cy.findAllByText("McClure-Lockman");
+
+ /**
+ * Helper function related to this test only!
+ */
+ function runQuestion({ question, sandboxValue } = {}) {
+ // Run the question
+ cy.visit(`/question/${question}?sandbox=${sandboxValue}`);
+ // Wait for results
+ cy.wait("@cardQuery");
+ }
});
it("simple sandboxing should work (metabase#14629)", () => {
@@ -1036,7 +1006,7 @@ describeEE("formatting > sandboxes", () => {
});
});
- it.skip("should be able to visit ad-hoc/dirty question when permission is granted to the linked table column, but not to the linked table itself (metabase#15105)", () => {
+ it("should be able to visit ad-hoc/dirty question when permission is granted to the linked table column, but not to the linked table itself (metabase#15105)", () => {
cy.sandboxTable({
table_id: ORDERS_ID,
attribute_remappings: {
diff --git a/enterprise/backend/src/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions.clj b/enterprise/backend/src/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions.clj
index 39653df98505a49d693075b95ee4823487e99007..5d7ff356c83bc4f1220c2898ddb1db1e54569b7c 100644
--- a/enterprise/backend/src/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions.clj
+++ b/enterprise/backend/src/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions.clj
@@ -23,6 +23,7 @@
[metabase.query-processor.error-type :as qp.error-type]
^{:clj-kondo/ignore [:deprecated-namespace]}
[metabase.query-processor.middleware.fetch-source-query-legacy :as fetch-source-query-legacy]
+ [metabase.query-processor.pipeline :as qp.pipeline]
[metabase.query-processor.store :as qp.store]
[metabase.server.middleware.session :as mw.session]
[metabase.util :as u]
@@ -173,12 +174,16 @@
(mu/defn- native-query-metadata :- [:+ :map]
[source-query :- [:map [:source-query :any]]]
- (let [result (mw.session/as-admin
- ((requiring-resolve 'metabase.query-processor/process-query)
- {:database (u/the-id (lib.metadata/database (qp.store/metadata-provider)))
- :type :query
- :query {:source-query source-query
- :limit 0}}))]
+ (let [result
+ ;; Rebind *result* in case the outer query is being streamed back to the client. The streaming code binds this
+ ;; to a custom handler, and we don't want to accidentally terminate the stream here!
+ (binding [qp.pipeline/*result* qp.pipeline/default-result-handler]
+ (mw.session/as-admin
+ ((requiring-resolve 'metabase.query-processor/process-query)
+ {:database (u/the-id (lib.metadata/database (qp.store/metadata-provider)))
+ :type :query
+ :query {:source-query source-query
+ :limit 0}})))]
(or (-> result :data :results_metadata :columns not-empty)
(throw (ex-info (tru "Error running query to determine metadata")
{:source-query source-query
@@ -196,17 +201,17 @@
(let [table-metadata (original-table-metadata table-id)
;; make sure source query has `:source-metadata`; add it if needed
[metadata save?] (cond
- ;; if it already has `:source-metadata`, we're good to go.
+ ;; if it already has `:source-metadata`, we're good to go.
(seq source-metadata)
[source-metadata false]
- ;; if it doesn't have source metadata, but it's an MBQL query, we can preprocess the query to
- ;; get the expected metadata.
+ ;; if it doesn't have source metadata, but it's an MBQL query, we can preprocess the query to
+ ;; get the expected metadata.
(not (get-in source-query [:source-query :native]))
[(mbql-query-metadata source-query) true]
- ;; otherwise if it's a native query we'll have to run the query really quickly to get the
- ;; expected metadata.
+ ;; otherwise if it's a native query we'll have to run the query really quickly to get the
+ ;; expected metadata.
:else
[(native-query-metadata source-query) true])
metadata (reconcile-metadata metadata table-metadata)]
diff --git a/enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj b/enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj
index 6aa1df3381fc874e59ae21ddda4e23b8c98189df..a5f7a310a66d8c9abec23174a4111f36ca30a07d 100644
--- a/enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj
@@ -28,6 +28,7 @@
[metabase.query-processor.pivot :as qp.pivot]
[metabase.query-processor.preprocess :as qp.preprocess]
[metabase.query-processor.store :as qp.store]
+ [metabase.query-processor.streaming.test-util :as streaming.test-util]
[metabase.query-processor.util :as qp.util]
[metabase.query-processor.util.add-alias-info :as add]
[metabase.server.middleware.session :as mw.session]
@@ -1222,3 +1223,16 @@
:attributes {"cat" 50}}
(data-perms/set-table-permission! &group (mt/id :people) :perms/view-data :blocked)
(is (= 10 (count (mt/rows (qp/process-query (mt/mbql-query venues)))))))))
+
+(deftest native-sandbox-no-query-metadata-streaming-test
+ (testing "A sandbox powered by a native query source card can be used via the streaming API even if the card has no
+ stored results_metadata (#49985)"
+ (met/with-gtaps! {:gtaps {:venues (venues-category-native-gtap-def)}
+ :attributes {"cat" 50}}
+ (let [sandbox-card-id (t2/select-one-fn :card_id
+ :model/GroupTableAccessPolicy
+ :group_id (:id &group)
+ :table_id (mt/id :venues))]
+ (is (nil? (t2/select-one-fn :result_metadata :model/Card sandbox-card-id)))
+ (is (= 10 (count (mt/rows (streaming.test-util/process-query-basic-streaming :api (mt/mbql-query venues))))))
+ (is (not (nil? (t2/select-one-fn :result_metadata :model/Card sandbox-card-id))))))))
diff --git a/src/metabase/query_processor/pipeline.clj b/src/metabase/query_processor/pipeline.clj
index 1fa230c08a240d0732ddba4744153139aac0af09..4f8cf0799cd7e4e9a0c66e56e6fcf653d0180218 100644
--- a/src/metabase/query_processor/pipeline.clj
+++ b/src/metabase/query_processor/pipeline.clj
@@ -23,14 +23,19 @@
[]
(some-> *canceled-chan* a/poll!))
-(defn ^:dynamic *result*
- "Called exactly once with the final result, which is the result of either [[*reduce*]] (if query completed
- successfully), or an Exception (if it did not)."
+(defn default-result-handler
+ "Default implementation for *result*."
[result]
(if (instance? Throwable result)
(throw result)
result))
+(defn ^:dynamic *result*
+ "Called exactly once with the final result, which is the result of either [[*reduce*]] (if query completed
+ successfully), or an Exception (if it did not)."
+ [result]
+ (default-result-handler result))
+
(defn ^:dynamic *execute*
"Called by [[*run*]] to have driver run query. By default, [[metabase.driver/execute-reducible-query]]. `respond` is a
callback with the signature:
diff --git a/src/metabase/query_processor/streaming.clj b/src/metabase/query_processor/streaming.clj
index b5342a843172c43bbc56900f17a1f8117084dc73..310ad1324afadd80d149ffd47e43d5bce223f08e 100644
--- a/src/metabase/query_processor/streaming.clj
+++ b/src/metabase/query_processor/streaming.clj
@@ -136,18 +136,17 @@
(mu/defn- streaming-result-fn :- fn?
[results-writer :- (lib.schema.common/instance-of-class metabase.query_processor.streaming.interface.StreamingResultsWriter)
^OutputStream os :- (lib.schema.common/instance-of-class OutputStream)]
- (let [orig qp.pipeline/*result*]
- (fn result [result]
- (when (= (:status result) :completed)
- (log/debug "Finished writing results; closing results writer.")
- (try
- (qp.si/finish! results-writer result)
- (catch EofException e
- (log/error e "Client closed connection prematurely")))
- (u/ignore-exceptions
- (.flush os)
- (.close os)))
- (orig result))))
+ (fn result [result]
+ (when (= (:status result) :completed)
+ (log/debug "Finished writing results; closing results writer.")
+ (try
+ (qp.si/finish! results-writer result)
+ (catch EofException e
+ (log/error e "Client closed connection prematurely")))
+ (u/ignore-exceptions
+ (.flush os)
+ (.close os)))
+ (qp.pipeline/default-result-handler result)))
(defn do-with-streaming-rff
"Context to pass to the QP to streaming results as `export-format` to an output stream. Can be used independently of