Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve documentation to include more complex examples #192

Open
jacobemcken opened this issue Jan 2, 2020 · 20 comments
Open

Improve documentation to include more complex examples #192

jacobemcken opened this issue Jan 2, 2020 · 20 comments

Comments

@jacobemcken
Copy link

jacobemcken commented Jan 2, 2020

I don't use Compojure on a regular basis (sometimes with years in between), so I always hit the wiki to find out how it is used. The basics are really intuitive and easy to get up and running. But I always struggle to figure out how the more complex stuff works. Specifically I have two reoccurring issues that I get stuck with every time, where I have to turn to web searches (usually ending up on StackOverflow) to figure it out.

  1. How are you supposed to combine several route definitions.
  2. How to apply middleware on parts of the routes and among others the pitfalls around middleware being applied before the route is matched.

I would love to have the Wiki to be one-stop for documentation... am I the only one?

@weavejester
Copy link
Owner

That's not a bad idea. There isn't much documentation for wrap-routes, which is often used to solve your second problem.

I'm a little confused at why you think 1 is a problem, though, as there's quite a few examples and explanations on how to combine routes with routes.

@jacobemcken
Copy link
Author

jacobemcken commented Jan 4, 2020

I know there is an example (at least one) on how to combine routes. But I'm talking about combining route definitions like the following:

(defroutes finance-routes
  (context "/finance"
    (GET "/accounts" [] ...)
    (POST "/record" [] ...)))

(defroutes billing-routes
  (context "/billing"
    (POST "/invoice" [] ...)
    (GET "/invoice/:invoice-id/lines/" [] ...)))

(defroutes api-routes
  ...)

From experimenting I found that I can combine these with routes as you mentioned:

(routes
  (GET "/health" [] {:status 200})
  finance-routes
  billing-routes
  api-routes)

But for me it wasn't intuitive that I can treat a list of routes (what I've called a "route definition" above) the same as a single route (i.e. the health route).

But to be honest I think my biggest issue here is that I don't have a clear picture of how I should structure everything (routes & context) when it comes to middleware. I look at my code that works and I'm left with this feeling of unease that. Is this really the best way to do it, am I using this lib as it was intended. I would really love to take a look at the Wiki and see... ahh yes that is how it should be done :-)

About wrap-routes ... I would love to see an example of applying multiple pieces of middleware as well.

@weavejester
Copy link
Owner

weavejester commented Jan 4, 2020

Routes are just Ring handler functions that may return nil instead of a response map. A nil response indicates the route did not match.

The routes function combines multiple routes into one. So this:

(defroutes finance-routes
  (context "/finance"
    (GET "/accounts" [] ...)
    (POST "/record" [] ...)))

Is the same as:

(def finance-routes
  (routes
   (context "/finance"
     (GET "/accounts" [] ...)
     (POST "/record" [] ...))))

And because there's only one argument to routes, in this particular case you can just omit it altogether:

(def finance-routes
  (context "/finance"
    (GET "/accounts" [] ...)
    (POST "/record" [] ...))))

If you want to apply middleware to a route, you can sometimes just apply it directly:

(def finance-routes
  (context "/finance"
    (-> (GET "/accounts" [] ...)
        (wrap-some-middleware {:some-option 123}))
    (POST "/record" [] ...))))

Remember that a route is just a Ring handler function that may return nil instead of a response map. So if the middleware has no side effects, and doesn't care about a nil response (or you're sure that there is no nil reponse), you can just apply it directly.

However, it's usually safer to use wrap-routes, which applies the middleware only if the route matches. To use this, you can just add it before any middleware you apply to a particular route:

(def finance-routes
  (context "/finance"
    (-> (GET "/accounts" [] ...)
        (wrap-routes wrap-some-middleware {:some-option 123}))
    (POST "/record" [] ...))))

And of course you can also apply middleware to groups of routes:

(def finance-routes
  (-> (context "/finance"
        (GET "/accounts" [] ...)
        (POST "/record" [] ...))
      (wrap-routes wrap-some-middleware {:some-option 123})))

