compojure icon indicating copy to clipboard operation
compojure copied to clipboard

Add the route context to the request map

Open c2nes opened this issue 4 years ago • 7 comments

This provides access to the route prefix as a companion to the existing :compojure/route data allowing the full route to be reconstructed. Since the context macro only accepts a path the new :compojure/context value simply contains the route path instead of the [method route] pair present in :compojure/route.

This data was introduced as a new field in the request path, rather than updating :compojure/route to include the context for the sake of backwards compatibility.

c2nes avatar Aug 25 '21 20:08 c2nes

Commit messages of this repository should follow the seven rules of a great Git commit message, as mentioned in the project's contributing guidelines. It looks like there's a few issues with the commit messages in this pull request:

  • The commit message should not contain markdown.

atomist[bot] avatar Aug 25 '21 20:08 atomist[bot]

Hi @weavejester, sorry to pester, but I saw your recommendation to do so in the project contribution guidelines. Do you think you could give this a look if/when you have a chance? And thank you for building this library!

c2nes avatar Sep 08 '21 13:09 c2nes

Can you provide a little more explanation on the purpose of this PR, and can you also explain why you chose to reconstruct the context from the route.

weavejester avatar Sep 08 '21 13:09 weavejester

I am trying to write reusable middleware functions to report metrics and capture traces for HTTP requests to our Clojure services. When Compojure is used I would like to enrich those metrics and traces to include tags/attributes for the HTTP route. I can use :compojure/route and wrap-routes to get part of the route, but when compojure.core/context is used then :compojure/route only includes the route suffix, while I would like my metrics and tracing middleware to include the full route (i.e. including any prefixes set using compojure.core/context).

There is :context, but this includes the matched URI prefix rather then the route pattern.

c2nes avatar Sep 08 '21 14:09 c2nes

Apologies for the delay in getting back to you. This PR fell through the cracks in my inbox. I think this is overall good, but for efficiency we can pass the path through to the context-request function, rather than doing any string transformation on the route itself. So:

  `(make-context
    ~(context-route path)
    ~path
    (fn [request#]
      (let-request [~args request#]
        (routes ~@routes))))

And:

(context-request request route path)

Also, instead of :compojure/context, perhaps instead :compojure/route-context, to indicate its connection to :compojure/route.

weavejester avatar Apr 28 '22 20:04 weavejester

Would really love to see this added to compojure to allow getting information about the full unparsed path after the route is matched. And avoid workarounds for nested contexts described in https://danlebrero.com/2021/02/03/prometheus-clojure-ring-sql-compojure-reitit/ and implemented in https://github.com/dlebrero/clojure-prometheus-example/blob/8756e5245379f4574830f02c6cb19118cf9a1dc2/src/prometheus_example/handler/example.clj#L26.

How can we help to make it happen?

mrkam2 avatar Aug 15 '23 21:08 mrkam2

I think take the changes made here, apply the suggestions I gave in my previous comment, and we should be good to go.

weavejester avatar Aug 16 '23 07:08 weavejester