Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
mccraigmccraig opened this issue May 9, 2019 · 4 comments
Open

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

mccraigmccraig opened this issue May 9, 2019 · 4 comments

Comments

@mccraigmccraig
Copy link

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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

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

@mccraigmccraig
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants