weavejester / compojure

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

Add the route context to the request map #202

Open c2nes opened 3 years ago

c2nes commented 3 years ago

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.

atomist[bot] commented 3 years ago

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:

c2nes commented 3 years ago

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!

weavejester commented 3 years ago

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.

c2nes commented 3 years ago

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.

weavejester commented 2 years ago

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.

mrkam2 commented 1 year ago

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?

weavejester commented 1 year ago

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