[bug] wrap-nested-params remove namespaces in keywords

[kwladyka]

(def app-stateless
  (-> handler
      (wrap-keyword-params {:parse-namespaces? true})
      ;(wrap-nested-params)
      (wrap-params)
      (add-headers)
      (wrap-restful-format)))

(-> (peridot/session core/app-stateless)
    (peridot/content-type "application/edn")
    (peridot/request "/authentication" :request-method :post
                     :body (pr-str {:action "sign-up"
                                    :user/email "foo@example.com"
                                    :user/password "qwaszx"
                                    :foo {:bar/baz "mee"}})))

params value

What is wrong:

When wrap-nested-params is uncomment it removes all namespaces in 1-deep, but no deeper.
{:action "sign-up", :email "foo@example.com", :password "qwaszx", :foo #:bar{:baz "mee"}}
^ removed :user/, but not :bar/.

When it is commented I get values as I expected:
{:action "sign-up", :user/email "foo@example.com", :user/password "qwaszx", :foo #:bar{:baz "mee"}}

How it should be
It shouldn't remove namespaces.
{:action "sign-up", :user/email "foo@example.com", :user/password "qwaszx", :foo #:bar{:baz "mee"}}

[weavejester]

Thank you for the issue report, but I'm afraid I don't understand your example. You're writing edn to the request body, but you're reporting a bug in form-encoded parameters, for middleware designed for use with form-encoded parameters.

[kwladyka]

This middleware affect body request by removing namespaces. I think it shouldn't. Otherwise developers have to make 2 separate app: first for JSON / EDN, second for POST forms or add middleware separate to each response function.

So if developer want to have responses for FORM inputs and JSON with namespaces in the same app:

1. has to figure out wrap-nested-params is the source of the issue of disappearing namespaces
2. has to remove wrap-nested-params from def app
3. has to add wrap-nested-params separate in each function for handler to response for form request

For now I have plan to use only JSON / GraphQL and maybe EDN only, so this bug is not the issue for me at that moment. But I found it so I reported. Maybe somebody else will have the same issue and will find here solution. Figuring out this wrapper removes namespaces for requests take a while.

For me solution is to just remove this middleware, because I have no plan to use it. It was there because I copy it from another app as default scratch to begin.

[weavejester]

This middleware affect body request by removing namespaces.

It's a little more specific than that. wrap-nested-params uses name to read the keys of the :params map, so your issue is occuring because you have namespaced keywords in your :params map when wrap-nested-params is called.

One workaround is just to rearrange your middleware so that wrap-nested-params and wrap-params are placed more at the top of your middleware chain.

However, wrap-nested-params should ideally ignore non-string keys, as they're not relevant for what it does. I'd accept a PR to fix that.

[yakryder]

However, wrap-nested-params should ideally ignore non-string keys, as they're not relevant for what it does. I'd accept a PR to fix that.

I'll take a quick look at this

[yakryder]

@weavejester When you say ignore non-string keys, you mean not operate on them and pass them along, yes?

[kwladyka]

@weavejester When you say ignore non-string keys, you mean not operate on them and pass them along, yes?

{:foo/bar 1} pass as it is. By ignore he means not modify it.

[yakryder]

@kwladyka Thanks for the confirmation

[yakryder]

Spent a few hours looking at this (I'm new to Clojure, everything takes a while). I won't be doing any more with it. I'm including below my assessment should it prove useful for whoever picks it up.

By my fallible understanding, parse-nested-keys (called by nested-params-request, in turn called by wrap-nested-params) was the most sensible place to make the change. Specifically would recommend adding a string check in the rematches block of the let that destructures to [_ k ks], such that name is still called if param-name is a string and otherwise returned unmodified.

Aside from the unlikely possibility of somebody relying on the current stringifying or namespace-truncating behavior--and maybe the need to benchmark the old and new implementation if there are any doubts about the probably trivial-in-most-to-all-reasonable-contexts performance difference--it seems like just write a few failing tests and drop a string check in parse-nested-keys.

(defn parse-nested-keys
  "Parse a parameter name into a list of keys using a 'C'-like index
  notation.

  For example:
    \"foo[bar][][baz]\"
    => [\"foo\" \"bar\" \"\" \"baz\"]"
  [param-name]
  (println param-name)
  (let [[_ k ks] (re-matches #"(?s)(.*?)((?:\[.*?\])*)" (name param-name)) ; <- Add bare return here if param-name not string
        keys     (if ks (map second (re-seq #"\[(.*?)\]" ks)))]
    (cons k keys)))