ring-codec icon indicating copy to clipboard operation
ring-codec copied to clipboard

form-decode : catch-all return nil is bad

Open mccraigmccraig opened this issue 5 years ago • 4 comments

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

mccraigmccraig avatar May 09 '19 10:05 mccraigmccraig

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.

weavejester avatar May 09 '19 11:05 weavejester

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 ?

mccraigmccraig avatar May 09 '19 11:05 mccraigmccraig

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

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

weavejester avatar May 09 '19 13:05 weavejester

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

mccraigmccraig avatar May 09 '19 16:05 mccraigmccraig