ring-clojure / ring-codec

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

Passing nil to decoder decodes string to nil #32

Closed danielcompton closed 3 years ago

danielcompton commented 3 years ago

In 1.1.0, the behaviour of form-decode changed after https://github.com/ring-clojure/ring-codec/commit/34e33290f581475bb4c923e77b52c4860993bb0e.

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?

weavejester commented 3 years ago

Yes, this should be fixed. The behaviour when nil is supplied should be the same as when no argument is supplied, and it's especially important to fix because it's a change from previous behaviour.

weavejester commented 3 years ago

Thanks for the PR. Could you change the commit message to:

Fix nil argument regression in form-decode

Supplying nil as the encoding argument should have the same effect as
omitting the argument. This was the behavior in 1.0.1.

Fixes #32.
danielcompton commented 3 years ago

@weavejester would it be possible to cut a release with this fix? Or push a snapshot?

weavejester commented 3 years ago

I released 1.2.0. Apologies for the delay.