Non-success status codes should not always result in a thrown excepti…

…on (#30)

* Add a throw? flag, defaults to true, so that non-success status do not automatically throw an exception

project.clj

@@ -9,29 +9,29 @@
                              :password :env/clojars_passwd
                              :sign-releases false}]]
 
-  :plugins [[lein-cljfmt "0.6.4" :exclusions [org.clojure/clojure]]
+  :plugins [[lein-cljfmt "0.9.2" :exclusions [org.clojure/clojure]]
             [lein-nsorg "0.3.0" :exclusions [org.clojure/clojure]]
-            [lein-ancient "0.6.14" :exclusions [commons-logging com.fasterxml.jackson.core/jackson-databind com.fasterxml.jackson.core/jackson-core]]]
+            [lein-ancient "0.7.0" :exclusions [commons-logging com.fasterxml.jackson.core/jackson-databind com.fasterxml.jackson.core/jackson-core]]]
 
-  :dependencies [[org.clojure/clojure "1.10.3"]
-                 [cheshire "5.10.1"]
-                 [clj-commons/clj-yaml "0.7.107"]
-                 [http-kit "2.5.3"]
+  :dependencies [[org.clojure/clojure "1.11.3"]
+                 [cheshire "5.13.0"]
+                 [clj-commons/clj-yaml "1.0.27"]
+                 [http-kit "2.8.0"]
                  [nubank/clj-github-app "0.2.1"]
-                 [nubank/state-flow "5.14.0"]
-                 [clj-commons/fs "1.6.310"]
+                 [nubank/state-flow "5.17.0"]
+                 [clj-commons/fs "1.6.311"]
                  [ring/ring-codec "1.2.0"]]
 
   :cljfmt {:indents {flow       [[:block 1]]
                      assoc-some [[:block 0]]}}
 
   :profiles {:dev {:plugins [[lein-project-version "0.1.0"]]
-                   :dependencies [[ch.qos.logback/logback-classic "1.3.0-alpha4" :exclusions [com.sun.mail/javax.mail]]
+                   :dependencies [[ch.qos.logback/logback-classic "1.3.0" :exclusions [com.sun.mail/javax.mail]]
                                   [org.clojure/test.check "1.1.1"]
-                                  [nubank/matcher-combinators "3.3.1" :exclusions [mvxcvi/puget commons-codec]]
-                                  [tortue/spy "2.9.0"]
+                                  [nubank/matcher-combinators "3.9.1" :exclusions [mvxcvi/puget commons-codec]]
+                                  [tortue/spy "2.14.0"]
                                   [http-kit.fake "0.2.2"]
-                                  [metosin/reitit-core "0.5.15"]
+                                  [metosin/reitit-core "0.7.0"]
                                   [dev.nubank/clj-github-mock "0.2.0"]]}}
 
   :aliases {"coverage" ["cloverage" "-s" "coverage"]

src/clj_github/httpkit_client.clj

@@ -29,13 +29,35 @@
   (or (get-in response [:headers :content-type])
       (get-in response [:headers "Content-Type"])))
 
-(defn request [client req-map]
-  (let [response @(httpkit/request (prepare client req-map))]
-    (if (success-codes (:status response))
-      (update response :body (partial parse-body (content-type response)))
+(defn request
+  "Sends a synchronous request to GitHub, largely as a wrapper around HTTP Kit.
+
+  In addition to normal HTTP Kit keys in the request map, two additional keys are added.
+
+  :path - used to create the :url key for HTTP kit; this is the path relative to https://api.github.com.
+
+  :throw? - if true (the default), then non-success status codes (codes outside the range of 200 to 204)
+    result in a thrown exception. If set to false, then the response is returned, regardless and the
+    caller can decide what to do with failure statuses."
+  [client req-map]
+  (let [{:keys [throw?]
+         :or   {throw? true}} req-map
+        {:keys [error status body opts] :as response} @(httpkit/request (prepare client req-map))]
+    (cond
+      error
+      (throw (ex-info "Failure sending request to GitHub"
+                      {:opts opts}
+                      error))
+
+      (and throw?
+           (not (success-codes status)))
       (throw (ex-info "Request to GitHub failed"
-                      {:response (select-keys response [:status :body])}
-                      (:error response))))))
+                      {:response (select-keys response [:status :body])
+                       :opts     opts}
+                      error))
+
+      :else
+      (assoc response :body (parse-body (content-type response) body)))))
 
 (defn new-client [{:keys [app-id private-key token org] :as opts}]
   (cond

test/clj_github/httpkit_client_test.clj

@@ -31,11 +31,18 @@
                                   "Content-Type" "application/json"}}
                        {:status 200}]
         (is (match? {:status 200} (sut/request client {})))))
-    (testing "it throws an exception when status code is not succesful"
+    (testing "it throws an exception when status code is not successful"
       (with-fake-http [{} {:status 400}]
         (is (thrown-with-msg? clojure.lang.ExceptionInfo #"(?i)Request to GitHub failed"
                               (sut/request client {})))))
-    ;; in case of lower level errors, e.g. DNS lookup failue there will not be a status code
+    (testing "it does not throw with a failure status if so enabled"
+      (with-fake-http [{:headers {"Authorization" "Bearer token"
+                                  "Content-Type"  "application/json"}}
+                       {:status 404}]
+                      (is (match?
+                            {:status 404}
+                            (sut/request client {:throw? false})))))
+    ;; in case of lower level errors, e.g. DNS lookup failure there will not be a status code
     (testing "it throws an exception when there is no status code"
       (with-fake-http [{} {:status nil}]
         (is (thrown-with-msg? clojure.lang.ExceptionInfo #"(?i)Request to GitHub failed"
@@ -44,7 +51,7 @@
       (let [cause (Exception. "Exception for testing purpose")]
         (with-fake-http [{} {:error cause :status nil}]
           (let [e (try (sut/request client {}) (catch Exception e e))]
-            (is (re-matches #"(?i)Request to GitHub failed" (.getMessage e)))
+            (is (re-matches #"(?i)Failure sending request to GitHub" (.getMessage e)))
             (is (= cause (.getCause e)))))))
     (testing "url path contains special character `|`"
       (with-fake-http [{:url "https://api.github.com/test%7Ctest"}