ring-clojure / ring-codec Goto Github PK

This library doesn't handle nested maps in a format that can be later decoded with form-decode, eg.:

(ring.util.codec/form-encode {:a {:b 1 :c 2}}) ;; "a=b=1&c=2"

(ring.util.codec/form-decode "a=b=1&c=2") ;; {"a" "b=1", "c" "2"}

Specifically it should handle nested params in such a way that it is then parseable by ring.middleware.nested-params eg.:

(ring.util.codec/form-decode "a%5Bb%5D=1&a%5Bc%5D=2") ;; {"a[b]" "1", "a[c]" "2"}

(ring.middleware.nested-params/nested-params-request {:params {"a[b]" "1" "a[c]" "2"}}) ;; {:params {"a" {"b" "1", "c" "2"}}}

Is this something we could fix ?

In 1.1.0, the behaviour of form-decode changed after 34e3329.

1.0.1:

(form-decode "x=y" "UTF-8")
=> {"x" "y"}
(form-decode "x=y")
=> {"x" "y"}
(form-decode "x=y" nil)
=> {"x" "y"}

In 1.1.0 it silently fails to decode and returns a map of nils if you pass a nil encoding:

=> {"x" "y"}
(form-decode "x=y" "UTF-8")
=> {"x" "y"}
(form-decode "x=y")
=> {"x" "y"}
(form-decode "x=y" nil)
=> {nil nil}

Do you think it makes sense to still default to UTF-8 encoding if nil is passed as an encoding? This isn't purely theoretical, I was hit by this in Yada. After upgrading ring-codec (via a ring-mock transitive dependency), form decoding broke.

Arguably Yada should be providing a default encoding, but given this is a regression in behaviour in ring-codec, I thought I'd report it here too.

I'm happy to make a PR to use "UTF-8" if encoding is nil if you'd like?

Functions like form-decode should check for and return nil before doing Java interop.
If you agree on that, I could fix the offending functions.

