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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

liamchzh
Copy link
Contributor

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

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

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

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

atomist bot commented May 2, 2022

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 begin with a capital letter.
  • The commit message subject is over 50 characters.

@liamchzh liamchzh force-pushed the feature/compojure-full-route branch from 6a2a8a7 to f185750 Compare May 2, 2022 20:44
@liamchzh
Copy link
Contributor Author

liamchzh commented May 2, 2022

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

@liamchzh
Copy link
Contributor Author

liamchzh commented May 9, 2022

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

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?

@liamchzh liamchzh force-pushed the feature/compojure-full-route branch from 7295b9c to 2dc42f8 Compare May 10, 2022 18:40
weavejester
weavejester requested changes May 10, 2022
View reviewed changes
Copy link
Owner

@weavejester weavejester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you separate out the context feature into another PR?

src/compojure/core.clj Outdated
@@ -261,7 +269,8 @@
(-> request
(assoc-route-params (decode-route-params params))
(assoc :path-info (if (= subpath "") "/" subpath)
:context (remove-suffix uri subpath))))))
:context (remove-suffix uri subpath))
(update :compojure/context-path (fn [ctx] (str ctx context-path)))))))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be simplified to:

(update :compojure/context-path str context-path)

test/compojure/core_test.clj Outdated
@@ -38,7 +38,7 @@
(assoc :params {:y "bar"}))]
((GET "/:x" [x :as r]
(is (= x "foo"))
(is (= (dissoc r :params :route-params :compojure/route)
(is (= (dissoc r :params :route-params :compojure/route :compojure/full-route)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep lines within 80 characters, please.

src/compojure/core.clj Outdated
@@ -134,9 +134,17 @@
(defn- wrap-route-info [handler route-info]
(fn
([request]
(handler (assoc request :compojure/route route-info)))
(let [full-route (str (:compojure/context-path request) (second route-info))]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in another PR I suggested :compojure/route-context. Also better to use destructuring outside the fn rather than second.

@liamchzh liamchzh force-pushed the feature/compojure-full-route branch from 2dc42f8 to 39c4a7d Compare May 11, 2022 18:05
@liamchzh @calvis
Add a full-route key to the request map
5abd2eb 
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.

Co-authored-by: Liam Chen <[email protected]>
Co-authored-by: Claire Alvis <[email protected]>
@liamchzh liamchzh force-pushed the feature/compojure-full-route branch from 39c4a7d to 5abd2eb Compare May 11, 2022 18:08
@liamchzh liamchzh requested a review from weavejester May 11, 2022 18:08
@weavejester
Copy link
Owner

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 liamchzh mentioned this pull request May 11, 2022
Merged
@liamchzh
Copy link
Contributor Author

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

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

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 liamchzh closed this May 12, 2022
@liamchzh
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weavejester weavejester Awaiting requested review from weavejester

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Extract unparsed path from context
2 participants
@liamchzh @weavejester