search

exoscale / coax

Clojure.spec coercion library for clj(s)
Other
118 stars 4 forks source link

Merged specs are coerced twice, throws error when coercer-fn can't coerce value that already conforms to the spec #14

Closed eerohele closed 3 years ago

eerohele commented 3 years ago

Repro:

(require
  '[clojure.spec.alpha :as spec]
  '[exoscale.coax :as coax])

(spec/def ::x qualified-keyword?)
(coax/def ::x (fn [x _] (keyword "y" x)))

(spec/def ::m1 (spec/keys :opt [::x]))
(spec/def ::mm (spec/merge ::m1))

(coax/coerce ::m1 {::x "quux"}) ;; works
(coax/coerce ::mm {::x "quux"}) ;; throws clojure.lang.Keyword incompatible with java.lang.String

I took a brief look at the code and it looks like gen-coerce-merge coerces twice. That is, it first coerces the value, then tries coercing the already-coerced value, which causes the error. I didn't get further than that, so I'm not sure what exactly is going on.

mpenet commented 3 years ago

Thanks for the report. I will have a look this afternoon.

mpenet commented 3 years ago

Ok I fixed that in https://github.com/exoscale/coax/pull/15, it's indeed a double merge. I think it's a remnant from the previous impl.

On a side note you hit it (and we didn't) because your coercer is not idempotent. But, it's great you did that tho, otherwise the bug would have gone unnoticed, but you might want to make your coercers safe by checking if the value received is not already valid. In your case you would have checked if the val is a qualified-keyword? with namespace "y" otherwise trigger the coercion.

something like:

(coax/def ::x (fn [x _] (cond->> x
                          (not (and (qualified-keyword? x)
                                    (= "y" (namespace x))))
                          (keyword "y"))))
eerohele commented 3 years ago

On a side note you hit it (and we didn't) because your coercer is not idempotent.

Indeed — the coercion function I actually use is idempotent, but I figured it might be best to report this issue anyway to avoid the double coercion.

Thanks for the quick fix! 👍🏻

mpenet commented 3 years ago

My example is even wrong, you might also return a path with :exoscale.coax/invalid if you do not know how to coerce the value ! In most cases you do not care, but in some it matters : ex when s/or comes into play.

mpenet commented 3 years ago

On a side note you hit it (and we didn't) because your coercer is not idempotent.

Indeed — the coercion function I actually use is idempotent, but I figured it might be best to report this issue anyway to avoid the double coercion.

Thanks for the quick fix! 👍🏻

you're welcome, I ll release shortly

mpenet commented 3 years ago

fyi exoscale/coax "1.0.0-alpha12"