Skip to content

Commit

Permalink
(maint) replace wrappers with shared implementation
Browse files Browse the repository at this point in the history
This removes the use of `verify-accepts-json` and `verify-content-type`
to leverage the ring-middleware additions in 2.0.3 of ring-middleware.

Corresponding tests were removed as well.
  • Loading branch information
jonathannewman authored and austb committed Jul 23, 2024
1 parent 220b580 commit a735526
Show file tree
Hide file tree
Showing 6 changed files with 14 additions and 86 deletions.
7 changes: 4 additions & 3 deletions src/puppetlabs/puppetdb/http/command.clj
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@
[clojure.core.async :as async]
[puppetlabs.kitchensink.core :as kitchensink]
[puppetlabs.comidi :as cmdi]
[puppetlabs.i18n.core :refer [trs tru]])
[puppetlabs.i18n.core :refer [trs tru]]
[puppetlabs.ring-middleware.core :as rmc])
(:import
(clojure.lang ExceptionInfo)
(java.net HttpURLConnection)
Expand Down Expand Up @@ -317,8 +318,8 @@
add-received-param ;; must be (temporally) after wrap-with-request-params-validation
wrap-with-request-params-validation
wrap-with-request-normalization
mid/verify-accepts-json
(mid/verify-content-type ["application/json"])
rmc/wrap-accepts-json
(rmc/wrap-content-type ["application/json"])
(mid/verify-content-encoding utils/supported-content-encodings)
(mid/fail-when-payload-too-large reject-large-commands? max-command-size)
(mid/wrap-with-metrics (atom {}) http/leading-uris)
Expand Down
9 changes: 4 additions & 5 deletions src/puppetlabs/puppetdb/http/server.clj
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,12 @@
wrap-with-metrics
wrap-with-illegal-argument-catch
wrap-with-exception-handling
verify-accepts-json
verify-content-type
make-pdb-handler
verify-sync-version]]
[puppetlabs.comidi :as cmdi]
[puppetlabs.puppetdb.http.handlers :as handlers]
[puppetlabs.i18n.core :refer [tru]]))
[puppetlabs.i18n.core :refer [tru]]
[puppetlabs.ring-middleware.core :as rmc]))

(defn- refuse-retired-api
[version]
Expand Down Expand Up @@ -70,8 +69,8 @@
(fn [req]
(let [handler (-> (make-pdb-handler routes identity)
wrap-with-illegal-argument-catch
verify-accepts-json
(verify-content-type ["application/json"])
rmc/wrap-accepts-json
(rmc/wrap-content-type ["application/json"])
verify-sync-version
(wrap-with-metrics (atom {}) http/leading-uris)
(wrap-with-globals get-shared-globals)
Expand Down
5 changes: 3 additions & 2 deletions src/puppetlabs/puppetdb/meta.clj
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
[puppetlabs.puppetdb.meta.version :as v]
[puppetlabs.puppetdb.time :refer [now]]
[puppetlabs.puppetdb.config :as conf]
[puppetlabs.ring-middleware.core :as rmc]
[puppetlabs.comidi :as cmdi]
[bidi.schema :as bidi-schema]
[puppetlabs.puppetdb.schema :as pls]
Expand Down Expand Up @@ -49,7 +50,7 @@
(http/error-response (tru "Could not find version") 404))))

(catch java.io.IOException e
(log/debug e (trs "Error when checking for latest version") )
(log/debug e (trs "Error when checking for latest version"))
(http/error-response
(tru "Error when checking for latest version: {0}" (.getMessage e))))))))

Expand All @@ -69,5 +70,5 @@
[get-shared-globals config]
(-> (meta-routes get-shared-globals config)
mid/make-pdb-handler
mid/verify-accepts-json
rmc/wrap-accepts-json
mid/validate-no-query-params))
39 changes: 0 additions & 39 deletions src/puppetlabs/puppetdb/middleware.clj
Original file line number Diff line number Diff line change
Expand Up @@ -162,21 +162,6 @@
(http/error-response (tru "An unexpected error occurred")
HttpURLConnection/HTTP_INTERNAL_ERROR)))))

(defn verify-accepts-content-type
"Ring middleware that requires a request for the wrapped `app` to accept the
provided `content-type`. If the content type isn't acceptable, a 406 Not
Acceptable status is returned, with a message informing the client it must
accept the content type."
[app content-type]
{:pre [(string? content-type)]}
(fn [{:keys [headers] :as req}]
(if (http/acceptable-content-type
content-type
(headers "accept"))
(app req)
(http/error-response (tru "must accept {0}" content-type)
HttpURLConnection/HTTP_NOT_ACCEPTABLE))))

