Add filter query string option #29
Conversation
src/ring/logger/messages.clj
Outdated
| (if (and query-string | ||
| (:log-query-string? options)) | ||
| (str uri "?" query-string) | ||
| uri) |
There was a problem hiding this comment.
A minor thing: As we are here, could you please extract uri out from the conditional into a separate line, and change the conditional to something like
(when (and query-string (:log-query-string? options)
(str "?" query-string))`Thanks!
src/ring/logger/messages.clj
Outdated
| :server-port | ||
| :uri]] | ||
| (debug logger (str "Request details: " (select-keys req (if log-query-string? | ||
| default-keys |
There was a problem hiding this comment.
Another minor thing:
I think it would read slightly better to remove query-string from default-keys and then invert this conditional to conj default-keys :query-string
But I'm fine if you prefer to keep it this way :)
There was a problem hiding this comment.
Also, I think the debug is missing some indentation, it seems to be at the same level as the let that wraps it
There was a problem hiding this comment.
Actually I've decided to replace it with cond->, seems to me both more flexible and readable, I hope you are ok with that.
There was a problem hiding this comment.
I also like the cond-> better, thanks!
src/ring/logger/messages.clj
Outdated
| (if (and query-string | ||
| (:log-query-string? options)) | ||
| (str uri "?" query-string) | ||
| uri) |
There was a problem hiding this comment.
same thing about extracting uri and turn if into when
| @@ -123,6 +123,20 @@ | |||
| (->> (map #(nth % 3) @*entries*) | |||
| (filter #(re-find #"secret" %))))))) | |||
|
|
|||
| (deftest basic-ok-request-logging-without-query-params | |||
There was a problem hiding this comment.
Thanks for adding this test!
I don't think we are testing the opposite (and default) case where query-string is logged anywhere else. It would be great if you could add a test to show that query string is logged out when log-query-string? is not set (so it takes the default value). It could have a param from the set of redact-keys, like password for example, to show it goes through unredacted
There was a problem hiding this comment.
I've added new test which covers default settings (log-query-string? not being set)
|
@pmensik thanks for your contribution! I took some time to respond because I'm thinking what direction should the library take, but I think this change is a nice thing to add: if we provide a redact mechanism, we shouldn't log the query params unredacted, without a mechanism to avoid it. I added some comments, I'll wait for your response before merging and making a new release. I'll post an issue soon with my thoughts about the next steps for the library, and this will probably be the last addition (except for bug fixes) on the |
|
Thanks @pmensik. I'll cut a release later today |
I've added an option to disable query string logging based on #27 issue. Please feel free to review it and suggest enhancements. Best regards, Petr