Skip to content

Commit

Permalink
Support dashboard params for Cards with nested queries (metabase#12531)
Browse files Browse the repository at this point in the history 
* Fix metabase#12501

* Make sure Query results_metadata comes back with inferred base type from annotate

* Support filtering against questions that use another question as their source [ci drivers]

* Test fixes 🔧

* Lint fix [ci drivers]

* Clean up metabase.query-processor.parameters.mbql-test

* Tweak codecov requirements [ci drivers]
  • Loading branch information
@camsaul
camsaul authored May 14, 2020
1 parent e2e3691 commit 85d29f2
Show file tree
Hide file tree
Showing 19 changed files with 625 additions and 486 deletions.
5 changes: 2 additions & 3 deletions backend/mbql/src/metabase/mbql/util.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
[amount :as t.amount]
[core :as t.core]]
[metabase.mbql.schema :as mbql.s]
[metabase.mbql.schema.helpers :as mbql.s.helpers]
[metabase.mbql.util.match :as mbql.match]
[metabase.util
[i18n :refer [tru]]
Expand Down Expand Up @@ -418,9 +419,7 @@
:else
source-table-id))

(s/defn unwrap-field-clause :- (s/if (partial is-clause? :field-id)
mbql.s/field-id
mbql.s/field-literal)
(s/defn unwrap-field-clause :- (mbql.s.helpers/one-of mbql.s/field-id mbql.s/field-literal)
"Un-wrap a `Field` clause and return the lowest-level clause it wraps, either a `:field-id` or `:field-literal`."
[clause :- mbql.s/Field]
(match-one clause
Expand Down
8 changes: 6 additions & 2 deletions codecov.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,11 @@ coverage:
status:
project:
default:
# New code must have minimum 80% test coverage
target: 75%
# Project must always have at least 78% coverage (by line)
target: 78%
# Whole-project test coverage is allowed to drop up to 5%. (For situtations where we delete code with full coverage)
threshold: 5%
patch:
default:
# Changes must have at least 75% test coverage (by line)
target: 75%
4 changes: 2 additions & 2 deletions frontend/src/metabase-lib/lib/metadata/Table.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export default class Table extends Base {
entity_type: ?EntityType;

hasSchema(): boolean {
return (this.schema_name && this.db.schemas.length > 1) || false;
return (this.schema_name && this.db && this.db.schemas.length > 1) || false;
}

// $FlowFixMe Could be replaced with hydrated database property in selectors/metadata.js (instead / in addition to `table.db`)
Expand All @@ -52,7 +52,7 @@ export default class Table extends Base {

question(): Question {
return Question.create({
databaseId: this.db.id,
databaseId: this.db && this.db.id,
tableId: this.id,
metadata: this.metadata,
});
Expand Down
9 changes: 7 additions & 2 deletions frontend/src/metabase/parameters/components/Parameters.jsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import type {
} from "metabase/meta/types/Parameter";

import type { DashboardWithCards } from "metabase/meta/types/Dashboard";
import Dimension from "metabase-lib/lib/Dimension";
import type Field from "metabase-lib/lib/metadata/Field";
import type Metadata from "metabase-lib/lib/metadata/Metadata";

Expand Down Expand Up @@ -81,9 +82,13 @@ export default class Parameters extends Component {
// widget, we should start with an array to match.
value = [value];
}
// field IDs can be either ["field-id", <id>] or ["field-literal", <name>, <type>]
const fieldIds = parameter.field_ids || [];
// $FlowFixMe
const fields = fieldIds.map(id => metadata.field(id));
const fields = fieldIds.map(
id =>
// $FlowFixMe
metadata.field(id) || Dimension.parseMBQL(id, metadata).field(),
);
// $FlowFixMe
setParameterValue(parameter.id, parseQueryParam(value, fields));
}
Expand Down
16 changes: 14 additions & 2 deletions modules/drivers/mongo/src/metabase/driver/mongo/query_processor.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -602,10 +602,22 @@
handle-limit
handle-page]))

(defn- query->collection-name
"Return `:collection` from a source query, if it exists."
[query]
(mbql.u/match-one query
(_ :guard (every-pred map? :collection))
;; ignore source queries inside `:joins` or `:collection` outside of a `:source-query`
(when (and (= (last &parents) :source-query)
(not (contains? (set &parents) :joins)))
(:collection &match))))

(defn mbql->native
"Process and run an MBQL query."
[{{source-table-id :source-table} :query, :as query}]
(let [{source-table-name :name} (qp.store/table source-table-id)]
[query]
(let [source-table-name (if-let [source-table-id (mbql.u/query->source-table-id query)]
(:name (qp.store/table source-table-id))
(query->collection-name query))]
(binding [*query* query]
(let [{proj :projections, generated-pipeline :query} (generate-aggregation-pipeline (:query query))]
(log-aggregation-pipeline generated-pipeline)
Expand Down
27 changes: 27 additions & 0 deletions modules/drivers/mongo/test/metabase/driver/mongo/query_processor_test.clj
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
(ns metabase.driver.mongo.query-processor-test
(:require [clojure.test :refer :all]
[metabase.driver.mongo.query-processor :as mongo.qp]))

(deftest query->collection-name-test
(testing "query->collection-name"
(testing "should be able to extract :collection from :source-query")
(is (= "checkins"
(#'mongo.qp/query->collection-name {:query {:source-query
{:collection "checkins"
:native []}}})))
(testing "should work for nested-nested queries"
(is (= "checkins"
(#'mongo.qp/query->collection-name {:query {:source-query {:source-query
{:collection "checkins"
:native []}}}}))))

(testing "should ignore :joins"
(is (= nil
(#'mongo.qp/query->collection-name {:query {:source-query
{:native []}
:joins [{:source-query "wow"}]}}))))

(testing "should ignore other :collection keys"
(is (= nil
(#'mongo.qp/query->collection-name {:query {:source-query
{:native [{:collection "wow"}]}}}))))))
60 changes: 29 additions & 31 deletions modules/drivers/mongo/test/metabase/driver/mongo_test.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,9 @@
[metabase.models
[field :refer [Field]]
[table :as table :refer [Table]]]
[metabase.test.data :as data]
[metabase.test.data.interface :as tx]
[taoensso.nippy :as nippy]
[toucan.db :as db]
[toucan.util.test :as tt])
[toucan.db :as db])
(:import org.bson.types.ObjectId))

