Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

metabase.util.schema to Malli #27471

Merged
merged 19 commits into from
Jan 5, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .lsp/config.edn
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,5 @@
:clean {:ns-inner-blocks-indentation :keep}
:linters {:clojure-lsp/unused-public-var {:level :warning
:exclude-when-defined-by
#{metabase.api.common/defendpoint
metabase.api.common/malli-defendpoint}}}}
#{metabase.api.common/defendpoint-schema
metabase.api.common/defendpoint}}}}
25 changes: 21 additions & 4 deletions src/metabase/api/common.clj
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@
;; TODO - several of the things `defendpoint` does could and should just be done by custom Ring middleware instead
;; e.g. `auto-parse`
(defmacro defendpoint-schema
"Define an API function.
"Define an API function with Plumatic Schema.
This automatically does several things:

- calls `auto-parse` to automatically parse certain args. e.g. `id` is converted from `String` to `Integer` via
Expand All @@ -290,8 +290,11 @@
- 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.

- Generates a super-sophisticated Markdown-formatted docstring"
{:arglists '([method route docstr? args schemas-map? & body])}
- Generates a super-sophisticated Markdown-formatted docstring.

DEPRECATED: use `defendpoint` instead where we use Malli to define the schema."
{:arglists '([method route docstr? args schemas-map? & body])
:deprecated "0.46.0"}
[& defendpoint-args]
(let [{:keys [args body arg->schema], :as defendpoint-args} (parse-defendpoint-args defendpoint-args)]
`(defendpoint* ~(assoc defendpoint-args
Expand Down Expand Up @@ -326,6 +329,20 @@
(wrap-response-if-needed
(do ~@body))))))))

(defmacro defendpoint-async-schema
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

write this to be asymmetry with defendpoint-schema and defendpoint

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch

"Like `defendpoint`, but generates an endpoint that accepts the usual `[request respond raise]` params.

DEPRECATED: use `defendpoint-async` instead where we use Malli to define the schema."
{:arglists '([method route docstr? args schemas-map? & body])
:deprecated "0.46.0"}
[& defendpoint-args]
(let [{:keys [args body arg->schema], :as defendpoint-args} (parse-defendpoint-args defendpoint-args)]
`(defendpoint* ~(assoc defendpoint-args
:args []
:body `((fn ~args
~@(validate-params arg->schema)
~@body))))))

(defmacro defendpoint-async
"Like `defendpoint`, but generates an endpoint that accepts the usual `[request respond raise]` params."
{:arglists '([method route docstr? args schemas-map? & body])}
Expand All @@ -334,7 +351,7 @@
`(defendpoint* ~(assoc defendpoint-args
:args []
:body `((fn ~args
~@(validate-params arg->schema)
~@(malli-validate-params arg->schema)
~@body))))))

(defn- namespace->api-route-fns
Expand Down
4 changes: 2 additions & 2 deletions src/metabase/api/geojson.clj
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@
(respond (-> (response/response is)
(response/content-type "application/json")))))

(api/defendpoint-async GET "/:key"
(api/defendpoint-async-schema GET "/:key"
"Fetch a custom GeoJSON file as defined in the `custom-geojson` setting. (This just acts as a simple proxy for the
file specified for `key`)."
[{{:keys [key]} :params} respond raise]
Expand All @@ -130,7 +130,7 @@
(raise (ex-info (tru "GeoJSON URL failed to load") {:status-code 400}))))
(raise (ex-info (tru "Invalid custom GeoJSON key: {0}" key) {:status-code 400}))))

(api/defendpoint-async GET "/"
(api/defendpoint-async-schema GET "/"
"Load a custom GeoJSON file based on a URL or file path provided as a query parameter.
This behaves similarly to /api/geojson/:key but doesn't require the custom map to be saved to the DB first."
[{{:keys [url]} :params} respond raise]
Expand Down
2 changes: 1 addition & 1 deletion src/metabase/models/pulse_card.clj
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
{:card_id su/IntGreaterThanZero
:pulse_id su/IntGreaterThanZero
:dashboard_card_id su/IntGreaterThanZero
(s/optional-key :position) (s/maybe su/NonNegativeInt)
(s/optional-key :position) (s/maybe su/IntGreaterThanOrEqualToZero)
(s/optional-key :include_csv) (s/maybe s/Bool)
(s/optional-key :include_xls) (s/maybe s/Bool)})

Expand Down
52 changes: 28 additions & 24 deletions src/metabase/util/malli/describe.clj
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,22 @@

(defn- diamond [s] (str "<" s ">"))
(defn- titled [schema] (if-let [t (-> schema mc/properties :title)] (str "(titled: ‘" t "’) ") ""))
(defn- min-max-suffix [schema]
(defn- min-max-suffix-length [schema]
(let [{:keys [min max]} (-> schema mc/properties)]
(cond
(and min max) (str " with length between " min " and " max)
(and min max) (str " with length between " min " and " max " inclusive")
min (str " with length longer than " min)
max (str " with length shorter than " max)
:else "")))

(defn- min-max-suffix-number [schema]
(let [{:keys [min max]} (-> schema mc/properties)]
(cond
(and min max) (str " between " min " and " max " inclusive")
min (str " greater than or equal to " min)
max (str " less than or equal to " max)
:else "")))

(defmulti accept
"Can this be accepted?"
(fn [name _schema _children _options] name) :default ::default)
Expand Down Expand Up @@ -86,22 +94,22 @@
" dispatched by " dispatcher)))

(defmethod accept :map-of [_ schema children _]
(str "a map " (titled schema) "from " (diamond (first children)) " to " (diamond (second children)) (min-max-suffix schema)))
(str "a map " (titled schema) "from " (diamond (first children)) " to " (diamond (second children)) (min-max-suffix-length schema)))

(defmethod accept 'vector? [_ schema children _] (str "vector" (titled schema) (min-max-suffix schema) " of " (first children)))
(defmethod accept :vector [_ schema children _] (str "vector" (titled schema) (min-max-suffix schema) " of " (first children)))
(defmethod accept 'vector? [_ schema children _] (str "vector" (titled schema) (min-max-suffix-length schema) " of " (first children)))
(defmethod accept :vector [_ schema children _] (str "vector" (titled schema) (min-max-suffix-length schema) " of " (first children)))

(defmethod accept 'sequential? [_ schema children _] (str "sequence" (titled schema) (min-max-suffix schema) " of " (first children)))
(defmethod accept :sequential [_ schema children _] (str "sequence" (titled schema) (min-max-suffix schema) " of " (first children)))
(defmethod accept 'sequential? [_ schema children _] (str "sequence" (titled schema) (min-max-suffix-length schema) " of " (first children)))
(defmethod accept :sequential [_ schema children _] (str "sequence" (titled schema) (min-max-suffix-length schema) " of " (first children)))

(defmethod accept 'set? [_ schema children _] (str "set" (titled schema) (min-max-suffix schema) " of " (first children)))
(defmethod accept :set [_ schema children _] (str "set" (titled schema) (min-max-suffix schema) " of " (first children)))
(defmethod accept 'set? [_ schema children _] (str "set" (titled schema) (min-max-suffix-length schema) " of " (first children)))
(defmethod accept :set [_ schema children _] (str "set" (titled schema) (min-max-suffix-length schema) " of " (first children)))

(defmethod accept 'string? [_ schema _ _] (str "string" (min-max-suffix schema)))
(defmethod accept :string [_ schema _ _] (str "string" (min-max-suffix schema)))
(defmethod accept 'string? [_ schema _ _] (str "string" (min-max-suffix-length schema)))
(defmethod accept :string [_ schema _ _] (str "string" (min-max-suffix-length schema)))

(defmethod accept 'number? [_ _ _ _] "number")
(defmethod accept :number [_ _ _ _] "number")
(defmethod accept 'number? [_ schema _ _] (str "number" (min-max-suffix-number schema)))
(defmethod accept :number [_ schema _ _] (str "number" (min-max-suffix-number schema)))

(defmethod accept 'pos-int? [_ _ _ _] "integer greater than 0")
(defmethod accept :pos-int [_ _ _ _] "integer greater than 0")
Expand All @@ -112,21 +120,21 @@
(defmethod accept 'nat-int? [_ _ _ _] "natural integer")
(defmethod accept :nat-int [_ _ _ _] "natural integer")

(defmethod accept 'float? [_ _ _ _] "float")
(defmethod accept :float [_ _ _ _] "float")
(defmethod accept 'float? [_ schema _ _] (str "float" (min-max-suffix-number schema)))
(defmethod accept :float [_ schema _ _] (str "float" (min-max-suffix-number schema)))

(defmethod accept 'pos? [_ _ _ _] "number greater than 0")
(defmethod accept :pos [_ _ _ _] "number greater than 0")

(defmethod accept 'neg? [_ _ _ _] "number less than 0")
(defmethod accept :neg [_ _ _ _] "number less than 0")

(defmethod accept 'integer? [_ schema _ _] (str "integer" (min-max-suffix schema)))
(defmethod accept 'int? [_ schema _ _] (str "integer" (min-max-suffix schema)))
(defmethod accept :int [_ schema _ _] (str "integer" (min-max-suffix schema)))
(defmethod accept 'integer? [_ schema _ _] (str "integer" (min-max-suffix-number schema)))
(defmethod accept 'int? [_ schema _ _] (str "integer" (min-max-suffix-number schema)))
(defmethod accept :int [_ schema _ _] (str "integer" (min-max-suffix-number schema)))

(defmethod accept 'double? [_ schema _ _] (str "double" (min-max-suffix schema)))
(defmethod accept :double [_ schema _ _] (str "double" (min-max-suffix schema)))
(defmethod accept 'double? [_ schema _ _] (str "double" (min-max-suffix-number schema)))
(defmethod accept :double [_ schema _ _] (str "double" (min-max-suffix-number schema)))

(defmethod accept :merge [_ schema _ options] ((::describe options) (mc/deref schema) options))
(defmethod accept :union [_ schema _ options] ((::describe options) (mc/deref schema) options))
Expand Down Expand Up @@ -185,10 +193,6 @@
(defmethod accept 'keyword? [_ _ _ _] "keyword")
(defmethod accept :keyword [_ _ _ _] "keyword")

(defmethod accept 'integer? [_ _ _ _] "integer")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was overriding the 'integer methods above.

(defmethod accept 'int? [_ _ _ _] "integer")
(defmethod accept :int [_ _ _ _] "integer")

(defn- -map [_n schema children _o]
(let [optional (set (->> children (filter (mc/-comp :optional second)) (mapv first)))
additional-properties (:closed (mc/properties schema))
Expand Down
Loading