Question about wrap-route

[rsl...@gmail.com]

Hi,

I'm trying to use the wrap-route function in Noir 1.3.0 Beta 2, but maybe I'm doing something wrong. When I try to wrap a route with a middleware, all routes are wrapped. What am I doing wrong? Here is a sample code of what I'm trying to do:

(ns wraptest.views.welcome

  (:use [noir.core :only [defpage]]

        [noir.server :only [wrap-route]]

        [hiccup.core :only [html]]))

(defpage welcome "/welcome" []

  (html

    [:head

      [:title "wraptest"]]

    [:body

      [:h1 "Wraptest"]

      [:p "Welcome to wraptest"]]))

(defpage start "/start" []

  (html

    [:head

      [:title "wraptest"]]

    [:body

      [:h1 "Wraptest"]

      [:p "Starting a test for wrap-route"]]))

(defn trace-handler [app]

  (fn [request]

    (println "starting request...")

    (app request)

    (println "finishing request...")))

(wrap-route :start trace-handler)

--
Ricardo da Silva Lima
rsl...@gmail.com

[Chris Granger]

It shouldn't be :start (a keyword) it should be the thing you use to reference the route. In this case, that's the symbol start.

(wrap-route start trace-handler)

Cheers,
Chris.

[rsl...@gmail.com]

Hmmm, Well, this doesn't work because route->name will return nil if I pass start instead of :start. Here is the implementation of wrap-route in noir.server.handler ns. The implementation in noir.server simply delegates to this one:

