Actions to toucan2 (#30685)

enterprise/backend/test/metabase_enterprise/models/entity_id_test.clj

@@ -34,9 +34,9 @@
   - exported as a child of something else (eg. timeline_event under timeline)
   so they don't need a generated entity_id."
   #{:metabase.db.data-migrations/DataMigrations
-    :metabase.models.action/HTTPAction
-    :metabase.models.action/ImplicitAction
-    :metabase.models.action/QueryAction
+    :model/HTTPAction
+    :model/ImplicitAction
+    :model/QueryAction
     :metabase.models.activity/Activity
     :metabase.models.application-permissions-revision/ApplicationPermissionsRevision
     :metabase.models.bookmark/BookmarkOrdering

src/metabase/models/action.clj

@@ -10,41 +10,61 @@
    [metabase.util :as u]
    [metabase.util.i18n :refer [tru]]
    [metabase.util.log :as log]
+   [methodical.core :as methodical]
    [toucan.db :as db]
-   [toucan.hydrate :refer [hydrate]]
-   [toucan.models :as models]
    [toucan2.core :as t2]))
 
-(models/defmodel QueryAction :query_action)
-(models/defmodel HTTPAction :http_action)
-(models/defmodel ImplicitAction :implicit_action)
-(models/defmodel Action :action)
+;;; -------------------------------------------- Entity & Life Cycle ----------------------------------------------
 
-(defn type->model
-  "Returns the model from an action type.
-   `action-type` can be a string or a keyword."
-  [action-type]
-  (case action-type
-    :http     HTTPAction
-    :implicit ImplicitAction
-    :query    QueryAction))
+(methodical/defmethod t2/table-name :model/Action [_model] :action)
+(methodical/defmethod t2/table-name :model/QueryAction [_model] :query_action)
+(methodical/defmethod t2/table-name :model/HTTPAction [_model] :http_action)
+(methodical/defmethod t2/table-name :model/ImplicitAction [_model] :implicit_action)
 
-;;; You can read/write an Action if you can read/write its model (Card)
-(doto Action
-  (derive ::mi/read-policy.full-perms-for-perms-set)
-  (derive ::mi/write-policy.full-perms-for-perms-set))
+;; Used to be the toucan1 model name defined using [[toucan.models/defmodel]], now it's a reference to the toucan2 model name.
+;; We'll keep this till we replace all the Actions symbol in our codebase.
+(def Action         "Action model"         :model/Action)
+(def QueryAction    "QueryAction model"    :model/QueryAction)
+(def HTTPAction     "HTTPAction model"     :model/HTTPAction)
+(def ImplicitAction "ImplicitAction model" :model/ImplicitAction)
 
-(defmethod mi/perms-objects-set Action
-  [instance read-or-write]
-  (mi/perms-objects-set (t2/select-one Card :id (:model_id instance)) read-or-write))
+(def ^:private action-sub-models [:model/QueryAction :model/HTTPAction :model/ImplicitAction])
 
-(models/add-type! ::json-with-nested-parameters
-  :in  (comp mi/json-in
-             (fn [template]
-               (u/update-if-exists template :parameters mi/normalize-parameters-list)))
-  :out (comp (fn [template]
-               (u/update-if-exists template :parameters (mi/catch-normalization-exceptions mi/normalize-parameters-list)))
-             mi/json-out-with-keywordization))
+(doto :model/Action
+  (derive :metabase/model)
+  ;;; You can read/write an Action if you can read/write its model (Card)
+  (derive ::mi/read-policy.full-perms-for-perms-set)
+  (derive ::mi/write-policy.full-perms-for-perms-set)
+  (derive :hook/entity-id)
+  (derive :hook/timestamped?))
+
+(doseq [model action-sub-models]
+  (derive model :metabase/model))
+
+(methodical/defmethod t2/primary-keys :model/QueryAction    [_model] [:action_id])
+(methodical/defmethod t2/primary-keys :model/HTTPAction     [_model] [:action_id])
+(methodical/defmethod t2/primary-keys :model/ImplicitAction [_model] [:action_id])
+
+(t2/deftransforms :model/Action
+  {:type                   mi/transform-keyword
+   :parameter_mappings     mi/transform-parameters-list
+   :parameters             mi/transform-parameters-list
+   :visualization_settings mi/transform-visualization-settings})
+
+(t2/deftransforms :model/QueryAction
+  ;; shouldn't this be mi/transform-metabase-query?
+  {:dataset_query mi/transform-json})
+
+(def ^:private transform-json-with-nested-parameters
+  {:in  (comp mi/json-in
+              (fn [template]
+                (u/update-if-exists template :parameters mi/normalize-parameters-list)))
+   :out (comp (fn [template]
+                (u/update-if-exists template :parameters (mi/catch-normalization-exceptions mi/normalize-parameters-list)))
+              mi/json-out-with-keywordization)})
+
+(t2/deftransforms :model/HTTPAction
+  {:template transform-json-with-nested-parameters})
 
 (mi/define-simple-hydration-method model
   :model
@@ -58,51 +78,39 @@
     (throw (ex-info (tru "Actions must be made with models, not cards.")
                     {:status-code 400}))))
 
