Skip to content

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

Open
ray1729 wants to merge 1 commit into
clojure-liberator:masterfrom
ray1729:context-aware-wrap-trace
Open

URLs generated by wrap-trace middleware should be aware of the context#142
ray1729 wants to merge 1 commit into
clojure-liberator:masterfrom
ray1729:context-aware-wrap-trace

Conversation

@ray1729

@ray1729 ray1729 commented Jul 3, 2014

Copy link
Copy Markdown

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
Copy Markdown
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.

@ordnungswidrig ordnungswidrig left a comment

Copy link
Copy Markdown
Member

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.

Comment thread src/liberator/dev.clj
[: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
Copy Markdown
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.

Comment thread src/liberator/dev.clj
""])]
[:body
[:a {:href mount-url} "List of all traces"]
[:a {:href (str *context* mount-url)} "List of all traces"]

Copy link
Copy Markdown
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))] ...)

Comment thread src/liberator/dev.clj
(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
Copy Markdown
Member

Choose a reason for hiding this comment

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

See line 93

Comment thread src/liberator/dev.clj
[: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
Copy Markdown
Member

Choose a reason for hiding this comment

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

See line 93

Comment thread src/liberator/dev.clj
[: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
Copy Markdown
Member

Choose a reason for hiding this comment

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

See line 93

Comment thread src/liberator/dev.clj
given trace id"
[id]
(str (with-slash mount-url) id))
(str (with-slash (str *context* mount-url)) id))

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