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

URLs generated by wrap-trace middleware should be aware of the context #142

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

Conversation

ray1729
Copy link

@ray1729 ray1729 commented Jul 3, 2014

The liberator.dev/wrap-trace middleware was generating URLS at /x-liberator/requests/ which only works when the handler is installed at the root context. This pull request adds support for handlers deployed under any context, and generates URLs relative to the context.

@ordnungswidrig
Copy link
Member

Thanks for the PR. I will have to investigate as I do not yet fully understand the implications of the issue and the PR.

Copy link
Member

@ordnungswidrig ordnungswidrig left a comment

Choose a reason for hiding this comment

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

Thanks for the patch which sounds reasonable. I've left some comment regarding the dynamic binding. Also, I think having some tests would be helpful to ensure no new bugs have been introduced.

[:span (h request-method)] " " [:span (h uri)]]
[:span " at " [:span (h d)] " " [:span "(" (seconds-ago d) "s ago)"]]]) @logs)])])))

(defn css-url [] (str (with-slash mount-url) "styles.css"))
(defn css-url [] (str (with-slash (str *context* mount-url)) "styles.css"))
Copy link
Member

Choose a reason for hiding this comment

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

Instead of dynamic binding I'd prefer an explicit argument.

""])]
[:body
[:a {:href mount-url} "List of all traces"]
[:a {:href (str *context* mount-url)} "List of all traces"]
Copy link
Member

Choose a reason for hiding this comment

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

Use the :context from the request instead and (let [base-url (str (:context request) "/" mount-url))] ...)

@@ -94,10 +102,10 @@
[:ol (map (fn [[l [n r]]] [:li (h l) ": " (h n) " "
(if (nil? r) [:em "nil"] (h (pr-str r)))]) log)]
[:div {:style "text-align: center;"}
[:object {:id "trace" :data (str mount-url "trace.svg") :width "90%"
[:object {:id "trace" :data (str *context* mount-url "trace.svg") :width "90%"
Copy link
Member

Choose a reason for hiding this comment

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

See line 93

@@ -114,11 +122,11 @@
(html5 [:head [:title "Liberator Request Trace #" id " not found."]]
[:body [:h1 "Liberator Request Trace #" id " not found."]
[:p "The requested trace was not found. Maybe it is expired."]
[:p "You can access a " [:a {:href mount-url} "list of traces"] "."]])))
[:p "You can access a " [:a {:href (str *context* mount-url)} "list of traces"] "."]])))
Copy link
Member

Choose a reason for hiding this comment

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

See line 93

@@ -132,11 +140,11 @@
" header in the http response."]]
[:ol (map (fn [[id [d {:keys [request-method uri]} log]]]
[:ul
[:a {:href (h (str (with-slash mount-url) id))}
[:a {:href (h (str (with-slash (str *context* mount-url)) id))}
Copy link
Member

Choose a reason for hiding this comment

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

See line 93

@@ -145,7 +153,7 @@
"Build the url under which the trace information can be found for the
given trace id"
[id]
(str (with-slash mount-url) id))
(str (with-slash (str *context* mount-url)) id))
Copy link
Member

Choose a reason for hiding this comment

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

See line 93

@ordnungswidrig
Copy link
Member

@ray1729 I realize that this PR is a little old, but still valuable. If you don't want to participate in it any more simply let me know and I'm going to clean it up myself :-)

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