diff --git a/docs/operations-guide/start.md b/docs/operations-guide/start.md
index ee86cdd0431b1917f9c12a1a533f72d6f5a9f8d8..8ce4f7aa320513058086635a04263b2c93e28c09 100644
--- a/docs/operations-guide/start.md
+++ b/docs/operations-guide/start.md
@@ -81,6 +81,7 @@ The application database is where Metabase stores information about users, saved
**NOTE:** currently Metabase does not provide automated support for migrating data from one application database to another, so if you start with H2 and then want to move to Postgres you'll have to dump the data from H2 and import it into Postgres before relaunching the application.
#### [H2](http://www.h2database.com/) (default)
+
To use the H2 database for your Metabase instance you don't need to do anything at all. When the application is first launched it will attempt to create a new H2 database in the same filesystem location the application is launched from.
You can see these database files from the terminal:
@@ -98,6 +99,7 @@ If for any reason you want to use an H2 database file in a separate location fro
export MB_DB_FILE=/the/path/to/my/h2.db
java -jar metabase.jar
+Note that H2 automatically appends `.mv.db` or `.h2.db` to the path you specify; do not include those in you path! In other words, `MB_DB_FILE` should be something like `/path/to/metabase.db`, rather than something like `/path/to/metabase.db.mv.db` (even though this is the file that actually gets created).
#### [Postgres](http://www.postgresql.org/)
@@ -132,13 +134,13 @@ This will tell Metabase to look for its application database using the supplied
# Migrating from using the H2 database to MySQL or Postgres
-If you decide to use the default application database (H2) when you initially start using Metabase, but decide later that you'd like to switch to a more production ready database such as MySQL or Postgres we make the transition easy for you.
+If you decide to use the default application database (H2) when you initially start using Metabase, but later decide that you'd like to switch to a more production-ready database such as MySQL or Postgres, we make the transition easy for you.
Metabase provides a custom migration command for upgrading H2 application database files by copying their data to a new database. Here's what you'll want to do:
1. Shutdown your Metabase instance so that it's not running. This ensures no accidental data gets written to the db while migrating.
2. Make a backup copy of your H2 application database by following the instructions in [Backing up Metabase Application Data](#backing-up-metabase-application-data). Safety first!
-3. Run the Metabase data migration command using the appropriate environment variables for the target database you want to migrate to. You can find details about specifying MySQL and Postgres databases at [Configuring the application database](#configuring-the-metabase-application-database). Here's an example of migrating to Postgres.
+3. Run the Metabase data migration command using the appropriate environment variables for the target database you want to migrate to. You can find details about specifying MySQL and Postgres databases at [Configuring the application database](#configuring-the-metabase-application-database). Here's an example of migrating to Postgres:
```
export MB_DB_TYPE=postgres
@@ -147,20 +149,21 @@ export MB_DB_PORT=5432
export MB_DB_USER=<username>
export MB_DB_PASS=<password>
export MB_DB_HOST=localhost
-java -jar metabase.jar load-from-h2 <path-to-metabase-h2-database-file>
+java -jar metabase.jar load-from-h2 /path/to/metabase.db # do not include .mv.db or .h2.db suffix
```
-It is expected that you will run the command against a brand new (empty!) database and Metabase will handle all of the work of creating the database schema and migrating the data for you.
+It is expected that you will run the command against a brand-new (empty!) database; Metabase will handle all of the work of creating the database schema and migrating the data for you.
###### Notes
-* It is required that wherever you are running this migration command can connect to the target MySQL or Postgres database. So if you are attempting to move the data to a cloud database make sure you take that into consideration.
+* It is required that you can connect to the target MySQL or Postgres database in whatever environment you are running this migration command in. So, if you are attempting to move the data to a cloud database, make sure you take that into consideration.
* The code that handles these migrations uses a Postgres SQL command that is only available in Postgres 9.4 or newer versions. Please make sure you Postgres database is version 9.4 or newer.
+* H2 automatically adds a `.h2.db` or `.mv.db` extension to the database path you specify, so make sure the path to the DB file you pass to the command *does not* include it. For example, if you have a file named `/path/to/metabase.db.h2.db`, call the command with `load-from-h2 /path/to/metabase.db`.
# Running Metabase database migrations manually
-When Metabase is starting up it will typically attempt to determine if any changes are required to the application database and it will execute those changes automatically. If for some reason you wanted to see what these changes are and run them manually on your database then we let you do that.
+When Metabase is starting up, it will typically attempt to determine if any changes are required to the application database, and, if so, will execute those changes automatically. If for some reason you wanted to see what these changes are and run them manually on your database then we let you do that.
Simply set the following environment variable before launching Metabase:
diff --git a/frontend/src/metabase/components/ListSearchField.jsx b/frontend/src/metabase/components/ListSearchField.jsx
index a3dfd9d9f72886fc4374cd5a8ac6a61fc94498b4..18db099aeb928953665481bcdd9bd2a2f046dbb6 100644
--- a/frontend/src/metabase/components/ListSearchField.jsx
+++ b/frontend/src/metabase/components/ListSearchField.jsx
@@ -2,6 +2,7 @@ import React, { Component } from "react";
import PropTypes from "prop-types";
import Icon from "metabase/components/Icon.jsx";
+import Input from "metabase/components/Input.jsx";
import { t } from "c-3po";
export default class ListSearchField extends Component {
@@ -43,7 +44,7 @@ export default class ListSearchField extends Component {
<span className="px1">
<Icon name="search" size={16} />
</span>
- <input
+ <Input
className={inputClassName}
type="text"
placeholder={placeholder}
diff --git a/frontend/src/metabase/components/__mocks__/ExplicitSize.jsx b/frontend/src/metabase/components/__mocks__/ExplicitSize.jsx
new file mode 100644
index 0000000000000000000000000000000000000000..91e740969979b01f4c19ef9b0fc2f439cb76c1c3
--- /dev/null
+++ b/frontend/src/metabase/components/__mocks__/ExplicitSize.jsx
@@ -0,0 +1,8 @@
+/* eslint-disable react/display-name */
+import React from "react";
+
+const ExplicitSize = ComposedComponent => props => (
+ <ComposedComponent width={1000} height={1000} {...props} />
+);
+
+export default ExplicitSize;
diff --git a/frontend/test/public/public.integ.spec.js b/frontend/test/public/public.integ.spec.js
index 3021cb9eec6bb679917ecc5965c24924089b8852..e2a9e2c74fe4a70e0f7c0f74e4ab92f8bcdf24e8 100644
--- a/frontend/test/public/public.integ.spec.js
+++ b/frontend/test/public/public.integ.spec.js
@@ -1,11 +1,18 @@
+jest.mock("metabase/components/ExplicitSize");
+
// Converted from an old Selenium E2E test
import {
useSharedAdminLogin,
logout,
createTestStore,
+ createDashboard,
restorePreviousLogin,
waitForRequestToComplete,
} from "__support__/integrated_tests";
+
+import _ from "underscore";
+import jwt from "jsonwebtoken";
+
import { click, clickButton, setInputValue } from "__support__/enzyme_utils";
import { mount } from "enzyme";
@@ -39,9 +46,14 @@ import {
ADD_PARAM_VALUES,
FETCH_TABLE_METADATA,
} from "metabase/redux/metadata";
+import {
+ FETCH_DASHBOARD_CARD_DATA,
+ FETCH_CARD_DATA,
+} from "metabase/dashboard/dashboard";
import RunButton from "metabase/query_builder/components/RunButton";
import Scalar from "metabase/visualizations/visualizations/Scalar";
import ParameterFieldWidget from "metabase/parameters/components/widgets/ParameterFieldWidget";
+import TextWidget from "metabase/parameters/components/widgets/TextWidget.jsx";
import SaveQuestionModal from "metabase/containers/SaveQuestionModal";
import { LOAD_COLLECTIONS } from "metabase/questions/collections";
import SharingPane from "metabase/public/components/widgets/SharingPane";
@@ -53,6 +65,11 @@ import * as Urls from "metabase/lib/urls";
import QuestionEmbedWidget from "metabase/query_builder/containers/QuestionEmbedWidget";
import EmbedWidget from "metabase/public/components/widgets/EmbedWidget";
+import { CardApi, DashboardApi, SettingsApi } from "metabase/services";
+
+const PEOPLE_TABLE_ID = 2;
+const PEOPLE_ID_FIELD_ID = 13;
+
async function updateQueryText(store, queryText) {
// We don't have Ace editor so we have to trigger the Redux action manually
const newDatasetQuery = getQuery(store.getState())
@@ -336,17 +353,268 @@ describe("public/embedded", () => {
// that expect that we're already logged in
afterAll(() => restorePreviousLogin());
});
+ });
- afterAll(async () => {
- const store = await createTestStore();
+ describe("dashboards", () => {
+ let publicDashUrl = null;
+ let embedDashUrl = null;
+ let dashboardId = null;
+ let sqlCardId = null;
+ let mbqlCardId = null;
+
+ it("should allow creating a public/embedded Dashboard with parameters", async () => {
+ // create a Dashboard
+ const dashboard = await createDashboard({
+ name: "Test Dashboard",
+ parameters: [
+ { name: "Num", slug: "num", id: "537e37b4", type: "category" },
+ {
+ name: "People ID",
+ slug: "people_id",
+ id: "22486e00",
+ type: "people_id",
+ },
+ ],
+ });
+ dashboardId = dashboard.id;
+
+ // create the 2 Cards we will need
+ const sqlCard = await CardApi.create({
+ name: "SQL Card",
+ display: "scalar",
+ visualization_settings: {},
+ dataset_query: {
+ database: 1,
+ type: "native",
+ native: {
+ query: "SELECT {{num}} AS num",
+ template_tags: {
+ num: {
+ name: "num",
+ display_name: "Num",
+ type: "number",
+ required: true,
+ default: 1,
+ },
+ },
+ },
+ },
+ });
+ sqlCardId = sqlCard.id;
+
+ const mbqlCard = await CardApi.create({
+ name: "MBQL Card",
+ display: "scalar",
+ visualization_settings: {},
+ dataset_query: {
+ database: 1,
+ type: "query",
+ query: {
+ source_table: PEOPLE_TABLE_ID,
+ aggregation: ["count"],
+ },
+ },
+ });
+ mbqlCardId = mbqlCard.id;
+
+ // add the two Cards to the Dashboard
+ const sqlDashcard = await DashboardApi.addcard({
+ dashId: dashboard.id,
+ cardId: sqlCard.id,
+ });
+ const mbqlDashcard = await DashboardApi.addcard({
+ dashId: dashboard.id,
+ cardId: mbqlCard.id,
+ });
+
+ // wire up the params for the Cards
+ await DashboardApi.reposition_cards({
+ dashId: dashboard.id,
+ cards: [
+ {
+ id: sqlDashcard.id,
+ card_id: sqlCard.id,
+ row: 0,
+ col: 0,
+ sizeX: 4,
+ sizeY: 4,
+ series: [],
+ visualization_settings: {},
+ parameter_mappings: [
+ {
+ card_id: sqlCard.id,
+ target: ["variable", ["template-tag", "num"]],
+ parameter_id: "537e37b4",
+ },
+ ],
+ },
+ {
+ id: mbqlDashcard.id,
+ card_id: mbqlCard.id,
+ row: 0,
+ col: 4,
+ sizeX: 4,
+ sizeY: 4,
+ series: [],
+ visualization_settings: {},
+ parameter_mappings: [
+ {
+ card_id: mbqlCard.id,
+ target: ["dimension", ["field-id", PEOPLE_ID_FIELD_ID]],
+ parameter_id: "22486e00",
+ },
+ ],
+ },
+ ],
+ });
- // Disable public sharing and embedding after running tests
- await store.dispatch(
- updateSetting({ key: "enable-public-sharing", value: false }),
+ // make the Dashboard public + save the URL
+ const publicDash = await DashboardApi.createPublicLink({
+ id: dashboard.id,
+ });
+ publicDashUrl = getRelativeUrlWithoutHash(
+ Urls.publicDashboard(publicDash.uuid),
);
- await store.dispatch(
- updateSetting({ key: "enable-embedding", value: false }),
+
+ // make the Dashboard embeddable + make params editable + save the URL
+ await DashboardApi.update({
+ id: dashboard.id,
+ embedding_params: {
+ num: "enabled",
+ people_id: "enabled",
+ },
+ enable_embedding: true,
+ });
+
+ const settings = await SettingsApi.list();
+ const secretKey = _.findWhere(settings, { key: "embedding-secret-key" })
+ .value;
+
+ const token = jwt.sign(
+ {
+ resource: {
+ dashboard: dashboard.id,
+ },
+ params: {},
+ },
+ secretKey,
);
+ embedDashUrl = Urls.embedDashboard(token);
+ });
+
+ describe("as an anonymous user", () => {
+ beforeAll(() => logout());
+
+ async function runSharedDashboardTests(store, dashUrl) {
+ store.pushPath(dashUrl);
+
+ const app = mount(store.getAppContainer());
+
+ // I think this means we *wait* for the Cards to load?
+ await store.waitForActions([
+ FETCH_DASHBOARD_CARD_DATA,
+ FETCH_CARD_DATA,
+ ]);
+
+ // We need to wait for the API requests to finish or something like that.
+ // TODO - what's the right way to do this without using a stupid DELAY?
+ await delay(1000);
+
+ const getValueOfSqlCard = () =>
+ app
+ .update()
+ .find(Scalar)
+ .find(".ScalarValue")
+ .at(0)
+ .text();
+ const getValueOfMbqlCard = () =>
+ app
+ .update()
+ .find(Scalar)
+ .find(".ScalarValue")
+ .at(1)
+ .text();
+
+ const setParam = async (paramIndex, newValue) => {
+ app
+ .update()
+ .find(TextWidget)
+ .at(paramIndex)
+ .props()
+ .setValue(newValue);
+
+ // TODO - not sure what the correct way to wait for the cards to reload is
+ await store.waitForActions([
+ FETCH_DASHBOARD_CARD_DATA,
+ FETCH_CARD_DATA,
+ ]);
+ waitForRequestToComplete("GET", /.*/);
+ await delay(500);
+ };
+
+ // check that initial value of SQL Card is 1
+ expect(getValueOfSqlCard()).toBe("1");
+
+ // check that initial value of People Count MBQL Card is 2500 (or whatever people.count is supposed to be)
+ expect(getValueOfMbqlCard()).toBe("2,500");
+
+ // now set the SQL param to '50' & wait for Dashboard to reload. check that value of SQL Card is updated
+ await setParam(0, "50");
+ expect(getValueOfSqlCard()).toBe("50");
+
+ // now set our MBQL param' & wait for Dashboard to reload. check that value of the MBQL Card is updated
+ await setParam(1, "40");
+ expect(getValueOfMbqlCard()).toBe("1");
+ }
+
+ it("should handle parameters in public Dashboards correctly", async () => {
+ if (!publicDashUrl)
+ throw new Error(
+ "This test fails because test setup code didn't produce a public Dashboard URL.",
+ );
+
+ const publicUrlTestStore = await createTestStore({ publicApp: true });
+ await runSharedDashboardTests(publicUrlTestStore, publicDashUrl);
+ });
+
+ it("should handle parameters in embedded Dashboards correctly", async () => {
+ if (!embedDashUrl)
+ throw new Error(
+ "This test fails because test setup code didn't produce a embedded Dashboard URL.",
+ );
+
+ const embedUrlTestStore = await createTestStore({ embedApp: true });
+ await runSharedDashboardTests(embedUrlTestStore, embedDashUrl);
+ });
+ afterAll(restorePreviousLogin);
});
+
+ afterAll(() => {
+ // delete the Dashboard & Cards we created
+ DashboardApi.update({
+ id: dashboardId,
+ archived: true,
+ });
+ CardApi.update({
+ id: sqlCardId,
+ archived: true,
+ });
+ CardApi.update({
+ id: mbqlCardId,
+ archived: true,
+ });
+ });
+ });
+
+ afterAll(async () => {
+ const store = await createTestStore();
+
+ // Disable public sharing and embedding after running tests
+ await store.dispatch(
+ updateSetting({ key: "enable-public-sharing", value: false }),
+ );
+ await store.dispatch(
+ updateSetting({ key: "enable-embedding", value: false }),
+ );
});
});
diff --git a/project.clj b/project.clj
index 0aa3c2b5b30c8074adb6c0b13189634abba923a4..c1b9f8accb8c7e69bc8c681bd57629e82be9f734 100644
--- a/project.clj
+++ b/project.clj
@@ -66,7 +66,6 @@
[hiccup "1.0.5"] ; HTML templating
[honeysql "0.8.2"] ; Transform Clojure data structures to SQL
[io.crate/crate-jdbc "2.1.6"] ; Crate JDBC driver
- [instaparse "1.4.0"] ; Insaparse parser generator
[io.forward/yaml "1.0.6" ; Clojure wrapper for YAML library SnakeYAML (which we already use for liquidbase)
:exclusions [org.clojure/clojure
org.yaml/snakeyaml]]
diff --git a/src/metabase/api/embed.clj b/src/metabase/api/embed.clj
index 7ca413b4ea187d6ba98c2573f1d9050e79a8ea5d..46ac0d35eabe40d2546cc473e700765955d0f710 100644
--- a/src/metabase/api/embed.clj
+++ b/src/metabase/api/embed.clj
@@ -145,15 +145,19 @@
[card]
(update card :parameters concat (template-tag-parameters card)))
-(defn- apply-parameter-values
+(s/defn ^:private apply-parameter-values :- (s/maybe [{:slug su/NonBlankString
+ :type su/NonBlankString
+ :target s/Any
+ :value s/Any}])
"Adds `value` to parameters with `slug` matching a key in `parameter-values` and removes parameters without a
`value`."
[parameters parameter-values]
- (for [param parameters
- :let [value (get parameter-values (keyword (:slug param)))]
- :when (some? value)]
- (assoc (select-keys param [:type :target])
- :value value)))
+ (when (seq parameters)
+ (for [param parameters
+ :let [value (get parameter-values (keyword (:slug param)))]
+ :when (some? value)]
+ (assoc (select-keys param [:type :target :slug])
+ :value value))))
(defn- resolve-card-parameters
"Returns parameters for a card (HUH?)" ; TODO - better docstring
diff --git a/src/metabase/api/public.clj b/src/metabase/api/public.clj
index f31a4063ee265febb0071ab5ca9116ef68149a13..77115c833ef4d672755e32926133c0fb7ea69ddf 100644
--- a/src/metabase/api/public.clj
+++ b/src/metabase/api/public.clj
@@ -3,6 +3,7 @@
(:require [cheshire.core :as json]
[clojure.walk :as walk]
[compojure.core :refer [GET]]
+ [medley.core :as m]
[metabase
[db :as mdb]
[query-processor :as qp]
@@ -63,16 +64,15 @@
(api/check-public-sharing-enabled)
(card-with-uuid uuid))
-
(defn run-query-for-card-with-id
"Run the query belonging to Card with CARD-ID with PARAMETERS and other query options (e.g. `:constraints`)."
+ {:style/indent 2}
[card-id parameters & options]
- (u/prog1 (-> (let [parameters (if (string? parameters) (json/parse-string parameters keyword) parameters)]
- ;; run this query with full superuser perms
- (binding [api/*current-user-permissions-set* (atom #{"/"})
- qp/*allow-queries-with-no-executor-id* true]
- (apply card-api/run-query-for-card card-id, :parameters parameters, :context :public-question, options)))
- (u/select-nested-keys [[:data :columns :cols :rows :rows_truncated] [:json_query :parameters] :error :status]))
+ (u/prog1 (-> ;; run this query with full superuser perms
+ (binding [api/*current-user-permissions-set* (atom #{"/"})
+ qp/*allow-queries-with-no-executor-id* true]
+ (apply card-api/run-query-for-card card-id, :parameters parameters, :context :public-question, options))
+ (u/select-nested-keys [[:data :columns :cols :rows :rows_truncated] [:json_query :parameters] :error :status]))
;; if the query failed instead of returning anything about the query just return a generic error message
(when (= (:status <>) :failed)
(throw (ex-info "An error occurred while running the query." {:status-code 400})))))
@@ -81,7 +81,10 @@
"Run query for a *public* Card with UUID. If public sharing is not enabled, this throws an exception."
[uuid parameters & options]
(api/check-public-sharing-enabled)
- (apply run-query-for-card-with-id (api/check-404 (db/select-one-id Card :public_uuid uuid, :archived false)) parameters options))
+ (apply run-query-for-card-with-id
+ (api/check-404 (db/select-one-id Card :public_uuid uuid, :archived false))
+ parameters
+ options))
(api/defendpoint GET "/card/:uuid/query"
@@ -89,7 +92,7 @@
credentials. Public sharing must be enabled."
[uuid parameters]
{parameters (s/maybe su/JSONString)}
- (run-query-for-card-with-public-uuid uuid parameters))
+ (run-query-for-card-with-public-uuid uuid (json/parse-string parameters keyword)))
(api/defendpoint GET "/card/:uuid/query/:export-format"
"Fetch a publicly-accessible Card and return query results in the specified format. Does not require auth
@@ -98,7 +101,8 @@
{parameters (s/maybe su/JSONString)
export-format dataset-api/ExportFormat}
(dataset-api/as-format export-format
- (run-query-for-card-with-public-uuid uuid parameters, :constraints nil)))
+ (run-query-for-card-with-public-uuid uuid (json/parse-string parameters keyword), :constraints nil)))
+
;;; ----------------------------------------------- Public Dashboards ------------------------------------------------
@@ -127,6 +131,80 @@
(api/check-public-sharing-enabled)
(dashboard-with-uuid uuid))
+(defn- dashboard->dashcard-param-mappings
+ "Get a sequence of all the `:parameter_mappings` for all the DashCards in this `dashboard-or-id`."
+ [dashboard-or-id]
+ (for [params (db/select-field :parameter_mappings DashboardCard
+ :dashboard_id (u/get-id dashboard-or-id))
+ param params
+ :when (:parameter_id param)]
+ param))
+
+(defn- matching-dashboard-param-with-target
+ "Find an entry in `dashboard-params` that matches `target`, if one exists. Since `dashboard-params` do not themselves
+ have targets they are matched via the `dashcard-param-mappings` for the Dashboard. See `resolve-params` below for
+ more details."
+ [dashboard-params dashcard-param-mappings target]
+ (some (fn [{id :parameter_id, :as param-mapping}]
+ (when (= target (:target param-mapping))
+ ;; ...and once we find that, try to find a Dashboard `:parameters`
+ ;; entry with the same ID...
+ (m/find-first #(= (:id %) id)
+ dashboard-params)))
+ dashcard-param-mappings))
+
+(s/defn ^:private resolve-params :- (s/maybe [{s/Keyword s/Any}])
+ "Resolve the parmeters passed in to the API (`query-params`) and make sure they're actual valid parameters the
+ Dashboard with `dashboard-id`. This is done to prevent people from adding in parameters that aren't actually present
+ on the Dashboard. When successful, this will return a merged sequence based on the original `dashboard-params`, but
+ including the `:value` from the appropriate query-param.
+
+ The way we pass in parameters is complicated and silly: for public Dashboards, they're passed in as JSON-encoded
+ parameters that look something like (when decoded):
+
+ [{:type :category, :target [:variable [:template-tag :num]], :value \"50\"}]
+
+ For embedded Dashboards they're simply passed in as query parameters, e.g.
+
+ [{:num 50}]
+
+ Thus resolving the params has to take either format into account. To further complicate matters, a Dashboard's
+ `:parameters` column contains values that look something like:
+
+ [{:name \"Num\", :slug \"num\", :id \"537e37b4\", :type \"category\"}
+
+ This is sufficient to resolve slug-style params passed in to embedded Dashboards, but URL-encoded params for public
+ Dashboards do not have anything that can directly match them to a Dashboard `:parameters` entry. However, they
+ already have enough information for the query processor to handle resolving them itself; thus we simply need to make
+ sure these params are actually allowed to be used on the Dashboard. To do this, we can match them against the
+ `:parameter_mappings` for the Dashboard's DashboardCards, which look like:
+
+ [{:card_id 1, :target [:variable [:template-tag :num]], :parameter_id \"537e37b4\"}]
+
+ Thus for public Dashboards JSON-encoded style we can look for a matching Dashcard parameter mapping, based on
+ `:target`, and then find the matching Dashboard parameter, based on `:id`.
+
+ *Cries*
+
+ TODO -- Tom has mentioned this, and he is very correct -- our lives would be much easier if we just used slug-style
+ for everything, rather than the weird JSON-encoded format we use for public Dashboards. We should fix this!"
+ [dashboard-id :- su/IntGreaterThanZero, query-params :- (s/maybe [{s/Keyword s/Any}])]
+ (when (seq query-params)
+ (let [dashboard-params (db/select-one-field :parameters Dashboard, :id dashboard-id)
+ slug->dashboard-param (u/key-by :slug dashboard-params)
+ dashcard-param-mappings (dashboard->dashcard-param-mappings dashboard-id)]
+ (for [{slug :slug, target :target, :as query-param} query-params
+ :let [dashboard-param
+ (or
+ ;; try to match by slug...
+ (slug->dashboard-param slug)
+ ;; ...if that fails, try to find a DashboardCard param mapping with the same target...
+ (matching-dashboard-param-with-target dashboard-params dashcard-param-mappings target)
+ ;; ...but if we *still* couldn't find a match, throw an Exception, because we don't want people
+ ;; trying to inject new params
+ (throw (Exception. (str "Invalid param: " slug))))]]
+ (merge query-param dashboard-param)))))
+
(defn- check-card-is-in-dashboard
"Check that the Card with `card-id` is in Dashboard with `dashboard-id`, either in a DashboardCard at the top level or
as a series, or throw an Exception. If not such relationship exists this will throw a 404 Exception."
@@ -146,7 +224,10 @@
[dashboard-id card-id parameters & {:keys [context]
:or {context :public-dashboard}}]
(check-card-is-in-dashboard card-id dashboard-id)
- (run-query-for-card-with-id card-id parameters, :context context, :dashboard-id dashboard-id))
+ (run-query-for-card-with-id card-id (resolve-params dashboard-id (if (string? parameters)
+ (json/parse-string parameters keyword)
+ parameters))
+ :context context, :dashboard-id dashboard-id))
(api/defendpoint GET "/dashboard/:uuid/card/:card-id"
"Fetch the results for a Card in a publicly-accessible Dashboard. Does not require auth credentials. Public
diff --git a/src/metabase/db/migrations.clj b/src/metabase/db/migrations.clj
index a8640fb425b1c6ce2127734208f76747e887b5bf..47ea98304ea4a953bc94ad6f2ed7e28087b4ce66 100644
--- a/src/metabase/db/migrations.clj
+++ b/src/metabase/db/migrations.clj
@@ -339,10 +339,15 @@
;; missing those database ids
(defmigration ^{:author "senior", :added "0.27.0"} populate-card-database-id
(doseq [[db-id cards] (group-by #(get-in % [:dataset_query :database])
- (db/select [Card :dataset_query :id] :database_id [:= nil]))
+ (db/select [Card :dataset_query :id :name] :database_id [:= nil]))
:when (not= db-id virtual-id)]
- (db/update-where! Card {:id [:in (map :id cards)]}
- :database_id db-id)))
+ (if (and (seq cards)
+ (db/exists? Database :id db-id))
+ (db/update-where! Card {:id [:in (map :id cards)]}
+ :database_id db-id)
+ (doseq [{id :id card-name :name} cards]
+ (log/warnf "Cleaning up orphaned Question '%s', associated to a now deleted database" card-name)
+ (db/delete! Card :id id)))))
;; Prior to version 0.28.0 humanization was configured using the boolean setting `enable-advanced-humanization`.
;; `true` meant "use advanced humanization", while `false` meant "use simple humanization". In 0.28.0, this Setting
@@ -370,11 +375,26 @@
;; `pre-update` implementation.
;;
;; Caching these permissions will prevent 1000+ DB call API calls. See https://github.com/metabase/metabase/issues/6889
-(defmigration ^{:author "camsaul", :added "0.29.0"} populate-card-read-permissions
+;;
+;; NOTE: This used used to be
+;; (defmigration ^{:author "camsaul", :added "0.28.2"} populate-card-read-permissions
+;; (run!
+;; (fn [card]
+;; (db/update! Card (u/get-id card) {}))
+;; (db/select-reducible Card :archived false, :read_permissions nil)))
+;; But due to bug https://github.com/metabase/metabase/issues/7189 was replaced
+(defmigration ^{:author "camsaul", :added "0.28.2"} populate-card-read-permissions
+ (log/info "Not running migration `populate-card-read-permissions` as it has been replaced by a subsequent migration "))
+
+;; Migration from 0.28.2 above had a flaw in that passing in `{}` to the update results in
+;; the functions that do pre-insert permissions checking don't have the query dictionary to analyze
+;; and always short-circuit due to the missing query dictionary. Passing the card itself into the
+;; check mimicks how this works in-app, and appears to fix things.
+(defmigration ^{:author "salsakran", :added "0.28.3"} repopulate-card-read-permissions
(run!
(fn [card]
- (db/update! Card (u/get-id card) {}))
- (db/select-reducible Card :archived false, :read_permissions nil)))
+ (db/update! Card (u/get-id card) card))
+ (db/select-reducible Card :archived false)))
;; Starting in version 0.29.0 we switched the way we decide which Fields should get FieldValues. Prior to 29, Fields
;; would be marked as special type Category if they should have FieldValues. In 29+, the Category special type no
diff --git a/src/metabase/models/card.clj b/src/metabase/models/card.clj
index 72c2757284a41f9f095bfd1383e83178fbcb6a62..75343c242f995fdf847e8d7b730eef1caa9541d5 100644
--- a/src/metabase/models/card.clj
+++ b/src/metabase/models/card.clj
@@ -69,6 +69,38 @@
;;; ---------------------------------------------- Permissions Checking ----------------------------------------------
+;; Is calculating permissions for Cards complicated? Some would say so. Refer to this handy flow chart to see how things
+;; get calculated.
+;;
+;; Note that `can-read?/can-write?` and `pre-insert/pre-update` are the two entry points into the permissions
+;; labyrinth. `pre-insert`/`pre-update` calculate permissions for the *query* (disregarding collection and publicness)
+;; and thus skip to `query-perms-set`; `can-read?`/`can-write?` want to take those into account (as well as cached
+;; read permissions, if available) and thus starts higher up.
+;;
+;;
+;; can-read?/can-write? --> perms-set-taking-collection-etc-into-account
+;; |
+;; public? <------------------------------+---------------------------> in collection?
+;; ↓ else ↓
+;; #{} ↓ collection/perms-objects-set
+;; card-perms-set-for-query
+;; |
+;; write perms <---------------------+---------------------> read perms
+;; | |
+;; | does not have cached read_permissions <-----+-----> has cached read_permissions
+;; | ↓ ↓
+;; +-------------------> query-perms-set <------------------+ return cached read_permssions
+;; pre-insert/ ↑ | |
+;; pre-update ---------------------------+ | |
+;; (maybe-update | |
+;; -read-perms) native card? <------+-----> mbql card? |
+;; ↓ ↓ |
+;; native-perms-path mbql-perms-path-set | (recursively for source card)
+;; | |
+;; no source card <----+----> has source card
+;; ↓
+;; tables->permissions-path-set
+
(defn- native-permissions-path
"Return the `:read` (for running) or `:write` (for saving) native permissions path for DATABASE-OR-ID."
[read-or-write database-or-id]
@@ -103,8 +135,18 @@
(declare query-perms-set)
(defn- mbql-permissions-path-set
- "Return the set of required permissions needed to run QUERY."
- [read-or-write query]
+ "Return the set of required permissions needed to run QUERY.
+
+ Optionally specify `disallowed-source-card-ids`: this is a sequence of Card IDs that should not be allowed to be a
+ source Card ID in this case. For example, you would want to disallow a Card from being its own source; when
+ recursing, this is used to keep track of source Card IDs we've already seen in order to prevent circular
+ references.
+
+ Also optionally specify `throw-exceptions?` -- normally this function avoids throwing Exceptions to avoid breaking
+ things when a single Card is busted (e.g. API endpoints that filter out unreadable Cards) and instead returns 'only
+ admins can see this' permissions -- `#{\"db/0\"}` (DB 0 will never exist, thus normal users will never be able to
+ get permissions for it, but admins have root perms and will still get to see (and hopefully fix) it)."
+ [read-or-write query & [disallowed-source-card-ids throw-exceptions?]]
{:pre [(map? query) (map? (:query query))]}
(try
(or
@@ -116,13 +158,26 @@
;;
;; See issue #6845 for further discussion.
(when-let [source-card-id (qputil/query->source-card-id query)]
- (query-perms-set (db/select-one-field :dataset_query Card :id source-card-id) :read))
+ ;; If this source card ID is disallowed (e.g. due to it being a circular reference) then throw an Exception.
+ ;; Bye Felicia!
+ (when ((set disallowed-source-card-ids) source-card-id)
+ (throw
+ (Exception.
+ (str "Cannot calculate permissions due to circular references. This means a question is either using itself "
+ "as a source or one or more questions are using each other as sources."))))
+ ;; ok, if we've decided that this is not a loooopy situation then go ahead and recurse
+ (query-perms-set (db/select-one-field :dataset_query Card :id source-card-id)
+ :read
+ (conj disallowed-source-card-ids source-card-id)
+ throw-exceptions?))
;; otherwise if there's no source card then calculate perms based on the Tables referenced in the query
(let [{:keys [query database]} (qp/expand query)]
(tables->permissions-path-set read-or-write database (query->source-and-join-tables query))))
;; if for some reason we can't expand the Card (i.e. it's an invalid legacy card) just return a set of permissions
;; that means no one will ever get to see it (except for superusers who get to see everything)
(catch Throwable e
+ (when throw-exceptions?
+ (throw e))
(log/warn "Error getting permissions for card:" (.getMessage e) "\n"
(u/pprint-to-str (u/filtered-stacktrace e)))
#{"/db/0/"}))) ; DB 0 will never exist
@@ -138,39 +193,45 @@
for read permissions you should look at a Card's `:read_permissions`, which is precalculated. If you specifically
need to calculate permissions for a query directly, and ignore anything precomputed, use this function. Otherwise
you should rely on one of the optimized ones below."
- [{query-type :type, database :database, :as query} read-or-write]
+ [{query-type :type, database :database, :as query} read-or-write & [disallowed-source-card-ids throw-exceptions?]]
(cond
(empty? query) #{}
(= (keyword query-type) :native) #{(native-permissions-path read-or-write database)}
- (= (keyword query-type) :query) (mbql-permissions-path-set read-or-write query)
+ (= (keyword query-type) :query) (mbql-permissions-path-set read-or-write query disallowed-source-card-ids throw-exceptions?)
:else (throw (Exception. (str "Invalid query type: " query-type)))))
(defn- card-perms-set-for-query
"Return the permissions required to `read-or-write` `card` based on its query, disregarding the collection the Card is
in, whether it is publicly shared, etc. This will return precalculated `:read_permissions` if they are present."
- [{read-perms :read_permissions, id :id, query :dataset_query} read-or-write]
+ [{read-perms :read_permissions, card-id :id, query :dataset_query} read-or-write]
(cond
;; for WRITE permissions always recalculate since these will be determined relatively infrequently (hopefully)
;; e.g. when updating a Card
- (= :write read-or-write) (query-perms-set query :write)
- ;; if the Card has populated `:read_permissions` and we're looking up read pems return those rather than calculating
- ;; on-the-fly. Cast to `set` to be extra-double-sure it's a set like we'd expect when it gets deserialized from JSON
- read-perms (set read-perms)
+ (= :write read-or-write) (query-perms-set query :write [card-id])
+ ;; if the Card has *populated* `:read_permissions` and we're looking up read pems return those rather than
+ ;; calculating on-the-fly. Cast to `set` to be extra-double-sure it's a set like we'd expect when it gets
+ ;; deserialized from JSON
+ (seq read-perms) (set read-perms)
;; otherwise if :read_permissions was NOT populated. This should not normally happen since the data migration
;; should have pre-populated values for all the Cards. If it does it might mean something like we fetched the Card
;; without its `read_permissions` column. Since that would be "doing something wrong" warn about it.
- :else (do (log/warn "Card" id "is missing its read_permissions. Calculating them now...")
- (query-perms-set query :read))))
+ :else (do (log/info "Card" card-id "does not have cached read_permissions.")
+ (query-perms-set query :read [card-id]))))
-(defn- card-perms-set-for-current-user
- "Calculate the permissions required to `read-or-write` `card` *for the current user*. This takes into account whether
- the Card is publicly available, or in a collection the current user can view; it also attempts to use precalcuated
+(defn- card-perms-set-taking-collection-etc-into-account
+ "Calculate the permissions required to `read-or-write` `card`*for a user. This takes into account whether the Card is
+ publicly available, or in a collection the current user can view; it also attempts to use precalcuated
`read_permissions` when possible. This is the function that should be used for general permissions checking for a
- Card."
+ Card.
+
+ This function works the same regardless of whether called with a current user (e.g. `api/*current-user*`, etc.) or
+ not! It simply calculates the permssions a User would need to see the Card."
[{collection-id :collection_id, public-uuid :public_uuid, in-public-dash? :in_public_dashboard,
- outer-query :dataset_query, :as card}
+ outer-query :dataset_query, card-id :id, :as card}
read-or-write]
+ (when-not (seq card)
+ (throw (Exception. "`card` is nil or empty. Cannot calculate permissions.")))
(let [source-card-id (qputil/query->source-card-id outer-query)]
(cond
;; you don't need any permissions to READ a public card, which is PUBLIC by definition :D
@@ -182,12 +243,6 @@
collection-id
(collection/perms-objects-set collection-id read-or-write)
- ;; if this is based on a source card then our permissions are based on that; recurse. You can always save a new
- ;; Card based on a source card you can read, thus read/write permissions for this new Card will be the same as
- ;; read permissions for its source. This is dicussed in further detail above in `mbql-permissions-path-set`
- source-card-id
- (card-perms-set-for-current-user (Card source-card-id) :read)
-
:else
(card-perms-set-for-query card read-or-write))))
@@ -246,10 +301,37 @@
:query_type (keyword query-type)})
card))
+(defn- maybe-update-read-permissions
+ "When inserting or updating a `card`, if `:dataset_query` is going to change, calculate the updated `:read_permssions`
+ and `assoc` those to the output so they get changed as well. These cached `read_permissions` are the permissions for
+ the underlying query, disregarding whether the Card is in a collection or present in a public Dashboard or is itself
+ public. Only query permissions are expensive to calculate, so that is the only thing we cache. The other stuff is
+ caclulated every time by `card-perms-set-taking-collection-etc-into-account`.
+
+ (maybe-update-read-permssions card-to-be-saved) ;-> updated-card-to-be-saved"
+ [{query :dataset_query, card-id :id, :as card}]
+ (if-not (seq query)
+ card
+ ;; Calculate read_permissions using `query-perms-set`, which calculates perms based on the query along (ignoring
+ ;; collection perms, presence on public dashboards, etc.).
+ (assoc card :read_permissions (query-perms-set query
+ :read
+ ;; If this is an UPDATE operation send along the `card-id` to the
+ ;; list of `disallowed-source-card-ids` because, needless to say, a
+ ;; Card should not be allowed to use itself as a source, whether
+ ;; directly or indirectly. See `query-perms-set` itself for further
+ ;; discussion.
+ (when card-id [card-id])
+ ;; tell `query-perms-set` to throw Exceptions so we don't end up
+ ;; saving a Card that is for some reason invalid
+ :throw-exceptions))))
+
(defn- pre-insert [{query :dataset_query, :as card}]
;; TODO - make sure if `collection_id` is specified that we have write permissions for that collection
- ;; Save the new Card with read permissions since calculating them dynamically is so expensive.
- (u/prog1 (assoc card :read_permissions (query-perms-set query :read))
+ ;;
+ ;; updated Card with updated read permissions when applicable. (New Cards should never be created without a valid
+ ;; `:dataset_query` so this should always happen)
+ (u/prog1 (maybe-update-read-permissions card)
;; for native queries we need to make sure the user saving the card has native query permissions for the DB
;; because users can always see native Cards and we don't want someone getting around their lack of permissions
;; that way
@@ -267,8 +349,8 @@
(field-values/update-field-values-for-on-demand-dbs! field-ids))))
(defn- pre-update [{archived? :archived, query :dataset_query, :as card}]
- ;; save the updated Card with updated read permissions.
- (u/prog1 (assoc card :read_permissions (query-perms-set query :read))
+ ;; save the updated Card with updated read permissions when applicable.
+ (u/prog1 (maybe-update-read-permissions card)
;; if the Card is archived, then remove it from any Dashboards
(when archived?
(db/delete! 'DashboardCard :card_id (u/get-id card)))
@@ -320,7 +402,7 @@
(merge i/IObjectPermissionsDefaults
{:can-read? (partial i/current-user-has-full-permissions? :read)
:can-write? (partial i/current-user-has-full-permissions? :write)
- :perms-objects-set card-perms-set-for-current-user})
+ :perms-objects-set card-perms-set-taking-collection-etc-into-account})
revision/IRevisioned
(assoc revision/IRevisionedDefaults
diff --git a/src/metabase/models/params.clj b/src/metabase/models/params.clj
index 3fae1874079acbb88f055ced75ba9d5525c9d8f0..19390eb6e02d6e6ae0b5c2d70e1f039ca11e82df 100644
--- a/src/metabase/models/params.clj
+++ b/src/metabase/models/params.clj
@@ -15,7 +15,7 @@
;;; | SHARED |
;;; +----------------------------------------------------------------------------------------------------------------+
-(defn- field-form->id
+(defn field-form->id
"Expand a `field-id` or `fk->` FORM and return the ID of the Field it references.
(field-form->id [:field-id 100]) ; -> 100"
diff --git a/src/metabase/query_processor/middleware/parameters/mbql.clj b/src/metabase/query_processor/middleware/parameters/mbql.clj
index 3b9f1bdcbc162146ce2274f950eed9473d70d6b4..e3a2b1e20d1a54a1bcbda1b1d30858c18fe691e3 100644
--- a/src/metabase/query_processor/middleware/parameters/mbql.clj
+++ b/src/metabase/query_processor/middleware/parameters/mbql.clj
@@ -1,23 +1,38 @@
(ns metabase.query-processor.middleware.parameters.mbql
"Code for handling parameter substitution in MBQL queries."
(:require [clojure.string :as str]
- [metabase.query-processor.middleware.parameters.dates :as date-params]))
+ [metabase.models
+ [field :refer [Field]]
+ [params :as params]]
+ [metabase.query-processor.middleware.parameters.dates :as date-params]
+ [toucan.db :as db]))
(defn- parse-param-value-for-type
"Convert PARAM-VALUE to a type appropriate for PARAM-TYPE.
The frontend always passes parameters in as strings, which is what we want in most cases; for numbers, instead
convert the parameters to integers or floating-point numbers."
- [param-type param-value]
+ [param-type param-value field-id]
(cond
+ ;; for `id` type params look up the base-type of the Field and see if it's a number or not. If it *is* a number
+ ;; then recursively call this function and parse the param value as a number as appropriate.
+ (and (= (keyword param-type) :id)
+ (isa? (db/select-one-field :base_type Field :id field-id) :type/Number))
+ (recur :number param-value field-id)
+
;; no conversion needed if PARAM-TYPE isn't :number or PARAM-VALUE isn't a string
(or (not= (keyword param-type) :number)
- (not (string? param-value))) param-value
+ (not (string? param-value)))
+ param-value
+
;; if PARAM-VALUE contains a period then convert to a Double
- (re-find #"\." param-value) (Double/parseDouble param-value)
+ (re-find #"\." param-value)
+ (Double/parseDouble param-value)
+
;; otherwise convert to a Long
- :else (Long/parseLong param-value)))
+ :else
+ (Long/parseLong param-value)))
-(defn- build-filter-clause [{param-type :type, param-value :value, [_ field :as target] :target}]
+(defn- build-filter-clause [{param-type :type, param-value :value, [_ field :as target] :target, :as param}]
(cond
;; multipe values. Recursively handle them all and glue them all together with an OR clause
(sequential? param-value)
@@ -26,11 +41,12 @@
;; single value, date range. Generate appropriate MBQL clause based on date string
(str/starts-with? param-type "date")
- (date-params/date-string->filter (parse-param-value-for-type param-type param-value) field)
+ (date-params/date-string->filter (parse-param-value-for-type param-type param-value (params/field-form->id field))
+ field)
;; single-value, non-date param. Generate MBQL [= <field> <value>] clause
:else
- [:= field (parse-param-value-for-type param-type param-value)]))
+ [:= field (parse-param-value-for-type param-type param-value (params/field-form->id field))]))
(defn- merge-filter-clauses [base addtl]
(cond
diff --git a/src/metabase/query_processor/middleware/parameters/sql.clj b/src/metabase/query_processor/middleware/parameters/sql.clj
index 86f9a27faa8c3833326978c55ba8fcf64579fcf6..759c3cf8e3415c6d11698530754dfc30221c5241 100644
--- a/src/metabase/query_processor/middleware/parameters/sql.clj
+++ b/src/metabase/query_processor/middleware/parameters/sql.clj
@@ -7,17 +7,20 @@
[clojure.tools.logging :as log]
[honeysql.core :as hsql]
[instaparse.core :as insta]
+ [medley.core :as m]
[metabase.driver :as driver]
[metabase.models.field :as field :refer [Field]]
[metabase.query-processor.middleware.parameters.dates :as date-params]
[metabase.query-processor.middleware.expand :as ql]
[metabase.util :as u]
[metabase.util.schema :as su]
+ [puppetlabs.i18n.core :refer [tru]]
[schema.core :as s]
[toucan.db :as db])
(:import clojure.lang.Keyword
honeysql.types.SqlCall
java.text.NumberFormat
+ java.util.regex.Pattern
metabase.models.field.FieldInstance))
;; The Basics:
@@ -86,9 +89,16 @@
(s/optional-key :default) s/Any})
(def ^:private DimensionValue
- {:type su/NonBlankString
- :target s/Any
- (s/optional-key :value) s/Any}) ; not specified if the param has no value. TODO - make this stricter
+ {:type su/NonBlankString
+ :target s/Any
+ ;; not specified if the param has no value. TODO - make this stricter
+ (s/optional-key :value) s/Any
+ ;; The following are not used by the code in this namespace but may or may not be specified depending on what the
+ ;; code that constructs the query params is doing. We can go ahead and ignore these when present.
+ (s/optional-key :slug) su/NonBlankString
+ (s/optional-key :name) su/NonBlankString
+ (s/optional-key :default) s/Any
+ (s/optional-key :id) s/Any}) ; used internally by the frontend
(def ^:private SingleValue
"Schema for a valid *single* value for a param. As of 0.28.0 params can either be single-value or multiple value."
@@ -112,7 +122,7 @@
{s/Keyword ParamValue})
(def ^:private ParamSnippetInfo
- {(s/optional-key :replacement-snippet) s/Str ; allowed to be blank if this is an optional param
+ {(s/optional-key :replacement-snippet) s/Str ; allowed to be blank if this is an optional param
(s/optional-key :prepared-statement-args) [s/Any]})
;;; +----------------------------------------------------------------------------------------------------------------+
@@ -197,12 +207,12 @@
(.parse (NumberFormat/getInstance) ^String s))
(s/defn ^:private value->number :- (s/cond-pre s/Num CommaSeparatedNumbers)
- "Parse a 'numeric' param value. Normally this returns an integer or floating-point number,
- but as a somewhat undocumented feature it also accepts comma-separated lists of numbers. This was a side-effect of
- the old parameter code that unquestioningly substituted any parameter passed in as a number directly into the SQL.
- This has long been changed for security purposes (avoiding SQL injection), but since users have come to expect
- comma-separated numeric values to work we'll allow that (with validation) and return an instance of
- `CommaSeperatedNumbers`. (That is converted to SQL as a simple comma-separated list.)"
+ "Parse a 'numeric' param value. Normally this returns an integer or floating-point number, but as a somewhat
+ undocumented feature it also accepts comma-separated lists of numbers. This was a side-effect of the old parameter
+ code that unquestioningly substituted any parameter passed in as a number directly into the SQL. This has long been
+ changed for security purposes (avoiding SQL injection), but since users have come to expect comma-separated numeric
+ values to work we'll allow that (with validation) and return an instance of `CommaSeperatedNumbers`. (That is
+ converted to SQL as a simple comma-separated list.)"
[value]
(cond
;; if not a string it's already been parsed
@@ -274,7 +284,8 @@
(defprotocol ^:private ISQLParamSubstituion
"Protocol for specifying what SQL should be generated for parameters of various types."
(^:private ->replacement-snippet-info [this]
- "Return information about how THIS should be converted to SQL, as a map with keys `:replacement-snippet` and `:prepared-statement-args`.
+ "Return information about how THIS should be converted to SQL, as a map with keys `:replacement-snippet` and
+ `:prepared-statement-args`.
(->replacement-snippet-info \"ABC\") -> {:replacement-snippet \"?\", :prepared-statement-args \"ABC\"}"))
@@ -393,23 +404,6 @@
;;; | PARSING THE SQL TEMPLATE |
;;; +----------------------------------------------------------------------------------------------------------------+
-(def ^:private sql-template-parser
- (insta/parser
- "SQL := (ANYTHING_NOT_RESERVED | SINGLE_BRACKET_PLUS_ANYTHING | SINGLE_BRACE_PLUS_ANYTHING | OPTIONAL | PARAM)*
-
- (* Treat double brackets and braces as special, pretty much everything else is good to go *)
- <SINGLE_BRACKET_PLUS_ANYTHING> := !'[[' '[' (ANYTHING_NOT_RESERVED | ']' | SINGLE_BRACKET_PLUS_ANYTHING | SINGLE_BRACE_PLUS_ANYTHING)*
- <SINGLE_BRACE_PLUS_ANYTHING> := !'{{' '{' (ANYTHING_NOT_RESERVED | '}' | SINGLE_BRACE_PLUS_ANYTHING | SINGLE_BRACKET_PLUS_ANYTHING)*
- <ANYTHING_NOT_RESERVED> := #'[^\\[\\]\\{\\}]+'
-
- (* Parameters can have whitespace, but must be word characters for the name of the parameter *)
- PARAM = <'{{'> <WHITESPACE*> TOKEN <WHITESPACE*> <'}}'>
-
- (* Parameters, braces and brackets are all good here, just no nesting of optional clauses *)
- OPTIONAL := <'[['> (ANYTHING_NOT_RESERVED | SINGLE_BRACKET_PLUS_ANYTHING | SINGLE_BRACE_PLUS_ANYTHING | PARAM)* <']]'>
- <TOKEN> := #'(\\w)+'
- WHITESPACE := #'\\s+'"))
-
(defrecord ^:private Param [param-key sql-value prepared-statement-args])
(defn- param? [maybe-param]
@@ -436,48 +430,80 @@
(and (param? maybe-param)
(no-value? (:sql-value maybe-param))))
-(defn- transform-sql
- "Returns the combined query-map from all of the parameters, optional clauses etc. At this point there should not be
- a NoValue leaf. If so, it's an error (i.e. missing a required parameter."
- [param-key->value]
- (fn [& nodes]
- (doseq [maybe-param nodes
- :when (no-value-param? maybe-param)]
- (throw (ex-info (format "Unable to substitute '%s': param not specified.\nFound: %s"
- (:param-name maybe-param) (keys param-key->value))
- {:status-code 400})))
- (-> (reduce merge-query-map empty-query-map nodes)
- (update :query str/trim))))
-
-(defn- transform-optional
- "Converts the `OPTIONAL` clause to a query map. If one or more parameters are not populated for this optional
- clause, it will return an empty-query-map, which will be omitted from the query."
- [& nodes]
- (if (some no-value-param? nodes)
- empty-query-map
- (reduce merge-query-map empty-query-map nodes)))
-
-(defn- transform-param
- "Converts a `PARAM` parse leaf to a query map that includes the SQL snippet to replace the `{{param}}` value and the
- param itself for the prepared statement"
- [param-key->value]
- (fn [token]
- (let [val (get param-key->value (keyword token) (NoValue.))]
- (if (no-value? val)
- (map->Param {:param-key token, :sql-value val, :prepared-statement-args []})
- (let [{:keys [replacement-snippet prepared-statement-args]} (->replacement-snippet-info val)]
- (map->Param {:param-key token
- :sql-value replacement-snippet
- :prepared-statement-args prepared-statement-args}))))))
-
-(defn- parse-transform-map
- "Instaparse returns things like [:SQL token token token...]. This map will be used when crawling the parse tree from
- the bottom up. When encountering the a `:PARAM` node, it will invoke the included function, invoking the function
- with each item in the list as arguments "
- [param-key->value]
- {:SQL (transform-sql param-key->value)
- :OPTIONAL transform-optional
- :PARAM (transform-param param-key->value)})
+(defn- quoted-re-pattern [s]
+ (-> s Pattern/quote re-pattern))
+
+(defn- split-delimited-string
+ "Interesting parts of the SQL string (vs. parts that are just passed through) are delimited,
+ i.e. {{something}}. This function takes a `delimited-begin` and `delimited-end` regex and uses that to separate the
+ string. Returns a map with the prefix (the string leading up to the first `delimited-begin`) and `:delimited-strings` as
+ a seq of maps where `:delimited-body` is what's in-between the delimited marks (i.e. foo in {{foo}} and then a
+ suffix, which is the characters after the trailing delimiter but before the next occurrence of the `delimited-end`."
+ [delimited-begin delimited-end s]
+ (let [begin-pattern (quoted-re-pattern delimited-begin)
+ end-pattern (quoted-re-pattern delimited-end)
+ [prefix & segmented-strings] (str/split s begin-pattern)]
+ (when-let [^String msg (and (seq segmented-strings)
+ (not-every? #(str/index-of % delimited-end) segmented-strings)
+ (tru "Found ''{0}'' with no terminating ''{1}'' in query ''{2}''"
+ delimited-begin delimited-end s))]
+ (throw (IllegalArgumentException. msg)))
+ {:prefix prefix
+ :delimited-strings (for [segmented-string segmented-strings
+ :let [[token-str & rest-of-segment] (str/split segmented-string end-pattern)]]
+ {:delimited-body token-str
+ :suffix (apply str rest-of-segment)})}))
+
+(defn- token->param
+ "Given a `token` and `param-key->value` return a `Param`. If no parameter value is found, return a `NoValue` param"
+ [token param-key->value]
+ (let [val (get param-key->value (keyword token) (NoValue.))
+ {:keys [replacement-snippet,
+ prepared-statement-args]} (->replacement-snippet-info val)]
+ (map->Param (merge {:param-key token}
+ (if (no-value? val)
+ {:sql-value val, :prepared-statement-args []}
+ {:sql-value replacement-snippet
+ :prepared-statement-args prepared-statement-args})))))
+
+(defn- parse-params
+ "Parse `s` for any parameters. Returns a seq of strings and `Param` instances"
+ [s param-key->value]
+ (let [{:keys [prefix delimited-strings]} (split-delimited-string "{{" "}}" s)]
+ (cons prefix
+ (mapcat (fn [{:keys [delimited-body suffix]}]
+ [(-> delimited-body
+ str/trim
+ (token->param param-key->value))
+ suffix])
+ delimited-strings))))
+
+(defn- parse-params-and-throw
+ "Same as `parse-params` but will throw an exception if there are any `NoValue` parameters"
+ [s param-key->value]
+ (let [results (parse-params s param-key->value)]
+ (if-let [{:keys [param-key]} (m/find-first no-value-param? results)]
+ (throw (ex-info (tru "Unable to substitute ''{0}'': param not specified.\nFound: {1}"
+ (name param-key) (pr-str (map name (keys param-key->value))))
+ {:status-code 400}))
+ results)))
+
+(defn- parse-optional
+ "Attempts to parse `s`. Parses any optional clauses or parameters found, returns a query map."
+ [s param-key->value]
+ (let [{:keys [prefix delimited-strings]} (split-delimited-string "[[" "]]" s)]
+ (reduce merge-query-map empty-query-map
+ (apply concat (parse-params-and-throw prefix param-key->value)
+ (for [{:keys [delimited-body suffix]} delimited-strings
+ :let [optional-clause (parse-params delimited-body param-key->value)]]
+ (if (some no-value-param? optional-clause)
+ (parse-params-and-throw suffix param-key->value)
+ (concat optional-clause (parse-params-and-throw suffix param-key->value))))))))
+
+(defn- parse-template [sql param-key->value]
+ (-> sql
+ (parse-optional param-key->value)
+ (update :query str/trim)))
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | PUTTING IT ALL TOGETHER |
@@ -489,10 +515,8 @@
(s/defn ^:private expand-query-params
[{sql :query, :as native}, param-key->value :- ParamValues]
(merge native
- (-> (parse-transform-map param-key->value)
- (insta/transform (insta/parse sql-template-parser sql))
- ;; `prepare-sql-param-for-driver` can't be lazy as it needs `*driver*` to be bound
- (update :params #(mapv prepare-sql-param-for-driver %)))))
+ ;; `prepare-sql-param-for-driver` can't be lazy as it needs `*driver*` to be bound
+ (update (parse-template sql param-key->value) :params #(mapv prepare-sql-param-for-driver %))))
(defn- ensure-driver
"Depending on where the query came from (the user, permissions check etc) there might not be an driver associated to
diff --git a/test/metabase/api/card_test.clj b/test/metabase/api/card_test.clj
index 055f44e5ea9a6a5c1cb31fe42c8c12eab14f1b6b..a8b811397a3f06b5814fb5918f5e1e707934afad 100644
--- a/test/metabase/api/card_test.clj
+++ b/test/metabase/api/card_test.clj
@@ -556,17 +556,11 @@
(Pulse pulse-id)]))
;; Adding an additional breakout will cause the alert to be removed
-(tt/expect-with-temp [Database
- [{database-id :id}]
-
- Table
- [{table-id :id} {:db_id database-id}]
-
- Card
+(tt/expect-with-temp [Card
[card {:display :line
:visualization_settings {:graph.goal_value 10}
:dataset_query (assoc-in
- (mbql-count-query database-id table-id)
+ (mbql-count-query (data/id) (data/id :checkins))
[:query :breakout]
[["datetime-field" (data/id :checkins :date) "hour"]])}]
@@ -592,9 +586,9 @@
(et/with-fake-inbox
(et/with-expected-messages 1
((user->client :crowberto) :put 200 (str "card/" (u/get-id card))
- {:dataset_query (assoc-in (mbql-count-query database-id table-id)
+ {:dataset_query (assoc-in (mbql-count-query (data/id) (data/id :checkins))
[:query :breakout] [["datetime-field" (data/id :checkins :date) "hour"]
- ["datetime-field" (data/id :checkins :date) "second"]])}))
+ ["datetime-field" (data/id :checkins :date) "minute"]])}))
[(et/regex-email-bodies #"the question was edited by Crowberto Corv")
(Pulse pulse-id)]))
diff --git a/test/metabase/api/dashboard_test.clj b/test/metabase/api/dashboard_test.clj
index 788c6bd7e03ab9fa42aaad86ebe2b79465df447f..5b34d82224da43c7b05b96779d936fafacbc0087 100644
--- a/test/metabase/api/dashboard_test.clj
+++ b/test/metabase/api/dashboard_test.clj
@@ -133,7 +133,7 @@
:display "table"
:query_type nil
:dataset_query {}
- :read_permissions []
+ :read_permissions nil
:visualization_settings {}
:query_average_duration nil
:in_public_dashboard false
diff --git a/test/metabase/api/database_test.clj b/test/metabase/api/database_test.clj
index 0b27c853768a83a5ddac7378a3929d967a1146cf..53a4b7eb5d13edd36f64dfc9fe3708c47126edab 100644
--- a/test/metabase/api/database_test.clj
+++ b/test/metabase/api/database_test.clj
@@ -460,18 +460,18 @@
Card [_ (assoc (card-with-mbql-query "Cum Count Card"
:source-table (data/id :checkins)
:aggregation [[:cum-count]]
- :breakout [[:datetime-field [:field-id (data/id :checkins :date) :month]]])
+ :breakout [[:datetime-field [:field-id (data/id :checkins :date)] :month]])
:result_metadata [{:name "num_toucans"}])]]
(saved-questions-virtual-db
(virtual-table-for-card ok-card))
(fetch-virtual-database))
-;; cum sum using old-style single aggregation syntax
+;; cum count using old-style single aggregation syntax
(tt/expect-with-temp [Card [ok-card (ok-mbql-card)]
Card [_ (assoc (card-with-mbql-query "Cum Sum Card"
:source-table (data/id :checkins)
- :aggregation [:cum-sum]
- :breakout [[:datetime-field [:field-id (data/id :checkins :date) :month]]])
+ :aggregation [:cum-count]
+ :breakout [[:datetime-field [:field-id (data/id :checkins :date)] :month]])
:result_metadata [{:name "num_toucans"}])]]
(saved-questions-virtual-db
(virtual-table-for-card ok-card))
diff --git a/test/metabase/api/embed_test.clj b/test/metabase/api/embed_test.clj
index f9679ebc23764760661b8377880a19cde51c71c0..57dc48532ad914a333b3566dbd4e45584158dcbc 100644
--- a/test/metabase/api/embed_test.clj
+++ b/test/metabase/api/embed_test.clj
@@ -68,7 +68,7 @@
:source "aggregation", :extra_info {}, :id nil, :target nil, :display_name "count",
:base_type "type/Integer", :remapped_from nil, :remapped_to nil}]
:rows [[100]]}
- :json_query {:parameters []}
+ :json_query {:parameters nil}
:status "completed"})
([results-format]
(case results-format
diff --git a/test/metabase/api/preview_embed_test.clj b/test/metabase/api/preview_embed_test.clj
index 322278c8b19d45837cd336a6af6c26a0565643ee..7113c0816a9cfdc8d95cb2b7cb7dfd6aec38e04a 100644
--- a/test/metabase/api/preview_embed_test.clj
+++ b/test/metabase/api/preview_embed_test.clj
@@ -1,10 +1,15 @@
(ns metabase.api.preview-embed-test
(:require [expectations :refer :all]
[metabase.api.embed-test :as embed-test]
- [metabase.models.dashboard :refer [Dashboard]]
- [metabase.test.data :as data]
- [metabase.test.data.users :as test-users]
- [metabase.test.util :as tu]
+ [metabase.models
+ [card :refer [Card]]
+ [dashboard :refer [Dashboard]]]
+ [metabase.test
+ [data :as data]
+ [util :as tu]]
+ [metabase.test.data
+ [datasets :as datasets]
+ [users :as test-users]]
[metabase.util :as u]
[toucan.util.test :as tt]))
@@ -338,21 +343,59 @@
(expect
"completed"
(embed-test/with-embedding-enabled-and-new-secret-key
- (embed-test/with-temp-card [card {:dataset_query
- {:database (data/id)
- :type "native"
- :native {:query (str "SELECT {{num_birds}} AS num_birds,"
- " {{2nd_date_seen}} AS 2nd_date_seen")
- :template_tags {:equipment {:name "num_birds"
- :display_name "Num Birds"
- :type "number"}
- :7_days_ending_on {:name "2nd_date_seen",
- :display_name "Date Seen",
- :type "date"}}}}}]
- (-> (embed-test/with-temp-dashcard [dashcard {:dash {:enable_embedding true}}]
- ((test-users/user->client :crowberto) :get 200 (str (dashcard-url dashcard
- {:_embedding_params {:num_birds :locked
- :2nd_date_seen :enabled}
- :params {:num_birds 2}})
- "?2nd_date_seen=2018-02-14")))
- :status))))
+ (-> (embed-test/with-temp-dashcard [dashcard {:dash {:enable_embedding true}}]
+ ((test-users/user->client :crowberto) :get 200 (str (dashcard-url dashcard
+ {:_embedding_params {:num_birds :locked
+ :2nd_date_seen :enabled}
+ :params {:num_birds 2}})
+ "?2nd_date_seen=2018-02-14")))
+ :status)))
+
+;; Make sure that editable params do not result in "Invalid Parameter" exceptions (#7212)
+(expect
+ [[50]]
+ (embed-test/with-embedding-enabled-and-new-secret-key
+ (tt/with-temp Card [card {:dataset_query {:database (data/id)
+ :type :native
+ :native {:query "SELECT {{num}} AS num"
+ :template_tags {:num {:name "num"
+ :display_name "Num"
+ :type "number"
+ :required true
+ :default "1"}}}}}]
+ (embed-test/with-temp-dashcard [dashcard {:dash {:parameters [{:name "Num"
+ :slug "num"
+ :id "537e37b4"
+ :type "category"}]}
+ :dashcard {:card_id (u/get-id card)
+ :parameter_mappings [{:card_id (u/get-id card)
+ :target [:variable
+ [:template-tag :num]]
+ :parameter_id "537e37b4"}]}}]
+ (-> ((test-users/user->client :crowberto) :get (str (dashcard-url dashcard {:_embedding_params {:num "enabled"}})
+ "?num=50"))
+ :data
+ :rows)))))
+
+;; Make sure that ID params correctly get converted to numbers as needed (Postgres-specific)...
+(datasets/expect-with-engine :postgres
+ [[1]]
+ (embed-test/with-embedding-enabled-and-new-secret-key
+ (tt/with-temp Card [card {:dataset_query {:database (data/id)
+ :type :query
+ :query {:source-table (data/id :venues)
+ :aggregation [:count]}}}]
+ (embed-test/with-temp-dashcard [dashcard {:dash {:parameters [{:name "Venue ID"
+ :slug "venue_id"
+ :id "22486e00"
+ :type "id"}]}
+ :dashcard {:card_id (u/get-id card)
+ :parameter_mappings [{:parameter_id "22486e00"
+ :card_id (u/get-id card)
+ :target [:dimension
+ [:field-id
+ (data/id :venues :id)]]}]}}]
+ (-> ((test-users/user->client :crowberto) :get (str (dashcard-url dashcard {:_embedding_params {:venue_id "enabled"}})
+ "?venue_id=1"))
+ :data
+ :rows)))))
diff --git a/test/metabase/api/public_test.clj b/test/metabase/api/public_test.clj
index bc919af463324437de3755af140b2dac3758087e..e1dd4619b888739ff4804061f950e3d3b8a7f2e8 100644
--- a/test/metabase/api/public_test.clj
+++ b/test/metabase/api/public_test.clj
@@ -47,13 +47,21 @@
~@body))))
(defmacro ^:private with-temp-public-dashboard {:style/indent 1} [[binding & [dashboard]] & body]
- `(let [dashboard-settings# (merge (shared-obj) ~dashboard)]
+ `(let [dashboard-settings# (merge
+ {:parameters [{:name "Venue ID"
+ :slug "venue_id"
+ :type "id"
+ :target [:dimension (data/id :venues :id)]
+ :default nil}]}
+ (shared-obj)
+ ~dashboard)]
(tt/with-temp Dashboard [dashboard# dashboard-settings#]
(let [~binding (assoc dashboard# :public_uuid (:public_uuid dashboard-settings#))]
~@body))))
-(defn- add-card-to-dashboard! [card dashboard]
- (db/insert! DashboardCard :dashboard_id (u/get-id dashboard), :card_id (u/get-id card)))
+(defn- add-card-to-dashboard! {:style/indent 2} [card dashboard & {:as kvs}]
+ (db/insert! DashboardCard (merge {:dashboard_id (u/get-id dashboard), :card_id (u/get-id card)}
+ kvs)))
(defmacro ^:private with-temp-public-dashboard-and-card
{:style/indent 1}
@@ -176,10 +184,11 @@
;; Check that we can exec a PublicCard with `?parameters`
(expect
- [{:type "category", :value 2}]
+ [{:name "Venue ID", :slug "venue_id", :type "id", :value 2}]
(tu/with-temporary-setting-values [enable-public-sharing true]
(with-temp-public-card [{uuid :public_uuid}]
- (get-in (http/client :get 200 (str "public/card/" uuid "/query"), :parameters (json/encode [{:type "category", :value 2}]))
+ (get-in (http/client :get 200 (str "public/card/" uuid "/query")
+ :parameters (json/encode [{:name "Venue ID", :slug "venue_id", :type "id", :value 2}]))
[:json_query :parameters]))))
;; make sure CSV (etc.) downloads take editable params into account (#6407)
@@ -213,8 +222,8 @@
(binding [http/*url-prefix* (str "http://localhost:" (config/config-str :mb-jetty-port) "/")]
(http/client :get 200 (str "public/question/" uuid ".csv")
:parameters (json/encode [{:type :date/quarter-year
- :target [:dimension [:template-tag :date]]
- :value "Q1-2014"}]))))))
+ :target [:dimension [:template-tag :date]]
+ :value "Q1-2014"}]))))))
;;; ---------------------------------------- GET /api/public/dashboard/:uuid -----------------------------------------
@@ -256,7 +265,7 @@
;;; --------------------------------- GET /api/public/dashboard/:uuid/card/:card-id ----------------------------------
-(defn- dashcard-url-path [dash card]
+(defn- dashcard-url [dash card]
(str "public/dashboard/" (:public_uuid dash) "/card/" (u/get-id card)))
@@ -265,14 +274,14 @@
"An error occurred."
(tu/with-temporary-setting-values [enable-public-sharing false]
(with-temp-public-dashboard-and-card [dash card]
- (http/client :get 400 (dashcard-url-path dash card)))))
+ (http/client :get 400 (dashcard-url dash card)))))
;; Check that we get a 400 if PublicDashboard doesn't exist
(expect
"An error occurred."
(tu/with-temporary-setting-values [enable-public-sharing true]
(with-temp-public-dashboard-and-card [_ card]
- (http/client :get 400 (dashcard-url-path {:public_uuid (UUID/randomUUID)} card)))))
+ (http/client :get 400 (dashcard-url {:public_uuid (UUID/randomUUID)} card)))))
;; Check that we get a 400 if PublicCard doesn't exist
@@ -280,7 +289,7 @@
"An error occurred."
(tu/with-temporary-setting-values [enable-public-sharing true]
(with-temp-public-dashboard-and-card [dash _]
- (http/client :get 400 (dashcard-url-path dash Integer/MAX_VALUE)))))
+ (http/client :get 400 (dashcard-url dash Integer/MAX_VALUE)))))
;; Check that we get a 400 if the Card does exist but it's not part of this Dashboard
(expect
@@ -288,7 +297,7 @@
(tu/with-temporary-setting-values [enable-public-sharing true]
(with-temp-public-dashboard-and-card [dash _]
(tt/with-temp Card [card]
- (http/client :get 400 (dashcard-url-path dash card))))))
+ (http/client :get 400 (dashcard-url dash card))))))
;; Check that we *cannot* execute a PublicCard via a PublicDashboard if the Card has been archived
(expect
@@ -296,23 +305,55 @@
(tu/with-temporary-setting-values [enable-public-sharing true]
(with-temp-public-dashboard-and-card [dash card]
(db/update! Card (u/get-id card), :archived true)
- (http/client :get 400 (dashcard-url-path dash card)))))
+ (http/client :get 400 (dashcard-url dash card)))))
;; Check that we can exec a PublicCard via a PublicDashboard
(expect
[[100]]
(tu/with-temporary-setting-values [enable-public-sharing true]
(with-temp-public-dashboard-and-card [dash card]
- (qp-test/rows (http/client :get 200 (dashcard-url-path dash card))))))
+ (qp-test/rows (http/client :get 200 (dashcard-url dash card))))))
;; Check that we can exec a PublicCard via a PublicDashboard with `?parameters`
(expect
- [{:type "category", :value 2}]
+ [{:name "Venue ID"
+ :slug "venue_id"
+ :target ["dimension" (data/id :venues :id)]
+ :value [10]
+ :default nil
+ :type "id"}]
(tu/with-temporary-setting-values [enable-public-sharing true]
(with-temp-public-dashboard-and-card [dash card]
- (get-in (http/client :get 200 (dashcard-url-path dash card), :parameters (json/encode [{:type "category", :value 2}]))
+ (get-in (http/client :get 200 (dashcard-url dash card)
+ :parameters (json/encode [{:name "Venue ID"
+ :slug :venue_id
+ :target [:dimension (data/id :venues :id)]
+ :value [10]}]))
[:json_query :parameters]))))
+;; Make sure params are validated: this should pass because venue_id *is* one of the Dashboard's :parameters
+(expect
+ [[1]]
+ (tu/with-temporary-setting-values [enable-public-sharing true]
+ (with-temp-public-dashboard-and-card [dash card]
+ (-> (http/client :get 200 (dashcard-url dash card)
+ :parameters (json/encode [{:name "Venue ID"
+ :slug :venue_id
+ :target [:dimension (data/id :venues :id)]
+ :value [10]}]))
+ qp-test/rows))))
+
+;; Make sure params are validated: this should fail because venue_name is *not* one of the Dashboard's :parameters
+(expect
+ "An error occurred."
+ (tu/with-temporary-setting-values [enable-public-sharing true]
+ (with-temp-public-dashboard-and-card [dash card]
+ (http/client :get 400 (dashcard-url dash card)
+ :parameters (json/encode [{:name "Venue Name"
+ :slug :venue_name
+ :target [:dimension (data/id :venues :name)]
+ :value ["PizzaHacker"]}])))))
+
;; Check that an additional Card series works as well
(expect
[[100]]
@@ -323,7 +364,128 @@
:card_id (u/get-id card)
:dashboard_id (u/get-id dash))
:card_id (u/get-id card-2)}]
- (qp-test/rows (http/client :get 200 (dashcard-url-path dash card-2))))))))
+ (qp-test/rows (http/client :get 200 (dashcard-url dash card-2))))))))
+
+;; Make sure that parameters actually work correctly (#7212)
+(expect
+ [[50]]
+ (tu/with-temporary-setting-values [enable-public-sharing true]
+ (tt/with-temp Card [card {:dataset_query {:database (data/id)
+ :type :native
+ :native {:query "SELECT {{num}} AS num"
+ :template_tags {:num {:name "num"
+ :display_name "Num"
+ :type "number"
+ :required true
+ :default "1"}}}}}]
+ (with-temp-public-dashboard [dash {:parameters [{:name "Num"
+ :slug "num"
+ :id "537e37b4"
+ :type "category"}]}]
+ (add-card-to-dashboard! card dash
+ :parameter_mappings [{:card_id (u/get-id card)
+ :target [:variable
+ [:template-tag :num]]
+ :parameter_id "537e37b4"}])
+ (-> ((test-users/user->client :crowberto)
+ :get (str (dashcard-url dash card)
+ "?parameters="
+ (json/generate-string
+ [{:type :category
+ :target [:variable [:template-tag :num]]
+ :value "50"}])))
+ :data
+ :rows)))))
+
+;; ...with MBQL Cards as well...
+(expect
+ [[1]]
+ (tu/with-temporary-setting-values [enable-public-sharing true]
+ (tt/with-temp Card [card {:dataset_query {:database (data/id)
+ :type :query
+ :query {:source-table (data/id :venues)
+ :aggregation [:count]}}}]
+ (with-temp-public-dashboard [dash {:parameters [{:name "Venue ID"
+ :slug "venue_id"
+ :id "22486e00"
+ :type "id"}]}]
+ (add-card-to-dashboard! card dash
+ :parameter_mappings [{:parameter_id "22486e00"
+ :card_id (u/get-id card)
+ :target [:dimension
+ [:field-id
+ (data/id :venues :id)]]}])
+ (-> ((test-users/user->client :crowberto)
+ :get (str (dashcard-url dash card)
+ "?parameters="
+ (json/generate-string
+ [{:type :id
+ :target [:dimension [:field-id (data/id :venues :id)]]
+ :value "50"}])))
+ :data
+ :rows)))))
+
+;; ...and also for DateTime params
+(expect
+ [[733]]
+ (tu/with-temporary-setting-values [enable-public-sharing true]
+ (tt/with-temp Card [card {:dataset_query {:database (data/id)
+ :type :query
+ :query {:source-table (data/id :checkins)
+ :aggregation [:count]}}}]
+ (with-temp-public-dashboard [dash {:parameters [{:name "Date Filter"
+ :slug "date_filter"
+ :id "18a036ec"
+ :type "date/all-options"}]}]
+ (add-card-to-dashboard! card dash
+ :parameter_mappings [{:parameter_id "18a036ec"
+ :card_id (u/get-id card)
+ :target [:dimension
+ [:field-id
+ (data/id :checkins :date)]]}])
+ (-> ((test-users/user->client :crowberto)
+ :get (str (dashcard-url dash card)
+ "?parameters="
+ (json/generate-string
+ [{:type "date/all-options"
+ :target [:dimension [:field-id (data/id :checkins :date)]]
+ :value "~2015-01-01"}])))
+ :data
+ :rows)))))
+
+;; make sure DimensionValue params also work if they have a default value, even if some is passed in for some reason
+;; as part of the query (#7253)
+;; If passed in as part of the query however make sure it doesn't override what's actually in the DB
+(expect
+ [["Wow"]]
+ (tu/with-temporary-setting-values [enable-public-sharing true]
+ (tt/with-temp Card [card {:dataset_query {:database (data/id)
+ :type :native
+ :native {:query "SELECT {{msg}} AS message"
+ :template_tags {:msg {:id "181da7c5"
+ :name "msg"
+ :display_name "Message"
+ :type "text"
+ :required true
+ :default "Wow"}}}}}]
+ (with-temp-public-dashboard [dash {:parameters [{:name "Message"
+ :slug "msg"
+ :id "181da7c5"
+ :type "category"}]}]
+ (add-card-to-dashboard! card dash
+ :parameter_mappings [{:card_id (u/get-id card)
+ :target [:variable [:template-tag :msg]]
+ :parameter_id "181da7c5"}])
+ (-> ((test-users/user->client :crowberto)
+ :get (str (dashcard-url dash card)
+ "?parameters="
+ (json/generate-string
+ [{:type :category
+ :target [:variable [:template-tag :msg]]
+ :value nil
+ :default "Hello"}])))
+ :data
+ :rows)))))
;;; --------------------------- Check that parameter information comes back with Dashboard ---------------------------
diff --git a/test/metabase/events/dependencies_test.clj b/test/metabase/events/dependencies_test.clj
index dd1215d4ad04711556b9cdd0751da2c31b150b3b..3b23f13b84491346eff8c6502deb9728adb7b06b 100644
--- a/test/metabase/events/dependencies_test.clj
+++ b/test/metabase/events/dependencies_test.clj
@@ -6,64 +6,85 @@
[database :refer [Database]]
[dependency :refer [Dependency]]
[metric :refer [Metric]]
+ [segment :refer [Segment]]
[table :refer [Table]]]
- [metabase.test.data :refer :all]
+ [metabase.test.data :as data]
+ [metabase.util :as u]
[toucan.db :as db]
[toucan.util.test :as tt]))
+(defn- temp-segment []
+ {:definition {:database (data/id)
+ :filter [:= [:field-id (data/id :categories :id)] 1]}})
+
;; `:card-create` event
-(expect
+(tt/expect-with-temp [Segment [segment-1 (temp-segment)]
+ Segment [segment-2 (temp-segment)]]
#{{:dependent_on_model "Segment"
- :dependent_on_id 2}
+ :dependent_on_id (u/get-id segment-1)}
{:dependent_on_model "Segment"
- :dependent_on_id 3}}
- (tt/with-temp Card [card {:dataset_query {:database (id)
+ :dependent_on_id (u/get-id segment-2)}}
+ (tt/with-temp Card [card {:dataset_query {:database (data/id)
:type :query
- :query {:source_table (id :categories)
- :filter ["AND" [">" 4 "2014-10-19"] ["=" 5 "yes"] ["SEGMENT" 2] ["SEGMENT" 3]]}}}]
+ :query {:source_table (data/id :categories)
+ :filter ["AND"
+ ["="
+ (data/id :categories :name)
+ "Toucan-friendly"]
+ ["SEGMENT" (u/get-id segment-1)]
+ ["SEGMENT" (u/get-id segment-2)]]}}}]
(process-dependencies-event {:topic :card-create
:item card})
(set (map (partial into {})
- (db/select [Dependency :dependent_on_model :dependent_on_id], :model "Card", :model_id (:id card))))))
+ (db/select [Dependency :dependent_on_model :dependent_on_id]
+ :model "Card", :model_id (u/get-id card))))))
;; `:card-update` event
(expect
[]
- (tt/with-temp Card [card {:dataset_query {:database (id)
+ (tt/with-temp Card [card {:dataset_query {:database (data/id)
:type :query
- :query {:source_table (id :categories)}}}]
+ :query {:source_table (data/id :categories)}}}]
(process-dependencies-event {:topic :card-create
:item card})
- (db/select [Dependency :dependent_on_model :dependent_on_id], :model "Card", :model_id (:id card))))
+ (db/select [Dependency :dependent_on_model :dependent_on_id], :model "Card", :model_id (u/get-id card))))
;; `:metric-create` event
-(expect
+(tt/expect-with-temp [Segment [segment-1 (temp-segment)]
+ Segment [segment-2 (temp-segment)]]
#{{:dependent_on_model "Segment"
- :dependent_on_id 18}
+ :dependent_on_id (u/get-id segment-1)}
{:dependent_on_model "Segment"
- :dependent_on_id 35}}
+ :dependent_on_id (u/get-id segment-2)}}
(tt/with-temp* [Database [{database-id :id}]
Table [{table-id :id} {:db_id database-id}]
Metric [metric {:table_id table-id
:definition {:aggregation ["count"]
- :filter ["AND" ["SEGMENT" 18] ["SEGMENT" 35]]}}]]
+ :filter ["AND"
+ ["SEGMENT" (u/get-id segment-1)]
+ ["SEGMENT" (u/get-id segment-2)]]}}]]
(process-dependencies-event {:topic :metric-create
:item metric})
(set (map (partial into {})
- (db/select [Dependency :dependent_on_model :dependent_on_id], :model "Metric", :model_id (:id metric))))))
+ (db/select [Dependency :dependent_on_model :dependent_on_id]
+ :model "Metric", :model_id (u/get-id metric))))))
;; `:card-update` event
-(expect
+(tt/expect-with-temp [Segment [segment-1 (temp-segment)]
+ Segment [segment-2 (temp-segment)]]
#{{:dependent_on_model "Segment"
- :dependent_on_id 18}
+ :dependent_on_id (u/get-id segment-1)}
{:dependent_on_model "Segment"
- :dependent_on_id 35}}
+ :dependent_on_id (u/get-id segment-2)}}
(tt/with-temp* [Database [{database-id :id}]
Table [{table-id :id} {:db_id database-id}]
Metric [metric {:table_id table-id
:definition {:aggregation ["count"]
- :filter ["AND" ["SEGMENT" 18] ["SEGMENT" 35]]}}]]
+ :filter ["AND"
+ ["SEGMENT" (u/get-id segment-1)]
+ ["SEGMENT" (u/get-id segment-2)]]}}]]
(process-dependencies-event {:topic :metric-update
:item metric})
(set (map (partial into {})
- (db/select [Dependency :dependent_on_model :dependent_on_id], :model "Metric", :model_id (:id metric))))))
+ (db/select [Dependency :dependent_on_model :dependent_on_id]
+ :model "Metric", :model_id (u/get-id metric))))))
diff --git a/test/metabase/models/card_test.clj b/test/metabase/models/card_test.clj
index 26fd2374dfb40afcba5e36f1ad1584234bd7e15f..66ab17d6eff1474db3b8cb40a2712e4cf00a4eb6 100644
--- a/test/metabase/models/card_test.clj
+++ b/test/metabase/models/card_test.clj
@@ -1,8 +1,9 @@
(ns metabase.models.card-test
- (:require [expectations :refer :all]
+ (:require [cheshire.core :as json]
+ [expectations :refer :all]
[metabase.api.common :refer [*current-user-permissions-set*]]
[metabase.models
- [card :refer :all]
+ [card :refer :all :as card]
[dashboard :refer [Dashboard]]
[dashboard-card :refer [DashboardCard]]
[database :as database]
@@ -217,3 +218,129 @@
(db/update! Card id {:name "another name"
:dataset_query (dummy-dataset-query (data/id))})
(into {} (db/select-one [Card :name :database_id] :id id)))]))
+
+
+
+;;; ------------------------------------------ Circular Reference Detection ------------------------------------------
+
+(defn- card-with-source-table
+ "Generate values for a Card with `source-table` for use with `with-temp`."
+ {:style/indent 1}
+ [source-table & {:as kvs}]
+ (merge {:dataset_query {:database (data/id)
+ :type :query
+ :query {:source-table source-table}}}
+ kvs))
+
+(defn- force-update-card-to-reference-source-table!
+ "Skip normal pre-update stuff so we can force a Card to get into an invalid state."
+ [card source-table]
+ (db/update! Card {:where [:= :id (u/get-id card)]
+ :set (-> (card-with-source-table source-table
+ ;; clear out cached read permissions to make sure those aren't used for calcs
+ :read_permissions nil)
+ ;; we have to manually JSON-encode since we're skipping normal pre-update stuff
+ (update :dataset_query json/generate-string))}))
+
+;; No circular references = it should work!
+(expect
+ {:card-a #{(perms/object-path (data/id) "PUBLIC" (data/id :venues))}
+ :card-b #{(perms/object-path (data/id) "PUBLIC" (data/id :venues))}}
+ ;; Make two cards. Card B references Card A.
+ (tt/with-temp* [Card [card-a (card-with-source-table (data/id :venues))]
+ Card [card-b (card-with-source-table (str "card__" (u/get-id card-a)))]]
+ {:card-a (#'card/card-perms-set-taking-collection-etc-into-account (Card (u/get-id card-a)) :read)
+ :card-b (#'card/card-perms-set-taking-collection-etc-into-account (Card (u/get-id card-b)) :read)}))
+
+;; If a Card uses itself as a source, perms calculations should fallback to the 'only admins can see it' perms of
+;; #{"/db/0"} (DB 0 will never exist, so regular users will never get to see it, but because admins have root perms,
+;; they will still get to see it and perhaps fix it.)
+(expect
+ Exception
+ (tt/with-temp Card [card (card-with-source-table (data/id :venues))]
+ ;; now try to make the Card reference itself. Should throw Exception
+ (db/update! Card (u/get-id card)
+ (card-with-source-table (str "card__" (u/get-id card))))))
+
+;; if for some reason somebody such an invalid Card was already saved in the DB make sure that calculating permissions
+;; for it just returns the admin-only #{"/db/0"} perms set
+(expect
+ #{"/db/0/"}
+ (tt/with-temp Card [card (card-with-source-table (data/id :venues))]
+ ;; now *make* the Card reference itself
+ (force-update-card-to-reference-source-table! card (str "card__" (u/get-id card)))
+ ;; ok. Calculate perms. Should fail and fall back to admin-only perms
+ (#'card/card-perms-set-taking-collection-etc-into-account (Card (u/get-id card)) :read)))
+
+;; Do the same stuff with circular reference between two Cards... (A -> B -> A)
+(expect
+ Exception
+ (tt/with-temp* [Card [card-a (card-with-source-table (data/id :venues))]
+ Card [card-b (card-with-source-table (str "card__" (u/get-id card-a)))]]
+ (db/update! Card (u/get-id card-a)
+ (card-with-source-table (str "card__" (u/get-id card-b))))))
+
+(expect
+ #{"/db/0/"}
+ ;; Make two cards. Card B references Card A
+ (tt/with-temp* [Card [card-a (card-with-source-table (data/id :venues))]
+ Card [card-b (card-with-source-table (str "card__" (u/get-id card-a)))]]
+ ;; force Card A to reference Card B
+ (force-update-card-to-reference-source-table! card-a (str "card__" (u/get-id card-b)))
+ ;; perms calc should fail and we should get admin-only perms
+ (#'card/card-perms-set-taking-collection-etc-into-account (Card (u/get-id card-a)) :read)))
+
+;; ok now try it with A -> C -> B -> A
+(expect
+ Exception
+ (tt/with-temp* [Card [card-a (card-with-source-table (data/id :venues))]
+ Card [card-b (card-with-source-table (str "card__" (u/get-id card-a)))]
+ Card [card-c (card-with-source-table (str "card__" (u/get-id card-b)))]]
+ (db/update! Card (u/get-id card-a)
+ (card-with-source-table (str "card__" (u/get-id card-c))))))
+
+(expect
+ #{"/db/0/"}
+ (tt/with-temp* [Card [card-a (card-with-source-table (data/id :venues))]
+ Card [card-b (card-with-source-table (str "card__" (u/get-id card-a)))]
+ Card [card-c (card-with-source-table (str "card__" (u/get-id card-b)))]]
+ ;; force Card A to reference Card C
+ (force-update-card-to-reference-source-table! card-a (str "card__" (u/get-id card-c)))
+ ;; perms calc should fail and we should get admin-only perms
+ (#'card/card-perms-set-taking-collection-etc-into-account (Card (u/get-id card-a)) :read)))
+
+
+;;; ---------------------------------------------- Updating Read Perms -----------------------------------------------
+
+;; Make sure when saving a new Card read perms get calculated
+(expect
+ #{(format "/db/%d/native/read/" (data/id))}
+ (tt/with-temp Card [card {:dataset_query {:database (data/id)
+ :type :native
+ :native {:query "SELECT 1"}}}]
+ ;; read_permissions should have been populated
+ (db/select-one-field :read_permissions Card :id (u/get-id card))))
+
+;; Make sure when updating a Card's query read perms get updated
+(expect
+ #{(format "/db/%d/schema/PUBLIC/table/%d/" (data/id) (data/id :venues))}
+ (tt/with-temp Card [card {:dataset_query {:database (data/id)
+ :type :native
+ :native {:query "SELECT 1"}}}]
+ ;; now change the query...
+ (db/update! Card (u/get-id card) :dataset_query {:database (data/id)
+ :type :query
+ :query {:source-table (data/id :venues)}})
+ ;; read permissions should have been updated
+ (db/select-one-field :read_permissions Card :id (u/get-id card))))
+
+;; Make sure when updating a Card but not changing query read perms do not get changed
+(expect
+ #{(format "/db/%d/native/read/" (data/id))}
+ (tt/with-temp Card [card {:dataset_query {:database (data/id)
+ :type :native
+ :native {:query "SELECT 1"}}}]
+ ;; now change something *besides* the query...
+ (db/update! Card (u/get-id card) :name "Cam's super-awesome CARD")
+ ;; read permissions should *not* have been updated
+ (db/select-one-field :read_permissions Card :id (u/get-id card))))
diff --git a/test/metabase/query_processor/middleware/parameters/mbql_test.clj b/test/metabase/query_processor/middleware/parameters/mbql_test.clj
index 4d9c72018f29744453e575f3cb2858cd37ba010f..5193b192a54f88706d8fa9f19b6f73485d586f24 100644
--- a/test/metabase/query_processor/middleware/parameters/mbql_test.clj
+++ b/test/metabase/query_processor/middleware/parameters/mbql_test.clj
@@ -1,20 +1,14 @@
(ns metabase.query-processor.middleware.parameters.mbql-test
"Tests for *MBQL* parameter substitution."
(:require [expectations :refer :all]
- [honeysql.core :as hsql]
[metabase
[query-processor :as qp]
[query-processor-test :refer [first-row format-rows-by non-timeseries-engines rows]]
[util :as u]]
- [metabase.driver.generic-sql :as sql]
- [metabase.models
- [field :refer [Field]]
- [table :refer [Table]]]
[metabase.query-processor.middleware.expand :as ql]
- [metabase.query-processor.middleware.parameters.mbql :refer :all]
+ [metabase.query-processor.middleware.parameters.mbql :as mbql-params :refer :all]
[metabase.test.data :as data]
- [metabase.test.data.datasets :as datasets]
- [metabase.util.honeysql-extensions :as hx]))
+ [metabase.test.data.datasets :as datasets]))
(defn- expand-parameters [query]
(expand (dissoc query :parameters) (:parameters query)))
@@ -24,7 +18,7 @@
(expect
{:database 1
:type :query
- :query {:filter [:= ["field-id" 123] "666"]
+ :query {:filter [:= ["field-id" (data/id :venues :name)] "Cam's Toucannery"]
:breakout [17]}}
(expand-parameters {:database 1
:type :query
@@ -32,35 +26,40 @@
:parameters [{:hash "abc123"
:name "foo"
:type "id"
- :target ["dimension" ["field-id" 123]]
- :value "666"}]}))
+ :target ["dimension" ["field-id" (data/id :venues :name)]]
+ :value "Cam's Toucannery"}]}))
;; multiple filters are conjoined by an "AND"
(expect
- {:database 1
- :type :query
- :query {:filter ["AND" ["AND" ["AND" ["=" 456 12]] [:= ["field-id" 123] "666"]] [:= ["field-id" 456] "999"]]
- :breakout [17]}}
+ {:database 1
+ :type :query
+ :query {:filter ["AND"
+ ["AND"
+ ["AND"
+ ["=" (data/id :venues :id) 12]]
+ [:= ["field-id" (data/id :venues :name)] "Cam's Toucannery"]]
+ [:= ["field-id" (data/id :venues :id)] 999]]
+ :breakout [17]}}
(expand-parameters {:database 1
:type :query
- :query {:filter ["AND" ["=" 456 12]]
+ :query {:filter ["AND" ["=" (data/id :venues :id) 12]]
:breakout [17]}
:parameters [{:hash "abc123"
:name "foo"
:type "id"
- :target ["dimension" ["field-id" 123]]
- :value "666"}
+ :target ["dimension" ["field-id" (data/id :venues :name)]]
+ :value "Cam's Toucannery"}
{:hash "def456"
:name "bar"
:type "category"
- :target ["dimension" ["field-id" 456]]
- :value "999"}]}))
+ :target ["dimension" ["field-id" (data/id :venues :id)]]
+ :value 999}]}))
;; date range parameters
(expect
{:database 1
:type :query
- :query {:filter ["TIME_INTERVAL" ["field-id" 123] -30 "day" {:include-current false}]
+ :query {:filter ["TIME_INTERVAL" ["field-id" (data/id :users :last_login)] -30 "day" {:include-current false}]
:breakout [17]}}
(expand-parameters {:database 1
:type :query
@@ -68,13 +67,13 @@
:parameters [{:hash "abc123"
:name "foo"
:type "date"
- :target ["dimension" ["field-id" 123]]
+ :target ["dimension" ["field-id" (data/id :users :last_login)]]
:value "past30days"}]}))
(expect
{:database 1
:type :query
- :query {:filter ["TIME_INTERVAL" ["field-id" 123] -30 "day" {:include-current true}]
+ :query {:filter ["TIME_INTERVAL" ["field-id" (data/id :users :last_login)] -30 "day" {:include-current true}]
:breakout [17]}}
(expand-parameters {:database 1
:type :query
@@ -82,13 +81,13 @@
:parameters [{:hash "abc123"
:name "foo"
:type "date"
- :target ["dimension" ["field-id" 123]]
+ :target ["dimension" ["field-id" (data/id :users :last_login)]]
:value "past30days~"}]}))
(expect
{:database 1
:type :query
- :query {:filter ["=" ["field-id" 123] ["relative_datetime" -1 "day"]]
+ :query {:filter ["=" ["field-id" (data/id :users :last_login)] ["relative_datetime" -1 "day"]]
:breakout [17]}}
(expand-parameters {:database 1
:type :query
@@ -96,13 +95,13 @@
:parameters [{:hash "abc123"
:name "foo"
:type "date"
- :target ["dimension" ["field-id" 123]]
+ :target ["dimension" ["field-id" (data/id :users :last_login)]]
:value "yesterday"}]}))
(expect
{:database 1
:type :query
- :query {:filter ["BETWEEN" ["field-id" 123] "2014-05-10" "2014-05-16"]
+ :query {:filter ["BETWEEN" ["field-id" (data/id :users :last_login)] "2014-05-10" "2014-05-16"]
:breakout [17]}}
(expand-parameters {:database 1
:type :query
@@ -110,14 +109,14 @@
:parameters [{:hash "abc123"
:name "foo"
:type "date"
- :target ["dimension" ["field-id" 123]]
+ :target ["dimension" ["field-id" (data/id :users :last_login)]]
:value "2014-05-10~2014-05-16"}]}))
-;;; +-------------------------------------------------------------------------------------------------------+
-;;; | END-TO-END TESTS |
-;;; +-------------------------------------------------------------------------------------------------------+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | END-TO-END TESTS |
+;;; +----------------------------------------------------------------------------------------------------------------+
;; for some reason param substitution tests fail on Redshift & (occasionally) Crate so just don't run those for now
(def ^:private ^:const params-test-engines (disj non-timeseries-engines :redshift :crate))
@@ -246,3 +245,12 @@
:value ["2014-06" "2015-06"]}]))]
(-> (qp/process-query outer-query)
:data :native_form)))
+
+;; make sure that "ID" type params get converted to numbers when appropriate
+(expect
+ [:= ["field-id" (data/id :venues :id)] 1]
+ (#'mbql-params/build-filter-clause {:type "id"
+ :target ["dimension" ["field-id" (data/id :venues :id)]]
+ :slug "venue_id"
+ :value "1"
+ :name "Venue ID"}))
diff --git a/test/metabase/query_processor/middleware/parameters/sql_test.clj b/test/metabase/query_processor/middleware/parameters/sql_test.clj
index 91efbb7834ffcf9560770b0a91a50b3478f58dc5..3b3016fd793de8edc4c0f68fcf83566e02002b55 100644
--- a/test/metabase/query_processor/middleware/parameters/sql_test.clj
+++ b/test/metabase/query_processor/middleware/parameters/sql_test.clj
@@ -15,41 +15,89 @@
;;; ------------------------------------------ basic parser tests ------------------------------------------
+(defn- parse-template
+ ([sql]
+ (parse-template sql {}))
+ ([sql param-key->value]
+ (binding [metabase.query-processor.middleware.parameters.sql/*driver* (driver/engine->driver :h2)]
+ (#'sql/parse-template sql param-key->value))))
+
+(expect
+ {:query "select * from foo where bar=1"
+ :params []}
+ (parse-template "select * from foo where bar=1"))
+
(expect
- [:SQL "select * from foo where bar=1"]
- (#'sql/sql-template-parser "select * from foo where bar=1"))
+ {:query "select * from foo where bar=?"
+ :params ["foo"]}
+ (parse-template "select * from foo where bar={{baz}}" {:baz "foo"}))
(expect
- [:SQL "select * from foo where bar=" [:PARAM "baz"]]
- (#'sql/sql-template-parser "select * from foo where bar={{baz}}"))
+ {:query "select * from foo where bar = ?"
+ :params ["foo"]}
+ (parse-template "select * from foo [[where bar = {{baz}} ]]" {:baz "foo"}))
+;; Multiple optional clauses, all present
(expect
- [:SQL "select * from foo " [:OPTIONAL "where bar = " [:PARAM "baz"] " "]]
- (#'sql/sql-template-parser "select * from foo [[where bar = {{baz}} ]]"))
+ {:query "select * from foo where bar1 = ? and bar2 = ? and bar3 = ? and bar4 = ?"
+ :params (repeat 4 "foo")}
+ (parse-template (str "select * from foo where bar1 = {{baz}} "
+ "[[and bar2 = {{baz}}]] "
+ "[[and bar3 = {{baz}}]] "
+ "[[and bar4 = {{baz}}]]")
+ {:baz "foo"}))
+;; Multiple optional clauses, none present
(expect
- [:SQL "select * from foobars "
- [:OPTIONAL " where foobars.id in (string_to_array(" [:PARAM "foobar_id"] ", ',')::integer" "[" "]" ") "]]
- (#'sql/sql-template-parser "select * from foobars [[ where foobars.id in (string_to_array({{foobar_id}}, ',')::integer[]) ]]"))
+ {:query "select * from foo where bar1 = ?"
+ :params ["foo"]}
+ (parse-template (str "select * from foo where bar1 = {{baz}} "
+ "[[and bar2 = {{none}}]] "
+ "[[and bar3 = {{none}}]] "
+ "[[and bar4 = {{none}}]]")
+ {:baz "foo"}))
(expect
- [:SQL
- "SELECT " "[" "test_data.checkins.venue_id" "]" " AS " "[" "venue_id" "]"
- ", " "[" "test_data.checkins.user_id" "]" " AS " "[" "user_id" "]"
- ", " "[" "test_data.checkins.id" "]" " AS " "[" "checkins_id" "]"
- " FROM " "[" "test_data.checkins" "]" " LIMIT 2"]
- (-> (str "SELECT [test_data.checkins.venue_id] AS [venue_id], "
- " [test_data.checkins.user_id] AS [user_id], "
- " [test_data.checkins.id] AS [checkins_id] "
- "FROM [test_data.checkins] "
- "LIMIT 2")
- (#'sql/sql-template-parser)
- (update 1 #(apply str %))))
+ {:query "select * from foobars where foobars.id in (string_to_array(?, ',')::integer[])"
+ :params ["foo"]}
+ (parse-template "select * from foobars [[ where foobars.id in (string_to_array({{foobar_id}}, ',')::integer[]) ]]"
+ {:foobar_id "foo"}))
+
+(expect
+ {:query (str "SELECT [test_data.checkins.venue_id] AS [venue_id], "
+ " [test_data.checkins.user_id] AS [user_id], "
+ " [test_data.checkins.id] AS [checkins_id] "
+ "FROM [test_data.checkins] "
+ "LIMIT 2")
+ :params []}
+ (parse-template (str "SELECT [test_data.checkins.venue_id] AS [venue_id], "
+ " [test_data.checkins.user_id] AS [user_id], "
+ " [test_data.checkins.id] AS [checkins_id] "
+ "FROM [test_data.checkins] "
+ "LIMIT 2")))
;; Valid syntax in PG
(expect
- [:SQL "SELECT array_dims(1 || '" "[" "0:1" "]" "=" "{" "2,3" "}" "'::int" "[" "]" ")"]
- (#'sql/sql-template-parser "SELECT array_dims(1 || '[0:1]={2,3}'::int[])"))
+ {:query "SELECT array_dims(1 || '[0:1]={2,3}'::int[])"
+ :params []}
+ (parse-template "SELECT array_dims(1 || '[0:1]={2,3}'::int[])"))
+
+;; Testing that invalid/unterminated template params/clauses throw an exception
+(expect
+ java.lang.IllegalArgumentException
+ (parse-template "select * from foo [[where bar = {{baz}} " {:baz "foo"}))
+
+(expect
+ java.lang.IllegalArgumentException
+ (parse-template "select * from foo [[where bar = {{baz]]" {:baz "foo"}))
+
+(expect
+ java.lang.IllegalArgumentException
+ (parse-template "select * from foo {{bar}} {{baz" {:bar "foo" :baz "foo"}))
+
+(expect
+ java.lang.IllegalArgumentException
+ (parse-template "select * from foo [[clause 1 {{bar}}]] [[clause 2" {:bar "foo"}))
;;; ------------------------------------------ simple substitution -- {{x}} ------------------------------------------