search

ring-clojure / ring-codec

Utility library for encoding and decoding data
MIT License
63 stars 30 forks source link

form-decode : catch-all return nil is bad #22

Open mccraigmccraig opened 5 years ago

mccraigmccraig commented 5 years ago

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

weavejester commented 5 years ago

If I recall, it was for consistency with the other functions, and because most of the time we want to be permissive about bad form data.

I'd be cautiously open to adding new functions with exceptions and possibly deprecating the existing ones. If so, we'd probably want to be consistent and throw parsing errors as well.

mccraigmccraig commented 5 years ago

it could also leave the permissive behaviour but require a non-nil encoding arg - in this case we ended up with a {nil nil} parsed form because encoding was nil and so both the key and value parse threw during form-decode-str

the 1.0.1 behaviour was to default the encoding to "UTF-8", HTTP says it should be " ISO-8859-1" - i'd be happy with defaulting to either of those or throwing an exception - what dyu think ?

i'm happy to do a PR if you want ?

weavejester commented 5 years ago

Throwing an exception on a non-string encoding sounds fine.

Out of interest, where does HTTP say it should be ISO-8859-1?

mccraigmccraig commented 5 years ago

http 1.1 sec 3.7.1 - although that seems only to apply to text/* so perhaps it's really undefined for application/x-www-form-urlencoded data