weavejester / compojure

A concise routing library for Ring/Clojure
Eclipse Public License 1.0
4.08k stars 259 forks source link

wrap-routes produces unexpected ordering of middleware #169

Closed RutledgePaulV closed 7 years ago

RutledgePaulV commented 7 years ago

Hi James,

This is likely just a failure on my part to understand how wrap-routes is supposed to work, but I was hoping you could clarify. Usually I define my routes using a thread macro and then applying a series of middleware with the understanding that later middleware receive the request earlier. Now, sometimes I want these middleware to only run if it matches a known route so I use wrap-routes. Once I introduce wrap-routes into the equation, the ordering of my middleware gets rather messed up in a way I wouldn't expect!

Version: 1.5.1

I would expect:

All my non wrap-routes middleware to execute first and then if the route matches, my wrap-routes middleware to execute in the same order as they were defined. However, instead the wrap-routes middleware actually execute in the opposite order compared to the order my other middleware execute in.

I have a small test case to reproduce:

(ns ring-middleware-ordering.core
  (:require [compojure.core :refer :all]
            [ring.util.response :as response]
            [ring.adapter.jetty :as jetty]))

(defroutes application
  (GET "/" request
       (->
         (response/response "Hello, World")
         (response/content-type "text/plain"))))

(defn ordering [handler order]
  (fn [request]
    (println "Before request:" order)
    (let [response (handler request)]
      (println "Before response:" order)
      response)))

(defn wrap-with-middleware [routes]
  (-> routes
      (ordering 1)
      (ordering 2)
      (wrap-routes #(ordering % 3))
      (wrap-routes #(ordering % 4))
      (ordering 5)))

(defn -main [& args]
  (jetty/run-jetty
    (wrap-with-middleware application)
    {:port 3000}))

(-main)

When hitting the route, the above logs:

Before request: 5
Before request: 2
Before request: 1
Before request: 3
Before request: 4
Before response: 4
Before response: 3
Before response: 1
Before response: 2
Before response: 5

I would expect:

Before request: 5
Before request: 2
Before request: 1
Before request: 4
Before request: 3
Before response: 3
Before response: 4
Before response: 1
Before response: 2
Before response: 5

Is this intentional behavior? Would you consider changing this? (I realize that changing ordering of execution of middleware may have subtle downstream impacts).

RutledgePaulV commented 7 years ago

Oops! Looks like this was fixed in https://github.com/weavejester/compojure/pull/157. I'll close and update to 1.6+ :) Thanks for all you do!