Skip to content
Snippets Groups Projects
Unverified Commit 4005c0a7 authored by github-automation-metabase's avatar github-automation-metabase Committed by GitHub
Browse files

[openapi] reduce amount of oneOf/allOf we generate in documentation (#51495) (#51569) 


Co-authored-by: default avatarAlexander Solovyov <alexander@solovyov.net>
parent de9424e1
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
src/metabase/api/common/openapi.clj
+ 71
− 84
View file @ 4005c0a7
......@@ -3,12 +3,12 @@
(:require
[clojure.set :as set]
[clojure.string :as str]
[clojure.walk :as walk]
[malli.core :as mc]
[malli.json-schema :as mjs]
[medley.core :as m]
[metabase.util :as u]
[metabase.util.malli.schema :as ms])
(:import [clojure.lang PersistentVector]))
(:import
(clojure.lang PersistentVector)))
(set! *warn-on-reflection* true)
......@@ -16,77 +16,62 @@
;;; Helpers
(defn- fix-locations
"Fixes all {:$ref ....} locations - could be in properties, or in allOf/anyOf with arbitrary nesting."
[schema]
(walk/postwalk (fn [x]
(cond-> x
(:$ref x) (update :$ref str/replace "#/definitions/" "#/components/schemas/")))
schema))
(defn- json-schema-transform [malli-schema]
(let [jss (mjs/transform malli-schema)
definitions (update-vals (:definitions jss) fix-locations)]
(defn- mjs-collect-definitions
"We transform json-schema in a few different places, but we need to collect all defitions in a single one."
[malli-schema]
(let [jss (mjs/transform malli-schema {::mjs/definitions-path "#/components/schemas/"})]
(when *definitions*
(swap! *definitions* merge definitions))
(-> jss
(dissoc :definitions)
(update :properties fix-locations))))
(defn- fix-type
"Change type of params to make it more understandable to Rapidoc."
[{:keys [schema] :as param}]
;; TODO: figure out how to teach rapidoc `[:or [:enum "all"] nat-int?]`
(cond
(and (:oneOf schema)
(= (second (:oneOf schema)) {:type "null"}))
(let [real-schema (merge (first (:oneOf schema))
(select-keys schema [:description :default]))]
(recur
(assoc param :required false :schema real-schema)))
(= (:enum schema) (mc/children ms/BooleanValue))
(assoc param :schema (-> (dissoc schema :enum) (assoc :type "boolean")))
(= (:enum schema) (mc/children ms/MaybeBooleanValue))
(assoc param
:schema (-> (dissoc schema :enum) (assoc :type "boolean"))
:required false)
:else
param))
(let [file-schema (json-schema-transform ms/File)]
;; TODO: unify this with `fix-type` somehow, but `:required` is making this hard
(defn- fix-schema
"Change type of request body to make it more understandable to Rapidoc."
[{:keys [required] :as schema}]
(let [not-required (atom #{})]
(-> schema
(update :properties (fn [props]
(into {}
(for [[k v] props]
[k
(cond
(and (:oneOf v)
(= (second (:oneOf v)) {:type "null"}))
(do
(swap! not-required conj k)
(merge (first (:oneOf v))
(select-keys v [:description :default])))
(= (select-keys v (keys file-schema)) file-schema)
;; I got this from StackOverflow and docs are not in agreement, but RapiDoc
;; shows file input, so... :)
(merge {:type "string" :format "binary"}
(select-keys v [:description :default]))
(= (:enum v) ["true" "false" true false])
(-> (dissoc v :enum) (assoc :type "boolean"))
:else
v)]))))
(assoc :required (vec (remove @not-required required)))))))
(swap! *definitions* merge (:definitions jss)))
(dissoc jss :definitions)))
(defn- merge-required [schema]
(let [optional? (set (keep (fn [[k v]] (when (:optional v) k))
(:properties schema)))]
(-> schema
(m/update-existing :required #(vec (remove optional? %)))
(m/update-existing :properties #(update-vals % (fn [v] (dissoc v :optional)))))))
(let [file-schema (mjs/transform ms/File)]
(defn- fix-json-schema
"Clean-up JSON schema to make it more understandable for OpenAPI tools.
Returns a new schema WITH an indicator if it's *NOT* required.
NOTE: maybe instead of fixing it up later we should re-work Malli's json-schema transformation into a way we want it to be?"
[schema]
(cond
;; we're using `[:maybe ...]` a lot, and it generates `{:oneOf [... {:type "null"}]}`
;; it needs to be cleaned up to be presented in OpenAPI viewers
(and (:oneOf schema)
(= (second (:oneOf schema)) {:type "null"}))
(recur (-> (first (:oneOf schema))
(merge (select-keys schema [:description :default]))
(assoc :optional true)))
;; this happens when we use `[:and ... [:fn ...]]`, the `:fn` schema gets converted into an empty object
(:allOf schema)
(cond
(= {} (first (:allOf schema))) (recur (merge (second (:allOf schema))
(select-keys schema [:description :default])))
(= {} (second (:allOf schema))) (recur (merge (first (:allOf schema))
(select-keys schema [:description :default])))
:else (update schema :allOf (partial mapv fix-json-schema)))
(= (select-keys schema (keys file-schema)) file-schema)
;; I got this from StackOverflow and docs are not in agreement, but RapiDoc
;; shows file input, so... :)
(merge {:type "string" :format "binary"}
(select-keys schema [:description :default]))
(:properties schema)
(merge-required
(update schema :properties #(update-vals % fix-json-schema)))
(= (:type schema) "array")
(update schema :items fix-json-schema)
:else
schema)))
(defn- path->openapi [path]
(str/replace path #":([^/]+)" "{$1}"))
......@@ -145,20 +130,22 @@
(defn- schema->params
"https://spec.openapis.org/oas/latest.html#parameter-object"
[full-path args schema]
(let [{:keys [properties required]} (json-schema-transform schema)
(let [{:keys [properties required]} (mjs-collect-definitions schema)
required (set required)
in-path? (set (map (comp keyword second) (re-seq #"\{([^}]+)\}" full-path)))
in-query? (set (compojure-query-params args))
renames (compojure-renames args)]
(for [[k param-schema] properties
:let [k (get renames k k)]
:when (or (in-path? k) (in-query? k))]
(fix-type
(cond-> {:in (if (in-path? k) :path :query)
:name k
:required (contains? required k)
:schema (dissoc param-schema :description)}
(:description param-schema) (assoc :description (:description param-schema)))))))
:when (or (in-path? k) (in-query? k))
:let [schema (fix-json-schema param-schema)
;; if schema does not indicate it's optional, it's not :)
optional? (:optional schema)]]
(cond-> {:in (if (in-path? k) :path :query)
:name k
:required (and (contains? required k) (not optional?))
:schema (dissoc schema :optional :description)}
(:description schema) (assoc :description (:description schema))))))
(defn- defendpoint->path-item
"Generate OpenAPI desc for a single handler
......@@ -171,8 +158,8 @@
body-params (when (not= method :get)
(remove #(non-body-param? (first %)) (rest (:schema data))))
body-schema (when (seq body-params)
(fix-schema
(json-schema-transform (into [:map] body-params))))
(fix-json-schema
(mjs-collect-definitions (into [:map] body-params))))
ctype (if (:multipart (meta handler-var))
"multipart/form-data"
"application/json")]
......@@ -200,7 +187,7 @@
(catch Exception e
(throw (ex-info (str "Exception at " path) {} e))))))]
{:paths paths
:components {:schemas @*definitions*}})))
:components {:schemas (update-vals @*definitions* fix-json-schema)}})))
(comment
;; See what is the result of generation, could be helpful debugging what's wrong with display in rapidoc
......
src/metabase/lib/schema/common.cljc
+ 2
− 1
View file @ 4005c0a7
......@@ -52,7 +52,8 @@
;;; Schema for a string that cannot be blank.
(mr/def ::non-blank-string
[:and
{:error/message "non-blank string"}
{:error/message "non-blank string"
:json-schema {:type "string" :minLength 1}}
[:string {:min 1}]
[:fn
{:error/message "non-blank string"}
......
src/metabase/util/malli/schema.clj
+ 5
− 3
View file @ 4005c0a7
......@@ -269,14 +269,15 @@
(def ^:private keyword-or-non-blank-str-malli
(mc/schema
[:or :keyword NonBlankString]))
[:or {:json-schema {:type "string" :minLength 1}} :keyword NonBlankString]))
(def BooleanValue
"Schema for a valid representation of a boolean
(one of `\"true\"` or `true` or `\"false\"` or `false`.).
Used by [[metabase.api.common/defendpoint]] to coerce the value for this schema to a boolean.
Garanteed to evaluate to `true` or `false` when passed through a json decoder."
(-> [:enum {:decode/json (fn [b] (contains? #{"true" true} b))}
(-> [:enum {:decode/json (fn [b] (contains? #{"true" true} b))
:json-schema {:type "boolean"}}
"true" "false" true false]
(mu/with-api-error-message
(deferred-tru "value must be a valid boolean string (''true'' or ''false'')."))))
......@@ -284,7 +285,8 @@
(def MaybeBooleanValue
"Same as above, but allows distinguishing between `nil` (the user did not specify a value)
and `false` (the user specified `false`)."
(-> [:enum {:decode/json (fn [b] (some->> b (contains? #{"true" true})))}
(-> [:enum {:decode/json (fn [b] (some->> b (contains? #{"true" true})))
:json-schema {:type "boolean" :optional true}}
"true" "false" true false nil]
(mu/with-api-error-message
(deferred-tru "value must be a valid boolean string (''true'' or ''false'')."))))
......
test/metabase/api/common/openapi_test.clj
+ 91
− 8
View file @ 4005c0a7
......@@ -10,6 +10,40 @@
[metabase.util.malli :as mu]
[metabase.util.malli.schema :as ms]))
;;; json-schema upgrades
(deftest json-schema-conversion
(testing ":maybe turns into optionality"
(is (= {:type "object"
:required []
:properties {:name {:type "string"}}}
(#'openapi/fix-json-schema
(mjs/transform [:map [:name [:maybe string?]]])))))
(testing ":json-schema basically works (see definition of NonBlankString)"
(is (=? {:description map?
:$ref "#/definitions/metabase.lib.schema.common~1non-blank-string",
:definitions {"metabase.lib.schema.common/non-blank-string" {:type "string", :minLength 1}}}
(mjs/transform ms/NonBlankString))))
(testing "maps-with-unique-key do not generate weirdness"
(is (=? {:description map?
:type "array"
:items {:type "object"
:required [:id]
:properties {:id {:type "integer"}}}}
(#'openapi/fix-json-schema
(mjs/transform (ms/maps-with-unique-key [:sequential [:map [:id :int]]] :id))))))
(testing "nested data structures are still fixed up"
(is (=? {:type "array"
:items {:type "object"
:properties {:params {:type "array"
:items {:type "string"}}}}}
(#'openapi/fix-json-schema
(mjs/transform [:sequential [:map
[:params {:optional true} [:maybe [:sequential :string]]]]]))))))
;;; inner helpers
(deftest ^:parallel parse-compojure-test
......@@ -59,13 +93,31 @@
{c ms/PositiveInt}
{:count c})
(api/defendpoint PUT "/complex/:id"
"More complex body schema"
[id :as {data :body}]
{id ms/PositiveInt
data [:map
[:name {:optional true} [:maybe ms/NonBlankString]]
[:dashcards (ms/maps-with-unique-key
[:sequential [:map
[:id int?]
[:params {:optional true} [:maybe [:sequential [:map
[:param_id ms/NonBlankString]
[:target :any]]]]]]]
:id)]]}
{:id id :data data})
(api/define-routes)
;;; tests
(deftest ^:parallel fix-locations-test
(is (=? {:properties {:value {:$ref "#/components/schemas/metabase.lib.schema.common~1non-blank-string"}}}
(#'openapi/fix-locations (mjs/transform [:map [:value ms/NonBlankString]])))))
(deftest ^:parallel collect-definitions-test
(binding [openapi/*definitions* (atom [])]
(is (=? {:properties {:value {:$ref "#/components/schemas/metabase.lib.schema.common~1non-blank-string"}}}
(#'openapi/mjs-collect-definitions [:map [:value ms/NonBlankString]])))
(is (= [{"metabase.lib.schema.common/non-blank-string" {:type "string", :minLength 1}}]
@@#'openapi/*definitions*))))
(deftest ^:parallel path->openapi-test
(is (= "/{model}/{yyyy-mm}"
......@@ -73,7 +125,8 @@
(deftest ^:parallel collect-routes-test
(testing "can collect routes in simple case"
(is (=? [{:path "/export"}
(is (=? [{:path "/complex/{id}"}
{:path "/export"}
{:path "/rename"}
{:path "/{id}"}
{:path "/{id}"}
......@@ -143,15 +196,45 @@
:name :count
:required false
:schema {:type "integer" :minimum 1}}]}}
(#'openapi/defendpoint->path-item nil "/rename" #'GET_rename))))
(#'openapi/defendpoint->path-item nil "/rename" #'GET_rename)))
(is (=? {:put
{:summary "PUT /complex/{id}"
:parameters
[{:in :path
:name :id
:required true
:schema {:type "integer", :minimum 1}
:description map?}]
:requestBody
{:content
{"application/json"
{:schema
{:type "object"
:required [:data]
:properties
{:data
{:type "object"
:required [:dashcards]
:properties
{:name {:description map?
:$ref "#/components/schemas/metabase.lib.schema.common~1non-blank-string"}
:dashcards {:type "array"
:description map?
:items {:type "object"
:required [:id]
:properties {:id {:type "integer"}
:params {:type "array"
:items {:type "object"
:required [:param_id :target]
:properties {:param_id {}
:target {}}}}}}}}}}}}}}}}
(#'openapi/defendpoint->path-item nil "/complex/{id}" #'PUT_complex_:id))))
(deftest ^:parallel openapi-object-test
(is (=? {:paths {"/{id}" {:get {}
:post {}}
"/{id}/upload" {:post {}}}
:components {:schemas {"metabase.lib.schema.common/non-blank-string"
{:allOf [{:type "string", :minLength 1}
{}]}}}}
:components {:schemas {"metabase.lib.schema.common/non-blank-string" {:type "string", :minLength 1}}}}
(openapi/openapi-object #'routes))))
(deftest ^:parallel openapi-all-routes
......
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