From 8c2c556ad3ebf9025a9f185832303ade4ae82e50 Mon Sep 17 00:00:00 2001
From: Braden Shepherdson <braden@metabase.com>
Date: Tue, 16 Apr 2024 13:16:24 -0400
Subject: [PATCH] Use cached Malli validators/explainers; make `:fn` schemas
cacheable (#39947)
This improves our Malli performance substantially! Many of our code paths were calling vanilla `malli.core/validate` or `malli.core/explain`; these redo the (possibly expensive) compilation of the schema into a validator or explainer for every call.
We should use the caching versions in `metabase.util.malli.registry` everywhere, and our runtime performance will be much improved.
However when I started using these cached versions, I found that the memory use was growing out of control. Eventually I tracked this down to `:fn` schemas. Functions are only comparable by pointer equality, so they make poor cache keys. `(fn ...)` or `comp` calls in a schema on a `mu/defn` function **get re-created for every call of the `mu/defn`'d function**! That's a big time sink recompiling the schemas if we're *not* caching, and a huge memory sink if we *are* caching!
This PR pulls every such schema I could find out into a `def` so it uses the same closure and is cacheable. I'd like to automate that in some part of the Malli pipeline, or maybe a linter rule, but I haven't found a good way to do it yet.
Part of #39946.
---
.../metabase_enterprise/sandbox/api/table.clj | 5 +-
src/metabase/api/database.clj | 2 +-
src/metabase/api/native_query_snippet.clj | 2 +-
src/metabase/domain_entities/core.clj | 4 +-
.../driver/common/parameters/dates.clj | 4 +-
src/metabase/driver/sql_jdbc/execute.clj | 19 ++++---
src/metabase/email.clj | 2 +-
src/metabase/integrations/google.clj | 4 +-
src/metabase/lib/convert.cljc | 6 +--
src/metabase/lib/metadata/calculation.cljc | 13 +++--
src/metabase/lib/query.cljc | 6 +--
src/metabase/lib/remove_replace.cljc | 6 ++-
src/metabase/lib/schema/common.cljc | 9 ++--
.../lib/schema/expression/arithmetic.cljc | 3 +-
.../lib/schema/expression/temporal.cljc | 3 +-
src/metabase/lib/schema/literal.cljc | 3 +-
src/metabase/lib/schema/mbql_clause.cljc | 5 +-
src/metabase/models/collection.clj | 16 +++---
src/metabase/models/dispatch.clj | 13 -----
src/metabase/models/interface.clj | 1 -
src/metabase/models/pulse.clj | 26 +++++-----
src/metabase/pulse.clj | 3 +-
src/metabase/query_processor/execute.clj | 15 +++---
.../query_processor/middleware/annotate.clj | 13 +++--
.../middleware/normalize_query.clj | 19 ++++---
.../sync_metadata/sync_table_privileges.clj | 4 +-
src/metabase/transforms/core.clj | 3 +-
src/metabase/util/date_2/parse.clj | 8 ++-
src/metabase/util/malli/schema.clj | 49 ++++++++++---------
test/metabase/test/data.clj | 21 ++++----
30 files changed, 152 insertions(+), 135 deletions(-)
diff --git a/enterprise/backend/src/metabase_enterprise/sandbox/api/table.clj b/enterprise/backend/src/metabase_enterprise/sandbox/api/table.clj
index a4ecb26ea56..85d1e604c51 100644
--- a/enterprise/backend/src/metabase_enterprise/sandbox/api/table.clj
+++ b/enterprise/backend/src/metabase_enterprise/sandbox/api/table.clj
@@ -7,14 +7,13 @@
[metabase.lib.util.match :as lib.util.match]
[metabase.models.card :refer [Card]]
[metabase.models.data-permissions :as data-perms]
- [metabase.models.interface :as mi]
[metabase.models.table :as table :refer [Table]]
[metabase.util :as u]
[metabase.util.malli :as mu]
[metabase.util.malli.schema :as ms]
[toucan2.core :as t2]))
-(mu/defn ^:private find-gtap-question :- [:maybe (mi/InstanceOf Card)]
+(mu/defn ^:private find-gtap-question :- [:maybe (ms/InstanceOf Card)]
"Find the associated GTAP question (if there is one) for the given `table-or-table-id` and
`user-or-user-id`. Returns nil if no question was found."
[table-or-table-id user-or-user-id]
@@ -30,7 +29,7 @@
(mu/defn only-sandboxed-perms? :- :boolean
"Returns true if the user has sandboxed permissions. If a sandbox policy exists, it overrides existing permission on
the table."
- [table :- (mi/InstanceOf Table)]
+ [table :- (ms/InstanceOf Table)]
(contains? (->> (sandbox.api.util/enforced-sandboxes-for api/*current-user-id*)
(map :table_id)
set)
diff --git a/src/metabase/api/database.clj b/src/metabase/api/database.clj
index 46e5efc2459..3201a2b39e7 100644
--- a/src/metabase/api/database.clj
+++ b/src/metabase/api/database.clj
@@ -313,7 +313,7 @@
;;; --------------------------------------------- GET /api/database/:id ----------------------------------------------
-(mu/defn ^:private expanded-schedules [db :- (mi/InstanceOf Database)]
+(mu/defn ^:private expanded-schedules [db :- (ms/InstanceOf Database)]
{:metadata_sync (u.cron/cron-string->schedule-map (:metadata_sync_schedule db))
:cache_field_values (some-> (:cache_field_values_schedule db) u.cron/cron-string->schedule-map)})
diff --git a/src/metabase/api/native_query_snippet.clj b/src/metabase/api/native_query_snippet.clj
index b39fbb70353..b4ae3f15558 100644
--- a/src/metabase/api/native_query_snippet.clj
+++ b/src/metabase/api/native_query_snippet.clj
@@ -16,7 +16,7 @@
(set! *warn-on-reflection* true)
-(mu/defn ^:private hydrated-native-query-snippet :- [:maybe (mi/InstanceOf NativeQuerySnippet)]
+(mu/defn ^:private hydrated-native-query-snippet :- [:maybe (ms/InstanceOf NativeQuerySnippet)]
[id :- ms/PositiveInt]
(-> (api/read-check (t2/select-one NativeQuerySnippet :id id))
(t2/hydrate :creator)))
diff --git a/src/metabase/domain_entities/core.clj b/src/metabase/domain_entities/core.clj
index b1bd6a30421..29a74ef4a03 100644
--- a/src/metabase/domain_entities/core.clj
+++ b/src/metabase/domain_entities/core.clj
@@ -6,10 +6,10 @@
[metabase.legacy-mbql.util :as mbql.u]
[metabase.lib.util.match :as lib.util.match]
[metabase.models.card :refer [Card]]
- [metabase.models.interface :as mi]
[metabase.models.table :as table :refer [Table]]
[metabase.util :as u]
[metabase.util.malli :as mu]
+ [metabase.util.malli.schema :as ms]
[toucan2.core :as t2]))
(def ^:private ^{:arglists '([field])} field-type
@@ -28,7 +28,7 @@
(def SourceEntity
"A source for a card. Can be either a table or another card."
- [:or (mi/InstanceOf Table) (mi/InstanceOf Card)])
+ [:or (ms/InstanceOf Table) (ms/InstanceOf Card)])
(def Bindings
"Top-level lexical context mapping source names to their corresponding entity and constituent dimensions. See also
diff --git a/src/metabase/driver/common/parameters/dates.clj b/src/metabase/driver/common/parameters/dates.clj
index 5a858eef388..0c9c886926f 100644
--- a/src/metabase/driver/common/parameters/dates.clj
+++ b/src/metabase/driver/common/parameters/dates.clj
@@ -387,8 +387,8 @@
(def ^:private TemporalRange
[:map
- [:start {:optional true} [:fn #(instance? Temporal %)]]
- [:end {:optional true} [:fn #(instance? Temporal %)]]
+ [:start {:optional true} (lib.schema.common/instance-of-class Temporal)]
+ [:end {:optional true} (lib.schema.common/instance-of-class Temporal)]
[:unit TemporalUnit]])
(mu/defn ^:private adjust-inclusive-range-if-needed :- [:maybe TemporalRange]
diff --git a/src/metabase/driver/sql_jdbc/execute.clj b/src/metabase/driver/sql_jdbc/execute.clj
index 73b52074f48..10d02abe8e6 100644
--- a/src/metabase/driver/sql_jdbc/execute.clj
+++ b/src/metabase/driver/sql_jdbc/execute.clj
@@ -251,6 +251,16 @@
(seq more)
(recur more)))))
+(def ^:private DbOrIdOrSpec
+ [:and
+ [:or :int :map]
+ [:fn
+ ;; can't wrap a java.sql.Connection here because we're not
+ ;; responsible for its lifecycle and that means you can't use
+ ;; `with-open` on the Connection you'd get from the DataSource
+ {:error/message "Cannot be a JDBC spec wrapping a java.sql.Connection"}
+ (complement :connection)]])
+
(mu/defn do-with-resolved-connection-data-source :- (lib.schema.common/instance-of-class DataSource)
"Part of the default implementation for [[do-with-connection-with-options]]: get an appropriate `java.sql.DataSource`
for `db-or-id-or-spec`. Not for use with a JDBC spec wrapping a `java.sql.Connection` (a spec with the key
@@ -258,14 +268,7 @@
Connections provided by this DataSource."
{:added "0.47.0", :arglists '(^javax.sql.DataSource [driver db-or-id-or-spec options])}
[driver :- :keyword
- db-or-id-or-spec :- [:and
- [:or :int :map]
- [:fn
- ;; can't wrap a java.sql.Connection here because we're not
- ;; responsible for its lifecycle and that means you can't use
- ;; `with-open` on the Connection you'd get from the DataSource
- {:error/message "Cannot be a JDBC spec wrapping a java.sql.Connection"}
- (complement :connection)]]
+ db-or-id-or-spec :- DbOrIdOrSpec
{:keys [^String session-timezone], :as _options} :- ConnectionOptions]
(if-not (u/id db-or-id-or-spec)
;; not a Database or Database ID... this is a raw `clojure.java.jdbc` spec, use that
diff --git a/src/metabase/email.clj b/src/metabase/email.clj
index 244170bb0cf..1d0d33384f8 100644
--- a/src/metabase/email.clj
+++ b/src/metabase/email.clj
@@ -184,7 +184,7 @@
`:metabase.email/error`, which will either be `nil` (indicating no error) or an instance of [[java.lang.Throwable]]
with the error."
[:map {:closed true}
- [::error [:maybe [:fn #(instance? Throwable %)]]]])
+ [::error [:maybe (ms/InstanceOfClass Throwable)]]])
(defn send-message!
"Send an email to one or more `:recipients`. `:recipients` is a sequence of email addresses; `:message-type` must be
diff --git a/src/metabase/integrations/google.clj b/src/metabase/integrations/google.clj
index 75b76019d11..2b4accb8595 100644
--- a/src/metabase/integrations/google.clj
+++ b/src/metabase/integrations/google.clj
@@ -6,7 +6,6 @@
[metabase.api.common :as api]
[metabase.config :as config]
[metabase.integrations.google.interface :as google.i]
- [metabase.models.interface :as mi]
[metabase.models.setting :as setting :refer [defsetting]]
[metabase.models.setting.multi-setting
:refer [define-multi-setting-impl]]
@@ -16,6 +15,7 @@
[metabase.util.i18n :refer [deferred-tru tru]]
[metabase.util.log :as log]
[metabase.util.malli :as mu]
+ [metabase.util.malli.schema :as ms]
[toucan2.core :as t2]))
;; Load EE implementation if available
@@ -124,7 +124,7 @@
:last_name last-name}))
(assoc user :first_name first-name :last_name last-name))
-(mu/defn ^:private google-auth-fetch-or-create-user! :- (mi/InstanceOf User)
+(mu/defn ^:private google-auth-fetch-or-create-user! :- (ms/InstanceOf User)
[first-name last-name email]
(let [existing-user (t2/select-one [User :id :email :last_login :first_name :last_name] :%lower.email (u/lower-case-en email))]
(if existing-user
diff --git a/src/metabase/lib/convert.cljc b/src/metabase/lib/convert.cljc
index 4195dc2d7b1..6c7ceea52da 100644
--- a/src/metabase/lib/convert.cljc
+++ b/src/metabase/lib/convert.cljc
@@ -3,7 +3,6 @@
[clojure.data :as data]
[clojure.set :as set]
[clojure.string :as str]
- [malli.core :as mc]
[malli.error :as me]
[medley.core :as m]
[metabase.legacy-mbql.normalize :as mbql.normalize]
@@ -17,6 +16,7 @@
[metabase.lib.util :as lib.util]
[metabase.util :as u]
[metabase.util.malli :as mu]
+ [metabase.util.malli.registry :as mr]
#?@(:clj ([metabase.util.log :as log])))
#?@(:cljs [(:require-macros [metabase.lib.convert :refer [with-aggregation-list]])]))
@@ -52,7 +52,7 @@
(binding [lib.schema.expression/*suppress-expression-type-check?* true]
(loop [almost-stage almost-stage
removals []]
- (if-let [[error-type error-location] (->> (mc/explain ::lib.schema/stage.mbql almost-stage)
+ (if-let [[error-type error-location] (->> (mr/explain ::lib.schema/stage.mbql almost-stage)
:errors
(filter (comp stage-keys first :in))
(map (juxt :type :in))
@@ -61,7 +61,7 @@
error-desc (pr-str (or error-type
;; if `error-type` is missing, which seems to happen sometimes,
;; fall back to humanizing the entire error.
- (me/humanize (mc/explain ::lib.schema/stage.mbql almost-stage))))]
+ (me/humanize (mr/explain ::lib.schema/stage.mbql almost-stage))))]
#?(:cljs (js/console.warn "Clean: Removing bad clause due to error!" error-location error-desc
(u/pprint-to-str (first (data/diff almost-stage new-stage))))
:clj (log/warnf "Clean: Removing bad clause in %s due to error %s:\n%s"
diff --git a/src/metabase/lib/metadata/calculation.cljc b/src/metabase/lib/metadata/calculation.cljc
index a06980c5c8d..c3ea6e8f872 100644
--- a/src/metabase/lib/metadata/calculation.cljc
+++ b/src/metabase/lib/metadata/calculation.cljc
@@ -235,11 +235,14 @@
{:query query, :stage-number stage-number, :x x}
e)))))
-(mu/defn metadata :- [:map [:lib/type [:and
- :keyword
- [:fn
- {:error/message ":lib/type should be a :metadata/ keyword"}
- #(= (namespace %) "metadata")]]]]
+(def ^:private MetadataMap
+ [:map [:lib/type [:and
+ :keyword
+ [:fn
+ {:error/message ":lib/type should be a :metadata/ keyword"}
+ #(= (namespace %) "metadata")]]]])
+
+(mu/defn metadata :- MetadataMap
"Calculate an appropriate `:metadata/*` object for something. What this looks like depends on what we're calculating
metadata for. If it's a reference or expression of some sort, this should return a single `:metadata/column`
map (i.e., something satisfying the `::lib.schema.metadata/column` schema."
diff --git a/src/metabase/lib/query.cljc b/src/metabase/lib/query.cljc
index f5588169b69..6f58d687131 100644
--- a/src/metabase/lib/query.cljc
+++ b/src/metabase/lib/query.cljc
@@ -1,7 +1,6 @@
(ns metabase.lib.query
(:refer-clojure :exclude [remove])
(:require
- [malli.core :as mc]
[medley.core :as m]
[metabase.legacy-mbql.normalize :as mbql.normalize]
[metabase.lib.convert :as lib.convert]
@@ -19,7 +18,8 @@
[metabase.lib.util.match :as lib.util.match]
[metabase.shared.util.i18n :as i18n]
[metabase.util :as u]
- [metabase.util.malli :as mu]))
+ [metabase.util.malli :as mu]
+ [metabase.util.malli.registry :as mr]))
(defmethod lib.metadata.calculation/metadata-method :mbql/query
[_query _stage-number _query]
@@ -64,7 +64,7 @@
"Returns whether the query is runnable. Manually validate schema for cljs."
[query :- ::lib.schema/query]
(and (binding [lib.schema.expression/*suppress-expression-type-check?* true]
- (mc/validate ::lib.schema/query query))
+ (mr/validate ::lib.schema/query query))
(boolean (can-run-method query))))
(mu/defn can-save :- :boolean
diff --git a/src/metabase/lib/remove_replace.cljc b/src/metabase/lib/remove_replace.cljc
index 374224d61b0..eb1fdb0844c 100644
--- a/src/metabase/lib/remove_replace.cljc
+++ b/src/metabase/lib/remove_replace.cljc
@@ -12,10 +12,12 @@
[metabase.lib.metadata.calculation :as lib.metadata.calculation]
[metabase.lib.options :as lib.options]
[metabase.lib.ref :as lib.ref]
+ [metabase.lib.schema :as lib.schema]
[metabase.lib.util :as lib.util]
[metabase.lib.util.match :as lib.util.match]
[metabase.util :as u]
- [metabase.util.malli :as mu]))
+ [metabase.util.malli :as mu]
+ [metabase.util.malli.registry :as mr]))
(defn- stage-paths
[query stage-number]
@@ -339,7 +341,7 @@
replacement :- :metabase.lib.schema.expression/expression]
(mu/disable-enforcement
(loop [query (tweak-expression unmodified-query stage-number target replacement)]
- (let [explanation (mc/explain :metabase.lib.schema/query query)
+ (let [explanation (mr/explain ::lib.schema/query query)
error-paths (->> (:errors explanation)
(keep #(on-stage-path query %))
distinct)]
diff --git a/src/metabase/lib/schema/common.cljc b/src/metabase/lib/schema/common.cljc
index 3ee80512a06..6f588a25791 100644
--- a/src/metabase/lib/schema/common.cljc
+++ b/src/metabase/lib/schema/common.cljc
@@ -155,9 +155,7 @@
[:options {:optional true} ::options]])
#?(:clj
- (defn instance-of-class
- "Convenience for defining a Malli schema for an instance of a particular Class."
- [& classes]
+ (defn- instance-of-class* [& classes]
[:fn {:error/message (str "instance of "
(str/join " or "
(map #(.getName ^Class %) classes)))}
@@ -165,3 +163,8 @@
(some (fn [klass]
(instance? klass x))
classes))]))
+
+#?(:clj
+ (def ^{:arglists '([& classes])} instance-of-class
+ "Convenience for defining a Malli schema for an instance of a particular Class."
+ (memoize instance-of-class*)))
diff --git a/src/metabase/lib/schema/expression/arithmetic.cljc b/src/metabase/lib/schema/expression/arithmetic.cljc
index 6c4fc42a531..bc7557f9d3e 100644
--- a/src/metabase/lib/schema/expression/arithmetic.cljc
+++ b/src/metabase/lib/schema/expression/arithmetic.cljc
@@ -1,7 +1,6 @@
(ns metabase.lib.schema.expression.arithmetic
"Arithmetic expressions like `:+`."
(:require
- [malli.core :as mc]
[medley.core :as m]
[metabase.lib.hierarchy :as lib.hierarchy]
[metabase.lib.schema.common :as common]
@@ -17,7 +16,7 @@
(isa? expr-type :type/Time) ::temporal-bucketing/unit.time.interval
(isa? expr-type :type/DateTime) ::temporal-bucketing/unit.date-time.interval)]
(if unit-schema
- (mc/validate unit-schema unit)
+ (mr/validate unit-schema unit)
true)))
(mr/def ::args.numbers
diff --git a/src/metabase/lib/schema/expression/temporal.cljc b/src/metabase/lib/schema/expression/temporal.cljc
index b3c9f84aea7..b9b44d7448a 100644
--- a/src/metabase/lib/schema/expression/temporal.cljc
+++ b/src/metabase/lib/schema/expression/temporal.cljc
@@ -1,7 +1,6 @@
(ns metabase.lib.schema.expression.temporal
(:require
[clojure.set :as set]
- [malli.core :as mc]
[metabase.lib.hierarchy :as lib.hierarchy]
[metabase.lib.schema.common :as common]
[metabase.lib.schema.expression :as expression]
@@ -152,7 +151,7 @@
(when (= value :current)
(cond
(= unit :default) :type/DateTime
- (mc/validate ::temporal-bucketing/unit.date unit) :type/Date
+ (mr/validate ::temporal-bucketing/unit.date unit) :type/Date
:else :type/DateTime))
;; handle year-month and year string regexes, which are not allowed as date literals unless wrapped in
;; `:absolute-datetime`.
diff --git a/src/metabase/lib/schema/literal.cljc b/src/metabase/lib/schema/literal.cljc
index f4590245a8f..6912442b0de 100644
--- a/src/metabase/lib/schema/literal.cljc
+++ b/src/metabase/lib/schema/literal.cljc
@@ -2,7 +2,6 @@
"Malli schemas for string, temporal, number, and boolean literals."
(:require
#?@(:clj ([metabase.lib.schema.literal.jvm]))
- [malli.core :as mc]
[metabase.lib.schema.common :as common]
[metabase.lib.schema.expression :as expression]
[metabase.lib.schema.mbql-clause :as mbql-clause]
@@ -81,7 +80,7 @@
(defmethod expression/type-of-method :dispatch-type/string
[s]
- (condp mc/validate s
+ (condp mr/validate s
::string.datetime #{:type/Text :type/DateTime}
::string.date #{:type/Text :type/Date}
::string.time #{:type/Text :type/Time}
diff --git a/src/metabase/lib/schema/mbql_clause.cljc b/src/metabase/lib/schema/mbql_clause.cljc
index 42211013142..3495ef281aa 100644
--- a/src/metabase/lib/schema/mbql_clause.cljc
+++ b/src/metabase/lib/schema/mbql_clause.cljc
@@ -18,6 +18,9 @@
[tag]
(keyword "mbql.clause" (name tag)))
+(def ^:private invalid-clause-schema
+ [:fn {:error/message "not a known MBQL clause"} (constantly false)])
+
(defn- clause-schema
"Build the schema for `::clause`, a `:multi` schema that maps MBQL clause tag -> the schema
in [[clause-schema-registry]]."
@@ -28,7 +31,7 @@
(if-let [tag (common/mbql-clause-tag value)]
(str "Invalid " tag " clause: " (pr-str value))
"not an MBQL clause"))}
- [::mc/default [:fn {:error/message "not a known MBQL clause"} (constantly false)]]]
+ [::mc/default invalid-clause-schema]]
(map (fn [tag]
[tag [:ref (tag->registered-schema-name tag)]]))
@tag-registry))
diff --git a/src/metabase/models/collection.clj b/src/metabase/models/collection.clj
index 4a06ea65368..15a6ef0c84e 100644
--- a/src/metabase/models/collection.clj
+++ b/src/metabase/models/collection.clj
@@ -375,7 +375,7 @@
;;; | Nested Collections: Ancestors, Childrens, Child Collections |
;;; +----------------------------------------------------------------------------------------------------------------+
-(mu/defn ^:private ancestors* :- [:maybe [:sequential (mi/InstanceOf Collection)]]
+(mu/defn ^:private ancestors* :- [:maybe [:sequential (ms/InstanceOf Collection)]]
[{:keys [location]}]
(when-let [ancestor-ids (seq (location-path->ids location))]
(t2/select [Collection :name :id :personal_owner_id]
@@ -389,7 +389,7 @@
[collection]
(ancestors* collection))
-(mu/defn ^:private effective-ancestors* :- [:sequential [:or RootCollection (mi/InstanceOf Collection)]]
+(mu/defn ^:private effective-ancestors* :- [:sequential [:or RootCollection (ms/InstanceOf Collection)]]
[collection :- CollectionWithLocationAndIDOrRoot]
(if (collection.root/is-root-collection? collection)
[]
@@ -441,7 +441,7 @@
(def ^:private Children
[:schema
{:registry {::children [:and
- (mi/InstanceOf Collection)
+ (ms/InstanceOf Collection)
[:map
[:children [:set [:ref ::children]]]]]}}
[:ref ::children]])
@@ -548,7 +548,7 @@
:from [[:collection :col]]
:where (apply effective-children-where-clause collection additional-honeysql-where-clauses)})
-(mu/defn ^:private effective-children* :- [:set (mi/InstanceOf Collection)]
+(mu/defn ^:private effective-children* :- [:set (ms/InstanceOf Collection)]
[collection :- CollectionWithLocationAndIDOrRoot & additional-honeysql-where-clauses]
(set (t2/select [Collection :id :name :description]
{:where (apply effective-children-where-clause collection additional-honeysql-where-clauses)})))
@@ -822,7 +822,7 @@
would immediately become invisible to all save admins, because no Group would have perms for it. This is obviously a
bad experience -- we do not want a User to move a Collection that they have read/write perms for (by definition) to
somewhere else and lose all access for it."
- [collection :- (mi/InstanceOf Collection) new-location :- LocationPath]
+ [collection :- (ms/InstanceOf Collection) new-location :- LocationPath]
(copy-collection-permissions! (parent {:location new-location}) (cons collection (descendants collection))))
(mu/defn ^:private revoke-perms-when-moving-into-personal-collection!
@@ -831,7 +831,7 @@
need to be deleted, so other users cannot access this newly-Personal Collection.
This needs to be done recursively for all descendants as well."
- [collection :- (mi/InstanceOf Collection)]
+ [collection :- (ms/InstanceOf Collection)]
(t2/query-one {:delete-from :permissions
:where [:in :object (for [collection (cons collection (descendants collection))
path-fn [perms/collection-read-path
@@ -1107,14 +1107,14 @@
:name collection-name
:slug (u/slugify collection-name)))))
-(mu/defn user->existing-personal-collection :- [:maybe (mi/InstanceOf Collection)]
+(mu/defn user->existing-personal-collection :- [:maybe (ms/InstanceOf Collection)]
"For a `user-or-id`, return their personal Collection, if it already exists.
Use [[metabase.models.collection/user->personal-collection]] to fetch their personal Collection *and* create it if
needed."
[user-or-id]
(t2/select-one Collection :personal_owner_id (u/the-id user-or-id)))
-(mu/defn user->personal-collection :- (mi/InstanceOf Collection)
+(mu/defn user->personal-collection :- (ms/InstanceOf Collection)
"Return the Personal Collection for `user-or-id`, if it already exists; if not, create it and return it."
[user-or-id]
(or (user->existing-personal-collection user-or-id)
diff --git a/src/metabase/models/dispatch.clj b/src/metabase/models/dispatch.clj
index 020ad5b89c9..0716c9e2cdd 100644
--- a/src/metabase/models/dispatch.clj
+++ b/src/metabase/models/dispatch.clj
@@ -15,16 +15,3 @@
"True if `x` is a Toucan instance, but not a Toucan model."
[x]
(t2/instance? x))
-
-(defn InstanceOf
- "Helper for creating a Malli schema to check whether something is an instance of `model`. Use this instead of of using
- the `<Model>Instance` or calling [[type]] or [[class]] on a model yourself, since that won't work once we switch to
- Toucan 2.
-
- (mu/defn my-fn :- (mi/InstanceOf User)
- []
- ...)"
- [model]
- [:fn
- {:error/message (format "instance of a %s" (name model))}
- (partial instance-of? model)])
diff --git a/src/metabase/models/interface.clj b/src/metabase/models/interface.clj
index 2a4a6554436..4808d42a271 100644
--- a/src/metabase/models/interface.clj
+++ b/src/metabase/models/interface.clj
@@ -43,7 +43,6 @@
(p/import-vars
[models.dispatch
toucan-instance?
- InstanceOf
instance-of?
model
instance])
diff --git a/src/metabase/models/pulse.clj b/src/metabase/models/pulse.clj
index de167bbc194..ddac37283e3 100644
--- a/src/metabase/models/pulse.clj
+++ b/src/metabase/models/pulse.clj
@@ -257,29 +257,29 @@
;;; ---------------------------------------- Notification Fetching Helper Fns ----------------------------------------
-(mu/defn hydrate-notification :- (mi/InstanceOf Pulse)
+(mu/defn hydrate-notification :- (ms/InstanceOf Pulse)
"Hydrate Pulse or Alert with the Fields needed for sending it."
- [notification :- (mi/InstanceOf Pulse)]
+ [notification :- (ms/InstanceOf Pulse)]
(-> notification
(t2/hydrate :creator :cards [:channels :recipients])
(m/dissoc-in [:details :emails])))
-(mu/defn ^:private hydrate-notifications :- [:sequential (mi/InstanceOf Pulse)]
+(mu/defn ^:private hydrate-notifications :- [:sequential (ms/InstanceOf Pulse)]
"Batched-hydrate multiple Pulses or Alerts."
- [notifications :- [:sequential (mi/InstanceOf Pulse)]]
+ [notifications :- [:sequential (ms/InstanceOf Pulse)]]
(as-> notifications <>
(t2/hydrate <> :creator :cards [:channels :recipients])
(map #(m/dissoc-in % [:details :emails]) <>)))
-(mu/defn ^:private notification->pulse :- (mi/InstanceOf Pulse)
+(mu/defn ^:private notification->pulse :- (ms/InstanceOf Pulse)
"Take a generic `Notification`, and put it in the standard Pulse format the frontend expects. This really just
consists of removing associated `Alert` columns."
- [notification :- (mi/InstanceOf Pulse)]
+ [notification :- (ms/InstanceOf Pulse)]
(dissoc notification :alert_condition :alert_above_goal :alert_first_only))
;; TODO - do we really need this function? Why can't we just use `t2/select` and `hydrate` like we do for everything
;; else? (#40016)
-(mu/defn retrieve-pulse :- [:maybe (mi/InstanceOf Pulse)]
+(mu/defn retrieve-pulse :- [:maybe (ms/InstanceOf Pulse)]
"Fetch a single *Pulse*, and hydrate it with a set of 'standard' hydrations; remove Alert columns, since this is a
*Pulse* and they will all be unset."
[pulse-or-id]
@@ -287,7 +287,7 @@
hydrate-notification
notification->pulse))
-(mu/defn retrieve-notification :- [:maybe (mi/InstanceOf Pulse)]
+(mu/defn retrieve-notification :- [:maybe (ms/InstanceOf Pulse)]
"Fetch an Alert or Pulse, and do the 'standard' hydrations, adding `:channels` with `:recipients`, `:creator`, and
`:cards`."
[notification-or-id & additional-conditions]
@@ -295,15 +295,15 @@
(some-> (apply t2/select-one Pulse :id (u/the-id notification-or-id), additional-conditions)
hydrate-notification))
-(mu/defn ^:private notification->alert :- (mi/InstanceOf Pulse)
+(mu/defn ^:private notification->alert :- (ms/InstanceOf Pulse)
"Take a generic `Notification` and put it in the standard `Alert` format the frontend expects. This really just
consists of collapsing `:cards` into a `:card` key with whatever the first Card is."
- [notification :- (mi/InstanceOf Pulse)]
+ [notification :- (ms/InstanceOf Pulse)]
(-> notification
(assoc :card (first (:cards notification)))
(dissoc :cards)))
-(mu/defn retrieve-alert :- [:maybe (mi/InstanceOf Pulse)]
+(mu/defn retrieve-alert :- [:maybe (ms/InstanceOf Pulse)]
"Fetch a single Alert by its `id` value, do the standard hydrations, and put it in the standard `Alert` format."
[alert-or-id]
(some-> (t2/select-one Pulse, :id (u/the-id alert-or-id), :alert_condition [:not= nil])
@@ -313,7 +313,7 @@
(defn- query-as [model query]
(t2/select model query))
-(mu/defn retrieve-alerts :- [:sequential (mi/InstanceOf Pulse)]
+(mu/defn retrieve-alerts :- [:sequential (ms/InstanceOf Pulse)]
"Fetch all Alerts."
([]
(retrieve-alerts nil))
@@ -342,7 +342,7 @@
:when (:card alert)]
alert))))
-(mu/defn retrieve-pulses :- [:sequential (mi/InstanceOf Pulse)]
+(mu/defn retrieve-pulses :- [:sequential (ms/InstanceOf Pulse)]
"Fetch all `Pulses`. When `user-id` is included, only fetches `Pulses` for which the provided user is the creator
or a recipient."
[{:keys [archived? dashboard-id user-id]
diff --git a/src/metabase/pulse.clj b/src/metabase/pulse.clj
index 995180423cb..43ff4778239 100644
--- a/src/metabase/pulse.clj
+++ b/src/metabase/pulse.clj
@@ -27,6 +27,7 @@
[metabase.util.i18n :refer [trs tru]]
[metabase.util.log :as log]
[metabase.util.malli :as mu]
+ [metabase.util.malli.schema :as ms]
[metabase.util.retry :as retry]
[metabase.util.ui-logic :as ui-logic]
[metabase.util.urls :as urls]
@@ -220,7 +221,7 @@
(mu/defn defaulted-timezone :- :string
"Returns the timezone ID for the given `card`. Either the report timezone (if applicable) or the JVM timezone."
- [card :- (mi/InstanceOf :model/Card)]
+ [card :- (ms/InstanceOf :model/Card)]
(or (some->> card database-id (t2/select-one Database :id) qp.timezone/results-timezone-id)
(qp.timezone/system-timezone-id)))
diff --git a/src/metabase/query_processor/execute.clj b/src/metabase/query_processor/execute.clj
index 94cd44e79e8..0211301e781 100644
--- a/src/metabase/query_processor/execute.clj
+++ b/src/metabase/query_processor/execute.clj
@@ -60,15 +60,18 @@
(log/infof "%s changed, rebuilding %s" varr `execute*)
(rebuild-execute-fn!))))
+(def ^:private CompiledQuery
+ [:and
+ [:map
+ [:database ::lib.schema.id/database]]
+ [:fn
+ {:error/message "Query must be compiled -- should have either :native or :qp/compiled."}
+ (some-fn :native :qp/compiled)]])
+
;;; TODO -- consider whether this should return an `IReduceInit` that we can reduce as a separate step.
(mu/defn execute :- some?
"Execute a compiled query, then reduce the results."
- [compiled-query :- [:and
- [:map
- [:database ::lib.schema.id/database]]
- [:fn
- {:error/message "Query must be compiled -- should have either :native or :qp/compiled."}
- (some-fn :native :qp/compiled)]]
+ [compiled-query :- CompiledQuery
rff :- ::qp.schema/rff]
(qp.setup/with-qp-setup [compiled-query compiled-query]
(execute* compiled-query rff)))
diff --git a/src/metabase/query_processor/middleware/annotate.clj b/src/metabase/query_processor/middleware/annotate.clj
index 0eaaaa6c753..5a259076dda 100644
--- a/src/metabase/query_processor/middleware/annotate.clj
+++ b/src/metabase/query_processor/middleware/annotate.clj
@@ -366,16 +366,19 @@
(update-keys u/->snake_case_en)
(dissoc :lib/type)))))
+(def ^:private LegacyInnerQuery
+ [:and
+ :map
+ [:fn
+ {:error/message "legacy inner-query with :source-table or :source-query"}
+ (some-fn :source-table :source-query)]])
+
(mu/defn aggregation-name :- ::lib.schema.common/non-blank-string
"Return an appropriate aggregation name/alias *used inside a query* for an `:aggregation` subclause (an aggregation
or expression). Takes an options map as schema won't support passing keypairs directly as a varargs.
These names are also used directly in queries, e.g. in the equivalent of a SQL `AS` clause."
- [inner-query :- [:and
- :map
- [:fn
- {:error/message "legacy inner-query with :source-table or :source-query"}
- (some-fn :source-table :source-query)]]
+ [inner-query :- LegacyInnerQuery
ag-clause]
(lib/column-name (mlv2-query inner-query) (lib.convert/->pMBQL ag-clause)))
diff --git a/src/metabase/query_processor/middleware/normalize_query.clj b/src/metabase/query_processor/middleware/normalize_query.clj
index 7fc64aeaa3e..ca72ffdc479 100644
--- a/src/metabase/query_processor/middleware/normalize_query.clj
+++ b/src/metabase/query_processor/middleware/normalize_query.clj
@@ -20,14 +20,17 @@
;; a well-formed pMBQL query, we don't need that and it actually ends up breaking some stuff
(lib/query metadata-provider (dissoc query :lib.convert/converted?))))
-(mu/defn normalize-preprocessing-middleware :- [:and
- [:map
- [:database ::lib.schema.id/database]
- [:lib/type {:optional true} [:= :mbql/query]]
- [:type {:optional true} [:= :internal]]]
- [:fn
- {:error/message "valid pMBQL query or :internal audit query"}
- (some-fn :lib/type :type)]]
+(def ^:private NormalizedQuery
+ [:and
+ [:map
+ [:database ::lib.schema.id/database]
+ [:lib/type {:optional true} [:= :mbql/query]]
+ [:type {:optional true} [:= :internal]]]
+ [:fn
+ {:error/message "valid pMBQL query or :internal audit query"}
+ (some-fn :lib/type :type)]])
+
+(mu/defn normalize-preprocessing-middleware :- NormalizedQuery
"Preprocessing middleware. Normalize a query, meaning do things like convert keys and MBQL clause tags to kebab-case
keywords. Convert query to pMBQL if needed."
[query :- [:map [:database ::lib.schema.id/database]]]
diff --git a/src/metabase/sync/sync_metadata/sync_table_privileges.clj b/src/metabase/sync/sync_metadata/sync_table_privileges.clj
index fe1a833e91a..8df930fda35 100644
--- a/src/metabase/sync/sync_metadata/sync_table_privileges.clj
+++ b/src/metabase/sync/sync_metadata/sync_table_privileges.clj
@@ -2,8 +2,8 @@
(:require
[metabase.driver :as driver]
[metabase.driver.util :as driver.u]
- [metabase.models.interface :as mi]
[metabase.util.malli :as mu]
+ [metabase.util.malli.schema :as ms]
[toucan2.core :as t2]))
(set! *warn-on-reflection* true)
@@ -12,7 +12,7 @@
"Sync the `table_privileges` table with the privileges in the database.
This is a cache of the data returned from `driver/table-privileges`, but it's stored in the database for performance."
- [database :- (mi/InstanceOf :model/Database)]
+ [database :- (ms/InstanceOf :model/Database)]
(let [driver (driver.u/database->driver database)]
(when (and (not= :redshift driver)
;; redshift does support table-privileges, but we don't want to sync it now because table privileges are
diff --git a/src/metabase/transforms/core.clj b/src/metabase/transforms/core.clj
index bbc52f18f50..daa59707d50 100644
--- a/src/metabase/transforms/core.clj
+++ b/src/metabase/transforms/core.clj
@@ -19,6 +19,7 @@
[metabase.util :as u]
[metabase.util.i18n :refer [tru]]
[metabase.util.malli :as mu]
+ [metabase.util.malli.schema :as ms]
[toucan2.core :as t2]))
(mu/defn ^:private add-bindings :- Bindings
@@ -137,7 +138,7 @@
:dimensions (infer-resulting-dimensions local-bindings step query)})))
(def ^:private Tableset
- [:sequential (mi/InstanceOf Table)])
+ [:sequential (ms/InstanceOf Table)])
(mu/defn ^:private find-tables-with-domain-entity :- Tableset
[tableset :- Tableset
diff --git a/src/metabase/util/date_2/parse.clj b/src/metabase/util/date_2/parse.clj
index 80706d1e00b..c6d63639702 100644
--- a/src/metabase/util/date_2/parse.clj
+++ b/src/metabase/util/date_2/parse.clj
@@ -42,8 +42,12 @@
:when (.isSupported temporal-accessor field)]
[k (.getLong temporal-accessor field)])))
-(mu/defn parse-with-formatter :- [:maybe [:fn {:error/message "Instance of java.time.temporal.Temporal"}
- (partial instance? Temporal)]]
+(def ^:private InstanceOfTemporal
+ [:fn
+ {:error/message "Instance of a java.time.temporal.Temporal"}
+ (partial instance? Temporal)])
+
+(mu/defn parse-with-formatter :- [:maybe InstanceOfTemporal]
"Parse a String with a DateTimeFormatter, returning an appropriate instance of an `java.time` temporal class."
[formattr
s :- [:maybe :string]]
diff --git a/src/metabase/util/malli/schema.clj b/src/metabase/util/malli/schema.clj
index 7172f2da9eb..b4a5147d194 100644
--- a/src/metabase/util/malli/schema.clj
+++ b/src/metabase/util/malli/schema.clj
@@ -21,38 +21,41 @@
;;; -------------------------------------------------- Utils --------------------------------------------------
;;; TODO -- consider renaming this to `InstanceOfModel` to differentiate it from [[InstanceOfClass]]
-(defn InstanceOf
+(def ^{:arglists '([model])} InstanceOf
"Helper for creating a schema to check whether something is an instance of `model`.
(ms/defn my-fn
[user :- (ms/InstanceOf User)]
...)"
- [model]
- (mu/with-api-error-message
- [:fn
- {:error/message (format "value must be an instance of %s" (name model))}
- #(models.dispatch/instance-of? model %)]
- (deferred-tru "value must be an instance of {0}" (name model))))
+ (memoize
+ (fn [model]
+ (mu/with-api-error-message
+ [:fn
+ {:error/message (format "value must be an instance of %s" (name model))}
+ #(models.dispatch/instance-of? model %)]
+ (deferred-tru "value must be an instance of {0}" (name model))))))
-(defn InstanceOfClass
+(def ^{:arglists '([^Class klass])} InstanceOfClass
"Helper for creating schemas to check whether something is an instance of a given class."
- [^Class klass]
- [:fn
- {:error/message (format "Instance of a %s" (.getCanonicalName klass))}
- (partial instance? klass)])
+ (memoize
+ (fn [^Class klass]
+ [:fn
+ {:error/message (format "Instance of a %s" (.getCanonicalName klass))}
+ (partial instance? klass)])))
-(defn maps-with-unique-key
+(def ^{:arglists '([maps-schema k])} maps-with-unique-key
"Given a schema of a sequence of maps, returns a schema that does an additional unique check on key `k`."
- [maps-schema k]
- (mu/with-api-error-message
- [:and
- [:fn (fn [maps]
- (= (count maps)
- (-> (map #(get % k) maps)
- distinct
- count)))]
- maps-schema]
- (deferred-tru "value must be seq of maps in which {0}s are unique" (name k))))
+ (memoize
+ (fn [maps-schema k]
+ (mu/with-api-error-message
+ [:and
+ [:fn (fn [maps]
+ (= (count maps)
+ (-> (map #(get % k) maps)
+ distinct
+ count)))]
+ maps-schema]
+ (deferred-tru "value must be seq of maps in which {0}s are unique" (name k))))))
;;; -------------------------------------------------- Schemas --------------------------------------------------
diff --git a/test/metabase/test/data.clj b/test/metabase/test/data.clj
index 881c245873f..84e33948709 100644
--- a/test/metabase/test/data.clj
+++ b/test/metabase/test/data.clj
@@ -187,19 +187,22 @@
[table-name & [query]]
`(run-mbql-query* (mbql-query ~table-name ~(or query {}))))
+(def ^:private FormattableName
+ [:or
+ :keyword
+ :string
+ :symbol
+ [:fn
+ {:error/message (str "Cannot format `nil` name -- did you use a `$field` without specifying its Table? "
+ "(Change the form to `$table.field`, or specify a top-level default Table to "
+ "`$ids` or `mbql-query`.)")}
+ (constantly false)]])
+
(mu/defn format-name :- :string
"Format a SQL schema, table, or field identifier in the correct way for the current database by calling the current
driver's implementation of [[ddl.i/format-name]]. (Most databases use the default implementation of `identity`; H2
uses [[clojure.string/upper-case]].) This function DOES NOT quote the identifier."
- [a-name :- [:or
- :keyword
- :string
- :symbol
- [:fn
- {:error/message (str "Cannot format `nil` name -- did you use a `$field` without specifying its Table? "
- "(Change the form to `$table.field`, or specify a top-level default Table to "
- "`$ids` or `mbql-query`.)")}
- (constantly false)]]]
+ [a-name :- FormattableName]
(ddl.i/format-name (tx/driver) (name a-name)))
(defn id
--
GitLab