(defn verify-content-encoding
"Verification for the specified list of content-encodings."
[app allowed-encodings]
Expand All @@ -192,22 +177,6 @@
content-encoding)
HttpURLConnection/HTTP_UNSUPPORTED_TYPE)))))

(defn verify-content-type
"Verification for the specified list of content-types."
[app content-types]
{:pre [(coll? content-types)
(every? string? content-types)]}
(fn [{:keys [headers] :as req}]
(if (= (:request-method req) :post)
(let [content-type (headers "content-type")
mediatype (if (nil? content-type) nil
(kitchensink/base-type content-type))]
(if (or (nil? mediatype) (some #{mediatype} content-types))
(app req)
(http/error-response (tru "content type {0} not supported" mediatype)
HttpURLConnection/HTTP_UNSUPPORTED_TYPE)))
(app req))))

(def params-schema {(s/optional-key :optional) [s/Str]
(s/optional-key :required) [s/Str]})

Expand Down Expand Up @@ -248,14 +217,6 @@
[app]
(validate-query-params app {}))

(def verify-accepts-json
"Ring middleware which requires a request for `app` to accept
application/json as a content type. If the request doesn't accept
application/json, a 406 Not Acceptable status is returned with an error
message indicating the problem."
(fn [app]
(verify-accepts-content-type app "application/json")))

(def http-metrics-registry (get-in metrics/metrics-registries [:http :registry]))

(defn wrap-with-metrics
Expand Down
4 changes: 2 additions & 2 deletions test/puppetlabs/puppetdb/http/nodes_test.clj
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
query-response]]
[puppetlabs.puppetdb.testutils.log :refer [notable-pdb-event?]]
[puppetlabs.puppetdb.testutils.nodes :refer [store-example-nodes]]
[puppetlabs.trapperkeeper.testutils.logging :as tk-log :refer [with-log-suppressed-unless-notable]])
[puppetlabs.trapperkeeper.testutils.logging :refer [with-log-suppressed-unless-notable]])
(:import
(java.net HttpURLConnection)))

Expand Down Expand Up @@ -675,7 +675,7 @@
(assoc-in [:headers "content-type"] "application/x-www-form-urlencoded")
*app*)]
(is (= 415 status))
(is (= "content type application/x-www-form-urlencoded not supported" body))))
(is (re-find #"content-type application/x-www-form-urlencoded is not a supported" body))))
(testing "content type application/json should be accepted"
(let [{:keys [status] :as response}
(-> (tu/query-request :post endpoint ["extract" "certname" ["=" "certname" "puppet.example.com"]])
Expand Down
36 changes: 1 addition & 35 deletions test/puppetlabs/puppetdb/middleware_test.clj
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,9 @@
merge-param-specs
validate-query-params
verify-content-encoding
verify-content-type
wrap-cert-authn
wrap-with-certificate-cn
wrap-with-metrics] ]
wrap-with-metrics]]
[clojure.test :refer :all]
[puppetlabs.puppetdb.testutils :refer [temp-file]]
[puppetlabs.ssl-utils.core :refer [get-cn-from-x509-certificate]]
Expand Down Expand Up @@ -128,39 +127,6 @@
:headers {"Content-Type" http/error-response-content-type}
:body "Unsupported query parameter 'wazzup'"})))))

(deftest verify-content-type-test
(testing "with content-type of application/json"
(let [test-req {:request-method :post
:content-type "application/json"
:headers {"content-type" "application/json"}}]

(testing "should succeed with matching content type"
(let [wrapped-fn (verify-content-type identity ["application/json"])]
(is (= (wrapped-fn test-req) test-req))))

(testing "should fail with no matching content type"
(let [wrapped-fn (verify-content-type identity ["application/bson" "application/msgpack"])]
(is (= (wrapped-fn test-req)
{:status 415
:headers {"Content-Type" http/error-response-content-type}
:body "content type application/json not supported"}))))))

(testing "with content-type of APPLICATION/JSON"
(let [test-req {:content-type "APPLICATION/JSON"
:headers {"content-type" "APPLICATION/JSON"}}]

(testing "should succeed with matching content type"
(let [wrapped-fn (verify-content-type identity ["application/json"])]
(is (= (wrapped-fn test-req) test-req))))))

(testing "with content-type of application/json;parameter=foo"
(let [test-req {:content-type "application/json;parameter=foo"
:headers {"content-type" "application/json;parameter=foo"}}]

(testing "should succeed with matching content type"
(let [wrapped-fn (verify-content-type identity ["application/json"])]
(is (= (wrapped-fn test-req) test-req)))))))

(deftest verify-content-encoding-test
(testing "with content-encoding of gzip"
(let [test-req {:request-method :post
Expand Down

0 comments on commit a735526

Please sign in to comment.