From 6f73bcd9614440403eb20f0e6b6d323adc14781e Mon Sep 17 00:00:00 2001 From: Philipp Meier Date: Fri, 2 Oct 2015 14:42:37 +0200 Subject: [PATCH 1/2] Fix handler invocation: status, messages etc. Closes #76 * Fix missing response from handle-options * Status can be overriden by decisions * If no handler is given :message is looked up in context --- CHANGES.markdown | 6 ++++++ src/liberator/core.clj | 22 ++++++++++------------ test/test_errors.clj | 6 +++--- test/test_execution_model.clj | 21 +++++++++++++++++---- 4 files changed, 36 insertions(+), 19 deletions(-) diff --git a/CHANGES.markdown b/CHANGES.markdown index 7ddc540..73d5a49 100644 --- a/CHANGES.markdown +++ b/CHANGES.markdown @@ -6,7 +6,13 @@ * Values can be added to the context at the beginning of the execution flow using the :initialize-context action. +* If no handler is specified, the key :message is looked up from the + context to create a default response. +## Bugs fixed + +* #76 Nullpointer with post options +* Allow decisions to override status in context * JSON body can be parsed into :request-entity by setting representation/parse-request-entity for :processable? ## Bugs fixed diff --git a/src/liberator/core.clj b/src/liberator/core.clj index 0e3dcab..d355b7b 100644 --- a/src/liberator/core.clj +++ b/src/liberator/core.clj @@ -136,7 +136,7 @@ combine ;; Status - {:status status} + {:status (:status context)} ;; ETags (when-let [etag (gen-etag context)] @@ -147,7 +147,7 @@ {:headers {"Last-Modified" (http-date last-modified)}}) ;; 201 created required a location header to be send - (when (#{201 301 303 307} status) + (when (#{201 301 303 307} (:status context)) (if-let [f (or (get context :location) (get resource :location))] {:headers {"Location" (str ((make-function f) context))}})) @@ -171,17 +171,14 @@ (set-header-maybe "Vary" (build-vary-header representation)))} ;; Finally the result of the handler. We allow the handler to ;; override the status and headers. - - - (let [as-response (:as-response resource)] - (as-response - (if-let [handler (get resource (keyword name))] - (handler context) - (get context :message)) - context)))))] + (when-let [result (if-let [handler (get resource (keyword name))] + (handler context) + (get context :message))] + (let [as-response (:as-response resource)] + (as-response result context))))))] (cond (or (= :options (:request-method request)) (= 405 (:status response))) - (merge-with merge + (merge-with combine {:headers (build-options-headers resource)} response) (= :head (:request-method request)) @@ -563,7 +560,8 @@ :patch-content-types [] ;; The default function used extract a ring response from a handler's response - :as-response as-response + :as-response (fn [data ctx] + (when data (as-response data ctx))) ;; Directives :available-media-types [] diff --git a/test/test_errors.clj b/test/test_errors.clj index 3e886e5..2d6cbe3 100644 --- a/test/test_errors.clj +++ b/test/test_errors.clj @@ -55,7 +55,7 @@ (fact resp => (content-type #"text/plain;charset=.*")))) (facts "custom exception handler not invoked if handler throws exception" - (let [res (resource :service-available? (fn [_ _] (throw (RuntimeException. "foo"))) - :handle-exception (fn [_ _] (throw (RuntimeException. "bar"))))] + (let [res (resource :service-available? (fn [_] (throw (RuntimeException. "error in service-available"))) + :handle-exception (fn [_] (throw (RuntimeException. "error in handle-exception"))))] (fact (res (-> (request :get "/") - (header "Accept" "text/plain"))) => (throws RuntimeException)))) + (header "Accept" "text/plain"))) => (throws #"handle-exception")))) diff --git a/test/test_execution_model.clj b/test/test_execution_model.clj index 3be5430..d8a36e9 100644 --- a/test/test_execution_model.clj +++ b/test/test_execution_model.clj @@ -50,11 +50,24 @@ => (contains {:a 1 :status 404}))) (facts "handler functions" - (fact "keyword as handler" + (fact "handler is a function" (-> (request :get "/") - ((resource :exists? {:some-key "foo"} - :handle-ok :some-key))) - => (contains {:status 200 :body "foo"}))) + ((resource :exists? false + :handle-not-found (fn [ctx] "not found")))) + => (contains {:status 404 :body "not found"})) + (fact "keyword as handler" + (-> (request :get "/") + ((resource :exists? {:some-key "foo"} + :handle-ok :some-key))) + => (contains {:status 200 :body "foo"})) + (fact "default handler uses message key" + (-> (request :get "/") + ((resource :exists? [false {:message "absent"}]))) + => (contains {:status 404 :body "absent"})) + (fact "decisions can override status" + (-> (request :get "/") + ((resource :exists? [false {:status 444 :message "user defined status code"}]))) + => (contains {:status 444 :body "user defined status code"}))) (facts "context merge leaves nested objects intact (see #206)" (fact "using etag and if-match" From 948b02f6b38b01f8ed08c620a6d7e36e9dbcd7e6 Mon Sep 17 00:00:00 2001 From: Philipp Meier Date: Fri, 13 Nov 2015 13:26:06 +0100 Subject: [PATCH 2/2] Add test for options body and workaround for #76 You can actually return custom headers for an error or options handler but it requires some wrestling with as-response and ring-response. --- test/test_response.clj | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/test/test_response.clj b/test/test_response.clj index d92597f..c8d430f 100644 --- a/test/test_response.clj +++ b/test/test_response.clj @@ -83,3 +83,26 @@ ((resource :handle-ok "ok"))) => (fn [c] (not (contains? (:headers c) "Allow")))) ) + + +(facts "Options can return a body" + (fact "return a simple response" + (-> (request :options "/") + ((resource :allowed-methods [:get :options] + :handle-ok "ok" + :handle-options "options"))) + => (body "options")) + (fact "return a ring response" + (let [resp (-> (request :options "/") + ((resource :allowed-methods [:get :options] + :available-media-types ["text/plain" "text/html"] + :handle-ok "ok" + :handle-options (fn [ctx] + ;; workaround until issue #152 is fixed + (-> "options" + (as-response (assoc-in ctx [:representation :media-type] + "text/plain")) + (assoc-in [:headers "X-Foo"] "bar") + (ring-response))))))] + resp => (body "options") + resp) => (header-value "X-Foo" "bar")))