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

Add the route context to the request map #202

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

c2nes
Copy link

@c2nes c2nes commented Aug 25, 2021

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
Copy link

atomist bot commented Aug 25, 2021

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.

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 c2nes force-pushed the context-route-in-request-map branch from 8549853 to 5dcf92f Compare August 25, 2021 20:45
@c2nes
Copy link
Author

c2nes commented Sep 8, 2021

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
Copy link
Owner

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
Copy link
Author

c2nes commented Sep 8, 2021

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
Copy link
Owner

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
Copy link

mrkam2 commented Aug 15, 2023

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
Copy link
Owner

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

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

Successfully merging this pull request may close these issues.

3 participants