juxt / yada

A powerful Clojure web library, full HTTP, full async - see https://juxt.pro/yada/index.html
MIT License
735 stars 98 forks source link

:exists? property doesn't return a 404 for HEAD request but does for GET #202

Open danielcompton opened 7 years ago

danielcompton commented 7 years ago

It's quite possible I'm misunderstanding yada or the HTTP spec, but I was surprised by this behaviour:

(deftest put-missing-test
  (let [res (resource
             {:id :put-test
              :properties {:exists? false}
              :methods {:get (fn [ctx]
                               "GET")
                        :post (fn [ctx]
                                "POST")
                        :put (fn [ctx]
                               "PUT")
                        :delete (fn [ctx]
                                  "DELETE")}})]
    (are [method] (= 404 (:status (yada.test/response-for res method)))
      :get
      :head
      :put
      :post
      :delete)))

FAIL in (put-missing-test) (put_resource_test.clj:28)
expected: (= 404 (:status (yada.test/response-for res :head)))
  actual: (not (= 404 200))

FAIL in (put-missing-test) (put_resource_test.clj:28)
expected: (= 404 (:status (yada.test/response-for res :put)))
  actual: (not (= 404 200))

FAIL in (put-missing-test) (put_resource_test.clj:28)
expected: (= 404 (:status (yada.test/response-for res :post)))
  actual: (not (= 404 200))

FAIL in (put-missing-test) (put_resource_test.clj:28)
expected: (= 404 (:status (yada.test/response-for res :delete)))
  actual: (not (= 404 200))

I expected that a resource that didn't exist would return a 404 for PUT/POST/DELETE, but maybe that's an incorrect assumption? I can see that PUT looks for a existence after the method has run.

I'm pretty sure that HEAD is just a bug, see also #201. I've noticed in my logs that HEAD requests are bubbling up unconsumed manifold exceptions, possibly because there isn't a d/catch on the HEAD request protocol implementation?

danielcompton commented 7 years ago

After talking on Slack, I think the only unexpected behaviour is HEAD requests. For the rest it's entirely normal to POST or PUT to a missing resource.

p-himik commented 4 years ago

possibly because there isn't a d/catch on the HEAD request protocol implementation?

Right. But it seems to me that using d/error-deferred everywhere is not really needed - manifold will wrap regular exceptions for you, so you can just throw them. Replacing d/error-deferred with throw everywhere except when d/error-deferred is really needed should automatically fix all such issues.