Skip to content
Snippets Groups Projects
Unverified Commit b603c439 authored by Cam Saul's avatar Cam Saul Committed by GitHub
Browse files

Remove `u/optional` (#23877) 

* Remove u/optional

* Spec => `s/` and Schema => `schema/`

* Fix docstring generation.

* Another test fix :wrench:

* Remove unused namespaces

* Parallelize some of the defendpoint tests
parent 1088634f
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
src/metabase/api/common.clj
+ 42
− 34
View file @ b603c439
(ns metabase.api.common
"Dynamic variables and utility functions/macros for writing API functions."
(:require [clojure.string :as str]
(:require [clojure.spec.alpha :as s]
[clojure.string :as str]
[clojure.tools.logging :as log]
[compojure.core :as compojure]
[honeysql.types :as htypes]
......@@ -12,7 +13,7 @@
[metabase.util :as u]
[metabase.util.i18n :as i18n :refer [deferred-tru tru]]
[metabase.util.schema :as su]
[schema.core :as s]
[schema.core :as schema]
[toucan.db :as db]))
(declare check-403 check-404)
......@@ -228,27 +229,34 @@
;;; --------------------------------------- DEFENDPOINT AND RELATED FUNCTIONS ----------------------------------------
(defn- parse-defendpoint-args [[method route & more]]
(let [fn-name (route-fn-name method route)
route (add-route-param-regexes route)
[docstr [args & more]] (u/optional string? more)
[arg->schema body] (u/optional (every-pred map? #(every? symbol? (keys %))) more)]
(when-not docstr
(s/def ::defendpoint-args
(s/cat
:method symbol?
:route (some-fn string? sequential?)
:docstr (s/? string?)
:args vector?
:arg->schema (s/? (s/map-of symbol? any?))
:body (s/* any?)))
(defn- parse-defendpoint-args [args]
(let [parsed (s/conform ::defendpoint-args args)]
(when (= parsed ::s/invalid)
(throw (ex-info (str "Invalid defendpoint args: " (s/explain-str ::defendpoint-args args))
(s/explain-data ::defendpoint-args args))))
(let [{:keys [method route docstr args arg->schema body]} parsed
fn-name (route-fn-name method route)
route (add-route-param-regexes route)
;; eval the vals in arg->schema to make sure the actual schemas are resolved so we can document
;; their API error messages
docstr (route-dox method route docstr args (m/map-vals eval arg->schema) body)]
;; Don't i18n this, it's dev-facing only
(log/warn (u/format-color 'red "Warning: endpoint %s/%s does not have a docstring. Go add one."
(ns-name *ns*) fn-name)))
{:method method
:route route
:fn-name fn-name
;; eval the vals in arg->schema to make sure the actual schemas are resolved so we can document
;; their API error messages
:docstr (route-dox method route docstr args (m/map-vals eval arg->schema) body)
:args args
:arg->schema arg->schema
:body body}))
(when-not docstr
(log/warn (u/format-color 'red "Warning: endpoint %s/%s does not have a docstring. Go add one."
(ns-name *ns*) fn-name)))
(assoc parsed :fn-name fn-name, :route route, :docstr docstr))))
(defmacro defendpoint*
"Impl macro for `defendpoint`; don't use this directly."
"Impl macro for [[defendpoint]]; don't use this directly."
[{:keys [method route fn-name docstr args body]}]
{:pre [(or (string? route) (vector? route))]}
`(def ~(vary-meta fn-name
......@@ -418,7 +426,7 @@
(check (not (and limit (not offset))) [400 (tru "When including a limit, an offset must also be included.")])
(check (not (and offset (not limit))) [400 (tru "When including an offset, a limit must also be included.")]))
(s/defn column-will-change? :- s/Bool
(schema/defn column-will-change? :- schema/Bool
"Helper for PATCH-style operations to see if a column is set to change when `object-updates` (i.e., the input to the
endpoint) is applied.
......@@ -428,7 +436,7 @@
(api/column-will-change? :archived (Collection 10) {:archived false}) ; -> false, because value did not change
(api/column-will-change? :archived (Collection 10) {}) ; -> false; value not specified in updates (request body)"
[k :- s/Keyword, object-before-updates :- su/Map, object-updates :- su/Map]
[k :- schema/Keyword, object-before-updates :- su/Map, object-updates :- su/Map]
(boolean
(and (contains? object-updates k)
(not= (get object-before-updates k)
......@@ -436,12 +444,12 @@
;;; ------------------------------------------ COLLECTION POSITION HELPER FNS ----------------------------------------
(s/defn reconcile-position-for-collection!
(schema/defn reconcile-position-for-collection!
"Compare `old-position` and `new-position` to determine what needs to be updated based on the position change. Used
for fixing card/dashboard/pulse changes that impact other instances in the collection"
[collection-id :- (s/maybe su/IntGreaterThanZero)
old-position :- (s/maybe su/IntGreaterThanZero)
new-position :- (s/maybe su/IntGreaterThanZero)]
[collection-id :- (schema/maybe su/IntGreaterThanZero)
old-position :- (schema/maybe su/IntGreaterThanZero)
new-position :- (schema/maybe su/IntGreaterThanZero)]
(let [update-fn! (fn [plus-or-minus position-update-clause]
(doseq [model '[Card Dashboard Pulse]]
(db/update-where! model {:collection_id collection-id
......@@ -464,25 +472,25 @@
(def ^:private ModelWithPosition
"Intended to cover Cards/Dashboards/Pulses, it only asserts collection id and position, allowing extra keys"
{:collection_id (s/maybe su/IntGreaterThanZero)
:collection_position (s/maybe su/IntGreaterThanZero)
s/Any s/Any})
{:collection_id (schema/maybe su/IntGreaterThanZero)
:collection_position (schema/maybe su/IntGreaterThanZero)
schema/Any schema/Any})
(def ^:private ModelWithOptionalPosition
"Intended to cover Cards/Dashboards/Pulses updates. Collection id and position are optional, if they are not
present, they didn't change. If they are present, they might have changed and we need to compare."
{(s/optional-key :collection_id) (s/maybe su/IntGreaterThanZero)
(s/optional-key :collection_position) (s/maybe su/IntGreaterThanZero)
s/Any s/Any})
{(schema/optional-key :collection_id) (schema/maybe su/IntGreaterThanZero)
(schema/optional-key :collection_position) (schema/maybe su/IntGreaterThanZero)
schema/Any schema/Any})
(s/defn maybe-reconcile-collection-position!
(schema/defn maybe-reconcile-collection-position!
"Generic function for working on cards/dashboards/pulses. Checks the before and after changes to see if there is any
impact to the collection position of that model instance. If so, executes updates to fix the collection position
that goes with the change. The 2-arg version of this function is used for a new card/dashboard/pulse (i.e. not
updating an existing instance, but creating a new one)."
([new-model-data :- ModelWithPosition]
(maybe-reconcile-collection-position! nil new-model-data))
([{old-collection-id :collection_id, old-position :collection_position, :as _before-update} :- (s/maybe ModelWithPosition)
([{old-collection-id :collection_id, old-position :collection_position, :as _before-update} :- (schema/maybe ModelWithPosition)
{new-collection-id :collection_id, new-position :collection_position, :as model-updates} :- ModelWithOptionalPosition]
(let [updated-collection? (and (contains? model-updates :collection_id)
(not= old-collection-id new-collection-id))
......
src/metabase/util.clj
+ 0
− 19
View file @ b603c439
......@@ -89,25 +89,6 @@
[& body]
`(try ~@body (catch Throwable ~'_)))
(defn optional
"Helper function for defining functions that accept optional arguments. If `pred?` is true of the first item in `args`,
a pair like `[first-arg other-args]` is returned; otherwise, a pair like `[default other-args]` is returned.
If `default` is not specified, `nil` will be returned when `pred?` is false.
(defn
^{:arglists ([key? numbers])}
wrap-nums [& args]
(let [[k nums] (optional keyword? args :nums)]
{k nums}))
(wrap-nums 1 2 3) -> {:nums [1 2 3]}
(wrap-nums :numbers 1 2 3) -> {:numbers [1 2 3]}"
{:arglists '([pred? args]
[pred? args default])}
[pred? args & [default]]
(if (pred? (first args)) [(first args) (next args)]
[default args]))
(defmacro varargs
"Make a properly-tagged Java interop varargs argument. This is basically the same as `into-array` but properly tags
the result.
......
test/metabase/api/common_test.clj
+ 41
− 21
View file @ b603c439
(ns metabase.api.common-test
(:require [clojure.test :refer :all]
[metabase.api.common :as api :refer :all]
[metabase.api.common.internal :refer :all]
[metabase.api.common :as api]
[metabase.api.common.internal :as api.internal]
[metabase.server.middleware.exceptions :as mw.exceptions]
[metabase.server.middleware.misc :as mw.misc]
[metabase.server.middleware.security :as mw.security]
[metabase.test.data :refer :all]
[metabase.util.schema :as su]))
[metabase.server.middleware.security :as mw.security]))
;;; TESTS FOR CHECK (ETC)
......@@ -36,19 +34,19 @@
identity
(fn [e] (throw e))))
(defn- my-mock-api-fn []
(defn my-mock-api-fn []
(mock-api-fn
(fn [_]
(check-404 @*current-user*)
(api/check-404 @api/*current-user*)
{:status 200
:body @*current-user*})))
:body @api/*current-user*})))
(deftest check-404-test
(deftest ^:parallel check-404-test
(testing "check that `check-404` doesn't throw an exception if `test` is true"
(is (= {:status 200
:body "Cam Saul"
:headers {"Content-Type" "text/plain"}}
(binding [*current-user* (atom "Cam Saul")]
(binding [api/*current-user* (atom "Cam Saul")]
(my-mock-api-fn)))))
(testing "check that 404 is returned otherwise"
......@@ -60,7 +58,7 @@
(is (= four-oh-four
(-> (mock-api-fn
(fn [_]
(let-404 [user nil]
(api/let-404 [user nil]
{:user user})))
(update-in [:headers "Last-Modified"] string?)))))
......@@ -69,20 +67,42 @@
((mw.exceptions/catch-api-exceptions
(fn [_ respond _]
(respond
(let-404 [user {:name "Cam"}]
(api/let-404 [user {:name "Cam"}]
{:user user}))))
nil
identity
(fn [e] (throw e)))))))
(deftest defendpoint-test
(deftest ^:parallel parse-defendpoint-args-test
(is (= {:method 'POST
:route ["/:id/dimension" :id "[0-9]+"]
:docstr String
:args '[id :as {{dimension-type :type, dimension-name :name} :body}]
:arg->schema '{dimension-type schema.core/Int, dimension-name schema.core/Str}
:fn-name 'POST_:id_dimension}
(-> (#'api/parse-defendpoint-args
'[POST "/:id/dimension"
"Sets the dimension for the given object with ID."
[id :as {{dimension-type :type, dimension-name :name} :body}]
{dimension-type schema.core/Int
dimension-name schema.core/Str}])
(update :docstr class)
;; two regex patterns are not equal even if they're the exact same pattern so convert to string so we can
;; compare easily.
(update-in [:route 2] str)))))
(deftest ^:parallel defendpoint-test
;; replace regex `#"[0-9]+"` with str `"#[0-9]+" so expectations doesn't barf
(binding [*auto-parse-types* (update-in *auto-parse-types* [:int :route-param-regex] (partial str "#"))]
(binding [api.internal/*auto-parse-types* (update-in api.internal/*auto-parse-types* [:int :route-param-regex] (partial str "#"))]
(is (= '(def GET_:id
(compojure.core/GET ["/:id" :id "#[0-9]+"] [id]
(metabase.api.common.internal/auto-parse [id]
(metabase.api.common.internal/validate-param 'id id metabase.util.schema/IntGreaterThanZero)
(metabase.api.common.internal/wrap-response-if-needed (do (select-one Card :id id))))))
(macroexpand `(defendpoint GET "/:id" [~'id]
{~'id su/IntGreaterThanZero}
(~'select-one ~'Card :id ~'id)))))))
(compojure.core/GET
["/:id" :id "#[0-9]+"]
[id]
(metabase.api.common.internal/auto-parse [id]
(metabase.api.common.internal/validate-param 'id id metabase.util.schema/IntGreaterThanZero)
(metabase.api.common.internal/wrap-response-if-needed
(do
(select-one Card :id id))))))
(macroexpand '(metabase.api.common/defendpoint compojure.core/GET "/:id" [id]
{id metabase.util.schema/IntGreaterThanZero}
(select-one Card :id id)))))))
test/metabase/api/setup_test.clj
+ 32
− 22
View file @ b603c439
(ns metabase.api.setup-test
"Tests for /api/setup endpoints."
(:require [clojure.core.async :as a]
[clojure.spec.alpha :as s]
[clojure.test :refer :all]
[medley.core :as m]
[metabase.analytics.snowplow-test :as snowplow-test]
......@@ -18,7 +19,7 @@
[metabase.test.fixtures :as fixtures]
[metabase.util :as u]
[metabase.util.schema :as su]
[schema.core :as s]
[schema.core :as schema]
[toucan.db :as db]))
;; make sure the default test users are created before running these tests, otherwise we're going to run into issues
......@@ -65,7 +66,7 @@
(fn []
(with-redefs [api.setup/*allow-api-setup-after-first-user-is-created* true]
(testing "API response should return a Session UUID"
(is (schema= {:id (s/pred mt/is-uuid-string? "UUID string")}
(is (schema= {:id (schema/pred mt/is-uuid-string? "UUID string")}
(client/client :post 200 "setup" request-body))))
;; reset our setup token
(setup/create-token!)
......@@ -87,11 +88,11 @@
(testing "Should record :user-joined Activity (#12933)"
(let [user-id (u/the-id (db/select-one-id User :email email))]
(is (schema= {:topic (s/eq :user-joined)
:model_id (s/eq user-id)
:user_id (s/eq user-id)
:model (s/eq "user")
s/Keyword s/Any}
(is (schema= {:topic (schema/eq :user-joined)
:model_id (schema/eq user-id)
:user_id (schema/eq user-id)
:model (schema/eq "user")
schema/Keyword schema/Any}
(wait-for-result #(db/select-one Activity :topic "user-joined", :user_id user-id)))))))))))
(deftest invite-user-test
......@@ -187,10 +188,10 @@
:name db-name
:details (:details (mt/db))}}
(testing ":database-create events should have been fired"
(is (schema= {:topic (s/eq :database-create)
:item {:id su/IntGreaterThanZero
:name (s/eq db-name)
s/Keyword s/Any}}
(is (schema= {:topic (schema/eq :database-create)
:item {:id su/IntGreaterThanZero
:name (schema/eq db-name)
schema/Keyword schema/Any}}
(mt/wait-for-result chan 100))))
(testing "Database should be synced"
......@@ -210,19 +211,28 @@
:database {:engine "my-fake-driver"
:name (mt/random-name)
:details {}})))))))))
(s/def ::setup!-args
(s/cat :expected-status (s/? integer?)
:f any?
:args (s/* any?)))
(defn- setup!
{:arglists '([expected-status? f & args])}
[& args]
(let [[expected-status args] (u/optional integer? args)
[f & args] args
body {:token (setup/create-token!)
:prefs {:site_name "Metabase Test"}
:user {:first_name (mt/random-name)
:last_name (mt/random-name)
:email (mt/random-email)
:password "anythingUP12!!"}}
body (apply f body args)]
(do-with-setup* body #(client/client :post (or expected-status 400) "setup" body))))
(let [parsed (s/conform ::setup!-args args)]
(when (= parsed ::s/invalid)
(throw (ex-info (str "Invalid setup! args: " (s/explain-str ::setup!-args args))
(s/explain-data ::setup!-args args))))
(let [{:keys [expected-status f args]} parsed
body {:token (setup/create-token!)
:prefs {:site_name "Metabase Test"}
:user {:first_name (mt/random-name)
:last_name (mt/random-name)
:email (mt/random-email)
:password "anythingUP12!!"}}
body (apply f body args)]
(do-with-setup* body #(client/client :post (or expected-status 400) "setup" body)))))
(deftest setup-validation-test
(testing "POST /api/setup validation"
......@@ -341,7 +351,7 @@
(fn [& args]
(apply orig args)
(throw (ex-info "Oops!" {}))))]
(is (schema= {:message (s/eq "Oops!"), s/Keyword s/Any}
(is (schema= {:message (schema/eq "Oops!"), schema/Keyword schema/Any}
(client/client :post 500 "setup" body))))
(testing "New user shouldn't exist"
(is (= false
......
test/metabase/http_client.clj
+ 36
− 23
View file @ b603c439
......@@ -3,6 +3,7 @@
(:require [cheshire.core :as json]
[clj-http.client :as http]
[clojure.edn :as edn]
[clojure.spec.alpha :as s]
[clojure.string :as str]
[clojure.test :as t]
[clojure.tools.logging :as log]
......@@ -16,7 +17,7 @@
[metabase.util.date-2 :as u.date]
[metabase.util.schema :as su]
[ring.util.codec :as codec]
[schema.core :as s]))
[schema.core :as schema]))
;;; build-url
......@@ -108,11 +109,11 @@
(def UUIDString
"Schema for a canonical string representation of a UUID."
(s/constrained
(schema/constrained
su/NonBlankString
(partial re-matches #"^[0-9a-f]{8}-(?:[0-9a-f]{4}-){3}[0-9a-f]{12}$")))
(s/defn authenticate :- UUIDString
(schema/defn authenticate :- UUIDString
"Authenticate a test user with `username` and `password`, returning their Metabase Session token; or throw an
Exception if that fails."
[credentials :- Credentials]
......@@ -173,15 +174,19 @@
:delete http/delete})
(def ^:private ClientParamsMap
{(s/optional-key :credentials) (s/maybe (s/cond-pre UUIDString Credentials))
:method (apply s/enum (keys method->request-fn))
(s/optional-key :expected-status) (s/maybe su/IntGreaterThanZero)
:url su/NonBlankString
(s/optional-key :http-body) (s/maybe su/Map)
(s/optional-key :query-parameters) (s/maybe su/Map)
(s/optional-key :request-options) (s/maybe su/Map)})
(s/defn ^:private -client
{(schema/optional-key :credentials) (schema/maybe (schema/cond-pre UUIDString Credentials))
:method (apply schema/enum (keys method->request-fn))
(schema/optional-key :expected-status) (schema/maybe su/IntGreaterThanZero)
:url su/NonBlankString
;; body can be either a map or a vector -- we encode it as JSON. Of course, other things are valid JSON as well, but
;; currently none of our endpoints accept them -- add them if needed.
(schema/optional-key :http-body) (schema/cond-pre
(schema/maybe su/Map)
(schema/maybe clojure.lang.IPersistentVector))
(schema/optional-key :query-parameters) (schema/maybe su/Map)
(schema/optional-key :request-options) (schema/maybe su/Map)})
(schema/defn ^:private -client
;; Since the params for this function can get a little complicated make sure we validate them
[{:keys [credentials method expected-status url http-body query-parameters request-options]} :- ClientParamsMap]
(initialize/initialize-if-needed! :db :web-server)
......@@ -214,20 +219,28 @@
(check-status-code method-name url body expected-status status)
(update response :body parse-response)))
(s/def ::http-client-args
(s/cat
:credentials (s/? (some-fn map? string?))
:method #{:get :put :post :delete}
:expected-status (s/? integer?)
:url string?
:request-options (s/? (every-pred map? :request-options))
:http-body (s/? (some-fn map? sequential?))
:query-parameters (s/* (s/cat :k keyword? :v any?))))
(defn- parse-http-client-args
"Parse the list of required and optional `args` into the various separated params that `-client` requires"
[args]
(let [[credentials [method & args]] (u/optional #(or (map? %) (string? %)) args)
[expected-status [url & args]] (u/optional integer? args)
[{:keys [request-options]} args] (u/optional (every-pred map? :request-options) args {:request-options {}})
[body [& {:as query-parameters}]] (u/optional map? args)]
{:credentials credentials
:method method
:expected-status expected-status
:url url
:http-body body
:query-parameters query-parameters
:request-options request-options}))
(let [parsed (s/conform ::http-client-args args)]
(when (= parsed ::s/invalid)
(throw (ex-info (str "Invalid http-client args: " (s/explain-str ::http-client-args args))
(s/explain-data ::http-client-args args))))
(cond-> parsed
;; un-nest {:request-options {:request-options <my-options>}} => {:request-options <my-options>}
(:request-options parsed) (update :request-options :request-options)
;; convert query parameters into a flat map [{:k :a, :v 1} {:k :b, :v 2}] => {:a 1, :b 2}
(:query-parameters parsed) (update :query-parameters (partial into {} (map (juxt :k :v)))))))
(def ^:private response-timeout-ms (u/seconds->ms 45))
......
test/metabase/test/data/users.clj
+ 1
− 1
View file @ b603c439
......@@ -167,7 +167,7 @@
(apply client-fn username args))))))
(s/defn ^:deprecated user->client :- (s/pred fn?)
"Returns a `metabase.http-client/client` partially bound with the credentials for User with `username`.
"Returns a [[metabase.http-client/client]] partially bound with the credentials for User with `username`.
In addition, it forces lazy creation of the User if needed.
((user->client) :get 200 \"meta/table\")
......
test/metabase/util/schema_test.clj
+ 3
− 5
View file @ b603c439
......@@ -17,8 +17,9 @@
(api/defendpoint POST "/:id/dimension"
"Sets the dimension for the given object with ID."
[id :as {{dimension-type :type, dimension-name :name} :body}]
{dimension-type (su/api-param "type" (s/enum "internal" "external"))
dimension-name su/NonBlankString})
{dimension-type (su/api-param "type" (s/enum "internal" "external"))
dimension-name su/NonBlankString})
(alter-meta! #'POST_:id_dimension assoc :private true)
(deftest ^:parallel api-param-test
......@@ -53,13 +54,10 @@
(deftest ^:parallel distinct-test
(is (= nil
(s/check (su/distinct [s/Int]) [])))
(is (= nil
(s/check (su/distinct [s/Int]) [1])))
(is (= nil
(s/check (su/distinct [s/Int]) [1 2])))
(is (some? (s/check (su/distinct [s/Int]) [1 2 1]))))
(deftest ^:parallel open-schema-test
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment