-
Notifications
You must be signed in to change notification settings - Fork 259
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
Add the route context to the request map #202
base: master
Are you sure you want to change the base?
Conversation
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:
|
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.
8549853
to
5dcf92f
Compare
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! |
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. |
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 There is |
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 `(make-context
~(context-route path)
~path
(fn [request#]
(let-request [~args request#]
(routes ~@routes)))) And: (context-request request route path) Also, instead of |
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? |
I think take the changes made here, apply the suggestions I gave in my previous comment, and we should be good to go. |
This provides access to the route prefix as a companion to the existing
:compojure/route
data allowing the full route to be reconstructed. Since thecontext
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.