(defn wrap-route [url func & params]



  (swap! wrappers conj {:url (noir/route->name url) :func #(apply func % params)}))

So it essentially calls route->name in the url and adds an entry to the wrappers atom. Well, route->name implementation is:

(defn ^{:skip-wiki true} route->name



  "Parses a set of route args into the keyword name for the route"



  [route]



  (cond



    (keyword? route) route



    (fn? route) (keyword (:name (meta route)))



    :else (let [res (first (parse-route [{} [route]] 'compojure.core/GET))]



            (keyword (:fn-name res)))))

So when I pass a keyword, it returns the keyword. When I pass a function symbol as in (wrap-route start middleware), it will execute 

(keyword (:name (meta route)))

but the metadata is associated with the start var not the function. If you test on the repl, you'll get:

wraptest.server=> (use 'wraptest.views.welcome)

nil

wraptest.server=> (meta start)

nil

wraptest.server=> (meta #'start)

{:ns #<Namespace wraptest.views.welcome>, :name start, :file "wraptest/views/welcome.clj", :line 14, :arglists ([[]]), :noir.core/action compojure.core/GET, :noir.core/url "/start", :noir.core/args []}

Anyway, reading this code, I realized that passing :start should work, but it's still not working because all handlers are wrapped by the middleware and I spent a good amount of time yesterday trying to understand the code and I still can't find what is wrong with it.

[Chris Granger]

If you don't use named routes, you'd just do "start". My suggestion is against using (defpage somename "/somename" ...) unfortunately it doesn't seem to be robust enough for what most people really want to do. Instead, I would use vars of strings if you want named routes:

(def start "/start")

(defpage start []

  )

(wrap-route start trace-handler)

Just to confirm are you restarting the server when you change your middleware? Unfortunately that's one of the only things that requires it. Assuming nothing's stale, I'm not sure why the middleware would apply to more than just your start route.

Cheers,
Chris.

[Nick Bauman]

Chris,

I've been using (defpage somename "/somename" ...) now for a while
and now I hear you say it's not recommended. The look of the (def
start "/start")

(defpage start []
)

is, to me, not as nice. What are the pitfalls of (defpage somename
"/somename" ...) ?

[rsl...@gmail.com]

Well, I tried that with the same result. Weirdly enough, when I try http://localhost:8080/start it actually returns the default 404 page if I wrap the middleware, but the printing in the console happens with every request. http://localhost:8080/welcome gives me the right page, though.

[Chris Granger]

Circular dependencies mostly. Usually the point of using the named routes is for url-for and if you have pages that have links to eachother in them, it's very hard to make that work. While using a set of strings that can exist in some other file you'll remove that issue entirely.

Cheers,
Chris.

[Nick Bauman]

Chris,

"Usually the point of using the named routes is for url-for and if you
have pages that have links to eachother in them, it's very hard to
make that work."

Makes total sense, thank you. What do you think about people solving
this by assiduous/minimal use of (declare ...) at the top of the file?

[Chris Granger]

Ah I think there's a misconception about how middleware works here:

When you access some url, that request goes down through the list of routes and calls the handler function in each case to determine if this route has a response. If it does then that would bubble back up through the middleware stack and move on. If, however, it returns nil, that means that that route wasn't the right one for that url and it continues trying the rest of the routes in the table. In your case, those will be printed every time the wrapped route comes before the one that will actually return a response. 

The simple way to solve this would be to only print if the route returns a response. Right now your middleware forces the response to be nil (since the return of println is always nil).

Cheers,
Chris.

[Chris Granger]

The problem is that won't work across files. You can't declare a var from another NS. One potential solution to this problem is to have url-for use a different mechanism to get the meta information from the route.. perhaps that's a solution I should consider for 1.3.0, though it would require a breaking change - those wouldn't be callable as functions anymore.

Cheers,
Chris.

[Chris Granger]

FWIW, I'm not happy with either of things discussed in here and I think bother merit some rethinking for the 1.3 release. For example, you don't really want the route-specific middleware to wrap around the entire route check, just around the function for a route hit. And the named route thing just doesn't work very well.

I'd be happy to hear some thoughts on solutions to either.

Cheers,
Chris.

On Wed, Apr 11, 2012 at 3:29 PM, Nick Bauman <nick....@gmail.com> wrote:

Good points. I tried writing something that used a different mechanism
but I found that interrogating the routes wasn't easy: ended up with a
40-line function that worked 75% of the time: a sign I was barking up
the wrong tree. Thanks and sorry if I might have hijacked this thread
a bit.

All my efforts came from a desire to have something like
`reverse("route-name")` from Django equivalent a while back. I settled
on url-for with some frobbing and haven't had a problem yet, but I
feel like I will...

[Nick Bauman]

Why not allow a route to have a unique ID as a string for the app?
This would solve the circular dependency problem and keep things
nicely decoupled?

I was actually going to take a crack at this myself, but will hold
back pending your thoughts.

[Nick Bauman]

Good points. I tried writing something that used a different mechanism
but I found that interrogating the routes wasn't easy: ended up with a
40-line function that worked 75% of the time: a sign I was barking up
the wrong tree. Thanks and sorry if I might have hijacked this thread
a bit.

All my efforts came from a desire to have something like
`reverse("route-name")` from Django equivalent a while back. I settled
on url-for with some frobbing and haven't had a problem yet, but I
feel like I will...

[rsl...@gmail.com]

Hmmm,

I thought wrap-route would wrap the middleware only for the particular route passed at least the docstring says so. Anyway I was wondering how could I identify which route the middleware was handling. Essentially, I want to create a security middleware which would check the permissions for some routes, but not for all of them. Any ideas?

[Chris Granger]

That's what pre-routes are for, don't use middleware :)

(pre-route "/admin-area*" []

   (when-not (allowed-to-see-this)

      (redirect-or-whatever)))

Cheers,
Chris.

[rsl...@gmail.com]

Great! Thanks!

[John Szakmeister]

On Wed, Apr 11, 2012 at 3:15 PM, Chris Granger <ibd...@gmail.com> wrote:
> Ah I think there's a misconception about how middleware works here:
>
> When you access some url, that request goes down through the list of routes
> and calls the handler function in each case to determine if this route has a
> response. If it does then that would bubble back up through the middleware
> stack and move on. If, however, it returns nil, that means that that route
> wasn't the right one for that url and it continues trying the rest of the
> routes in the table. In your case, those will be printed every time the
> wrapped route comes before the one that will actually return a response.

That is confusing. I thought the same thing as Ricardo... that
wrap-route was somehow specific to a route. If it get's called like
this, why even pass the route information to the wrap-route call?

Sorry for the dumb question, I'm just hoping to understand the mechanism better.

Thank you!

-John

[Chris Granger]

Not a dumb question at all, it's the result of me not thinking through that completely when I did it. It's a broken implementation and it should be fixed. I'll try and get to it over the next few days, but it'll require some fairly involved changes to the way Noir stores routes. Right now it stores the result of the compojure GET/POST/etc functions, but instead it'll need to store the info needed to do that and compojurify them at the very end so that I can wrap the inner functions in the middleware.

Cheers,
Chris.

[John Szakmeister]

On Tue, May 1, 2012 at 12:10 PM, Chris Granger <ibd...@gmail.com> wrote:
> Not a dumb question at all, it's the result of me not thinking through that
> completely when I did it. It's a broken implementation and it should be
> fixed. I'll try and get to it over the next few days, but it'll require some
> fairly involved changes to the way Noir stores routes. Right now it stores
> the result of the compojure GET/POST/etc functions, but instead it'll need
> to store the info needed to do that and compojurify them at the very end so
> that I can wrap the inner functions in the middleware.

Okay. I just thought I might be missing something. Thanks again for
the explanation! I'm looking forward to the change. Let me know if I
can help in some way!

Thanks!

-John

[rsl...@gmail.com]

Well, I'm still interested in this functionality so I'll be glad to help.

[Juan Antonio Plaza]

Hi Chris,

I'm using noir 1.3.0-beta10, any update on this?