Skip to content

Commit

Permalink
Enable metric preview only when an aggregation is selected
Browse files Browse the repository at this point in the history
Fixes #41928.
  • Loading branch information
metamben committed Apr 29, 2024
1 parent 93661f9 commit af266e3
Show file tree
Hide file tree
Showing 9 changed files with 55 additions and 27 deletions.
4 changes: 2 additions & 2 deletions frontend/src/metabase-lib/query.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,8 @@ export function sourceTableOrCardId(query: Query): TableId | null {
return ML.source_table_or_card_id(query);
}

export function canRun(query: Query): boolean {
return ML.can_run(query);
export function canRun(query: Query, cardType: CardType): boolean {
return ML.can_run(query, cardType);
}

export function canSave(query: Query, cardType: CardType): boolean {
Expand Down
4 changes: 3 additions & 1 deletion frontend/src/metabase-lib/v1/Question.ts
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,9 @@ class Question {
*/
canRun(): boolean {
const { isNative } = Lib.queryDisplayInfo(this.query());
return isNative ? this.legacyQuery().canRun() : Lib.canRun(this.query());
return isNative
? this.legacyQuery().canRun()
: Lib.canRun(this.query(), this.type());
}

canWrite(): boolean {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export const NotebookNativePreview = (): JSX.Element => {
const engineType = getEngineNativeType(engine);

const sourceQuery = question.query();
const canRun = Lib.canRun(sourceQuery);
const canRun = Lib.canRun(sourceQuery, question.type());
const payload = Lib.toLegacyQuery(sourceQuery);
const { data, error, isFetching } = useGetNativeDatasetQuery(payload);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,8 @@ function NotebookStep({
} = STEP_UI[step.type] || {};

const color = getColor();
const canPreview = Lib.canRun(step.query) && step.active && step.visible;
const canPreview =
Lib.canRun(step.query, step.question.type()) && step.active && step.visible;
const hasPreviewButton = !isPreviewOpen && canPreview;
const canRevert = typeof step.revert === "function" && !readOnly;

Expand Down
11 changes: 7 additions & 4 deletions src/metabase/lib/js.cljs
Original file line number Diff line number Diff line change
Expand Up @@ -2169,18 +2169,21 @@

(defn ^:export can-run
"Returns true if the query is runnable.
`card-type` is optional and defaults to \"question\".
MBQL queries are always runnable. Native queries can run when:
- Every *extra* from [[native-extras]] has a value, and
- The native query is non-empty.
> **Code health:** Healthy"
[a-query]
(lib.cache/side-channel-cache
:can-run a-query
([a-query]
(can-run a-query "question"))
([a-query card-type]
(lib.cache/side-channel-cache
(keyword "can-run" card-type) a-query
(fn [_]
(lib.core/can-run a-query))))
(lib.core/can-run a-query (keyword card-type))))))

(defn ^:export can-save
"Returns true if the query can be saved.
Expand Down
2 changes: 1 addition & 1 deletion src/metabase/lib/native.cljc
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@
(= :write (:native-permissions (lib.metadata/database query))))

(defmethod lib.query/can-run-method :mbql.stage/native
[query]
[query _card-type]
(and
(set/subset? (required-native-extras query)
(set (keys (native-extras query))))
Expand Down
23 changes: 12 additions & 11 deletions src/metabase/lib/query.cljc
Original file line number Diff line number Diff line change
Expand Up @@ -55,19 +55,26 @@

(defmulti can-run-method
"Returns whether the query is runnable based on first stage :lib/type"
(fn [query]
(fn [query _card-type]
(:lib/type (lib.util/query-stage query 0))))

(defmethod can-run-method :default
[_query]
[_query _card-type]
true)

(defmethod can-run-method :mbql.stage/mbql
[query card-type]
(or (not= card-type :metric)
(let [last-stage (lib.util/query-stage query -1)]
(= (-> last-stage :aggregation count) 1))))

(mu/defn can-run :- :boolean
"Returns whether the query is runnable. Manually validate schema for cljs."
[query :- ::lib.schema/query]
[query :- ::lib.schema/query
card-type :- ::lib.schema.metadata/card.type]
(and (binding [lib.schema.expression/*suppress-expression-type-check?* true]
(mr/validate ::lib.schema/query query))
(boolean (can-run-method query))))
(boolean (can-run-method query card-type))))

(defmulti can-save-method
"Returns whether the query can be saved based on first stage :lib/type."
Expand All @@ -78,18 +85,12 @@
[_query _card-type]
true)

(defmethod can-save-method :mbql.stage/mbql
[query card-type]
(or (not= card-type :metric)
(let [last-stage (lib.util/query-stage query -1)]
(= (-> last-stage :aggregation count) 1))))

(mu/defn can-save :- :boolean
"Returns whether `query` for a card of `card-type` can be saved."
[query :- ::lib.schema/query
card-type :- ::lib.schema.metadata/card.type]
(and (lib.metadata/editable? query)
(can-run query)
(can-run query card-type)
(boolean (can-save-method query card-type))))

(mu/defn query-with-stages :- ::lib.schema/query
Expand Down
13 changes: 8 additions & 5 deletions test/metabase/lib/native_test.cljc
Original file line number Diff line number Diff line change
Expand Up @@ -300,19 +300,22 @@
:name "foo"
:widget-type :text
:display-name "foo"
:dimension [:field {:lib/uuid (str (random-uuid))} 1]}})))
(is (lib/can-run lib.tu/venues-query))
:dimension [:field {:lib/uuid (str (random-uuid))} 1]}})
:question))
(is (lib/can-run lib.tu/venues-query :question))
(mu/disable-enforcement
(is (not (lib/can-run (lib/native-query meta/metadata-provider ""))))
(is (not (lib/can-run (lib/native-query meta/metadata-provider "") :question)))
(is (not (lib/can-run (lib/with-template-tags
(lib/native-query meta/metadata-provider "select * {{foo}}")
{"foo" {:type :dimension
:id "1"
:name "foo"
:widget-type :text
:display-name "foo"}}))))
:display-name "foo"}})
:question)))
(is (not (lib/can-run (update-in (lib/native-query (metadata-provider-requiring-collection) "select * {{foo}}" nil {:collection "foobar"})
[:stages 0] dissoc :collection))))))
[:stages 0] dissoc :collection)
:question)))))

(deftest ^:parallel engine-test
(is (= :h2 (lib/engine lib.tu/native-query))))
Expand Down
20 changes: 19 additions & 1 deletion test/metabase/lib/query_test.cljc
Original file line number Diff line number Diff line change
Expand Up @@ -188,10 +188,28 @@
(lib.query/query meta/metadata-provider
{:database 74001, :type :query, :query {:source-table 74040}})))))

(deftest ^:parallel can-run-test
(mu/disable-enforcement
(are [can-run? card-type query]
(= can-run? (lib.query/can-run query card-type))
true :question lib.tu/venues-query
false :question (assoc lib.tu/venues-query :database nil) ; database unknown - no permissions
true :question (lib/native-query meta/metadata-provider "SELECT")
false :question (lib/native-query meta/metadata-provider "")
false :metric lib.tu/venues-query
true :metric (-> lib.tu/venues-query
(lib/aggregate (lib/count)))
false :metric (-> lib.tu/venues-query
(lib/aggregate (lib/count))
(lib/aggregate (lib/sum (meta/field-metadata :venues :id))))
true :metric (-> lib.tu/venues-query
(lib/aggregate (lib/count))
(lib/breakout (first (lib/breakoutable-columns lib.tu/venues-query)))))))

(deftest ^:parallel can-save-test
(mu/disable-enforcement
(are [can-save? card-type query]
(= can-save? (lib.query/can-save query card-type))
(= can-save? (lib.query/can-save query card-type))
true :question lib.tu/venues-query
false :question (assoc lib.tu/venues-query :database nil) ; database unknown - no permissions
true :question (lib/native-query meta/metadata-provider "SELECT")
Expand Down

0 comments on commit af266e3

Please sign in to comment.