-(t2/define-before-insert Action
+(t2/define-before-insert :model/Action
   [{model-id :model_id, :as action}]
   (u/prog1 action
     (check-model-is-not-a-saved-question model-id)))
 
-(t2/define-before-update Action
+(t2/define-before-update :model/Action
   [{archived? :archived, id :id, model-id :model_id, :as changes}]
   (u/prog1 changes
     (if archived?
       (t2/delete! DashboardCard :action_id id)
       (check-model-is-not-a-saved-question model-id))))
 
-(mi/define-methods
- Action
- {:types      (constantly {:type                   :keyword
-                           :parameter_mappings     :parameters-list
-                           :parameters             :parameters-list
-                           :visualization_settings :visualization-settings})
-  :properties (constantly {::mi/timestamped? true
-                           ::mi/entity-id    true})})
-
-(def ^:private Action-subtype-IModel-impl
-  "[[models/IModel]] impl for `HTTPAction`, `ImplicitAction`, and `QueryAction`"
-  {:primary-key (constantly :action_id)}) ; This is ok as long as we're 1:1
-
-(mi/define-methods
- QueryAction
- (merge
-  Action-subtype-IModel-impl
-  {:types (constantly {:dataset_query :json})}))
-
-(mi/define-methods
- ImplicitAction
- Action-subtype-IModel-impl)
-
-(mi/define-methods
- HTTPAction
- (merge Action-subtype-IModel-impl
-        {:types (constantly {:template ::json-with-nested-parameters})}))
+(defmethod mi/perms-objects-set :model/Action
+  [instance read-or-write]
+  (mi/perms-objects-set (t2/select-one Card :id (:model_id instance)) read-or-write))
 
 (def action-columns
   "The columns that are common to all Action types."
   [:archived :created_at :creator_id :description :entity_id :made_public_by_id :model_id :name :parameter_mappings
    :parameters :public_uuid :type :updated_at :visualization_settings])
 
+(defn type->model
+  "Returns the model from an action type.
+   `action-type` can be a string or a keyword."
+  [action-type]
+  (case action-type
+    :http     :model/HTTPAction
+    :implicit :model/ImplicitAction
+    :query    :model/QueryAction))
+
+
+;;; ------------------------------------------------ CRUD fns -----------------------------------------------------
+
 (defn insert!
   "Inserts an Action and related type table. Returns the action id."
   [action-data]
@@ -110,15 +118,15 @@
     (let [action (first (t2/insert-returning-instances! Action (select-keys action-data action-columns)))
           model  (type->model (:type action))]
       (t2/query-one {:insert-into (t2/table-name model)
-                    :values [(-> (apply dissoc action-data action-columns)
-                                 (assoc :action_id (:id action))
-                                 (cond->
-                                   (= (:type action) :implicit)
-                                   (dissoc :database_id)
-                                   (= (:type action) :http)
-                                   (update :template json/encode)
-                                   (= (:type action) :query)
-                                   (update :dataset_query json/encode)))]})
+                     :values [(-> (apply dissoc action-data action-columns)
+                                  (assoc :action_id (:id action))
+                                  (cond->
+                                    (= (:type action) :implicit)
+                                    (dissoc :database_id)
+                                    (= (:type action) :http)
+                                    (update :template json/encode)
+                                    (= (:type action) :query)
+                                    (update :dataset_query json/encode)))]})
       (:id action))))
 
 (defn update!
@@ -200,7 +208,7 @@
                                      :when table-id]
                                  [table-id card]))
         tables (when-let [table-ids (seq (keys card-by-table-id))]
-                 (hydrate (t2/select 'Table :id [:in table-ids]) :fields))]
+                 (t2/hydrate (t2/select 'Table :id [:in table-ids]) :fields))]
     (into {}
           (for [table tables
                 :let [fields (:fields table)]
@@ -298,7 +306,7 @@
   (eduction (map hydrate-subtype)
             (db/select-reducible 'Action)))
 
-(defmethod serdes/hash-fields Action [_action]
+(defmethod serdes/hash-fields :model/Action [_action]
   [:name (serdes/hydrated-hash :model) :created_at])
 
 (defmethod serdes/extract-one "Action" [_model-name _opts action]

test/metabase/models_test.clj

+(ns metabase.models-test
+  (:require
+   [clojure.test :refer :all]
+   [metabase.moderation]
+   [methodical.core :as methodical]
+   [toucan2.core :as t2]))
+
+(def toucan2-models
+  (->> (methodical/primary-methods t2/table-name)
+       keys
+       (filter keyword)
+       (remove #{:default})
+       (remove #(isa? % :toucan1/model))))
+
+(deftest toucan2-models-should-derive-test
+  (doseq [model toucan2-models]
+    (testing (format "base model %s should derive :metabase/model" model)
+      (is (true? (isa? model :metabase/model))))))