Repository: clj-commons/aleph
Pull request #679 (2023-02-27 08:51:57)

Add wrap-validation middleware based on Malli
@arnaudgeiser What do you think of this?

Also moves the schemas to a common aleph.http.schema ns, so they can be shared.

Not sure we need each individual var schema just yet, so I've hidden the ns from cljdoc for now.

KingMob: We should probably tighten the schemas soon. I've had bad experiences in the past using spec, when people let specs be loose for too long, they became concerned that locking them down would break something (E.g., one team only accepts ports as numbers, but another team also accepts them as strings which it parses...)
KingMob: ...and I'm already reminded of why `:pedantic :abort` was a huge pain. It lead to people excluding things at random because they just want their build to succeed.
arnaudgeiser: > @arnaudgeiser What do you think of this?

I'm totally fine with using Malli instead of spec, I don't have strong opinions about them.
However, I'm still wondering if the overhead of validating spec is worth. Actually, only the presence of the  `request-method` is strongly required and currently leads to intelligible error.

> We should probably tighten the schemas soon. I've had bad experiences in the past using spec, when people let specs be loose for too long, they became concerned that locking them down would break something (E.g., one team only accepts ports as numbers, but another team also accepts them as strings which it parses...)

Isn't it too late already?

> ...and I'm already reminded of why :pedantic :abort was a huge pain. It lead to people excluding things at random because they just want their build to succeed.

You're a bit harsh here. :smile: 
We don't add dependencies everyday and I prefer having errors at build time instead of at runtime with libraries incompatibilities. It might be annoying, but I would say it's manageable on a project like Aleph.

In the current example, I'm wondering if we should also pin the `org.clojure/clojure` like metosin does for malli.
KingMob: Let's take a step back and decide what problem we actually want to solve. 

The original issue (#162) was lvh complaining about the error message he got when he forgot the method key.

What else (if anything) do we want out of validation? Just to return some better error messages for bad requests? To lock down the request format users submit to the client? To capture our current loose schemas, but then transform the input into something more stringent for Aleph's use? Get kondo support for our own use?

Personally, I:
- am slightly in favor of better error messages for users
- think locking it down will cause breakage
- think transforming is nice to have, but not important
- think kondo support would help a lot, given how kondo-unfriendly aleph and manifold are
arnaudgeiser: > Just to return some better error messages for bad requests?

For me, that should be the focus.
KingMob: @DerGuteMoritz Any thoughts on the goal of validation?
DerGuteMoritz: @KingMob Thanks for providing context of the original issue, I was missing that :pray: 

> Personally, I:
> 
> * am slightly in favor of better error messages for users

Looking at the original issue, that definitely should be the priority. Question is whether this alone justifies pulling in malli, especially if it's only for this particular API. We'd end up with a mixed bag of schema-based and hand-rolled validation (and errors). I feel like adopting a schema-based approach should be done in a more holistic fashion. For https://github.com/clj-commons/aleph/issues/162 I now lean towards a minimally invasive patch with hand-rolled validation.

> * think locking it down will cause breakage

Agreed.

> * think transforming is nice to have, but not important

Agreed.

> * think kondo support would help a lot, given how kondo-unfriendly aleph and manifold are

Indeed. As mentioned above, I think doing this via malli is worthwhile since it will also give us opt-in runtime validation (which could then supercede the hand-rolled one) and generative testing support at the same time. Ideally then for all of Aleph's public API.
KingMob: I'd like to pull in Malli as a vote for the future, then, and because kondo support would help me, personally, a lot. We might not need it for any given small PR, but if it's already a dep, we can start to build and rely on it.

Patch:
diff --git a/deps.edn b/deps.edn
index 78fa004..0bf9253 100644
--- a/deps.edn
+++ b/deps.edn
@@ -25,7 +25,9 @@
   io.netty/netty-handler {:mvn/version "4.1.89.Final"},
   io.netty/netty-handler-proxy {:mvn/version "4.1.89.Final"},
   io.netty/netty-resolver {:mvn/version "4.1.89.Final"},
-  io.netty/netty-resolver-dns {:mvn/version "4.1.89.Final"}},
+  io.netty/netty-resolver-dns {:mvn/version "4.1.89.Final"},
+  metosin/malli
+  {:mvn/version "0.10.4", :exclusions [org.clojure/clojure]}},
  :aliases
  {:lein2deps
   {:deps
diff --git a/project.clj b/project.clj
index 26ed4b9..0d45e9e 100644
--- a/project.clj
+++ b/project.clj
@@ -22,7 +22,8 @@
                  [io.netty/netty-handler ~netty-version]
                  [io.netty/netty-handler-proxy ~netty-version]
                  [io.netty/netty-resolver ~netty-version]
-                 [io.netty/netty-resolver-dns ~netty-version]]
+                 [io.netty/netty-resolver-dns ~netty-version]
+                 [metosin/malli "0.10.4" :exclusions [org.clojure/clojure]]]
   :profiles {:dev  {:dependencies [[org.clojure/clojure "1.11.0"]
                                    [criterium "0.4.6"]
                                    [cheshire "5.10.0"]
diff --git a/src/aleph/http/client_middleware.clj b/src/aleph/http/client_middleware.clj
index a9c45d4..c7083e0 100644
--- a/src/aleph/http/client_middleware.clj
+++ b/src/aleph/http/client_middleware.clj
@@ -4,6 +4,7 @@
   (:require
     ;; leave this dependency to make sure that HeaderMap is already compiled
     [aleph.http.core :as http]
+    [aleph.http.schema :as schema]
     [clj-commons.byte-streams :as bs]
     [clojure.edn :as edn]
     [clojure.string :as str]
@@ -920,9 +921,15 @@
       (opt req :save-request)
       (assoc :aleph/request req'))))
 
+(defn ^:no-doc wrap-validation [req]
+  (when-not (schema/valid-request? req)
+    (throw (IllegalArgumentException. (format "Invalid spec: %s" (schema/explain-request req)))))
+  req)
+
 (def ^:no-doc default-middleware
   [wrap-method
    wrap-url
+   wrap-validation
    wrap-nested-params
    wrap-query-params
    wrap-form-params
diff --git a/src/aleph/http/schema.clj b/src/aleph/http/schema.clj
new file mode 100644
index 0000000..8b2cd43
--- /dev/null
+++ b/src/aleph/http/schema.clj
@@ -0,0 +1,47 @@
+(ns ^:no-doc aleph.http.schema
+  (:require [malli.core :as m]))
+
+(def server-port [:maybe :int])
+(def server-name [:maybe :string])
+(def remote-addr [:maybe :string])
+(def uri [:maybe :string])
+(def query-string [:maybe :string])
+(def scheme [:enum :http :https])
+(def request-method :keyword)
+(def content-type [:maybe [:or :string :keyword]])
+(def content-length [:maybe :int])
+(def character-encoding [:maybe :string])
+
+(def ring-request [:map
+                   [:request-method request-method]
+                   [:server-port {:optional true} server-port]
+                   [:server-name {:optional true} server-name]
+                   [:remote-addr {:optional true} remote-addr]
+                   [:uri {:optional true} uri]
+                   [:query-string {:optional true} query-string]
+                   [:scheme {:optional true} scheme]
+                   [:content-type {:optional true} content-type]
+                   [:content-length {:optional true} content-length]
+                   [:character-encoding {:optional true} character-encoding]
+                   [:body {:optional true} :any]])
+
+(def valid-request? (m/validator ring-request))
+(def explain-request (m/explainer ring-request))
+
+
+
+
+
+(comment
+
+  (map #(m/validate server-port %) ["bingo" nil 123])
+
+  (m/validate ring-request
+              {:server-port 80
+               :server-name "www.example.com"
+               :remote-addr "127.0.0.1"
+               :uri "/blah/blah"
+               :query-string "?foo=bar"
+               :scheme :https
+               :request-method :get})
+  )


Test patch:
diff --git a/test/aleph/http/client_middleware_test.clj b/test/aleph/http/client_middleware_test.clj
index 00ff0e6..58c613f 100644
--- a/test/aleph/http/client_middleware_test.clj
+++ b/test/aleph/http/client_middleware_test.clj
@@ -1,7 +1,10 @@
 (ns aleph.http.client-middleware-test
   (:require
    [aleph.http.client-middleware :as middleware]
-   [clojure.test :as t :refer [deftest is]])
+   [aleph.http.schema :as schema]
+   [clojure.test :as t :refer [deftest is]]
+   [malli.core :as m]
+   [malli.generator :as mg])
   (:import
    (java.net URLDecoder)))
 
@@ -46,7 +49,8 @@
         (middleware/coerce-form-params {:form-params {:foo :bar}}))))
 
 (deftest test-nested-query-params
-  (let [req {:query-params {:foo {:bar "baz"}}}
+  (let [req {:request-method :get
+             :query-params {:foo {:bar "baz"}}}
         {:keys [query-string]} (reduce #(%2 %1) req middleware/default-middleware)]
     (is (= "foo[bar]=baz" (URLDecoder/decode query-string)))))
 
@@ -131,12 +135,16 @@
   (URLDecoder/decode (req->body-raw req)))
 
 (deftest test-nested-params
-  (is (= "foo[bar]=baz" (req->query-string {:query-params {:foo {:bar "baz"}}})))
-  (is (= "foo[bar]=baz" (req->query-string {:query-params {:foo {:bar "baz"}}
+  (is (= "foo[bar]=baz" (req->query-string {:request-method :get
+                                            :query-params {:foo {:bar "baz"}}})))
+  (is (= "foo[bar]=baz" (req->query-string {:request-method :get
+                                            :query-params {:foo {:bar "baz"}}
                                             :content-type :json})))
-  (is (= "foo[bar]=baz" (req->query-string {:query-params {:foo {:bar "baz"}}
+  (is (= "foo[bar]=baz" (req->query-string {:request-method :get
+                                            :query-params {:foo {:bar "baz"}}
                                             :ignore-nested-query-string false})))
-  (is (= "foo={:bar \"baz\"}" (req->query-string {:query-params {:foo {:bar "baz"}}
+  (is (= "foo={:bar \"baz\"}" (req->query-string {:request-method :get
+                                                  :query-params {:foo {:bar "baz"}}
                                                   :ignore-nested-query-string true})))
   (is (= "foo[bar]=baz" (req->body-decoded {:method :post
                                             :form-params {:foo {:bar "baz"}}})))
@@ -169,3 +177,23 @@
         (middleware/generate-query-string {:name ["John" "Mark"]} nil :array)))
   (is (= "name[0]=John&name[1]=Mark"
         (middleware/generate-query-string {:name ["John" "Mark"]} nil :indexed))))
+
+
+(deftest test-wrap-validation
+  (doseq [req (mg/sample schema/ring-request)]
+    (is (middleware/wrap-validation req)))
+
+  (is (thrown-with-msg?
+        IllegalArgumentException
+        #"Invalid spec.*:in \[:request-method\].*:type :malli.core/missing-key"
+        (middleware/wrap-validation {})))
+  (is (thrown-with-msg?
+        IllegalArgumentException
+        #"Invalid spec.*:in \[:content-length\].*:value \"10\""
+        (middleware/wrap-validation {:request-method :post
+                                     :content-length "10"})))
+  (is (thrown-with-msg?
+        IllegalArgumentException
+        #"Invalid spec.*:in \[:content-type\].*:value 10"
+        (middleware/wrap-validation {:request-method :post
+                                     :content-type 10}))))