;; ## Constants + Helper Fns/Macros
Expand Down Expand Up @@ -83,7 +81,7 @@
(-> (qp/process-query {:native {:query native-query
:collection "venues"}
:type :native
:database (data/id)})
:database (mt/id)})
(m/dissoc-in [:data :results_metadata] [:data :insights]))))))

;; ## Tests for individual syncing functions
Expand All @@ -94,7 +92,7 @@
{:schema nil, :name "categories"}
{:schema nil, :name "users"}
{:schema nil, :name "venues"}}}
(driver/describe-database :mongo (data/db))))))
(driver/describe-database :mongo (mt/db))))))

(deftest describe-table-test
(mt/test-driver :mongo
Expand All @@ -119,7 +117,7 @@
:database-type "java.lang.Long"
:base-type :type/Integer
:pk? true}}}
(driver/describe-table :mongo (data/db) (Table (data/id :venues)))))))
(driver/describe-table :mongo (mt/db) (Table (mt/id :venues)))))))

(deftest nested-columns-test
(mt/test-driver :mongo
Expand All @@ -141,28 +139,28 @@

(deftest all-num-columns-test
(mt/test-driver :mongo
(data/dataset all-null-columns
(mt/dataset all-null-columns
(is (= [{:name "_id", :database_type "java.lang.Long", :base_type :type/Integer, :special_type :type/PK}
{:name "favorite_snack", :database_type "NULL", :base_type :type/*, :special_type nil}
{:name "name", :database_type "java.lang.String", :base_type :type/Text, :special_type :type/Name}]
(map
(partial into {})
(db/select [Field :name :database_type :base_type :special_type]
:table_id (data/id :bird_species)
:table_id (mt/id :bird_species)
{:order-by [:name]})))))))

(deftest table-rows-sample-test
(mt/test-driver :mongo
(driver/sync-in-context :mongo (data/db)
(driver/sync-in-context :mongo (mt/db)
(fn []
(is (= [[1 "Red Medicine"]
[2 "Stout Burgers & Beers"]
[3 "The Apple Pan"]
[4 "Wurstküche"]
[5 "Brite Spot Family Restaurant"]]
(vec (take 5 (metadata-queries/table-rows-sample (Table (data/id :venues))
[(Field (data/id :venues :id))
(Field (data/id :venues :name))])))))))))
(vec (take 5 (metadata-queries/table-rows-sample (Table (mt/id :venues))
[(Field (mt/id :venues :id))
(Field (mt/id :venues :name))])))))))))


;; ## Big-picture tests for the way data should look post-sync
Expand All @@ -173,7 +171,7 @@
{:active true, :name "users"}
{:active true, :name "venues"}]
(for [field (db/select [Table :name :active]
:db_id (data/id)
:db_id (mt/id)
{:order-by [:name]})]
(into {} field)))
"Test that Tables got synced correctly")))
Expand All @@ -200,7 +198,7 @@
(vec (for [table-name table-names]
(vec (for [field (db/select [Field :name :base_type :special_type]
:active true
:table_id (data/id table-name)
:table_id (mt/id table-name)
{:order-by [:name]})]
(into {} field))))))))))

Expand All @@ -215,8 +213,8 @@
(deftest bson-ids-test
(mt/test-driver :mongo
(is (= [[2 "Lucky Pigeon" (ObjectId. "abcdefabcdefabcdefabcdef")]]
(rows (data/dataset with-bson-ids
(data/run-mbql-query birds
(rows (mt/dataset with-bson-ids
(mt/run-mbql-query birds
{:filter [:= $bird_id "abcdefabcdefabcdefabcdef"]}))))
"Check that we support Mongo BSON ID and can filter by it (#1367)")))

Expand All @@ -226,8 +224,8 @@
(letfn [(rows-count [query]
(count (rows (qp/process-query {:native query
:type :native
:database (data/id)}))))]
(data/dataset with-bson-ids
:database (mt/id)}))))]
(mt/dataset with-bson-ids
(is (= 1
(rows-count {:query "[{\"$match\": {\"bird_id\": ObjectId(\"abcdefabcdefabcdefabcdef\")}}]"
:collection "birds"}))))
Expand All @@ -251,17 +249,17 @@
(mt/test-driver :mongo
(testing "make sure x-rays don't use features that the driver doesn't support"
(is (= true
(->> (magic/automagic-analysis (Field (data/id :venues :price)) {})
(->> (magic/automagic-analysis (Field (mt/id :venues :price)) {})
:ordered_cards
(mapcat (comp :breakout :query :dataset_query :card))
(not-any? #{[:binning-strategy [:field-id (data/id :venues :price)] "default"]})))))))
(not-any? #{[:binning-strategy [:field-id (mt/id :venues :price)] "default"]})))))))

(deftest no-values-test
(mt/test-driver :mongo
(testing (str "if we query a something an there are no values for the Field, the query should still return "
"successfully! (#8929 and #8894)")
;; add a temporary Field that doesn't actually exist to test data categories
(tt/with-temp Field [_ {:name "parent_id", :table_id (data/id :categories)}]
(mt/with-temp Field [_ {:name "parent_id", :table_id (mt/id :categories)}]
;; ok, now run a basic MBQL query against categories Table. When implicit Field IDs get added the `parent_id`
;; Field will be included
(testing (str "if the column does not come back in the results for a given document we should fill in the "
Expand All @@ -270,7 +268,7 @@
[2 "American" nil]
[3 "Artisan" nil]]}
(->
(data/run-mbql-query categories
(mt/run-mbql-query categories
{:order-by [[:asc [:field-id $id]]]
:limit 3})
qp.t/data
Expand All @@ -287,12 +285,12 @@
(is (= [[22]]
(mt/rows
(qp/process-query
{:database (data/id)
:type :native
:native {:projections [:count]
:query [{"$project" {"price" "$price"}}
{"$match" {"price" {"$eq" 1}}}
{"$group" {"_id" nil, "count" {"$sum" 1}}}
{"$sort" {"_id" 1}}
{"$project" {"_id" false, "count" true}}]
:collection "venues"}})))))))
{:database (mt/id)
:type :native
:native {:projections [:count]
:query [{"$project" {"price" "$price"}}
{"$match" {"price" {"$eq" 1}}}
{"$group" {"_id" nil, "count" {"$sum" 1}}}
{"$sort" {"_id" 1}}
{"$project" {"_id" false, "count" true}}]
:collection "venues"}})))))))
Loading

0 comments on commit 85d29f2

Please sign in to comment.