Remove orphaned sync jobs from the database (#13121)

* convert tests to deftest

convert more expectations based tests to deftest

* Remove orphaned sync jobs from the database

If a database is deleted, make sure that any applicable sync jobs are
removed upon the next sync run.

Resolves #11813

* rename mdb to database for convention

src/metabase/task/sync_databases.clj

@@ -9,7 +9,7 @@
             [metabase
              [task :as task]
              [util :as u]]
-            [metabase.models.database :refer [Database]]
+            [metabase.models.database :as database :refer [Database]]
             [metabase.sync
              [analyze :as analyze]
              [field-values :as field-values]
@@ -27,6 +27,8 @@
 ;;; |                                                   JOB LOGIC                                                    |
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
+(declare unschedule-tasks-for-db!)
+
 (s/defn ^:private job-context->database-id :- (s/maybe su/IntGreaterThanZero)
   "Get the Database ID referred to in `job-context`."
   [job-context]
@@ -34,25 +36,39 @@
 
 ;; 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]
+
+(defn sync-and-analyze-database
+  "The sync and analyze database job, as a function that can be used in a test"
+  [job-context]
   (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)))]
+                            (do
+                              (unschedule-tasks-for-db! (database/map->DatabaseInstance {:id 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)))))
+        (analyze/analyze-db! database)))) )
 
-(jobs/defjob ^{org.quartz.DisallowConcurrentExecution true} UpdateFieldValues [job-context]
+(jobs/defjob ^{org.quartz.DisallowConcurrentExecution true} SyncAndAnalyzeDatabase [job-context]
+  (sync-and-analyze-database job-context))
+
+(defn update-field-values
+  "The update field values job, as a function that can be used in a test"
+  [job-context]
   (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))]
+                            (do
+                              (unschedule-tasks-for-db! (database/map->DatabaseInstance {:id 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))))))
 
+(jobs/defjob ^{org.quartz.DisallowConcurrentExecution true} UpdateFieldValues [job-context]
+  (update-field-values job-context))
 
 ;;; +----------------------------------------------------------------------------------------------------------------+
 ;;; |                                         TASK INFO AND GETTER FUNCTIONS                                         |

test/metabase/task/sync_databases_test.clj

@@ -5,7 +5,6 @@
   (:require [clojure
              [string :as str]
              [test :refer :all]]
-            [expectations :refer [expect]]
             [metabase.models.database :refer [Database]]
             [metabase.task.sync-databases :as sync-db]
             [metabase.test.util :as tu]
@@ -64,62 +63,59 @@
                   :data          {"db-id" "<id>"}}]})
 
 ;; Check that a newly created database automatically gets scheduled
-(expect
-  [sync-job fv-job]
-  (with-scheduler-setup
-    (tt/with-temp Database [database {:engine :postgres}]
-      (current-tasks-for-db database))))
-
+(deftest new-db-jobs-scheduled-test
+  (is (= [sync-job fv-job]
+         (with-scheduler-setup
+           (tt/with-temp Database [database {:engine :postgres}]
+             (current-tasks-for-db database))))))
 
 ;; Check that a custom schedule is respected when creating a new Database
-(expect
-  [(assoc-in sync-job [:triggers 0 :cron-schedule] "0 30 4,16 * * ? *")
-   (assoc-in fv-job   [:triggers 0 :cron-schedule] "0 15 10 ? * 6#3")]
-  (with-scheduler-setup
-    (tt/with-temp Database [database {:engine                      :postgres
-                                      :metadata_sync_schedule      "0 30 4,16 * * ? *" ; 4:30 AM and PM daily
-                                      :cache_field_values_schedule "0 15 10 ? * 6#3"}] ; 10:15 on the 3rd Friday of the Month
-      (current-tasks-for-db database))))
-
+(deftest custom-schedule-test
+  (is (= [(assoc-in sync-job [:triggers 0 :cron-schedule] "0 30 4,16 * * ? *")
+          (assoc-in fv-job   [:triggers 0 :cron-schedule] "0 15 10 ? * 6#3")]
+         (with-scheduler-setup
+           (tt/with-temp Database [database {:engine                      :postgres
+                                             :metadata_sync_schedule      "0 30 4,16 * * ? *" ; 4:30 AM and PM daily
+                                             :cache_field_values_schedule "0 15 10 ? * 6#3"}] ; 10:15 on the 3rd Friday of the Month
+             (current-tasks-for-db database))))))
 
 ;; Check that a deleted database gets unscheduled
-(expect
-  [(update sync-job :triggers empty)
-   (update fv-job   :triggers empty)]
-  (with-scheduler-setup
-    (tt/with-temp Database [database {:engine :postgres}]
-      (db/delete! Database :id (u/get-id database))
-      (current-tasks-for-db database))))
+(deftest unschedule-deleted-database-test
+  (is (= [(update sync-job :triggers empty)
+          (update fv-job   :triggers empty)]
+        (with-scheduler-setup
+          (tt/with-temp Database [database {:engine :postgres}]
+            (db/delete! Database :id (u/get-id database))
+            (current-tasks-for-db database))))))
 
 ;; Check that changing the schedule column(s) for a DB properly updates the scheduled tasks
