ring-clojure / ring-spec

Clojure specs for Ring
51 stars 7 forks source link

Replace 'fdef' with simpler 'and' spec #7

Closed bhb closed 5 years ago

bhb commented 5 years ago

This fixes an issue I saw with spec errors that didn't appropriately describe the problem when creating a combined sync and async handler. Perhaps there is a reason why the and spec cannot work - please let me know if I've overlooked something!

   ;; Problem: errors are confusing
  (s/explain :ring.sync+async/handler (fn handler [req] {}))
  ;; val: ({:server-port 1, :server-name "", :remote-addr "", :uri "/", :scheme :http, :protocol "HTTP/1.1", :headers {}, :request-method :a}) fails spec: :ring.sync+async/handler predicate: (apply fn),  Wrong number of args (1) passed to: spec/eval2776/handler--2777
  ;; In: [:args 0] val: :sync fails spec: :ring/request at: [:fn :sync :args :request] predicate: map?
  ;; In: [:args 0] val: :sync fails spec: :ring/request at: [:fn :async :args :request] predicate: map?
  ;; In: [:ret] val: [:async {}] fails spec: :ring/response at: [:fn :sync :ret] predicate: map?

  ;; Note the value of 'val' is confusing in the last three errors.
  ;; The issue is that the 'fn' spec receives the *conformed* values
  ;; of arguments. The conformed values will look like [:sync ,,,,] or [:async ,,,]
  (s/conform (s/or :sync  :ring.sync.handler/args :async :ring.async.handler/args)
             [{:headers {}
               :request-method :get
               :protocol ""
               :scheme :http
               :uri "/"
               :remote-addr ""
               :server-name ""
               :server-port 3000}])
  ;; [:sync {:request {:headers {}, :request-method :get, :protocol "", :scheme :http, :uri "/", :remote-addr "", :server-name "", :server-port 3000}}]

  ;; If we replace with a different spec...
  (s/def :ring.sync+async/handler
    (s/and :ring.sync/handler
           :ring.async/handler))

  ;; ... then errors are more intuitive

  (s/explain :ring.sync+async/handler (fn handler [req] {}))
  ;; val: {} fails spec: :ring/response at: [:ret] predicate: (contains? % :status)
  ;; val: {} fails spec: :ring/response at: [:ret] predicate: (contains? % :headers)

  (s/explain :ring.sync+async/handler (fn handler [req] {:status 200
                                                         :headers {}}))

  ;; val: ({:server-port 1, :server-name "", :remote-addr "", :uri "/", :scheme :http, :protocol "HTTP/1.1", :headers {}, :request-method :a} #object[clojure.spec.alpha$fspec_impl$reify__2451$fn__2454 0x781cdd19 "clojure.spec.alpha$fspec_impl$reify__2451$fn__2454@781cdd19"] #object[clojure.spec.alpha$fspec_impl$reify__2451$fn__2454 0x2b0d719c "clojure.spec.alpha$fspec_impl$reify__2451$fn__2454@2b0d719c"]) fails spec: :ring.async/handler predicate: (apply fn),  Wrong number of args (3) passed to: spec/eval2870/handler--2871

  ;; But still avoids invalid handlers
  (s/explain :ring.sync+async/handler (fn handler
                                        ([req] ::invalid)
                                        ([req respond raise] nil)))
  ;; val: :ring.core.spec/invalid fails spec: :ring/response at: [:ret] predicate: map?
weavejester commented 5 years ago

If this works, it seems reasonable. I'll check it over - in the meantime can you remove that extra copyright year commit? It's not related to this PR.

bhb commented 5 years ago

in the meantime can you remove that extra copyright year commit? It's not related to this PR.

Done!

bhb commented 5 years ago

Let me know if there's any additional help I can provide here.

weavejester commented 5 years ago

Sorry for the delay in testing this. It responds fine to the tests I've put this through. Can you fix the indentation on line 172 and 173 and I'll merge it in :)

bhb commented 5 years ago

Sorry for the delay in testing this

No worries, I'm sure you're quite busy!

Can you fix the indentation on line 172 and 173 and I'll merge it in :)

Happy to! Do you prefer a force-push so there is a single commit? Or do you prefer a second commit to fix the whitespace?

weavejester commented 5 years ago

An amend and force push, please :)

bhb commented 5 years ago

Done!