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

@@ -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

.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.")

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))

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