ring.util.codec/form-encode does not handle nested maps in a way that e.g. ring.middleware.nested-params would be able to parse (cf. #19), but it also does not throw an error when being passed such map. It would be nice if it would handle this case better (cf. #19 (comment)).

the form-decode-str behaviour of catching any Exception, logging nothing and returning nil just caused a painful morning, in combination with the 1.0.1 -> 1.1.0 change of not assuming UTF-8 if the form-decode encoding param is nil

https://github.com/yapsterapp/ring-codec/blob/default-decode-charset/src/ring/util/codec.clj#L131

is there a reason a decode exception is not propagated ? returning nil with no logging seems like the worst of all worlds...

form-encode fails to encode if nil value is in map:

user=> (require '[ring.util.codec :as codec])
user=> (codec/form-encode (codec/form-decode "a&b=1"))
IllegalArgumentException No implementation of method: :form-encode* of protocol: #'ring.util.codec/FormEncodeable found for class: nil clojure.core/-cache-protocol-fn (core_deftype.clj:541)

It would be nicer if nil value treated as is.

user=> (extend-type nil
#=> codec/FormEncodeable
#=> (form-encode* [_ _] ""))
nil
user=> (codec/form-encode (codec/form-decode "a&b=1"))
"b=1&a="

Hello,
I think there's an opportunity for optimization in ring.util.codec/form-decode-str:
In cases where a string does not contain the chars +% it is faster to check if the string contains them than to call form-decode-str for every string size.
The tricky part is that URLDecoder.decode can also change the string's encoding, but if there was a way to guarantee the string is already encoded correctly.
A middle ground I'm less keen on is to just re-encode the string in the case where it doesn't need to be decoded. It's faster than calling URLDecoder.decode but not as fast as possible.

What do you think?

Would you have interest in a pull request that made this work with ClojureScript?

Hey!

Would it make sense to extend FormEncodeable (here) to nil values? I got thinking about it when I tried to mock some requests with malformed values.

The java.util.Base64 class comes with Java 8. Java 7 is no longer publicly supported. I think it would be safe to replace the commons-codec dependency with pure Java: this library only uses it for base64 encoding/decoding.

This has an additional benefit: this library will become compatible with babashka:

(require '[babashka.deps :as deps]
         '[org.httpkit.server :as server])

(deps/add-deps '{:deps {ring/ring {:mvn/version "1.9.0"}
                        ring/ring-codec {:git/url "https://github.com/borkdude/ring-codec"
                                         :sha "5142b1334e1a8a2e5b1a88759b0746c77e33a0b8"
                                         :deps/manifest :deps}}})

(require '[ring.middleware.params :as p])

(server/run-server (p/wrap-params
                    (fn [req]
                      {:body (:query-params req)}))
                   {:port 8090})

@(promise)

http://localhost:8090/?q=1&q=2
;; =>
["q" ["1" "2"]]

I have already applied this change at a fork here: https://github.com/borkdude/ring-codec
and all the tests are passing.

Let me know if you want to receive a PR.

The :body of a ring request is an InputStream. Right now, to decode a urlencoded form, you must redundantly specify the encoding. I would like to remove that requirement and decode the message body directly.

The below examples assume you have populated the :character-encoding on the request. ring-servlet/build-request-map does this for the most common case.

;;; How I use form-decode today
(let [encoding (:character-encoding request)]
  (form-decode (slurp (:body request) :encoding encoding) encoding))

;;; How I would like to use form-decode
(form-decode (:body request) (:character-encoding request))

url-encode and form-encode have very different implementations but seem to do almost the same thing - the only practical difference I have seen is that url-encode does not encode "+" which means it's not suitable for encoding URLs (so I'm not sure what it is useful for!). Note that JavaScript's encodeURIComponent does encode "+"

I'm sure this has caused confusion and possibly coding errors in the past - it would be useful to expand the documentation to explain when you would use one as opposed to the other

We are using namespaced keywords in our query parameters, i.e. ?my.app%2fstatus=archived. This works well from Swagger and from clients, but when we write a Ring Mock test like

(ring.mock.request/request :get "/queries/my-detail" {:my.app/status :archived})

the query string gets coerced to the namespaceless version:

(form-encode {:my.app/status :archived})
=> "status=%3Aarchived"

The root cause is the call to name on the key:

I don't know if there is a safe way to change this behaviour without massively breaking backwards compatibility. I wondered if you were interested in supporting this, perhaps with a dynamic var or other configuration option?

The alternative for us is to cast the keywords to a string ahead of time which works but loses the keywordy goodness.

Currently bashing my head against an issue that appears to have been solved by #8 last year.

Also, you mentioned releasing a 1.0.1 version here which would include that.

So, any chance at one? 🙏

I am relying ring.util.codec/form-decode to parse url query strings.

Today I received for the 1st time a url that has ; as parameter delimiter instead of &.
It also seem to be supported and encouraged by the standard.
http://stackoverflow.com/questions/642758/delimiter-to-use-within-a-query-string-value
http://stackoverflow.com/questions/3481664/semicolon-as-url-query-separator

Specially the latter pointing to:
https://www.w3.org/TR/html401/appendix/notes.html#h-B.2.2

https://github.com/ring-clojure/ring-codec/blob/master/src/ring/util/codec.clj#L131

RFC 3986 defines the plus sign as a reserved character.

url-encode should convert it to %2B

Specifying the encoding format as string incurs a lookup for every call of decode.

We can pull it out to one lookup and an instance check, gaining some more performance in the default cases.

This is an offshoot of #37, but since we agreed to split the changes to separate PRs, I thought having a related issue to track it would be appropriate.

Accessing URLs like

• http://example.com/%s/
• http://example.com/foo%sbar
• http://example.com/%XX

results in a NumberFormatException. This means that our app returns a 500 rather than the expected 404.

Looks like the regex #"%.." here should only match hex digits (#"[0-9A-Fa-f]{2}"), possibly.

The spec (last paragraph of 2.4) seems to say that URLs like the ones above are not valid, because the % should be encoded as %25 when it's not being used for percent-encoding.

So maybe the correct fix is to catch the exception and return a Client Error (e.g. 400 - Bad Request)?