Merge pull request #210 from metabase/defendpoint_annotations

Defendpoint annotations

src/metabase/api/common.clj

@@ -5,7 +5,8 @@
             [medley.core :refer :all]
             [metabase.api.common.internal :refer :all]
             [metabase.db :refer :all]
-            [metabase.db.internal :refer [entity->korma]])
+            [metabase.db.internal :refer [entity->korma]]
+            [metabase.util :as u])
   (:import com.metabase.corvus.api.ApiException))
 
 (declare check-404)
@@ -170,28 +171,82 @@
 
 ;;; ## DEFENDPOINT AND RELATED FUNCTIONS
 
+
+;;; ### Arg annotation fns
+
+(defmulti arg-annotation-fn
+  "Multimethod that should return a form suitable for use in a let binding.
+
+   Dispatches on the arg annotation as a keyword, and is also passed the symbol
+   of the argument that should be checked.
+
+    (defendpoint GET ... [id] {id Required})
+
+     -> (let [id ~(arg-annotation-fn :Required id)]
+           ...)
+
+     -> (let [id (do (require-params id) id)]
+          ...)"
+  (fn [annotation-kw arg-symb]
+    {:pre [(keyword? annotation-kw)
+           (symbol? arg-symb)]}
+    annotation-kw))
+
+;; By default, throw an exception if we see an arg annotation we don't understand
+(defmethod arg-annotation-fn :default [annotation-kw arg-symbol]
+  (throw (Exception. (format "Don't know what to do with arg annotation '%s' on arg '%s'!" (name annotation-kw) (name arg-symbol)))))
+
+;; ### defannotation
+
+(defmacro defannotation
+  "Convenience for defining a new `defendpoint` arg annotation.
+
+   BINDING is the actual symbol name of the arg being checked; `defannotation` returns form(s)
+   that will be included in the let binding for the annotated arg.
+
+    (defannotation Required [param]
+      `(require-params ~param)       ; quasiquoting needed to keep require-params from being evaluated at macroexpansion time
+      param)"
+  [annotation-name [binding] & body]
+  `(defmethod arg-annotation-fn ~(keyword annotation-name) [~'_ ~binding]
+     `(do ~~@body)))
+
+;; ### common annotation definitions
+
+;; `required` just calls require-params
+(defannotation Required [param]
+  `(require-params ~param)
+  param)
+
+
+;;; ### defendpoint
+
 (defmacro defendpoint
   "Define an API function.
    This automatically does several things:
 
    -  calls `auto-parse` to automatically parse certain args. e.g. `id` is converted from `String` to `Integer` via `Integer/parseInt`
    -  converts ROUTE from a simple form like `\"/:id\"` to a typed one like `[\"/:id\" :id #\"[0-9]+\"]`
+   -  sequentially applies specified annotation functions on args to validate or cast them.
    -  executes BODY inside a `try-catch` block that handles `ApiExceptions`
    -  automatically calls `wrap-response-if-needed` on the result of BODY
    -  tags function's metadata in a way that subsequent calls to `define-routes` (see below)
       will automatically include the function in the generated `defroutes` form."
-  [method route args & body]
+  {:arglists '([method route args annotations-map? & body])}
+  [method route args & more]
   {:pre [(or (string? route)
              (vector? route))
          (vector? args)]}
   (let [name (route-fn-name method route)
-        route (typify-route route)]
+        route (typify-route route)
+        [arg-annotations body] (u/optional #(and (map? %) (every? symbol? (keys %))) more)]
     `(do (def ~name
            (~method ~route ~args
                     (auto-parse ~args
                       (catch-api-exceptions
-                        (-> (do ~@body)
-                            wrap-response-if-needed)))))
+                        (let-annotated-args ~arg-annotations
+                          (-> (do ~@body)
+                              wrap-response-if-needed))))))
          (alter-meta! #'~name assoc :is-endpoint? true))))
 
 (defmacro define-routes

src/metabase/api/common/internal.clj

 (ns metabase.api.common.internal
   "Internal functions used by `metabase.api.common`."
   (:require [clojure.tools.logging :as log]
-            [metabase.util :refer [fn-> regex?]])
+            [metabase.util :as u])
   (:import com.metabase.corvus.api.ApiException))
 
 ;;; # DEFENDPOINT HELPER FUNCTIONS + MACROS
@@ -70,7 +70,7 @@
     (route-arg-keywords \"/:id/cards\") -> [:id]"
   [route]
   (->> (re-seq #":([\w-]+)" route)
-       (map (fn-> second keyword))))
+       (map (u/fn-> second keyword))))
 
 (defn typify-args
   "Given a sequence of keyword ARGS, return a sequence of `[:arg pattern :arg pattern ...]`
@@ -145,3 +145,39 @@
     (if (is-wrapped? response) response
         {:status 200
          :body response})))
+
+
+;;; ## ARG ANNOTATION FUNCTIONALITY
+
+(defn arg-annotation-let-binding
+  "Return a pair like `[arg-symb arg-annotation-form]`, where `arg-annotation-form` is the result of calling the `arg-annotation-fn` implementation
+   for ANNOTATION-KW."
+  [[annotation-kw arg-symb]] ; dispatch-fn passed as a param to avoid circular dependencies
+  {:pre [(keyword? annotation-kw)
+         (symbol? arg-symb)]}
+  `[~arg-symb ~((eval 'metabase.api.common/arg-annotation-fn) annotation-kw arg-symb)])
+
+(defn process-arg-annotations [annotations-map]
+  {:pre [(or (nil? annotations-map)
+             (map? annotations-map))]}
+  (some->> annotations-map
+           (mapcat (fn [[arg annotations]]
+                     {:pre [(symbol? arg)
+                            (or (symbol? annotations)
+                                (every? symbol? annotations))]}
+                     (if (sequential? annotations) (->> annotations
+                                                        (map keyword)
+                                                        (map (u/rpartial vector arg)))
+                         [[(keyword annotations) arg]])))
+           (mapcat arg-annotation-let-binding)))
+
+(defmacro let-annotated-args
+  "Wrap BODY in a let-form that calls corresponding implementations of `arg-annotation-fn` for annotated args in ANNOTATED-ARGS-FORM."
+  [arg-annotations & body]
+  {:pre [(or (nil? arg-annotations)
+             (map? arg-annotations))]}
+  (let [annotations (process-arg-annotations arg-annotations)]
+    (if (seq annotations)
+      `(let [~@annotations]
+         ~@body)
+      `(do ~@body))))

src/metabase/api/emailreport.clj

@@ -81,9 +81,9 @@
   (del EmailReport :id id))
 
 
-(defendpoint POST "/:id" [id]
-  ;; TODO - implementation (execute a report)
-  {:TODO "TODO"})
+;; TODO
+;; (defendpoint POST "/:id" [id]
+;;   {:TODO "TODO"})
 
 
 (defendpoint GET "/:id/executions" [id]

src/metabase/api/user.clj

@@ -9,6 +9,14 @@
             [metabase.util :refer [is-email? select-non-nil-keys]]
             [metabase.util.password :as password]))
 
+(defannotation Email [email]
+  `(check (is-email? ~email) [400 (format ~(str (name email) " '%s' is not a valid email.") ~email)])
+  email)
+
+(defannotation ComplexPassword [password]
+  `(check (password/is-complex? ~password) [400 "Insufficient password strength"])
+  password)
+
 
 (defendpoint GET "/" []
   ;; user must be a superuser to proceed
@@ -28,20 +36,20 @@
 
 
 (defendpoint PUT "/:id" [id :as {{:keys [email] :as body} :body}]
+  {email [Required Email]}
   ;; user must be getting their own details OR they must be a superuser to proceed
   (check-403 (or (= id *current-user-id*) (:is_superuser @*current-user*)))
   ;; can't change email if it's already taken BY ANOTHER ACCOUNT
   (when id
-    (check-400 (is-email? email))
     (check-400 (not (exists? User :email email :id [not= id]))))
   (check-500 (->> (select-non-nil-keys body :email :first_name :last_name)
                   (mapply upd User id)))
   (sel :one User :id id))
 
 
-(defendpoint PUT "/:id/password" [id :as {{:keys [password old_password] :as body} :body}]
-  (require-params password old_password)
-  (check (password/is-complex? password) [400 "Insufficient password strength"])
+(defendpoint PUT "/:id/password" [id :as {{:keys [password old_password]} :body}]
+  {password [Required ComplexPassword]
+   old_password Required}
   (check-403 (or (= id *current-user-id*)
                  (:is_superuser @*current-user*)))
   (let-404 [user (sel :one [User :password_salt :password] :id id)]

test/metabase/api/common_test.clj

@@ -175,8 +175,11 @@
        (GET ["/:id" :id "#[0-9]+"] [id]
          (metabase.api.common.internal/auto-parse [id]
            (metabase.api.common.internal/catch-api-exceptions
-             (clojure.core/-> (do (->404 (sel :one Card :id id)))
-                              metabase.api.common.internal/wrap-response-if-needed)))))
+             (metabase.api.common.internal/let-annotated-args
+              {id Required}
+              (clojure.core/-> (do (->404 (sel :one Card :id id)))
+                               metabase.api.common.internal/wrap-response-if-needed))))))
      (clojure.core/alter-meta! #'GET_:id clojure.core/assoc :is-endpoint? true))
    (defendpoint GET "/:id" [id]
+     {id Required}
      (->404 (sel :one Card :id id)))))

test/metabase/api/user_test.clj

@@ -149,7 +149,7 @@
 
 ;; Check that a non-superuser CANNOT update someone else's user details
 (expect "You don't have permissions to do that."
-  ((user->client :rasta) :put 403 (str "user/" (user->id :trashbird))))
+  ((user->client :rasta) :put 403 (str "user/" (user->id :trashbird)) {:email "toucan@metabase.com"}))
 
 
 ;; ## PUT /api/user/:id/password