-(expect
-  [(assoc-in sync-job [:triggers 0 :cron-schedule] "0 15 10 ? * MON-FRI")
-   (assoc-in fv-job   [:triggers 0 :cron-schedule] "0 11 11 11 11 ?")]
-  (with-scheduler-setup
-    (tt/with-temp Database [database {:engine :postgres}]
-      (db/update! Database (u/get-id database)
-        :metadata_sync_schedule      "0 15 10 ? * MON-FRI" ; 10:15 AM every weekday
-        :cache_field_values_schedule "0 11 11 11 11 ?")    ; Every November 11th at 11:11 AM
-      (current-tasks-for-db database))))
+(deftest schedule-change-test
+  (is (= [(assoc-in sync-job [:triggers 0 :cron-schedule] "0 15 10 ? * MON-FRI")
+          (assoc-in fv-job   [:triggers 0 :cron-schedule] "0 11 11 11 11 ?")]
+         (with-scheduler-setup
+           (tt/with-temp Database [database {:engine :postgres}]
+             (db/update! Database (u/get-id database)
+               :metadata_sync_schedule      "0 15 10 ? * MON-FRI" ; 10:15 AM every weekday
+               :cache_field_values_schedule "0 11 11 11 11 ?")    ; Every November 11th at 11:11 AM
+             (current-tasks-for-db database))))))
 
 ;; Check that changing one schedule doesn't affect the other
-(expect
-  [sync-job
-   (assoc-in fv-job [:triggers 0 :cron-schedule] "0 15 10 ? * MON-FRI")]
-  (with-scheduler-setup
-    (tt/with-temp Database [database {:engine :postgres}]
-      (db/update! Database (u/get-id database)
-        :cache_field_values_schedule "0 15 10 ? * MON-FRI")
-      (current-tasks-for-db database))))
-
-(expect
-  [(assoc-in sync-job [:triggers 0 :cron-schedule] "0 15 10 ? * MON-FRI")
-   fv-job]
-  (with-scheduler-setup
-    (tt/with-temp Database [database {:engine :postgres}]
-      (db/update! Database (u/get-id database)
-        :metadata_sync_schedule "0 15 10 ? * MON-FRI")
-      (current-tasks-for-db database))))
+(deftest schedule-changes-only-expected-test
+  (is (= [sync-job
+          (assoc-in fv-job [:triggers 0 :cron-schedule] "0 15 10 ? * MON-FRI")]
+        (with-scheduler-setup
+          (tt/with-temp Database [database {:engine :postgres}]
+            (db/update! Database (u/get-id database)
+              :cache_field_values_schedule "0 15 10 ? * MON-FRI")
+            (current-tasks-for-db database)))))
+
+  (is (= [(assoc-in sync-job [:triggers 0 :cron-schedule] "0 15 10 ? * MON-FRI")
+          fv-job]
+         (with-scheduler-setup
+           (tt/with-temp Database [database {:engine :postgres}]
+             (db/update! Database (u/get-id database)
+               :metadata_sync_schedule "0 15 10 ? * MON-FRI")
+             (current-tasks-for-db database))))))
 
 (deftest validate-schedules-test
   (testing "Check that you can't INSERT a DB with an invalid schedule"
@@ -138,6 +134,32 @@
                (db/update! Database (u/get-id database)
                  k "2 CANS PER DAY"))))))))
 
+(defrecord MockJobExecutionContext [job-data-map]
+  org.quartz.JobExecutionContext
+  (getMergedJobDataMap [this] (org.quartz.JobDataMap. job-data-map))
+
+  clojurewerkz.quartzite.conversion/JobDataMapConversion
+  (from-job-data [this]
+    (.getMergedJobDataMap this)))
+
+(deftest check-orphaned-jobs-removed-test
+  (testing "jobs for orphaned databases are removed during sync run"
+    (with-scheduler-setup
+      (doseq [sync-fn [sync-db/update-field-values sync-db/sync-and-analyze-database]]
+        (testing (str sync-fn)
+          (tt/with-temp Database [database {:engine :postgres}]
+            (let [db-id (:id database)]
+              (is (= [sync-job fv-job]
+                     (current-tasks-for-db database)))
+
+              (db/delete! Database :id db-id)
+              (let [ctx (MockJobExecutionContext. {"db-id" db-id})]
+                (sync-fn ctx))
+
+              (is (= [(update sync-job :triggers empty)
+                      (update fv-job :triggers empty)]
+                     (current-tasks-for-db database))))))))))
+
 ;;; +----------------------------------------------------------------------------------------------------------------+
 ;;; |                                    CHECKING THAT SYNC TASKS RUN CORRECT FNS                                    |
 ;;; +----------------------------------------------------------------------------------------------------------------+