From f926e90838e46f22db7b0e2f2e0219516dada5d0 Mon Sep 17 00:00:00 2001
From: Chris Truter <crisptrutski@users.noreply.github.com>
Date: Tue, 16 Jan 2024 12:52:36 +0200
Subject: [PATCH] Add linter to rachet down implicitly non-exported defsettings
(#37697)
* Add kondo for missing :export? on defsetting
* And tons of explicit ignores, including some inadvertent test vars, probably
* Disable linter in tests
* Handle multi settings
---
.clj-kondo/config.edn | 5 +-
.clj-kondo/hooks/metabase/models/setting.clj | 268 ++++++++++++++++---
src/metabase/driver/util.clj | 3 +-
src/metabase/models/setting.clj | 12 +-
4 files changed, 242 insertions(+), 46 deletions(-)
diff --git a/.clj-kondo/config.edn b/.clj-kondo/config.edn
index 0364d53c904..0b46ca46f5f 100644
--- a/.clj-kondo/config.edn
+++ b/.clj-kondo/config.edn
@@ -18,6 +18,8 @@
:use {:level :warning}
:warn-on-reflection {:level :warning}
+ :metabase/defsetting-must-specify-export {:level :warning}
+
;;
;; disabled linters
;;
@@ -531,7 +533,7 @@
metabase.models.interface/define-batched-hydration-method hooks.metabase.models.interface/define-hydration-method
metabase.models.pulse-test/with-dashboard-subscription-in-collection hooks.common/with-four-bindings
metabase.models.pulse-test/with-pulse-in-collection hooks.common/with-four-bindings
- metabase.models.setting.multi-setting/define-multi-setting hooks.metabase.models.setting/defsetting
+ metabase.models.setting.multi-setting/define-multi-setting hooks.metabase.models.setting/define-multi-setting
metabase.models.setting/defsetting hooks.metabase.models.setting/defsetting
metabase.public-settings.premium-features/defenterprise hooks.metabase.public-settings.premium-features/defenterprise
metabase.pulse.test-util/checkins-query-card hooks.metabase.test.data/$ids
@@ -637,6 +639,7 @@
{:inline-def {:level :off}
:missing-docstring {:level :off}
:private-call {:level :off}
+ :metabase/defsetting-must-specify-export {:level :off}
:hooks.metabase.test.data/mbql-query-first-arg {:level :error}}}
source-namespaces
diff --git a/.clj-kondo/hooks/metabase/models/setting.clj b/.clj-kondo/hooks/metabase/models/setting.clj
index 600d7c515af..4ec17037ea6 100644
--- a/.clj-kondo/hooks/metabase/models/setting.clj
+++ b/.clj-kondo/hooks/metabase/models/setting.clj
@@ -3,6 +3,205 @@
[clj-kondo.hooks-api :as hooks]
[hooks.common :as common]))
+(def ^:private ignored-implicit-export?
+ '#{active-users-count
+ admin-email
+ analytics-uuid
+ anon-tracking-enabled
+ api-key
+ attachment-table-row-limit
+ audit-max-retention-days
+ bcc-enabled?
+ breakout-bin-width
+ check-for-updates
+ cloud-gateway-ips
+ config-from-file-sync-databases
+ custom-homepage
+ custom-homepage-dashboard
+ database-enable-actions
+ deprecation-notice-version
+ dismissed-custom-dashboard-toast
+ email-configured?
+ email-from-address
+ email-from-name
+ email-reply-to
+ email-smtp-host
+ email-smtp-password
+ email-smtp-port
+ email-smtp-security
+ email-smtp-username
+ embedding-app-origin
+ embedding-secret-key
+ enable-password-login
+ enable-public-sharing
+ enable-query-caching
+ engines
+ enum-cardinality-threshold
+ follow-up-email-sent
+ ga-code
+ ga-enabled
+ google-auth-auto-create-accounts-domain
+ google-auth-client-id
+ google-auth-configured
+ google-auth-enabled
+ has-sample-database?
+ has-user-setup
+ help-link
+ help-link-custom-destination
+ instance-creation
+ is-hosted?
+ is-metabot-enabled
+ jdbc-data-warehouse-max-connection-pool-size
+ jdbc-data-warehouse-unreturned-connection-timeout-seconds
+ jwt-attribute-email
+ jwt-attribute-firstname
+ jwt-attribute-groups
+ jwt-attribute-lastname
+ jwt-configured
+ jwt-enabled
+ jwt-group-mappings
+ jwt-group-sync
+ jwt-identity-provider-uri
+ jwt-shared-secret
+ last-acknowledged-version
+ ldap-attribute-email
+ ldap-attribute-firstname
+ ldap-attribute-lastname
+ ldap-bind-dn
+ ldap-configured?
+ ldap-enabled
+ ldap-group-base
+ ldap-group-mappings
+ ldap-group-membership-filter
+ ldap-group-sync
+ ldap-host
+ ldap-password
+ ldap-port
+ ldap-security
+ ldap-sync-user-attributes
+ ldap-sync-user-attributes-blacklist
+ ldap-user-base
+ ldap-user-filter
+ load-analytics-content
+ map-tile-server-url
+ metabot-default-embedding-model
+ metabot-feedback-url
+ metabot-get-prompt-templates-url
+ metabot-prompt-generator-token-limit
+ multi-setting-read-only
+ notification-link-base-url
+ num-metabot-choices
+ openai-api-key
+ openai-available-models
+ openai-model
+ openai-organization
+ other-sso-enabled?
+ password-complexity
+ persist-models-enabled
+ persisted-model-refresh-cron-schedule
+ premium-embedding-token
+ prometheus-server-port
+ query-caching-max-kb
+ query-caching-max-ttl
+ query-caching-min-ttl
+ query-caching-ttl-ratio
+ redirect-all-requests-to-https
+ reset-token-ttl-hours
+ retired-setting
+ retry-initial-interval
+ retry-max-attempts
+ retry-max-interval-millis
+ retry-multiplier
+ retry-randomization-factor
+ saml-application-name
+ saml-attribute-email
+ saml-attribute-firstname
+ saml-attribute-group
+ saml-attribute-lastname
+ saml-configured
+ saml-enabled
+ saml-group-mappings
+ saml-group-sync
+ saml-identity-provider-certificate
+ saml-identity-provider-issuer
+ saml-identity-provider-uri
+ saml-keystore-alias
+ saml-keystore-password
+ saml-keystore-path
+ send-email-on-first-login-from-new-device
+ send-new-sso-user-admin-email?
+ session-cookie-samesite
+ session-cookies
+ session-timeout
+ setup-token
+ show-database-syncing-modal
+ show-metabase-links
+ site-url
+ site-uuid
+ site-uuid-for-premium-features-token-checks
+ site-uuid-for-unsubscribing-url
+ site-uuid-for-version-info-fetching
+ slack-app-token
+ slack-cached-channels-and-usernames
+ slack-channels-and-usernames-last-updated
+ slack-files-channel
+ slack-token
+ slack-token-valid?
+ snowplow-available
+ snowplow-enabled
+ snowplow-url
+ sql-jdbc-fetch-size
+ ssh-heartbeat-interval-sec
+ ssl-certificate-public-key
+ startup-time-millis
+ token-features
+ token-status
+ toucan-name
+ uncached-setting
+ uploads-table-prefix
+ user-visibility
+ version
+ version-info
+ version-info-last-checked})
+
+(defn- defsetting-lint [node setting-name docstring options-list]
+ (let [anon-binding (common/with-macro-meta (hooks/token-node '_) node)
+ ;; (defn my-setting [] ...)
+ getter-node (-> (list
+ (hooks/token-node 'defn)
+ setting-name
+ (hooks/string-node "Docstring.")
+ (hooks/vector-node []))
+ hooks/list-node
+ (with-meta (meta node)))
+ ;; (defn my-setting! [_x] ...)
+ setter-node (-> (list
+ (hooks/token-node 'defn)
+ (with-meta
+ (hooks/token-node (symbol (str (hooks/sexpr setting-name) \!)))
+ (meta setting-name))
+ (hooks/string-node "Docstring.")
+ (hooks/vector-node [(hooks/token-node '_value-or-nil)]))
+ hooks/list-node
+ (with-meta (update (meta node) :clj-kondo/ignore #(hooks/vector-node (cons :clojure-lsp/unused-public-var (:children %))))))]
+
+ (when (nil? (second (drop-while (comp not #{[:k :export?]} first) options-list)))
+ (when-not (contains? ignored-implicit-export? (:value setting-name))
+ (hooks/reg-finding! (assoc (meta node)
+ :message "Setting definition must provide an explicit value for :export? indicating whether the setting should be exported or not with serialization."
+ :type :metabase/defsetting-must-specify-export))))
+
+ {:node (-> (list
+ (hooks/token-node 'let)
+ ;; include description and the options map so they can get validated as well.
+ (hooks/vector-node
+ [anon-binding docstring
+ anon-binding (hooks/map-node options-list)])
+ getter-node
+ setter-node)
+ hooks/list-node
+ (with-meta (meta node)))}))
+
(defn defsetting
"Rewrite a [[metabase.models.defsetting]] form like
@@ -11,54 +210,45 @@
as
(let [_ \"Description\"
- _ :type
- _ :boolean]
+ _ {:type :boolean}]
(defn my-setting \"Docstring.\" [])
(defn my-setting! \"Docstring.\" [_value-or-nil]))
for linting purposes."
[{:keys [node]}]
- (let [[setting-name & args] (rest (:children node))
- ;; (defn my-setting [] ...)
- getter-node (-> (list
- (hooks/token-node 'defn)
- setting-name
- (hooks/string-node "Docstring.")
- (hooks/vector-node []))
- hooks/list-node
- (with-meta (meta node)))
- ;; (defn my-setting! [_x] ...)
- setter-node (-> (list
- (hooks/token-node 'defn)
- (with-meta
- (hooks/token-node (symbol (str (hooks/sexpr setting-name) \!)))
- (meta setting-name))
- (hooks/string-node "Docstring.")
- (hooks/vector-node [(hooks/token-node '_value-or-nil)]))
- hooks/list-node
- (with-meta (update (meta node) :clj-kondo/ignore #(hooks/vector-node (cons :clojure-lsp/unused-public-var (:children %))))))]
- {:node (-> (list
- (hooks/token-node 'let)
- ;; include description and the options map so they can get validated as well.
- (hooks/vector-node (vec (interleave (repeat (common/with-macro-meta (hooks/token-node '_) node))
- args)))
- getter-node
- setter-node)
- hooks/list-node
- (with-meta (meta node)))}))
+ (let [[setting-name docstring & options] (rest (:children node))]
+ (defsetting-lint node setting-name docstring options)))
+
+(defn define-multi-setting
+ "Rewrite a [[metabase.models.define-multi-setting]] form like
+
+ (defsetting my-setting \"Description\" :key :type :boolean)
+
+ as
+
+ (let [_ \"Description\"
+ _ {:type :boolean, :multi-thunk :key}]
+ (defn my-setting \"Docstring.\" [])
+ (defn my-setting! \"Docstring.\" [_value-or-nil]))
+
+ for linting purposes."
+ [{:keys [node]}]
+ (let [[setting-name docstring thunk & options] (rest (:children node))]
+ (defsetting-lint node setting-name docstring (concat options [(hooks/token-node :multi-thunk) thunk]))))
(comment
(defn- defsetting* [form]
(hooks/sexpr
- (:node
- (defsetting
- {:node
- (hooks/parse-string
- (with-out-str
- (clojure.pprint/pprint
- form)))}))))
-
- (defn- x []
+ (:node
+ (defsetting
+ {:node
+ (hooks/parse-string
+ (with-out-str
+ #_{:clj-kondo/ignore [:unresolved-namespace]}
+ (clojure.pprint/pprint
+ form)))}))))
+
+ (defn x []
(defsetting*
'(defsetting active-users-count
(deferred-tru "Cached number of active users. Refresh every 5 minutes.")
diff --git a/src/metabase/driver/util.clj b/src/metabase/driver/util.clj
index d42e55775fa..aec5c429438 100644
--- a/src/metabase/driver/util.clj
+++ b/src/metabase/driver/util.clj
@@ -118,11 +118,12 @@
"Consider [[metabase.driver/can-connect?]] / [[can-connect-with-details?]] to have failed if they were not able to
successfully connect after this many milliseconds. By default, this is 10 seconds."
:visibility :internal
+ :export? false
:type :integer
;; for TESTS use a timeout time of 3 seconds. This is because we have some tests that check whether
;; [[driver/can-connect?]] is failing when it should, and we don't want them waiting 10 seconds to fail.
;;
- ;; Don't set the timeout too low -- I've have Circle fail when the timeout was 1000ms on *one* occasion.
+ ;; Don't set the timeout too low -- I've had Circle fail when the timeout was 1000ms on *one* occasion.
:default (if config/is-test?
3000
10000))
diff --git a/src/metabase/models/setting.clj b/src/metabase/models/setting.clj
index 4abe4efc02a..cc8f8ebed4a 100644
--- a/src/metabase/models/setting.clj
+++ b/src/metabase/models/setting.clj
@@ -255,6 +255,7 @@
:tag (s/maybe Symbol) ; type annotation, e.g. ^String, to be applied. Defaults to tag based on :type
:sensitive? s/Bool ; is this sensitive (never show in plaintext), like a password? (default: false)
:visibility Visibility ; where this setting should be visible (default: :admin)
+ :export? s/Bool ; should this setting be serialized?
:cache? s/Bool ; should the getter always fetch this value "fresh" from the DB? (default: false)
:deprecated (s/maybe s/Str) ; if non-nil, contains the Metabase version in which this setting was deprecated
@@ -280,11 +281,7 @@
;; should be used for most non-sensitive settings, and will log the value returned by its getter, which may be a
;; the default getter or a custom one.
;; (default: `:no-value`)
- :audit (s/maybe (s/enum :never :no-value :raw-value :getter))
-
- ;; TODO: make this required and deprecate setting/exported-setting
- (s/optional-key :export?) s/Bool ; should this setting be serialized?
- })
+ :audit (s/maybe (s/enum :never :no-value :raw-value :getter))})
(defonce ^{:doc "Map of loaded defsettings"}
registered-settings
@@ -890,6 +887,7 @@
:setter (partial (default-setter-for-type setting-type) setting-name)
:tag (default-tag-for-type setting-type)
:visibility :admin
+ :export? false
:sensitive? false
:cache? true
:feature nil
@@ -1056,6 +1054,10 @@
'Settings Managers' are non-admin users with the 'settings' permission, which gives them access to the Settings page
in the Admin Panel.
+ ###### `:export?`
+
+ Whether this Setting is included when producing a serializing settings export.
+
###### `:getter`
A custom getter fn, which takes no arguments. Overrides the default implementation. (This can in turn call functions
--
GitLab