Fix sync not being triggered during setup (#12942)

* Fix :user-login activity not being recorded during setup (#12933)

* Test fixes 

* Fix Database sync not happening at setup (#12826)

* Test cleanup. Wow!

* More test cleanup

* Test/lint fix 

* test fix 

* Test fix

* Remove unneeded wait

* Some more tests & don't instrument log forms

* Exclude defmulti/defonce forms from instrumentation; more tests

project.clj

@@ -371,7 +371,17 @@
      :plugins      [[lein-cloverage  "1.1.3-SNAPSHOT"]]
      :source-paths ^:replace ["src" "backend/mbql/src"]
      :test-paths   ^:replace ["test" "backend/mbql/test"]
-     :cloverage    {:fail-threshold 69}}]
+     :cloverage    {:fail-threshold 69
+                    :exclude-call
+                    [;; don't instrument logging forms, since they won't get executed as part of tests anyway
+                     ;; log calls expand to these
+                     clojure.tools.logging/logf
+                     clojure.tools.logging/logp
+                     ;; defonce and defmulti forms get instrumented incorrectly and are false negatives
+                     ;; -- see https://github.com/cloverage/cloverage/issues/294. Once this issue is
+                     ;; fixed we can remove this exception.
+                     defonce
+                     defmulti]}}]
 
    ;; build the uberjar with `lein uberjar`
    :uberjar

src/metabase/api/common.clj

@@ -393,7 +393,7 @@
     [400 (tru "Embedding is not enabled.")]))
 
 (defn check-not-archived
-  "Check that the OBJECT exists and is not `:archived`, or throw a `404`. Returns OBJECT as-is if check passes."
+  "Check that the `object` exists and is not `:archived`, or throw a `404`. Returns `object` as-is if check passes."
   [object]
   (u/prog1 object
     (check-404 object)

src/metabase/api/setup.clj

@@ -54,18 +54,19 @@
     ;; return user ID and session ID
     {:session-id session-id, :user-id user-id}))
 
-(defn- setup-create-database! [{:keys [name driver details schedules database]}]
+(defn- setup-create-database!
+  "Create a new Database. Returns newly created Database."
+  [{:keys [name driver details schedules database]}]
   (when driver
-    (when-not (driver/available? (driver/the-driver driver))
-      (throw (ex-info (tru "Cannot create Database: cannot find driver {0}." driver)
-                      {:driver driver})))
-    (let [db (db/insert! Database
-               (merge
-                {:name name, :engine driver, :details details}
-                (u/select-non-nil-keys database #{:is_on_demand :is_full_sync :auto_run_queries})
-                (when schedules
-                  (database-api/schedule-map->cron-strings schedules))))]
-      (events/publish-event! :database-create db))))
+    (when-not (some-> (u/ignore-exceptions (driver/the-driver driver)) driver/available?)
+      (let [msg (tru "Cannot create Database: cannot find driver {0}." driver)]
+        (throw (ex-info msg {:errors {:database {:engine msg}}, :status-code 400}))))
+    (db/insert! Database
+      (merge
+       {:name name, :engine driver, :details details}
+       (u/select-non-nil-keys database #{:is_on_demand :is_full_sync :auto_run_queries})
+       (when schedules
+         (database-api/schedule-map->cron-strings schedules))))))
 
 (defn- setup-set-settings! [request {:keys [email site-name site-locale allow-tracking?]}]
   ;; set a couple preferences
@@ -101,27 +102,30 @@
    allow_tracking   (s/maybe (s/cond-pre s/Bool su/BooleanString))
    schedules        (s/maybe database-api/ExpandedSchedulesMap)
    auto_run_queries (s/maybe s/Bool)}
-  (try
-    (db/transaction
-      (let [{:keys [session-id user-id]} (setup-create-user!
-                                          {:email email, :first-name first_name, :last-name last_name, :password password})]
-        (setup-create-database! {:name name, :driver engine, :details details, :schedules schedules, :database database})
-        (setup-set-settings!
-         request
-         {:email email, :site-name site_name, :site-locale site_locale, :allow-tracking? allow_tracking})
-        ;; clear the setup token now, it's no longer needed
-        (setup/clear-token!)
-        ;; notify that we've got a new user in the system AND that this user logged in
-        (events/publish-event! :user-create {:user_id user-id})
-        (events/publish-event! :user-login {:user_id user-id, :session_id session-id, :first_login true})
-        ;; return response with session ID and set the cookie as well
-        (mw.session/set-session-cookie request {:id session-id} (UUID/fromString session-id))))
-    (catch Throwable e
-      ;; if the transaction fails, restore the Settings cache from the DB again so any changes made in this endpoint
-      ;; (such as clearing the setup token) are reverted. We can't use `dosync` here to accomplish this because
-      ;; there is `io!` in this block
-      (setting.cache/restore-cache!)
-      (throw e))))
+  (letfn [(create! []
+            (try
+              (db/transaction
+                (let [user-info (setup-create-user!
+                                 {:email email, :first-name first_name, :last-name last_name, :password password})
+                      db        (setup-create-database!
+                                 {:name name, :driver engine, :details details, :schedules schedules, :database database})]
+                  (setup-set-settings!
+                   request
+                   {:email email, :site-name site_name, :site-locale site_locale, :allow-tracking? allow_tracking})
+                  ;; clear the setup token now, it's no longer needed
+                  (setup/clear-token!)
+                  (assoc user-info :database db)))
+              (catch Throwable e
+                ;; if the transaction fails, restore the Settings cache from the DB again so any changes made in this
+                ;; endpoint (such as clearing the setup token) are reverted. We can't use `dosync` here to accomplish
+                ;; this because there is `io!` in this block
+                (setting.cache/restore-cache!)
+                (throw e))))]
+    (let [{:keys [user-id session-id database]} (create!)]
+      (events/publish-event! :database-create database)
+      (events/publish-event! :user-login {:user_id user-id, :session_id session-id, :first_login true})
+      ;; return response with session ID and set the cookie as well
+      (mw.session/set-session-cookie request {:id session-id} (UUID/fromString session-id)))))
 
 (api/defendpoint POST "/validate"
   "Validate that we can connect to a database given a set of details."

src/metabase/events.clj

@@ -11,30 +11,36 @@
   (:require [clojure.core.async :as async]
             [clojure.string :as str]
             [clojure.tools.logging :as log]
-            [metabase
-             [config :as config]
-             [util :as u]]
             [metabase.plugins.classloader :as classloader]
+            [metabase.util :as u]
             [metabase.util.i18n :refer [trs]]))
 
 ;;; --------------------------------------------------- LIFECYCLE ----------------------------------------------------
 
+(defmulti init!
+  "Initialize event handlers. All implementations of this method are called once when the event system is started. Add a
+  new implementation of this method to define new event initialization logic. All `metabase.events.*` namespaces are
+  loaded automatically during event initialization before invoking implementations of `init!`.
+
+  `unique-key` is not used internally but must be unique."
+  {:arglists '([unique-key])}
+  keyword)
 
 (defonce ^:private events-initialized?
   (atom nil))
 
 (defn- find-and-load-event-handlers!
-  "Search Classpath for namespaces that start with `metabase.events.`, and call their `events-init` function if it
-  exists."
+  "Look for namespaces that start with `metabase.events.`, and call their `events-init` function if it exists."
   []
-  (when-not config/is-test?
-    (doseq [ns-symb u/metabase-namespace-symbols
-            :when   (.startsWith (name ns-symb) "metabase.events.")]
-      (classloader/require ns-symb)
-      ;; look for `events-init` function in the namespace and call it if it exists
-      (when-let [init-fn (ns-resolve ns-symb 'events-init)]
-        (log/info (trs "Starting events listener:") (u/format-color 'blue ns-symb) (u/emoji "👂"))
-        (init-fn)))))
+  (doseq [ns-symb u/metabase-namespace-symbols
+          :when   (.startsWith (name ns-symb) "metabase.events.")]
+    (classloader/require ns-symb))
+  (doseq [[k f] (methods init!)]
+    (log/info (trs "Starting events listener:") (u/format-color 'blue k) (u/emoji "👂"))
+    (try
+      (f k)
+      (catch Throwable e
+        (log/error e (trs "Error starting events listener"))))))
 
 (defn initialize-events!
   "Initialize the asynchronous internal events system."
@@ -47,12 +53,11 @@
 ;;; -------------------------------------------------- PUBLICATION ---------------------------------------------------
 
 
-(def ^:private events-channel
-  "Channel to host events publications."
+(defonce ^:private ^{:doc "Channel to host events publications."} events-channel
   (async/chan))
 
-(def ^:private events-publication
-  "Publication for general events channel. Expects a map as input and the map must have a `:topic` key."
+(defonce ^:private ^{:doc "Publication for general events channel. Expects a map as input and the map must have a
+  `:topic` key."} events-publication
   (async/pub events-channel :topic))
 
 (defn publish-event!
@@ -60,7 +65,9 @@
   {:style/indent 1}
   [topic event-item]
   {:pre [(keyword topic)]}
-  (async/go (async/>! events-channel {:topic (keyword topic), :item event-item}))
+  (let [event {:topic (keyword topic), :item event-item}]
+    (log/tracef "Publish event %s" (pr-str event))
+    (async/put! events-channel event))
   event-item)
 
 
@@ -74,7 +81,7 @@
   (async/sub events-publication (keyword topic) channel)
   channel)
 
-(defn- subscribe-to-topics!
+(defn subscribe-to-topics!
   "Convenience method for subscribing to a series of topics against a single channel."
   [topics channel]
   {:pre [(coll? topics)]}
@@ -90,30 +97,30 @@
   ;; start listening for events we care about and do something with them
   (async/go-loop []
     ;; try/catch here to get possible exceptions thrown by core.async trying to read from the channel
-    (try
-      (handler-fn (async/<! channel))
-      (catch Throwable e
-        (log/error e (trs "Unexpected error listening on events"))))
-    (recur)))
+    (when-let [val (async/<! channel)]
+      (try
+        (handler-fn val)
+        (catch Throwable e
+          (log/error e (trs "Unexpected error listening on events"))))
+      (recur))))
 
 
 ;;; ------------------------------------------------ HELPER FUNCTIONS ------------------------------------------------
 
 (defn topic->model
-  "Determine a valid `model` identifier for the given TOPIC."
+  "Determine a valid `model` identifier for the given `topic`."
   [topic]
   ;; just take the first part of the topic name after splitting on dashes.
   (first (str/split (name topic) #"-")))
 
 (defn object->model-id
-  "Determine the appropriate `model_id` (if possible) for a given OBJECT."
+  "Determine the appropriate `model_id` (if possible) for a given `object`."
   [topic object]
   (if (contains? (set (keys object)) :id)
     (:id object)
     (let [model (topic->model topic)]
       (get object (keyword (format "%s_id" model))))))
 
-(defn object->user-id
-  "Determine the appropriate `user_id` (if possible) for a given OBJECT."
-  [object]
-  (or (:actor_id object) (:user_id object) (:creator_id object)))
+(def ^{:arglists '([object])} object->user-id
+  "Determine the appropriate `user_id` (if possible) for a given `object`."
+  (some-fn :actor_id :user_id :creator_id))

src/metabase/events/activity_feed.clj

@@ -36,11 +36,10 @@
     :segment-delete
     :user-login}) ; this is only used these days the first time someone logs in to record 'user-joined' events
 
-(def ^:private activity-feed-channel
-  "Channel for receiving event notifications we want to subscribe to for the activity feed."
+(defonce ^:private ^{:doc "Channel for receiving event notifications we want to subscribe to for the activity feed."}
+  activity-feed-channel
   (async/chan))
 
-
 ;;; ------------------------------------------------ EVENT PROCESSING ------------------------------------------------
 
 (defn- process-card-activity! [topic {query :dataset_query, :as object}]
@@ -157,7 +156,6 @@
 
 ;;; ---------------------------------------------------- LIFECYLE ----------------------------------------------------
 
-(defn events-init
-  "Automatically called during startup; start the events listener for the activity feed."
-  []
+(defmethod events/init! ::ActivityFeed
+  [_]
   (events/start-event-listener! activity-feed-topics activity-feed-channel process-activity-event!))

src/metabase/events/dependencies.clj

@@ -14,8 +14,8 @@
     :metric-create
     :metric-update})
 
-(def ^:private dependencies-channel
-  "Channel for receiving event notifications we want to subscribe to for dependencies events."
+(defonce ^:private ^{:doc "Channel for receiving event notifications we want to subscribe to for dependencies events."}
+  dependencies-channel
   (async/chan))
 
 
@@ -46,8 +46,6 @@
 
 ;;; ## ---------------------------------------- LIFECYLE ----------------------------------------
 
-
-(defn events-init
-  "Automatically called during startup; start the events listener for dependencies topics."
-  []
+(defmethod events/init! ::Dependencies
+  [_]
   (events/start-event-listener! dependencies-topics dependencies-channel process-dependencies-event))

src/metabase/events/driver_notifications.clj

@@ -10,12 +10,12 @@
              [driver :as driver]
              [events :as events]]))
 
-(def ^:const ^:private driver-notifications-topics
+(def ^:private driver-notifications-topics
   "The `Set` of event topics which are subscribed to for use in driver notifications."
   #{:database-update :database-delete})
 
-(def ^:private driver-notifications-channel
-  "Channel for receiving event notifications we want to subscribe to for driver notifications events."
+(defonce ^:private ^{:doc "Channel for receiving event notifications we want to subscribe to for driver notifications
+  events."} driver-notifications-channel
   (async/chan))
 
 
@@ -37,8 +37,6 @@
 
 ;;; ---------------------------------------------------- LIFECYLE ----------------------------------------------------
 
-
-(defn events-init
-  "Automatically called during startup; start event listener for database sync events."
-  []
+(defmethod events/init! ::DriverNotifications
+  [_]
   (events/start-event-listener! driver-notifications-topics driver-notifications-channel process-driver-notifications-event))

src/metabase/events/last_login.clj

@@ -9,8 +9,8 @@
   "The `Set` of event topics which are subscribed to for use in last login tracking."
   #{:user-login})
 
-(def ^:private last-login-channel
-  "Channel for receiving event notifications we want to subscribe to for last login events."
+(defonce ^:private ^{:doc "Channel for receiving event notifications we want to subscribe to for last login events."}
+  last-login-channel
   (async/chan))
 
 
@@ -33,8 +33,6 @@
 
 ;;; ## ---------------------------------------- LIFECYLE ----------------------------------------
 
-
-(defn events-init
-  "Automatically called during startup; start the events listener for last login events."
-  []
+(defmethod events/init! ::LastLogin
+  [_]
   (events/start-event-listener! last-login-topics last-login-channel process-last-login-event))

src/metabase/events/metabot_lifecycle.clj

@@ -9,8 +9,9 @@
   "The `Set` of event topics which are subscribed to for use in metabot lifecycle."
   #{:settings-update})
 
-(def ^:private metabot-lifecycle-channel
-  "Channel for receiving event notifications we want to subscribe to for metabot lifecycle events."
+(defonce ^:private ^{:doc "Channel for receiving event notifications we want to subscribe to for MetaBot lifecycle
+  events."}
+  metabot-lifecycle-channel
   (async/chan))
 
 
@@ -37,8 +38,6 @@
 
 ;;; ## ---------------------------------------- LIFECYLE ----------------------------------------
 
-
-(defn events-init
-  "Automatically called during startup; start event listener for metabot lifecycle events."
-  []
+(defmethod events/init! ::MetaBotLifecycle
+  [_]
   (events/start-event-listener! metabot-lifecycle-topics metabot-lifecycle-channel process-metabot-lifecycle-event))

src/metabase/events/notifications.clj

@@ -21,8 +21,8 @@
   #{:metric-update
     :segment-update})
 
-(def ^:private notifications-channel
-  "Channel for receiving event notifications we want to subscribe to for notifications events."
+(defonce ^:private ^{:doc "Channel for receiving event notifications we want to subscribe to for notifications events."}
+  notifications-channel
   (async/chan))
 
 
@@ -95,7 +95,6 @@
 
 ;;; --------------------------------------------------- Lifecycle ----------------------------------------------------
 
-(defn events-init
-  "Automatically called during startup; start event listener for notifications events."
-  []
+(defmethod events/init! ::Notifications
+  [_]
   (events/start-event-listener! notifications-topics notifications-channel process-notifications-event!))

src/metabase/events/revision.clj

@@ -25,8 +25,8 @@
     :segment-update
     :segment-delete})
 
-(def ^:private revisions-channel
-  "Channel for receiving event notifications we want to subscribe to for revision events."
+(defonce ^:private ^{:doc "Channel for receiving event notifications we want to subscribe to for revision events."}
+  revisions-channel
   (async/chan))
 
 
@@ -76,8 +76,6 @@
 
 ;;; ## ---------------------------------------- LIFECYLE ----------------------------------------
 
-
-(defn events-init
-  "Automatically called during startup; start event listener for revision events."
-  []
+(defmethod events/init! ::Revisions
+  [_]
   (events/start-event-listener! revisions-topics revisions-channel process-revision-event!))

src/metabase/events/sync_database.clj

@@ -12,10 +12,11 @@
 (def ^:const sync-database-topics
   "The `Set` of event topics which are subscribed to for use in database syncing."
   #{:database-create
+    ;; published by POST /api/database/:id/sync -- a message to start syncing the DB right away
     :database-trigger-sync})
 
-(def ^:private sync-database-channel
-  "Channel for receiving event notifications we want to subscribe to for database sync events."
+(defonce ^:private ^{:doc "Channel for receiving event notifications we want to subscribe to for database sync events."}
+  sync-database-channel
   (async/chan))
 
 
@@ -23,27 +24,27 @@
 
 
 (defn process-sync-database-event
-  "Handle processing for a single event notification received on the sync-database-channel"
-  [sync-database-event]
+  "Handle processing for a single event notification received on the `sync-database-channel`"
+  [{topic :topic, object :item, :as event}]
   ;; try/catch here to prevent individual topic processing exceptions from bubbling up.  better to handle them here.
   (try
-    (when-let [{topic :topic object :item} sync-database-event]
+    (when event
       (when-let [database (Database (events/object->model-id topic object))]
         ;; just kick off a sync on another thread
-        (future (try
-                  ;; only do the 'full' sync if this is a "full sync" database. Otherwise just do metadata sync only
-                  (if (:is_full_sync database)
-                    (sync/sync-database! database)
-                    (sync-metadata/sync-db-metadata! database))
-                  (catch Throwable e
-                    (log/error e (trs "Error syncing Database {0}" (u/get-id database))))))))
+        (future
+          (try
+            ;; only do the 'full' sync if this is a "full sync" database. Otherwise just do metadata sync only
+            (if (:is_full_sync database)
+              (sync/sync-database! database)
+              (sync-metadata/sync-db-metadata! database))
+            (catch Throwable e
+              (log/error e (trs "Error syncing Database {0}" (u/get-id database))))))))
     (catch Throwable e
-      (log/warn e (trs "Failed to process sync-database event.") (:topic sync-database-event)))))
+      (log/warn e (trs "Failed to process sync-database event.") topic))))
 
 
 ;;; ---------------------------------------------------- LIFECYLE ----------------------------------------------------
 
-(defn events-init
-  "Automatically called during startup; start event listener for database sync events."
-  []
+(defmethod events/init! ::Sync
+  [_]
   (events/start-event-listener! sync-database-topics sync-database-channel process-sync-database-event))

src/metabase/events/view_log.clj

@@ -5,21 +5,20 @@
             [metabase.models.view-log :refer [ViewLog]]
             [toucan.db :as db]))
 
-(def ^:private ^:const view-counts-topics
+(def ^:private ^:const view-log-topics
   "The `Set` of event topics which we subscribe to for view counting."
   #{:card-create
     :card-read
     :dashboard-read})
 
-(def ^:private view-counts-channel
-  "Channel for receiving event notifications we want to subscribe to for view counting."
+(defonce ^:private ^{:doc "Channel for receiving event notifications we want to subscribe to for view counting."}
+  view-log-channel
   (async/chan))
 
 
 ;;; ## ---------------------------------------- EVENT PROCESSING ----------------------------------------
 
-
-(defn- record-view
+(defn- record-view!
   "Simple base function for recording a view of a given `model` and `model-id` by a certain `user`."
   [model model-id user-id]
   ;; TODO - we probably want a little code that prunes old entries so that this doesn't get too big
@@ -28,13 +27,13 @@
     :model    model
     :model_id model-id))
 
-(defn process-view-count-event
-  "Handle processing for a single event notification received on the view-counts-channel"
+(defn handle-view-event!
+  "Handle processing for a single event notification received on the view-log-channel"
   [event]
   ;; try/catch here to prevent individual topic processing exceptions from bubbling up.  better to handle them here.
   (try
     (when-let [{topic :topic object :item} event]
-      (record-view
+      (record-view!
         (events/topic->model topic)
         (events/object->model-id topic object)
         (events/object->user-id object)))
@@ -44,8 +43,6 @@
 
 ;;; ## ---------------------------------------- LIFECYLE ----------------------------------------
 
-
-(defn events-init
-  "Automatically called during startup; start the events listener for view events."
-  []
-  (events/start-event-listener! view-counts-topics view-counts-channel process-view-count-event))
+(defmethod events/init! ::ViewLog
+  [_]
+  (events/start-event-listener! view-log-topics view-log-channel handle-view-event!))

src/metabase/models/database.clj

@@ -38,7 +38,6 @@
   "Unschedule any currently pending sync operation tasks for `database`."
   [database]
   (try
-    (classloader/the-classloader)
     (classloader/require 'metabase.task.sync-databases)
     ((resolve 'metabase.task.sync-databases/unschedule-tasks-for-db!) database)
     (catch Throwable e
@@ -96,7 +95,6 @@
 (defn- perms-objects-set [database _]
   #{(perms/object-path (u/get-id database))})
 
-
 (u/strict-extend (class Database)
   models/IModel
   (merge models/IModelDefaults

src/metabase/models/table.clj

@@ -29,7 +29,8 @@
   `:database`     - use the same order as in the table definition in the DB;
   `:alphabetical` - order alphabetically by name;
   `:custom`       - the user manually set the order in the data model
-  `:smart`        - Try to be smart and order like you'd usually want it: first PK, followed by `:type/Name`s, then `:type/Temporal`s, and from there on in alphabetical order."
+  `:smart`        - Try to be smart and order like you'd usually want it: first PK, followed by `:type/Name`s, then
+                    `:type/Temporal`s, and from there on in alphabetical order."
   #{:database :alphabetical :custom :smart})
 
 

src/metabase/task.clj

@@ -216,12 +216,12 @@
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
 (defn- job-detail->info [^JobDetail job-detail]
-  {:key                                (-> (.getKey job-detail) .getName)
-   :class                              (-> (.getJobClass job-detail) .getCanonicalName)
-   :description                        (.getDescription job-detail)
-   :concurrent-executation-disallowed? (.isConcurrentExectionDisallowed job-detail)
-   :durable?                           (.isDurable job-detail)
-   :requests-recovery?                 (.requestsRecovery job-detail)})
+  {:key                              (-> (.getKey job-detail) .getName)
+   :class                            (-> (.getJobClass job-detail) .getCanonicalName)
+   :description                      (.getDescription job-detail)
+   :concurrent-execution-disallowed? (.isConcurrentExectionDisallowed job-detail)
+   :durable?                         (.isDurable job-detail)
+   :requests-recovery?               (.requestsRecovery job-detail)})
 
 (defmulti ^:private trigger->info
   {:arglists '([trigger])}
@@ -238,32 +238,53 @@
    :previous-fire-time (.getPreviousFireTime trigger)
    :priority           (.getPriority trigger)
    :start-time         (.getStartTime trigger)
-   :may-fire-again?    (.mayFireAgain trigger)})
+   :may-fire-again?    (.mayFireAgain trigger)
+   :data               (.getJobDataMap trigger)})
 
 (defmethod trigger->info CronTrigger
   [^CronTrigger trigger]
-  (merge
+  (assoc
    ((get-method trigger->info Trigger) trigger)
-   {:misfire-instruction
-    ;; not 100% sure why `case` doesn't work here...
-    (condp = (.getMisfireInstruction trigger)
-      CronTrigger/MISFIRE_INSTRUCTION_IGNORE_MISFIRE_POLICY "IGNORE_MISFIRE_POLICY"
-      CronTrigger/MISFIRE_INSTRUCTION_SMART_POLICY          "SMART_POLICY"
-      CronTrigger/MISFIRE_INSTRUCTION_FIRE_ONCE_NOW         "FIRE_ONCE_NOW"
-      CronTrigger/MISFIRE_INSTRUCTION_DO_NOTHING            "DO_NOTHING"
-      (format "UNKNOWN: %d" (.getMisfireInstruction trigger)))}))
+   :schedule
+   (.getCronExpression trigger)
+
+   :misfire-instruction
+   ;; not 100% sure why `case` doesn't work here...
+   (condp = (.getMisfireInstruction trigger)
+     CronTrigger/MISFIRE_INSTRUCTION_IGNORE_MISFIRE_POLICY "IGNORE_MISFIRE_POLICY"
+     CronTrigger/MISFIRE_INSTRUCTION_SMART_POLICY          "SMART_POLICY"
+     CronTrigger/MISFIRE_INSTRUCTION_FIRE_ONCE_NOW         "FIRE_ONCE_NOW"
+     CronTrigger/MISFIRE_INSTRUCTION_DO_NOTHING            "DO_NOTHING"
+     (format "UNKNOWN: %d" (.getMisfireInstruction trigger)))))
+
+(defn- ->job-key ^JobKey [x]
+  (cond
+    (instance? JobKey x) x
+    (string? x)          (JobKey. ^String x)))
+
+(defn job-info
+  "Get info about a specific Job (`job-key` can be either a String or `JobKey`).
+
+    (task/job-info \"metabase.task.sync-and-analyze.job\")"
+  [job-key]
+  (let [job-key (->job-key job-key)]
+    (try
+      (assoc (job-detail->info (qs/get-job (scheduler) job-key))
+             :triggers (for [trigger (sort-by #(-> ^Trigger % .getKey .getName)
+                                              (qs/get-triggers-of-job (scheduler) job-key))]
+                         (trigger->info trigger)))
+      (catch Throwable e
+        (log/warn e (trs "Error fetching details for Job: {0}" (.getName job-key)))))))
+
+(defn- jobs-info []
+  (->> (some-> (scheduler) (.getJobKeys nil))
+       (sort-by #(.getName ^JobKey %))
+       (map job-info)
+       (filter some?)))
 
 (defn scheduler-info
   "Return raw data about all the scheduler and scheduled tasks (i.e. Jobs and Triggers). Primarily for debugging
   purposes."
   []
-  {:scheduler
-   (str/split-lines (.getSummary (.getMetaData (scheduler))))
-
-   :jobs
-   (for [^JobKey job-key (->> (.getJobKeys (scheduler) nil)
-                              (sort-by #(.getName ^JobKey %) ))]
-     (assoc (job-detail->info (qs/get-job (scheduler) job-key))
-       :triggers (for [trigger (->> (qs/get-triggers-of-job (scheduler) job-key)
-                                    (sort-by #(-> ^Trigger % .getKey .getName)))]
-                   (trigger->info trigger))))})
+  {:scheduler (some-> (scheduler) .getMetaData .getSummary str/split-lines)
+   :jobs      (jobs-info)})

src/metabase/task/send_anonymous_stats.clj

@@ -25,7 +25,8 @@
 (def ^:private job-key     "metabase.task.anonymous-stats.job")
 (def ^:private trigger-key "metabase.task.anonymous-stats.trigger")
 
-(defmethod task/init! ::SendAnonymousUsageStats [_]
+(defmethod task/init! ::SendAnonymousUsageStats
+  [_]
   (let [job     (jobs/build
                  (jobs/of-type SendAnonymousUsageStats)
                  (jobs/with-identity (jobs/key job-key)))

src/metabase/task/sync_databases.clj

@@ -16,7 +16,8 @@
              [sync-metadata :as sync-metadata]]
             [metabase.util
              [cron :as cron-util]
-             [i18n :refer [trs]]]
+             [i18n :refer [trs]]
+             [schema :as su]]
             [schema.core :as s]
             [toucan.db :as db])
   (:import metabase.models.database.DatabaseInstance
@@ -26,25 +27,31 @@
 ;;; |                                                   JOB LOGIC                                                    |
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
-(s/defn ^:private job-context->database :- (s/maybe DatabaseInstance)
-  "Get the Database referred to in `job-context`. Returns `nil` if Database no longer exists. (Normally, a Database's
-  sync jobs *should* get deleted when the Database itself is deleted, but better to be safe here just in case.)"
+(s/defn ^:private job-context->database-id :- (s/maybe su/IntGreaterThanZero)
+  "Get the Database ID referred to in `job-context`."
   [job-context]
-  (Database (u/get-id (get (qc/from-job-data job-context) "db-id"))))
+  (u/get-id (get (qc/from-job-data job-context) "db-id")))
 
 ;; The DisallowConcurrentExecution on the two defrecords below attaches an annotation to the generated class that will
 ;; constrain the job execution to only be one at a time. Other triggers wanting the job to run will misfire.
 (jobs/defjob ^{org.quartz.DisallowConcurrentExecution true} SyncAndAnalyzeDatabase [job-context]
-  (when-let [database (job-context->database job-context)]
-    (sync-metadata/sync-db-metadata! database)
-    ;; only run analysis if this is a "full sync" database
-    (when (:is_full_sync database)
-      (analyze/analyze-db! database))))
+  (when-let [database-id (job-context->database-id job-context)]
+    (log/info (trs "Starting sync task for Database {0}." database-id))
+    (when-let [database (or (Database database-id)
+                            (log/warn (trs "Cannot sync Database {0}: Database does not exist." database-id)))]
+      (sync-metadata/sync-db-metadata! database)
+      ;; only run analysis if this is a "full sync" database
+      (when (:is_full_sync database)
+        (analyze/analyze-db! database)))))
 
 (jobs/defjob ^{org.quartz.DisallowConcurrentExecution true} UpdateFieldValues [job-context]
-  (when-let [database (job-context->database job-context)]
-    (when (:is_full_sync database)
-      (field-values/update-field-values! database))))
+  (when-let [database-id (job-context->database-id job-context)]
+    (log/info (trs "Update Field values task triggered for Database {0}." database-id))
+    (when-let [database (or (Database database-id)
+                            (log/warn "Cannot update Field values for Database {0}: Database does not exist." database-id))]
+      (if (:is_full_sync database)
+        (field-values/update-field-values! database)
+        (log/info (trs "Skipping update, automatic Field value updates are disabled for Database {0}." database-id))))))
 
 
 ;;; +----------------------------------------------------------------------------------------------------------------+
@@ -183,10 +190,12 @@
   (task/add-job! sync-analyze-job)
   (task/add-job! field-values-job))
 
-(defmethod task/init! ::SyncDatabases [_]
+(defmethod task/init! ::SyncDatabases
+  [_]
   (job-init)
   (doseq [database (db/select Database)]
     (try
+      ;; TODO -- shouldn't all the triggers be scheduled already?
       (schedule-tasks-for-db! database)
       (catch Throwable e
         (log/error e (trs "Failed to schedule tasks for Database {0}" (:id database)))))))

src/metabase/util/cron.clj

@@ -131,9 +131,8 @@
          (not= hours "*"))                        "daily"
     :else                                         "hourly"))
 
-
 (s/defn ^{:style/indent 0} cron-string->schedule-map :- ScheduleMap
-  "Convert a normal CRON-STRING into the expanded ScheduleMap format used by the frontend."
+  "Convert a normal `cron-string` into the expanded ScheduleMap format used by the frontend."
   [cron-string :- CronScheduleString]
   (let [[_ _ hours day-of-month _ day-of-week _] (str/split cron-string #"\s+")]
     {:schedule_day   (cron->day-of-week day-of-week)

test/expectations.clj

@@ -137,8 +137,8 @@
     ;; expecting it.
     (when-not (env/env :drivers)
       (t/testing "Don't write any new tests using expect!"
-        (t/is (<= total-expect-forms 1555))
-        (t/is (<= total-namespaces-using-expect 106))))))
+        (t/is (<= total-expect-forms 1084))
+        (t/is (<= total-namespaces-using-expect 97))))))
 
 (defmacro ^:deprecated expect
   "Simple macro that simulates converts an Expectations-style `expect` form into a `clojure.test` `deftest` form."