Skip to content
Snippets Groups Projects
Unverified Commit 1e9e60a0 authored by Cam Saul's avatar Cam Saul Committed by GitHub
Browse files

Cam's module linter 5000 (#42250)

* Cam's module linter 5000

* Get Kondo passing

* Revert changes to usage of mdb.setup/setup-db!

* Check ns shapes

* Add ns :require shape linter; PR feedback

* Linter fixes :wrench:
parent f418e685
No related branches found
No related tags found
No related merge requests found
......@@ -14,6 +14,7 @@
:redundant-fn-wrapper {:level :warning}
:refer-all {:level :warning, :exclude [clojure.test]}
:single-key-in {:level :warning}
:unsorted-required-namespaces {:level :warning}
:unused-referred-var {:exclude {compojure.core [GET DELETE POST PUT]}}
:use {:level :warning}
:warn-on-reflection {:level :warning}
......@@ -22,6 +23,7 @@
:metabase/i-like-making-cams-eyes-bleed-with-horrifically-long-tests {:level :warning}
:metabase/mbql-query-first-arg {:level :warning}
:metabase/missing-test-expr-requires-in-cljs {:level :warning}
:metabase/require-shape-checker {:level :warning}
:metabase/test-helpers-use-non-thread-safe-functions {:level :warning}
:metabase/validate-deftest {:level :warning}
:metabase/validate-logging {:level :warning}
......@@ -122,7 +124,138 @@
{:exclude
[colorize.core]}
:unsorted-required-namespaces {:level :warning}
;; A "module" is any `metabase.x` namespace in `src`.
:metabase/ns-module-checker
{:level :warning
;; Map of module name => the "core" external public-facing API namespace(s). You have three options here:
;;
;; 1. Special sentinel value `:any` means means this module does not (yet) have external public-facing API
;; namespace(s). This is mostly a temporary placeholder until we go in and create module namespaces, which means
;; you should go create one.
;;
;; 2. A set of namespace symbols. All namespaces in other modules will only be allowed ;; to use namespaces from
;; this set. Ideally this set should only have one namespace, but restricting it to a set of several is still
;; better than `:any`.
;;
;; 3. `nil` or not listed here -- we default to assuming there is one API namespace that matches the name of the
;; module itself, e.g. the module namespace for `metabase.setup` would be `metabase.setup`.
;;
;; `nil` or an empty set Otherwise this should be a set of namespace symbols.
:api-namespaces
{metabase.actions :any
metabase.analytics :any
metabase.analyze :any
metabase.api :any
metabase.async :any
metabase.automagic-dashboards :any
metabase.bootstrap #{metabase.bootstrap}
metabase.cmd :any
metabase.config #{metabase.config}
metabase.core :any
metabase.db #{metabase.db
metabase.db.metadata-queries ; FIXME, this should probably be separate from metabase.db
metabase.db.query ; FIXME, this is mostly util stuff like `metabase.db.query/query` that we don't even need anymore.
metabase.db.setup} ; FIXME, these are only calling `metabase.db.setup/setup-db!` and there's a slightly different version in `metabase.db`
metabase.domain-entities :any
metabase.driver :any
metabase.email :any
metabase.embed :any
metabase.events :any
metabase.formatter :any
metabase.integrations :any
metabase.legacy-mbql :any
metabase.lib :any
metabase.logger #{metabase.logger}
metabase.metabot :any
metabase.models :any
metabase.moderation #{metabase.moderation}
metabase.native-query-analyzer :any
metabase.permissions :any
metabase.plugins :any
metabase.public-settings :any
metabase.pulse :any
metabase.query-processor :any
metabase.related #{metabase.related}
metabase.sample-data #{metabase.sample-data}
metabase.search :any
metabase.server :any
metabase.setup #{metabase.setup}
metabase.shared :any
metabase.sync :any
metabase.task :any
metabase.transforms :any
metabase.troubleshooting #{metabase.troubleshooting}
metabase.types #{metabase.types}
metabase.upload :any
metabase.util :any}
;; Map of module => other modules you're allowed to use there. You have two options here:
;;
;; 1. `:any` means namespaces in this module are allowed to use any other module -- allowed modules are not
;; enforced for this module. Module API namespaces for modules that have them defined are still enforced. For
;; ones that are `nil`, please go in and add a list of allowed modules. `:any` is mostly meant a temporary
;; placeholder until we can fill these all out, so feel free to fix these.
;;
;; 2. A set of module symbols. This is the list of modules that are allowed to be referenced. An empty set means no
;; other modules are allowed to be referenced; this is the default for any modules that aren't listed here.
:allowed-modules
{metabase.actions :any
metabase.analytics :any
metabase.analyze :any
metabase.api :any
metabase.async :any
metabase.automagic-dashboards :any
metabase.bootstrap #{}
metabase.cmd :any
metabase.config #{metabase.plugins}
metabase.core :any
metabase.db :any
metabase.domain-entities :any
metabase.driver :any
metabase.email :any
metabase.embed :any
metabase.events :any
metabase.formatter :any
metabase.integrations :any
metabase.legacy-mbql :any
metabase.lib :any
metabase.logger #{metabase.config
metabase.plugins}
metabase.metabot :any
metabase.models :any
metabase.moderation :any
metabase.native-query-analyzer :any
metabase.permissions :any
metabase.plugins :any
metabase.public-settings :any
metabase.pulse :any
metabase.query-processor :any
metabase.related :any
metabase.sample-data :any
metabase.search :any
metabase.server :any
metabase.setup :any
metabase.shared :any
metabase.sync :any
metabase.task :any
metabase.transforms :any
metabase.troubleshooting :any
metabase.types #{metabase.util}
metabase.upload :any
metabase.util :any}
;; namespaces matching these patterns (with `re-find`) are excluded from the module linter. Since regex literals
;; aren't allowed in EDN just used the `(str <regex>)` version i.e. two slashes instead of one.
;;
;; this is mostly intended for excluding test namespaces or those rare 'glue' namespaces that glue multiple modules
;; together, e.g. `metabase.lib.metadata.jvm`.
:ignored-namespace-patterns
#{"-test$" ; anything ending in `-test`
"test[-.]util" ; anything that contains `test.util` or `test-util`
"test\\.impl" ; anything that contains `test.impl`
"^metabase\\.test" ; anything starting with `metabase.test`
"^metabase\\.http-client$"}} ; `metabase.http-client` which is a test-only namespace despite its name.
:consistent-alias
{:aliases
......@@ -491,7 +624,8 @@
:hooks
{:analyze-call
{cljs.test/deftest hooks.clojure.test/deftest
{clojure.core/ns hooks.clojure.core/lint-ns
cljs.test/deftest hooks.clojure.test/deftest
cljs.test/is hooks.clojure.test/is
cljs.test/use-fixtures hooks.clojure.test/use-fixtures
clojure.test/deftest hooks.clojure.test/deftest
......
(ns hooks.clojure.core
(:require
[clj-kondo.hooks-api :as hooks]
[clojure.set :as set]
[clojure.string :as str]))
(defn- node->qualified-symbol [node]
......@@ -15,7 +16,7 @@
(catch Exception _
nil)))
(def ^:private white-card-symbols
(def ^:private symbols-allowed-in-fns-not-ending-in-an-exclamation-point
'#{;; these toucan methods might actually set global values if it's used outside of a transaction,
;; but since mt/with-temp runs in a transaction, so we'll ignore them in this case.
toucan2.core/delete!
......@@ -159,7 +160,7 @@
(walk f child)))]
(walk (fn [form]
(when-let [qualified-symbol (node->qualified-symbol form)]
(when (and (not (contains? white-card-symbols qualified-symbol))
(when (and (not (contains? symbols-allowed-in-fns-not-ending-in-an-exclamation-point qualified-symbol))
(end-with-exclamation? qualified-symbol))
(hooks/reg-finding! (assoc (meta form-name)
:message (format "The name of this %s should end with `!` because it contains calls to non thread safe form `%s`."
......@@ -203,3 +204,123 @@
:findings))
(do (non-thread-safe-form-should-end-with-exclamation* (hooks/parse-string form)) nil))
(defn- ns-form-node->require-node [ns-form-node]
(some (fn [node]
(when (and (hooks/list-node? node)
(let [first-child (first (:children node))]
(and (hooks/keyword-node? first-child)
(= (hooks/sexpr first-child) :require))))
node))
(:children ns-form-node)))
(defn- lint-require-shapes [ns-form-node]
(doseq [node (-> ns-form-node
ns-form-node->require-node
:children
rest)]
(cond
(not (hooks/vector-node? node))
(hooks/reg-finding! (assoc (meta node)
:message "All :required namespaces should be wrapped in vectors [:metabase/require-shape-checker]"
:type :metabase/require-shape-checker))
(hooks/vector-node? (second (:children node)))
(hooks/reg-finding! (assoc (meta node)
:message "Don't use prefix forms inside :require [:metabase/require-shape-checker]"
:type :metabase/require-shape-checker)))))
(defn- require-node->namespace-symb-nodes [require-node]
(let [[_ns & args] (:children require-node)]
(into []
;; prefixed namespace forms are NOT SUPPORTED!!!!!!!!1
(keep (fn [node]
(cond
(hooks/vector-node? node)
(first (:children node))
(hooks/token-node? node)
node
:else
(printf "Don't know how to figure out what namespace is being required in %s\n" (pr-str node)))))
args)))
(defn- ns-form-node->ns-symb [ns-form-node]
(some-> (some (fn [node]
(when (and (hooks/token-node? node)
(not= (hooks/sexpr node) 'ns))
node))
(:children ns-form-node))
hooks/sexpr))
(defn- module
"E.g.
(module 'metabase.qp.middleware.wow) => 'metabase.qp"
[ns-symb]
(some-> (re-find #"^metabase\.[^.]+" (str ns-symb)) symbol))
(defn- ignored-namespace? [ns-symb config]
(some
(fn [pattern-str]
(re-find (re-pattern pattern-str) (str ns-symb)))
(:ignored-namespace-patterns config)))
(defn- module-api-namespaces
"Set API namespaces for a given module. `:any` means you can use anything, there are no API namespaces for this
module (yet). If unspecified, the default is just the namespace with the same name as the module e.g.
`metabase.db`."
[module config]
(let [module-config (get-in config [:api-namespaces module])]
(cond
(= module-config :any)
nil
(set? module-config)
module-config
:else
#{module})))
(defn- lint-modules [ns-form-node config]
(let [ns-symb (ns-form-node->ns-symb ns-form-node)]
(when-not (ignored-namespace? ns-symb config)
(when-let [current-module (module ns-symb)]
(let [allowed-modules (get-in config [:allowed-modules current-module])
required-namespace-symb-nodes (-> ns-form-node
ns-form-node->require-node
require-node->namespace-symb-nodes)]
(doseq [node required-namespace-symb-nodes
:let [required-namespace (hooks/sexpr node)
required-module (module required-namespace)]
;; ignore stuff not in a module i.e. non-Metabase stuff.
:when required-module
:let [in-current-module? (= required-module current-module)]
:when (not in-current-module?)
:let [allowed-module? (or (= allowed-modules :any)
(contains? (set allowed-modules) required-module))
module-api-namespaces (module-api-namespaces required-module config)
allowed-module-namespace? (or (empty? module-api-namespaces)
(contains? module-api-namespaces required-namespace))]]
(when-let [error (cond
(not allowed-module?)
(format "Module %s should not be used in the %s module. [:metabase/ns-module-checker :allowed-modules %s]"
required-module
current-module
current-module)
(not allowed-module-namespace?)
(format "Namespace %s is not an allowed external API namespace for the %s module. [:metabase/ns-module-checker :api-namespaces %s]"
required-namespace
required-module
required-module))]
(hooks/reg-finding! (assoc (meta node)
:message error
:type :metabase/ns-module-checker)))))))))
(defn lint-ns [x]
(lint-require-shapes (:node x))
(lint-modules (:node x) (get-in x [:config :linters :metabase/ns-module-checker]))
x)
......@@ -205,9 +205,9 @@
(update acc (if (= (keyword card-type) :model) :dataset :card) conj collection-id))
{:dataset #{}
:card #{}}
(mdb.query/reducible-query {:select-distinct [:collection_id :type]
:from [:report_card]
:where [:= :archived false]}))
(t2/reducible-query {:select-distinct [:collection_id :type]
:from [:report_card]
:where [:= :archived false]}))
collections-with-details (map collection/personal-collection-with-ui-details collections)]
(collection/collections->tree collection-type-ids collections-with-details)))))
......@@ -563,11 +563,11 @@
(update acc (if (= (keyword card-type) :model) :dataset :card) conj collection-id))
{:dataset #{}
:card #{}}
(mdb.query/reducible-query {:select-distinct [:collection_id :type]
:from [:report_card]
:where [:and
[:= :archived false]
[:in :collection_id descendant-collection-ids]]}))
(t2/reducible-query {:select-distinct [:collection_id :type]
:from [:report_card]
:where [:and
[:= :archived false]
[:in :collection_id descendant-collection-ids]]}))
collections-containing-dashboards
(->> (t2/query {:select-distinct [:collection_id]
......
......@@ -160,27 +160,27 @@
xform)
(completing conj #(t2/hydrate % :collection))
[]
(mdb.query/reducible-query {:select [:name :description :database_id :dataset_query :id :collection_id :result_metadata
[{:select [:status]
:from [:moderation_review]
:where [:and
[:= :moderated_item_type "card"]
[:= :moderated_item_id :report_card.id]
[:= :most_recent true]]
:order-by [[:id :desc]]
:limit 1}
:moderated_status]]
:from [:report_card]
:where (into [:and
[:not= :result_metadata nil]
[:= :archived false]
[:= :type (u/qualified-name card-type)]
[:in :database_id ids-of-dbs-that-support-source-queries]
(collection/visible-collection-ids->honeysql-filter-clause
(collection/permissions-set->visible-collection-ids
@api/*current-user-permissions-set*))]
additional-constraints)
:order-by [[:%lower.name :asc]]}))))
(t2/reducible-query {:select [:name :description :database_id :dataset_query :id :collection_id :result_metadata
[{:select [:status]
:from [:moderation_review]
:where [:and
[:= :moderated_item_type "card"]
[:= :moderated_item_id :report_card.id]
[:= :most_recent true]]
:order-by [[:id :desc]]
:limit 1}
:moderated_status]]
:from [:report_card]
:where (into [:and
[:not= :result_metadata nil]
[:= :archived false]
[:= :type (u/qualified-name card-type)]
[:in :database_id ids-of-dbs-that-support-source-queries]
(collection/visible-collection-ids->honeysql-filter-clause
(collection/permissions-set->visible-collection-ids
@api/*current-user-permissions-set*))]
additional-constraints)
:order-by [[:%lower.name :asc]]}))))
(mu/defn ^:private source-query-cards-exist?
"Truthy if a single Card that can be used as a source query exists."
......
......@@ -27,6 +27,7 @@
[metabase.util.malli.schema :as ms]
[toucan2.core :as t2]
[toucan2.instance :as t2.instance]
[toucan2.jdbc.options :as t2.jdbc.options]
[toucan2.realize :as t2.realize]))
(set! *warn-on-reflection* true)
......@@ -499,7 +500,10 @@
to-toucan-instance (fn [row]
(let [model (-> row :model search.config/model-to-db-model :db-model)]
(t2.instance/instance model row)))
reducible-results (mdb.query/reducible-query search-query :max-rows search.config/*db-max-results*)
reducible-results (reify clojure.lang.IReduceInit
(reduce [_this rf init]
(binding [t2.jdbc.options/*options* (assoc t2.jdbc.options/*options* :max-rows search.config/*db-max-results*)]
(reduce rf init (t2/reducible-query search-query)))))
xf (comp
(map t2.realize/realize)
(map to-toucan-instance)
......
......@@ -111,7 +111,7 @@
;; should be ok now that #16344 is resolved -- we might be able to remove this code entirely now. Quoting identifiers
;; is still a good idea tho.)
(let [source-keys (keys (first objs))
quote-fn (partial mdb.setup/quote-for-application-db (mdb/quoting-style target-db-type))
quote-fn (partial mdb/quote-for-application-db (mdb/quoting-style target-db-type))
dest-keys (for [k source-keys]
(quote-fn (name k)))]
{:cols dest-keys
......
......@@ -3,7 +3,7 @@
(:require
[clojure.java.io :as io]
[clojure.string :as str]
[metabase.db.data-source :as mdb.data-source]
[metabase.db :as mdb]
[metabase.util :as u]
[metabase.util.log :as log]))
......@@ -25,7 +25,7 @@
"Create a [[javax.sql.DataSource]] for the H2 database with `h2-filename`."
^javax.sql.DataSource [h2-filename]
(let [h2-filename (add-file-prefix-if-needed h2-filename)]
(mdb.data-source/broken-out-details->DataSource :h2 {:db h2-filename})))
(mdb/broken-out-details->DataSource :h2 {:db h2-filename})))
(defn delete-existing-h2-database-files!
"Delete existing h2 database files."
......
......@@ -17,7 +17,6 @@
[metabase.cmd.copy.h2 :as copy.h2]
[metabase.cmd.rotate-encryption-key :as rotate-encryption]
[metabase.db :as mdb]
[metabase.db.connection :as mdb.connection]
[metabase.util.log :as log]))
(defn dump-to-h2!
......@@ -39,5 +38,5 @@
(copy.h2/delete-existing-h2-database-files! h2-filename))
(copy/copy! (mdb/db-type) (mdb/data-source) :h2 h2-data-source)
(when dump-plaintext?
(binding [mdb.connection/*application-db* (mdb.connection/application-db :h2 h2-data-source)]
(mdb/with-application-db (mdb/application-db :h2 h2-data-source)
(rotate-encryption/rotate-encryption-key! nil))))))
......@@ -11,7 +11,9 @@
[metabase.config :as config]
[metabase.db.connection :as mdb.connection]
[metabase.db.connection-pool-setup :as mdb.connection-pool-setup]
[metabase.db.data-source :as mdb.data-source]
[metabase.db.env :as mdb.env]
[metabase.db.jdbc-protocols :as mdb.jdbc-protocols]
[metabase.db.setup :as mdb.setup]
[metabase.db.spec :as mdb.spec]
[potemkin :as p]))
......@@ -19,24 +21,32 @@
(set! *warn-on-reflection* true)
(p/import-vars
[mdb.connection
quoting-style
db-type
unique-identifier
data-source]
[mdb.connection
application-db
data-source
db-type
quoting-style
unique-identifier]
[mdb.connection-pool-setup
recent-activity?]
[mdb.connection-pool-setup
recent-activity?]
[mdb.env
db-file]
[mdb.data-source
broken-out-details->DataSource]
[mdb.setup
migrate!]
[mdb.env
db-file]
[mdb.spec
make-subname
spec])
[mdb.jdbc-protocols
clob->str]
[mdb.setup
migrate!
quote-for-application-db]
[mdb.spec
make-subname
spec])
;; TODO -- consider whether we can just do this automatically when `getConnection` is called on
;; [[mdb.connection/*application-db*]] (or its data source)
......@@ -99,3 +109,15 @@
(assert (or (not config/is-prod?)
(config/config-bool :mb-enable-test-endpoints)))
(alter-var-root #'mdb.connection/*application-db* assoc :id (swap! mdb.connection/application-db-counter inc)))
(defn do-with-application-db
"Impl for [[with-application-db]] macro."
[application-db thunk]
(binding [mdb.connection/*application-db* application-db]
(thunk)))
(defmacro with-application-db
"Bind the current application database and execute body."
{:style/indent [:defn]}
[application-db & body]
`(do-with-application-db ~application-db (^:once fn* [] ~@body)))
......@@ -158,25 +158,6 @@
:uncompiled sql-args-or-honey-sql-map}
e)))))))
(defn reducible-query
"Replacement for [[toucan.db/reducible-query]] -- uses Honey SQL 2 instead of Honey SQL 1, to ease the transition to
the former (and to Toucan 2).
Query the application database and return an `IReduceInit`.
See namespace documentation for [[metabase.db.query]] for pro debugging tips."
[sql-args-or-honey-sql-map & {:as jdbc-options}]
;; make sure [[metabase.db.setup]] gets loaded so default Honey SQL options and the like are loaded.
(classloader/require 'metabase.db.setup)
(let [sql-args (compile sql-args-or-honey-sql-map)]
;; It doesn't really make sense to put a try-catch around this since it will return immediately and not execute
;; until we actually reduce it
(reify clojure.lang.IReduceInit
(reduce [_this rf init]
(binding [t2.jdbc.options/*options* (merge t2.jdbc.options/*options* jdbc-options)]
(reduce rf init (t2/reducible-query sql-args)))))))
(defmacro with-conflict-retry
"Retry a database mutation a single time if it fails due to concurrent insertions.
May retry for other reasons."
......
......@@ -5,7 +5,6 @@
[java-time.api :as t]
[metabase.config :as config]
[metabase.db :as mdb]
[metabase.db.jdbc-protocols :as mdb.jdbc-protocols]
[metabase.driver :as driver]
[metabase.driver.common :as driver.common]
[metabase.driver.h2.actions :as h2.actions]
......@@ -552,7 +551,7 @@
(Class/forName true (classloader/the-classloader)))]
(if (isa? classname Clob)
(fn []
(mdb.jdbc-protocols/clob->str (.getObject rs i)))
(mdb/clob->str (.getObject rs i)))
(fn []
(.getObject rs i)))))
......
......@@ -2,7 +2,7 @@
"Logic for updating FK properties of Fields from metadata fetched from a physical DB."
(:require
[honey.sql :as sql]
[metabase.db.connection :as mdb.connection]
[metabase.db :as mdb]
[metabase.driver :as driver]
[metabase.driver.util :as driver.u]
[metabase.models.table :as table]
......@@ -42,7 +42,7 @@
[:= :t.visibility_type nil]]})
fk-field-id-query (field-id-query db-id fk-table-schema fk-table-name fk-column-name)
pk-field-id-query (field-id-query db-id pk-table-schema pk-table-name pk-column-name)
q (case (mdb.connection/db-type)
q (case (mdb/db-type)
:mysql
{:update [:metabase_field :f]
:join [[fk-field-id-query :fk] [:= :fk.id :f.id]
......@@ -76,7 +76,7 @@
[:or
[:= :f.fk_target_field_id nil]
[:not= :f.fk_target_field_id pk-field-id-query]]]})]
(sql/format q :dialect (mdb.connection/quoting-style (mdb.connection/db-type)))))
(sql/format q :dialect (mdb/quoting-style (mdb/db-type)))))
(mu/defn ^:private mark-fk!
"Updates the `fk_target_field_id` of a Field. Returns 1 if the Field was successfully updated, 0 otherwise."
......
......@@ -11,7 +11,6 @@
[java-time.api :as t]
[malli.core :as mc]
[metabase.config :as config]
[metabase.db.query :as mdb.query]
[metabase.driver.h2 :as h2]
[metabase.driver.util :as driver.u]
[metabase.lib.schema.id :as lib.schema.id]
......@@ -347,7 +346,7 @@
(catch Exception e
(log/warnf e "Error updating database %d for randomized schedules" (u/the-id db))
counter))))
(mdb.query/reducible-query
(t2/reducible-query
{:select [:id :details]
:from [:metabase_database]
:where [:or
......
......@@ -3,7 +3,7 @@
(:require
[clojure.data :as data]
[clojure.java.jdbc :as jdbc]
[metabase.db.jdbc-protocols]
[metabase.db]
[metabase.driver :as driver]
[metabase.driver.sql-jdbc.execute :as sql-jdbc.execute]
[metabase.util :as u]
......@@ -11,7 +11,7 @@
(set! *warn-on-reflection* true)
(comment metabase.db.jdbc-protocols/keep-me)
(comment metabase.db/keep-me)
(defn- jdbc-spec [db-file]
{:classname "org.h2.Driver"
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment