Repository: oliyh/martian
Pull request #70 (2019-09-11 17:08:48)

Handle free form objects
At the moment if you have a swagger property like this :

```clojure
{:Pet {:type  "object" :properties {}}}
```

The Plumatic schema that you get is:
```clojure
{:Pet {}}
```
This only allows an empty map as a posible value, which is quite restrictive and probably not very useful.

So, this PR changes the existing code so that if you have no explicit properties, it will produce the following to allow any value:

```clojure
{:Pet s/Any}
```

Happy to hear your thoughts on the approach and implementation. Relevant swagger docs for free form objects https://swagger.io/docs/specification/data-models/data-types#free-form.

Oliver


oliyh: Hi Oliver,

Thanks for the PR. Interesting, my initial thought was that it's done the right thing - the swagger definition is saying that it's an object (map) with no keys.

I would imagine Swagger has a more explicit way of describing a blob, I would want to find out what that was first and making sure Martian supported it before accepting this change. I'll try to find that out soon and reply, unless you have time first.
oliyh: By the way how much control over this do you have on the server side, are you using pedestal-api or compojure-api or something else that uses metosin's swagger generation?
omartell: Thanks for the reply Oliy. 

The server is using compojure-api. Basically the schema on the server looks like this:

```clojure
{:conversion          {s/Any s/Any}
  (s/optional-key :contact) {s/Any s/Any}
  (s/optional-key :account) {s/Any s/Any}}
```

And the only way I can make martians interact with the endpoint is by submitting:

```clojure
{:conversion {}}
```

omartell: The only relevant swagger information I could find was the free form object section on here https://swagger.io/docs/specification/data-models/data-types#free-form. Maybe that helps.
omartell: For more context this is an example swagger schema with those properties:

```clojure
{:swagger     "2.0",
 :info        {:title "Swagger API", :version "0.0.1"},
 :produces    ["application/json"
               "application/transit+msgpack"
               "application/transit+json"
               "application/edn"],
 :consumes    ["application/json"
               "application/transit+msgpack"
               "application/transit+json"
               "application/edn"],
 :basePath    "/",
 :paths       {:api/create-pet {:post {:operationId "create-pet",
                                       :parameters  [{:in          "body",
                                                      :name        "Body50766",
                                                      :description "",
                                                      :required    true,
                                                      :schema      {:$ref "#/definitions/Body50766"}}],
                                       :responses   {:default {:description ""}}}}},
 :definitions {:Body50766      {:type                 "object",
                                :properties           {:name  {:$ref "#/definitions/Body50766Name"},
                                                       :owner {:$ref "#/definitions/Body50766Owner"},
                                                       :age   {:$ref "#/definitions/Body50766Age"}},
                                :additionalProperties false,
                                :required             ["name"]},
               :Body50766Name  {:type "object", :additionalProperties {}},
               :Body50766Owner {:type "object", :additionalProperties {}},
               :Body50766Age   {:type "object", :additionalProperties {}}}}
```

My thinking was that the issue was within martian as the previous swagger schema works fine through the swagger UI. Swagger is basically allowing to add any kv to `name`, `owner` and `age`, but martian is more restrictive by only allowing the empty map `{}`. Perhaps the schema I suggested before `s/Any` is not right, and it should be `{s/Any s/Any}` to model an object as the swagger spec does. Ideally we should look at `additionalProperties` to support the free form object.
oliyh: Yes, your proposal sounds good, and the swagger schema looks as I expected it to as well. Also, we should merge `{s/Any s/Any}` with whatever keys are defined.
omartell: Hey @oliyh, sorry for the delay. I just pushed an update, let me know what you think of the code and if you think there's anything missing/could be improved.
oliyh: Hi @omartell 

Thanks for this, it looks great - just one small point I have questioned, could you check if my assumption is correct?

Cheers
omartell: @oliyh just pushed an update removing the `or`.

Patch:
diff --git a/core/project.clj b/core/project.clj
index 2084e42..1f8208a 100644
--- a/core/project.clj
+++ b/core/project.clj
@@ -26,7 +26,7 @@
                    :dependencies [[org.clojure/clojure "1.10.1"]
                                   [org.clojure/clojurescript "1.10.520"]
                                   [org.clojure/tools.reader "1.2.2"]
-                                  [cider/piggieback "0.3.6"]
+                                  [cider/piggieback "0.4.1"]
                                   [org.clojure/tools.nrepl "0.2.13"]]
                    :repl-options {:nrepl-middleware [cider.piggieback/wrap-cljs-repl]}}}
   :aliases {"test" ["do" ["clean"] ["test"] ["doo" "nashorn" "test" "once"]]}
diff --git a/core/src/martian/core.cljc b/core/src/martian/core.cljc
index a9d84ba..b4fd322 100644
--- a/core/src/martian/core.cljc
+++ b/core/src/martian/core.cljc
@@ -22,7 +22,9 @@
 
 (defn- enrich-handler [handler]
   (-> handler
-      (assoc :parameter-aliases (let [ks (schema/parameter-keys (map handler parameter-schemas))]
+      (assoc :parameter-aliases (let [ks (->> (map handler parameter-schemas)
+                                              schema/parameter-keys
+                                              (remove #(= % s/Any)))]
                                   (zipmap (map ->kebab-case-keyword ks) ks)))))
 
 (defn- concise->handlers [concise-handlers global-produces global-consumes]
diff --git a/core/src/martian/schema.cljc b/core/src/martian/schema.cljc
index f9d330f..d0ffbe9 100644
--- a/core/src/martian/schema.cljc
+++ b/core/src/martian/schema.cljc
@@ -110,7 +110,7 @@
 
 (defn make-schema
   "Takes a swagger parameter and returns a schema"
-  [definitions {:keys [name required type enum schema properties $ref items] :as param}]
+  [definitions {:keys [name required type enum schema properties $ref items additionalProperties] :as param}]
 
   (cond
     $ref
@@ -130,7 +130,8 @@
               [(schema-type definitions (assoc (:items schema) :required true))]
 
               (= "object" type)
-              (schemas-for-parameters definitions (map (fn [[name p]] (assoc p :name name)) properties))
+              (cond-> (schemas-for-parameters definitions (map (fn [[name p]] (assoc p :name name)) properties))
+                additionalProperties (assoc (s/optional-key s/Any) s/Any))
 
               :else
               (schema-type definitions param))


Test patch:
diff --git a/core/test/martian/schema_test.cljc b/core/test/martian/schema_test.cljc
index dd81d41..2862dae 100644
--- a/core/test/martian/schema_test.cljc
+++ b/core/test/martian/schema_test.cljc
@@ -5,6 +5,26 @@
             #?(:clj [clojure.test :refer :all]
                :cljs [cljs.test :refer-macros [deftest testing is run-tests]])))
 
+(deftest free-form-object-test
+  (let [schema (schema/make-schema {:Body55523       {:type                 "object"
+                                                      :properties           {:age    {:$ref "#/definitions/Body55523Age"}
+                                                                             :name   {:$ref "#/definitions/Body55523Name"}
+                                                                             :colour {:$ref "#/definitions/Body55523Colour"}}
+                                                      :additionalProperties false
+                                                      :required             ["name"]}
+                                    :Body55523Age    {:type "object" :additionalProperties {}}
+                                    :Body55523Name   {:type "object" :additionalProperties {}}
+                                    :Body55523Colour {:type "object" :additionalProperties {}}}
+                                   {:in          "body"
+                                    :name        "Body55523"
+                                    :description ""
+                                    :required    true
+                                    :schema      {:$ref "#/definitions/Body55523"}})]
+    (is (= {(s/optional-key :name)   (s/maybe {(s/optional-key s/Any) s/Any})
+            (s/optional-key :age)    (s/maybe {(s/optional-key s/Any) s/Any})
+            (s/optional-key :colour) (s/maybe {(s/optional-key s/Any) s/Any})}
+           schema))))
+
 (deftest enum-test
   (is (= (s/enum "desc" "asc")
          (schema/make-schema {} {:name "sort"
@@ -112,7 +132,27 @@
               :in "path"
               :required true
               :type "integer"}]))))
-
+  (testing "object with additional properties"
+    (is (= {:Pet {:CamelBodyKey          s/Int
+                  (s/optional-key s/Any) s/Any}}
+           (schema/schemas-for-parameters
+            {:Pet {:type                 "object"
+                   :additionalProperties true
+                   :properties           {:CamelBodyKey {:type     "integer"
+                                                         :required true}}}}
+            [{:name     "Pet"
+              :in       "path"
+              :required true
+              :schema   {:$ref "#/definitions/Pet"}}]))))
+  (testing "object with empty properties"
+    (is (= {:Pet {}}
+           (schema/schemas-for-parameters
+            {:Pet {:type       "object"
+                   :properties {}}}
+            [{:name     "Pet"
+              :in       "path"
+              :required true
+              :schema   {:$ref "#/definitions/Pet"}}]))))
   (testing "body params"
     (is (= {:Pet {:CamelBodyKey s/Int}}