Skip to content
Snippets Groups Projects
Unverified Commit b9bc8c1c authored by Noah Moss's avatar Noah Moss Committed by GitHub
Browse files

Make sure the query to fetch result_metadata for native sandboxes doesn't use... 

Make sure the query to fetch result_metadata for native sandboxes doesn't use the streaming results handler (#50049)
parent 4d5aa805
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
Showing
with 132 additions and 139 deletions
e2e/test/scenarios/permissions/sandboxes.cy.spec.js
+ 83
− 113
View file @ b9bc8c1c
......@@ -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: {
......
enterprise/backend/src/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions.clj
+ 16
− 11
View file @ b9bc8c1c
......@@ -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)]
......
enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj
+ 14
− 0
View file @ b9bc8c1c
......@@ -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))))))))
src/metabase/query_processor/pipeline.clj
+ 8
− 3
View file @ b9bc8c1c
......@@ -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:
......
src/metabase/query_processor/streaming.clj
+ 11
− 12
View file @ b9bc8c1c
......@@ -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
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment