Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Non-success status codes should not always result in a thrown exception #30

Merged
merged 4 commits into from
Jun 11, 2024
Merged

Non-success status codes should not always result in a thrown exception #30

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
35540c9
Add a throw? flag, defaults to true, so that non-success status do no…
hlship May 24, 2024
c69505a
Use local symbol
hlship May 24, 2024
82dd9bc
Back down logback-classic version
hlship May 24, 2024
793a736
Tweak documentation on function
hlship Jun 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 12 additions & 12 deletions project.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the version bumps essential to this PR? Otherwise separate them out at least to a separate commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the cost of getting a commit merged is too high, you tend to put a lot into one PR. These changes are not essential, but I tend to update dependencies as hygiene, when possible.

[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"]
Expand Down
34 changes: 28 additions & 6 deletions src/clj_github/httpkit_client.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 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."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 for adding better docs.

Maybe specific what non-success status codes are, just to be sure. You can refer to the success-codes function.

Also add that in case of success the response body is conditionally parsed into json as returned as a clojure data structure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding docs is what I do ™️.

[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
Expand Down
13 changes: 10 additions & 3 deletions test/clj_github/httpkit_client_test.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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})))))
Copy link
Contributor

@phmeier-nubank phmeier-nubank Jun 11, 2024 •
edited by hlship
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should also verify that the headers and body are not mangled.

;; 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"
Expand All @@ -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"}
Expand Down
Loading