Repository: oliyh/martian
Pull request #196 (2024-01-23 18:48:40)

Fix Reference Object usage on parameters for swagger spec
As I commented in the issue
https://github.com/oliyh/martian/issues/195

This is not working for swagger, but it is for OpenAPI v3. I tried to refactor the code to use the same logic, adding tests for it.


oliyh: This is great, thank you!

Patch:
diff --git a/core/src/martian/openapi.cljc b/core/src/martian/openapi.cljc
index cd1724c..0884ccd 100644
--- a/core/src/martian/openapi.cljc
+++ b/core/src/martian/openapi.cljc
@@ -23,14 +23,6 @@
              api-root
              (get json :basePath ""))))))
 
-(defn- lookup-ref [components reference]
-  (if (string/starts-with? reference "#/components/")
-    (or (get-in components (drop 2 (map keyword (string/split reference #"/"))))
-        (throw (ex-info "Cannot find reference"
-                        {:reference reference})))
-    (throw (ex-info "References start with something other than #/components/ aren't supported yet. :("
-                    {:reference reference}))))
-
 (defn- wrap-nullable [{:keys [nullable]} schema]
   (if nullable
     (s/maybe schema)
@@ -49,7 +41,7 @@
      (if (contains? seen-set reference)
        s/Any ; If we've already seen this, then we're in a loop. Rather than
              ; trying to solve for the fixpoint, just return Any.
-       (recur (lookup-ref components reference)
+       (recur (martian.schema/lookup-ref reference {:components components})
               components
               (conj seen-set reference)))
      (wrap schema
@@ -144,17 +136,6 @@
             (keyword param-name)
             %) parts)))
 
-(defn- resolve-ref
-  "`components` are the keywordized value of the :components key from an
-   OpenAPI specification.
-   `param` is one of the values from
-   [:paths <some path> <some HTTP method> :parameters] of an OpenAPI spec."
-  [components param]
-  (if-let [ref (:$ref param)]
-     (lookup-ref components ref)
-     param))
-
-;; After martian is updated to use Clojure 1.11.0+ then replace this and the uses with update-vals.
 (defn update-vals-future
   "An implementation of `update-vals` that is in Clojure 1.11.0+."
   [m f]
@@ -162,20 +143,20 @@
 
 (defn openapi->handlers [openapi-json content-types]
   (let [openapi-spec (keywordize-keys openapi-json)
+        resolve-ref (martian.schema/resolve-ref-fn openapi-spec)
         components (:components openapi-spec)]
     (for [[url methods] (:paths openapi-spec)
-          :let [common-parameters (map (partial resolve-ref components) (:parameters methods))]
+          :let [common-parameters (map resolve-ref (:parameters methods))]
           [method definition] (dissoc methods :parameters)
           ;; We only care about things which have a defined operationId, and
           ;; which aren't the associated OPTIONS call.
           :when (and (:operationId definition)
                      (not= :options method))
           :let [parameters (group-by (comp keyword :in) (concat common-parameters
-                                                                (map (partial resolve-ref components)
-                                                                     (:parameters definition))))
+                                                                (map resolve-ref (:parameters definition))))
                 body       (process-body (:requestBody definition) components (:encodes content-types))
                 responses  (process-responses (update-vals-future (:responses definition)
-                                                                  (partial resolve-ref components))
+                                                                  resolve-ref)
                                               components (:decodes content-types))]]
       (-> {:path-parts         (vec (tokenise-path url))
            :method             method
diff --git a/core/src/martian/schema.cljc b/core/src/martian/schema.cljc
index e2fb695..3b01928 100644
--- a/core/src/martian/schema.cljc
+++ b/core/src/martian/schema.cljc
@@ -5,6 +5,7 @@
             [schema.coerce :as sc]
             [schema-tools.core :as st]
             [schema-tools.coerce :as stc]
+            [clojure.string :as string]
             [martian.parameter-aliases :refer [unalias-data]])
   #?(:clj (:import [schema.core AnythingSchema Maybe EnumSchema EqSchema])))
 
@@ -71,9 +72,31 @@
                (make-schema ref-lookup param)}))
        (into {})))
 
-(defn- resolve-ref [ref-lookup ref]
-  (let [[_ category k] (re-find #"#/(definitions|parameters)/(.*)" ref)]
-    (get-in ref-lookup [(keyword category) (keyword k)])))
+(defn lookup-ref
+  [ref ref-lookup]
+  (if (string/starts-with? ref "#/")
+    (or (get-in ref-lookup (drop 1 (map keyword (string/split ref #"/"))))
+        (throw (ex-info "Cannot find reference"
+                        {:reference ref})))
+    (throw (ex-info "Non-local references are not supported yet. References should start ref with '#/'"
+                    {:reference ref}))))
+
+(defn resolve-ref-object
+  "resolve-ref-object receives a map with '$ref` key and returns the referenced object.
+  Throws an exception if the reference is cyclic, it doesn't exist or is not supported"
+  ([ref-object ref-lookup]
+   (resolve-ref-object ref-object ref-lookup nil))
+  ([ref-object ref-lookup visited-list]
+   (if-let [ref (:$ref ref-object)]
+     (if (contains? (set visited-list) ref)
+       (throw (ex-info "Cyclic reference" {:type :cyclic-reference :reference ref :visited visited-list}))
+       (resolve-ref-object (lookup-ref ref ref-lookup) ref-lookup (conj (vec visited-list) ref)))
+     ref-object)))
+
+(defn resolve-ref-fn
+  "returns a function that receives an object and resolves it using resolve-ref-object"
+  [ref-lookup]
+  (fn [obj] (resolve-ref-object obj ref-lookup)))
 
 (def URI
   #?(:clj java.net.URI
@@ -140,7 +163,6 @@
 (defn make-schema
   "Takes a swagger parameter and returns a schema"
   [ref-lookup {:keys [required type schema $ref items] :as param}]
-
   (if (let [ref (or $ref (:$ref schema))]
         (and ref (contains? *visited-refs* ref)))
     s/Any ;; avoid potential recursive loops
@@ -149,12 +171,12 @@
       $ref
       (binding [*visited-refs* (conj *visited-refs* $ref)]
         (make-schema ref-lookup (-> (dissoc param :$ref)
-                                    (merge (resolve-ref ref-lookup $ref)))))
+                                    (merge (resolve-ref-object param ref-lookup)))))
 
       (:$ref schema)
       (binding [*visited-refs* (conj *visited-refs* (:$ref schema))]
         (make-schema ref-lookup (-> (dissoc param :schema)
-                                    (merge (resolve-ref ref-lookup (:$ref schema))))))
+                                    (merge (resolve-ref-object schema ref-lookup)))))
 
       :else
       (cond-> (cond
diff --git a/core/src/martian/swagger.cljc b/core/src/martian/swagger.cljc
index b70b1e2..e084fab 100644
--- a/core/src/martian/swagger.cljc
+++ b/core/src/martian/swagger.cljc
@@ -7,24 +7,30 @@
             [schema.core :as s]
             [martian.utils :as utils]))
 
+(defn resolve-swagger-params [ref-lookup swagger-params category]
+  (->> swagger-params
+       (map (schema/resolve-ref-fn ref-lookup))
+       (filter #(= category (:in %)))
+       not-empty))
+
 (defn- body-schema [ref-lookup swagger-params]
-  (when-let [body-params (not-empty (filter #(= "body" (:in %)) swagger-params))]
+  (when-let [body-params (resolve-swagger-params ref-lookup swagger-params "body")]
     (schema/schemas-for-parameters ref-lookup body-params)))
 
 (defn- form-schema [ref-lookup swagger-params]
-  (when-let [form-params (not-empty (filter #(= "formData" (:in %)) swagger-params))]
+  (when-let [form-params (resolve-swagger-params ref-lookup swagger-params "formData")]
     (schema/schemas-for-parameters ref-lookup form-params)))
 
 (defn- path-schema [ref-lookup swagger-params]
-  (when-let [path-params (not-empty (filter #(= "path" (:in %)) swagger-params))]
+  (when-let [path-params (resolve-swagger-params ref-lookup swagger-params "path")]
     (schema/schemas-for-parameters ref-lookup path-params)))
 
 (defn- query-schema [ref-lookup swagger-params]
-  (when-let [query-params (not-empty (filter #(= "query" (:in %)) swagger-params))]
+  (when-let [query-params (resolve-swagger-params ref-lookup swagger-params "query")]
     (schema/schemas-for-parameters ref-lookup query-params)))
 
 (defn- headers-schema [ref-lookup swagger-params]
-  (when-let [header-params (not-empty (filter #(= "header" (:in %)) swagger-params))]
+  (when-let [header-params (resolve-swagger-params ref-lookup swagger-params "header")]
     (schema/schemas-for-parameters ref-lookup header-params)))
 
 (defn- response-schemas [ref-lookup swagger-responses]


Test patch:
diff --git a/core/test/martian/schema_test.cljc b/core/test/martian/schema_test.cljc
index 1eff252..ad11f3b 100644
--- a/core/test/martian/schema_test.cljc
+++ b/core/test/martian/schema_test.cljc
@@ -3,7 +3,8 @@
             [schema.core :as s]
             [schema-tools.core :as st]
             #?(:clj [clojure.test :refer [deftest testing is]]
-               :cljs [cljs.test :refer-macros [deftest testing is]])))
+               :cljs [cljs.test :refer-macros [deftest testing is]]))
+  #?(:clj (:import [clojure.lang ExceptionInfo])))
 
 (deftest free-form-object-test
   (let [schema (schema/make-schema {:definitions
@@ -400,3 +401,34 @@
 
     (is (= (s/maybe {(s/optional-key :b) (s/maybe {(s/optional-key :a) s/Any})})
            schema))))
+
+
+
+(deftest resolve-ref-object-test
+  (let [params       {:idParam {:name "id" :in "query"}}
+        swagger-spec {:definitions params}
+        openapi-spec {:components {:parameters params}}
+        #?(:cljs ExceptionInfo) #?(:cljs js/Error)]
+    (testing "resolve simple references"
+      (is (= {:name "id" :in "query"} (schema/resolve-ref-object {:$ref "#/definitions/idParam"} swagger-spec)))
+      (is (= {:name "id" :in "query"} (schema/resolve-ref-object {:$ref "#/components/parameters/idParam"} openapi-spec))))
+    (testing "resolve parameters through multiple refs"
+      (is (= {:name "id" :in "query"} (schema/resolve-ref-object {:$ref "#/definitions/allId"}
+                                                                 {:definitions (assoc params :allId {:$ref "#/definitions/idParam"})})))
+      (is (= {:name "id" :in "query"} (schema/resolve-ref-object {:$ref "#/components/parameters/allId"}
+                                                                 {:components {:parameters (assoc params :allId {:$ref "#/components/parameters/idParam"})}})))
+      )
+    (testing "throws non local references"
+      (is (thrown-with-msg? ExceptionInfo #"^Non-local references are not supported yet.*"
+                            (schema/resolve-ref-object {:$ref "parameters.json#/definitions/idParam"} swagger-spec)))
+      (is (thrown-with-msg? ExceptionInfo #"^Non-local references are not supported yet.*"
+                            (schema/resolve-ref-object {:$ref "parameters.json#/components/parameters/idParam"} openapi-spec))))
+    (testing "throws on cyclic references"
+      (let [cyclic-params {:idParam {:$ref "#/definitions/id"}
+                           :id {:$ref "#/definitions/idParam"}}]
+        (is (thrown-with-msg? ExceptionInfo #"^Cyclic reference.*"
+                              (schema/resolve-ref-object {:$ref "#/definitions/idParam"} {:definitions cyclic-params})))
+        ))
+    (testing "throws when does not find reference"
+      (is (thrown-with-msg? ExceptionInfo #"^Cannot find reference.*" (schema/resolve-ref-object {:$ref "#/definitions/otherParam"} swagger-spec)))
+      )))
diff --git a/core/test/martian/swagger_test.cljc b/core/test/martian/swagger_test.cljc
index e302a8d..8c10701 100644
--- a/core/test/martian/swagger_test.cljc
+++ b/core/test/martian/swagger_test.cljc
@@ -18,6 +18,28 @@
     (is (= {:id s/Str} (:path-schema get-handler)))
     (is (= nil (:query-schema get-handler)))))
 
+(deftest parameter-schemas-ref-test
+  (let [params {:idParam {:name     "id"
+                         :in       "path"
+                         :type     "string"
+                         :required "true"}}]
+    (is (= {:id s/Str} (-> {:definitions params
+                            "paths"      {"/pets/{id}"
+                                          {"get" {"operationId" "getPetById"
+                                                  "method"      "get"
+                                                  "parameters"  [{"$ref" "#/definitions/idParam"}]}}}}
+                           swagger/swagger->handlers
+                           first
+                           :path-schema)))
+    (is (= {:id s/Str} (-> {:definitions params
+                            "paths"      {"/pets/{id}"
+                                          {"parameters" [{"$ref" "#/definitions/idParam"}]
+                                           "get"        {"operationId" "getPetById"
+                                                         "method"      "get"}}}}
+                           swagger/swagger->handlers
+                           first
+                           :path-schema)))))
+
 (deftest path-item-obj-parameters-test
   (let [[get-handler delete-handler]
         (swagger/swagger->handlers {"paths" {"/pets/{id}"