search

weavejester / compojure

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

Add a full-route key to the request map #210

Closed liamchzh closed 2 years ago

liamchzh commented 2 years ago

This provides access to the full route that contains the common prefix. The full-route info is added to the :compojure/full-route key in the request map.

We use :compojure/route to get the route info and add it to the attributes of our metrics and traces, but when compojure.core/context is used, we are not able to get the parameters that are not instantiated. This change adds :compojure/full-route key that serves similarly as the existing :compojure/route does - the only difference is that the new key has a common prefix.

Close #207

liamchzh commented 2 years ago

Hi @weavejester, could you take a look at this PR when you have a chance?

We decided to attach meta info to the compiled route because we don't want to change make-context, as it is public. We could make a more direct map-based implementation if backward compatibility of make-context is not important.

We are not entirely sure what the full syntax is of context, so it's possible we're missing a case to handle in the context-route.

Let me know if you have any comments, thank you!

weavejester commented 2 years ago

This looks rather complex for what it attempts to achieve. I notice that there's already a PR, #202, for adding the context route to the request map. Would that solve the issue?

liamchzh commented 2 years ago

Thanks for the prompt reply. Yes, #202 would solve the issue.

It seems that it's fine to change make-context to pass the path down. If so, the implementation could be simpler. I am also happy to update this PR if 202 couldn't get merged.

atomist[bot] commented 2 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:

liamchzh commented 2 years ago

@weavejester I've updated my PR to pass the path through make-context(as you suggested in #202). The approach looks simpler now.

liamchzh commented 2 years ago

@weavejester Sorry for pestering you again, given that no updates on #202, can we get this PR across the line if you have a chance?

weavejester commented 2 years ago

I got distracted by some other PRs - apologies for letting this one slip. I notice there's some formatting changes that look like they've accidentally crept in. Can you ensure the commit contains only the changes relevant to the feature being implemented?

weavejester commented 2 years ago

It looks good, but it still needs to be separated out into a "full context" PR and "full route" PR, as I mentioned in my previous review.

liamchzh commented 2 years ago

It looks good, but it still needs to be separated out into a "full context" PR and "full route" PR, as I mentioned in my previous review.

Sure thing! I've opened #212. I will update this PR when #212 goes in.

weavejester commented 2 years ago

I've thought about this a little, and I think it's sufficient to have :compojure/route and :compojure/route-context. The full route can be derived from those keys, so we don't need to add it as a key in an of itself.

liamchzh commented 2 years ago

Yes, that's fair. Having the route-context key would address our issue. It's a bit redundant to have a full-route key.

I am happy to close this PR. Thanks!

liamchzh commented 2 years ago

Could you push a new release when you have a chance?