The wrap-routes function is only needed for routes that may return nil. Your outer route, the one you pass to Ring, should be a valid handler and therefore never return nil. Typically this means adding a "catch-all" 404 not-found route:

(def handler
  (routes
   (GET "/" [] "<h1>Hello World</h1>")
   (route/not-found "<h1>Page not found</h1>")))

Because this is a valid handler, and will never return nil, we know it's safe to apply middleware directly:

(def handler
  (-> (routes
        (GET "/" [] "<h1>Hello World</h1>")
        (route/not-found "<h1>Page not found</h1>"))
      (wrap-some-middleware {:some-option 123})))

Does that make things clearer?

@jacobemcken
Copy link
Author

Yes mostly... what if I want to add multiple middlewares, would it then look like so?

(def finance-routes
  (context "/finance"
    (-> (GET "/accounts" [] ...)
        (wrap-routes wrap-some-middleware {:some-option 123}))
        (wrap-routes wrap-some-other-middleware))
    (POST "/record" [] ...))))

@weavejester
Copy link
Owner

Exactly. Though I should mention it's slightly more efficient to combine the middleware before passing it to wrap-routes, e.g.

(def finance-routes
  (context "/finance"
    (-> (GET "/accounts" [] ...)
        (wrap-routes 
         #(-> %
              (wrap-some-middleware {:some-option 123})
              (wrap-some-other-middleware)))
    (POST "/record" [] ...))))

But in practice it shouldn't make much difference, and using wrap-routes twice looks a little neater.

@jacobemcken
Copy link
Author

Alternatively to #(-> ... sometimes it might make sense to combine the middlewares in a named function and just wrap-route that. I guess it would be a case by case evaluation.

About this:

If you want to apply middleware to a route, you can sometimes just apply it directly:

I've slightly modified your example:

(def finance-routes
  (context "/finance"
    (GET "/accounts/:id" [id] ...)
    (-> (GET "/accounts" [] ...)
        (wrap-some-middleware {:some-option 123}))
    (POST "/record" [] ...))))

Is it correctly understood that wrap-some-middleware isn't applied when the route GET /finance/accounts/:id is matched, but will be applied for anything matching both GET /finance/accounts and POST /finance/record (and anything after)?

@weavejester
Copy link
Owner

weavejester commented Jan 4, 2020

The middleware is triggered when the route is reached, whether or not it matches. If the middleware has no side-effects this can be fine. So for example:

(defn wrap-some-middleware [handler options]
  (fn [request]
    (handler (assoc request ::foo 1))))

This would be fine, because the middleware function doesn't care about the output of the handler and has no side effects. In this case there's no real difference between this and using wrap-routes.

The POST route you have there will see any side-effects triggered by the middleware, but not any changes to the request map (since that's only seen by the wrapped route).

@jacobemcken
Copy link
Author

Would you be interested in me writing a small wiki page about Middleware?

@jacobemcken
Copy link
Author

jacobemcken commented Jan 4, 2020

I want it to highlight the following:

  • How to apply middleware. Both on specific routes/context (using wrap-routes) and global.
  • How to apply multiple middleware.
  • Mention the importance of order (some middleware might expect a different middleware to have already been applied).
  • Mention that middleware is applied even though it wraps a route that isn't matched (unless you use wrap-routes).

@weavejester
Copy link
Owner

I think that would be a good idea, and much appreciated.

@jacobemcken
Copy link
Author

jacobemcken commented Jan 4, 2020

This is my initial take and should give you an idea of what I had in mind:
https://github.com/weavejester/compojure/wiki/Middleware

I thought it would fit in the list on the index page between Nesting Routes and Response Rendering. What do you think?

I'm going to read through it again tomorrow for those obvious mistakes that usually hides in the first round.

I looked for an opening to insert a link to https://github.com/ring-clojure/ring/wiki/Concepts#middleware somewhere but I couldn't find a god place (and sentence) for it.

Also you mentioned something about valid handlers never returning nil. Not sure if it should go in or if it is just going to confuse people (like me not knowing much about Ring and hoping not having to).

Feel free to modify. 😁

@weavejester
Copy link
Owner

weavejester commented Jan 4, 2020

Thanks 😃 . I updated the page and updated the explanations to be more accurate and examples to be a little more concrete.

@jacobemcken
Copy link
Author

jacobemcken commented Jan 5, 2020

Look good thanks 👍 😄

@jacobemcken
Copy link
Author

Just a note. You wrote:

... either because it has side effects or cannot deal with a nil response ...

I gives me the feeling that these are the only two valid reasons to use wrap-routes, and I don't see my reasons fit into that.

My use cases where:

  1. Enforce having a (valid) Bearer token in the Authorization header on API routes only.
  2. Only enable ring.middleware.multipart-params/wrap-multipart-params on that single route that needed support for file uploads.

My reasoning might be stupid but I'm just trying to give context on why it confuses me that the reasoning in the Wiki seems specific and technical. Couldn't business requirements also warrant use of specific middleware on specific routes and other technical reasons?

I guess my real question underneath it all is: Should I solve my above use cases in a different way?

@weavejester
Copy link
Owner

In both those cases you're likely dealing with side-effects. Parsing multipart parameters involves consuming the body input stream, which is side-effectful, and while enforcing a bearer token isn't necessarily side-effectful, it likely involves choosing whether to execute a side-effectful route.

However, I admit it could definitely be clearer! I'll consider how to reword it to make the use of wrap-routes more clear.

@jacobemcken
Copy link
Author

Maybe just replace either with usually... it loosens up the sentence.

Also consider this. When I read the following sentence I understood the word it as referring back to the middleware.

... either because it has side effects ...

Now in your above comment you write:

... it likely involves choosing whether to execute a side-effectful route.

Does that mean wrap-routes should be used in both cases if either the middleware triggers side effects or the route could be a side-effectful route. (My token verification doesn't have state but the routes "behind" it deffo will.

@n2o
Copy link

n2o commented Jan 29, 2021

This issue helped me a lot in understanding and working with wrap-routes. Maybe we could just link this to the Wiki until there are more information about wrap-routes etc? Took me a while to find this issue :D

@devurandom
Copy link

It appears to me that wrap-routes does not work well with context or nested routes.

When http get http://localhost:5010/api/resource is requested from a server running the following code:

(defn- wrap-compojure-route
  [handler]
  (compojure/wrap-routes handler (fn [route-handler]
                                   (fn [request]
                                     (log/infof "ROUTE: %s" (-> request :compojure/route second))
                                     (route-handler request)))))

(defn resource-handler [request]
  (log/infof "REQUEST: %s" request)
  {:status 200})

(def route-handler
  (wrap-compojure-route
    (compojure/routes
      (compojure/context "/api" []
        (compojure/GET "/resource" [] #'resource-handler)))))

(defn start-server []
  (jetty/run-jetty route-handler {:port 5010 :join? false}))

It logs ROUTE: /resource instead of ROUTE: /api/resource as I would expect. Is this correct? Am I using wrap-routes, routes or context wrongly?

@weavejester
Copy link
Owner

:compojure/route contains the route; :compojure/route-context contains the context. You should be able to concatenate these two pieces together to get the full route, context included.

@devurandom
Copy link

devurandom commented Dec 12, 2023

Thanks! This works:

(defn- wrap-compojure-route
  [handler]
  (compojure/wrap-routes handler (fn [route-handler]
                                   (fn [request]
                                     (let [{:compojure/keys [route route-context]} request]
                                       (log/infof "ROUTE: %s" (str route-context (second route))))
                                     (route-handler request)))))

(defn resource-handler [request]
  (log/infof "REQUEST: %s" request)
  {:status 200})

(def route-handler
  (wrap-compojure-route
   (compojure/routes
    (compojure/context "/api" []
      (compojure/GET "/resource" [] #'resource-handler)))))

(defn start-server []
  (jetty/run-jetty #'route-handler {:port 5010 :join? false}))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants